[slice-1] Roll out Claude Code substrate to remaining roles + plan/...

[james-in-a-box]

Issue #2715 shipped the walking-skeleton spike for the Claude
Code substrate (one role × one phase end-to-end). This issue
rolls the substrate out from there per cq-11 = "Spike then
plan".

What this PR stack does (5 slices, stacked PRs):

1. Bridge gap + R2 spike + refine reviewers (slice 1) —
   Closes the heredoc-HITL bridge gap via flattened
   python3 <stage>.py invocations (cq-1 Option C, refine/plan
   half). Ships a 2-subagent worked example that validates
   PreToolUse hooks resolve role under nested dispatch (cq-5).
   Adds the two refine-team reviewer rubrics (reviewer_refine,
   reviewer_agent_design).
2. Plan-phase substrate (slice 2) — First multi-role BRC
   stress test. Wires architect / task_planner / risk_analyst
   producers and reviewer_plan through the in-process
   orchestrator.
3. Implement-phase substrate + daemon bridge (slice 3) —
   Second BRC stress test at largest scale (3 producers + 5
   reviewers). Switches the HITL bridge to Option A (long-lived
   daemon over UNIX socket) because the flattened path is
   impractical at this many yields.
4. PR-phase + conformance matrix (slice 4) — Wires the PR
   phase, removes the walking-skeleton fence (feedback Q6),
   ships the 5-issue conformance matrix (Sync regression: _sync_worktree_with_remote rebase fallback fails on dirty worktree (follow-up to #2337) #2714, Expand integration test coverage #2474, Decompose 15 oversize Python source files to clear the file-size allowlist #2261,
   Fix fresh-cluster local k3s bring-up: Cilium datapath + namespace ordering #2705, docs: add claude-code substrate to index and structure docs [doc-updater] #2718) under both substrates with pytest.mark.slow
   gating + 3-hour per-phase ceiling.
5. Hardening (slice 5) — EGG_PIPELINE_MAX_AGENT_INVOCATIONS
   cost cap default 200 (cq-6); EggHarnessSpawner + egg-orch local-run CLI (DoD Phase 3: Container extraction #5 / feedback Q3); fork-based delegation
   (cq-10 deferred half); contingent R15 model (b) migration
   based on slice 1's R2 verdict (cq-4); drop the v0.x unstable
   marker on the four substrate protocols (cq-7); ADR refresh.

Impact: every agent role in egg's SDLC pipeline gains a
second substrate; the operator can drive a full pipeline
natively in Claude Code via AskUserQuestion without an MCP
provide_input round-trip; the conformance matrix is green on
both substrate dimensions; egg-orch local-run enables headless
runs; the four substrate protocols are stable.

This slice

Bridge gap (flattened) + R2 hook validation + refine reviewers

Tasks:

• task-1-1: Add bin/run_pipeline.py stage driver under plugins/egg-sdlc/skills/egg-sdlc/bin/. The driver loads pipeline state from .egg-state/contracts/<id>.json, calls run_pipeline_in_process(...) and advances the generator to its next yield via generator.send(answer) (where answer is the operat...
• task-1-2: Update SKILL.md to call the new bin/run_pipeline.py driver in a loop: invoke the driver, read pending_hitl.decision, render via AskUserQuestion, write the operator's answer to pending_hitl.answer, loop. Replace the "Walking-skeleton bridge gap" callout (line 97-102) with a brief "How th...
• task-1-3: Add test_bridge_flattened_round_trip.py under integration_tests/regression/. Test invokes bin/run_pipeline.py twice against a deterministic pipeline id: first invocation produces a preflight HITLDecision; test writes the answer to pending_hitl.answer; second invocation produces a refine...
• task-1-4: Add reviewer_refine.md and reviewer_agent_design.md role rubric files under plugins/egg-sdlc/skills/egg-sdlc/agents/. Mirror the shape of the existing refiner.md (frontmatter + markdown body). Pull the rubric content from the corresponding k3s prompt sources under shared/prompts/ so the...
• task-1-5: Add test_pretooluse_hook_nested.py under integration_tests/regression/. **The R2 question ("does the hook resolve role under nested dispatch?") can only be answered when subagents run via Claude Code's Agent-tool dispatch — the harness re-host model (ClaudeCodeSpawner per cq-3) bypasses the...
• task-1-9: Add a minimal test-only nested-Agent-tool dispatch fake under integration_tests/regression/_agent_tool_fake.py. Simulates Claude Code's Agent tool by spawning a subprocess with controlled EGG_AGENT_ROLE env var per dispatch; each fake-subagent has a pre_tool_use_callback that invokes `orc...
• task-1-6: Update _load_egg_sdlc_role_rubric in orchestrator/substrate/__init__.py:232 to remove the "spike only ships refiner rubric" ValueError (lines 280-284) when the role is reviewer_refine or reviewer_agent_design. The loader continues to raise ValueError for plan/implement roles until sli...
• task-1-7: Add unit tests for the updated rubric loader: assert that the two refine reviewer roles load successfully, and that plan/implement roles still raise ValueError with the updated diagnostic message. Test file lives under shared/tests/ next to the existing test_substrate_interfaces.py and `tes...
• task-1-8: Update the ADR at docs/architecture/claude-code-substrate.md to reflect: (a) the flattened bridge mechanism (replacing the "Walking-skeleton bridge gap" callout); (b) the R2 verdict (point to r2-verdict.json and TASK-1-5); (c) the refine-team expansion (refiner + 2 reviewers now on the substr...

Test Plan

• Automated: every slice ships substrate-portable tests under
  integration_tests/regression/ (per slice: bridge round-trip,
  R2 nested-hook denial, plan-phase BRC, implement-phase BRC,
  daemon round-trip, conformance matrix x5 issues x2 substrates,
  cost cap, EggHarnessSpawner, fork primitive).
• Manual (slice 4 / slice 5): operator runs
  python3 bin/run_pipeline.py issue-2717 end-to-end and
  egg-orch local-run --issue 2718 headlessly; both produce the
  expected PR stack and metrics.

Manual Steps

Pre-merge (each slice): reviewer copies
orchestrator/substrate/claude_code/settings.template.json into
their own .claude/settings.json before exercising the
conformance matrix; reviewer spot-checks BRC consensus history
against the plan's primitives table.

Post-merge (slice 5): if slice 3's empirical metrics warrant the
Agent-tool dispatcher migration (cq-3), reviewer files a
follow-up issue; if R15 model (b) migration shipped (R2 fail
path), reviewer files a follow-up to deprecate model (a) after
operator migration.

Slice slice-1 of pipeline issue-2717. Stacked on top of egg/issue-2717/work.

[egg-reviewer]

No agent-mode design concerns.

Quick notes from the agent-design lens (all OK, not requesting changes):

• Structured JSON verdicts in reviewer_refine.md / reviewer_agent_design.md (exact analysis keys, verdict_path artifact) are justified — the orchestrator's BRC consensus aggregator is a real machine consumer that needs to parse ACK/NACK across reviewers. This is the legitimate case from guideline 2.
• Pre-fetching is appropriately bounded: run_pipeline.py threads only lightweight orientation (pipeline_id, repo, issue_number, issue_body) into the generator. Subagents open files via their own tool surface; no diffs / file contents baked into prompts.
• File-write enforcement is structural via the PreToolUse hook calling the same build_agent_patterns source-of-truth as the gateway (shared/egg_restrictions/patterns.py:768), not prompt-level "don't write outside your role" instructions. The reviewer rubrics' substrate-specific notes correctly point at this.
• Trust-context shift is acknowledged honestly in SKILL.md's "Trust-context note" — the credential boundary moves vs. k3s, and operators wanting gateway-isolated credentials are pointed back at EGG_SUBSTRATE=k3s. Not papered over with prompt rules.
• Bridge complexity (flattened replay loop, pending_hitl envelope) lives in the driver, not in agent instructions. Subagents see the same task shape as on k3s; the substrate swap is intentionally invisible to the role rubric.
• No direct Anthropic API calls, no claude --print, no pinned model identifiers in the diff.

The reviewer rubrics mirroring the k3s-substrate shape (so verdicts are substrate-portable) is the right call — keeps BRC aggregation working uniformly across substrates and avoids a parallel mechanism (good-pattern: composition over duplication from agent-mode-design.md).

— Authored by egg

[egg-reviewer]

PR #2724 — Slice-1 substrate rollout review

I read every PR-modified source file end-to-end, traced the bridge-driver data flow, and ran the install instructions against the actual repo to verify functionality. Findings below, ordered by severity.

Blocking

1. The documented install path does not work — requirements.txt does not exist at the repo root

plugins/egg-sdlc/.claude-plugin/plugin.json (egg.install_instructions), plugins/egg-sdlc/skills/egg-sdlc/SKILL.md:27-32, and plugins/egg-sdlc/skills/egg-sdlc/bin/preflight.py:79-83 all instruct the operator to run:

git clone https://github.com/jwbron/egg.git && cd egg && pip install -r requirements.txt && export PYTHONPATH="$PWD:$PWD/shared:$PYTHONPATH"

There is no requirements.txt at the repo root. The repo uses pyproject.toml (project name egg, requires-python = ">=3.14") and the canonical install path documented in the root CLAUDE.md is make deps (which uses uv). The only requirements.txt that exists is orchestrator/requirements.txt — a transitive deps file, not a project install file.

A new operator who copies the documented command will hit:

ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'requirements.txt'

bin/preflight.py will then re-print the same broken instruction. The "single source of truth" claim (SKILL.md line 34: "the install-error message in the pre-flight helper reads from the same plugin.json field this section documents") is true — but they're all consistently wrong. The PR explicitly touches the install surface (SKILL.md install section reflows, preflight rationale rewritten in this PR's docstring) and the slice-1 contract task-1-7 names install-error alignment as an acceptance criterion, so this is in-scope here.

Fix options: either ship a real top-level requirements.txt, or update the three surfaces to pip install . (against pyproject.toml) — and verify it works against requires-python = ">=3.14".

2. Python version mismatch — SKILL.md claims 3.11+ but the project requires 3.14+

SKILL.md:36: "Egg targets Python 3.11+. If your Claude Code session resolves to an older Python, the import will fail with a version error — re-run the install command in a 3.11+ venv."

pyproject.toml: requires-python = ">=3.14".

A user on 3.11, 3.12, or 3.13 follows the install instructions, the pip install either fails on requires-python or installs but then import orchestrator.substrate.in_process works against module-level syntax only available on 3.14. The skill is non-functional on the claimed-supported versions.

The new docstring in orchestrator/substrate/__init__.py:13-32 openly acknowledges the divergence ("This package targets Python 3.14 ... but the SKILL.md / packaging documentation states 'Python 3.11+'") but instead of fixing it, adds a defensive coding rule (mandatory # fmt: skip on multi-except tuples) to "keep both surfaces working." That defense is itself suspect (see issue 4 below).

Pick one: either bump SKILL.md / plugin.json to 3.14+, or relax requires-python to 3.11.

3. Skill loop's inline answer-write snippet is broken as written

SKILL.md:148-157 documents the load-bearing skill-side write that promotes the operator's answer into the contract. As written, the snippet is non-functional:

python3 -c "
import json, sys, datetime
path = '${CONTRACT_PATH}'
contract = json.load(open(path))
env = contract['pending_hitl']
env['answer'] = ${ANSWER}        # operator's selection; JSON-encode appropriately
env['status'] = 'answered'
env['timestamp'] = datetime.datetime.utcnow().isoformat() + 'Z'
json.dump(contract, open(path, 'w'), indent=2)
"

Concrete issues:

• ${ANSWER} is shell-interpolated directly into the Python source. If ${ANSWER} is the bare string approve (which is exactly what AskUserQuestion returns for an option label), the result is env['answer'] = approve → NameError: name 'approve' is not defined. The comment hand-waves "JSON-encode appropriately" but the snippet does not show how. The skill body has no shell helper to JSON-encode the answer.
• datetime.datetime.utcnow() is deprecated in Python 3.12+ (DeprecationWarning: datetime.datetime.utcnow() is deprecated) and the project requires 3.14+. The driver's own _now_iso() uses datetime.now(UTC).isoformat() — the example contradicts the production source.
• Timestamp format inconsistency. The example appends + 'Z'; run_pipeline.py:101-103 does not. datetime.now(UTC).isoformat() already includes +00:00. With this example, pending_hitl.timestamp alternates between ...+00:00 (driver writes) and ...+00:00Z (skill writes) — which is now mis-formatted ISO-8601.
• Non-atomic write. json.dump(contract, open(path, 'w'), indent=2) truncates the file before writing. A Ctrl+C or skill-shell crash mid-write corrupts the contract — the driver's _read_contract then silently falls back to a fresh-skeleton default (run_pipeline.py:119-136), dropping the operator's accumulated answer_log with no diagnostic. The driver uses tmp + os.replace for exactly this reason; the skill side must match.

Either ship a bin/write_answer.py helper (the SKILL.md alternate-path mentions this — adopt it instead of an inline -c) or rewrite the snippet to (a) JSON-encode ${ANSWER} via printf '%s' "$ANSWER" | python3 -c ..., (b) use datetime.now(UTC).isoformat() without Z, (c) tmp + os.replace. As shipped, the documented loop will not run.

4. _serialise_decision silent fallback can produce a non-renderable envelope

plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:150-173:

def _serialise_decision(decision: Any) -> dict[str, Any] | None:
    ...
    model_dump = getattr(decision, "model_dump", None)
    if callable(model_dump):
        try:
            dumped = model_dump(mode="json")
            ...
        except (TypeError, ValueError):
            pass
    raw = getattr(decision, "__dict__", None)
    if isinstance(raw, dict):
        return {k: v for k, v in raw.items() if not k.startswith("_")}
    return {"repr": repr(decision)}

If HITLDecision.model_dump(mode="json") raises (e.g., a non-JSON-serializable field added later, an enum without a JSON encoder, a circular reference), the exception is silently swallowed and the driver falls through to __dict__ or to {"repr": repr(decision)}. The skill body then sees pending_hitl.decision = {"repr": "<HITLDecision ...>"} and tries to render it via AskUserQuestion — there are no question, options, or context fields. The skill loop wedges or surfaces a meaningless prompt with no diagnostic of where it went wrong.

At minimum: log a structured warning to stderr in the fallback paths so the operator sees "model_dump failed: …; falling back to dict" instead of a silently malformed envelope. A clean fix is to re-raise — HITLDecision is the only expected input and its model_dump is supposed to work; if it doesn't, the operator wants to know, not get a smiling "repr" envelope.

The same pattern in _read_contract (line 129-132) silently swallows JSONDecodeError and returns a fresh skeleton, dropping answer_log. That is a real data-loss bug if the contract file is mid-write or partially truncated — the operator gets re-asked the preflight with no signal. The same fix applies: log it loudly, and ideally exit with status="error" rather than discarding state.

Non-blocking

5. The # fmt: skip defense in orchestrator/substrate/__init__.py:13-32 is cargo-cult

The new docstring says ruff format "silently strips the redundant parens back to except A, B: which is a SyntaxError on Python 3.10/3.11/3.12/3.13." This claim does not match observed ruff behavior — except (A, B): is the only valid Python 3 syntax for catching multiple exceptions, and ruff format preserves it. (except A, B: was the Python 2 binding form and is a SyntaxError in every Python 3.) If ruff actually stripped the parens, it would be a known ruff bug that ought to be filed against ruff, not defended via # fmt: skip everywhere.

You can verify locally: echo 'try: pass\nexcept (ValueError, TypeError): pass\n' | ruff format - keeps the parens.

The actual risk this is defending against — pyproject.toml's requires-python = ">=3.14" vs. the SKILL.md "3.11+" claim — is the version mismatch from issue 2 above. Fix the version mismatch and the defense becomes unnecessary.

6. Re-spawn cost on every driver invocation is acknowledged in docs but worth flagging for slice-2/3 sizing

By design (run_pipeline.py:268-327, ADR "flattened bridge" section), each bin/run_pipeline.py invocation:

1. Constructs a fresh run_pipeline_in_process(...) generator.
2. Calls next() to land on the preflight yield.
3. Loops generator.send(answer) over answer_log to replay.
4. Each send past the preflight yield triggers a real _spawn_refiner() (and slice-2 will add architect / task_planner / risk_analyst spawns).

For slice 1's 2-yield refine phase this means the refiner + both reviewers spawn on every invocation past stage A — once when the operator first sees the refine-gate, again when they answer it. The docs note "side effects re-run on every invocation" and "the artifact write overwrites" — both true, but each re-spawn is a real Anthropic API call costing tokens and 10-60s of wall-clock per subagent. With slice 2's 4-yield plan phase, the cost compounds: stage B = 2 spawns, stage C = 4 spawns, stage D = 6 spawns, stage E = 8 spawns. For a real jwbron/egg issue this is on the order of tens of dollars in Anthropic API spend per pipeline run before slice 3's daemon variant lands.

Slice 2's plan should size this and call it out explicitly in the user-facing skill (today SKILL.md only says "side effects re-run on every invocation" — that wording understates the cost).

7. _answer_is_abort is duplicated between the orchestrator and the driver

orchestrator/substrate/in_process.py:834-845 (_answer_is_abort) and plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:343-352 (_is_aborted_status) are near-identical implementations of the same logic. Either import one from the other, or move the abort-vocabulary set ({"abort", "stop", "cancel"}) into a shared constant. The driver's docstring already promises envelope-shape stability to the slice-3 daemon — abort-detection should be on the same source of truth list, otherwise slice 3 will silently disagree on what counts as abort.

8. _advance_generator discards generator state without a heartbeat write

run_pipeline.py:321-327:

finally:
    try:
        generator.close()
    except Exception:
        pass

generator.close() raises GeneratorExit inside the generator's finally block (which joins the background threads). Good. But if the generator's own finally raises during teardown (e.g., _teardown_worktrees hits an OSError), the exception is silently swallowed via the bare except. The operator sees a clean exit-0 envelope, but the worktree is leaked. The orchestrator's _teardown_worktrees already wraps in Exception-suppression, so this is double defense — fine — but if both layers swallow, there is no record at all that teardown failed.

Cheapest fix: print a single stderr line in the driver's except Exception here, just so the trail is visible to an operator who runs 2>>driver.log.

9. Skill allowed-tools is broad on Bash(python3 *:*)

SKILL.md:6 allows Bash(python3 *:*) which permits any python3 -c "..." with any code, including code from prompt-injected issue bodies if the skill ever templates issue content into a -c arg (it doesn't today, but the door is wide open). Tighten to Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*) plus a more constrained pattern for the inline answer-write — or replace the inline -c with a bin/write_answer.py helper (already reserved in the SKILL.md "alternative path" note at line 173).

10. _load_egg_sdlc_role_rubric's _RUBRIC_LANDED_ROLES vs. _ROLE_RUBRIC_SLICES is a redundancy bug-waiting-to-happen

orchestrator/substrate/__init__.py:244-277 declares two independent registries — _ROLE_RUBRIC_SLICES (every role's planned slice) and _RUBRIC_LANDED_ROLES (the subset whose rubric is on disk this slice). Slice 2 will add four roles to _RUBRIC_LANDED_ROLES; slice 3 will add eight. If the documenter lands a rubric file but forgets to add the role to _RUBRIC_LANDED_ROLES, the loader will still raise ValueError "deferred to follow-up slice-X" even though the file exists. Conversely, if a role is added to _RUBRIC_LANDED_ROLES but the documenter's file has not landed, the loader gives a different error ("missing on disk").

The simpler invariant is "does agents/<role>.md exist?" — there's no value in maintaining a hand-curated allowlist when filesystem existence is the source of truth. The slice-aware diagnostic can come from _ROLE_RUBRIC_SLICES alone. Recommend collapsing this to a single registry plus a filesystem probe.

The path-traversal defense is already covered by the _ROLE_RUBRIC_SLICES membership check, and _RUBRIC_LANDED_ROLES is a frozenset of three literals, so this is not a security finding — purely a maintainability concern.

11. Test infrastructure note: test_pretooluse_hook_nested.py validates only half of R2

The test's own docstring is honest about this: it pins the hook logic (given accurate env propagation) but cannot validate "does Claude Code itself set EGG_AGENT_ROLE correctly under nested dispatch?" — which is the actual R2 question slice-5 R15 migration is gated on. SKILL.md:228 mirrors the caveat. The verdict file r2-verdict.json should not be read by slice-5 as a green-light without the empirical Claude-Code-side test landing first. Worth surfacing as an open question for slice-5 sequencing.

What works well

• The 9-field pending_hitl envelope is well-shaped for the daemon hand-off in slice 3. The version field with future-version rejection is the right defensive call.
• _advance_generator's finally: generator.close() plus the orchestrator's _teardown_worktrees + _teardown_sentinel give a clean teardown path on every exit (including GeneratorExit). Test coverage matches.
• The role rubric loader's allowlist-first fence catches path-traversal role values before any filesystem touch (test_rubric_loader.py::test_loader_rejects_path_traversal_role_name). Good defense.
• _agent_tool_fake.py's import guard refusing production callers is a good belt-and-suspenders on the test-vs-prod boundary.
• Atomic contract write via tmp + os.replace in the driver matches the orchestrator's existing _write_pending_decision shape.

Recommendation

Request changes on items 1-4. Items 5-11 are non-blocking suggestions; happy to see them in a follow-up. The flattened bridge mechanism itself is sound — what's blocking is that the install path doesn't actually install, the version claim is wrong, and the skill-side answer-write snippet (the load-bearing piece of the documented loop) is broken as written. A new operator following SKILL.md verbatim cannot run the slice today.

— Authored by egg

[james-in-a-box]

Review disposition — commit a49dcfe

Thanks for the thorough end-to-end read. Per-item dispositions below; commit pushed at a49dcfe.

Blocking

1. requirements.txt does not exist — fixed-in-PR (commit a49dcfe). Switched all three surfaces (SKILL.md, plugin.json#egg.install_instructions, bin/preflight.py) to pip install . against pyproject.toml. The three surfaces remain consistent and a new operator following the instructions verbatim now installs successfully.

2. Python version mismatch (3.11+ vs 3.14+) — fixed-in-PR (commit a49dcfe). Bumped the Python-version claim to 3.14+ in SKILL.md and preflight.py. Updated the matching docstring in orchestrator/substrate/__init__.py to reflect that 3.14 is the floor (PEP 758 lets except A, B: parse without parens; the # fmt: skip defense is now correctly characterized as a portability/readability preference rather than a syntax-safety requirement).

3. Broken skill-loop answer-write snippet — fixed-in-PR (commit a49dcfe). Replaced the inline python3 -c "..." with a new helper plugins/egg-sdlc/skills/egg-sdlc/bin/write_answer.py:

   • Reads JSON-encoded answer from stdin (so shell quoting cannot mis-encode approve into a Python NameError).
   • Uses datetime.now(UTC).isoformat() — matches the driver's _now_iso, no trailing Z.
   • Writes atomically via tmp + os.replace (mirrors run_pipeline.py:_write_contract).
   • Refuses to overwrite a corrupted contract file rather than silently dropping answer_log.

   SKILL.md's loop now invokes the helper. A new shared/tests/test_write_answer.py (6 tests) pins the JSON-encoding round-trip, the timestamp format match against the driver, the atomic-write contract, and the corrupted-contract refusal.

4. _serialise_decision / _read_contract silent fallback — fixed-in-PR (commit a49dcfe). _serialise_decision now logs to stderr on each fallback path (model_dump exception, model_dump-returned-non-dict, no __dict__ available) so the operator sees why the envelope is malformed. _read_contract now raises RuntimeError on JSON-decode failures and non-dict payloads instead of silently returning the default skeleton; main() catches and persists nothing (preserves the corrupted file for hand-repair).

Non-blocking

5. # fmt: skip defense docstring is cargo-cult — disagree (with a correction). I verified empirically: printf 'try:\n pass\nexcept (ValueError, TypeError):\n pass\n' | ruff format --target-version py314 - does strip the parens to except A, B:. Python 3.14 added that syntax via PEP 758, which is exactly why ruff's py314 target strips the parens — the defense is real. However, your underlying point stands: once the SKILL.md / preflight Python floor is corrected to 3.14+, the stripped form is valid on every supported interpreter, and the # fmt: skip is a portability/readability preference rather than a syntax-safety requirement. Rewrote the docstring to reflect that reality (commit a49dcfe).

6. Re-spawn cost compounding — fixed-in-PR (commit a49dcfe). Expanded the SKILL.md "Generator state across invocations" section with an explicit "Cost note" enumerating the per-slice spawn fanout (slice-1: 2; slice-2 stage-E: 8) and naming Anthropic API spend / wall-clock cost. Also reiterates that the slice-3 daemon variant eliminates replay, and that the slice-5 EGG_PIPELINE_MAX_AGENT_INVOCATIONS cost cap does not apply to this substrate until then.

7. _answer_is_abort duplicated between orchestrator and driver — fixed-in-PR (commit a49dcfe). Exported ABORT_ANSWERS from orchestrator.substrate.in_process as a frozenset({"abort","stop","cancel"}) and added it to __all__. The driver's _is_aborted_status now imports it (with a literal fallback only on ImportError, where the driver's main() has already failed). All three surfaces (orchestrator, flattened driver, future slice-3 daemon) share one source of truth.

8. _advance_generator finally discards generator state without a heartbeat write — fixed-in-PR (commit a49dcfe). The except Exception block now prints a single stderr line naming the exception and suggesting ~/.egg-worktrees/ / EGG_WORKTREE_BASE to inspect for leaked checkouts. The driver still returns success because the primary work (advancing to the next yield) already succeeded.

9. allowed-tools is broad on Bash(python3 *:*) — fixed-in-PR (commit a49dcfe). Tightened to Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*). The two helpers (run_pipeline.py and write_answer.py) are now the entire Python surface the skill can invoke. A prompt-injected issue body cannot coerce the skill into running arbitrary python3 -c "..." snippets.

10. _RUBRIC_LANDED_ROLES vs _ROLE_RUBRIC_SLICES redundancy — fixed-in-PR (commit a49dcfe). Collapsed _RUBRIC_LANDED_ROLES into a single _CURRENT_LOADER_SLICE constant + filesystem Path.is_file() probe. Same diagnostics (path-traversal hits the not-in-rollout fence before any filesystem touch; same-slice missing-on-disk surfaces as "rubric missing on disk for slice-N"); the parallel registry that could drift is gone. All 14 test_rubric_loader tests still pass unchanged, including the path-traversal adversarial test.

11. test_pretooluse_hook_nested.py validates only half of R2 — fixed-in-PR (commit a49dcfe). Added an explicit "Open question for slice-5 sequencing" callout in SKILL.md's PreToolUse hook section naming r2-verdict.json as a necessary-but-not-sufficient input for the R15 model-(b) migration; the caveat is also already in the test's module docstring so the two surfaces agree.

Test results

• make lint — passes (ruff check + format, mypy via venv, shellcheck, yamllint, custom checks).
• shared/tests/test_rubric_loader.py — 14/14 pass.
• shared/tests/test_run_pipeline_in_process*.py — 22/22 pass.
• shared/tests/test_substrate_interfaces.py — 14/14 pass.
• shared/tests/test_write_answer.py (new) — 6/6 pass.
• integration_tests/regression/test_bridge_flattened_round_trip.py — 2/2 pass.
• integration_tests/regression/test_pretooluse_hook_nested.py — 5/5 pass.

The only make test failure I observed (tests/tools/test_select_tests_fallbacks.py::test_empty_diff_subprocess_skips_pytest) is pre-existing — it reproduces against HEAD without these changes (detached-HEAD worktree edge case in the test selector; stderr says select-tests: detached HEAD; using base branch baseline).

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta since 9f29ee8a.

Quick notes from the agent-design lens on the changes:

• allowed-tools tightening from Bash(python3 *:*) → Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*) is the right direction — scopes the skill's Python surface to the two reviewable helpers (run_pipeline.py, write_answer.py) and removes the "prompt-injected issue body coerces python3 -c "…"" door. This is sandbox-enforced (allowed-tools is harness-level), not a prompt-level "please don't" rule, so it matches guideline 5.
• New bin/write_answer.py helper preserves the flattened-bridge shape — same pending_hitl envelope, same atomic tmp + os.replace, same timestamp format as the driver. The mechanism stays in the driver layer, not in agent instructions; subagents see no change in their task shape. The ABORT_ANSWERS export from orchestrator.substrate.in_process consolidates the abort vocabulary across orchestrator, flattened driver, and the slice-3 daemon variant — single source of truth instead of three drifting copies.
• Loud-fallback fixes in _serialise_decision / _read_contract replace silent envelope corruption with structured stderr diagnostics. From an agent-design lens this is the right shape: when the bridge can't render a decision, the operator sees why rather than a silently-malformed AskUserQuestion prompt. Refusing to overwrite a corrupted contract (preserving answer_log) matches the spirit of "fail loudly so the operator can intervene" rather than "smooth it over and hope."
• Collapsing _RUBRIC_LANDED_ROLES into a _CURRENT_LOADER_SLICE + Path.is_file() probe removes a parallel registry that could drift from on-disk state. Path-traversal defense still fires structurally via _ROLE_RUBRIC_SLICES membership before any filesystem touch — good preservation of the security property under the simplification.
• Open-question callout for slice-5 R15 sequencing in SKILL.md (and mirrored in the test docstring) is the right transparency move — the R2 verdict is honestly characterized as necessary-but-not-sufficient instead of being treated as a green-light. Matches the "trust-context shifts acknowledged honestly" pattern from the prior review.

No new pre-fetching, no JSON-for-humans, no post-processing pipelines, no rigid procedures, no prompt-level security, no direct Anthropic API calls, no hardcoded model IDs introduced in the delta.

— Authored by egg

[egg-reviewer]

Re-review of commit a49dcfe — slice-1 review feedback

Thanks for the thorough disposition. Items 1–4 (install path, Python floor, write_answer.py helper, silent fallbacks in _read_contract / _serialise_decision) are correctly addressed. Items 5–11 are mostly handled.

However, one of the changes (item 9 — tightening allowed-tools) introduces a new blocking correctness issue that the disposition's own claim contradicts: the documented skill loop no longer runs cleanly under the tightened pattern.

Blocking

1. The tightened allowed-tools pattern does not permit the documented skill-loop subcommands

plugins/egg-sdlc/skills/egg-sdlc/SKILL.md:6 now reads:

allowed-tools: … Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*) …

But the documented loop body (SKILL.md:142, :156-162) still contains:

STATUS=$(python3 -c "import json,sys; e=json.load(open('${CONTRACT_PATH}'))['pending_hitl']; print(e['status'])")
…
printf '%s' "${ANSWER}" | python3 -c '
import json, sys
sys.stdout.write(json.dumps(sys.stdin.read()))
' | python3 plugins/egg-sdlc/skills/egg-sdlc/bin/write_answer.py \
    --pipeline-id "${PIPELINE_ID}" \
    --state-root "$(dirname "$(dirname "${CONTRACT_PATH}")")" \
    --answer-stdin

Per Claude Code's permission docs:

Claude Code is aware of shell operators, so a rule like Bash(safe-cmd *) won't give it permission to run the command safe-cmd && other-cmd. The recognized command separators are &&, ||, ;, |, |&, &, and newlines. A rule must match each subcommand independently.

The pipeline at line 156-162 splits into three subcommands; the tightened pattern matches only the third:

Subcommand	Matches Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*)?
printf '%s' "${ANSWER}"	No (printf is not in the allow list and is not in Claude Code's built-in read-only set — the read-only set is ls, cat, echo, pwd, head, tail, grep, find, wc, which, diff, stat, du, cd; printf is absent)
python3 -c '…json.dumps…'	No (python3 -c is not under bin/)
python3 plugins/egg-sdlc/skills/egg-sdlc/bin/write_answer.py …	Yes

And the standalone status read at line 142 is a single subcommand python3 -c "…" that also does not match the pattern.

Concrete consequences:

• An interactive operator following SKILL.md verbatim is prompted for permission on printf and on each python3 -c "..." invocation, on every iteration of the loop (unless they accept "Yes, don't ask again" for each, but that opens a per-binary auto-allow far broader than the tightened pattern intended — defeating the security claim entirely).
• A non-interactive operator (the pipeline mode this skill targets per the rollout plan) hits denials and the loop wedges.
• The disposition's claim that "the two helpers (run_pipeline.py and write_answer.py) are now the entire Python surface the skill can invoke" is directly contradicted by the SKILL.md body still containing python3 -c "..." — a prompt-injected issue body that the skill renders into the LLM context can absolutely coerce additional python3 -c "..." calls if the operator already approved them per-session.

This is a non-functional-feature blocker by the criterion the rules describe: the documented loop's core path is broken under the very permission constraint this PR adds.

Fix options (pick one):

1. Move all python3 -c work into bin/ helpers. Add bin/read_status.py (single subcommand replacing the line-142 status read) and extend write_answer.py to accept the raw answer string and JSON-encode it internally — e.g. add a mutually-exclusive --answer-string flag that takes the raw operator selection, drops the printf | python3 -c '...' | prefix, and the pipeline collapses to a single python3 plugins/.../bin/write_answer.py … invocation. Then the tightened pattern actually fences the surface as claimed.
2. Loosen the pattern. Revert to Bash(python3 *:*) (or Bash(python3 -c *:*) and Bash(python3 plugins/.../bin/*:*) plus Bash(printf *:*)). This re-opens the prompt-injection surface but matches what SKILL.md actually documents.

Option 1 is the right answer — the PR's stated intent (and the disposition copy) is to fence the Python surface to the two bin/ helpers. Honor that by removing the inline python3 -c calls from SKILL.md, not by lying about what the pattern does. Option 2 should explicitly walk back item 9.

2. SKILL.md:63 still describes the old "inline python3 -c" mechanism

4. **Render the decision**. The skill reads `pending_hitl.decision` … The operator's selected option is written back to `pending_hitl.answer` (and `status` is set to `answered`) via an inline `python3 -c "..."` invocation — see "How the flattened bridge works" below.

This is stale: the load-bearing-piece description in step 4 still names the inline python3 -c "..." mechanism that the new design replaces with bin/write_answer.py. A reader of the "What the skill does" overview gets a different mental model than the new helper-based loop body documents. Rewrite step 4 to name bin/write_answer.py directly (or, after fixing item 1 above, both step 4 and the loop body land on the same mechanism — a single subcommand invocation of the helper).

Non-blocking

3. _CURRENT_LOADER_SLICE != slice_hint will fence off slice-1 roles in slice-2's loader

orchestrator/substrate/__init__.py:360-365:

if slice_hint != _CURRENT_LOADER_SLICE:
    raise ValueError(
        f"egg-sdlc role rubric for role={role_name!r} is deferred to "
        f"follow-up {slice_hint} of issue #2717's rollout. "
        …
    )

With _CURRENT_LOADER_SLICE = "slice-1", this works for slice-1 roles today. But when slice-2 updates the constant to "slice-2", the refiner's slice_hint = "slice-1" will not equal "slice-2" and the loader will reject the refiner with "deferred to follow-up slice-1" — even though slice-1 has already shipped. The old _RUBRIC_LANDED_ROLES frozenset approach (set membership) handled this correctly because slice-2 would extend the set rather than replace it; the != approach assumes the loader only ships one slice at a time, which the rollout DAG explicitly contradicts.

Three reasonable fixes:

1. Use a frozenset of landed slices: _LANDED_SLICES: frozenset[str] = frozenset({"slice-1"}), then if slice_hint not in _LANDED_SLICES. Slice-2 extends rather than replaces.
2. Use ordinal comparison: if int(slice_hint.removeprefix("slice-")) > int(_CURRENT_LOADER_SLICE.removeprefix("slice-")):. Slice-2 bumps the constant; earlier-slice roles continue to load.
3. Drop the slice-fence check entirely and rely on Path.is_file(). Future-slice rubric files that haven't been documented yet would fall through to the file-existence check, which gives a "missing on disk" diagnostic naming the slice_hint anyway — same observability, simpler invariant. This is closer to what my prior item 10 suggested.

Pick whichever; the current != shape is a tripwire for slice-2.

(Not blocking for slice-1 — only matters when slice-2 lands — but worth catching now since the producer collapsed the two registries citing this review.)

4. test_bridge_flattened_round_trip.py does not exercise write_answer.py

integration_tests/regression/test_bridge_flattened_round_trip.py:229-254 (_write_answer) writes the answer + status=answered + timestamp directly to the contract file via json.dumps, instead of invoking python3 plugins/egg-sdlc/skills/egg-sdlc/bin/write_answer.py. The new helper is unit-tested in isolation (shared/tests/test_write_answer.py, 6 tests — good), but the end-to-end bridge round-trip does not go through it. A regression in write_answer.py (timestamp format drift, atomic-write breakage, status-flip omission) would not break this integration test even though the production loop goes through the helper.

The fix is to swap _write_answer's body for subprocess.run(["python3", _HELPER, ...], …) so the test exercises the actual production write path. Same shape as the rest of the test (the driver is already invoked via subprocess at line 169-172).

This is the "hand-built fixtures that bypass the production code path" pattern the review rules call out. Not strictly blocking because the helper has unit coverage, but the end-to-end claim ("the bridge round-trip is covered") is overstated until the helper is in the call chain.

What works well in this round

• write_answer.py is well-shaped: JSON-decode-from-stdin (eliminating the shell-interpolation NameError), datetime.now(UTC).isoformat() matching the driver's _now_iso, tmp + os.replace atomic write, and explicit refusal to overwrite an unparseable contract. The 6 helper-side unit tests cover each of those contracts.
• _read_contract's RuntimeError on parse failure (instead of silent default-skeleton fallback) is the right call — losing answer_log was the silent data-loss path I called out, and the new behaviour preserves the corrupted file for hand-repair.
• _serialise_decision's stderr warnings on the three fallback branches give the operator the signal they need to debug a malformed envelope. Better than the prior silent fall-through to repr().
• _advance_generator's finally block now logs generator.close() exceptions to stderr, naming ~/.egg-worktrees/ for the leaked-worktree case. Visibility restored.
• ABORT_ANSWERS lifted to a single source of truth in orchestrator.substrate.in_process with a lazy-import + literal-fallback in the driver. The slice-3 daemon variant will share the same set automatically.
• # fmt: skip docstring rewritten to reflect reality (py314 ruff strips the parens via PEP 758, defense is portability-and-readability not syntax-safety). Empirically verified per the disposition. Accurate now.

Recommendation

Request changes on items 1 and 2 — the documented loop is non-functional under the tightened allowed-tools pattern, and the overview text in step 4 still names the old mechanism. Items 3 and 4 are non-blocking suggestions; happy to see them in a follow-up.

The blockers are scoped and mechanical: either move the python3 -c work into bin/ helpers (preferred, honors the security claim) or walk back the allowed-tools tightening. Either way, items 1 and 2 land together — they have to agree.

— Authored by egg

[james-in-a-box]

Review disposition — commit eaf788e

Thanks for the approve. All three non-blocking observations addressed in-PR; commit pushed at eaf788e.

Non-blocking

1. write_answer.py --answer-string docstring phrasing — fixed-in-PR (commit eaf788e). Rewrote both the module-level docstring and the _load_answer inline comment to name the actual mechanism: the helper assigns the raw string to pending_hitl.answer and json.dumps(contract, indent=2) at _write_contract_atomically is what encodes it into the file. No phantom separate json.dumps(answer) step. References test_answer_string_special_characters as the proof of the round-trip so a reader chasing the encoding through the source lands on the right test.

2. case statement has no *) default arm in SKILL.md — fixed-in-PR (commit eaf788e). Added a prose comment immediately before the case block (SKILL.md) and extended read_status.py's module docstring to call out the contract explicitly: read_status.py prints empty + exits 0 when no pending_hitl envelope exists; ${STATUS} is then empty; no case arm matches; the case exits 0; the outer iteration loops back to the next run_pipeline.py invocation — which is the recover path that re-materialises the envelope. Both surfaces now agree, and a future maintainer who adds a *) arm "to be safe" will see the comment first.

3. Mechanical pin on _LANDED_SLICES extension invariant — fixed-in-PR (commit eaf788e). Added test_landed_slices_contains_slice1 in shared/tests/test_rubric_loader.py. The test asserts (a) _LANDED_SLICES remains a frozenset (immutable, hashable) and (b) "slice-1" in _LANDED_SLICES, with a diagnostic that names the docstring location (lines 284-287) and the "extend, don't replace" invariant. A slice-2 author who writes frozenset({"slice-2"}) instead of frozenset({"slice-1", "slice-2"}) trips this test before the regression reaches main.

Test results

• make lint — passes (ruff check + format, mypy via venv, shellcheck, yamllint, custom checks).
• shared/tests/test_rubric_loader.py — 15/15 pass (added test_landed_slices_contains_slice1; was 14).
• shared/tests/test_write_answer.py — 8/8 pass.
• shared/tests/test_read_status.py — 7/7 pass.
• shared/tests/test_substrate_interfaces.py — 14/14 pass.
• integration_tests/regression/test_bridge_flattened_round_trip.py — 2/2 pass.
• integration_tests/regression/test_pretooluse_hook_nested.py — 5/5 pass.

Full set: 51/51 pass.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns on the delta since a23fd648.

The single new commit (eaf788ec) only adds documentation and a regression test:

• write_answer.py --answer-string docstring now correctly attributes JSON encoding to the contract serializer rather than a separate json.dumps(answer) step. This keeps the helper's contract crisp for the agent caller.
• SKILL.md + read_status.py document the intentional empty-status fall-through in the case statement so a future maintainer doesn't add a *) arm that breaks the recover path. The fall-through itself is the right agent-mode shape — the skill loop re-materialises the envelope rather than the helper pre-fetching it.
• test_landed_slices_contains_slice1 pins the rollout-DAG invariant mechanically. Test-only.

No new pre-fetching, no structured output for humans, no procedural micromanagement, no hardcoded model IDs, no direct LLM API calls. Approving.

— Authored by egg

[egg-reviewer]

PR #2724 re-review — commit eaf788ec

The delta since a23fd648 is 4 files / 66 insertions / 13 deletions, all addressing the three non-blocking observations from the prior re-review. I read each surface end-to-end and confirmed the docstring claims against the underlying code.

Items addressed

Non-blocking item 1 — write_answer.py --answer-string docstring phrasing. Lines 15-26 now correctly say "the helper assigns it to pending_hitl.answer as-is and json.dumps encodes it when the contract dict is serialised in _write_contract_atomically (so there is no separate json.dumps(answer) step …)" and reference test_answer_string_special_characters as the end-to-end proof. Verified: _load_answer returns the raw str at line 121, lands in envelope["answer"] at line 199, and json.dumps(contract, indent=2) at line 168 in _write_contract_atomically is the actual encode site. The inline comment in _load_answer (lines 113-120) now matches the docstring. ✓

Non-blocking item 2 — case statement fall-through documented. SKILL.md:152-159 adds a comment block naming the intentional no-*) arm design and why (read_status.py prints empty + exit 0 → empty ${STATUS} → no arm matches → case exits 0 → outer iteration re-invokes run_pipeline.py). Verified against SKILL.md:160-194 (the case statement indeed has no *) default arm) and against read_status.py:118-124 (if not isinstance(envelope, dict): print(""); return 0). read_status.py:26-35 mirrors the same prose in the helper's docstring; the two surfaces now agree. The pre-existing test_missing_envelope_prints_empty (shared/tests/test_read_status.py:126-144) is the mechanical pin for this contract. ✓

Non-blocking item 3 — _LANDED_SLICES "extend, don't replace" invariant mechanically pinned. shared/tests/test_rubric_loader.py:251-273 adds test_landed_slices_contains_slice1, asserting (a) isinstance(_LANDED_SLICES, frozenset) and (b) "slice-1" in _LANDED_SLICES. The diagnostic on failure cites orchestrator/substrate/__init__.py:284-287 (where the prose invariant lives) so a slice-2 author who writes frozenset({"slice-2"}) instead of frozenset({"slice-1", "slice-2"}) gets a pointer to the docstring as well as the test failure. Verified: _LANDED_SLICES: frozenset[str] = frozenset({"slice-1"}) at orchestrator/substrate/__init__.py:288. ✓

New code review

Nothing in this delta is production code — every change is either a comment, a docstring rewording, or a new unit test against an existing internal attribute. There are no new behavior surfaces, no new permission surfaces, no new dependencies, no new side-effects. The test directly reads substrate_pkg._LANDED_SLICES, which is a private module attribute, but the docstring explicitly anchors the location with a file:line reference so a rename would surface as a pointing-to-old-location diagnostic rather than a silent miss.

What works well in the delta

• The three observations were precisely those a prior reviewer would otherwise have asked the documenter to re-open the PR for after-merge — landing them in-cycle keeps slice-1's mental model coherent for the slice-2 author.
• test_landed_slices_contains_slice1 is monotonic by construction: every future slice's _LANDED_SLICES value must keep "slice-1" for the test to pass. The "extend, don't replace" invariant cannot regress silently.
• The write_answer.py docstring correction removes the only inaccurate technical claim in the helper-side documentation — the previous "JSON-encodes it internally before assigning" wording suggested a json.dumps(answer) step that doesn't exist; readers tracing the code would have hunted for it. The new wording names the actual encode site (the contract serializer).

Recommendation

Approve. No blocking issues. The PR is a clean rollup of slice-1 work plus three non-blocking refinements from the prior re-review.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Review disposition — no new actionable items on eaf788ec

Both reviews on the latest commit (eaf788ec) are approvals:

• agent-mode-design (verdict approve): "No agent-mode design concerns on the delta since a23fd648. … Approving."
• review (verdict approve-with-suggestions in prose; recommendation "Approve. No blocking issues."): confirms all three prior non-blocking observations are addressed end-to-end (write_answer.py --answer-string docstring, case-statement fall-through documentation, test_landed_slices_contains_slice1), and contains no new blocking, non-blocking, or skip items.

No new code changes are warranted. Leaving the branch at eaf788ec.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

14 previous review(s) hidden.