Improve Intent core loading for monorepos and agent adapters#124
Open
LadyBluenotes wants to merge 13 commits intomainfrom
Open
Improve Intent core loading for monorepos and agent adapters#124LadyBluenotes wants to merge 13 commits intomainfrom
LadyBluenotes wants to merge 13 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR extracts intent listing/loading into a new core module, adds fast-path skill resolution, exclude-pattern handling, markdown destination rewriting, debug output, CLI flags (--debug, --exclude), and updates CLI commands, tests, docs, and a benchmark to use the new core APIs. ChangesIntent Core & CLI Integration
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (load command)
participant Core as Intent Core
participant Parser as parseSkillUse()
participant Excludes as Exclude Checker
participant FastPath as Fast-Path Resolver
participant Scanner as Scanner (full-scan)
participant Resolver as Resolver
participant FileIO as File I/O / Markdown Rewriter
participant User as User Output
CLI->>Core: loadIntentSkill(use, options)
Core->>Parser: parse use string
Parser-->>Core: SkillUse { packageName, skillName }
Core->>Excludes: getEffectiveExcludePatterns(...)
Excludes-->>Core: patterns
Core->>Excludes: isPackageExcluded(packageName, patterns)
Excludes-->>Core: boolean
alt package excluded
Core->>CLI: throw IntentCoreError('package-excluded')
CLI->>User: error message
else
Core->>FastPath: resolveSkillUseFastPath(parsedUse,...)
alt fast-path result
FastPath-->>Core: ResolveSkillResult
else
Core->>Scanner: scanForIntents(...)
Scanner-->>Core: ScanResult
Core->>Resolver: resolveSkillUse(parsedUse, ScanResult)
Resolver-->>Core: ResolveSkillResult
end
Core->>FileIO: read file at resolved.path
FileIO-->>Core: raw content
Core->>FileIO: rewriteLoadedSkillMarkdownDestinations(...)
FileIO-->>Core: rewritten content
Core-->>CLI: LoadedIntentSkill { content, metadata, debug }
CLI->>User: render content/path/json/debug
end
sequenceDiagram
participant CLI as CLI (list command)
participant Core as Intent Core
participant Scanner as Scanner
participant Excludes as Exclude Collector
participant Filter as Package/Warning Filter
participant Formatter as Use Formatter
participant User as CLI Output
CLI->>Core: listIntentSkills(options)
Core->>Scanner: scanForIntents(...)
Scanner-->>Core: ScanResult { packages, warnings, conflicts }
Core->>Excludes: getEffectiveExcludePatterns(...)
Excludes-->>Core: patterns
Core->>Filter: exclude packages/warnings/conflicts by patterns
Filter-->>Core: filtered results
Core->>Formatter: format skill uses and package summaries
Formatter-->>Core: IntentSkillList
alt debug=true
Core->>Core: attach debug metadata
end
Core-->>CLI: IntentSkillList
CLI->>User: render table/json/debug
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
View your CI Pipeline Execution ↗ for commit 2146c51
☁️ Nx Cloud last updated this comment at |
commit: |
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/intent/src/core.ts`:
- Around line 166-187: The package-boundary check is currently lexical and can
be bypassed by a symlink; before calling isPathInsidePackageRoot and existsSync,
resolve symlinks for the target file and package root (e.g., use realpathSync on
resolved.path and resolved.packageRoot) and use those real paths (e.g.,
realResolvedPath, realPackageRoot) when checking with isPathInsidePackageRoot,
when calling existsSync, and when passing skillFilePath to
rewriteLoadedSkillMarkdownDestinations (instead of the original
resolved.path/resolved.packageRoot), so the containment check and file reads
operate on the canonical filesystem locations.
- Around line 111-118: The packages list currently omits a stable package
identifier, causing collisions; update the object built in IntentSkillList (the
mapping inside core.ts that creates packages with name/version/source) to
include the packageRoot (or unique id/root) field so each package retains a
stable identity, and then update packages/intent/src/commands/list.ts
getPackageSkills(...) to index/lookup packages by that packageRoot instead of
the name+version+source tuple so monorepo duplicated installs remain distinct.
In `@packages/intent/src/core/load-resolution.ts`:
- Around line 144-181: The fast-path loader in resolveSkillUseFastPath can
return stale node_modules results in Yarn PnP repos; modify
resolveSkillUseFastPath to detect Yarn PnP (presence of .pnp.cjs or .pnp.js at
process.cwd() or the workspace root) and immediately return null when PnP is
detected so the full PnP-aware scan runs instead; implement this check at the
top of resolveSkillUseFastPath before calling
getLoadFastPathCandidateDirs/scanIntentPackageAtRoot and ensure the early
bail-out preserves the existing behavior when options.globalOnly is true.
In `@packages/intent/src/scanner.ts`:
- Around line 573-584: The retry to probe Yarn PnP is using
packageCountBeforeNodeModules captured too early (before walkWorkspacePackages),
so workspace discoveries can suppress the PnP fallback; change the baseline to
capture the count immediately before the dependency-discovery steps (i.e., just
before calling scanTarget(nodeModules.local) and the dependency scans) or
otherwise compute a baseline specifically for dependency scans, then after
scanTarget(nodeModules.local), walkKnownPackages(), and walkProjectDeps() check
that dependency-specific count hasn't increased and only then call
getPnpApi()/scanPnpPackages(); reference packageCountBeforeNodeModules,
scanTarget(nodeModules.local), walkWorkspacePackages, walkKnownPackages,
walkProjectDeps, getPnpApi, and scanPnpPackages to locate and update the logic.
🪄 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
Run ID: 56ebcea7-25fb-43be-a39b-a3cc20d1067d
📒 Files selected for processing (20)
benchmarks/intent/load.bench.tsdocs/cli/intent-list.mddocs/cli/intent-load.mdpackages/intent/package.jsonpackages/intent/src/cli-support.tspackages/intent/src/cli.tspackages/intent/src/commands/list.tspackages/intent/src/commands/load.tspackages/intent/src/core.tspackages/intent/src/core/excludes.tspackages/intent/src/core/load-resolution.tspackages/intent/src/core/markdown.tspackages/intent/src/core/package-json.tspackages/intent/src/core/types.tspackages/intent/src/resolver.tspackages/intent/src/scanner.tspackages/intent/tests/cli.test.tspackages/intent/tests/core.test.tspackages/intent/tests/resolver.test.tspackages/intent/tests/scanner.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/intent/tests/cli.test.ts (1)
1005-1025: 💤 Low valueTest creates
SKILL.mdas a directory instead of a file.Line 1014 uses
mkdirSyncto createSKILL.mdas a directory. While this tests that--pathdoesn't read the file content (since reading a directory would fail), the test assertion expects the path output to includeSKILL.mdwhich is now a directory path. This works but is confusing and could mask real issues if the resolution logic changes.Consider using a proper file that exists but verifying through other means (like mocking
readFileSync) that content isn't read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/cli.test.ts` around lines 1005 - 1025, The test 'prints a skill path without reading skill content' currently creates SKILL.md as a directory via mkdirSync; change this to create an actual file (use writeFileSync or similar) at node_modules/@tanstack/query/skills/fetching/SKILL.md so the path points to a real file, and keep the assertion on main(['load', '@tanstack/query#fetching', '--path']) and logSpy intact; if you need to assert that file content is not read, add a mock/spy on fs.readFileSync (or the internal file-read helper used by main) to ensure it is not called while still creating the SKILL.md file.packages/intent/src/core/excludes.ts (1)
61-67: 💤 Low valueReDoS risk from user-supplied glob patterns is low but worth noting.
The static analysis flags potential ReDoS. In practice, the risk is mitigated because:
- Patterns come from
package.jsonconfig or CLI flags (trusted sources)- The transformation escapes most metacharacters and only allows
.*wildcardsHowever, patterns like
*****would generate.*.*.*.*.*which could cause backtracking on long package names. Consider adding a simple sanity check or documenting the expected pattern format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/core/excludes.ts` around lines 61 - 67, The globToRegExp function can produce catastrophic regexes from inputs like "*****"; modify globToRegExp to sanity-check and normalize the pattern before building the regex: reject or trim overly long patterns (e.g., max length 200), collapse consecutive '*' into a single '*' (or reject runs longer than, say, 3), and optionally throw a clear error for invalid patterns; implement these checks inside globToRegExp (referencing function name globToRegExp and the local variable source) so only normalized/safe patterns are converted to RegExp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/intent/src/core/excludes.ts`:
- Around line 51-59: getEffectiveExcludePatterns currently calls
getConfigExcludePatterns(process.cwd(), context) which ignores an override
passed in options.cwd; update getEffectiveExcludePatterns to pass options.cwd
when present (e.g., use options?.cwd || process.cwd()) into
getConfigExcludePatterns so config-based excludes are read from the intended
working directory; ensure you reference the getEffectiveExcludePatterns function
and the getConfigExcludePatterns call when making the change and handle the case
where options or options.cwd is undefined.
- Around line 82-91: warningMentionsPackage currently only validates the
character after packageName which allows false positives like
"prefix@tanstack/query"; update the function (warningMentionsPackage) to also
check the character before the found match: iterate through all occurrences
using indexOf(packageName, fromIndex), for each occurrence verify the char
before is either undefined (start of string) or matches /[^a-zA-Z0-9_-]/ and the
char after is undefined or matches /[^a-zA-Z0-9_-]/; return true on the first
occurrence that satisfies both bounds, otherwise continue searching and return
false if none match.
In `@packages/intent/tests/scanner.test.ts`:
- Around line 907-916: The `@tanstack/react-start` test fixture created via
writeJson(join(reactStartDir, 'package.json'), ...) is missing the intent field;
update that writeJson call to include the same intent entry used in the other
PnP fixture (e.g., add an intent property with the pnp value/shape used
elsewhere such as intent: { type: 'pnp' }) so both Yarn PnP test fixtures are
consistent.
---
Nitpick comments:
In `@packages/intent/src/core/excludes.ts`:
- Around line 61-67: The globToRegExp function can produce catastrophic regexes
from inputs like "*****"; modify globToRegExp to sanity-check and normalize the
pattern before building the regex: reject or trim overly long patterns (e.g.,
max length 200), collapse consecutive '*' into a single '*' (or reject runs
longer than, say, 3), and optionally throw a clear error for invalid patterns;
implement these checks inside globToRegExp (referencing function name
globToRegExp and the local variable source) so only normalized/safe patterns are
converted to RegExp.
In `@packages/intent/tests/cli.test.ts`:
- Around line 1005-1025: The test 'prints a skill path without reading skill
content' currently creates SKILL.md as a directory via mkdirSync; change this to
create an actual file (use writeFileSync or similar) at
node_modules/@tanstack/query/skills/fetching/SKILL.md so the path points to a
real file, and keep the assertion on main(['load', '@tanstack/query#fetching',
'--path']) and logSpy intact; if you need to assert that file content is not
read, add a mock/spy on fs.readFileSync (or the internal file-read helper used
by main) to ensure it is not called while still creating the SKILL.md file.
🪄 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
Run ID: baeea595-7b7f-404d-8ef6-721292e8ac8a
📒 Files selected for processing (11)
knip.jsonpackages/intent/src/commands/list.tspackages/intent/src/commands/load.tspackages/intent/src/core.tspackages/intent/src/core/excludes.tspackages/intent/src/core/load-resolution.tspackages/intent/src/core/types.tspackages/intent/src/scanner.tspackages/intent/tests/cli.test.tspackages/intent/tests/core.test.tspackages/intent/tests/scanner.test.ts
✅ Files skipped from review due to trivial changes (1)
- knip.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/intent/src/core/types.ts
- packages/intent/tests/core.test.ts
- packages/intent/src/scanner.ts
- packages/intent/src/commands/load.ts
Comment on lines
+51
to
+59
| export function getEffectiveExcludePatterns( | ||
| options: IntentCoreOptions, | ||
| context?: ProjectContext, | ||
| ): Array<string> { | ||
| return [ | ||
| ...getConfigExcludePatterns(process.cwd(), context), | ||
| ...normalizeExcludePatterns(options.exclude), | ||
| ] | ||
| } |
Contributor
There was a problem hiding this comment.
Hardcoded process.cwd() ignores the options.cwd override.
getEffectiveExcludePatterns always reads config from process.cwd() instead of using options.cwd when provided. This means config-based excludes won't be discovered correctly if the caller specifies a different working directory.
Proposed fix
export function getEffectiveExcludePatterns(
options: IntentCoreOptions,
context?: ProjectContext,
): Array<string> {
+ const cwd = options.cwd ?? process.cwd()
return [
- ...getConfigExcludePatterns(process.cwd(), context),
+ ...getConfigExcludePatterns(cwd, context),
...normalizeExcludePatterns(options.exclude),
]
}📝 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
| export function getEffectiveExcludePatterns( | |
| options: IntentCoreOptions, | |
| context?: ProjectContext, | |
| ): Array<string> { | |
| return [ | |
| ...getConfigExcludePatterns(process.cwd(), context), | |
| ...normalizeExcludePatterns(options.exclude), | |
| ] | |
| } | |
| export function getEffectiveExcludePatterns( | |
| options: IntentCoreOptions, | |
| context?: ProjectContext, | |
| ): Array<string> { | |
| const cwd = options.cwd ?? process.cwd() | |
| return [ | |
| ...getConfigExcludePatterns(cwd, context), | |
| ...normalizeExcludePatterns(options.exclude), | |
| ] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/core/excludes.ts` around lines 51 - 59,
getEffectiveExcludePatterns currently calls
getConfigExcludePatterns(process.cwd(), context) which ignores an override
passed in options.cwd; update getEffectiveExcludePatterns to pass options.cwd
when present (e.g., use options?.cwd || process.cwd()) into
getConfigExcludePatterns so config-based excludes are read from the intended
working directory; ensure you reference the getEffectiveExcludePatterns function
and the getConfigExcludePatterns call when making the change and handle the case
where options or options.cwd is undefined.
Comment on lines
+82
to
+91
| export function warningMentionsPackage( | ||
| warning: string, | ||
| packageName: string, | ||
| ): boolean { | ||
| const idx = warning.indexOf(packageName) | ||
| if (idx === -1) return false | ||
|
|
||
| const after = warning[idx + packageName.length] | ||
| return after === undefined || /[^a-zA-Z0-9_-]/.test(after) | ||
| } |
Contributor
There was a problem hiding this comment.
warningMentionsPackage doesn't check the character before the match.
The function only validates the character after the package name but not before. A warning containing prefix@tanstack/query would incorrectly match when checking for @tanstack/query.
Proposed fix
export function warningMentionsPackage(
warning: string,
packageName: string,
): boolean {
const idx = warning.indexOf(packageName)
if (idx === -1) return false
+ const before = warning[idx - 1]
+ if (before !== undefined && /[a-zA-Z0-9_-]/.test(before)) return false
+
const after = warning[idx + packageName.length]
return after === undefined || /[^a-zA-Z0-9_-]/.test(after)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/core/excludes.ts` around lines 82 - 91,
warningMentionsPackage currently only validates the character after packageName
which allows false positives like "prefix@tanstack/query"; update the function
(warningMentionsPackage) to also check the character before the found match:
iterate through all occurrences using indexOf(packageName, fromIndex), for each
occurrence verify the char before is either undefined (start of string) or
matches /[^a-zA-Z0-9_-]/ and the char after is undefined or matches
/[^a-zA-Z0-9_-]/; return true on the first occurrence that satisfies both
bounds, otherwise continue searching and return false if none match.
Comment on lines
+907
to
+916
| writeJson(join(reactStartDir, 'package.json'), { | ||
| name: '@tanstack/react-start', | ||
| version: '1.167.52', | ||
| repository: { | ||
| type: 'git', | ||
| url: 'git+https://github.com/TanStack/router.git', | ||
| directory: 'packages/react-start', | ||
| }, | ||
| homepage: 'https://tanstack.com/start', | ||
| }) |
Contributor
There was a problem hiding this comment.
Same missing intent field as the first PnP test
The @tanstack/react-start fixture written at lines 907–916 omits the intent field for the same reason flagged above. Apply the same fix to keep both Yarn PnP test fixtures consistent.
🛠️ Proposed fix
writeJson(join(reactStartDir, 'package.json'), {
name: '@tanstack/react-start',
version: '1.167.52',
+ intent: {
+ version: 1,
+ repo: 'TanStack/router',
+ docs: 'https://tanstack.com/start',
+ },
repository: {
type: 'git',
url: 'git+https://github.com/TanStack/router.git',
directory: 'packages/react-start',
},
homepage: 'https://tanstack.com/start',
})📝 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
| writeJson(join(reactStartDir, 'package.json'), { | |
| name: '@tanstack/react-start', | |
| version: '1.167.52', | |
| repository: { | |
| type: 'git', | |
| url: 'git+https://github.com/TanStack/router.git', | |
| directory: 'packages/react-start', | |
| }, | |
| homepage: 'https://tanstack.com/start', | |
| }) | |
| writeJson(join(reactStartDir, 'package.json'), { | |
| name: '@tanstack/react-start', | |
| version: '1.167.52', | |
| intent: { | |
| version: 1, | |
| repo: 'TanStack/router', | |
| docs: 'https://tanstack.com/start', | |
| }, | |
| repository: { | |
| type: 'git', | |
| url: 'git+https://github.com/TanStack/router.git', | |
| directory: 'packages/react-start', | |
| }, | |
| homepage: 'https://tanstack.com/start', | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/tests/scanner.test.ts` around lines 907 - 916, The
`@tanstack/react-start` test fixture created via writeJson(join(reactStartDir,
'package.json'), ...) is missing the intent field; update that writeJson call to
include the same intent entry used in the other PnP fixture (e.g., add an intent
property with the pnp value/shape used elsewhere such as intent: { type: 'pnp'
}) so both Yarn PnP test fixtures are consistent.
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
1 participant
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
node_moduleshides valid PnP packages@tanstack/intent/coreAPIs for list/load consumerslistandloadthrough the shared core behaviorloadresolution for workspace packages, root deps, pnpm isolated workspace deps, and full-scan fallback@tanstack/router-core#auth-and-guards--debugoutput forlistandloadwithout changing stdoutSummary by CodeRabbit
New Features
Documentation