Conversation
WalkthroughThis PR introduces optional import-graph skipping via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PlanAgent as Plan Agent
participant FileSystem as File System
participant PlanReviewer as Plan Reviewer<br/>(optional)
User->>PlanAgent: Provide task description
PlanAgent->>FileSystem: Create dev/active/{task-name}/
PlanAgent->>FileSystem: Load existing plan.md (if present)
PlanAgent->>FileSystem: Grep/Glob codebase for evidence
PlanAgent->>PlanAgent: Triage: blast radius, risk, complexity
PlanAgent->>User: Present triage table + scope options
User->>PlanAgent: Confirm scope or request changes
alt medium/large scope
PlanAgent->>PlanReviewer: Invoke reviewer for critical-issues-only feedback
PlanReviewer->>FileSystem: Review draft plan
PlanReviewer-->>PlanAgent: Return critical issues
PlanAgent->>PlanAgent: Update plan.md with feedback
end
PlanAgent->>FileSystem: Write/update plan.md (< 100 lines)<br/>with goals, approach, phases, decisions
PlanAgent->>User: Present final plan + execution prompt
sequenceDiagram
participant User
participant ExecuteAgent as Execute Agent
participant ExecutorSubagents as Executor<br/>Subagents<br/>(parallel)
participant FileSystem as File System
participant TestSuite as Test Suite
User->>ExecuteAgent: Execute plan (or load from dev/active/)
ExecuteAgent->>FileSystem: Load plan.md + read verdict scope
ExecuteAgent->>ExecuteAgent: Determine phase-by-phase execution
loop Per Phase
ExecuteAgent->>ExecutorSubagents: Spawn parallel executors per task<br/>(haiku or sonnet per tag)
ExecutorSubagents->>FileSystem: Execute task: read/modify files
ExecutorSubagents-->>ExecuteAgent: Return structured summary<br/>(task, files, tests, issues)
ExecuteAgent->>ExecuteAgent: Aggregate summaries
ExecuteAgent->>FileSystem: Mark completed tasks in plan.md
alt tests: fail (task's own files)
ExecuteAgent->>ExecutorSubagents: Single retry for fix-up
else tests: fail (other files)
ExecuteAgent->>User: Stop + report test failure
else issues present
ExecuteAgent->>ExecuteAgent: Handle design/blocker with potential re-spawn
end
alt large scope
ExecuteAgent->>User: Confirm before next phase
end
end
ExecuteAgent->>TestSuite: Run full repo test suite
ExecuteAgent->>FileSystem: Move dev/active/{task}/ to dev/inactive/
ExecuteAgent->>User: Report completion + file count + test status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/commands/add.js (1)
203-207: Narrowdev/gitignore injection to agent resources only.
name === 'plan' || name === 'execute'is name-based and can accidentally affect future non-agent resources with the same names.Proposed guard
- if (name === 'plan' || name === 'execute') { + if (resourceType.targetDir === '.claude/agents' && (name === 'plan' || name === 'execute')) { ensureDevGitignore(repoPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/add.js` around lines 203 - 207, The current check uses the resource name (name === 'plan' || name === 'execute') to decide to call ensureDevGitignore(repoPath), which can wrongly apply to non-agent resources; change the guard to detect agent resources explicitly (e.g., check a resource type/kind property or an isAgent flag) before calling ensureDevGitignore. Locate the block using name and ensureDevGitignore in src/commands/add.js and replace the name-based condition with an explicit agent check (for example: if (resource.type === 'agent' && (name === 'plan' || name === 'execute')) or if (isAgentResource(resource) && (name === 'plan' || name === 'execute')) ), or add a helper like isAgentResource(resource) and use that to ensure only agent resources trigger ensureDevGitignore(repoPath).src/templates/commands/dev-docs-update.md (1)
11-13: Clarify the exact per-task plan path format.Line 11–13 can be read as a single
plan.mdunder/dev/active/. Consider explicitly stating/dev/active/<task-name>/plan.mdto avoid mis-updates.As per coding guidelines: "
src/templates/**: User-facing skill/agent/hook templates. Review for clarity and correctness."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/templates/commands/dev-docs-update.md` around lines 11 - 13, Update the ambiguous instruction in src/templates/commands/dev-docs-update.md so it explicitly references the per-task plan path format; replace or clarify "For each task in `/dev/active/`" and the subsequent "Update `plan.md`" with the explicit path `/dev/active/<task-name>/plan.md` (or equivalent phrasing) so it is clear you're updating each task's individual plan file; ensure the example shows the placeholder `<task-name>` and retains the note about marking completed tasks as checked (`[x]`) to avoid accidental edits to a single shared file.src/templates/agents/plan-reviewer.md (1)
43-47: Consider making the 20-line cap a soft limit.For complex plans, a hard cap can drop critical findings. A soft cap (“keep concise, include all blockers”) preserves brevity without losing safety signals.
As per coding guidelines: "
src/templates/**: User-facing skill/agent/hook templates. Review for clarity and correctness."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/templates/agents/plan-reviewer.md` around lines 43 - 47, Update the user-facing template text in the plan-reviewer template to make the "20-line cap" a soft guideline: replace the hard-sounding "keep under 20 lines total" with wording that asks reviewers to "aim for under 20 lines; prioritize including all critical blockers and gaps" so important findings are not dropped. Edit the header line labeled "Output (keep under 20 lines total):" and adjust the numbered instruction 1 to remain concise but allow necessary detail (e.g., "Verdict — Ready / Needs revision / Major concerns (1 line; aim for brevity)"). Preserve the rest of the structure (the Issues block and "Skip sections with no findings") and ensure the template still instructs skipping empty sections.src/templates/agents/execute.md (1)
27-48: Add language specifier to code block.The executor spawning example lacks a language hint. Adding one would satisfy markdownlint MD040.
Optional fix
-``` +```text Use the Agent tool:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/templates/agents/execute.md` around lines 27 - 48, The fenced code block starting with "Use the Agent tool:" in src/templates/agents/execute.md is missing a language specifier (triggering markdownlint MD040); update the opening triple-backtick to include a language tag (for example "text") so the block becomes ```text, ensuring the rest of the block content stays unchanged and the linter no longer flags it.src/templates/agents/plan.md (1)
50-58: Consider adding language specifier to fenced block.The triage example table uses a bare code fence. Adding a language hint (e.g.,
markdownor leave as plain text outside fence) would satisfy markdownlint MD040.Optional fix
-``` +```text | Dimension | Rating | Evidence |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/templates/agents/plan.md` around lines 50 - 58, The fenced block containing the triage table in the Plan template should include a language specifier to satisfy markdownlint MD040; update the backticks around the table (the fenced block showing the | Dimension | Rating | Evidence | table) to use a language hint such as "text" or "markdown" (for example change ``` to ```text) or remove the fence and leave the table as plain markdown, ensuring the fenced block’s opening fence is updated where the table appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 215-234: Update the options table under the "aspens add <type>
[name]" section to include the missing --force option; add a new table row with
`--force` and the description "Overwrite existing skills" so the README matches
the CLI flag defined in bin/cli.js (flag currently described at line where the
CLI defines "Overwrite existing skills"). Locate the table containing `--list`
and `--from <file>` and insert the `--force` row between or alongside them
ensuring formatting matches the existing Markdown table.
In `@src/commands/add.js`:
- Around line 209-218: The ensureDevGitignore function performs file
reads/writes that can throw raw errors; wrap the filesystem operations
(existsSync/readFileSync/writeFileSync) in a try-catch inside ensureDevGitignore
and on failure throw a CliError (not process.exit) that includes a clear
message, remediation steps (e.g., "check file permissions or run with
appropriate user"), and attach the original error for diagnostics; reference the
ensureDevGitignore function and use the existing CliError class to construct the
error so callers get structured CLI errors while preserving the original error
details.
In `@src/commands/doc-init.js`:
- Around line 378-380: The preview icon logic can mislead: when isDomainsOnly is
true the existingDocsStrategy stays "fresh" and shouldForce may be false, so
files shown with '~' may still be skipped by writeSkillFiles(); change the icon
decision to reflect whether writeSkillFiles() will actually write the file by
computing a willBeWritten flag (e.g. if file exists then willBeWritten =
shouldForce || existingDocsStrategy !== 'fresh', else true) and use that to
choose pc.green('+') vs pc.yellow('~'); update the same logic for the duplicate
occurrence around lines 410-411 and reference variables/functions
existingDocsStrategy, shouldForce, isDomainsOnly, writeSkillFiles(), and the
file.path/existsSync check when making this change.
- Around line 939-942: The code reads a file using domain.name without
sanitization (existingDomainPath and domain.name), allowing path traversal; fix
by validating/sanitizing domain.name before joining: call the existing
sanitizePath() helper used for writes (or implement equivalent) to reject values
containing '..' or a leading '/', and only allow names that match the allowed
pattern (e.g., safe token chars) or follow the allowed skill-path rules (exact
"CLAUDE.md" or start with ".claude/"); after building the resolved path against
the skills root (join(repoPath, '.claude', 'skills')), assert that
pathIsInside(skillsRoot, resolvedPath) and skip/log and do not read the file if
validation fails.
- Around line 803-809: The code in the doc-init flow pushes raw triple-backtick
fenced content into parts (using claudeMdPath read into existing/truncated and
basePath read directly), which can be broken by embedded fences and can be
unbounded; implement a helper (e.g., sanitizeAndWrapDoc(content, maxLen)) that
truncates/summarizes to a safe max (e.g., 5k), escapes or wraps the text in a
wrapper that cannot be closed by Markdown content (such as an HTML <pre> or a
unique sentinel wrapper), and replace the direct interpolations where parts.push
is called for CLAUDE.md (claudeMdPath/existing/truncated), base skill
(basePath), and the domain-skill branch so all three use the same safe wrapper
and length cap before adding to parts.
In `@src/lib/runner.js`:
- Line 47: The current construction of maxTokensFlags uses a truthy check so 0
is treated as unset and invalid types slip through; update the logic around the
maxTokens variable (used to build maxTokensFlags) to explicitly validate that
maxTokens is a finite integer >= 0 (e.g., Number.isInteger(maxTokens) &&
maxTokens >= 0), only then create maxTokensFlags = ['--max-tokens',
String(maxTokens)]; otherwise handle invalid values explicitly (either omit the
flag or throw a clear TypeError) so non-numeric, NaN, negative or fractional
inputs are not accepted silently.
In `@src/prompts/doc-sync.md`:
- Line 7: The sentence that currently reads "Update CLAUDE.md only if repo-level
structure/commands changed" should be expanded to require CLAUDE.md updates for
convention-only changes as well; update the line in doc-sync.md that says
"Update only affected skills. Create new domain skills if the diff introduces a
new feature area. Update CLAUDE.md only if repo-level structure/commands
changed." to instead mention that CLAUDE.md must be updated for repo-level
structure/commands and for any convention-only changes that affect usage or
expectations, so maintainers know to update CLAUDE.md when conventions change.
In `@src/templates/agents/execute.md`:
- Around line 14-18: The numbered list in the template has a sequencing typo:
the third item starts with "3." but should be "2."; update the ordered list so
the item that begins "Check the plan's verdict line for scope
(trivial/small/medium/large)..." uses "2." instead of "3." to restore proper
numbering in src/templates/agents/execute.md and ensure the list reads 1, 2 in
the "Find the plan:"/verdict steps.
In `@src/templates/agents/ghost-writer.md`:
- Line 52: The sentence "Flag voice/audience questions only if you cannot
proceed without them" in the ghost-writer.md template is awkwardly worded with a
repeated "only"; replace it with a tighter phrasing such as "Only flag
voice/audience questions when required to proceed" or "Flag voice/audience
questions if you cannot proceed without them" by updating the line containing
that exact string to the chosen clearer variant so the output rule is more
concise and easier to parse.
---
Nitpick comments:
In `@src/commands/add.js`:
- Around line 203-207: The current check uses the resource name (name === 'plan'
|| name === 'execute') to decide to call ensureDevGitignore(repoPath), which can
wrongly apply to non-agent resources; change the guard to detect agent resources
explicitly (e.g., check a resource type/kind property or an isAgent flag) before
calling ensureDevGitignore. Locate the block using name and ensureDevGitignore
in src/commands/add.js and replace the name-based condition with an explicit
agent check (for example: if (resource.type === 'agent' && (name === 'plan' ||
name === 'execute')) or if (isAgentResource(resource) && (name === 'plan' ||
name === 'execute')) ), or add a helper like isAgentResource(resource) and use
that to ensure only agent resources trigger ensureDevGitignore(repoPath).
In `@src/templates/agents/execute.md`:
- Around line 27-48: The fenced code block starting with "Use the Agent tool:"
in src/templates/agents/execute.md is missing a language specifier (triggering
markdownlint MD040); update the opening triple-backtick to include a language
tag (for example "text") so the block becomes ```text, ensuring the rest of the
block content stays unchanged and the linter no longer flags it.
In `@src/templates/agents/plan-reviewer.md`:
- Around line 43-47: Update the user-facing template text in the plan-reviewer
template to make the "20-line cap" a soft guideline: replace the hard-sounding
"keep under 20 lines total" with wording that asks reviewers to "aim for under
20 lines; prioritize including all critical blockers and gaps" so important
findings are not dropped. Edit the header line labeled "Output (keep under 20
lines total):" and adjust the numbered instruction 1 to remain concise but allow
necessary detail (e.g., "Verdict — Ready / Needs revision / Major concerns (1
line; aim for brevity)"). Preserve the rest of the structure (the Issues block
and "Skip sections with no findings") and ensure the template still instructs
skipping empty sections.
In `@src/templates/agents/plan.md`:
- Around line 50-58: The fenced block containing the triage table in the Plan
template should include a language specifier to satisfy markdownlint MD040;
update the backticks around the table (the fenced block showing the | Dimension
| Rating | Evidence | table) to use a language hint such as "text" or "markdown"
(for example change ``` to ```text) or remove the fence and leave the table as
plain markdown, ensuring the fenced block’s opening fence is updated where the
table appears.
In `@src/templates/commands/dev-docs-update.md`:
- Around line 11-13: Update the ambiguous instruction in
src/templates/commands/dev-docs-update.md so it explicitly references the
per-task plan path format; replace or clarify "For each task in `/dev/active/`"
and the subsequent "Update `plan.md`" with the explicit path
`/dev/active/<task-name>/plan.md` (or equivalent phrasing) so it is clear you're
updating each task's individual plan file; ensure the example shows the
placeholder `<task-name>` and retains the note about marking completed tasks as
checked (`[x]`) to avoid accidental edits to a single shared file.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6abd805c-430f-4081-b4b3-6a12c3372f12
📒 Files selected for processing (33)
README.mdbin/cli.jssrc/commands/add.jssrc/commands/customize.jssrc/commands/doc-init.jssrc/commands/doc-sync.jssrc/commands/scan.jssrc/lib/runner.jssrc/prompts/add-skill.mdsrc/prompts/customize-agents.mdsrc/prompts/discover-architecture.mdsrc/prompts/discover-domains.mdsrc/prompts/doc-init-claudemd.mdsrc/prompts/doc-init-domain.mdsrc/prompts/doc-init.mdsrc/prompts/doc-sync-refresh.mdsrc/prompts/doc-sync.mdsrc/prompts/partials/examples.mdsrc/prompts/partials/skill-format.mdsrc/templates/agents/auto-error-resolver.mdsrc/templates/agents/code-architecture-reviewer.mdsrc/templates/agents/code-refactor-master.mdsrc/templates/agents/documentation-architect.mdsrc/templates/agents/execute.mdsrc/templates/agents/ghost-writer.mdsrc/templates/agents/plan-reviewer.mdsrc/templates/agents/plan.mdsrc/templates/agents/refactor-planner.mdsrc/templates/agents/ux-ui-designer.mdsrc/templates/agents/web-research-specialist.mdsrc/templates/commands/dev-docs-update.mdsrc/templates/commands/dev-docs.mdtests/prompt-loader.test.js
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/commands/doc-init.js (2)
803-809:⚠️ Potential issue | 🟠 MajorSafe-wrap and cap the embedded docs.
These branches still inline raw markdown inside triple-backtick fences, and the base/domain skill reads are still unbounded. Existing fences in
CLAUDE.mdorskill.mdcan break the wrapper early, and large hand-written docs can still blow the prompt budget this PR is trying to reduce.Also applies to: 870-873, 944-945, 1009-1011
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-init.js` around lines 803 - 809, The code in the branches that push existing docs into parts (references: claudeMdPath, basePath and the parts.push calls) embeds raw markdown inside triple-backtick fences without sanitization or size limits; update these places to (1) truncate the file contents to a safe maximum (e.g., 1000–2000 chars) before inlining, and (2) sanitize/escape any existing triple-backtick sequences (or replace them with a safe marker) so embedded fences cannot break the wrapper; apply the same truncation+sanitization logic to the other locations noted (lines around 870–873, 944–945, 1009–1011) to ensure all inlined docs are size-capped and safe-wrapped.
378-380:⚠️ Potential issue | 🟠 MajorPreview and write behavior are still out of sync.
Existing files are shown as
~, butwriteSkillFiles()still skips them unlessforceis true.--mode chunked --domains ...reruns can therefore preview an overwrite and then do nothing. Drive both the icon and the writer from the samewillWrite/shouldForcedecision.Also applies to: 410-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-init.js` around lines 378 - 380, Compute a single willWrite boolean (e.g., const willWrite = force || !willOverwrite || hasIssues) and use that value both for choosing the preview icon and for the actual write decision (i.e., pass or derive the same condition used by writeSkillFiles()/writer instead of recomputing with willOverwrite alone); update the icon assignment (replace the current ternary using willOverwrite/hasIssues with one based on willWrite) and ensure writeSkillFiles() or its caller uses the identical willWrite/shouldForce logic so preview and actual write behavior match.
🧹 Nitpick comments (1)
src/commands/doc-init.js (1)
722-726: Cap the hotspots list too.This helper limits hubs and rankings, but it still emits every hotspot. On churn-heavy repos that can dominate the architecture prompt and erase most of the token savings from splitting the contexts.
🛠️ Suggested fix
- for (const h of graph.hotspots) { + for (const h of graph.hotspots.slice(0, 10)) { sections.push(`- \`${h.path}\` — ${h.churn} changes, ${h.lines} lines`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-init.js` around lines 722 - 726, The hotspots list emitted from graph.hotspots is unbounded and should be capped like hubs/rankings to avoid huge outputs; change the loop that pushes hotspots (the for (const h of graph.hotspots) block that calls sections.push) to iterate only the first N entries (e.g., graph.hotspots.slice(0, MAX_HOTSPOTS) or use the existing max constant used for hubs/rankings) and, if graph.hotspots.length > MAX_HOTSPOTS, append a brief marker (e.g., "...and X more hotspots") so callers know the list was truncated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 199-201: Add documentation for the CLI option `--no-graph` to the
option tables for the `scan`, `doc init`, and `doc sync` commands in the README:
create a table row for `--no-graph` with a short description like "Skip import
graph analysis" (and mark it as applicable to those commands) so the README
matches the CLI behavior; ensure the flag name is spelled exactly `--no-graph`
and place the entry alongside the other command options in each command's
options table.
In `@src/commands/doc-init.js`:
- Line 822: Detect Claude timeouts from the runClaude call (the result of
runClaude(fullPrompt, makeClaudeOptions(timeoutMs, verbose, model,
claudeSpinner, 16000))) and, instead of calling p.confirm(), immediately switch
to the chunked/fallback path when a timeout or retryable error is seen; also add
and honor non-interactive flags --mode and --strategy so that when these flags
are set (bypassing interactive prompts) the code will automatically fall back to
chunked mode on timeout. Update the logic around runClaude/fullPrompt to catch
timeout errors, check CLI options mode/strategy, and invoke the existing
chunked-processing function (the chunked fallback code path) directly without
prompting via p.confirm().
In `@src/templates/agents/execute.md`:
- Around line 27-48: The markdown fenced code block in
src/templates/agents/execute.md starting at the "Use the Agent tool:" block is
missing a language and triggers MD040; update the opening triple-backtick to
include a language (e.g., ```yaml) so the block is fenced as YAML, leaving the
contents unchanged; locate the block by the exact header text "Use the Agent
tool:" and the fields "prompt:" and "model:" to ensure you modify the correct
code fence.
- Around line 81-85: Update the "Never read files" rule in the execute.md
template to allow targeted, auditable file reads for verification: change the
bullet "Never read files the executors are working on — trust their summaries."
to a constrained policy that permits limited, explicit file reads only for
verification and failure diagnosis (using safe methods such as Grep/Glob or
read-only, whitelisted paths), requires logging/annotating why the read was
necessary, and insists on respecting privacy/security boundaries; preserve the
other guidance about not holding executor output and preferring
code-map/import-graph checks. This relaxes the strict ban while keeping the
protections described in the surrounding bullets.
---
Duplicate comments:
In `@src/commands/doc-init.js`:
- Around line 803-809: The code in the branches that push existing docs into
parts (references: claudeMdPath, basePath and the parts.push calls) embeds raw
markdown inside triple-backtick fences without sanitization or size limits;
update these places to (1) truncate the file contents to a safe maximum (e.g.,
1000–2000 chars) before inlining, and (2) sanitize/escape any existing
triple-backtick sequences (or replace them with a safe marker) so embedded
fences cannot break the wrapper; apply the same truncation+sanitization logic to
the other locations noted (lines around 870–873, 944–945, 1009–1011) to ensure
all inlined docs are size-capped and safe-wrapped.
- Around line 378-380: Compute a single willWrite boolean (e.g., const willWrite
= force || !willOverwrite || hasIssues) and use that value both for choosing the
preview icon and for the actual write decision (i.e., pass or derive the same
condition used by writeSkillFiles()/writer instead of recomputing with
willOverwrite alone); update the icon assignment (replace the current ternary
using willOverwrite/hasIssues with one based on willWrite) and ensure
writeSkillFiles() or its caller uses the identical willWrite/shouldForce logic
so preview and actual write behavior match.
---
Nitpick comments:
In `@src/commands/doc-init.js`:
- Around line 722-726: The hotspots list emitted from graph.hotspots is
unbounded and should be capped like hubs/rankings to avoid huge outputs; change
the loop that pushes hotspots (the for (const h of graph.hotspots) block that
calls sections.push) to iterate only the first N entries (e.g.,
graph.hotspots.slice(0, MAX_HOTSPOTS) or use the existing max constant used for
hubs/rankings) and, if graph.hotspots.length > MAX_HOTSPOTS, append a brief
marker (e.g., "...and X more hotspots") so callers know the list was truncated.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f4e4cff-94c6-4186-b6d9-b8c2de31d729
📒 Files selected for processing (8)
README.mdsrc/commands/add.jssrc/commands/doc-init.jssrc/lib/runner.jssrc/prompts/doc-sync.mdsrc/templates/agents/execute.mdsrc/templates/agents/ghost-writer.mdsrc/templates/commands/dev-docs-update.md
✅ Files skipped from review due to trivial changes (2)
- src/prompts/doc-sync.md
- src/templates/commands/dev-docs-update.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/runner.js
- src/commands/add.js
- src/templates/agents/ghost-writer.md
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/doc-init.js (1)
71-74:⚠️ Potential issue | 🟡 MinorUse
CliErrorinstead ofprocess.exit(1).Per coding guidelines, command handlers should throw
CliErrorfor structured error handling, not callprocess.exit()directly.Proposed fix
if (!existsSync(skillsDir)) { - p.log.error('No skills found in .claude/skills/. Run `aspens doc init` first.'); - process.exit(1); + throw new CliError('No skills found in .claude/skills/. Run `aspens doc init` first.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-init.js` around lines 71 - 74, Replace the direct process.exit(1) with throwing a CliError: keep the existing p.log.error call (or replace it with throwing only), import or require CliError into this module, and throw new CliError('No skills found in .claude/skills/. Run `aspens doc init` first.') instead of calling process.exit; update the block that uses existsSync and p.log.error so it throws CliError to allow structured error handling.
🧹 Nitpick comments (2)
src/templates/agents/execute.md (1)
18-18: Match case with plan.md verdict values for consistency.The plan agent writes "Trivial", "Small", "Medium", "Large" (capitalized), but this line references them in lowercase. Use consistent casing for clarity.
Proposed fix
-2. Check the plan's verdict line for scope (trivial/small/medium/large). This determines execution behavior. +2. Check the plan's verdict line for scope (Trivial/Small/Medium/Large). This determines execution behavior.Based on learnings, context snippet 1 from
src/templates/agents/plan.md:37-44shows the exact verdict tokens are "Trivial", "Small", "Medium", "Large" (capitalized).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/templates/agents/execute.md` at line 18, Update the wording in the execute.md template so the scope tokens match the plan agent's actual verdict casing: replace the lowercase tokens "(trivial/small/medium/large)" with the capitalized tokens "(Trivial/Small/Medium/Large)"; reference the exact verdict tokens emitted by the plan agent ("Trivial", "Small", "Medium", "Large") to ensure consistency with plan.md and avoid mismatch.src/commands/doc-init.js (1)
956-966: Path traversal check added.The check at lines 959-961 addresses the prior concern about unsanitized
domain.namebeing used in file paths. Basic traversal patterns (.., leading/) are rejected.Consider using a centralized
sanitizePath()helper (per learnings) for consistency with other path validation in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-init.js` around lines 956 - 966, Replace the ad-hoc path traversal check around domain.name in the strategy === 'improve' block with a call to the project's centralized path sanitizer (e.g., sanitizePath) and use its sanitized output when building existingDomainPath; specifically, validate domain.name via sanitizePath(domain.name) (or bail/return the same failure object if sanitizePath flags it invalid), then use the sanitized value when computing existingDomainPath and when reading the file to populate existingDomainSection instead of directly using domain.name. Ensure you reference the same symbols: domain.name, existingDomainPath, existingDomainSection, and the sanitizePath helper so behavior matches other codepaths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/doc-sync/skill.md:
- Line 27: The doc lists `truncateDiff()` but the module actually exports
`truncate`, so update the reference in the doc to match the real helper name:
change `truncateDiff()` to `truncate` (or alternatively change the import in the
command to import `truncateDiff` from `src/lib/diff-helpers.js` if you prefer
that name); ensure the entry alongside `getSelectedFilesDiff()` and
`buildPrioritizedDiff()` uses the exact exported symbol `truncate` so the
documentation matches the code.
- Line 23: Update the doc summary line for src/commands/doc-sync.js to clarify
that the graph rebuild step is optional and is skipped when the --no-graph flag
is passed; explicitly mention the sequence as "git diff → (optional) graph
rebuild → skill mapping → Claude update → write" and note that doc-sync supports
a refresh mode and exports skillToDomain().
In `@src/commands/add.js`:
- Around line 203-206: The current check calls ensureDevGitignore(repoPath) for
any resource named 'plan' or 'execute'; restrict this so it only runs when those
names are being added as part of an agent install. Update the condition around
ensureDevGitignore in src/commands/add.js to require both (name === 'plan' ||
name === 'execute') AND the installation target/type is an agent (e.g., check
the variable that indicates install kind such as resourceType / type / target /
kind used in this file), e.g. change to if ((name === 'plan' || name ===
'execute') && target === 'agent') { ensureDevGitignore(repoPath); } so
.gitignore edits only occur for agent installs; use the actual identifier
present in this module for the install target.
---
Outside diff comments:
In `@src/commands/doc-init.js`:
- Around line 71-74: Replace the direct process.exit(1) with throwing a
CliError: keep the existing p.log.error call (or replace it with throwing only),
import or require CliError into this module, and throw new CliError('No skills
found in .claude/skills/. Run `aspens doc init` first.') instead of calling
process.exit; update the block that uses existsSync and p.log.error so it throws
CliError to allow structured error handling.
---
Nitpick comments:
In `@src/commands/doc-init.js`:
- Around line 956-966: Replace the ad-hoc path traversal check around
domain.name in the strategy === 'improve' block with a call to the project's
centralized path sanitizer (e.g., sanitizePath) and use its sanitized output
when building existingDomainPath; specifically, validate domain.name via
sanitizePath(domain.name) (or bail/return the same failure object if
sanitizePath flags it invalid), then use the sanitized value when computing
existingDomainPath and when reading the file to populate existingDomainSection
instead of directly using domain.name. Ensure you reference the same symbols:
domain.name, existingDomainPath, existingDomainSection, and the sanitizePath
helper so behavior matches other codepaths.
In `@src/templates/agents/execute.md`:
- Line 18: Update the wording in the execute.md template so the scope tokens
match the plan agent's actual verdict casing: replace the lowercase tokens
"(trivial/small/medium/large)" with the capitalized tokens
"(Trivial/Small/Medium/Large)"; reference the exact verdict tokens emitted by
the plan agent ("Trivial", "Small", "Medium", "Large") to ensure consistency
with plan.md and avoid mismatch.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd0f4311-58ce-48c3-b4d4-6dd0c331ddf7
📒 Files selected for processing (15)
.claude/skills/agent-customization/skill.md.claude/skills/base/skill.md.claude/skills/claude-runner/skill.md.claude/skills/doc-sync/skill.md.claude/skills/import-graph/skill.md.claude/skills/repo-scanning/skill.md.claude/skills/skill-generation/skill.md.claude/skills/skill-rules.json.claude/skills/template-library/skill.mdCLAUDE.mdREADME.mdsrc/commands/add.jssrc/commands/doc-init.jssrc/commands/doc-sync.jssrc/templates/agents/execute.md
✅ Files skipped from review due to trivial changes (4)
- .claude/skills/base/skill.md
- .claude/skills/repo-scanning/skill.md
- README.md
- .claude/skills/skill-rules.json
| ## Key Files | ||
| - `src/commands/doc-sync.js` — Main command: git diff → graph rebuild → skill mapping → Claude update → write | ||
| - `src/prompts/doc-sync.md` — System prompt sent to Claude (uses `{{skill-format}}` partial) | ||
| - `src/commands/doc-sync.js` — Main command: git diff → graph rebuild → skill mapping → Claude update → write. Also contains refresh mode and `skillToDomain()` export. |
There was a problem hiding this comment.
Clarify that graph rebuild is optional.
This line reads as unconditional, but doc-sync now skips graph work when --no-graph is set.
Proposed fix
-- `src/commands/doc-sync.js` — Main command: git diff → graph rebuild → skill mapping → Claude update → write. Also contains refresh mode and `skillToDomain()` export.
+- `src/commands/doc-sync.js` — Main command: git diff → (optional graph rebuild, unless `--no-graph`) → skill mapping → Claude update → write. Also contains refresh mode and `skillToDomain()` export.📝 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
| - `src/commands/doc-sync.js` — Main command: git diff → graph rebuild → skill mapping → Claude update → write. Also contains refresh mode and `skillToDomain()` export. | |
| - `src/commands/doc-sync.js` — Main command: git diff → (optional graph rebuild, unless `--no-graph`) → skill mapping → Claude update → write. Also contains refresh mode and `skillToDomain()` export. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/doc-sync/skill.md at line 23, Update the doc summary line for
src/commands/doc-sync.js to clarify that the graph rebuild step is optional and
is skipped when the --no-graph flag is passed; explicitly mention the sequence
as "git diff → (optional) graph rebuild → skill mapping → Claude update → write"
and note that doc-sync supports a refresh mode and exports skillToDomain().
| - `src/prompts/doc-sync.md` — System prompt for diff-based sync (uses `{{skill-format}}` partial) | ||
| - `src/prompts/doc-sync-refresh.md` — System prompt for `--refresh` mode (full skill review) | ||
| - `src/lib/git-helpers.js` — `isGitRepo()`, `getGitDiff()`, `getGitLog()`, `getChangedFiles()` — git primitives | ||
| - `src/lib/diff-helpers.js` — `getSelectedFilesDiff()`, `buildPrioritizedDiff()`, `truncateDiff()` — diff budgeting |
There was a problem hiding this comment.
Fix helper name mismatch in the doc.
The command imports truncate (not truncateDiff) from src/lib/diff-helpers.js, so this reference is currently inaccurate.
Proposed fix
-- `src/lib/diff-helpers.js` — `getSelectedFilesDiff()`, `buildPrioritizedDiff()`, `truncateDiff()` — diff budgeting
+- `src/lib/diff-helpers.js` — `getSelectedFilesDiff()`, `buildPrioritizedDiff()`, `truncate()` — diff budgeting📝 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
| - `src/lib/diff-helpers.js` — `getSelectedFilesDiff()`, `buildPrioritizedDiff()`, `truncateDiff()` — diff budgeting | |
| - `src/lib/diff-helpers.js` — `getSelectedFilesDiff()`, `buildPrioritizedDiff()`, `truncate()` — diff budgeting |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/doc-sync/skill.md at line 27, The doc lists `truncateDiff()`
but the module actually exports `truncate`, so update the reference in the doc
to match the real helper name: change `truncateDiff()` to `truncate` (or
alternatively change the import in the command to import `truncateDiff` from
`src/lib/diff-helpers.js` if you prefer that name); ensure the entry alongside
`getSelectedFilesDiff()` and `buildPrioritizedDiff()` uses the exact exported
symbol `truncate` so the documentation matches the code.
Comment on lines
+203
to
+206
| // Plan/execute agents need dev/ gitignored for plan storage | ||
| if (name === 'plan' || name === 'execute') { | ||
| ensureDevGitignore(repoPath); | ||
| } |
There was a problem hiding this comment.
Scope dev/ gitignore updates to agent installs only.
This condition currently runs for any resource named plan/execute, not just agents. That can cause unintended .gitignore edits for future command/hook templates with those names.
Proposed fix
- if (name === 'plan' || name === 'execute') {
+ if (resourceType.targetDir === '.claude/agents' && (name === 'plan' || name === 'execute')) {
ensureDevGitignore(repoPath);
}📝 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
| // Plan/execute agents need dev/ gitignored for plan storage | |
| if (name === 'plan' || name === 'execute') { | |
| ensureDevGitignore(repoPath); | |
| } | |
| // Plan/execute agents need dev/ gitignored for plan storage | |
| if (resourceType.targetDir === '.claude/agents' && (name === 'plan' || name === 'execute')) { | |
| ensureDevGitignore(repoPath); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/add.js` around lines 203 - 206, The current check calls
ensureDevGitignore(repoPath) for any resource named 'plan' or 'execute';
restrict this so it only runs when those names are being added as part of an
agent install. Update the condition around ensureDevGitignore in
src/commands/add.js to require both (name === 'plan' || name === 'execute') AND
the installation target/type is an agent (e.g., check the variable that
indicates install kind such as resourceType / type / target / kind used in this
file), e.g. change to if ((name === 'plan' || name === 'execute') && target ===
'agent') { ensureDevGitignore(repoPath); } so .gitignore edits only occur for
agent installs; use the actual identifier present in this module for the install
target.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Why
Closes #
How I tested
npm testpasses--dry-runoutput looks correct (if applicable)Checklist
Summary by CodeRabbit
New Features
--no-graphflag to skip import graph analysis inscan,doc init, anddoc synccommands.--refreshflag todoc syncto review all skills against current codebase.aspens doc graphcommand to rebuild import graph cache.--install-hookand--remove-hookoptions fordoc sync.aspens add skillwith file references..gitignoremanagement fordev/directory.Documentation