* refactor(#3071): normalize execGit's call and result shape, unify ExecGitFn ExecGitFn was declared four times. Three were hand-copies of one signature and two of those were wrong: they typed exitCode as number|null when _spawnResult returns `result.status ?? 1` and can never yield null, weakened signal from NodeJS.Signals to string, and widened error from Error to unknown. Only verification.cts got it right, via `typeof execGit`. The root cause was a missing export: SpawnResultOutput was declared without `export`, so no other module could name the return type of execGit. Three authors independently hand-copied it instead. Exported now. Normalizing the type alone would have left the pressure that caused the divergence, so the function is normalized on both sides. It now ACCEPTS every call its consumers make — worktree-safety's declaration could not express an env-carrying call at all — and RETURNS every result code they need: timedOut moves into _spawnResult, so execGit, execNpm and execTool all carry it and the one extension that justified a separate type disappears. All four sites are now `typeof execGit` with nothing left to restate. timedOut reuses the existing isSpawnTimeout predicate introduced by #3050 rather than re-deriving it. That predicate checks error.code === 'ETIMEDOUT' only; the signal === 'SIGTERM' conjunct was deliberately dropped there because Windows does not reliably report SIGTERM and requiring it risks a false negative. There is no false-positive risk, and a test proves it: an externally-delivered SIGTERM leaves error null, so it is still not reported as a timeout. No dead null-checks surfaced. Every exitCode comparison in the two affected modules is === 0, !== 0 or === 128 — never a null guard — so the nullable declaration had never been written against. Closes #3071 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3056): add the in-process fault-injection adapter Adds tests/helpers/faulty-deps.cjs — makeFaultyGit() and withFaultyFs() — so a module's error branch can be driven deterministically and its degraded verdict asserted, instead of a counter-test that only proves the call did not throw. makeFaultyGit returns a value structurally assignable to `typeof execGit`, so one stub satisfies all four seams that the #3071 normalization collapsed into that single shape. A parity test drives the same stub through a real injectable entry point in each of worktree-safety, git-base-branch, worktree-base-ref and verification; it fails the moment any of them re-grows its own shape. Faults are scoped rather than global — by argv predicate and by call ordinal — because a fault adapter that faults everything looks like it works and proves nothing, and because verification.cts's two-call error handling needs to fault the second call only. Invocations are recorded so a test can assert an exact call count. The timeout fault carries error.code === 'ETIMEDOUT', and a test asserts the real isSpawnTimeout predicate matches it, so the fixture cannot drift from the production definition of a timeout. A companion test asserts an externally delivered SIGTERM with a null error is still NOT reported as a timeout — the false-positive guard for #3050's dropped conjunct. withFaultyFs restores in a finally so a throwing body still restores, patches only the named methods, and nests without clobbering an outer saved original. It never uses chmod: that no-ops under root, so the test would pass with zero coverage in root Docker/CI. The adapter is in-process via deps only. The Phase 1 process seam is documented as deliberately not a fault-injection surface — it cannot distinguish an injected timeout from a genuine bench OOM and would retry it. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3071): add changeset fragment for the execGit normalization Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3071): route the last two timeout checks through the shared predicate An isolated review found this branch had normalised the timeout verdict but left two callers still hand-rolling the fragile version of it. check-command-router's runBoundedShell computed `timedOut: r.signal === 'SIGTERM'` while the correctly-derived `r.timedOut` sat on the same result object. graphify's execGraphify branched on `result.signal === 'SIGTERM'`, with a comment asserting the very premise isSpawnTimeout exists to reject. Both fail in both directions. On Windows a genuine timeout is not reliably reported as SIGTERM, so the guard silently fails to fire — the false negative #3050 was raised for. And an externally-delivered SIGTERM is not a timeout at all, so the check also fires when it should not; isSpawnTimeout avoids that because `error` is null in that case and it keys on error.code. Both now read the derived `timedOut`, and graphify's comment states the actual rule instead of the fragile assumption. Also replaces a vacuous test: "execGitDefault now accepts env" never called execGitDefault (it is unexported), called execGit — whose signature already accepted env before this branch — and asserted only that exitCode was a number, which would pass whether or not the change under test existed. It now proves env reaches the child by asserting `git var GIT_EDITOR` returns the injected sentinel. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3071): make the graphify timeout fixture faithful to a real timeout The remote matrix failed on both Linux lanes: graphify's "returns exitCode 124 on timeout" got 1 instead of 124. The fixture was wrong, not the production change. It stubbed spawnSync as { status: null, signal: 'SIGTERM', error: undefined }. That is not a timeout. A real spawnSync timeout also sets error.code 'ETIMEDOUT'; a SIGTERM with no error is an externally delivered signal — a kill. The old `result.signal === 'SIGTERM'` check accepted it as a timeout, which is the false positive the shared predicate exists to reject, so this test was locking that bug in rather than guarding against it. The fixture now carries a real ETIMEDOUT error and all three original assertions pass unchanged. A counter-test is added alongside it: an externally delivered SIGTERM with no error must NOT be reported as a timeout. That is the assertion whose absence let the false positive live. Swept every SIGTERM/SIGKILL stub under tests/ for the same unfaithful shape. No other instance: the worktree-safety, worktree-base-ref and commit-staging fixtures already set ETIMEDOUT, and the remaining hits are either deliberate external-kill tests or feed code that never consults timedOut. Two sites keep their own signal check deliberately and are NOT changed: capability-source.cts:1301,1386 fail closed on ANY abnormal termination, which is correct — reading timedOut there would stop it failing closed on a kill and let it parse stdout from a killed process. Their reason strings, and check-latest-version.cjs:115, label any signal as "timed out", which is imprecise wording over a correct verdict, not a silent failure. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3071): backfill changeset pr number to 3077 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
323 lines
14 KiB
JavaScript
323 lines
14 KiB
JavaScript
'use strict';
|
|
|
|
/**
|
|
* Phase 2 test matrix for issue #3056 (fault-injection adapter) and #3071
|
|
* (execGit normalization, folded in). See
|
|
* .gsd/phase/test-3056-fault-injection-adapter/50-test-matrix.md.
|
|
*
|
|
* Section D (row 24 — injection through the process seam is unsupported) has
|
|
* no runtime assertion: it is documented in tests/helpers/faulty-deps.cjs's
|
|
* module JSDoc and in CONTRIBUTING.md, per the matrix's own note that this
|
|
* row is "asserted by review + absence, not a runtime test." Likewise row 23
|
|
* (no chmod anywhere in the helper) is asserted by review of
|
|
* tests/helpers/faulty-deps.cjs, not by a runtime test.
|
|
*/
|
|
|
|
const { describe, test, mock } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const path = require('node:path');
|
|
const childProcess = require('node:child_process');
|
|
|
|
const {
|
|
execGit,
|
|
execNpm,
|
|
execTool,
|
|
isSpawnTimeout,
|
|
} = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'shell-command-projection.cjs'));
|
|
|
|
const worktreeSafety = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'worktree-safety.cjs'));
|
|
const { trySymbolicRef } = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'git-base-branch.cjs'));
|
|
const { evaluateWorktreeBaseDegrade } = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'worktree-base-ref.cjs'));
|
|
const { defaultPhaseCleanCommitTimesMs } = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'verification.cjs'));
|
|
|
|
const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs');
|
|
const { makeFaultyGit, withFaultyFs } = require('./helpers/faulty-deps.cjs');
|
|
|
|
// ─── A. execGit normalization (#3071) ──────────────────────────────────────
|
|
|
|
describe('A. execGit normalization (#3071)', () => {
|
|
let tmpDir;
|
|
|
|
test('1. execGit result carries timedOut', (t) => {
|
|
tmpDir = createTempGitProject();
|
|
t.after(() => cleanup(tmpDir));
|
|
const result = execGit(['status', '--porcelain'], { cwd: tmpDir });
|
|
assert.strictEqual(result.timedOut, false);
|
|
assert.strictEqual(typeof result.exitCode, 'number');
|
|
});
|
|
|
|
test('2. every exec* result carries timedOut', () => {
|
|
const npmResult = execNpm(['--version']);
|
|
const toolResult = execTool(process.execPath, ['--version']);
|
|
assert.strictEqual(typeof npmResult.timedOut, 'boolean');
|
|
assert.strictEqual(npmResult.timedOut, false);
|
|
assert.strictEqual(typeof toolResult.timedOut, 'boolean');
|
|
assert.strictEqual(toolResult.timedOut, false);
|
|
});
|
|
|
|
test('3. ENOENT path still sets timedOut:false', () => {
|
|
// Reached via a real execTool call to a nonexistent binary — the early
|
|
// ENOENT return in _spawnResult (shell-command-projection.cts) must not
|
|
// omit the field the rest of the shape now always carries.
|
|
const result = execTool('definitely-not-a-real-program-fault-3056', []);
|
|
assert.strictEqual(result.exitCode, 127);
|
|
assert.strictEqual(result.timedOut, false);
|
|
});
|
|
|
|
test('4. a timed-out git reports timedOut', (t) => {
|
|
tmpDir = createTempGitProject();
|
|
t.after(() => cleanup(tmpDir));
|
|
// timeout:1 is real (no mocking) — process creation alone cannot
|
|
// complete inside 1ms, so the kill path is deterministic, matching the
|
|
// existing "wall-clock timeout" pattern used for dispatchGsdCommand in
|
|
// shell-command-projection-dispatch.test.cjs.
|
|
const result = execGit(['status', '--porcelain'], { cwd: tmpDir, timeout: 1 });
|
|
assert.strictEqual(result.timedOut, true);
|
|
assert.strictEqual(result.error && result.error.code, 'ETIMEDOUT');
|
|
});
|
|
|
|
test('5. an external SIGTERM is not reported as a timeout', (t) => {
|
|
// Direct predicate check: a SIGTERM with no accompanying ETIMEDOUT error
|
|
// (the shape an externally-delivered kill produces) must not trip
|
|
// isSpawnTimeout — proves dropping the SIGTERM conjunct (#3050) did not
|
|
// widen the predicate into a false positive.
|
|
assert.strictEqual(isSpawnTimeout({ error: null }), false);
|
|
|
|
// End-to-end: mock spawnSync to return the externally-killed shape and
|
|
// confirm the real execGit seam reports timedOut:false. Same
|
|
// mock.method(childProcess, 'spawnSync', ...) technique already
|
|
// established in shell-command-projection-dispatch.test.cjs for the
|
|
// Windows-shaped-timeout case, exercising the same non-destructured
|
|
// childProcess import for the opposite direction.
|
|
mock.method(childProcess, 'spawnSync', () => ({
|
|
status: null,
|
|
stdout: '',
|
|
stderr: '',
|
|
signal: 'SIGTERM',
|
|
error: null,
|
|
}));
|
|
t.after(() => mock.restoreAll());
|
|
|
|
const result = execGit(['status', '--porcelain']);
|
|
assert.strictEqual(result.signal, 'SIGTERM');
|
|
assert.strictEqual(result.timedOut, false);
|
|
});
|
|
|
|
test('6. execGit env opt actually reaches the child process', (t) => {
|
|
tmpDir = createTempGitProject();
|
|
t.after(() => cleanup(tmpDir));
|
|
// worktree-safety.cts's execGitDefault is now a thin passthrough to this
|
|
// exact seam (see worktree-safety.cts:38 docstring), and its widened opts
|
|
// type ({cwd, env, timeout} together) is proven at build time by the
|
|
// strict tsc build, not here. This test instead proves the runtime
|
|
// behavior the type describes: `git var GIT_EDITOR` reflects the
|
|
// GIT_EDITOR env var, so a sentinel value passed via opts.env must come
|
|
// back verbatim on stdout — a real proof env reaches the child, not a
|
|
// liveness check that passes regardless of whether env is wired through.
|
|
const result = execGit(['var', 'GIT_EDITOR'], {
|
|
cwd: tmpDir,
|
|
env: { GIT_EDITOR: 'fault-3056-sentinel-editor' },
|
|
timeout: 5000,
|
|
});
|
|
assert.strictEqual(result.exitCode, 0);
|
|
assert.strictEqual(result.stdout, 'fault-3056-sentinel-editor');
|
|
});
|
|
|
|
test('7. exitCode is always a number', (t) => {
|
|
tmpDir = createTempGitProject();
|
|
t.after(() => cleanup(tmpDir));
|
|
const success = execGit(['status', '--porcelain'], { cwd: tmpDir });
|
|
const enoent = execTool('definitely-not-a-real-program-fault-3056', []);
|
|
const timeout = execGit(['status', '--porcelain'], { cwd: tmpDir, timeout: 1 });
|
|
assert.strictEqual(typeof success.exitCode, 'number');
|
|
assert.strictEqual(typeof enoent.exitCode, 'number');
|
|
assert.strictEqual(typeof timeout.exitCode, 'number');
|
|
});
|
|
|
|
test('8. one git stub satisfies every ExecGitFn seam', (t) => {
|
|
tmpDir = createTempDir();
|
|
t.after(() => cleanup(tmpDir));
|
|
|
|
// Single shared stub, deliberately configured with only the default
|
|
// benign passthrough — the point of this row is that ONE value is
|
|
// accepted everywhere, not what any one fault produces.
|
|
const sharedStub = makeFaultyGit();
|
|
|
|
// worktree-safety.cts:33 — via resolveWorktreeContext, the exported
|
|
// entry point that threads deps.execGit.
|
|
const wsResult = worktreeSafety.resolveWorktreeContext(tmpDir, {
|
|
execGit: sharedStub,
|
|
existsSync: () => false,
|
|
});
|
|
assert.strictEqual(typeof wsResult.effectiveRoot, 'string');
|
|
assert.strictEqual(typeof wsResult.mode, 'string');
|
|
assert.strictEqual(typeof wsResult.reason, 'string');
|
|
|
|
// git-base-branch.cts:32 — trySymbolicRef takes execGit directly as its
|
|
// second positional argument (no deps wrapper).
|
|
assert.doesNotThrow(() => trySymbolicRef(tmpDir, sharedStub));
|
|
|
|
// worktree-base-ref.cts:88 — evaluateWorktreeBaseDegrade threads
|
|
// deps.execGit.
|
|
const wbrResult = evaluateWorktreeBaseDegrade({ execGit: sharedStub, cwd: tmpDir });
|
|
assert.strictEqual(typeof wbrResult.shouldDegrade, 'boolean');
|
|
assert.strictEqual(typeof wbrResult.reason, 'string');
|
|
|
|
// verification.cts:226 — defaultPhaseCleanCommitTimesMs takes execGitFn
|
|
// directly as its third positional argument, typed `= typeof execGit`.
|
|
assert.doesNotThrow(() => {
|
|
const map = defaultPhaseCleanCommitTimesMs(tmpDir, ['a.md', 'b.md'], sharedStub);
|
|
assert.ok(map instanceof Map);
|
|
});
|
|
});
|
|
});
|
|
|
|
// ─── B. FaultyGit ───────────────────────────────────────────────────────────
|
|
|
|
describe('B. FaultyGit', () => {
|
|
test('9. FaultyGit timeout trips isSpawnTimeout', () => {
|
|
const faultyGit = makeFaultyGit({ faults: [{ kind: 'timeout' }] });
|
|
const result = faultyGit(['status']);
|
|
assert.strictEqual(result.exitCode, 1);
|
|
assert.strictEqual(result.timedOut, true);
|
|
assert.strictEqual(result.error && result.error.code, 'ETIMEDOUT');
|
|
assert.strictEqual(isSpawnTimeout(result), true);
|
|
});
|
|
|
|
test('10. FaultyGit non-zero exit', () => {
|
|
const faultyGit = makeFaultyGit({ faults: [{ kind: 'exit', exitCode: 3, stderr: 'boom' }] });
|
|
const result = faultyGit(['status']);
|
|
assert.strictEqual(result.exitCode, 3);
|
|
assert.strictEqual(result.stderr, 'boom');
|
|
assert.strictEqual(result.timedOut, false);
|
|
});
|
|
|
|
test('11. FaultyGit spawn failure', () => {
|
|
const faultyGit = makeFaultyGit({ faults: [{ kind: 'spawnFail' }] });
|
|
const result = faultyGit(['status']);
|
|
assert.strictEqual(result.exitCode, 127);
|
|
assert.strictEqual(result.error && result.error.code, 'ENOENT');
|
|
assert.strictEqual(result.timedOut, false);
|
|
});
|
|
|
|
test('12. FaultyGit faults only what it was told to', () => {
|
|
const faultyGit = makeFaultyGit({
|
|
faults: [{ kind: 'timeout', when: ['worktree', 'list'] }],
|
|
});
|
|
const unmatched = faultyGit(['status', '--porcelain']);
|
|
assert.strictEqual(unmatched.timedOut, false);
|
|
assert.strictEqual(unmatched.exitCode, 0);
|
|
});
|
|
|
|
test('13. FaultyGit scopes a fault to one argv', () => {
|
|
const faultyGit = makeFaultyGit({
|
|
faults: [{ kind: 'exit', exitCode: 9, when: ['worktree', 'list'] }],
|
|
});
|
|
const matched = faultyGit(['worktree', 'list', '--porcelain']);
|
|
const unmatched = faultyGit(['status', '--porcelain']);
|
|
assert.strictEqual(matched.exitCode, 9);
|
|
assert.strictEqual(unmatched.exitCode, 0);
|
|
});
|
|
|
|
test('14. FaultyGit faults the Nth call only', () => {
|
|
const faultyGit = makeFaultyGit({
|
|
faults: [{ kind: 'exit', exitCode: 5, onCall: 2 }],
|
|
});
|
|
const first = faultyGit(['rev-parse', 'HEAD']);
|
|
const second = faultyGit(['rev-parse', 'HEAD']);
|
|
const third = faultyGit(['rev-parse', 'HEAD']);
|
|
assert.strictEqual(first.exitCode, 0);
|
|
assert.strictEqual(second.exitCode, 5);
|
|
assert.strictEqual(third.exitCode, 0);
|
|
});
|
|
|
|
test('15. FaultyGit records its calls', () => {
|
|
const faultyGit = makeFaultyGit();
|
|
faultyGit(['rev-parse', 'HEAD'], { cwd: '/a' });
|
|
faultyGit(['status'], { cwd: '/b' });
|
|
assert.strictEqual(faultyGit.calls.length, 2);
|
|
assert.deepStrictEqual(faultyGit.calls[0].args, ['rev-parse', 'HEAD']);
|
|
assert.strictEqual(faultyGit.calls[0].opts.cwd, '/a');
|
|
assert.deepStrictEqual(faultyGit.calls[1].args, ['status']);
|
|
assert.strictEqual(faultyGit.calls[1].opts.cwd, '/b');
|
|
});
|
|
|
|
test('16. FaultyGit result is a valid execGit result', () => {
|
|
const faultyGit = makeFaultyGit({ faults: [{ kind: 'timeout' }] });
|
|
const result = faultyGit(['status']);
|
|
for (const key of ['exitCode', 'stdout', 'stderr', 'signal', 'error', 'timedOut']) {
|
|
assert.ok(Object.prototype.hasOwnProperty.call(result, key), `missing ${key}`);
|
|
}
|
|
assert.strictEqual(typeof result.exitCode, 'number');
|
|
assert.strictEqual(typeof result.stdout, 'string');
|
|
assert.strictEqual(typeof result.stderr, 'string');
|
|
assert.strictEqual(typeof result.timedOut, 'boolean');
|
|
});
|
|
});
|
|
|
|
// ─── C. FaultyFs ────────────────────────────────────────────────────────────
|
|
|
|
describe('C. FaultyFs', () => {
|
|
test('17. FaultyFs read throws', () => {
|
|
const fs = require('node:fs');
|
|
const injected = new Error('injected read failure');
|
|
withFaultyFs({ readFileSync: () => { throw injected; } }, () => {
|
|
assert.throws(() => fs.readFileSync('/whatever'), /injected read failure/);
|
|
});
|
|
});
|
|
|
|
test('18. FaultyFs write throws', () => {
|
|
const fs = require('node:fs');
|
|
const injected = new Error('injected write failure');
|
|
withFaultyFs({ writeFileSync: () => { throw injected; } }, () => {
|
|
assert.throws(() => fs.writeFileSync('/whatever', 'x'), /injected write failure/);
|
|
});
|
|
});
|
|
|
|
test('19. FaultyFs restores on success', () => {
|
|
const fs = require('node:fs');
|
|
const original = fs.readFileSync;
|
|
withFaultyFs({ readFileSync: () => { throw new Error('injected'); } }, () => {
|
|
assert.notStrictEqual(fs.readFileSync, original);
|
|
});
|
|
assert.strictEqual(fs.readFileSync, original);
|
|
});
|
|
|
|
test('20. FaultyFs restores when the body throws', () => {
|
|
const fs = require('node:fs');
|
|
const original = fs.readFileSync;
|
|
assert.throws(() => {
|
|
withFaultyFs({ readFileSync: () => { throw new Error('injected'); } }, () => {
|
|
throw new Error('body exploded');
|
|
});
|
|
}, /body exploded/);
|
|
assert.strictEqual(fs.readFileSync, original);
|
|
});
|
|
|
|
test('21. nested FaultyFs restore in order', () => {
|
|
const fs = require('node:fs');
|
|
const original = fs.readFileSync;
|
|
const outerPatch = () => { throw new Error('outer'); };
|
|
const innerPatch = () => { throw new Error('inner'); };
|
|
|
|
withFaultyFs({ readFileSync: outerPatch }, () => {
|
|
assert.strictEqual(fs.readFileSync, outerPatch);
|
|
withFaultyFs({ readFileSync: innerPatch }, () => {
|
|
assert.strictEqual(fs.readFileSync, innerPatch);
|
|
});
|
|
// Inner restored WITHOUT clobbering the outer's still-active patch.
|
|
assert.strictEqual(fs.readFileSync, outerPatch);
|
|
});
|
|
assert.strictEqual(fs.readFileSync, original);
|
|
});
|
|
|
|
test('22. FaultyFs patches only the named method', () => {
|
|
const fs = require('node:fs');
|
|
const originalWrite = fs.writeFileSync;
|
|
withFaultyFs({ readFileSync: () => { throw new Error('injected'); } }, () => {
|
|
assert.strictEqual(fs.writeFileSync, originalWrite);
|
|
});
|
|
assert.strictEqual(fs.writeFileSync, originalWrite);
|
|
});
|
|
});
|