diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index bd7e1dcc0..21fe2a3d6 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.11.0", + "version": "0.12.0", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-Authored-By trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop — safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention and babysit-prs config, or apply — interview the repo and write the tracked convention config), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable per repo via a tracked .claude/source-control.md config written by a re-runnable setup skill; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", @@ -82,7 +82,7 @@ "type": "string", "multiple": true, "title": "Babysit approval-downgrade reviewer logins", - "description": "AI reviewer logins whose APPROVED verdict is downgraded when the review body still carries blocking language. Absent: the downgrade heuristic stays dormant." + "description": "AI reviewer logins whose approval is surfaced as a `material` finding instead of `ignored` in the one case the structural approval-downgrade reaches: a review body carrying blocking-looking prose that still parses as an approval verdict (no CRITICAL/IMPORTANT or required-fix marker). Every bot's such approval is downgraded to non-blocking regardless; naming a login opts its own into the more-conservative `material` bucket rather than being ignored. Does not affect a review already in the APPROVED state or a plain clean approval with no blocking-looking prose — both are ignored regardless. Absent: such approvals are ignored for every bot." }, "babysit_skip_downgrade_logins": { "type": "string", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 1022af4c2..f3df12ae1 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,35 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.12.0] + +### Fixed + +- **`babysit-prs` snapshot no longer classifies an Approve-with-nits bot review as blocking bot + feedback.** A `claude[bot]` PR review posted as an issue-level comment with an explicit + **Approve** verdict and only 🟡-nit findings (no `CRITICAL`/`IMPORTANT` or other severity + marker) was surfaced as a blocking, genuinely-fresh finding because the body's prose contained + the word "blocking" ("blocking criteria", "blocking checks", "No blocking issues"), which the + text heuristic matched. The classifier now parses the verdict and severity markers: an explicit + approval carrying no genuine severity marker is downgraded structurally (for any bot, not only a + configured login) to a non-blocking result, consistent with `babysit-readiness-gate.sh` + reporting `findings=0` for the same review. Detection of genuinely blocking feedback is + unweakened — in a comment or a non-`APPROVED`-state review, a `CRITICAL`/`IMPORTANT` finding or + a Request-changes verdict still classifies as blocking, and `CRITICAL`/`IMPORTANT` are now + recognized as blocking-severity markers in their own right. (A review submitted in the formal + `APPROVED`/`DISMISSED` state is routed to `ignored` before the severity check — pre-existing + behavior this change does not alter; whether such reviews should be severity-scanned first is + tracked as a follow-up in #621.) A negated severity conclusion — a clean approval stating `No CRITICAL or IMPORTANT + findings` — is redacted before the severity check, the structured-marker analogue of the + existing `no P1/P2 issues` redaction, so introducing severity-marker detection does not itself + re-create a false blocker for that common clean-verdict phrasing. A login named in + `babysit_approval_downgrade_logins` opts that bot's approval into the more-conservative + `material` bucket (surfaced but non-blocking) instead of `ignored` in the one case the + structural downgrade reaches — a review body carrying blocking-looking prose that still parses + as an approval verdict. It does not affect a review already in the APPROVED state or a plain + clean approval whose body carries no blocking-looking prose: both are ignored regardless of the + setting, since neither reaches the downgrade branch. + ## [0.11.0] ### Added diff --git a/plugins/source-control/README.md b/plugins/source-control/README.md index 088cbeb43..c8b319c99 100644 --- a/plugins/source-control/README.md +++ b/plugins/source-control/README.md @@ -158,7 +158,7 @@ repo's owner. | `babysit_review_gate_context` | string | review gate treated as absent | | `babysit_ci_gateway_context` | string | gateway check unused | | `babysit_extra_bot_logins` | string (multiple) | structural bot detection only | -| `babysit_approval_downgrade_logins` | string (multiple) | downgrade heuristic dormant | +| `babysit_approval_downgrade_logins` | string (multiple) | an approval carrying blocking-looking prose is downgraded to ignored structurally (every bot); a named login instead surfaces its own as material. Real APPROVED-state reviews and plain clean approvals are ignored regardless. | | `babysit_skip_downgrade_logins` | string (multiple) | downgrade heuristic dormant | | `babysit_max_quiet_recheck_seconds` | number | 14400 | | `babysit_advisory_fix_round_cap` | number | 100 | diff --git a/plugins/source-control/skills/babysit-prs/SKILL.md b/plugins/source-control/skills/babysit-prs/SKILL.md index a3d5553c1..88eb6fb30 100644 --- a/plugins/source-control/skills/babysit-prs/SKILL.md +++ b/plugins/source-control/skills/babysit-prs/SKILL.md @@ -293,7 +293,7 @@ tier authority. | `babysit_review_gate_context` | `${user_config.babysit_review_gate_context}` | `--review-gate-context` (snapshot) | gate treated as absent | | `babysit_ci_gateway_context` | `${user_config.babysit_ci_gateway_context}` | `--ci-gateway-context` (snapshot) | gateway check unused | | `babysit_extra_bot_logins` | `${user_config.babysit_extra_bot_logins}` | `--extra-bot-logins` (snapshot) | structural bot detection only | -| `babysit_approval_downgrade_logins` | `${user_config.babysit_approval_downgrade_logins}` | `--approval-downgrade-logins` (snapshot) | downgrade heuristic dormant | +| `babysit_approval_downgrade_logins` | `${user_config.babysit_approval_downgrade_logins}` | `--approval-downgrade-logins` (snapshot) | an approval carrying blocking-looking prose is downgraded to ignored structurally (every bot); a named login instead surfaces its own as material. Real APPROVED-state reviews and plain clean approvals are ignored regardless. | | `babysit_skip_downgrade_logins` | `${user_config.babysit_skip_downgrade_logins}` | `--skip-downgrade-logins` (snapshot) | downgrade heuristic dormant | | `babysit_max_quiet_recheck_seconds` | `${user_config.babysit_max_quiet_recheck_seconds}` | `--max-quiet-recheck-seconds` (snapshot) | `14400` | | `babysit_advisory_fix_round_cap` | `${user_config.babysit_advisory_fix_round_cap}` | `--fix-round-cap` (snapshot, ledger) | `100` | diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py index e63aaf0e3..e53d85aaa 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py @@ -23,6 +23,7 @@ BLOCKING_TEXT_RE = re.compile( r"\b(p0|p1|p2|high[- ]severity|not approving|changes requested|" + + r"request(?:s|ing)? changes|" + r"required fix|must fix|blocking|regression|vulnerability)\b", re.I, ) @@ -35,6 +36,19 @@ + r"(?:issues?|findings?|defects?|problems?|regressions?|vulnerabilities?)\b", re.I, ) +# Negated CRITICAL/IMPORTANT conclusions ("No CRITICAL or IMPORTANT findings", +# "No CRITICAL issues found") are the structured-severity analogue of +# NEGATED_SEVERITY_LIST_RE: a clean approval stating the absence of high-severity +# findings, not a live one. The severity tokens stay case-sensitive (uppercase +# only) for the same reason BLOCKING_SEVERITY_RE is -- lowercase "critical"/ +# "important" are ordinary prose -- while the negator and trailing noun are not. +NEGATED_SEVERITY_MARKER_RE = re.compile( + r"(?i:\b(?:no|zero|without)\s+(?:actionable\s+)?)" + + r"(?:CRITICAL|IMPORTANT)" + + r"(?:(?i:\s*,?\s*(?:(?:and|or)\s+)?)(?:CRITICAL|IMPORTANT))*" + + r"(?i:\s+(?:issues?|findings?|defects?|problems?|regressions?|" + + r"vulnerabilities?))\b" +) NEGATED_BLOCKING_TERM_RE = re.compile( r"\b(?:no|zero|without)\s+(?:actionable\s+)?(?:p[012]|high[- ]severity|" + r"blocking|regressions?|vulnerabilities?|required fixes?|changes requested)\b|" @@ -61,6 +75,15 @@ + r"request(?:s|ing)? changes|regression|vulnerability|high[- ]severity)\b", re.I, ) +# Blocking-severity markers, case-sensitive and whole-word, matching the +# vocabulary babysit-readiness-gate.sh counts as findings (CRITICAL/IMPORTANT). +# Deliberately NOT case-insensitive: lowercase "critical"/"important" occur +# constantly in ordinary review prose ("it is important to note", "critical +# path"), whereas the uppercase tokens are the reviewer's structured severity +# labels. SUGGESTION is intentionally excluded here -- like a 🟡 nit it is a +# non-blocking marker -- so an approval carrying only suggestions/nits stays +# non-blocking, consistent with the issue's "CRITICAL/IMPORTANT vs nits" split. +BLOCKING_SEVERITY_RE = re.compile(r"\b(?:CRITICAL|IMPORTANT)\b") REVIEW_SKIP_RE = re.compile( r"\bbugbot\b[^\n.]{0,80}?\b(?:skipped|did(?:n't| not) run|" + r"could(?:n't| not) run|was not run|unable to run|usage limit)\b" @@ -85,9 +108,18 @@ class FeedbackConfig: """Caller-supplied identity configuration; every set ships empty. `extra_bot_logins` supplements structural bot detection for accounts whose - metadata misreports them as users. `approval_downgrade_logins` and - `skip_downgrade_logins` name the reviewer logins whose blocking-looking - text may be downgraded by the approval-verdict and review-skip heuristics. + metadata misreports them as users. A clean approval (explicit approval + verdict, no CRITICAL/IMPORTANT or required-fix marker) is treated as + non-blocking for every bot structurally. `approval_downgrade_logins` names + the reviewer logins whose approval is surfaced as a material finding rather + than ignored in the one case the structural downgrade reaches: a review body + carrying blocking-looking prose that still parses as an approval verdict. It + does not affect a review already in the APPROVED state or a plain clean + approval whose body carries no blocking-looking prose -- both are ignored + regardless, since neither reaches the downgrade branch. + `skip_downgrade_logins` names the reviewer logins + whose not-approving text may be downgraded to material when their review + provably could not run. """ extra_bot_logins: frozenset[str] = field(default_factory=frozenset) @@ -228,19 +260,41 @@ def has_blocking_text(text: str) -> bool: return bool(BLOCKING_TEXT_RE.search(redacted)) +def has_blocking_severity(text: str) -> bool: + """True when a CRITICAL/IMPORTANT severity marker survives negation redaction. + + A companion to `has_blocking_text` for the structured severity vocabulary + the readiness gate counts as findings. A bot review that raises a genuine + high-severity finding is blocking even when its prose contains none of + `BLOCKING_TEXT_RE`'s imperative terms. + """ + redacted = NEGATED_SEVERITY_LIST_RE.sub("", text) + redacted = NEGATED_SEVERITY_MARKER_RE.sub("", redacted) + redacted = NEGATED_BLOCKING_TERM_RE.sub("", redacted) + return bool(BLOCKING_SEVERITY_RE.search(redacted)) + + def approval_downgrade(text: str) -> bool: """True when a reviewer bot states an explicit approval verdict. Requires a clear approval/non-blocking conclusion, no negated approval - language, and no required-fix language surviving negation redaction. - Anything ambiguous stays blocking. + language, and neither a required-fix term nor a CRITICAL/IMPORTANT severity + marker surviving negation redaction. Anything ambiguous -- and any genuine + high-severity finding carried in an approval-verdict body that reaches this + check -- stays blocking. A review submitted in the formal APPROVED/DISMISSED + state is routed to `ignored` upstream, before this predicate; whether that + short-circuit should be severity-scanned first is tracked as a follow-up in + issue #621. """ if NON_APPROVAL_RE.search(text): return False if not APPROVAL_VERDICT_RE.search(text): return False redacted = NEGATED_SEVERITY_LIST_RE.sub("", text) + redacted = NEGATED_SEVERITY_MARKER_RE.sub("", redacted) redacted = NEGATED_BLOCKING_TERM_RE.sub("", redacted) + if BLOCKING_SEVERITY_RE.search(redacted): + return False return not REQUIRED_FIX_RE.search(redacted) @@ -250,14 +304,15 @@ def skip_downgrade(text: str) -> bool: A not-approving comment whose stated reason is a skipped review run (for example a usage limit) and which carries no findings of its own is a - user-triage item, not a code blocker. Any residual blocking language keeps - it blocking. + user-triage item, not a code blocker. Any residual blocking language -- + imperative blocking text or a surviving CRITICAL/IMPORTANT severity marker + -- keeps it blocking. """ if not REVIEW_SKIP_RE.search(text): return False remainder = NOT_APPROVING_RE.sub("", text) remainder = REVIEW_SKIP_RE.sub("", remainder) - return not has_blocking_text(remainder) + return not has_blocking_text(remainder) and not has_blocking_severity(remainder) def collect_feedback( @@ -326,7 +381,7 @@ def collect_feedback( blocking.append(record) elif state in {"APPROVED", "DISMISSED"}: ignored.append(record) - elif has_blocking_text(text): + elif has_blocking_text(text) or has_blocking_severity(text): disposition = json_object((dispositions or {}).get(fid)) current_head = str(pr.get("headRefOid") or "") # Only honor a disposition recorded at the current head. If the PR has @@ -340,11 +395,24 @@ def collect_feedback( if disposition_applies: record["disposed_reason"] = str(disposition.get("reason") or "") material.append(record) - elif normalized_bot_login( - item - ) in approval_downgrade_logins and approval_downgrade(text): + elif approval_downgrade(text): + # An explicit approval verdict with no genuine severity marker + # (no CRITICAL/IMPORTANT, no required-fix term surviving negation + # redaction): the blocking-looking text is descriptive prose -- + # e.g. "blocking criteria", "blocking checks", "no blocking + # issues" -- not a live finding. This is structural, not + # login-gated: a clean approval reads the same from any bot, and + # this keeps the snapshot consistent with + # babysit-readiness-gate.sh reporting findings=0 for the very + # same review. A login named in `approval_downgrade_logins` opts + # that bot's clean approvals into the more-conservative `material` + # bucket (surfaced but non-blocking) instead of being fully + # ignored; the safe default for every other bot is `ignored`. record["downgrade"] = "approval_verdict" - material.append(record) + if normalized_bot_login(item) in approval_downgrade_logins: + material.append(record) + else: + ignored.append(record) elif normalized_bot_login( item ) in skip_downgrade_logins and skip_downgrade(text): diff --git a/plugins/source-control/skills/babysit-prs/scripts/pr_queue_snapshot.py b/plugins/source-control/skills/babysit-prs/scripts/pr_queue_snapshot.py index 413896ae1..b3eaf8c3b 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/pr_queue_snapshot.py +++ b/plugins/source-control/skills/babysit-prs/scripts/pr_queue_snapshot.py @@ -518,8 +518,15 @@ def main() -> int: "--approval-downgrade-logins", default=None, help=( - "Comma-separated reviewer-bot logins whose blocking-looking text " - "may downgrade on an explicit approval verdict (ships empty)." + "Comma-separated reviewer-bot logins whose approval is surfaced as a " + "material finding rather than ignored in the one case the structural " + "downgrade reaches: a review body carrying blocking-looking prose that " + "still parses as an approval verdict (no CRITICAL/IMPORTANT or " + "required-fix marker). Every bot's such approval is downgraded to " + "non-blocking by default; naming a login opts its own into the " + "more-conservative material bucket instead of ignored. Does not affect " + "a review already in the APPROVED state or a plain clean approval with " + "no blocking-looking prose -- both are ignored regardless (ships empty)." ), ) parser.add_argument( diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py index cee9228ca..4ab30ec90 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py @@ -112,6 +112,54 @@ def test_negated_severity_is_not_blocking(self) -> None: result = fb.collect_feedback(pr) self.assertEqual(result["blocking"], []) + def test_bot_approval_with_descriptive_blocking_prose_is_ignored(self) -> None: + # #499: an Approve verdict whose body uses the word "blocking" only + # descriptively ("blocking criteria"/"blocking checks") and carries no + # severity marker must be fully non-blocking, matching the readiness + # gate's findings=0 — not routed to blocking or even material. + pr = _pr([ + {"id": 20, "author": {"login": "claude", "__typename": "Bot"}, + "body": "Verdict: Approve. No blocking issues. Checked the REVIEW.md " + "blocking criteria; all six blocking checks are inapplicable. " + "\U0001F7E1 Nit: worth noting, not worth a change."} + ]) + result = fb.collect_feedback(pr) + self.assertEqual(result["blocking"], []) + self.assertEqual(result["material"], []) + self.assertEqual(len(result["ignored"]), 1) + self.assertEqual(result["ignored"][0]["downgrade"], "approval_verdict") + + def test_configured_login_surfaces_clean_approval_as_material(self) -> None: + # An opt-in `approval_downgrade_logins` login routes the same clean + # approval to the more-conservative material bucket instead of ignored. + comment = {"id": 21, "author": {"login": "claude", "__typename": "Bot"}, + "body": "Verdict: Approve. Checked every blocking criterion; " + "all blocking checks are inapplicable."} + config = fb.FeedbackConfig(approval_downgrade_logins=frozenset({"claude"})) + result = fb.collect_feedback(_pr([comment]), config=config) + self.assertEqual(result["blocking"], []) + self.assertEqual(len(result["material"]), 1) + self.assertEqual(result["material"][0]["downgrade"], "approval_verdict") + + def test_approval_verdict_with_critical_marker_stays_blocking(self) -> None: + pr = _pr([ + {"id": 22, "author": {"login": "claude", "__typename": "Bot"}, + "body": "Verdict: Approve overall. \U0001F534 CRITICAL: hardcoded " + "secret; must fix before merge."} + ]) + result = fb.collect_feedback(pr) + self.assertEqual(len(result["blocking"]), 1) + self.assertEqual(result["material"], []) + + def test_request_changes_verdict_is_blocking(self) -> None: + pr = _pr([ + {"id": 23, "author": {"login": "claude", "__typename": "Bot"}, + "body": "Verdict: Request changes. The endpoint skips the tenant " + "scope check."} + ]) + result = fb.collect_feedback(pr) + self.assertEqual(len(result["blocking"]), 1) + def test_disposition_only_applies_at_matching_head(self) -> None: comment = {"id": 6, "author": {"login": "bugbot[bot]", "__typename": "Bot"}, "body": "P1 must fix regression."} @@ -202,12 +250,66 @@ def test_approval_downgrade_requires_clear_verdict_without_required_fix(self) -> self.assertFalse(fb.approval_downgrade("Approved, but P1 must fix remains.")) self.assertFalse(fb.approval_downgrade("Not approving this change yet.")) + def test_approval_downgrade_rejects_critical_or_important_marker(self) -> None: + self.assertFalse(fb.approval_downgrade("Approve, but CRITICAL: fix this.")) + self.assertFalse(fb.approval_downgrade("Approve. IMPORTANT: revisit.")) + # Lowercase severity words are ordinary prose, not markers. + self.assertTrue( + fb.approval_downgrade("Approve; it is important to note the nit.") + ) + + def test_has_blocking_severity_matches_only_uppercase_markers(self) -> None: + self.assertTrue(fb.has_blocking_severity("CRITICAL: null deref")) + self.assertTrue(fb.has_blocking_severity("IMPORTANT: revisit this")) + self.assertFalse(fb.has_blocking_severity("this is a critical path")) + self.assertFalse(fb.has_blocking_severity("SUGGESTION: rename the var")) + + def test_negated_severity_marker_is_not_blocking(self) -> None: + # A clean approval stating the absence of high-severity findings uses the + # same structured vocabulary in its verdict; it must not re-introduce a + # false blocker (the mirror of no-P1/P2 redaction for CRITICAL/IMPORTANT). + self.assertFalse( + fb.has_blocking_severity("No CRITICAL or IMPORTANT findings.") + ) + self.assertFalse(fb.has_blocking_severity("No CRITICAL issues found.")) + self.assertTrue( + fb.approval_downgrade( + "Verdict: Approve. No CRITICAL or IMPORTANT findings." + ) + ) + # A live marker without a negator still blocks. + self.assertTrue(fb.has_blocking_severity("CRITICAL: null deref remains")) + def test_skip_downgrade_only_when_review_could_not_run(self) -> None: self.assertTrue( fb.skip_downgrade("bugbot skipped: usage limit reached. Not approving.") ) self.assertFalse(fb.skip_downgrade("Not approving; P1 regression found.")) + def test_skip_downgrade_rejects_live_severity_marker(self) -> None: + # A skip comment carrying a genuine CRITICAL/IMPORTANT marker has no + # imperative blocking text, so has_blocking_text alone would let it + # downgrade; the severity guard keeps the live finding blocking. + self.assertFalse( + fb.skip_downgrade( + "bugbot skipped: usage limit reached. Not approving. " + "CRITICAL: authorization bypass." + ) + ) + self.assertFalse( + fb.skip_downgrade( + "bugbot skipped: usage limit reached. Not approving. " + "IMPORTANT: unbounded retry loop." + ) + ) + # A negated severity conclusion still downgrades -- it is not a finding. + self.assertTrue( + fb.skip_downgrade( + "bugbot skipped: usage limit reached. Not approving. " + "No CRITICAL or IMPORTANT issues seen before the cutoff." + ) + ) + if __name__ == "__main__": unittest.main() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_pr_queue_snapshot.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_pr_queue_snapshot.py index fec066930..1986d71ef 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_pr_queue_snapshot.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_pr_queue_snapshot.py @@ -147,6 +147,120 @@ def test_unresolvable_self_login_leaves_discovery_authors_intact(self) -> None: self.assertEqual(snapshot.resolve_self_logins(["alice"]), ["alice"]) +HEAD = "a" * 40 +OBS = "2026-07-19T16:10:00Z" + +# Faithful abbreviation of the claude[bot] issue-comment review captured from +# workflow run 29694104425: an explicit **Approve** verdict whose only findings +# are two 🟡 nits, each self-deprioritized, and no CRITICAL/IMPORTANT/P-severity +# marker anywhere. The load-bearing tokens are the descriptive occurrences of +# the word "blocking" ("blocking criteria", "blocking checks", "No blocking +# issues") that made the old text heuristic misfire. +APPROVE_WITH_NITS_BODY = ( + "**Claude finished @kyle-sexton's task** —— [View job](run/29694104425)\n\n" + "### PR Review\n\n" + "**Verdict: Approve** — clean, focused, well-reasoned patch. No blocking " + "issues. I checked each changed file against the REVIEW.md blocking " + "criteria — none of the security/authorization gates apply here.\n\n" + "### 🟡 Nit — parenthetical inside code fence\n" + "A first-time reader might misread the parenthetical. Not worth a change on " + "its own, but worth noting.\n\n" + "### 🟡 Nit — table cell verbosity\n" + "These files are AI-readable instruction documents, so this is low impact.\n\n" + "### No concerns on REVIEW.md blocking criteria\n" + "All six blocking checks (auth, tenancy, secrets, injection, audit logging, " + "atomicity) are inapplicable — a documentation-only change." +) +APPROVE_BUT_CRITICAL_BODY = ( + "**Verdict: Approve** overall, but one item stands out.\n\n" + "### 🔴 CRITICAL — hardcoded secret\n" + "A live credential is committed in config; must fix before merge." +) +REQUEST_CHANGES_BODY = ( + "**Verdict: Request changes** — the new endpoint skips the tenant scope " + "check, so cross-tenant reads are possible." +) + + +def _pr_with_claude_review(body: str) -> dict[str, object]: + return { + "repo": "melodic-software/claude-code-plugins", + "number": 492, + "url": "u", + "title": "docs patch", + "state": "OPEN", + "author": {"login": "kyle-sexton", "__typename": "User"}, + "headRefName": "feature", + "headRefOid": HEAD, + "baseRefName": "main", + "baseRefOid": "b" * 40, + "headRepository": {"nameWithOwner": "melodic-software/claude-code-plugins"}, + "headRepositoryOwner": {"login": "melodic-software"}, + "isCrossRepository": False, + "isDraft": False, + "maintainerCanModify": True, + "baseRepositoryArchived": False, + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "reviewDecision": "", + "reviews": [], + "latestReviews": [], + "comments": [ + {"id": 1, "author": {"login": "claude", "__typename": "Bot"}, "body": body} + ], + "statusCheckRollup": [], + "updatedAt": "2026-07-19T16:00:00Z", + } + + +class ApproveWithNitsClassification(unittest.TestCase): + """The snapshot classifier must agree with babysit-readiness-gate.sh. + + Reproduces melodic-software/claude-code-plugins#499: the gate reports + `READINESS_OK findings=0` for #492's Approve-with-nits review (no severity + marker present), while the snapshot classified the same review as a blocking + bot-feedback item because its prose contains the word "blocking". After the + fix the two agree: an Approve verdict carrying only non-blocking nits is + non-blocking, and a genuine CRITICAL finding or a Request-changes verdict + still blocks. + """ + + _CONFIG = delta.ClassifyConfig( + allowed_owners=frozenset({"melodic-software"}) + ) + + def _classify(self, body: str) -> dict[str, object]: + return delta.classify_pr( + _pr_with_claude_review(body), None, None, OBS, config=self._CONFIG + ) + + def test_approve_with_only_nits_is_not_blocking(self) -> None: + result = self._classify(APPROVE_WITH_NITS_BODY) + self.assertEqual(result["feedback"]["blocking"], []) + self.assertEqual(result["new_feedback"]["blocking"], []) + self.assertEqual(result["feedback"]["material"], []) + self.assertNotIn( + "1 blocking bot feedback item(s)", result["blockers"] + ) + # Consistent with the gate's findings=0: a clean approval is fully + # non-blocking, so a worker is never dispatched for it and the PR routes + # straight to the direct merge gate. + self.assertFalse(result["needs_worker"]) + self.assertTrue(result["pr_clean_ready_for_direct_gate"]) + + def test_approve_with_critical_finding_still_blocks(self) -> None: + result = self._classify(APPROVE_BUT_CRITICAL_BODY) + self.assertEqual(len(result["feedback"]["blocking"]), 1) + self.assertIn("1 blocking bot feedback item(s)", result["blockers"]) + self.assertTrue(result["needs_worker"]) + + def test_request_changes_verdict_still_blocks(self) -> None: + result = self._classify(REQUEST_CHANGES_BODY) + self.assertEqual(len(result["feedback"]["blocking"]), 1) + self.assertIn("1 blocking bot feedback item(s)", result["blockers"]) + self.assertTrue(result["needs_worker"]) + + class BuildConfigSelfLoginsTests(unittest.TestCase): def test_resolved_self_logins_populate_config_self_logins(self) -> None: args = argparse.Namespace(