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

[jwbron]

Summary

Tactical fix for #3076 — phase 1 of #3077 ("coordination state is served, not replicated"). Removes the phantom-NACK failure mode without new protocol machinery: reviewers get working, concrete read paths for proposals they're asked to review, and the dead channel stops masquerading as a live one.

Two structural causes, both confirmed in code (full analysis: #3076 comment):

1. read_peer_artifact is structurally empty mid-phase: it reads .egg-state/brc-history/ from the agent's own worktree, but the orchestrator writes that file only at phase completion, into the work branch — it reaches agent worktrees only via the spawn fork point. The bare ok: true, items: [] result read as "peers produced nothing."
2. The re-review delta ran git log {last_reviewed}..HEAD in the reviewer's worktree — per-role worktrees mean the reviewer's HEAD never contains the producer's commits, so the range is empty even after a revision (the literal "re-review delta is empty" v2 NACK on pipeline-2b3d8b0b). The first-review fallback said "fetch and read the diffs yourself" without saying where.

Changes

• orchestrator/routes/consensus.py — pending_reviews entries carry the producer's proposal_commit_sha (already tracked in the proposal snapshot; one field threaded through).
• orchestrator/routes/event_prompt.py —
  • Re-review delta endpoint becomes the proposal SHA when the payload carries one ({last_reviewed}..{proposal_sha}); HEAD only for legacy payloads. The SHA resolves from any agent worktree via the shared host object store — no push/merge choreography needed.
  • First-review fallback renders concrete working commands (git show <sha>:<path> per artifact, git log <sha> --not origin/<base> -p) plus an explicit "a plain Read of the path failing is NOT evidence the artifact doesn't exist — do not NACK before reading via these commands."
  • The (no commits in range — re-review is a no-op) verdict is only rendered when the range was proposal-scoped; an empty HEAD-scoped range gets an explicit caution instead.
  • The SHA is hex-validated (7-64 chars) before interpolation into rendered commands.
• sandbox/egg_agent_tools/handlers/brc.py — the no-file branch of read_peer_artifact returns a hint explaining the emptiness is structural (phase-completion artifact, spawn-time delivery) and pointing at the live channel. Hint is absent when a file exists (filtered-to-empty is a real answer).
• sandbox/egg_agent_tools/tools/brc.py — tool description rewritten to say what the tool actually is: a completed-phase transcript reader, not live peer-artifact access.
• docs/reference/agent-tools.md row updated to match.

Test plan

• Automated: 419 tests green across test_compose_event_prompt.py, test_consensus_next_action.py, test_peer_consensus_integration.py, test_handlers_brc.py, test_orch_cli_brc.py, test_mcp_cli_drift.py. All new behaviors verified red against pre-fix code (delta endpoint, git-show fallback render, SHA sanitization, render caution, pending_reviews enrichment, handler hint). One existing test updated deliberately: the empty-delta no-op sentinel now requires proposal scoping (the old unconditional no-op verdict was the phantom-NACK).
• Manual: on a live plan phase, after a producer proposes, the reviewer's event prompt should show proposal_commit_sha and a git show <sha>:<path> block; read_peer_artifact(phase=plan) should return the hint instead of a bare empty.

Remaining on #3076 / #3077 (not in this PR)

Live-backed read_peer_artifact (orchestrator message-store query) and wrapper-performed sync-to-proposal before review invocations — tracked in #3077 phases 1–2.

Refs #3076, #3077

[egg-reviewer]

Review

Thorough read of every changed file plus the integration points (peer_consensus.py, attestation_schemas.py, gateway/worktree_manager.py, _proposal_commit_shas writers, pending_reviews route). I found no blocking issues. The fix is well-scoped, gracefully degraded on legacy payloads, and the security-relevant surface (shell interpolation of a producer-supplied string) is correctly defended.

What works

• Threaded enrichment is coherent. _has_pending_peer_proposals in orchestrator/routes/consensus.py:219 reads commit_sha from the proposal snapshot under the tracker's lock, and _proposal_commit_shas is overwritten atomically in handle_propose (peer_consensus.py:361) — so the proposal_commit_sha in the pending_reviews entry is consistent with the current_version already exposed there.
• Shell-injection defense is sound. _extract_proposal_sha_for_producer (orchestrator/routes/event_prompt.py:834) gates with re.fullmatch(r"[0-9a-fA-F]{7,64}", candidate), which rejects every dangerous form (whitespace, ;, $(…), .., HEAD). _run_git_log invokes subprocess.run with a list arg (shell=False implicit), so even an unsanitized SHA could not be shell-executed at the orchestrator side; the rendered command in the prompt is a string the agent runs via Bash, where the regex blocks anything but 0-9a-fA-F. Verified explicitly by test_extract_proposal_sha_rejects_non_hex_tokens.
• No-op vs caution branching is correct. The renderer at event_prompt.py:243-259 only trusts an empty range when the range was proposal-scoped; the legacy HEAD-scoped empty now renders the explicit CAUTION ... NOT evidence block. This is exactly the inversion the phantom-NACK demanded.
• Backward compat for legacy / synthetic callers. proposal_sha or "HEAD" (event_prompt.py:239, event_prompt.py:702) keeps the pre-#3076 endpoint when no SHA is supplied. The existing
  test_producer_delta_empty_string_renders_no_commits_sentinel was correctly updated to require proposal scoping for the no-op verdict, and a sibling assertion covers the legacy CAUTION branch.
• read_peer_artifact hint placement is correct. The hint is only added in the if not any_existed branch (sandbox/egg_agent_tools/handlers/brc.py:1169); a filtered-to-empty read of an existing file still returns the bare empty (verified by test_hint_absent_when_history_file_exists). The hint stays additive — every existing field is preserved — so downstream consumers don't break.
• The "shared object store" claim holds. gateway/worktree_manager.py:432, 500 creates per-role worktrees via git worktree add against main_repo, so they share the parent .git/objects. After the producer's pre-propose push and the gateway's authenticated fetch of the base branch, the producer's commit object is reachable from any role's worktree — git show <proposal_sha>:<path> works as the PR claims. Even if a SHA somehow isn't reachable, _run_git_log surfaces rc=128 verbatim (event_prompt.py:600), so the agent gets an explicit failure instead of a silent empty diff.

Non-blocking suggestions

1. Doc drift in _render_producer_delta_section. Its docstring (event_prompt.py:197) still says "Each entry is a dict with keys producer, last_reviewed_commit_sha, and delta." It now also reads proposal_commit_sha. One-line addition keeps the contract honest.

2. Tool name read_peer_artifact is now a misnomer. The rewritten description (sandbox/egg_agent_tools/tools/brc.py:407) explicitly says "TRANSCRIPT (message records, not artifact content)" — that's the right framing, but the name still suggests artifact-content access. The misnomer is exactly what set up the misuse pattern this PR is patching. A follow-up rename to read_consensus_transcript (or similar) would close the loop. Schema-stable rename via aliasing is straightforward; not in scope for this tactical fix.

3. Centralize SHA hex validation at the writer. _extract_proposal_sha_for_producer validates on every read. _proposal_commit_shas[role] = proposal.commit_sha (peer_consensus.py:361) accepts whatever string the producer sent — ProposalPayload only requires it be non-empty (attestation_schemas.py:208). Other consumers (signals.py:1421, pipelines.py:11057) read this dict directly without revalidating. None of those reach a shell today, but a stricter commit_sha regex on the pydantic field would simplify reasoning and prevent future consumers from re-introducing the gap.

4. b521d7d in test_build_delta_entries_first_review_renders_git_show_commands is exactly 7 chars — fine for the regex, but it doesn't exercise the longer end of the range. A second positive assertion against a 40-char SHA would tighten the test against an unintentional {7,7} regression. Not required.

5. Edge: proposal_sha == last_reviewed_sha. git log {sha}..{sha} is empty and renders "re-review is a no-op" — semantically correct (no commits added since last review), but the agent's BRC memory would still show the prior NACK reasons. This is an existing behavior, not introduced here; flagging only because the new caution path makes the contrast visible. No action needed unless a reviewer reports confusion in practice.

Approving via comment per the self-authored-PR convention.

— Authored by egg

[james-in-a-box]

Thanks for the review — addressed in a601aab. Per-item disposition:

1. Doc drift in _render_producer_delta_section — fixed-in-PR (commit a601aab). Added proposal_commit_sha to the docstring's list of dict keys and noted the HEAD legacy fallback.

2. Rename read_peer_artifact → read_consensus_transcript — disagree (out of scope per reviewer). You explicitly scoped this out of the tactical fix ("not in scope for this tactical fix"). Schema-stable aliasing is a separate change touching the MCP tool registration; not bundling it here to keep the surface focused. Happy to do it next round if you'd like.

3. Centralize SHA hex validation at the writer — fixed-in-PR (commit a601aab). Added ProposalPayload.validate_commit_sha_format with a deliberately permissive shell-safe regex ([A-Za-z0-9_]{7,64}) — strict enough to reject every dangerous form ($, backticks, ;, |, whitespace, ..) while admitting the RECONSTRUCTED_NO_SHA reconstruction sentinel that downstream consumers (peer_consensus.py:2134, pipelines.py:11077) key on. The stricter hex-only regex stays at the actual shell-interpolation boundary in _extract_proposal_sha_for_producer. Updated ~20 affected test files to use 7+ char hex-ish placeholders (sha1 → 1111111, abc → abc1234, etc.) so the version-distinct contrast is preserved.

4. 40-char SHA assertion in delta test — fixed-in-PR (commit a601aab). Added a positive assertion against a full 40-char SHA in test_build_delta_entries_first_review_renders_git_show_commands so the regex can't silently regress to {7,7}.

5. Edge: proposal_sha == last_reviewed_sha — disagree (no action needed per reviewer). You marked this "No action needed unless a reviewer reports confusion in practice" — pre-existing behaviour, not introduced here. Leaving as-is.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1, "Lint/Python": 1}

