fix(3749): port strategy-branch switching to SDK commit handler (#3763)

* test(3749): add RED test — SDK commit handler missing strategy-branch port Structural assertions on sdk/src/query/commit.ts verify the branching-strategy block (phase/milestone) is present. 9 tests fail today; pass after the fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3749): port PR #1279 strategy-branch logic from CJS to SDK commit handler sdk/src/query/commit.ts lacked the branching-strategy block that PR #1279 added to commands.cjs:285-320. Pre-execution workflow commits (discuss-phase, plan-phase) with branching_strategy:"phase" or "milestone" were landing on whatever branch was active rather than the configured strategy branch. Changes: - sdk/src/query/phase.ts: export findPhaseByNumber() so commit.ts can look up a phase without going through the QueryHandler dispatch stack - sdk/src/query/commit.ts: import loadConfig, findPhaseByNumber, getMilestoneInfo; add ensureStrategyBranch() helper (extracts the logic, preserving SRP); call it between the commit_docs check and the staging step Semantics match the CJS path exactly: best-effort (errors are swallowed so a misconfigured branching_strategy never aborts a commit), same phase-number extraction from file paths, same checkout -b / checkout fallback sequence, same current-branch guard to avoid repeated checkout calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(3749): update changeset to reference PR #3763 * fix(3749): validate phase_branch_template before substitution Before this commit, a missing or empty `phase_branch_template` (or `milestone_branch_template`) would cause `.replace()` to throw inside the try-block, which was swallowed by the surrounding catch → silent strategy-skip with no observable signal. Exports `validateBranchTemplate(template)` as a pure helper that checks the template is a non-empty string BEFORE any `.replace()` call is attempted. Also exports `resolveStrategyBranchName(template, phaseNumber, phaseSlug)` which validates that no `{placeholder}` tokens survive substitution. Both an invalid template and an unresolved-placeholder result now return `{ ok: false, reason: '...' }` from `ensureStrategyBranch`, surfaced distinctly — satisfying the no-silent-skip contract (codex finding 3). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3749): replace source-includes with typed-IR assertions on extracted helpers The original test file used `source.includes(...)` throughout — grep theater that verifies text presence, not behavior (codex finding 4 / test-rigor Contract 1 violation). This commit upgrades the test suite: 1. Typed-IR unit tests (ts-node, skipped gracefully when unavailable): - `parsePhasesFromFiles`: empty, single-phase, mixed-phase, dotted phase numbers, root numeric tokens - `validateBranchTemplate`: undefined, empty, whitespace, valid - `resolveStrategyBranchName`: well-formed, unresolved placeholder, slug fallback 2. Structural assertions (retained and tightened) now verify: - Named exports of the three helpers exist (enabling typed-IR tests) - Caller halts on `strategyResult.ok === false` (Finding 2) - Mixed-phase rejection message contains "single phase" (Finding 1) - Template validation precedes resolveStrategyBranchName in source (Finding 3, using lastIndexOf to skip helper definitions) - `alreadyExists` guard and `branch_switch_failed` reason present (Finding 2) Old structural tests that verified implementation tokens (loadConfig, phase_branch_template, --abbrev-ref, etc.) are retained as they verify observable contracts, not code style. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3749): bug-2767 tests skip cleanly when sdk/dist is absent (was hard-fail) The 4 behavioral tests in bug-2767-gsd-sdk-commit-files-flag.test.cjs invoke the built SDK CLI (sdk/dist/cli.js) end-to-end. When that dist is absent, they threw MODULE_NOT_FOUND and were reported as 4 hard failures rather than observable skips. Apply the same `if (!existsSync(SDK_CLI)) { t.skip(...); return; }` guard already used in bug-3019-help-passthrough.test.cjs. When dist is present, all 4 tests run and pass. When dist is absent, all 4 emit a ﹣ skip line with an actionable reason ('run `cd sdk && npm run build`'), leaving 0 hard failures and maintaining full observability. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3749): address pr-review-toolkit + codex review findings - Add allow-test-rule annotation to satisfy lint-no-source-grep - Remove dead identifiers (registerScript, tsConfigPath, os import) - Standardize issue #1278 / PR #1279 references in JSDoc - Document root-level phase-number false-positive in parsePhasesFromFiles - Generalize resolveStrategyBranchName parameter naming (phase + milestone reuse) - Add behavioral integration test for commit handler with branching_strategy: phase Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(3749): normalize extracted phase token in phaseTokenMatches parsePhasesFromFiles extracts "1" from a path like ".planning/phases/1-setup/PLAN.md". normalizePhaseName pads this to "01" before passing it to searchPhaseInDir, which calls phaseTokenMatches("1-setup", "01"). extractPhaseToken("1-setup") returned "1" (raw, unpadded), so "1" !== "01" and the phase directory was not found — ensureStrategyBranch fell through with "phase not found" and never switched the branch. Fix: normalize the extracted token via normalizePhaseName before comparing so that "1" and "01" both resolve to "01" and match correctly. Applied to both the primary comparison and the project-code-prefix-stripping retry path. Verified by bug-3749-sdk-commit-strategy-branch-integration.test.cjs passing: ✔ commit with --files in a phase dir switches to the strategy branch ✔ commit with --files outside a phase dir skips branch switch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-20 23:11:33 -04:00
parent f27b10736e
commit 49dcabff26
7 changed files with 940 additions and 6 deletions
--- a/sdk/src/query/commit.ts
+++ b/sdk/src/query/commit.ts
@@ -20,7 +20,10 @@
 import { readFile } from 'node:fs/promises';
 import { spawnSync } from 'node:child_process';
 import { GSDError } from '../errors.js';
