From d022dab47094f81503ab53d9ea09119b9e2e7eee Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 23 May 2026 23:10:25 -0400 Subject: [PATCH] fix(#105): skip strategy-branch auto-switch when use_worktrees=false (#150) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#105): skip strategy branch auto-switch when use_worktrees is false When workflow.use_worktrees is false, the primary checkout is shared or pinned to a base branch. Auto-switching HEAD in that mode silently moves the shared checkout and allows concurrent commits to land on the wrong branch. ensureStrategyBranch now returns ok:true with an explicit skip reason instead of calling git checkout in this configuration. Co-Authored-By: Claude Sonnet 4.6 * test(#105): replace source-grep tests with behavioral coverage Remove the root tests/bug-105-*.test.cjs file which used source-text structural assertions (reading commit.ts as a string) — tests that pass even when the runtime behavior is broken. Replace with a Vitest test at sdk/src/query/commit.bug-105.test.ts that invokes ensureStrategyBranch directly with use_worktrees:false and asserts the guard fires before any git invocation, and that the guard does NOT fire when use_worktrees is true or absent. Co-Authored-By: Claude Opus 4.7 * chore(#105): add changeset fragment Co-Authored-By: Claude Opus 4.7 * fix(#105): address review — type WorkflowConfig.use_worktrees, accept "false" string, stub git in tests - Add `use_worktrees?: boolean | string` to WorkflowConfig interface with doc comment - Remove `(config.workflow as unknown as Record)` double cast; use typed `config.workflow?.use_worktrees` - Accept string `"false"` alongside boolean `false` via `isExplicitlyFalse` guard (YAML/JSON parser resilience) - Add `vi.mock('node:child_process')` stub in commit.bug-105.test.ts; assert no-call on skip-path tests - Add new test case for string `"false"` coercion Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .../105-no-auto-switch-worktrees-false.md | 5 + sdk/src/config.ts | 8 + sdk/src/query/commit.bug-105.test.ts | 195 ++++++++++++++++++ sdk/src/query/commit.ts | 16 ++ 4 files changed, 224 insertions(+) create mode 100644 .changeset/105-no-auto-switch-worktrees-false.md create mode 100644 sdk/src/query/commit.bug-105.test.ts diff --git a/.changeset/105-no-auto-switch-worktrees-false.md b/.changeset/105-no-auto-switch-worktrees-false.md new file mode 100644 index 000000000..f8cfb4777 --- /dev/null +++ b/.changeset/105-no-auto-switch-worktrees-false.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 150 +--- +Strategy-branch auto-switch is now skipped when `workflow.use_worktrees` is `false` — the SDK no longer switches the primary branch in single-worktree setups. diff --git a/sdk/src/config.ts b/sdk/src/config.ts index 99b840e2c..8414b99a5 100644 --- a/sdk/src/config.ts +++ b/sdk/src/config.ts @@ -64,6 +64,14 @@ export interface WorkflowConfig { * verify-phase (validation gate, non-blocking). Set false to disable both. */ context_coverage_gate: boolean; + /** + * Issue #105. When false, the primary checkout is shared or pinned (concurrent + * sessions / deliberate base-branch lock) and the commit handler must NOT + * auto-switch HEAD to the strategy branch. String value `"false"` is accepted + * for resilience against YAML/JSON parsers that leave boolean-like fields as + * strings. + */ + use_worktrees?: boolean | string; } export interface HooksConfig { diff --git a/sdk/src/query/commit.bug-105.test.ts b/sdk/src/query/commit.bug-105.test.ts new file mode 100644 index 000000000..9bd53dfa1 --- /dev/null +++ b/sdk/src/query/commit.bug-105.test.ts @@ -0,0 +1,195 @@ +/** + * Behavioral regression tests for bug #105. + * + * `gsd-tools commit` / SDK commit unconditionally switches the current + * checkout to the strategy branch with no opt-out, silently moving a + * shared HEAD and causing commits from parallel sessions to land on the + * wrong branch. + * + * Fix: when `workflow.use_worktrees` is `false`, `ensureStrategyBranch` + * must return `{ ok: true, reason: }` WITHOUT + * performing a `git checkout`. + * + * These tests exercise the real runtime path via Vitest (no source-grep). + */ + +import { describe, it, expect, afterEach, vi, beforeEach } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +// Stub out the git layer so the skip-path tests can assert no git invocation +// occurred, and the no-skip-path tests can assert it was attempted. +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawnSync: vi.fn(() => ({ + status: 1, + stdout: '', + stderr: 'not a git repository', + })), + }; +}); + +import { spawnSync } from 'node:child_process'; + +// ─── helpers ────────────────────────────────────────────────────────────── + +/** + * Create a minimal project directory with .planning/config.json. + * Returns the temp directory path. + * + * The directory is intentionally NOT a git repo so that any attempt to + * run `git checkout` inside it will fail — which would propagate as + * ok: false from ensureStrategyBranch. The guard in the fix must fire + * BEFORE any git invocation when use_worktrees is false. + */ +function makeTmpProject(workflowOverrides: Record = {}): string { + const tmpDir = mkdtempSync(join(tmpdir(), 'bug-105-')); + const planningDir = join(tmpDir, '.planning'); + mkdirSync(planningDir, { recursive: true }); + const config = { + git: { + branching_strategy: 'phase', + phase_branch_template: 'phase/{phase}-{slug}', + milestone_branch_template: 'ms/{milestone}-{slug}', + quick_branch_template: null, + }, + workflow: { + use_worktrees: false, + ...workflowOverrides, + }, + }; + writeFileSync(join(planningDir, 'config.json'), JSON.stringify(config)); + return tmpDir; +} + +const tmpDirs: string[] = []; +const spawnSyncMock = vi.mocked(spawnSync); + +beforeEach(() => { + vi.clearAllMocks(); +}); + +afterEach(() => { + for (const d of tmpDirs.splice(0)) { + try { rmSync(d, { recursive: true, force: true }); } catch { /* ignore */ } + } +}); + +// ─── bug-105 behavioral tests ───────────────────────────────────────────── + +describe('bug-105: ensureStrategyBranch skips branch switch when use_worktrees is false', () => { + it('returns ok:true when use_worktrees is false', async () => { + const { ensureStrategyBranch } = await import('./commit.js'); + const tmpDir = makeTmpProject({ use_worktrees: false }); + tmpDirs.push(tmpDir); + + const result = await ensureStrategyBranch(tmpDir, undefined, ['1-setup/plan.md']); + + expect(result.ok).toBe(true); + // Guard must fire BEFORE any git invocation + expect(spawnSyncMock).not.toHaveBeenCalled(); + }); + + it('reason mentions use_worktrees when skipping', async () => { + const { ensureStrategyBranch } = await import('./commit.js'); + const tmpDir = makeTmpProject({ use_worktrees: false }); + tmpDirs.push(tmpDir); + + const result = await ensureStrategyBranch(tmpDir, undefined, ['1-setup/plan.md']); + + expect(result.ok).toBe(true); + const reason = (result as { ok: true; reason?: string }).reason ?? ''; + expect(reason).toContain('use_worktrees'); + expect(spawnSyncMock).not.toHaveBeenCalled(); + }); + + it('does not attempt git checkout when use_worktrees is false (non-git dir stays ok:true)', async () => { + // The tmpDir is NOT a git repo. If ensureStrategyBranch were to run + // `git checkout` it would exit non-zero and return ok:false with a + // branch_switch_failed reason. The guard must fire BEFORE the git call. + const { ensureStrategyBranch } = await import('./commit.js'); + const tmpDir = makeTmpProject({ use_worktrees: false }); + tmpDirs.push(tmpDir); + + const result = await ensureStrategyBranch(tmpDir, undefined, ['2-build/state.md']); + + // If the guard fired correctly, we get ok:true even without a git repo. + expect(result.ok).toBe(true); + const reason = (result as { ok: true; reason?: string }).reason ?? ''; + // Must be the use_worktrees skip reason, not a git failure or phase error. + expect(reason).toContain('use_worktrees'); + // No git command must have been spawned + expect(spawnSyncMock).not.toHaveBeenCalled(); + }); + + it('skips when use_worktrees is the string "false" (YAML/JSON parser coercion)', async () => { + // YAML/JSON parsers can leave boolean-like fields as strings. + // The guard must treat the string "false" identically to the boolean false. + const { ensureStrategyBranch } = await import('./commit.js'); + const tmpDir = makeTmpProject({ use_worktrees: 'false' }); + tmpDirs.push(tmpDir); + + const result = await ensureStrategyBranch(tmpDir, undefined, ['1-setup/plan.md']); + + expect(result.ok).toBe(true); + const reason = (result as { ok: true; reason?: string }).reason ?? ''; + expect(reason).toContain('use_worktrees'); + expect(spawnSyncMock).not.toHaveBeenCalled(); + }); + + it('does NOT skip when use_worktrees is true (guard does not fire → reason is not use_worktrees)', async () => { + // With use_worktrees: true, the guard must NOT fire. + // Phase lookup finds nothing in the tmp dir → skips for a different reason. + const { ensureStrategyBranch } = await import('./commit.js'); + const tmpDir = makeTmpProject({ use_worktrees: true }); + tmpDirs.push(tmpDir); + + const result = await ensureStrategyBranch(tmpDir, undefined, ['1-setup/plan.md']); + + // The result may be ok:true (phase not found) or ok:false (git failed). + // Either way the reason must NOT mention the use_worktrees guard. + if (result.ok) { + const reason = (result as { ok: true; reason?: string }).reason ?? ''; + expect(reason).not.toContain('use_worktrees'); + } else { + const reason = (result as { ok: false; reason: string }).reason; + expect(reason).not.toContain('use_worktrees'); + } + // The use_worktrees guard must NOT have fired before any git call — confirm + // that spawnSync was NOT called due to an early guard return. + // (Phase not found causes an fs-only skip before git — that is acceptable: + // what matters is the reason is not "use_worktrees".) + }); + + it('does NOT skip when use_worktrees is absent (undefined → reason is not use_worktrees)', async () => { + // When workflow.use_worktrees is not set at all, the guard must not fire. + const { ensureStrategyBranch } = await import('./commit.js'); + const tmpDir = mkdtempSync(join(tmpdir(), 'bug-105-absent-')); + tmpDirs.push(tmpDir); + const planningDir = join(tmpDir, '.planning'); + mkdirSync(planningDir, { recursive: true }); + // Config with no workflow.use_worktrees key at all + writeFileSync(join(planningDir, 'config.json'), JSON.stringify({ + git: { + branching_strategy: 'phase', + phase_branch_template: 'phase/{phase}-{slug}', + milestone_branch_template: 'ms/{milestone}-{slug}', + quick_branch_template: null, + }, + workflow: {}, + })); + + const result = await ensureStrategyBranch(tmpDir, undefined, ['1-setup/plan.md']); + + if (result.ok) { + const reason = (result as { ok: true; reason?: string }).reason ?? ''; + expect(reason).not.toContain('use_worktrees'); + } else { + const reason = (result as { ok: false; reason: string }).reason; + expect(reason).not.toContain('use_worktrees'); + } + }); +}); diff --git a/sdk/src/query/commit.ts b/sdk/src/query/commit.ts index fe9dbc312..f21ad69da 100644 --- a/sdk/src/query/commit.ts +++ b/sdk/src/query/commit.ts @@ -233,6 +233,22 @@ export async function ensureStrategyBranch( return { ok: true, reason: 'strategy-skipped: config load failed' }; } + // Issue #105: when use_worktrees is false the primary checkout is shared or + // pinned (concurrent sessions / deliberate base-branch lock). Auto-switching + // HEAD in that mode silently moves the shared checkout and causes commits from + // parallel sessions to land on the wrong branch. Skip the switch entirely; + // the caller commits on whatever branch the primary is currently on. + // String "false" is tolerated for resilience against YAML/JSON parsers that + // leave boolean-like fields as strings. + const useWorktrees = config.workflow?.use_worktrees; + const isExplicitlyFalse = useWorktrees === false || useWorktrees === 'false'; + if (isExplicitlyFalse) { + return { + ok: true, + reason: 'strategy-skipped: use_worktrees is false — shared/pinned primary checkout; branch auto-switch suppressed (#105)', + }; + } + const strategy = config.git.branching_strategy; if (!strategy || strategy === 'none') return { ok: true };