Cache CLI filesystem reads and de-dupe scanner work#127
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR integrates an IntentFsCache into intent discovery/resolution to memoize package.json reads and skill-file discovery, threads the cache through scanning/walking/resolution codepaths, and exposes packageJsonReadCount/packageJsonCacheHits in scan results and CLI debug output. ChangesFilesystem Caching & Scan Integration
Sequence DiagramsequenceDiagram
participant CLI
participant Core
participant Scanner
participant Registrar
participant FsCache
participant Disk
CLI->>Core: list/load (--debug)
Core->>Scanner: scanForIntents(..., fsCache?)
alt no fsCache
Core->>FsCache: createIntentFsCache()
end
Core->>Scanner: scanForIntents(with fsCache)
Scanner->>FsCache: findSkillFiles(skillsDir)
Scanner->>Registrar: createPackageRegistrar(scanNodeModulesDir)
Registrar->>FsCache: readPackageJsonResult(packageRoot)
FsCache->>Disk: read package.json (cached)
Disk-->>FsCache: package.json content / error
FsCache-->>Registrar: parsed package.json / null
Registrar->>Scanner: register package entries
Scanner-->>Core: ScanResult + stats
Core-->>CLI: debug output includes packageJsonReadCount & packageJsonCacheHits
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 6 minutes and 6 seconds.Comment |
|
View your CI Pipeline Execution ↗ for commit 1685b96
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit d42a075
☁️ Nx Cloud last updated this comment at |
commit: |
Merging this PR will improve performance by 59.96%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | scans a consumer workspace |
61.7 ms | 38.5 ms | +59.96% |
Comparing scanner-fixes (a178ca9) with main (1639791)1
Footnotes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/intent/src/core/load-resolution.ts (1)
3-3: 💤 Low valueESLint
[error]on new import: split type and value imports.The static analysis reports two rule violations on the new line:
sort-imports: thetype IntentFsCachespecifier should precedecreateIntentFsCache.import/consistent-type-specifier-style: the project prefers a separate top-levelimport typestatement over an inlinetypemodifier.🔧 Suggested fix
-import { createIntentFsCache, type IntentFsCache } from '../fs-cache.js' +import type { IntentFsCache } from '../fs-cache.js' +import { createIntentFsCache } from '../fs-cache.js'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/core/load-resolution.ts` at line 3, The import mixes a type and a value which triggers ESLint: split the inline type into a separate top-level type import and ensure the type import comes before the value import; replace the single line importing both with two lines — one "import type { IntentFsCache } from '../fs-cache.js'" and one "import { createIntentFsCache } from '../fs-cache.js'" — so IntentFsCache is imported as a type-only import before the createIntentFsCache value import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/intent/src/utils.ts`:
- Around line 20-28: The current silent catch in the directory read (where
entries = readdirSync(dir, { withFileTypes: true, encoding: 'utf8' })) swallows
failures and makes callers like validate.ts and staleness.ts unable to
distinguish "no skills" from "read failed"; update the catch in the utils.ts
function to either (a) emit a warning/error via the existing logger (e.g.,
processLogger or the module logger) including the directory path and error
details, or (b) set an error flag or counter on the cache/stats object the
function returns so callers (validate.ts, staleness.ts) can detect incomplete
reads — modify the catch block accordingly and ensure the unique symbols
readdirSync usage and the entries variable handling remain consistent so callers
can react to the logged warning or stats flag.
---
Nitpick comments:
In `@packages/intent/src/core/load-resolution.ts`:
- Line 3: The import mixes a type and a value which triggers ESLint: split the
inline type into a separate top-level type import and ensure the type import
comes before the value import; replace the single line importing both with two
lines — one "import type { IntentFsCache } from '../fs-cache.js'" and one
"import { createIntentFsCache } from '../fs-cache.js'" — so IntentFsCache is
imported as a type-only import before the createIntentFsCache value import.
🪄 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: 16b8a89b-19b2-491a-8e18-46f8fbbc194c
📒 Files selected for processing (14)
packages/intent/src/commands/list.tspackages/intent/src/commands/load.tspackages/intent/src/core.tspackages/intent/src/core/load-resolution.tspackages/intent/src/core/types.tspackages/intent/src/discovery/walk.tspackages/intent/src/fs-cache.tspackages/intent/src/scanner.tspackages/intent/src/types.tspackages/intent/src/utils.tspackages/intent/tests/cli.test.tspackages/intent/tests/core.test.tspackages/intent/tests/fs-cache.test.tspackages/intent/tests/scanner.test.ts
|
|
||
| let entries: Array<Dirent<string>> | ||
| try { | ||
| entries = readdirSync(dir, { withFileTypes: true, encoding: 'utf8' }) | ||
| } catch { | ||
| return files | ||
| } | ||
|
|
||
| for (const entry of entries) { |
There was a problem hiding this comment.
Silent error handling may mask failures from callers.
The try/catch returns an empty array on directory read failure, but callers like validate.ts and staleness.ts (see relevant code snippets) treat the returned array as authoritative with no way to distinguish "no skills found" from "directory read failed". This could cause:
validate.ts: Passing validation when subdirectories failed to readstaleness.ts: Incomplete skill metadata collection
Consider either logging a warning on catch, or tracking errors in the cache stats so callers can detect incomplete results.
💡 Suggested improvement: Add warning on read failure
let entries: Array<Dirent<string>>
try {
entries = readdirSync(dir, { withFileTypes: true, encoding: 'utf8' })
} catch {
+ // Optionally log or track that this directory failed to read
+ // console.warn(`Warning: Could not read directory ${dir}`)
return files
}📝 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.
| let entries: Array<Dirent<string>> | |
| try { | |
| entries = readdirSync(dir, { withFileTypes: true, encoding: 'utf8' }) | |
| } catch { | |
| return files | |
| } | |
| for (const entry of entries) { | |
| let entries: Array<Dirent<string>> | |
| try { | |
| entries = readdirSync(dir, { withFileTypes: true, encoding: 'utf8' }) | |
| } catch { | |
| // Optionally log or track that this directory failed to read | |
| // console.warn(`Warning: Could not read directory ${dir}`) | |
| return files | |
| } | |
| for (const entry of entries) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/utils.ts` around lines 20 - 28, The current silent catch
in the directory read (where entries = readdirSync(dir, { withFileTypes: true,
encoding: 'utf8' })) swallows failures and makes callers like validate.ts and
staleness.ts unable to distinguish "no skills" from "read failed"; update the
catch in the utils.ts function to either (a) emit a warning/error via the
existing logger (e.g., processLogger or the module logger) including the
directory path and error details, or (b) set an error flag or counter on the
cache/stats object the function returns so callers (validate.ts, staleness.ts)
can detect incomplete reads — modify the catch block accordingly and ensure the
unique symbols readdirSync usage and the entries variable handling remain
consistent so callers can react to the logged warning or stats flag.
LadyBluenotes
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/intent/tests/scanner.test.ts (2)
122-129: ⚡ Quick winAssert a real cache hit in this fixture.
These checks only prove that
statsis wired through. This scan rereads the discovered package during dependency walking, sopackageJsonCacheHitsshould also be> 0here; otherwise a regression that bypasses the cache would still pass.Suggested test tightening
expect(result.stats).toEqual( expect.objectContaining({ packageJsonReadCount: expect.any(Number), packageJsonCacheHits: expect.any(Number), }), ) expect(result.stats!.packageJsonReadCount).toBeGreaterThan(0) + expect(result.stats!.packageJsonCacheHits).toBeGreaterThan(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/scanner.test.ts` around lines 122 - 129, The test currently only asserts that stats are present and that packageJsonReadCount > 0, but doesn't verify an actual cache hit; update the assertion for the scan result (referencing result.stats and the packageJsonCacheHits stat) to assert that packageJsonCacheHits is greater than zero (e.g., expect(result.stats!.packageJsonCacheHits).toBeGreaterThan(0)) so the fixture validates that the package.json cache was used during dependency walking.
131-149: ⚡ Quick winCover the
scanIntentPackageAtRoot()fallback path too.This only exercises the full scan.
packages/intent/src/scanner.tsnow routes the single-package fallback through the same cached skill discovery branch, so adding the same non-directoryskillscase there would protect the load fallback from regressing independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/scanner.test.ts` around lines 131 - 149, Add a test that exercises the single-package fallback path by creating a package at the repository root with a package.json containing an intent block and a non-directory "skills" file, then call scanIntentPackageAtRoot (in addition to the existing scanForIntents test) and assert the returned package exists and its skills array is empty; this mirrors the existing test but targets the fallback branch in packages/intent/src/scanner.ts to prevent regressions in scanIntentPackageAtRoot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/intent/tests/scanner.test.ts`:
- Around line 122-129: The test currently only asserts that stats are present
and that packageJsonReadCount > 0, but doesn't verify an actual cache hit;
update the assertion for the scan result (referencing result.stats and the
packageJsonCacheHits stat) to assert that packageJsonCacheHits is greater than
zero (e.g., expect(result.stats!.packageJsonCacheHits).toBeGreaterThan(0)) so
the fixture validates that the package.json cache was used during dependency
walking.
- Around line 131-149: Add a test that exercises the single-package fallback
path by creating a package at the repository root with a package.json containing
an intent block and a non-directory "skills" file, then call
scanIntentPackageAtRoot (in addition to the existing scanForIntents test) and
assert the returned package exists and its skills array is empty; this mirrors
the existing test but targets the fallback branch in
packages/intent/src/scanner.ts to prevent regressions in
scanIntentPackageAtRoot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c7e601b-1570-4ae1-9fb0-64438e1b1f44
📒 Files selected for processing (4)
packages/intent/src/discovery/register.tspackages/intent/src/discovery/walk.tspackages/intent/src/scanner.tspackages/intent/tests/scanner.test.ts
1 participant
Summary
This PR reduces repeated filesystem work during Intent CLI scans while preserving existing discovery behavior.
skillspathsBehavior Notes
This does not make broad
node_modulesscanning fallback-only. The scanner still enumerates the samenode_modulesdirectories as before; it only skips package roots or scan targets already attempted in the same scan.Package candidates are de-duped by resolved package root path, not by package name, so multiple installed versions of the same package are still inspected and tracked as variants.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor