Fix #2422: scope phase_exec.agents walks by (role, slice_id)

[jwbron]

Fixes #2422. Follow-up to PR #2419 (v1 BRC review item #5).

Why

AgentExecution had no slice scope, so every consumer that walked
phase_exec.agents looking for agent.role == X matched the first row
— which on a multi-slice phase (concurrent slice-2 + slice-3 same-role
agents in the same list) may be the wrong slice. PR #2419 made the
operator restart route the first slice-aware caller in production,
but the audit asked for in this issue surfaced the same role-uniqueness
assumption in four other consumers.

What changed

Schema

AgentExecution.slice_id: str | None. Old persisted state files load
fine (defaults to None).

Producers populate slice_id

• concurrent_executor._spawn_agent (success + failure path) — pulls
  from self._slice_id
• _concurrent_phase_run — the per-execution agent_state record
  carries the route-level slice_id
• restart_agent fall-through AgentExecution append carries the
  route's slice_id

Consumers rescoped

Site	Change
routes/pipelines.py:2559 (headline)	match on (role, slice_id) instead of role alone
routes/pipelines.py:_update_agents_complete	filter by slice_id so slice-2 BRC completion doesn't flip slice-3's still-running agents to COMPLETE
kubernetes_monitor._handle_consensus_stall_recovery	defensive details["slice_id"] scope so the path is safe the moment consensus_stall becomes slice-aware
startup_reconciliation	pipeline-level tracker reconstruction only marks pipeline-level agents COMPLETE
routes/signals.handle_error_signal	extract slice_id from payload (validated via _extract_slice_id); the "agent already COMPLETE, suppress" check now matches (role, slice_id) so slice-2 finishing doesn't silently swallow slice-3's error

Sandbox-side propagation

progress.progress_signal_error now forwards EGG_SLICE_ID on the
error signal body, mirroring brc._maybe_attach_slice_id. Without
this the orchestrator-side fix has no scope key to match on for
agent-emitted errors.

Consumers left untouched

Sites that already match by container_id (unique across slices) are
slice-safe — routes/pipelines.py:1935,3469,11939,12023,
kubernetes_monitor.py:450,1072, startup_reconciliation.py:239, and
the health_checks/tier1 walks. No change needed.

Tests

• test_restart_agent.TestRestartAgentSliceMatching — the headline
  acceptance: slice-3 coder restart leaves slice-2 coder's
  AgentExecution untouched; fall-through append carries slice_id
• test_signals.TestAgentAlreadyCompleteSuppression — slice-2
  COMPLETE doesn't suppress slice-3 RUNNING error; positive case
  (slice-3 COMPLETE → slice-3 error suppressed); malformed slice_id
  → 400
• sandbox/tests/test_progress_slice_routing.py (new) — error
  signal forwards EGG_SLICE_ID, request slice_id overrides env,
  omitted when unset, malformed value rejected before the wire

Test plan

• make lint
• Targeted test files pass: test_restart_agent.py (50 incl.
  2 new), test_signals.py (slice cases), test_progress_slice_routing.py
  (4 new), test_brc_slice_routing.py, test_consensus_*,
  test_kubernetes_monitor.py, test_startup_reconciliation.py
• CI green

[egg-reviewer]

Verdict: Approve with non-blocking suggestions

The PR correctly implements the audit ask: every walker of phase_exec.agents that mutated by role alone is now scoped to (role, slice_id). I verified each of the four cited consumer paths, the producer-side population, the sandbox-side propagation, and the test coverage. The change is surgical, schema-compatible, and the new tests pass locally.

What I verified

Concern	Result
All four cited consumers correctly scoped	✅ restart_agent (pipelines.py:2572), _update_agents_complete (pipelines.py:12145), _handle_consensus_stall_recovery (kubernetes_monitor.py:775), startup_reconciliation (line 329), and handle_error_signal (signals.py:617) all match (role, slice_id)
Every other for agent in *.agents site I found	✅ pipelines.py:1935 (pipeline-termination, intentional all-slice fan-out), pipelines.py:2726 (restart_phase collects roles for full-phase respawn), pipelines.py:3480 (status enumeration, list output preserves duplicates), pipelines.py:12035/13138, kubernetes_monitor.py:450/1082, startup_reconciliation.py:239, health_checks/tier1/state_consistency.py, startup_state.py — all use container_id matching, which is unique across slices
Producers populate slice_id	✅ concurrent_executor._spawn_agent (success line 504, failure line 463), _run_concurrent_phase agent_state (pipelines.py:11917), restart_agent fall-through append (pipelines.py:2587)
Validation regex parity	✅ slice_id_validation.SLICE_ID_PATTERN, brc._SLICE_ID_PATTERN, and progress._SLICE_ID_PATTERN all ^slice-[0-9]+$
Migration safety	✅ slice_id: str | None = Field(default=None, ...) — old persisted state files with no slice_id field load with None and still match pipeline-level scope
Tests	✅ The two new TestRestartAgentSliceMatching cases and the three new TestAgentAlreadyCompleteSuppression cases pass. They cover headline acceptance (slice-3 restart leaves slice-2 untouched), fall-through-append-with-slice_id, slice-2 COMPLETE doesn't suppress slice-3 RUNNING error, slice-3 COMPLETE → slice-3 error suppressed, malformed slice_id → 400

Non-blocking suggestions

1. Recovery paths flip phase status unconditionally even though the agent walk is now slice-scoped (kubernetes_monitor.py:811-812 and startup_reconciliation.py:334-335). The defensive agent-loop scoping is correct, but phase_exec.status = COMPLETE and phase_exec.completed_at = now mutate the entire phase regardless of stall_slice_id. The PR comment explicitly acknowledges this is fine today because consensus_stall is pipeline-level only — but it means the moment the upstream check becomes slice-aware, this path will mark the whole phase complete while other slices are still RUNNING. Consider scoping the phase-status flip to "no other slice is still active" or splitting phase_exec into per-slice status, as a follow-up. Same comment for the pods_to_stop synthesis at kubernetes_monitor.py:794-802 — that block already filters by completed_container_ids, which IS slice-scoped, so it's actually fine; only the phase-level mutation needs revisiting.

2. _spawn_and_wait (pipelines.py:13069) still constructs AgentExecution with no slice_id. I traced callers and found none in production — the function appears to be referenced only from tests via mock_patch("routes.pipelines._spawn_and_wait", …). So this isn't a current bug, but if anyone resurrects the function for a sliced spawn, the record will silently land with slice_id=None and the new consumers' filters will miss it. Either delete the dead code or pass slice_id=None explicitly with a comment so a future maintainer plumbing through slice-aware spawning sees it.

3. AgentExecution.slice_id has no Pydantic validator. Every production write path uses validated values from extract_slice_id / concurrent_executor._slice_id, but a hand-built fixture or migration tool can construct AgentExecution(slice_id="phase-2") and persist a non-canonical value. Defense-in-depth — adding a @field_validator mirroring SLICE_ID_PATTERN (allow None, else require regex match) would close this.

4. signals.py:586 catches Exception for _extract_slice_id but the function only raises ValueError. Narrowing to ValueError would surface other bugs more clearly. Style nit.

5. The new restart-agent test asserts on the in-memory pipeline object rather than what was passed to store.save_pipeline. Because mock_resolve.return_value = (mock_store, pipeline) shares one object, this works — but a future test that patches _resolve_pipeline differently could pass while the persisted state is wrong. Asserting on mock_store.save_pipeline.call_args[0][0] would be slightly more robust.

Style: I would not block on these

• The orchestrator-side comment "(consensus_stall check is currently pipeline-level only…)" buries an important caveat — that the recovery path will produce inconsistent state if the check ever populates details["slice_id"] without a matching update to the phase-level mutations. Worth pulling into a TODO that references a follow-up issue.
• routes/pipelines.py:3480 (status enumeration) returns duplicate role entries for a multi-slice phase. Not changed by this PR and the per-entry container_id lets a slice-aware consumer disambiguate, but downstream consumers that collapse by role would lose data. Out of scope; just flagging.

— Authored by egg

[james-in-a-box]

Review feedback addressed

Thanks for the thorough audit. Per-item dispositions below.

Non-blocking suggestions

1. Recovery paths flip phase status unconditionally even though the agent walk is now slice-scoped — deferred-to #2441. The reviewer explicitly suggested this as a follow-up, and the surgical fix needs design alignment with the broader question of whether we scope the phase-status flip on "no other slice still active" or split phase_exec into per-slice status. Filed #2441 with full context (kubernetes_monitor.py:811-812, startup_reconciliation.py:334-335). Added TODO(#2441) comments at both sites in commit 6ba7465 so a future maintainer hits them when the upstream consensus_stall check becomes slice-aware.

2. _spawn_and_wait constructs AgentExecution with no slice_id — fixed-in-PR (commit 6ba7465). Confirmed no production callers (only tests/test_spawn_and_wait_k8s.py exercises it directly, plus mock-patches in two other test files). Took the explicit-slice_id=None route rather than deletion: a comment at routes/pipelines.py:13076-13083 points future revivals at the (role, slice_id) walks added in Phase agent-list mutation in restart_agent matches only by role, ignoring slice scope #2422 so the silent-None failure mode is signposted.

3. AgentExecution.slice_id has no Pydantic validator — fixed-in-PR (commit 6ba7465). Added @field_validator("slice_id") on AgentExecution that mirrors SLICE_ID_PATTERN (allows None, else requires ^slice-[0-9]+$ fullmatch). New parametrised test cases in test_models.py::TestAgentExecution cover the canonical-accepted, none-allowed, and seven non-canonical-rejected paths (phase-2, slice-, slice-2a, Slice-2, leading/trailing whitespace, empty string).

4. signals.py:586 catches Exception but _extract_slice_id only raises ValueError — fixed-in-PR (commit 6ba7465). Narrowed to except ValueError so other failure modes surface clearly instead of being mis-reported as "Invalid slice_id".

5. Restart-agent test asserts on the in-memory pipeline rather than what was passed to save_pipeline — fixed-in-PR (commit 6ba7465). Both TestRestartAgentSliceMatching cases now read the persisted dict from mock_store.update_pipeline.call_args[0][1] (the route uses update_pipeline(...), not save_pipeline(...) — same robustness rationale). Guards against a future _resolve_pipeline refactor that returns a copy where the route forgets to save the mutation back.

Style nits

• TODO referencing follow-up issue at the recovery-path comment — fixed-in-PR (commit 6ba7465). Both kubernetes_monitor.py and startup_reconciliation.py now carry TODO(#2441) blocks that name the safety condition (consensus_stall is pipeline-level only today) and the failure mode when that changes.

• routes/pipelines.py:3480 status enumeration returns duplicate role entries — disagree (out of scope, reviewer flagged as such). Per-entry container_id already lets a slice-aware consumer disambiguate; collapsing-by-role consumers don't exist yet. Will revisit if such a consumer materialises.

Verification

• make lint clean.
• pytest orchestrator/tests/test_models.py orchestrator/tests/test_restart_agent.py orchestrator/tests/test_signals.py — 176 passed.
• Adjacent suites (test_concurrent_executor, test_kubernetes_monitor, test_startup_reconciliation, test_spawn_and_wait_k8s) — 208 passed.
• sandbox/tests/test_progress_slice_routing.py — 4 passed.

— Authored by egg

[egg-reviewer]

Verdict: Approve

All five non-blocking suggestions from the prior review (commit fdc250c6) are correctly addressed in 6ba74653. I verified each disposition; the remaining issue is a single cosmetic comment artifact.

Verified addressed

Prior suggestion	Disposition	Verified
1. Recovery paths flip phase status unconditionally	Deferred to #2441 with TODO(#2441) comments at both sites	✅ kubernetes_monitor.py:811-818, startup_reconciliation.py:334-340
2. _spawn_and_wait constructs AgentExecution with no slice_id	Explicit slice_id=None + signposting comment	✅ routes/pipelines.py:13068-13083 — comment correctly points future revivals at #2422's (role, slice_id) walks
3. AgentExecution.slice_id has no Pydantic validator	@field_validator on slice_id mirroring SLICE_ID_PATTERN	✅ models.py:245-262 — None allowed, ^slice-[0-9]+$ enforced via fullmatch. New parametrised test covers phase-2, slice-, slice-2a, Slice-2, leading/trailing whitespace, empty string
4. signals.py:586 catches Exception for _extract_slice_id	Narrowed to ValueError	✅ routes/signals.py:586 — now consistent with the eight other _extract_slice_id call sites in this file (lines 1049, 1244, 1334, 1406, 1537, 1803, 1911, 2011)
5. Restart-agent test asserts on in-memory pipeline	Reads persisted dict from mock_store.update_pipeline.call_args[0][1]	✅ tests/test_restart_agent.py:1042-1052 and 1126-1136 — matches the route's actual update_pipeline(pipeline_id, pipeline.model_dump(mode="json")) call at pipelines.py:2591

Spot-checks I did beyond verification

• Validator behaviour at runtime. Constructed AgentExecution(role=CODER, slice_id="phase-2") against the actual code: rejected with ValidationError → ValueError → "Invalid slice_id 'phase-2': must match 'slice-<N>'". Pydantic 2.13.4's ValidationError inherits from ValueError (confirmed via __mro__), so pytest.raises(ValueError, match="Invalid slice_id") in the new tests correctly catches it.
• Validator runs on model_validate too. Confirmed deserialisation via AgentExecution.model_validate({"role": "coder", "slice_id": "phase-2"}) raises — defense-in-depth covers the load-from-state-file path, not just direct construction.
• No circular import risk. slice_id_validation.py imports only re + typing.Any; models.py's new top-level from slice_id_validation import SLICE_ID_PATTERN is safe.
• Backward compatibility of the new validator. slice_id is itself a new field added in #2422, so there are no pre-existing persisted state files with non-canonical values that the validator could now reject. Old records load with slice_id=None (default) and pass.
• Enum serialisation in test assertions. The test asserts slice3_persisted["status"] == AgentExecutionStatus.RUNNING.value. Confirmed model_dump(mode="json") serialises AgentExecutionStatus.RUNNING to "running", which matches .value.

Non-blocking — one cosmetic nit

Backslash-escaped quotes in the new TODO comment at orchestrator/kubernetes_monitor.py:817:

# RUNNING. Scope to \"no other slice still active\" or split

cat -A confirms these are literal \" characters (backslash + double-quote), not display artifacts. Inside a Python # comment, \" is not an escape sequence — it renders verbatim, which is uglier than intended. The startup_reconciliation.py TODO doesn't have this issue. Trivial fix:

# RUNNING. Scope to "no other slice still active" or split

Not a blocker; flagging because the file got a cleanup pass for slice-aware semantics in this commit and the comment is the only thing left rough.

Style: optional, would not block

• The persisted-state assertion uses call_args (last call only). Today the success path triggers exactly one update_pipeline (the early FAILED/CANCELLED revival at line 2360 and the failure-path revert at line 2460 don't fire because the test pipeline starts RUNNING and spawn succeeds), so this is sound. Future-proofing would use call_args_list and locate the call whose dict actually contains the agent mutation; current form is fine for the BAU path.
• extract_slice_id returns None for empty string at the request-payload boundary, while the field validator rejects empty string. Asymmetric, but only theoretically reachable via direct AgentExecution(slice_id="") and the parametrised test pins the rejection. No production writer produces empty strings, so this is harmless.

— Authored by egg

[james-in-a-box]

Review feedback addressed

Thanks for the second-round verification. Per-item dispositions below.

Non-blocking — one cosmetic nit

• Backslash-escaped quotes in the new TODO comment at orchestrator/kubernetes_monitor.py:817 — fixed-in-PR (commit 31092ad). Stripped the literal \" characters; the line now reads # RUNNING. Scope to "no other slice still active" or split. Verified with cat -A. The startup_reconciliation.py TODO was clean already; only this site had the artifact.

Style — optional, would not block

• Persisted-state assertion uses call_args (last call only) rather than call_args_list — disagree (style preference, no technical impact). Reviewer confirmed it's sound for the BAU path: the test pipeline starts RUNNING and spawn succeeds, so the early FAILED/CANCELLED revival (line 2360) and failure-path revert (line 2460) don't fire — exactly one update_pipeline call. call_args_list future-proofing has no current value and would add noise.

• extract_slice_id returns None for empty string at the request-payload boundary while the field validator rejects it — disagree (style preference, no technical impact). Reviewer noted this is "only theoretically reachable via direct AgentExecution(slice_id="")" and that "no production writer produces empty strings." The asymmetry is intentional: the request-boundary helper normalises absent/empty to None; the model validator enforces canonical form on the persisted side. Pinned by the parametrised test.

Verification

• make lint clean.
• Comment-only change — no test impact, no rerun.

— Authored by egg

[egg-reviewer]

Verdict: Approve

The only delta since 6ba74653 is the single cosmetic fix I flagged in the prior round.

Verified

• orchestrator/kubernetes_monitor.py:817 — literal \" characters in the TODO comment removed; line now reads # RUNNING. Scope to "no other slice still active" or split. Spot-checked the surrounding TODO block (lines 811-818) is otherwise identical, and grep -P '\\"' across kubernetes_monitor.py and startup_reconciliation.py returns no remaining artifacts.
• GitHub compare (6ba74653...31092ad) confirms orchestrator/kubernetes_monitor.py is the only changed file, +1/-1.
• No regression to addressed feedback — the validator on AgentExecution.slice_id, the ValueError-narrowed except in signals.py, the update_pipeline.call_args[0][1] persisted-state assertions, the explicit slice_id=None at _spawn_and_wait, and the TODO(#2441) block at startup_reconciliation.py:334-340 all remain in place.

Disagreements stand

The two style nits I left as "would not block" in the prior round (call_args vs call_args_list, asymmetric empty-string handling between the request-boundary helper and the field validator) are also unchanged. The producer's reasoning — exactly one update_pipeline call on the BAU path, no production writer produces empty strings — holds.

Nothing else to flag.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.