Skip to content

fix(providers): add PATCH handler to provider connection route (CLI rotate 405) - #10366

Open
benzntech wants to merge 4 commits into
diegosouzapw:release/v3.8.50from
benzntech:fix/providers-route-patch-method
Open

fix(providers): add PATCH handler to provider connection route (CLI rotate 405)#10366
benzntech wants to merge 4 commits into
diegosouzapw:release/v3.8.50from
benzntech:fix/providers-route-patch-method

Conversation

@benzntech

Copy link
Copy Markdown
Contributor

Problem

omniroute providers rotate <name> --new-key <key> fails with HTTP 405 on every run, while still reporting success. The CLI (both the hand-written bin/cli/commands/providers.mjs and the auto-generated bin/cli/api-commands/providers.mjs) sends PATCH /api/providers/[id] — matching the OpenAPI spec (docs/openapi.yaml declares patch for that path) — but the route only implements PUT, so PATCH returns 405.

Worse, the CLI's DB-write fallback only catches thrown exceptions, not non-OK HTTP responses, so the failure is silent: the key is never updated but the command exits 0.

Fix

Add a PATCH handler to src/app/api/providers/[id]/route.ts that delegates to the existing PUT handler. Both apply the same partial-update schema (updateProviderConnectionSchema — only provided fields are applied), so PATCH and PUT have identical semantics; the route now honors the method the spec and CLI already use.

Regression test

tests/unit/providers-route-patch-method.test.ts:

  • asserts the route exports a PATCH handler
  • invokes PATCH with an unauthenticated request and asserts it reaches the shared auth path (401), proving delegation rather than a 405/undefined
  • asserts PUT and DELETE exports remain intact

Verified to fail without the fix (2/3 tests fail on the pre-fix route) and pass with it. Route-validation gate passes (606 route files).

benzntech added a commit to benzntech/OmniRoute that referenced this pull request Aug 14, 2026
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for tracking this down — the PATCH/PUT mismatch between docs/openapi.yaml and the route is real and confirmed still present on the release branch, and delegating PATCH to the existing PUT handler is exactly the right fix (matches the same pattern already used by ~10 other routes in the codebase).

Two things need to land before this can merge:

  1. The regression test currently fails in CI as submitted — job "Unit Tests (6/8)" on this PR's run got 404 !== 401 at tests/unit/providers-route-patch-method.test.ts:37. The hardcoded 401 expectation assumes a specific unauthenticated-request outcome that doesn't hold in the CI DB/auth state. Could you loosen that assertion to something environment-independent, e.g. assert.notEqual(patchResult.status, 405) (which is really what you want to prove — delegation instead of a 405), or explicitly stub requireManagementAuth to a known state?
  2. Base branch — this PR targets main, but the repo's active branch is release/v3.8.50. Could you retarget there? Two side effects to be aware of when you do: the Quality Ratchet CodeQL check will pass once retargeted (the failure you're seeing is main's stale baseline, unrelated to your change), and the CHANGELOG.md hunk will need to be re-added by hand in release/v3.8.50's "Living section" format since the structure differs from main's.

Everything else — code, security, scope — looks good. Happy to help land this once those two are sorted.

benzntech and others added 4 commits August 15, 2026 17:18
The OpenAPI spec and the CLI (omniroute providers rotate, generated
api-commands) both use PATCH /api/providers/[id], but the route only
implemented PUT — PATCH requests returned 405 and key rotation via the
CLI silently failed while reporting success (the DB-write fallback only
catches thrown exceptions, not non-OK HTTP responses).

Add a PATCH handler delegating to the PUT handler: both apply the same
partial-update schema, so the semantics are identical.

Regression test proves the PATCH export exists and delegates into the
shared auth path; verified to fail without the fix.
The 'PATCH delegates to PUT' assertion hardcoded a 401, which only holds
when management auth is enforced (dev). In the CI unit-test env auth is not
required, so the flow falls through to 'Connection not found' (404) for an
unknown id — the test failed on the status code while the PATCH->PUT
delegation itself is correct. Assert on delegation equivalence instead:
PATCH must never 405 (the regression) and must return the same status as
PUT for the same input.

Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
The same Request was passed to both PATCH and PUT — PUT consumes the
body via request.json(), so the second call got an empty body (400
validation) vs the first (404 not-found): a false status mismatch on
bases where management auth is bypassed in the test env (release
v3.8.50). Fresh Request per invocation makes identical inputs produce
identical statuses.
@benzntech
benzntech force-pushed the fix/providers-route-patch-method branch from d2feb89 to 5129094 Compare August 15, 2026 11:50
@benzntech
benzntech changed the base branch from main to release/v3.8.50 August 15, 2026 11:50
@benzntech

Copy link
Copy Markdown
Contributor Author

Both points addressed — thanks for the guidance! 🙏

1. Test made environment-independent. The previous version passed the same Request to both PATCH and PUT — but PUT consumes the body via request.json(), so the second call got an empty body (400) vs the first (404), a false mismatch on bases where auth is bypassed in the test env. Now each handler gets a fresh Request with identical input, so the assertion PATCH.status === PUT.status (and ≠ 405) is robust everywhere. All 3 tests pass on the release/v3.8.50 base.

2. Retargeted to release/v3.8.50. Rebased the branch onto it (diff is now exactly 3 files: route.ts + test + CHANGELOG). The changelog hunk was re-applied in the living-section format — it sits under ### 🐛 Bug Fixes of the 3.8.50 living section, matching the surrounding entries.

CI should pick up the retarget now — happy to address anything that surfaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants