From eb3b840db7b3a24daa3e3623b3481b003a75b9cb Mon Sep 17 00:00:00 2001 From: namastex888 Date: Fri, 10 Jul 2026 04:02:44 -0300 Subject: [PATCH] =?UTF-8?q?fix(resource-shipping):=20close=20LOW=20follow-?= =?UTF-8?q?ups=20=E2=80=94=20cleanup,=20guard=20precision,=20replay-safe?= =?UTF-8?q?=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../wishes/plugin-resource-shipping/WISH.md | 6 +- scripts/fresh-install-smoke.test.ts | 71 ++++++++++++++++++- scripts/fresh-install-smoke.ts | 31 +++++--- scripts/skills-lint.test.ts | 20 ++++++ scripts/skills-lint.ts | 16 +++-- 5 files changed, 129 insertions(+), 15 deletions(-) diff --git a/.genie/wishes/plugin-resource-shipping/WISH.md b/.genie/wishes/plugin-resource-shipping/WISH.md index 24ebdbe87..938dfa065 100644 --- a/.genie/wishes/plugin-resource-shipping/WISH.md +++ b/.genie/wishes/plugin-resource-shipping/WISH.md @@ -92,7 +92,11 @@ test ! -e templates/wish-template.md || { echo "repo-root template still present grep -q 'CLAUDE_SKILL_DIR}/templates/wish-template.md' skills/wish/SKILL.md || { echo "scaffold not CLAUDE_SKILL_DIR-addressed"; exit 1; } grep -nE '^\s*(bash )?bun run wishes:lint' skills/wish/SKILL.md skills/brainstorm/SKILL.md skills/README.md | grep -v 'grep -q' && { echo "unguarded executable lint invocation remains"; exit 1; } grep -q 'cp templates/wish-template.md' skills/wish/SKILL.md && { echo "old bare cp form survives in wish skill"; exit 1; } -grep -rn 'templates/wish-template.md' . 2>/dev/null | grep -v 'skills/wish/templates/wish-template.md' | grep -v 'CLAUDE_SKILL_DIR}/templates/wish-template.md' | grep -v '^\./\.genie/' | grep -v '^\./node_modules/' | grep -v '^\./\.docs-vendor/' | grep -v '^\./\.git/' && { echo "stale old-path reference (any extension)"; exit 1; } +# git grep is tracked-only (auto-skips node_modules/.git and the .docs-vendor submodule); +# :(exclude) drops .genie history docs and the lint rule's own negative fixtures +# (skills-lint.test.ts intentionally ships bare `cp templates/...` strings) so the +# sweep stays replay-safe post-G2 while still catching a real stale old-path reference. +git grep -In 'templates/wish-template\.md' -- ':(exclude).genie/' ':(exclude)scripts/skills-lint.test.ts' | grep -v 'skills/wish/templates/wish-template.md' | grep -v 'CLAUDE_SKILL_DIR}/templates/wish-template.md' && { echo "stale old-path reference (any extension)"; exit 1; } grep -rn 'REPO_ROOT/templates' tests/ && { echo "e2e still reads repo-root template"; exit 1; } bun run wishes:lint || exit 1 bun run check || exit 1 diff --git a/scripts/fresh-install-smoke.test.ts b/scripts/fresh-install-smoke.test.ts index 57b4ce383..69dfc19c4 100644 --- a/scripts/fresh-install-smoke.test.ts +++ b/scripts/fresh-install-smoke.test.ts @@ -1,10 +1,18 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; const SMOKE_SCRIPT = join(import.meta.dir, 'fresh-install-smoke.ts'); +// Internal work dirs the script mkdtemps in runWishScaffoldSmoke. A survivor +// with this prefix (and NOT this test's own '-fixture-' dirs) means the phase-b +// cleanup was skipped. +const WORKDIR_PREFIX = 'genie-fresh-install-'; +function scaffoldWorkDirs(): Set { + return new Set(readdirSync(tmpdir()).filter((n) => n.startsWith(WORKDIR_PREFIX) && !n.includes('-fixture-'))); +} + function runSmoke(args: string[] = []): { code: number; stdout: string; stderr: string } { const result = Bun.spawnSync(['bun', SMOKE_SCRIPT, ...args], { stdout: 'pipe', @@ -48,4 +56,65 @@ describe('fresh-install-smoke', () => { expect(result.stderr).toContain('does not resolve to a real file'); }); }); + + // Phase-b failures create a scaffold work dir BEFORE the assertion trips, so + // they are the path where the old process.exit() bypassed cleanup. Induce one + // and prove the temp dir is gone regardless. + describe('phase-b failure cleanup', () => { + let skillsDir: string; + + // Wish skill whose SKILL.md references its in-skill template (phase-a + // passes) but whose template omits `## Execution Groups`, so the phase-b + // structural check fails after the work dir already exists. + function writeWishFixture(templateBody: string): void { + const wishDir = join(skillsDir, 'wish'); + mkdirSync(join(wishDir, 'templates'), { recursive: true }); + writeFileSync( + join(wishDir, 'SKILL.md'), + ['# wish', '', '```bash', 'cp "${CLAUDE_SKILL_DIR}/templates/wish-template.md" out.md', '```', ''].join('\n'), + ); + writeFileSync(join(wishDir, 'templates', 'wish-template.md'), templateBody); + } + + const FULL_SECTIONS = [ + '## Summary', + '## Scope', + '### IN', + '### OUT', + '## Success Criteria', + '## Execution Strategy', + ]; + + beforeEach(() => { + skillsDir = mkdtempSync(join(tmpdir(), 'phaseb-fixture-')); + }); + afterEach(() => { + rmSync(skillsDir, { recursive: true, force: true }); + }); + + test('a phase-b failure exits non-zero and leaves no scaffold temp dir behind', () => { + writeWishFixture(`${FULL_SECTIONS.join('\n')}\n`); // no '## Execution Groups' + const before = scaffoldWorkDirs(); + + const result = runSmoke(['--skills-dir', skillsDir]); + + expect(result.code).not.toBe(0); + expect(result.stderr).toContain('fresh-install-smoke: FAIL'); + expect(result.stderr).toContain('## Execution Groups'); + + const leaked = [...scaffoldWorkDirs()].filter((n) => !before.has(n)); + expect(leaked).toEqual([]); + }); + + test('a clean phase-b run exits 0 and leaves no scaffold temp dir behind', () => { + writeWishFixture(`${[...FULL_SECTIONS, '## Execution Groups'].join('\n')}\n`); + const before = scaffoldWorkDirs(); + + const result = runSmoke(['--skills-dir', skillsDir]); + + expect(result.code).toBe(0); + const leaked = [...scaffoldWorkDirs()].filter((n) => !before.has(n)); + expect(leaked).toEqual([]); + }); + }); }); diff --git a/scripts/fresh-install-smoke.ts b/scripts/fresh-install-smoke.ts index 7d268e695..2518c0e99 100644 --- a/scripts/fresh-install-smoke.ts +++ b/scripts/fresh-install-smoke.ts @@ -26,9 +26,14 @@ import { dirname, join, resolve, sep } from 'node:path'; const REPO_ROOT = new URL('..', import.meta.url).pathname.replace(/\/$/, ''); +// A checked smoke violation. Thrown (not process.exit'd) so any `finally` +// cleanup on the call stack — notably the tmp-dir removal in +// runWishScaffoldSmoke — runs before we translate it to the exit-1 contract in +// main(). process.exit() would skip those finalizers and orphan the temp dir. +class SmokeFailure extends Error {} + function fail(message: string): never { - console.error(`fresh-install-smoke: FAIL — ${message}`); - process.exit(1); + throw new SmokeFailure(message); } function parseArgs(argv: string[]): { skillsDir: string } { @@ -143,12 +148,20 @@ function runWishScaffoldSmoke(skillsDir: string): void { } 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})`); + 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) { + // Checked violations become the exit-1 contract CI depends on; any other + // error propagates untouched (non-zero exit + stack trace). + if (!(err instanceof SmokeFailure)) throw err; + console.error(`fresh-install-smoke: FAIL — ${err.message}`); + process.exit(1); + } } -main(); +if (import.meta.main) main(); diff --git a/scripts/skills-lint.test.ts b/scripts/skills-lint.test.ts index 92e385e6e..8d91e4210 100644 --- a/scripts/skills-lint.test.ts +++ b/scripts/skills-lint.test.ts @@ -34,6 +34,26 @@ describe('checkResourceLine — imperative discriminators', () => { expect(checkResourceLine(guarded)).toEqual([]); }); + test('passes other short-circuit package.json probe shapes', () => { + expect(checkResourceLine('test -f package.json && bun run skills:lint')).toEqual([]); + expect(checkResourceLine('[ -f package.json ] && bun run wishes:lint')).toEqual([]); + }); + + test('flags a line that only mentions package.json incidentally', () => { + // Trailing comment — the probe does not gate the command. + expect(checkResourceLine('bun run skills:lint # regenerates package.json entries').map((v) => v.rule)).toEqual([ + 'unguarded-repo-lint', + ]); + // package.json referenced AFTER the command — no short-circuit guard. + expect(checkResourceLine('bun run wishes:lint && cat package.json').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', + ]); + }); + test('flags an imperative repo-script invocation but not a descriptive mention', () => { expect(checkResourceLine('bun run scripts/skills-lint.ts').map((v) => v.rule)).toEqual(['repo-script-invocation']); expect(checkResourceLine('node scripts/foo.ts').map((v) => v.rule)).toEqual(['repo-script-invocation']); diff --git a/scripts/skills-lint.ts b/scripts/skills-lint.ts index 1b201f5eb..c4dda3726 100644 --- a/scripts/skills-lint.ts +++ b/scripts/skills-lint.ts @@ -170,10 +170,18 @@ export function checkResourceLine(line: string): ResourceViolation[] { } // (b) Repo-only lint invocation without the SAME-LINE package.json guard. - // A split-line guard (probe on the previous line) does not count — the probe - // must sit on the same line as the command it protects. - if (/\bbun run (?:wishes|skills):lint\b/.test(line) && !line.includes('package.json')) { - violations.push({ rule: 'unguarded-repo-lint', snippet }); + // The guard must be a package.json probe that short-circuits (`&&`) INTO the + // command, e.g. `grep -q '"wishes:lint"' package.json 2>/dev/null && bun run + // wishes:lint` or `test -f package.json && bun run skills:lint`. A bare + // mention of package.json elsewhere on the line — a trailing comment, an echo + // arg, or a reference AFTER the command — does not gate the run, so it must + // NOT exempt it. A split-line guard (probe on the previous line) also fails: + // the probe must sit on the same line, ahead of the command it protects. + 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); + if (!guarded) violations.push({ rule: 'unguarded-repo-lint', snippet }); } // (c) Imperative execution of a repo-root script — scripts/*.ts is repo-only.