Repository navigation
Add package approvals to connect secret - #311
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughAdds support for package-scoped secret allowlisting via an Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (UI)
participant Route as ConnectSecretRoute (Client)
participant Server as Connect-Secret Handler (Server)
participant MCP as MCP Secrets Service
participant Store as Secret Storage
Browser->>Route: open /connect/secret?allowedPackages=pkgA,pkgB
Route->>Route: normalizeAllowedPackages([...])
Route->>Server: POST create/update secret with allowedPackages
Server->>MCP: setSecretAllowedPackages(secretName, normalizedAllowedPackages)
MCP->>Store: persist allowed_packages with secret
Store-->>MCP: confirm persisted
MCP-->>Server: success
Server-->>Route: API response with allowed_packages
Route-->>Browser: render review UI showing Packages to approve
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
kentcdodds
marked this pull request as ready for review
May 1, 2026 17:40
Contributor
|
🔎 Preview deployed: https://kody-pr-311.kentcdodds.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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/worker/client/routes/connect-secret.tsx (1)
266-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat an empty package list as an explicit clear here.
Users can now remove every package row in the editor, but this fallback preserves the old connector binding whenever the normalized list is empty. That makes the saved connector metadata diverge from the reviewed form state.
♻️ Suggested fix
- const allowedPackages = - normalizedAllowedPackages.length > 0 - ? normalizedAllowedPackages - : resolved.allowedPackages + const allowedPackages = Object.hasOwn(body, 'allowedPackages') + ? normalizedAllowedPackages + : resolved.allowedPackages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/connect-secret.tsx` around lines 266 - 279, The POST body currently omits allowedPackages which causes an empty editor list to fall back to the old connector binding; update the fetch body in connect-secret.tsx so it always sends the normalized package list (e.g. add allowedPackages: params.allowedPackages ?? [] to the JSON body) so an explicit empty array clears bindings when users remove all packages.packages/worker/src/app/handlers/connect-secret.ts (1)
258-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve an explicit empty package list when updating connector metadata.
The new UI lets users remove every package row, but this fallback reuses the previously stored packages whenever the normalized list is empty. That means the review screen can show “None” while the connector binding still keeps the old package list.
♻️ Suggested fix
- const allowedPackages = - normalizedAllowedPackages.length > 0 - ? normalizedAllowedPackages - : resolved.allowedPackages + const allowedPackages = Object.hasOwn(body, 'allowedPackages') + ? normalizedAllowedPackages + : resolved.allowedPackages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/app/handlers/connect-secret.ts` around lines 258 - 279, The code currently falls back to resolved.allowedPackages whenever normalizedAllowedPackages.length === 0, which overwrites an intentional "clear all" action; update the allowedPackages assignment to treat an explicitly provided empty array as a valid value (i.e., check for undefined/null rather than length). Concretely, in the block that builds allowedPackages (and only that variable), replace the ternary using normalizedAllowedPackages.length > 0 with a check like normalizedAllowedPackages !== undefined (or normalizedAllowedPackages != null) so that an empty array from the UI is preserved when calling saveValue for buildConnectSecretConnectorBindingValueName(connector).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/worker/client/routes/connect-secret.tsx`:
- Around line 266-279: The POST body currently omits allowedPackages which
causes an empty editor list to fall back to the old connector binding; update
the fetch body in connect-secret.tsx so it always sends the normalized package
list (e.g. add allowedPackages: params.allowedPackages ?? [] to the JSON body)
so an explicit empty array clears bindings when users remove all packages.
In `@packages/worker/src/app/handlers/connect-secret.ts`:
- Around line 258-279: The code currently falls back to resolved.allowedPackages
whenever normalizedAllowedPackages.length === 0, which overwrites an intentional
"clear all" action; update the allowedPackages assignment to treat an explicitly
provided empty array as a valid value (i.e., check for undefined/null rather
than length). Concretely, in the block that builds allowedPackages (and only
that variable), replace the ternary using normalizedAllowedPackages.length > 0
with a check like normalizedAllowedPackages !== undefined (or
normalizedAllowedPackages != null) so that an empty array from the UI is
preserved when calling saveValue for
buildConnectSecretConnectorBindingValueName(connector).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55c99b16-b426-4a37-aa72-aeedda3384eb
📒 Files selected for processing (8)
docs/guides/connect-secret.mde2e/connect-secret.spec.tspackages/worker/client/routes/connect-secret.tsxpackages/worker/client/routes/secret-editor-fields.tsxpackages/worker/client/routes/secret-normalization.tspackages/worker/src/app/handlers/connect-secret.node.test.tspackages/worker/src/app/handlers/connect-secret.tspackages/worker/src/mcp/generated-ui-api.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b7d0bab. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/connect-secret.node.test.ts (1)
121-123: ⚡ Quick winUse the real package normalization behavior in this test setup.
The current mock returns the input unchanged, so these handler tests won’t catch regressions in trimming, deduping, or sorting
allowedPackages. Add a case with whitespace or duplicate package names, or mirror the real normalizer here.♻️ Proposed fix
vi.mock('#mcp/secrets/allowed-packages.ts', () => ({ - normalizeAllowedPackages: (packages: Array<string>) => packages, + normalizeAllowedPackages: (packages: Array<string>) => + Array.from( + new Set( + packages.map((value) => value.trim()).filter((value) => value.length > 0), + ), + ).sort((left, right) => left.localeCompare(right)), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/app/handlers/connect-secret.node.test.ts` around lines 121 - 123, The test currently mocks normalizeAllowedPackages to return inputs verbatim, hiding regressions; update the mock for '#mcp/secrets/allowed-packages.ts' so normalizeAllowedPackages mirrors the real normalizer (trim entries, remove empty strings, deduplicate, and sort) or simply import and re-export the real normalizeAllowedPackages implementation for use in these tests; ensure test inputs include whitespace and duplicate package names to validate trimming/deduping/sorting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/app/handlers/connect-secret.node.test.ts`:
- Around line 121-123: The test currently mocks normalizeAllowedPackages to
return inputs verbatim, hiding regressions; update the mock for
'#mcp/secrets/allowed-packages.ts' so normalizeAllowedPackages mirrors the real
normalizer (trim entries, remove empty strings, deduplicate, and sort) or simply
import and re-export the real normalizeAllowedPackages implementation for use in
these tests; ensure test inputs include whitespace and duplicate package names
to validate trimming/deduping/sorting behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc63025a-9d1c-4c44-8f6a-6c79b18b4da5
📒 Files selected for processing (2)
packages/worker/src/app/handlers/connect-secret.node.test.tspackages/worker/src/app/handlers/connect-secret.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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
2 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
allowedPackagesparsing, editing, review, and save support to/connect/secret.allowedPackagesarray as a clear operation for connector bindings.Walkthrough
Testing
npx oxfmt --check packages/worker/src/app/handlers/connect-secret.ts packages/worker/client/routes/connect-secret.tsx packages/worker/client/routes/secret-editor-fields.tsx packages/worker/client/routes/secret-normalization.ts packages/worker/src/mcp/generated-ui-api.ts packages/worker/src/app/handlers/connect-secret.node.test.ts e2e/connect-secret.spec.ts docs/guides/connect-secret.mdnpx vitest run packages/worker/src/app/handlers/connect-secret.node.test.tsnpm run typechecknpm run test:e2e:run -- connect-secret.spec.tsnpm run lintnpx oxfmt --check packages/worker/src/app/handlers/connect-secret.ts packages/worker/src/app/handlers/connect-secret.node.test.tsnpx oxfmt --check packages/worker/src/app/handlers/connect-secret.node.test.tsSummary by CodeRabbit
New Features
Documentation
API
Tests