Skip to content

fix(skills): name the remove command when install refuses a duplicate - #12711

Open
redswimmer wants to merge 1 commit into
NVIDIA:mainfrom
redswimmer:fix/skill-install-remove-hint
Open

redswimmer wants to merge 1 commit into
NVIDIA:mainfrom
redswimmer:fix/skill-install-remove-hint

Conversation

@redswimmer

@redswimmer redswimmer commented Oct 6, 2026 •

Copy link
Copy Markdown

Outcome

skill install refuses to replace a skill that is already in the canonical writable root. Before, the refusal didn't say how to recover. Now it ends with the exact command to run, for example Remove it first with: nemoclaw alpha skill remove code-review.

Reason

Agents without a native add command, such as Hermes, use the canonical-root fallback. When a skill was already installed, users had to look up or guess the supported removal command.

Related issues

Fixes #12668

Changes

  • buildCanonicalSkillAddCommand takes an optional remove command and prints it in the refusal. It is passed to printf as a quoted argument, not interpolated into the format string.
  • installSandboxSkill passes <cli> <sandbox> skill remove <name>.
  • Tests: a Linux-only test runs the generated script against an existing skill and checks the exit code, the hint, and that the existing skill is unchanged. It reuses the existing staging helper. An action-level test checks that the sandbox-scoped command is passed through.

Verification

  • Live Hermes sandbox, NemoClaw built from source, OpenShell 0.0.116, OpenRouter. A second skill install on main exited 1 with the issue's message. On this branch it exited 1 and printed Remove it first with: nemoclaw skilltest skill remove code-review. Running that command and then installing again exited 0.
  • npx vitest run src/lib/skill-install.test.ts src/lib/actions/sandbox/skill-install.test.ts passed (48 tests). Both new (#12668) tests fail without the fix.
  • npm run test:titles:check, npm run source-shape:check, tsc -p tsconfig.cli.json, oxfmt and oxlint all passed.
  • The diff contains no secrets, API keys, or credentials.

Review notes

The local pi-qualification-receipt-refresh and hadolint hooks also fail on unmodified main, so they were skipped for this push. This change touches neither area.


Signed-off-by: Andrew Savala andrew@redswimmer.com

Summary by CodeRabbit

  • Bug Fixes
    • When adding a skill would overwrite an existing skill, installation now stops and leaves the existing files unchanged.
    • The error message provides a command to remove the conflicting skill when available, or guidance for managing native skills otherwise.
    • Hermes fallback instructions now include the relevant skill removal command, scoped to the sandbox and skill being installed. This helps identify the conflicting skill and the appropriate removal action.

@copy-pr-bot

copy-pr-bot Bot commented Oct 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/NemoClaw/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 8aaa365f-7ec0-4edb-bdf3-c725de337f7d
📥 Commits

Reviewing files that changed from the base of the PR and between 972dcf4 and f573e14.

📒 Files selected for processing (2)
  • src/lib/actions/sandbox/skill-install.test.ts
  • src/lib/skill-install.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/actions/sandbox/skill-install.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

The canonical skill-add command accepts an optional removal command and includes it in the refusal when a skill destination already exists. The sandbox action supplies a sandbox-scoped removal command.

Changes

Canonical skill installation

Layer / File(s) Summary
Existing-skill refusal and test coverage
src/lib/skill-install.ts, src/lib/skill-install.test.ts
The canonical skill-add command accepts an optional removal hint and includes it when refusing an existing destination. The Linux-only test checks the hint, exit status, and preservation of the existing file. Test setup now uses a shared staging helper.
Sandbox removal-command wiring
src/lib/actions/sandbox/skill-install.ts, src/lib/actions/sandbox/skill-install.test.ts
The sandbox action passes a sandbox-scoped removal command to the canonical skill-add command. The fallback test checks the generated command.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: cv

Merge Risk: ⚪ Minimal · up to f573e

The change provides sandbox-scoped removal guidance while refusing to replace an existing canonical skill. The inspected quoting and refusal path reveal no actionable merge-blocking risk.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: duplicate-skill install refusals now name the removal command.
Linked Issues check ✅ Passed Issue #12668 requires the canonical-root fallback to refuse replacement, preserve the existing skill, and show the supported removal command. The prior reviewed evidence establishes that `buildCanonic…
Out of Scope Changes check ✅ Passed The current incremental changes add comments that describe temporary test fixtures and a test shim. These comments relate to the issue’s regression tests. The previous assessment found the whole-PR ch…
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrew Savala <andrew@redswimmer.com>
@redswimmer
redswimmer force-pushed the fix/skill-install-remove-hint branch from 972dcf4 to f573e14 Compare October 6, 2026 23:25

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Ubuntu 24.04][Agent&Skills] skill install refusal for an existing skill does not tell the user to remove it first

1 participant