Skip to content

fix(skills): install all default sandbox skills (build-os ref + resilient install) #733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sweetmantech merged 2 commits into from
Jul 1, 2026
+18 −10
Merged

fix(skills): install all default sandbox skills (build-os ref + resilient install) #733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f0f757
fix(skills): install all default sandbox skills (build-os ref + resil…
sweetmantech Jul 1, 2026
e6fc562
Merge branch 'main' into fix/default-skill-install
sweetmantech Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/skills/__tests__/defaultGlobalSkillRefs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ describe("DEFAULT_GLOBAL_SKILL_REFS", () => {
const sourceNames = DEFAULT_GLOBAL_SKILL_REFS.map(r => `${r.source}::${r.skillName}`);
expect(sourceNames).toContain("recoupable/skills::recoup-platform-api-access");
expect(sourceNames).toContain("recoupable/skills::recoup-platform-email-helper");
expect(sourceNames).toContain("recoupable/skills::recoup-platform-build-workspace");
expect(sourceNames).toContain("recoupable/skills::recoup-platform-build-os");
expect(sourceNames).toContain("recoupable/skills::recoup-roster-add-artist");
expect(sourceNames).toContain("recoupable/skills::recoup-roster-list-artists");
expect(sourceNames).toContain("recoupable/skills::recoup-roster-manage-artist");
Expand Down
15 changes: 11 additions & 4 deletions lib/skills/__tests__/installGlobalSkills.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ describe("installGlobalSkills", () => {
expect(exec).toHaveBeenCalledTimes(1);
});

it("throws when any install command fails", async () => {
it("continues past a failed skill and still installs the rest (best-effort, never throws)", async () => {
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
// First skill fails; subsequent calls use the beforeEach success default.
exec.mockResolvedValueOnce({
success: false,
exitCode: 1,
Expand All @@ -61,8 +63,13 @@ describe("installGlobalSkills", () => {
truncated: false,
});

await expect(installGlobalSkills({ sandbox, globalSkillRefs: [REF_API] })).rejects.toThrow(
/package not found/,
);
await expect(
installGlobalSkills({ sandbox, globalSkillRefs: [REF_API, REF_WORKSPACE] }),
).resolves.toBeUndefined();

// 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();

@cubic-dev-ai cubic-dev-ai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>

});
});
2 changes: 1 addition & 1 deletion lib/skills/defaultGlobalSkillRefs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export const DEFAULT_GLOBAL_SKILL_REFS: readonly GlobalSkillRef[] = [
// Reliable email send (Node helper) — pairs with the api-access skill.
{ source: "recoupable/skills", skillName: "recoup-platform-email-helper" },
// Folder-tree / RECOUP.md scaffolding + roster ops (was `artist-workspace`).
{ source: "recoupable/skills", skillName: "recoup-platform-build-workspace" },
{ source: "recoupable/skills", skillName: "recoup-platform-build-os" },
{ source: "recoupable/skills", skillName: "recoup-roster-add-artist" },
{ source: "recoupable/skills", skillName: "recoup-roster-list-artists" },
{ source: "recoupable/skills", skillName: "recoup-roster-manage-artist" },
Expand Down
9 changes: 5 additions & 4 deletions lib/skills/installGlobalSkills.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ const GLOBAL_SKILLS_INSTALL_TIMEOUT_MS = 120_000;
/**
* Installs the supplied skill refs into the sandbox by running
* `npx skills add ...` for each one. Refs are validated and deduped
* via `globalSkillRefsSchema` before any command runs. Throws on the
* first failure — caller is expected to handle the error
* (typically best-effort: log and continue).
* via `globalSkillRefsSchema` before any command runs. Best-effort per
* skill: a single failed install is logged and skipped so it never blocks
* the remaining skills (a bad/renamed ref used to abort the whole batch).
*
* @param params.sandbox - The connected sandbox handle.
* @param params.globalSkillRefs - Refs to install (defaults + user prefs).
Expand All @@ -32,9 +32,10 @@ export async function installGlobalSkills(params: {
);

if (!result.success) {
throw new Error(
console.error(
`Failed to install global skill ${ref.skillName} from ${ref.source}: ${result.stderr || result.stdout || "unknown error"}`,
);
continue;
}
}
}
Loading