673 climcp deploy#674
Merged
Merged
Conversation
Closed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDeploy command timing, idempotency, and retryability infrastructure: the CLI now increases the default deploy wait from 120 to 720 seconds, sends unique idempotency keys per invocation to prevent accidental replay, classifies HTTP errors as retryable with Retry-After parsing, implements bounded automatic retry on rate limits for per-settings deploys, emits stable machine-readable error codes, and removes empty token placeholders from deployment creation output. Tests, docs, and MCP tool descriptions align to these semantics. ChangesDeploy command timeout, idempotency, and error handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mcps/mcp/src/__tests__/tools/deploy-manage.test.ts`:
- Around line 83-106: The test's expectation for the default wait budget is
wrong: update the assertion that checks description for "120" to the correct
default (either "720" for seconds or "12" for minutes); locate the test using
registerDeployTool(...) and server.getTool('deploy_manage')!, inspect the
config.description string (variable description) and replace
expect(description).toContain('120') with expect(description).toContain('720')
or expect(description).toContain('12') to match the corrected deploy_manage
documentation.
In `@packages/mcps/mcp/src/tools/deploy-manage.ts`:
- Line 23: Update the hardcoded "120-second" timeout text to "720-second" in the
deploy tool help/description string (the string beginning "deploy waits for the
deployment to reach a terminal status...") and update the corresponding unit
test assertion in deploy-manage.test (the test that currently expects a
120-second budget) to expect 720 seconds instead; ensure both description
occurrences and the test expectation are changed so the tool docs and test align
with the new 720-second default.
In `@packages/web/destinations/snowplow/src/types/index.ts`:
- Around line 18-27: The comment and imports incorrectly claim all six Snowplow
types are used in local type positions; update types/index.ts to only import
Action in a local type position (import type { Action } from
'`@snowplow/browser-plugin-snowplow-ecommerce`') and change Product, Cart, Refund,
User, and Page to be re-exported directly (export type { Product, Cart, Refund,
User, Page } from '`@snowplow/browser-plugin-snowplow-ecommerce`'); also update
the comment to reflect that only Action is used locally and the others are
simple re-exports so the imports can be simplified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 764b8298-e6a6-4dec-bd77-646a68ee17b9
📒 Files selected for processing (15)
.changeset/cli-honest-deploy-waits.md.changeset/mcp-deploy-manage-contract.mdpackages/cli/src/__tests__/unit/commands/deploy.test.tspackages/cli/src/__tests__/unit/commands/deployments.test.tspackages/cli/src/cli.tspackages/cli/src/commands/deploy/index.tspackages/cli/src/commands/deployments/index.tspackages/cli/src/core/__tests__/api-error.test.tspackages/cli/src/core/api-error.tspackages/mcps/mcp/src/__tests__/tools/deploy-manage.test.tspackages/mcps/mcp/src/tools/deploy-manage.tspackages/web/destinations/snowplow/src/types/index.tsskills/walkeros-using-cli/commands-reference.mdwebsite/docs/apps/cli.mdxwebsite/docs/apps/mcp.mdx
| const DESCRIPTION = | ||
| 'Deploy walkerOS flows and manage deployments. ' + | ||
| 'For get/delete actions pass flowId (required) plus optional slug to disambiguate when a flow has multiple active deployments. ' + | ||
| 'deploy waits for the deployment to reach a terminal status by default (wait=true), with a 120-second budget; pass wait=false to return immediately with the deployment id. ' + |
There was a problem hiding this comment.
Timeout budget across MCP tool and test not updated from 120 to 720 seconds.
The MCP tool description (deploy-manage.ts lines 23, 54) still claims a "120-second budget," and the test (deploy-manage.test.ts line 91) validates that incorrect value. The correct default is 12 minutes (720 seconds), as documented in cli.mdx and skills/walkeros-using-cli/commands-reference.md and confirmed by the AI summary ("increased default wait from 120 to 720 seconds"). Both the tool description and test must be updated together to reflect the new 720-second (12-minute) budget.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/mcps/mcp/src/tools/deploy-manage.ts` at line 23, Update the
hardcoded "120-second" timeout text to "720-second" in the deploy tool
help/description string (the string beginning "deploy waits for the deployment
to reach a terminal status...") and update the corresponding unit test assertion
in deploy-manage.test (the test that currently expects a 120-second budget) to
expect 720 seconds instead; ensure both description occurrences and the test
expectation are changed so the tool docs and test align with the new 720-second
default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
deploy createno longer prints an empty token placeholder or embeds a token in sample outputDocumentation