fix(brc): scope review deltas to the proposal SHA; make read_peer_artifact honest about mid-phase emptiness

docs/reference/agent-tools.md

@@ -130,7 +130,7 @@ that requires the handler docstring to explain why no CLI exists.
 | `mcp__brc__send_heartbeat` | Emit a structured `HEARTBEAT` (schema-validated, per-role deduped, rate-limited) to the dedicated `/heartbeat` endpoint. Use `state=WAITING_ON_ROLE` + `waiting_on=<peer>` while blocking on BRC. Valid states: `WORKING`, `WAITING_ON_ROLE`, `WAITING_FOR_EVENT`, `PROPOSED`, `IDLE`. | `handlers.message.message_heartbeat` | `egg-orch message heartbeat` |
 
 > **Blocking waits use Bash, not MCP** (#2211). Long-poll waits don't fit the MCP transport — both transports cap tool calls below typical quiet-phase intervals (~30 s streamable-HTTP, ~60 s in-process SDK), and every cap-elapsed return is a wasted LLM turn. Use `egg-orch message wait` / `egg-orch message wait-loop` (sandbox) and `egg-orch pipeline wait-status` (host) via Bash. The §1 idiom in `docs/reference/agent-wait-patterns.md` is the canonical shape.
-| `mcp__brc__read_peer_artifact` | Read entries from `.egg-state/brc-history/<identifier>-<phase>.json` (and the per-slice partition `<identifier>-implement-<slice_id>.json` when `EGG_SLICE_ID` is set and `phase == "implement"`; the sibling `<identifier>-implement-unattributed.json` is merged in by default and disabled via `include_unattributed=False`). Optional filters: `peer_role` / `producer_role` (alias), `message_type` (str or list). `limit` / `cursor` pagination (default `limit=50`, max 500). The identifier is resolved server-side from `EGG_ISSUE_NUMBER` / `EGG_PIPELINE_ID` (agents cannot pass an arbitrary id; path-traversal hardening). Returns `{items: [...], next_cursor: <str|None>, total_available: <int>, skipped_malformed: <int>}`. | `handlers.brc.brc_read_peer_artifact` | `egg-orch brc read-peer-artifact` *(slice-5 of #2908; thin wrapper, registration still `cli_command=None` — see callout below)* |
+| `mcp__brc__read_peer_artifact` | Read entries from `.egg-state/brc-history/<identifier>-<phase>.json` (and the per-slice partition `<identifier>-implement-<slice_id>.json` when `EGG_SLICE_ID` is set and `phase == "implement"`; the sibling `<identifier>-implement-unattributed.json` is merged in by default and disabled via `include_unattributed=False`). Optional filters: `peer_role` / `producer_role` (alias), `message_type` (str or list). `limit` / `cursor` pagination (default `limit=50`, max 500). The identifier is resolved server-side from `EGG_ISSUE_NUMBER` / `EGG_PIPELINE_ID` (agents cannot pass an arbitrary id; path-traversal hardening). Returns `{items: [...], next_cursor: <str|None>, total_available: <int>, skipped_malformed: <int>, hint?: <str>}` — `hint` is present only when no history file exists on disk, the expected state for the phase currently in flight (#3076): brc-history is written at phase *completion* and reaches an agent worktree only at spawn, so a current-phase read is structurally empty and is not evidence that peers have not proposed (live proposals: `pending_reviews[].proposal_commit_sha` + `git show <sha>:<path>`). | `handlers.brc.brc_read_peer_artifact` | `egg-orch brc read-peer-artifact` *(slice-5 of #2908; thin wrapper, registration still `cli_command=None` — see callout below)* |
 | `mcp__brc__resolve_obligation` | Mark a reviewer's conditional-ACK obligation as satisfied in-cycle (#2338). Required: `reviewer_role`, `producer_role`. Optional: `commit_sha`, `note`. The matrix keeps the obligation text for audit, but `get_pre_merge_conditions` filters resolved entries — the PR body and HITL gate stop surfacing the obligation. The orchestrator persists a `CONSENSUS_OBLIGATION_RESOLVED` message so the resolution survives orchestrator restart, and rejects `resolver_role == producer_role` so a producer cannot self-resolve their own obligation. Resolution is per-version: any later ACK / NACK / invalidate on the same edge resets the resolved flag. | `handlers.brc.brc_resolve_obligation` | `egg-orch brc resolve-obligation` *(slice-5 of #2908; thin wrapper, registration still `cli_command=None` — see callout below)* |
 
 #### `brc_propose` push behavior

integration_tests/regression/test_brc_concurrency.py

@@ -94,7 +94,7 @@ def test_six_reviewers_acking_in_parallel_all_recorded(
         n = 6
         graph = self._wide_graph(n)
         tracker = make_tracker(self.PIPELINE_ID, graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
 
         barrier = threading.Barrier(n)
 
@@ -130,7 +130,7 @@ def test_propose_and_ack_interleaved_does_not_corrupt_state(self, two_reviewer_g
         with the recorded ACK versions).
         """
         tracker = make_tracker(self.PIPELINE_ID + "-interleave", two_reviewer_graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="v1"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="1111111"))
 
         # reviewer_code ACKs v1.
         tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()})
@@ -141,7 +141,7 @@ def repropose() -> dict:
             barrier.wait(timeout=5)
             return tracker.handle_re_propose(
                 "coder",
-                propose_payload(commit_sha="v2"),
+                propose_payload(commit_sha="2222222"),
                 changed_artifacts=["a.py"],
             )
 
@@ -195,7 +195,7 @@ def test_two_concurrent_nacks_block_repropose_until_acknowledged(
         been informed) is accepted.
         """
         tracker = make_tracker(self.PIPELINE_ID, two_reviewer_graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
 
         barrier = threading.Barrier(2)
 
@@ -220,7 +220,7 @@ def nack(reviewer: str) -> dict:
         # surfaced before the producer can advance.
         first = tracker.handle_re_propose(
             "coder",
-            propose_payload(commit_sha="def"),
+            propose_payload(commit_sha="def5678"),
             changed_artifacts=["a.py"],
         )
         assert first["status"] == "open_nacks_blocked"
@@ -230,7 +230,7 @@ def nack(reviewer: str) -> dict:
         # Producer has now been informed — retry proceeds.
         second = tracker.handle_re_propose(
             "coder",
-            propose_payload(commit_sha="def"),
+            propose_payload(commit_sha="def5678"),
             changed_artifacts=["a.py"],
         )
         assert second["version"] == 2
@@ -252,7 +252,7 @@ def test_withdraw_within_cooldown_is_rejected(self) -> None:
 
         graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)])
         tracker = make_tracker(self.PIPELINE_ID, graph, cooldown_seconds=60)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
 
         # Immediate withdraw — well within the 60s cooldown.
         with pytest.raises(ValueError, match="[Cc]ooldown"):
@@ -288,14 +288,14 @@ def test_flip_flop_lockout_after_repeated_withdrawals(self) -> None:
         tracker.register_agent("reviewer_code")
 
         # Cycle 1: propose → withdraw → counter=1, allowed.
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
         r1 = tracker.handle_withdraw("coder", reason="bug found")
         assert r1["status"] == "withdrawn"
 
         # Cycle 2: propose → withdraw → peek=2, 2 >= max=2, locked out.
         # The guard's peek is ``current + 1 >= max`` so with max=2 the
         # second withdraw is the one that gets locked out.
-        tracker.handle_propose("coder", propose_payload(commit_sha="def"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="def5678"))
         r2 = tracker.handle_withdraw("coder", reason="another bug")
         assert r2["status"] == "locked_out"
         assert r2["needs_escalation"] is True

integration_tests/regression/test_brc_edge_cases.py

@@ -53,8 +53,10 @@ def test_tester_confirms_only_when_both_state_machines_confirm(self) -> None:
         tracker = make_tracker(self.PIPELINE_ID, graph)
 
         # Both producers propose.
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
-        tracker.handle_propose("tester", propose_payload(commit_sha="def", artifacts=["test_x.py"]))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
+        tracker.handle_propose(
+            "tester", propose_payload(commit_sha="def5678", artifacts=["test_x.py"])
+        )
 
         # All cross-ACKs.
         tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()})
@@ -119,15 +121,15 @@ def test_two_slices_have_independent_trackers(self) -> None:
             tracker_2.register_agent(role)
 
         # slice-1: PROPOSE → NACK (consensus blocked).
-        tracker_1.handle_propose("coder", propose_payload(commit_sha="aaa"))
+        tracker_1.handle_propose("coder", propose_payload(commit_sha="aaa1111"))
         tracker_1.handle_nack(
             "reviewer_code",
             "coder",
             {"nack_version": 1, "artifact_references": ["a.py"], "reason": "bug"},
         )
 
         # slice-2: PROPOSE → ACK → CONFIRMED (consensus reached).
-        tracker_2.handle_propose("coder", propose_payload(commit_sha="bbb"))
+        tracker_2.handle_propose("coder", propose_payload(commit_sha="bbb2222"))
         tracker_2.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()})
         tracker_2.handle_confirmed("coder")
         tracker_2.handle_confirmed("reviewer_code")