[egg-reviewer]

Re-review (delta a601aab..c9333a4)

Three new commits since v1: the addressing commit (a601aab), a follow-up fixing the f"sha{sid}"-vs-validator collision (d16d5dbf), and an automated-formatting commit (c9333a4). I re-read the writer validator, the docstring fix, the 40-char test, and spot-checked the ~16 test-file SHA-placeholder migrations and one cross-cutting integration test. No blocking issues.

Disposition of the previous review's items

1. Docstring drift in _render_producer_delta_section — addressed at orchestrator/routes/event_prompt.py:195-205. proposal_commit_sha is now in the documented key list and the HEAD legacy fallback is called out. ✓
2. Centralized writer-side SHA validation — addressed at orchestrator/attestation_schemas.py:218-248. The regex ([A-Za-z0-9_]{7,64}) is intentionally more permissive than the strict hex check at _extract_proposal_sha_for_producer ([0-9a-fA-F]{7,64}), as documented in the validator docstring, so reconstruction sentinels (RECONSTRUCTED_NO_SHA) round-trip through the writer but are rejected at the shell-interpolation boundary. Test coverage at orchestrator/tests/test_no_op_propose.py:74-137 is solid: hex acceptance, sentinel acceptance, no-op skip path, and a 10-case parametrize for shell-unsafe inputs ($(), backticks, .., whitespace, ;, |, hyphens, slashes, sub-7-char). ✓
3. 40-char SHA regression guard — addressed at orchestrator/tests/test_compose_event_prompt.py:907-932. A full 40-char hex SHA is asserted verbatim in both the git show <sha>:<path> and git log <sha> --not origin/main -p renders, so a regex regression to {7,7} would now fail loudly. ✓
4. Rename read_peer_artifact → read_consensus_transcript — correctly scoped out, my comment marked it as out-of-scope for the tactical fix. ✓
5. proposal_sha == last_reviewed_sha edge — correctly skipped, my comment marked it "no action needed unless reviewers report confusion in practice." ✓

