Skip to content

feat(source-control): dedicated --self/--extra-self self-identity flags for babysit snapshot#882

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/511-babysit-self-identity-decouple
Jul 21, 2026
Merged

feat(source-control): dedicated --self/--extra-self self-identity flags for babysit snapshot#882
kyle-sexton merged 3 commits into
mainfrom
fix/511-babysit-self-identity-decouple

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Fixes #511. The babysit PR-queue snapshot derived self_logins — the posting identities whose comments self-classification suppresses — from the --author discovery filter, conflating two distinct concerns (which authors' PRs to discover vs whose comments to suppress). That broke in both directions:

  • Under-inclusion: configured extra self identities (babysit_self_logins) rode on --author, so when autopilot widening drops --author entirely, a bot poster's own comments re-fired new_human_blocking_feedback every cycle.
  • Over-inclusion: a --queue --author alice run authenticated as a different login made alice a self-login, suppressing Alice's genuine feedback from the worker-dispatch arm.

The fix

pr_queue_snapshot.py now resolves self-identity from dedicated --self (full override — exactly the given logins, @me not added) / --extra-self (added on top of the authenticated @me) flags, mirroring the existing babysit-readiness-gate.sh semantics, resolved independently of --author and before the --pr/--queue scope split (so single-PR scope is covered too). This supersedes the @me-only union added in #494, and the author-derived self fallback in build_config is removed so no discovery author can leak into the self set in any path.

The skill's step-4 invocation and the babysit_self_logins userConfig table row now route the configured extras through --extra-self instead of overloading --author @me,<self-logins>; because self-identity no longer rides on --author, it survives autopilot widening.

