Improve log hygiene and interactions in deploy and configure commands #572
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR removes the global CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI Entry Point
participant LV as LOG_VERBOSE
participant Cmd as Command Handler
participant UX as UX Module
Note over User,CLI: User invokes CLI with --verbose
User->>CLI: run command (--verbose)
CLI->>LV: set(True)
CLI->>Cmd: invoke command
Cmd->>LV: get()
LV-->>Cmd: True
Cmd->>UX: print_verbose("message")
UX->>Cmd: conditional verbose output
Cmd-->>User: command output (verbose gated)
sequenceDiagram
participant Deploy as Deploy Command
participant API as MCPAppClient
participant Store as App Backend
Note over Deploy: Old flow (ID-based)
Deploy->>API: get_app_id_by_name(name)
API->>Store: list/query apps
Store-->>API: matched app (appId)
API-->>Deploy: appId (str)
rect rgb(235, 245, 235)
Note over Deploy: New flow (object-based)
Deploy->>API: get_app_by_name(name)
API->>Store: list/query apps
Store-->>API: matched MCPApp object
API-->>Deploy: MCPApp (appId + metadata)
Deploy->>API: get_app_id_by_name(name) (wrapper)
API->>API: delegate -> get_app_by_name
API-->>Deploy: MCPApp.appId
end
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 |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
E.g. deploy: Configure: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
LLMS.txt (1)
1411-1415: Doc still references removed--quietflagThe signature now exposes only
verboseandcolor, but this parameter section still documentsquiet, which is gone. Please drop or update thequietbullet so the CLI help matches what users actually see.- `verbose` (bool, optional): Default is typer.Option(False, '--verbose', '-v', help='Enable verbose mode') - - `quiet` (bool, optional): Default is typer.Option(False, '--quiet', '-q', help='Disable output') - `color` (bool, optional): Default is typer.Option(True, '--color/--no-color', help='Enable/disable color output')src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
201-211: Sensitive secrets file may be bundled — exclude configured secrets too.Only MCP_SECRETS_FILENAME is excluded. The configured secrets file (MCP_CONFIGURED_SECRETS_FILENAME) can contain real tokens and should be excluded from copy and fingerprint.
Apply:
-from mcp_agent.cli.core.constants import ( - CLOUDFLARE_ACCOUNT_ID, - CLOUDFLARE_EMAIL, - WRANGLER_SEND_METRICS, -) +from mcp_agent.cli.core.constants import ( + CLOUDFLARE_ACCOUNT_ID, + CLOUDFLARE_EMAIL, + WRANGLER_SEND_METRICS, + MCP_SECRETS_FILENAME, + MCP_CONFIGURED_SECRETS_FILENAME, +) @@ - if (name.startswith(".") and name not in {".env"}) or name in { + if (name.startswith(".") and name not in {".env"}) or name in { "logs", "__pycache__", "node_modules", "venv", - MCP_SECRETS_FILENAME, # Exclude mcp_agent.secrets.yaml only + MCP_SECRETS_FILENAME, + MCP_CONFIGURED_SECRETS_FILENAME, }: ignored.add(name) @@ - ignore_names={ + ignore_names={ ".git", "logs", "__pycache__", "node_modules", "venv", - MCP_SECRETS_FILENAME, + MCP_SECRETS_FILENAME, + MCP_CONFIGURED_SECRETS_FILENAME, },Also applies to: 296-306
src/mcp_agent/cli/cloud/commands/configure/main.py (1)
45-97: Return type should be Optional[str] (function returns None in some paths).configure_app returns None on cancellation and a string elsewhere; annotation says -> str.
-def configure_app( +def configure_app( ctx: typer.Context, @@ -) -> str: +) -> Optional[str]:
♻️ Duplicate comments (3)
src/mcp_agent/cli/commands/keys.py (2)
176-178: Duplicate: Same verbose handling pattern as flagged in build.py.This is the same pattern flagged in build.py (lines 204-206). See the refactoring suggestion in that review comment to eliminate duplication and handle defaults safely.
459-461: Duplicate: Same verbose handling pattern as flagged in build.py.This is the same pattern flagged in build.py (lines 204-206). See the refactoring suggestion in that review comment to eliminate duplication and handle defaults safely.
src/mcp_agent/cli/commands/config.py (1)
130-132: Duplicate: Same verbose handling pattern as flagged in build.py.This is the same pattern flagged in build.py (lines 204-206). See the refactoring suggestion in that review comment to eliminate duplication and handle defaults safely.
🧹 Nitpick comments (10)
src/mcp_agent/cli/utils/ux.py (1)
38-42: Consider renaming or updating docstring for accuracy.The docstring claims "fixed-width label" but the function doesn't enforce any width. The width appears fixed only because the output format is consistent (dot + spaces).
src/mcp_agent/cli/commands/server.py (1)
698-701: Avoid leaking global verbose state across the process.Setting LOG_VERBOSE without restoring may persist beyond this command in the same process. Capture/restore the prior value.
Example:
- if verbose: - LOG_VERBOSE.set(True) - verbose = LOG_VERBOSE.get() + prev_verbose = LOG_VERBOSE.get() + if verbose: + LOG_VERBOSE.set(True) + verbose = LOG_VERBOSE.get() + try: + ... + finally: + LOG_VERBOSE.set(prev_verbose)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
193-195: Minor: consider downgrading this to verbose.“Using ignore patterns from …” is operational noise; route via print_verbose to keep default output clean.
- print_info(f"Using ignore patterns from {ignore_file}") + print_verbose(f"Using ignore patterns from {ignore_file}")src/mcp_agent/cli/mcp_app/api_client.py (1)
351-374: Exact-name lookup OK; add resilient fallback.If the API’s nameFilter isn’t exact, consider a case-insensitive fallback and tighten max_results for efficiency.
- apps = await self.list_apps(name_filter=name, max_results=10) + apps = await self.list_apps(name_filter=name, max_results=10) if not apps.apps: return None - # Return the app with exact name match - return next((app for app in apps.apps if app.name == name), None) + exact = next((app for app in apps.apps if app.name == name), None) + if exact: + return exact + # Fallback: case-insensitive match + lowered = name.lower() + return next((app for app in apps.apps if app.name.lower() == lowered), None)src/mcp_agent/cli/cloud/commands/configure/main.py (2)
66-66: Nit: typo in help text.“tranformed” -> “transformed”.
- help="Path to write prompted and tranformed secrets to. Defaults to mcp_agent.configured.secrets.yaml", + help="Path to write prompted and transformed secrets to. Defaults to mcp_agent.configured.secrets.yaml",
45-45: Unused ctx parameter.If not used, drop it; or document future use. Keeping it unused may confuse.
-def configure_app( - ctx: typer.Context, +def configure_app(Or prefix with “_ctx” to signal intentional non-use.
src/mcp_agent/cli/cloud/commands/deploy/main.py (4)
172-175: Reset LOG_VERBOSE after the command to avoid cross-command leakage.Set returns a ContextVar token; reset it in finally for hygiene.
- if verbose: - LOG_VERBOSE.set(True) + token = None + if verbose: + token = LOG_VERBOSE.set(True) try: ... - except Exception as e: + except Exception as e: end_time = datetime.now(timezone.utc).isoformat().replace("+00:00", "Z") if LOG_VERBOSE.get(): import traceback typer.echo(traceback.format_exc()) raise CLIError(f"[{end_time}] Deployment failed: {str(e)}") from e + finally: + if verbose and token is not None: + try: + LOG_VERBOSE.reset(token) + except Exception: + # Fallback: ensure verbose is off if reset fails + LOG_VERBOSE.set(False)Also applies to: 459-466
9-10: Fix typing: usetyping.Anyinstead of builtinany.Improves type-checking and avoids confusing the builtin
anyfunction with a type.-from typing import Optional, List, Tuple +from typing import Optional, List, Tuple, Any ... - deployment_properties_display_info: List[Tuple[str, any, bool]] = [ + deployment_properties_display_info: List[Tuple[str, Any, bool]] = [Also applies to: 294-300
389-397: Validate--ignore-filewhen provided to fail fast on typos.If the user passes a path, ensure it exists before proceeding; otherwise bundling may behave unexpectedly.
ignore_path: Optional[Path] = None if ignore_file is not None: - ignore_path = ignore_file + if not ignore_file.exists(): + raise CLIError(f"Ignore file not found: {ignore_file}", retriable=False) + ignore_path = ignore_file else: candidate = config_dir / ".mcpacignore" if not candidate.exists(): candidate = Path.cwd() / ".mcpacignore" ignore_path = candidate if candidate.exists() else None
544-548: Avoid mutating Rich spinner internals.Modifying
spinner_column.spinner.framesis brittle across Rich versions. Prefer stopping the task or simply updating the description.- spinner_column.spinner.frames = spinner_column.spinner.frames[-2:-1] - progress.update( + # Optionally stop the spinner to indicate completion cleanly: + # progress.stop_task(deploy_task) + progress.update( deploy_task, description=f"MCP App deployed successfully{attempt_suffix}!", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
LLMS.txt(1 hunks)docs/cli-reference.mdx(0 hunks)src/mcp_agent/cli/cloud/commands/configure/main.py(9 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(11 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(7 hunks)src/mcp_agent/cli/commands/build.py(2 hunks)src/mcp_agent/cli/commands/config.py(2 hunks)src/mcp_agent/cli/commands/keys.py(3 hunks)src/mcp_agent/cli/commands/server.py(2 hunks)src/mcp_agent/cli/config/settings.py(2 hunks)src/mcp_agent/cli/main.py(2 hunks)src/mcp_agent/cli/mcp_app/api_client.py(2 hunks)src/mcp_agent/cli/utils/ux.py(8 hunks)
💤 Files with no reviewable changes (1)
- docs/cli-reference.mdx
🧰 Additional context used
🧬 Code graph analysis (5)
src/mcp_agent/cli/cloud/commands/configure/main.py (4)
src/mcp_agent/cli/utils/ux.py (4)
print_verbose(65-77)print_configuration_header(268-306)print_info(44-62)print_success(80-92)src/mcp_agent/cli/mcp_app/api_client.py (1)
configure_app(430-467)src/mcp_agent/cli/mcp_app/mock_client.py (1)
configure_app(171-222)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-10)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
src/mcp_agent/cli/utils/git_utils.py (3)
get_git_metadata(52-85)compute_directory_fingerprint(130-167)utc_iso_now(88-89)src/mcp_agent/cli/utils/ux.py (4)
print_error(110-122)print_warning(95-107)print_info(44-62)print_verbose(65-77)
src/mcp_agent/cli/main.py (1)
src/mcp_agent/cli/utils/ux.py (1)
print_error(110-122)
src/mcp_agent/cli/mcp_app/api_client.py (3)
src/mcp_agent/core/context.py (1)
name(185-188)src/mcp_agent/server/app_server.py (1)
app(135-137)src/mcp_agent/cli/mcp_app/mock_client.py (1)
get_app_id_by_name(36-45)
src/mcp_agent/cli/cloud/commands/deploy/main.py (5)
src/mcp_agent/cli/mcp_app/api_client.py (4)
MCPApp(23-32)get_app_by_name(351-373)create_app(136-175)update_app(259-311)src/mcp_agent/cli/utils/ux.py (4)
print_deployment_header(216-265)print_info(44-62)print_success(80-92)print_verbose(65-77)src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(108-115)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/core/api_client.py (2)
UnauthenticatedError(9-12)get(103-112)
🔇 Additional comments (10)
src/mcp_agent/cli/utils/ux.py (1)
268-306: LGTM! Enhanced configuration header provides better UX.The updated header clearly displays all relevant configuration details (app server URL, required secrets, files, dry-run mode) before the user confirms. The conditional logic appropriately handles cases with and without required parameters.
src/mcp_agent/cli/config/settings.py (1)
42-44: Remove this review comment; the identified scenario does not apply to the commands mentioned.The verification found that
build.py,keys.py, andconfig.pydo not importmcp_agent.cli.config.settings(either directly or transitively). They import frommcp_agent.configinstead, which is a different module. Since these commands do not trigger the import of the CLI settings module, the module-levelLOG_VERBOSE.set()on line 42-44 will not overwrite their verbose flags. The hypothetical import order conflict described in the review does not occur for these specific commands.Likely an incorrect or invalid review comment.
src/mcp_agent/cli/commands/server.py (1)
15-15: Good move: centralized verbose state.Importing LOG_VERBOSE here aligns server commands with global verbosity control.
src/mcp_agent/cli/main.py (2)
18-18: Centralizing UX utilities looks good.Using LOG_VERBOSE and print_error from one place improves consistency.
123-125: Set verbose early — good.Global verbose is flipped at entry; downstream commands can rely on LOG_VERBOSE.
src/mcp_agent/cli/cloud/commands/configure/main.py (4)
111-113: Verbose wiring matches the new global pattern.Setting LOG_VERBOSE from the flag is consistent with the CLI’s centralized control.
175-183: Nice: default secrets output path logged only in verbose.Keeps default output quiet while discoverable.
237-243: Good: consolidated header via UX utility.Improves consistency across commands.
275-277: Frame mutation confirmed compatible and in parity with deploy bundler.The "aesthetic" spinner is a valid, defined spinner in Rich, and the frame mutation pattern
spinner_column.spinner.frames = spinner_column.spinner.frames[-2:-1](which slices to the second-to-last frame) is correctly implemented. The configure code uses the identical pattern as both deploy/wrangler_wrapper.py:403 and deploy/main.py:544, confirming full parity. No compatibility issues detected.src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
371-376: The review comment's concerns are not substantiated.Rich version 13.5.3 is in use, and the "aesthetic" spinner exists in the SPINNERS registry. Additionally, frames are stored as a Python list (mutable), not an immutable structure, so the mutation operations in lines 403, 275, and 544 are valid. No runtime errors or incompatibilities exist with the current code.
Likely an incorrect or invalid review comment.
| if config.appServerInfo: | ||
| server_url = config.appServerInfo.serverUrl | ||
| print_info(f"App Server URL: [link={server_url}]{server_url}[/link]") | ||
| print_info( | ||
| f"Use this configured app as an MCP server at {server_url}/sse\n\nMCP configuration example:" | ||
| ) | ||
|
|
||
| # Use the app name if available, otherwise use a simple default | ||
| app_name = config.app.name if config.app else "configured-app" | ||
|
|
||
| mcp_config = { | ||
| "mcpServers": { | ||
| app_name: { | ||
| "url": f"{server_url}/sse", | ||
| "transport": "sse", | ||
| "headers": {"Authorization": f"Bearer {effective_api_key}"}, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.print( | ||
| f"[bright_black]{json.dumps(mcp_config, indent=2)}[/bright_black]", | ||
| soft_wrap=True, | ||
| ) |
There was a problem hiding this comment.
Do not print API keys in generated config.
Leaking effective_api_key to console is a high-severity secret exposure risk (terminal history, logs, screenshots).
Replace with a placeholder:
- mcp_config = {
+ mcp_config = {
"mcpServers": {
app_name: {
"url": f"{server_url}/sse",
"transport": "sse",
- "headers": {"Authorization": f"Bearer {effective_api_key}"},
+ "headers": {"Authorization": "Bearer <YOUR_API_KEY>"},
}
}
}Optionally, print the real header only if the user explicitly asks (e.g., a new --show-secret flag), never by default.
📝 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 config.appServerInfo: | |
| server_url = config.appServerInfo.serverUrl | |
| print_info(f"App Server URL: [link={server_url}]{server_url}[/link]") | |
| print_info( | |
| f"Use this configured app as an MCP server at {server_url}/sse\n\nMCP configuration example:" | |
| ) | |
| # Use the app name if available, otherwise use a simple default | |
| app_name = config.app.name if config.app else "configured-app" | |
| mcp_config = { | |
| "mcpServers": { | |
| app_name: { | |
| "url": f"{server_url}/sse", | |
| "transport": "sse", | |
| "headers": {"Authorization": f"Bearer {effective_api_key}"}, | |
| } | |
| } | |
| } | |
| console.print( | |
| f"[bright_black]{json.dumps(mcp_config, indent=2)}[/bright_black]", | |
| soft_wrap=True, | |
| ) | |
| if config.appServerInfo: | |
| server_url = config.appServerInfo.serverUrl | |
| print_info(f"App Server URL: [link={server_url}]{server_url}[/link]") | |
| print_info( | |
| f"Use this configured app as an MCP server at {server_url}/sse\n\nMCP configuration example:" | |
| ) | |
| # Use the app name if available, otherwise use a simple default | |
| app_name = config.app.name if config.app else "configured-app" | |
| mcp_config = { | |
| "mcpServers": { | |
| app_name: { | |
| "url": f"{server_url}/sse", | |
| "transport": "sse", | |
| "headers": {"Authorization": "Bearer <YOUR_API_KEY>"}, | |
| } | |
| } | |
| } | |
| console.print( | |
| f"[bright_black]{json.dumps(mcp_config, indent=2)}[/bright_black]", | |
| soft_wrap=True, | |
| ) |
| mcp_config = { | ||
| "mcpServers": { | ||
| app_name: { | ||
| "url": f"{server_url}/sse", | ||
| "transport": "sse", | ||
| "headers": {"Authorization": f"Bearer {effective_api_key}"}, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.print( | ||
| f"[bright_black]{json.dumps(mcp_config, indent=2)}[/bright_black]", | ||
| soft_wrap=True, | ||
| ) |
There was a problem hiding this comment.
Do not print or log the raw API key (secret exposure).
The MCP config snippet logs the full Authorization header to console and logger via print_info. This leaks the user's API key into logs and terminals. Replace with a placeholder/env reference.
Apply this diff:
- mcp_config = {
+ mcp_config = {
"mcpServers": {
app_name: {
"url": f"{server_url}/sse",
"transport": "sse",
- "headers": {"Authorization": f"Bearer {effective_api_key}"},
+ # Never print secrets. Use an env placeholder in examples.
+ "headers": {"Authorization": "Bearer ${MCP_API_KEY}"},
}
}
}Optionally, if you want to show that a key is present, log a masked preview only in verbose mode (e.g., "sk_Abc…Xyz").
Committable suggestion skipped: line range outside the PR's diff.
| if verbose: | ||
| LOG_VERBOSE.set(True) | ||
| verbose = LOG_VERBOSE.get() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor duplicated verbose handling pattern.
This pattern appears in multiple commands (build.py, keys.py, config.py). Issues with current approach:
- If
verbose=False,LOG_VERBOSE.get()will fail unless initialized elsewhere (depends on import order) - Setting then immediately reading back seems redundant
- Code duplication across files
Consider extracting this pattern into a utility function:
# In src/mcp_agent/cli/utils/ux.py
def init_verbose(verbose_flag: bool) -> bool:
"""Initialize and return verbose state."""
if verbose_flag:
LOG_VERBOSE.set(True)
return LOG_VERBOSE.get(False) # Safe defaultThen in commands:
verbose = init_verbose(verbose)This centralizes the logic and handles the default case safely.
🤖 Prompt for AI Agents
In src/mcp_agent/cli/commands/build.py around lines 204-206, the current verbose
handling sets LOG_VERBOSE when verbose is True then immediately reads
LOG_VERBOSE.get(), which duplicates logic, can fail if LOG_VERBOSE is
uninitialized, and is duplicated across commands; extract this into a utility
function (e.g., src/mcp_agent/cli/utils/ux.py:init_verbose(verbose_flag: bool)
-> bool) that sets LOG_VERBOSE when verbose_flag is True and returns
LOG_VERBOSE.get(False) to provide a safe default, then replace the three-line
pattern in build.py with a single call: verbose = init_verbose(verbose), and do
the same in keys.py and config.py to centralize behavior and avoid import-order
failures.
|
|
||
| from contextvars import ContextVar | ||
|
|
||
| LOG_VERBOSE = ContextVar("log_verbose") |
There was a problem hiding this comment.
Critical: ContextVar requires default value to prevent runtime errors.
LOG_VERBOSE.get() is called without a default argument in print_verbose (line 76) and throughout the codebase. If the context value hasn't been set, this will raise LookupError.
Apply this diff to provide a safe default:
-LOG_VERBOSE = ContextVar("log_verbose")
+LOG_VERBOSE = ContextVar("log_verbose", default=False)📝 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.
| LOG_VERBOSE = ContextVar("log_verbose") | |
| LOG_VERBOSE = ContextVar("log_verbose", default=False) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/utils/ux.py around line 14, the ContextVar LOG_VERBOSE is
declared without a default which makes LOG_VERBOSE.get() raise LookupError if
not set; change the declaration to provide a safe default (e.g., LOG_VERBOSE =
ContextVar("log_verbose", default=False)) so callers can safely call
LOG_VERBOSE.get() without a default argument, and scan the file for other
ContextVar declarations used similarly to give them appropriate defaults.
petersonbill64
force-pushed
the
feat/cli-log-hygiene
branch
from
fe5fea4 to
58c6ccd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/mcp_agent/cli/cloud/commands/configure/main.py (1)
298-321: Critical: Do not print API keys in generated config.This is a duplicate of a previously flagged issue. Printing
effective_api_keyto the console creates a high-severity secret exposure risk (terminal history, logs, screenshots, screen shares).Replace the actual key with a placeholder:
mcp_config = { "mcpServers": { app_name: { "url": f"{server_url}/sse", "transport": "sse", - "headers": {"Authorization": f"Bearer {effective_api_key}"}, + "headers": {"Authorization": "Bearer <YOUR_API_KEY>"}, } } }Alternatively, reference an environment variable:
"Bearer ${MCP_API_KEY}"to guide users toward secure practices.src/mcp_agent/cli/utils/ux.py (2)
14-14: Critical: ContextVar requires default value to prevent runtime errors.This is a duplicate of a previously flagged issue.
LOG_VERBOSE.get()is called without a default argument throughout the codebase (line 76, and in other files). If the context value hasn't been set, this will raiseLookupError.Apply this diff:
-LOG_VERBOSE = ContextVar("log_verbose") +LOG_VERBOSE = ContextVar("log_verbose", default=False)
222-222: Fix type hint: useAnyinstead ofany.This is a duplicate of a previously flagged issue. The type annotation uses lowercase
anywhich is not valid. Should beAnyfrom the typing module (already imported on line 5).Apply this diff:
- deployment_properties_display_info: List[Tuple[str, any, bool]], + deployment_properties_display_info: List[Tuple[str, Any, bool]],src/mcp_agent/cli/mcp_app/api_client.py (1)
375-390: Critical: Missing await on async call.This is a duplicate of a previously flagged issue (marked as addressed in commit fe5fea4, but still present). Line 389 calls the async method
get_app_by_name()without awaiting it, which will return a coroutine object instead of the MCPApp. This will causeapp.appIdto fail.Apply this diff:
async def get_app_id_by_name(self, name: str) -> Optional[str]: """Get the app ID for a given app name via the API. Args: name: The name of the MCP App Returns: Optional[str]: The UUID of the MCP App, or None if not found Raises: ValueError: If the name is empty or invalid httpx.HTTPStatusError: If the API returns an error httpx.HTTPError: If the request fails """ - app = self.get_app_by_name(name) + app = await self.get_app_by_name(name) return app.appId if app else Nonesrc/mcp_agent/cli/cloud/commands/deploy/main.py (1)
442-455: Critical: Do not print or log the raw API key.This is a duplicate of a previously flagged issue. The MCP config snippet logs the full Authorization header with
effective_api_keyto console. This leaks the user's API key into logs, terminal history, and screenshots.Replace with a placeholder or environment variable reference:
mcp_config = { "mcpServers": { app_name: { "url": f"{server_url}/sse", "transport": "sse", - "headers": {"Authorization": f"Bearer {effective_api_key}"}, + "headers": {"Authorization": "Bearer ${MCP_API_KEY}"}, } } }This guides users to store the key securely in environment variables rather than hardcoding it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
logs/marketing-20251022_200928.jsonl(1 hunks)src/mcp_agent/cli/cloud/commands/configure/main.py(9 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(11 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(7 hunks)src/mcp_agent/cli/mcp_app/api_client.py(2 hunks)src/mcp_agent/cli/utils/ux.py(8 hunks)tests/cli/commands/test_configure.py(7 hunks)tests/cli/commands/test_deploy_command.py(13 hunks)
✅ Files skipped from review due to trivial changes (1)
- logs/marketing-20251022_200928.jsonl
🧰 Additional context used
🧬 Code graph analysis (6)
src/mcp_agent/cli/mcp_app/api_client.py (3)
src/mcp_agent/core/context.py (1)
name(185-188)src/mcp_agent/server/app_server.py (1)
app(135-137)src/mcp_agent/cli/mcp_app/mock_client.py (1)
get_app_id_by_name(36-45)
src/mcp_agent/cli/cloud/commands/deploy/main.py (7)
src/mcp_agent/cli/mcp_app/api_client.py (4)
MCPApp(23-32)get_app_by_name(351-373)create_app(136-175)update_app(259-311)src/mcp_agent/cli/utils/retry.py (2)
retry_async_with_exponential_backoff(101-161)RetryError(12-20)src/mcp_agent/cli/utils/ux.py (4)
print_deployment_header(216-265)print_info(44-62)print_success(80-92)print_verbose(65-77)src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(108-115)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/core/api_client.py (2)
UnauthenticatedError(9-12)get(103-112)src/mcp_agent/cli/mcp_app/mock_client.py (2)
create_app(91-138)update_app(140-169)
src/mcp_agent/cli/cloud/commands/configure/main.py (4)
src/mcp_agent/cli/utils/ux.py (3)
print_verbose(65-77)print_configuration_header(268-306)print_info(44-62)src/mcp_agent/cli/mcp_app/api_client.py (1)
configure_app(430-467)src/mcp_agent/cli/mcp_app/mock_client.py (1)
configure_app(171-222)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-10)
tests/cli/commands/test_configure.py (2)
src/mcp_agent/cli/cloud/commands/configure/main.py (1)
configure_app(44-323)src/mcp_agent/cli/mcp_app/mock_client.py (1)
configure_app(171-222)
tests/cli/commands/test_deploy_command.py (3)
src/mcp_agent/cli/mcp_app/api_client.py (4)
get_app_id_by_name(375-390)create_app(136-175)update_app(259-311)get_app_by_name(351-373)src/mcp_agent/cli/mcp_app/mock_client.py (3)
get_app_id_by_name(36-45)create_app(91-138)update_app(140-169)src/mcp_agent/cli/cloud/commands/apps/update/main.py (1)
update_app(20-126)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
src/mcp_agent/cli/utils/git_utils.py (3)
get_git_metadata(52-85)compute_directory_fingerprint(130-167)utc_iso_now(88-89)src/mcp_agent/cli/utils/ux.py (2)
print_info(44-62)print_verbose(65-77)
🔇 Additional comments (8)
src/mcp_agent/cli/cloud/commands/configure/main.py (1)
111-112: LGTM: Verbose handling correctly implemented.The verbose flag properly sets the LOG_VERBOSE context variable, enabling verbose output throughout the command execution.
tests/cli/commands/test_configure.py (1)
31-32: LGTM: Test updates correctly reflect API changes.The test updates properly handle:
- Mock context (ctx) as first parameter
- New verbose parameter (default False)
- Mock app.name attribute for metadata presence
- Patched typer.confirm for non-interactive testing
These changes align with the updated configure_app signature and interactive confirmation flow.
Also applies to: 51-51, 55-57, 71-74, 98-98, 314-315, 355-355
tests/cli/commands/test_deploy_command.py (1)
163-163: LGTM: Async mock updates correctly handle API changes.The test updates properly reflect:
- AsyncMock for async API client methods (get_app_id_by_name, get_app_by_name, create_app, update_app)
- Flexible assertions that check either create_app or update_app based on deployment path
- Mock app objects with proper attributes
- verbose parameter in deploy invocations
These changes align with the refactored deployment flow that supports both create and update paths.
Also applies to: 167-168, 303-303, 307-308, 381-381, 385-386, 452-453, 457-458, 520-520, 524-525, 936-943, 948-949, 971-971
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
1-1: LGTM: Verbose logging and UI improvements.The changes properly implement:
- Consistent use of
print_verbosefor debug-level information- Consolidated file listing output for cleaner verbose logs
- Updated Progress UI with dedicated SpinnerColumn for consistent styling
- Appropriate spinner frame adjustment on success
These improvements align with the PR's goal of improving log hygiene.
Also applies to: 19-25, 195-195, 266-271, 308-310, 371-371, 373-375, 403-404
src/mcp_agent/cli/cloud/commands/deploy/main.py (4)
172-173: LGTM: Verbose handling correctly implemented.The verbose flag properly:
- Sets LOG_VERBOSE context variable for command-scoped verbose output
- Uses print_verbose for internal/debug messages
- Maintains clean default output while enabling detailed logging when needed
This aligns with the PR's goal of improving log hygiene.
Also applies to: 197-201, 221-225
227-300: LGTM: Deployment diff tracking improves UX.The new deployment properties tracking system provides clear visibility into:
- Which app properties are being updated
- Differences between existing and new values
- Visual distinction (yellow vs. gray) for changed vs. unchanged properties
This enhancement helps users understand the deployment's impact before proceeding.
311-324: LGTM: Interactive confirmation flow with timestamps.The confirmation prompt and timestamped messages improve UX by:
- Allowing users to review changes before proceeding
- Respecting non-interactive mode for CI/CD environments
- Providing clear audit trail with UTC timestamps
This aligns with the PR objectives for better user interaction.
326-344: LGTM: Create vs. update logic is clear and correct.The branching logic properly handles:
- Creating new apps with provided metadata
- Updating existing apps only when update_payload has values
- Appropriate success messages for each path
- Verbose logging of app IDs
The flow is well-structured and maintainable.
| # Print output for debugging | ||
| if result.exit_code != 0: | ||
| print(f"Command failed with exit code {result.exit_code}") | ||
| print(f"Output: {result.stdout}") | ||
| print(f"Error: {result.stderr}") | ||
|
|
||
| assert result.exit_code == 0, f"Command failed: {result.stdout}\n{result.stderr}" | ||
|
|
||
| # Check which methods were called | ||
| print(f"create_app called: {mock_client.create_app.called}") | ||
| print(f"create_app call count: {mock_client.create_app.call_count}") | ||
| print(f"update_app called: {mock_client.update_app.called}") | ||
| print(f"update_app call count: {mock_client.update_app.call_count}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug print statements from test.
These print statements appear to be debugging artifacts and should be removed before merging.
- # Print output for debugging
- if result.exit_code != 0:
- print(f"Command failed with exit code {result.exit_code}")
- print(f"Output: {result.stdout}")
- print(f"Error: {result.stderr}")
-
assert result.exit_code == 0, f"Command failed: {result.stdout}\n{result.stderr}"
-
- # Check which methods were called
- print(f"create_app called: {mock_client.create_app.called}")
- print(f"create_app call count: {mock_client.create_app.call_count}")
- print(f"update_app called: {mock_client.update_app.called}")
- print(f"update_app call count: {mock_client.update_app.call_count}")📝 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.
| # Print output for debugging | |
| if result.exit_code != 0: | |
| print(f"Command failed with exit code {result.exit_code}") | |
| print(f"Output: {result.stdout}") | |
| print(f"Error: {result.stderr}") | |
| assert result.exit_code == 0, f"Command failed: {result.stdout}\n{result.stderr}" | |
| # Check which methods were called | |
| print(f"create_app called: {mock_client.create_app.called}") | |
| print(f"create_app call count: {mock_client.create_app.call_count}") | |
| print(f"update_app called: {mock_client.update_app.called}") | |
| print(f"update_app call count: {mock_client.update_app.call_count}") | |
| assert result.exit_code == 0, f"Command failed: {result.stdout}\n{result.stderr}" |
🤖 Prompt for AI Agents
In tests/cli/commands/test_deploy_command.py around lines 200 to 212, remove the
debugging print statements that output command result and mock call details;
instead leave the existing assertion(s) to surface failures and, if needed,
replace prints with proper assertions on mock_client.create_app/update_app
called and call_count—delete the four prints for
result.exit_code/result.stdout/result.stderr and the four prints for mock_client
call status, and add/assert expected boolean/call_count values if the test
requires explicit checks.
…lastmile-ai#572) * Unify verbose logging configuration flags * Use context var to define unconditional verbose logger * Prune some extraneous logs
Summary by CodeRabbit
Removed Features
New Features
Documentation
Tests