feat(agent/pr-reviewer): log diagnostic on fail-closed request-changes

[bborbe]

Why

The pr-reviewer intermittently posts GitHub CHANGES_REQUESTED while its review body is a clean approve (e.g. vault-ui PR #26, 2026-07-04 — admin-merged after owner OK). Root-causing it is blocked because the raw ## Review text ParseVerdict actually saw is lost once the Job pod is GC'd — and on the posted body, current ParseVerdict correctly returns approve, so the failure lives in raw-output geometry we can't retrieve after the fact.

This makes the failure legible: when it recurs, pod logs will name the reason and show the tail of the text the parser saw.

What

In postAndRoute, after ParseVerdict, when the verdict is request-changes from a fail-closed reason (empty review text / no verdict block / malformed JSON: / unknown verdict:), emit:

glog.Warningf("pr-reviewer fail-closed to request-changes: reason=%q reviewBody_len=%d reviewBody_tail=%q", ...)

Legitimate (model-authored) request-changes verdicts do not log — the gate is isFailClosedReason, so normal rejections stay quiet.

• Pure helpers isFailClosedReason + lastChars (rune-safe tail) in verdict.go, exposed via export_test.go, table-tested in verdict_diagnostics_test.go.
• No change to the verdict type or verdict→event mapping.

Test

make precommit in agent/pr-reviewer — 13 packages green, 0 lint, 0 vulns.

Follow-up

Once a recurrence is captured, the log pins whether it's trailing-content (no verdict block), malformed JSON, or an unknown verdict — driving the actual parser fix. Tracked in [[Fix pr-reviewer False CHANGES_REQUESTED Regression]].

[ben-s-pull-request-reviewer]

Based on my thorough review of the diff, here is my assessment:

Files changed (5 files, +103/-0):

1. CHANGELOG.md (+4 lines) — Unreleased entry correctly describes the feature, purpose, and no behavior change to verdict/event mapping.

2. verdict.go (+29 lines) — Adds isFailClosedReason and lastChars:

   • isFailClosedReason correctly identifies all 4 fail-closed reasons ParseVerdict emits ("empty review text", "no verdict block", "malformed JSON:"*, "unknown verdict:"*) using exact equality for fixed strings and HasPrefix for variable ones.
   • lastChars is rune-safe (converts to []rune), handles all edge cases: n <= 0 returns "", empty input returns "", len(s) <= n returns full string, multi-byte chars are not split.

3. steps_checkout_execution.go (+16 lines) — The diagnostic glog.Warningf is correctly gated by verdict.Verdict == VerdictRequestChanges && isFailClosedReason(verdict.Reason), ensuring it only fires for suspicious fail-closed cases. Logging does not alter control flow or verdict mapping.

4. export_test.go (+10 lines) — Correctly exposes both helpers via IsFailClosedReasonForTest and LastCharsForTest to the pkg_test package.

5. verdict_diagnostics_test.go (+44 lines) — Comprehensive Ginkgo DescribeTable tests:

   • isFailClosedReason: 7 entries covering all 4 fail-closed + 3 non-fail-closed (including empty string)
   • lastChars: 8 entries covering all edge cases + multi-byte rune non-split ("aé😀xy", 2, "xy")

The isFailClosedReason comment correctly documents the maintenance hazard (must stay in sync with ParseVerdict literals/prefixes) — this is an acceptable tradeoff since the alternative (exporting constants) would expose internal implementation details.

No issues found.

{
  "verdict": "approve",
  "summary": "This PR adds diagnostic logging for fail-closed request-changes verdicts. Two helper functions (isFailClosedReason, lastChars) are correctly implemented with rune-safe string handling. The diagnostic is appropriately gated and does not alter control flow. Tests are comprehensive with 15 table entries covering all edge cases.",
  "comments": [],
  "concerns_addressed": [
    "correctness: isFailClosedReason correctly identifies all 4 ParseVerdict fail-closed reasons (verdict.go:178-183)",
    "correctness: lastChars is rune-safe and handles all edge cases correctly (verdict.go:188-197)",
    "correctness: glog.Warningf diagnostic in postAndRoute is gated and does not alter control flow or verdict mapping (steps_checkout_execution.go:311-318)",
    "tests: verdict_diagnostics_test.go provides comprehensive table tests for both helpers including multi-byte character edge cases"
  ]
}