Merge branch 'next' into fix/1400-agent-skills-stdout-flush
This commit is contained in:
Tom Boucher
5
.changeset/1532-core-lock-liveness.md
Normal file
5
.changeset/1532-core-lock-liveness.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 1532
|
||||
---
|
||||
**Core-path file locks now verify the holder process is alive before stealing a stale lock (#1532)** — the STATE.md write lock (`acquireStateLock`) and the `.planning/` workspace lock (`withPlanningLock`) previously stole locks on a bare `mtime` timer with no liveness check, so a live-but-slow holder (e.g. a deep `.planning/` scan on slow NFS) could have its lock stolen mid-write, corrupting STATE.md or losing an update. Both locks now gate stealing on `process.kill(pid,0)` liveness with a deadman ceiling above the wait budget (pid-reuse backstop), `withPlanningLock` no longer force-steals a live holder on timeout (and can no longer leak an uncaught `EEXIST`), `writeStateMd` computes its disk scan inside the lock, and `acquireStateLock` no longer leaks a file descriptor or strands an empty lock on a recoverable write error. The steal itself is now race-safe: a lock is never stolen while its body is still being written (the create→pid-write window), and stealing uses an atomic rename with an identity re-confirm so two waiters can no longer both reclaim the same lock and end up holding it concurrently. The uncontended path is unchanged.
|
||||
5
.changeset/proud-sloths-glide.md
Normal file
5
.changeset/proud-sloths-glide.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 1574
|
||||
---
|
||||
**OpenCode and other AGENTS-native runtimes now get a root `AGENTS.md` from `/gsd:new-project`** — the workflow hardcoded a codex-only branch that sent every other runtime to `.claude/CLAUDE.md`, a location OpenCode never loads. A shared `getProjectInstructionFile(runtime)` policy (claude→`.claude/CLAUDE.md`, codex/opencode/kilo/kimi→`AGENTS.md`, copilot→`.github/copilot-instructions.md`, antigravity/gemini→`GEMINI.md`) is now the single source of truth consumed by both the new-project workflow and the generate-claude-md path, with a parity test guarding drift.
|
||||
@@ -638,7 +638,7 @@ async function main() {
|
||||
'from-gsd2, frontmatter, gap-analysis, generate-claude-md, generate-claude-profile, ' +
|
||||
'generate-dev-preferences, generate-slug, graphify, history-digest, init, intel, ' +
|
||||
'capability, classify-confidence, git, learnings, list-seeds, list-todos, loop, milestone, package-legitimacy, phase, phase-plan-index, phases, profile-questionnaire, ' +
|
||||
'profile-sample, progress, prompt-budget, requirements, research-plan, research-store, resolve-granularity, resolve-model, roadmap, scaffold, state, ' +
|
||||
'profile-sample, progress, project-instruction-file, prompt-budget, requirements, research-plan, research-store, resolve-granularity, resolve-model, roadmap, scaffold, state, ' +
|
||||
'task, template, user-story, validate, verify, verify-path-exists, verify-summary, workstream, worktree\n\n' +
|
||||
'Global flags:\n' +
|
||||
' --raw Emit raw output without post-processing\n' +
|
||||
@@ -689,6 +689,10 @@ async function main() {
|
||||
'worktree', 'prompt-budget',
|
||||
'research-store', 'research-plan', 'package-legitimacy', 'classify-confidence',
|
||||
'user-story', // pure string validation — no .planning/ access needed
|
||||
// #1529: pure runtime→filename projection via getProjectInstructionFile; no
|
||||
// .planning/ access needed, and resolving project root would break workflow
|
||||
// invocations that run before .planning/ exists (new-project Step 1).
|
||||
'project-instruction-file',
|
||||
]);
|
||||
if (!SKIP_ROOT_RESOLUTION.has(command)) {
|
||||
cwd = findProjectRoot(cwd);
|
||||
@@ -1103,6 +1107,29 @@ async function runCommand(command, args, cwd, raw, defaultValue, originalCommand
|
||||
break;
|
||||
}
|
||||
|
||||
case 'project-instruction-file': {
|
||||
// #1529: pure runtime→filename projection. Backs the
|
||||
// `gsd_run query project-instruction-file --runtime <r>` call in
|
||||
// new-project.md so the bash workflow and profile-output.cjs share one
|
||||
// source of truth (getProjectInstructionFile in runtime-name-policy.cjs).
|
||||
// No SDK bridge — pure local lookup, runs before .planning/ exists.
|
||||
const { getProjectInstructionFile } = require('./lib/runtime-name-policy.cjs');
|
||||
// Parse --runtime <value> (space or = form); default to empty so the
|
||||
// safe AGENTS.md cross-agent default applies.
|
||||
const pifArgs = args.slice(1);
|
||||
let pifRuntime = '';
|
||||
for (let i = 0; i < pifArgs.length; i++) {
|
||||
const a = pifArgs[i];
|
||||
if (a === '--runtime' && pifArgs[i + 1] !== undefined) { pifRuntime = pifArgs[++i]; continue; }
|
||||
if (a.startsWith('--runtime=')) { pifRuntime = a.slice('--runtime='.length); continue; }
|
||||
// First positional that isn't a flag also works (lenient); otherwise ignore unknown flags.
|
||||
if (!a.startsWith('-') && !pifRuntime) { pifRuntime = a; }
|
||||
}
|
||||
const filename = getProjectInstructionFile(pifRuntime);
|
||||
process.stdout.write(filename + '\n');
|
||||
break;
|
||||
}
|
||||
|
||||
case 'list-todos': {
|
||||
commands.cmdListTodos(cwd, args[1], raw);
|
||||
break;
|
||||
|
||||
@@ -109,9 +109,9 @@ elif [ -n "$OPENCODE_CONFIG_DIR" ] || [ -n "$OPENCODE_CONFIG" ]; then RUNTIME="o
|
||||
else RUNTIME="claude"; fi
|
||||
```
|
||||
|
||||
Set the instruction file variable:
|
||||
Set the instruction file variable via the shared runtime-name policy adapter (`gsd-tools query project-instruction-file`, backed by `getProjectInstructionFile` in `runtime-name-policy.cjs` — the single source of truth shared with `profile-output.cjs`):
|
||||
```bash
|
||||
if [ "$RUNTIME" = "codex" ]; then INSTRUCTION_FILE="AGENTS.md"; else INSTRUCTION_FILE=".claude/CLAUDE.md"; fi
|
||||
INSTRUCTION_FILE=$(gsd_run query project-instruction-file --runtime "$RUNTIME")
|
||||
```
|
||||
|
||||
All subsequent references to the project instruction file use `$INSTRUCTION_FILE`.
|
||||
@@ -1533,7 +1533,7 @@ PHASE1_HAS_UI=$(echo "$PHASE1_SECTION" | grep -qi "UI hint.*yes" && echo "true"
|
||||
- `.planning/REQUIREMENTS.md`
|
||||
- `.planning/ROADMAP.md`
|
||||
- `.planning/STATE.md`
|
||||
- `$INSTRUCTION_FILE` (`AGENTS.md` for Codex, `.claude/CLAUDE.md` for all other runtimes)
|
||||
- `$INSTRUCTION_FILE` (runtime-derived via the shared `getProjectInstructionFile` policy: `AGENTS.md` for codex/opencode/kilo/kimi, `.github/copilot-instructions.md` for copilot, `GEMINI.md` for gemini/antigravity, `.claude/CLAUDE.md` for claude)
|
||||
|
||||
</output>
|
||||
|
||||
@@ -1555,7 +1555,7 @@ PHASE1_HAS_UI=$(echo "$PHASE1_SECTION" | grep -qi "UI hint.*yes" && echo "true"
|
||||
- [ ] ROADMAP.md created with phases, requirement mappings, success criteria
|
||||
- [ ] STATE.md initialized
|
||||
- [ ] REQUIREMENTS.md traceability updated
|
||||
- [ ] `$INSTRUCTION_FILE` generated with GSD workflow guidance (AGENTS.md for Codex, `.claude/CLAUDE.md` otherwise; an existing hand-crafted file without GSD markers is left untouched unless `--force`)
|
||||
- [ ] `$INSTRUCTION_FILE` generated with GSD workflow guidance (runtime-derived via the shared `getProjectInstructionFile` policy — `AGENTS.md` for codex/opencode/kilo/kimi, `.github/copilot-instructions.md` for copilot, `GEMINI.md` for gemini/antigravity, `.claude/CLAUDE.md` for claude; an existing hand-crafted file without GSD markers is left untouched unless `--force`)
|
||||
- [ ] User knows next step is `/gsd:discuss-phase 1`
|
||||
|
||||
**Atomic commits:** Each phase commits its artifacts immediately. If context is lost, artifacts persist.
|
||||
|
||||
@@ -37,6 +37,65 @@ process.on('exit', () => {
|
||||
}
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Lock liveness probe (test seam) — audit M1
|
||||
//
|
||||
// mtime is a leaky proxy for "the holder is alive". The prior withPlanningLock
|
||||
// timeout fallback unconditionally unlinked WHATEVER lock existed — even a fresh,
|
||||
// live holder's — and re-acquired it, force-stealing a live writer's critical
|
||||
// section. We backport capability-lock.cts's pid-liveness gate: a dead holder is
|
||||
// stolen promptly inside the polite loop; a live holder is waited on. The
|
||||
// indirection lets unit tests inject a deterministic isPidAlive without real pids.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** Is `pid` a live process? process.kill(pid, 0) succeeds for a live (signalable) process. */
|
||||
function _realIsPidAlive(pid: number): boolean {
|
||||
try {
|
||||
process.kill(pid, 0);
|
||||
return true; // signalable → alive
|
||||
} catch (err) {
|
||||
// EPERM = process exists but we cannot signal it (still ALIVE). ESRCH = gone.
|
||||
return (err as NodeJS.ErrnoException).code === 'EPERM';
|
||||
}
|
||||
}
|
||||
|
||||
const _planningLockProbes: { isPidAlive: (pid: number) => boolean } = { isPidAlive: _realIsPidAlive };
|
||||
|
||||
function _planningLockIsPidAlive(pid: number): boolean {
|
||||
return _planningLockProbes.isPidAlive(pid);
|
||||
}
|
||||
|
||||
// Test seam (PR #1532 review): beforeSteal fires AFTER the steal decision but BEFORE
|
||||
// the identity re-confirm + atomic rename-steal, so a test can recreate a fresh lock
|
||||
// in the decision→steal gap and prove the identity re-confirm aborts a double-steal.
|
||||
// Defaults to a no-op; real callers are byte-for-behaviour unchanged.
|
||||
interface PlanningLockTestHooks {
|
||||
beforeSteal?: (ctx: { lockPath: string }) => void;
|
||||
}
|
||||
const _planningLockTestHooks: PlanningLockTestHooks = {};
|
||||
|
||||
// Monotonic sequence for unique stale-steal rename targets (no crypto dependency).
|
||||
let _planningStealSeq = 0;
|
||||
|
||||
/**
|
||||
* Is the holder recorded in the .lock body VERIFIED-LIVE? The body is JSON
|
||||
* { pid, cwd, acquired }. Returns true ONLY when the body parses AND the recorded
|
||||
* pid signals alive. A garbage / pid-less / unreadable body (or a dead pid) is NOT
|
||||
* verified-live, so the lock stays stealable — corrupt locks never block forever,
|
||||
* and a live holder is never force-stolen.
|
||||
*/
|
||||
function _planningHolderVerifiedLive(lockPath: string): boolean {
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = JSON.parse(fs.readFileSync(lockPath, 'utf-8'));
|
||||
} catch {
|
||||
return false; // unreadable / unparseable body → cannot verify → not verified-live
|
||||
}
|
||||
const pid = (parsed as { pid?: unknown } | null)?.pid;
|
||||
if (typeof pid !== 'number' || !Number.isInteger(pid) || pid <= 0) return false;
|
||||
return _planningLockIsPidAlive(pid);
|
||||
}
|
||||
|
||||
// Transient errno codes that indicate a temporary filesystem condition under
|
||||
// concurrent O_EXCL races — Docker overlay-fs (ENOENT/EINVAL/EIO), NFS
|
||||
// (ESTALE), and OS-level interrupt/retry signals (EAGAIN/EINTR). These are
|
||||
@@ -118,6 +177,12 @@ function withPlanningLock<T>(cwd: string, fn: () => T, clock?: Clock): T {
|
||||
if (clock === undefined) clock = realClock;
|
||||
const lockPath = path.join(planningDir(cwd), '.lock');
|
||||
const lockTimeout = 10000; // 10 seconds
|
||||
// Deadman ceiling (audit M1 / R4-FIX) — set ABOVE lockTimeout so a holder that reads
|
||||
// as alive but is actually a pid-reuse alias (the .lock body has no startTime, so
|
||||
// liveness alone cannot detect reuse) is still recovered once its lock ages past this
|
||||
// absolute ceiling. Without it, a false-alive holder would make withPlanningLock throw
|
||||
// on every call with no self-heal. Mirrors acquireStateLock's deadmanCeilingMs.
|
||||
const deadmanCeilingMs = 60000;
|
||||
const start = clock.now();
|
||||
|
||||
// Ensure .planning/ exists
|
||||
@@ -160,16 +225,68 @@ function withPlanningLock<T>(cwd: string, fn: () => T, clock?: Clock): T {
|
||||
continue;
|
||||
}
|
||||
if (nodeErr.code === 'EEXIST') {
|
||||
// Lock exists — check if stale (>30s old)
|
||||
// Liveness-gated steal (audit M1). Steal the lock PROMPTLY only when its
|
||||
// recorded holder is NOT verified-live (crashed/dead pid or garbage body).
|
||||
// A verified-live holder is waited on — never force-stolen — because nuking
|
||||
// a slow-but-live writer's lock corrupts the .planning/ critical section.
|
||||
// The steal is an ATOMIC rename-then-recreate guarded by an identity re-confirm
|
||||
// so a racer that recreates a fresh lock in the decision→steal gap never has
|
||||
// its replacement deleted (audit M2 / PR #1532 review, window b). The body is
|
||||
// written atomically (writeFileSync …{flag:'wx'}) so there is no empty-body
|
||||
// create window here — only the double-steal needs hardening.
|
||||
try {
|
||||
const stat = fs.statSync(lockPath);
|
||||
if (clock.now() - stat.mtimeMs > 30000) {
|
||||
fs.unlinkSync(lockPath);
|
||||
continue; // retry
|
||||
const decisionStat = fs.statSync(lockPath);
|
||||
// Snapshot the decision-time body too: (dev, ino) alone is defeated by inode
|
||||
// REUSE (a racer's unlink+recreate can land on the same inode), so the body
|
||||
// content binds the identity as well — mirrors capability-lock.cts's (dev,
|
||||
// ino, ts) re-confirm.
|
||||
let decisionBody: string | null;
|
||||
try { decisionBody = fs.readFileSync(lockPath, 'utf-8'); } catch { decisionBody = null; }
|
||||
let stealable = !_planningHolderVerifiedLive(lockPath);
|
||||
if (!stealable) {
|
||||
// Verified-live, but recover anyway once the lock crosses the absolute
|
||||
// deadman ceiling — defeats a pid-reuse false-alive that would otherwise
|
||||
// block forever (R4-FIX; mtime age is from lock creation, not this call).
|
||||
const age = clock.now() - decisionStat.mtimeMs;
|
||||
stealable = age > deadmanCeilingMs;
|
||||
}
|
||||
if (stealable) {
|
||||
if (_planningLockTestHooks.beforeSteal) _planningLockTestHooks.beforeSteal({ lockPath });
|
||||
// Identity re-confirm immediately before the steal: a racer that stole +
|
||||
// recreated a fresh lock in the decision→steal gap changes (dev, ino) → do
|
||||
// NOT delete the replacement; back off and re-evaluate.
|
||||
let confirmStat: fs.Stats;
|
||||
try {
|
||||
confirmStat = fs.statSync(lockPath);
|
||||
} catch {
|
||||
continue; // vanished between decision and steal — retry the create.
|
||||
}
|
||||
let confirmBody: string | null;
|
||||
try { confirmBody = fs.readFileSync(lockPath, 'utf-8'); } catch { confirmBody = null; }
|
||||
const sameInstance =
|
||||
typeof decisionStat.dev === 'number' && typeof decisionStat.ino === 'number' &&
|
||||
confirmStat.dev === decisionStat.dev && confirmStat.ino === decisionStat.ino &&
|
||||
decisionBody !== null && confirmBody === decisionBody;
|
||||
if (!sameInstance) {
|
||||
clock.sleep(100); // a racer won the steal + recreated — re-evaluate, don't delete it.
|
||||
continue;
|
||||
}
|
||||
// Atomic steal: rename the inode aside, then remove it. Only ONE racer can
|
||||
// win the rename; a failed rename means another process already stole it, so
|
||||
// we must NOT fall through to a delete — back off and retry the create.
|
||||
const stolen = lockPath + '.stale-' + process.pid + '-' + clock.now() + '-' + (_planningStealSeq++);
|
||||
let renamed = false;
|
||||
try { fs.renameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ }
|
||||
if (renamed) {
|
||||
try { fs.rmSync(stolen, { force: true }); } catch { /* best-effort */ }
|
||||
continue; // dead/garbage/expired holder freed — retry immediately to grab it.
|
||||
}
|
||||
clock.sleep(100); // lost the steal race — back off and retry.
|
||||
continue;
|
||||
}
|
||||
} catch { continue; }
|
||||
|
||||
// Wait and retry (cross-platform, no shell dependency)
|
||||
// Live holder — wait and retry (cross-platform, no shell dependency).
|
||||
clock.sleep(100);
|
||||
continue;
|
||||
}
|
||||
@@ -177,10 +294,18 @@ function withPlanningLock<T>(cwd: string, fn: () => T, clock?: Clock): T {
|
||||
}
|
||||
}
|
||||
|
||||
// Timeout — stale-lock recovery, then re-acquire atomically before entering critical section.
|
||||
try { fs.unlinkSync(lockPath); } catch { /* ok */ }
|
||||
acquireLock();
|
||||
return runWithHeldLock();
|
||||
// Timeout against a holder still present at budget exhaustion. The polite loop
|
||||
// already stole any DEAD holder; reaching here means the holder is verified-live
|
||||
// (or a pid-reuse alias we must not corrupt). Do NOT force-steal — the prior
|
||||
// unconditional `unlinkSync(lockPath); acquireLock()` here (audit M1) robbed live
|
||||
// writers, and its re-acquire sat OUTSIDE any try so a concurrent re-create raced
|
||||
// a raw EEXIST out of the helper (audit M2). Surface a clear timeout error instead.
|
||||
const timeoutErr = new Error(
|
||||
'withPlanningLock: ' + lockPath + ' held by a live process for ' +
|
||||
(clock.now() - start) + 'ms (exceeded ' + lockTimeout + 'ms budget)'
|
||||
);
|
||||
(timeoutErr as unknown as Record<string, unknown>).lockTimeout = true;
|
||||
throw timeoutErr;
|
||||
}
|
||||
|
||||
function createPlanningWorkspace(cwd: string, opts: WorkstreamAdapterOpts = {}): {
|
||||
@@ -269,4 +394,19 @@ export = {
|
||||
getActiveWorkstream,
|
||||
setActiveWorkstream,
|
||||
findContextMdIn,
|
||||
// Test seam (audit M1): inject a deterministic isPidAlive so the liveness-gated
|
||||
// steal decision is exercised without real pids. Mirrors capability-lock.cts.
|
||||
_setLockProbes(probes: Partial<{ isPidAlive: (pid: number) => boolean }>): void {
|
||||
if (typeof probes.isPidAlive === 'function') _planningLockProbes.isPidAlive = probes.isPidAlive;
|
||||
},
|
||||
_resetLockProbes(): void {
|
||||
_planningLockProbes.isPidAlive = _realIsPidAlive;
|
||||
},
|
||||
// Test seam (PR #1532 review): script the steal decision→steal gap (window b).
|
||||
_setPlanningLockTestHooks(hooks: PlanningLockTestHooks): void {
|
||||
if ('beforeSteal' in hooks) _planningLockTestHooks.beforeSteal = hooks.beforeSteal;
|
||||
},
|
||||
_resetPlanningLockTestHooks(): void {
|
||||
delete _planningLockTestHooks.beforeSteal;
|
||||
},
|
||||
};
|
||||
|
||||
@@ -25,7 +25,7 @@ const { loadConfig } = configLoader;
|
||||
import { platformReadSync as safeReadFile, platformWriteSync, platformEnsureDir } from './shell-command-projection.cjs';
|
||||
import { getGlobalSkillDir, getGlobalConfigDir } from './runtime-homes.cjs';
|
||||
import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs';
|
||||
import { resolveRuntimeNameFromCandidates } from './runtime-name-policy.cjs';
|
||||
import { resolveRuntimeNameFromCandidates, getProjectInstructionFile } from './runtime-name-policy.cjs';
|
||||
|
||||
// ─── Types ────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -1120,20 +1120,32 @@ function cmdGenerateClaudeMd(cwd: string, options: CmdGenerateClaudeMdOptions, r
|
||||
// repo-root `CLAUDE.md`, so generated GSD content does not land next to — or
|
||||
// pollute — a hand-crafted repo-root CLAUDE.md. An explicit `claude_md_path`
|
||||
// config value or `--output` still wins.
|
||||
let configClaudeMdPath = './.claude/CLAUDE.md';
|
||||
let configClaudeMdPath = '.claude/CLAUDE.md';
|
||||
try {
|
||||
const config = loadConfig(cwd);
|
||||
if (config['claude_md_path']) configClaudeMdPath = config['claude_md_path'] as string;
|
||||
if (config['claude_md_assembly']) assemblyConfig = config['claude_md_assembly'] as Record<string, unknown>;
|
||||
// #3163: When runtime is codex, override the output target to AGENTS.md
|
||||
// regardless of claude_md_path, so Codex projects never write to CLAUDE.md.
|
||||
// GSD_RUNTIME env var takes precedence over config.runtime, mirroring detectRuntime().
|
||||
// #1529: When no explicit --output is provided, derive the instruction
|
||||
// file from the runtime via the shared `getProjectInstructionFile` policy
|
||||
// (single source of truth in runtime-name-policy.cjs, shared with the
|
||||
// new-project.md bash workflow via `gsd-tools query
|
||||
// project-instruction-file`). Previously this was a codex-only override
|
||||
// (#3163) that left AGENTS-native runtimes (opencode/kilo/kimi) emitting
|
||||
// CLAUDE.md; copilot now resolves to .github/copilot-instructions.md, and
|
||||
// antigravity/gemini to GEMINI.md. GSD_RUNTIME env var takes precedence
|
||||
// over config.runtime, mirroring detectRuntime().
|
||||
//
|
||||
// Non-claude runtimes always win over a stale `claude_md_path` (the #3163
|
||||
// rationale: a Codex/AGENTS-native project must never write to CLAUDE.md
|
||||
// even if a prior Claude setup left a `claude_md_path` behind). For the
|
||||
// claude runtime, `claude_md_path` config is honored — it IS the
|
||||
// Claude-specific output setting (per #1098 and the #3163 non-codex test).
|
||||
const effectiveRuntime = resolveRuntimeNameFromCandidates(
|
||||
process.env['GSD_RUNTIME'],
|
||||
config['runtime']
|
||||
);
|
||||
if (!options.output && effectiveRuntime === 'codex') {
|
||||
configClaudeMdPath = './AGENTS.md';
|
||||
if (!options.output && effectiveRuntime && effectiveRuntime !== 'claude') {
|
||||
configClaudeMdPath = getProjectInstructionFile(effectiveRuntime);
|
||||
}
|
||||
} catch { /* use default */ }
|
||||
|
||||
|
||||
@@ -89,6 +89,48 @@ export function resolveRuntimeNameFromCandidates(...candidates: unknown[]): stri
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Map a runtime id to its project instruction file path (relative to project
|
||||
* root). Bug #1529: this is the SINGLE source of truth shared by both
|
||||
* consumption surfaces —
|
||||
* (A) the Node surface: profile-output.cjs (generate-claude-md handler)
|
||||
* (B) the bash surface: `gsd-tools query project-instruction-file --runtime <r>`,
|
||||
* consumed by gsd-core/workflows/new-project.md to set $INSTRUCTION_FILE
|
||||
*
|
||||
* Mapping table (per the #1529 issue contract):
|
||||
*
|
||||
* claude → .claude/CLAUDE.md
|
||||
* codex, opencode, kilo, kimi → AGENTS.md
|
||||
* copilot → .github/copilot-instructions.md
|
||||
* antigravity, gemini → GEMINI.md
|
||||
* unknown / future runtimes → AGENTS.md (safe cross-agent default)
|
||||
*
|
||||
* Source-of-truth references for each runtime's read path:
|
||||
* - copilot: GitHub Docs — repository-wide custom instructions are read ONLY
|
||||
* from `.github/copilot-instructions.md`; a root `copilot-instructions.md`
|
||||
* is not a read path. `AGENTS.md` is also read (agent instructions).
|
||||
* https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions
|
||||
* (Installer parity: runtime-config-adapter-registry.cts installSurface
|
||||
* 'copilot-instructions' writes the same `.github/copilot-instructions.md`.)
|
||||
* - codex/opencode/kilo/kimi: AGENTS.md is the documented cross-agent
|
||||
* instruction file (agentsmd/agents.md convention).
|
||||
* - antigravity/gemini: GEMINI.md is Gemini CLI's contextFileName.
|
||||
*
|
||||
* Aliases are normalized via `canonicalizeRuntimeName` first, so inputs like
|
||||
* `codex-cli` resolve to `codex` → `AGENTS.md`. Replaces the prior codex-only
|
||||
* override in profile-output.cjs (#3163) which left AGENTS-native runtimes
|
||||
* (opencode/kilo/kimi) incorrectly emitting `.claude/CLAUDE.md`. Pure: no I/O.
|
||||
*/
|
||||
export function getProjectInstructionFile(runtime: unknown): string {
|
||||
const canonical = canonicalizeRuntimeName(runtime);
|
||||
if (canonical === 'claude') return '.claude/CLAUDE.md';
|
||||
if (canonical === 'copilot') return '.github/copilot-instructions.md';
|
||||
if (canonical === 'antigravity' || canonical === 'gemini') return 'GEMINI.md';
|
||||
// codex, opencode, kilo, kimi, AND unknown/future runtimes all default to
|
||||
// root AGENTS.md (the safe cross-agent instruction file).
|
||||
return 'AGENTS.md';
|
||||
}
|
||||
|
||||
/**
|
||||
* Map a canonical runtime id to its on-disk local config directory name
|
||||
* (e.g. `cursor` -> `.cursor`, `windsurf` -> `.devin`). Unknown/empty inputs
|
||||
|
||||
282
src/state.cts
282
src/state.cts
@@ -152,6 +152,116 @@ process.on('exit', () => {
|
||||
}
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Lock liveness probe (test seam) — audit M1
|
||||
//
|
||||
// mtime is a LEAKY proxy for "the holder is still alive": a live-but-slow writer
|
||||
// whose critical section runs past staleThresholdMs ages out and a waiter would
|
||||
// steal its lock → two writers in STATE.md's read-modify-write window → lost
|
||||
// update / corruption (the recurring #500/#905/#1230 family). The real signal —
|
||||
// process.kill(pid, 0) — is already used by capability-lock.cts. We backport it
|
||||
// here. The indirection lets unit tests inject a deterministic isPidAlive without
|
||||
// real pids (mirrors capability-lock's _lockProbes / _setLockProbes seam).
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** Is `pid` a live process? process.kill(pid, 0) succeeds for a live (signalable) process. */
|
||||
function _realIsPidAlive(pid: number): boolean {
|
||||
try {
|
||||
process.kill(pid, 0);
|
||||
return true; // signalable → alive
|
||||
} catch (err) {
|
||||
// EPERM = process exists but we cannot signal it (still ALIVE). ESRCH = gone.
|
||||
return (err as NodeJS.ErrnoException).code === 'EPERM';
|
||||
}
|
||||
}
|
||||
|
||||
const _stateLockProbes: { isPidAlive: (pid: number) => boolean } = { isPidAlive: _realIsPidAlive };
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// State-lock test hooks (test seam) — audit M8 / M9
|
||||
//
|
||||
// Both M8 (scan-before-lock TOCTOU in writeStateMd) and M9 (orphan empty lock +
|
||||
// fd leak on a recoverable writeSync/closeSync error in acquireStateLock) are
|
||||
// concurrency / resource-safety issues a single-threaded test cannot otherwise
|
||||
// observe. These purpose-built hooks make the failure windows deterministic
|
||||
// (mirrors the M1 _setLockProbes seam above):
|
||||
//
|
||||
// afterAcquire(lockPath) — fired inside writeStateMd immediately AFTER the lock
|
||||
// is acquired. A test can mutate the disk here (simulate a concurrent writer
|
||||
// landing in the scan→lock window) to prove the disk scan runs INSIDE the lock.
|
||||
// simulateWriteError — a ONE-SHOT errno string. When set, the next writeSync
|
||||
// inside acquireStateLock throws it (and the hook self-clears), forcing the
|
||||
// openSync-succeeds-then-write-fails cleanup path without an OS-level fault.
|
||||
// onLoopIteration(ctx) — fired at the TOP of each acquireStateLock retry
|
||||
// iteration so a test can snapshot whether an orphan lock is stranded.
|
||||
// beforeSteal(ctx) — fired AFTER the steal decision but BEFORE the identity
|
||||
// re-confirm + atomic rename-steal. A test can recreate a fresh lock here to
|
||||
// simulate a racer winning the steal in the decision→steal gap, proving the
|
||||
// identity re-confirm aborts a double-steal (PR #1532 review window b).
|
||||
//
|
||||
// All hooks default to no-ops; real callers are byte-for-behaviour unchanged.
|
||||
// ---------------------------------------------------------------------------
|
||||
interface StateLockTestHooks {
|
||||
afterAcquire?: (lockPath: string) => void;
|
||||
simulateWriteError?: string | null;
|
||||
onLoopIteration?: (ctx: { iteration: number }) => void;
|
||||
beforeSteal?: (ctx: { lockPath: string }) => void;
|
||||
}
|
||||
const _stateLockTestHooks: StateLockTestHooks = {};
|
||||
|
||||
/**
|
||||
* Consume the one-shot simulateWriteError errno, if set. Returns an Error with the
|
||||
* configured `.code` and self-clears so only the NEXT writeSync throws (the retry
|
||||
* then succeeds). Returns null when no injection is pending.
|
||||
*/
|
||||
function _consumeSimulatedWriteError(): NodeJS.ErrnoException | null {
|
||||
const code = _stateLockTestHooks.simulateWriteError;
|
||||
if (!code) return null;
|
||||
_stateLockTestHooks.simulateWriteError = null; // one-shot
|
||||
const e = new Error('simulated writeSync failure (' + code + ')') as NodeJS.ErrnoException;
|
||||
e.code = code;
|
||||
return e;
|
||||
}
|
||||
|
||||
function _stateLockIsPidAlive(pid: number): boolean {
|
||||
return _stateLockProbes.isPidAlive(pid);
|
||||
}
|
||||
|
||||
/**
|
||||
* Is the holder recorded in the lock body VERIFIED-LIVE? The STATE.md lock body is
|
||||
* a bare pid (written at acquire time). Returns true ONLY when the body parses to a
|
||||
* positive integer pid AND that pid signals alive. A garbage / non-numeric / legacy
|
||||
* body (or a dead pid) is NOT verified-live, so the lock stays stealable — corrupt
|
||||
* locks never block forever, and a live holder is never stolen.
|
||||
*/
|
||||
function _stateHolderVerifiedLive(lockPath: string): boolean {
|
||||
const pid = _stateLockBodyPid(lockPath);
|
||||
return pid !== null && _stateLockIsPidAlive(pid);
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse the lock body to its recorded pid, or null when the body is empty / non-numeric
|
||||
* / unreadable (legacy or mid-creation). Distinguishing a COMPLETE dead-pid body (steal
|
||||
* promptly) from an EMPTY/unparseable one (the create→write window — do not steal while
|
||||
* fresh) is what `_stateHolderVerifiedLive` alone cannot express, so the steal decision
|
||||
* in acquireStateLock reads the pid directly (PR #1532 review, window a).
|
||||
*/
|
||||
function _stateLockBodyPid(lockPath: string): number | null {
|
||||
let body: string;
|
||||
try {
|
||||
body = fs.readFileSync(lockPath, 'utf-8');
|
||||
} catch {
|
||||
return null; // unreadable body → cannot verify
|
||||
}
|
||||
const trimmed = body.trim();
|
||||
const pid = parseInt(trimmed, 10);
|
||||
if (!Number.isInteger(pid) || pid <= 0 || String(pid) !== trimmed) return null;
|
||||
return pid;
|
||||
}
|
||||
|
||||
// Monotonic sequence for unique stale-steal rename targets (no crypto dependency).
|
||||
let _stateStealSeq = 0;
|
||||
|
||||
// Hoisted to module scope — compiled once, not per call (#320). Stateless (/i, used with .match).
|
||||
const byPhaseTablePattern = /(\|\s*Phase\s*\|\s*Plans\s*\|\s*Total\s*\|\s*Avg\/Plan\s*\|[ \t]*\n\|(?:[- :\t]+\|)+[ \t]*\n)((?:[ \t]*\|[^\n]*\n)*)(?=\n|$)/i;
|
||||
|
||||
@@ -1587,8 +1697,23 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string {
|
||||
if (clock === undefined) clock = realClock;
|
||||
const lockPath = statePath + '.lock';
|
||||
const retryDelay = 200; // ms
|
||||
const staleThresholdMs = 10000;
|
||||
const maxWaitMs = 30000;
|
||||
// Deadman ceiling (audit M1) — set ABOVE maxWaitMs so a holder that reads as
|
||||
// VERIFIED-LIVE is NEVER stolen within the wait budget; only a crashed (dead
|
||||
// pid) or unparseable-body lock is stolen, and a pid-reuse holder (reads alive
|
||||
// but is unrelated) is recovered once age crosses this absolute ceiling rather
|
||||
// than blocking forever. The prior mtime-only `staleThresholdMs = 10000` gate
|
||||
// was BELOW maxWaitMs, so a live-but-slow holder >10 s was robbed mid-write.
|
||||
const deadmanCeilingMs = 60000;
|
||||
// Fresh-create floor (PR #1532 review, window a) — a lock with an EMPTY/unparseable
|
||||
// body is either mid-creation (O_EXCL create done, pid not yet written by the holder)
|
||||
// or a genuine orphan. While such a body is younger than this floor it is treated as
|
||||
// mid-creation and is NEVER stolen — stealing it at age ≈ 0 robs a holder still
|
||||
// writing its pid (the lost-update window capability-lock.cts's `age <= LOCK_STALE_MS`
|
||||
// floor closes). The create→write gap is sub-millisecond; this floor is orders of
|
||||
// magnitude larger yet well under maxWaitMs so a real orphan still clears within budget.
|
||||
// A COMPLETE dead-pid body is NOT subject to this floor — it is stolen promptly.
|
||||
const freshCreateFloorMs = 1000;
|
||||
const startedAt = clock.now();
|
||||
|
||||
// Shared helper: check the time budget then back off with jitter before the
|
||||
@@ -1607,11 +1732,33 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string {
|
||||
clock.sleep(retryDelay + jitter);
|
||||
};
|
||||
|
||||
let _loopIteration = 0;
|
||||
while (true) {
|
||||
if (_stateLockTestHooks.onLoopIteration) _stateLockTestHooks.onLoopIteration({ iteration: _loopIteration++ });
|
||||
try {
|
||||
const fd = fs.openSync(lockPath, fs.constants.O_CREAT | fs.constants.O_EXCL | fs.constants.O_WRONLY);
|
||||
fs.writeSync(fd, String(process.pid));
|
||||
fs.closeSync(fd);
|
||||
// Audit M9 (resource-safety): once the exclusive create SUCCEEDS, a
|
||||
// writeSync/closeSync failure must NOT leak the fd or strand the just-created
|
||||
// (now empty) lock — an orphan body self-blocks every later acquirer until a
|
||||
// liveness steal or the deadman. On any write/close error, guardedly close the
|
||||
// fd and unlink the file we created, then re-throw to the existing outer catch
|
||||
// (which keeps classifying recoverable vs fatal errnos — DRY). A FATAL errno
|
||||
// still propagates after cleanup; a RECOVERABLE one retries from a clean slate.
|
||||
// Mirrors capability-lock.cts:415-425.
|
||||
try {
|
||||
const injected = _consumeSimulatedWriteError();
|
||||
if (injected) throw injected; // test seam: one-shot writeSync failure (M9)
|
||||
fs.writeSync(fd, String(process.pid));
|
||||
fs.closeSync(fd);
|
||||
} catch (writeErr) {
|
||||
try { fs.closeSync(fd); } catch { /* best-effort — fd may already be closed */ }
|
||||
// Best-effort unlink of the lock WE just created. Guarded so we never throw
|
||||
// here; if another acquirer already stole the empty lock the unlink is a
|
||||
// harmless ENOENT no-op (we do not double-unlink someone else's lock — the
|
||||
// open(O_EXCL) above guarantees we created this path this iteration).
|
||||
try { fs.unlinkSync(lockPath); } catch { /* best-effort — no orphan */ }
|
||||
throw writeErr; // re-throw to the outer catch for recoverable/fatal classification
|
||||
}
|
||||
// Exit-time cleanup keeps a crashed locked region from leaving a stale file (#1916).
|
||||
_heldStateLocks.add(lockPath);
|
||||
return lockPath;
|
||||
@@ -1625,31 +1772,80 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string {
|
||||
continue;
|
||||
}
|
||||
if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err; // propagate — silent bypass causes lost updates
|
||||
// Only unlink a lock we did not place when it has crossed the staleness
|
||||
// threshold (crashed holder). Nuking a fresh lock held by a slow-but-live
|
||||
// writer causes lost updates (#3711 regression).
|
||||
// Liveness-gated steal (audit M1) + steal-safety (PR #1532 review). The steal
|
||||
// decision is three-way on the lock body:
|
||||
// - VERIFIED-LIVE holder (parseable pid that signals alive): NEVER stolen until
|
||||
// its age crosses the absolute deadman ceiling (the pid-reuse backstop) —
|
||||
// nuking a slow-but-live writer's lock causes lost updates (#3711 / #500/#905/
|
||||
// #1230 family).
|
||||
// - COMPLETE DEAD pid (parseable pid, not alive): stolen PROMPTLY regardless of
|
||||
// age — a crashed holder left a full body.
|
||||
// - EMPTY / unparseable body: liveness is unknowable. While FRESH (age <=
|
||||
// freshCreateFloorMs) it is a lock still mid-creation (O_EXCL done, pid not yet
|
||||
// written) and is NOT stolen (window a); only once aged past the floor is it a
|
||||
// genuine orphan and stealable.
|
||||
// The steal itself is an ATOMIC rename-then-recreate (only one racer can rename the
|
||||
// inode) guarded by an identity re-confirm, so a racer that recreates a fresh lock
|
||||
// in the decision→steal gap never has its replacement deleted (window b). Mirrors
|
||||
// capability-lock.cts:455-499.
|
||||
try {
|
||||
const stat = fs.statSync(lockPath);
|
||||
if ((clock).now() - stat.mtimeMs > staleThresholdMs) {
|
||||
let removed = false;
|
||||
try { fs.unlinkSync(lockPath); removed = true; } catch { /* swallow: bounded below */ }
|
||||
if (removed) {
|
||||
// Successful steal — retry immediately to grab the just-freed lock.
|
||||
// Must NOT call checkBudgetAndSleep here: a throw-after-delete would
|
||||
// corrupt the filesystem state, and the budget is already bounded on
|
||||
// the next iteration's EEXIST or open attempt (#1217 regression fix).
|
||||
const ageMs = clock.now() - stat.mtimeMs;
|
||||
const bodyPid = _stateLockBodyPid(lockPath);
|
||||
const holderLive = bodyPid !== null && _stateLockIsPidAlive(bodyPid);
|
||||
let steal: boolean;
|
||||
if (holderLive) {
|
||||
steal = ageMs > deadmanCeilingMs; // pid-reuse backstop only
|
||||
} else if (bodyPid !== null) {
|
||||
steal = true; // complete dead pid → prompt steal
|
||||
} else {
|
||||
steal = ageMs > freshCreateFloorMs; // empty/garbage → protect the create window
|
||||
}
|
||||
if (steal) {
|
||||
if (_stateLockTestHooks.beforeSteal) _stateLockTestHooks.beforeSteal({ lockPath });
|
||||
// Identity re-confirm immediately before the steal: a racer that stole +
|
||||
// recreated a fresh lock in the decision→steal gap changes (dev, ino) and/or
|
||||
// the body pid → do NOT delete the replacement; re-evaluate from scratch.
|
||||
let confirmStat: fs.Stats;
|
||||
try {
|
||||
confirmStat = fs.statSync(lockPath);
|
||||
} catch {
|
||||
continue; // lock vanished between decision and steal — retry the create.
|
||||
}
|
||||
const sameInstance =
|
||||
typeof stat.dev === 'number' && typeof stat.ino === 'number' &&
|
||||
confirmStat.dev === stat.dev && confirmStat.ino === stat.ino &&
|
||||
_stateLockBodyPid(lockPath) === bodyPid;
|
||||
if (!sameInstance) {
|
||||
// The lock changed under us (a racer won the steal + recreated). Back off
|
||||
// and re-evaluate rather than deleting the racer's fresh replacement.
|
||||
checkBudgetAndSleep('lock changed before steal');
|
||||
continue;
|
||||
}
|
||||
// Persistent unlinkSync failure — apply budget + backoff so it cannot
|
||||
// busy-spin (#1217).
|
||||
checkBudgetAndSleep('stale lock removal failed');
|
||||
// Atomic steal: rename the inode aside, then remove it. Only ONE racer can
|
||||
// win the rename; a failed rename means another process already stole it, so
|
||||
// we must NOT fall through to a delete — back off and retry the create.
|
||||
const stolen = lockPath + '.stale-' + process.pid + '-' + clock.now() + '-' + (_stateStealSeq++);
|
||||
let renamed = false;
|
||||
try { fs.renameSync(lockPath, stolen); renamed = true; } catch { /* another racer won */ }
|
||||
if (renamed) {
|
||||
try { fs.rmSync(stolen, { force: true }); } catch { /* best-effort */ }
|
||||
// Successful steal — retry immediately to grab the just-freed lock.
|
||||
// Must NOT call checkBudgetAndSleep here: a throw-after-rename would
|
||||
// corrupt filesystem state, and the budget is already bounded on the next
|
||||
// iteration's EEXIST or open attempt (#1217 regression fix).
|
||||
continue;
|
||||
}
|
||||
// Lost the steal race (or a transient rename failure) — apply budget + backoff
|
||||
// so it cannot busy-spin (#1217).
|
||||
checkBudgetAndSleep('stale lock steal lost to racer');
|
||||
continue;
|
||||
}
|
||||
} catch (err) {
|
||||
// Re-throw a budget-exceeded error from the unlinkSync failure path above
|
||||
// unchanged — its message already names the real cause ("stale lock removal
|
||||
// failed") and double-wrapping it would replace that with the misleading
|
||||
// "statSync failed after EEXIST" context string (#1217 diagnostic fix).
|
||||
// Re-throw a budget-exceeded error from the steal path above unchanged — its
|
||||
// message already names the real cause ("lock changed before steal" / "stale
|
||||
// lock steal lost to racer") and double-wrapping it would replace that with the
|
||||
// misleading "statSync failed after EEXIST" context string (#1217 diagnostic fix).
|
||||
if ((err as Record<string, unknown>)?.lockBudgetExceeded) throw err;
|
||||
// statSync failed — lock was likely released between our EEXIST and this
|
||||
// stat call. Apply budget + backoff so a persistent statSync failure
|
||||
@@ -1689,13 +1885,24 @@ function withStateLock<T>(statePath: string, fn: () => T): T {
|
||||
* Optional clock seam; defaults to realClock. Passed through to acquireStateLock.
|
||||
*/
|
||||
function writeStateMd(statePath: string, content: string, cwd?: string, clock?: StateLockClock): void {
|
||||
// Invalidate disk scan cache before computing new frontmatter — the write
|
||||
// may create new PLAN/SUMMARY files that buildStateFrontmatter must see.
|
||||
// Safe for any calling pattern, not just short-lived CLI processes (#1967).
|
||||
if (cwd) _diskScanCache.delete(cwd);
|
||||
const synced = syncStateFrontmatter(content, cwd);
|
||||
const lockPath = acquireStateLock(statePath, clock);
|
||||
// Test seam (audit M8): fire AFTER the lock is taken so a test can simulate a
|
||||
// concurrent writer landing in the (now-closed) scan→lock window.
|
||||
if (_stateLockTestHooks.afterAcquire) _stateLockTestHooks.afterAcquire(lockPath);
|
||||
try {
|
||||
// Audit M8 (leaky-abstractions): the disk scan that counts PLAN/SUMMARY files
|
||||
// to build the frontmatter is the READ half of this read-modify-write — it must
|
||||
// run INSIDE the lock (mirroring readModifyWriteStateMd), not before it. Scanning
|
||||
// before acquireStateLock left a TOCTOU window where a concurrent writer that
|
||||
// committed a new PLAN/SUMMARY between our scan and our lock made writeStateMd
|
||||
// stamp STALE progress counts (lost update — the #500/#905/#1230 family). The
|
||||
// scan order is otherwise byte-for-behaviour identical for single-threaded
|
||||
// callers — only the concurrent-writer window closes.
|
||||
//
|
||||
// Invalidate the disk scan cache first — the write may create new PLAN/SUMMARY
|
||||
// files that buildStateFrontmatter must see (#1967).
|
||||
if (cwd) _diskScanCache.delete(cwd);
|
||||
const synced = syncStateFrontmatter(content, cwd);
|
||||
platformWriteSync(statePath, synced);
|
||||
} finally {
|
||||
releaseStateLock(lockPath);
|
||||
@@ -2891,4 +3098,27 @@ export = {
|
||||
cmdStateMilestoneSwitch,
|
||||
cmdSignalWaiting,
|
||||
cmdSignalResume,
|
||||
// Test seam (audit M1): inject a deterministic isPidAlive so the liveness-gated
|
||||
// steal decision is exercised without real pids. Mirrors capability-lock.cts.
|
||||
_setLockProbes(probes: Partial<{ isPidAlive: (pid: number) => boolean }>): void {
|
||||
if (typeof probes.isPidAlive === 'function') _stateLockProbes.isPidAlive = probes.isPidAlive;
|
||||
},
|
||||
_resetLockProbes(): void {
|
||||
_stateLockProbes.isPidAlive = _realIsPidAlive;
|
||||
},
|
||||
// Test seam (audit M8/M9): inject deterministic hooks for the scan-in-lock window
|
||||
// (afterAcquire), the one-shot recoverable writeSync failure (simulateWriteError),
|
||||
// and per-iteration orphan-lock snapshots (onLoopIteration). See _stateLockTestHooks.
|
||||
_setStateLockTestHooks(hooks: StateLockTestHooks): void {
|
||||
if ('afterAcquire' in hooks) _stateLockTestHooks.afterAcquire = hooks.afterAcquire;
|
||||
if ('simulateWriteError' in hooks) _stateLockTestHooks.simulateWriteError = hooks.simulateWriteError;
|
||||
if ('onLoopIteration' in hooks) _stateLockTestHooks.onLoopIteration = hooks.onLoopIteration;
|
||||
if ('beforeSteal' in hooks) _stateLockTestHooks.beforeSteal = hooks.beforeSteal;
|
||||
},
|
||||
_resetStateLockTestHooks(): void {
|
||||
delete _stateLockTestHooks.afterAcquire;
|
||||
delete _stateLockTestHooks.simulateWriteError;
|
||||
delete _stateLockTestHooks.onLoopIteration;
|
||||
delete _stateLockTestHooks.beforeSteal;
|
||||
},
|
||||
};
|
||||
|
||||
@@ -36,7 +36,8 @@ const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
|
||||
const { makeFakeClock } = require('./helpers/clock.cjs');
|
||||
const { acquireStateLock, releaseStateLock, readModifyWriteStateMd } = require('../gsd-core/bin/lib/state.cjs');
|
||||
const stateMod = require('../gsd-core/bin/lib/state.cjs');
|
||||
const { acquireStateLock, releaseStateLock, readModifyWriteStateMd } = stateMod;
|
||||
const { withPlanningLock } = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs');
|
||||
|
||||
@@ -123,6 +124,221 @@ describe('acquireStateLock clock seam', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// 1a. acquireStateLock PID-liveness staleness (audit M1)
|
||||
//
|
||||
// mtime is a leaky proxy for "holder is alive": a live-but-slow holder whose
|
||||
// critical section runs past staleThresholdMs ages out and gets its lock stolen
|
||||
// by a waiter → two writers in STATE.md's critical section → lost update.
|
||||
// The fix gates the steal on a real liveness signal (process.kill(pid,0),
|
||||
// injected via the _setLockProbes seam) and orders the deadman ceiling ABOVE the
|
||||
// wait budget so a verified-live holder is NEVER stolen within budget. A dead
|
||||
// holder is stolen promptly regardless of age. A garbage/legacy body is treated
|
||||
// as not-verified-live so corrupt locks stay recoverable under the deadman ceiling.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock PID-liveness staleness (audit M1)', () => {
|
||||
let tmpDir;
|
||||
let statePath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-liveness-state-'));
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
statePath = path.join(tmpDir, '.planning', 'STATE.md');
|
||||
fs.writeFileSync(statePath, '# State\n');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
stateMod._resetLockProbes();
|
||||
try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('exports _setLockProbes / _resetLockProbes seams', () => {
|
||||
assert.ok(typeof stateMod._setLockProbes === 'function', '_setLockProbes seam must be exported');
|
||||
assert.ok(typeof stateMod._resetLockProbes === 'function', '_resetLockProbes seam must be exported');
|
||||
});
|
||||
|
||||
test('live holder is NOT stolen even when aged past the stale threshold (waiter budgets out)', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
const livePid = 4242;
|
||||
fs.writeFileSync(lockPath, String(livePid));
|
||||
|
||||
// Holder pid reads as ALIVE via the injected probe (deterministic, no real pid).
|
||||
stateMod._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
// Drive the clock so the lock is aged WELL past the 10 000 ms stale threshold
|
||||
// (stale < age) but the waiter only ever budgets out at maxWaitMs (30 000 ms).
|
||||
// sleep advances time; once the 30 000 ms budget is exhausted it must throw,
|
||||
// and it must NOT have unlinked the live holder's lock.
|
||||
const clock = makeFakeClock(60000); // age = now - mtime ≫ 10 000 ms
|
||||
assert.throws(
|
||||
() => acquireStateLock(statePath, clock),
|
||||
/acquireStateLock.*exceeded.*30000ms budget/,
|
||||
'a verified-live holder must never be stolen within the wait budget — waiter must time out instead'
|
||||
);
|
||||
|
||||
// The live holder's lock body must be intact (never unlinked + re-created).
|
||||
assert.ok(fs.existsSync(lockPath), 'live holder lock must still exist (not stolen)');
|
||||
assert.strictEqual(fs.readFileSync(lockPath, 'utf-8'), String(livePid), 'live holder lock body must be unchanged');
|
||||
|
||||
fs.unlinkSync(lockPath);
|
||||
});
|
||||
|
||||
test('dead holder is stolen promptly without waiting out the full budget', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
const deadPid = 777;
|
||||
fs.writeFileSync(lockPath, String(deadPid));
|
||||
|
||||
// Holder pid reads as DEAD via the injected probe → eligible for immediate steal.
|
||||
stateMod._setLockProbes({ isPidAlive: () => false });
|
||||
|
||||
// Fresh, NON-aged lock (mtime ≈ now). Without liveness the old mtime-only gate
|
||||
// would refuse to steal a <10 000 ms lock and force a long wait; with liveness
|
||||
// a dead holder is stolen immediately regardless of age.
|
||||
const clock = makeFakeClock(Date.now());
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
assert.ok(fs.existsSync(acquired), 'dead holder lock must be stolen and re-acquired');
|
||||
assert.strictEqual(
|
||||
clock.sleepCalls.length, 0,
|
||||
'a dead holder must be stolen promptly — no wait/backoff sleeps before acquisition'
|
||||
);
|
||||
releaseStateLock(acquired);
|
||||
});
|
||||
|
||||
test('garbage/legacy lock body → not-verified-live → recoverable under the deadman ceiling, never an infinite block', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
fs.writeFileSync(lockPath, 'not-a-pid\x00garbage'); // unreadable / non-numeric body
|
||||
|
||||
// Probe would say "alive" for ANY pid — proves the steal does not depend on a
|
||||
// bogus parse succeeding: an unparseable body is treated as not-verified-live.
|
||||
stateMod._setLockProbes({ isPidAlive: () => true });
|
||||
|
||||
// Age the body past the deadman ceiling (above maxWaitMs) so the corrupt lock
|
||||
// is recoverable rather than blocking forever.
|
||||
const clock = makeFakeClock(Date.now() + 120000);
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
assert.ok(fs.existsSync(acquired), 'corrupt/legacy lock must be recoverable (stolen under the deadman ceiling)');
|
||||
releaseStateLock(acquired);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// 1c. Steal-safety windows (PR #1532 review — trek-e)
|
||||
//
|
||||
// The PID-liveness backport (audit M1) dropped two pieces of capability-lock.cts's
|
||||
// race-free steal machinery, reopening the #500/#905/#1230 lost-update family:
|
||||
//
|
||||
// (a) Empty-body create window — acquireStateLock creates the lock with O_EXCL and
|
||||
// writes the pid in a SEPARATE writeSync. A lock observed in that window has an
|
||||
// EMPTY body → _stateHolderVerifiedLive('') is false → the no-floor steal gate
|
||||
// robs it at age ≈ 0, mid-creation. capability-lock never steals a FRESH lock
|
||||
// (age <= LOCK_STALE_MS) regardless of body, which is what protects that window.
|
||||
//
|
||||
// (b) Double-steal — the steal is a bare fs.unlinkSync with no identity re-confirm
|
||||
// between the decision and the unlink. A racer that steals + recreates a fresh
|
||||
// lock in that gap has its replacement deleted by the first stealer's unlink →
|
||||
// two concurrent holders. capability-lock re-confirms (dev,ino) immediately
|
||||
// before an ATOMIC rename-steal so only one racer can win.
|
||||
//
|
||||
// Both are driven deterministically through the lock seams (clock + pid probe +
|
||||
// onLoopIteration + beforeSteal) — no wall-clock, no real concurrency.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock steal-safety windows (PR #1532)', () => {
|
||||
let tmpDir;
|
||||
let statePath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-stealsafety-state-'));
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
statePath = path.join(tmpDir, '.planning', 'STATE.md');
|
||||
fs.writeFileSync(statePath, '# State\n');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
stateMod._resetLockProbes();
|
||||
stateMod._resetStateLockTestHooks();
|
||||
try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('a FRESH empty-body lock (mid-creation) is NOT stolen at age ~0 — acquirer backs off', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
// Simulate the create→pid-write window of a CONCURRENT acquirer: the lockfile
|
||||
// exists (O_EXCL create succeeded) but the pid has not been written yet → empty body.
|
||||
fs.writeFileSync(lockPath, '');
|
||||
const freshTime = new Date();
|
||||
fs.utimesSync(lockPath, freshTime, freshTime); // mtime ≈ now → age ≈ 0 (fresh)
|
||||
|
||||
// The body is empty, so liveness cannot be determined from it — the probe value is
|
||||
// irrelevant. The (buggy) no-floor gate steals it regardless; the fix must wait.
|
||||
stateMod._setLockProbes({ isPidAlive: () => false });
|
||||
|
||||
// After the first encounter, clear the empty lock so the (correctly-waiting) acquirer
|
||||
// can complete instead of budgeting out — keeps the test bounded and the assertion
|
||||
// about the FIRST decision, not the eventual outcome.
|
||||
stateMod._setStateLockTestHooks({
|
||||
onLoopIteration: ({ iteration }) => {
|
||||
if (iteration >= 1) { try { fs.unlinkSync(lockPath); } catch { /* already gone */ } }
|
||||
},
|
||||
});
|
||||
|
||||
const clock = makeFakeClock(freshTime.getTime());
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
|
||||
assert.ok(fs.existsSync(acquired), 'lock must eventually be acquired');
|
||||
assert.ok(
|
||||
clock.sleepCalls.length >= 1,
|
||||
'a fresh empty-body lock is mid-creation and must NOT be stolen at age ~0 — ' +
|
||||
'the acquirer must back off (sleep) at least once, not unlink + steal immediately'
|
||||
);
|
||||
releaseStateLock(acquired);
|
||||
});
|
||||
|
||||
test('a dead holder whose lock is recreated by a racer mid-steal is NOT double-stolen (identity re-confirm)', () => {
|
||||
const lockPath = statePath + '.lock';
|
||||
const deadPid = 4040;
|
||||
const livePid = 5050;
|
||||
// Decision-time holder: a DEAD pid → eligible for steal.
|
||||
fs.writeFileSync(lockPath, String(deadPid));
|
||||
const t = new Date();
|
||||
fs.utimesSync(lockPath, t, t);
|
||||
|
||||
stateMod._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
// Inject a concurrent waiter that, in the gap between our steal-DECISION and our
|
||||
// steal, already stole + recreated a FRESH lock owned by a LIVE pid. A correct
|
||||
// (identity-re-confirming) acquirer must notice the lock instance changed and must
|
||||
// NOT delete the racer's live replacement.
|
||||
let injected = false;
|
||||
stateMod._setStateLockTestHooks({
|
||||
beforeSteal: () => {
|
||||
if (injected) return;
|
||||
injected = true;
|
||||
try { fs.unlinkSync(lockPath); } catch { /* ok */ }
|
||||
fs.writeFileSync(lockPath, String(livePid)); // different identity + live holder
|
||||
const f = new Date();
|
||||
fs.utimesSync(lockPath, f, f);
|
||||
},
|
||||
});
|
||||
|
||||
const clock = makeFakeClock(t.getTime());
|
||||
// The racer's replacement is held by a LIVE pid → the acquirer must wait on it and
|
||||
// budget out rather than stealing it. (A double-steal would instead delete it and
|
||||
// succeed.)
|
||||
assert.throws(
|
||||
() => acquireStateLock(statePath, clock),
|
||||
(err) => err && err.lockBudgetExceeded === true,
|
||||
'acquirer must not double-steal the racer\'s live replacement — it must wait + budget out'
|
||||
);
|
||||
assert.strictEqual(
|
||||
fs.readFileSync(lockPath, 'utf-8'), String(livePid),
|
||||
'the racer\'s freshly-recreated live lock must survive — never deleted by a stale-decision unlink'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// 1b. Regression #1217 — acquireStateLock ENOENT (recoverable errno) busy-spin
|
||||
//
|
||||
@@ -377,11 +593,12 @@ describe('acquireStateLock boundary coverage — recoverable-errno budget (#1217
|
||||
// before continuing, so they throw within maxWaitMs.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () => {
|
||||
describe('acquireStateLock statSync/steal spin paths bounded (#1217)', () => {
|
||||
let tmpDir;
|
||||
let statePath;
|
||||
let origStatSync;
|
||||
let origUnlinkSync;
|
||||
let origRenameSync;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-clock-spin-'));
|
||||
@@ -390,11 +607,17 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () =
|
||||
fs.writeFileSync(statePath, '# State\n');
|
||||
origStatSync = fs.statSync;
|
||||
origUnlinkSync = fs.unlinkSync;
|
||||
origRenameSync = fs.renameSync;
|
||||
// Force the recorded holder (pid 99999) DEAD so the steal path is exercised
|
||||
// deterministically — these tests probe the steal's bounded-backoff, not liveness.
|
||||
stateMod._setLockProbes({ isPidAlive: () => false });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
fs.statSync = origStatSync;
|
||||
fs.unlinkSync = origUnlinkSync;
|
||||
fs.renameSync = origRenameSync;
|
||||
stateMod._resetLockProbes();
|
||||
try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
@@ -434,29 +657,26 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () =
|
||||
try { origUnlinkSync(lockPath); } catch { /* ok */ }
|
||||
});
|
||||
|
||||
test('persistent unlinkSync failure in stale-lock path throws budget-exceeded (not busy-spin)', () => {
|
||||
// Set up an EEXIST condition with a STALE lock (mtime well in the past)
|
||||
test('persistent renameSync failure in steal path throws budget-exceeded (not busy-spin)', () => {
|
||||
// Set up an EEXIST condition with a steal-eligible DEAD holder (pid 99999 — not us,
|
||||
// not alive). The steal is an ATOMIC rename (PR #1532); a persistent rename failure
|
||||
// (e.g. EPERM — file locked by an AV scanner) must back off + budget out, not spin.
|
||||
const lockPath = statePath + '.lock';
|
||||
fs.writeFileSync(lockPath, '99999');
|
||||
// Back-date mtime by 15 000 ms so the stale-threshold (10 000 ms) is exceeded
|
||||
const staleMs = 15000;
|
||||
const staledTime = new Date(Date.now() - staleMs);
|
||||
fs.utimesSync(lockPath, staledTime, staledTime);
|
||||
|
||||
// Make unlinkSync always fail (e.g. EPERM — file locked by AV scanner)
|
||||
const unlinkErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' });
|
||||
fs.unlinkSync = (p) => {
|
||||
if (p === lockPath) throw unlinkErr;
|
||||
return origUnlinkSync(p);
|
||||
// Make renameSync always fail for the steal of our lock path.
|
||||
const renameErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' });
|
||||
fs.renameSync = (from, to) => {
|
||||
if (from === lockPath) throw renameErr;
|
||||
return origRenameSync(from, to);
|
||||
};
|
||||
|
||||
// Clock where now() returns current real time so the stale check fires,
|
||||
// Clock where now() returns current real time so the steal branch fires,
|
||||
// but sleep advances a fixed 1000ms per call so budget is hit deterministically.
|
||||
const realNow = Date.now();
|
||||
let _elapsed = 0;
|
||||
const sleepCalls = [];
|
||||
const clock = {
|
||||
// Return a time far past the stale threshold so the stale branch is taken
|
||||
now() { return realNow + _elapsed; },
|
||||
sleep(ms) { sleepCalls.push(ms); _elapsed += 1000; },
|
||||
};
|
||||
@@ -464,34 +684,31 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () =
|
||||
assert.throws(
|
||||
() => acquireStateLock(statePath, clock),
|
||||
/acquireStateLock.*exceeded.*30000ms budget/,
|
||||
'persistent unlinkSync failure in stale-lock path must throw budget-exceeded, not spin forever'
|
||||
'persistent renameSync failure in steal path must throw budget-exceeded, not spin forever'
|
||||
);
|
||||
|
||||
assert.ok(sleepCalls.length >= 1, `sleep must have been called at least once (got ${sleepCalls.length}); zero means busy-spin`);
|
||||
assert.ok(_elapsed >= 30000, `elapsed must reach 30 000 ms budget (got ${_elapsed}ms)`);
|
||||
|
||||
// Restore unlinkSync for cleanup
|
||||
fs.unlinkSync = origUnlinkSync;
|
||||
// Restore renameSync for cleanup
|
||||
fs.renameSync = origRenameSync;
|
||||
try { origUnlinkSync(lockPath); } catch { /* ok */ }
|
||||
});
|
||||
|
||||
test('persistent unlinkSync failure error message names stale-lock-removal cause, not statSync (#1217 diagnostic)', () => {
|
||||
// Regression guard for the misleading-error-context bug: when unlinkSync
|
||||
// fails on the stale-lock path and checkBudgetAndSleep throws at the budget
|
||||
// boundary, the outer statSync catch must NOT re-wrap it with
|
||||
// "statSync failed after EEXIST". The thrown error must contain the original
|
||||
// context "stale lock removal failed" so operators can identify the real cause.
|
||||
test('persistent renameSync failure error message names steal cause, not statSync (#1217 diagnostic)', () => {
|
||||
// Regression guard for the misleading-error-context bug: when the steal's renameSync
|
||||
// fails and checkBudgetAndSleep throws at the budget boundary, the outer statSync
|
||||
// catch must NOT re-wrap it with "statSync failed after EEXIST". The thrown error
|
||||
// must name the real cause ("stale lock steal lost to racer") so operators can
|
||||
// identify it.
|
||||
const lockPath = statePath + '.lock';
|
||||
fs.writeFileSync(lockPath, '99999');
|
||||
const staleMs = 15000;
|
||||
const staledTime = new Date(Date.now() - staleMs);
|
||||
fs.utimesSync(lockPath, staledTime, staledTime);
|
||||
|
||||
// unlinkSync always fails — the budget will be exhausted on the first sleep.
|
||||
const unlinkErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' });
|
||||
fs.unlinkSync = (p) => {
|
||||
if (p === lockPath) throw unlinkErr;
|
||||
return origUnlinkSync(p);
|
||||
// renameSync always fails — the budget will be exhausted on the first sleep.
|
||||
const renameErr = Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' });
|
||||
fs.renameSync = (from, to) => {
|
||||
if (from === lockPath) throw renameErr;
|
||||
return origRenameSync(from, to);
|
||||
};
|
||||
|
||||
const realNow = Date.now();
|
||||
@@ -508,17 +725,17 @@ describe('acquireStateLock statSync/unlinkSync spin paths bounded (#1217)', () =
|
||||
thrownErr = e;
|
||||
}
|
||||
|
||||
assert.ok(thrownErr, 'must throw when unlinkSync persistently fails and budget is exhausted');
|
||||
assert.ok(thrownErr, 'must throw when renameSync persistently fails and budget is exhausted');
|
||||
assert.ok(
|
||||
/stale lock removal failed/.test(thrownErr.message),
|
||||
`error message must contain "stale lock removal failed" (got: ${thrownErr.message})`
|
||||
/stale lock steal lost to racer/.test(thrownErr.message),
|
||||
`error message must contain "stale lock steal lost to racer" (got: ${thrownErr.message})`
|
||||
);
|
||||
assert.ok(
|
||||
!/statSync failed after EEXIST/.test(thrownErr.message),
|
||||
`error message must NOT contain "statSync failed after EEXIST" (the misleading re-wrap) (got: ${thrownErr.message})`
|
||||
);
|
||||
|
||||
fs.unlinkSync = origUnlinkSync;
|
||||
fs.renameSync = origRenameSync;
|
||||
try { origUnlinkSync(lockPath); } catch { /* ok */ }
|
||||
});
|
||||
|
||||
@@ -649,58 +866,38 @@ describe('withPlanningLock clock seam', () => {
|
||||
assert.ok(!fs.existsSync(path.join(tmpDir, '.planning', '.lock')), 'lock must be released even when fn() throws');
|
||||
});
|
||||
|
||||
test('timeout fires when clock exceeds lockTimeout (10 000 ms)', () => {
|
||||
test('timeout fires (sleep seam exercised) when a LIVE holder is contended past lockTimeout', () => {
|
||||
// Audit M1 rewrite: the prior version asserted the now-REMOVED force-steal
|
||||
// fallback (timeout → unconditional unlink + re-acquire). That fallback robbed
|
||||
// live writers; the fix replaces it with a clear timeout throw. This test now
|
||||
// pins the new contract: a verified-LIVE holder held past lockTimeout makes the
|
||||
// waiter exercise the clock.sleep seam and then throw — never force-stolen.
|
||||
const lockPath = path.join(tmpDir, '.planning', '.lock');
|
||||
fs.writeFileSync(lockPath, String(process.pid)); // simulate held lock
|
||||
const livePid = 9191;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({ pid: livePid, cwd: tmpDir, acquired: new Date().toISOString() }));
|
||||
|
||||
// Holder reads as ALIVE via the injected probe → waited on, never stolen.
|
||||
require('../gsd-core/bin/lib/planning-workspace.cjs')._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
// Clock that advances past lockTimeout on every sleep call so the while
|
||||
// condition trips immediately after the first retry.
|
||||
let nowValue = 0;
|
||||
|
||||
// withPlanningLock exits the while loop (timeout), deletes the lock, then
|
||||
// calls runWithHeldLock() which tries writeFileSync with { flag: 'wx' }.
|
||||
// Since our lock file is still there (we placed it), runWithHeldLock throws EEXIST.
|
||||
// That exception propagates — so we get an error (either EEXIST or the
|
||||
// function succeeds on the post-timeout acquisition attempt depending on timing).
|
||||
// What we need to assert: the clock.sleep was invoked (timeout path was reached).
|
||||
//
|
||||
// Because withPlanningLock removes the lock file at timeout and re-acquires,
|
||||
// and we placed the lock file ourselves (not via withPlanningLock), the re-acquire
|
||||
// will SUCCEED (wx open on an absent file). So the function returns normally.
|
||||
// Remove our self-placed lock so withPlanningLock can take it over.
|
||||
fs.unlinkSync(lockPath);
|
||||
|
||||
// Now seed the lock AFTER withPlanningLock starts by using a wrapper that
|
||||
// creates the lock file on the first sleep call.
|
||||
let seeded = false;
|
||||
nowValue = 0;
|
||||
const clock2 = {
|
||||
now() { return nowValue; },
|
||||
sleep(ms) {
|
||||
if (!seeded) {
|
||||
seeded = true;
|
||||
// The test: verify withPlanningLock calls clock.sleep when contended
|
||||
// (confirms the seam is wired, not that Atomics.wait is called).
|
||||
}
|
||||
nowValue += ms + 11000;
|
||||
},
|
||||
sleep(ms) { nowValue += ms + 11000; }, // advance past lockTimeout on first sleep
|
||||
};
|
||||
|
||||
// Re-seed the lock (simulating a competing process)
|
||||
fs.writeFileSync(lockPath, '12345'); // non-existent PID; stale check uses mtime
|
||||
|
||||
// Set mtime to now so the stale check (>30s) does NOT fire
|
||||
const now = new Date();
|
||||
fs.utimesSync(lockPath, now, now);
|
||||
|
||||
// With the lock fresh and held, withPlanningLock will enter the retry loop
|
||||
// and call clock2.sleep at least once. After advancing past lockTimeout,
|
||||
// it exits the while loop and tries to recover by unlinking and re-acquiring.
|
||||
const result = withPlanningLock(tmpDir, () => 'recovered', clock2);
|
||||
assert.strictEqual(result, 'recovered', 'must succeed after timeout recovery path');
|
||||
// clock2.sleep was called, confirming the seam was exercised
|
||||
// (the sleep method must have advanced nowValue past lockTimeout)
|
||||
assert.ok(nowValue > 10000, 'clock must have advanced past lockTimeout via sleep calls');
|
||||
try {
|
||||
assert.throws(
|
||||
() => withPlanningLock(tmpDir, () => 'should-not-run', clock2),
|
||||
/exceeded.*10000ms budget/,
|
||||
'a live holder held past lockTimeout must throw a clear timeout error (not force-steal)'
|
||||
);
|
||||
// The sleep seam must have been exercised (timeout path reached).
|
||||
assert.ok(nowValue > 10000, 'clock must have advanced past lockTimeout via the sleep seam');
|
||||
// The live holder's lock must be intact (never unlinked).
|
||||
assert.ok(fs.existsSync(lockPath), 'live holder lock must survive the timeout (not force-stolen)');
|
||||
} finally {
|
||||
require('../gsd-core/bin/lib/planning-workspace.cjs')._resetLockProbes();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
124
tests/m8-writestatemd-scan-after-lock.test.cjs
Normal file
124
tests/m8-writestatemd-scan-after-lock.test.cjs
Normal file
@@ -0,0 +1,124 @@
|
||||
'use strict';
|
||||
// allow-test-rule: architectural-invariant (see #1531)
|
||||
// writeStateMd's "scan happens INSIDE the lock" property is a concurrency invariant.
|
||||
// A single-threaded test cannot observe the difference between scan-before-lock and
|
||||
// scan-after-lock unless something mutates the disk in the window between the two.
|
||||
// The afterAcquire test hook (fired inside writeStateMd right after the lock is
|
||||
// taken) is the deterministic seam that simulates a concurrent writer landing in
|
||||
// exactly that window — the only level at which the TOCTOU is observable.
|
||||
|
||||
/**
|
||||
* M8 — writeStateMd scans the disk (syncStateFrontmatter / PLAN-SUMMARY count)
|
||||
* BEFORE taking the lock, so a concurrent writer that commits a new PLAN/SUMMARY
|
||||
* between our scan and our lock acquisition makes writeStateMd stamp STALE
|
||||
* progress counts (a lost-update of the frontmatter progress block).
|
||||
* readModifyWriteStateMd (the atomic variant) correctly scans INSIDE its lock —
|
||||
* this non-atomic variant was the outlier.
|
||||
*
|
||||
* Deterministic repro (no wall-clock, no threads): the afterAcquire test hook
|
||||
* fires inside writeStateMd immediately after the lock is acquired and adds a
|
||||
* second PLAN file to the phase dir — simulating a concurrent writer who landed
|
||||
* in the scan→lock window. The written frontmatter's progress.total_plans then
|
||||
* reveals whether the scan ran before the hook (stale: 1) or after it (fresh: 2).
|
||||
*
|
||||
* RED (pre-fix): scan runs BEFORE acquire → before the hook → total_plans = 1.
|
||||
* GREEN (post-fix): scan runs AFTER acquire → after the hook → total_plans = 2.
|
||||
*
|
||||
* Recurring closed family this guards: #500 / #905 / #1230 (STATE.md write
|
||||
* corruption). #453 deleted the flaky race tests in favor of seams, so this exact
|
||||
* path was under-tested — the hook restores deterministic coverage.
|
||||
*/
|
||||
|
||||
const { test, describe, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
|
||||
const stateMod = require('../gsd-core/bin/lib/state.cjs');
|
||||
const { writeStateMd } = stateMod;
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Helpers
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
const MINIMAL_STATE_MD = [
|
||||
'# Project State',
|
||||
'',
|
||||
'**Status:** Planning',
|
||||
'**Current Phase:** 01',
|
||||
].join('\n') + '\n';
|
||||
|
||||
/** Parse progress.total_plans out of the STATE.md frontmatter block. */
|
||||
function readTotalPlans(statePath) {
|
||||
const written = fs.readFileSync(statePath, 'utf-8');
|
||||
const fmMatch = written.match(/^---\r?\n([\s\S]*?)\r?\n---/);
|
||||
assert.ok(fmMatch, 'STATE.md must have a frontmatter block after writeStateMd');
|
||||
const m = fmMatch[1].match(/total_plans:\s*(\d+)/);
|
||||
assert.ok(m, 'frontmatter must carry a progress.total_plans line');
|
||||
return parseInt(m[1], 10);
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// M8 — afterAcquire hook proves the scan runs INSIDE the lock
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('M8: writeStateMd scans disk AFTER acquiring the lock (scan-in-lock)', () => {
|
||||
let tmpDir;
|
||||
let statePath;
|
||||
let phaseDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-m8-'));
|
||||
const planningDir = path.join(tmpDir, '.planning');
|
||||
phaseDir = path.join(planningDir, 'phases', '01-init');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
// Start with exactly ONE plan file on disk.
|
||||
fs.writeFileSync(path.join(phaseDir, '01-PLAN.md'), '# Plan 01\n');
|
||||
statePath = path.join(planningDir, 'STATE.md');
|
||||
fs.writeFileSync(statePath, MINIMAL_STATE_MD);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
stateMod._resetStateLockTestHooks();
|
||||
try { fs.unlinkSync(statePath + '.lock'); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('a PLAN added in the post-acquire window is reflected in the written progress count', () => {
|
||||
// The hook simulates a concurrent writer who commits a second PLAN file in the
|
||||
// window between scan and lock. It MUST be observed only if the scan runs after
|
||||
// the lock (and therefore after this hook fires).
|
||||
let fired = 0;
|
||||
stateMod._setStateLockTestHooks({
|
||||
afterAcquire() {
|
||||
fired++;
|
||||
fs.writeFileSync(path.join(phaseDir, '02-PLAN.md'), '# Plan 02\n');
|
||||
},
|
||||
});
|
||||
|
||||
writeStateMd(statePath, MINIMAL_STATE_MD, tmpDir);
|
||||
|
||||
assert.equal(fired, 1, 'afterAcquire hook must fire exactly once inside writeStateMd');
|
||||
|
||||
const totalPlans = readTotalPlans(statePath);
|
||||
// RED pre-fix: scan ran before the hook → counts only 01-PLAN.md → 1.
|
||||
// GREEN post-fix: scan ran after the hook → counts both PLANs → 2.
|
||||
assert.equal(
|
||||
totalPlans, 2,
|
||||
'writeStateMd must scan the disk INSIDE the lock (after the concurrent ' +
|
||||
'writer landed), stamping total_plans=2 — not the stale pre-lock count of 1'
|
||||
);
|
||||
});
|
||||
|
||||
test('single-threaded callers (no hook) are byte-for-behaviour unchanged: count = 1', () => {
|
||||
// Regression guard: with no concurrent writer (hook unset), the count must be
|
||||
// exactly the on-disk truth — the fix must NOT change the uncontended result.
|
||||
writeStateMd(statePath, MINIMAL_STATE_MD, tmpDir);
|
||||
assert.equal(
|
||||
readTotalPlans(statePath), 1,
|
||||
'uncontended writeStateMd must stamp the real on-disk plan count (1)'
|
||||
);
|
||||
});
|
||||
});
|
||||
142
tests/m9-statelock-write-error-orphan.test.cjs
Normal file
142
tests/m9-statelock-write-error-orphan.test.cjs
Normal file
@@ -0,0 +1,142 @@
|
||||
'use strict';
|
||||
// allow-test-rule: architectural-invariant (see #1531)
|
||||
// acquireStateLock's "no orphan empty lock + no fd leak on a recoverable
|
||||
// writeSync/closeSync error" property is a resource-safety invariant of a private
|
||||
// function. A single-threaded test cannot otherwise force the openSync-succeeds-
|
||||
// then-writeSync-throws window. The simulateWriteError seam injects exactly that
|
||||
// one-shot failure; the onLoopIteration seam snapshots the lock file's existence
|
||||
// at the top of the retry that follows — the only level at which the orphan is
|
||||
// observable deterministically (no wall-clock, no threads).
|
||||
|
||||
/**
|
||||
* M9 — acquireStateLock leaks the fd AND strands the just-created empty lock
|
||||
* when writeSync/closeSync throws a RECOVERABLE errno (e.g. EAGAIN) after
|
||||
* openSync(O_CREAT|O_EXCL) already created the lock file. The pre-fix catch did
|
||||
* checkBudgetAndSleep + continue WITHOUT closeSync(fd) or unlinkSync(lockPath),
|
||||
* so every occurrence leaked a descriptor and left a content-less lock behind.
|
||||
*
|
||||
* capability-lock.cts:415-425 already ships the cleanup-before-bail pattern this
|
||||
* mirrors. The fix wraps the writeSync/closeSync in an inner try that
|
||||
* closeSync(fd) (guarded) + unlinkSync(lockPath) (guarded), then re-throws to the
|
||||
* existing outer catch (which keeps classifying recoverable vs fatal errnos — DRY).
|
||||
*
|
||||
* Deterministic repro (no wall-clock, no threads):
|
||||
* - simulateWriteError: 'EAGAIN' injects a ONE-SHOT writeSync failure.
|
||||
* - onLoopIteration snapshots fs.existsSync(lockPath) at the top of each retry.
|
||||
* On the retry iteration that follows the injected error:
|
||||
* RED (pre-fix): the empty lock is still stranded → lockExists === true.
|
||||
* GREEN (post-fix): cleanup unlinked it → lockExists === false.
|
||||
* And in BOTH the call still ultimately succeeds (M1's liveness steal recovers an
|
||||
* orphan) — so the orphan PRESENCE on the retry is the discriminating signal.
|
||||
*
|
||||
* A FATAL errno (e.g. ENOSPC, not in ACQUIRE_LOCK_RETRY_ERRNOS) must still
|
||||
* propagate after cleanup — covered by the fatal-propagation test below.
|
||||
*
|
||||
* Recurring closed family this guards: #500 / #905 / #1230 (STATE.md write
|
||||
* corruption); #453 deleted the flaky race tests so this path was under-tested.
|
||||
*/
|
||||
|
||||
const { test, describe, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const os = require('node:os');
|
||||
|
||||
const { makeFakeClock } = require('./helpers/clock.cjs');
|
||||
const stateMod = require('../gsd-core/bin/lib/state.cjs');
|
||||
const { acquireStateLock, releaseStateLock } = stateMod;
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
|
||||
describe('M9: acquireStateLock cleans up fd + orphan lock on recoverable write error', () => {
|
||||
let tmpDir;
|
||||
let statePath;
|
||||
let lockPath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-m9-'));
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
statePath = path.join(tmpDir, '.planning', 'STATE.md');
|
||||
lockPath = statePath + '.lock';
|
||||
fs.writeFileSync(statePath, '# State\n');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
stateMod._resetStateLockTestHooks();
|
||||
try { fs.unlinkSync(lockPath); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('a one-shot recoverable writeSync error leaves NO stranded empty lock before the retry', () => {
|
||||
const clock = makeFakeClock(0);
|
||||
const lockExistsAtIterationTop = [];
|
||||
|
||||
stateMod._setStateLockTestHooks({
|
||||
simulateWriteError: 'EAGAIN', // one-shot: thrown by the first writeSync
|
||||
onLoopIteration() {
|
||||
lockExistsAtIterationTop.push(fs.existsSync(lockPath));
|
||||
},
|
||||
});
|
||||
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
|
||||
// The call must still ultimately succeed and hold the lock.
|
||||
assert.equal(acquired, lockPath, 'acquireStateLock must succeed after recovering from the write error');
|
||||
assert.ok(fs.existsSync(lockPath), 'a real lock must be held when acquire returns');
|
||||
|
||||
// At least two iterations: the failing attempt, then the recovery retry.
|
||||
assert.ok(
|
||||
lockExistsAtIterationTop.length >= 2,
|
||||
'expected the injected write error to force at least one retry iteration'
|
||||
);
|
||||
|
||||
// The discriminator: on the retry that FOLLOWS the injected write error, no
|
||||
// orphan empty lock may remain. Pre-fix it is still stranded (true); post-fix
|
||||
// the inner cleanup unlinked it (false).
|
||||
assert.equal(
|
||||
lockExistsAtIterationTop[1], false,
|
||||
'the empty lock created by the failed attempt must be unlinked (cleanup-before-retry) — ' +
|
||||
'no orphan lock may be stranded after a recoverable writeSync error (M9 / capability-lock.cts:415-425)'
|
||||
);
|
||||
|
||||
releaseStateLock(acquired);
|
||||
assert.ok(!fs.existsSync(lockPath), 'lock removed after release');
|
||||
});
|
||||
|
||||
test('the held lock body is a valid pid after recovery (write actually completed on retry)', () => {
|
||||
const clock = makeFakeClock(0);
|
||||
stateMod._setStateLockTestHooks({ simulateWriteError: 'EAGAIN' });
|
||||
|
||||
const acquired = acquireStateLock(statePath, clock);
|
||||
const body = fs.readFileSync(lockPath, 'utf-8').trim();
|
||||
assert.equal(body, String(process.pid), 'recovered lock must carry the real pid (no content-less lock survives)');
|
||||
releaseStateLock(acquired);
|
||||
});
|
||||
|
||||
test('a FATAL (non-recoverable) write error still propagates after cleanup — orphan not masked', () => {
|
||||
const clock = makeFakeClock(0);
|
||||
let iterations = 0;
|
||||
|
||||
stateMod._setStateLockTestHooks({
|
||||
simulateWriteError: 'ENOSPC', // fatal: NOT in ACQUIRE_LOCK_RETRY_ERRNOS
|
||||
onLoopIteration() {
|
||||
// A fatal error must propagate on the FIRST attempt — never retried.
|
||||
iterations++;
|
||||
},
|
||||
});
|
||||
|
||||
assert.throws(
|
||||
() => acquireStateLock(statePath, clock),
|
||||
(err) => err && err.code === 'ENOSPC',
|
||||
'a fatal write errno must propagate (not be masked by cleanup or retried)'
|
||||
);
|
||||
|
||||
assert.equal(iterations, 1, 'a fatal write errno must NOT be retried (single attempt then propagate)');
|
||||
|
||||
// After the throw, the empty lock created by the failed openSync must NOT be
|
||||
// left behind — cleanup runs even on the fatal path before re-throw.
|
||||
assert.ok(
|
||||
!fs.existsSync(lockPath),
|
||||
'fatal write error must still unlink the orphan lock before propagating (no stranded lock)'
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -4,6 +4,9 @@ const fs = require('fs');
|
||||
const os = require('os');
|
||||
const path = require('path');
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
const { makeFakeClock } = require('./helpers/clock.cjs');
|
||||
|
||||
const planningWorkspaceDirect = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
|
||||
const {
|
||||
createPlanningWorkspace,
|
||||
@@ -13,9 +16,7 @@ const {
|
||||
withPlanningLock,
|
||||
getActiveWorkstream,
|
||||
setActiveWorkstream,
|
||||
} = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
|
||||
const planningWorkspaceDirect = require('../gsd-core/bin/lib/planning-workspace.cjs');
|
||||
} = planningWorkspaceDirect;
|
||||
|
||||
describe('planning-workspace: planningDir/planningPaths parity', () => {
|
||||
const cwd = '/fake/repo';
|
||||
@@ -185,3 +186,177 @@ describe('planning-workspace direct: functions expose matching behavior', () =>
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// withPlanningLock PID-liveness staleness + EEXIST safety (audit M1 + M2)
|
||||
//
|
||||
// M1: the prior timeout fallback unconditionally unlinked WHATEVER lock existed —
|
||||
// even a fresh, live holder's — then re-acquired. A legitimate op taking
|
||||
// longer than lockTimeout (10 000 ms) got its lock force-stolen. The fix gates
|
||||
// stealing on a real liveness signal (injected via _setLockProbes): a dead
|
||||
// holder is stolen promptly inside the polite loop; a LIVE holder is waited on
|
||||
// and, on genuine timeout, the waiter throws a clear timeout error rather than
|
||||
// corrupting the live holder's critical section.
|
||||
//
|
||||
// M2: the timeout-fallback re-acquire (acquireLock with { flag: 'wx' }) sat OUTSIDE
|
||||
// any try/catch — if another process re-created the lock between the unlink and
|
||||
// the wx write, a raw EEXIST escaped the helper and crashed the command. The
|
||||
// fix removes the unconditional force-steal so no raw EEXIST can escape.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('withPlanningLock PID-liveness staleness + EEXIST safety (audit M1+M2)', () => {
|
||||
let tmpDir;
|
||||
let lockPath;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-liveness-planning-'));
|
||||
fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true });
|
||||
lockPath = path.join(tmpDir, '.planning', '.lock');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
planningWorkspaceDirect._resetLockProbes();
|
||||
if (typeof planningWorkspaceDirect._resetPlanningLockTestHooks === 'function') {
|
||||
planningWorkspaceDirect._resetPlanningLockTestHooks();
|
||||
}
|
||||
try { fs.unlinkSync(lockPath); } catch { /* ok */ }
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('a dead holder recreated by a racer mid-steal is NOT double-stolen (identity re-confirm — PR #1532)', () => {
|
||||
const deadPid = 4040;
|
||||
const livePid = 5050;
|
||||
// Decision-time holder: a DEAD pid → eligible for steal inside the polite loop.
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: deadPid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
// Inject a concurrent waiter that, in the gap between our steal-DECISION and our
|
||||
// steal, already stole + recreated a FRESH lock owned by a LIVE pid. A correct
|
||||
// (identity-re-confirming) acquirer must notice the instance changed and must NOT
|
||||
// delete the racer's live replacement.
|
||||
let injected = false;
|
||||
planningWorkspaceDirect._setPlanningLockTestHooks({
|
||||
beforeSteal: () => {
|
||||
if (injected) return;
|
||||
injected = true;
|
||||
try { fs.unlinkSync(lockPath); } catch { /* ok */ }
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: livePid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
},
|
||||
});
|
||||
|
||||
let ranCriticalSection = false;
|
||||
const clock = makeFakeClock(0);
|
||||
// The racer's replacement is held by a LIVE pid → the acquirer must wait on it and
|
||||
// budget out, NOT delete it and run the critical section (which a double-steal does).
|
||||
assert.throws(
|
||||
() => withPlanningLock(tmpDir, () => { ranCriticalSection = true; return 'x'; }, clock),
|
||||
(err) => err && err.lockTimeout === true,
|
||||
'acquirer must not double-steal the racer\'s live replacement — it must wait + time out'
|
||||
);
|
||||
assert.strictEqual(ranCriticalSection, false, 'critical section must NOT run — the live replacement was not stolen');
|
||||
assert.ok(fs.existsSync(lockPath), 'the racer\'s live replacement lock must survive');
|
||||
const body = JSON.parse(fs.readFileSync(lockPath, 'utf-8'));
|
||||
assert.strictEqual(body.pid, livePid, 'the racer\'s freshly-recreated live lock body must be intact (never deleted by a stale-decision unlink)');
|
||||
});
|
||||
|
||||
test('exports _setLockProbes / _resetLockProbes seams', () => {
|
||||
assert.ok(typeof planningWorkspaceDirect._setLockProbes === 'function', '_setLockProbes seam must be exported');
|
||||
assert.ok(typeof planningWorkspaceDirect._resetLockProbes === 'function', '_resetLockProbes seam must be exported');
|
||||
});
|
||||
|
||||
test('live holder held past lockTimeout is NOT force-stolen — waiter throws a clear timeout error', () => {
|
||||
const livePid = 5151;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: livePid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
// Holder pid reads as ALIVE → must never be force-stolen.
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
let ranCriticalSection = false;
|
||||
// Fake clock whose sleep advances past lockTimeout (10 000 ms) so the polite
|
||||
// loop budgets out; the live holder must survive and the waiter must throw.
|
||||
const clock = makeFakeClock(0);
|
||||
assert.throws(
|
||||
() => withPlanningLock(tmpDir, () => { ranCriticalSection = true; return 'stolen'; }, clock),
|
||||
/lock/i,
|
||||
'a live holder must never be force-stolen on timeout — the waiter must throw a clear timeout error'
|
||||
);
|
||||
|
||||
assert.strictEqual(ranCriticalSection, false, 'critical section must NOT run against a live holder (no force-steal)');
|
||||
assert.ok(fs.existsSync(lockPath), 'live holder lock must still exist (not unlinked)');
|
||||
const body = JSON.parse(fs.readFileSync(lockPath, 'utf-8'));
|
||||
assert.strictEqual(body.pid, livePid, 'live holder lock body must be unchanged');
|
||||
});
|
||||
|
||||
test('dead holder is stolen promptly inside the polite loop (no full timeout wait)', () => {
|
||||
const deadPid = 888;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: deadPid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
// Holder pid reads as DEAD → eligible for prompt steal inside the loop.
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: () => false });
|
||||
|
||||
const clock = makeFakeClock(0);
|
||||
const result = withPlanningLock(tmpDir, () => 'acquired', clock);
|
||||
assert.strictEqual(result, 'acquired', 'dead holder lock must be stolen and the critical section must run');
|
||||
assert.ok(!fs.existsSync(lockPath), 'lock must be released after the critical section completes');
|
||||
});
|
||||
|
||||
test('M2: no raw EEXIST escapes the helper on the timeout path against a live holder', () => {
|
||||
const livePid = 6262;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: livePid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === livePid });
|
||||
|
||||
const clock = makeFakeClock(0);
|
||||
let caught;
|
||||
try {
|
||||
withPlanningLock(tmpDir, () => 'x', clock);
|
||||
} catch (err) {
|
||||
caught = err;
|
||||
}
|
||||
assert.ok(caught, 'helper must surface a failure rather than silently force-stealing a live lock');
|
||||
assert.notStrictEqual(caught.code, 'EEXIST', 'a raw EEXIST must never escape the lock helper (M2)');
|
||||
});
|
||||
|
||||
test('R4-FIX: false-alive pid-reuse holder aged past the deadman ceiling IS stolen (self-heal)', () => {
|
||||
const reusedPid = 7373;
|
||||
fs.writeFileSync(lockPath, JSON.stringify({
|
||||
pid: reusedPid,
|
||||
cwd: tmpDir,
|
||||
acquired: new Date().toISOString(),
|
||||
}));
|
||||
|
||||
// Probe says the recorded pid is ALIVE — simulating pid-reuse: the original holder
|
||||
// crashed but its pid was recycled by an unrelated live process. The .lock body has
|
||||
// no startTime, so liveness alone cannot distinguish this from a genuine live holder.
|
||||
planningWorkspaceDirect._setLockProbes({ isPidAlive: (pid) => pid === reusedPid });
|
||||
|
||||
// Lock mtime ≈ now (real); seed the fake clock ABOVE the 60 000 ms deadman ceiling so
|
||||
// age = clock.now() - mtimeMs ≫ ceiling → the lock must be recovered despite "alive".
|
||||
// Without the ceiling, withPlanningLock would throw on every call with no self-heal.
|
||||
const clock = makeFakeClock(Date.now() + 120000);
|
||||
const result = withPlanningLock(tmpDir, () => 'self-healed', clock);
|
||||
assert.strictEqual(result, 'self-healed', 'a false-alive lock past the deadman ceiling must be stolen (no infinite block)');
|
||||
assert.ok(!fs.existsSync(lockPath), 'lock must be released after the critical section completes');
|
||||
});
|
||||
});
|
||||
|
||||
111
tests/project-instruction-file-parity.test.cjs
Normal file
111
tests/project-instruction-file-parity.test.cjs
Normal file
@@ -0,0 +1,111 @@
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* Bug #1529 parity / drift guard.
|
||||
*
|
||||
* The runtime → project-instruction-file mapping is shared between two
|
||||
* parallel surfaces:
|
||||
* (A) the Node surface — `getProjectInstructionFile` in runtime-name-policy.cjs,
|
||||
* consumed by profile-output.cjs (the generate-claude-md handler).
|
||||
* (B) the bash surface — `gsd-tools query project-instruction-file --runtime <r>`,
|
||||
* consumed by gsd-core/workflows/new-project.md to set $INSTRUCTION_FILE.
|
||||
*
|
||||
* Per DEFECT.GENERATIVE-FIX, any shared mapping between two surfaces MUST
|
||||
* carry a parity assertion that fails when they diverge. This test is that
|
||||
* guard: it asserts (A) and (B) return the same filename for every runtime,
|
||||
* AND that the new-project.md workflow derives $INSTRUCTION_FILE from the
|
||||
* shared query rather than a hardcoded codex-only branch (the original bug).
|
||||
*
|
||||
* Boundary coverage (per RULESET.TESTS.boundary-coverage): claude (the
|
||||
* kept-as-is case) and an unknown runtime (the AGENTS.md default) are both
|
||||
* exercised alongside every runtime family in the mapping table.
|
||||
*/
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const RUNTIME_NAME_POLICY_PATH = path.join(
|
||||
ROOT,
|
||||
'gsd-core',
|
||||
'bin',
|
||||
'lib',
|
||||
'runtime-name-policy.cjs',
|
||||
);
|
||||
const GSD_TOOLS_PATH = path.join(ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs');
|
||||
const NEW_PROJECT_WORKFLOW_PATH = path.join(
|
||||
ROOT,
|
||||
'gsd-core',
|
||||
'workflows',
|
||||
'new-project.md',
|
||||
);
|
||||
|
||||
const { getProjectInstructionFile } = require(RUNTIME_NAME_POLICY_PATH);
|
||||
|
||||
const RUNTIMES = [
|
||||
'claude',
|
||||
'codex',
|
||||
'opencode',
|
||||
'kilo',
|
||||
'kimi',
|
||||
'copilot',
|
||||
'antigravity',
|
||||
'gemini',
|
||||
'future-runtime-xyz',
|
||||
'',
|
||||
];
|
||||
|
||||
function queryInstructionFile(runtime) {
|
||||
const args = [
|
||||
GSD_TOOLS_PATH,
|
||||
'query',
|
||||
'project-instruction-file',
|
||||
'--runtime',
|
||||
runtime,
|
||||
];
|
||||
return execFileSync('node', args, {
|
||||
cwd: ROOT,
|
||||
encoding: 'utf8',
|
||||
env: { ...process.env, GSD_RUNTIME: '' },
|
||||
}).trim();
|
||||
}
|
||||
|
||||
describe('bug #1529: getProjectInstructionFile ↔ gsd-tools query parity', () => {
|
||||
for (const runtime of RUNTIMES) {
|
||||
const label = runtime === '' ? '<empty>' : runtime;
|
||||
test(`Node function and CLI query agree for runtime=${label}`, () => {
|
||||
const fromFunction = getProjectInstructionFile(runtime);
|
||||
const fromQuery = queryInstructionFile(runtime);
|
||||
assert.strictEqual(
|
||||
fromQuery,
|
||||
fromFunction,
|
||||
`gsd-tools query project-instruction-file --runtime ${label} returned "${fromQuery}" but getProjectInstructionFile() returned "${fromFunction}"; the two surfaces drifted.`,
|
||||
);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
describe('bug #1529: new-project.md workflow uses the shared policy query', () => {
|
||||
// allow-test-rule: structural drift guard for #1529 — the workflow's bash block MUST invoke the
|
||||
// shared `gsd_run query project-instruction-file` query rather than a hardcoded
|
||||
// codex-only `if/else` branch; there is no typed IR for "this bash block calls a
|
||||
// specific gsd-tools query instead of a hardcoded mapping".
|
||||
const workflow = fs.readFileSync(NEW_PROJECT_WORKFLOW_PATH, 'utf8');
|
||||
|
||||
test('workflow derives INSTRUCTION_FILE from the shared query', () => {
|
||||
assert.ok(
|
||||
/INSTRUCTION_FILE=\$\(gsd_run query project-instruction-file --runtime "\$RUNTIME"\)/.test(workflow),
|
||||
'new-project.md must derive INSTRUCTION_FILE via `gsd_run query project-instruction-file --runtime "$RUNTIME"` (the shared policy adapter)',
|
||||
);
|
||||
});
|
||||
|
||||
test('workflow no longer hardcodes the codex-only branch', () => {
|
||||
assert.ok(
|
||||
!/if \[ "\$RUNTIME" = "codex" \]; then INSTRUCTION_FILE="AGENTS\.md"; else INSTRUCTION_FILE="\.claude\/CLAUDE\.md"; fi/.test(workflow),
|
||||
'new-project.md must not contain the retired codex-only `if [ "$RUNTIME" = "codex" ]; then INSTRUCTION_FILE="AGENTS.md"; else INSTRUCTION_FILE=".claude/CLAUDE.md"; fi` branch (#1529 regression guard)',
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -9,6 +9,7 @@ const ROOT = path.join(__dirname, '..');
|
||||
const {
|
||||
canonicalizeRuntimeName,
|
||||
resolveRuntimeNameFromCandidates,
|
||||
getProjectInstructionFile,
|
||||
} = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'runtime-name-policy.cjs'));
|
||||
|
||||
describe('runtime-name-policy canonical runtime ids', () => {
|
||||
@@ -71,3 +72,55 @@ describe('runtime-name-policy windsurf alias parity — manifest vs FALLBACK_ALI
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('runtime-name-policy getProjectInstructionFile (#1529)', () => {
|
||||
test('claude maps to .claude/CLAUDE.md (kept-as-is boundary case)', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('claude'), '.claude/CLAUDE.md');
|
||||
});
|
||||
|
||||
test('codex maps to AGENTS.md', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('codex'), 'AGENTS.md');
|
||||
});
|
||||
|
||||
test('opencode maps to AGENTS.md (the #1529 bug surface)', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('opencode'), 'AGENTS.md');
|
||||
});
|
||||
|
||||
test('kilo maps to AGENTS.md', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('kilo'), 'AGENTS.md');
|
||||
});
|
||||
|
||||
test('kimi maps to AGENTS.md', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('kimi'), 'AGENTS.md');
|
||||
});
|
||||
|
||||
test('copilot maps to .github/copilot-instructions.md (GitHub docs read path)', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('copilot'), '.github/copilot-instructions.md');
|
||||
});
|
||||
|
||||
test('gemini maps to GEMINI.md', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('gemini'), 'GEMINI.md');
|
||||
});
|
||||
|
||||
test('antigravity maps to GEMINI.md', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('antigravity'), 'GEMINI.md');
|
||||
});
|
||||
|
||||
test('unknown runtime maps to AGENTS.md (safe cross-agent default, boundary case)', () => {
|
||||
assert.strictEqual(getProjectInstructionFile('future-runtime-xyz'), 'AGENTS.md');
|
||||
assert.strictEqual(getProjectInstructionFile(''), 'AGENTS.md');
|
||||
assert.strictEqual(getProjectInstructionFile(null), 'AGENTS.md');
|
||||
assert.strictEqual(getProjectInstructionFile(undefined), 'AGENTS.md');
|
||||
});
|
||||
|
||||
test('aliases normalize via canonicalizeRuntimeName before mapping', () => {
|
||||
// codex-cli is an alias for codex; it must resolve to the codex mapping.
|
||||
assert.strictEqual(getProjectInstructionFile('codex-cli'), 'AGENTS.md');
|
||||
// opencode-cli is an alias for opencode.
|
||||
assert.strictEqual(getProjectInstructionFile('opencode-cli'), 'AGENTS.md');
|
||||
// gemini-cli is an alias for gemini.
|
||||
assert.strictEqual(getProjectInstructionFile('gemini-cli'), 'GEMINI.md');
|
||||
// github-copilot is an alias for copilot.
|
||||
assert.strictEqual(getProjectInstructionFile('github-copilot'), '.github/copilot-instructions.md');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -45,7 +45,7 @@
|
||||
"milestone-summary.md": 11774,
|
||||
"mvp-phase.md": 13582,
|
||||
"new-milestone.md": 32422,
|
||||
"new-project.md": 61802,
|
||||
"new-project.md": 62324,
|
||||
"new-workspace.md": 11254,
|
||||
"next.md": 20094,
|
||||
"node-repair.md": 4173,
|
||||
|
||||
Reference in New Issue
Block a user