Merged
Conversation
Node's spawn() can't locate .cmd wrappers without shell: true. Scoped to win32 only. Closes #25
Allows regenerating specific domains without touching the base skill or CLAUDE.md. Skips discovery and shows actionable retry hint when domains fail.
WalkthroughRefines docs and Node requirement, hardens optional-chaining and graph handling, adds domain-filtered chunked generation with discovery-skipping, improves error logging and Windows process handling, and adds timeouts/robustness to hook scripts and tests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/doc-init.js`:
- Around line 874-875: The retry hints currently print commands that default to
the current working directory; update the two console.log lines that reference
skippedDomains so they include the actual target repository path used by the
command invocation (the variable in this file that holds the target repo, e.g.
targetPath/targetDir/repoDir) instead of assuming cwd; modify both occurrences
that build the strings for "aspens doc init --mode chunked --domains
\"${skippedDomains.join(',')}\"" and "aspens doc init --mode chunked --timeout
600" to interpolate the repository path variable (the same variable used earlier
in this module when resolving the target repo) so the suggested retry commands
point to the original target.
- Around line 127-128: The code conflates two different modes by using
skipDiscovery for both base-only and retry/domains flows, causing the
base-generation path and existing-docs prompt to be skipped; split this into two
explicit booleans (e.g., isBaseOnly = options.mode === 'base-only' and
isDomainsOnly = options.mode === 'chunked' && extraDomains &&
extraDomains.length > 0 or isRetryOnly) and update all uses: replace
skipDiscovery in the repoGraph check and the later domainsOnly logic with the
appropriate boolean (use isBaseOnly where you want the base-only behavior and
isDomainsOnly/isRetryOnly where you want to skip discovery/retry-only behavior)
so that base-only runs still generate base files and trigger the existing-docs
strategy prompt.
In `@src/lib/runner.js`:
- Around line 53-56: The timeout cleanup currently calls child.kill('SIGTERM')
but on Windows (where spawn is created with shell: process.platform === 'win32')
that only terminates the shell (cmd.exe) and leaves the Claude process tree
orphaned; modify the timeout handler that references the spawned child (the
variable child from spawn(...)) to run platform-specific cleanup: if
process.platform === 'win32' invoke a child process to run `taskkill /pid
<child.pid> /t /f` to kill the entire process tree, otherwise call
child.kill('SIGTERM') as before; ensure you handle errors from taskkill and only
attempt taskkill when child.pid exists.
In `@src/templates/hooks/graph-context-prompt.sh`:
- Around line 53-54: Remove the external GNU `timeout 5s` call from the shell
hook and implement an internal, portable timeout inside
graph-context-prompt.mjs: in graph-context-prompt.mjs set a Node.js timer
(setTimeout) that sends SIGTERM to process after the desired timeout, clear the
timer (clearTimeout) on normal completion or error, and ensure the module reads
from stdin and exits with appropriate codes; keep the hook invocation as a
simple NODE_NO_WARNINGS=1 node graph-context-prompt.mjs >"$STDOUT_FILE"
2>"$STDERR_FILE" so macOS works without coreutils.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e44f30f-b450-4560-920d-036be65947a5
📒 Files selected for processing (7)
CLAUDE.mdsrc/commands/doc-graph.jssrc/commands/doc-init.jssrc/commands/doc-sync.jssrc/lib/runner.jssrc/templates/hooks/graph-context-prompt.shtests/graph-persistence.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/doc-init.js`:
- Around line 875-876: The printed retry commands in src/commands/doc-init.js
use the variable repoPath unquoted which will break when the path contains
spaces; update the two console.log calls that use pc.dim (the lines referencing
skippedDomains.join(',') and the timeout variant) to wrap repoPath in quotes
(use "${repoPath}") so the generated shell commands are robust for paths with
spaces; keep the rest of the string intact and ensure you preserve the existing
interpolation for skippedDomains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5dd338d-1fde-46e0-bb22-c624285c4a79
📒 Files selected for processing (4)
src/commands/doc-init.jssrc/lib/runner.jssrc/templates/hooks/graph-context-prompt.mjssrc/templates/hooks/graph-context-prompt.sh
✅ Files skipped from review due to trivial changes (1)
- src/templates/hooks/graph-context-prompt.sh
Comment on lines
+875
to
+876
| console.log(pc.dim(` Retry just these: aspens doc init --mode chunked --domains "${skippedDomains.join(',')}" ${repoPath}`)); | ||
| console.log(pc.dim(` Or retry all: aspens doc init --mode chunked --timeout 600 ${repoPath}`)); |
There was a problem hiding this comment.
Quote the repo path to handle spaces.
If repoPath contains spaces, these commands will fail. Wrap it in quotes for robustness.
Proposed fix
- console.log(pc.dim(` Retry just these: aspens doc init --mode chunked --domains "${skippedDomains.join(',')}" ${repoPath}`));
- console.log(pc.dim(` Or retry all: aspens doc init --mode chunked --timeout 600 ${repoPath}`));
+ console.log(pc.dim(` Retry just these: aspens doc init --mode chunked --domains "${skippedDomains.join(',')}" "${repoPath}"`));
+ console.log(pc.dim(` Or retry all: aspens doc init --mode chunked --timeout 600 "${repoPath}"`));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log(pc.dim(` Retry just these: aspens doc init --mode chunked --domains "${skippedDomains.join(',')}" ${repoPath}`)); | |
| console.log(pc.dim(` Or retry all: aspens doc init --mode chunked --timeout 600 ${repoPath}`)); | |
| console.log(pc.dim(` Retry just these: aspens doc init --mode chunked --domains "${skippedDomains.join(',')}" "${repoPath}"`)); | |
| console.log(pc.dim(` Or retry all: aspens doc init --mode chunked --timeout 600 "${repoPath}"`)); |
🤖 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 875 - 876, The printed retry commands
in src/commands/doc-init.js use the variable repoPath unquoted which will break
when the path contains spaces; update the two console.log calls that use pc.dim
(the lines referencing skippedDomains.join(',') and the timeout variant) to wrap
repoPath in quotes (use "${repoPath}") so the generated shell commands are
robust for paths with spaces; keep the rest of the string intact and ensure you
preserve the existing interpolation for skippedDomains.
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
Bug Fixes
Documentation
Chores