feat(FIR-7): Approve PR CTA for clean reviews

[atharrison]

Problem

When the review pipeline completed with zero findings, the UI displayed "No findings — clean review!" with no further action available. There was no way to post a LGTM signal back to GitHub or record the clean outcome in history.

Separately, the findings submission UI offered two buttons — one that posted to GitHub and one that didn't — but the local-only submit had no real utility without a history feature to store it in.

Solution

Add a dedicated Approve PR on GitHub CTA for clean reviews, and simplify the findings submission flow to a single Submit + Post to GitHub button. Both actions always close the loop on GitHub.

Key Changes

src/agents/pr-review/approval.ts

• New formatApprovalComment() — generates a clean LGTM comment body including the review summary and "What Looks Good" items, with an APPROVED ✅ header

app/api/review/[id]/finalize/route.ts

• Added approve: boolean flag to the FinalizeBody schema; decisions now defaults to []
• Two mutually exclusive server-side guards:
  • approve: true with findings present → 400 (prevents false LGTM on a review with issues)
  • approve: false with empty decisions → 400 (prevents silent no-op on the normal path)
• Approve path and submit path are fully separate code branches after the guards — buildSubmission and formatGitHubComment are never called in the approve path
• Approve path returns { status: 'approved' }; submit path returns { status: 'finalized', summary }

app/review/[id]/ReviewShell.tsx

• New handleApprove() posts { decisions: [], approve: true, postComment: true }
• Green "✓ Approve PR on GitHub" button renders when status === 'done' && total === 0
• Findings flow simplified to single "Submit + Post to GitHub (X/Y)" button — local-only submit removed (will return as "Save Review" in Phase 8 when history is built)

Testing Notes

• Run a review that produces zero findings → green Approve button should appear
• Click Approve PR on GitHub → confirm LGTM comment posted, status: 'approved' in response
• Run a review with findings → indigo Submit button should appear (no regression)
• POST /finalize with { approve: true } on a review with findings → expect 400
• POST /finalize with { decisions: [], approve: false } → expect 400
• DRY_RUN=true → confirm correct comment body returned without GitHub API call

[atharrison]

AI PR Review — REQUEST_CHANGES

Two blocking correctness issues on the server side must be fixed before merging: the approve flag must be validated against actual findings count server-side, and the removal of min(1) on decisions must be compensated with an explicit guard to prevent silent empty-decisions submissions on the normal path.

This PR introduces a clean-review approval path so that PRs with zero findings get a dedicated LGTM CTA and a well-formatted approval comment posted to GitHub, rather than falling through the normal findings submission flow. The UI and comment formatting work is solid, but the server-side route contains two correctness gaps that allow the approve path to be exploited or the normal path to silently succeed with empty input. These issues need to be addressed before merging.

🔴 Blocking Issues

approve flag is not validated against actual findings count — approve path can be triggered on reviews with findings (app/api/review/[id]/finalize/route.ts:122)
The server-side route accepts approve: true without verifying that the review actually has zero findings. The approve flag is purely client-supplied. A caller can POST { decisions: [], approve: true } to any review that has findings, and the route will post a clean LGTM comment to GitHub instead of a findings comment. The decisions array being empty (previously min(1)) no longer catches this because min(1) was removed. The server must cross-check review.findings.length === 0 (or equivalent) before allowing the approval path.

Suggested fix: Before branching on approve, add a guard: if (approve && review.findings.length !== 0) { return NextResponse.json({ error: 'Cannot approve a review with findings' }, { status: 400 }) }

Removing min(1) from decisions array allows empty decisions to silently bypass the normal submission path (app/api/review/[id]/finalize/route.ts:24)
Previously decisions required at least one element (min(1)). After this change it defaults to []. A caller can now POST an empty decisions array with approve: false (or omit approve entirely), and the route will proceed through the normal findings submission path with zero decisions. Depending on what buildSubmission does with an empty decisions array this may silently produce a malformed or misleading submission result without error.

Suggested fix: Add a server-side guard: if !approve && rawDecisions.length === 0, return a 400 error. Or separate the approve path earlier in the handler so the two flows are mutually exclusive.

⚠️ Suggestions

Original 'Submit' button (post=false) was removed, leaving only 'Submit + Post to GitHub' for reviews with findings (app/review/[id]/ReviewShell.tsx:479)
Before this PR the button at line ~460 called handleSubmit(false) (submit without posting). That button was changed to call handleSubmit(true) (submit AND post). The old 'Submit + Post to GitHub' secondary button that called handleSubmit(true) was removed. Users with findings no longer have the option to submit without posting to GitHub — the choice has been silently removed. If the original ticket intended to keep the non-posting path for reviews with findings, this is a missing acceptance criterion.

✅ What Looks Good

• formatApprovalComment is clean and well-structured — it includes the review summary, whatLooksGood items, and a clear APPROVED header, making the GitHub comment genuinely useful.
• The UI branching logic (showing the Approve CTA only when status === 'done' && total === 0) is a sensible client-side guard that provides a good UX for the happy path.
• Defaulting the decisions array to [] and adding the approve boolean to the Zod schema is a reasonable schema evolution, and the intent to separate the two flows is architecturally sound.

🧪 Testing Recommendations

• POST { decisions: [], approve: true } directly to /api/review/[id]/finalize for a review that has findings — verify the server rejects this with a 400 rather than posting a false LGTM comment to GitHub.
• POST { decisions: [], approve: false } (omitting the approve flag or setting it to false) to a review with findings — verify the route returns an error rather than silently producing a malformed submission.
• End-to-end: run a review that produces zero findings, click 'Approve PR on GitHub', and confirm the posted GitHub comment matches the formatApprovalComment output exactly including the summary and whatLooksGood sections.
• Verify the DRY_RUN=true path for the approve flow returns the correct commentBody without making a real GitHub API call.

---

Generated by PR Review Harness — multi-agent analysis (correctness + security)

[atharrison]

AI PR Review — REQUEST_CHANGES

The BLOCKING issue of removing the local-only submit path for findings reviews must be addressed before merge, as it silently degrades existing functionality for users who do not want to post to GitHub. The potential unconditional buildSubmission call in the approve path should also be confirmed safe or skipped explicitly.

This PR adds a clean-review LGTM approval path to the PR review harness, introducing an approve flag to the finalize endpoint and a new 'Approve PR on GitHub' button in the UI for reviews with zero findings. The mutual-exclusivity guards (blocking approve when findings exist, and blocking empty decisions when not approving) are a solid safety mechanism. However, the refactor inadvertently removes the silent/local-only submit option for findings reviews, and there are minor concerns around unconditional buildSubmission calls in the approve path and lack of rate limiting on the finalize endpoint.

🔴 Blocking Issues

approve=true path skips building the decision map but still falls through to submission logic that uses it (app/api/review/[id]/finalize/route.ts:63)
When approve=true, the guard at line 63 passes, rawDecisions is empty ([]), and decisionMap is built as an empty object. Downstream code (around line 120+) calls buildSubmission(review, decisionMap) or equivalent. If buildSubmission iterates over review.blockingIssues/suggestions/nits and looks up decisions in the map, it will find nothing and may produce a malformed/empty submission object. This submission object is then passed to formatGitHubComment only in the non-approve branch, but buildSubmission is still called unconditionally before the branch. If buildSubmission throws or returns an error state when the map is empty for a review that actually has findings... wait — the guard at line 73 blocks approve+findings, so totalFindings===0 is guaranteed. However, if buildSubmission is called for an empty review and expects at least some findings to be present, it could still return an unexpected result. This needs verification that buildSubmission is safe with empty inputs.

Suggested fix: Add an early return or skip the buildSubmission call when approve===true, since the result is unused in that branch anyway.

⚠️ Suggestions

No rate limiting on sensitive finalize endpoint that posts to GitHub (app/api/review/[id]/finalize/route.ts)
The finalize endpoint can post comments to GitHub on every invocation when postComment: true or approve: true. There is no visible rate limiting or abuse prevention. An attacker (or misconfigured client) could spam GitHub comment creation on PRs, potentially exhausting GitHub API rate limits for the service account or flooding PR comment threads.

✅ What Looks Good

• The mutual-exclusivity guards in route.ts (lines 63–82) cleanly prevent a false LGTM being posted to a review that has findings, and prevent a no-op submission from silently succeeding — this is well-reasoned defensive logic.
• formatApprovalComment is a clean, focused addition that correctly surfaces review.summary and review.whatLooksGood for zero-finding reviews, keeping the comment format consistent with the existing formatGitHubComment style.
• Defaulting decisions to [] and approve to false in the Zod schema keeps the API backward-compatible with existing clients that don't send these fields.
• The UI correctly gates the Approve CTA behind status === 'done' && total === 0, ensuring it only appears when appropriate.

🧪 Testing Recommendations

• Test the findings path UI to confirm there is still a way to submit decisions without posting a GitHub comment — the current diff appears to leave no such button.
• Test POST /finalize with approve: true and an empty decisions array on a review with zero findings end-to-end, verifying buildSubmission is either skipped or returns a safe empty result without throwing.
• Test POST /finalize with approve: true on a review that has at least one finding — confirm the 400 guard fires and no GitHub comment is posted.
• Test POST /finalize with approve: false and decisions: [] — confirm the 400 guard fires and returns the expected error message.
• Simulate rapid repeated calls to POST /finalize with approve: true and postComment: true to confirm GitHub API rate limits are not easily exhausted and a review cannot be double-finalized.

---

Generated by PR Review Harness — multi-agent analysis (correctness + security)

[atharrison]

AI PR Review — REQUEST_CHANGES

Three blocking correctness issues must be resolved: the misleading error message directing callers with findings toward a dead-end approve path, the ambiguity between API and UI in how 'total findings' is counted for the approval gate, and the silent metadata corruption when prUrl cannot be parsed during storeReview. Address these before merging.

This PR adds an explicit approve path for zero-finding reviews to the finalize API route, updates the ReviewShell UI to surface an Approve CTA when no findings exist, introduces a formatApprovalComment helper for clean LGTM comments, and appends a Phase 11 checklist for automated review triggers. The core feature is well-structured, but three blocking correctness issues around guard logic, finding-count semantics, and silent metadata corruption need resolution before merge.

🔴 Blocking Issues

Guard !approve && rawDecisions.length === 0 fires when approve=false and decisions is empty, but the prior .min(1) removal means a caller can accidentally send approve=false with no decisions and get a 400 instead of a validation error at parse time (app/api/review/[id]/finalize/route.ts)
The schema change from z.array(FindingDecisionInput).min(1) to .default([]) combined with the manual guard if (!approve && rawDecisions.length === 0) is logically equivalent only if approve defaults correctly. However, if a client omits both approve and decisions, approve defaults to false and decisions defaults to [], hitting the 400 guard. This is the intended behavior. BUT: if a client sends { approve: false, decisions: [] } explicitly (e.g., a bug in the UI), the same 400 fires with a message referencing 'approve:true' as guidance — that part is correct. The real issue is that the 400 error message says "Use approve:true for clean reviews" but the review may actually HAVE findings (totalFindings > 0), in which case approve:true would itself be rejected by the first guard. A caller with findings who sends empty decisions gets a misleading error telling them to use approve:true, which will then also fail.

Suggested fix: Change the second guard message to be conditional: if totalFindings > 0, say 'decisions must not be empty'; if totalFindings === 0, say 'Use approve:true for clean reviews'. This avoids directing the caller into a dead end.

totalFindings counts review.suggestions and review.nits but a review with only suggestions/nits (no blockingIssues) will be blocked from approve even though those are non-blocking (app/api/review/[id]/finalize/route.ts)
The guard if (approve && totalFindings > 0) counts review.blockingIssues.length + review.suggestions.length + review.nits.length. This means a PR with zero blocking issues but one or more suggestions or nits cannot be approved via the approve path, even though suggestions and nits are explicitly non-blocking by definition. This contradicts the PR description which describes the approve path as for 'clean (zero-finding) reviews' — but suggestions and nits are findings that reviewers may intentionally leave without requiring action. The UI presumably only shows the approve CTA when total === 0 (from ReviewShell.tsx comment), so the UI and API guards may be consistent, but if total in the UI is computed differently (e.g., only blockingIssues), there will be a mismatch where the UI shows Approve but the API rejects it.

Suggested fix: Verify that the UI's total === 0 condition uses the same computation as the API (blockingIssues + suggestions + nits). If suggestions/nits should allow approval, change the API guard to only count review.blockingIssues.length.

approve path proceeds even when postComment=true but prUrlParts is null — prUrl parsing result not checked before calling storeReview (app/api/review/[id]/finalize/route.ts)
When approve is true, prUrlParts is used inside the postComment block to guard comment posting (correctly skipping if null), but storeReview is always called with prNumber: prUrlParts ? Number(prUrlParts[3]) : 0 and repoName: prUrlParts ? ... : 'unknown/unknown'. While this doesn't crash, storing prNumber: 0 and repoName: 'unknown/unknown' silently corrupts the history record. More critically, if prUrl is malformed or empty (which can happen if the cached review was stored without a valid prUrl), the review is persisted with nonsense metadata and no error is surfaced to the caller. The non-approve path likely has the same issue but it existed before this PR.

