feat: add dependency-walked discovery and source-aware package reporting#112
Open
LadyBluenotes wants to merge 16 commits intomainfrom
Open
feat: add dependency-walked discovery and source-aware package reporting#112LadyBluenotes wants to merge 16 commits intomainfrom
LadyBluenotes wants to merge 16 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors intent scanning by extracting package registration and dependency-walking into factories, adds an explicit Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Scanner as scanForIntents()
participant Walker as DependencyWalker
participant Registrar as PackageRegistrar
participant FS as FileSystem
CLI->>Scanner: scanForIntents(root, { includeGlobal: true })
Scanner->>Registrar: createPackageRegistrar(...) / prepare state
Scanner->>Walker: createDependencyWalker(...)
alt includeGlobal true
Scanner->>Registrar: scanTarget(nodeModules.global, 'global')
Registrar->>FS: readdir global/node_modules
FS-->>Registrar: package dirs
Registrar->>FS: read package.json, read skills/
Registrar->>Registrar: build IntentPackage(source='global')
Registrar->>Scanner: register package
end
Scanner->>Walker: walkProjectDeps()
Walker->>FS: read project package.json
FS-->>Walker: package.json
Walker->>Walker: getDeps -> resolveDepDir
Walker->>Registrar: tryRegister(depDir, depName)
Registrar->>FS: read package.json, read skills/
Registrar->>Registrar: build IntentPackage(source='local')
Registrar->>Scanner: register package
alt workspace present
Scanner->>Walker: walkWorkspacePackages()
Walker->>FS: discover workspace package dirs
Walker->>Registrar: tryRegister per workspace package
Registrar->>Registrar: select by depth/version
end
Scanner->>CLI: return ScanResult { packages: [...] }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 4aa03c0
☁️ Nx Cloud last updated this comment at |
commit: |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cli/intent-stale.md (1)
18-23:⚠️ Potential issue | 🟡 MinorClarify no-package behavior for JSON mode.
The current bullets imply the “No intent-enabled packages found.” message always prints, but
--jsonnow returns[]instead.Suggested doc tweak
-- If no packages are found, prints `No intent-enabled packages found.` +- In text mode, if no packages are found, prints `No intent-enabled packages found.` +- In `--json` mode, outputs `[]` when no packages are found.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli/intent-stale.md` around lines 18 - 23, The doc text is ambiguous about what happens when no packages are found; update the bullet list in intent-stale.md so it clearly states the behavior for JSON mode by replacing or augmenting the line that says "If no packages are found, prints `No intent-enabled packages found.`" with an explicit note that the CLI prints that message in text mode but returns an empty JSON array (`[]`) when invoked with the `--json` flag; mention both outputs and reference the `--json` option and the exact text message "No intent-enabled packages found." so readers know the difference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli/intent-list.md`:
- Around line 18-23: The documentation's "discovery scope" wording is too
narrow; update the intent-list docs to reflect that scanForIntents() examines
workspace packages as well as project and workspace dependencies and that intent
list can also include accessible global packages and discovery warnings—adjust
the bulleted scope lines and the sentence that states what is scanned so it
explicitly mentions "workspace packages (not just dependencies),
workspace/project dependencies, and accessible global packages" and retain the
notes about warnings, summary line, and table columns.
In `@docs/overview.md`:
- Line 8: Update the casing of the term "markdown" to the proper noun "Markdown"
in the sentence that defines Skills (the sentence containing "Skills are
markdown documents..."); replace the lowercase instance with "Markdown" to
ensure consistent technical documentation convention across docs/overview.md.
In `@packages/intent/src/discovery/register.ts`:
- Around line 108-123: The replacement logic for package candidates ignores the
package source and can let a global package replace a local one; update the
decision in the register logic to enforce local-over-global first: before
applying the depth/version tie-break, check candidate.source and existing.source
(e.g., if candidate.source === 'local' and existing.source !== 'local' then set
shouldReplace = true, and if existing.source === 'local' and candidate.source
!== 'local' then set shouldReplace = false), otherwise fall back to the
existingDepth/candidateDepth and opts.comparePackageVersions(existing.version,
candidate.version) logic and then assign into opts.packages[existingIndex] when
shouldReplace is true. Ensure you reference and use existing.packageRoot,
candidate.packageRoot, existingIndex, opts.getPackageDepth and
opts.comparePackageVersions as currently used.
In `@packages/intent/src/discovery/walk.ts`:
- Line 3: The named imports on the import line are not alphabetized; reorder the
members in the import from '../utils.js' into alphabetical order (getDeps,
listNodeModulesPackageDirs, resolveDepDir) so the import becomes { getDeps,
listNodeModulesPackageDirs, resolveDepDir }, then save and re-run the linter/CI
to confirm the sort-imports rule is satisfied.
In `@packages/intent/src/scanner.ts`:
- Around line 419-437: The dependency walker is dropping source info by calling
tryRegister(...) with only two args so transitive deps inherit the default
'local'; update the walker (created by createDependencyWalker) to track the
source of the package currently being walked and forward it into tryRegister as
the third argument (i.e., call tryRegister(depPath, depName, currentSource)).
Specifically, modify the walk logic invoked by walkWorkspacePackages,
walkKnownPackages and walkProjectDeps so when scanning dependencies of a package
discovered with a non-local source (e.g., after scanTarget(nodeModules.global,
'global')) the propagated source is passed into tryRegister; adjust
createDependencyWalker and any internal helpers to accept/propagate a source
parameter for recursive walks and ensure existing tryRegister call sites (the
ones referenced in the review) are updated to include that source.
---
Outside diff comments:
In `@docs/cli/intent-stale.md`:
- Around line 18-23: The doc text is ambiguous about what happens when no
packages are found; update the bullet list in intent-stale.md so it clearly
states the behavior for JSON mode by replacing or augmenting the line that says
"If no packages are found, prints `No intent-enabled packages found.`" with an
explicit note that the CLI prints that message in text mode but returns an empty
JSON array (`[]`) when invoked with the `--json` flag; mention both outputs and
reference the `--json` option and the exact text message "No intent-enabled
packages found." so readers know the difference.
🪄 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: 452eb54f-c0f3-494a-ae6e-5b0cf84d0542
📒 Files selected for processing (17)
docs/cli/intent-install.mddocs/cli/intent-list.mddocs/cli/intent-stale.mddocs/getting-started/quick-start-maintainers.mddocs/overview.mdpackages/intent/src/cli-support.tspackages/intent/src/cli.tspackages/intent/src/commands/list.tspackages/intent/src/commands/stale.tspackages/intent/src/discovery/index.tspackages/intent/src/discovery/register.tspackages/intent/src/discovery/walk.tspackages/intent/src/index.tspackages/intent/src/scanner.tspackages/intent/src/types.tspackages/intent/tests/cli.test.tspackages/intent/tests/scanner.test.ts
Comment on lines
+18
to
+23
| - Scans project and workspace dependencies for intent-enabled packages and skills | ||
| - Intentionally includes accessible global packages when listing installed skills | ||
| - Includes warnings from discovery | ||
| - If no packages are discovered, prints `No intent-enabled packages found.` | ||
| - Summary line with package count, skill count, and detected package manager | ||
| - Package table columns: `PACKAGE`, `VERSION`, `SKILLS`, `REQUIRES` | ||
| - Package table columns: `PACKAGE`, `SOURCE`, `VERSION`, `SKILLS`, `REQUIRES` |
Contributor
There was a problem hiding this comment.
The discovery scope text is still narrower than the implementation.
scanForIntents() walks workspace packages directly, not just dependencies, so this wording undersells what intent list can return.
📝 Suggested wording
-- Scans project and workspace dependencies for intent-enabled packages and skills
+- Scans the project, workspace packages, and their dependencies for intent-enabled packages and skills🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cli/intent-list.md` around lines 18 - 23, The documentation's "discovery
scope" wording is too narrow; update the intent-list docs to reflect that
scanForIntents() examines workspace packages as well as project and workspace
dependencies and that intent list can also include accessible global packages
and discovery warnings—adjust the bulleted scope lines and the sentence that
states what is scanned so it explicitly mentions "workspace packages (not just
dependencies), workspace/project dependencies, and accessible global packages"
and retain the notes about warnings, summary line, and table columns.
| `@tanstack/intent` is a CLI for shipping and consuming Agent Skills as package artifacts. | ||
|
|
||
| Skills are markdown documents that teach AI coding agents how to use your library correctly. Intent versions them with your releases, ships them inside npm packages, discovers them from local and accessible global `node_modules`, and helps agents load them automatically when working on matching tasks. | ||
| Skills are markdown documents that teach AI coding agents how to use your library correctly. Intent versions them with your releases, ships them inside npm packages, discovers them from your project and workspace by default, and helps agents load them automatically when working on matching tasks. |
Contributor
There was a problem hiding this comment.
Use proper casing for “Markdown”.
Use Markdown (proper noun) for consistency with technical documentation conventions.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...lls as package artifacts.
Skills are markdown documents that teach AI coding agents h...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/overview.md` at line 8, Update the casing of the term "markdown" to the
proper noun "Markdown" in the sentence that defines Skills (the sentence
containing "Skills are markdown documents..."); replace the lowercase instance
with "Markdown" to ensure consistent technical documentation convention across
docs/overview.md.
Comment on lines
+108
to
+123
| const existingDepth = opts.getPackageDepth( | ||
| existing.packageRoot, | ||
| opts.projectRoot, | ||
| ) | ||
| const candidateDepth = opts.getPackageDepth( | ||
| candidate.packageRoot, | ||
| opts.projectRoot, | ||
| ) | ||
| const shouldReplace = | ||
| candidateDepth < existingDepth || | ||
| (candidateDepth === existingDepth && | ||
| opts.comparePackageVersions(candidate.version, existing.version) > 0) | ||
|
|
||
| if (shouldReplace) { | ||
| opts.packages[existingIndex] = candidate | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce local-over-global precedence before depth/version tie-breaking.
The replacement rule ignores candidate.source, so the new “prefer local over global” behavior currently depends on filesystem layout. A hoisted local package scanned from a workspace subdir can tie a nearby explicit global root on getPackageDepth(), and a newer global version will replace the local candidate.
💡 Suggested fix
const shouldReplace =
- candidateDepth < existingDepth ||
- (candidateDepth === existingDepth &&
- opts.comparePackageVersions(candidate.version, existing.version) > 0)
+ existing.source === 'global' && candidate.source === 'local'
+ ? true
+ : existing.source === 'local' && candidate.source === 'global'
+ ? false
+ : candidateDepth < existingDepth ||
+ (candidateDepth === existingDepth &&
+ opts.comparePackageVersions(
+ candidate.version,
+ existing.version,
+ ) > 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/discovery/register.ts` around lines 108 - 123, The
replacement logic for package candidates ignores the package source and can let
a global package replace a local one; update the decision in the register logic
to enforce local-over-global first: before applying the depth/version tie-break,
check candidate.source and existing.source (e.g., if candidate.source ===
'local' and existing.source !== 'local' then set shouldReplace = true, and if
existing.source === 'local' and candidate.source !== 'local' then set
shouldReplace = false), otherwise fall back to the existingDepth/candidateDepth
and opts.comparePackageVersions(existing.version, candidate.version) logic and
then assign into opts.packages[existingIndex] when shouldReplace is true. Ensure
you reference and use existing.packageRoot, candidate.packageRoot,
existingIndex, opts.getPackageDepth and opts.comparePackageVersions as currently
used.
| @@ -0,0 +1,113 @@ | |||
| import { existsSync, readFileSync } from 'node:fs' | |||
| import { join } from 'node:path' | |||
| import { listNodeModulesPackageDirs, resolveDepDir, getDeps } from '../utils.js' | |||
Contributor
There was a problem hiding this comment.
Fix import member ordering to satisfy ESLint.
Line 3 has non-alphabetized named imports (sort-imports error), which can fail lint in CI.
🔧 Proposed fix
-import { listNodeModulesPackageDirs, resolveDepDir, getDeps } from '../utils.js'
+import { getDeps, listNodeModulesPackageDirs, resolveDepDir } from '../utils.js'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { listNodeModulesPackageDirs, resolveDepDir, getDeps } from '../utils.js' | |
| import { getDeps, listNodeModulesPackageDirs, resolveDepDir } from '../utils.js' |
🧰 Tools
🪛 ESLint
[error] 3-3: Member 'getDeps' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/discovery/walk.ts` at line 3, The named imports on the
import line are not alphabetized; reorder the members in the import from
'../utils.js' into alphabetical order (getDeps, listNodeModulesPackageDirs,
resolveDepDir) so the import becomes { getDeps, listNodeModulesPackageDirs,
resolveDepDir }, then save and re-run the linter/CI to confirm the sort-imports
rule is satisfied.
Comment on lines
+419
to
437
| const { walkKnownPackages, walkProjectDeps, walkWorkspacePackages } = | ||
| createDependencyWalker({ | ||
| packages, | ||
| projectRoot, | ||
| workspacePatterns, | ||
| )) { | ||
| const wsNodeModules = join(wsDir, 'node_modules') | ||
| if (existsSync(wsNodeModules)) { | ||
| for (const dirPath of listNodeModulesPackageDirs(wsNodeModules)) { | ||
| tryRegister(dirPath, 'unknown') | ||
| } | ||
| } | ||
|
|
||
| const wsPkg = readPkgJson(wsDir) | ||
| if (wsPkg) { | ||
| walkDepsFromPkgJson(wsPkg, wsDir) | ||
| } | ||
| } | ||
| } | ||
| readPkgJson, | ||
| tryRegister, | ||
| warnings, | ||
| }) | ||
|
|
||
| walkWorkspacePackages() | ||
| walkKnownPackages() | ||
| walkProjectDeps() | ||
|
|
||
| if ( | ||
| explicitGlobalNodeModules || | ||
| packages.length === 0 || | ||
| !nodeModules.local.exists | ||
| ) { | ||
| if (includeGlobal) { | ||
| ensureGlobalNodeModules() | ||
| scanTarget(nodeModules.global) | ||
| scanTarget(nodeModules.global, 'global') | ||
| walkKnownPackages() | ||
| walkProjectDeps() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -p 'walk.ts' packages/intent/src/discovery -x sed -n '1,260p' {}
echo '--- tryRegister call sites in walk.ts ---'
rg -n -C3 '\btryRegister\s*\(' packages/intent/src/discovery/walk.ts
echo '--- global/source coverage in scanner tests ---'
rg -n -C2 'includeGlobal|source' packages/intent/tests/scanner.test.tsRepository: TanStack/intent
Length of output: 4845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tryRegister function definition in scanner.ts ==="
rg -n -B2 -A8 'tryRegister\s*[:=]|function tryRegister' packages/intent/src/scanner.ts | head -50
echo -e "\n=== How scanTarget handles source parameter ==="
rg -n -B3 -A10 'scanTarget' packages/intent/src/scanner.ts | head -80
echo -e "\n=== Source field assignments in scanner.ts ==="
rg -n "source\s*[:=]" packages/intent/src/scanner.tsRepository: TanStack/intent
Length of output: 112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== createDependencyWalker call site in scanner.ts ==="
rg -n -B5 -A15 'createDependencyWalker' packages/intent/src/scanner.ts
echo -e "\n=== Look for tryRegister callback definition in scanner.ts ==="
rg -n 'tryRegister.*=' packages/intent/src/scanner.ts | head -20Repository: TanStack/intent
Length of output: 1336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== createPackageRegistrar implementation ==="
rg -n -B2 -A30 'export function createPackageRegistrar|export const createPackageRegistrar' packages/intent/src/discovery/
echo -e "\n=== Full context around line 402 in scanner.ts ==="
sed -n '395,440p' packages/intent/src/scanner.tsRepository: TanStack/intent
Length of output: 3675
Transitive global packages are misclassified as local sources.
When scanTarget(nodeModules.global, 'global') finds intent packages directly in the global node_modules, they are correctly labeled with source: 'global'. However, walkKnownPackages() and walkProjectDeps() discover dependencies of those packages via the dependency walker in packages/intent/src/discovery/walk.ts.
The walker's tryRegister calls (lines 38, 80, 97) pass only two arguments and omit the third source parameter. Since tryRegister defaults source to 'local', any intent-enabled transitive dependency discovered through a global package will be incorrectly labeled as source: 'local' instead of inheriting source: 'global'.
The walker needs to track and forward source information when registering transitive packages discovered during the dependency walk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/scanner.ts` around lines 419 - 437, The dependency walker
is dropping source info by calling tryRegister(...) with only two args so
transitive deps inherit the default 'local'; update the walker (created by
createDependencyWalker) to track the source of the package currently being
walked and forward it into tryRegister as the third argument (i.e., call
tryRegister(depPath, depName, currentSource)). Specifically, modify the walk
logic invoked by walkWorkspacePackages, walkKnownPackages and walkProjectDeps so
when scanning dependencies of a package discovered with a non-local source
(e.g., after scanTarget(nodeModules.global, 'global')) the propagated source is
passed into tryRegister; adjust createDependencyWalker and any internal helpers
to accept/propagate a source parameter for recursive walks and ensure existing
tryRegister call sites (the ones referenced in the review) are updated to
include that source.
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
intent list, and keepintent stalescoped to local/workspace packages with correct empty JSON outputNotes
intent listnow intentionally includes accessible global packages and shows aSOURCEcolumnintent staledoes not include global packages by default; from a package it checks that package, and from a monorepo root it checks workspace packages with skillsSummary by CodeRabbit
New Features
intent listnow shows a SOURCE column and can include accessible global packages while preferring local versions.Documentation
list,stale, andinstallupdated to describe project, workspace, and optional global scanning and context-dependent stale checks.Tests