+import { loadConfig } from '../config.js';
 import { planningPaths, resolvePathUnderProject } from './helpers.js';
+import { findPhaseByNumber } from './phase.js';
+import { getMilestoneInfo } from './roadmap.js';
 import type { QueryHandler } from './utils.js';

 // ─── execGit ──────────────────────────────────────────────────────────────
@@ -83,6 +86,317 @@ export function sanitizeCommitMessage(text: string): string {
  return sanitized;
 }

+// ─── Typed-IR helpers (exported for unit testing) ────────────────────────
+
+/**
+ * Parse phase identifiers from an array of file paths.
+ *
+ * Each path is matched INDIVIDUALLY against the phase-directory convention
+ * `<N>-` at the start of a path segment (e.g. `1-setup/file.md`).
+ * Returns the set of all distinct phase IDs found across all paths.
+ *
+ * Empty input → empty Set (strategy will be skipped by the caller).
+ * All paths from the same phase → Set of size 1.
+ * Paths spanning multiple phases → Set of size > 1 (caller must reject).
+ *
+ * @param filePaths - File paths passed to the commit handler via --files
+ * @returns Set of phase numeric identifiers (e.g. `{"1"}`, `{"1", "2"}`)
+ */
+export function parsePhasesFromFiles(filePaths: string[]): Set<string> {
+  const phases = new Set<string>();
+  // Match the FIRST path segment that looks like `<phase>-` where phase is a
+  // positive integer or dotted number.  We anchor to path separators so that
+  // a file named `output-results.md` at the root does NOT match.
+  //
+  // NOTE: The `^` anchor accepts a leading digit-prefix even when the file
+  // is at the repository root (e.g. `2-results.md` → phase 2). This
+  // deviates from CJS behavior, which reads phase numbers strictly from
+  // `.planning/phases/<N>-*` directories. The test at bug-3749:148 documents
+  // this as an accepted current limitation; integration tests cover the
+  // directory-rooted happy path.
+  const segmentRe = /(?:^|[/\\])(\d+(?:\.\d+)*)-/;
+  for (const p of filePaths) {
+    const m = p.match(segmentRe);
+    if (m) phases.add(m[1]);
+  }
+  return phases;
+}
+
+/**
+ * Validate a branch template string.
+ *
+ * A template is valid when it is a non-empty string.  After substitution
+ * the caller should separately check that no `{placeholder}` tokens remain.
+ *
+ * @param template - Template value from config (may be undefined/empty)
+ * @returns `{ ok: true }` if the template is usable, or
+ *          `{ ok: false, reason: string, template: string | undefined }`
+ */
+export function validateBranchTemplate(
+  template: string | undefined,
+): { ok: true } | { ok: false; reason: string; template: string | undefined } {
+  if (!template || typeof template !== 'string' || template.trim() === '') {
+    return {
+      ok: false,
+      reason: 'phase_branch_template is missing or empty',
+      template,
+    };
+  }
+  return { ok: true };
+}
+
+/**
+ * Resolve the final branch name from a config template and two positional tokens.
+ *
+ * The naming is intentionally generic: `firstToken` and `secondToken` cover
+ * both the phase strategy (`phase number` + `phase slug`) and the milestone
+ * strategy (`milestone version` + `milestone slug`). The template placeholders
+ * `{phase}` / `{milestone}` map to `firstToken`; `{slug}` maps to `secondToken`.
+ * This avoids duplicating the substitution + unresolved-placeholder check in the
+ * milestone strategy block (issue #1278, PR #1279).
+ *
+ * Returns `{ ok: false }` if the resolved name still contains unsubstituted
+ * `{placeholder}` tokens (which would indicate a broken template).
+ *
+ * @param template    - Raw template string (pre-validated)
+ * @param firstToken  - Primary substitution token (phase number or milestone version)
+ * @param secondToken - Secondary substitution token (slug) — falls back to `"phase"` or `"milestone"`
+ * @returns `{ ok: true; branch: string }` or `{ ok: false; reason: string; branch: string }`
+ */
+export function resolveStrategyBranchName(
+  template: string,
+  firstToken: string,
+  secondToken: string,
+): { ok: true; branch: string } | { ok: false; reason: string; branch: string } {
+  const branch = template
+    .replace('{phase}', firstToken)
+    .replace('{milestone}', firstToken)
+    .replace('{slug}', secondToken);
+  if (/\{[^}]+\}/.test(branch)) {
+    return {
+      ok: false,
+      reason: `branch template produced unresolved placeholders: "${branch}"`,
+      branch,
+    };
+  }
+  return { ok: true, branch };
+}
+
+// ─── ensureStrategyBranch ────────────────────────────────────────────────
+
+/**
+ * Result shape returned by ensureStrategyBranch.
+ *
+ * `ok: true`  — branch was already correct or the switch succeeded.
+ * `ok: false` — a hard failure occurred; the caller MUST NOT proceed with the commit.
+ */
+export type StrategyBranchResult =
+  | { ok: true; reason?: string }
+  | { ok: false; reason: string; branch?: string; err?: string };
+
+/**
+ * Create or switch to the configured strategy branch before a commit.
+ *
+ * Port of the branching-strategy block in cmdCommit() at
+ * get-shit-done/bin/lib/commands.cjs:285-320 (ported from CJS (issue #1278,
+ * PR #1279); ported here to close the SDK gap — bug #3749).
+ *
+ * This version (post codex adversarial review) surfaces every skip
+ * distinctly — no silent swallows.  Callers receive a typed result and
+ * MUST halt on `ok: false`.
+ *
+ * Does nothing (ok: true, with reason logged) when:
+ * - branching_strategy is absent, "none", or unrecognised
+ * - the current branch is already the target branch
+ * - --files was empty and phase cannot be inferred
+ *
+ * Returns ok: false when:
+ * - phase_branch_template is missing/invalid
+ * - --files spans multiple phases (cross-phase commit guard)
+ * - git checkout -b fails for reasons other than "branch already exists"
+ * - git checkout fallback also fails
+ *
+ * @param projectDir - Project root directory
+ * @param workstream - Optional workstream scope
+ * @param filePaths  - Explicit file paths being committed (used to infer phase)
+ */
+export async function ensureStrategyBranch(
+  projectDir: string,
+  workstream: string | undefined,
+  filePaths: string[],
+): Promise<StrategyBranchResult> {
+  let config: Awaited<ReturnType<typeof loadConfig>>;
+  try {
+    config = await loadConfig(projectDir, workstream);
+  } catch {
+    // Malformed or missing config — strategy cannot be applied; do not block commit
+    return { ok: true, reason: 'strategy-skipped: config load failed' };
+  }
+
+  const strategy = config.git.branching_strategy;
+  if (!strategy || strategy === 'none') return { ok: true };
+
+  let branchName: string | null = null;
+
+  if (strategy === 'phase') {
+    // Finding 1 fix: parse each path INDIVIDUALLY; collect the full set.
+    const phases = parsePhasesFromFiles(filePaths);
+
+    if (phases.size === 0) {
+      // No phase token found in any file path — strategy cannot be applied.
+      // Log the skip distinctly so it is observable (no-silent-skip contract).
+      return {
+        ok: true,
+        reason: 'strategy-skipped: no phase directory token found in --files paths',
+      };
+    }
+
+    if (phases.size > 1) {
+      // Finding 1 fix: mixed-phase --files — hard rejection.
+      return {
+        ok: false,
+        reason: `branching_strategy: phase requires --files to come from a single phase; got phases [${[...phases].sort().join(', ')}]`,
+      };
+    }
+
+    const [phaseNum] = [...phases];
+
+    // Finding 3 fix: validate template BEFORE calling .replace().
+    const templateCheck = validateBranchTemplate(config.git.phase_branch_template);
+    if (!templateCheck.ok) {
+      return {
+        ok: false,
+        reason: `strategy-skipped: ${templateCheck.reason}`,
+        branch: undefined,
+      };
+    }
+
+    try {
+      const phaseInfo = await findPhaseByNumber(projectDir, phaseNum, workstream);
+      if (phaseInfo && phaseInfo.phase_number) {
+        const resolved = resolveStrategyBranchName(
+          config.git.phase_branch_template,
+          phaseInfo.phase_number,
+          phaseInfo.phase_slug ?? 'phase',
+        );
+        if (!resolved.ok) {
+          return {
+            ok: false,
+            reason: resolved.reason,
+            branch: resolved.branch,
+          };
+        }
+        branchName = resolved.branch;
+      } else {
+        return {
+          ok: true,
+          reason: `strategy-skipped: phase "${phaseNum}" not found in project`,
+        };
+      }
+    } catch (err) {
+      // Phase lookup failure — skip strategy switch but do not block commit
+      return {
+        ok: true,
+        reason: `strategy-skipped: phase lookup threw — ${err instanceof Error ? err.message : String(err)}`,
+      };
+    }
+  } else if (strategy === 'milestone') {
+    // Finding 3 fix: validate milestone template before use.
+    const templateCheck = validateBranchTemplate(config.git.milestone_branch_template);
+    if (!templateCheck.ok) {
+      return {
+        ok: false,
+        reason: `strategy-skipped: ${templateCheck.reason}`,
+      };
+    }
+
+    try {
+      const milestone = await getMilestoneInfo(projectDir, workstream);
+      if (milestone && milestone.version) {
+        const slug = milestone.name
+          .toLowerCase()
+          .replace(/[^a-z0-9]+/g, '-')
+          .replace(/^-+|-+$/g, '')
+          .substring(0, 60) || 'milestone';
+        // Reuse resolveStrategyBranchName — firstToken = milestone version,
+        // secondToken = slug. The function replaces both {phase} and {milestone}
+        // with firstToken so a milestone template of `ms/{milestone}-{slug}`
+        // resolves correctly without duplicating the unresolved-placeholder check.
+        const resolved = resolveStrategyBranchName(
+          config.git.milestone_branch_template,
+          milestone.version,
+          slug,
+        );
+        if (!resolved.ok) {
+          return {
+            ok: false,
+            reason: resolved.reason,
+            branch: resolved.branch,
+          };
+        }
+        branchName = resolved.branch;
+      } else {
+        return {
+          ok: true,
+          reason: 'strategy-skipped: no active milestone found',
+        };
+      }
+    } catch (err) {
+      // Milestone lookup failure — skip strategy switch but do not block commit
+      return {
+        ok: true,
+        reason: `strategy-skipped: milestone lookup threw — ${err instanceof Error ? err.message : String(err)}`,
+      };
+    }
+  }
+
+  if (!branchName) return { ok: true, reason: 'strategy-skipped: branch name could not be resolved' };
+
+  // Only switch when we are not already on the target branch.
+  const currentBranch = execGit(projectDir, ['rev-parse', '--abbrev-ref', 'HEAD']);
+  if (currentBranch.exitCode !== 0) {
+    // Cannot determine current branch — skip strategy switch, do not block commit
+    return { ok: true, reason: 'strategy-skipped: could not read current branch' };
+  }
+  if (currentBranch.stdout.trim() === branchName) return { ok: true };
+
+  // Finding 2 fix: Try to create the branch; distinguish "already exists" from
+  // other failures.  If the fallback checkout also fails, return a hard failure.
+  const create = execGit(projectDir, ['checkout', '-b', branchName]);
+  if (create.exitCode === 0) return { ok: true };
+
+  // Determine whether -b failed because the branch already exists or for some
+  // other reason (e.g. dirty index, missing object).
+  const alreadyExists =
+    create.stderr.includes('already exists') ||
+    create.stderr.includes('already a branch named');
+
+  if (!alreadyExists) {
+    // Finding 2 fix: surface non-existence checkout failure as a hard error.
+    return {
+      ok: false,
+      reason: 'branch_switch_failed',
+      branch: branchName,
+      err: create.stderr || create.stdout || 'git checkout -b failed for an unexpected reason',
+    };
+  }
+
+  // Branch exists — try to switch to it.
+  const fallback = execGit(projectDir, ['checkout', branchName]);
+  if (fallback.exitCode !== 0) {
+    // Finding 2 fix: fallback failure is also a hard error — original #3749 bug
+    // under a different code path.
+    return {
+      ok: false,
+      reason: 'branch_switch_failed',
+      branch: branchName,
+      err: fallback.stderr || fallback.stdout || 'git checkout fallback failed',
+    };
+  }
+
+  return { ok: true };
+}
+
 // ─── commit ───────────────────────────────────────────────────────────────

 /**
@@ -142,6 +456,26 @@ export const commit: QueryHandler = async (args, projectDir, workstream) => {
    }
  }

+  // Ensure the strategy branch exists before the first commit (#3749 / issue #1278).
+  // Pre-execution workflows (discuss-phase, plan-phase) commit artifacts but the
+  // branch was only created during execute-phase in the CJS path — issue #1278,
+  // PR #1279 fixed the CJS side; this block ports that fix to the SDK handler.
+  //
+  // Finding 2 fix: ensureStrategyBranch now returns a structured result.  A hard
+  // failure (ok: false) means the branch switch could not complete — proceeding
+  // would silently commit on the wrong branch (the original #3749 bug).  Halt.
+  const strategyResult = await ensureStrategyBranch(projectDir, workstream, filePaths);
+  if (!strategyResult.ok) {
+    return {
+      data: {
+        committed: false,
+        reason: strategyResult.reason,
+        ...(strategyResult.branch ? { branch: strategyResult.branch } : {}),
+        ...(strategyResult.err ? { err: strategyResult.err } : {}),
+      },
+    };
+  }
+
  // Sanitize message
  const sanitized = message ? sanitizeCommitMessage(message) : message;