fix(source-control): review-trigger head-staleness hardening (BLOCKED-behind guard, stale-head reactions)

[kyle-sexton]

Closes #327

Summary

Two head-staleness defects in the dormant-by-default AI review-trigger flow (both found by Codex on #322, P2). The module only activates when babysit_review_trigger_phrase + babysit_review_bot_logins + babysit_review_gate_context are configured, so neither affects the default babysit tiers.

F7 — reject compare-behind BLOCKED heads before requesting review

request_review.py's pre-POST freshness guard rejected only the literal BEHIND merge state. GitHub reports mergeStateStatus=BLOCKED for a PR that is also behind its base (BLOCKED masks BEHIND), so the guard passed and posted the one-shot trigger on a stale head. The guard now reuses compute_branch_freshness(current) — off the _blocked_base_compare enrichment view_pr already computes — so a compare-confirmed behind head is rejected and the branch-refresh flow runs first.

F8 — ignore stale-head reactions for new review windows

The candidate predicate's unconditional and not reactions blocked candidacy whenever any reaction existed. Reactions carry no commit SHA, so a reviewer-bot reaction left on an earlier head persisted onto later heads; the new head never accrued missing_observations and the trigger never posted for the updated head. The check is now scoped to associated_reactions (reactions associated with, or newly observed for, the current head — already computed nearby).

Scope note

The live post-time guard at request_review.py (if reaction_signals: raise) and the state-string branch at babysit_review_trigger.py remain conservative by design — they still refuse to post while any unproven reaction lingers. F8 restores the observation window (candidacy / missing_observations accrual) without weakening a fail-safe post guard. Fully unblocking end-to-end posting through a persistent stale reaction is a larger call related to #324 and is intentionally out of scope here.

Verification

• engine.test.sh locally: 184 unittest OK, ruff clean, guarded-wrapper checks pass.
• New regression tests in test_review_trigger_race.py: F7 (BLOCKED-masked-BEHIND rejected; BLOCKED-but-fresh still passes), F8 (stale earlier-head reaction no longer suppresses the window; current-head reaction still suppresses).
• source-control 0.9.0 → 0.9.1 + CHANGELOG.

Related

• feat(source-control): converge babysit-prs tiers + engine (0.8.0) #322 (PR-B), fix(source-control): reconcile review-trigger 'pending or failing' gate contract with pending-only code #324 (sibling review-trigger reconciliation — not pulled in; one PR per issue).

🤖 Generated with Claude Code

[claude]

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

---

PR Review

• Gather context from PR diff and changed files
• Review F7 fix — BLOCKED-masked-BEHIND guard in request_review.py
• Review F8 fix — stale-head reaction scoping in babysit_review_trigger.py
• Review new regression tests in test_review_trigger_race.py
• Check CHANGELOG and version bump
• Final summary

---

Overall

Clean, well-scoped fix for two real head-staleness defects. The logic is sound, the tests are targeted and faithful to the described failure modes, and the version bump is appropriate. A few observations below — none are blockers.

---

F7 — BLOCKED-masked-BEHIND guard (request_review.py)

Correct. compute_branch_freshness correctly falls back to _blocked_base_compare only when mergeStateStatus == "BLOCKED" and the compare API confirms behind_by > 0, so a PR that is BLOCKED purely for pending review (_blocked_base_compare absent or behind_by == 0) continues to pass the guard. The mutually-exclusive relationship between behind and mergeable == "CONFLICTING" (both trace through compute_branch_freshness) means the reason-string priority order in the revised block is safe.

One observation: The candidate predicate in classify_review_request still checks only merge_state not in {"", "BEHIND", ...} — it does not call compute_branch_freshness. This means a BLOCKED-but-behind PR can accrue observations and reach request_eligible = True, only to fail at validate_current_candidate. The post attempt is then recorded as "ambiguous" and blocks future retries until state is manually cleared. This is a pre-existing gap (not introduced by this PR), but it's now exposed as the realistic path through the code. Worth a follow-on issue if the intent is to keep the candidate predicate and the post guard in sync.

---

F8 — stale-head reaction scoping (babysit_review_trigger.py)

Correct. The associated_reactions computation is precisely specified: reactions are associated with the current head only if they arrived while reaction_head_sha == head_sha was already recorded in state (the "new reactions since last cycle" path) or they sit on a trigger comment attributed to this head's request/attempt history. A reaction left on a prior head's state record cannot satisfy either condition, so it cannot suppress the observation window.

State semantics note: After F8, it is valid to observe state == "engagement_signal_unverified" simultaneously with missing_head_sha == HEAD and missing_observations > 0. This was impossible before (any reaction blocked candidate), so any downstream consumer of classify_review_request's return dict that branches on state alone — rather than consulting missing_head_sha or request_eligible — may misread the new combined state as "not observing". The PR description flags this as intentional and limited to the #324 follow-on; fine for now, but worth auditing call-sites when #324 lands.

---

Tests (test_review_trigger_race.py)

Sound. The two F8 tests correctly model the key invariants:

• test_stale_earlier_head_reaction_does_not_suppress_new_window: passes empty prior ({}) — no reaction_head_sha match — so associated_reaction_ids starts and ends empty. Stale reaction doesn't block. ✓
• test_current_head_reaction_still_suppresses_candidacy: prior has reaction_head_sha == HEAD with empty reaction_signal_ids, so the new reaction falls into the "new since last cycle" branch and enters associated_reaction_ids. Blocks candidacy. ✓

The F7 tests correctly exercise the two sides of the freshness gate: the _blocked_base_compare path (rejected with "BEHIND") and the absent-compare path (passes through to return pr).

Minor: test_blocked_but_fresh_head_passes_the_freshness_guard applies all six patches via explicit tuple indexing (patches[0], patches[1], ...). An ExitStack or contextlib.nullcontext chain would scale better as the patch set grows, but this is readable at the current size.

Gap worth considering: There is no test for a stale reaction scoped to "trigger_comment" whose comment_id is NOT in known_trigger_comment_ids for the current head. That path correctly falls through to "not associated" (same outcome as the "pull_request" stale test), but explicit coverage would guard against a future refactor that accidentally broadens the trigger-comment inclusion logic.

---

CHANGELOG / version

0.9.0 → 0.9.1 is correct for a bug-fix-only release. CHANGELOG entries are clear and accurate.

---

Summary

Both fixes are correct and the tests exercise the right invariants. The two items to track going forward: (1) align the candidate predicate in classify_review_request with the post guard's freshness check so a BLOCKED-behind PR can't accumulate observations that produce an unresolvable ambiguous attempt record (pre-existing gap, now on the hot path); (2) audit call-sites of classify_review_request for state-only branching before #324 lands. Neither is a reason to hold this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 24e8f7ac9e

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

[claude]

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

---

PR Review — Updated for commit b1a9315

• Identify new commit (b1a9315) since last review
• Read changed files — babysit_review_trigger.py, request_review.py, test_review_trigger_race.py
• Review F8 scoping propagation to posting guard (new commit)
• Check Codex P2 finding is addressed
• Final summary

---

What's new in b1a9315

This commit addresses the Codex P2 finding from the prior review: request_review.validate_current_candidate was still gating on the raw fetch_reaction_signals result, meaning a PR made eligible by the F8 fix (stale reaction from an earlier head) would still have every posting attempt rejected as "ambiguous". The scoping logic is extracted into a reusable resolve_associated_reactions helper and applied at every reaction-gating site.

---

resolve_associated_reactions extraction (babysit_review_trigger.py:302–351)

Correct and safe. The helper is semantically identical to the prior inline block in classify_review_request, with one change worth noting: the old inline code iterated over the already-extracted local request_history and attempt_history variables; the new helper re-reads prior.get("request_history") and prior.get("request_attempt_history") directly. These are behaviorally equivalent — json_object empty-defaults on missing keys in both paths, and neither path mutates the dicts before the lookup.

The docstring is explicit about the contract: every caller that gates on reaction signals must go through this helper, not the raw list. That's the right invariant to document given the three-site scatter.

One cosmetic note: reaction_ids at classify_review_request:390 is still computed and used for state recording (reaction_signal_ids at line 494) — not dead code, just looks orphaned at first glance since it no longer feeds into resolve_associated_reactions.

---

request_review.py — three application sites

All correct.

1. Pre-POST guard (validate_current_candidate:133–135, L147): if reaction_signals: → if associated_reactions:. The review_trigger dict passed in from run_locked is already the inner sub-dict (json_object(pr_state.get("review_trigger")), with histories merged from the ledger), which matches the shape resolve_associated_reactions expects. Call-site consistent.

2. Pre-POST guard (second call, inside the apply lock) (L435): uses the refreshed review_trigger re-extracted at line 408. Correct.

3. Post-POST unexpected_reactions check (L550–558): resolve_associated_reactions(post_reactions, expected_head_sha, review_trigger) before the not trigger_comment filter. This is exactly the right fix — the post-POST check was independently subject to the same staleness bug, and the inline comment explains why.

validate_current_candidate now has a required 6th parameter; all call sites inside request_review.py and in the tests pass it. The same-named function in refresh_pr_branch.py is independent (module-local) and unaffected.

---

Tests (test_review_trigger_race.py)

Sound. New test classes added in this commit:

• ValidateCurrentCandidateReactionScopingTests — tests the pre-POST posting guard directly: stale earlier-head reaction passes (no reaction_head_sha match → empty associated_reactions); current-head reaction blocks (new since last snapshot). Both paths verified against the actual function signature with the 6th arg.

• ResolveAssociatedReactionsTests — four unit tests for the helper itself: stale-different-head, new-since-last-snapshot, already-seen-for-this-head, and trigger-comment attribution. These cover the primary association paths cleanly.

One gap: There is no dedicated test for the post-POST unexpected_reactions scoping (the third application site at line 550). It's structurally identical to the pre-POST case and covered implicitly by ResolveAssociatedReactionsTests, but an explicit test that exercises the post-POST branch (verifying a stale reaction on post_reactions is excluded from unexpected_reactions) would close the coverage delta if the logic is ever refactored. Not a blocker at current scope.

---

CHANGELOG

Already updated at lines 23–29 with an accurate description of the follow-on fix. No version bump needed beyond 0.9.1.

---

Summary

The Codex P2 finding is fully addressed. resolve_associated_reactions is correctly extracted, exported, and applied at all three reaction-gating sites in request_review.py. The extraction is semantically faithful to the original inline code. Tests cover the key paths for the helper and the pre-POST guard. This commit completes the F8 fix end-to-end. No further blockers.

[kyle-sexton]

Classification of the claude[bot] review's observations (no reactions endpoint on review bodies — exempt per review-discipline §3 D4.5):

#	Finding	Classification	Evidence
1	F7: candidate predicate in classify_review_request doesn't call compute_branch_freshness, so a BLOCKED-but-behind PR can accrue observations then fail at validate_current_candidate, recording an unresolvable "ambiguous" attempt.	VALID (defer)	Confirmed pre-existing (not introduced by this PR) by reading classify_review_request's candidate predicate — it checks only merge_state not in {...}. Reviewer explicitly flags it as "worth a follow-on issue," not a blocker for this PR. Deferring to a tracked follow-up rather than expanding this PR's scope.
2	F8: after the fix, state == "engagement_signal_unverified" can coexist with missing_head_sha == HEAD and missing_observations > 0; a state-only downstream consumer could misread this as "not observing."	VALID (defer)	Confirmed by reading classify_review_request's state derivation — the combination is now reachable where it previously wasn't. Reviewer notes the PR description already flags this as intentional, scoped to the #324 follow-on, and explicitly "fine for now." No call-site in this repo currently branches on state alone for this case (checked babysit_delta.py and pr_queue_snapshot.py), so no in-repo fix is required now.
3	Tests: test_blocked_but_fresh_head_passes_the_freshness_guard applies 6 patches via explicit tuple indexing; an ExitStack would scale better.	VALID (defer)	Confirmed — cosmetic only, reviewer calls it "readable at the current size," not a defect. Left as-is; the new tests added in this follow-up commit use the same established pattern in this file for consistency.
4	Tests: no explicit coverage for a stale reaction scoped to "trigger_comment" whose comment_id is NOT in known_trigger_comment_ids.	VALID (defer)	Confirmed as a real coverage gap distinct from the fix in this follow-up. Deferring — it guards against a future refactor regression, not a defect in the current code path (the "pull_request"-scope stale test already exercises the same fall-through outcome).

None of these are blockers per the reviewer's own summary ("Neither is a reason to hold this PR"). Separately, this follow-up commit (b1a9315) fixes a related, more severe live defect flagged by @chatgpt-codex-connector's P2 (reaction scoping not propagated to the posting guard) — see the threaded reply on that finding for details.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.