source-control:babysit-prs: should formal APPROVED-state reviews be severity-scanned before the ignored short-circuit (bot AND human paths)

[kyle-sexton]

Lane: babysit. Triggering PR: #567 (fix/499-babysit-snapshot-approve-nits-classification). Source: Codex P1 re-review thread #567 (comment).

Finding

A reviewer bot can submit a formal APPROVED-state review whose body carries a live CRITICAL/IMPORTANT finding (e.g. Verdict: Approve overall. IMPORTANT: authorization bypass.). In collect_feedback (plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py) the elif state in {"APPROVED", "DISMISSED"}: ignored.append(record) branch short-circuits to ignored before the has_blocking_text / has_blocking_severity predicate is reached, so the severity marker is never inspected — no blocker, no worker dispatch. The same short-circuit exists on the non-bot (human) path a few lines above.

Why this is a deferred design question, not a PR #567 fix

• Pre-existing, not introduced by fix(source-control): babysit snapshot classifies Approve-with-nits bot review as non-blocking (#499) #567. Diffed against the PR's merge-base (789ec6c183): the elif state in {"APPROVED", "DISMISSED"}: ignored lines are unchanged context in that diff — PR fix(source-control): babysit snapshot classifies Approve-with-nits bot review as non-blocking (#499) #567 neither added nor modified the short-circuit. It only added severity detection on the *non-*APPROVED-state branch.
• Design-shaped. Making a formal APPROVED verdict yield a blocker based on body prose is a contract change to how explicit review states are treated, and it spans both the bot and human paths (the human path has the identical APPROVED/DISMISSED → ignored short-circuit). It carries a false-positive surface (descriptive severity words in an otherwise-clean approval) and interacts with the approval-downgrade heuristic.

Per the no-false-guarantees rule, PR #567 does not ship a present-tense guarantee this finding contradicts: its CHANGELOG/docstring wording was narrowed to the enforced behavior (severity stays blocking in a comment or non-APPROVED-state review), with this issue cited as the caveat. The classifier itself is left unchanged in #567.

The design question to decide

Should a formal APPROVED/DISMISSED-state review (bot and human) have its body severity-scanned before the ignored short-circuit — i.e. can a live CRITICAL/IMPORTANT in the body override the reviewer's explicit approval state? If yes, define the precedence and the false-positive guard; apply consistently across both paths.

Belongs to the #534 shared-classifier design space; likely umbrella-fold at triage.

[kyle-sexton]

This was generated by AI during triage.

Triage (umbrella-fold) — reporter self-flagged this as belonging to #534's shared-classifier design space (the APPROVED/DISMISSED short-circuit precedence question, bot and human paths). Folding rather than routing standalone per the umbrella-fold rule.

Blocked by #534's decision. Not routed to needs-human individually.

[kyle-sexton]

This was generated by AI during triage (historic normalization, rule 8g).

Reporter severity note (rule 5 sub-sort): this masks CRITICAL/IMPORTANT findings behind a formal APPROVED-state review — high consequence if it fires, even though tier stays priority: medium per rule 8c (source-control is a priority-set plugin, which affects ordering within the tier, not the tier itself).

The design question is genuinely open, not a single defaultable fork: should a formal APPROVED review ever short-circuit before severity inspection, or should severity markers always gate regardless of review state (and if so, on both the bot and human paths, which currently share the same short-circuit)? Reserved for a human call rather than defaulted.