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

[jwbron]

Closes #2634.
Closes #2656.

Summary

• Creates integration_tests/regression/ — the cross-module k3s regression tier called out in Expand integration test coverage #2474 but never landed — with a HITL-focused module pinning the parts of the round-trip reachable from the test runner.
• 58 passing invariants on /api/v1/pipelines/<id>/decisions/...: route registration, lifecycle-auth parity, 404 envelope shape, malformed bodies, HTTP method enforcement, payload edge cases, oversized payload, InvalidPipelineIdError mapping.
• Fixes POST /api/v1/pipelines/<id>/decisions returns 500 on non-object JSON bodies #2656 — queue_decision and resolve_decision now reject non-object JSON bodies with the canonical 400 envelope instead of 500; the 4 xfail-marked tests are flipped to PASS and matching unit tests pin the invariant in the inner-loop suite.
• Files Test infra: ephemeral pipeline + pod-level LLM injection for HITL coverage #2657 to capture the infra needed to unblock concurrency / multi-decision / real-round-trip coverage.

What's pinned

Class	Pins
TestHitlRoutesRegistered	All 6 HITL endpoints resolve on the live blueprint.
TestHitlLifecycleAuth	/resolve and /cancel reject missing / bogus / non-Bearer headers — #1769 parity.
TestHitlUnknownPipelineReturns404	Agent-facing routes return canonical 404 envelopes referencing the pipeline id.
TestHitlQueueDecisionPayloadValidation	Missing question / invalid decision_type / invalid phase / no-body → structured 400 (or 415 for no Content-Type), never 500.
TestHitlResolveRequiresResolution	/resolve with auth + empty body → 400, pinning body-validation-after-auth ordering.
TestHitlPipelineIdValidation	Malformed / path-traversal pipeline ids → 400 (InvalidPipelineIdError).
TestHitlHttpMethodEnforcement	DELETE/PUT/PATCH on POST routes, GET on /resolve / /cancel, POST on /status → 405.
TestHitlMalformedJsonBody	Invalid JSON → 400. Non-object JSON (list/scalar) → 400 (#2656 fix). null body coerces correctly.
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).
TestHitlCancelOnUnknownDecision	/cancel with auth on missing decision → 404 envelope with pipeline id.
TestHitlOversizedPayload	5 MB question body doesn't 500 or hang.
test_deterministic_pipeline_id_is_syntactically_valid	Helper emits pipeline-<8hex> so 404 assertions don't silently turn into 400 assertions.

#2656 fix details

queue_decision and resolve_decision 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. queue_decision is unauthenticated by design (agents queue decisions in-cluster), so a misbehaving agent could flood the orchestrator's error logs with stack traces.

Both handlers now reject non-dict bodies with 400 Request body must be a JSON object before any .get call. null still coerces to {} via or {} and falls through to the existing missing-field 400 branches.

The 4 xfail-marked primitive-body cases in TestHitlMalformedJsonBody::test_non_object_json_body_400 now pass on real behavior, so the marks are dropped. New parametrized cases in orchestrator/tests/test_decisions_routes.py (test_non_object_json_body_returns_400, test_resolve_non_object_json_body_returns_400) pin the same invariant in the inner-loop suite.

The issue's spot-check note (same request.get_json() or {} + .get pattern in routes/messages.py, routes/health.py, routes/containers.py, routes/anchors.py) is intentionally NOT swept in this PR — those routes span unrelated subsystems and warrant a separate follow-up.

