Skip to content

fix(pr-merge): read the post-merge answer afresh in lane_room on any reuse refusal - #10154

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/room-shared-refusal-reads-afresh
Oct 26, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
fix/room-shared-refusal-reads-afresh

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: found while diagnosing the merge-lane stall in pr-merge run 40255059710.

Summary

No lane pull request merged after 23:46Z on 2026-10-25. Scheduled pr-merge run 40255059710 logged lane room: {"refusal":null,"unreadable":"post-merge answer demotion-changed",...}. It left 12 lane pull requests unarmed and disarmed #10100 and #10116.

babysit_merge_policy.lane_room read the run's shared post-merge answer again only when shared_refusal returned unverified-stale. Every other refusal (stale, a future computedAt, demotion-changed, new-merge:#<n>, binding) made the room unreadable. The gate (assess) reads afresh on any refusal, as the shared_refusal docstring says.

What triggered it: the answer was computed at 01:36:59Z. #10149 was opened by melodic-automation-lanes[bot] at 01:40:01Z, and triage then labeled and commented on it (01:40:26Z to 01:42:28Z). _demotion_changed counts any issue the agent identity opened whose updated_at differs from created_at, so it returned True. The issue's body has no demotion marker.

Fix

  • lane_room: when post_merge is given, any shared_refusal result now goes through _recompute_shared, the same full read the unverified-stale path already used. That read includes read_demotion, so a real demotion still holds the rung off. The cheap _still_current keep-path is still limited to unverified-stale, so a stale answer is never kept unchanged. The recompute uses the room's own self logins (none), as the room binds them.
  • With no post_merge, or a recompute that raises or comes back unreadable, the room stays unreadable as before.
  • _demotion_changed is unchanged. It is a lane safety control, and narrowing it is the owner's decision. One effect of this PR: whenever the issue lane files an issue and triage touches it shortly before a room read, the room pays one full post-merge check. Run 40255059710's post-merge step took 4m56s (01:36:59Z to 01:41:55Z), inside LANE_ROOM_TIMEOUT_MS (8 minutes).
  • Updated the docstrings, the enable-auto-merge.mjs header and docs/conventions/pr-pipeline/README.md to match. Added changelog fragments for source-control and pr-pipeline.

Verification

  • Two existing tests in test_babysit_lane_room.py encoded fix(pr-merge): recompute an unverified-stale post-merge answer in lane_room and annotate room holds #9602's narrower behavior: test_an_answer_the_gate_would_not_reuse_is_unreadable and test_a_lane_merge_the_answer_does_not_count_is_unreadable. Both asserted that no recompute ran. They are rewritten to assert the recompute. fix(pr-merge): recompute an unverified-stale post-merge answer in lane_room and annotate room holds #9602's body only says "every other refusal behaves as before"; it gives no reason those refusals had to stay unreadable.
  • New tests, run against origin/main's babysit_merge_policy.py: 6 fail (stale, future, demotion-changed, new-merge, cheap path not taken for stale, fresh demotion holds rung off). With this change all 27 lane-room tests pass.
  • test_an_unusable_answer_with_no_fresh_read_is_unreadable covers no check, a check that raises, and an unreadable check. It passes both before and after, since that behavior is kept.
  • Full babysit-prs Python suite: 2958 tests OK. enable-auto-merge.test.mjs and workflow.test.mjs: 133 pass, 0 fail. Ruff check is clean on the changed files. The pre-push check-affected passed.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 25, 2026 22:37
…reuse refusal

lane_room read the run's shared post-merge answer again only when
shared_refusal returned unverified-stale. Every other refusal (stale,
future, demotion-changed, new-merge, binding) made the room unreadable,
so enable-auto-merge left every lane pull request unarmed and disarmed
the armed ones. The gate itself reads afresh on any refusal.

Run 40255059710 hit this on demotion-changed: issue #10149, filed by
the lanes App after the answer was computed and then labeled by triage,
has updated_at != created_at, which _demotion_changed counts.

The room now recomputes on any refusal when it has post_merge, keeping
the cheap _still_current path for unverified-stale only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 26, 2026 02:53
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T02:55:26.327115Z 2354cbd Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown
Contributor

claude-review reviewed 2354cbd617a73680a6103088b22bf4b473f0f28e: 0 finding(s), 0 as inline comments.

