fix(api-public): return 405 for method mismatch instead of 500

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/rivet

• #5417 fix(api-public): return 405 for method mismatch instead of 5 … 👈
• main

Get stack: forklift get 5417
Push local edits: forklift submit
Merge when ready: forklift merge 5417

_{change ltrttuxy}

[claude]

Review: fix(api-public): return 405 for method mismatch instead of 500

Summary

Adds an api.method_not_allowed error and wires Router::method_not_allowed_fallback into api-public's router so requests to a known path with an unsupported method return a structured 405 instead of tripping the "auth not handled" guard in auth_middleware and falling through to a bogus 500. The ApiNotFound-style error struct, artifact JSON, and status-code mapping all follow existing conventions cleanly.

Bug: the 405 fallback also bypasses CORS, not just auth

method_not_allowed_fallback(...) is registered after both .layer(CorsLayer::new()...) and .layer(middleware::from_fn(auth_middleware)) in engine/packages/api-public/src/router.rs. The comment reasons this makes the fallback "run outside" auth_middleware, which is correct, but the same reasoning also excludes it from the CORS layer, which is very likely not intended.

I traced this through axum 0.8.4 (the version pinned in this workspace's Cargo.lock):

• Router::method_not_allowed_fallback calls PathRouter::method_not_allowed_fallback, which for every currently-registered route does *rt = rt.clone().default_fallback(handler) (path_router.rs).
• MethodRouter::default_fallback only fires when the current fallback is still Fallback::Default(_), in which case it calls self.fallback(handler), which does self.fallback = Fallback::BoxedHandler(BoxedIntoRoute::from_handler(handler)) (method_routing.rs). This unconditionally replaces whatever was in self.fallback with a brand-new, unwrapped handler; it does not compose with the previous value.
• Both .layer(CorsLayer) and .layer(auth_middleware) were applied earlier via MethodRouter::layer, which wraps self.fallback through Fallback::map while preserving the Default tag. So right before .method_not_allowed_fallback() runs, self.fallback is Fallback::Default(cors_and_auth_wrapped_route), but that whole wrapped value gets thrown away and replaced with the bare method_not_allowed_handler.
• At request time, MethodRouter::call_with_state dispatches to fallback.clone().call_with_state(...) whenever the method doesn't match any registered handler for that path, so the unwrapped handler runs with no CORS layer and no auth middleware, not just no auth middleware.

Concrete impact: a browser-based client that sends a cross-origin request to a valid path with an unsupported method (e.g. DELETE /namespaces) gets a 405 response with no Access-Control-Allow-Origin header. The browser will block the script from reading the response and surface a generic CORS network error in devtools instead of the structured 405 body, which undercuts the actual goal of this PR (giving API/browser consumers something more actionable than an opaque failure).

Suggested direction: register .method_not_allowed_fallback(method_not_allowed_handler) before .layer(CorsLayer::new()...) so the CORS layer wraps the custom fallback the same way it wraps a normal route (Fallback::map wraps BoxedHandler too, not just Default), and switch the auth layer from .layer(middleware::from_fn(auth_middleware)) to .route_layer(middleware::from_fn(auth_middleware)). MethodRouter::route_layer only wraps the per-method handlers (get/post/.../put) and never touches self.fallback, so it robustly excludes the fallback regardless of registration order, more resilient than relying on layer-ordering alone (which is easy to get subtly wrong, as this PR shows).

Minor: no test coverage for the new behavior

api-builder already has tests/errors.rs using axum_test::TestServer to assert status + error body for each error type, but api-public (where method_not_allowed_fallback is actually wired up) has no test directory. A test asserting 405 + {group: "api", code: "method_not_allowed"} on a mismatched-method request, plus an assertion that CORS headers are still present on the response, would have caught the bug above directly.

Nits

• The new ApiMethodNotAllowed error struct, error_response.rs status mapping, and the JSON artifact under engine/artifacts/errors/ all match the existing ApiNotFound pattern and repo conventions (the missing trailing newline in the JSON file matches the existing sibling artifacts, so that's fine as-is).
• Comment style on method_not_allowed_handler and the .method_not_allowed_fallback(...) call site is clear and follows the "complete sentences, no dashes" convention from CLAUDE.md.

Overall: a good, minimal fix for the 500 case, but the CORS regression above should be addressed before merging, ideally with a regression test covering both the status code and the CORS headers.

[abcxff]

#5418