[#2717] Substrate-swap follow-up: integration branch - #2731
Merged
Conversation
Surface 7 multi-choice decisions (cq-1..cq-7) and 6 open-ended feedback questions covering the bridge-gap design, slice decomposition, Agent- tool dispatcher migration, R15 model-(b) timing, R2 hook validation timing, cost cap default, and k3s adapter scope.
Maps refine-phase HITL decisions (cq-1 through cq-7 + feedback Q1-Q6) onto concrete component changes across 5 slices: 1. Bridge gap closure (Option B stage-script MVP) + R2 hook role-resolution spike (2-subagent worked example). 2. Plan-phase substrate (architect/task_planner/risk_analyst + reviewer_plan). 3. Implement-phase substrate (coder/tester/documenter + 5 reviewers; cq-3 empirical metrics collected here). 4. PR-phase substrate + 5x2 conformance matrix + scope-fence removal. 5. Parallel hardening: EggHarnessSpawner + local-run CLI, cost cap (EGG_PIPELINE_MAX_AGENT_INVOCATIONS=200), drop v0.x markers, ADR refresh. Includes runtime-primitive surfacing per #2594: every cited primitive has file:line evidence and is tagged with purpose (deployed-pod vs test-only) and execution context (in-sandbox-agent vs trusted-CI-runner).
Adds risk_analyst output (.egg-state/agent-outputs/2717-risk_analyst-output.json) covering 18 risks (R17–R34) specific to the post-spike rollout that wires the remaining 15 roles + plan/implement/pr phases onto the Claude Code substrate. Key risks called out: - R17: HITL bridge dual-architecture (cq-1 Option C-hybrid) - R18: 15-rubric authorship + structural depth-gap closure - R19/R29: 8-way harness re-host stress on parent session (cq-3 deferred) - R20: existing reviewer rubrics need substrate-aware extension (Q5 declined) - R21: 5-issue conformance reproducibility (Q1 fixed set) - R22: #2261 slice-15 coordination - R23: cost-cap at 200 (cq-6) needs visibility - R26: EggHarnessSpawner as 3rd protocol implementation (Q3 Option A) - R27: MCP-validator fallback structural enforcement gap - R31: 15-subagent trust-context scaling (Q4 declined extras) - 11 implementation recommendations with priorities + open questions for implement-phase HITL. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Decompose the rollout into the phase-sequential chain settled by
the refine HITL (cq-2 = Option 3):
slice-1 (bridge gap + R2 hook validation + refine reviewers)
-> slice-2 (plan-phase substrate)
-> slice-3 (implement-phase substrate + daemon HITL bridge)
-> slice-4 (pr-phase + 5-issue conformance matrix +
scope-fence removal)
-> slice-5 (hardening: cost cap + EggHarnessSpawner +
R15 contingent + fork primitive + ADR +
v0.x marker drop)
Each slice has exactly one DAG parent (forest constraint per #2137
satisfied). 42 tasks across the five slices; primitives audit per
#2594 cites every named symbol with file:line or marks (NEW —
TASK-X-Y). Trust-boundary scope is named: conformance tests live
under integration_tests/regression/ (substrate-portable), not
integration_tests/local_pipeline/ (kubectl-gated).
Blocking fixes: - TASK-1-5 (R2 spike): the harness re-host model bypasses the PreToolUse hook entirely (shared/egg_harness/client.py uses its own ToolRegistry.set_permission_callback, no hook_entry import). Add TASK-1-9 introducing a test-only nested-Agent-tool dispatch fake at integration_tests/regression/_agent_tool_fake.py (underscored helper => coder-owned per MCP file-restriction check). Reframe TASK-1-5 to use the fake; document the empirical- vs-test-fake limitation in the test docstring. Production stays on ClaudeCodeSpawner harness re-host per cq-3. - TASK-4-4 (conformance matrix): switch from "recorded transcripts that no task produces" to MagicMock-style stubs mirroring test_substrate_smoke.py:56. Document the trade-off in the test docstring and note that #2714's closed state is irrelevant per feedback Q1. - TASK-4-2 (fence removal): cite both :212 (call site) and :807-826 (method def) so the coder removes both, not just the call. Non-blocking fixes: - TASK-2-5: agent-side enforcement target moved from orchestrator/mcp_tools.py (wrong surface) to sandbox/egg_agent_tools/handlers/restrictions.py (the in-sandbox tool handler that exposes check_file_restriction at :70 today). - TASK-2-6 / TASK-2-7: renumbered to match file order. - TASK-1-6: explicit dependency note on TASK-1-4. - TASK-3-2: daemon must detach via start_new_session=True so it survives the calling Bash exit. - TASK-1-1: pending_hitl envelope marked as the shared state- serialization contract between Option B (flattened) and Option A (daemon), closing risk_analyst R17 dual-bridge concern. - TASK-5-5 fork primitive: stays on harness re-host (subprocess + egg_harness.run_agent) instead of Agent-tool dispatch, aligning with cq-3's "decide empirically post-implement" deferral. - Primitives table: LocalWorktreeManager line corrected to :59; _maybe_fence dual-location citation added.
This was referenced May 19, 2026
jwbron
marked this pull request as ready for review
May 20, 2026 01:04
#2724) * docs: add claude-code substrate to index and structure docs [doc-updater] (#2718) * docs: add claude-code substrate to index and structure docs * docs: fix substrate/claude_code listing per reviewer feedback - Drop incorrect '+ Agent tool' from spawner.py description; the spike runs egg_harness.run_agent in-process and does NOT dispatch via the Agent tool (Agent-tool spawner is an ADR follow-up). - Add hook_entry.py to the listing — it is the standalone PreToolUse hook script and the largest file in the package (~31 KB). - Clarify policy.py is the PolicyEnforcer adapter wrapping hook_entry.py. - List settings.template.json for navigability. Addresses egg-reviewer CHANGES_REQUESTED on PR #2718. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: update deployment guide for Cilium portmap CNI changes [doc-updater] (#2716) * docs: document portmap CNI and iptables reboot requirement (#2713) * docs: mention netfilter-persistent as alternative to re-running after reboot Addresses non-blocking review feedback on #2716. The reviewer noted that install-cilium.sh's own comment block calls out netfilter-persistent / iptables-restore as a system-level persistence alternative to re-running the script after every reboot. Mirror that in the operator-facing doc so long-running k3s host operators know they have an option beyond manual re-runs. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: add reconcile_autostash_pop_conflict to push diagnostic list (#2720) Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> * slice-1 coder: bridge driver + R2 nested-dispatch fake + loader expansion Implements three #2717 slice-1 coder tasks toward the substrate-swap follow-up rollout per cq-1=Option C hybrid bridge (refine/plan flattened, implement daemon), cq-2 phase-sequential slicing, cq-5 early-spike R2 validation, and cq-3 harness re-host alignment. TASK-1-1 (bin/run_pipeline.py): flattened single-yield stage driver that advances `run_pipeline_in_process` to its next yield, serialises the yielded HITLDecision to .egg-state/contracts/<id>.json under a stable `pending_hitl` envelope schema (version, decision, answer, answer_log, status, result, error), and exits. Cross-process generator state is recovered by replaying `answer_log` on each invocation — viable for refine/plan because the generator is deterministic; slice-3's daemon variant (TASK-3-2) consumes the same envelope schema so the two bridges share a state-serialization contract (risk_analyst R17 mitigation). End-to-end round-trip verified: first invocation yields preflight decision; operator answer round-trips through the contract; second invocation replays and advances to the refine-gate decision. TASK-1-9 (integration_tests/regression/_agent_tool_fake.py): test- only nested-Agent-tool dispatch fake. Simulates Claude Code's Agent tool by spawning a child subprocess with controlled EGG_AGENT_ROLE; the child invokes orchestrator/substrate/claude_code/hook_entry.py `decide(...)` directly. Validates the hook-logic half of R2 — given accurate EGG_AGENT_ROLE propagation, does the hook deny a write that violates the *child's* role pattern even when the parent's role would allow it? Hard import guard prevents production use; the file is coder-owned (underscored helper name, mirroring _helpers.py) rather than tester-owned. Production dispatch stays on ClaudeCodeSpawner (harness re-host) per cq-3. TASK-1-6 (orchestrator/substrate/__init__.py): extends `_load_egg_sdlc_role_rubric` so reviewer_refine and reviewer_agent_design are recognised as supported (alongside the existing refiner). Introduces a `_ROLE_RUBRIC_SLICES` mapping that names which #2717 rollout slice ships each role's rubric (slice-1: refine team; slice-2: plan team; slice-3: implement team) so future slice loaders can extend the set declaratively, and a `_RUBRIC_LANDED_ROLES` set documenting which rubric .md files exist on disk today. Roles outside the landed set raise a structured ValueError citing the correct rollout slice ("deferred to follow-up slice-2 of issue #2717's rollout"). The acceptance criterion's "follow-up slice 2" hint for architect is now produced. Lint clean (ruff check + format); mypy clean on the new files; the pre-existing 6 mypy errors in substrate/__init__.py:180-198 are unrelated to this change. 52 existing claude-code-substrate tests still pass. Refs #2717 (slice-1 coder). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): refine-team rubrics + flattened-bridge docs + ADR rollout deltas Slice 1 of the #2717 substrate-swap rollout adds two refine-team reviewer rubrics, closes the heredoc-HITL bridge gap for refine-phase via a flattened bin/run_pipeline.py stage driver, and adds the cq-5 R2 spike for nested PreToolUse-hook role-routing. This commit lands the documenter-owned half: TASK-1-4: New reviewer rubric files at plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_refine.md plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_agent_design.md mirroring the layout of plugins/refine-plan/skills/refine-plan/agents/ so the in-process orchestrator's build_system_prompt(sources) loader picks them up without per-skill custom logic. Both files carry frontmatter (name + description) and the body documents the substrate-specific operational deltas (worktree path, PreToolUse-enforced restrictions, AskUserQuestion surfacing, verdict path) on top of the same rubric the k3s reviewers use. TASK-1-2: SKILL.md is updated to replace the "Walking-skeleton bridge gap" callout with a "How the flattened bridge works" section that names pending_hitl as the single-yield carrier and documents the skill→driver loop. The "What the skill is designed to do" step list moves from aspirational to actually-shipping. The R2 PreToolUse-hook section points at the new test infrastructure (test_pretooluse_hook_nested.py + _agent_tool_fake.py) and the r2-verdict.json file. Frontmatter description re-flows to reflect the slice-1 scope. TASK-1-8: docs/architecture/claude-code-substrate.md is updated for the ADR-level audit trail: - Title and status banner reframe from "spike" to "spike → rollout". - cq-2 / cq-7 / cq-11 table rows reflect what slice 1 lands. - The in-process orchestrator section gets a "The flattened bridge" subsection naming the cq-1 hybrid (Option C) and the slice-3 daemon variant that consumes the same pending_hitl envelope shape (R17 mitigation). - The egg-sdlc plugin section enumerates the three refine-team rubrics and the new bin/run_pipeline.py driver. - The R2 risk-mitigation subsection points at the slice-1 worked example and the slice-5 contingent fallback (cq-6 option 2 + R15 model (b)). - The R15 subsection makes the model (a) → (b) migration contingent on the slice-1 R2 verdict. - The "Open work" + "Follow-up issue draft" sections are replaced with a unified "Rollout deltas" section split into Completed-in-this-rollout (3 slice-1 items, marked with [x] + strikethrough on the obsolete text) and Pending-in-this-rollout (9 items mapped to slices 2-5). The acceptance bar is unchanged. - The primitives table picks up the four new slice-1 modules (bin/run_pipeline.py, _agent_tool_fake.py, test_pretooluse_hook_nested.py, the two new reviewer rubrics). - The conformance-proof section names the slice-1 regression-test additions (test_bridge_flattened_round_trip.py, test_rubric_loader.py). - Stale anchor links to the removed "Follow-up issue draft" section are redirected to the new "Rollout deltas" anchor. These doc changes satisfy TASK-1-2, TASK-1-4, and TASK-1-8 from slice 1 of the #2717 plan; no source or test files are touched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester: rubric loader + bridge round-trip + R2 nested-dispatch tests Adds three test files covering #2717 slice-1's tester contract tasks: TASK-1-7 (shared/tests/test_rubric_loader.py): unit tests for `_load_egg_sdlc_role_rubric`. Pins the four AC cases: refiner regression, reviewer_refine load, reviewer_agent_design load, architect raises ValueError with the updated "follow-up slice 2" diagnostic. Adversarial probing layered on: AgentRole enum vs str input equivalence, defense against path-traversal role values, structured fence for unshipped plan-phase roles (reviewer_plan, reviewer_code, task_planner). Eight of 10 tests pass today; two (reviewer_refine, reviewer_agent_design loads) are documenter-dependency failures that flip green once task-1-4 lands the rubric .md files. TASK-1-3 (integration_tests/regression/test_bridge_flattened_round_trip.py): end-to-end round-trip test for the flattened bridge driver. Runs the real `bin/run_pipeline.py` in a fresh subprocess twice against a deterministic pipeline id: stage A captures the preflight HITLDecision into `pending_hitl.decision`, the test writes `answer="approve" + status="answered"`, stage B re-enters the process and replays the answer to advance to the refine-gate decision. Validates that the generator state survives via the contract-state round-trip across process exit. Substrate isolation via a `-c` shim that monkey-patches `orchestrator.substrate.select_substrate` to a MagicMock bundle — no real Claude Code / Anthropic API call. Also pins driver-side idempotency (a re-invocation without a new answer must not silently advance the generator). TASK-1-5 (integration_tests/regression/test_pretooluse_hook_nested.py): cq-5 early-spike R2 verdict test. Uses task-1-9's `_agent_tool_fake` to drive a deterministic nested-dispatch scenario: parent_role=architect + child_role=tester + write_target=orchestrator/foo.py — asserts the hook returns `{"decision": "block", "reason": ...}` and that the deny reason names the child (tester) role rather than the parent. Writes `.egg-state/<pipeline_id>/r2-verdict.json` with the pass verdict per AC. Adds in-role allow control + cross-role probe (parent=coder, child=tester writing orchestrator/* — must deny by the child's role) + dataclass shape pin + EGG_AGENT_ROLE leak guard. Docstring documents the empirical-vs-test-fake limitation cq-3 explicitly accepts (production stays on the harness re-host until slice-5 R15 flips dispatch). All 15 of 17 tests pass today. The 2 failing rubric tests are contracted documenter-dependency failures (task-1-4 not landed yet) and are expected to flip green once the documenter ships. Lint clean (ruff check + format). Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): address reviewer_code v1 NACK on SKILL.md envelope + CLI Reviewer_code NACKed v1 with three blockers against SKILL.md (the two rubric files and the ADR were ACKed as-is). This commit addresses all three blockers plus four non-blocking polish items. Blocking fixes in SKILL.md: 1. CLI invocation example was wrong (used --pipeline-id / --issue, but the driver at plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:355-402 takes pipeline_id as a positional arg and --issue-number as the flag). Rewrote the bash loop example to match the actual argparse signature so a copy-paste invocation actually runs. 2. The documented pending_hitl envelope was 5 fields (version, pipeline_id, timestamp, decision, answer) but the driver writes 9 (adds status, result, error, answer_log). Replaced the truncated schema with the full envelope and added per-field semantics (especially the status field, which is the skill's loop predicate: pending / answered / completed / aborted / error). The slice-3 daemon variant inherits all 9 fields. 3. No documented mechanism for the skill body to write pending_hitl.answer (the frontmatter allowed-tools does not include the Write tool). Documented option (a) from the reviewer's NACK: an inline python3 -c "..." invocation, which is covered by the existing Bash(python3 *:*) allowed-tool. The "skill loop" code block now demonstrates the round-trip with a case statement keyed on pending_hitl.status. Non-blocking polish in SKILL.md: - Loop semantics now name "replay" explicitly (the driver spawns a fresh generator and replays answer_log on every invocation; previous text suggested cheap single-step resumption). Added a dedicated "Generator state across invocations (replay semantics)" subsection naming the practical consequence — side effects re-run every call — and pointing at slice 3 as the daemon-variant escape hatch. - Failure-mode bullet for "pending_hitl.decision == null" replaced with the more general "pending_hitl.status ∈ {completed, aborted, error}" bullet so each terminal state has documented diagnostic guidance. Non-blocking polish in docs/architecture/claude-code-substrate.md: - The "Flattened bridge" bullet in the in-process orchestrator section now names the replay path explicitly (promotes answer → answer_log, replays the full log every call, deterministic same-yield-boundary property) and the 5→9 envelope field list mirrors SKILL.md. - Daemon-variant bullet enumerates the same 9-field shape so reviewers comparing slice-1 and slice-3 against the ADR see the full contract. - Schema source-of-truth pointer added to bin/run_pipeline.py:20-46 so future drift triggers fail in one place. Rubric files (reviewer_refine.md, reviewer_agent_design.md) are unchanged — reviewer_code ACKed them in v1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test_bridge_flattened_round_trip: fix subprocess PYTHONPATH The subprocess shim's PYTHONPATH pointed at `<repo>/orchestrator/` and `<repo>/shared/` directly, which lets the subprocess `import substrate` but NOT `import orchestrator.substrate` — the latter requires the *parent* of `orchestrator/` (the repo root) on the path because ``orchestrator/__init__.py`` makes it a real package. Set PYTHONPATH to ``<repo>/shared`` + ``<repo>`` + ``<repo>/gateway`` so: - ``<repo>/shared`` lets ``egg_contracts`` (transitive import from ``orchestrator.substrate.k3s_adapter``) resolve. - ``<repo>`` lets ``import orchestrator`` resolve. Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester v2: fix subprocess PYTHONPATH + non-blocking improvements Addresses reviewer_code v1 blocking #1 + non-blocking items: BLOCKING FIX — subprocess PYTHONPATH: Add `<repo>/orchestrator` to the subprocess shim's PYTHONPATH so bare- name top-level imports inside the `orchestrator/` tree resolve cleanly. Without this, `orchestrator/models.py:16` (`from slice_id_validation import SLICE_ID_PATTERN`) and `in_process.py:531-534`'s bare `from models import HITLDecision` fallback both fail, crashing the driver subprocess with `ModuleNotFoundError` before it yields the first HITL decision. Mirrors the Makefile's `PYTHONPATH := shared:gateway: orchestrator` (test target). Verified: tests now pass with `PYTHONPATH=.:shared:orchestrator pytest <files>` (reviewer_code's exact reproduction env). NON-BLOCKING (reviewer_code v1): - test_bridge_flattened_round_trip.py: drop stale "whichever the coder picks" docstring phrasing — driver locked in positional argv[1]. - test_bridge_flattened_round_trip.py: mirror the driver's ISO-8601 UTC timestamp format in _write_answer instead of `str(time.time())`. - test_rubric_loader.py: extend `test_loader_accepts_enum_and_string_role` parametrization to cover the two NEW roles (reviewer_refine, reviewer_agent_design) — not just the regression role. - test_rubric_loader.py: strengthen path-traversal assertion to verify the allowlist's slice-fence branch fires (not the file-missing-on- disk branch) — pinning the structural defence. - test_pretooluse_hook_nested.py: derive the r2-verdict.json content from the dispatch outcome and write it BEFORE the structured assertions so slice-5 sees an accurate empirical record even when a regression fails one of the assertions. Adds {"r2_verdict": "fail", "reason": ...} payload format for the failure path. Tests verified: 21/21 pass under both the canonical Makefile PYTHONPATH shape and reviewer_code's `.:shared:orchestrator` reproduction shape. Refs #2717 (slice-1 tester, v2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-1 (#2548) * Address slice-1 review: fix install path, bridge answer-write, silent fallbacks Reviewer findings from PR #2724 (egg-reviewer slice-1 review): Blockers (1-4): * The documented `pip install -r requirements.txt` failed — no top-level requirements.txt exists. Switch SKILL.md, plugin.json's `egg.install_instructions`, and `bin/preflight.py` to `pip install .` against pyproject.toml. * SKILL.md claimed Python 3.11+ but pyproject.toml requires >=3.14. Update SKILL.md and preflight to reflect the real floor; correct the matching docstring in orchestrator/substrate/__init__.py. * The inline `python3 -c "..."` answer-write snippet in SKILL.md was broken (shell-interpolated `${ANSWER}` → NameError, deprecated `datetime.utcnow`, non-atomic write). Replace with a dedicated `bin/write_answer.py` helper that reads the JSON-encoded answer from stdin, uses `datetime.now(UTC)`, and writes atomically via tmp + `os.replace`. SKILL.md's loop now invokes the helper. * `_serialise_decision` silently swallowed `model_dump` failures, and `_read_contract` silently overwrote unparseable contracts (dropping `answer_log`). Log to stderr on serialisation fallbacks; refuse to overwrite a corrupted contract and exit 1 instead. Non-blocking (5-11): * `orchestrator/substrate/__init__.py` docstring rewritten to reflect reality: Python 3.14+ introduced PEP 758 (`except A, B:` without parens); ruff under py314 target strips redundant parens, hence `# fmt: skip` on multi-except lines. * Stderr warning in `_advance_generator`'s `finally` so a teardown failure inside `generator.close()` is at least observable. * `allowed-tools` tightened from `Bash(python3 *:*)` to `Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*)` — the two helper scripts are the entire Python surface the skill can invoke. * Re-spawn cost note expanded in SKILL.md: each driver invocation re-runs every prior subagent spawn (real Anthropic API spend); slice-2 compounds to 8 spawns at the final stage. * `_RUBRIC_LANDED_ROLES` registry collapsed into `_CURRENT_LOADER_SLICE` + filesystem probe; same diagnostics, no parallel registry that can drift from disk state. * Abort vocabulary exported as `ABORT_ANSWERS` from `orchestrator.substrate.in_process`; the driver imports it (with a literal fallback when the orchestrator package is unimportable) so the driver, orchestrator, and slice-3 daemon share a single source of truth. * R2 caveat surfaced as an explicit open question for slice-5 sequencing in SKILL.md. Tests: * New `shared/tests/test_write_answer.py` (6 tests, all passing) pins the JSON-encoding round-trip, the timestamp format match against the driver, the atomic-write contract, and the corrupted-contract refusal. * Existing `test_rubric_loader`, `test_run_pipeline_in_process*`, `test_substrate_interfaces`, `test_bridge_flattened_round_trip`, and `test_pretooluse_hook_nested` all still pass. * The pre-existing `test_empty_diff_subprocess_skips_pytest` failure in `tests/tools/` reproduces against HEAD without these changes (detached-HEAD worktree edge case in the test selector). Authored-by: egg * Move skill-loop python3 -c calls into bin/ helpers Address review feedback on PR #2724: - Add bin/read_status.py and extend write_answer.py with --answer-string so every subcommand in SKILL.md's loop body is a single `python3 plugins/.../bin/<helper>.py` invocation. Honors the tightened allowed-tools pattern `Bash(python3 plugins/.../bin/*:*)` per Claude Code's compound-command permission rules — no separate `Bash(python3 -c *)` or `Bash(printf *)` rule needed, no prompt-injection door left open. - Update SKILL.md step 4 to name bin/write_answer.py directly (matches the new loop body). - Replace `slice_hint != _CURRENT_LOADER_SLICE` rubric-loader fence with `slice_hint not in _LANDED_SLICES` (frozenset) so future slices extend rather than replace the landed set — slice-2 won't fence off slice-1's already-shipped refine roles. - Wire test_bridge_flattened_round_trip's _write_answer through subprocess(write_answer.py) so the integration test exercises the production write path end-to-end. - Add test_read_status.py (7 tests) and --answer-string coverage in test_write_answer.py (2 tests). * Address non-blocking review notes on PR #2724 - write_answer.py: clarify --answer-string docstring — the JSON encoding happens at contract serialisation time (json.dumps(contract)), not as a separate json.dumps(answer) step. Reference the special-characters test as the proof of the round-trip. - SKILL.md / read_status.py: document the case statement's intentional fall-through on empty STATUS. read_status.py prints empty + exit 0 when no pending_hitl envelope exists; the case has no *) arm, so the empty value falls through, the case exits 0, and the outer iteration re-invokes run_pipeline.py — which is the recover path. - test_rubric_loader.py: add test_landed_slices_contains_slice1 to mechanically pin the 'extend, don't replace' invariant on _LANDED_SLICES so a future slice cannot silently regress slice-1 by writing frozenset({'slice-2'}) instead of frozenset({'slice-1', 'slice-2'}). --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-orchestrator <egg@localhost>
#2726) * docs: add claude-code substrate to index and structure docs [doc-updater] (#2718) * docs: add claude-code substrate to index and structure docs * docs: fix substrate/claude_code listing per reviewer feedback - Drop incorrect '+ Agent tool' from spawner.py description; the spike runs egg_harness.run_agent in-process and does NOT dispatch via the Agent tool (Agent-tool spawner is an ADR follow-up). - Add hook_entry.py to the listing — it is the standalone PreToolUse hook script and the largest file in the package (~31 KB). - Clarify policy.py is the PolicyEnforcer adapter wrapping hook_entry.py. - List settings.template.json for navigability. Addresses egg-reviewer CHANGES_REQUESTED on PR #2718. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: update deployment guide for Cilium portmap CNI changes [doc-updater] (#2716) * docs: document portmap CNI and iptables reboot requirement (#2713) * docs: mention netfilter-persistent as alternative to re-running after reboot Addresses non-blocking review feedback on #2716. The reviewer noted that install-cilium.sh's own comment block calls out netfilter-persistent / iptables-restore as a system-level persistence alternative to re-running the script after every reboot. Mirror that in the operator-facing doc so long-running k3s host operators know they have an option beyond manual re-runs. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: add reconcile_autostash_pop_conflict to push diagnostic list (#2720) Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> * slice-1 coder: bridge driver + R2 nested-dispatch fake + loader expansion Implements three #2717 slice-1 coder tasks toward the substrate-swap follow-up rollout per cq-1=Option C hybrid bridge (refine/plan flattened, implement daemon), cq-2 phase-sequential slicing, cq-5 early-spike R2 validation, and cq-3 harness re-host alignment. TASK-1-1 (bin/run_pipeline.py): flattened single-yield stage driver that advances `run_pipeline_in_process` to its next yield, serialises the yielded HITLDecision to .egg-state/contracts/<id>.json under a stable `pending_hitl` envelope schema (version, decision, answer, answer_log, status, result, error), and exits. Cross-process generator state is recovered by replaying `answer_log` on each invocation — viable for refine/plan because the generator is deterministic; slice-3's daemon variant (TASK-3-2) consumes the same envelope schema so the two bridges share a state-serialization contract (risk_analyst R17 mitigation). End-to-end round-trip verified: first invocation yields preflight decision; operator answer round-trips through the contract; second invocation replays and advances to the refine-gate decision. TASK-1-9 (integration_tests/regression/_agent_tool_fake.py): test- only nested-Agent-tool dispatch fake. Simulates Claude Code's Agent tool by spawning a child subprocess with controlled EGG_AGENT_ROLE; the child invokes orchestrator/substrate/claude_code/hook_entry.py `decide(...)` directly. Validates the hook-logic half of R2 — given accurate EGG_AGENT_ROLE propagation, does the hook deny a write that violates the *child's* role pattern even when the parent's role would allow it? Hard import guard prevents production use; the file is coder-owned (underscored helper name, mirroring _helpers.py) rather than tester-owned. Production dispatch stays on ClaudeCodeSpawner (harness re-host) per cq-3. TASK-1-6 (orchestrator/substrate/__init__.py): extends `_load_egg_sdlc_role_rubric` so reviewer_refine and reviewer_agent_design are recognised as supported (alongside the existing refiner). Introduces a `_ROLE_RUBRIC_SLICES` mapping that names which #2717 rollout slice ships each role's rubric (slice-1: refine team; slice-2: plan team; slice-3: implement team) so future slice loaders can extend the set declaratively, and a `_RUBRIC_LANDED_ROLES` set documenting which rubric .md files exist on disk today. Roles outside the landed set raise a structured ValueError citing the correct rollout slice ("deferred to follow-up slice-2 of issue #2717's rollout"). The acceptance criterion's "follow-up slice 2" hint for architect is now produced. Lint clean (ruff check + format); mypy clean on the new files; the pre-existing 6 mypy errors in substrate/__init__.py:180-198 are unrelated to this change. 52 existing claude-code-substrate tests still pass. Refs #2717 (slice-1 coder). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): refine-team rubrics + flattened-bridge docs + ADR rollout deltas Slice 1 of the #2717 substrate-swap rollout adds two refine-team reviewer rubrics, closes the heredoc-HITL bridge gap for refine-phase via a flattened bin/run_pipeline.py stage driver, and adds the cq-5 R2 spike for nested PreToolUse-hook role-routing. This commit lands the documenter-owned half: TASK-1-4: New reviewer rubric files at plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_refine.md plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_agent_design.md mirroring the layout of plugins/refine-plan/skills/refine-plan/agents/ so the in-process orchestrator's build_system_prompt(sources) loader picks them up without per-skill custom logic. Both files carry frontmatter (name + description) and the body documents the substrate-specific operational deltas (worktree path, PreToolUse-enforced restrictions, AskUserQuestion surfacing, verdict path) on top of the same rubric the k3s reviewers use. TASK-1-2: SKILL.md is updated to replace the "Walking-skeleton bridge gap" callout with a "How the flattened bridge works" section that names pending_hitl as the single-yield carrier and documents the skill→driver loop. The "What the skill is designed to do" step list moves from aspirational to actually-shipping. The R2 PreToolUse-hook section points at the new test infrastructure (test_pretooluse_hook_nested.py + _agent_tool_fake.py) and the r2-verdict.json file. Frontmatter description re-flows to reflect the slice-1 scope. TASK-1-8: docs/architecture/claude-code-substrate.md is updated for the ADR-level audit trail: - Title and status banner reframe from "spike" to "spike → rollout". - cq-2 / cq-7 / cq-11 table rows reflect what slice 1 lands. - The in-process orchestrator section gets a "The flattened bridge" subsection naming the cq-1 hybrid (Option C) and the slice-3 daemon variant that consumes the same pending_hitl envelope shape (R17 mitigation). - The egg-sdlc plugin section enumerates the three refine-team rubrics and the new bin/run_pipeline.py driver. - The R2 risk-mitigation subsection points at the slice-1 worked example and the slice-5 contingent fallback (cq-6 option 2 + R15 model (b)). - The R15 subsection makes the model (a) → (b) migration contingent on the slice-1 R2 verdict. - The "Open work" + "Follow-up issue draft" sections are replaced with a unified "Rollout deltas" section split into Completed-in-this-rollout (3 slice-1 items, marked with [x] + strikethrough on the obsolete text) and Pending-in-this-rollout (9 items mapped to slices 2-5). The acceptance bar is unchanged. - The primitives table picks up the four new slice-1 modules (bin/run_pipeline.py, _agent_tool_fake.py, test_pretooluse_hook_nested.py, the two new reviewer rubrics). - The conformance-proof section names the slice-1 regression-test additions (test_bridge_flattened_round_trip.py, test_rubric_loader.py). - Stale anchor links to the removed "Follow-up issue draft" section are redirected to the new "Rollout deltas" anchor. These doc changes satisfy TASK-1-2, TASK-1-4, and TASK-1-8 from slice 1 of the #2717 plan; no source or test files are touched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester: rubric loader + bridge round-trip + R2 nested-dispatch tests Adds three test files covering #2717 slice-1's tester contract tasks: TASK-1-7 (shared/tests/test_rubric_loader.py): unit tests for `_load_egg_sdlc_role_rubric`. Pins the four AC cases: refiner regression, reviewer_refine load, reviewer_agent_design load, architect raises ValueError with the updated "follow-up slice 2" diagnostic. Adversarial probing layered on: AgentRole enum vs str input equivalence, defense against path-traversal role values, structured fence for unshipped plan-phase roles (reviewer_plan, reviewer_code, task_planner). Eight of 10 tests pass today; two (reviewer_refine, reviewer_agent_design loads) are documenter-dependency failures that flip green once task-1-4 lands the rubric .md files. TASK-1-3 (integration_tests/regression/test_bridge_flattened_round_trip.py): end-to-end round-trip test for the flattened bridge driver. Runs the real `bin/run_pipeline.py` in a fresh subprocess twice against a deterministic pipeline id: stage A captures the preflight HITLDecision into `pending_hitl.decision`, the test writes `answer="approve" + status="answered"`, stage B re-enters the process and replays the answer to advance to the refine-gate decision. Validates that the generator state survives via the contract-state round-trip across process exit. Substrate isolation via a `-c` shim that monkey-patches `orchestrator.substrate.select_substrate` to a MagicMock bundle — no real Claude Code / Anthropic API call. Also pins driver-side idempotency (a re-invocation without a new answer must not silently advance the generator). TASK-1-5 (integration_tests/regression/test_pretooluse_hook_nested.py): cq-5 early-spike R2 verdict test. Uses task-1-9's `_agent_tool_fake` to drive a deterministic nested-dispatch scenario: parent_role=architect + child_role=tester + write_target=orchestrator/foo.py — asserts the hook returns `{"decision": "block", "reason": ...}` and that the deny reason names the child (tester) role rather than the parent. Writes `.egg-state/<pipeline_id>/r2-verdict.json` with the pass verdict per AC. Adds in-role allow control + cross-role probe (parent=coder, child=tester writing orchestrator/* — must deny by the child's role) + dataclass shape pin + EGG_AGENT_ROLE leak guard. Docstring documents the empirical-vs-test-fake limitation cq-3 explicitly accepts (production stays on the harness re-host until slice-5 R15 flips dispatch). All 15 of 17 tests pass today. The 2 failing rubric tests are contracted documenter-dependency failures (task-1-4 not landed yet) and are expected to flip green once the documenter ships. Lint clean (ruff check + format). Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): address reviewer_code v1 NACK on SKILL.md envelope + CLI Reviewer_code NACKed v1 with three blockers against SKILL.md (the two rubric files and the ADR were ACKed as-is). This commit addresses all three blockers plus four non-blocking polish items. Blocking fixes in SKILL.md: 1. CLI invocation example was wrong (used --pipeline-id / --issue, but the driver at plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:355-402 takes pipeline_id as a positional arg and --issue-number as the flag). Rewrote the bash loop example to match the actual argparse signature so a copy-paste invocation actually runs. 2. The documented pending_hitl envelope was 5 fields (version, pipeline_id, timestamp, decision, answer) but the driver writes 9 (adds status, result, error, answer_log). Replaced the truncated schema with the full envelope and added per-field semantics (especially the status field, which is the skill's loop predicate: pending / answered / completed / aborted / error). The slice-3 daemon variant inherits all 9 fields. 3. No documented mechanism for the skill body to write pending_hitl.answer (the frontmatter allowed-tools does not include the Write tool). Documented option (a) from the reviewer's NACK: an inline python3 -c "..." invocation, which is covered by the existing Bash(python3 *:*) allowed-tool. The "skill loop" code block now demonstrates the round-trip with a case statement keyed on pending_hitl.status. Non-blocking polish in SKILL.md: - Loop semantics now name "replay" explicitly (the driver spawns a fresh generator and replays answer_log on every invocation; previous text suggested cheap single-step resumption). Added a dedicated "Generator state across invocations (replay semantics)" subsection naming the practical consequence — side effects re-run every call — and pointing at slice 3 as the daemon-variant escape hatch. - Failure-mode bullet for "pending_hitl.decision == null" replaced with the more general "pending_hitl.status ∈ {completed, aborted, error}" bullet so each terminal state has documented diagnostic guidance. Non-blocking polish in docs/architecture/claude-code-substrate.md: - The "Flattened bridge" bullet in the in-process orchestrator section now names the replay path explicitly (promotes answer → answer_log, replays the full log every call, deterministic same-yield-boundary property) and the 5→9 envelope field list mirrors SKILL.md. - Daemon-variant bullet enumerates the same 9-field shape so reviewers comparing slice-1 and slice-3 against the ADR see the full contract. - Schema source-of-truth pointer added to bin/run_pipeline.py:20-46 so future drift triggers fail in one place. Rubric files (reviewer_refine.md, reviewer_agent_design.md) are unchanged — reviewer_code ACKed them in v1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test_bridge_flattened_round_trip: fix subprocess PYTHONPATH The subprocess shim's PYTHONPATH pointed at `<repo>/orchestrator/` and `<repo>/shared/` directly, which lets the subprocess `import substrate` but NOT `import orchestrator.substrate` — the latter requires the *parent* of `orchestrator/` (the repo root) on the path because ``orchestrator/__init__.py`` makes it a real package. Set PYTHONPATH to ``<repo>/shared`` + ``<repo>`` + ``<repo>/gateway`` so: - ``<repo>/shared`` lets ``egg_contracts`` (transitive import from ``orchestrator.substrate.k3s_adapter``) resolve. - ``<repo>`` lets ``import orchestrator`` resolve. Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester v2: fix subprocess PYTHONPATH + non-blocking improvements Addresses reviewer_code v1 blocking #1 + non-blocking items: BLOCKING FIX — subprocess PYTHONPATH: Add `<repo>/orchestrator` to the subprocess shim's PYTHONPATH so bare- name top-level imports inside the `orchestrator/` tree resolve cleanly. Without this, `orchestrator/models.py:16` (`from slice_id_validation import SLICE_ID_PATTERN`) and `in_process.py:531-534`'s bare `from models import HITLDecision` fallback both fail, crashing the driver subprocess with `ModuleNotFoundError` before it yields the first HITL decision. Mirrors the Makefile's `PYTHONPATH := shared:gateway: orchestrator` (test target). Verified: tests now pass with `PYTHONPATH=.:shared:orchestrator pytest <files>` (reviewer_code's exact reproduction env). NON-BLOCKING (reviewer_code v1): - test_bridge_flattened_round_trip.py: drop stale "whichever the coder picks" docstring phrasing — driver locked in positional argv[1]. - test_bridge_flattened_round_trip.py: mirror the driver's ISO-8601 UTC timestamp format in _write_answer instead of `str(time.time())`. - test_rubric_loader.py: extend `test_loader_accepts_enum_and_string_role` parametrization to cover the two NEW roles (reviewer_refine, reviewer_agent_design) — not just the regression role. - test_rubric_loader.py: strengthen path-traversal assertion to verify the allowlist's slice-fence branch fires (not the file-missing-on- disk branch) — pinning the structural defence. - test_pretooluse_hook_nested.py: derive the r2-verdict.json content from the dispatch outcome and write it BEFORE the structured assertions so slice-5 sees an accurate empirical record even when a regression fails one of the assertions. Adds {"r2_verdict": "fail", "reason": ...} payload format for the failure path. Tests verified: 21/21 pass under both the canonical Makefile PYTHONPATH shape and reviewer_code's `.:shared:orchestrator` reproduction shape. Refs #2717 (slice-1 tester, v2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-1 (#2548) * docs(#2717 slice-2): plan-team rubrics + SKILL.md plan-phase section Land the four plan-team agent rubric files under plugins/egg-sdlc/skills/egg-sdlc/agents/ for the claude-code substrate of the egg SDLC pipeline (task-2-3): architect, task_planner, risk_analyst, reviewer_plan. Each rubric mirrors its k3s-substrate counterpart in plugins/refine-plan/skills/refine-plan/agents/ for body content (the substrate swap is structurally invisible to the role) and follows the reviewer_refine.md / reviewer_agent_design.md shape from slice-1 for the substrate-specific notes (worktree layout, PreToolUse hook enforcement, HITL-via-AskUserQuestion, concurrent peers in this slice, output path stability). Update plugins/egg-sdlc/skills/egg-sdlc/SKILL.md (task-2-7): - Bump the rollout-status callout from "slice 1 landed" to "slices 1 + 2 landed"; enumerate both the refine and plan rosters. - Replace the "What's NOT in this skill > Plan / implement / pr" bullet's plan deferral with a dedicated **Plan phase** subsection naming the four roles, their spawn order (architect solo, then task_planner + risk_analyst concurrently, with reviewer_plan ACK/NACK on each producer edge), output paths, and the four standard plan-HITL gate options (approve / request_changes / change_approach / stop). - Bump step 8 (phase fence) into a 10-step flow that walks the plan stage spawn order and the plan-HITL gate. The fence now triggers on "approve and continue to implement" with a pointer to slice 3. - Refresh stale "refine-only" / "refine-team subagents" / artifact-path and failure-mode strings to cover both phases. * slice-2 coder: plan-phase BRC stage + rubric loader expansion (#2717) Implements TASK-2-1 + TASK-2-2 for slice-2 of the #2717 rollout. TASK-2-5 closes as no-op per slice-1's R2 = pass verdict (the PreToolUse hook resolves the child's role correctly under nested dispatch; structural enforcement stays hook-side, no MCP-validator-side parallel layer needed). TASK-2-1 — `_run_plan_phase` on `_InProcessOrchestrator` ======================================================== After the refine HITL gate's `approve_continue` answer, the in-process generator now dispatches the plan phase: a `ThreadPoolExecutor` spawns architect / task_planner / risk_analyst concurrently through the same `ClaudeCodeSpawner` the refiner uses, then reviewer_plan is dispatched once with the producer artifacts as its input. `PeerConsensusTracker` drives the BRC mechanics (`handle_propose` / `handle_ack` / `handle_confirmed`); after consensus the stage yields a plan-HITL gate (`HITLDecision` with `phase="plan"` and the canonical 4-way options). The walking-skeleton fence still fires on `approve_continue` past the plan gate — its diagnostic now points at slice-3 / slice-4 of the #2717 rollout instead of #2623. Why the orchestrator records BRC transitions on the subagents' behalf: the in-process substrate's spawner is synchronous (returns AFTER the agent finishes). In the production HTTP daemon the subagents would emit `egg-orch consensus propose/ack/confirmed` themselves and the daemon's gateway listener would advance the tracker. In-process the spawn-completion IS the signal that the subagent proposed or reviewed, so the orchestrator drives the BRC transitions deterministically — the test (harness-faked subagents that never emit BRC messages) and production (real harness agents whose emissions would be no-op duplicates in this path) both reach CONSENSUS_CONFIRMED on the same code path. TASK-2-2 — `_load_egg_sdlc_role_rubric` extension ================================================== `_RUBRIC_LANDED_ROLES` now includes architect / task_planner / risk_analyst / reviewer_plan alongside the slice-1 refine roster (refiner + reviewer_refine + reviewer_agent_design). The structured- error contract for unshipped roles is preserved: implement-team roles (coder / tester / documenter + 5 reviewers) still raise `ValueError` with a slice-3 pointer. The "missing on disk" fallback diagnostic mentions both TASK-1-4 (slice-1 refine) and TASK-2-3 (slice-2 plan) so a reviewer hitting the error in a re-run knows which documenter task needs to land first. TASK-2-5 — agent-side restriction enforcement (no-op) ====================================================== Slice-1's `test_pretooluse_hook_denies_nested_child_write` confirmed the PreToolUse hook denies a child write outside the child's role under nested dispatch (R2 = pass, recorded in `.egg-state/<pipeline_id>/r2-verdict.json` when the test runs). Per the contingent task description, no `sandbox/egg_agent_tools/handlers/restrictions.py` change is needed; structural enforcement stays hook-side. Tester's TASK-2-6 becomes a regression guard asserting the validator helper is a no-op for in-allow-list writes — handled in tester's slice-2 commit. Smoke (manual, in-process, fake subagents) ========================================== * preflight → refine gate → plan gate sequence yields the expected decisions; spawner is called 5 times (1 refiner + 3 plan producers + 1 plan reviewer); tracker.evaluate() reports is_complete=True with all 4 plan-team agents in CONFIRMED state. * Terminal answer at refine gate (e.g. "stop") still returns the refine artifact path — plan phase is NOT entered. * `approve_continue` at the plan gate still raises `NotImplementedError` with the slice-3 / slice-4 pointer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-2 tester: plan-phase BRC E2E + R2-pass restrictions no-op (#2717) TASK-2-4 — integration_tests/regression/test_inprocess_plan_brc.py ================================================================== Plan-phase in-process BRC end-to-end test covering the four AC bullets: * boots `run_pipeline_in_process` against a deterministic pipeline id with harness-faked subagents (no real Anthropic / Claude Code spawn); * advances past the refine HITL gate via `approve` → `approve_continue`; * asserts the plan stage spawns 3 producers (architect, task_planner, risk_analyst) + 1 reviewer (reviewer_plan) — observed via the fake spawner's `.call_args_list`; * asserts the BRC mechanics reach CONSENSUS_CONFIRMED on every producer edge (architect → reviewer_plan, task_planner → reviewer_plan, risk_analyst → reviewer_plan) by reading `_plan_tracker.evaluate()` — the in-process analogue of bus-side CONSENSUS_CONFIRMED messages (the coder's TASK-2-1 implementation drives `PeerConsensusTracker.handle_propose/handle_ack/ handle_confirmed` deterministically since the substrate's spawner is synchronous); * asserts the plan-HITL decision is yielded with `phase="plan"`, `decision_type="phase_gate"`, non-empty `id` / `question` / `options`. Adversarial probing layered on top: * plan stage MUST NOT run when the operator answers `stop` at the refine gate — a regression that fanned into plan on any non-continue answer would burn three unauthorised subagent spawns; * plan stage MUST NOT spawn implement-phase roles — pins the negative invariant against a misrouted `_PHASE_ROLES` lookup; * refiner is spawned exactly once — pins the single-refiner-spawn invariant against an off-by-one role iteration; * every plan-phase spawn carries `EGG_PHASE=plan` in its env — pins the env-propagation contract so spawned subagents see the right phase. The test skips gracefully when the coder's `_run_plan_phase` is absent (scaffold-first per the role's guidance); 7/7 pass against the coder's slice-2 commit 3a46689. TASK-2-6 — tests/sandbox/egg_agent_tools/test_restrictions_validator.py ======================================================================= Contingent test per slice-1's R2 verdict = `pass`. Per the contract task-2-5 description, "If R2 = pass, this task is a no-op (close with note). Tests for this code path land in TASK-2-6 (tester-owned)." Tests for this code path land here as a **no-op regression guard**: * in-allow-list response shape stable (coder/orchestrator, tester/ tests, documenter/docs) — pins the documented gateway-shape fields `{ok, role, path, can_write, reason, alternative_role}` exactly; * cross-role denial shape stable — pins `can_write=False`, denial `reason` references `shared/egg_restrictions/patterns.py`, `alternative_role` names the single producer that can write; * no new validator symbol — asserts `validate_write_target` (and peers) are NOT present on the restrictions handler module, since R2 = pass meant the cq-6 option-2 enforcement work should NOT have landed; * defensive probes — missing `path` raises HandlerError, unknown role raises HandlerError, list-shaped path returns per-path results with documented shapes. 9/9 pass against the unchanged restrictions handler (no slice-2 source edits in `sandbox/egg_agent_tools/handlers/restrictions.py`). Configured-check results: * ruff check . — PASS (all checks passed) * ruff format check . — FAILS on `orchestrator/substrate/in_process.py` (coder's TASK-2-1 file, 5 long-call sites need re-formatting). My test files pass format check cleanly. This is being NACKed to the coder; my proposal will follow once they push the format fix. * mypy on tester-authored files — PASS (251 source files OK). * Custom checks (scripts/check-*.py) — all 13 pass. * `make lint` / `make test` / `make security` cannot complete in this sandbox: the venv sync fails when uv tries to download pinned wheels (flask, oauthlib) — the wheels.pythonhosted.org TLS chain is "UnknownIssuer" inside the sandbox image (same env constraint the slice-1 tester hit). Tests + lint + custom checks were exercised directly via system pytest / ruff / mypy with the Makefile's canonical `PYTHONPATH := shared:gateway:orchestrator`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-2 coder v2: address NACK blockers (#2717) Addresses 3 NACK reviewers from v1 (commit 3a46689): reviewer_concurrency NACKs: - C1: removed `_write_active_role_sentinel` from `_spawn_plan_producer`'s concurrent path. Each producer carries `EGG_AGENT_ROLE` in its own spawn env (the load-bearing role-resolution channel under concurrent dispatch); the single-valued sentinel cannot disambiguate three concurrent role-holders. The synchronous `_spawn_plan_reviewer` retains the sentinel write because it never overlaps another spawn. - C2: added `self._current_phase` state on `_InProcessOrchestrator` (default "refine"; flipped to "plan" at the top of `_run_plan_phase`). `_publish_heartbeat` reads from it so HEARTBEAT messages carry the right phase across the refine→plan transition. Without this, stuck-phase-transition watchdogs filtering by `phase` would see "refine" while the plan stage is actively running. reviewer_code_holistic NACKs: - H1: architect-first then fanout. `_run_plan_phase_inner` now spawns architect synchronously first, records its CONSENSUS_PROPOSE on the tracker, then fans out task_planner + risk_analyst concurrently through a ThreadPoolExecutor with max_workers=2. The architect's per-role output path is passed into each downstream producer's spawn env (`EGG_ARCHITECT_OUTPUT_PATH`) and prompt_text so they can read its `key_design_decisions` rather than re-deriving them. This matches the role-dependency declarations at `shared/egg_contracts/agent_roles.py:398/422` (TASK_PLANNER_ROLE / RISK_ANALYST_ROLE both list ARCHITECT as their sole dependency) and the architect / task_planner / risk_analyst rubric bodies the documenter shipped. - H2: reviewer_plan verdict-JSON parsing. New helpers `read_plan_reviewer_verdicts` (parses `.egg-state/agent-outputs/<issue>-reviewer_plan-output.json`) and `_apply_reviewer_verdicts` drive per-edge ACK / NACK on the tracker based on the reviewer's actual verdict rather than the exit-code-only heuristic v1 used. Fail-closed when the verdict file is missing AND the reviewer's spawn failed (NACK every edge); optimistic ACK only when the verdict file is missing AND the reviewer's spawn returned exit 0 (harness-faked test path), with the "verdict-not-parsed" status surfaced in the placeholder body so the operator sees the discrepancy at the HITL gate. tester NACK: - T1: ran `ruff format` on the affected files. `_spawn_plan_reviewer` also dropped the dead `EGG_PRODUCER_ARTIFACT_PATHS` env var (reviewer_code_holistic v1 non-blocking #3) in favor of per-role `EGG_<ROLE>_OUTPUT_PATH` env vars that the reviewer_plan rubric actually consumes. Non-blocker polish landed alongside the blockers: - `_synthetic_commit_for(role)` derives a per-role hex SHA so the three concurrent ProposalPayload entries remain commit-distinguishable in the tracker (reviewer_concurrency v1 NB #2). - Tracker-guard rejections (`handle_propose` / `handle_ack` / `handle_nack` / `handle_confirmed`) now log via `logging.getLogger("orchestrator.substrate.in_process").warning` instead of silent `except Exception: pass` (reviewer_code_holistic v1 NB). - `_format_plan_placeholder` now also renders reviewer_plan diagnostics + verdict-parsing status (reviewer_code_holistic v1 NB). File decomposition: - ruff format expanded the v1 diff to 1879 lines, breaching the 1500-line hard cap in `scripts/file-size-allowlist.yaml`. Extracted the plan-phase body (~700 lines) into `orchestrator/substrate/_plan_phase.py` as module-level functions that take the `_InProcessOrchestrator` instance as their first argument. The class's `_run_plan_phase` / `_spawn_plan_producer` / `_spawn_plan_reviewer` / `_plan_producer_output_path` / `_read_plan_reviewer_verdicts` methods stay on the class as thin delegates so the existing test surface (and tester's 16 passing tests against v1) keeps the same method names. `in_process.py` now lands at 1093 lines (under both caps); `_plan_phase.py` at 680 lines. Manual in-process smoke (harness fakes, MagicMock subagents): - Happy path: preflight → refine gate → plan gate; spawner called 5 times in order [refiner, architect, task_planner|risk_analyst, task_planner|risk_analyst, reviewer_plan]; tracker reaches `is_complete=True`. - Refine stop: returns refine artifact path; spawner called 1 time (no plan dispatch). - Mixed verdict: with a per_producer verdict JSON {architect:ACK, task_planner:NACK, risk_analyst:ACK}, the tracker records the NACK on task_planner → reviewer_plan; `is_complete=False`; blocking_agents includes reviewer_plan (unresolved critical NACK) and task_planner (not fully ACKed). - Fail-closed: with reviewer spawn exit_code=1 and no verdict file, the tracker NACKs every critical edge; risk_analyst (advisory edge) still confirms; reviewer_plan blocks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address slice-1 review: fix install path, bridge answer-write, silent fallbacks Reviewer findings from PR #2724 (egg-reviewer slice-1 review): Blockers (1-4): * The documented `pip install -r requirements.txt` failed — no top-level requirements.txt exists. Switch SKILL.md, plugin.json's `egg.install_instructions`, and `bin/preflight.py` to `pip install .` against pyproject.toml. * SKILL.md claimed Python 3.11+ but pyproject.toml requires >=3.14. Update SKILL.md and preflight to reflect the real floor; correct the matching docstring in orchestrator/substrate/__init__.py. * The inline `python3 -c "..."` answer-write snippet in SKILL.md was broken (shell-interpolated `${ANSWER}` → NameError, deprecated `datetime.utcnow`, non-atomic write). Replace with a dedicated `bin/write_answer.py` helper that reads the JSON-encoded answer from stdin, uses `datetime.now(UTC)`, and writes atomically via tmp + `os.replace`. SKILL.md's loop now invokes the helper. * `_serialise_decision` silently swallowed `model_dump` failures, and `_read_contract` silently overwrote unparseable contracts (dropping `answer_log`). Log to stderr on serialisation fallbacks; refuse to overwrite a corrupted contract and exit 1 instead. Non-blocking (5-11): * `orchestrator/substrate/__init__.py` docstring rewritten to reflect reality: Python 3.14+ introduced PEP 758 (`except A, B:` without parens); ruff under py314 target strips redundant parens, hence `# fmt: skip` on multi-except lines. * Stderr warning in `_advance_generator`'s `finally` so a teardown failure inside `generator.close()` is at least observable. * `allowed-tools` tightened from `Bash(python3 *:*)` to `Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*)` — the two helper scripts are the entire Python surface the skill can invoke. * Re-spawn cost note expanded in SKILL.md: each driver invocation re-runs every prior subagent spawn (real Anthropic API spend); slice-2 compounds to 8 spawns at the final stage. * `_RUBRIC_LANDED_ROLES` registry collapsed into `_CURRENT_LOADER_SLICE` + filesystem probe; same diagnostics, no parallel registry that can drift from disk state. * Abort vocabulary exported as `ABORT_ANSWERS` from `orchestrator.substrate.in_process`; the driver imports it (with a literal fallback when the orchestrator package is unimportable) so the driver, orchestrator, and slice-3 daemon share a single source of truth. * R2 caveat surfaced as an explicit open question for slice-5 sequencing in SKILL.md. Tests: * New `shared/tests/test_write_answer.py` (6 tests, all passing) pins the JSON-encoding round-trip, the timestamp format match against the driver, the atomic-write contract, and the corrupted-contract refusal. * Existing `test_rubric_loader`, `test_run_pipeline_in_process*`, `test_substrate_interfaces`, `test_bridge_flattened_round_trip`, and `test_pretooluse_hook_nested` all still pass. * The pre-existing `test_empty_diff_subprocess_skips_pytest` failure in `tests/tools/` reproduces against HEAD without these changes (detached-HEAD worktree edge case in the test selector). Authored-by: egg * slice-2 coder v4: support rubric-default single-verdict JSON schema (#2717) Addresses reviewer_code_holistic v3 NACK blocker H3 — the rubric the documenter shipped (plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_plan.md "Verdict JSON shape", lines 57-80) documents a single top-level verdict object (verdict ∈ {ACK, NACK}, analysis carrying the eight criteria, feedback blob, artifact_references), not the per_producer wrapper v2/v3's parser expected. A rubric-following reviewer's NACK would silently fall into the "verdict file present but no parseable per_producer entries" branch and the orchestrator's optimistic-ACK fallback would mask the NACK from the operator at the plan-HITL gate. v4 makes `read_plan_reviewer_verdicts` accept BOTH schemas: 1. Rubric-default single-verdict (broadcast). When the JSON's top-level `verdict` is "ACK" or "NACK", the verdict is broadcast to every plan producer edge — ACK acks all three, NACK nacks all three with `feedback` propagated as the per-edge `reason` (a synthetic placeholder fires if `feedback` is empty so the tracker's NACK guard doesn't reject the payload). This is "Option (c)" from the v3 NACK; per-edge granularity is lost but the rubric's "ACK only if every criterion passes" semantic IS preserved. 2. Per-producer extension (per-edge). The existing per_producer wrapper still takes precedence when present and well-formed. Reviewers that want explicit edge granularity (ACK architect + NACK task_planner) write the wrapper; the rubric's default shape stays broadcast-compatible. The function now takes an optional `plan_producers` kwarg so the caller (the in-process orchestrator) can broadcast the single verdict to the right role set. The `_read_plan_reviewer_verdicts` class method delegate also propagates the kwarg so tester-side tests that call the method retain their access pattern. Smoke (manual, in-process, MagicMock subagents): - Rubric-default single-verdict NACK: tracker NACKs architect + task_planner (critical edges), risk_analyst still confirms (advisory), reviewer_plan blocks consensus. is_complete=False; blocking_agents=['architect', 'task_planner', 'reviewer_plan']. - Rubric-default single-verdict ACK: every edge confirmed; is_complete=True. - per_producer wrapper still works: mixed ACK/NACK applied per edge. - Harness-fake path (no verdict file, reviewer exit 0): optimistic ACK preserved so tester's existing 16 passing tests keep their access pattern. - Fail-closed path (no verdict file, reviewer exit non-zero): critical edges NACK'd (unchanged from v2/v3). ruff format + ruff check + file-size lint all pass. `_plan_phase.py` is 747 lines; `in_process.py` 1095 lines. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-2 (#2548) * Move skill-loop python3 -c calls into bin/ helpers Address review feedback on PR #2724: - Add bin/read_status.py and extend write_answer.py with --answer-string so every subcommand in SKILL.md's loop body is a single `python3 plugins/.../bin/<helper>.py` invocation. Honors the tightened allowed-tools pattern `Bash(python3 plugins/.../bin/*:*)` per Claude Code's compound-command permission rules — no separate `Bash(python3 -c *)` or `Bash(printf *)` rule needed, no prompt-injection door left open. - Update SKILL.md step 4 to name bin/write_answer.py directly (matches the new loop body). - Replace `slice_hint != _CURRENT_LOADER_SLICE` rubric-loader fence with `slice_hint not in _LANDED_SLICES` (frozenset) so future slices extend rather than replace the landed set — slice-2 won't fence off slice-1's already-shipped refine roles. - Wire test_bridge_flattened_round_trip's _write_answer through subprocess(write_answer.py) so the integration test exercises the production write path end-to-end. - Add test_read_status.py (7 tests) and --answer-string coverage in test_write_answer.py (2 tests). * Update slice-1 rubric loader tests to match slice-2's loader expansion Slice-1's recent tester commits (831239d / 601df90) added tests pinning 'architect raises ValueError' and 'reviewer_plan/task_planner deferred'. Slice-2's loader extension to the plan team (task-2-2 + task-2-3) makes those roles loadable, so the slice-1 tests fail after the merge. This commit aligns the tests with slice-2's loader reality: - Replace test_load_architect_raises_value_error_with_slice2_hint with test_load_architect_rubric, mirroring the slice-1 success-path tests. - Remove REVIEWER_PLAN and TASK_PLANNER from test_loader_still_rejects_unshipped_roles parameters; keep REVIEWER_CODE (slice-3, still deferred). - Refresh module docstring to reflect architect-loads (vs architect-raises). * Address non-blocking review notes on PR #2724 - write_answer.py: clarify --answer-string docstring — the JSON encoding happens at contract serialisation time (json.dumps(contract)), not as a separate json.dumps(answer) step. Reference the special-characters test as the proof of the round-trip. - SKILL.md / read_status.py: document the case statement's intentional fall-through on empty STATUS. read_status.py prints empty + exit 0 when no pending_hitl envelope exists; the case has no *) arm, so the empty value falls through, the case exits 0, and the outer iteration re-invokes run_pipeline.py — which is the recover path. - test_rubric_loader.py: add test_landed_slices_contains_slice1 to mechanically pin the 'extend, don't replace' invariant on _LANDED_SLICES so a future slice cannot silently regress slice-1 by writing frozenset({'slice-2'}) instead of frozenset({'slice-1', 'slice-2'}). * Address slice-2 review: phase plumbing, doc drift, defensive checks Addresses reviewer_code feedback on PR #2726 (#2717 slice-2): B1 (blocking): thread `phase` through `_write_pending_decision` and `current_phase` so plan-gate decisions persist with `phase: "plan"` instead of the hardcoded `"refine"` left over from the spike. Regression test pins the persisted-vs-yielded phase invariant. B2 (blocking) + N1 + N2 + N3 (SKILL.md doc drift): - Replace plain `approve` with the canonical `approve_continue` so operators following the docs trip the fence instead of silently completing. - Trim overclaim that slice-2 implements `request_changes` / `change_approach` re-spawn loops (it doesn't — they're surfaced but treated as stop). - Document the failure-path `retry` / `abort` option set. - Update the NotImplementedError quote to match the actual raise. N4: delete dead `_SYNTHETIC_PLAN_COMMIT` (no callers — real producers route through `synthetic_commit_for(role)`); fold the "never escape this constant" caveat into `synthetic_commit_for`'s docstring. N5: document the `per_producer` extension shape in `reviewer_plan.md` so reviewers who need per-edge granularity have the documented opt-in instead of guessing. N6: drop unused `pre_merge_condition` plumbing from the plan-phase verdict reader — pre-merge conditions are a PR-merge concept with no consumer in plan-phase. N7 + N8: unlink `<contract>.lock` after the critical section and bound `fcntl.flock` with `LOCK_EX | LOCK_NB` + a 30 s retry deadline so crashed lock-holders surface as `BlockingIOError` instead of hanging the orchestrator forever. N9: defensive `architect_output_path.is_file()` check before the downstream fan-out; surface the broken handoff as a NACK on the architect edge so the operator sees the partial state at the plan-HITL gate instead of debugging chained downstream errors. N10: clear the active-role sentinel at the start of the plan phase so the PreToolUse hook's fallback path doesn't resolve concurrent plan-producers to the stale `refiner` role. N11: drop the misleading `patch.object(restrictions, "get_agent_role", ...)` in `test_unknown_role_raises_handler_error` — `check_file_restriction` short-circuits on the truthy `req["role"]` so the patch never fired; the test still pins the real invariant without the misleading scaffolding. * Address slice-2 v2 review: NB1-NB4 (N9 fail-fast, flock pattern) --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org>
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No agent-mode design concerns.
Spot-checked the agent-design-relevant surfaces:
- Spawn prompts (
orchestrator/substrate/_plan_phase.py:606-615,spawn_plan_reviewer) carry only orientation (pipeline ID, role, artifact paths). Subagents fetch input via their own tools — no pre-fetched diffs or file contents baked into prompts. - HITL goes through
AskUserQuestion, the appropriate human-facing surface. - JSON output schemas in the role rubrics (architect, task_planner, risk_analyst, reviewer_plan, reviewer_refine, reviewer_agent_design) are inter-agent contracts consumed by the BRC orchestrator and downstream agents, not human-facing output — appropriate use of structured output per guideline #2.
- The
pending_hitlenvelope is process-coordination state written by the orchestrator generator and read by the next driver invocation; the LLM only rendersdecisionviaAskUserQuestionand persists the operator'sanswer. This is not the JSON-output-pipeline anti-pattern (no LLM output parsing involved). - File-write enforcement uses the PreToolUse hook calling
build_agent_patterns— infrastructure layer, same source of truth as the gateway. - No direct API calls,
httpx/requests/Anthropic-SDK imports, or hardcoded model identifiers in the substrate / plugin code (orchestrator/substrate/,plugins/egg-sdlc/). - The new
reviewer_agent_designrubric is itself a structural enforcement of agent-mode discipline — exactly the right layer for that check.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Contract Verification — PR #2731 (post-merge advisory)
This PR was already merged at 2026-05-20T01:12:58Z (merge commit 82c4ba4), so this review is advisory. Posting as a comment, not request-changes, because the integration branch landed.
Scope landed
work carries:
- refine + plan phase artifacts (analyses, BRC history, contract init, HITL statefiles) from the orchestrator runs
- slice-1 (#2724) — refine-team rubrics, flattened bridge driver, R2 spike
- slice-2 (#2726) — plan-team rubrics + plan-phase BRC
Slices 3–5 are explicitly out of scope and not present, matching the PR description.
Per-task verification (.egg-state/contracts/issue-2717.json)
I cannot call egg-contract verify-criterion --criterion ac-N because (a) the orchestrator is unreachable from this review session and (b) the contract has no top-level acceptance_criteria array — its acceptance_criteria is [] and criteria are embedded per-task as a string field on each slices[].tasks[]. The verify-criterion CLI is built for the older top-level schema. I verified each task's embedded criteria by reading the files instead.
Slice 1 — refine-team + flattened bridge + R2 spike
| Task | Status | Evidence |
|---|---|---|
1-1 bin/run_pipeline.py driver |
✅ Met | Module exists; lazily imports run_pipeline_in_process from orchestrator.substrate.in_process (line 286); pending_hitl envelope schema documented at top with explicit stability promise; exit codes 0/1 per spec |
1-2 SKILL.md flattened-bridge usage |
✅ Met | "Walking-skeleton bridge gap" callout replaced with "How the flattened bridge works" subsection; allowed-tools carries AskUserQuestion, Read, Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*) |
1-3 test_bridge_flattened_round_trip.py |
✅ Met | 2 test functions including test_bridge_flattened_round_trip round-tripping a HITLDecision across two driver invocations |
| 1-4 reviewer_refine + reviewer_agent_design rubrics | ✅ Met | Both files exist with name: / description: frontmatter; both bodies open "You are the reviewer_refine/_agent_design running on the Claude Code substrate"; rubric criteria preserved from k3s counterparts |
1-5 test_pretooluse_hook_nested.py |
✅ Met | Test writes .egg-state/<pipeline_id>/r2-verdict.json with r2_verdict: pass|fail; uses the TASK-1-9 fake; documents the empirical-vs-test-fake limitation in the module docstring |
1-9 _agent_tool_fake.py |
✅ Met | Exposes dispatch(parent_role, child_role, write_target); underscored helper-style name keeps it outside the test_*.py glob (coder-owned); top-of-file if not __name__.startswith("integration_tests") import guard at line 91 |
1-6 substrate loader (orchestrator/substrate/__init__.py) |
✅ Met | reviewer_refine + reviewer_agent_design mapped to slice-1 in _LANDED_SLICES; ARCHITECT/plan roles still raise but with updated slice-2 / slice-3 diagnostic instead of "follow-up issue per cq-11" |
1-7 test_rubric_loader.py |
✅ Met | Contains 8 test functions (well over the required 4) covering refiner regression, reviewer_refine, reviewer_agent_design, and ARCHITECT ValueError — plus enum/string-input parametrization |
1-8 ADR (docs/architecture/claude-code-substrate.md) |
✅ Met | "Rollout deltas" reorganized into "Completed in this rollout" subsection with strikethrough for the closed bridge gap; cq-7 row updated to reference flattened driver + R17 envelope; refine-team scope reflected; R2 fallback path documented |
Slice 2 — plan-phase substrate (3 producers + 1 reviewer)
| Task | Status | Evidence |
|---|---|---|
2-1 in_process.py plan-phase BRC |
✅ Met | _run_plan_phase method added at orchestrator/substrate/in_process.py:901, delegates to orchestrator/substrate/_plan_phase.py:run_plan_phase; spawns architect synchronously first, fans out task_planner + risk_analyst, runs BRC against reviewer_plan; plan HITL gate at line 724 yields after CONSENSUS_CONFIRMED on every producer edge |
2-2 substrate __init__.py plan roles |
✅ Met | architect, task_planner, risk_analyst, reviewer_plan all mapped to slice-2 in role table (_LANDED_SLICES now {slice-1, slice-2}); implement-phase roles still fenced |
| 2-3 plan-team rubric files | ✅ Met | All four files exist (architect.md, task_planner.md, risk_analyst.md, reviewer_plan.md) with valid frontmatter; bodies open with "You are the running on the Claude Code substrate"; plan-phase rubrics + handoff JSON shapes preserved from k3s |
2-4 test_inprocess_plan_brc.py |
✅ Met | 9 test functions including test_plan_stage_spawns_three_producers_and_one_reviewer, test_plan_stage_reaches_consensus_confirmed_for_each_producer, plan-gate yield, refine-rejection short-circuit, env-var threading, and architect-handoff fail-fast |
2-5 restrictions.py validator |
✅ Met | check_file_restriction(req) returns gateway-shape response; report_impasse companion for blocked path discovery |
2-6 test_restrictions_validator.py |
✅ Met | Test file present with 8+ assertions covering coder/tester/documenter in-allow-list shape, denial shape stability, no new validator symbol introduced (R2-pass guard), and HandlerError on missing path |
2-7 SKILL.md plan-phase additions |
✅ Met | Rollout status banner updated to "slices 1 + 2 of #2717 landed"; plan-phase steps (8, 9) describe architect-first dispatch and CONSENSUS_CONFIRMED gate; plan-gate role table at line 269 lists all four plan roles with output paths |
Contract drift — blocking observation
The contract on disk does not reflect any of the work that landed in this PR:
current_phase: "refine"— but slice-1 (the entire refine team) and slice-2 (the entire plan phase) have shipped. This should have advanced to at leastplanorimplement.- Every task across every slice is
status: "pending"withcommit: null— including the 16 tasks (1-1 through 1-9, 2-1 through 2-7) whose deliverables I verified above as fully implemented. - Slice statuses are all
"pending"— slice-1 and slice-2 should becomplete. - No top-level
acceptance_criteria[]entries — task-level criteria are present (and are met) but the contract was never structured forverify-criterion --criterion ac-N. - No
phases[]array at all on the contract.
Net effect: anyone reading .egg-state/contracts/issue-2717.json after the merge would believe none of this work has been started, even though slice-1 + slice-2 are in main. The PR description acknowledges the PR "was opened late — it should have been created when the plan phase completed (tracked in #2729)"; the contract bookkeeping appears to have been skipped alongside.
Suggested follow-up (do not block on this PR — it's merged):
- On the next pipeline turn, complete tasks 1-1 → 1-9 and 2-1 → 2-7 with their landed commits (
33c6617for slice-1,afa3455for slice-2 — or the per-task commits inside those merges if recoverable). - Advance
current_phasepastrefine. - Mark
slices[0]andslices[1]status: "complete". - Decide whether top-level
acceptance_criteria[]is meant to be populated for this contract shape — ifverify-criterionis expected to work on slice-DAG contracts, a follow-up issue against the contract schema is warranted.
Verdict
Approve-equivalent (advisory comment, post-merge). Every shipped task's embedded acceptance criteria are met on disk. The only blocking issue is contract bookkeeping drift, which is recoverable post-merge and called out in the PR description (#2729). Slices 3–5 remain to land; this PR's stated scope (refine + plan + slices 1–2) is fully delivered.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
|
egg review completed. View run logs 2 previous review(s) hidden. |
jwbron
added a commit
that referenced
this pull request
May 21, 2026
* Revert Claude Code substrate Abandons the Claude Code substrate initiative. Reverts the three commits that landed substrate code/docs on main: - 802f77d (#2715) walking-skeleton spike - 173f483 (#2718) substrate docs in index/structure docs - 82c4ba4 (#2731) substrate-swap rollout integration (slices 1-2) Removes orchestrator/substrate/, plugins/egg-sdlc/, the claude-code-substrate ADR, associated tests, and #2717/#2623 .egg-state pipeline artifacts. * Parenthesize except tuples in lifecycle_secret Restore the (OSError, subprocess.TimeoutExpired) and (ValueError, UnicodeDecodeError) paren-wraps the substrate PR had added. PEP 758 makes the bare form valid on Python 3.14, but it reads as a Python 2 SyntaxError to most reviewers, which is the kind of nit that would otherwise be flagged later. * Fix checks: apply automated formatting fixes --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@localhost>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Integration PR for the #2717 substrate-swap rollout. The rollout ships as a stack of per-slice PRs that merge up into
egg/issue-2717/work; this PR landsworkonmain.Opened late — it should have been created when the plan phase completed (tracked in #2729).
workcurrently carries the refine + plan phase artifacts only:Slice stack
Slice PRs merge into
workbefore this PR is mergeable:work)slice-1; GitHub auto-retargets toworkwhen [slice-1] Roll out Claude Code substrate to remaining roles + plan/... #2724 merges)Kept as a draft until every slice has merged up and #2717's definition-of-done is verified on
work.Test plan
workworkworkRefs #2717, #2729