I found nothing to block or flag in this pull request. I read the whole diff and the PR context, and I read lane_room, _recompute_shared and shared_refusal in babysit_merge_policy.py. I ran no tests, so the PR's test counts (2958 Python tests, 133 mjs tests, the 6 new failing-then-passing tests) are author-claimed and unverified.

  • Logic: lane_room now reads the post-merge answer again on any shared_refusal, as assess does. The cheap _still_current keep-path stays limited to unverified-stale.
  • Self logins: The recompute now passes frozenset() instead of shared.self_logins. This matches the self_logins=frozenset() that shared_refusal is called with in the same function. It also covers the new binding refusals, self-logins-differ among them. Before this change the unverified-stale path could only be reached when shared.self_logins was already empty, so nothing changes there.
  • Demotion: The recompute goes through read_demotion, so a real demotion still holds the rung off. The new test test_a_demotion_read_again_holds_the_room_off covers it.
  • Failure handling: With no post_merge, or a recompute that raises or comes back unreadable, the room stays unreadable. The new test test_an_unusable_answer_with_no_fresh_read_is_unreadable covers it.
  • Tests: The two old tests that asserted "no recompute" are rewritten to assert the recompute. New tests cover the cheap path not being taken for a stale answer. The new test_an_unusable_answer_is_never_kept_on_the_cheap_path asserts runs_unchanged is never called.
  • Changelog: Both fragments use bump: patch, which fits a bug fix.
  • Manifests: The diff touches no plugin.json or marketplace.json, so no manifest <field> lines apply.
  • Closing issue: The PR names no closing issue, so there are no criteria to check.
  • Cost: The PR notes one extra full post-merge check (about 5 minutes, inside the 8-minute LANE_ROOM_TIMEOUT_MS) when a refusal triggers a recompute. It uses CI time only, with no billing change and no hold.
  • Other lanes: None of the changed files match the trusted paths. I made no security finding, and no paved-path or rule-file criteria applied that I checked.

@github-actions

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 2354cbd617a73680a6103088b22bf4b473f0f28e: 0 finding(s), 0 as inline comments.

No security issues found in PR #10154 (head 2354cbd). critical-findings: 0

I read the whole diff (7 files) and the PR context. lane_room in babysit_merge_policy.py is the only code change. The rest is tests, docstrings, the enable-auto-merge.mjs header, the pr-pipeline README and two changelog fragments. No changed file matches a trusted path. No plugin manifest changed, so there are no manifest <field> lines. The diff deletes no standing instruction, so the instruction-surface lens did not apply. I did not read docs/conventions/instruction-exception-register/README.md. Nothing in the diff removes a guardrail.

What changed: any shared_refusal result, where before only unverified-stale qualified, now makes the room read the post-merge answer again with _recompute_shared. That covers stale, a future computedAt, demotion-changed, new-merge:#<n> and the binding refusals. The _still_current keep-path is still limited to unverified-stale.

Why this does not fail open:

  • Demotion and failures still hold. The fresh read goes through read_demotion and the post-merge gate. shared.gate.refusal (post-merge:<refusal>) and the effective_rung(demoted=…) check (rung-off:<input>) both still run on the fresh answer, so a real demotion or a newly failed run still stops arming. The new test test_a_demotion_read_again_holds_the_room_off covers this.
  • Failed reads stay unreadable. With no post_merge, a check that raises, or an unreadable result, the room returns unreadable, which does not arm and disarms as before.
  • The stale answer is never kept unchanged. The cheap keep-path is guarded by unusable == "unverified-stale". A test asserts runs_unchanged is not called for a stale answer.
  • Self logins match the gate. The fresh read now uses frozenset() instead of shared.self_logins. That is what shared_refusal already compares against and what the gate binds a candidate with, so the demotion read has the same view as the gate. The empty set does not exclude any lane or bot account from the demotion read, which is the stricter direction.
  • Identity is the config's own. The fresh answer takes its identity from the default-branch config, not from the stale report, so a report bound to another identity cannot pass through.

Residual note (not a finding): a recompute costs a full post-merge check, about 5 minutes in the cited run, against the 8-minute LANE_ROOM_TIMEOUT_MS. A timeout ends as unreadable, which is the safe direction.

Spend: this runs one extra post-merge check inside the existing lane run, which draws only on existing CI usage. It is not a cost hold.

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.

1 participant