refactor(cli): centralize oclif exit helpers#3615
Conversation
📝 WalkthroughWalkthroughThis PR centralizes CLI exit handling by adding a CommandExitResult type and NemoClawCommand helpers (setExitCode, failWithLines, applyExitResult), migrates multiple commands to use those helpers instead of direct process.exit/console.error patterns, and updates tests to assert via process.exitCode. ChangesUnified exit-result framework and adoption
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Selective E2E Results — ✅ All requested jobs passedRun: 25933769295
|
…ntions' into refactor/oclif-exit-helper-conventions
Selective E2E Results — ✅ All requested jobs passedRun: 25933940120
|
![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
🤖 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 `@src/lib/commands/sandbox/skill.test.ts`:
- Around line 16-27: The test sets a spy `const error = vi.spyOn(console,
"error")` but never restores it, which can leak into other tests; update the
finally block in this test to restore the spy (e.g., call `error.mockRestore()`
or `error.mockReset()` in the finally alongside restoring `process.exitCode`) so
the global `console.error` is returned to its original behavior after calling
`SkillCliCommand.run([], rootDir)` and assertions involving
`sandboxSkillInstall`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d231a642-e0b6-4be0-96f2-e5bead8ff83a
📒 Files selected for processing (5)
src/lib/commands/global-oclif-command-adapters.test.tssrc/lib/commands/inference/get.tssrc/lib/commands/inference/set.tssrc/lib/commands/sandbox/skill.test.tssrc/lib/commands/sandbox/skill.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/commands/inference/get.ts
| const error = vi.spyOn(console, "error").mockImplementation(() => undefined); | ||
| const previousExitCode = process.exitCode; | ||
| process.exitCode = undefined; | ||
|
|
||
| try { | ||
| await expect(SkillCliCommand.run([], rootDir)).resolves.toBeUndefined(); | ||
| expect(process.exitCode).toBe(2); | ||
| expect(error).toHaveBeenCalledWith("Missing required sandboxName for skill."); | ||
| expect(sandboxSkillInstall).not.toHaveBeenCalled(); | ||
| } finally { | ||
| process.exitCode = previousExitCode; | ||
| } |
There was a problem hiding this comment.
Restore the console.error spy in test cleanup.
The test currently restores process.exitCode but leaves the global console.error spy active, which can leak into subsequent tests if global restore behavior changes.
Suggested fix
} finally {
+ error.mockRestore();
process.exitCode = previousExitCode;
}🤖 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 `@src/lib/commands/sandbox/skill.test.ts` around lines 16 - 27, The test sets a
spy `const error = vi.spyOn(console, "error")` but never restores it, which can
leak into other tests; update the finally block in this test to restore the spy
(e.g., call `error.mockRestore()` or `error.mockReset()` in the finally
alongside restoring `process.exitCode`) so the global `console.error` is
returned to its original behavior after calling `SkillCliCommand.run([],
rootDir)` and assertions involving `sandboxSkillInstall`.
Selective E2E Results — ✅ All requested jobs passedRun: 25934788619
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/commands/credentials/list.ts (1)
31-35: ⚡ Quick winReuse centralized credentials failure lines to avoid message drift.
This branch duplicates gateway recovery guidance text that now exists in
credentialsGatewayRecoveryFailureLines("query"). Reusing the helper keeps credentials flows consistent.Suggested diff
-import { isBridgeProviderName, recoverGatewayOrExit } from "./common"; +import { + credentialsGatewayRecoveryFailureLines, + isBridgeProviderName, + recoverGatewayOrExit, +} from "./common"; @@ if (result.status !== 0) { - this.failWithLines([ - " Could not query OpenShell gateway. Is it running?", - ` Run 'openshell gateway start --name nemoclaw' or '${CLI_NAME} onboard' first.`, - ]); + this.failWithLines(credentialsGatewayRecoveryFailureLines("query")); return; }🤖 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 `@src/lib/commands/credentials/list.ts` around lines 31 - 35, The failure branch in list.ts duplicates gateway recovery text; replace the hard-coded array passed to this.failWithLines([...]) with a call to credentialsGatewayRecoveryFailureLines("query") so the command reuses the centralized message; locate the failing branch that currently calls this.failWithLines and swap its argument to this.failWithLines(credentialsGatewayRecoveryFailureLines("query")) (keeping the early return) to ensure consistency.
🤖 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.
Nitpick comments:
In `@src/lib/commands/credentials/list.ts`:
- Around line 31-35: The failure branch in list.ts duplicates gateway recovery
text; replace the hard-coded array passed to this.failWithLines([...]) with a
call to credentialsGatewayRecoveryFailureLines("query") so the command reuses
the centralized message; locate the failing branch that currently calls
this.failWithLines and swap its argument to
this.failWithLines(credentialsGatewayRecoveryFailureLines("query")) (keeping the
early return) to ensure consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22f3dea5-2dae-4b88-a00b-99df1742178d
📒 Files selected for processing (7)
src/lib/commands/credentials/common.tssrc/lib/commands/credentials/list.tssrc/lib/commands/credentials/reset.tssrc/lib/commands/debug.tssrc/lib/commands/sandbox/config/get.tssrc/lib/commands/uninstall.tstest/credentials-cli-command.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/commands/credentials/reset.ts
- test/credentials-cli-command.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 25935331096
|
Selective E2E Results — ✅ All requested jobs passedRun: 25939118076
|
Selective E2E Results — ✅ All requested jobs passedRun: 25940218386
|
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Clean refactor — all four stated changes land cleanly and CI's green. Four small nits, none blocking:
src/lib/commands/debug.ts:67-70— theexitclosure is unreachable.runDebugCommandWithOptionsnever invokesparseDebugArgs, so it's dead today and becomes a footgun if anyone wires it back. Suggest dropping it.uninstall.ts:32-35+debug.ts:67—return undefined as neverlies to TS. Either keepprocess.exit(code)in the injected exit, or changerunUninstallCommand/parseDebugArgsreturn types offnever.sandbox/skill.ts:17+uninstall.ts:22—this.parsed = trueis oclif private-instance-field territory (suppresses the "did you forget to callparse()?" warning when readingthis.argvdirectly). Worth a 1-line comment so it survives the next cleanup pass.nemoclaw-oclif-command.test.ts:87-89—Object.create(TestCommand.prototype)intentionally bypasses the oclifCommandbase constructor. Worth documenting that, or it'll get "fixed" later.
Happy to approve once these land or you tell me you're shipping as-is — none of them block.
Selective E2E Results — ✅ All requested jobs passedRun: 25940869135
|
2 participants
Summary
Centralizes common oclif command exit handling in
NemoClawCommandnow that the command base is shared. This keeps command adapters from throwing oclifExitErrors for action-style results and standardizes multi-line failure output across command-owned failure paths.Changes
setExitCode,applyExitResult, andfailWithLineshelpers toNemoClawCommand.applyExitResult(...).Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit