fix(skills): report the engines with no skills primitive instead of dropping them - #166
Merged
ralyodio merged 1 commit intoAug 1, 2026
Conversation
…ropping them
prd/0003 R8 requires `/skill install` to report engines without a skills
primitive as skipped, and the PRD's own UX example prints a
`skipped — no skills primitive` line for them. planSkillInstall maps
SKILL_ENGINES, so codex, opencode, privacycode and aider never enter the
plan, never reach runSkillInstall, and never reach the summary. The
`{ skip: "no skills primitive" }` branch in skillInstallAction is
unreachable through the command.
Plan over every engine, ordered SKILL_ENGINES first then the remainder,
derived from ENGINES exactly as the /skill list matrix already derives it.
runSkillInstall checks skip before installed, so a skipped engine reports
its reason whether or not it is installed.
Merged
ralyodio
added a commit
that referenced
this pull request
Aug 1, 2026
install.sh resolves releases/latest, so the sixteen commits merged since v0.13.3 have been sitting on main unreachable — including a fix for a page that locks browsers up. The headline is the pit. /pit rendered every ending an account held and a form per name under each, with no bound on either: at 50 endings x 100 names that was 3.1 MiB of HTML and 36,082 DOM elements, and it managed to jam a browser with no script on the page at all (#167). It now draws a window and says what it is not drawing — 173 KiB, 1,926 elements — with a filter box over the top that takes `eggs` as a substring and `def*` as a glob, debounced against the API (#168). The namespace also stopped being the one part of the product a script could not touch: /api/moshpit/* now accepts the same API key /api/me and /api/sessions already did (#169), and /pit/dns finally documents the TronBrowser route for machines whose DNS is not theirs to change (#165). moshcode: foreign keys are enforced, and the licence package.json claims actually ships (#154) cli: help aliases exit 0 (#157), invalid integration commands fail (#160), `--` is honoured (#159), a BOM before a shebang no longer breaks (#158) skills: engines with no skills primitive are reported, not dropped (#166); `--name` requires a value (#156) mcp: an unsupported flag is rejected rather than registered as the server name (#164) pit: the namespace rules are vendored again with a drift test holding them to the published package (#161, #162, #163) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
prd/0003R8 says engines without a skills primitive MUST be reported as skipped, and the PRD's own UX Notes print exactly that:planSkillInstallmapsSKILL_ENGINES, socodex,opencode,privacycodeandaidernever enter the plan, never reachrunSkillInstall, and never reachsummarize. The user is not told they were skipped — they are told nothing at all.A consequence worth stating: the
default: return { skip: "no skills primitive" }branch inskillInstallActionis unreachable through the command. It is only ever called with keys fromSKILL_ENGINES.Reproduced through the real CLI
No hand-editing,
HOMEpointed at a scratch dir so nothing touched a real skills directory:Two rows. Four engines silently absent.
The asymmetry that makes it a bug and not a choice
/skill listalready gets this right, and its comment states the principle:Same command family, same run, same machine:
/skill list(the matrix)/skill install(the fan-out) — before/skill install(the fan-out) — afterprivacycodeis the concrete case the matrix comment warns about: an engine added afterSKILL_ENGINESwas written, present in the matrix, missing from the fan-out.The fix
10 insertions, 2 deletions, one file. Plan over every engine,
SKILL_ENGINESfirst then the remainder, derived fromENGINESthe same way the matrix derives it.runSkillInstallalready checksskipbeforeinstalled, so a skipped engine reports its reason whether or not the binary is present — matchingrunMcpAdd's existing precedent for R6.After:
Tests
New
test/skill-install-fanout.test.mjs, 16 tests.Seven are the bug: plan covers every
ENGINESkey; each non-primitive engine carries the skip reason; the fan-out reports them asskipped; skipped beatsnot-installedwhen nothing is installed; an installed non-primitive engine is skipped and never spawned (asserted by counting spawns);privacycodespecifically; fan-out and matrix name the same set.Nine are controls in the opposite direction, so the fix cannot buy green by over-reporting:
SKILL_ENGINESstill exactly["claude","gemini"]; the primitive engines still come first in order; claude'sgit cloneargv byte-identical; gemini's argv byte-identical; installed/not-installed still decides a real target; a signal still reportsfailed; a non-zero exit still reportsfailedand not a skip; no engine planned twice; every entry carries its realbin.Fail-before / pass-after: 7 fail, 9 pass unpatched → 16/16 patched.
Suite: 622 → 638 tests, 0 failing (pass 499 → 515, skipped 123 unchanged).
A test that passes by accident
test/skills.test.mjs:49— "skillInstallAction: engines without a skills primitive are skipped" — guards precisely the right function and passes unchanged, because it callsskillInstallAction("codex", …)directly, with keysplanSkillInstallnever passes it. The skip branch is proven correct and simultaneously unreachable in production. The new tests go throughplanSkillInstall/runSkillInstall, which is where the defect lives.Deliberately out of scope
planMcpAddmapsMCP_ENGINES, which omitsaider, somcp installprints five rows where the PRD example shows a sixth (· aider skipped — no MCP support). Observed in the same session and not claimed or changed here: that is R6/R7 territory with its own per-engine reasons, and it deserves its own change rather than riding along on this one.