Coverage gaps (infra-blocked, tracked in #2657)

• Pipeline status transition into AWAITING_HUMAN (needs an agent calling register_open_question).
• Resume out of AWAITING_HUMAN within Overseer container crashlooping at exit_code=1 after long-running pipelines (correlates with slice cascade) #2430's bypass deadline.
• HITL during slice phases / combined with restart_agent / multiple sequential HITLs on a real pipeline.
• HITL timeout behaviour.
• Resolve idempotency on a real decision (409 on second call).
• Concurrent resolve/cancel races on the same decision.
• FIFO ordering of pending decisions.

These need an ephemeral-pipeline fixture (could be unblocked by a lifecycle-authed test-seed endpoint that doesn't need a real repo + gateway round-trip) and pod-level LLM-trajectory injection (the ScriptedProvider gap documented in #2474 and the feedback_scripted_provider_pod_injection memo). #2657 lays out the phasing.

Recovered #2474 attempt — three bugs avoided

The recovered commit 79589b2b8 had a test_hitl_round_trip.py that wouldn't have worked even if it had merged:

1. POSTed to /decisions/<id> (no /resolve suffix) with {"action": "select", "selected": "opt-1"} — actual endpoint is /decisions/<id>/resolve with {"resolution": "..."} and requires lifecycle auth.
2. Used regression-<12hex> pipeline ids, which state_store.PIPELINE_ID_PATTERN rejects with InvalidPipelineIdError → 400 instead of 404 — masking the assertion.
3. Pointed at gateway_url instead of orchestrator_url.

The new conftest's deterministic_pipeline_id helper emits pipeline-<8hex> and a self-test pins the shape.

Test plan

• make test-integration green on local k3s — 182 passed, 20 skipped (pre-existing legacy-docker skips), 4 xfailed (POST /api/v1/pipelines/<id>/decisions returns 500 on non-object JSON bodies #2656).
• After POST /api/v1/pipelines/<id>/decisions returns 500 on non-object JSON bodies #2656 fix: 4 xfails flipped to PASS; unit tests in orchestrator/tests/test_decisions_routes.py green (55 passed).
• make lint green.
• CI required-check Test / aggregate passes on this PR.

[egg-reviewer]

Thorough review of the integration test surface, the #2656 fix, and the _validate_pipeline_id hoist. No blocking issues. The integration assertions match real handler behavior (verified against orchestrator/routes/decisions.py, orchestrator/lifecycle_auth.py, and orchestrator/state_store.py). The except E1, E2: syntax in conftest.py is valid Python 3.14 (PEP 758) given the project's requires-python = ">=3.14". Below are items I would address before/with merge — the first two are the ones I'd prioritize.

Should fix before merge

1. test_deterministic_pipeline_id_is_syntactically_valid duplicates the regex it's supposed to pin against

integration_tests/regression/test_hitl_round_trip.py:1042-1050 re-implements state_store.PIPELINE_ID_PATTERN inline:

pattern = _re.compile(
    r"^("
    r"issue-[0-9]+(-[a-z0-9]+)*"
    r"|[A-Z][A-Z0-9]+-[0-9]+(-[a-z0-9]+)*"
    r"|local-[0-9a-f]{8}"
    r"|pipeline-[0-9a-f]{8}"
    r"|pr-[0-9]+"
    r")$"
)

The test's whole stated purpose is to prevent deterministic_pipeline_id from silently drifting away from the validator (so that 404 assertions don't quietly turn into 400 assertions). But the test now compares against a hand-typed copy of the regex — if state_store.PIPELINE_ID_PATTERN grows a new pipeline-id shape, or deterministic_pipeline_id is updated to emit one, this test keeps using the stale copy and silently passes. That is the exact failure mode the docstring warns about, just one level up.

Fix:

from state_store import PIPELINE_ID_PATTERN
...
for nodeid in samples:
    pid = deterministic_pipeline_id(nodeid)
    assert PIPELINE_ID_PATTERN.match(pid), ...

Same applies to importing _validate_pipeline_id and asserting it doesn't raise — even simpler.

2. lifecycle_bearer shells out to kubectl once per parametrized test

integration_tests/regression/conftest.py:118 declares lifecycle_bearer as function-scoped, and lifecycle_secret() runs kubectl -n egg-system get secret gateway-secrets ... with a 15 s timeout on every invocation. The test classes that depend on it parametrize over multiple bodies — TestHitlResolvePayloadEdgeCases alone fans out to 7 cases. On a slow cluster or a developer laptop without RBAC (where the call ultimately fails after the timeout), this multiplies into tens of seconds of pure subprocess overhead, and pytest.skip in a function-scoped fixture is invoked on each iteration too.

Change to @pytest.fixture(scope="session") — the lifecycle secret is a singleton per cluster, there is no reason to re-read it.

Non-blocking suggestions

3. except OSError, subprocess.TimeoutExpired: — use parens for readability

integration_tests/regression/conftest.py:104 and :114 rely on PEP 758 (parens-less except), which is valid in Python 3.14 but still uncommon enough that many reviewers will read it as Python 2 syntax for except Type as name: on first glance. Linters and IDE syntax highlighters that haven't caught up will also flag it. Prefer the explicit tuple:

except (OSError, subprocess.TimeoutExpired):
    return None

(Same for the ValueError, UnicodeDecodeError clause.)

4. Falsy non-dict bodies bypass the new "JSON object" message

orchestrator/routes/decisions.py:554-556:

data = request.get_json() or {}
if not isinstance(data, dict):
    return make_error_response("Request body must be a JSON object")

For truthy non-dicts ([1,2,3], "a", 42, true) you get the new explicit "Request body must be a JSON object" 400. For falsy non-dicts ([], 0, false, "") the or {} coerces them to {} and the operator gets "Missing question" instead — different message for the same class of error.

The status code is still 400 in all cases, so this is a UX nit rather than a bug, but it would be trivial to make consistent:

raw = request.get_json(silent=True)
if raw is not None and not isinstance(raw, dict):
    return make_error_response("Request body must be a JSON object")
data = raw or {}

The integration test TestHitlMalformedJsonBody::test_non_object_json_body_400 happens to only exercise truthy non-dicts plus null (which goes through the or {} coercion to "Missing question"). It would not detect this inconsistency.

5. _validate_pipeline_id is imported across module boundaries despite its underscore prefix

orchestrator/routes/__init__.py:186 imports a private function:

from state_store import (
    PipelineNotFoundError,
    _validate_pipeline_id,
    ...
)

This works but is a smell — Python treats the underscore prefix as the module's own "do not import" signal. Either promote _validate_pipeline_id to validate_pipeline_id (and update internal callers), or expose a public helper in state_store that wraps the private one. Otherwise a future refactor inside state_store may move/rename _validate_pipeline_id without checking external imports.

6. The hoisted _validate_pipeline_id runs twice in the fast path

orchestrator/routes/__init__.py:194 now calls _validate_pipeline_id(pipeline_id) unconditionally. The downstream store.load_pipeline(pipeline_id) call then runs through _get_pipeline_path which also validates. This is harmless (regex match on a short string), but the comment "Validate format before any repo lookup so InvalidPipelineIdError is always raised for malformed IDs even when no repos are discovered" should call out that the validation is intentionally redundant with the downstream check, so a future reader doesn't "clean up" the duplicate.

7. TestHitlOversizedPayload docstring says "50 MB"; the code uses 5 MB

integration_tests/regression/test_hitl_round_trip.py:989 says "a DoS-shaped body — 50 MB of JSON" but line 1004 uses "x" * (5 * 1024 * 1024) (5 MB). The PR description also says 5 MB. Pick one — either bump the test to 50 MB or fix the docstring.

8. test_invalid_json_with_json_content_type_400 skips the envelope assertion

integration_tests/regression/test_hitl_round_trip.py:782-804 only asserts status_code == 400. Every other malformed-body test in this module calls _assert_error_envelope. Werkzeug's BadRequest is rendered through api.py's handle_unhandled_exception so the canonical envelope should be present — pin it here too:

_assert_error_envelope(resp, f"POST {path} body={raw_body!r}")

This would catch a regression where the JSON-decode error escapes the app-level error mapper.

9. TestHitlRoutesRegistered.test_route_is_not_404 is weaker than its name implies for non-lifecycle routes

test_hitl_round_trip.py:361-364 accepts resp.status_code in (200, 400, 404) for unauthed agent-facing routes. The intent is to prove "the route exists and dispatched into the handler" — but Flask's stock 404 for an unregistered route also has status_code == 404, and the orchestrator's handle_unhandled_exception renders Werkzeug's 404 with the canonical {"success": false, "message": ...} envelope too. So a regression that drops one of these routes from the blueprint would still pass this test.

For the 404 branch specifically, assert that the message references the pipeline id (which the handler embeds but Flask's stock 404 description does not):

if resp.status_code == 404:
    body = _assert_error_envelope(resp, ...)
    assert regression_pipeline_id in (body.get("message") or ""), (
        f"{method} {path}: 404 envelope must reference pipeline id "
        f"(otherwise this is a Flask routing 404, not a handler 404)"
    )

10. test_invalid_pipeline_id_400 for path-traversal IDs accepts either 400 or 404

test_hitl_round_trip.py:676-680 accepts 400 or 404 for .., ../etc/passwd, foo/bar. The comment correctly notes Flask normalises .. before our handler runs, so 404 is from the URL parser, not from InvalidPipelineIdError. But on the 404 branch you should pin that the message does 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.

11. _format_path always passes decision_id even for paths that don't use it

test_hitl_round_trip.py:254-255 — str.format silently ignores extras, so path.format(pipeline_id="x", decision_id="y") on a template without {decision_id} works. This is mostly harmless but reads as a bug to anyone scanning the parametrized table. Either filter the kwargs to the placeholders present in the template, or split into _format_pipeline_path / _format_decision_path.

12. Sweep-tracking for the rest of the request.get_json() or {} + .get pattern

The PR description correctly flags that routes/messages.py, routes/health.py, routes/containers.py, and routes/anchors.py all share the same #2656-shaped pattern and are intentionally out of scope. #2657 (per the description) tracks the HITL-coverage gaps but not this sweep. Either expand #2657 or file a separate follow-up so the sweep doesn't fall through the cracks.

---

The fix for #2656 is correct, the validation hoist in get_state_store_for_pipeline is sound (PipelineNotFoundError was previously masking InvalidPipelineIdError when no repos were discovered — verified against routes/__init__.py post-PR), and the new integration tests do exercise the real production code path through the live orchestrator. The two "should fix" items above are the only ones I'd actually hold the PR for; the rest are quality-of-life improvements.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. All 12 items addressed in-PR — commit a149705. Per-item dispositions below.

Should fix before merge

1. test_deterministic_pipeline_id_is_syntactically_valid duplicates the regex — fixed-in-PR (commit a149705). Now imports and calls state_store.validate_pipeline_id directly. Re-implementing the regex inline was the exact failure mode the test exists to prevent, one level up.

2. lifecycle_bearer shells out per parametrized test — fixed-in-PR (commit a149705). Changed to @pytest.fixture(scope="session") — the secret is a per-cluster singleton.

Non-blocking suggestions

3. except OSError, subprocess.TimeoutExpired: parens — fixed-in-PR (commit a149705). Both clauses now use explicit tuple form except (OSError, subprocess.TimeoutExpired):.

4. Falsy non-dict bodies bypass the new "JSON object" message — fixed-in-PR (commit a149705). Switched to the suggested raw = request.get_json(); if raw is not None and not isinstance(raw, dict): ...; data = raw if raw is not None else {} pattern in queue_decision and resolve_decision. Falsy non-dicts ([], 0, false, "") now get the explicit "Request body must be a JSON object" 400 instead of the misleading "Missing question". Added parametrized cases in test_decisions_routes.py and test_hitl_round_trip.py::test_non_object_json_body_400 to pin the falsy branch.

5. _validate_pipeline_id imported across module boundaries — fixed-in-PR (commit a149705). Added validate_pipeline_id = _validate_pipeline_id public alias in state_store.py; routes/__init__.py now imports the public name. Existing test-side callers of _validate_pipeline_id are left as-is (they live in orchestrator/tests/ so the underscore-crossing is intra-package).

6. Hoisted _validate_pipeline_id runs twice in fast path — fixed-in-PR (commit a149705). Updated the inline comment in get_state_store_for_pipeline to call out that the validation is intentionally redundant with the downstream _get_pipeline_path check, so a future reader doesn't "clean up" the duplicate.

7. TestHitlOversizedPayload docstring says "50 MB"; code uses 5 MB — fixed-in-PR (commit a149705). Docstring corrected to "5 MB".

8. test_invalid_json_with_json_content_type_400 skips envelope assertion — fixed-in-PR (commit a149705). Added _assert_error_envelope(resp, ...) call after the status check.

9. TestHitlRoutesRegistered.test_route_is_not_404 weaker than name implies — fixed-in-PR (commit a149705). Added an inner assertion that 404 messages must reference the pipeline id, so Flask's stock route-missing 404 (which doesn't embed the id) surfaces as a test failure rather than silently passing the envelope check.

10. test_invalid_pipeline_id_400 for path-traversal accepts either 400 or 404 — fixed-in-PR (commit a149705). Added an inner assertion that for the 404 branch the message must NOT look like a pipeline-not-found ("not found" + "pipeline"), so a regression where the regex check is removed and Flask routes the literal segment to the state-store would no longer silently pass.

11. _format_path always passes decision_id — fixed-in-PR (commit a149705). _format_path now takes decision_id: str | None = None and only inserts it when the template references {decision_id}. Raises ValueError if the template needs decision_id but none was provided.

12. Sweep request.get_json() or {} + .get pattern in other routes — fixed-in-PR (commit a149705). Did the sweep inline rather than tracking it: the same raw is not None and not isinstance(raw, dict) guard now sits in routes/messages.py (both send_message and post_heartbeat), routes/health.py (resolve_pipeline_health_alerts), routes/containers.py (spawn_container, stop_container), and routes/anchors.py (create_or_update_anchor, gc_anchors). The fix is mechanical and trivially testable, so bundling it in this PR was cheaper than opening a follow-up.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
integration_tests/regression/conftest.py	Additive (add/add)	Combined both fixture sets in one file

Background. Two PRs landed against the freshly-created integration_tests/regression/ tier in parallel:

• This PR added a conftest.py with HITL-specific helpers (deterministic_pipeline_id, lifecycle_secret, lifecycle_bearer, regression_pipeline_id) for test_hitl_round_trip.py.
• test(brc): add k3s-tier regression coverage for BRC consensus (#2635) #2649 (merged to main) added a conftest.py with BRC consensus fixtures (event_capture, filter_events, single/two_reviewer/advisory_blocker_graph, autouse _reset_tracker_registry) for test_brc_*.py plus a sibling _helpers.py.

The two fixture sets are non-overlapping — different fixtures, different test modules. Resolution kept both in one file, with a combined docstring documenting which subsystem each fixture set serves and which tests consume it. Imports merged: HEAD's base64/hashlib/subprocess + main's sys/Path/path-bootstrap + the in-process orchestrator imports (events, peer_consensus, review_graph, _helpers).

A subsequent commit on the PR branch (a149705cc — review-feedback addressing #2645) had already converted the except clauses to paren form and made lifecycle_bearer session-scoped; that commit auto-merged cleanly on top of the combined conftest with no further conflicts.

Local verification:

• python3 -c "import ast; ast.parse(...)" parses clean.
• make lint clean (only pre-existing soft-cap line-length warnings unrelated to this merge).
• ruff check integration_tests/regression/ — all checks passed.
• All 30 BRC regression tests (test_brc_*.py) pass.
• All 55 orchestrator decisions-routes unit tests pass (the POST /api/v1/pipelines/<id>/decisions returns 500 on non-object JSON bodies #2656 fix's inner-loop coverage).
• All 58 HITL invariants collect cleanly (full run requires k3s — kept for CI).
• 2 unrelated failures in orchestrator/tests/test_commit_statefiles_scoping.py — sandbox blocks git init; these tests need real git repos and predate the merge.

Please review: Nothing semantically risky — this is a straightforward additive merge of two independent fixture sets at the same path. Both consuming test suites should be unaffected.

— Authored by egg

[egg-reviewer]

Re-review of PR #2645 against commit f75cc04 (delta since 60cae820: a149705 review-feedback + d3ab972 main-merge + f75cc04 format-fix).

Previous-review items — verified

#	Item	Status
1	test_deterministic_pipeline_id_is_syntactically_valid duplicates the regex	✅ Fixed — now calls state_store.validate_pipeline_id (test_hitl_round_trip.py:951-969).
2	lifecycle_bearer shells out per parametrized test	✅ Fixed — @pytest.fixture(scope="session") (conftest.py:133).
3	except OSError, subprocess.TimeoutExpired: — use parens	⚠️ See "Disposition note" below — author's response says fixed but the auto-formatter (f75cc04) reverted the parens.
4	Falsy non-dict bodies bypass the "JSON object" message	✅ Fixed — raw is not None and not isinstance(raw, dict) in queue_decision (decisions.py:554-557) and resolve_decision (decisions.py:716-719). Parametrized cases for []/0/false/"" added in both test_hitl_round_trip.py:716-719 and test_decisions_routes.py:472,613.
5	_validate_pipeline_id imported across module boundaries	✅ Fixed — validate_pipeline_id = _validate_pipeline_id alias added (state_store.py:115); routes/__init__.py:188 imports the public name.
6	Hoisted _validate_pipeline_id runs twice in fast path	✅ Fixed — comment in routes/__init__.py:191-197 calls out the intentional redundancy.
7	TestHitlOversizedPayload docstring "50 MB"	✅ Fixed — docstring now says "5 MB" (test_hitl_round_trip.py:900,909,925,932).
8	test_invalid_json_with_json_content_type_400 skips envelope assertion	✅ Fixed — _assert_error_envelope(resp, ...) added at test_hitl_round_trip.py:700.
9	TestHitlRoutesRegistered.test_route_is_not_404 weaker than name implies	✅ Fixed — inner 404-branch assertion at test_hitl_round_trip.py:235-241 requires the envelope message to reference the pipeline id.
10	test_invalid_pipeline_id_400 404-branch under-pinned	✅ Fixed — test_hitl_round_trip.py:563-571 asserts the 404 message must NOT look like pipeline-not-found.
11	_format_path always passes decision_id	✅ Fixed — test_hitl_round_trip.py:100-116 filters kwargs to placeholders the template references.
12	Sweep the request.get_json() or {} + .get pattern across other routes	⚠️ Author swept inline across routes/messages.py, routes/anchors.py, routes/containers.py, routes/health.py — see "New observations" below. The sweep itself is correct but has no test coverage.

Disposition notes / new observations

These are all non-blocking — the core PR is correct and the two should-fix items from the prior review are properly resolved.

A. The except parens fix landed in a149705 but was reverted by the auto-formatter in f75cc04

integration_tests/regression/conftest.py:119,129 is back to the PEP 758 paren-less form (except OSError, subprocess.TimeoutExpired: and except ValueError, UnicodeDecodeError:). I traced the patch chain: a149705 added parens, then f75cc04 ("Fix checks: apply automated formatting fixes") removed them. Ruff 0.15.12 has a PEP 758 auto-fix that the project's lint config is letting through.

The author's response comment to item #3 says "Both clauses now use explicit tuple form except (OSError, subprocess.TimeoutExpired):" — that was true at the time the comment was posted, but no longer reflects the merged state. Not a correctness issue (PEP 758 is valid Python 3.14+, and requires-python = ">=3.14"), but the disposition note in the response is misleading for future archaeology. If you want the parens for readability, the ruff rule that strips them needs to be disabled in pyproject.toml; otherwise close out item #3 honestly as "won't fix — formatter strips parens."

B. Sweep across 4 additional routes has no test coverage

The author bundled the #2656 pattern fix into routes/messages.py (both send_message and post_heartbeat), routes/anchors.py (create_or_update_anchor and gc_anchors), routes/containers.py (spawn_container and stop_container), and routes/health.py (resolve_pipeline_health_alerts). The production-code change is mechanically correct and mirrors the in-scope fix.

But the bundle added zero tests for these routes. test_messages.py, test_anchors_routes.py, test_container_*.py, and test_health_routes.py already exist in orchestrator/tests/ — the natural homes are right there. The author's response acknowledged "the fix is mechanical and trivially testable" but didn't add the trivial test. A future refactor that drops one of these isinstance guards will only be caught by a real client hitting it, not by CI.

These routes are authenticated (in contrast to the unauthenticated queue_decision that motivated #2656), so the log-spam risk is lower. Calling this non-blocking, but I'd add at least one parametrized test_non_object_json_body_returns_400 per swept module before considering this PR's job done — the original review explicitly asked for a tracked follow-up if the sweep wasn't done in-PR, and trading a follow-up for an untested sweep is a worse outcome than either alternative.

C. Behavior change for {} body in send_message / create_or_update_anchor

The sweep changed if not body: to if body is None: + if not isinstance(body, dict):. For body = {}:

• Before: not body was True (empty dict is falsy) → "Missing request body" 400.
• After: body is None is False; isinstance({}, dict) is True → falls through to body.get("from_role") / body.get("agent_id") → "Missing from_role" / "agent_id not in URL/body matches" 400.

Both 400 with descriptive messages — this is a diagnostic improvement, not a regression. Worth a one-line callout in a future commit message; not worth blocking the PR.

D. health.py envelope key is "error" not "message"

routes/health.py:370 (the new line from the sweep) returns {"success": False, "error": "Request body must be a JSON object"}, consistent with neighboring lines 362/366 but inconsistent with decisions.py / messages.py / containers.py / anchors.py which use "message". The _assert_error_envelope helper in the new integration tests expects "message", so a regression-tier test against /health/alerts/resolve written with the canonical envelope would fail.

This is pre-existing inconsistency in health.py (lines 219/315/327 use "message"; 233/239/285/362/366 use "error") — not something this PR introduced. Mentioning in case the maintainer wants to track a follow-up to normalize, but health.py is not exercised by the new test surface so the inconsistency is invisible to this PR's invariants.

Conclusion

All blocking items from the previous review are properly resolved. The #2656 fix is correct end-to-end (production code + unit tests + integration tests). The hoist of validate_pipeline_id is sound and now uses the public alias. Items 7–11 (test rigor) are visibly tighter.

The two real callouts above (A: formatter-reverted parens with a misleading disposition note; B: untested sweep across 4 routes) are non-blocking but worth landing in a follow-up rather than letting them fall through the cracks the same way item #12 originally did.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
integration_tests/regression/conftest.py	Additive	Combined three fixture tiers side-by-side

Both sides added new helpers to the same conftest file without touching each other's logic:

• HEAD added the HITL HTTP round-trip helpers (Expand integration test coverage #2474, Integration test coverage: HITL round-trip #2634): deterministic_pipeline_id, lifecycle_secret, lifecycle_bearer, regression_pipeline_id, plus base64 / hashlib imports.
• origin/main (Integration test coverage: slice spawn & DAG execution #2632 via test(integration): slice spawn + restart regression guards (#2632) #2651) added the k3s slice-spawn helpers: kubectl_get_pod_yaml, env_from_pod, the spawner fixture, plus json / time / typing.Any imports.
• The BRC consensus fixtures (Integration test coverage: BRC consensus protocol #2635) are byte-identical on both sides.

Resolution:

• Merged imports (base64, hashlib, json, time, typing.Any — all needed by their respective sections).
• Rewrote the module docstring to describe all three tiers (HITL HTTP + BRC consensus + k3s slice-spawn) in one place.
• Kept the helper sections side-by-side under explicit # ---- HITL ----, # ---- k3s ----, # ---- BRC ---- banners so each tier's code is locatable.
• Preserved the PR's __all__ export list.

Verification:

• python3 -c "import ast; ast.parse(...)" clean.
• .venv/bin/ruff check integration_tests/regression/conftest.py clean.
• make lint exits 0.
• .venv/bin/pytest orchestrator/tests/test_decisions_routes.py — 63 passed (covers the POST /api/v1/pipelines/<id>/decisions returns 500 on non-object JSON bodies #2656 fix this PR depends on).
• .venv/bin/pytest integration_tests/regression/ (excluding the k3s-requiring tests) — 30 passed, including the regression-tier helper validation test_deterministic_pipeline_id_is_syntactically_valid and all 5 BRC single-cycle invariants from Integration test coverage: BRC consensus protocol #2635.
• The 4 k3s-requiring test modules collect cleanly (97 total tests collected, 0 errors) but were not run locally — they need an egg_stack k3s harness and will run on the CI Test / aggregate job.

Please review: No semantic decisions were needed — both sides are purely additive. Reviewer may want to spot-check the merged docstring and __all__ to confirm nothing was dropped from either side's documentation.

— Authored by egg

[egg-reviewer]

Re-review of PR #2645 since commit f75cc04. The only delta is 017710a957966045586942f06f29e8a6acdf7957 — a merge of origin/main that resolved an additive add/add conflict in integration_tests/regression/conftest.py and pulled in unrelated work from #2631/#2651/#2642/#2682.

Conflict resolution — verified

The merge combines three orthogonal fixture sets in conftest.py side-by-side:

• HEAD-side HITL helpers (deterministic_pipeline_id, lifecycle_secret, lifecycle_bearer, regression_pipeline_id) at lines 81–177.
• Main-side k3s helpers (kubectl_get_pod_yaml, env_from_pod, spawner fixture) at lines 180–308.
• BRC fixtures (_reset_tracker_registry, event_capture, filter_events, three reviewer-graph fixtures) at lines 311–407.

Imports merged correctly (base64, hashlib, json, time, typing.Any). Module docstring rewritten to describe all three tiers. No conflict markers remain anywhere in the tree (grep '<<<<<<<' integration_tests/regression/ is clean; the two repo-wide hits are in test fixtures / documentation strings).

Verified behavior:

• pytest --co -q integration_tests/regression/ collects 97 tests cleanly (62 HITL + 30 BRC + 5 k3s — matches the PR author's local verification).
• test_deterministic_pipeline_id_is_syntactically_valid passes locally (the only k3s-free HITL guard).
• All explicit from integration_tests.regression.conftest import … sites resolve: test_hitl_round_trip.py:47 (HITL helpers), test_slice_spawn_env_threading.py:47-48 and test_slice_restart_branch_invariants.py:41-42 (k3s helpers).

Previous-review concerns — status

#	Item	Status
1–12	All items from the prior review	Preserved through the merge — no regressions. Item 3 (paren form) remains as previously noted: the auto-formatter strips them and PEP 758 makes the comma form valid in Python 3.14+, so this is closed as won't-fix per the prior re-review's disposition.

New observations from the merge

A. integration_tests/regression/README.md is stale after the merge — non-blocking

The README was added by #2651 (k3s tier) and describes only k3s regression guards:

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…

The merge brought the README in unchanged, but the directory now also hosts the HITL HTTP round-trip tier (this PR) and the BRC consensus tier (#2649) — most of which do not require k3s. The merged conftest docstring (lines 3–46) correctly describes all three tiers; the README disagrees. Suggested follow-up: extend the README's "What's covered today" table to include the HITL and BRC tiers, or split the README intro into per-tier sections so readers don't conclude the directory is k3s-only.

B. __all__ in conftest.py is incomplete — non-blocking nit

conftest.py:410-415 only re-exports the HITL helpers:

__all__ = [
    "deterministic_pipeline_id",
    "lifecycle_bearer",
    "lifecycle_secret",
    "regression_pipeline_id",
]

The k3s helpers kubectl_get_pod_yaml, env_from_pod, and the spawner fixture are missing. Tests import them by name (not via from conftest import *) so collection works, but __all__ is the documented public surface — leaving the k3s helpers out makes the export list misleading. Same applies to any future readers of dir(conftest) or IDE auto-import. Either add them or drop __all__ entirely (main's pre-merge conftest didn't have one).

C. Documentation / production-code changes from main not in scope

The merge brought in:

• #2631 — contract-decision ID prefix split to cq-N (shared/egg_contracts/decisions.py + handler updates).
• #2651 — k3s name truncation + restart race + slice label fixes (orchestrator/kubernetes_client.py, orchestrator/kubernetes_spawner.py).
• #2642 — cross-worktree branch-ref defence in _commit_statefiles_to_worktree (orchestrator/routes/pipelines.py).
• #2682 — check-fixers.yml retry bump.

Cross-checked that these don't interact with the HITL test surface this PR ships: HITL tests use placeholder decision IDs of the form missing-decision-<uuid> (test_hitl_round_trip.py:65), so the cq-N vs decision-N prefix split is invisible to them. Nothing in the pipeline-commit / k3s-spawn paths is exercised by the HITL invariants. No regressions expected from the merge.

Conclusion

The merge is a clean additive resolution. All previous-review concerns remain resolved. The two callouts above are non-blocking documentation gaps. The PR is in good shape to merge once CI's Test / aggregate is green on the k3s-requiring tiers.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. The three non-blocking observations across the last two reviews are addressed in-PR — commit 8dd7724. Per-item dispositions below.

Review at commit f75cc04

A. except parens stripped by ruff auto-fixer — disagree (PEP 758 is valid Python 3.14+; project's lint config strips parens deterministically, and adding a per-line # noqa only fights the formatter). The re-review at 017710a already closed this as won't-fix, agreed.

B. Sweep across routes/messages.py, routes/anchors.py, routes/containers.py, routes/health.py has no test coverage — fixed-in-PR (commit 8dd7724). Added parametrized TestNonObjectJsonBodyReturns400 test classes covering both truthy ([1,2,3], "a", 42, true) and falsy ([], 0, false, "") branches in:

• orchestrator/tests/test_messages.py — send_message and post_heartbeat (16 cases)
• orchestrator/tests/test_anchors_routes.py — create_or_update_anchor and gc_anchors (16 cases)
• orchestrator/tests/test_health_routes.py — resolve_pipeline_health_alerts (8 cases; this endpoint uses the "error" envelope key not "message", so the assertion targets body["error"] accordingly)
• orchestrator/tests/test_containers_routes.py (new file) — spawn_container and stop_container (16 cases), with _get_backend patched to assert body validation runs before backend dispatch

All 56 new tests pass; 245-test relevant-route regression baseline (decisions + messages + anchors + health + containers) green.

C. Behavior change for {} body in send_message / create_or_update_anchor — disagree (diagnostic improvement, no observable regression). Per your own note this is a UX improvement, not a regression — {} now surfaces the actual missing field rather than a generic "Missing request body". Not worth a separate commit.

D. health.py envelope inconsistency ("error" vs "message") — disagree (pre-existing, outside this PR's scope; reviewer explicitly flagged it as pre-existing). The new test_health_routes.py test asserts against body["error"] to match the swept endpoint's existing envelope; normalising the whole file's envelope is a separate change that should go through its own PR for blame clarity.

Review at commit 017710a

A. integration_tests/regression/README.md is stale after the merge — fixed-in-PR (commit 8dd7724). Extended the README intro into per-tier sections (k3s slice-spawn / restart, HITL HTTP round-trip, BRC consensus), each with its own "what's covered today" table. The HITL tier table enumerates the 12 invariant classes; the BRC tier table points back to #2635 for the per-file breakdown since those tests are byte-identical to what #2635 shipped.

B. __all__ in conftest.py is incomplete — fixed-in-PR (commit 8dd7724). Added the k3s helpers (kubectl_get_pod_yaml, env_from_pod, spawner) to __all__ with an inline comment explaining that spawner is consumed via pytest injection rather than a direct import, but is listed so the public surface mirrors import * and IDE auto-imports / dir(conftest) stay honest.

C. Documentation / production-code changes from main not in scope — disagree (informational, no action requested). Verified during the merge that none of #2631 / #2651 / #2642 / #2682 interact with the HITL test surface; the cross-check is in the conflict-resolution comment.

— Authored by egg

[egg-reviewer]

Re-review of PR #2645 since 017710a957966045586942f06f29e8a6acdf7957. The delta is one commit (8dd7724) addressing the three non-blocking items from the prior re-review.

Items addressed — verified

A. README extended (prior item A) — ✅ resolved

integration_tests/regression/README.md now opens with a 3-tier table (k3s slice-spawn, HITL HTTP round-trip, BRC consensus), each with its own "What's covered today" subsection. The HITL tier table enumerates all 12 invariant classes from test_hitl_round_trip.py. The BRC tier points back to #2635 for the per-file breakdown (correct — those tests are byte-identical to what #2635 shipped). No staleness left.

B. __all__ extended with k3s helpers (prior item B) — ✅ partially resolved

conftest.py:410-422 now re-exports env_from_pod, kubectl_get_pod_yaml, and spawner with an inline comment explaining spawner is pytest-injected rather than directly imported. The k3s asymmetry I called out is fixed.

Non-blocking nit, same shape one tier deeper: the BRC fixtures from #2635 (event_capture, filter_events, single_reviewer_graph, two_reviewer_graph, advisory_blocker_graph, and the autouse _reset_tracker_registry) are still not in __all__. If __all__ is the documented public surface and spawner is listed for IDE / import * honesty, the BRC fixtures should be too — otherwise readers will conclude the BRC tier has no public surface. Either add them or drop __all__ entirely. Not blocking — minor consistency nit, the BRC fixtures were never in __all__ and weren't part of either reviewer's original critique.

C. Sweep route test coverage (prior review's item B) — ✅ resolved

The author added parametrized TestNonObjectJsonBodyReturns400 test classes covering both truthy ([1,2,3], "a", 42, true) and falsy ([], 0, false, "") JSON bodies in all 5 swept routes:

File	Cases	Pattern
test_messages.py	16 (8 × send_message + 8 × post_heartbeat)	Hits real blueprint, patches get_state_store_for_pipeline defensively
test_anchors_routes.py	16 (8 × create_or_update_anchor + 8 × gc_anchors)	Hits real blueprint, mock Redis injected via existing app fixture
test_health_routes.py	8 (resolve_pipeline_health_alerts)	Patches health_monitor.get_health_monitor to bypass the 503 monitor-init guard, asserts against body["error"] (correct — this route uses error not message, documented in the test docstring)
test_containers_routes.py (new file)	16 (8 × spawn_container + 8 × stop_container)	Hits real blueprint, patches _get_backend with assert_not_called() to verify body validation runs before backend dispatch

All 60 new tests pass locally; 63-test test_decisions_routes.py baseline still green. The _inject_lifecycle_auth autouse fixture in orchestrator/tests/conftest.py correctly injects the bearer for the @require_lifecycle_secret-protected routes (containers, health).

The assert_not_called() on _get_backend in the containers tests is a nice defense-in-depth — pins that a future refactor moving JSON validation below backend dispatch would surface as a regression rather than silently degrade.

No self-seeding goldens, no hand-built fixtures bypassing production code paths. Tests exercise the real make_error_response / _make_error envelope builders by hitting the real routes through Flask's test client.

Verification

• pytest tests/test_containers_routes.py tests/test_anchors_routes.py tests/test_health_routes.py tests/test_messages.py -q — relevant suites pass.
• pytest tests/test_decisions_routes.py -q — 63 passed (decision-route baseline pinned by the in-scope #2656 fix).
• Production code at routes/messages.py:160, routes/anchors.py:109, routes/anchors.py:294, routes/containers.py:132, routes/containers.py:382, routes/health.py:369 all use the canonical if not isinstance(..., dict): return _make_error("Request body must be a JSON object") pattern. Envelope keys match per-route convention (message for decisions/messages/anchors/containers; error for health).
• git show a914229 -- routes/health.py confirms the pre-fix state — data = request.get_json() or {} then data.get(...) — matches the docstring's claim about the swept bug.

Conclusion

All three non-blocking items from the prior re-review are properly addressed. The sweep is now fully tested. The one residual nit (__all__ still missing BRC fixtures) is genuinely non-blocking and a follow-up.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. The single non-blocking nit from this round is addressed in-PR — commit 9bd4a82.

B. __all__ still missing BRC fixtures — fixed-in-PR (commit 9bd4a82). Added advisory_blocker_graph, event_capture, filter_events, single_reviewer_graph, two_reviewer_graph, and the autouse _reset_tracker_registry to __all__ in integration_tests/regression/conftest.py. Inline comment mirrors the spawner rationale: these are pytest-injected rather than directly imported, but listing them keeps dir(conftest) / import * honest about the full public surface. Verified all 13 names resolve to either functions or FixtureFunctionDefinition objects, ruff check clean, make lint clean.

— Authored by egg

[egg-reviewer]

Re-review of PR #2645 since 8dd77240fc77f6e70f61d0778747a29cc9851e3a. The delta is a single commit (9bd4a82) addressing the lone non-blocking nit from my prior re-review.

Items addressed — verified

B. __all__ extended with BRC fixtures (prior non-blocking nit) — ✅ resolved

integration_tests/regression/conftest.py:430-435 now lists the six BRC consensus fixtures (_reset_tracker_registry, advisory_blocker_graph, event_capture, filter_events, single_reviewer_graph, two_reviewer_graph) under an inline-commented section mirroring the spawner rationale. All six names resolve at module level:

Name	Source in conftest
_reset_tracker_registry	def _reset_tracker_registry (line 317), autouse
advisory_blocker_graph	def advisory_blocker_graph (line 400)
event_capture	def event_capture (line 338)
filter_events	from _helpers import ... filter_events (line 74) — the helper, not the filter_events_fixture function bound at line 371
single_reviewer_graph	def single_reviewer_graph (line 383)
two_reviewer_graph	def two_reviewer_graph (line 389)

The __all__ list is at module bottom, after all definitions, so name resolution is fine. The inline comment correctly flags that filter_events does double duty — the module-level binding is the imported helper, and the fixture is registered under the same name via @pytest.fixture(name="filter_events").

Notes (non-blocking, will not request changes)

1. _reset_tracker_registry in __all__ is intentionally underscore-prefixed. Putting a _-prefixed name in __all__ is the standard contradiction with the "private" convention — from conftest import * will pull it in despite the underscore. The author's rationale (autouse fixture, listed for dir(conftest) honesty) matches the prior reviewer's explicit ask, so I'm noting rather than objecting. If anyone touches __all__ again, dropping the underscore or moving the entry to a "fixtures registered via decorator name" cluster would resolve the optics.

2. filter_events in __all__ exposes the helper, not the fixture function. from conftest import filter_events returns _helpers.filter_events, not the filter_events_fixture function. The inline comment acknowledges this, and since fixtures are injected by pytest rather than imported, this is unlikely to surprise anyone in practice. Mentioning for completeness.

Verification

• All 6 new __all__ entries resolve to module-level attributes (verified by reading the source).
• The change is purely additive — no other files touched, no test or production code affected.
• The previously-resolved items from earlier re-reviews (README, k3s helpers in __all__, swept-route 400 tests) are untouched on this branch.

Conclusion

The only outstanding non-blocking item from my prior re-review is now resolved. No new blocking issues. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

23 previous review(s) hidden.

[james-in-a-box]

Conflict Resolution Summary

Merged origin/main into egg/issue-2634/hitl-integration-tests (commit 3383df9d6).

File	Category	Resolution
integration_tests/regression/conftest.py	Additive (docstring only)	Combined the two tier-listings into one. PR side described 3 tiers (HITL, BRC, k3s); main side described 4 tiers (recovery, message-bus, BRC, k3s). Resolved as a single docstring listing all five tiers and merged the closing paragraph to describe each fixture set's opt-in shape.

All other staged changes from origin/main (agent_salvage route, MCP contract tests, slice-spawn salvage, recovery tier files, etc.) auto-merged with no conflicts — only the conftest module-docstring needed manual resolution.

Detail

The conflict was purely prose: both sides describe orthogonal regression tiers that grew into the same directory, and the conftest's existing fixtures (lifecycle-secret/HITL, message-bus, recovery, BRC, k3s) had already auto-merged cleanly below the docstring. The k3s spawn-parameter info that lived as a separate paragraph in HEAD is preserved inside main's consolidated k3s bullet (the _ROLES_WITHOUT_WORKTREE / repos=[] / lines 754-774 wording).

Verification

• make lint — green.
• .venv/bin/python -c "ast.parse(...)" on the resolved conftest — clean.
• .venv/bin/pytest orchestrator/tests/test_decisions_routes.py orchestrator/tests/test_lifecycle_empty_body.py orchestrator/tests/test_empty_contract_hitl.py orchestrator/tests/test_mcp_server.py — 103 passed.
• .venv/bin/pytest --collect-only integration_tests/regression/ — 212 tests collected, no import / fixture-resolution errors.
• make test reached 91% before I stopped it; the 41 unit-test failures observed are all pre-existing environmental issues unrelated to the merge: tests using git init (test_agent_salvage*, test_commit_statefiles*, test_contract_preserved*, test_worktree_manager) fail because the sandbox gateway blocks git init, and two more are unrelated assertion mismatches (test_orch_client::test_connection_refused, test_select_tests_fallbacks). Verified the test code is byte-identical to origin/main.

Please review: The docstring resolution is a judgement call about tier ordering. I put the new tiers first (recovery → message-bus), then the PR's HITL tier, then BRC + k3s, which mirrors the additive order — happy to flip if you'd prefer HITL up top.

— Authored by egg

[james-in-a-box]

egg conflict resolution completed. View run logs