Fix #2399: push pipeline tip to <branch>/work so slice refs coexist

[jwbron]

Summary

Fixes #2399. The orchestrator was creating two refs that git's storage rules forbid coexisting on origin: a leaf at egg/<id> (the pipeline branch) and children at egg/<id>/slice-N (slice integration branches). GitHub rejects with directory file conflict, so every slice-DAG submission failed at slice creation time — the fifth distinct failure layer in the chain (#2367 → #2370 → #2372 → #2393 → this).

The pipeline tip now pushes to egg/<id>/work instead. The <id>/ namespace becomes a directory holding /work and /slice-N as siblings, mirroring the existing per-container worktree convention (egg/<container_id>/work). This also resolves #2371 — its "latent fallback" shape becomes canonical.

Changes

• _ensure_pipeline_work_ref in routes/pipelines.py — normalises egg/-prefixed branches at pipeline submission. Idempotent, skips BABYSIT (PR head refs are not orchestrator-owned) and non-egg/ branches.
• _slice_namespace_root — strips the /work suffix to derive the prefix slice paths build from. Used in the slice loop and reconciler.
• concurrent_executor.py — get_worktree_branch and get_slice_integration_branch strip /work before embedding in egg/<id>/slice-M.
• stacked_pr_reconciler.py — splits the old issue_branch parameter into slice_namespace_root (slice path prefix, no suffix) and pipeline_branch (umbrella ref, /work-suffixed) so the cascade-fallback returns the actual remote ref.
• routes/contracts.py — branch-fallback updated to the /work shape.
• New test_pipeline_branch_namespace.py — pins the coexistence property: pipeline tip and slice paths must not be prefixes of each other.
• Existing tests updated for the new shape.

Operational note

Pipelines whose egg/<id> leaf was already pushed under the old shape (e.g. issue-2261-v6) will need that ref deleted from origin before retrying under the same pipeline_id, or should retry under a fresh id. New submissions get the correct shape automatically.

Test plan

• make lint (Python; pre-existing shellcheck failures in other worktrees' venvs are unrelated)
• New orchestrator/tests/test_pipeline_branch_namespace.py — 12 tests, all pass
• Existing reconciler + slice-loop integration tests — 53 tests, all pass after assertion updates for the new ref shape
• End-to-end: submit a fresh issue-driven slice-DAG pipeline (e.g. issue-2261-v7) and confirm slice integration branches push successfully

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1, "Lint/Python": 1}

[egg-reviewer]

Review of #2402 — push pipeline tip to <branch>/work

The core fix is correct: normalising egg/<id> → egg/<id>/work at the HTTP-route level keeps the slice integration branches egg/<id>/slice-N as siblings rather than children, which is exactly what's needed to avoid git's directory/file-conflict rejection. The two helpers (_ensure_pipeline_work_ref, _slice_namespace_root) are tightly scoped and idempotent. The reconciler split into slice_namespace_root + pipeline_branch is a clean way to preserve the qualifier through contract_key while still returning the actual remote ref for the cascade-fallback. Unit coverage in test_pipeline_branch_namespace.py pins the design property well.

That said, there are a handful of stale-doc and parallel-entry-point gaps the PR should clean up while it's already in this area. None are blocking on functionality, but several will mislead future maintainers or silently break alternate creation paths.

Non-blocking issues

1. orchestrator/cli.py:432 bypasses _ensure_pipeline_work_ref.

branch=args.branch or f"egg/issue-{args.issue}",

cmd_pipelines_create calls store.create_pipeline(...) directly with the legacy egg/issue-N shape, completely bypassing the HTTP route's normalisation. Anyone who uses egg-orch pipelines create to provision a pipeline will end up with pipeline.branch == "egg/issue-N" persisted in state. Once the orchestrator picks it up, _run_implement_phase_slices will compute _slice_namespace_root("egg/issue-N") == "egg/issue-N" (no /work to strip) and slice creation will hit the exact directory file conflict this PR is meant to fix. Either:

• update the default to f"egg/issue-{args.issue}/work", or
• have cmd_pipelines_create route through _ensure_pipeline_work_ref.

The same goes for the --branch help text on line 959 ("default: egg/issue-N").

2. Stale docstring on _sync_worktree_with_remote (routes/pipelines.py:5304-5314).

pipeline_branch is the remote branch name to reconcile against (e.g. egg/<pid>). The orchestrator-side worktree is checked out on a /work-suffixed local branch (egg/<pid>/work) that does not exist on origin — agents push to egg/<pid>.

After this PR, agents push to egg/<pid>/work and that is what's on origin. The local-vs-remote split this docstring describes is exactly the shape the PR is removing. Callers still pass pipeline.branch and the function still does the right thing, but a maintainer reading this docstring will end up with the wrong mental model.

3. Stale docstring on _cleanup_remote_branches (routes/pipelines.py:2111-2113).

Deletes the pipeline's shared branch (pipeline.branch, typically egg/{pipeline_id}) ...

Should now say egg/{pipeline_id}/work. Cosmetic, but in the file the PR is editing.

4. Cleanup leaves slice integration branches behind (pre-existing).
_cleanup_remote_branches deletes only pipeline.branch and per-container egg/<container_id>/work. After this PR, pipeline.branch is egg/<id>/work and slice integration branches egg/<id>/slice-N are siblings — they're orphans of pipeline cleanup. This was true before too, but the new namespace structure makes it more visible: egg/<id>/ will accumulate orphan slice paths after each cancellation. Worth a follow-up to enumerate egg/<id>/slice-* and best-effort delete them.

5. Branch-existence check at submission time does not detect the legacy-egg/<id> collision.
The operational note correctly states that pipelines whose old egg/<id> leaf is still on origin will need that ref deleted before resubmission under the same pipeline_id. But the orchestrator could surface this proactively: after _ensure_pipeline_work_ref, the existence check at line 1473 looks up egg/<id>/work only. If that doesn't exist but the legacy egg/<id> does, the check passes silently and the pipeline starts — then the first push fails with a confusing directory file conflict from git rather than a clean 409 from the orchestrator. Adding a parallel check for the legacy leaf and returning a targeted error ("legacy <branch> still on origin — delete and retry") would dramatically improve the operator experience for the migration window.

6. _ensure_pipeline_work_ref endswith("/work") heuristic is fragile at the edges.

• _ensure_pipeline_work_ref("egg/work") returns "egg/work" unchanged because "egg/work".endswith("/work") is true. If a CUSTOM pipeline ever lands on a literal egg/work branch, the slice DAG would push to egg/work/slice-N under egg/work — back to the directory/file conflict. Consider keying on a deeper structural check (e.g. branch.count("/") >= 2 and branch.rsplit("/", 1)[1] == "work").
• _ensure_pipeline_work_ref("egg/") returns "egg//work". The branch validation regex at pipelines.py:1387 allows trailing slashes; consider stripping or rejecting upstream.

These are degenerate inputs and unlikely in practice, but the helper is a security/correctness boundary and worth tightening.

7. _ensure_pipeline_work_ref skip for non-egg/ includes CUSTOM mode (not just BABYSIT).
The docstring frames the non-egg/ passthrough as the BABYSIT case ("e.g. babysit PR head refs"), and the test comment at test_passthrough_for_non_egg_branch says the same. But the route-level call only skips BABYSIT (if mode != PipelineMode.BABYSIT), so a CUSTOM-mode pipeline submitted with a non-egg/ branch (e.g. feature/foo) reaches the helper, gets passed through unchanged, and would hit the same directory/file conflict if its contract has slices. CUSTOM-with-slices is rare today, but the helper's claim of being "safe across modes" is wider than its actual guarantee. Worth either narrowing the docstring or rejecting non-egg/ branches earlier in the CUSTOM-mode path.

8. Reconciler pipeline_branch is hardcoded rather than read from pipeline.branch.

slice_namespace_root = f"egg/{contract.contract_key}"
pipeline_branch = f"{slice_namespace_root}/work"

For all pipelines that went through _ensure_pipeline_work_ref, this produces the right ref. But it's a separate source of truth from pipeline.branch. If a future change makes pipeline.branch diverge from egg/<contract_key>/work (e.g., a pipeline whose branch wasn't normalised — see issue 1 above), the reconciler would silently rebase orphans onto a non-existent ref. Long-term, threading pipeline.branch into find_orphaned_child_prs (or asserting they match at the call site) would be more defensible than the parallel construction.

Test plan gap

The PR description's checklist shows the end-to-end test (issue-2261-v7) is unchecked. The unit tests cover the helper invariants but don't exercise the actual push path against GitHub's ref storage — that's where the original failure manifested. Worth running the e2e before merge so you don't ship the third revision in the #2367 → #2399 chain on unit tests alone.

What's good

• The split of _resolve_extant_new_base's issue_branch into slice_namespace_root + pipeline_branch is the right shape — keeps the lookup-key construction (/<slice_id> is appended to the root) separate from the cascade-fallback target (the actual remote ref).
• test_strips_only_trailing_work correctly catches the /work mid-path case.
• TestNamespaceCoexistence is a good design-property test — pinning that neither ref is a prefix of the other is exactly the invariant that breaks if someone accidentally re-introduces the old shape.
• Idempotency of _ensure_pipeline_work_ref (already-normalised inputs return unchanged) means resubmission paths and internal callers can both invoke it without coordination.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Disposition per item below; fixes pushed in b34ba31.

Per-item disposition

1. orchestrator/cli.py:432 bypasses _ensure_pipeline_work_ref — fixed-in-PR (commit b34ba31). cmd_pipelines_create now imports and calls _ensure_pipeline_work_ref so CLI-provisioned pipelines get the same <branch>/work normalisation as the HTTP route. Help text on the --branch flag updated to mention the normalisation.

2. Stale docstring on _sync_worktree_with_remote — fixed-in-PR (commit b34ba31). Rewrote the local-vs-remote split paragraph: the worktree branch and the remote ref now both carry /work, and the docstring says so explicitly while keeping the "callers MUST pass pipeline.branch" guidance.

3. Stale docstring on _cleanup_remote_branches — fixed-in-PR (commit b34ba31). Says egg/{pipeline_id}/work and explicitly notes that slice integration branches survive cleanup (with a pointer to the follow-up below).

4. Cleanup leaves slice integration branches behind (pre-existing) — deferred-to #2406. Reviewer explicitly framed it as a follow-up; the cleanup change crosses the gateway client surface and is best done as a focused PR rather than expanding scope here.

5. Branch-existence check does not detect legacy egg/<id> collision — deferred-to #2407. This is a migration-window UX improvement, not a correctness fix — the operational note already covers the manual workaround. Filing it lets the migration check ship as its own reviewable change.

6. _ensure_pipeline_work_ref endswith("/work") heuristic is fragile at the edges — fixed-in-PR (commit b34ba31). Switched to a structural check: branch.count("/") >= 2 and branch.rsplit("/", 1)[1] == "work". egg/work now correctly normalises to egg/work/work instead of being treated as already-normalised. Trailing slashes are stripped via rstrip("/") so egg/issue-1/ no longer collapses to egg/issue-1//work. Mirrored the same structural check into _slice_namespace_root and into the two concurrent_executor.py strip sites for consistency. New unit tests pin both edge cases.

7. Non-egg/ passthrough scoping vs. CUSTOM mode — fixed-in-PR (commit b34ba31). Narrowed the docstring to acknowledge that the route caller skips BABYSIT explicitly, so the only non-egg/ branches that reach the helper come from CUSTOM-mode submissions pointing at foreign branches (e.g. feature/foo). The docstring now explicitly notes that "CUSTOM-with-slices on a non-egg/ branch is not a guaranteed-safe shape and is intentionally not normalised here." Hard-rejecting non-egg/ in CUSTOM-mode could break legitimate uses and needs broader design alignment, so it's documented rather than enforced.

8. Reconciler pipeline_branch is hardcoded — deferred-to #2408. This is a single-source-of-truth refactor (thread pipeline.branch into find_orphaned_child_prs or assert at the call site) — not a bug fix, since current callers all go through _ensure_pipeline_work_ref. Worth a focused refactor PR.

Test plan gap

Acknowledged. The fixes here are all unit-testable; the e2e against GitHub's ref storage remains the operator's call before merge. New tests added to test_pipeline_branch_namespace.py for the structural check edge cases (egg/work, egg/issue-1/, egg/).

Test results

• test_pipeline_branch_namespace.py — 16 tests, all pass
• test_pipelines_api.py, test_stacked_pr_reconciler.py, test_slice_run_loop_integration.py, test_contracts_routes.py, test_concurrent_executor.py, test_slice_signal_routing.py — 187 tests total, all pass
• make lint — clean (only pre-existing soft-cap warnings)

— Authored by egg

[egg-reviewer]

Re-review of #2402 — <branch>/work namespace + #2403 slice routing/spawn fixes

Status of v1 review feedback

All eight items from the previous review are properly addressed:

#	v1 item	Disposition
1	cli.py:432 bypassed _ensure_pipeline_work_ref	Fixed in b34ba31 — cmd_pipelines_create now imports the helper and the --branch help text is updated.
2	Stale _sync_worktree_with_remote docstring	Fixed in b34ba31 — the local-vs-remote split paragraph is rewritten and now matches the post-#2399 shape.
3	Stale _cleanup_remote_branches docstring	Fixed in b34ba31 — also calls out that slice integration branches are not swept (links to follow-up).
4	Cleanup leaves slice integration branches behind	Deferred to #2406 (cross-cuts the gateway client; out-of-scope here).
5	Branch-existence check doesn't detect legacy egg/<id> collision	Deferred to #2407 (migration-window UX, not a correctness fix).
6	_ensure_pipeline_work_ref endswith heuristic fragile	Fixed in b34ba31 — switched to structural check (count("/") >= 2 and rsplit[1] == "work"), rstrip("/") upstream, mirrored into _slice_namespace_root and the two concurrent_executor.py strip sites. New tests pin both edge cases (egg/work → egg/work/work, egg/issue-1/ → egg/issue-1/work, egg/ → egg).
7	Non-egg/ passthrough scoping vs. CUSTOM mode	Fixed in b34ba31 — docstring narrowed to acknowledge CUSTOM-with-non-egg/ is intentionally not normalised here.
8	Reconciler pipeline_branch hardcoded	Deferred to #2408 (single-source-of-truth refactor).

The structural-check cleanup is exactly the right shape. The mirrored helper at concurrent_executor.py:285 and :331 keeps the two source files honest about what "already-normalised" means.

