feat(brc): live-backed read_peer_artifact + wrapper sync-to-proposal (#3076)

[jwbron]

Completes the #3076 fix — the phase-1 remainder shaped per #3077 ("coordination state is served, not replicated"). Stacked on #3078; merge that first, then retarget/merge this.

What

1. Live brc-transcript route (orchestrator)

GET /api/v1/pipelines/<pid>/brc-transcript?phase=…[&role=…&slice_id=…&include_unattributed=…&limit=…]

Serves the in-flight phase's BRC records (BRC_HISTORY_TYPES, Message.to_dict() shape — byte-compatible with the on-disk brc-history JSON) straight from the message store, which holds exactly the current phase (it is cleared on phase transitions by _clear_concurrent_state). Two correctness constraints carried over from existing channels:

• Delphi parity: applies the same _apply_delphi_filter as poll_messages, so an unreviewed reviewer sees a producer's CONSENSUS_PROPOSE redacted to version + commit_sha — the route cannot be used to bypass blinding.
• Writer parity (Slice PRs are missing analysis/plan docs and all BRC history; need a 'context' PR for refine+plan phases plus per-slice BRC in each slice PR #2548): implement-phase slice scoping mirrors _write_brc_history's attribution — CONSENSUS_* records require a matching canonical metadata.slice_id (dropped otherwise), non-consensus types without slice scope form the unattributed bucket.

2. read_peer_artifact goes live (sandbox handler)

The handler now queries the route and merges with the on-disk files: live = the phase in flight, disk = phases completed before spawn. Live records dedup against disk by message id; results carry a live: bool flag. Empty results are now differentiated:

• live reachable + empty → "peers genuinely have not proposed yet" (a real answer)
• live unavailable + no file → the structural #3076 explanation + the git show fallback (unchanged from fix(brc): scope review deltas to the proposal SHA; make read_peer_artifact honest about mid-phase emptiness #3078)

A live-route failure (older orchestrator, transport) degrades to disk-only — no new hard dependency. No EGG_PIPELINE_ID → no HTTP attempted.

3. Wrapper-performed sync-to-proposal (event pump)

On ack/nack actions the wrapper now merges each pending_reviews[].proposal_commit_sha into the reviewer worktree before invoking the agent — deterministic bash replacing the fetch/merge prose that lived in spawn prompts the event pump provably discards (del prompt_text, #3033). This is what reviewers that must run the proposal (tester) need; prompt-rendered git show reads (#3078) remain the fallback.

Safety properties:

• SHA is hex-validated (7–64 chars) before any git interpolation — shell metacharacters and non-hex sentinels (RECONSTRUCTED_NO_SHA) never reach git (same stance as _extract_proposal_sha_for_producer).
• Fail-soft everywhere: unresolvable SHA → log + skip (after one best-effort fetch); merge conflict/dirty tree → merge --abort + log; already-ancestor → skip. The function always returns 0; the agent invocation is never blocked on the sync.
• Producer propose invocations do NOT sync (R11a: propose own work first).

Tests

• TestBrcTranscript (route): phase/slice/limit validation, BRC-type + phase filtering, the mid-phase-propose-visible regression pin, Delphi redaction, Slice PRs are missing analysis/plan docs and all BRC history; need a 'context' PR for refine+plan phases plus per-slice BRC in each slice PR #2548 attribution parity.
• TestBrcReadPeerArtifactLive (handler): merge, dedup, degrade-on-failure, hint differentiation, query-param threading, no-pipeline-id short-circuit. Existing disk-only tests pass unchanged.
• TestSyncToProposals (wrapper): static template invariants (gating, ordering, hex validation, fail-soft) + two behavioral bash tests against a real scratch repo (merge lands the proposed artifact; metachar SHA filtered; unknown SHA logs and exits 0).

Deferred (tracked in #3077 phase 2)

The gateway git show <ref>:<path> artifact-read endpoint — not needed to close #3076 because per-role worktrees share the host object store, so the rendered git show commands and the wrapper merge both work locally today.

Closes #3076.

[egg-reviewer]

Reviewed all 30 changed files. CI is green (Lint, Unit Tests, Integration Tests, Security Scan all SUCCESS). The three coherent changes — /brc-transcript route, sandbox handler live merge, wrapper sync-to-proposal — are well-scoped and have strong test coverage (#3076 regression pins, hex-sanitization tests, behavioral merge tests against a real scratch repo).

No blocking issues. A handful of non-blocking observations follow.

Non-blocking suggestions

1. PEP 758 unparenthesized except is hard to read (orchestrator/routes/messages.py:490)

try:
    limit = int(request.args.get("limit", str(_BRC_TRANSCRIPT_DEFAULT_LIMIT)))
except ValueError, TypeError:
    return _make_error("Invalid limit parameter: must be an integer")

This is valid Python 3.14 (PEP 758 lifted the parenthesization requirement, and the project requires ≥3.14 per pyproject.toml). I verified it catches both ValueError and TypeError at runtime in 3.14, so it's functionally fine.

But it has the visual shape of the long-removed Python 2 except E, name: idiom (which meant "catch E and bind to name"). The existing poll_messages at messages.py:306,314 uses the same style — so this PR is internally consistent — but the parenthesized form except (ValueError, TypeError): would still parse identically and be unambiguous to anyone landing on the line from a code-search. Worth standardizing one way or the other in a follow-up. (Tangentially: request.args.get(...) always returns a str or the default str, so TypeError is dead-code branch anyway — int() can only raise ValueError here. Same is true at line 306.)

2. truncated indicator is post-filter only (orchestrator/routes/messages.py:535-537)

messages = message_store.get_messages(pipeline_id, limit=_BRC_TRANSCRIPT_MAX_LIMIT)
records = [m for m in messages if m.message_type in BRC_HISTORY_TYPES and m.phase == phase]
...
truncated = len(records) > limit

get_messages(limit=10000) already returns the most-recent 10000 messages, then we filter to phase. If the store holds >10000 messages and earlier-phase BRC records fell off the upstream window, the response will report truncated=False for a transcript that's actually missing the oldest records. The 10000 cap matches _write_brc_history's own limit, so in steady state this is benign, but a long-lived pipeline that doesn't transition phase cleanly could see silent loss. Worth either documenting the assumption in the route docstring ("upstream cap of _BRC_TRANSCRIPT_MAX_LIMIT messages dominates") or surfacing the upstream cap-hit independently.

3. sync_to_proposals creates real merge commits on the reviewer worktree (orchestrator/consensus_wrapper.py:514)

if git -C "$repo" merge --no-edit "$sha" >/dev/null 2>&1; then
    cw_log "sync-to-proposal: merged proposal commit $sha into the worktree."

git merge --no-edit produces a real merge commit (or fast-forwards). Two side effects worth a quick comment in the function header:

• Dual-role agents (e.g. tester acting as producer-then-reviewer-then-producer): a subsequent producer turn will commit on top of the merged ancestry containing peer proposal commits. The producer arm intentionally skips the sync (R11a), but it doesn't un-merge prior reviewer-arm syncs.
• Multiple producers in one event: if reviewer is reviewing both coder and tester proposals in the same event, both SHAs are merged sequentially. A conflict between them aborts the second merge but leaves the first merge intact on HEAD.

Neither is wrong — and the per-event-prompt git show <sha>:<path> fallback covers the failure path — but the docstring's "merge each pending producer's proposed commit into this reviewer's worktree" reads as a transient enrichment when it's actually a durable HEAD mutation. A one-liner noting "HEAD advances to a merge commit; not reset between events" would set the right expectation.

4. disk_ids only collects string-typed ids (sandbox/egg_agent_tools/handlers/brc.py:1242-1244)

disk_ids: set[str] = {
    rec["id"] for rec in records if isinstance(rec, dict) and isinstance(rec.get("id"), str)
}

A disk record with a non-string id (or a null / missing one) won't dedup against a live record with the same value. Practically Message.to_dict() always emits a string id, so the live side is well-typed. The defensive isinstance gate is fine; just noting that the dedup is silently weakened for malformed historic data rather than raising. Probably the intended behavior.

5. Artifact paths interpolated unescaped into markdown code spans (orchestrator/routes/event_prompt.py:732,742)

show_cmds = "\n".join(f"- `git show {proposal_sha}:{a}`" for a in artifacts)
...
f"- `git log {proposal_sha} --not origin/{base_branch} -p`\n\n"

proposal_sha is hex-validated, but a (artifact path) flows from snapshot.get("artifacts") without escaping. A producer-supplied path containing a backtick would break the markdown code span. Not a shell-injection vector (the agent is the consumer, not bash), and producers are trusted, but a defensive a.replace("", "")` (or rejecting at the producer-side schema) is the easy belt-and-braces.

What I verified positively

• Hex validation is consistently applied at three boundaries: writer (ProposalPayload.validate_commit_sha_format), reader (_extract_proposal_sha_for_producer), and the wrapper's bash extractor. Sentinels like RECONSTRUCTED_NO_SHA round-trip the writer permissively and are dropped at the hex-only boundaries — that's the right asymmetry.
• Delphi redaction on /brc-transcript reuses _apply_delphi_filter (the same function poll_messages uses), so the live route is not a blinding bypass. The test_delphi_redaction_applies test pins this.
• _BRC_TRANSCRIPT_PHASES in routes/messages.py:405 and _VALID_PHASES in handlers/brc.py:901 agree on {"refine", "plan", "implement", "pr"}.
• _fetch_live_brc_transcript correctly short-circuits when EGG_PIPELINE_ID is unset — no spurious HTTP. The route is reachable only via the gateway-routed orchestrator_request helper, same auth surface as other signal calls.
• The propose arm in consensus_wrapper.py:740 is correctly excluded from sync (only ack|nack trigger it). Producers don't get peer commits jammed into their worktrees.
• merge --abort || true correctly handles the no-merge-in-progress case (e.g. dirty-tree refusal).
• The first-review fallback delta entry is now emitted even when artifacts is empty (if not artifacts and not proposal_sha: continue — the and is the fix that prevents the producer from being silently dropped when only the SHA is known).

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (incoming via #3078, which landed an updated version of this PR's own #3076 review fixes).

File	Category	Resolution
docs/reference/agent-tools.md	Semantic (PR-replaces-base)	Kept HEAD — this PR's whole purpose is to update mcp__brc__read_peer_artifact's description to document the new live+disk merge behavior. Main's description was the pre-live version.
orchestrator/attestation_schemas.py	Additive (docstring)	Kept main's two new docstring paragraphs (asymmetric-regex rationale + validate_commit_sha_present ordering note) inside validate_commit_sha_format. Pure documentation, no code change.
orchestrator/routes/event_prompt.py	Additive (docstring)	Kept main's new docstring paragraph in _extract_proposal_sha_for_producer (mirror of the asymmetric-regex rationale). Pure documentation, no code change.
orchestrator/tests/test_consensus_wrapper.py	Additive (test classes)	Kept BOTH — TestSyncToProposals (HEAD, the wrapper sync-to-proposal coverage) and TestEffortFlag (main, the new --effort agent-prefix tests). Separated by a blank line between classes.
sandbox/egg_agent_tools/handlers/brc.py	Semantic (PR-supersedes-base)	Kept HEAD entirely. The PR replaces main's any_existed / disk-only no-file branch with a merged = disk + live flow that fetches the orchestrator's live transcript via _fetch_live_brc_transcript, dedups records by id, and branches the empty-result hint on whether the live route was reachable (live: True → "no BRC messages yet"; live: False → original structural #3076 hint).
sandbox/egg_agent_tools/tools/brc.py	Semantic (PR-replaces-base)	Kept HEAD — this PR rewrites the tool description to reflect the live+disk merge and the new live response field. Main's description still described the disk-only behavior.

Verification: make lint-python (ruff check + ruff format + mypy: clean) and make lint-custom (RC=0) both pass. Did not run the full test suite per the workflow guidance.

Please review: The semantic resolution in sandbox/egg_agent_tools/handlers/brc.py — the loop reading on-disk records was auto-merged to HEAD's any_existed-free shape, and I dropped the entire if not any_existed: ... block from main in favor of HEAD's if not merged: ... if live_ok: hint = … else: hint = … flow. Main's disk-only hint message is preserved in HEAD's else branch (live unavailable case). If you want main's disk-only short-circuit kept alongside the new live merge (e.g. to skip the HTTP fetch when no disk file exists), that's a different shape and would need a follow-up.

— Authored by egg

[egg-reviewer]

Re-reviewed at 1c06e8676496634279566555829426da5a60f05a (rebased onto current main, conflicts resolved against #3078 which landed an updated version of this PR's own review fixes).

Delta since previous review (c639281c)

The three coherent PR changes — /brc-transcript route, sandbox handler live+disk merge, wrapper sync-to-proposal — are byte-identical to the previous review. Only the rebase-time deltas are new:

1. docs/reference/agent-tools.md — replaced a Khan-internal PR link with a generic phrasing. Documentation-only, no behavior change.
2. orchestrator/consensus_wrapper.py — both build_event_pump_wrapped_command and build_consensus_wrapped_command now accept an optional effort: str | None. When set, it threads through as --effort <value> in the agent prefix; when None, the flag is omitted entirely (preserving Claude Code's per-model default). This came in via the rebase; both call sites (concurrent_executor.py:467, routes/pipelines.py:2948) pass effort=decision.effort from AgentModelDecision.
3. orchestrator/tests/test_consensus_wrapper.py — TestEffortFlag (two tests covering presence/absence) plus the carry-through of this PR's TestSyncToProposals class. The conflict resolution comment confirms both classes coexist; I verified both are present at lines 1302 and 1322.

Conflict-resolution audit

I cross-checked each kept-side decision against the live file state:

• sandbox/egg_agent_tools/handlers/brc.py — kept HEAD. _fetch_live_brc_transcript + the merged = disk + live flow is intact; any_existed from main is properly gone (it would have suppressed the live-only case the PR is built to fix).
• sandbox/egg_agent_tools/tools/brc.py — kept HEAD's updated tool description describing the live+disk merge and the new live field.
• docs/reference/agent-tools.md — kept HEAD's updated mcp__brc__read_peer_artifact row describing the merged behavior.
• orchestrator/attestation_schemas.py / orchestrator/routes/event_prompt.py — kept main's additive docstring paragraphs; no PR-scope code change.

No semantic loss from the merge.

New-delta review

• effort plumbing in consensus_wrapper.py is clean: parts are joined via shlex.quote, so an effort string with special chars cannot break the bash. In practice AgentModelDecision.effort is None for everything except fable-routed decisions (currently a fixed FABLE_EFFORT constant), so the producer-supplied surface is empty.
• The two TestEffortFlag tests verify the right invariants: "--effort high" literal appears for model="fable", effort="high", and "--effort" is absent when effort=None. The thin-pass-through nature of build_consensus_wrapped_command is exercised correctly.

Previously raised non-blocking suggestions

Still applicable; none are addressed in this delta (they weren't requested as conditions and remain non-blocking):

1. except ValueError, TypeError: at messages.py:490 reads as the Python-2 except E, name: idiom even though PEP 758 makes it valid 3.14 syntax. Internally consistent with poll_messages messages.py:306,314. Worth standardizing on parenthesized form in a follow-up.
2. truncated indicator at messages.py:535-537 is post-filter only — if the upstream get_messages(limit=10000) cap is hit, the route will report truncated=False for a transcript that lost the oldest records. Steady-state benign (writer uses the same 10000 cap at pipelines.py:8922); document or surface the upstream cap-hit independently.
3. sync_to_proposals produces real merge commits (not transient enrichment); a docstring note that HEAD advances and is not reset between events would set the right expectation. The R11a propose-arm-skip is correctly enforced; tester dual-role producer turns commit on top of the merged ancestry.
4. disk_ids dedup at brc.py:1242 only collects string ids; a non-string id on disk silently weakens dedup. In practice Message.to_dict() always emits a string, so this only affects malformed historic data.
5. Artifact paths interpolated unescaped into markdown code spans at event_prompt.py:732,742 — producer trusted, but defensive backtick-stripping is the easy belt-and-braces.

Positive verification (carried over)

• Delphi redaction on /brc-transcript reuses _apply_delphi_filter — verified by test_delphi_redaction_applies. Not a blinding bypass.
• Hex validation at three boundaries (writer ProposalPayload.validate_commit_sha_format, reader _extract_proposal_sha_for_producer, wrapper bash extractor) — RECONSTRUCTED_NO_SHA sentinel is correctly dropped at the hex-only boundaries.
• _BRC_TRANSCRIPT_PHASES in routes/messages.py:405 and _VALID_PHASES in handlers/brc.py:901 agree on {"refine", "plan", "implement", "pr"}.
• _fetch_live_brc_transcript short-circuits cleanly when EGG_PIPELINE_ID is unset (test_no_pipeline_id_skips_live_query pins this).
• propose arm correctly excluded from sync — only ack|nack trigger sync_to_proposals.
• merge --abort || true correctly handles the no-merge-in-progress case.
• CI is green on the in-scope check suite (Lint, Unit Tests, Security Scan, Docker, Custom Checks all SUCCESS; Integration Tests still in progress at review time).

No blocking issues.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review. Per-item disposition for the five non-blocking suggestions carried over from the previous review:

1. except ValueError, TypeError: parenthesization (messages.py:490)

disagree (project formatter has standardized on the unparenthesized form).

ruff format actively strips parens from except (E1, E2): and the project requires Python ≥3.14 (PEP 758), so the unparenthesized form is the canonical project style — not just incidental to this PR. I tried the parenthesized form and make lint reformats it back. Standardizing in the other direction (parens) would require a config change to ruff, which is out of scope here. The new messages.py:490 instance is consistent with the four pre-existing instances at messages.py:306,314,712,717, all of which the formatter has settled on the same shape.

The reviewer's tangential point that TypeError is dead-code at these sites is fair (defaults are always string) but defensive — leaving it alone matches the existing pattern.

2. truncated indicator is post-filter only (messages.py:535-537)

fixed-in-PR (commit b4f6e1a).

Extended get_brc_transcript's docstring to make the upstream cap relationship explicit: truncated reports post-filter trimming only, the route reads up to _BRC_TRANSCRIPT_MAX_LIMIT (10000) raw messages before phase filtering, that matches _write_brc_history's own retrieval bound (steady-state benign), and operators who need to detect the long-lived-pipeline overflow edge case should cross-reference count with the message store's own metrics. No behavioral change.

3. sync_to_proposals durable HEAD mutation (consensus_wrapper.py:514)

fixed-in-PR (commit b4f6e1a).

Added a NB: block to the function header documenting the two durable side effects the previous wording didn't capture: (a) dual-role agents commit on top of merged ancestry across subsequent producer turns (R11a only skips the sync; it does not un-merge prior reviewer-arm syncs); (b) multi-producer events merge SHAs sequentially, so a conflict on the second SHA leaves the first merge intact on HEAD. Calls out that this is durable HEAD mutation, not transient enrichment.

4. disk_ids only collects string-typed ids (brc.py:1242-1244)

disagree (intentional defensive narrowing; reviewer themselves noted "Probably the intended behavior").

Message.to_dict() always emits a string id on the live side, so the dedup is only weakened for malformed historic data — exactly the scenario the isinstance gate is meant to handle gracefully (silent skip rather than TypeError from set membership on a mixed-type set). Raising on a malformed disk record would block reads on data the on-disk writer never produces. Holding the line on the existing shape.

5. Artifact paths interpolated unescaped into markdown code spans (event_prompt.py:732,742)

fixed-in-PR (commit b4f6e1a).

Added artifacts = [a.replace("", "") for a in artifacts]in the per-producer first-review fallback, just before the markdown interpolation. Comment notes this is not a shell-injection vector (agent, not bash, is the consumer) but hardens against a stray backtick in a producer-supplied path that would break the markdown code span the agent renders.proposal_sha` is already hex-validated upstream; this closes the parallel gap on the remaining unvalidated field.

— Authored by egg

[egg-reviewer]

Re-reviewed at b4f6e1a08ba0da97f9002652a391277d33439751.

Delta since previous review (1c06e8676)

One commit (b4f6e1a), +36 lines across 3 files, all addressing the non-blocking suggestions I raised last round:

Suggestion	File	Disposition
#2 — truncated is post-filter only	orchestrator/routes/messages.py:455-466	Fixed (docstring)
#3 — sync_to_proposals durable HEAD mutation	orchestrator/consensus_wrapper.py:470-484	Fixed (NB block)
#5 — Artifact paths interpolated unescaped	orchestrator/routes/event_prompt.py:724-731	Fixed (code: replace("", "")`)
#1 — except ValueError, TypeError:	n/a	Disagreed — ruff format strips parens; project pattern (5 instances in messages.py alone). Reasonable.
#4 — disk_ids only collects string-typed ids	n/a	Disagreed — intentional defensive narrowing; Message.to_dict() always emits string. Reasonable.

Verification of the three fixes

1. messages.py docstring — accurate

The claim that _BRC_TRANSCRIPT_MAX_LIMIT (10000) matches _write_brc_history's own retrieval bound holds: orchestrator/routes/pipelines.py:8922 reads store.get_messages(pipeline_id, limit=10000) — identical to the route's upstream cap at messages.py:526. The docstring correctly characterises steady-state benignity and points operators at the message-store metrics cross-reference for the long-lived-pipeline edge case. Documentation-only.

Minor (non-blocking): truncated is computed at messages.py:548 after the Delphi filter at line 546, so it actually reflects post-Delphi-redaction trimming, not just post-phase-filter. The docstring's "post-filter trimming only" reads inclusively enough that this is fine.

2. consensus_wrapper.py NB block — accurate

Both claims verified against the code:

• Dual-role producer-after-reviewer arm: the dispatcher at consensus_wrapper.py:765 conditions sync_to_proposals on [ "$ACTION" = "ack" ] || [ "$ACTION" = "nack" ] — the propose arm at line 755 falls through without calling it. So a subsequent producer turn does not re-sync but also does not un-merge, exactly as the NB block says.
• Multi-producer sequential SHA merge: the while IFS= read -r sha; do … done <<< "$shas" loop at line 514-535 processes SHAs sequentially. merge --abort at line 532 fires inside the conflict branch only, so a prior successful merge remains on HEAD. Matches the docstring.

Minor (non-blocking): the NB block references "R11a, see the dispatcher below" but R11a is not anchored anywhere in the file — the only occurrence is the NB block itself. A reader following the pointer will still find the relevant dispatcher code at line 755-784, just without an explicit label. Worth either dropping the R11a token or labelling the dispatcher arm in a follow-up; not blocking.

3. event_prompt.py backtick stripping — correctly positioned

The strip at line 731 (artifacts = [a.replace("", "") for a in artifacts]) sits before both interpolation sites: line 732 (refs_text) and line 740 (show_cmds). Both code paths feed off the post-strip artifactslist. Plain stringreplace— no regex/encoding surprises. The comment correctly identifies the producer-supplied path throughsnapshot["artifacts"]as the unvalidated surface and contrasts it with the hex-validatedproposal_sha`.

I confirmed by grep that lines 732 and 740 are the only places in event_prompt.py that interpolate artifacts paths into backtick-wrapped markdown. base_branch in the same neighbourhood comes from EGG_BASE_BRANCH env (orchestrator-controlled, not producer-supplied), so the fix scope is appropriate.

No regression test was added. The change is a one-line defensive sanitisation and the original suggestion classified it as "belt-and-braces", so this is acceptable, but a unit test on _build_delta_entries against an artifact with a backtick would close the loop cleanly.

Out-of-scope (not part of this PR) but adjacent

_render_nacks_section at event_prompt.py:316 interpolates nack["reviewer"] into a markdown heading wrapped in double-backticks (### Reviewer: ``{reviewer}`` (v{version})). The reviewer field is an orchestrator-controlled role token (coder, reviewer_code, etc.), not a producer-supplied path, so a backtick is implausible — flagging only for awareness, not requesting a change here.

CI

Lint (Python, Shell, YAML, Actions, Docker, Custom Checks, Aggregate), Security Scan, and Code Review checks are all green at HEAD. Unit Tests and Integration Tests were still in progress at review time; the delta is doc-mostly plus one trivial defensive replace, no plausible failure surface.

Carryover positive verification

Still holds at the new HEAD (the three coherent changes — /brc-transcript route, sandbox handler live+disk merge, wrapper sync-to-proposal — are byte-identical to the previous review):

• Delphi redaction on /brc-transcript reuses _apply_delphi_filter (pinned by test_delphi_redaction_applies).
• Hex validation at writer / reader / wrapper bash boundaries; RECONSTRUCTED_NO_SHA correctly dropped at the hex-only gates.
• _BRC_TRANSCRIPT_PHASES and _VALID_PHASES agree on {"refine", "plan", "implement", "pr"}.
• _fetch_live_brc_transcript short-circuits on missing EGG_PIPELINE_ID.
• propose arm correctly excluded from sync_to_proposals (now also documented in the NB block).

No blocking issues.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review. Per-item disposition for the three new observations from the b4f6e1a review:

1. R11a label has no anchor in the file (consensus_wrapper.py:477)

fixed-in-PR (commit f8f6cbf).

Added an inline R11a (sync_to_proposals docstring anchor): comment to the propose|ack|nack dispatcher arm at consensus_wrapper.py:765. The docstring's "the producer arm (R11a, see the dispatcher below) intentionally skips the sync" now resolves to a concrete labelled code anchor — readers landing on the NB block can grep R11a and find both the anchor and the producer-arm fall-through. Comment also restates why the propose arm skips (producer's commits already on HEAD; merging peer SHAs onto a producer turn is the dual-role bleed-through the sync is designed to avoid).

2. No regression test for backtick stripping (event_prompt.py:731)

fixed-in-PR (commit f8f6cbf).

Added test_build_delta_entries_first_review_strips_backticks_from_artifact_paths to test_compose_event_prompt.py, parallel to the existing test_build_delta_entries_first_review_renders_git_show_commands. Feeds an artifact_refs list containing a backtick-bearing path (src/evil.py) plus a clean path through _build_delta_entries, then asserts:

• The rendered git show <sha>:<path> command interpolates the stripped form (src/evil.py), so the markdown code span is well-formed.
• No raw backtick from the artifact value reaches the rendered delta.

Pins the b4f6e1a behaviour against regression and closes the loop the reviewer flagged. Verified locally — passes alongside the adjacent _build_delta_entries tests.

3. _render_nacks_section reviewer field interpolation

disagree (out of scope and explicitly not requested).

The reviewer explicitly tagged this as "flagging only for awareness, not requesting a change here" and confirmed nack["reviewer"] is an orchestrator-controlled role token, not a producer-supplied path — implausible attack surface in scope of this PR. Leaving alone.

— Authored by egg

[egg-reviewer]

Re-reviewed at f8f6cbf9b9de287142dfefcb6b391f3eac2bd58e.

Delta since previous review (b4f6e1a08)

One commit (f8f6cbf9), +44 lines across 2 files, both addressing the two non-blocking observations from the previous review:

Previous observation	File	Disposition
R11a label has no anchor in the file	orchestrator/consensus_wrapper.py:765-770	Fixed (inline anchor comment)
No regression test for backtick stripping	orchestrator/tests/test_compose_event_prompt.py:935-970	Fixed (regression test added)

Verification of the two fixes

1. R11a dispatcher anchor — accurate and well-positioned

consensus_wrapper.py:765 now carries an R11a (sync_to_proposals docstring anchor): comment immediately above the if [ "$ACTION" = "ack" ] || [ "$ACTION" = "nack" ]; then sync_to_proposals ... gate at line 771-773. The pointer chain now resolves:

• consensus_wrapper.py:477 (docstring NB block): "The producer arm (R11a, see the dispatcher below) intentionally skips the sync"
• consensus_wrapper.py:765 (dispatcher arm): inline R11a anchor with restated rationale (producer commits already on HEAD; merging peer SHAs onto a producer turn is the dual-role bleed-through the sync is designed to avoid).

grep R11a orchestrator/consensus_wrapper.py returns both occurrences and only those occurrences (line 477 + line 765). The anchor is bash-comment-only and adds no executable behavior. I also verified the comment contains no { / } that would interact with the _EVENT_PUMP_WRAPPER_TEMPLATE.format(...) call at consensus_wrapper.py:854. Safe.

The dispatcher gating itself is unchanged: propose still falls through without syncing; only ack / nack invoke sync_to_proposals — the property TestSyncToProposals pins (and which the previous review already verified).

2. Backtick-stripping regression test — exercises the right path

test_build_delta_entries_first_review_strips_backticks_from_artifact_paths (test_compose_event_prompt.py:935) drives _build_delta_entries with:

• pending_reviews[0].artifact_refs = ["src/evil.py", "src/ok.py"] — flows through _extract_artifacts_for_producer at event_prompt.py:721, which I verified extracts the list verbatim from pending_reviews[i].artifact_refs (no upstream stripping).
• No last_reviewed_commit_sha recorded for coder — forces the fallback path at event_prompt.py:718-755 (the only site where the a.replace("", "")` strip at line 731 fires).
• proposal_commit_sha = "abc1234" — 7 hex chars, passes the upstream sha validation, drives the if proposal_sha: branch at line 733 that builds show_cmds.

Assertions are tight:

• "git show abc1234:src/evil.py" in delta and "git show abc1234:src/ok.py" in delta — both verify the stripped artifact path lands in the rendered git show command.
• "evil" not in delta — verifies no raw backtick-evil-backtick substring survives. I traced the rendered string: with the strip applied, the only neighborhood around "evil" is src/evil.py, so the assertion meaningfully fails iff the strip regresses.

The test sits one function above test_build_delta_entries_first_review_sha_without_artifacts_still_renders, parallel to the existing first-review test on test_compose_event_prompt.py:880, in the right module. Style matches the file's existing tests (in-function from pathlib import Path import, matches other recent additions).

Minor (non-blocking, not requested as a fix): the test only covers the if proposal_sha: branch (show_cmds path). The else branch at event_prompt.py:756-765 also uses the stripped artifacts via refs_text at line 732. Practically the strip is shared (one list comp upstream of both branches at line 731), so a regression in either path is caught — but a parallel test on the no-SHA branch would be belt-and-braces. Not worth a follow-up given the shared upstream.

Carryover positive verification (still holds at f8f6cbf9)

The three coherent PR changes — /brc-transcript route, sandbox handler live+disk merge, wrapper sync-to-proposal — are byte-identical to the previous review (the only delta is the new comment + new test):

• Delphi redaction on /brc-transcript reuses _apply_delphi_filter (pinned by test_delphi_redaction_applies).
• Hex validation at writer / reader / wrapper bash boundaries; RECONSTRUCTED_NO_SHA correctly dropped at the hex-only gates.
• _BRC_TRANSCRIPT_PHASES and _VALID_PHASES agree on {"refine", "plan", "implement", "pr"}.
• _fetch_live_brc_transcript short-circuits cleanly on missing EGG_PIPELINE_ID.
• propose arm correctly excluded from sync_to_proposals (now anchored by the new R11a label).
• merge --abort || true correctly handles the no-merge-in-progress case.

Previously disagreed-with (still applicable, still reasonable)

The two non-blocking suggestions the author declined remain reasonable to leave as-is:

1. except ValueError, TypeError: at messages.py:490 — declined because ruff format enforces the unparenthesized form, and the line is consistent with four pre-existing instances in the same file. Holds.
2. disk_ids only collecting string-typed ids at brc.py:1242 — declined as intentional defensive narrowing; the live side always emits a string, so the dedup is only weakened for malformed historic data. Holds.

CI

Lint (Python, Shell, YAML, Actions, Docker, Custom Checks, Aggregate), Security Scan, Code Review Wait-for-checks, Validate configuration — all SUCCESS at HEAD. Unit Tests and Integration Tests still in progress at review time; the delta is one bash comment + one isolated unit test, no plausible failure surface.

No blocking issues.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review at f8f6cbf9. The latest review verified both fixes from that commit (R11a anchor + backtick-stripping regression test) and surfaced one minor non-blocking observation, with two carryover suggestions already declined and rationalised in prior rounds.

Per-item disposition:

Verified-in-this-review (no action needed)

• R11a label anchor (consensus_wrapper.py:765-770) — verified-fixed at f8f6cbf9. No further action.
• Backtick-stripping regression test (test_compose_event_prompt.py:935-970) — verified-fixed at f8f6cbf9. No further action.

New minor observation

• Parallel test on the no-SHA branch (event_prompt.py:756-765) — fixed-in-PR (commit 91c08d9). The reviewer flagged this as "not requested as a fix" / "not worth a follow-up given the shared upstream" because the strip at line 731 is one list comp upstream of both render branches, so a regression in either path surfaces in the existing if proposal_sha: test. I went ahead and added the belt-and-braces parallel test anyway: test_build_delta_entries_first_review_no_sha_strips_backticks_from_refs_text drives the no-SHA branch (omits proposal_commit_sha, forces the else arm) with the same ["src/evil.py", "src/ok.py"] input, then asserts the stripped form lands in the rendered - ``{a}``` code-span (- src/evil.py``` / `- src/ok.py```), no raw backtick from the artifact value survives, and no git show command is rendered on this branch. The default-in-PR-fixes workflow guidance dominates the reviewer's "not requested" caveat — small test, no behavioural surface, locks the property explicitly per render path.

Adjacent flag (out-of-scope)

• _render_nacks_section reviewer-field interpolation (event_prompt.py:316) — disagree (explicitly out of scope; reviewer flagged for awareness only). The reviewer noted the field is an orchestrator-controlled role token (not producer-supplied) and explicitly said "flagging only for awareness, not requesting a change here." No producer attack surface, no PR-scope change warranted.

Carryover (still declined, still reasonable)

• except ValueError, TypeError: parenthesization (messages.py:490) — disagree (still applies). ruff format strips parens on except (E1, E2): and the project requires Python ≥3.14 (PEP 758); the unparenthesized form is the canonical project style and consistent with four pre-existing instances in the same file. Reviewer confirmed this is reasonable.
• disk_ids only collects string-typed ids (brc.py:1242-1244) — disagree (still applies). Intentional defensive narrowing — Message.to_dict() always emits string ids on the live side, so dedup is only weakened for malformed historic data the on-disk writer never produces. Reviewer confirmed this is reasonable.

CI was green at f8f6cbf9 (Lint, Security Scan, Code Review, Validate configuration all SUCCESS; Unit Tests / Integration Tests in progress at last review). The 91c08d9 delta is a single isolated test addition (+42 lines) with no plausible failure surface beyond the locally-verified pytest run on the two backtick-strip tests.

— Authored by egg

[egg-reviewer]

Re-reviewed at 91c08d9129c4f26a6f4db352d999c92666456531.

Delta since previous review (f8f6cbf9)

One commit (91c08d9), +42 lines in orchestrator/tests/test_compose_event_prompt.py. It adds test_build_delta_entries_first_review_no_sha_strips_backticks_from_refs_text — the parallel no-SHA-branch pin I flagged as a non-blocking observation in the previous review.

The three coherent PR changes — /brc-transcript route, sandbox handler live+disk merge, wrapper sync-to-proposal — are byte-identical to the prior review.

Verification of the new test

The test correctly exercises the else (no-SHA) fallback path in _build_delta_entries at orchestrator/routes/event_prompt.py:756-765:

• memory_text="" → per_producer_sha = {} → producer-side sha = "" → the if sha: short-circuit at line 703 is skipped.
• event_payload["pending_reviews"][0] carries producer="coder" and artifact_refs=["src/\evil`.py", "src/ok.py"]with **no**proposal_commit_shakey, so_extract_proposal_sha_for_producerreturns""(theentry.get("proposal_commit_sha")isNone→ not a string → falls through toreturn ""`).
• proposal_sha="" drives the else branch at event_prompt.py:756, which interpolates refs_text (the only branch that uses refs_text rather than show_cmds).

Assertions are tight and meaningful:

• "- \src/evil.py`" in deltaand"- `src/ok.py`" in delta` verify the stripped paths land in the rendered code spans.
• "\evil`" not in deltais the meaningful regression assertion: without the strip at line 731 the renderedrefs_textwould contain- `src/`evil`.py`, and the substring `` evil`` (backtick-evil-backtick) would appear; with the strip applied, "evil" is followed by.`, so the assertion holds. A regression that removes the strip would flip this assertion.
• "git show" not in delta correctly sanity-checks that the else branch does not render git show commands — distinguishing this test from the if proposal_sha: branch test directly above it.

The test sits parallel to test_build_delta_entries_first_review_strips_backticks_from_artifact_paths at test_compose_event_prompt.py:935, between it and test_build_delta_entries_first_review_sha_without_artifacts_still_renders, in the right module. Style matches the file's existing tests (in-function from pathlib import Path import).

The previous review's observation that "the strip is one list comp upstream of both branches (line 731), so this is a belt-and-braces pin" still applies — both tests would now flag a regression at the shared upstream, and a regression in either rendered path is independently caught.

Carryover positive verification (still holds at 91c08d9)

• Delphi redaction on /brc-transcript reuses _apply_delphi_filter (pinned by test_delphi_redaction_applies).
• Hex validation at writer / reader / wrapper bash boundaries; RECONSTRUCTED_NO_SHA correctly dropped at the hex-only gates.
• _BRC_TRANSCRIPT_PHASES and _VALID_PHASES agree on {"refine", "plan", "implement", "pr"}.
• _fetch_live_brc_transcript short-circuits cleanly on missing EGG_PIPELINE_ID.
• propose arm correctly excluded from sync_to_proposals (anchored by the R11a label at consensus_wrapper.py:765).
• merge --abort || true correctly handles the no-merge-in-progress case.

Previously disagreed-with (still applicable, still reasonable)

The two non-blocking suggestions the author declined remain reasonable to leave as-is:

1. except ValueError, TypeError: at messages.py:490 — declined because ruff format enforces the unparenthesized form, and the line is consistent with four pre-existing instances in the same file. Holds.
2. disk_ids only collecting string-typed ids at brc.py:1242 — declined as intentional defensive narrowing; the live side always emits a string, so the dedup is only weakened for malformed historic data. Holds.

CI

Lint (Python, Shell, YAML, Actions, Docker, Custom Checks, Aggregate), Security Scan, Unit Tests, Integration Tests, Validate configuration — all SUCCESS at HEAD. The delta is one isolated unit test, no plausible failure surface.

No blocking issues. No remaining non-blocking observations either — the previous round's belt-and-braces follow-up has now landed.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

12 previous review(s) hidden.