feat: add compact skill resolution for install mappings#115
Open
LadyBluenotes wants to merge 27 commits intomainfrom
Open
feat: add compact skill resolution for install mappings#115LadyBluenotes wants to merge 27 commits intomainfrom
LadyBluenotes wants to merge 27 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Scanner
participant InstallWriter
participant FileSystem as File System
User->>CLI: npx intent install [flags]
CLI->>Scanner: scanIntentsOrFail(scope)
Scanner->>FileSystem: scan local packages
Scanner->>FileSystem: scan global packages (if enabled)
Scanner-->>CLI: ScanResult
CLI->>InstallWriter: buildIntentSkillsBlock(scanResult)
InstallWriter-->>CLI: block (when/use mappings)
alt --dry-run
CLI->>User: print generated block
else write to file
CLI->>InstallWriter: writeIntentSkillsBlock(block, root)
InstallWriter->>FileSystem: locate/create config file
InstallWriter->>FileSystem: write managed block
InstallWriter-->>CLI: result (created/updated/unchanged)
CLI->>InstallWriter: verifyIntentSkillsBlockFile(...)
InstallWriter->>FileSystem: read and validate block
InstallWriter-->>CLI: verification result
alt verification ok
CLI->>User: print success message
else verification failed
CLI->>User: print error and fail
end
end
sequenceDiagram
participant User
participant CLI
participant Parser as Skill Use Parser
participant Scanner
participant Resolver
participant FileSystem as File System
User->>CLI: npx intent resolve `@pkg`#skill [flags]
CLI->>Parser: parseSkillUse(`@pkg`#skill)
Parser-->>CLI: {packageName, skillName}
CLI->>Scanner: scanIntentsOrFail(scope)
Scanner->>FileSystem: scan packages (local, global, or both)
Scanner-->>CLI: ScanResult
CLI->>Resolver: resolveSkillUse(use, scanResult)
Resolver->>Resolver: select package (prefer local)
Resolver->>Resolver: find skill in package
Resolver->>Resolver: lookup conflicts/warnings
alt skill found
Resolver-->>CLI: ResolveSkillResult
alt --json
CLI->>User: print JSON (package, path, version, warnings)
else human readable
CLI->>User: print path
CLI->>User: print warnings to stderr
end
else skill not found
Resolver-->>CLI: ResolveSkillUseError
CLI->>User: print error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 3b0ae0c
☁️ 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
+109
to
+111
| warnings: scanResult.warnings.filter((warning) => { | ||
| return warning.includes(packageName) | ||
| }), |
Contributor
There was a problem hiding this comment.
Substring-based warning filter risks false positives/negatives.
warning.includes(packageName) can match unrelated warnings that happen to contain the package name as a substring (e.g., a warning about react-router would match packageName === 'react'), and conversely can miss warnings that reference the package via a normalized or alternate form. If ScanResult.warnings entries carry a structured packageName field, prefer matching on that; otherwise consider word-boundary matching to reduce accidental matches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/resolver.ts` around lines 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.
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.61%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | scans a consumer workspace |
67.5 ms | 58.3 ms | +15.61% |
Comparing install-prompt (3b0ae0c) with main (6069ffc)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<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 resolvecommand to convert skill references into file paths--dry-run,--print-prompt,--global, and--global-onlyflags to install and list commandswhen/useformatDocumentation