Allow env to be saved as secrets, add new app-scoped secrets command and materialize an mcp_agent.deployed.config.yaml #601
Conversation
…, and materialize a deployed.config.yaml
WalkthroughThis PR adds environment-variable capture and management to cloud deployments: a new Settings.env field and validation, materialization of deployment artifacts (mcp_agent.deployed.secrets.yaml and mcp_agent.deployed.config.yaml) via a SecretsClient during deploy, post-deploy CLI secrets commands (list/add/remove/pull), and runtime population of os.environ from declared env specs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeployCLI as Deploy CLI
participant Config as Settings Loader
participant Materialize as Materialize Module
participant SecretsAPI as Secrets API
participant FS as File System
User->>DeployCLI: mcp-agent deploy
DeployCLI->>Config: load mcp_agent.config.yaml
Config-->>DeployCLI: Settings (env specs)
DeployCLI->>Materialize: materialize_deployment_artifacts(...)
Materialize->>Config: iter_env_specs()
Config-->>Materialize: [("KEY", fallback?), ...]
Materialize->>Materialize: resolve from os.environ or fallback
Materialize->>SecretsAPI: create_secret / set_secret_value
SecretsAPI-->>Materialize: secret handle
Materialize->>FS: write mcp_agent.deployed.secrets.yaml
Materialize->>FS: write mcp_agent.deployed.config.yaml
Materialize-->>DeployCLI: deployed paths
DeployCLI->>User: deployment complete
sequenceDiagram
participant User
participant SecretsCLI as cloud secrets
participant SecretsAPI as Secrets API
participant FS as File System
User->>SecretsCLI: cloud secrets list
SecretsCLI->>SecretsAPI: list handles
SecretsAPI-->>SecretsCLI: handles
SecretsCLI-->>User: masked table
User->>SecretsCLI: cloud secrets add KEY VALUE
SecretsCLI->>SecretsAPI: create or update secret
SecretsAPI-->>SecretsCLI: success
User->>SecretsCLI: cloud secrets pull
SecretsCLI->>SecretsAPI: fetch values
SecretsAPI-->>SecretsCLI: values
SecretsCLI->>FS: write mcp_agent.cloud.secrets.yaml
FS-->>User: file written
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/config/test_env_settings.py (1)
6-17: Test coverage is adequate for basic scenarios.The tests correctly validate the core functionality of
iter_env_specsand empty string rejection. However, consider adding test cases for additional edge cases to strengthen validation:
- Dict with multiple keys (should raise ValueError per validation logic)
- Non-string/non-dict entries in the env list
- Dict with empty key (whitespace-only after strip)
- Empty dict
{}Example additions:
def test_env_validation_rejects_dict_with_multiple_keys(): with pytest.raises(ValueError, match="exactly one key-value pair"): Settings(env=[{"KEY1": "val1", "KEY2": "val2"}]) def test_env_validation_rejects_invalid_types(): with pytest.raises(ValueError, match="must be specified as strings or single-key mappings"): Settings(env=[123]) def test_env_validation_rejects_whitespace_only_key(): with pytest.raises(ValueError, match="must be non-empty strings"): Settings(env=[" "])src/mcp_agent/cli/cloud/commands/secrets/main.py (2)
148-199: Consider validating environment variable key format.The command correctly handles create/update logic and provides clear feedback. However, it doesn't validate that
keyis a valid environment variable name.Consider adding validation after Line 150:
def add_secret( key: str = typer.Argument(..., help="Environment variable to store as a secret"), value: str = typer.Argument(..., help="Secret value to store"), # ... rest of parameters ) -> None: """Create or update an environment secret.""" # Validate key format if not key or not key.strip(): raise CLIError("Environment variable key must be non-empty.") import re if not re.match(r'^[A-Z_][A-Z0-9_]*$', key): print_info( f"Warning: '{key}' doesn't follow standard environment variable naming conventions (A-Z, 0-9, underscore)." ) if not value: raise CLIError("Secret value must be non-empty.")This prevents typos and ensures consistency with common environment variable naming conventions.
243-311: Consider error handling when fetching secret values.The command has good UX with confirmation prompts and proper YAML formatting. However, if
get_secret_valuefails for any handle, the entire command will fail without partial results.Consider wrapping the fetch loop with error handling:
resolved: Dict[str, str] = {} failed_keys: list[str] = [] for key, handle in handles.items(): try: value = run_async(client.get_secret_value(handle)) resolved[key] = value except Exception as e: failed_keys.append(key) print_error(f"Failed to fetch secret for {key}: {e}") if failed_keys: print_error(f"Failed to fetch {len(failed_keys)} secret(s). Proceeding with {len(resolved)} successful fetches.") if not resolved: raise typer.Exit(1)This provides partial results and clearer diagnostics when individual secrets are inaccessible.
src/mcp_agent/app.py (1)
246-265: Consider adding debug logging for environment binding operations.The implementation correctly handles fallback values and skips secret handles. However, the absence of logging makes it difficult to debug when environment variables aren't being set as expected.
Consider adding debug logging:
def _apply_environment_bindings(self) -> None: """Populate os.environ with values declared in settings.env when the value is available.""" try: specs = list(self._config.iter_env_specs()) except Exception: return for key, value in specs: if not key: continue if os.environ.get(key): + # Could add: self.logger.debug(f"Skipping {key}: already set in environment") continue if value is None: + # Could add: self.logger.debug(f"Skipping {key}: no fallback value") continue str_value = str(value) if str_value.startswith("mcpac_sc_"): # Actual secret values are injected by the deployment environment; skip handles. + # Could add: self.logger.debug(f"Skipping {key}: secret handle placeholder") continue os.environ[key] = str_value + # Could add: self.logger.debug(f"Set environment variable {key}")This helps diagnose configuration issues during initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/cloud/deployment-quickstart.mdx(3 hunks)docs/cloud/mcp-agent-cloud/manage-secrets.mdx(3 hunks)docs/reference/cli.mdx(3 hunks)src/mcp_agent/app.py(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(6 hunks)src/mcp_agent/cli/cloud/commands/deploy/materialize.py(1 hunks)src/mcp_agent/cli/cloud/commands/secrets/__init__.py(1 hunks)src/mcp_agent/cli/cloud/commands/secrets/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/utils.py(3 hunks)src/mcp_agent/cli/cloud/main.py(2 hunks)src/mcp_agent/cli/core/constants.py(1 hunks)src/mcp_agent/config.py(3 hunks)tests/cli/cloud/test_materialize.py(1 hunks)tests/config/test_env_settings.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
Repo: lastmile-ai/mcp-agent PR: 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/core/constants.pydocs/cloud/deployment-quickstart.mdx
📚 Learning: 2025-08-28T15:07:10.015Z
Learnt from: saqadri
Repo: lastmile-ai/mcp-agent PR: 386
File: src/mcp_agent/mcp/mcp_server_registry.py:110-116
Timestamp: 2025-08-28T15:07:10.015Z
Learning: In MCP server registry methods, when client_session_factory parameters are updated to accept additional context parameters, ensure the type hints match what is actually passed (Context instance vs ServerSession) and that the default factory (MCPAgentClientSession) can handle the number of arguments being passed to avoid TypeError at runtime.
Applied to files:
src/mcp_agent/app.py
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
Repo: lastmile-ai/mcp-agent PR: 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/**/*.py : Use mcp-agent's Agent abstraction for ALL LLM interactions, including quality evaluation, to ensure consistent tool access, logging, and error handling.
Applied to files:
src/mcp_agent/app.py
🧬 Code graph analysis (7)
src/mcp_agent/cli/cloud/commands/utils.py (4)
src/mcp_agent/app.py (1)
config(191-192)src/mcp_agent/config.py (1)
get_settings(1340-1463)tests/cli/cloud/test_materialize.py (1)
config_file(27-30)src/mcp_agent/core/context.py (2)
name(235-238)description(241-244)
tests/cli/cloud/test_materialize.py (1)
src/mcp_agent/cli/cloud/commands/deploy/materialize.py (1)
materialize_deployment_artifacts(90-192)
src/mcp_agent/app.py (1)
src/mcp_agent/config.py (1)
iter_env_specs(1262-1270)
src/mcp_agent/cli/cloud/commands/deploy/main.py (3)
src/mcp_agent/cli/secrets/api_client.py (1)
SecretsClient(12-206)src/mcp_agent/cli/cloud/commands/utils.py (1)
get_app_defaults_from_config(181-203)src/mcp_agent/cli/cloud/commands/deploy/materialize.py (1)
materialize_deployment_artifacts(90-192)
src/mcp_agent/cli/cloud/commands/deploy/materialize.py (4)
src/mcp_agent/cli/exceptions.py (1)
CLIError(4-10)src/mcp_agent/cli/secrets/api_client.py (1)
SecretsClient(12-206)src/mcp_agent/cli/secrets/yaml_tags.py (2)
dump_yaml_with_secrets(110-126)load_yaml_with_secrets(97-107)src/mcp_agent/config.py (2)
Settings(1094-1270)iter_env_specs(1262-1270)
tests/config/test_env_settings.py (1)
src/mcp_agent/config.py (2)
Settings(1094-1270)iter_env_specs(1262-1270)
src/mcp_agent/cli/cloud/commands/secrets/main.py (7)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(108-115)src/mcp_agent/cli/cloud/commands/utils.py (2)
get_app_defaults_from_config(181-203)resolve_server(97-101)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-10)src/mcp_agent/cli/mcp_app/api_client.py (2)
MCPAppClient(133-767)get_app_by_name(351-373)src/mcp_agent/cli/core/constants.py (1)
SecretType(36-40)src/mcp_agent/cli/secrets/api_client.py (1)
SecretsClient(12-206)src/mcp_agent/cli/utils/ux.py (3)
print_error(110-122)print_info(44-62)print_success(80-92)
🪛 LanguageTool
docs/cloud/mcp-agent-cloud/manage-secrets.mdx
[uncategorized] ~72-~72: Do not mix variants of the same word (‘materialise’ and ‘materialize’) within a single text.
Context: ....environ[KEY]`; the cloud control plane materialises the secret before your app starts. ## ...
(EN_WORD_COHERENCY)
🔇 Additional comments (15)
src/mcp_agent/cli/cloud/commands/utils.py (1)
181-203: LGTM! Clean utility function with proper defensive checks.The implementation correctly:
- Handles missing or non-existent config files
- Uses
set_global=Falseto avoid modifying global state- Validates extracted values are non-empty strings
- Provides graceful fallback with broad exception handling
src/mcp_agent/cli/cloud/commands/secrets/main.py (6)
1-30: LGTM! Standard Typer app setup with appropriate imports.The imports are well-organized and the Typer app configuration is appropriate for a command group.
33-47: LGTM! Proper credential resolution and client construction.The credential resolution chain (option → env → stored) is appropriate, and error handling provides clear user guidance.
49-81: LGTM! Robust app resolution with good fallback logic.The function correctly:
- Resolves from explicit identifier with both MCPApp and MCPAppConfiguration handling
- Falls back to config defaults when no identifier provided
- Provides clear error messages for resolution failures
83-99: LGTM! Defensive secret handle loading with proper validation.The implementation correctly:
- Uses namespaced prefix pattern for app-scoped secrets
- Handles API response variations (secretId vs secret_id)
- Validates secret names match expected prefix before processing
101-146: LGTM! Clean list command with good UX.The implementation provides:
- Clear informational message when no secrets exist
- Well-formatted table output
- Proper handle masking for security
- Consistent ordering via sorting
201-241: LGTM! Clean remove command with proper error handling.The implementation correctly:
- Validates secret existence before attempting deletion
- Provides clear error messages
- Uses appropriate exit code for user-facing errors
src/mcp_agent/cli/cloud/commands/secrets/__init__.py (1)
1-5: LGTM! Standard module initialization pattern.Clean public API exposure for the secrets command group.
src/mcp_agent/cli/core/constants.py (1)
15-15: LGTM! Consistent constant naming and placement.The new constant follows the established naming convention and is logically grouped with related deployment artifact filenames.
src/mcp_agent/cli/cloud/main.py (2)
31-31: LGTM! Clean import following established patterns.
161-161: LGTM! Proper registration of secrets command group.The secrets subcommand is correctly registered with clear help text and follows the established pattern for other command groups.
docs/cloud/deployment-quickstart.mdx (3)
61-61: LGTM! Clear documentation of both secret provisioning methods.The update correctly describes the two approaches for managing secrets and provides a helpful link to detailed guidance.
83-84: LGTM! Important guidance on deployment artifacts.The note correctly explains the purpose of the deployed artifact files and provides good advice about committing them for reproducible deployments.
108-109: LGTM! Helpful documentation of post-deployment secret management.The section correctly documents the secrets management commands and importantly notes their CI-safety, which is valuable for automation scenarios.
src/mcp_agent/app.py (1)
271-272: LGTM! Correct placement ensures environment variables are available early.Calling
_apply_environment_bindingsbefore context initialization ensures that any downstream code can access the populated environment variables.
| if handle: | ||
| run_async(secrets_client.set_secret_value(handle, value)) | ||
| else: | ||
| handle = run_async( | ||
| secrets_client.create_secret( | ||
| name=secret_name, | ||
| secret_type=SecretType.DEVELOPER, | ||
| value=value, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Handle secret update failures explicitly.
SecretsClient.set_secret_value returns a boolean success flag, but the current code ignores it. If the API responds with {"success": false} (e.g., handle revoked or backend failure), we silently proceed and persist the stale handle, leaving deployments with an outdated value and no warning. Capture the return value and raise a CLIError when the update fails so deploys cannot succeed with a dropped secret.
- if handle:
- run_async(secrets_client.set_secret_value(handle, value))
+ if handle:
+ success = run_async(secrets_client.set_secret_value(handle, value))
+ if not success:
+ raise CLIError(
+ f"Failed to update secret handle '{handle}' for '{spec.key}'. "
+ "Retry the deployment or re-materialize the secret."
+ )📝 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.
| if handle: | |
| run_async(secrets_client.set_secret_value(handle, value)) | |
| else: | |
| handle = run_async( | |
| secrets_client.create_secret( | |
| name=secret_name, | |
| secret_type=SecretType.DEVELOPER, | |
| value=value, | |
| ) | |
| ) | |
| if handle: | |
| success = run_async(secrets_client.set_secret_value(handle, value)) | |
| if not success: | |
| raise CLIError( | |
| f"Failed to update secret handle '{handle}' for '{spec.key}'. " | |
| "Retry the deployment or re-materialize the secret." | |
| ) | |
| else: | |
| handle = run_async( | |
| secrets_client.create_secret( | |
| name=secret_name, | |
| secret_type=SecretType.DEVELOPER, | |
| value=value, | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/deploy/materialize.py around lines 169 to
178, the code calls secrets_client.set_secret_value but ignores its boolean
result; update failures should abort and not persist a stale handle. Change the
branch so you capture the return value from
run_async(secrets_client.set_secret_value(handle, value)); if it returns False,
raise a CLIError with a clear message (including secret_name and handle) and do
not proceed to persist or use the existing handle; only on True continue, and
keep the existing logic for creating a new secret when handle is missing.
There was a problem hiding this comment.
@rholinshead Thanks for confirming! The suggestion should help catch secret update failures early during deployment rather than silently continuing with stale handles. Let me know if you need any clarification on the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/mcp_agent/cli/cloud/commands/deploy/materialize.py (1)
181-190: Handle secret update failures explicitly.The return value from
set_secret_valueis ignored. If the API call fails (e.g., handle revoked, backend error), the deployment proceeds silently with a stale secret, which could cause runtime failures.Apply this diff to check the return value and fail the deployment when secret updates fail:
if handle: - run_async(secrets_client.set_secret_value(handle, value)) + success = run_async(secrets_client.set_secret_value(handle, value)) + if not success: + raise CLIError( + f"Failed to update secret '{secret_name}' (handle: {handle}). " + "The handle may be revoked or invalid. Retry the deployment." + ) else: handle = run_async(
🧹 Nitpick comments (1)
tests/cli/cloud/test_materialize.py (1)
12-24: Consider adding failure scenario tests for secret operations.The
FakeSecretsClientalways returnsTruefromset_secret_value, which means failure scenarios (e.g., revoked handles, API errors) aren't covered. Since the implementation doesn't currently check the return value (per the past review comment on the implementation file), adding tests for this failure path would help ensure robust error handling once that issue is addressed.Consider adding a test like:
def test_materialize_handles_secret_update_failure( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ): cfg = tmp_path / "mcp_agent.config.yaml" cfg.write_text("env:\n - OPENAI_API_KEY\n", encoding="utf-8") deployed_secrets = tmp_path / "mcp_agent.deployed.secrets.yaml" deployed_secrets.write_text( yaml.safe_dump({"env": [{"OPENAI_API_KEY": "mcpac_sc_old"}]}), encoding="utf-8", ) class FailingSecretsClient(FakeSecretsClient): async def set_secret_value(self, handle, value): return False # Simulate failure monkeypatch.setenv("OPENAI_API_KEY", "new-value") client = FailingSecretsClient() with pytest.raises(CLIError, match="Failed to update secret"): materialize_deployment_artifacts( config_dir=tmp_path, app_id="app_123", config_file=cfg, deployed_secrets_path=deployed_secrets, secrets_client=client, non_interactive=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp_agent/cli/cloud/commands/deploy/materialize.py(1 hunks)tests/cli/cloud/test_materialize.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp_agent/cli/cloud/commands/deploy/materialize.py (6)
src/mcp_agent/cli/exceptions.py (1)
CLIError(4-10)src/mcp_agent/cli/secrets/api_client.py (1)
SecretsClient(12-206)src/mcp_agent/cli/secrets/yaml_tags.py (2)
dump_yaml_with_secrets(110-126)load_yaml_with_secrets(97-107)src/mcp_agent/app.py (1)
config(191-192)src/mcp_agent/config.py (3)
Settings(1094-1270)get_settings(1340-1463)iter_env_specs(1262-1270)tests/cli/cloud/test_materialize.py (4)
config_file(28-31)set_secret_value(22-24)create_secret(17-20)create_secret(96-97)
tests/cli/cloud/test_materialize.py (1)
src/mcp_agent/cli/cloud/commands/deploy/materialize.py (1)
materialize_deployment_artifacts(92-204)
🔇 Additional comments (14)
tests/cli/cloud/test_materialize.py (5)
34-57: LGTM!The test effectively validates the basic materialization flow: env vars are resolved from the environment, deployed artifacts are created, and secret handles have the expected format.
59-81: LGTM!The test correctly validates fallback behavior when environment variables aren't set.
83-112: LGTM!The test validates idempotency correctly—existing secret handles are reused and updated rather than creating duplicates.
114-133: LGTM!The test ensures graceful degradation when config files are malformed—returning the original config path and creating an empty secrets file prevents deployment failures.
135-169: LGTM!The test validates the correct precedence hierarchy: MCPApp configuration takes priority over file-based config, which is the expected behavior.
src/mcp_agent/cli/cloud/commands/deploy/materialize.py (9)
1-35: LGTM!The imports and
EnvSpecdataclass are well-structured. Usingslots=Trueis a good optimization for this simple data holder.
37-71: LGTM!The helper functions are well-designed with appropriate defensive checks. The
_load_deployed_secretsfunction correctly handles missing files, and_extract_existing_env_handlesvalidates the structure before extracting values.
74-89: LGTM!The config persistence logic correctly materializes only non-default, non-None, and non-unset values, keeping the deployed configuration minimal and focused.
92-136: LGTM!The function correctly handles the config loading hierarchy and gracefully degrades when configuration is invalid—ensuring empty secrets files are created to prevent downstream issues.
138-145: LGTM!The early return when no environment specifications exist is a good optimization, while still ensuring consistency by creating an empty secrets file if needed.
147-176: LGTM!The environment variable resolution logic correctly handles the precedence hierarchy (environment → fallback → prompt) and validates that resolved values are non-empty.
192-204: LGTM!The finalization logic correctly assembles the normalized entries and persists them. The user notification (lines 194-199) when values are manually captured provides good feedback.
207-227: Verify module cleanup on all exit paths.The function manipulates
sys.modulesandsys.pathbut only guarantees cleanup ofsys.pathvia the finally block. If an exception occurs afterdel sys.modules[module_name](line 218) but before successful import, the module remains deleted fromsys.modules, which could affect subsequent operations.Consider whether
sys.modulesshould be restored in the finally block:finally: if added_path and module_path in sys.path: try: sys.path.remove(module_path) except ValueError: pass # Consider: restore sys.modules[module_name] if it was present initially?Alternatively, verify that the current behavior (leaving the module deleted) is acceptable for the CLI context, where module state typically doesn't persist across operations.
228-264: LGTM!The MCPApp discovery logic correctly validates that exactly one instance exists and provides clear user feedback for error cases. The exception handling is comprehensive and user-friendly.
| Use the `cloud secrets` commands to inspect or rotate values captured via the `env` list: | ||
|
|
||
| ```bash | ||
| uvx mcp-agent cloud secrets list my-app |
There was a problem hiding this comment.
Ah, ok, looks like it's any of the identifiers (name,id,server)
| 2. Stores it as a deployment secret named `apps/<app_id>/env/<KEY>`. | ||
| 3. Inserts the resulting handle into the `env:` list inside `mcp_agent.deployed.secrets.yaml`. | ||
|
|
||
| The runtime still references the value via `os.environ[KEY]`; the cloud control plane materialises the secret before your app starts. |
There was a problem hiding this comment.
We'll need to do the materialization part in the deployment backend still, right?
There was a problem hiding this comment.
Oh, actually I think below with the env handling from the config in mcp-agent it should already be handled because the handle will already be resolved as normal in the deployed secrets file and then the env specs are put back into env by mcp-agent itself
| - `list` masks secret handles so you can confirm which keys are present. | ||
| - `add` updates the stored value (under the hood it calls the Secrets API and rewrites the handle if needed). | ||
| - `remove` deletes the value entirely. | ||
| - `pull` resolves values to a local YAML file (`mcp_agent.cloud.secrets.yaml` by default) without touching `mcp_agent.secrets.yaml`, mirroring Vercel’s `env pull`. |
There was a problem hiding this comment.
What do you do with the cloud.secrets yaml afterwards? Wondering if it's a bit too much complexity having yet another yaml file vs just printing to the console or something
|
|
||
| Use `--dry-run` for validation without uploading, and let the CLI guide you through classifying developer versus user secrets. | ||
|
|
||
| When deployment completes you will see two new artefacts next to your source: |
There was a problem hiding this comment.
| When deployment completes you will see two new artefacts next to your source: | |
| When deployment completes you will see two new artifacts next to your source: |
| - `--dry-run` validates using mock clients without persisting anything. | ||
|
|
||
| - **Secrets (`mcp-agent cloud secrets …`)** – manage environment secrets captured during deploy. | ||
| - `secrets list [APP]` displays stored environment keys with masked handles. |
There was a problem hiding this comment.
Just noting the possibility for confusion with these commands being named 'secrets' but actually specifically being env secrets and not general secrets that may be used/stored in the secrets config; would something like env, env_vars or env_secrets be a clearer command name?
| exclude_defaults=not include_overrides, | ||
| ) | ||
| with open(config_path, "w", encoding="utf-8") as handle: | ||
| yaml.safe_dump(materialized, handle, default_flow_style=False, sort_keys=False) |
There was a problem hiding this comment.
Can you double-check this works with ouath examples? For context, when trying to deploy oauth/auth config settings to cloud on Friday I was seeing an error resulting from the AnyHttpUrl types in the config, which can't serialize properly. I didn't have enough time to fully fix in the deployment daemon (will continue on that tomorrow) - but it would either require custom dump handling or use mode="json". The latter adds an extra "/" at the end, though. But in any case, my understanding is safe_dump will actually raise an exception instead, so apps with oauth wouldn't be able to be deployed without error during materialization
There was a problem hiding this comment.
Side-note, fwiw claude thinks mode="json" would be safe except for specific provider handling:
Based on this detailed analysis, here's what I found:
🎯 The Good News: Most Critical Paths Are Safe
✅ Cache Keys - All Canonicalized
- Resource metadata cache: URLs go through normalize_resource() first
- Authorization metadata cache: URLs go through _canonicalize_url() first
- Verdict: Trailing slashes won't cause cache issues
✅ Token Storage - Indirectly Canonicalized
- Resource URLs: Processed by normalize_resource() before storage
- Authorization server URLs: Processed by _canonicalize_url() before storage
- Verdict: Trailing slashes won't cause token storage mismatches
✅ URL Comparisons - Both Sides Canonicalized
- Authorization server validation canonicalizes both the provided URL and the resolved issuer
- Verdict: Trailing slashes won't cause comparison failures
⚠️ The Potential Issues
1. OAuth Provider API Calls
# These are used RAW from metadata/config:
authorize_url = httpx.URL(str(auth_metadata.authorization_endpoint) + "?" + params)
token_endpoint = str(auth_metadata.token_endpoint)
redirect_uri = redirect_uri # From config
Impact: If OAuth providers do strict string matching on redirect URIs, a mismatch between registered https://example.com/callback and Pydantic-normalized
https://example.com/callback/ could cause failures.
2. Callback URL Registration
When you register your OAuth app with a provider, you specify exact callback URLs. If:
- You register: https://example.com/callback
- But Pydantic normalizes to: https://example.com/callback/
- And that gets sent to the OAuth provider
Then the OAuth provider might reject it due to URL mismatch.
🎯 Conclusion
The trailing slash issue is likely NOT a problem for most functionality because:
1. Internal caching and storage is safe - everything gets canonicalized
2. URL comparisons are safe - both sides get canonicalized
3. Most URL construction is safe - rstrip('/') handles both cases
The only real risk is OAuth provider callback URL validation, which depends on:
- Whether OAuth providers do exact string matching on redirect URIs
- Whether the registered callback URLs match what gets sent after Pydantic normalization
For most OAuth providers, this probably isn't an issue since they typically normalize URLs themselves. But it's worth testing with actual OAuth flows to be sure.
There was a problem hiding this comment.
Ok, #605 should be all we need to allow us to just use mode="json" here when yaml dumping
| # Nothing further to do; ensure secrets file exists if previously created | ||
| if not deployed_secrets_path.exists(): |
There was a problem hiding this comment.
Can you clarify this case? It seems to just be creating an empty deployed secrets file if it doesn't exist yet. But for apps that don't have secrets, we wouldn't want that, right?
| if handle: | ||
| run_async(secrets_client.set_secret_value(handle, value)) | ||
| else: | ||
| handle = run_async( | ||
| secrets_client.create_secret( | ||
| name=secret_name, | ||
| secret_type=SecretType.DEVELOPER, | ||
| value=value, | ||
| ) | ||
| ) |
| module_file = Path(getattr(module, "__file__", "")).resolve() | ||
| if not module_file or project_root not in module_file.parents: | ||
| typer.secho( | ||
| f"Module 'main' resolved outside project directory ({module_file}); skipping MCPApp load.", |
There was a problem hiding this comment.
Is this sys/module manipulation an additional security over checking main.py exists?
|
|
||
| def _load_existing_handles(client: SecretsClient, app_id: str) -> Dict[str, str]: | ||
| prefix = _env_secret_prefix(app_id) | ||
| secrets = run_async(client.list_secrets(name_filter=prefix)) |
There was a problem hiding this comment.
(later) We can also add the association between the app and secrets in the DB to be able to query more efficiently
Summary
TLDR -- tries to implement these ergonomics:
Declaring environment variables meant editing raw YAML, secrets were processed server-side, and there was no convenient CLI to inspect or rotate env secrets after initial deploys. This PR brings the developer experience closer to Vercel’s
envworkflow by:mcp_agent.config.yamlcloud deploy, alongside a frozen config snapshot (mcp_agent.deployed.config.yaml)mcp-agent cloud secretscommands (list/add/remove/pull) so teams can manage env secrets without redeployingFunctional Changes
Config schema & runtime
Settings.envnow accepts a list of strings or single-key dicts, e.g.:Added validation helpers plus
iter_env_specs()to preserve ordering and union string/dict entries.MCPAppinjects literal fallbacks intoos.environif the process is missing the variable; handles (mcpac_sc_*) are ignored so the control plane can still supply real values.Deployment workflow
materialize_deployment_artifacts()helper invoked fromcloud deploy:mcp_agent.deployed.config.yaml(user config snapshot)envspec, resolves values (os.environ→ fallback literal), creates/updates secrets (apps/<app_id>/env/<KEY>), and writes their handles underenv:inmcp_agent.deployed.secrets.yaml.CLI enhancements
mcp-agent cloud secretscommand group:list [app]– show env secrets with masked handles.add <key> <value> [app]– create or update a secret value.remove <key> [app]– delete the secret handle.pull [app] --output mcp_agent.cloud.secrets.yaml– download resolved values without touchingmcp_agent.secrets.yaml.--config-dir,--api-url, and--api-keyand fall back to project defaults when possible.Documentation
envdeclaration examples and how they feed intomcp_agent.deployed.secrets.yaml.mcp_agent.deployed.config.yaml.cloud secretssubcommands.env pullexperience.Backend coordination
BUNDLE_CONFIG_FILENAME) and still applies cloud overrides (Temporal, SSE). It skips gracefully if the file is missing or invalid.Usage Walkthrough
mcp_agent.deployed.secrets.yamlnow contains:And
mcp_agent.deployed.config.yamlmirrors the project config after overrides, letting the control plane skip redundant transformations.Summary by CodeRabbit
New Features
Documentation
Tests