fix(report): reject already_done on PR-fix attempts (#1229)

[joryirving]

Changes

Restrict already_done to taskType: "implement" reports without a non-null prFixItem. Invalid combinations return HTTP 400 before AgentRun/idempotency writes or either resolution path.

Add regressions for followup-pr reports with and without an attempt token, implement reports carrying an attempt token, and generation-fenced already_addressed settlement. Existing valid implement close coverage remains intact. Authorization, freshness, evidence grounding, and replay guards are unchanged.

Closes #1229 when merged.

Validation

• Route tests: 131 passed
• npm run typecheck: passed (after regenerating local Prisma client)
• npm run lint: passed
• git diff --check: passed

Review

Left open for maintainer review. No approval or merge performed.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M3 (anthropic) — primary route · pr-reviewer-action v3.4.1

Recommendation

Approve. This is a narrow input-validation boundary fix that closes the contract hole called out in PR 1229: already_done is now restricted to taskType: "implement" and a non-null prFixItem is refused, both with a clear HTTP 400 returned before any AgentRun / idempotency write or either resolution path. CI is fully green (lint, typecheck, build, tests, coverage, migrations, smoke, image builds).

Change-by-change findings

• src/app/api/agents/[agentName]/tasks/report/route.ts (lines 289–293): a new guard immediately after isAlreadyDone is computed and before the evidence/close-criteria parsing and the prFixItem structural validation. Because it sits in the input-validation block that runs ahead of the prisma.$transaction that creates agentReportDedupe + agentRun, and ahead of both the PR-fix and close resolvers (lines ~525 and ~563 in the same file), an invalid combination returns 400 with zero DB writes and zero GitHub side effects. The guard uses (raw.prFixItem !== undefined && raw.prFixItem !== null), which correctly accepts undefined and explicitly null and rejects any populated token — matching the issue's must not carry a non-null prFixItem requirement.
• src/app/api/agents/[agentName]/tasks/report/route.test.ts (new it.each at lines ~1762–1776): exercises the three invalid combinations the issue called out — followup-pr without a token, followup-pr with a token, and implement with a token — and asserts the exact 400 + error regex plus mockAgentRun.create, mockDedupe.create, prFixResolveMock, and closeResolveMock were all never called. The mocks backing those asserts are defined in the hoisted block (lines ~10–55) with vi.fn() spies, so the zero-side-effect assertions are meaningful.
• Same file (lines ~1909–1923): the new "settles followup-pr already_addressed with its generation token" test confirms the unchanged followup-pr + already_addressed + prFixItem path continues to flow through prFixResolveFromAgentReport with the { itemId, generation } attempt token. This satisfies the issue's last required regression and is the only path the PR leaves open.

Claim-by-claim disposition

• Claim 1 ("Authorization, freshness, evidence grounding, and replay guards are unchanged."): held for the items I checked. The PR diff adds only the three-line guard and the new test rows. The authorizeRequest + enforceWorkerAgentScope calls run earlier in the handler; the enforceAlreadyDoneWorkerScope (worker-tier tracked + live-assigned gate from PR 1219) still sits inside the isAlreadyDone block at line ~563; the reportPayloadHash / P2002 / agentReportDedupe.findUnique replay path is untouched; the close-policy call still passes evidence, criteria, boundAgent, and the apply-time freshness enforceAlreadyDoneWorkerScope recheck is still called immediately before resolveAlreadyDoneFromAgentReport. Nothing in the diff removes or weakens these guards.
• Claim 2 ("## Validation" — 131 route tests passed, typecheck/lint/git diff --check passed): held as far as the corpus shows. The PR's CI section in the corpus lists every check (Build, Lint, Typecheck, Tests, Coverage, Database migrations, Database integration, Workflow lint, Docker Build, Docker Build (MCP), smoke, npm audit) as success, including the Tests run. No failing evidence is in the corpus.

Linked Issue Fit

The PR satisfies every numbered requirement in PR 1229:

1. Validation runs before the AgentRun/idempotency write and before any GitHub side effect (the guard is at lines 291–293; the prisma.$transaction and resolvers are at lines ~470–610), and returns HTTP 400 with a clear message for both invalid combinations.
2. The three required invalid-combination regressions are in the new it.each (with zero side-effect assertions), and the unchanged valid implement+already_done path keeps its existing regression at line ~1847 ("returns 200 and runs the close resolver for a valid already_done report"). The fourth regression (followup-pr + already_addressed + generation-fenced prFixItem) is the new test at lines ~1909–1923.
3. The auth, freshness, evidence grounding, and replay guards from PR 1219 are untouched (see Claim 1 above).

The out-of-scope items (PR 1229 §"Out of scope") are respected: no PR-fix queue redesign, no new outcome, no already_done settlement of PR-fix generations, and no already_addressed contract change.

Standards Compliance

• Validation before DB operations (AGENTS.md §Code Standards PR 5): the new guard runs ahead of prisma.$transaction and both resolvers, so DB writes and GitHub side effects cannot occur on an invalid combination. ✅
• HTTP status codes / JSON responses (PR 3): invalid input returns 400 via the existing errorResponse helper; the body is { error: "already_done is only valid for implement reports without prFixItem" }. ✅
• Lint/typecheck blocks CI; must pass: both green in the CI matrix. ✅
• Secret handling (PR 6): no secrets touched. The test file references ⟦redacted:credential⟧ markers, which are harness redactions over the real token value, not real content. ✅

Tool Harness Findings

The tool calls all returned successfully (read_file ×8, git_grep ×1). They confirmed: (a) the new guard sits at route.ts:292 in the input-validation block, well before prisma.$transaction and the resolver calls; (b) the test mocks (mockAgentRun.create, mockDedupe.create, prFixResolveMock, closeResolveMock) are vi.fn() spies so the not.toHaveBeenCalled() assertions are meaningful; (c) the unchanged valid implement+already_done test at route.test.ts:1847 still exercises the close resolver; (d) the new followup-pr+already_addressed+prFixItem test at lines ~1909–1923 exercises the PR-fix path with the { itemId, generation } attempt token. No tool failed.