fix(source-control): require per-thread pins for autonomous bulk babysit thread resolves

[kyle-sexton]

Summary

babysit_resolve_thread.py's bulk resolve path (--resolve with no --thread-id) did not require the per-thread TOCTOU pins the single-thread path already enforces. Because an autonomous worker's own push marks a review thread isOutdated, the bulk --autonomous --resolve path could clear threads that changed since they were vetted with no proof the finding was addressed — the actor signing its own permission slip. This implements the maintainer-confirmed Option 1: reject bulk autonomous resolves and require a per-thread vetted loop.

Fix

• Guard (root cause): In babysit_resolve_thread.py, --autonomous --resolve without --thread-id is now refused (exit 2, before any network fetch), forcing the unattended-worker path through a per-thread loop where each thread reuses the existing pin guard (--expected-comment-count + --expected-last-updated). No second pinning model is introduced.
• Closed bypass (deliberate extension beyond AC1's literal "without --thread-id"): --allow-unpinned-thread is also refused in --autonomous mode — otherwise --autonomous --resolve --thread-id X --allow-unpinned-thread would reopen the exact "resolve with no pins" hole. It stays an interactive-only override. Flagged here explicitly for maintainer visibility; it is restriction-direction (auto-ratified) and fail-closed, consistent with the Option 1 rationale.
• Docs: Updated the module docstring and SKILL.md Autopilot step 2 from a bulk call to the per-thread vetted loop, aligning them with the pinned form already documented in reference/orchestration.md, reference/safety.md, and the worker prompt template (those were already correct; the code and step 2 were the stragglers).
• Test: Two regression tests in test_guards.py prove the bulk autonomous resolve is refused and that --allow-unpinned-thread cannot bypass pins in autonomous mode.
• Version bump 0.9.3 → 0.10.0 (behavior change to the autonomous-worker contract) + CHANGELOG entry.

Verification

Full plugin engine suite (stdlib unittest + ruff + guarded-wrapper behavior) via bash engine.test.sh:

== unittest suite ==
Ran 219 tests in 1.293s
OK
== ruff ==
All checks passed!
== guarded-wrapper behavior ==
PASS: merge wrapper rejects --allow-unpinned-head
PASS: merge wrapper reaches fail-closed CLI (no allowlist)
PASS: resolve wrapper reaches fail-closed CLI (no allowlist)
=== EXIT: 0 ===

New regression tests (python -m unittest tests.test_guards -v):

test_autonomous_allow_unpinned_thread_is_refused ... ok
test_autonomous_bulk_resolve_without_thread_id_is_refused ... ok
... (11 tests) ... OK

markdownlint-cli2 on the two changed Markdown files: Summary: 0 error(s). Plugin + catalog manifests: All plugin manifests and the catalog validated.

Closes #326

Related

• fix(source-control): require per-thread pins for autonomous bulk babysit thread resolves #326 — the issue this closes (maintainer decision: Option 1, reject bulk autonomous resolves).
• feat(source-control): converge babysit-prs tiers + engine (0.8.0) #322 — the PR whose review pass originated this finding.
• fix(source-control): reconcile review-trigger 'pending or failing' gate contract with pending-only code #324 — sibling issue from the same review pass.
• Autonomous thread-resolve: pin pre-push isOutdated state (per-thread comment pins don't stop own-push displacement) #571 — machine-enforced fix for the displacement bypass (autonomous worker clearing a bot thread its own push merely displaced); the per-thread pins in this PR do not close it.

🤖 Generated with a claude-code-plugins work-2 lane implementation subagent (model: opus)

[claude]

PR Review in Progress

• Gather context and understand the PR
• Read changed files
• Review Python guard logic
• Review test coverage
• Review documentation changes
• Review version/changelog
• Post findings

View job run

[claude]

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

---

PR Review — fix(source-control): require per-thread pins for autonomous bulk babysit thread resolves

• Gather context and understand the PR
• Read changed files
• Review Python guard logic (babysit_resolve_thread.py)
• Review test coverage (test_guards.py)
• Review documentation changes (SKILL.md, CHANGELOG.md)
• Review version bump (plugin.json)
• Post findings

---

Overall assessment

This PR is well-executed and correct. The security fix is well-scoped, the guards are layered correctly, and the tests directly exercise the new refusal paths. No actionable bugs found.

---

Guard logic — babysit_resolve_thread.py

The two new guards slot into the pre-network validation block in the right order:

line 296  --expected-comment-count without --thread-id → exit 2
line 311  --expected-last-updated without --thread-id → exit 2
line 326  --autonomous --resolve without --thread-id  → exit 2  ← NEW
line 346  --autonomous --resolve --allow-unpinned-thread → exit 2  ← NEW
line 363  --resolve --thread-id without pins (existing) → exit 2

Guard ordering trace — all adversarial inputs are closed:

Input	Guard fires	Result
--autonomous --resolve (no thread-id)	line 326	refused ✓
--autonomous --resolve --allow-unpinned-thread (no thread-id)	line 326 first	refused ✓
--autonomous --resolve --thread-id X --allow-unpinned-thread	line 346	refused ✓
--autonomous --resolve --thread-id X (no pins)	line 363 (existing)	refused ✓
--autonomous --resolve --thread-id X --expected-comment-count N --expected-last-updated T	none	proceeds to TOCTOU check ✓

The "before any network fetch" claim in the test comments is accurate — parse_repo_number (line 290) is a pure string parse, not a network call.

One minor observation on guard #2 (line 346): The condition is args.resolve and args.autonomous and args.allow_unpinned_thread — it correctly requires --resolve to be present. Without --resolve, --allow-unpinned-thread in autonomous mode is harmless (list-only), and blocking it would add noise for any autonomous list call. The condition is tighter than it needs to be and that's a good thing.

The classify() function at line 194 still filters not isOutdated threads in autonomous mode — so even if a bug slipped past these guards, the isOutdated requirement would remain as a backstop. Defense in depth is intact.

---

Test coverage — test_guards.py

Both new tests are correctly structured:

• test_autonomous_bulk_resolve_without_thread_id_is_refused — asserts exit 2 and that "thread-id" and "bulk-resolve" both appear in the error. Both strings are present in the error message verbatim. ✓

• test_autonomous_allow_unpinned_thread_is_refused — asserts exit 2 and "allow-unpinned-thread" in the error, which matches the error string. ✓

The subprocess-based test pattern (no gh stub needed, refused before any network call) is consistent with the existing guard tests in the class and is the right approach here.

Observation: There's no positive-path test for the newly required form (--autonomous --resolve --thread-id X --expected-comment-count N --expected-last-updated T reaching the TOCTOU check rather than hitting a guard). This would require a gh stub to go further, which the existing infrastructure doesn't do for this module. The refusal tests are the critical coverage for the security boundary, so this is an acceptable gap in scope.

---

Documentation

SKILL.md step 7 — the updated runbook text is accurate and complete. The per-thread loop form is now documented with the correct flag set:

--autonomous --resolve --thread-id <id> --expected-comment-count <n> --expected-last-updated <ts>

The inline rationale ("so a worker cannot clear a thread its own push merely displaced") correctly explains why the bulk form is refused, which is important for operators who may have relied on the old behavior.

CHANGELOG.md — the entry under [0.10.0] is detailed and accurate. It correctly calls out the behavior change to the autonomous-worker contract and covers both the bulk-resolve refusal and the --allow-unpinned-thread restriction. This is useful for consumers upgrading from 0.9.3.

---

Version bump — plugin.json

0.9.3 → 0.10.0 (minor bump) is correct. This is a breaking change to the autonomous-worker call contract (commands that previously worked will now exit 2), so a minor bump is appropriate. A patch bump would have understated the change.

---

Summary

No bugs or security gaps found. The fix is minimal, fail-closed, correctly layered with the existing TOCTOU guard, and documents the new contract clearly. Approve.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37c7a02fd5

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

[kyle-sexton]

Review-response classification

#	Source	Finding	Classification	Evidence
1	claude[bot] review (job)	Guard ordering, tests, docs, and version bump review	No action — explicit approval	"No bugs or security gaps found... Approve." Guard-ordering trace and defense-in-depth observations verified against the code; the two "observations" (tighter-than-needed guard #2 condition; no positive-path TOCTOU test) are called out as acceptable, not actionable.
2	chatgpt-codex-connector P1 (thread)	Pin the pre-push isOutdated state before resolving	VALID (defer → #571)	Verified: the per-thread comment pins don't detect a displacing push that flips isOutdated without changing comments; falsifies the "cannot clear a thread its own push merely displaced" claim. Full analysis, deterministic-indistinguishability limit, and merge-gate posture trade-off in #571. Codex thread left unresolved pending the fix.

No code changes pushed in this pass — finding 2's fix is a flow redesign gated on a merge-gate posture decision (see #571); finding 1 requires none.

[kyle-sexton]

Control-tower audit verdict: HOLD — one cheap, separable fix required before merge.

The diff faithfully implements #326's ratified Option 1. But SKILL.md step-7 (diff L59) and the docstring (L26-33) assert a worker cannot clear a thread its own push merely displaced — and the PR's own unresolved P1 thread (classified VALID by the author, root fix deferred to #571) demonstrates that claim is false as shipped: the per-thread pins cover comment count/last-updated only, so a displacing push flips isOutdated while both pins still match, and a pinned resolve clears an unaddressed finding. Verified against babysit_merge.py:71-95 — resolved-via-displacement threads drop out of the merge blocker set, so the hole is live, not theoretical.

Required before merge: correct/caveat the L59 + docstring language to describe only the enforced behavior (pins prevent comment-state races; displacement bypass remains until #571). Deferring the displacement fix to #571 is a legitimate, correctly-surfaced posture call — shipping a known-false present-tense guarantee in an area:security skill is the only blocker. Keep #571 linked in ## Related.

[claude]

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

---

Review of new commit — docs(source-control): caveat displacement-bypass gap in bulk-resolve pin guarantee

• Gather context: new commit b9abec0 addresses control-tower HOLD
• Read changed files in new commit (3 files: babysit_resolve_thread.py, SKILL.md, CHANGELOG.md)
• Verify docstring caveat is accurate and complete
• Verify SKILL.md tool description + step-7 caveat is accurate and complete
• Confirm HOLD condition is resolved
• Post findings

---

HOLD status: Resolved

The control-tower required:

Correct/caveat the L59 + docstring language to describe only the enforced behavior (pins prevent comment-state races; displacement bypass remains until #571).

Commit b9abec0 corrects all four affected locations. Each now accurately states:

• What the pins do enforce: comment count + latest comment-edit timestamp — a reply or edit after vetting blocks the resolve.
• What the pins do not enforce: displacement — a push that flips isOutdated without touching comments still passes both pins.
• That the displacement bypass is a known open gap tracked in Autonomous thread-resolve: pin pre-push isOutdated state (per-thread comment pins don't stop own-push displacement) #571, not a silent hole.
• That the mitigation for displacement is agent-discipline (pre-push-outdated rule in reference/orchestration.md), not machine enforcement.

---

Per-location verification

Module docstring (babysit_resolve_thread.py:30–40)

The old text claimed the per-thread pins are "what prove each thread is unchanged since the agent vetted it" — that was the false guarantee. The new text:

• Calls out comment-state enforcement explicitly ("they refuse a thread whose comment count or latest comment-edit timestamp drifted")
• States plainly that a displacing push flips isOutdated without touching a comment so both pins still match
• Names the mitigation as agent discipline (the pre-push-outdated rule in reference/orchestration.md)
• Points to Autonomous thread-resolve: pin pre-push isOutdated state (per-thread comment pins don't stop own-push displacement) #571 for the machine-enforced fix

This is technically precise and complete for a module docstring.

SKILL.md tool description (SKILL.md:213–220)

The old text said the worker "requires the thread to have been outdated in the PRE-push snapshot, pinned via --expected-comment-count and --expected-last-updated" — this implied a pre-push-outdated machine check that doesn't exist. The new text correctly says those pins enforce comment-state only, names the displacement bypass, and points to #571. ✓

SKILL.md step 7 (SKILL.md:405–410)

The old text concluded with "so a worker cannot clear a thread its own push merely displaced" — the exact false guarantee the control-tower flagged. The new text replaces this with an accurate description of what the bulk-resolve refusal actually prevents (missing comment-state pins) versus what it doesn't (displacement). ✓

Minor cosmetic observation (not blocking): The step-7 sentence ending "tracked in #571" is immediately followed by "In autopilot, after addressing..." without a paragraph break. The transition is slightly abrupt — "In autopilot" starts a conceptually separate topic. This pre-existed; the new commit inserted the displacement caveat inline in the existing paragraph. Not inaccurate, just a readability note for a future prose pass.

CHANGELOG.md (CHANGELOG.md:16–21)

The "fail-closed by construction" overclaim is gone. The new language correctly scopes the fix: "the per-thread pins now close the bulk and comment-drift gaps. They do not close the displacement bypass." This is accurate and appropriately scoped for a changelog. ✓

---

Consistency check

All four locations say the same thing about the displacement bypass and all four point to #571. The language is consistent. Cross-references to reference/orchestration.md for the pre-push-outdated agent-discipline rule are preserved where they were and are not added where they weren't (no scope creep).

---

Overall

The new commit precisely addresses the control-tower HOLD — no false guarantees remain in the documentation, the displacement gap is accurately characterized in all affected locations, and the #571 cross-reference is consistent. The security-correctness fix from 37c7a02 (guard logic + tests) remains intact and correct as reviewed in the previous pass.

HOLD is lifted. Ready to merge.

[kyle-sexton]

HOLD addressed — displacement-bypass caveat applied (docs-only)

Per the control-tower HOLD, the present-tense guarantee that a worker cannot clear a thread its own push merely displaced was false as shipped: the per-thread pins (--expected-comment-count, --expected-last-updated) cover comment-state only, so a displacing push flips isOutdated while both pins still match and a pinned autonomous resolve clears an unaddressed finding. Verified against babysit_merge.py unresolved_threads() and the resolve classify() gate (babysit_resolve_thread.py:194-197), which requires isOutdated but never checks pre-push outdatedness.

Corrected the language to describe only enforced behavior (pins prevent comment-state races) and to state plainly that the displacement bypass remains open — it rests on the pre-push-outdated agent-discipline rule (reference/orchestration.md), not machine enforcement — in commit b9abec0:

• babysit_resolve_thread.py module docstring (was L26-33)
• SKILL.md thread-resolution tool description (was L213-216)
• SKILL.md operational-runbook step 7 (was ~L401-402)
• CHANGELOG.md 0.10.0 entry — dropped the "fail-closed by construction" overclaim (grep turned this up as a fourth restatement of the same guarantee; the reference/orchestration.md / reference/safety.md framings were already correct, phrasing it as agent discipline rather than a tool guarantee, so they were left as-is)

No babysit_merge.py logic touched; the guarantees that are actually enforced (bulk autonomous resolve refused, --allow-unpinned-thread refused in autonomous mode, not-outdated threads skipped, comment-count/last-updated race guards) are unchanged. Ruff, the 11 test_guards regression tests, and markdownlint all pass.

#571 is now linked in the PR's ## Related section as the tracked root fix for the machine-enforced displacement guard.