refactor(task-board): fence reviewer dispatch on ids, sign reviewer identity

apps/api/src/storage/task-board.ts

@@ -1185,7 +1185,8 @@ export class TaskBoardStorage {
    * the Super Agent, returning the updated item to the single winner and null
    * to everyone else. The auto-resolve reaction fires from two triggers that
    * can coincide — a reviewer's approval (`review-decision`) and the PR
-   * modal's poll (`prs-get`) — so, like `claimReviewer`, this fence must be
+   * modal's poll (`prs-get`) — so, like the reviewer dispatch's derived thread
+   * id, this fence must be
    * atomic: a read-then-write would let both dispatch a Super Agent run on the
    * same PR. The assignee re-check closes a second race: the caller's
    * assignee check runs against a read taken before this write, so a human
@@ -1466,67 +1467,12 @@ export class TaskBoardStorage {
       .execute();
   }
 
-  /**
-   * Atomically claim the (task, reviewer, cycle) slot, minting a token.
-   * `claimed` is false when the slot was already taken (a concurrent enqueue
-   * won the race) — the caller then skips enqueueing that reviewer. Either way
-   * returns the winning claim's `token`, which the reviewer run carries and
-   * echoes back to prove its identity when it records a decision.
-   */
-  async claimReviewer(
-    taskBoardItemId: string,
-    reviewer: string,
-    cycleAt: Date,
-  ): Promise<{ claimed: boolean; token: string }> {
-    const token = `rtok_${crypto.randomUUID()}`;
-    const inserted = await this.db
-      .insertInto("task_board_review_claims")
-      .values({
-        task_board_item_id: taskBoardItemId,
-        reviewer,
-        cycle_at: cycleAt,
-        token,
-      })
-      .onConflict((oc) =>
-        oc.columns(["task_board_item_id", "reviewer", "cycle_at"]).doNothing(),
-      )
-      .returning("token")
-      .executeTakeFirst();
-    if (inserted) return { claimed: true, token: inserted.token };
-    const existing = await this.db
-      .selectFrom("task_board_review_claims")
-      .select("token")
-      .where("task_board_item_id", "=", taskBoardItemId)
-      .where("reviewer", "=", reviewer)
-      .where("cycle_at", "=", cycleAt)
-      .executeTakeFirst();
-    return { claimed: false, token: existing?.token ?? token };
-  }
-
-  /**
-   * Release a reviewer's claim on a cycle — the counterpart to `claimReviewer`
-   * for when the dispatch it was minted for never actually ran (e.g. the
-   * enqueue itself threw). Without this, a transient dispatch failure leaves
-   * the slot permanently claimed with no thread behind it: `claimReviewer`'s
-   * unique (task, reviewer, cycle) key would refuse every retry for the rest
-   * of that review cycle, so that reviewer would simply never run.
-   */
-  async releaseReviewerClaim(
-    taskBoardItemId: string,
-    reviewer: string,
-    cycleAt: Date,
-  ): Promise<void> {
-    await this.db
-      .deleteFrom("task_board_review_claims")
-      .where("task_board_item_id", "=", taskBoardItemId)
-      .where("reviewer", "=", reviewer)
-      .where("cycle_at", "=", cycleAt)
-      .execute();
-  }
-
-  /** Resolve a review token to its claim (which reviewer, which cycle) for a
-   *  task. Null when the token doesn't belong to this task — used to verify
-   *  that a decision's caller really is the reviewer it claims to be. */
+  /** Resolve a legacy review token to its claim for a task. Null when the token
+   *  doesn't belong to this task.
+   *
+   *  LEGACY: claims are no longer written — reviewer identity is an HMAC now
+   *  (`tools/task-board/review-token.ts`). This only serves runs dispatched
+   *  before that deploy; delete it with the table once they've drained. */
   async resolveReviewClaimByToken(
     taskBoardItemId: string,
     token: string,

apps/api/src/tools/task-board/enqueue-reviewer.test.ts

@@ -7,11 +7,11 @@
 import { describe, expect, it, test } from "bun:test";
 import type { TaskBoardItem } from "@/storage/types";
 import {
-  hasSpentAttemptThisCycle,
   MAX_REVIEWER_ATTEMPTS,
   REVIEWER_DISALLOWED_TOOLS,
   reviewerAttemptsExhausted,
   reviewerHandledThisCycle,
+  spentAttemptsThisCycle,
 } from "./enqueue-reviewer";
 import { REVIEW_RUN_TOOL_NAMES } from "./task-run-context";
 
@@ -101,8 +101,7 @@ describe("reviewerHandledThisCycle", () => {
 
   // The bug: a FAILED reviewer thread satisfied "created this cycle", so when
   // both reviewers died on a database-connection timeout the card sat In Review
-  // with two dead reviewer threads, a claim row nothing released, and no
-  // verdicts. A failure is not a review.
+  // with two dead reviewer threads and no verdicts. A failure is not a review.
   it("is FALSE for a reviewer thread that FAILED this cycle — it gets retried", () => {
     const task = taskWith([
       thread({
@@ -112,15 +111,15 @@ describe("reviewerHandledThisCycle", () => {
       }),
     ]);
     expect(reviewerHandledThisCycle(task, "qa", CYCLE_START, NOW)).toBe(false);
-    expect(hasSpentAttemptThisCycle(task, "qa", CYCLE_START, NOW)).toBe(true);
+    expect(spentAttemptsThisCycle(task, "qa", CYCLE_START, NOW)).toBe(1);
   });
 
   /**
    * The deadlock. A reviewer whose pod dies mid-run keeps `in_progress`
    * forever: the per-pod idle reaper can't see it and `failNeverStartedThreads`
    * only covers runs that never started. Taking the status at face value made
-   * the thread own the cycle permanently — claim spent, nothing re-dispatched,
-   * and a merge gate waiting on a verdict that was never coming. One card sat
+   * the thread own the cycle permanently — nothing re-dispatched, and a merge
+   * gate waiting on a verdict that was never coming. One card sat
    * that way while its co-reviewer had approved in 68 seconds.
    */
   it("is FALSE for a non-terminal thread whose heartbeat went cold", () => {
@@ -133,8 +132,9 @@ describe("reviewerHandledThisCycle", () => {
       }),
     ]);
     expect(reviewerHandledThisCycle(task, "qa", CYCLE_START, NOW)).toBe(false);
-    // …and its claim row is released first, or the retry loses to the corpse.
-    expect(hasSpentAttemptThisCycle(task, "qa", CYCLE_START, NOW)).toBe(true);
+    // …and it counts as an attempt, so the retry fences on a fresh id
+    // instead of colliding with the corpse.
+    expect(spentAttemptsThisCycle(task, "qa", CYCLE_START, NOW)).toBe(1);
   });
 
   it("a warm heartbeat still owns the cycle — a slow reviewer is not a hung one", () => {
@@ -147,7 +147,7 @@ describe("reviewerHandledThisCycle", () => {
       }),
     ]);
     expect(reviewerHandledThisCycle(task, "qa", CYCLE_START, NOW)).toBe(true);
-    expect(hasSpentAttemptThisCycle(task, "qa", CYCLE_START, NOW)).toBe(false);
+    expect(spentAttemptsThisCycle(task, "qa", CYCLE_START, NOW)).toBe(0);
   });
 
   // The opposite mistake to the deadlock: a reviewer whose pod keeps dying must
@@ -223,7 +223,7 @@ describe("reviewerHandledThisCycle", () => {
         createdAt: "2026-01-01T09:30:00Z",
       }),
     ]);
-    expect(hasSpentAttemptThisCycle(task, "qa", CYCLE_START, NOW)).toBe(false);
+    expect(spentAttemptsThisCycle(task, "qa", CYCLE_START, NOW)).toBe(0);
   });
 
   it("scopes to the given reviewer — the other reviewer's thread and the Super Agent's don't count", () => {