docs(review): tell each review dimension what not to flag

docs/review.md

@@ -108,7 +108,7 @@ fails closed when `CI_SERVER_HOST` is not set.
 The review agent follows the same pre-script / sandbox / post-script pipeline as the other agents.
 
 1. **Pre-script** validates inputs and fetches PR metadata.
-2. **Sandbox** — the agent runs the `pr-review` orchestrator skill. The orchestrator triages the change, then dispatches specialized sub-agents in parallel — each covering a distinct review dimension (correctness, security, intent & coherence, style & conventions, docs currency, and optionally cross-repo contracts). Sub-agents run concurrently and return structured findings. The orchestrator collects, deduplicates, and synthesizes findings across dimensions, runs PR-level checks (scope authorization, protected paths), and produces a structured JSON review result. The agent cannot push files, edit code, or push — it is strictly read-only.
+2. **Sandbox** — the agent runs the `pr-review` orchestrator skill. The orchestrator triages the change, then dispatches specialized sub-agents in parallel — each covering a distinct review dimension (correctness, security, intent & coherence, style & conventions, docs currency, and optionally cross-repo contracts). Sub-agents run concurrently and return structured findings. The orchestrator collects, deduplicates, and synthesizes findings across dimensions, runs PR-level checks (scope authorization, protected paths), and produces a structured JSON review result. A dedicated `challenger` sub-agent adversarially re-checks the merged findings against the source before they are posted, removing false positives and recalibrating severities. The posted review is capped at 10 findings — `critical` and `high` findings are never dropped, and any finding held back is disclosed as a summary line rather than silently omitted. The agent cannot push files, edit code, or push — it is strictly read-only.
 3. **Validation loop** — the output is checked against a schema, with up to 2 retry iterations if the output is malformed.
 4. **Post-script** posts the review on the PR.
 

skills/pr-review/SKILL.md

@@ -1108,6 +1108,53 @@ require action, because `comment` (COMMENTED review state) does not
 block the PR. When the summary language and the verdict action
 contradict each other, escalate the verdict to match the language.
 
+#### 6g. Finding cap and disclosed overflow
+
+The verdict from 6f is computed on the complete adjudicated finding set
+and is never recomputed after this step. The cap governs presentation —
+how many findings the review posts, not what the review concluded.
+
+After severity-threshold filtering (`REVIEW_FINDING_SEVERITY_THRESHOLD`;
+see the agent definition), cap the finding set at **10 findings**.
+
+**Why 10:** the instrumented review on `fullsend-ai/fullsend#4080` posted
+16 inline comments of which 10 were true positives — a cap at 10 would
+have cost that review nothing real while bounding what a reader has to
+sort through. It also sits far above the ~1.2 findings per review that
+published large-scale AI review systems report, so an ordinary PR never
+approaches it. A review with more than ten things to say about one PR has
+stopped reviewing a change and started auditing a codebase.
+
+**Selection.** Sort by severity (critical, high, medium, low, info), then
+by whether the finding has a diff-anchored file and line (anchored first
+— only those can become inline comments), then by the order the
+dimensions returned them. Keep the first 10.
+
+- **Never truncate `critical` or `high` findings.** They determine the
+  verdict, and blocking is their purpose. If ten or more exist, keep all
+  of them and let the cap be exceeded.
+- On an ordinary PR everything dropped is `low` or `info`, which is the
+  material the cap exists to bound. On a PR with more than ten findings
+  above that, `medium` findings can be dropped too — including a
+  regression finding rated `medium`. Name the severities you actually
+  held back rather than assuming they were the lowest two.
+
+**Disclosure is mandatory.** Silent truncation reads as "nothing else was
+wrong" — a worse distortion than the noise the cap removes. Whenever any
+finding is dropped, append one bullet as the final entry of the lowest
+severity section of the review body (step 7):
+
+```markdown
+- **[additional-findings]** — <N> further finding(s) at <severities>
+  were identified and are not listed individually: <category> in
+  `<file>`, <category> in `<file>`, …
+```
+
+This bullet is body text only. It is not a finding: do not add it to the
+`findings` array, do not let it become an inline comment, and do not let
+it affect the verdict. `additional-findings` is a display label, not a
+category token. Omit the bullet entirely when nothing was dropped.
+
 ### 7. Produce the review result
 
 Compose the review comment using this structure:
