fix(skills): install all default sandbox skills (build-os ref + resilient install)#733
Conversation
…ient install) Two bugs left task sandboxes with only 2 of 6 default skills: - DEFAULT_GLOBAL_SKILL_REFS listed `recoup-platform-build-workspace`, which was renamed to `recoup-platform-build-os` → `npx skills add` 404s on it. - installGlobalSkills threw on the first failed skill, so that bad ref aborted the loop and every skill after it (all 3 recoup-roster-*) never installed. Fix the ref and make installGlobalSkills best-effort per skill (log + continue), so one bad ref can't block the rest. Verified on the Apache→OneRPM run (recoupable/chat#1833): loading recoup-roster-list-artists returned "Skill not found. Available: recoup-platform-api-access, recoup-platform-email-helper". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe default global skill reference was changed from ChangesGlobal Skill Install Updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 4/5
- In
lib/skills/__tests__/installGlobalSkills.test.ts,console.erroris restored only on passing paths, so a failing assertion can leak a mocked/silenced logger into later tests and mask real failures or debugging output; move restoration toafterEach(or atry/finally) so cleanup always runs before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/skills/__tests__/installGlobalSkills.test.ts">
<violation number="1" location="lib/skills/__tests__/installGlobalSkills.test.ts:73">
P2: `errorSpy.mockRestore()` only fires on test pass — if any assertion fails, `console.error` stays mocked (silenced) for all subsequent tests. `vi.clearAllMocks()` in `beforeEach` clears call data but does NOT restore the original implementation.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Installer as installGlobalSkills()
participant Sandbox as Sandbox
participant Npx as npx skills add
participant Console as console
participant Refs as GlobalSkillRefs[]
Note over Refs: Includes recoup-platform-build-os (corrected name)
Installer->>Installer: Validate & dedupe refs via globalSkillRefsSchema
loop For each globalSkillRef in refs
Installer->>Npx: npx skills add <source>/<skillName>
Npx->>Sandbox: Execute install command
Sandbox-->>Npx: Result { success, stderr, stdout }
alt success === true
Npx-->>Installer: Command completed
Installer->>Installer: Continue to next skill
else success === false
Npx-->>Installer: Failure result
Installer->>Console: console.error("Failed to install skill: ...")
Installer->>Installer: continue (skip failed skill)
Note right of Installer: NEW: never throws, logs and continues
end
end
Note over Installer: OLD: first failure threw Error, aborted entire loop
Installer-->>Installer: Function returns (undefined)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // A single bad skill must NOT block the rest — both are attempted. | ||
| expect(exec).toHaveBeenCalledTimes(2); | ||
| expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("package not found")); | ||
| errorSpy.mockRestore(); |
There was a problem hiding this comment.
P2: errorSpy.mockRestore() only fires on test pass — if any assertion fails, console.error stays mocked (silenced) for all subsequent tests. vi.clearAllMocks() in beforeEach clears call data but does NOT restore the original implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/skills/__tests__/installGlobalSkills.test.ts, line 73:
<comment>`errorSpy.mockRestore()` only fires on test pass — if any assertion fails, `console.error` stays mocked (silenced) for all subsequent tests. `vi.clearAllMocks()` in `beforeEach` clears call data but does NOT restore the original implementation.</comment>
<file context>
@@ -61,8 +63,13 @@ describe("installGlobalSkills", () => {
+ // A single bad skill must NOT block the rest — both are attempted.
+ expect(exec).toHaveBeenCalledTimes(2);
+ expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("package not found"));
+ errorSpy.mockRestore();
});
});
</file context>
Preview verification — all 6 default skills now install (was 2)Ran a sandbox skills-availability check on this PR's preview — Result: a fresh sandbox installs all 6 default skills, and the previously-broken roster skills load.
All 6 present — including Loading {"success":true,"skillName":"recoup-roster-list-artists","skillPath":".../recoup-roster-list-artists","content":"# Recoup — List Artists…"}→ vs. before #733On the original OneRPM prod run (chat
Because the renamed Net: task sandboxes now get their full toolkit — agents can resolve/verify the roster instead of guessing, directly improving grounding. |
1 participant
Part of recoupable/chat#1833 (prerequisite item). Base:
main.Problem
Task sandboxes shipped with only 2 of 6 default skills. Two bugs:
DEFAULT_GLOBAL_SKILL_REFSlistedrecoup-platform-build-workspace(renamed torecoup-platform-build-os) →npx skills add404s.installGlobalSkillsthrew on the first failure, so that bad ref aborted the loop and every skill after it (all threerecoup-roster-*) never installed.Verified on the Apache→OneRPM run (chat#1833): loading
recoup-roster-list-artistsreturned "Skill not found. Available: recoup-platform-api-access, recoup-platform-email-helper."Fix
defaultGlobalSkillRefs.ts:recoup-platform-build-workspace→recoup-platform-build-os.installGlobalSkills.ts: best-effort per skill —console.error+continueinstead ofthrow, so one bad ref can't block the rest.Tests (TDD)
installGlobalSkillscontinues past a failed skill, still attempts the rest, logs the failure, never throws.lib/skillssuite green (49),tsc+ eslint clean.🤖 Generated with Claude Code
Summary by cubic
Ensure all default sandbox skills install by fixing a renamed ref and making installs resilient. One bad ref no longer blocks the rest.
DEFAULT_GLOBAL_SKILL_REFS:recoup-platform-build-workspace→recoup-platform-build-osto stopnpx skills add404s.installGlobalSkillsbest-effort: never throws; logs and continues so later skills (e.g.,recoup-roster-*) still install.Written for commit e6fc562. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes