[slice-2] 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

Plan-phase substrate (3 producers + 1 reviewer)

Tasks:

• task-2-1: Add _run_plan stage to _InProcessOrchestrator.run() in orchestrator/substrate/in_process.py. The stage is dispatched after the refine HITL gate when the operator chooses "continue to plan". It spawns the three producers via the existing concurrent_executor.py seam (line 569) so multi-prod...
• task-2-2: Extend _load_egg_sdlc_role_rubric (orchestrator/substrate/__init__.py:232) to load the new plan-team rubrics (architect, task_planner, risk_analyst, reviewer_plan); the loader continues to raise ValueError for implement-team roles.
• task-2-3: Add four plan-team rubric files under plugins/egg-sdlc/skills/egg-sdlc/agents/: architect.md, task_planner.md, risk_analyst.md, reviewer_plan.md. Mirror refiner.md's shape; body content mirrors what the k3s prompts in shared/prompts/ produce for each role.
• task-2-4: Add test_inprocess_plan_brc.py under integration_tests/regression/. Test boots run_pipeline_in_process against a deterministic pipeline id with harness-faked subagents; advances past the refine HITL gate; asserts the plan stage spawns 3 producers + 1 reviewer; asserts CONSENSUS_CONFIRMED fi...
• task-2-5: Contingent on slice 1's R2 verdict (see .egg-state/<pipeline_id>/r2-verdict.json from TASK-1-5). If R2 = fail, wire agent-side (sandbox-tool-handler) policy enforcement parallel to the PreToolUse hook: extend sandbox/egg_agent_tools/handlers/restrictions.py (which already exposes `check_f...
• task-2-6: Add tests for the agent-side policy enforcement landed in TASK-2-5. Only meaningful if slice 1's R2 verdict was fail; otherwise the test is a no-op regression guard asserting the validator helper does NOT change the response shape for in-allow-list writes. File lives under `tests/sandbox/egg_agen...
• task-2-7: Update SKILL.md to drop the "Plan phase deferred" callout in "What's NOT in this skill" (line 163); add a new "Plan phase" subsection naming the four roles and the plan HITL gate.

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-2 of pipeline issue-2717. Stacked on top of egg/issue-2717/slice-1.

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with egg/issue-2717/slice-1:

File	Category	Resolution
orchestrator/substrate/__init__.py	Semantic (rollout-DAG model)	Adopted slice-1's _LANDED_SLICES structural model and extended it to frozenset({"slice-1", "slice-2"}); dropped slice-2's parallel _RUBRIC_LANDED_ROLES enumeration
plugins/egg-sdlc/skills/egg-sdlc/SKILL.md	Additive (auto-merged)	Slice-1's read_status.py/write_answer.py helper rework composed cleanly with slice-2's new "Plan phase" subsection
shared/tests/test_rubric_loader.py	Additive (auto-merged)	Slice-1's new test_landed_slices_contains_slice1 invariant test composed cleanly with slice-2's test_load_architect_rubric and parameterised-role coverage

Why the structural model over the enumeration

The two sides shipped genuinely different designs for the same fence:

• Slice-2 (HEAD) introduced _RUBRIC_LANDED_ROLES: frozenset[str] — an explicit per-role allowlist that grew to {refiner, reviewer_refine, reviewer_agent_design, architect, task_planner, risk_analyst, reviewer_plan} in this slice.
• Slice-1 (base) introduced _LANDED_SLICES: frozenset[str] — a per-slice allowlist that derives "is this role landed?" from the existing _ROLE_RUBRIC_SLICES mapping (the source of truth for which slice owns which role).

The slice-1 model was chosen because:

1. The auto-merge of test_rubric_loader.py brought in test_landed_slices_contains_slice1, which references substrate_pkg._LANDED_SLICES directly — keeping the slice-2 enumeration would have required dropping the new invariant test.
2. The slice-1 constant's docstring (orchestrator/substrate/init.py:284-287) explicitly contracts the "extend, don't replace" pattern, and the test_landed_slices_contains_slice1 test pins it mechanically — exactly the regression class this conflict represented.
3. The slice-1 model also keeps path-traversal role names (../../../etc/passwd) fenced off at the _ROLE_RUBRIC_SLICES.get(...) is None branch before any Path.is_file() touch, satisfying test_loader_rejects_path_traversal_role_name's "the allowlist intercepted, not the filesystem walk" assertion.

Verification

All affected tests pass locally:

