Skip to content

fix(secrets): make OAuth token rotation atomic and fail-closed - #1003

Merged
kody-bot merged 3 commits into
mainfrom
cursor/oauth-token-rotation-safety-fcb3
Jul 28, 2026
Merged

kody-bot merged 3 commits into
mainfrom
cursor/oauth-token-rotation-safety-fcb3

Conversation

@kentcdodds

@kentcdodds kentcdodds commented Jul 28, 2026 •

Copy link
Copy Markdown
Owner

Summary

OAuth refresh token rotation had two production failure modes that permanently strand integrations (confirmed on Notion and x-kodykoala):

  1. Non-atomic dual write — rotated refresh + access tokens were persisted as two sequential secret_set calls. A worker eviction or failed second write left the integration with an unusable token pair.
  2. Authz after consume — package code without an allowed_packages grant still hit the provider first; when the subsequent persist was denied, rotating providers had already invalidated the old refresh token.

Survey (refresh-token rotation)

Providers that issue a new refresh token and invalidate the old one on refresh include X, Notion, GitHub (expiring user tokens), Slack (when rotation is enabled), and Linear. Spotify typically reuses the refresh token. The fix is shaped for every rotating provider, not X alone.

Fix

  • New secret_set_many capability: assertOnly preflight, then one D1 batch write.
  • Refresh path (kody:runtime + OpenAPI host-side refresh): assert mutate grants before the provider request; persist refresh-then-access in one atomic batch.
  • Raw tokens still travel as {{secret:…}} placeholders on the provider request; host allowlists are unchanged.

Tests

  • Package without grant fails before any provider request
  • Failed atomic persist leaves prior secret state unchanged
  • Successful rotation persists both secrets (refresh before access), asserted on stored state

npm run validate passes.

System recap — extends existing primitives (medium risk)

Mode: recap · Base: main · Head: cursor/oauth-token-rotation-safety-fcb3

Classification: extends — secrets write path and OAuth refresh helpers gain atomic multi-secret persist plus fail-closed mutate authorization before provider token requests.

Primitives touched

Primitive Group Impact
secrets assistant extends — secret_set_many + D1 batch atomic upsert/update
capabilities-execute runtime extends — refresh asserts grants then persists atomically
openapi-bindings assistant extends — host-side refresh matches the same fail-closed/atomic path
mcp-server surfaces extends — assertCanSetSecrets + secrets service/repo batch helpers

System map

OAuth refresh now authorizes package mutate grants, calls the provider with secret placeholders, then commits refresh+access secrets in one D1 batch.

Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).

flowchart LR
	capabilitiesExecute["capabilities-execute<br/>Capabilities execute runtime"]:::extended
	secrets["secrets<br/>Secret references"]:::extended
	openapiBindings["openapi-bindings<br/>OpenAPI provider bindings"]:::extended
	d1AppDb["d1-app-db<br/>D1 app database"]:::untouched
	capabilitiesExecute -->|"assertOnly then secret_set_many"| secrets
	openapiBindings -->|"assertCanSetSecrets then setSecretsAtomically"| secrets
	secrets -->|"D1 batch upsert/update"| d1AppDb
	classDef touched fill:#1a7f37,color:#fff
	classDef extended fill:#9a6700,color:#fff
	classDef added fill:#cf222e,color:#fff
	classDef untouched fill:#57606a,color:#fff
Loading

Change flow

sequenceDiagram
	participant Runtime as refreshAccessToken
	participant Secrets as secret_set_many
	participant Provider as OAuth token URL
	participant D1 as APP_DB batch
	Runtime->>Secrets: assertOnly (mutate grants)
	alt denied
		Secrets-->>Runtime: fail closed (provider never called)
	else allowed
		Runtime->>Provider: refresh_token via {{secret:…}}
		Provider-->>Runtime: access_token (+ optional refresh_token)
		Runtime->>Secrets: persist refresh then access
		Secrets->>D1: single batch commit
	end
Loading

Before / after

Path Before After
Persist two secret_set calls one secret_set_many D1 batch
Package without grant provider refresh, then deny deny before provider request
Write order refresh then access (sequential) refresh then access (atomic batch)

Invariants

  • Per-user secret isolation unchanged; package mutate still requires allowed_packages.
  • Integration host allowlist still enforced before bearer attachment.
  • Raw tokens stay out of fetch requests as placeholders ({{secret:…}}).
Open in Web Open in Cursor 

Summary by CodeRabbit

  • New Features
    • Added kody.secret_set_many to set multiple secrets in one operation, including an assertOnly mode.
    • Introduced atomic multi-secret persistence for consistent updates across related secrets.
  • Bug Fixes
    • Improved OAuth refresh flows to persist refreshed refresh/access token secrets together.
    • Strengthened authorization and fail-closed behavior for package-based secret writes.
  • Documentation
    • Updated sandbox/tooling guidance to reference secret_set_many and describe OAuth refresh usage.
  • Tests
    • Updated and expanded coverage for batched secret setting and atomic failure behavior.

Providers that rotate refresh tokens (X, Notion, GitHub with expiry, Slack
with rotation, Linear, and others) invalidate the old refresh token as soon
as they issue a replacement. Persist refresh+access secrets in one D1 batch
(refresh first) and assert package mutate grants before the provider request
so a permission denial cannot strand the integration.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@cursor[bot], you've reached your PR review limit, so we couldn't start this review.

Next review available in: 38 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: eba256fa-c5e1-443b-979c-bb784a2a6f5b

📥 Commits

Reviewing files that changed from the base of the PR and between 3ffa56a and b10ee95.

📒 Files selected for processing (1)
  • packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts
📝 Walkthrough

Walkthrough

The change adds atomic multi-secret persistence, package-aware write authorization, and the secret_set_many MCP capability. OAuth refresh flows now assert secret permissions before provider requests and persist refreshed refresh/access tokens together. Runtime, OpenAPI, registry, service, and authorization tests cover the updated behavior.

Changes

Atomic secret rotation

