refactor(cli): model onboard flags with oclif

[cv]

Summary

Model the onboard/setup/setup-spark flag surface in oclif while preserving the existing onboarding action validation for Dockerfile paths, agents, sessions, and third-party notice semantics. This lets oclif own unknown flags and missing flag values before the onboarding action runs.

Stack Navigation

• Position: 21 of 60
• Previous PR: #2912 — refactor(cli): model debug flags with oclif
• Next PR: #2915 — docs: sync oclif UX command reference

Changes

• Added oclif flags for onboard, setup, and setup-spark options.
• Added onboard examples for common interactive, resume, fresh, custom Dockerfile, and non-interactive invocations.
• Reconstructed legacy argv for the existing onboarding action so domain validation stays centralized.
• Kept deprecated setup/setup-spark help paths routed through the existing deprecation messaging.
• Updated CLI expectations for parser-owned unknown flag errors.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Verification

• npx prek run --all-files passes
• npm test passes
• Tests added or updated for new or changed behavior
• No secrets, API keys, or credentials committed
• Docs updated for user-facing behavior changes
• make docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

• Refactor

  • CLI commands now use stricter flag parsing and convert parsed flags into the legacy argument format, improving validation and behavior for onboarding and setup flows.

• Documentation

  • Improved usage text, examples, and more predictable handling of help requests for onboard and setup commands.

• Tests

  • CLI tests updated for stricter parsing, standardized parser exit codes, and updated error output expectations for invalid flags.

[copy-pr-bot]

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.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Three CLI commands (onboard, setup, setup-spark) switch from forwarding raw argv to strict Oclif Flags parsing. A shared typed flag schema and a converter produce legacy-style string[] arguments which are passed to existing action handlers; setup commands special-case --help/-h before parsing.

Changes

CLI Strict Flag Parsing

Layer / File(s)	Summary
Type & Flag Definitions src/lib/onboard-cli-commands.ts	Adds OnboardFlags type, buildOnboardFlags() that builds an Oclif Flags schema, onboardUsage, and onboardExamples.
Flag-to-Legacy Conversion src/lib/onboard-cli-commands.ts	Adds toLegacyOnboardArgs(flags) converting parsed flags into legacy string[] args (stringifies --control-ui-port, emits the dynamic NOTICE_ACCEPT_FLAG when set).
Command Implementations src/lib/onboard-cli-commands.ts	OnboardCliCommand, SetupCliCommand, SetupSparkCliCommand set static strict = true, expose static flags = buildOnboardFlags(), parse via this.parse(...), and call existing actions with toLegacyOnboardArgs(flags). Setup commands detect --help/-h on this.argv and short-circuit to the help action.
Tests / Assertions test/cli.test.ts	Introduces PARSER_EXIT_CODE = 2; updates onboard --help assertions to look for generic usage/branding markers; updates unknown-flag tests to expect exit code 2 and output containing Nonexistent flag: --non-interactiv; updates setup --non-interactiv to the same expectation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through flags both new and old,
Typed petals folded into strings of gold.
Help checked early, parsing kept in line,
Legacy whispers preserved just fine.
Hooray — clean paws, tidy CLI!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main change: refactoring the CLI to model onboard flags using oclif.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/oclif-ux-onboard-flags

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/cli.test.ts (1)

908-910: ⚡ Quick win

Assert parser-owned failure code explicitly in unknown-flag tests

These checks now only require “non-zero,” which can pass for downstream action failures. Since this suite is validating parser ownership, assert the parser exit code directly.

Suggested fix

-    expect(r.code).not.toBe(0);
+    expect(r.code).toBe(2);
     expect(r.out).toContain("Nonexistent flag: --non-interactiv");

Apply this to each updated unknown-flag test in this block.

Also applies to: 914-916, 920-922, 934-935

🤖 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.test.ts` around lines 908 - 910, Replace the loose
"expect(r.code).not.toBe(0)" assertions in the unknown-flag tests with a strict
assertion against the parser's specific exit code (e.g.,
expect(r.code).toBe(PARSER_EXIT_CODE)); reference the test result variables
r.code and r.out and either use an existing parser exit-code constant or define
one (PARSER_EXIT_CODE) near the tests so the assertion verifies parser-owned
failures; apply the same change to the other occurrences mentioned (the blocks
around assertions for lines 914-916, 920-922, and 934-935).

🤖 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/onboard-cli-commands.ts`:
- Around line 63-65: The current truthy checks drop empty-string flag values
when rebuilding argv; update the conditions for flags.from, flags.name, and
flags.agent so they test for undefined (or null) instead of truthiness. Replace
the three checks that read "if (flags.from) ..." / "if (flags.name) ..." / "if
(flags.agent) ..." with explicit presence checks (e.g., if (flags.from !==
undefined) args.push("--from", flags.from); and similarly for flags.name and
flags.agent) so "" is preserved and the action layer receives the flag value.

---

Nitpick comments:
In `@test/cli.test.ts`:
- Around line 908-910: Replace the loose "expect(r.code).not.toBe(0)" assertions
in the unknown-flag tests with a strict assertion against the parser's specific
exit code (e.g., expect(r.code).toBe(PARSER_EXIT_CODE)); reference the test
result variables r.code and r.out and either use an existing parser exit-code
constant or define one (PARSER_EXIT_CODE) near the tests so the assertion
verifies parser-owned failures; apply the same change to the other occurrences
mentioned (the blocks around assertions for lines 914-916, 920-922, and
934-935).

🪄 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: fbadc1d8-2d18-430f-bbbf-fcb65a2651b5

📥 Commits

Reviewing files that changed from the base of the PR and between 0f13026 and 239c600.

📒 Files selected for processing (2)

• src/lib/onboard-cli-commands.ts
• test/cli.test.ts

[cv]

Addressed CodeRabbit feedback in 0e5e3799:

• Preserve empty string values for --from, --name, and --agent when rebuilding legacy onboard argv by checking !== undefined.
• Tightened onboard/setup unknown-flag tests to assert the parser exit code directly.

Validation run locally:

• npm run build:cli
• npx vitest run test/cli.test.ts -t "onboard|setup forwards unknown"
• npm run typecheck:cli

[prekshivyas]

LGTM. Three onboard-family commands (onboard/setup/setup-spark) flip strict=true with explicit oclif Flags declarations for the full flag surface; toLegacyOnboardArgs helper rebuilds the legacy argv so the existing action layer keeps all domain validation. Setup/setup-spark preserve their deprecation help via an early --help shortcut.

Three intentional, tested public-surface deltas (all declared in PR description):

• Unknown flags now exit 2 (oclif's Nonexistent flag: --foo) instead of 1 (Unknown onboard option(s)). Tests refactored with PARSER_EXIT_CODE = 2.
• Help heading is oclif's USAGE instead of Usage:. Test updated.
• --control-ui-port now has min: 1024, max: 65535. Defensible (avoids privileged ports / out-of-range integers) but not covered by an explicit boundary test in this diff — worth a callout if any user was relying on lower ports.

Scope hygiene clean — only onboard-cli-commands.ts + test/cli.test.ts. src/nemoclaw.ts untouched.

CI: pr.yaml mostly green (lint/dco/check-hash/legacy-path-guard/changes PASS); macos-e2e/wsl-e2e/checks + pr-self-hosted builds still in flight at review time. No failures.