Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds skill scaffolding ( Changes
Files marked with Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as bin/cli.js
participant AddCmd as src/commands/add.js
participant Claude as Claude API
participant FS as File System
participant SkillLib as src/lib/skill-reader.js
participant Rules as src/lib/skill-writer.js
User->>CLI: aspens add skill --from <path>
CLI->>AddCmd: addCommand('skill', name, {from, timeout, model})
AddCmd->>FS: validate reference file exists
AddCmd->>AddCmd: loadPrompt('add-skill', ...)
AddCmd->>Claude: runClaude(prompt + repo context)
Claude-->>AddCmd: generated skill file(s)
AddCmd->>FS: parseFileOutput → write skill files
AddCmd->>SkillLib: extract activation/metadata
AddCmd->>Rules: extractRulesFromSkills() → write skill-rules.json
FS-->>User: skill created and rules updated
sequenceDiagram
participant User
participant CLI as bin/cli.js
participant DocSync as src/commands/doc-sync.js
participant Git as src/lib/git-helpers.js
participant Graph as src/lib/graph-builder.js
participant Persist as src/lib/graph-persistence.js
participant Claude as Claude API
participant FS as File System
User->>CLI: aspens doc sync --refresh
CLI->>DocSync: docSyncCommand(path, {refresh: true})
DocSync->>Git: scan repo / getChangedFiles()
Git-->>DocSync: changed file list
DocSync->>Graph: buildRepoGraph(repoPath)
Graph->>Persist: persistGraphArtifacts(rawGraph)
Persist-->>DocSync: graph artifacts (.claude/*)
loop batched domains
DocSync->>Claude: run doc-sync-refresh prompt for domain
Claude-->>DocSync: updated skill content
end
DocSync->>FS: write updated skill files
DocSync-->>User: report coverage and updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: 6
🧹 Nitpick comments (9)
tests/timeout.test.js (1)
43-73: Add edge-case tests for empty and partially numeric env values.Given Line 16/17 behavior in
src/lib/timeout.js, add coverage forASPENS_TIMEOUT=''andASPENS_TIMEOUT='30s'so malformed input handling is locked down.Proposed test additions
+ it('returns envWarning when env var is empty string', () => { + process.env.ASPENS_TIMEOUT = ''; + const { timeoutMs, envWarning } = resolveTimeout(undefined, 120); + expect(envWarning).toBe(true); + expect(timeoutMs).toBe(120000); + }); + + it('returns envWarning when env var is partially numeric', () => { + process.env.ASPENS_TIMEOUT = '30s'; + const { timeoutMs, envWarning } = resolveTimeout(undefined, 120); + expect(envWarning).toBe(true); + expect(timeoutMs).toBe(120000); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/timeout.test.js` around lines 43 - 73, Add two edge-case tests for resolveTimeout to assert malformed env inputs: set process.env.ASPENS_TIMEOUT = '' and process.env.ASPENS_TIMEOUT = '30s', call resolveTimeout(undefined, 120) and ensure envWarning is true and timeoutMs equals the default 120000; mirror the style of existing tests (clean/reset env between tests if needed) and place them alongside the other ASPENS_TIMEOUT cases in tests/timeout.test.js so resolveTimeout's handling of empty and partially numeric strings is covered.src/commands/customize.js (1)
14-14: Surface invalidASPENS_TIMEOUTwarnings in this command too.Line 14 ignores
envWarning, so bad env input silently falls back. Emitting a warning here would align behavior with other commands and reduce config confusion.Proposed tweak
- const { timeoutMs } = resolveTimeout(options.timeout, 300); + const { timeoutMs, envWarning } = resolveTimeout(options.timeout, 300); + if (envWarning) { + p.log.warn('Invalid ASPENS_TIMEOUT; using default timeout.'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/customize.js` at line 14, resolveTimeout currently returns an envWarning that is ignored in customize.js; update the destructuring to capture envWarning (e.g. const { timeoutMs, envWarning } = resolveTimeout(options.timeout, 300);) and, if envWarning is truthy, surface it (e.g. emit a warning via console.warn or the existing logger.warn) so invalid ASPENS_TIMEOUT inputs are reported; keep timeoutMs usage unchanged.src/lib/errors.js (1)
12-13: Use a nullish check when forwardingcause.Current truthy check drops falsy causes. Prefer
cause === undefinedso all explicit causes are preserved.Proposed fix
- constructor(message, { exitCode = 1, logged = false, cause } = {}) { - super(message, cause ? { cause } : undefined); + constructor(message, { exitCode = 1, logged = false, cause } = {}) { + super(message, cause === undefined ? undefined : { cause });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/errors.js` around lines 12 - 13, The constructor currently forwards the cause using a truthy check which drops falsy explicit causes; update the super call in the constructor (the line with super(message, ...)) to forward the cause when it is explicitly provided (use a check like cause !== undefined or cause === undefined ? undefined : { cause }) so that null/false/0/'' are preserved as explicit causes.src/commands/doc-init.js (1)
22-27: Consider handlingenvWarningfor consistency.
resolveTimeoutreturns{ timeoutMs, envWarning }, butenvWarningis ignored here. Indoc-sync.js, this warning is used to notify users of invalidASPENS_TIMEOUTvalues. Consider logging a warning ifenvWarningis true.♻️ Optional: add envWarning handling
function autoTimeout(scan, userTimeout) { const sizeDefaults = { 'small': 120, 'medium': 300, 'large': 600, 'very-large': 900 }; const fallback = sizeDefaults[scan.size?.category] || 300; - const { timeoutMs } = resolveTimeout(userTimeout, fallback); - return timeoutMs; + const { timeoutMs, envWarning } = resolveTimeout(userTimeout, fallback); + return { timeoutMs, envWarning }; }Then destructure and optionally warn in the caller.
🤖 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 22 - 27, autoTimeout currently ignores the envWarning returned by resolveTimeout; update autoTimeout to destructure both { timeoutMs, envWarning } from resolveTimeout(scan, fallback) and, when envWarning is truthy, emit the same warning used in doc-sync.js (e.g., call the project's warning logger or console.warn with a clear message) so users are notified of invalid ASPENS_TIMEOUT values; reference the autoTimeout function and resolveTimeout call when making the change.CLAUDE.md (1)
20-38: Add language specifier to fenced code block.The architecture diagram code block on line 20 is missing a language specifier. Consider adding
textorplaintextfor lint compliance.♻️ Add language specifier
-``` +```text bin/cli.js # entry point — Commander program, CliError handler🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 20 - 38, The fenced code block that shows the repository file tree (the block containing "bin/cli.js ... tests/") is missing a language specifier; update that opening fence to include a plain text language (for example use "text" or "plaintext") so the block becomes ```text to satisfy the linter and ensure consistent rendering.src/lib/git-helpers.js (1)
1-10: Minor:isGitRepousesexecSyncwhile others useexecFileSync.
execFileSyncis generally preferred (no shell interpolation risks). Consider consistency:Optional: use execFileSync
export function isGitRepo(repoPath) { try { - execSync('git rev-parse --git-dir', { cwd: repoPath, stdio: 'pipe', timeout: 5000 }); + execFileSync('git', ['rev-parse', '--git-dir'], { cwd: repoPath, stdio: 'pipe', timeout: 5000 }); return true; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/git-helpers.js` around lines 1 - 10, The isGitRepo function uses execSync which invokes a shell while other helpers use execFileSync; change isGitRepo to call execFileSync('git', ['rev-parse','--git-dir'], { cwd: repoPath, stdio: 'pipe', timeout: 5000 }) so it avoids shell interpolation and matches the existing pattern, preserving the same try/catch return-true/false behavior in the isGitRepo export.src/lib/diff-helpers.js (1)
45-52: Edge case: truncation falls back tomaxCharsif nodiff --gitboundary found.When the diff contains no
diff --gitmarker after position 0,lastHunkBoundaryis-1, socutPointbecomesmaxChars—which could still cut mid-line. This is acceptable for rare edge cases, but worth noting.Optional: cut at last newline instead
const truncated = diff.slice(0, maxChars); const lastHunkBoundary = truncated.lastIndexOf('\ndiff --git'); - const cutPoint = lastHunkBoundary > 0 ? lastHunkBoundary : maxChars; + const cutPoint = lastHunkBoundary > 0 ? lastHunkBoundary : truncated.lastIndexOf('\n'); + const finalCut = cutPoint > 0 ? cutPoint : maxChars; - return diff.slice(0, cutPoint) + `\n\n... (diff truncated — use Read tool to see full files)`; + return diff.slice(0, finalCut) + `\n\n... (diff truncated — use Read tool to see full files)`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/diff-helpers.js` around lines 45 - 52, The truncateDiff function currently falls back to cutPoint === maxChars which can split a line mid-text; update truncateDiff to, when lastHunkBoundary <= 0, search truncated (diff.slice(0, maxChars)) for the last newline character ('\n') and use that index (or maxChars if none found) as the cutPoint so truncation prefers whole-line boundaries; refer to the truncateDiff function and variables truncated, lastHunkBoundary, and cutPoint when making the change.tests/add-skill.test.js (1)
72-80: Date comparison has a rare edge case.If the test runs exactly at midnight UTC while
addCommandexecutes on the previous day (or vice versa), the assertion could fail. Unlikely in practice, but worth knowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/add-skill.test.js` around lines 72 - 80, The test's strict equality on a single ISO date can fail around UTC midnight; update the assertion in the test that calls addCommand and reads the generated skill.md to accept a small UTC-boundary variance: compute today = new Date().toISOString().split('T')[0] and also yesterday = new Date(Date.now() - 86400000).toISOString().split('T')[0], then assert that content contains either `**Last Updated:** ${today}` or `**Last Updated:** ${yesterday}` (or alternatively parse the date from content and assert it's within one day); modify the test surrounding addCommand, readFileSync, TEST_DIR and the skill.md check accordingly.src/commands/add.js (1)
302-309: Use the shared writer for scaffolded skills.This path still hand-writes
.claude/skills/<name>/skill.md. ReusingwriteSkillFiles()keepsadd skillaligned with the rest of the skill pipeline's create/overwrite behavior instead of growing a second write path here.Based on learnings, write skills to
.claude/skills/<name>/skill.mdin the target repository using the skill-writer module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/add.js` around lines 302 - 309, Replace the direct fs writes to skillDir/skillPath with the shared skill-writer: stop using mkdirSync()/writeFileSync() in add.js and call writeSkillFiles(...) to create the .claude/skills/<name>/skill.md in the target repo so the add flow reuses the same create/overwrite semantics; reference the existing scaffold string and pass it (and the target repo/skillsDir context) into writeSkillFiles, then keep the existing console logs and updateSkillRules(skillsDir) call after the writer completes.
🤖 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/skill-generation/skill.md:
- Around line 22-32: The pipeline definition is inconsistent between the file
list and Key Concepts: update the sequence so both places use the same 9-step
flow; choose one canonical sequence (e.g., either "scan → graph → discovery →
strategy → mode → generate → validate → preview → write → hooks" or "scan +
graph → discovery → strategy → mode → generate → validate → write → hooks") and
replace the other occurrence accordingly in this document (update the bullet
under `src/commands/doc-init.js` and the "9-step pipeline" line in Key Concepts)
so both `src/commands/doc-init.js` description and the Key Concepts paragraph
match exactly.
In `@src/commands/add.js`:
- Around line 364-377: The current code writes every file returned by
parseFileOutput() directly (iterating over files and calling writeFileSync on
fullPath), which allows a model to overwrite unrelated repo files; instead,
ensure you reject or ignore any file whose path is not exactly
.claude/skills/<skillName>/skill.md (or otherwise constrained into the
.claude/skills/<skillName>/ directory), and route the approved content through
the existing writeSkillFiles(...) function from the skill-writer module so the
command uses the same safe write path as the rest of the pipeline; update the
code around parseFileOutput, the files loop, and use writeSkillFiles(skillName,
content, repoPath) (or the actual signature) rather than writeFileSync to
perform the final write.
In `@src/commands/doc-sync.js`:
- Around line 127-146: The picker allows an empty selection which then calls
getSelectedFilesDiff(repoPath, selectedFiles, actualCommits) with selectedFiles
=== [], removing path filtering; guard against this by checking selectedFiles
after the p.multiselect return: if selectedFiles.length === 0, call p.cancel('No
files selected') and return (or set activeDiff to an empty/zero diff and skip
sending), otherwise proceed; update the logic around selectedFiles,
changedFiles, and the activeDiff assignment that currently calls
getSelectedFilesDiff to ensure an empty selection never falls back to the full
diff.
- Around line 247-256: After writeSkillFiles(...) completes, regenerate the
skill rules artifact so hooks see updated Activation/Keywords; call the same
rule-build logic used elsewhere (e.g., invoke updateSkillRules or the function
used by refreshAllSkills()) immediately after writeSkillFiles returns, import
the helper if necessary, run it synchronously and surface any errors before
exiting, and ensure the skill-rules.json file is written/overwritten so the
runtime uses the new patterns.
- Around line 373-454: The parsed Claude outputs (from parseFileOutput) are
being pushed wholesale into allUpdatedFiles, allowing unrelated files to
overwrite other skills or CLAUDE.md; instead, after calling
parseFileOutput(result.text) filter the returned files to an allowlist for the
current refresh: in the base refresh block (where baseSpinner is used) accept
only files whose path matches the expected base file(s) for that pass; in the
domain batch block (inside the batch.map async where skillSpinner is used)
filter files to only include those with file.path === skill.path (or other
allowed paths for that skill) before pushing to allUpdatedFiles; and in the
CLAUDE.md branch (using claudeMdPath / 'CLAUDE.md') accept only files that
target CLAUDE.md. Only push the filtered results to allUpdatedFiles and preserve
the existing handling of empty results and spinner messages.
In `@src/lib/timeout.js`:
- Around line 16-19: The current ASPENS_TIMEOUT handling in src/lib/timeout.js
uses parseInt and accepts strings like "30s"; change the logic that reads
process.env.ASPENS_TIMEOUT so you only accept a pure numeric string (e.g., match
/^\d+$/) before converting to a number, and if it fails validation do not use
the partially parsed value but instead return the default/undefined timeout with
envWarning: true; update the branch that currently returns { timeoutMs: parsed *
1000, envWarning: false } to only run when the raw env value is a pure integer
string and otherwise set envWarning true and avoid using parseInt results.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 20-38: The fenced code block that shows the repository file tree
(the block containing "bin/cli.js ... tests/") is missing a language specifier;
update that opening fence to include a plain text language (for example use
"text" or "plaintext") so the block becomes ```text to satisfy the linter and
ensure consistent rendering.
In `@src/commands/add.js`:
- Around line 302-309: Replace the direct fs writes to skillDir/skillPath with
the shared skill-writer: stop using mkdirSync()/writeFileSync() in add.js and
call writeSkillFiles(...) to create the .claude/skills/<name>/skill.md in the
target repo so the add flow reuses the same create/overwrite semantics;
reference the existing scaffold string and pass it (and the target
repo/skillsDir context) into writeSkillFiles, then keep the existing console
logs and updateSkillRules(skillsDir) call after the writer completes.
In `@src/commands/customize.js`:
- Line 14: resolveTimeout currently returns an envWarning that is ignored in
customize.js; update the destructuring to capture envWarning (e.g. const {
timeoutMs, envWarning } = resolveTimeout(options.timeout, 300);) and, if
envWarning is truthy, surface it (e.g. emit a warning via console.warn or the
existing logger.warn) so invalid ASPENS_TIMEOUT inputs are reported; keep
timeoutMs usage unchanged.
In `@src/commands/doc-init.js`:
- Around line 22-27: autoTimeout currently ignores the envWarning returned by
resolveTimeout; update autoTimeout to destructure both { timeoutMs, envWarning }
from resolveTimeout(scan, fallback) and, when envWarning is truthy, emit the
same warning used in doc-sync.js (e.g., call the project's warning logger or
console.warn with a clear message) so users are notified of invalid
ASPENS_TIMEOUT values; reference the autoTimeout function and resolveTimeout
call when making the change.
In `@src/lib/diff-helpers.js`:
- Around line 45-52: The truncateDiff function currently falls back to cutPoint
=== maxChars which can split a line mid-text; update truncateDiff to, when
lastHunkBoundary <= 0, search truncated (diff.slice(0, maxChars)) for the last
newline character ('\n') and use that index (or maxChars if none found) as the
cutPoint so truncation prefers whole-line boundaries; refer to the truncateDiff
function and variables truncated, lastHunkBoundary, and cutPoint when making the
change.
In `@src/lib/errors.js`:
- Around line 12-13: The constructor currently forwards the cause using a truthy
check which drops falsy explicit causes; update the super call in the
constructor (the line with super(message, ...)) to forward the cause when it is
explicitly provided (use a check like cause !== undefined or cause === undefined
? undefined : { cause }) so that null/false/0/'' are preserved as explicit
causes.
In `@src/lib/git-helpers.js`:
- Around line 1-10: The isGitRepo function uses execSync which invokes a shell
while other helpers use execFileSync; change isGitRepo to call
execFileSync('git', ['rev-parse','--git-dir'], { cwd: repoPath, stdio: 'pipe',
timeout: 5000 }) so it avoids shell interpolation and matches the existing
pattern, preserving the same try/catch return-true/false behavior in the
isGitRepo export.
In `@tests/add-skill.test.js`:
- Around line 72-80: The test's strict equality on a single ISO date can fail
around UTC midnight; update the assertion in the test that calls addCommand and
reads the generated skill.md to accept a small UTC-boundary variance: compute
today = new Date().toISOString().split('T')[0] and also yesterday = new
Date(Date.now() - 86400000).toISOString().split('T')[0], then assert that
content contains either `**Last Updated:** ${today}` or `**Last Updated:**
${yesterday}` (or alternatively parse the date from content and assert it's
within one day); modify the test surrounding addCommand, readFileSync, TEST_DIR
and the skill.md check accordingly.
In `@tests/timeout.test.js`:
- Around line 43-73: Add two edge-case tests for resolveTimeout to assert
malformed env inputs: set process.env.ASPENS_TIMEOUT = '' and
process.env.ASPENS_TIMEOUT = '30s', call resolveTimeout(undefined, 120) and
ensure envWarning is true and timeoutMs equals the default 120000; mirror the
style of existing tests (clean/reset env between tests if needed) and place them
alongside the other ASPENS_TIMEOUT cases in tests/timeout.test.js so
resolveTimeout's handling of empty and partially numeric strings is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0284929a-77f6-4a9b-b6a6-0b9b0d3b087a
📒 Files selected for processing (28)
.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/skill-generation/skill.md.claude/skills/template-library/skill.md.gitignoreCLAUDE.mdbin/cli.jssrc/commands/add.jssrc/commands/customize.jssrc/commands/doc-init.jssrc/commands/doc-sync.jssrc/lib/diff-helpers.jssrc/lib/errors.jssrc/lib/git-helpers.jssrc/lib/git-hook.jssrc/lib/graph-persistence.jssrc/lib/skill-reader.jssrc/lib/timeout.jssrc/prompts/add-skill.mdsrc/prompts/doc-init-claudemd.mdsrc/prompts/doc-sync-refresh.mdsrc/prompts/doc-sync.mdtests/add-skill.test.jstests/git-hook.test.jstests/skill-mapper.test.jstests/timeout.test.js
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
aspens doc graph,aspens add skill(including--from) anddoc sync --refresh;--hooks-onlyflag.Improvements
Tests