[From PR #1077] Consider extracting auth/host/origin gate tests from http_router_test.exs

[claude]

Background

Raised during review of PR #1077 (fix(http): validate Host and Origin for every MCP endpoint method).

Observation

mcp_server/test/ptc_runner_mcp/http_router_test.exs is ~902 lines. The auth/Host/Origin/content-type gate tests (the security-critical request-validation path) are scattered across top-level test blocks rather than grouped. As the suite grows, the security gate coverage gets harder to locate and reason about as a unit.

Current State (verified 2026-06-03)

• File: mcp_server/test/ptc_runner_mcp/http_router_test.exs, 902 lines — unchanged since PR fix(http): validate Host and Origin for every MCP endpoint method #1077 merged.
• Only one describe block exists today: describe "failed auth rate limiting" (~line 696), which already groups the rate-limiter tests including the Host/Origin "do not increment the failure counter" cases.
• All tests share a single non-trivial setup block (stops/starts SessionRegistry, inits ConcurrencyGate, snapshots/restores TraceConfig/Log level, on_exit teardown).

Gate tests that would move into a grouping (by current name)

• unauthenticated GET /mcp returns 401 bearer challenge (auth)
• health and ready are unauthenticated (auth)
• missing and bad auth return bearer challenges (auth)
• GET /mcp with bad Host returns 403 before 405 (Host)
• loopback bind rejects hostile Host before reading MCP POST body (Host)
• loopback bind rejects hostile Host on DELETE (Host)
• GET /mcp with invalid Origin returns 403 before 405 (Origin)
• missing Origin is allowed when Host is loopback (Origin/Host)
• invalid browser Origin is rejected even with loopback Host (Origin)
• POST rejects present non-JSON content types (content-type gate)

Suggested Approach

Prefer a clearly-named top-level describe "security gates (auth / Host / Origin / content-type)" block within the existing file, rather than a separate http_security_gates_test.exs.

Rationale: the shared setup block is non-trivial. A separate file would force either duplicating that setup or extracting it into a shared test-support helper — extra complexity for a purely cosmetic reorganization. A describe block reuses the existing setup for free and achieves the navigability goal. Only split into a separate file if a shared setup helper is later introduced for other reasons.

Acceptance Criteria

• The gate tests listed above are grouped under one clearly-named describe block (or a separate file with shared setup, if that path is chosen).
• No test assertions are changed — this is a pure move/regroup.
• mix test (in mcp_server/) passes with the same number of tests as before.
• The existing describe "failed auth rate limiting" block is left intact (or, optionally, nested logically near the gate block).

Decision / Priority

Deferred by the PR #1077 reviewer: "not worth splitting for a one-test addition. Revisit if the file continues to grow."

As of 2026-06-03 the file is still 902 lines — the deferral condition (file growth) has not been met. This remains a low-value, low-risk cosmetic tech-debt item. Recommend keeping it deferred until the file grows materially or a contributor picks it up as a quick cleanup; the acceptance criteria above make it actionable whenever that happens.

Source: PR #1077 reviewer suggestion

[claude]

Review Summary

Recommendation: deferred (low-priority tech-debt) — not adding ready-for-implementation.

What I verified

• Correct path is mcp_server/test/ptc_runner_mcp/http_router_test.exs (issue previously omitted the mcp_server/ prefix — fixed in the body).
• File is still exactly 902 lines — it has not grown since PR fix(http): validate Host and Origin for every MCP endpoint method #1077 merged, so the reviewer's deferral condition ("revisit if the file continues to grow") is not met.
• Only one describe block exists today (failed auth rate limiting, ~L696); the auth/Host/Origin/content-type gate tests are scattered across top-level test blocks.
• All tests share a non-trivial setup block (registry start/stop, ConcurrencyGate, TraceConfig/Log snapshot+restore, on_exit).

What I changed in the issue body

• Added the verified current state and the exact list of gate tests to regroup.
• Recommended a top-level describe block within the existing file over a separate *_test.exs, because a separate file would force duplicating or extracting the shared setup — needless complexity for a cosmetic move.
• Added concrete acceptance criteria (pure move, no assertion changes, same test count, mix test green).

Dependencies

None. No blockers.

Why deferred rather than ready

This is a purely cosmetic, low-value reorganization. The PR #1077 reviewer explicitly deferred it, and the triggering condition (file growth) still hasn't occurred. Auto-implementing it now would churn a security-critical test file for marginal benefit. Labeled tech-debt + deferred; acceptance criteria are in place so it's actionable whenever the file grows or someone picks it up as a quick cleanup.