fix(intent): make workspace package resolution glob-aware and reusable#92
Closed
LadyBluenotes wants to merge 7 commits intomainfrom
Closed
fix(intent): make workspace package resolution glob-aware and reusable#92LadyBluenotes wants to merge 7 commits intomainfrom
LadyBluenotes wants to merge 7 commits intomainfrom
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughExtracted workspace discovery logic from setup.ts into a new workspace-patterns.ts module; implemented normalized, deduplicated, sorted workspace pattern parsing and segment-by-segment glob resolution that supports Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (intent)
participant Setup as setup.ts
participant Patterns as workspace-patterns.ts
participant FS as Filesystem
CLI->>Setup: run workspace-related command
Setup->>Patterns: findWorkspaceRoot(startPath)
Patterns->>FS: stat/read ancestor dirs for pnpm-workspace.yaml / package.json
FS-->>Patterns: workspace config or null
Patterns-->>Setup: workspace root or null
Setup->>Patterns: readWorkspacePatterns(root)
Patterns->>FS: read pnpm-workspace.yaml or package.json (workspaces)
FS-->>Patterns: raw patterns
Patterns->>Patterns: normalize, dedupe, sort patterns
Setup->>Patterns: resolveWorkspacePackages(root, patterns)
Patterns->>FS: traverse directories (expand segments, skip node_modules/hidden)
FS-->>Patterns: directories containing package.json
Patterns-->>Setup: sorted package directories (exclusions applied)
Setup-->>CLI: packages list / proceed with further actions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
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: 2
🧹 Nitpick comments (1)
packages/intent/tests/workspace-patterns.test.ts (1)
1-8: Add one pnpm-root end-to-end case.This suite is solid for the pure resolver, but the extracted runtime helpers now used by
packages/intent/src/cli-support.tsandpackages/intent/src/setup.tsarefindWorkspaceRootandfindPackagesWithSkills. One test that writespnpm-workspace.yaml, starts from a nested child directory, and asserts only packages withSKILL.mdare returned would cover the moved code path much better.Also applies to: 37-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/workspace-patterns.test.ts` around lines 1 - 8, Add an end-to-end test in packages/intent/tests/workspace-patterns.test.ts that writes a pnpm-workspace.yaml, creates a workspace tree with multiple package folders (only some containing SKILL.md), starts the test from a nested child directory, then exercises the runtime helpers findWorkspaceRoot and findPackagesWithSkills to resolve the workspace and packages; assert that the returned package list is based on pnpm-workspace.yaml and only includes folders that contain SKILL.md. Ensure the test uses temporary directories (mkdtempSync/mkdirSync/writeFileSync), changes the CWD to the nested child for resolution, and restores cleanup with rmSync/afterEach.
🤖 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/workspace-patterns.ts`:
- Around line 72-82: resolveWorkspacePackages currently treats patterns with a
leading '!' as literal segments (e.g., '!packages' after split) which causes
exclusion patterns from pnpm-workspace.yaml to be ignored; change
resolveWorkspacePackages to first partition normalizeWorkspacePatterns(patterns)
into inclusions and exclusions (patterns starting with '!' stripped of the '!'),
call resolveWorkspacePatternSegments(root, ...) separately for each set to
populate two Sets (includedDirs and excludedDirs), then compute the final result
as includedDirs minus excludedDirs and return that sorted array; keep references
to resolveWorkspacePackages, normalizeWorkspacePatterns, and
resolveWorkspacePatternSegments when locating where to implement this logic.
- Line 1: Import binding style violates the rule by mixing a type import inline;
change the import so that existsSync, readFileSync and readdirSync remain in the
runtime import and Dirent is imported in a separate type-only statement (e.g.,
keep the first line importing { existsSync, readFileSync, readdirSync } from
'node:fs' and add a second line `import type { Dirent } from 'node:fs'`) so
functions using Dirent in workspace-patterns.ts reference the type-only import.
---
Nitpick comments:
In `@packages/intent/tests/workspace-patterns.test.ts`:
- Around line 1-8: Add an end-to-end test in
packages/intent/tests/workspace-patterns.test.ts that writes a
pnpm-workspace.yaml, creates a workspace tree with multiple package folders
(only some containing SKILL.md), starts the test from a nested child directory,
then exercises the runtime helpers findWorkspaceRoot and findPackagesWithSkills
to resolve the workspace and packages; assert that the returned package list is
based on pnpm-workspace.yaml and only includes folders that contain SKILL.md.
Ensure the test uses temporary directories
(mkdtempSync/mkdirSync/writeFileSync), changes the CWD to the nested child for
resolution, and restores cleanup with rmSync/afterEach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4aabfb0-1459-4abb-bef5-2f8e744c8323
📒 Files selected for processing (6)
.changeset/busy-peas-fail.mdpackages/intent/src/cli-support.tspackages/intent/src/scanner.tspackages/intent/src/setup.tspackages/intent/src/workspace-patterns.tspackages/intent/tests/workspace-patterns.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…add related tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/intent/src/workspace-patterns.ts (1)
73-83:⚠️ Potential issue | 🟠 Major
resolveWorkspacePackagesstill ignores pnpm exclusion patterns (!…).At Line 79, patterns starting with
!are resolved as literal path segments (e.g.,!packages), so excluded workspaces remain included in results.Suggested fix
export function resolveWorkspacePackages( root: string, patterns: Array<string>, ): Array<string> { - const dirs = new Set<string>() - - for (const pattern of normalizeWorkspacePatterns(patterns)) { - resolveWorkspacePatternSegments(root, pattern.split('/'), dirs) - } - - return [...dirs].sort((a, b) => a.localeCompare(b)) + const includedDirs = new Set<string>() + const excludedDirs = new Set<string>() + + for (const pattern of normalizeWorkspacePatterns(patterns)) { + if (pattern.startsWith('!')) { + const negated = pattern.slice(1) + if (negated) { + resolveWorkspacePatternSegments(root, negated.split('/'), excludedDirs) + } + continue + } + + resolveWorkspacePatternSegments(root, pattern.split('/'), includedDirs) + } + + return [...includedDirs] + .filter((dir) => !excludedDirs.has(dir)) + .sort((a, b) => a.localeCompare(b)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/workspace-patterns.ts` around lines 73 - 83, The function resolveWorkspacePackages currently treats pnpm-style exclusion patterns (starting with '!') as literal segments; update the loop that iterates normalizeWorkspacePatterns(patterns) so that when a pattern startsWith('!') you treat it as an exclusion: call resolveWorkspacePatternSegments with the pattern (stripping the leading '!') into a temporary Set to collect matching dirs, then remove those matches from the main dirs Set (instead of adding them); for non-exclusion patterns keep the existing behavior of resolving into dirs. Reference resolveWorkspacePackages, normalizeWorkspacePatterns, and resolveWorkspacePatternSegments when applying this change.
🧹 Nitpick comments (1)
packages/intent/src/workspace-patterns.ts (1)
145-156: Normalizestartto absolute path for defensive clarity.All current callsites already pass absolute paths, but normalizing to absolute using
resolve()and replacingjoin(dir, '..')withdirname()improves code clarity and guards against future misuse if this function is called with relative paths elsewhere.Suggested improvement
-import { join } from 'node:path' +import { dirname, join, resolve } from 'node:path' ... export function findWorkspaceRoot(start: string): string | null { - let dir = start + let dir = resolve(start) while (true) { if (readWorkspacePatterns(dir)) { return dir } - const next = join(dir, '..') + const next = dirname(dir) if (next === dir) return null dir = next } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/workspace-patterns.ts` around lines 145 - 156, Normalize the input and use dirname traversal: in findWorkspaceRoot, call resolve(start) up-front to ensure start is absolute, then replace the upward-traversal logic that uses join(dir, '..') with dirname(dir) to get the parent; keep the readWorkspacePatterns(dir) check and return dir when it matches, and return null when dirname(dir) === dir to stop. This ensures defensive handling of relative inputs while preserving the existing readWorkspacePatterns behavior.
🤖 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/workspace-patterns.ts`:
- Around line 1-5: The import order in workspace-patterns.ts violates the ESLint
import/order rule because the type import "import type { Dirent } from
'node:fs'" is placed among value imports; move that line to after the other
imports (group with any other type-only imports) so all runtime imports
(existsSync, readFileSync, readdirSync, join, parseYaml, findSkillFiles) come
first and the type import appears last, preserving the same specifier and symbol
name.
---
Duplicate comments:
In `@packages/intent/src/workspace-patterns.ts`:
- Around line 73-83: The function resolveWorkspacePackages currently treats
pnpm-style exclusion patterns (starting with '!') as literal segments; update
the loop that iterates normalizeWorkspacePatterns(patterns) so that when a
pattern startsWith('!') you treat it as an exclusion: call
resolveWorkspacePatternSegments with the pattern (stripping the leading '!')
into a temporary Set to collect matching dirs, then remove those matches from
the main dirs Set (instead of adding them); for non-exclusion patterns keep the
existing behavior of resolving into dirs. Reference resolveWorkspacePackages,
normalizeWorkspacePatterns, and resolveWorkspacePatternSegments when applying
this change.
---
Nitpick comments:
In `@packages/intent/src/workspace-patterns.ts`:
- Around line 145-156: Normalize the input and use dirname traversal: in
findWorkspaceRoot, call resolve(start) up-front to ensure start is absolute,
then replace the upward-traversal logic that uses join(dir, '..') with
dirname(dir) to get the parent; keep the readWorkspacePatterns(dir) check and
return dir when it matches, and return null when dirname(dir) === dir to stop.
This ensures defensive handling of relative inputs while preserving the existing
readWorkspacePatterns behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47d78f7d-32fa-452b-8533-5d8e0d81e9eb
📒 Files selected for processing (1)
packages/intent/src/workspace-patterns.ts
- Return null from parseWorkspacePatterns when normalized result is empty (prevents
empty workspace configs from being treated as valid monorepo roots)
- Replace empty catch {} in setup.ts monorepo detection with console.error warning
- Add absolute-path guard to findWorkspaceRoot
- Add JSDoc to all exported functions in workspace-patterns.ts
- Add tests: pnpm-workspace.yaml reading, YAML precedence, Yarn classic format,
null-return for non-workspace dirs, exclusion normalization, corrupted parent
package.json warning
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
KyleAMathews
approved these changes
Mar 25, 2026
Collaborator
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Ran a comprehensive review (code quality, error handling, test coverage, type design, comments). The workspace patterns extraction is well done — the segment-by-segment glob resolver is a big improvement over the old regex-strip approach.
Applied fixes in 5f38aa7:
workspace-patterns.ts:parseWorkspacePatternsnow returnsnullfor empty normalized arrays — previously returned[](truthy), which causedfindWorkspaceRootto incorrectly identify dirs with empty workspace configs as workspace rootsworkspace-patterns.ts: Added absolute-path guard tofindWorkspaceRootto prevent incorrect traversal on relative pathsworkspace-patterns.ts: Added JSDoc to all 4 exported functions (lost during extraction from setup.ts)setup.ts: Replaced emptycatch {}in monorepo detection withconsole.errorwarning — was silently swallowing all errors including corrupted parent package.json- Tests: Added 7 new test cases covering pnpm-workspace.yaml reading, YAML-over-package.json precedence, Yarn classic
workspaces.packagesformat, null return for non-workspace dirs, empty pattern normalization, exclusion!./normalization, and corrupted parent package.json warning
All 227 tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
*, and**!-prefixed matches from resolved package rootsNotes
This fixes nested workspace patterns like
apps/*/packages/*, which were previously resolved incorrectly because the old resolver stripped glob suffixes and treated the remaining path as literal.It also makes pnpm-style exclusion patterns work during workspace package resolution.
Fixes #88 and lays the groundwork for later shared project/context resolution work.
Summary by CodeRabbit