fix(cli): tighten oclif compatibility routing#3599
Conversation
📝 WalkthroughWalkthroughRefactored multiple Oclif-based command classes to extend a shared ChangesCLI Command Inheritance and Oclif Config Branding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/nemoclaw.ts (1)
161-163: ⚡ Quick winHonor
--when detecting help flags.This helper now runs before oclif parsing, so treating any later
--help/-has metadata changes normal CLI semantics. A token after--should stay positional/pass-through instead of short-circuiting into front-controller help.Suggested change
function hasHelpFlag(args: readonly string[]): boolean { - return args.includes("--help") || args.includes("-h"); + for (const arg of args) { + if (arg === "--") { + return false; + } + if (arg === "--help" || arg === "-h") { + return true; + } + } + return false; }🤖 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/nemoclaw.ts` around lines 161 - 163, The hasHelpFlag helper treats any occurrence of "--help" or "-h" as a help request even if it appears after a "--" token; update hasHelpFlag to scan args only up to the first "--" (i.e., stop when you encounter "--") and return true only if "--help" or "-h" appears before that token so positional/pass-through args after "--" are ignored by the front-controller help detection.test/cli-oclif-compatibility.test.ts (1)
111-121: ⚡ Quick winMake the spawned alias-help check hermetic.
This child inherits
NEMOCLAW_DISABLE_AUTO_DISPATCHfrom the parent environment, which can silently skip the CLI path the test is supposed to cover. Adding a timeout here also prevents a routing regression from hanging the suite indefinitely.Suggested change
it("uses the alias binary name in native oclif help", () => { + const env = { ...process.env, NO_COLOR: "1" }; + delete env.NEMOCLAW_DISABLE_AUTO_DISPATCH; + const result = spawnSync( process.execPath, ["bin/nemohermes.js", "sandbox", "channels", "start", "--help"], { cwd: process.cwd(), encoding: "utf8", - env: { - ...process.env, - NO_COLOR: "1", - }, + env, + timeout: 10_000, }, );🤖 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 `@test/cli-oclif-compatibility.test.ts` around lines 111 - 121, The spawned CLI test is non-hermetic because the child inherits NEMOCLAW_DISABLE_AUTO_DISPATCH from the parent and can skip the path under test; update the spawnSync invocation that assigns result (the call to spawnSync with process.execPath and ["bin/nemohermes.js", "sandbox", "channels", "start", "--help"]) to explicitly set NEMOCLAW_DISABLE_AUTO_DISPATCH in the env (e.g. "0" or remove it) and add a timeout (e.g. timeout: 10000) to the spawnSync options so the test cannot hang indefinitely.
🤖 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/nemoclaw.ts`:
- Around line 161-163: The hasHelpFlag helper treats any occurrence of "--help"
or "-h" as a help request even if it appears after a "--" token; update
hasHelpFlag to scan args only up to the first "--" (i.e., stop when you
encounter "--") and return true only if "--help" or "-h" appears before that
token so positional/pass-through args after "--" are ignored by the
front-controller help detection.
In `@test/cli-oclif-compatibility.test.ts`:
- Around line 111-121: The spawned CLI test is non-hermetic because the child
inherits NEMOCLAW_DISABLE_AUTO_DISPATCH from the parent and can skip the path
under test; update the spawnSync invocation that assigns result (the call to
spawnSync with process.execPath and ["bin/nemohermes.js", "sandbox", "channels",
"start", "--help"]) to explicitly set NEMOCLAW_DISABLE_AUTO_DISPATCH in the env
(e.g. "0" or remove it) and add a timeout (e.g. timeout: 10000) to the spawnSync
options so the test cannot hang indefinitely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d524d732-04b6-4dbd-acff-3d765ef16c8f
📒 Files selected for processing (8)
src/lib/cli/oclif-runner.test.tssrc/lib/cli/oclif-runner.tssrc/lib/commands/root/help.tssrc/lib/commands/root/version.tssrc/lib/commands/sandbox/skill.tssrc/lib/commands/sandbox/snapshot.tssrc/nemoclaw.tstest/cli-oclif-compatibility.test.ts
Selective E2E Results — ❌ Some jobs failedRun: 25927765067
|
Summary
Tightens the oclif compatibility layer so legacy sandbox help stays side-effect free and native oclif help respects alias branding. This also documents the front-controller boundary, starts migrating parent commands to the shared NemoClaw oclif base, and adds regression coverage for the routing behavior.
Changes
src/nemoclaw.tsas the compatibility front controller between public legacy sandbox syntax and oclif-native command IDs.execute()sonemohermes sandbox ... --helpuses the alias binary name.NemoClawCommand.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
New Features
Refactor
Tests