Repository navigation
Scope repo entity-source lookups by user in the query - #925
Merged
Merged
Conversation
Review follow-up. resolveRepoSourceReference and repo_show_publish_note loaded a source by id and then compared source.user_id in app code. Use the existing getEntitySourceByIdForUser helper so the user predicate is part of the query, matching what #922 did for resolveOwnedPackageSource. Behaviour is unchanged; another user's source now simply is not returned rather than being returned and rejected. Also covers the missing-identity branch of resolveRepoSourceReference, which the previous commit converted without a test.
📝 WalkthroughWalkthroughRepository capabilities now retrieve entity sources through the caller-scoped ChangesRepository source ownership
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
Contributor
|
🔎 Preview deployed: https://kody-pr-925.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: 1
🤖 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/repo/repo-show-publish-note.node.test.ts`:
- Around line 140-143: Update the assertion for getEntitySourceByIdForUser in
the isolation test to verify the complete lookup input, including the caller’s
expected userId alongside source-1. Preserve the existing expectation that the
first argument is accepted and ensure the assertion fails when userId is omitted
or incorrect.
🪄 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: 8342ebbb-c899-4bc1-886a-88bb3b0d6b9e
📒 Files selected for processing (6)
packages/worker/src/mcp/capabilities/repo/repo-open-session.node.test.tspackages/worker/src/mcp/capabilities/repo/repo-resolve-target.node.test.tspackages/worker/src/mcp/capabilities/repo/repo-resolve-target.tspackages/worker/src/mcp/capabilities/repo/repo-show-publish-note.node.test.tspackages/worker/src/mcp/capabilities/repo/repo-show-publish-note.tspackages/worker/src/mcp/capabilities/repo/repo-workflow.node.test.ts
Comment on lines
+140
to
+143
| expect(mockModule.getEntitySourceByIdForUser).toHaveBeenCalledWith( | ||
| expect.anything(), | ||
| expect.objectContaining({ id: 'source-1' }), | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert the caller’s userId in this isolation test.
The test only verifies id, so it would still pass if the lookup received an omitted or incorrect userId. Assert the complete scoped lookup contract.
As per coding guidelines, every Kody multi-user read path must be scoped by userId.
Proposed test fix
expect(mockModule.getEntitySourceByIdForUser).toHaveBeenCalledWith(
expect.anything(),
- expect.objectContaining({ id: 'source-1' }),
+ { id: 'source-1', userId: 'user-1' },
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(mockModule.getEntitySourceByIdForUser).toHaveBeenCalledWith( | |
| expect.anything(), | |
| expect.objectContaining({ id: 'source-1' }), | |
| ) | |
| expect(mockModule.getEntitySourceByIdForUser).toHaveBeenCalledWith( | |
| expect.anything(), | |
| { id: 'source-1', userId: 'user-1' }, | |
| ) |
🤖 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-show-publish-note.node.test.ts`
around lines 140 - 143, Update the assertion for getEntitySourceByIdForUser in
the isolation test to verify the complete lookup input, including the caller’s
expected userId alongside source-1. Preserve the existing expectation that the
first argument is accepted and ensure the assertion fails when userId is omitted
or incorrect.
Source: Coding guidelines
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
Review follow-up to #923, which merged before I could fold this in. CodeRabbit raised a valid point on that PR that I agree with.
resolveRepoSourceReferenceandrepo_show_publish_noteloaded an entity source by id and then comparedsource.user_idin application code:AGENTS.mdasks for every read path to be scoped byuserId, and the repo already hasgetEntitySourceByIdForUserfor exactly this. #922 applied the same change toresolveOwnedPackageSource; these two were the remaining instances on caller-supplied ids.Behaviour is unchanged — another user's source was already rejected. The difference is that it is now filtered by the query rather than fetched and then discarded, so there is no window where a row belonging to someone else is in hand.
Also adds coverage for the missing-identity branch of
resolveRepoSourceReference(Repo source identity is required.), which #923 converted toMcpCallerErrorwithout a test — CodeRabbit's nitpick on the same review, and a fair one.Scope note
getEntitySourceByIdremains in use elsewhere (repo-session-do.ts,jobs/service.ts,package-runtime/**, and others). Those resolve ids from our own records rather than from caller input, and converting them is a broader change than this review point warrants. Left alone deliberately.Validation
npm run validategreen in full:format:check,lint(1 pre-existing warning insentry-tunnel.node.test.ts, 0 errors),typecheck, 1295 unit tests across 400 files, 18 Playwright E2E, 2 MCP E2E,backup:build,primitives:check,migrations:check.The
repo_show_publish_notecross-user test now asserts the lookup returns nothing for the caller, rather than returning another user's row for the handler to reject — which is the actual contract after this change.System recap — composes existing primitives (low risk)
Mode: recap · Base:
main@28452280· Head:7085f365Classification: composes — swaps two call sites onto an existing user-scoped data-access helper. No primitive changes shape or behaviour; no schema change.
Primitives touched
capability-registryd1-app-dbuser_idpredicate moves into theentity_sourcesquerySystem map
Repo capabilities resolve a source through the user-scoped helper, so ownership is enforced by the query instead of a post-read comparison.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Strengthens per-user isolation:
entity_sourcesreads on caller-supplied ids now carry theuserIdpredicate in SQL, so a row belonging to another user is never returned to application code on these paths.Summary by CodeRabbit