Skip to content

feat(source-control): evidence-gated independent-resolver mode for thread resolution - #1782

Merged
kyle-sexton merged 8 commits into
mainfrom
feat/1632-independent-resolver
Jul 30, 2026
Merged

feat(source-control): evidence-gated independent-resolver mode for thread resolution#1782
kyle-sexton merged 8 commits into
mainfrom
feat/1632-independent-resolver

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

The gap

--autonomous resolves only threads GitHub marks isOutdated — the one deterministic "addressed"
signal it has. That guard is right about the actor it constrains: a self-resolved thread would
otherwise satisfy the merge gate's zero-unresolved-threads predicate, the actor signing its own
permission slip.

But isOutdated means the referenced code moved. On a prose or documentation PR a finding is
normally fixed by rewriting elsewhere in the file, so the anchor never moves, the finding is
genuinely addressed, and the guard refuses. Across the two runs #1632 measured, 7 of 20 resolved
threads were still not isOutdated — and that undercounts, since a worker's own push flips the flag
without touching a comment. The practical consequence was that an autonomous prose lane had no
legitimate route to zero unresolved threads: resolve outside the guard, stay blocked forever, or
escalate every current thread to a human. All 20 of those resolves took the first route, via raw
gh api graphql. This closes that.

The design

--independent-resolver is a third mode, parallel to --autonomous and never a relaxation of
it
. Widening --autonomous would delete the anti-self-certification property for exactly the
actor it was written to constrain, so the two properties that replace isOutdated are different
ones:

  • Independence — the mode is dispatched to a fresh context that is neither the merging worker
    nor the author of the fix. The actor resolving is not the actor whose permission slip it is. This
    is a property of the dispatch and no script can verify it, which is precisely why the other
    half is machine-checked.
  • Validated disposition evidence--disposition names the claim and carries exactly its own
    evidence flag, and the script validates it against the world, never against the assertion:
    • fixed + --fix-commit <sha> — must be reachable from the PR's current head commit, not
      merely exist. Resolved through the head repository, so a fork PR compares correctly. A SHA on
      some other branch is not evidence that this PR carries the fix.
    • deferred + --tracker-item <owner/repo#N|#N|N> — must exist and still be open. A closed
      follow-up is not a deferral; it is the finding disappearing.
    • incorrect + --counter-evidence <text> — must already appear in a reply on the thread.
      The opening comment is excluded, so the bot's own finding text can never satisfy the claim that
      the finding is wrong, and the rebuttal is visible where the finding is rather than only on the
      command line of the process resolving it.

Fail-closed throughout. Missing, unparsable, mismatched, or surplus evidence is a usage error
at exit 2 before any lookup. Evidence the world rejects refuses the resolve with its own
per-thread action: refused-fix-commit-not-on-head, refused-tracker-item-not-found,
refused-tracker-item-not-open, refused-counter-evidence-not-found, and
refused-evidence-unverifiable — the last kept deliberately distinct, because reporting an API
outage as a false claim would send a caller to fix the wrong thing. Refusing leaves the thread
unresolved, which is the recoverable direction; a suppressed finding is not.

Two judgment calls worth flagging for review:

  • Evidence is validated in list mode too. A dry run of an evidence-gated mode that skipped
    validation would report would-resolve for evidence the world rejects — the one answer this mode
    exists to prevent. Cost is one or two gh calls on a single pinned thread.
  • Bulk is refused in every mode here, list included — stricter than --autonomous, which
    refuses bulk only under --resolve. Evidence is a claim about ONE finding, so a bulk call would
    apply one thread's evidence to every thread.

Everything else --autonomous guards is retained deliberately: bot-only authorship, a single pinned
--thread-id with both TOCTOU pins, and the security/P1 bright line — this is still an unattended
path, so "never a security or P1 thread" stays unconditional and no evidence buys past it.
--autonomous, --include-human, and --allow-unpinned-thread are each refused alongside the mode.

Acceptance criteria

Criterion Status
New mode implemented; existing modes unchanged Done — ExistingModesUnchanged asserts --autonomous still refuses a non-outdated thread; resolve.autonomous-bulk-refused still passes
Human threads refused in the new mode, with a test Done — test_human_thread_is_still_refused plus predicate classify.independent-keeps-the-human-line
Passing + failing evidence test per disposition Done — 3 passing, 5 failing (not-on-head, unverifiable, not-found, not-open, counter-evidence-absent)
Evidence validated against the world, not the assertion Done — head-branch reachability, tracker state, on-thread reply; every validator consults gh
Receipt shape unchanged; refusals distinguishable Done — additive mode / disposition / refusedEvidence only; five distinct refused-* actions
scripts/engine.test.sh green Done — 535 tests, ruff clean, wrapper behaviour suite green
Wrapper needs no new refusal rule Confirmed — it is "$@" passthrough, and resolve.wrapper-filters-nothing already pins that it filters nothing
Documented in SKILL.md "Guarded mutations" and reference/safety.md Done — see the line-budget note below

