Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a Claude hooks system for session-sticky skill activation and file edit tracking. It adds hook scripts (shell and Node.js), CLI controls for hook installation/removal, skill rule validation, git hook management for background doc sync, and utilities for skill discovery and settings merging. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Claude
participant SkillActivationHook as skill-activation<br/>prompt.sh
participant SkillMatcher as skill-activation<br/>prompt.mjs
participant SkillRules as .claude/skills/
participant SessionCache as /tmp/session<br/>cache
User->>Claude: Submit user prompt
Claude->>SkillActivationHook: Invoke UserPromptSubmit hook
SkillActivationHook->>SkillActivationHook: Capture stdin<br/>(prompt JSON)
SkillActivationHook->>SkillMatcher: Pipe stdin + run Node.js
SkillMatcher->>SkillRules: Load skill-rules.json
SkillMatcher->>SessionCache: Load session<br/>active skills
SkillMatcher->>SkillMatcher: Match skills:<br/>keywords, intents,<br/>alwaysActivate
SkillMatcher->>SkillRules: Load matched skill.md<br/>content
SkillMatcher->>SkillActivationHook: Write stdout<br/>(injected markdown)
SkillActivationHook->>Claude: Output formatted<br/>skill blocks
Claude->>User: Include activated<br/>skills in context
sequenceDiagram
participant Claude
participant Tool as Edit/MultiEdit/<br/>Write tool
participant PostToolHook as post-tool-use<br/>tracker.sh
participant RepoDetect as detect_repo
participant CacheDir as .claude/tsc-cache/
participant CommandGen as get_build_command<br/>get_tsc_command
participant SessionCache as /tmp/session<br/>cache
Claude->>Tool: Execute file edit
Tool->>PostToolHook: Invoke PostToolUse hook<br/>(JSON on stdin)
PostToolHook->>PostToolHook: Extract file_path<br/>tool_name, session_id
PostToolHook->>RepoDetect: Detect repo identifier
PostToolHook->>CacheDir: Create per-session<br/>cache dir
PostToolHook->>CacheDir: Append to<br/>edited-files.log
PostToolHook->>CacheDir: Dedupe + write<br/>affected-repos.txt
PostToolHook->>CommandGen: Generate build/tsc<br/>commands
PostToolHook->>CacheDir: Dedupe + write<br/>commands.txt
PostToolHook->>SessionCache: Persist skill domain<br/>(session-sticky)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans heterogeneous concerns: bash/Node.js hook logic, CLI option integration, file parsing rework, git hook marker management, skill rule extraction with pattern generation, and test coverage. Each area requires distinct reasoning despite consistent patterns (deduplication, template injection, marker-based installation). Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
Fix/doc sync hook automation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
.claude/hooks/skill-activation-prompt.mjs (1)
219-227: Skill content loaded twice.
formatOutputloads content at lines 220-227, butmain()already loads it at lines 329-334 before callingformatOutput. The duplicate loading is wasteful.Since
main()setsskill.contentbefore callingformatOutput, the inner loading informatOutputis redundant (guarded byif (!skill.content)but still checked every time).Also applies to: 329-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/skill-activation-prompt.mjs around lines 219 - 227, formatOutput currently re-checks/loading skill content with the loop that calls readSkillContent for each item in matched, but main() already populates skill.content before calling formatOutput, making that work redundant; remove the content-loading block from formatOutput (the for-loop that checks if (!skill.content) and calls readSkillContent) and rely on main() to set skill.content (or, if defensive behavior is desired, change to a no-op guard comment), leaving readSkillContent usage only in main() where skill.content is initially set.src/commands/doc-init.js (2)
472-474: dryRun skips rule file write but doesn't indicate it.When
dryRunis true,skill-rules.jsonisn't written but there's no log output indicating this. Consider adding a dry-run indicator for completeness.🤖 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 472 - 474, The current block that writes skill rules uses writeFileSync(rulesPath, JSON.stringify(rules, null, 2) + '\n') only when !options.dryRun, but provides no feedback when options.dryRun is true; update the conditional around writeFileSync so that when options.dryRun is true you log a clear dry-run message (e.g., using console.log or the existing logger) mentioning rulesPath and that the rules JSON was not written, while preserving the existing write behavior when options.dryRun is false and still using rules and rulesPath.
508-511: Stub regex replacement is fragile.The pattern
detect_skill_domain\(\)\s*\{[\s\S]*?\n\}assumes the function body ends at the first\n}. If the template function contains nested braces with newlines, this could truncate content.Consider using a more robust marker-based replacement (e.g.,
# BEGIN detect_skill_domain/# END detect_skill_domain).🤖 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 508 - 511, The current replacement using stubRegex (detect_skill_domain\(\)\s*\{[\s\S]*?\n\}) is fragile and can truncate detect_skill_domain() when it contains nested braces or multiple newlines; update the logic that modifies trackerContent to look for explicit markers instead—e.g., search for a block delimited by unique tokens like "# BEGIN detect_skill_domain" and "# END detect_skill_domain" (or add those markers to the template if missing), then replace the entire marked block with domainPatterns.trim(); ensure the code references the same markers when detecting and replacing, and keep a safe fallback if markers aren't present to avoid accidental truncation..claude/hooks/post-tool-use-tracker.sh (2)
53-53: ShellCheck SC2155: Declare and assign separately.
local repo=$(echo ...)masks the return value of the command. While functional here, it's a minor code quality issue.♻️ Fix pattern
- local repo=$(echo "$relative_path" | cut -d'/' -f1) + local repo + repo=$(echo "$relative_path" | cut -d'/' -f1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/post-tool-use-tracker.sh at line 53, The declaration/assignment of repo should be split to satisfy ShellCheck SC2155: first declare the local variable, then assign it; i.e., replace the single-line "local repo=$(echo "$relative_path" | cut -d'/' -f1)" with a separate "local repo" declaration followed by "repo=$(echo "$relative_path" | cut -d'/' -f1)"; use the existing variable names repo and relative_path so the change is minimal and keeps the same behavior.
209-231: Hardcodeddetect_skill_domainpatterns will be overwritten.The patterns here (agent-customization, claude-runner, etc.) appear to be aspens-specific defaults. Per the AI summary and
src/commands/doc-init.js, this function is replaced bygenerateDomainPatterns()during hook installation. This is fine for the template but may confuse readers.Consider adding a comment like
# STUB: replaced during installationat the function declaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/post-tool-use-tracker.sh around lines 209 - 231, The detect_skill_domain function currently contains hardcoded aspens-specific patterns that are overwritten during hook installation by generateDomainPatterns(); update the function declaration to include a clear single-line stub comment (e.g., "# STUB: replaced during installation by generateDomainPatterns()") so readers know this is a template placeholder, and place that comment immediately above or on the same line as the detect_skill_domain() declaration to prevent confusion.src/lib/skill-writer.js (3)
420-422:dedupeStringslowercases all strings.This loses casing for display purposes. If skill names or keywords should preserve casing (e.g., "GraphQL", "TypeScript"), this normalization will flatten them.
function dedupeStrings(arr) { return [...new Set(arr.map(s => s.toLowerCase()))]; }If case preservation matters, consider a case-insensitive Set check that preserves the first occurrence's casing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skill-writer.js` around lines 420 - 422, The dedupeStrings function currently lowercases every string which destroys original casing; change it to perform case-insensitive deduplication while preserving the first occurrence's original casing by iterating over arr, tracking seen lowercased values in a Set, and pushing the original string only when its lowercased form hasn't been seen yet (update function dedupeStrings accordingly so it returns the deduped list preserving original casing of first occurrences).
199-204: Duplicate detection via JSON.stringify is order-dependent.
JSON.stringify(e) === JSON.stringify(templateEntry)will fail to detect duplicates if object keys are in different orders. This could lead to duplicate hooks being appended.♻️ Consider a deep equality helper
// Simple deep equality for hook entries function hookEntriesEqual(a, b) { return JSON.stringify(sortKeys(a)) === JSON.stringify(sortKeys(b)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skill-writer.js` around lines 199 - 204, The duplicate-check using JSON.stringify in merged.hooks[eventType].some(e => JSON.stringify(e) === JSON.stringify(templateEntry)) is order-dependent; replace it with a deterministic deep-equality check (e.g., implement a helper like hookEntriesEqual(a,b) that compares objects after sorting keys or performs a recursive deepEqual) and use merged.hooks[eventType].some(e => hookEntriesEqual(e, templateEntry)); add the helper (e.g., hookEntriesEqual or stableStringify) adjacent to the current logic so duplicate detection works regardless of property order.
123-127: Bash regex patterns are not escaped for special characters.If
bashPatternscontain regex metacharacters (from glob conversion), they're inserted directly into[[ "$file" =~ ${p} ]]. WhileglobToBashPatternproduces relatively safe output, a malformed skill name or pattern could inject unintended regex behavior.Consider validating or escaping patterns before insertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/skill-writer.js` around lines 123 - 127, The code injects raw regex patterns into the bash test ([[ "$file" =~ ${p} ]]) which can allow regex metacharacter injection; fix by validating or escaping patterns before building conditions: add an escape function (e.g. escapeBashRegex) that backslashes regex metacharacters or whitelist allowed characters, apply it to each pattern when you build uniquePatterns or in the .map step that produces conditions, and if a pattern fails validation throw/log a clear error referencing skillName so unsafe patterns are rejected instead of inserted into clauses.src/lib/runner.js (2)
167-179: Fence detection may miss edge cases.The regex
/^```[^\n]*\n[\s\S]*?\n```/gmrequires both a newline after the opening fence and before the closing fence. This misses:
- Fences at the very start of output (no
^match withmflag starts at newlines)- Unclosed fences (the
*?non-greedy match handles this gracefully by not matching)For typical Claude output this should work, but edge cases may cause false positives where
</file>inside an unusual fence structure is incorrectly treated as valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/runner.js` around lines 167 - 179, The current fence detection (fenceRegex and isInsideFence) is too strict and can miss or mis-handle edge cases because it requires newlines after the opening fence and before the closing fence; update the fenceRegex to robustly detect fenced code blocks at the start/end of the string and unclosed fences (for example use a pattern that allows the opening fence at string start or after a newline and matches until a closing fence or end-of-string, enabling dotall behavior), then when iterating matches for fenceRanges (the same loop that builds fenceRanges) treat matches that lack a closing fence as running to output.length so isInsideFence correctly marks positions inside unclosed fences; keep using fenceRanges and isInsideFence unchanged otherwise.
181-189:</file>at position 0 won't be detected.The
closeRegexpattern/\n<\/file>/grequires a leading newline. If</file>appears at the very start of output (position 0), it won't be found inclosePositions. This is likely fine for real Claude output but could cause issues in edge cases or tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/runner.js` around lines 181 - 189, The regex only matches "</file>" when preceded by a newline, so a closing tag at position 0 is missed; update the matching logic to allow either start-of-string or newline before the tag (e.g. change closeRegex to use a group like /(^|\n)<\/file>/g) and when you compute the position to push into closePositions use the actual tag start (cm.index + length of the captured prefix, i.e. cm[1].length) so isInsideFence receives the true index; keep using closePositions, closeRegex, isInsideFence and output in the same loop.
🤖 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/hooks/skill-activation-prompt.mjs:
- Around line 35-52: readSkillContent currently builds file paths from the
unvalidated skillName (used in possiblePaths) which allows path traversal; fix
by validating/normalizing skillName before using join: ensure skillName contains
only a safe basename (e.g., no path separators or '..') or strip to
path.basename(skillName), then construct paths and use path.resolve on each
candidate and verify the resolved path starts with the intended skills directory
(resolve(join(projectDir, '.claude', 'skills'))); if validation fails or the
resolved path is outside that directory, skip or return null – update
readSkillContent to perform these checks and keep existing try/catch behavior.
In @.claude/skills/skill-rules.json:
- Around line 44-50: Normalize promptTriggers.keywords before persisting rules:
in the rules-generation path, locate where promptTriggers.keywords are
read/assembled (the code that writes .claude/skills/skill-rules.json) and run a
normalization step that trims whitespace and leading/trailing punctuation
(commas, periods, colons, semicolons, etc.) and lowercases each keyword so
variants like "conventions," become "conventions"; apply the same step to all
keyword arrays referenced in the file (e.g., the entries around lines 44–50,
74–75, 167–168, 238–240) so all rules are written with cleaned keywords.
In `@bin/cli.js`:
- Around line 104-105: The CLI defines the option as .option('--no-hooks', ...)
but the runtime check uses options.noHooks (in the code path around the
hooks-install logic), which never exists because Commander maps --no-hooks to
options.hooks; update the conditional that guards hook installation (the check
currently looking at options.noHooks) to use the Commander-mapped property by
changing it to if (options.hooks !== false) (or alternately rename the option to
.option('--skip-hooks', ...) and keep the existing check), ensuring the
hooks-install branch respects the flag.
In `@src/lib/runner.js`:
- Around line 270-278: The containment check is vulnerable to partial prefix
matches; update the loop that computes resolved from refPath (variables:
refPath, repoPath, resolved, join) to use path.resolve/path.normalize and then
verify containment using a safe test such as computing path.relative(repoPath,
resolved) and ensuring it does not start with '..' (or alternatively check
resolved starts with repoPath + path.sep) before proceeding to existsSync and
pushing into issues; ensure glob and traversal checks remain and keep the same
issue push behavior for non-existent paths.
In `@src/prompts/partials/skill-format.md`:
- Line 101: The inline code span in the sentence "File patterns MUST be on their
own line, prefixed with - , wrapped in backticks." contains a trailing space
after the dash which triggers MD038; edit the text in skill-format.md to remove
the trailing space inside the inline code span so the code span contains only
the dash character, and if you need to show that a space follows the dash in
practice, state that verbally (e.g., "prefixed with a dash followed by a space")
rather than leaving the space inside the inline code span.
In `@src/templates/hooks/skill-activation-prompt.mjs`:
- Around line 35-52: readSkillContent has a path traversal risk because
skillName is interpolated into possiblePaths; add validation at the top of
readSkillContent to reject or sanitize any skillName containing path separators
or traversal sequences (e.g., '/', '\', '..') and restrict to a safe whitelist
(e.g., /^[A-Za-z0-9_-]+$/) before building possiblePaths; if validation fails,
return null (or throw) to preserve current behavior. Use the function name
readSkillContent and the variable skillName when locating where to add this
check.
In `@src/templates/hooks/skill-activation-prompt.sh`:
- Line 37: The unguarded cd "$SCRIPT_DIR" can fail silently; update the script
so the cd invocation that uses the SCRIPT_DIR variable is error-checked and the
hook exits on failure — after attempting cd "$SCRIPT_DIR" test its exit status
and if non-zero print a clear error to stderr (mentioning SCRIPT_DIR and the
failed cd) and exit with a non‑zero status so the hook cannot continue in the
wrong directory.
---
Nitpick comments:
In @.claude/hooks/post-tool-use-tracker.sh:
- Line 53: The declaration/assignment of repo should be split to satisfy
ShellCheck SC2155: first declare the local variable, then assign it; i.e.,
replace the single-line "local repo=$(echo "$relative_path" | cut -d'/' -f1)"
with a separate "local repo" declaration followed by "repo=$(echo
"$relative_path" | cut -d'/' -f1)"; use the existing variable names repo and
relative_path so the change is minimal and keeps the same behavior.
- Around line 209-231: The detect_skill_domain function currently contains
hardcoded aspens-specific patterns that are overwritten during hook installation
by generateDomainPatterns(); update the function declaration to include a clear
single-line stub comment (e.g., "# STUB: replaced during installation by
generateDomainPatterns()") so readers know this is a template placeholder, and
place that comment immediately above or on the same line as the
detect_skill_domain() declaration to prevent confusion.
In @.claude/hooks/skill-activation-prompt.mjs:
- Around line 219-227: formatOutput currently re-checks/loading skill content
with the loop that calls readSkillContent for each item in matched, but main()
already populates skill.content before calling formatOutput, making that work
redundant; remove the content-loading block from formatOutput (the for-loop that
checks if (!skill.content) and calls readSkillContent) and rely on main() to set
skill.content (or, if defensive behavior is desired, change to a no-op guard
comment), leaving readSkillContent usage only in main() where skill.content is
initially set.
In `@src/commands/doc-init.js`:
- Around line 472-474: The current block that writes skill rules uses
writeFileSync(rulesPath, JSON.stringify(rules, null, 2) + '\n') only when
!options.dryRun, but provides no feedback when options.dryRun is true; update
the conditional around writeFileSync so that when options.dryRun is true you log
a clear dry-run message (e.g., using console.log or the existing logger)
mentioning rulesPath and that the rules JSON was not written, while preserving
the existing write behavior when options.dryRun is false and still using rules
and rulesPath.
- Around line 508-511: The current replacement using stubRegex
(detect_skill_domain\(\)\s*\{[\s\S]*?\n\}) is fragile and can truncate
detect_skill_domain() when it contains nested braces or multiple newlines;
update the logic that modifies trackerContent to look for explicit markers
instead—e.g., search for a block delimited by unique tokens like "# BEGIN
detect_skill_domain" and "# END detect_skill_domain" (or add those markers to
the template if missing), then replace the entire marked block with
domainPatterns.trim(); ensure the code references the same markers when
detecting and replacing, and keep a safe fallback if markers aren't present to
avoid accidental truncation.
In `@src/lib/runner.js`:
- Around line 167-179: The current fence detection (fenceRegex and
isInsideFence) is too strict and can miss or mis-handle edge cases because it
requires newlines after the opening fence and before the closing fence; update
the fenceRegex to robustly detect fenced code blocks at the start/end of the
string and unclosed fences (for example use a pattern that allows the opening
fence at string start or after a newline and matches until a closing fence or
end-of-string, enabling dotall behavior), then when iterating matches for
fenceRanges (the same loop that builds fenceRanges) treat matches that lack a
closing fence as running to output.length so isInsideFence correctly marks
positions inside unclosed fences; keep using fenceRanges and isInsideFence
unchanged otherwise.
- Around line 181-189: The regex only matches "</file>" when preceded by a
newline, so a closing tag at position 0 is missed; update the matching logic to
allow either start-of-string or newline before the tag (e.g. change closeRegex
to use a group like /(^|\n)<\/file>/g) and when you compute the position to push
into closePositions use the actual tag start (cm.index + length of the captured
prefix, i.e. cm[1].length) so isInsideFence receives the true index; keep using
closePositions, closeRegex, isInsideFence and output in the same loop.
In `@src/lib/skill-writer.js`:
- Around line 420-422: The dedupeStrings function currently lowercases every
string which destroys original casing; change it to perform case-insensitive
deduplication while preserving the first occurrence's original casing by
iterating over arr, tracking seen lowercased values in a Set, and pushing the
original string only when its lowercased form hasn't been seen yet (update
function dedupeStrings accordingly so it returns the deduped list preserving
original casing of first occurrences).
- Around line 199-204: The duplicate-check using JSON.stringify in
merged.hooks[eventType].some(e => JSON.stringify(e) ===
JSON.stringify(templateEntry)) is order-dependent; replace it with a
deterministic deep-equality check (e.g., implement a helper like
hookEntriesEqual(a,b) that compares objects after sorting keys or performs a
recursive deepEqual) and use merged.hooks[eventType].some(e =>
hookEntriesEqual(e, templateEntry)); add the helper (e.g., hookEntriesEqual or
stableStringify) adjacent to the current logic so duplicate detection works
regardless of property order.
- Around line 123-127: The code injects raw regex patterns into the bash test
([[ "$file" =~ ${p} ]]) which can allow regex metacharacter injection; fix by
validating or escaping patterns before building conditions: add an escape
function (e.g. escapeBashRegex) that backslashes regex metacharacters or
whitelist allowed characters, apply it to each pattern when you build
uniquePatterns or in the .map step that produces conditions, and if a pattern
fails validation throw/log a clear error referencing skillName so unsafe
patterns are rejected instead of inserted into clauses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcce7423-7fa1-4ece-b0a7-c91070897fee
📒 Files selected for processing (15)
.claude/hooks/post-tool-use-tracker.sh.claude/hooks/skill-activation-prompt.mjs.claude/hooks/skill-activation-prompt.sh.claude/settings.json.claude/skills/skill-rules.jsonbin/cli.jssrc/commands/doc-init.jssrc/lib/runner.jssrc/lib/skill-reader.jssrc/lib/skill-writer.jssrc/prompts/partials/skill-format.mdsrc/templates/hooks/post-tool-use-tracker.shsrc/templates/hooks/skill-activation-prompt.mjssrc/templates/hooks/skill-activation-prompt.shsrc/templates/settings/settings.json
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/commands/doc-sync.js (2)
324-333: Cross-platform consideration:whichis not available on Windows.The
whichcommand won't work on Windows systems. Consider usingwhere aspenson Windows or a cross-platform approach.Suggested cross-platform fix
function resolveAspensPath() { + const isWindows = process.platform === 'win32'; + const cmd = isWindows ? 'where aspens' : 'which aspens'; try { - const resolved = execSync('which aspens', { + const resolved = execSync(cmd, { encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'], - }).trim(); + }).trim().split('\n')[0]; // 'where' may return multiple lines if (resolved && existsSync(resolved)) return resolved; } catch { /* not in PATH */ } return 'npx aspens'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-sync.js` around lines 324 - 333, The resolveAspensPath function uses execSync('which aspens') which fails on Windows; update resolveAspensPath to choose the platform-appropriate lookup (use execSync('where aspens') when process.platform === 'win32' and execSync('which aspens') otherwise), keep the existing encoding/stdio options, trim the result, and still validate with existsSync before returning; if lookup fails return the existing fallback 'npx aspens' and ensure you catch and handle the execSync error (use a catch(error) if needed) so behavior remains unchanged on lookup failure.
348-379:shasummay not be available on all Linux distributions.Some systems have
sha1suminstead ofshasum. The hash generation could fail silently.More portable hash command
- REPO_HASH="\$(echo "\$REPO_ROOT" | shasum | cut -c1-8)" + REPO_HASH="\$(echo "\$REPO_ROOT" | (shasum 2>/dev/null || sha1sum 2>/dev/null || md5sum 2>/dev/null) | cut -c1-8)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-sync.js` around lines 348 - 379, The hook uses shasum to compute REPO_HASH which isn’t available on all systems; update the __aspens_doc_sync hook’s REPO_HASH computation to use a portable fallback (try shasum, then sha1sum, or use a single-command portable approach) so the hash generation won't fail silently — specifically replace the line setting REPO_HASH (currently using echo "$REPO_ROOT" | shasum | cut -c1-8) inside the __aspens_doc_sync block with a fallback that detects shasum or sha1sum and extracts the first 8 hex characters.src/commands/doc-init.js (1)
517-535: Minor: Stub regex replacement could be fragile.The regex
/detect_skill_domain\(\)\s*\{[\s\S]*?\n\}/stops at the first\n}. If the template stub ever contains nested braces on their own lines, this would break. Consider using a more explicit marker-based approach.Alternative: marker-based replacement
In the template, wrap the stub with markers:
# >>> detect_skill_domain stub >>> detect_skill_domain() { echo "" } # <<< detect_skill_domain stub <<<Then replace using:
const stubRegex = /# >>> detect_skill_domain stub >>>[\s\S]*?# <<< detect_skill_domain stub <<</;🤖 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 517 - 535, The current regex used to replace the detect_skill_domain stub in post-tool-use-tracker.sh (see detect_skill_domain(), trackerContent, trackerSrc, trackerDest) is brittle and can fail with nested braces; change the approach to marker-based replacement: update the template in TEMPLATES_DIR/hooks/post-tool-use-tracker.sh to wrap the stub with unique start/end markers (e.g. "# >>> detect_skill_domain stub >>>" and "# <<< detect_skill_domain stub <<<") and then replace using a regex that matches everything between those markers (use a /start[\s\S]*?end/ style pattern) when generating trackerContent; ensure you trim/validate replacement and keep the writeFileSync/chmodSync flow unchanged and still respect options.dryRun.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands/doc-init.js`:
- Around line 517-535: The current regex used to replace the detect_skill_domain
stub in post-tool-use-tracker.sh (see detect_skill_domain(), trackerContent,
trackerSrc, trackerDest) is brittle and can fail with nested braces; change the
approach to marker-based replacement: update the template in
TEMPLATES_DIR/hooks/post-tool-use-tracker.sh to wrap the stub with unique
start/end markers (e.g. "# >>> detect_skill_domain stub >>>" and "# <<<
detect_skill_domain stub <<<") and then replace using a regex that matches
everything between those markers (use a /start[\s\S]*?end/ style pattern) when
generating trackerContent; ensure you trim/validate replacement and keep the
writeFileSync/chmodSync flow unchanged and still respect options.dryRun.
In `@src/commands/doc-sync.js`:
- Around line 324-333: The resolveAspensPath function uses execSync('which
aspens') which fails on Windows; update resolveAspensPath to choose the
platform-appropriate lookup (use execSync('where aspens') when process.platform
=== 'win32' and execSync('which aspens') otherwise), keep the existing
encoding/stdio options, trim the result, and still validate with existsSync
before returning; if lookup fails return the existing fallback 'npx aspens' and
ensure you catch and handle the execSync error (use a catch(error) if needed) so
behavior remains unchanged on lookup failure.
- Around line 348-379: The hook uses shasum to compute REPO_HASH which isn’t
available on all systems; update the __aspens_doc_sync hook’s REPO_HASH
computation to use a portable fallback (try shasum, then sha1sum, or use a
single-command portable approach) so the hash generation won't fail silently —
specifically replace the line setting REPO_HASH (currently using echo
"$REPO_ROOT" | shasum | cut -c1-8) inside the __aspens_doc_sync block with a
fallback that detects shasum or sha1sum and extracts the first 8 hex characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: afebb115-cd81-4d59-89e2-ee96f8efb39a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
bin/cli.jssrc/commands/doc-init.jssrc/commands/doc-sync.jstests/git-hook.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/cli.js
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/hooks/post-tool-use-tracker.sh:
- Around line 235-239: get_session_file currently hardcodes /tmp which
mismatches the Node side that uses os.tmpdir() (respecting TMPDIR); update
get_session_file to derive the temp directory from the TMPDIR env var (e.g.
TMPDIR:-/tmp) instead of hardcoding /tmp so both shells and the Node code
(skill-activation-prompt.mjs using os.tmpdir()) reference the same location;
keep the existing hash logic and file name (claude-skills-${hash}.json) and only
replace the /tmp prefix with the resolved TMPDIR variable.
In @.claude/hooks/skill-activation-prompt.mjs:
- Around line 94-103: The getSessionActiveSkills function currently returns
session.active_skills unconditionally; instead, load the session (from
sessionFile), compare session.repo to the current repo identifier and only
return session.active_skills when they match (otherwise return an empty array).
Update getSessionActiveSkills to accept or derive a currentRepo parameter (or
compute it from projectDir) and perform the check before returning
session.active_skills; reference the sessionFile/session and
session.active_skills symbols and ensure callers pass the current repo id if you
add a parameter.
- Around line 41-49: The current containment check uses a hardcoded '/' prefix
which breaks on Windows; replace the startsWith check for candidate against
skillsRoot with a platform-aware test using path.relative: compute
relative(skillsRoot, candidate) and skip the candidate if it begins with '..' or
is an absolute escape; update the loop that iterates possiblePaths (variables:
skillsRoot, possiblePaths, candidate) to use path.relative(...) to determine if
candidate is outside skillsRoot and only accept candidates that resolve inside
the skills directory.
In `@src/commands/doc-init.js`:
- Around line 496-497: The mkdirSync call unconditionally creates hooksDir
causing side-effects during --dry-run; update installHooks to only create the
directory when not in dry run by checking options.dryRun (i.e., wrap or guard
the mkdirSync(hooksDir, { recursive: true }) with a condition like if
(!options.dryRun) so hooksDir is not created when options.dryRun is true; locate
this change in the installHooks function and ensure hooksDir and options.dryRun
are the referenced symbols.
In `@src/lib/runner.js`:
- Around line 251-263: Add validation after computing contentAfterFrontmatter to
ensure the required markdown sections exist: check contentAfterFrontmatter
(using the same variables fmEnd/contentAfterFrontmatter/filePath) for headings
"Activation", "Key Files", "Key Concepts", and "Critical Rules" (e.g. regex
looking for heading lines like /^#+\s*Activation\b/m). If any are missing, push
an issues entry (similar to the existing issues.push) — e.g. issues.push({ file:
filePath, issue: 'missing-sections', detail: 'Missing sections: Activation, Key
Files, Key Concepts, Critical Rules' }) — and list which specific section names
are absent in the detail field.
- Around line 200-212: The parsing currently always picks closePos from
closePositions and consumes through that closing tag even when another <file
...> open starts earlier; update the logic around closePos, contentStart,
openTagPattern, and nextOpen so that if a nextOpen (from
remaining.match(/<file\s+path=/)) exists and its index is less than the computed
closePos offset you treat that nextOpen as the truncation boundary: set content
to the slice up to nextOpen.index (or up to closePos if nextOpen is absent or
after closePos), and adjust openTagPattern.lastIndex to the correct position
after the truncation (either the start of the next <file...> or past the </file>
you actually consumed) so you never swallow the following file body.
In `@src/lib/skill-writer.js`:
- Around line 46-58: Use the on-disk slug (e.g., skill.slug or derive from
skill.filepath) as the rule key instead of using frontmatter.name: keep
frontmatter.name only as display metadata (leave description =
skill.frontmatter?.description), derive keywords as before (parseKeywords,
deriveKeywords, dedupeStrings) and generate intentPatterns using that slug-based
key (replace uses of name where the rule ID/lookup is needed). Also ensure
skill-writer writes the skill files into the target repo path
.claude/skills/<slug>/skill.md (store frontmatter.name inside the file as the
display name) so lookups performed by readSkillContent resolve correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 217b3477-63f9-4eca-b8ee-eaff1189e006
📒 Files selected for processing (13)
.claude/hooks/post-tool-use-tracker.sh.claude/hooks/skill-activation-prompt.mjs.claude/hooks/skill-activation-prompt.sh.claude/settings.json.bak.claude/skills/skill-rules.json.claude/tsc-cache/a73e8196-0e5a-4a0f-ad77-f145a75bc3f0/affected-repos.txtsrc/commands/doc-init.jssrc/lib/runner.jssrc/lib/skill-writer.jssrc/prompts/partials/skill-format.mdsrc/templates/hooks/post-tool-use-tracker.shsrc/templates/hooks/skill-activation-prompt.mjssrc/templates/hooks/skill-activation-prompt.sh
✅ Files skipped from review due to trivial changes (3)
- .claude/tsc-cache/a73e8196-0e5a-4a0f-ad77-f145a75bc3f0/affected-repos.txt
- .claude/settings.json.bak
- .claude/skills/skill-rules.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/prompts/partials/skill-format.md
- src/templates/hooks/post-tool-use-tracker.sh
- src/templates/hooks/skill-activation-prompt.mjs
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/lib/runner.js (1)
200-212:⚠️ Potential issue | 🟠 MajorStop at the next
<file ...>when it appears before the chosen close tag.This still consumes through a later file’s
</file>if another<file path=...>starts first, so the earlier file can swallow the later file body and the later file never gets parsed. TreatnextOpen < closePosas the truncation boundary here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/runner.js` around lines 200 - 212, When choosing the truncation boundary, account for a following "<file" that appears before the selected closePos so we stop at that next open tag instead of consuming into a later file; compute remaining = output.slice(contentStart) and nextOpen = remaining.match(/<file\s+path=/), then compare absolute positions (contentStart + nextOpen.index) against closePos and use the earlier of the two as the end bound (e.g., endPos = nextOpen && contentStart + nextOpen.index < closePos ? contentStart + nextOpen.index : closePos) when slicing content and when updating openTagPattern.lastIndex; update logic around closePos, remaining, nextOpen, content and openTagPattern.lastIndex accordingly.
🤖 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/hooks/post-tool-use-tracker.sh:
- Around line 217-223: The current broad regex match for "/tests/" in the
post-tool-use-tracker.sh detection logic causes first-match-wins false positives
(e.g., tests/graph-builder.test.js hitting the claude-runner branch), so tighten
the claude-runner test pattern in skill-rules.json (replace the generic
"tests/*" style pattern with the specific "tests/*extract*" or similar more
constrained pattern) or reorder rules so filename-specific patterns (e.g.,
graph-builder.test, scanner.test) are evaluated before generic directory
matches; update the generator that produces the /tests/ pattern so the
claude-runner filePatterns no longer emit a plain "/tests/" matcher and ensure
detected_skills assignment (detected_skills="claude-runner") only fires for the
narrowed pattern.
In `@src/commands/doc-init.js`:
- Around line 547-556: The hook-install failure paths currently swallow errors
by calling return after logging (e.g., the templateSettings JSON read block that
assigns templateSettings and the similar block around lines 597-600); change
these to propagate failure instead of silently returning by either rethrowing
the caught error (throw new Error with contextual message) or returning a
clearly defined failure sentinel that the caller checks and uses to abort
messaging; update the catch blocks around readFileSync/TEMPLATES_DIR and any
other hook-install catch handlers to include contextual information (hookSpinner
state, file path) in the thrown/reported error so the caller can detect and stop
with an appropriate failure status.
In `@src/commands/doc-sync.js`:
- Around line 383-388: The current check treats legacy hooks (containing 'aspens
doc sync') as already-installed and returns early, preventing the migration path
recommended by removeGitHook(); instead detect the legacy marker and migrate it:
in the existsSync(hookPath) branch (variable hookPath, const existing) update
the logic so that if existing.includes('aspens doc sync') you transform/replace
the legacy hook contents to include the new marker string ('aspens doc-sync
hook' or whatever current marker your installer uses) and write the updated
content back (then continue installation), while still keeping the early-return
for truly up-to-date hooks that already include the new marker; reference
removeGitHook() as the related function that expects re-install/migration
behavior.
- Around line 347-376: The hookBlock embeds the resolved executable path via the
aspensCmd variable without quotes, which will break if the path contains spaces
or special chars; update the template injected in hookBlock (the
__aspens_doc_sync function) to quote the executable when invoking it by
replacing the unquoted ${aspensCmd} doc sync with a quoted invocation
("${aspensCmd}" doc sync) so the generated shell hook calls the correct path
safely.
In `@src/lib/runner.js`:
- Around line 253-256: The validator only checks for 'name:' in the extracted
frontmatter (frontmatterMatch) so files missing 'description:' slip through;
update the validation inside the block that uses frontmatterMatch to verify both
'name:' and 'description:' exist in frontmatterMatch[1] (e.g., check
includes('name:') && includes('description:') or use a small regex to match
description:) and push the same missing-frontmatter issue when either is absent,
updating the detail message to explicitly list both required keys (name,
description); touch the code around frontmatterMatch and the issues.push call to
implement this check.
- Around line 193-196: The loop that processes openTagPattern matches
(openMatch, openTagPattern, output, sanitizePath) must skip any <file ...>
openings that reside inside fenced code blocks (the same fence-aware logic you
applied to filtering </file> closes); update the while ((openMatch =
openTagPattern.exec(output))...) body to check whether openMatch.index (or the
match span) falls inside a fenced block using the existing fence-detection
routine/logic and continue if so, so opens inside fences are ignored and cannot
bind to real closing tags.
---
Duplicate comments:
In `@src/lib/runner.js`:
- Around line 200-212: When choosing the truncation boundary, account for a
following "<file" that appears before the selected closePos so we stop at that
next open tag instead of consuming into a later file; compute remaining =
output.slice(contentStart) and nextOpen = remaining.match(/<file\s+path=/), then
compare absolute positions (contentStart + nextOpen.index) against closePos and
use the earlier of the two as the end bound (e.g., endPos = nextOpen &&
contentStart + nextOpen.index < closePos ? contentStart + nextOpen.index :
closePos) when slicing content and when updating openTagPattern.lastIndex;
update logic around closePos, remaining, nextOpen, content and
openTagPattern.lastIndex accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da7cbd8e-0527-4807-a5c7-e55e010a10eb
📒 Files selected for processing (8)
.claude/hooks/post-tool-use-tracker.sh.claude/hooks/skill-activation-prompt.mjs.claude/tsc-cache/dcec4868-8c5d-4534-a3d7-e41784332d3c/affected-repos.txtsrc/commands/doc-init.jssrc/commands/doc-sync.jssrc/lib/runner.jssrc/templates/hooks/post-tool-use-tracker.shsrc/templates/hooks/skill-activation-prompt.mjs
✅ Files skipped from review due to trivial changes (2)
- .claude/tsc-cache/dcec4868-8c5d-4534-a3d7-e41784332d3c/affected-repos.txt
- .claude/hooks/skill-activation-prompt.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/templates/hooks/skill-activation-prompt.mjs
Comment on lines
+217
to
+223
| elif [[ "$file" =~ /runner ]] || [[ "$file" =~ /skill-writer ]] || [[ "$file" =~ /prompts/ ]] || [[ "$file" =~ /tests/ ]]; then | ||
| detected_skills="claude-runner" | ||
| elif [[ "$file" =~ /doc-sync ]]; then | ||
| detected_skills="doc-sync" | ||
| elif [[ "$file" =~ /graph-builder ]] || [[ "$file" =~ /graph-builder.test ]]; then | ||
| detected_skills="import-graph" | ||
| elif [[ "$file" =~ /scanner ]] || [[ "$file" =~ /scan ]] || [[ "$file" =~ /scanner.test ]]; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, view the full context around lines 217-223
head -n 230 .claude/hooks/post-tool-use-tracker.sh | tail -n 20Repository: aspenkit/aspens
Length of output: 1236
🏁 Script executed:
# Find and examine the generator file that creates post-tool-use-tracker.sh
find . -type f -name "*.js" -o -name "*.sh" | head -20
git ls-files | grep -E "(skill-writer|generate|hook)" | head -20Repository: aspenkit/aspens
Length of output: 964
🏁 Script executed:
# Search for references to post-tool-use-tracker.sh generation
rg "post-tool-use-tracker" --type sh --type js -B 2 -A 2Repository: aspenkit/aspens
Length of output: 1668
🏁 Script executed:
# Check if there are actual test files that would match the patterns
find . -type f -name "*graph-builder.test*" -o -name "*scanner.test*"Repository: aspenkit/aspens
Length of output: 112
🏁 Script executed:
# Find the skill-rules.json file
find . -name "skill-rules.json" -o -name "*skill*rules*"Repository: aspenkit/aspens
Length of output: 92
🏁 Script executed:
# Check the template file to see the original rules
cat src/templates/hooks/post-tool-use-tracker.sh | head -50Repository: aspenkit/aspens
Length of output: 1489
🏁 Script executed:
# View the skill-rules.json to see the source patterns
cat ./.claude/skills/skill-rules.json | jq . | head -100Repository: aspenkit/aspens
Length of output: 2747
🏁 Script executed:
# Also check how the patterns are applied in doc-init.js
sed -n '1,50p' src/commands/doc-init.js | grep -A 20 "post-tool-use-tracker"Repository: aspenkit/aspens
Length of output: 41
🏁 Script executed:
# Search for where the skill rules are processed/ordered
rg "filePatterns" -A 5 -B 5 --type jsRepository: aspenkit/aspens
Length of output: 4581
🏁 Script executed:
# Get the complete skill-rules.json to see all filePatterns
cat ./.claude/skills/skill-rules.json | jq '.skills | keys' && echo "---" && cat ./.claude/skills/skill-rules.json | jq '.skills | to_entries[] | {skill: .key, patterns: .value.filePatterns}'Repository: aspenkit/aspens
Length of output: 1305
🏁 Script executed:
# Check the globToBashPattern function to understand the conversion
rg "globToBashPattern" -A 20 src/lib/skill-writer.jsRepository: aspenkit/aspens
Length of output: 1519
🏁 Script executed:
# See the complete globToBashPattern function to understand how tests/*extract* is converted
rg "function globToBashPattern" -A 40 src/lib/skill-writer.jsRepository: aspenkit/aspens
Length of output: 1349
🏁 Script executed:
# Verify what the actual generated patterns look like by checking if there's a debug mode or examining the actual conversion
python3 << 'EOF'
# Simulate the globToBashPattern logic
from pathlib import Path
def glob_to_bash_pattern(glob_str):
"""Mimic the globToBashPattern function logic"""
if not glob_str:
return None
cleaned = glob_str
# Remove leading **/ or */
cleaned = cleaned.lstrip('*').lstrip('/')
if glob_str.startswith('**/'):
cleaned = glob_str[3:]
elif glob_str.startswith('*/'):
cleaned = glob_str[2:]
else:
cleaned = glob_str
# Handle dir/**/*.ext patterns
if cleaned.endswith('/**') or '//**/' in cleaned:
# Extract directory
parts = cleaned.split('/')
if parts:
return f"/{parts[0]}"
# Check for file with extension and no wildcards
if '.' in cleaned and '*' not in cleaned:
filename = Path(cleaned).name
name = filename.rsplit('.', 1)[0] if '.' in filename else filename
if len(name) > 2:
return f"/{name}"
return None
# For patterns with wildcards in middle (like tests/*extract*)
# Extract the directory part before wildcard
parts = cleaned.split('/')
if parts and parts[0]:
return f"/{parts[0]}"
return None
patterns_to_test = [
"src/lib/runner.js",
"src/lib/skill-writer.js",
"src/prompts/**/*.md",
"tests/*extract*",
"src/lib/graph-builder.js",
"tests/graph-builder.test.js",
"src/lib/scanner.js",
"tests/scanner.test.js"
]
for pattern in patterns_to_test:
result = glob_to_bash_pattern(pattern)
print(f"{pattern:40} → {result}")
EOFRepository: aspenkit/aspens
Length of output: 494
The broad /tests/ match masks specific test-domain rules.
Detection is first-match-wins, so tests/graph-builder.test.js and tests/scanner.test.js will incorrectly activate claude-runner instead of import-graph and repo-scanning. The tests/*extract* pattern in claude-runner's filePatterns converts to /tests/, which is too broad. Fix this in skill-rules.json by making the claude-runner test pattern more specific (e.g., tests/*extract* should not match test files for other domains), or refactor the pattern ordering in the generator to prioritize specific filename matches over generic directory patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/post-tool-use-tracker.sh around lines 217 - 223, The current
broad regex match for "/tests/" in the post-tool-use-tracker.sh detection logic
causes first-match-wins false positives (e.g., tests/graph-builder.test.js
hitting the claude-runner branch), so tighten the claude-runner test pattern in
skill-rules.json (replace the generic "tests/*" style pattern with the specific
"tests/*extract*" or similar more constrained pattern) or reorder rules so
filename-specific patterns (e.g., graph-builder.test, scanner.test) are
evaluated before generic directory matches; update the generator that produces
the /tests/ pattern so the claude-runner filePatterns no longer emit a plain
"/tests/" matcher and ensure detected_skills assignment
(detected_skills="claude-runner") only fires for the narrowed pattern.
Comment on lines
+547
to
+556
| // 9d: Merge settings.json | ||
| let templateSettings; | ||
| try { | ||
| templateSettings = JSON.parse( | ||
| readFileSync(join(TEMPLATES_DIR, 'settings', 'settings.json'), 'utf8') | ||
| ); | ||
| } catch (err) { | ||
| hookSpinner.stop(pc.yellow('Hook installation incomplete')); | ||
| p.log.warn(`Could not read template settings: ${err.message}`); | ||
| return; |
There was a problem hiding this comment.
Propagate hook-install failures instead of only logging them.
These failure paths just return, so --hooks-only can still end with “Hooks updated” and the main flow can exit successfully even when hook installation was incomplete or failed. Return an explicit failure status or rethrow so the caller can stop or adjust its messaging.
Also applies to: 597-600
🤖 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 547 - 556, The hook-install failure
paths currently swallow errors by calling return after logging (e.g., the
templateSettings JSON read block that assigns templateSettings and the similar
block around lines 597-600); change these to propagate failure instead of
silently returning by either rethrowing the caught error (throw new Error with
contextual message) or returning a clearly defined failure sentinel that the
caller checks and uses to abort messaging; update the catch blocks around
readFileSync/TEMPLATES_DIR and any other hook-install catch handlers to include
contextual information (hookSpinner state, file path) in the thrown/reported
error so the caller can detect and stop with an appropriate failure status.
Comment on lines
+347
to
+376
| const aspensCmd = resolveAspensPath(); | ||
|
|
||
| const hookBlock = ` | ||
| # >>> aspens doc-sync hook (do not edit) >>> | ||
| __aspens_doc_sync() { | ||
| REPO_ROOT="\$(git rev-parse --show-toplevel 2>/dev/null)" || return 0 | ||
| REPO_HASH="\$(echo "\$REPO_ROOT" | (shasum 2>/dev/null || sha1sum 2>/dev/null || md5sum 2>/dev/null) | cut -c1-8)" | ||
| ASPENS_LOCK="/tmp/aspens-sync-\${REPO_HASH}.lock" | ||
| ASPENS_LOG="/tmp/aspens-sync-\${REPO_HASH}.log" | ||
|
|
||
| # Cooldown: skip if last sync was less than 5 minutes ago | ||
| if [ -f "\$ASPENS_LOCK" ]; then | ||
| LAST_RUN=\$(cat "\$ASPENS_LOCK" 2>/dev/null || echo 0) | ||
| NOW=\$(date +%s) | ||
| if [ \$((NOW - LAST_RUN)) -lt 300 ]; then | ||
| return 0 | ||
| fi | ||
| fi | ||
| fi | ||
| echo \$(date +%s) > "\$ASPENS_LOCK" | ||
| echo \$(date +%s) > "\$ASPENS_LOCK" | ||
|
|
||
| # Clean up stale lock files older than 1 hour | ||
| find /tmp -maxdepth 1 -name "aspens-sync-*.lock" -mmin +60 -delete 2>/dev/null | ||
| # Clean up stale lock files older than 1 hour | ||
| find /tmp -maxdepth 1 -name "aspens-sync-*.lock" -mmin +60 -delete 2>/dev/null | ||
|
|
||
| # Run in background so commit isn't blocked | ||
| npx aspens doc sync --commits 1 "\$(git rev-parse --show-toplevel)" & | ||
| `; | ||
| # Truncate log if over 200 lines | ||
| if [ -f "\$ASPENS_LOG" ] && [ "\$(wc -l < "\$ASPENS_LOG" 2>/dev/null || echo 0)" -gt 200 ]; then | ||
| tail -100 "\$ASPENS_LOG" > "\$ASPENS_LOG.tmp" && mv "\$ASPENS_LOG.tmp" "\$ASPENS_LOG" | ||
| fi | ||
|
|
||
| const hookFull = `#!/bin/sh${hookCommand}`; | ||
| # Run in background with logging | ||
| (echo "[sync] \$(date '+%Y-%m-%d %H:%M:%S') started" >> "\$ASPENS_LOG" && ${aspensCmd} doc sync --commits 1 "\$REPO_ROOT" >> "\$ASPENS_LOG" 2>&1; echo "[sync] \$(date '+%Y-%m-%d %H:%M:%S') finished (exit \$?)" >> "\$ASPENS_LOG") & |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and find the relevant code
wc -l src/commands/doc-sync.jsRepository: aspenkit/aspens
Length of output: 87
🏁 Script executed:
# Get the function containing lines 347-376 and surrounding context
sed -n '320,400p' src/commands/doc-sync.js | cat -nRepository: aspenkit/aspens
Length of output: 3584
🏁 Script executed:
# Find the resolveAspensPath() function definition
rg -n 'resolveAspensPath' src/commands/doc-sync.js -A 5 -B 2Repository: aspenkit/aspens
Length of output: 610
Quote the embedded executable path in the shell hook.
Line 357 embeds ${aspensCmd} unquoted into the shell script. If the resolved path contains spaces or special characters, it will break or alter the command. Change ${aspensCmd} doc sync to "${aspensCmd}" doc sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/doc-sync.js` around lines 347 - 376, The hookBlock embeds the
resolved executable path via the aspensCmd variable without quotes, which will
break if the path contains spaces or special chars; update the template injected
in hookBlock (the __aspens_doc_sync function) to quote the executable when
invoking it by replacing the unquoted ${aspensCmd} doc sync with a quoted
invocation ("${aspensCmd}" doc sync) so the generated shell hook calls the
correct path safely.
Comment on lines
383
to
388
| if (existsSync(hookPath)) { | ||
| const existing = readFileSync(hookPath, 'utf8'); | ||
| if (existing.includes('aspens doc sync')) { | ||
| if (existing.includes('aspens doc-sync hook') || existing.includes('aspens doc sync')) { | ||
| console.log(pc.yellow('\n Hook already installed.\n')); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Legacy hooks can’t be upgraded from this path.
This returns “already installed” for a legacy aspens doc sync hook, but removeGitHook() tells users to re-install first so marker-based removal can work. That upgrade path never happens. Migrate legacy hooks here, or change the remove guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/doc-sync.js` around lines 383 - 388, The current check treats
legacy hooks (containing 'aspens doc sync') as already-installed and returns
early, preventing the migration path recommended by removeGitHook(); instead
detect the legacy marker and migrate it: in the existsSync(hookPath) branch
(variable hookPath, const existing) update the logic so that if
existing.includes('aspens doc sync') you transform/replace the legacy hook
contents to include the new marker string ('aspens doc-sync hook' or whatever
current marker your installer uses) and write the updated content back (then
continue installation), while still keeping the early-return for truly
up-to-date hooks that already include the new marker; reference removeGitHook()
as the related function that expects re-install/migration behavior.
Comment on lines
+253
to
+256
| const frontmatterMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); | ||
| if (!frontmatterMatch || !frontmatterMatch[1].includes('name:')) { | ||
| issues.push({ file: filePath, issue: 'missing-frontmatter', detail: 'Missing YAML frontmatter (name, description)' }); | ||
| } |
There was a problem hiding this comment.
Require description: in frontmatter too.
This validator currently accepts a skill with only name:, so malformed skills can still pass validation and get written. As per coding guidelines, "Skill files must use YAML frontmatter (name, description) followed by structured markdown with sections: Activation, Key Files, Key Concepts, Critical Rules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/runner.js` around lines 253 - 256, The validator only checks for
'name:' in the extracted frontmatter (frontmatterMatch) so files missing
'description:' slip through; update the validation inside the block that uses
frontmatterMatch to verify both 'name:' and 'description:' exist in
frontmatterMatch[1] (e.g., check includes('name:') && includes('description:')
or use a small regex to match description:) and push the same
missing-frontmatter issue when either is absent, updating the detail message to
explicitly list both required keys (name, description); touch the code around
frontmatterMatch and the issues.push call to implement this check.
This was referenced Mar 22, 2026
Merged
Merged
Merged
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.
What
Why
Closes #
How I tested
npm testpasses--dry-runoutput looks correct (if applicable)Checklist
Summary by CodeRabbit
New Features
--hooks-only,--no-hooks, and--remove-hook.Requirements