feat: add compact skill resolution for install mappings#115
Open
LadyBluenotes wants to merge 32 commits intomainfrom
Open
feat: add compact skill resolution for install mappings#115LadyBluenotes wants to merge 32 commits intomainfrom
LadyBluenotes wants to merge 32 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as intent load CLI
participant Scanner
participant Resolver
participant Filesystem
participant Agent
User->>CLI: npx `@tanstack/intent` load `@tanstack/query`#fetching
CLI->>CLI: Parse & validate use="@tanstack/query#fetching"
CLI->>Scanner: scanIntentsOrFail(scanOptions)
Scanner->>Filesystem: Scan local/global packages
Scanner-->>CLI: ScanResult with packages & skills
CLI->>Resolver: resolveSkillUse(use, scanResult)
Resolver->>Resolver: Find matching package (prefer local)
Resolver->>Resolver: Find matching skill in package
Resolver->>Filesystem: Construct resolved path
Resolver-->>CLI: ResolveSkillResult
CLI->>Filesystem: Read SKILL.md content
Filesystem-->>CLI: skill file content
CLI->>Agent: Print skill content to stdout
Agent->>Agent: Load skill into context
sequenceDiagram
actor User
participant CLI as intent install CLI
participant Scanner
participant BlockWriter
participant Filesystem
participant Verifier
User->>CLI: npx `@tanstack/intent` install --map
CLI->>Scanner: scanIntentsOrFail(scanOptions)
Scanner->>Filesystem: Scan packages & skills
Scanner-->>CLI: ScanResult
CLI->>BlockWriter: buildIntentSkillsBlock(scanResult)
BlockWriter->>BlockWriter: Sort packages & skills
BlockWriter->>BlockWriter: Generate YAML with when/use
BlockWriter-->>CLI: IntentSkillsBlockResult
CLI->>BlockWriter: resolveIntentSkillsBlockTargetPath(root, mappingCount)
BlockWriter->>Filesystem: Check for existing managed blocks
BlockWriter-->>CLI: target file path (or null)
CLI->>BlockWriter: writeIntentSkillsBlock({block, root})
BlockWriter->>Filesystem: Replace/create managed block
Filesystem-->>BlockWriter: write result
CLI->>Verifier: verifyIntentSkillsBlockFile({expectedBlock})
Verifier->>Filesystem: Read written file
Verifier->>Verifier: Validate markers, YAML, skill identifiers
Verifier-->>CLI: verification success/failure
CLI->>User: Print confirmation or errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 |
|
View your CI Pipeline Execution ↗ for commit f8523d2
☁️ Nx Cloud last updated this comment at |
commit: |
2 tasks
![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
🧹 Nitpick comments (10)
packages/intent/src/display.ts (1)
39-64: Minor: hint loses package context whenpackageNameis undefined.When
opts.packageNameis omitted (e.g., any caller that hasn't been updated),formatRuntimeSkillLookupHintrenders onlyskill "…"with no package qualifier, which makes thenpx … list --jsoninstruction ambiguous across packages. All in-tree callers (intent-library.ts, per summarycommands/list.ts) now pass it, so this is a forward-looking concern — consider makingpackageNamerequired onprintSkillTree/printSkillLineto prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/display.ts` around lines 39 - 64, printSkillLine can emit an ambiguous lookup hint when opts.packageName is undefined because formatRuntimeSkillLookupHint then omits the package qualifier; make packageName required to avoid regressions by updating the function signature of printSkillLine (and its caller printSkillTree) to require opts.packageName, adjust any call sites to pass through the packageName (e.g., places in intent-library.ts and commands/list.ts), and ensure formatRuntimeSkillLookupHint is always called with a defined packageName and skillName so the runtime lookup hint includes the package context.packages/intent/src/skill-use.ts (1)
29-42: Minor:formatSkillUsedoesn't reject embedded#.
formatSkillUse('a#b', 'c')produces"a#b#c", whichparseSkillUsewill then split aspackageName='a',skillName='b#c'— a silent round-trip mismatch. In practice npm package names can't contain#, so this is unreachable from scanner input; consider adding a guard (or a brief comment) ifformatSkillUseis ever exposed to user-supplied values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/skill-use.ts` around lines 29 - 42, formatSkillUse currently allows packageName or skillName to contain '#' leading to ambiguous outputs (e.g. "a#b#c"); update formatSkillUse to validate that neither packageName nor skillName includes the '#' character and throw a SkillUseParseError (similar to the existing 'empty-package'/'empty-skill' cases) when found, so the function rejects embedded separators and keeps round-trip consistency with parseSkillUse; reference the formatSkillUse function and reuse SkillUseParseError for the new 'invalid-package'/'invalid-skill' or a single 'invalid-skill-use' error code as appropriate.packages/intent/src/skill-paths.ts (1)
1-4: Nit: import order (ESLintimport/order).Static analysis flags the type import of
./types.jsas coming before the value import of./utils.js. Swap to appease the linter.Proposed fix
import { existsSync } from 'node:fs' import { join, relative } from 'node:path' -import type { SkillEntry } from './types.js' import { toPosixPath } from './utils.js' +import type { SkillEntry } from './types.js'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/skill-paths.ts` around lines 1 - 4, The import order is reversed causing an ESLint import/order nit: move the type-only import "SkillEntry" from './types.js' so it comes after the value import "toPosixPath" from './utils.js' (i.e., import toPosixPath and other value imports first, then import type { SkillEntry }); keep the same import specifiers and modules, just swap their positions to satisfy the linter.packages/intent/src/resolver.ts (1)
77-77:availablePackagesmay contain duplicates.When the same package name appears in both local and global scan variants,
scanResult.packages.map(...)will list it twice in the error message. Consider deduplicating viaArray.from(new Set(...))for cleaner error output.♻️ Proposed tweak
- availablePackages: scanResult.packages.map((candidate) => candidate.name), + availablePackages: Array.from( + new Set(scanResult.packages.map((candidate) => candidate.name)), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/resolver.ts` at line 77, availablePackages is built from scanResult.packages.map(candidate => candidate.name) and can contain duplicate package names (e.g., same name in local and global scans); change the construction of availablePackages to deduplicate names (for example by using a Set) so the error message lists each package once—update the expression that builds availablePackages (currently using scanResult.packages.map(...)) to convert the names into a unique list before assigning.packages/intent/src/cli-output.ts (1)
1-8: Consider emitting warnings tostderrinstead ofstdout.Human-facing warnings are conventionally written to stderr so they don't pollute pipelines or structured (e.g.,
--json) output on stdout. If any command usingprintWarningsalso emits machine-readable data to stdout, mixing the streams can break consumers that parse the output.♻️ Proposed tweak
-export function printWarnings(warnings: Array<string>): void { - if (warnings.length === 0) return - - console.log('Warnings:') - for (const warning of warnings) { - console.log(` ⚠ ${warning}`) - } -} +export function printWarnings(warnings: Array<string>): void { + if (warnings.length === 0) return + + console.error('Warnings:') + for (const warning of warnings) { + console.error(` ⚠ ${warning}`) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/cli-output.ts` around lines 1 - 8, The printWarnings function currently writes human-facing warnings to stdout; change it to emit to stderr so machine-readable stdout isn't contaminated. In the printWarnings function (symbol: printWarnings), keep the same empty-array early return but replace the console.log calls with stderr output (e.g., console.error or process.stderr.write) for the header and each warning line so all warning text goes to stderr while preserving formatting and behavior.packages/intent/README.md (1)
127-127: Table entry forlistomits the new--global/--global-onlyflags.Lines 51–55 document
list --global, but the CLI commands table only showslist [--json]. Consider updating the table signature for completeness and discoverability.♻️ Proposed tweak
-| `npx `@tanstack/intent`@latest list [--json]` | Discover local intent-enabled packages | +| `npx `@tanstack/intent`@latest list [--json] [--global\|--global-only]` | Discover local (and optionally global) intent-enabled packages |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/README.md` at line 127, Update the CLI commands table entry for the "list" command in packages/intent/README.md so it includes the new --global and --global-only flags; specifically change the table row that currently shows `npx `@tanstack/intent`@latest list [--json]` to include `--global` and `--global-only` (e.g., `list [--json] [--global|--global-only]`) and ensure the short description still reads "Discover local intent-enabled packages" or is adjusted to reflect global discovery when flags are present; update any adjacent explanatory text if necessary to match the documented behavior in lines describing `list --global`.packages/intent/tests/library-scanner.test.ts (1)
189-197: Heads up:symlinkSyncmay fail on Windows CI without Developer Mode or elevated privileges.If this suite runs on Windows, the symlink creation here will throw
EPERM. Consider guarding the test withit.skipIf(process.platform === 'win32')or an equivalent check so non-Windows coverage still runs while avoiding false failures on Windows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/library-scanner.test.ts` around lines 189 - 197, The symlink creation in the test using symlinkSync can fail on Windows CI (EPERM); update the test to skip on Windows by wrapping the test or the symlink block with a platform guard such as it.skipIf(process.platform === 'win32') or if (process.platform === 'win32') { return this.skip?.(); } so the symlinkSync calls (the lines invoking symlinkSync for queryStoreDir and routerStoreDir) are not executed on Windows; ensure the skip is applied at the test level (the describe/it containing these symlinkSync calls) so other assertions still run on non-Windows platforms.docs/getting-started/quick-start-consumers.md (1)
95-98: Trim trailing blank lines at EOF.There are multiple trailing empty lines at the end of the file. Usually a single newline is preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/quick-start-consumers.md` around lines 95 - 98, The file ends with multiple trailing blank lines; remove the extra empty lines so the file ends with a single newline (ensure exactly one trailing newline and no additional blank lines at EOF) — open quick-start-consumers.md, delete the redundant blank lines at the end, and save so the file terminates with a single newline character.packages/intent/src/cli.ts (1)
53-64: Use the exportedResolveCommandOptionsfor consistency.
listandinstallactions import their options types from their command modules; theresolveaction instead inlines the shape. IfResolveCommandOptionsever gains a field (e.g., another global-scan flag picked up fromGlobalScanFlags), this inline type will silently drift and the action could drop options beforerunResolveCommandsees them.♻️ Suggested refactor
-import { runResolveCommand } from './commands/resolve.js' +import { + runResolveCommand, + type ResolveCommandOptions, +} from './commands/resolve.js' @@ - .action( - async ( - use: string | undefined, - options: { - json?: boolean - global?: boolean - globalOnly?: boolean - }, - ) => { - await runResolveCommand(use, options, scanIntentsOrFail) - }, - ) + .action(async (use: string | undefined, options: ResolveCommandOptions) => { + await runResolveCommand(use, options, scanIntentsOrFail) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/cli.ts` around lines 53 - 64, The action handler in the resolve command inlines the options shape instead of using the exported ResolveCommandOptions type, risking divergence if that type later gains fields; update the action callback signature to accept the exported ResolveCommandOptions type (the same one used by list/install) and pass it through to runResolveCommand so all flags defined on ResolveCommandOptions (and any future GlobalScanFlags-derived fields) are preserved when calling runResolveCommand.packages/intent/src/commands/install-writer.ts (1)
64-95: Only the first marker pair is recognized — consider flagging duplicates.
readManagedBlockreturns on the firstintent-skills:start/endpair it finds. If a file ends up with two managed blocks (e.g., a user-authored copy/paste), the second block is silently left alone and the writer will only refresh the first. The post-writeverifyIntentSkillsBlockFilewill still pass because it also only looks at the first pair.Not a blocker — the write path is under your own control and most code paths reject malformed content — but a quick "duplicate managed block" diagnostic here (or in the verifier) would make it easier to notice stale fragments that slip in through manual edits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/commands/install-writer.ts` around lines 64 - 95, readManagedBlock currently only returns the first INTENT_SKILLS_START/INTENT_SKILLS_END pair and ignores any subsequent managed markers; update readManagedBlock to detect duplicate managed blocks by scanning the remainder of content after the found end (use INTENT_SKILLS_START and INTENT_SKILLS_END) and, if any additional start/end markers are present, push a descriptive error like "Duplicate intent-skills managed block found" into errors and still set hasMarker=true while returning the first managedBlock; reference the existing symbols readManagedBlock, INTENT_SKILLS_START, INTENT_SKILLS_END (and optionally mirror the same duplicate-checking logic in verifyIntentSkillsBlockFile) so duplicates are surfaced rather than silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli/intent-list.md`:
- Line 29: The inline code span containing the delimiter `, ` triggers
markdownlint MD038 due to leading/trailing spaces; edit the
docs/cli/intent-list.md line that mentions `REQUIRES` and `intent.requires` to
avoid a code span with surrounding spaces—either remove the backticks and write
the delimiter as ', ' plainly or escape the space (e.g. use ' ' or replace
the code span with a phrase like "joined by a comma and a space") so the
description still reads "REQUIRES uses intent.requires values joined by ', ';
empty values render as –" without MD038 violations.
In `@docs/getting-started/quick-start-consumers.md`:
- Around line 16-22: Change the list after "This creates or updates an
`intent-skills` block. It:" to use third-person singular verbs to match the
subject "It" — e.g., replace "Check" with "Checks", "Run" with "Runs", "Write"
with "Writes", "Preserve" with "Preserves", and "Verify" with "Verifies" in the
items describing AGENTS.md, CLAUDE.md, .cursorrules, local package scan, compact
mappings, preserving outside content, and managed block verification.
In `@packages/intent/src/resolver.ts`:
- Around line 109-111: The current filter on scanResult.warnings uses substring
matching (warning.includes(packageName)) which risks false positives/negatives;
update the filter in resolver.ts to match package identity instead: if warning
entries include a structured field (e.g., warning.packageName or similar), use
strict equality against packageName (warning.packageName === packageName);
otherwise normalize both sides and use a word-boundary/regex or tokenize the
warning message to match exact package tokens rather than a raw substring (e.g.,
/\b... \b/ logic) so the warnings array is filtered by true package matches when
producing warnings in the warnings property referenced here.
In `@packages/intent/tests/integration/scaffold.ts`:
- Around line 239-249: The runInstallCommand function buffers child process
output with execSync and switched stdio to 'pipe', but doesn't increase
execSync's default maxBuffer (1 MiB), causing ENOBUFS on real installs; update
the execSync call in runInstallCommand to pass a larger maxBuffer (e.g., 10 *
1024 * 1024 or 20 * 1024 * 1024) in the options object while keeping cwd, env,
and stdio: 'pipe' so install output is captured without being killed, and ensure
the thrown Error still uses formatInstallError(command, cwd, error).
---
Nitpick comments:
In `@docs/getting-started/quick-start-consumers.md`:
- Around line 95-98: The file ends with multiple trailing blank lines; remove
the extra empty lines so the file ends with a single newline (ensure exactly one
trailing newline and no additional blank lines at EOF) — open
quick-start-consumers.md, delete the redundant blank lines at the end, and save
so the file terminates with a single newline character.
In `@packages/intent/README.md`:
- Line 127: Update the CLI commands table entry for the "list" command in
packages/intent/README.md so it includes the new --global and --global-only
flags; specifically change the table row that currently shows `npx
`@tanstack/intent`@latest list [--json]` to include `--global` and `--global-only`
(e.g., `list [--json] [--global|--global-only]`) and ensure the short
description still reads "Discover local intent-enabled packages" or is adjusted
to reflect global discovery when flags are present; update any adjacent
explanatory text if necessary to match the documented behavior in lines
describing `list --global`.
In `@packages/intent/src/cli-output.ts`:
- Around line 1-8: The printWarnings function currently writes human-facing
warnings to stdout; change it to emit to stderr so machine-readable stdout isn't
contaminated. In the printWarnings function (symbol: printWarnings), keep the
same empty-array early return but replace the console.log calls with stderr
output (e.g., console.error or process.stderr.write) for the header and each
warning line so all warning text goes to stderr while preserving formatting and
behavior.
In `@packages/intent/src/cli.ts`:
- Around line 53-64: The action handler in the resolve command inlines the
options shape instead of using the exported ResolveCommandOptions type, risking
divergence if that type later gains fields; update the action callback signature
to accept the exported ResolveCommandOptions type (the same one used by
list/install) and pass it through to runResolveCommand so all flags defined on
ResolveCommandOptions (and any future GlobalScanFlags-derived fields) are
preserved when calling runResolveCommand.
In `@packages/intent/src/commands/install-writer.ts`:
- Around line 64-95: readManagedBlock currently only returns the first
INTENT_SKILLS_START/INTENT_SKILLS_END pair and ignores any subsequent managed
markers; update readManagedBlock to detect duplicate managed blocks by scanning
the remainder of content after the found end (use INTENT_SKILLS_START and
INTENT_SKILLS_END) and, if any additional start/end markers are present, push a
descriptive error like "Duplicate intent-skills managed block found" into errors
and still set hasMarker=true while returning the first managedBlock; reference
the existing symbols readManagedBlock, INTENT_SKILLS_START, INTENT_SKILLS_END
(and optionally mirror the same duplicate-checking logic in
verifyIntentSkillsBlockFile) so duplicates are surfaced rather than silently
ignored.
In `@packages/intent/src/display.ts`:
- Around line 39-64: printSkillLine can emit an ambiguous lookup hint when
opts.packageName is undefined because formatRuntimeSkillLookupHint then omits
the package qualifier; make packageName required to avoid regressions by
updating the function signature of printSkillLine (and its caller
printSkillTree) to require opts.packageName, adjust any call sites to pass
through the packageName (e.g., places in intent-library.ts and
commands/list.ts), and ensure formatRuntimeSkillLookupHint is always called with
a defined packageName and skillName so the runtime lookup hint includes the
package context.
In `@packages/intent/src/resolver.ts`:
- Line 77: availablePackages is built from scanResult.packages.map(candidate =>
candidate.name) and can contain duplicate package names (e.g., same name in
local and global scans); change the construction of availablePackages to
deduplicate names (for example by using a Set) so the error message lists each
package once—update the expression that builds availablePackages (currently
using scanResult.packages.map(...)) to convert the names into a unique list
before assigning.
In `@packages/intent/src/skill-paths.ts`:
- Around line 1-4: The import order is reversed causing an ESLint import/order
nit: move the type-only import "SkillEntry" from './types.js' so it comes after
the value import "toPosixPath" from './utils.js' (i.e., import toPosixPath and
other value imports first, then import type { SkillEntry }); keep the same
import specifiers and modules, just swap their positions to satisfy the linter.
In `@packages/intent/src/skill-use.ts`:
- Around line 29-42: formatSkillUse currently allows packageName or skillName to
contain '#' leading to ambiguous outputs (e.g. "a#b#c"); update formatSkillUse
to validate that neither packageName nor skillName includes the '#' character
and throw a SkillUseParseError (similar to the existing
'empty-package'/'empty-skill' cases) when found, so the function rejects
embedded separators and keeps round-trip consistency with parseSkillUse;
reference the formatSkillUse function and reuse SkillUseParseError for the new
'invalid-package'/'invalid-skill' or a single 'invalid-skill-use' error code as
appropriate.
In `@packages/intent/tests/library-scanner.test.ts`:
- Around line 189-197: The symlink creation in the test using symlinkSync can
fail on Windows CI (EPERM); update the test to skip on Windows by wrapping the
test or the symlink block with a platform guard such as
it.skipIf(process.platform === 'win32') or if (process.platform === 'win32') {
return this.skip?.(); } so the symlinkSync calls (the lines invoking symlinkSync
for queryStoreDir and routerStoreDir) are not executed on Windows; ensure the
skip is applied at the test level (the describe/it containing these symlinkSync
calls) so other assertions still run on non-Windows platforms.
🪄 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: 75cb9047-256b-44b1-9c21-f1a16c473ca5
📒 Files selected for processing (33)
docs/cli/intent-install.mddocs/cli/intent-list.mddocs/cli/intent-resolve.mddocs/config.jsondocs/getting-started/quick-start-consumers.mddocs/getting-started/quick-start-maintainers.mddocs/overview.mdpackages/intent/README.mdpackages/intent/src/cli-output.tspackages/intent/src/cli-support.tspackages/intent/src/cli.tspackages/intent/src/commands/install-writer.tspackages/intent/src/commands/install.tspackages/intent/src/commands/list.tspackages/intent/src/commands/resolve.tspackages/intent/src/discovery/register.tspackages/intent/src/discovery/walk.tspackages/intent/src/display.tspackages/intent/src/index.tspackages/intent/src/intent-library.tspackages/intent/src/library-scanner.tspackages/intent/src/resolver.tspackages/intent/src/scanner.tspackages/intent/src/skill-paths.tspackages/intent/src/skill-use.tspackages/intent/src/types.tspackages/intent/tests/cli.test.tspackages/intent/tests/install-writer.test.tspackages/intent/tests/integration/scaffold.tspackages/intent/tests/library-scanner.test.tspackages/intent/tests/resolver.test.tspackages/intent/tests/skill-paths.test.tspackages/intent/tests/skill-use.test.ts
Comment on lines
+239
to
+249
| function runInstallCommand( | ||
| command: string, | ||
| cwd: string, | ||
| env: NodeJS.ProcessEnv, | ||
| ): void { | ||
| try { | ||
| execSync(command, { cwd, env, stdio: 'pipe' }) | ||
| } catch (error) { | ||
| throw new Error(formatInstallError(command, cwd, error)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Consider raising maxBuffer for buffered install output.
Switching stdio from 'ignore' to 'pipe' means npm/pnpm/yarn/bun install output is now buffered in memory. execSync's default maxBuffer is 1 MiB, which real installs (especially npm install with verbose resolver output or pnpm/yarn on monorepos) can exceed, causing the child to be killed with ENOBUFS and producing a misleading "install failed" error in tests that would previously have succeeded.
🔧 Proposed fix
try {
- execSync(command, { cwd, env, stdio: 'pipe' })
+ execSync(command, { cwd, env, stdio: 'pipe', maxBuffer: 64 * 1024 * 1024 })
} catch (error) {
throw new Error(formatInstallError(command, cwd, error))
}📝 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
| function runInstallCommand( | |
| command: string, | |
| cwd: string, | |
| env: NodeJS.ProcessEnv, | |
| ): void { | |
| try { | |
| execSync(command, { cwd, env, stdio: 'pipe' }) | |
| } catch (error) { | |
| throw new Error(formatInstallError(command, cwd, error)) | |
| } | |
| } | |
| function runInstallCommand( | |
| command: string, | |
| cwd: string, | |
| env: NodeJS.ProcessEnv, | |
| ): void { | |
| try { | |
| execSync(command, { cwd, env, stdio: 'pipe', maxBuffer: 64 * 1024 * 1024 }) | |
| } catch (error) { | |
| throw new Error(formatInstallError(command, cwd, error)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/tests/integration/scaffold.ts` around lines 239 - 249, The
runInstallCommand function buffers child process output with execSync and
switched stdio to 'pipe', but doesn't increase execSync's default maxBuffer (1
MiB), causing ENOBUFS on real installs; update the execSync call in
runInstallCommand to pass a larger maxBuffer (e.g., 10 * 1024 * 1024 or 20 *
1024 * 1024) in the options object while keeping cwd, env, and stdio: 'pipe' so
install output is captured without being killed, and ensure the thrown Error
still uses formatInstallError(command, cwd, error).
Merging this PR will improve performance by 16.65%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | scans a consumer workspace |
67.5 ms | 57.8 ms | +16.65% |
Comparing install-prompt (f8523d2) with main (6069ffc)
- Fix loose `.includes()` warning filter in resolver that matched package name prefixes (e.g. @tanstack/query matching @tanstack/query-core) - Fix "reference-only" → "reference" in docs and INSTALL_PROMPT to match the NON_ACTIONABLE_SKILL_TYPES set - Add newline/tab escaping to quoteYamlString for defense-in-depth - Update runtime skill lookup hints to reference `intent resolve` instead of the older `intent list --json` - Unify SkillLookupTarget with SkillUse (identical types) - Add missing packageRoot to intent-list.md JSON schema - Code simplifier: extend GlobalScanFlags, use localeCompare, consolidate exports, simplify arrow functions - Add test coverage for --global/--global-only mutual exclusion, verify when target file missing, empty description fallback, whitespace collapsing in descriptions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Review
Pushed a commit with bug fixes, test coverage, and cleanup — all 312 tests pass.
Fixes applied
- Warning prefix match bug —
resolver.tsused.includes(packageName)to filter warnings, which caused@tanstack/queryto match warnings about@tanstack/query-core. Now checks the character after the match to ensure it's a word boundary. - Doc/code mismatch — docs and
INSTALL_PROMPTsaid "reference-only" butNON_ACTIONABLE_SKILL_TYPESfilters on'reference'. Also added the missing'maintainer'to the INSTALL_PROMPT list. - Stale
intent list --jsonreferences —formatRuntimeSkillLookupHintstill pointed agents atintent list --jsoninstead of the newintent resolvecommand. quoteYamlStringmissing newline escaping — defense-in-depth fix.formatWhencollapses whitespace before reaching it, so not currently exploitable, but the function should be correct on its own.SkillLookupTarget→SkillUse— identical types in separate files, unified.- Missing
packageRootinintent-list.mdJSON schema
Test coverage added
--global+--global-onlymutual exclusion (list, install, resolve)verifyIntentSkillsBlockFilewhen target file doesn't exist- Empty description fallback in
formatWhen - Whitespace collapsing in skill descriptions
- Warning prefix false-match scenario
Code simplifier changes
ResolveCommandOptionsextendsGlobalScanFlagsfor consistency- CLI uses typed
ResolveCommandOptionsinstead of inline type localeComparereplaces manual</>string comparison- Consolidated split export blocks in
index.ts - Simplified arrow functions in
resolver.ts
Design feedback: intent install default behavior
Per conversation with Kyle — the current default of dumping all actionable skill mappings into the agent config may not be what users want. Most skills from installed packages aren't relevant to a particular project (transitive deps, unused features, etc.).
Proposed direction:
intent install(default) → write a lightweight "skill check" instruction block that tells the agent to discover and load skills on demand vialist/resolveintent install --map→ explicit opt-in for the current behavior of writing all skill mappings directly
This keeps the install-writer infrastructure intact (moves behind --map) and makes the default more useful out of the box.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
docs/cli/intent-list.md (1)
29-29:⚠️ Potential issue | 🟡 MinorFix markdownlint MD038 in the delimiter code span.
The inline code span
`, `still contains surrounding whitespace, which markdownlint flags.📝 Proposed fix
-`REQUIRES` uses `intent.requires` values joined by `, `; empty values render as `–`. +`REQUIRES` uses `intent.requires` values joined by a comma and space; empty values render as `–`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/intent-list.md` at line 29, The inline code span for the separator currently includes surrounding whitespace (the token `` `, ` ``) which triggers markdownlint MD038; change it to a code span without surrounding spaces, e.g., use `','` (or ','), in the sentence that mentions joining intent.requires values (the `REQUIRES` description referencing `intent.requires`) so the code span contains only the comma character.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli/intent-list.md`:
- Around line 79-85: The example JSON incorrectly shows nodeModules.local.path
as always a string; update the docs to reflect that NodeModulesScanTarget.path
is nullable by documenting nodeModules.local.path as "string | null" (and adjust
the example value to null where appropriate) and mention the same for
nodeModules.global.path; ensure references to NodeModulesScanTarget.path in the
text and example match the type "string | null".
In `@packages/intent/src/cli.ts`:
- Around line 13-22: The import lines use inline type specifiers which ESLint
flags; split the option types into type-only imports by changing imports so
values (runInstallCommand, runListCommand, runMetaCommand, runResolveCommand)
remain regular imports and the option interfaces (InstallCommandOptions,
ListCommandOptions, ResolveCommandOptions) are imported with "import type { ...
}" (e.g., keep "import { runInstallCommand } from './commands/install.js'" and
add "import type { InstallCommandOptions } from './commands/install.js'"),
applying the same pattern for runListCommand/ListCommandOptions,
runResolveCommand/ResolveCommandOptions while leaving runMetaCommand as a plain
import.
In `@packages/intent/src/commands/install.ts`:
- Around line 40-49: The user-facing "no skills" message is inconsistent: update
the message used in runInstallCommand (and any string constants in
packages/intent/src/commands/install.ts that currently read "No actionable
intent skills found.") to exactly "No intent-enabled skills found." so the
prompt, CLI behavior, and docs match; search for the string "No actionable
intent skills found." and replace it with the canonical "No intent-enabled
skills found." in the runInstallCommand flow and any related exported messages
in this module.
- Around line 3-14: Split all type-only imports into separate `import type`
statements and keep value imports ordered alphabetically: move `type
GlobalScanFlags` out of the mixed import from '../cli-support.js' into its own
`import type { GlobalScanFlags } from '../cli-support.js'`, change `import type
{ ScanOptions, ScanResult } from '../types.js'` to ensure both are type-only
imports, and ensure the value imports `printWarnings` and
`scanOptionsFromGlobalFlags` remain in a single non-type import (ordered
alphabetically). Also ensure the value imports from './install-writer.js' —
`buildIntentSkillsBlock`, `resolveIntentSkillsBlockTargetPath`,
`verifyIntentSkillsBlockFile`, `writeIntentSkillsBlock` — are kept in one value
import and ordered consistently (alphabetically) while no type annotations are
mixed into that line.
In `@packages/intent/src/commands/resolve.ts`:
- Around line 2-8: The import mixes a value import with an inline type specifier
which trips the import/consistent-type-specifier-style rule; change the import
so the type GlobalScanFlags is imported in its own type-only import statement
(i.e., keep scanOptionsFromGlobalFlags and scanOptionsFromGlobalFlags in the
existing import but remove the inline "type GlobalScanFlags" and add a separate
`import type { GlobalScanFlags } from '../cli-support.js'`) so the module
imports use consistent type-only syntax while leaving resolveSkillUse,
parseSkillUse, ScanOptions, and ScanResult untouched.
In `@packages/intent/src/skill-paths.ts`:
- Around line 1-5: ESLint flagged import ordering: move the value import
toPosixPath from './utils.js' so it appears before the type-only imports
(SkillUse and SkillEntry). Update the import order in skill-paths.ts so that
toPosixPath is imported immediately after node imports (existsSync, join,
relative) and before the type imports "import type { SkillUse } from
'./skill-use.js'" and "import type { SkillEntry } from './types.js'".
- Around line 10-16: The isAbsolutePath function currently only checks for
double-backslash UNC paths and misses single leading-backslash Windows
root-relative paths; update isAbsolutePath to treat a single leading backslash
as absolute (e.g., add path.startsWith('\\') or adjust the regex to match a
single or double leading backslash) while keeping the existing checks for
forward slash and drive-letter paths (function name: isAbsolutePath).
In `@packages/intent/tests/resolver.test.ts`:
- Line 2: The import members are out of alphabetical order; update the import
statement so ResolveSkillUseError appears before resolveSkillUse (i.e., import {
ResolveSkillUseError, resolveSkillUse } from '../src/resolver.js') to satisfy
ESLint ordering; ensure you only reorder the named imports and keep the same
source and file extension.
In `@scripts/create-github-release.mjs`:
- Around line 118-186: The changelog/prerelease bug: update parseVersion and the
headingPattern in getChangelogSections to accept optional prerelease/build
suffixes (e.g., allow "-beta.1" and "+build.1") so headings like "##
1.0.0-beta.1" are matched, and replace the fallback localeCompare in
compareVersions with a proper SemVer precedence algorithm: parse a version into
core numbers and an ordered prerelease identifier array (split on dots), compare
core numeric parts first, then if cores equal apply SemVer prerelease rules (no
prerelease > any prerelease; otherwise compare identifier pairs numeric vs
alphanumeric, numeric < non-numeric, compare numeric values, lexicographic for
strings, longer list wins when all equal), and use this semver compare in
isVersionInRange; reference functions parseVersion, compareVersions,
isVersionInRange and headingPattern/ getChangelogSections when making these
changes.
---
Duplicate comments:
In `@docs/cli/intent-list.md`:
- Line 29: The inline code span for the separator currently includes surrounding
whitespace (the token `` `, ` ``) which triggers markdownlint MD038; change it
to a code span without surrounding spaces, e.g., use `','` (or ','), in the
sentence that mentions joining intent.requires values (the `REQUIRES`
description referencing `intent.requires`) so the code span contains only the
comma character.
🪄 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: 16814a1c-3b4d-4785-aeb7-7aa498b51dad
📒 Files selected for processing (17)
.changeset/yummy-balloons-bow.mddocs/cli/intent-install.mddocs/cli/intent-list.mdpackages/intent/package.jsonpackages/intent/src/cli.tspackages/intent/src/commands/install-writer.tspackages/intent/src/commands/install.tspackages/intent/src/commands/resolve.tspackages/intent/src/display.tspackages/intent/src/index.tspackages/intent/src/resolver.tspackages/intent/src/skill-paths.tspackages/intent/tests/cli.test.tspackages/intent/tests/install-writer.test.tspackages/intent/tests/resolver.test.tspackages/intent/tests/skill-paths.test.tsscripts/create-github-release.mjs
✅ Files skipped from review due to trivial changes (6)
- packages/intent/package.json
- .changeset/yummy-balloons-bow.md
- packages/intent/tests/skill-paths.test.ts
- packages/intent/tests/install-writer.test.ts
- packages/intent/src/commands/install-writer.ts
- packages/intent/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/intent/src/display.ts
- packages/intent/src/resolver.ts
Comment on lines
+79
to
+85
| "nodeModules": { | ||
| "local": { | ||
| "path": "string", | ||
| "detected": true, | ||
| "exists": true, | ||
| "scanned": true | ||
| }, |
Contributor
There was a problem hiding this comment.
Document nodeModules.local.path as nullable.
NodeModulesScanTarget.path is string | null for both local and global scan targets, so the JSON schema example should not imply local always has a path.
📝 Proposed fix
"local": {
- "path": "string",
+ "path": "string | null",
"detected": true,
"exists": true,
"scanned": true📝 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
| "nodeModules": { | |
| "local": { | |
| "path": "string", | |
| "detected": true, | |
| "exists": true, | |
| "scanned": true | |
| }, | |
| "nodeModules": { | |
| "local": { | |
| "path": "string | null", | |
| "detected": true, | |
| "exists": true, | |
| "scanned": true | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cli/intent-list.md` around lines 79 - 85, The example JSON incorrectly
shows nodeModules.local.path as always a string; update the docs to reflect that
NodeModulesScanTarget.path is nullable by documenting nodeModules.local.path as
"string | null" (and adjust the example value to null where appropriate) and
mention the same for nodeModules.global.path; ensure references to
NodeModulesScanTarget.path in the text and example match the type "string |
null".
Comment on lines
+2
to
+8
| import { | ||
| scanOptionsFromGlobalFlags, | ||
| type GlobalScanFlags, | ||
| } from '../cli-support.js' | ||
| import { resolveSkillUse } from '../resolver.js' | ||
| import { parseSkillUse } from '../skill-use.js' | ||
| import type { ScanOptions, ScanResult } from '../types.js' |
Contributor
There was a problem hiding this comment.
Split the type import to satisfy ESLint.
The current inline type GlobalScanFlags import is flagged by import/consistent-type-specifier-style and can fail lint CI.
🧹 Proposed fix
import { fail } from '../cli-error.js'
import {
scanOptionsFromGlobalFlags,
- type GlobalScanFlags,
} from '../cli-support.js'
+import type { GlobalScanFlags } from '../cli-support.js'
import { resolveSkillUse } from '../resolver.js'
import { parseSkillUse } from '../skill-use.js'
import type { ScanOptions, ScanResult } from '../types.js'📝 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
| import { | |
| scanOptionsFromGlobalFlags, | |
| type GlobalScanFlags, | |
| } from '../cli-support.js' | |
| import { resolveSkillUse } from '../resolver.js' | |
| import { parseSkillUse } from '../skill-use.js' | |
| import type { ScanOptions, ScanResult } from '../types.js' | |
| import { | |
| scanOptionsFromGlobalFlags, | |
| } from '../cli-support.js' | |
| import type { GlobalScanFlags } from '../cli-support.js' | |
| import { resolveSkillUse } from '../resolver.js' | |
| import { parseSkillUse } from '../skill-use.js' | |
| import type { ScanOptions, ScanResult } from '../types.js' |
🧰 Tools
🪛 ESLint
[error] 4-4: Member 'GlobalScanFlags' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 4-4: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/resolve.ts` around lines 2 - 8, The import mixes
a value import with an inline type specifier which trips the
import/consistent-type-specifier-style rule; change the import so the type
GlobalScanFlags is imported in its own type-only import statement (i.e., keep
scanOptionsFromGlobalFlags and scanOptionsFromGlobalFlags in the existing import
but remove the inline "type GlobalScanFlags" and add a separate `import type {
GlobalScanFlags } from '../cli-support.js'`) so the module imports use
consistent type-only syntax while leaving resolveSkillUse, parseSkillUse,
ScanOptions, and ScanResult untouched.
Comment on lines
+118
to
186
| function parseVersion(version) { | ||
| const match = version.match(/^(\d+)\.(\d+)\.(\d+)$/) | ||
|
|
||
| if (!match) { | ||
| return null | ||
| } | ||
|
|
||
| const changelog = fs.readFileSync(changelogPath, 'utf8') | ||
| const marker = `## ${version}` | ||
| const start = changelog.indexOf(marker) | ||
| return match.slice(1).map(Number) | ||
| } | ||
|
|
||
| if (start === -1) { | ||
| return null | ||
| function compareVersions(leftVersion, rightVersion) { | ||
| const left = parseVersion(leftVersion) | ||
| const right = parseVersion(rightVersion) | ||
|
|
||
| if (!left || !right) { | ||
| return leftVersion.localeCompare(rightVersion) | ||
| } | ||
|
|
||
| const bodyStart = changelog.indexOf('\n', start) | ||
| const nextSection = changelog.indexOf('\n## ', bodyStart + 1) | ||
| for (let index = 0; index < left.length; index += 1) { | ||
| if (left[index] !== right[index]) { | ||
| return left[index] - right[index] | ||
| } | ||
| } | ||
|
|
||
| return changelog | ||
| .slice(bodyStart + 1, nextSection === -1 ? undefined : nextSection) | ||
| .trim() | ||
| return 0 | ||
| } | ||
|
|
||
| function isVersionInRange(version, previousVersion, currentVersion) { | ||
| if (compareVersions(version, currentVersion) > 0) { | ||
| return false | ||
| } | ||
|
|
||
| if (!previousVersion) { | ||
| return true | ||
| } | ||
|
|
||
| return compareVersions(version, previousVersion) > 0 | ||
| } | ||
|
|
||
| function getChangelogSections(changelogPath, previousVersion, currentVersion) { | ||
| if (!fs.existsSync(changelogPath)) { | ||
| return [] | ||
| } | ||
|
|
||
| const changelog = fs.readFileSync(changelogPath, 'utf8') | ||
| const sections = [] | ||
| const headingPattern = /^## (\d+\.\d+\.\d+)\s*$/gm | ||
| const headings = Array.from(changelog.matchAll(headingPattern)) | ||
|
|
||
| for (let index = 0; index < headings.length; index += 1) { | ||
| const heading = headings[index] | ||
| const version = heading[1] | ||
|
|
||
| if (!isVersionInRange(version, previousVersion, currentVersion)) { | ||
| continue | ||
| } | ||
|
|
||
| const nextHeading = headings[index + 1] | ||
| const bodyStart = changelog.indexOf('\n', heading.index) | ||
| const body = changelog.slice(bodyStart + 1, nextHeading?.index).trim() | ||
|
|
||
| sections.push({ | ||
| version, | ||
| body: body || '- No changelog entries', | ||
| }) | ||
| } | ||
|
|
||
| return sections | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any non-plain-semver H2 headings already committed to package changelogs.
fd -t f 'CHANGELOG.md' packages | xargs rg -nP '^##\s+\d+\.\d+\.\d+[^\s]'
# And list all H2 version-ish headings for spot-check.
fd -t f 'CHANGELOG.md' packages | xargs rg -nP '^##\s+\S+'Repository: TanStack/intent
Length of output: 433
🏁 Script executed:
#!/bin/bash
# Check git history for any use of --prerelease flag
git log --all --oneline --grep="prerelease" | head -20
# Check if any package.json has a prerelease version
fd -t f 'package.json' packages | xargs rg '"version":\s+".*-'
# Look for recent release tags to see if any are prerelease style
git tag -l | grep -E '[0-9]+\.[0-9]+\.[0-9]+-' | tail -10Repository: TanStack/intent
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search the script itself for prerelease handling logic or --prerelease usage
rg -n 'prerelease|beta|rc|alpha' scripts/create-github-release.mjs
# Check the actual invocation in CI to see if --prerelease is passed
fd -t f -E node_modules '.github/workflows/*.{yml,yaml}' -o '.github/*.yml' -o '.github/*.yaml' 2>/dev/null | xargs grep -l 'create-github-release' 2>/dev/null | head -3 | xargs cat | head -100Repository: TanStack/intent
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Look at the script to see how isPrerelease is used
sed -n '1,20p' scripts/create-github-release.mjs
sed -n '245,275p' scripts/create-github-release.mjs
# Check if isPrerelease affects changelog parsing or only gh release
rg -n 'isPrerelease' scripts/create-github-release.mjs -A 3 -B 1Repository: TanStack/intent
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Check workflows to see if --prerelease is ever invoked
find .github/workflows -name '*.yml' -o -name '*.yaml' | xargs grep -l 'create-github-release' 2>/dev/null | head -5 | xargs catRepository: TanStack/intent
Length of output: 3309
🏁 Script executed:
#!/bin/bash
# Verify Changesets behavior: does it emit prerelease-suffixed versions in CHANGELOG?
# Check if there's a .changeset/pre.json indicating prerelease mode
if [ -f .changeset/pre.json ]; then
cat .changeset/pre.json
else
echo ".changeset/pre.json does not exist (no active prerelease)"
fi
# Look for Changesets config to understand its behavior
find .changeset -type f -name '*.json' -o -name '*.yaml' -o -name '*.yml' 2>/dev/null | head -5 | xargs cat 2>/dev/nullRepository: TanStack/intent
Length of output: 564
Prerelease versions are silently dropped from release notes.
This script has a --prerelease mode that is invoked from CI when pushing to *-pre branches. In that scenario, Changesets generates prerelease versions (e.g., 1.0.0-beta.1) and changelog headings like ## 1.0.0-beta.1. However, both parseVersion (line ~119, /^(\d+)\.(\d+)\.(\d+)$/) and the heading scanner in getChangelogSections (line ~164, /^## (\d+\.\d+\.\d+)\s*$/gm) only match plain X.Y.Z, so prerelease headings will not match and those sections will be silently omitted from the generated release notes body (yielding - No changelog entries).
Additionally, if pkg.version or pkg.previousVersion is a prerelease, compareVersions falls back to localeCompare, which does not correctly order semver prerelease identifiers (e.g., 1.2.0-rc.10 vs 1.2.0-rc.2), so isVersionInRange may include or exclude the wrong versions.
Update both regex patterns to accept optional prerelease/build suffixes and update compareVersions to handle prerelease ordering according to semver precedence rules.
🔧 Minimal regex update to at least capture prerelease headings
-function parseVersion(version) {
- const match = version.match(/^(\d+)\.(\d+)\.(\d+)$/)
+function parseVersion(version) {
+ const match = version.match(/^(\d+)\.(\d+)\.(\d+)(?:-[0-9A-Za-z.-]+)?$/)
...
- const headingPattern = /^## (\d+\.\d+\.\d+)\s*$/gm
+ const headingPattern = /^## (\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?)\s*$/gmNote: this alone is insufficient — compareVersions must be updated to order prerelease versions correctly (SemVer §11) rather than relying on localeCompare.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/create-github-release.mjs` around lines 118 - 186, The
changelog/prerelease bug: update parseVersion and the headingPattern in
getChangelogSections to accept optional prerelease/build suffixes (e.g., allow
"-beta.1" and "+build.1") so headings like "## 1.0.0-beta.1" are matched, and
replace the fallback localeCompare in compareVersions with a proper SemVer
precedence algorithm: parse a version into core numbers and an ordered
prerelease identifier array (split on dots), compare core numeric parts first,
then if cores equal apply SemVer prerelease rules (no prerelease > any
prerelease; otherwise compare identifier pairs numeric vs alphanumeric, numeric
< non-numeric, compare numeric values, lexicographic for strings, longer list
wins when all equal), and use this semver compare in isVersionInRange; reference
functions parseVersion, compareVersions, isVersionInRange and headingPattern/
getChangelogSections when making these changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/intent/src/commands/install.ts (1)
193-193: Dead call toscanOptionsFromGlobalFlags.This invocation discards its return value and is effectively a no-op. The real usage is on Line 238 inside the
--mapbranch. Remove it to avoid confusion.♻️ Proposed fix
- scanOptionsFromGlobalFlags(options) - if (!options.map) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/commands/install.ts` at line 193, Remove the dead, no-op invocation of scanOptionsFromGlobalFlags that discards its return value; locate the standalone call to scanOptionsFromGlobalFlags in the install command and delete it (do not change the valid usage inside the --map branch where scanOptionsFromGlobalFlags is already used properly), or if the call was intended to set up state, instead capture and use its return value where appropriate (e.g., assign to a variable and pass it into the existing --map branch call).
🤖 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/commands/install.ts`:
- Around line 198-205: The dry-run path uses a non-null assertion on
resolveIntentSkillsBlockTargetPath(process.cwd(), 1) which can be falsy and
crash; change the branch to guard the return the same way the --map branch does
by checking the result of resolveIntentSkillsBlockTargetPath before calling
formatTargetPath and printing generated.block, and print a friendly "no target
found" message (or fallback text) when it returns null/undefined; update the
code around the options.dryRun block (references:
resolveIntentSkillsBlockTargetPath and formatTargetPath) to perform a null check
instead of using targetPath!.
In `@packages/intent/src/commands/load.ts`:
- Around line 19-27: The containment check in isPathInsidePackageRoot (and the
related logic in runLoadCommand) uses resolve()/relative() which are lexical and
can be bypassed by symlinks; instead obtain the real filesystem paths via
fs.realpathSync (or fs.promises.realpath) for both packageRoot and the candidate
path before computing relative() and performing the containment check, and call
realpath on files before passing them to readFileSync in runLoadCommand so
symlinked files outside the package root are rejected; update
isPathInsidePackageRoot to compare real paths and add proper try/catch around
realpath calls to surface errors.
In `@packages/intent/src/skill-paths.ts`:
- Around line 52-59: The rewrite currently uses hasStableSymlink to decide
whether to rewrite skill.path to node_modules/<packageName>/..., but it doesn't
verify that packageRoot is a top-level package directory; update the condition
so we only rewrite when hasStableSymlink is true AND
hasPackageManagerInternalPath(packageRoot) returns false. In practice: inside
the loop or before altering skill.path (the block that uses
toPosixPath(relative(packageRoot, skill.path)) and assigns `skill.path =
node_modules/${packageName}/${relFromPackage}`), add a guard that calls
hasPackageManagerInternalPath(packageRoot) and skips the rewrite when it detects
pnpm/.bun/.yarn internals, so we don't invent top-level load paths for internal
package roots.
---
Nitpick comments:
In `@packages/intent/src/commands/install.ts`:
- Line 193: Remove the dead, no-op invocation of scanOptionsFromGlobalFlags that
discards its return value; locate the standalone call to
scanOptionsFromGlobalFlags in the install command and delete it (do not change
the valid usage inside the --map branch where scanOptionsFromGlobalFlags is
already used properly), or if the call was intended to set up state, instead
capture and use its return value where appropriate (e.g., assign to a variable
and pass it into the existing --map branch call).
🪄 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: 50917aad-3874-43b6-ade4-c90580b0a1ee
📒 Files selected for processing (20)
docs/cli/intent-install.mddocs/cli/intent-list.mddocs/cli/intent-load.mddocs/config.jsondocs/getting-started/quick-start-consumers.mddocs/getting-started/quick-start-maintainers.mddocs/overview.mdpackages/intent/README.mdpackages/intent/src/cli.tspackages/intent/src/commands/install-writer.tspackages/intent/src/commands/install.tspackages/intent/src/commands/load.tspackages/intent/src/intent-library.tspackages/intent/src/skill-paths.tspackages/intent/tests/cli.test.tspackages/intent/tests/install-writer.test.tspackages/intent/tests/integration/scaffold.tspackages/intent/tests/resolver.test.tspackages/intent/tests/skill-paths.test.tsterminalOutput
✅ Files skipped from review due to trivial changes (7)
- docs/getting-started/quick-start-maintainers.md
- docs/config.json
- packages/intent/src/intent-library.ts
- docs/overview.md
- terminalOutput
- docs/cli/intent-load.md
- packages/intent/tests/install-writer.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/intent/tests/resolver.test.ts
- packages/intent/README.md
- packages/intent/src/commands/install-writer.ts
Comment on lines
+198
to
+205
| if (options.dryRun) { | ||
| const targetPath = resolveIntentSkillsBlockTargetPath(process.cwd(), 1) | ||
| console.log( | ||
| `Generated skill loading guidance for ${formatTargetPath(targetPath!)}.`, | ||
| ) | ||
| console.log(generated.block) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Non-null assertion can mask a crash in the guidance dry-run path.
resolveIntentSkillsBlockTargetPath(process.cwd(), 1) is asserted non-null with targetPath!, but the --map branch at Line 248 explicitly treats the same function's return as possibly falsy and handles the "no target" case. If resolution can yield no target here (e.g., unsupported cwd), this will throw a confusing runtime error instead of a friendly message. Either guard it symmetrically or document why a target is guaranteed in the guidance branch.
🛡️ Suggested guard
- if (options.dryRun) {
- const targetPath = resolveIntentSkillsBlockTargetPath(process.cwd(), 1)
- console.log(
- `Generated skill loading guidance for ${formatTargetPath(targetPath!)}.`,
- )
- console.log(generated.block)
- return
- }
+ if (options.dryRun) {
+ const targetPath = resolveIntentSkillsBlockTargetPath(process.cwd(), 1)
+ if (!targetPath) {
+ fail('Could not resolve a target file for intent-skills guidance.')
+ }
+ console.log(
+ `Generated skill loading guidance for ${formatTargetPath(targetPath)}.`,
+ )
+ console.log(generated.block)
+ return
+ }📝 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
| if (options.dryRun) { | |
| const targetPath = resolveIntentSkillsBlockTargetPath(process.cwd(), 1) | |
| console.log( | |
| `Generated skill loading guidance for ${formatTargetPath(targetPath!)}.`, | |
| ) | |
| console.log(generated.block) | |
| return | |
| } | |
| if (options.dryRun) { | |
| const targetPath = resolveIntentSkillsBlockTargetPath(process.cwd(), 1) | |
| if (!targetPath) { | |
| fail('Could not resolve a target file for intent-skills guidance.') | |
| } | |
| console.log( | |
| `Generated skill loading guidance for ${formatTargetPath(targetPath)}.`, | |
| ) | |
| console.log(generated.block) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/install.ts` around lines 198 - 205, The dry-run
path uses a non-null assertion on
resolveIntentSkillsBlockTargetPath(process.cwd(), 1) which can be falsy and
crash; change the branch to guard the return the same way the --map branch does
by checking the result of resolveIntentSkillsBlockTargetPath before calling
formatTargetPath and printing generated.block, and print a friendly "no target
found" message (or fallback text) when it returns null/undefined; update the
code around the options.dryRun block (references:
resolveIntentSkillsBlockTargetPath and formatTargetPath) to perform a null check
instead of using targetPath!.
Comment on lines
+19
to
+27
| function isPathInsidePackageRoot(path: string, packageRoot: string): boolean { | ||
| const relativePath = relative( | ||
| resolveFromCwd(packageRoot), | ||
| resolveFromCwd(path), | ||
| ) | ||
| return ( | ||
| relativePath === '' || | ||
| (!relativePath.startsWith('..') && !isAbsolute(relativePath)) | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the current containment check and read order.
# Expectation before the fix: containment uses resolve()/relative() only, then readFileSync() reads the path.
sed -n '15,67p' packages/intent/src/commands/load.tsRepository: TanStack/intent
Length of output: 1501
Resolve real paths before enforcing package-root containment to prevent symlink bypass.
The containment check on line 20 performs lexical path comparison using relative() and resolve(), which do not follow symlinks. A symlink inside the package root pointing outside it would pass this check, but readFileSync() follows symlinks, allowing unchecked access.
🛡️ Proposed fix
-import { existsSync, readFileSync } from 'node:fs'
+import { existsSync, readFileSync, realpathSync } from 'node:fs'
@@
function isPathInsidePackageRoot(path: string, packageRoot: string): boolean {
+ const packageRootPath = realpathSync(resolveFromCwd(packageRoot))
+ const skillPath = realpathSync(resolveFromCwd(path))
const relativePath = relative(
- resolveFromCwd(packageRoot),
- resolveFromCwd(path),
+ packageRootPath,
+ skillPath,
)
@@
const result = await scanIntentsOrFail(scanOptionsFromGlobalFlags(options))
const resolved = resolveSkillUse(use, result)
const resolvedPath = resolveFromCwd(resolved.path)
- if (!isPathInsidePackageRoot(resolved.path, resolved.packageRoot)) {
+ if (!existsSync(resolvedPath)) {
+ fail(`Resolved skill file was not found: ${resolved.path}`)
+ }
+
+ if (!isPathInsidePackageRoot(resolvedPath, resolved.packageRoot)) {
fail(
`Resolved skill path for "${use}" is outside package root: ${resolved.path}`,
)
}
-
- if (!existsSync(resolvedPath)) {
- fail(`Resolved skill file was not found: ${resolved.path}`)
- }Also applies to: lines 47–57 in runLoadCommand
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/load.ts` around lines 19 - 27, The containment
check in isPathInsidePackageRoot (and the related logic in runLoadCommand) uses
resolve()/relative() which are lexical and can be bypassed by symlinks; instead
obtain the real filesystem paths via fs.realpathSync (or fs.promises.realpath)
for both packageRoot and the candidate path before computing relative() and
performing the containment check, and call realpath on files before passing them
to readFileSync in runLoadCommand so symlinked files outside the package root
are rejected; update isPathInsidePackageRoot to compare real paths and add
proper try/catch around realpath calls to surface errors.
Comment on lines
+52
to
+59
| const hasStableSymlink = | ||
| packageName !== '' && | ||
| existsSync(join(projectRoot, 'node_modules', packageName)) | ||
|
|
||
| for (const skill of skills) { | ||
| if (hasStableSymlink) { | ||
| const relFromPackage = toPosixPath(relative(packageRoot, skill.path)) | ||
| skill.path = `node_modules/${packageName}/${relFromPackage}` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether rewriteSkillLoadPaths currently treats top-level path existence
# as sufficient for rewriting, without checking that it matches the scanned packageRoot.
sed -n '41,64p' packages/intent/src/skill-paths.tsRepository: TanStack/intent
Length of output: 674
🏁 Script executed:
# Find all calls to rewriteSkillLoadPaths
rg -n 'rewriteSkillLoadPaths' --type tsRepository: TanStack/intent
Length of output: 699
🏁 Script executed:
# Get the full function signature and surrounding context
head -80 packages/intent/src/skill-paths.tsRepository: TanStack/intent
Length of output: 2581
🏁 Script executed:
# Check the call site in discovery/register.ts
sed -n '70,90p' packages/intent/src/discovery/register.tsRepository: TanStack/intent
Length of output: 606
🏁 Script executed:
# Check the call site in library-scanner.ts
sed -n '140,160p' packages/intent/src/library-scanner.tsRepository: TanStack/intent
Length of output: 632
🏁 Script executed:
# Get more context for the register.ts call site - where does dirPath come from?
sed -n '50,95p' packages/intent/src/discovery/register.tsRepository: TanStack/intent
Length of output: 1421
🏁 Script executed:
# Get more context for the library-scanner.ts call site - where does dir come from?
sed -n '120,160p' packages/intent/src/library-scanner.tsRepository: TanStack/intent
Length of output: 1163
🏁 Script executed:
# Find where tryAddPackage is called in register.ts
rg -n 'tryAddPackage' packages/intent/src/discovery/register.ts -B2 -A2Repository: TanStack/intent
Length of output: 41
🏁 Script executed:
# Get the full scanPackages function to see how processPackage is called
sed -n '80,180p' packages/intent/src/library-scanner.tsRepository: TanStack/intent
Length of output: 2846
🏁 Script executed:
# Find the resolveDepDir function
rg -n 'function resolveDepDir|const resolveDepDir' packages/intent/src/library-scanner.ts -A 20Repository: TanStack/intent
Length of output: 41
🏁 Script executed:
# Also check if there's a utility file with path resolution
fd -e ts packages/intent/src | xargs rg -l 'resolveDepDir|resolve.*[Dd]ir'Repository: TanStack/intent
Length of output: 993
🏁 Script executed:
# Search for resolveDepDir definition in library-scanner.ts
rg -n 'resolveDepDir' packages/intent/src/library-scanner.ts -B5 -A15Repository: TanStack/intent
Length of output: 1193
🏁 Script executed:
# Check imports in library-scanner.ts
head -30 packages/intent/src/library-scanner.tsRepository: TanStack/intent
Length of output: 885
🏁 Script executed:
# Find resolveDepDir in utils.ts
rg -n 'function resolveDepDir|const resolveDepDir|export.*resolveDepDir' packages/intent/src/utils.ts -A 20Repository: TanStack/intent
Length of output: 899
🏁 Script executed:
# Look at the test cases to understand intended behavior
cat packages/intent/tests/skill-paths.test.tsRepository: TanStack/intent
Length of output: 4874
🏁 Script executed:
# Check if there's a test that covers the scenario: pnpm path exists AND top-level path exists
grep -n "node_modules/@tanstack" packages/intent/tests/skill-paths.test.ts | head -20Repository: TanStack/intent
Length of output: 615
🏁 Script executed:
# Verify the exact directory structure created in the pnpm test
sed -n '103,130p' packages/intent/tests/skill-paths.test.tsRepository: TanStack/intent
Length of output: 811
🏁 Script executed:
# Create a test to verify the actual issue: pnpm path + top-level path both exist
cat > /tmp/test-scenario.js << 'EOF'
// Simulate the scenario:
// 1. packageRoot is in .pnpm/
// 2. Top-level node_modules/@tanstack/query exists
// 3. Current code checks only existsSync(top-level), not if it matches packageRoot
const packageRoot = 'node_modules/.pnpm/@tanstack+query@5.0.0/node_modules/@tanstack/query'
const topLevelPath = 'node_modules/@tanstack/query'
// Current logic in rewriteSkillLoadPaths
const hasStableSymlink = packageRoot !== '' && true // assume topLevelPath exists
console.log('packageRoot:', packageRoot)
console.log('topLevelPath exists:', true)
console.log('hasStableSymlink (current logic):', hasStableSymlink)
console.log('Would incorrectly rewrite to top-level:', hasStableSymlink && packageRoot !== topLevelPath)
// What the review suggests:
const hasPackageManagerInternalPath = packageRoot.includes('.pnpm')
console.log('\nWith proposed check:')
console.log('hasPackageManagerInternalPath:', hasPackageManagerInternalPath)
console.log('Should skip rewrite:', hasPackageManagerInternalPath)
EOF
node /tmp/test-scenario.jsRepository: TanStack/intent
Length of output: 335
🏁 Script executed:
# Check if there's a test that covers BOTH: pnpm packageRoot AND top-level path existing
grep -A 30 "does not invent stable top-level" packages/intent/tests/skill-paths.test.ts | grep -E "(mkdirSync|join|node_modules)"Repository: TanStack/intent
Length of output: 394
Only rewrite through the top-level package path when packageRoot is a standard top-level package.
Line 54 checks only that a top-level node_modules/<packageName> exists, without verifying that the scanned packageRoot is actually that top-level path. If packageRoot is a pnpm internal path (e.g., node_modules/.pnpm/@tanstack+query@5.0.0/node_modules/@tanstack/query) while a top-level node_modules/@tanstack/query also exists, the code incorrectly rewrites skills to the top-level version, potentially loading SKILL.md from the wrong package version.
Add a check to prevent rewriting when packageRoot contains package manager internals:
const hasStableSymlink =
packageName !== '' &&
- existsSync(join(projectRoot, 'node_modules', packageName))
+ existsSync(join(projectRoot, 'node_modules', packageName)) &&
+ !hasPackageManagerInternalPath(packageRoot)The file already exports hasPackageManagerInternalPath(), which detects .pnpm, .bun, and .yarn segments. Note: The test "does not invent stable top-level load paths for pnpm-internal package paths" only covers the case where the top-level path does not exist, so it does not catch this issue.
📝 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
| const hasStableSymlink = | |
| packageName !== '' && | |
| existsSync(join(projectRoot, 'node_modules', packageName)) | |
| for (const skill of skills) { | |
| if (hasStableSymlink) { | |
| const relFromPackage = toPosixPath(relative(packageRoot, skill.path)) | |
| skill.path = `node_modules/${packageName}/${relFromPackage}` | |
| const hasStableSymlink = | |
| packageName !== '' && | |
| existsSync(join(projectRoot, 'node_modules', packageName)) && | |
| !hasPackageManagerInternalPath(packageRoot) | |
| for (const skill of skills) { | |
| if (hasStableSymlink) { | |
| const relFromPackage = toPosixPath(relative(packageRoot, skill.path)) | |
| skill.path = `node_modules/${packageName}/${relFromPackage}` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/skill-paths.ts` around lines 52 - 59, The rewrite
currently uses hasStableSymlink to decide whether to rewrite skill.path to
node_modules/<packageName>/..., but it doesn't verify that packageRoot is a
top-level package directory; update the condition so we only rewrite when
hasStableSymlink is true AND hasPackageManagerInternalPath(packageRoot) returns
false. In practice: inside the loop or before altering skill.path (the block
that uses toPosixPath(relative(packageRoot, skill.path)) and assigns `skill.path
= node_modules/${packageName}/${relFromPackage}`), add a guard that calls
hasPackageManagerInternalPath(packageRoot) and skips the rewrite when it detects
pnpm/.bun/.yarn internals, so we don't invent top-level load paths for internal
package roots.
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>#<skill>mappings so generated agent config storeswhen/useinstead of path-heavytask/loadentriesintent resolve <use>so agents can resolve mappings at runtimelist,install, andresolve, with explicit--globaland--global-onlyflags for global packagesintent resolve, explicit global scanning, and the new install behaviorNotes
intent installnow writes or updates the managed block directly and verifies it before reporting successAGENTS.mdno longer stores absolute paths,.pnpm/.bun/.yarnpaths, or generatedloadentriesSummary by CodeRabbit
Release Notes
New Features
intent load <package>#<skill>command to resolve and load specific skills with--path,--json,--global, and--global-onlyoptions.--globaland--global-onlyflags tointent listandintent installcommands; global packages now require explicit flags (local scanning is default).intent installwith--map(explicit mappings),--dry-run, and--print-promptoptions.Documentation