Repository navigation
Conversation
PATCH /api/credential/:id accepted only a label, so rotating an expired API key through the update endpoint silently kept the old secret. The core Credential.update already supported updating the value; only the protocol payload and the handler dropped it. - credential.update now takes an optional value next to label - the handler passes the value through to Credential.update - publish credential.updated when only the value changed, so connected clients refresh the rotated credential - regenerate openapi.json and the client bindings Fixes anomalyco#58043
There was a problem hiding this comment.
Rotation through PATCH /api/credential/:id works: the PR's rotation test fails on the base (old key kept) and passes here. Typecheck passes in protocol, server, core and client, and the generated OpenAPI spec and client files match a fresh regeneration.
Public API (additive, backwards compatible):
PATCH /api/credential/:credentialIDtakes a new optionalvalue(Credential.Value). Callers that send onlylabelare unaffected.CredentialUpdateInputin both client libraries (promise and effect) gainsvalue?.credential.updatednow fires for value-only updates too, including internal OAuth token refreshes.
Blocking: the event change in Credential.update breaks packages/core/test/credential.test.ts ('publishes global events only for observable credential mutations'). That test passes on the base. It also turns every internal OAuth refresh into a global event, which makes every connected client re-fetch integrations. See the inline comment.
The label-only `credential.updated` guard was deliberate: value-only updates are also how internal OAuth refreshes are stored, and publishing globally makes every connected client re-fetch integrations on each refresh. The API-level value passthrough alone unblocks rotation, per review.
There was a problem hiding this comment.
The new commit addresses the earlier point: Credential.update is back to the base version, value-only updates (including OAuth refreshes) stay silent, and the core event test passes again. Rotation still works: both server tests pass, and the stored key is read again on every model resolution, so the next request uses the new key. Typecheck passes in server and core.
Public API (additive, backwards compatible): PATCH /api/credential/:credentialID takes a new optional value (Credential.Value), and CredentialUpdateInput in both client libraries (promise and effect) gains value?. Event behaviour is unchanged.
No further concerns.
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
The new push only merges the latest dev. Against it, the change is the same as at my last review; the only difference is the regenerated CredentialUpdateInput, now in the flat format from #58875. Re-running the client generator and the protocol check:generated script both leave no diff, and both server tests pass. A PATCH with value: null returns 204 and keeps the stored key, so the | null in the generated type is harmless. I couldn't run the packages' typecheck scripts (they need a newer Bun than the one available here); plain tsc passes for protocol and client.
Public API (additive, backwards compatible, unchanged since last review): PATCH /api/credential/:credentialID takes a new optional value, and CredentialUpdateInput in both client libraries gains value?.
No new concerns.
Conflict in packages/server/src/handlers/credential.ts resolved by keeping the upstream Effect.gen / LocationServiceMap structure and carrying over the credential.update payload value passthrough from this branch.
Issue for this PR
Fixes #58043
Type of change
What does this PR do?
PATCH /api/credential/:idaccepted onlylabel, so rotating an expired API key through the update endpoint silently kept the old secret (204, no error, stored value untouched). The coreCredential.updatealready supported updatingvalue— only the protocol payload and the handler dropped it.What changed:
credential.updatetakes an optionalvaluenext tolabelvaluethrough toCredential.updatecredential.updatedstays limited to label changes; value-only updates (including internal OAuth refreshes) stay silent, per reviewopenapi.jsonand the client bindings are regeneratedScope: this unblocks rotation at the API level. The dashboard rename dialog is untouched and stays label-only; wiring it (or a "Replace key" action) to the new field can follow separately.
Reproduced the report against a local server before the change:
PATCHwith avaluereturned 204 andGET /api/credentialstill showed the old key. After the change the same sequence stores the rotated key.How did you verify your code works?
bun test test/credential.test.tsinpackages/server— one test rotates the key through PATCH and asserts the stored value, one asserts a label-only PATCH keeps the stored key. Also ran typecheck (protocol/server/client/core) and the pre-push gate (oxlint, sdk-docs, turbo typecheck 34/34).Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #42690, #56586
Public API: adds optional
valueto PATCH /api/credential/:credentialID and the clientcredential.updateinput (backwards compatible)