Review agent should run a coverage-stability check before emitting findings on re-review

[fullsend-ai-retro]

What happened

On PR #508, Review 1 examined commit a896335 and produced 4 findings (1 medium protected-path, 3 low). Review 2 examined commit 33e6bfb — which differed by only a one-line sanitization change in gha_echo — and surfaced 2 new medium-severity findings: (1) raw ISSUE_NUMBER interpolated in the comment body without sanitization (inline comment), and (2) stale RESULT_FILE edge case at no-op exit paths (inline comment). Both vulnerabilities existed in the original commit that Review 1 examined. The coverage gap triggered an extra fix cycle (run 30382926532) and a third review (run 30384841747), adding ~1 hour of wall time and ~$4 in agent compute.

What could go better

The review agent's dimension sub-agents (correctness, security, style-conventions, intent-coherence) are nondeterministic in their coverage — different runs examine different aspects of the same code, leading to findings that surface only on subsequent passes. This is high-confidence: the diff between the two reviewed commits was a single sanitization line addition, yet Review 2 found two entirely new medium-severity classes of issue that Review 1 missed.

The root cause is structural: each sub-agent independently decides what to focus on in a single pass, with no mechanism to ensure coverage of specific vulnerability classes (like input sanitization consistency or stale-state edge cases). The challenger pass filters findings but cannot add findings the sub-agents never surfaced.

Related issues #343 (scoped re-review) and #302 (coverage gap self-reporting) address adjacent concerns but not this root cause. Issue #380 (symmetric defensive pattern application) would catch one of the two missed findings but not the other (stale RESULT_FILE).

Proposed change

Add a coverage-stability verification step to the review orchestrator in agents/review.md or the review harness (harness/review.yaml). After the dimension sub-agents and challenger produce findings, run a targeted verification pass that checks a checklist of high-value vulnerability classes against the diff:

1. Input sanitization symmetry — For every variable that is sanitized at one call site in the diff, verify it is sanitized at all other interpolation points in the same function/scope. (This would catch the ISSUE_NUMBER injection.)
2. Stale-state assumptions — For every file read or variable dereference, verify the value is guaranteed to be current at the point of use (not from a prior iteration or stale path). (This would catch the RESULT_FILE concern.)
3. Error-path coverage — For every new function called in a non-error path, verify equivalent handling exists when the function is reached via error/early-exit paths.

This checklist should be applied deterministically (not left to sub-agent discretion) as a post-processing step before the challenger. If the checklist surfaces findings the sub-agents missed, they should be added to the finding set. The checklist can be maintained in a dedicated file (e.g., skills/review-coverage-checklist.md) and extended as new gap patterns are identified through retros.

Validation criteria

Over the next 10 PRs that go through 2+ review rounds in fullsend-ai/agents, track whether re-reviews surface new medium+ findings on unchanged or minimally-changed code. The target is zero new medium+ findings on code that was present in the prior review's diff. If the checklist catches findings that sub-agents missed, log them for coverage analysis. Success: fewer than 1 in 10 re-reviews surfaces a genuinely new medium+ finding on unchanged code.

---

Generated by retro agent from #508

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 6:35 PM UTC · Completed 6:38 PM UTC
Commit: 32b8aec · View workflow run →

[fullsend-ai-triage]

Triage Summary

Severity: Medium · Category: Feature

The review agent's dimension sub-agents are nondeterministic in their coverage — different runs may examine different aspects of the same code. This was clearly demonstrated on PR #508, where a re-review surfaced two new medium-severity findings on code that was present in the original commit. The coverage gap triggered an extra fix-review cycle.

Root Cause

Each sub-agent independently decides what to focus on per pass, with no mechanism to ensure coverage of specific vulnerability classes. The challenger filters findings but cannot add findings the sub-agents never produced.

Recommended Approach

Add a deterministic coverage-stability checklist as a post-processing step in the review orchestrator, running after sub-agent findings and before the challenger. The checklist should cover:

1. Input sanitization symmetry — if a variable is sanitized at one call site, verify it is sanitized at all interpolation points in the same scope
2. Stale-state assumptions — verify file reads/variable dereferences use current values at point of use
3. Error-path coverage — verify equivalent handling exists on error/early-exit paths

The checklist should live in a dedicated extensible file (e.g., skills/review-coverage-checklist.md) and append any newly found issues to the finding set.

Related Issues

This is distinct from #343 (scoped re-review), #302 (coverage gap self-reporting), and #380 (symmetric defensive patterns) — those address adjacent concerns but not the structural root cause of nondeterministic sub-agent coverage.

Proposed Test

Eval case: Create a PR diff containing a variable sanitized at one call
site but raw-interpolated at another in the same script. Run the review
agent multiple times and verify the unsanitized interpolation is flagged
deterministically on every run — not sporadically.

Second eval case: Include a file read valid on the happy path but stale
on an early-exit path. Verify the stale-state assumption is flagged
consistently.

---

Labels: This is a feature request targeting the review agent, filed by the retro agent. Applying 'feature' and 'review-agent' area labels, and transitioning from 'ready-for-triage' to 'triaged'.

---

Next steps:

• /fs-code — agent creates a PR to implement this issue
• /fs-code <your instruction> — agent implements with your specific guidance

[fullsend-ai-prioritize]

RICE Priority Score: 1.6

Score breakdown

Dimension	Score	Reasoning
Reach	2	The review agent is a core platform component that runs on every PR. Nondeterministic coverage affects all users relying on the review agent across orgs — missed security findings on one pass could slip through if a PR is not re-reviewed. Scoring 2 (most active users across orgs).
Impact	1.5	Missing medium-severity security findings (input sanitization, stale-state bugs) on first review is a significant reliability gap. The demonstrated incident added ~1 hour and ~$4 per occurrence, but the deeper concern is security findings silently not being surfaced. This is a significant pain point for review quality. Scoring 1.5 (high).
Confidence	0.8	The issue is exceptionally well-described: it includes a specific real-world incident from PR #508 with exact commits and findings, clear root cause analysis, a concrete proposed solution, and measurable validation criteria. The triage confirmed the analysis. Scoring 0.8 (high).
Effort	1.5	Implementation requires creating a new coverage checklist file, adding a deterministic post-processing step in the review orchestrator (agents/review.md or harness/review.yaml), and implementing checks for three vulnerability class patterns. This touches multiple components and requires design work on how the checklist integrates with the existing sub-agent/challenger pipeline. Scoring 1.5 (moderate).

Formula: (2 x 1.5 x 0.8) / 1.5 = 1.6