diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index 21fe2a3d6a..e75728ed77 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.12.0", + "version": "0.13.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", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index f3df12ae13..a891929203 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,42 @@ 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.0] + +### Changed + +- **`babysit-prs` authorship / finding / approval classification is now one shared module.** The + self/bot/human authorship test, the finding severity + lifetime-vs-open counting, and the + approval-verdict heuristics were hand-rolled independently across the snapshot classifier, the + merge gate, the resolve-thread reporter, and the readiness gate, and the surfaces disagreed on + identical input — the six-issue misclassification class this refactor closes. They now consume + one classifier: `babysit_delta`, `babysit_feedback`, and `babysit_merge` import the self-login + membership test and authorship/finding/approval primitives directly instead of re-deriving them, + `babysit_resolve_thread` shares the same `is_bot` test, and `babysit-readiness-gate.sh` shells + out to the shared finding counter (mirroring the existing merge-gate wrapper) rather than + re-implementing the severity vocabulary in bash grep. Every surface stays a pure predicate with + no writes. Each formerly-divergent member issue is now a golden fixture, regression-proof by + construction. + +### Fixed + +- **`babysit-readiness-gate.sh` no longer over-counts lifetime findings as unaddressed.** The gate + counted every severity marker ever posted across a PR's lifetime — including markers in review + threads GitHub already reports resolved or outdated — so a fully-classified PR with re-review + history reported `READINESS_BLOCKED reason=under-decomposed` permanently even when every open + item was addressed. The shared finding counter discounts a marker carried in a resolved or + outdated thread, counting currently-open findings only. (De-duplicating the same concern restated + across re-review rounds within still-open threads is deliberately out of scope — there is no + reliable mechanical "same concern" signal — so restatements still count.) The bash counting is + retained only as the Python-free safe-tier degrade, which cannot see thread state; a convergence + test pins the two counts together on thread-state-free input. +- **`source-control-babysit-resolve-thread` no longer reports `humanThreadsActed` for a + Bot-authored thread.** The counter incremented for any acted thread whose comments were not + *all* bots (`botOnly` false), so a bot-opened thread carrying a later human reply was reported as + a human-thread action that never happened, undermining the human-thread safety rail's own + telemetry. It now counts only threads whose opening author is human, via the shared authorship + classifier — the same author check the `--include-human` eligibility decision already uses. + ## [0.12.0] ### Fixed diff --git a/plugins/source-control/scripts/babysit-readiness-gate.sh b/plugins/source-control/scripts/babysit-readiness-gate.sh index ab885e42ce..0a7d82b47c 100755 --- a/plugins/source-control/scripts/babysit-readiness-gate.sh +++ b/plugins/source-control/scripts/babysit-readiness-gate.sh @@ -241,6 +241,42 @@ classified=${classified//[^0-9]/} findings=$((${sev_words:-0} + ${sev_badges:-0} + ${sev_plain:-0})) classified=${classified:-0} +# --- Prefer the shared Python classifier when available ----------------------- + +# The bash counts above are the Python-free safe-tier degrade (reference/loop.md +# is that path, and it runs this gate). When a Python 3.11+ interpreter is +# present, re-count via the shared babysit_classify module instead: it owns the +# severity vocabulary as ONE source of truth rather than a second bash copy that +# 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. +# 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. +PY_SCRIPTS="$SCRIPT_DIR/../skills/babysit-prs/scripts" +if [[ "${BABYSIT_READINESS_BASH_ONLY:-}" != 1 && -f "$PY_SCRIPTS/babysit-python.sh" ]]; then + # shellcheck source=../skills/babysit-prs/scripts/babysit-python.sh + . "$PY_SCRIPTS/babysit-python.sh" + self_csv_joined="$( + IFS=, + printf '%s' "${SELF_LOGINS[*]}" + )" + if [[ -n "$COMMENTS_JSON" ]]; then + py_out="$(babysit_python "$PY_SCRIPTS/babysit_findings.py" \ + --comments-json "$COMMENTS_JSON" --self "$self_csv_joined" 2>/dev/null)" + else + py_out="$(babysit_python "$PY_SCRIPTS/babysit_findings.py" \ + --pr "$PR_NUMBER" --self "$self_csv_joined" 2>/dev/null)" + fi + if [[ "$py_out" =~ findings=([0-9]+)[[:space:]]+classified=([0-9]+) ]]; then + findings="${BASH_REMATCH[1]}" + classified="${BASH_REMATCH[2]}" + fi +fi + # --- R6: checklist completeness ---------------------------------------------- unticked=0 diff --git a/plugins/source-control/scripts/babysit-readiness-gate.test.sh b/plugins/source-control/scripts/babysit-readiness-gate.test.sh index b552a32fe4..39a3b123eb 100755 --- a/plugins/source-control/scripts/babysit-readiness-gate.test.sh +++ b/plugins/source-control/scripts/babysit-readiness-gate.test.sh @@ -262,4 +262,73 @@ r=$(run_gate "$F") assert_contains "out-of-range [P-num] -> findings=0" "$r" "findings=0" assert_contains "out-of-range [P-num] -> OK" "$r" "READINESS_OK" +# --- Case: #465 lifetime findings in resolved/outdated threads are discounted - +# A severity marker carried in a thread GitHub reports resolved or outdated is a +# lifetime artifact of an already-addressed round, not a live finding. The shared +# Python classifier (babysit_findings.py) discounts it (open-state aware) so a +# fully-classified PR with re-review history no longer false-BLOCKs. The bash +# degrade cannot see thread state and counts lifetime markers, so this enriched +# behavior is asserted only when a Python 3.11+ interpreter is present -- the same +# path the gate itself prefers. Three lifetime markers, only one open: findings=1. +probe_py() { + "$@" -c 'import sys; raise SystemExit(0 if sys.version_info[:2] >= (3, 11) else 1)' \ + >/dev/null 2>&1 +} +if probe_py py -3 || probe_py python3 || probe_py python; then + F=$(mkjson lifetime-open '[ + {author:"codex[bot]", body:"[CRITICAL] resolved earlier", isResolved:true}, + {author:"codex[bot]", body:"[CRITICAL] outdated round", isOutdated:true}, + {author:"codex[bot]", body:"[P1] still open null deref"}, + {author:"me[bot]", body:"| 1 | null deref | VALID | fixed abc123 |"} + ]') + r=$(run_gate "$F") + assert_contains "#465 lifetime discount -> findings=1 (only open)" "$r" "findings=1" + assert_contains "#465 lifetime discount -> READINESS_OK" "$r" "READINESS_OK" +else + pass "#465 lifetime discount skipped (no Python 3.11+; bash degrade counts lifetime)" +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 +# a finding under both, or the safe tier and the engine-backed tier disagree on +# readiness. BABYSIT_READINESS_BASH_ONLY=1 forces the degrade so both counts are +# observable in one run; every representative fixture must yield identical +# `findings=/classified=`. (On a host without Python both runs already take the +# bash path and agree trivially; the assertion still holds.) +gate_counts() { # gate_counts -> "findings=N classified=N" + bash "$GATE" 123 --comments-json "$1" --self 'me[bot]' 2>/dev/null | + grep -oE 'findings=[0-9]+ classified=[0-9]+' +} +converge() { # converge + local py bash_only + py="$(gate_counts "$2")" + bash_only="$(BABYSIT_READINESS_BASH_ONLY=1 gate_counts "$2")" + if [[ -n "$py" && "$py" == "$bash_only" ]]; then + pass "convergence [$1]: python == bash degrade ($py)" + else + fail "convergence [$1]: python == bash degrade" "$py" "$bash_only" + fi +} +F=$(mkjson conv-words '[ + {author:"claude[bot]", body:"CRITICAL a and IMPORTANT b on one line\nSUGGESTION c"}, + {author:"me[bot]", body:"| 1 | a | VALID | x |"} +]') +converge "severity-words" "$F" +F=$(mkjson conv-badge '[ + {author:"chatgpt-codex-connector[bot]", body:"![P1 Badge](https://img.shields.io/badge/P1-red?style=flat) and ![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat)"}, + {author:"me[bot]", body:"| 1 | x | VALID | y |"} +]') +converge "codex-badges" "$F" +F=$(mkjson conv-plain '[ + {author:"some-reviewer[bot]", body:"[P1] null deref\n[P2] missing timeout"}, + {author:"me[bot]", body:"| 1 | null deref | VALID | fixed |"} +]') +converge "plain-p-markers" "$F" +F=$(mkjson conv-selfrow '[ + {author:"claude[bot]", body:"CRITICAL null deref in handler"}, + {author:"me[bot]", body:"| 1 | CRITICAL: null deref | VALID | fixed abc123 |"} +]') +converge "self-row-exclusion" "$F" + [[ $FAILED -eq 0 ]] || exit 1 diff --git a/plugins/source-control/skills/babysit-prs/reference/orchestration.md b/plugins/source-control/skills/babysit-prs/reference/orchestration.md index 8571988b57..e571c69d4a 100644 --- a/plugins/source-control/skills/babysit-prs/reference/orchestration.md +++ b/plugins/source-control/skills/babysit-prs/reference/orchestration.md @@ -12,8 +12,11 @@ subdirectory of the plugin data directory. Spawn a fresh 1:1 worker for a PR **only when the snapshot's `needs_worker` field for that PR is `true`**. This is a deterministic engine output, not something to re-derive by eyeballing -`material_findings` text or the raw `classification`. Read it straight from the per-PR output of -the snapshot engine (see `needs_worker_reasons` for why): +`material_findings` text or the raw `classification`. The authorship, finding, and approval +classification behind those fields is one shared classifier locked by golden fixtures — the same +classifier the readiness gate and merge gate consume — so eyeballing it is strictly less reliable +than the field it would second-guess, not a safety check on top of it. Read it straight from the +per-PR output of the snapshot engine (see `needs_worker_reasons` for why): ```text python "${CLAUDE_PLUGIN_ROOT}/skills/babysit-prs/scripts/pr_queue_snapshot.py" --queue --author @me --owners --state-dir --write-state diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py new file mode 100755 index 0000000000..d2311792e4 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py @@ -0,0 +1,426 @@ +#!/usr/bin/env python3 +"""Shared authorship / finding / approval classifier for the babysit engine. + +Single source of truth extracted per issue #534. Every babysit surface that +must agree on "who authored this", "is this a live finding", and "is this an +approval" consumes these primitives instead of hand-rolling its own copy: +`babysit_feedback` (snapshot bot/human feedback bucketing), `babysit_delta` and +`babysit_merge` (self-login self-exemption), `babysit_resolve_thread` (opening +author typing), and `babysit-readiness-gate.sh` (finding decomposition counting, +via the `babysit_findings.py` entrypoint). Divergence among those surfaces was +the six-issue misclassification class this module exists to close. + +Leaf module: it depends only on `babysit_util`, never on the surfaces that +import it, so the dependency graph stays acyclic. + +Three concern areas: + +* Authorship (self / bot / human): `is_bot`, `actor_kind`, `normalize_login_set`, + self-login normalization/membership, and the dependency-manager author test. +* 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). +* Approval verdict: the approval / non-approval / required-fix heuristics and the + structural approval and review-skip downgrades. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass, field +from typing import Any + +from babysit_util import is_json_object + +# --- Finding + approval heuristics ------------------------------------------- + +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, +) +BOT_ERROR_RE = re.compile( + r"(encountered an error|could not|unable to|failed to run)", re.I +) +NEGATED_SEVERITY_LIST_RE = re.compile( + r"\b(?:no|zero|without)\s+(?:actionable\s+)?p[012]" + + r"(?:\s*,?\s*(?:(?:and|or)\s+)?p[012])*\s+" + + 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|" + + r"\bnot\s+(?:an?\s+)?(?:blocking|regression|vulnerability)\b", + re.I, +) +APPROVAL_VERDICT_RE = re.compile( + r"\bapproved?\b|\blgtm\b|\bready (?:for|to) merge\b|" + + r"\b(?:pr|change|changes|implementation|code) (?:is|are|looks?) sound\b|" + + r"\bnone of (?:the|these|my) (?:observations|findings|issues|comments|" + + r"suggestions) (?:is|are) blocking\b|" + + r"\bno blocking (?:issues?|findings?|defects?|problems?|concerns?)\b|" + + r"\bnothing blocking\b", + re.I, +) +NON_APPROVAL_RE = re.compile( + r"\b(?:not?|cannot|can't|won't|wouldn't|unable to|refus\w+|declin\w+|" + + r"do(?:es)?n't|isn't|aren't)\s+(?:be\s+|yet\s+)?" + + r"(?:approv\w+|ready|sound|mergeable)\b|\bnot ready\b", + re.I, +) +REQUIRED_FIX_RE = re.compile( + r"\b(?:p0|p1|required fix(?:es)?|must fix|fix required|changes requested|" + + 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" + + r"|\busage limit (?:reached|hit|exceeded)\b", + re.I, +) +NOT_APPROVING_RE = re.compile(r"\bnot approving\b", re.I) + +# Dependency-manager product bots (a tool taxonomy, not a tenant identity): +# their PRs feed the cross-tier hold-merge rule. +DEPENDENCY_MANAGER_LOGINS = frozenset( + { + "dependabot", + "dependabot-preview", + "renovate", + "renovate-bot", + } +) + + +@dataclass(frozen=True) +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. 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) + approval_downgrade_logins: frozenset[str] = field(default_factory=frozenset) + skip_downgrade_logins: frozenset[str] = field(default_factory=frozenset) + + +DEFAULT_FEEDBACK_CONFIG = FeedbackConfig() + + +# --- Authorship: self / bot / human ------------------------------------------ + + +def normalize_login_set(logins: Any) -> frozenset[str]: + """Normalize a login collection for comparison: casefold, strip `[bot]`.""" + return frozenset( + str(login).casefold().removesuffix("[bot]") + for login in (logins or []) + if str(login).strip() + ) + + +def normalize_self_logins(logins: Any) -> frozenset[str]: + """Casefold a self-login collection for membership tests. + + Unlike `normalize_login_set` this preserves any `[bot]` suffix: the self set + is matched against a comment's raw author login (which carries the suffix for + a bot posting identity), so stripping it here would make a personal login and + a same-stem bot login collide. Empty tokens are dropped so an unset config + yields an empty set and every self-gated behavior stays dormant. + """ + return frozenset( + str(login).casefold() for login in (logins or []) if str(login).strip() + ) + + +def is_self_login(login: Any, normalized_self: frozenset[str]) -> bool: + """True when `login` is one of the configured self identities (casefolded).""" + return str(login or "").casefold() in normalized_self + + +def author_login(item: dict[str, Any]) -> str: + author = item.get("author") + if is_json_object(author): + return str(author.get("login") or author.get("name") or "") + return str(author or "") + + +def is_bot( + login: object, typename: object, extra_bot_logins: Any = frozenset() +) -> bool: + """Bot iff GitHub's authoritative actor type says so, the login carries the + structural `[bot]` suffix every GitHub App bot account uses, or the login is + one the caller explicitly named a bot. + + Both structural signals come from the API and cannot go stale; the + `extra_bot_logins` fallback ships empty, so an unconfigured caller relies on + structure alone. + """ + if typename == "Bot": + return True + if not isinstance(login, str): + return False + if login.endswith("[bot]"): + return True + if not extra_bot_logins: + return False + return login.casefold().removesuffix("[bot]") in normalize_login_set( + extra_bot_logins + ) + + +def actor_kind( + item: dict[str, Any], config: FeedbackConfig = DEFAULT_FEEDBACK_CONFIG +) -> str: + """Classify actors from authoritative type metadata, then exact fallbacks.""" + author = item.get("author") + if is_json_object(author): + typename = str(author.get("__typename") or "") + if typename == "Bot" or author.get("is_bot") is True: + return "bot" + if ( + typename in {"Mannequin", "Organization", "User"} + or author.get("is_bot") is False + ): + return "human" + login = author_login(item).casefold() + return "bot" if is_bot(login, None, config.extra_bot_logins) else "human" + + +def normalized_bot_login(item: dict[str, Any]) -> str: + return author_login(item).casefold().removesuffix("[bot]") + + +def is_dependency_author(login: str) -> bool: + """Pure dependency-manager author test feeding the cross-tier hold-merge rule.""" + normalized = str(login or "").casefold().removeprefix("app/").removesuffix("[bot]") + return normalized in DEPENDENCY_MANAGER_LOGINS + + +def body_text(item: dict[str, Any]) -> str: + parts: list[str] = [] + for key in ("body", "bodyText", "state", "comment", "message"): + value = item.get(key) + if isinstance(value, str): + parts.append(value) + return "\n".join(parts) + + +# --- Finding heuristics ------------------------------------------------------- + + +def has_blocking_text(text: str) -> bool: + """Apply blocking heuristics after redacting common negated findings.""" + redacted = NEGATED_SEVERITY_LIST_RE.sub("", text) + redacted = NEGATED_BLOCKING_TERM_RE.sub("", redacted) + 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)) + + +# --- Approval verdict -------------------------------------------------------- + + +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 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) + + +def skip_downgrade(text: str) -> bool: + """True when a reviewer bot withholds approval only because its review + could not run. + + 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 -- + 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) and not has_blocking_severity(remainder) + + +# --- Finding decomposition counting (readiness gate) ------------------------- +# +# The readiness gate (babysit-readiness-gate.sh) delegates its finding and +# classification counts here so the severity vocabulary is defined once, not +# re-implemented in bash grep. The bash script retains an equivalent grep +# fallback ONLY for the Python-free safe-tier degrade (loop.md is that path); +# the convergence test pins the two counts together on thread-state-free +# fixtures. See babysit_findings.py for the CLI entrypoint. + +# Whole-word severity words (claude's vocabulary). Case-sensitive so lowercase +# priority labels (priority:p0-critical) and prose ("it is important") do not +# false-count -- the same rationale as BLOCKING_SEVERITY_RE, plus SUGGESTION. +SEVERITY_WORDS_RE = re.compile(r"\b(?:CRITICAL|IMPORTANT|SUGGESTION)\b") +# codex's shields.io P-severity badge: the `/badge/P{N}-` URL segment appears +# exactly once per finding (the badge's alt text carries the same P-number token +# a second time, so a bare `P[0-3]` would double-count). `/badge/P0-` never +# matches a lowercase priority:p0-critical label. +SEVERITY_BADGE_RE = re.compile(r"/badge/P[0-3]-") +# Plain bracketed P-severity markers ([P0]..[P3]) -- neither a severity word nor +# a shields badge. Bounded to the documented P0-P3 range so incidental [P4]+ +# text cannot inflate the count into a false READINESS_BLOCKED. +SEVERITY_PLAIN_RE = re.compile(r"\[P[0-3]\]") +CLASSIFY_TOKEN_RE = re.compile(r"\b(?:VALID|INCORRECT|UNCERTAIN)\b") +# A classification table row: a markdown `|`-prefixed line carrying a +# classification token bounded by non-letters (so "INVALID" rows still count as +# source content, not classifications). +CLASSIFY_ROW_RE = re.compile(r"^[ \t]*\|.*[^A-Za-z](?:VALID|INCORRECT|UNCERTAIN)(?:[^A-Za-z]|$)") +PIPE_ROW_RE = re.compile(r"^[ \t]*\|") + + +def thread_is_open(comment: dict[str, Any]) -> bool: + """False when a comment is carried in a resolved or outdated review thread. + + This is the lifetime-vs-open discriminator behind #465: a severity marker in + a thread GitHub reports `isResolved` or `isOutdated` is a lifetime artifact + of an already-addressed round, not a currently-open finding, so it must not + inflate the denominator. A comment with neither field set (issue-level or + review-summary comments, which are not review threads, and the bash-compatible + fixture shape) counts -- there is nothing to discount. + """ + return not ( + bool(comment.get("isResolved")) or bool(comment.get("isOutdated")) + ) + + +def _severity_occurrences(text: str) -> int: + return ( + len(SEVERITY_WORDS_RE.findall(text)) + + len(SEVERITY_BADGE_RE.findall(text)) + + len(SEVERITY_PLAIN_RE.findall(text)) + ) + + +def count_findings( + comments: list[dict[str, Any]], self_logins: frozenset[str] +) -> int: + """Count currently-open source-finding occurrences across every comment body. + + Ports the readiness gate's occurrence counting (one marker per finding, not + per line, so a multi-finding line is not under-counted) with two rules the + gate documents: + + * Self classification-table rows are excluded from the finding corpus so a + reply row that repeats the source severity word does not mint a phantom + finding. Non-table self content (a maintainer authoring a genuine source + finding) still counts. + * A marker carried in a resolved or outdated thread does not count + (`thread_is_open`), the #465 lifetime-vs-open discount. + """ + total = 0 + for comment in comments: + if not is_json_object(comment) or not thread_is_open(comment): + continue + author = str(comment.get("author") or "") + body = str(comment.get("body") or "") + if is_self_login(author, self_logins): + body = _strip_classification_rows(body) + total += _severity_occurrences(body) + return total + + +def _strip_classification_rows(body: str) -> str: + return "\n".join( + line for line in body.splitlines() if not CLASSIFY_ROW_RE.search(line) + ) + + +def count_classified( + comments: list[dict[str, Any]], self_logins: frozenset[str] +) -> int: + """Count classification table rows across self comment bodies. + + A classification is a markdown `|`-prefixed line carrying a + VALID/INCORRECT/UNCERTAIN token, counted one per line so prose repetition + never inflates the count. Only self authors' bodies contribute -- the + classification reply is the self surface's mandated per-finding format. + + A classification carried in a resolved or outdated thread is a lifetime + artifact of an already-addressed round (`thread_is_open`), mirroring + `count_findings`'s discount -- otherwise a stale classification could keep + inflating the denominator against a fresh, still-unclassified finding. + """ + total = 0 + for comment in comments: + if not is_json_object(comment) or not thread_is_open(comment): + continue + if not is_self_login(str(comment.get("author") or ""), self_logins): + continue + for line in str(comment.get("body") or "").splitlines(): + if PIPE_ROW_RE.search(line) and CLASSIFY_TOKEN_RE.search(line): + total += 1 + return total diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py index bcc5ebece6..764ab8d6ec 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py @@ -17,13 +17,15 @@ classify_checks, persisted_check_identity_keys, ) -from babysit_feedback import ( +from babysit_classify import ( DEFAULT_FEEDBACK_CONFIG, FeedbackConfig, actor_kind, author_login, - collect_feedback, + is_self_login, + normalize_self_logins, ) +from babysit_feedback import collect_feedback from babysit_gh import find_open_prs_for_head_ref from babysit_review_trigger import ( DEFAULT_REVIEW_TRIGGER_CONFIG, @@ -211,7 +213,7 @@ def detect_foreign_activity( the arm is dormant. """ recognizer = trigger_regex(config.review_trigger.trigger_phrase) - self_logins = {login.casefold() for login in config.self_logins if login} + self_logins = normalize_self_logins(config.self_logins) if recognizer is None or not self_logins: return {"detected": False, "evidence": []} prior = json_object((previous or {}).get("review_trigger")) @@ -224,7 +226,7 @@ def detect_foreign_activity( for comment in json_array(pr.get("comments")): if not is_json_object(comment): continue - if author_login(comment).casefold() not in self_logins: + if not is_self_login(author_login(comment), self_logins): continue if not recognizer.fullmatch(str(comment.get("body") or "")): continue @@ -269,7 +271,7 @@ def detect_attribution_drift( unconfigured classifier never fires false positives. """ intended = config.intended_write_identity.casefold() - self_logins = {login.casefold() for login in config.self_logins if login} + self_logins = normalize_self_logins(config.self_logins) if not intended or not self_logins: return {"detected": False, "evidence": []} prior = json_object((previous or {}).get("review_trigger")) @@ -293,7 +295,7 @@ def detect_attribution_drift( # drift (the degrade case). A non-self author on a ledgered id is not # this arm's concern -- foreign-activity semantics, and structurally # unreachable for an immutable comment we posted. - if landed_cf == intended or landed_cf not in self_logins: + if landed_cf == intended or not is_self_login(landed, self_logins): continue evidence.append( { @@ -436,7 +438,7 @@ def classify_pr( # gate. Filtering it there instead would silently strip the solo maintainer's # ability to human-stop their own PR. Here it only stops re-dispatching a # worker onto the engine's own prior output. - self_logins = {login.casefold() for login in config.self_logins if login} + self_logins = normalize_self_logins(config.self_logins) new_blocking_feedback = [ item for item in feedback["blocking"] if item["id"] not in prev_blocking_ids ] @@ -479,7 +481,7 @@ def classify_pr( item for item in (*feedback["human_blocking"], *feedback["human"]) if item["id"] not in prev_human_ids - and str(item.get("author") or "").casefold() not in self_logins + and not is_self_login(item.get("author"), self_logins) ] changed = bool(prev) and ( prev.get("head_sha") != head_sha or prev.get("updated_at") != updated_at @@ -747,7 +749,7 @@ def classify_pr( item for item in feedback["human_blocking"] if item["id"] not in prev_human_blocking_ids - and str(item.get("author") or "").casefold() not in self_logins + and not is_self_login(item.get("author"), self_logins) ] # Symmetric to `resolved_failing_checks`: a PR previously blocked only by # human feedback (CHANGES_REQUESTED, an unresolved inline thread) that the 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 e53d85aaa3..76c1cafe96 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py @@ -4,16 +4,39 @@ Actor typing is structural first (`__typename`/`is_bot`/`[bot]` suffix); any login-based fallback comes from caller-supplied configuration and ships empty. Downgrade heuristics likewise apply only to reviewer logins the caller names. + +The authorship / finding / approval primitives live in the shared +`babysit_classify` module (extracted per #534 so every babysit surface agrees); +this module orchestrates them into the snapshot's feedback buckets. The moved +names are re-exported here so existing consumers keep importing them from +`babysit_feedback`. """ from __future__ import annotations import hashlib import json -import re -from dataclasses import dataclass, field from typing import Any +from babysit_classify import ( + BOT_ERROR_RE, + DEFAULT_FEEDBACK_CONFIG, + DEPENDENCY_MANAGER_LOGINS, + FeedbackConfig, + actor_kind, + approval_downgrade, + author_login, + body_text, + has_blocking_severity, + has_blocking_text, + is_bot, + is_dependency_author, + is_self_login, + normalize_login_set, + normalize_self_logins, + normalized_bot_login, + skip_downgrade, +) from babysit_gh import ( fetch_issue_comments, fetch_pull_request_reviews, @@ -21,171 +44,31 @@ ) from babysit_util import is_json_object, json_array, json_object -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, -) -BOT_ERROR_RE = re.compile( - r"(encountered an error|could not|unable to|failed to run)", re.I -) -NEGATED_SEVERITY_LIST_RE = re.compile( - r"\b(?:no|zero|without)\s+(?:actionable\s+)?p[012]" - + r"(?:\s*,?\s*(?:(?:and|or)\s+)?p[012])*\s+" - + 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|" - + r"\bnot\s+(?:an?\s+)?(?:blocking|regression|vulnerability)\b", - re.I, -) -APPROVAL_VERDICT_RE = re.compile( - r"\bapproved?\b|\blgtm\b|\bready (?:for|to) merge\b|" - + r"\b(?:pr|change|changes|implementation|code) (?:is|are|looks?) sound\b|" - + r"\bnone of (?:the|these|my) (?:observations|findings|issues|comments|" - + r"suggestions) (?:is|are) blocking\b|" - + r"\bno blocking (?:issues?|findings?|defects?|problems?|concerns?)\b|" - + r"\bnothing blocking\b", - re.I, -) -NON_APPROVAL_RE = re.compile( - r"\b(?:not?|cannot|can't|won't|wouldn't|unable to|refus\w+|declin\w+|" - + r"do(?:es)?n't|isn't|aren't)\s+(?:be\s+|yet\s+)?" - + r"(?:approv\w+|ready|sound|mergeable)\b|\bnot ready\b", - re.I, -) -REQUIRED_FIX_RE = re.compile( - r"\b(?:p0|p1|required fix(?:es)?|must fix|fix required|changes requested|" - + 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" - + r"|\busage limit (?:reached|hit|exceeded)\b", - re.I, -) -NOT_APPROVING_RE = re.compile(r"\bnot approving\b", re.I) -# Dependency-manager product bots (a tool taxonomy, not a tenant identity): -# their PRs feed the cross-tier hold-merge rule. -DEPENDENCY_MANAGER_LOGINS = frozenset( - { - "dependabot", - "dependabot-preview", - "renovate", - "renovate-bot", - } -) - - -@dataclass(frozen=True) -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. 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) - approval_downgrade_logins: frozenset[str] = field(default_factory=frozenset) - skip_downgrade_logins: frozenset[str] = field(default_factory=frozenset) - - -DEFAULT_FEEDBACK_CONFIG = FeedbackConfig() - - -def normalize_login_set(logins: Any) -> frozenset[str]: - """Normalize a login collection for comparison: casefold, strip `[bot]`.""" - return frozenset( - str(login).casefold().removesuffix("[bot]") - for login in (logins or []) - if str(login).strip() - ) - - -def author_login(item: dict[str, Any]) -> str: - author = item.get("author") - if is_json_object(author): - return str(author.get("login") or author.get("name") or "") - return str(author or "") - - -def actor_kind( - item: dict[str, Any], config: FeedbackConfig = DEFAULT_FEEDBACK_CONFIG -) -> str: - """Classify actors from authoritative type metadata, then exact fallbacks.""" - author = item.get("author") - if is_json_object(author): - typename = str(author.get("__typename") or "") - if typename == "Bot" or author.get("is_bot") is True: - return "bot" - if ( - typename in {"Mannequin", "Organization", "User"} - or author.get("is_bot") is False - ): - return "human" - login = author_login(item).lower() - normalized = login.removesuffix("[bot]") - if login.endswith("[bot]") or normalized in normalize_login_set( - config.extra_bot_logins - ): - return "bot" - return "human" - - -def normalized_bot_login(item: dict[str, Any]) -> str: - return author_login(item).casefold().removesuffix("[bot]") - - -def is_dependency_author(login: str) -> bool: - """Pure dependency-manager author test feeding the cross-tier hold-merge rule.""" - normalized = str(login or "").casefold().removeprefix("app/").removesuffix("[bot]") - return normalized in DEPENDENCY_MANAGER_LOGINS - - -def body_text(item: dict[str, Any]) -> str: - parts: list[str] = [] - for key in ("body", "bodyText", "state", "comment", "message"): - value = item.get(key) - if isinstance(value, str): - parts.append(value) - return "\n".join(parts) +__all__ = [ + "BOT_ERROR_RE", + "DEFAULT_FEEDBACK_CONFIG", + "DEPENDENCY_MANAGER_LOGINS", + "FeedbackConfig", + "actor_kind", + "approval_downgrade", + "author_login", + "body_text", + "collect_feedback", + "fetch_current_human_stop", + "has_blocking_severity", + "has_blocking_text", + "human_stop_state", + "is_bot", + "is_dependency_author", + "is_self_login", + "item_id", + "latest_reviews_by_author", + "normalize_login_set", + "normalize_self_logins", + "normalized_bot_login", + "review_commit_oid", + "skip_downgrade", +] def item_id(prefix: str, item: dict[str, Any]) -> str: @@ -253,68 +136,6 @@ def latest_reviews_by_author( return [entry[1] for entry in latest.values()] + anonymous -def has_blocking_text(text: str) -> bool: - """Apply blocking heuristics after redacting common negated findings.""" - redacted = NEGATED_SEVERITY_LIST_RE.sub("", text) - redacted = NEGATED_BLOCKING_TERM_RE.sub("", redacted) - 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 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) - - -def skip_downgrade(text: str) -> bool: - """True when a reviewer bot withholds approval only because its review - could not run. - - 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 -- - 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) and not has_blocking_severity(remainder) - - def collect_feedback( pr: dict[str, Any], inline_comments: list[dict[str, Any]] | None = None, diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_findings.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_findings.py new file mode 100755 index 0000000000..b3959c2e06 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_findings.py @@ -0,0 +1,159 @@ +#!/usr/bin/env python3 +"""Finding-decomposition counting entrypoint for babysit-readiness-gate.sh. + +The readiness gate is a pure predicate that blocks a readiness declaration while +source findings outnumber their per-finding classification rows. Counting "what +is a finding" used to be a bash grep re-implementation of the severity +vocabulary the Python classifier already owns; #534 makes the gate shell out +here so the vocabulary lives once, in `babysit_classify`. The gate retains an +equivalent bash count solely for the Python-free safe-tier degrade (see +`reference/loop.md`); a convergence test pins the two on thread-state-free input. + +Counting only currently-open findings is the fix for #465: a severity marker +carried in a review thread GitHub reports resolved or outdated is a lifetime +artifact of an already-addressed round, not a live finding, and must not inflate +the denominator. That discount is mechanical (resolved/outdated thread state); +de-duplicating the same concern restated across re-review rounds is deliberately +out of scope (no reliable mechanical "same concern" signal), so restatements +within still-open threads still count. + +Input is either a comments JSON file (`--comments-json`, the gate's fixture / +network-free path; the bash-emitted `fetch-all-pr-comments.sh` schema, optionally +carrying per-comment `isResolved` / `isOutdated`) or a live PR (`--pr`), for +which this entrypoint fetches issue comments, review summaries, and review +threads (with resolution state) itself. + +Usage: + babysit_findings.py --comments-json --self + babysit_findings.py --pr --self [--repo ] + +Stdout (one line, parsed by the gate): + findings= classified= + +Exit codes: + 0 counts emitted + 2 runtime failure (fetch / parse) + 3 invalid argument +""" + +from __future__ import annotations + +import argparse +import json +import sys +from typing import Any + +from babysit_classify import count_classified, count_findings, normalize_self_logins +from babysit_gh import ( + fetch_issue_comments, + fetch_pull_request_reviews, + fetch_review_threads, + run_gh, +) +from babysit_util import configure_stdio, is_json_array, is_json_object + + +def _author_login(author: Any) -> str: + if is_json_object(author): + return str(author.get("login") or author.get("name") or "") + return str(author or "") + + +def _comment(author: Any, body: Any, thread: dict[str, Any] | None = None) -> dict[str, Any]: + return { + "author": _author_login(author), + "body": str(body or ""), + "isResolved": bool(thread.get("isResolved")) if thread else False, + "isOutdated": bool(thread.get("isOutdated")) if thread else False, + } + + +def load_comments_json(path: str) -> list[dict[str, Any]]: + with open(path, encoding="utf-8") as handle: + data = json.load(handle) + if not is_json_array(data): + raise ValueError("comments JSON must be an array") + return [item for item in data if is_json_object(item)] + + +def resolve_repo(explicit: str | None) -> str: + if explicit: + return explicit + return run_gh(["repo", "view", "--json", "nameWithOwner", "-q", ".nameWithOwner"]).strip() + + +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 + thread's severity markers are discounted. Resolved threads are included + (`include_resolved=True`) precisely so they can be discounted rather than + silently dropped. + + `fetch_review_threads` caps each thread's comment connection and flags an + oversized thread `comments_truncated` rather than raising, so one giant + thread cannot fail the whole snapshot for consumers that do not need every + comment. This counter DOES need every comment: a severity marker or + classification row past the cap that were dropped would under-count and let + the gate declare READINESS_OK while a later open finding sits unclassified. + So we honor that contract and fail closed on any truncated thread by + raising -- `main` maps this to exit 2, which emits no count line and leaves + the gate on its complete REST/bash degrade count (`reference/loop.md`). + """ + comments: list[dict[str, Any]] = [ + _comment(row.get("author"), row.get("body")) + for row in fetch_issue_comments(repo, number) + ] + comments.extend( + _comment(row.get("author"), row.get("body")) + for row in fetch_pull_request_reviews(repo, number) + ) + for thread in fetch_review_threads(repo, number, include_resolved=True): + if not is_json_object(thread): + continue + if thread.get("comments_truncated"): + raise RuntimeError( + f"review thread {thread.get('id')} has " + f"{thread.get('comments_total_count')} comments exceeding the " + "per-thread fetch cap; failing closed to the bash finding count" + ) + for node in thread.get("comments") or []: + if is_json_object(node): + comments.append(_comment(node.get("author"), node.get("body"), thread)) + return comments + + +def parse_args(argv: list[str]) -> argparse.Namespace: + parser = argparse.ArgumentParser(add_help=True) + source = parser.add_mutually_exclusive_group(required=True) + source.add_argument("--pr", type=int, help="live PR number to fetch and count") + source.add_argument("--comments-json", help="comments JSON file (no network)") + parser.add_argument("--self", dest="self_csv", default="", help="self logins, csv") + parser.add_argument("--repo", help="owner/repo (default: current repo)") + return parser.parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + configure_stdio() + args = parse_args(sys.argv[1:] if argv is None else argv) + self_logins = normalize_self_logins( + token for token in (args.self_csv or "").split(",") + ) + try: + if args.comments_json: + comments = load_comments_json(args.comments_json) + else: + comments = fetch_live_comments(resolve_repo(args.repo), args.pr) + except (OSError, ValueError, json.JSONDecodeError, RuntimeError) as exc: + 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) + print(f"findings={findings} classified={classified}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 3cc8053d36..3316df3384 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -40,7 +40,7 @@ from typing import Any, cast from babysit_checks import check_identity_key, classify_checks -from babysit_feedback import is_dependency_author +from babysit_classify import is_dependency_author, is_self_login, normalize_self_logins from babysit_gh import ( fetch_review_threads, gh_capture, @@ -150,7 +150,7 @@ def evaluate( number: int, expected_head: str | None, allowed: set[str], - self_logins: set[str], + self_logins: frozenset[str], allow_dependency: bool, allow_unprotected: bool, ) -> dict[str, Any]: @@ -284,7 +284,7 @@ def evaluate( ) # On an unprotected base, CLEAN proves nothing (no required checks/reviews). # A non-self author's PR there is held unless explicitly allowed. - author_is_self = str(author_login or "").casefold() in self_logins + author_is_self = is_self_login(author_login, self_logins) if base_is_unprotected and not author_is_self and not allow_unprotected: blockers.append( "base branch is unprotected (0 required reviews AND 0 required " @@ -458,16 +458,16 @@ def main() -> int: # resolution is a network call, and the guard's contract is that malformed # input is rejected before any network access. try: - self_logins = {login.casefold() for login in resolve_authors(args.self_logins)} + self_logins = normalize_self_logins(resolve_authors(args.self_logins)) except RuntimeError: # '@me' could not be resolved to a gh login; fail closed by keeping only # the explicit non-'@me' logins -- an unresolved self identity holds own # PRs on an unprotected base rather than merging on a guessed identity. - self_logins = { - token.casefold() + self_logins = normalize_self_logins( + token for token in (args.self_logins or "").split(",") - if token.strip() and token.strip().casefold() != "@me" - } + if token.strip().casefold() != "@me" + ) try: result = evaluate( diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py index 2338986bfa..80d3825c53 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py @@ -74,19 +74,11 @@ import json from typing import Any, cast +from babysit_classify import is_bot from babysit_gh import fetch_review_threads, gh_capture, parse_repo_number from babysit_util import configure_stdio, dig, is_json_object -def is_bot(login: object, typename: object) -> bool: - """Bot iff GitHub's authoritative actor type says so, or the login carries - the structural `[bot]` suffix every GitHub App bot account uses. No hardcoded - identity list -- both signals come from the API and cannot go stale.""" - if typename == "Bot": - return True - return isinstance(login, str) and login.endswith("[bot]") - - def _comment_author(comment: object) -> dict[str, object]: author: Any = (comment.get("author") if is_json_object(comment) else None) or {} return author if is_json_object(author) else {} @@ -471,11 +463,18 @@ def main() -> int: "action": "resolve" if args.resolve else "list", "onlyOutdated": args.only_outdated, "includeHuman": args.include_human, + # Count only threads whose OPENING author is human. `botOnly` + # (every comment in the thread is a bot) is the wrong gate: a + # bot-opened thread carrying a later human reply is `botOnly: + # false` yet was never a human's thread, so counting it here + # reported a human-thread action that never happened (#512). The + # opening-author test matches the `--include-human` eligibility + # decision, via the shared `is_bot` authorship classifier. "humanThreadsActed": len( [ r for r in results - if not r["botOnly"] + if not is_bot(r["author"], r["authorType"]) and r["action"] in ("would-resolve", "resolved") ] ), 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 new file mode 100644 index 0000000000..ba9596ec56 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py @@ -0,0 +1,158 @@ +"""Golden fixtures for the shared authorship / finding / approval classifier. + +`babysit_classify` is the single source of truth extracted per #534 so the +snapshot, the readiness gate, the merge gate, and resolve-thread cannot diverge +on "who authored this", "is this a live finding", and "is this an approval". One +fixture class per member concern of the umbrella: + +* Authorship (self / bot / human) -- `is_bot`, `normalize_self_logins`, + `is_self_login`; #497's empty-`self_logins` dormancy is a membership property. +* Finding lifetime-vs-open -- #465: `count_findings` discounts a severity marker + carried in a resolved or outdated thread. +* Approval verdict -- #499: an Approve-with-nits body carries no live finding. +""" + +from __future__ import annotations + +import pathlib +import sys +import unittest + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) + +import babysit_classify as bc + + +class IsBotTests(unittest.TestCase): + def test_authoritative_typename_is_a_bot(self) -> None: + self.assertTrue(bc.is_bot("ambiguous", "Bot")) + + def test_structural_bot_suffix_is_a_bot(self) -> None: + self.assertTrue(bc.is_bot("linter[bot]", "User")) + + def test_human_login_and_typename_is_not_a_bot(self) -> None: + self.assertFalse(bc.is_bot("robotics-fan", "User")) + + def test_extra_bot_login_fallback_when_named(self) -> None: + self.assertTrue(bc.is_bot("svc-account", "", {"svc-account"})) + self.assertFalse(bc.is_bot("svc-account", "")) + + def test_non_string_login_without_bot_typename_is_not_a_bot(self) -> None: + self.assertFalse(bc.is_bot(None, "User")) + + +class SelfLoginTests(unittest.TestCase): + def test_normalize_casefolds_and_keeps_bot_suffix(self) -> None: + # The self set is matched against a raw author login; stripping [bot] + # would collide a personal login with a same-stem bot posting identity. + self.assertEqual( + bc.normalize_self_logins(["Me", "Project-Bot[bot]", "", " "]), + frozenset({"me", "project-bot[bot]"}), + ) + + def test_membership_is_casefolded(self) -> None: + selves = bc.normalize_self_logins(["kyle-sexton", "bot[bot]"]) + self.assertTrue(bc.is_self_login("Kyle-Sexton", selves)) + self.assertTrue(bc.is_self_login("bot[bot]", selves)) + self.assertFalse(bc.is_self_login("someone-else", selves)) + + def test_empty_self_set_is_dormant(self) -> None: + # #497: an empty self set (single-`--pr` mode before the fix) matches + # nobody, so every self-gated behavior stays off rather than misfiring. + self.assertEqual(bc.normalize_self_logins([]), frozenset()) + self.assertFalse(bc.is_self_login("anyone", frozenset())) + + +class FindingLifetimeTests(unittest.TestCase): + """#465: only currently-open findings count toward decomposition.""" + + SELF = bc.normalize_self_logins(["me[bot]"]) + + def test_resolved_and_outdated_markers_are_discounted(self) -> None: + comments = [ + {"author": "codex[bot]", "body": "[CRITICAL] a", "isResolved": True}, + {"author": "codex[bot]", "body": "[CRITICAL] b", "isOutdated": True}, + {"author": "codex[bot]", "body": "[P1] c still open"}, + ] + self.assertEqual(bc.count_findings(comments, self.SELF), 1) + + def test_open_thread_markers_all_count(self) -> None: + comments = [ + {"author": "codex[bot]", "body": "CRITICAL a\nIMPORTANT b"}, + ] + self.assertEqual(bc.count_findings(comments, self.SELF), 2) + + def test_self_classification_rows_do_not_mint_phantom_findings(self) -> None: + comments = [ + {"author": "me[bot]", "body": "| 1 | CRITICAL null deref | VALID | fixed |"}, + ] + self.assertEqual(bc.count_findings(comments, self.SELF), 0) + + def test_self_source_finding_still_counts(self) -> None: + comments = [{"author": "me[bot]", "body": "Found a CRITICAL leak here"}] + self.assertEqual(bc.count_findings(comments, self.SELF), 1) + + +class ClassificationCountTests(unittest.TestCase): + SELF = bc.normalize_self_logins(["me[bot]"]) + + def test_pipe_rows_with_tokens_count_once_per_line(self) -> None: + comments = [ + { + "author": "me[bot]", + "body": "| 1 | a | VALID | x |\n| 2 | b | INCORRECT | y |\nprose VALID VALID", + } + ] + self.assertEqual(bc.count_classified(comments, self.SELF), 2) + + def test_only_self_rows_count(self) -> None: + comments = [{"author": "codex[bot]", "body": "| 1 | a | VALID | x |"}] + self.assertEqual(bc.count_classified(comments, self.SELF), 0) + + def test_resolved_thread_classification_is_discounted(self) -> None: + """Mirrors `count_findings`'s #465 discount: a classification row + carried in a resolved thread is a lifetime artifact of an + already-addressed round, not evidence a fresh finding was classified. + Without the discount, this stale row would inflate the denominator + and let the gate's `classified >= findings` predicate pass despite + the new finding having no classification.""" + comments = [ + { + "author": "me[bot]", + "body": "| 1 | old finding | VALID | fixed |", + "isResolved": True, + }, + {"author": "codex[bot]", "body": "[CRITICAL] new unclassified finding"}, + ] + self.assertEqual(bc.count_classified(comments, self.SELF), 0) + self.assertEqual(bc.count_findings(comments, self.SELF), 1) + self.assertLess( + bc.count_classified(comments, self.SELF), + bc.count_findings(comments, self.SELF), + ) + + +class ApprovalVerdictTests(unittest.TestCase): + """#499: an Approve-with-nits review carries no live finding.""" + + SELF = bc.normalize_self_logins(["me[bot]"]) + + def test_approve_with_nits_downgrades_and_has_no_severity_finding(self) -> None: + body = ( + "Approve. Two 🟡 nits, low impact, not worth a change on its own. " + "No blocking issues." + ) + self.assertTrue(bc.approval_downgrade(body)) + self.assertFalse(bc.has_blocking_severity(body)) + self.assertEqual( + bc.count_findings([{"author": "claude[bot]", "body": body}], self.SELF), 0 + ) + + def test_genuine_critical_finding_is_not_downgraded(self) -> None: + body = "CRITICAL: a null dereference will crash the handler." + self.assertFalse(bc.approval_downgrade(body)) + self.assertTrue(bc.has_blocking_severity(body)) + + +if __name__ == "__main__": + unittest.main() 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 new file mode 100644 index 0000000000..5606034d99 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_findings.py @@ -0,0 +1,86 @@ +"""Finding-count entrypoint the readiness gate shells out to. + +`babysit_findings` builds the live-PR finding corpus across the three feedback +surfaces and prints the `findings= classified=` line the gate parses. The +load-bearing safety property is what it does when the GraphQL review-thread +paginator returns a thread truncated at the per-thread comment cap: it must NOT +emit a count line, so the gate stays on its complete REST/bash degrade count +instead of a Python count missing every comment past the cap (which could drop +a later open finding and let the gate declare READINESS_OK). + +Network is stubbed by monkeypatching the fetch seams in the `babysit_findings` +namespace, where they are imported by name; no real gh process is spawned. +""" + +from __future__ import annotations + +import io +import pathlib +import sys +import unittest +from contextlib import redirect_stdout +from unittest import mock + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) + +import babysit_findings as bf + + +def _thread(comments, *, truncated=False, total=None, id="t1", isResolved=False, + isOutdated=False): + return { + "id": id, + "isResolved": isResolved, + "isOutdated": isOutdated, + "comments": comments, + "comments_total_count": total if total is not None else len(comments), + "comments_truncated": truncated, + } + + +def _comment(login="codex[bot]", body="[CRITICAL] a"): + return {"author": {"__typename": "Bot", "login": login}, "body": body} + + +class FetchLiveCommentsTruncationTests(unittest.TestCase): + """A truncated thread must fail closed rather than under-count silently.""" + + def test_truncated_thread_raises(self) -> None: + with mock.patch.object(bf, "fetch_issue_comments", return_value=[]), \ + mock.patch.object(bf, "fetch_pull_request_reviews", return_value=[]), \ + mock.patch.object( + bf, "fetch_review_threads", + return_value=[_thread([_comment()], truncated=True, total=150)]): + with self.assertRaises(RuntimeError): + bf.fetch_live_comments("owner/repo", 1) + + def test_untruncated_thread_collects_comments(self) -> None: + with mock.patch.object(bf, "fetch_issue_comments", return_value=[]), \ + mock.patch.object(bf, "fetch_pull_request_reviews", return_value=[]), \ + mock.patch.object( + bf, "fetch_review_threads", + return_value=[_thread([_comment(body="[CRITICAL] open")])]): + comments = bf.fetch_live_comments("owner/repo", 1) + self.assertEqual([c["body"] for c in comments], ["[CRITICAL] open"]) + + +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.""" + + def test_main_exits_2_with_no_count_line_on_truncation(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=[]), \ + mock.patch.object( + bf, "fetch_review_threads", + return_value=[_thread([_comment()], truncated=True, total=150)]): + with redirect_stdout(buffer): + code = bf.main(["--pr", "1", "--self", "me[bot]"]) + self.assertEqual(code, 2) + self.assertNotIn("findings=", buffer.getvalue()) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py new file mode 100644 index 0000000000..e36e69338b --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py @@ -0,0 +1,85 @@ +"""Resolve-thread reporting counters. + +Golden regression for #512: `humanThreadsActed` must count only threads whose +OPENING author is human, gated through the shared `is_bot` authorship +classifier -- not `botOnly` (every comment is a bot), which mislabels a +bot-opened thread carrying a later human reply as a human-thread action that +never happened. +""" + +from __future__ import annotations + +import io +import json +import pathlib +import sys +import unittest +from contextlib import redirect_stdout +from unittest import mock + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) + +import babysit_resolve_thread as rt + + +def _thread( + thread_id: str, author: str, author_type: str, *, bot_only: bool +) -> dict[str, object]: + return { + "id": thread_id, + "author": author, + "authorType": author_type, + "path": "a.py", + "isResolved": False, + "isOutdated": False, + "botOnly": bot_only, + "commentCount": 2, + "lastCommentUpdatedAt": None, + } + + +def _run(threads: list[dict[str, object]], argv: list[str]) -> dict[str, object]: + buffer = io.StringIO() + with ( + mock.patch.object(rt, "fetch_threads", return_value=threads), + mock.patch.object(sys, "argv", ["babysit_resolve_thread.py", *argv]), + redirect_stdout(buffer), + ): + rt.main() + return json.loads(buffer.getvalue()) + + +class HumanThreadsActedCounter(unittest.TestCase): + def test_bot_opened_thread_with_human_reply_is_not_a_human_thread(self) -> None: + # #512: authorType "Bot", botOnly False (a human replied). Eligible under + # --include-human, so it is acted on -- but the OPENING author is a bot, + # so it must not count toward humanThreadsActed. + result = _run( + [_thread("T_bot", "codex[bot]", "Bot", bot_only=False)], + ["owner/repo#1", "--allowed-owners", "owner", "--include-human"], + ) + self.assertEqual(result["eligibleCount"], 1) + self.assertEqual(result["humanThreadsActed"], 0) + + def test_human_opened_thread_counts(self) -> None: + result = _run( + [_thread("T_human", "alice", "User", bot_only=False)], + ["owner/repo#1", "--allowed-owners", "owner", "--include-human"], + ) + self.assertEqual(result["eligibleCount"], 1) + self.assertEqual(result["humanThreadsActed"], 1) + + def test_mixed_counts_only_the_human_opener(self) -> None: + result = _run( + [ + _thread("T_bot", "codex[bot]", "Bot", bot_only=False), + _thread("T_human", "alice", "User", bot_only=False), + ], + ["owner/repo#1", "--allowed-owners", "owner", "--include-human"], + ) + self.assertEqual(result["eligibleCount"], 2) + self.assertEqual(result["humanThreadsActed"], 1) + + +if __name__ == "__main__": + unittest.main()