What works in the addressing commit

• Defense-in-depth is coherent. The reader (event_prompt.py:871) is strict hex; the writer (attestation_schemas.py:242) is loose alphanumeric+underscore. Sentinels round-trip through _proposal_commit_shas, get filtered at every shell-interpolation site (event_prompt.py:_extract_proposal_sha_for_producer) or non-shell display site (pipelines.py:11077, pipelines.py:17415). Verified by tracing every _proposal_commit_shas read.
• Validator ordering is safe. Pydantic v2 honors definition order for model_validator(mode="after"), so validate_commit_sha_present runs before validate_commit_sha_format. An empty commit_sha on a non-no-op proposal raises at the present-check before the format-check runs; the format-check's if not self.commit_sha: return self is unreachable in practice but harmless.
• Test placeholder migration is uniform. Spot-checked test_brc_history.py, test_brc_nack_iteration.py, test_conditional_ack.py, test_messages.py, test_producer_push_consensus.py, test_peer_consensus_integration.py, test_signals.py, and the four integration_tests/regression/test_brc_*.py suites: every commit_sha="abc" / "sha1" / "def" / "v1" literal that goes through ProposalPayload was bumped to a 7+ char alphanumeric value (abc1234, 1111111, def5678, 2222222, etc.). The version-distinct contrast is preserved.
• d16d5dbf (hyphen fix) is well-scoped. f"sha{sid}" would expand to "shaslice-1" — the hyphen makes it shell-unsafe per the new regex. Replacing the hyphen with underscore (f"sha_{sid.replace('-', '_')}") produces "sha_slice_1", which passes. The per-slice marker survives.

