fix: resolve ConfigCache.has() stat inflation and hook system errors#582
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
area: agents
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughConfigCache.has() now checks presence and TTL inline without calling get(); hook runtime will create a session if loadSession returns null; precompact hook resolves runner path and spawns a detached child process (or exits silently if missing); synapse-engine introduces a run() entrypoint and uses exitCode/timeouts; manifest hashes/timestamp updated. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (2)
.aiox-core/core/config/config-cache.js (1)
74-85: Extract the expiry/eviction check into one helper.
get(),has(), andclearExpired()now all carry their own TTL logic here, and the same behavior is mirrored again in.aiox-core/infrastructure/scripts/config-cache.js. Centralizing the “is entry still valid, otherwise evict it” path would reduce drift on this core module.♻️ Possible refactor
+ _hasLiveEntry(key, now = Date.now()) { + if (!this.cache.has(key)) { + return false; + } + + const timestamp = this.timestamps.get(key); + if (now - timestamp > this.ttl) { + this.cache.delete(key); + this.timestamps.delete(key); + return false; + } + + return true; + } + get(key) { - if (!this.cache.has(key)) { + if (!this._hasLiveEntry(key)) { this.misses++; return null; } - - const timestamp = this.timestamps.get(key); - const now = Date.now(); - - // Check if expired - if (now - timestamp > this.ttl) { - this.cache.delete(key); - this.timestamps.delete(key); - this.misses++; - return null; - } this.hits++; return this.cache.get(key); } has(key) { - if (!this.cache.has(key)) { - return false; - } - const timestamp = this.timestamps.get(key); - if (Date.now() - timestamp > this.ttl) { - this.cache.delete(key); - this.timestamps.delete(key); - return false; - } - return true; + return this._hasLiveEntry(key); }As per coding guidelines, "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 @.aiox-core/core/config/config-cache.js around lines 74 - 85, The TTL/eviction logic is duplicated across has(), get(), and clearExpired() (and mirrored in the script version); extract that logic into a single helper method (e.g., isExpiredAndEvict(key) or ensureNotExpired(key)) that checks timestamps.get(key) vs Date.now(), deletes from this.cache and this.timestamps when expired, and returns a boolean indicating validity; update has(), get(), and clearExpired() to call this helper so behavior remains identical and keep method name/signature stable to preserve backwards compatibility.tests/synapse/hook-entry.test.js (1)
337-343: Please cover the successfulcreateSession()path too.These assertions only exercise
loadSession() === nullpluscreateSession() === null, so the new happy path inresolveHookRuntime()is still untested. Add one case wherecreateSession()returns a real session object and mirror it in the in-process variant below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/synapse/hook-entry.test.js` around lines 337 - 343, Add a new test case (or extend the existing one) so that when loadSession() returns null but createSession() returns a real session object, resolveHookRuntime() follows the happy path and returns/uses that session; update both the existing in-process test and its in-process variant to mock createSession() to return a real session object (e.g., an object with prompt_count and any other expected fields) and add assertions that the resolved session equals that object and that any derived values (such as prompt_count) match; target the functions loadSession, createSession, and resolveHookRuntime in tests/synapse/hook-entry.test.js to ensure coverage of the successful createSession path.
🤖 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/hooks/precompact-session-digest.cjs:
- Around line 73-75: The hook currently calls resolveRunnerPath() then executes
the runner which uses setImmediate(async => ...) in-process, keeping the event
loop alive; instead spawn a detached child process to run
.aiox-core/hooks/unified/runners/precompact-runner.js so the digest is
fire-and-forget. Modify the code that uses resolveRunnerPath() / runnerPath to
fork or spawn a detached process (child_process.fork or spawn) with the runner
path as the module/entry, set detached: true, stdio: 'ignore', and call
child.unref() so the parent can exit immediately; ensure any args or env needed
by the runner are forwarded and preserve the existing early return when
!runnerPath.
In @.claude/hooks/synapse-engine.cjs:
- Around line 90-94: The timeout callback in run() currently sets
process.exitCode = 0 which doesn't force termination; change the callback to
call process.exit(0) so the 5s limit is enforced (i.e., replace the body of the
setTimeout in run() to invoke process.exit(0)); keep the existing timer handling
(including timer.unref() if present) but ensure the timeout uses process.exit(0)
to forcibly terminate when HOOK_TIMEOUT_MS elapses.
---
Nitpick comments:
In @.aiox-core/core/config/config-cache.js:
- Around line 74-85: The TTL/eviction logic is duplicated across has(), get(),
and clearExpired() (and mirrored in the script version); extract that logic into
a single helper method (e.g., isExpiredAndEvict(key) or ensureNotExpired(key))
that checks timestamps.get(key) vs Date.now(), deletes from this.cache and
this.timestamps when expired, and returns a boolean indicating validity; update
has(), get(), and clearExpired() to call this helper so behavior remains
identical and keep method name/signature stable to preserve backwards
compatibility.
In `@tests/synapse/hook-entry.test.js`:
- Around line 337-343: Add a new test case (or extend the existing one) so that
when loadSession() returns null but createSession() returns a real session
object, resolveHookRuntime() follows the happy path and returns/uses that
session; update both the existing in-process test and its in-process variant to
mock createSession() to return a real session object (e.g., an object with
prompt_count and any other expected fields) and add assertions that the resolved
session equals that object and that any derived values (such as prompt_count)
match; target the functions loadSession, createSession, and resolveHookRuntime
in tests/synapse/hook-entry.test.js to ensure coverage of the successful
createSession path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 184d4eef-e5d1-4164-8b21-7711fa2e342b
📒 Files selected for processing (7)
.aiox-core/core/config/config-cache.js.aiox-core/core/synapse/runtime/hook-runtime.js.aiox-core/infrastructure/scripts/config-cache.js.aiox-core/install-manifest.yaml.claude/hooks/precompact-session-digest.cjs.claude/hooks/synapse-engine.cjstests/synapse/hook-entry.test.js
|
Heads up @Pedrovaleriolopez — esse fix do ConfigCache.has() já está no nosso PR #498 (aberto 24/fev, há mais de 2 semanas). Mesmo bug, mesma solução. O #498 já tá rebased e pronto pra review. |
|
Hey @oalanicolas — CodeRabbit flagged 2 actionable issues + 2 nitpicks: Actionable:
Nitpick (nice-to-have): Please apply fixes and push. CI is green, just need CR approval to unblock merge. |
- precompact-session-digest.cjs: spawn detached child process instead of require()-ing runner in-process. The runner uses setImmediate internally which kept the event loop alive and caused the hook to block (wait for the 9 s safety timeout) instead of being fire-and-forget. Context is forwarded via AIOX_HOOK_CONTEXT env var to the inline Node script. - synapse-engine.cjs: replace process.exitCode = 0 with process.exit(0) in the timeout callback. process.exitCode alone does not terminate the process when active handles remain (e.g. stdout backpressure), defeating the 5 s hard limit that protects Claude Code from a blocking hook. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 @.claude/hooks/precompact-session-digest.cjs:
- Around line 93-104: The spawn call can emit an asynchronous 'error' event (and
JSON.stringify(context) can throw), so wrap the JSON.stringify(context) and
spawn(...) in a try/catch and, after obtaining the child from spawn, attach an
'error' listener on the returned ChildProcess (child.on('error', err => { /* log
or ignore */ })) to prevent unhandled exceptions; reference the inlineScript,
spawn(...), AIOX_HOOK_CONTEXT and onPreCompact usage and ensure any
serialization failure falls back to a safe/empty context before spawning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 176707ae-4e10-4b5e-b1b1-4dcab8aa1769
📒 Files selected for processing (2)
.claude/hooks/precompact-session-digest.cjs.claude/hooks/synapse-engine.cjs
Review ⚡ @devopsCode: APPROVED ✅ — Both fixes are solid:
Blocker: ❌ Merge conflict — please rebase against git fetch origin && git rebase origin/mainOnce rebased, this is ready to merge. |
…497, #528) Reimplemented has() to check Map directly without touching metrics. Applied to both core/config/ and infrastructure/scripts/ copies. runner path didn't account for node_modules layout. Added path resolution with fallback candidates. Also fixed: - synapse-engine.cjs: replaced process.exit() with process.exitCode for clean stdout flush - hook-runtime.js: create session file on first prompt when loadSession returns null (prevents silent updateSession failures) Updated tests to match new behavior (process.exitCode, createSession export). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- precompact-session-digest.cjs: spawn detached child process instead of require()-ing runner in-process. The runner uses setImmediate internally which kept the event loop alive and caused the hook to block (wait for the 9 s safety timeout) instead of being fire-and-forget. Context is forwarded via AIOX_HOOK_CONTEXT env var to the inline Node script. - synapse-engine.cjs: replace process.exitCode = 0 with process.exit(0) in the timeout callback. process.exitCode alone does not terminate the process when active handles remain (e.g. stdout backpressure), defeating the 5 s hard limit that protects Claude Code from a blocking hook. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t hook Addresses CodeRabbit review: handle spawn 'error' event and JSON.stringify failures to prevent unhandled exceptions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
oalanicolas
force-pushed
the
fix/bugfix-497-528-config-cache-hooks
branch
from
d1bb475 to
25d27ec
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
Fixes two reported bugs with correct implementations:
bug: ConfigCache.has() inflates hit/miss stats by calling get() internally #497 —
ConfigCache.has()delegated toget(), inflatinghits/missesstats on every existence check. Reimplementedhas()to check the Map directly with TTL expiration logic, without touching metrics. Applied to bothcore/config/config-cache.jsandinfrastructure/scripts/config-cache.js.bug: install registers precompact-session-digest hook in UserPromptSubmit with missing runner #528 —
precompact-session-digest.cjsfailed in installed projects because the runner path didn't account fornode_modules/layout. Added multi-path resolution with fallback. Also fixed:synapse-engine.cjs: replacedprocess.exit()withprocess.exitCodefor clean stdout flush (prevents truncated hook output)hook-runtime.js: callcreateSession()whenloadSession()returns null on first prompt (fixes silentupdateSessionfailures)Related PRs
Test plan
npm run lint— 0 errorshook-entry.test.jsupdated to match new behavior (process.exitCode, createSession mock)config-cachehas() no longer inflates stats when calledprecompact-session-digestresolves runner path in both framework-dev and installed-project modesCloses #497
Closes #528
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores