Skip to content

fix(source-control): thread extra_bot_logins into resolve_thread.py - #1337

Merged
kyle-sexton merged 7 commits into
mainfrom
fix/637-babysit-resolve-thread-extra-bot-logins
Jul 26, 2026
Merged

fix(source-control): thread extra_bot_logins into resolve_thread.py#1337
kyle-sexton merged 7 commits into
mainfrom
fix/637-babysit-resolve-thread-extra-bot-logins

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #637

This was generated by AI during work-loop execution.

Summary

babysit_resolve_thread.py called the shared is_bot classifier at two sites without passing
the caller's extra_bot_logins config — project_thread's botOnly computation (L117-120) and
the humanThreadsActed reporting counter (L477) — unlike every other classifier call site (e.g.
actor_kind in babysit_classify.py). An operator who registered a non-structural bot account
via babysit_extra_bot_logins (no [bot] login suffix, API __typename reports User) had
that account's threads miscategorized at both sites. Pre-existing relative to #534/#634 (that PR
migrated these call sites to the shared is_bot but did not introduce the omission).

This PR adds an --extra-bot-logins CLI flag to babysit_resolve_thread.py (same
comma-separated shape as the snapshot wrapper's flag), threads it through project_thread and
fetch_threads via a closure, and passes it to both is_bot call sites. SKILL.md's
babysit_extra_bot_logins flag-delivery table now lists resolve-thread alongside snapshot.
Patch-bumps source-control to 0.26.3 with a matching CHANGELOG entry.

Test plan

  • Added ProjectThreadExtraBotLogins (site 1) and HumanThreadsActedExtraBotLogins (site 2) to
    tests/test_babysit_resolve_thread.py — each asserts a configured non-structural login is
    correctly classified as bot, and that an unconfigured one still falls back to structural
    detection alone (regression coverage for both directions).
  • python -m pytest tests/ -q in plugins/source-control/skills/babysit-prs/scripts — 352
    passed, 58 subtests passed (full existing suite, no regressions).
  • python -c "import ast; ast.parse(...)" — syntax check on the modified script.

Related

N/A

🤖 Generated with Claude Code

…thread.py's is_bot calls

Two is_bot call sites in babysit_resolve_thread.py -- project_thread's botOnly
computation and the humanThreadsActed counter -- omitted the caller's
extra_bot_logins config, unlike every other classifier call site (e.g.
actor_kind in babysit_classify.py). An operator who registered a
non-structural bot account via babysit_extra_bot_logins (no [bot] suffix,
API __typename reports User) had that account's threads miscategorized at
both sites.

Add an --extra-bot-logins CLI flag (matching the snapshot wrapper's shape),
thread it through project_thread and fetch_threads via a closure, and pass
it to both is_bot call sites. Update SKILL.md's flag-delivery table to list
resolve-thread as a consumer, and bump the plugin patch version with a
CHANGELOG entry.

Closes #637

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: fix(source-control): thread extra_bot_logins into resolve_thread.py

  • Gather context and understand the request
  • Review PR diff and changed files
  • Analyze correctness of the fix
  • Check test coverage
  • Review documentation/changelog updates
  • Post final review

This is a well-targeted, minimal fix that correctly resolves a real omission. Here's my analysis.

Summary

The bug: babysit_resolve_thread.py called is_bot() at two sites without forwarding extra_bot_logins, unlike every other call site in the codebase. An operator-registered non-structural bot account (no [bot] suffix, __typename = "User") would be misclassified as human at both sites, causing its threads to be un-resolvable and inflating the humanThreadsActed counter.

The fix correctly threads extra_bot_logins through the call chain: CLI arg → parse_extra_bot_loginsfetch_threads → closure → project_threadis_bot (site 1), and separately into the humanThreadsActed list comprehension (site 2).

Correctness

Site 1 — project_thread's botOnly computation
babysit_resolve_thread.py L101–129: The extra_bot_logins keyword-only parameter with frozenset() default is the right shape — backward-compatible, explicit, and consistent with the rest of the codebase.

Site 2 — humanThreadsActed counter
babysit_resolve_thread.py L499–506: The extra_bot_logins variable is in scope (parsed at L298), so the fix is straightforward and correct.

Closure pattern in fetch_threads
babysit_resolve_thread.py L154–171: The inner _project closure is a clean way to forward extra_bot_logins to project_thread while honoring the projection callback contract. functools.partial would also work but the closure is more readable here.

parse_extra_bot_logins
babysit_resolve_thread.py L217–220: Semantically identical to _csv() in pr_queue_snapshot.py — strips whitespace, skips empty parts, returns frozenset. The duplication is fine; these are standalone CLI scripts that shouldn't cross-import for a utility this small.

No correctness issues found.

Test Coverage

tests/test_babysit_resolve_thread.py L95–147

The four new test methods are well-structured:

  • ProjectThreadExtraBotLogins.test_configured_login_is_bot_only — calls project_thread() directly with extra_bot_logins=frozenset({"svc-account"}), asserts botOnly=True. Directly exercises site 1.
  • ProjectThreadExtraBotLogins.test_unconfigured_login_is_not_bot_only — same input without extra_bot_logins, asserts botOnly=False. Confirms structural-detection-only fallback.
  • HumanThreadsActedExtraBotLogins.test_configured_login_excluded_from_human_count — runs main() end-to-end with --extra-bot-logins svc-account and a bot_only=True thread, asserts humanThreadsActed=0. Exercises site 2 via the CLI.
  • HumanThreadsActedExtraBotLogins.test_unconfigured_login_counts_as_human — same thread without the flag, asserts humanThreadsActed=1. Regression coverage for the counter.

One observation: the HumanThreadsActedExtraBotLogins tests mock fetch_threads, so they don't exercise the CLI→fetch_threads_projectproject_thread chain. But since ProjectThreadExtraBotLogins directly tests project_thread() and the closure is trivially correct, this gap doesn't represent meaningful risk. An integration test passing --extra-bot-logins all the way through a live fetch_threads call would be overkill given the structure.

Documentation and Versioning

CHANGELOG — The 0.26.3 entry accurately describes both omission sites, the severity characterization ("Low severity — dormant unless an operator has configured the userConfig key"), and the pre-existing nature relative to #534/#634. No issues.

SKILL.md — The one-line change updating --extra-bot-logins (snapshot)--extra-bot-logins (snapshot, resolve-thread) in the flag-delivery table is correct and complete.

plugin.json — Patch bump 0.26.2 → 0.26.3 is appropriate.

Overall

The change is correct, minimal, consistent with codebase patterns, and properly tested in both directions for both affected call sites. No blocking issues.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


Security Review

  • Gather PR diff and changed files
  • Review babysit_resolve_thread.py changes
  • Review test changes
  • Review plugin.json, CHANGELOG.md, SKILL.md changes
  • Report findings

Findings

No security issues found.

Scope reviewed: babysit_resolve_thread.py (new --extra-bot-logins flag, parse_extra_bot_logins, updated project_thread / fetch_threads signatures, two is_bot call sites), test_babysit_resolve_thread.py (two new test classes), plugin.json, CHANGELOG.md, SKILL.md.

What was checked and why it's safe:

Injection surfaces (command, path, template)parse_extra_bot_logins does only comma-splitting and whitespace-stripping; the resulting frozenset[str] is consumed exclusively by is_bot for set-membership tests (in normalize_login_set(...)). The value is never interpolated into a shell command, file path, SQL query, or template string. No injection surface exists.

Trust boundary for extra_bot_logins — the value originates from ${user_config.babysit_extra_bot_logins} (operator-controlled config), not from untrusted external input such as PR content, commit messages, or GitHub API-returned user-controlled fields. The risk of an attacker influencing the set is out-of-scope for this change.

Authorization bypass via bot-classification — classifying a human as a bot would allow their threads to be auto-resolved, bypassing human review. However, the only way to trigger this is to control the operator's userConfig, which requires the same trust level as other operator-scoped settings already present in this skill. The change does not widen the attack surface compared to what snapshot already exposed.

Mutable-default-argument footgun — both project_thread and fetch_threads default to frozenset() (immutable), not [] or set(). No shared-mutable-state risk.

GitHub Actions patterns — no workflow files are modified; not applicable.

Secrets / credentials — no new secret handling introduced.

@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: 4184c85bbb

ℹ️ 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/SKILL.md
kyle-sexton and others added 2 commits July 25, 2026 19:31
… command form

The delivery-table row alone did not deliver anything: configuration reaches
these scripts only through CLI flags, and the exact resolver commands the
agent copies -- the two pinned degradation forms in safety.md, the Worker
Contract clause and the Worker Prompt Template in orchestration.md, and the
thread-resolution bullet in SKILL.md -- all still invoked the new option with
its empty default, leaving a registered non-structural bot account's threads
classified as human and skipped in worker tier.

safety.md now states the rule alongside the wrapper's other flag contracts so
a future command form does not drop it again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012SV88ZgGDKdUKEPJaJtDpG
Both sides appended independent test classes to
test_babysit_resolve_thread.py; kept both. main released 0.28.0, so this
branch's entry moves to 0.28.1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012SV88ZgGDKdUKEPJaJtDpG
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

# Conflicts:
#	plugins/source-control/.claude-plugin/plugin.json
#	plugins/source-control/CHANGELOG.md
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 3s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@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: 1f82ce256a

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

…ne skill cap

The #637 addition pushed babysit-prs/SKILL.md to 501 lines, over the
skill-quality hard cap. Rewrapping the thread-resolution bullet at the
file's 100-column prose width restores headroom with identical content.
# Conflicts:
#	plugins/source-control/.claude-plugin/plugin.json
#	plugins/source-control/CHANGELOG.md
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@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: 54267ab45f

ℹ️ 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/reference/safety.md
…er form

safety.md's read-only-forms bullet listed the resolve-thread thread-list
command without the flag, contradicting the rule stated directly below it
that every resolve-thread form — listing and mutating alike — carries
--extra-bot-logins. A listing run without it reports a configured
non-structural bot's threads as human, which is the miscategorization
#637 exists to remove.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@kyle-sexton
kyle-sexton merged commit 0557884 into main Jul 26, 2026
27 checks passed
@kyle-sexton
kyle-sexton deleted the fix/637-babysit-resolve-thread-extra-bot-logins branch July 26, 2026 00:51
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.

source-control(babysit-prs): babysit_resolve_thread.py's is_bot calls omit extra_bot_logins at 2 sites

1 participant