test(integration): pin HITL HTTP round-trip invariants on k3s (#2634)

integration_tests/regression/README.md

@@ -1,17 +1,24 @@
 # `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.
+Cross-module regression guards that pin invariants the SDLC pipeline
+has regressed historically. The directory hosts three orthogonal
+tiers (see `conftest.py` for the fixture catalog):
 
-Originating issue: [#2632](https://github.com/jwbron/egg/issues/2632).
+| Tier | Drives | Needs k3s? | Originating issue |
+|---|---|---|---|
+| **k3s slice-spawn / restart** | Real `KubernetesSpawner` against the locally-deployed egg stack; pod specs read back with `kubectl get pod -o yaml`. | ✅ yes | [#2632](https://github.com/jwbron/egg/issues/2632) |
+| **HITL HTTP round-trip** | The `/api/v1/pipelines/<id>/decisions/...` HTTP surface against the live orchestrator + gateway. Auth-rejection / shape tests run without k3s; happy-path tests skip cleanly when `gateway-secrets/lifecycle-secret` is unreachable from the test runner. | ⚠️ partial — happy-path needs the lifecycle secret from `egg-system` | [#2474](https://github.com/jwbron/egg/issues/2474), [#2634](https://github.com/jwbron/egg/issues/2634) |
+| **BRC consensus** | `PeerConsensusTracker` and timeout-handler entry points in-process — the shape #2474 recommends after ScriptedProvider pod-injection was ruled out. | ❌ no | [#2635](https://github.com/jwbron/egg/issues/2635) |
+
+Per [#2474](https://github.com/jwbron/egg/issues/2474), agents writing
+the k3s-tier tests can't validate them locally — correctness is
+verified by the `Test / aggregate` required check on the PR. The BRC
+and HITL-shape tiers run on a developer laptop without a cluster.
 
 ## What's covered today
 
+### k3s slice-spawn / restart tier
+
 | 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 |
@@ -20,6 +27,27 @@ Originating issue: [#2632](https://github.com/jwbron/egg/issues/2632).
 | `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) |
 
+### HITL HTTP round-trip tier
+
+| File | Invariant | Status |
+|---|---|---|
+| `test_hitl_round_trip.py::TestHitlRoutesRegistered` | All 6 HITL endpoints resolve on the live blueprint; 404 envelopes reference the pipeline id (so Flask's stock route-missing 404 doesn't silently pass). | ✅ green |
+| `test_hitl_round_trip.py::TestHitlLifecycleAuth` | `/resolve` and `/cancel` reject missing / bogus / non-Bearer headers — #1769 parity. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlUnknownPipelineReturns404` | Agent-facing routes return canonical 404 envelopes referencing the pipeline id. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlQueueDecisionPayloadValidation` | Missing question / invalid `decision_type` / invalid `phase` / no-body → structured 400 (or 415 for no Content-Type), never 500. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlResolveRequiresResolution` | `/resolve` with auth + empty body → 400, pinning body-validation-after-auth ordering. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlPipelineIdValidation` | Malformed / path-traversal pipeline ids → 400 (`InvalidPipelineIdError`); 404 branches must NOT look like pipeline-not-found. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlHttpMethodEnforcement` | DELETE/PUT/PATCH on POST routes, GET on /resolve / /cancel, POST on /status → 405. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlMalformedJsonBody` | Invalid JSON → 400 (with canonical envelope). Non-object JSON (list / scalar, truthy and falsy) → 400 (#2656 fix). `null` body coerces correctly. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlResolvePayloadEdgeCases` | Null / `""` / `"   "` / dict / list / int / `False` resolution — pins which trip the `if not resolution` check vs which fall through (and that dict/list don't 500 the `json.dumps` normalisation). | ✅ green |
+| `test_hitl_round_trip.py::TestHitlCancelOnUnknownDecision` | `/cancel` with auth on missing decision → 404 envelope with pipeline id. | ✅ green |
+| `test_hitl_round_trip.py::TestHitlOversizedPayload` | 5 MB question body doesn't 500 or hang. | ✅ green |
+| `test_hitl_round_trip.py::test_deterministic_pipeline_id_is_syntactically_valid` | Helper emits `pipeline-<8hex>` (validated by importing `state_store.validate_pipeline_id`) so 404 assertions don't silently turn into 400 assertions. | ✅ green |
+
+### BRC consensus tier
+
+In-process tests of `PeerConsensusTracker`, timeout-handler triage, and review-graph topology — 30 invariants across `test_brc_*.py` files originally landed by #2635. These are byte-identical on this PR (the merge from `main` brought them in unchanged); see #2635 for the per-file breakdown.
+
 ## Bugs surfaced while writing these tests
 
 ### #2644 — `KubernetesClient.delete_job` name-truncation asymmetry

integration_tests/regression/__init__.py

@@ -0,0 +1,13 @@
+"""Regression tier for k3s integration tests.
+
+Each module pins a specific invariant against the deployed orchestrator
++ gateway in the k3s overlay. The parent conftest's ``egg_stack`` and
+``orchestrator_url`` fixtures auto-skip when ``kubectl`` is unavailable,
+so a local ``make test`` without a cluster cleanly skips this whole
+subtree.
+
+See ``integration_tests/regression/conftest.py`` for the helpers
+specific to this tier (lifecycle-secret lookup, ephemeral pipeline
+ids, etc.) — the parent ``integration_tests/conftest.py`` still owns
+the k3s harness.
+"""

integration_tests/regression/conftest.py

@@ -1,6 +1,6 @@
 """Shared fixtures for ``integration_tests/regression/``.
 
-This directory hosts four orthogonal regression tiers:
+This directory hosts five orthogonal regression tiers:
 
 * **Pipeline recovery / unpushed-commit salvage** (issue #2633) — sits
   between the unit-tier (``orchestrator/tests/``) and the k3s-tier
@@ -26,6 +26,19 @@
   ``MessageStore`` (in-memory) and ``RedisMessageStore`` backed by
   ``fakeredis.FakeRedis`` so a regression in either backend surfaces.
 
+* **HITL HTTP round-trip helpers** (issues #2474, #2634) — pin the
+  ``/api/v1/pipelines/<id>/decisions/...`` HTTP surface against the
+  locally-deployed egg stack. The tier uses
+  :func:`deterministic_pipeline_id` to derive a syntactically valid
+  ``pipeline-{8 hex chars}`` id from each test's pytest nodeid so
+  re-runs reuse the same id (and 404 assertions don't silently turn
+  into 400 ``InvalidPipelineIdError`` ones), and
+  :func:`lifecycle_secret` / :func:`lifecycle_bearer` to read the
+  orchestrator's lifecycle bearer from
+  ``gateway-secrets/lifecycle-secret`` in ``egg-system``. Happy-path
+  tests skip cleanly when the secret is unreachable; auth-rejection
+  tests don't need it.
+
 * **BRC consensus** (issue #2635) — exercises ``PeerConsensusTracker``
   and the timeout-handler entry points in-process. Does NOT require
   k3s and never calls into the ``egg_stack`` fixture — drives the
@@ -48,14 +61,15 @@
   ``coder`` regression in #2428 fired through. This keeps the test
   green on a fresh CI runner where ``$HOME/repos`` is empty.
 
-All four tiers are marked ``integration`` (via module-level
+All five tiers are marked ``integration`` (via module-level
 ``pytestmark`` in each test file) and run under
 ``make test-integration`` / the ``Test / integration`` CI required
 check. The k3s fixtures only fire when a test takes the ``spawner`` /
 ``egg_stack`` fixtures; the message-bus tests use ``fakeredis`` and
 ``unittest.mock.patch`` for the pipeline state-store and the inner
 context-PR hook; the BRC fixtures are either autouse (tracker
-registry) or opt-in.
+registry) or opt-in; the HITL fixtures are opt-in via
+``lifecycle_bearer`` / ``regression_pipeline_id``.
 
 Plain helper functions (``make_tracker``, ``propose_payload``,
 ``filter_events``, the git/worktree builders, …) live in
@@ -66,6 +80,8 @@
 
 from __future__ import annotations
 
+import base64
+import hashlib
 import json
 import os
 import subprocess
@@ -211,6 +227,105 @@ def lifecycle_auth_headers() -> dict[str, str]:
     return {"Authorization": f"Bearer {_TEST_LIFECYCLE_SECRET}"}
 
 
+# ---------------------------------------------------------------------------
+# HITL HTTP round-trip helpers (#2474, #2634)
+# ---------------------------------------------------------------------------
+
+_LIFECYCLE_SECRET_NAMESPACE = "egg-system"
+_LIFECYCLE_SECRET_NAME = "gateway-secrets"
+_LIFECYCLE_SECRET_KEY = "lifecycle-secret"
+
+
+def deterministic_pipeline_id(test_nodeid: str) -> str:
+    """Return a stable, **syntactically valid** pipeline id from a nodeid.
+
+    The id matches the ``pipeline-{8 hex chars}`` arm of
+    ``state_store.PIPELINE_ID_PATTERN`` — any other shape (e.g. the
+    ``regression-<hex>`` shape from #2474's recovered attempt) trips
+    ``InvalidPipelineIdError`` → 400 before the 404 path runs, masking
+    "pipeline not found" assertions.
+
+    SHA-1 is used as a stable digest, not a cryptographic hash, so the
+    Bandit warning is suppressed.
+    """
+    digest = hashlib.sha1(test_nodeid.encode("utf-8")).hexdigest()  # noqa: S324
+    return f"pipeline-{digest[:8]}"
+
+
+def lifecycle_secret() -> str | None:
+    """Return the orchestrator's ``EGG_LIFECYCLE_SECRET`` if reachable.
+
+    Reads ``gateway-secrets/lifecycle-secret`` from the ``egg-system``
+    namespace. Returns ``None`` if kubectl is missing, the secret is
+    absent, or the value cannot be decoded — callers should
+    ``pytest.skip`` rather than fail in that case so happy-path tests
+    are skipped cleanly when run by a developer without read access on
+    the secret (CI has it).
+    """
+    cmd = [
+        "kubectl",
+        "-n",
+        _LIFECYCLE_SECRET_NAMESPACE,
+        "get",
+        "secret",
+        _LIFECYCLE_SECRET_NAME,
+        "-o",
+        f"jsonpath={{.data.{_LIFECYCLE_SECRET_KEY}}}",
+    ]
+    try:
+        result = subprocess.run(
+            cmd,
+            capture_output=True,
+            text=True,
+            timeout=15,
+            check=False,
+        )
+    except OSError, subprocess.TimeoutExpired:
+        return None
+    if result.returncode != 0 or not result.stdout:
+        return None
+    try:
+        # ``.strip()`` because ``kubectl create secret --from-file`` keeps
+        # every byte of the source file including the trailing newline;
+        # a ``\n`` inside ``f"Bearer {secret}"`` is rejected by
+        # ``http.client.putheader``.
+        return base64.b64decode(result.stdout).decode("utf-8").strip()
+    except ValueError, UnicodeDecodeError:
+        return None
+
+
+@pytest.fixture(scope="session")
+def lifecycle_bearer() -> str:
+    """Return an ``Authorization: Bearer ...`` value or skip the test.
+
+    Used by happy-path tests that need to call
+    ``@require_lifecycle_secret`` endpoints. When the secret is not
+    reachable from the test runner (developer laptop without rbac on
+    the secret) the test is skipped, not failed.
+
+    Session-scoped: the lifecycle secret is a singleton per cluster, so
+    we read it once per pytest session instead of per parametrized
+    case. ``TestHitlResolvePayloadEdgeCases`` alone fans out to 7
+    cases, each of which would otherwise re-shell-out to ``kubectl``
+    with a 15-second timeout — tens of seconds of pure subprocess
+    overhead per run on a slow cluster.
+    """
+    secret = lifecycle_secret()
+    if not secret:
+        pytest.skip(
+            "lifecycle-secret not readable from gateway-secrets in "
+            f"namespace {_LIFECYCLE_SECRET_NAMESPACE} — happy-path "
+            "lifecycle endpoint tests skipped"
+        )
+    return f"Bearer {secret}"
+
+
+@pytest.fixture
+def regression_pipeline_id(request: pytest.FixtureRequest) -> str:
+    """Stable pipeline id derived from the calling test's pytest nodeid."""
+    return deterministic_pipeline_id(request.node.nodeid)
+
+
 # ---------------------------------------------------------------------------
 # k3s slice-spawn helpers (#2632)
 # ---------------------------------------------------------------------------
@@ -471,3 +586,31 @@ def advisory_blocker_graph() -> ReviewGraph:
             ReviewEdge("reviewer_contract", "coder", ReviewCriticality.ADVISORY),
         ]
     )
+
+
+__all__ = [
+    # HITL HTTP round-trip helpers (#2474, #2634).
+    "deterministic_pipeline_id",
+    "lifecycle_bearer",
+    "lifecycle_secret",
+    "regression_pipeline_id",
+    # k3s slice-spawn helpers (#2632). The ``spawner`` fixture is
+    # consumed via pytest injection rather than a direct import, but is
+    # listed here so the public surface mirrors what ``import *`` would
+    # expose and IDE auto-imports / ``dir(conftest)`` stay honest.
+    "env_from_pod",
+    "kubectl_get_pod_yaml",
+    "spawner",
+    # BRC consensus fixtures (#2635). Listed for the same reason as
+    # ``spawner`` — these are pytest-injected, not directly imported,
+    # but belong in the public surface so ``dir(conftest)`` and ``import
+    # *`` reflect the full set. ``_reset_tracker_registry`` is autouse
+    # and ``filter_events`` is both the bare helper from ``_helpers`` and
+    # the fixture name it's exposed under via ``name="filter_events"``.
+    "_reset_tracker_registry",
+    "advisory_blocker_graph",
+    "event_capture",
+    "filter_events",
+    "single_reviewer_graph",
+    "two_reviewer_graph",
+]