Reducing print messages, moving msg to verbose #546
Conversation
WalkthroughRefactors deploy CLI output and verbosity: replaces many prints with VERBOSE-gated logs, removes Rich Panel usage, standardizes “mcp-agent” branding, adjusts app name/description selection messaging, clarifies authentication guidance, tweaks app existence and secrets reuse prompts (including non-interactive handling), and limits wrangler deploy info logs to verbose mode. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as mcp-agent deploy CLI
participant Auth as Auth Check
participant API as Cloud API
participant W as Wrangler
U->>C: run deploy [--app ...] [--api-key ...]
Note over C: Print header (no Panel)\nVerbose gating for info logs
C->>Auth: verify credentials
alt not authenticated
C-->>U: Guidance: login/env var/flag (multi-line)
C->>U: Exit (retriable=false)
else authenticated
C->>C: Resolve app_name/description (config/default)
C->>API: get or create app
alt app not found
API-->>C: created app (id)
opt VERBOSE
C-->>U: Created app id
end
else app exists
C-->>U: Using existing app (name, short id)
end
C->>C: Secrets handling
alt non-interactive and deployed secrets exist
C-->>U: Reuse deployed secrets (non-interactive)
else interactive
C->>U: Prompt reuse/regenerate secrets
U-->>C: Choice
end
C->>W: wrangler_deploy(...)
opt VERBOSE
W-->>C: Info logs (ignore file, deployed hash)
end
C-->>U: Deployment complete (concise)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
Screen.Recording.2025-10-07.at.7.45.05.PM.mov |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| deployed_secrets_file = None # Will trigger re-processing | ||
| else: | ||
| print_info(f"Using existing '{MCP_DEPLOYED_SECRETS_FILENAME}'.") | ||
| deployed_secrets_file = None # Will trigger re-processing) |
There was a problem hiding this comment.
There's a syntax error in this line - an extra closing parenthesis after the comment. Additionally, the original code included an else clause that informed users when they chose to reuse existing secrets, but this message has been removed in the new version.
Consider:
- Removing the trailing parenthesis:
deployed_secrets_file = None # Will trigger re-processing - Adding back the informational message in the
elseclause with a verbose check:
else:
if settings.VERBOSE:
print_info(f"Using existing '{MCP_DEPLOYED_SECRETS_FILENAME}'.")This maintains the user feedback while following the PR's goal of moving non-critical messages to verbose mode.
| deployed_secrets_file = None # Will trigger re-processing) | |
| deployed_secrets_file = None # Will trigger re-processing | |
| else: | |
| if settings.VERBOSE: | |
| print_info(f"Using existing '{MCP_DEPLOYED_SECRETS_FILENAME}'.") |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/mcp_agent/cli/cloud/commands/deploy/main.py (4)
265-283: Clearer secrets handling with minor typo.The updated messaging for secrets handling is much clearer, especially the non-interactive mode message. The verbose gating is appropriate.
Fix the typo in the comment on line 279:
- deployed_secrets_file = None # Will trigger re-processing) + deployed_secrets_file = None # Will trigger re-processing
291-291: Remove commented-out code.Commented-out code should be removed to keep the codebase clean. Version control preserves the history if this needs to be referenced later.
- # print_info("Processing secrets file...")
355-363: Inconsistent label formatting in output block.Line 355 omits the colon after "App Name" while lines 362-363 use colons ("App URL:", "App Status:"). Consider standardizing the format for consistency.
Apply this diff to add consistency:
- print_info(f"App Name '{app_name}'") + print_info(f"App Name: '{app_name}'")
222-236: LGTM — short IDs improve UX and slicing is safe
Ensure that the API always returns IDs in theapp_<UUID>format (i.e. at least 12 characters) or add a simple length check before slicing to guarantee meaningful output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcp_agent/cli/cloud/commands/deploy/main.py(10 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(2 hunks)src/mcp_agent/cli/utils/ux.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp_agent/cli/cloud/commands/deploy/main.py (3)
src/mcp_agent/cli/utils/ux.py (2)
print_info(32-49)print_success(52-63)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/core/utils.py (1)
run_async(12-27)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)
🔇 Additional comments (9)
src/mcp_agent/cli/utils/ux.py (1)
199-199: LGTM! Branding consistency improvement.The title change from "MCP Agent Deployment" to "mcp-agent deployment" aligns with the PR's goal of standardizing the product name to lowercase "mcp-agent" across user-facing strings.
src/mcp_agent/cli/cloud/commands/deploy/main.py (7)
1-1: LGTM! Branding consistency in docstrings.Updated references from "MCP Agent" to "mcp-agent" align with the PR's standardization goals.
Also applies to: 131-131
174-182: LGTM! Clearer app name selection messaging.The updated logic clearly communicates which app name is being used, whether from config or defaulting to 'default'. The removal of the description print reduces noise while still using the description internally (line 186).
200-207: LGTM! Significantly improved authentication guidance.The multi-line error message provides clear, actionable options for users to authenticate, replacing the previous terse message. This is a substantial UX improvement.
209-210: LGTM! Appropriate verbose gating.Moving these informational messages behind
settings.VERBOSEreduces noise in standard mode while keeping them available for debugging. This aligns with the PR's objective of reducing print messages.Also applies to: 216-217
304-308: LGTM! Appropriate verbose detail gating.The success message is always shown (line 304), while implementation details (transformed file path) are appropriately gated behind
settings.VERBOSE. This provides a good balance between user feedback and information overload.
322-326: LGTM! Enhanced git tag metadata.Including the app name and ID in the git tag message improves traceability of deployments. The formatting is consistent with other messages in the file.
460-461: LGTM! Consistent verbose gating for retry messaging.Gating the retry configuration message behind
settings.VERBOSEis appropriate and consistent with the PR's goal of reducing informational output in standard mode.src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
187-188: LGTM! Consistent verbose gating in wrangler wrapper.Both changes appropriately gate informational messages behind
settings.VERBOSE, reducing noise in standard mode while preserving debugging information when needed. This is consistent with the verbose gating applied throughout the deployment flow.Also applies to: 298-299
| ) | ||
|
|
||
| print_info(f"App ID: {app_id}") | ||
| print_info(f"App Name '{app_name}'") |
There was a problem hiding this comment.
Can we keep this as ID? Otherwise we lose that critical info for debugging
| secrets_transformed_path = None | ||
| if secrets_file and not deployed_secrets_file: | ||
| print_info("Processing secrets file...") | ||
| # print_info("Processing secrets file...") |
There was a problem hiding this comment.
| # print_info("Processing secrets file...") | |
| if settings.VERBOSE: | |
| print_info("Processing secrets file...") |
| if settings.VERBOSE: | ||
| print_info(f"New app id: `{app_id}`") | ||
| else: | ||
| short_id = f"{app_id[:8]}…" |
There was a problem hiding this comment.
| short_id = f"{app_id[:8]}…" | |
| short_id = f"{app_id[:8]}…{app_id[-4:]}" |
recommend including some substring from the end since the first 8 digits can definitely match other ids
3 participants
Summary by CodeRabbit
Style
Chores