From 1fd0ea5d6878078d592d39b5e3a229bd06fed738 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 11 Sep 2026 19:34:47 +0000 Subject: [PATCH] fix: preserve file identities and join Windows test processes --- bin/gstack-autoplan-snapshot.ts | 22 +++++++------- test/autoplan-init.test.ts | 44 ++++++++++++++++++++++++++++ test/claude-code-windows-job.test.ts | 32 +++++++++++++++----- 3 files changed, 81 insertions(+), 17 deletions(-) diff --git a/bin/gstack-autoplan-snapshot.ts b/bin/gstack-autoplan-snapshot.ts index c4cb5c2ff..60ce79b18 100644 --- a/bin/gstack-autoplan-snapshot.ts +++ b/bin/gstack-autoplan-snapshot.ts @@ -352,12 +352,12 @@ export function amendImplementation(phase: string, activePlan: string, snapshotP throw new Error('Assembled accepted obligations do not match; no overwrite'); } if (next !== original) { - const before = statSync(source); + const before = statSync(source, { bigint: true }); const directory = mkdtempSync(join(dirname(source), '.autoplan-amend-')); try { const stage = join(directory, 'plan'); - writeFileSync(stage, next, { flag: 'wx', mode: before.mode & 0o777 }); - const current = statSync(source); + writeFileSync(stage, next, { flag: 'wx', mode: Number(before.mode & 0o777n) }); + const current = statSync(source, { bigint: true }); if (before.dev !== current.dev || before.ino !== current.ino || readFileSync(source, 'utf8') !== original) { throw new Error('Active plan changed during amendment; no overwrite'); } @@ -378,13 +378,15 @@ export function initializePlan(sourcePlan: string, activePlan: string, restorePa missing.unshift(basename(parent)); parent = dirname(parent); } const canonical = join(realpathSync(parent), ...missing, basename(file)); - const state = lstatSync(canonical, { throwIfNoEntry: false }); + const state = lstatSync(canonical, { throwIfNoEntry: false, bigint: true }); if (state && !state.isFile()) throw new Error('Initialization destinations must be regular files, not links or directories'); return { file: canonical, state, bytes: state ? readFileSync(canonical) : undefined }; }; const active = destination(activePlan); const restore = destination(restorePath); - const sourceState = statSync(source); + // Windows file IDs can exceed Number's exact range. Preserve their full + // identity for alias, concurrent-change and rollback ownership checks. + const sourceState = statSync(source, { bigint: true }); if (!sourceState.isFile()) throw new Error('Initialization source must be a regular file'); const sourceBytes = readFileSync(source); const sameFile = (a: typeof sourceState, b: typeof sourceState) => a.dev === b.dev && a.ino === b.ino; @@ -423,8 +425,8 @@ export function initializePlan(sourcePlan: string, activePlan: string, restorePa const next = normalized(sourceBytes); const expectedScopeHash = sha256(extractImplementationPlan(next.toString('utf8'))); const unchanged = () => { - const now = statSync(source); - const current = lstatSync(active.file, { throwIfNoEntry: false }); + const now = statSync(source, { bigint: true }); + const current = lstatSync(active.file, { throwIfNoEntry: false, bigint: true }); if (!sameFile(now, sourceState) || !readFileSync(source).equals(sourceBytes) || (active.state ? !current?.isFile() || !sameFile(current, active.state) || !readFileSync(active.file).equals(active.bytes!) : current !== undefined)) { throw new Error('Initialization input or destination changed; refusing to overwrite it'); @@ -454,7 +456,7 @@ export function initializePlan(sourcePlan: string, activePlan: string, restorePa restoreStage = mkdtempSync(join(dirname(restore.file), '.gstack-autoplan-restore-')); const stagedActive = join(activeStage, 'active.md'); const stagedRestore = join(restoreStage, 'original.md'); - writeFileSync(stagedActive, next, { flag: 'wx', mode: active.state ? active.state.mode & 0o777 : 0o600 }); + writeFileSync(stagedActive, next, { flag: 'wx', mode: active.state ? Number(active.state.mode & 0o777n) : 0o600 }); writeFileSync(stagedRestore, sourceBytes, { flag: 'wx', mode: 0o400 }); unchanged(); // Link publishes complete restore bytes exclusively; an existing backup is never replaced. @@ -472,8 +474,8 @@ export function initializePlan(sourcePlan: string, activePlan: string, restorePa } finally { // On pre-publication failure remove only the restore inode this invocation published. if (backupPublished && !activePublished && restoreStage) { - const current = lstatSync(restore.file, { throwIfNoEntry: false }); - if (current?.isFile() && sameFile(current, statSync(join(restoreStage, 'original.md')))) unlinkSync(restore.file); + const current = lstatSync(restore.file, { throwIfNoEntry: false, bigint: true }); + if (current?.isFile() && sameFile(current, statSync(join(restoreStage, 'original.md'), { bigint: true }))) unlinkSync(restore.file); } if (activeStage) rmSync(activeStage, { recursive: true, force: true }); if (restoreStage) rmSync(restoreStage, { recursive: true, force: true }); diff --git a/test/autoplan-init.test.ts b/test/autoplan-init.test.ts index 4b9cff9b2..a8a059b87 100644 --- a/test/autoplan-init.test.ts +++ b/test/autoplan-init.test.ts @@ -171,6 +171,50 @@ test('line endings and Unicode survive normalization; only a missing final separ } }); +test('large file identities remain distinct on reuse while real hardlink aliases are rejected', () => { + const f = fixture(); + const worker = join(f.dir, 'large-file-ids.ts'); + writeFileSync(worker, `import { mock } from 'bun:test'; +const real = { ...await import('node:fs') }; +const ids = new Map(); +function observed(kind, file, options) { + const exact = real[kind](file, { ...options, bigint: true }); + if (!exact) return exact; + const key = exact.dev + ':' + exact.ino; + if (!ids.has(key)) ids.set(key, 2n ** 60n + BigInt(ids.size)); + const ino = ids.get(key); + const state = options?.bigint ? exact : real[kind](file, options); + return new Proxy(state, { get(target, key, receiver) { + return key === 'ino' ? (options?.bigint ? ino : Number(ino)) : Reflect.get(target, key, receiver); + } }); +} +mock.module('node:fs', () => ({ ...real, + statSync: (file, options) => observed('statSync', file, options), + lstatSync: (file, options) => observed('lstatSync', file, options), +})); +const { initializePlan } = await import(${JSON.stringify(TOOL)}); +const [source, active, restore, alias] = process.argv.slice(2); +const initial = initializePlan(source, active, restore); +const reused = initializePlan(source, active, restore); +real.linkSync(source, alias); +let rejected = false; +try { initializePlan(source, alias, restore + '.other'); } +catch (error) { rejected = error.message.includes('ambiguous alias'); } +console.log(JSON.stringify({ initial: initial.reused, reused: reused.reused, rejected, + roundedIds: new Set([...ids.values()].map(Number)).size, exactIds: ids.size })); +`); + const result = spawnSync(process.execPath, [worker, f.source, f.active, f.restore, join(f.dir, 'hardlink.md')], { + encoding: 'utf8', timeout: 10_000, + }); + expect(result.status, result.stderr).toBe(0); + const report = JSON.parse(result.stdout); + expect(report).toMatchObject({ initial: false, reused: true, rejected: true, roundedIds: 1 }); + expect(report.exactIds).toBeGreaterThan(2); + expect(readFileSync(f.source)).toEqual(original); + expect(readFileSync(f.restore)).toEqual(original); + expect(existsSync(f.restore + '.other')).toBe(false); +}); + test('staging failure cleans owned temporary files without changing source or active bytes', () => { const f = fixture(); const active = join(f.dir, 'harness', 'plans', 'assigned.md'); diff --git a/test/claude-code-windows-job.test.ts b/test/claude-code-windows-job.test.ts index 0307264da..17e7347d9 100644 --- a/test/claude-code-windows-job.test.ts +++ b/test/claude-code-windows-job.test.ts @@ -9,6 +9,7 @@ const DIR = mkdtempSync(path.join(tmpdir(), 'claude-windows-job-')); const FAKE = path.join(DIR, 'fake claude.ts'); const DESCENDANT = path.join(DIR, 'pipe holder.ts'); const PID_FILE = path.join(DIR, 'descendant.pid'); +const PROVIDER_PID_FILE = path.join(DIR, 'provider.pid'); const CLI = path.join(ROOT, 'bin/gstack-claude-code'); const FAILED_JOB = path.join(DIR, 'failed job.ts'); @@ -35,6 +36,7 @@ writeFileSync(FAKE, ` import { spawn } from 'node:child_process'; import { existsSync, rmSync, writeFileSync } from 'node:fs'; await Bun.stdin.text(); +writeFileSync(process.env.PROVIDER_PID_FILE!, String(process.pid)); rmSync(process.env.PID_FILE!, { force: true }); const child = spawn(process.execPath, [process.env.DESCENDANT!], { stdio: ['ignore', 'inherit', 'inherit'], @@ -62,6 +64,7 @@ function environment(mode: string): NodeJS.ProcessEnv { GSTACK_CLAUDE_BIN_ARGS: JSON.stringify([FAKE]), FAKE_MODE: mode, PID_FILE, + PROVIDER_PID_FILE, DESCENDANT, }; } @@ -71,15 +74,18 @@ function alive(pid: number): boolean { } async function expectDead(pid: number) { + expect(Number.isSafeInteger(pid) && pid > 0).toBe(true); for (let i = 0; i < 40 && alive(pid); i++) await Bun.sleep(25); expect(alive(pid)).toBe(false); } -function cleanupDescendant() { - try { - const pid = Number(readFileSync(PID_FILE, 'utf8')); - if (alive(pid)) process.kill(pid, 'SIGKILL'); - } catch { /* No descendant was created. */ } +function cleanupOwnedProcesses() { + for (const file of [PID_FILE, PROVIDER_PID_FILE]) { + try { + const pid = Number(readFileSync(file, 'utf8')); + if (Number.isSafeInteger(pid) && pid > 0 && alive(pid)) process.kill(pid, 'SIGKILL'); + } catch { /* No owned process remains. */ } + } } // These exercise the actual standalone CLI boundary. A job must never be @@ -101,7 +107,10 @@ describe('Windows Claude CLI job containment', () => { for (const [mode, expected] of [['descendant', 'output-drain'], ['timeout', 'timeout']]) { test.skipIf(process.platform !== 'win32')(`${mode} kills owned descendants and preserves a sibling process`, async () => { + rmSync(PID_FILE, { force: true }); + rmSync(PROVIDER_PID_FILE, { force: true }); const sibling = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { stdio: 'ignore' }); + const siblingClosed = new Promise(resolve => sibling.once('close', () => resolve())); const started = Date.now(); try { const result = spawnSync(process.execPath, [CLI, '--cwd', DIR, '--access', 'none', '--timeout-ms', '2000'], { @@ -114,19 +123,23 @@ describe('Windows Claude CLI job containment', () => { expect(parsed.error.code).toBe(expected); expect(Date.now() - started).toBeLessThan(6000); await expectDead(Number(readFileSync(PID_FILE, 'utf8'))); + await expectDead(Number(readFileSync(PROVIDER_PID_FILE, 'utf8'))); expect(alive(sibling.pid!)).toBe(true); } finally { - cleanupDescendant(); + cleanupOwnedProcesses(); sibling.kill('SIGKILL'); + await siblingClosed; } }); } test.skipIf(process.platform !== 'win32')('abrupt runner exit closes the job and reaps its descendants', async () => { rmSync(PID_FILE, { force: true }); + rmSync(PROVIDER_PID_FILE, { force: true }); const runner = spawn(process.execPath, [CLI, '--cwd', DIR, '--access', 'none', '--timeout-ms', '10000'], { env: environment('timeout'), stdio: ['pipe', 'ignore', 'ignore'], }); + const runnerClosed = new Promise(resolve => runner.once('close', () => resolve())); runner.stdin!.end('review'); try { let descendant = 0; @@ -136,9 +149,14 @@ describe('Windows Claude CLI job containment', () => { expect(descendant).toBeGreaterThan(0); runner.kill('SIGKILL'); await expectDead(descendant); + // The fake provider also owns the fixture cwd. Job termination starts + // every member's shutdown; leaf death alone does not prove it is done. + await expectDead(Number(readFileSync(PROVIDER_PID_FILE, 'utf8'))); } finally { runner.kill('SIGKILL'); - cleanupDescendant(); + cleanupOwnedProcesses(); + // Join the direct child and close its streams before fixture teardown. + await runnerClosed; } }); });