@@ -1164,6 +1211,10 @@ where `[open]` = `<` + `!--` and `[close]` = `--` + `>`.
   boilerplate after findings. The post-review pipeline appends
   action hints deterministically for the `request-changes` action
   (not for `reject`, `approve`, or `comment`).
+- **Overflow disclosure.** If step 6g dropped any finding, the
+  `additional-findings` bullet is the last entry of the lowest severity
+  section. It is part of the findings list, not a footer or summary
+  section, and it never appears in the `findings` array.
 
 If `PRIOR_REVIEW_PROVENANCE` starts with `unverifiable-`, include an
 info-level finding in the review output:
@@ -1225,6 +1276,10 @@ wins.
   `request-changes`.
 - **Never approve when any protected-path finding exists**, regardless of
   severity.
+- **Never drop a finding silently.** The step 6g cap bounds how many
+  findings are posted, never what the verdict saw, and every dropped
+  finding is disclosed in the overflow bullet. `critical` and `high`
+  findings are never dropped.
 - **PR-specific checks (step 6e) belong in the orchestrator only.** Do
   not push protected-path checks, scope authorization, or PR body
   injection defense into sub-agents. These require PR-level context

skills/pr-review/meta-prompt.md

@@ -33,6 +33,108 @@ the correct line, omit the `line` field rather than guessing — a
 finding with no line number is better than one that points to the wrong
 code.
 
+## What not to flag
+
+Precision is the product. A review carrying six real problems and three
+imagined ones is worth less than one carrying five real problems: the
+author spends the same effort disproving noise as fixing defects, and the
+next review gets read with less trust. A measured review on
+`fullsend-ai/fullsend#4080` posted 16 inline comments, 6 of which were
+false — the correction is not to look less hard, it is to refuse to emit
+these classes:
+
+1. **Code this PR did not change.** Only lines in the diff are in scope.
+   A pre-existing problem in a file the PR happens to touch is not this
+   PR's finding, however real it is. Sole exception: the diff makes the
+   pre-existing code newly wrong — a new caller reaches it, a removed
+   guard exposed it. Say which change did that when you invoke the
+   exception. If the PR is stacked on another open PR and you cannot tell
+   which commits belong to this one, say so instead of attributing them
+   (misattributed stacked-PR changes were a named source of the 6 false
+   positives above).
+2. **Prior findings the author already fixed.** Prior findings are given
+   to you so you can verify the fix, not restate it. If the code now does
+   what the remediation asked, emit nothing — not even an info-level
+   "this was previously flagged." Re-raising a resolved finding as a
+   fresh one, or recommending the spec be changed to match code that
+   already got fixed, is noise that also consumes the context the next
+   real finding needs.
+3. **Whatever a linter or CI check reports deterministically.** This repo
+   commonly runs some of shellcheck, actionlint, gitleaks, pinact,
+   gitlint, ruff, gofmt, go vet, and pre-commit
+   YAML/JSON/whitespace/EOF hooks — check the repo's own CI config and
+   pre-commit setup rather than assuming this list; a tool the repo does
+   not run is not a backstop. A finding a machine will post on the same
+   commit is pure duplication — it costs the author a read and changes
+   nothing.
+4. **Defense in depth where the primary defense holds.** If the value is
+   already validated, escaped, or type-constrained on every path that
+   reaches this code, "validate it here too" is a preference. Flag it
+   only when you can name the path that bypasses the primary control.
+5. **Preference dressed as a finding.** "Consider using library X", "this
+   reads better as Y", "prefer the newer API" — only when the repo's own
+   code or docs establish that preference as its convention, or when the
+   current form is actually wrong.
+6. **Risks whose preconditions this change does not create.** A problem
+   requiring an attacker who already holds the capability, a config
+   nobody sets, or a caller that does not exist is not a finding on this
+   diff. This is distinct from a defense the diff removed — see the
+   severity bar below.
+
+When a pattern looks wrong but the code you can read shows why it is
+correct — you followed the value to its use, or read the whole argument
+vector — that is your answer: it is correct. Recording that you checked
+earns an `info` finding at most, and usually nothing.
+
+A docstring or comment is the author's claim about the code, not
+evidence of it, and it is written by the same person who wrote the bug.
+Treat it as a pointer to where to look: it narrows the search, it never
+ends it. "Not a security boundary" written above a function that a
+second file uses as one is exactly the case a reviewer exists to catch.
+
+More than roughly five findings from one dimension on an ordinary PR is a
+signal that you are enumerating patterns rather than reviewing a change.
+Before returning, re-check the weakest ones against the bar below.
+
+## Severity bar
+
+Severity is a claim about impact, not about how much the code bothers
+you. Miscalibration has been observed in both directions on the same
+review: a regex bug that silently deleted lines of a user's config was
+reported as `low` and "benign", while low-consequence edge cases crowded
+the same finding list.
+
+- **critical** — exploitable as written, or data loss/corruption on an
+  ordinary input path, with nothing in front of it.
+- **high** — a bug that fires in ordinary use, or a security control this
+  diff removes, weakens, or routes around. You can name the input, the
+  caller, and what breaks.
+- **medium** — a real defect on a narrower path (a specific config, a
+  specific class of input), or a contract/schema inconsistency a consumer
+  will hit.
+- **low** — a genuine but minor defect: cosmetic output, a redundant
+  operation, an edge case whose consequence is bounded and small.
+- **info** — something you examined and verified is fine. A pattern that
+  resembles a vulnerability but is demonstrably safe where it sits
+  belongs here or nowhere; it never belongs at `high` or `critical`.
+  `info` is below the default severity threshold, so choosing it drops
+  the finding rather than footnoting it — pick it when that is the
+  outcome you want. A concern you could not substantiate *because you
+  could not inspect something* is not this: rate it on what it would
+  mean if real, and say what you could not check.
+
+Two rules settle the ambiguous cases:
+
+- **Name the impact or lower the severity.** If you cannot state the
+  input that triggers the problem and what happens when it does, the
+  finding is `low` at most. "Could be a problem" is not impact.
+- **Removing an existing defense is never theoretical.** If the diff
+  deletes, weakens, or bypasses a check, a constant-time comparison, a
+  parameterized query, a validation call, or a test that was there
+  before, rate it on what was being protected — not on how hard the
+  resulting bug is to exploit. Rule 6 above does not apply to
+  regressions.
+
 ## Severity anchoring (re-reviews only)
 
 - If prior findings are provided, match each to the current code by