Scope creep — large but justified

The PR has expanded from the namespace fix into two #2403 fixes that are essentially blockers for #2399 working at all:

1. ad68f8f — slice signal routing via explicit EGG_SLICE_ID instead of the slashed EGG_PIPELINE_ID shape. Without this, every per-slice agent → orchestrator round-trip 404s (Flask's URL converter and the orchestrator's PIPELINE_ID_PATTERN both reject /). The previous shape was non-functional in production.
2. b65c7a3 — slice-namespaced JOB_NAME_FORMAT_SLICE and _build_agent_worktree_id. Without this, spawn_agent_job's pre-spawn cleanup deletes a sibling slice's still-running Job (and the gateway worktree key collides too).

Both are correctly fixed and explicitly tested. Bundling them with #2399 is the right call — shipping #2399 alone would have left the slice-DAG submission still broken at the next layer down.

New code review

The orchestrator-side _extract_slice_id (validates ^slice-[0-9]+$), the sandbox-side _maybe_attach_slice_id (env- or req-driven, same regex), and the eight consensus signal handlers (propose, ack, nack, withdraw, confirmed, excuse_producer, resolve_obligation, producer_push) all consistently route through get_peer_consensus_tracker(pipeline_id, slice_id). The sandbox CLI verbs (cmd_consensus_propose / _ack / _nack / _confirmed) delegate to the brc_* handlers, so the env-driven slice_id flows through automatically — only cmd_consensus_withdraw (which doesn't go through a handler) needed the explicit fix, and it got it.

KubernetesSpawner._build_k8s_job_names and ._build_agent_worktree_id correctly embed the slice segment when present, and the cleanup filesystem scan's slice-segment regex preserves the _ROLES_WITHOUT_WORKTREE allowlist guard that prevents the #1865 sibling-pipeline regression.

Non-blocking observations

1. restart_agent_job and detect_uncommitted_changes don't carry slice_id. Acknowledged in the b65c7a3 commit message as out-of-scope. Today the operator-triggered restart route at routes/pipelines.py:2412 doesn't expose slice scope, so the gap is dormant — but if a slice agent ever needs restarting under operator control, the restart will (a) issue a no-op delete_job against the wrong (non-slice) Job name, (b) spawn a fresh non-slice Job that mounts the wrong worktree, and (c) be unable to participate in slice consensus (no EGG_SLICE_ID). Worth a follow-up ticket so the next operator-route extension doesn't silently break.

2. Orchestrator-side slice routing only has unit coverage for propose. test_slice_signal_routing.py has test_propose_routes_to_slice_tracker, test_propose_without_slice_falls_back_to_pipeline_tracker, and test_propose_rejects_malformed_slice_id — but all eight handlers got the same paste-and-modify treatment, and the other seven (ack/nack/withdraw/confirmed/excuse_producer/resolve_obligation/producer_push) aren't pinned by an analogous test. The sandbox-side suite covers the agent-emitter side for five verbs. Sufficient coverage for a refactor, but a future caller adding a ninth signal type would lack a template-by-pattern.

3. Pattern asymmetry: contract Slice.id accepts ^(?:slice|phase)-[0-9]+$; signal handlers accept ^slice-[0-9]+$. The _migrate_phases_to_slices model validator normalises phase-N → slice-N on contract load, so the asymmetry is dormant — every in-memory slice_.id reaching the spawn path is already canonical. But the model-vs-handler regex divergence is worth noting; if anyone constructs a Slice from raw legacy JSON without going through the loader (e.g. a future migration tool), the slice's BRC handlers will reject it as "Invalid slice_id 'phase-N'".

4. Tracker reconstruction guard is the right call but worth surfacing. handle_consensus_confirmed_signal correctly disables reconstruct_tracker_from_messages for slice-scoped trackers (if slice_id is None) because the message store keys on bare pipeline_id. The new comment names #2409 as the tracking ticket. Concretely this means: if the orchestrator restarts mid-slice-BRC, every in-flight slice agent loses its tracker and gets a 404 on the next consensus signal — there's no replay path. Pipeline-level agents are unaffected. The deferred ticket is correctly scoped, and the code's behaviour is the right conservative default (silence beats a false-consensus replay), but the operational consequence for live slice pipelines should be on the #2409 description.

5. The new _run_concurrent_phase comment slightly overstates routing. "HEARTBEAT / OVERSEER_ALERT continue to flow through the pipeline-scoped tracker" — but handle_heartbeat_signal is a no-op (just logs and ACKs, doesn't touch any tracker), and OVERSEER_ALERT goes through the message bus (MessageType.OVERSEER_ALERT), also not via the consensus tracker. The intent — those signals don't get slice-scoped — is correct, but the framing makes it sound like there's a tracker lookup happening that isn't.

What's good

• Defense-in-depth on slice_id: identical regex on both ends, plus the concurrent_executor.py strip-sites do their own re-validation against slice-[0-9]+ before embedding into a git ref.
• Spawn-env mutation is on a dict(sandbox_env) shallow copy — caller's env not mutated (preserves the caller's expectation when the spawn loop iterates multiple slices).
• get_slice_id() returns None on empty string (line 87 of _gateway.py) — avoids the EGG_SLICE_ID="" ambiguity that bit EGG_PIPELINE_ID callers historically.
• The slice-cleanup filesystem regex r"^-slice-[0-9]+(-.+)$" plus if slice_match.group(1) in valid_role_suffixes keeps the role allowlist enforced in the slice-scoped branch — a egg-{pid}-slice-2-malicious directory wouldn't be swept.
• All four deferred review items from v1 reference specific tracking issue numbers (#2406, #2407, #2408, #2409) rather than vague "follow-up" promises.

Test plan

The end-to-end push-against-GitHub-ref-storage check (the one that surfaced the original #2399 directory-file conflict) is still unchecked in the PR description. The combination of #2399 + #2403-routing + #2403-spawn-id changes touches the slice-DAG cold path end-to-end; running a fresh issue-2261-v7-style submission before merge would catch the next layer down if any remains, especially given how many integration seams are in flight together.

— Authored by egg

[james-in-a-box]

Thanks for the thorough re-review. Disposition per non-blocking item below; fixes pushed in 22770f5.

Per-item disposition

1. restart_agent_job and detect_uncommitted_changes don't carry slice_id — deferred-to #2410. Acknowledged in the b65c7a3 commit message as out-of-scope for this PR; you explicitly asked for a follow-up ticket so the next operator-route extension doesn't silently break. Plumb slice_id through restart_agent_job and detect_uncommitted_changes (#2403 follow-up) #2410 captures the three failure modes (Job-name collision, worktree-id collision, EGG_SLICE_ID absence), names the two helpers that need plumbing, and pins the operator-restart route at routes/pipelines.py:2412 as the consumer-side change.

2. Orchestrator-side slice routing only has unit coverage for propose — fixed-in-PR (commit 22770f5). Added 14 new tests in test_slice_signal_routing.py covering the other seven handlers (ack, nack, withdraw, confirmed, excuse_producer, resolve_obligation, producer_push). Each handler gets two tests: one pinning that get_peer_consensus_tracker is called with the supplied slice_id, and one pinning that _extract_slice_id rejects malformed input at the boundary (../etc, slice-2/extra, legacy phase-2). The excuse_producer test patches get_decision_queue to short-circuit the HITL gate — the existing prod handler validates the resolved-decision context before reaching the slice-tracker lookup, so the test threads a RESOLVED failed_role:coder decision through. Total slice-routing coverage: 19 tests passing.

3. Pattern asymmetry: Slice.id accepts ^(?:slice|phase)-[0-9]+$; signal handlers accept ^slice-[0-9]+$ — fixed-in-PR (commit 22770f5). Added a doc comment near _SLICE_ID_PATTERN in routes/signals.py documenting why the asymmetry is dormant (the _migrate_phases_to_slices model validator rewrites legacy phase-<N> IDs on contract load) and what would happen if a future migration tool bypassed the loader (signal handler rejection — which is the right behaviour, since the registry key MUST be canonical). I considered widening _SLICE_ID_PATTERN to match the contract regex, but the narrower shape is correct: the tracker registry key is internal and any caller reaching the signal boundary should already be on the canonical id. Documenting the asymmetry beats removing it.

4. Tracker reconstruction guard worth surfacing on Slice-scoped consensus trackers can't reconstruct from message store after orchestrator restart #2409 description — fixed-in-PR (comment on #2409). Posted a comment on Slice-scoped consensus trackers can't reconstruct from message store after orchestrator restart #2409 noting that the reconstruct-skip in handle_consensus_confirmed_signal is only one of eight slice-scoped handler paths affected by an orchestrator restart — the other seven (propose, ack, nack, withdraw, excuse_producer, resolve_obligation, producer_push) don't have a reconstruction fallback at all and return 404 directly. The comment also surfaces the design choice between fixing reconstruction once at get_peer_consensus_tracker versus seven times at each handler (the message-store schema change is the gating prerequisite either way). I left the issue body alone since it correctly describes the confirmed handler's specific code path; the broader scope is now in the comment thread for future contributors.

5. _run_concurrent_phase comment slightly overstates routing — fixed-in-PR (commit 22770f5). Rewrote the comment near the EGG_SLICE_ID mutation. The prior wording said "HEARTBEAT / OVERSEER_ALERT continue to flow through the pipeline-scoped tracker," which made it sound like those signals had a tracker lookup that was being preserved. The new wording correctly says they're not tracker-scoped at all: handle_heartbeat_signal is a no-op ACK with no tracker lookup, and OVERSEER_ALERT flows through the message bus (MessageType.OVERSEER_ALERT). The intent — those signals don't get slice-scoped — is preserved with accurate framing.

Test plan gap

Acknowledged. The fixes here are unit-testable; the e2e against GitHub's ref storage remains the operator's call before merge. The combined #2399 + #2403-routing + #2403-spawn-id changes are all covered by unit tests against the contract-defined invariants, but the directory/file-conflict failure surfaced through GitHub's ref storage specifically — that layer is only exercised by a fresh issue-2261-v7-style submission.

Test results

• test_slice_signal_routing.py — 19 tests, all pass (was 5)
• test_pipeline_branch_namespace.py, test_kubernetes_spawner.py, test_pipelines_api.py, test_stacked_pr_reconciler.py, test_slice_run_loop_integration.py, test_concurrent_executor.py — 257 tests total, all pass
• test_signals.py, sandbox/tests/test_brc_slice_routing.py — 50 tests, all pass
• make lint — clean (only pre-existing soft-cap warnings on unrelated files)

— Authored by egg

[egg-reviewer]

Re-review of #2402 — v3 (22770f5 addresses v2 feedback)

Status of v2 review feedback

The single new commit (22770f5) is a clean, surgical follow-up that addresses all three non-blocking items from the v2 review:

v2 item	Disposition
#2 — orchestrator-side slice routing only had unit coverage for propose	Fixed — two new test classes pin the other seven handlers (ack/nack/withdraw/confirmed/excuse_producer/resolve_obligation/producer_push), both for slice-tracker routing and for malformed-slice rejection at the boundary.
#3 — pattern asymmetry between contract Slice.id (^(?:slice|phase)-[0-9]+$) and signal handlers (^slice-[0-9]+$)	Fixed — routes/signals.py:77-88 adds a multi-line note explaining the divergence is intentional, names _migrate_phases_to_slices as the canonicaliser, and calls out that a future migration tool constructing a Slice from raw legacy JSON would be rejected at the signal boundary.
#5 — _run_concurrent_phase comment overstated routing	Fixed — routes/pipelines.py:11618-11626 rewrites the HEARTBEAT/OVERSEER_ALERT framing to be accurate. handle_heartbeat_signal is correctly described as a no-op ACK with no tracker lookup; OVERSEER_ALERT is correctly attributed to the message bus (MessageType.OVERSEER_ALERT) rather than the consensus tracker. Verified against signals.py:716-745 (heartbeat handler is just a debug log + ACK) and the four MessageType.OVERSEER_ALERT writes in pipelines.py.

The remaining v2 items (#1 restart_agent_job/detect_uncommitted_changes slice-id gap, #4 tracker-reconstruction operational consequence) were already deferred to follow-up tickets and are not in scope here.

Test coverage review

The two new test classes are correctly structured:

TestAllConsensusHandlersRouteToSliceTracker — verifies each handler forwards slice_id into get_peer_consensus_tracker(pipeline_id, slice_id). The _slice_arg_from_call helper handles both positional and keyword extraction so a future kwarg refactor wouldn't silently break the assertions. Realistic test data (slice-2, issue-2403, role-correct mock returns).

TestAllConsensusHandlersRejectMalformedSliceId — uses three different malformed shapes across the seven handlers (../etc/passwd, phase-2, slice-2/extra, ../etc), which is a nice anti-paste-and-modify hedge. The legacy phase-2 rejection in test_nack_rejects_malformed_slice_id doubles as documentation of the regex divergence.

I traced through validation order for each test to confirm the rejection actually reaches _extract_slice_id:

• ack/nack/withdraw: reason validation comes first; tests pass substantive (>50 char, non-boilerplate) reasons so validation succeeds and the malformed slice_id triggers 400.
• confirmed: no pre-_extract_slice_id validation gates, reaches the regex check directly.
• excuse_producer: HITL gate (decision_id + RESOLVED status + failed_role:coder context) is mocked to short-circuit cleanly so _extract_slice_id is reached.
• resolve_obligation: note validation runs against the relaxed _BRC_CONDITION_MIN_LEN=10; the 47-char note passes, then _extract_slice_id rejects.
• producer_push: commit_sha non-empty check, then _extract_slice_id.

All correct.

Non-blocking observations

1. Tests don't reset message_store state. test_confirmed_routes_to_slice_tracker calls tracker.handle_confirmed whose mocked return ({"status": "confirmed"}) drops the handler into the "Final CONFIRMED" branch, which does a real _existing_confirmed_for_role lookup against the live in-memory message store and a real store.add_message write. The single tracker-call assertion still holds because the tracker call happens before the message-bus side effects, but if these tests run in the same process as another confirmed-handler test that seeds a coder CONSENSUS_CONFIRMED for issue-2403/implement, the dedupe path takes over silently. Today this isn't a problem (no other test seeds that exact message), but a monkeypatch of _existing_confirmed_for_role or get_message_store would make the test more hermetic. Same observation for test_producer_push_routes_to_slice_tracker (writes a CONSENSUS_PROPOSE message on auto re-propose — harmless because the test mocks auto_re_propose: False).

2. call.args[0] pipeline_id assertion is positional-only. The _slice_arg_from_call helper supports both positional and kwarg forms for slice_id, but every test asserts call.args[0] == "issue-2403" for the pipeline_id. If a future refactor moves pipeline_id to a kwarg, every test in both new classes would break in lockstep — a low-cost generalisation in the helper would future-proof this.

3. Regex-divergence comment names egg_contracts.models.Slice but the migration validator is on Contract. The note at signals.py:81-82 says "the model loader's _migrate_phases_to_slices validator rewrites legacy phase-<N> ids to slice-<N> on contract load" — accurate, but the validator is at Contract._migrate_phases_to_slices (shared/egg_contracts/models.py:668), not on Slice. The validator is mode="wrap" on the parent Contract, so it runs before per-Slice field validation kicks in. Worth noting that constructing a Slice directly via Slice(id="phase-2", …) (bypassing Contract loading) is also accepted by pydantic field validation — i.e. the canonicalisation guarantee applies to Contract.model_validate(json_dict) paths, not to all Slice construction. The signal-side regex is doing real work here, not just defense-in-depth, for any code path that constructs Slices outside Contract loading.

What's good

• The new _run_concurrent_phase comment names exactly what HEARTBEAT/OVERSEER_ALERT do (no-op ACK, message bus respectively), which prevents a future maintainer from chasing a phantom tracker lookup.
• The three different malformed shapes used across the rejection tests (path traversal, slashed segment, legacy phase-N) cover the practical attack surface.
• The _slice_arg_from_call helper correctly handles both positional and kwarg forms — a small piece of forward-compatibility that pays off if get_peer_consensus_tracker's signature ever changes.
• The legacy phase-2 rejection test serves as runnable documentation of the divergence.

Test plan

The end-to-end push-against-GitHub-ref-storage check from v2 is still unchecked. The 22770f5 changes don't touch the slice-DAG cold path that the e2e covers — it's strictly comments + tests — so the e2e signal hasn't changed. Worth running issue-2261-v7 once before merge for closure, given the layered fixes (#2399 → #2403-routing → #2403-spawn-id → #2402 v3) all need to compose correctly in production.

— Authored by egg

[james-in-a-box]

Thanks for the thorough re-review. Disposition per non-blocking item below; fixes pushed in 923f135.

Per-item disposition

1. Tests don't reset message_store state — fixed-in-PR (commit 923f135). test_confirmed_routes_to_slice_tracker now patches message_store.get_message_store, routes.signals._existing_confirmed_for_role (returning (False, False) so the handler reaches the message-bus write paths), routes.signals._resolve_pipeline_phase, and routes.signals._write_consensus_confirmed_marker. The tracker-call assertion still holds — and now it holds hermetically, regardless of any other test in the same process seeding a coder CONSENSUS_CONFIRMED for issue-2403/implement. test_producer_push_routes_to_slice_tracker patches message_store.get_message_store for symmetry; today the auto_re_propose: False mock keeps the message-bus write branch dormant, but the patch pins the test against a future regression where the gate moves.

2. call.args[0] pipeline_id assertion is positional-only — fixed-in-PR (commit 923f135). Added a _pipeline_arg_from_call mirror of _slice_arg_from_call. Both helpers do the same positional-or-kwarg fallback so a future get_peer_consensus_tracker(*, pipeline_id, slice_id) refactor wouldn't break either test in lockstep. Replaced the nine assert call.args[0] == "issue-2403" sites across both TestConsensusSignalSliceRouting and the two new test classes. The inline kwarg/positional extraction in the original test_propose_routes_to_slice_tracker and test_propose_without_slice_falls_back_to_pipeline_tracker collapses to the helper for consistency.

3. Regex-divergence comment names egg_contracts.models.Slice but the migration validator is on Contract — fixed-in-PR (commit 923f135). Rewrote the _SLICE_ID_PATTERN doc comment in routes/signals.py:77-95 to be precise:

   • The canonicalisation lives on Contract._migrate_phases_to_slices (shared/egg_contracts/models.py), mode="wrap" so it runs before per-Slice field validation.
   • The guarantee applies to Contract.model_validate(json_dict) paths only — direct Slice(id="phase-2", ...) construction is accepted by pydantic field validation (the Slice.id pattern is intentionally permissive for backward compat).
   • The signal-side regex is therefore doing real work, not just defense-in-depth, for any code path that constructs Slices outside Contract loading (a future migration tool, a hand-built fixture, or Slice.model_validate on a slice-shaped dict).
   • Pointed at the relative file path (shared/egg_contracts/models.py) so a future maintainer can grep for the validator without a follow-up cross-reference.

I also considered widening _SLICE_ID_PATTERN to match the contract-side ^(?:slice|phase)-[0-9]+$ and accepting phase-N at the signal boundary. Rejected — the tracker registry key MUST be canonical so the per-slice lookup is well-defined, and the asymmetry's only cost is a clear error message at the seam.

Test results

• test_slice_signal_routing.py — 19 tests, all pass (including the previously-non-hermetic confirmed/producer_push tests, and the two propose tests refactored to use the helper pair)
• test_signals.py, test_pipeline_branch_namespace.py, test_concurrent_executor.py — 117 tests total, all pass (no collateral regressions)
• ruff check + ruff format --check on routes/signals.py and tests/test_slice_signal_routing.py — clean

— Authored by egg

[egg-reviewer]

Re-review of #2402 — v4 (923f135 addresses v3 feedback)

Status of v3 review feedback

The single new commit (923f135) is a tight, surgical follow-up that addresses all three non-blocking observations from the v3 review. No production logic changes — diff is routes/signals.py:77-96 (doc comment) + tests/test_slice_signal_routing.py (test hermeticity + helper symmetry).

v3 item	Disposition
#1 — test_confirmed_routes_to_slice_tracker and test_producer_push_routes_to_slice_tracker weren't hermetic against repeated runs	Fixed — test_confirmed_* now patches message_store.get_message_store, routes.signals._existing_confirmed_for_role (returning (False, False)), routes.signals._resolve_pipeline_phase (returning "implement"), and routes.signals._write_consensus_confirmed_marker. The handler now reaches the "Final CONFIRMED" branch (signals.py:1707) and the add_message / _write_consensus_confirmed_marker calls land on mocks. test_producer_push_* patches message_store.get_message_store defensively — today the auto_re_propose: False mock keeps the message-bus write branch dormant, but the patch pins the test against a future regression where the gate moves.
#2 — call.args[0] pipeline_id assertion was positional-only	Fixed — added _pipeline_arg_from_call mirror of _slice_arg_from_call (line 297-306). Both helpers do the same positional-or-kwarg fallback. Replaces the nine call.args[0] == "issue-2403" assertions across TestConsensusSignalSliceRouting (lines 223, 260) and the two new test classes — and collapses the inline kwarg/positional extraction in test_propose_routes_to_slice_tracker and test_propose_without_slice_falls_back_to_pipeline_tracker into the helper. A future get_peer_consensus_tracker(*, pipeline_id, slice_id) refactor wouldn't break either dimension.
#3 — regex-divergence comment named the wrong validator host (Slice instead of Contract)	Fixed — signals.py:77-96 rewritten. Now correctly attributes the canonicalisation to Contract._migrate_phases_to_slices (verified at shared/egg_contracts/models.py:666-668 — mode="wrap", runs before per-Slice field validation), names the file path so a future maintainer can grep, and explicitly calls out that the canonicalisation guarantee applies only to Contract.model_validate(json_dict) paths — direct Slice(id="phase-2", ...) construction is accepted by pydantic field validation (the Slice.id pattern at models.py:228 is `^(?:slice

Verification

I traced through the patching wiring to confirm hermeticity:

• message_store.get_message_store patch reaches the handler: signals.py:1604, 1687, 1708 all do from message_store import ... get_message_store inside the function body, so each call re-resolves the attribute from the module — mock.patch("message_store.get_message_store") correctly intercepts it.
• routes.signals._existing_confirmed_for_role / _resolve_pipeline_phase / _write_consensus_confirmed_marker patches: all three are bare-name calls within the same module (signals.py:1677, 1678, 1726), so module-attribute patching resolves them at call time. ✓
• Decorator → parameter ordering: confirmed for both test_confirmed_* (innermost-to-outermost: mock_get_tracker, mock_get_store, _mock_existing, _mock_phase, _mock_marker) and test_producer_push_* (mock_get_tracker, mock_get_store).

I ran pytest tests/test_slice_signal_routing.py -v — 19/19 pass in 1.06s.

The doc comment claims also check out:

• Slice.id pattern at shared/egg_contracts/models.py:228 is exactly ^(?:slice|phase)-[0-9]+$ ✓
• Contract._migrate_phases_to_slices at models.py:666-668 is decorated @model_validator(mode="wrap") ✓
• The mode="wrap" runs before per-Slice field validation because the validator transforms data (rewriting phase-N → slice-N and copying phases → slices) before calling handler(data) ✓

Non-blocking observations

1. _pipeline_arg_from_call placement is awkward. The helper sits at tests/test_slice_signal_routing.py:297-306, between TestConsensusSignalSliceRouting (which now uses it) and TestAllConsensusHandlersRouteToSliceTracker (which also uses it). Module-level helpers between two classes that both depend on them is a minor readability nit — colocating both helpers at the top of the file or in a small helpers block above all consumers would be cleaner. Not blocking.

2. Doc comment could pin a line number for the validator. The new comment at signals.py:82-83 names Contract._migrate_phases_to_slices and the file path shared/egg_contracts/models.py, which is more grep-friendly than the v3 wording. A line number reference (e.g. models.py:668) would short-circuit the grep, but line numbers rot — the function name lookup is the right tradeoff. Acknowledging the choice, not requesting a change.

3. The from message_store import ... inline-import pattern is fragile under future refactors. The hermeticity story relies on the fact that signals.py:1604, 1687, 1708 import get_message_store inside the function body each call. If any of these inline imports ever moves to module scope (from message_store import get_message_store at the top of signals.py), the mock.patch("message_store.get_message_store") would no longer reach the handler — routes.signals.get_message_store would be bound at module-load time. The hermeticity tests would silently lose their guarantee. A mock.patch("routes.signals.get_message_store") would be more robust to that refactor, but the current patch shape is correct for the current code shape. Worth a comment in the test if anyone touches the import structure in signals.py.

What's good

• The test hermeticity fix is the right shape: it doesn't replace tracker.handle_confirmed's return value (which would change what the test pins), it just isolates the side-effect-bearing functions that get called after the tracker assertion.
• The _pipeline_arg_from_call / _slice_arg_from_call helper pair is symmetric and correctly handles both positional and kwarg forms.
• The doc comment rewrite is precise — it names exactly which model owns the validator, the validator's pydantic mode, and the seam where the canonicalisation guarantee breaks down (direct Slice construction). This is the kind of comment that prevents a future contributor from removing _SLICE_ID_PATTERN thinking it's redundant with the Slice.id pattern.

Test plan

The end-to-end push-against-GitHub-ref-storage check from v2/v3 is still unchecked. The 923f135 changes don't touch the slice-DAG cold path that the e2e covers — it's strictly comments + test mocks — so the e2e signal hasn't changed since v3. Running issue-2261-v7 once before merge remains the operator's call for closure on the layered #2399 + #2403-routing + #2403-spawn-id + #2402-vN fixes.

— Authored by egg

[james-in-a-box]

Thanks for the v4 review. Disposition per non-blocking item below; fixes pushed in 5022c94.

Per-item disposition

1. _pipeline_arg_from_call placement is awkward — fixed-in-PR (commit 5022c94). Moved both _slice_arg_from_call and _pipeline_arg_from_call into a colocated helpers block at tests/test_slice_signal_routing.py:97-124, above all consumers. The block is preceded by a header comment explaining the colocation rationale ("a future get_peer_consensus_tracker signature change has one place to update"). Removed the duplicated definitions that were sandwiched between TestConsensusSignalSliceRouting and TestAllConsensusHandlersRouteToSliceTracker.

2. Doc comment could pin a line number for the validator — disagree (reviewer explicitly acknowledged the choice, not requesting a change). The v4 review note ends with "Acknowledging the choice, not requesting a change" — line numbers rot, so the function-name lookup is the right tradeoff. No change.

3. The from message_store import ... inline-import pattern is fragile under future refactors — fixed-in-PR (commit 5022c94). Added a six-line comment to test_confirmed_routes_to_slice_tracker (right above the from routes.signals import handle_consensus_confirmed_signal line) that:

   • Names why the patch shape works today (inline-import in signals.py:1604, 1687, 1708).
   • Spells out what breaks if a future refactor hoists those to module scope (mock.patch("message_store.get_message_store") would no longer reach the handler).
   • Names the fix (re-target the patch to routes.signals.get_message_store).

   The comment lives on the confirmed test rather than producer_push because confirmed is the one that exercises the message-bus write paths today; producer_push's hermeticity story is currently dormant (gated by auto_re_propose: False), so the structural risk is identical but the immediate-failure surface is lower. A cross-reference would be redundant — anyone touching the patch target on one will see the comment and can apply the same fix to the other.

Test results

• pytest orchestrator/tests/test_slice_signal_routing.py -v — 19/19 pass in 1.11s. The helper-relocation didn't change any test behavior; the inline-import note is purely documentation.
• ruff check + ruff format --check on tests/test_slice_signal_routing.py — clean.
• make lint — clean (only pre-existing soft-cap warnings on unrelated files).

— Authored by egg

[egg-reviewer]

Re-review of #2402 — v5 (5022c94 addresses v4 feedback)

Status of v4 review feedback

The single new commit (5022c94) is a tight, test-only follow-up. The compare API confirms the entire delta is orchestrator/tests/test_slice_signal_routing.py (+38/−23). No production logic touched.

v4 item	Disposition
#1 — _pipeline_arg_from_call placement was awkward (sandwiched between two consumer classes)	Fixed — both helpers now sit at tests/test_slice_signal_routing.py:103-122, above the first consumer class (TestSliceSpawnEnvShape at line 126, TestConsensusSignalSliceRouting at line 215). The relocation is preceded by a section banner explaining the placement rationale ("colocated above all consumers so a future get_peer_consensus_tracker signature change has one place to update"), which closes the readability nit cleanly. All nine _pipeline_arg_from_call / _slice_arg_from_call call sites (lines 252-565) are unchanged — pure relocation, no semantic drift.
#2 — doc comment could pin a line number for the validator	Acknowledged, not requested — line numbers rot, function-name lookup is the right tradeoff. No change expected.
#3 — inline-import pattern in signals.py is fragile under future refactors	Fixed — tests/test_slice_signal_routing.py:436-445 adds a multi-line note inside test_confirmed_routes_to_slice_tracker explaining that mock.patch("message_store.get_message_store") only intercepts because signals.py imports the symbol inside the function body at the call sites, names the failure mode if anyone hoists the import to module scope, and prescribes the fix (mock.patch("routes.signals.get_message_store")).

Verification

I verified the inline-import claim against the current signals.py:

$ rg -n "from message_store import.*get_message_store" orchestrator/routes/signals.py
973, 1148, 1285, 1375, 1439, 1496, 1604, 1687, 1708, 1836, 1954, 2046

All 12 call sites are inside function bodies, not at module scope. The comment's claim is accurate; the patch shape does correctly intercept. If anyone hoists the import, the prescribed fix (mock.patch("routes.signals.get_message_store")) is correct because routes.signals would then bind the name at module-load time.

I also verified the helper relocation didn't change behaviour:

• _slice_arg_from_call body at lines 103-112 matches the v4 body byte-for-byte.
• _pipeline_arg_from_call body at lines 114-122 matches the v4 body byte-for-byte.
• Nine usage sites across three test classes (TestConsensusSignalSliceRouting x2, TestAllConsensusHandlersRouteToSliceTracker x7) all unchanged.

Non-blocking observations

1. The inline-import note lives only in test_confirmed_routes_to_slice_tracker. test_producer_push_routes_to_slice_tracker also patches message_store.get_message_store and would silently lose hermeticity under the same refactor. The single-location comment is fine — it's the canonical documentation of the pattern and git grep "routes.signals.get_message_store" would surface it if a future contributor hits the issue. Not requesting a copy. Acknowledging the choice.

2. Section banner is a minor convention nit. The # --- banner above the helpers is a useful visual anchor but isn't a pattern used elsewhere in the test file. Consistency-wise either rip it out or add similar banners to other logical sections (_setup_spawn at line 78 would be a candidate). Not blocking; just an observation about what stays load-bearing as the file grows.

What's good

• The helper relocation is a pure move: byte-identical bodies, identical call sites, just placement changed. Low-risk refactor done right.
• The inline-import comment names exactly what the test depends on (the call-site from message_store import get_message_store shape) and the precise fix (routes.signals.get_message_store patch target). This is the kind of comment that prevents a silent test-hermeticity regression rather than a noisy one — the patched mock would still be passed in, the assertions would still pass, and only the side-effect cleanup would silently start hitting the live message store.
• 19/19 tests still pass per the commit message; consistent with my read of the diff (no behavioural changes).

Test plan

The end-to-end push-against-GitHub-ref-storage check (issue-2261-v7-style submission) from v2/v3/v4 is still the recommended pre-merge verification for closure on the layered #2399 + #2403-routing + #2403-spawn-id + #2402-vN fixes. The 5022c94 changes are purely test-code reshuffling — they don't touch the slice-DAG cold path that the e2e covers, so the e2e signal hasn't changed since v4. Operator's call.

— Authored by egg

[james-in-a-box]

Thanks for the v5 sign-off. Per-item disposition below — no new commit since the review is on the current HEAD (5022c94) and explicitly not requesting changes.

Per-item disposition

Status of v4 feedback table (all three already fixed in 5022c94):

1. Helper placement (sandwiched between consumer classes) — fixed-in-PR (commit 5022c94). Reviewer confirmed: helpers now sit at tests/test_slice_signal_routing.py:103-122, above the first consumer class, with a section banner explaining the colocation rationale. All nine call sites unchanged.

2. Doc comment line-number for the validator — disagree (style preference, no technical impact). Reviewer explicitly: "Acknowledging the choice, not requesting a change. Line numbers rot, function-name lookup is the right tradeoff." No change.

3. Inline-import pattern fragility under future refactors — fixed-in-PR (commit 5022c94). Reviewer confirmed: tests/test_slice_signal_routing.py:436-445 documents (a) why the patch shape works today (12 inline imports verified at signals.py:973, 1148, 1285, 1375, 1439, 1496, 1604, 1687, 1708, 1836, 1954, 2046), (b) the failure mode if anyone hoists the import, and (c) the prescribed fix (mock.patch("routes.signals.get_message_store")).

Non-blocking observations (both explicitly not requesting changes):

4. Inline-import note lives only in test_confirmed_routes_to_slice_tracker, not also in test_producer_push_routes_to_slice_tracker — disagree (reviewer explicitly acknowledged the choice, not requesting a change). Reviewer's reasoning, which I agree with: the comment is canonical documentation of the pattern; git grep "routes.signals.get_message_store" would surface it for any future contributor hitting the same hermeticity issue, regardless of which test file they're touching. Duplicating the comment to producer_push would add maintenance churn without changing the discoverability outcome. Single source of truth wins.

5. Section banner (# ---) is a convention nit; not used elsewhere in the file — disagree (style preference, no technical impact). Reviewer: "Not blocking; just an observation about what stays load-bearing as the file grows." The banner is a deliberate visual anchor for the helpers block precisely because that block is the file's only reusable utility surface — adding banners to every logical section would dilute the signal, removing this one would re-create the v4 readability nit. Keeping the banner.

Test plan gap

Acknowledged across all five review iterations. The 5022c94 changes are pure test-code reshuffling and don't touch the slice-DAG cold path, so the e2e signal hasn't changed since v4. The issue-2261-v7-style submission against GitHub's ref storage remains the operator's call before merge — same status as v2/v3/v4.

Test results

No new commit, but I re-confirmed the v4 test results hold:

• pytest orchestrator/tests/test_slice_signal_routing.py -v — 19/19 pass
• make lint — clean (only pre-existing soft-cap warnings on unrelated files)

— Authored by egg

[egg-reviewer]

Re-review of #2402 — v6 (delta: 5022c94..00c2b4d is the merged-in #2419 work)

Status

The PR is MERGED. The single new commit since v5 is the squash-merge of PR #2419 (00c2b4d) — the follow-up that closes the dormant gap from v2 review item #1: restart_agent_job and detect_uncommitted_changes did not carry slice_id. That work went through its own review cycle (v1, v2, v3) on #2419 and is now on this branch. Re-reviewing it here for completeness.

Status of v2 review item #1 (the deferred gap)

Failure mode from v2 review	Disposition
delete_job targets the wrong (pipeline-level) Job name	Fixed: restart_agent_job accepts slice_id and forwards to _build_k8s_job_names(pipeline_id, agent_role, slice_id=slice_id) (kubernetes_spawner.py:1153-1155). Test test_delete_targets_slice_scoped_job_name pins it.
Worktree id wrong/absent under restart	Fixed: detect_uncommitted_changes accepts slice_id and constructs the worktree id as f"{pipeline_id}-{slice_id}-{agent_role}" (kubernetes_spawner.py:1304-1306). Tests cover slice/pipeline isolation in both directions.
EGG_SLICE_ID absent on respawned Job	Fixed: spawn_agent_job injects EGG_SLICE_ID from the slice_id parameter (kubernetes_spawner.py:760-761). The restart_agent_job → spawn_agent_job chain forwards slice_id (:1219).

Architectural improvements on top of the literal gap-fix

1. Single source of truth for EGG_SLICE_ID. v2 review of #2419 caught that _run_concurrent_phase was setting EGG_SLICE_ID in sandbox_env while the spawner was also setting it from the slice_id parameter. The fix added EGG_SLICE_ID to _PROTECTED_ENV_KEYS and dropped the caller-side setter. This is the correct shape: extra_env-based override is logged and dropped, so a future caller that passes a mismatched value via extra_env cannot silently end up with the agent's signals tagged for one slice while its Job + worktree belong to another. Tests test_extra_env_cannot_override_egg_slice_id and test_extra_env_cannot_inject_egg_slice_id_when_pipeline_level pin both directions.

2. Regex consolidation. v1 review of #2419 caught that the canonical slice-<N> regex was duplicated across routes/signals.py, the new concurrent_executor.py validation sites, and the new restart route. The fix lifted the pattern into orchestrator/slice_id_validation.py and threaded the import through three call sites. Drift can't reintroduce a fourth shape on the orchestrator side.

3. Per-slice restart budget. The restart key is now (pipeline_id, agent_role, slice_id) so concurrent slices each get an independent budget and lock. reset_restart_counts(pipeline_id) still sweeps all of them via k[0] == pipeline_id filter. Test test_restart_count_is_per_slice and test_reset_restart_counts_clears_slice_buckets pin both behaviors. The concurrency-guard test test_restart_lock_created_per_key adds the new slice scope to the differentiation matrix.

4. Restart-count reporting follows the same key. v1 review of #2419 caught that the restart route was reading get_restart_count(pipeline_id, agent_role) after a slice-scoped restart had bumped the per-slice bucket — both the audit log and the JSON response would misreport "you've burned N of M restarts" telemetry. Now the route forwards slice_id to get_restart_count. Test test_slice_id_query_param_forwarded_to_spawner asserts both kwargs include slice_id.

New code review

orchestrator/slice_id_validation.py:39-61 (SLICE_ID_PATTERN + extract_slice_id):

• Correctly anchored regex (^slice-[0-9]+$).
• None and "" both return None (no slice).
• Non-string non-empty values raise ValueError (the isinstance(raw, str) guard before fullmatch correctly rejects slice_id: 123 rather than blowing up in regex).
• Module docstring is precise: it names Contract._migrate_phases_to_slices as the canonicaliser, calls out that direct Slice(id="phase-2", ...) construction bypasses canonicalization, and explains why this regex is doing real work (registry key must be canonical, Job names / worktree ids must be RFC-1123 safe).

orchestrator/routes/pipelines.py:2330-2338 (restart route slice extraction):

• Query-wins-over-body precedence is documented and consistent.
• request.args.get("slice_id") for ?slice_id= returns "" (Flask convention), which extract_slice_id correctly maps to None — a quirk worth knowing but not a bug.
• Validation failures return 400 before any spawn-side state is mutated. Test test_invalid_slice_id_returns_400 pins four malformed shapes (phase-2, slice-2/etc, ../slice-2, slice-).

orchestrator/kubernetes_spawner.py:760-761 (EGG_SLICE_ID injection):

• Set before extra_env is processed, so the protected-key path can detect and reject conflicts.
• Wrapped in if slice_id is not None so pipeline-level spawns leave the env clean. Test test_spawn_without_slice_id_does_not_set_egg_slice_id pins this.

orchestrator/routes/pipelines.py:2476 (consensus reset routing):

• get_peer_consensus_tracker(pipeline_id, slice_id) correctly targets the per-slice tracker. The pipeline-level tracker has no record of the slice agent, so without this the remove_agent call would be a silent no-op against the wrong tracker.

orchestrator/concurrent_executor.py:294-300 and :332-336:

• Inline import re blocks replaced with the shared SLICE_ID_PATTERN. Comment correctly attributes the consolidation to the cross-cutting concern.

Non-blocking observations

1. Sandbox-side regex is a fourth copy outside the orchestrator's "single source of truth." sandbox/egg_agent_tools/handlers/brc.py:23 keeps its own _SLICE_ID_PATTERN = re.compile(r"^slice-[0-9]+$"). That's correct — the sandbox is a separate process and can't import from orchestrator/. But the new slice_id_validation.py module docstring says "The orchestrator pins slice ids to the canonical slice-<N> shape at every gateway-facing seam … signal handlers (#2403), the operator-triggered restart route (#2410), and the gateway-bound branch builders in concurrent_executor." A reader could conclude this is the only copy. Worth a one-line note that the agent-side regex is intentionally duplicated across the process boundary.

2. ?slice_id= (empty query value) is silently treated as no-slice and masks any body-level slice_id. With ?slice_id=&body={"slice_id":"slice-2"}, the route extracts None and ignores the body. The "query wins over body" docstring covers it, but no test pins this edge case. A regression where the precedence was swapped wouldn't be caught.

3. Concurrent execution reconstruction in the slice-scoped restart path is implicitly mocked. The new tests in TestRestartAgentEndpointSliceScope mock get_container_spawner directly, so the is_concurrent_execution → ConcurrentPhaseExecutor.get_agent_env reconstruction is never exercised under a slice scope. If a future change broke executor.get_agent_env(role) for slice-scoped pipelines, the route's extra_env would be wrong but the test would still pass. Acknowledging — covering this would require constructing a fuller pipeline fixture.

4. The restart-route docstring documents query string slice_id but example shows it in the body. routes/pipelines.py:2275-2286 lists query-string slice_id first, then shows the body example with slice_id too. Functionally fine (both are accepted), but a reader might miss that one wins over the other unless they read the implementation comment.

What's good

• The _PROTECTED_ENV_KEYS extension to EGG_SLICE_ID is a tight, defensive choice. It catches both directions (extra_env overriding a slice-scoped spawn's value, and extra_env injecting a value into a pipeline-level spawn).
• Test coverage is comprehensive: spawn slice/no-slice, restart slice/no-slice, restart budget per-slice, worktree isolation in both directions, malformed slice rejection at the route boundary, query/body parameter precedence, protected-key contract on extra_env.
• The module docstring on slice_id_validation.py is the right level of detail — it names the validator that performs the canonicalisation, the file it lives in, the migration history (#2137 backward compat), and the failure modes (RFC-1123 safety, registry-key uniqueness).
• Restart-budget telemetry is now correct under slice scope: the JSON response and audit log report the per-slice count, which is what the operator running /restart?slice_id=slice-2 expects.
• _get_restart_lock returning the same lock for the same key (per-key map, not per-call) means a future concurrent-restart bug for slice agents would be caught by the same mutual-exclusion guarantee that already exists for pipeline-level agents.
• The decision to keep _restart_locks entries even after reset_restart_counts (per the in-code comment) preserves the mutual-exclusion guarantee against in-flight concurrent restarts. The slice-scope extension doesn't weaken this.

Test plan

Same status as v5: the slice-DAG cold path against GitHub's ref storage is the unchecked e2e. The 00c2b4d work is restart-route + spawn-side, not the push path that #2399 originally fixed, so the e2e signal hasn't changed. The combined #2399 + #2403 + #2410 changes need an issue-2261-v7-style submission to confirm they compose correctly under live GitHub ref-storage semantics — same operator-call status as before.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

22 previous review(s) hidden.