refactor(intent): add shared project context resolver for validate and edit-package-json#93
Merged
LadyBluenotes merged 11 commits intomainfrom Mar 25, 2026
Merged
refactor(intent): add shared project context resolver for validate and edit-package-json#93LadyBluenotes merged 11 commits intomainfrom
LadyBluenotes merged 11 commits intomainfrom
Conversation
…e packaging warnings
… improving monorepo detection
…add related tests
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThe PR refactors monorepo and workspace detection by extracting shared logic into dedicated modules. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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)
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: |
- Add process.exitCode = 1 on JSON parse failure in setup.ts (was silently returning success) - Skip _artifacts validation for monorepo packages in validate.ts (artifacts live at workspace root) - Add JSDoc to resolveProjectContext explaining dual-fallback resolution strategy - Add comment to resolveWorkspacePatternSegments describing glob matching behavior - Add standalone (non-monorepo) project test for resolveProjectContext Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
KyleAMathews
approved these changes
Mar 24, 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). Changes look solid — the refactoring successfully consolidates three separate monorepo-detection implementations into a clean shared ProjectContext.
Applied a few fixes in 094609e:
setup.ts: Added missingprocess.exitCode = 1on JSON parse failure (was silently returning success, breaking CI detection)validate.ts: Skip_artifactsvalidation for monorepo packages (artifacts live at workspace root, not under each package)project-context.ts: Added JSDoc explaining the dual-fallback workspace root resolution strategyworkspace-patterns.ts: Added comment on the recursive glob segment matcherproject-context.test.ts: Added test for standalone (non-monorepo) project path — was previously untested at the unit level
All 226 tests pass.
![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 (3)
packages/intent/src/commands/validate.ts (1)
5-8: Split the mixed type/value import.ESLint is already erroring on this form (
import/consistent-type-specifier-style), so CI will keep failing untilProjectContextmoves to a separateimport type.Suggested fix
-import { - type ProjectContext, - resolveProjectContext, -} from '../core/project-context.js' +import { resolveProjectContext } from '../core/project-context.js' +import type { ProjectContext } from '../core/project-context.js'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/commands/validate.ts` around lines 5 - 8, The current import mixes a type and a value which triggers ESLint's import/consistent-type-specifier-style; split the import into a type-only import for ProjectContext and a separate value import for resolveProjectContext by replacing the single mixed import with an "import type { ProjectContext } from '../core/project-context.js'" and a normal "import { resolveProjectContext } from '../core/project-context.js'"; ensure you only change the import statements and keep the module path and identifiers (ProjectContext, resolveProjectContext) intact.packages/intent/tests/setup.test.ts (1)
188-220: Add the workspace-root package case too.These tests lock in the workspace-package paths, but not the case where the workspace root itself owns
skills/. If the new context signal is reused there,runEditPackageJson(monoRoot)can incorrectly omit!skills/_artifactsfor the root package as well, so I'd add that regression before this lands.Based on learnings:
!skills/_artifactsshould be omitted only for sub-packages inside a monorepo, not for the workspace root package itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/setup.test.ts` around lines 188 - 220, Add a new test case that runs runEditPackageJson against the monorepo root (created by createMonorepo) to verify that the workspace-root package keeps the '!skills/_artifacts' exclusion; call runEditPackageJson(monoRoot), assert result.added includes 'files: "skills"', then read the root package.json and assert pkg.files contains both 'skills' and '!skills/_artifacts' (opposite of the sub-package assertions), and finally clean up with rmSync(monoRoot, { recursive: true, force: true }).packages/intent/tests/workspace-patterns.test.ts (1)
34-37: Restore every saved cwd inafterEach.
withCwd()is stack-based, but this cleanup only unwinds one frame. A future nested helper call would leakprocess.cwd()into the next test.♻️ Suggested tweak
afterEach(() => { - if (cwdStack.length > 0) { - process.chdir(cwdStack.pop()!) - } + while (cwdStack.length > 0) { + process.chdir(cwdStack.pop()!) + } for (const root of roots.splice(0)) {🤖 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 34 - 37, The afterEach cleanup only pops a single cwd from cwdStack, which can leak nested withCwd() changes; modify the afterEach hook that references cwdStack and process.chdir so it fully unwinds the stack (e.g., while (cwdStack.length > 0) pop and chdir each saved cwd) to restore every saved cwd and ensure no process.cwd() state is leaked between tests.
🤖 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/commands/validate.ts`:
- Around line 69-74: The check for `_artifacts` uses context.isMonorepo which is
too broad; change the gate to detect whether the target package is a workspace
sub-package (i.e., not the workspace root). Replace occurrences that read
`!context.isMonorepo` (used with `files.includes('!skills/_artifacts')` and the
duplicate block at 171-173) with a condition that checks the package is inside a
workspace but not the workspace root, e.g. verify `context.workspaceRoot &&
context.workspaceRoot !== context.packageRoot` (or use an explicit
`context.isWorkspaceSubpackage` boolean if available) so only sub-packages
skip/require the `!skills/_artifacts` validation and warning.
In `@packages/intent/src/core/project-context.ts`:
- Around line 35-47: The isMonorepo flag incorrectly uses workspaceRoot !==
null; change it so it only becomes true when a workspace exists AND the current
package is inside it (i.e. packageRoot is not the workspace root). Update the
returned isMonorepo expression in project-context.ts to check both workspaceRoot
!== null and packageRoot !== workspaceRoot (or equivalent null-safe comparison)
so the root package is not treated as a sub-package by callers like validate and
runEditPackageJson.
---
Nitpick comments:
In `@packages/intent/src/commands/validate.ts`:
- Around line 5-8: The current import mixes a type and a value which triggers
ESLint's import/consistent-type-specifier-style; split the import into a
type-only import for ProjectContext and a separate value import for
resolveProjectContext by replacing the single mixed import with an "import type
{ ProjectContext } from '../core/project-context.js'" and a normal "import {
resolveProjectContext } from '../core/project-context.js'"; ensure you only
change the import statements and keep the module path and identifiers
(ProjectContext, resolveProjectContext) intact.
In `@packages/intent/tests/setup.test.ts`:
- Around line 188-220: Add a new test case that runs runEditPackageJson against
the monorepo root (created by createMonorepo) to verify that the workspace-root
package keeps the '!skills/_artifacts' exclusion; call
runEditPackageJson(monoRoot), assert result.added includes 'files: "skills"',
then read the root package.json and assert pkg.files contains both 'skills' and
'!skills/_artifacts' (opposite of the sub-package assertions), and finally clean
up with rmSync(monoRoot, { recursive: true, force: true }).
In `@packages/intent/tests/workspace-patterns.test.ts`:
- Around line 34-37: The afterEach cleanup only pops a single cwd from cwdStack,
which can leak nested withCwd() changes; modify the afterEach hook that
references cwdStack and process.chdir so it fully unwinds the stack (e.g., while
(cwdStack.length > 0) pop and chdir each saved cwd) to restore every saved cwd
and ensure no process.cwd() state is leaked between tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0f8dd4d-b037-4053-8824-43a6019bcb54
📒 Files selected for processing (12)
.changeset/busy-peas-fail.md.changeset/red-regions-shine.mdpackages/intent/src/cli-support.tspackages/intent/src/commands/validate.tspackages/intent/src/core/project-context.tspackages/intent/src/scanner.tspackages/intent/src/setup.tspackages/intent/src/workspace-patterns.tspackages/intent/tests/cli.test.tspackages/intent/tests/project-context.test.tspackages/intent/tests/setup.test.tspackages/intent/tests/workspace-patterns.test.ts
Comment on lines
69
to
74
| // In monorepos, _artifacts lives at repo root, not under packages — | ||
| // the negation pattern is a no-op and shouldn't be added. | ||
| const isMonorepoPkg = isInsideMonorepo(root) | ||
|
|
||
| if (!isMonorepoPkg && !files.includes('!skills/_artifacts')) { | ||
| if (!context.isMonorepo && !files.includes('!skills/_artifacts')) { | ||
| warnings.push( | ||
| '"!skills/_artifacts" is not in the "files" array — artifacts will be published unnecessarily', | ||
| ) |
Contributor
There was a problem hiding this comment.
context.isMonorepo is too broad for the _artifacts gate.
resolveProjectContext marks the workspace root package as monorepo too, so these branches now skip the local skills/_artifacts validation and !skills/_artifacts warning for a root package that still needs them. Gate this on “target package is a workspace sub-package” instead of “a workspace root exists”.
Suggested direction
+const isWorkspaceSubpackage =
+ context.packageRoot !== null &&
+ context.workspaceRoot !== null &&
+ context.packageRoot !== context.workspaceRoot
+
// In monorepos, _artifacts lives at repo root, not under packages —
// the negation pattern is a no-op and shouldn't be added.
- if (!context.isMonorepo && !files.includes('!skills/_artifacts')) {
+ if (!isWorkspaceSubpackage && !files.includes('!skills/_artifacts')) {
warnings.push(
'"!skills/_artifacts" is not in the "files" array — artifacts will be published unnecessarily',
)
}
@@
const artifactsDir = join(skillsDir, '_artifacts')
-if (!context.isMonorepo && existsSync(artifactsDir)) {
+if (!isWorkspaceSubpackage && existsSync(artifactsDir)) {Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/validate.ts` around lines 69 - 74, The check for
`_artifacts` uses context.isMonorepo which is too broad; change the gate to
detect whether the target package is a workspace sub-package (i.e., not the
workspace root). Replace occurrences that read `!context.isMonorepo` (used with
`files.includes('!skills/_artifacts')` and the duplicate block at 171-173) with
a condition that checks the package is inside a workspace but not the workspace
root, e.g. verify `context.workspaceRoot && context.workspaceRoot !==
context.packageRoot` (or use an explicit `context.isWorkspaceSubpackage` boolean
if available) so only sub-packages skip/require the `!skills/_artifacts`
validation and warning.
Comment on lines
+35
to
+47
| const workspaceRoot = | ||
| findWorkspaceRoot(packageRoot ?? resolvedTargetPath) ?? | ||
| findWorkspaceRoot(resolvedCwd) | ||
| const workspacePatterns = workspaceRoot | ||
| ? (readWorkspacePatterns(workspaceRoot) ?? []) | ||
| : [] | ||
|
|
||
| return { | ||
| cwd: resolvedCwd, | ||
| workspaceRoot, | ||
| packageRoot, | ||
| isMonorepo: workspaceRoot !== null, | ||
| workspacePatterns, |
Contributor
There was a problem hiding this comment.
Don’t mark the workspace root package as isMonorepo.
workspaceRoot !== null conflates “a workspace exists” with “this package is inside the workspace”. When packageRoot === workspaceRoot, callers like validate and runEditPackageJson will treat the root package like a sub-package and skip the !skills/_artifacts handling that still needs to happen there.
[suggested fix]
🐛 Suggested fix
const workspaceRoot =
findWorkspaceRoot(packageRoot ?? resolvedTargetPath) ??
findWorkspaceRoot(resolvedCwd)
+ const isMonorepo =
+ packageRoot !== null &&
+ workspaceRoot !== null &&
+ workspaceRoot !== packageRoot
const workspacePatterns = workspaceRoot
? (readWorkspacePatterns(workspaceRoot) ?? [])
: []
return {
cwd: resolvedCwd,
workspaceRoot,
packageRoot,
- isMonorepo: workspaceRoot !== null,
+ isMonorepo,Based on learnings: packaging-related monorepo checks should answer whether the package is inside the workspace, not whether the package root itself is the workspace root.
📝 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
| const workspaceRoot = | |
| findWorkspaceRoot(packageRoot ?? resolvedTargetPath) ?? | |
| findWorkspaceRoot(resolvedCwd) | |
| const workspacePatterns = workspaceRoot | |
| ? (readWorkspacePatterns(workspaceRoot) ?? []) | |
| : [] | |
| return { | |
| cwd: resolvedCwd, | |
| workspaceRoot, | |
| packageRoot, | |
| isMonorepo: workspaceRoot !== null, | |
| workspacePatterns, | |
| const workspaceRoot = | |
| findWorkspaceRoot(packageRoot ?? resolvedTargetPath) ?? | |
| findWorkspaceRoot(resolvedCwd) | |
| const isMonorepo = | |
| packageRoot !== null && | |
| workspaceRoot !== null && | |
| workspaceRoot !== packageRoot | |
| const workspacePatterns = workspaceRoot | |
| ? (readWorkspacePatterns(workspaceRoot) ?? []) | |
| : [] | |
| return { | |
| cwd: resolvedCwd, | |
| workspaceRoot, | |
| packageRoot, | |
| isMonorepo, | |
| workspacePatterns, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/core/project-context.ts` around lines 35 - 47, The
isMonorepo flag incorrectly uses workspaceRoot !== null; change it so it only
becomes true when a workspace exists AND the current package is inside it (i.e.
packageRoot is not the workspace root). Update the returned isMonorepo
expression in project-context.ts to check both workspaceRoot !== null and
packageRoot !== workspaceRoot (or equivalent null-safe comparison) so the root
package is not treated as a sub-package by callers like validate and
runEditPackageJson.
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
resolveProjectContext()helper to centralize workspace root, package root, monorepo, workspace pattern, and target path resolutionvalidateandedit-package-jsonthrough the shared resolver instead of using command-specific monorepo detectionpnpm-workspace.yamlwas ignored, leading to false!skills/_artifactswarnings and incorrect package targeting*, and**!-prefixed matches from resolved package rootsNotes
resolveProjectContext()now provides one consistent answer for:package.jsonpathskillsdirectoryvalidatenow makes packaging decisions from the resolved target package rather thanprocess.cwd()alonerunEditPackageJson()now updates the owning package for a target path, including workspace packageskillsdirectoriespackages/foo/skillstargetingpnpm-workspace.yamlSummary by CodeRabbit
Release Notes
Bug Fixes
*and**wildcards).Tests