fix(#3311): milestone lock makes parallel-phase state conflicts visible (#3455)

* fix(#3311): milestone lock makes parallel-phase state conflicts visible

Two sessions running different phases in one working tree silently
clobbered STATE.md's single un-scoped ## Current Position slot: byte-level
serialization already existed (STATE.md.lock since #464), but nothing ever
surfaced that two sessions claimed two different phases, and
state.advance-plan — which takes no phase argument — kept advancing whatever
plan the just-clobbered position named.

Adds the maintainer-chosen milestone lock (issue #3311 comment): an advisory
.planning/milestone.lock claim keyed by phase + session id (session identity
via getWorkstreamSessionKey: env-first, then controlling TTY). begin-phase
claims (inside the STATE.md lock), advance-plan detects a claim/position
mismatch and heartbeats a matching claim, phase.complete warns via warnings[]
and releases the claim when the claimed phase completes. Conflicts warn
(stderr + typed milestone_conflict JSON field) instead of blocking, per the
decision's blocking/warning latitude; TTL 4h with heartbeat liveness expires
abandoned claims. milestone.lock is registered in the canonical artifact
registry so validate.health W019 recognizes it.

* chore(#3311): point changeset fragment at pr 3455

---------

Co-authored-by: sim <sim@local>

src/artifacts.cts

@@ -26,6 +26,7 @@ export const CANONICAL_EXACT: ReadonlySet<string> = new Set([
   'RETROSPECTIVE.md',
   'WINDOWS.md', // #3224: broken-windows ledger (src/broken-windows.cts, LEDGER_FILE_NAME)
   'STATE-ARCHIVE.md', // state.cts's cmdStatePrune writes this at the .planning/ root
+  'milestone.lock', // #3311: milestone (phase + session) claim (src/milestone-lock.cts); persistent, unlike the transient STATE.md.lock/WAITING.json
 ]);
 
 // Pattern-match canonical file names (regex tests on the basename)

src/milestone-lock.cts

@@ -0,0 +1,264 @@
+/**
+ * Milestone Lock — advisory (phase, session) claim over the single Current
+ * Position slot in STATE.md (#3311).
+ *
+ * Problem it solves: two sessions running DIFFERENT phases in the same working
+ * tree both read-modify-write `## Current Position`. The byte-level STATE.md
+ * lock (#464) already serializes those writes, so no write is lost — but the
+ * SEMANTIC clobber is silent: `state.advance-plan` takes no phase argument and
+ * advances whatever plan the (possibly just clobbered) Current Position names,
+ * and nothing ever surfaces that two sessions claimed two different phases
+ * against the one single-slot field.
+ *
+ * This module is the maintainer-chosen fix (issue #3311 comment): a milestone
+ * lock keyed by phase + session id, with a second session WARNED instead of
+ * silently overwriting. It is an advisory claim file, not a mutex:
+ *
+ * - `.planning/milestone.lock` (workstream-scoped via planningDir) holds
+ *   `{ phase, session, pid, updated_at }`.
+ * - Session identity reuses `getWorkstreamSessionKey()`
+ *   (active-workstream-store.cjs): env-first (GSD_SESSION_KEY,
+ *   CLAUDE_SESSION_ID, CODEX_THREAD_ID, …), then the controlling TTY. Two
+ *   agent sessions in different terminals/runtimes resolve different keys;
+ *   headless runs resolve to null.
+ * - Liveness is age-based only: a claim is live while
+ *   `now - updated_at < MILESTONE_LOCK_TTL_MS`. There is deliberately NO
+ *   pid-liveness gate — CLI invocations are short-lived, so the recorded pid
+ *   is dead by the next invocation regardless of whether the agent SESSION is
+ *   still active; pid-death would make every claim instantly stale. Active
+ *   sessions heartbeat their claim (advance-plan on a matching position
+ *   refreshes updated_at); an abandoned claim self-expires after the TTL.
+ * - Conflict rule: a live claim for a DIFFERENT phase held by a DIFFERENT
+ *   session. Same phase never conflicts (the lock is keyed by phase — two
+ *   sessions on one phase are doing the same work). A non-null session key
+ *   equal to the caller's never conflicts (one orchestrating session may
+ *   re-target its own claim). A null session key never counts as "same" as
+ *   anything, so headless parallel phases are still detected. The one
+ *   exception is advance-plan (checkMilestonePosition), which reports ANY
+ *   live claim/position phase mismatch regardless of session — a session's
+ *   own legitimate re-targeting goes through begin-phase, which keeps the
+ *   claim in sync, so a same-session mismatch is just as anomalous.
+ * - On conflict the existing claim is LEFT INTACT (not stolen): as long as two
+ *   phases are concurrently active, every Current Position mutation keeps
+ *   reporting the conflict. The conflicting command still proceeds (warn, not
+ *   block — a stale-but-live claim must not brick later sessions).
+ */
+
+import fs from 'node:fs';
+import path from 'node:path';
+import { realClock } from './clock.cjs';
+// eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module
+import planningWorkspace = require('./planning-workspace.cjs');
+// eslint-disable-next-line @typescript-eslint/no-require-imports -- active-workstream-store.cjs is an export= CommonJS module
+import activeWorkstreamStore = require('./active-workstream-store.cjs');
+
+export const MILESTONE_LOCK_FILENAME = 'milestone.lock';
+
+/**
+ * How long a claim stays live without a heartbeat. Active sessions heartbeat
+ * on every advance-plan against their claimed phase; a phase's planning stage
+ * (begin-phase → first completed plan) can legitimately run for hours, so the
+ * floor must comfortably exceed the longest expected gap between heartbeats
+ * while still expiring an abandoned claim within a working day.
+ */
+export const MILESTONE_LOCK_TTL_MS = 4 * 60 * 60 * 1000;
+
+interface MilestoneClaim {
+  phase: string;
+  session: string | null;
+  pid: number;
+  updated_at: number;
+}
+
+export interface MilestoneConflict {
+  /** Phase the live claim holds. */
+  locked_phase: string;
+  /** Session key of the live claim's holder (null = headless/unknown). */
+  locked_session: string | null;
+  /** Phase the calling command targets / the Current Position names. */
+  phase: string;
+  /** Calling session's key (null = headless/unknown). */
+  session: string | null;
+}
+
+function milestoneLockPath(cwd: string): string {
+  return path.join(planningWorkspace.planningDir(cwd), MILESTONE_LOCK_FILENAME);
+}
+
+/**
+ * Normalize a phase token for claim comparison: trim, and collapse an integer
+ * spelling to its canonical form so "01", "1" and " 1 " compare equal while
+ * decimals ("2.5") compare by their trimmed text.
+ */
+function normalizePhaseToken(value: string): string {
+  const trimmed = String(value).trim();
+  if (/^\d+$/.test(trimmed)) return String(parseInt(trimmed, 10));
+  return trimmed;
+}
+
+function sameSession(a: string | null, b: string | null): boolean {
+  // A null key is "unknown", not "equal" — two headless sessions must still
+  // conflict across phases (the CI/cron shape of #3311).
+  if (a === null || b === null) return false;
+  return a === b;
+}
+
+function parseClaim(raw: string | null | undefined): MilestoneClaim | null {
+  if (!raw) return null;
+  try {
+    const parsed = JSON.parse(raw) as Partial<MilestoneClaim>;
+    if (typeof parsed.phase !== 'string' || !parsed.phase.trim()) return null;
+    return {
+      phase: parsed.phase,
+      session: typeof parsed.session === 'string' ? parsed.session : null,
+      pid: typeof parsed.pid === 'number' ? parsed.pid : 0,
+      updated_at: typeof parsed.updated_at === 'number' ? parsed.updated_at : 0,
+    };
+  } catch {
+    // Corrupt body (concurrent partial write, hand edit) — treat as no claim.
+    // The next writer replaces the file; never crash a state command over an
+    // advisory sidecar.
+    return null;
+  }
+}
+
+function readClaim(cwd: string): MilestoneClaim | null {
+  try {
+    return parseClaim(fs.readFileSync(milestoneLockPath(cwd), 'utf-8'));
+  } catch {
+    return null; // absent or unreadable — same posture as parseClaim
+  }
+}
+
+function writeClaim(cwd: string, claim: MilestoneClaim): void {
+  ensureClaimDir(cwd);
+  fs.writeFileSync(milestoneLockPath(cwd), JSON.stringify(claim, null, 2) + '\n');
+}
+
+// planningWorkspace.planningDir targets .planning (or the workstream-scoped
+// variant); the parent is created by every GSD command that gets this far, but
+// the claim write must not throw if it is somehow missing.
+function ensureClaimDir(cwd: string): void {
+  try {
+    fs.mkdirSync(path.dirname(milestoneLockPath(cwd)), { recursive: true });
+  } catch {
+    /* best-effort — see writeClaim callers' advisory posture */
+  }
+}
+
+function isClaimLive(claim: MilestoneClaim, nowMs: number): boolean {
+  return nowMs - claim.updated_at < MILESTONE_LOCK_TTL_MS;
+}
+
+function currentSessionKey(): string | null {
+  return activeWorkstreamStore.getWorkstreamSessionKey();
+}
+
+function toConflict(claim: MilestoneClaim, phase: string, session: string | null): MilestoneConflict {
+  return {
+    locked_phase: claim.phase,
+    locked_session: claim.session,
+    phase,
+    session,
+  };
+}
+
+/**
+ * Human-visible warning for a conflict. Emitted on stderr unconditionally —
+ * visibility is the entire point of this lock (#3311's "silent last-write-wins").
+ */
+export function warnMilestoneConflict(conflict: MilestoneConflict, action: string): void {
+  const holder = conflict.locked_session ?? 'an unknown (headless) session';
+  const actor = conflict.session ?? 'an unknown (headless) session';
+  process.stderr.write(
+    `[gsd-tools] WARNING: milestone lock conflict (#3311): ${holder} holds the milestone claim for phase ` +
+      `${conflict.locked_phase}, but ${actor} is running ${action} for phase ${conflict.phase}. ` +
+      `STATE.md's ## Current Position is a single slot — concurrent phases overwrite each other's ` +
+      `position. Verify Current Position before trusting it.\n`,
+  );
+}
+
+/**
+ * begin-phase entry point: claim `phase` for this session.
+ *
+ * Returns a conflict descriptor (and leaves the existing claim intact) when a
+ * live claim for a DIFFERENT phase is held by a DIFFERENT session; otherwise
+ * takes/refreshes the claim and returns null.
+ */
+export function claimMilestonePhase(cwd: string, phase: string, nowMs?: number): MilestoneConflict | null {
+  const now = nowMs ?? realClock.now();
+  const session = currentSessionKey();
+  const existing = readClaim(cwd);
+  if (
+    existing !== null &&
+    isClaimLive(existing, now) &&
+    normalizePhaseToken(existing.phase) !== normalizePhaseToken(phase) &&
+    !sameSession(existing.session, session)
+  ) {
+    return toConflict(existing, phase, session);
+  }
+  writeClaim(cwd, { phase: String(phase).trim(), session, pid: process.pid, updated_at: now });
+  return null;
+}
+
+/**
+ * advance-plan entry point: `positionPhase` is the phase STATE.md's Current
+ * Position currently names.
+ *
+ * A live claim naming a DIFFERENT phase means the position was moved away from
+ * the claimed phase without a matching begin-phase — the #3311 flip — and is
+ * returned as a conflict REGARDLESS of session: this session's own legitimate
+ * re-targeting goes through begin-phase, which keeps the claim in sync, so a
+ * same-session mismatch is exactly as anomalous as a cross-session one. A live
+ * claim matching the position phase is heartbeat-refreshed (this session is
+ * actively working that phase, so the claim stays live). No claim is ever
+ * created here: advance-plan has no phase argument of its own, so it can only
+ * corroborate or contradict an existing claim, not originate one.
+ */
+export function checkMilestonePosition(cwd: string, positionPhase: string, nowMs?: number): MilestoneConflict | null {
+  const now = nowMs ?? realClock.now();
+  const existing = readClaim(cwd);
+  if (existing === null || !isClaimLive(existing, now)) return null;
+  const session = currentSessionKey();
+  if (normalizePhaseToken(existing.phase) === normalizePhaseToken(positionPhase)) {
+    // Heartbeat — keep the claim's liveness anchored to actual activity.
+    writeClaim(cwd, { phase: existing.phase, session: existing.session, pid: process.pid, updated_at: now });
+    return null;
+  }
+  return toConflict(existing, positionPhase, session);
+}
+
+/**
+ * phase.complete entry point (read-only check): a live claim for a DIFFERENT
+ * phase held by a DIFFERENT session is a conflict. phase.complete never claims
+ * — it ends work on a phase rather than starting it.
+ */
+export function checkMilestoneConflictForPhase(cwd: string, phase: string, nowMs?: number): MilestoneConflict | null {
+  const now = nowMs ?? realClock.now();
+  const existing = readClaim(cwd);
+  if (existing === null || !isClaimLive(existing, now)) return null;
+  if (normalizePhaseToken(existing.phase) === normalizePhaseToken(phase)) return null;
+  const session = currentSessionKey();
+  if (!sameSession(existing.session, session)) {
+    return toConflict(existing, phase, session);
+  }
+  return null;
+}
+
+/**
+ * Release the claim when the phase it names completes — regardless of which
+ * session completes it (an orchestrator cleaning up after a dead session must
+ * not be blocked by the dead session's own claim). Returns whether a matching
+ * claim was removed.
+ */
+export function releaseMilestonePhase(cwd: string, phase: string): boolean {
+  const existing = readClaim(cwd);
+  if (existing === null) return false;
+  if (normalizePhaseToken(existing.phase) !== normalizePhaseToken(phase)) return false;
+  try {
+    fs.unlinkSync(milestoneLockPath(cwd));
+    return true;
+  } catch {
+    return false; // already gone — same outcome for the caller
+  }
+}

src/phase.cts

@@ -83,6 +83,8 @@ const { computeHaltPropagation, buildSummaryFileIndex, isSummaryFileHalted } = p
 
 const { planningDir, withPlanningLock, listAvailableWorkstreams, getActiveWorkstream } =
   planningWorkspace;
+// eslint-disable-next-line @typescript-eslint/no-require-imports -- milestone-lock.cjs is an export= CommonJS module
+import milestoneLockMod = require('./milestone-lock.cjs');
 const { extractFrontmatter } = frontmatterMod;
 const {
   readModifyWriteStateMd,
@@ -2248,7 +2250,28 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
   let nextPhaseName: string | null = null;
   let isLastPhase = true;
 
+  // #3311: typed conflict descriptor surfaced on the result JSON alongside the
+  // warnings[] entry below (same parity pattern as
+  // verification_stale_check_indeterminate).
+  let milestoneConflict: milestoneLockMod.MilestoneConflict | null = null;
+
   const verificationBlocked = withPlanningLock(cwd, () => {
+    // #3311: completing a phase while a live milestone claim (phase + session)
+    // holds a DIFFERENT phase means two sessions are working two phases against
+    // the single Current Position slot. Warn via the established warnings[]
+    // channel (rendered by execute-phase.md's "If has_warnings is true" step)
+    // rather than blocking — the claim may simply be stale-but-live.
+    milestoneConflict = milestoneLockMod.checkMilestoneConflictForPhase(cwd, phaseNum);
+    if (milestoneConflict) {
+      const holder = milestoneConflict.locked_session ?? 'an unknown (headless) session';
+      const actor = milestoneConflict.session ?? 'an unknown (headless) session';
+      warnings.push(
+        `milestone lock conflict (#3311): ${holder} holds the milestone claim for phase ` +
+          `${milestoneConflict.locked_phase}, but ${actor} is completing phase ${phaseNum} — ` +
+          `STATE.md's Current Position is a single slot; verify it before trusting it`,
+      );
+      milestoneLockMod.warnMilestoneConflict(milestoneConflict, `phase.complete ${phaseNum}`);
+    }
     // #2617: pass the project's runtime so the blocked-completion error below
     // suggests the command surface this runtime actually installs
     // ($gsd-… on Codex) rather than a hard-coded Claude-style string.
@@ -2969,6 +2992,11 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
     } else {
       runPhaseCompleteTransaction();
     }
+    // #3311: a successful completion of the CLAIMED phase releases the
+    // milestone claim — regardless of which session completes it (an
+    // orchestrator cleaning up after a dead session must not be blocked by the
+    // dead session's own claim). No-ops when the claim names another phase.
+    milestoneLockMod.releaseMilestonePhase(cwd, phaseNum);
     return null;
   });
 
@@ -3027,6 +3055,7 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void {
     warnings,
     has_warnings: warnings.length > 0,
     verification_stale_check_indeterminate: staleCheckIndeterminate,
+    milestone_conflict: milestoneConflict,
   };
 
   output(result, raw);

src/state.cts

@@ -47,6 +47,11 @@ import stateTransitionMod = require('./state-transition.cjs');
 // #2573 D5: used to pin `git rev-parse` to the project's own repo. Imports only
 // node builtins, so it introduces no cycle on this path.
 import { findProjectRoot } from './project-root.cjs';
+// #3311: advisory (phase, session) claim over the single Current Position slot.
+// Imports only node builtins + planning-workspace + active-workstream-store, so
+// it introduces no cycle on this path.
+// eslint-disable-next-line @typescript-eslint/no-require-imports
+import milestoneLockMod = require('./milestone-lock.cjs');
 const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } = stateTransitionMod;
 type StateTransitionIntent = stateTransitionMod.StateTransitionIntent;
 type StateTransitionDeps = stateTransitionMod.StateTransitionDeps;
@@ -592,7 +597,25 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
   };
 
   let resultData: Record<string, unknown> | undefined;
+  // #3311: the milestone (phase + session) claim is consulted INSIDE the
+  // STATE.md lock, so the position read and the claim read cannot interleave
+  // with another session's Current Position write.
+  let milestoneConflict: milestoneLockMod.MilestoneConflict | null = null;
   readModifyWriteStateMd(statePath, (content) => {
+    // advance-plan has no phase argument of its own — the phase it advances is
+    // whatever ## Current Position names. Compare that against the milestone
+    // claim: a mismatch means another session moved the single-slot position
+    // away from the claimed phase (the #3311 flip) and must be surfaced, not
+    // silently absorbed.
+    const body = stripFrontmatter(content);
+    const positionScope = matchCurrentPositionSection(body) ?? body;
+    const positionPhase = parseProsePhaseField(stateExtractField(positionScope, 'Phase')).phase;
+    if (positionPhase !== null) {
+      milestoneConflict = milestoneLockMod.checkMilestonePosition(cwd, positionPhase);
+      if (milestoneConflict) {
+        milestoneLockMod.warnMilestoneConflict(milestoneConflict, 'state.advance-plan');
+      }
+    }
     const result = transitionCore(content, intent, deps);
     resultData = result.data;
     return result.content;
@@ -604,9 +627,9 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
   }
 
   if (resultData['advanced'] === false) {
-    output(resultData, raw, 'false');
+    output({ ...resultData, milestone_conflict: milestoneConflict }, raw, 'false');
   } else {
-    output(resultData, raw, 'true');
+    output({ ...resultData, milestone_conflict: milestoneConflict }, raw, 'true');
   }
 }
 
@@ -2891,7 +2914,17 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName
     authoritativeFm: intent.phaseName ? { current_phase_name: intent.phaseName } : undefined,
   };
   let updated: string[] = [];
+  // #3311: begin-phase is the claim point — it is the one Current Position
+  // transition that explicitly names its phase, so it both records this
+  // session's claim and detects a conflicting live claim for a different
+  // phase. The check runs INSIDE the STATE.md lock so concurrent begin-phase
+  // calls cannot both read "no claim" and both write.
+  let milestoneConflict: milestoneLockMod.MilestoneConflict | null = null;
   readModifyWriteStateMd(statePath, (content) => {
+    milestoneConflict = milestoneLockMod.claimMilestonePhase(cwd, String(phaseNumber));
+    if (milestoneConflict) {
+      milestoneLockMod.warnMilestoneConflict(milestoneConflict, `state.begin-phase ${phaseNumber}`);
+    }
     const result = transitionCore(content, intent, deps);
     updated = result.updated;
     // #3127 resume: the core preserved the mid-flight Current Phase Name, so
@@ -2904,7 +2937,11 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName
     return result.content;
   }, cwd, rmwOptions);
 
-  output({ updated, phase: phaseNumber, phase_name: phaseName || null, plan_count: planCount || null }, raw, updated.length > 0 ? 'true' : 'false');
+  output(
+    { updated, phase: phaseNumber, phase_name: phaseName || null, plan_count: planCount || null, milestone_conflict: milestoneConflict },
+    raw,
+    updated.length > 0 ? 'true' : 'false',
+  );
 }
 
 /**