docs: Update concurrent execution docs for BRC protocol [doc-updater]

[james-in-a-box]

Update documentation to reflect changes from 84968ec (Replace consensus with peer Broadcast-Review-Converge protocol #1122):

• docs/guides/concurrent-execution.md: Replaced the ConsensusEvaluator READY-tallying description with the new BRC (Broadcast-Review-Converge) protocol. Documents the PROPOSED/REVIEWING/CONFIRMED state machine, the propose/ack/nack/withdraw/confirmed flow, egg-orch consensus CLI commands, new EGG_BRC_* environment variables injected into concurrent agents, new CONSENSUS_* message types, and the Redis Streams message store backend. Also notes long-poll support (?wait=<s>) on the messages endpoint.
• docs/development/STRUCTURE.md: Added the 5 new orchestrator modules (approval_matrix.py, attestation_schemas.py, peer_consensus.py, redis_message_store.py, review_graph.py) and updated the message_store.py and consensus.py descriptions to reflect their new roles.

Triggered by: #1122

Issue: none

Test plan:

• Review that the BRC protocol flow in concurrent-execution.md matches the implementation in orchestrator/peer_consensus.py
• Verify egg-orch consensus command signatures match sandbox/egg_lib/orch_cli.py
• Verify new STRUCTURE.md entries match files present in orchestrator/

Authored-by: egg

[egg-reviewer]

Documentation Review: BRC Protocol Docs Update

The STRUCTURE.md changes are accurate — all 5 new files exist with correct descriptions, and the updated consensus.py and message_store.py entries match their current roles. The CLI command documentation (egg-orch consensus propose/ack/nack/withdraw/confirmed/status) also matches the implementation in orch_cli.py. The BRC protocol flow, environment variables, and message types are all verified correct.

However, the consensus status JSON example has 3 inaccuracies that will mislead agents trying to parse the status response.

---

Blocking: Consensus JSON example does not match the actual API response

File: docs/guides/concurrent-execution.md (new JSON example block)

The example shows:

{
  "is_complete": false,
  "blocking_agents": ["tester"],
  "has_objections": false,
  "agents": {
    "coder": {"phase": "PROPOSED", "confirmed": false},
    "reviewer_code": {"phase": "REVIEWING", "confirmed": false}
  },
  "approval_matrix": {
    "coder": {"reviewer_code": "ACK", "reviewer_contract": "pending"}
  }
}

Issue 1: phase field does not exist — actual fields are producer_phase and reviewer_phase

PeerConsensusTracker.evaluate() at peer_consensus.py:507-517 builds per-agent data with separate producer_phase and reviewer_phase keys (since agents can be both). There is no single phase field. The correct example:

"agents": {
    "coder": {"producer_phase": "PROPOSED", "confirmed": false},
    "reviewer_code": {"reviewer_phase": "REVIEWING", "confirmed": false}
}

An agent that reads this doc and tries to access response.agents.coder.phase will get undefined instead of the actual state.

Issue 2: approval_matrix is not in the status endpoint response

While PeerConsensusTracker.evaluate() returns approval_matrix (line 524), the status endpoint at pipelines.py:1010-1015 explicitly constructs the response with only agents, is_complete, blocking_agents, and protocol. The approval_matrix is filtered out. Remove it from the example.

Issue 3: has_objections is not in the status endpoint response

Same filtering issue — evaluate() returns it (line 522, always False for BRC), but the status endpoint drops it. Remove from the example.

The correct response structure matching pipelines.py:1010-1015:

{
  "is_complete": false,
  "blocking_agents": ["tester"],
  "protocol": "brc",
  "agents": {
    "coder": {"producer_phase": "PROPOSED", "confirmed": false},
    "reviewer_code": {"reviewer_phase": "REVIEWING", "confirmed": false}
  }
}

---

Non-blocking suggestions

1. propose example could show --risk: The attestation requirements (documented in CLAUDE.md) require producers to include "one risk considered" in proposals. The propose CLI accepts --risk (orch_cli.py:1687), but the example omits it. Consider:

   egg-orch consensus propose --summary "Implemented feature X" \
     --artifacts src/feature.py --risk "No retry on transient failures"

2. Long-poll fallback behavior: The docs note ?wait=<s> for long-poll but don't mention that long-poll only works with the Redis Streams backend. The in-memory store silently falls back to non-blocking poll (messages.py:166-169 catches TypeError and strips wait). Worth a brief note so agents don't wonder why long-poll doesn't block in test environments.

3. protocol field: The response includes "protocol": "brc" (or "readiness" for legacy). Since this field helps agents determine which response shape to expect, consider documenting it in the JSON example.

— Authored by egg

[james-in-a-box]

Contract Verification: PR #1123

Contract Status

No contract exists for PR #1123. This is a doc-updater PR triggered by PR #1122 with Issue: none. The parent contract for issue #1110 covers sandbox/.claude/rules/mission.md (task-5-1) but does NOT include docs/guides/concurrent-execution.md or docs/development/STRUCTURE.md in any task's files_affected. These documentation files fall outside the contract scope.

Since there is no contract to verify against, this review instead verifies documentation accuracy against the implementation.

---

STRUCTURE.md — VERIFIED

All 5 new module entries match files present in orchestrator/:

• approval_matrix.py — exists, description accurate
• attestation_schemas.py — exists, description accurate
• peer_consensus.py — exists, description accurate
• redis_message_store.py — exists, description accurate
• review_graph.py — exists, description accurate

Updated descriptions for consensus.py ("Legacy READY-tallying consensus, deprecated, kept for transition") and message_store.py ("Redis Streams when available, in-memory fallback") both verified against source.

concurrent-execution.md — VERIFIED WITH ISSUES

Accurate (verified against implementation):

• BRC protocol flow (propose → ack/nack → converge) matches peer_consensus.py handlers
• ConsensusPhase enum states (WORKING, PROPOSED, REVIEWING, CONFIRMED) match peer_consensus.py:44
• max_flip_flops default of 3 matches DEFAULT_MAX_FLIP_FLOPS = 3 in peer_consensus.py:53
• handle_agent_crash() exists and behavior matches docs
• Environment variables (EGG_BRC_ROLE_TYPE, EGG_BRC_REVIEWERS, EGG_BRC_PRODUCERS) verified in concurrent_executor.py:118-124
• All 5 new CONSENSUS_* message types verified in both message_store.py:28-32 and shared/egg_orchestrator/types.py:65-69
• EGG_MESSAGE_STORE_BACKEND selection logic ("auto"/"redis"/"memory") verified in message_store.py:185-208
• Long-poll ?wait=<s> parameter verified in routes/messages.py:148-162 (clamped 0-60s)
• CLI commands (propose, ack, nack, withdraw, confirmed, status) all exist in orch_cli.py
• --wait flag on message poll verified in orch_cli.py:1623

Issue 1 — Agent state key name mismatch (lines 227-228):

The example JSON response shows:

"coder": {"phase": "PROPOSED", "confirmed": false}

But PeerConsensusTracker.evaluate() at peer_consensus.py:509-515 returns:

"coder": {"producer_phase": "PROPOSED", "confirmed": false}

The implementation uses "producer_phase" and "reviewer_phase" (separate keys for dual-role agents like tester), not a single "phase" key. This also affects the CLI at orch_cli.py:1288 which reads agent_data.get("phase", "unknown") — it would display phase=unknown for all agents since the key doesn't exist. This appears to be a bug in both the docs and the CLI inherited from PR #1122.

Issue 2 — approval_matrix and has_objections not in API response (lines 222-234):

The example response includes has_objections and approval_matrix fields, but _get_concurrent_status() in routes/pipelines.py:1010-1014 only passes through agents, is_complete, blocking_agents, and protocol. The has_objections and approval_matrix fields from evaluate() are dropped at the route level.

Additionally, the documented approval_matrix format:

{"coder": {"reviewer_code": "ACK", "reviewer_contract": "pending"}}

doesn't match ApprovalMatrix.to_dict() which returns:

{"entries": {"reviewer_code->coder": {full_entry_dict}}, "proposal_versions": {...}, "revision_counts": {...}}

Issue 3 — protocol field missing from docs:

The actual response includes "protocol": "brc" but this isn't shown in the example JSON.

---

Verdict

The high-level BRC protocol documentation is excellent — protocol flow, state machines, CLI commands, environment variables, and message types are all accurate. The STRUCTURE.md changes are correct.

However, the example API response format (lines 221-234) has three inaccuracies that could mislead consumers of the status endpoint. These appear to stem from implementation inconsistencies in PR #1122 rather than documentation errors per se, but the docs should reflect reality.

Recommendation: Fix the example JSON to match actual _get_concurrent_status() output, or note that the format shown is the intended/planned format. At minimum, change "phase" to "producer_phase"/"reviewer_phase" since that's what the code produces.

— Authored by egg

[james-in-a-box]

Agreed. All three blocking issues fixed in b982c69:

Issue 1 (phase key mismatch): Changed to producer_phase/reviewer_phase in the JSON example. Also fixed the CLI at orch_cli.py:1284-1301 which had the same bug — it was reading agent_data.get("phase") and would always return "unknown" for BRC. Now displays producer=PROPOSED and reviewer=REVIEWING as separate fields for dual-role agents.

Issue 2 (approval_matrix removed): Removed from the example — _get_concurrent_status() at pipelines.py:1010-1014 filters it out.

Issue 3 (has_objections removed): Removed from the example — same filtering.

Non-blocking suggestions addressed:

1. Added --risk "No retry on transient failures" to the propose example (matches attestation requirements in CLAUDE.md).
2. Added a note about long-poll fallback: in-memory store silently returns non-blocking responses.
3. Added "protocol": "brc" to the JSON example.

— Authored by egg

[egg-reviewer]

Re-review: All blocking issues fixed

All three blocking issues from my previous review at f2f5c3e have been correctly resolved in b982c69.

Previous blocking issues — verified fixed

1. phase → producer_phase/reviewer_phase (docs + CLI): The JSON example now shows producer_phase and reviewer_phase as separate keys, matching PeerConsensusTracker.evaluate() at peer_consensus.py:507-517. The CLI fix at orch_cli.py:1288-1301 correctly reads both keys and handles dual-role agents (displays both producer=X reviewer=Y).

2. approval_matrix removed from docs: Removed from the JSON example. Correct — _get_concurrent_status() at pipelines.py:1010-1014 filters it out.

3. has_objections removed from docs: Removed from the JSON example. Correct — same filtering.

Previous non-blocking suggestions — all addressed

• --risk added to propose example (matches CLAUDE.md attestation requirements)
• Long-poll fallback note added (accurate — messages.py:166-169 catches TypeError for in-memory store)
• "protocol": "brc" added to JSON example

Non-blocking observations

1. Objections section still references has_objections (line 239): The PR removed has_objections from the JSON example, but 4 lines below, the Objections section still says "…has_objections: true is returned." Since _get_concurrent_status() doesn't include this field in the response, the prose is now inconsistent with both the example and the implementation. Consider rephrasing to clarify where objections surface (e.g., orchestrator logs / HITL decisions) rather than implying the field is in the API response.

2. Dead approval_matrix and review_graph display code in CLI (lines 1303-1322): The CLI fix correctly handles producer_phase/reviewer_phase, but the function still has display code for approval_matrix (lines 1303-1313) and review_graph (lines 1315-1322). Since the API response doesn't include either field, these blocks never execute. Not harmful, but could be cleaned up since the PR is already modifying this function.

— Authored by egg

[james-in-a-box]

Contract Verification — Re-review (delta: f2f5c3e → b982c69)

Verdict: APPROVE — All changes since last review are correct and well-scoped.

Delta Summary

Commit b982c69 addresses prior review feedback with 3 changes:

1. docs/guides/concurrent-execution.md — Fixed consensus status JSON example to use producer_phase/reviewer_phase instead of phase; added protocol field; added --risk flag to propose CLI example; added note about long-poll fallback with in-memory store.
2. sandbox/egg_lib/orch_cli.py — Fixed cmd_consensus_status to read producer_phase/reviewer_phase from the BRC API response instead of the old phase key.
3. docs/development/STRUCTURE.md — No changes in this delta (unchanged since last review).

Verification Against Test Plan

Criterion	Status	Evidence
BRC protocol flow matches peer_consensus.py	Verified	peer_consensus.py:500-527 returns producer_phase/reviewer_phase/confirmed per agent, protocol: "brc", is_complete, blocking_agents — matches doc example
egg-orch consensus command signatures match orch_cli.py	Verified	--risk flag confirmed at orch_cli.py:1695; propose/ack/nack/withdraw/confirmed/status subcommands all present
STRUCTURE.md entries match files in orchestrator/	Verified	All 5 new files exist: approval_matrix.py, attestation_schemas.py, peer_consensus.py, redis_message_store.py, review_graph.py

Code Fix Verification (orch_cli.py)

The CLI fix correctly handles the BRC response shape:

• Reads producer_phase and reviewer_phase independently (agents may have one or both)
• Falls back to "phase=unknown" only when neither is present
• Correctly appends [CONFIRMED] based on the confirmed field

Minor Note (Non-blocking)

The doc's JSON example omits has_objections, approval_matrix, and review_graph fields that are present in the actual PeerConsensusTracker.evaluate() response (peer_consensus.py:519-527). This is acceptable as a simplified example, but worth noting: has_objections always returns False for BRC, approval_matrix and review_graph are auxiliary debugging fields.

Regression Check

Previously verified aspects (STRUCTURE.md entries, BRC protocol flow, new env vars, message types) remain intact. No regressions detected.

— Authored by egg

[james-in-a-box]

Agreed. Both non-blocking observations fixed in 8ec401c:

Observation 1 (has_objections prose): Rephrased the Objections section to say the orchestrator "detects the objection and surfaces it to the human as a HITL decision" instead of claiming has_objections: true is returned in the API response.

Observation 2 (dead CLI display code): Removed the approval_matrix (lines 1303-1313) and review_graph (lines 1315-1322) display blocks from cmd_consensus_status — these never executed since the status endpoint doesn't include those fields.

— Authored by egg

[egg-reviewer]

Re-review: Both non-blocking observations fixed correctly

Commit 8ec401c addresses the two non-blocking observations from my previous review at b982c69. Both are correctly implemented.

Previous non-blocking observations — verified fixed

1. has_objections prose (line 239): Previously said "has_objections: true is returned" — now correctly says "the orchestrator detects the objection and surfaces it to the human as a HITL decision." Accurate: OBJECTING is a readiness state (separate from BRC phases), and the orchestrator handles it through HITL, not through a field in the consensus API response.

2. Dead CLI display code removed: The approval_matrix (was lines 1303-1313) and review_graph (was lines 1315-1322) display blocks are gone from cmd_consensus_status. The function now cleanly displays is_complete, per-agent states (producer_phase/reviewer_phase), and blocking_agents — matching the actual API response shape from _get_concurrent_status() at pipelines.py:1010-1015.

New code in delta — no issues

The delta is small and surgical. No new concerns.

Non-blocking observation

handle_agent_crash() docs slightly overstate producer crash escalation (line 247-248): The docs say "If the crash requires escalation (e.g., a critical producer or reviewer is lost), the orchestrator logs a warning and creates a HITL decision." For reviewer crashes, escalation IS implemented when the crashed reviewer is the sole reviewer for a producer (peer_consensus.py:427-437). But for producer crashes, the current implementation is pass with a comment about future logic (peer_consensus.py:411-413). The "e.g., a critical producer" example implies producer crash escalation exists when it doesn't yet. Consider changing to "e.g., a sole reviewer is lost" to match what's currently implemented.

— Authored by egg

[james-in-a-box]

Contract Verification — Re-review (delta: b982c69 → 8ec401c)

Verdict: APPROVE — Both non-blocking suggestions from the prior review cycle are correctly addressed. No regressions.

Delta Since Last Review

Commit 8ec401c makes two targeted fixes:

1. Fixed has_objections prose (docs/guides/concurrent-execution.md:239)

Previous text referenced has_objections: true being "returned" — implying it's in the API response. New text correctly describes the behavior: the orchestrator detects the objection and surfaces it as a HITL decision. Verified against routes/pipelines.py:2627-2655 where has_objections triggers pipeline.add_decision() with "Override objections / Wait for resolution / Abort phase" options. The field is internal to PeerConsensusTracker.evaluate() and is filtered out by _get_concurrent_status() at pipelines.py:1010-1015.

2. Removed dead CLI display code (sandbox/egg_lib/orch_cli.py)

Removed 21 lines of approval_matrix (old lines 1303-1313) and review_graph (old lines 1315-1322) display blocks from cmd_consensus_status. These blocks read from consensus.get("approval_matrix") and consensus.get("review_graph") — fields that _get_concurrent_status() does not include in the API response. Confirmed dead code; removal is correct.

Regression Check

Previously verified aspects remain intact:

• JSON example: producer_phase/reviewer_phase keys, protocol: "brc" — unchanged ✓
• CLI phase key reading (orch_cli.py:1288-1301): producer_phase/reviewer_phase — unchanged ✓
• STRUCTURE.md entries — unchanged since initial commit ✓
• --risk flag in propose example — unchanged ✓
• Long-poll fallback note — unchanged ✓

Remaining Function Shape (cmd_consensus_status)

After the deletion, cmd_consensus_status displays: is_complete, per-agent producer_phase/reviewer_phase/confirmed, and blocking_agents. This matches the fields returned by _get_concurrent_status(). No dead code remains.

Concern

None identified. The changes are minimal, correct, and well-scoped. Both fix real inconsistencies between docs/CLI and the implementation.

— Authored by egg

[james-in-a-box]

Agreed. Fixed in 02c9f35: Changed the handle_agent_crash() example from "e.g., a critical producer or reviewer is lost" to "e.g., a sole reviewer is lost and no other reviewer can cover the producer" — matching the actual implementation where only reviewer crashes trigger escalation (peer_consensus.py:427-437), not producer crashes (peer_consensus.py:411-413 is a pass).

— Authored by egg

[james-in-a-box]

Contract Verification — Re-review (Incremental)

Previous review commit: 8ec401cf0da1ea4732925d6896352751866b9daf
Current HEAD: 02c9f35f4d13f80c2fe0ec731061715148009ebb

Delta Since Last Review

Single commit 02c9f35 — one-line fix to docs/guides/concurrent-execution.md line 248:

Before: "If the crash requires escalation (e.g., a critical producer or reviewer is lost)"
After: "If the crash requires escalation (e.g., a sole reviewer is lost and no other reviewer can cover the producer)"

Verification Against Implementation

Verified against orchestrator/peer_consensus.py handle_agent_crash() (lines 405-443):

• The method checks if the crashed reviewer was the sole reviewer for any producer by iterating through assigned producers and checking if remaining_reviewers is empty.
• Escalation (action: "escalate") only triggers when a producer has zero remaining reviewers after the crash.
• The fix correctly narrows the parenthetical from the vague "critical producer or reviewer" to the precise condition actually checked by the implementation.

Verdict: Accurate. The new wording correctly describes the implementation's escalation logic.

Full PR Re-verification (Spot Check)

Re-confirmed all previously verified claims still hold:

1. BRC protocol flow (propose → review → converge → re-propose) matches peer_consensus.py methods
2. All 6 egg-orch consensus CLI commands exist in orch_cli.py with correct signatures
3. All 5 new orchestrator files exist (approval_matrix.py, attestation_schemas.py, peer_consensus.py, redis_message_store.py, review_graph.py)
4. JSON consensus response structure (producer_phase, reviewer_phase, confirmed, protocol) matches evaluate() return shape
5. EGG_BRC_* environment variables correctly set by concurrent_executor.py

Contract Status

No SDLC contract exists for this PR (issue #1027 not found). This is a standalone doc-update PR triggered by #1122, not part of an SDLC pipeline. Verification based on code-to-docs accuracy instead.

Result

Approve — No regressions from previous review. The new commit correctly fixes imprecise documentation to match the actual implementation.

— Authored by egg

[egg-reviewer]

Re-review: Non-blocking observation fixed correctly

Commit 02c9f35 addresses the non-blocking observation from my previous review at 8ec401cf. The fix is correct and matches the implementation.

Previous non-blocking observation — verified fixed

handle_agent_crash() docs (line 247-248): Previously said "e.g., a critical producer or reviewer is lost" — now correctly says "e.g., a sole reviewer is lost and no other reviewer can cover the producer." Verified against peer_consensus.py:405-443: producer crashes hit a pass with no escalation logic (lines 408-411), while reviewer crashes check for sole-reviewer status and escalate only when sole_reviewer_for is non-empty (lines 416-438). The docs now match the implementation.

Verification of caller behavior

Confirmed that concurrent_executor.py:259-276 handles the handle_agent_crash() result: when action == "escalate", it logs a warning (line 261-263), then always creates a HITL decision with the standard single-failure options (line 272-276). Both escalation and non-escalation paths produce a HITL decision — the escalation path adds a warning log. The docs accurately describe this: escalation "logs a warning and creates a HITL decision", non-escalation is "treated as a single-agent failure" (which also creates a HITL decision per the Failure Recovery section).

Full PR state — no remaining issues

Re-examined the complete PR diff across all three files:

• STRUCTURE.md: All 5 new module entries verified present. Updated descriptions for consensus.py and message_store.py accurate.
• concurrent-execution.md: BRC protocol flow, phase states, CLI commands, environment variables, message types, JSON example (producer_phase/reviewer_phase, protocol: "brc"), long-poll fallback note, --risk flag example, objections prose, and agent failure docs all verified against implementation.
• orch_cli.py: cmd_consensus_status correctly reads producer_phase/reviewer_phase independently, handles dual-role agents, falls back to phase=unknown for unrecognized shapes, and dead approval_matrix/review_graph display code removed.

No new concerns.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

12 previous review(s) hidden.