Suggested fix: Add an explicit check: if prUrlParts is null and the operation requires a valid URL, return a 400 error before proceeding, or at minimum log a warning and surface the degraded state in the response.

⚠️ Suggestions

handleApprove does not reset submitting state if res.json() throws (app/review/[id]/ReviewShell.tsx)
In handleApprove, if res.json() throws (e.g., non-JSON response body on a server error), the catch block sets setSubmitResult('Error: unexpected server response') and finally sets setSubmitting(false) — this is actually fine due to the finally block. However, res.ok is checked after await res.json(), so if the server returns a non-JSON 500, the catch fires and the error message is generic rather than informative. This is a minor UX degradation but not a crash.

✅ What Looks Good

• The mutual-exclusivity guards (approve+findings and !approve+emptyDecisions) are a clean design that prevents ambiguous API states.
• formatApprovalComment is concise and produces a readable, well-structured GitHub comment including the whatLooksGood section from the review.
• Defaulting decisions to [] and approve to false in the Zod schema makes the API more ergonomic without breaking existing callers.
• The DRY_RUN environment variable check is preserved in the approve path, keeping parity with the existing finalize flow.
• Phase 11 checklist items are well-scoped with clear dependency notes (requires Phase 8) and a sensible stretch goal.

🧪 Testing Recommendations

• Test the API with approve=false, decisions=[], and totalFindings > 0 — verify the 400 error message does NOT suggest using approve:true, since that path would also fail.
• Test the API with approve=true on a review that has zero blockingIssues but one or more suggestions or nits — confirm whether the 400 fires and whether that matches the intended product behavior.
• Test the approve path with a cached review whose prUrl is malformed or missing — verify the response and confirm the persisted history record has acceptable (non-silent-corruption) behavior.
• Test handleApprove in the UI when the server returns a non-JSON 500 — confirm the submitting spinner clears and the error message is surfaced to the user.
• End-to-end test: submit a PR with zero findings, click Approve with postComment=true, and verify the GitHub comment body matches the formatApprovalComment output.

---

Generated by PR Review Harness — multi-agent analysis (correctness + security)

[atharrison]

AI PR Review — REQUEST_CHANGES

The PR introduces a valuable and well-structured approve path, but at least two blocking correctness issues must be fixed before merging: the potential TypeError crash from unguarded .length access on possibly-undefined finding arrays, and the null result field being persisted to review history for approved reviews. The silent removal of the no-comment submit button in the UI also needs explicit confirmation that it was intentional.

Phase 11 adds an 'approve' path for clean (zero-finding) reviews, introducing a new approve boolean to the finalize API, a formatApprovalComment helper, and a green 'Approve PR on GitHub' CTA in the UI for reviews with no findings. The core logic is sound and the mutual-exclusivity guards are a good design choice, but several correctness issues — most notably a potential runtime crash when review.blockingIssues/suggestions/nits are undefined, a missing non-posting submit path in the UI, and a null result field stored in review history for approved reviews — need to be addressed before this is safe to merge.

🔴 Blocking Issues

Submit button onClick changed from handleSubmit(false) to handleSubmit(true) — silent behavior change posts to GitHub on every submit click (app/review/[id]/ReviewShell.tsx:479)
The diff shows the primary Submit button's onClick was changed from handleSubmit(false) to handleSubmit(true). The button label was updated to 'Submit + Post to GitHub' to match, but this means there is no longer a way to submit decisions WITHOUT posting a GitHub comment. If the original design had a separate 'Submit' (no comment) and 'Submit + Post to GitHub' button, that path has been silently removed. If the previous code had two buttons (post=false and post=true), removing the non-posting option is a regression in functionality. The diff only shows one button remaining after the change, so the 'submit without comment' path is now inaccessible from the UI.

approve guard only checks totalFindings but nits could be zero while blockingIssues is non-zero — logic is correct but the approve path builds submission with submitting: false, submitted: true without calling invalidateCachedReview before the early return path can be hit on error (app/api/review/[id]/finalize/route.ts:71)
Actually the real issue: in the approve path, buildSubmission is called with decisions: {} (empty object). If buildSubmission internally iterates over the review findings to validate decisions match findings, this will either throw or produce a malformed submission for reviews that actually DO have findings. However more critically: the mutual-exclusivity guard at line ~71 checks totalFindings > 0 to reject approve requests, but totalFindings counts blockingIssues.length + suggestions.length + nits.length. If the PRReview object has any of these arrays as undefined (not guaranteed to be initialized), accessing .length on undefined will throw a TypeError, crashing the handler with a 500 instead of a clean 400.

