Uh oh!
There was an error while loading. Please reload this page.
fix(pr-followup): route ai-pr-reviewer CHANGES_REQUESTED reviews by structured findings - #630
Conversation
…tructured findings
A CHANGES_REQUESTED review's lane was chosen by classifyFeedback() running
keyword regexes over the review prose, defaulting to needs_human when nothing
matched. The ai-pr-reviewer writes long narrative markdown that matches none
of those patterns, so every AI review fell through to needs_human, was routed
to the NEEDS_HUMAN lane, and dead-ended at human escalation — the PR-fix
auto-fix loop never engaged (mirrors the check_run path's own warning against
prose-classifying).
The reviewer already embeds a machine-readable verdict in the body as
<!-- ai-pr-reviewer:{...} --> with open_findings. Parse it and, when it carries
findings with messages, route to NORMAL so the coder attempts a fix; the
PR_FIX_MAX_ATTEMPTS -> ESCALATED -> NEEDS_HUMAN ladder handles 'coder can't fix
it', exactly as for CI failures. Human prose reviews (no payload) still fall
back to classifyFeedback.
AI assistance: authored with Claude Code.There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
Review: Approve
This PR correctly fixes the routing bug where ai-pr-reviewer structured reviews were falling through to NEEDS_HUMAN instead of engaging the auto-fix loop.
Change-by-Change Findings
src/lib/pr-followup-ingestion.ts (+60/-1):
parseAiReviewerFindings()— New function that extracts the machine-readable<!-- ai-pr-reviewer:{...json...} -->payload from review bodies. Handles the bare marker, sibling siblingsha/fingerprintcomments, null/undefined/empty input, and malformed JSON by returningnull. The regex/<!--\s*ai-pr-reviewer:\s*([\s\S]*?)-->/is non-greedy (*?) on the JSON content, which is correct sinceJSON.parsewon't consume-->as part of the value. JSDoc is thorough and explains the design rationale.reviewLane()— New lane-routing function forCHANGES_REQUESTEDreviews. If the parsed payload contains any finding with a non-emptymessage, it returnsNORMAL(actionable by the coder). Otherwise it falls back tolaneFor()which usesclassifyFeedback()on the prose. This is the correct priority: structured findings override prose classification.INGEST_DESCRIPTORS['REVIEW']— Single-line change replacinglane: laneFor(event.body ?? "")withlane: reviewLane(event.body ?? ""). All other event types and lane paths are unchanged.
src/lib/pr-followup-ingestion.test.ts (+92/-0):
parseAiReviewerFindingsunit tests — Three cases: full JSON payload extraction, bare/sibling-comment markers returningnull, and malformed/absent payload returningnull.routes ai-pr-reviewer review with findings to NORMAL— Includes an inline guard assertionexpect(classifyFeedback(body)).toBe("needs_human")proving the prose alone would dead-end, then verifiesingestReviewEventreturnslane: "NORMAL". This is the regression test for the real-world trigger (misospace/miso-chat#691).routes vague CHANGES_REQUESTED with no payload to NEEDS_HUMAN— Confirms the prose-classification fallback path is preserved when no structured payload exists.
Standards Compliance
| Convention | Status |
|---|---|
Error handling (error instanceof Error pattern) | ✅ N/A — JSON.parse wrapped in try/catch with silent return null |
| API routes return appropriate HTTP status codes | ✅ N/A — library function only |
| Validation before DB operations | ✅ N/A — no DB writes |
| TypeScript interfaces for structured data | ✅ AiReviewerFinding, AiReviewerPayload defined |
| Exported for testing | ✅ parseAiReviewerFindings explicitly exported |
| JSDoc comments for public functions | ✅ Full JSDoc on parseAiReviewerFindings, reviewLane |
No agent-specific names in generic docs, no secrets committed, no schema changes, no auth changes. Fully compliant.
CI Verification
- Typecheck: ✅ success
- Lint: ✅ success
- Tests: ✅ success (73 passed including new tests)
- Build: ✅ success
- Docker Build: ✅ success
All CI checks passed. No blockers.
Tool Harness Findings
No tool calls issued; reviewed corpus directly. No findings.
Summary
classifyFeedback()running keyword regexes over the review prose, defaulting toneeds_humanon no match. The ai-pr-reviewer writes long narrative markdown that matches none of those patterns, so every AI review fell through toneeds_human→ NEEDS_HUMAN lane → immediate human escalation, and the PR-fix auto-fix loop never engaged.<!-- ai-pr-reviewer:{...} -->withopen_findings. NewparseAiReviewerFindings()reads it; the review path now routes to NORMAL (auto-fix) when findings carry messages, and only falls back toclassifyFeedbackfor human prose reviews with no payload.PR_FIX_MAX_ATTEMPTS → ESCALATED → NEEDS_HUMANladder still handles "coder can't fix it" — same as thecheck_runpath, which already hardcodes NORMAL with a comment warning against prose-classifying.Verification
vitest runonpr-followup-ingestion,pr-followup/{sync,webhook},pr-fix-queue: 73 passed. New tests:parseAiReviewerFindings(extract / ignore sibling markers / null on malformed); an ai-reviewer review with findings routes to NORMAL even though its prose classifies asneeds_human; a payload-less vague review still routes to NEEDS_HUMAN.tsc --noEmitandeslintclean.Notes
misospace/miso-chat#691— saffron correctly requested changes (blocker: no package upgrade performed), but it was bounced to a human instead of auto-fixed.