On the SKILL.md line budget (#1626)

1632's body warned that babysit-prs/SKILL.md sat at 499/500 and that documenting this mode would

need that wall addressed first. It no longer sits there: on current main the file is 469 lines,
so the hub edit fits without touching #1626. With this change it is 484/500. That is under the
hard cap and CI passes, but it does consume half the remaining headroom, so #1626's underlying
pressure is real and unaddressed — flagging it rather than letting it be discovered at the next
edit.

Verification

  • bash scripts/engine.test.sh — 535 tests OK, ruff check clean, guarded-wrapper behaviour suite
    all PASS (this is the suite feat(source-control): independent-resolver mode for babysit_resolve_thread.py (evidence-gated, non-outdated bot threads) #1632 names as the gate)
  • python tests/guard_contract.py --emitreference/guard-contract.md regenerated from the five
    new refusal rows and three new predicates; GeneratedDocIsCurrent passes against it
  • python -m unittest test_babysit_resolve_thread — 67 tests (was 41; 26 added)
  • Every other source-control contract test run individually: pr-body-linkage-gate.test.sh (135
    passed), pr-linkage-mcp-gate.test.sh, babysit-wrapper-help.test.sh,
    babysit-readiness-gate.test.sh — all PASS
  • Repo gates against origin/main: check-changelog-parity.sh --check, --check-bump,
    --check-order (0.41.0 → 0.42.0 with its ## [0.42.0] entry); check-changed-skills.sh
    (1 skill, 0 failed); check-skill-portability.sh; check-orphaned-fixtures.sh;
    check-contract-slice-prune.sh --check-diff; check-cross-plugin-source-drift.sh;
    check-silent-skips.sh; check-shell-portability.sh; validate-plugins.sh
  • markdownlint-cli2 "plugins/source-control/**/*.md" — 48 files, 0 errors

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C

kyle-sexton and others added 2 commits July 30, 2026 01:02
…read resolution

--autonomous admits only threads GitHub marks isOutdated. That is the right
guard for the merging worker -- a self-resolved thread would otherwise satisfy
the merge gate's zero-unresolved-threads predicate -- but isOutdated means the
referenced code MOVED. On a prose or documentation PR a finding is normally
addressed by rewriting elsewhere in the file, so the anchor never moves, the
finding is genuinely addressed, and the guard refuses. An autonomous prose lane
was left with no sanctioned route to zero unresolved threads.

--independent-resolver is a parallel third mode, never a relaxation. It replaces
isOutdated with caller independence -- a fresh context that is neither the
merging worker nor the author of the fix, so the actor resolving is not the
actor whose permission slip it is -- plus disposition evidence the script
validates against the world rather than trusting. Independence is a property of
the dispatch that no script can verify, which is why the evidence half is
machine-checked here.

--disposition carries exactly its own evidence flag: fixed + --fix-commit (must
be reachable from the PR's current head, resolved through the head repository so
a fork PR compares correctly), deferred + --tracker-item (must exist and still
be open), incorrect + --counter-evidence (must already appear in a reply on the
thread, the opener excluded so the bot's own finding cannot prove itself wrong).

Fail-closed throughout. Missing, unparseable, mismatched, or surplus evidence is
a usage error before any lookup; evidence the world rejects refuses with its own
per-thread action, with refused-evidence-unverifiable kept distinct so an API
outage is never reported as a false claim. Evidence is validated in list mode
too, so a dry run proves the evidence instead of predicting the resolve.

Every other guard is retained: bot-only authorship, both TOCTOU pins, and the
security/P1 bright line, because an independent resolver is still an unattended
path. --autonomous, --include-human, and --allow-unpinned-thread are each
refused alongside it, and bulk is refused in every mode here since evidence is a
claim about one finding. The bin wrapper needed no change; its contract row
already pins that it filters nothing.

Refs #1632

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
The typos gate's dictionary rewrites "unparseable"; every other occurrence in
this repository already uses the corrected form.

Refs #1632

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

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: feat(source-control): evidence-gated independent-resolver mode

  • Gather context and read PR diff
  • Review babysit_resolve_thread.py — core implementation
  • Review test_babysit_resolve_thread.py — test coverage
  • Review guard_contract.py — contract validation
  • Review docs: SKILL.md, safety.md, guard-contract.md
  • Review plugin.json and CHANGELOG.md
  • Synthesize findings and post review

Summary

This is a carefully engineered change. The design rationale (independence as a dispatch property + machine-checked evidence) is sound, the fail-closed philosophy is applied consistently, and the guard surface is well-documented. The implementation matches the stated design. I have one correctness concern, one inconsistency worth noting, and a handful of minor observations.


Correctness: One Real Issue

verify_tracker_item conflates API failures with "item not found" (line 509)

proc = gh_capture(["api", f"repos/{repo}/issues/{match.group('number')}"])
if proc.returncode != 0:
    return False, "refused-tracker-item-not-found"

Any non-zero exit from gh api — including HTTP 403 (forbidden) and HTTP 500 (server error) — is reported as refused-tracker-item-not-found. The action naming implies "the issue doesn't exist," but a GitHub outage or permission error should produce refused-evidence-unverifiable instead. Directing a caller to fix a reference that doesn't exist when the API is actually unavailable is exactly what refused-evidence-unverifiable was defined to prevent.

Compare with verify_fix_commit: the gh pr view call (line 459) correctly returns refused-evidence-unverifiable when it fails. The compare call (line 481) has the same conflation issue (404 and 5xx both return refused-fix-commit-not-on-head) but the inline comment explains this is intentional there ("a SHA absent from the head repository 404s here"). For issues that's less compelling — a 403 on an issues endpoint is a real permission distinction, not a lookup miss.

The fix is to parse the gh exit code or stderr to distinguish 4xx from other errors, or at minimum treat any non-404 as refused-evidence-unverifiable. There's also no test covering verify_tracker_item when gh api returns a non-404 failure code.

Fix this →


Inconsistency: Stale Pins Are Not Checked in List Mode

stale_pin at line 979 is gated on args.resolve:

stale_pin = (
    args.resolve and args.thread_id and (count_stale or last_updated_stale)
)

This means a list-mode dry run with a stale pin + valid evidence reports would-resolve, while a --resolve invocation with the same inputs reports refused-stale-pin. The dry run prediction is wrong. This is pre-existing for --autonomous too, so it's consistent with existing behavior — but it's a known limitation the independent mode inherits. Worth a note in SKILL.md or a TODO near the stale-pin calculation, since the PR description explicitly calls out that "evidence is validated in list mode too" to make dry runs trustworthy.


Security

  • Input validation before any API call is thorough. FIX_COMMIT_RE (hex only), TRACKER_ITEM_RE (alphanumeric owner/repo), and the empty-needle guard in verify_counter_evidence all prevent injection.
  • All gh calls use list arguments (no shell interpolation).
  • The --autonomous + --independent-resolver combination is refused before any network call. The --include-human and --allow-unpinned-thread refusals close the obvious widening combinations.
  • verify_counter_evidence correctly uses casefold() (Unicode-aware case folding) and rejects empty strings (an empty needle would match every body — correctly refused, not silently treated as evidence).
  • The replyBodies list correctly excludes the opening comment (line 332), preventing the bot's own finding text from satisfying incorrect evidence.

Correctness: verify_fix_commit Logic Is Sound

The reachability check at lines 474-494 calls compare/{sha}...{head_oid} (base=sha, head=head_oid), then accepts only when status in ("identical", "ahead") and behind_by == 0. With base=fix-SHA and head=PR-head:

  • ahead → PR head has commits sha doesn't → sha is in the history of PR head ✓
  • identical → sha == PR head ✓
  • behind_by == 0 is a redundant belt-and-suspenders that prevents a contradictory status/behind_by pair from slipping through ✓

Cross-fork correctness: the check resolves headRepository + headRepositoryOwner from the PR rather than using repo (lines 464-470), so a fork PR correctly compares against the fork's history, not the base repo's.


Test Coverage

The 26 new tests are comprehensive for the main paths. A few gaps:

  1. No test for verify_tracker_item with a non-404 failure (the correctness issue above — currently all non-zero exit codes return the same action).
  2. No test for verify_fix_commit with status == "identical" (only "ahead" is exercised in test_fixed_resolves_when_the_sha_is_reachable_from_head).
  3. No test for the behind_by != 0 / status == "ahead" combination (belt-and-suspenders check at line 492).
  4. No test for list-mode dry run with a stale pin — as noted above, this currently predicts would-resolve when --resolve would refuse.

These are low-severity gaps; the main acceptance criteria are well-covered and the guard contract tests provide a second execution layer.


Minor Observations

  • classify function signature (line 383): the new independent parameter defaults to False, which is correct and backward-compatible.
  • refusedEvidence counter correctly excludes refused-stale-pin (line 1064). The distinction between evidence refusals and TOCTOU refusals is preserved in the output.
  • disposition field in output: appears in both the success path (line 998) and the refusal path (line 1009), which is the right design — callers can always see what was claimed and whether it held.
  • verify_disposition dispatch (lines 542-557): the else branch implicitly handles "incorrect" — if a new disposition were added to DISPOSITION_EVIDENCE without a matching elif, it would silently dispatch to verify_counter_evidence. Not an immediate issue since all three dispositions are accounted for, but a match or explicit else-if chain would be defensive.
  • SKILL.md line budget: noted in the PR (484/500). The safety.md additions are precise and non-redundant with the existing unattended-path rules.

Verdict

The design is correct and the implementation matches it. The one real issue is the verify_tracker_item error-code conflation, which can cause misleading caller signals when the GitHub API is temporarily unavailable during a deferred evidence check. Everything else is solid. The guard contract and doc updates are well-aligned with the code changes.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1782

  • Load tools
  • Read PR diff
  • Read changed files (babysit_resolve_thread.py, babysit_gh.py, tests)
  • Analyze for security issues
  • Post findings

No CRITICAL issues found. Two findings below.


IMPORTANT · PLAUSIBLE — verify_fix_commit uses unvalidated API-returned fields in URL construction

File: babysit_resolve_thread.py lines 464–476

head_oid   = dig(payload, "headRefOid")
head_name  = dig(payload, "headRepository", "name")
head_owner = dig(payload, "headRepositoryOwner", "login")
if not (isinstance(head_oid, str) and isinstance(head_name, str) and isinstance(head_owner, str)):
    return False, "refused-evidence-unverifiable"

compare = gh_capture(
    ["api", f"repos/{head_owner}/{head_name}/compare/{sha}...{head_oid}"]
)

head_owner, head_name, and head_oid come from the GitHub API response and are checked only to be strings. They are interpolated directly into the gh api URL path. Contrast with two places in the existing codebase that handle the same data defensively:

  • parse_repo_number in babysit_gh.py validates owner/repo against GITHUB_OWNER_RE / GITHUB_REPOSITORY_RE before any use.
  • fetch_blocked_base_compare in babysit_gh.py:404 validates head_sha against r"[0-9a-fA-F]{7,40}" before constructing an identical compare/... URL — precisely the same call shape as verify_fix_commit.

This is not shell injection (list args, no shell). The call boundary is safe. But a crafted API response — possible if the automation's token is compromised, or if a future code path fetches the PR data from a less-trusted source — could redirect the compare call to an unintended GitHub API path. The existing inconsistency with fetch_blocked_base_compare (which does validate) is the most concrete signal: there was already a reason to add that guard once, and the new function omits it.

The behavior remains fail-closed in all cases: an unexpected response or 404 resolves to a refused-* action, never a resolve. The risk is SSRF-class misdirection within GitHub's API, bounded by the automation token's scope.

Suggested fix: validate head_owner and head_name against GITHUB_OWNER_RE/GITHUB_REPOSITORY_RE, and head_oid against re.fullmatch(r"[0-9a-fA-F]{7,40}", head_oid), before the compare call — matching the pattern fetch_blocked_base_compare already uses for the same family of data. Fix this →


SUGGESTION · CONFIRMED — Opener-bot's own classification replies can satisfy the incorrect counter-evidence check

File: babysit_resolve_thread.py lines 332–336 (projection) and lines 523–539 (validator)

replyBodies is built from comments[1:] — all comments after the opener, with no authorship filter:

"replyBodies": [
    body
    for c in comments[1:]
    if isinstance(body := (c.get("body") if is_json_object(c) else None), str)
],

The documented workflow (reference/review-discipline.md) requires the finding bot to post a classification reply on every thread it evaluates. That classification reply restates the finding, its severity, and its status — and is included verbatim in replyBodies even when the reply is self-authored (opener bot == reply bot). verify_counter_evidence then does a case-insensitive substring search across all of replyBodies:

if any(isinstance(body, str) and needle in body.casefold() for body in replies):
    return True, ""

Concrete scenario: Finding-Bot opens a thread: "The anchor points at generated output." Finding-Bot's mandatory classification reply says: "Status: incorrect — the anchor points at generated output." Independent-Resolver-Bot calls --disposition incorrect --counter-evidence "anchor points at generated output". The substring matches Finding-Bot's own reply, satisfying the counter-evidence check. The same entity that made the finding is the source of the evidence that the finding is wrong.

The PR description correctly notes the opener is excluded ("The opening comment is excluded, so the bot's own finding text can never satisfy the claim that the finding is wrong"), but the reasoning does not extend to subsequent replies by the same bot. Compare with _severity_scan_body, which explicitly applies strip_classification_rows to self-login replies precisely because classification replies restate severity markers — but replyBodies applies no equivalent stripping and no authorship filtering.

The severity and human-thread bright lines remain intact; this cannot bypass a P1/security thread. The practical risk is an incorrect resolution on a still-valid finding when the opener bot's own language matches the asserted counter-evidence text. The trust property that makes incorrect meaningful — an independent party posted the rebuttal where the finding is — is not guaranteed.

Suggested fix: Filter replyBodies to exclude comments whose author login matches the thread opener's login (or, more narrowly, the configured self-logins), mirroring the authorship awareness already in place for _severity_scan_body. Alternatively, document explicitly that self-authored classification reply text is an acceptable source of counter-evidence, and add a test that exercises that path so the contract is visible.


No other security issues found. The guard layering (TOCTOU pins, bulk refusal, severity bright line, bot-only requirement, owner allowlist, argument ordering, fail-closed evidence refusals) is sound. All subprocess calls use list args with gh as the sole allowed executable. No shell expansion surface.

@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: ae3a16894c

ℹ️ 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_resolve_thread.py Outdated
kyle-sexton added a commit that referenced this pull request Jul 30, 2026
The rolling link-check report listed **8 errors**. Only **3 were dead
links** — the other 5 are live
URLs the checker cannot reach, and their fix lands upstream (see below).
Every replacement here was
verified against the live target, not inferred from the URL shape.

## The three dead links

**`docs/adr/0002-…md`** — GitHub retired the
`repositories/configuring-branches-and-merges…/managing-protected-branches/`
path for the required-status-checks troubleshooting page. It now lives
under
`pull-requests/how-tos/merge-and-close-pull-requests/`. Verified 200
with **no redirect**, H1 reads
"Troubleshooting required status checks", and it still covers the
check-never-reports case the ADR
cites it for ("Associated checks stay in a 'Pending' state and block
merging").

An independent fresh-context lookup caught that my first replacement
here was itself a 301 — the
`collaborating-with-pull-requests/…` path redirects to the `how-tos/`
one. Both resolve today, but a
redirect is a second thing that can be retired, and lychee already hints
to prefer resolved URLs, so
the second commit swaps in the canonical target. Verified both
directions: the old path returns 301
with that Location, the new one returns 200 with none.

**`plugins/dometrain/README.md`** — Dometrain moved its plans page from
`/pro/` to `/dometrain-pro/`.
Verified 200, `<title>Dometrain Plans - Dometrain</title>`. The link
text stays "Dometrain Pro"
because the slug and the product name both still are.

**`plugins/source-control/skills/babysit-prs/reference/freshness.md`** —
the most interesting of the
three. `graphql/reference/enums` did not 404; it became a **navigation
index** and no longer carries
any enum definitions at all, which is why the failure was `Cannot find
fragment` rather than a dead
page. GitHub split the GraphQL reference by domain, so
`MergeStateStatus` now lives on the `pulls`
page. The replacement was verified structurally, not just by status
code: `id="enum-mergestatestatus"`
is present in the **served HTML** (so lychee's fragment check resolves
it, rather than the anchor
being JS-injected), and the page carries both descriptions this doc
quotes verbatim — "The head ref
is out of date" and "The merge is blocked".

## The other five are not content defects, and are fixed upstream

`lychee.toml` is a **`managed` component** for this repo per
`standards/distribution/sync-manifest.yml`,
so editing it here would be silently overwritten by the next sync. The
config half of this report is
therefore **melodic-software/standards#303**:

- **`www.gnu.org/software/coreutils/…` (429)** — verified 200. A 429 is
the server rate-limiting the
checker and lands on whichever host the shared runner IP is throttled
against that run, so the fix
is `accept`-ing 429 rather than excluding a healthy host that would just
be replaced by a
  different one next run.
- **`dl.acm.org` and `queue.acm.org` (403)** — 403 even with a full
browser User-Agent; no header
  tuning reaches them.
- **`docs.genius.com` (403)** — 200 with a browser User-Agent; the
documented bot-block case.
- **`www.ntia.gov` (SSL not trusted)** — the chain verifies locally
(`openssl s_client` →
`Verify return code: 0 (ok)`, curl 200 under strict verification). A
trust store failing an ECC
  chain, not an untrustworthy host.

This PR merging alone will not clear the report; #303 has to land and
sync. Flagging that plainly
rather than letting a half-clear look like a regression.

## Verification

Run with the real `lychee.toml` plus the proposed upstream config, over
all seven files the report
named:

```text
🔍 118 Total  🔗 116 Unique  ✅ 114 OK  🚫 0 Errors  👻 4 Excluded
```

All 8 reported errors resolved. Also run against this repo's gates:

- `scripts/check-changed-skills.sh origin/main` — 1 skill checked, 0
failed
- `scripts/check-contract-slice-prune.sh --check-diff origin/main` —
pass (no `docs/topics/` path is
touched; that file's two ACM URLs are handled by exclusion, not by
editing it)
- `scripts/check-changelog-parity.sh --check-bump origin/main` — pass.
**No plugin version bump**: a
corrected external URL in a reference doc changes no behavior contract,
no gate requires one, and
bumping `source-control` would collide with the in-flight bump on #1782
— the collision class
  tracked as #1746.
- `markdownlint-cli2`, `typos` — clean

## Related

- Fixes #640
- melodic-software/standards#303 — the upstream half; owns `lychee.toml`
for this repo
- #1746 — the concurrent version-bump collision class, the reason this
change deliberately bumps
  nothing

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

<https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…the thread

Four review findings on the independent-resolver mode, all fail-closed.

A multi-finding thread is now refused (`skipped-multi-finding-thread`). One
`--disposition` is a claim about ONE finding while `resolveReviewThread` clears
the whole thread, dropping every comment it carries out of the readiness
denominator — so evidence for finding A suppressed an unaddressed finding B and
let the merge gate pass over it. The count comes from the shared severity
vocabulary (`babysit_classify.severity_occurrences`, now public so the resolver
and the readiness counters cannot drift) over the thread's own comments, with a
self classification reply's table rows stripped, and an unknown count refuses.
Scoped to this mode: `--autonomous` rests on the thread-level `isOutdated`, so
it carries no per-finding claim to under-cover.

Only a confirmed HTTP 404 now earns an evidence-specific refusal. Every other
`gh` failure — 403, 429, 5xx, timeout, no HTTP response — reports
`refused-evidence-unverifiable`, so an outage no longer sends a caller off to
replace evidence that may be perfectly valid.

`--counter-evidence` excludes every reply under the thread OPENER's login, not
just the opening comment: the mandated classification reply restates the
finding's own text, so a finding bot replying on its own thread supplied the
words asserted as the rebuttal. A different bot's reply and the caller's own
`--self-logins` reply stay admissible.

The compare URL's path segments are format-validated before interpolation,
matching `babysit_gh.fetch_blocked_base_compare` for the identical call shape.
Two of the three arrive in an API response body, so "the API said so" was their
only provenance.

Also: a stale `--thread-id` pin is reported in list mode, so a dry run predicts
what `--resolve` would do; and `verify_disposition`'s fallthrough refuses rather
than reaching whichever validator happened to be last.

Six classifier predicates added to the guard contract; 37 tests added, covering
the reachability and tracker-item gaps the review named.
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


PR Review Update — feat(source-control): evidence-gated independent-resolver mode (commit 4556fe4)

  • Check latest commit diff vs previous review state
  • Review the multi-finding gate fix (4556fe4)
  • Verify all prior findings were addressed
  • Post updated review

The new commit 4556fe4 directly addresses all six issues raised across the two prior reviews (Claude review from ae3a168, the security review, and the Codex P1/P2 findings). Each fix is implemented consistently with the fail-closed philosophy of the original PR.


What Was Fixed

Codex P1 — Multi-finding thread refusal

The new _is_single_finding guard at line 438 computes a findingCount via severity_occurrences (now public in babysit_classify) over the same stripped bodies that the readiness counters read — shared vocabulary so the resolver and the counters can't diverge. A count of None (truncated page) fails closed, since a hidden comment could carry another finding. --autonomous is correctly not narrowed: its signal is thread-level isOutdated, not per-finding.

Claude review + Codex P2 + Security review — API failure disambiguation

gh_http_status at line 521 parses (HTTP NNN) from gh's stderr (which gh_capture already captures via capture_output=True). Only a confirmed 404 produces the evidence-specific refusal; every other failure — 403, 5xx, rate limit, timeout, no HTTP response — produces refused-evidence-unverifiable. Applied consistently to both verify_fix_commit and verify_tracker_item. The comment notes "verified against gh 2.95.0"; importantly, the fallback when the regex doesn't match is None, which the callers treat as refused-evidence-unverifiable — fail-closed even if gh ever changes its error format.

Security review — Opener login exclusion for counter-evidence

_reply_body at line 355 now excludes every reply whose author login (case-folded) matches the thread opener's. A different bot's reply and the caller's own self-login reply remain admissible. The edge case when opener_key is None is handled fail-closed: no replies are admitted at all (documented in the inline comment), and such a thread would already fail botOnly.

Security review — URL path segment validation

The format guard at line 576 validates head_owner against GITHUB_OWNER_RE, head_name against GITHUB_REPOSITORY_RE, adds an explicit head_name not in {".", ".."} guard (needed because the regex allows single/double dots), and validates both head_oid and sha against FIX_COMMIT_RE. This matches the pattern fetch_blocked_base_compare already uses. All three fields arrive from an API response body, so format validation is the only provenance check available.

Claude review — Stale pin in list mode

stale_pin = count_stale or last_updated_stale at line 1127 removes the args.resolve and args.thread_id guards. The invariant holds: count_stale and last_updated_stale can only be true when args.expected_comment_count / args.expected_last_updated are set, and argument validation at lines 878 and 893 requires --thread-id for those flags. Dry runs now predict what --resolve would do.

Claude review — verify_disposition explicit fallthrough

The new explicit if disposition == "incorrect" branch plus return False, "refused-evidence-unverifiable" at line 704 closes the silent-dispatch gap: a future disposition added to DISPOSITION_EVIDENCE without a matching validator here now refuses rather than accidentally delegating to verify_counter_evidence.


Test Coverage (37 new tests)

The gaps named in both prior reviews are now covered:

  • test_tracker_item_403_is_unverifiable_not_missing, test_tracker_item_500_is_unverifiable_not_missing, test_tracker_item_with_no_http_response_is_unverifiable — the conflation issue in verify_tracker_item
  • test_compare_500_is_unverifiable_not_a_rejected_sha, test_compare_rate_limit_is_unverifiable_not_a_rejected_sha — the same issue in verify_fix_commit
  • test_identical_status_is_reachable — previously untested "identical" branch
  • test_ahead_with_nonzero_behind_by_is_refused — the belt-and-suspenders behind_by == 0 check
  • test_list_mode_reports_a_stale_pin_instead_of_would_resolve, test_list_mode_reports_a_stale_comment_count — the dry-run prediction gap
  • test_opener_own_later_reply_is_not_admissible_evidence, test_end_to_end_opener_reply_cannot_satisfy_the_claim — the classification-reply self-sourcing path
  • test_two_findings_refuse_under_the_independent_mode, test_an_unknown_count_fails_closed — the multi-finding gate

The test_last_reported_status_wins_over_an_earlier_retry and test_no_status_at_all_reports_none tests verify gh_http_status directly including the retry-output edge case.


One Residual Minor Observation

gh_http_status locale sensitivity: the regex r"\(HTTP (\d{3})\)" parses a gh internal format string. If gh ever localizes that string (the (HTTP NNN) part), the regex would return None and every non-zero exit would fall through to refused-evidence-unverifiable. That's the correct fail-closed behavior, so the system degrades gracefully — but a comment noting this intentional choice would make future maintenance clearer. No code change needed.


Verdict

All issues from the prior reviews are addressed. The implementation matches the stated design, and the new tests close the named gaps. The changes are incremental, fail-closed throughout, and do not widen any of the existing mode guards.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1782

  • Load tools
  • Read PR diff and changed files
  • Analyze authentication / trust boundaries
  • Analyze input validation and injection surfaces
  • Synthesize and post findings

Scope note: The four findings the prior review runs raised (tracker-item error conflation, verify_fix_commit URL validation, opener-bot self-reply counter-evidence, multi-finding thread bypass) were all addressed in the sync commit (4556fe4). This review covers the PR as currently landed.


No CRITICAL issues found.


SUGGESTION · CONFIRMED — verify_tracker_item skips format-validation of the caller-supplied owner/repo before URL construction

File: babysit_resolve_thread.py lines 633–637

match = TRACKER_ITEM_RE.match(token.strip())
if not match:
    return False, "refused-tracker-item-not-found"
repo = match.group("repo") or default_repo
proc = gh_capture(["api", f"repos/{repo}/issues/{match.group('number')}"])

TRACKER_ITEM_RE's owner portion is [A-Za-z0-9._-]+, which allows a leading . (e.g. .., .org). GITHUB_OWNER_RE used elsewhere in the codebase is ^[A-Za-z0-9][A-Za-z0-9-]*$ — requires the owner to start with an alphanumeric character and prohibits dots entirely.

Compare with verify_fix_commit, which validates API-returned owner and name against GITHUB_OWNER_RE/GITHUB_REPOSITORY_RE and explicitly guards head_name not in {".", ".."} before the analogous compare/ URL:

# verify_fix_commit — lines 587–594
if not (
    GITHUB_OWNER_RE.fullmatch(head_owner)
    and GITHUB_REPOSITORY_RE.fullmatch(head_name)
    and head_name not in {".", ".."}
    and FIX_COMMIT_RE.match(head_oid)
    and FIX_COMMIT_RE.match(sha)
):
    return False, "refused-evidence-unverifiable"

verify_tracker_item applies no equivalent guard to the caller-supplied owner/repo component. A --tracker-item value such as validowner/..#1 constructs repos/validowner/../issues/1, which an HTTP client normalizes to repos/issues/1 — not a valid GitHub API endpoint, so GitHub returns 404. With the current 404-detection logic that correctly maps to refused-tracker-item-not-found, the security outcome is the same: the resolve is refused.

The practical consequence is a misleading refusal reason, not a bypass. The resolution still fails closed. But the refused-tracker-item-not-found action would tell the caller to fix a reference that does not exist, when the actual problem is a malformed owner/repo in the tracker item. refused-evidence-unverifiable — the action already defined for "the world could not be consulted for reasons other than the item being absent" — is the more accurate signal for a path that wasn't a real GitHub API endpoint in the first place.

Suggested fix: validate the explicit owner/repo extracted by TRACKER_ITEM_RE against GITHUB_OWNER_RE/GITHUB_REPOSITORY_RE (and guard against "..") before constructing the URL, returning refused-evidence-unverifiable on mismatch — matching the pattern verify_fix_commit already applies for the identical call shape. The default_repo path is already validated by parse_repo_number and needs no additional check. Fix this →


Assessment of remaining surface

  • All subprocess calls use list args. No shell injection surface anywhere in the new code paths.
  • gh_http_status relying on gh's stderr format ((HTTP NNN)) is a fragility — if gh changes its format, gh_http_status returns None, and callers fall through to refused-evidence-unverifiable. That is the safe direction. Not a security issue.
  • verify_counter_evidence has no minimum needle length. A single character satisfies the check. The --counter-evidence argument is supplied by the trusted independent resolver; the empty-string guard is correct. Short strings are a quality concern for the resolver agent's judgment, not a vulnerability: the security property (rebuttal is visible on the thread where the finding is) holds regardless of length.
  • _reply_body opener exclusion (lines 360–367) now correctly filters all comments under the opener's login from replyBodies, not just the opener comment itself. The prior finding (classification reply from the opener bot satisfying incorrect counter-evidence) is closed by the sync commit.
  • stale_pin is now evaluated in list mode (line 1132, no args.resolve gate), making dry-run predictions accurate. Consistent with the stated design.
  • parse_repo_number casefolds the owner, so the owner not in allowed comparison at line 1058 is side-matched with parse_allowed_owners's own casefold — no case-sensitivity bypass in the allowlist gate.
  • TOCTOU pins, bulk refusal, severity bright line, bot-only requirement are all retained for --independent-resolver and verified by the new guard-contract predicates.

No other security issues found.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Per-finding classification for the security review. Both findings were valid and both are fixed in 4556fe4f.

# Finding Classification Evidence Reacted
1 verify_fix_commit interpolates API-returned head_owner / head_name / head_oid into the compare URL checked only for isinstance(str), while fetch_blocked_base_compare validates the same data for the identical call shape VALID — fixed Confirmed: the pre-fix code went straight from the three isinstance checks to the f-string, and babysit_gh.py:404-410 does validate head_sha before an identical compare/ URL 👍
2 replyBodies is comments[1:] with no authorship filter, so the opener bot's own classification reply can satisfy an incorrect counter-evidence claim VALID — fixed Confirmed, and your scenario reproduces exactly: excluding only comments[0] left every later comment by the same bot admissible 👍
3 "No CRITICAL issues found" INCORRECT — not a finding The reviewer stating the absence of high-severity issues; no work item, recorded here because the readiness gate's vocabulary counts the marker occurrence 👍

Finding 1. Every segment is format-validated before interpolation now, using the same regexes you pointed at: head_owner against GITHUB_OWNER_RE, head_name against GITHUB_REPOSITORY_RE plus an explicit .. rejection (GITHUB_REPOSITORY_RE admits dots, and parse_repo guards that separately for the same reason), and head_oid against the commit-SHA pattern. Any failure returns refused-evidence-unverifiable.

I also validated sha itself at the same point rather than resting on main's --fix-commit check. verify_fix_commit is a public function with its own unit tests, so its contract should hold for every caller, not only the one that happens to pre-validate.

Your framing is the accurate one and worth restating so the record is not overstated: the call boundary was already safe (list args, no shell), and an unexpected response always refused, so this narrows the reachable surface rather than fixing an exploitable resolve. The concrete argument — that the guard existed once already in fetch_blocked_base_compare and the new function omitted it — is what made it worth closing rather than deferring.

Tests: CompareUrlFieldsAreValidated, 5 cases covering an owner carrying path syntax, a repository name carrying path syntax, a .. name, a non-hex head_oid, and an unparsable sha at the function boundary. Each scripts a single gh response, so reaching the compare call would raise — that is what proves the refusal comes first rather than merely that it happens.

Finding 2. Fixed by the first of your two options, filtering on the thread opener's login rather than the configured self-logins. Taking the narrower reading would have broken the mode outright: the worker's own reply under a --self-logins identity is the intended evidence source under the documented D5 flow, so excluding it would leave no admissible rebuttal anywhere. Opener-login exclusion is the trust property you articulated — an independent party posted the rebuttal — without removing the independent parties.

So the rule is now: the opening comment is excluded, and so is every later reply under the opener's login (casefolded). A different bot's reply stays admissible, and the caller's own reply stays admissible. An opener whose login the API withheld admits nothing at all, since independence cannot be established against an unknown author — such a thread is already not botOnly, so this is belt-and-braces.

Tests: CounterEvidenceExcludesTheFindingsAuthor, 6 cases — the opener's own later reply is dropped, a different bot's reply is kept, the caller's own reply is kept, the login match is case-insensitive, a withheld opener login admits nothing, and an end-to-end run refusing the claim.

On your other observations. The four you confirmed as correct — the independent parameter defaulting to False, refusedEvidence excluding refused-stale-pin, disposition appearing on both the success and refusal paths, and the reachability logic including the behind_by == 0 belt-and-suspenders — all still hold; nothing in this commit changes them. verify_disposition's implicit else is now an explicit incorrect branch with a refusing fallthrough, which was raised separately in the code review.

Verification: 572 tests pass (baseline before this commit was 535). markdownlint-cli2, typos, and editorconfig-checker clean on every changed file; check-changelog-parity.sh --check-bump origin/main and check-changed-skills.sh origin/main both PASS.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Per-finding classification for the code review. Seven work items, all valid, all fixed in 4556fe4f.

# Finding Classification Evidence Reacted
1 verify_tracker_item reports any non-zero gh api exit as refused-tracker-item-not-found, so a 403 or 500 reads as "the issue does not exist" VALID — fixed Probed the live CLI: gh api exits 1 for every HTTP failure, so returncode != 0 genuinely could not distinguish 404 from 403/5xx 👍
2 The compare call has the same conflation (404 and 5xx both yield refused-fix-commit-not-on-head) VALID — fixed Same probe; the inline comment claiming it was intentional was resting on the 404 case alone 👍
3 stale_pin is gated on args.resolve, so a list-mode dry run with a stale pin predicts would-resolve while --resolve refuses VALID — fixed Reproduced from the code: stale_pin = args.resolve and ... made the prediction wrong in exactly the direction that matters 👍
4 No test for verify_tracker_item when gh returns a non-404 failure VALID — fixed Confirmed absent; the only failure fixture was a bare _proc(1) 👍
5 No test for verify_fix_commit with status == "identical" VALID — fixed Confirmed: only "ahead" was exercised 👍
6 No test for the status == "ahead" / behind_by != 0 combination VALID — fixed Confirmed: the belt-and-suspenders branch was unexercised 👍
7 verify_disposition's else implicitly handles "incorrect", so a new disposition would silently dispatch to verify_counter_evidence VALID — fixed Confirmed by reading the dispatch; a match or explicit chain was the right call 👍

Findings 1 and 2 — the error-code conflation. Fixed together, since they are one defect at two call sites. gh does not distinguish failures by exit code, so the only available signal is the status it writes to stderr. Verified empirically against gh 2.95.0 rather than assumed: a missing issue and a missing commit both produce exactly gh: Not Found (HTTP 404), exit 1. New gh_http_status parses (HTTP nnn) and takes the last match, so a retried request reports its final outcome.

Only a confirmed 404 now earns an evidence-specific refusal at either site; a 403, 429, 5xx, timeout, or a failure that never reached an HTTP response at all reports refused-evidence-unverifiable. You were right that the issues endpoint makes this sharper than the compare one — a 403 there is a real permission distinction — but the compare site had the same defect for 5xx, so both are fixed rather than only the more compelling one. Codex raised the compare half independently as its P2; the two threads share this fix.

Finding 3 — the stale-pin dry-run gap. Fixed rather than noted. You offered a note in SKILL.md or a TODO near the calculation, but the argument the PR already makes for validating evidence in list mode applies unchanged to the pin: a dry run exists to predict the resolve, and a prediction that is wrong when the pins have drifted is wrong in the one direction that matters. stale_pin is now count_stale or last_updated_stale with no mode gate. Both pin flags already require --thread-id (a usage error otherwise), so the pin always has a target whenever either comparison can be true.

This does change --autonomous's list-mode output too — the pre-existing behaviour you flagged. That is called out explicitly in the CHANGELOG so the change is not silent, and a regression test pins it.

Findings 4 through 6 — the test gaps. All three are covered, plus the fourth gap you named in the same list (list-mode dry run with a stale pin, which is finding 3's regression test):

  • OutageIsNotARejection — 10 cases: 403, 429, 500, and a no-HTTP-response stderr at both call sites, the 404 path at both, and that the last reported status wins over an earlier retry.
  • FixCommitReachability — 3 cases: status == "identical" accepted, "ahead" with behind_by == 2 refused, and a missing behind_by refused.
  • ListModePredictsTheResolve — 4 cases: list mode reports refused-stale-pin for a drifted timestamp and for a drifted comment count, --autonomous list mode reports it too, and matching pins still predict would-resolve.

Finding 7 — the implicit dispatch. verify_disposition now names all three dispositions explicitly and its fallthrough returns refused-evidence-unverifiable. Taking the refusing fallthrough rather than a match statement means a disposition added to DISPOSITION_EVIDENCE without a validator fails closed instead of being reported as validated by a check that never read its evidence. Test: DispositionDispatchIsExplicit.

On the stale-pin inconsistency you traced to --autonomous. Worth recording that your reading was right about scope: it was pre-existing rather than introduced here, which is why it could have been deferred. It was not deferred because the fix is three tokens and the mode's own stated design principle already demanded it.

Verification: 572 tests pass (baseline before this commit was 535, so 37 added). markdownlint-cli2, typos, and editorconfig-checker clean on every changed file. check-changelog-parity.sh --check-bump origin/main PASS; check-changed-skills.sh origin/main PASS (SKILL.md 487/500 lines, the one warning being the pre-existing soft 200-line target). guard-contract.md regenerated from its module and confirmed byte-identical on a second emit.

…e lookup

`TRACKER_ITEM_RE` admits an owner/repo SHAPE, not a valid one: its character
class allows a leading dot and a bare `..`, so `validowner/..#1` built
`repos/validowner/../issues/1` — a path that was never a GitHub endpoint. The
resulting 404 reported `refused-tracker-item-not-found`, naming a missing item
for a lookup that never addressed one.

`verify_tracker_item` now applies the same format validation `verify_fix_commit`
applies to the analogous compare URL, on the resolved repo so there is one rule
rather than two. Still fail-closed either way; what changes is that the refusal
names the malformed reference.

Also records why `gh_http_status` parses another tool's message text: a format
or locale change there yields None, which every caller already treats as
unverifiable, so the coupling costs accuracy and never correctness.
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


PR Review Update — feat(source-control): evidence-gated independent-resolver mode (commit 634cd89)

  • Load tools
  • Read the new commit diff (634cd89)
  • Review verify_tracker_item owner/repo validation fix
  • Check test coverage for the new validation
  • Verify all prior findings remain addressed
  • Post updated review

This commit closes the one remaining SUGGESTION from the last security review: verify_tracker_item was interpolating a caller-supplied owner/repo into a URL path without validating its format, while verify_fix_commit applied GITHUB_OWNER_RE/GITHUB_REPOSITORY_RE for the analogous compare URL.

What Changed

babysit_resolve_thread.py lines 652–658 — format validation inserted before the gh_capture call:

owner, _, name = repo.partition("/")
if not (
    GITHUB_OWNER_RE.fullmatch(owner)
    and GITHUB_REPOSITORY_RE.fullmatch(name)
    and name not in {".", ".."}
):
    return False, "refused-evidence-unverifiable"

The implementation is correct:

  • Leading-dot owner (e.g., .org/repo): caught by GITHUB_OWNER_RE, which requires ^[A-Za-z0-9][A-Za-z0-9-]*$.
  • .. name (e.g., owner/..): GITHUB_REPOSITORY_RE is ^[A-Za-z0-9._-]+$ and admits dots, so the explicit name not in {".", ".."} guard is necessary and correct — the same guard already present in verify_fix_commit at line 596.
  • Returns refused-evidence-unverifiable rather than refused-tracker-item-not-found: no lookup was performed, so naming a missing item would be the wrong signal. Matches the stated definition of refused-evidence-unverifiable.
  • Applied to the resolved repo (post-default_repo fallback): slightly redundant for the default_repo path since parse_repo_number has already validated it, but harmless and keeps one rule rather than two. The comment at line 643–651 explains this choice.

gh_http_status docstring — the commit adds a comment explaining that the function parses gh's message text intentionally unparsable-safe: a format change yields None, which every caller already treats as unverifiable. This documents the intentional coupling-cost/correctness tradeoff noted in the last review.

Test Coverage

Three new cases in OutageIsNotARejection:

Test Input Expected
test_tracker_item_owner_carrying_path_syntax_is_unverifiable validowner/..#1 refused-evidence-unverifiable
test_tracker_item_leading_dot_owner_is_unverifiable .org/repo#1 refused-evidence-unverifiable
test_a_well_formed_explicit_repo_is_still_looked_up other-owner/other.repo#1 resolves → (True, "")

The refusal tests use side_effect=[], proving the guard fires before gh_capture is reached (any call to the mock would raise). The happy-path test confirms that a well-formed explicit owner/repo still reaches the API and resolves correctly.

One minor note: GITHUB_REPOSITORY_RE also admits trailing dots and repeated dots (e.g., re..po), both valid on GitHub. The name not in {".", ".."} guard covers only the two filesystem-special paths. This is the correct scope — any other dot-containing name is a legitimate repo name. Consistent with verify_fix_commit's treatment.

Assessment

All findings from all three prior review passes are now addressed:

Finding Source Status
verify_tracker_item conflates 403/5xx with 404 Claude review Fixed in 4556fe4
verify_fix_commit has same conflation Codex P2 Fixed in 4556fe4
verify_fix_commit uses unvalidated API-returned fields in URL Security review Fixed in 4556fe4
Opener bot's classification reply can satisfy incorrect counter-evidence Security review Fixed in 4556fe4
Multi-finding thread bypass Codex P1 Fixed in 4556fe4
Stale pin not checked in list mode Claude review Fixed in 4556fe4
verify_disposition implicit else branch Claude review Fixed in 4556fe4
verify_tracker_item skips format-validation of caller-supplied owner/repo Security review (suggestion) Fixed in 634cd89

No new issues found. The implementation is correct, the fix matches the pattern verify_fix_commit already uses, and the tests close the named gap without gaps of their own. The PR is clean.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1782 (commit 634cd89)

  • Read PR diff and changed files
  • Analyze trust boundaries and authorization logic
  • Analyze input validation and injection surfaces
  • Synthesize and post findings

Scope note: This pass covers the full PR as currently landed (634cd89), including the three prior fix commits (9df80ba, 4556fe4, 634cd89) since the last security review was written against 4556fe4. The five findings that earlier passes raised have all been addressed. This pass found no new security issues.


No CRITICAL or IMPORTANT issues found.


Assessment

verify_tracker_item format validation (634cd89) — the finding the previous pass raised (SUGGESTION, CONFIRMED) is correctly closed. GITHUB_REPOSITORY_RE admits . and .. because its character class is [A-Za-z0-9._-]+. The new guard (name not in {".", ".."}) is therefore necessary and correctly placed alongside the regex check, matching the identical guard in verify_fix_commit. GITHUB_OWNER_RE = r"^[A-Za-z0-9][A-Za-z0-9-]*$" requires an alphanumeric lead and admits no dots, so owner cannot be . or .. regardless, but the same guard-then-no-dots pattern is consistently applied to both sides for clarity.

URL path segment validation surface — all three fields interpolated into the compare URL in verify_fix_commit (head_owner, head_name, head_oid), and both fields in the issues URL in verify_tracker_item (owner, name), are format-validated before interpolation against GITHUB_OWNER_RE/GITHUB_REPOSITORY_RE plus the .. guard. sha is also validated at the function boundary rather than relying on the caller's prior check. All gh invocations use list arguments throughout; no shell expansion surface.

HTTP status disambiguation (gh_http_status)GH_HTTP_STATUS_RE.findall across stderr returns the LAST match, so a retried request reports its final status. When no (HTTP NNN) pattern appears in stderr (timeout, credential failure, pre-dispatch error), the function returns None, and every caller maps None to refused-evidence-unverifiable. The fail-closed direction is correct: a format change in gh's error output degrades accuracy, not correctness.

Opener-login exclusion for counter-evidence_reply_body excludes all replies whose casefold() login matches the opener's casefold() login. When opener_key is None (login withheld by the API), every reply is excluded and replyBodies is empty — verify_counter_evidence then returns refused-counter-evidence-not-found. Such a thread is already botOnly: false (a bot with an undetectable login would not satisfy the is_bot check), so the incorrect disposition would never reach resolution for that thread anyway. Double-closed.

Multi-finding gate_is_single_finding returns True only on isinstance(count, int) and count <= 1. findingCount = None on a truncated thread returns False (not single-finding), and classify already sets severity_flagged = True on truncation. These two truncation guards are complementary: the severity bright line blocks first, and the multi-finding guard blocks as backup. The two guards use the same body-scan vocabulary (severity_occurrences over _severity_scan_body-preprocessed bodies), so classification-reply echoes are stripped consistently and neither guard can be made to diverge on what counts as a finding.

TOCTOU surface — evidence validation uses the fetched replyBodies snapshot, and verify_counter_evidence searches that snapshot. A comment providing counter-evidence that is subsequently deleted would change commentCount (the live total), triggering count_stale. A comment that is edited to remove the counter-evidence text would change lastCommentUpdatedAt, triggering last_updated_stale. Both stale-pin flags are now evaluated in list mode and under --resolve, so a dry run and the resolve call see consistent protection.

Authorization layering--independent-resolver refuses --autonomous, --include-human, and --allow-unpinned-thread before any network call. --allowed-owners is checked after argument parsing, before threads are fetched. The security and P1 bright line (severityFlagged) is checked before the multi-finding gate and before evidence validation. All subprocess calls use gh as the sole executable with list arguments. No injection surfaces were found.


No security issues found in this pass.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification for the two review runs on 4556fe4f. Both raised one new item; both are fixed in 634cd89e.

# Finding Classification Evidence Reacted
1 Security review, SUGGESTION · CONFIRMED — verify_tracker_item does not format-validate the caller-supplied owner/repo, so validowner/..#1 yields a misleading refused-tracker-item-not-found VALID — fixed Confirmed by reading the pattern: TRACKER_ITEM_RE's character class is [A-Za-z0-9._-]+, which does admit a leading dot and a bare .., while GITHUB_OWNER_RE is ^[A-Za-z0-9][A-Za-z0-9-]*$ 👍
2 Code review, residual observation — gh_http_status parses a gh internal format string; a locale or format change would degrade every failure to unverifiable, which is correct, but the intentional choice deserves a comment VALID — fixed Confirmed: the regex is coupled to another tool's message text, and nothing in the code said so 👍
3 "No CRITICAL issues found" INCORRECT — not a finding The reviewer stating the absence of high-severity issues; recorded because the readiness vocabulary counts the marker occurrence 👍

Finding 1. Fixed, and your reading of the consequence is exactly right — it was a misleading refusal reason rather than a bypass, since the malformed path 404s and the resolve refuses either way. What made it worth fixing rather than deferring is that it is the same inconsistency argument that justified the verify_fix_commit fix one round earlier: the guard existed elsewhere in the codebase for the identical call shape and this function omitted it. It is also self-introduced — verify_tracker_item and TRACKER_ITEM_RE both arrive with this PR — so deferral was not available for it.

One deviation from your suggested fix, and it is deliberate. You scoped the validation to the explicit owner/repo path, noting default_repo is already validated by parse_repo_number. That is true, but I applied it to the resolved repo instead, so there is one rule rather than a validated branch and an exempt branch. It costs nothing on the default_repo path (an already-valid value revalidates trivially) and removes the standing question of whether the exemption is still sound if default_repo's provenance ever changes.

The refusal is refused-evidence-unverifiable, as you specified — the path was never a real GitHub endpoint, so "the world could not be consulted" is the accurate signal.

Tests: 3 cases — a .. component and a leading-dot owner both refuse before any lookup (gh_capture is scripted with an empty side-effect list, so reaching the lookup would raise), and a well-formed explicit other-owner/other.repo#1 is still looked up and accepted, which is what pins that the guard did not over-tighten past what GitHub actually permits in a repository name.

Finding 2. Comment added on gh_http_status, recording that it parses another tool's message text on purpose: a format or locale change yields None, every caller already treats None as unverifiable, so the coupling costs accuracy on a format change and never correctness. You called this out as needing no code change and I agree — the code was already right; what was missing was the reader being told the fragility is chosen rather than overlooked.

On the rest of your assessment. Recording the points you confirmed so they are not re-litigated next round: list-args-only subprocess calls, the gh_http_status fail-closed direction, verify_counter_evidence's empty-needle guard with no minimum length (agreed — needle length is the resolver agent's judgment, not a security property; the property is that the rebuttal is visible on the thread), _reply_body's opener-login exclusion closing the prior counter-evidence finding, stale_pin now accurate in list mode, and parse_repo_number's casefold matching parse_allowed_owners's so the allowlist has no case bypass.

Verification on 634cd89e: 575 tests pass (535 before this PR's review round began, so 40 added across the two commits). markdownlint-cli2, typos, and editorconfig-checker clean on every changed file; check-changelog-parity.sh --check-bump origin/main PASS. All 31 required checks were green on 4556fe4f and are re-running on this head.

…ker-ref shape

The comment's illustrative value carried an `owner/repo#N` shape, which the
comment-hygiene scanner reads as a back-reference to a tracker item. The
constraint it documents is unchanged; the example now names the offending
repository component in prose instead.
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


PR Review Update — feat(source-control): evidence-gated independent-resolver mode (commit 4c8b791)

  • Read the new commit diff (4c8b791)
  • Review the comment/docstring changes
  • Verify all prior findings remain addressed
  • Post updated review

This commit is a single-file, comment-only rewrite. No logic changed, no tests changed, no other files touched.

What Changed

babysit_resolve_thread.py lines 643–651 — the nine-line comment on the verify_tracker_item format-validation guard was reworded. The functional code it describes is unchanged:

owner, _, name = repo.partition("/")
if not (
    GITHUB_OWNER_RE.fullmatch(owner)
    and GITHUB_REPOSITORY_RE.fullmatch(name)
    and name not in {".", ".."}
):
    return False, "refused-evidence-unverifiable"

The rewrite makes two adjustments:

  1. Replaces the inline concrete example (validowner/..#1repos/validowner/../issues/1) with an abstract path template (repos/<owner>/../issues/<n>). The abstract form is actually clearer — it shows which segment carries the traversal without anchoring to a specific owner name.

  2. Changes .. in the first clause from a backtick literal to prose "dot-dot", then re-introduces backtick-quoted `..` later when describing the actual path component. The split is intentional: the first reference is describing the regex character class's permissiveness (a prose claim), the second is naming the literal string in a URL context (a code reference). The distinction is correct.

Everything else — the three-part justification (shape vs. valid, refusal names the malformation, one rule not two), the mention of verify_fix_commit's analogous guard, and the default_repo / parse_repo_number note — carries over unchanged and remains accurate against the code.

Assessment

Comment is accurate. The rewrite is a readability improvement with no behavioral implications. All findings from prior review passes remain addressed, and no new issues were introduced.


@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1782 (commit 4c8b791)

  • Read PR diff
  • Analyze changed files
  • Review trust boundaries and authorization logic
  • Review input validation and injection surfaces
  • Synthesize and post findings

Scope note: This pass covers the PR as currently landed on 4c8b791, the reword-only tip commit over 634cd89. The prior four security review passes raised four confirmed findings; all were addressed in 4556fe4 and 634cd89. This pass found no new security issues.


No CRITICAL or IMPORTANT issues found.


Assessment

verify_fix_commit and verify_tracker_item URL segment validation — both call sites now validate every segment interpolated into a gh api path against GITHUB_OWNER_RE/GITHUB_REPOSITORY_RE/FIX_COMMIT_RE before construction, with explicit not in {".", ".."} guards that complement the regex (both patterns admit a single dot via [A-Za-z0-9._-]+). The validation order is correct: format check before network call, with refused-evidence-unverifiable returned on mismatch rather than a lookup miss. This closes the SSRF-class misdirection surface the second security review raised, and matches the pattern babysit_gh.fetch_blocked_base_compare already used for the same call shape.

HTTP status disambiguation (gh_http_status) — only a confirmed HTTP 404 from gh's stderr produces an evidence-specific refusal; every other non-zero outcome (403, 429, 5xx, timeout, pre-dispatch failure) produces refused-evidence-unverifiable. The LAST match in stderr wins, so a retried request reports its final status. When gh rewrites or localizes its error format, the function returns None and every caller degrades to unverifiable — the recoverable direction, documented in the function's docstring as an intentional coupling choice. The GH_HTTP_STATUS_RE = r"\(HTTP (\d{3})\)" regex captures only digit sequences and cannot be manipulated by URL content: all interpolated URL segments are validated to disallow parentheses (GITHUB_OWNER_RE, GITHUB_REPOSITORY_RE, FIX_COMMIT_RE, and the issue-number group [0-9]+).

Opener-login exclusion for counter-evidence_reply_body at line 360 excludes every comment whose casefold() login matches the opener's. When opener_key is None (login withheld by the API), NO replies are admitted. A comment with a withheld reply-author login is also excluded (not isinstance(login, str)). Both exclusions are over-conservative (fail-closed), not permissive. The prior finding — that the opener bot's own classification reply could satisfy incorrect counter-evidence — is closed.

Multi-finding guard_is_single_finding at line 441 returns True only on isinstance(count, int) and count <= 1. None (truncated page), absent key, and any non-integer all return False, refusing. The count uses the same severity_occurrences vocabulary as the readiness gate (now public for exactly this reason), so the resolver's finding count and the gate's finding count cannot drift. Self-login comments have their classification-table rows stripped before counting, preventing the mandatory classification reply from inflating the count.

Mode combination guards--independent-resolver is refused with --autonomous, --include-human, and --allow-unpinned-thread at line 954 before any thread fetch. Bulk is refused (no --thread-id → exit 2). --disposition without the correct evidence flag is refused. Surplus evidence flags are refused. All checks run at argument-parse time, before any network call.

All subprocess calls use list arguments. gh_capture and run_command never pass a shell string. run_command checks the executable against an allowlist. No shell injection surface in any new code path.

TOCTOU pins retained and correctedstale_pin is now evaluated in list mode too (the args.resolve and args.thread_id gate removed), making dry-run predictions accurate for the case where pins have drifted since vetting. The security property — a resolve that arrives after an edit still sees the stale value — is unaffected.

verify_disposition fallthrough — now explicitly names all three dispositions and returns refused-evidence-unverifiable on the fallthrough. A future disposition added to DISPOSITION_EVIDENCE without a validator here refuses rather than accidentally delegating to verify_counter_evidence.

No other security issues found.


@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Closing classification for the four review runs on 634cd89e and 4c8b7911. None of them raised a finding, so there is no work item to disposition — the rows below record the severity markers those bodies carry so the per-finding ledger stays complete rather than leaving counted markers unaccounted for.

# Marker Classification Evidence Reacted
1 Code review on 634cd89e — "closes the one remaining SUGGESTION from the last security review" INCORRECT — not a finding A back-reference to the already-fixed tracker-item validation item, classified and fixed in 634cd89e 👍
2 Security review on 634cd89e — "No CRITICAL or IMPORTANT issues found" INCORRECT — not a finding The reviewer stating absence; its body explicitly reports "No security issues found in this pass" 👍
3 Security review on 634cd89e — the prior SUGGESTION recorded as closed INCORRECT — not a finding Same already-dispositioned item as row 1 👍
4 Security review on 4c8b7911 — "No CRITICAL or IMPORTANT issues found" INCORRECT — not a finding The reviewer stating absence; "No other security issues found" 👍

The code review on 4c8b7911 carries no severity marker at all and reports the comment reword as accurate with no behavioural implication.

One observation worth answering rather than leaving silent. The 634cd89e code review noted that GITHUB_REPOSITORY_RE also admits trailing and repeated dots (re..po), and that the name not in {".", ".."} guard covers only the two filesystem-special components. Agreed, and that is the intended scope: a name like re..po is a legitimate GitHub repository name and normalizes to nothing surprising in a URL path, whereas . and .. are the only two components a path normalizer collapses. Tightening further would reject valid repositories, which is the wrong direction for a guard whose purpose is to keep a malformed reference from being reported as a missing item. verify_fix_commit draws the line in the same place, so the two functions stay consistent.

Scope note on one detail in the 634cd89e security review. Its scope line lists three prior fix commits including 9df80ba, which is not a commit on this branch — the branch's history is the original work, a merge of origin/main, then 4556fe4f, 634cd89e, and 4c8b7911. Recording it here so the reference is not later mistaken for a missing commit; it does not affect any finding.

Consolidated state of this PR's review round. Every finding raised across all passes is fixed and pushed, with no deferrals and nothing left open:

Finding Source Fixed in
Only one finding validated before the whole thread is resolved Codex, blocking 4556fe4f
API outages reported as rejected evidence (compare) Codex 4556fe4f
Same conflation in verify_tracker_item Code review 4556fe4f
Unvalidated API-returned fields in the compare URL Security review 4556fe4f
Opener bot's own reply can satisfy incorrect counter-evidence Security review 4556fe4f
Stale pin not evaluated in list mode Code review 4556fe4f
verify_disposition's implicit else branch Code review 4556fe4f
Four named test-coverage gaps Code review 4556fe4f
Caller-supplied owner/repo unvalidated in verify_tracker_item Security review 634cd89e
gh_http_status format coupling undocumented Code review 634cd89e

Verification on 4c8b7911: all 31 required checks SUCCESS. 575 tests pass (535 before this round, so 40 added). markdownlint-cli2, typos, editorconfig-checker clean; check-changelog-parity.sh --check-bump origin/main and check-changed-skills.sh origin/main both PASS; guard-contract.md regenerated from its module and byte-stable on re-emit. Branch merged up to origin/main (merge-only, no rebase) and zero commits behind.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1782 (commit 07cfb5a)

  • Load tools
  • Read PR diff and changed files
  • Analyze trust boundaries and authorization logic
  • Analyze input validation and injection surfaces
  • Synthesize and post findings

Scope note: The tip commit 07cfb5a is a merge of origin/main into the feature branch. The only file it touched in the source-control plugin was reference/freshness.md (two-character doc update). All security-relevant files — babysit_resolve_thread.py, babysit_classify.py, the test suite, safety.md — are byte-identical to 4c8b791, which was the subject of the previous security review. This pass therefore covers the full PR diff against origin/main; the incremental change since the last pass is nil on the security surface.


No CRITICAL or IMPORTANT issues found.


Assessment

Mode combination guards--independent-resolver is refused alongside --autonomous, --include-human, and --allow-unpinned-thread at argument-parse time, before any thread fetch. The order matters: conflicting-mode refusal happens first, then disposition/thread-id/evidence checks, then the owner-scope check, then the network calls. Nothing in the merge commit changes this ordering.

Severity vocabulary consistency_has_severity_marker (drives severityFlagged) uses SEVERITY_BLOCK_P01_RE + SEVERITY_BLOCK_WORD_RE + SECURITY_TEXT_RE, while severity_occurrences (drives findingCount) uses SEVERITY_WORDS_RE (CRITICAL/IMPORTANT/SUGGESTION) + SEVERITY_BADGE_RE. The two sets are intentionally different: IMPORTANT and SUGGESTION threads pass the severity bright line and are countable as single findings; P0/P1/CRITICAL/security threads are hard-refused regardless of finding count. A thread with two IMPORTANT markers has findingCount = 2 and is refused by the multi-finding guard; a thread with a P1 marker is refused earlier by the severity bright line. The two guards compose correctly — no bypass path through their vocabulary gap.

finding_count = 0 and the single-finding predicate_is_single_finding returns True for any count <= 1, including 0. A thread with no severity markers has findingCount = 0 and is treated as containing at most one unmarked finding. This is intentional per the inline comment and consistent with the design goal: the guard's purpose is to refuse threads where ONE evidence tuple would suppress a SECOND unaddressed finding. An unmarked thread cannot by definition have a severity-marked second finding counted separately; a thread where the bot genuinely filed two concerns without markers is a reviewer-discipline gap, not one this guard can catch. The practical risk is bounded: such a thread passes severityFlagged = False so has no severity markers, and resolving it with evidence still requires proof against the world.

URL segment injection surface — All five segments interpolated into gh api paths are format-validated before construction: head_owner against GITHUB_OWNER_RE, head_name against GITHUB_REPOSITORY_RE plus an explicit not in {".", ".."} guard, head_oid and sha against FIX_COMMIT_RE (hex-only), match.group("number") via TRACKER_ITEM_RE's [0-9]+ group (digits only). The owner/name fields from TRACKER_ITEM_RE are re-validated against GITHUB_OWNER_RE/GITHUB_REPOSITORY_RE before use. All gh invocations use list arguments. No injection surface.

gh_http_status stderr parsingGH_HTTP_STATUS_RE = r"\(HTTP (\d{3})\)" captures three-digit sequences in a specific format from gh's own stderr. All URL segments are validated before being passed to gh, so no user-supplied value can cause gh to emit an unexpected (HTTP NNN) pattern. When no match is found — format change, locale, pre-dispatch failure — the function returns None, and every caller maps None to refused-evidence-unverifiable. The fail-closed direction is correct.

Counter-evidence opener-login exclusion_reply_body now correctly excludes every reply whose casefold() login matches the opener's. When the opener's login is absent (opener_key is None), no replies are admitted. When a reply author's login is absent (not isinstance(login, str)), that reply is also excluded. Both exclusions are over-conservative: the prior finding (opener bot's classification reply satisfying the incorrect check) remains closed, and the independent-party property is intact for the remaining admissible replies.

TOCTOU pin accuracystale_pin = count_stale or last_updated_stale is now evaluated in every mode (the prior args.resolve and args.thread_id gate removed), making list-mode dry run predictions match what --resolve would do. A reply posted or edited after the caller pinned the thread is detected in both list and resolve modes.

Authorization layering — Owner-scope check precedes @me resolution (no network call on out-of-scope owners). @me resolution failure degrades to explicit logins only (unresolved self identity is never silently trusted). Bulk refusal is enforced at argument-parse time for --independent-resolver (stricter than --autonomous, which refuses bulk only under --resolve). The refusedEvidence counter excludes refused-stale-pin (correct — TOCTOU refusals are not evidence refusals).


No security issues found in this pass. The five findings that prior review rounds raised were all addressed in 4556fe4 and 634cd89; the current tip adds nothing new to the security surface.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


PR Review Update — feat(source-control): evidence-gated independent-resolver mode (merge commit 07cfb5a)

  • Load tools
  • Review merge commit diff (07cfb5a) against prior tip (4c8b791)
  • Verify no conflicts or regressions from the merge
  • Confirm all prior findings remain addressed
  • Post findings

This commit is a sync merge from origin/main into the feature branch. It brings in 20 files across other plugins and CI scripts — none of which touch the independent-resolver implementation.

What Changed in the Merge

Source-control plugin — one file touched:

plugins/source-control/skills/babysit-prs/reference/freshness.md (one line)

-https://docs.github.com/en/graphql/reference/enums#mergestatestatus
+https://docs.github.com/en/graphql/reference/pulls#enum-mergestatestatus

A corrected GitHub Docs URL for the MergeStateStatus enum. The prior URL pointed to the generic enums index; the new one resolves to the correct anchor under the Pulls reference. Factually accurate and unrelated to the independent-resolver feature.

Everything else in the merge (session-flow plugin updates, work-items eval additions, dometrain/simulation doc edits, new check-skill-portability.sh CI script and its tests) is entirely orthogonal to the source-control plugin being reviewed.

Merge Quality

  • No conflicts: The merge is clean. The nine files this PR owns (babysit_resolve_thread.py, test_babysit_resolve_thread.py, guard_contract.py, babysit_classify.py, SKILL.md, safety.md, guard-contract.md, plugin.json, CHANGELOG.md) are not touched by the merge commit.
  • All prior findings remain addressed: The ten findings addressed across 4556fe4 and 634cd89 are unaffected. No new regression surface introduced.
  • No new issues: Nothing in the merged code interacts with the independent-resolver logic, evidence validation, or the guard contract.

Assessment

The merge is a routine sync. The feature implementation is unchanged and all prior review findings remain closed. No new issues.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification for the two review runs on the sync merge 07cfb5a3. Neither raised a finding. Both confirm the merge touched nothing this PR owns — its only source-control change is a one-line GitHub Docs URL correction in reference/freshness.md, arriving from origin/main.

The rows below account for every severity-marker occurrence those two bodies carry. Each is prose — a stated absence, or the reviewer quoting the code's own vocabulary constants while explaining how two guards compose. None is a work item.

# Marker occurrence Classification Evidence Reacted
1 "No CRITICAL … issues found" INCORRECT — not a finding Stated absence 👍
2 "… or IMPORTANT issues found" INCORRECT — not a finding Same stated absence 👍
3 Quoting SEVERITY_WORDS_RE's first constant INCORRECT — not a finding The reviewer citing the code's own vocabulary, not raising an issue 👍
4 Quoting SEVERITY_WORDS_RE's second constant INCORRECT — not a finding Same citation 👍
5 Quoting SEVERITY_WORDS_RE's third constant INCORRECT — not a finding Same citation 👍
6 "… threads pass the severity bright line" (first word) INCORRECT — not a finding Explaining how the two guards compose 👍
7 "… threads pass the severity bright line" (second word) INCORRECT — not a finding Same explanation 👍
8 "P0/P1/… /security threads are hard-refused" INCORRECT — not a finding Same explanation 👍
9 "a thread with two … markers has findingCount = 2" INCORRECT — not a finding Worked example of the guard behaving as designed 👍

Two observations in the security pass worth answering rather than leaving silent, since both concern the multi-finding guard's intended scope:

The reviewer notes the two severity vocabularies differ — _has_severity_marker (driving severityFlagged) versus severity_occurrences (driving findingCount) — and concludes they compose without a bypass. That is right, and it is deliberate: the bright line is narrower than the counting vocabulary on purpose, because advisory threads are exactly what the worker is documented to resolve once addressed, so a bright line as wide as the counting vocabulary would self-block the merge gate on its own advisory threads. The composition the reviewer traced is the intended one: the bright line refuses first, the finding count refuses second.

The reviewer also notes _is_single_finding admits findingCount == 0, so a thread whose bot filed two concerns without markers would pass. Agreed, and that is a real residual limit rather than an oversight — an unmarked second concern is invisible to any marker-based count, so this guard cannot catch it. It is not a deferral of the finding that was raised: the finding was that one validated disposition could clear a thread carrying several counted findings, and that is closed. The residual is bounded exactly as the reviewer describes — such a thread carries no severity marker at all, and resolving it still requires evidence proven against the world. Closing it would need a different mechanism (per-finding structure in the thread rather than marker counting), which is a change to what reviewers emit, not to this guard.

State of this PR. Every finding raised across all review passes is fixed and pushed; none deferred, none outstanding. Head 07cfb5a3, all 31 required checks SUCCESS, mergeStateStatus CLEAN, zero commits behind origin/main (freshened by merge, never rebased). 575 tests pass. Both codex threads are resolved, each after its fix was pushed and cited in-thread. I am not merging this — that call is not mine.

…ts behavior

The test built its thread through the `_bot_thread` fixture helper, which takes
`replyBodies` as a literal — so a hardcoded empty list asserted nothing about
`project_thread`'s opener-login exclusion and passed identically with that fix
reverted. The three sibling cases in the class carried the real coverage.

It now projects through `project_thread` from a record whose OPENER posts the
later reply carrying the exact counter-evidence text. Verified by reverting the
exclusion in a scratch copy: the class goes from 6 passing to 4 failing,
including this case.
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


PR Review Update — feat(source-control): evidence-gated independent-resolver mode (commit ee0d2a9)

  • Load tools
  • Read the new commit diff (ee0d2a9)
  • Review test changes for the opener-exclusion end-to-end test
  • Verify all prior findings remain addressed
  • Post updated review

This commit is a single-file, test-only change. No production code was touched.

What Changed

test_babysit_resolve_thread.pytest_end_to_end_opener_reply_cannot_satisfy_the_claim

The previous version of this test used _bot_thread(reply_bodies=[]), which passes replyBodies as a literal to the fixture helper — bypassing project_thread entirely. Because the exclusion logic lives inside project_thread, an empty hardcoded list would produce the same replyBodies result whether or not the opener-login exclusion was applied. The test was therefore vacuously passing: it would have remained green even with the fix reverted.

The fix replaces the fixture helper with a round-trip through project_thread:

projected = rt.project_thread(
    self._record(
        self._comment("codex", "The anchor points at generated output."),
        self._comment("codex", "Status: the anchor points at generated output."),
    )
)
self.assertEqual(projected["replyBodies"], [])

This directly asserts that project_thread suppresses the opener's classification reply. The subsequent _run_independent call then uses this projected data (with protocol fields added via dict(projected, ...)) so the end-to-end resolution attempt operates on what project_thread actually produced.

The commit message confirms mutation testing: "Verified by reverting the exclusion in a scratch copy: the class goes from 6 passing to 4 failing, including this case." That is the correct standard for a test that claims to verify a specific guard — the test must fail when the guard is absent.

Correctness

The fix is correct in its diagnosis and its repair:

  • Root cause is right: _bot_thread takes replyBodies as a literal, so any exclusion happening inside project_thread is invisible to it.
  • Intermediate assertion added: self.assertEqual(projected["replyBodies"], []) makes the exclusion property separately visible before the end-to-end run. If project_thread ever regresses, this assertion will fail on its own rather than the refusal happening to produce the right exit code for an unrelated reason.
  • Protocol fields added correctly: commentCount=2 matches the two comments in the record; findingCount=1 matches the single finding in the thread; lastCommentUpdatedAt=PINNED_UPDATED preserves the TOCTOU pin that the other end-to-end cases use.
  • No new gaps introduced: The three sibling cases in CounterEvidenceExcludesTheFindingsAuthor that carry the real unit coverage of _reply_body are unchanged.

Assessment

All findings from the full review cycle remain addressed. This commit closes a gap in the test suite — the end-to-end case that names the opener-exclusion security property was not actually sensitive to the presence or absence of that property. It now is. The PR is clean.


@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1782 (commit ee0d2a9)

  • Read PR diff (full + incremental since last review at 07cfb5a)
  • Read changed files (babysit_resolve_thread.py, babysit_classify.py, test suite)
  • Analyze trust boundaries and authorization logic
  • Analyze input validation and injection surfaces
  • Synthesize and post findings

Scope note: The tip commit ee0d2a9 modifies only the test file. All production code (babysit_resolve_thread.py, babysit_classify.py, babysit_gh.py) is byte-identical to 07cfb5a, which was the subject of the previous security review pass. This pass covers the incremental test change in full, and verifies the complete PR security surface against origin/main.


No CRITICAL or IMPORTANT issues found.


Incremental Change (ee0d2a9) — Security Analysis

The commit rewrites test_end_to_end_opener_reply_cannot_satisfy_the_claim in CounterEvidenceExcludesTheFindingsAuthor. The change is a security regression test strengthening, not a code change.

Before: The test called _bot_thread(reply_bodies=[]) — a fixture helper that supplies replyBodies as a hardcoded empty list. The end-to-end run then correctly returned refused-counter-evidence-not-found, but that outcome was governed by the empty list rather than by the exclusion logic. Reverting the _reply_body opener-login exclusion (removing the login.casefold() == opener_key guard at line 364) would leave the test passing unchanged.

After: The test calls project_thread with the opener ("codex") posting both the finding and a follow-up reply containing the exact counter-evidence text, then asserts that projected["replyBodies"] == [] and that the end-to-end run refuses. If the exclusion is reverted, the opener's classification reply appears in replyBodies, the substring match succeeds, and the thread resolves — the test catches the regression. The revised test is a genuine falsifying witness for the security property it documents.


Full PR Surface Assessment (against origin/main)

URL segment injection — All five segments interpolated into gh api paths are format-validated before construction (lines 593–600 for verify_fix_commit; lines 652–658 for verify_tracker_item). All gh invocations use list arguments throughout; no shell expansion surface.

HTTP status disambiguationgh_http_status (line 523) applies GH_HTTP_STATUS_RE.findall across stderr and takes the last match. All URL segments supplied to the gh subprocess are hex-only or alphanumeric-only, so no user-controlled data can craft (HTTP NNN) in stderr. When no match is found, callers receive refused-evidence-unverifiable.

Counter-evidence searchverify_counter_evidence (line 683) does needle in body.casefold() — pure Python string operations, no subprocess calls or file system access. The empty-needle guard (if not needle) correctly rejects empty or whitespace-only strings. The replyBodies values come from the GitHub API and are strings; no injection surface.

Opener-login exclusion_reply_body (line 360) excludes every reply whose casefolded login matches the opener's. When opener_key is None (login withheld by the API), no replies are admitted. A reply with a non-string author login is also excluded. Both cases are over-conservative (fail-closed). The new test in ee0d2a9 pins this as a falsifying regression test.

Mode combination guards--independent-resolver is refused with --autonomous, --include-human, and --allow-unpinned-thread at argument-parse time before any thread fetch or network call (lines 953–970). Bulk is refused (no --thread-id → exit 2). Surplus evidence flags are refused.

Multi-finding guard_is_single_finding (line 438) accepts only isinstance(count, int) and count <= 1. None (truncated page) and any non-integer value both return False, refusing. The count uses the same severity_occurrences vocabulary as the readiness gate, now public from babysit_classify rather than re-implemented, preventing the two from diverging on what constitutes a finding.

TOCTOU pinsstale_pin = count_stale or last_updated_stale is evaluated in every mode; the prior args.resolve and args.thread_id gate (which made dry-run predictions wrong) is removed. Evidence validation happens only when verdict == "eligible" and not stale_pin — a drifted pin refuses before reaching any evidence check.

Severity bright line — The (autonomous or independent) and severityFlagged check (line 483) runs before the multi-finding guard and before evidence validation. It is unconditional for both unattended modes; no evidence tuple buys past it.

severity_occurrences visibility changebabysit_classify.py makes _severity_occurrences public as severity_occurrences. The function body is unchanged; only the name is exported. No security implication.


No security issues found in this pass. The five findings that prior review rounds raised were addressed in 4556fe4 and 634cd89. The incremental change in ee0d2a9 is a test-only strengthening of the opener-exclusion regression coverage. The implementation is correct and the security surface is unchanged from the previous pass.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification for the two review runs on ee0d2a96, plus a correction to one claim I made earlier in this thread.

# Marker occurrence Classification Evidence Reacted
1 Security review on ee0d2a96 — "No CRITICAL … issues found" INCORRECT — not a finding Stated absence; the pass reports "No security issues found in this pass" 👍
2 Security review on ee0d2a96 — "… or IMPORTANT issues found" INCORRECT — not a finding Same stated absence 👍

The code review on ee0d2a96 carries no severity marker and raises nothing. Neither pass found a new issue.

What ee0d2a96 fixes

An independent reviewer caught that test_end_to_end_opener_reply_cannot_satisfy_the_claim was a mislabeled test: it built its thread through the _bot_thread fixture helper, which takes replyBodies as a literal, so the hardcoded empty list — not project_thread's opener-login exclusion — was producing the refusal. It would have passed identically with that fix reverted. A test that cannot fail when the behavior it names is removed is not coverage, and the class's headline case was the one carrying that defect while its three siblings did the real work.

It now projects through project_thread from a record where the opener itself posts the later reply carrying the exact counter-evidence text, and asserts both replyBodies == [] and the end-to-end refusal.

I verified it pins the behavior rather than assuming: reverting the exclusion in a scratch copy takes the class from 6 passing to 4 failing, this case among them. The production file was restored byte-identical to the prior commit afterward — ee0d2a96 touches the test file only, which the security pass independently confirms.

Correction to my earlier reply

In my round-two comment I described the test coverage for the opener-exclusion finding as six cases pinning the behavior. That was one case too generous — five pinned it and one did not. The finding itself was correctly fixed and remains so; it was my evidence for it that was overstated. Corrected here rather than left standing, since an overstated coverage claim in a classification reply is the kind of thing later rounds build on.

Guard-contract counts — checked, and the CHANGELOG is accurate

The same reviewer flagged the CHANGELOG line "Five refusal rows and six classifier predicates were added to the guard contract" as false, having measured the diff of 4556fe4f alone (which added 0 refusal rows and 3 predicates). I checked before changing anything, because that sentence sits in the 0.42.0 entry and a CHANGELOG entry describes a release, not one commit. Counted against this PR's merge-base with origin/main:

  • id="resolve.* (REFUSALS): 10 → 15, so five added
  • id="classify.* (PREDICATES): 7 → 13, so six added

The sentence is exactly right at the granularity it is written for; the five refusal rows arrived with the original #1632 commit and three of the six predicates did. I have left it unchanged rather than "correcting" an accurate statement into a wrong one.

One genuine inaccuracy does exist at commit granularity: 4556fe4f's own message says "Six classifier predicates added to the guard contract" when that commit added three. Rewriting it needs a force-push, which is off the table here, so it stands — recorded in this thread instead so the discrepancy is explained rather than discovered.

State

Head ee0d2a96, all 31 required checks SUCCESS, mergeStateStatus CLEAN, zero commits behind origin/main, zero unresolved threads. 575 tests pass. Every finding raised across every review pass is fixed and pushed, with no deferrals.

@kyle-sexton
kyle-sexton merged commit 9647149 into main Jul 30, 2026
31 checks passed
@kyle-sexton
kyle-sexton deleted the feat/1632-independent-resolver branch July 30, 2026 14:50
kyle-sexton added a commit that referenced this pull request Jul 30, 2026
Resolves the source-control version collision: this branch bumped 0.41.0 ->
0.41.1 for the shared hook-utils sync while main landed 0.42.0 (#1782).

- plugins/source-control/.claude-plugin/plugin.json: version 0.42.1, taking
  main's 0.42.0 as the new floor and reapplying this branch's patch bump.
- plugins/source-control/CHANGELOG.md: this branch's entry re-headed
  ## [0.42.1] above main's ## [0.42.0], keeping the file newest-first with no
  repeated version. Entry prose is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W5KaGv34aWCL1epZMSiupC
kyle-sexton added a commit that referenced this pull request Aug 9, 2026
…urrent bot thread (#1826)

Fixes #1641

## Summary

A `babysit-prs worker` that correctly disproves a bot finding —
classifies it `INCORRECT`, posts
counter-evidence — ships no fix by definition, so the thread stays
**current** and satisfies neither
`babysit_resolve_thread.py::classify`'s `isOutdated` requirement under
`--autonomous` nor the Worker
Contract's tighter pre-push-outdated rule. A grounded `VALID (defer)`
and a prose fix that rewrote
elsewhere in the file land in exactly the same place. Under a base whose
ruleset requires thread
resolution, the PR then sits unmergeable on a finding that was fully and
correctly addressed.

**Scope note, because the issue predates the mechanism.** #1641 was
filed before #1782 landed
`--independent-resolver`. That PR supplied the *mechanism* — an
evidence-gated third mode that
replaces `isOutdated` with caller independence plus validated
disposition evidence. Nothing supplied
the **route**: the only dispatch that could invoke it was
`babysit-loop`'s pre-escalation resolver,
reachable only on the explicit `autopilot` + `--merge c3-this-run`
widening. So on every ordinary
worker-tier run the D7.5 routing rule #1633 wrote down terminated in a
fail-closed report, and the
capability gap the issue names stayed open. This is what makes that
routing rule executable rather
than merely descriptive.

This takes the issue's **option 1** (orchestrator-side retirement),
which the orchestrator lane's
comment on the issue endorsed.

## Fix

**The route.** The worker now **reports** an addressed-but-unresolvable
current bot thread — thread
id, disposition, and where the evidence lives (the reply carrying the
counter-evidence, the tracker
item id, or the commit SHA) — instead of leaving it silently. Reporting
nothing strands the thread,
because the orchestrator cannot re-derive from a snapshot which current
threads were addressed this
round. The orchestrator then routes it, **under the PR's worker lease
and before Cleanup releases
it**, to a fresh subagent that authored neither the fix nor the
counter-evidence.

**The guard is untouched.** `classify`'s `isOutdated` requirement under
`--autonomous` is not
weakened — no script changed at all. The property it was a proxy for is
what the dispatch preserves:
*the context that authored the evidence is not the context that acts on
it.* The orchestrator does
**not** resolve the thread itself; it holds the merge decision, so
adjudicating its own unblock would
be the same self-certification one hop up.

**Independence is made load-bearing in prose, because the wrapper cannot
check it.**
`verify_counter_evidence` requires only that the text appear in a reply
by someone other than the
thread's **opener** — a worker's own reply under a `--self-logins`
identity is admissible input. So
the dispatched resolver re-derives the evidence at the live head rather
than passing the
orchestrator's brief through; otherwise the dispatch is a laundering
hop, not an adjudication.

**Pins are read fresh, never forwarded.** The worker's dispatch-snapshot
`commentCount` /
`lastCommentUpdatedAt` are pre-reply, and the worker's own mandated D5
classification reply moves
both — forwarding them produces `refused-stale-pin` deterministically.
The documented flow lists the
thread first (list mode validates the evidence too), takes the pins from
that output, then resolves.
`--self-logins` is documented as non-optional on this route: omit it and
the worker's reply flips
`botOnly` false and the thread returns `skipped-human-thread`.

**One contract, two callers.** Almost the whole of
`babysit-loop/reference/pre-escalation-dispatch.md`
was babysit-prs mechanics — the D7.5 per-finding ledger, the worker
lease, the worktree lifecycle,
the guarded wrappers. Writing a second copy into `orchestration.md`
would have forked it, so the
reusable contract moved to the skill that owns the wrapper: new
`babysit-prs/reference/independent-resolution.md`.
`pre-escalation-dispatch.md` keeps only its
widening-specific bounds (frontier tier, the four blocker classes it
never touches, the post-dispatch
re-partition) and points there.

**The fail-closed fallback survives verbatim** for every bound the
dispatch cannot cross — a
security/P1 thread (`skipped-severity-marked`), a multi-finding thread
(`skipped-multi-finding-thread`), a human thread, evidence the world
rejects, or no subagent tools to
dispatch to: *leave the thread unresolved, do not merge, and report the
PR with the
addressed-but-unresolvable thread named.* This adds a path; it does not
replace the fallback.
`safety.md`'s Security/P1 "only one dispatch path" bullet is unchanged
in substance and now says so
explicitly — the orchestrator-side dispatch is not a second route to
that exception, because the
wrapper's severity bright line refuses those threads on it.

**Stale claims corrected.** `review-discipline.md`'s D7.5 authorization
rule and
`babysit-prs/reference/loop.md`'s Never-Do entry both asserted the
dispatch was "reachable only on
the explicit `autopilot` + `--merge c3-this-run` widening". True when
written; now it names the two
invocations that reach one.

## Verification

| Criterion (issue #1641) | Status |
|---|---|
| A disproved current finding reaches a terminal state — retired by an
authorized context, or a clearly-reported escalation | Done —
orchestrator dispatch on the ordinary worker-tier path; the fail-closed
report survives verbatim for every bound the dispatch cannot cross |
| `classify`'s `isOutdated` requirement under `--autonomous` is not
weakened | Done — **no script logic changed**; `git diff` touches only
`tests/guard_contract.py`'s doc-source table |
| The context that authored the counter-evidence is not the one that
unblocks its own merge | Done — worker reports and never resolves;
orchestrator dispatches and never resolves; the resolver re-derives
evidence at the live head rather than accepting the brief's |
| `guard-contract.md` gains a row for any new refusal or allowance |
Done — no new refusal or allowance exists (the mode shipped in #1782),
so the row added is the new file's
`independent-resolution.dispatch-commands` doc-command source;
`test_every_doc_naming_a_wrapper_is_covered` requires it, and
`guard-contract.md` is regenerated, never hand-edited |

Commands run in the worktree:

- `bash
plugins/source-control/skills/babysit-prs/scripts/engine.test.sh` —
**575 tests, OK**; ruff
  clean; guarded-wrapper behavior suite green. This includes
`test_every_documented_wrapper_command`, which now parser-validates both
copyable commands in the
new reference file, and `test_every_doc_naming_a_wrapper_is_covered`,
which fails if a new .md
  spells a wrapper command with no `DOC_COMMAND_SOURCES` row.
- `python tests/guard_contract.py --emit` — `guard-contract.md`
regenerated from the module (hand
  edits fail CI).
- `npx markdownlint-cli2 "plugins/source-control/**/*.md"` — 49 files, 0
errors.
- `scripts/check-changelog-parity.sh --check` / `--check-bump
origin/main` / `--check-order` — pass.
- `scripts/check-skill-portability.sh origin/main`,
`check-skill-leaf-names.sh`,
  `check-silent-skips.sh` — pass.

`babysit-prs/SKILL.md` is 490 lines, under the 500 cap #1626 tracks —
the new contract went into a
reference file, not SKILL.md.

Rebased onto `origin/main` after `0.42.1` landed mid-work, and later
merged `origin/main` again
after `0.48.0` shipped; `plugins/source-control` bumped to `0.49.0`
(feature: a new route, plus
a new reference file).

## Related

- Refs #1782 — shipped `--independent-resolver`, the mechanism this PR
supplies the route for
- Refs #1633 — wrote down the D7.5 routing rule that had no reachable
dispatch on the worker path
- Refs #1614 — the adjudication whose `isOutdated` guard must not be
weakened, and is not
- Refs #571 — the still-open machine-enforced displacement fix,
untouched here

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

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
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.

feat(source-control): independent-resolver mode for babysit_resolve_thread.py (evidence-gated, non-outdated bot threads)

1 participant