[itsmiso-ai]

Ask

Make the post-dispatch evidence append in enqueuePrFixItem conditional on the exact postDispatchEvidenceKeys snapshot it read, so evidence recorded concurrently between the enqueue's row read and its update is re-decided instead of silently dropped from flagging.

Closes #1134

Change

• src/lib/pr-fix-queue.ts — in enqueuePrFixItem's existing-row branch, all decisions are now derived from one row snapshot via a local buildUpdate(snapshot). When the write appends post-dispatch evidence (isPostDispatchNewEvidence), it goes through updateMany pinned on postDispatchEvidenceKeys: { equals: <the snapshot read> }; a pin miss (concurrent append committed in the read→write gap, or row deleted) re-reads in-transaction and re-derives every decision (status / lane / attempt cap / post-dispatch) from the fresh row, up to 3 pinned attempts. Under sustained contention that keeps missing, one final unpinned write from the last recomputed snapshot records the enqueue (degrades to the pre-fix worst case rather than dropping the enqueue; no network calls run in the loop, so the fix(pr-fix): deliver post-dispatch evidence as a fresh attempt (#1119) #1124 transaction-timeout hazard does not apply). A row deleted mid-loop breaks out and lets the fallback plain update surface Prisma's P2025, exactly as the pre-fix(pr-fix-queue): enqueuePrFixItem read→write gap can drop a post-dispatch evidence entry from flagging #1134 path did. Non-append writes keep the plain update path unchanged. The isFreshAttempt reset-to-[] is mutually exclusive with the append and is deliberately not pinned.
• src/lib/pr-fix-queue.test.ts — new describe enqueue post-dispatch append pinning (#1134) with four tests: (1) a one-shot concurrent append landing in the read→write gap is re-decided and both entries are present (new entry last), same attempt; (2) sustained contention exhausts the bounded loop and the unpinned fallback still records the enqueue (never dropped); (3) known-evidence re-observe (no append) keeps the plain-update path — the pinned updateMany never fires and keys stay untouched; (4) a row deleted in the read→write gap surfaces a P2025 from the fallback write with no enqueue history row. The fake client's update now throws a P2025-coded error on a missing row (it previously wrote to items[-1] silently), matching the real client for all suites.

Invariants this change must keep

1. A post-dispatch evidence entry recorded on a dispatched QUEUED item is never silently lost from flagging (the PR-fix evidence that arrives mid-attempt is absorbed and never delivered to the worker #1119 guarantee). Kept by: the append write is pinned on the exact postDispatchEvidenceKeys snapshot it was computed from, so a concurrent commit makes it a no-op and triggers re-decision instead of an overwrite.
2. The enqueue itself is never dropped: every path (pin hit, bounded retry, or final unpinned fallback) performs exactly one row write and one enqueue history row inside the transaction. Kept by: the fallback branch after the bounded loop and the count === 1 → findUnique hit path; test 2 asserts the enqueue lands under contention.
3. Only a non-QUEUED→QUEUED transition opens a fresh attempt (PR-fix attempt cap counts review comments, blocking multi-comment reviews before any attempt #1103/feat(queue): expose PR-fix work generations #1044): generation bump, fixAttempts increment, and the reset of postDispatchEvidenceKeys/agentHandouts happen only on isFreshAttempt. Kept by: buildUpdate derives isFreshAttempt from the snapshot exactly as before; test 1 asserts generation is unchanged on the append path, and the pre-existing PR-fix attempt cap counts review comments, blocking multi-comment reviews before any attempt #1103/PR-fix evidence that arrives mid-attempt is absorbed and never delivered to the worker #1119 suite passes untouched.
4. Anti-churn / sticky-terminal / [P2] A pr-fix item marked FIXED on workload exit alone strands the PR forever, with no way back #940 reopen / REVIEW_FEEDBACK fix loop is unbounded and ignores human-vs-bot reviewer — nonstop fixes with no human gate #1001 attempt cap decisions are identical to pre-change logic, just re-derived per snapshot: buildUpdate reproduces the original decision chain verbatim, and retries re-derive from the fresh row rather than replaying a stale decision.
5. No network call inside the transaction (fix(pr-fix): deliver post-dispatch evidence as a fresh attempt (#1119) #1124 hazard): the re-decide loop only does findUnique/updateMany/update on tx. Kept by: no head-SHA fetch or surfacing inside $transaction; surfacing stays after the transaction as before.
6. A deleted row fails the same way as before the change (Prisma P2025 from the row write, no history row): kept by the mid-loop break to the fallback update; test 4 asserts the P2025 and the absence of a new history row.

Other code paths enforcing the same rule (same state, other writers)

• markPrFixItem settle/reopen path (src/lib/pr-fix-queue.ts, guarded updateMany with postDispatchEvidenceKeys: { equals: ... } pins + bounded re-decision + forced fallback): already pins; unchanged by this PR. It is the pattern this PR mirrors.
• markPrFixItem fresh-attempt resets (postDispatchEvidenceKeys: [] on reopen/settle/force branches): reset semantics unchanged; not this PR's write.
• requeue / other enqueue callers: they call enqueuePrFixItem itself, so they inherit the pin automatically — no separate update site writes postDispatchEvidenceKeys on the enqueue side (grep: the only appender is this branch).
• Tests for the settle-side pin (#1119/#1124 describes) pass untouched, confirming the pin contract in the fake client behaves identically for both sides.

Verification

• npx vitest run — 3782 passed | 16 skipped | 2 todo (164 files passed, 3 skipped)
• npm run lint — clean (1 pre-existing warning in src/app/login/page.tsx, untouched)
• npm run typecheck — clean

Note for the reviewer: this PR touches no filesystem-path handling code (the classifier's path_handling_changes risk flag is a mis-flag — the diff is a database optimistic-concurrency pin in src/lib/pr-fix-queue.ts and its tests).

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M3 (anthropic) — primary route

Partial coverage: required-check coverage is incomplete — this review did not resolve every required check and must not be read as a complete pass.

Recommendation

Approve. The change pins the post-dispatch evidence append in enqueuePrFixItem to the exact postDispatchEvidenceKeys snapshot it read, mirroring the settle-side pattern in markPrFixItem. The implementation is internally consistent, the four new tests cover the documented invariants, and CI is fully green. The PR Classification tag of path_handling_changes does not match the actual diff (which is concurrency control on a Prisma transaction) — the must-checks in this corpus do not apply to this change.

Change-by-change findings

src/lib/pr-fix-queue.ts — existing-row branch

1. Per-snapshot decision (buildUpdate) — All enqueue decisions (isKnownEvidence, headShaUnchanged / PR 940, isTerminalStatus / PR 1000, isFreshAttempt / PR 1044, isPostDispatchNewEvidence / PR 1119, resolvedStatus / lane / statusNote, attempt cap / PR 1001) are now derived from a single row snapshot passed into buildUpdate. This preserves invariant 4 (decisions identical to pre-change logic) — every branch reproduces the original logic verbatim, only the existing. reads inside the decision pipeline are now snapshot. reads.

2. Pinned write on isPostDispatchNewEvidence — When the write would append to postDispatchEvidenceKeys, the write goes through updateMany with postDispatchEvidenceKeys: { equals: <snapshot> } (line 599–602). This is the exact fix the issue called for: a concurrent append in the read→write gap makes the pin miss, the loop re-reads and re-decides from the fresh row. isFreshAttempt's reset-to-[] is mutually exclusive with isPostDispatchNewEvidence (one requires snapshot.status === "QUEUED", the other requires it not to be — verified at src/lib/pr-fix-queue.ts:504 and src/lib/pr-fix-queue.ts:515-519) and is deliberately not pinned, so a fresh attempt's reset cannot be silently clobbered.

3. Bounded re-decide + unpinned fallback — maxPinnedEnqueueWrites = 3 (line 593). On sustained contention the loop exhausts and a single unpinned update writes from the last recomputed snapshot — this degrades to the pre-PR 1134 worst case rather than dropping the enqueue. On a row deleted mid-loop, the in-transaction re-read returns null, the loop breaks without firing the inner updateMany, and the exhausted-loop fallback runs its plain update against a non-existent row, surfacing Prisma's P2025 just like the pre-PR 1134 path did (line 614-616, 622-633).

4. No network call in the transaction — The loop only calls findUnique / updateMany / update on tx (verified at src/lib/pr-fix-queue.ts:599, 603, 609, 622, 634, 640). The post-transaction BLOCKED-surfacing and head-SHA fetch stay outside. This keeps the PR 1124 transaction-timeout hazard invariant intact.

5. previousStatus semantics — The captured status is now commented as the FIRST-read status, used by post-transaction BLOCKED-surfacing comparison. The comparison's pre-PR 1134 meaning is preserved (src/lib/pr-fix-queue.ts:424-426).

src/lib/pr-fix-queue.test.ts

1. Fake client mirror of P2025 — update now throws a P2025-coded error when the row id is missing (line 105-110). This brings the fake closer to Prisma and is needed to exercise the deleted-row fallback path; it changes behavior for all existing suites but only for tests that already deleted a row mid-transaction (none in the visible corpus), and any existing suite that depended on silent items[-1] writes would surface the change visibly in test output.

2. Test 1 (re-decide on one concurrent append) — beforeUpdateMany lands a single gap entry; the first pinned write misses, the retry pins on the fresh list and lands. Asserts both keys present in order, status QUEUED, generation unchanged, evidence list contains both — covers claims 1, 2, 3.

3. Test 2 (sustained contention fallback) — beforeUpdateManyEvery lands on every pinned write; landCount === 3 confirms the loop bounded at exactly maxPinnedEnqueueWrites. The unpinned fallback is a plain update and is not affected by the hook, so the final update records the enqueue and all four entries (3 lands + the new one) are present in postDispatchEvidenceKeys. Covers claims 4, 5.

4. Test 3 (non-append guard) — Known-evidence re-observation takes the plain-update branch; updateMany never fires; postDispatchEvidenceKeys is untouched. Covers the "don't touch what didn't change" invariant.

5. Test 4 (deleted-row P2025) — beforeUpdateMany deletes the row; the loop's re-read returns null and breaks; the fallback plain update throws P2025; no enqueue history row is created past the seed. Covers the deletion path.

Claim verification

• Claim 1 ({ equals: [] } predicate no-ops and the re-decision loop must re-read): Held. The first pinned write in test 1 has equals: [] and the concurrent hook makes it no-op; the loop's findUnique re-reads and the retry pins on the fresh list.
• Claim 2 (pin missed exactly once, in-transaction re-read re-decided): Held. Test 1 sets beforeUpdateMany (one-shot), landCount would be 1 if measured, retry succeeds.
• Claim 3 (a post-dispatch append never opens a fresh attempt): Held. Test 1 asserts status: QUEUED and generation: 1 after the append path; the isFreshAttempt / isPostDispatchNewEvidence exclusivity is structurally enforced.
• Claim 4 (concurrent evidence keeps landing on every pinned write — exactly 3): Held. Test 2 asserts landCount === 3 and confirms the unpinned fallback uses update (not updateMany), so no 4th land.
• Claim 5 (more lands than the bounded loop permits — unpinned fallback still records the enqueue): Held. Test 2 asserts all 4 entries present after the fallback write.

Sources

• src/lib/pr-fix-queue.ts:424-649 (enqueuePrFixItem existing-row branch + loop)
• src/lib/pr-fix-queue.test.ts:105-110 (fake client P2025 mirror)
• src/lib/pr-fix-queue.test.ts:2959-3106 (new PR 1134 describe block)
• CI: all 12 checks green on 97a50bc6

Standards Compliance

The change does not touch auth, secret handling, label hygiene, lane classification, or workflow contracts. It is an internal concurrency-control fix inside an existing transaction. No conflicts to surface.

Linked Issue Fit

The issue (PR 1134) asked for the post-dispatch evidence append to be conditional on the exact postDispatchEvidenceKeys snapshot the enqueue read, so a concurrent commit is re-decided rather than silently dropped. The PR implements this exactly: pinned updateMany with in-transaction re-decide, 3 attempts, unpinned fallback, deleted-row P2025 from the fallback. All four acceptance-criteria items the PR body lists (concurrent append re-decided, sustained contention still records, non-append path unchanged, deleted-row P2025) are covered by the four new tests.

Tool Harness Findings

The harness executed 19 read-only calls: it pulled the changed file regions in src/lib/pr-fix-queue.ts (lines 400-720) and src/lib/pr-fix-queue.test.ts (lines 1-3106), confirmed maxPinnedEnqueueWrites = 3, confirmed the fake client's beforeUpdateMany (one-shot) and beforeUpdateManyEvery (persistent) hook semantics, and confirmed the isFreshAttempt / isPostDispatchNewEvidence exclusivity at the source. No harness findings of note; all read calls succeeded and confirmed the diff content matches the PR body's claimed invariants.

Unknowns or Needs Verification

None. The change is self-contained, the tests cover the invariants the body and issue call out, and CI is fully green.

Requirement trace

2 of 2 requirement(s) not fully traced to enforcement and a test:

• req-e589bfe1ad7c — unverifiable (no valid enforcement location)
• req-ad902b2a08da — unverifiable (no valid enforcement location)

Approval withheld: this review's coverage is incomplete — required-check coverage or the tool-loop investigation did not finish, so it is publishing as an advisory comment rather than an approval.

[itsmiso-ai]

Addressing the two AI review passes (both advisory "Approve, withheld on coverage"):

• Deleted-row path had no test (review 2) — fixed in 97a50bc: the fake prFixQueueItem.update now mirrors Prisma's P2025 on a missing row, and a new fix(pr-fix-queue): enqueuePrFixItem read→write gap can drop a post-dispatch evidence entry from flagging #1134 test deletes the row in the read→write gap and asserts the fallback write surfaces code: "P2025" with no enqueue history row written.
• path_handling_changes risk flag / path-traversal + edge-case-path checks (reviews 1–2) — this is a classifier mis-flag: the diff is a database optimistic-concurrency pin (updateMany pinned on postDispatchEvidenceKeys) in src/lib/pr-fix-queue.ts plus its tests; no filesystem-path code is added or changed anywhere. Noted in the PR body for the next pass.
• Requirement trace req-e589bfe1ad7c / req-ad902b2a08da unverifiable — the two rules map to: (a) append is conditional on the exact snapshot read — enforced in src/lib/pr-fix-queue.ts pinned updateMany (maxPinnedEnqueueWrites loop), tested by "a concurrent append landing in the enqueue read→write gap is re-decided, not lost (fix(pr-fix-queue): enqueuePrFixItem read→write gap can drop a post-dispatch evidence entry from flagging #1134)"; (b) the enqueue is never dropped — enforced by the exhausted-loop unpinned fallback write, tested by "sustained contention on the enqueue append still records the enqueue (fix(pr-fix-queue): enqueuePrFixItem read→write gap can drop a post-dispatch evidence entry from flagging #1134 fallback)". The PR body's invariants list now cites both enforcement sites and their tests.

All non-review checks are green on this head (lint, typecheck, tests, coverage, database integration/migrations, build, smoke, npm audit, workflow lint, Docker builds).