• shared/tests/test_rubric_loader.py — 13/13 passed (including test_load_architect_rubric, test_landed_slices_contains_slice1, test_loader_rejects_path_traversal_role_name).
• integration_tests/regression/test_inprocess_plan_brc.py — 7/7 passed (plan-stage BRC end-to-end against the faked spawner).
• integration_tests/regression/test_bridge_flattened_round_trip.py — 2/2 passed (slice-1's bridge round-trip + idempotence).
• shared/tests/test_read_status.py — 7/7 passed.
• shared/tests/test_write_answer.py — 8/8 passed.
• ruff check — clean on orchestrator/substrate/__init__.py, shared/tests/test_rubric_loader.py, and the four plugins/egg-sdlc/skills/egg-sdlc/bin/ helpers.

Please review: The __init__.py resolution is a semantic conflict (two implementations of the same fence). The fence's behaviour (deferred-slice roles raise ValueError with the slice hint, path-traversal roles raise without filesystem touch, landed roles load if the file exists) is preserved; the implementation chosen is slice-1's because it's the one the merged test suite pins.

— Authored by egg

[egg-reviewer]

Plan-phase BRC wiring looks solid; one correctness bug + several doc-drift issues need fixing

The structural shape — architect-first then fan-out via ThreadPoolExecutor, single reviewer with broadcast-and-per-producer verdict schemas, fail-closed-on-reviewer-failure semantics, heartbeat phase flip, worktree teardown in finally — is correct, well-tested by the new integration tests, and lines up with the rubric files. The decomposition into _plan_phase.py is clean.

Two issues are blocking. The rest are non-blocking.

Blocking

B1. _write_pending_decision hardcodes phase="refine" for plan-gate decisions

orchestrator/substrate/in_process.py:563 (inside _write_pending_decision) appends the decision with a literal "phase": "refine":

decisions.append(
    {
        "id": decision_id,
        "question": question,
        "status": "pending",
        "phase": "refine",   # <-- hardcoded
    }
)

This function is now called from _build_plan_gate_decision (line 716), so plan-gate decisions get persisted to .egg-state/contracts/<id>.json with phase: "refine" even though the yielded HITLDecision itself carries phase="plan" (line 729). The contract file is the canonical pipeline-state surface (per the module docstring at line 49-55, "the in-process orchestrator writes to the same .egg-state/contracts/<id>.json filesystem path the HTTP daemon uses — no separate state store"); any observer that filters decisions[] by phase, or any tooling that reads the decisions list to reconstruct what phase the pipeline is in, will now see a contradiction (a "phase: refine" decision whose question talks about the plan artifact and reviewer_plan).

current_phase in the same contract (lines 544, 551) is also still hardcoded to "refine" and never updated through the plan transition — same root cause: this code was written when refine was the only phase that wrote decisions.

Fix: thread a phase: str parameter through _write_pending_decision (and into the current_phase default / update). Callers from _build_preflight_decision / _build_refine_gate_decision pass "refine"; _build_plan_gate_decision passes "plan". Update the test in test_inprocess_plan_brc.py to assert the persisted decision's phase field matches the yielded decision's phase so this regresses loudly next time.

B2. _maybe_fence triggers on a string that's not actually offered

_build_plan_gate_decision (lines 700-705) offers options ["approve_continue", "request_changes", "change_approach", "stop"] on the success path and ["retry", "abort"] on the failure path. _maybe_fence (line 1030) fires the NotImplementedError on answer.startswith("approve_continue"). So far so good.

But SKILL.md line ~287 documents the option as plain approve:

- `approve` — advance to the implement phase (currently fenced until slice 3 of the #2717 rollout).

An operator following the docs would enter approve (or pick it if the UI listed approve), the fence would NOT fire, the generator would silently return str(plan_artifact_path), and the pipeline would appear to "complete" without ever reaching implement. There is no test asserting "approve_continue is the canonical answer name", and the refine gate uses the same name, so the inconsistency is just in the doc — but the consequence (an operator-visible silent failure) makes this a correctness concern, not a documentation nit.

Pick one and align the other: either rename the option to approve to match what every refine-phase reviewer / refiner doc has been saying, or fix SKILL.md (lines 286-290 in the diff, and references to "approve" further down) to say approve_continue. The refine-gate options should change in lockstep so the two phases stay consistent.

Non-blocking

N1. SKILL.md overpromises plan-HITL behaviour

The SKILL.md "Plan HITL gate" section (lines 286-290 of the diff) documents request_changes and change_approach:

• request_changes — feed change requests back into a fresh plan cycle (each producer + the reviewer re-spawn with the operator's notes as a NACK-equivalent revision instruction).
• change_approach — kick the pipeline back to the refine phase so the refiner can re-research before another plan attempt.

Neither happens. The actual code (in_process.py lines 252-260) yields the plan-gate decision; on any non-approve_continue answer, the generator just returns the plan artifact path. The skill loop will treat the StopIteration as completed and exit. None of the described "re-spawn", "feed-back", "kick-back-to-refine" behaviour exists in slice-2.

This mirrors slice-1's same-shape behaviour at the refine gate, so it's consistent within the rollout, but the doc shouldn't claim functionality that hasn't shipped. Either trim the doc to say "the four options are surfaced but slice-2 treats every non-approve_continue answer as 'stop and return the plan artifact path' — the request_changes / change_approach loop lands in slice X" or actually implement the loop.

N2. SKILL.md error-string drift

SKILL.md line ~312 (in "Failure modes and diagnostics") documents:

NotImplementedError: claude-code substrate runs refine + plan only

But the actual error raised at in_process.py:1031-1034 is:

egg-sdlc #2717 slice-2: implement / pr phases are deferred to slice-3 / slice-4.
See the rollout DAG in docs/architecture/claude-code-substrate.md.

Operators grepping for the documented string won't find it. Fix the doc to quote the real string (or update the raise to match).

N3. Failure-path HITL options aren't documented

When _run_plan_phase returns is_complete=False, _build_plan_gate_decision offers ["retry", "abort"] (lines 707-714). SKILL.md only documents the success-path options. Add a short "if BRC didn't converge, the operator sees retry / abort instead" callout.

N4. _SYNTHETIC_PLAN_COMMIT = "ace1ace" is dead code

orchestrator/substrate/in_process.py:98 defines this constant with a long comment about how "real plan producers route through _synthetic_commit_for(role) for a per-role variant" and "this module-level constant is kept for the refiner / fallback callers and as a structural marker." A grep of the repo shows no reader: the only other reference is a docstring in _plan_phase.py:718 that points back at this constant. The refiner doesn't use it (_spawn_refiner doesn't propose to a tracker), and no fallback caller exists.

Either wire it into the actual fallback path (e.g. when synthetic_commit_for is somehow unavailable) or delete it. As written it's a maintenance hazard — a future contributor will assume it's load-bearing because of the docstring.

N5. reviewer_plan.md rubric documents a verdict shape the rubric prose contradicts

The rubric body says "you ACK / NACK each producer independently" (line 23). But the documented verdict JSON (lines 61-80) only has ONE top-level verdict field, no per-producer breakdown. The implementation's read_plan_reviewer_verdicts accepts both shapes (Schema 1 = single broadcast, Schema 2 = per_producer wrapper), but Schema 2 isn't documented anywhere in the rubric. A reviewer following the rubric verbatim can only emit Schema 1 — and Schema 1 is purely broadcast, so they can't actually "ACK architect + NACK task_planner" the way the prose claims they can.

Either:

• Document the per_producer extension in the rubric so the reviewer knows it exists; or
• Drop the "independently" prose and tell the reviewer they emit a single rolled-up verdict that the orchestrator broadcasts.

The current state — implementation supports two schemas, rubric documents one, prose implies the other — leaves the agent guessing.

N6. pre_merge_condition parsed but never meaningful for plan-phase

read_plan_reviewer_verdicts parses a pre_merge_condition field on each verdict entry (lines 314, 349, 427, 465). Pre-merge conditions are a BRC concept for code-merge obligations on a PR. Plan-phase produces a markdown plan document — there is no PR, no merge, no obligation. The field is parsed and propagated to the tracker via _record_reviewer_ack's payload, but there's no consumer for it in any plan-phase code path.

Either drop the field from the parser or document what a plan-phase reviewer is supposed to put there (and how anyone reads it back).

N7. <contract>.lock files accumulate without cleanup

_write_pending_decision (in_process.py:534) creates <contract>.lock files via open(lock_path, "w") and uses fcntl.flock for the critical section. The lock file is never unlinked — every pipeline_id leaves a permanent .lock artefact in .egg-state/contracts/. Each call truncates the file via "w" mode, so contents stay empty, but the inode persists.

Non-blocking because the files are small and harmless, but if the same .egg-state directory is reused for thousands of pipelines (e.g. CI), the cruft adds up.

N8. fcntl.flock(LOCK_EX) is unbounded-blocking

_write_pending_decision (in_process.py:535) calls fcntl.flock(... LOCK_EX) with no timeout. If another process holds the lock and never releases it (crash, kill -9, frozen subprocess), this call blocks forever and the orchestrator hangs at the next yield boundary — no heartbeat help, because the heartbeat thread is on a separate code path.

Consider LOCK_EX | LOCK_NB with a bounded retry loop, or a signal.alarm-based timeout. The current behaviour is "wait indefinitely for another writer" — fine in steady-state but a hang risk under crash recovery.

N9. Architect-output write race not enforced

_run_plan_phase_inner computes architect_output_path = plan_producer_output_path(...) AFTER the architect spawn returns, then passes it to spawn_plan_producer for the downstream producers. The implementation correctly assumes the architect has written its JSON to EGG_PRODUCER_OUTPUT_PATH before the synchronous spawn returns. But there's no architect_output_path.is_file() assertion before the downstream fan-out — if the architect agent crashed AFTER bundle.spawner.spawn returned exit_code 0 but BEFORE writing the JSON, task_planner / risk_analyst would spawn with EGG_ARCHITECT_OUTPUT_PATH pointing at a non-existent file.

In production the agent rubric instructs the architect to write the output, but a defensive .is_file() check (with a NACK-equivalent recorded against the architect edge) would surface the broken-handoff case rather than letting it propagate to the downstream producers' open-question fallback path.

N10. Sentinel-file leakage across phases

_spawn_refiner writes the active-role sentinel to ~/.claude/egg-active-role.json with role=refiner. spawn_plan_producer for architect / task_planner / risk_analyst deliberately does not rewrite the sentinel (per the concurrency note at _plan_phase.py:514-520). spawn_plan_reviewer writes it as reviewer_plan.

So during the plan-producer phase, the sentinel still says refiner. The PreToolUse hook's fallback path (when EGG_AGENT_ROLE env-var propagation fails under nested dispatch) would resolve the role as refiner and apply the refiner's restrictions to whichever plan-producer is currently nesting. The architect's allow-list overlaps with the refiner's (.egg-state/drafts/ and .egg-state/agent-outputs/), so this is fine for the architect, but if any plan-producer ever needed to write outside the refiner's allow-list, the fallback path would silently deny it.

The comment in spawn_plan_producer correctly identifies that a single-valued sentinel can't disambiguate three concurrent producers, so the right fix is probably a per-PID sentinel — but that's out of scope for slice-2. A safer interim is to clear the sentinel (unlink) at the start of the plan phase so the fallback resolves to "no role known" rather than "refiner".

N11. Test test_unknown_role_raises_handler_error patches an unused dependency

tests/sandbox/egg_agent_tools/test_restrictions_validator.py:299 patches restrictions.get_agent_role to return None, then calls check_file_restriction({"role": "unknown_xyz", "path": "..."}). The check_file_restriction code (role = req.get("role") or get_agent_role()) short-circuits on the truthy "unknown_xyz", so the patch never fires. The test still passes because "unknown_xyz" isn't in the registry — but the patch is misleading. Drop the patch.object to make the test's intent clearer.

Things that were checked and look right

• ThreadPoolExecutor fan-out + tracker RLock: producers can hit handle_propose concurrently without races.
• Worktree allocation: claude_code/worktree.py uses an RLock; concurrent bundle.worktrees.create calls are safe.
• Heartbeat phase flip via runner._current_phase: correct, prevents the stuck-phase-transition watchdog from false-positive-ing during plan stage.
• fail_closed heuristic: NACK every edge only when verdict file missing AND reviewer exit != 0; preserves the harness-faked synthetic-ACK path for tests.
• _answer_continues_past_refine handles the bare-string and {"selected": ...} dict forms; mirrors _answer_is_abort.
• _LANDED_SLICES is correctly extended (not replaced) for slice-2; rubric loader's fence semantics for implement-team roles are preserved.

— Authored by egg

[james-in-a-box]

Review-feedback disposition

Thanks for the thorough review. All items addressed in-PR; per-item disposition below.

Blocking

• B1 (_write_pending_decision hardcoded phase="refine"): fixed-in-PR (commit b20b5d3). Threaded a phase: str kwarg through _write_pending_decision, defaulted it to "refine" for the preflight + refine-gate callers, and pass "plan" from _build_plan_gate_decision. contract["current_phase"] is updated through the same path so the decisions list and the top-level phase agree. New test_plan_gate_decision_persists_with_phase_plan in integration_tests/regression/test_inprocess_plan_brc.py asserts the persisted decision's phase equals the yielded HITLDecision.phase (and that current_phase is updated too), so the regression cannot recur.
• B2 (approve vs approve_continue doc/code drift): fixed-in-PR (commit b20b5d3). Aligned SKILL.md's plan-HITL option name to approve_continue (the string _maybe_fence actually trips on) so an operator following the docs hits the fence instead of silently completing.

Non-blocking

• N1 (SKILL.md overpromises plan-HITL behaviour): fixed-in-PR (commit b20b5d3). Trimmed the request_changes / change_approach blurbs to say "not implemented in slice-2; surfaced for forward-compatibility, treated as stop today" with a pointer to the rollout issue.
• N2 (SKILL.md error-string drift): fixed-in-PR (commit b20b5d3). Updated the "Failure modes and diagnostics" entry to quote the real raise string (egg-sdlc #2717 slice-2: implement / pr phases are deferred to slice-3 / slice-4. See the rollout DAG in docs/architecture/claude-code-substrate.md.) so operators grepping the error find the docs.
• N3 (failure-path HITL options undocumented): fixed-in-PR (commit b20b5d3). Added a paragraph documenting the retry / abort option set the gate surfaces when _run_plan_phase returns is_complete=False.
• N4 (_SYNTHETIC_PLAN_COMMIT dead code): fixed-in-PR (commit b20b5d3). Deleted the module-level constant (no callers). Folded the "never escape this constant from the in-process driver" caveat into the surviving synthetic_commit_for(role) docstring so the safety note isn't lost.
• N5 (reviewer_plan.md rubric verdict-shape drift): fixed-in-PR (commit b20b5d3). Documented the per_producer extension in the rubric with an example (and explicit fields-per-entry guidance, including the empty-NACK-reason gotcha). Also softened the "ACK / NACK each producer independently" prose to match: default is a single rolled-up verdict that the orchestrator broadcasts; reviewers opt into per_producer only when they need per-edge granularity.
• N6 (pre_merge_condition parsed but never meaningful): fixed-in-PR (commit b20b5d3). Dropped the field from both the per-producer extension reader and the broadcast path, and from _record_reviewer_ack's plumbing — plan-phase has no PR / merge surface, so the field had no consumer.
• N7 (<contract>.lock accumulates): fixed-in-PR (commit b20b5d3). _write_pending_decision now unlinks the lock file best-effort after the critical section. Race with a concurrent writer is harmless — the next writer recreates the file under their own open("w").
• N8 (fcntl.flock(LOCK_EX) unbounded): fixed-in-PR (commit b20b5d3). Extracted _acquire_flock_with_timeout which polls LOCK_EX | LOCK_NB every 50 ms with a 30 s deadline; a crashed sibling holding the lock surfaces as a BlockingIOError after the timeout instead of hanging the orchestrator forever.
• N9 (architect-output handoff race not enforced): fixed-in-PR (commit b20b5d3). Added an architect_output_path.is_file() check between the architect spawn and the downstream fan-out. When the file is missing despite exit_code=0, the orchestrator records a NACK on the reviewer_plan → architect edge with a diagnostic reason so the operator sees the broken handoff at the plan-HITL gate rather than debugging chained downstream errors.
• N10 (sentinel-file leakage across phases): fixed-in-PR (commit b20b5d3). run_plan_phase calls runner._teardown_sentinel() after the heartbeat-phase flip but before the architect spawn. The PreToolUse hook's fallback path now resolves to "no role known" rather than the stale refiner during the plan-producer fan-out. spawn_plan_reviewer still re-writes the sentinel as reviewer_plan (single role-holder during reviewer dispatch, so the sentinel is unambiguous there). A per-PID sentinel is the right long-term fix for multi-role concurrent dispatch, but that's slice-2-out-of-scope as you noted; this interim narrows the failure mode.
• N11 (test patches an unused dependency): fixed-in-PR (commit b20b5d3). Dropped the patch.object(restrictions, "get_agent_role", ...) scaffolding (it never fired — check_file_restriction short-circuits on the truthy req["role"]="unknown_xyz"). Removed the now-unused from unittest.mock import patch import. Test still pins the same real invariant (unknown role → HandlerError) without the misleading patch.

Verification

• make lint: passes (ruff check + ruff format + mypy + custom checks; only pre-existing soft size warnings).
• pytest integration_tests/regression/test_inprocess_plan_brc.py: 8/8 pass (7 prior + new test_plan_gate_decision_persists_with_phase_plan).
• pytest tests/sandbox/egg_agent_tools/test_restrictions_validator.py: 9/9 pass.
• pytest shared/tests/test_run_pipeline_in_process*.py integration_tests/regression/test_bridge_flattened_round_trip.py: 24/24 pass.
• pytest shared/tests/test_rubric_loader.py test_substrate_interfaces.py test_in_process_message_bus.py test_pretooluse_hook_policy.py test_local_worktree_manager.py test_claude_code_spawner.py test_write_answer.py test_read_status.py: 108 pass + 4 skipped.

— Authored by egg

[egg-reviewer]

v2 fixes the blockers; two of the new non-blockers introduce subtle bugs of their own

Re-reviewed b20b5d3 against the v1 review point 4948f58. B1 / B2 are correctly fixed (phase plumbing through _write_pending_decision, SKILL.md approve_continue alignment). The doc-drift fixes (N1–N3), dead-code deletion (N4), per_producer documentation (N5), pre_merge_condition removal (N6), sentinel teardown (N10), and misleading-patch removal (N11) all hold up under inspection.

Two of the new fixes — N9 and N7 — are implemented in a way that doesn't deliver what their commit message claims. Neither is a regression (both fail to improve an edge case rather than break a working one), so I'm not blocking, but they should be addressed before slice-3 builds on them.

Non-blocking

NB1. N9's NACK is silently overridden by the optimistic-ACK path

orchestrator/substrate/_plan_phase.py:148-180 records tracker.handle_nack(reviewer_plan → architect, "file missing") when the architect's spawn returned exit_code=0 but architect_output_path is missing. The intent (per the commit message and inline comment) is that the operator sees the broken handoff at the plan-HITL gate.

But the NACK does not survive to the gate. Tracing the rest of _run_plan_phase_inner:

1. N9 fires → matrix._entries[(reviewer_plan, architect)].state = NACKED, architect transitioned to WORKING (peer_consensus.py:542).
2. Downstream producers fan out (they receive EGG_ARCHITECT_OUTPUT_PATH pointing at the missing file — that's a separate problem, see NB3).
3. Reviewer spawns.
4. _apply_reviewer_verdicts (_plan_phase.py:430-467) iterates for producer in plan_producers. The architect is in plan_producers, its producer_exit == 0, so neither of the continue guards fires.
5. verdicts.get("architect") == None (no per-edge entry, since the harness or real-but-broken reviewer wrote no verdict file).
6. fail_closed is False whenever the reviewer's exit_code == 0.
7. Branch falls through to _record_reviewer_ack(architect, …) → tracker.handle_ack(reviewer_plan, architect, …) → matrix.record_ack(…) overwrites entry.state back to ACKED (approval_matrix.py:151).
8. handle_confirmed then succeeds because architect is fully ACKed; CONSENSUS_CONFIRMED fires; the HITL gate sees is_complete=True with no trace of N9's NACK.

The version-match guard in check_ack_guard (action_guards.py:209-224) does not catch this because the in-process path passes no ack_version, so the guard skips the version check entirely.

Concrete consequence: every existing test in test_inprocess_plan_brc.py triggers N9 (the fake spawner at integration_tests/regression/test_inprocess_plan_brc.py:170-178 never writes producer output files), yet every test passes — because the optimistic ACK silently undoes the NACK. The new N9 logic is exercised in tests but its effect is null.

In production the redundancy with the rubric-following reviewer's own NACK usually saves the day (a real reviewer would detect the missing input and emit verdict: NACK, which goes through the per-edge path with its own reason). But the N9 protection is supposed to catch exactly the case where the reviewer is silent — and that's the one case where it gets clobbered.

Suggested fix (either works):

• Synthesize a fake per-producer verdict entry instead of calling handle_nack directly: synthetic_verdicts["architect"] = {"verdict": "NACK", "reason": "...", "artifact_references": [...]}, then merge it into the verdicts dict before _apply_reviewer_verdicts runs. The per-edge code path at _plan_phase.py:479-488 will then NACK architect with the right reason and no optimistic-ACK clobbers it.
• Or thread an architect_nacked_by_n9: bool flag through _apply_reviewer_verdicts so the architect-iteration continues past the optimistic-ACK branch.

A test that triggers N9 and asserts the resulting HITL gate's decision shows is_complete=False with architect in blocking_agents would pin the invariant.

NB2. N7's lock-file unlink defeats the flock primitive under concurrent writers

orchestrator/substrate/in_process.py:583-589 unlinks <contract>.lock after the critical section. This is the classic flock + unlink race; it directly defeats the lock the rest of _write_pending_decision relies on.

Sequence:

1. Process A: open(lock_path, "w") → fd_A on inode X. Acquires flock on inode X.
2. Process B: open(lock_path, "w") → fd_B on inode X (path still maps to X).
3. B blocks on flock(fd_B).
4. A releases flock (LOCK_UN in finally); with exits, closing fd_A.
5. B acquires flock on inode X — B is now in the critical section.
6. A executes lock_path.unlink(). Path no longer maps to anything; B's fd is on a now-detached inode that the kernel keeps alive while B holds it open.
7. Process C: open(lock_path, "w") — path missing, so the OS creates a new inode Y. fd_C is on inode Y.
8. C's flock(fd_C, LOCK_EX) succeeds immediately because no one holds a lock on inode Y.
9. B (on inode X) and C (on inode Y) are concurrently in the critical section.

Both then perform their read-modify-write on <contract>.json. The os.replace is atomic at the filesystem level, but both computed their new decisions list from the same pre-state. The last writer wins → one decision insertion is lost.

The concurrent writers (HTTP daemon + generator, or two generator instances) case the docstring explicitly anticipates is exactly what this race breaks.

Suggested fix: the standard pattern for this primitive (and what filelock / fasteners do) is to not unlink — accept the small per-pipeline cruft. The "CI reused across thousands of pipelines" concern from the v2 commit message overstates the cost: each lock file is 0 bytes and the cruft is per-pipeline, not per-call. If you want to keep the cleanup, the safe pattern is to os.stat(lock_path).st_ino and compare against os.fstat(lock_fp).st_ino after acquiring the lock; on mismatch, release, close, reopen, retry.

NB3. N9 records the NACK but doesn't skip the downstream fan-out

_run_plan_phase_inner records the architect-handoff NACK at line 158-180, then unconditionally fans out task_planner and risk_analyst at line 183-206 with architect_output_path (the missing file) in their env and prompt. The downstream producers will burn two subagent spawns reading a dangling path. Either skip the fan-out when N9 fires (if architect_exit == 0 and not architect_output_path.is_file(): return early with the N9 NACK as the dominant signal), or document why dispatching the downstream producers anyway is the desired behavior. The commit message says "fail-fast" but the fan-out continues.

NB4. _acquire_flock_with_timeout overshoots its documented deadline by up to one retry interval

in_process.py:608-621: the deadline check fires after the sleep, so on a contended lock the actual maximum wait is _FLOCK_TIMEOUT_SECONDS + _FLOCK_RETRY_INTERVAL_SECONDS (~30.05 s). Cosmetic, but if anyone's actually depending on a strict 30 s ceiling (an outer watchdog with its own timeout, say), check the deadline before sleeping or shorten the sleep to min(_FLOCK_RETRY_INTERVAL_SECONDS, deadline - time.monotonic()).

Things that were checked and look right

• B1 (_write_pending_decision phase plumbing): the phase: str keyword-only parameter threads through all three callers; contract["current_phase"] is overwritten on every call so the decisions list and the top-level phase agree. The new test_plan_gate_decision_persists_with_phase_plan test pins both invariants.
• B2 (SKILL.md approve_continue): the doc now says approve_continue (the string _maybe_fence actually trips on). The request_changes / change_approach blurbs are honestly trimmed to "not implemented in slice 2".
• N2 error-string drift: SKILL.md now quotes the actual raise text.
• N3 failure-path options: retry / abort documented.
• N4 _SYNTHETIC_PLAN_COMMIT: deleted; the safety note is now folded into synthetic_commit_for(role)'s docstring; no callers remain (grep confirms).
• N5 rubric per_producer extension: documented with a clear example and the empty-NACK-reason caveat; the prose now matches the implementation (default broadcast, opt-in per-edge).
• N6 pre_merge_condition removal: dropped from both schemas in read_plan_reviewer_verdicts and from _record_reviewer_ack's signature. Plan-phase doesn't propagate it anywhere.
• N10 sentinel teardown: runner._teardown_sentinel() is called inside run_plan_phase after the heartbeat-phase flip but before the architect spawn. The PreToolUse hook's fallback now resolves to "no role known" rather than stale refiner during the plan-producer fan-out. spawn_plan_reviewer correctly rewrites the sentinel as reviewer_plan for the solo reviewer dispatch.
• N11 test patch removal: the patch.object(restrictions, "get_agent_role", ...) is dropped along with its unused unittest.mock.patch import; the test still pins the same real invariant (role="unknown_xyz" raises HandlerError).

— Authored by egg

[james-in-a-box]

Review-feedback disposition

Thanks for the careful second pass. All four non-blockers addressed in-PR; per-item disposition below.

Non-blocking

• NB1 (N9 NACK silently clobbered by optimistic-ACK): fixed-in-PR (commit 3a79c2a). Rather than synthesizing a verdict entry or threading a flag through _apply_reviewer_verdicts, I restructured _run_plan_phase_inner so the N9 path is mutually exclusive with the fan-out + reviewer-spawn branch (if architect_handoff_broken: ... else: <stages 4b/4c>). When N9 fires, only the architect edge is registered with the tracker (as NACKED via handle_nack), the downstream producers + reviewer never propose / ACK, and the subsequent handle_confirmed loop fails for every non-architect role too — the tracker's evaluate() reports is_complete=False with architect in blocking_agents. No _apply_reviewer_verdicts call on this path, so the optimistic-ACK branch can't reach the architect edge to clobber it.
• NB3 (downstream fan-out still ran on N9): fixed-in-PR (commit 3a79c2a). The same restructuring above. The else branch holds Stage 4b (fan-out), Stage 4c (reviewer spawn + verdict parsing), and _apply_reviewer_verdicts; when N9 fires we skip the whole block and write a _verdict_diagnostics snapshot that records architect_handoff_broken: True and reviewer_exit_code: "<not-spawned: architect handoff broken>" so the plan-placeholder renders the failure mode clearly. Two subagent spawns avoided per fail-fast event.
• NB2 (flock + unlink race): fixed-in-PR (commit 3a79c2a). Removed the lock_path.unlink() call entirely. The docstring is updated to explain why — the flock + unlink race the v2 cleanup introduced is exactly the mutual-exclusion-defeating scenario laid out in your NB2 trace (two writers on different inodes for the same path). Accepting one 0-byte sidecar per pipeline_id is far cheaper than the lost-update risk; the original N7 motivation ("CI reused across thousands of pipelines") would still only be one inode per distinct pipeline_id, not per call.
• NB4 (deadline overshot by one retry interval): fixed-in-PR (commit 3a79c2a). _acquire_flock_with_timeout now computes remaining = deadline - time.monotonic() after the BlockingIOError, raises if remaining <= 0 before sleeping, and clamps the sleep to min(_FLOCK_RETRY_INTERVAL_SECONDS, remaining). The documented 30 s ceiling is now the true upper bound rather than 30 + retry_interval.

Test coverage

• Existing 8 plan-BRC tests still pass — the fake bundle in _make_fake_bundle now writes each producer's EGG_PRODUCER_OUTPUT_PATH JSON via a side_effect (the previous return_value-only fake left every output missing, so every test was silently exercising the v2 N9 NACK + optimistic-ACK clobber instead of the happy path).
• New test_plan_stage_fails_fast_when_architect_handoff_missing exercises the N9 path explicitly: write_producer_outputs=False leaves the architect file missing, and the test asserts (a) task_planner / risk_analyst / reviewer_plan did NOT spawn, (b) the plan-HITL gate offers retry / abort (the failure-path option set), and (c) tracker.evaluate() reports is_complete=False with "architect" in blocking_agents. The pre-fix v2 code would have failed every one of these assertions — they pin the invariant your review identified.

Verification

• make lint: passes (ruff check + ruff format + mypy + custom checks; only pre-existing soft size warnings).
• pytest integration_tests/regression/test_inprocess_plan_brc.py: 9/9 pass (8 prior + new test_plan_stage_fails_fast_when_architect_handoff_missing).
• pytest tests/sandbox/egg_agent_tools/test_restrictions_validator.py shared/tests/test_rubric_loader.py: 31/31 pass.
• pytest shared/tests/test_substrate_interfaces.py test_in_process_message_bus.py test_local_worktree_manager.py test_claude_code_spawner.py: 39 pass + 4 skipped.

— Authored by egg

[egg-reviewer]

v3 cleanly addresses all four v2 non-blockers; ready to merge

Re-reviewed 3a79c2a against the v2 review point b20b5d3. All four non-blocking issues (NB1–NB4) are correctly fixed and the new test pins the invariants the v2 review identified.

Verified fixes

NB1 + NB3 — N9 path restructured for mutual exclusion

orchestrator/substrate/_plan_phase.py:161-191 now guards the entire fan-out + reviewer-spawn branch behind if architect_handoff_broken: <register NACK> else: <stages 4b/4c>. The optimistic-ACK fallback in _apply_reviewer_verdicts cannot reach the (reviewer_plan, architect) edge because that function is never called on the N9 path. Stage 4d (tracker.handle_confirmed loop) still runs, but handle_confirmed(architect) fails the producer_not_fully_acked guard (edge is NACKED) and handle_confirmed(task_planner|risk_analyst) fails the global zero_proposal guard — so evaluate() correctly reports is_complete=False with architect in blocking_agents.

The diagnostics snapshot is also updated up-front (_plan_phase.py:186-191) with reviewer_exit_code: "<not-spawned: architect handoff broken>" and architect_handoff_broken: True, so the plan-placeholder rendering surfaces the failure mode rather than a half-populated verdict block.

Two subagent spawns avoided per fail-fast event (NB3). The downstream producers no longer see a dangling EGG_ARCHITECT_OUTPUT_PATH env var.

The new test test_plan_stage_fails_fast_when_architect_handoff_missing (integration_tests/regression/test_inprocess_plan_brc.py:919-999) pins all three NB1+NB3 invariants:

1. task_planner, risk_analyst, reviewer_plan are never spawned (asserts via _spawned_roles(bundle) & forbidden_after_n9).
2. The plan-HITL decision offers ["retry", "abort"], not the success-path approve_continue options.
3. tracker.evaluate() returns is_complete=False with "architect" in blocking_agents.

The pre-fix v2 code would have failed assertion 1 (downstream spawned) AND 2 (gate showed success options because optimistic-ACK clobbered the NACK). Solid regression pin.

NB2 — lock-file unlink removed entirely

orchestrator/substrate/in_process.py:583-589 (the unlink block) is gone. The docstring at in_process.py:527-536 now explains the tradeoff: a 0-byte sidecar per pipeline_id is far cheaper than the lost-update risk that flock + unlink introduced. Since open(lock_path, "w") only truncates (doesn't relink), all opens of the path resolve to the same inode, and LOCK_EX correctly serializes concurrent writers. The v1 N7 lock-accumulation concern was non-blocking and the docstring tradeoff is sound.

NB4 — flock deadline clamped to true upper bound

orchestrator/substrate/in_process.py:614-623:

deadline = time.monotonic() + self._FLOCK_TIMEOUT_SECONDS
while True:
    try:
        fcntl.flock(lock_fp.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
        return
    except BlockingIOError:
        remaining = deadline - time.monotonic()
        if remaining <= 0:
            raise
        time.sleep(min(self._FLOCK_RETRY_INTERVAL_SECONDS, remaining))

remaining is computed AFTER BlockingIOError, checked before sleeping, and the sleep is clamped to min(retry_interval, remaining). The documented _FLOCK_TIMEOUT_SECONDS=30.0 is now the strict upper bound. Worst-case overshoot is one syscall, not one retry interval.

Bonus catch: existing tests now actually exercise the happy path

_make_fake_bundle(tmp_path) previously used return_value=MagicMock(...) only, so the fake spawner never wrote producer output files — meaning every prior test silently triggered the v2 N9 NACK + optimistic-ACK clobber path instead of the BRC convergence path the test names claimed. The new side_effect=_spawn writes each producer's EGG_PRODUCER_OUTPUT_PATH JSON before returning, so the happy-path tests now actually exercise the happy path (and the new fail-fast test opts out with write_producer_outputs=False). This is a meaningful improvement to test coverage that came along with the NB1/NB3 fix.

Things that were checked and still look right

• B1 / B2 (phase plumbing through _write_pending_decision; SKILL.md approve_continue alignment): unchanged from v2, still correct.
• N1–N6, N10, N11: unchanged from v2, still correct.
• The N9 NACK now uses tracker.handle_nack(plan_reviewer.value, architect_role.value, ...) with a clear reason and artifact_references — the operator sees actionable diagnostics in the plan-HITL gate's unresolved_nack_details.
• Worktrees created for the architect on the N9 path are still cleaned up via the pipeline-level _teardown_worktrees() in the generator's finally block (in_process.py:254-261).
• _record_producer_propose correctly short-circuits on exit_code != 0, so the architect's proposal is only registered when its spawn succeeded — the architect_handoff_broken check then correctly distinguishes "spawn failed" (no NACK needed; downstream sees its own missing-input error) from "spawn succeeded but produced nothing" (N9 NACK + fail-fast).

— Authored by egg

[egg-reviewer]

v4: no functional delta since v3 approval; tree is byte-identical

Re-reviewed a300b1f0 against my v3 approval point 3a79c2a.

The PR head tree is identical to the approved tree. Verified via the GitHub commits API:

• 3a79c2a (v3 approval) tree → 8fb1b0ebad921f787a3534c222c6152c72d5f6c1
• a300b1f0 (current head) tree → 8fb1b0ebad921f787a3534c222c6152c72d5f6c1

The compare endpoint reports files_changed: 0 for 3a79c2a...a300b1f0. Two intervening commits, both zero-tree-delta against slice-2:

1. 33c66173 — [slice-1] Roll out Claude Code substrate ... (#2724). slice-1's content was already present in slice-2 (slice-2 was branched on top of slice-1's tip), so merging this contributes no new files to the slice-2 tree.
2. a300b1f0 — Merge egg/issue-2717/work into slice-2 (squash-merge history reconcile). Per the commit message, this is a deliberate -s ours merge to repair the DAG after #2724 squash-merged slice-1 into work (which dropped the shared-history link to slice-2's individual slice-1 commits and caused GitHub to flag #2726 as conflicting on retarget). The strategy records work as merged without altering slice-2's tree. Verified end-state: tree sha unchanged from v3.

No new code to review. My v3 approval at 3a79c2a stands for a300b1f0 unchanged.

Note: PR is already merged

PR is in MERGED state (afa34558 on main). Posting this as a comment for the audit trail only — there is nothing actionable here.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

11 previous review(s) hidden.