diff --git a/orchestrator/peer_consensus.py b/orchestrator/peer_consensus.py index ef12ac109a..6322478794 100644 --- a/orchestrator/peer_consensus.py +++ b/orchestrator/peer_consensus.py @@ -114,6 +114,14 @@ def __init__( self._proposal_artifacts: dict[str, list[str]] = {} # Track proposal commit SHAs per producer (#1473) self._proposal_commit_shas: dict[str, str] = {} + # Per-version commit SHA history per producer (#2887). Unlike + # ``_proposal_commit_shas`` (overwritten each propose, holds only + # the current version) this accumulates ``{version: commit_sha}`` + # across re-proposes so a re-review notice can resolve the commit + # a given reviewer last verdicted at (``entry.version`` → + # commit_sha) and emit an authoritative per-reviewer delta range + # (``..HEAD``) instead of a hardcoded v1→v2 anchor. + self._proposal_commit_sha_history: dict[str, dict[int, str]] = {} # Whether handle_timeout() has already processed the timeout self._timeout_handled: bool = False # Auto re-propose safety: debounce timestamps and counters @@ -354,6 +362,16 @@ def _handle_propose_inner( self._proposal_timestamps[agent_role] = datetime.now(UTC) self._proposal_artifacts[agent_role] = list(proposal.artifacts) self._proposal_commit_shas[agent_role] = proposal.commit_sha + # Pin this version's commit SHA in the accumulating history so a + # later re-review notice can resolve any prior reviewer's + # last-verdicted version back to the commit they actually saw + # (#2887). ProposalPayload already requires a non-empty + # commit_sha; the guard is defensive so the history never holds + # an empty anchor. + if proposal.commit_sha: + self._proposal_commit_sha_history.setdefault(agent_role, {})[version] = ( + proposal.commit_sha + ) # Detect reviewers who confirmed on a stale version and need re-review. # This prevents deadlocks where a confirmed reviewer never sees a new @@ -1401,6 +1419,7 @@ def excuse_producer(self, producer_role: str, reason: str = "") -> dict[str, Any self._flip_flop_counts.pop(producer_role, None) self._proposal_artifacts.pop(producer_role, None) self._proposal_commit_shas.pop(producer_role, None) + self._proposal_commit_sha_history.pop(producer_role, None) # Remaining producers may now be fully_acked if the excused # producer held a dual role (producer + reviewer). The next @@ -1499,6 +1518,21 @@ def get_proposal_commit_sha(self, role: str) -> str: """Return the commit SHA from a producer's last proposal (#1473).""" return self._proposal_commit_shas.get(role, "") + def get_commit_sha_for_version(self, producer: str, version: int) -> str: + """Return the commit SHA a producer's proposal was at for ``version``. + + Resolves a reviewer's last-verdicted version (``entry.version``) + back to the commit they actually reviewed, so a re-review notice + can emit an authoritative per-reviewer delta range + ``..HEAD`` instead of the legacy hardcoded v1→v2 anchor + (#2887). Returns "" when no commit was pinned for that version + (version 0 / pre-proposal verdicts, or a producer this tracker + has no proposal history for), letting callers fall back to the + reviewer-self-tracked ``last_reviewed_commit`` range from + REVIEWER-SYNC.md. + """ + return self._proposal_commit_sha_history.get(producer, {}).get(version, "") + def get_pre_merge_conditions(self) -> list[dict[str, Any]]: """Return active conditional-ACK obligations across all producers. @@ -1730,6 +1764,8 @@ def clear(self) -> None: self._proposal_timestamps.clear() self._flip_flop_counts.clear() self._proposal_artifacts.clear() + self._proposal_commit_shas.clear() + self._proposal_commit_sha_history.clear() self._timeout_handled = False self._last_auto_repropose_timestamp.clear() self._auto_repropose_counts.clear() diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 51141a207e..749a4c6abb 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -13414,9 +13414,14 @@ def _build_brc_preamble( "it, then re-confirm via `egg-orch consensus confirmed`. Do NOT " "ignore these messages.\n\n" " **This is adversarial re-review, not blocker-verification.** " - "Your v2 review has TWO equal-weight mandates: (1) verify v1 " - "blockers were addressed AND (2) audit the v2 delta as a fresh " - "reviewer with no NACK history. Both must pass to ACK. " + "Your re-review has TWO equal-weight mandates: (1) verify the " + "blockers from your prior NACK were addressed AND (2) audit the " + "delta since your last review — the commits landed since the " + "version you last verdicted (per REVIEWER-SYNC.md: `git log " + "{last_reviewed_commit}..HEAD --not origin/{base_branch} -p`) — " + "as a fresh reviewer with no NACK history, bounded to that " + "delta, NOT the whole accumulated surface. Both must pass to " + "ACK. " "**The message body is authoritative for the full framing** — " "the orchestrator appends an adversarial re-prime to every " "re-review trigger with the complete dual-mandate decomposition, " @@ -13733,7 +13738,11 @@ def _build_reviewer_preparation( ) -def _re_review_priming_block() -> str: +def _re_review_priming_block( + *, + version: int | None = None, + delta_range: str | None = None, +) -> str: """Adversarial re-prime injected at the moment of every re-review. Counter-anchors the persistent reviewer against the "verify named @@ -13742,13 +13751,31 @@ def _re_review_priming_block() -> str: the v2 delta introducing a non-executable inline `python3 -c` snippet that a downstream GitHub-bot reviewer caught immediately). - Two design choices worth flagging: + Three design choices worth flagging: - **Delta-scoped, not exploration-forcing.** The block tells the - reviewer to re-read the delta adversarially, not to re-traverse - the codebase. The amortized exploration from cycle-1 is the - feature; re-Reading every referenced file on every cycle would - throw away BRC's cost advantage. + reviewer to re-read *the delta since their own last review* + adversarially, not to re-traverse the codebase. The amortized + exploration from cycle-1 is the feature; re-Reading every + referenced file on every cycle would throw away BRC's cost + advantage. + - **Per-reviewer delta, not a fixed version pair (#2887).** The + block was originally hardcoded to the v1→v2 transition and took + no arguments, yet was appended verbatim to every re-review (v3, + v4, …). 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 scope each cycle and + blocking multi-round convergence. The block is now parameterized + by the current proposal version (``vN`` / its prior ``v(N-1)``) + and, on per-reviewer ``CONSENSUS_RE_REVIEW`` notices, anchored to + that reviewer's own ``..HEAD`` ``delta_range`` + (resolved orchestrator-side from the reviewer's last-verdicted + version). When ``delta_range`` is absent (the broadcast + ``CONSENSUS_PROPOSE`` body, ``to_role=all`` — one text for + reviewers sitting at different last-reviewed versions) the block + references the reviewer-self-tracked range from REVIEWER-SYNC.md + (``git log {last_reviewed_commit}..HEAD --not origin/{base} -p``) + instead. - **Economic framing is explicit.** "Re-reviews are cheap / NACK without hesitance" is load-bearing — without it, persistent reviewers naturally optimize for convergence (ACK to end the @@ -13759,35 +13786,90 @@ def _re_review_priming_block() -> str: (signals.py, both withdrawal/re-propose and push-after-propose paths) and to ``CONSENSUS_PROPOSE`` bodies when the producer is re-proposing (version > 1, ``changed_artifacts`` set). Reviewers - who NACK'd v1 receive ``CONSENSUS_PROPOSE`` rather than - ``CONSENSUS_RE_REVIEW`` on a re-propose, so both surfaces need + who NACK'd the prior version receive ``CONSENSUS_PROPOSE`` rather + than ``CONSENSUS_RE_REVIEW`` on a re-propose, so both surfaces need the re-prime to reach every reviewer. + + Args: + version: The current (re-proposed) proposal version ``N``. When + ``None`` (legacy / defensive callers) the block falls back + to generic "current" / "prior" wording without numbered + anchors. + delta_range: A concrete ``..HEAD`` git range scoping this + reviewer's mandate-2 audit to the commits landed since their + own last verdict. Only available on the per-reviewer + ``CONSENSUS_RE_REVIEW`` path; omitted on the broadcast + ``CONSENSUS_PROPOSE`` body. """ + # Adjective placed before "review"/"verdict" ("Your v6 review" / + # "Your current review"); and the prior-version qualifier placed + # before "blockers"/"NACK history" ("named v5 blockers" / "named + # prior blockers"). Both read naturally with or without a version. + vN = f"v{version}" if version is not None else "current" + vNm1 = f"v{version - 1}" if version is not None and version >= 2 else "prior" + # Mandate-2's delta anchor. On the per-reviewer path we have an + # authoritative range; on the broadcast path we point at the + # reviewer-self-tracked range REVIEWER-SYNC.md already defines, so + # each reviewer scopes to the commits since *their* last review + # rather than the whole accumulated surface. + if delta_range: + delta_clause = ( + f"the delta since your last review (`git log {delta_range} " + "--not origin/ -p` — the commits landed since the " + "version you last verdicted)" + ) + delta_short = f"this delta (`{delta_range}`)" + else: + # NOTE: `{last_reviewed_commit}` and `{base_branch}` here are + # *literal* braces, deliberately matching the placeholder names + # the reviewer agent already learned from REVIEWER-SYNC.md + # (shared/prompts/REVIEWER-SYNC.md:110) — the agent substitutes + # them at read-time from its own bookkeeping. Do NOT convert this + # string to an f-string: there are no Python locals named + # `last_reviewed_commit` / `base_branch` here, so f-stringifying + # would raise `NameError` at call time. The per-reviewer branch + # above uses `` instead because that path embeds a + # concrete, orchestrator-resolved range — only `` remains + # for the reviewer to fill in, so the angle-bracket convention + # makes the (already-resolved vs. still-to-resolve) distinction + # visible at a glance. + delta_clause = ( + "the delta since your last review (per REVIEWER-SYNC.md: " + "`git log {last_reviewed_commit}..HEAD --not " + "origin/{base_branch} -p` — the commits landed since the " + "version you last verdicted, NOT the whole accumulated " + "proposal surface)" + ) + delta_short = "this delta (the commits since your last review)" return ( "\n\n**Adversarial re-review**\n\n" - "**Your v2 review has TWO equal-weight mandates:**\n\n" - "1. **Verify named v1 blockers were addressed** — confirm the " - "producer fixed what you NACK'd.\n" - "2. **Audit the v2 delta as a fresh reviewer** — ignore your v1 " - "NACK history. Read the v2 diff as if you'd never seen v1. Apply " - "your lens (security threat-model, concurrency races, contract " - "AC, line-by-line bugs, silent-fallback shapes — whichever your " - "role owns) to the v2 delta itself, not to whether your previous " - "concerns were satisfied.\n\n" + f"**Your {vN} review has TWO equal-weight mandates:**\n\n" + f"1. **Verify named {vNm1} blockers were addressed** — confirm " + "the producer fixed what you NACK'd.\n" + f"2. **Audit {delta_clause} as a fresh reviewer** — ignore your " + f"{vNm1} NACK history. Read that diff as if you'd never seen the " + "prior version. Apply your lens (security threat-model, " + "concurrency races, contract AC, line-by-line bugs, " + "silent-fallback shapes — whichever your role owns) to the " + "delta itself, not to whether your previous concerns were " + "satisfied. **Mandate 2 is bounded to this delta** — it does " + "NOT ask you to re-traverse the whole accumulated surface from " + "earlier cycles; that work was amortized when you first " + "reviewed those commits.\n\n" "Both mandates have equal weight. If (1) passes but (2) finds new " "issues, you NACK. ACK requires both pass.\n\n" "**The named-blockers anchor is a known trap. Every reviewer " "lens has a mandate-2 in its own territory** — security has " - "v2-introduced threat surfaces, concurrency has v2-introduced " - "races, contract has v2-introduced AC drift, code has " - "v2-introduced line-by-line bugs. The four issues that escaped " - "PR #2724 to the GitHub bot were all of code-lens shape " + "newly-introduced threat surfaces, concurrency has newly-" + "introduced races, contract has newly-introduced AC drift, code " + "has newly-introduced line-by-line bugs. The four issues that " + "escaped PR #2724 to the GitHub bot were all of code-lens shape " "(`${ANSWER}` as bare Python, deprecated `datetime.utcnow()`, " "non-atomic write, bare `except: pass`) — the persistent " - 'reviewer correctly answered mandate 1 ("did v1 issues get ' - 'fixed? yes") and skipped mandate 2 ("does v2 introduce new ' - 'issues? actually yes"). The shape generalizes: whatever your ' - "lens, the v2 delta can introduce issues your prior NACK " + 'reviewer correctly answered mandate 1 ("did prior issues get ' + 'fixed? yes") and skipped mandate 2 ("does this delta introduce ' + 'new issues? actually yes"). The shape generalizes: whatever ' + "your lens, this delta can introduce issues your prior NACK " "didn't name. Watching the producer deliver a targeted fix " 'pulls strongly toward "verify my fix-request landed → ACK." ' "Recognize the pull and do mandate 2 anyway.\n\n" @@ -13798,16 +13880,16 @@ def _re_review_priming_block() -> str: "- Apply every rubric pass to the new hunks. New issues outside " "the scope of your prior NACK are blocking; your prior NACK does " "not bound this re-review.\n" - "- **Fresh-reviewer simulation.** Before issuing your v2 verdict, " - "ask: would a reviewer who has only seen the v2 diff with no NACK " - "history ACK this? If you can't argue yes from the v2 diff alone, " - "NACK.\n" - "- **External-bot anchor.** Imagine `egg-reviewer[bot]` reads only " - "your v2 diff with no NACK context. What would it flag? Anything " - "it'd flag, you should NACK first.\n\n" - "**Your v2 verdict must enumerate both halves** so mandate 2 " + "- **Fresh-reviewer simulation.** Before issuing your " + f"{vN} verdict, ask: would a reviewer who has only seen " + f"{delta_short} with no NACK history ACK this? If you can't " + "argue yes from that diff alone, NACK.\n" + "- **External-bot anchor.** Imagine `egg-reviewer[bot]` reads " + f"only {delta_short} with no NACK context. What would it flag? " + "Anything it'd flag, you should NACK first.\n\n" + f"**Your {vN} verdict must enumerate both halves** so mandate 2 " "doesn't silently disappear from the record:\n\n" - "- (a) Which v1 blockers you verified-fixed (mandate 1).\n" + f"- (a) Which {vNm1} blockers you verified-fixed (mandate 1).\n" "- (b) What new issues you audited-and-did-not-find (mandate 2). " 'Name the specific shapes you checked — not "reviewed thoroughly," ' 'but "checked for silent fallbacks, doc-snippet executability, ' diff --git a/orchestrator/routes/signals.py b/orchestrator/routes/signals.py index d063c5ec48..70c5919a87 100644 --- a/orchestrator/routes/signals.py +++ b/orchestrator/routes/signals.py @@ -86,7 +86,10 @@ def _is_sigterm_after_completion(pipeline: Pipeline, error_message: str) -> bool ) -def _get_re_review_priming_text() -> str: +def _get_re_review_priming_text( + version: int | None = None, + delta_range: str | None = None, +) -> str: """Return the adversarial re-review priming block, or "" if unavailable. Centralizes the lazy import of ``_re_review_priming_block`` from @@ -96,6 +99,15 @@ def _get_re_review_priming_text() -> str: falls back to the un-primed message body — a regression that would silently drop the re-prime surfaces in logs instead of degrading the feature invisibly (see #2724 post-mortem). + + Args: + version: Current (re-proposed) proposal version, so the block + anchors to ``vN`` / ``v(N-1)`` rather than a hardcoded + v1→v2 transition (#2887). + delta_range: Per-reviewer ``..HEAD`` range scoping mandate + 2 to the commits since that reviewer's own last verdict. + Only passed on the per-reviewer ``CONSENSUS_RE_REVIEW`` + path; omitted on the broadcast ``CONSENSUS_PROPOSE`` body. """ try: from routes.pipelines import _re_review_priming_block @@ -108,7 +120,47 @@ def _get_re_review_priming_text() -> str: "re-review priming will not be appended to message bodies" ) return "" - return _re_review_priming_block() + return _re_review_priming_block(version=version, delta_range=delta_range) + + +def _resolve_reviewer_delta_range( + tracker: Any, + producer: str, + reviewer: str, + head_sha: str, +) -> str | None: + """Return a ``..`` range for a reviewer's re-review. + + Scopes the reviewer's mandate-2 audit to exactly the commits landed + since *their own* last verdict (#2887): the reviewer's last-verdicted + proposal version (``entry.version``) resolves, via the tracker's + per-version commit history, to the commit they actually reviewed, and + the range runs from there to the new proposal commit. + + Returns ``None`` when the prior-reviewed commit can't be resolved — no + prior verdict, a version-0 pre-proposal ACK, missing commit history, + or an empty/unchanged head — so the caller falls back to the priming + block's generic, reviewer-self-tracked range from REVIEWER-SYNC.md. + """ + if not head_sha: + return None + # Both ``tracker.matrix.get_entry`` and ``tracker.get_commit_sha_for_version`` + # are real on ``PeerConsensusTracker``, but several call sites pass a + # ``MagicMock`` tracker (the pre-#2887 propagation tests). We catch + # ``AttributeError`` across both reads so a stub missing either surface + # degrades to the REVIEWER-SYNC fallback rather than 500-ing — the + # asymmetry of catching one but not the other was a foot-gun flagged + # in PR review. + try: + entry = tracker.matrix.get_entry(reviewer, producer) + if entry is None or not entry.version: + return None + last_sha = tracker.get_commit_sha_for_version(producer, entry.version) + except AttributeError: + return None + if not last_sha or last_sha == head_sha: + return None + return f"{last_sha}..{head_sha}" signals_bp = Blueprint("signals", __name__, url_prefix="/api/v1/pipelines") @@ -1382,7 +1434,11 @@ def handle_consensus_propose_signal( # don't reach every reviewer. See #2724 post-mortem. propose_body = payload.get("summary", "") if changed_artifacts: - propose_body = propose_body + _get_re_review_priming_text() + # Broadcast body (to_role="all") — one text shared across + # reviewers who may sit at different last-reviewed versions, + # so no per-reviewer delta_range; the block points each + # reviewer at the REVIEWER-SYNC self-tracked range (#2887). + propose_body = propose_body + _get_re_review_priming_text(version=result.get("version")) store.add_message( Message( @@ -1412,7 +1468,15 @@ def handle_consensus_propose_signal( f"Your previous confirmation was on an earlier version. " f"Please re-review and ACK/NACK the new proposal." ) - re_review_body = re_review_body + _get_re_review_priming_text() + # Per-reviewer delta: scope mandate 2 to the commits since + # this reviewer's own last verdict, resolved authoritatively + # from their last-reviewed version's commit (#2887). + delta_range = _resolve_reviewer_delta_range( + tracker, agent_role, stale_reviewer, commit_sha + ) + re_review_body = re_review_body + _get_re_review_priming_text( + version=result.get("version"), delta_range=delta_range + ) store.add_message( Message( @@ -2392,7 +2456,7 @@ def handle_consensus_producer_push_signal( propose_body = ( f"Producer {agent_role} pushed new commit {commit_sha}. " f"Existing ACKs invalidated; re-review required." - ) + _get_re_review_priming_text() + ) + _get_re_review_priming_text(version=result.get("version")) store.add_message( Message( pipeline_id=pipeline_id, @@ -2419,11 +2483,18 @@ def handle_consensus_producer_push_signal( result.get("stale_reviewers", []) + result.get("invalidated_reviewers", []) ) for reviewer in notified_reviewers: + # Per-reviewer delta: scope mandate 2 to the commits since + # this reviewer's own last verdict (#2887). + delta_range = _resolve_reviewer_delta_range( + tracker, agent_role, reviewer, commit_sha + ) re_review_body = ( f"Producer {agent_role} has pushed new commits after " f"proposing. Your previous review is invalidated. " f"Please re-review and ACK/NACK the updated work." - ) + _get_re_review_priming_text() + ) + _get_re_review_priming_text( + version=result.get("version"), delta_range=delta_range + ) store.add_message( Message( pipeline_id=pipeline_id, diff --git a/orchestrator/tests/test_pipeline_prompts.py b/orchestrator/tests/test_pipeline_prompts.py index 0abd658201..a71aa1d6c4 100644 --- a/orchestrator/tests/test_pipeline_prompts.py +++ b/orchestrator/tests/test_pipeline_prompts.py @@ -4675,18 +4675,29 @@ def test_priming_block_helper_returns_load_bearing_phrases(self): cannot disappear without violating the #2724 design intent. The block is structured around explicit dual-mandate - decomposition (verify v1 blockers / audit v2 delta as fresh - reviewer). Vague-adversarial language alone leaves the + decomposition (verify prior blockers / audit the delta as a + fresh reviewer). Vague-adversarial language alone leaves the named-blockers anchor in place; naming the bias and giving the reviewer two checkable mandates is the load-bearing piece. + + Parameterized by version (#2887): a vN call must anchor to vN / + v(N-1) rather than the legacy hardcoded v1→v2 transition, so a + v6 re-review is told to audit *its* delta, not "the v2 delta". """ from routes.pipelines import _re_review_priming_block - block = _re_review_priming_block() + block = _re_review_priming_block(version=6) # Dual-mandate decomposition — the structural anchor. assert "TWO equal-weight mandates" in block - assert "named v1 blockers" in block - assert "Audit the v2 delta as a fresh reviewer" in block + # Version anchoring is dynamic (#2887): vN / v(N-1), not v1/v2. + assert "Your v6 review" in block + assert "named v5 blockers" in block + assert "v2" not in block and "v1 " not in block + # Mandate 2 is delta-scoped, not whole-surface. + assert "Audit the delta since your last review" in block + assert "as a fresh reviewer" in block + assert "bounded to this delta" in block.lower() + assert "NOT the whole accumulated" in block or "not the whole accumulated" in block.lower() assert "equal weight" in block.lower() assert "ACK requires both pass" in block # Explicit bias-naming — the trap the priming counters. @@ -4714,6 +4725,44 @@ def test_priming_block_helper_returns_load_bearing_phrases(self): # GitHub-as-no-op standard. assert "GitHub" in block + def test_priming_block_per_reviewer_delta_range_embedded(self): + """When a concrete ``delta_range`` is supplied (the per-reviewer + ``CONSENSUS_RE_REVIEW`` path), the block embeds that authoritative + ``..HEAD`` git range so mandate 2 is bounded to the commits + since *that* reviewer's own last verdict — the core #2887 fix. + """ + from routes.pipelines import _re_review_priming_block + + block = _re_review_priming_block(version=4, delta_range="abc123..def456") + assert "abc123..def456" in block + assert "git log abc123..def456" in block + assert "Your v4 review" in block + + def test_priming_block_broadcast_path_uses_reviewer_sync_range(self): + """Without a ``delta_range`` (the broadcast ``CONSENSUS_PROPOSE`` + body, shared across reviewers at different last-reviewed versions), + the block points each reviewer at the REVIEWER-SYNC self-tracked + range rather than a hardcoded version delta (#2887). + """ + from routes.pipelines import _re_review_priming_block + + block = _re_review_priming_block(version=3) + assert "last_reviewed_commit}..HEAD" in block + assert "REVIEWER-SYNC.md" in block + + def test_priming_block_no_version_falls_back_gracefully(self): + """A version-less call (defensive / legacy) must not crash and + must avoid the stale v1/v2 anchors — it uses generic + "current"/"prior" wording instead (#2887). + """ + from routes.pipelines import _re_review_priming_block + + block = _re_review_priming_block() + assert "TWO equal-weight mandates" in block + assert "Your current review" in block + assert "named prior blockers" in block + assert "your prior NACK history" in block + @pytest.mark.parametrize( ("role", "phase"), [ diff --git a/orchestrator/tests/test_producer_push_consensus.py b/orchestrator/tests/test_producer_push_consensus.py index 5809022d61..375c8a14c6 100644 --- a/orchestrator/tests/test_producer_push_consensus.py +++ b/orchestrator/tests/test_producer_push_consensus.py @@ -771,3 +771,52 @@ def test_multiple_pushes_then_consensus(self, simple_tracker): # Invariants clean violations = tracker.validate_invariants() assert len(violations) == 0 + + +class TestProposalCommitShaHistory: + """Per-version commit SHA history backing delta-scoped re-review (#2887). + + The tracker pins each proposal version's commit SHA so a re-review + notice can resolve a reviewer's last-verdicted version back to the + commit they actually saw and emit a per-reviewer ``..HEAD`` + delta range, replacing the legacy hardcoded v1->v2 anchor. + """ + + def test_history_accumulates_across_versions(self, simple_tracker): + """Each propose / auto-push pins its version's SHA; prior versions + remain resolvable (unlike the single-slot ``_proposal_commit_shas``).""" + tracker = simple_tracker + + tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + tracker.handle_producer_push("coder", "sha2") + tracker.handle_producer_push("coder", "sha3") + + assert tracker.get_commit_sha_for_version("coder", 1) == "sha1" + assert tracker.get_commit_sha_for_version("coder", 2) == "sha2" + assert tracker.get_commit_sha_for_version("coder", 3) == "sha3" + # The current-only slot tracks only the latest. + assert tracker.get_proposal_commit_sha("coder") == "sha3" + + def test_unknown_version_returns_empty(self, simple_tracker): + """A version with no pinned commit (e.g. version 0 / not yet + proposed) resolves to "" so callers fall back to REVIEWER-SYNC.""" + tracker = simple_tracker + + tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + assert tracker.get_commit_sha_for_version("coder", 99) == "" + assert tracker.get_commit_sha_for_version("unknown_producer", 1) == "" + + def test_reviewer_last_verdict_resolves_to_reviewed_commit(self, simple_tracker): + """A reviewer's NACK entry version resolves to the commit they + reviewed even after the producer advances past it — the core + lookup the re-review delta range depends on (#2887).""" + tracker = simple_tracker + + tracker.handle_propose("coder", make_proposal(commit_sha="sha1")) + nack_producer(tracker, "reviewer_code", "coder", reason="v1 bug") + # Producer re-proposes; reviewer's entry still points at v1. + tracker.handle_propose("coder", make_proposal(commit_sha="sha2")) + + entry = tracker.matrix.get_entry("reviewer_code", "coder") + assert entry.version == 1 + assert tracker.get_commit_sha_for_version("coder", entry.version) == "sha1" diff --git a/orchestrator/tests/test_signals.py b/orchestrator/tests/test_signals.py index 059c992a1e..a527c1ae5d 100644 --- a/orchestrator/tests/test_signals.py +++ b/orchestrator/tests/test_signals.py @@ -2147,3 +2147,176 @@ def test_nack_rejected_when_nack_version_negative(self, mock_subprocess_run, app assert body["success"] is False assert ">= 1" in body["message"] mock_tracker.handle_nack.assert_not_called() + + +class TestResolveReviewerDeltaRange: + """`_resolve_reviewer_delta_range` resolves each reviewer's own + `..HEAD` re-review range from their last-verdicted version, + backing delta-scoped re-review (#2887). Falls back to None (→ the + priming block's REVIEWER-SYNC range) when no anchor is resolvable. + """ + + @pytest.fixture + def tracker(self): + from attestation_schemas import AttestationStrictness + from peer_consensus import PeerConsensusTracker + from review_graph import ReviewCriticality, ReviewEdge, ReviewGraph + + graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)]) + t = PeerConsensusTracker( + "test-pipeline", + graph, + cooldown_seconds=0, + attestation_strictness=AttestationStrictness.RELAXED, + auto_repropose_debounce_seconds=0, + ) + t.register_agent("coder") + t.register_agent("reviewer_code") + return t + + def test_range_spans_reviewer_last_verdict_to_head(self, tracker): + from routes.signals import _resolve_reviewer_delta_range + + tracker.handle_propose( + "coder", + {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "sha1"}, + ) + tracker.handle_nack( + "reviewer_code", "coder", {"artifact_references": ["a.py"], "reason": "x"} + ) + # Producer re-proposes at sha2; reviewer's entry still pins v1/sha1. + tracker.handle_propose( + "coder", + {"summary": "v2", "artifacts": ["a.py"], "commit_sha": "sha2"}, + ) + + rng = _resolve_reviewer_delta_range(tracker, "coder", "reviewer_code", "sha2") + assert rng == "sha1..sha2" + + def test_no_prior_verdict_returns_none(self, tracker): + from routes.signals import _resolve_reviewer_delta_range + + tracker.handle_propose( + "coder", + {"summary": "v1", "artifacts": ["a.py"], "commit_sha": "sha1"}, + ) + # Reviewer never verdicted (entry.version == 0). + rng = _resolve_reviewer_delta_range(tracker, "coder", "reviewer_code", "sha2") + assert rng is None + + def test_empty_head_returns_none(self, tracker): + from routes.signals import _resolve_reviewer_delta_range + + assert _resolve_reviewer_delta_range(tracker, "coder", "reviewer_code", "") is None + + +class TestReReviewDeltaRangeReachesMessageBody: + """End-to-end #2887 verification: a real ``PeerConsensusTracker`` walked + through propose-v1 → ACK → producer-push-v2 emits a per-reviewer + ``CONSENSUS_RE_REVIEW`` whose body actually contains the resolved + ``..`` delta range. + + The existing unit tests cover ``_resolve_reviewer_delta_range`` + (``TestResolveReviewerDeltaRange``) and the underlying SHA-history + accumulator (``TestProposalCommitShaHistory`` in + ``test_producer_push_consensus.py``) in isolation, and the existing + MagicMock-based propagation tests (``TestProposeMessagePhasePropagation`` + in ``test_brc_phase_propagation.py``) pin that *some* re-prime text is + appended. None of those exercise the seam #2887 actually patches: the + delta range being resolved correctly but then dropped (wrong argument, + accidental ``None``, helper return ignored) before reaching the emitted + message body. This test runs the real handler with a real tracker and + asserts the concrete substring lands in the ``CONSENSUS_RE_REVIEW`` body. + """ + + def test_per_reviewer_re_review_body_contains_concrete_delta_range(self, app): + from attestation_schemas import AttestationStrictness + from message_store import MessageType + from peer_consensus import ( + create_peer_consensus_tracker, + remove_peer_consensus_tracker, + ) + from review_graph import ReviewCriticality, ReviewEdge, ReviewGraph + + pipeline_id = "issue-2887-e2e" + v1_sha = "v1sha1234abcd" + v2_sha = "v2sha5678efef" + + graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)]) + tracker = create_peer_consensus_tracker( + pipeline_id, + graph, + cooldown_seconds=0, + attestation_strictness=AttestationStrictness.RELAXED, + auto_repropose_debounce_seconds=0, + ) + try: + tracker.register_agent("coder") + tracker.register_agent("reviewer_code") + + # v1: producer proposes at sha1; reviewer ACKs at v1 (their + # matrix entry now pins entry.version=1 → sha1). + tracker.handle_propose( + "coder", + { + "summary": "v1 implementation", + "artifacts": ["src/auth.py"], + "commit_sha": v1_sha, + }, + ) + tracker.handle_ack( + "reviewer_code", + "coder", + {"artifact_references": ["src/auth.py"]}, + ) + + # v2: producer pushes a new commit. The signal handler auto + # re-proposes, invalidates the v1 ACK, and emits a + # CONSENSUS_RE_REVIEW to the reviewer. The body must embed + # the concrete `..` delta range from + # `_resolve_reviewer_delta_range` — the #2887 contract. + mock_msg_store = MagicMock() + with ( + app.app_context(), + patch("message_store.get_message_store", return_value=mock_msg_store), + ): + from routes.signals import handle_consensus_producer_push_signal + + _response, status_code = handle_consensus_producer_push_signal( + pipeline_id, + { + "agent_role": "coder", + "commit_sha": v2_sha, + # Omit ``changed_files`` so all ACKs are invalidated + # (the conservative path in ``handle_producer_push``), + # which is what populates ``invalidated_reviewers`` + # for the per-reviewer CONSENSUS_RE_REVIEW emission. + }, + Path("/tmp/repo"), + ) + + assert status_code == 200 + + re_review_messages = [ + call.args[0] + for call in mock_msg_store.add_message.call_args_list + if call.args[0].message_type == MessageType.CONSENSUS_RE_REVIEW + and call.args[0].to_role == "reviewer_code" + ] + assert len(re_review_messages) == 1, ( + f"Expected exactly one CONSENSUS_RE_REVIEW for reviewer_code, " + f"got {len(re_review_messages)}" + ) + body = re_review_messages[0].body + + # The core #2887 assertion: the resolved per-reviewer delta + # range is embedded as a concrete `..` string, NOT + # the broadcast-path REVIEWER-SYNC placeholder. + assert f"{v1_sha}..{v2_sha}" in body + assert f"git log {v1_sha}..{v2_sha}" in body + assert "{last_reviewed_commit}..HEAD" not in body + # Version anchoring is dynamic (vN / v(N-1)). + assert "Your v2 review" in body + assert "named v1 blockers" in body + finally: + remove_peer_consensus_tracker(pipeline_id)