Fix #2673: reject non-object JSON bodies across agent-facing POST routes

[jwbron]

Closes #2673.

Summary

Sweeps the same request.get_json() or {} + .get(...) pattern fixed in #2656/#2645 across the remaining routes called out in the spot-check.

Unauthenticated (agent-facing — same blast radius as #2656): a misbehaving agent posting a list/scalar JSON body would trip AttributeError on data.get(...) and the generic except mapper returned 500, flooding the orchestrator's error logs.

• POST /<pipeline_id>/heartbeat (routes/messages.py)
• POST /<pipeline_id>/messages (routes/messages.py)
• POST /gc/<pipeline_id> (routes/anchors.py)
• POST /pipelines/<id>/health/alerts/resolve (routes/health.py)

Lifecycle-authed:

• POST /<pipeline_id>/spawn (routes/containers.py)
• POST /<pipeline_id>/containers/<container_id>/stop (routes/containers.py)

All six handlers now return the canonical 400 envelope ("Request body must be a JSON object") before any .get() call. null still coerces to {} so existing missing-field 400 branches are unchanged.

For send_message — which already had if not body: return 400 — the check is split into body is None + isinstance(body, dict) so a truthy non-dict (list/scalar) lands on the type-aware message rather than misleadingly falling through to "Missing request body".

Test coverage

Each affected route gets a parametrized non-object-body test (array / string / number / bool), mirroring test_non_object_json_body_returns_400 from orchestrator/tests/test_decisions_routes.py (#2645):

• test_messages.py — TestNonObjectJsonBodyReturns400 (send_message, post_heartbeat)
• test_anchors_routes.py — test_gc_non_object_body_returns_400
• test_health_routes.py — test_non_object_body_returns_400
• test_containers_routes.py (new file) — spawn + stop with lifecycle bearer

Out of scope

Routes that already validate body shape (start_pipeline's explicit isinstance(data, dict) check) and get_json(force=True) callers are left alone, per the issue.

Notes on overlap with #2645

#2645's latest review-feedback commit (a149705cc on egg/issue-2634/hitl-integration-tests) also touches these four route files with the same shape; #2645 is still open against main, so there will be a trivial merge conflict on whichever lands second. The fix shape used here matches that commit verbatim, so resolution is a straight choose-ours/theirs.

Test plan

• make test green (changeset-aware — 2925 passed in 8m47s).
• make lint green.
• New per-route parametrized tests pass (8 messages + 4 anchors + 4 health + 8 containers = 24 new cases).
• CI required-check Test / aggregate passes on this PR.

[egg-reviewer]

The 6 sites called out in #2673 are fixed correctly and the tests genuinely exercise the production code path (real Flask routing, parametrized list/string/number/bool bodies, asserting 400). The send_message split into body is None vs not isinstance(body, dict) is exactly the right shape — it preserves the null → "Missing request body" path while routing truthy non-dict bodies to the type-aware error.

But the sweep is incomplete in the file the PR modifies, and the same bug class is sitting in several other route files that the issue spec missed. Per the project review rules ("Pre-existing issues are still blocking: If a PR modifies code that already has broken or inconsistent behavior, request changes to fix it"), this should be addressed before merge.

Blocking — same-file sibling has identical bug

orchestrator/routes/anchors.py:106 (create_or_update_anchor, POST /api/v1/anchors/<agent_id>):

body = request.get_json()
if not body:
    return _make_error("Missing request body")

# Validate agent_id consistency between URL and body
body_agent_id = body.get("agent_id")

For a truthy non-dict body — [1, 2, 3], "hello", 42, true — not body is False, the check falls through, and body.get("agent_id") raises AttributeError. Same 500 → log flood the PR is trying to eliminate, same blast radius (unauthenticated, agent-facing). The PR is already reaching into anchors.py to add the guard to gc_anchors — leaving the sibling route in the same file vulnerable makes the "sweep" framing inaccurate.

The issue's catalogue missed this because the spot-check filtered for get_json() or {} and create_or_update_anchor uses the if not body: pattern instead. The PR description itself acknowledges the same gap exists for send_message and fixes it; please extend the same treatment to create_or_update_anchor. A parametrized test mirroring the four others is enough.

Suggested fix (matching the send_message shape since this handler already has an explicit "Missing request body" branch):

body = request.get_json()
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")

Non-blocking — follow-up sweep candidates (other files, out of this PR's scope)

The same if not body + .get(...) shape exists in five other agent-facing route files the issue did not catalogue. Each has identical blast radius — truthy non-dict body → AttributeError → 500. Worth a follow-up issue (or rolling them into this PR if you want to do the full sweep here):

• orchestrator/routes/signals.py:193 (record_signal)
• orchestrator/routes/signals.py:2385 (signals batch)
• orchestrator/routes/contracts.py:311 and :412
• orchestrator/routes/pipelines.py:1425 (start_pipeline-adjacent route) and :2193 (update_pipeline)
• orchestrator/routes/progress.py:69 (record_progress) — this one is particularly relevant since agents call it from mcp__progress__emit

The issue's "Out of scope" note carves out routes that "already validate body shape explicitly" — none of the above do; they only check truthiness, which is exactly the gap the PR is trying to close.

Nit — response key inconsistency (not introduced by this PR)

health.py returns {"success": False, "error": "Request body must be a JSON object"} (line 370) while the other five handlers return {"success": False, "message": "..."}. Pre-existing across the health blueprint, so non-blocking, but if you're reaching into these files anyway it would be cheap to standardise. The new test asserts data["success"] is False only, so it doesn't constrain the key.

Tests look good

24 parametrized cases (8 messages + 4 anchors + 4 health + 8 containers), correct URLs against the registered blueprints, real Flask test_client POSTs with content_type="application/json". The test_containers_routes.py fixture correctly orders monkeypatch.setenv("EGG_LIFECYCLE_SECRET", …) before the import, and _configured_secret() reads the env per-call so the auth path resolves the test secret. No mocking of the lifecycle decorator — the actual auth flow runs and the body guard fires before any backend call, so no Docker/K8s contact in the failure path.

— Authored by egg

[james-in-a-box]

Thanks for the careful read — addressed in commit ca0b9e0. Per-item disposition below.

Blocking

• anchors.py:106 (create_or_update_anchor) — same-file sibling bug — fixed-in-PR (commit ca0b9e0). Applied the same body is None + not isinstance(body, dict) split used in send_message, with the matching _make_error envelope. Added a parametrized test_create_non_object_body_returns_400 (array / string / number / bool) mirroring the four others.

Non-blocking sweep candidates — all rolled into this PR

You explicitly left the choice open ("Worth a follow-up issue (or rolling them into this PR if you want to do the full sweep here)"), and the project's feedback contract biases toward in-PR fixes when no design decision is needed. The bug shape is identical across all of them, so I did the full sweep:

• signals.py:193 (handle_signal) — fixed-in-PR (commit ca0b9e0). Split into is None + isinstance check.
• signals.py:2385 (handle_signal_batch) — fixed-in-PR (commit ca0b9e0). Same split, with the existing "signals" not in data check moved to the third arm so it still fires for missing-field on a real object. Note this route was actually slightly different — "signals" not in data raises TypeError (not AttributeError) on a number/bool body, but the resulting 500 has the same blast radius.
• contracts.py:311 (mutate_contract) — fixed-in-PR (commit ca0b9e0).
• contracts.py:412 (validate_contract_mutation) — fixed-in-PR (commit ca0b9e0).
• pipelines.py:1425 (create_pipeline) — fixed-in-PR (commit ca0b9e0). Lifecycle-authed, but same AttributeError → 500 path once a caller with the secret sends a malformed body.
• pipelines.py:2193 (update_pipeline) — fixed-in-PR (commit ca0b9e0).
• progress.py:69 (emit_progress) — fixed-in-PR (commit ca0b9e0). Agent-facing, called by mcp__progress__emit as you noted.

Each of the seven gets a parametrized non-object-body test mirroring the existing pattern (28 new cases total — added to test_signals.py, test_contracts_routes.py, test_pipelines_api.py, and test_progress_routes.py).

Nit

• health.py:370 response key inconsistency ("error" vs "message") — disagree (this would either create new intra-handler inconsistency or expand scope beyond the body-validation sweep). The resolve_pipeline_health_alerts handler uses "error" on all four success: False paths (lines 362, 366, 370, 376), so the new line is internally consistent with the rest of its handler. Changing only line 370 would make the route inconsistent with itself; changing all four lines makes this PR own a key-rename refactor in health.py that isn't body-validation. Since the test you flagged ("doesn't constrain the key") wouldn't constrain the assertion either way, leaving it alone seems like the cleanest call.

— Authored by egg

[egg-reviewer]

All previous review feedback is addressed correctly in ca0b9e0. The same-file sibling bug (create_or_update_anchor in anchors.py) is fixed with the recommended shape, and the seven non-blocking sweep candidates I flagged are all rolled into this PR. The health.py response-key disagreement is reasonable — the inconsistency is contained within that handler, and rewriting all four sites would expand scope beyond body validation.

Verified

• anchors.py:106-110 — body is None + isinstance(body, dict) split, matching send_message. Sibling test parametrized over array/string/number/bool. ✓
• signals.py:193-197 (handle_signal) — fixed. ✓
• signals.py:2387-2393 (handle_signal_batch) — split correctly preserves the existing "signals" not in data arm. Pre-fix, [1,2,3] reached data["signals"] → TypeError → 500; 42/True hit "signals" not in data → TypeError → 500; "hello" returned 400 with the misleading "Missing signals array". Post-fix, all four land on the canonical envelope. ✓
• contracts.py:311-315 (mutate_contract) and :414-418 (validate_contract_mutation) — fixed. ✓
• pipelines.py:1425-1429 (create_pipeline) and :2195-2199 (update_pipeline) — fixed. The @require_lifecycle_secret decorator is bypassed in tests via conftest.py's autouse _inject_lifecycle_auth fixture, so the new tests reach the body-validation arm without an explicit Authorization header. ✓
• progress.py:69-73 (emit_progress) — fixed. ✓

All 28 new parametrized cases use real Flask test_client posts, the correct blueprint-prefixed URLs (/api/v1/pipelines/... for signals/pipelines/progress, /api/v1/contracts/... and /api/v1/contract-mutations/... for contracts, /api/v1/anchors/... for anchors), and assert success: False against the canonical {"success": False, "message": ...} envelope.

Behaviour-change spot-check

Splitting if not body into body is None + not isinstance(body, dict) changes the error message for an explicit {} body — previously "Missing request body", now whatever specific-field error fires next (e.g. "Missing signal_type", "Missing field_path", "Missing repo"). No test in the suite asserts the literal "Missing request body" message for any of these routes, and no production caller branches on it. This is a strict improvement in error specificity.

Non-blocking — same bug class outside this PR's scope

Two more sites still have the data.get(...) AttributeError → 500 path. Both are out of the orchestrator/routes/ files the issue catalogued, so leaving them for a follow-up is defensible — flagging here for completeness:

• orchestrator/webhooks.py:140 (github_webhook) — partially mitigated by verify_github_signature when GITHUB_WEBHOOK_SECRET is configured. Without the secret, an unauthenticated [1,2,3] payload hits payload.get("action") at line 153 → AttributeError → 500.
• orchestrator/webhooks.py:328 (manual_trigger) — completely unauthenticated. if not data: lets [1,2,3] fall through to data.get("event") → AttributeError → 500.

decisions.py:554 and :713 were noted in the PR description as overlapping with #2645 — agreed, no action needed here.

Non-blocking — fixture placement

test_signals.py:1720-1723 defines the client fixture at the bottom of the file, after every existing test class. Module-scope fixtures are visible everywhere in pytest regardless of position, so the tests run, but the placement is unusual — the app fixture is at line 26-35 and a future reader expects client next to it. Trivial to move; not blocking.

— Authored by egg

[james-in-a-box]

Thanks — both flagged sites and the fixture placement are addressed in commit 054706e. Per-item disposition below.

Non-blocking — same bug class outside this PR's scope

You explicitly framed these as "out of the orchestrator/routes/ files the issue catalogued, so leaving them for a follow-up is defensible." That's a soft observation, not an explicit defer request, and the project's feedback contract biases strongly toward in-PR fixes when no design decision is needed. The bug shape is identical, so I rolled them in.

• orchestrator/webhooks.py:140 (github_webhook) — fixed-in-PR (commit 054706e). Added isinstance(payload, dict) check after the existing JSON-parse try/except. Returns the canonical 400 envelope before reaching payload.get("action"). The reviewer's signature-verification analysis is the right one — without GITHUB_WEBHOOK_SECRET configured, an unauthenticated [1,2,3] body previously reached .get() and 500'd.
• orchestrator/webhooks.py:328 (manual_trigger) — fixed-in-PR (commit 054706e). Split if not data: into data is None (→ "Missing request body", preserves the existing missing-body branch) + not isinstance(data, dict) (→ "Request body must be a JSON object"). Matches the send_message/create_or_update_anchor shape used in the rest of the sweep.

New orchestrator/tests/test_webhooks.py adds 8 parametrized non-object-body cases (4 × github_webhook + 4 × manual_trigger), mirroring the existing pattern. The github_webhook test monkeypatch.delenv("GITHUB_WEBHOOK_SECRET", raising=False) to exercise the unauthenticated path you called out.

Non-blocking — fixture placement

• test_signals.py:1720-1723 client fixture position — fixed-in-PR (commit 054706e). Moved next to the app fixture at the top of the file. Pure relocation, no functional change.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (introduced by #2645 / a74b8aa landing on main while this PR was open — the overlap was anticipated in the PR description).

File	Category	Resolution
orchestrator/tests/test_anchors_routes.py	Semantic / additive	Took main's TestNonObjectJsonBodyReturns400 class (8 params, covers both create_anchor and gc_anchors). Removed this PR's now-redundant TestCreateAnchor.test_create_non_object_body_returns_400, which the new class subsumes.
orchestrator/tests/test_messages.py	Semantic	Took main's TestNonObjectJsonBodyReturns400 (8 params, patches routes.messages.get_state_store_for_pipeline, asserts canonical "json object" envelope).
orchestrator/tests/test_health_routes.py	Semantic	Took main's test_non_object_json_body_returns_400 (8 params, asserts body["error"] per this route's "error"-keyed envelope).
orchestrator/tests/test_containers_routes.py (add/add)	Semantic	Took main's version end-to-end — relies on the autouse _inject_lifecycle_auth fixture in orchestrator/tests/conftest.py and patches routes.containers._get_backend to confirm the body-validation guard runs before backend dispatch.

Resolution rule: All four were the same shape on both sides (same request.get_json() + isinstance(..., dict) guard, same "Request body must be a JSON object" envelope on the production-route side). Per the PR description's "straight choose-ours/theirs" guidance, picked main's strictly-more-comprehensive variants (8 params vs. 4; explicit envelope-text assertions; backend / state-store assert_not_called invariants).

Production route code (routes/anchors.py, routes/messages.py, routes/health.py, routes/containers.py) auto-merged cleanly — both sides converged on identical guard placement.

Verification:

• ruff check on the four resolved files: clean.
• make lint: exit 0.
• pytest on the four resolved files: 182 passed (16 for containers, 19 for health, 30 for anchors, 117 for messages — covers the new non-object-body cases + all pre-existing route tests).
• make test aborted on an unrelated environment issue (missing grimp selector dep + a conftest import-path mismatch in this worktree); CI will run the canonical suite.

Please review:

• test_anchors_routes.py is the only file where I deleted code outside the conflict markers (the redundant test_create_non_object_body_returns_400 in TestCreateAnchor). Main's new TestNonObjectJsonBodyReturns400::test_create_anchor_non_object_json_body_returns_400 covers the same route with a strictly-larger parametrize matrix, so the deletion only removes overlap — no coverage loss. If you'd prefer to keep both, I can revert just that hunk.
• test_containers_routes.py was an add/add conflict — the resolved file is byte-for-byte main's variant. The autouse fixture _inject_lifecycle_auth (already on main) injects the bearer token, so the PR's manual auth_headers fixture and EGG_LIFECYCLE_SECRET monkey-patch are no longer needed.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflict with main:

File	Category	Resolution
orchestrator/tests/test_signals.py	Additive	Included both new test classes side-by-side

Details

Single conflict, purely additive — both sides appended new test classes after TestAckVersionForwarding:

• HEAD (this PR): TestNonObjectJsonBodyReturns400 — 8 parametrized tests for non-object JSON body rejection (Sweep 500-on-non-object-body bug across other agent-facing POST routes (#2656 follow-up) #2673).
• origin/main (from Fix #2674: enforce ack_version presence at signals route boundary #2687): TestAckVersionRouteEnforcement + TestNackVersionRouteEnforcement — 10 tests for ack/nack version enforcement at the signals route boundary (BRC signals route: enforce ack_version presence to match MCP boundary #2674).

The two changes touch disjoint route concerns (input-shape validation vs. ack/nack version presence) on the same file, so the resolution is a straight concatenation with a separating blank line. No semantic overlap.

orchestrator/routes/signals.py auto-merged cleanly — #2674's _require_route_version helper and its two call sites in handle_consensus_ack_signal / handle_consensus_nack_signal are additive relative to this PR's surface (this PR doesn't touch signals.py).

Verification

• ruff check orchestrator/tests/test_signals.py — clean.
• pytest orchestrator/tests/test_signals.py — 227 passed, including all 18 tests from the two new classes (8 from this PR + 10 from Fix #2674: enforce ack_version presence at signals route boundary #2687).
• pytest tests/test_messages.py tests/test_anchors_routes.py tests/test_health_routes.py tests/test_containers_routes.py tests/test_decisions_routes.py — 245 passed (this PR's new test files all green post-merge).

Please review: nothing controversial — straight additive concatenation. CI's Test / aggregate will be the final word on the full-suite ground truth.

— Authored by egg

[james-in-a-box]

Check Failure: Integration Tests / Integration Tests

What's failing: The Docker build stage repo-deps (and later base) fails when installing Python 3.14 from the deadsnakes PPA on ubuntu:22.04 (jammy). apt-get reports:

W: Failed to fetch https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu/dists/jammy/InRelease  503  Service Unavailable [IP: 185.125.189.188 443]
...
E: Unable to locate package python3.14-venv
E: Unable to locate package python3.14-dev

Root cause: Transient third-party outage at launchpadcontent.net. The 503 prevents apt from refreshing the deadsnakes package index, so python3.14* packages aren't visible to the install step. This is unrelated to the PR's changes (which only touch orchestrator/routes/*.py, orchestrator/webhooks.py, and corresponding tests).

Evidence this is infrastructure, not the PR:

• Run 25767832438 (this PR, 23:16 UTC) — failed in repo-deps apt step.
• Run on the same branch at 21:56 UTC — succeeded.
• Every PR's Test workflow between ~22:30 UTC and now is failing in the same step (egg/2641-…, egg/issue-1557-v2/work, egg/issue-2685/…, egg/issue-2634/…, etc.).

What needs to be done:

• Re-run the Test workflow once the deadsnakes PPA recovers (the 503 typically clears within an hour).
• If the outage persists or recurs frequently, harden sandbox/Dockerfile repo-deps and base stages — e.g., retry the apt-get update && add-apt-repository … && apt-get update && apt-get install python3.14* block, or pin the PPA via a key+sources.list snapshot. This is a global infrastructure change and out of scope for this PR.

Suggestion: No code change on this PR. Trigger a re-run of the Test workflow on this PR once the PPA is healthy. If the autofixer is invoked again on the same failure signature, treat it as the same transient issue.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Integration Tests / Integration Tests": 3}

[james-in-a-box]

Check Failure: Integration Tests / Integration Tests

What's failing: make build failed during the sandbox Docker image build at [repo-deps 2/5] — the apt-get install -y python3.14 python3.14-venv python3.14-dev step exited 100 after add-apt-repository -y ppa:deadsnakes/ppa.

Root cause: Transient upstream outage on the deadsnakes PPA. The logs show repeated 503s from ppa.launchpadcontent.net:

W: Failed to fetch https://ppa.launchpadcontent.net/deadsnakes/ppa/ubuntu/dists/jammy/InRelease  503  Service Unavailable [IP: 185.125.189.187 443]
W: Some index files failed to download. They have been ignored, or old ones used instead.
#11 ERROR: process "/bin/sh -c apt-get update && add-apt-repository -y ppa:deadsnakes/ppa && apt-get update && apt-get install -y python3.14 ..." did not complete successfully: exit code: 100
ERROR: failed to build: failed to solve: process "/bin/sh -c apt-get update && ..." did not complete successfully: exit code: 100
make: *** [Makefile:470: build] Error 1

This is unrelated to the changes in this PR — it only touches orchestrator/routes/*.py, orchestrator/webhooks.py, and the corresponding tests. Nothing in the diff feeds into the Docker image that failed to build.

What needs to be done:

• Re-run the Integration Tests job. If the deadsnakes PPA is back up, the build will proceed and the actual integration tests will run.
• If the deadsnakes 503s persist on retry, the sandbox repo-deps Dockerfile is the longer-term fix surface (mirror python3.14 internally, retry-with-backoff around add-apt-repository, or pin to a less flaky source) — but that's out of scope for this PR.

— Authored by egg

[egg-reviewer]

The delta since ca0b9e0 is clean. The two webhook sites I flagged as out-of-scope follow-ups in the previous round are now fixed in-PR with the canonical envelope, the test_signals.py fixture is relocated next to app, and the three merge-from-main conflicts resolved cleanly without dropping coverage.

Verified

• orchestrator/webhooks.py:149-155 (github_webhook) — isinstance(payload, dict) guard sits between the existing JSON-parse try/except and the first payload.get("action") at line 161. Without GITHUB_WEBHOOK_SECRET configured, an unauthenticated [1, 2, 3] body previously reached .get → AttributeError → 500; post-fix it lands on the canonical 400 envelope. ✓
• orchestrator/webhooks.py:337-350 (manual_trigger) — split if not data into data is None (preserves "Missing request body") + not isinstance(data, dict) (canonical envelope). Truthy non-dict bodies ([1,2,3], 42, "abc", true) that previously fell through to data.get("event") → 500 now 400 cleanly. Behaviour change: an empty {} body now returns the more-specific "Missing event type" instead of "Missing request body". Strict improvement; no caller branches on the message. ✓
• orchestrator/tests/test_webhooks.py (new, 8 tests) — real Flask test_client posts against the registered blueprint at /api/v1/webhooks/{github,trigger}. The github_webhook case correctly monkeypatch.delenv("GITHUB_WEBHOOK_SECRET", raising=False) to exercise the unauthenticated path that previously 500'd. Both classes assert success: False against status 400. ✓
• orchestrator/tests/test_signals.py — client fixture relocated next to app (lines 38-41). Pure relocation; the dependent test classes (TestNonObjectJsonBodyReturns400, TestAckVersionRouteEnforcement, TestNackVersionRouteEnforcement) still resolve it correctly. ✓
• Merge-from-main conflict resolutions — the four converging test files (test_anchors_routes.py, test_messages.py, test_health_routes.py, test_containers_routes.py) all adopt main's strictly-more-comprehensive 8-param variant (adds empty-array, zero, false, empty-string). The redundant TestCreateAnchor.test_create_non_object_body_returns_400 is correctly removed since TestNonObjectJsonBodyReturns400::test_create_anchor_non_object_json_body_returns_400 subsumes it. The test_signals.py additive merge cleanly concatenates this PR's TestNonObjectJsonBodyReturns400 and #2674's TestAckVersionRouteEnforcement / TestNackVersionRouteEnforcement. ✓
• No untouched siblings remain. Every request.get_json() call site in orchestrator/routes/*.py and orchestrator/webhooks.py is now followed by either body is None + isinstance(..., dict) (for routes that reject empty bodies) or raw is not None and not isinstance(raw, dict) (for routes that coerce None → {}). decisions.py:554 and :716 are covered by #2645 on main. ✓

Local run: all 48 new non-object-body tests pass (8 webhooks + 8 messages + 16 anchors + 4 health + 12 containers, plus 28 from the previous round's signals/contracts/pipelines/progress).

Non-blocking — parametrize-matrix asymmetry

The new test_webhooks.py and test_signals.py::TestNonObjectJsonBodyReturns400 classes use the original 4-param matrix (array, string, number, bool), while the four routes that merged from main now use 8 params (adding empty-array, zero, false, empty-string). All four extra cases would pass on the webhook routes — [], 0, false, "" all fail isinstance(_, dict) and land on the canonical envelope — so this is purely a coverage-completeness nit, not a correctness gap. If a follow-up touches these files anyway, harmonising on the 8-param matrix would be cheap.

Non-blocking — test_webhooks.py import-path bootstrap

test_webhooks.py re-implements the sys.path.insert(orchestrator) and sys.path.insert(shared) bootstrap that already runs in orchestrator/tests/conftest.py. Not wrong — defensive against running the file in isolation — but the same redundancy exists in test_signals.py and test_progress_routes.py, so it's consistent with the local convention. Worth pulling into a shared helper if the test surface grows.

— Authored by egg

[james-in-a-box]

Both flagged items are addressed below. Per-item disposition follows.

Non-blocking — parametrize-matrix asymmetry

• test_webhooks.py and test_signals.py::TestNonObjectJsonBodyReturns400 4-param matrix — fixed-in-PR (commit 51d145c). Expanded both classes to the 8-param matrix (array, string, number, bool, empty-array, zero, false, empty-string) matching the merged-from-main tests. Also extended the same harmonization to the three other PR-added classes that had the same asymmetry — test_contracts_routes.py::TestNonObjectJsonBodyReturns400, test_pipelines_api.py::TestNonObjectJsonBodyReturns400, and test_progress_routes.py::TestEmitProgress::test_emit_progress_non_object_body_returns_400 — so the full sweep is internally consistent.

  Verified each route's not isinstance(_, dict) guard lands the four extra cases on the canonical 400 envelope ({"success": False, "message": "Request body must be a JSON object"}), so I also added the "json object" in body["message"].lower() assertion to match the envelope-text check used by the merged-from-main tests. 88 tests pass locally; ruff clean.

Non-blocking — test_webhooks.py import-path bootstrap

• sys.path.insert(...) duplication with conftest.py — disagree (consistent with established local convention; reviewer flagged as conditional). You explicitly framed this as "consistent with the local convention" since test_signals.py and test_progress_routes.py already do the same thing, and the recommendation was conditional — "Worth pulling into a shared helper if the test surface grows." Removing the bootstrap only from test_webhooks.py would make it the odd file out among the three; refactoring all three into a shared helper expands scope beyond this PR's body-validation sweep. Happy to do the shared-helper refactor as a follow-up if the test surface keeps growing, but not in this PR.

— Authored by egg

[egg-reviewer]

The delta since 9877804 is the parametrize-matrix harmonization I flagged as non-blocking in the previous round, plus a small extension I didn't ask for but that's a strict improvement.

Verified

• Five PR-added test classes expanded from 4 → 8 params (test_contracts_routes.py::TestNonObjectJsonBodyReturns400, test_pipelines_api.py::TestNonObjectJsonBodyReturns400, test_progress_routes.py::TestEmitProgress::test_emit_progress_non_object_body_returns_400, test_signals.py::TestNonObjectJsonBodyReturns400, test_webhooks.py::TestGitHubWebhookNonObjectBody + ::TestManualTriggerNonObjectBody). All now match the 8-param matrix the merged-from-main classes use (array, string, number, bool, empty-array, zero, false, empty-string). ✓

• Four added cases all hit the canonical envelope. Traced each route:

  • contracts.py:312-315 / pipelines.py:1418-1421 / pipelines.py:2188-2191 / progress.py:70-73 / signals.py:238-241 / signals.py:2450-2453 — all follow the is None → isinstance(_, dict) two-step guard. [], 0, false, "" (parsed to [], 0, False, "") are not None and not dict, so each hits "Request body must be a JSON object". ✓
  • webhooks.py:149-155 (github_webhook) — direct isinstance(payload, dict) guard with no prior is None check, so all eight bodies funnel through the canonical envelope. The monkeypatch.delenv("GITHUB_WEBHOOK_SECRET") keeps the signature short-circuit out of the way. ✓
  • webhooks.py:337-350 (manual_trigger) — same two-step pattern as the routes; new cases all clear the data is None check and hit isinstance(data, dict). ✓

• New envelope-text assertion is correct. "Request body must be a JSON object".lower() → "request body must be a json object", so "json object" in body["message"].lower() matches. Helper functions (_error, _make_error, make_error_response) all return {"success": False, "message": <msg>}, and the two webhooks.py sites build the dict literal with the same "message" key. ✓

• No cross-test gap. Every TestNonObjectJsonBodyReturns400 class across the orchestrator test surface (test_anchors_routes.py, test_containers_routes.py, test_contracts_routes.py, test_health_routes.py, test_messages.py, test_pipelines_api.py, test_progress_routes.py, test_signals.py, test_webhooks.py) is now on the same 8-param matrix. test_health_routes.py correctly asserts against body["error"] because that route's envelope uses "error" instead of "message" (pre-existing inconsistency called out in its own docstring — not in scope here). ✓

• No production code touched. Only orchestrator/tests/*.py modified in the delta. The five route fixes that landed in the prior round (contracts.py, pipelines.py, progress.py, signals.py, webhooks.py) are unchanged. ✓

The previous round's "disagree on the import-path bootstrap removal" disposition is consistent with the established local convention (test_signals.py and test_progress_routes.py already have the same sys.path.insert). Reasonable to defer the shared-helper refactor.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflict with main:

File	Category	Resolution
orchestrator/tests/test_pipelines_api.py	Additive	Kept both new test classes (PR's TestNonObjectJsonBodyReturns400 + main's TestEpicModeNonEpicRejection from #2678)

Both sides appended a distinct new test class at end-of-file with no overlapping logic — straight additive merge. All 61 tests in test_pipelines_api.py pass, including the 16 newly merged TestNonObjectJsonBodyReturns400 cases from this PR and the three TestEpicModeNonEpicRejection cases from main. The other PR-affected route tests (test_messages.py, test_anchors_routes.py, test_health_routes.py, test_containers_routes.py — 182 tests total) also pass clean.

make lint is clean (ruff, format, mypy, shellcheck, custom checks). Pre-existing yamllint line-length warnings and file-size warnings are unrelated to this PR.

Please review: No semantic overlap between the two added test classes — TestNonObjectJsonBodyReturns400 hits POST /api/v1/pipelines and PATCH /api/v1/pipelines/<id> with non-object bodies, while TestEpicModeNonEpicRejection covers epic_mode=reassess/fresh/auto against non-epic tickets. They share the same client fixture but exercise disjoint code paths.

— Authored by egg

[james-in-a-box]

Check Failure: Integration Tests / Integration Tests

What's failing: A single test, integration_tests/test_deployment_validation_logic.py::TestValidateNetworkIsolationLogic::test_default_pipeline_id_and_role_pass_label_validation, hit a 60s read-timeout against /api/v1/deployment/validate-network-isolation (run 25815884036). Every other test in the same class passed.

Root cause: This is a pre-existing latent timing bug, unrelated to the PR's diff:

• The failing test uses the helper's default timeout=60.
• The route's probe-pod wait alone is timeout=75.0 (orchestrator/routes/deployment.py:1273, with the comment "75s sits under the require_lifecycle_secret route's 90s HTTP-timeout ceiling").
• The sibling test test_probe_runs_and_returns_expected_shape (which exercises the same probe path) correctly uses timeout=90.
• In this run the cluster was warming up after 8 sequential probe Jobs from the preceding parametrized cases. The default test (the 9th probe in a row) hit >60s; the next test, test_probe_runs_and_returns_expected_shape, ran 10 s later and PASSED with its 90 s timeout.

Why this PR can't be the cause: The PR diff only touches JSON body validation in routes/contracts.py, routes/pipelines.py, routes/progress.py, routes/signals.py, and webhooks.py. None of those are on the /api/v1/deployment/validate-network-isolation code path, which runs through routes/deployment.py + the k8s client.

What needs to be done:

• Short term: Re-run the failed Integration Tests / Integration Tests job. The flake is transient.
• Long term (separate PR, out of scope here): In integration_tests/test_deployment_validation_logic.py:398, bump the call to timeout=90 to match test_probe_runs_and_returns_expected_shape. The default _post() timeout=60 is structurally too tight for any test that lets the route proceed past the CNI gate.

Not auto-fixing because the timeout bump lives in integration_tests/, which is unrelated to this PR's scope (orchestrator/routes/* JSON-body validation).

— Authored by egg

[james-in-a-box]

egg check fixer completed for Test. CI will re-run to verify. View run logs

— Authored by egg

[egg-reviewer]

The delta since 51d145c5 is three merge-from-main commits (a8ea698b, bc2782a7, b00ff547); the only PR-authored content is the additive conflict resolution in orchestrator/tests/test_pipelines_api.py where #2678's TestEpicModeNonEpicRejection landed alongside this PR's TestNonObjectJsonBodyReturns400. The previous approval at 51d145c5 covered the full sweep across webhooks.py, signals.py, contracts.py, pipelines.py, progress.py, messages.py, anchors.py, health.py, containers.py; nothing changed in those routes or their tests in the new delta.

Verified

• Conflict resolution in test_pipelines_api.py — clean additive merge. TestNonObjectJsonBodyReturns400 (lines 1294-1350, PR-added) and TestEpicModeNonEpicRejection (lines 1353-1473, from #2678 via main) exercise disjoint code paths against the same client fixture. The PR class uses the 8-param matrix harmonised in 51d145c5. No semantic overlap; the file's from pathlib import Path and from unittest.mock import MagicMock, patch imports (lines 8-9) are pre-existing and serve both classes.

• create_pipeline body guard still wraps data.get(...) — pipelines.py:1417-1421 is unchanged in position. #1557's new jira_ticket_arg = data.get("jira_ticket") (line ~1456) and epic_mode_arg = data.get("epic_mode") land after the dict guard, so a list/scalar body trips the canonical 400 envelope before reaching any new .get(...) call. Confirmed by pytest orchestrator/tests/test_pipelines_api.py::TestNonObjectJsonBodyReturns400 — all 16 cases (8 raw bodies × 2 routes) pass.

• update_pipeline body guard still wraps data.get(...) — guard moved from line 2191 → 2270 because intervening create_pipeline body grew, but the structural relationship (get_json() → is None check → isinstance(_, dict) check → call site) is unchanged.

• Full test_pipelines_api.py green — 61/61 pass locally, including the 16 PR-added cases and the 3 merged-from-main epic-rejection cases.

• No regressions in other PR-touched files — diff against 51d145c5 shows the only PR-relevant files that changed are pipelines.py (additive Jira-epic code from main, body guards untouched) and test_pipelines_api.py (additive conflict resolution). webhooks.py, signals.py, contracts.py, progress.py, messages.py, anchors.py, health.py, containers.py, and their test files are unchanged in the new delta.

Approving on the same basis as the prior round: the sweep is complete, the merge resolution preserves both sides' tests without losing coverage, and the new Jira-epic code added to create_pipeline lives behind the existing dict guard.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

21 previous review(s) hidden.