fix(review-gate): say why an unrecognized ledger verb held a finding

[kkroo]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Its reviewer agent, Ally, closes earlier findings through a ### Prior Findings Dispositioned ledger, and review/ally-comment reads that ledger to decide whether a carried finding is still open
• The parser only retires on verbs it knows, and fails closed on anything else — correct, because failing open would silently clear a live finding
• But the resulting red says the finding is "still undispositioned" while Ally's ledger visibly dispositions it, so nothing tells a reader the verb is the reason
• This pull request makes the gate name the unrecognized verb when that verb is what left the finding unretired
• The benefit is that the next vocabulary gap is self-explaining instead of costing a diagnosis

Linked Issues or Issue Description

No existing issue — this is the follow-up flagged in #1568 (merged bd35f20), described inline as a bug report.

What happened. #1568 shipped a fail-closed allowlist of disposition verbs. During its own review Ally reported that no-longer-applicable was missing from that allowlist, so ledger entries using it were parsed and then dropped, leaving the finding blocking with nothing able to retire it.

Expected. A reader looking at the red status can tell why the finding is still blocking.

Actual. The status read:

An unresolved finding from Ally's review of 731ced5 is still undispositioned;
no comment attests the current head.

That sentence is true but actively misleading here: Ally's ledger does disposition the finding. Distinguishing "the verb is unknown to the parser" from "the finding is genuinely open" required reading the gate source. Ally described this as the finding being "silently ignored".

