fix(hooks): complete hook system fix — 8 bugs resolved#551
fix(hooks): complete hook system fix — 8 bugs resolved#551riaworks wants to merge 2 commits intoSynkraAI:mainfrom
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>
|
@riaworks is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request refactors hook registration and execution by removing timeout-based process exit handling, simplifying hook paths to relative resolution, adding session creation fallback logic, and implementing orphaned temporary file cleanup on first prompt. Hook configurations are updated across installer, settings, and tests to reflect these changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.aios-core/core/synapse/runtime/hook-runtime.js (1)
146-149:⚠️ Potential issue | 🟠 MajorCI blocker: regenerate manifest for this core file change.
Pipeline reports
core/synapse/runtime/hook-runtime.jshash mismatch. Please runnpm run generate:manifestand commit updated manifest artifacts.🤖 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 146 - 149, CI reports a hash mismatch for core/synapse/runtime/hook-runtime.js after the change exporting resolveHookRuntime and buildHookOutput; run the manifest regeneration step (npm run generate:manifest), verify the generated manifest artifacts include the updated hash for this file, and commit the updated manifest files (and any related generated artifacts) so the pipeline picks up the new core/synapse/runtime/hook-runtime.js export changes..claude/hooks/precompact-session-digest.cjs (1)
58-67:⚠️ Potential issue | 🔴 CriticalThe hardcoded runner path is incorrect and will fail at runtime; fix to actual location and add error handling.
The path constructed in lines 58–67 points to
node_modules/aios-core/.aios-core/hooks/unified/runners/precompact-runner.js, but that location does not exist in this repository. The actual runner is located at.aios-core/hooks/unified/runners/precompact-runner.jsat the project root. The unguardedrequire(runnerPath)on line 81 will throw immediately when this hook runs, breaking the entire precompaction flow.Update the path to the correct location and guard the require with try-catch or early-exit to prevent the hook from blocking critical operations:
Recommended fix
const path = require('path'); +const fs = require('fs'); @@ const runnerPath = path.join( PROJECT_ROOT, - 'node_modules', - 'aios-core', '.aios-core', 'hooks', 'unified', 'runners', 'precompact-runner.js', ); + + if (!fs.existsSync(runnerPath)) { + console.warn(`Precompact runner not found at ${runnerPath}; skipping hook`); + return; + } @@ - const { onPreCompact } = require(runnerPath); - await onPreCompact(context); + try { + const { onPreCompact } = require(runnerPath); + if (typeof onPreCompact === 'function') { + await onPreCompact(context); + } + } catch (error) { + console.error(`Failed to execute precompact hook: ${error.message}`); + // Continue without precompaction rather than block the flow + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/precompact-session-digest.cjs around lines 58 - 67, The constructed runnerPath currently points to node_modules/... and will not exist; update the path construction for runnerPath to point to the root-level .aios-core/hooks/unified/runners/precompact-runner.js (use PROJECT_ROOT joined with '.aios-core','hooks','unified','runners','precompact-runner.js') and wrap the dynamic load (the require(runnerPath) call) in a try-catch or guard that logs an informative warning and early-returns so the hook does not throw and block precompaction; reference and update the runnerPath constant and the require(runnerPath) usage to implement this error handling.
🧹 Nitpick comments (1)
.aios-core/core/synapse/runtime/hook-runtime.js (1)
135-141: Update output JSDoc to match the new payload shape.The return type annotation still documents only
additionalContext, but Line 140 now includeshookEventName.🤖 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 135 - 141, The JSDoc for buildHookOutput is outdated: the returned object now includes hookSpecificOutput.hookEventName as well as additionalContext; update the function's `@returns` annotation to reflect the new payload shape (e.g., return type should document hookSpecificOutput: { hookEventName: string, additionalContext: string }) so the documented contract matches the actual return value from buildHookOutput and references both hookEventName and additionalContext.
🤖 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: cleanOrphanTmpFiles currently unconditionally deletes any
filename containing ".tmp." which can remove temp files still in use by
concurrent atomic writers; update the function (cleanOrphanTmpFiles and the
similar logic used elsewhere) to only target strictly-matching temp filenames
(e.g., a regex like /\.tmp\.[^.]+$/ or the project's temp-naming convention) and
add an age threshold check using fs.statSync(path).mtime (skip files younger
than a safe window, e.g., 1–5 minutes) before unlinking; also catch and log stat
errors and continue so locked/permissioned files are skipped safely.
---
Outside diff comments:
In @.aios-core/core/synapse/runtime/hook-runtime.js:
- Around line 146-149: CI reports a hash mismatch for
core/synapse/runtime/hook-runtime.js after the change exporting
resolveHookRuntime and buildHookOutput; run the manifest regeneration step (npm
run generate:manifest), verify the generated manifest artifacts include the
updated hash for this file, and commit the updated manifest files (and any
related generated artifacts) so the pipeline picks up the new
core/synapse/runtime/hook-runtime.js export changes.
In @.claude/hooks/precompact-session-digest.cjs:
- Around line 58-67: The constructed runnerPath currently points to
node_modules/... and will not exist; update the path construction for runnerPath
to point to the root-level .aios-core/hooks/unified/runners/precompact-runner.js
(use PROJECT_ROOT joined with
'.aios-core','hooks','unified','runners','precompact-runner.js') and wrap the
dynamic load (the require(runnerPath) call) in a try-catch or guard that logs an
informative warning and early-returns so the hook does not throw and block
precompaction; reference and update the runnerPath constant and the
require(runnerPath) usage to implement this error handling.
---
Nitpick comments:
In @.aios-core/core/synapse/runtime/hook-runtime.js:
- Around line 135-141: The JSDoc for buildHookOutput is outdated: the returned
object now includes hookSpecificOutput.hookEventName as well as
additionalContext; update the function's `@returns` annotation to reflect the new
payload shape (e.g., return type should document hookSpecificOutput: {
hookEventName: string, additionalContext: string }) so the documented contract
matches the actual return value from buildHookOutput and references both
hookEventName and additionalContext.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3004a34f-9685-4bb9-bb51-ee0e4b06161b
📒 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
| 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.
Avoid deleting live .tmp files during concurrent atomic writes.
cleanOrphanTmpFiles() deletes every file containing .tmp. immediately. Concurrent hook processes can still be using fresh temp files, causing rename failures/data loss. Add an age threshold (and stricter filename match) before deletion.
🛠️ Suggested safeguard
+const ORPHAN_TMP_MIN_AGE_MS = 60 * 1000;
@@
function cleanOrphanTmpFiles(sessionsDir) {
try {
const files = fs.readdirSync(sessionsDir);
let removed = 0;
for (const f of files) {
- if (f.includes('.tmp.')) {
+ if (/\.tmp\./.test(f)) {
+ const filePath = path.join(sessionsDir, f);
try { fs.unlinkSync(path.join(sessionsDir, f)); removed++; }
catch (_) { /* locked/permissions — skip */ }
+ try {
+ const stat = fs.statSync(filePath);
+ if ((Date.now() - stat.mtimeMs) < ORPHAN_TMP_MIN_AGE_MS) continue;
+ fs.unlinkSync(filePath);
+ removed++;
+ } catch (_) { /* locked/permissions/stat failure — skip */ }
}
}
return removed;
} catch (_) { return 0; }
}As per coding guidelines ".aios-core/core/**: Check for race conditions in orchestration modules (lock-manager, session-state)."
Also applies to: 112-120
🤖 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,
cleanOrphanTmpFiles currently unconditionally deletes any filename containing
".tmp." which can remove temp files still in use by concurrent atomic writers;
update the function (cleanOrphanTmpFiles and the similar logic used elsewhere)
to only target strictly-matching temp filenames (e.g., a regex like
/\.tmp\.[^.]+$/ or the project's temp-naming convention) and add an age
threshold check using fs.statSync(path).mtime (skip files younger than a safe
window, e.g., 1–5 minutes) before unlinking; also catch and log stat errors and
continue so locked/permissioned files are skipped safely.
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Excelente diagnóstico — os 8 bugs documentados são reais e a abordagem de correção é sólida. Revisando em relação à issue #544 (tier-based hook installation) que acompanhei: as correções aqui são complementares e não conflitam com aquele escopo.
Bug 1 — precompact-session-digest.cjs no evento errado:
Correto remover do UserPromptSubmit. O hook de digestão de sessão pertence ao PreCompact — o mapeamento estava invertido.
Bug 2 — hookEventName ausente no output:
Adição de hookEventName: 'UserPromptSubmit' em buildHookOutput() é necessária conforme a spec do Claude Code. Sem isso, o Claude Code não consegue rotear a resposta corretamente.
Bug 3 — process.exit(0) cortando stdout no Windows:
A solução de deixar o Node encerrar naturalmente (via timer.unref() + then/catch vazios) é a abordagem correta. O safeExit() com check de JEST_WORKER_ID era uma gambiarra; removê-lo simplifica o código.
Bug 4 — createSession() nunca chamado:
Esse era o mais crítico. O comentário no código (BUG-FIX: Prior to this fix...) explica bem a cadeia de falha. A lógica loadSession() → null → createSession() é correta.
Bug 5 — path do runner do PreCompact:
Adicionar node_modules/aios-core/ ao path resolve o problema de resolução de módulo sem hardcodar caminhos absolutos.
Bug 6 + timeout:
Usar caminhos relativos em vez de $CLAUDE_PROJECT_DIR e remover timeout: 10 são decisões acertadas. O Claude Code gerencia timeouts nativamente (60s default).
Testes:
Os 4 testes atualizados em artifact-copy-pipeline.test.js refletem corretamente o novo comportamento (timeout indefinido). Cobertura adequada para o escopo.
Único ponto de observação:
O cleanOrphanTmpFiles() usa .includes('.tmp.') — o padrão é seguro para o caso atual, mas se algum arquivo legítimo tiver .tmp. no nome por outra razão, seria deletado silenciosamente. Considerar .endsWith('.tmp') ou um padrão regex mais específico em iteração futura.
PR bem estruturado, bugs bem documentados e correções pontuais sem over-engineering. LGTM.
2 participants
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
Bug Fixes
Chores