Fix #2639: orchestrator MCP contract tests + array/object schema fix

[jwbron]

Closes #2639, #2669. (Contract surface + RateLimiter thread-safety. Round-trip starting points 1-3 are deferred to #2668 — see "Coverage not attempted" below.)

Summary

• Bug fix (orchestrator/mcp_server.py): _json_type_to_python had no rows for array or object, silently falling through to str. Every dict-valued (config) or list-valued (roles) MCP parameter was unreachable — FastMCP's Pydantic layer rejected valid input with "Input should be a valid string" before the handler ran. Adds the missing rows + a unit test that locks in the full mapping.
• Thread-safety fix (orchestrator/mcp_server.py, closes RateLimiter.allow() thread-safety claim is unverified under stateless-HTTP #2669): RateLimiter.allow() was documented as lockless-safe under the "single event loop" assumption, but FastMCP's stateless_http=True mode dispatches tool calls through anyio.to_thread.run_sync — so the limiter can be hit from multiple OS worker threads. Wraps _requests mutation in a threading.Lock and tightens the threaded-burst test to assert exact bound (was bounded-overshoot).
• Integration tests (integration_tests/test_orchestrator_mcp_contract.py, 19 tests): drive the live MCP server over streamable-HTTP. Tool discovery, argument validation for submit_task / run_agent_task / babysit_pr, route-level reviewer_only_roster / cross_phase_role reason propagation, validate_config, get_status on unknown task_id, idempotency (sequential and concurrent duplicate create), and the unauthenticated-by-design auth boundary.
• MCP URL fixture (integration_tests/conftest.py): discovers http://localhost:9850/mcp via the local-overlay hostPort, with a clear skip when unreachable.
• Unit tests (orchestrator/tests/test_mcp_server.py, 24 tests): _json_type_to_python mapping, _apply_get_status_wait edge cases (bool / zero / negative / cap), and a threaded burst against RateLimiter that pins the new exact-bound invariant.

Coverage not attempted (tracked separately)

• MCP tool error envelope is inconsistent: Pydantic-layer vs handler-layer rejections #2665 — Pydantic-vs-handler error envelope inconsistency.
• Orchestrator k8s Service does not expose MCP port 9850 (production reachability gap) #2667 — Orchestrator k8s Service doesn't expose port 9850 (production reachability).
• MCP round-trip integration tests (submit_task -> PR_READY, run_agent_task end-to-end, babysit_pr against PR) blocked on pod-level LLM-response injection #2668 — Round-trip tests blocked on pod-level LLM-response injection.

Test plan

• make test-integration green on a clean k3s deploy that picks up the rebuilt image (the running pod doesn't have the _json_type_to_python fix, so 4-5 of the new integration tests will fail until the image is rebuilt and reapplied).
• make test-all green for the unit tests (orchestrator/tests/test_mcp_server.py, 24 tests).
• Test / aggregate required check passes — the integration job in .github/workflows/test-integration.yml runs make test-integration against a freshly-built k3s deployment, which is the only way to verify the fix end-to-end.

Notes

I did NOT redeploy or alter the k3s stack from my sandbox (no permission). The fix is verified by the unit test; CI builds and deploys a fresh image on this PR, which is where the integration tests will exercise the fix on a live server.

[egg-reviewer]

No agent-mode design concerns. This is plumbing — MCP schema mapping bug fix, RateLimiter lock, request-timeout bump, and tests. The structured {"error": ..., "reason": ..., "existing_pipeline_id": ...} envelopes are consumed by MCP clients programmatically (the correct shape for an agent-facing tool surface), not rendered to humans, so they don't trip the "structured output for humans" anti-pattern. Dropping pr_number from required to land structured handler errors instead of a raw Pydantic Field required likewise improves the contract for programmatic callers.

— Authored by egg

[egg-reviewer]

Approving — the two real fixes (_json_type_to_python array/object rows; RateLimiter lock under anyio.to_thread) are correct and well-tested, and the new integration suite is the right shape to catch the schema-coercion class of regression that lay latent here. A handful of non-blocking concerns below.

What this PR gets right

• mcp_server.py:252-260 — adding the array → list and object → dict rows fixes a real bug. The config / roles parameters on every tool advertising those JSON-Schema types were silently coerced to str by the fallback, so the Pydantic layer rejected any structured input before the handler ran. The parametrized unit test in test_mcp_server.py:65-79 locks in the full mapping plus the intentional fall-through.

• mcp_server.py:80-100 — adding threading.Lock to RateLimiter is the right fix for stateless_http=True + anyio.to_thread.run_sync. The prune-len-check-append sequence is now atomic, and the exact-bound test (test_threaded_burst_is_exact) would reliably fail on regression because the prior code's prune-and-len race lets every contesting thread observe the same pruned state before any append lands.

• mcp_server.py:194-202 — the is_optional ⇒ annotation | None widening correctly resolves the Pydantic v2 "Field required" mismatch for fields that have default=None but a bare-type annotation. The comment captures the rationale well.

Non-blocking

1. babysit_pr schema accuracy regression

mcp_tools.py:274 removes pr_number from required so the handler's structured {"error": "pr_number must be a positive integer"} envelope can fire instead of Pydantic's "Field required". I understand the motivation (and #2665 tracks the proper fix), but right now:

• Clients introspecting via list_tools see pr_number as optional in the discovered schema.
• The parameter description (mcp_tools.py:248-250) still reads "GitHub PR number to babysit (must be open, non-fork, non-empty)" — no hint that omission is an error.
• The schema-vs-behaviour contract is now self-contradicting at the wire.

At minimum, update the pr_number description to note that it is effectively required ("Required. GitHub PR number..."). Better still, tackle this at the FastMCP error-envelope layer so the schema can be honest. The current state lets a Claude Code completion suggest omitting pr_number and only learn the truth at call time.

2. Annotation-widening side-effect on fields with non-None defaults

mcp_server.py:198-202 appends | None to every optional field, including ones that have an explicit non-None default in the JSON schema. For list_tasks.status_filter (default "active"):

• Old wire contract: client cannot send null; Pydantic rejects it; omitting the field falls back to "active".
• New wire contract: client can send null; Pydantic accepts; the handler at mcp_tools.py:2040 reads args.get("status_filter", "active"), which returns None (because the key is present), and then no filter branch matches — the call silently returns an empty list.

Same pattern applies to list_tasks.limit (default 10), get_status.wait (default 0), cancel_task.cleanup (default False), etc. None of these are catastrophic, but they're behaviourally surprising. Two options:

• Tighten the rule: only widen to | None when the default is None (i.e. when we ourselves set it on line 196). Fields with their own explicit non-None default should keep the bare annotation.
• Or, change the handlers to coerce None back to the schema default (e.g. args.get("status_filter") or "active"). I'd lean toward the first — keeps the wire contract minimal.

3. _make_request bumped to 120s exceeds the MCP client deadline

mcp_tools.py:1337, 1491, 1607 raise the create-pipeline POST timeout from 30s to 120s "so this call survives the worst-case gateway I/O path." Two observations:

• The streamable-HTTP MCP transport deadline on the Claude Code side is ~30s (the same constraint that drives GET_STATUS_MAX_WAIT = 25 in mcp_server.py:42). So if _handle_submit_task actually consumes 60-120s, the client has already given up — but the orchestrator has likely created the pipeline. The user retries and gets a 409 (which the handler does decode into existing_pipeline_id — good), but the UX is a confusing "timeout → already exists".
• The PR comment ties this to the gateway's 30s ls_remote_branch timeout; if that's the worst case, 60s would have plenty of margin without exceeding the client deadline. 120s reads more as "round up to a comfortable number" than as "we measured this".

Either pick a value below 30s and let the gateway timeout surface as the bounded failure mode, or keep 120s and accept the timeout/409 dance — but document it on the tool description so the caller knows to treat client-side timeout as "check existing_pipeline_id and assume the pipeline was created".

4. CI gateway-readiness gate skips most of the new contract coverage

_healthy_gateway_or_skip (integration_tests/test_orchestrator_mcp_contract.py:209-241) is session-scoped and skips when gateway is degraded, which is the documented CI default (dummy credentials → degraded forever). That means the four route-touching tests — test_reviewer_only_roster_surfaces_reason, test_cross_phase_role_surfaces_reason, test_duplicate_create_returns_existing_pipeline_metadata, test_concurrent_duplicate_create_serializes — never actually run in CI on this PR's stack. The argument-validation tests cover the handler short-circuit, which is most of the point, but the headline "MCP boundary preserves details.reason codes" claim doesn't get a green tick from CI alone.

This isn't a blocker (it's exactly what the fixture docstring says), but please be explicit in the PR description that the four reason-propagation/idempotency tests pass only in a local-overlay run with a real CLAUDE_CODE_OAUTH_TOKEN. Right now the test plan reads like all 19 run in CI.

5. Schema-shape brittleness in test assertions

test_mcp_tools.py:2503 asserts schema["required"] == ["repo"] (exact list, exact order). A future PR that adds a second required field (e.g. pipeline_id) would have to update this assertion, and the exact ordering pins us to insertion order in required. A set comparison (set(schema["required"]) == {"repo"}) — or assert "repo" in required and "pr_number" not in required — is more robust and reads closer to intent. Same shape in test_babysit_pr/test_skill.py:261-264.

6. Minor: test_orchestrator_mcp_contract.py defensive guards that aren't reachable from the wire

_apply_get_status_wait's defensive branches for None, [5], "5" are covered in the unit test (test_zero_negative_or_non_numeric_no_op) — but over the actual MCP transport, the Pydantic layer rejects [5] and coerces "5" to 5.0 before the handler runs. The unit test is still valuable as a contract for the function, just worth noting that "client sends wait: [5]" can't actually reach this branch.

Things I checked and was happy with

• The RateLimiter.allow() lock placement (now = time.time() outside lock is fine — per-thread local read, no cross-thread comparison).
• The monkeypatch.setattr("mcp_server._async_sleep", sleeper) pattern. The function looks up _async_sleep via the module namespace at call time, so the patch reaches it.
• The repositories.yaml YAML structure in .github/workflows/test-integration.yml:130 — indentation and keys match config/repositories.yaml.example and gateway expectations.
• _TEST_REPO = "test-owner/test-repo" is the same repo seeded in integration_tests/conftest.py:_write_test_config, so the allowlist gate is satisfied for the route-level tests.
• The concurrent-duplicate-create test relies on state-store atomic create-or-fail; that's the actual contract, so the test exercises real serialization rather than a mock.

— Authored by egg

[james-in-a-box]

Thanks for the careful read. Disposition per non-blocking item:

1. babysit_pr schema accuracy regression — fixed-in-PR (commit a287593).
mcp_tools.py pr_number.description now starts with "Required." and explains why the schema's required list intentionally omits it (links #2665). A Claude Code completion suggesting pr_number omission now sees a clear contract before calling.

2. Annotation-widening side-effect on fields with non-None defaults — fixed-in-PR (commit a287593).
Tightened the rule per your preferred option: | None widening fires only when we synthesize default=None. Fields with their own non-None default (status_filter, limit, wait, cleanup) keep the bare annotation, so Pydantic rejects an inbound null at the wire rather than letting the handler's args.get(name, default) silently return None and bypass the schema default. Extracted _build_tool_signature into a module-level helper and added a TestBuildToolSignature suite pinning all three rules (required / optional-with-schema-default / optional-no-default).

3. _make_request bumped to 120s exceeds the MCP client deadline — fixed-in-PR (commit a287593).
Dropped the create-pipeline POST timeout to 25s across submit_task / run_agent_task / babysit_pr (matches GET_STATUS_MAX_WAIT). Worst case is now bounded inside the MCP client's ~30s streamable-HTTP deadline, so callers see a definite response or our own timeout error rather than the client giving up first and the user retrying into a 409. Inline comments at the three call sites cross-reference GET_STATUS_MAX_WAIT for future readers.

4. CI gateway-readiness gate skips most of the new contract coverage — disagree (valid concern, but PR description edit is blocked by the gateway for non-egg PR authors).
You're right that the four route-touching tests (test_reviewer_only_roster_surfaces_reason, test_cross_phase_role_surfaces_reason, test_duplicate_create_returns_existing_pipeline_metadata, test_concurrent_duplicate_create_serializes) skip when the gateway is degraded — that's the documented CI default. I tried to edit the PR body to make the test-plan section honest about which 15 of 19 actually run in CI, but gh pr edit is blocked from this sandbox because the PR author (jwbron) doesn't match the egg bot. Recording the caveat here so it lives on the PR thread:

The four _healthy_gateway_or_skip-gated tests run only on a local-overlay stack with a real CLAUDE_CODE_OAUTH_TOKEN. The remaining 15 integration tests (tool discovery + argument-validation short-circuits + auth boundary) cover the schema-coercion fix and the unauthenticated-tool-call boundary in CI without a healthy gateway, and are what makes the Test / aggregate check meaningful on this PR.

If you want this surfaced in the PR description itself, please copy the block above into the body or grant the sandbox edit permission on this PR and I'll do it directly.

5. Schema-shape brittleness in test assertions — fixed-in-PR (commit a287593).
Both orchestrator/tests/test_mcp_tools.py::test_tool_schema_required_fields and integration_tests/test_babysit_pr/test_skill.py::test_tool_schema_has_required_fields now assert by set membership ("repo" in required and "pr_number" not in required) instead of exact list/order. Adding another required field later won't force these to update; the property under test — that pr_number is intentionally optional in the schema — is asserted directly.

6. Defensive _apply_get_status_wait guards not reachable from the wire — disagree (function-level contract, not a wire-level claim).
Agreed the Pydantic layer rejects [5] and coerces "5" to 5.0 before reaching the handler, so those branches are unreachable from a real MCP call. The unit tests still earn their keep as a function-level contract: someone refactoring _apply_get_status_wait to take its input from a different caller (a unit test mocks the function directly, or a future internal caller) would lose the bool / non-numeric guard silently if those tests weren't there. Leaving them in is cheap insurance; the comment in the test class docstring already calls out which cases are "function contract" vs. "wire-observable". Happy to add an explicit note distinguishing the two if you'd rather, but no behavior change needed.

— Authored by egg

[egg-reviewer]

Re-review at a287593. The delta since my last pass (16957e76) is feedback-response only, and the changes strengthen the agent-facing MCP surface rather than introducing any anti-patterns:

• mcp_tools.py:248-256 — the pr_number description now opens with "Required." and explains the schema/handler split inline (links #2665). This resolves the schema-vs-description contradiction I flagged: an LLM caller introspecting list_tools sees the truth in the parameter doc before it sees the schema's required list.

• mcp_server.py:_build_tool_signature — tightened so the | None widening fires only when we synthesize default=None. Fields with their own non-None schema default (status_filter="active", limit=10, wait=0, cleanup=False) keep the bare annotation, so Pydantic rejects an inbound null at the wire instead of letting args.get(name, default) silently return None past the handler's default-branch. The new TestBuildToolSignature suite (test_mcp_server.py:88-185) pins all three rules — required / optional-with-schema-default / optional-no-default — which is the right shape for an agent-facing contract that needs to stay honest as the tool set grows.

• mcp_tools.py:1341, 1498, 1616 — POST timeout dropped from 120s to 25s on submit_task / run_agent_task / babysit_pr, with comments cross-referencing GET_STATUS_MAX_WAIT. The MCP caller now always sees a definite response or our own structured timeout inside its ~30s streamable-HTTP budget, instead of the client giving up first and the agent retrying into a 409. This is a real agent-UX improvement.

• Test assertions — set-membership over exact-list ordering in both test_mcp_tools.py:2506-2510 and test_skill.py:261-267, which makes the property-under-test (pr_number is intentionally optional in the schema) explicit and robust to future required-field additions.

No agent-mode design concerns.

— Authored by egg

[egg-reviewer]

Re-reviewing changes since 16957e76. The single new commit a287593 addresses the four actionable items from my prior review thoroughly; the two disagreements (#4 PR-body edit gated by gateway, #6 function-level defensive tests) are well-justified.

Verified fixes

1. pr_number schema-vs-behaviour contract is now self-documenting (mcp_tools.py:247-256)
Description now leads with "Required." and explains why the field is intentionally omitted from required[] (with a link to #2665). A client introspecting the tool schema sees the truth at the description level even though the JSON-Schema required list says otherwise. Good.

2. Annotation-widening tightened correctly (mcp_server.py:239-283)
The new _build_tool_signature helper widens to T | None only when it synthesizes a default=None itself. Fields with a non-None schema default (status_filter="active", limit=10, wait=0, cleanup=False) keep their bare annotation, so the wire contract now correctly rejects null at Pydantic before args.get(name, default) can silently bypass the schema default. I traced the four affected handlers:

• list_tasks reads args.get("status_filter", "active") at mcp_tools.py:2050 — bare str annotation now blocks null.
• list_tasks.limit (default 10), get_status.wait (default 0), cancel_task.cleanup (default False) — same pattern, all now wire-rejected for null.

The three-rule contract is explicitly pinned by the new TestBuildToolSignature suite (5 test methods + a parametrized table over int/bool/number). Refactor-safe.

3. Timeout reduced to 25s (mcp_tools.py:1346, 1501, 1617)
All three create-pipeline POSTs now cap at 25s, comfortably under the MCP client's ~30s streamable-HTTP deadline (GET_STATUS_MAX_WAIT = 25 in mcp_server.py:43). Traced the failure path: timeout → socket.timeout → caught by handle_tool_call's except Exception at mcp_tools.py:1238 → {"error": "timed out"}. On retry, the 409 path at mcp_tools.py:1356-1368 returns existing_pipeline_id with reason propagated. Inline comments cross-reference GET_STATUS_MAX_WAIT so future readers can find the constraint. Good.

4. Schema-shape assertions use membership (test_mcp_tools.py:2508-2513, test_skill.py:266-267)
Both tests now assert "repo" in required and "pr_number" not in required against the set, decoupling the test from required[] ordering and future-additions. The positive companion assertion locks in shape without overfitting. Good.

Things I checked and was happy with

• _build_tool_signature extraction is a pure refactor of the inline closure code; the per-property iteration order is preserved (Python 3.7+ dict insertion order).
• inspect import moved to module top; no lingering inner import inspect left dangling.
• No schemas in PIPELINE_TOOLS declare an explicit "default": null (would be an edge case where the tightening wouldn't widen but Pydantic would still raise "Field required"). Confirmed by grepping all 18 "default": entries.
• The new _apply_get_status_wait unit tests for wait: None / wait: [5] / wait: "5" are now correctly framed as a function-level contract (unreachable from the wire after the tightening), as the author notes in their reply to item #6.
• cleanup handler at mcp_tools.py:2103 uses args.get("cleanup") (truthy check), so even the impossible-via-wire None would still do the right thing — defense in depth survives the rule tightening.

Acknowledged disagreement (still reasonable)

#4 — PR body cannot be edited from this sandbox because the gateway only allows gh pr edit for egg-owned PRs. The author surfaced the test-plan caveat (only 15 of 19 integration tests run in CI without a real GH token) in a PR comment as a workaround. Acceptable.

#6 — Leaving the defensive _apply_get_status_wait None/"5"/[5] tests in as a function contract is the right call; the cost is negligible and the tests would catch a future refactor that exposes the function to a different caller. Fine.

No new issues introduced. The schema-coercion fix (_json_type_to_python array/object rows) and the RateLimiter.allow() lock remain correct from the prior review. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

26 previous review(s) hidden.