diff --git a/integration_tests/regression/README.md b/integration_tests/regression/README.md index e424506825..86d2fd5448 100644 --- a/integration_tests/regression/README.md +++ b/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//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//slice-` and `EGG_SLICE_ID=slice-` 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 diff --git a/integration_tests/regression/__init__.py b/integration_tests/regression/__init__.py index e69de29bb2..3869cb8fc0 100644 --- a/integration_tests/regression/__init__.py +++ b/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. +""" diff --git a/integration_tests/regression/conftest.py b/integration_tests/regression/conftest.py index 4efad7599f..3bb83eabfd 100644 --- a/integration_tests/regression/conftest.py +++ b/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//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-`` 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", +] diff --git a/integration_tests/regression/test_hitl_round_trip.py b/integration_tests/regression/test_hitl_round_trip.py new file mode 100644 index 0000000000..fd52977bcc --- /dev/null +++ b/integration_tests/regression/test_hitl_round_trip.py @@ -0,0 +1,969 @@ +"""Regression tests for the HITL HTTP round-trip (#2634 / #2430). + +#2634 expands integration-test coverage for the human-in-the-loop +pause/resume flow. The full round-trip — pipeline → AWAITING_HUMAN → +``provide_input`` → resume — requires an agent that calls +``register_open_question`` from inside a sandbox pod. That path is +blocked on ScriptedProvider pod-injection (see #2474's constraint +write-up and the ``feedback_scripted_provider_pod_injection`` memo): +deployed agent pods run the real Claude provider with no mechanism to +consume canned trajectories from the test harness. + +So this module pins the part of the round-trip that **is** reachable +from the test runner: the orchestrator's ``/api/v1/pipelines// +decisions/...`` HTTP surface that ``provide_input`` and +``register_open_question`` are layered on. Concretely: + +* Every HITL route is registered on the live blueprint (no + ``404 — route not found``). +* Lifecycle-protected routes (``/resolve``, ``/cancel``) fail closed + with ``401`` or ``503`` for missing / bogus bearers — the #1769 + HITL auto-approval incident parity check. +* Agent-facing routes (queue / list / get / status) reject malformed + payloads with structured ``400``\\ s rather than ``500``\\ s. +* Every route returns the canonical ``{"success": false, + "message": ...}`` envelope on error — a regression that ate the + envelope would let upstream tooling (sdlc-skill, ``provide_input``) + silently treat malformed responses as success. + +Gaps that **require** pod-level provider injection and are out of +scope for this PR (filed as follow-ups when this lands): + +* Pipeline status transition into ``AWAITING_HUMAN`` (driven by an + agent calling ``register_open_question``). +* Resume out of ``AWAITING_HUMAN`` within #2430's bypass deadline. +* HITL during slice phases / HITL combined with ``restart_agent`` / + multiple sequential HITLs on a real pipeline. +* HITL timeout behaviour. +""" + +from __future__ import annotations + +import uuid + +import pytest +import requests + +from integration_tests.regression.conftest import deterministic_pipeline_id + +pytestmark = pytest.mark.integration + + +# --------------------------------------------------------------------------- +# Route registry — the live blueprint must expose every HITL endpoint +# --------------------------------------------------------------------------- + + +def _placeholder_decision_id() -> str: + """A decision id that is syntactically arbitrary but never resolves. + + The handlers' 404 path runs once the auth + body checks pass, so the + id only has to be a non-empty string; it intentionally carries no + structure so an accidental registration of this id in the queue + would not silently make a 404 assertion pass. + """ + return f"missing-decision-{uuid.uuid4().hex[:8]}" + + +# (method, path_template, json_body_factory, requires_lifecycle_secret) +# ``path_template`` uses ``{pipeline_id}`` / ``{decision_id}`` placeholders. +_HITL_ROUTES: list[tuple[str, str, dict | None, bool]] = [ + ("GET", "/api/v1/pipelines/{pipeline_id}/decisions", None, False), + ( + "POST", + "/api/v1/pipelines/{pipeline_id}/decisions", + {"question": "regression-test placeholder"}, + False, + ), + ( + "GET", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}", + None, + False, + ), + ( + "POST", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/resolve", + {"resolution": "regression-test placeholder"}, + True, + ), + ( + "POST", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/cancel", + None, + True, + ), + ("GET", "/api/v1/pipelines/{pipeline_id}/decisions/status", None, False), +] + + +def _format_path(template: str, pipeline_id: str, decision_id: str | None = None) -> str: + """Substitute the placeholders the template actually references. + + ``str.format`` silently ignores extra kwargs, but the asymmetry — + passing ``decision_id`` to a template that never references it — + reads as a bug to anyone scanning the parametrized table. Filter + the kwargs to the placeholders present in the template so the + call site only carries what the template needs. + """ + kwargs: dict[str, str] = {"pipeline_id": pipeline_id} + if "{decision_id}" in template: + if decision_id is None: + raise ValueError( + f"Template {template!r} references {{decision_id}} but none was provided" + ) + kwargs["decision_id"] = decision_id + return template.format(**kwargs) + + +def _call( + orchestrator_url: str, + method: str, + path: str, + *, + json_body: dict | None = None, + headers: dict[str, str] | None = None, + timeout: float = 15.0, +) -> requests.Response: + url = f"{orchestrator_url}{path}" + kwargs: dict = {"timeout": timeout, "headers": headers or {}} + if json_body is not None: + kwargs["json"] = json_body + return requests.request(method, url, **kwargs) + + +def _assert_error_envelope(resp: requests.Response, context: str) -> dict: + """Every error must be ``{"success": false, "message": ...}`` JSON. + + Returns the decoded body so callers can make additional assertions. + """ + try: + body = resp.json() + except ValueError as e: + pytest.fail( + f"{context}: response was not JSON (status={resp.status_code}): " + f"{e}; body[:500]={resp.text[:500]!r}" + ) + assert isinstance(body, dict), f"{context}: response body must be a JSON object: {body!r}" + assert body.get("success") is False, ( + f"{context}: error responses must carry success=false; body was {body!r}" + ) + message = body.get("message") + assert isinstance(message, str) and message, ( + f"{context}: error envelope must include non-empty 'message'; got {message!r}" + ) + return body + + +def _assert_lifecycle_auth_rejected(resp: requests.Response, context: str) -> None: + """Mirror of ``test_k8s_deployment_tools._assert_auth_rejected`` for HITL. + + 503 is acceptable because a deployment that forgot to mount + ``EGG_LIFECYCLE_SECRET`` fails closed by design (see + ``orchestrator/lifecycle_auth.py``). Either outcome demonstrates the + decorator is wired up. 404 specifically must NOT come back here: + that would mean the route was dropped from the blueprint or the + decorator fires AFTER routing (which would re-open #1769's bypass). + """ + assert resp.status_code != 404, ( + f"{context}: route returned 404 — decisions blueprint missing or path renamed (regression)." + ) + assert resp.status_code in (401, 503), ( + f"{context}: expected 401 or 503 (lifecycle auth reject path), " + f"got {resp.status_code}: {resp.text[:500]}" + ) + body = _assert_error_envelope(resp, context) + msg = (body.get("message") or "").lower() + assert any( + hint in msg + for hint in ("authorization", "lifecycle", "egg_lifecycle_secret", "misconfigured") + ), f"{context}: message doesn't look like an auth-layer reject: {msg!r}" + + +class TestHitlRoutesRegistered: + """Every HITL endpoint resolves on the live orchestrator blueprint. + + Regression for a refactor that drops or renames a decisions route: + the decomposition table in ``orchestrator/CLAUDE.md`` calls out + ``_decisions.py`` as a future submodule of the pipelines pre-split, + which is exactly the change shape that historically broke route + registration in #2421. + """ + + @pytest.mark.parametrize( + ("method", "template", "body", "lifecycle"), + _HITL_ROUTES, + ids=[f"{m} {t}" for m, t, _, _ in _HITL_ROUTES], + ) + def test_route_is_not_404( + self, + orchestrator_url: str, + regression_pipeline_id: str, + method: str, + template: str, + body: dict | None, + lifecycle: bool, + ) -> None: + path = _format_path(template, regression_pipeline_id, _placeholder_decision_id()) + # No auth header — the route should reject (auth-required) or + # process and 404 the unknown pipeline. Either way: NOT 404 from + # the Flask routing layer (which would mean the path wasn't bound). + resp = _call(orchestrator_url, method, path, json_body=body) + if lifecycle: + # /resolve and /cancel fire the lifecycle decorator FIRST, + # so an unauthed call returns 401/503 — never 404. + _assert_lifecycle_auth_rejected(resp, f"{method} {path}") + else: + # Agent-facing reads / queue: 404 here means *pipeline* + # not found (the route DID run), which still proves the + # route exists. A blueprint regression would surface as + # a different shape — see the envelope assertion below. + envelope = _assert_error_envelope(resp, f"{method} {path}") + assert resp.status_code in (200, 400, 404), ( + f"{method} {path}: unexpected status {resp.status_code} for " + f"unknown pipeline + minimal body: {resp.text[:500]}" + ) + # Strengthen the 404 branch — Flask's stock 404 for an + # unregistered route also surfaces via the orchestrator's + # ``handle_unhandled_exception`` with the canonical envelope, + # so a dropped route would still pass the envelope check + # above. The handler's pipeline-not-found 404 embeds the + # pipeline id; Flask's routing 404 does not. Assert the id + # is present so a regression that drops one of these routes + # from the blueprint surfaces as a routing 404 missing the + # id rather than a handler 404 carrying it. + if resp.status_code == 404: + assert regression_pipeline_id in (envelope.get("message") or ""), ( + f"{method} {path}: 404 envelope must reference the pipeline id " + f"({regression_pipeline_id!r}) — otherwise this is a Flask " + f"routing 404 (route missing from blueprint), not a handler " + f"404. Got: {envelope!r}" + ) + + +class TestHitlLifecycleAuth: + """#1769 parity for the HITL ``/resolve`` and ``/cancel`` endpoints. + + The HITL auto-approval incident was specifically that lifecycle- + state-changing endpoints accepted unauthenticated calls. Pin the + two routes that mutate decision state here so a future deploy that + drops the decorator surfaces a red CI before reaching prod. + """ + + _LIFECYCLE_ROUTES = [ + ( + "POST", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/resolve", + {"resolution": "regression-test placeholder"}, + ), + ( + "POST", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/cancel", + None, + ), + ] + + @pytest.mark.parametrize( + ("method", "template", "body"), + _LIFECYCLE_ROUTES, + ids=[f"{m} {t}" for m, t, _ in _LIFECYCLE_ROUTES], + ) + def test_no_auth_header_is_rejected( + self, + orchestrator_url: str, + regression_pipeline_id: str, + method: str, + template: str, + body: dict | None, + ) -> None: + path = _format_path(template, regression_pipeline_id, _placeholder_decision_id()) + resp = _call(orchestrator_url, method, path, json_body=body) + _assert_lifecycle_auth_rejected(resp, f"{method} {path} (no auth)") + + @pytest.mark.parametrize( + ("method", "template", "body"), + _LIFECYCLE_ROUTES, + ids=[f"{m} {t}" for m, t, _ in _LIFECYCLE_ROUTES], + ) + def test_invalid_bearer_is_rejected( + self, + orchestrator_url: str, + regression_pipeline_id: str, + method: str, + template: str, + body: dict | None, + ) -> None: + path = _format_path(template, regression_pipeline_id, _placeholder_decision_id()) + resp = _call( + orchestrator_url, + method, + path, + json_body=body, + headers={"Authorization": "Bearer bogus-hitl-test-bearer-do-not-accept"}, + ) + _assert_lifecycle_auth_rejected(resp, f"{method} {path} (bogus bearer)") + + @pytest.mark.parametrize( + ("method", "template", "body"), + _LIFECYCLE_ROUTES, + ids=[f"{m} {t}" for m, t, _ in _LIFECYCLE_ROUTES], + ) + def test_non_bearer_scheme_is_rejected( + self, + orchestrator_url: str, + regression_pipeline_id: str, + method: str, + template: str, + body: dict | None, + ) -> None: + """``Authorization: `` (no ``Bearer `` prefix) must be rejected. + + Exact #1769 incident shape — the decorator must require the + ``Bearer `` prefix even if the bare secret value would otherwise + compare equal. + """ + path = _format_path(template, regression_pipeline_id, _placeholder_decision_id()) + # Even with a real-looking-but-prefix-less header value, the + # decorator must reject. The bare value is intentionally not the + # actual secret — we just need to prove the prefix check runs. + resp = _call( + orchestrator_url, + method, + path, + json_body=body, + headers={"Authorization": "this-is-not-prefixed-with-bearer"}, + ) + _assert_lifecycle_auth_rejected(resp, f"{method} {path} (non-Bearer scheme)") + + +class TestHitlUnknownPipelineReturns404: + """Agent-facing HITL reads return a structured 404 for unknown pipelines. + + These routes are intentionally unauthenticated (see + ``orchestrator/lifecycle_auth.py`` rationale: agents legitimately + call them) so the 404 is the only signal that a path-traversal / + state-store-bypass regression has appeared. ``POST /decisions`` + (queue) is also unauthenticated and must surface the same 404 so a + compromised agent can't queue decisions on arbitrary pipeline ids. + """ + + @pytest.mark.parametrize( + ("method", "template", "body"), + [ + ("GET", "/api/v1/pipelines/{pipeline_id}/decisions", None), + ( + "POST", + "/api/v1/pipelines/{pipeline_id}/decisions", + {"question": "regression placeholder"}, + ), + ( + "GET", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}", + None, + ), + ("GET", "/api/v1/pipelines/{pipeline_id}/decisions/status", None), + ], + ids=lambda v: v if isinstance(v, str) else None, + ) + def test_unknown_pipeline_404( + self, + orchestrator_url: str, + regression_pipeline_id: str, + method: str, + template: str, + body: dict | None, + ) -> None: + path = _format_path(template, regression_pipeline_id, _placeholder_decision_id()) + resp = _call(orchestrator_url, method, path, json_body=body) + # The unauthenticated routes hit the state-store lookup, which + # raises PipelineNotFoundError → 404. A regression that bypassed + # the lookup would either 200 (silently fabricating state) or + # 500 (uncaught exception) — both red flags. + assert resp.status_code == 404, ( + f"{method} {path}: expected 404 for unknown pipeline, " + f"got {resp.status_code}: {resp.text[:500]}" + ) + body_dict = _assert_error_envelope(resp, f"{method} {path}") + # The handler embeds the pipeline id in the message — pin it so + # a refactor that drops the diagnostic breaks loudly. We don't + # require the exact phrasing, only that the id surfaces. + assert regression_pipeline_id in (body_dict.get("message") or ""), ( + f"{method} {path}: 404 envelope should reference the pipeline id " + f"({regression_pipeline_id!r}); got {body_dict!r}" + ) + + +class TestHitlQueueDecisionPayloadValidation: + """``POST /decisions`` rejects malformed bodies before touching state. + + The endpoint is unauthenticated — agents in the cluster legitimately + queue decisions — so a regression that lets a bad payload reach + ``DecisionQueue.queue_decision`` would surface as a 500 with no + pipeline-id scope in the error, leaking ``DecisionQueue`` internals + to callers. + """ + + @pytest.mark.parametrize( + ("body", "expected_message_hint"), + [ + ({}, "question"), + ({"question": ""}, "question"), + ( + {"question": "q", "decision_type": "not-a-real-type"}, + "decision_type", + ), + ({"question": "q", "phase": "not-a-real-phase"}, "phase"), + ], + ids=[ + "empty-object", + "empty-question", + "invalid-decision-type", + "invalid-phase", + ], + ) + def test_malformed_queue_payload_400( + self, + orchestrator_url: str, + regression_pipeline_id: str, + body: dict, + expected_message_hint: str, + ) -> None: + path = f"/api/v1/pipelines/{regression_pipeline_id}/decisions" + # Each case sends a JSON dict that is structurally well-formed + # but rejected by the handler's body validation. + resp = _call(orchestrator_url, "POST", path, json_body=body) + assert resp.status_code == 400, ( + f"POST {path} body={body!r}: expected 400, got {resp.status_code}: {resp.text[:500]}" + ) + env = _assert_error_envelope(resp, f"POST {path} body={body!r}") + assert expected_message_hint.lower() in (env.get("message") or "").lower(), ( + f"POST {path}: error message should mention {expected_message_hint!r}; got {env!r}" + ) + + def test_no_body_rejected_at_or_before_handler( + self, + orchestrator_url: str, + regression_pipeline_id: str, + ) -> None: + """A POST with no body must not crash the handler. + + Flask returns 415 when no ``application/json`` Content-Type is + present, so the orchestrator handler never even sees the + request. Either 400 (handler-level "Missing request body") or + 415 (Flask-level) is fine — both are structured rejections; + what we care about is "not 500" and "envelope present" (415s + from Flask still carry the canonical envelope because Werkzeug + renders them via the orchestrator's error handlers). + """ + path = f"/api/v1/pipelines/{regression_pipeline_id}/decisions" + # No json=, no Content-Type — Flask 415 path. + resp = requests.post(f"{orchestrator_url}{path}", timeout=15) + assert resp.status_code in (400, 415), ( + f"POST {path} no-body: expected 400 or 415, got {resp.status_code}: {resp.text[:500]}" + ) + _assert_error_envelope(resp, f"POST {path} no-body") + + +class TestHitlResolveRequiresResolution: + """``/resolve`` requires a non-empty ``resolution`` field. + + With a valid bearer the handler's first validation is "Missing + resolution" → 400. Without a bearer we hit the lifecycle decorator + instead (covered by ``TestHitlLifecycleAuth``). This test pins the + happy-path validation order so a refactor that swapped the checks + (e.g. running body validation BEFORE the decorator and leaking + decision-existence info via differential responses) would surface. + """ + + def test_resolve_without_resolution_field_with_auth( + self, + orchestrator_url: str, + regression_pipeline_id: str, + lifecycle_bearer: str, + ) -> None: + path = ( + f"/api/v1/pipelines/{regression_pipeline_id}/decisions/" + f"{_placeholder_decision_id()}/resolve" + ) + resp = _call( + orchestrator_url, + "POST", + path, + json_body={}, + headers={"Authorization": lifecycle_bearer}, + ) + # 400 (missing resolution) is the expected response. 404 here + # would mean the body validation runs AFTER state-store lookup + # — that ordering would let a polled-then-deleted decision id + # leak through with a different status code than a never-existed + # one. + assert resp.status_code == 400, ( + f"POST {path} with auth + empty body: expected 400 " + f"(missing resolution), got {resp.status_code}: {resp.text[:500]}" + ) + env = _assert_error_envelope(resp, f"POST {path} (empty body)") + assert "resolution" in (env.get("message") or "").lower(), ( + f"POST {path}: error message should mention resolution; got {env!r}" + ) + + +class TestHitlPipelineIdValidation: + """Path-traversal / malformed pipeline IDs are rejected at the route boundary. + + ``state_store._validate_pipeline_id`` enforces a strict regex + (``issue-N`` / ``pr-N`` / ``pipeline-`` / ``KA-N`` / + ``local-``). Anything else — including the dotted / + slashed shapes a path-traversal attempt would use — raises + ``InvalidPipelineIdError``, which the handlers must surface as a + structured 400 rather than letting it bubble to a 500. + """ + + # Each value is rejected by the regex in ``state_store.py``. We + # explicitly include shapes that historically tripped path-traversal + # bugs (``..`` segments) plus the trivially-empty-segment case. + @pytest.mark.parametrize( + "bad_pipeline_id", + [ + "..", + "../etc/passwd", + "foo/bar", + "regression-too-short", + "issue-", + "issue-abc", + ], + ids=lambda v: v, + ) + def test_invalid_pipeline_id_400( + self, + orchestrator_url: str, + bad_pipeline_id: str, + ) -> None: + # Use the unauthenticated GET list route — the same validation + # runs uniformly across the blueprint, so one route is enough + # to pin the contract. + path = f"/api/v1/pipelines/{bad_pipeline_id}/decisions" + resp = _call(orchestrator_url, "GET", path) + # 400 here proves InvalidPipelineIdError reached the handler's + # error mapper. A 404 would mean the regex check was bypassed + # and the handler hit PipelineNotFoundError instead — which + # masks the path-traversal-attempt signal in operator logs. + # Flask's path matcher returns 404 for ``..``-bearing URIs + # before reaching our handler, so accept 404 only for the + # path-traversal candidates that the URL parser itself rejects. + if ".." in bad_pipeline_id or "/" in bad_pipeline_id: + assert resp.status_code in (400, 404), ( + f"GET {path}: expected 400 or 404 for path-traversal-shaped " + f"id; got {resp.status_code}: {resp.text[:500]}" + ) + # If the URL parser rejected it (404), the message must NOT + # look like a pipeline-not-found ("pipeline … not found"). + # Otherwise a regression where the regex check is removed + # and Flask routes the literal ``..`` segment to the + # state-store would silently pass this test. + if resp.status_code == 404: + env = _assert_error_envelope(resp, f"GET {path}") + msg = (env.get("message") or "").lower() + assert "not found" not in msg or "pipeline" not in msg, ( + f"GET {path}: 404 envelope looks like a pipeline-not-found " + f"reply for a path-traversal id — the regex check was " + f"bypassed and Flask routed the literal segment to the " + f"state-store handler. Got: {env!r}" + ) + else: + assert resp.status_code == 400, ( + f"GET {path}: expected 400 for invalid pipeline_id; " + f"got {resp.status_code}: {resp.text[:500]}" + ) + env = _assert_error_envelope(resp, f"GET {path}") + assert "pipeline" in (env.get("message") or "").lower(), ( + f"GET {path}: error message should mention pipeline; got {env!r}" + ) + + +class TestHitlHttpMethodEnforcement: + """HITL routes only accept the methods their handlers declare. + + Flask's MethodView routing returns 405 (with the canonical + Werkzeug error envelope) for unmatched methods. Pin this so a + refactor that silently widened a route's method list — e.g. + accidentally adding ``DELETE`` to ``/decisions`` because of a + blueprint-level ``methods=["GET", "POST", "DELETE"]`` typo — fails + loud rather than silent. + """ + + # (method, path_template) pairs that the blueprint MUST reject. + # Each pair targets a route that exists in ``_HITL_ROUTES`` but with + # the wrong HTTP method. + _DISALLOWED = [ + ("DELETE", "/api/v1/pipelines/{pipeline_id}/decisions"), + ("PUT", "/api/v1/pipelines/{pipeline_id}/decisions"), + ("PATCH", "/api/v1/pipelines/{pipeline_id}/decisions"), + ( + "DELETE", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}", + ), + ( + "PUT", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}", + ), + ( + "PATCH", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}", + ), + ( + "GET", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/resolve", + ), + ( + "DELETE", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/resolve", + ), + ( + "GET", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/cancel", + ), + ( + "DELETE", + "/api/v1/pipelines/{pipeline_id}/decisions/{decision_id}/cancel", + ), + ("POST", "/api/v1/pipelines/{pipeline_id}/decisions/status"), + ] + + @pytest.mark.parametrize( + ("method", "template"), + _DISALLOWED, + ids=[f"{m} {t}" for m, t in _DISALLOWED], + ) + def test_disallowed_method_405( + self, + orchestrator_url: str, + regression_pipeline_id: str, + method: str, + template: str, + ) -> None: + path = _format_path(template, regression_pipeline_id, _placeholder_decision_id()) + resp = _call(orchestrator_url, method, path) + assert resp.status_code == 405, ( + f"{method} {path}: expected 405 (method not allowed), got " + f"{resp.status_code}: {resp.text[:500]}" + ) + + +class TestHitlMalformedJsonBody: + """Bodies that claim ``application/json`` but aren't valid JSON. + + Flask's ``get_json(silent=False)`` raises ``BadRequest`` here, which + Werkzeug renders as 400. The handler downstream uses ``request.get_json()`` + without ``silent=`` — if that ever switched to ``silent=True``, + invalid JSON would silently parse as ``None`` and the "Missing + request body" path would shadow the JSON-decode failure, losing + operator-visible diagnostic in the audit log. + """ + + @pytest.mark.parametrize( + "raw_body", + [ + "this is not json", + "{not: valid", + "{", + '"unterminated string', + ], + ids=["plain-text", "unclosed-brace", "single-brace", "unterminated-string"], + ) + def test_invalid_json_with_json_content_type_400( + self, + orchestrator_url: str, + regression_pipeline_id: str, + raw_body: str, + ) -> None: + path = f"/api/v1/pipelines/{regression_pipeline_id}/decisions" + resp = requests.post( + f"{orchestrator_url}{path}", + data=raw_body.encode("utf-8"), + headers={"Content-Type": "application/json"}, + timeout=15, + ) + # Flask's bad-JSON handler returns 400 (the body is + # ``application/json`` but doesn't parse). Werkzeug 3.x uses the + # default error renderer, which the orchestrator overrides to + # the canonical envelope — so 400 with a JSON body is the + # expected shape. We don't probe the exact message because + # Werkzeug's wording changes between minor versions. + assert resp.status_code == 400, ( + f"POST {path} body={raw_body!r}: expected 400, got " + f"{resp.status_code}: {resp.text[:500]}" + ) + # Pin the canonical envelope here too — a regression where the + # JSON-decode error escapes the app-level error mapper (e.g. + # ``handle_unhandled_exception`` skipped for ``BadRequest``) + # would surface as a missing ``success: false`` envelope. + _assert_error_envelope(resp, f"POST {path} body={raw_body!r}") + + @pytest.mark.parametrize( + "non_object_json", + [ + # Truthy non-dicts: were 500s pre-#2656 because ``data.get`` + # raised AttributeError on the list/scalar. + pytest.param("[1, 2, 3]", id="array"), + pytest.param('"a string body"', id="string"), + pytest.param("42", id="number"), + pytest.param("true", id="bool"), + # Falsy non-dicts: pre-#2656 were coerced to ``{}`` by the + # ``or {}`` guard and surfaced as "Missing question" — same + # status (400) but a misleading diagnostic. The handler now + # rejects these with the explicit "Request body must be a + # JSON object" 400 too. + pytest.param("[]", id="empty-array"), + pytest.param("0", id="zero"), + pytest.param("false", id="bool-false"), + pytest.param('""', id="empty-string"), + # ``null`` deserialises to ``None``, which the handler + # still treats as "no body" — coerces to ``{}`` and the + # ``Missing question`` 400 branch catches it. Pinning it + # guards that coercion path so a refactor that drops the + # ``raw is None`` short-circuit doesn't regress to + # AttributeError on ``None.get(...)``. + pytest.param("null", id="null"), + ], + ) + def test_non_object_json_body_400( + self, + orchestrator_url: str, + regression_pipeline_id: str, + non_object_json: str, + ) -> None: + """A syntactically-valid JSON body that isn't a dict. + + Fix for #2656: ``queue_decision`` previously did ``data = + request.get_json() or {}`` then ``data.get("question")`` — when + ``data`` was a truthy list / scalar, ``.get`` raised + ``AttributeError`` and the handler's generic ``except + Exception`` mapper returned 500. Falsy non-dicts (``[]``, + ``0``, ``false``, ``""``) were coerced to ``{}`` and surfaced + as "Missing question" — a 400 but with a misleading message. + The handler now rejects all non-object bodies (truthy and + falsy) with an explicit "Request body must be a JSON object" + 400 before any ``.get`` call. + """ + path = f"/api/v1/pipelines/{regression_pipeline_id}/decisions" + resp = requests.post( + f"{orchestrator_url}{path}", + data=non_object_json.encode("utf-8"), + headers={"Content-Type": "application/json"}, + timeout=15, + ) + assert resp.status_code == 400, ( + f"POST {path} body={non_object_json!r}: expected 400 " + f"(non-object body), got {resp.status_code}: {resp.text[:500]}" + ) + _assert_error_envelope(resp, f"POST {path} body={non_object_json!r}") + + +class TestHitlResolvePayloadEdgeCases: + """Edge-case ``resolution`` values exercised via the live HTTP path. + + ``resolve_decision`` normalises non-string resolutions to a JSON + string (``json.dumps(resolution)``) and rejects empty/missing ones + with 400. Once those checks pass, the handler hits the state-store + lookup, which 404s for our placeholder decision id — so each test + here pins what happens *before* the lookup runs. + """ + + @pytest.mark.parametrize( + ("body", "expect_validation_error"), + [ + ({"resolution": None}, True), + ({"resolution": ""}, True), + ({"resolution": " "}, False), # whitespace bypasses ``if not`` + ({"resolution": {"opt": "a"}}, False), # dict → json.dumps + ({"resolution": ["a", "b"]}, False), # list → json.dumps + ({"resolution": 42}, False), # int passes the truthiness check + ({"resolution": False}, True), # falsy primitive + ], + ids=[ + "explicit-null", + "empty-string", + "whitespace-only", + "dict", + "list", + "int", + "bool-false", + ], + ) + def test_resolution_edge_case( + self, + orchestrator_url: str, + regression_pipeline_id: str, + lifecycle_bearer: str, + body: dict, + expect_validation_error: bool, + ) -> None: + path = ( + f"/api/v1/pipelines/{regression_pipeline_id}/decisions/" + f"{_placeholder_decision_id()}/resolve" + ) + resp = _call( + orchestrator_url, + "POST", + path, + json_body=body, + headers={"Authorization": lifecycle_bearer}, + ) + if expect_validation_error: + # The ``if not resolution`` branch catches None, "", 0, False. + # Anything else passes validation and falls through to the + # state-store lookup, which 404s for our placeholder + # decision id. + assert resp.status_code == 400, ( + f"POST {path} body={body!r}: expected 400 " + f"(missing resolution), got {resp.status_code}: " + f"{resp.text[:500]}" + ) + env = _assert_error_envelope(resp, f"POST {path}") + assert "resolution" in (env.get("message") or "").lower(), ( + f"POST {path}: 400 envelope should mention resolution; got {env!r}" + ) + else: + # Validation passed → lookup → 404. Crucially, NOT 500: + # the handler must not bubble TypeError / AttributeError + # from the json.dumps normalisation path. A regression that + # removed the ``isinstance(resolution, str)`` check before + # passing to ``queue.resolve_decision`` would surface as a + # 500 here on dict/list bodies. + assert resp.status_code == 404, ( + f"POST {path} body={body!r}: expected 404 " + f"(pipeline or decision not found, validation passed), " + f"got {resp.status_code}: {resp.text[:500]}" + ) + env = _assert_error_envelope(resp, f"POST {path}") + # The pipeline check fires before the decision lookup, so + # an unknown pipeline surfaces "Pipeline … not found"; an + # unknown decision on a real pipeline would surface + # "Decision … not found". Both are acceptable here — we + # don't have real-pipeline infra (filed as #2657) so we + # observe the pipeline-not-found path. The point of the + # assertion is that the envelope names what was missing, + # not which check fired first. + msg = (env.get("message") or "").lower() + assert "pipeline" in msg or "decision" in msg, ( + f"POST {path}: 404 envelope should reference the missing " + f"pipeline or decision; got {env!r}" + ) + + +class TestHitlCancelOnUnknownDecision: + """``/cancel`` with valid auth on a missing decision returns a structured 404. + + The handler is body-less (no payload to validate) so the path + immediately reaches the state-store lookup after auth. The 404 + envelope must carry the decision id so an operator can trace which + cancel call missed. + """ + + def test_cancel_unknown_decision_404( + self, + orchestrator_url: str, + regression_pipeline_id: str, + lifecycle_bearer: str, + ) -> None: + decision_id = _placeholder_decision_id() + path = f"/api/v1/pipelines/{regression_pipeline_id}/decisions/{decision_id}/cancel" + resp = _call( + orchestrator_url, + "POST", + path, + headers={"Authorization": lifecycle_bearer}, + ) + # Pipeline doesn't exist → 404 via PipelineNotFoundError. + # If the pipeline DID exist we'd 404 via DecisionNotFoundError + # with the decision id in the message. Both paths must surface + # the envelope; we only have the no-pipeline path reachable + # without real-pipeline infra (filed as a follow-up). + assert resp.status_code == 404, ( + f"POST {path} with auth: expected 404, got {resp.status_code}: {resp.text[:500]}" + ) + env = _assert_error_envelope(resp, f"POST {path}") + # The pipeline-not-found message references the pipeline id; + # a refactor that consolidated 404 paths without preserving the + # id would break operator triage. + assert regression_pipeline_id in (env.get("message") or ""), ( + f"POST {path}: 404 envelope should reference the pipeline id; got {env!r}" + ) + + +class TestHitlOversizedPayload: + """The orchestrator must reject pathologically large bodies cleanly. + + Werkzeug enforces ``MAX_CONTENT_LENGTH`` if set; without it the + handler still has to cope. The unauthenticated ``POST /decisions`` + is the most exposed surface (anything in-cluster can queue) so a + DoS-shaped body — 5 MB of JSON — must surface as a structured 4xx, + never a 500 or a hang past our test timeout. + """ + + def test_large_question_payload( + self, + orchestrator_url: str, + regression_pipeline_id: str, + ) -> None: + # ~5 MB question — big enough to stress the handler without + # making the test runner OOM under parallel execution. The + # field has no documented max; if the handler accepts this + # silently, a follow-up should add a cap. The point of this + # test is "doesn't hang, doesn't 500" — any structured + # 4xx/2xx response inside the 30-second budget is acceptable. + huge_question = "x" * (5 * 1024 * 1024) + path = f"/api/v1/pipelines/{regression_pipeline_id}/decisions" + resp = _call( + orchestrator_url, + "POST", + path, + json_body={"question": huge_question}, + timeout=30, + ) + assert resp.status_code < 500, ( + f"POST {path} with 5 MB body: handler must not 500; got " + f"{resp.status_code}: {resp.text[:500]}" + ) + # The body shape is JSON for any 4xx the handler emits; on a + # 413 from Werkzeug we'd still expect JSON because the + # orchestrator's error mapper renders it. + if resp.status_code >= 400: + _assert_error_envelope(resp, f"POST {path} (5 MB body)") + + +def test_deterministic_pipeline_id_is_syntactically_valid() -> None: + """The helper's output must pass ``state_store.validate_pipeline_id``. + + A regression here would silently turn every 404 assertion in this + module into a 400 assertion — the entire HITL-route coverage above + would still pass with no real signal. The recovered #2474 attempt + used ``regression-<12hex>`` which trips this exact failure mode and + is part of why that branch was abandoned. + + Calls the real validator from ``state_store`` rather than + re-implementing the regex inline — a hand-typed copy would drift + silently the next time ``PIPELINE_ID_PATTERN`` (or + ``deterministic_pipeline_id`` itself) gains a new shape, which is + the exact failure mode this test exists to prevent, just one + level up. + """ + from state_store import InvalidPipelineIdError, validate_pipeline_id + + # Sample a handful of nodeids to make sure we don't accidentally + # emit a shape that only works for one input. + samples = [ + "integration_tests/regression/test_hitl_round_trip.py::test_a", + "integration_tests/regression/test_hitl_round_trip.py::test_b", + "tests/x[param-1]", + ] + for nodeid in samples: + pid = deterministic_pipeline_id(nodeid) + try: + validate_pipeline_id(pid) + except InvalidPipelineIdError as exc: + pytest.fail( + f"deterministic_pipeline_id({nodeid!r}) = {pid!r} does not pass " + f"state_store.validate_pipeline_id — 404 assertions would silently " + f"turn into 400 assertions in regression tests. {exc}" + ) diff --git a/orchestrator/routes/__init__.py b/orchestrator/routes/__init__.py index aad6113b2b..49fff798cb 100644 --- a/orchestrator/routes/__init__.py +++ b/orchestrator/routes/__init__.py @@ -185,8 +185,18 @@ def get_state_store_for_pipeline(pipeline_id: str) -> tuple["StateStore", "Pipel PipelineNotFoundError, discover_repo_paths, get_state_store, + validate_pipeline_id, ) + # Validate format before any repo lookup so InvalidPipelineIdError is + # always raised for malformed IDs even when no repos are discovered + # (e.g. in k8s where EGG_REPO_PATH may point to an empty directory). + # This is intentionally redundant with the validation inside + # ``_get_pipeline_path``: the downstream check still runs once we + # find a repo, but the empty-repo-set path would otherwise raise + # ``PipelineNotFoundError`` and mask the real cause. + validate_pipeline_id(pipeline_id) + base_path = get_repo_path() # Fast path: base_path is itself a git repo diff --git a/orchestrator/routes/anchors.py b/orchestrator/routes/anchors.py index 34f923acf3..861bc87de9 100644 --- a/orchestrator/routes/anchors.py +++ b/orchestrator/routes/anchors.py @@ -104,8 +104,10 @@ def create_or_update_anchor(agent_id: str) -> tuple[Response, int]: return _make_error(agent_id_error) body = request.get_json() - if not body: + if body is None: return _make_error("Missing request body") + if not isinstance(body, dict): + return _make_error("Request body must be a JSON object") # Validate agent_id consistency between URL and body body_agent_id = body.get("agent_id") @@ -288,7 +290,10 @@ def gc_anchors(pipeline_id: str) -> tuple[Response, int]: For completed pipelines: archive to checkpoint then clear from Redis. For failed pipelines: set 7-day TTL. """ - body = request.get_json() or {} + raw = request.get_json() + if raw is not None and not isinstance(raw, dict): + return _make_error("Request body must be a JSON object") + body = raw if raw is not None else {} pipeline_status = body.get("status", "completed") r = _get_redis() diff --git a/orchestrator/routes/containers.py b/orchestrator/routes/containers.py index 698f79494b..8b2cf25449 100644 --- a/orchestrator/routes/containers.py +++ b/orchestrator/routes/containers.py @@ -128,7 +128,10 @@ def spawn_container(pipeline_id: str) -> tuple[Response, int]: } } """ - data = request.get_json() or {} + raw = request.get_json() + if raw is not None and not isinstance(raw, dict): + return make_error_response("Request body must be a JSON object") + data = raw if raw is not None else {} # Parse agent role agent_role = None @@ -375,7 +378,10 @@ def stop_container(pipeline_id: str, container_id: str) -> tuple[Response, int]: } } """ - data = request.get_json() or {} + raw = request.get_json() + if raw is not None and not isinstance(raw, dict): + return make_error_response("Request body must be a JSON object") + data = raw if raw is not None else {} timeout = data.get("timeout", 10) try: diff --git a/orchestrator/routes/decisions.py b/orchestrator/routes/decisions.py index d1ecb7ef20..01bd2f9d75 100644 --- a/orchestrator/routes/decisions.py +++ b/orchestrator/routes/decisions.py @@ -551,7 +551,10 @@ def queue_decision(pipeline_id: str) -> tuple[Response, int]: } } """ - data = request.get_json() or {} + raw = request.get_json() + if raw is not None and not isinstance(raw, dict): + return make_error_response("Request body must be a JSON object") + data = raw if raw is not None else {} question = data.get("question") if not question: @@ -710,7 +713,10 @@ def resolve_decision(pipeline_id: str, decision_id: str) -> tuple[Response, int] } } """ - data = request.get_json() or {} + raw = request.get_json() + if raw is not None and not isinstance(raw, dict): + return make_error_response("Request body must be a JSON object") + data = raw if raw is not None else {} resolution = data.get("resolution") if not resolution: diff --git a/orchestrator/routes/health.py b/orchestrator/routes/health.py index 7226f03283..eea28ca879 100644 --- a/orchestrator/routes/health.py +++ b/orchestrator/routes/health.py @@ -365,7 +365,10 @@ def resolve_pipeline_health_alerts(pipeline_id: str) -> tuple[Response, int]: if monitor is None: return jsonify({"success": False, "error": "Health monitor not initialized"}), 503 - data = request.get_json() or {} + raw = request.get_json() + if raw is not None and not isinstance(raw, dict): + return jsonify({"success": False, "error": "Request body must be a JSON object"}), 400 + data = raw if raw is not None else {} agent_id = data.get("agent_id") alert_type = data.get("alert_type") diff --git a/orchestrator/routes/messages.py b/orchestrator/routes/messages.py index b1abf28c98..f266285e21 100644 --- a/orchestrator/routes/messages.py +++ b/orchestrator/routes/messages.py @@ -155,8 +155,10 @@ def send_message(pipeline_id: str) -> tuple[Response, int]: questions atomically with the review verdict. """ body = request.get_json() - if not body: + if body is None: return _make_error("Missing request body") + if not isinstance(body, dict): + return _make_error("Request body must be a JSON object") from_role = body.get("from_role") if not from_role: @@ -571,7 +573,10 @@ def post_heartbeat(pipeline_id: str) -> tuple[Response, int]: TASK-3-4. Slice-scoping (#2471) keeps sibling slices that share a role from sharing each other's rate budget. """ - body = request.get_json() or {} + raw = request.get_json() + if raw is not None and not isinstance(raw, dict): + return _make_error("Request body must be a JSON object") + body = raw if raw is not None else {} from_role = body.get("from_role") if not from_role: diff --git a/orchestrator/state_store.py b/orchestrator/state_store.py index 8005e59bfe..2acd6c27ef 100644 --- a/orchestrator/state_store.py +++ b/orchestrator/state_store.py @@ -109,6 +109,12 @@ def _validate_pipeline_id(pipeline_id: str) -> None: raise InvalidPipelineIdError(f"Invalid pipeline ID format: {pipeline_id}") +# Public alias: ``_validate_pipeline_id`` predates the cross-module callers in +# ``routes/__init__.py``. Underscore-prefixed names should stay internal, so +# importers (production code, tests) can use ``validate_pipeline_id`` instead. +validate_pipeline_id = _validate_pipeline_id + + class StateStore: """Git-backed state store for pipeline state. diff --git a/orchestrator/tests/test_anchors_routes.py b/orchestrator/tests/test_anchors_routes.py index debba83e31..96bcc919d4 100644 --- a/orchestrator/tests/test_anchors_routes.py +++ b/orchestrator/tests/test_anchors_routes.py @@ -309,3 +309,72 @@ def test_gc_failed_pipeline_sets_ttl(self, client, mock_redis): ) assert response.status_code == 200 mock_redis.expire.assert_called() + + +class TestNonObjectJsonBodyReturns400: + """Sweep of the #2656 fix into the anchors routes (PR #2645). + + ``create_or_update_anchor`` and ``gc_anchors`` previously did + ``data = request.get_json() or {}`` then ``data.get(...)``. When the + body was syntactically-valid JSON but not an object (list / scalar), + ``.get`` raised ``AttributeError`` and the handler's generic + ``except Exception`` mapper returned 500. Both handlers now reject + non-dict bodies with ``400 Request body must be a JSON object`` + before any ``.get`` call, mirroring the original decisions-route fix. + """ + + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_create_anchor_non_object_json_body_returns_400(self, client, raw_body): + """POST /anchors/ with non-object JSON body returns 400.""" + response = client.post( + "/api/v1/anchors/coder-abc12345", + content_type="application/json", + data=raw_body, + ) + assert response.status_code == 400, response.data + body = response.get_json() + assert body["success"] is False + assert "json object" in body["message"].lower(), body + + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_gc_anchors_non_object_json_body_returns_400(self, client, raw_body): + """POST /anchors/gc/ with non-object JSON body returns 400. + + The guard runs before any Redis call so a non-dict body must + reject regardless of whether the pipeline has anchors. + """ + response = client.post( + "/api/v1/anchors/gc/issue-1032", + content_type="application/json", + data=raw_body, + ) + assert response.status_code == 400, response.data + body = response.get_json() + assert body["success"] is False + assert "json object" in body["message"].lower(), body diff --git a/orchestrator/tests/test_containers_routes.py b/orchestrator/tests/test_containers_routes.py new file mode 100644 index 0000000000..093c48f29e --- /dev/null +++ b/orchestrator/tests/test_containers_routes.py @@ -0,0 +1,118 @@ +"""Tests for container API routes (orchestrator/routes/containers.py). + +The container-spawner / monitor / backend integrations are covered by +``test_container_spawner*.py`` and ``test_container_backend.py``. This +file covers route-level input validation that runs before the backend +is touched — specifically the #2656 sweep landed in PR #2645. +""" + +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +# Add orchestrator and shared to path +_orchestrator_path = Path(__file__).parent.parent +if str(_orchestrator_path) not in sys.path: + sys.path.insert(0, str(_orchestrator_path)) + +_shared_path = Path(__file__).parent.parent.parent / "shared" +if _shared_path.exists() and str(_shared_path) not in sys.path: + sys.path.insert(0, str(_shared_path)) + + +@pytest.fixture +def app(): + """Create a test Flask app with the containers blueprint.""" + from flask import Flask + from routes.containers import containers_bp + + app = Flask(__name__) + app.register_blueprint(containers_bp) + app.config["TESTING"] = True + yield app + + +@pytest.fixture +def client(app): + """Create a test client. Lifecycle auth is injected by the + orchestrator-level ``_inject_lifecycle_auth`` autouse fixture.""" + return app.test_client() + + +class TestNonObjectJsonBodyReturns400: + """Sweep of the #2656 fix into the containers routes (PR #2645). + + ``spawn_container`` and ``stop_container`` previously did + ``data = request.get_json() or {}`` then ``data.get(...)``. When the + body was syntactically-valid JSON but not an object (list / scalar), + ``.get`` raised ``AttributeError`` and the handler's generic + exception mapper returned 500. Both handlers now reject non-dict + bodies with ``400 Request body must be a JSON object`` before any + ``.get`` call, mirroring the original decisions-route fix. + + Both routes sit behind ``@require_lifecycle_secret``; the + autouse ``_inject_lifecycle_auth`` fixture in + ``orchestrator/tests/conftest.py`` injects the bearer token. The + backend (``_get_backend``) is patched per-test so the body-validation + rejection is never racing a real Docker / Kubernetes call. + """ + + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_spawn_non_object_json_body_returns_400(self, client, raw_body): + """POST /pipelines//spawn with non-object JSON body → 400.""" + with patch("routes.containers._get_backend") as mock_get_backend: + response = client.post( + "/api/v1/pipelines/test-pipeline/spawn", + content_type="application/json", + data=raw_body, + ) + assert response.status_code == 400, response.data + body = response.get_json() + assert body["success"] is False + assert "json object" in body["message"].lower(), body + # Body validation must run before backend dispatch — the + # backend should never have been asked for a handle. + mock_get_backend.assert_not_called() + + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_stop_non_object_json_body_returns_400(self, client, raw_body): + """POST /pipelines//containers//stop with non-object JSON body → 400.""" + with patch("routes.containers._get_backend") as mock_get_backend: + response = client.post( + "/api/v1/pipelines/test-pipeline/containers/abc123/stop", + content_type="application/json", + data=raw_body, + ) + assert response.status_code == 400, response.data + body = response.get_json() + assert body["success"] is False + assert "json object" in body["message"].lower(), body + mock_get_backend.assert_not_called() diff --git a/orchestrator/tests/test_decisions_routes.py b/orchestrator/tests/test_decisions_routes.py index b3c8cf6651..07a3f442eb 100644 --- a/orchestrator/tests/test_decisions_routes.py +++ b/orchestrator/tests/test_decisions_routes.py @@ -467,6 +467,44 @@ def test_empty_body_returns_400(self, client, tmp_path): assert response.status_code == 400 + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_non_object_json_body_returns_400(self, client, tmp_path, raw_body): + """Fix for #2656: non-object JSON bodies must 400, not 500. + + Previously ``data = request.get_json() or {}`` left a list / + scalar in ``data`` and the subsequent ``data.get("question")`` + raised ``AttributeError`` → 500. The handler now rejects + non-dict bodies before any ``.get`` call. + + Falsy non-dicts (``[]``, ``0``, ``false``, ``""``) were + previously coerced to ``{}`` by the ``or {}`` guard and + surfaced as "Missing question"; pinning the explicit + ``Request body must be a JSON object`` message keeps the + diagnostic consistent across truthy and falsy non-dicts. + """ + response = client.post( + "/api/v1/pipelines/test-pipeline/decisions", + content_type="application/json", + data=raw_body, + ) + assert response.status_code == 400, response.data + body = response.get_json() + assert body["success"] is False + assert "json object" in body["message"].lower(), body + @patch("routes.decisions.get_state_store_for_pipeline") @patch("routes.decisions.get_decision_queue") def test_create_with_choice_type_and_options( @@ -570,6 +608,37 @@ def test_resolve_missing_resolution_returns_400(self, client, tmp_path): data = response.get_json() assert data["success"] is False + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_resolve_non_object_json_body_returns_400(self, client, tmp_path, raw_body): + """Resolve mirrors the queue_decision #2656 fix: non-dict body → 400. + + Falsy non-dicts get the same explicit ``Request body must be a + JSON object`` 400 as truthy ones — no fall-through to the + downstream ``Missing resolution`` branch via ``or {}`` coercion. + """ + response = client.post( + "/api/v1/pipelines/test-pipeline/decisions/decision-1/resolve", + content_type="application/json", + data=raw_body, + ) + assert response.status_code == 400, response.data + body = response.get_json() + assert body["success"] is False + assert "json object" in body["message"].lower(), body + @patch("routes.decisions.get_state_store_for_pipeline") @patch("routes.decisions.get_decision_queue") def test_resolve_not_found_returns_404( diff --git a/orchestrator/tests/test_health_routes.py b/orchestrator/tests/test_health_routes.py index 6a86f71b9d..6c6adc0993 100644 --- a/orchestrator/tests/test_health_routes.py +++ b/orchestrator/tests/test_health_routes.py @@ -105,6 +105,49 @@ def test_happy_path_returns_200(self, mock_get_monitor, client): assert data["resolved"] is True mock_monitor.resolve_alerts.assert_called_once_with("coder", "heartbeat_timeout") + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + @patch("health_monitor.get_health_monitor") + def test_non_object_json_body_returns_400(self, mock_get_monitor, client, raw_body): + """Sweep of the #2656 fix into the health routes (PR #2645). + + ``resolve_pipeline_health_alerts`` previously did + ``data = request.get_json() or {}`` then ``data.get(...)``. When + the body was syntactically-valid JSON but not an object (list / + scalar), ``.get`` raised ``AttributeError`` and the handler's + generic exception mapper returned 500. The handler now rejects + non-dict bodies with ``400 Request body must be a JSON object`` + before any ``.get`` call. + + Note: this endpoint's error envelope uses the ``"error"`` key + rather than ``"message"`` (pre-existing inconsistency with the + decisions / messages / anchors / containers routes), so the + assertion targets ``body["error"]`` here. + """ + mock_get_monitor.return_value = MagicMock() + + response = client.post( + "/api/v1/pipelines/test-pipeline/health/alerts/resolve", + content_type="application/json", + data=raw_body, + ) + assert response.status_code == 400, response.data + body = response.get_json() + assert body["success"] is False + assert "json object" in body["error"].lower(), body + class TestHealthEndpointIsolationFromMessageStore: """Issue #1897 TASK-4-3 (regression lock): ``GET /api/v1/health`` MUST diff --git a/orchestrator/tests/test_messages.py b/orchestrator/tests/test_messages.py index 6299f94973..39a7197206 100644 --- a/orchestrator/tests/test_messages.py +++ b/orchestrator/tests/test_messages.py @@ -2951,3 +2951,89 @@ def test_timeout_zero_coerced_to_1s_minimum(self, client, app): "per routes/messages.py:382-385. If this drops to ~0s, " "the silent floor has been removed — update the docstring." ) + + +class TestNonObjectJsonBodyReturns400: + """Sweep of the #2656 fix into the messages routes (PR #2645). + + ``send_message`` and ``post_heartbeat`` previously did + ``data = request.get_json() or {}`` then ``data.get(...)``. When the + body was syntactically-valid JSON but not an object (list / scalar), + ``.get`` raised ``AttributeError`` and the handler's generic + ``except Exception`` mapper returned 500. Both handlers now reject + non-dict bodies with ``400 Request body must be a JSON object`` + before any ``.get`` call, mirroring the original decisions-route fix. + + Falsy non-dicts (``[]``, ``0``, ``false``, ``""``) get the same + explicit message — the previous ``or {}`` coercion path that + surfaced as "Missing from_role" / "Missing state" for falsy + non-dicts is replaced by the explicit envelope. + """ + + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_send_message_non_object_json_body_returns_400(self, client, app, raw_body): + """POST /messages with non-object JSON body returns 400 with the + canonical "Request body must be a JSON object" envelope. + + The state-store mock is patched out because the body-validation + check runs before any pipeline lookup — a non-dict body must + reject even if the pipeline does not exist. + """ + with app.test_request_context(): + with patch("routes.messages.get_state_store_for_pipeline"): + resp = client.post( + "/api/v1/pipelines/test-pipeline/messages", + content_type="application/json", + data=raw_body, + ) + assert resp.status_code == 400, resp.data + body = json.loads(resp.data) + assert body["success"] is False + assert "json object" in body["message"].lower(), body + + @pytest.mark.parametrize( + "raw_body", + ["[1, 2, 3]", '"a string body"', "42", "true", "[]", "0", "false", '""'], + ids=[ + "array", + "string", + "number", + "bool", + "empty-array", + "zero", + "false", + "empty-string", + ], + ) + def test_heartbeat_non_object_json_body_returns_400(self, client, app, raw_body): + """POST /heartbeat with non-object JSON body returns 400 with the + canonical "Request body must be a JSON object" envelope. + + Same invariant as ``send_message``: the guard runs before the + ``from_role`` / ``state`` checks so a list-shaped body doesn't + surface as "Missing from_role". + """ + with app.test_request_context(): + with patch("routes.messages.get_state_store_for_pipeline"): + resp = client.post( + "/api/v1/pipelines/test-pipeline/heartbeat", + content_type="application/json", + data=raw_body, + ) + assert resp.status_code == 400, resp.data + body = json.loads(resp.data) + assert body["success"] is False + assert "json object" in body["message"].lower(), body