Autonomous thread-resolve: pin pre-push isOutdated state (per-thread comment pins don't stop own-push displacement)

[kyle-sexton]

Summary

Follow-up from Codex P1 on #561 (thread on plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py). The per-thread pins added in #561 (--expected-comment-count, --expected-last-updated) close the bulk-resolve and comment-drift gaps, but they do not close the specific hole the PR set out to close: an autonomous worker clearing a bot thread that its own push merely displaced (not addressed).

The gap

--expected-comment-count and --expected-last-updated pin the comment count and latest comment-edit timestamp. A code push that displaces a finding's anchored lines flips GitHub's isOutdated to true without adding a reply or editing a comment — so both pins still match. Under --autonomous, classify() requires isOutdated == true and accepts the now-outdated thread; the unchanged comment pins match; the resolve succeeds. The finding is cleared even though the worker's push only displaced it.

This falsifies the property the PR docstring (lines 26–33) and SKILL.md step 7 (~L398–402) currently claim: "a worker cannot clear a thread its own push merely displaced."

Why it is not a trivial patch

isOutdated == true because my push addressed the finding and isOutdated == true because my push displaced the finding are deterministically indistinguishable at the post-push head: same flag, same unchanged comments. A post-push --expected-outdated pin is inert (it reads true in both the legit and attack cases). The only deterministic discriminator is: resolve only threads that were outdated at the pre-push head — i.e. add --expected-head and require the resolve to run while the live head still equals the vetted pre-push head, moving thread resolution to before the worker's push (an active thread is then refused naturally by classify()'s isOutdated gate).

The posture decision this forces (why it is deferred, not auto-fixed)

The merge gate (babysit_merge.py:253) blocks on any unresolved review thread regardless of isOutdated. Under the pre-push-head fix, a thread that the worker's own fix genuinely addressed only becomes outdated after the push, so it can no longer be autonomously resolved — it will block the merge gate and require attended resolution. That is a defensible security posture (an actor must not clear findings on the strength of its own unverified push), but it materially reduces autonomous-merge reach for the common case (worker fixes a finding → that finding's thread blocks the gate). Choosing that trade-off — vs. a narrower discriminator (e.g. gating on the reviewing bot's re-review/dismissal signal rather than raw isOutdated), vs. accepting the residual risk — is a design/policy call that should be made deliberately, not bundled into a review-response pass on #561.

Note: no deterministic guard defeats an actively malicious worker that reorders (push, then pin the post-push head, then resolve); isOutdated-based distinguishability is impossible there. The realistic goal is making the honest documented flow fail-closed.

Proposed work

1. Add --expected-head to babysit_resolve_thread.py; require live PR head == pinned head for --autonomous --resolve (mirroring babysit_merge.py's --expected-head, but anchored to the pre-push head).
2. Rewrite SKILL.md step 7 to resolve pre-push-outdated bot threads before the worker's push, pinning the pre-push head.
3. Correct the docstring (L26–33) so the stated guarantee matches what the guards deliver.
4. Decide and document the merge-gate posture for own-push-addressed threads (attended resolution vs. an alternative discriminator).
5. Add a test mirroring the attack trace: not-outdated-at-snapshot → refused.

Source: #561 (comment)

[kyle-sexton]

This was generated by AI during triage.

Triage — T3, needs-human, wayfind: design.

Genuine security-posture policy call, not a single maintainer-vetoable default: the deterministic fix (pin the pre-push head, resolve only threads outdated before the worker's own push) is clear, but its consequence — a thread the worker's own fix genuinely addressed can no longer be autonomously resolved, since it only becomes outdated after the push — materially reduces autonomous-merge reach for the common case. Choosing between (1) accept that reach reduction (attended resolution for own-fix-addressed threads), (2) a narrower discriminator keyed on the reviewing bot's re-review/dismissal signal instead of raw isOutdated, or (3) accepting the residual risk, is a deliberate security/product tradeoff the issue itself explicitly asks to be made deliberately, not bundled into a review-response pass.

[kyle-sexton]

Decision brief — 2-minute read

Generated by AI. area: security posture call; the issue explicitly asks for it to be made deliberately, not bundled into a review-response pass.

Problem

Under --autonomous, the resolver treats GitHub's isOutdated == true as the "finding addressed" signal — but a worker's own push flips a thread to outdated by merely displacing the anchored lines, without touching any comment. So the per-thread comment pins from #561 (--expected-comment-count, --expected-last-updated) still match, and the thread auto-resolves with no proof the finding was fixed — an autonomous actor clearing a live finding on the strength of its own unverified push.

Today (verified)

• Resolve gate. babysit_resolve_thread.py → classify(): under --autonomous, a thread is only eligible when thread["isOutdated"] is true (skipped-not-outdated otherwise). The pins are comment-state only — project_thread() computes commentCount and lastCommentUpdatedAt from comments; neither reacts to a code push that displaces a finding. The module docstring already documents this exact gap and points here (Autonomous thread-resolve: pin pre-push isOutdated state (per-thread comment pins don't stop own-push displacement) #571) for the machine-enforced fix.
• Merge gate. babysit_merge.py → unresolved_threads() (fetched at ~L171; blocked at ~L256, the "{len(threads)} unresolved review thread(s)" refusal): the gate blocks on any unresolved thread and does not filter by isOutdated. This is why the fix reduces reach — see below.
• Discipline today. The pre-push-outdated rule is currently enforced only by agent discipline in the Worker Contract (reference/orchestration.md, ~L508–516) and reference/safety.md, not by the guard. General resolve policy (bot-authored, addressed, never-human, never-own) lives in reference/review-discipline.md §D7.5.

Why pinning the pre-push head reduces reach. The deterministic fix — resolve only threads that were isOutdated at the pre-push head (add --expected-head, run resolution before the worker's push) — is airtight against displacement. But a thread the worker's own fix genuinely addressed only becomes outdated after that push, so under the fix it can no longer be autonomously resolved. It stays unresolved, and the merge gate (which ignores isOutdated) then blocks the merge → attended resolution for the common case (worker fixes a finding → that finding's own thread now gates the merge).

External fact (grounds the mechanism)

GitHub GraphQL, live schema introspection: PullRequestReviewThread.isOutdated: Boolean! — "Indicates whether this thread was outdated by newer changes." Key word newer changes: the flag fires on any subsequent change, including the worker's own push. It carries no "addressed vs. merely displaced" distinction — that distinction is deterministically unrecoverable at the post-push head (same flag, same unchanged comments), which is what forces this to be a posture choice rather than a patch.

---

Options (recommendation first)

✅ RECOMMENDED — Option 1: pin the pre-push head; accept attended resolution for own-fix-addressed threads

Add --expected-head to babysit_resolve_thread.py (mirroring babysit_merge.py), require live head == pinned pre-push head, and move autonomous thread resolution to before the worker's push. A thread outdated only by the worker's own push is refused; a genuinely-addressed thread becomes outdated post-push and falls to attended resolution.

• Tradeoff: correct and fail-closed; costs autonomous-merge reach for the common own-fix case (those merges now need a human to resolve the thread).
• Reversibility: high. It is the safe floor — fully relaxable later if Option 2's reach-recovery lands, with no data migration or irreversible commitment.
• Why recommended (area: security): the failure it closes is an autonomous merge of a PR carrying an unaddressed finding. For a security-relevant autonomy gate, correctness/fail-closed outranks reach. It is immediately implementable, it makes the already-documented honest flow actually fail-closed, and it is reversible — so it dominates "accept the exposure."

Option 2 (deferred, composable with Option 1 — not an alternative): recover reach via the reviewing bot's own re-review/dismissal signal

After the push, instead of the worker self-resolving, trigger the reviewer to re-review and let the bot resolve/dismiss its own thread — the entity that raised the finding clears it, which is strictly safer than the worker clearing it. babysit-prs already ships the summon machinery (request_review.py, reference/review-trigger.md), so this layers on top of Option 1 rather than replacing it.

• Open question gating viability (needs a spike, do not assume clean): does the reviewing bot deterministically emit a thread resolution or dismissal on re-review? review-trigger.md today verifies completion from a submitted review, not from automatic thread resolution — so the discriminator may not exist off-the-shelf.
• Tradeoff: recovers the lost reach without the worker self-certifying; cost is dependence on an external bot's behavior and the spike to confirm the signal.
• Reversibility: high — additive on top of Option 1's floor; if the signal proves non-deterministic, drop it and keep Option 1.
• Recommendation: record as deferred reach-recovery with the trigger = "spike confirms the reviewing bot emits a deterministic resolve/dismiss on re-review."

Option 3 (not recommended): accept the residual risk — keep current behavior

Leave --autonomous keyed on raw isOutdated, understanding a worker can clear a finding its own push merely displaced.

• Tradeoff: maximum autonomous-merge reach; leaves a standing security exposure — autonomous merge of a PR with an unaddressed finding.
• Reversibility: high to adopt, but every autonomous merge in the meantime is an unbounded, un-auditable instance of the exposure.
• Why not: for an area: security autonomy gate, a known standing exposure with a clean fail-closed fix available is not a defensible resting state.

---

Note (from the issue, worth keeping)

No deterministic guard defeats an actively malicious worker that reorders (push → pin the post-push head → resolve); isOutdated-based distinguishability is impossible there. The realistic goal is making the honest documented flow fail-closed — which Option 1 achieves.

Cross-links

• fix(source-control): require per-thread pins for autonomous bulk babysit thread resolves #561 — the PR this follows up on (added the per-thread comment pins that this shows are insufficient against displacement).
• feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476 — autopilot merge tier; the autonomous-merge reach this decision constrains.
• Code/policy: babysit_resolve_thread.py classify(); babysit_merge.py unresolved_threads(); reference/orchestration.md Worker Contract (~L508–516); reference/safety.md; reference/review-discipline.md §D7.5.