fix: monorepo fix for subdirectory projects + impact + readme#39
Merged
fix: monorepo fix for subdirectory projects + impact + readme#39
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a new deterministic CLI command Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "aspens CLI"
participant Analyzer as "analyzeImpact()"
participant Scanner as "scanRepo()"
participant Config as "loadConfig()"
participant State as "collectSourceState()"
participant Graph as "buildRepoGraph()"
participant LLM as "LLM Backend (optional)"
participant Summarizer as "summarizeTarget()"
User->>CLI: npx aspens doc impact [path]
CLI->>Analyzer: analyzeImpact(repoPath, options)
Analyzer->>Scanner: scanRepo(repoPath)
Scanner-->>Analyzer: scan results
Analyzer->>Config: loadConfig(repoPath) / infer targets
Config-->>Analyzer: targets
Analyzer->>State: collectSourceState(repoPath)
State-->>Analyzer: source snapshot
Analyzer->>Graph: buildRepoGraph(repoPath) (optional)
Graph-->>Analyzer: graph or null
loop per target
Analyzer->>Summarizer: summarizeTarget(target, scan, graph, state)
Summarizer-->>Analyzer: target report (coverage, hubs, drift, health, actions)
end
alt LLM interpretation enabled
Analyzer->>LLM: runLLM(prompt JSON)
LLM-->>Analyzer: JSON interpretation
end
Analyzer-->>CLI: aggregated impact report + recommended actions
CLI->>User: formatted report (and optional apply flow)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.claude/skills/skill-rules.json (1)
145-161: Tighten doc-impact keywords to avoid noisy skill activationSeveral keywords are too generic (e.g.,
doc,context,health,analysis,domain,commands,tests) and may activate this skill for unrelated prompts. Prefer more specific phrases tied to impact analysis behavior.Proposed keyword narrowing
"promptTriggers": { "keywords": [ "impact", "freshness", "coverage", "drift", "health score", "context health", - "doc", "doc impact", - "context", - "health", - "analysis", - "domain", - "commands", "doc-impact", - "tests", - "impact.test" + "impact analysis", + "context drift", + "domain coverage", + "impact report" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/skill-rules.json around lines 145 - 161, The "keywords" array in .claude/skills/skill-rules.json is too broad and uses generic tokens (e.g., "doc", "context", "health", "analysis", "domain", "commands", "tests") that cause noisy skill activation; update the keywords list used by the skill rule (the "keywords" array) to replace generic single-word tokens with more specific, impact-analysis-focused phrases (for example "doc-impact", "impact analysis", "context health score", "coverage impact", "drift detection", "impact.test") or remove overly generic entries so only targeted triggers remain, ensuring the skill activates only for relevant prompts.README.md (1)
78-80: Consider varying sentence structure.Three consecutive sentences begin with "Use", which slightly affects readability.
✨ Alternative phrasing
-Use `claude` if you want hooks plus docs. -Use `codex` if you want instruction files plus skills. -Use `all` if your team works across both. +Choose `claude` for hooks plus docs, `codex` for instruction files plus skills, or `all` if your team works across both environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 78 - 80, The three consecutive lines beginning with "Use" (the options "claude", "codex", and "all") reduce readability; rewrite them to vary sentence structure while preserving meaning — for example, make one an imperative ("Choose `claude` for hooks plus docs"), another a descriptive clause ("`codex` provides instruction files plus skills"), and the third a conditional or explanatory sentence ("select `all` if your team works across both"). Update the three lines accordingly so each starts differently but keeps the original option names and descriptions..agents/skills/skill-generation/SKILL.md (1)
53-53: Vague documentation.The "verification line" description is too vague. Consider specifying what information is included in the summary (e.g., "prints summary including target count, domain count, and hook installation status").
✨ Suggested improvement
-- **Verification line:** After config persist, prints a summary of targets configured, domains analyzed, and hook status. +- **Verification line:** After config persist, prints a summary including the number of targets configured, domains analyzed, and hook installation status to confirm successful initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/skill-generation/SKILL.md at line 53, Replace the vague "Verification line" description with a concrete summary of what gets printed: state that after config persistence the verification line prints a summary including target count, domain count, and hook installation status (e.g., "prints summary including target count, domain count, and hook installation status"), and optionally include exact fields shown so readers know what to expect; update the "Verification line" sentence in SKILL.md 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 @.agents/skills/base/SKILL.md:
- Around line 21-22: The SKILL.md command summary for the doc-init workflow
omits non-interactive flags `--mode` and `--strategy`, causing docs to miss CI
usage details; update the `aspens doc init [path]` description to list all
supported non-interactive flags including `--recommended`, `--mode <value>`, and
`--strategy <value>` (and any accepted values or aliases) so users can run
`aspens doc init` in CI/non-interactive contexts without prompts; reference the
`aspens doc init` command entry and ensure the wording matches existing flag
style in the file (e.g., backticked flags) and mention that these bypass
interactive prompts.
In @.agents/skills/skill-generation/SKILL.md:
- Line 51: Update the documentation line so it correctly attributes the monorepo
hook logic to the file where those functions are implemented: change the
reference of createHookSettings() and getGitRoot() to indicate they are defined
in doc-init.js (not git-hook.js) and mention that doc-init.js contains the
monorepo hook handling and git root resolution used when adjusting hook command
paths and installing hooks in subdirectory projects.
- Line 52: Update the documentation line describing the git hook project marker
to show the exact opening and closing marker formats used by the implementation:
replace the current `aspens doc-sync hook (<label>)` text with the full opening
marker `# >>> aspens doc-sync hook (${projectLabel}) (do not edit) >>>` and the
full closing marker `# <<< aspens doc-sync hook (${projectLabel}) <<<` so
readers see the required comment delimiters and the “(do not edit)” instruction
used by the hook installation logic.
---
Nitpick comments:
In @.agents/skills/skill-generation/SKILL.md:
- Line 53: Replace the vague "Verification line" description with a concrete
summary of what gets printed: state that after config persistence the
verification line prints a summary including target count, domain count, and
hook installation status (e.g., "prints summary including target count, domain
count, and hook installation status"), and optionally include exact fields shown
so readers know what to expect; update the "Verification line" sentence in
SKILL.md accordingly.
In @.claude/skills/skill-rules.json:
- Around line 145-161: The "keywords" array in .claude/skills/skill-rules.json
is too broad and uses generic tokens (e.g., "doc", "context", "health",
"analysis", "domain", "commands", "tests") that cause noisy skill activation;
update the keywords list used by the skill rule (the "keywords" array) to
replace generic single-word tokens with more specific, impact-analysis-focused
phrases (for example "doc-impact", "impact analysis", "context health score",
"coverage impact", "drift detection", "impact.test") or remove overly generic
entries so only targeted triggers remain, ensuring the skill activates only for
relevant prompts.
In `@README.md`:
- Around line 78-80: The three consecutive lines beginning with "Use" (the
options "claude", "codex", and "all") reduce readability; rewrite them to vary
sentence structure while preserving meaning — for example, make one an
imperative ("Choose `claude` for hooks plus docs"), another a descriptive clause
("`codex` provides instruction files plus skills"), and the third a conditional
or explanatory sentence ("select `all` if your team works across both"). Update
the three lines accordingly so each starts differently but keeps the original
option names and descriptions.
🪄 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: bedaf86a-f8b2-4f18-a3f8-3c94fd19c7fd
📒 Files selected for processing (15)
.agents/skills/architecture/references/code-map.md.agents/skills/base/SKILL.md.agents/skills/doc-impact/SKILL.md.agents/skills/doc-sync/SKILL.md.agents/skills/skill-generation/SKILL.md.agents/skills/template-library/SKILL.md.claude/skills/base/skill.md.claude/skills/doc-impact/skill.md.claude/skills/doc-sync/skill.md.claude/skills/skill-generation/skill.md.claude/skills/skill-rules.json.claude/skills/template-library/skill.mdAGENTS.mdCLAUDE.mdREADME.md
✅ Files skipped from review due to trivial changes (9)
- CLAUDE.md
- .claude/skills/template-library/skill.md
- .claude/skills/skill-generation/skill.md
- .agents/skills/template-library/SKILL.md
- .agents/skills/doc-impact/SKILL.md
- .claude/skills/base/skill.md
- .agents/skills/architecture/references/code-map.md
- .claude/skills/doc-sync/skill.md
- .claude/skills/doc-impact/skill.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/doc-impact.js (1)
15-19:⚠️ Potential issue | 🟡 MinorWrap
analyzeImpactin try/catch to stop spinner on failure.If
analyzeImpact()throws, the spinner remains spinning. The past review correctly flagged this—ensure the spinner is stopped before propagating the error.Proposed fix
const spinner = p.spinner(); spinner.start('Inspecting repo context coverage...'); - const report = await analyzeImpact(repoPath, options); - const comparison = summarizeValueComparison(report.targets); - spinner.stop(pc.green('Impact report ready')); + let report; + try { + report = await analyzeImpact(repoPath, options); + spinner.stop(pc.green('Impact report ready')); + } catch (err) { + spinner.stop(pc.red('Impact analysis failed')); + throw new Error(`Failed to analyze impact: ${err.message}. Try "aspens scan ${repoPath}" first.`); + } + const comparison = summarizeValueComparison(report.targets);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-impact.js` around lines 15 - 19, The spinner may remain spinning if analyzeImpact(repoPath, options) throws; wrap the call to analyzeImpact (and subsequent summarizeValueComparison(report.targets)) in a try/catch so you always call spinner.stop(...) before rethrowing the error: call spinner.start(...) before the try, call spinner.stop(pc.green('Impact report ready')) on success, and in the catch call spinner.stop(pc.red('Impact report failed')) (or similar) then rethrow the caught error to preserve behavior.
🧹 Nitpick comments (3)
tests/skill-writer.test.js (1)
117-157: Add a regression case for “same marker, different command” entries.The dedupe test currently checks only length. Please add a case where two aspens-managed entries share marker type but have different command paths (or different matcher) and verify both are preserved.
Suggested test addition
describe('mergeSettings', () => { + it('keeps distinct aspens entries when command paths differ', () => { + const existing = { + hooks: { + UserPromptSubmit: [ + { hooks: [{ type: 'command', command: '"$CLAUDE_PROJECT_DIR/frontend/.claude/hooks/graph-context-prompt.sh"' }] }, + { hooks: [{ type: 'command', command: '"$CLAUDE_PROJECT_DIR/backend/.claude/hooks/graph-context-prompt.sh"' }] }, + ], + }, + }; + const merged = mergeSettings(existing, { hooks: { UserPromptSubmit: [] } }); + const commands = merged.hooks.UserPromptSubmit.flatMap(e => e.hooks.map(h => h.command)); + expect(commands).toContain('"$CLAUDE_PROJECT_DIR/frontend/.claude/hooks/graph-context-prompt.sh"'); + expect(commands).toContain('"$CLAUDE_PROJECT_DIR/backend/.claude/hooks/graph-context-prompt.sh"'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/skill-writer.test.js` around lines 117 - 157, Add a regression test to ensure mergeSettings only deduplicates truly identical aspens-managed hook entries and preserves entries that share the same marker/type but differ in command or matcher; extend the test in tests/skill-writer.test.js that currently asserts merged.hooks.UserPromptSubmit length to include a new case where two entries both have the same marker/type (e.g., both created by aspens) but different command paths or matcher values, call mergeSettings(existing, template) and assert that both distinct entries remain (e.g., expect merged.hooks.UserPromptSubmit to contain both unique command strings or have length 2).src/lib/impact.js (2)
34-38: Consider logging graph-build failures in verbose mode.Silently swallowing errors makes debugging difficult. When
options.verboseis truthy, consider loggingerr.messageso users can understand why hub coverage data is unavailable.Proposed fix
if (options.graph !== false) { try { graph = await buildRepoGraph(repoPath, scan.languages); - } catch { + } catch (err) { + if (options.verbose) { + console.error(`[verbose] Graph build failed: ${err.message}`); + } graph = null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/impact.js` around lines 34 - 38, The catch block that swallows errors when calling buildRepoGraph(repoPath, scan.languages) should log the error message when options.verbose is truthy; update the catch to accept the thrown error (e.g., catch (err)) and, if options.verbose, emit a clear log containing err.message (and optionally err.stack) so callers know why graph is null while keeping behavior the same when verbose is falsy; locate this logic around the buildRepoGraph call and the graph variable to implement the change.
226-254: Parameter naming is misleading.The parameter is named
targetbut receives a composite object containingstatus,drift,domainCoverage, etc. as direct properties. Consider renaming toinputorcontextfor clarity.Proposed fix
-export function recommendActions(target) { +export function recommendActions(input) { const actions = []; - if (target.status.instructions === 'missing' || target.status.domains === 'missing') { + if (input.status.instructions === 'missing' || input.status.domains === 'missing') { actions.push('aspens doc init --recommended'); - } else if (target.status.instructions === 'stale' || target.drift.changedCount > 0) { + } else if (input.status.instructions === 'stale' || input.drift.changedCount > 0) { actions.push('aspens doc sync'); } - if (target.status.hooks === 'missing' || target.status.hooks === 'broken') { + if (input.status.hooks === 'missing' || input.status.hooks === 'broken') { actions.push('aspens doc init --hooks-only'); } // ... continue renaming throughout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/impact.js` around lines 226 - 254, The parameter name target in the recommendActions function is misleading because the function expects a composite object with properties like status, drift, domainCoverage, hubCoverage; rename the parameter to a clearer name such as context or input in the function signature (recommendActions) and update every reference inside the function (status, drift, domainCoverage, hubCoverage, etc.) to use the new parameter name; ensure exported symbol recommendActions still matches external callers by updating any call sites and related tests/docs to the new parameter name so nothing breaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/skill-writer.js`:
- Around line 359-362: The current dedupe uses a coarse key built from
aspensMarkers.sort().join('|') and entry.matcher (variable key), which can
incorrectly collapse distinct entries that differ by command or other hook
fields; change the key generation used with the seen Set (where seen.add and
seen.has are used) to include the differentiating fields (e.g., entry.command
and any non-aspens hook identifiers or serialize the full entry) so only truly
identical hook entries are deduped before result.push(entry).
---
Duplicate comments:
In `@src/commands/doc-impact.js`:
- Around line 15-19: The spinner may remain spinning if analyzeImpact(repoPath,
options) throws; wrap the call to analyzeImpact (and subsequent
summarizeValueComparison(report.targets)) in a try/catch so you always call
spinner.stop(...) before rethrowing the error: call spinner.start(...) before
the try, call spinner.stop(pc.green('Impact report ready')) on success, and in
the catch call spinner.stop(pc.red('Impact report failed')) (or similar) then
rethrow the caught error to preserve behavior.
---
Nitpick comments:
In `@src/lib/impact.js`:
- Around line 34-38: The catch block that swallows errors when calling
buildRepoGraph(repoPath, scan.languages) should log the error message when
options.verbose is truthy; update the catch to accept the thrown error (e.g.,
catch (err)) and, if options.verbose, emit a clear log containing err.message
(and optionally err.stack) so callers know why graph is null while keeping
behavior the same when verbose is falsy; locate this logic around the
buildRepoGraph call and the graph variable to implement the change.
- Around line 226-254: The parameter name target in the recommendActions
function is misleading because the function expects a composite object with
properties like status, drift, domainCoverage, hubCoverage; rename the parameter
to a clearer name such as context or input in the function signature
(recommendActions) and update every reference inside the function (status,
drift, domainCoverage, hubCoverage, etc.) to use the new parameter name; ensure
exported symbol recommendActions still matches external callers by updating any
call sites and related tests/docs to the new parameter name so nothing breaks.
In `@tests/skill-writer.test.js`:
- Around line 117-157: Add a regression test to ensure mergeSettings only
deduplicates truly identical aspens-managed hook entries and preserves entries
that share the same marker/type but differ in command or matcher; extend the
test in tests/skill-writer.test.js that currently asserts
merged.hooks.UserPromptSubmit length to include a new case where two entries
both have the same marker/type (e.g., both created by aspens) but different
command paths or matcher values, call mergeSettings(existing, template) and
assert that both distinct entries remain (e.g., expect
merged.hooks.UserPromptSubmit to contain both unique command strings or have
length 2).
🪄 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: 6a6538ce-a6e4-4331-946f-9081dde73324
📒 Files selected for processing (10)
bin/cli.jssrc/commands/doc-impact.jssrc/commands/doc-init.jssrc/lib/impact.jssrc/lib/skill-writer.jssrc/prompts/impact-analyze.mdsrc/templates/settings/settings.jsontests/hook-runtime.test.jstests/impact.test.jstests/skill-writer.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/cli.js
- tests/impact.test.js
- src/commands/doc-init.js
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/commands/doc-impact.js (1)
15-19:⚠️ Potential issue | 🟠 MajorStop the spinner and rethrow a command-level error here.
If
analyzeImpact()rejects, this leaves the spinner running and bubbles a raw exception out of the handler. Wrap this block intry/catch, stop/fail the spinner on error, and rethrow aCliErrorwith a remediation hint.As per coding guidelines,
src/commands/*.js: Throw descriptive errors with remediation hints; callprocess.exit(1)on unrecoverable failures, andsrc/commands/**/*.js: PreferCliErrorfrom command handlers; top-level handling lives inbin/cli.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-impact.js` around lines 15 - 19, The spinner and report logic around analyzeImpact(repoPath, options) should be wrapped in a try/catch so the spinner is stopped/failed on errors; if analyzeImpact rejects, call spinner.fail(...) (or spinner.stop with a failure message) and rethrow a CliError with a short remediation hint and include the original error as the cause. Update the block that uses spinner, analyzeImpact, and summarizeValueComparison to catch any exception, stop/fail spinner, and throw new CliError('Inspecting repo context coverage failed: <brief hint>', { cause: err }) so top-level bin/cli.js can handle exit semantics.
🧹 Nitpick comments (2)
.claude/skills/claude-runner/skill.md (1)
38-38: Documentation accurately reflects code behavior.The description correctly documents:
- All three markers in
ASPENS_HOOK_MARKERS(confirmed by snippet from src/lib/skill-writer.js:314-330)- Post-merge deduplication via
dedupeAspensHookEntries()(confirmed by snippet from src/lib/skill-writer.js:305-310)- Preservation of non-aspens hooks (confirmed by snippet from src/lib/skill-writer.js:335-362)
Optional: Consider clarifying "matcher"
The phrase "keyed by marker + matcher combo" could be slightly more specific. The actual key construction is
${aspensMarkers.sort().join('|')}::${entry.matcher || ''}, wherematcherrefers to theentry.matcherfield from the hook entry. However, for high-level skill documentation, the current phrasing may be appropriately concise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/claude-runner/skill.md at line 38, Update the documentation sentence about deduplication to precisely describe the key construction used by dedupeAspensHookEntries(): state that entries are keyed by the sorted aspens markers joined with '|' plus '::' and the entry.matcher (i.e. key = `${aspensMarkers.sort().join('|')}::${entry.matcher || ''}`), and clarify that "matcher" refers to the entry.matcher field on each hook; reference mergeSettings(), dedupeAspensHookEntries(), and ASPENS_HOOK_MARKERS to locate the relevant logic.src/lib/target-transform.js (1)
253-263: Consider extracting shared hub-rendering logic.
buildHubFilesSectionduplicates the hub rendering fromgenerateCondensedCodeMap(lines 273-281). Both iteratehubs.slice(0, 5)and format identically.♻️ Optional: Extract shared helper
+function formatHubLines(hubs, limit = 5) { + return hubs.slice(0, limit).map(hub => '- `' + hub.path + '` - ' + hub.fanIn + ' dependents'); +} + function buildHubFilesSection(serializedGraph) { if (!serializedGraph?.hubs?.length) return null; - const lines = ['## Key Files', '', '**Hub files (most depended-on):**']; - for (const hub of serializedGraph.hubs.slice(0, 5)) { - lines.push('- `' + hub.path + '` - ' + hub.fanIn + ' dependents'); - } - lines.push(''); - - return lines.join('\n'); + return ['## Key Files', '', '**Hub files (most depended-on):**', ...formatHubLines(serializedGraph.hubs), ''].join('\n'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/target-transform.js` around lines 253 - 263, buildHubFilesSection duplicates hub rendering logic present in generateCondensedCodeMap (both iterate serializedGraph.hubs.slice(0, 5) and format the same lines); extract that shared logic into a single helper (e.g., renderTopHubs or formatTopHubs) that accepts a hubs array and returns the formatted lines or string, then replace the loop/formatting in both buildHubFilesSection and generateCondensedCodeMap to call this helper so formatting is centralized and the duplicate slice/loop is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 23-26: Replace the user-specific absolute path
`/Users/MV/aspenkit/dev/release.md` in AGENTS.md with a relative or
repository-root-relative path (for example `./dev/release.md` or
`dev/release.md`) or explicitly mark it as a local developer reference; update
the Release bullet under the "Release" heading to point to the relative path or
add a brief note like "local dev reference" so other contributors can access the
document without a user-specific filesystem path.
In `@src/commands/doc-impact.js`:
- Around line 146-159: The current code always prompts and applies changes
because applyPlan (from buildApplyPlan) is executed unconditionally; change the
flow to require an explicit options.apply opt-in: only build/apply the plan and
call p.confirm/applyRecommendedAction when options.apply is true, and preserve
existing confirmation logic (p.confirm, buildApplyConfirmationMessage,
applyRecommendedAction, repoPath, options, item.target) inside that guarded
block; also add a boolean --apply option to the command registration in
bin/cli.js so the flag is exposed and passed into options when the command runs.
In `@src/commands/doc-init.js`:
- Around line 710-714: The summary line incorrectly reports "hooks updated where
supported" based only on hasHookTarget and options.hooks; change the logic to
inspect the actual result from installHooks() (e.g., a boolean or object like
installResult/hooksApplied returned by installHooks()) and only append 'hooks
updated where supported' when that result confirms hooks were applied; otherwise
append 'no hook changes' or a failure/skipped message based on the
installHooks() return value or error. Locate the message construction around
hasHookTarget/options.hooks and replace its hook-status ternary with a value
derived from the installHooks() outcome (handle skipped, partial, and error
cases) so the console output reflects the real install result.
- Around line 1499-1501: Root-key-files normalization
(ensureRootKeyFilesSection) is only applied in the chunked CLAUDE.md path — move
it into the shared post-processing step used for any generated root instructions
file so all modes get the enforced "## Key Files" section; after building the
prompt/content (e.g. where claudeMdPrompt is assembled and similarly for the
all-at-once output around the code that currently handles lines ~1523-1525),
call ensureRootKeyFilesSection(...) on the final root instructions string and
use the normalized result for writing/saving the file; update references to
buildRootInstructionsGraphContext and ensureRootKeyFilesSection to locate and
replace the final content rather than keeping ensureRootKeyFilesSection confined
to only the chunked path.
- Around line 289-290: The current branch sets skipDiscovery = true when
recommended is true, causing effectiveDomains to prefer reusedDomains over
scan.domains and drop newly discovered modules; remove or avoid setting
skipDiscovery in the recommended branch and instead treat scan.domains as the
base set: use scan.domains as the initial effectiveDomains and merge
reusedDomains metadata into those entries (or only skip the LLM discovery step
while still using scan.domains), ensuring the effectiveDomains computation
prefers entries from scan.domains augmented by reusedDomains rather than
replacing scan.domains entirely; update the logic around recommended,
skipDiscovery, effectiveDomains, reusedDomains, and scan.domains accordingly.
In `@src/lib/impact.js`:
- Around line 550-552: The repo walk() function currently stops at fixed depth 5
which undercounts changes in deeper monorepos (affecting newestSourceMtime,
changedCount, affectedDomains); update walk(dir, depth) to use a larger or
configurable depth limit (e.g., define MAX_TRAVERSAL_DEPTH and set to 10 or read
from env/config) and apply the same change where depth checks occur around the
walk implementation and the related calculations that update newestSourceMtime,
changedCount, and affectedDomains so deeper paths like apps/web/src/... are
included.
- Around line 243-250: The condition that pushes the base-only rewrite action
allows 'broken' hooks because it checks target.status.hooks !== 'missing';
change this to require healthy hooks (e.g., target.status.hooks === 'healthy')
so the actions.push('aspens doc init --mode base-only --strategy rewrite') in
src/lib/impact.js only runs when target.status.hooks is explicitly 'healthy'.
- Around line 273-274: The affectedTargets filter currently counts targets with
drift.changedCount>0 or non-'healthy' domains/instructions but ignores hook-only
failures; update the filter for affectedTargets (which iterates targets and
checks target.drift.changedCount, target.status.domains,
target.status.instructions) to also include targets whose hook status indicates
failure by adding a condition for target.status.hooks === 'missing' ||
target.status.hooks === 'broken' (or otherwise not 'healthy'), making the
predicate: include target if changedCount>0 OR domains !== 'healthy' OR
instructions !== 'healthy' OR hooks === 'missing' OR hooks === 'broken' (handle
undefined safely).
---
Duplicate comments:
In `@src/commands/doc-impact.js`:
- Around line 15-19: The spinner and report logic around analyzeImpact(repoPath,
options) should be wrapped in a try/catch so the spinner is stopped/failed on
errors; if analyzeImpact rejects, call spinner.fail(...) (or spinner.stop with a
failure message) and rethrow a CliError with a short remediation hint and
include the original error as the cause. Update the block that uses spinner,
analyzeImpact, and summarizeValueComparison to catch any exception, stop/fail
spinner, and throw new CliError('Inspecting repo context coverage failed: <brief
hint>', { cause: err }) so top-level bin/cli.js can handle exit semantics.
---
Nitpick comments:
In @.claude/skills/claude-runner/skill.md:
- Line 38: Update the documentation sentence about deduplication to precisely
describe the key construction used by dedupeAspensHookEntries(): state that
entries are keyed by the sorted aspens markers joined with '|' plus '::' and the
entry.matcher (i.e. key = `${aspensMarkers.sort().join('|')}::${entry.matcher ||
''}`), and clarify that "matcher" refers to the entry.matcher field on each
hook; reference mergeSettings(), dedupeAspensHookEntries(), and
ASPENS_HOOK_MARKERS to locate the relevant logic.
In `@src/lib/target-transform.js`:
- Around line 253-263: buildHubFilesSection duplicates hub rendering logic
present in generateCondensedCodeMap (both iterate serializedGraph.hubs.slice(0,
5) and format the same lines); extract that shared logic into a single helper
(e.g., renderTopHubs or formatTopHubs) that accepts a hubs array and returns the
formatted lines or string, then replace the loop/formatting in both
buildHubFilesSection and generateCondensedCodeMap to call this helper so
formatting is centralized and the duplicate slice/loop is removed.
🪄 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: eb3ec2fe-f2a6-45e7-944c-f90745ed610b
📒 Files selected for processing (23)
.agents/skills/architecture/references/code-map.md.agents/skills/base/SKILL.md.agents/skills/claude-runner/SKILL.md.agents/skills/doc-impact/SKILL.md.agents/skills/template-library/SKILL.md.claude/skills/base/skill.md.claude/skills/claude-runner/skill.md.claude/skills/doc-impact/skill.md.claude/skills/skill-rules.json.claude/skills/template-library/skill.mdAGENTS.mdCLAUDE.mdbin/cli.jssrc/commands/doc-impact.jssrc/commands/doc-init.jssrc/lib/impact.jssrc/lib/target-transform.jssrc/lib/target.jssrc/prompts/doc-init-claudemd.mdtests/doc-impact.test.jstests/impact.test.jstests/target-transform.test.jstests/target.test.js
✅ Files skipped from review due to trivial changes (5)
- src/prompts/doc-init-claudemd.md
- tests/target.test.js
- .agents/skills/architecture/references/code-map.md
- .claude/skills/template-library/skill.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .agents/skills/base/SKILL.md
- bin/cli.js
- .claude/skills/skill-rules.json
- .agents/skills/doc-impact/SKILL.md
- .claude/skills/doc-impact/skill.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/codex-support/SKILL.md:
- Line 33: Update the description of ensureRootKeyFilesSection(content,
graphSerialized) to clarify when it runs: state that it is invoked on the
canonical CLAUDE.md during doc-init chunked mode and applied before
target-specific transforms (not after AGENTS.md generation), and mention it
guarantees a ## Key Files section by replacing an incomplete section or
inserting before ## Behavior / **Last Updated** so readers understand the exact
pipeline stage where ensureRootKeyFilesSection is executed.
In `@AGENTS.md`:
- Around line 52-53: Update the churn counts in AGENTS.md by changing the entry
for `src/commands/doc-init.js` from 30 to 27 and the entry for
`src/commands/doc-sync.js` from 20 to 19 so the documented change counts match
the recorded hotspot data; locate the two lines containing the exact strings
`src/commands/doc-init.js` and `src/commands/doc-sync.js` and update their
numeric values accordingly.
🪄 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: 5b012bde-fea4-4c59-a776-d5c8c747d98e
📒 Files selected for processing (10)
.agents/skills/architecture/references/code-map.md.agents/skills/codex-support/SKILL.md.agents/skills/doc-impact/SKILL.md.agents/skills/skill-generation/SKILL.md.claude/skills/codex-support/skill.md.claude/skills/doc-impact/skill.md.claude/skills/skill-generation/skill.md.claude/skills/skill-rules.jsonAGENTS.mdCLAUDE.md
✅ Files skipped from review due to trivial changes (5)
- CLAUDE.md
- .agents/skills/architecture/references/code-map.md
- .claude/skills/codex-support/skill.md
- .claude/skills/doc-impact/skill.md
- .agents/skills/doc-impact/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/skills/skill-rules.json
- .claude/skills/skill-generation/skill.md
- .agents/skills/skill-generation/SKILL.md
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.claude/hooks/skill-activation-prompt.mjs (1)
218-228:⚠️ Potential issue | 🟡 MinorStale JSDoc parameter.
The
@param {string} projectDiron line 225 references a parameter that was removed from the function signature.📝 Proposed fix
/** * Format the output for Claude's context injection. * High-priority skills get full content in <!-- skill --> blocks. * Medium/low skills are listed as available. * * `@param` {Array<{ name: string, matchType: string, config: Object, content?: string }>} matched * `@param` {string} currentRepo - Current repository name - * `@param` {string} projectDir - Absolute path to the project root * `@returns` {string} Formatted output for stdout */ export function formatOutput(matched, currentRepo) {🤖 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 218 - 228, JSDoc for formatOutput is stale: the `@param` {string} projectDir entry no longer matches the function signature; remove the obsolete `@param` line (or update the signature if you intentionally need projectDir) so the JSDoc accurately reflects the actual parameters of the export function formatOutput(matched, currentRepo), and ensure other `@param` tags still match the parameter names and types.src/commands/doc-init.js (2)
241-266:⚠️ Potential issue | 🟠 MajorHonor the persisted backend in
--recommended.When both CLIs are available, this still falls back to the default backend instead of the backend already stored in
.aspens.json. That can silently switch generators and then overwrite the saved backend with the new default.💡 Suggested change
- let recommendedTargetIds = null; - if (recommended && !options.target) { - const { config } = loadConfig(repoPath, { persist: false }); - if (config?.targets?.length) { - recommendedTargetIds = config.targets; - } - } + let recommendedTargetIds = null; + const recommendedBackendId = recommended ? existingConfig?.backend || null : null; + if (recommended && !options.target && existingConfig?.targets?.length) { + recommendedTargetIds = existingConfig.targets; + } if (options.backend) { backendResult = resolveBackend({ backendFlag: options.backend, available }); + } else if (recommendedBackendId) { + backendResult = resolveBackend({ backendFlag: recommendedBackendId, available }); } else if (recommended && recommendedTargetIds?.length === 1) { backendResult = resolveBackend({ targetId: recommendedTargetIds[0], available });Based on learnings, "Keep target/backend semantics straight: target is output format/location; backend is the generating CLI. Persist config in
.aspens.json."Also applies to: 303-309
🤖 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 241 - 266, The current flow ignores a persisted backend in the loaded config when `recommended` is true and both CLIs are available, causing a silent backend switch; change the selection logic after `const { config } = loadConfig(repoPath, { persist: false })` so that if `recommended` is true and `config.backend` (or equivalent persisted backend field) exists you call `resolveBackend({ backendFlag: config.backend, available })` and set `backendResult` to that, instead of prompting or falling through to the default branch; update the same logic paths that use `recommendedTargetIds` (including the other block noted around 303-309) to honor the persisted `config.backend` first.
633-655:⚠️ Potential issue | 🟠 MajorDon't make chunked generation depend on an interactive incremental-write prompt.
This reintroduces a prompt into flagged/CI flows, and answering “no” aborts instead of falling back to the existing buffered write path.
💡 Suggested change
- const shouldWriteIncrementally = mode === 'chunked' && !options.dryRun; - const incrementalWriteState = shouldWriteIncrementally + const nonInteractiveFlow = !!options.mode || !!options.strategy || recommended; + let shouldWriteIncrementally = + mode === 'chunked' && + !options.dryRun && + !nonInteractiveFlow; + let incrementalWriteState = shouldWriteIncrementally ? { contentsByPath: new Map(), resultsByPath: new Map() } : null; ... - if (shouldWriteIncrementally) { + if (mode === 'chunked' && !options.dryRun && !nonInteractiveFlow) { console.log(); const allowIncrementalWrite = await p.confirm({ message: 'Allow aspens to write generated files incrementally during chunked generation? This includes skills, repo docs, and supported hook/config updates.', initialValue: true, }); - if (p.isCancel(allowIncrementalWrite) || !allowIncrementalWrite) { + if (p.isCancel(allowIncrementalWrite)) { p.cancel('Aborted'); return; } + shouldWriteIncrementally = !!allowIncrementalWrite; + incrementalWriteState = shouldWriteIncrementally + ? { contentsByPath: new Map(), resultsByPath: new Map() } + : null; }As per coding guidelines,
src/commands/doc-init.js: Support non-interactive/CI usage by accepting--modeand--strategyflags in doc-init command to bypass interactive prompts.
♻️ Duplicate comments (8)
src/commands/doc-impact.js (2)
15-19:⚠️ Potential issue | 🟠 MajorStop the spinner and rethrow a command-level error here.
If
analyzeImpact()throws, the spinner never stops and the user gets raw upstream failure context instead of a repo-specific remediation hint. Wrap this block intry/catch, stop the spinner on both paths, and rethrow aCliError.As per coding guidelines,
src/commands/*.js: Throw descriptive errors with remediation hints; and{bin/cli.js,src/commands/**/*.{js,ts}}: PreferCliErrorfrom command handlers; top-level error handling lives inbin/cli.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-impact.js` around lines 15 - 19, Wrap the analyzeImpact/summarizeValueComparison block in try/catch (or try/finally) to ensure spinner (variable spinner) is stopped on both success and error paths; on error, stop the spinner and throw a CliError (imported/used where other commands do) with a descriptive message that includes repo-specific remediation hint (mention repoPath/options context) instead of letting the raw upstream error bubble up; rethrow or include the original error as cause/detail so bin/cli.js can render it.
164-177:⚠️ Potential issue | 🟠 MajorKeep
doc impactread-only unless--applyis explicitly set.This path prompts for repo mutations on every run, so a reporting command becomes interactive and mutating by default.
💡 Suggested change
- const applyPlan = buildApplyPlan(report.targets); + const applyPlan = options.apply ? buildApplyPlan(report.targets) : []; - if (applyPlan.length > 0) { + if (options.apply && applyPlan.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/doc-impact.js` around lines 164 - 177, The command currently prompts to mutate the repo whenever buildApplyPlan(report.targets) returns actions; change it to be read-only unless the user passes an explicit --apply flag: before calling p.confirm or running applyRecommendedAction, check a single explicit flag (e.g., options.apply or args.apply) and only then call p.confirm({ message: buildApplyConfirmationMessage(), ... }) and loop over applyPlan to call applyRecommendedAction(repoPath, item.action, options, item.target); if the flag is not set, skip p.confirm and the apply loop and instead log the planned actions summary (keeping buildApplyPlan, buildApplyConfirmationMessage, p.confirm and applyRecommendedAction usage intact when --apply is provided).src/commands/doc-init.js (3)
374-376:⚠️ Potential issue | 🟠 MajorDon't let
--recommendedreplace scanner domains with reused ones.Setting
skipDiscovery = truehere makeseffectiveDomainspreferreusedDomainsoverscan.domains, so new modules that are not already represented in existing skills disappear from generation. Keepscan.domainsas the base set and only merge reused metadata into it.Also applies to: 492-496
🤖 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 374 - 376, The current branch sets skipDiscovery = true when the recommended flag is set, causing effectiveDomains to prefer reusedDomains and completely replace scan.domains; instead remove or avoid setting skipDiscovery in the recommended branch so scan.domains remains the base, and change the merge logic where effectiveDomains is computed to merge reusedDomains metadata into scan.domains (augmenting existing domain entries) rather than replacing them; apply the same fix to the second occurrence of this pattern around the other recommended-check (the block at 492-496) so reused metadata is merged into scan.domains rather than overriding it.
1676-1703:⚠️ Potential issue | 🟠 MajorApply the root-key-files normalization in all generation modes.
This
ensureRootKeyFilesSection()call is still only in the chunkedCLAUDE.mdpath. SuccessfulgenerateAllAtOnce()runs bypass the same post-process, so hub coverage stays mode-dependent.🤖 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 1676 - 1703, The post-processing call to ensureRootKeyFilesSection is only applied to chunked CLAUDE.md outputs (inside the runLLM/retry flow) and must be applied to all generation modes; update the flow that handles completed generated files (the variable files after parseLLMOutput and also the return value of generateAllAtOnce) to run ensureRootKeyFilesSection(file.content, repoGraph) for any file.path === 'CLAUDE.md' before calling validateGeneratedChunk or writing files out (i.e., centralize the normalization so both the chunked path and generateAllAtOnce path invoke ensureRootKeyFilesSection on CLAUDE.md).
821-825:⚠️ Potential issue | 🟡 MinorReport hook status from the install result, not from the flags.
This still prints
hooks updated where supportedwhenever Claude is selected and hooks are enabled, even ifinstallHooks()skipped or failed earlier.Based on learnings, "Verify before claiming — Never state that something is configured, running, scheduled, or complete without confirming it 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 821 - 825, The status message incorrectly reports hook updates based on flags (hasHookTarget and options.hooks) rather than the actual outcome; update the console.log construction to use the real install result from installHooks() (e.g., an installResult or hooksResult returned by installHooks/performInstall) and derive the hook text from that object (like installResult.hooksUpdated, installResult.hooksSkipped, or installResult.hooksFailed) instead of hasHookTarget/options.hooks, while keeping the other parts (targets.map(...), effectiveDomains.length) unchanged so the message reflects the true result of installHooks().src/lib/impact.js (3)
275-283:⚠️ Potential issue | 🟡 MinorCount hook-only failures in
affectedTargets.A target with healthy docs/domains but
hooks === 'missing'orhooks === 'broken'contributes torepoStatusandmissing, but not this summary count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/impact.js` around lines 275 - 283, The affectedTargets count currently filters targets by drift.changedCount and target.status.domains/instructions, but omits targets whose hooks are 'missing' or 'broken'; update the filter used to compute affectedTargets (the targets.filter(...) expression) to also treat a target as affected when target.status.hooks !== 'healthy' (i.e., include hooks status alongside domains and instructions) so hook-only failures are counted.
752-796:⚠️ Potential issue | 🟠 MajorThe fixed depth cap still undercounts drift in deep monorepos.
Stopping at depth 5 misses common layouts like
apps/web/src/..., sonewestSourceMtime,changedCount, andaffectedDomainscan all be understated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/impact.js` around lines 752 - 796, The depth cap in function walk (used to populate files, newestSourceMtime) stops recursion at depth > 5 and misses deeper monorepo paths (e.g., apps/web/src), undercounting newestSourceMtime, changedCount and affectedDomains; remove or increase the hardcoded cap and instead use a configurable MAX_DEPTH or no cap, update references in walk and its call walk(repoPath, 0) accordingly, and ensure heavy directories are still pruned by the existing skip-list (node_modules, dist, etc.) rather than an arbitrary shallow depth limit so deep source files are discovered and newestSourceMtime is accurate.
251-258:⚠️ Potential issue | 🟠 MajorRequire healthy hooks before recommending a base-only rewrite.
target.status.hooks !== 'missing'still passes forbroken, so the report can suggest rewriting root instructions even though hook setup is still unhealthy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/impact.js` around lines 251 - 258, The condition that gates recommending a base-only rewrite allows broken hooks because it checks target.status.hooks !== 'missing'; change the check to require hooks are healthy (e.g., target.status.hooks === 'healthy') before pushing the action in the block containing actions.push('aspens doc init --mode base-only --strategy rewrite') so the rewrite is only suggested when hooks are fully healthy.
🧹 Nitpick comments (6)
src/templates/commands/resume-handoff-latest.md (1)
9-11: Small readability tweak: reduce repeated “If” sentence starts.The step flow is clear, but rewording one line will read cleaner and avoid repetitive openings.
As per coding guidelines, this template should optimize clarity for user-facing instructions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/templates/commands/resume-handoff-latest.md` around lines 9 - 11, Reword the second bullet in src/templates/commands/resume-handoff-latest.md to avoid repeating "If" at the start of each step: keep step 1 as-is, change step 2 to something like "When index.json is missing, stale, or points to a deleted file, list `.aspens/sessions/*-handoff.md` and pick the newest by filename" and keep step 3 unchanged; this preserves logic but improves readability by varying sentence openings.src/lib/git-hook.js (1)
83-93: Consider wrapping file read in try/catch.Per coding guidelines,
readFileSyncat line 84 could throw if the file is deleted betweenexistsSynccheck and read (TOCTOU). Same applies to line 122.🛡️ Suggested defensive read
if (existsSync(hookPath)) { - const existing = readFileSync(hookPath, 'utf8'); + let existing; + try { + existing = readFileSync(hookPath, 'utf8'); + } catch { + // File removed between check and read — treat as new + writeFileSync(hookPath, '#!/bin/sh\n' + hookBlock, 'utf8'); + chmodSync(hookPath, 0o755); + console.log(pc.green(`\n Installed post-commit hook for ${projectLabel}.\n`)); + return; + } if (existing.includes(`# >>> aspens doc-sync hook (${projectLabel})`)) {As per coding guidelines: "Use try/catch for file reads and return null on failure rather than throwing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/git-hook.js` around lines 83 - 93, Wrap the synchronous file reads around hookPath (the readFileSync calls used to load the existing hook and the other read at line ~122) in try/catch blocks so IO races don't throw; if readFileSync fails, handle the error by returning null (or exiting the current install/upgrade flow) instead of letting the exception propagate. Specifically, around the code that calls readFileSync(hookPath, 'utf8') used with existing, and the later readFileSync usage, catch any error, optionally log a debug message, and return null so downstream logic (hasUnlabeledAspensBlock, buildUnlabeledMarkerRegex, writeFileSync) doesn’t run on a failed read..claude/settings.json (1)
6-19: Inconsistent command quoting.Lines 8 and 27 (existing hooks) omit quotes around the path:
"command": "$CLAUDE_PROJECT_DIR/.claude/hooks/skill-activation-prompt.sh"Lines 16, 37, 45 (new hooks) include escaped quotes:
"command": "\"$CLAUDE_PROJECT_DIR/.claude/hooks/save-tokens-prompt-guard.sh\""The
buildSaveTokensSettings()function (context snippet 1) generates quoted commands. Either both styles work, or there's a subtle difference in how Claude Code expands these. Consider aligning for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/settings.json around lines 6 - 19, The hook "command" values are inconsistent: some are plain strings and others contain escaped quotes produced by buildSaveTokensSettings(); fix by making the styles consistent — preferably remove the extra escaped quotes so the JSON command values are plain paths (e.g., $CLAUDE_PROJECT_DIR/.claude/hooks/...), and update buildSaveTokensSettings() to stop wrapping the command in literal quotes when it generates the hook; ensure any other hook generators follow the same unquoted string format so all "command" fields match..claude/hooks/graph-context-prompt.mjs (1)
374-384: Redundant dynamic import offs.
writeFileSyncis already available via the static import at line 16 (readFileSync,existsSync). Just addwriteFileSyncto the existing import instead of dynamically importingfsagain.Proposed fix
-import { readFileSync, existsSync } from 'fs'; +import { readFileSync, existsSync, writeFileSync } from 'fs';Then at line 376-377:
- const { writeFileSync: wfs } = await import('fs'); - wfs('/tmp/aspens-debug-graph-context.json', JSON.stringify({ + writeFileSync('/tmp/aspens-debug-graph-context.json', JSON.stringify({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/graph-context-prompt.mjs around lines 374 - 384, The dynamic import of fs inside the ASPENS_DEBUG block is redundant; add writeFileSync to the static import that already provides readFileSync and existsSync (so writeFileSync is available at module scope) and remove the await import('fs') and destructuring inside the try block; then call the module-scope writeFileSync to write the debug file when process.env.ASPENS_DEBUG === '1' and debugInfo is present (preserve writing timestamp, projectDir, prompt, and ...debugInfo and the try/catch behavior)..claude/agents/plan.md (1)
59-67: Add fence languages to keep markdownlint clean.Both examples use bare fences, so MD040 will keep warning here.
markdownfor the triage example andyamlortextfor the Agent snippet would fix it.Suggested patch
-``` +```markdown | Dimension | Rating | Evidence | |---|---|---| | Blast radius | Local | `runner.js` imported by 3 files, all in src/lib/ | | Risk profile | Mutative-safe | Tests exist in tests/runner.test.js | | Complexity | Tactical | Clear goal, one design choice | Verdict: Small — all dimensions at or below small threshold. -``` +``` -``` +```yaml Use the Agent tool: prompt: "Review this plan for {task-name}: {paste plan.md content}. Return: verdict, critical issues only." model: sonnet -``` +```Also applies to: 114-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/plan.md around lines 59 - 67, The markdown fences around the plan table and the "Use the Agent tool:" snippet are bare and trigger MD040; update the opening/closing fence for the table block to use ```markdown and update the Agent snippet fences to use a language label such as ```yaml or ```text (apply the same change to the other occurrence noted around lines 114-118), making sure the fenced block that contains the table and the fenced block that starts with "Use the Agent tool:" both include the language token so markdownlint no longer flags them..claude/hooks/save-tokens.mjs (1)
97-146: Keep this hook in sync with the shipped template.
src/commands/save-tokens.jsinstallssrc/templates/hooks/save-tokens.mjs, andtests/save-tokens-hook-lib.test.jsonly exercises that copy. This checked-in hook now emits a different handoff format, so dogfooding in this repo drifts from the shipped and tested behavior. Please source both from one implementation or keep them equivalent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/save-tokens.mjs around lines 97 - 146, The checked-in hook .claude/hooks/save-tokens.mjs diverges from the shipped template and tests; make the implementations identical by either (A) replacing the body of saveHandoff in .claude/hooks/save-tokens.mjs with the same implementation used in src/templates/hooks/save-tokens.mjs, or (B) refactoring to a single shared implementation that both files import (e.g., move the logic that calls sessionTokenSnapshot, extractPrompt, readTranscriptExcerpt, writeLatestIndex and pruneOldHandoffs into a shared module and have saveHandoff stub call it), ensuring saveHandoff, writeLatestIndex and pruneOldHandoffs behavior/format exactly match the tested template.
🤖 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/agents/ux-ui-designer.md:
- Around line 41-46: The spec currently mixes GUI/web requirements into the
terminal-only UX guideline: update the "Spec the component" list (the items
mentioning "Responsive behavior (mobile → tablet → desktop)", "Interactions
(click, hover, keyboard, drag)" and any "mobile-first" references) to remove
web-specific patterns and replace them with terminal-appropriate requirements —
e.g., terminal window size/TTY breakpoints, keyboard-only interactions and
focus/aria-like textual affordances (keyboard navigation, shortcuts, Enter/Esc),
text-wrapping and truncation rules, handling of missing images as fallback text,
and states appropriate for CLI (loading, empty, error, success, disabled,
focus). Also sweep the nearby blocks that repeat these web patterns (the other
occurrences around the same component spec sections) and align them to the
terminal-only rule declared earlier.
- Around line 37-40: The fenced code block that starts with ``` and contains
"Use Glob to find existing commands: src/commands/*.js" and "Use Grep to find
`@clack/prompts` usage: confirm|select|multiselect|spinner" lacks a language
identifier (triggers MD040); fix it by adding a language id (e.g., text) after
the opening ``` so the block becomes ```text, ensuring markdownlint no longer
flags MD040 and the block renders as plain text.
In @.claude/hooks/graph-context-prompt.mjs:
- Around line 406-411: The timeout callback currently sets process.exitCode = 0
which does not terminate the process if main() hangs; change the timeout
callback to call process.exit(0) to forcefully terminate; update the code where
setTimeout defines timer (and references process.stderr.write and
process.exitCode) to call process.exit(0) instead, and keep the existing
clearTimeout(timer) in main().finally() to prevent the forceful exit when main()
completes normally.
In @.claude/hooks/post-tool-use-tracker.sh:
- Around line 26-30: Guard against empty SCRIPT_DIR by validating the result of
get_script_dir before using it to compute PROJECT_DIR: call get_script_dir into
SCRIPT_DIR, then only run cd "$SCRIPT_DIR/../.." and assign PROJECT_DIR if
SCRIPT_DIR is non-empty and the cd succeeds; keep the existing fallback to
CLAUDE_PROJECT_DIR and ensure the conditional that checks [[ -z
"$CLAUDE_PROJECT_DIR" ]] && [[ -z "$PROJECT_DIR" ]] remains and exits early when
both are still unset. Refer to get_script_dir, SCRIPT_DIR, PROJECT_DIR, and
CLAUDE_PROJECT_DIR to locate and update the assignment and early-exit logic.
- Around line 270-275: The session update path never updates the stored repo, so
sessions from one repo persist when switching; modify the jq invocation that
writes to "$session_file" to also pass the current repo (e.g. add --arg repo
"$repo") and set .repo = $repo in the filter (e.g. prepend '.repo = $repo | '
before the existing modifications) so the session file's repo field is kept in
sync with the current repository when updating via the session_file/jq block
that uses $skill and $time.
In @.claude/hooks/save-tokens-precompact.sh:
- Around line 17-18: The hook currently runs the Node script without forwarding
stdin, but save-tokens.mjs's readHookInput() needs stdin; modify the invocation
in save-tokens-precompact.sh so it forwards stdin to the Node process (e.g., use
piping or explicit stdin redirection) and keep the existing env vars: call
something like "ASPENS_PROJECT_DIR=\"$PROJECT_DIR\" NODE_NO_WARNINGS=1 node
\"$SCRIPT_DIR/save-tokens.mjs\" precompact <&0" (or equivalent piping used in
save-tokens-statusline.sh) so readHookInput() receives the hook input.
In @.claude/hooks/save-tokens.mjs:
- Around line 294-301: safeProjectFilePath currently calls resolve(filePath)
which resolves relative paths against cwd instead of the project root; change it
to resolve the user-provided filePath against projectRoot (e.g., use
path.resolve(projectRoot, filePath)) and then compute the relative path with
path.relative(projectRoot, candidate) to perform the boundary check; ensure you
still return the absolute candidate when inside the project and null otherwise,
and keep using the same function name safeProjectFilePath and variables
projectRoot, candidate, rel for clarity.
In `@src/commands/save-tokens.js`:
- Around line 190-200: readJsonFile can return null on parse failure, but the
code proceeds to mergeSettings and writeFileSync, overwriting the invalid
settings; update the flow in save-tokens.js to detect when existingSettings ===
null (from readJsonFile) and in that case do NOT create a .bak or write a new
settings.json; instead log a descriptive error including the settingsPath and
suggested remediation (e.g., fix or restore the JSON) and call process.exit(1)
to abort; apply the same guard where mergeSettings/writeFileSync is used
(including the other occurrence referenced around the 216-223 block) so invalid
parsed config never gets silently overwritten.
In `@src/lib/save-tokens.js`:
- Around line 74-78: The recommendation text in buildSaveTokensRecommendations
currently hard-codes "200k" causing contradictory guidance when
config.compactAtTokens is changed; update the second string in
buildSaveTokensRecommendations to use the compact threshold value (e.g.,
formatCompactLabel(config.compactAtTokens) or config.compactAtTokens) instead of
the literal "200k" so both lines reflect the configured compactAtTokens value.
In `@src/templates/commands/resume-handoff-latest.md`:
- Line 10: Update the template text in resume-handoff-latest.md to clarify how
"newest" is determined: replace "pick the newest by filename" with a
deterministic rule—either require filenames to follow a sortable timestamp
convention (e.g., YYYYMMDD-HHMMSS-suffix) and state that files are chosen by
lexicographic order, or explicitly state a fallback to file modification time
(mtime) when filenames are not timestamped; also mention that this fallback will
use the most recent mtime among .aspens/sessions/*-handoff.md and that
index.json absence/staleness triggers this selection.
In `@src/templates/hooks/save-tokens-prompt-guard.sh`:
- Around line 17-18: The wrapper script fails to forward stdin so
runPromptGuard()'s readHookInput() in save-tokens.mjs gets no payload; capture
the hook JSON from stdin (e.g., INPUT=$(cat) or similar) and pipe it into the
Node invocation that runs save-tokens.mjs with the prompt-guard arg (preserving
ASPENS_PROJECT_DIR and NODE_NO_WARNINGS and the final exit 0), mirroring the
approach used in save-tokens-statusline.sh so readHookInput() in save-tokens.mjs
receives the hook input.
In `@src/templates/hooks/save-tokens.mjs`:
- Around line 307-320: The code reads transcriptPath directly which allows
relative paths to resolve against CWD and permits absolute/outside-repo reads;
update the logic around transcriptPath (and the use of existsSync/readFileSync)
to first resolve the input against the selected projectDir using path.resolve or
path.join (e.g., resolvedTranscript = path.resolve(projectDir,
input.transcript_path || input.transcriptPath || '')), then validate the
resolved path is inside projectDir (use path.relative(projectDir,
resolvedTranscript) and reject if it starts with '..' or is outside), and only
then call existsSync/readFileSync on the resolved path; for any stored/display
path use path.relative(projectDir, resolvedTranscript) to avoid saving raw
user-provided paths.
In `@tests/impact.test.js`:
- Around line 127-134: The test fixture passed to computeHealthScore uses the
field hooksInstalled, but the implementation evaluates hooksHealthy; change the
object property to hooksHealthy (e.g., replace hooksInstalled: false with
hooksHealthy: false) so the test exercises the actual hook-health code path in
computeHealthScore and will fail if hook-health behavior regresses; update any
other fixtures in tests referencing hooksInstalled to use hooksHealthy to keep
them consistent with computeHealthScore's API.
---
Outside diff comments:
In @.claude/hooks/skill-activation-prompt.mjs:
- Around line 218-228: JSDoc for formatOutput is stale: the `@param` {string}
projectDir entry no longer matches the function signature; remove the obsolete
`@param` line (or update the signature if you intentionally need projectDir) so
the JSDoc accurately reflects the actual parameters of the export function
formatOutput(matched, currentRepo), and ensure other `@param` tags still match the
parameter names and types.
In `@src/commands/doc-init.js`:
- Around line 241-266: The current flow ignores a persisted backend in the
loaded config when `recommended` is true and both CLIs are available, causing a
silent backend switch; change the selection logic after `const { config } =
loadConfig(repoPath, { persist: false })` so that if `recommended` is true and
`config.backend` (or equivalent persisted backend field) exists you call
`resolveBackend({ backendFlag: config.backend, available })` and set
`backendResult` to that, instead of prompting or falling through to the default
branch; update the same logic paths that use `recommendedTargetIds` (including
the other block noted around 303-309) to honor the persisted `config.backend`
first.
---
Duplicate comments:
In `@src/commands/doc-impact.js`:
- Around line 15-19: Wrap the analyzeImpact/summarizeValueComparison block in
try/catch (or try/finally) to ensure spinner (variable spinner) is stopped on
both success and error paths; on error, stop the spinner and throw a CliError
(imported/used where other commands do) with a descriptive message that includes
repo-specific remediation hint (mention repoPath/options context) instead of
letting the raw upstream error bubble up; rethrow or include the original error
as cause/detail so bin/cli.js can render it.
- Around line 164-177: The command currently prompts to mutate the repo whenever
buildApplyPlan(report.targets) returns actions; change it to be read-only unless
the user passes an explicit --apply flag: before calling p.confirm or running
applyRecommendedAction, check a single explicit flag (e.g., options.apply or
args.apply) and only then call p.confirm({ message:
buildApplyConfirmationMessage(), ... }) and loop over applyPlan to call
applyRecommendedAction(repoPath, item.action, options, item.target); if the flag
is not set, skip p.confirm and the apply loop and instead log the planned
actions summary (keeping buildApplyPlan, buildApplyConfirmationMessage,
p.confirm and applyRecommendedAction usage intact when --apply is provided).
In `@src/commands/doc-init.js`:
- Around line 374-376: The current branch sets skipDiscovery = true when the
recommended flag is set, causing effectiveDomains to prefer reusedDomains and
completely replace scan.domains; instead remove or avoid setting skipDiscovery
in the recommended branch so scan.domains remains the base, and change the merge
logic where effectiveDomains is computed to merge reusedDomains metadata into
scan.domains (augmenting existing domain entries) rather than replacing them;
apply the same fix to the second occurrence of this pattern around the other
recommended-check (the block at 492-496) so reused metadata is merged into
scan.domains rather than overriding it.
- Around line 1676-1703: The post-processing call to ensureRootKeyFilesSection
is only applied to chunked CLAUDE.md outputs (inside the runLLM/retry flow) and
must be applied to all generation modes; update the flow that handles completed
generated files (the variable files after parseLLMOutput and also the return
value of generateAllAtOnce) to run ensureRootKeyFilesSection(file.content,
repoGraph) for any file.path === 'CLAUDE.md' before calling
validateGeneratedChunk or writing files out (i.e., centralize the normalization
so both the chunked path and generateAllAtOnce path invoke
ensureRootKeyFilesSection on CLAUDE.md).
- Around line 821-825: The status message incorrectly reports hook updates based
on flags (hasHookTarget and options.hooks) rather than the actual outcome;
update the console.log construction to use the real install result from
installHooks() (e.g., an installResult or hooksResult returned by
installHooks/performInstall) and derive the hook text from that object (like
installResult.hooksUpdated, installResult.hooksSkipped, or
installResult.hooksFailed) instead of hasHookTarget/options.hooks, while keeping
the other parts (targets.map(...), effectiveDomains.length) unchanged so the
message reflects the true result of installHooks().
In `@src/lib/impact.js`:
- Around line 275-283: The affectedTargets count currently filters targets by
drift.changedCount and target.status.domains/instructions, but omits targets
whose hooks are 'missing' or 'broken'; update the filter used to compute
affectedTargets (the targets.filter(...) expression) to also treat a target as
affected when target.status.hooks !== 'healthy' (i.e., include hooks status
alongside domains and instructions) so hook-only failures are counted.
- Around line 752-796: The depth cap in function walk (used to populate files,
newestSourceMtime) stops recursion at depth > 5 and misses deeper monorepo paths
(e.g., apps/web/src), undercounting newestSourceMtime, changedCount and
affectedDomains; remove or increase the hardcoded cap and instead use a
configurable MAX_DEPTH or no cap, update references in walk and its call
walk(repoPath, 0) accordingly, and ensure heavy directories are still pruned by
the existing skip-list (node_modules, dist, etc.) rather than an arbitrary
shallow depth limit so deep source files are discovered and newestSourceMtime is
accurate.
- Around line 251-258: The condition that gates recommending a base-only rewrite
allows broken hooks because it checks target.status.hooks !== 'missing'; change
the check to require hooks are healthy (e.g., target.status.hooks === 'healthy')
before pushing the action in the block containing actions.push('aspens doc init
--mode base-only --strategy rewrite') so the rewrite is only suggested when
hooks are fully healthy.
---
Nitpick comments:
In @.claude/agents/plan.md:
- Around line 59-67: The markdown fences around the plan table and the "Use the
Agent tool:" snippet are bare and trigger MD040; update the opening/closing
fence for the table block to use ```markdown and update the Agent snippet fences
to use a language label such as ```yaml or ```text (apply the same change to the
other occurrence noted around lines 114-118), making sure the fenced block that
contains the table and the fenced block that starts with "Use the Agent tool:"
both include the language token so markdownlint no longer flags them.
In @.claude/hooks/graph-context-prompt.mjs:
- Around line 374-384: The dynamic import of fs inside the ASPENS_DEBUG block is
redundant; add writeFileSync to the static import that already provides
readFileSync and existsSync (so writeFileSync is available at module scope) and
remove the await import('fs') and destructuring inside the try block; then call
the module-scope writeFileSync to write the debug file when
process.env.ASPENS_DEBUG === '1' and debugInfo is present (preserve writing
timestamp, projectDir, prompt, and ...debugInfo and the try/catch behavior).
In @.claude/hooks/save-tokens.mjs:
- Around line 97-146: The checked-in hook .claude/hooks/save-tokens.mjs diverges
from the shipped template and tests; make the implementations identical by
either (A) replacing the body of saveHandoff in .claude/hooks/save-tokens.mjs
with the same implementation used in src/templates/hooks/save-tokens.mjs, or (B)
refactoring to a single shared implementation that both files import (e.g., move
the logic that calls sessionTokenSnapshot, extractPrompt, readTranscriptExcerpt,
writeLatestIndex and pruneOldHandoffs into a shared module and have saveHandoff
stub call it), ensuring saveHandoff, writeLatestIndex and pruneOldHandoffs
behavior/format exactly match the tested template.
In @.claude/settings.json:
- Around line 6-19: The hook "command" values are inconsistent: some are plain
strings and others contain escaped quotes produced by buildSaveTokensSettings();
fix by making the styles consistent — preferably remove the extra escaped quotes
so the JSON command values are plain paths (e.g.,
$CLAUDE_PROJECT_DIR/.claude/hooks/...), and update buildSaveTokensSettings() to
stop wrapping the command in literal quotes when it generates the hook; ensure
any other hook generators follow the same unquoted string format so all
"command" fields match.
In `@src/lib/git-hook.js`:
- Around line 83-93: Wrap the synchronous file reads around hookPath (the
readFileSync calls used to load the existing hook and the other read at line
~122) in try/catch blocks so IO races don't throw; if readFileSync fails, handle
the error by returning null (or exiting the current install/upgrade flow)
instead of letting the exception propagate. Specifically, around the code that
calls readFileSync(hookPath, 'utf8') used with existing, and the later
readFileSync usage, catch any error, optionally log a debug message, and return
null so downstream logic (hasUnlabeledAspensBlock, buildUnlabeledMarkerRegex,
writeFileSync) doesn’t run on a failed read.
In `@src/templates/commands/resume-handoff-latest.md`:
- Around line 9-11: Reword the second bullet in
src/templates/commands/resume-handoff-latest.md to avoid repeating "If" at the
start of each step: keep step 1 as-is, change step 2 to something like "When
index.json is missing, stale, or points to a deleted file, list
`.aspens/sessions/*-handoff.md` and pick the newest by filename" and keep step 3
unchanged; this preserves logic but improves readability by varying sentence
openings.
🪄 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: 6ed3d943-f4ae-48ec-a102-23b1c606f756
📒 Files selected for processing (72)
.agents/skills/architecture/references/code-map.md.agents/skills/base/SKILL.md.agents/skills/claude-runner/SKILL.md.agents/skills/codex-support/SKILL.md.agents/skills/doc-impact/SKILL.md.agents/skills/save-tokens/SKILL.md.agents/skills/skill-generation/SKILL.md.agents/skills/template-library/SKILL.md.aspens.json.aspens/sessions/.gitignore.aspens/sessions/README.md.claude/agents/auto-error-resolver.md.claude/agents/code-architecture-reviewer.md.claude/agents/code-refactor-master.md.claude/agents/documentation-architect.md.claude/agents/execute.md.claude/agents/ghost-writer.md.claude/agents/plan-reviewer.md.claude/agents/plan.md.claude/agents/refactor-planner.md.claude/agents/ux-ui-designer.md.claude/agents/web-research-specialist.md.claude/commands/resume-handoff-latest.md.claude/commands/resume-handoff.md.claude/commands/save-handoff.md.claude/hooks/graph-context-prompt.mjs.claude/hooks/graph-context-prompt.sh.claude/hooks/post-tool-use-tracker.sh.claude/hooks/save-tokens-precompact.sh.claude/hooks/save-tokens-prompt-guard.sh.claude/hooks/save-tokens-statusline.sh.claude/hooks/save-tokens.mjs.claude/hooks/skill-activation-prompt.mjs.claude/settings.json.claude/skills/base/skill.md.claude/skills/claude-runner/skill.md.claude/skills/codex-support/skill.md.claude/skills/doc-impact/skill.md.claude/skills/save-tokens/skill.md.claude/skills/skill-generation/skill.md.claude/skills/skill-rules.json.claude/skills/template-library/skill.md.gitignoreAGENTS.mdCLAUDE.mdbin/cli.jssrc/commands/doc-impact.jssrc/commands/doc-init.jssrc/commands/save-tokens.jssrc/lib/git-hook.jssrc/lib/impact.jssrc/lib/runner.jssrc/lib/save-tokens.jssrc/lib/skill-writer.jssrc/lib/target.jssrc/templates/commands/resume-handoff-latest.mdsrc/templates/commands/resume-handoff.mdsrc/templates/commands/save-handoff.mdsrc/templates/hooks/graph-context-prompt.mjssrc/templates/hooks/post-tool-use-tracker.shsrc/templates/hooks/save-tokens-precompact.shsrc/templates/hooks/save-tokens-prompt-guard.shsrc/templates/hooks/save-tokens-statusline.shsrc/templates/hooks/save-tokens.mjssrc/templates/hooks/skill-activation-prompt.mjstests/git-hook.test.jstests/impact.test.jstests/save-tokens-hook-lib.test.jstests/save-tokens-prompt-guard.test.jstests/save-tokens.test.jstests/skill-writer.test.jstests/target.test.js
✅ Files skipped from review due to trivial changes (20)
- .gitignore
- .aspens/sessions/.gitignore
- .aspens/sessions/README.md
- .claude/commands/save-handoff.md
- src/templates/commands/save-handoff.md
- .claude/hooks/save-tokens-prompt-guard.sh
- src/templates/hooks/save-tokens-statusline.sh
- .claude/commands/resume-handoff.md
- .aspens.json
- src/templates/commands/resume-handoff.md
- .claude/agents/plan-reviewer.md
- .claude/agents/auto-error-resolver.md
- .claude/agents/documentation-architect.md
- .agents/skills/architecture/references/code-map.md
- .claude/agents/code-architecture-reviewer.md
- AGENTS.md
- .claude/skills/save-tokens/skill.md
- .agents/skills/save-tokens/SKILL.md
- .claude/skills/template-library/skill.md
- .claude/skills/doc-impact/skill.md
🚧 Files skipped from review as they are similar to previous changes (12)
- CLAUDE.md
- src/templates/hooks/graph-context-prompt.mjs
- .claude/skills/skill-generation/skill.md
- .agents/skills/base/SKILL.md
- src/lib/target.js
- .claude/skills/codex-support/skill.md
- src/templates/hooks/post-tool-use-tracker.sh
- .claude/skills/skill-rules.json
- src/templates/hooks/skill-activation-prompt.mjs
- .agents/skills/doc-impact/SKILL.md
- .agents/skills/skill-generation/SKILL.md
- .agents/skills/codex-support/SKILL.md
Comment on lines
+37
to
+40
| ``` | ||
| Use Glob to find existing commands: src/commands/*.js | ||
| Use Grep to find @clack/prompts usage: confirm|select|multiselect|spinner | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
Line 37 starts a fenced block without a language, which triggers markdownlint MD040.
Suggested patch
- ```
+ ```text
Use Glob to find existing commands: src/commands/*.js
Use Grep to find `@clack/prompts` usage: confirm|select|multiselect|spinner
```🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/ux-ui-designer.md around lines 37 - 40, The fenced code block
that starts with ``` and contains "Use Glob to find existing commands:
src/commands/*.js" and "Use Grep to find `@clack/prompts` usage:
confirm|select|multiselect|spinner" lacks a language identifier (triggers
MD040); fix it by adding a language id (e.g., text) after the opening ``` so the
block becomes ```text, ensuring markdownlint no longer flags MD040 and the block
renders as plain text.
Comment on lines
+41
to
+46
| 3. **Spec the component** — For each component, define: | ||
| - Layout and visual hierarchy | ||
| - All states: loading, empty, error, success, disabled, hover, focus | ||
| - Responsive behavior (mobile → tablet → desktop) | ||
| - Interactions (click, hover, keyboard, drag) | ||
| - Content limits (what happens with long text, missing images, etc.) |
There was a problem hiding this comment.
Remove web/mobile UX requirements that conflict with terminal-only scope.
Line 44, Line 45, and Line 63 introduce web/app patterns (responsive, click/hover/drag, mobile-first) that conflict with Line 12’s terminal-only rule. This will misdirect the agent toward non-CLI specs.
Suggested patch
-description: UX/UI design guidance — component specs with states, accessibility audits, user flow analysis, design system recommendations. For developers building interfaces.
+description: UX/UI design guidance — CLI interaction specs with states, accessibility checks, user-flow analysis, and terminal design recommendations. For developers building terminal interfaces.
@@
3. **Spec the component** — For each component, define:
- Layout and visual hierarchy
- All states: loading, empty, error, success, disabled, hover, focus
- - Responsive behavior (mobile → tablet → desktop)
+ - Terminal behavior across contexts (interactive TTY vs non-interactive/CI)
- - Interactions (click, hover, keyboard, drag)
+ - Interactions (keyboard navigation, selection, cancellation, confirmation)
- - Content limits (what happens with long text, missing images, etc.)
+ - Content limits (long text wrapping/truncation, narrow terminal widths)
@@
- - Screen readers: do images have alt text? Do buttons have labels? Do dynamic changes announce themselves?
+ - Screen readers: are prompt labels explicit, and are status updates announced in plain text?
- - Color contrast: WCAG AA minimum (4.5:1 for text, 3:1 for large text)
+ - Color dependence: does output remain understandable without color?
@@
-- Mobile-first — design for small screens, enhance for large ones
+- Terminal-first — design for narrow terminal widths, then enhance for wider terminalsAlso applies to: 48-53, 59-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/ux-ui-designer.md around lines 41 - 46, The spec currently
mixes GUI/web requirements into the terminal-only UX guideline: update the "Spec
the component" list (the items mentioning "Responsive behavior (mobile → tablet
→ desktop)", "Interactions (click, hover, keyboard, drag)" and any
"mobile-first" references) to remove web-specific patterns and replace them with
terminal-appropriate requirements — e.g., terminal window size/TTY breakpoints,
keyboard-only interactions and focus/aria-like textual affordances (keyboard
navigation, shortcuts, Enter/Esc), text-wrapping and truncation rules, handling
of missing images as fallback text, and states appropriate for CLI (loading,
empty, error, success, disabled, focus). Also sweep the nearby blocks that
repeat these web patterns (the other occurrences around the same component spec
sections) and align them to the terminal-only rule declared earlier.
Comment on lines
+406
to
+411
| if (process.argv[1] === fileURLToPath(import.meta.url)) { | ||
| const timer = setTimeout(() => { | ||
| process.stderr.write('[Graph] Timeout after 5s\n'); | ||
| process.exitCode = 0; | ||
| }, 5000); | ||
| main().finally(() => clearTimeout(timer)); |
There was a problem hiding this comment.
Timeout guard doesn't actually terminate the process.
Setting process.exitCode = 0 doesn't stop execution. If main() hangs, the timeout fires but the process continues indefinitely. Use process.exit(0) to forcibly terminate.
Proposed fix
if (process.argv[1] === fileURLToPath(import.meta.url)) {
const timer = setTimeout(() => {
process.stderr.write('[Graph] Timeout after 5s\n');
- process.exitCode = 0;
+ process.exit(0);
}, 5000);
main().finally(() => clearTimeout(timer));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (process.argv[1] === fileURLToPath(import.meta.url)) { | |
| const timer = setTimeout(() => { | |
| process.stderr.write('[Graph] Timeout after 5s\n'); | |
| process.exitCode = 0; | |
| }, 5000); | |
| main().finally(() => clearTimeout(timer)); | |
| if (process.argv[1] === fileURLToPath(import.meta.url)) { | |
| const timer = setTimeout(() => { | |
| process.stderr.write('[Graph] Timeout after 5s\n'); | |
| process.exit(0); | |
| }, 5000); | |
| main().finally(() => clearTimeout(timer)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/graph-context-prompt.mjs around lines 406 - 411, The timeout
callback currently sets process.exitCode = 0 which does not terminate the
process if main() hangs; change the timeout callback to call process.exit(0) to
forcefully terminate; update the code where setTimeout defines timer (and
references process.stderr.write and process.exitCode) to call process.exit(0)
instead, and keep the existing clearTimeout(timer) in main().finally() to
prevent the forceful exit when main() completes normally.
Comment on lines
+26
to
+30
| SCRIPT_DIR="$(get_script_dir)" | ||
| PROJECT_DIR="$(cd "$SCRIPT_DIR/../.." && pwd)" | ||
|
|
||
| # Exit early if neither CLAUDE_PROJECT_DIR nor PROJECT_DIR is set | ||
| if [[ -z "$CLAUDE_PROJECT_DIR" ]] && [[ -z "$PROJECT_DIR" ]]; then |
There was a problem hiding this comment.
Bail out if project-dir resolution fails.
If get_script_dir returns nothing here, cd "$SCRIPT_DIR/../.." resolves to /, so Line 30 never trips and the hook starts writing under /.claude/.... Guard both assignments and skip on failure.
Suggested fix
-SCRIPT_DIR="$(get_script_dir)"
-PROJECT_DIR="$(cd "$SCRIPT_DIR/../.." && pwd)"
+SCRIPT_DIR="$(get_script_dir)" || exit 0
+[[ -n "$SCRIPT_DIR" ]] || exit 0
+PROJECT_DIR="$(cd "$SCRIPT_DIR/../.." && pwd)" || exit 0📝 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
| SCRIPT_DIR="$(get_script_dir)" | |
| PROJECT_DIR="$(cd "$SCRIPT_DIR/../.." && pwd)" | |
| # Exit early if neither CLAUDE_PROJECT_DIR nor PROJECT_DIR is set | |
| if [[ -z "$CLAUDE_PROJECT_DIR" ]] && [[ -z "$PROJECT_DIR" ]]; then | |
| SCRIPT_DIR="$(get_script_dir)" || exit 0 | |
| [[ -n "$SCRIPT_DIR" ]] || exit 0 | |
| PROJECT_DIR="$(cd "$SCRIPT_DIR/../.." && pwd)" || exit 0 | |
| # Exit early if neither CLAUDE_PROJECT_DIR nor PROJECT_DIR is set | |
| if [[ -z "$CLAUDE_PROJECT_DIR" ]] && [[ -z "$PROJECT_DIR" ]]; then |
🤖 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 26 - 30, Guard against
empty SCRIPT_DIR by validating the result of get_script_dir before using it to
compute PROJECT_DIR: call get_script_dir into SCRIPT_DIR, then only run cd
"$SCRIPT_DIR/../.." and assign PROJECT_DIR if SCRIPT_DIR is non-empty and the cd
succeeds; keep the existing fallback to CLAUDE_PROJECT_DIR and ensure the
conditional that checks [[ -z "$CLAUDE_PROJECT_DIR" ]] && [[ -z "$PROJECT_DIR"
]] remains and exits early when both are still unset. Refer to get_script_dir,
SCRIPT_DIR, PROJECT_DIR, and CLAUDE_PROJECT_DIR to locate and update the
assignment and early-exit logic.
Comment on lines
+270
to
+275
| # Create or update session file (jq required — checked at script entry) | ||
| if [[ -f "$session_file" ]]; then | ||
| # Check if jq is available | ||
| if command -v jq &> /dev/null; then | ||
| # Add skill to array, keeping unique values | ||
| jq --arg skill "$skill" --arg time "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | ||
| '.active_skills = ((.active_skills + [$skill]) | unique) | .last_updated = $time' \ | ||
| "$session_file" > "${session_file}.tmp" 2>/dev/null && \ | ||
| mv "${session_file}.tmp" "$session_file" | ||
| else | ||
| # Fallback: simple append check without jq | ||
| if ! grep -q "\"$skill\"" "$session_file" 2>/dev/null; then | ||
| # Read existing skills from file, append new one, rewrite | ||
| local existing_skills="" | ||
| if [[ -f "$session_file" ]]; then | ||
| # Extract skills array content: strip brackets, quotes, whitespace | ||
| existing_skills=$(grep -o '"active_skills":\[[^]]*\]' "$session_file" 2>/dev/null | sed 's/"active_skills":\[//;s/\]//;s/"//g;s/ //g') | ||
| fi | ||
| # Build new skills list | ||
| local new_skills="" | ||
| if [[ -n "$existing_skills" ]]; then | ||
| new_skills="\"$(echo "$existing_skills" | sed 's/,/","/g')\",\"$skill\"" | ||
| else | ||
| new_skills="\"$skill\"" | ||
| fi | ||
| echo "{\"repo\":\"$repo\",\"active_skills\":[$new_skills],\"last_updated\":\"$(date -u +%Y-%m-%dT%H:%M:%SZ)\"}" > "$session_file" | ||
| fi | ||
| fi | ||
| jq --arg skill "$skill" --arg time "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | ||
| '.active_skills = ((.active_skills + [$skill]) | unique) | .last_updated = $time' \ | ||
| "$session_file" > "${session_file}.tmp" 2>/dev/null && \ | ||
| mv "${session_file}.tmp" "$session_file" |
There was a problem hiding this comment.
Repo-scoped session state breaks after the first repo switch.
The update path never changes repo, but the reader drops the whole session when session.repo !== currentRepo in src/templates/hooks/skill-activation-prompt.mjs, Lines 95-114. In a monorepo, edits in repo B get appended into repo A's session file and will never become sticky.
Suggested fix
- jq --arg skill "$skill" --arg time "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \
- '.active_skills = ((.active_skills + [$skill]) | unique) | .last_updated = $time' \
+ jq --arg skill "$skill" --arg repo "$repo" --arg time "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \
+ 'if (.repo // $repo) != $repo
+ then .repo = $repo | .active_skills = [$skill]
+ else .repo = $repo | .active_skills = (((.active_skills // []) + [$skill]) | unique)
+ end
+ | .last_updated = $time' \
"$session_file" > "${session_file}.tmp" 2>/dev/null && \
mv "${session_file}.tmp" "$session_file"📝 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
| # Create or update session file (jq required — checked at script entry) | |
| if [[ -f "$session_file" ]]; then | |
| # Check if jq is available | |
| if command -v jq &> /dev/null; then | |
| # Add skill to array, keeping unique values | |
| jq --arg skill "$skill" --arg time "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | |
| '.active_skills = ((.active_skills + [$skill]) | unique) | .last_updated = $time' \ | |
| "$session_file" > "${session_file}.tmp" 2>/dev/null && \ | |
| mv "${session_file}.tmp" "$session_file" | |
| else | |
| # Fallback: simple append check without jq | |
| if ! grep -q "\"$skill\"" "$session_file" 2>/dev/null; then | |
| # Read existing skills from file, append new one, rewrite | |
| local existing_skills="" | |
| if [[ -f "$session_file" ]]; then | |
| # Extract skills array content: strip brackets, quotes, whitespace | |
| existing_skills=$(grep -o '"active_skills":\[[^]]*\]' "$session_file" 2>/dev/null | sed 's/"active_skills":\[//;s/\]//;s/"//g;s/ //g') | |
| fi | |
| # Build new skills list | |
| local new_skills="" | |
| if [[ -n "$existing_skills" ]]; then | |
| new_skills="\"$(echo "$existing_skills" | sed 's/,/","/g')\",\"$skill\"" | |
| else | |
| new_skills="\"$skill\"" | |
| fi | |
| echo "{\"repo\":\"$repo\",\"active_skills\":[$new_skills],\"last_updated\":\"$(date -u +%Y-%m-%dT%H:%M:%SZ)\"}" > "$session_file" | |
| fi | |
| fi | |
| jq --arg skill "$skill" --arg time "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | |
| '.active_skills = ((.active_skills + [$skill]) | unique) | .last_updated = $time' \ | |
| "$session_file" > "${session_file}.tmp" 2>/dev/null && \ | |
| mv "${session_file}.tmp" "$session_file" | |
| # Create or update session file (jq required — checked at script entry) | |
| if [[ -f "$session_file" ]]; then | |
| jq --arg skill "$skill" --arg repo "$repo" --arg time "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | |
| 'if (.repo // $repo) != $repo | |
| then .repo = $repo | .active_skills = [$skill] | |
| else .repo = $repo | .active_skills = (((.active_skills // []) + [$skill]) | unique) | |
| end | |
| | .last_updated = $time' \ | |
| "$session_file" > "${session_file}.tmp" 2>/dev/null && \ | |
| mv "${session_file}.tmp" "$session_file" |
🤖 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 270 - 275, The session
update path never updates the stored repo, so sessions from one repo persist
when switching; modify the jq invocation that writes to "$session_file" to also
pass the current repo (e.g. add --arg repo "$repo") and set .repo = $repo in the
filter (e.g. prepend '.repo = $repo | ' before the existing modifications) so
the session file's repo field is kept in sync with the current repository when
updating via the session_file/jq block that uses $skill and $time.
| ## Steps | ||
|
|
||
| 1. Read `.aspens/sessions/index.json`. If it has a `latest` field, verify the referenced file exists before reading it. If the file is missing, fall back to step 2. | ||
| 2. If `index.json` is missing, stale, or points to a deleted file, list `.aspens/sessions/*-handoff.md`, pick the newest by filename. |
There was a problem hiding this comment.
Clarify “newest” selection rule to avoid wrong handoff restore.
“Pick the newest by filename” can select the wrong file unless filename ordering is guaranteed to encode time. Please specify a sortable naming contract (or fallback to mtime) in this template.
Proposed wording update
-2. If `index.json` is missing, stale, or points to a deleted file, list `.aspens/sessions/*-handoff.md`, pick the newest by filename.
+2. If `index.json` is missing, stale, or points to a deleted file, list `.aspens/sessions/*-handoff.md` and pick the newest using a deterministic rule: sortable timestamp in filename (preferred), otherwise file modified time.As per coding guidelines, this template needs clear and correct user-facing behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 2. If `index.json` is missing, stale, or points to a deleted file, list `.aspens/sessions/*-handoff.md`, pick the newest by filename. | |
| 2. If `index.json` is missing, stale, or points to a deleted file, list `.aspens/sessions/*-handoff.md` and pick the newest using a deterministic rule: sortable timestamp in filename (preferred), otherwise file modified time. |
🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ile is missing, fall back to step 2. 2. If index.json is missing, stale, or poin...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/templates/commands/resume-handoff-latest.md` at line 10, Update the
template text in resume-handoff-latest.md to clarify how "newest" is determined:
replace "pick the newest by filename" with a deterministic rule—either require
filenames to follow a sortable timestamp convention (e.g.,
YYYYMMDD-HHMMSS-suffix) and state that files are chosen by lexicographic order,
or explicitly state a fallback to file modification time (mtime) when filenames
are not timestamped; also mention that this fallback will use the most recent
mtime among .aspens/sessions/*-handoff.md and that index.json absence/staleness
triggers this selection.
Comment on lines
+127
to
+134
| const score = computeHealthScore({ | ||
| instructionExists: false, | ||
| skillCount: 1, | ||
| hooksInstalled: false, | ||
| domainCoverage: { covered: 1, total: 4 }, | ||
| hubCoverage: { mentioned: 1, total: 3 }, | ||
| drift: { changedFiles: [{}, {}, {}] }, | ||
| }, { supportsHooks: true }); |
There was a problem hiding this comment.
Use the computeHealthScore() field this code path actually consumes.
This fixture sets hooksInstalled, but the implementation penalizes hooksHealthy. The assertion still passes because undefined is falsy, so this test would miss a regression in the hook-health path.
Suggested patch
const score = computeHealthScore({
instructionExists: false,
skillCount: 1,
- hooksInstalled: false,
+ hooksHealthy: false,
domainCoverage: { covered: 1, total: 4 },
hubCoverage: { mentioned: 1, total: 3 },
drift: { changedFiles: [{}, {}, {}] },
}, { supportsHooks: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const score = computeHealthScore({ | |
| instructionExists: false, | |
| skillCount: 1, | |
| hooksInstalled: false, | |
| domainCoverage: { covered: 1, total: 4 }, | |
| hubCoverage: { mentioned: 1, total: 3 }, | |
| drift: { changedFiles: [{}, {}, {}] }, | |
| }, { supportsHooks: true }); | |
| const score = computeHealthScore({ | |
| instructionExists: false, | |
| skillCount: 1, | |
| hooksHealthy: false, | |
| domainCoverage: { covered: 1, total: 4 }, | |
| hubCoverage: { mentioned: 1, total: 3 }, | |
| drift: { changedFiles: [{}, {}, {}] }, | |
| }, { supportsHooks: true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/impact.test.js` around lines 127 - 134, The test fixture passed to
computeHealthScore uses the field hooksInstalled, but the implementation
evaluates hooksHealthy; change the object property to hooksHealthy (e.g.,
replace hooksInstalled: false with hooksHealthy: false) so the test exercises
the actual hook-health code path in computeHealthScore and will fail if
hook-health behavior regresses; update any other fixtures in tests referencing
hooksInstalled to use hooksHealthy to keep them consistent with
computeHealthScore's API.
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
Documentation
Improvements
Tests