Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion src/plugins/workflow/checkpoints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ describe("verifyDelegatedCheckpoint", () => {
});

test("records reviewer outcomes with call binding and rejects stale callbacks", async () => {
const { runId } = await startedGeneration("binding");
const { runId, reviewWorkItemId } = await startedGeneration("binding");
recordCheckpointReviewerLaunch(store, {
runId,
checkpointId: "CHECKPOINT-R-001",
Expand All @@ -404,6 +404,15 @@ describe("verifyDelegatedCheckpoint", () => {
expect(mismatch.ok).toBe(false);
if (!mismatch.ok) expect(mismatch.errorCode).toBe("CALLBACK_MISMATCH");

// A live launch keeps the reviewer in flight on the linked review item,
// so a relaunch attempt is still rejected while the reviewer works.
const tracked = store.beginTrackedLaunch({
sessionId: SESSION,
workItemId: reviewWorkItemId,
agent: "vv-code-reviewer",
});
expect(tracked.ok).toBe(true);

const duplicate = recordCheckpointReviewerLaunch(store, {
runId,
checkpointId: "CHECKPOINT-R-001",
Expand All @@ -414,6 +423,61 @@ describe("verifyDelegatedCheckpoint", () => {
if (!duplicate.ok) expect(duplicate.errorCode).toBe("ALREADY_LAUNCHED");
});

test("releases a provably dead reviewer binding after a transport-failed launch (issue #43)", async () => {
const { runId, reviewWorkItemId } = await startedGeneration("dead-binding");
recordCheckpointReviewerLaunch(store, {
runId,
checkpointId: "CHECKPOINT-R-001",
reviewer: "code",
callId: "rv-dead",
});
const tracked = store.beginTrackedLaunch({
sessionId: SESSION,
workItemId: reviewWorkItemId,
agent: "vv-code-reviewer",
});
expect(tracked.ok).toBe(true);
// The launch dies at transport: the tracked launch reverts the round and
// the reviewer leaves inFlightReviewers, but the checkpoint binding must
// not stay dead forever.
const reverted = store.revertReviewerLaunch({
sessionId: SESSION,
workItemId: reviewWorkItemId,
agent: "vv-code-reviewer",
});
expect(reverted.ok).toBe(true);

const relaunched = recordCheckpointReviewerLaunch(store, {
runId,
checkpointId: "CHECKPOINT-R-001",
reviewer: "code",
callId: "rv-fresh",
});
expect(relaunched.ok).toBe(true);

const recorded = recordCheckpointReviewerResult(store, {
runId,
checkpointId: "CHECKPOINT-R-001",
reviewer: "code",
callId: "rv-fresh",
status: "PASS",
});
expect(recorded.ok).toBe(true);
if (recorded.ok) expect(recorded.roundComplete).toBe(true);

// A completed review still rejects a relaunch of the same role (the
// original guard order — ALREADY_LAUNCHED before ALREADY_RECORDED — is
// preserved when a result is already recorded).
const afterResult = recordCheckpointReviewerLaunch(store, {
runId,
checkpointId: "CHECKPOINT-R-001",
reviewer: "code",
callId: "rv-3",
});
expect(afterResult.ok).toBe(false);
if (!afterResult.ok) expect(afterResult.errorCode).toBe("ALREADY_LAUNCHED");
});

test("FAIL settles the checkpoint as failed even after the review item closes", async () => {
const { runId, reviewWorkItemId } = await startedGeneration("fail");
const launched = store.beginTrackedLaunch({
Expand Down
36 changes: 31 additions & 5 deletions src/plugins/workflow/checkpoints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -927,11 +927,37 @@ export function recordCheckpointReviewerLaunchInStore(
};
}
if (active.review.reviewerCallIds[input.reviewer]) {
return {
ok: false,
errorCode: "ALREADY_LAUNCHED",
message: `ALREADY_LAUNCHED: ${input.reviewer} already launched for generation ${active.review.generation}`,
};
// A reviewer launch that dies at transport (a host-terminal error before
// any protocol report) used to leave its callId bound here forever: the
// tracked launch reverts the linked review item's round (the role leaves
// inFlightReviewers) but nothing released this binding, so the slot could
// never be relaunched — verify reports "incomplete" forever, recover
// refuses while the generation counts as live, and start refuses
// ALREADY_IN_REVIEW (issue #43). Release a provably dead binding: no
// result recorded for the role AND the linked review item's active round
// has the role out of flight. A live relaunch (role still in flight) and
// a completed review (result recorded) still reject exactly as before.
const linkedRecord = [...data.records.values()].find(
(entry) =>
entry.sessionId === active.run.sessionId &&
entry.workItemId === active.review.reviewWorkItemId,
);
const roleInFlight = Boolean(
linkedRecord?.currentRound &&
linkedRecord.currentRound.status === "active" &&
Array.isArray(linkedRecord.currentRound.inFlightReviewers) &&
linkedRecord.currentRound.inFlightReviewers.includes(input.reviewer),
);
const resultRecorded = Boolean(active.review.results[input.reviewer]);
if (!roleInFlight && !resultRecorded) {
delete active.review.reviewerCallIds[input.reviewer];
} else {
return {
ok: false,
errorCode: "ALREADY_LAUNCHED",
message: `ALREADY_LAUNCHED: ${input.reviewer} already launched for generation ${active.review.generation}`,
};
}
}
if (active.review.results[input.reviewer]) {
return {
Expand Down