@@ -171,7 +173,7 @@ def test_conditional_ack_then_obligation_resolved_clears_merge_block(
             ]
         )
         tracker = make_tracker(self.PIPELINE_ID, graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
 
         # Conditional ACK — reviewer says "I'd merge if you ran git mv X Y first".
         tracker.handle_ack(
@@ -223,7 +225,7 @@ class TestStaleVersionAckRejection:
     def test_acking_old_version_after_repropose_is_rejected(self, single_reviewer_graph) -> None:
         """A reviewer can't ACK v1 after the producer has re-proposed to v2."""
         tracker = make_tracker(self.PIPELINE_ID, single_reviewer_graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="v1"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="1111111"))
         # Reviewer NACKs v1.
         tracker.handle_nack(
             "reviewer_code",
@@ -233,7 +235,7 @@ def test_acking_old_version_after_repropose_is_rejected(self, single_reviewer_gr
         # Producer re-proposes (v2).
         tracker.handle_re_propose(
             "coder",
-            propose_payload(commit_sha="v2"),
+            propose_payload(commit_sha="2222222"),
             changed_artifacts=["a.py"],
         )
         # Reviewer mistakenly tries to ACK the old v1 — guard rejects.

integration_tests/regression/test_brc_phase_timeout.py

@@ -124,7 +124,7 @@ def test_critical_blocker_lands_phase_override_in_alert(
         pipeline = _make_pipeline(pipeline_id, config, PipelinePhase.PLAN)
         graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)])
         tracker = make_tracker(pipeline_id, graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
         tracker.handle_nack(
             "reviewer_code",
             "coder",
@@ -181,7 +181,7 @@ def test_advisory_only_path_is_silent_at_alert_layer(
         )
         graph = ReviewGraph([ReviewEdge("reviewer_contract", "coder", ReviewCriticality.ADVISORY)])
         tracker = make_tracker(pipeline_id, graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
         tracker.handle_nack(
             "reviewer_contract",
             "coder",
@@ -229,7 +229,7 @@ def test_idle_critical_reviewer_fires_overseer_alert(
         graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)])
         tracker = make_tracker(pipeline_id, graph)
         # Producer proposes; reviewer never ACKs or NACKs.
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
 
         capture, alerts = _capture_alerts()
         with capture:
@@ -271,7 +271,7 @@ def test_idle_advisory_reviewer_fires_notification(self, event_capture, filter_e
         )
         graph = ReviewGraph([ReviewEdge("reviewer_contract", "coder", ReviewCriticality.ADVISORY)])
         tracker = make_tracker(pipeline_id, graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
 
         capture, alerts = _capture_alerts()
         with capture:
@@ -300,7 +300,7 @@ def test_consensus_reached_before_timeout_is_no_op(self, event_capture, filter_e
         pipeline = _make_pipeline(pipeline_id, PipelineConfig(), PipelinePhase.PLAN)
         graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)])
         tracker = make_tracker(pipeline_id, graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
         tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()})
         tracker.handle_confirmed("coder")
         tracker.handle_confirmed("reviewer_code")
@@ -366,7 +366,7 @@ def test_resolved_minutes_match_alert_metadata(
         # branch — that's the path that publishes an alert.
         graph = ReviewGraph([ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL)])
         tracker = make_tracker(pipeline_id, graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
         tracker.handle_nack(
             "reviewer_code",
             "coder",

integration_tests/regression/test_brc_single_cycle.py

@@ -177,7 +177,7 @@ def test_pre_proposal_ack_without_version_is_recorded_then_invalidated(
         assert result["status"] == "acked"
         assert result["version"] == 0
         # Now the producer proposes — invalidation kicks in.
-        result = tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        result = tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
         assert "reviewer_code" in result["stale_reviewers"]
 
     def test_pre_proposal_ack_with_explicit_version_is_rejected(
@@ -210,7 +210,7 @@ def test_producer_confirm_before_full_acks_is_pending(self, two_reviewer_graph)
         """
         pipeline_id = self.PIPELINE_ID + "-prod"
         tracker = make_tracker(pipeline_id, two_reviewer_graph)
-        tracker.handle_propose("coder", propose_payload(commit_sha="abc"))
+        tracker.handle_propose("coder", propose_payload(commit_sha="abc1234"))
         tracker.handle_ack("reviewer_code", "coder", {"ack_version": 1, **ack_payload()})
         # Only reviewer_code has ACKed; reviewer_contract has not.
         result = tracker.handle_confirmed("coder")

orchestrator/attestation_schemas.py

@@ -5,11 +5,14 @@
 actually doing the work — and enable cross-verification by reviewers.
 """
 
+import re
 from enum import StrEnum
 from typing import Any
 
 from pydantic import BaseModel, Field, model_validator
 
+_COMMIT_SHA_PATTERN = re.compile(r"[A-Za-z0-9_]{7,64}")
+
 
 class AttestationStrictness(StrEnum):
     """Strictness level for attestation validation."""
@@ -212,6 +215,55 @@ def validate_commit_sha_present(self) -> ProposalPayload:
             )
         return self
 
+    @model_validator(mode="after")
+    def validate_commit_sha_format(self) -> ProposalPayload:
+        """Reject commit_sha values containing shell metacharacters (#3076).
+
+        The producer-supplied SHA is interpolated into rendered shell
+        commands in the reviewer's event prompt (the per-producer
+        ``git log <sha>..<proposal_sha>`` and ``git show <sha>:<path>``
+        renders in ``orchestrator/routes/event_prompt.py``).
+        ``_extract_proposal_sha_for_producer`` gates the read path with
+        a stricter hex-only regex, but downstream consumers
+        (``orchestrator/peer_consensus.py``,
+        ``orchestrator/routes/signals.py``,
+        ``orchestrator/routes/pipelines.py``) read
+        ``_proposal_commit_shas`` directly without revalidating — so
+        enforce a shell-safe baseline at the writer too: only
+        ``[A-Za-z0-9_]`` permitted, 7-64 chars. This rejects every
+        dangerous form (whitespace, ``;``, ``$(…)``, ``..``, ranges)
+        while accepting reconstruction sentinels like
+        ``RECONSTRUCTED_NO_SHA`` that downstream code keys on. Skipped
+        for a no-op propose (#3027): ``commit_sha`` is empty by design
+        there.
+
+        Asymmetric regex with
+        ``orchestrator/routes/event_prompt.py::_extract_proposal_sha_for_producer``
+        is intentional: the strict hex-only check there is the
+        shell-interpolation boundary (rejects sentinels before they
+        reach a rendered ``git`` command), while this loose
+        alphanumeric+underscore check is the writer-side baseline
+        (admits sentinels so they can round-trip through
+        ``_proposal_commit_shas`` to non-shell consumers). Do not
+        unify — tightening this regex breaks the sentinel round-trip;
+        loosening the reader regex re-opens the shell-injection gap.
+
+        Relies on ``validate_commit_sha_present`` running first
+        (pydantic ``model_validator(mode="after")`` honours definition
+        order) to reject empty non-no-op proposals before this check
+        sees them; the ``not self.commit_sha`` guard below is
+        defence-in-depth and unreachable in practice.
+        """
+        if self.no_changes_needed or not self.commit_sha:
+            return self
+        if not _COMMIT_SHA_PATTERN.fullmatch(self.commit_sha):
+            raise ValueError(
+                "Proposal commit_sha must be 7-64 alphanumeric/underscore "
+                "characters (no shell metacharacters); got "
+                f"{self.commit_sha!r}."
+            )
+        return self
+
 
 class ReviewPayload(BaseModel):
     """Payload for CONSENSUS_ACK and CONSENSUS_NACK messages.

orchestrator/routes/consensus.py

@@ -207,6 +207,16 @@ def _has_pending_peer_proposals(
                     "prior_version": entry.version if entry else 0,
                     "prior_verdict": entry.state.value if entry else "pending",
                     "artifact_refs": artifact_refs,
+                    # The producer's proposed commit SHA (#3076). Per-role
+                    # worktrees share the host repo's object store, so the
+                    # composer can scope the re-review delta to
+                    # ``{last_reviewed}..{proposal_commit_sha}`` and render
+                    # ``git show <sha>:<path>`` reads that work from the
+                    # reviewer's worktree without any push/merge
+                    # choreography. ``{sha}..HEAD`` against the REVIEWER's
+                    # HEAD never contains the producer's commits — that was
+                    # the "re-review delta is empty" phantom-NACK.
+                    "proposal_commit_sha": str(snapshot.get("commit_sha") or ""),
                 }
             )
     return bool(pending), pending