Skip to content

fix(source-control): scope babysit classification credit to its surface (#642)#666

Merged
kyle-sexton merged 6 commits into
mainfrom
fix/642-classified-thread-scope
Jul 20, 2026
Merged

fix(source-control): scope babysit classification credit to its surface (#642)#666
kyle-sexton merged 6 commits into
mainfrom
fix/642-classified-thread-scope

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

The babysit readiness 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. A review thread's findings are
discounted when it resolves (the #465 lifetime-vs-open guard), but a PR-level
comment can never be — so 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.

This was the single live fail-open on main and the ratified backstop for the
#476 gate-off flip: per the backstop clause it must be fixed before the flip.

The dispatched fix shape ("non-thread PR-level comments never contribute to
classified") turned out unsafe on verification: review-discipline.md §D5
mandates that issue/review-level (PR-level) findings be answered with a
detached PR-level classification comment. Thread-only counting would count every
such classification as zero and permanently block any PR whose findings come
from review summaries — and break the Python↔bash convergence. Corrected in
agreement with the tower.

Fix

  • Per-surface classification credit (babysit_classify.py).
    count_effective_classified buckets comments by surface and caps credit within
    each bucket via min(classified, findings), then sums — a classification can
    only offset a finding on its own surface, so a stale PR-level row can no longer
    spill over to cover an open-thread finding. count_classified and
    count_findings stay pure raw counters; the bucketing composes them. On
    unsignalled input every comment lands in one bucket and this collapses to
    min(classified, findings), preserving existing behavior and the bash
    convergence property.
  • Surface discriminator (comment_surface). Three surfaces — review-thread,
    PR-level, and an isolated bucket for comments bearing no surface signal —
    resolved from two signals in order: the explicit in_review_thread stamp
    (authoritative when present), then the fetch-all-pr-comments.sh type tag on
    the --comments-json reuse path (inline → thread; general/review
    PR-level). A comment with neither signal is isolated so its rows cannot offset —
    and its findings cannot be offset by — a known surface (fail-closed for unknown
    provenance), preserving the "no signal = PR-level lifetime" model thread_is_open
    documents.
  • Surface stamping (babysit_findings.py). _comment() records
    in_review_thread (true only when fetched from a review thread); issue-level and
    review-summary comments are stamped PR-level. The entrypoint emits the effective
    count.
  • Bash degrade cap (babysit-readiness-gate.sh). The thread-blind safe-tier
    degrade gains the thread-state-free analogue classified = min(classified, findings) so a row over-count can't mask a finding and the degrade stays
    convergent with the Python min on unsignalled input. Per-surface bucketing is
    inherently surface-aware and remains Python-only, exactly like the source-control:babysit-prs: babysit-readiness-gate.sh over-counts lifetime P-badges as 'findings', false READINESS_BLOCKED reason=under-decomposed on fully-classified PRs #465 discount.

Behavior flip (documented, fail-closed)

A PR whose inline-thread findings are answered only by detached PR-level
classification replies now reports READINESS_BLOCKED where it previously
passed — a PR-level row no longer offsets an inline-thread finding. This
mechanically enforces §D5's already-ratified reply routing (inline findings MUST
reply threaded, "NEVER a detached pr comment"). Runs already following §D5 are
unaffected; only runs relying on the previously-tolerated detached-reply shape
change verdict.

Residual (documented, not closable here)

An orphaned PR-level classification covering a fresh PR-level finding is
irreducible: GitHub's flat issue comments carry no finding↔classification
linkage, so a stale PR-level row is data-identical to a live one. It is reachable
only via a §D5 routing violation (an inline finding answered with a detached PR
comment) or a reviewer editing/deleting a finding; a §D5-compliant, no-edit run
never hits it. The per-surface fix closes the entire linkable (thread) side, on
both the live and --comments-json reuse paths.

Verification

Versioning

Related

Closes #642

🤖 Generated with Claude Code

https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64

…ce (#642)

## Summary

The readiness gate blocks while source findings outnumber their per-finding
classification rows. `count_classified` counted self-authored classification
pipe-rows in ANY comment, including PR-level review-summary comments that are
never thread-resolved. Because a review thread's findings are discounted when
it resolves (the #465 lifetime-vs-open guard) but a PR-level comment can never
be, a stale classification posted outside a thread kept counting after its
finding was discounted — inflating `classified` past a fresh, still-unclassified
open-thread finding and emitting a fail-open `READINESS_OK`. This is the single
live fail-open on main and the ratified backstop for the #476 gate-off flip.

The naive fix (only count classifications inside review threads) is unsafe: it
contradicts the D5 routing rule in `review-discipline.md`, which *mandates* that
issue/review-level (PR-level) findings be answered with a detached PR-level
classification comment. Under thread-only counting every such classification
would count zero and the gate would permanently block any PR whose findings come
from review summaries.

## Fix

- **Per-surface classification credit (`babysit_classify.py`).**
`count_effective_classified` buckets comments by surface (review-thread vs
PR-level) and caps credit within each bucket via `min(classified, findings)`,
then sums: a non-thread classification can only offset a non-thread finding, an
open-thread classification only an open-thread finding. A stale PR-level row can
no longer spill over to cover an open-thread finding. `count_classified` and
`count_findings` stay pure raw counters; the bucketing composes them. On
thread-state-free input the thread bucket is empty and this collapses to
`min(classified, findings)`, so existing behavior and the bash convergence
property are preserved.
- **Surface stamping (`babysit_findings.py`).** `_comment()` records
`in_review_thread` (true only when a comment is fetched from a review thread);
the new `is_thread_comment` discriminator reads it. Issue-level and
review-summary comments are stamped PR-level. The entrypoint emits the effective
count.
- **Bash degrade cap (`babysit-readiness-gate.sh`).** The thread-blind safe-tier
degrade gains the thread-state-free analogue `classified = min(classified,
findings)`, so an over-count of rows can't mask a finding and the degrade stays
convergent with the Python `min` on thread-state-free input. Per-surface
bucketing is inherently thread-aware and remains Python-only, exactly like the
#465 discount.

## Residual (documented, not closable here)

An orphaned PR-level classification covering a fresh *PR-level* finding is
irreducible: GitHub's flat issue comments carry no finding↔classification
linkage, so a stale PR-level row is data-identical to a live one. It is reachable
only via a D5 routing violation (an inline finding answered with a detached PR
comment) or a reviewer editing/deleting a finding; a D5-compliant, no-edit run
never hits it. The per-bucket fix closes the entire linkable (thread) side.

Strict per-surface enforcement means a single consolidated PR-level
classification table will not cover inline-thread findings — which is exactly
what D5 already forbids ("NEVER a detached pr comment"), so this enforces the
already-ratified routing rather than introducing new policy.

## Verification

- `python -m unittest discover -s tests` (266 tests) — OK; new
`EffectiveClassifiedTests`, `SurfaceStampingTests`, `Main642FailOpenTests`.
- `babysit-readiness-gate.test.sh` — 56 pass, incl. the #642 scenario (fresh
open-thread finding + stale PR-level row → BLOCKED, Python-gated like #465) and
the over-classified-cap convergence case.
- `engine.test.sh` — pass (unittest + `ruff check`).

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 0s —— View job


PR Review — fix(source-control): scope babysit classification credit to its surface (#642)

  • Gather context
  • Read changed files
  • Review correctness & logic
  • Review security
  • Review test coverage
  • Post findings

Summary

This is a correct, well-scoped fix for a real fail-open. The surface-bucketing approach in count_effective_classified is the right abstraction: PR-level comments can never be thread-resolved, so their classification rows must not cross-credit open-thread findings. The bash degrade cap is the appropriate coarse analog. I have no blocking concerns, but have a few observations worth reviewing.


Findings

1. Minor — _capped_credit evaluates count_classified and count_findings independently, each iterating the same slice

babysit_classify.py:476–482

def _capped_credit(comments, self_logins):
    return min(
        count_classified(comments, self_logins),
        count_findings(comments, self_logins),
    )

Each counter iterates comments independently, so per call to count_effective_classified the thread slice is walked twice and the non-thread slice is walked twice. At PR-comment scale this is negligible. Not a bug.


2. Observation — convergence test uses \n in a bash double-quoted string

babysit-readiness-gate.test.sh (new convergence case)

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"

In bash double-quotes, \n is a literal backslash-n, not a newline. Whether jq (inside mkjson) interprets it as a newline character depends on how mkjson embeds the string. If mkjson passes the literal via jq -n or a jq string context, jq does expand \n to a newline — but if it embeds via --arg or --rawfile, it is passed verbatim.

If \n stays literal, both bash and Python see a single-line body: bash counts 1 matching line and Python's CLASSIFY_ROW_RE matches once, so classified=1 and the cap is never triggered. The test still passes, but the over-classified scenario (2 rows, 1 finding) is not actually exercised.

If the intent is to verify the cap with classified=2 > findings=1, it's worth confirming mkjson expands \n. The safest fix would be a real multi-line bash string:

F=$(mkjson conv-overclassified '[
  {author:"claude[bot]", body:"CRITICAL a"},
  {author:"me[bot]", body:"| 1 | a | VALID | x |\n| 2 | spurious | INCORRECT | y |"}
]')

…or splitting the body across the fixture like existing multi-line fixtures. The unit test test_thread_state_free_input_collapses_to_min in Python (where body is a native Python string with \n) correctly exercises the cap at the Python layer regardless.


3. Observation — count_classified remains exported but is no longer used in the production path

babysit_classify.py:420–444

babysit_findings.py now imports count_effective_classified, not count_classified. count_classified is retained and exported; the tests use it to document the pre-fix defect surface (e.g., self.assertEqual(bc.count_classified(comments, self.SELF), 1) alongside self.assertEqual(bc.count_effective_classified(...), 0)). This is a valid and intentional design choice — it makes the defect visible in tests. No action needed; just confirming it's deliberate.


4. Confirmed correct — is_thread_comment default to PR-level on absent field

babysit_classify.py:362–375

When in_review_thread is absent (legacy fixture shape, bash-degrade input), bool(comment.get("in_review_thread")) returns False → comment is treated as PR-level. Combined with the bash degrade cap, this keeps the Python count convergent with bash on thread-state-free input, exactly as intended.


5. Confirmed correct — _comment() stamps in_review_thread correctly

babysit_findings.py:66–73

thread is not None is a clean boolean stamp. Issue comments and review summaries are fetched without a thread argument and are stamped False; review-thread comments receive the thread object and are stamped True. The isResolved/isOutdated fields are correctly pulled from the thread (thread-level resolution), not individual comment nodes. Sound.


6. Confirmed correct — bash degrade cap

babysit-readiness-gate.sh:251

((classified > findings)) && classified="$findings"

Prevents an over-count of classification rows from masking an unclassified finding in the bash degrade, and keeps the degrade convergent with Python's min(classified, findings) on thread-state-free fixtures. Placed before the Python re-count, so Python's output unconditionally overwrites both counts when available — the cap is only load-bearing when Python is absent. Correct.


Security

No new security concerns. No new shell injection surfaces, no unvalidated external input at new boundaries, no filesystem writes or egress. Regex patterns are well-bounded.


Verdict

Approve. The fix is architecturally correct, the per-surface bucketing is the right abstraction, and the tests cover the key failure scenario. Finding 2 above is worth verifying before merge to confirm the bash convergence test actually exercises the classified>findings cap scenario.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08fe4d7969

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py Outdated
…cap (#642)

The DETECTION header still described `classified` as an uncapped row count and
`BLOCK when classified < findings` with no mention of the cap added in the fix
below it — the exact header/behavior drift #534's single-source-of-truth intent
guards against. State the min-cap (and the Python per-surface refinement) at the
`classified`/BLOCK bullet, and widen the `usage()` sed range so the three extra
header lines don't push exit codes 1/3/4 out of `--help`.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 4s —— View job


PR Review — fix(source-control): scope babysit classification credit to its surface (#642)

  • Gather context
  • Read changed files
  • Assess Codex P2 finding (inline comment surface inference)
  • Review correctness & logic
  • Review security
  • Post findings

Summary

The surface-bucketing fix in count_effective_classified is correct and well-scoped. The docs commit (1416eff) correctly syncs the DETECTION header with the cap behavior. No blocking issues.

Two findings from this review pass worth addressing before merge:


Finding 1 — Prior review's \n concern is a non-issue (clarification)

The previous review (finding 2) flagged that \n in a bash double-quoted mkjson string might not expand to a newline, making the conv-overclassified convergence test a no-op for the 2-row scenario.

This is incorrect. mkjson calls jq -n "$2" where the argument is a jq expression. In jq's string grammar, \n is the newline escape — the same as in JSON. Confirmed:

$ jq -rn '"| 1 | a | VALID | x |\n| 2 | spurious | INCORRECT | y |"' | wc -l
2

The body arrives as a 2-line string. CLASSIFY_ROW_RE matches both rows, count_classified returns 2, count_findings returns 1, and min(2,1) = 1 — the cap is exercised exactly as intended. No change needed.


Finding 2 — Codex P2: is_thread_comment does not infer from type: "inline" (valid, follow-up scope)

babysit_classify.py:362–375

The Codex finding is real: when the gate is invoked with --comments-json <file> where <file> was produced by fetch-all-pr-comments.sh, inline review comments carry type: "inline" but no in_review_thread field. is_thread_comment reads only in_review_thread, so inline findings fall into the non-thread bucket alongside any PR-level classification rows. A stale PR-level row can then cross-credit an inline finding — the original #642 fail-open scenario, on the reuse path.

However, the PR's scope and documentation are accurate:

Closing the gap would require adding a fallback in is_thread_comment:

def is_thread_comment(comment: dict[str, Any]) -> bool:
    if "in_review_thread" in comment:
        return bool(comment["in_review_thread"])
    # Infer from fetch-all-pr-comments.sh schema when explicit stamp absent
    return comment.get("type") == "inline"

This would make the Python classifier surface-aware on fetch-all-pr-comments.sh output without needing isResolved/isOutdated (inline findings would be thread-bucketed; resolution-state discounting would still be absent, but cross-surface credit would be blocked). That said, this is additive to the fix's stated scope, and count_effective_classified's docstring explicitly covers it as a thread-state-free fallback. Recommend tracking as a follow-up rather than blocking this PR.


Finding 3 — Docs commit (1416eff) is correct and complete

The classified bullet now accurately describes both the min-cap and the Python per-surface refinement. The sed -n '2,58p' range correctly includes the three new lines so exit codes 1/3/4 don't fall off --help. No issues.


Previously confirmed findings (still hold)

  • _capped_credit double-iteration (prior finding 1): negligible at PR-comment scale, not a bug.
  • count_classified still exported (prior finding 3): intentional — test fixtures use it to document the pre-fix defect surface. Sound.
  • is_thread_comment defaulting to PR-level on absent field (prior finding 4): correct convergence behavior on bash-degrade input.
  • _comment() stamping (prior finding 5): clean boolean stamp, thread resolution pulled from the right level.
  • Bash degrade cap (prior finding 6): correctly placed before the Python re-count override.

Security

No new concerns on the updated commits. No new shell injection surfaces, no unvalidated external input, no filesystem writes or egress.


Verdict

Approve. The per-surface bucketing is architecturally correct and closes the live-path fail-open. The --comments-json+fetch-all-pr-comments.sh gap (Codex P2) is a real but in-scope-acknowledged limitation of the thread-blind reuse path, not a regression. Recommend a follow-up issue to infer type == "inline" in is_thread_comment.

…omments type (#642)

Review (Codex P2) surfaced a fail-open still reachable on the `--comments-json`
reuse path: `fetch-all-pr-comments.sh` tags inline review comments `type:
"inline"` but carries no `in_review_thread` stamp, so `is_thread_comment` bucketed
an inline finding as PR-level alongside a detached PR-level classification row,
which then cross-credited it — the exact #642 fail-open, on the reuse path.

`is_thread_comment` now falls back to the schema's `type == "inline"` tag when the
explicit stamp is absent, making the reuse path surface-aware. The explicit
`in_review_thread` stamp still wins when present, so a live PR-level comment is
never re-inferred as a thread; legacy `{author, body}` fixtures (no stamp, no
type) stay PR-level, so the existing convergence cases are untouched. The
tag-driven bucketing is a Python enrichment the thread-blind bash degrade cannot
see, so the new regression is Python-gated like the #465 and live-path #642 cases.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 56s —— View job


PR Review — fix(source-control): scope babysit classification credit to its surface (#642)

  • Gather context
  • Read new commit changes (7d575dd)
  • Verify fetch-all-pr-comments.sh type enum
  • Review correctness & logic
  • Post findings

Reviewing new commit 7d575dd — Codex P2 address: is_thread_comment type-tag inference

This commit closes the gap identified in the second review pass: the --comments-json reuse path was thread-blind because fetch-all-pr-comments.sh output tags inline comments type: "inline" but carries no in_review_thread stamp. That left an inline finding and a detached PR-level classification row in the same non-thread bucket, restoring the exact #642 fail-open on the reuse path.


Implementation — confirmed correct

babysit_classify.py:387–389

if "in_review_thread" in comment:
    return bool(comment["in_review_thread"])
return comment.get("type") == "inline"

The two-signal priority chain is exactly right:

  1. The in membership check — not .get() — means an explicit in_review_thread: False stamp (a live PR-level comment) is honored and never re-inferred as a thread even if a stray type: "inline" is present. The test test_explicit_stamp_wins_over_type_tag pins this behavior.

  2. type == "inline" is the only inline-surface tag emitted by fetch-all-pr-comments.sh; "general" (issue-level) and "review" (review summaries) are both PR-level and correctly fall to False. Verified against the producer at fetch-all-pr-comments.sh:116, L140, L163.

  3. Absent both signals (bash-degrade shape, legacy {author, body} fixtures) → PR-level default. Existing convergence tests are unaffected.


Tests — complete and correct

Python unit tests (test_babysit_classify.py):

  • test_inline_type_tag_is_bucketed_as_thread_on_reuse_path — directly reproduces the Codex P2 scenario: type:"inline" finding + type:"review" classification row → count_findings=1, count_effective_classified=0. Also asserts is_thread_comment returns the expected value for each comment directly.
  • test_explicit_stamp_wins_over_type_tag — the priority-inversion guard: {in_review_thread: False, type: "inline"}is_thread_comment returns False. Critical to prevent a future refactor from silently re-enabling the cross-credit.

Bash integration test (babysit-readiness-gate.test.sh):

  • #642 reuse-path inline tag case: type:"inline" finding + type:"review" classification → findings=1 classified=0 → READINESS_BLOCKED. Correctly Python-gated with the same probe_py guard as the live-path case. The comment accurately documents that the bash degrade is thread-blind and false-passes here — the accepted degrade coarseness.

No issues found

This commit is minimal, correct, and exhaustively tested. The surface-inference approach is the right one: it uses the producer's own schema tag (rather than adding a new stamp at the producer, which would be a larger change) and preserves strict precedence so the explicit stamp always wins. The docstring is accurate and matches the three-surface enum.

Verdict: Approve. The full PR — original bucketing fix (08fe4d7), docs sync (1416eff), and this reuse-path close (7d575dd) — is ready to merge.

…it surface credit (#642)

Follow-up to the reuse-path fix: harden the absent/unknown-surface case fail
closed. `is_thread_comment` (bool) forced every comment into thread-or-not, so a
comment bearing neither an `in_review_thread` stamp nor a `fetch-all-pr-comments.sh`
`type` tag defaulted to the PR-level bucket, where a PR-level classification row
could offset it. A single 2-bucket default cannot be fail-closed in both
directions — with per-bucket `min`, whichever bucket holds "unknown", that
bucket's rows can offset that bucket's findings.

Replace the bool discriminator with `comment_surface` → thread / pr_level /
unknown, and credit three buckets. Unknown provenance is isolated: its rows
cannot offset a known-surface finding and its findings cannot be offset by a
known-surface row. This preserves the "no signal = PR-level lifetime" model
`thread_is_open` documents and needs no fixture edits — uniform unsignalled input
(the legacy `{author, body}` and bash-degrade shapes) collapses to one bucket,
identical to today, so existing tests and the convergence property hold. The
guarded surfaces are defensive: the live path stamps every comment and
fetch-all-pr-comments.sh always emits `type`.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 25s —— View job


PR Review — fix(source-control): scope babysit classification credit to its surface (#642)

  • Gather context
  • Read new commit changes (1120f22)
  • Review correctness & logic
  • Review security
  • Post findings

Reviewing new commit 1120f22 — isolate unsignalled-provenance comments in babysit surface credit

This commit hardens the surface-bucketing from the prior three commits by converting the binary is_thread_comment → bool into a three-way comment_surface → str (THREAD_SURFACE / PR_LEVEL_SURFACE / UNKNOWN_SURFACE). Previously, a comment with no surface signal (no in_review_thread stamp, no type tag) fell silently into the non-thread bucket. Now it lands in an isolated UNKNOWN_SURFACE bucket where no cross-surface credit is possible — fail-closed for unknown provenance. The single-pass dict bucketing in count_effective_classified is also a cleaner implementation than the prior double list-comprehension.

Implementation is correct. The three-level priority chain in comment_surface is sound:

  1. "in_review_thread" in comment (membership, not .get()) → authoritative; explicit False on a PR-level comment is never overridden by a stray type tag — test_explicit_stamp_wins_over_type_tag pins this.
  2. type == "inline" → THREAD; type in ("general", "review") → PR_LEVEL.
  3. All else → UNKNOWN.

The convergence property holds: on thread-state-free input (all bare {author, body} fixtures, the bash-degrade shape), every comment lands in UNKNOWN_SURFACE, so count_effective_classified = min(classified, findings) — identical to the bash degrade's own cap. ✓


Finding 1 — Stale comment in test_thread_state_free_input_collapses_to_min

test_babysit_classify.py:199–202

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.

The comment says "every comment is PR-level" but after this commit, bare {author, body} comments (no type, no in_review_thread) now land in UNKNOWN_SURFACE, not PR_LEVEL_SURFACE. The convergence property and assertion still hold (one bucket → collapses to min), but the comment misidentifies the bucket. Should read something like "every comment is in one bucket (UNKNOWN_SURFACE, the unsignalled shape)".

Not a bug — just a stale test comment. Fix this →


Finding 2 — Observation: D5 test exercises UNKNOWN bucket, not PR_LEVEL

test_babysit_classify.py:171–184

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...
    comments = [
        {"author": "claude[bot]", "body": "### 1. [CRITICAL] a\n..."},
        {"author": "me[bot]",     "body": "| 1 | a | VALID | fixed |..."},
    ]

The fixture uses bare {author, body} comments with no type or in_review_thread field. After this commit both land in UNKNOWN_SURFACE, not PR_LEVEL_SURFACE. The assertion (effective_classified == 2) still passes because within-bucket credit flows in UNKNOWN exactly as in PR_LEVEL, and the D5 scenario is correctly gated (the gate doesn't block). But this test no longer actually exercises the PR_LEVEL_SURFACE bucket — it exercises UNKNOWN.

A more complete fixture for the live D5 path would stamp in_review_thread: False, and for the --comments-json path would use type: "general" / type: "review". As-is the PR_LEVEL bucket has no explicit unit test that exercises within-bucket credit. Not blocking — the bucket logic is identical for UNKNOWN and PR_LEVEL — but worth a follow-up test for completeness.


Previously confirmed findings (still hold)

  • _capped_credit single-pass bucketing — now one pass through comments (up from two list comprehensions), cleaner. Not a bug in either direction.
  • count_classified still exported — used in defect-surface documentation tests. Sound.
  • is_thread_comment fully removed — no stale references anywhere in the tree. ✓
  • Bash degrade convergence — the converge fixtures are all bare {author, body} → all land in UNKNOWN → single bucket → collapses to min. Convergence unchanged. ✓
  • New bash test is correctly Python-gated — when Python is absent, the test is skipped with a pass, not a fail. Matches the pattern for the other surface-aware cases above it. ✓

Security

No new concerns. The UNKNOWN bucket is purely an isolation mechanism in the Python counter; it adds no new external inputs, no shell surfaces, no egress.


Verdict

Approve. The unsignalled-provenance isolation is the right fail-closed default. The three-way surface taxonomy is clean, the priority chain is correct, and the convergence claim holds. Finding 1 (stale comment text) is trivial; Finding 2 (D5 test covering UNKNOWN not PR_LEVEL) is a follow-up gap, not a bug. The full PR is ready to merge.

kyle-sexton and others added 2 commits July 20, 2026 06:09
…edit (#642)

Bump the source-control plugin to 0.13.3 and add the Keep-a-Changelog entry for
the #642 readiness-gate fix. The entry documents the fail-open closure and
explicitly calls out the 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, mechanically enforcing
review-discipline.md §D5's already-ratified reply routing. Fail-closed direction.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 14s —— View job


PR Review — fix(source-control): scope babysit classification credit to its surface (#642)

  • Gather context
  • Review new commit (da0d08c) — release bump and changelog
  • Verify version bump correctness
  • Verify CHANGELOG accuracy against PR changes
  • Post findings

Reviewing new commit da0d08c — release 0.13.3

This commit bumps version in plugin.json from 0.13.2 to 0.13.3 and adds the Keep-a-Changelog entry for the full #642 fix set (commits 08fe4d7 through 1120f22).


Version bump — correct, with one semantic-versioning note

plugin.json:4

0.13.2 → 0.13.3 is a patch increment. The primary change is a bug fix (close a fail-open), which maps to a patch bump. The behavior flip (inline findings no longer offset-able by detached PR-level classification rows) is documented under ### Changed in the CHANGELOG — under strict semver that entry normally warrants a minor increment.

However, this is a pre-1.0 version (0.y.z) where the public API is not yet stabilized, the flip enforces already-ratified §D5 policy (not new policy), only D5-violating runs change verdict, and the "BEHAVIOR FLIP" heading in the changelog makes the change explicit. The patch bump is a defensible call. If the team convention is to use minor for any observable behavior change, 0.14.0 would also be correct — but this is not a blocker.


CHANGELOG — accurate and complete

CHANGELOG.md:6–34

All four functional commits are covered:

Claim in CHANGELOG Matches implementation
Fail-open mechanism (lifetime discount discounting a finding, PR-level classification still counting) ✓ root cause in babysit_classify.py
Three-way surface bucket (review-thread, PR-level, unsignalled) THREAD_SURFACE/PR_LEVEL_SURFACE/UNKNOWN_SURFACE from 1120f22
Cap within each bucket _capped_credit per bucket in count_effective_classified
Bash degrade min(classified, findings) babysit-readiness-gate.sh:251
Per-surface refinement is Python-only; convergent with degrade on unsignalled input ✓ UNKNOWN bucket collapses to single-bucket min, identical to bash cap
Behavior flip (D5-violating detached reply → BLOCKED) ✓ documented correctly; fail-closed direction

No inaccuracies found.


Carry-forward: stale test comment still unfixed

test_babysit_classify.py:200–201

# Convergence invariant: with no thread markers every comment is PR-level,
# so effective classified is min(classified, findings) -- what the bash

This was flagged as Finding 1 in the previous review pass (commit 1120f22). After 1120f22, bare {author, body} comments (no type, no in_review_thread) land in UNKNOWN_SURFACE, not PR_LEVEL_SURFACE. The convergence invariant still holds — one bucket collapses to min — but the comment says "PR-level" when the correct description is "UNKNOWN_SURFACE (the bare author/body fixture shape)". The assertion at line 212 is unaffected.

Still not a bug. Still trivial. Fix this →


Previously confirmed findings (all still hold, nothing regressed)

  • Functional correctness (all prior passes): surface-bucketing, priority chain in comment_surface, _capped_credit, bash degrade cap — all correct.
  • \n in bash convergence test (pass 1 finding 2, resolved in pass 2): jq expands \n — the cap is exercised.
  • Reuse-path type-tag inference (pass 2 finding 2, fixed in 7d575dd): type == "inline" → thread bucket; explicit in_review_thread stamp wins.
  • Unsignalled isolation (pass 3 finding 1, fixed in 1120f22): UNKNOWN_SURFACE bucket is fail-closed for absent-signal comments.
  • D5 test exercises UNKNOWN not PR_LEVEL (pass 3 finding 2): documented follow-up gap; bucket logic is identical for both surfaces — no bug.

Security

No new concerns. The release commit touches only plugin.json (version scalar) and CHANGELOG.md (prose documentation).


Verdict

Approve. The release commit is correct: the version bump is justified, the CHANGELOG entry is accurate, and it covers all four functional commits completely. The one carry-forward (stale "PR-level" comment text in test_thread_state_free_input_collapses_to_min) is the same trivial documentation gap flagged in the prior pass — not a blocker. The full PR is ready to merge.

@kyle-sexton
kyle-sexton merged commit 9d7ca0c into main Jul 20, 2026
19 checks passed
@kyle-sexton
kyle-sexton deleted the fix/642-classified-thread-scope branch July 20, 2026 10:18
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
main advanced source-control to 0.13.3 (#651, #666) while this branch
bumped from an older 0.13.1 base; resolve the version and CHANGELOG
conflict by re-homing this PR's entry as [0.13.4] on top of main's
[0.13.3], keeping all main entries intact.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
… filter (#654) (#668)

## Summary

The `work-items` GitHub adapter's "Open linked PRs" mechanic (shipped in
#463 / PR #643) decided
whether an issue already had an open closing PR by running a
closing-keyword `jq` regex over the
**raw** PR body. GitHub treats fenced code blocks and HTML comments as
inert for auto-close
linkage, but the raw-body regex did not: a PR body carrying a fenced
`Closes #<N>` example matched
`true` and dropped the still-open, still-pickable issue `#<N>` from the
`/work-items:work`
frontier. #643 shipped a partial `gsub` fence-stripper alongside the
regex, but it recognized only
exactly-three backtick/tilde fences and silently missed
four-or-more-backtick and indented fences —
exactly the fragile hand-roll #654 flagged as not-small.

## Fix

Replace the entire raw-body heuristic (the `gh pr list --search "<N>
in:body"` prefilter, the
closing-keyword `jq` `test`, and the partial `gsub` fence-stripper) with
GitHub's own computed
close-linkage, queried directly against the target issue number via the
GraphQL
`Issue.closedByPullRequestsReferences` connection, keeping only
`OPEN`-state PR nodes. This is the
root-cause fix #654 pointed toward: the signal is now GitHub's parsed
issue linkage — the same set
GitHub renders in the issue sidebar and acts on at merge time — rather
than a text match this
mechanic has to keep fence-, word-, and number-boundary-aware itself.

Consequences folded into the mechanic's rationale in the README:

- **Fenced code blocks and HTML comments are inert for free** — no
fence-tracking heuristic to
  maintain, and the fence-blindness #654 filed is closed.
- **No word-boundary / number-boundary guards** — `#463` cannot collide
with `#4630` / `#1463`
because the reference is GitHub's parsed linkage, not a regex over raw
text.
- **Base-branch correctness (behavior change, called out explicitly):**
GitHub forms the close-link
only for a PR that targets the default branch. A `Closes #<N>` on a
non-default-base PR is
ignored by GitHub and now no longer excludes its issue — where the
retired raw-body regex counted
it regardless of base. This is *more* correct per GitHub's real
merge-time auto-close semantics.
- **Opt-out (`Refs #<num>`) stays intrinsic** — a
reference-without-closing never enters the
  closing linkage, so it correctly does not exclude its issue.

Scope is held to the `work-items` adapter. The sibling fence-blind
matcher noted in #654 — the
pre-create gate `KEYWORD_REGEX` in
`plugins/source-control/skills/pull-request/reference/create.md`
— is **not** touched here: it is a different plugin, and it gates a PR
body *before the PR exists*,
so it cannot use this issue-side GraphQL linkage. Whether it should
adopt a different fence-aware
approach is left as a forward-reference follow-up, not folded into this
adapter patch.

## Verification

Empirical research proving `Issue.closedByPullRequestsReferences` is
queryable via `gh api graphql`
and returns GitHub's genuine auto-close linkage (open-state filtered) —
not body text matches.

**Schema exists and is queryable** (live introspection, `gh api
graphql`):

```
closedByPullRequestsReferences args:
  includeClosedPrs  Boolean = false  "Include closed PRs in results"
  first/last/after/before, orderByState, userLinkedOnly
```

**Positive cases — real prose closing keywords link, filtered to open
PRs** (the exact mechanic
this PR ships, run live against this repo):

```
issue 642  -> true    (open issue; open PR #666 carries prose `Closes #642`)
issue 654  -> false   (this issue; no open closing PR)
issue 487  -> false   (closed issue; its closing PR #629 is MERGED, so filtered out by state=="OPEN")
issue 644  -> false   (baseline before this PR)
issue 667  -> false   (baseline before this PR)
```

**Negative isolation — fenced and HTML-comment closing keywords are
excluded.** This PR body is
itself the live experiment. It carries a real `Closes #654` in prose
(positive control: an open
issue closed via prose, in *this* draft PR, at *this* moment), plus two
deliberately inert
fixtures — a closing keyword for open issue **#644 only inside a fenced
code block**, and one for
open issue **#667 only inside an HTML comment**:

<!-- inert HTML-comment fixture (must NOT link #667): Fixes #667 -->

```text
inert fenced fixture (must NOT link #644):
Closes #644
```

Because #644 and #667 are open issues (baselines `false` above) and this
same draft PR *does* link
#654 from prose, the only difference for #644/#667 is that their keyword
sits in a fence / comment.
Live query results against this PR are recorded below.

Live results (this PR is #668, base branch `main`):

```
# PR-side — what GitHub links THIS PR as closing:
pullRequest(668).closingIssuesReferences  ->  [654]        # ONLY 654. #644 (fenced) and #667 (comment) excluded.

# Issue-side — the exact mechanic this PR ships:
issue(654).closedByPullRequestsReferences  ->  [{668, OPEN}]   # real prose `Closes #654` links (positive control)
issue(644).closedByPullRequestsReferences  ->  []              # fenced `Closes #644` inert — #668 absent
issue(667).closedByPullRequestsReferences  ->  []              # HTML-comment `Fixes #667` inert — #668 absent
```

Conclusion: within one PR body at one instant, GitHub linked the prose
closing keyword (#654) and
excluded the identical keyword when it sat inside a fenced code block
(#644) or an HTML comment
(#667). Since #644/#667 are open issues that this same draft PR would
otherwise be capable of
linking, the fence / comment is the sole cause of exclusion. Both the
fenced-example bug #654
reported and HTML-comment inertness are handled by
`closedByPullRequestsReferences` with no
heuristic of our own.

Closes #654

## Related

- #654 — this issue (fence-blindness in the open-linked-PR
closing-keyword regex).
- #463 / PR #643 — shipped the "Open linked PRs" mechanic (and its
partial `gsub` fence-stripper)
  this PR replaces.

🤖 Generated with a Claude Code implementation subagent (issue #654)

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
… disabled by default (#665)

## Summary

Implements the #476 autopilot merge tier for `babysit-prs`: at day-scale
throughput, human
approve-and-merge is the pipeline bottleneck. The tier lets the fleet
**satisfy** the branch
ruleset instead of bypassing it — a second bot account (author ≠
approver) runs a genuine review
pass and submits an approving review **only when clean**, after which
the pinned merge gate merges
**only when every criterion holds**. The ruleset itself is never
touched; the bot review is what
makes this a real gate rather than a rubber stamp.

**HELD FOR OPERATOR: do-not-merge until the operator personally reviews
the safety-contract
change.** This PR moves through the normal pipeline as a proposal only.
The babysit safety contract
(`reference/safety.md`) changes *if and only if* the operator merges
this PR. It carries the
`do-not-merge` label at creation; `do-not-merge.yml` will hold the
required check red by design —
that is the intended hold, not a CI failure to fix.

## Criteria (verbatim from the 2026-07-19 15:49 maintainer decision)

Enforced deterministically in `babysit_merge.py` behind the fail-closed
`--autopilot-merge-tier`
umbrella flag. Base-gate criteria already existed; the tier layers the
rest.

| Decision criterion | Where enforced |
| --- | --- |
| required checks green incl. review workflow | base gate
(`mergeStateStatus` CLEAN + required-context reconciliation) |
| issue-linked | tier — `closingIssuesReferences` non-empty |
| authored by a pipeline lane | tier — `--lane-logins` membership |
| no human CHANGES_REQUESTED | base gate (`reviewDecision`) |
| no human blocking comment | tier — shared `has_blocking_text` /
`has_blocking_severity` over human comments |
| no unresolved thread | base gate (unresolved review threads) |
| no do-not-merge label | tier — `--block-labels` |
| no unratified decision-default marker on the linked issue | tier —
scans `closingIssuesReferences` comments for a `Decision defaulted`
marker; ratified only by a human `OWNER`/`MEMBER` comment after it |
| head SHA unchanged since review | tier — distinct-bot approval pinned
to the live head; `--expected-head` as always |
| author ≠ approver via bot identity | tier — distinct-bot approving
review (`--approver-bot-logins`, shared `is_bot`) |

**No deviations from the decision comment.** Every criterion predicate
is reused from the shared
`babysit_classify` module (#634), not re-implemented. Any criterion
failing falls back to today's
behavior: the PR is reported on the human merge-ready list — the tier
never routes around the gate.

## Flip preconditions (tier stays disabled until these land)

Both are enforcement-in-the-gate placements recorded on #476; the
pre-tier world (a human reads the
comments at GATE-ON) stays safe meanwhile.

- **Decision-default veto** — added in this PR as a merge criterion
(table above). Reactions are
deliberately not consulted for ratification: the reactions API carries
no `authorAssociation`, so a
reaction cannot be attributed to a maintainer, and attributing it via
the operator's self-logins
would let pipeline automation clear its own veto (the #450
attribution-drift hazard). Ratification
is therefore a maintainer comment, or a manual merge. Note: #476's own
thread quotes the marker
phrase, so once the tier is active #665 would self-hold under this
criterion — harmless, since it
  is `do-not-merge`-held and merged by the operator by hand regardless.
- **#642** — the `count_classified` fail-open (stale pipe-rows in
non-thread comments) has **landed**
(source-control 0.13.3 via #666) and is merged into this branch, so this
flip-precondition is now
cleared. It was never worked around locally — the tier consumes the
shared classifier directly. The
remaining holds are the operator's safety-contract review and the
disabled-by-default flip.

## Disabled by default

The tier exists only while the new `babysit_autopilot_merge_tier`
userConfig (boolean, default
**off**) is enabled; the skill wires the `--autopilot-merge-tier` flags
only then. Enabling the
flag, and any later gate-off flip, is a separate, announced operator
step. Absent the flag the merge
gate is byte-for-byte its prior self, so `worker`/`autopilot`'s existing
gate-proven merges are
unchanged. The umbrella flag is fail-closed: it refuses (exit 3) unless
`--lane-logins`,
`--approver-bot-logins`, and `--block-labels` are all supplied.

## safety.md rationale

`reference/safety.md`'s "Never do automatically → merge" contract is
updated deliberately to codify
the tier and its criteria as an explicit, config-gated carve-out: the
fleet may generate its own
approving review and merge **only** under the enumerated criteria,
fail-closed, with a genuine bot
review and an untouched ruleset. This is the safety-posture change the
operator signs off by
merging.

## Test plan

- `python -m unittest discover -s tests` — 287 tests pass, including new
`test_babysit_merge.py`
(each criterion with a passing **and** a fall-back fixture, the
decision-default veto's
pass / unratified / fetch-error cases, plus the tier-absent no-network
invariant) and guard /
  skill-contract additions for the fail-closed CLI and tier prose.
- `engine.test.sh` — unittest suite + ruff clean + guarded-wrapper
checks (including the new
  wrapper-level tier fail-closed) all pass.
- `babysit-readiness-gate.test.sh` — unchanged, green (Python-free
degrade + convergence intact).
- markdownlint, typos, and plugin-manifest JSON-schema validation all
clean locally.

## Related

Closes #476

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

babysit classifier: count_classified counts stale pipe-row classifications from non-thread review-summary comments

1 participant