Allow specifying app name optionally in deploy command #527
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds root name/description to the config schema and Settings model; defers MCPApp name/description resolution to use config-first fallbacks; simplifies OpenTelemetry exporters schema to an enum list; enhances deploy CLI to resolve config/working dirs and default app name/description from config; updates and extends related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Config Loader
participant A as MCPApp
participant M as MCP Instance
Note over C,A: App startup / initialization
C->>A: Load settings (including name, description)
alt Config provides name/description
A->>A: Set app.name / app.description from config
else Config missing fields
A->>M: Read mcp.name / mcp.instructions
A->>A: Use MCP values as fallback (if present)
end
sequenceDiagram
autonumber
participant U as User (CLI)
participant D as deploy_config
participant P as Path Resolver
participant CF as get_config_files
participant GS as get_settings
participant W as wrangler_deploy
U->>D: deploy --config-dir [opt] --working-dir [opt] [--app-name/--description]
D->>P: Resolve config_dir relative to working_dir
P-->>D: Validated config_dir
D->>CF: Load config files from config_dir
D->>GS: Read settings (name, description)
GS-->>D: Settings (name, description)
alt CLI app-name/description not provided
D->>D: Default app_name/app_description from Settings or directory name
end
D->>U: Prompt update/reuse secrets (default: reuse)
alt Reuse secrets
D->>D: Use existing deployed secrets path
else Regenerate
D->>D: Set deployed_secrets_file = None
end
D->>W: Deploy with resolved app_name/description and secrets handling
W-->>D: Result
D-->>U: Exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 1
🧹 Nitpick comments (5)
src/mcp_agent/app.py (1)
113-120: Config-first resolution for name/description looks goodBehavior: CLI arg → config → MCP instance; description falls back to static default. Consider guaranteeing a non-empty string for self.name if callers pass None (optional).
src/mcp_agent/cli/cloud/commands/deploy/main.py (4)
142-156: Config directory resolution/validation looks correctHandles absolute/relative paths against working_dir. Optional: call .resolve() on resolved_config_dir for clearer logs.
221-224: Tiny grammar fix in confirm prompt“Do you want to deploy an update…” reads better.
- f"Do you want deploy an update to the existing app ID: {app_id}?", + f"Do you want to deploy an update to the existing app ID: {app_id}?",
278-281: Path join stylePrefer Path composition over f-strings for portability.
- secrets_transformed_path = Path( - f"{config_dir}/{MCP_DEPLOYED_SECRETS_FILENAME}" - ) + secrets_transformed_path = config_dir / MCP_DEPLOYED_SECRETS_FILENAME
492-517: Adjust return typing and doc for _get_app_info_from_configFunction always returns a 2-tuple (possibly Nones), not Optional[tuple]. Also update docstring to mention description.
-def _get_app_info_from_config(config_file: Path) -> Optional[tuple[str, str]]: - """Return a default deployment name sourced from configuration if available.""" +def _get_app_info_from_config( + config_file: Path, +) -> tuple[Optional[str], Optional[str]]: + """Return (app_name, app_description) sourced from configuration if available.""" @@ - except Exception: - return None, None + except Exception: + return None, None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
schema/mcp-agent.config.schema.json(4 hunks)src/mcp_agent/app.py(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(6 hunks)src/mcp_agent/config.py(1 hunks)tests/cli/commands/test_deploy_command.py(2 hunks)tests/cli/commands/test_wrangler_wrapper.py(11 hunks)tests/test_app.py(2 hunks)tests/utils/test_config_preload.py(10 hunks)
🧰 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:
tests/utils/test_config_preload.pysrc/mcp_agent/app.py
🧬 Code graph analysis (6)
tests/utils/test_config_preload.py (1)
src/mcp_agent/config.py (1)
get_settings(770-870)
src/mcp_agent/app.py (1)
src/mcp_agent/core/context.py (1)
mcp(102-103)
src/mcp_agent/cli/cloud/commands/deploy/main.py (3)
src/mcp_agent/config.py (1)
get_settings(770-870)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-10)src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)
tests/cli/commands/test_wrangler_wrapper.py (1)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
_needs_requirements_modification(35-41)
tests/cli/commands/test_deploy_command.py (2)
src/mcp_agent/cli/mcp_app/api_client.py (2)
get_app_id_by_name(286-308)create_app(134-164)src/mcp_agent/cli/mcp_app/mock_client.py (2)
get_app_id_by_name(36-45)create_app(86-124)
tests/test_app.py (2)
tests/workflows/router/conftest.py (1)
mock_context(17-67)src/mcp_agent/app.py (1)
config(173-174)
🪛 GitHub Actions: Pull Request Checks
tests/cli/commands/test_deploy_command.py
[error] 266-266: Deploy defaults to directory name when config missing name. AssertionError: expected temp_config_dir.name but got 'default'. The test fails because deployment uses 'default' instead of the working directory name.
🔇 Additional comments (11)
tests/test_app.py (2)
20-21: LGTM: explicit None config fields for name/descriptionGood setup to exercise config-first fallback paths.
121-122: LGTM: custom settings with None name/descriptionKeeps constructor paths deterministic in tests.
tests/cli/commands/test_deploy_command.py (4)
145-202: Test asserts config-derived app name when CLI omits nameCovers expected happy path; will pass once deploy fallback logic prefers config, else directory (see main.py fix below).
204-267: Test asserts directory-name fallback when config lacks nameThis is the source of the pipeline failure; current implementation defaults to "default". The fix is to use config_dir.name. See proposed diff in deploy/main.py.
269-328: Test asserts config-derived description reuseGood coverage for description defaulting. Aligns with helper that reads description from config.
394-396: Nit: assertion simplification is fineInline equality read is clearer.
src/mcp_agent/cli/cloud/commands/deploy/main.py (2)
61-83: New --config-dir and --working-dir optionsSolid ergonomics. Minor: help text for --config-dir mentions resolution against --working-dir (good). No change required.
255-266: Secrets reuse prompt defaults to reuse=TrueNice UX; behavior matches non_interactive path. No change requested.
schema/mcp-agent.config.schema.json (3)
361-373: GoogleSettings.default_model addedSchema looks correct (string|null, default null). Ensure config model maps this field.
882-892: OTel exporters simplified to enum arraySimplifies config. Verify downstream code tolerates string list vs prior discriminated union.
Also applies to: 938-938
1182-1207: Root-level name/description fieldsMatches app/config/test changes; good addition.
| config_file, secrets_file, deployed_secrets_file = get_config_files(config_dir) | ||
|
|
||
| default_app_name, default_app_description = _get_app_info_from_config( | ||
| config_file | ||
| ) | ||
|
|
||
| if app_name is None: | ||
| if default_app_name: | ||
| print_info( | ||
| f"No app name provided. Using '{default_app_name}' from configuration." | ||
| ) | ||
| app_name = default_app_name | ||
| else: | ||
| app_name = "default" | ||
| print_info("No app name provided. Using 'default' as app name.") | ||
|
|
||
| if app_description is None: | ||
| if default_app_description: | ||
| print_info( | ||
| "No app description provided. Using description from configuration." | ||
| ) | ||
| app_description = default_app_description | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Fix fallback: use directory name when config has no name (test blocker)
Current code defaults to "default" when CLI name and config name are missing; tests expect config_dir.name. Apply this change.
- if app_name is None:
- if default_app_name:
- print_info(
- f"No app name provided. Using '{default_app_name}' from configuration."
- )
- app_name = default_app_name
- else:
- app_name = "default"
- print_info("No app name provided. Using 'default' as app name.")
+ if app_name is None:
+ if default_app_name:
+ print_info(
+ f"No app name provided. Using '{default_app_name}' from configuration."
+ )
+ app_name = default_app_name
+ else:
+ # Fallback: use directory name when config does not define one
+ app_name = config_dir.name
+ print_info(
+ f"No app name provided. Using directory name '{app_name}' as app name."
+ )After this change, test_deploy_defaults_to_directory_name_when_config_missing_name should pass.
Also applies to: 163-172
🏁 Script executed:
#!/usr/bin/env bash
set -eux
# Print lines 130-230 for context in main.py
sed -n '130,230p' src/mcp_agent/cli/cloud/commands/deploy/main.py || true
# Search for the specific test
rg -n "test_deploy_defaults_to_directory_name_when_config_missing_name" -C3 -g "*.py" . || trueLength of output: 4743
Fix fallback: use directory name when config has no name
File: src/mcp_agent/cli/cloud/commands/deploy/main.py
Replace the "default" fallback under if app_name is None with config_dir.name so the test passes:
- if app_name is None:
- if default_app_name:
- print_info(
- f"No app name provided. Using '{default_app_name}' from configuration."
- )
- app_name = default_app_name
- else:
- app_name = "default"
- print_info("No app name provided. Using 'default' as app name.")
+ if app_name is None:
+ if default_app_name:
+ print_info(
+ f"No app name provided. Using '{default_app_name}' from configuration."
+ )
+ app_name = default_app_name
+ else:
+ # Fallback: use directory name when config does not define one
+ app_name = config_dir.name
+ print_info(
+ f"No app name provided. Using directory name '{app_name}' as app name."
+ )After this change, test_deploy_defaults_to_directory_name_when_config_missing_name will pass.
📝 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.
| config_file, secrets_file, deployed_secrets_file = get_config_files(config_dir) | |
| default_app_name, default_app_description = _get_app_info_from_config( | |
| config_file | |
| ) | |
| if app_name is None: | |
| if default_app_name: | |
| print_info( | |
| f"No app name provided. Using '{default_app_name}' from configuration." | |
| ) | |
| app_name = default_app_name | |
| else: | |
| app_name = "default" | |
| print_info("No app name provided. Using 'default' as app name.") | |
| if app_description is None: | |
| if default_app_description: | |
| print_info( | |
| "No app description provided. Using description from configuration." | |
| ) | |
| app_description = default_app_description | |
| if app_name is None: | |
| if default_app_name: | |
| print_info( | |
| f"No app name provided. Using '{default_app_name}' from configuration." | |
| ) | |
| app_name = default_app_name | |
| else: | |
| # Fallback: use directory name when config does not define one | |
| app_name = config_dir.name | |
| print_info( | |
| f"No app name provided. Using directory name '{app_name}' as app name." | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/deploy/main.py around lines 157 to 179, the
fallback that sets app_name to the literal "default" when no app name is
provided should be replaced to use the directory name; change the assignment to
set app_name = config_dir.name and update the accompanying print_info message to
reflect using the config directory name instead of the string "default".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/cli/commands/test_deploy_command.py (3)
145-202: Strengthen assertions to verify create_app uses the resolved name.You assert the lookup used "fixture-app"; also assert the app creation used the same name (and optionally that wrangler_deploy was called).
@@ ): - result = runner.invoke( + result = runner.invoke( app, [ "deploy", "--working-dir", temp_config_dir, @@ ) assert result.exit_code == 0, f"Deploy command failed: {result.stdout}" first_call = mock_client.get_app_id_by_name.await_args_list[0] assert first_call.args[0] == "fixture-app" + # Ensure we created the app with the resolved name + create_call = mock_client.create_app.await_args + assert create_call.args[0] == "fixture-app"If the deploy flow changed to pass name by keyword, switch to
create_call.kwargs["name"]. Also consider naming the wrangler patch (as mock_wrangler_deploy) and assertingmock_wrangler_deploy.assert_called_once().
204-267: Rename test to reflect behavior (“default” string), and assert create_app uses it.Function name says “directory name” but behavior expects literal "default". Rename for clarity and add an assertion on create_app’s name.
-def test_deploy_defaults_to_directory_name_when_config_missing_name( +def test_deploy_defaults_to_default_name_when_config_missing_name( runner, temp_config_dir ): - """Fallback uses the default name when config doesn't define one.""" + """Fallback uses the literal 'default' name when config doesn't define one.""" @@ assert result.exit_code == 0, f"Deploy command failed: {result.stdout}" first_call = mock_client.get_app_id_by_name.await_args_list[0] assert first_call.args[0] == "default" + # Ensure create_app was invoked with the defaulted name + create_call = mock_client.create_app.await_args + assert create_call.args[0] == "default"
269-328: Also assert name passed to create_app when only description comes from config.This verifies both defaults: name fallback and description reuse.
@@ assert result.exit_code == 0, f"Deploy command failed: {result.stdout}" create_call = mock_client.create_app.await_args assert create_call.kwargs["description"] == "Configured app description" + # Name should fall back when not provided in CLI or config + assert create_call.args[0] == "default"Optionally add a complementary test where a CLI description overrides the config description.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/cli/commands/test_deploy_command.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cli/commands/test_deploy_command.py (2)
src/mcp_agent/cli/mcp_app/api_client.py (2)
get_app_id_by_name(286-308)create_app(134-164)src/mcp_agent/cli/mcp_app/mock_client.py (2)
get_app_id_by_name(36-45)create_app(86-124)
🔇 Additional comments (1)
tests/cli/commands/test_deploy_command.py (1)
394-396: LGTM: clearer one-line assertion with message.
| "title": "MCPSettings", | ||
| "type": "object" | ||
| }, | ||
| "OTelConsoleExporterSettings": { |
There was a problem hiding this comment.
Unrelated, but I suppose we can also clean up the fallback handling for the deprecated settings now?
|
Thanks! |
2 participants
Makes the app name optional in the deploy command. In case it's not specified, we now look in the config to see if an app name is defined there, and if not, default to
"default"as the name.Also improved the wording for the secrets prompt.
Summary by CodeRabbit
New Features
Refactor
Tests