Skip to content

refactor(cli): remove compatibility command index - #3103

Merged
cv merged 105 commits into
mainfrom
refactor/remove-compat-command-index
May 6, 2026
Merged

refactor(cli): remove compatibility command index#3103
cv merged 105 commits into
mainfrom
refactor/remove-compat-command-index

Conversation

@cv

@cv cv commented May 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Removes the legacy explicit src/lib/commands/index.ts command map now that oclif pattern discovery and metadata-derived command display are in place. Command metadata lookup now relies only on discovered src/commands/** entrypoints.

Changes

  • Delete the compatibility command index under src/lib/commands/index.ts.
  • Remove the fallback metadata loader for the compatibility index.
  • Update discovery and metadata tests to assert pattern discovery is the command source of truth.
  • Simplify the layer boundary exception that previously allowed the command index.

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

    • Simplified internal command discovery by consolidating to a single pattern-based mechanism and removing the legacy compatibility fallback.
  • Tests

    • Added and updated tests to validate the new pattern-based command discovery and to ensure the legacy compatibility index is no longer relied upon.

cv added 30 commits May 2, 2026 13:36
@cv cv self-assigned this May 6, 2026
@copy-pr-bot

copy-pr-bot Bot commented May 6, 2026

Copy link
Copy Markdown

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

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 43753ee9-197b-4713-a121-340ad04e99cf

📥 Commits

Reviewing files that changed from the base of the PR and between 70c78ec and 34663d0.

📒 Files selected for processing (2)
  • scripts/check-layer-import-boundaries.ts
  • src/lib/cli/oclif-metadata.ts

📝 Walkthrough

Walkthrough

Removed the compatibility fallback path for loading Oclif commands; command loading now calls pattern-discovery only. Import-boundary detection was tightened to ignore the prior compatibility index reference. Tests were adjusted/added to assert pattern discovery and absence of reliance on the compatibility index.

Changes

Command discovery -> pattern-only

Layer / File(s) Summary
Core Implementation
src/lib/cli/oclif-metadata.ts
loadOclifCommands() now returns loadPatternDiscoveredCommands() exclusively; the previous compatibility-index fallback (loadCompatibilityCommandIndex) was removed.
Boundary Check Update
scripts/check-layer-import-boundaries.ts
isGeneratedOrRegistryCommandFile detection changed to only match paths ending with /common.ts (removed special-casing of src/lib/commands/index.ts).
Tests / Assertions
src/lib/cli/oclif-metadata.test.ts, src/lib/cli/oclif-pattern-discovery.test.ts
Added test "looks up internal commands from pattern discovery"; updated test descriptions; new tests assert discovery of every command ID from src/commands and that src/lib/commands/index.ts is not relied upon.
Compatibility Index File
src/lib/commands/index.ts
No functional changes in this PR (file content unchanged / no runtime reliance in loader).

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant OclifMetadata as OclifMetadataLoader
  participant PatternDiscovery as PatternDiscovery
  participant FileSystem as FileSystem

  Caller->>OclifMetadata: request commands
  OclifMetadata->>PatternDiscovery: loadPatternDiscoveredCommands()
  PatternDiscovery->>FileSystem: scan src/commands pattern
  FileSystem-->>PatternDiscovery: discovered command modules
  PatternDiscovery-->>OclifMetadata: command list
  OclifMetadata-->>Caller: return commands
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through code with a twitchy nose,
Found one clear path where the pattern grows.
Out went the fallback, tidy and bright,
Commands now found by their own keen sight. ✨

🚥 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 accurately describes the main change: removing a legacy compatibility command index from the CLI module.
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/remove-compat-command-index

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

@cv cv added NemoClaw CLI refactor PR restructures code without intended behavior change labels May 6, 2026
@cv

cv commented May 6, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cv

cv commented May 6, 2026

Copy link
Copy Markdown
Collaborator Author

Automated PR review summary

Reviewed PR #3103: refactor(cli): remove compatibility command index

Recommendation

  • Recommendation: PASS
  • Highest observed severity: low
  • Block merge: no
  • Why: The main merge risk was that deleting src/lib/commands/index.ts might leave a hidden dependency in built/package runtime help or command metadata. The installed environment showed the opposite: dist/lib/cli/oclif-metadata.js now loads only pattern-discovered commands, there was no dist/lib/commands/index.js fallback artifact, representative command help still rendered correctly, and the live sandbox flow was unaffected.
  • Reviewer summary: Reviewed PR refactor(cli): remove compatibility command index #3103 with PR-specific adversarial probes against the installed NemoClaw CLI and a live OpenShell sandbox. I focused on the claimed removal of the compatibility command index and looked for hidden runtime/package dependencies on that legacy path. Representative help/metadata resolution still worked, the built metadata loader uses pattern discovery only, no built compatibility index file was present, and a real sandboxed agent invocation remained healthy.

Installation and setup findings

  • Installed from the local checkout using the real installer flow and verified the NemoClaw-created sandbox end to end. The main installer command timed out late in onboarding, but I confirmed the created sandbox was ready, SSH worked, and OpenClaw inside the sandbox reached the configured provider and answered correctly.

What was validated

  • The PR revision was checked out in an isolated review environment.
  • The local checkout was installed using the repository installer flow as closely as the environment allowed.
  • Adversarial, PR-specific probes were then run against the installed environment and relevant repository context.
  • Diff summary:
 .../skills/nemoclaw-contributor-create-pr/SKILL.md |   20 +-
 .../nemoclaw-maintainer-pr-comparator/SKILL.md     |  121 --
 .../checks/tier-0-gates.md                         |   61 -
 .../checks/tier-1-correctness.md                   |   86 --
 .../checks/tier-2-quality.md                       |   64 -
 .../repo-policy.md                                 |   86 --
 .../scripts/check-coderabbit-threads.sh            |  105 --
 .../scripts/collect-gates.sh                       |   84 --
 .../scripts/find-candidates.sh                     |   86 --
 .../scripts/parse-supersession.sh                  |   68 -
 .../scripts/render-verdict.py                      |  232 ----
 .../templates/verdict.md                           |   88 --
 .../tiebreakers.md                                 |   61 -
 .../validation/backtest.md                         |   69 --
 .agents/skills/nemoclaw-skills-guide/SKILL.md      |    2 +-
 .../references/agent-skills.md                     |    4 +-
 .../nemoclaw-user-configure-inference/SKILL.md     |  329 ++---
 .../references/inference-options.md                |    4 +-
 .../references/set-up-sub-agent.md                 |  120 --
 .../references/switc
...[truncated]

Failing tests and unresolved impact

  • No failing adversarial tests were captured.

Passing tests and why they mattered

Passing test 1: Runtime command help survives without compatibility index

  • What was tested: The installed CLI still discovers commands and command summaries from oclif pattern discovery alone after removing the legacy command index.
  • Why it mattered: If false, users would lose command visibility or misleading help text after this refactor despite CI passing.
  • Observed result: nemoclaw help still listed nemoclaw <name> logs with the expected summary, and direct help for nemoclaw my-assistant logs --help rendered normally. Supporting inspection showed dist/lib/cli/oclif-metadata.js now calls only loadPatternDiscoveredCommands() and no built dist/lib/commands/index.js was present.
  • Command: bash /tmp/pr3103-test1.sh
  • Recommended follow-up coverage: Add or keep an integration/regression test that asserts representative public and internal commands remain visible in built help output when the compatibility index file is absent.

Passing test 2: Real sandbox remains operational through installed command path

  • What was tested: Removing the compatibility command index did not break the installed runtime path that users exercise against a live sandbox.
  • Why it mattered: If false, the refactor could silently break real workflows even if unit tests around discovery passed.
  • Observed result: nemoclaw my-assistant status reported healthy inference, and the sandboxed openclaw agent invocation completed with JSON status ok and payload text PR3103_OK. This was end-to-end evidence from the real OpenShell sandbox.
  • Command: ssh -F /tmp/sshcfg ... openshell-my-assistant 'openclaw agent --agent main --session-id pr3103 --message "Reply with exactly: PR3103_OK" --json'
  • Recommended follow-up coverage: No unit test needed for the remote provider response itself; if desired, add a lightweight install/integration smoke test that exercises one representative installed CLI path against a sandbox fixture.

Passing test 3: Adversarial check for hidden fallback dependency in built install

  • What was tested: The installed package no longer relies on any generated or built compatibility command index file for help/metadata resolution.
  • Why it mattered: If false, the PR claim would be misleading and packaged installs could regress even if source-tree tests passed.
  • Observed result: The expected legacy built file /workspace/nemoclaw/dist/lib/commands/index.js did not exist (INDEX_MOVED=0), yet nemoclaw my-assistant logs --help and root help still worked. That indicates no hidden runtime fallback dependency remained in the installed build.
  • Command: bash /tmp/pr3103-test4.sh
  • Recommended follow-up coverage: Add a regression test at the packaging/integration layer that asserts the dist tree lacks dist/lib/commands/index.js while representative help/metadata still resolves.

Bottom line

  • Based on the install evidence and adversarial probes, this PR looks reasonable to approve.

@cjagwani cjagwani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference audit clean — no stale imports of src/lib/commands/index left in the codebase at this stack base. layer-boundaries exemption removed correctly (the file no longer exists).

@cv
cv marked this pull request as ready for review May 6, 2026 20:42
@cv
cv changed the base branch from refactor/legacy-dispatch-rewrite to main May 6, 2026 20:42
@cv
cv enabled auto-merge (squash) May 6, 2026 20:42
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv
cv requested a review from prekshivyas May 6, 2026 20:43

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean cleanup PR — drops the temporary compatibility fallback now that the pattern-discovery and metadata-derived display stack is in place. src/lib/commands/index.ts (108-line hardcoded commandId → CommandClass map) deleted entirely. loadCompatibilityCommandIndex function and the ?? loadCompatibilityCommandIndex() fallback chain in oclif-metadata.ts removed; loadOclifCommands() now simply returns loadPatternDiscoveredCommands() directly. scripts/check-layer-import-boundaries.ts drops the src/lib/commands/index.ts exception since the file no longer exists.

Right shape of regression net: oclif-pattern-discovery.test.ts adds expect(fs.existsSync(...src/lib/commands/index.ts)).toBe(false) — if anyone re-creates the file, the test fails. oclif-metadata.test.ts existing case renamed for accuracy ("not in the compatibility command index" → "from pattern discovery"); same assertion validating that internal commands resolve via pattern discovery.

Observation (non-blocking): With the fallback gone, if loadPatternDiscoveredCommands() returns null (e.g., package.json#oclif.commands.target missing or dist/commands absent), metadata lookups return null with no safety net. In production this can't happen — the bundled package always has dist/ built. In dev/test, vitest depends on npm run build:cli running first (already true). Worth knowing the contract is now stricter.

CI: pr.yaml lightweight checks pass (commit-lint, dco, layer-boundary, check-hash, changes). checks / macos-e2e / wsl-e2e / build-sandbox-images / current self-hosted run still in progress at approval time.

@cv
cv merged commit 600a7e1 into main May 6, 2026
16 checks passed
alessandro-festa added a commit to alessandro-festa/NemoClaw that referenced this pull request May 7, 2026
Brings the 40 upstream commits accumulated on main into the US-507
remote-mode-on-oclif feature branch.

Conflict resolutions (upstream-drives default for NemoClaw):

- src/lib/commands/onboard.ts: adopt upstream's new ../actions/global
  path; keep our ../remote-onboard import alongside.
- src/lib/commands/sandbox/connect.ts: same shape — upstream's
  ../../actions/sandbox/runtime + our ../../remote-connect.
- src/lib/commands/index.ts: deleted (modify/delete). Upstream
  removed the compatibility command index in NVIDIA#3103 because oclif
  pattern discovery now drives command registration. Zero importers
  on this branch; nothing to migrate.
- src/lib/cli/oclif-dispatch.ts: dropped the dead-code FLAT_SANDBOX_ROUTES
  + NESTED_SANDBOX_ROUTES blocks. They were a stale upstream snapshot
  (introduced in NVIDIA#2923) that upstream itself has refactored away —
  replaced by the sandboxCommands()/legacyRoutes() data flow. The
  blocks were unreferenced and the FlatSandboxRoute / NestedSandboxRoute
  types are undefined in the codebase.
- src/nemoclaw.ts: rename one call site from resolveSandboxOclifDispatch
  to resolveLegacySandboxDispatch (upstream's new name; the old name
  is still exported as a compatibility alias).

Production source typechecks clean. Remaining typecheck errors are all
in upstream test/* files (dist/lib/* references that need npm run build,
plus messagingChannelConfig vs messagingChannels drift from upstream
fix NVIDIA#3133) — pre-existing on upstream/main, not regressions.

Committed with --no-verify because the local prek environment fails
on a Python/setuptools SSL cert chain issue unrelated to this merge.
@cv
cv deleted the refactor/remove-compat-command-index branch May 27, 2026 21:18
@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output and removed NemoClaw CLI labels Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants