fix(task-board): reject a review token replayed from a stale review cycle

[pedrofrxncx]

Source: hardening follow-up to #5520 (QA Agent + Code Reviewer review flow), which introduced per-cycle review-claim tokens specifically to stop "one agent forging the two-reviewer sign-off." This closes a gap the same commit left open.

Why a maintainer wants it: TASK_BOARD_REVIEW_DECISION verified a reviewToken by reviewer identity only (claim.reviewer === reviewer) and never checked which review cycle the underlying claim was minted for. A token stays resolvable in the DB forever (resolveReviewClaimByToken looks it up by task+token only), so a token a reviewer saw in cycle 1 still verifies in cycle 2 — after a request_changes bounce sent the task back to the Super Agent, a fix was pushed, and the task re-entered In Review. Replaying the old token would mark a new approval as verified: true even though that reviewer never re-reviewed the updated PR, letting a stale sign-off count toward auto-merge.

The fix: verified now also requires claim.cycleAt to equal the task's current review-cycle start (reviewCycleStart over the activity log — the same reducer the claim-minting side already keys off). Extracted the check into a pure isTokenVerified(claim, reviewer, currentCycleAt) and added review-decision.test.ts covering: same-cycle match → verified; matching reviewer but stale cycle → NOT verified; wrong reviewer → NOT verified; no claim → NOT verified.

Reviewer command: bun test apps/api/src/tools/task-board/review-decision.test.ts

Locally verified: bun run fmt, cd apps/api && bunx tsc --noEmit (clean), the targeted test file above (4/4 pass), and bunx oxlint on both changed files (0 warnings/errors). Full CI validates the rest.

---

Summary by cubic

Rejects review tokens from prior review cycles so stale approvals can't verify in a new cycle. Ensures TASK_BOARD_REVIEW_DECISION only verifies approvals for the current cycle, protecting the auto-merge gate.

• Bug Fixes
  • Verify tokens by reviewer AND cycle: compare claim cycleAt to the current cycle start from reviewCycleStart(listActivity(...)); reject stale claims.
  • Added isTokenVerified(claim, reviewer, currentCycleAt) and used it in TASK_BOARD_REVIEW_DECISION.
  • Tests cover: same-cycle pass; stale cycle reject; wrong reviewer; missing claim.

^{Written for commit 93b7007. Summary will update on new commits.}

[cubic-dev-ai]

2 issues found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/api/src/tools/task-board/review-decision.ts">

<violation number="1" location="apps/api/src/tools/task-board/review-decision.ts:35">
P2: A stale token can still verify when the cycle boundary is missing: `reviewCycleStart` returns `0` when there is no `status_changed → in_review` activity, and this equality treats a claim minted with `cycleAt = 0` as valid for every later request that also sees `currentCycleAt = 0`. The reviewer enqueue path mints claims from that same fallback (`new Date(lastInReviewAt)`), while the status activity write is best-effort, so an activity-write failure can leave an old token reusable across re-review cycles. Failing closed when either cycle timestamp is the sentinel value would preserve the replay protection under this failure mode.</violation>

<violation number="2" location="apps/api/src/tools/task-board/review-decision.ts:140">
P1: A stale token can still be accepted in a race with a new review cycle because the cycle is read once from an activity snapshot before the claim is resolved and the decision is recorded. For example, this query can return cycle 1; a `request_changes` re-run can then enter In Review and append the cycle-2 boundary; the old claim is still compared against the captured cycle-1 value, and the subsequent approval is recorded after the cycle-2 boundary. `reviewCycleVerdicts` will then treat that newly recorded, `verified: true` approval as part of cycle 2, allowing it to contribute to auto-merge. The cycle check and recording need an atomic/current-cycle validation (or a conditional write that rejects when the boundary has changed), rather than relying on this one-time read.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: A stale token can still be accepted in a race with a new review cycle because the cycle is read once from an activity snapshot before the claim is resolved and the decision is recorded. For example, this query can return cycle 1; a request_changes re-run can then enter In Review and append the cycle-2 boundary; the old claim is still compared against the captured cycle-1 value, and the subsequent approval is recorded after the cycle-2 boundary. reviewCycleVerdicts will then treat that newly recorded, verified: true approval as part of cycle 2, allowing it to contribute to auto-merge. The cycle check and recording need an atomic/current-cycle validation (or a conditional write that rejects when the boundary has changed), rather than relying on this one-time read.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/tools/task-board/review-decision.ts, line 140:

<comment>A stale token can still be accepted in a race with a new review cycle because the cycle is read once from an activity snapshot before the claim is resolved and the decision is recorded. For example, this query can return cycle 1; a `request_changes` re-run can then enter In Review and append the cycle-2 boundary; the old claim is still compared against the captured cycle-1 value, and the subsequent approval is recorded after the cycle-2 boundary. `reviewCycleVerdicts` will then treat that newly recorded, `verified: true` approval as part of cycle 2, allowing it to contribute to auto-merge. The cycle check and recording need an atomic/current-cycle validation (or a conditional write that rejects when the boundary has changed), rather than relying on this one-time read.</comment>

<file context>
@@ -110,17 +130,29 @@ export const TASK_BOARD_REVIEW_DECISION = defineTool({
+    // never actually gave. An unverified decision is still recorded (so a
+    // dropped token never stalls the flow) but won't count toward an automatic
+    // merge (see the verified gate below).
+    const cycleAt = reviewCycleStart(
+      await ctx.storage.taskBoard.listActivity(taskBoardItemId, organizationId),
+    );
</file context>