Distinguish npm and workspace sources with the same package name#191
Conversation
- Updated `isSourcePermitted` to accept an optional `packageKind` parameter, allowing for more granular source validation based on package type. - Modified the logic in `applySourcePolicy` to pass the package kind when checking source permissions. - Improved the discovery check for sources in explicit mode by utilizing a `sourceIdentityKey` function to create unique identifiers based on both package kind and name. - Added a comment in `excludes.ts` to clarify the intentional design choice regarding package exclusion handling.
📝 WalkthroughWalkthroughThis PR tightens skill-source authorization in the intent package to use a ChangesKind-aware skill source identity
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
|
View your CI Pipeline Execution ↗ for commit 9dd06a7
☁️ Nx Cloud last updated this comment at |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/intent/tests/core.test.ts (1)
905-1010: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid coverage of the kind-mismatch late gate; consider adding a same-name collision test.
The four new cases correctly cover fast-path rejection, full-scan rejection, positive workspace authorization, and exclusion-before-resolution precedence.
One gap worth adding: a case where an npm-installed copy and a workspace member of the same name coexist in the same project (e.g., a workspace package also reachable under
node_modules), withintent.skills: ['workspace:<name>']. This directly exercises the scenario this PR is meant to close and would also validate thedroppedNames/discovery-notice assumptions insource-policy.tsunder a real collision (see companion comment there).🤖 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/intent/tests/core.test.ts` around lines 905 - 1010, The current tests cover the workspace-only and exclusion paths, but they do not exercise a same-name collision between an npm-installed package and a workspace member. Add a new `loadIntentSkill` test alongside the existing `kind-mismatch late gate` cases that creates both a workspace package and an installed `node_modules` copy with the same package name, sets `intent.skills` to `workspace:<name>`, and asserts the workspace authorization wins while the npm-installed copy is rejected/ignored as intended. Use the existing `loadIntentSkill`, `writeInstalledIntentPackage`, and workspace setup helpers in `core.test.ts` to verify the collision 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.
Nitpick comments:
In `@packages/intent/tests/core.test.ts`:
- Around line 905-1010: The current tests cover the workspace-only and exclusion
paths, but they do not exercise a same-name collision between an npm-installed
package and a workspace member. Add a new `loadIntentSkill` test alongside the
existing `kind-mismatch late gate` cases that creates both a workspace package
and an installed `node_modules` copy with the same package name, sets
`intent.skills` to `workspace:<name>`, and asserts the workspace authorization
wins while the npm-installed copy is rejected/ignored as intended. Use the
existing `loadIntentSkill`, `writeInstalledIntentPackage`, and workspace setup
helpers in `core.test.ts` to verify the collision behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c9a6dfc-9a1e-4f0c-8b54-815f278e873b
📒 Files selected for processing (9)
.changeset/tidy-buses-hunt.mdpackages/intent/src/core/excludes.tspackages/intent/src/core/intent-core.tspackages/intent/src/core/load-resolution.tspackages/intent/src/core/source-policy.tspackages/intent/src/core/types.tspackages/intent/tests/core-types.test.tspackages/intent/tests/core.test.tspackages/intent/tests/source-policy.test.ts
1 participant
Summary
intent.skillsentries and the discovery scanner both compared sources by package name alone, soworkspace:fooandnpm:foowere treated as the same source. A project could unintentionally authorize the wrongfoo— e.g. an npm-installed dependency slipping through an allowlist entry meant only for a local workspace package of the same name.This tightens matching to
(kind, id)everywhere a source's identity is checked:1. Allowlist matching
isSourcePermittednow takes the discovered package's kind and requires it to match the declared source's kind, not just the name. The "declared but not discovered" notice is keyed the same way.2. Load-path late gate
intent loadresolves a skill before the scanner has told it kind — a name-only pre-check stays for the fast, no-I/O rejection path. Once resolution reveals the real kind (fast path or full scan), a second gate re-checks it and refuses with the samepackage-not-listederror either way, so the two resolution paths can't disagree on the same input.3. Excludes stay name-only (deliberate)
intent.skills.excludeintentionally continues to match by name regardless of kind — an exclude is a blanket "don't load skills from anything named X," not tied to a specific source. Documented in place, no behavior change.Impact / compatibility
workspace:foocurrently authorizing an npm-discoveredfoo(or vice versa) — that combination now requires its own matching allowlist entry.Summary by CodeRabbit
intent loadnow rejects mismatched packages more consistently, including after fallback resolution.