feat: add compact skill resolution for install mappings#115
Open
LadyBluenotes wants to merge 29 commits intomainfrom
Open
feat: add compact skill resolution for install mappings#115LadyBluenotes wants to merge 29 commits intomainfrom
LadyBluenotes wants to merge 29 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Parser as Skill Use Parser
participant Scanner
participant Resolver
participant FS as "File System"
User->>CLI: npx `@tanstack/intent` resolve <package>#<skill> [--json/--global]
CLI->>Parser: parseSkillUse(<package>#<skill>)
Parser-->>CLI: { packageName, skillName }
CLI->>Scanner: scanIntentsOrFail(scanOptionsFromGlobalFlags)
Scanner->>FS: scan local/global packages
Scanner-->>CLI: ScanResult
CLI->>Resolver: resolveSkillUse(use, ScanResult)
Resolver->>Resolver: choose package (prefer local), find skill, filter warnings/conflicts
alt found
Resolver-->>CLI: ResolveSkillResult (path, source, version, warnings)
alt --json
CLI->>User: print JSON (package, skill, path, version, warnings)
else
CLI->>User: print path
CLI->>User: print warnings to stderr
end
else not found
Resolver-->>CLI: ResolveSkillUseError
CLI->>User: print error message
end
sequenceDiagram
participant User
participant CLI
participant Scanner
participant InstallWriter
participant FS as "File System"
User->>CLI: npx `@tanstack/intent` install [--dry-run/--print-prompt/--global]
CLI->>Scanner: scanIntentsOrFail(scanOptionsFromGlobalFlags)
Scanner->>FS: scan packages (local, maybe global)
Scanner-->>CLI: ScanResult
CLI->>InstallWriter: buildIntentSkillsBlock(ScanResult)
InstallWriter-->>CLI: block (intent-skills YAML)
alt --dry-run
CLI->>User: print generated block (no file writes)
else
CLI->>InstallWriter: resolveIntentSkillsBlockTargetPath(root, mappingCount)
InstallWriter->>FS: read/locate target file
InstallWriter->>FS: write or replace managed block (create/update/unchanged)
InstallWriter-->>CLI: write result
CLI->>InstallWriter: verifyIntentSkillsBlockFile(...)
InstallWriter->>FS: read and validate block content
InstallWriter-->>CLI: verification result
alt verification ok
CLI->>User: print success message
else
CLI->>User: print error and fail
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 d1e0452
☁️ 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
| ## JSON output | ||
|
|
||
| `--json` prints the `ScanResult` object: | ||
| `REQUIRES` uses `intent.requires` values joined by `, `; empty values render as `–`. |
Contributor
There was a problem hiding this comment.
Fix markdownlint MD038: spaces inside code span.
The inline code span `, ` contains leading/trailing whitespace, which markdownlint flags as MD038. Either escape the space or rephrase.
📝 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 `–`.📝 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
| `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 `–`. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 29-29: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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 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.
Comment on lines
+16
to
+22
| This creates or updates an `intent-skills` block. It: | ||
|
|
||
| 1. Check for existing `intent-skills` mappings in your config files (`AGENTS.md`, `CLAUDE.md`, `.cursorrules`, etc.) | ||
| 2. Run a local package scan to discover available skills from installed packages | ||
| 3. Write compact mappings for actionable skills | ||
| 4. Preserve content outside the managed block | ||
| 5. Verify the managed block before reporting success |
Contributor
There was a problem hiding this comment.
Grammar: use third-person singular verbs after "It:".
The sentence "This creates or updates an intent-skills block. It:" introduces a list, but the items begin with imperative verbs. They should agree with the subject ("It").
📝 Proposed fix
-1. Check for existing `intent-skills` mappings in your config files (`AGENTS.md`, `CLAUDE.md`, `.cursorrules`, etc.)
-2. Run a local package scan to discover available skills from installed packages
-3. Write compact mappings for actionable skills
-4. Preserve content outside the managed block
-5. Verify the managed block before reporting success
+1. Checks for existing `intent-skills` mappings in your config files (`AGENTS.md`, `CLAUDE.md`, `.cursorrules`, etc.)
+2. Runs a local package scan to discover available skills from installed packages
+3. Writes compact mappings for actionable skills
+4. Preserves content outside the managed block
+5. Verifies the managed block before reporting success📝 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
| This creates or updates an `intent-skills` block. It: | |
| 1. Check for existing `intent-skills` mappings in your config files (`AGENTS.md`, `CLAUDE.md`, `.cursorrules`, etc.) | |
| 2. Run a local package scan to discover available skills from installed packages | |
| 3. Write compact mappings for actionable skills | |
| 4. Preserve content outside the managed block | |
| 5. Verify the managed block before reporting success | |
| This creates or updates an `intent-skills` block. It: | |
| 1. Checks for existing `intent-skills` mappings in your config files (`AGENTS.md`, `CLAUDE.md`, `.cursorrules`, etc.) | |
| 2. Runs a local package scan to discover available skills from installed packages | |
| 3. Writes compact mappings for actionable skills | |
| 4. Preserves content outside the managed block | |
| 5. Verifies the managed block before reporting success |
🤖 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 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.
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 15.6%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | scans a consumer workspace |
67.5 ms | 58.4 ms | +15.6% |
Comparing install-prompt (d1e0452) 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
+13
to
+22
| import { | ||
| runInstallCommand, | ||
| type InstallCommandOptions, | ||
| } from './commands/install.js' | ||
| import { runListCommand, type ListCommandOptions } from './commands/list.js' | ||
| import { runMetaCommand } from './commands/meta.js' | ||
| import { | ||
| runResolveCommand, | ||
| type ResolveCommandOptions, | ||
| } from './commands/resolve.js' |
Contributor
There was a problem hiding this comment.
Split command option types into type-only imports.
ESLint flags the inline type specifiers in these imports.
🧹 Proposed fix
-import {
- runInstallCommand,
- type InstallCommandOptions,
-} from './commands/install.js'
-import { runListCommand, type ListCommandOptions } from './commands/list.js'
+import { runInstallCommand } from './commands/install.js'
+import type { InstallCommandOptions } from './commands/install.js'
+import { runListCommand } from './commands/list.js'
+import type { ListCommandOptions } from './commands/list.js'
import { runMetaCommand } from './commands/meta.js'
-import {
- runResolveCommand,
- type ResolveCommandOptions,
-} from './commands/resolve.js'
+import { runResolveCommand } from './commands/resolve.js'
+import type { ResolveCommandOptions } from './commands/resolve.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 { | |
| runInstallCommand, | |
| type InstallCommandOptions, | |
| } from './commands/install.js' | |
| import { runListCommand, type ListCommandOptions } from './commands/list.js' | |
| import { runMetaCommand } from './commands/meta.js' | |
| import { | |
| runResolveCommand, | |
| type ResolveCommandOptions, | |
| } from './commands/resolve.js' | |
| import { runInstallCommand } from './commands/install.js' | |
| import type { InstallCommandOptions } from './commands/install.js' | |
| import { runListCommand } from './commands/list.js' | |
| import type { ListCommandOptions } from './commands/list.js' | |
| import { runMetaCommand } from './commands/meta.js' | |
| import { runResolveCommand } from './commands/resolve.js' | |
| import type { ResolveCommandOptions } from './commands/resolve.js' |
🧰 Tools
🪛 ESLint
[error] 15-15: Member 'InstallCommandOptions' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 15-15: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 17-17: Member 'ListCommandOptions' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 17-17: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 21-21: Member 'ResolveCommandOptions' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 21-21: 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/cli.ts` around lines 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.
Comment on lines
+3
to
+14
| import { | ||
| printWarnings, | ||
| scanOptionsFromGlobalFlags, | ||
| type GlobalScanFlags, | ||
| } from '../cli-support.js' | ||
| import type { ScanOptions, ScanResult } from '../types.js' | ||
| import { | ||
| buildIntentSkillsBlock, | ||
| resolveIntentSkillsBlockTargetPath, | ||
| verifyIntentSkillsBlockFile, | ||
| writeIntentSkillsBlock, | ||
| } from './install-writer.js' |
Contributor
There was a problem hiding this comment.
Split type imports and keep value imports ordered.
ESLint flags the inline type GlobalScanFlags import and the current ordering around ./install-writer.js.
🧹 Proposed fix
import { fail } from '../cli-error.js'
import {
printWarnings,
scanOptionsFromGlobalFlags,
- type GlobalScanFlags,
} from '../cli-support.js'
-import type { ScanOptions, ScanResult } from '../types.js'
+import type { GlobalScanFlags } from '../cli-support.js'
import {
buildIntentSkillsBlock,
resolveIntentSkillsBlockTargetPath,
verifyIntentSkillsBlockFile,
writeIntentSkillsBlock,
} from './install-writer.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 { | |
| printWarnings, | |
| scanOptionsFromGlobalFlags, | |
| type GlobalScanFlags, | |
| } from '../cli-support.js' | |
| import type { ScanOptions, ScanResult } from '../types.js' | |
| import { | |
| buildIntentSkillsBlock, | |
| resolveIntentSkillsBlockTargetPath, | |
| verifyIntentSkillsBlockFile, | |
| writeIntentSkillsBlock, | |
| } from './install-writer.js' | |
| import { | |
| printWarnings, | |
| scanOptionsFromGlobalFlags, | |
| } from '../cli-support.js' | |
| import type { GlobalScanFlags } from '../cli-support.js' | |
| import { | |
| buildIntentSkillsBlock, | |
| resolveIntentSkillsBlockTargetPath, | |
| verifyIntentSkillsBlockFile, | |
| writeIntentSkillsBlock, | |
| } from './install-writer.js' | |
| import type { ScanOptions, ScanResult } from '../types.js' |
🧰 Tools
🪛 ESLint
[error] 6-6: Member 'GlobalScanFlags' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 6-6: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 9-14: ./install-writer.js import should occur before type import of ../types.js
(import/order)
🤖 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 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.
Comment on lines
40
to
49
| 2. DISCOVER AVAILABLE SKILLS | ||
| Run: \`npx @tanstack/intent@latest list\` | ||
| This outputs each skill's name, description, full path, and whether it was found in | ||
| project-local node_modules or accessible global node_modules. | ||
| This scans project-local node_modules by default and outputs each package and skill's name, | ||
| description, and source. | ||
| If the user explicitly wants globally installed skills included, run: | ||
| \`npx @tanstack/intent@latest list --global\` | ||
| This works best in Node-compatible environments (npm, pnpm, Bun, or Deno npm interop | ||
| with node_modules enabled). | ||
| If no skills are found, do not create a config file. Report: "No intent-enabled skills found." | ||
|
|
Contributor
There was a problem hiding this comment.
Use the same no-actionable-skills message as the command.
The prompt tells agents to report No intent-enabled skills found., but runInstallCommand and the docs report No actionable intent skills found.. Keeping these aligned avoids inconsistent user-facing install results.
📝 Proposed fix
- If no skills are found, do not create a config file. Report: "No intent-enabled skills found."
+ If no actionable skills are found, do not create a config file. Report: "No actionable intent skills found."🤖 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 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.
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
+1
to
+5
| import { existsSync } from 'node:fs' | ||
| import { join, relative } from 'node:path' | ||
| import type { SkillUse } from './skill-use.js' | ||
| import type { SkillEntry } from './types.js' | ||
| import { toPosixPath } from './utils.js' |
Contributor
There was a problem hiding this comment.
Move the value import before type-only imports.
ESLint reports ./utils.js should be ordered before the local type imports.
🧹 Proposed fix
import { existsSync } from 'node:fs'
import { join, relative } from 'node:path'
+import { toPosixPath } from './utils.js'
import type { SkillUse } from './skill-use.js'
import type { SkillEntry } from './types.js'
-import { toPosixPath } from './utils.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 { existsSync } from 'node:fs' | |
| import { join, relative } from 'node:path' | |
| import type { SkillUse } from './skill-use.js' | |
| import type { SkillEntry } from './types.js' | |
| import { toPosixPath } from './utils.js' | |
| import { existsSync } from 'node:fs' | |
| import { join, relative } from 'node:path' | |
| import { toPosixPath } from './utils.js' | |
| import type { SkillUse } from './skill-use.js' | |
| import type { SkillEntry } from './types.js' |
🧰 Tools
🪛 ESLint
[error] 5-5: ./utils.js import should occur before type import of ./skill-use.js
(import/order)
🤖 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 - 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'".
Comment on lines
+10
to
+16
| export function isAbsolutePath(path: string): boolean { | ||
| return ( | ||
| path.startsWith('/') || | ||
| path.startsWith('\\\\') || | ||
| /^[A-Za-z]:[\\/]/.test(path) | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Node treats single-backslash Windows paths as absolute.
node - <<'NODE'
const path = require('node:path')
for (const candidate of ['\\Users\\alice\\repo\\SKILL.md', '\\temp\\SKILL.md', '\\\\server\\share\\SKILL.md', 'C:\\repo\\SKILL.md']) {
console.log(candidate, path.win32.isAbsolute(candidate))
}
NODERepository: TanStack/intent
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Find and examine isStableLoadPath function
rg "isStableLoadPath" --type ts -B 2 -A 10Repository: TanStack/intent
Length of output: 4964
🏁 Script executed:
#!/bin/bash
# Look for tests related to isAbsolutePath
fd ".*\.test\." --type ts | xargs rg "isAbsolutePath" -lRepository: TanStack/intent
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Check for any other usages of the double-backslash pattern
rg "startsWith\('\\\\\\\\'\)" --type tsRepository: TanStack/intent
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Read the test file to check coverage
cat -n packages/intent/tests/skill-paths.test.tsRepository: TanStack/intent
Length of output: 5859
Fix single-backslash Windows root-relative path detection.
The function currently rejects only double-backslash UNC paths (\\\\server\\share) but misses single-backslash Windows root-relative paths like \Users\alice\repo\SKILL.md, which Node.js treats as absolute. This lets machine-specific Windows paths through isStableLoadPath() incorrectly.
Proposed fix
export function isAbsolutePath(path: string): boolean {
return (
path.startsWith('/') ||
- path.startsWith('\\\\') ||
+ path.startsWith('\\') ||
/^[A-Za-z]:[\\/]/.test(path)
)
}📝 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 isAbsolutePath(path: string): boolean { | |
| return ( | |
| path.startsWith('/') || | |
| path.startsWith('\\\\') || | |
| /^[A-Za-z]:[\\/]/.test(path) | |
| ) | |
| } | |
| export function isAbsolutePath(path: string): boolean { | |
| return ( | |
| path.startsWith('/') || | |
| path.startsWith('\\') || | |
| /^[A-Za-z]:[\\/]/.test(path) | |
| ) | |
| } |
🤖 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 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).
| @@ -0,0 +1,263 @@ | |||
| import { describe, expect, it } from 'vitest' | |||
| import { resolveSkillUse, ResolveSkillUseError } from '../src/resolver.js' | |||
Contributor
There was a problem hiding this comment.
Sort the resolver import members.
ESLint expects ResolveSkillUseError before resolveSkillUse.
🧹 Proposed fix
-import { resolveSkillUse, ResolveSkillUseError } from '../src/resolver.js'
+import { ResolveSkillUseError, resolveSkillUse } from '../src/resolver.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 { resolveSkillUse, ResolveSkillUseError } from '../src/resolver.js' | |
| import { ResolveSkillUseError, resolveSkillUse } from '../src/resolver.js' |
🧰 Tools
🪛 ESLint
[error] 2-2: Member 'ResolveSkillUseError' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/tests/resolver.test.ts` at 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.
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.
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
New Features
intent resolveto turn compactpackage#skillreferences into resolved skill paths.install/listnow support--dry-run,--print-prompt,--global, and--global-only(global scanning is opt-in; dry-run previews mappings).installnow creates/updates a validated compactintent-skillsmanaged block (useswhen/usewith<package>#<skill>).Documentation
Tests