Layer / File(s) Summary
Atomic secret storage
packages/worker/src/mcp/secrets/repo.ts, packages/worker/src/mcp/secrets/service.ts, packages/worker/src/mcp/secrets/service.node.test.ts
Secret repository and service operations now validate, encrypt, authorize, and persist multiple secret updates atomically, with package-approval and rollback tests.
Secret write authorization and capability
packages/worker/src/mcp/secrets/package-access.ts, packages/worker/src/mcp/capabilities/secrets/*
Adds package-aware write authorization and exposes secret_set_many with assert-only validation and atomic persistence.
Execute-helper token refresh
packages/worker/src/mcp/execute-modules/*, packages/worker/src/mcp/instructions/execute-tool-description.ts
Execute helpers and generated preludes assert token secret availability before refresh and persist rotated tokens in one secret_set_many call.
OpenAPI integration refresh
packages/worker/src/mcp/capabilities/openapi-provider/operation-request.ts, packages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.ts
Integration 401 retries validate both token secrets and save refreshed tokens through setSecretsAtomically.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ExecuteHelper
  participant SecretCapability
  participant OAuthProvider
  ExecuteHelper->>SecretCapability: Assert refresh and access secret writes
  ExecuteHelper->>OAuthProvider: Request token refresh
  OAuthProvider-->>ExecuteHelper: Return rotated token payload
  ExecuteHelper->>SecretCapability: Persist refresh and access tokens atomically
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: atomic, fail-closed OAuth token rotation in secrets handling.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/oauth-token-rotation-safety-fcb3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kody-bot
kody-bot marked this pull request as ready for review July 28, 2026 17:44
@github-actions

github-actions Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor

🔎 Preview deployed: https://kody-pr-1003.kody-a99.workers.dev

Worker: kody-pr-1003
D1: kody-pr-1003-db
KV: kody-pr-1003-oauth-kv

Mocks:

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (5)
packages/worker/src/mcp/capabilities/openapi-provider/operation-request.ts (1)

387-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Guidance text misdescribes a preflight-permission failure as a persistence failure.

This catch fires when assertCanSetSecrets denies the mutate grant, before the provider is ever contacted (no tokens were fetched yet). The message says "cannot persist refreshed tokens... approving the package for those secrets", which implies a refresh already happened. Since this kodyGuidance text is surfaced to the calling agent to decide what to do next, more precise wording (e.g., "refusing to refresh tokens: missing permission to update...") would reduce confusion about what state the integration is actually in.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/capabilities/openapi-provider/operation-request.ts`
around lines 387 - 393, Update the catch guidance returned by the OpenAPI
request flow to describe the preflight permission denial from
assertCanSetSecrets, not failed token persistence. State that token refresh was
refused because the package lacks permission to update the integration’s
secrets, and instruct the caller to approve the package for those secrets before
retrying refreshAccessToken; preserve the existing error details and provider
context.
packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts (1)

45-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No unit test for this capability's handler or its assertOnly validation branch.

The superRefine contract (value required unless assertOnly) and the authorize-then-short-circuit ordering are the security-relevant parts of this file, and neither is exercised — run-kody-registry.node.test.ts only stubs the tool name. Want me to open an issue to track adding secret-set-many.node.test.ts?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts` around lines
45 - 56, The secret-set-many capability lacks tests covering its
security-sensitive validation and handler flow. Add secret-set-many.node.test.ts
tests for the superRefine contract, ensuring empty or non-string secret values
are rejected unless assertOnly is true, and verify the handler authorizes before
short-circuiting assertOnly requests.
packages/worker/src/mcp/secrets/package-access.node.test.ts (1)

299-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Good fail-closed test; consider pairing it with the allow path.

This only proves denial. A companion case where resolveSecret returns allowedPackages: ['pkg-1'] and assertCanSetSecrets resolves would lock in that a granted package isn't accidentally blocked by the intent: 'mutate' change.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/package-access.node.test.ts` around lines 299
- 330, Add a companion allow-path test alongside the existing
assertCanSetSecrets denial test. Configure mockModule.resolveSecret to return
allowedPackages containing 'pkg-1', invoke assertCanSetSecrets with the same
package context, and assert it resolves successfully, preserving the existing
mutate-grant behavior.
packages/worker/src/mcp/secrets/service.ts (1)

257-273: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Every secret is resolved and decrypted twice per secret_set_many request. Both layers independently call resolveSecret on the same names to answer the same question — does this user-scoped secret already exist — and each call performs a bucket lookup plus an AES-GCM decrypt whose plaintext is then discarded.

  • packages/worker/src/mcp/secrets/service.ts#L257-L273: this is the redundant second probe on the capability path; replace it with a getSecretEntry existence check, or let the authorization layer own the check and drop the loop.
  • packages/worker/src/mcp/secrets/package-access.ts#L157-L172: keep this as the authoritative probe, and consider resolving the secrets concurrently instead of sequentially since the checks are independent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/service.ts` around lines 257 - 273, Remove
the redundant decrypting existence probe in the user-scoped package path around
the service logic at packages/worker/src/mcp/secrets/service.ts:257-273 by using
getSecretEntry for existence only, or eliminate that loop and rely on
authorization. Keep packages/worker/src/mcp/secrets/package-access.ts:157-172 as
the authoritative resolveSecret check, and optionally make its independent
secret checks concurrent without changing authorization behavior.
packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts (1)

204-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Four copies of the same secret_set_many entry-builder. The assert and persist helpers differ only in whether each entry carries value and whether assertOnly is set; the generated prelude then duplicates both again. Any future change to name normalization or the availability guard has to be made in four places and kept in sync across the TS/string-template boundary.

  • packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts#L204-L263: collapse assertSecretsCanBePersisted and persistSecretsAtomically into one helper taking { assertOnly }, with the value included only when persisting.
  • packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts#L622-L671: apply the same collapse to __kodyAssertSecretsCanBePersisted / __kodyPersistSecretsAtomically so the prelude stays a mirror of the TS implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts` around lines
204 - 263, Collapse the duplicate secret entry-building logic into a single
helper accepting an assertOnly option, including each secret value only for
persistence and preserving the existing validation and secret_set_many
availability guard. Update both
assertSecretsCanBePersisted/persistSecretsAtomically at
packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts:204-263 and their
generated prelude counterparts
__kodyAssertSecretsCanBePersisted/__kodyPersistSecretsAtomically at
packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts:622-671 so both
implementations remain synchronized; both sites require direct changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts`:
- Around line 58-61: Update the secretSetManyCapabilityInputJsonSchema
construction to mark the nested properties of each secrets array item,
specifically the item value field, rather than only annotating the top-level
secrets property; preserve the existing schema conversion and
markSecretInputFields usage while passing the correct nested marker path or
handling the array items before flattening.

In `@packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts`:
- Around line 74-77: Update EXECUTE_HELPER_CAPABILITY_NAMES to retain both
secret_set and secret_set_many alongside integration_get, preserving the
existing advertised capability and compatibility for saved packages while
keeping the newer bulk helper available.

In `@packages/worker/src/mcp/secrets/repo.ts`:
- Around line 225-244: Replace the top-level RAISE(ABORT, ...) in the
assertAllApproved prepared statement with D1-supported validation that queries
for an unapproved secret before updates execute. Have the worker detect any
returned violation and abort the batch using its normal error path, preserving
fail-closed behavior without relying on RAISE() in an outer SELECT.

In `@packages/worker/src/mcp/secrets/service.ts`:
- Around line 390-402: Remove the broad /ABORT/i condition from the catch block
around the secret mutation flow, so only the explicit “package cannot mutate one
or more secrets” sentinel triggers the permission-denial error. Continue
rethrowing all other errors unchanged, including unrelated transaction or fetch
abort failures.
- Around line 334-376: Update the batch processing around the secret update loop
and saveSecretsAtomically to accumulate each secret’s storage delta and perform
one assertWithinStorageBytesEntitlement check after all entries are prepared,
before committing. In saveSecretsAtomically, likewise aggregate new-entry count
and invoke assertWithinEntitlement({ resource: 'secrets' }) once for the
complete batch, preserving existing validation and atomic commit behavior.

---

Nitpick comments:
In `@packages/worker/src/mcp/capabilities/openapi-provider/operation-request.ts`:
- Around line 387-393: Update the catch guidance returned by the OpenAPI request
flow to describe the preflight permission denial from assertCanSetSecrets, not
failed token persistence. State that token refresh was refused because the
package lacks permission to update the integration’s secrets, and instruct the
caller to approve the package for those secrets before retrying
refreshAccessToken; preserve the existing error details and provider context.

In `@packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts`:
- Around line 45-56: The secret-set-many capability lacks tests covering its
security-sensitive validation and handler flow. Add secret-set-many.node.test.ts
tests for the superRefine contract, ensuring empty or non-string secret values
are rejected unless assertOnly is true, and verify the handler authorizes before
short-circuiting assertOnly requests.

In `@packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts`:
- Around line 204-263: Collapse the duplicate secret entry-building logic into a
single helper accepting an assertOnly option, including each secret value only
for persistence and preserving the existing validation and secret_set_many
availability guard. Update both
assertSecretsCanBePersisted/persistSecretsAtomically at
packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts:204-263 and their
generated prelude counterparts
__kodyAssertSecretsCanBePersisted/__kodyPersistSecretsAtomically at
packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts:622-671 so both
implementations remain synchronized; both sites require direct changes.

In `@packages/worker/src/mcp/secrets/package-access.node.test.ts`:
- Around line 299-330: Add a companion allow-path test alongside the existing
assertCanSetSecrets denial test. Configure mockModule.resolveSecret to return
allowedPackages containing 'pkg-1', invoke assertCanSetSecrets with the same
package context, and assert it resolves successfully, preserving the existing
mutate-grant behavior.

In `@packages/worker/src/mcp/secrets/service.ts`:
- Around line 257-273: Remove the redundant decrypting existence probe in the
user-scoped package path around the service logic at
packages/worker/src/mcp/secrets/service.ts:257-273 by using getSecretEntry for
existence only, or eliminate that loop and rely on authorization. Keep
packages/worker/src/mcp/secrets/package-access.ts:157-172 as the authoritative
resolveSecret check, and optionally make its independent secret checks
concurrent without changing authorization behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ed584128-d421-41b4-9dbb-5f691777af0e

📥 Commits

Reviewing files that changed from the base of the PR and between 4d3ccd8 and d37d91b.

📒 Files selected for processing (14)
  • packages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.ts
  • packages/worker/src/mcp/capabilities/openapi-provider/operation-request.ts
  • packages/worker/src/mcp/capabilities/secrets/domain.ts
  • packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts
  • packages/worker/src/mcp/execute-modules/authenticated-fetch.node.test.ts
  • packages/worker/src/mcp/execute-modules/kody-runtime-utils.node.test.ts
  • packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts
  • packages/worker/src/mcp/instructions/execute-tool-description.ts
  • packages/worker/src/mcp/run-kody-registry.node.test.ts
  • packages/worker/src/mcp/secrets/package-access.node.test.ts
  • packages/worker/src/mcp/secrets/package-access.ts
  • packages/worker/src/mcp/secrets/repo.ts
  • packages/worker/src/mcp/secrets/service.node.test.ts
  • packages/worker/src/mcp/secrets/service.ts

Comment thread packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts Outdated
Comment thread packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts
Comment thread packages/worker/src/mcp/secrets/repo.ts Outdated
Comment thread packages/worker/src/mcp/secrets/service.ts
Comment thread packages/worker/src/mcp/secrets/service.ts
Replace SQLite RAISE (invalid outside triggers on D1) with a single CASE
UPDATE that commits all package-approved secrets or none. Mark nested
secret_set_many value fields, accumulate entitlement checks for the batch,
keep secret_set in the execute helper capability list, and stop rewriting
unrelated abort errors as permission denials.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
packages/worker/src/mcp/secrets/service.ts (2)

248-255: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Reject duplicate secret names alongside the scope check.

Repeated names make the repo's COUNT(*) = names.length guard fail, surfacing a misleading not approved for package error, and they also double-count against the storage entitlement. Validate uniqueness here where the batch is assembled.

♻️ Proposed validation
 	const scopes = new Set(input.secrets.map((secret) => secret.scope))
 	if (scopes.size !== 1) {
 		throw new Error('Atomic secret writes must share a single scope.')
 	}
+	const names = input.secrets.map((secret) => secret.name.trim())
+	if (new Set(names).size !== names.length) {
+		throw new Error('Atomic secret writes must use distinct secret names.')
+	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/service.ts` around lines 248 - 255, Update
the atomic secret batch validation around the existing scopes check to reject
duplicate secret names before persistence or entitlement checks. Build a Set
from the names in input.secrets, compare its size with the batch length, and
throw a clear validation error when duplicates are present; preserve the
existing single-scope and non-empty validations.

257-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Existence preflight decrypts every secret sequentially.

resolveSecret fetches and decrypts, so this loop costs N sequential D1 round trips plus N AES operations just to answer "does it exist" — and a corrupt ciphertext turns the create-not-allowed guard into a decrypt failure. Resolve the user bucket once and use getSecretEntry; the repo-level approval check remains the authoritative gate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/service.ts` around lines 257 - 273, Update
the user-scoped package preflight in the secret creation flow to resolve the
user bucket once, then check each trimmed name with getSecretEntry instead of
calling resolveSecret sequentially. Preserve the existing empty-name validation
and McpCallerError for missing entries, while allowing existence checks to avoid
decrypting ciphertext; keep the repository-level approval check as the
authoritative gate.
packages/worker/src/mcp/secrets/repo.ts (1)

216-221: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Per-update updatedAt is only honored for single-update batches.

The multi-update path stamps every row with updates[0].updatedAt and drops the rest, while the length === 1 path uses that row's own value. Hoist updatedAt to a single top-level input field so the signature can't express something the statement won't do.

♻️ Proposed signature change
 export async function updateApprovedUserSecretEntriesForPackageAtomically(input: {
 	db: D1Database
 	userId: string
 	packageId: string
+	updatedAt: string
 	updates: Array<{
 		name: string
 		description: string
 		encryptedValue: string
-		updatedAt: string
 	}>
 }): Promise<void> {
-	const updatedAt = input.updates[0]?.updatedAt
-	if (!updatedAt) {
-		throw new Error('At least one secret update is required.')
-	}
+	const updatedAt = input.updatedAt

Also applies to: 246-249

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/repo.ts` around lines 216 - 221, Update the
update-input type and related handling in the repository update flow so
updatedAt is a single top-level value shared by the entire batch, rather than a
field on each updates entry. Remove per-entry updatedAt usage in both the
single- and multi-update paths, and ensure the SQL statement consistently uses
the top-level timestamp.
packages/worker/src/mcp/secrets/service.node.test.ts (1)

612-612: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test name promises ordering it doesn't assert.

"refresh then access" is a stated guarantee of this PR, but the test only checks both final values. Either assert the bind/write order (e.g. capture the prepared params and check xRefreshToken precedes xAccessToken) or drop "refresh then" from the name.

Also applies to: 648-661

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/service.node.test.ts` at line 612, Update the
setSecretsAtomically persists refresh then access tokens together for package
grants test to verify write/bind ordering by capturing prepared parameters and
asserting xRefreshToken precedes xAccessToken; otherwise rename the test to
remove the unverified “refresh then” guarantee. Apply the same correction to the
related assertions around the additionally referenced test range.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/worker/src/mcp/secrets/repo.ts`:
- Around line 216-221: Update the update-input type and related handling in the
repository update flow so updatedAt is a single top-level value shared by the
entire batch, rather than a field on each updates entry. Remove per-entry
updatedAt usage in both the single- and multi-update paths, and ensure the SQL
statement consistently uses the top-level timestamp.

In `@packages/worker/src/mcp/secrets/service.node.test.ts`:
- Line 612: Update the setSecretsAtomically persists refresh then access tokens
together for package grants test to verify write/bind ordering by capturing
prepared parameters and asserting xRefreshToken precedes xAccessToken; otherwise
rename the test to remove the unverified “refresh then” guarantee. Apply the
same correction to the related assertions around the additionally referenced
test range.

In `@packages/worker/src/mcp/secrets/service.ts`:
- Around line 248-255: Update the atomic secret batch validation around the
existing scopes check to reject duplicate secret names before persistence or
entitlement checks. Build a Set from the names in input.secrets, compare its
size with the batch length, and throw a clear validation error when duplicates
are present; preserve the existing single-scope and non-empty validations.
- Around line 257-273: Update the user-scoped package preflight in the secret
creation flow to resolve the user bucket once, then check each trimmed name with
getSecretEntry instead of calling resolveSecret sequentially. Preserve the
existing empty-name validation and McpCallerError for missing entries, while
allowing existence checks to avoid decrypting ciphertext; keep the
repository-level approval check as the authoritative gate.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 52d40dea-9fcf-4756-b0ca-dcd57e5fc511

📥 Commits

Reviewing files that changed from the base of the PR and between d37d91b and 3ffa56a.

📒 Files selected for processing (6)
  • packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts
  • packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts
  • packages/worker/src/mcp/run-kody-registry.node.test.ts
  • packages/worker/src/mcp/secrets/repo.ts
  • packages/worker/src/mcp/secrets/service.node.test.ts
  • packages/worker/src/mcp/secrets/service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/worker/src/mcp/run-kody-registry.node.test.ts
  • packages/worker/src/mcp/capabilities/secrets/secret-set-many.ts
  • packages/worker/src/mcp/execute-modules/kody-runtime-utils.ts

Narrow the object type before reading nested secrets.items.value so
typecheck accepts the x-kody-secret marker write.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b10ee95. Configure here.

await assertSecretsCanBePersisted(kody, providerName, [
{ name: refreshTokenSecretName, secretKind: 'refresh token' },
{ name: accessTokenSecretName, secretKind: 'access token' },
])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refresh mutate checked unnecessarily

Medium Severity

Package OAuth refresh now runs an assert-only secret_set_many that requires mutate approval on both refresh and access token secrets before calling the provider. When the provider only returns a new access token, the prior flow only checked mutate on the access secret, so integrations with access-only package approval (common for non-rotating providers) can fail before refresh even though the refresh secret would not be written.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b10ee95. Configure here.

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.

3 participants