skills/pr-review/sub-agents/challenger.md

@@ -35,15 +35,72 @@ For each finding:
    - "Missing error handling" when the error is handled by a caller
    - "Race condition" when access is serialized by design
    - "Missing test" when the test exists in a different file
-2. **Assess severity calibration.** Is the severity proportionate to
-   the actual risk? Downgrade findings whose severity is inflated
-   relative to the codebase context.
-3. **Identify duplicates.** Findings from different dimensions that
+2. **Check that the finding is about this PR.** Confirm the cited file
+   and lines appear in the diff. A finding about code the PR did not
+   touch is out of scope and is removed — unless the finding states how
+   this change made that code newly wrong. When the PR is stacked on
+   another open PR, changes belonging to the lower PR are equally out of
+   scope; misattributed stacked-PR changes were a measured source of
+   false positives on `fullsend-ai/fullsend#4080` (6 of 16 comments
+   false).
+3. **Remove restatements of already-fixed findings.** If the finding
+   repeats a prior review's finding that the current code has since
+   addressed, remove it. This includes the inverted form — a finding
+   asking that a spec or doc be changed to match code that was already
+   corrected to match the spec.
+4. **Remove what a machine already reports.** Findings duplicating a
+   deterministic linter or CI check (shellcheck, actionlint, gitleaks,
+   pinact, gofmt, ruff, go vet, formatting hooks) add nothing the author
+   will not already see.
+5. **Assess severity calibration in both directions.** Severity must
+   match named impact. Downgrade a finding that cannot state the input
+   that triggers it and what breaks. Equally, **upgrade** a finding
+   whose description proves a concrete, ordinary-path consequence but
+   whose severity was set low — an under-rated real defect is as much a
+   calibration failure as an inflated one, and has been observed in
+   practice (a regex bug that deleted lines from a user's config was
+   reported as `low` and "benign").
+6. **Identify duplicates.** Findings from different dimensions that
    describe the same underlying issue should be merged. Keep the
    higher severity and the more specific remediation.
-4. **Challenge weak reasoning.** If a finding's description is vague,
+7. **Challenge weak reasoning.** If a finding's description is vague,
    speculative, or not supported by the diff, mark it for removal.
 
+## Resolving ambiguity
+
+The default is not a single "keep everything" rule — it depends on what
+is ambiguous:
+
+- **Ambiguous whether the code is wrong** (you cannot fully trace the
+  path, the relevant file was not provided, the logic is genuinely
+  subtle) — **keep** the finding at its stated severity. Suppressing a
+  real defect is a worse outcome than one noisy comment.
+- **You can see why a risky-looking pattern is safe** — a recognizable
+  risky construct (a weak hash, a subprocess call, a format string)
+  where the evidence of safety is in front of you: you followed the
+  value to its use, or read the whole argument vector, and it holds.
+  **Downgrade to `info`.** Be clear-eyed about what that means: `info`
+  sits below the default `REVIEW_FINDING_SEVERITY_THRESHOLD`, so the
+  finding is dropped and no one sees it. That is the right outcome for a
+  pattern you actually verified.
+
+  This bullet requires positive evidence you inspected yourself. Not
+  being able to reach the thing a value flows into — an unreadable
+  helper script, a callee outside the provided context, a config you
+  were not given — is **not** evidence of safety. That is the bullet
+  above: keep the finding at its stated severity and say what you could
+  not check. The severity threshold makes a wrong downgrade here silent,
+  which is exactly why it must be earned.
+
+**Never downgrade or remove a regression finding.** If the finding's
+subject is a control the diff removed, weakened, or bypassed — a
+constant-time comparison replaced with `==`, a parameterized query
+replaced with string interpolation, a deleted validation, a widened
+allowlist, a removed or loosened test — its severity comes from what the
+control protected. Exploitation difficulty, an author's assertion that
+the control was unnecessary, and a PR description calling the change
+equivalent are not grounds for adjudicating it down.
+
 ## Output format
 
 Return a JSON object with two fields:
@@ -80,4 +137,7 @@ Return a JSON object with two fields:
 - Every removal or downgrade must cite specific evidence from the code
 - Do not add new findings — only adjudicate existing ones
 - Do not write any files
-- Err on the side of keeping findings when evidence is ambiguous
+- Resolve ambiguous evidence per "Resolving ambiguity" above: keep the
+  finding when it is unclear whether the code is wrong, or when you could
+  not inspect what a value reaches; downgrade to `info` only when you saw
+  the evidence of safety yourself

skills/pr-review/sub-agents/correctness.md

@@ -106,3 +106,43 @@ based on common patterns — read it.
 If the file cannot be read (e.g., it is in another repository or
 inaccessible), state that you were unable to verify the contents.
 Never present unverified file contents as fact in a finding.
+
+## What not to flag
+
+The shared non-issue classes in the review context apply. These are the
+ones this dimension gets wrong most often:
+
+- **Inputs the function cannot receive.** Before flagging a missing nil
+  check, bounds check, or type guard, read the call sites. If every
+  caller passes a value the compiler, the schema, or an earlier
+  validation already constrains, there is no defect. "A future caller
+  might" is not a finding.
+- **Error paths a caller handles.** An unchecked error at one level is
+  not a gap when the caller checks it, the function returns it up, or
+  the process is meant to die there. Trace one level out before flagging.
+- **Equivalent-logic rewrites.** "This could be a switch", "this loop
+  could be a map lookup", "extract this into a helper" — restructuring
+  correct code is not correctness. Structure belongs to
+  `intent-coherence`, naming to `style-conventions`.
+- **Unmeasured performance.** Allocation counts, extra passes, and
+  string concatenation in code that is not on a hot path, and where the
+  diff introduces no new complexity class, are not defects.
+- **Missing tests that exist elsewhere.** Search for the behavior by
+  name before claiming it is untested — coverage often lives in a
+  differently named file or a table-driven case. This narrows the
+  finding; it does not remove it: a **new** code path with no test
+  anywhere is still a genuine `missing-test`, and is one of the gaps
+  human reviewers most consistently catch that this dimension misses.
+- **Edge cases with no consequence.** An unusual input that produces a
+  slightly different but still correct result is not a bug. Enumerating
+  such cases is what a low-precision review looks like — the exhaustive
+  edge-case list is where false positives concentrate.
+
+**The counterweight:** when you do flag, construct the concrete failure.
+Name the input, walk it through the changed code, and state the wrong
+value or wrong behavior that comes out — an arithmetic change that
+returns a result 100× too large is a `high` finding even when the diff
+is one character, and the PR description claiming the change is safe is
+not evidence. Reasoning about concrete impact is precisely what
+distinguishes the findings this dimension gets right from the ones it
+invents.