[issue-2908][slice-3/6] Delta-scoped re-analysis + prompt collapse - #2949
Conversation
… doc surface Documenter tasks for slice-3 of #2908: task-3-4: Rewrite the Concurrent Execution Mode section of sandbox/agent-config/rules/mission.md to the event-handler contract: the agent is invoked one-shot per actionable event by the BRC event-pump wrapper; act on the event, update BRC memory, exit naturally. Removes "never exit before the orchestrator stops you", the producer/reviewer STAY-ALIVE wording, and the explicit wait-loop / cursor plumbing — under the post-#2908 model the wrapper bash owns the lifecycle. Preserves Anti-Sycophancy, Structured Progress Reporting, HITL vs OVERSEER_ALERT, and Handling Agent Failures sections verbatim. A legacy-path note preserves the pre-flip contract until slice-4 flips the default. sandbox/claude-rules/mission.md is a symlink to sandbox/agent-config/rules/, so both paths reflect the change and the diff-empty acceptance assertion holds trivially. task-3-5: Extend docs/architecture/orchestrator.md and docs/reference/agent-wait-patterns.md with the slice-3 surface: the compose_event_prompt composer (10 KB envelope, 2 KB memory truncation, NACK payload from peer_consensus.py:949-1024, verbatim per-producer git-log delta scaled by change size), the full-delta adversarial re-review rationale (REVIEWER-SYNC.md contract + risk_analyst R6), tail-position memory delivery (architect od-6 Option B, sidesteps the non-existent --append-context flag), composer interplay with EGG_BRC_MEMORY across off / write-only / full modes, the _build_brc_preamble collapse (kept/removed table + the three caller sites unchanged), the sandbox-image rebuild trigger gating slice-4's flag flip, and the slice-3 unit/snapshot verification stance. Documents the architect's open-decision resolutions od-1 / od-2 / od-3 / od-4 / od-6 with their slice-1 / slice-2 / slice-3 implementation cites and explicitly marks od-5 as deferred to slice-6's MCP→CLI latency baseline. Cross-links between brc-memory.md (slice-1 writer), orchestrator.md (slice-3 architecture), and agent-wait-patterns.md §10.9 (slice-3 wait-side companion); brc-memory.md "How slice-3 reads it" now points to the landed reader-side sections. Refs #2908 tasks 3-4, 3-5
Three substantive corrections from reviewer_code's v1 ACK observations: 1. **Preamble collapse is unconditional, not gated by EGG_BRC_EVENT_PUMP.** The legacy-path note in mission.md previously implied the legacy wrapper still carried "the full event-blocking / cursor / persistent-session plumbing" in the preamble — but task-3-3 collapses _build_brc_preamble unconditionally, so both wrapper paths see the collapsed preamble. The legacy wrapper re-supplies wait / restart instructions through its own recovery system prompt baked into _CONSENSUS_WRAPPER_TEMPLATE, not through the preamble. Reword to clarify what the EGG_BRC_EVENT_PUMP flag actually selects (the wrapper, not the preamble), and propagate the same clarification through orchestrator.md and §10.9 of agent-wait-patterns.md. 2. **Heading "(behind EGG_BRC_MEMORY=full)" was misleading.** The composer itself runs whenever the event-pump branch runs (EGG_BRC_EVENT_PUMP), and only the memory-excerpt *content* is gated by EGG_BRC_MEMORY. The matrix table got this right but the section heading suggested the whole composer was behind the memory flag. Drop the "behind ..." qualifier from the H2/H3 headings and add a "Flag mapping (read this first)" callout at the top of both the orchestrator.md and agent-wait-patterns.md sections. Update all four cross-link anchors (orchestrator.md, agent-wait-patterns.md, brc-memory.md) to the new slug. 3. **Line-number references in a 24.5k-line file are doc-drift prone.** Add an explicit caveat noting the numbers come from the slice-3 contract spec and reflect post-collapse positions, and lean on function- and banner-name references (_build_brc_preamble, "the dual-mandate banner") for reading the live file. Apply the same softening in agent-wait-patterns.md §10.9.5. The reviewer also flagged that the sandbox image rebuild is not exercised by this slice — that's a slice-4 coordination obligation sitting outside the BRC review surface (documenter cannot exercise make build / make k3s-import / make deploy from the pod); the docs record the rebuild-trigger as required by task-3-4's acceptance criterion so slice-4 can verify the new image deployed before flipping EGG_BRC_EVENT_PUMP. No content change needed for that one. Refs #2908 tasks 3-4, 3-5 (re-propose v2 after reviewer_code ACK)
Three substantive corrections from reviewer_code's v2 ACK
observations. None are blocking; each addresses a factual / drift
risk the reviewer surfaced inline.
1. **mission.md doc cross-references now use $EGG_REPO_PATH/docs/...
convention.** The three new links at mission.md:153 (BRC memory
artifact) and :165 (BRC Event-Pump Wrapper + agent-wait-patterns
§10) previously used 2-dot relative paths (../../docs/...). The
sibling rule files in the same directory (checkpoint.md,
contract.md, orchestrator.md) use the $EGG_REPO_PATH/docs/...
convention, and the same mission.md file's other doc references
already follow that pattern. The 2-dot paths resolve to
sandbox/docs/... from the source tree and /docs/... from the
runtime mount at /opt/claude-rules/, neither of which exists.
Switch to the dominant convention so the links are followable
from any context.
2. **"exits 0 once role_complete flips" replaced with the actual
flag name.** The role_complete flag does not exist anywhere in
the code. The brc next-action route returns {"action": "complete"}
(orchestrator/routes/consensus.py _VALID_ACTIONS + role-complete
short-circuit) and the wrapper checks is_complete from the
consensus status payload. Reword the third event-handler step to
"exits 0 once `brc next-action` returns the `complete` action
(i.e. the role is marked complete in `consensus status`)" so the
doc no longer references a non-existent flag.
3. **peer_consensus.py line-number caveat propagated.** The slice-3
prompt-composer tables in both orchestrator.md (§"Per-event
prompt composer") and agent-wait-patterns.md §10.9.1 cited
peer_consensus.py:949-1024 for _open_nacks_barrier_response. The
actual function spans 949–1046 in this branch — the same
doc-drift risk the v2 commit already softened for the 24.5k-line
pipelines.py. Extend the same caveat to the peer_consensus.py
citation so future drift on either file is contained by the
function-name reference rather than caught only by the line
number.
The reviewer also surfaced a coordination note: task-3-4's
acceptance criterion is "rebuild produces a new image tag", which
the documenter cannot exercise from the BRC pod (no sudo / k3s
access). That half is structurally deferred to slice-4's
pre-flag-flip kubectl-exec assertion (task-4-1 per the contract).
The docs already record the rebuild trigger (make build /
make k3s-import / make deploy) as required by the acceptance
criterion. No content change needed — the slice-3 PR body should
surface the deferral explicitly so slice-4 doesn't have to
rediscover it.
Refs #2908 tasks 3-4, 3-5 (re-propose v3 after reviewer_code v2
ACK with non-blocking observations)
…omposer
Adds ``orchestrator/routes/event_prompt.py::compose_event_prompt`` —
the slice-3 replacement for the slice-2 wrapper's minimal stub prompt
at ``orchestrator/consensus_wrapper.py::invoke_agent_for_event``.
Per the slice-3 plan TASK-3-1:
- Positional signature ``(role, event_payload, memory_excerpt, nacks,
git_log_delta, base_branch) -> str`` matches the contract verbatim
so the wrapper bash's ``python3 -c`` call site is stable across
refactors.
- Memory excerpt rendered at the user-prompt TAIL position (architect
od-6 Option B); the illustrative ``--append-context`` flag from the
analysis pseudocode does not exist on ``build_agent_command``
(verified at ``shared/egg_agent/command.py:11-46``).
- Per-producer ``git log {sha}..HEAD --not origin/{base_branch} -p``
delta rendered verbatim alongside the executed command (NOT a
``changed_artifacts``-only shortcut — per
``docs/architecture/REVIEWER-SYNC.md`` the re-review must audit
the full delta as a fresh review).
- Open-NACK payload rendered per-reviewer with reason and
artifact_refs (#2142 aggregated-NACK barrier shape from
``peer_consensus.py:_open_nacks_barrier_response``).
- Envelope bound at ``PROMPT_ENVELOPE_MAX_BYTES = 10 KB`` (excluding
the delta, which scales with the change); memory excerpt capped at
``MEMORY_EXCERPT_MAX_CHARS = 2 KB``.
Placed in a sibling module rather than at the bottom of
``orchestrator/routes/pipelines.py`` (the plan explicitly allows the
sibling-module form when the host file is over the decomposition cap,
which ``pipelines.py`` at ~24 800 lines already is). ``pipelines.py``
adds a one-line re-export so callers importing via
``orchestrator.routes.pipelines.compose_event_prompt`` continue to
work — the contract assigns task-3-1 to ``pipelines.py`` so the
re-export keeps the public surface aligned with that.
Wiring (TASK-3-2), the BRC preamble collapse (TASK-3-3), and tests
land in follow-up commits.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t_prompt
Slice-2's ``invoke_agent_for_event`` shipped a minimal stub prompt
("BRC event-pump handler / Role / Slice / Action / Event payload");
this commit wires it to the slice-3 ``compose_event_prompt`` via a CLI
entry-point on ``orchestrator/routes/event_prompt.py``.
The CLI is the wrapper-bash injection seam — calling
``compose_event_prompt`` directly from a Python heredoc would force
the ``orchestrator.routes`` package ``__init__.py`` (which imports
Flask) to load on the agent pod, and Flask is not in the sandbox
runtime. The CLI bypasses that by being the script's
``if __name__ == "__main__":`` entry-point: the wrapper bash invokes
``python3 /opt/egg-runtime/orchestrator/routes/event_prompt.py
<action>`` with the event_payload JSON on stdin, and the script
imports its own neighbouring helpers without traversing the
package init.
CLI responsibilities (matches plan TASK-3-2):
* Read ``EGG_AGENT_ROLE`` / ``EGG_BASE_BRANCH`` / ``EGG_REPO_PATH`` /
``EGG_BRC_MEMORY`` from env (set on every agent pod per
``orchestrator/kubernetes_spawner.py:818-823``).
* Read the per-role memory file at
``.egg-state/agent-outputs/<role>/brc-memory.md`` iff
``EGG_BRC_MEMORY=full`` (slice-1 writer's read gate; slice-4
flips the default to ``full``).
* Parse per-producer ``last_reviewed_commit_sha`` from the memory
file's structured ``### <role>`` blocks (slice-1 writer schema)
even in ``write-only`` mode — the architect plan splits the
memory-excerpt gate (full only) from the SHA-lookup gate (always)
so the wrapper still renders the per-producer delta against the
fallback baseline.
* For each ``last_reviewed_commit_sha``, run
``git log {sha}..HEAD --not origin/{base_branch} -p`` via
subprocess inside the worktree. The gateway allows ``--not`` and
``-p`` on ``git log`` per #2905.
* Extract the open-NACK list (``event_payload['nacks']`` /
``aggregated_nacks``) and forward it through.
* Call ``compose_event_prompt`` and write the rendered prompt to
stdout.
The wrapper bash captures stdout into ``$prompt`` and passes it as
the positional argument to ``python3 -m egg_agent``. On any failure
(script missing, schema drift, git log subprocess crash) the wrapper
falls back to the slice-2 stub so the event-pump keeps running rather
than failing the agent invocation — the idle-budget safety net catches
a wedged event-pump even under a degraded composer.
The composer call obeys the env-flag split:
* ``EGG_BRC_MEMORY=full`` — memory excerpt included; per-producer
delta rendered.
* ``EGG_BRC_MEMORY=write-only`` (slice-1 default) — memory excerpt
omitted from prompt; per-producer delta still rendered against
the memory file's stored SHAs as a fallback baseline. Matches the
plan TASK-3-2 wording verbatim.
* ``EGG_BRC_MEMORY=off`` — both omitted.
Defensive safety rails added:
* ``_GIT_LOG_TIMEOUT_SECS = 60`` so a hung gateway doesn't deadlock
the event-pump.
* ``_GIT_LOG_DELTA_MAX_BYTES = 256 KiB`` per producer with explicit
truncation sentinel so a pathologically large refactor doesn't
blow past Claude's context budget silently.
* Stdin-based event_payload (#2741 prose-arg discipline) instead of
argv.
The TASK-3-3 preamble collapse and slice-3 tests land in follow-up
commits.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…pump model Strips wait-loop / STAY-ALIVE / cursor-threading guidance from the BRC consensus preamble emitted to every concurrent-mode agent prompt. The slice-2 event-pump wrapper owns lifecycle now; the agent's contract is one-shot per actionable event, not "stay alive in a wait-loop until SIGTERM". What changed in _build_brc_preamble: * Producer Lifecycle step 4 (RESPOND TO REVIEWS) — removed the pre-confirm wait-loop invocation (``_brc_preconfirm_wait_line``) and the "Do not include CONSENSUS_CONFIRMED in this pre-confirm wait" foot-gun guidance + the directed STATUS-nudge prose (#2531). Kept the #2142 aggregation rule (still relevant) and the NACK pushback paragraph in condensed form. * Producer step 6 (STAY ALIVE) — deleted entirely. * Producer step 7 (HANDLE RE-REVIEW) — re-numbered to step 6 and rewritten in event-pump terms ("when you are re-invoked with a CONSENSUS_RE_REVIEW event"). Step 8 (RESOLVE OBLIGATIONS) becomes step 7. * Reviewer step 2 (POLL) — replaced with INVOKED PER EVENT framing. The wrapper invokes the reviewer when the producer's CONSENSUS_PROPOSE lands; the reviewer no longer self-drives a wait-loop. * Reviewer step 7 (STAY ALIVE) — deleted. * Reviewer step 8 — re-numbered to step 7, simplified the recovery-trigger language. The adversarial-re-review dual-mandate banner (the "TWO equal-weight mandates" + "Both must pass to ACK" paragraph) is preserved per plan acceptance. * Dual-Role Execution Order banner — kept structurally (per plan acceptance) but updated to describe the event-pump invocation pattern: the coder's PROPOSE re-invokes the tester rather than triggering an in-process wait-loop return. * Pre-seeded empty-producer shortcut (#2581) — references to step 6 STAY ALIVE replaced with "exit; the wrapper re-invokes you with the next event". The detailed STATUS-nudge / wait-loop filter set guidance collapses to "the wrapper will re-invoke you on the next event". * Trailing "If you exit before the orchestrator stops you, you have FAILED your role" warning — replaced with the event-handler contract: "the wrapper drives lifecycle, exit naturally after acting". * Reviewer ACK/NACK section — condensed the #1998 conditional-ACK example (kept the rule + flag, dropped the verbatim command example), folded the #2336 alternative-obligation block into one sentence at the tail of the #2338 drop-obligation block, and trimmed the stale-version paragraph. * Helpers ``_brc_preconfirm_wait_line`` and ``_brc_stay_alive_wait_line`` — deleted (zero callers post-collapse). Byte-size delta (per the plan acceptance ≥ 25% target softened from the original ≥ 40%): * coder 9664 -> 7238 bytes (25.1% drop) * reviewer_code 12606 -> 9346 bytes (25.9% drop) * tester 24139 -> 17635 bytes (26.9% drop) The "Both must pass to ACK" phrase from the dual-mandate banner is preserved (verified across all three role variants); the byte-size drop is across all three. Coverage caveat per the plan: slice-3 ships the collapsed preamble against the LEGACY wrapper path by default (the EGG_BRC_EVENT_PUMP flag stays off until slice-4). The intermediate state (slice-3 merged but slice-4 not yet) is intentionally inert because the legacy wrapper's capped-restart safety net catches the resulting "agent exits after one pass" behavior — slice-4's flag flip plus deletion of the legacy template close that window. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ts, update legacy preamble tests The coder-owns-tests policy (#2936) means the slice-3 coder authors the initial test scaffold for TASK-3-6 (compose_event_prompt) and TASK-3-7 (collapsed preamble snapshot); the tester reviews-and-hardens in their own pass. New tests: * ``orchestrator/tests/test_compose_event_prompt.py`` (TASK-3-6) — prompt-shape tests per role variant (producer / reviewer / dual-role), memory-excerpt truncation at the 2 KB cap, open-NACK rendering across 0 / 1 / 2+ reviewers (the #2142 aggregated barrier case), verbatim ``git log {sha}..HEAD --not origin/{base_branch} -p`` command emission (with a regression guard against future ``changed_artifacts``-only shortcut per REVIEWER-SYNC.md + risk_analyst R6), envelope-budget assertion (≤ 10 KB excluding the rendered delta, which scales with the change), and defensive shape (None inputs, empty role, empty base branch). * ``orchestrator/tests/test_brc_preamble_collapsed.py`` (TASK-3-7) — three role-shape snapshots (coder / reviewer_code / tester); absent-strings (STAY-ALIVE, positive wait-loop instructions, cursor-threading, ready_to_confirm STATUS-nudge); kept-strings ("Both must pass to ACK", dual-mandate banner, agent roster, producer/reviewer lifecycle skeleton); event-handler contract framing replaces the legacy "you have FAILED your role" warning; byte-size drop ≥ 25% per role variant. Legacy preamble tests updated to match the slice-3 collapsed shape in ``orchestrator/tests/test_pipeline_prompts.py`` and ``orchestrator/tests/test_concurrent_integration.py``: * Updated (retargeted assertions, preserved purpose): - ``TestBrcPreambleSyncStep::test_reviewer_sync_step_after_poll`` (POLL → INVOKED PER EVENT) - ``TestBrcPreambleSyncStep::test_reviewer_lifecycle_renumbered`` (steps 1-7, STAY ALIVE deleted) - ``TestAdversarialReReviewPriming::test_reviewer_lifecycle_step8_carries_adversarial_framing`` (banner moved from step 8 to step 7; substantive content preserved) - ``TestAdversarialReReviewPriming::test_producer_respond_to_reviews_legitimizes_new_findings`` (NACK pushback paragraph condensed but preserved) - ``TestDirectedCoordinationGuidance::test_directed_coordination_before_exit_warning`` (precedes the Event-handler contract instead of the deleted "you have FAILED" warning) - Three ``TestDualRoleExecutionOrdering`` tests (banner-presence + REVIEW token; wait-loop allowlist references dropped) - ``TestConcurrentPromptLifecycle::test_concurrent_prompt_includes_lifecycle_preamble`` (asserts the slice-3 Event-handler-contract framing instead of the legacy STAY-ALIVE / "FAILED your role" framing) * Deleted (entirely about deleted functionality): - ``TestReviewerPollUsesWaitLoop`` (POLL is gone) - ``TestReviewerWaitLoopMentionsAutoCursor`` (cursor-threading is gone) - ``TestProducerRespondToReviewsWaitLoop::test_step4_lists_pre_confirm_allowlist`` and ``test_step4_explains_status_ready_to_confirm_nudge`` (the wait-loop allowlist is gone; the orchestrator's ``brc next-action`` route drives the producer pre-confirm waits now). The ``test_step4_excludes_consensus_confirmed`` regression guard is preserved. - Seven ``TestDualRoleExecutionOrdering`` tests that pinned the wait-loop allowlist content of the banner. - ``TestConcurrentPromptLifecycle::test_reviewer_stay_alive_uses_canonical_for_list`` (STAY ALIVE is gone). The deletions preserve issue cross-references in adjacent comments so the audit trail (#1943, #2064, #2323, #2482, #2531, #2749) is not lost. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The slice-3 task-3-3 patch was authored against a base that included the #2936 coder-owns-tests rewrite; rebasing onto origin/slice-3 (which precedes #2936) required two small reconciliations: * Dual-role banner preface — restored ``propose right after`` and ``self-block`` phrasings (required by ``test_dual_role_banner_states_propose_first``) while keeping the event-pump framing from the slice-3 collapse. * ``test_no_positive_wait_loop_instructions`` — broadened the negative-qualifier allowlist to accept ``Do NOT call`` / ``do NOT call`` so the producer-orientation copy (``Do NOT call `wait-loop`...``) and the new banner preface (``Do NOT block on a reviewer wait...``) both satisfy the regression guard. The intent is unchanged: any line mentioning ``wait-loop`` must qualify it with a negative directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three blocking findings addressed; one additional non-blocker drop: 1. **Env-var prefix bug in wrapper bash** (NACK #1). The form ``EGG_AGENT_ROLE=... printf '%s' "$event_payload" | python3 ...`` attached env-vars only to ``printf``, not to ``python3``. The agent pod's parent shell currently exports the needed vars so this works in production, but the comment claimed the re-export protected against a parent shell that hadn't propagated them — which the construct didn't actually do. Moved the env prefix to the RHS of the pipe so ``python3`` actually gets the named vars. Also captured stderr to a temp file and surfaced the first line in the fallback ``cw_log`` so the operator can tell script-not-found / schema-drift / subprocess crash apart (non-blocking observation from reviewer_concurrency). 2. **CLI tests for the memory-mode handling** (NACK #2 — plan TASK-3-2 acceptance "snapshot test verifies both branches"). Added three subprocess-level tests in ``test_compose_event_prompt.py`` driving the ``_cli`` entry point against a real tmp-repo + populated memory file: * ``test_cli_full_mode_emits_memory_and_delta`` * ``test_cli_write_only_mode_omits_memory_keeps_delta`` * ``test_cli_off_mode_omits_memory_and_uses_changed_artifacts_fallback`` Each one verifies the ``EGG_BRC_MEMORY``-mode-gated behaviour of the slice-1 default (``write-only``) and the slice-4 target (``full``). 3. **``changed_artifacts`` fallback implemented in ``_build_delta_entries``** (NACK #3 — plan TASK-3-2 acceptance + documenter's docs at ``docs/architecture/orchestrator.md`` / ``docs/reference/agent-wait-patterns.md`` describe the same degraded-baseline fallback). When no per-producer SHA is recorded yet (off mode, first-ever ACK before any memory write, parse failure, file missing) and the event payload carries a ``changed_artifacts`` list, render a single fallback entry naming the producer and the artifact list — explicitly labelled as a degraded baseline so the agent does NOT mistake it for an adversarial-re-review-grade diff. Adds ``_extract_changed_artifacts`` and ``_extract_producer_role`` helpers and threads ``event_payload`` through the ``_cli`` call. Four new unit tests cover (a) the fallback fires when no SHA + non-empty ``changed_artifacts``, (b) the producer role is pulled from ``event_payload.producer`` / ``event_payload.producer_role``, (c) no fallback when neither SHA nor ``changed_artifacts``, (d) a real SHA always wins over the fallback. 4. **Drop dead ``type`` fallback in ``_render_event_section``** (non-blocking observation). The ``next-action`` route emits ``action`` only (``consensus.py::_VALID_ACTIONS``); the ``type`` fallback was hedging against a schema that doesn't exist in this codebase. All 629 tests in the directly-affected suites pass; ruff lint + format clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two follow-on findings from the aggregated v1 NACK barrier: * **tester NACK** — ``ruff format --check`` failed on ``orchestrator/routes/pipelines.py`` (quote-style on the ``#2338`` drop-obligation paragraph). Ran ``ruff format`` on the file; ``make lint`` now passes literally. * **reviewer_contract NACK #2** — ``test_consensus_wrapper.py`` had no test that pinned the wrapper template's ``invoke_agent_for_event`` invocation shape. The ``TestEventPumpInvokesComposer`` class adds six snapshot tests that fail if a future refactor: * drops the ``invoke_agent_for_event`` function definition; * changes the script path reference or removes the ``EGG_EVENT_PROMPT_SCRIPT`` env-var indirection; * breaks the ``EGG_AGENT_ROLE`` / ``EGG_BASE_BRANCH`` / ``EGG_BRC_MEMORY`` env-var re-export contract to the CLI; * reverts the v1-NACK fix (env-var prefix landing on ``printf`` instead of ``python3``); the ordering test asserts the textual order ``printf '%s' ... | EGG_AGENT_ROLE=... python3`` so a re-introduction of the bug trips the test; * changes the ``python3 "$script_path" "$action"`` call shape; * accidentally references ``event_prompt.py`` from the legacy flag-off template (the composer is event-pump-only). All 635 tests in the directly-affected suites pass; ruff lint + format clean across all slice-3 files. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Empty commit recording documenter participation in slice-3 BRC consensus restart (#2908). The slice-3 documenter work for task-3-4 (mission.md event-handler rewrite) and task-3-5 (orchestrator.md + agent-wait-patterns.md slice-3 doc surface) is already on the slice-3 integration branch from prior cycles (commits 6137694, e3ab9e9, 90b2cb5, plus reconciliation commits dfad765, 63c6bfe, fc2e82e, 66b7e73); this commit anchors a fresh BRC propose by the slice-3 documenter at restart. Verified on the slice-3 branch HEAD prior to this commit: - diff sandbox/agent-config/rules/mission.md sandbox/claude-rules/mission.md returns empty (byte-identical via symlink). - rg 'STAY-ALIVE\b|wait-loop|never exit' inside the rewritten Concurrent Execution Mode section of mission.md returns zero matches. - The "BRC Per-Event Prompt Composer + Preamble Collapse (slice-3, #2908)" section in docs/architecture/orchestrator.md is present and reflects the reviewer_code v1/v2 corrections (heading does not say "behind EGG_BRC_MEMORY"; preamble collapse documented as unconditional; byte-size drop softened to ≥ 25%; line-number references carry the "drift-prone — prefer function-name" caveat). - The "Per-Event Prompt Shape and Memory Consumption (slice-3, #2908)" section in docs/reference/agent-wait-patterns.md is present at §10.9 as a subsection of slice-2's §10 (BRC Event-Pump Wrapper), wiring the $EGG_REPO_PATH/docs/... convention. - EGG_BRC_MEMORY env var entry is in the orchestrator.md Environment Variables table. - Cross-refs to brc-memory.md (slice-1) and the BRC Event-Pump Wrapper section (slice-2) all resolve on this branch (slice-1/slice-2 docs are in the slice-3 branch's history). Refs #2908 tasks 3-4, 3-5
Empty commit recording coder participation in slice-3 BRC consensus restart (#2908). The slice-3 coder work for tasks 3-1 (compose_event_prompt), 3-2 (wire event-pump wrapper to composer), and 3-3 (collapse _build_brc_preamble) is already on the slice-3 integration branch from prior cycles (commits ed8a4c5, 27ed9d0, 2261d5c, plus the test-author commit 7cff8d1, lint-fix 63c6bfe, reconciliation dfad765, and the NACK-address commits fc2e82e, 66b7e73); this commit anchors a fresh BRC propose by the slice-3 coder at restart. Verified on slice-3 branch HEAD prior to this commit: - orchestrator/tests/test_compose_event_prompt.py: 23/23 pass — covers prompt shape per role (producer / reviewer / dual-role), memory-excerpt truncation at the 2 KB cap, NACK rendering for 0/1/2+ reviewers, full 'git log SHA..HEAD --not origin/BASE -p' delta command emitted verbatim with per-producer last_reviewed_commit_sha substituted (no changed_artifacts-only shortcut), envelope bounded ≤ 10 KB. - orchestrator/tests/test_brc_preamble_collapsed.py: 26/26 pass — covers STAY-ALIVE / wait-loop / cursor strings absent (per task-3-3), dual-mandate 'Both must pass to ACK' phrase preserved, agent roster preserved, producer/reviewer lifecycle skeletons preserved, preamble byte size drops ≥ 25% vs the pre-collapse snapshot baseline. - orchestrator/tests/test_pipeline_prompts.py: 431/431 pass — full pipeline-prompts regression suite (event-handler contract present, dual-role banner integration, etc.). - orchestrator/tests/test_consensus_wrapper.py: passes — including the TestEventPumpInvokesComposer suite added in 66b7e73 that pins the wrapper template's invoke_agent_for_event invocation shape. - ruff check + ruff format on the four touched source files passes literally (orchestrator/routes/event_prompt.py, orchestrator/consensus_wrapper.py, orchestrator/routes/pipelines.py, the two new test files).
… v2 NACKs Addresses four blocking findings from the slice-3 v2 NACK barrier (reviewer_code v2 finding #1 + reviewer_code_holistic v2 findings #1/#2/#3). All four are cross-module asymmetries between the slice-3 composer (orchestrator/routes/event_prompt.py) and the next-action route (orchestrator/routes/consensus.py). ### 1. reviewer_code v2 finding #1 — REVIEWER-SYNC.md path The composer's module docstring (event_prompt.py:16) and the "Per-producer re-review delta" prompt section (event_prompt.py:153) both cited 'docs/architecture/REVIEWER-SYNC.md' — a file that does not exist at that path. The real location is 'shared/prompts/REVIEWER-SYNC.md' (verified with git show origin/egg/issue-2908-impl2/slice-3:shared/prompts/REVIEWER-SYNC.md; every other reference in the codebase uses the correct shared/ path). Reviewers following the cited link would 404; this is the exact 'documented snippet that doesn't work as a copy-paster reads it' regression the review criteria flag. Fix: replace both occurrences with 'shared/prompts/REVIEWER-SYNC.md'. Regression guards: test_per_producer_delta_section_cites_correct_reviewer_sync_path and test_module_docstring_cites_correct_reviewer_sync_path now assert the rendered prompt contains 'shared/prompts/REVIEWER-SYNC.md' AND does not contain 'docs/architecture/REVIEWER-SYNC.md' (so a future move/rename can't silently re-break this). ### 2. reviewer_code_holistic v2 finding #2 — unresolved_nacks _extract_nacks (event_prompt.py:602-630) accepted only 'nacks' and 'aggregated_nacks' keys, but next-action's _derive_next_action (consensus.py lines 329/346) emits 'unresolved_nacks' for the single-reviewer NACK propose path — the COMMON case. The open-NACK barrier shape ('nacks' key) requires 2+ distinct reviewers; a single-reviewer NACK uses 'unresolved_nacks' and was silently dropped from the per-event prompt. Fix: add 'unresolved_nacks' as a third accepted key (priority: nacks → unresolved_nacks → aggregated_nacks). Producer re-invoked to address a single-reviewer NACK now sees the structured Open-NACKs section with reviewer 'reason' + 'artifact_refs' inline (the round-trip-per-NACK signal #2142 was built to enforce). Regression guards: - test_extract_nacks_accepts_unresolved_nacks_key_from_next_action pins the new key against the real _derive_next_action payload shape. - test_compose_event_prompt_renders_unresolved_nacks_section asserts end-to-end that the structured Open NACKs section renders with the reviewer's identity / reason / artifact_refs. - test_extract_nacks_priority_order_nacks_over_unresolved_nacks pins barrier-shape priority over the convenience key. ### 3. reviewer_code_holistic v2 finding #3 — scoping to current producer _build_delta_entries iterated EVERY producer in the reviewer's memory file (for producer in sorted(per_producer.keys())) and emitted a delta for each, irrespective of which producer the CURRENT event named. The user-visible failure: reviewer-A ACKed coder at v1 → memory stores coder's SHA. Tester then proposes for the first time. Reviewer-A is re-invoked with event_payload = {pending_reviews: [{producer: tester, ...}]}. The renderer emitted ONLY coder's stale delta — tester's first review had no delta at all, and the section title 'Per-producer re-review delta' implied the rendered delta WAS the producer being reviewed (but it was the wrong one). Fix: scope delta enumeration to producers named in event_payload.pending_reviews (or top-level producer/producer_role on the producer side). Treat memory's per-producer SHA as a per-producer LOOKUP keyed by the current producer, not as an ENUMERATION source. Legacy / synthetic-test paths with no pending_reviews key fall back to enumerating all stored SHAs (backward compat). New helpers: - _extract_current_producers(event_payload) walks pending_reviews / top-level producer keys, de-dupes in first-seen order. - _extract_artifacts_for_producer(event_payload, producer) pulls artifact_refs from the matching pending_reviews entry first (production path), falls back to top-level changed_artifacts only when the top-level producer matches (prevents cross-producer artifact leak). Regression guards: - test_build_delta_entries_scopes_to_pending_reviews_producer pins the scoping invariant (memory has X SHA, event names Y → render Y, not X). - test_build_delta_entries_pending_reviews_with_sha_renders_real_delta asserts _run_git_log invoked only for the current producer's SHA (not the stale one). - test_build_delta_entries_multiple_pending_reviews_renders_each covers the multi-pending-review case. - test_build_delta_entries_no_pending_reviews_falls_back_to_memory_enum pins the backward-compat path. - test_extract_current_producers_* / _extract_artifacts_for_producer_* pin the new helper behaviour. ### 4. reviewer_code_holistic v2 finding #1 — changed_artifacts fallback wired through The documented changed_artifacts fallback (docs/architecture/orchestrator.md + docs/reference/agent-wait-patterns.md) was dead code in production: next-action's _derive_next_action never emits a top-level changed_artifacts key, but the fallback path in _build_delta_entries looked for one. First-time reviewers of any producer (no stored SHA, no top-level changed_artifacts in the real payload) silently saw an empty 'Per-producer re-review delta' section. Fix: enrich next-action's reviewer-side pending_reviews entries with artifact_refs sourced from PeerConsensusTracker.get_current_proposal_snapshot(producer). The lock is reentrant (threading.RLock at peer_consensus.py:101) so the call inside _derive_next_action's locked block is safe. The composer's _extract_artifacts_for_producer prefers pending_reviews[i].artifact_refs over the legacy top-level changed_artifacts key, restoring the documented fallback. Regression guard: - test_next_action_reviewer_pending_reviews_includes_artifact_refs asserts a pending_reviews entry from the production next-action route carries artifact_refs mirroring the producer's current proposal artifacts (the seeded a.py from _propose). ### Tests 660 tests pass under the slice-3 regression scope (compose + preamble + pipeline_prompts + consensus_wrapper + consensus_next_action + concurrent_integration). Ruff lint + format clean on the four touched files.
…ble collapse Adversarial coverage added on top of the coder-authored test scaffolds for TASK-3-6 (compose_event_prompt) and TASK-3-7 (collapsed preamble). Per #2936 the coder authors its tests; the tester reviews-and-hardens in their own pass. Probes the boundaries the coder-authored happy-path tests do not: * ``_truncate`` exact-boundary + just-over-boundary semantics (the off-by-one at the cap was not pinned). * Multi-byte UTF-8 memory excerpt — truncation respects the code-point cap not the byte cap; envelope still respected. * Whitespace-only memory excerpt → section omitted (strip() guard). * Producer-delta iteration order preserves caller order (no implicit sort — sorting belongs to ``_build_delta_entries``'s memory-enum fallback, not the renderer). * ``(no commits in range — re-review is a no-op)`` sentinel for an empty delta string (the post-confirm re-confirm shape). * Producer-delta entries with missing keys render defensive defaults (``(unknown)`` producer label, ``<no prior review>`` SHA sentinel). * Non-string delta is coerced via ``str()`` rather than crashing. * NACK with missing ``reviewer`` / ``reason`` / non-list ``artifact_refs`` renders sentinels rather than crashing. * ``_parse_per_producer_sha`` edge cases — ``-`` sentinel skip, first-match-wins per heading, orphan bullet ignored, backtick-wrapped heading canonicalised. * ``_extract_nacks`` third-priority ``aggregated_nacks`` fallback (the coder pins ``nacks`` > ``unresolved_nacks``; this pins the third tier). * ``_extract_nacks`` drops non-dict entries silently. * ``_extract_changed_artifacts`` filters ``None`` / whitespace; defensive against non-dict payloads. * ``_extract_current_producers`` robust against mixed-shape ``pending_reviews`` entries; defensive against non-dict payloads. * ``_extract_artifacts_for_producer`` priority: ``pending_reviews`` > top-level ``changed_artifacts``; empty / whitespace producer returns empty list. * ``_run_git_log`` subprocess error paths — timeout sentinel, non-zero rc sentinel with stderr, 256 KiB truncation marker. * CLI: ``--event-payload-file`` reads from disk; missing file returns rc=2 with explicit error; malformed-JSON stdin falls back to surfacing the raw payload under ``raw``; empty stdin falls back to ``{"action": <argv>}`` action-only payload. Hardens the preamble-collapse contract beyond the line-by-line ``wait-loop`` qualifier check: * Positive ``egg-orch message wait-loop`` invocations forbidden (each occurrence must sit inside a ±200-char window with a ``do NOT`` / ``Do NOT`` / ``not block on`` / ``not call`` / ``not issue`` negation anchor — catches multi-line code blocks). * ``--for CONSENSUS_*`` filter-flag patterns absent (legacy wait-loop plumbing). * ``never exit`` warning gone (TASK-3-3 explicit collapse target; pairs with the coder's ``"you have FAILED your role"`` check). * ``/tmp/egg-wait-cursor-`` plumbing path absent (full-text scan, complements the substring check). * Agent roster names the producer + reviewer roles, not just the heading. * No ``Ready to confirm — all confirm preconditions satisfied`` STATUS-nudge anchors (#2531 plumbing collapsed). * Tester preamble fits inside a 20 KB ceiling (absolute bound complements the ≥ 25% relative-drop assertion against the hardcoded baseline — a runaway re-expansion fails on both anchors). * Unknown role doesn't crash; the agent-roster section still renders. * Event-handler contract surfaces the wrapper-owned-wait framing (wrapper / drives your lifecycle / one-shot) — pins the central intent of the collapse beyond merely the contract heading. All 115 tests pass under PYTHONPATH=. pytest; ruff lint + format clean on both files. Configured ``make test`` / ``make lint`` / ``make security`` cannot run end-to-end in this pod because the venv-sync step fails to fetch dev wheels (no PyPI egress) — see the propose attestation for the ``tests_execution_blocked`` rationale and the slice-3-scoped check counts that DID execute directly.
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 1, "Test/Integration Tests / Integration Tests": 1} |
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Agent-mode design review
The broader direction here is well-aligned with the agent-mode-design guidelines, with one narrowly-scoped concern worth raising.
What's good
The slice-3 preamble collapse and mission.md rewrite are a clear reduction in procedural micromanagement (guideline #4, "prefer what over how"):
mission.mdreplaces the "Never exit before the orchestrator stops you", STAY-ALIVE loop,wait-loop --for, cursor-threading guidance with a lean event-handler contract: act on one event, update memory, exit naturally. That is exactly the kind of guidance the agent benefits from — lifecycle context the agent can't discover otherwise.- The collapsed
_build_brc_preamblestrips the producer-step-4/step-6 wait-loop plumbing and the cursor-thread/tmp/egg-wait-cursor-*guidance. Keeping the dual-mandate adversarial-re-review banner is the right call — that is behavioural framing, not seam-related procedural overhead. - Moving the heartbeat and gateway-session keep-alive from the agent into the wrapper means the agent no longer has to be coached on liveness mechanics it shouldn't be entangled with in the first place.
Net: agents stop carrying a stateful loop the model can fall out of, and the prompt stops needing prose to compensate for that seam.
One concern: full git-log delta pre-fetched into the prompt
compose_event_prompt / _run_git_log bake the FULL git log {last_reviewed_commit_sha}..HEAD --not origin/{base_branch} -p per producer into the rendered prompt, with a per-producer cap of 256 KiB and an explicit carve-out from the 10 KB envelope budget:
"Envelope budget (≤ 10 KB) excludes the git-log delta. The delta scales with the actual change size; capping it would defeat the whole point of full-delta re-review."
This pattern-matches the explicit anti-pattern in docs/guides/agent-mode-design.md:
"Flag these clear anti-patterns: 1. Excessive pre-fetching: Baking large diffs (10KB+) or full file contents into prompts."
And the auto-review case study calls out "Pre-fetch PR diff and bake into prompt" as the wrong approach, recommending "Agent fetches diff itself (can fetch more context if needed)" instead.
The agent already has everything it needs to fetch the delta itself:
- the per-producer
last_reviewed_commit_shais in the memory file the agent reads anyway, - the exact command (
git log {sha}..HEAD --not origin/{base} -p) is rendered verbatim in the prompt regardless, - the wrapper / sandbox-gateway already allows that
git logshape (per the docstring's reference to #2905), - the fallback path in
_build_delta_entriesliterally instructs the agent to "fetch and read the actual file diffs yourself before issuing a verdict" — which is a tell that this is feasible.
The stated rationale (REVIEWER-SYNC.md / risk_analyst R6: re-review must audit the full delta, not just changed_artifacts) is a strong argument for the scope being the full delta, but not necessarily for the delivery being pre-fetched. The same scope guarantee could be expressed as: hand the agent the SHA + base branch + the command, and have the agent run it as its first tool call.
Tradeoffs I see for keeping it pre-fetched:
- Pro: deterministic scope, one fewer tool round-trip per invocation, scope can't drift if the agent decides to skim.
- Con: prompts can balloon to ~256 KiB on large refactors, the agent has less freedom to scope (e.g. focus on one file at a time, or pull related files outside the diff for context), and you're paying the cacheable-prefix invalidation cost on every event since the delta sits in the early part of the prompt.
Not requesting changes — this is a defensible engineering choice and the team has clearly thought through the rationale. But it's the one place in this slice where the design is fighting the agent-mode-design guidelines explicitly, and it's worth flagging so a follow-up issue can revisit it once the event-pump path is in production and you can measure whether the determinism win is worth the prompt-size and exploration cost.
Other items checked, no concerns
- No structured-output-for-humans pattern: the agent's outputs (propose/ACK/NACK/confirm) flow through tools, not JSON parsing.
- No new direct Anthropic API calls in orchestrator/gateway/shared.
- No
claude --printinvocations. - Model identifiers not touched.
- The JSON event payload rendered into the prompt is input-orientation (what event fired), not a requirement on agent output — that's fine.
- The "do NOT call
wait-loopyourself, the wrapper owns the wait" guidance is lifecycle context the agent can't discover, not procedural micromanagement.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review — slice-3 of #2908 (delta-scoped re-analysis + prompt collapse)
I read the full diff (skipping .egg-state/agent-outputs/issue-2908-impl2-implement-slice-3.{json,md}, ~38k lines of pipeline artifacts per review rules) and traced the data flow from routes/consensus.py → _extract_nacks / _build_delta_entries → compose_event_prompt → the wrapper bash invocation. Below are findings classed by severity.
Strengths (intentional, worth pinning)
- Symlink reality respects the duplication concern.
sandbox/claude-rulesis a git symlink (mode120000) →agent-config/rules, present already atorigin/mainandorigin/egg/issue-2908-impl2/slice-2. The PR only editssandbox/agent-config/rules/mission.md; the symlinked path picks up the same content automatically, sodiff sandbox/agent-config/rules/mission.md sandbox/claude-rules/mission.mdreturns empty trivially. This is correct, even though it diverges from the plan's "BOTH files MUST be rewritten" framing (see Minor #5). - Env-prefix attaches to
python3(RHS), notprintf(LHS).consensus_wrapper.py:1071-1075is correct; the inline comment at:1060-1064calls out the bug class explicitly. The wrapper test classTestEventPumpInvokesComposerpins it. _run_git_loghas a hard 256 KiB per-producer ceiling with an explicit truncation marker. Pathological refactors fail loud (event_prompt.py:_run_git_log+_GIT_LOG_DELTA_MAX_BYTES), and the testtest_run_git_log_truncates_oversized_output_with_explicit_markerregression-traps the cap.- CLI subprocess tests drive real
gitagainst tmp_path repos across theEGG_BRC_MEMORY={full,write-only,off}matrix — no self-seeded fixture / mocked subprocess shortcuts. - Tester adversarial hardening is real.
test_brc_preamble_collapsed.py:213-428adds full-text scans for the literal command-line shape (multi-line bypass guard the original line-by-linedo NOTcheck would miss), UTF-8 CJK truncation at the codepoint not byte cap, agent-roster content check (not just heading-present), and a generous-ceiling complement to the relative-drop ratio. Same posture intest_compose_event_prompt.py:1400-1783for_parse_per_producer_sha,_extract_nackspriority, and_run_git_logsentinels. - Defensive shape against schema drift.
_extract_nacksacceptsnacks>unresolved_nacks>aggregated_nackswith documented priority; non-dict / non-list entries silently drop rather than crash._build_delta_entriesscopes the producer set topending_reviewsper event with a memory-enumeration fallback for legacy payloads.compose_event_promptacceptsNonefornacks/git_log_delta/memory_excerptand tolerates empty role / base_branch. - Composer-failure fallback to slice-2 stub prompt.
invoke_agent_for_eventcaptures stderr toerr_tmp, surfaces its first line on the cw_log line, and falls through to the slice-2 stub rather than killing the event-pump — symmetric with the wrapper's "block, alert, continue" stance.
Minor (worth a follow-up, none block ship)
1. PROMPT_ENVELOPE_MAX_BYTES is documented and tested but NOT enforced in production code.
event_prompt.py:69defines the constant.event_prompt.py:315-317docstring claims "The envelope (everything EXCLUDING the rendered delta) is bounded toPROMPT_ENVELOPE_MAX_BYTESbytes."- Tests assert the bound on representative payloads.
- But
compose_event_prompt(event_prompt.py:319-352) does no envelope check — it concatenates sections and returns. The 2 KB memory truncation and 256 KiB delta cap are the only hard ceilings. A pathological NACK payload (e.g. 6 reviewers each with a 2 KBreason) could push the envelope past 10 KB silently. Worst case is extra token cost / weaker prefix caching, not a crash. - Cheap mitigation: hard-truncate the rendered envelope at the end (excluding
delta_section) with an explicit marker, mirroring_GIT_LOG_DELTA_MAX_BYTES's pattern. Or relax the docstring to say "bounded under representative load" if the soft contract is intentional.
2. EGG_REPO_PATH inheritance is comment-clean but env-prefix-inconsistent.
consensus_wrapper.py:1058-1059comment: "EGG_AGENT_ROLE/EGG_BASE_BRANCH/EGG_REPO_PATH/EGG_BRC_MEMORYare read by the script from env directly".- Actual env prefix on
python3(:1072-1075) explicitly re-exports onlyEGG_AGENT_ROLE/EGG_BASE_BRANCH/EGG_BRC_MEMORY.EGG_REPO_PATHmust already live in the parent shell env to reach the child;_clifalls back toos.getcwd()if unset. - Functionally fine (orchestrator sets
EGG_REPO_PATHin the agent env, andcwdis the worktree root anyway), but the comment implies parity with the explicitly-defaulted vars. Either dropEGG_REPO_PATHfrom the comment list or re-export it for symmetry.
3. Slice-3 default-off mode burns one restart per agent on first invocation.
- With
EGG_BRC_EVENT_PUMP=false(slice-3 default), the agent runs against the legacy_CONSENSUS_WRAPPER_TEMPLATE(capped-restart) with the collapsed preamble (which tells the agent "exit naturally when your handling of the event is complete"). - Legacy wrapper sees clean exit → consensus not yet reached → restart loop fires →
_RECOVERY_SYSTEM_PROMPTis injected with "Stay alive — keep polling withegg-orch message poll --wait 30" (consensus_wrapper.py:135-136). - Net effect: every agent in slice-3 default-off mode consumes 1 of 3
MAX_CONSENSUS_RESTARTS. This is documented as the intentional bridge inmission.md:167("Legacy path note") and docsagent-wait-patterns.md §10.9.5. - Concern: production telemetry will show every agent restart once even on success. That can muddy any "restart rate" SLO between this slice and slice-4. Worth a note in the slice-4 PR body or a release-note for operators.
4. Stale line-number anchors throughout new architecture / wait-patterns docs.
docs/architecture/orchestrator.md: claims_build_brc_preambleis atpipelines.py:12348— actual:12180. Caller sites claimed at:13659, :13692, :13720— actual:13366, 13399, 13427. Dual-mandate banner claimed at12849-12872/ "Both must pass to ACK" at12856-12857— actual:12567inside the block starting at12561.- Same numbers carried into
docs/reference/agent-wait-patterns.md §10.9. - The docs do include disclaimers ("line numbers come from the slice-3 contract spec … may not match the pre-collapse positions … prefer function/banner-name references"), but the actual numbers are 200-400 lines off. The numbers don't aid navigation in their current form — they actively mislead. Either update them post-collapse or drop them and lean fully on the function-name anchors the disclaimers already prefer.
5. PR plan TASK-3-4's "byte-identical duplicates with NO automated sync" framing was already stale before the slice started.
sandbox/claude-rulesis a git symlink toagent-config/rulesatorigin/main,origin/egg/issue-2908-impl2/slice-2, and HEAD. The PR's edit tosandbox/agent-config/rules/mission.mdis the right shape under this reality.- Not a bug in the PR — the right thing happened. Worth flagging for whoever drafts the slice-4 plan body so the "duplicates" framing doesn't propagate, and so the follow-up referenced inside TASK-3-4 ("A separate follow-up issue should consolidate or symlink the two paths — out of scope for this slice") gets updated to reflect that the symlink already exists.
Notes on items I considered and ruled out
_extract_nackspriority order silently droppingunresolved_nackswhennacksis also present. Re-readevent_prompt.py:752-765: this is documented in the function docstring as intentional ("nacks" wins as the canonical 2+-reviewer barrier shape;unresolved_nacksis the single-reviewer fallback). In normal use only one of the keys is populated. If both appeared, taking the barrier shape is the correct choice (it's a superset). Not a bug._parse_per_producer_sharegex anchoring. Handles dash-sentinel ("no prior review"), backticked headings, orphan bullets, and first-match-wins — covered by dedicated tests.- Dockerfile
COPY sandbox/claude-rules/*.md. Theclaude-rulespath is symlinked; Docker COPY follows the symlink at build time and copies the actual file content. No.dockerignoreexclusion would hit this. Not a build-break risk. - Test deletions in
test_concurrent_integration.py/test_pipeline_prompts.py. The removed tests pinned the legacy wait-loop / POLL plumbing the slice-3 collapse intentionally deletes. Each deletion is replaced with a comment block explaining the removal — the equivalent failure mode (e.g. "reviewer misses CONSENSUS_PROPOSE wakes") moves to the event-pump regression suite, not the preamble. Correct collapse, not a coverage regression.
Verdict
Ship-ready. None of the minor findings block; #1 (envelope cap unenforced) and #4 (stale doc anchors) deserve a follow-up commit before slice-4 flips defaults. #3 (restart-budget consumption in default-off mode) is documented behaviour and the right tradeoff for the slice-3 / slice-4 staging.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Enforce PROMPT_ENVELOPE_MAX_BYTES in compose_event_prompt by byte-truncating the NACKs section (the variable-size driver) with an explicit sentinel when the envelope would otherwise overflow (reviewer_holistic minor #1). Memory tail-position contract and delta-exclusion are preserved. - Re-export EGG_REPO_PATH on the python3 invocation in the event-pump wrapper so the env-prefix matches the in-source comment listing all four wrapper-supplied vars (reviewer_holistic minor #2). Test updated to assert all four env-var re-exports. - Update stale pipelines.py line anchors in docs/architecture/ orchestrator.md and docs/reference/agent-wait-patterns.md to the actual post-collapse positions (12180 for _build_brc_preamble, 13366/13399/13427 for callers, 12561-12573 for the dual-mandate banner, 12567 for the 'Both must pass to ACK' anchor); add a reminder that the function/banner-name anchor is the drift-resistant reference (reviewer_holistic minor #4). - Add an operator-telemetry note to agent-wait-patterns.md §10.9.5 calling out the slice-3 default-off restart-budget consumption (one restart per agent per phase) so operator SLOs aren't surprised by the slice-3-vs-slice-4 metric step (reviewer_holistic minor #3).
Review feedback addressed (commit
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — slice-3 of #2908 (delta vs aaaa17f1)
Single new commit on this PR (a159d6a1) addressing the partial-fix concern I raised on the prior review against aaaa17f1. Two files touched: orchestrator/routes/event_prompt.py (~48 lines of production code) and orchestrator/tests/test_compose_event_prompt.py (~130 lines of tests). I re-traced the data path from _producer_has_open_barrier / _producer_has_unresolved_nacks_on_current_version (routes/consensus.py:210-277) → _cli → _extract_nacks → compose_event_prompt → _render_event_section against the new code.
Minor #1 (NACK payload double-rendered in event_section JSON) — addressed correctly
_strip_nacks_for_json at event_prompt.py:113-135 returns a shallow copy of event_payload with each of nacks / unresolved_nacks / aggregated_nacks (matching the priority order in _extract_nacks at :849-854) replaced by a cross-reference marker when the value is a list. _render_event_section calls it before json.dumps (event_prompt.py:165).
Verified end to end:
- The keys covered are exactly the three
_extract_nackswalks — no schema-drift gap between strip and extract. - The
isinstance(value, list)guard preserves non-list values unchanged, matching_extract_nacks's "coerce to empty list on non-list" stance (degrades gracefully on drift rather than panicking). - The marker text mirrors the
_GIT_LOG_DELTA_MAX_BYTEStruncation sentinel shape — agent sees that NACKs are attached and where to read them, rather than silently dropping the field. Plural/singular handling (entry/entries) is correct. - Shallow copy preserves dict key ordering and non-NACK values, so the JSON block still surfaces
status/producer/current_version/nacking_reviewersfor context. - The else-branch keeps non-dict
event_payload(type-signature says it shouldn't happen, but the prior code was also defensive here) untouched — symmetric with the prior behaviour.
The two new regression tests pin the production payload shapes exactly:
test_production_shaped_open_barrier_payload_honours_envelope_capuses the_producer_has_open_barriershape (status="open_nacks_blocked",nacking_reviewers,nackswith the full per-reviewer dict includingtimestamp) with 6 reviewers × ~2.8 KB reasons — my prior worked example. Asserts bothlen(prompt) <= PROMPT_ENVELOPE_MAX_BYTESAND"Pathologically long blocker"is absent from the JSON block (the load-bearing assertion — the marker text being present would not by itself prove the leaked reason text is gone).test_production_shaped_unresolved_nacks_payload_honours_envelope_capuses the_producer_has_unresolved_nacks_on_current_versionshape (unresolved_nackskey, single reviewer) with ~9 KB reason — same assertion.
Ran the full orchestrator/tests/test_compose_event_prompt.py locally: 69 passed.
Items I checked and ruled out
- Cross-reference marker referencing a section that doesn't exist. Only relevant when
event_payloadcarries nacks but thenacksargument is empty. In production,_cliderives thenacksargument fromevent_payloadvia_extract_nacks(:951), so the section will always render whenever the strip fires. Could be misleading in synthetic tests, but the test set never exercises that asymmetry — and it's purely cosmetic. - Other large fields in the JSON (e.g.
pending_reviews,nacking_reviewers). The reviewer-sidepending_reviewspayload is on a different code path that doesn't carrynacks, and per-entry artifact_refs are small.nacking_reviewersis a list of role strings (~10s of bytes each). None of these are variable-size drivers in the production payload shape. - The truncation pass at
:427-436no longer firing in production. Once NACKs are stripped fromevent_section, the only remaining variable-size driver in the envelope ISnacks_section. The truncation pass continues to govern that section — defence in depth, even though the worked example no longer trips the cap after the strip. - The
event_payload is Nonebranch. Lines 158-159 substitute{}forNone; the isinstance check passes, the strip runs against an empty dict, returns an empty dict. No behavioural change. Symmetric with prior behaviour.
Still open (tracked elsewhere, not blocking)
The full git-log delta pre-fetched into the prompt (#2950) is unchanged in this commit. The agent-mode-design re-review confirmed the disposition is correct.
Verdict
Ship-ready. The fix is focused, the test coverage matches the production shape, and the single source of truth for rendered NACK bytes is now nacks_section.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review feedback addressed (no new commit)Per-item disposition for the two Agent-mode-design re-review (
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…s in dual-role banner The conflicts in orchestrator/routes/pipelines.py and orchestrator/tests/test_pipeline_prompts.py both touched the BRC dual-role banner. main updated it for 'coder-owns-tests' (#2749 follow-up #2936) using wait-loop semantics; slice-3 of #2908 is collapsing exactly those wait-loop / STAY-ALIVE semantics in favor of the event-pump wrapper that invokes the agent one-shot per event. Accepted HEAD's event-pump version for both files — main's wait-loop guidance is replaced by the wrapper-driven re-invocation framing that already encodes the coder-tester rendezvous ("the wrapper re-invokes you when that proposal arrives; you propose right after").
Conflict Resolution SummaryResolved merge conflicts with
Why HEAD won on both sidesBoth conflicts sat inside the BRC dual-role banner / reviewer-lifecycle prose that slice-3 task-3-3 is explicitly collapsing (per the task's acceptance criterion:
Including both sides would directly contradict slice-3's acceptance criteria. Discarding HEAD would undo the slice. The HEAD resolution preserves both the structural #2749 constraint ("BRC round cannot close until every producer has proposed") and the coder-owns-tests intent ("propose right after"), reframed in event-pump terms. Verification
Please reviewThe decision to discard main's — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: no new agent-mode design concerns
The only PR-authored delta since a159d6a is the merge commit 39a08bb resolving conflicts with main in orchestrator/routes/pipelines.py and orchestrator/tests/test_pipeline_prompts.py.
The resolution kept HEAD's event-pump dual-role banner / reviewer-lifecycle prose and discarded main's wait-loop / STAY-ALIVE additions (#2936 / coder-owns-tests follow-up). This is consistent with — and required by — slice-3's stated acceptance criterion of collapsing exactly that scaffolding. From an agent-mode-design standpoint, moving from "in-prompt wait-loop with explicit STAY-ALIVE rendezvous prose" to "wrapper re-invokes you on the upstream CONSENSUS_PROPOSE" is the cleaner orientation: the agent gets the event when it needs to act, instead of being told to micromanage a polling loop.
The previously-flagged git-log delta pre-fetching concern remains tracked in #2950 per the original disposition. No new findings.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #2949 — merge resolution (39a08bb)
Reviewed delta: git log a159d6a..HEAD --not origin/main — only commit since last review is the merge of origin/main into the slice-3 branch. All review focus is on the conflict resolution.
🔴 Blocking — Internal contradiction reintroduced by the merge resolution
The merge resolution kept HEAD's dual-role banner verbatim while letting all of main's #2936 (coder-owns-tests) changes auto-merge into the surrounding non-conflicting sections. The result is a preamble that tells the tester two opposite things in the same prompt — exactly the failure mode #2936's commit message called out.
Banner (kept from HEAD), orchestrator/routes/pipelines.py:12348-12383:
"### Dual-Role Execution Order (READ FIRST — #2749)\n\n"
...
"**Execute the lifecycles in this strict order:**\n\n"
"1. **Producer steps 1–3 (ORIENT → WORK → PROPOSE) come FIRST.** "
"While you are doing them, you may *opportunistically* "
"do the Reviewer Lifecycle's `1. PREPARE` work — read the "
"contract, scan the upstream producer's commits as they land "
"on the branch, draft scaffolding. ..."
Producer orientation (auto-merged from main), orchestrator/routes/pipelines.py:13188-13195:
"**The coder now authors its own tests.** Your job is to "
"**review-and-harden the coder's tests**, not to write them "
"from scratch in parallel. **Orient only until the coder "
"proposes** — read the contract, scan the existing test suite, "
"and form your view of where coverage and adversarial cases "
"should land, but do NOT write test files before the coder's "
"CONSENSUS_PROPOSE ..."
Reviewer preparation (auto-merged from main), orchestrator/routes/pipelines.py:12886-12889:
"Do NOT write test files while waiting — the coder authors the "
"tests, so there is nothing to harden until it proposes. ..."
The tester is told simultaneously to (a) execute Producer steps 1–3 first and draft scaffolding while waiting, and (b) NOT write test files before the coder's CONSENSUS_PROPOSE. These are direct opposites.
Why the merge commit message is wrong about this
The merge commit asserts that "the wrapper-driven re-invocation framing… already encodes the coder-tester rendezvous." That covers the timing primitive (wait-loop vs. wrapper re-invocation), but #2936 was not only a wait-loop rewording — it was a semantic change to the tester's role:
- Pre-#2936: tester was a parallel producer that scaffolded tests from
tasks[].fileswhile the coder worked. - Post-#2936: tester orients only, waits for the coder's PROPOSE, then reviews-and-hardens the coder's tests.
f8d320a (#2936) explicitly deleted "draft scaffolding" from the banner AND from _build_producer_orientation AND from _build_reviewer_preparation AND from the role descriptions — because that text was load-bearing for the old scaffold-first model. Keeping HEAD's banner re-imports the old scaffold-first directive into a prompt whose surrounding text has already been updated to forbid it.
What the tester now sees
Read the preamble top-to-bottom as an agent would:
- Banner: "Producer steps 1–3 (ORIENT → WORK → PROPOSE) come FIRST." → tester reads "WORK comes FIRST."
- Banner: "draft scaffolding" while doing the producer steps → tester reads "scaffold tests now."
- Producer orientation: "do NOT write test files before the coder's CONSENSUS_PROPOSE." → tester reads "wait."
This is exactly the two-opposite-directives failure mode the body of f8d320a explicitly called out as the bug it was fixing. The merge resolution has reintroduced it from the other side.
Fix
Replace the banner's step-1 wording with the post-#2936 semantic. Concretely:
- Remove "draft scaffolding" from the opportunistic-prepare list (it's the scaffold-first directive in disguise).
- Reword "Producer steps 1–3 (ORIENT → WORK → PROPOSE) come FIRST" — for the tester, WORK is now legitimately gated on the coder's PROPOSE, so "first" misleads. The producer-orientation phrasing ("Orient only until the coder proposes") is the load-bearing constraint and should win.
- Keep slice-3's "the wrapper re-invokes you when that proposal arrives; you propose right after" — that is correctly orthogonal to the semantic change and does belong in the banner.
This is a banner-only edit; the surrounding sections already have the right text.
🔴 Blocking — Test gap that allowed the contradiction through
orchestrator/tests/test_pipeline_prompts.py:3851 test_tester_orientation_directs_review_and_harden_after_propose:
orient = _build_producer_orientation("tester", "implement", [])
lower = orient.lower()
...
assert "scaffold" not in lowerThis is the test that should catch "draft scaffolding" creeping back into the tester's prompt. It only inspects _build_producer_orientation's output — not the full preamble, which is where the banner lives. So the banner can carry "draft scaffolding" forever without tripping this guard.
test_dual_role_banner_states_propose_first at :5250 was relaxed in this slice to focus on the wrapper/event-pump framing and no longer asserts the coder-owns-tests semantic, so it also does not catch the contradiction.
Fix
Either:
- (preferred) Tighten
test_tester_orientation_directs_review_and_harden_after_proposeto run against the full_build_brc_preamble("tester", "implement")(the assertions are already worded in a way that works on the full preamble —"harden","consensus_propose","do not write test files before","scaffold" not in lower), OR - Add a new test that pulls the banner substring (
preamble.index("### Dual-Role Execution Order")→preamble.index("### Producer Lifecycle")— the slice already uses this idiom) and asserts the coder-owns-tests semantic on it:"scaffold" not in banner.lower(),"do not write test files" in banner.lower()(or equivalent wording),"review-and-harden" in banner.lower()or"after the coder proposes" in banner.lower().
The second option is more robust to future banner-only regressions and is what I recommend.
🟡 Minor — Merge commit message is misleading
"main's wait-loop guidance is replaced by the wrapper-driven re-invocation framing that already encodes the coder-tester rendezvous"
This describes only the timing axis of #2936. The semantic axis — "the coder authors its own tests; the tester does not scaffold" — is independent of how the tester waits and was not subsumed by the event-pump framing. The commit message reads as if #2936's changes were redundant with slice-3, which is the framing that caused the conflict to be resolved by HEAD-wins rather than by re-deriving the banner from both halves.
I would amend or annotate the merge commit message (or capture this in the PR description) so a future archaeologist looking at why the banner contradicts the rest of the preamble has the right pointer.
What still looks fine
- All non-conflicted parts of main auto-merged cleanly into slice-3.
- The remaining slice-3 deltas from my prior review (event-pump compose helper,
_build_brc_preamblecollapse, mission.md updates) are unchanged at the bytes level sincea159d6a; my previous ACK stands for those. test_brc_preamble_collapsed.py'sPRE_COLLAPSE_BASELINE_BYTESand STAY-ALIVE absence tests still pass against the merged preamble — they verify the slice-3 collapse, which is orthogonal to the issue above.
Verdict
NACK on the merge resolution as it stands. Both blocking items are in the prompt-construction code that this PR explicitly owns; they are not pre-existing in main, and they are not pre-existing in the slice-3 branch — the contradiction was created by this merge. Fix the banner text and tighten/extend the banner test, then re-propose.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The merge resolution in 39a08bb kept HEAD's banner verbatim while main's #2936 (coder-owns-tests) auto-merged into surrounding sections. The banner directed the tester to "execute Producer steps 1-3 FIRST" and "draft scaffolding" while the surrounding producer orientation tells the tester "Orient only until the coder proposes" and "do NOT write test files before the coder's CONSENSUS_PROPOSE". Two opposite directives in the same prompt. Fix (banner-only edit; surrounding sections already correct): - Remove "draft scaffolding" from the opportunistic-prepare list (it's the scaffold-first directive in disguise). - Reword "Producer steps 1-3 come FIRST" to "Producer ORIENT (step 1) comes FIRST" + an explicit pivot on the role-specific orientation for whether WORK (step 2) runs immediately or is gated on the upstream CONSENSUS_PROPOSE. - Name the tester case explicitly (the dual-role agent whose WORK is gated per #2936) so the agent reading the banner can reconcile it with its role-specific orientation. - Keep "the wrapper re-invokes you when that proposal arrives" and "propose right after" — slice-3's event-pump framing remains correct. Add ``test_dual_role_banner_does_not_contradict_coder_owns_tests`` that scopes assertions to the banner SUBSTRING (between ``### Dual-Role Execution Order`` and ``### Producer Lifecycle``) so a future banner-only edit can't sneak the scaffold-first prose back in. Pairs with the existing ``test_tester_orientation_directs_review_and_harden_after_propose`` which covers ``_build_producer_orientation`` — together they pin both halves of the prompt the merge resolution desynced.
Review feedback addressed (commit
|
This comment has been minimized.
This comment has been minimized.
|
egg is investigating the Test check failure...
|
Check Failure: Integration Tests / Integration TestsWhat's failing: The Root cause: Transient network/infrastructure issue. The DockerHub registry timed out while resolving What needs to be done:
Suggestion: A human with workflow re-run permissions can re-trigger via — Authored by egg |
|
egg check fixer completed for Test. CI will re-run to verify. View run logs — Authored by egg |
Update documentation to reflect changes from d117874 (#2949): - Add architecture index entry for BRC Per-Event Prompt Composer + Preamble Collapse (slice-3), pointing to the new section in orchestrator.md added by the slice-3 commit - Update Agent Wait Patterns index entry to mention §10.9 (per-event prompt composer + preamble collapse) added by the slice-3 commit Triggered by: d117874 ([issue-2908][slice-3/6] Delta-scoped re-analysis + prompt collapse (#2949)) Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
1 participant
Context
BRC consensus today depends on the agent re-entering a blocking
egg-orch message wait-loopbetween every event. That re-entry is aseam the model can fall out of by emitting a final assistant
message instead of re-entering the wait. Claude usually re-enters;
qwen3.7-max does not (#2906) — it exits success=True after one
match, the wrapper sees no CONSENSUS_CONFIRMED, the 3-restart cap
trips (#2806), and the pipeline FAILs after ~$1 and ~20 min of
churn. Prompt-only mitigations narrow the seam for one model; the
seam itself exists for every model (lineage: #2323, #2064, #2482,
#2036, #1995, #2451).
Changes
Reframe consensus-agent execution from a long-lived participant
that holds blocking waits into a deterministic wrapper-driven
event pump that invokes the agent one-shot per actionable event,
with continuity carried by a durable per-role memory file. Lands
in six linear slices behind
EGG_BRC_EVENT_PUMP(default falseuntil slice-4):
egg-orch brc next-action,brc get-state,brc list-blocking,phase get-contextCLIsubcommands. Durable BRC memory artifact at
.egg-state/agent-outputs/<role>/brc-memory.mdwithaction-scaffolded writes into
brc_ack/brc_nackhandlers,atomic writes via promoted
_persist_atomic_template,last_reviewed_commit_shaper producer in the schema,fail-closed path construction. Gated by
EGG_BRC_MEMORY=write-only(writes accumulate; reads landin slice-3). Purely additive — zero behavior change.
orchestrator/consensus_wrapper.pyas a deterministic eventpump gated by
EGG_BRC_EVENT_PUMP. Drop the 3-restart cap;replace with idle/no-progress safety budget. Migrate the
heartbeat (Overseer flags BRC reviewers/testers as stalled when they're correctly blocked in mcp__brc__wait_loop (implement-phase false positive) #2036) and gateway-session keep-alive (Orchestrator heartbeat-session lookup fails: container_id missing slice-N segment for non-coder roles #2451) from
the agent-side handler into the wrapper's blocking wait.
Heartbeat payload carries
slice_idfromEGG_SLICE_ID(regression guard). Verification is unit-test-only — no
in-process test double can drive a deployed pod end-to-end
(the pod-injection avenue was ruled out per Expand integration test coverage #2474); true
E2E deferred to slice-4 via the
egg_stackreal-podfixture. Old path retained verbatim alongside.
invocation to hand the agent the memory delta plus the full
git log {last_reviewed_commit_sha}..HEAD --not origin/{base_branch} -pdelta per producer (NOT just orchestrator-side
changed_artifacts— per REVIEWER-SYNC.md the re-review mustaudit the full delta as a fresh review). Strip the
STAY-ALIVE / wait-loop / cursor-threading guidance from
_build_brc_preambleandmission.md; replace with a leanevent-handler contract. Sandbox image rebuilt + agent pod
restarted BEFORE slice-4 flag flip.
EGG_BRC_EVENT_PUMP/EGG_BRC_MEMORYdefaults to on, gated onthe slice-2 / slice-3 unit + BRC in-process regression suites
passing on the new default; delete the capped-restart bash, the
_RECOVERY_SYSTEM_PROMPT, the SSE machinery, and theagent-side wait_loop heartbeat code. Rollback plan: revert
slices 1–3 via
git revertif production traffic regresses. Theqwen3.7-max qwen3.7-max BRC agents end their agentic loop early (success=True) without reaching CONSENSUS_CONFIRMED → consensus-wrapper restart churn #2906 k3s spike that previously validated this
end-to-end was removed (qwen route unavailable in k3s;
Claude-route E2E deferred to Rewrite TestCredentialIsolation as k3s-native (currently skipped under k3s) #2585).
subcommands. Add stdin /
--reason-file/--summary-file/
--files-reviewed-fileplumbing toconsensus propose --summary,consensus ack --reason,consensus nack --reason,consensus withdraw --reason(docs: steer agents to the structured contract tool, not Bash-composed egg-contract #2741 regression guard). Add
egg-orch brc resolve-obligationand
egg-orch brc read-peer-artifactCLI subcommands theslice-6 deletion depends on. Argv kept as fallback during
transition (deprecation warning).
MCP tools (~1,515 LOC across 7 namespace files + the 4
infra files +
server.py), theSYSTEM_PROMPT_NUDGE, and theMCP registration block in
shared/egg_agent/client.py:299-353INCLUDING the
EGG_MCP_TOOLSenv flag at :311 (no orphanflag). Retire
tests/tools/test_mcp_cli_drift.py. Migrate theMCP E2E test so the agent's first action is
consensus ack/nackvia stdin/file (preserves SDK-spawn exercise).Verify per-event wall-clock latency within 5%.
Impact
Operator-facing: the BRC consensus subsystem becomes
model-portable — any agent that exits naturally after handling
one event reaches CONFIRMED, instead of needing prompt nudges to
keep re-entering an in-process wait. Cost-per-phase drops because
restart churn disappears, replaced by short bounded per-event
invocations that hit the prefix cache (≥ 60-min TTL on both
routes per WS7 closure). Net code deletion: the capped-restart
template, the SSE machinery, the recovery system prompt, the 28
MCP tool schemas, the
EGG_MCP_TOOLSflag, the cursor-threadingguidance, and the agent-side heartbeat all go away. The agent
primitive (pod / worktree / SDK / permissions / restrictions) is
untouched.
This slice
Delta-scoped re-analysis + prompt collapse
Files affected:
orchestrator/routes/pipelines.pyorchestrator/consensus_wrapper.pysandbox/agent-config/rules/mission.mdsandbox/claude-rules/mission.mddocs/architecture/orchestrator.mddocs/reference/agent-wait-patterns.mdorchestrator/tests/test_compose_event_prompt.pyorchestrator/tests/test_brc_preamble_collapsed.pyTasks:
compose_event_prompt(role, event_payload, memory_excerpt, nacks, git_log_delta, base_branch) -> strhelper toorchestrator/routes/pipelines.py(or a new sibling module if the file is at the size limit — coder's call). Returns the single-event prompt the wrapper invokes the agent with. Shape: role banner + one-line event description + memory excerpt (≤ 2 KB) appended at tail position (architect od-6 Option B — do NOT reference the illustrative--append-contextflag, which does not exist onbuild_agent_command); the FULLgit log {last_reviewed_commit_sha}..HEAD --not origin/{base_branch} -pdelta per producer (NOT just orchestrator-sidechanged_artifacts— perdocs/architecture/REVIEWER-SYNC.mdthe re-review must audit the full delta as a fresh review or the stateless pump systematically weakens adversarial re-review, risk_analyst R6); NACK payload frompeer_consensus.py:949-1024_open_nacks_barrier_responsenacks[](per-reviewer with reason + artifact_refs); the single action expected. The git-log delta is scaled by actual change size and is NOT counted against the ≤ 10 KB envelope — the envelope bounds the surrounding prose only.last_reviewed_commit_shasubstituted in; NACK payload renders per-reviewer with reason + artifact_refs.compose_event_prompt(TASK-3-1) at per-event invocation time. Read the memory excerpt from.egg-state/agent-outputs/<role>/brc-memory.md(slice-1 writer) whenEGG_BRC_MEMORY=full; withEGG_BRC_MEMORY=write-only(slice-1 default), pass empty memory_excerpt — writes happen but reads are no-ops, preserving slice-1's inert default. Memory is delivered inline at the user-prompt tail (architect od-6 Option B); the illustrative--append-contextfrom the analysis pseudocode is NOT a real flag onbuild_agent_command(verified atshared/egg_agent/command.py:11-46). Read the per-producerlast_reviewed_commit_shafrom the memory file's structured section and pass it through tocompose_event_promptso the git-log delta command is parameterised correctly.compose_event_promptinvocation; withEGG_BRC_MEMORY=fulland a populated memory file, the prompt includes both the memory excerpt and the per-producer git-log delta; withEGG_BRC_MEMORY=write-only(slice-1 default), the prompt omits memory but still emits the git-log delta against the orchestrator's signal-levelchanged_artifactsas a fallback baseline; snapshot test verifies both branches._build_brc_preambleatorchestrator/routes/pipelines.py:12348. Delete the STAY-ALIVE / wait-loop mechanics / cursor-threading / pre-confirm-wait foot-gun guidance (Producer Lifecycle step 4 wait-loop plumbing; Producer step 6 STAY-ALIVE loop; cursor /--sinceguidance). KEEP: agent roster, reviewer/producer assignments, dual-role ordering banner; AND the dual-mandate adversarial re-review banner atorchestrator/routes/pipelines.py:12849-12872(the "Your re-review has TWO equal-weight mandates…" block — behavioural framing anchored on by risk_analyst R6, NOT seam-related). The three callers atorchestrator/routes/pipelines.py:13659, :13692, :13720are unchanged — only the preamble text collapses. Slice-3 keeps the flag off by default so the collapsed preamble runs against the legacy wrapper path today; slice-4 makes it the default once the event-pump path is live.orchestrator/tests/test_brc_preamble_collapsed.py; STAY-ALIVE / wait-loop / cursor sections absent; roster + assignments preserved; the phraseBoth must pass to ACK(verified atorchestrator/routes/pipelines.py:12856-12857inside the dual-mandate banner at pipelines.py:12849-12872) appears in the post-collapse preamble snapshot — phrase choice corrects reviewer_plan v2's finding that "Both mandates have equal weight" lives at line 13292 inside_build_adversarial_reprimerather than inside_build_brc_preamble; preamble byte size drops by ≥ 25% (measured against pre-collapse snapshot; the exact number is the snapshot baseline result, not a pre-set target — softened from ≥ 40% per reviewer_plan v2 non-blocker).mission.md(lines 151–154 plus surrounding "Concurrent Execution Mode" section starting at line 137) to the event-handler contract: the agent is invoked one-shot per event by the wrapper; act on the single event, update memory, exit naturally. Remove "never exit before the orchestrator stops you" — under the new model the wrapper owns lifecycle. Keep the Anti-Sycophancy / Structured-Progress-Reporting / HITL-vs-OVERSEER_ALERT / Handling-Agent-Failures sections unchanged. The mission.md rule exists at TWO paths on disk that are maintained as byte-identical duplicates with NO automated sync (reviewer_plan blocker; independently verified viasandbox/Dockerfile:212-214COPY sandbox/claude-rules/*.mdandsandbox/entrypoint.py:967_CLAUDE_RULES_DIR = Path("/opt/claude-rules")). The Dockerfile-baked path that reaches the running agent pod issandbox/claude-rules/mission.md— that one is the canonical runtime source.sandbox/agent-config/rules/mission.mdis the documentation-style duplicate. Both files MUST be rewritten so that after the slice-3 commitdiff sandbox/agent-config/rules/mission.md sandbox/claude-rules/mission.mdstill returns empty AND the new content is present in both. Treatsandbox/claude-rules/mission.mdas the runtime-load truth source; the other path is kept in lock step for now. (A separate follow-up issue should consolidate or symlink the two paths — out of scope for this slice.) The mission.md rewrite reaches the agent pod only after the sandbox image is rebuilt and pods are restarted; this MUST land BEFORE slice-4's flag flip — the rebuild-verification is part of this task's acceptance. Role assignment:documenter(deviating from Coder role should be allowed to modify .md files in agent-config/rules/ #1537's coder-spirit for agent-config rule files). Reason: the gateway-enforced patterns atshared/egg_restrictions/patterns.pyexemptsandbox/agent-config/rules/*.mdfrom the coder docs block (lines 246-247, per Coder role should be allowed to modify .md files in agent-config/rules/ #1537) but do NOT exemptsandbox/claude-rules/*.md— and the Dockerfile-baked runtime copy is theclaude-rulespath (sandbox/Dockerfile:212-214). A coder-role task cannot pushsandbox/claude-rules/mission.md(verified viacheck_file_restriction: coder is blocked, documenter can write). Documenter has write access to both paths via theDEFAULT_DOCS_GLOBS**/*.mdpattern atpatterns.py:177-181. Splitting this into two tasks (coder + documenter) doubles the BRC review surface for one rewrite — keeping it as a single documenter task is the lighter-weight resolution. (A follow-up issue should either consolidate the two paths or add theclaude-rulesallowlist entry to coder.)sandbox/agent-config/rules/mission.mdANDsandbox/claude-rules/mission.mdreflect event-handler semantics; the "stay alive" / "wait-loop" / "never exit" lines are replaced with the event-handler contract; the other four sections unchanged;diff sandbox/agent-config/rules/mission.md sandbox/claude-rules/mission.mdreturns empty after the commit (the two duplicates remain byte-identical);rg 'STAY-ALIVE\b|wait-loop|never exit'against both files returns zero matches. The sandbox-image build step (documented indocs/guides/sandbox-image.mdor equivalent — locate via Grep at implement-time) is exercised and produces a new image tag; the documented rebuild-trigger is recorded in the PR body so slice-4 can verify the new image deployed BEFORE the flag flip.docs/architecture/orchestrator.md(the BRC subsystem section) anddocs/reference/agent-wait-patterns.mdto describe the delta-scoped re-analysis behaviour and the per-event prompt shape, including the fullgit log {last_reviewed_commit_sha}..HEAD --not origin/{base_branch} -pdelivery per producer (and why — REVIEWER-SYNC.md adversarial re-review requirement, risk_analyst R6). Cover the architect's open-decision resolutions: od-1 (subdirectory layout), od-2 (distill memory), od-3 (newbrc next-actionendpoint), od-4 (30-min idle budget), od-6 (memory inline at tail position — Option B). Link to the newdocs/architecture/brc-memory.mdfrom slice-1.compose_event_promptatorchestrator/tests/test_compose_event_prompt.py. Cover: each role's prompt shape; memory excerpt truncation at the 2 KB cap; NACK delta with 0 / 1 / 2+ reviewers; git-log delta command emitted verbatim with the per-producerlast_reviewed_commit_shasubstituted (NOchanged_artifacts-only shortcut); total prompt envelope (excluding git-log delta) ≤ 10 KB per case.make test; one test per role; envelope assertion verified per case; assertion against the git-log-delta command string fails on regression to achanged_artifacts-only shortcut.orchestrator/tests/test_brc_preamble_collapsed.py(new file). Loads the rendered preamble for each of the three caller sites (pipelines.py:13659, :13692, :13720) and asserts (a) the new snapshot matches; (b) STAY-ALIVE / wait-loop / cursor strings absent; (c) agent roster present; (d) byte size drop ≥ 40% vs the prior snapshot baseline.make test; snapshots committed; absent-strings assertions trigger on regression; byte-size assertion stable (with a 5% tolerance band).Test Plan
Per-slice automated coverage:
reviewer / dual-role / open-NACK barrier Producer can re-propose after only one reviewer NACK in multi-reviewer BRC, wasting review cycles #2142 /
conditional ACK / stale-version Producer agent calls 'egg-orch message wait-loop --for CONSENSUS_CONFIRMED'; validator rejects #2482), memory-write
side-effects on
brc_ack/brc_nackcovering all sixrequired fields (incl.
last_reviewed_commit_shaperproducer); atomic-write contract test; fail-closed path
constructor test (raise on unset
EGG_AGENT_ROLE); CLIround-trip with lifecycle-secret auth.
wrapper-side heartbeat unit test asserting
slice_idpropagation from
EGG_SLICE_ID; idle-budget overseer-alertthreshold test; in-process
PeerConsensusTrackerregression(
integration_tests/regression/test_brc_*.py) with flag offestablishes zero orchestrator-side regression; E2E deferred
to slice-4 via
egg_stack(no in-process test double candrive a deployed pod end-to-end per Expand integration test coverage #2474).
compose_event_promptunit tests; collapsedpreamble snapshot; full git-log delta in per-event prompt
asserted; per-event prompt envelope (excluding delta) ≤ 10 KB;
sandbox-image rebuild verification (new mission.md reachable
in pod) BEFORE slice-4 flag flip.
spike was removed (qwen route unavailable in k3s; Claude-route
E2E blocked on ScriptedProvider pod injection, deferred to
Rewrite TestCredentialIsolation as k3s-native (currently skipped under k3s) #2585). The flag flip is gated on the slice-2 / slice-3 unit +
BRC in-process regression suites passing on the new default;
the deletions in TASK-4-2 are covered by the updated unit tests
in TASK-4-3.
--reason-file/--summary-file/--files-reviewed-fileround-trip tests for prose containing$VAR/ backticks /;/&&/ embedded newlines (docs: steer agents to the structured contract tool, not Bash-composed egg-contract #2741regression guard); CLI subcommand tests for
brc resolve-obligationandbrc read-peer-artifact; argv--reasondeprecation-warning test.server registered; migrated E2E runs first action as
consensus ack/nackvia stdin/file (preserves SDK-spawnexercise); per-event wall-clock latency within 5% of
pre-slice-6 baseline.
Manual verification:
so the new
mission.mdis reachable in the pod BEFORE slice-4flag flip.
(brc-memory content for reasoning fidelity;
last_reviewed_commit_shaupdated per producer; cost-per-phasedelta vs restart-churn baseline) and
consents to default-on flip via PR review.
mid-run against pre-slice agents; gate deploy on drain or
cancel.
Manual Steps
Pre-merge:
slice-4's flag flip (the new
mission.mdis the slice-4assumption; if pods are still running the old image when the
flag flips, the event-pump path will reference STAY-ALIVE
semantics that have been deleted from the preamble).
last_reviewed_commit_shacorrectness, cost delta)before the
EGG_BRC_EVENT_PUMPdefault flips to true.in-flight pipelines exist before deploy (MCP tools are deleted
so an old wrapper that still injects them starts with no MCP
server registered).
Post-merge:
"Agent exited without BRC consensus" entries; fall back via
EGG_BRC_EVENT_PUMP=falsedeployment env if seen. Rollbackpath for full spike falsification:
git revertslices 1–3 (noproduction traffic touched the new path because the flag
stayed off until slice-4).
wall-clock regression beyond the 5% budget.
Stack
issue-2908-impl2egg/issue-2908-impl2/slice-2Slice slice-3 of pipeline issue-2908-impl2. Stacked on top of
egg/issue-2908-impl2/slice-2.