feat: consumer installer with per-skill lockfile and managed delivery#204
feat: consumer installer with per-skill lockfile and managed delivery#204LadyBluenotes wants to merge 36 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds lockfile-backed skill integrity checks, cached session catalogues and hooks, interactive installation and synchronization workflows, managed link state, strict configuration handling, cross-platform delivery tests, and extensive unit, integration, and benchmark coverage. ChangesIntent workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
View your CI Pipeline Execution ↗ for commit 1746da4
☁️ Nx Cloud last updated this comment at |
commit: |
LadyBluenotes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (17)
packages/intent/src/core/lockfile/lockfile-state.ts (1)
17-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the suffix-matching heuristic in
packageRelativeSkillFile.The
forloop that walkspackageSegmentslooking for the longest suffix matching a prefix ofskillSegmentsis correct for the tested shapes (barenode_modules/<pkg>/...and full workspace-relative paths) but is non-obvious. Since this function determines the canonical skill path baked intointent.lock(the security-relevant acceptance record), a short comment explaining why it scans fromstart=0upward (to prefer the longest/most-specific suffix match) would reduce the risk of a subtly wrong edit later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/core/lockfile/lockfile-state.ts` around lines 17 - 52, Document the suffix-matching heuristic in packageRelativeSkillFile immediately above the loop: explain that scanning packageSegments from start=0 upward checks progressively shorter suffixes while preferring the longest, most-specific suffix matching the skillSegments prefix, preserving canonical lockfile path resolution.packages/intent/src/core/lockfile/lockfile.ts (1)
156-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
readIntentLockfilebypasses the injectable-fs pattern used elsewhere in this PR.Unlike
computeSkillContentHash,buildCurrentLockfileSources, andscanIntentPackageAtRoot, which all accept aReadFs/fsCache,readIntentLockfilecallsreadFileSyncfromnode:fsdirectly. Its two call sites —intent-core.ts'stoResolvedIntentSkillandcatalog-lock.ts'sapplyCatalogueLock— both otherwise thread an injected/cachedreadFsthrough the rest of the same function, so this is the one lockfile read that always hits real disk regardless of caching. Consider accepting an optionalfs/readFsparameter (defaulting to realreadFileSync) for consistency and to avoid an uncached disk read + JSON parse on every skill resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/core/lockfile/lockfile.ts` around lines 156 - 167, Update readIntentLockfile to accept the injected ReadFs/fsCache reader used by computeSkillContentHash, buildCurrentLockfileSources, and scanIntentPackageAtRoot, while defaulting to the real filesystem reader for existing callers. Thread the available readFs through intent-core.ts’s toResolvedIntentSkill and catalog-lock.ts’s applyCatalogueLock so lockfile reads use the same cache instead of direct readFileSync.packages/intent/tests/core.test.ts (1)
541-571: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
resolveIntentSkillassertion for parity with the sibling tests.The two adjacent tests (content-hash mismatch, missing lock entry) both assert
loadIntentSkillandresolveIntentSkillthrow identically. This test only checksloadIntentSkill, leaving the wrong-source-kind path unverified forresolveIntentSkill, even though both share the sametoResolvedIntentSkillcheck.✅ Proposed addition
expect(() => loadIntentSkill('`@tanstack/query`#fetching', { cwd: root }), ).toThrow( 'Cannot load skill use "`@tanstack/query`#fetching": skill is not accepted in intent.lock.', ) + expect(() => + resolveIntentSkill('`@tanstack/query`#fetching', { cwd: root }), + ).toThrow( + 'Cannot load skill use "`@tanstack/query`#fetching": skill is not accepted in intent.lock.', + ) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/core.test.ts` around lines 541 - 571, Extend the wrong-source-kind test to also assert that resolveIntentSkill rejects the same `@tanstack/query`#fetching request with the identical intent.lock error, matching the adjacent content-hash and missing-entry tests. Keep the existing loadIntentSkill assertion unchanged and reuse the same root and fixture setup.packages/intent/src/discovery/scanner.ts (1)
169-171: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize per root — each call re-executes the PnP runtime.
loadPnpApirequires and runs.pnp.cjssetup(), which patches the globalfsmodule process-wide.getProjectReadFsis invoked at least once fromgetIntentCatalogContextand again as the default argument inapplyCatalogueLock, so a singleintent catalogrun can load and set up the PnP runtime twice. The scanner already caches this lazily via its internalpnpApivariable; a small module-levelMap<string, ReadFs>here would give the same guarantee to external callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/discovery/scanner.ts` around lines 169 - 171, Memoize getProjectReadFs results per root to avoid repeated PnP runtime setup. Add a module-level Map<string, ReadFs>, return the cached filesystem when available, and cache either the loaded PnP readFs or nodeReadFs before returning from getProjectReadFs.packages/intent/src/core/source-policy.ts (1)
199-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscovery warnings are dropped here.
scanForIntentsreturnswarnings(invalidintentfields, version conflicts) whichscanForConfiguredIntentsdiscards, sointent synccan silently ignore real discovery problems while only surfacingpolicy.notices. Consider returningwarningsalongsidediscovered/policyso the sync output can include them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/core/source-policy.ts` around lines 199 - 207, Update scanForConfiguredIntents to preserve and return the warnings from scanForIntents alongside discovered and policy. Ensure the intent sync output can consume these discovery warnings in addition to policy.notices, without filtering or replacing the existing warning details.packages/intent/tests/cli.test.ts (1)
291-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear
logSpybetween lifecycle phases.Each assertion joins the cumulative call buffer, so output emitted by an earlier
main(['sync'])in this test keeps satisfying latertoContainchecks. If a phase stops printing its block, the test can still pass. AddlogSpy.mockClear()immediately before eachmain(['sync'])whose output you then assert.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/cli.test.ts` around lines 291 - 338, Clear the cumulative log buffer before each lifecycle phase in the test’s sync flow: call logSpy.mockClear() immediately before every main(['sync']) invocation whose output is subsequently asserted. Keep the existing assertions and sync behavior unchanged while ensuring each check validates only that phase’s log output.packages/intent/src/session-catalog.ts (2)
310-317: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the upward walk.
while (directory !== workspaceRoot)relies on exact string equality to terminate; any normalization drift betweenpolicyRootandworkspaceRoot(trailing separator, case on Windows) turns this into an infinite loop, sincedirnamebecomes a fixed point at the filesystem root. Add a depth/dirnamefixed-point guard.🛡️ Proposed guard
const manifests: Array<string> = [] let directory = policyRoot while (directory !== workspaceRoot) { manifests.push(join(directory, 'package.json')) - directory = dirname(directory) + const parent = dirname(directory) + if (parent === directory) break + directory = parent }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/session-catalog.ts` around lines 310 - 317, Bound the upward manifest walk by updating the loop around directory and workspaceRoot to stop when the current directory reaches workspaceRoot or dirname(directory) becomes unchanged at the filesystem root. Preserve collecting package.json for each traversed directory and avoid adding entries indefinitely when path normalization differs.
249-266: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFingerprint reads full lockfile contents on every hook invocation.
hash.update(readFileSync(file))slurps every lockfile (pnpm-lock.yaml,package-lock.json, …) plus every workspacepackage.jsonon eachgetSessionCataloguecall, including cache hits. In a large monorepo that is tens of MB of I/O inside a session-start hook that has a 10s timeout. Hashingsize+mtimeMsfromstatSync(falling back to content only when needed) would give the same invalidation signal far more cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/session-catalog.ts` around lines 249 - 266, Update the fingerprinting loop in the session-catalog fingerprint function to avoid reading full lockfiles and workspace package.json files on every invocation. Use each file’s stat metadata, including size and modification time, as the primary hash input, and fall back to hashing file contents only when stat metadata cannot be obtained; preserve the existing missing-file handling and deterministic path ordering.packages/intent/tests/catalog-api.test.ts (1)
72-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests leak session-cache files into the OS temp dir.
getIntentCatalogContextwrites to the defaultos.tmpdir()/tanstack-intent/cataloguesandafterEachonly removes the fixture roots, so every run leaves orphaned cache JSON behind. Threading acacheDiroption throughgetIntentCatalogContext(already supported bygetSessionCatalogue) would let these tests point at a fixture-local directory and clean up deterministically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/catalog-api.test.ts` around lines 72 - 77, Update the test setup around getIntentCatalogContext to provide a fixture-local cacheDir, using the existing roots-based temporary directory rather than the default OS temp location. Ensure the option is threaded through the context creation to getSessionCatalogue, and retain the afterEach cleanup so the cache files are removed with the fixture roots.packages/intent/tests/lockfile.test.ts (1)
73-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on the rejection reason, not just that something throws.
All seven cases use bare
toThrow(), so a parse failure for an unrelated reason (or a future message change that collapses two distinct validations into one) still passes. Since this is the fail-closed path for lockfile integrity, match the expected messages, e.g.toThrow(/source kind/)for the"kind":"git"case and/duplicate/ifor the duplicate id/path cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/lockfile.test.ts` around lines 73 - 103, Update the rejection assertions in the lockfile validation test to match each case’s expected error message, rather than using bare toThrow() calls. Anchor the expectations to the relevant validation symbols and messages: unknown top-level fields, unsupported lockfileVersion, invalid sources type, invalid source kind, duplicate source identity, duplicate skill paths, and traversal paths should each assert a targeted message; use patterns such as /source kind/ and /duplicate/i where appropriate.packages/intent/src/hooks/install.ts (2)
428-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
isIntentGateScriptReferencenow that it matches catalog scripts too.The predicate no longer identifies only gate scripts;
isIntentHookScriptReferencereflects the widened regex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/hooks/install.ts` around lines 428 - 437, Rename isIntentGateScriptReference to isIntentHookScriptReference throughout its declaration and all call sites, preserving the existing regex and behavior that matches both gate and catalog scripts.
354-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid materializing an empty
PreToolUsekey in agent configs.
removeIntentHooks(arrayValue(hooks.PreToolUse))always assigns an array, so a config that never hadPreToolUsegains"PreToolUse": [](and a user whose only Intent hook is removed is left with an empty array). Drop the key when the result is empty to keep the written config clean.♻️ Proposed cleanup helper
- hooks.PreToolUse = removeIntentHooks(arrayValue(hooks.PreToolUse)) + const preToolUse = removeIntentHooks(arrayValue(hooks.PreToolUse)) + if (preToolUse.length > 0) hooks.PreToolUse = preToolUse + else delete hooks.PreToolUseAlso applies to: 377-377, 389-390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/hooks/install.ts` at line 354, Update the hook cleanup assignments around hooks.PreToolUse so empty results do not create or retain a PreToolUse key; only assign the filtered array when non-empty, otherwise remove the property. Apply the same behavior to the additional affected hook-processing paths while preserving existing handling for non-empty hooks.packages/intent/src/commands/sync/gitignore.ts (1)
12-15: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the block matcher to a module constant.
START/ENDare static, so the escaping andRegExpconstruction can be computed once instead of on every call. (The ReDoS hint from static analysis is a false positive here — the pattern derives from constants and is escaped.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/commands/sync/gitignore.ts` around lines 12 - 15, Hoist the escaped START/END block matcher from the current function into a module-level constant, preserving the existing non-greedy pattern and replacement behavior in the sync logic. Remove the per-call RegExp construction while continuing to use the shared matcher for prefix replacement.Source: Linters/SAST tools
packages/intent/src/commands/sync/command.ts (1)
90-97: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
writeTextFileAtomicfor the.gitignoreupdate.Every other managed file in this module (
package.json, install state) is written atomically; a plainwriteFileSynchere can truncate a user's.gitignoreif the process dies mid-write.♻️ Proposed fix
- writeFileSync(path, after, 'utf8') + writeTextFileAtomic(path, after) return true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/commands/sync/command.ts` around lines 90 - 97, Update writeGitignore to replace the direct writeFileSync call with the module’s existing writeTextFileAtomic helper, preserving the current before/after comparison and boolean return behavior.packages/intent/src/commands/sync/prepare.ts (2)
16-20: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueIdempotency only holds for
&&-chained scripts.A
preparevalue like"build; intent sync"or"pnpm exec intent sync"won't be detected, so a second wiring appends a duplicate&& intent sync. Splitting on[;&]+(and optionally allowing a runner prefix) would cover the common shapes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/commands/sync/prepare.ts` around lines 16 - 20, Update containsIntentSync to recognize intent sync commands separated by both && and semicolons, and allow common runner prefixes such as pnpm exec before intent sync. Preserve whitespace handling and ensure these command forms are detected so repeated wiring remains idempotent.
33-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNon-string existing
prepareis silently overwritten.If
scripts.prepareis present but not a string (malformed manifest), the edit replaces it with"intent sync"without warning. Consider failing loudly like the parse-error path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/commands/sync/prepare.ts` around lines 33 - 42, Update the prepare-edit logic around scripts.prepare to detect a present non-string value and fail loudly using the same error behavior as the parse-error path, rather than replacing it with "intent sync". Preserve the existing handling for missing, string, and intent-sync-containing prepare values.packages/intent/package.json (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPut
typesbeforeimportin the new./catalogcondition block.Conditional exports are matched in declaration order; TypeScript's
node16/bundlerresolvers can matchimportfirst and only findcatalog.d.mtsvia sibling-name fallback. Orderingtypesfirst makes declaration resolution explicit and consistent with the other subpaths.♻️ Proposed ordering fix
"./catalog": { - "import": "./dist/catalog.mjs", - "types": "./dist/catalog.d.mts" + "types": "./dist/catalog.d.mts", + "import": "./dist/catalog.mjs" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/package.json` around lines 20 - 22, Reorder the conditions in the "./catalog" export block so the existing "types" entry appears before "import", matching the ordering used by other subpaths and ensuring declaration resolution selects catalog.d.mts explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/intent/src/commands/sync/links.ts`:
- Around line 96-104: Update removeLink to report removal failure as a
non-throwing result, allowing reconcileManagedLinks callers at the removal sites
to add entry.path to conflicts when unlinkSync and rmdirSync both fail. Preserve
successful handling for file links and directory symlinks, and ensure
reconciliation continues so install state can still be written.
In `@packages/intent/src/commands/sync/prompts.ts`:
- Around line 1-27: Update createClackSyncReviewPrompter.reviewNewDependencies
to use the caller-provided dependency summaries when constructing the prompt.
Surface each package name and its pending skillCount, matching the summary-style
feedback already used by selectClackSkills, while preserving the existing
decision options and cancellation behavior.
In `@packages/intent/src/commands/sync/state.ts`:
- Around line 37-62: Update parseEntry to reject path values that are not
project-relative POSIX paths by adding the existing isProjectRelativePath
validation to its rejection checks. Keep accepting valid string paths and
preserve the current InstallStateEntry mapping for all other validated fields.
In `@packages/intent/src/session-catalog.ts`:
- Around line 181-234: Harden getSessionCatalogue’s default cache location
against cross-user tampering by using a per-user root derived from
XDG_CACHE_HOME or os.homedir() instead of os.tmpdir(), creating the cache
directory with mode 0o700, and rejecting cache files not owned by the current
uid before readCache accepts them. Preserve explicit cacheDir behavior as
appropriate while ensuring cache hits cannot use foreign-owned files, including
entries with empty verification.
In `@packages/intent/src/shared/local-path.ts`:
- Around line 61-63: Update the local-path detection condition in the visible
path-classification logic to treat USER_DATA_POSIX_ROOTS paths with two or more
segments as local by changing the segments.length threshold. Add regression
cases covering /home/alice and /Users/alice, ensuring both are redacted while
preserving existing behavior for shorter paths.
In `@packages/intent/tests/sync.test.ts`:
- Around line 170-198: Update the symlink setup in the tests around “does not
replace unmanaged links and makes dry runs non-writing” and “treats an
unreadable owned link target as a conflict” to use the existing platform-aware
makeLink helper instead of raw symlinkSync(..., 'dir') calls. Preserve the
unmanaged-target and nonexistent-target scenarios while ensuring directory links
use junctions on Windows.
---
Nitpick comments:
In `@packages/intent/package.json`:
- Around line 20-22: Reorder the conditions in the "./catalog" export block so
the existing "types" entry appears before "import", matching the ordering used
by other subpaths and ensuring declaration resolution selects catalog.d.mts
explicitly.
In `@packages/intent/src/commands/sync/command.ts`:
- Around line 90-97: Update writeGitignore to replace the direct writeFileSync
call with the module’s existing writeTextFileAtomic helper, preserving the
current before/after comparison and boolean return behavior.
In `@packages/intent/src/commands/sync/gitignore.ts`:
- Around line 12-15: Hoist the escaped START/END block matcher from the current
function into a module-level constant, preserving the existing non-greedy
pattern and replacement behavior in the sync logic. Remove the per-call RegExp
construction while continuing to use the shared matcher for prefix replacement.
In `@packages/intent/src/commands/sync/prepare.ts`:
- Around line 16-20: Update containsIntentSync to recognize intent sync commands
separated by both && and semicolons, and allow common runner prefixes such as
pnpm exec before intent sync. Preserve whitespace handling and ensure these
command forms are detected so repeated wiring remains idempotent.
- Around line 33-42: Update the prepare-edit logic around scripts.prepare to
detect a present non-string value and fail loudly using the same error behavior
as the parse-error path, rather than replacing it with "intent sync". Preserve
the existing handling for missing, string, and intent-sync-containing prepare
values.
In `@packages/intent/src/core/lockfile/lockfile-state.ts`:
- Around line 17-52: Document the suffix-matching heuristic in
packageRelativeSkillFile immediately above the loop: explain that scanning
packageSegments from start=0 upward checks progressively shorter suffixes while
preferring the longest, most-specific suffix matching the skillSegments prefix,
preserving canonical lockfile path resolution.
In `@packages/intent/src/core/lockfile/lockfile.ts`:
- Around line 156-167: Update readIntentLockfile to accept the injected
ReadFs/fsCache reader used by computeSkillContentHash,
buildCurrentLockfileSources, and scanIntentPackageAtRoot, while defaulting to
the real filesystem reader for existing callers. Thread the available readFs
through intent-core.ts’s toResolvedIntentSkill and catalog-lock.ts’s
applyCatalogueLock so lockfile reads use the same cache instead of direct
readFileSync.
In `@packages/intent/src/core/source-policy.ts`:
- Around line 199-207: Update scanForConfiguredIntents to preserve and return
the warnings from scanForIntents alongside discovered and policy. Ensure the
intent sync output can consume these discovery warnings in addition to
policy.notices, without filtering or replacing the existing warning details.
In `@packages/intent/src/discovery/scanner.ts`:
- Around line 169-171: Memoize getProjectReadFs results per root to avoid
repeated PnP runtime setup. Add a module-level Map<string, ReadFs>, return the
cached filesystem when available, and cache either the loaded PnP readFs or
nodeReadFs before returning from getProjectReadFs.
In `@packages/intent/src/hooks/install.ts`:
- Around line 428-437: Rename isIntentGateScriptReference to
isIntentHookScriptReference throughout its declaration and all call sites,
preserving the existing regex and behavior that matches both gate and catalog
scripts.
- Line 354: Update the hook cleanup assignments around hooks.PreToolUse so empty
results do not create or retain a PreToolUse key; only assign the filtered array
when non-empty, otherwise remove the property. Apply the same behavior to the
additional affected hook-processing paths while preserving existing handling for
non-empty hooks.
In `@packages/intent/src/session-catalog.ts`:
- Around line 310-317: Bound the upward manifest walk by updating the loop
around directory and workspaceRoot to stop when the current directory reaches
workspaceRoot or dirname(directory) becomes unchanged at the filesystem root.
Preserve collecting package.json for each traversed directory and avoid adding
entries indefinitely when path normalization differs.
- Around line 249-266: Update the fingerprinting loop in the session-catalog
fingerprint function to avoid reading full lockfiles and workspace package.json
files on every invocation. Use each file’s stat metadata, including size and
modification time, as the primary hash input, and fall back to hashing file
contents only when stat metadata cannot be obtained; preserve the existing
missing-file handling and deterministic path ordering.
In `@packages/intent/tests/catalog-api.test.ts`:
- Around line 72-77: Update the test setup around getIntentCatalogContext to
provide a fixture-local cacheDir, using the existing roots-based temporary
directory rather than the default OS temp location. Ensure the option is
threaded through the context creation to getSessionCatalogue, and retain the
afterEach cleanup so the cache files are removed with the fixture roots.
In `@packages/intent/tests/cli.test.ts`:
- Around line 291-338: Clear the cumulative log buffer before each lifecycle
phase in the test’s sync flow: call logSpy.mockClear() immediately before every
main(['sync']) invocation whose output is subsequently asserted. Keep the
existing assertions and sync behavior unchanged while ensuring each check
validates only that phase’s log output.
In `@packages/intent/tests/core.test.ts`:
- Around line 541-571: Extend the wrong-source-kind test to also assert that
resolveIntentSkill rejects the same `@tanstack/query`#fetching request with the
identical intent.lock error, matching the adjacent content-hash and
missing-entry tests. Keep the existing loadIntentSkill assertion unchanged and
reuse the same root and fixture setup.
In `@packages/intent/tests/lockfile.test.ts`:
- Around line 73-103: Update the rejection assertions in the lockfile validation
test to match each case’s expected error message, rather than using bare
toThrow() calls. Anchor the expectations to the relevant validation symbols and
messages: unknown top-level fields, unsupported lockfileVersion, invalid sources
type, invalid source kind, duplicate source identity, duplicate skill paths, and
traversal paths should each assert a targeted message; use patterns such as
/source kind/ and /duplicate/i where appropriate.
🪄 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 Plus
Run ID: 76f89a70-96e1-47f3-87de-849146236d71
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
.github/workflows/pr.ymlbenchmarks/intent/catalog.bench.tsbenchmarks/intent/install-plan.bench.tsbenchmarks/intent/lockfile-hash.bench.tsbenchmarks/intent/sync.bench.tsbenchmarks/intent/tsconfig.jsonknip.jsonpackages/intent/package.jsonpackages/intent/src/catalog-lock.tspackages/intent/src/catalog.tspackages/intent/src/cli.tspackages/intent/src/commands/catalog.tspackages/intent/src/commands/install/command.tspackages/intent/src/commands/install/config.tspackages/intent/src/commands/install/consumer.tspackages/intent/src/commands/install/plan.tspackages/intent/src/commands/install/prompts.tspackages/intent/src/commands/sync/command.tspackages/intent/src/commands/sync/gitignore.tspackages/intent/src/commands/sync/links.tspackages/intent/src/commands/sync/plan.tspackages/intent/src/commands/sync/prepare.tspackages/intent/src/commands/sync/prompts.tspackages/intent/src/commands/sync/state.tspackages/intent/src/commands/sync/targets.tspackages/intent/src/core/intent-core.tspackages/intent/src/core/load-resolution.tspackages/intent/src/core/lockfile/hash.tspackages/intent/src/core/lockfile/lockfile-diff.tspackages/intent/src/core/lockfile/lockfile-state.tspackages/intent/src/core/lockfile/lockfile.tspackages/intent/src/core/skill-path.tspackages/intent/src/core/source-policy.tspackages/intent/src/core/types.tspackages/intent/src/discovery/scanner.tspackages/intent/src/hooks/adapters.tspackages/intent/src/hooks/agents/claude.tspackages/intent/src/hooks/agents/codex.tspackages/intent/src/hooks/agents/copilot.tspackages/intent/src/hooks/install.tspackages/intent/src/hooks/policy.tspackages/intent/src/hooks/types.tspackages/intent/src/session-catalog.tspackages/intent/src/shared/atomic-write.tspackages/intent/src/shared/command-runner.tspackages/intent/src/shared/local-path.tspackages/intent/src/skills/resolver.tspackages/intent/tests/catalog-api.test.tspackages/intent/tests/cli.test.tspackages/intent/tests/consumer-install.test.tspackages/intent/tests/core.test.tspackages/intent/tests/hash.test.tspackages/intent/tests/hooks-install.test.tspackages/intent/tests/hooks.test.tspackages/intent/tests/install-config.test.tspackages/intent/tests/install-plan.test.tspackages/intent/tests/local-path.test.tspackages/intent/tests/lockfile-diff.test.tspackages/intent/tests/lockfile-state.test.tspackages/intent/tests/lockfile.test.tspackages/intent/tests/resolver.test.tspackages/intent/tests/session-catalog.test.tspackages/intent/tests/skill-path.test.tspackages/intent/tests/sync.test.ts
💤 Files with no reviewable changes (8)
- packages/intent/src/hooks/agents/copilot.ts
- packages/intent/src/shared/command-runner.ts
- benchmarks/intent/tsconfig.json
- packages/intent/src/hooks/agents/claude.ts
- packages/intent/src/hooks/agents/codex.ts
- packages/intent/tests/hooks.test.ts
- packages/intent/src/hooks/policy.ts
- packages/intent/src/hooks/types.ts
| // `rmSync` with recursive+force silently leaves some directory symlinks in place. | ||
| // On Windows a directory symlink or junction needs `rmdirSync`, not `unlinkSync`. | ||
| function removeLink(path: string): void { | ||
| try { | ||
| unlinkSync(path) | ||
| } catch { | ||
| rmdirSync(path) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
removeLink can throw and abort the entire reconciliation.
If unlinkSync fails for a reason other than "is a directory" (e.g. EPERM/EBUSY, common on Windows), rmdirSync will also throw and the error propagates out of reconcileManagedLinks, leaving the run half-applied and no install state written. Surface it as a conflict instead.
🛡️ Proposed fix
-function removeLink(path: string): void {
+function removeLink(path: string): boolean {
try {
unlinkSync(path)
+ return true
} catch {
- rmdirSync(path)
+ try {
+ rmdirSync(path)
+ return true
+ } catch {
+ return false
+ }
}
}Callers at Lines 159 and 181 then record entry.path in conflicts when removal fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/src/commands/sync/links.ts` around lines 96 - 104, Update
removeLink to report removal failure as a non-throwing result, allowing
reconcileManagedLinks callers at the removal sites to add entry.path to
conflicts when unlinkSync and rmdirSync both fail. Preserve successful handling
for file links and directory symlinks, and ensure reconciliation continues so
install state can still be written.
| import { cancel, isCancel, outro, select } from '@clack/prompts' | ||
| import { selectClackSkills } from '../install/prompts.js' | ||
| import type { NewDependencyDecision, SyncReviewPrompter } from './command.js' | ||
|
|
||
| export function createClackSyncReviewPrompter(): SyncReviewPrompter { | ||
| return { | ||
| complete(message: string): void { | ||
| outro(message) | ||
| }, | ||
| async reviewNewDependencies(): Promise<NewDependencyDecision | null> { | ||
| const decision = await select<NewDependencyDecision>({ | ||
| message: 'How do you want to handle these dependencies?', | ||
| options: [ | ||
| { value: 'review', label: 'Review and install' }, | ||
| { value: 'exclude', label: 'Exclude these packages' }, | ||
| { value: 'later', label: 'Remind me later' }, | ||
| ], | ||
| }) | ||
| if (!isCancel(decision)) return decision | ||
| cancel('Sync review cancelled. New dependencies remain pending.') | ||
| return null | ||
| }, | ||
| selectSkills(packages) { | ||
| return selectClackSkills(packages, false) | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
reviewNewDependencies drops the dependency summaries passed by the caller.
The sync command calls prompts.reviewNewDependencies(packages.map((pkg) => ({ name: sourceName(pkg), skillCount: pkg.skills.length }))), but this implementation ignores the argument entirely and shows a generic prompt. Users choosing "Review and install" / "Exclude these packages" / "Remind me later" never see which packages or how many skills are pending, unlike selectClackSkills which does surface a note() with counts.
🛠️ Proposed fix to surface the dependency list
-import { cancel, isCancel, outro, select } from '`@clack/prompts`'
+import { cancel, isCancel, note, outro, select } from '`@clack/prompts`'
import { selectClackSkills } from '../install/prompts.js'
import type { NewDependencyDecision, SyncReviewPrompter } from './command.js'
export function createClackSyncReviewPrompter(): SyncReviewPrompter {
return {
complete(message: string): void {
outro(message)
},
- async reviewNewDependencies(): Promise<NewDependencyDecision | null> {
+ async reviewNewDependencies(
+ dependencies,
+ ): Promise<NewDependencyDecision | null> {
+ note(
+ dependencies
+ .map(
+ (dep) =>
+ `${dep.name} (${dep.skillCount} ${dep.skillCount === 1 ? 'skill' : 'skills'})`,
+ )
+ .join('\n'),
+ 'New dependencies found',
+ )
const decision = await select<NewDependencyDecision>({📝 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.
| import { cancel, isCancel, outro, select } from '@clack/prompts' | |
| import { selectClackSkills } from '../install/prompts.js' | |
| import type { NewDependencyDecision, SyncReviewPrompter } from './command.js' | |
| export function createClackSyncReviewPrompter(): SyncReviewPrompter { | |
| return { | |
| complete(message: string): void { | |
| outro(message) | |
| }, | |
| async reviewNewDependencies(): Promise<NewDependencyDecision | null> { | |
| const decision = await select<NewDependencyDecision>({ | |
| message: 'How do you want to handle these dependencies?', | |
| options: [ | |
| { value: 'review', label: 'Review and install' }, | |
| { value: 'exclude', label: 'Exclude these packages' }, | |
| { value: 'later', label: 'Remind me later' }, | |
| ], | |
| }) | |
| if (!isCancel(decision)) return decision | |
| cancel('Sync review cancelled. New dependencies remain pending.') | |
| return null | |
| }, | |
| selectSkills(packages) { | |
| return selectClackSkills(packages, false) | |
| }, | |
| } | |
| } | |
| import { cancel, isCancel, note, outro, select } from '`@clack/prompts`' | |
| import { selectClackSkills } from '../install/prompts.js' | |
| import type { NewDependencyDecision, SyncReviewPrompter } from './command.js' | |
| export function createClackSyncReviewPrompter(): SyncReviewPrompter { | |
| return { | |
| complete(message: string): void { | |
| outro(message) | |
| }, | |
| async reviewNewDependencies( | |
| dependencies, | |
| ): Promise<NewDependencyDecision | null> { | |
| note( | |
| dependencies | |
| .map( | |
| (dep) => | |
| `${dep.name} (${dep.skillCount} ${dep.skillCount === 1 ? 'skill' : 'skills'})`, | |
| ) | |
| .join('\n'), | |
| 'New dependencies found', | |
| ) | |
| const decision = await select<NewDependencyDecision>({ | |
| message: 'How do you want to handle these dependencies?', | |
| options: [ | |
| { value: 'review', label: 'Review and install' }, | |
| { value: 'exclude', label: 'Exclude these packages' }, | |
| { value: 'later', label: 'Remind me later' }, | |
| ], | |
| }) | |
| if (!isCancel(decision)) return decision | |
| cancel('Sync review cancelled. New dependencies remain pending.') | |
| return null | |
| }, | |
| selectSkills(packages) { | |
| return selectClackSkills(packages, false) | |
| }, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/src/commands/sync/prompts.ts` around lines 1 - 27, Update
createClackSyncReviewPrompter.reviewNewDependencies to use the caller-provided
dependency summaries when constructing the prompt. Surface each package name and
its pending skillCount, matching the summary-style feedback already used by
selectClackSkills, while preserving the existing decision options and
cancellation behavior.
| function parseEntry(value: unknown): InstallStateEntry | null { | ||
| if (!isRecord(value) || !isRecord(value.source)) return null | ||
| const keys = Object.keys(value).sort().join(',') | ||
| if (keys !== 'alias,linkTarget,path,skillPath,source,targetDirectory') | ||
| return null | ||
| if (Object.keys(value.source).sort().join(',') !== 'id,kind') return null | ||
| if ( | ||
| typeof value.targetDirectory !== 'string' || | ||
| typeof value.path !== 'string' || | ||
| typeof value.alias !== 'string' || | ||
| typeof value.skillPath !== 'string' || | ||
| typeof value.linkTarget !== 'string' || | ||
| typeof value.source.id !== 'string' || | ||
| (value.source.kind !== 'npm' && value.source.kind !== 'workspace') | ||
| ) { | ||
| return null | ||
| } | ||
| return { | ||
| targetDirectory: value.targetDirectory, | ||
| path: value.path, | ||
| alias: value.alias, | ||
| source: { kind: value.source.kind, id: value.source.id }, | ||
| skillPath: value.skillPath, | ||
| linkTarget: value.linkTarget, | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Validate that path is a project-relative POSIX path.
parseEntry accepts any string, so an absolute or ..-containing path survives parsing and readInstallStateForLinks will resolve it outside the project root — where reconcileManagedLinks may unlink it as a "stale owned link". Rejecting absolute/traversing paths keeps removal confined to the project.
🛡️ Proposed guard
+function isProjectRelativePath(value: string): boolean {
+ return (
+ value !== '' &&
+ !value.startsWith('/') &&
+ !/^[a-zA-Z]:/.test(value) &&
+ !value.includes('\\') &&
+ !value.split('/').includes('..')
+ )
+}
+
function parseEntry(value: unknown): InstallStateEntry | null {Then add || !isProjectRelativePath(value.path) to the rejection checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/src/commands/sync/state.ts` around lines 37 - 62, Update
parseEntry to reject path values that are not project-relative POSIX paths by
adding the existing isProjectRelativePath validation to its rejection checks.
Keep accepting valid string paths and preserve the current InstallStateEntry
mapping for all other validated fields.
| export async function getSessionCatalogue({ | ||
| cacheDir = join(tmpdir(), 'tanstack-intent', 'catalogues'), | ||
| discover, | ||
| refresh = false, | ||
| root, | ||
| policyRoot = root, | ||
| readFs, | ||
| }: { | ||
| cacheDir?: string | ||
| discover: () => | ||
| | DiscoveredSessionCatalogue | ||
| | Promise<DiscoveredSessionCatalogue> | ||
| refresh?: boolean | ||
| root: string | ||
| policyRoot?: string | ||
| readFs?: ReadFs | ||
| }): Promise<SessionCatalogueResult> { | ||
| const workspaceRoot = normalizeRoot(root) | ||
| const normalizedPolicyRoot = normalizeRoot(policyRoot) | ||
| const dependencyFingerprint = computeCatalogueFingerprint( | ||
| workspaceRoot, | ||
| normalizedPolicyRoot, | ||
| ) | ||
| const cachePath = join( | ||
| cacheDir, | ||
| `${createHash('sha256').update(workspaceRoot).update('\0').update(normalizedPolicyRoot).digest('hex')}.json`, | ||
| ) | ||
| const cached = readCache(cachePath) | ||
|
|
||
| if ( | ||
| !refresh && | ||
| cached?.workspaceRoot === workspaceRoot && | ||
| cached.policyRoot === normalizedPolicyRoot && | ||
| cached.dependencyFingerprint === dependencyFingerprint && | ||
| verifyCatalogueContent(cached.verification, readFs) | ||
| ) { | ||
| return { | ||
| cachePath, | ||
| cacheStatus: 'hit', | ||
| catalogue: cached.catalogue, | ||
| } | ||
| } | ||
|
|
||
| const refreshed = await discover() | ||
| const catalogue = buildSessionCatalogue(refreshed.result) | ||
| const entry: IntentSessionCatalogueCache = { | ||
| schemaVersion: CACHE_SCHEMA_VERSION, | ||
| workspaceRoot, | ||
| policyRoot: normalizedPolicyRoot, | ||
| dependencyFingerprint, | ||
| catalogue, | ||
| verification: refreshed.verification, | ||
| } | ||
| writeCache(cachePath, entry) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Shared-tmp cache is attacker-influenceable on multi-user hosts.
The cache lives at a predictable path under os.tmpdir() (tanstack-intent/catalogues/<sha256>.json). On a shared Linux box another local user can pre-create tanstack-intent/catalogues (or the target file as a symlink) — mkdirSync(..., { recursive: true }) succeeds against a foreign-owned directory and readCache performs no ownership or mode check.
The content check does not close this: verifyCatalogueContent returns true for an empty verification array, so a planted entry with verification: [] and a matching dependencyFingerprint (derivable from world-readable repo files) is accepted as a hit, and its catalogue.skills/warnings text is fed straight into agent context. Prefer a per-user cache root (e.g. XDG_CACHE_HOME/os.homedir()) created with mode 0o700, and reject cache files not owned by the current uid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/src/session-catalog.ts` around lines 181 - 234, Harden
getSessionCatalogue’s default cache location against cross-user tampering by
using a per-user root derived from XDG_CACHE_HOME or os.homedir() instead of
os.tmpdir(), creating the cache directory with mode 0o700, and rejecting cache
files not owned by the current uid before readCache accepts them. Preserve
explicit cacheDir behavior as appropriate while ensuring cache hits cannot use
foreign-owned files, including entries with empty verification.
| (root !== undefined && | ||
| USER_DATA_POSIX_ROOTS.has(root) && | ||
| segments.length >= 3) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact two-segment home paths.
At Line 62, /home/alice and /Users/alice have only two segments, so they reach agent context unredacted. Treat user-data roots with two or more segments as local and add regression cases.
Proposed fix
- segments.length >= 3)
+ segments.length >= 2)📝 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.
| (root !== undefined && | |
| USER_DATA_POSIX_ROOTS.has(root) && | |
| segments.length >= 3) | |
| (root !== undefined && | |
| USER_DATA_POSIX_ROOTS.has(root) && | |
| segments.length >= 2) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/src/shared/local-path.ts` around lines 61 - 63, Update the
local-path detection condition in the visible path-classification logic to treat
USER_DATA_POSIX_ROOTS paths with two or more segments as local by changing the
segments.length threshold. Add regression cases covering /home/alice and
/Users/alice, ensuring both are redacted while preserving existing behavior for
shorter paths.
| it('does not replace unmanaged links and makes dry runs non-writing', () => { | ||
| const root = tempRoot('intent-sync-conflict-') | ||
| const link = expected(root) | ||
| mkdirSync(join(root, '.github', 'skills'), { recursive: true }) | ||
| symlinkSync(join(root, 'somewhere-else'), link.path, 'dir') | ||
| const conflict = reconcileManagedLinks({ | ||
| dryRun: false, | ||
| expected: [link], | ||
| stateResult: { status: 'missing' }, | ||
| }) | ||
| expect(conflict.conflicts).toEqual([link.path]) | ||
| const dryRunLink = { | ||
| ...link, | ||
| path: join(root, '.github', 'skills', 'dry-run'), | ||
| } | ||
| const dryRun = reconcileManagedLinks({ | ||
| dryRun: true, | ||
| expected: [dryRunLink], | ||
| stateResult: { status: 'missing' }, | ||
| }) | ||
| expect(dryRun.created).toEqual([dryRunLink.path]) | ||
| expect(existsSync(dryRunLink.path)).toBe(false) | ||
| }) | ||
|
|
||
| it('treats an unreadable owned link target as a conflict', () => { | ||
| const root = tempRoot('intent-sync-unreadable-') | ||
| const link = expected(root) | ||
| mkdirSync(join(root, '.github', 'skills'), { recursive: true }) | ||
| symlinkSync(join(root, 'missing'), link.path, 'dir') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
These tests create raw 'dir' symlinks, which can fail on Windows — the platform the new delivery job targets.
links.ts deliberately uses 'junction' on win32 because creating directory symlinks requires elevation/Developer Mode. Lines 174 and 198 bypass that and call symlinkSync(target, path, 'dir') directly, so the delivery matrix job may fail with EPERM rather than exercising the conflict paths. Line 198 also points at a nonexistent target, which junctions tolerate but symlink creation may not.
🛡️ Proposed test helper
+function makeLink(target: string, path: string): void {
+ symlinkSync(target, path, process.platform === 'win32' ? 'junction' : 'dir')
+}Then use makeLink(...) at Lines 174 and 198.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/tests/sync.test.ts` around lines 170 - 198, Update the
symlink setup in the tests around “does not replace unmanaged links and makes
dry runs non-writing” and “treats an unreadable owned link target as a conflict”
to use the existing platform-aware makeLink helper instead of raw
symlinkSync(..., 'dir') calls. Preserve the unmanaged-target and
nonexistent-target scenarios while ensuring directory links use junctions on
Windows.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/intent/tests/integration/source-policy-surfaces.test.ts (1)
90-105: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlso cover agent-facing warning and source redaction.
The result exposes
warningsandhiddenSourcesin addition topackagesandnotices. Assert thatUNLISTEDis absent from warnings and thathiddenSourcesis empty foraudience: 'agent'; otherwise a privacy regression could pass this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/integration/source-policy-surfaces.test.ts` around lines 90 - 105, Extend the agent-audience test around listIntentSkills to assert that no warning contains UNLISTED and that result.hiddenSources is empty. Keep the existing package and notice assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/intent/tests/integration/source-policy-surfaces.test.ts`:
- Around line 90-105: Extend the agent-audience test around listIntentSkills to
assert that no warning contains UNLISTED and that result.hiddenSources is empty.
Keep the existing package and notice assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c2cba6f-62c8-470d-8ac7-43c41c7f3c8a
📒 Files selected for processing (7)
packages/intent/src/commands/exclude.tspackages/intent/src/commands/install/plan.tspackages/intent/src/core/lockfile/lockfile.tspackages/intent/tests/install-plan.test.tspackages/intent/tests/integration/source-policy-surfaces.test.tspackages/intent/tests/lockfile.test.tspackages/intent/tests/scanner.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/intent/tests/install-plan.test.ts
- packages/intent/src/commands/install/plan.ts
- packages/intent/src/core/lockfile/lockfile.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/intent/src/core/intent-core.ts (1)
345-365: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant policy compilation on the fast path.
isSourcePermittedandisSkillPermittedeach callcompileSkillSourcePolicy(config)internally, so this block recompiles matchers for the sameconfigtwice per fast-path resolution. Since this runs on every skill load, consider compiling once and reusing bothpermits/permitsSkill, mirroring howcheckLoadAllowedalready does this insource-policy.ts.♻️ Proposed refactor
if (fastPathResolved) { - if ( - !isSourcePermitted(config, parsedUse.packageName, fastPathResolved.kind) - ) { - const lateRefusal = packageNotListedRefusal(use, parsedUse.packageName) - throw new IntentCoreError(lateRefusal.code, lateRefusal.message) - } - if ( - !isSkillPermitted( - config, - parsedUse.packageName, - parsedUse.skillName, - fastPathResolved.kind, - ) - ) { + const fastPathPolicy = compileSkillSourcePolicy(config) + if (!fastPathPolicy.permits(parsedUse.packageName, fastPathResolved.kind)) { + const lateRefusal = packageNotListedRefusal(use, parsedUse.packageName) + throw new IntentCoreError(lateRefusal.code, lateRefusal.message) + } + if ( + !fastPathPolicy.permitsSkill( + parsedUse.packageName, + parsedUse.skillName, + fastPathResolved.kind, + ) + ) { const lateRefusal = skillNotListedRefusal(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/core/intent-core.ts` around lines 345 - 365, Update the fast-path policy checks in the intent resolution flow to compile the skill source policy once and reuse its permits and permitsSkill functions for both source and skill validation. Mirror the existing checkLoadAllowed pattern in source-policy.ts, replacing the separate isSourcePermitted and isSkillPermitted calls while preserving the current refusal errors and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/intent/src/commands/list.ts`:
- Around line 104-108: Update the hiddenSkills condition in the list command’s
console.log so the “not listed” text is rendered only when hiddenSkills contains
at least one item. Use a length-based guard that safely handles undefined, while
preserving the existing output for non-empty arrays and the standard count-only
output otherwise.
---
Nitpick comments:
In `@packages/intent/src/core/intent-core.ts`:
- Around line 345-365: Update the fast-path policy checks in the intent
resolution flow to compile the skill source policy once and reuse its permits
and permitsSkill functions for both source and skill validation. Mirror the
existing checkLoadAllowed pattern in source-policy.ts, replacing the separate
isSourcePermitted and isSkillPermitted calls while preserving the current
refusal errors and behavior.
🪄 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 Plus
Run ID: 14d0e55c-8fe4-4b3e-85c8-835a458f74d7
📒 Files selected for processing (11)
packages/intent/src/commands/install/plan.tspackages/intent/src/commands/list.tspackages/intent/src/core/excludes.tspackages/intent/src/core/intent-core.tspackages/intent/src/core/skill-sources.tspackages/intent/src/core/source-policy.tspackages/intent/src/core/types.tspackages/intent/tests/core.test.tspackages/intent/tests/install-plan.test.tspackages/intent/tests/skill-sources.test.tspackages/intent/tests/source-policy.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/intent/tests/install-plan.test.ts
- packages/intent/src/commands/install/plan.ts
| const count = `${source.skillCount} ${source.skillCount === 1 ? 'skill' : 'skills'}` | ||
| console.log( | ||
| ` ${source.name} (${source.skillCount} ${source.skillCount === 1 ? 'skill' : 'skills'})`, | ||
| source.hiddenSkills | ||
| ? ` ${source.name} (${count} not listed: ${source.hiddenSkills.join(', ')})` | ||
| : ` ${source.name} (${count})`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard hiddenSkills by length, not truthiness.
An empty array is truthy in JavaScript, so hiddenSkills: [] prints not listed: with no names. Use source.hiddenSkills?.length or ensure producers omit empty arrays.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/src/commands/list.ts` around lines 104 - 108, Update the
hiddenSkills condition in the list command’s console.log so the “not listed”
text is rendered only when hiddenSkills contains at least one item. Use a
length-based guard that safely handles undefined, while preserving the existing
output for non-empty arrays and the standard count-only output otherwise.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/intent/src/cli.ts (1)
203-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared agent list instead of a hardcoded literal.
ALL_HOOK_AGENTS(used byparseAgentsinpackages/intent/src/hooks/install.ts) already enumerates the valid agents. Duplicating['copilot', 'claude', 'codex']here means adding an agent requires touching two places, and theas HookAgentcast hides that drift from the compiler.♻️ Suggested refactor
- if (!['copilot', 'claude', 'codex'].includes(options.agent)) { + const { ALL_HOOK_AGENTS } = await import('./hooks/types.js') + if (!ALL_HOOK_AGENTS.includes(options.agent as HookAgent)) { fail( `Unknown hook agent: ${options.agent}. Expected copilot, claude, or codex.`, ) }Adjust the import path to wherever
ALL_HOOK_AGENTSis exported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/cli.ts` around lines 203 - 215, Update the run action validation in the CLI to reuse the exported ALL_HOOK_AGENTS list from hooks/install.ts instead of duplicating the agent literals. Import that shared symbol, validate options.agent against it, and pass the resulting value to runSessionCatalogueHook without relying on a cast that can hide list drift.packages/intent/tests/consumer-install.test.ts (1)
137-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
toHaveBeenCalledOnce()relies on no other test touching the real prompter.
clackPromptMocks.selectis module-scoped and there's no visible reset between tests, so this assertion becomes order-dependent the moment another case constructscreateClackInstallerPrompter(). Add abeforeEach(() => { vi.clearAllMocks() })(or reset the two hoisted mocks explicitly) to keep it isolated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/consumer-install.test.ts` around lines 137 - 145, Add per-test mock isolation for the module-scoped clackPromptMocks used by createClackInstallerPrompter tests: add a beforeEach that clears all Vitest mocks (or explicitly resets the relevant hoisted mocks) before each case, preserving the existing selectMethod assertions.packages/intent/src/hooks/install.ts (1)
149-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
upsertClaudeHooksandupsertCodexHooksare now identical.After dropping the gate logic both bodies are byte-for-byte the same; they can collapse into a single matcher-based helper, leaving only the Copilot variant distinct.
Separately,
hooks.PreToolUse = removeIntentHooks(...)runs unconditionally, so configs that never declaredPreToolUsenow gain an emptyPreToolUse: []key. Skipping the assignment when the resulting array is empty and the key was absent keeps user configs clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/hooks/install.ts` around lines 149 - 199, Collapse the identical upsertClaudeHooks and upsertCodexHooks implementations into one shared matcher-based helper, and update their callers to use it while keeping upsertCopilotHooks distinct. In the shared and Copilot helpers, only assign hooks.PreToolUse after removeIntentHooks when the key already existed or the resulting array is non-empty, so configs without PreToolUse do not gain an empty array.packages/intent/tests/hooks-install.test.ts (1)
117-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCopilot expectation depends on the fixture lacking a
packageManagermarker.This root has no
packageManagerfield, sodetectPackageManagerfalls back and the command becomesnpx …. A copilot install run from a pnpm project would writepnpm dlx …into the user-scope config. Adding a case that installs copilot from the pnpm-pinned fixture would pin down the intended behavior (see the related note inpackages/intent/src/hooks/install.tslines 104-119).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/hooks-install.test.ts` around lines 117 - 119, Add a test case in the hooks installation suite that runs the Copilot installation against the pnpm-pinned fixture and verifies the user-scope configuration records a pnpm dlx command, while preserving the existing npx expectation for fixtures without a packageManager marker. Use the existing Copilot install flow and related install logic in install.ts as the implementation reference.packages/intent/src/commands/install/consumer.ts (1)
197-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkipped hook installs are dropped silently.
runInstallHooksreturnsstatus: 'skipped'with areason(seepackages/intent/src/hooks/install.tslines 93-102), but both filters discard those entries. If an agent is skipped the user only sees it missing from "Installed hook agents", with no explanation. Surfacing the reasons — the wayformatHookInstallResultdoes — would make the outcome self-explanatory.Also,
hookAgentsis only used to deriveprojectAgents, while the Copilot path is keyed offtargets.includes('github')instead; folding the copilot check into the mapped agents would remove the parallel source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/commands/install/consumer.ts` around lines 197 - 219, Update the hook-install handling around hookAgentForTarget and runInstallHooks to retain skipped results and surface each result’s reason through the existing formatHookInstallResult mechanism instead of silently filtering them out. Derive the Copilot user-scope installation decision from the mapped hook agents rather than separately checking targets.includes('github'), while preserving the existing scope and consent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/intent/src/commands/install/config.ts`:
- Line 5: Update readIntentConsumerConfig and the InstallMethod validation to
recognize unsupported legacy values such as "map" during config reads, then
provide a clear migration or upgrade-path error instead of the generic
unknown-method validation failure; preserve normal handling for supported
"symlink" and "hooks" methods.
In `@packages/intent/src/hooks/install.ts`:
- Around line 104-119: The catalog command in
packages/intent/src/hooks/install.ts lines 104-119 should use a scope-neutral
runner when scope is user, while retaining detectPackageManager(root) for
project-scoped installs; update the formatIntentCommand call accordingly. In
packages/intent/tests/hooks-install.test.ts lines 117-119, add a Copilot
user-scope install case using the packageManager: pnpm@10.0.0 fixture and assert
the generated command uses the neutral runner.
---
Nitpick comments:
In `@packages/intent/src/cli.ts`:
- Around line 203-215: Update the run action validation in the CLI to reuse the
exported ALL_HOOK_AGENTS list from hooks/install.ts instead of duplicating the
agent literals. Import that shared symbol, validate options.agent against it,
and pass the resulting value to runSessionCatalogueHook without relying on a
cast that can hide list drift.
In `@packages/intent/src/commands/install/consumer.ts`:
- Around line 197-219: Update the hook-install handling around
hookAgentForTarget and runInstallHooks to retain skipped results and surface
each result’s reason through the existing formatHookInstallResult mechanism
instead of silently filtering them out. Derive the Copilot user-scope
installation decision from the mapped hook agents rather than separately
checking targets.includes('github'), while preserving the existing scope and
consent behavior.
In `@packages/intent/src/hooks/install.ts`:
- Around line 149-199: Collapse the identical upsertClaudeHooks and
upsertCodexHooks implementations into one shared matcher-based helper, and
update their callers to use it while keeping upsertCopilotHooks distinct. In the
shared and Copilot helpers, only assign hooks.PreToolUse after removeIntentHooks
when the key already existed or the resulting array is non-empty, so configs
without PreToolUse do not gain an empty array.
In `@packages/intent/tests/consumer-install.test.ts`:
- Around line 137-145: Add per-test mock isolation for the module-scoped
clackPromptMocks used by createClackInstallerPrompter tests: add a beforeEach
that clears all Vitest mocks (or explicitly resets the relevant hoisted mocks)
before each case, preserving the existing selectMethod assertions.
In `@packages/intent/tests/hooks-install.test.ts`:
- Around line 117-119: Add a test case in the hooks installation suite that runs
the Copilot installation against the pnpm-pinned fixture and verifies the
user-scope configuration records a pnpm dlx command, while preserving the
existing npx expectation for fixtures without a packageManager marker. Use the
existing Copilot install flow and related install logic in install.ts as the
implementation reference.
🪄 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 Plus
Run ID: 9f1a27d3-ea08-4e3e-8b40-09259c4a63d3
📒 Files selected for processing (15)
packages/intent/src/cli.tspackages/intent/src/commands/install/config.tspackages/intent/src/commands/install/consumer.tspackages/intent/src/commands/install/plan.tspackages/intent/src/commands/install/prompts.tspackages/intent/src/commands/sync/command.tspackages/intent/src/hooks/adapters.tspackages/intent/src/hooks/install.tspackages/intent/tests/catalog-api.test.tspackages/intent/tests/cli.test.tspackages/intent/tests/consumer-install.test.tspackages/intent/tests/hooks-install.test.tspackages/intent/tests/install-config.test.tspackages/intent/tests/install-plan.test.tspackages/intent/tests/integration/source-policy-surfaces.test.ts
💤 Files with no reviewable changes (1)
- packages/intent/src/hooks/adapters.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/intent/tests/catalog-api.test.ts
- packages/intent/tests/install-config.test.ts
- packages/intent/tests/install-plan.test.ts
- packages/intent/src/commands/install/plan.ts
- packages/intent/tests/cli.test.ts
- packages/intent/tests/integration/source-policy-surfaces.test.ts
- packages/intent/src/commands/sync/command.ts
| import { compileExcludePatterns } from '../../core/excludes.js' | ||
| import { parseSkillSources } from '../../core/skill-sources.js' | ||
|
|
||
| export type InstallMethod = 'symlink' | 'hooks' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=ts -C2 "'map'" packages/intent/src | rg -n -C2 'InstallMethod|install'
rg -n -C3 '\bmap\b' packages/intent/CHANGELOG.md 2>/dev/null | head -40Repository: TanStack/intent
Length of output: 1430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config file outline =="
ast-grep outline packages/intent/src/commands/install/config.ts --view expanded || true
echo
echo "== relevant config source =="
cat -n packages/intent/src/commands/install/config.ts
echo
echo "== usages of InstallMethod, validateInstall, readIntentConsumerConfig, setIntentConsumerConfig =="
rg -n "InstallMethod|validateInstall|readIntentConsumerConfig|setIntentConsumerConfig|install\\.method" packages -g '*.{ts,tsx,js,mjs,cjs,json,md}' | head -200Repository: TanStack/intent
Length of output: 15945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git diff/stat for changed files =="
git diff --stat || true
git diff --name-only || true
echo
echo "== references to 'map' in packages/intent =="
rg -n --type=ts -n --type=tsx -n -C2 -e "['\"]map['\"]|MapConfig|map" packages/intent/src | head -200Repository: TanStack/intent
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path('packages/intent/src/commands/install/config.ts').read_text()
m = re.search(r"^(?:const )?INSTALL_METHODS:.*=.*?\{(?P<body>.*?)\n\}", source, re.S | re.M)
if m:
body = m.group('body')
print("INSTALL_METHODS body:")
print(body)
print()
print("method keys:", sorted(re.findall(r"(?:^\s*)?([A-Za-z]+):", body, re.M)))
print("method string values:", sorted(re.findall(r"(?:^\s*)?[A-Za-z]+\s*:\s*new\s+Set\(\[[^\]]*?(['\"][A-Za-z]+['\"])", body, re.S)))
else:
print("INSTALL_METHODS not found")
cfg = Path('packages/intent/CHANGELOG.md').read_text()
# naive scan for installed-install/config/map wording
terms = {
"method:",
"install.method",
"install method",
"symlink",
"hooks",
"map",
"mapping",
}
hits = []
for i, line in enumerate(cfg.splitlines(), 1):
if any(term in line for term in terms):
hits.append((i, line))
print("\\nCHANGELOG lines containing install method terms:")
for i, line in hits[:120]:
print(f"{i}: {line}")
PY
echo
echo "== tests around unknown install methods =="
sed -n '1,110p' packages/intent/tests/install-config.test.ts
sed -n '160,245p' packages/intent/tests/consumer-install.test.ts
echo
echo "== install command entrypoints =="
rg -n "readIntentConsumerConfig\\(|intent install|intent sync|setupInstall|selectMethod|readConfig" packages/intent/src/commands -g '*.ts' -C 3Repository: TanStack/intent
Length of output: 19977
Handle unsupported intent.install.method values during config reads.
readIntentConsumerConfig rejects any package.json with intent.install.method: "map" before intent install or intent sync can proceed. Since this no longer matches an installed method, give users a clear upgrade path, e.g. catching unsupported legacy values and/or providing a migration, instead of a generic unknown-method validation error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/intent/src/commands/install/config.ts` at line 5, Update
readIntentConsumerConfig and the InstallMethod validation to recognize
unsupported legacy values such as "map" during config reads, then provide a
clear migration or upgrade-path error instead of the generic unknown-method
validation failure; preserve normal handling for supported "symlink" and "hooks"
methods.
…g it Running the installer without @tanstack/intent as a devDependency no longer fails. The prepare script is only wired when the dependency is present, and the installer says what that costs: skills will not re-sync automatically.
Hook configs and AGENTS.md blocks are written once and executed on every agent session, so `@latest` meant a file written today would silently run a future major version. Generated commands now carry the writing CLI's own version: an exact pin for prereleases, since npm ranges exclude them, and major.minor otherwise so committed files stay stable across patch releases. The AGENTS.md verifier accepts pinned specifiers alongside the older forms.
…m skill content hashing
1 participant
🎯 Changes
Draft. Do not merge — see Not done yet.
intent installbecomes a real setup command. Today a consumer has to discover every skill-shipping dependency and hand-type each one intopackage.json#intent.skills; nothing in the CLI writes that file. This branch makes one confirmed command write the policy, the content baseline, the delivery preferences, and the delivery artifacts together.Four layers, one command
intent.skills/intent.exclude) records which sources may contribute.intent.lockrecords the content baseline that was accepted, hashed per skill.intent installwalks method → targets → skills → confirm, then writes once.intent syncreconciles delivery afterwards without prompting and without ever writingintent.lock, so it is safe to wire intoprepare.Per-skill lockfile
intent.lockhashes each skill directory individually —SKILL.mdplus supportedreferences/,assets/, andscripts/files. A new, changed, or removed skill therefore does not invalidate its unchanged accepted siblings from the same package.intent loadand catalogue generation both refuse content that no longer matches the accepted baseline.Managed symlinks
Skill folders are linked directly from their installed packages using the
skills-npmpackage-prefixed convention, so the source identity survives in the installed name and same-named skills from different packages cannot collide.Cleanup is the part that needed care. Intent removes a link only when persisted ownership state says it created it and the on-disk entry is still a link to the exact target recorded. A real directory, an unmanaged link, or a link that now points elsewhere is a conflict and stays untouched. The
npm-/workspace-prefix is a readability convention, never proof of ownership.Symlinks expose live package content, which the installer states plainly and requires explicit confirmation for. Intent detects drift on its next run; it cannot prevent a read in between. That is a real difference from
intent load, which checks the lock immediately before returning content.Removes the session-wide hook edit gate
One observed
intent loadcould not prove the loaded skill matched the current task, so the gate blocked edits without evidence. Reinstalling hooks clears old gate entries without touching unrelated user hooks.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Changesets are deliberately deferred until the public surface stops moving. This box is not yet truthful — do not merge before it is.
Summary by CodeRabbit
intent catalogwith cached session guidance (--json,--refresh).intent syncfor verified link synchronization (--json, interactive review).intent installwith TTY-based interactive method/target/skill selection and improved dry-run previews.intent.lockverification to prevent stale or mismatched skills from being used..gitignoreupdates.