Repository navigation
Add package invocation URL guardrails - #568
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds owner-scoped package invocation URL builders and descriptors, threads invocation metadata through package lookup and search outputs, updates unscoped invocation HTTP handling, and revises the token-setup guide and related wording. ChangesOwner-scoped package invocation flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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
June 26, 2026 13:41
Contributor
|
🔎 Preview deployed: https://kody-pr-568.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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/guides/account-package-invocation-token-setup.md (1)
148-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the
invalid_tokenvsowner_slug_requiredboundary.With the new handler, a request missing the owner slug returns
owner_slug_required(404), notinvalid_token, and a present-but-wrong slug returnsnot_found. So by the time a caller actually receivesinvalid_token, the route shape and slug presence are already guaranteed correct. The guidance to "first verify the endpoint includes the@:usernameowner slug" on aninvalid_tokenresponse is therefore somewhat redundant and could re-introduce the diagnostic confusion this PR is trying to remove. Consider reframing so thatinvalid_tokenpoints directly at token rotation/secret correctness, while slug/route-shape checks are tied to theowner_slug_required/not_foundresponses.🤖 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 `@docs/guides/account-package-invocation-token-setup.md` around lines 148 - 156, The guidance around `invalid_token` in the token setup doc is mixing up route/slug validation with auth failure handling. Update the section in the package invocation token setup guide so `invalid_token` points directly to token rotation or external secret correctness, and move the owner slug / route-shape checks to the `owner_slug_required` and `not_found` cases. Keep the route examples aligned with the `@:username` pattern and make the troubleshooting flow reflect the new handler behavior.packages/worker/src/package-invocations/public-url.ts (1)
43-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant re-normalization of the export name.
routeExportNameis already the normalized route segment, but it is then passed asexportNametobuildPackageInvocationUrl/buildPackageInvocationPath, which internally callbuildPackageInvocationRouteExportName(and thusnormalizePackageInvocationExportName) again. It is idempotent so the output is correct, but passing an already-route-form value into helpers that expect a raw export name is easy to misread. Consider passingnormalizedExportName(the raw-but-normalized form) to make the contract clearer, or document the intent.🤖 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/package-invocations/public-url.ts` around lines 43 - 60, The export name is being normalized twice in the public URL/path assembly, which makes the contract of the helpers unclear. In the package-invocations/public-url logic, use the raw normalized value from normalizePackageInvocationExportName when calling buildPackageInvocationUrl and buildPackageInvocationPath, since those helpers already invoke buildPackageInvocationRouteExportName internally. Keep routeExportName only if you need the final route segment locally, otherwise pass normalizedExportName to match the expected input shape and avoid the misleading re-normalization.
🤖 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/shared/src/public-urls.ts`:
- Around line 42-50: The package invocation path builder is using a root-export
marker that becomes a single dot segment and gets normalized away by URL
parsers, breaking matching for root exports. Update buildPackageInvocationPath
to use a URL-safe sentinel for the root export and adjust
parsePackageInvocationPath in the worker HTTP handler to translate that sentinel
back to the root export. Keep the change consistent with
buildPackageInvocationRouteExportName so both path construction and parsing
agree on the same symbol.
In `@packages/worker/src/mcp/capabilities/packages/get-package.ts`:
- Around line 45-54: `get-package.ts` currently throws when
`resolvePublicUsername` returns null, which makes `package_get` fail entirely
instead of returning the rest of the package data. Update the `package_get` flow
to match `search-format.ts` by treating the external invocation as optional:
keep building metadata/exports/imports, and set `external_invocation` to null or
omit it when no username can be resolved. Also update the `shared.ts` schema for
`external_invocation` so it is nullable/optional, and ensure any code using
`buildExternalPackageInvocationDescriptor` or the `get-package` response handles
the missing value safely.
---
Nitpick comments:
In `@docs/guides/account-package-invocation-token-setup.md`:
- Around line 148-156: The guidance around `invalid_token` in the token setup
doc is mixing up route/slug validation with auth failure handling. Update the
section in the package invocation token setup guide so `invalid_token` points
directly to token rotation or external secret correctness, and move the owner
slug / route-shape checks to the `owner_slug_required` and `not_found` cases.
Keep the route examples aligned with the `@:username` pattern and make the
troubleshooting flow reflect the new handler behavior.
In `@packages/worker/src/package-invocations/public-url.ts`:
- Around line 43-60: The export name is being normalized twice in the public
URL/path assembly, which makes the contract of the helpers unclear. In the
package-invocations/public-url logic, use the raw normalized value from
normalizePackageInvocationExportName when calling buildPackageInvocationUrl and
buildPackageInvocationPath, since those helpers already invoke
buildPackageInvocationRouteExportName internally. Keep routeExportName only if
you need the final route segment locally, otherwise pass normalizedExportName to
match the expected input shape and avoid the misleading re-normalization.
🪄 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: a4af2c96-2676-4c70-9dd7-14bb3530e237
📒 Files selected for processing (14)
docs/guides/account-package-invocation-token-setup.mdpackages/shared/src/public-urls.tspackages/worker/src/mcp/capabilities/coding/kody-official-guide.tspackages/worker/src/mcp/capabilities/packages/get-package.node.test.tspackages/worker/src/mcp/capabilities/packages/get-package.tspackages/worker/src/mcp/capabilities/packages/shared.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-invocations/http.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/public-url.node.test.tspackages/worker/src/package-invocations/public-url.tspackages/worker/src/package-invocations/service.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
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.
Walkthrough
package_invocation_token_setupguide with the owner-scoped invocation route shape, the Kent YouTube WebSub Worker example, and troubleshooting guidance that checks the owner slug before rotating tokens.package_getand package entity search details.POST /api/package-invocations/...requests to a structuredowner_slug_required404 before OAuth token handling.invalid_token; future agents should use the canonical URL helper/metadata instead of manually constructing package invocation routes.Testing
npx vitest run packages/worker/src/package-invocations/http.workers.test.ts packages/worker/src/package-invocations/public-url.node.test.ts packages/worker/src/mcp/capabilities/packages/get-package.node.test.ts packages/worker/src/mcp/tools/search-format.node.test.tsnpm run format:checknpm run lint(passes; reports existing warnings in unrelated test files)npm run typecheckSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests