refactor(cli): remove deprecated deploy command - #10576
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (11)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe deprecated ChangesDeploy command removal
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to The PR removes the deprecated deploy workflow, updates command discovery, documentation, and credential migration coverage, and reports successful targeted validation and end-to-end checks. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The changes remain within the linked issue scope. The ALLOWED_CHAT_IDS cleanup is related legacy credential-migration cleanup for the removed deployment path. No unrelated changes to the installer, onboarding, Brev Launchable workflows, or E2E infrastructure are shown. Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 7 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 0a873aa in the TypeScript / code-coverage/cliThe overall line coverage in commit 0a873aa in the Show a line coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-10576.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
PR Review Advisor finished for commit |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Verdict
Request changes. The clean-build diff is internally consistent, but the supported source-checkout upgrade path retains the deleted command. The PR therefore does not yet satisfy the accepted outcome that the CLI no longer registers or displays deploy.
Blocking finding
[P1] Make normal CLI builds remove retired command artifacts
Location: test/package-contract/cli/command-registry.test.ts:125
The new assertion passes only after clean:cli. TypeScript does not remove emitted files for deleted sources, while build:cli does not clean dist. The source-checkout installer and contributor setup both invoke build:cli without cleaning first.
Reproduction against commit 0a873aa:
- Clean and build the merge-base commit c166756.
- Switch to the latest PR commit.
- Run npm run build:cli without clean:cli.
- Observe that dist/commands/deploy.js, dist/lib/actions/deploy.js, dist/lib/deploy/index.js, and deploy metadata remain.
In this state, nemoclaw deploy --help still displays the deprecated Brev command. The package-contract test fails three assertions, and deploy remains a global token, so a sandbox named deploy cannot use the sandbox-first grammar. Invoking the stale command exits with runDeployAction is not a function; it stops before credential transfer or remote execution.
Make the normal build prune retired outputs, or explicitly retire these artifacts before metadata generation. Add an upgrade regression that seeds the previous compiled deploy artifacts before running the normal build.
Root-cause sibling paths checked:
- package.json build:cli retains existing dist output.
- scripts/install.sh source-checkout installation calls build:cli without cleaning.
- scripts/dev-setup.sh contributor setup calls build:cli without cleaning.
- Fresh managed-install clones start without dist and are not affected.
Security rubric
| Category | Verdict | Reason |
|---|---|---|
| Secrets and Credentials | PASS | The stale public command fails before the removed credential-transfer action executes. |
| Input Validation and Data Sanitization | PASS | The clean build correctly accepts deploy as a normalized sandbox name. |
| Authentication and Authorization | PASS | No authentication or authorization boundary changes. |
| Dependencies and Third-Party Libraries | PASS | No dependency changes. |
| Error Handling and Logging | WARNING | The stale command exposes an internal missing-function error. |
| Cryptography and Data Protection | PASS | No cryptographic or protected-data changes. |
| Configuration and Security Headers | WARNING | The normal build configuration retains deleted executable artifacts. |
| Security Testing | WARNING | Clean-build tests do not cover the supported upgrade state. |
| System Security | WARNING | Clean and upgraded installations expose different command and routing surfaces. |
Validation
- Clean PR build: 957 focused tests passed.
- Upgrade reproduction: 3 of 825 package-contract assertions failed.
- Merge with current main b80e47c: repository checks, 957 focused tests, and npm run docs passed after a clean build.
- GitHub required checks pass on the latest PR commit.
All 20 changed files and the adjacent build, installer, command-discovery, routing, credential-migration, and documentation-generation paths were reviewed.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Request changes on exact head 0a873aab5dee5b91dba067c8dfc8e24d6b1abf34. I independently reproduced the supported source-checkout upgrade failure: compiling the previous version, switching to this head, and running the normal non-clean build leaves the deleted dist/commands/deploy.js, dist/lib/actions/deploy.js, and dist/lib/deploy/ artifacts. deploy is still discovered as the old Brev command, but the newly compiled global action no longer exports runDeployAction, so upgraded checkouts expose a broken command and do not route a sandbox named deploy like fresh installs do. build:cli, scripts/install.sh, and scripts/dev-setup.sh all permit this path. Make the normal build prune retired outputs and add an upgrade regression that seeds the previous compiled artifacts before the normal build. Clean-head verification otherwise passed: 957 focused tests, repository architecture checks, docs build, required CI, CodeRabbit, and all nine Advisor specialists.
| expect(discoveredIds.has(command.commandId), command.usage).toBe(true); | ||
| }); | ||
|
|
||
| it("does not discover the removed deploy command (#10572)", () => { |
There was a problem hiding this comment.
[P1] This proves only a clean build. A normal source-checkout upgrade keeps deleted TypeScript outputs because build:cli does not clean dist; I reproduced this head retaining the old deploy command and then failing at runDeployAction is not a function. Add a regression that seeds the previous deploy artifacts, runs the normal build path, and proves command discovery, global routing, and deploy --help are clean.
3 participants
Outcome
Remove the deprecated
nemoclaw deployBrev compatibility command and its dedicated implementation.The CLI no longer registers or displays that command;
deployis available only as a sandbox name in the sandbox-first grammar.Reason
The maintained remote-host workflow provisions the host separately, runs the hosted installer, and uses
nemoclaw onboardon that host.Retaining the deprecated wrapper preserved an unsupported Brev provisioning, SSH, and credential-transfer path with no current consumer.
Related issues
Fixes #10572
Changes
deployfrom the reserved sandbox-name list.ALLOWED_CHAT_IDSfrom legacy credential migration because its only consumer was the removed deploy path.Verification
vitest run --project cli src/commands/simple-global-oclif-adapters.test.ts src/lib/actions/global.test.ts src/lib/onboard/entry-options.test.ts— 3 files passed, 56 tests passed.vitest run --project integration test/cli/onboard-compatibility.test.ts— 1 file passed, 18 tests passed.vitest run --project package-contract test/package-contract/cli/command-registry.test.ts— 1 file passed, 824 tests passed.vitest run --project cli src/commands/credentials.test.ts— 1 file passed, 11 tests passed.vitest run --project integration test/credentials/credentials.test.ts— 1 file passed, 48 tests passed.npm run typecheck:cli— passed after the initial change and after advisor remediation.npm run docs— passed after the initial change and after advisor remediation, with 0 errors and 2 existing warnings.npm run checks:repository— passed, including the lowered source-architecture budget.targets=full-e2e— passed on commit0a873aa; install, onboard, CLI operations, hosted and sandbox inference, launch turns, logs, performance budgets, and cleanup passed with 0 unhandled errors.npm run check— not successful locally: 83 unrelated CLI/integration failures and 1 unrelated plugin failure occurred during concurrent worktree tests, dominated by fixed-budget timeouts, cascading temporary-state failures, and host-state leakage. This result is not used as validation; current-base CI is the broad-gate authority.Review notes
The PR Review Advisor identified two valid completion gaps on commit
742ca2a: the obsoleteALLOWED_CHAT_IDSmigration key and the stale troubleshooting reserved-name entry.Commit
0a873aaaddresses both findings and adds the credential migration regression.The second advisor pass on
0a873aafound no remaining issue across behavior, reduction, dependency use, architecture, documentation, migration, operations, test design, or trust.The change removes a sensitive remote-execution and credential-transfer path; it adds no credential flow, network authority, external write, migration, or compatibility fallback.
Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
Breaking Changes
deploycommand and its deployment workflows.deployname can now be used as a sandbox name and is normalized like other names.Documentation
Security & Maintenance
ALLOWED_CHAT_IDSfrom credential staging and storage.