refactor: migrate CLI to cac and fix monorepo workflow generation#85
Merged
LadyBluenotes merged 16 commits intomainfrom Mar 17, 2026
Merged
refactor: migrate CLI to cac and fix monorepo workflow generation#85LadyBluenotes merged 16 commits intomainfrom
LadyBluenotes merged 16 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 185fb56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughReplaces the in-file Intent CLI with a cac-based command dispatcher, splits commands into modular handlers, adds CLI error/support utilities, and updates setup logic and template generation to be monorepo-aware (workspace root detection, PACKAGE_LABEL/PAYLOAD_PACKAGE/WATCH_PATHS, and repo-level workflow placement). Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client
participant CLI as Intent CLI
participant Cmd as Command Module
participant FS as File System
participant GH as Workflow Renderer
User->>CLI: run `intent setup-github-actions` (or other command)
CLI->>Cmd: parse args (cac) and dispatch to runSetupGithubActionsCommand
Cmd->>FS: detect workspace root, read package.json(s)
FS-->>Cmd: return workspace & package info
Cmd->>GH: render workflow templates with substitutions (PACKAGE_LABEL, PAYLOAD_PACKAGE, WATCH_PATHS)
GH-->>Cmd: rendered workflow content
Cmd->>FS: write workflows to workspace root (skip if exists)
Cmd-->>CLI: return status
CLI-->>User: print status / messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/intent/src/commands/list.ts (1)
44-51: Deferdisplay.jsimport until non-JSON output path.Line 44 eagerly imports table/tree helpers even when
--jsonreturns early. Moving the import below the JSON branch keeps the fast path leaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/src/commands/list.ts` around lines 44 - 51, The import of computeSkillNameWidth, printSkillTree, and printTable from '../display.js' is done before checking options.json, causing unnecessary module loading on the JSON fast path; to fix, move the dynamic import of those symbols to after the if (options.json) { ... return } block so you only await import('../display.js') when options.json is false (keep the call to scanIntentsOrFail and the JSON output unchanged and reference computeSkillNameWidth, printSkillTree, printTable only after that relocated 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/package.json`:
- Line 41: The smoke-test script uses POSIX redirection ("> /dev/null") which
breaks on Windows; update the "test:smoke" npm script to use a cross-platform
Node-based suppression instead of shell redirection. Replace the current value
of the "test:smoke" script with a Node one-liner that runs "dist/cli.mjs --help"
via child_process (e.g., spawnSync or execSync) with stdio set to "ignore", so
the command exits successfully on all platforms while suppressing output; locate
the "test:smoke" script entry in package.json and update it accordingly.
In `@packages/intent/src/cli-support.ts`:
- Around line 40-42: The path resolution for targetDir uses join(process.cwd(),
targetDir) which incorrectly concatenates when targetDir is absolute; change to
use resolve(process.cwd(), targetDir) (or simply resolve(targetDir) if
appropriate) so absolute targetDir resets to the filesystem root; update the
variable resolvedRoot and any related tests/usages to import/use path.resolve
instead of path.join and ensure behavior remains the same for relative inputs.
In `@packages/intent/src/commands/meta.ts`:
- Around line 61-62: The usage footer prints a hard-coded path to SKILL.md which
can be incorrect; change it to derive the path from the existing metaDir
variable used in this module. Replace the second console.log that currently
prints 'node_modules/@tanstack/intent/meta/<name>/SKILL.md' with one that
constructs the path using metaDir and the meta name (the same value used
elsewhere in this file), e.g. join metaDir with the meta name and 'SKILL.md'
before logging so the hint always reflects the real install location.
- Around line 37-59: Wrap filesystem reads and frontmatter parsing in try/catch
so one bad skill doesn't abort the whole listing: guard the initial
readdirSync(metaDir, ...) call with try/catch and log/exit only on fatal errors,
and inside the for (const entry of entries) loop wrap the
parseFrontmatter(skillFile) (and any read of SKILL.md) in a try/catch; if
parseFrontmatter throws or the file is unreadable/invalid, console.warn a brief
message including entry.name and the error, then continue to the next entry and
skip printing that skill. Ensure you still build shortDesc from a safe
description string and only call .replace/.slice on validated strings returned
from parseFrontmatter.
In `@packages/intent/src/commands/scaffold.ts`:
- Line 63: Update the scaffold prompt text to use the consistent artifact
directory name `_artifacts/` instead of `artifacts`; locate the prompt string in
packages/intent/src/commands/scaffold.ts (the scaffold command/prompt generation
code that emits the step "2. Commit skills/ and artifacts") and change that
occurrence to "2. Commit skills/ and _artifacts/" so it matches the earlier
guidance and avoids operator confusion.
In `@packages/intent/src/commands/stale.ts`:
- Around line 12-20: The early return when reports.length === 0 bypasses JSON
output; update the logic in the stale command so that the options.json branch
runs before the plain-text early exit (or explicitly emit JSON empty array when
options.json is true). Concretely, adjust the block that checks reports.length
=== 0 and the options.json handling so that when options.json is true you always
call console.log(JSON.stringify(reports, null, 2)) (yielding [] for an empty
reports array) and then return; keep the existing plain text "No intent-enabled
packages found." only for the non-JSON path.
In `@packages/intent/src/commands/validate.ts`:
- Around line 66-79: The monorepo detection in isMonorepoPkg prematurely returns
false when encountering a parent package.json that exists but lacks workspaces;
change the logic so that after reading/parsing parent (parentPkg -> parent) you
only return true when Array.isArray(parent.workspaces) ||
parent.workspaces?.packages is truthy, otherwise continue the ancestor loop (do
not return false), and likewise on JSON parse errors (catch) continue to the
next ancestor instead of returning false; this ensures isMonorepoPkg checks
higher-up package.json files before deciding it is not a monorepo.
- Around line 154-167: The code currently assumes parseYaml(match[1]) returns an
object; instead after parsing (parseYaml) validate the type of fm by checking
typeof fm === 'object' && fm !== null && !Array.isArray(fm); if that check fails
push errors.push({ file: rel, message: 'Invalid YAML frontmatter' }) and
continue, otherwise cast fm to Record<string, unknown> and then perform the
existing checks for fm.name and fm.description (optionally tighten those checks
to typeof fm.name !== 'string' / typeof fm.description !== 'string' if you want
to require strings). This uses the existing symbols parseYaml, fm, errors, rel
and the name/description checks in validate.ts.
---
Nitpick comments:
In `@packages/intent/src/commands/list.ts`:
- Around line 44-51: The import of computeSkillNameWidth, printSkillTree, and
printTable from '../display.js' is done before checking options.json, causing
unnecessary module loading on the JSON fast path; to fix, move the dynamic
import of those symbols to after the if (options.json) { ... return } block so
you only await import('../display.js') when options.json is false (keep the call
to scanIntentsOrFail and the JSON output unchanged and reference
computeSkillNameWidth, printSkillTree, printTable only after that relocated
import).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03fb6035-0f69-4455-9349-c387f59c5188
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.changeset/thin-rings-behave.mddocs/cli/intent-setup.mdpackages/intent/package.jsonpackages/intent/src/cli-error.tspackages/intent/src/cli-support.tspackages/intent/src/cli.tspackages/intent/src/commands/edit-package-json.tspackages/intent/src/commands/install.tspackages/intent/src/commands/list.tspackages/intent/src/commands/meta.tspackages/intent/src/commands/scaffold.tspackages/intent/src/commands/setup-github-actions.tspackages/intent/src/commands/stale.tspackages/intent/src/commands/validate.tspackages/intent/src/intent-library.tspackages/intent/src/setup.tspackages/intent/tests/cli.test.tspackages/intent/tests/setup.test.ts
| "scripts": { | ||
| "prepack": "npm run build", | ||
| "build": "tsdown src/index.ts src/cli.ts src/setup.ts src/intent-library.ts src/library-scanner.ts --format esm --dts", | ||
| "test:smoke": "pnpm run build && node dist/cli.mjs --help > /dev/null", |
There was a problem hiding this comment.
Use a cross-platform smoke test command.
Line 41 uses > /dev/null, which is POSIX-specific and fails in Windows shells. Prefer keeping output or using a Node-based suppression strategy.
Suggested minimal fix
- "test:smoke": "pnpm run build && node dist/cli.mjs --help > /dev/null",
+ "test:smoke": "pnpm run build && node dist/cli.mjs --help",📝 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
| "test:smoke": "pnpm run build && node dist/cli.mjs --help > /dev/null", | |
| "test:smoke": "pnpm run build && node dist/cli.mjs --help", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/package.json` at line 41, The smoke-test script uses POSIX
redirection ("> /dev/null") which breaks on Windows; update the "test:smoke" npm
script to use a cross-platform Node-based suppression instead of shell
redirection. Replace the current value of the "test:smoke" script with a Node
one-liner that runs "dist/cli.mjs --help" via child_process (e.g., spawnSync or
execSync) with stdio set to "ignore", so the command exits successfully on all
platforms while suppressing output; locate the "test:smoke" script entry in
package.json and update it accordingly.
Comment on lines
+40
to
+42
| const resolvedRoot = targetDir | ||
| ? join(process.cwd(), targetDir) | ||
| : process.cwd() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, what is the behavior difference between path.join(process.cwd(), absolutePath) and path.resolve(process.cwd(), absolutePath), especially when absolutePath starts with "/"?
💡 Result:
-
path.join(process.cwd(), absolutePath)always concatenates the segments and then normalizes. So ifabsolutePathstarts with/, it’s treated like “a segment that happens to begin with a separator”, not as “reset to filesystem root”. Example (POSIX):path.join('/home/me/app', '/tmp')→/home/me/app/tmp(the//becomes/during normalization). [1] -
path.resolve(process.cwd(), absolutePath)resolves segments right-to-left and stops once it hits an absolute path; any earlier segments (likeprocess.cwd()) are discarded. So ifabsolutePathstarts with/, it wins. Example (POSIX):path.resolve('/home/me/app', '/tmp/file/')→/tmp/file. [2]
So when absolutePath starts with /:
join(cwd, absolutePath)≈ “force it undercwd”resolve(cwd, absolutePath)≈ “useabsolutePathas-is (absolute overrides)”
[1] (nodejs.org)
[2] (nodejs.org)
Citations:
- 1: https://nodejs.org/download/release/v22.13.0/docs/api/path.html
- 2: https://nodejs.org/download/release/v22.13.0/docs/api/path.html
🏁 Script executed:
cat -n packages/intent/src/cli-support.ts | head -50Repository: TanStack/intent
Length of output: 1816
🏁 Script executed:
rg -A 2 "resolveStaleTargets" --type tsRepository: TanStack/intent
Length of output: 1028
🏁 Script executed:
grep -A 10 -B 5 "targetDir" packages/intent/src/cli.ts | head -40Repository: TanStack/intent
Length of output: 531
Use resolve() instead of join() for targetDir path resolution.
When targetDir is an absolute path (e.g., stale /tmp/project), join(process.cwd(), targetDir) concatenates the segments incorrectly. The / in the absolute path is treated as a regular path segment, not a filesystem root reset. Use resolve() instead, which correctly handles absolute paths by stopping resolution once an absolute component is encountered.
🔧 Proposed fix
-import { dirname, join, relative } from 'node:path'
+import { dirname, join, relative, resolve } from 'node:path'
@@
- const resolvedRoot = targetDir
- ? join(process.cwd(), targetDir)
- : process.cwd()
+ const resolvedRoot = targetDir
+ ? resolve(process.cwd(), targetDir)
+ : process.cwd()📝 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 resolvedRoot = targetDir | |
| ? join(process.cwd(), targetDir) | |
| : process.cwd() | |
| import { dirname, join, relative, resolve } from 'node:path' | |
| // ... other code ... | |
| const resolvedRoot = targetDir | |
| ? resolve(process.cwd(), targetDir) | |
| : process.cwd() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/cli-support.ts` around lines 40 - 42, The path resolution
for targetDir uses join(process.cwd(), targetDir) which incorrectly concatenates
when targetDir is absolute; change to use resolve(process.cwd(), targetDir) (or
simply resolve(targetDir) if appropriate) so absolute targetDir resets to the
filesystem root; update the variable resolvedRoot and any related tests/usages
to import/use path.resolve instead of path.join and ensure behavior remains the
same for relative inputs.
Comment on lines
+37
to
+59
| const entries = readdirSync(metaDir, { withFileTypes: true }) | ||
| .filter((entry) => entry.isDirectory()) | ||
| .filter((entry) => existsSync(join(metaDir, entry.name, 'SKILL.md'))) | ||
|
|
||
| if (entries.length === 0) { | ||
| console.log('No meta-skills found.') | ||
| return | ||
| } | ||
|
|
||
| console.log('Meta-skills (for library maintainers):\n') | ||
|
|
||
| for (const entry of entries) { | ||
| const skillFile = join(metaDir, entry.name, 'SKILL.md') | ||
| const fm = parseFrontmatter(skillFile) | ||
| let description = '' | ||
| if (typeof fm?.description === 'string') { | ||
| description = fm.description.replace(/\s+/g, ' ').trim() | ||
| } | ||
|
|
||
| const shortDesc = | ||
| description.length > 60 ? `${description.slice(0, 57)}...` : description | ||
| console.log(` ${entry.name.padEnd(28)} ${shortDesc}`) | ||
| } |
There was a problem hiding this comment.
Make listing mode tolerant to unreadable/invalid SKILL.md files.
A single readdirSync/parseFrontmatter failure currently aborts the whole intent meta listing. This should degrade gracefully so one bad skill doesn’t hide all others.
Proposed hardening
- const entries = readdirSync(metaDir, { withFileTypes: true })
- .filter((entry) => entry.isDirectory())
- .filter((entry) => existsSync(join(metaDir, entry.name, 'SKILL.md')))
+ let entries
+ try {
+ entries = readdirSync(metaDir, { withFileTypes: true })
+ .filter((entry) => entry.isDirectory())
+ .filter((entry) => existsSync(join(metaDir, entry.name, 'SKILL.md')))
+ } catch (err) {
+ const msg = err instanceof Error ? err.message : String(err)
+ fail(`Failed to read meta-skills directory: ${msg}`)
+ }
if (entries.length === 0) {
console.log('No meta-skills found.')
return
}
console.log('Meta-skills (for library maintainers):\n')
for (const entry of entries) {
const skillFile = join(metaDir, entry.name, 'SKILL.md')
- const fm = parseFrontmatter(skillFile)
+ let fm: { description?: unknown } | undefined
+ try {
+ fm = parseFrontmatter(skillFile)
+ } catch {
+ fm = undefined
+ }
let description = ''
if (typeof fm?.description === 'string') {
description = fm.description.replace(/\s+/g, ' ').trim()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/meta.ts` around lines 37 - 59, Wrap filesystem
reads and frontmatter parsing in try/catch so one bad skill doesn't abort the
whole listing: guard the initial readdirSync(metaDir, ...) call with try/catch
and log/exit only on fatal errors, and inside the for (const entry of entries)
loop wrap the parseFrontmatter(skillFile) (and any read of SKILL.md) in a
try/catch; if parseFrontmatter throws or the file is unreadable/invalid,
console.warn a brief message including entry.name and the error, then continue
to the next entry and skip printing that skill. Ensure you still build shortDesc
from a safe description string and only call .replace/.slice on validated
strings returned from parseFrontmatter.
Comment on lines
+61
to
+62
| console.log('\nUsage: load the SKILL.md into your AI agent conversation.') | ||
| console.log('Path: node_modules/@tanstack/intent/meta/<name>/SKILL.md') |
There was a problem hiding this comment.
Use metaDir for the path hint instead of a hard-coded install path.
The current footer can print the wrong location when invoked from non-node_modules contexts. Build the hint from metaDir to keep it accurate.
Suggested adjustment
- console.log('Path: node_modules/@tanstack/intent/meta/<name>/SKILL.md')
+ console.log(`Path: ${join(metaDir, '<name>', 'SKILL.md')}`)📝 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
| console.log('\nUsage: load the SKILL.md into your AI agent conversation.') | |
| console.log('Path: node_modules/@tanstack/intent/meta/<name>/SKILL.md') | |
| console.log('\nUsage: load the SKILL.md into your AI agent conversation.') | |
| console.log(`Path: ${join(metaDir, '<name>', 'SKILL.md')}`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/meta.ts` around lines 61 - 62, The usage footer
prints a hard-coded path to SKILL.md which can be incorrect; change it to derive
the path from the existing metaDir variable used in this module. Replace the
second console.log that currently prints
'node_modules/@tanstack/intent/meta/<name>/SKILL.md' with one that constructs
the path using metaDir and the meta name (the same value used elsewhere in this
file), e.g. join metaDir with the meta name and 'SKILL.md' before logging so the
hint always reflects the real install location.
| ## After all skills are generated | ||
|
|
||
| 1. Run \`intent validate\` in each package directory | ||
| 2. Commit skills/ and artifacts |
There was a problem hiding this comment.
Keep artifact directory naming consistent in the prompt.
Line 63 says artifacts, while earlier guidance uses _artifacts/ (Line 22). This can cause incorrect operator action.
Suggested fix
-2. Commit skills/ and artifacts
+2. Commit skills/ and _artifacts/📝 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
| 2. Commit skills/ and artifacts | |
| 2. Commit skills/ and _artifacts/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/scaffold.ts` at line 63, Update the scaffold
prompt text to use the consistent artifact directory name `_artifacts/` instead
of `artifacts`; locate the prompt string in
packages/intent/src/commands/scaffold.ts (the scaffold command/prompt generation
code that emits the step "2. Commit skills/ and artifacts") and change that
occurrence to "2. Commit skills/ and _artifacts/" so it matches the earlier
guidance and avoids operator confusion.
Comment on lines
+12
to
+20
| if (reports.length === 0) { | ||
| console.log('No intent-enabled packages found.') | ||
| return | ||
| } | ||
|
|
||
| if (options.json) { | ||
| console.log(JSON.stringify(reports, null, 2)) | ||
| return | ||
| } |
There was a problem hiding this comment.
--json mode should return JSON even when no reports exist.
Line 12 currently short-circuits before JSON handling, so intent stale --json can emit plain text instead of [].
🔧 Proposed fix
- if (reports.length === 0) {
- console.log('No intent-enabled packages found.')
- return
- }
-
if (options.json) {
console.log(JSON.stringify(reports, null, 2))
return
}
+
+ if (reports.length === 0) {
+ console.log('No intent-enabled packages found.')
+ return
+ }📝 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
| if (reports.length === 0) { | |
| console.log('No intent-enabled packages found.') | |
| return | |
| } | |
| if (options.json) { | |
| console.log(JSON.stringify(reports, null, 2)) | |
| return | |
| } | |
| if (options.json) { | |
| console.log(JSON.stringify(reports, null, 2)) | |
| return | |
| } | |
| if (reports.length === 0) { | |
| console.log('No intent-enabled packages found.') | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/stale.ts` around lines 12 - 20, The early return
when reports.length === 0 bypasses JSON output; update the logic in the stale
command so that the options.json branch runs before the plain-text early exit
(or explicitly emit JSON empty array when options.json is true). Concretely,
adjust the block that checks reports.length === 0 and the options.json handling
so that when options.json is true you always call
console.log(JSON.stringify(reports, null, 2)) (yielding [] for an empty reports
array) and then return; keep the existing plain text "No intent-enabled packages
found." only for the non-JSON path.
Comment on lines
+66
to
+79
| const isMonorepoPkg = (() => { | ||
| let dir = join(root, '..') | ||
| for (let i = 0; i < 5; i++) { | ||
| const parentPkg = join(dir, 'package.json') | ||
| if (existsSync(parentPkg)) { | ||
| try { | ||
| const parent = JSON.parse(readFileSync(parentPkg, 'utf8')) | ||
| return ( | ||
| Array.isArray(parent.workspaces) || parent.workspaces?.packages | ||
| ) | ||
| } catch { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Monorepo detection returns too early on non-workspace parent package.json.
At Lines 70-78, if a parent package.json exists but has no workspaces, the function returns false immediately. That skips checking higher ancestors and can produce false packaging warnings in nested repos.
💡 Suggested fix
const isMonorepoPkg = (() => {
let dir = join(root, '..')
for (let i = 0; i < 5; i++) {
const parentPkg = join(dir, 'package.json')
if (existsSync(parentPkg)) {
try {
const parent = JSON.parse(readFileSync(parentPkg, 'utf8'))
- return (
- Array.isArray(parent.workspaces) || parent.workspaces?.packages
- )
+ if (
+ Array.isArray(parent.workspaces) ||
+ (parent.workspaces &&
+ typeof parent.workspaces === 'object' &&
+ 'packages' in parent.workspaces)
+ ) {
+ return true
+ }
} catch {
- return false
+ // ignore malformed intermediate package.json and continue walking up
}
}
const next = dirname(dir)
if (next === dir) break
dir = next
}
return false
})()🤖 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 66 - 79, The monorepo
detection in isMonorepoPkg prematurely returns false when encountering a parent
package.json that exists but lacks workspaces; change the logic so that after
reading/parsing parent (parentPkg -> parent) you only return true when
Array.isArray(parent.workspaces) || parent.workspaces?.packages is truthy,
otherwise continue the ancestor loop (do not return false), and likewise on JSON
parse errors (catch) continue to the next ancestor instead of returning false;
this ensures isMonorepoPkg checks higher-up package.json files before deciding
it is not a monorepo.
Comment on lines
+154
to
+167
| let fm: Record<string, unknown> | ||
| try { | ||
| fm = parseYaml(match[1]) as Record<string, unknown> | ||
| } catch { | ||
| errors.push({ file: rel, message: 'Invalid YAML frontmatter' }) | ||
| continue | ||
| } | ||
|
|
||
| if (!fm.name) { | ||
| errors.push({ file: rel, message: 'Missing required field: name' }) | ||
| } | ||
| if (!fm.description) { | ||
| errors.push({ file: rel, message: 'Missing required field: description' }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For npm package yamlversion 2.7.0, what canparse()return for YAML inputs likenull, a string scalar, and a sequence? Is a mapping/object guaranteed?
💡 Result:
In yaml@2.7.0, parse() returns the JavaScript value corresponding to the root node type of the YAML document (it’s not guaranteed to be a mapping/object) [1][2]:
- YAML
null(e.g.null,~, or an empty value) →null[1][2] - A string scalar (e.g.
hello) → a JS scalar (typically astring; other scalars may becomeboolean/numberdepending on schema) [1][2] - A sequence (e.g.
- a\n- b) → a JSArray[1][2] - A mapping (e.g.
a: 1) → a JS plainobject[1][2]
So: no, a mapping/object is not guaranteed—parse() can return null, a primitive, an array, or an object depending on the YAML input [1][2].
Sources: [1] eemeli.org/yaml docs for YAML.parse return behavior, [2] yaml@2.7.0 TypeScript declarations (dist/public-api.d.ts) on UNPKG.
Guard parsed YAML type before accessing frontmatter fields.
Line 156 casts parseYaml() result to Record<string, unknown>, but yaml@2.7.0 parse() returns the root YAML node type without guarantee of an object—it can return null, a scalar, or an array. Accessing fm.name or fm.description on a non-object will throw a TypeError at runtime.
Suggested fix
- let fm: Record<string, unknown>
+ let fm: Record<string, unknown>
try {
- fm = parseYaml(match[1]) as Record<string, unknown>
+ const parsed = parseYaml(match[1])
+ if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
+ errors.push({
+ file: rel,
+ message: 'Frontmatter must be a YAML mapping/object',
+ })
+ continue
+ }
+ fm = parsed as Record<string, unknown>
} catch {
errors.push({ file: rel, message: 'Invalid YAML frontmatter' })
continue
}📝 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
| let fm: Record<string, unknown> | |
| try { | |
| fm = parseYaml(match[1]) as Record<string, unknown> | |
| } catch { | |
| errors.push({ file: rel, message: 'Invalid YAML frontmatter' }) | |
| continue | |
| } | |
| if (!fm.name) { | |
| errors.push({ file: rel, message: 'Missing required field: name' }) | |
| } | |
| if (!fm.description) { | |
| errors.push({ file: rel, message: 'Missing required field: description' }) | |
| } | |
| let fm: Record<string, unknown> | |
| try { | |
| const parsed = parseYaml(match[1]) | |
| if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { | |
| errors.push({ | |
| file: rel, | |
| message: 'Frontmatter must be a YAML mapping/object', | |
| }) | |
| continue | |
| } | |
| fm = parsed as Record<string, unknown> | |
| } catch { | |
| errors.push({ file: rel, message: 'Invalid YAML frontmatter' }) | |
| continue | |
| } | |
| if (!fm.name) { | |
| errors.push({ file: rel, message: 'Missing required field: name' }) | |
| } | |
| if (!fm.description) { | |
| errors.push({ file: rel, message: 'Missing required field: description' }) | |
| } |
🤖 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 154 - 167, The code
currently assumes parseYaml(match[1]) returns an object; instead after parsing
(parseYaml) validate the type of fm by checking typeof fm === 'object' && fm !==
null && !Array.isArray(fm); if that check fails push errors.push({ file: rel,
message: 'Invalid YAML frontmatter' }) and continue, otherwise cast fm to
Record<string, unknown> and then perform the existing checks for fm.name and
fm.description (optionally tighten those checks to typeof fm.name !== 'string' /
typeof fm.description !== 'string' if you want to require strings). This uses
the existing symbols parseYaml, fm, errors, rel and the name/description checks
in validate.ts.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/intent/package.json (1)
41-41:⚠️ Potential issue | 🟡 MinorMake smoke test script shell-agnostic (still POSIX-specific at Line 41).
> /dev/nullis not portable to Windows shells. This was previously flagged and still appears unresolved.Suggested minimal fix
- "test:smoke": "pnpm run build && node dist/cli.mjs --help > /dev/null", + "test:smoke": "pnpm run build && node dist/cli.mjs --help",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/package.json` at line 41, The "test:smoke" script currently uses a POSIX-only redirection ("> /dev/null") which breaks on Windows; change the script to invoke Node directly and discard output without shell redirection — e.g. replace the current command for "test:smoke" in package.json with a Node one-liner that runs the CLI and ignores stdio (for example using child_process.spawnSync('node',['dist/cli.mjs','--help'],{stdio:'ignore'}) in a node -e invocation) so it works cross-platform and does not rely on /dev/null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/intent/package.json`:
- Line 41: The "test:smoke" script currently uses a POSIX-only redirection (">
/dev/null") which breaks on Windows; change the script to invoke Node directly
and discard output without shell redirection — e.g. replace the current command
for "test:smoke" in package.json with a Node one-liner that runs the CLI and
ignores stdio (for example using
child_process.spawnSync('node',['dist/cli.mjs','--help'],{stdio:'ignore'}) in a
node -e invocation) so it works cross-platform and does not rely on /dev/null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 380300a8-60bb-4b85-b44a-1aeded9d1653
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/intent/package.jsonpackages/intent/tests/cli.test.ts
- Brand CliFailure with symbol to prevent duck-typing false positives - Handle CACError in catch block for clean CLI error output - Fix unsafe (err as Error).message cast in scanIntentsOrFail - Include YAML parse error details in validation messages - Add monorepo artifact rationale comment in validate.ts - Fix misleading src path comment in setup.ts - Complete template variable list in docs - Add comment explaining cac double-argument quirk - Extract shared printWarnings to cli-support module - Move ValidationError interface to module scope - Extract isInsideMonorepo helper from IIFE Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/intent/src/commands/validate.ts (1)
148-161:⚠️ Potential issue | 🔴 CriticalValidate the parsed frontmatter shape before reading
nameordescription.
yaml.parse()is not guaranteed to return an object. If the frontmatter root isnull, this branch throws before you can report a validation error.Suggested fix
let fm: Record<string, unknown> try { - fm = parseYaml(match[1]) as Record<string, unknown> + const parsed = parseYaml(match[1]) + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + errors.push({ file: rel, message: 'Invalid YAML frontmatter' }) + continue + } + fm = parsed as Record<string, unknown> } catch (err) { const detail = err instanceof Error ? err.message : String(err) errors.push({ file: rel, message: `Invalid YAML frontmatter: ${detail}` }) continue }For the npm package `yaml`, does `parse()` guarantee an object result, or can it return `null`, scalars, or arrays depending on the document root? Please use the docs and type declarations for the version used in this repository.🤖 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 148 - 161, The parsed frontmatter result (fm from parseYaml(match[1])) can be null or a non-object (scalar/array), so before accessing fm.name or fm.description validate that fm is an object (e.g., typeof fm === 'object' && fm !== null && !Array.isArray(fm')) and if not, push an errors entry for rel like "Invalid frontmatter: expected mapping/object" and continue; then safely read fm.name and fm.description (with appropriate type guards or casting) and keep the existing missing-field error pushes unchanged.
🤖 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/list.ts`:
- Around line 93-99: Replace the hard-coded node_modules meta path printed by
the console.log call that outputs " Load:
node_modules/@tanstack/intent/meta/feedback-collection/SKILL.md" with an
install-location-agnostic hint; change the message emitted by the console.log
statement to point users to run the CLI command "intent meta
feedback-collection" (or programmatically resolve the meta path if you prefer)
so the output no longer assumes a node_modules layout. Locate the console.log
that prints the "Load: ..." string in list.ts and update its text accordingly or
replace it with a resolved path lookup before logging.
In `@packages/intent/src/commands/scaffold.ts`:
- Around line 23-25: The post-generation checklist is missing a repo-root
validation pass for monorepos: when the scaffold logic detects a workspace
(monorepo) it should also validate artifact files placed at the workspace root
(_artifacts/domain_map.yaml and _artifacts/skill_tree.yaml). Update the function
that builds the checklist (e.g., postGenerationChecklist or scaffoldCommand) to
add a step that either invokes the existing validation routine (the same code
used by package-level checks, e.g., intent validate) against the workspace root
or directly calls the domain/skill validation helpers to check
_artifacts/domain_map.yaml and _artifacts/skill_tree.yaml; ensure the new step
only runs when monorepo/workspace detection returns true.
---
Duplicate comments:
In `@packages/intent/src/commands/validate.ts`:
- Around line 148-161: The parsed frontmatter result (fm from
parseYaml(match[1])) can be null or a non-object (scalar/array), so before
accessing fm.name or fm.description validate that fm is an object (e.g., typeof
fm === 'object' && fm !== null && !Array.isArray(fm')) and if not, push an
errors entry for rel like "Invalid frontmatter: expected mapping/object" and
continue; then safely read fm.name and fm.description (with appropriate type
guards or casting) and keep the existing missing-field error pushes unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9a39637-0341-4e73-9c09-67f7b86b0783
📒 Files selected for processing (9)
docs/cli/intent-setup.mdpackages/intent/src/cli-error.tspackages/intent/src/cli-support.tspackages/intent/src/cli.tspackages/intent/src/commands/list.tspackages/intent/src/commands/meta.tspackages/intent/src/commands/scaffold.tspackages/intent/src/commands/validate.tspackages/intent/src/setup.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/intent/src/cli-support.ts
Comment on lines
+93
to
+99
| console.log('Feedback:') | ||
| console.log( | ||
| ' Submit feedback on skill usage to help maintainers improve the skills.', | ||
| ) | ||
| console.log( | ||
| ' Load: node_modules/@tanstack/intent/meta/feedback-collection/SKILL.md', | ||
| ) |
There was a problem hiding this comment.
Use an install-location-agnostic feedback hint.
This hard-coded node_modules/... path breaks for global, linked, and npx installs. Point users at intent meta feedback-collection or derive the real meta path instead.
Suggested adjustment
console.log(
- ' Load: node_modules/@tanstack/intent/meta/feedback-collection/SKILL.md',
+ ' Run: intent meta feedback-collection',
)📝 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
| console.log('Feedback:') | |
| console.log( | |
| ' Submit feedback on skill usage to help maintainers improve the skills.', | |
| ) | |
| console.log( | |
| ' Load: node_modules/@tanstack/intent/meta/feedback-collection/SKILL.md', | |
| ) | |
| console.log('Feedback:') | |
| console.log( | |
| ' Submit feedback on skill usage to help maintainers improve the skills.', | |
| ) | |
| console.log( | |
| ' Run: intent meta feedback-collection', | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/list.ts` around lines 93 - 99, Replace the
hard-coded node_modules meta path printed by the console.log call that outputs "
Load: node_modules/@tanstack/intent/meta/feedback-collection/SKILL.md" with an
install-location-agnostic hint; change the message emitted by the console.log
statement to point users to run the CLI command "intent meta
feedback-collection" (or programmatically resolve the meta path if you prefer)
so the output no longer assumes a node_modules layout. Locate the console.log
that prints the "Load: ..." string in list.ts and update its text accordingly or
replace it with a resolved path lookup before logging.
Comment on lines
+23
to
+25
| 4. For monorepos: | ||
| - Domain map artifacts go at the REPO ROOT: _artifacts/ | ||
| - Skills go INSIDE EACH PACKAGE: packages/<pkg>/skills/ |
There was a problem hiding this comment.
The monorepo checklist skips repo-root artifact validation.
Earlier steps place monorepo artifacts under the workspace-root _artifacts/, but the post-generation checklist only tells maintainers to run intent validate inside each package. That leaves domain_map.yaml and skill_tree.yaml unchecked unless they add a separate root-level validation pass.
Suggested adjustment
## After all skills are generated
-1. Run `intent validate` in each package directory
+1. For monorepos, run `intent validate .` once at the repo root to validate `_artifacts/`
+2. Run `intent validate` in each package directoryAlso applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/intent/src/commands/scaffold.ts` around lines 23 - 25, The
post-generation checklist is missing a repo-root validation pass for monorepos:
when the scaffold logic detects a workspace (monorepo) it should also validate
artifact files placed at the workspace root (_artifacts/domain_map.yaml and
_artifacts/skill_tree.yaml). Update the function that builds the checklist
(e.g., postGenerationChecklist or scaffoldCommand) to add a step that either
invokes the existing validation routine (the same code used by package-level
checks, e.g., intent validate) against the workspace root or directly calls the
domain/skill validation helpers to check _artifacts/domain_map.yaml and
_artifacts/skill_tree.yaml; ensure the new step only runs when
monorepo/workspace detection returns true.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/intent/tests/cli.test.ts (2)
48-52: ImprovegetHelpOutput()fidelity to avoid losing output textOn Line 50, only
call[0]is captured, so multi-arg console calls are partially dropped. Joining without delimiters on Line 51 can also blur boundaries between lines.Suggested diff
function getHelpOutput(): string { return [...infoSpy.mock.calls, ...logSpy.mock.calls] - .map((call) => String(call[0] ?? '')) - .join('') + .map((call) => call.map((arg) => String(arg)).join(' ')) + .join('\n') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/cli.test.ts` around lines 48 - 52, getHelpOutput() currently only records call[0] so any additional console args are lost and concatenating calls with '' removes boundaries; update getHelpOutput to serialize all arguments from each call (use infoSpy.mock.calls and logSpy.mock.calls), convert each arg to string, join args with a space for each call, and then join the calls with a newline (or other delimiter) to preserve argument order and line boundaries; reference the getHelpOutput function and the infoSpy/logSpy mock calls when implementing this change.
176-183: Make scaffold output assertion less brittleLine 178 assumes the prompt is always the first
console.logcall. If preface logs are added later, this test can fail even when the command is correct.Suggested diff
it('prints the scaffold prompt', async () => { const exitCode = await main(['scaffold']) - const output = String(logSpy.mock.calls[0]?.[0]) + const output = logSpy.mock.calls + .map((call) => call.map((arg) => String(arg)).join(' ')) + .join('\n') expect(exitCode).toBe(0) expect(output).toContain('## Step 1') expect(output).toContain('meta/domain-discovery/SKILL.md') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/intent/tests/cli.test.ts` around lines 176 - 183, The test currently assumes the scaffold prompt is the first console.log by using logSpy.mock.calls[0]?.[0]; update the assertion to search the recorded log calls for the scaffold output instead (e.g., use logSpy.mock.calls.find(...) or map/join over logSpy.mock.calls to locate an entry that contains '## Step 1' or 'meta/domain-discovery/SKILL.md') so the test no longer breaks if preface logs are added; adjust the test around main(['scaffold']) and logSpy to assert against the found log entry rather than index 0.
🤖 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/cli.test.ts`:
- Around line 48-52: getHelpOutput() currently only records call[0] so any
additional console args are lost and concatenating calls with '' removes
boundaries; update getHelpOutput to serialize all arguments from each call (use
infoSpy.mock.calls and logSpy.mock.calls), convert each arg to string, join args
with a space for each call, and then join the calls with a newline (or other
delimiter) to preserve argument order and line boundaries; reference the
getHelpOutput function and the infoSpy/logSpy mock calls when implementing this
change.
- Around line 176-183: The test currently assumes the scaffold prompt is the
first console.log by using logSpy.mock.calls[0]?.[0]; update the assertion to
search the recorded log calls for the scaffold output instead (e.g., use
logSpy.mock.calls.find(...) or map/join over logSpy.mock.calls to locate an
entry that contains '## Step 1' or 'meta/domain-discovery/SKILL.md') so the test
no longer breaks if preface logs are added; adjust the test around
main(['scaffold']) and logSpy to assert against the found log entry rather than
index 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e54853cd-4c3e-4a61-8698-eea2a906be85
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/intent/package.jsonpackages/intent/tests/cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/intent/package.json
WolfieLeader
added a commit
to WolfieLeader/tanstack-intent
that referenced
this pull request
Mar 17, 2026
Resolve conflicts with PR TanStack#85 (cac CLI migration): - Accept upstream's cac-based CLI structure - Keep our enhanced workspace detection (pnpm, deno, nested patterns) - Port pnpm-aware monorepo detection to commands/validate.ts - Integrate upstream's isGenericWorkspaceName/deriveWorkspacePackageName
This was referenced Mar 17, 2026
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
Refactors the
@tanstack/intentCLI from hand-rolled argument parsing tocac, decomposing a 745-line monolith into focused command modules. Also fixes monorepo workflow generation from #71. No user-facing API changes — all commands and flags remain the same.Approach
CLI restructuring: The previous
cli.tsmixed command routing, argument parsing, help text, validation, staleness orchestration, and error handling in one file. This PR splits it into:cli.ts— entrypoint: creates thecacinstance, registers commands, dispatchescli-error.ts—CliFailuretype with symbol brand,fail()helper,isCliFailure()guardcli-support.ts— shared utilities (scanIntentsOrFail,resolveStaleTargets,printWarnings,getMetaDir)commands/*.ts— one file per command (list,meta,validate,stale,install,scaffold,edit-package-json,setup-github-actions)Error handling:
CliFailureuses a symbol brand (not duck-typing) to prevent false matches. The catch block handlesCliFailure→ clean message,Error→ clean message, unknown → re-throw.fail()returnsneverfor TypeScript control flow.Monorepo setup:
setup-github-actionsnow detects workspace roots, writes workflows there, and generates monorepo-aware watch paths. Avoids bogus paths likepackages/root/src/**by detecting generic workspace names.Dependency injection:
runListCommandandrunStaleCommandaccept their async dependencies as parameters, improving testability. Commands use lazyimport()to keep CLI startup fast.Key Invariants
fail(), neverprocess.exit()CliFailureis branded with a unique symbol — structural duck-typing cannot produce false positivescacparse expectsprocess.argvformat (two ignored prefix entries)Non-goals
--verboseflag or stack trace output for debuggingmain()end-to-end, which is resilient to internal refactoring)Verification
pnpm --filter @tanstack/intent exec vitest run tests/cli.test.ts pnpm --filter @tanstack/intent test:lib pnpm --filter @tanstack/intent test:types pnpm --filter @tanstack/intent test:smokeFiles changed
src/cli.tssrc/cli-error.tsCliFailuretype with symbol brand,fail(),isCliFailure()src/cli-support.tsprintWarnings, stale target resolutionsrc/commands/*.tssrc/setup.tsderiveWorkspacePackageName, watch pathstests/cli.test.tstests/setup.test.tsdocs/cli/intent-setup.mdFrom #71
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation