fix(hooks): complete hook system fix — 8 bugs resolved#8
Open
fix(hooks): complete hook system fix — 8 bugs resolved#8
Conversation
…l.json The installer generated platform-specific hook commands that caused issues: - Windows: absolute paths with escaped backslashes (machine-dependent, fragile) - Unix: $CLAUDE_PROJECT_DIR variable (known bugs GH #6023/#5814) Both approaches led to UserPromptSubmit hook errors in installed projects. Changes: - Use relative path `node .claude/hooks/<file>` on all platforms - Remove hardcoded `timeout: 10` from HOOK_EVENT_MAP and generated settings (Claude Code manages hook timeouts natively, each hook has internal safety) - Remove unused `isWindows` and `hookFilePath` variables - Update tests to reflect timeout removal The HOOK_EVENT_MAP event routing (MIS-3.1) remains unchanged — precompact is correctly mapped to PreCompact, not UserPromptSubmit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- settings.json: remove precompact from UserPromptSubmit (wrong event type), remove code-intel-pretool.cjs (file doesn't exist), use relative paths, remove timeout override - hook-runtime.js: add hookEventName to buildHookOutput (required by Claude Code), wire createSession() for session persistence, add cleanOrphanTmpFiles() - synapse-engine.cjs: remove process.exit() (kills stdout pipe on Windows), let Node exit naturally - precompact-session-digest.cjs: fix runner path to node_modules/aios-core/ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThis PR refactors hook execution infrastructure by removing explicit timeout configurations across hook settings, eliminating Windows-specific path handling in favor of uniform relative paths, adding session management and orphaned file cleanup to hook runtime, and exposing previously internal synapse-engine APIs for external use. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aios-core/core/synapse/runtime/hook-runtime.js:
- Around line 16-27: The cleanOrphanTmpFiles function is too broad (it unlinks
any filename containing ".tmp.") — narrow it to the atomic-write leftover
pattern and add an age check to avoid racing with active writers: in
cleanOrphanTmpFiles(sessionsDir) filter files using a strict regex like
/\.json\.tmp\.\d+$/ (or the exact session filename base your code uses) and
before unlinking check file age via fs.statSync(path).mtime (e.g., only unlink
if now - mtime > thresholdMillis, e.g. 60_000) and keep the existing try/catch
around unlinkSync to skip locked/permission files; return the removed count as
before and preserve existing top-level try/catch for session dir reads.
- Around line 140-141: The hook output schema was changed to include
hookEventName in the hookSpecificOutput object; update the JSDoc near the
hookSpecificOutput declaration (around the existing comment at line ~135) to
document the new hookEventName:string property, and update the unit test in
tests/synapse/hook-entry.test.js (the assertion at ~line 285) to expect the new
key set (e.g., ['hookEventName','additionalContext']) instead of only
['additionalContext'] so the test matches the new schema; ensure the change
keeps the metadata shape stable for consumers of hookSpecificOutput.
In `@packages/installer/src/wizard/ide-config-generator.js`:
- Around line 791-794: Validate and sanitize hookFileName before interpolating
it into hookCommand: ensure hookFileName contains only a safe whitelist of
characters (e.g. letters, digits, dot, underscore, dash), reject or skip any
names with slashes, shell metacharacters, or null bytes, and fail/omit
registration if validation fails; update the code that constructs hookCommand
(the const hookCommand = `node .claude/hooks/${hookFileName}`) to only use a
validated value or to bail with a clear error, referencing the hookFileName
variable and the hookCommand construction point so no untrusted filename is
directly interpolated into shell commands.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 488a762f-9439-4799-a68a-f52f2c69b36a
📒 Files selected for processing (6)
.aios-core/core/synapse/runtime/hook-runtime.js.claude/hooks/precompact-session-digest.cjs.claude/hooks/synapse-engine.cjs.claude/settings.jsonpackages/installer/src/wizard/ide-config-generator.jspackages/installer/tests/unit/artifact-copy-pipeline/artifact-copy-pipeline.test.js
Comment on lines
+16
to
+27
| function cleanOrphanTmpFiles(sessionsDir) { | ||
| try { | ||
| const files = fs.readdirSync(sessionsDir); | ||
| let removed = 0; | ||
| for (const f of files) { | ||
| if (f.includes('.tmp.')) { | ||
| try { fs.unlinkSync(path.join(sessionsDir, f)); removed++; } | ||
| catch (_) { /* locked/permissions — skip */ } | ||
| } | ||
| } | ||
| return removed; | ||
| } catch (_) { return 0; } |
There was a problem hiding this comment.
Narrow orphan tmp cleanup to the atomic-write filename pattern.
Line 21 currently matches any filename containing .tmp.. In core session storage, this can delete non-orphan files if a valid session filename contains that substring, and it can also race with active atomic writes from another process. Match only true atomic-write leftovers (e.g. *.json.tmp.<pid>) and optionally apply an age threshold before unlinking.
💡 Proposed hardening
function cleanOrphanTmpFiles(sessionsDir) {
try {
- const files = fs.readdirSync(sessionsDir);
+ const files = fs.readdirSync(sessionsDir, { withFileTypes: true });
let removed = 0;
- for (const f of files) {
- if (f.includes('.tmp.')) {
- try { fs.unlinkSync(path.join(sessionsDir, f)); removed++; }
+ const now = Date.now();
+ for (const entry of files) {
+ if (!entry.isFile()) continue;
+ const f = entry.name;
+ if (!/\.json\.tmp\.\d+$/.test(f)) continue;
+ const filePath = path.join(sessionsDir, f);
+ const ageMs = now - fs.statSync(filePath).mtimeMs;
+ if (ageMs < 60_000) continue; // avoid deleting in-flight temp files
+ try { fs.unlinkSync(filePath); removed++; }
catch (_) { /* locked/permissions — skip */ }
- }
}
return removed;
} catch (_) { return 0; }
}As per coding guidelines, ".aios-core/core/**: Check for race conditions in orchestration modules (lock-manager, session-state)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aios-core/core/synapse/runtime/hook-runtime.js around lines 16 - 27, The
cleanOrphanTmpFiles function is too broad (it unlinks any filename containing
".tmp.") — narrow it to the atomic-write leftover pattern and add an age check
to avoid racing with active writers: in cleanOrphanTmpFiles(sessionsDir) filter
files using a strict regex like /\.json\.tmp\.\d+$/ (or the exact session
filename base your code uses) and before unlinking check file age via
fs.statSync(path).mtime (e.g., only unlink if now - mtime > thresholdMillis,
e.g. 60_000) and keep the existing try/catch around unlinkSync to skip
locked/permission files; return the removed count as before and preserve
existing top-level try/catch for session dir reads.
Comment on lines
+140
to
141
| hookEventName: 'UserPromptSubmit', | ||
| additionalContext: xml || '', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Run the provided verification script to find strict schema assertions
rg -n -C2 "Object\.keys\(output\.hookSpecificOutput\)\.toEqual\(\['additionalContext'\]\)" --type=jsRepository: riaworks/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Also search for broader patterns of schema validation on hookSpecificOutput
rg -n "hookSpecificOutput" --type=js -A 2 -B 2 | head -100Repository: riaworks/aios-core
Length of output: 6811
🏁 Script executed:
#!/bin/bash
# Check the context of the change in hook-runtime.js
fd "hook-runtime.js" --type fRepository: riaworks/aios-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
# View the hook-runtime.js file around the change
fd "hook-runtime.js" -path "*/synapse/runtime/*" --type fRepository: riaworks/aios-core
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Read the hook-runtime.js file to see context around lines 140-141
cat -n .aios-core/core/synapse/runtime/hook-runtime.js | sed -n '130,150p'Repository: riaworks/aios-core
Length of output: 563
🏁 Script executed:
#!/bin/bash
# Check the specific test that has the strict check - is it updated?
cat -n tests/synapse/hook-entry.test.js | sed -n '275,290p'Repository: riaworks/aios-core
Length of output: 856
Breaking schema change requires test updates in the same changeset.
hookEventName was added to hookSpecificOutput (line 140), but the test at tests/synapse/hook-entry.test.js:285 explicitly checks for strict key equality with only ['additionalContext'] and will fail. Additionally, the JSDoc at line 135 documents the old schema. Update the test expectation and JSDoc to reflect the new schema structure alongside this change.
This violates the coding guideline requirement: "Ensure backwards compatibility — core modules are consumed by all agents."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aios-core/core/synapse/runtime/hook-runtime.js around lines 140 - 141, The
hook output schema was changed to include hookEventName in the
hookSpecificOutput object; update the JSDoc near the hookSpecificOutput
declaration (around the existing comment at line ~135) to document the new
hookEventName:string property, and update the unit test in
tests/synapse/hook-entry.test.js (the assertion at ~line 285) to expect the new
key set (e.g., ['hookEventName','additionalContext']) instead of only
['additionalContext'] so the test matches the new schema; ensure the change
keeps the metadata shape stable for consumers of hookSpecificOutput.
Comment on lines
+791
to
+794
| // Use relative path — works on all platforms, no $CLAUDE_PROJECT_DIR bugs | ||
| // (GH #6023/#5814), no fragile absolute Windows paths with escaped backslashes. | ||
| // Claude Code resolves relative paths from the project root (cwd). | ||
| const hookCommand = `node .claude/hooks/${hookFileName}`; |
There was a problem hiding this comment.
Sanitize hook filenames before building shell command strings.
Line 794 interpolates hookFileName directly into a command. Since filenames are read from disk, a crafted .cjs name with shell metacharacters could inject commands. Restrict names to a safe pattern before registration.
🔒 Proposed fix
for (const hookFileName of hookFiles) {
+ if (!/^[a-z0-9][a-z0-9.-]*\.cjs$/i.test(hookFileName)) {
+ continue;
+ }
const hookConfig = HOOK_EVENT_MAP[hookFileName] || DEFAULT_HOOK_CONFIG;
const eventName = hookConfig.event;
@@
- const hookCommand = `node .claude/hooks/${hookFileName}`;
+ const hookCommand = `node ".claude/hooks/${hookFileName}"`;As per coding guidelines, "**/*.js: Look for potential security vulnerabilities."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/installer/src/wizard/ide-config-generator.js` around lines 791 -
794, Validate and sanitize hookFileName before interpolating it into
hookCommand: ensure hookFileName contains only a safe whitelist of characters
(e.g. letters, digits, dot, underscore, dash), reject or skip any names with
slashes, shell metacharacters, or null bytes, and fail/omit registration if
validation fails; update the code that constructs hookCommand (the const
hookCommand = `node .claude/hooks/${hookFileName}`) to only use a validated
value or to bail with a clear error, referencing the hookFileName variable and
the hookCommand construction point so no untrusted filename is directly
interpolated into shell commands.
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.
Summary
Complete fix for the AIOS hook system on Windows. Resolves 8 interconnected bugs that caused
UserPromptSubmit hook erroron every prompt.Bugs Fixed
precompact-session-digest.cjsregistered as UserPromptSubmit (wrong event)hookEventNamein JSON outputhookEventName: 'UserPromptSubmit'tobuildHookOutput()process.exit(0)kills pipe before flushprocess.exit(), let Node exit naturallycreateSession()never called in hook flowcreateSession()whenloadSession()returns null.aios-core/hooks/but runner is innode_modules/aios-core/.aios-core/hooks/node_modules/aios-core/to path$CLAUDE_PROJECT_DIRfails on Windowsnode .claude/hooks/synapse-engine.cjstimeout: 10kills hook prematurelycode-intel-pretool.cjsreferenced but missingFiles Changed
.claude/settings.json— clean hook registration (only synapse-engine in UserPromptSubmit).claude/hooks/synapse-engine.cjs— remove process.exit(), let Node exit naturally.claude/hooks/precompact-session-digest.cjs— fix runner path to node_modules/aios-core/.aios-core/core/synapse/runtime/hook-runtime.js— add hookEventName, wire createSession(), add orphan .tmp cleanupTest plan
.synapse/sessions/UserPromptSubmit hook erroron fresh Claude Code session🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor