[issue-2908][slice-6/6] MCP→CLI deletion: delete agent MCP server +... - #2953
[issue-2908][slice-6/6] MCP→CLI deletion: delete agent MCP server +...#2953james-in-a-box[bot] wants to merge 58 commits into
Conversation
…r branch The slice-4 coder branch was created based on main but the slice-4 parent at origin/egg/issue-2908-impl2/slice-4 has the slice-1, slice-2, and slice-3 implementation work that slice-4 builds on. Merge it in before doing task-4-1 / task-4-2. Conflicts resolved in orchestrator/routes/pipelines.py and orchestrator/tests/test_pipeline_prompts.py — both in the BRC preamble dual-role banner. The coder-owns-tests refinement (#2936) that landed on main and the event-pump banner collapse from slice-3 task-3-3 of #2908 both touch the dual-role banner text. Resolution: keep slice-3's event-pump structure (no wait-loop filter allowlists in the preamble — the wrapper drives invocation) and layer in the coder-owns-tests semantics on top: the tester's first invocation does ORIENT/PREPARE only; the wrapper re-invokes it on the coder's CONSENSUS_PROPOSE; it does its producer WORK (review + harden the coder's tests) + PROPOSE + ACK/NACK in that single invocation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ORY defaults
Slice-4 task-4-1 makes the event-pump wrapper the production default
by flipping two env-flag defaults:
* ``EGG_BRC_EVENT_PUMP`` flips from unset→OFF (legacy) to unset→ON
(event-pump). Setting ``EGG_BRC_EVENT_PUMP=false`` (or
``0`` / ``no`` / ``off``, case-insensitive) keeps the legacy
capped-restart template available for a one-release rollback
window. Unrecognised tokens fall through to event-pump so a typo
cannot silently downgrade the production path. Slice-4 task-4-2
will delete the legacy template entirely and the env flag with it.
* ``EGG_BRC_MEMORY`` flips from unset→``off`` (slice-1 inert) to
unset→``full`` (event-pump composer reads memory by default).
Setting ``EGG_BRC_MEMORY=off`` is the one-release rollback escape
hatch. Unknown values still fail-safe to ``off`` (the fallback
target stays restrictive — an undocumented value is a
misconfiguration signal, NOT a write-bearing default to mask).
Files touched:
* ``orchestrator/consensus_wrapper.py``:
- ``_event_pump_enabled()`` default flipped; falsy-token allowlist
captures rollback path; docstring + module-level reframe updated.
- Wrapper template's inline ``EGG_BRC_MEMORY:-off`` → ``...:-full``
so the wrapper's invocation of ``event_prompt.py`` inherits the
new default even on shells that don't export the var explicitly.
* ``sandbox/egg_agent_tools/handlers/brc_memory.py``:
- ``get_memory_mode()`` defaults to ``MODE_FULL``; new
``MODE_DEFAULT`` constant pins the contract.
* ``orchestrator/routes/event_prompt.py``:
- CLI ``memory_mode`` default flipped from ``"off"`` to ``"full"``.
* Tests updated to match the new defaults:
- ``orchestrator/tests/test_consensus_wrapper.py``:
``TestEventPumpTemplateSelection`` rewritten — unset-env now pins
event-pump, ``EGG_BRC_EVENT_PUMP=false`` pins legacy.
``TestBuildConsensusWrappedCommand``,
``TestConsensusWrapperBehavior``, ``TestBufferOverflowDetection``,
``TestEventDrivenWait``, ``TestSSESigtermGrace`` gain an autouse
``_force_legacy_template`` fixture that engages the rollback
escape hatch so they continue to drive the legacy template.
Slice-4 task-4-2 deletes the entire fixture + these classes
alongside the legacy template.
- ``tests/sandbox/egg_agent_tools/test_handlers_brc.py``:
renamed ``test_unset_defaults_to_off`` → ``test_unset_defaults_to_full``;
the unset-env pin now asserts the memory file is written.
- ``orchestrator/tests/test_compose_event_prompt.py``: docstring
note that ``write-only`` is the rollback target, not the default.
Verified manually with Python smoke tests that the flag-flip works
for unset, truthy, and the full falsy-token allowlist
(``false`` / ``0`` / ``no`` / ``off`` / case variants), and that
``EGG_BRC_MEMORY=writeonly`` (typo) still fails safe to ``off`` with
a warning while ``EGG_BRC_MEMORY=full`` and unset both enable
writes + reads.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…menter branch # Conflicts: # orchestrator/routes/pipelines.py # orchestrator/tests/test_pipeline_prompts.py
Rewrite docs/architecture/orchestrator.md "BRC Consensus Wrapper" section
(renamed from "BRC Event-Pump Wrapper (slice-2, behind EGG_BRC_EVENT_PUMP)")
to describe the post-deletion steady state. Event-pump is now the only
consensus-wrapper path; the legacy capped-restart template and the
agent-side heartbeat / keep-alive path were removed in slice-4 task-4-2.
Changes:
- docs/architecture/orchestrator.md
- Renamed section to "BRC Consensus Wrapper"; updated anchor link
from #brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump.
- Replaced the slice-2 caveat blockquote with a four-slice rollout
summary that names the deleted symbols (_CONSENSUS_WRAPPER_TEMPLATE,
_RECOVERY_SYSTEM_PROMPT, SSE consensus.reached, MAX_CONSENSUS_RESTARTS).
- Reframed "Why a new wrapper template" → "Why the wrapper drives the
loop"; rewrote in past tense so the doc reads as if the event-pump
has always been the only model.
- Rewrote "Wrapper-side heartbeat (#2036 migration)" and
"Wrapper-side gateway-session keep-alive (#2451 migration)" with
"completed in slice-4" qualifier; described agent-side deletion.
- Rewrote "Idle / no-progress safety budget" to drop the comparison
table with the legacy 3-restart cap; replaced with a single
behaviour table for EGG_BRC_IDLE_BUDGET_MIN.
- Added new "Rollback plan" subsection documenting git revert of
slice-4 → slice-3 → slice-2 → slice-1 in reverse-merge order, the
integration check operators must run, and the partial-revert
interaction (reverting only slice-4 restores the dual-emission
state).
- Renamed "Slice-2 verification stance — unit-test-only" to
"Verification stance — unit-test-only"; explained that the
snapshot tests pinning the byte-for-byte legacy template emission
were retired in slice-4 task-4-3.
- Renamed "BRC Per-Event Prompt Composer + Preamble Collapse
(slice-3)" to drop the slice marker; reframed "Flag mapping" to
"What's gated by what" since EGG_BRC_EVENT_PUMP no longer gates
anything.
- Updated EGG_BRC_MEMORY table: full is now the slice-4 default;
write-only is the opt-in regression path.
- Updated env vars table: EGG_BRC_EVENT_PUMP is a deprecated no-op
pointing at the rollback plan; EGG_BRC_IDLE_BUDGET_MIN is no
longer gated on EGG_BRC_EVENT_PUMP=true.
- docs/guides/concurrent-execution.md
- Replaced the slice-2 "two emission paths" caveat with a single
post-deletion summary linking to the new orchestrator.md section.
- Rewrote the "Consensus Wrapper" body to describe the deterministic
event-pump loop (steps 1–6) as the only path; removed
MAX_CONSENSUS_RESTARTS-based restart cap, the recovery system
prompt, and the final-consensus-check restart cycle.
- Updated the configuration table: dropped `max_restarts` row;
added EGG_BRC_IDLE_BUDGET_MIN; updated transient-crash recovery
paragraph to reference the idle/no-progress budget instead of
the deleted MAX_CONSENSUS_RESTARTS hard cap.
- docs/architecture/README.md
- Updated the cross-link card to point at the renamed section and
summarise the slice-4 deletion + rollback plan.
Cross-links to docs/architecture/brc-memory.md (slice-1) retained
throughout. The wait-side companion at agent-wait-patterns §10 is
referenced from each cross-link card.
Satisfies contract task-4-4. Acceptance: doc reads as if the event
pump has always been the only model; legacy-path caveats removed;
cross-links present; rollback plan documented; markdown renders
clean (no conflict markers; section anchors resolve).
…nd agent-side heartbeat Slice-4 task-4-2 collapses ``consensus_wrapper.py`` onto the event-pump template that slice-2 introduced and slice-3 wired the per-event composer into. The event-pump is now the only production path; rollback under a regression is a ``git revert`` of slices 1-3 per the PR body, not an env-flag flip. Deleted from ``orchestrator/consensus_wrapper.py``: * ``_CONSENSUS_WRAPPER_TEMPLATE`` (the ~600-line legacy capped-restart bash template). * ``_RECOVERY_SYSTEM_PROMPT`` and ``_RECOVERY_USER_PROMPT`` — the restart-time recovery prompts. * The SSE ``consensus.reached`` curl path (issue #1897) that lived inside the legacy template — the event-pump uses ``egg-orch message wait-loop`` instead. * ``MAX_CONSENSUS_RESTARTS`` (issue #2806) and its companion constants ``MAX_READY_POLL_CYCLES``, ``TRANSIENT_RESTART_BACKOFF_INITIAL``, ``STARTUP_FAILURE_WINDOW_SECONDS``. The idle/no-progress safety budget (env ``EGG_BRC_IDLE_BUDGET_MIN``, default 30 min) is the replacement liveness ceiling. * ``_event_pump_enabled()`` — the ``EGG_BRC_EVENT_PUMP`` env-flag read. The flag is now silently inert; operators with it lingering in k8s manifests can leave it set to either truthy or falsy and still get the event-pump template. * The legacy-template branch in ``build_consensus_wrapped_command``, which is now a thin alias for ``build_event_pump_wrapped_command``. Preserved by relocating into ``_EVENT_PUMP_WRAPPER_TEMPLATE`` (per task-4-2 acceptance, "Keep ``is_buffer_overflow`` / ``is_transient_crash`` / ``is_startup_failure`` classifiers"): * ``is_buffer_overflow()`` — Claude Agent SDK 1 MiB JSON reader overflow detector (#2804). * ``is_transient_crash()`` — signal-based exits (134, 136, 137, 139, 255). * ``is_startup_failure()`` — exit 1 within a 30 s startup window. * ``STARTUP_FAILURE_WINDOW_SECONDS`` — kept as a bash-scope shell variable inside the template (was a Python module constant). The classifiers are not yet wired into the event-pump's ``propose|ack|nack`` agent-invocation failure path (which uses ``AGENT_FAIL_STREAK`` + idle-budget escalation today); they live as named helpers for future revisions. Deleted from ``sandbox/egg_agent_tools/handlers/message.py``: * ``_WAIT_LOOP_HEARTBEAT_INTERVAL_SECS`` — the 60-s cadence constant. * ``_default_emit_wait_loop_heartbeat`` — the agent-side ``WAITING_FOR_EVENT`` / ``WORKING`` heartbeat emitter (#2036). * ``_start_wait_loop_heartbeat`` — the threaded periodic-tick helper. * The per-iteration ``emit_hb`` / ``stop_hb`` calls inside ``message_wait_loop``, including the ``try/finally`` block that drove the final ``WORKING`` beat on wait exit. The event-pump wrapper now owns both heartbeat liveness (#2036) and slice-scoped gateway-session keep-alive (#2451) via the wrapper- owned ``start_background_heartbeat`` subshell. ``message_heartbeat`` (the explicit handler invoked by ``egg-orch message heartbeat``) is unchanged — the wrapper calls it. Test updates: * ``orchestrator/tests/test_consensus_wrapper.py``: deleted the ``TestBuildConsensusWrappedCommand`` / ``TestConsensusWrapperBehavior`` / ``TestBufferOverflowDetection`` / ``TestEventDrivenWait`` / ``TestSSESigtermGrace`` classes (and the ``_force_legacy_template`` fixture that fed them). The buffer-overflow / SSE / capped-restart surfaces they covered no longer exist. The event-pump classes (``TestEventPumpTemplateSelection`` and siblings) cover the new production path; ``TestEventPumpTemplateSelection`` is reworked to pin the post-task-4-2 invariant that ``EGG_BRC_EVENT_PUMP`` is silently inert (any value, including ``false`` / ``0`` / ``no`` / ``off``, emits the event-pump template). * ``orchestrator/tests/test_consensus_wrapper_anchor.py``: deleted in full — every test pinned ``_RECOVERY_SYSTEM_PROMPT`` / ``_CONSENSUS_WRAPPER_TEMPLATE`` symbols that no longer exist. * ``orchestrator/tests/test_brc_nack_iteration.py``: removed ``TestConsensusWrapperNackFeedback`` (4 tests) that pinned the legacy recovery prompt's NACK-feedback placeholder + helper. The equivalent event-pump assertion lives in ``orchestrator/tests/test_compose_event_prompt.py``. * ``tests/sandbox/egg_agent_tools/test_handlers_message.py``: removed ``TestMessageWaitLoopHeartbeat`` (16 tests) that pinned the agent-side heartbeat path. ``TestMessageHeartbeat`` (the explicit handler tests) is unchanged. * ``integration_tests/regression/test_brc_concurrency.py``: updated the slice-2 verification-stance docstring to reflect the slice-4 post-deletion steady state (E2E deferred to #2585 via ``egg_stack``; in-process tracker coverage unchanged). Defensive grep assertions all return zero matches on ``orchestrator/consensus_wrapper.py``: rg 'consensus\.reached|sse_url|_RECOVERY_SYSTEM_PROMPT|MAX_CONSENSUS_RESTARTS' \ orchestrator/consensus_wrapper.py # → 0 hits Smoke-verified that the event-pump template is emitted regardless of ``EGG_BRC_EVENT_PUMP`` value, that the three classifiers survive in the event-pump template, and that the deleted agent-side heartbeat helpers are no longer importable from ``handlers.message``. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Five blocking findings + three non-blockers from reviewer_code's v1 NACK on PR-proposal v1. Address all five blockers; defer the non-blockers (env-var coordination is gated on coder task-4-1 / task-4-2 landing first). Blocker 1+2+3 — restored post-#2936 coder-owns-tests content in docs/guides/concurrent-execution.md (the v1 proposal overwrote the post-#2936 wording when I copied the slice-4 base, which predates the #2936 merge): - §"HANDOFF" table row example: "Coder can't push test files → HANDOFF to tester" → "Tester can't push a .github/ CI fix → HANDOFF to coder with the required end-state" (matches docs/reference/agent-roles.md). - §"Worked Example: Role-Boundary Handoff" rewrite: drop the reinstated pre-#2936 coder→tester test-handoff example, restore the post-#2936 tester→coder .github/-staging example, and keep the explicit lead sentence "the coder→tester test handoff that used to live here is gone: the coder now authors and pushes its own tests". - §"Rebase rarely conflicts" paragraph: "Rebase cannot conflict because agents have mutually exclusive file write permissions" / "role restrictions guarantee non-overlapping file sets" was a doc lie after #2936 — restore the pre-rewrite "rarely conflicts" wording and the follow-up paragraph that names the shared test scope and the serialize-by-time-not-concurrent invariant. Blocker 4+5 — dead anchors and stale §10 / §10.9 framing in docs/reference/agent-wait-patterns.md and docs/architecture/brc-memory.md: - agent-wait-patterns.md §10 retitled from "BRC Event-Pump Wrapper (slice-2, behind EGG_BRC_EVENT_PUMP)" to "BRC Consensus Wrapper (event-pump model)"; intro blockquote rewritten to drop the slice-2 "OFF by default" framing and instead describe the post-deletion steady state + rollback path; §10.8 retitled from "Flag-off as the temporary default — when slice-4 flips it" to "Rollout completed in slice-4" with body rewritten accordingly; §10.9 retitled to drop the (slice-3) suffix and the "Flag mapping" blockquote rewritten to "What's gated by what" since EGG_BRC_EVENT_PUMP no longer gates anything. - All five dead inbound anchor references repointed to the new anchors: agent-wait-patterns.md lines 1178, 1411, 1653, 1654 and brc-memory.md lines 235, 237. - Reverse direction — repointed orchestrator.md, README.md, and concurrent-execution.md cross-links from #10-brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump to #10-brc-consensus-wrapper-event-pump-model (3 occurrences in orchestrator.md, 1 each in README.md and concurrent-execution.md); same for #109-brc-per-event-prompt-composer--preamble-collapse-slice-3 → #109-brc-per-event-prompt-composer--preamble-collapse. - Verified via grep across docs/ that no remaining link points at the old anchors and no remaining body text carries the "(slice-2, behind EGG_BRC_EVENT_PUMP)" framing. Non-blockers deferred: - Rollback-plan example precision (compose_event_prompt slice-3 vs slice-2 wrapper) — useful tightening but doesn't change the correctness of the doc. - "schema is unchanged" past-tense alignment — minor. - EGG_BRC_EVENT_PUMP "no-op" vs "removed" wording — gated on coder's task-4-1 / task-4-2 final state. Will re-pass once the coder's proposal lands so the doc and code agree.
…r branch Documenter landed task-4-4 (post-deletion consensus-wrapper docs) on the slice-4 branch in parallel with the coder's task-4-1 + task-4-2 work. Their merge resolved the dual-role banner conflict by taking the slice-3 base text (general event-pump description). This commit keeps the coder's resolution (coder-owns-tests refined: tester's first invocation does ORIENT/PREPARE only, wrapper re-invokes on coder's CONSENSUS_PROPOSE carrying proposal in event_payload, tester does producer WORK + PROPOSE + ACK/NACK in that single invocation) since the main-branch coder-owns-tests semantics (#2936) need to be preserved under the event-pump model. The other documenter changes (orchestrator.md, brc-memory.md cross-link, etc.) flow through unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…to egg/issue-2908-impl2-slice-4-coder/work
v2 cleared mandate 1 (all v1 blockers fixed) but mandate 2 found four new blocking findings in agent-wait-patterns.md §10.3 / §10.4 / §10.5 / §10.7 — subsection bodies still described the flag-off vs flag-on dual-emission world in present tense as if both paths still shipped, contradicting the §10 intro blockquote rewritten in v2. Address all four: - §10.3 (Heartbeat ownership) — dropped the two-row flag-off vs flag-on table; rewrote in past-tense post-migration framing mirroring orchestrator.md §"Wrapper-side heartbeat (#2036 migration completed in slice-4)" — the wrapper owns heartbeating now and the pre-#2908 agent-side path in `message_wait_loop` was deleted in slice-4 task-4-2. - §10.4 (Gateway-session keep-alive) — struck the closing "With the flag off the agent-side keep-alive still runs" sentence and replaced it with the slice-4-deletion qualifier matching §10.3 / orchestrator.md. - §10.5 (Idle / no-progress safety budget) — dropped the parenthetical "(replaces the 3-restart FAIL cap)" from the heading; dropped the two-row flag-off vs flag-on table; replaced with a single-row `EGG_BRC_IDLE_BUDGET_MIN` table mirroring orchestrator.md's steady-state version; rewrote the present-tense "MAX_CONSENSUS_RESTARTS = 3 cap" framing in past tense. - §10.7 (Verification stance) — dropped "Slice-2" from the heading; rewrote the body in past tense matching orchestrator.md's §"Verification stance — unit-test-only"; removed the "snapshot equality for the flag-off path" and "deferred to slice-4" framing (slice-4 is this work; flag-off snapshot tests were retired in slice-4 task-4-3); flipped the integration-tests bullet from "runs with EGG_BRC_EVENT_PUMP=false" to "runs against the event-pump wrapper". Adjacent cleanups for body/header coherence: - §10.1 ASCII diagram: relabelled "LEGACY (flag off, today's default)" → "PRE-#2908 (deleted in slice-4 task-4-2 — kept here for git-blame readers)" and "EVENT-PUMP (flag on)" → "STEADY STATE (event-pump, the only path after slice-4)". - §10.9.4 EGG_BRC_MEMORY mode table: marked `full` as the slice-4 default (mirrors orchestrator.md); dropped the slice-3-rollout "operators opt into full just as they opt into EGG_BRC_EVENT_PUMP=true" paragraph since EGG_BRC_EVENT_PUMP is no longer consulted. - §10.9.5 `_build_brc_preamble` collapse: rewrote the closing paragraph in past tense — the collapse runs unconditionally now because the event-pump wrapper is the only path; flipped "Slice-4 flips the wrapper default" → "Slice-4 flipped the wrapper default" so the doc reads as steady state. - §10.9.6 `mission.md` sandbox-rebuild paragraph: flipped "Slice-4's flag-flip is gated" → past-tense "The slice-4 default flip was gated". - §10.9.7 Composer / preamble verification stance: dropped "Slice-3" from the heading; rewrote in past tense matching the §10.7 rewrite; removed "deferred to slice-4" since slice-4 is this work. - §10.9.8 Architect open-decision resolutions: "resolved across slices 1–3" → "resolved across slices 1–4". - §11 Related Documentation cross-link: updated the Concurrent Execution Wrapper card from "how the wrapper uses SSE + wait-loop" (SSE machinery was deleted in slice-4 task-4-2) to "the deterministic event-pump bash loop driver". The two §10.7 non-blockers (slice-2 contract back-reference at §10.7 tail, architect-corrected-pseudocode parenthetical) survive as audit history — the reviewer marked them non-blocking and the context is still useful for future maintainers tracing the slice-2 design review.
…to egg/issue-2908-impl2-slice-4-documenter/work
… not no-op Reviewer_code v2 non-blocker #3 was deferred awaiting coder task-4-1 / task-4-2 final state. The coder's task-4-2 commit (15664e8) has now landed and the docstring at orchestrator/consensus_wrapper.py:35 confirms the env var itself was deleted ("the EGG_BRC_EVENT_PUMP env flag itself"), not just left as a dead branch. Update the docs to match: - docs/architecture/orchestrator.md env-vars table EGG_BRC_EVENT_PUMP row: "Deprecated no-op after slice-4" → "Removed in slice-4 task-4-2"; default "unset (no-op)" → "n/a (removed)"; added the helm-values / pod-spec drop-row note for operators that referenced it explicitly. - docs/architecture/orchestrator.md §"Operator-facing env vars (cross-link)": "the EGG_BRC_EVENT_PUMP selector is no longer consulted — setting it has no effect because the legacy template it selected to is gone" → "was removed in slice-4 task-4-2 — the env var is no longer read by the orchestrator, so setting it has no effect on a post-slice-4 codebase." - docs/architecture/orchestrator.md §"Rollback plan" partial-revert paragraph: tightened the post-slice-4-revert narrative to say the env var itself comes back when slice-4 is reverted (because task-4-2 is what deleted it), and operators wanting event-pump back set EGG_BRC_EVENT_PUMP=true (not =false — the defaults flip back to off). Also tightened the example of why reverse-merge order matters (slice-2 wrapper template references a composer slice-3 added, not "a composer that no longer exists"). - docs/reference/agent-wait-patterns.md §10.8: same shift — env var was deleted alongside the legacy template, so setting it has no effect; rollback path is reverse-merge order.
Fix the six broken tests and four stale docstrings the holistic
reviewer surfaced on v1 (the gateway-blocked test execution missed
them; the structural issues are all visible from grep alone).
Tests (orchestrator/tests/test_consensus_wrapper.py +
orchestrator/tests/test_brc_nack_iteration.py):
* Restored ``import os`` / ``import shlex`` / ``import subprocess`` —
the surviving event-pump test classes still need them
(``TestEventPumpConfirmFailureRaisesIdleAlert`` uses
``shlex.quote`` for stubbed PATH binaries;
``TestEventPumpHeartbeatSubshellLifecycle`` and the
brc_snapshot tests use ``os.environ``).
* Deleted ``TestEventPumpHeartbeatCadence::test_flag_off_heartbeat_path_unchanged``
— its invariant ("legacy template does not emit
``egg-orch message heartbeat``") no longer applies; the legacy
template is gone. Replaced with an inline comment cross-linking
to the post-deletion positive invariant.
* Deleted ``TestEventPumpKeepAliveCadence::test_flag_off_keep_alive_remains_agent_side``
— same reason.
* Deleted ``TestEventPumpIdleBudgetAlert::test_flag_off_idle_budget_not_used``
— same reason.
* Deleted ``TestEventPumpRoleCompleteConfirm::test_flag_off_legacy_path_does_not_auto_call_consensus_confirmed``
— the legacy template is gone; the event-pump's confirm invocation
is strictly orchestrator-driven via the ``case "$ACTION"`` arms,
not auto-invoked on agent exit, so the symmetry guard is
structurally satisfied.
* Renamed ``TestEventPumpFlagIsolation::test_flag_on_does_not_inherit_legacy_max_restarts``
to ``test_event_pump_relies_on_idle_budget_not_legacy_restart_cap``
and dropped the ``max_restarts=7`` kwarg (the legacy kwarg was
deleted from ``build_consensus_wrapped_command`` by task-4-2).
The remaining assertion — ``EGG_BRC_IDLE_BUDGET_MIN`` is in the
script — is the salient invariant.
* Deleted ``TestEventPumpInvokesComposer::test_flag_off_legacy_template_does_not_reference_event_prompt``
— same legacy-path-only invariant.
* Removed the orphaned ``assert "unresolved_nacks" in _CONSENSUS_WRAPPER_TEMPLATE``
line at the bottom of ``test_brc_nack_iteration.py`` (was left
outside any function by the original ``TestConsensusWrapperNackFeedback``
deletion; this is a pure cleanup of slice-4 v1 commit
15664e8).
Docstrings:
* ``sandbox/egg_agent_tools/handlers/brc_memory.py:546`` —
``record_review_event`` docstring updated to reflect the
slice-4 task-4-1 default flip (``EGG_BRC_MEMORY`` defaults to
``full`` now, not ``off``).
* ``orchestrator/routes/event_prompt.py:787`` — CLI docstring
updated to ``default full``; documents that ``off`` is the
one-release rollback escape hatch and ``write-only`` keeps the
writer warm without consuming the excerpt.
* ``orchestrator/consensus_wrapper.py:81`` — module-level template
comment rewritten: the env-flag predicate is gone, the
event-pump template is the only template path post-task-4-2.
* ``orchestrator/consensus_wrapper.py:723`` —
``build_event_pump_wrapped_command`` docstring rewritten to
describe the post-task-4-2 reality (no env-flag gate; legacy
template deleted; ``compose_event_prompt`` already wired).
Defensive (addresses the non-blocking observation #1):
* ``tests/sandbox/egg_agent_tools/test_handlers_message.py:TestMessageHeartbeat``
gains an autouse ``_isolate_slice_id_env`` fixture that clears
``EGG_SLICE_ID``. ``message_heartbeat`` auto-attaches
``slice_id`` from that env via ``_maybe_attach_slice_id``, so a
developer-machine ``EGG_SLICE_ID`` (e.g. inside the egg sandbox)
would otherwise add an unexpected key to the request body and
fail the strict-equality assertions.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…to egg/issue-2908-impl2-slice-4-coder/work
Blocking finding:
* test_consensus_wrapper.py top-level imports missed ``import sys``;
``test_persistent_confirm_failure_fires_overseer_alert`` (the §1 + §6.2
lock-in test, the most operator-critical assertion in the file)
uses ``sys.executable`` at line ~1092 and would raise NameError on
execution, silently disabling the regression guard. The reviewer
caught it via grep — same shape as the reviewer_code_holistic v1
NACK that surfaced the missing os/shlex/subprocess imports.
Fix: add ``import sys`` alongside os/shlex/subprocess.
Non-blocking findings (all addressed in this v3 since they're cheap):
* TestEventPumpIdleBudgetAlert class docstring rewritten —
``The old template keeps MAX_CONSENSUS_RESTARTS verbatim`` was
present-tense framing for the legacy template that task-4-2 deleted.
Now reads ``The legacy template that owned the historical restart
cap was deleted in slice-4 task-4-2; the idle budget is now the only
liveness ceiling in the wrapper.``
* TestEventPumpFlagIsolation class renamed to TestEventPumpIdleBudgetCeiling
with docstring rewritten — after task-4-2 there is no flag-on /
flag-off partition to police, so the original name and ``cross-cutting
guards`` framing no longer apply. The class retains its single
surviving test (``test_event_pump_relies_on_idle_budget_not_legacy_restart_cap``)
which is correct against the post-deletion state.
* test_persistent_confirm_failure_fires_overseer_alert inline comment
rewritten — ``_event_pump_enabled`` was deleted by task-4-2; the
``monkeypatch.setenv("EGG_BRC_EVENT_PUMP", "true")`` is harmlessly
retained as a defensive guard against a future regression that
re-introduces a flag-gated branch. Comment now reads as such.
The reviewer flagged the docstring drift as non-blocking but I'm
folding it into the same commit because the cost is one edit each
and the docstring↔code mismatch the holistic v1 NACK called out is
the same class of issue. Keeping the surface honest.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ocker #2 (test_pipeline_prompts fixture) Follow-up to v3 (e093f67 pushed) that addressed the reviewer_code v2 blocker (missing import sys). This commit addresses the reviewer_code_holistic v2 blocker (2): two pre-existing test failures in orchestrator/tests/test_pipeline_prompts.py. Root cause: the slice-4 base-merge in 06c5a6c resolved the conflict on test_pipeline_prompts.py by keeping slice-3's _PLAN_WITH_MISASSIGNED_TASK fixture (``role: coder`` + ``files: integration_tests/conftest.py``). But main's #2936 ("coder authors its own tests; tester reviews-and- hardens") explicitly excluded coder→test-files from the role↔files alignment validator. The fixture no longer trips the reject path, breaking TestPlannerRoleAlignmentValidation::test_rejects_misassigned_plan_at_propose_time and ::test_rejected_proposal_does_not_mutate_tracker. Fix: cherry-pick main's fixture update — switch the misassignment fixture from a test-file path to a docs path (docs/fixtures.md), which IS still a misassignment, since docs remain the documenter's scope. Added an explanatory comment above the fixture citing #2936 and the slice-3 merge-resolution context so future readers do not re-revert under a conflict resolution that "looks like" the slice-3 text. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…x (tester v3 NACK) Tester v3 NACK had two blockers: 1. ``_auto_populate_contract_at_implement_start`` was deleted from ``orchestrator/routes/pipelines.py`` during the slice-4 base merge (commit 06c5a6c). The orphan import in ``orchestrator/tests/test_auto_populate_contract.py`` broke ``pytest --collect-only`` and blocked ``make test`` from running any tests at all (collection aborts on the first ImportError). Verified by the tester via ``git diff origin/main..origin/egg/issue-2908-impl2/slice-4`` that the function was dropped, not renamed. Fix: restored the function body verbatim from ``origin/main`` (the #2915 production implementation) and re-added the call site inside the slice-loop-mode gate where it lived on main. The function: * lives between ``_check_origin_has_plan_draft`` and ``_populate_contract_from_plan_safe`` (matches main's ordering). * is called from the ``_use_slice_loop`` check in ``_run_pipeline`` when ``_slice_count == 0``, exactly as on main. * uses ``_populate_contract_from_plan``, ``PopulateOutcome``, ``ForestValidationError``, ``_commit_statefiles_to_worktree``, and ``_pipeline_identifier`` — all present in the current file (no further imports needed). The function has a slice-4 v4 banner in its docstring explaining the restore so future merge resolutions don't re-drop it. 2. ``orchestrator/consensus_wrapper.py:50`` had a ruff I001 unsorted imports failure — an extra blank line between ``import shlex`` and the next module-level constant. Fix: removed the extra blank line (one-line deletion). Verified locally: * ``pytest --collect-only`` no longer aborts on ``ImportError: cannot import name '_auto_populate_contract_at_implement_start'``. * ``orchestrator/tests/test_auto_populate_contract.py`` imports clean. * ``orchestrator.routes.pipelines`` module imports clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer_code re-reviewed coder v6 and NACKed with 4 blocking ruff failures + 1 ruff-format failure that would block ``make lint`` in CI: 1. ``orchestrator/tests/test_consensus_wrapper.py:13-23`` — I001 unsorted-import-block (resolved as a side-effect of fixes 2 and 3 reducing the import block to a single from-import). 2. ``orchestrator/tests/test_consensus_wrapper.py:18`` — F401 ``pytest`` imported but unused. The two surviving call sites inside function bodies use ``import pytest as _pytest`` so the top-level name was dead after the v2 test deletions. Fix: remove the top-level ``import pytest``. 3. ``orchestrator/tests/test_consensus_wrapper.py:22`` — F401 ``consensus_wrapper.build_event_pump_wrapped_command`` imported but unused (zero references in the file after the test-deletion sweep). Fix: drop the second name from the from-import. 4. ``tests/sandbox/egg_agent_tools/test_handlers_message.py:10`` — F401 ``threading`` imported but unused. Slice-4 task-4-2 (15664e8) deleted the threaded ``message_wait_loop`` heartbeat machinery; the test cases that exercised it were also removed but the top-level ``import threading`` was left behind. Fix: remove the now-dead import. 5. ``orchestrator/tests/test_pipeline_prompts.py:5129-5131`` — ruff format-check failure on a multi-line assertion message. Pre- existing from the slice-3 tester commit 7cff8d1 but surfaced only now that the file is in lint scope. Fix: ``ruff format`` collapses the two-string concatenation into a single line. Verified locally: * ``ruff check .`` → ``All checks passed!`` * ``ruff format --check .`` → ``872 files already formatted`` * ``pytest orchestrator/tests/test_consensus_wrapper.py`` → 33 passed. * ``pytest tests/sandbox/egg_agent_tools/test_handlers_message.py`` → 24 passed. * ``pytest orchestrator/tests/test_pipeline_prompts.py`` → 431 passed. Non-blocking observations from reviewer_code v6 (the _auto_populate_contract restore in routes/pipelines.py and the v4 consensus_wrapper.py I001 deletion) were already verified-clean in the prior review and remain unchanged in v7. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Document the slice-5 additive CLI surface across the four docs that already carry consensus-protocol prose: - docs/reference/orchestrator-cli.md * New "Prose-bearing args: stdin and --*-file channels (#2741)" subsection under ## BRC Consensus Protocol covering --summary-file, --reason-file, --files-reviewed-file, and the stdin sentinel `-`. * New "## BRC verb-level operations (egg-orch brc)" section documenting the next-action / get-state / list-blocking / resolve-obligation / read-peer-artifact subcommands. * Deprecation-warning note on the argv --summary / --reason path. - docs/reference/agent-tools.md * MCP↔CLI table: mcp__brc__get_state, mcp__brc__list_blocking (slice-1), and mcp__brc__read_peer_artifact, mcp__brc__resolve_obligation (slice-5) flipped from "no CLI" to their new egg-orch brc subcommands. * cli_command=None rationale list: drop the four promoted verbs and add a callout summarizing the slice-5 promotion. * Schema-derivation paragraph: shrink the "tools with no CLI" list accordingly. - docs/reference/agent-wait-patterns.md * Update re-propose / stale-version examples to use --summary-file / --reason-file (the canonical idiom for any wrapper-composed CLI). * New "Prose-bearing args use stdin / --*-file, not argv (#2741)" subsection under §1 with channel table, examples, and rationale. * Related Documentation: cross-link to the new orchestrator-cli.md BRC verb-level operations section and to #2741. - docs/guides/concurrent-execution.md * Refresh the worked Consensus Protocol example to use --summary-file for propose, --reason-file for ack, and the stdin sentinel for nack / withdraw. Add a brc resolve-obligation example. * New "egg-orch brc — verb-level read/derive surface" subsection cross-linking the canonical reference in orchestrator-cli.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rface Slice-5 of the BRC event-pump rollout extends the CLI surface for the in-bash wrapper that ships in slice-2. Three coder tasks land here: task-5-1 — Prose-arg channels for consensus propose/ack/nack/withdraw. The wrapper composes CLI invocations via ``bash -c``, so argv-only prose (``--summary "$RAW"``, ``--reason "$RAW"``) gets corrupted by shell metacharacters (``$VAR`` / backticks / ``$()`` / ``;`` / ``&&`` / embedded newlines) — the #2741 failure mode. This slice generalises the mitigation: every prose-bearing arg now offers a paired ``--FOO-file PATH`` flag and accepts ``-`` as the argv sentinel for stdin. Argv prose still works for humans and during transition but emits ``DeprecationWarning`` so a regression to argv-only inside the wrapper surfaces. ``--files-reviewed-file PATH`` carries an array with one path per line (blank lines and ``#`` comments stripped), per architect v2 §verification_strategy.slice_5. Two helpers in orch_cli.py — ``_resolve_prose_arg`` and ``_resolve_files_reviewed_arg`` — handle channel selection (file → stdin → argv), enforce mutual exclusion, and emit the deprecation. task-5-2 — ``egg-orch brc resolve-obligation`` CLI. Verb-level wrapper around ``mcp__brc__resolve_obligation`` (#2338). Slice-6 deletes the agent-side MCP server, so the wrapper bash needs this verb reachable without an MCP round-trip. Args mirror the handler: ``--reviewer-role`` and ``--producer-role`` are required; ``--commit-sha`` and ``--note`` are optional. The ``--note`` flag uses the same prose-arg plumbing as the other reason / summary args. task-5-3 — ``egg-orch brc read-peer-artifact`` CLI. Verb-level wrapper around ``mcp__brc__read_peer_artifact``. Stdout JSON; pagination via ``--limit`` + opaque ``--cursor`` round-trip; ``--message-type`` is ``action="append"`` for repeated use; ``--no-include-unattributed`` opts out of the slice-scoped + cross-cutting merge (default on, per the handler's per-slice-partition contract from #2548). Tests authored by the coder (tester reviews-and-hardens): * ``tests/sandbox/egg_lib/test_orch_cli_prose_args.py`` — #2741 regression-guard. Parametrises seven representative prose payloads (``$VAR`` / backticks / ``$()`` / shell-control / newline+tab / UTF-8 / quotes+escapes) across each delivery channel (file, stdin sentinel, argv) for ``consensus propose``, ``ack``, ``nack``, and ``withdraw``. Asserts byte-equality between the on-disk / stdin input and the request body received by the orchestrator fake. Argv-path tests assert the ``DeprecationWarning`` fires. Mutual- exclusion paths return exit 2 with helpful stderr. ``--files- reviewed-file`` one-path-per-line semantics covered (blank lines + ``#`` comments stripped). The ``consensus propose --file`` JSON payload path (from issue #1738) is explicitly tested to NOT emit the deprecation warning — only the per-arg argv channels are deprecated. * ``tests/sandbox/egg_lib/test_orch_cli_brc.py`` — extends slice-1's test file with ``TestBrcResolveObligation`` (happy path / commit SHA / note via file / note via stdin / help / parser registration) and ``TestBrcReadPeerArtifact`` (happy path / peer-role filter / message-type list / limit+cursor pagination round-trip / no-include-unattributed default flip / phase choices restricted / help / parser registration). All 306 tests pass on the changed paths; existing consensus-push, slice-1 BRC, and CLI parity tests continue to pass unchanged. Files: sandbox/egg_lib/orch_cli.py; tests/sandbox/egg_lib/test_orch_cli_brc.py; tests/sandbox/egg_lib/test_orch_cli_prose_args.py. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address three blocking findings plus the in-scope non-blocking ones raised by reviewer_code on the v1 documenter proposal. Blocking 1 — `--files-reviewed-file` is one-path-per-line (NOT JSON): The `_resolve_files_reviewed_arg` handler in orch_cli.py:884-922 reads the file as newline-delimited paths, strips blank lines and `#`-prefixed comments, and never calls `json.loads`. Update the example comment in orchestrator-cli.md to say "one path per line; blank lines and `#` comments stripped" and rewrite the heredoc example to demonstrate the comment-stripping behavior. Mirror the clarification in agent-wait-patterns.md. Blocking 2 — schema-derivation claim was wrong: The four BRC tool registrations in sandbox/egg_agent_tools/tools/brc.py (get_state / list_blocking / read_peer_artifact / resolve_obligation) ALL still declare `cli_command=None`. Slice-1 / slice-5 added thin CLI wrappers (`egg-orch brc <verb>`) over the same handlers but deliberately did NOT flip the registrations. The MCP-side schemas continue to be hand-authored in `schemas.py`; the `derive_schema_from_argparse` path is skipped. Restore the four BRC verbs to the `cli_command=None` bullet list with the additional context that a thin CLI wrapper exists; revise the "promoted to CLI" callout to "CLI surface added (registration unchanged)"; revise the schema-derivation paragraph; tag the CLI- counterpart cells with "thin wrapper, registration still cli_command=None — see callout below". Blocking 3 — `brc read-peer-artifact` does NOT use the gateway: The handler reads `.egg-state/brc-history/<identifier>-<phase>.json` files from local disk (verified: no `orchestrator_request(...)` call in `brc_read_peer_artifact`). EGG_ORCHESTRATOR_URL / EGG_LIFECYCLE_SECRET do not apply. Rewrite the "all five subcommands" sentence in orchestrator-cli.md to scope the auth claim to the other four and explain the local-disk semantics so operators don't misdiagnose missing-secret failures. Non-blocking (in-scope to the row I touched): - agent-tools.md: fix the pre-existing handler typo `handlers.brc.read_peer_artifact` → `handlers.brc.brc_read_peer_artifact` (every sibling row uses the brc_ prefix). - agent-tools.md: tighten the read_peer_artifact description to mention `<identifier>-<phase>.json` (not `<pipeline_id>`), the per-slice `<identifier>-implement-<slice_id>.json` partition, the unattributed sibling merge + `include_unattributed=False` toggle, and the `message_type` filter (single value or list). - concurrent-execution.md: author a distinct `reviewer-code-cond-ack.md` for the conditional ACK example so the prose narrative matches the obligation case (instead of re-using the unconditional ACK file). - concurrent-execution.md: show the `cat > /tmp/obligation-resolved.md` heredoc step on the `brc resolve-obligation` example (every other prose-arg example in the same section creates the file inline). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ror, return rc=2 Tester v1 NACK on coder v1 (commit 0a8a7f6): `_resolve_prose_arg` caught `OSError` but not `UnicodeDecodeError`, so a binary or non-UTF-8 file passed to `--reason-file` / `--summary-file` / `--note-file` / `--files-reviewed-file` raised a raw traceback to the wrapper bash instead of the actionable `Error: failed to read ...` message. The tester's one-line fix recommendation was to add `UnicodeDecodeError` to the `except` clauses. That's done. While there, also aligned the helpers to the established orch_cli pattern of `return 2` from `cmd_*` (the same pattern `cmd_consensus_ack` already uses for its `--pre-merge-condition-resolved-in-diff` guard): * Added `_ProseArgError` sentinel exception. `_resolve_prose_arg` and `_resolve_files_reviewed_arg` now `raise _ProseArgError` on any CLI-level validation failure (mutual exclusion, missing required arg, file-read failure incl. `UnicodeDecodeError`); the cmd_* functions catch it and return rc=2. The stderr error message is emitted by the helper before the raise — cmd_* only translates the exception to the exit code. * No more `sys.exit(2)` inside the helpers — `sys.exit` from within a cmd_* call raises `SystemExit`, which fails pytest tests that expect a clean returned rc (the tester's bug-finding test, `test_non_utf8_file_surfaces_clean_error_not_traceback`, makes this explicit by checking `assert rc == 2` after the call). Tester's adversarial test file pulled in (tester committed it as `tests/sandbox/egg_lib/test_orch_cli_prose_args_adversarial.py` at 847985f3d8). Two of the tester's tests that used `pytest.raises(SystemExit)` updated to the new `return rc=2` contract: * `TestProseFileReadErrors::test_missing_reason_file_path_surfaces_clean_error` * `TestProseArgEmptyEdges::test_empty_string_argv_treated_as_missing` Both now assert `rc == 2` returned. The `test_invalid_phase_rejected_at_parse_time` test continues to use `pytest.raises(SystemExit)` because argparse's `choices=` rejection is genuinely a parse-time SystemExit, not a cmd_* validation path. Mirror change in `tests/sandbox/egg_lib/test_orch_cli_prose_args.py`: the coder-authored `test_reason_and_reason_file_mutually_exclusive`, `test_missing_reason_fails_cleanly`, and `test_files_reviewed_and_file_mutually_exclusive` likewise switch from `pytest.raises(SystemExit)` to `assert rc == 2` + `capsys` stderr inspection. Verification: * All 142 tests in `tests/sandbox/egg_lib/` pass. * Broader regression: 193 tests across `egg_agent_tools/test_handlers_brc.py`, `test_cli_parity.py`, `test_orch_cli_consensus_push.py`, and `test_orch_cli_slice_id.py` pass unchanged. * ruff check + ruff format clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds the kubectl-gated integration test
``integration_tests/test_mcp_baseline_capture.py`` that drives a
real-LLM 5-role consensus through the still-live MCP surface
(slice-5 is additive only) and writes per-event wall-clock samples
to ``.egg-state/agent-outputs/latency-mcp-baseline.json``. The
schema (``samples: [{role, event_type, start_ts, end_ts,
duration_seconds, exit_code}]`` plus aggregate p50/p95/max/sum) is
the input slice-6 TASK-6-6 will compare against the CLI-only
baseline it captures after the MCP→CLI collapse lands.
The test skips cleanly when ``_kubectl_available()`` returns False
via the session-scoped ``egg_stack`` fixture, and also skips when
the gateway is unhealthy (dummy GH creds in CI) — mirroring the
guard used by ``test_orchestrator_mcp_contract.py``.
Also commits a synthetic placeholder baseline JSON marked
``_meta.synthetic: true`` so slice-6 has a file to read while the
capture test waits on a real-LLM run; ``_meta.synthetic_reason``
explains exactly how to regenerate.
No ``ScriptedProvider`` import / reference (per the slice-5 plan
re-scope: real LLM route, not in-process provider swap).
Refreshes the agent-facing documentation surface to reflect slice-6's deletion of the in-process Claude Agent SDK MCP tool surface (sandbox/egg_agent_tools/tools/*.py, server.py, SYSTEM_PROMPT_NUDGE, build_sandbox_mcp_server) and the EGG_MCP_TOOLS env flag. The shared pure-Python handler layer at sandbox/egg_agent_tools/handlers/*.py is preserved and continues to back the CLI; the operator-facing orchestrator MCP server (port 9850) is unaffected. - docs/reference/agent-tools.md rewritten as the agent-pipeline-lifecycle surface reference: deletion rationale, what's preserved (handler layer, operator MCP, slice-1/5 brc verb subcommands), CLI surface index, slice-5 prose-arg channels, wait-loop reaffirmation, post-#2908 architecture diagram, updated test inventory. - CLAUDE.md Key Entry Points: clarifies operator submit_task MCP server vs the (now retired) agent-side MCP tools — agents drive lifecycle ops through egg-orch / egg-contract / egg-checkpoint. - docs/index.md: agent-tools entry updated for the new surface. - docs/guides/sdlc-pipeline.md: 'Agent MCP tools (EGG_MCP_TOOLS flag)' section retired (replaced with the post-slice-6 CLI-surface paragraph). - sandbox/agent-config/rules/{environment,README,orchestrator,contract, checkpoint,mission,overseer,push-recovery}.md: EGG_MCP_TOOLS row + Agent MCP tools section removed; 'MCP tool equivalents' sections in the orchestrator / contract / checkpoint rules replaced with prose-arg channel guidance (--<arg>-file PATH / stdin sentinel) and direct CLI-subcommand references; mission.md producer flow now names the CLI surfaces (egg-orch consensus propose --push, egg-contract add-decision, egg-orch overseer alert). - docs/releases/agent-mcp-tools.md: annotated as superseded by #2908 slice-6; kept as historical record of iteration 1. - docs/reference/conditional-ack.md, shared/prompts/REVIEWER-SYNC.md, shared/prompts/code-review-criteria.md: stale mcp__brc__resolve_obligation pointers redirected to egg-orch brc resolve-obligation with retirement context. - gateway/README.md, docs/architecture/git-isolation.md, docs/architecture/orchestrator.md, docs/guides/concurrent-execution.md, docs/guides/pipeline-health-monitoring.md, docs/guides/agent-teams.md, docs/reference/agent-wait-patterns.md, docs/reference/orchestrator-cli.md, sandbox/egg_lib/data/hitl_editing_rules.md: stale mcp__* pointers (mcp__brc__propose / mcp__brc__confirm / mcp__progress__overseer_alert / mcp__sdlc__register_open_question / mcp__sdlc__show_contract) updated to their CLI counterparts with retirement context. Satisfies task-6-5 acceptance criteria: all references to the agent-side MCP tools are updated to the CLI surface, EGG_MCP_TOOLS references are removed (live docs), orchestrator/mcp_server.py references are preserved (operator MCP server, unaffected), CLAUDE.md edited in place via the DEFAULT_DOCS_GLOBS **/*.md pattern — no .egg-state/agent-outputs/ staging detour.
Delete the in-process Claude Agent SDK MCP tool surface
(`sandbox/egg_agent_tools/tools/` — 7 namespace modules + 4 infra
files, plus `server.py` and the orphan `schemas.py`) and the MCP
registration block in `shared/egg_agent/client.py` (including the
`EGG_MCP_TOOLS` env-flag check at line 311 — no orphan flag).
Sandbox agents now drive every consensus / phase / task / progress /
SDLC verb through the `egg-orch` / `egg-contract` shell CLIs in
`sandbox/egg_lib/{orch_cli,contract_cli}.py`; the shared handler
layer at `sandbox/egg_agent_tools/handlers/` is unchanged, so both
surfaces collapse to one (the CLI surface), not zero.
The operator-facing `orchestrator/mcp_server.py` is untouched.
Retired tests
- `tests/tools/test_mcp_cli_drift.py` (the MCP↔CLI drift gate no
longer applies — there is no MCP side to drift from).
- `tests/tools/test_rule_doc_drift.py` (depended on the deleted
`TOOL_REGISTRY`; rule-doc parity is moot post-deletion).
- `tests/sandbox/egg_agent_tools/test_{server,full_tool_registry,
tools,schemas,sdk_surface}.py` (all MCP-surface specific).
Migrated tests
- `integration_tests/test_sandbox_mcp_tools_e2e.py` — preserved the
SDK-spawn exercise: instead of asserting the agent's first
`tool_use` block names an `mcp__*` tool, the migrated test
asserts the agent's first action is a `Bash` invocation of
`egg-orch consensus ack/nack --reason-file PATH` (slice-5 prose
plumbing). Direct-handler invocation covers what the schema /
registration assertions previously did.
- `tests/shared/egg_agent/test_client.py::TestMcpToolsFlag` removed;
the post-deletion guard "no `sdlc` / `brc` / `phase` / `progress` /
`task` / `checkpoint` keys appear in `options.mcp_servers`" lives
in the migrated E2E test instead.
- `sandbox/tests/test_restrictions_handlers.py::TestToolRegistration`
removed (sourced from the deleted `egg_agent_tools.tools.sdlc`).
New test (task-6-6)
- `integration_tests/test_mcp_to_cli_latency.py` — drives a 5-role
consensus on the post-deletion CLI surface (`egg_stack`-gated),
reads the slice-5 baseline at
`.egg-state/agent-outputs/latency-mcp-baseline.json`, writes the
post-deletion measurement to
`.egg-state/agent-outputs/latency-mcp-vs-cli.json`, and asserts
per-event p95 latency hasn't regressed > 5%. On regression
emits a structured `OVERSEER_ALERT` priority `medium` envelope
to stderr (the architect od-5 fallback decision — whether to
ship the persistent `egg-orch` daemon — surfaces from there).
Skips with a clear message when the slice-5 baseline is the
synthetic placeholder; the comparison file is still written for
operator review. Unit-level coverage for the helper
(`_compute_comparison`, `_emit_overseer_alert`) runs every PR
without the cluster.
`EGG_MCP_TOOLS` env-flag references trimmed across the Python tree
so `rg 'EGG_MCP_TOOLS' --include='*.py'` returns zero matches.
Documentation references in `docs/` and `sandbox/agent-config/rules/`
are out of scope for this commit (coder file-boundary blocks `**/*.md`);
documenter task-6-5 cleans them up.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer_code v1 NACK identified 11 blocking findings. All are
verified-correct against the slice-5/slice-6 CLI source
(sandbox/egg_lib/orch_cli.py, contract_cli.py, checkpoint_cli.py).
Every finding is addressed:
1. consensus ack / consensus nack — producer_role is POSITIONAL (not
--producer-role flag) and --files-reviewed / --ack-version /
--nack-version are required. Fixed in
sandbox/agent-config/rules/orchestrator.md (BRC consensus verbs),
sandbox/agent-config/rules/push-recovery.md (conditional-ACK recipe),
docs/reference/agent-tools.md (canonical recipes block).
2. --pre-merge-condition-file does not exist; only inline
--pre-merge-condition is supported today. Fixed in
orchestrator.md, push-recovery.md, agent-tools.md prose-arg table.
3. --detail-file / --recommend-file / --error-file on overseer alert /
progress emit / signal error do NOT exist; slice-5 prose-arg work
only covered consensus propose / ack / nack / withdraw and brc
resolve-obligation. Dropped the -file claims from orchestrator.md.
4. egg-contract has NO prose-file channels (--question / --notes /
--options are inline-only); contract_cli.py was not touched by
slice-5. Dropped --question-file / --notes-file from contract.md,
mission.md, overseer.md, and the cross-doc references.
5. egg-checkpoint search has NO --text-file channel; checkpoint_cli.py
was not touched by slice-5. Dropped from checkpoint.md.
6. --push is OPT-IN (default off); no --no-push flag exists. Fixed
the wrong defaults in orchestrator.md, mission.md,
git-isolation.md, concurrent-execution.md, and agent-tools.md.
7. agent-tools.md prose-arg table overclaimed; now lists only the
four args slice-5 actually shipped (--summary / --reason / --note /
--files-reviewed) with the precise subcommand × arg matrix.
8. Canonical recipe in agent-tools.md fixed to use positional
producer_role + required --files-reviewed / --nack-version.
9. Test inventory in agent-tools.md fixed to name the real on-disk
paths (test_orch_cli_consensus_push.py, test_orch_cli_slice_id.py,
test_mcp_baseline_capture.py); the latency comparison test is
reframed as slice-6 task-6-6 (now on disk after the in-flight
coder/tester merge).
10. gateway/README.md contradiction resolved: dropped the verbatim
quote of the now-outdated error string and the contradictory
footnote; instead describe the actionable target (egg-orch
consensus propose --push) and note that the literal wording in
gateway/gateway.py is the source of truth (operator-only update
surface).
11. Temporal coupling: at v1 propose time the coder's deletions had
not landed. Between v1 and v2 the coder's task-6-1/task-6-2
deletions and the tester's task-6-3/task-6-6 commits merged into
origin/slice-6 (verified post-merge: sandbox/egg_agent_tools/tools/
is gone, EGG_MCP_TOOLS is no longer read in
shared/egg_agent/client.py, integration_tests/test_mcp_to_cli_latency.py
exists). Updated the temporal language throughout: 'X is gone' is
now accurate against the slice-6 branch state; "is being retired
in slice-6" framing kept on doc front-pages so readers landing
here mid-slice understand the merge boundary.
Updated 16 files. No new doc files; in-place edits via the documenter
role's DEFAULT_DOCS_GLOBS **/*.md pattern.
…ormat Adds tester hardening on top of coder v1 (commit bdace09): 1. New `tests/sandbox/egg_agent_tools/test_mcp_surface_retired.py` — 29 hardening tests (18 run without SDK, 11 SDK-gated): - **Repo-walk regression guard** (6 parametrised cases). Asserts every production .py file in `git ls-files` is free of the slice-6-deleted symbols (`EGG_MCP_TOOLS`, `build_sandbox_mcp_server`, `SYSTEM_PROMPT_NUDGE`, `from egg_agent_tools.tools`). Excludes `.egg-state/` archived BRC history and explicit self-referencing test files (this file + the migrated e2e + latency tests + test_client.py + test_restrictions_handlers.py rationale comments). Catches any future commit re-introducing the surface. - **`egg_agent_tools` package shape** (5 cases). Freezes the post-deletion contract: package imports clean, `__all__` is empty, deleted attributes (`SYSTEM_PROMPT_NUDGE`, `TOOL_LIST`, `TOOL_REGISTRY`, etc.) no longer reachable via the namespace, handler layer at `egg_agent_tools.handlers.{brc,sdlc,task}` still present and callable (kept half of the collapse), every deleted submodule (`tools`, `tools.brc`, `server`, `schemas`) raises ImportError. - **EGG_MCP_TOOLS is a no-op** (11 parametrised cases). Sweeps `{true, 1, yes, on, TRUE, True, false, 0, no, off, garbage}` — setting the env flag to any value must not produce egg namespaces (sdlc/brc/phase/progress/task/checkpoint) on `options.mcp_servers`. Catches a partial re-introduction that restores only the `true` branch. Uses the same offline ClaudeAgentOptions capture pattern as the migrated e2e test. - **Latency comparison adversarial corners** (7 cases). Probes the coder's `_compute_comparison` / `_emit_overseer_alert` defensive corners that the happy-path TestCompareComparisonHelper leaves uncovered: negative baseline → ratio=None (no false regression); missing `p95_seconds` key → treated as zero; None value → guard-clause exercised (no float(None) TypeError); +inf measured → regression flagged; NaN measured → ratio=NaN, regression=False (pins IEEE 754 semantics so a future math.isfinite refactor is deliberate); OVERSEER_ALERT envelope carries all required top-level + detail keys; None ratio renders to JSON null without raising. 2. Ruff-format auto-fix for the coder's two migrated test files — `integration_tests/test_mcp_to_cli_latency.py` and `integration_tests/test_sandbox_mcp_tools_e2e.py`. The coder wrote `except (X, Y):` with parens and one long URL on a wrapped line; ruff 0.15.12 prefers the PEP 758 unparenthesized form (`except X, Y:`) and a single-line URL. Net change is style-only and matches the slice-5 baseline-capture test in the same directory. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ron/egg into egg/issue-2908-impl2-slice-6-tester/work
|
egg review completed. View run logs 23 previous review(s) hidden. |
|
egg is resolving merge conflicts for this PR... |
…tion.md and rules/orchestrator.md - docs/guides/concurrent-execution.md: Combined main's removal of the TRANSIENT_RESTART_BACKOFF_INITIAL row and rewrite of STARTUP_FAILURE_WINDOW_SECONDS to reflect the inert classifier helpers, with HEAD's slice-6 conversion of EGG_BRC_IDLE_BUDGET_MIN to reference egg-orch overseer alert (the agent MCP tool mcp__progress__overseer_alert is deleted in this slice). - sandbox/agent-config/rules/orchestrator.md: Kept HEAD's reference pointer; dropped main's "BRC introspection (#2908)" block listing the mcp__brc__* MCP tools, since slice-6 deletes the agent-side MCP surface.
Conflict Resolution SummaryResolved merge conflicts with
Please review:
Verified locally: — Authored by egg |
|
egg conflict resolution completed. View run logs |
There was a problem hiding this comment.
Re-review at c8bb00b2 — no new agent-mode concerns.
Delta since the prior review at 251b6118 is a single merge commit (c8bb00b2) resolving conflicts with main. The PR-authored changes only touch two docs files:
docs/guides/concurrent-execution.md—EGG_BRC_IDLE_BUDGET_MINrow now points ategg-orch overseer alertinstead of the (deleted)mcp__progress__overseer_alert.sandbox/agent-config/rules/orchestrator.md— dropped main's incoming "BRC introspection (#2908)" block listingmcp__brc__*agent-side tools, which would have been stale on merge.
Both resolutions are consistent with slice-6's CLI-only direction: agents now reach the same operations through egg-orch rather than the deleted in-process MCP surface. No pre-fetching, structured-for-humans output, post-processing pipelines, rigid procedures, prompt-level security, direct Anthropic API calls, or hardcoded model IDs are introduced — and nothing about the agent-facing prompt shape changed except the verb names.
Ship it from an agent-mode design perspective.
— Authored by egg
|
egg agent-mode-design completed. View run logs 23 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review of #2953 (delta 251b6118f..c8bb00b27)
Single new commit since the last review: c8bb00b27 — a merge from main with two non-trivial conflict resolutions.
Prior re-review findings — all addressed at 251b6118f
sandbox/egg_lib/orch_cli.pydocstring:mcp__progress__emitreference removed. ✅- Four future-tense doc lines (
sandbox/agent-config/rules/environment.mdL17-20,sandbox/agent-config/rules/README.mdL70-74,docs/index.md:85,docs/guides/sdlc-pipeline.md:1540): all now past-tense ("was retired"). ✅ tests/shared/egg_agent/test_client.py:1335-1356comment now correctly notes the agent does not invoke the operator-facing orchestrator MCP server. ✅
Merge-conflict resolution verification — both correct
docs/guides/concurrent-execution.md: combinedmain's restart-handling rewrite (single-loopAGENT_FAIL_STREAK++/ 1 s backoff, inertSTARTUP_FAILURE_WINDOW_SECONDSplaceholder, removedTRANSIENT_RESTART_BACKOFF_INITIALrow) with slice-6's CLI reference (egg-orch overseer alert). Slice-6 intent preserved; main's content preserved; nomcp__*re-exposure. ✅sandbox/agent-config/rules/orchestrator.md: kept slice-6's CLI-only "BRC consensus verbs" / "BRC introspection verbs" / "Progress + overseer" sections; dropped main's "MCP tool equivalents" / "BRC introspection (#2908)" tables that listedmcp__brc__*tools as live. ✅lookup_open_prrename / unification (from main's PR #2934) integrated cleanly inorchestrator/gateway_client.py,orchestrator/routes/pipelines.py, and the threeorchestrator/tests/test_*files. ✅
🟡 Medium — docs/reference/conditional-ack.md:104-110 invokes a deleted tool
The "Satisfying agent path" walkthrough still presents an executable code block calling mcp__brc__resolve_obligation:
mcp__brc__resolve_obligation \
reviewer_role="reviewer_contract" \
producer_role="coder" \
commit_sha="<sha>" \
note="Mechanical patch-path rewrites cherry-picked from coder's prior commit"
This is the one normative code example for the in-cycle resolution flow. Agents reading this guide as a how-to will copy it verbatim and fail, because slice-6 deleted the tool. The same file's pointer block at line 125 correctly directs readers to egg-orch brc resolve-obligation — the example needs to match. Suggested replacement:
egg-orch brc resolve-obligation \
--reviewer-role reviewer_contract \
--producer-role coder \
--commit-sha <sha> \
--note-file /tmp/resolve-note.md(Slice-5 added --note-file PATH / - stdin sentinel for --note; bare --note "…" also works for shell-safe strings.)
Why this is Medium not NIT: this isn't a stale prose reference, it's the primary worked example for the satisfier path. The merge touched this file (line 125 area), so the diff has the area open.
NIT — three remaining present-tense mcp__* references
The slice-6 doc sweep missed these three. They're internally inconsistent with explicit past-tense statements in the same files:
docs/reference/conditional-ack.md:41— "The same surface is available via themcp__brc__ackMCP tool — passpre_merge_conditionalongsideartifact_referencesandreason." → was available; or drop the sentence.docs/reference/conditional-ack.md:65— "…the satisfier callsmcp__brc__resolve_obligationto mark the obligation satisfied." → callsegg-orch brc resolve-obligation.docs/reference/orchestrator-cli.md:27, 29, 30— three rows describing CLI commands as "verb-level alias formcp__phase__get_context" / "mcp__brc__get_state" / "mcp__brc__list_blocking". Same file at line 573 says: "the previousmcp__brc__*MCP wrappers around these same handlers were retired with the rest of the agent-side MCP surface in #2908 slice-6". Either drop the alias phrasing or past-tense it ("formerly aliased bymcp__brc__get_statebefore #2908 slice-6").
These don't break a runtime path (unlike the Medium above), but they tell agents the MCP surface is live, which is exactly what slice-6 is trying to undo.
Net
Two clean merge resolutions, mechanical integration of main's lookup_open_pr work, no new code regressions in the delta. One MEDIUM (active how-to invoking deleted tool) and three NITs (stale present-tense prose) — all confined to two doc files. None block merge if the operator wants to ship and follow up; the MEDIUM is worth a single-commit fix before merge because the code-block form has zero ambiguity for agents copy-pasting it.
— Authored by egg
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
|
egg review completed. View run logs 23 previous review(s) hidden. |
|
egg is resolving merge conflicts for this PR... |
…nd tools/sdlc.py - docs/reference/agent-tools.md: kept slice-6 reorientation (CLI surface section), dropping main's MCP-tool-table row update for mcp__sdlc__check_file_restriction; the underlying handler change (phase-aware behavior, #2968) lands via the preserved handlers.restrictions.check_file_restriction handler and the new shared/egg_restrictions/phase_patterns.py mirror. - sandbox/egg_agent_tools/tools/sdlc.py: kept slice-6 deletion; main's update was to a tool that no longer exists after slice-6. — Authored by egg
Conflict Resolution SummaryResolved merge conflicts with
The remaining auto-merged files (gateway, orchestrator routes, docs, handler-layer changes, new Please review:
— Authored by egg |
|
egg conflict resolution completed. View run logs |
There was a problem hiding this comment.
No agent-mode design concerns.
Re-review delta since c8bb00b2 is a single merge commit (06ddc129) bringing origin/main into the slice-6 branch. The conflict resolutions in docs/reference/agent-tools.md and sandbox/egg_agent_tools/tools/sdlc.py correctly keep the slice-6 deletions and drop main's updates that targeted the now-deleted MCP surface — consistent with the slice-6 goal of collapsing the dual MCP/CLI surface down to the CLI. The underlying check_file_restriction handler change from #2968 is preserved via the retained handler layer + new shared/egg_restrictions/phase_patterns.py mirror, so functionality is not lost.
Slice-6 still aligns well with agent-mode principles: it removes a dual-surface carrying cost, eliminates the MCP-loop reentry seam that non-Claude models fell out of (#2906), and leaves the operator-facing orchestrator MCP server (port 9850) untouched. The CLI becomes the single agent surface for pipeline lifecycle operations.
— Authored by egg
|
egg agent-mode-design completed. View run logs 23 previous review(s) hidden. |
|
egg review failed. View run logs 23 previous review(s) hidden. |
|
egg is resolving merge conflicts for this PR... |
…nd post-checkpoint-removal cleanup
Conflicts resolved:
- gateway/gateway.py: kept HEAD's CLI references (slice-6 retires the MCP surface)
- docs/guides/concurrent-execution.md, docs/guides/sdlc-pipeline.md, docs/index.md,
docs/reference/agent-tools.md, gateway/README.md,
sandbox/agent-config/rules/{README,contract,environment,orchestrator}.md:
kept HEAD's post-slice-6 CLI-only narrative and dropped stale checkpoint refs
(main removed the checkpoint subsystem in #2993).
- DU (PR deleted, main modified): kept slice-6 deletions of
sandbox/egg_agent_tools/tools/{__init__,_common}.py,
tests/sandbox/egg_agent_tools/test_{full_tool_registry,server}.py,
tests/tools/test_mcp_cli_drift.py.
- UD (PR modified, main deleted): accepted main's deletion of
sandbox/agent-config/rules/checkpoint.md and
shared/egg_contracts/checkpoint_cli.py.
Conflict Resolution SummaryResolved merge conflicts with
Please review:
— Authored by egg |
|
egg conflict resolution completed. View run logs |
There was a problem hiding this comment.
No agent-mode design concerns.
Re-reviewed the delta since 06ddc129. The only PR-authored change since the prior approval is the merge commit 399e98fd that brings origin/main (notably #2993, the checkpoint-subsystem removal) into the branch and resolves conflicts.
Verified the conflict resolutions preserve slice-6's CLI-over-MCP direction without introducing new agent-mode patterns:
gateway/gateway.py— HEAD kept; slice-6'srecommended_clierror payload (and the rest of the MCP-deletion surface) survives the merge. Newrouting_policy.py/RouteHopimports came in from main and are out of scope for this design review.- Agent-facing rules (
sandbox/agent-config/rules/{README,contract,environment,mission,orchestrator}.md) — consistently past-tense the slice-6 retirement and consistently describeegg-orch/egg-contractas the agent surface. The DU resolution accepted main's deletion ofcheckpoint.md, which is consistent with main's removal of the checkpoint subsystem; the previously-approved CLI-only narrative is preserved. docs/index.md,docs/reference/agent-tools.md,docs/architecture/*— checkpoint references removed (downstream of #2993). Themcp__*tool-namespace list shown in the slice-6 retirement callout correctly dropsmcp__checkpoint__*(it was already dead on main).- DU resolutions (
sandbox/egg_agent_tools/tools/{__init__,_common}.py,tests/sandbox/egg_agent_tools/test_{full_tool_registry,server}.py,tests/tools/test_mcp_cli_drift.py) — slice-6 deletions correctly kept against main's modifications. - UD resolutions (
sandbox/agent-config/rules/checkpoint.md,shared/egg_contracts/checkpoint_cli.py) — main's deletions correctly accepted.
No new pre-fetching, structured output for humans, post-processing pipelines, rigid procedures, prompt-level security, direct LLM API calls, or hardcoded model identifiers introduced by the merge. The CLI-as-single-agent-surface direction the prior reviews approved is preserved end-to-end.
Approving on agent-mode design grounds.
— Authored by egg
|
egg agent-mode-design completed. View run logs 23 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review — slice-6/6 (#2908)
Delta since prior review at c8bb00b2: 2 merge commits (06ddc129, 399e98fd) pulling main into the branch. No author-side fixes to the issues flagged in the prior review.
Confirmed clean
- Merge conflict resolutions in
399e98fdare mechanically correct:gateway/gateway.pycomment-only conflict properly resolved to HEAD's CLI-recommendation framing.docs/guides/concurrent-execution.md,docs/guides/sdlc-pipeline.md,docs/index.md,docs/reference/agent-tools.md,gateway/README.md, and the foursandbox/agent-config/rules/*.mdfiles correctly drop checkpoint references (#2993 from main).- Accepted main's deletions of
sandbox/agent-config/rules/checkpoint.mdandshared/egg_contracts/checkpoint_cli.py.
tests/sandbox/egg_agent_tools/test_mcp_surface_retired.pyadversarial_DELETED_SYMBOLSwalk still pins the deletion correctly.gateway/gateway.py:1422-1435pipeline-push 403 payload usesrecommended_cli="egg-orch consensus propose --push"with past-tense MCP retirement framing — good.- Checkpoint imports (
checkpoint_handler,transcript_buffer,_is_checkpoint_repo_for_request) cleanly absent fromgateway/gateway.py.
Unaddressed from prior review
These were called out at c8bb00b2 and remain unfixed after two intervening merge commits.
MEDIUM — docs/reference/conditional-ack.md:104-110
Code block still invokes the deleted mcp__brc__resolve_obligation MCP tool as if it were a live shell command:
mcp__brc__resolve_obligation \
reviewer_role="reviewer_contract" \
producer_role="coder" \
commit_sha="<sha>" \
note="..."
This is the canonical reference doc for the conditional-ACK workflow — a reader who copies this verbatim will get "command not found." Replace with the CLI that already exists at sandbox/egg_lib/orch_cli.py:4390-4457:
egg-orch brc resolve-obligation \
--reviewer-role reviewer_contract \
--producer-role coder \
--commit-sha <sha> \
--note-file /tmp/resolve-note.md
(Use --note-file since the note is LLM-authored prose — same rationale as the rest of slice-5's prose-via-file convention.)
NIT — docs/reference/conditional-ack.md:41, 65
Two narrative references to mcp__brc__ack (line 41) and mcp__brc__resolve_obligation (line 65) still describe the deleted MCP surface as the available path. Should name the egg-orch brc ack / egg-orch brc resolve-obligation CLIs instead.
NIT — docs/reference/orchestrator-cli.md:27, 29, 30
Three "verb-level alias for mcp__phase__get_context / mcp__brc__get_state / mcp__brc__list_blocking" notes describe a relationship that no longer exists. The same file at line 573 already uses past tense ("were retired"), so this is just internal inconsistency. Drop the alias parentheticals or rephrase as historical context.
New findings
NIT — docs/releases/agent-mcp-tools.md:7
Supersedence banner lists egg-orch / egg-contract / egg-checkpoint as the replacement surface, but egg-checkpoint was removed from sandbox/bin/ by main's #2993 before this branch landed. Drop the egg-checkpoint reference.
NIT — shared/egg_restrictions/phase_patterns.py:14
The module docstring still says:
The
check_file_restrictionMCP tool runs in the sandbox — nowhere near the gateway — and historically consulted only the role layer
Slice-6 deleted that MCP tool; the present-tense "runs" is now stale. Rephrase as past-tense (matching the past-tense framing the rest of slice-6 already applies). This docstring originated in main (#2968), so it's collateral — but slice-6 made it stale and the cleanup belongs here.
Verdict
Documentation-only findings; safety floor (gateway phase-gate + adversarial guard test) holds. No security or correctness concerns. The MEDIUM is the only one that meaningfully misleads a reader — the others are stale-reference cleanup.
Reiterating the prior MEDIUM since it was not addressed despite two intervening commits.
— Authored by egg
|
egg review completed. View run logs 23 previous review(s) hidden. |
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
|
egg is resolving merge conflicts for this PR... |
|
Resolved merge conflicts with Conflict resolution summaryMain added Confluence/Jira MCP tool wrappers via #2994 / #2996, which directly clashed with slice-6's deletion of the agent-side MCP tool surface. Followed the established pattern from the previous merge (399e98f): slice-6's deletion wins for the agent-side tool layer; main's handler-layer additions are kept (slice-6 explicitly preserves
Verification
— Authored by egg |
|
egg conflict resolution completed. View run logs |
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
MCP→CLI deletion: delete agent MCP server + migrate tests
Files affected:
sandbox/egg_agent_tools/tools/brc.pysandbox/egg_agent_tools/tools/checkpoint.pysandbox/egg_agent_tools/tools/message.pysandbox/egg_agent_tools/tools/phase.pysandbox/egg_agent_tools/tools/progress.pysandbox/egg_agent_tools/tools/sdlc.pysandbox/egg_agent_tools/tools/task.pysandbox/egg_agent_tools/tools/__init__.pysandbox/egg_agent_tools/tools/_common.pysandbox/egg_agent_tools/tools/_registry.pysandbox/egg_agent_tools/tools/_tool_compat.pysandbox/egg_agent_tools/server.pyshared/egg_agent/client.pytests/tools/test_mcp_cli_drift.pyintegration_tests/test_sandbox_mcp_tools_e2e.pytests/sandbox/egg_agent_tools/test_server.pydocs/architecture/sandbox.mddocs/reference/agent-tools.mdCLAUDE.mdintegration_tests/test_mcp_to_cli_latency.pyTasks:
sandbox/egg_agent_tools/tools/{brc,checkpoint,message,phase,progress,sdlc,task}.py(~1,515 LOC). Delete the 4 infrastructure files (sandbox/egg_agent_tools/tools/{__init__,_common,_registry,_tool_compat}.py). Delete theSYSTEM_PROMPT_NUDGEconstant atsandbox/egg_agent_tools/server.py:61and thebuild_sandbox_mcp_serverfactory in the same file. The shared handler layer atsandbox/egg_agent_tools/handlers/*.pyis RETAINED — both surfaces collapse to one (the CLI / direct handler path), not zero.server.pyis reduced to whatever else lives there (only the MCP-specific exports — verify via grep at implement-time; if no non-MCP exports remain, deleteserver.pytoo).rg 'from egg_agent_tools.tools|build_sandbox_mcp_server|SYSTEM_PROMPT_NUDGE'across the tree returns zero matches; the handler layer atsandbox/egg_agent_tools/handlers/*.pyunchanged; deletion lands in a single coder commit.shared/egg_agent/client.py:299–353: theEGG_MCP_TOOLSenv-flag gate at :311 (no orphan flag — per architect v2 slice-6 goal "INCLUDING the EGG_MCP_TOOLS env-flag check at line 311"), thebuild_sandbox_mcp_serverimport at :316, themcp_servers = build_sandbox_mcp_server()call at :319, theoptions.mcp_servers = {...}assignment at :323, and theSYSTEM_PROMPT_NUDGEappend at :332. The operator-facingorchestrator/mcp_server.pyis out of scope — confirm via grep that nothing in the deletion accidentally touches it.shared/egg_agent/client.pyno longer references MCP tools or theEGG_MCP_TOOLSenv flag; client.py options no longer setmcp_servers(or sets only the operator-facingorchestrator/mcp_server.pyif separately registered — confirm by grep);rg 'EGG_MCP_TOOLS'across the tree returns zero matches (no orphan references).tests/tools/test_mcp_cli_drift.py(delete; the MCP↔CLI drift contract no longer applies since the MCP surface is gone). The shared handler layer keeps both surfaces honest in spirit; the formal drift suite is retired.tests/tools/test_mcp_cli_drift.pydeleted;rg 'test_mcp_cli_drift'across the tree returns zero matches; existing test suite remains green.integration_tests/test_sandbox_mcp_tools_e2e.pyto exercise the CLI surface. The architect v2 slice-6 goal specifies the test must "preserve the SDK-spawn exercise rather than collapsing to direct-handler" — the agent's first action becomesegg-orch consensus ack/nackvia stdin/file (using the slice-5 prose plumbing from TASK-5-1). Where the original tests assert the MCP-tool surface (schema, registration, system-prompt-nudge), replace with equivalent assertions: subcommand exists,--helpmirrors the expected fields, stdin/file round-trip works. Where they assert handler-layer behaviour, simplify to direct handler invocation. Migratetests/sandbox/egg_agent_tools/test_server.pyseparately — the MCP-registration test goes away with the MCP server.integration_tests/test_sandbox_mcp_tools_e2e.pyexercises the CLI surface AND the SDK-spawn end-to-end (not just the handler layer);tests/sandbox/egg_agent_tools/test_server.pyno longer asserts MCP registration; both files pass undermake test;rg 'from sandbox.egg_agent_tools.tools'in test paths returns zero matches.docs/architecture/sandbox.md,docs/reference/agent-tools.md(locate via Grepdocs/for "MCP tools" / "SYSTEM_PROMPT_NUDGE" / "EGG_MCP_TOOLS"), and the projectCLAUDE.mdQuick Reference if it references the agent MCP surface. Cover: the MCP tool surface is retired in favour of the CLI; theEGG_MCP_TOOLSenv flag is no longer recognised; the shared handler layer atsandbox/egg_agent_tools/handlers/*.pybacks both today (CLI only after this slice); the operator-facingorchestrator/mcp_server.pyis unaffected and remains the operator's MCP surface. The documenter role has direct write access toCLAUDE.mdvia theDEFAULT_DOCS_GLOBS**/*.mdpattern atshared/egg_restrictions/patterns.py:177-181— no staging workaround needed (reviewer_plan non-blocker).EGG_MCP_TOOLSreferences removed;orchestrator/mcp_server.pyreferences preserved; CLAUDE.md edited in place (if applicable) — no.egg-state/agent-outputs/staging detour.integration_tests/test_mcp_to_cli_latency.py(directly underintegration_tests/;local_pipeline/does not exist). Baseline-capture strategy: TASK-5-7 captures the baseline DURING slice-5 (before slice-6's deletions land) on the still-live MCP surface using theegg_stackfixture and commits the result to.egg-state/agent-outputs/latency-mcp-baseline.json. TASK-6-6 (this task) drives the SAME consensus shape on the post-deletion CLI-only surface — also viaegg_stack(session-scoped atintegration_tests/conftest.py:340) — reads the slice-5-captured baseline, and asserts the comparison. Both measurements use the same real-LLM tier — the only delta is the tool surface. Latency regression budget: ≤ 5%. On regression > 5%, slice-6 surfaces a structuredOVERSEER_ALERTprioritymediumwith the measured delta for human review (the fallback decision is whether to ship the persistentegg-orchdaemon per architect od-5).latency-mcp-baseline.jsonexists under.egg-state/agent-outputs/at slice-6 entry (captured by TASK-5-7); the post-deletion measurement is captured to.egg-state/agent-outputs/latency-mcp-vs-cli.json; assertion fails only if regression exceeds the 5% budget; on failure the test surfaces a structuredOVERSEER_ALERTprioritymediumwith the measured delta. No vendored MCP source tarball. NoScriptedProviderimport or reference. Test gated viaegg_stack(skips if_kubectl_available()returns False).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-5Slice slice-6 of pipeline issue-2908-impl2. Stacked on top of
egg/issue-2908-impl2/slice-5.