Tests (red-green, with transparency on the rewritten spec)

  • Added SelfIdentityDecouplingTests asserting both directions at the build_snapshot level (extra-self survives a dropped --author; a discovery author is not treated as self). Verified these fail against the pre-fix author-union code and pass after the fix.
  • The resolve_self_logins unit tests pinned the old author-union contract; they are rewritten to the new flag-based contract, and test_discovery_authors_are_unioned_with_the_self_login (which asserted the over-inclusion behavior now fixed) is deleted. test_raw_author_fallback_drops_me_when_unresolved becomes test_missing_resolved_self_logins_yields_empty_not_author. These are a ratified spec change (the issue's converged decision), not test weakening.

Verification (local gates)

  • python -m unittest discover -s tests -p 'test_*.py' (the CI entry via engine.test.sh) — 341 tests OK.
  • check-changelog-parity.sh --check-bump origin/main — PASS (0.16.0 entry present).
  • check-skill-portability.sh origin/main — PASS.
  • check-changed-skills.sh — SKILL.md 499/500 lines (under the hard cap); the one reported "engine.test.sh failed" is the pre-existing repo-wide ruff E402 quirk that does not reproduce in CI (main is green; merged babysit PRs chore: declare shell: bash on every skill with dynamic-context injections #860/fix(source-control): actionable owner/repo diagnostic on readiness-gate fetch failure #839 pass skill-quality-gate), and this change adds zero new ruff errors (32 before == 32 after).
  • markdownlint-cli2 on the changed docs — clean; plugin.json valid.
  • Independent fresh-context code review (producer ≠ reviewer).

Version

Per-plugin bump 0.15.9 → 0.16.0 (minor — new --self/--extra-self CLI surface alongside the behavior fix). marketplace.json pins no version for source-control, so only plugin.json changes.

Closes #511

Related

🤖 Generated with Claude Code

…gs for babysit snapshot

The babysit snapshot derived `self_logins` (the posting identities whose
comments self-classification suppresses) from the `--author` discovery
filter, conflating two distinct concerns. That broke in both directions
(#511): configured extra self identities were dropped whenever autopilot
widening drops `--author` (a bot poster's own comments re-fired
`new_human_blocking_feedback` every cycle), and a discovery `--author` for
a different login was wrongly treated as self (suppressing that author's
genuine feedback from the worker-dispatch arm).

`pr_queue_snapshot.py` now resolves self-identity from dedicated `--self`
(full override) / `--extra-self` (added to `@me`) flags, mirroring
`babysit-readiness-gate.sh`'s flag semantics, independent of `--author`
and for both `--queue` and `--pr` scope. This supersedes the `@me`-only
union from #494 and removes the author-derived self fallback in
`build_config` (and the now-dead `resolved_authors` carrier), so no
discovery author can leak into the self set.

Red-green regression tests assert both directions at the `build_snapshot`
level. The `resolve_self_logins` unit tests that pinned the old
author-union contract are rewritten to the new flag-based contract.

Structural parity with the gate, not exact on one edge: an unresolvable
`@me` raises (fail-loud) rather than degrading to the extras; that parity
decision is tracked in #881.

Per-plugin bump 0.15.9 -> 0.16.0 (minor: new CLI surface) + CHANGELOG.

Closes #511
Refs #881

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review — feat(source-control): dedicated --self/--extra-self self-identity flags for babysit snapshot

  • Gather context and read changed files
  • Review pr_queue_snapshot.py (core logic change)
  • Review test_pr_queue_snapshot.py (test coverage)
  • Review SKILL.md (skill invocation changes)
  • Review plugin.json and CHANGELOG.md
  • Compile and post findings

Summary

This is a clean, well-scoped fix for #511. The conceptual separation of discovery authors from self-identity is correct and the implementation follows through cleanly. The regression test structure — asserting both the under-inclusion and over-inclusion directions at the build_snapshot level — is exactly the right shape for this kind of invariant. Four observations below, none blockers.


Observations

1. if self_login: guard is inconsistent with the documented fail-loud contract (Low)

pr_queue_snapshot.py L120–122:

self_login = gh.resolve_author("@me")
if self_login:
    source.append(self_login)

The docstring (L113–114) says:

an unresolvable @me raises here (via resolve_author) rather than degrading to the extras as the gate does — fail-loud on broken auth

But the if self_login: guard silently swallows a None return. If gh.resolve_author ever returns None instead of raising (the old test test_unresolvable_self_login_leaves_discovery_authors_intact showed it could), the code degrades silently rather than fail-loud, contradicting the docstring.

The test test_unresolvable_authenticated_login_raises only tests the RuntimeError-propagation path (correct behavior when resolve_author raises). There is no test for the None-return path, which the if self_login: guard implies is reachable. If the contract is truly "it always raises", the guard is dead code and could be removed; if None is possible, the guard should either raise explicitly or the docstring should be updated to say "degrades". Tracked in #881, but worth a note here for future readers.


2. --self + --extra-self together silently drops --extra-self (Low)

pr_queue_snapshot.py L116–122:

if self_csv:
    source = _csv_list(self_csv)
else:
    source = _csv_list(extra_self_csv)
    ...

When both --self and --extra-self are supplied, --extra-self is silently ignored. This matches the gate's semantics (presumably), but neither the --extra-self help text nor the --self help text documents this precedence. A user who passes both expecting them to union will be surprised. Consider adding "If --self is also provided, --extra-self is ignored" to the --extra-self help string. There's also no test for this edge case.

Fix this →


3. Minor duplication between _csv and _csv_list (Trivial)

pr_queue_snapshot.py L44–51:

def _csv(value: str | None) -> frozenset[str]:
    return frozenset(
        part.strip() for part in (value or "").split(",") if part.strip()
    )

def _csv_list(value: str | None) -> list[str]:
    return [part.strip() for part in (value or "").split(",") if part.strip()]

Near-identical bodies with only the container type differing. _csv_list could be defined as list(_csv(value)) — though that loses insertion order, which matters for the dedup logic in resolve_self_logins. The current duplication is harmless; just noting it.


4. SKILL.md step 4 — unquoted <self-logins> placeholder (Trivial)

SKILL.md L388–394:

append `--extra-self <self-logins>`

The value could contain commas (bot1,bot2). If the skill assembles the shell command without quoting, word splitting would break it. This was also true of the old --author @me,<self-logins> form, so it's at parity with the prior text — but worth quoting in the next pass as --extra-self "<self-logins>".


What's correct

  • Core fix is sound. resolve_self_logins no longer reads from authors; build_config no longer falls back to resolved_authors; no discovery-author path can leak into the self set.
  • Both regression directions are covered. SelfIdentityDecouplingTests.test_extra_self_survives_when_author_is_dropped and test_discovery_author_is_not_treated_as_self target exactly the two bugs described in babysit-prs: decouple self-identity from --author so configured self-login extras survive autopilot widening #511 and are asserted at the build_snapshot level — the right depth.
  • @me is still resolved by default. resolve_self_logins(None, None) resolves @me internally, so callers that only use --author @me (without --self/--extra-self) still get the authenticated login in the self set.
  • --pr scope is covered. Self-identity resolution happens before the --pr/--queue scope split in build_snapshot, so single-PR runs get the correct self set.
  • Spec-change tests are transparent. The deleted test_discovery_authors_are_unioned_with_the_self_login asserted the bug, not a feature — its removal is appropriate and the PR description calls it out clearly.
  • Version bump and CHANGELOG are correct for a minor (new CLI surface + behavior fix).

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #882

  • Gather PR diff and changed files
  • Review pr_queue_snapshot.py for injection, logic, and credential handling
  • Review SKILL.md for command injection via context variables
  • Review test file for security regressions
  • Review plugin.json and CHANGELOG.md
  • Compile and post findings

Summary

No CRITICAL or IMPORTANT vulnerabilities found in this diff. The core change — decoupling self_logins resolution from the --author discovery filter — is implemented safely: the new --self/--extra-self flags are parsed via argparse, their values are used only as Python strings for set membership comparisons, and no shell execution is performed with user-controlled values inside the Python code itself. Two low-severity observations follow.


SUGGESTION — --self "" silently bypasses the full-override contract | Confidence: Medium

File: pr_queue_snapshot.py, lines 116–122

if self_csv:
    source = _csv_list(self_csv)
else:
    source = _csv_list(extra_self_csv)
    self_login = gh.resolve_author("@me")
    if self_login:
        source.append(self_login)

self_csv comes from getattr(args, "self_logins", None), which argparse sets to the --self flag value. If someone passes --self "" (empty string), self_csv is "" — falsy in Python — so the if self_csv: branch is skipped and the code falls through to the else path, which resolves and appends @me. The result is ["<me>"], not an empty self-set.

This is inconsistent with the documented contract: "Full override of the self-identity set: exactly these comma-separated logins are treated as self … @me is NOT added." An empty --self value should produce an empty self-set (no suppression), but instead produces [@me]. The skill's step-4 instructions would only supply this via the babysit_self_logins userConfig expansion, making the scenario narrow — but a direct CLI caller expecting no-self-suppression with --self "" would be silently wrong.

A simple guard fixes it: treat argparse None (flag absent) and "" (flag present but empty) distinctly, e.g. by keeping argparse's default=None and checking self_csv is not None instead of truthiness, then delegating to _csv_list only when the flag was supplied.

Fix this →


SUGGESTION — plugin.json babysit_self_logins description references "discovery scope" which no longer applies | Confidence: High

File: plugin.json, line 36

"description": "Extra GitHub posting identities … the self set babysit-prs uses for
  discovery scope, readiness-gate classification rows, and the merge-gate self-exemption."

After this fix babysit_self_logins is no longer wired to --author and no longer affects discovery scope. The description still says "discovery scope", which could cause an operator to believe their self-login configuration governs which PRs are discovered, potentially leading to incorrect --author / --owners mental models. While not directly exploitable, a stale trust-boundary description in a configuration table is the kind of thing that leads to misconfiguration. The SKILL.md table row (line 291) was correctly updated; plugin.json was not.

Fix this →


Notes on non-findings

  • SKILL.md ${user_config.babysit_self_logins} interpolation / shell injection: The userConfig value is expanded into the skill's prose instructions, not into a machine-executed template. The LLM assembles the shell command at runtime. A maliciously crafted config value could influence the constructed command (e.g., flag injection via unquoted expansion), but this is a self-attack vector — the user controls their own userConfig and would only affect their own session. This injection surface is equivalent to the previous --author @me,<self-logins> interpolation; this PR does not worsen it.
  • if self_login: dead-code guard (line 121): The docstring and test comment note that in production resolve_author("@me") raises rather than returning None/falsy, making the if self_login: check effectively unreachable in production. Not a security issue, but the guard silently swallows a falsy return that the contract says shouldn't happen — worth noting against source-control:babysit-prs: pr_queue_snapshot self-identity resolution raises on unresolvable @me instead of degrading to --extra-self (readiness-gate parity) #881.
  • No subprocess/shell execution with user-controlled values in the changed Python code.
  • No new workflow files, permissions changes, or pull_request_target patterns in this diff.

@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: 20f974366a

ℹ️ 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 added a commit that referenced this pull request Jul 21, 2026
claude-ops: #844 rebased and re-derived its claim from 0.17.2 to 0.17.4,
colliding exactly with this branch's prior 0.17.4. Bump to 0.17.5.

source-control: a new PR, #882 (fix/511-babysit-self-identity-decouple),
claims 0.16.0 — a minor bump above this branch's prior 0.15.11 and above
#840's 0.15.10. Bump to 0.16.1 to stay ahead of both.

repo-hygiene (0.4.6), guardrails (0.9.6), and work-items (0.19.1) were
re-verified fresh against current main and every live open PR
(main: repo-hygiene 0.4.5, guardrails 0.9.5, work-items 0.19.0; #861 0.19.0)
and remain correct, no change needed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
kyle-sexton added a commit that referenced this pull request Jul 21, 2026
#844 (claude-ops) merged into main at exactly 0.17.4, which this branch's
prior 0.17.5 was already one past — kept as-is, no re-bump needed for the
number itself, just resolved the resulting plugin.json/CHANGELOG.md conflict.

Full fresh collision sweep after the merge found claude-ops now has NO open-PR
collision at all. It also found #861 (work-items) re-derived its own claim
from 0.19.0 to 0.20.0 since the last check (following #857's Jira-adapter
minor bump into main) — colliding with this branch's prior 0.19.1. Re-bumped
work-items to 0.20.1.

source-control (0.16.1, held behind #882's 0.16.0 and #840's 0.15.10),
repo-hygiene (0.4.6, no collision), and guardrails (0.9.6, no collision)
re-verified against current main and all live open PRs — unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
`resolve_self_logins` tested `self_csv` for truthiness, so an explicit
`--self ""` (flag supplied, empty value) fell through to the "flag
omitted" branch and resolved/added `@me` -- silently contradicting the
documented full-override contract ("`@me` is NOT added"). Switch the
guard to `is not None` so only an actually-omitted flag falls through.

Also document that `--self` takes precedence over `--extra-self` when
both are supplied, and drop `plugin.json`'s stale `babysit_self_logins`
description clause: self-identity no longer feeds discovery scope after
the `--author`/self decoupling in this branch.

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

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


  • Gather diff and changed files
  • Review new commit 51d4760 (--self "" empty-override fix)
  • Check fix correctness and test coverage
  • Review plugin.json description update
  • Compile findings

Review — follow-up commit 51d4760

This commit addresses four of the five observations raised in the earlier code and security reviews. All four fixes are correct and well-targeted.


What was fixed

--self "" silently bypassed the full-override contract (was: Security SUGGESTION)

pr_queue_snapshot.py L116:

# Before:  if self_csv:
# After:   if self_csv is not None:

Correct fix. None (flag absent) and "" (flag present, empty value) are now distinguished. --self "" yields an empty self-set as documented, rather than silently falling through to resolve and append @me.

Covered by the new test_empty_self_is_a_full_override_yielding_no_logins test, which mocks resolve_author to AssertionError — ensuring the @me resolution path is not entered at all. That's the right shape for this invariant.


--self + --extra-self precedence was undocumented (was: Code review Observation 2)

Both --self and --extra-self help strings now document the precedence explicitly. The new test_self_takes_precedence_over_extra_self_when_both_supplied test also mocks resolve_author to AssertionError, confirming @me is never resolved when --self wins — closes the gap cleanly.


plugin.json babysit_self_logins description referenced "discovery scope" (was: Security SUGGESTION)

Removed. The description now correctly reads: "the self set babysit-prs uses for readiness-gate classification rows and the merge-gate self-exemption." Matches what SKILL.md line 291 already said.


What remains open (pre-existing, tracked)

if self_login: guard vs. fail-loud docstring — still present at L121. If resolve_author("@me") always raises on failure (as the docstring and test_unresolvable_authenticated_login_raises assert), the guard is dead code; if it can return falsy, the guard silently degrades where the contract says fail-loud. Either way it's a documentation/cleanup item, not a behavior regression. Tracked in #881.

SKILL.md --extra-self <self-logins> unquoted placeholder — not addressed here, at parity with the previous --author @me,<self-logins> form. Low priority.


Codex comment on discovery scope

The Codex review comment on SKILL.md L389 argues that babysit_self_logins extras should also feed --author for non-autopilot discovery. This is the pre-fix behavior that #511 explicitly removes — self-identity should not ride on the discovery filter. The Codex suggestion inverts the fix's goal and can be dismissed.


Overall

The core fix in commit 20f9743 is sound. Commit 51d4760 closes the remaining observations cleanly without introducing new concerns. The test suite covers both regression directions (SelfIdentityDecouplingTests) and the new edge cases (test_empty_self_is_a_full_override_yielding_no_logins, test_self_takes_precedence_over_extra_self_when_both_supplied). Ready to merge.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #882

  • Gather PR diff
  • Review pr_queue_snapshot.py — injection, logic, credential handling
  • Review SKILL.md — command injection via context variables
  • Review test file — security regressions
  • Review plugin.json and CHANGELOG.md
  • Compile and post findings

Summary

No CRITICAL or IMPORTANT vulnerabilities found. The two SUGGESTION-level findings from the prior security review on commit 20f9743 have both been resolved in the follow-on commit 51d4760 (also in this PR). One low-confidence residual observation below.


Prior findings: both resolved

SUGGESTION 1 — --self "" bypasses the full-override contract (from prior review)

Commit 51d4760 fixes this: pr_queue_snapshot.py L116 now reads if self_csv is not None: (not a truthiness check), so an empty --self "" correctly produces an empty self set rather than falling through to add @me. The corresponding test test_empty_self_is_a_full_override_yielding_no_logins (which asserts resolve_author is never called when self_csv is "") is present and covering the fix.

SUGGESTION 2 — stale "discovery scope" in plugin.json description (from prior review)

Resolved: the plugin.json description now reads "readiness-gate classification rows and the merge-gate self-exemption", with "discovery scope" removed.


Residual observation

if self_login: dead-code guard | Severity: informational | Confidence: Low

pr_queue_snapshot.py L121

self_login = gh.resolve_author("@me")
if self_login:
    source.append(self_login)

The docstring states resolve_author("@me") raises on broken auth (fail-loud, not degrade); test_unresolvable_authenticated_login_raises confirms the raise-path is what's tested. If resolve_author returns None or "" instead of raising, the guard silently omits the authenticated login from the self set, causing the bot's own comments to be classified as human feedback rather than suppressed — a functional misnomer, not an unauthorized-access or data-egress risk. This is tracked in #881 and not a security concern.


Notes on non-findings

  • No subprocess / shell execution with user-controlled values in the changed Python code. --self and --extra-self flag values are used exclusively as Python strings for in-process casefold set-membership comparison — no shell interpolation in this layer.
  • SKILL.md ${user_config.babysit_self_logins} interpolation: The userConfig value is expanded into the skill's prose instructions; the LLM assembles the resulting shell command at runtime. A crafted config value could influence the constructed command, but this is a self-attack vector identical to the prior --author @me,<self-logins> form. This PR does not widen the surface.
  • No GitHub Actions / workflow changes in this diff.
  • No new remote MCP connections, new subprocess calls, or unguarded file-path expansion in the changed code.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author
# Finding Classification Evidence Reacted
1 --self "" silently bypasses full-override contract VALID — fixing Confirmed: if self_csv: treated empty-string as falsy, falling through to the @me-resolving branch. Fixed in 51d4760 (is not None guard) with a new regression test. 👍
2 plugin.json babysit_self_logins description references stale "discovery scope" VALID — fixing Confirmed: this branch decouples self-identity from --author discovery entirely, so the description was stale. Fixed in 51d4760. 👍

(The "No CRITICAL or IMPORTANT vulnerabilities found" line earlier in this comment is prose, not a live finding — not tabulated.)

@kyle-sexton

Copy link
Copy Markdown
Contributor Author
# Finding Classification Evidence Reacted
1 if self_login: guard inconsistent with fail-loud docstring (Low) VALID (defer) Already tracked in #881 per this comment's own text; out of scope to re-fix here. 👍
2 --self + --extra-self together silently drops --extra-self, undocumented (Low) VALID — fixing Confirmed: no precedence documented and no test existed. Fixed in 51d4760 — added precedence note to both flags' help text and a test_self_takes_precedence_over_extra_self_when_both_supplied regression test. 👍
3 Minor duplication between _csv and _csv_list (Trivial) INCORRECT _csv_list preserves insertion order (needed by the dedup logic in resolve_self_logins); _csv returns a frozenset which does not. Collapsing them (e.g. list(_csv(...))) would lose that ordering, so the duplication is load-bearing, not accidental. Leaving as-is. 👍
4 SKILL.md step 4 unquoted <self-logins> placeholder (Trivial) VALID (defer) Confirmed pre-existing — the comment itself notes it was "also true of the old form," i.e. not introduced by this branch. Out of scope for this PR. 👍

…discovery filter (#897)

Author ruling on the #897 discovery-contract fork (Codex flagged on #882):
filter and stopped joining babysit_self_logins onto --author, but the
userConfig `description` still listed "discovery scope" first — code and
documented contract disagreed.

and "a distinct concern from --author (which authors' PRs to discover)," and
its fix direction explicitly drops the `--author @me,<self-logins>`
extension. So the pre-#511 discovery-widening was an incidental side effect
of the old author-derived self set, not a stated goal. Ruling: keep the
discovery-drop (option B); babysit_self_logins is a
suppression/classification/merge-exemption set only — which authors' PRs the
queue discovers stays --author's job.

Correct the userConfig description to match; no code change (the code already
behaves this way post-#511). Part of the unreleased 0.16.0 (#511) entry.

Refs #511 #897

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review — commit 23de05c (docs: babysit_self_logins is suppression-only)

  • Update comment with task list
  • Read diff of new commit
  • Review changed files
  • Post findings

Summary

Docs-only commit. No code changes — just corrects the plugin.json description and adds a CHANGELOG note to record the author ruling on the discovery-contract fork opened in the prior review cycle.


What changed

plugin.json babysit_self_logins description (L36)

Before:

"the self set babysit-prs uses for readiness-gate classification rows and the merge-gate self-exemption"

After:

"the self set babysit-prs treats as its own: self-comment suppression, same-login classification,
readiness-gate classification rows, and the merge-gate self-exemption. Not a discovery filter —
which authors' PRs the queue discovers is `--author`'s job, independent of this set."

This resolves the code/doc mismatch flagged in the previous review and in the SKILL.md Codex thread: the code already behaved this way post-#511; the description now agrees with it. The enumerated use list ("self-comment suppression, same-login classification, readiness-gate classification rows, merge-gate self-exemption") is accurate against the implementation in pr_queue_snapshot.py. The "Not a discovery filter" sentence is the explicit author ruling on the #897 fork.

CHANGELOG.md 0.16.0 Added entry (view)

Appends a note to the existing 0.16.0 bullet documenting that the discovery-drop is intentional (not a regression), that the pre-#511 --author @me,<self-logins> widening was an incidental side effect of the old author-derived self set, and that #897 (option B) was selected. This is the right call: consumers reading the changelog for a breaking-ish behavior change need to know the intent.


Assessment

Both changes are correct and well-scoped. No concerns:


Remaining open items (pre-existing, tracked)


Overall

This commit closes out the last open thread from the prior review cycles. The PR is clean: the core fix is sound, all raised findings have been addressed or explicitly deferred with tracking issues, and the docs now agree with the code. Ready to merge.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #882

  • Gather PR diff and enumerate changed files
  • Review pr_queue_snapshot.py for injection, logic, and credential handling
  • Review SKILL.md for command injection via context variables
  • Review test file for security regressions
  • Review plugin.json and CHANGELOG.md
  • Compile and post findings

Summary

No CRITICAL or IMPORTANT vulnerabilities found. This review covers the full three-commit diff, including the latest docs-only commit 23de05c. Both SUGGESTION-level findings from the prior security reviews on 20f9743 and 51d4760 have been resolved. One residual informational note below.


Prior SUGGESTION findings: both resolved

SUGGESTION 1 — --self "" bypassed the full-override contract (prior review on 20f9743)

pr_queue_snapshot.py L116 now reads if self_csv is not None: — only a genuinely absent flag falls through to resolve @me. An explicit --self "" correctly produces an empty self-set (no suppression) rather than silently inheriting the authenticated login. The corresponding test test_empty_self_is_a_full_override_yielding_no_logins mocks resolve_author to AssertionError, confirming the @me-resolution branch is never entered. Closed.

SUGGESTION 2 — stale "discovery scope" language in plugin.json description (prior review on 20f9743)

Fully resolved across two commits: 51d4760 dropped the stale clause; 23de05c rewrites the description to explicitly state "Not a discovery filter — which authors' PRs the queue discovers is --author's job, independent of this set" and enumerates the actual uses (self-comment suppression, same-login classification, readiness-gate classification, merge-gate self-exemption). No code change, no security implication. Closed.


Residual note (informational, pre-existing, not a security finding)

if self_login: dead-code guard | Severity: Informational | Confidence: Low

pr_queue_snapshot.py L120–122:

self_login = gh.resolve_author("@me")
if self_login:
    source.append(self_login)

The docstring (L113–114) and test_unresolvable_authenticated_login_raises both assert that resolve_author("@me") raises on broken auth rather than returning None. If that invariant holds, the guard is dead code; if resolve_author can return a falsy value in some edge case, the authenticated login is silently dropped from the self-set and the bot's own comments would be mis-classified as human feedback — a functional defect, not an unauthorized-access or data-egress risk. Tracked in #881, out of scope for this review.


Notes on non-findings

  • No subprocess or shell execution with user-controlled values. --self and --extra-self flag values are parsed by argparse, processed by _csv_list (split/strip only), and used exclusively for in-process casefold set-membership comparisons. No shell interpolation in the Python layer.
  • SKILL.md ${user_config.babysit_self_logins} expansion. The userConfig value is expanded into LLM instruction prose; the LLM assembles the shell command at runtime. A crafted babysit_self_logins value could influence the constructed --extra-self argument (flag injection, word splitting on unquoted commas). This is a self-attack vector — the user controls their own userConfig and can only affect their own session — identical in blast radius to the prior --author @me,<self-logins> form. This PR does not widen the surface.
  • resolve_scope_repos owner-allowlist check (L141–148) is unchanged and remains correct: --repo is validated against --owners before any further processing.
  • No GitHub Actions workflow files changed. No pull_request_target, workflow_run, or permission-widening patterns introduced.
  • No new credential surfaces. No new environment variable reads, token handling, or subprocess calls.
  • Latest commit 23de05c is docs-only (plugin.json description text, CHANGELOG.md). No code execution path changed; no new attack surface.

@kyle-sexton
kyle-sexton merged commit 7d3b596 into main Jul 21, 2026
23 checks passed
@kyle-sexton
kyle-sexton deleted the fix/511-babysit-self-identity-decouple branch July 21, 2026 19:29
kyle-sexton added a commit that referenced this pull request Jul 21, 2026
#882 (source-control) merged into main at 0.16.0, which this branch's prior
0.16.1 was already one past — resolved the resulting plugin.json/CHANGELOG.md
conflict, kept as-is.

Full fresh collision sweep after the merge found source-control now carries
TWO additional open PRs beyond #840: #895 (fix/548-babysit-worktree-head-safety,
claims 0.15.10) and #898 (feat/399-shared-worktree-helper, claims 0.17.0 — a
minor bump above this branch's prior 0.16.1). Re-bumped source-control to
0.17.1 to stay ahead of all three (#898's 0.17.0, #895's and #840's 0.15.10).

claude-ops (0.17.5, no collision), work-items (0.20.1, held behind #861's
0.20.0), repo-hygiene (0.4.6, no collision), and guardrails (0.9.6, no
collision) re-verified against current main and all live open PRs — unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
kyle-sexton added a commit that referenced this pull request Jul 21, 2026
Integrate the latest main (source-control advanced to 0.16.0 via #511's
pr_queue_snapshot self-identity work, plus #882). Re-derived this PR's bump to
0.16.1 (one past current main) and re-labeled the CHANGELOG entry from 0.15.10 to
0.16.1, stacked above the merged-in 0.16.0 entry. safety.md, orchestration.md,
review-discipline.md, and SKILL.md auto-merged (my bin/-path edits and the
upstream changes touch distinct regions); babysit-prs skill-quality gate green at
499/500 lines.
kyle-sexton added a commit that referenced this pull request Jul 22, 2026
…utate; merge-only, upstream refspec push (#895)

## Summary

Fixes #548. A babysit PR worker can be assigned a git worktree in
**detached HEAD** — because the PR branch is already checked out in a
sibling/foreign worktree — or on a **stale local branch tip** behind the
PR head. The checkout/freshness mechanics then merged and pushed from
that tip, so a stale-tip integration could **silently revert the newest
branch commit** — a near-miss where safety depended on the assigned
`HEAD` happening to match, not on a guard.

The initial reviewer pass confirmed the safety.md contract text was
sound but raised a **CRITICAL**: the actual worker mechanics
(loop.md/SKILL.md/orchestration.md) were unreconciled, so an agent
following the literal steps still hit the failure. This PR fixes the
contract **and** the mechanics that implement it.

## The fix

- **`reference/safety.md` Checkout And Push Invariants** — assert the
assigned worktree's `HEAD` equals the **true PR head** (`gh pr view <N>
--json headRefOid`; equal to `origin/<headRefName>` for a same-repo PR)
before any merge/edit/push; **stop** on a stale/detached mismatch. Push
by explicit refspec to the branch's configured upstream `git push "$(git
config --get branch.<headRefName>.remote)" HEAD:<headRefName>`
(fast-forward by construction, never `--force`/`--force-with-lease`).
The reuse rule is reconciled so it permits the detached-HEAD path under
the same assertion (no self-contradiction).
- **`reference/loop.md` §5.1.2** — acquire the head via `gh pr checkout`
(heals a behind-origin local branch; `--detach` for a sibling-locked
branch; resolves fork PRs), assert `HEAD == PR_HEAD` (the live
`headRefOid`) in every checkout path, degrade to read-only on mismatch.
`SKILL.md` Step 0.2 + the cross-tier invariants and
`reference/orchestration.md`'s conflict-worker follow the same assertion
+ refspec push. `worktrees.md` gets a one-line pointer (not a
duplicate).
- **Freshness is now merge-only.** The prior loop.md path
rebased-and-`--force-with-lease`d linear-history branches — which both
violated the skill's own never-force-push invariant (`safety.md` "Never
Do Automatically", `orchestration.md`) and was the silent-revert vector.
Behind-default branches now always integrate via `git merge` + a
fast-forward refspec push. **Behavior change:** linear-history branches
now carry an interim merge commit during freshness instead of being
rebased (the final squash merge still flattens history).

## Scope decisions (called out for review)

- **Fork/cross-repo heads (Option B — upstream-aware push).** The push
targets the branch's *configured upstream* — `git push "$(git config
--get branch.<headRefName>.remote)" HEAD:<headRefName>` — which resolves
to `origin` for a same-repo head and to the fork's remote for a
**write-allowed in-owner fork** head (a real supported case:
`branch_write_allowed=true` for cross-repo heads under
`<watched-owners>`). An earlier revision hardcoded `origin`, which the
re-review flagged as a regression that would silently write a same-named
branch on the base repo for a fork PR; this fixes it while keeping the
origin refspec for same-repo heads. An external-fork head *outside*
watched owners remains safety.md's read-only stop-and-ask case. The
assertion uses the live `headRefOid`, correct for any PR type.
- **Enforcement stays agent discipline.** Whether the head assertion
belongs in a deterministic push-safety guard (rather than prose the
worker follows) is filed as follow-up **#885**.

## Verification (local gates)

- `check-skill-portability.sh origin/main` — PASS (5 skill files).
- `check-changelog-parity.sh --check-bump origin/main` — PASS (0.15.10
entry present).
- `markdownlint-cli2` on the changed docs — clean; SKILL.md 499/500
lines; `evals.json` (id 6 updated to the merge-only + upstream-refspec
contract) valid; `plugin.json` valid.
- Python unittest suite unaffected (no code changed) — 339 tests OK.
- Two independent fresh-context review rounds: round 1 raised a CRITICAL
(the mechanics docs were unreconciled with the safety.md contract) —
fixed by reconciling loop.md/SKILL.md/orchestration.md; round 2 raised a
CRITICAL (hardcoded-origin push broke in-owner forks) — fixed by Option
B (upstream-aware push).

## Version

Per-plugin bump **0.15.9 → 0.15.10** (patch). `marketplace.json` pins no
version for `source-control`, so only `plugin.json` changes.

> **Ordering note for the merge lane:** my open PR #882 (#511) bumps
`source-control` to **0.16.0** and is unmerged. If #882 merges first,
re-bump this **above 0.16.0 (→ 0.16.1)** during recompose — do not keep
0.15.10.

Closes #548

## Related

- #885 — follow-up (filed): promote the head assertion from prose to a
deterministic push-safety guard.
- #524 — worker-lease cross-session exclusivity (distinct: *which
session* may work a PR; this is the *git mechanics* of the assigned
worktree).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5 <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-prs: decouple self-identity from --author so configured self-login extras survive autopilot widening

1 participant