From c5629bbe7486a44c1d2d0940d60fda295e3370bd Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 18 Sep 2026 03:16:25 -0400 Subject: [PATCH] fix(#4734): degrade worktree isolation when the root has no git repository (#4843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4734): non-git root must degrade worktree isolation (failing first) * fix(#4734): degrade worktree isolation when the root has no git repository * fix(#4734): review fold-ins — 3972 ladder fixture, parity fixture, docs row, message wording * chore(#4734): backfill changeset PR number (4843) --------- Co-authored-by: sim --- .changeset/witty-foxes-sprint.md | 5 + docs/CLI-TOOLS.md | 2 +- hooks/gsd-agent-isolation-guard.js | 24 +++++ src/worktree-base-ref.cts | 114 +++++++++++++-------- tests/cursor-subagent-isolation.test.cjs | 10 +- tests/gsd-agent-isolation-guard.test.cjs | 80 ++++++++++++++- tests/worktree-base-ref.test.cjs | 124 ++++++++++++++++++----- tests/worktree-safety.test.cjs | 14 +-- 8 files changed, 290 insertions(+), 83 deletions(-) create mode 100644 .changeset/witty-foxes-sprint.md diff --git a/.changeset/witty-foxes-sprint.md b/.changeset/witty-foxes-sprint.md new file mode 100644 index 000000000..4bb14bba4 --- /dev/null +++ b/.changeset/witty-foxes-sprint.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4843 +--- +**Agent dispatch no longer blocks on a project root that is not a git repository** — `worktree.base-check` now degrades to sequential when git definitively reports no repository, and the isolation guard's stale-sentinel fallback no longer demands `isolation="worktree"` where no worktree can be created (multi-repo workspace roots). Repos with a real HEAD behave exactly as before. (#4734) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 14af1f993..eb1e4823c 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1301,7 +1301,7 @@ node gsd-tools.cjs worktree set-baseref | `head-matches-fork` | `false` | HEAD and `origin/HEAD` are the same commit | | `head-diverged-from-fork` | `true` | Branch is ahead of or diverged from `origin/HEAD` | | `fork-ref-unknown` | `true` | `origin/HEAD` could not be resolved | -| `no-head` | `false` | Not in a git repo (no `HEAD`) — `git rev-parse HEAD` exited 128 (definitive), or exited 0 with empty stdout | +| `no-head` | `true` for exit 128, `false` for exit 0 with empty stdout | Exit 128 is git's definitive "no resolvable HEAD here" answer — not a git repository, or a repository with no commits; no harness worktree can be created, so the check degrades to sequential (#4734), with a `message` explaining why. Exit 0 with empty stdout is ambiguous (git completed without a definitive answer) and stays non-degrading (`headAbsenceVerified` distinguishes the two: `true` / `false`) | | `head-unresolvable` | `true` | `git rev-parse HEAD` did not return a definitive answer (timed out, `git` missing, or any other non-128 failure) — fails closed rather than being treated as `no-head` | **`worktree set-baseref`** applies a no-clobber write of `worktree.baseRef:"head"` to `.claude/settings.local.json`. If the file already contains an explicit `baseRef` value other than `"head"`, the existing value is preserved and `skipped:"explicit-other"` is returned. Malformed JSON causes an error rather than a silent overwrite. Both fresh installs and upgrades of GSD Core run this automatically when `workflow.use_worktrees` is enabled (the default); the command is also available for manual use — for example, to apply the setting when worktrees were toggled on after installation, or to re-apply it after a settings change. diff --git a/hooks/gsd-agent-isolation-guard.js b/hooks/gsd-agent-isolation-guard.js index 92d4659ff..c6b09f819 100644 --- a/hooks/gsd-agent-isolation-guard.js +++ b/hooks/gsd-agent-isolation-guard.js @@ -337,6 +337,30 @@ function resolveRegistryIsolation(cwd, configPath) { if (!useWorktrees) isolation = 'none'; } + // #4734: a harness worktree can never be created in a directory that is not + // a git repository (or a repository with no commits) — git's definitive + // exit-128 answer on `rev-parse HEAD`. Demanding the isolation flag there + // blocked every flag-less dispatch the moment the sentinel went stale, on a + // root where no worktree can exist. Degrade to 'none' exactly like the + // use_worktrees opt-out above; the classification is owned by + // `classifyGitHead` (worktree-base-ref.cjs) — the same single owner the + // base-check's degrade decision uses — and ambiguous or failed resolutions + // keep the conservative (enforce) default, matching that check's own + // fail-closed treatment of the same classes. + if (isolation === 'harness-worktree') { + try { + ensureRuntimeBuild(); + const { classifyGitHead } = require('../gsd-core/bin/lib/worktree-base-ref.cjs'); + if (classifyGitHead({ cwd }).status === 'definitive-absence') { + isolation = 'none'; + } + } catch { + // Unbuilt or unreadable runtime library — keep the conservative + // (enforce) default rather than silently disabling the guard, same + // posture as the ladder fallback above. + } + } + return { isolation, harnessFlag }; } diff --git a/src/worktree-base-ref.cts b/src/worktree-base-ref.cts index d68f7feeb..e77ab5eb7 100644 --- a/src/worktree-base-ref.cts +++ b/src/worktree-base-ref.cts @@ -106,6 +106,8 @@ function buildMsgBaserefHeadIgnored(headSha: string | null, forkRef: string | nu const MSG_HEAD_UNRESOLVABLE = `⚠ Cannot determine the worktree base (git rev-parse HEAD did not return a definitive answer). Running this phase sequentially on the main working tree to avoid an unverified base mismatch. Note: worktree.baseRef:"head" silences this check only where GSD itself creates the worktree (orchestrator-managed runtimes) — in harness mode it never applied (#48, #3659). Retry; if it persists, check for a stalled filesystem mount or a stale git index lock (.git/index.lock). See #683, #3050.`; +const MSG_NO_GIT_REPOSITORY = `⚠ No worktree base exists here (git resolved no HEAD — the root is not a git repository, or the repository has no commits), so a harness worktree cannot be created. Running this dispatch sequentially on the main working tree instead — no isolation flag is required. See #4734.`; + /** * Returns true when an execGit result indicates the subprocess was killed by * a timeout. A timeout means the command genuinely could not complete — it @@ -377,6 +379,46 @@ export function cmdWorktreeSetBaseRef( return output; } +/** + * One classification of `git rev-parse HEAD`, shared by every surface that + * must tell git's definitive "no repository here" answer apart from ambiguous + * or failed resolutions (#4734 — before this, evaluateWorktreeBaseDegrade was + * the only owner and the isolation guard's fallback simply had no check). + * Exit 128 covers BOTH "not a git repository" and "a repository with no + * commits" (`ambiguous argument 'HEAD'`); stderr text is localized, so the + * exit code is the stable contract — and in neither case can a harness + * worktree be created, which is the only question every consumer asks. + * + * - `present`: exit 0 with a non-empty sha (`headSha` carries it, trimmed). + * - `definitive-absence`: exit 128 — git completed and definitively answered. + * - `ambiguous-absence`: exit 0 with empty stdout — git completed without a + * definitive answer (#3057 B8). + * - `indeterminate`: timeout or any other non-success — never evidence of + * anything; consumers must fail closed (#3050). + */ +export function classifyGitHead(deps?: { + execGit?: ExecGitFn; + cwd?: string; +}): { status: 'present'; headSha: string } | { status: 'definitive-absence' | 'ambiguous-absence' | 'indeterminate'; headSha: null } { + const execGit: ExecGitFn = deps?.execGit ?? execGitSeam; + const cwdOpts = deps?.cwd ? { cwd: deps.cwd } : {}; + const headResult = execGit(['rev-parse', 'HEAD'], cwdOpts); + if (isExecGitTimeout(headResult)) { + return { status: 'indeterminate', headSha: null }; + } + const headStdout = headResult.stdout ? headResult.stdout.trim() : ''; + if (headResult.exitCode === 128) { + return { status: 'definitive-absence', headSha: null }; + } + if (headResult.exitCode === 0 && !headStdout) { + return { status: 'ambiguous-absence', headSha: null }; + } + if (headResult.exitCode !== 0) { + return { status: 'indeterminate', headSha: null }; + } + return { status: 'present', headSha: headStdout }; +} + /** * Evaluates whether the current worktree HEAD has diverged from the fork base * (origin/HEAD) that the Claude Code harness would use when creating a 'fresh' @@ -406,15 +448,14 @@ export function evaluateWorktreeBaseDegrade(deps?: { forkRef: string | null; forkSha: string | null; /** - * Only meaningful when `reason === 'no-head'` (both non-degrade outcomes); - * `null` for every other reason. `true` for exit 128 — git's definitive - * "not a git repository" answer. `false` for exit 0 with empty stdout: git - * completed but did NOT give a confirmed "no HEAD" answer, unlike exit 128 - * — this outcome is left `shouldDegrade:false` unchanged (pinned by an - * existing regression guard; the underlying product question of whether it - * SHOULD degrade is still open, see #3050 review), but a caller can now - * tell the two `'no-head'` causes apart instead of treating them as the - * same verified answer. (#3057 B8) + * Only meaningful when `reason === 'no-head'`; `null` for every other + * reason. Distinguishes the two no-head causes: `true` for exit 128 — + * git's definitive "no resolvable HEAD here" answer, which DEGRADES as of + * #4734 (a worktree can never be created; the maintainer brief on #4734 + * answered the previously-open product question). `false` for exit 0 with + * empty stdout — git completed but did NOT give a confirmed answer; that + * outcome stays `shouldDegrade:false` (pinned by an existing regression + * guard; the product question remains open, #3050/#3057 B8). */ headAbsenceVerified: boolean | null; } { @@ -438,44 +479,33 @@ export function evaluateWorktreeBaseDegrade(deps?: { return { shouldDegrade: false, reason: 'baseref-head', message: null, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: null }; } - // b. Resolve HEAD sha. - const headResult = execGit(['rev-parse', 'HEAD'], cwdOpts); - // A TIMEOUT means the command never completed — it is not evidence of "not a - // git repository" and must fail closed (distinct from the clean-exit-128 - // "no-head" case below, which genuinely completed and reported no HEAD). - if (isExecGitTimeout(headResult)) { + // b. Resolve HEAD sha — through the single classification owner (#4734). + const head = classifyGitHead({ execGit, cwd }); + if (head.status === 'indeterminate') { + // A timeout, git missing (exit 127), or any other non-128 non-success is + // NOT a definitive answer from git — fail closed (#3050). return { shouldDegrade: true, reason: 'head-unresolvable', message: MSG_HEAD_UNRESOLVABLE, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: null }; } - const headStdout = headResult.stdout ? headResult.stdout.trim() : ''; - // exit 128 is git's definitive "not a git repository" answer — it completed - // and genuinely reported no HEAD. Only this specific, confirmed outcome - // stays a benign non-degrade; every other non-success outcome below is - // NOT a definitive answer from git and must fail closed (#3050). - if (headResult.exitCode === 128) { - return { shouldDegrade: false, reason: 'no-head', message: null, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: true }; + if (head.status === 'definitive-absence') { + // Exit 128 is git's definitive "no resolvable HEAD here" answer — not a + // git repository, or a repository with no commits. In either case a + // harness worktree can NEVER be created, so demanding worktree isolation + // blocked every dispatch from e.g. a multi-repo workspace root (#4734). + // This wires the verdict `headAbsenceVerified` (#3057 B8) was added to + // enable; the maintainer brief on #4734 answers the previously-open + // product question: the definitive case degrades. + return { shouldDegrade: true, reason: 'no-head', message: MSG_NO_GIT_REPOSITORY, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: true }; } - // Exit 0 with empty stdout is pinned as benign no-degrade by an existing - // regression guard (tests/worktree-base-ref.test.cjs — "git rev-parse HEAD - // returns empty stdout"). Left unchanged deliberately; flagged in the - // #3050 review for a product-intent call rather than silently flipped. - // Unlike the exit-128 case above, git did NOT give a definitive "no HEAD" - // answer here — `headAbsenceVerified:false` names that gap explicitly - // instead of leaving it folded into an identical-looking 'no-head' reason - // (#3057 B8; the product question of whether this SHOULD degrade is - // unchanged and still open). - if (headResult.exitCode === 0 && !headStdout) { + if (head.status === 'ambiguous-absence') { + // Exit 0 with empty stdout is pinned as benign no-degrade by an existing + // regression guard (tests/worktree-base-ref.test.cjs — "git rev-parse HEAD + // returns empty stdout"). Unlike the exit-128 case above, git did NOT + // give a definitive "no HEAD" answer here — `headAbsenceVerified:false` + // names that gap explicitly (#3057 B8; the product question of whether + // this SHOULD degrade is unchanged and still open). return { shouldDegrade: false, reason: 'no-head', message: null, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: false }; } - if (headResult.exitCode !== 0) { - // Any other non-success outcome (e.g. exit 127 — git missing — or any - // other non-zero, non-128 exit) is not a definitive "not a repo" answer. - // Fail closed instead of silently treating it as benign. - // (`!headStdout` was previously OR'd in here but is unreachable: the - // exitCode===0 && !headStdout case is already handled above, and every - // other branch here has exitCode!==0 already true — #3050 review.) - return { shouldDegrade: true, reason: 'head-unresolvable', message: MSG_HEAD_UNRESOLVABLE, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: null }; - } - const headSha = headStdout; + const headSha = head.headSha; // c. Resolve fork base (what the harness forks 'fresh' worktrees from = origin/HEAD). let forkRef: string | null = null; diff --git a/tests/cursor-subagent-isolation.test.cjs b/tests/cursor-subagent-isolation.test.cjs index da6c01cc0..41e464c08 100644 --- a/tests/cursor-subagent-isolation.test.cjs +++ b/tests/cursor-subagent-isolation.test.cjs @@ -577,9 +577,13 @@ describe('executor-identity parity: hooks/gsd-agent-isolation-guard.js (Claude) let cursorProject; // harness-worktree GSD project, not an isolated worktree before(() => { - claudeProject = createTempDir('gsd-cs-parity-claude-'); - fs.mkdirSync(path.join(claudeProject, '.planning'), { recursive: true }); - fs.writeFileSync(path.join(claudeProject, '.planning', 'config.json'), JSON.stringify({ runtime: 'claude' })); + // Must be a REAL git repo (not a bare mkdir'd directory) — same reason as + // cursorProject below, and load-bearing since #4734: the Claude guard's + // fallback degrades to 'none' on a repository-less root, so a non-git + // fixture would flip the Claude side to allow and the parity probe would + // compare Claude-allowing against Cursor-denying instead of comparing two + // ACTUALLY enforcing configurations. + claudeProject = makeGitProject('gsd-cs-parity-claude-', JSON.stringify({ runtime: 'claude' })); // Must be a REAL git repo (not a bare mkdir'd directory): the Cursor hook's // #3045 MAJOR 3 "not a git repo -> INERT" branch would otherwise short-circuit diff --git a/tests/gsd-agent-isolation-guard.test.cjs b/tests/gsd-agent-isolation-guard.test.cjs index 782d7580b..9d0dce188 100644 --- a/tests/gsd-agent-isolation-guard.test.cjs +++ b/tests/gsd-agent-isolation-guard.test.cjs @@ -52,8 +52,9 @@ const os = require('node:os'); const fc = require('./helpers/fast-check-setup.cjs'); const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const { toLegacyResult, gitOrThrow } = require('./helpers/git-fixture.cjs'); -const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { PROBE_TIMEOUT_MS, GIT_FIXTURE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const { createTempDir, createTempProject, runGsdTools, cleanup } = require('./helpers.cjs'); +const { createFixture } = require('./fixtures/index.cjs'); const { SENTINEL_RELATIVE_PATH, SENTINEL_STALE_MS, readSentinel } = require('../hooks/lib/isolation-sentinel.js'); const { REASON_CODE, REASON_INTERPOLATION_MAX_LEN, sanitizeForReason, describeSentinelDiscard } = require('../hooks/lib/isolation-deny-reason.js'); const { runtimes } = require('../gsd-core/bin/lib/capability-registry.cjs'); @@ -121,9 +122,12 @@ function agentPayload(overrides = {}) { } function mkProject(prefix) { - const dir = createTempDir(prefix); - fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); - return dir; + // #4734: git-inited with a real HEAD. Production GSD project roots are git + // repositories, and the guard's fallback now degrades to 'none' when the + // root has NO repository — a non-git fixture here would exercise that + // degrade instead of the fallback enforcement these suites pin. The + // dedicated non-git world lives in the #4734 describe below. + return createFixture({ prefix, planning: true, git: true, projectDoc: false }); } function writeConfig(dir, content) { @@ -1652,6 +1656,14 @@ describe('guard fallback — worktreesOptedOut ladder (#3972)', () => { fs.mkdirSync(path.join(dir, '.planning', 'workstreams', 'alpha'), { recursive: true }); fs.writeFileSync(path.join(dir, '.planning', 'config.json'), JSON.stringify(rootCfg)); fs.writeFileSync(path.join(dir, '.planning', 'workstreams', 'alpha', 'config.json'), JSON.stringify(wsCfg)); + // #4734: a real repository HEAD. Production GSD workspaces are git repos; + // without this the fallback's new definitive-no-repository degrade — not + // the ladder — would answer the "no opt-out" pin below. + const gitOpts = { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }; + gitOrThrow(['init'], gitOpts); + gitOrThrow(['config', 'user.email', 'test@test.com'], gitOpts); + gitOrThrow(['config', 'user.name', 'Test'], gitOpts); + gitOrThrow(['commit', '--allow-empty', '-m', 'initial commit'], gitOpts); return dir; } @@ -1765,3 +1777,63 @@ describe('hooks/lib/isolation-deny-reason.js — sanitizeForReason (#4594 F2/F5/ assert.match(message, /dispatch phase="03" plan="07-01-x"/); }); }); + +// ─── #4734: a project root that is not a git repository ────────────────────── + +describe('gsd-agent-isolation-guard.js: #4734 — a non-git project root is never demanded worktree isolation', () => { + // The bug's world: `.planning/` at the root of a directory that is NOT a + // git repository (a multi-repo workspace). `git rev-parse HEAD` exits 128 — + // git's definitive answer that no repository exists here — so a harness + // worktree can never be created and the fallback must degrade to 'none' + // instead of demanding the flag and blocking every dispatch. + function mkNonGitProject(prefix) { + const dir = createTempDir(prefix); + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + writeConfig(dir, JSON.stringify({ runtime: 'claude' })); + return dir; + } + + function mkGitProject(prefix) { + // Mirror of mkNonGitProject with a real repository HEAD — the positive + // control proving the git check, not something else, drives the degrade. + const dir = createFixture({ prefix, planning: true, git: true, projectDoc: false }); + writeConfig(dir, JSON.stringify({ runtime: 'claude' })); + return dir; + } + + test('no sentinel (fallback path) + registry harness-worktree + non-git root → ALLOW a flag-less dispatch', (t) => { + const project = mkNonGitProject('gsd-aig-4734-nogit-'); + t.after(() => cleanup(project)); + const r = runHook(agentPayload(), project); + assert.equal(r.status, 0, `stdout: ${r.stdout} stderr: ${r.stderr}`); + assert.equal(r.stdout, '', 'an allowed dispatch must not write a block decision'); + }); + + test('stale sentinel lying "none" + non-git root → the fallback re-derives and still allows', (t) => { + const project = mkNonGitProject('gsd-aig-4734-nogit-stale-'); + t.after(() => cleanup(project)); + writeSentinel(project, { isolation: 'none', writtenAt: Date.now() - (SENTINEL_STALE_MS + 60000) }); + const r = runHook(agentPayload(), project); + assert.equal(r.status, 0, `stdout: ${r.stdout} stderr: ${r.stderr}`); + }); + + test('positive control: the same shape in a git-inited root still DENIES (the repository is the differentiator)', (t) => { + const project = mkGitProject('gsd-aig-4734-git-'); + t.after(() => cleanup(project)); + const r = runHook(agentPayload(), project); + assert.equal(r.status, 2, `stdout: ${r.stdout} stderr: ${r.stderr}`); + assert.equal(JSON.parse(r.stdout).decision, 'block'); + }); + + test('#4734 scope: a FRESH sentinel still governs a non-git root — the fix is fallback-only', (t) => { + // The sentinel-fresh path carries the workflow's own confirmed decision; + // with the base-check degrade (#4734a) that decision is made where git is + // actually consulted. The guard does not second-guess it here. + const project = mkNonGitProject('gsd-aig-4734-nogit-fresh-'); + t.after(() => cleanup(project)); + writeSentinel(project, { isolation: 'harness-worktree', harnessFlag: 'isolation="worktree"' }); + const r = runHook(agentPayload(), project); + assert.equal(r.status, 2, `stdout: ${r.stdout} stderr: ${r.stderr}`); + assert.equal(JSON.parse(r.stdout).decision, 'block'); + }); +}); diff --git a/tests/worktree-base-ref.test.cjs b/tests/worktree-base-ref.test.cjs index 75b97cd51..5fccfee7f 100644 --- a/tests/worktree-base-ref.test.cjs +++ b/tests/worktree-base-ref.test.cjs @@ -29,6 +29,7 @@ const { applyWorktreeBaseRef, resolveEffectiveBaseRef, evaluateWorktreeBaseDegrade, + classifyGitHead, cmdWorktreeBaseCheck, cmdWorktreeSetBaseRef, } = require(MODULE_PATH); @@ -254,19 +255,20 @@ describe('resolveEffectiveBaseRef', () => { // ─── evaluateWorktreeBaseDegrade ────────────────────────────────────────────── -describe('evaluateWorktreeBaseDegrade', () => { - // Stub helper: matches on args.join(' ') and returns canned results - function makeExecGit(responses) { - return function stubExecGit(args, _opts) { - const key = args.join(' '); - if (Object.prototype.hasOwnProperty.call(responses, key)) { - return responses[key]; - } - // Default: fail with a helpful error to surface unexpected calls - throw new Error(`Unexpected execGit call: ${JSON.stringify(args)}`); - }; - } +// Stub helper: matches on args.join(' ') and returns canned results. +// Module-scoped so the #4734 classifyGitHead describe shares one copy (review finding). +function makeExecGit(responses) { + return function stubExecGit(args, _opts) { + const key = args.join(' '); + if (Object.prototype.hasOwnProperty.call(responses, key)) { + return responses[key]; + } + // Default: fail with a helpful error to surface unexpected calls + throw new Error(`Unexpected execGit call: ${JSON.stringify(args)}`); + }; +} +describe('evaluateWorktreeBaseDegrade', () => { // #3659 rows share the diverged-HEAD stub shape — one builder keeps the // four fixtures from drifting apart. function makeDivergedExecGit(headSha, forkSha) { @@ -350,13 +352,13 @@ describe('evaluateWorktreeBaseDegrade', () => { assert.ok(result.message.includes('sequentially'), 'message must state the sequential fallback'); }); - test('git rev-parse HEAD fails → no degrade, reason no-head', () => { + test('git rev-parse HEAD exits 128 (definitive no-repository) → degrades, reason no-head (#4734)', () => { const result = evaluateWorktreeBaseDegrade({ execGit: makeExecGit({ 'rev-parse HEAD': { exitCode: 128, stdout: '', stderr: 'fatal: not a git repo', signal: null, error: null }, }), }); - assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.shouldDegrade, true); assert.strictEqual(result.reason, 'no-head'); assert.strictEqual(result.headSha, null); }); @@ -374,7 +376,10 @@ describe('evaluateWorktreeBaseDegrade', () => { // ─── #3050: fail-closed matrix for git rev-parse HEAD outcomes ───────────── // DECIDED RULE: degrade UNLESS git completed and gave a definitive answer. // - timeout → degrade, reason 'head-unresolvable' - // - exitCode === 128 → NO degrade, reason 'no-head' (unchanged) + // (#4734 revised the exit-128 row: git's definitive no-repository answer + // now DEGRADES — a worktree can never be created there. The ambiguous + // exit-0-empty row is unchanged, still deliberately non-degrading.) + // - exitCode === 128 → degrade, reason 'no-head' (#4734) // - exit 0 with non-empty sha → proceed (unchanged) // - anything else (127, other → degrade, reason 'head-unresolvable' // non-zero, exit 0 empty stdout @@ -441,32 +446,33 @@ describe('evaluateWorktreeBaseDegrade', () => { assert.strictEqual(result.reason, 'head-unresolvable'); }); - test('exitCode 128 ("not a git repository") still does NOT degrade (#3050 regression guard)', () => { + test('exitCode 128 ("not a git repository") degrades with a user-visible message (#4734; was a #3050 non-degrade pin)', () => { const result = evaluateWorktreeBaseDegrade({ execGit: makeExecGit({ 'rev-parse HEAD': { exitCode: 128, stdout: '', stderr: 'fatal: not a git repository', signal: null, error: null }, }), }); - assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.shouldDegrade, true); assert.strictEqual(result.reason, 'no-head'); + assert.strictEqual(result.headAbsenceVerified, true); + // Message presence is a typed fact (non-null, non-empty) — its prose is + // operator-facing text, not a test oracle (no raw-text matching). + assert.ok(typeof result.message === 'string' && result.message.length > 0, 'the degrade carries the message the workflow prints'); }); // ─── #3057 B8: headAbsenceVerified distinguishes the two "no-head" causes ── // - // Both outcomes below keep `shouldDegrade:false, reason:'no-head'` — that - // product decision is deliberately UNCHANGED (pinned by the regression - // guards above and flagged in the #3050 review as still an open question). - // What changes is that a caller can now tell git's DEFINITIVE "not a git - // repository" answer (exit 128) apart from git completing but returning - // nothing useful (exit 0, empty stdout) — the module's own #380-383 comment - // named this gap; these two paired tests prove it is closed. + // #4734 revised the exit-128 outcome (degrade, with headAbsenceVerified:true + // preserved) and left the exit-0-empty outcome deliberately unchanged — the + // paired tests below prove both, and that a caller can still tell the two + // 'no-head' causes apart. - test('exit 128 — git\'s definitive "not a git repository" answer → headAbsenceVerified:true', () => { + test('exit 128 — git\'s definitive "not a git repository" answer → degrades, headAbsenceVerified:true (#4734)', () => { const faultyGit = makeFaultyGit({ faults: [{ kind: 'exit', exitCode: 128, stderr: 'fatal: not a git repository' }], }); const result = evaluateWorktreeBaseDegrade({ execGit: faultyGit }); - assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.shouldDegrade, true); assert.strictEqual(result.reason, 'no-head'); assert.strictEqual(result.headAbsenceVerified, true); }); @@ -1401,3 +1407,69 @@ describe('execute-plan Pattern A: pre-dispatch worktree base-check (#2649)', () }); } + +describe('#4734: classifyGitHead — single owner of the HEAD-resolution classes', () => { + test('exit 0 with a sha → present, headSha carries the trimmed sha', () => { + const SHA = 'aabbccdd11223344aabbccdd11223344aabbccdd'; + const status = classifyGitHead({ + execGit: makeExecGit({ + 'rev-parse HEAD': { exitCode: 0, stdout: `${SHA}\n`, stderr: '', signal: null, error: null }, + }), + }); + assert.strictEqual(status.status, 'present'); + assert.strictEqual(status.headSha, SHA); + }); + + test('exit 128 → definitive-absence (not a repository, or a repository with no commits — neither can host a worktree)', () => { + const status = classifyGitHead({ + execGit: makeExecGit({ + 'rev-parse HEAD': { exitCode: 128, stdout: '', stderr: 'fatal: not a git repository', signal: null, error: null }, + }), + }); + assert.strictEqual(status.status, 'definitive-absence'); + assert.strictEqual(status.headSha, null); + }); + + test('a REAL non-git working directory degrades end-to-end (no injected seam — the #4734 fixture wording)', (t) => { + const { createTempDir, cleanup } = require('./helpers.cjs'); + const dir = createTempDir('gsd-4734-nogit-real-'); + t.after(() => cleanup(dir)); + const result = evaluateWorktreeBaseDegrade({ cwd: dir }); + assert.strictEqual(result.shouldDegrade, true); + assert.strictEqual(result.reason, 'no-head'); + assert.strictEqual(result.headAbsenceVerified, true); + assert.ok(typeof result.message === 'string' && result.message.length > 0); + }); + + test('exit 0 with empty stdout → ambiguous-absence (git completed without a definitive answer)', () => { + const status = classifyGitHead({ + execGit: makeExecGit({ + 'rev-parse HEAD': { exitCode: 0, stdout: '', stderr: '', signal: null, error: null }, + }), + }); + assert.strictEqual(status.status, 'ambiguous-absence'); + assert.strictEqual(status.headSha, null); + }); + + test('timeout → indeterminate (fail closed)', () => { + const timedOutErr = new Error('spawnSync git ETIMEDOUT'); + timedOutErr.code = 'ETIMEDOUT'; + const status = classifyGitHead({ + execGit: makeExecGit({ + 'rev-parse HEAD': { exitCode: null, stdout: '', stderr: '', signal: 'SIGTERM', error: timedOutErr }, + }), + }); + assert.strictEqual(status.status, 'indeterminate'); + assert.strictEqual(status.headSha, null); + }); + + test('other non-zero exit (git missing, exit 127) → indeterminate (fail closed)', () => { + const status = classifyGitHead({ + execGit: makeExecGit({ + 'rev-parse HEAD': { exitCode: 127, stdout: '', stderr: 'command not found', signal: null, error: null }, + }), + }); + assert.strictEqual(status.status, 'indeterminate'); + assert.strictEqual(status.headSha, null); + }); +}); diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index d0db8892f..800824955 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -326,12 +326,12 @@ describe('shared isSpawnTimeout predicate — parity for worktree-base-ref evalu assert.strictEqual(isSpawnTimeout(result), expectTimeout); // exitCode 128 ("not a git repository") is git's own definitive, - // completed answer — the ONLY non-timeout, non-success outcome that - // does not degrade. Pairing it with each non-timeout signal/error - // combination means: if isExecGitTimeout ever mis-classifies one of - // these as a timeout, this assertion flips from 'no-head' (no - // degrade) to 'head-unresolvable' (degrade) and the test fails — - // a real behavioral divergence signal, not a same-reason coincidence. + // completed answer. Since #4734 it degrades (no worktree can exist + // without a resolvable HEAD) but keeps its OWN reason — so pairing it + // with each non-timeout signal/error combination still yields a real + // divergence signal: if isExecGitTimeout ever mis-classifies one of + // these as a timeout, the reason flips from 'no-head' (#4734 degrade) + // to 'head-unresolvable' and the test fails. const execGit = () => ({ exitCode: expectTimeout ? null : 128, stdout: '', @@ -344,7 +344,7 @@ describe('shared isSpawnTimeout predicate — parity for worktree-base-ref evalu assert.strictEqual(degradeResult.shouldDegrade, true); assert.strictEqual(degradeResult.reason, 'head-unresolvable'); } else { - assert.strictEqual(degradeResult.shouldDegrade, false); + assert.strictEqual(degradeResult.shouldDegrade, true); assert.strictEqual(degradeResult.reason, 'no-head'); } });