Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion .genie/wishes/plugin-resource-shipping/WISH.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
71 changes: 70 additions & 1 deletion scripts/fresh-install-smoke.test.ts
Original file line number Diff line number Diff line change
@@ -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<string> {
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',
Expand Down Expand Up @@ -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([]);
});
});
});
31 changes: 22 additions & 9 deletions scripts/fresh-install-smoke.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } {
Expand Down Expand Up @@ -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();
20 changes: 20 additions & 0 deletions scripts/skills-lint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
]);
Comment on lines +51 to +54

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
// 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',
]);

});

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']);
Expand Down
16 changes: 12 additions & 4 deletions scripts/skills-lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

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 Badge Require a real package.json probe before exempting lint

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 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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 &.

Suggested change
const guarded = /\bpackage\.json\b[^&|;]*&&/.test(guard);
const guarded = /\bpackage\.json\b(?:[^&|;]|&&)*&&\s*$/.test(guard);

if (!guarded) violations.push({ rule: 'unguarded-repo-lint', snippet });
}

// (c) Imperative execution of a repo-root script — scripts/*.ts is repo-only.
Expand Down