Repository navigation
Auto-grant user secrets to self-authored packages, fork adoption, and one-click capability approval - #829
Merged
Conversation
Only packages forked from community listings still require an explicit allowed_packages grant. Provenance is determined by the presence of a community_forks row for the package id, with a new index on forked_package_id. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
The ?capability= deep link now renders the same Approve/Reject card as host and package approvals instead of prefilling the secret editor. Approving appends the capability to allowed_capabilities. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Require the explicit allowed_packages grant for secret mutations (secret_set, secret_delete, OpenAPI refresh writes) regardless of package provenance, and validate one-click capability approval names against a conservative identifier pattern. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe PR adds capability-scoped secret approvals, community-fork adoption, provenance-aware package access, mutation grants, indexed fork lookups, and corresponding documentation, instructions, and tests. ChangesSecret approval flow
Community-fork package authorization
Authorization contract and guidance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPExecutor
participant AccountSecretsUI
participant AccountSecretsHandler
participant SecretStorage
MCPExecutor->>AccountSecretsUI: provide capability approval URL
AccountSecretsUI->>AccountSecretsHandler: submit capability approval
AccountSecretsHandler->>SecretStorage: update allowed capabilities
SecretStorage-->>AccountSecretsHandler: return refreshed approval data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
kody-bot
marked this pull request as ready for review
July 21, 2026 18:48
Contributor
|
🔎 Preview deployed: https://kody-pr-829.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/account-secrets.ts (1)
772-889: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract shared logic across
package,capability, andhostapproval branches.All three branches (plus
package_bulk) repeat the same shape: list secrets, find the matching one, 404 if missing, call asetSecretAllowed*function with the deduped array, reload payload, return. Consolidating into a single helper parameterized by the field getter/setter would reduce this ~120 lines of near-duplicate logic and lower the risk of the branches silently diverging as new approval kinds are added (as capability was here).♻️ Sketch of a shared helper
async function applySingleSecretApprovalUpdate(input: { env: Env userId: string name: string scope: SecretScope storageContext: StorageContext | null update: (secret: SecretMetadata) => Promise<unknown> }) { const current = await listSecrets({ env: input.env, userId: input.userId, scope: input.scope, storageContext: input.storageContext, }) const secret = current.find( (item) => item.name === input.name && item.scope === input.scope, ) if (!secret) return null await input.update(secret) return secret }🤖 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/app/handlers/account-secrets.ts` around lines 772 - 889, Extract the repeated approval-update flow from the package, capability, and host branches—and the corresponding package_bulk path—into a shared helper near the approval handler. Parameterize it with the secret identity, environment/storage context, and an update callback that performs each branch’s existing deduplicated setter logic; have it return a missing-secret result so callers preserve the current 404 response, then retain each branch’s payload reload and response behavior.
🤖 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 `@docs/use/secrets-and-values.md`:
- Around line 17-24: Update the later package-approval section to match the
policy described here: user-authored packages may read and use user-scoped
secrets without explicit approval, while community forks require an
allowed_packages grant. State that explicit approval remains required for all
secret mutation paths, including secret_set, secret_delete, and OpenAPI
token-refresh writes.
In `@packages/worker/client/routes/account-secrets.tsx`:
- Around line 376-388: Update the “already added” message condition in the
surrounding account-secrets approval logic to match the capabilityAlreadyAdded
guard: require requestedCapability to be non-null and requestedHost and
requestedPackageId to be null before checking allowedCapabilities. Keep the
existing message and capabilityAlreadyAdded behavior unchanged.
In `@packages/worker/src/mcp/executor.ts`:
- Line 951: Update the capability-denial messaging flow around
extractFirstUrl(message) to branch when no approval URL is available: retain the
one-click approval-link instruction for a non-null URL, and provide manual
account-secrets instructions when it returns null before retrying.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 772-889: Extract the repeated approval-update flow from the
package, capability, and host branches—and the corresponding package_bulk
path—into a shared helper near the approval handler. Parameterize it with the
secret identity, environment/storage context, and an update callback that
performs each branch’s existing deduplicated setter logic; have it return a
missing-secret result so callers preserve the current 404 response, then retain
each branch’s payload reload and response 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: bd89d107-4cc6-45ee-92d5-5bbd1e7f0f04
📒 Files selected for processing (32)
docs/contributing/architecture/data-storage.mddocs/contributing/secret-host-approval.mddocs/guides/account-secret-setup.mddocs/guides/integration-bootstrap.mddocs/guides/package-authoring.mddocs/guides/package-lifecycle.mddocs/guides/secret-backed-integration.mddocs/use/secrets-and-values.mdpackages/worker/client/routes/account-approval-shared.node.test.tspackages/worker/client/routes/account-approval-shared.tspackages/worker/client/routes/account-secrets.tsxpackages/worker/migrations/0073-community-forks-forked-package-index.sqlpackages/worker/src/app/account-secrets-data.tspackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/app/loader-data.tspackages/worker/src/community/community-flow-test-schema.tspackages/worker/src/community/repo.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.tspackages/worker/src/mcp/capabilities/secrets/secret-delete.tspackages/worker/src/mcp/capabilities/secrets/secret-set.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/secrets/allowed-capabilities.node.test.tspackages/worker/src/mcp/secrets/allowed-capabilities.tspackages/worker/src/mcp/secrets/errors.node.test.tspackages/worker/src/mcp/secrets/errors.tspackages/worker/src/mcp/secrets/package-access.node.test.tspackages/worker/src/mcp/secrets/package-access.tspackages/worker/src/mcp/server-instructions.ts
A reviewed fork can be adopted via the new community_fork_adopt capability: adoption records adopted_at plus a review note on the community_forks row, preserving provenance while granting the same read/use auto-grant self-authored packages get. Mutations still require the explicit allowed_packages grant. Also fixes CodeRabbit findings: contradictory already-added capability notice and the capability denial next step without an approval URL. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
Bot
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/worker/src/mcp/capabilities/community/adopt.ts (1)
25-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider enforcing
package_id/kody_idexclusivity in the schema itself.The description says "provide exactly one of," but nothing in the Zod schema enforces it — the check only happens in
adoptCommunityFork. Adding a.refine()/.superRefine()would surface the constraint at the schema/tool-contract level instead of only via a service-thrown error.♻️ Optional schema-level refinement
- inputSchema: z.object({ + inputSchema: z.object({ package_id: z .string() .min(1) .optional() .describe( 'Saved package id (UUID). Provide exactly one of package_id or kody_id.', ), kody_id: z .string() .min(1) .optional() .describe( 'Package kody id in your account. Provide exactly one of package_id or kody_id.', ), review_summary: z .string() .min(10) .describe( 'What you reviewed in the forked code and why it is trusted enough to treat as your own for user-secret read/use.', ), - }), + }).refine((v) => (v.package_id !== undefined) !== (v.kody_id !== undefined), { + message: 'Provide exactly one of `package_id` or `kody_id`.', + }),🤖 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/community/adopt.ts` around lines 25 - 46, Update the inputSchema for the adopt capability to enforce that exactly one of package_id or kody_id is provided, using a Zod refine or superRefine at the object level. Keep the existing field validations and review_summary requirements unchanged, while preserving adoptCommunityFork’s existing behavior for valid inputs.packages/worker/src/community/repo.ts (1)
674-702: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider guarding against re-adoption races.
The
UPDATEhas noadopted_at IS NULLcondition, so two concurrent adopt calls for the same fork (racing past the service-layerif (fork.adoptedAt) returncheck) would both succeed and overwriteadoption_note/adopted_at. Low likelihood, but aWHERE ... AND adopted_at IS NULLclause combined with checkingchanges === 0would make this fully race-safe and let the service distinguish "already adopted concurrently" from "not found."🤖 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/community/repo.ts` around lines 674 - 702, Update markCommunityForkAdopted so its UPDATE only matches rows with adopted_at IS NULL, preserving the existing fork and user predicates. Keep the changes === 0 result handling so concurrent re-adoption or a missing fork returns null without overwriting existing adoption data.
🤖 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/community/repo.ts`:
- Around line 674-702: Update markCommunityForkAdopted so its UPDATE only
matches rows with adopted_at IS NULL, preserving the existing fork and user
predicates. Keep the changes === 0 result handling so concurrent re-adoption or
a missing fork returns null without overwriting existing adoption data.
In `@packages/worker/src/mcp/capabilities/community/adopt.ts`:
- Around line 25-46: Update the inputSchema for the adopt capability to enforce
that exactly one of package_id or kody_id is provided, using a Zod refine or
superRefine at the object level. Keep the existing field validations and
review_summary requirements unchanged, while preserving adoptCommunityFork’s
existing behavior for valid inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c8bb666a-bbb0-4999-88a7-a52ef7076b52
📒 Files selected for processing (25)
docs/contributing/architecture/data-storage.mddocs/guides/account-secret-setup.mddocs/guides/integration-bootstrap.mddocs/guides/package-authoring.mddocs/guides/package-lifecycle.mddocs/guides/secret-backed-integration.mddocs/use/secrets-and-values.mdpackages/worker/client/routes/account-secrets.tsxpackages/worker/migrations/0074-community-fork-adoption.sqlpackages/worker/src/community/community-flow-test-schema.tspackages/worker/src/community/community-service.node.test.tspackages/worker/src/community/repo.tspackages/worker/src/community/service.tspackages/worker/src/community/types.tspackages/worker/src/mcp/capabilities/community/adopt.node.test.tspackages/worker/src/mcp/capabilities/community/adopt.tspackages/worker/src/mcp/capabilities/community/domain.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/secrets/package-access.node.test.tspackages/worker/src/mcp/secrets/package-access.tspackages/worker/src/mcp/secrets/pending-package-secret-approvals.tspackages/worker/src/mcp/server-instructions.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/guides/package-lifecycle.md
- packages/worker/src/mcp/executor.ts
- docs/guides/package-authoring.md
- packages/worker/src/mcp/server-instructions.ts
- docs/guides/account-secret-setup.md
- docs/guides/integration-bootstrap.md
- packages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.ts
- docs/use/secrets-and-values.md
- packages/worker/src/mcp/fetch-gateway.node.test.ts
- docs/contributing/architecture/data-storage.md
- packages/worker/src/mcp/secrets/package-access.ts
- packages/worker/src/mcp/secrets/package-access.node.test.ts
- packages/worker/client/routes/account-secrets.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three friction reductions to the secret approval flows, keeping the gates that carry real security weight:
community_forksrow for the package id + user) can now read/use user-scoped secrets without an explicitallowed_packagesgrant. Enforced at the single chokepointassertPackageCanAccessResolvedSecret, with a newcommunity_forks.forked_package_idindex (0073). This is check-time logic only — nothing is ever written toallowed_packages.community_fork_adoptcapability (requires areview_summary; recordsadopted_at+ note on the fork row via migration0074). Adopted forks get the same read/use auto-grant while keeping full fork provenance (listing id, origin commit). Thepending_secret_package_approvalssteering now offers review-and-adopt as the first resolution, with per-secret/bulk approval links as the alternative.?capability=deep link now renders the same "Approve secret access" Approve/Reject card as host and package approvals, instead of prefilling the secret editor and requiring a full form save.Hardening added after an independent security review of the diff:
secret_set,secret_delete, and OpenAPI token-refresh writes from package runtimes passintent: 'mutate'and always require the explicitallowed_packagesgrant — regardless of provenance or adoption. Auto-grant covers read/use only (fetch placeholders, mounts, jwt-sign, capability inputs).[a-zA-Z0-9_.:-]{1,200}.Known accepted limitations
package_savecreates a "self-authored" package. Adoption makes the honest path cheaper than laundering while preserving provenance; host approval and the mutation gate remain the enforced backstops.Test plan
intent: 'mutate'deny/allow, adoption capability (happy path, not-a-fork, already-adopted, short review summary, cross-user isolation), capability approve/reject/dedupe/invalid-name, executor next-step with/without approval URLnpm run validate(format, lint, typecheck, unit tests, Playwright E2E, MCP E2E)System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@ff0ac1f8· Head:087a77e2Classification: extends — the package secret approval gate becomes fork-only for read/use (auto-grant for self-authored packages and adopted forks, mutations still gated), and capability approval gains the same one-click card hosts and packages already have.
Primitives touched
mcp-serverassertPackageCanAccessResolvedSecretallows non-forked and adopted packages forintent: 'use';secret_set/secret_delete/OpenAPI refresh passintent: 'mutate'; newcommunity_fork_adoptcapability; capability denial wording points at one-click approvalapp-ui?capability=deep link renders the Approve/Reject card; approve appends a format-validated name toallowed_capabilitiescommunity-listingsadopted_at/adoption_note(0074); new provenance/adoption repo + service functionsd1-app-dbcommunity_forks.forked_package_id(0073), adoption columns (0074)openapi-bindingsjobsSystem map
Package runtimes resolving user secrets consult community-fork provenance and adoption state before requiring an
allowed_packagesgrant (reads only), and capability approvals flow through the account secrets one-click card.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
Before: every package runtime (including packages the user authored) needed each user secret to list its id in
allowed_packages; capability approval required editing the secret form and saving.After: self-authored packages and adopted forks read/use user secrets without a grant; unadopted community forks keep the explicit grant with adopt-or-approve steering; mutations (
secret_set,secret_delete, OpenAPI refresh writes) always require the explicit grant. The?capability=link shows the same one-click Approve/Reject card as host/package approvals, with capability names validated against[a-zA-Z0-9_.:-]{1,200}.Invariants
Per-user isolation is preserved: the provenance lookup and the adoption UPDATE both filter by
forked_package_idandforker_user_id, and all secret resolution stays scoped byuserId. Host approval (deny-by-default egress) and capability allowlists remain enforced on every path, including ad hoc execute.Summary by CodeRabbit
New Features
Bug Fixes
Documentation