test(integration): slice spawn + restart regression guards (#2632)

integration_tests/regression/README.md

@@ -0,0 +1,72 @@
+# `integration_tests/regression/`
+
+k3s regression guards that pin invariants the SDLC pipeline has
+regressed historically. Tests in here drive the real
+`KubernetesSpawner` against the locally-deployed egg stack and
+inspect resulting pod specs with `kubectl get pod -o yaml`. Per
+[#2474](https://github.com/jwbron/egg/issues/2474), agents writing
+these can't validate them locally — correctness is verified by the
+`Test / aggregate` required check on the PR.
+
+Originating issue: [#2632](https://github.com/jwbron/egg/issues/2632).
+
+## What's covered today
+
+| File | Invariant | Status |
+|---|---|---|
+| `test_slice_spawn_env_threading.py::test_each_slice_gets_its_own_branch_env` | Each per-slice spawn lands `EGG_BRANCH=egg/<pid>/slice-<N>` and `EGG_SLICE_ID=slice-<N>` on the pod spec even when an upstream `extra_env` ships a conflicting pipeline-level `EGG_BRANCH`. Sibling slices in the same pipeline get distinct Job names and distinct EGG_BRANCH refs. Pins #2428 + #2410 + #2403. | ✅ green |
+| `test_slice_spawn_env_threading.py::test_baseline_spawn_without_extra_env_override` | Baseline: with no conflicting `extra_env`, the per-slice `branch` parameter still flows through to the pod's `EGG_BRANCH`. Catches a regression that would break the *default* env-derivation independent of the override path. | ✅ green |
+| `test_long_name_round_trip.py` | A Job created with a name > 63 chars (triggering truncation in `create_container`) must be round-trippable through `delete_job` using the same input name. Direct regression guard for [#2644](https://github.com/jwbron/egg/issues/2644). | ✅ green (was `xfail` before this PR shipped the [#2644](https://github.com/jwbron/egg/issues/2644) fix) |
+| `test_slice_restart_branch_invariants.py::test_restart_preserves_egg_branch_and_slice_id` | `restart_agent_job` for a slice-scoped agent preserves `EGG_BRANCH` and `EGG_SLICE_ID` on the new pod. The slice restart in #2632 starting-point #2. | ✅ green (was `xfail` before this PR shipped the [#2644](https://github.com/jwbron/egg/issues/2644) + [#2655](https://github.com/jwbron/egg/issues/2655) fixes) |
+| `test_slice_restart_branch_invariants.py::test_restart_isolates_slice_from_pipeline_level_agent` | Restarting a pipeline-level agent of the same role doesn't disturb the slice-scoped Job's env or restart-budget. | ✅ green (was `xfail` before this PR shipped the [#2644](https://github.com/jwbron/egg/issues/2644) + [#2655](https://github.com/jwbron/egg/issues/2655) fixes) |
+
+## Bugs surfaced while writing these tests
+
+### #2644 — `KubernetesClient.delete_job` name-truncation asymmetry
+
+`create_container` truncates Job names > 63 chars and appends an
+8-char SHA digest. `delete_job` / `read_namespaced_job` /
+`get_pod_for_job` do **not** apply the same truncation, so any
+operation against the un-truncated name silently 404s when the Job
+exists under the truncated form. The slice-DAG restart path
+deterministically hits this for any pipeline-id / role
+combination > 63 chars after prefixing (e.g.
+`issue-2261-v9` + `slice-2` + `reviewer_agent_design`).
+
+`test_long_name_round_trip.py` is the focused regression guard.
+
+### #2655 — `restart_agent_job` races the Foreground deletion finalizer
+
+Even after #2644 is fixed, `restart_agent_job`'s `Foreground`
+deletion returns before the Job is removed from the API server —
+the foreground finalizer waits for pods to terminate first. The
+immediate respawn that follows then races the finalizer and 409s
+on `AlreadyExists`. The two slice-restart tests need both #2644
+and #2655 to land before they flip to passing.
+
+## Gap audit — what should be in here but isn't yet
+
+Pulled from [#2632](https://github.com/jwbron/egg/issues/2632) and
+recent slice/BRC postmortems. Tracking issues:
+
+- **[#2664](https://github.com/jwbron/egg/issues/2664)** — umbrella for the remaining #2474 starting points (HITL, salvage, BRC, babysit-PR push, etc.).
+- **[#2666](https://github.com/jwbron/egg/issues/2666)** — slice-scoped pods missing an `egg.slice.id` label (operability gap).
+
+| Invariant | Why it matters | Why not yet |
+|---|---|---|
+| Slice DAG reaches `PR_READY` end-to-end with a mid-flight `restart_agent` | The full "starting point #2" of #2632. Pins that BRC consensus recovers cleanly across a slice-agent restart. | Requires real BRC consensus driving against a Claude provider, which a fresh CI runner cannot drive. See the ScriptedProvider pod-injection caveat in [#2474](https://github.com/jwbron/egg/issues/2474) and follow-up #2585. The branch-ref half is covered above. |
+| Live-pod guard on `start_pipeline` recovery (#2420) | `start_pipeline` refuses without `force=true` when pods are live; pins #2420's regression. | Direct HTTP-level test against orchestrator routes. Reasonable to add in a follow-up; doesn't need agent pods to actually run. |
+| Unpushed-commit salvage on push rejection (#2429) | A gateway push-rejection must produce an `egg/recovered/...` ref before the worktree is torn down. | Needs gateway push-failure injection — the existing `restricted-path` code path may be enough. Follow-up. |
+| HITL round-trip (#2430) | Pipeline pauses on `AWAITING_HUMAN`, resumes on `provide_input`. Pins #2430. | Needs a pipeline that actually reaches HITL — requires either a scripted provider or a pre-seeded contract. Follow-up. |
+| BRC single-cycle consensus message counts | Pins `feedback_brc_single_cycle.md`: producer PROPOSE → reviewer ACK → CONFIRMED in exactly N messages. | Needs ScriptedProvider pod injection. Deferred to #2585. |
+| Phase-aware consensus timeouts honored end-to-end | Pins that `phase_configs.<phase>.consensus_timeout_s` is wired through to the actual timeout fired. | Implementable as a unit test against the orchestrator's timeout source-of-truth; the k3s tier doesn't add much. |
+| Babysit-PR single final push to PR branch | Pins that 2 coder revisions produce exactly 1 push to the PR head ref. | Needs scripted provider for the coder revision loop. Deferred to #2585. |
+| Slice teardown isolation under partial-failure | Deleting one slice's Job mid-flight doesn't disturb sibling slices' worktrees or sessions. | Achievable with the existing spawner harness once #2644 is fixed; adds another regression test to this directory. |
+| `EGG_SLICE_ID` and `egg.slice.id` label parity | Today the slice scope is propagated as an env var on the pod but **not** as a Job/Pod label — operator queries like `kubectl get jobs -l egg.slice.id=slice-2` don't work, and the kubernetes_monitor can't filter by slice without parsing Job names. | Out of scope for #2632 (an operability gap, not a correctness regression) but worth opening separately if cluster-side slice introspection becomes important. |
+
+## Conventions
+
+- **Marker**: every file in here uses `pytestmark = pytest.mark.integration` so `make test-integration` picks them up.
+- **Cleanup**: every test paired with a `cleanup_jobs` autouse fixture that calls `spawner.cleanup_pipeline(...)`. Without it, leftover Jobs in the test namespace persist across runs and force operators to `kubectl delete` by hand.
+- **Role choice**: prefer roles in `_ROLES_WITHOUT_WORKTREE` (e.g. `REVIEWER_CODE`) for env-threading assertions — the spawner code paths in question are role-independent, and a worktree-free role keeps the test green on a fresh CI runner with `local_repos.paths: []`. Document the choice in the test's docstring when it matters.
+- **xfail discipline**: `xfail(strict=True, reason=...)` only — never `skip` to hide a real failure. Strict ensures the test re-arms automatically when the blocking bug lands.

integration_tests/regression/conftest.py

@@ -1,29 +1,47 @@
-"""Shared fixtures for BRC consensus regression tests (issue #2635).
-
-The regression tier covers behaviours that have been hand-rolled into
-postmortems — BRC single-cycle, phase-aware timeouts, NACK round-trip,
-reviewer disagreement, etc.  Tests live here (not under
-``orchestrator/tests/``) because they exercise the orchestrator's
-Python API at the integration boundary — the same shape #2474
-recommends after the ScriptedProvider pod-injection avenue was ruled
-out (the constraint write-up referenced from issue #2635).
-
-Tests in this folder are marked ``integration`` so they run under
-``make test-integration`` alongside the k3s tier, but they do NOT
-require k3s and never call into the ``egg_stack`` fixture — they
-drive ``PeerConsensusTracker`` and the timeout-handler entry points
-in-process against real implementations.
-
-Plain helper functions (``make_tracker``, ``propose_payload``,
-``filter_events``) live in ``_helpers.py``; pytest's conftest
-discovery only surfaces fixtures cross-module.
+"""Shared fixtures for ``integration_tests/regression/``.
+
+This directory hosts two orthogonal regression tiers:
+
+* **BRC consensus** (issue #2635): exercises ``PeerConsensusTracker``
+  and the timeout-handler entry points in-process.  These tests do
+  NOT require k3s — they drive the orchestrator's Python API at the
+  integration boundary (the shape #2474 recommends after the
+  ScriptedProvider pod-injection avenue was ruled out).
+* **k3s slice-spawn / restart guards** (issue #2632): drive the real
+  ``KubernetesSpawner`` against the locally-deployed egg stack and
+  read pod specs back with ``kubectl get pod -o yaml``.  These pin
+  invariants we've regressed historically (slice spawn env threading
+  from #2428, slice restart branch ref from the #2410/#2428 follow-ups).
+
+Both tiers are marked ``integration`` and run under
+``make test-integration``.  The k3s fixtures only fire when a test
+takes the ``spawner`` / ``egg_stack`` fixtures; the BRC fixtures are
+either autouse (tracker registry) or opt-in.
+
+The k3s fixtures intentionally pick spawn parameters that do NOT
+require a populated gateway test-repo: roles in
+``_ROLES_WITHOUT_WORKTREE`` and ``repos=[]``.  The env-threading and
+slice-id-threading code paths in ``kubernetes_spawner.py`` are
+role-independent (see lines 754-774 of that file at the time of
+writing), so a worktree-free role exercises the same seam the
+``coder`` regression in #2428 fired through.  This keeps the test
+green on a fresh CI runner where ``$HOME/repos`` is empty.
+
+Plain helper functions for BRC tests (``make_tracker``,
+``propose_payload``, ``filter_events``) live in ``_helpers.py``;
+pytest's conftest discovery only surfaces fixtures cross-module, so
+helpers usable in ``import`` statements have to live next door.
 """
 
 from __future__ import annotations
 
+import json
+import subprocess
 import sys
+import time
 from collections.abc import Callable, Generator
 from pathlib import Path
+from typing import Any
 
 # Make sibling ``_helpers.py`` and the orchestrator/shared trees
 # importable before any conftest-level imports below land.
@@ -45,6 +63,141 @@
 from peer_consensus import _trackers_lock as _global_trackers_lock  # noqa: E402
 from review_graph import ReviewCriticality, ReviewEdge, ReviewGraph  # noqa: E402
 
+# ---------------------------------------------------------------------------
+# k3s slice-spawn helpers (#2632)
+# ---------------------------------------------------------------------------
+
+
+def kubectl_get_pod_yaml(
+    namespace: str,
+    label_selector: str,
+    timeout_s: float = 60.0,
+) -> dict[str, Any]:
+    """Return the first pod matching ``label_selector`` as a parsed dict.
+
+    Polls ``kubectl get pods -l <selector>`` until at least one pod
+    exists or the timeout expires. The pod spec (including the env
+    var list) is populated as soon as the Job's pod template is
+    materialized — we do NOT wait for ``Running`` because a session
+    with a token-only gateway registration will still produce a pod
+    spec whether or not its image entrypoint succeeds.
+
+    Args:
+        namespace: k8s namespace.
+        label_selector: passed verbatim to ``kubectl -l``.
+        timeout_s: pod-appearance deadline.
+
+    Raises:
+        AssertionError: if no pod appears within ``timeout_s``.
+    """
+    deadline = time.monotonic() + timeout_s
+    last_err: str | None = None
+    while time.monotonic() < deadline:
+        proc = subprocess.run(
+            [
+                "kubectl",
+                "-n",
+                namespace,
+                "get",
+                "pods",
+                "-l",
+                label_selector,
+                "-o",
+                "json",
+            ],
+            capture_output=True,
+            text=True,
+            timeout=15,
+            check=False,
+        )
+        if proc.returncode != 0:
+            last_err = proc.stderr
+            time.sleep(1)
+            continue
+        try:
+            data = json.loads(proc.stdout)
+        except json.JSONDecodeError as e:
+            last_err = f"{e}: {proc.stdout[:200]}"
+            time.sleep(1)
+            continue
+        items = data.get("items") or []
+        if items:
+            return items[0]
+        time.sleep(1)
+    raise AssertionError(
+        f"No pod matched selector {label_selector!r} in {namespace} "
+        f"within {timeout_s}s (last error: {last_err})"
+    )
+
+
+def env_from_pod(pod: dict[str, Any]) -> dict[str, str]:
+    """Flatten the agent container's literal ``env`` list to a dict.
+
+    Skips ``valueFrom`` entries — they don't have a literal value at
+    the pod-spec level. ``EGG_BRANCH`` / ``EGG_SLICE_ID`` are always
+    set as literals by ``KubernetesSpawner`` so this is sufficient
+    for the invariants this directory pins.
+    """
+    containers = pod.get("spec", {}).get("containers") or []
+    if not containers:
+        raise AssertionError(f"Pod has no containers: {pod.get('metadata', {}).get('name')}")
+    out: dict[str, str] = {}
+    for entry in containers[0].get("env") or []:
+        if "value" in entry:
+            out[entry["name"]] = entry["value"]
+    return out
+
+
+@pytest.fixture
+def spawner(egg_stack: Any) -> Generator[Any]:
+    """Yield a ``KubernetesSpawner`` bound to the test agent namespace.
+
+    Uses the same launcher secret + gateway URL the rest of the
+    integration suite discovers via ``egg_stack``. The spawner's
+    ``KubernetesClient`` loads the local kubeconfig (the test process
+    runs out-of-cluster).
+    """
+    try:
+        from gateway_client import GatewayClient
+        from kubernetes_client import KubernetesClient
+        from kubernetes_spawner import KubernetesSpawner
+    except ImportError as e:
+        pytest.skip(f"Could not import orchestrator modules: {e}")
+
+    # Pin GatewayClient at the discovered gateway URL. ``egg_stack``
+    # already validated the gateway is reachable.
+    gateway_url = egg_stack.gateway_url.rstrip("/")
+    # ``gateway_url`` is ``http://<host>:<port>``; split it for the
+    # client's host/port kwargs.
+    parsed = gateway_url.removeprefix("http://").removeprefix("https://")
+    if ":" in parsed:
+        host, port_s = parsed.rsplit(":", 1)
+        port = int(port_s)
+    else:
+        host = parsed
+        port = egg_stack.gateway_port
+
+    # ``launcher_secret`` is passed explicitly to ``GatewayClient`` —
+    # the env-var fallback inside the client never fires here, so no
+    # ``os.environ`` mutation is needed.
+    gateway = GatewayClient(
+        gateway_host=host,
+        gateway_port=port,
+        launcher_secret=egg_stack.launcher_secret,
+    )
+    k8s = KubernetesClient(namespace=egg_stack.isolated_network)
+    s = KubernetesSpawner(
+        k8s_client=k8s,
+        gateway_client=gateway,
+        namespace=egg_stack.isolated_network,
+    )
+    yield s
+
+
+# ---------------------------------------------------------------------------
+# BRC consensus fixtures (#2635)
+# ---------------------------------------------------------------------------
+
 
 @pytest.fixture(autouse=True)
 def _reset_tracker_registry() -> Generator[None]: