fix(ship): review completed same-head salvage

src/channels/adminCoordinator.test.ts

@@ -7,7 +7,14 @@ import { NO_GRANTS, type Grants } from "../core/authz/types.js";
 import { InMemoryCoordinatorInstanceStore } from "../core/coordinator/instanceStore.js";
 import type { CoordinatorInstance, CoordinatorTag, CoordinatorUnit } from "../core/coordinator/contract.js";
 import type { ChildContract } from "../core/ship/contract.js";
-import type { RoundChecks } from "../core/ship/coordinator.js";
+import {
+  applyReturn,
+  nextAction,
+  openUnitPipeline,
+  type ChildFacts,
+  type RoundChecks,
+  type StepReturn,
+} from "../core/ship/coordinator.js";
 import type { DispatchOutcome } from "../core/dispatch/outcome.js";
 import { REPLAY_EVERYTHING, RunRegistry } from "../core/runRegistry.js";
 import { InMemoryRunStore } from "../core/runStore.js";
@@ -1297,6 +1304,104 @@ describe("POST /admin/coordinator/read-record — the renewal's facts off the re
     expect(unpriced.run.pushed).toBeUndefined();
   });
 
+  const salvageScenario = async (observedHead: string, salvageHead: string) => {
+    const branch = INSTANCE.branch;
+    const h = harness({ openPr: { number: 77, htmlUrl: "https://github.com/acme/api/pull/77", created: true } });
+    const child = h.registry.create("coding · child", {
+      agent: "coding",
+      channelId: INSTANCE.channelId,
+      userId: INSTANCE.userId,
+      threadKey: INSTANCE.threadKey,
+      ...TAG,
+    });
+    h.registry.finish(child.id, "completed");
+    const runId = child.id;
+    await h.instances.put(INSTANCE);
+    await h.instances.putUnits([
+      {
+        instanceId: INSTANCE.id,
+        unit: "U12",
+        slug: "u12",
+        title: "Warm the cache on wake",
+        branch,
+        dependsOn: [],
+        rounds: [],
+      },
+    ]);
+    await h.store.put(
+      record(runId, {
+        ...TAG,
+        handoff: { deviations: [], followUps: [], unproven: [] },
+        pushed: [{ ref: branch, sha: salvageHead, by: "salvage" }],
+        headSha: observedHead,
+      }),
+    );
+    h.registry.markPersisted(runId);
+
+    const driver = {
+      state: openUnitPipeline(
+        {
+          unit: { id: "U12", branch },
+          repo: INSTANCE.repo,
+          base: INSTANCE.base!,
+          caps: { maxRounds: 3, maxMinutes: 240 },
+          merge: "person",
+          generated: false,
+        },
+        NOW - 60_000,
+      ),
+    };
+    const feed = (answer: Record<string, unknown>) => {
+      const action = nextAction(driver.state);
+      if (action.type === "end") throw new Error("the unit ended before the scripted answer");
+      driver.state = applyReturn(driver.state, { ...answer, step: action.step } as StepReturn).state;
+    };
+    feed({ type: "pr-check", pr: { state: "none" }, at: NOW - 60_000 });
+    feed({ type: "branch", ok: true, at: NOW - 59_000 });
+    feed({ type: "spawn", outcome: "spawned", runId, at: NOW - 58_000 });
+    feed({ type: "wait", outcome: "event" });
+
+    const read = await handleCoordinatorRequest(
+      post(`${COORDINATOR_ADMIN_PREFIX}read-record`, { parentInstanceId: INSTANCE.id, runId, unit: "U12" }),
+      h.deps,
+    );
+    expect(read.status).toBe(200);
+    const run = (read.body as { run: ChildFacts }).run;
+    expect(run).toMatchObject({ headSha: observedHead, handoff: true, pushed: [{ sha: salvageHead, by: "salvage" }] });
+    feed({ type: "read-record", run, at: NOW });
+    return { h, driver, feed, runId };
+  };
+
+  it("a completed child's production read-record view carries its observed final head, so same-head salvage opens the pull request and enters review", async () => {
+    const HEAD = "a".repeat(40);
+    const { h, driver, feed, runId } = await salvageScenario(HEAD, HEAD);
+    const recover = nextAction(driver.state);
+    expect(recover).toMatchObject({ type: "pr-check", recover: { runId } });
+    if (recover.type !== "pr-check") throw new Error("expected the pull-request recovery step");
+    const opened = await handleCoordinatorRequest(
+      post(`${COORDINATOR_ADMIN_PREFIX}pr-check`, {
+        parentInstanceId: INSTANCE.id,
+        unit: "U12",
+        ...(recover.recover !== undefined ? { recover: recover.recover } : {}),
+      }),
+      h.deps,
+    );
+    expect(h.opens).toHaveLength(1);
+    expect(opened.body).toMatchObject({ ok: true, state: "open", prNumber: 77 });
+    feed({ type: "pr-check", pr: opened.body, at: NOW });
+    expect(nextAction(driver.state)).toMatchObject({
+      type: "spawn",
+      preset: "review",
+      round: { index: 1, kind: "review" },
+    });
+  });
+
+  it("a completed child whose salvage differs from the head returned by production read-record still aborts at the checkpoint", async () => {
+    const { h, driver } = await salvageScenario("b".repeat(40), "a".repeat(40));
+    expect(nextAction(driver.state)).toMatchObject({ type: "end", ending: { kind: "aborted" } });
+    expect(h.opens).toHaveLength(0);
+  });
+
   // Issue 1932: the failure by name rides the answer, so the machine can tell
   // a provider transient from the child failing on its task.
   it("answers a failed child's failure by name (`provider_transient`), and leaves the field off a record without one", async () => {

src/channels/adminCoordinator.ts

@@ -539,6 +539,8 @@ export interface CoordinatorRunView {
   /** The generation driving a live run elsewhere (run-history item 41). */
   ownerGen?: string;
   finalReply?: string;
+  /** The final Git head independently observed by the run loop. */
+  headSha?: string;
 }
 
 type Parsed<T> = { ok: true; value: T } | { ok: false; error: string };
@@ -1025,6 +1027,7 @@ function coordinatorRunView(
     ...(view.idempotencyKey !== undefined ? { idempotencyKey: view.idempotencyKey } : {}),
     ...(view.ownerGen !== undefined ? { ownerGen: view.ownerGen } : {}),
     ...(finalReply !== undefined ? { finalReply } : {}),
+    ...(view.headSha !== undefined ? { headSha: view.headSha } : {}),
   };
 }
 

src/core/dispatch/record.test.ts

@@ -226,6 +226,15 @@ describe("assembleRunRecord — the handoff on the record", () => {
     for (const key of ["verdict", "reviewHead", "dispositions"]) expect(key in record).toBe(false);
   });
 
+  it("carries the independently observed final workspace head, validates it, and omits it when the workspace had no head", () => {
+    const headSha = "a1b2c3d4e5f60718293a4b5c6d7e8f9012345678";
+    const record = assembleRunRecord({ ...base(), headSha });
+    expect(record).toMatchObject({ headSha });
+    expect(isRunRecord(JSON.parse(JSON.stringify(record)))).toBe(true);
+    expect("headSha" in assembleRunRecord(base())).toBe(false);
+    expect(isRunRecord({ ...record, headSha: "not-a-commit" })).toBe(false);
+  });
+
   it("carries the decision-record reservation and direct-task key from run_meta", () => {
     const record = assembleRunRecord({
       ...base(),

src/core/dispatch/record.ts

@@ -310,6 +310,9 @@ export function assembleRunRecord(input: {
    *  and seal are appended, the published total takes the larger count, and the
    *  two seal stamps ride the record — omitted when the seal has none. */
   seal?: SealResult;
+  /** The final workspace head the run loop observed independently of the
+   *  model and push events. Omitted when this run had no readable Git head. */
+  headSha?: string;
   /** The typed handoff the run submitted (docs/reference/specs/agent-ship.md item 14),
    *  as the tool accepted it; redacted HERE, the one assembly, so no caller
    *  can forget. Omitted (not set undefined) when the run submitted none. */
@@ -420,6 +423,7 @@ export function assembleRunRecord(input: {
     ...(referencesOfEvents(events).length > 0 ? { references: referencesOfEvents(events) } : {}),
     ...(msg.sourceUrl !== undefined ? { sourceUrl: msg.sourceUrl } : {}),
     ...(msg.userName !== undefined ? { userName: msg.userName } : {}),
+    ...(input.headSha !== undefined ? { headSha: input.headSha } : {}),
     ...(input.handoff !== undefined ? { handoff: redactHandoff(input.handoff) } : {}),
     ...(input.verdict !== undefined ? { verdict: redactVerdict(input.verdict) } : {}),
     ...(input.reviewHead !== undefined ? { reviewHead: input.reviewHead } : {}),
@@ -572,6 +576,8 @@ export interface FinishRecordContext {
   diagnosis: FrictionDiagnosis;
   root: Span;
   ledgerRun: LedgerRun | undefined;
+  /** The final Git head the run loop observed after its tail settled. */
+  headSha?: string;
   /** The handoff the run loop captured from `submit_handoff`, when one was submitted. */
   handoff?: Handoff;
   /** The verdict a review run submitted and the head it reviewed; the dispositions a fix round submitted. */
@@ -617,6 +623,7 @@ export function registerFinishRecord(deps: RecordDeps, ctx: FinishRecordContext)
     diagnosis,
     root,
     ledgerRun,
+    headSha,
     handoff,
     verdict,
     reviewHead,
@@ -648,6 +655,7 @@ export function registerFinishRecord(deps: RecordDeps, ctx: FinishRecordContext)
           status: failedAfterFinish && status === "completed" ? "failed" : status,
           diagnosis,
           seal,
+          ...(headSha !== undefined ? { headSha } : {}),
           ...(handoff !== undefined ? { handoff } : {}),
           ...(verdict !== undefined ? { verdict } : {}),
           ...(reviewHead !== undefined ? { reviewHead } : {}),

src/core/dispatch/runLoop.test.ts

@@ -1091,7 +1091,7 @@ describe("runLoop — the model turn and everything that rides on it", () => {
     s.ending.drain(undefined);
     await s.writer.settled();
     const rec = (await s.store.get("run-l"))!;
-    expect(rec.pushed).toEqual([{ ref: BRANCH, sha: HEAD, by: "salvage" }]);
+    expect(rec).toMatchObject({ headSha: HEAD, pushed: [{ ref: BRANCH, sha: HEAD, by: "salvage" }] });
     expect(rec.events).not.toContainEqual(expect.objectContaining({ type: "run_note", kind: "work_left_behind" }));
   });
 

src/core/dispatch/runLoop.ts

@@ -1921,6 +1921,7 @@ export async function runLoop(deps: RunDeps, ctx: RunLoopContext): Promise<RunLo
       diagnosis,
       root,
       ledgerRun,
+      ...(observedHead !== undefined ? { headSha: observedHead } : {}),
       ...(handoff !== undefined ? { handoff } : {}),
       ...(verdict !== undefined ? { verdict } : {}),
       ...(reviewHead !== undefined ? { reviewHead } : {}),

src/core/runRecord.ts

@@ -159,6 +159,11 @@ export interface RunRecord {
   record?: string;
   /** Stable text-free key for a direct task's re-issue. */
   recordTaskKey?: string;
+  /** The final workspace head the run loop independently observed (7 to 40
+   *  lowercase hex), after every tail step and mechanical salvage. Present
+   *  only when the workspace had a readable Git HEAD; absent on older records
+   *  and runs without a Git workspace. */
+  headSha?: string;
   /** The typed handoff a coding child submitted (docs/reference/specs/agent-ship.md
    *  item 14): its deviations from the plan unit, its follow-ups and the
    *  criteria it could not prove — redacted like every stored string. Present
@@ -1036,6 +1041,7 @@ export function isRunRecord(v: unknown): v is RunRecord {
   if (r.record !== undefined && (typeof r.record !== "string" || !/^\d{4}$/.test(r.record))) return false;
   if (r.recordTaskKey !== undefined && (typeof r.recordTaskKey !== "string" || !/^[0-9a-f]{16}$/.test(r.recordTaskKey)))
     return false;
+  if (r.headSha !== undefined && (typeof r.headSha !== "string" || !REVIEW_HEAD_PATTERN.test(r.headSha))) return false;
   // The handoff is checked for shape, not bounds (docs/reference/specs/agent-ship.md
   // item 14): redaction may lengthen a stored string past the tool's limit.
   if (r.handoff !== undefined && !isHandoffShape(r.handoff)) return false;

src/core/runsService.ts

@@ -174,10 +174,12 @@ export interface RunView {
    *  on a run without ship facts and on records written before the field. */
   pipeline?: PipelineSummary;
   /** The typed artifacts a finished run's record carries (run-history item 2) —
-   *  the review's verdict, reviewed head and post, the fix round's dispositions,
-   *  the coding child's handoff. A live view has none yet; a finished row the
-   *  registry still holds carries them from the store the moment the store
+   *  the final workspace head, the review's verdict, reviewed head and post,
+   *  the fix round's dispositions, and the coding child's handoff. A live view
+   *  has none yet; a finished row the registry still holds carries them from
+   *  the store the moment the store
    *  holds its record (`getRun`, item 21); a persisted row carries its own. */
+  headSha?: RunRecord["headSha"];
   verdict?: RunRecord["verdict"];
   reviewHead?: string;
   reviewPost?: RunRecord["reviewPost"];
@@ -808,6 +810,7 @@ export function createRunsService(deps: RunsServiceDeps): RunsService {
   ): Promise<
     Pick<
       RunView,
+      | "headSha"
       | "verdict"
       | "reviewHead"
       | "reviewPost"
@@ -831,6 +834,7 @@ export function createRunsService(deps: RunsServiceDeps): RunsService {
     }
     if (!row) return {};
     return {
+      ...(row.headSha !== undefined ? { headSha: row.headSha } : {}),
       ...(row.verdict !== undefined ? { verdict: row.verdict } : {}),
       ...(row.reviewHead !== undefined ? { reviewHead: row.reviewHead } : {}),
       ...(row.reviewPost !== undefined ? { reviewPost: row.reviewPost } : {}),

src/core/ship/coordinator.test.ts

@@ -1737,17 +1737,40 @@ describe("the transient re-run — round 0 dies on a provider transient with not
       finished({
         status: "failed",
         failure: { kind: "provider_transient" },
+        handoff: true,
+        headSha: HEAD_A,
         pushed: [{ ref: d.state.input.unit.branch, sha: HEAD_A, by: "salvage" }],
       }),
       T0 + 5 * MIN,
     );
     expect(d.action).toMatchObject({ type: "end", ending: { kind: "aborted" } });
     const report = renderUnitReport(d.state);
     expect(report).toContain(`the branch carries the interrupted work at \`${HEAD_A.slice(0, 7)}\``);
+    expect(report).toContain("the model provider's transport retry budget was spent");
     expect(report).toContain("The next reply in this thread resumes from that checkpoint");
     expect(report).not.toContain("Bad Gateway");
   });
 
+  it("a stopped child keeps its same-head salvage checkpoint even when it submitted a handoff", () => {
+    const d = fresh(input({ merge: "person" }));
+    d.answer({ type: "branch", ok: true, at: T0 });
+    runChild(
+      d,
+      "run-c0",
+      finished({
+        status: "stopped_soft",
+        handoff: true,
+        headSha: HEAD_A,
+        pushed: [{ ref: d.state.input.unit.branch, sha: HEAD_A, by: "salvage" }],
+      }),
+      T0 + 5 * MIN,
+    );
+    expect(d.action).toMatchObject({
+      type: "end",
+      ending: { kind: "stopped", checkpoint: { branch: d.state.input.unit.branch, sha: HEAD_A } },
+    });
+  });
+
   it("a completed coding child whose ending checkpoint reached the unit branch aborts with the resumable head", () => {
     const d = fresh(input({ merge: "person" }));
     d.answer({ type: "branch", ok: true, at: T0 });

src/core/ship/coordinator.ts

@@ -1211,6 +1211,10 @@ type Phase =
       childPushed?: PushedHeadFact[];
       childLeaseStartedAt?: number;
       childHandoff?: Handoff;
+      /** Open or recover a pull request from the branch. A completed child
+       *  whose same-head salvage merely republished ready work needs this path
+       *  without being classified as dead. */
+      recover?: true;
       /** The coding child died (`failed` or `interrupted`) after it may have
        *  pushed: the pr-check recovers a pushed branch by opening its pull
        *  request; with nothing pushed the unit ends with the child's own reason. */
@@ -1557,7 +1561,7 @@ export function nextAction(s: UnitPipelineState): CoordinatorAction {
       return {
         type: "pr-check",
         step: `${roundStep(s, p.round)}/pr-check`,
-        ...(p.dead !== undefined ? { recover: { runId: p.runId } } : {}),
+        ...(p.dead !== undefined || p.recover === true ? { recover: { runId: p.runId } } : {}),
         // The adopted pull request rides the check so the bot can follow it
         // when nothing heads the unit's branch (issue 1799).
         ...(s.pr !== undefined ? { pr: s.pr.number } : {}),
@@ -1810,14 +1814,37 @@ function nextReview(s: UnitPipelineState, notes: CoordinatorNote[] = []): Transi
 const stopMode = (status: RunStatus): "soft" | "hard" | undefined =>
   status === "stopped_soft" ? "soft" : status === "stopped_hard" ? "hard" : undefined;
 
+/** The last mechanical salvage push to this unit branch. */
+function salvagePush(s: UnitPipelineState, pushed: readonly PushedHeadFact[] | undefined): PushedHeadFact | undefined {
+  return pushed?.filter((p) => p.ref === s.input.unit.branch && p.by === "salvage").at(-1);
+}
+
+/** A completed child independently witnessed at the same head and carrying its
+ * handoff already declared the work ready; salvage merely republished its commit. */
+function completedSameHeadSalvage(
+  s: UnitPipelineState,
+  pushed: readonly PushedHeadFact[] | undefined,
+  child?: { status: RunStatus; handoff?: boolean; headSha?: string },
+): boolean {
+  if (child === undefined) return false;
+  const found = salvagePush(s, pushed);
+  const salvageHead = normalizeHead(found?.sha);
+  const childHead = normalizeHead(child.headSha);
+  return (
+    child.status === "completed" && child.handoff === true && salvageHead !== undefined && salvageHead === childHead
+  );
+}
+
 /** The last mechanical WIP push to this unit branch. Unlike an ordinary push,
- *  it says the child ended before its work was ready for review. */
+ * it says the child ended before its work was ready for review. */
 function interruptedCheckpoint(
   s: UnitPipelineState,
   pushed: readonly PushedHeadFact[] | undefined,
+  child?: { status: RunStatus; handoff?: boolean; headSha?: string },
 ): { branch: string; sha: string } | undefined {
-  const found = pushed?.filter((p) => p.ref === s.input.unit.branch && p.by === "salvage").at(-1);
-  return found ? { branch: found.ref, sha: found.sha } : undefined;
+  const found = salvagePush(s, pushed);
+  if (found === undefined || completedSameHeadSalvage(s, pushed, child)) return undefined;
+  return { branch: found.ref, sha: found.sha };
 }
 
 /** A coding run's confirmed end: round 0's child, or the run a findings step dispatched. */
@@ -1847,7 +1874,8 @@ function settleCoding(
         }
       : {}),
   };
-  const checkpoint = interruptedCheckpoint(next, facts.pushed);
+  const readySalvage = completedSameHeadSalvage(next, facts.pushed, facts);
+  const checkpoint = interruptedCheckpoint(next, facts.pushed, facts);
   const mode = stopMode(facts.status);
   if (mode !== undefined)
     return end(
@@ -1909,6 +1937,7 @@ function settleCoding(
       round,
       runId,
       ...(facts.headSha !== undefined ? { childHead: facts.headSha } : {}),
+      ...(readySalvage ? { recover: true as const } : {}),
       ...(facts.finalReply !== undefined ? { finalReply: facts.finalReply } : {}),
       ...(facts.pushed !== undefined ? { childPushed: facts.pushed } : {}),
       ...(facts.leaseStartedAt !== undefined ? { childLeaseStartedAt: facts.leaseStartedAt } : {}),