Repository navigation
fix(resource-shipping): close LOW follow-ups - #2543
Conversation
…on, replay-safe sweep Three ship-time LOW follow-ups from wish plugin-resource-shipping (PR #2540): 1. fresh-install-smoke.ts: fail() called process.exit(1), which skips finally blocks, so a phase-b failure orphaned its mkdtemp work dir despite the try/finally in runWishScaffoldSmoke. fail() now throws a SmokeFailure that main() catches and translates to the same exit-1 + stderr contract, so the existing finally runs on every exit path. Colocated test induces a phase-b failure and asserts no genie-fresh-install-* temp dir survives. 2. skills-lint.ts unguarded-repo-lint rule: the same-line guard was line.includes('package.json'), so any incidental mention (trailing comment, echo arg, reference after the command) exempted a repo-only lint invocation. Tightened to require a package.json probe that short-circuits (&&) into the command. Added fixtures for the false-exemption cases and broader probe shapes. 3. plugin-resource-shipping WISH.md G1 validation sweep (inline, not a validate/ dir): the recursive grep tripped on the lint rule's own negative fixtures in skills-lint.test.ts on replay. Switched to git grep -In with :(exclude) pathspecs (.genie history + the fixture file); still catches real stale refs.
|
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb3b840db7
ℹ️ 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 lintMatch = /\bbun run (?:wishes|skills):lint\b/.exec(line); | ||
| if (lintMatch) { | ||
| const guard = line.slice(0, lintMatch.index); | ||
| const guarded = /\bpackage\.json\b[^&|;]*&&/.test(guard); |
There was a problem hiding this comment.
When a skill command line mentions package.json before the lint invocation without testing it, for example echo package.json && bun run skills:lint, this regex still sets guarded to true because it only requires the token before an &&. That lets an incidental pre-command mention evade the unguarded-repo-lint rule, so shipped skill docs can still contain repo-only lint commands that run outside the genie repo; please restrict this to recognized probe forms such as grep ... package.json, test -f package.json, or [ -f package.json ].
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves cleanup reliability in the smoke tests by throwing a custom error instead of calling process.exit(1) directly, ensuring temporary directories are deleted even on failures. It also refactors the skills-lint script to more accurately detect when a lint command is properly guarded by a package.json check on the same line. The review feedback points out a potential bypass in the guard detection regex where a broken short-circuit chain (e.g., with an intervening semicolon) could falsely exempt a command, and suggests a more robust regex along with an additional test case to cover this scenario.
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.
| const lintMatch = /\bbun run (?:wishes|skills):lint\b/.exec(line); | ||
| if (lintMatch) { | ||
| const guard = line.slice(0, lintMatch.index); | ||
| const guarded = /\bpackage\.json\b[^&|;]*&&/.test(guard); |
There was a problem hiding this comment.
The current regex /\bpackage\.json\b[^&|;]*&&/ only checks if package.json is followed by && with no operators in between. However, it does not verify that this && short-circuit chain actually extends all the way to the command itself.
For example, if a line contains:
test -f package.json && echo "hello"; bun run wishes:lint
The guard segment extracted is test -f package.json && echo "hello"; . The regex matches package.json && at the beginning, but the trailing semicolon ; breaks the short-circuit chain, executing bun run wishes:lint unconditionally.
To prevent such false exemptions, we should ensure that the && short-circuit chain continues to the end of the guard segment (just before the command) without any intervening command separators like ;, |, or single &.
| const guarded = /\bpackage\.json\b[^&|;]*&&/.test(guard); | |
| const guarded = /\bpackage\.json\b(?:[^&|;]|&&)*&&\s*$/.test(guard); |
| // Mention in a `;`-joined prose segment is not a short-circuit guard. | ||
| expect(checkResourceLine('echo "see package.json"; bun run skills:lint').map((v) => v.rule)).toEqual([ | ||
| 'unguarded-repo-lint', | ||
| ]); |
There was a problem hiding this comment.
To ensure that broken short-circuit chains (e.g., due to trailing semicolons or other command separators) are correctly flagged as unguarded, we should add a test case covering this scenario.
| // Mention in a `;`-joined prose segment is not a short-circuit guard. | |
| expect(checkResourceLine('echo "see package.json"; bun run skills:lint').map((v) => v.rule)).toEqual([ | |
| 'unguarded-repo-lint', | |
| ]); | |
| // Mention in a `;`-joined prose segment is not a short-circuit guard. | |
| expect(checkResourceLine('echo "see package.json"; bun run skills:lint').map((v) => v.rule)).toEqual([ | |
| 'unguarded-repo-lint', | |
| ]); | |
| // Broken short-circuit chain due to a trailing semicolon. | |
| expect(checkResourceLine('test -f package.json && echo "hello"; bun run wishes:lint').map((v) => v.rule)).toEqual([ | |
| 'unguarded-repo-lint', | |
| ]); |
1 participant
Closes the three LOW follow-ups recorded at ship time of wish plugin-resource-shipping (merged to dev via PR #2540, commits
ecbb67fc/203c97df/bbd6439e). Each was verified against the actual code before fixing; all three were real.1.
scripts/fresh-install-smoke.ts— temp-dir cleanup bypassed on phase-b failurefail()calledprocess.exit(1), which does not runfinallyblocks, so thetry/finallytmp-dir removal inrunWishScaffoldSmokewas skipped on any phase-b failure — orphaning agenie-fresh-install-*dir despite the header comment claiming cleanup on failure.Fix:
fail()now throws aSmokeFailure;main()catches it and translates to the same exit-1 +fresh-install-smoke: FAIL — …stderr contract CI depends on. The existingfinallynow runs on every exit path. Any non-SmokeFailureerror still propagates untouched. Guardedmain()behindimport.meta.mainso the module is importable/testable.Verified: ran the pre-fix script vs the fixed script against an induced phase-b failure in an isolated
TMPDIR— original left 1 orphaned temp dir, fixed left 0, both exit 1 with identical stderr. Colocated test (phase-b failure cleanup) asserts non-zero exit + no surviving scaffold temp dir.2.
scripts/skills-lint.ts— substring-based same-line guardThe
unguarded-repo-lintrule exempted a repo-onlybun run wishes:lint/skills:lintwhenever the literalpackage.jsonappeared anywhere on the line (!line.includes('package.json')) — a trailing comment, an echo arg, or a mention after the command all falsely exempted it.Fix: the exemption now requires a
package.jsonprobe that short-circuits (&&) into the command (/\bpackage\.json\b[^&|;]*&&/over the segment before the invocation). Existing positive/negative fixtures still pass; added fixtures for the false-exemption cases (trailing comment, ref-after-command,;-joined prose) and for broader probe shapes (test -f package.json &&,[ -f package.json ] &&).3.
plugin-resource-shippingWISH.md G1 validation sweep — not replay-safe post-G2Note: there is no
validate/dir for this wish; the G1 sweep lives inline inWISH.md(Group 1 → Validation). The recursivegrep -rn 'templates/wish-template.md' .tripped on the lint rule's own negative fixtures inscripts/skills-lint.test.ts(barecp templates/wish-template.mdstrings), which are intentional test data.Fix: switched that sweep line to
git grep -In(tracked-only; auto-skips node_modules/.git and the.docs-vendorsubmodule) with:(exclude)pathspecs for.genie/history docs and the fixture file. Still catches real regressions.Verified: on the current tree the sweep passes clean (exit 0); injecting a stale
templates/wish-template.mdref into a tracked decoy file fires it (exit 1); the full G1 validation block runs green end-to-end.Gates (all green)
Run from
scratchpad/gates.sh(script file — inline multi-line bash false-PASSES in this env):bun run check— exit 0 (typecheck + lint + dead-code + skills:lint + wishes:lint + council-workflow lint + full suite, 810 pass / 1 skip / 0 fail)bun run skills:lint— exit 0 (31 files, 0 violations)bun run lint:complexity-budget— exit 0bun test scripts/fresh-install-smoke.test.ts scripts/skills-lint.test.ts— 21 pass / 0 failNo items skipped — all three follow-ups were real and are fixed.