From 5114f8c59f294cf1c0f3257668beaaed55031dbf Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 11 Sep 2026 17:55:28 +0000 Subject: [PATCH] fix: preserve source renders on Windows Compare canonical generator paths using native separators so an output sidecar pointing at the source cannot overwrite its skill or metadata. Keep the regression fixture isolated from the real checkout and expose freshness diagnostics before asserting subprocess status. Detach Windows drain-test pipe holders from the fake provider's automatic child cleanup while preserving the enclosing runner job and its assertions. --- scripts/gen-skill-docs.ts | 2 +- test/claude-code-runner.test.ts | 5 +++- test/claude-code-windows-job.test.ts | 4 +++ test/gen-skill-docs-idempotency.test.ts | 5 +++- test/gen-skill-docs-prune-stale.test.ts | 33 ++++++++++++++++++++++--- 5 files changed, 43 insertions(+), 6 deletions(-) diff --git a/scripts/gen-skill-docs.ts b/scripts/gen-skill-docs.ts index b08428b13..99a5f542c 100644 --- a/scripts/gen-skill-docs.ts +++ b/scripts/gen-skill-docs.ts @@ -817,7 +817,7 @@ function processExternalHost( const claudePath = ctx.tmplPath.replace(/\.tmpl$/, ''); try { const resolvedClaude = fs.realpathSync(claudePath); - const resolvedExternal = fs.realpathSync(path.dirname(outputPath)) + '/' + path.basename(outputPath); + const resolvedExternal = path.join(fs.realpathSync(path.dirname(outputPath)), path.basename(outputPath)); if (resolvedClaude === resolvedExternal) { symlinkLoop = true; } diff --git a/test/claude-code-runner.test.ts b/test/claude-code-runner.test.ts index d522ec3d9..5e0c3102f 100644 --- a/test/claude-code-runner.test.ts +++ b/test/claude-code-runner.test.ts @@ -30,7 +30,10 @@ writeFileSync(process.env.CAPTURE!, JSON.stringify({args:process.argv.slice(2),p const mode = process.env.FAKE_MODE; if (mode === 'timeout' || mode === 'descendant' || mode === 'escaped') { rmSync(process.env.PID_FILE!, { force: true }); - const child = spawn(process.execPath, [process.env.DESCENDANT!], {stdio:['ignore','inherit','inherit'], detached:mode === 'escaped'}); + // libuv on Windows kills non-detached children when this fake exits. The + // drain fixture must survive that exit so its inherited pipes remain open. + const child = spawn(process.execPath, [process.env.DESCENDANT!], {stdio:['ignore','inherit','inherit'], + detached:mode === 'escaped' || (process.platform === 'win32' && mode === 'descendant')}); const readyBy = Date.now() + 2000; while (!existsSync(process.env.PID_FILE!)) { if (child.exitCode !== null || Date.now() >= readyBy) throw new Error('Descendant did not initialize its inherited pipes'); diff --git a/test/claude-code-windows-job.test.ts b/test/claude-code-windows-job.test.ts index 224aa098e..0307264da 100644 --- a/test/claude-code-windows-job.test.ts +++ b/test/claude-code-windows-job.test.ts @@ -38,6 +38,10 @@ await Bun.stdin.text(); rmSync(process.env.PID_FILE!, { force: true }); const child = spawn(process.execPath, [process.env.DESCENDANT!], { stdio: ['ignore', 'inherit', 'inherit'], + // Bypass this fake's libuv auto-kill job so the pipe holder survives it. + // DETACHED does not request CREATE_BREAKAWAY_FROM_JOB: gstack's enclosing + // job still owns the descendant, which the assertions below require dead. + detached: process.platform === 'win32' && process.env.FAKE_MODE === 'descendant', }); const readyBy = Date.now() + 2000; while (!existsSync(process.env.PID_FILE!)) { diff --git a/test/gen-skill-docs-idempotency.test.ts b/test/gen-skill-docs-idempotency.test.ts index a30b08dc9..f3390563b 100644 --- a/test/gen-skill-docs-idempotency.test.ts +++ b/test/gen-skill-docs-idempotency.test.ts @@ -150,7 +150,6 @@ describe('gen-skill-docs idempotency', () => { stdio: ['ignore', 'pipe', 'pipe'], timeout: 60_000, }); - expect(result.status).toBe(0); const stdout = result.stdout?.toString() ?? ''; // STALE: prefix means a file would change. Count them. const staleLines = stdout.split('\n').filter(l => l.startsWith('STALE:')); @@ -161,6 +160,10 @@ describe('gen-skill-docs idempotency', () => { `\nRun \`bun run gen:skill-docs\` and commit the result.`, ); } + // Report freshness diagnostics before asserting the exit code, so a CI + // failure identifies the stale file or subprocess error that caused it. + expect({ status: result.status, signal: result.signal, error: result.error?.message, + stderr: result.stderr?.toString() ?? '' }, stdout).toMatchObject({ status: 0, signal: null, error: undefined }); }, 90_000); test('--host all idempotency: every host output is byte-stable across two runs', () => { diff --git a/test/gen-skill-docs-prune-stale.test.ts b/test/gen-skill-docs-prune-stale.test.ts index 68c3f2077..64d25ca3e 100644 --- a/test/gen-skill-docs-prune-stale.test.ts +++ b/test/gen-skill-docs-prune-stale.test.ts @@ -34,6 +34,25 @@ function gen(out: string, ...extra: string[]) { describe('gen-skill-docs stale-render prune', () => { test('a gstack-* render for a skill that no longer exists is removed; the sidecar symlink, real skills, un-bannered gstack-* dirs, and other hosts\' trees stay', () => { const out = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-prune-')); + // Keep the sidecar's source disposable: a broken symlink-loop guard must + // never overwrite this checkout's SKILL.md and poison parallel readers. + const source = path.join(out, 'source'); + fs.mkdirSync(path.join(source, 'scripts'), { recursive: true }); + fs.copyFileSync(path.join(ROOT, 'scripts', 'gen-skill-docs.ts'), path.join(source, 'scripts', 'gen-skill-docs.ts')); + for (const file of ['discover-skills.ts', 'gen-llms-txt.ts', 'gen-agents-digest.ts', 'models.ts']) { + fs.symlinkSync(path.join(ROOT, 'scripts', file), path.join(source, 'scripts', file), 'file'); + } + fs.symlinkSync(path.join(ROOT, 'scripts', 'resolvers'), path.join(source, 'scripts', 'resolvers'), 'dir'); + for (const dir of ['hosts', 'lib']) fs.symlinkSync(path.join(ROOT, dir), path.join(source, dir), 'dir'); + for (const skill of ['', 'ship']) { + fs.mkdirSync(path.join(source, skill), { recursive: true }); + fs.writeFileSync(path.join(source, skill, 'SKILL.md.tmpl'), `---\nname: ${skill || 'gstack'}\ndescription: Fixture skill.\n---\nFixture body.\n`); + } + const canonical = path.join(source, 'SKILL.md'); + const metadata = path.join(source, 'agents', 'openai.yaml'); + fs.mkdirSync(path.dirname(metadata), { recursive: true }); + fs.writeFileSync(canonical, 'canonical skill sentinel\n'); + fs.writeFileSync(metadata, 'canonical metadata sentinel\n'); const skills = path.join(out, '.agents', 'skills'); fs.mkdirSync(path.join(skills, 'gstack-retired-zzz'), { recursive: true }); fs.writeFileSync(path.join(skills, 'gstack-retired-zzz', 'SKILL.md'), staleRender('gstack-retired-zzz', 'stale')); @@ -47,15 +66,23 @@ describe('gen-skill-docs stale-render prune', () => { fs.mkdirSync(path.join(skills, 'gstack-lookalike'), { recursive: true }); fs.writeFileSync(path.join(skills, 'gstack-lookalike', 'SKILL.md'), '---\nname: gstack-lookalike\n---\n\nmine\n'); fs.mkdirSync(path.join(skills, 'not-ours'), { recursive: true }); - fs.symlinkSync(ROOT, path.join(skills, 'gstack')); + fs.symlinkSync(source, path.join(skills, 'gstack'), 'dir'); // Host isolation: a bannered stale-looking render under ANOTHER host's tree // in the same out-dir is not a codex run's to prune (or to touch at all). const factorySkills = path.join(out, '.factory', 'skills'); fs.mkdirSync(path.join(factorySkills, 'gstack-zzz'), { recursive: true }); fs.writeFileSync(path.join(factorySkills, 'gstack-zzz', 'SKILL.md'), staleRender('gstack-zzz', 'stale factory')); try { - const r = gen(out); - expect(r.status).toBe(0); + const r = spawnSync('bun', ['run', path.join(source, 'scripts', 'gen-skill-docs.ts'), '--host', 'codex', '--out-dir', out], { + cwd: source, encoding: 'utf-8', timeout: 180_000, + env: { ...process.env, GSTACK_DEFER_CLAUDE_RENAME_PRUNE: '0' }, + }); + expect(r.status, r.stderr).toBe(0); + // On Windows the old realpath + '/' comparison missed this alias, + // rewriting BOTH source artifacts through the sidecar. + expect(r.stdout.replace(/\\/g, '/')).toContain('SKIPPED (symlink loop): .agents/skills/gstack/SKILL.md'); + expect(fs.readFileSync(canonical, 'utf-8')).toBe('canonical skill sentinel\n'); + expect(fs.readFileSync(metadata, 'utf-8')).toBe('canonical metadata sentinel\n'); expect(r.stdout).toContain('pruned stale codex render: gstack-retired-zzz'); expect(fs.existsSync(path.join(skills, 'gstack-retired-zzz'))).toBe(false); // Exactly one prune in this run: the bannered stale render and nothing else.