Enforce user-scoped Kody package names - #454
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 (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThreads an optional expectedPackageScope (derived from the MCP user's DB row) through manifest parsing, repo checks, repo-session RPCs, and publish flows, and adds manifest scope validation plus updated tests/mocks asserting the scoped behavior. ChangesPackage scope validation and threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
May 12, 2026 19:22
Contributor
|
🔎 Preview deployed: https://kody-pr-454.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/src/mcp/capabilities/packages/publish-external-push.node.test.ts (1)
158-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an assertion for
expectedPackageScopein publish RPC call.The new behavior depends on forwarding scope, but this assertion block doesn’t verify it. Adding it will protect against regression of the enforcement wiring.
Test assertion tweak
expect(mockModule.publishFromExternalRef).toHaveBeenCalledWith( expect.objectContaining({ sourceId: 'source-1', userId: 'user-1', newCommit: 'commit-new', expectedHead: 'commit-new', allowForce: false, rebuildPackageArtifacts: false, + expectedPackageScope: 'user', }), )🤖 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/packages/publish-external-push.node.test.ts` around lines 158 - 167, Update the test assertion for mockModule.publishFromExternalRef to also verify the forwarded package scope by adding expectedPackageScope to the expect.objectContaining payload; inside the expect(...) for publishFromExternalRef include expectedPackageScope: <the test's forwarded scope value or the variable expectedPackageScope> so the RPC call check covers scope forwarding as well.packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts (1)
83-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScoped manifest validation is skipped for
repo_run_commandscheck runs.At Line 83,
sessionRpc.runCommandsruns checks withoutexpectedPackageScope, while Line 107 only applies scope duringpublishSession. This meansrun_checks: true+publish: falsecan report passing checks without username-scope validation.Suggested direction
+ const expectedPackageScope = + validatedSession.entity_type === 'package' + ? await getMcpUserPackageScope(ctx.env.APP_DB, user) + : undefined const result = await sessionRpc.runCommands({ sessionId: validatedSession.id, userId: user.userId, commands: args.commands, dryRun: args.dry_run, runChecks: args.run_checks, publish: false, + expectedPackageScope, }) // ... publish = await sessionRpc.publishSession({ sessionId: validatedSession.id, userId: user.userId, rebuildPackageArtifacts: false, - expectedPackageScope: - validatedSession.entity_type === 'package' - ? await getMcpUserPackageScope(ctx.env.APP_DB, user) - : undefined, + expectedPackageScope, })And in
RepoSessionBase.runCommands, threadexpectedPackageScopeintothis.runChecks(...)when checks are requested.Also applies to: 107-110
🤖 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/repo/repo-run-commands.ts` around lines 83 - 90, The runCommands call in repo-run-commands.ts currently invokes sessionRpc.runCommands({..., runChecks: args.run_checks, publish: false }) without passing expectedPackageScope, allowing check runs to skip username-scoped manifest validation; update the call site to include expectedPackageScope (the same scope used by publishSession) when args.run_checks is true, and update RepoSessionBase.runCommands / RepoSessionBase.runChecks to accept and thread an expectedPackageScope parameter into this.runChecks(...) so scoped manifest validation is enforced during non-publish check runs as well.
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/packages/save-package.ts (1)
12-17: ⚡ Quick winPrefer a single validation path via
parseAuthoredPackageJson({ expectedPackageScope }).You can remove the extra post-parse assertion and let parse enforce scope directly to avoid duplicated validation paths.
Suggested refactor
-import { - assertAuthoredPackageJsonNameScope, - parseAuthoredPackageJson, -} from '#worker/package-registry/manifest.ts' +import { parseAuthoredPackageJson } from '#worker/package-registry/manifest.ts' import { getMcpUserPackageScope } from '#worker/package-registry/user-scope.ts' ... - const manifest = parseAuthoredPackageJson({ + const expectedPackageScope = await getMcpUserPackageScope( + ctx.env.APP_DB, + user, + ) + const manifest = parseAuthoredPackageJson({ content: packageJsonContent, manifestPath: 'package.json', + expectedPackageScope, }) - assertAuthoredPackageJsonNameScope({ - manifest, - expectedPackageScope: await getMcpUserPackageScope( - ctx.env.APP_DB, - user, - ), - manifestPath: 'package.json', - })Also applies to: 79-90
🤖 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/packages/save-package.ts` around lines 12 - 17, Replace the two-step scope validation (calling parseAuthoredPackageJson then assertAuthoredPackageJsonNameScope) with a single call to parseAuthoredPackageJson that enforces scope by passing expectedPackageScope (computed via getMcpUserPackageScope) as an argument; remove usages of assertAuthoredPackageJsonNameScope in save-package.ts and update both occurrences (the initial parse block and the second block around lines 79-90) so parseAuthoredPackageJson({ expectedPackageScope }) performs the scope check and returns the parsed manifest used by buildSavedPackageEmbedText.
🤖 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.
Outside diff comments:
In
`@packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts`:
- Around line 158-167: Update the test assertion for
mockModule.publishFromExternalRef to also verify the forwarded package scope by
adding expectedPackageScope to the expect.objectContaining payload; inside the
expect(...) for publishFromExternalRef include expectedPackageScope: <the test's
forwarded scope value or the variable expectedPackageScope> so the RPC call
check covers scope forwarding as well.
In `@packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts`:
- Around line 83-90: The runCommands call in repo-run-commands.ts currently
invokes sessionRpc.runCommands({..., runChecks: args.run_checks, publish: false
}) without passing expectedPackageScope, allowing check runs to skip
username-scoped manifest validation; update the call site to include
expectedPackageScope (the same scope used by publishSession) when
args.run_checks is true, and update RepoSessionBase.runCommands /
RepoSessionBase.runChecks to accept and thread an expectedPackageScope parameter
into this.runChecks(...) so scoped manifest validation is enforced during
non-publish check runs as well.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/packages/save-package.ts`:
- Around line 12-17: Replace the two-step scope validation (calling
parseAuthoredPackageJson then assertAuthoredPackageJsonNameScope) with a single
call to parseAuthoredPackageJson that enforces scope by passing
expectedPackageScope (computed via getMcpUserPackageScope) as an argument;
remove usages of assertAuthoredPackageJsonNameScope in save-package.ts and
update both occurrences (the initial parse block and the second block around
lines 79-90) so parseAuthoredPackageJson({ expectedPackageScope }) performs the
scope check and returns the parsed manifest used by buildSavedPackageEmbedText.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d65a6d3-e69f-4a37-b24e-96310900c54f
📒 Files selected for processing (15)
packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.tspackages/worker/src/mcp/capabilities/packages/save-package.tspackages/worker/src/mcp/capabilities/repo/repo-publish-session.tspackages/worker/src/mcp/capabilities/repo/repo-run-checks.tspackages/worker/src/mcp/capabilities/repo/repo-run-commands.tspackages/worker/src/mcp/capabilities/repo/repo-workflow.node.test.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/package-registry/manifest.node.test.tspackages/worker/src/package-registry/manifest.tspackages/worker/src/package-registry/user-scope.tspackages/worker/src/repo/checks.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/repo-session-rpc.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 7ef2283. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Jul 20, 2026
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
package_save, repo session checks/publish, repo command checks/publishes, and external package publish flows.Testing
npx vitest run --project node-unit "packages/worker/src/package-registry/manifest.node.test.ts"npx vitest run --project workers-unit "packages/worker/src/mcp/observability.workers.test.ts"npx vitest run --project node-unit "packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts" "packages/worker/src/mcp/capabilities/repo/repo-workflow.node.test.ts"npm run typechecknpm run validateSummary by CodeRabbit
New Features
Bug Fixes