diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index 0cbe27a18..7b33e4576 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.13.2", + "version": "0.13.3", "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", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index d1ed2601e..2e68a1049 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,36 @@ 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.13.3] + +### Fixed + +- **`babysit-readiness-gate` now credits classification rows per comment surface, closing a + fail-open where a stale classification could pass the gate past a live unclassified finding + (#642).** The gate blocks while source findings outnumber their per-finding classification rows. + The shared classifier counted a self-authored classification pipe-row in ANY comment, including + PR-level review-summary comments that are never thread-resolved. Because a review thread's + findings drop when it resolves (the lifetime-vs-open discount) but a PR-level comment can never + resolve, a stale classification posted outside a thread kept counting after its finding was + discounted — inflating the classified count past a fresh, still-unclassified open-thread finding + and emitting a fail-open `READINESS_OK`. Classification credit is now bucketed by surface + (review-thread, PR-level, and an isolated bucket for comments bearing no surface signal) and + capped within each bucket, so a classification can only offset a finding on its own surface. The + Python-free bash degrade gains the thread-state-free analogue (`classified = min(classified, + findings)`); the per-surface refinement is Python-only, mirroring the existing lifetime discount, + and stays convergent with the degrade on unsignalled input. + +### Changed + +- **BEHAVIOR FLIP — a PR whose inline-thread findings are answered only by detached PR-level + classification replies now reports `READINESS_BLOCKED` where it previously passed.** With + per-surface credit, a PR-level classification row no longer offsets an inline-thread finding, so + the gate blocks until each inline finding is answered on its own thread. This enforces + `review-discipline.md` §D5's already-ratified reply routing (inline findings MUST reply threaded, + "NEVER a detached `pr comment`") mechanically rather than by prose. Runs that already follow §D5 + routing are unaffected; only runs relying on the previously-tolerated detached-reply shape change + verdict, and the fix direction is fail-closed. + ## [0.13.2] ### Fixed diff --git a/plugins/source-control/scripts/babysit-readiness-gate.sh b/plugins/source-control/scripts/babysit-readiness-gate.sh index 0a7d82b47..4731b3750 100755 --- a/plugins/source-control/scripts/babysit-readiness-gate.sh +++ b/plugins/source-control/scripts/babysit-readiness-gate.sh @@ -26,7 +26,10 @@ # classified = TABLE ROWS (`|`-prefixed lines) carrying a classification # token (VALID|INCORRECT|UNCERTAIN) across all SELF replies — # one per line, so prose repetition never inflates the count. -# Word-boundary matched so "INVALID" does not count as "VALID" +# Word-boundary matched so "INVALID" does not count as "VALID". +# Capped at findings so a surplus of rows cannot mask an +# unclassified finding (the Python path refines this to a +# per-surface credit — see the classifier-preference note below). # BLOCK when findings > 0 AND classified < findings (under-decomposed / # unaddressed — R1+R5), OR when a --checklist file has any "- [ ]" (R6). # @@ -65,7 +68,7 @@ SELF_CSV="" EXTRA_SELF_CSV="" usage() { - sed -n '2,55p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//' + sed -n '2,58p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//' exit 0 } @@ -241,6 +244,15 @@ classified=${classified//[^0-9]/} findings=$((${sev_words:-0} + ${sev_badges:-0} + ${sev_plain:-0})) classified=${classified:-0} +# Cap classified at findings: a surplus of classification rows cannot offset +# findings that do not exist. This is the coarse, thread-state-free analogue of +# the Python counter's per-surface credit (#642) — the bash degrade has no +# reply-thread links, so it cannot bucket thread vs PR-level, but capping still +# stops an over-count of rows from masking an unclassified finding, and keeps +# the degrade count convergent with the Python `min(classified, findings)` on +# thread-state-free input. +((classified > findings)) && classified="$findings" + # --- Prefer the shared Python classifier when available ----------------------- # The bash counts above are the Python-free safe-tier degrade (reference/loop.md @@ -250,9 +262,12 @@ classified=${classified:-0} # can drift from the snapshot classifier (the divergence class #534 exists to # close), and it discounts a severity marker carried in a resolved or outdated # thread, so a lifetime badge no longer inflates the count into a false -# READINESS_BLOCKED (#465). A convergence test pins the two counts together on -# thread-state-free input; if the Python counter cannot run (no interpreter, or a -# transient live fetch failure) the bash degrade counts above stand. +# READINESS_BLOCKED (#465). It also credits classifications per surface — a +# PR-level (non-thread) row cannot offset a finding raised fresh in an open +# review thread — closing a fail-open the thread-blind bash degrade cannot see +# (#642). A convergence test pins the two counts together on thread-state-free +# input; if the Python counter cannot run (no interpreter, or a transient live +# fetch failure) the bash degrade counts above stand. # BABYSIT_READINESS_BASH_ONLY=1 forces the degrade even when Python is present -- # the operator escape that exercises (and, in the gate's own tests, pins) the # Python-free path deterministically. diff --git a/plugins/source-control/scripts/babysit-readiness-gate.test.sh b/plugins/source-control/scripts/babysit-readiness-gate.test.sh index 39a3b123e..432bb7fab 100755 --- a/plugins/source-control/scripts/babysit-readiness-gate.test.sh +++ b/plugins/source-control/scripts/babysit-readiness-gate.test.sh @@ -288,6 +288,62 @@ else pass "#465 lifetime discount skipped (no Python 3.11+; bash degrade counts lifetime)" fi +# --- Case: #642 stale PR-level classification does NOT cover an open-thread finding +# A classification pipe-row in a PR-level (non-thread) comment can never be +# thread-resolved, so it must not offset a finding raised fresh in an OPEN review +# thread. The Python counter credits classifications per surface, confining the +# stale row to the (empty) PR-level finding bucket -> classified=0 < findings=1 +# -> BLOCKED. Thread-aware, so asserted only under Python; the bash degrade is +# reply-thread-blind and false-passes here (the accepted degrade coarseness, same +# as the #465 discount above). +if probe_py py -3 || probe_py python3 || probe_py python; then + F=$(mkjson stale-pr-classification '[ + {author:"codex[bot]", body:"[CRITICAL] fresh unclassified finding", in_review_thread:true}, + {author:"me[bot]", body:"| 1 | old resolved finding | VALID | fixed |"} + ]') + r=$(run_gate "$F") + assert_contains "#642 stale PR-level row -> classified=0 (no cross-surface credit)" "$r" "findings=1 classified=0" + assert_contains "#642 stale PR-level row -> BLOCKED" "$r" "READINESS_BLOCKED reason=under-decomposed" +else + pass "#642 per-surface credit skipped (no Python 3.11+; bash degrade is thread-blind)" +fi + +# --- Case: #642 reuse path — fetch-all-pr-comments.sh `type` tags are surface-aware +# On `--comments-json` fed fetch-all-pr-comments.sh output, an inline finding is +# tagged `type:"inline"` and a detached PR-level classification `type:"review"`. +# The Python counter buckets by that tag, so the stale review-surface row cannot +# cross-credit the inline finding -> classified=0 < findings=1 -> BLOCKED. +# Thread-aware (tag-driven), so Python-gated; the bash degrade greps all bodies +# and false-passes here, the accepted degrade coarseness. +if probe_py py -3 || probe_py python3 || probe_py python; then + F=$(mkjson reuse-inline-type '[ + {type:"inline", author:"codex[bot]", body:"[CRITICAL] inline finding"}, + {type:"review", author:"me[bot]", body:"| 1 | old | VALID | fixed |"} + ]') + r=$(run_gate "$F") + assert_contains "#642 reuse-path inline tag -> classified=0" "$r" "findings=1 classified=0" + assert_contains "#642 reuse-path inline tag -> BLOCKED" "$r" "READINESS_BLOCKED reason=under-decomposed" +else + pass "#642 reuse-path inline tag skipped (no Python 3.11+; bash degrade is thread-blind)" +fi + +# --- Case: #642 fail-closed on unsignalled provenance ------------------------ +# A finding bearing neither an `in_review_thread` stamp nor a `type` tag is +# isolated in the unknown bucket, where a PR-level (`type:"review"`) +# classification row cannot offset it -> classified=0 < findings=1 -> BLOCKED. +# Defensive (production paths always signal); Python-gated like the cases above. +if probe_py py -3 || probe_py python3 || probe_py python; then + F=$(mkjson unsignalled-provenance '[ + {author:"codex[bot]", body:"[CRITICAL] unsignalled finding"}, + {type:"review", author:"me[bot]", body:"| 1 | x | VALID | y |"} + ]') + r=$(run_gate "$F") + assert_contains "#642 unsignalled finding -> classified=0" "$r" "findings=1 classified=0" + assert_contains "#642 unsignalled finding -> BLOCKED" "$r" "READINESS_BLOCKED reason=under-decomposed" +else + pass "#642 unsignalled-provenance isolation skipped (no Python 3.11+)" +fi + # --- Convergence: Python counter and bash degrade agree on thread-state-free input # The gate prefers the shared Python counter but keeps the bash grep counting as # the Python-free safe-tier degrade. The two must not drift: a severity marker is @@ -330,5 +386,13 @@ F=$(mkjson conv-selfrow '[ {author:"me[bot]", body:"| 1 | CRITICAL: null deref | VALID | fixed abc123 |"} ]') converge "self-row-exclusion" "$F" +# Over-classified, thread-state-free: the Python per-surface credit collapses to +# min(classified, findings) and the bash degrade's own cap does the same, so both +# report classified=1 for one finding + two rows (#642 cap convergence). +F=$(mkjson conv-overclassified '[ + {author:"claude[bot]", body:"CRITICAL a"}, + {author:"me[bot]", body:"| 1 | a | VALID | x |\n| 2 | spurious | INCORRECT | y |"} +]') +converge "over-classified-cap" "$F" [[ $FAILED -eq 0 ]] || exit 1 diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py index d2311792e..2fe07bc31 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py @@ -20,7 +20,9 @@ * Finding (severity occurrence + lifetime-vs-open state): the blocking-text and structured-severity heuristics, and the finding/classification counting the readiness gate delegates here (`count_findings` discounts markers carried in a - resolved or outdated thread, so a lifetime badge no longer inflates the count). + resolved or outdated thread, so a lifetime badge no longer inflates the count; + `count_effective_classified` credits classifications per surface so a stale + PR-level row cannot offset an open-thread finding). * Approval verdict: the approval / non-approval / required-fix heuristics and the structural approval and review-skip downgrades. """ @@ -357,6 +359,50 @@ def thread_is_open(comment: dict[str, Any]) -> bool: ) +# The surface a comment lives on -- not just its resolution state -- is +# load-bearing for classification credit (#642): a review thread can be resolved +# (its findings and their in-thread classifications drop together), while a +# PR-level comment never can, so a stale classification posted there would +# otherwise count forever. Credit is bucketed by these surfaces and capped within +# each, so a row on one surface cannot offset a finding on another. +THREAD_SURFACE = "thread" +PR_LEVEL_SURFACE = "pr_level" +UNKNOWN_SURFACE = "unknown" + + +def comment_surface(comment: dict[str, Any]) -> str: + """Classify the surface a comment lives on for per-surface credit (#642). + + Two signals identify the surface, in order of authority: + + * `in_review_thread` -- the explicit stamp the live entrypoint applies (true + for a review-thread comment, false for a PR-level one). When present it + decides, so a live PR-level comment is never re-inferred as a thread. + * `type` -- the `fetch-all-pr-comments.sh` schema tag on the + `--comments-json` reuse path: `inline` is a review thread, `general` / + `review` are the two PR-level surfaces. Honoring it keeps that path + surface-aware; without it an inline finding and a detached PR-level + classification share a bucket and the row cross-credits the finding -- the + #642 fail-open, on the reuse path. + + A comment bearing neither signal (the bash-degrade shape and legacy + `{author, body}` fixtures) is `UNKNOWN_SURFACE`: isolated in its own bucket so + its rows cannot offset -- and its findings cannot be offset by -- a known + surface. That is the fail-closed direction for unknown provenance and it + preserves the "no signal = PR-level lifetime" model `thread_is_open` + documents; uniform unsignalled input collapses to one bucket, identical to a + flat count. + """ + if "in_review_thread" in comment: + return THREAD_SURFACE if comment["in_review_thread"] else PR_LEVEL_SURFACE + comment_type = comment.get("type") + if comment_type == "inline": + return THREAD_SURFACE + if comment_type in ("general", "review"): + return PR_LEVEL_SURFACE + return UNKNOWN_SURFACE + + def _severity_occurrences(text: str) -> int: return ( len(SEVERITY_WORDS_RE.findall(text)) @@ -424,3 +470,43 @@ def count_classified( if PIPE_ROW_RE.search(line) and CLASSIFY_TOKEN_RE.search(line): total += 1 return total + + +def count_effective_classified( + comments: list[dict[str, Any]], self_logins: frozenset[str] +) -> int: + """Classifications that effectively cover findings, credited per surface. + + The readiness gate blocks while findings outnumber their classifications. A + raw global count lets a classification row on one surface offset a finding on + another: a stale pipe-row in a PR-level (non-thread) comment -- which can + never be thread-resolved -- would keep covering a finding raised fresh in an + open review thread, a fail-open past a live unclassified finding (#642). + + Credit is therefore bucketed by surface (`comment_surface`: review-thread, + PR-level, or isolated unknown) and capped within each bucket -- a + classification can only offset a finding on its own surface. Resolved/outdated + thread comments are already discounted by `thread_is_open` inside both + counters, so they contribute to no bucket. On unsignalled input (every comment + in one bucket, the bash-degrade shape) this collapses to + `min(classified, findings)`, keeping the Python count convergent with the bash + degrade's own cap. + """ + buckets: dict[str, list[dict[str, Any]]] = { + THREAD_SURFACE: [], + PR_LEVEL_SURFACE: [], + UNKNOWN_SURFACE: [], + } + for comment in comments: + if is_json_object(comment): + buckets[comment_surface(comment)].append(comment) + return sum(_capped_credit(bucket, self_logins) for bucket in buckets.values()) + + +def _capped_credit( + comments: list[dict[str, Any]], self_logins: frozenset[str] +) -> int: + return min( + count_classified(comments, self_logins), + count_findings(comments, self_logins), + ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_findings.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_findings.py index b3959c2e0..49482c80f 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_findings.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_findings.py @@ -43,7 +43,11 @@ import sys from typing import Any -from babysit_classify import count_classified, count_findings, normalize_self_logins +from babysit_classify import ( + count_effective_classified, + count_findings, + normalize_self_logins, +) from babysit_gh import ( fetch_issue_comments, fetch_pull_request_reviews, @@ -63,6 +67,7 @@ def _comment(author: Any, body: Any, thread: dict[str, Any] | None = None) -> di return { "author": _author_login(author), "body": str(body or ""), + "in_review_thread": thread is not None, "isResolved": bool(thread.get("isResolved")) if thread else False, "isOutdated": bool(thread.get("isOutdated")) if thread else False, } @@ -86,8 +91,11 @@ def fetch_live_comments(repo: str, number: int) -> list[dict[str, Any]]: """Build the finding corpus for a live PR across all three feedback surfaces. Issue-level comments and review summaries are not review threads, so they - carry no resolution state and always count. Inline review-thread comments - inherit their thread's `isResolved` / `isOutdated` so an already-addressed + carry no resolution state and always count; they are stamped as the PR-level + surface (`in_review_thread` false) so a classification posted there is + credited only against PR-level findings, never a fresh open-thread one + (#642). Inline review-thread comments are stamped `in_review_thread` and + inherit their thread's `isResolved` / `isOutdated`, so an already-addressed thread's severity markers are discounted. Resolved threads are included (`include_resolved=True`) precisely so they can be discounted rather than silently dropped. @@ -150,7 +158,7 @@ def main(argv: list[str] | None = None) -> int: print(f"babysit_findings: {type(exc).__name__}: {exc}", file=sys.stderr) return 2 findings = count_findings(comments, self_logins) - classified = count_classified(comments, self_logins) + classified = count_effective_classified(comments, self_logins) print(f"findings={findings} classified={classified}") return 0 diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py index ba9596ec5..38989501c 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py @@ -132,6 +132,148 @@ def test_resolved_thread_classification_is_discounted(self) -> None: ) +class EffectiveClassifiedTests(unittest.TestCase): + """#642: classification credit is bucketed by surface (review-thread vs + PR-level) so a stale PR-level pipe-row cannot offset an open-thread finding. + A resolved thread drops its finding and its in-thread classification + together; a PR-level comment never resolves, so its rows must be confined to + covering PR-level findings.""" + + SELF = bc.normalize_self_logins(["me[bot]"]) + + def test_stale_pr_level_row_does_not_cover_open_thread_finding(self) -> None: + # The exact #642 fail-open: a fresh finding raised in an OPEN review + # thread, plus a stale classification pipe-row in a PR-level (non-thread) + # comment. A raw global count reports classified=1 >= findings=1 and the + # gate false-passes; per-surface credit confines the PR-level row to the + # (empty) PR-level finding bucket, so effective classified is 0 < 1. + comments = [ + { + "author": "codex[bot]", + "body": "[CRITICAL] fresh unclassified finding", + "in_review_thread": True, + }, + { + "author": "me[bot]", + "body": "| 1 | old resolved finding | VALID | fixed |", + }, + ] + self.assertEqual(bc.count_findings(comments, self.SELF), 1) + # Raw counter still sees the stale row (documents the defect surface). + self.assertEqual(bc.count_classified(comments, self.SELF), 1) + # Per-surface credit closes the fail-open. + self.assertEqual(bc.count_effective_classified(comments, self.SELF), 0) + self.assertLess( + bc.count_effective_classified(comments, self.SELF), + bc.count_findings(comments, self.SELF), + ) + + def test_pr_level_finding_and_pr_level_classification_still_pass(self) -> None: + # The D5-mandated flow: issue/review-summary findings answered by a + # detached PR-level classification. Both are non-thread, so they share a + # bucket and the classification credits normally -- restricting credit to + # threads would have permanently blocked this path. + comments = [ + {"author": "claude[bot]", "body": "### 1. [CRITICAL] a\n### 2. [IMPORTANT] b"}, + { + "author": "me[bot]", + "body": "| 1 | a | VALID | fixed |\n| 2 | b | INCORRECT | refuted |", + }, + ] + self.assertEqual(bc.count_findings(comments, self.SELF), 2) + self.assertEqual(bc.count_effective_classified(comments, self.SELF), 2) + + def test_open_thread_finding_covered_by_in_thread_classification(self) -> None: + # A finding and its classification both carried in the same open thread + # balance within the thread bucket. + comments = [ + {"author": "codex[bot]", "body": "[CRITICAL] a", "in_review_thread": True}, + { + "author": "me[bot]", + "body": "| 1 | a | VALID | fixed |", + "in_review_thread": True, + }, + ] + self.assertEqual(bc.count_effective_classified(comments, self.SELF), 1) + + def test_thread_state_free_input_collapses_to_min(self) -> None: + # Convergence invariant: with no thread markers every comment is PR-level, + # so effective classified is min(classified, findings) -- what the bash + # degrade computes with its own cap. + over = [ + {"author": "claude[bot]", "body": "CRITICAL a"}, + { + "author": "me[bot]", + "body": "| 1 | a | VALID | x |\n| 2 | spurious | INCORRECT | y |", + }, + ] + self.assertEqual(bc.count_findings(over, self.SELF), 1) + self.assertEqual(bc.count_classified(over, self.SELF), 2) + self.assertEqual(bc.count_effective_classified(over, self.SELF), 1) + + def test_inline_type_tag_is_bucketed_as_thread_on_reuse_path(self) -> None: + # The `--comments-json` reuse path is fed `fetch-all-pr-comments.sh` + # output, which tags inline review comments `type: "inline"` but carries + # no `in_review_thread` stamp. Honoring the tag keeps that path + # surface-aware: an inline finding + a detached PR-level (`type: + # "review"`) classification row must not cross-credit (#642 on the reuse + # path). Without the tag inference both share the non-thread bucket and + # the row false-passes the finding. + comments = [ + {"type": "inline", "author": "codex[bot]", "body": "[CRITICAL] inline finding"}, + {"type": "review", "author": "me[bot]", "body": "| 1 | old | VALID | fixed |"}, + ] + self.assertEqual(bc.comment_surface(comments[0]), bc.THREAD_SURFACE) + self.assertEqual(bc.comment_surface(comments[1]), bc.PR_LEVEL_SURFACE) + self.assertEqual(bc.count_findings(comments, self.SELF), 1) + self.assertEqual(bc.count_effective_classified(comments, self.SELF), 0) + + def test_explicit_stamp_wins_over_type_tag(self) -> None: + # An explicit `in_review_thread: false` (a live PR-level comment) is + # authoritative even if a stray `type` is present -- it is never + # re-inferred as a thread. + self.assertEqual( + bc.comment_surface( + {"in_review_thread": False, "type": "inline", "body": ""} + ), + bc.PR_LEVEL_SURFACE, + ) + + def test_unsignalled_provenance_is_isolated_from_known_surfaces(self) -> None: + # Fail-closed defense: a finding bearing neither an `in_review_thread` + # stamp nor a `type` tag lands in the isolated unknown bucket, where a + # PR-level classification row cannot offset it, so the gate still blocks. + # (Production paths always signal; this guards a malformed snapshot.) + comments = [ + {"author": "codex[bot]", "body": "[CRITICAL] unsignalled finding"}, + {"type": "review", "author": "me[bot]", "body": "| 1 | x | VALID | y |"}, + ] + self.assertEqual(bc.comment_surface(comments[0]), bc.UNKNOWN_SURFACE) + self.assertEqual(bc.count_findings(comments, self.SELF), 1) + self.assertEqual(bc.count_effective_classified(comments, self.SELF), 0) + + def test_resolved_thread_contributes_to_neither_bucket(self) -> None: + # A resolved thread's finding and classification both drop (thread_is_open + # discount), so a fresh PR-level finding stays uncovered. + comments = [ + { + "author": "codex[bot]", + "body": "[CRITICAL] addressed last round", + "in_review_thread": True, + "isResolved": True, + }, + { + "author": "me[bot]", + "body": "| 1 | addressed | VALID | fixed |", + "in_review_thread": True, + "isResolved": True, + }, + {"author": "claude[bot]", "body": "### [IMPORTANT] new PR-level finding"}, + ] + self.assertEqual(bc.count_findings(comments, self.SELF), 1) + self.assertEqual(bc.count_effective_classified(comments, self.SELF), 0) + + class ApprovalVerdictTests(unittest.TestCase): """#499: an Approve-with-nits review carries no live finding.""" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_findings.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_findings.py index 5606034d9..462d603ef 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_findings.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_findings.py @@ -64,6 +64,55 @@ def test_untruncated_thread_collects_comments(self) -> None: self.assertEqual([c["body"] for c in comments], ["[CRITICAL] open"]) +class SurfaceStampingTests(unittest.TestCase): + """#642: the corpus must record which surface a comment lives on so the + classifier can credit classifications per surface. Thread comments are + stamped `in_review_thread`; issue-level and review-summary comments are + not.""" + + def test_thread_comments_stamped_pr_level_comments_not(self) -> None: + with mock.patch.object( + bf, "fetch_issue_comments", + return_value=[{"author": "human", "body": "issue-level note"}]), \ + mock.patch.object( + bf, "fetch_pull_request_reviews", + return_value=[{"author": "codex[bot]", "body": "review summary"}]), \ + mock.patch.object( + bf, "fetch_review_threads", + return_value=[_thread([_comment(body="[CRITICAL] inline")])]): + comments = bf.fetch_live_comments("owner/repo", 1) + by_body = {c["body"]: c["in_review_thread"] for c in comments} + self.assertEqual(by_body["issue-level note"], False) + self.assertEqual(by_body["review summary"], False) + self.assertEqual(by_body["[CRITICAL] inline"], True) + + +class Main642FailOpenTests(unittest.TestCase): + """End-to-end: a fresh finding in an OPEN review thread plus a stale + classification pipe-row in a PR-level review summary must BLOCK, not + false-pass. Before the per-surface credit the row inflated `classified` to + 1 == findings and `main` printed a passing count.""" + + def test_stale_pr_level_row_blocks_open_thread_finding(self) -> None: + buffer = io.StringIO() + with mock.patch.object(bf, "resolve_repo", return_value="owner/repo"), \ + mock.patch.object(bf, "fetch_issue_comments", return_value=[]), \ + mock.patch.object( + bf, "fetch_pull_request_reviews", + return_value=[{ + "author": "me[bot]", + "body": "| 1 | old resolved finding | VALID | fixed |", + }]), \ + mock.patch.object( + bf, "fetch_review_threads", + return_value=[_thread( + [_comment(body="[CRITICAL] fresh unclassified finding")])]): + with redirect_stdout(buffer): + code = bf.main(["--pr", "1", "--self", "me[bot]"]) + self.assertEqual(code, 0) + self.assertIn("findings=1 classified=0", buffer.getvalue()) + + class MainTruncationContractTests(unittest.TestCase): """The gate parses stdout: on truncation `main` must exit 2 and print NO `findings=` line, so the gate's regex misses and the bash count stands."""