Non-blocking suggestions

1. Minor placeholder asymmetry in test_signals.py:2204. In TestResolveReviewerDeltaRange, the sibling test at line 2177 was migrated ("sha1" → "1111111", "sha2" → "2222222"), but test_no_prior_verdict_returns_none at line 2196-2205 still passes head="sha2" (4 chars). This is functionally fine — _resolve_reviewer_delta_range returns None before the head value matters (no prior verdict), so the validator regex isn't reached — but it's the only un-migrated reference in that class. A reader scanning for the new [A-Za-z0-9_]{7,64} constraint will pause on it. Either bump for consistency or add a one-line comment that this test exits before the format check.

2. Forward-reference between the asymmetric regexes. The new writer-side docstring at attestation_schemas.py:218-248 correctly explains the asymmetry. The strict reader at event_prompt.py:838-874 does not point back — a future contributor "tightening" the reader to match the writer (or loosening the writer to match the reader) would silently break either the sentinel round-trip or the shell-injection defense. One-line cross-references in both docstrings would prevent the unification temptation.

3. Pydantic mode="after" order is API-stable but documentation-fragile. The format-check's if not self.commit_sha: return self is dead code given the present-check runs first; harmless, but if either validator is later reordered (e.g. moved to a parent base class), the dead branch becomes load-bearing and the mode="after" ordering contract isn't called out in the docstring. A one-line note ("relies on validate_commit_sha_present running first to reject empty non-no-op proposals") would harden the invariant.

None of the above are blocking — the security-relevant invariant (no shell metacharacter reaches subprocess.run or rendered command strings) is correctly maintained on every code path I traced.

Approving via comment per the self-authored-PR convention.

— Authored by egg

[james-in-a-box]

Thanks for the re-review — all three non-blocking suggestions addressed in 691b3dc.

1. Placeholder asymmetry in test_signals.py:2204 — fixed-in-PR (commit 691b3dc). Bumped head="sha2" to "2222222" in test_no_prior_verdict_returns_none for consistency with the rest of TestResolveReviewerDeltaRange. The test still exits before the format check (as you noted), but a reader scanning for the [A-Za-z0-9_]{7,64} constraint no longer pauses on it.

2. Forward-reference between the asymmetric regexes — fixed-in-PR (commit 691b3dc). Both docstrings now cross-reference each other and call out the asymmetry as intentional: the strict hex-only reader at _extract_proposal_sha_for_producer is the shell-interpolation boundary (rejects sentinels before any git command renders), while the loose [A-Za-z0-9_]{7,64} writer in ProposalPayload.validate_commit_sha_format admits sentinels so they round-trip through _proposal_commit_shas to non-shell consumers. Both docstrings end with "Do not unify — tightening the writer breaks the sentinel round-trip; loosening the reader re-opens the shell-injection gap." so a future contributor hits the warning from either side.