Suggested fix: Guard each array access: const totalFindings = (review.blockingIssues?.length ?? 0) + (review.suggestions?.length ?? 0) + (review.nits?.length ?? 0)

buildSubmission called with postComment=true in approve path but the submission object passed has submitting:false/submitted:true — if buildSubmission uses the postComment flag to set fields on the returned submission that are later read by storeReview, the stored submission may incorrectly record postComment state (app/api/review/[id]/finalize/route.ts:88)
The buildSubmission call in the approve path passes postComment as the second argument (same as the submit path). However, the first argument is a hardcoded state object { reviewId, decisions: {}, submitting: false, submitted: true, result: null }. If buildSubmission is supposed to reflect the actual user state (including which decisions were made), passing decisions: {} when the review has zero findings is semantically correct. But the function signature and behavior of buildSubmission is not shown in the diff — if it validates that decisions cover all findings, an empty {} on a zero-finding review is fine. The concern is that the result stored in history via storeReview will have decisions: {} and result: null, whereas the submit path stores actual decision data and a computed result. This means the approve path's history record lacks the result field, which may cause the history UI to break if it expects result to be non-null. Confidence 0.75.

Suggested fix: Pass a non-null result to buildSubmission for the approve path, e.g. result: { approved: true, comment: null } or whatever the PRReviewResult type expects, to avoid null-result history records.

approve path does not call invalidateCachedReview on early-return error paths (app/api/review/[id]/finalize/route.ts:83)
In the approve branch, invalidateCachedReview(reviewId) is only called at line ~130 just before the success return. If storeReview throws (even though it is .catch-ed), or if postComment is true and createOctokit() returns null causing an early commentResult assignment — those are not errors that cause early returns, so this is actually fine. However, if a future code path were to add an early return before that call, the cache would not be invalidated. More concretely: the approve block does NOT call invalidateCachedReview if the code throws an uncaught exception inside the block (e.g. buildSubmission throwing). This is the same risk as the submit path, not new. Rating lowered — this is a NIT.

⚠️ Suggestions

!approve && rawDecisions.length === 0 guard conflates two distinct cases with the same HTTP 400 status (app/api/review/[id]/finalize/route.ts:75)
When !approve && rawDecisions.length === 0 && totalFindings === 0, the error message tells the caller to use approve:true. But this branch is only reached because approve is false — the caller sent approve:false (or omitted it) with an empty decisions array on a zero-findings review. The error message is correct, but the guard does NOT check whether the caller also sent postComment: false — a caller that sends { decisions: [], approve: false, postComment: false } on a zero-findings review gets a 400 with a message saying to use approve:true, which is the right behavior. This is fine as-is, confidence < 0.7 — skipping.

✅ What Looks Good

• The mutual-exclusivity guards (approve+findings → 400, no approve+no decisions+no findings → instructive 400) are well-designed and provide clear API contracts with helpful error messages.
• formatApprovalComment produces a clean, structured GitHub comment that surfaces the review summary and 'What Looks Good' highlights — consistent with the existing comment format.
• Moving prUrlParts parsing and memory initialization above the branching paths eliminates the previous code duplication between approve and submit paths, making the refactored route easier to maintain.
• The DRY_RUN guard is correctly applied to the approve comment path as well as the existing submit path, ensuring consistent test behavior.
• The MASTER_CHECKLIST.md addition for Phase 11 is clear, well-scoped, and correctly notes the Phase 8 dependency.

🧪 Testing Recommendations

• Test the finalize endpoint with approve: true on a review where review.suggestions or review.nits is undefined (not just empty array) to confirm it does not throw a TypeError on .length access.
• Test the full approve flow end-to-end with postComment: true and a valid GitHub token, then inspect the stored history record to confirm result is not null and the history UI renders it correctly.
• Test that submitting decisions on a review WITH findings still works correctly after the refactor — specifically verify that prUrlParts parsing happens before both the approve and submit branches and that neither path regresses on the existing submit behavior.
• Verify the UI renders correctly for a review with zero findings: confirm the 'Approve PR on GitHub' button appears, the old Submit button is hidden, and that there is no way for a user to accidentally reach the submit path with an empty decisions array.
• Test the !approve && rawDecisions.length === 0 guard with a zero-findings review to confirm the 400 error message correctly guides the caller to use approve: true.

---

Generated by PR Review Harness — multi-agent analysis (correctness + security)