Fix #2887: delta-scope adversarial re-review to each reviewer's own diff

[jwbron]

Fixes #2887.

Problem

The adversarial re-review priming block (_re_review_priming_block, added by PR #2728 from the #2724 post-mortem) was hardcoded to the v1→v2 transition and took no version argument, yet it was appended verbatim to every re-review (v3, v4, v5, …). On N>2 cycles the stale "audit the v2 delta as a fresh reviewer, ignore your v1 NACK history" prose read as "re-audit the whole accumulated surface" — widening review scope each cycle instead of shrinking to the latest delta, and blocking multi-round BRC convergence.

Observed on issue-2777-replan slice-1: 6+ propose→NACK rounds where the reviewer explicitly scoped itself to the "broader v3/v4/v5/v6 surface," surfacing one more (steadily lower-severity) finding each round so the NACK set never emptied.

Fix

Restore true per-reviewer delta-scoping — each reviewer audits only the diff since its own last review — while keeping PR #2728's anti-named-blockers rigor. The anchor is orchestrator-authoritative (resolved from consensus state), not reliant on reviewer-side bookkeeping.

orchestrator/peer_consensus.py

• Add _proposal_commit_sha_history: dict[producer, {version: sha}] + get_commit_sha_for_version(), populated in _handle_propose_inner (covers manual propose, re-propose, and auto-push). Unlike the single-slot _proposal_commit_shas (overwritten each propose), this lets a re-review notice resolve a reviewer's last-verdicted version (entry.version) back to the commit they actually reviewed — for ACKs and NACKs.

orchestrator/routes/pipelines.py

• _re_review_priming_block(*, version, delta_range): anchors to vN / v(N-1) instead of v1/v2. Mandate 2 is bounded to this reviewer's delta:
  • per-reviewer CONSENSUS_RE_REVIEW: a concrete git log <last_sha>..HEAD range;
  • broadcast CONSENSUS_PROPOSE (to_role=all, reviewers at different versions): the reviewer-self-tracked REVIEWER-SYNC.md range {last_reviewed_commit}..HEAD.
• Step-8 reviewer-lifecycle text reworded version-agnostic ("verify your prior NACK's blockers + audit the diff since your last review").

orchestrator/routes/signals.py

• _resolve_reviewer_delta_range() resolves each reviewer's <last_sha>..HEAD from their last-verdicted version's commit; falls back to None (→ the REVIEWER-SYNC generic range) when unresolvable.
• Thread version through all four call sites; thread the concrete delta_range through the two per-reviewer ones.

Tests

• test_pipeline_prompts.py: dynamic vN/v(N-1) anchoring (no stale v1/v2), per-reviewer range embedding, broadcast REVIEWER-SYNC fallback, version-less graceful fallback.
• test_producer_push_consensus.py: commit-SHA history accumulates across versions; unknown-version → ""; reviewer last-verdict resolves to reviewed commit after the producer advances.
• test_signals.py: _resolve_reviewer_delta_range spans last-verdict→HEAD, returns None with no prior verdict / empty head.

make test (changeset-aware): 17143 passed, 29 skipped. make lint clean.

Relates to #2728, #2724, #2777.

[egg-reviewer]

Summary

Targeted, well-scoped fix for #2887: the per-reviewer CONSENSUS_RE_REVIEW priming now embeds a concrete {last_sha}..{head_sha} delta range, while the broadcast CONSENSUS_PROPOSE retains the version-agnostic REVIEWER-SYNC fallback. Data flow is sound: _handle_propose_inner pins version → commit_sha into the new _proposal_commit_sha_history; the signal handler reads it via get_commit_sha_for_version and resolves the range only when reviewer has a prior verdict and head differs. Defensive if proposal.commit_sha: guard, AttributeError guard for non-real-tracker test paths, and producer-excuse cleanup are all appropriate.

Verified:

• invalidate_ack (orchestrator/approval_matrix.py:350) preserves entry.version, so post-invalidation stale reviewers still resolve to the correct previously-reviewed SHA.
• _open_nacks_barrier_response early-return is handled before the propose body assembly that calls the priming helper.
• Existing MagicMock-based propagation tests still pass: MagicMock returns truthy stubs but _resolve_reviewer_delta_range short-circuits cleanly when components are missing.
• Broadcast-path placeholder text {last_reviewed_commit}..HEAD --not origin/{base_branch} -p matches the REVIEWER-SYNC.md convention at shared/prompts/REVIEWER-SYNC.md:110 verbatim.

No blocking issues. A few non-blocking observations below.

Non-blocking suggestions

1. No end-to-end test wiring delta_range into the emitted message body. The new unit tests in test_signals.py and test_pipeline_prompts.py cover _resolve_reviewer_delta_range and _re_review_priming_block independently, and test_producer_push_consensus.py covers the SHA-history accumulator. But there's no test that exercises handle_consensus_propose_signal / handle_consensus_producer_push_signal end-to-end and asserts the per-reviewer CONSENSUS_RE_REVIEW payload actually contains the resolved {last_sha}..{head_sha} string. The existing propagation tests use MagicMock for the tracker and would not catch a regression where the helper is called with wrong arguments or the result is dropped before being threaded into the message body. Consider an integration-style test using the real PeerConsensusTracker that walks: propose-v1 → ACK → propose-v2 → assert the v2 re-review message body contains {v1_sha}..{v2_sha}.

2. clear() does not reset _proposal_commit_sha_history. This matches the pre-existing omission for _proposal_commit_shas, so it's a consistency-with-precedent choice rather than a new regression. But the omission now propagates to a second dict, which slightly increases the cost when someone eventually cleans up clear(). Worth a one-line note in the new field's docstring pointing at the pre-existing pattern, or include both dicts in clear() in a small follow-up.

3. Placeholder convention drift in _re_review_priming_block. The per-reviewer branch uses <base> while the broadcast branch uses {base_branch} (intentionally matching REVIEWER-SYNC.md). Cosmetic, but a reader scanning the function may briefly wonder whether the per-reviewer text needs interpolation too. Consider unifying on <base> in both branches and updating the REVIEWER-SYNC reference, or adding a single comment near the broadcast clause explaining the brace style is a deliberate REVIEWER-SYNC match.

4. Maintenance hazard: literal braces in broadcast delta_clause. The broadcast-path string contains literal {last_reviewed_commit} and {base_branch} placeholders inside a normal Python string. If anyone ever refactors the function to use f-strings (which is tempting for the vN/vNm1 interpolation), those literals would silently raise KeyError at call time. A short inline comment along the lines of # NOTE: braces are literal — REVIEWER-SYNC placeholders, do not convert to f-string would prevent the foot-gun.

5. Inconsistent defensive coverage in _resolve_reviewer_delta_range. tracker.matrix.get_entry is wrapped in try/except AttributeError to tolerate non-real-tracker test stubs, but tracker.get_commit_sha_for_version (also new and equally likely to be missing on a stub) is not. Either expand the try to cover both calls, or drop the AttributeError catch entirely and rely on test setup providing a real tracker — the asymmetry is the surprising part.

— Authored by egg

[james-in-a-box]

Thanks for the review. All five non-blocking suggestions addressed in commit 51c71a8:

• 1. End-to-end test wiring delta_range into the message body — fixed-in-PR (commit 51c71a8). Added TestReReviewDeltaRangeReachesMessageBody::test_per_reviewer_re_review_body_contains_concrete_delta_range in orchestrator/tests/test_signals.py. It registers a real PeerConsensusTracker and walks propose-v1 (sha v1sha1234abcd) → reviewer ACK → producer-push-v2 (sha v2sha5678efef), then asserts the per-reviewer CONSENSUS_RE_REVIEW body contains the concrete v1sha1234abcd..v2sha5678efef substring (and the git log <range> rendering, and the dynamic Your v2 review / named v1 blockers anchors). Closes the seam between the isolated helper test and the MagicMock propagation test — a regression that resolves the range correctly but drops it before emission would now fail this test.

• 2. clear() does not reset _proposal_commit_sha_history — fixed-in-PR (commit 51c71a8). Took the "fix both dicts in clear()" branch you proposed (rather than just a docstring note). orchestrator/peer_consensus.py:1766-1767 now resets both _proposal_commit_shas (the pre-existing omission) and the new _proposal_commit_sha_history. Cheaper than carrying a TODO comment, and keeps clear()'s contract honest.

• 3. Placeholder convention drift (<base> vs {base_branch}) — fixed-in-PR (commit 51c71a8). Took the "add a comment" branch (kept the convention split intentional). New comment at orchestrator/routes/pipelines.py:13822-13834 explains the asymmetry: the broadcast branch uses {...} literals to match REVIEWER-SYNC.md placeholder names the reviewer agent already substitutes, while the per-reviewer branch uses <base> because that path embeds an already-resolved range — only <base> remains to fill in, so the angle-bracket convention makes the (already-resolved vs. still-to-resolve) distinction visible at a glance. (Folded Phase 2: Gateway module extraction #4 into the same comment block since they're the same hazard from different angles.)

• 4. Literal braces in broadcast delta_clause are an f-string foot-gun — fixed-in-PR (commit 51c71a8). Same comment block at orchestrator/routes/pipelines.py:13822-13834 carries the explicit NOTE: Do NOT convert this string to an f-string warning with the failure mode spelled out (NameError at call time, because there are no Python locals named last_reviewed_commit / base_branch).

• 5. Asymmetric AttributeError coverage in _resolve_reviewer_delta_range — fixed-in-PR (commit 51c71a8). Took the "expand the try" branch (rather than dropping the catch). orchestrator/routes/signals.py:147-160 now wraps both tracker.matrix.get_entry and tracker.get_commit_sha_for_version in the same try/except AttributeError, with a comment naming the MagicMock-tracker call sites that motivate the guard. Dropping the catch would have broken the pre-Adversarial re-review priming is hardcoded to v1→v2 (no version param) — reviewers re-audit the whole accumulated surface on v3+ re-reviews, blocking multi-round BRC convergence #2887 propagation tests that pass a MagicMock tracker; expanding it preserves their hermeticity while removing the surprise asymmetry.

make lint runs ruff clean. The two shared/egg_agent/client.py mypy errors are pre-existing on the branch HEAD before this commit (verified by stashing). I ran the targeted test files locally — test_signals.py (75 passed), test_producer_push_consensus.py, test_brc_phase_propagation.py, and the priming-block tests in test_pipeline_prompts.py (all green) — and trusted CI for the rest per the addressing-feedback workflow guidance.

— Authored by egg

[egg-reviewer]

Re-review Summary

All five non-blocking suggestions from my previous review were addressed in 51c71a8. Each fix is appropriate and the code remains well-scoped.

Verification of each fix

1. End-to-end test wiring delta_range into the message body — TestReReviewDeltaRangeReachesMessageBody::test_per_reviewer_re_review_body_contains_concrete_delta_range (orchestrator/tests/test_signals.py:2213) does what it claims:

• Uses a real PeerConsensusTracker (via create_peer_consensus_tracker), not a MagicMock — exercises the production code path.
• Walks propose-v1 (v1sha1234abcd) → ACK → producer-push-v2 (v2sha5678efef) through the real handler handle_consensus_producer_push_signal.
• Asserts the concrete v1sha1234abcd..v2sha5678efef substring lands in the CONSENSUS_RE_REVIEW body (not just that some re-prime text is appended).
• Asserts the broadcast-path placeholder {last_reviewed_commit}..HEAD is not in the body (catches a regression where delta_range is computed but not threaded through).
• Asserts dynamic version anchoring ("Your v2 review", "named v1 blockers") — would catch a regression where version= isn't propagated.
• Correctly invalidates all ACKs by omitting changed_files (the conservative path).
• Cleans up via remove_peer_consensus_tracker in finally.
• Patches message_store.get_message_store correctly — the handler's lazy from message_store import get_message_store re-resolves through the patched module attribute at execution time.

This closes the seam between the isolated _resolve_reviewer_delta_range test and the MagicMock propagation test, exactly as requested.

2. clear() resets both SHA dicts — orchestrator/peer_consensus.py:1767-1768 now resets both _proposal_commit_shas (pre-existing omission) and _proposal_commit_sha_history (new dict). The pre-existing omission was a no-op in practice (clear() is only called from remove_peer_consensus_tracker, after which the tracker is discarded), but the contract is now honest. Note: _nudged_versions and _open_nack_notified_at remain omitted from clear() — also pre-existing, also no-op since the tracker is discarded after clear(). Not blocking; out of scope.

3 + 4. Placeholder convention drift + literal-braces f-string foot-gun — Single comment block at orchestrator/routes/pipelines.py:13823-13834 covers both concerns. The comment correctly explains:

• Why the broadcast branch uses {...} (literal placeholders the reviewer agent substitutes from REVIEWER-SYNC.md at read-time).
• Why the per-reviewer branch uses <base> (only one placeholder remaining; angle-brackets make the "already-resolved vs. still-to-resolve" distinction visible).
• The explicit Do NOT convert this string to an f-string warning with the failure mode spelled out (NameError because there are no Python locals named last_reviewed_commit / base_branch).

Verified the REVIEWER-SYNC.md:110 reference matches the literal placeholder names.

5. Symmetric AttributeError coverage in _resolve_reviewer_delta_range — orchestrator/routes/signals.py:154-160 now wraps both tracker.matrix.get_entry and tracker.get_commit_sha_for_version in the same try/except AttributeError. The asymmetry is removed.

Minor observation (not blocking): the surrounding comment frames the catch as "for MagicMock trackers", but plain MagicMock() instances (which is what the propagation tests in test_brc_phase_propagation.py use) don't raise AttributeError on missing attributes — they auto-mock. The catch actually only helps for spec-restricted mocks (MagicMock(spec=...)) or stub objects that explicitly don't implement the method. Practical effect: the catch is harmless and the symmetric coverage is still the right call; only the framing is slightly off. Not worth a follow-up.

Behavioral analysis

Traced the v1 → ACK → v2 path against the production code:

• record_proposal increments version 1→2; _proposal_commit_sha_history["coder"] accumulates {1: v1sha, 2: v2sha}.
• invalidate_ack preserves entry.version=1, so _resolve_reviewer_delta_range resolves to v1sha via get_commit_sha_for_version("coder", 1).
• check_auto_repropose passes with auto_repropose_debounce_seconds=0 (the elapsed < 0 check is always False) and the SHA-different guard.
• The CONSENSUS_RE_REVIEW emission embeds the resolved range correctly.

No new issues introduced. No security/correctness concerns. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.