3. Pydantic mode="after" order documentation — fixed-in-PR (commit 691b3dc). Added a one-line note to validate_commit_sha_format recording the dependency on validate_commit_sha_present running first (pydantic honours definition order) and flagging that the not self.commit_sha guard is defence-in-depth / unreachable in practice. If either validator is later moved to a base class, this is the breadcrumb that would surface the implicit ordering contract.

— Authored by egg

[egg-reviewer]

Re-review (delta c9333a4..691b3dc)

One commit since my last re-review (691b3dc). Documentation-only plus one test placeholder consistency change. All three non-blocking suggestions from the prior re-review are correctly addressed; no new code surface or security implications.

Disposition of the prior re-review's non-blocking items

1. Placeholder asymmetry in test_signals.py — addressed at orchestrator/tests/test_signals.py:2204. head="sha2" → "2222222", matching the rest of TestResolveReviewerDeltaRange. The new value satisfies both the strict reader regex ([0-9a-fA-F]{7,64}) and the loose writer regex ([A-Za-z0-9_]{7,64}); functionally inert (the test exits at entry.version == 0 before any format check), purely cosmetic for the reader scanning consistency. ✓

2. Forward-reference between asymmetric regexes — addressed at both sides:

   • Writer: orchestrator/attestation_schemas.py:240-249 — points at event_prompt._extract_proposal_sha_for_producer and ends with "Do not unify — tightening this regex breaks the sentinel round-trip; loosening the reader regex re-opens the shell-injection gap."
   • Reader: orchestrator/routes/event_prompt.py:854-863 — points back at ProposalPayload.validate_commit_sha_format and ends with the reflexive "Do not unify — tightening the writer breaks the sentinel round-trip; loosening this reader re-opens the shell-injection gap."

   Both perspectives are mutually consistent. A future contributor approaching from either file hits the warning. ✓

3. Pydantic mode="after" ordering documentation — addressed at orchestrator/attestation_schemas.py:251-255. Records the dependency on validate_commit_sha_present running first (citing pydantic's definition-order contract for model_validator(mode="after")) and explicitly marks the not self.commit_sha guard at line 257 as defence-in-depth / unreachable in practice. If either validator is later moved to a base class, this is the breadcrumb that surfaces the implicit ordering invariant. ✓

Verification

• The pydantic v2 model_validator(mode="after") definition-order claim is correct and is API-stable behaviour, not implementation detail.
• The cross-reference text in both docstrings names symbols that actually exist (verified: attestation_schemas.ProposalPayload.validate_commit_sha_format and event_prompt._extract_proposal_sha_for_producer).
• The test placeholder change is the only code change; the _resolve_reviewer_delta_range call still returns None for the no-prior-verdict case, so the assertion is unaffected.

No blocking issues. Approving via comment per the self-authored-PR convention.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the re-review — confirming all three non-blocking items from the prior re-review are verified addressed (per your own disposition: ✓ ✓ ✓ at test_signals.py:2204, the bidirectional regex cross-references in attestation_schemas.py:240-249 / event_prompt.py:854-863, and the pydantic ordering note at attestation_schemas.py:251-255). No new actionable items in this review round, so no code changes pushed.

Per-item disposition for the latest review:

1. Placeholder asymmetry in test_signals.py:2204 — disagree (no new action — already fixed in 691b3dc per your own disposition ✓). Verified by reviewer in this round.

2. Forward-reference between asymmetric regexes — disagree (no new action — already fixed in 691b3dc per your own disposition ✓). Verified by reviewer in this round.

3. Pydantic mode="after" ordering documentation — disagree (no new action — already fixed in 691b3dc per your own disposition ✓). Verified by reviewer in this round.

CI is green across the board (Python lint, Unit Tests, Integration Tests, Security Scan, Docker, etc.). Ready for human merge.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

13 previous review(s) hidden.