Repository navigation
feat(skills): plugin resource shipping — skills carry their own resources - #2540
Conversation
commented
Jul 10, 2026
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
left a comment
There was a problem hiding this comment.
Code Review
This pull request introduces a fresh-install smoke test and a skills linter to ensure that skills resolve their resources correctly using ${CLAUDE_SKILL_DIR} and do not invoke unguarded repo-only commands. It also updates various skill documents and end-to-end tests to align with these new linting rules, and documents the plugin update cadence. The review feedback highlights a resource leak in the smoke test where process.exit(1) bypasses temporary directory cleanup, and suggests a more robust regex in the linter to detect repo-root script invocations with path prefixes or alternative runners.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function fail(message: string): never { | ||
| console.error(`fresh-install-smoke: FAIL — ${message}`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Calling process.exit(1) inside fail immediately terminates the process, which bypasses the finally block in runWishScaffoldSmoke and leaves temporary directories on disk. Changing fail to throw an error allows the finally block to execute and clean up the temporary directories before the process exits.
function fail(message: string): never {
throw new Error(message);
}| function main(): void { | ||
| const { skillsDir } = parseArgs(process.argv.slice(2)); | ||
| if (!existsSync(skillsDir)) fail(`skills dir not found: ${skillsDir}`); | ||
| const refs = checkSkillDirReferences(skillsDir); | ||
| runWishScaffoldSmoke(skillsDir); | ||
| const summary = `${refs} \${CLAUDE_SKILL_DIR} reference(s) resolved, wish scaffold works with no genie on PATH`; | ||
| console.log(`fresh-install-smoke: OK (${summary})`); | ||
| } |
There was a problem hiding this comment.
Wrap the main execution in a try-catch block to catch the error thrown by fail, print the failure message, and exit with code 1. This ensures that any errors thrown during the smoke test are gracefully handled and the process exits with a non-zero code after cleaning up temporary directories.
| function main(): void { | |
| const { skillsDir } = parseArgs(process.argv.slice(2)); | |
| if (!existsSync(skillsDir)) fail(`skills dir not found: ${skillsDir}`); | |
| const refs = checkSkillDirReferences(skillsDir); | |
| runWishScaffoldSmoke(skillsDir); | |
| const summary = `${refs} \${CLAUDE_SKILL_DIR} reference(s) resolved, wish scaffold works with no genie on PATH`; | |
| console.log(`fresh-install-smoke: OK (${summary})`); | |
| } | |
| function main(): void { | |
| try { | |
| const { skillsDir } = parseArgs(process.argv.slice(2)); | |
| if (!existsSync(skillsDir)) fail(`skills dir not found: ${skillsDir}`); | |
| const refs = checkSkillDirReferences(skillsDir); | |
| runWishScaffoldSmoke(skillsDir); | |
| const summary = `${refs} \${CLAUDE_SKILL_DIR} reference(s) resolved, wish scaffold works with no genie on PATH`; | |
| console.log(`fresh-install-smoke: OK (${summary})`); | |
| } catch (err) { | |
| console.error(`fresh-install-smoke: FAIL — ${(err as Error).message}`); | |
| process.exit(1); | |
| } | |
| } |
| if (/(?:\bbun run |\bbun |\bnode |\.\/|\bsh |\bbash )scripts\/[A-Za-z0-9_./-]+\.ts\b/.test(line)) { | ||
| violations.push({ rule: 'repo-script-invocation', snippet }); | ||
| } |
There was a problem hiding this comment.
The regex for detecting repo-root script invocations does not match when there is a path prefix (like ./) between the execution command and scripts/ (e.g., bash ./scripts/foo.ts or bun ./scripts/foo.ts). Additionally, it doesn't match other common runners like tsx or ts-node. Updating the regex to allow optional ./ prefixes and more runners makes the lint rule more robust.
| if (/(?:\bbun run |\bbun |\bnode |\.\/|\bsh |\bbash )scripts\/[A-Za-z0-9_./-]+\.ts\b/.test(line)) { | |
| violations.push({ rule: 'repo-script-invocation', snippet }); | |
| } | |
| if (/(?:\bbun run |\bbun |\bnode |\.\/|\bsh |\bbash |\btsx |\bts-node )\s*(?:\.\/)?scripts\/[A-Za-z0-9_./-]+\.ts\b/.test(line)) { | |
| violations.push({ rule: 'repo-script-invocation', snippet }); | |
| } |
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a3143b437
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const stdout = execFileSync('bun', [SCRIPT], { | ||
| env: { ...process.env, SKILLS_LINT_DIR: dir }, |
There was a problem hiding this comment.
Make fixture lint tests independent of a built genie binary
In a clean checkout where dist/genie.js has not been built and no global genie is on PATH, these new fixture tests fail before exercising the resource rules because the spawned skills-lint.ts immediately probes genie --help (/bin/sh: 1: genie: not found). CI happens to run bun run build before bun test, but the documented bun test workflow now fails locally; the test should stub/provide the CLI surface or otherwise avoid requiring a prior build.
Useful? React with 👍 / 👎.
| ``` | ||
| Tasks carry the `--wish`/`--group` linkage; the dependency DAG stays in the WISH.md document, not in task rows. If creation fails (no `.genie/genie.db` yet, CLI unavailable), warn and continue — WISH.md in git is the source of truth and must remain usable by `/work` without task rows. | ||
| 9. **Handoff:** run `bun run wishes:lint`. If it reports any error, surface it and stop — never hand a structurally broken wish onward. Only after lint passes, auto-invoke `/review` (plan review) on the WISH.md. Never suggest `/work` directly — the review gate comes first. | ||
| 9. **Handoff:** run the wish linter — inside the genie repo, `grep -q '"wishes:lint"' package.json 2>/dev/null && bun run wishes:lint`. If it reports any error, surface it and stop — never hand a structurally broken wish onward. Only after lint passes, auto-invoke `/review` (plan review) on the WISH.md. Never suggest `/work` directly — the review gate comes first. |
There was a problem hiding this comment.
Make the optional wish-lint guard return success when absent
For fresh plugin consumers that are not the genie repo, this guarded command still exits non-zero when package.json is missing (grep exits 2) or lacks the script (grep exits 1), so following /wish can still be treated as a failed handoff before /review even though the repo-only linter is supposed to be optional outside the genie repo. Use a guard form that returns success when the script is unavailable but preserves bun run wishes:lint failures when it is present, such as a one-line if grep ...; then bun run ...; fi.
Useful? React with 👍 / 👎.
Summary
Fresh plugin installs break
/wishoutside the genie repo: the skill pointed at repo-roottemplates/wish-template.mdand ran the repo-only wish linter (observed live 2026-07-09). This wish makes skills self-contained and the regression class mechanically impossible.skills/wish/templates/), scaffold via${CLAUDE_SKILL_DIR}; repo-only lint behind a same-line package.json probe; e2e/README/prose consumers repointed (paraphrase rule keeps prose lint-clean)scripts/skills-lint.ts(scans fences AND inline-code spans; same-line probe discriminator; genie-hacks allowlist) + 15 fixture testsscripts/fresh-install-smoke.ts(${CLAUDE_SKILL_DIR} resolution + bare-repo scaffold with no genie on PATH) wired into CI's unit job + release-lag note in the plugin READMEExecution
Wish:
.genie/wishes/plugin-resource-shipping/WISH.md(plan review SHIP ×3 loops). Executed via orchestrated subagents under the routing matrix (engineers opus·high, reviewers opus·xhigh, final gate fable·high) — first wish run on the new routing economics. All groups 0 fix loops; per-group reviews SHIP; final gate proved all 5 success criteria, full check 773 pass / 1 skip / 0 fail. Branch rebuilt off origin/dev after a concurrent session switched the shared checkout (details in the wish's execution review).Follow-ups (LOW, noted in wish)