Fix: Update Deployment Secrets Processing UX #467
Conversation
WalkthroughRefactors deploy command to remove dry-run/no-secrets, add non-interactive semantics, and enforce real API usage. Introduces deployed-secrets workflow, revamped secret processing (deployment vs user secrets, reuse/skip), expanded UX headers/summaries, and tighter error handling. Updates tests to mock deployments and align with new secrets pipeline; removes legacy secrets tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as deploy_config
participant FS as get_config_files
participant SP as Secrets Processor
participant UX as UX headers
participant API as MCPAppClient
participant WR as wrangler_deploy
User->>CLI: Invoke deploy (opts: --non-interactive, --api-url, --api-key)
CLI->>FS: Discover config, secrets, deployed_secrets
FS-->>CLI: Paths (config_file, secrets_file, deployed_secrets_file?)
CLI->>UX: print_configuration_header / print_deployment_header
alt deployed_secrets_file present
CLI->>SP: process using existing deployed secrets (reuse/validate)
else
CLI->>SP: process secrets.yaml -> deployed.secrets.yaml
end
SP-->>CLI: {deployment_secrets, user_secrets, reused, skipped}
CLI->>API: get_app_id_by_name(app_name)
alt App exists
CLI->>User: Confirm update? (skipped if non-interactive)
opt Update approved
CLI->>API: update app metadata
end
else
CLI->>API: create_app(app_name, description)
end
API-->>CLI: appId
CLI->>WR: wrangler_deploy(bundle, appId, api creds)
WR-->>CLI: appId
CLI-->>User: Return appId
sequenceDiagram
autonumber
participant SP as Secrets Processor
participant SC as SecretsClient
participant UX as UX summary
note over SP: Input: raw secrets YAML (+ optional existing deployed.secrets)
loop Traverse config (dicts/lists)
alt Non-interactive
SP->>SC: create_secret(value)
SC-->>SP: handle (UUID)
SP-->>SP: record deployment_secret
else Interactive
SP->>User: Prompt: Deployment vs User Secret
alt Deployment
SP->>SC: create_secret(value)
SC-->>SP: handle (UUID)
SP-->>SP: record deployment_secret
else User Secret
SP-->>SP: tag as !user_secret (value omitted)
end
end
opt Existing transformed present
SP-->>SP: Reuse handle if consistent
end
opt Empty/invalid
SP-->>SP: mark skipped, record error
end
end
SP->>UX: print_secrets_summary(deployment, user, reused, skipped)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rholinshead
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (17)
src/mcp_agent/cli/secrets/processor.py (4)
52-63: Doc terminology drift: “developer secret” → “deployment secret”Align docs with the new UX to avoid confusion.
- - If non-interactive is True, automatically transforms all secrets to - developer secrets without prompting, reusing existing secrets where applicable + - If non-interactive is True, automatically transforms all secrets to + deployment secrets without prompting, reusing existing secrets where applicable - - Prompts to determine whether a secret is a developer secret to transform + - Prompts to determine whether a secret is a deployment secret to transformAlso applies to: 70-71
180-185: Type hint: use concrete mutable types for context listsThese collections are mutated (append). Sequence is misleading.
- secrets_context: Dict[str, Sequence] = { + secrets_context: Dict[str, List] = {
304-318: Existing-secrets reuse misses list values (arrays)Validation skips list-typed nodes, so array-based secrets can’t be reused. Add list handling to preserve reuse in arrays.
elif isinstance(existing_value, dict): # Always recursively process nested dictionaries input_dict = ( input_config.get(key, {}) if isinstance(input_config.get(key), dict) else {} ) nested_validated = await get_validated_config_secrets( input_dict, existing_value, client, non_interactive, current_path ) if nested_validated: validated_config[key] = nested_validated + elif isinstance(existing_value, list): + # Validate lists element-by-element to support paths like "foo[0].bar" + input_list = input_config.get(key, []) if isinstance(input_config.get(key), list) else [] + validated_list: List[Any] = [] + for idx, ev in enumerate(existing_value): + # Recurse only for dict/list elements; primitives can’t be validated here + if isinstance(ev, (dict, list)): + nested = await get_validated_config_secrets( + input_list[idx] if idx < len(input_list) and isinstance(input_list[idx], type(ev)) else {}, + ev, + client, + non_interactive, + f"{current_path}[{idx}]", + ) + validated_list.append(nested if nested else ({} if isinstance(ev, dict) else [])) + else: + validated_list.append(None) + if any(item not in (None, {}, []) for item in validated_list): + validated_config[key] = validated_list
509-513: Treat whitespace-only values as emptyPrevents creating secrets with “blank” values.
- if config_value is None or config_value == "": + if config_value is None or (isinstance(config_value, str) and config_value.strip() == ""): raise ValueError( f"\nSecret at {path} has no value. Deployment secrets must have values." )src/mcp_agent/cli/utils/ux.py (1)
111-115: Avoid mutable default argumentsUse None defaults and initialize locally.
-def print_secrets_summary( - deployment_secrets: List[Dict[str, str]], - user_secrets: List[str], - reused_secrets: Optional[List[Dict[str, str]]] = [], - skipped_secrets: Optional[List[str]] = [], -) -> None: +def print_secrets_summary( + deployment_secrets: List[Dict[str, str]], + user_secrets: List[str], + reused_secrets: Optional[List[Dict[str, str]]] = None, + skipped_secrets: Optional[List[str]] = None, +) -> None: @@ - reused_paths = ( - {secret["path"] for secret in reused_secrets} if reused_secrets else set() - ) - skipped_paths = set(skipped_secrets) if skipped_secrets else set() + reused_secrets = reused_secrets or [] + skipped_secrets = skipped_secrets or [] + reused_paths = {secret["path"] for secret in reused_secrets} + skipped_paths = set(skipped_secrets) @@ - reused_count = len(reused_secrets) - new_deployment_count = len(deployment_secrets) + reused_count = len(reused_secrets) + new_deployment_count = len(deployment_secrets)tests/cli/commands/test_deploy_command.py (6)
72-77: Also assert removed flags are absentGuard against regressions by asserting deprecated flags (e.g., --dry-run, --no-secrets) are not shown.
assert "--non-interactive" in clean_text + assert "--dry-run" not in clean_text + assert "--no-secrets" not in clean_text
120-133: Pass str(config_dir) to the CLI runnerClick/Typer expects strings; passing Path can be flaky across environments.
- temp_config_dir, + str(temp_config_dir),
135-143: Consider verifying transformed file contents tooYou already check the status line. Also assert the file actually contains the mocked payload to catch write path issues.
- assert "Transformed secrets file written to" in result.stdout + assert "Transformed secrets file written to" in result.stdout + out_path = temp_config_dir / MCP_DEPLOYED_SECRETS_FILENAME + with open(out_path, "r", encoding="utf-8") as f: + assert "test: value" in f.read()
171-189: Make this test fully hermetic: also patch process_config_secretsWithout patching, process_config_secrets could make network calls (api_url=http://test.api/). Patch it as in the first test.
- with ( - patch( - "mcp_agent.cli.cloud.commands.deploy.main.wrangler_deploy", - return_value=MOCK_APP_ID, - ), - patch( - "mcp_agent.cli.cloud.commands.deploy.main.MCPAppClient", - return_value=mock_client, - ), - ): + with ( + patch( + "mcp_agent.cli.cloud.commands.deploy.main.wrangler_deploy", + return_value=MOCK_APP_ID, + ), + patch( + "mcp_agent.cli.cloud.commands.deploy.main.MCPAppClient", + return_value=mock_client, + ), + patch( + "mcp_agent.cli.secrets.processor.process_config_secrets", + side_effect=lambda **kw: (Path(kw["output_path"]).write_text("# transformed\n"), {"deployment_secrets": [], "user_secrets": [], "reused_secrets": [], "skipped_secrets": []})[1], # noqa: E501 + ), + ):
201-214: Also assert deployed file content and branch coverage for both-files-present
- Add simple content assertion on mcp_agent.deployed.secrets.yaml.
- Consider another test covering: both secrets files present + non_interactive=True → no reprocessing message.
19-19: Import deploy_config from deploy.main to avoid re-export couplingdeploy_config is defined in src/mcp_agent/cli/cloud/commands/deploy/main.py and is re-exported from src/mcp_agent/cli/cloud/commands/init.py; change the test import at tests/cli/commands/test_deploy_command.py:19 to:
from mcp_agent.cli.cloud.commands.deploy.main import deploy_configsrc/mcp_agent/cli/cloud/commands/deploy/main.py (6)
69-74: Track the dry‑run comeback with an issue IDYou’ve left a TODO. Link an issue or add a clear follow‑up to avoid bit‑rot.
- # TODO(@rholinshead): Re-add dry-run and perform pre-validation of the app + # TODO(@rholinshead): Re-add dry-run and perform pre-validation of the app (see ISSUE-XXXX)
152-166: Fix grammar and cancellation semantics
- Prompt grammar: “Do you want to deploy an update…”
- Returning app_id on cancel is surprising for a function documented to return the newly deployed app ID. Consider raising typer.Exit or returning None.
- use_existing = typer.confirm( - f"Do you want deploy an update to the existing app ID: {app_id}?" - ) + use_existing = typer.confirm( + f"Do you want to deploy an update to the existing app ID: {app_id}?" + ) @@ - print_error( - "Cancelling deployment. Please choose a different app name." - ) - return app_id + print_error("Cancelling deployment. Please choose a different app name.") + # Consider: raise typer.Exit() or return None + return app_id
203-205: Header before processing is fine, but consider refreshing after re‑processingIf you reprocess secrets, printing a second header with the concrete deployed file path would improve UX.
210-213: Prefer Path composition over f‑string for pathsCleaner and OS‑agnostic.
- secrets_transformed_path = Path( - f"{config_dir}/{MCP_DEPLOYED_SECRETS_FILENAME}" - ) + secrets_transformed_path = config_dir / MCP_DEPLOYED_SECRETS_FILENAME
214-222: Pass client optionally to aid testing (dependency injection)Allowing an optional SecretsClient would simplify tests and reduce patching surface.
- process_config_secrets( + process_config_secrets( input_path=secrets_file, output_path=secrets_transformed_path, api_url=effective_api_url, api_key=effective_api_key, non_interactive=non_interactive, )Follow‑up: add a client: Optional[SecretsClient] param to deploy_config and thread it through when provided.
283-291: Use typing.Tuple for broader Python compatibility or confirm min versiontuple[...] requires Python 3.9+. If you support 3.8, switch to typing.Tuple; otherwise, note the min Python in packaging.
-from typing import Optional +from typing import Optional, Tuple @@ -def get_config_files(config_dir: Path) -> tuple[Path, Optional[Path], Optional[Path]]: +def get_config_files(config_dir: Path) -> Tuple[Path, Optional[Path], Optional[Path]]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/mcp_agent/cli/cloud/commands/deploy/main.py(8 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(1 hunks)src/mcp_agent/cli/core/utils.py(1 hunks)src/mcp_agent/cli/secrets/processor.py(12 hunks)src/mcp_agent/cli/utils/ux.py(3 hunks)tests/cli/commands/test_cli_secrets.py(0 hunks)tests/cli/commands/test_deploy_command.py(5 hunks)tests/cli/secrets/test_developer_secret_validation.py(0 hunks)tests/cli/secrets/test_secrets_transform.py(12 hunks)
💤 Files with no reviewable changes (2)
- tests/cli/commands/test_cli_secrets.py
- tests/cli/secrets/test_developer_secret_validation.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
PR: lastmile-ai/mcp-agent#0
File: examples/usecases/reliable_conversation/CLAUDE.md:0-0
Timestamp: 2025-07-22T18:59:49.368Z
Learning: Applies to examples/usecases/reliable_conversation/examples/reliable_conversation/src/utils/config.py : Configuration values such as quality_threshold, max_refinement_attempts, consolidation_interval, and evaluator_model_provider must be loaded from mcp_agent.config.yaml.
Applied to files:
src/mcp_agent/cli/cloud/commands/deploy/main.py
🧬 Code graph analysis (5)
tests/cli/commands/test_deploy_command.py (3)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
deploy_config(41-280)src/mcp_agent/cli/mcp_app/api_client.py (2)
get_app_id_by_name(285-307)create_app(133-163)src/mcp_agent/cli/mcp_app/mock_client.py (2)
get_app_id_by_name(36-45)create_app(86-124)
tests/cli/secrets/test_secrets_transform.py (6)
src/mcp_agent/cli/secrets/mock_client.py (2)
create_secret(30-68)get_secret_value(70-85)src/mcp_agent/cli/secrets/api_client.py (2)
create_secret(15-70)get_secret_value(72-98)tests/cli/fixtures/mock_secrets_client.py (2)
create_secret(26-63)get_secret_value(65-84)src/mcp_agent/cli/core/constants.py (1)
SecretType(35-39)src/mcp_agent/cli/secrets/processor.py (1)
transform_config_recursive(321-534)src/mcp_agent/cli/secrets/yaml_tags.py (3)
UserSecret(24-27)DeveloperSecret(30-33)load_yaml_with_secrets(97-107)
src/mcp_agent/cli/cloud/commands/deploy/main.py (4)
src/mcp_agent/cli/utils/ux.py (3)
print_error(80-91)print_info(32-49)print_deployment_header(185-208)src/mcp_agent/cli/core/api_client.py (1)
UnauthenticatedError(9-12)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/secrets/processor.py (1)
process_config_secrets(41-152)
src/mcp_agent/cli/secrets/processor.py (5)
src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/utils/ux.py (3)
print_info(32-49)print_warning(66-77)print_error(80-91)src/mcp_agent/cli/secrets/api_client.py (2)
get_secret_value(72-98)create_secret(15-70)src/mcp_agent/cli/secrets/yaml_tags.py (2)
DeveloperSecret(30-33)UserSecret(24-27)src/mcp_agent/cli/core/constants.py (1)
SecretType(35-39)
src/mcp_agent/cli/utils/ux.py (2)
src/mcp_agent/app.py (1)
logger(193-210)src/mcp_agent/logging/logger.py (1)
info(271-279)
🔇 Additional comments (13)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
12-14: Import formatting only — LGTMNo behavior change. Safe to merge.
src/mcp_agent/cli/core/utils.py (1)
30-33: Signature reflow — LGTMPurely stylistic; no functional impact.
src/mcp_agent/cli/utils/ux.py (2)
100-107: Context-to-UI glue — LGTMMapping to the new keys (deployment_secrets, reused, skipped) is correct.
185-208: Header updates — LGTMNew app_name/app_id context is clear; logs show both secrets files.
tests/cli/secrets/test_secrets_transform.py (3)
56-83: Interactive deployment secret path — LGTMCovers handle creation and value propagation correctly.
196-211: Empty value behavior test — LGTMMatches skip-on-empty semantics.
334-451: Reuse path coverage — LGTMValidates reuse, exclusion, and mixed user/deployment outputs.
tests/cli/commands/test_deploy_command.py (2)
84-95: Good: hermetic secrets processing via patched process_config_secrets
105-118: Patch targets look rightPatching process_config_secrets, MCPAppClient, and wrangler_deploy at their use sites keeps the test hermetic.
src/mcp_agent/cli/cloud/commands/deploy/main.py (4)
65-68: Non‑interactive help text is clearReads well and matches behavior. No change needed.
91-96: Docstring aligns with the deployed‑secrets flowAccurately describes mcp_agent.deployed.secrets.yaml creation and use.
120-129: Good explicit validation for API URL and keyClear error messages; no secret leakage in logs.
299-311: get_config_files is straightforward and robustClear errors and explicit returns. LGTM.
| elif isinstance(config_value, list): | ||
| # Process each item in the list | ||
| result_list = [] | ||
| for i, value in enumerate(config_value): | ||
| new_path = f"{path}[{i}]" if path else f"[{i}]" | ||
| result_list.append( | ||
| await transform_config_recursive( | ||
| value, | ||
| client, | ||
| new_path, | ||
| non_interactive, | ||
| secrets_context, | ||
| existing_config, | ||
| ) | ||
| ) | ||
| return result_list | ||
|
|
There was a problem hiding this comment.
Uncaught exceptions in list branch abort processing
A bad element in a list will bubble up and cancel the whole transform. Mirror the dict-branch try/except and track skipped paths.
- elif isinstance(config_value, list):
- # Process each item in the list
- result_list = []
- for i, value in enumerate(config_value):
- new_path = f"{path}[{i}]" if path else f"[{i}]"
- result_list.append(
- await transform_config_recursive(
- value,
- client,
- new_path,
- non_interactive,
- secrets_context,
- existing_config,
- )
- )
- return result_list
+ elif isinstance(config_value, list):
+ # Process each item in the list with per-item fault isolation
+ result_list = []
+ for i, value in enumerate(config_value):
+ new_path = f"{path}[{i}]" if path else f"[{i}]"
+ try:
+ item = await transform_config_recursive(
+ value,
+ client,
+ new_path,
+ non_interactive,
+ secrets_context,
+ existing_config,
+ )
+ result_list.append(item)
+ except Exception as e:
+ print_error(f"\nError processing secret at '{new_path}': {str(e)}\n Skipping this secret.")
+ if "skipped_secrets" not in secrets_context:
+ secrets_context["skipped_secrets"] = []
+ secrets_context["skipped_secrets"].append(new_path)
+ # Skip this element entirely
+ continue
+ return result_list📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif isinstance(config_value, list): | |
| # Process each item in the list | |
| result_list = [] | |
| for i, value in enumerate(config_value): | |
| new_path = f"{path}[{i}]" if path else f"[{i}]" | |
| result_list.append( | |
| await transform_config_recursive( | |
| value, | |
| client, | |
| new_path, | |
| non_interactive, | |
| secrets_context, | |
| existing_config, | |
| ) | |
| ) | |
| return result_list | |
| elif isinstance(config_value, list): | |
| # Process each item in the list with per-item fault isolation | |
| result_list = [] | |
| for i, value in enumerate(config_value): | |
| new_path = f"{path}[{i}]" if path else f"[{i}]" | |
| try: | |
| item = await transform_config_recursive( | |
| value, | |
| client, | |
| new_path, | |
| non_interactive, | |
| secrets_context, | |
| existing_config, | |
| ) | |
| result_list.append(item) | |
| except Exception as e: | |
| print_error(f"\nError processing secret at '{new_path}': {str(e)}\n Skipping this secret.") | |
| if "skipped_secrets" not in secrets_context: | |
| secrets_context["skipped_secrets"] = [] | |
| secrets_context["skipped_secrets"].append(new_path) | |
| # Skip this element entirely | |
| continue | |
| return result_list |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/secrets/processor.py around lines 388-404, the list branch
currently lets exceptions from a single element abort the whole transform;
mirror the dict-branch behavior by wrapping each per-item transform call in a
try/except, on exception record the failing path (e.g. ensure secrets_context
has a skipped_paths list and append new_path) and append a placeholder (None or
the original value) to result_list so processing continues, and only re-raise
when the same non-interactive semantics require it (follow the dict-branch logic
for logging and control flow).
|
Going to merge this in so I can test it! |
* WIP starting point * Reorder app creation and prompt to redeploy * Update secrets processing * Track skipped secrets * Improve messaging with path * Fix validation test * Transform tests * Clean up tests
Description
Update the deployment secrets processing UX to respect
mcp_agent.secrets.yamlas a "readonly" artifact, dropping the manual custom secrets tag / environment variable handling in favour of raw secrets transformation into themcp_agent.deployed.secrets.yaml. This also replaces the 'developer secret' naming with 'deployment secret' to be clearer in its use.More specifically, this means:
!developer_secretor!user_secrettags, or specify env var names for such secrets; they use their secrets file as prescribed bymcp-agentwithout special handling for cloudmcp_agent.deployed.secrets.yamlexists; if so, prompt if it should be reprocessed (if not, it will be used)--non-interactiveflag and the command will either just use the existing deployed secrets file as-is (if it exists) or create new deployment secrets for all secrets in themcp_agent.deployed.secrets.yamlfileRemoved some redundant tests and added some new ones for the transformation logic update. Removed --dry-run flag for now since it was broken. We should consider adding a new --dry-run implementation which spins up a local mcp app server and does a test list_tools call over sse.
Testing
Ran through the various flows and ensured secrets handling works as expected
Summary by CodeRabbit
New Features
Refactor