Repository navigation
fix(queue): settle only the issued PR-fix attempt and start head - #1079
Conversation
Queue-backed followup-pr reports must echo the prFixItem.{id,generation}
token issued by next-task; stale or tokenless reports are skipped without
mutating the current queue row. Settlement is conditioned on the issued
generation via updateMany (commit-time revalidation), the head guard now
compares against the immutable per-attempt attemptHeadSha baseline captured
when the attempt becomes actionable, and FIXED is written only after the
guard passes — no more transient FIXED-then-QUEUED rollback clobbering a
concurrent re-enqueue. Admin marks keep compatibility; bearer/bridge marks
require an expected generation.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash@https://litellm.jory.dev/v1 (openai) — routed smart (risk match: db_or_migration_changes)
PR PR 1079 — fix(queue): settle only the issued PR-fix attempt and start head
Recommendation: Approve. This PR faithfully implements the PR 1074 acceptance criteria: attempt-token settlement, an immutable per-attempt head baseline, commit-time revalidation with no transient FIXED, explicit legacy-report semantics, and preserved operator mark compatibility — all documented in AGENTS.md and docs/pr-review-fix-queue.md and covered by a substantial test matrix. Findings below are non-blocking (stale result type, two untested edge branches, a client-suppliable baseline field worth a follow-up, and coverage gaps). No data-loss risk in the migration; CI (typecheck, lint, tests, migrations, DB integration) is green on the head commit.
Change-by-change findings
prisma/schema.prisma+ migration20260926000000_add_pr_fix_attempt_head_sha— Purely additive nullableTEXTcolumn, no default,IF NOT EXISTSguarded. No data loss risk; existing rows get NULL, which the code explicitly treats as "no baseline" (guard accepts / falls back to mutableheadSha). The migration comment correctly distinguishes the new baseline from the mutableheadShaused by the PR 940 reopen check. Placement in the schema and migration ordering (after20260921100000) are consistent.src/lib/pr-fix-queue.ts(core) —markPrFixItemnow loads the row before any write, runs the PR 940 head guard before any status write againstattemptHeadSha(fallbackheadSha), and routes all status writes through generation-conditionalupdateManyinside$transactionwith the history row — so a refused FIXED never transiently exists and a concurrent re-issue makes the write a no-op. GitHub calls (merge state, head SHA) happen outside any transaction, exactly as the issue requires.resolvePrFixFromAgentReportsettles only by token: token-missing item / repo-PR mismatch / stale generation / already-settled all skip with zero mutation; token-less legacy reports match but never settle and make no GitHub calls. Minor: the result interface still declaresitemId?: number | nullthough ids are now string CUIDs (existingisany, so typecheck won't catch it), and the legacy-skip return omitsattemptGenerationwhile sibling returns echo it as null.src/app/api/agents/[agentName]/tasks/report/route.ts— Validates the optionalprFixItem.{id, generation}token (object check, non-empty trimmed id, integer ≥ 1) with 400s before any DB work, trims, threads it into the settlement call asattempt(null when absent), and includes it in the report payload so idempotency replay treats a token change as a 409. Matches the documented contract.src/app/api/pr-fix-queue/mark/route.ts— Bearer marks now requiregeneration(400 without), operator paths keep unconditional behavior,generation-mismatchmaps to 409 with no audit write,not-foundstill 404s. Minor: on an operator path the 409 message would interpolate anundefinedexpected generation (only reachable if the refusal write matches zero rows); and the newly acceptedattemptHeadShais client-suppliable — a bearer caller could plant a fabricated baseline to later pass the no-progress guard. Bearer callers are already trusted with the settlement flow (the report path never acceptsattemptHeadSha), so this is a hardening question, not a blocker.- Tests — Strong matrix: in-flight enrichment baseline stability with feedback preservation, no-progress re-baselining, late/superseded tokens with zero writes, commit-time race injection, legacy baseline fallback both ways, no-record acceptance, mark-route bearer/operator generation enforcement, route validation matrix, and idempotency replay/persistence of resolutions. Gaps: no test for the refused-FIXED write matching zero rows, no test for the "attempt item not found" token path, and the conditional-write semantics rest on hand-written in-memory mocks rather than the real Postgres integration suite. Next-task route tests gained the mock plumbing but never assert
attemptHeadShaon the issued payload (thetoAgentQueuePrFixItemunit test does cover the mapping). - Docs (
AGENTS.md,docs/pr-review-fix-queue.md) — Accurately describe the implemented behavior, including the 400/409/404 semantics and legacy-report no-op.
Linked Issue Fit
Issue PR 1074 is a P1 bug with explicit "Done when" criteria; the PR meets all of them:
- Token-validated settlement, legacy reports defined — done: report validation, stale/mismatch skips without mutation, and an explicit legacy reason string ("queue settlement requires an attempt token"), matching the issue's "no implicit settlement of a newer queue generation by repo/PR alone".
- Immutable per-attempt head baseline — done:
attemptHeadShacaptured when the attempt becomes dispatchable; the enrich-in-flight test proves a push followed by an in-flight enqueue still settles the old attempt without erasing newer feedback. - Conditional, atomic writes; no transient FIXED; no DB transaction across GitHub calls; commit-time revalidation — done: guard-before-write,
updateMany({where:{id,generation}})+ history inside$transaction, GitHub calls outside transactions, race covered by a dedicated test. - Required test coverage — done for unchanged head/no-progress, enqueue during work, new review before/during settlement, late/duplicate reports incl. idempotency-key retry, and no mutation of newer/terminal states.
- Administrative mark compatibility + generation requirement for bridge marks — done exactly as the issue phrases it.
Deployment note (info): once merged, pre-token bridges/workers will see 400s on generation-less marks and token-less reports will no longer settle items — this is the issue's intent, but ship the courier-side token echo in the same rollout window.
Standards Compliance
Standards reviewed against .github/ai-review-rules.md and AGENTS.md:
- CI gate (lint/typecheck must pass) — satisfied: head-commit CI shows Typecheck and Lint both successful, alongside Tests, Coverage, Build, and Docker builds.
- Token secrecy (agent/GitHub tokens never logged/persisted) — satisfied: nothing in the diff logs or persists
DISPATCH_AGENT_TOKENorGITHUB_TOKEN; the mark-route audit entry records actor identity, not credentials; the prFixItem token is a queue coordinate, not a secret. - Validation before DB operations — followed: both new inputs (
prFixItem,generation/attemptHeadSha) are validated before any query. - Appropriate HTTP status codes — followed: 400 validation, 404 not-found, 409 conflict with no mutation, 200 with informative skipped resolutions.
- Prisma conventions (strict relations, no nullable-FK hedging) — followed: the new column is a nullable scalar baseline, not a foreign key, and its NULL semantics are documented and deliberate.
Must-check items
- Review migration for data loss risk — verified from the SQL: single
ADD COLUMN IF NOT EXISTS "attemptHeadSha" TEXTonPrFixQueueItem; nullable, no default, no backfill, no rewrite — no data loss or locking concern, and NULL rows get the documented legacy "guard accepts" behavior. - Test migration on a copy of production schema — I could not run a staging migration myself in this review; the best available evidence is CI's "Database migrations" and "Database integration" checks, both successful on the head commit against a real Postgres service. Given the migration is a single additive nullable column, residual risk is negligible, but an operator deploying to a large production table should still see
prisma migrate deployapply it cleanly at startup as usual.
Tool Harness Findings
8 tool calls were executed (repository review rules read; issues PR 1074 and PR 940 fetched; src/lib/pr-fix-queue.ts read in five ranges covering the mark, enqueue, requeue, and settlement paths). Issue PR 1074's body confirms the acceptance criteria quoted above, and PR 940 confirms the no-progress guard context the rebuilt baseline protects. Direct reads of pr-fix-queue.ts confirmed the stale itemId type, the legacy-skip shape, the guard-before-write ordering, the generation-conditional updateMany branches, and the client-suppliable attemptHeadSha flow cited in the findings.
Unknowns or Needs Verification
- No staging (production-schema-copy) migration run is available in the corpus; CI migration checks stand in as described above.
- The repository-standards assertion that the deployment's operator auth modes (oidc/basic/disabled) should keep unconditional mark behavior comes from the issue text and PR docs, not from a deployment inventory — confirm with the operator/bridge owners that no bearer-mode deployment relies on token-less marks or token-less report settlement after rollout.
- Whether downstream
prFixResolutionconsumers depend onitemIdbeing numeric (per the stale result type) was not verifiable from the corpus; the type should be corrected tostring | nullregardless.
| action: "none" | "blocked" | "fixed" | "deferred" | "skipped"; | ||
| itemId?: number | null; | ||
| reason: string; | ||
| // Echoes the attempt generation the report carried (null when the report |
There was a problem hiding this comment.
Minor (bug): ResolvePrFixFromAgentReportResult.itemId is still typed number | null but settlement now resolves items by string CUID id via an any-typed row, so the stale type flows unchecked to route responses.
Automated finding from AI PR review.
| // The item moved to a newer attempt between the caller's read and | ||
| // this write. Conflict, not an error: no mutation happened. | ||
| return errorResponse( | ||
| `PR fix queue item generation mismatch: expected generation ${input.expectedGeneration} did not match the item's current generation`, |
There was a problem hiding this comment.
Minor (bug): The 409 message interpolates input.expectedGeneration, which is undefined on operator paths, so a refusal-path write matching zero rows (e.g., row deleted between read and write) returns a misleading "expected generation undefined" conflict.
Automated finding from AI PR review.
| // A fresh attempt with no fresher head observation: reset the | ||
| // per-attempt baseline so the next FIXED guard re-baselines from the | ||
| // (newer) mutable headSha. A caller that DOES carry a fresh head | ||
| // (sync re-observation) passes it through here (#1074). |
There was a problem hiding this comment.
Minor (security): The mark route stores a client-supplied attemptHeadSha (validated only as hex) as the supposedly immutable per-attempt baseline, so an authenticated bearer caller can plant a baseline that differs from the real head and later settle FIXED without an actual push; consider deriving it server-side from sync observations only.
Automated finding from AI PR review.
| }); | ||
| return reverted; | ||
| if (!refusal) return { mutated: false, reason: "generation-mismatch" }; | ||
| return { mutated: true, item: refusal }; |
There was a problem hiding this comment.
Minor: The refused-FIXED refusal write matching zero rows (head-unchanged plus a concurrent re-issue or row deletion) has no test, leaving the if (!refusal) return generation-mismatch branch unexercised.
Automated finding from AI PR review.
| if (attempt) { | ||
| // The token is the settlement authority: load the item the token names | ||
| // and cross-check the report's repo/PR against it (not the other way | ||
| // around). |
There was a problem hiding this comment.
Minor: The attempt-token path where the token names a nonexistent item ("attempt item not found") has no unit or route-level test.
Automated finding from AI PR review.
| return items[idx]; | ||
| }, | ||
| updateMany: async ({ where, data }: any) => { | ||
| // Generation-conditional (and/or id-scoped) bulk write — mirrors the |
There was a problem hiding this comment.
Minor: The core PR 1074 guarantee (generation-conditional updateMany settlement) is verified only against hand-written in-memory updateMany mocks; the DB-backed report-route integration suite was not extended to exercise attempt-token settlement against real Postgres.
Automated finding from AI PR review.
| matched: true, | ||
| action: "skipped", | ||
| itemId: existing.id ?? null, | ||
| reason: "legacy report without prFixItem attempt token; queue settlement requires an attempt token (#1074)", |
There was a problem hiding this comment.
Info (docs): The legacy token-less skip return omits attemptGeneration entirely, while the interface comment and all sibling returns echo it as null for token-less reports, giving replay/audit consumers an inconsistent shape.
Automated finding from AI PR review.
| // #1074: bearer (agent/bridge) marks must settle a specific attempt, so | ||
| // the generation token is required. Operator paths (oidc session, basic, | ||
| // disabled) keep the optional behavior for compatibility. | ||
| if (auth.type === "bearer" && input.expectedGeneration === undefined) { |
There was a problem hiding this comment.
Info (question): Operator paths (oidc/basic/disabled) can still settle or roll back any attempt without the generation token; this asymmetry matches the linked issue's explicit compatibility requirement but should be confirmed as acceptable for all deployment auth modes.
Automated finding from AI PR review.
Reason: Review comment on src/lib/pr-fix-queue.test.ts:83 Total attempts: 6 Attempts by lane:
Last attempt: src/lib/pr-fix-queue.test.ts:83 — Minor: The core PR 1074 guarantee (generation-conditional updateMany settlement) is verified only against hand-written in-memory updateMany mocks; the DB-backed report-route integration suite was not extended to exercise attempt-token settlement against real Postgres. Automated finding from AI PR review. Posted automatically by Dispatch on 2026-09-26T08:16:30.266Z |
|
Holding this for two consumers that break the moment it deploys. The change itself looks like the right design. Courier doesn't send the token yet. The Dispatch MCP Deploy order: courier#111 released, the MCP tool updated, then this. |
|
Both prerequisites have landed: Courier 0.1.2 is deployed and echoes |
Summary
Closes #1074
Settles a queue-backed
followup-prreport only against the exact issued PR-fix attempt (id + generation) and its immutable start-head baseline, without clobbering later review evidence.Problem (live evidence, misospace/courier#98)
A completed run reported
pr_updated; Dispatch wrote FIXED then immediately QUEUED because the mutable enqueue-timeheadShaalready equaled the pushed SHA. The guard ran after the write, and its unconditional rollback overwrote a concurrent re-enqueue carrying newer review evidence.next-taskalready exposedprFixItem.{id, generation}(#1044), buttasks/reportaccepted no attempt token.Changes
tasks/reportaccepts and validatesprFixItem.{id, generation}. Stale/mismatched attempts and legacy tokenless reports are skipped with zero mutation of the queue row and zero GitHub calls; legacy reports can no longer implicitly settle a newer queue generation by repo/PR alone.attemptHeadShacolumn (migration20260926000000_add_pr_fix_attempt_head_sha) captured when the attempt becomes dispatchable; the head guard compares the current PR head against that baseline (fallback toheadShafor legacy rows). Same-head success is not FIXED; a push followed by in-flight enqueue enrichment still settles the old attempt and preserves newer feedback.updateMany({ where: { id, generation } })so the generation is revalidated at commit time; the head guard runs before any status write, so there is no transient FIXED-then-unconditional-QUEUED rollback, and no DB transaction is held across GitHub network calls./api/pr-fix-queue/markkeeps operator (oidc/basic/disabled) behavior whengenerationis absent; bearer (worker/bridge) marks now require an expected generation. Mismatched generation → 409 with no mutation.docs/pr-review-fix-queue.mdand the AGENTS.md report-outcomes contract updated for the token requirement.Tests
Done-when matrix covered: unchanged head / no-progress, enqueue during work (baseline stability + feedback preservation), new review arriving before/during settlement (read-vs-commit race via
updateManycount), late/duplicate reports incl. idempotency-key replay, no mutation of terminal (BLOCKED/STALE/FIXED) or newer-generation states, legacy baseline fallback, mark-route generation enforcement (bearer required / operator optional).Full suite green locally: 2831 passed, 11 skipped;
typecheckandlintclean.Related: misospace/courier#98, #1044, #1045.