Merged
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR extends aspens from a Claude-only system to a multi-target architecture (Claude + Codex). It adds target/backend abstractions and config persistence, routes generation through a backend-aware runner, introduces post-generation transforms for non-claude targets, and updates prompts, docs, tests, and CLI options accordingly. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as bin/cli.js
participant Scanner as scanRepo()
participant Resolver as target/backend resolver
participant Backend as runLLM()/(claude|codex)
participant Transformer as transformForTarget()
participant Writer as writeSkillFiles()/writeTransformedFiles()
participant Config as .aspens.json
User->>CLI: aspens doc init --target all
CLI->>Scanner: scanRepo()
Scanner-->>CLI: repoGraph + target markers
CLI->>Resolver: resolveTargets & resolveBackend()
Resolver-->>CLI: targets, backendId
CLI->>Backend: runLLM(prompt, backendId, cwd)
Backend-->>CLI: canonicalFiles (Claude-format)
CLI->>Transformer: transformForTarget(canonicalFiles, sourceTarget, destTarget, context)
Transformer-->>CLI: transformedFiles
CLI->>Writer: writeSkillFiles(canonicalFiles)
CLI->>Writer: writeTransformedFiles(transformedFiles)
Writer-->>CLI: write results
CLI->>Config: writeConfig(targets, backend)
Config-->>CLI: persisted
CLI-->>User: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (7)
.claude/hooks/graph-context-prompt.sh (1)
67-70: Grep pattern truncates messages containing quotes.The pattern
[^"]*stops at the first"character, so a message like[Graph] Found 5 files in "src/utils"becomes[Graph] Found 5 files in.If this is intentional to strip JSON artifacts, consider documenting it. Otherwise:
-GRAPH_LINE=$(grep -o '\[Graph\] [^"]*' "$STDERR_FILE" | head -1) +GRAPH_LINE=$(grep '^\[Graph\]' "$STDERR_FILE" | head -1)Or if you need to strip only trailing JSON:
-GRAPH_LINE=$(grep -o '\[Graph\] [^"]*' "$STDERR_FILE" | head -1) +GRAPH_LINE=$(grep -oP '\[Graph\] [^\n{]*' "$STDERR_FILE" | head -1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/graph-context-prompt.sh around lines 67 - 70, The grep pattern used to populate GRAPH_LINE in graph-context-prompt.sh stops at the first double-quote and truncates messages; update the grep regex in the GRAPH_LINE assignment to capture the entire remainder of the line (e.g., replace the [^"]* fragment with a pattern that matches the rest of the line) so messages like `[Graph] Found 5 files in "src/utils"` are preserved; if you intentionally want to strip only trailing JSON artifacts instead, apply a follow-up strip step (e.g., a dedicated sed/awk cleanup) after setting GRAPH_LINE rather than truncating via the grep pattern.src/lib/graph-persistence.js (1)
496-503: Consider optional chaining for defensive access.The
target.supportsGraphaccess on line 501 is guarded bytarget &&, which is correct. However, the pattern could be slightly more concise with optional chaining.Current code is functionally correct — this is a minor style suggestion.
✨ Optional simplification
- if (target && target.supportsGraph === false) { + if (target?.supportsGraph === false) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/graph-persistence.js` around lines 496 - 503, In persistGraphArtifacts, simplify the defensive check for target.supportsGraph by using optional chaining; replace the current guarded check (target && target.supportsGraph === false) with a succinct target?.supportsGraph === false condition so the function still returns serialized when target exists but explicitly does not support graph artifacts.src/lib/context-builder.js (1)
114-131: Hardcoded alternate file toggle may limit future extensibility.Line 124 assumes only
CLAUDE.mdandAGENTS.mdexist. If additional targets are added, this ternary won't scale.Consider passing
altInstructionsFilevia options or deriving it from a target config, though this is fine for current scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/context-builder.js` around lines 114 - 131, The code in context-builder.js hardcodes a single alternate file via the altInstructionsFile ternary (based on instructionsFile) which won't scale; change the logic to accept alternatives from options (e.g., options.altInstructionsFile or options.instructionsAlternatives array) or a derived config and iterate over that list instead of a binary ternary. Locate the instructionsFile variable and the altInstructionsFile logic and replace the single-alt logic with a loop over a provided array (falling back to ['AGENTS.md'] if none supplied) that checks existsSync/readFileSafe for each candidate and pushes sections for any found files.docs/target-support.md (1)
58-58: Tighten repeated wording in this sentence.Consider replacing one “right” to avoid repetition (“right instruction files in the right places”).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/target-support.md` at line 58, The sentence "Codex support is based on writing the right instruction files in the right places" repeats "right"; change one occurrence to a synonym (e.g., "correct", "appropriate", or "proper") so it reads like "writing the right instruction files in the appropriate places" (locate this sentence in docs/target-support.md)..claude/skills/codex-support/skill.md (1)
2-3: Aligncodex-supportwith the canonical skill-domain taxonomy (or update that taxonomy).This adds an eighth domain label. If domain handling is expected to remain the canonical seven, this should be folded into an existing domain (or the canonical list should be explicitly updated in the source-of-truth docs/tooling).
Based on learnings: "Skills should be organized into seven skill domains: agent-customization, claude-runner, doc-sync, import-graph, repo-scanning, skill-generation, and template-library".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/codex-support/skill.md around lines 2 - 3, The skill "codex-support" adds an eighth domain label and must be aligned with the canonical seven-domain taxonomy: either change the skill's domain value to one of the existing domains (agent-customization, claude-runner, doc-sync, import-graph, repo-scanning, skill-generation, template-library) or update the canonical taxonomy/source-of-truth to include the new domain; locate the skill by the name "codex-support" in the skill metadata and either fold its domain into the appropriate existing domain or modify the taxonomy docs/tooling that enforce the seven-domain constraint so the new domain is accepted.src/commands/add.js (1)
260-266: Unused variable:backendIdcomputed but not used in scaffold path.
backendIdis assigned at line 263 but only used in the--fromcode path (viagenerateSkillFromDoc). In the scaffold path (lines 330-375), it's never referenced.Move backendId computation to where it's needed
async function addSkillCommand(repoPath, name, options) { const config = readConfig(repoPath); const target = resolveSkillTarget(config); - const backendId = config?.backend || target.id; const skillsDir = join(repoPath, target.skillsDir); const skillFilename = target.skillFilename; const relSkillsDir = target.skillsDir;The
backendIdis correctly computed insidegenerateSkillFromDocat line 380, so this line is redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/add.js` around lines 260 - 266, The variable backendId computed in addSkillCommand is unused in the scaffold code path; remove the redundant assignment in addSkillCommand and only compute backendId where it's actually needed (e.g., inside generateSkillFromDoc), or move the computation to the branch that calls generateSkillFromDoc so backendId is only created when used; update references to backendId to ensure generateSkillFromDoc receives it from its local computation or the branch that invokes it.tests/target.test.js (1)
21-27: Cleanup race condition handling is acceptable but could be more explicit.The empty catch block silently swallows errors. Consider logging in debug mode or making the comment more specific about what race condition is expected (e.g., parallel test runs, CI cleanup).
Optional: Add more context to the catch block
afterAll(() => { try { if (existsSync(FIXTURES_DIR)) { rmSync(FIXTURES_DIR, { recursive: true, force: true }); } - } catch { /* ignore cleanup race with other test files */ } + } catch (err) { + // Cleanup races can occur when parallel test suites share fixture parent dirs + if (process.env.DEBUG) console.warn('Fixture cleanup warning:', err.message); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/target.test.js` around lines 21 - 27, The empty catch in the afterAll cleanup silently swallows all errors; update the afterAll cleanup around existsSync(FIXTURES_DIR) and rmSync(FIXTURES_DIR, { recursive: true, force: true }) to either log the caught error at debug/trace level (e.g., processLogger.debug or console.debug) or replace the generic comment with a specific one describing the expected race (e.g., "ignore cleanup race with parallel test runs or CI artifacts removal"); ensure you still swallow the error for test stability but include the error object in the debug log or a more specific comment so failures can be diagnosed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aspens.json:
- Around line 1-7: The targets array in .aspens.json currently lists only
"codex" which prevents aspens from syncing and managing the Claude artifacts
under .claude; update the targets entry to include both platforms (set
"targets": ["claude","codex"]) so doc sync and commands like aspens add
agent/command/hook operate on both .claude and .agents, or if you've fully
migrated to Codex delete the .claude directory and keep targets as ["codex"];
adjust the "targets" key accordingly to match the repo's actual artifacts.
In `@README.md`:
- Line 7: The heading "Stop re-explaining your repo. Start shipping." is using
H3 (###) directly after the H1, causing a level jump; change that heading from
H3 to H2 (replace "### Stop re-explaining your repo. Start shipping." with "##
Stop re-explaining your repo. Start shipping.") so the markdown heading levels
increment properly and satisfy markdownlint.
In `@src/commands/customize.js`:
- Around line 16-24: Validate the requested command target before enforcing the
Codex-only gating: move or reorder the check that computes isCodexOnly (and the
subsequent CliError throw) in customize.js so it runs after the command/target
validation logic (the code that currently executes around lines 29-37), or
explicitly validate the target first and throw an "Unknown target" error if
invalid; keep references to readConfig, isCodexOnly, and the existing CliError
but ensure the "codex-only" error is only raised for a known valid target that
happens to be codex-only.
In `@src/commands/doc-init.js`:
- Around line 522-536: The current reuseExistingCanonical branch bypasses the
improvement backend by assigning loadTargetFiles(...) directly to allFiles;
change this so loaded Claude files are stored separately (e.g., canonicalFiles)
and do not short-circuit the improvement flow—keep reuseExistingCanonical
detection but avoid setting allFiles directly, then invoke the existing
improvement/publishing logic (the backend call that normally processes allFiles)
so the repo is checked and content is updated; use the same symbols shown:
reuseExistingCanonical, _reuseSourceTarget, loadTargetFiles, and allFiles to
locate and refactor the branch.
- Around line 58-64: The fallback that wraps untagged LLM output into a single
file should only apply for single-file prompts: in parseLLMOutput, change the
condition so you only create the fallback file when there are no parsed <file>
tags AND expectedPath is provided AND the prompt was targeting a single file
(e.g., allowedPaths exists and allowedPaths.length === 1 or expectedPath ===
allowedPaths[0]); keep the other checks (text.trim().length > 50). Ensure
generateAllAtOnce continues to pass allowedPaths/expectedPath so this
restriction prevents collapsing multi-file replies into CLAUDE.md.
- Around line 1063-1116: loadReusableDomains currently produces empty domain
scopes when Codex-transformed skills have their "## Activation" removed (due to
stripActivationSection used by transformDomainSkill) because
parseActivationPatterns returns []. Update loadReusableDomains to detect when a
skill's activationPatterns (or parseActivationPatterns result) is empty and then
fall back to deriving scope from an alternate source: first attempt to read
skill rules from skill-rules.json (as handled in the Claude branch) or, if
unavailable, use frontmatter fields (e.g., skill.frontmatter.path/name/other
metadata) to build directories/files; ensure this fallback logic is applied
where activationPatterns is consumed so domains get non-empty directories/files
even when parseActivationPatterns returns [].
In `@src/commands/doc-sync.js`:
- Around line 87-89: transformForTarget is being called with only scanResult, so
Codex graph artifacts never get emitted; when invoking transformForTarget (the
call with baseFiles, sourceTarget, target, { scanResult: scan }) include the
serialized graph from the scan (e.g. graphSerialized or scan.graphSerialized) in
the options passed to transformForTarget so the function can emit the Codex
architecture skill/reference. Update the caller in publishFilesForTargets to
forward graphSerialized alongside scanResult (or add graphSerialized:
scan.graphSerialized) so transformForTarget receives both scanResult and
graphSerialized.
- Around line 318-325: The code treats an unparseable assistant reply
(parseOutput returning []) the same as "no updates", so replace the silent
success path by detecting unparseable responses: after calling
parseOutput(result.text, allowedPaths) check whether result.text contains any
expected file markers (e.g. "<file>") or modify parseOutput to return a sentinel
(null/throw) on parse failure; if parse failed, stop the spinner and surface an
error via syncSpinner.stop and p.outro (or throw) instead of reporting "Docs are
up to date". Reference parseOutput, result.text, baseFiles, files,
publishFilesForTargets, and the existing syncSpinner.stop/p.outro calls when
implementing the check.
In `@src/lib/runner.js`:
- Around line 129-145: The runCodex function is currently passing the unsafe
'--full-auto' flag which grants workspace-write/shell permissions; remove
'--full-auto' from the args array and replace it with read-only sandboxing flags
such as '--sandbox', 'read-only' and an approval policy like
'--ask-for-approval', 'never' (or 'on-request' for interactive flows) so the
codex process is constrained to read-only file exploration; update the args
construction in runCodex to push these flags (and keep existing model/cwd
handling) so callers that expect read-only behavior (e.g., allowedTools:
['Read','Glob','Grep']) are honored.
In `@src/lib/skill-writer.js`:
- Around line 48-57: The writeTransformedFiles function currently only blocks
absolute paths and traversal but must also enforce allowed repo targets; update
the loop in writeTransformedFiles to validate each file.path using the same
rules as sanitizePath: reject paths with '..' or leading '/' (already done) and
additionally only allow paths that are exactly "CLAUDE.md" or "AGENTS.md" or
that start with the safe prefixes ".claude/", ".agents/", or ".codex/"; if a
path fails this allowed-path check, push a skipped result with reason 'unsafe
path' (same structure as existing skips) before attempting any write operations.
In `@src/lib/target-transform.js`:
- Around line 183-215: buildCodexSkillRefs currently always appends the
architecture skill ref, causing AGENTS.md to reference a non-existent file when
no graph was serialized; modify buildCodexSkillRefs(baseSkill, domainSkills,
destTarget) to accept a new boolean parameter (e.g. graphSerialized or hasGraph)
and only push the architecture entry ('architecture', destTarget.skillFilename
under destTarget.skillsDir) when that flag is true, then update callers
(syncCodexSkillsSection and generateCodexSkillReferences) to pass the graph
presence value (from generateCodexSkillReferences's graphSerialized) through so
the architecture line is gated by actual graph availability.
In `@src/lib/target.js`:
- Around line 177-180: The current hasCodexArtifacts check treats a lone
AGENTS.md (TARGETS.codex.instructionsFile) as enough to infer Codex; change the
logic in hasCodexArtifacts so Codex is only detected when either
TARGETS.codex.configDir ('.codex') or TARGETS.codex.skillsDir ('.agents/skills')
exists, or when the instructionsFile exists in combination with at least one of
those other artifacts; update the boolean expression that computes
hasCodexArtifacts (reference repoPath, TARGETS.codex.configDir,
TARGETS.codex.skillsDir, TARGETS.codex.instructionsFile) so a solitary AGENTS.md
no longer flips Codex mode.
- Around line 106-113: The loop that builds dirPrefixes currently uses
split('/')[0] for t.skillsDir and t.agentsDir which reduces ".agents/skills/**"
to ".agents/**"; change it so the skills allowlist remains at the full skills
path: replace dirPrefixes.add(t.skillsDir.split('/')[0] + '/') with
dirPrefixes.add(t.skillsDir + '/') and replace
dirPrefixes.add(t.agentsDir.split('/')[0] + '/') with
dirPrefixes.add(t.agentsDir + '/skills/'); keep the same
dirPrefixes.add(t.configDir + '/') and exactFiles.add(t.instructionsFile)
behavior.
In `@src/prompts/doc-sync-refresh.md`:
- Line 13: The template path currently hardcodes "billing"
("{{skillsDir}}/billing/{{skillFilename}}"), which causes refreshes to write to
the wrong skill; change the write target to use the mapped skill variable (e.g.,
"{{skillsDir}}/{{mappedSkill}}/{{skillFilename}}" or the equivalent variable
used by the skill-mapping code), and ensure the doc-sync flow follows the
required sequence: run prerequisite checks → compute git diff → map changed
files to skills via activation patterns (use the same mapping logic that
produces mappedSkill) → send filtered segments to Claude for updates (exclude
generic segments) → perform conditional writes only for the mappedSkill output.
Ensure any code that composes file paths references the mappedSkill variable
instead of the hardcoded "billing".
In `@src/prompts/doc-sync.md`:
- Line 13: The doc-sync output template currently hardcodes the skill path
segment ("{{skillsDir}}/billing/{{skillFilename}}"), which can misroute changes;
update the template and pipeline so the skill folder is derived from the
skill-mapping step (use a dynamic placeholder like "{{skill}}" instead of
"billing") and ensure the pipeline follows the required sequence: prerequisite
checks → git diff → skill mapping via activation patterns → Claude update
(filter generic segments) → conditional writes; update any references to the
hardcoded path in the doc-sync template and downstream write logic to use the
mapped "{{skill}}" variable so writes target the correct skill directory.
In `@tests/backend.test.js`:
- Around line 92-99: Replace the try/catch pattern with an assertion that the
call to resolveBackend throws: wrap the call in expect(() => resolveBackend({
available: { claude: false, codex: false } })) and assert it throws with a
message that contains BACKENDS.claude.installUrl and BACKENDS.codex.installUrl;
reference resolveBackend and BACKENDS.claude.installUrl /
BACKENDS.codex.installUrl so the test fails if no error is thrown and still
verifies both URLs are in the error message.
---
Nitpick comments:
In @.claude/hooks/graph-context-prompt.sh:
- Around line 67-70: The grep pattern used to populate GRAPH_LINE in
graph-context-prompt.sh stops at the first double-quote and truncates messages;
update the grep regex in the GRAPH_LINE assignment to capture the entire
remainder of the line (e.g., replace the [^"]* fragment with a pattern that
matches the rest of the line) so messages like `[Graph] Found 5 files in
"src/utils"` are preserved; if you intentionally want to strip only trailing
JSON artifacts instead, apply a follow-up strip step (e.g., a dedicated sed/awk
cleanup) after setting GRAPH_LINE rather than truncating via the grep pattern.
In @.claude/skills/codex-support/skill.md:
- Around line 2-3: The skill "codex-support" adds an eighth domain label and
must be aligned with the canonical seven-domain taxonomy: either change the
skill's domain value to one of the existing domains (agent-customization,
claude-runner, doc-sync, import-graph, repo-scanning, skill-generation,
template-library) or update the canonical taxonomy/source-of-truth to include
the new domain; locate the skill by the name "codex-support" in the skill
metadata and either fold its domain into the appropriate existing domain or
modify the taxonomy docs/tooling that enforce the seven-domain constraint so the
new domain is accepted.
In `@docs/target-support.md`:
- Line 58: The sentence "Codex support is based on writing the right instruction
files in the right places" repeats "right"; change one occurrence to a synonym
(e.g., "correct", "appropriate", or "proper") so it reads like "writing the
right instruction files in the appropriate places" (locate this sentence in
docs/target-support.md).
In `@src/commands/add.js`:
- Around line 260-266: The variable backendId computed in addSkillCommand is
unused in the scaffold code path; remove the redundant assignment in
addSkillCommand and only compute backendId where it's actually needed (e.g.,
inside generateSkillFromDoc), or move the computation to the branch that calls
generateSkillFromDoc so backendId is only created when used; update references
to backendId to ensure generateSkillFromDoc receives it from its local
computation or the branch that invokes it.
In `@src/lib/context-builder.js`:
- Around line 114-131: The code in context-builder.js hardcodes a single
alternate file via the altInstructionsFile ternary (based on instructionsFile)
which won't scale; change the logic to accept alternatives from options (e.g.,
options.altInstructionsFile or options.instructionsAlternatives array) or a
derived config and iterate over that list instead of a binary ternary. Locate
the instructionsFile variable and the altInstructionsFile logic and replace the
single-alt logic with a loop over a provided array (falling back to
['AGENTS.md'] if none supplied) that checks existsSync/readFileSafe for each
candidate and pushes sections for any found files.
In `@src/lib/graph-persistence.js`:
- Around line 496-503: In persistGraphArtifacts, simplify the defensive check
for target.supportsGraph by using optional chaining; replace the current guarded
check (target && target.supportsGraph === false) with a succinct
target?.supportsGraph === false condition so the function still returns
serialized when target exists but explicitly does not support graph artifacts.
In `@tests/target.test.js`:
- Around line 21-27: The empty catch in the afterAll cleanup silently swallows
all errors; update the afterAll cleanup around existsSync(FIXTURES_DIR) and
rmSync(FIXTURES_DIR, { recursive: true, force: true }) to either log the caught
error at debug/trace level (e.g., processLogger.debug or console.debug) or
replace the generic comment with a specific one describing the expected race
(e.g., "ignore cleanup race with parallel test runs or CI artifacts removal");
ensure you still swallow the error for test stability but include the error
object in the debug log or a more specific comment so failures can be diagnosed.
🪄 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: cae6a65e-cdfd-4846-90b2-b949ea276914
📒 Files selected for processing (56)
.agents/skills/agent-customization/SKILL.md.agents/skills/architecture/SKILL.md.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-sync/SKILL.md.agents/skills/import-graph/SKILL.md.agents/skills/repo-scanning/SKILL.md.agents/skills/skill-generation/SKILL.md.agents/skills/template-library/SKILL.md.aspens.json.claude/hooks/graph-context-prompt.sh.claude/skills/agent-customization/skill.md.claude/skills/base/skill.md.claude/skills/claude-runner/skill.md.claude/skills/codex-support/skill.md.claude/skills/doc-sync/skill.md.claude/skills/import-graph/skill.md.claude/skills/repo-scanning/skill.md.claude/skills/skill-generation/skill.md.claude/skills/skill-rules.json.claude/skills/template-library/skill.mdAGENTS.mdCLAUDE.mdREADME.mdbin/cli.jsdocs/target-support.mdsrc/commands/add.jssrc/commands/customize.jssrc/commands/doc-graph.jssrc/commands/doc-init.jssrc/commands/doc-sync.jssrc/lib/backend.jssrc/lib/context-builder.jssrc/lib/git-hook.jssrc/lib/graph-persistence.jssrc/lib/runner.jssrc/lib/scanner.jssrc/lib/skill-reader.jssrc/lib/skill-writer.jssrc/lib/target-transform.jssrc/lib/target.jssrc/prompts/doc-init-claudemd.mdsrc/prompts/doc-init-domain.mdsrc/prompts/doc-init.mdsrc/prompts/doc-sync-refresh.mdsrc/prompts/doc-sync.mdsrc/prompts/partials/examples.mdsrc/prompts/partials/guideline-format.mdsrc/prompts/partials/skill-format.mdtests/backend.test.jstests/parse-file-output.test.jstests/prompt-loader.test.jstests/target-transform.test.jstests/target.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/commands/doc-init.js (1)
86-91: Module-level mutable state is a maintenance risk.The
_backendId,_primaryTarget,_allowedPaths,_repoPath, and_reuseSourceTargetvariables create implicit coupling across functions. Consider passing these as parameters or bundling into a context object for testability.🤖 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 86 - 91, The module-level mutable variables (_backendId, _primaryTarget, _allowedPaths, _repoPath, _reuseSourceTarget) introduce implicit coupling; refactor by creating a single context object (e.g., docInitContext) that holds those properties and pass it explicitly into functions that currently read/write them (notably docInitCommand and runLLM and any helper functions referenced in this file). Update signatures to accept the context parameter, replace direct references to the top-level variables with context.property access, and initialize the context inside docInitCommand before invoking downstream functions to preserve behavior and improve testability.src/lib/target.js (2)
204-219:loadConfigauto-persists inferred config by default.When recovering config from artifacts, the function writes
.aspens.jsonunlesspersist: falseis passed. This could surprise users who only wanted to read config. Consider documenting this side effect in the JSDoc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/target.js` around lines 204 - 219, The loadConfig function auto-persists an inferred config by calling writeConfig(repoPath, inferred) unless options.persist is explicitly false; update the JSDoc for loadConfig to clearly state this side effect (that when readConfig returns null and inferConfig produces a config, the function will write .aspens.json unless options.persist === false) and reference the behavior involving readConfig, inferConfig, and writeConfig so callers know to pass { persist: false } when they only want to read without writing.
8-9:mkdirSyncis imported but never used.The import includes
mkdirSyncbutwriteConfigwrites directly to the repo root (.aspens.json), which doesn't require directory creation. This is harmless dead code but could be cleaned up.Remove unused import
-import { existsSync, readFileSync, writeFileSync, mkdirSync } from 'fs'; +import { existsSync, readFileSync, writeFileSync } from 'fs';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/target.js` around lines 8 - 9, Remove the unused mkdirSync import from the top-level import list in src/lib/target.js; update the import statement that currently imports existsSync, readFileSync, writeFileSync, mkdirSync from 'fs' to only import the used symbols (existsSync, readFileSync, writeFileSync) so there is no dead/unused import; you can leave writeConfig and other functions (e.g., writeConfig, any uses of join/dirname) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/add.js`:
- Line 8: The file defines a duplicate runLLM implementation that duplicates
logic already exported from src/lib/runner.js; update the import list in
src/commands/add.js to include runLLM from '../lib/runner.js' (alongside
runClaude, runCodex, loadPrompt, parseFileOutput) and then remove the local
runLLM function declaration (the duplicate) so the module uses the shared runLLM
export; ensure any references in add.js call the imported runLLM.
---
Nitpick comments:
In `@src/commands/doc-init.js`:
- Around line 86-91: The module-level mutable variables (_backendId,
_primaryTarget, _allowedPaths, _repoPath, _reuseSourceTarget) introduce implicit
coupling; refactor by creating a single context object (e.g., docInitContext)
that holds those properties and pass it explicitly into functions that currently
read/write them (notably docInitCommand and runLLM and any helper functions
referenced in this file). Update signatures to accept the context parameter,
replace direct references to the top-level variables with context.property
access, and initialize the context inside docInitCommand before invoking
downstream functions to preserve behavior and improve testability.
In `@src/lib/target.js`:
- Around line 204-219: The loadConfig function auto-persists an inferred config
by calling writeConfig(repoPath, inferred) unless options.persist is explicitly
false; update the JSDoc for loadConfig to clearly state this side effect (that
when readConfig returns null and inferConfig produces a config, the function
will write .aspens.json unless options.persist === false) and reference the
behavior involving readConfig, inferConfig, and writeConfig so callers know to
pass { persist: false } when they only want to read without writing.
- Around line 8-9: Remove the unused mkdirSync import from the top-level import
list in src/lib/target.js; update the import statement that currently imports
existsSync, readFileSync, writeFileSync, mkdirSync from 'fs' to only import the
used symbols (existsSync, readFileSync, writeFileSync) so there is no
dead/unused import; you can leave writeConfig and other functions (e.g.,
writeConfig, any uses of join/dirname) unchanged.
🪄 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: 0febfbd5-77f0-4c72-b737-ca947c9037cb
📒 Files selected for processing (19)
.aspens.json.claude/hooks/graph-context-prompt.shREADME.mddocs/target-support.mdsrc/commands/add.jssrc/commands/customize.jssrc/commands/doc-init.jssrc/commands/doc-sync.jssrc/lib/context-builder.jssrc/lib/graph-persistence.jssrc/lib/runner.jssrc/lib/skill-reader.jssrc/lib/skill-writer.jssrc/lib/target-transform.jssrc/lib/target.jssrc/prompts/doc-sync-refresh.mdsrc/prompts/doc-sync.mdtests/backend.test.jstests/target.test.js
✅ Files skipped from review due to trivial changes (5)
- .aspens.json
- src/prompts/doc-sync-refresh.md
- .claude/hooks/graph-context-prompt.sh
- tests/backend.test.js
- docs/target-support.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/commands/customize.js
- src/prompts/doc-sync.md
- src/lib/graph-persistence.js
- src/lib/context-builder.js
- src/lib/skill-writer.js
- tests/target.test.js
- src/lib/target-transform.js
- src/commands/doc-sync.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/target.js (1)
180-183: SimplifyhasCodexArtifactsto the effective condition.Current logic is equivalent to
hasCodexConfig || hasCodexSkills. Keeping the minimal expression makes intent clearer and avoids future drift.Proposed simplification
- const hasCodexArtifacts = - hasCodexConfig || - hasCodexSkills || - (hasCodexInstructions && (hasCodexConfig || hasCodexSkills)); + const hasCodexArtifacts = hasCodexConfig || hasCodexSkills;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/target.js` around lines 180 - 183, The expression assigned to hasCodexArtifacts is redundant: simplify its definition to the effective condition hasCodexConfig || hasCodexSkills by removing the unnecessary (hasCodexInstructions && (hasCodexConfig || hasCodexSkills)) term; update the initializer for hasCodexArtifacts accordingly (keeping the same variable name) and run tests to ensure no behavioral change.
🤖 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/target.js`:
- Around line 141-149: readConfig currently parses and returns any JSON from
CONFIG_FILE without validation; update readConfig (and the similar logic around
the other occurrence at the second block) to validate the parsed object against
the expected schema before returning: ensure top-level shape and types (e.g.,
that "targets" is an array when present, required keys exist, and other fields
have correct types), and if validation fails return null so loadConfig can fall
back to inference; implement this by adding a lightweight schema check inside
readConfig (use explicit type checks or a small validator function referenced
from readConfig) and reject malformed-but-parseable configs.
---
Nitpick comments:
In `@src/lib/target.js`:
- Around line 180-183: The expression assigned to hasCodexArtifacts is
redundant: simplify its definition to the effective condition hasCodexConfig ||
hasCodexSkills by removing the unnecessary (hasCodexInstructions &&
(hasCodexConfig || hasCodexSkills)) term; update the initializer for
hasCodexArtifacts accordingly (keeping the same variable name) and run tests to
ensure no behavioral change.
🪄 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: 65387ffb-5fce-433b-a038-f465630ae191
📒 Files selected for processing (12)
.agents/skills/architecture/references/code-map.md.agents/skills/claude-runner/SKILL.md.agents/skills/codex-support/SKILL.md.agents/skills/doc-sync/SKILL.md.agents/skills/skill-generation/SKILL.md.claude/skills/claude-runner/skill.md.claude/skills/codex-support/skill.md.claude/skills/doc-sync/skill.md.claude/skills/skill-generation/skill.mdAGENTS.mdsrc/commands/add.jssrc/lib/target.js
✅ Files skipped from review due to trivial changes (7)
- .agents/skills/architecture/references/code-map.md
- AGENTS.md
- .agents/skills/codex-support/SKILL.md
- .agents/skills/doc-sync/SKILL.md
- .agents/skills/skill-generation/SKILL.md
- .agents/skills/claude-runner/SKILL.md
- .claude/skills/doc-sync/skill.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/skills/codex-support/skill.md
- src/commands/add.js
- .claude/skills/claude-runner/skill.md
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
Tests
Chores