fix(source-control): don't let a worker's own bot-thread reply strand it outside every resolution scope

[kyle-sexton]

Summary

Fixes #1729.

babysit_resolve_thread.py's thread-resolution-scope classifier (project_thread's botOnly computation) inspects every fetched comment in a review thread, not just the opener, to decide botOnly -- correct for catching a genuine human reply. But the worker's own documented reply to a bot thread (a classification reply, a Fixed in <sha> follow-up -- reference/orchestration.md) is also a real API comment, posted under the worker's own login, not the bot's.

Before this fix: replying to a bot thread flipped botOnly false the moment the reply posted (indistinguishable from a genuine third-party human joining). classify()'s bright line then fires -- if not thread["botOnly"] and not include_human: return "skipped-human-thread" -- and --include-human is unset by design in worker/safe modes (must never touch a genuine human thread), so nothing lifts it back in. The thread is permanently stranded outside BOTH resolution scopes, even though replying to it was exactly the correct, expected action.

Fix

• --self-logins (new flag, mirroring babysit_merge.py's existing flag of the same name, and the already-configured-but-never-threaded babysit_self_logins userConfig key). project_thread's botOnly now treats a self-login-authored comment as a third admissible authorship alongside bot and third-party human: neutral for botOnly (does not disqualify it, unlike a genuine human reply) but not sufficient to earn it alone -- a thread still needs at least one ACTUAL bot comment to be botOnly.
• Second-order fix, found during review: fixing botOnly alone isn't enough. The mandated classification-reply table (reference/review-discipline.md) restates the source finding's own severity marker (e.g. a CRITICAL column, or "VALID -- not a security concern") as part of the worker's own reply -- so a raw severity scan over that self-reply would re-trip --autonomous's skipped-severity-marked guard the instant botOnly stopped blocking it, just moving the stranding to a different verdict. The severity scan now strips a self-authored comment's classification-table rows (not its whole body) before scanning, reusing babysit_classify.count_findings's identical existing rule (the helper is promoted from private to shared rather than reimplemented) -- non-table self content still flags.
• Every copyable resolve-thread command form (SKILL.md, reference/safety.md x3, reference/orchestration.md x2) now carries --self-logins @me,<self-logins> alongside --extra-bot-logins, verified by this repo's own test_every_documented_wrapper_command doc-parser-conformance gate.
• plugin.json's babysit_self_logins description now names the resolve-thread bot-only test among the surfaces the self set covers.
• CHANGELOG entry + patch version bump (0.34.0 -> 0.34.1, renumbered from 0.33.3 when main shipped 0.34.0 and its own 0.33.3 entry), matching this plugin's changelog-parity CI gate.

Test plan

• python -m unittest discover over plugins/source-control/skills/babysit-prs/scripts/tests -- 453 tests, all green (includes 11 new regression tests: the bug reproduced without --self-logins, the fix proven with it, a self-only thread still correctly not botOnly, a genuine third-party human still blocking botOnly regardless of --self-logins, truncation still failing closed, the severity-scan re-stranding scenario reproduced and fixed, non-table self content still flagging, third-party severity markers still flagging, and a full end-to-end --autonomous --only-outdated run in the exact tier the bug occurs in).
• test_guards.py's guard-contract suite (including test_reference_doc_matches_the_tables and test_every_documented_wrapper_command) -- green, no guard_contract.py changes needed since classify()'s predicate contract (which takes botOnly as an input) is unchanged.
• plugins/skill-quality/scripts/check-skill.sh babysit-prs -- PASS, 0 errors (SKILL.md at 497/500 lines).
• scripts/check-changelog-parity.sh --check -- passes.
• markdownlint-cli2 over every changed .md file -- 0 issues.

Related

• Closes babysit_resolve_thread.py: worker's own reply to a bot thread strands it outside every resolution scope #1729
• Refs Autonomous thread-resolve: pin pre-push isOutdated state (per-thread comment pins don't stop own-push displacement) #571 -- the adjacent, still-open gap in the same --autonomous resolve path: the per-thread
  comment pins do not stop a worker's own push from displacing a thread. This PR fixes the
  authorship-classification half of the stranding; Autonomous thread-resolve: pin pre-push isOutdated state (per-thread comment pins don't stop own-push displacement) #571 tracks the isOutdated pinning half.
• Refs fix(source-control): let a declared non-structural account count as reviewer evidence #1703 -- the sibling fix in the same plugin for how a declared non-structural account is
  classified, the same identity-classification family as --self-logins here.

🤖 Generated with Claude Code

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4a618c9ba

ℹ️ 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".

[claude]

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

---

PR Review

• Gather context and understand the PR
• Review changed files
• Analyze Codex finding (P1: opener must be bot-authored)
• Provide comprehensive review feedback

---

Summary

The core fix is correct and well-engineered. The stranding bug is real, the two-part fix (botOnly + severity scan) is exactly right, test coverage is thorough (11 targeted regressions), and every documented command form is updated consistently. One semantic gap in the new botOnly predicate needs attention before merging.

---

Finding: botOnly does not require the thread opener to be a bot

File: babysit_resolve_thread.py:229–233

The Codex P1 finding is grounded in a real policy mismatch. review-discipline.md lines 198–203 is explicit:

Resolve ONLY threads whose OPENING comment is authored by a BOT reviewer. NEVER resolve your OWN threads (any of your posting identities). Thread author = login of the THREAD-OPENING comment.

The new predicate checks all commenters but not the opener specifically:

bot_only = (
    not truncated
    and any(_author_is_bot(c) for c in comments)
    and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
)

Scenario that bypasses this: A self-login opens a review thread (a bot later replies with a follow-up). Then:

• any(_author_is_bot(c) for c in comments) → True (bot is present)
• all(_author_is_bot(c) or _author_is_self(c) for c in comments) → True (opener is self, reply is bot)
• bot_only = True → thread is eligible for autonomous resolution

This contradicts the canonical policy: a thread the worker itself opened should never be resolvable under the bot-only scope, but under this implementation it is, as long as a bot replied into it afterward.

This is a defense-in-depth gap — workers are documented not to open threads — but the code is the enforcement layer for that constraint. is_bot(comments[0]) is already computed into first_author at line 220; requiring the opener to be a bot aligns the predicate with the policy:

bot_only = (
    not truncated
    and bool(comments)                    # guard the [0] index below
    and _author_is_bot(comments[0])       # opener must be a bot (not self)
    and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
)

Requiring _author_is_bot(comments[0]) makes the any(...) clause redundant (the opener alone satisfies it), while also preventing the self-opener scenario. Existing tests pass because every SelfLoginsNeutralizeOwnReply fixture starts with a _comment("codex[bot]", "Bot") opener.

Fix this →

---

Everything else: correct

Primary bug fix (lines 229–233, new form): the three-condition bot_only correctly enforces "at least one actual bot + no disqualifying third-party human + not truncated." The core invariant (self-only thread is not botOnly) is tested and green.

Second-order fix (_severity_scan_body, lines 236–252): stripping classification-table rows before scanning a self-authored comment is the right approach — it mirrors count_findings's identical rule and prevents moving the stranding from skipped-human-thread to skipped-severity-marked. Non-table self prose still flags. Clean.

strip_classification_rows promotion (babysit_classify.py): renaming _strip_classification_rows → strip_classification_rows and adding a public docstring is the right call; the helper is now a shared contract rather than an internal. The promotion is minimal and doesn't change behavior.

@me resolution ordering (lines 585–599): resolving self logins after the owner-scope check (no network call for an out-of-scope owner) matches babysit_merge.py's pattern. The RuntimeError fallback (keep explicit logins, drop unresolved @me) is correct fail-closed behavior.

Documentation updates (SKILL.md, safety.md ×3, orchestration.md ×2): every resolve-thread command form now carries --self-logins @me,<self-logins>. This is the right scope — listing and mutating alike, since botOnly is computed on every call.

Test coverage: 11 new regression tests cover the exact failure modes described in the PR — the bug without the flag, recovery with it, self-only-thread-still-not-botOnly, genuine-third-party-human-still-blocks, truncation-independent-of-self-logins, and the full end-to-end --autonomous --only-outdated tier combination. Missing test: a self-login-as-opener + bot-as-replier case (which would currently pass incorrectly until the opener check is added).

---

Verdict

One issue to address before merge: add the _author_is_bot(comments[0]) opener guard to bot_only and a matching test. Everything else is correct and the primary bug fix is solid.

[claude]

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

---

Security Review

Tasks

• Load tools
• Fetch PR diff
• Read changed files
• Analyze for security issues
• Post findings

---

One confirmed security issue. No command-injection, SQL-injection, credential-leak, or supply-chain issues found in the changed files. The --self-logins parsing and @me resolution are sound: values pass through a frozenset membership check only, with no shell interpolation.

---

IMPORTANT — botOnly admits self-opener + bot-reply threads; reopens the "actor signs own permission slip" path

Severity: IMPORTANT Confidence: HIGH

Affected code:

claude-code-plugins/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py

Lines 229 to 233 in 2148410

	bot_only = (
	not truncated
	and any(_author_is_bot(c) for c in comments)
	and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
	)

bot_only = (
    not truncated
    and any(_author_is_bot(c) for c in comments)
    and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
)

What goes wrong:

The predicate requires at least one bot comment anywhere in the thread, but does not require that the opening (first) comment is bot-authored. When a configured --self-logins identity opens a review thread — the worker posting their own finding — and a bot account later replies (e.g. an automated reviewer like chatgpt-codex-connector[bot] or any CI bot), every condition is satisfied:

• any(_author_is_bot(c)) → True (bot is comment[1])
• all(_author_is_bot(c) or _author_is_self(c)) → True (comment[0] is self, comment[1] is bot)
• bot_only = True

classify() then returns "eligible" (not "skipped-human-thread"), and in --autonomous mode the thread is resolved once isOutdated — which the worker itself triggers with a commit. This is the exact "actor signing its own permission slip" scenario the design of --autonomous explicitly exists to prevent (documented at line 34 of the same file: "a self-resolved thread would otherwise satisfy the merge gate's 'zero unresolved threads' predicate — the actor signing its own permission slip").

The humanThreadsActed counter in the output does surface this (the self-login opener is classified as non-bot for that counter since extra_bot_logins is a distinct frozenset), but that is a reporting signal, not a mechanical guard — in unattended mode it does not block the resolve.

There are no tests covering the self-opener + bot-reply shape; test_self_only_thread_is_not_bot_only tests the self-only case (correctly false) and test_genuine_third_party_human_still_blocks_bot_only tests the genuine human case, but neither tests the [self-opens, bot-replies] ordering.

Fix:

Replace any(_author_is_bot(c) for c in comments) with a check that the first comment is bot-authored:

bot_only = (
    not truncated
    and bool(comments)
    and _author_is_bot(comments[0])          # opener must be bot-authored
    and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
)

This is strictly compatible with the intended flow (bot opens → worker replies → botOnly = True), tightens the invariant to match the docstring ("HUMAN-authored threads are NEVER touched"), and is consistent with the reference/review-discipline.md rule the Codex review cites. first_author is already computed at line 220 and its data is already exposed in the returned dict's "author" and "authorType" fields — _author_is_bot(comments[0]) reuses the exact same _author_is_bot closure already in scope.

Fix this →

---

[claude]

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

---

PR Review

• Read current state of babysit_resolve_thread.py to check if opener fix was applied
• Review changed files
• Assess whether previous finding was addressed
• Provide review feedback

---

Summary

The Codex P1 finding, confirmed independently by both prior Claude reviews, has been addressed in commit c670cda. The PR is ready to merge.

---

Previous finding: resolved

botOnly now requires the thread opener to be bot-authored

babysit_resolve_thread.py:249–254

bot_only = (
    not truncated
    and bool(comments)
    and _author_is_bot(comments[0])          # opener must be bot-authored
    and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
)

The any(_author_is_bot(c) for c in comments) clause that allowed a self-opener + bot-reply thread to report botOnly = True has been replaced with _author_is_bot(comments[0]), which agrees with the humanThreadsActed call site that has applied the same opener check since #512. This closes the "actor signs own permission slip" path under --autonomous.

Test coverage for BotOnlyRequiresABotOpener (lines 372–506) covers the full matrix the prior reviews identified as missing:

Scenario	Expected	Test
bot-opened + self-reply	True	test_bot_opened_with_self_reply_is_bot_only
self-opened + bot-reply	False	test_self_opened_with_bot_reply_is_not_bot_only ← the exploit path
bot-opened + third-party human	False	test_bot_opened_with_third_party_human_reply_is_not_bot_only
bot-opened alone	True	test_bot_opened_alone_is_bot_only
human-opened + bot-reply (no self-logins)	False	test_human_opened_with_bot_reply_is_not_bot_only
no comments	False	test_no_comments_is_not_bot_only
null-author opener	False	test_opener_with_no_author_is_not_bot_only
truncated with bot opener	False	test_truncated_page_is_not_bot_only_even_with_a_bot_opener
end-to-end under --autonomous --only-outdated	skipped-human-thread	test_end_to_end_self_opened_thread_is_skipped_under_autonomous

---

Everything else: correct

No new issues found in the additional commit. The primary fix (--self-logins neutralization), the second-order fix (_severity_scan_body stripping classification-table rows from self-comments), the strip_classification_rows promotion, the @me resolution ordering, and all documentation updates were all correct as assessed in the prior review — and are untouched by the follow-up commit.

[claude]

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

---

Security Review

• Load tools
• Fetch PR diff
• Read changed files
• Analyze for security issues
• Post findings

---

The Prior Critical Finding Has Been Resolved

The Codex P1 finding and both earlier Claude passes (commits d4a618c9ba and 2148410b20a0e1c6f88e63384a0f7e67dea36f8a) identified that botOnly lacked an opener check, which would allow a self-opened thread with a subsequent bot reply to satisfy the any(_author_is_bot(...)) predicate and become eligible for autonomous resolution — the "actor signs their own permission slip" scenario. That finding was correctly applied before this run. The current branch HEAD (5a203a7) already contains:

bot_only = (
    not truncated
    and bool(comments)
    and _author_is_bot(comments[0])          # opener must be bot-authored
    and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
)

The BotOnlyRequiresABotOpener test class (new in this PR) explicitly tests test_self_opened_with_bot_reply_is_not_bot_only and test_end_to_end_self_opened_thread_is_skipped_under_autonomous. No action required on that finding.

---

No Additional Security Issues Found

I reviewed all changed files for the vulnerability classes this lane covers. Findings for each surface:

--self-logins input handling. The CLI argument is split on commas, casefolded, and placed in a frozenset by normalize_self_logins. It is used only for str.casefold() in frozenset membership tests — never passed to a shell command, interpolated into a query, or treated as a regex. No injection surface.

@me resolution (resolve_author / run_gh). The gh api user --jq .login call runs via subprocess.run with an explicit list — no shell expansion. The returned login string is consumed only as a frozenset member for later equality comparison. The fallback on RuntimeError (drop unresolved @me, keep explicit tokens) is conservatively correct: an unresolved identity is simply absent from the self set and is therefore judged as a third-party human commenter, the safer outcome.

API data trust boundary. comment.body and author.login come from the GitHub GraphQL API. Both flow exclusively through regex matching (_has_severity_marker, CLASSIFY_ROW_RE) and frozenset membership tests. No deserialization, no eval, no shell interpolation — content from a malicious commenter body cannot escape the string-comparison path.

Self-login impersonation. _author_is_self checks author.login (the API-reported GitHub username) against the operator-configured --self-logins frozenset. GitHub usernames are globally unique; a third-party commenter cannot set their username to match the worker's configured login. A comment authored by a genuine third party is never treated as self-authored regardless of body content.

strip_classification_rows promotion. The _strip_classification_rows → strip_classification_rows rename is purely a visibility change. The regex (CLASSIFY_ROW_RE) and logic are identical; the only effect is that callers outside babysit_classify.py can now import it. No behavioral change, no new trust surface.

Severity scan stripping scope. Stripping is limited to classification-table rows (lines matching ^[ \t]*(?=\|).*VALID|INCORRECT|UNCERTAIN...). Non-table prose in a self-authored comment still reaches _has_severity_marker. Third-party comments — including bot comments containing genuine severity markers — are never stripped, regardless of --self-logins. The test_self_non_table_prose_still_flags_severity and test_third_party_severity_still_flags_regardless_of_self_logins tests verify both constraints.

Double-membership edge case (extra_bot_logins ∩ self_logins ≠ ∅). If an operator lists the same login in both flags, _author_is_bot returns True for that commenter via the extra_bot_logins path, and _author_is_self also returns True. The opener check uses _author_is_bot, so such a comment would satisfy it as a bot opener; severity scanning would strip its table rows. This is an operator configuration mistake and the operator is the trust root, so this is not a security vulnerability. The combination cannot be induced by a third party.

---

No security issues to report.

[claude]

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

---

PR Review

• Gather context and understand the PR
• Check latest commit (4f7c1df)
• Confirm prior findings are resolved
• Provide final review

---

Summary

The latest commit (4f7c1df) is a one-line CHANGELOG formatting fix — it adds the blank line between the 0.34.1 entry's last list item and the ## [0.34.0] heading that the merge-conflict resolution consumed. This addresses markdownlint MD032/MD022. No logic, behavior, or test changes are included.

---

Status of prior findings

Finding	Status
P1 (Codex + both Claude passes): botOnly accepted bot-anywhere rather than requiring a bot opener — self-opener + bot-reply thread could satisfy botOnly = True	Resolved in c670cda: predicate now requires _author_is_bot(comments[0])
Second-order: severity re-stranding via classification-table rows in self-comments	Addressed in original PR: _severity_scan_body strips classification-table rows before scanning self-authored comments
CHANGELOG formatting (MD032/MD022): blank line between 0.34.1 entry and ## [0.34.0] heading	Resolved in 4f7c1df (this commit)

All three findings are resolved. No new issues introduced.

---

Verdict

The PR is correct and ready to merge. The fix is sound, test coverage is thorough (453 tests, 11 targeted regressions covering the exact failure modes), documentation is fully updated, and all CI gates (markdownlint, check-changelog-parity, check-skill) pass.

[claude]

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

---

Security Review

• Fetch PR diff and read changed files
• Analyze for security issues (logic flaws, injection, auth gaps, token handling)
• Post findings

---

Prior Critical Finding: Resolved

The prior critical finding — botOnly admitting a self-opener + bot-reply thread, reopening the "actor signs own permission slip" path under --autonomous — was correctly addressed in commit c670cda and confirmed at HEAD 4f7c1df:

bot_only = (
    not truncated
    and bool(comments)
    and _author_is_bot(comments[0])          # opener must be bot-authored
    and all(_author_is_bot(c) or _author_is_self(c) for c in comments)
)

The only change between the last reviewed HEAD (5a203a7) and the current HEAD (4f7c1df) is a single blank line restored in CHANGELOG.md. No Python code changed.

---

No Additional Security Issues Found

I reviewed all 8 changed files against the vulnerability classes this lane covers.

--self-logins input handling. The CLI argument is split on commas, casefolded, and stored in a frozenset. It is consumed only via str.casefold() in frozenset membership tests — never shell-interpolated, never passed to a query, never treated as a regex. No injection surface.

@me resolution (resolve_author / run_gh). gh api user --jq .login runs via subprocess.run with an explicit list — no shell expansion. The resolved login flows into a frozenset for equality comparison only. The RuntimeError fallback (drop unresolved @me, keep explicit tokens) is correctly fail-closed: an unresolved identity is absent from the self set and is therefore classified as a third-party human commenter.

API data trust boundary. comment.body and author.login come from the GitHub GraphQL API. Both flow exclusively through regex matching (_has_severity_marker, CLASSIFY_ROW_RE) and frozenset membership tests. No deserialization, no eval, no shell interpolation. A malicious body cannot escape the string-comparison path.

Self-login impersonation. _author_is_self checks author.login (the API-reported GitHub username) against the --self-logins frozenset. GitHub usernames are globally unique; a third-party commenter cannot register the worker's configured login to have their comment treated as self-authored.

Severity scan strip scope. Stripping is gated on _author_is_self(comment) (the API-reported login), applied only to that comment's classification-table rows. Third-party comments — including bot comments with genuine severity markers — are never stripped regardless of --self-logins. The test_third_party_severity_still_flags_regardless_of_self_logins and test_self_non_table_prose_still_flags_severity tests verify both constraints.

Double-membership edge case (extra_bot_logins ∩ self_logins ≠ ∅). If an operator lists the same login in both flags, _author_is_bot returns True for that commenter via the extra_bot_logins path, so a self-opened thread would satisfy the opener check. This is an operator configuration mistake (the operator controls both flags) and cannot be induced by a third party. Not a vulnerability.

---

No security issues to report.