Not fixed by widening the allowlist. Adding no-longer-applicable (which #1568 did) closes one instance. The diagnosability gap recurs on the next verb.

What Changed

• pr-comment-review-gate.ts: when an unrecognized ledger verb is what left a carried finding unretired, the carried_finding reason names that verb.
• The diagnostic reuses the retirement matcher (namesFinding), so it can only name an entry that would otherwise have retired that finding. An unknown verb pointing at a different finding on the same head is not offered as the explanation.
• ally-review-detection.ts: ledger parsing collapses to one classified extractor, extractAllyPriorFindingDispositions, replacing the two filtered ones. classifyPriorDisposition makes the third state explicit — retires / blocks / unrecognized.
• still-present is classified blocks, distinct from unrecognized, so a correctly-blocking finding is never reported as vocabulary drift.

No behavioral change to what the gate blocks on. Verdict and outcome are identical; only the human-readable reason differs.

Verification

pnpm exec vitest run in server/:

• pr-comment-review-gate.test.ts — 30 passed (6 new)
• the seven other review-gate/webhook files — 309 passed across 8 files
• pnpm exec tsc --noEmit — clean

Every new test was confirmed to discriminate by mutation. This mattered: my first pass at the "only blame the verb that named this finding" guard passed every test with the guard removed, so I added a case where the ledger names a finding the head never raised.

mutation	test that goes red
never report unrecognized verbs	names-the-unrecognized-verb
blame any unretired finding, ignoring which one the verb named	names-a-finding-this-head-never-raised
drop the verb-list budget	140-character cap
classify still-present as unrecognized	does-not-blame-a-verb-it-never-named

Measured message lengths against GitHub's cap (ordinary reason 114, unrecognized-verb reason 96, worst case with a 63-character verb 112).

Risks

Low. Parsing and message construction only — no schema, config, or deployment change, and no change to which PRs the gate blocks.

• Truncation. The reason becomes the commit-status description, which GitHub cuts at 140 characters and which github-app-auth.ts and github-status-delivery-outbox.ts already .slice(0, 140). This is written as a replacement message rather than a suffix for that reason — the ordinary reason already spends 114 — and the verb list carries its own budget because the ledger regex accepts an arbitrarily long verb. Pinned by a test.
• Misattribution. Naming the wrong verb would be worse than saying nothing. Mitigated by reusing the retirement matcher and by the mutation above.
• API change. extractAllyDispositionedPriorFindings and extractAllyUnrecognizedPriorDispositions are removed in favour of the single classified extractor. Both were internal to server/src with no direct test coverage; the gate was the only caller.

Test-run note, reported because it is correlated with this change: the 8-file local combination above failed once on this branch with Hook timed out in 60000ms in github-webhook.test.ts's afterAll DB cleanup, while master passed the same set. All 309 tests passed in that run — it is a teardown timeout, not an assertion. It did not reproduce on re-run (220s vs the 272s failing run), so it reads as a load-sensitive hook timeout on the shared embedded postgres that the six extra tests can tip over. I have deliberately not raised hookTimeout, since that would mask pre-existing fragility in a file this PR does not otherwise touch. Flagging it rather than filtering it out.

Model Used

Anthropic claude-opus-5 (1M context), via Claude Code.

---

• I searched for similar open/closed PRs and confirmed this is not a duplicate

[kkroo]

@ally please review.

Context: review/ally-comment is green on this head but its description reads "No Ally consolidated-review comment attests to reviewing this head" — i.e. not_evaluated, the documented fail-open, not a clean review. No reviewer run appears to have fired for this PR since it opened.

Flagging it explicitly because this PR changes that gate's reasoning, and its sibling #1568 needed three rounds — each one a real defect, including a partial-ledger false-green I would otherwise have merged.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 779ee7b

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/pr-comment-review-gate.ts:257-259 — an unknown verb is reported even when a known still-present ledger entry for the same finding also exists, so the reason can falsely attribute the blocker to vocabulary drift rather than the explicit blocking disposition.
  • Only include an unrecognized verb when no applicable blocks entry names that finding (or otherwise give the known blocking disposition precedence), and add a regression test covering both entries for one finding.

Suggestions (0)

Strengths

• The classified ledger extractor keeps retirement, blocking, and unrecognized verbs explicit and prevents unknown verbs from failing open.
• The diagnostic reuses the same finding matcher as retirement and bounds the status reason to GitHub's 140-character limit.
• The added tests cover unrelated findings and partial ledgers, and the live CI checks are green.

Recommended Action

1. Fix the Important issue before merge.
2. Address Suggestions opportunistically.

[kkroo]

Addressed in 0239a61. The finding was correct and worth the round.

Two later reviews can disposition one finding differently — one still-present, one using a verb the parser doesn't know. My loop skipped retired findings but not explicitly-blocked ones, so it reported the unknown verb and named vocabulary drift as the cause of a block Ally had deliberately left open with an explicit answer. A wrong explanation is worse than the bare reason this diagnostic replaces, which is the whole premise of the PR, so the bug undercut its own purpose.

blocks now takes precedence: an unrecognized verb is only offered when no known blocking disposition names that finding. Regression test added with both entries on one finding, mutation-checked — removing the precedence line reds it.

Suites: 89 across the seven review-gate files, tsc clean.

On github-webhook.test.ts: I reported in the PR body that its afterAll hook timeout looked correlated with this change, since master passed the combined run and this branch didn't. That was one sample and the correlation was spurious — master reproduces the identical Hook timed out in 60000ms with these changes stashed, running the file alone. It's a pre-existing load-sensitive teardown flake on my machine; all 221 tests pass either way, and CI's server shards are green. Correcting it here so the PR body isn't the last word on it.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0239a61

Prior Findings Dispositioned (1)

• prior:779ee7b important 1 — fixed — server/src/services/pr-comment-review-gate.ts:271 — matching still-present ledger entries now take precedence over unrecognized verbs, so the diagnostic cannot misattribute an explicitly blocking finding to vocabulary drift; the regression is covered at server/src/__tests__/pr-comment-review-gate.test.ts:528.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The classified disposition extractor keeps retiring, blocking, and unrecognized verbs distinct while preserving fail-closed behavior.
• The diagnostic matches ledger entries to the specific reported finding and stays within the commit-status character cap.
• The added tests cover unrelated findings, long verbs, and precedence between explicit blocking and unknown dispositions.

Recommended Action

1. Merge when the remaining required checks are green.