fix: port ensureMemoryDaemon to TypeScript session-start hook#157
fix: port ensureMemoryDaemon to TypeScript session-start hook#157udhaya10 wants to merge 1 commit intoparcadei:mainfrom
Conversation
|
PR author is not in the allowed authors list. |
📝 WalkthroughWalkthroughThe TypeScript session-start hook now includes memory daemon auto-start logic: it adds an ensureMemoryDaemon() function that checks PID files, searches for the daemon script in multiple locations, and spawns it via Changes
Sequence DiagramsequenceDiagram
participant Session as Session Start Hook
participant PID as PID File
participant FS as File System
participant UV as uv run
participant Daemon as memory_daemon.py
Session->>PID: Check ~/.claude/memory-daemon.pid
alt PID exists and valid
Session->>Session: Treat as running, return null/status
else PID missing or stale
Session->>FS: Search for memory_daemon.py (multiple paths)
alt Script found
Session->>UV: Spawn detached: uv run <script> start (log to file)
UV->>Daemon: Launch daemon process
Daemon->>PID: Write new PID
Daemon-->>UV: Running
UV-->>Session: Async return
Session->>Session: Return "Started" status
else Script not found
Session->>Session: Log warning, return null
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![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 @.claude/hooks/src/session-start-continuity.ts:
- Line 369: The current cwd calculation (const cwd = path.resolve(daemonScript,
'..', '..', '..')) only works for one install layout and can point to
directories without pyproject.toml/uv.lock; change it to walk up from
daemonScript (using the daemonScript path variable) until you find a directory
containing pyproject.toml or uv.lock and set cwd to that directory (use that as
the working dir for uv run), and only fall back to the three-level-up resolution
if no project marker is found; update any related logic in
session-start-continuity.ts that uses daemonScript and cwd accordingly.
- Around line 372-379: Wrap the spawn call in a try/finally so logFd is always
closed, and after creating the child attach an 'error' listener to the spawned
process to prevent an unhandled exception; specifically, when calling
spawn('uv', ['run', daemonScript, 'start'], ...) ensure you add
child.on('error', err => { /* emit/serialize the same JSON failure output and
exit gracefully */ }) before unref(), and move fs.closeSync(logFd) into a
finally block so logFd is closed whether spawn throws synchronously or not
(references: logFd, spawn('uv', ...), child, daemonScript, child.unref(),
fs.closeSync).
- Around line 637-641: The ensureMemoryDaemon() call is never reached when
main() returns early from the if (!usedHandoffLedger) branch; move the daemon
startup so it always runs: invoke ensureMemoryDaemon() immediately after
projectDir is read (before the handoff/ledger checks) and remove the duplicate
call later, or alternatively replace the unconditional return in the if
(!usedHandoffLedger) block (referenced by usedHandoffLedger and main()) with a
fallthrough mechanism (set a flag or break out) so execution continues to the
ensureMemoryDaemon() call and subsequent JSON output; ensure the unique function
ensureMemoryDaemon() is only called once.
- Line 344: Replace the raw __dirname usage (the const hookDir = __dirname; at
top-level) with an ESM-safe dirname derived from import.meta.url: import the
URL-to-path and dirname helpers (fileURLToPath from 'url' and dirname from
'path') and set hookDir = dirname(fileURLToPath(import.meta.url));
alternatively, if targeting Node ≥21 you can use import.meta.dirname directly;
ensure the new hookDir assignment replaces the existing const hookDir so no
top-level ReferenceError occurs outside the existing try/catch.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.claude/hooks/dist/session-start-continuity.mjsis excluded by!**/dist/**,!**/dist/**,!**/.claude/hooks/dist/**
📒 Files selected for processing (1)
.claude/hooks/src/session-start-continuity.ts
|
|
||
| try { | ||
| // cwd = opc/ directory (3 levels up from the script) | ||
| const cwd = path.resolve(daemonScript, '..', '..', '..'); |
There was a problem hiding this comment.
cwd derivation is correct only for path option 1; uv run may fail for wizard-installed/global paths
The comment "3 levels up from the script" is accurate for path 1 (opc/scripts/core/memory_daemon.py → opc/), which is where uv's lockfile likely lives. However for paths 2 and 3 the three-level walk resolves to .claude/ and ~/.claude/ respectively — directories that probably lack a pyproject.toml/uv.lock. Running uv run from those locations may silently fall back to a different Python environment or fail.
Consider deriving the cwd based on which path matched (e.g., always resolving to the directory that contains pyproject.toml), or documenting the expected project structure for wizard and global installs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/src/session-start-continuity.ts at line 369, The current cwd
calculation (const cwd = path.resolve(daemonScript, '..', '..', '..')) only
works for one install layout and can point to directories without
pyproject.toml/uv.lock; change it to walk up from daemonScript (using the
daemonScript path variable) until you find a directory containing pyproject.toml
or uv.lock and set cwd to that directory (use that as the working dir for uv
run), and only fall back to the three-level-up resolution if no project marker
is found; update any related logic in session-start-continuity.ts that uses
daemonScript and cwd accordingly.
| const logFd = fs.openSync(logFile, 'a'); | ||
| const child = spawn('uv', ['run', daemonScript, 'start'], { | ||
| cwd, | ||
| stdio: ['ignore', logFd, logFd], | ||
| detached: true, | ||
| }); | ||
| child.unref(); | ||
| fs.closeSync(logFd); |
There was a problem hiding this comment.
Two issues: unhandled error event crashes the hook; logFd leaks if spawn throws
1. Missing error listener (critical): If uv is not in PATH, spawn() emits an asynchronous 'error' event on the child object. Without a listener, Node.js re-throws it as an uncaught exception — this is not caught by the enclosing try/catch (which only catches synchronous throws) and not caught by main().catch() (which only handles rejected promises). The hook process crashes before writing any JSON output.
2. File-descriptor leak (major): logFd is opened before spawn() but fs.closeSync(logFd) is only reachable if spawn() succeeds (does not throw synchronously). Use a finally block.
🐛 Proposed fix
- const logFd = fs.openSync(logFile, 'a');
- const child = spawn('uv', ['run', daemonScript, 'start'], {
- cwd,
- stdio: ['ignore', logFd, logFd],
- detached: true,
- });
- child.unref();
- fs.closeSync(logFd);
+ let logFd: number | null = null;
+ try {
+ logFd = fs.openSync(logFile, 'a');
+ const child = spawn('uv', ['run', daemonScript, 'start'], {
+ cwd,
+ stdio: ['ignore', logFd, logFd],
+ detached: true,
+ });
+ child.on('error', (err) => {
+ console.error(`Warning: memory daemon spawn failed (uv not found?): ${err.message}`);
+ });
+ child.unref();
+ } finally {
+ if (logFd !== null) fs.closeSync(logFd);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/src/session-start-continuity.ts around lines 372 - 379, Wrap
the spawn call in a try/finally so logFd is always closed, and after creating
the child attach an 'error' listener to the spawned process to prevent an
unhandled exception; specifically, when calling spawn('uv', ['run',
daemonScript, 'start'], ...) ensure you add child.on('error', err => { /*
emit/serialize the same JSON failure output and exit gracefully */ }) before
unref(), and move fs.closeSync(logFd) into a finally block so logFd is closed
whether spawn throws synchronously or not (references: logFd, spawn('uv', ...),
child, daemonScript, child.unref(), fs.closeSync).
| // Ensure memory daemon is running (auto-extracts learnings from ended sessions) | ||
| const daemonStatus = ensureMemoryDaemon(); | ||
| if (daemonStatus) { | ||
| console.error(`✓ ${daemonStatus}`); | ||
| } |
There was a problem hiding this comment.
Daemon is never started for new projects — early return at line 517 bypasses this call
The if (!usedHandoffLedger) branch at line 512 contains an unconditional return at line 517 when thoughts/ledgers does not exist (labelled "normal for new projects"). That return exits main() before reaching line 638, so ensureMemoryDaemon() is never invoked for any project without both a handoff ledger and a legacy ledger directory. This directly defeats the PR's stated goal of auto-starting the daemon on every session start.
Move the ensureMemoryDaemon() invocation to before the handoff/ledger conditional blocks (e.g., immediately after reading projectDir), or replace the early return at line 517 with a break/flag and fall through to the daemon call and JSON output.
🐛 Proposed fix — hoist the daemon call
async function main() {
const input: SessionStartInput = JSON.parse(await readStdin());
const projectDir = process.env.CLAUDE_PROJECT_DIR || process.cwd();
const sessionType = input.source || input.type;
+ // Ensure memory daemon is running regardless of ledger state
+ const daemonStatus = ensureMemoryDaemon();
+ if (daemonStatus) {
+ console.error(`✓ ${daemonStatus}`);
+ }
+
let message = '';
...And remove the duplicate call at line 637–641.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/src/session-start-continuity.ts around lines 637 - 641, The
ensureMemoryDaemon() call is never reached when main() returns early from the if
(!usedHandoffLedger) branch; move the daemon startup so it always runs: invoke
ensureMemoryDaemon() immediately after projectDir is read (before the
handoff/ledger checks) and remove the duplicate call later, or alternatively
replace the unconditional return in the if (!usedHandoffLedger) block
(referenced by usedHandoffLedger and main()) with a fallthrough mechanism (set a
flag or break out) so execution continues to the ensureMemoryDaemon() call and
subsequent JSON output; ensure the unique function ensureMemoryDaemon() is only
called once.
The memory daemon was never auto-started because ensure_memory_daemon() only existed in the Python hook (never registered) while the registered MJS hook had zero memory/daemon logic. Port the function to session-start-continuity.ts with improvements: - Logs to ~/.claude/memory-daemon.log instead of /dev/null - Warns on stderr if daemon script not found - Uses spawn() with detached + unref for proper daemonization Fixes parcadei#156 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
udhaya10
force-pushed
the
fix/memory-daemon-autostart
branch
from
6c427a5 to
541478a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.claude/hooks/src/session-start-continuity.ts (3)
637-641:⚠️ Potential issue | 🔴 CriticalThe early
returnat line 517 still bypasses this call for new projects.When neither a handoff ledger nor a legacy
thoughts/ledgersdirectory exists (usedHandoffLedger = falseand!fs.existsSync(ledgerDir)),main()returns at line 517 and never reaches line 638. The daemon is therefore never started for brand-new projects — the PR's primary objective.Move the
ensureMemoryDaemon()call to immediately afterprojectDiris read (before the handoff/ledger branches), or replace the earlyreturnat line 517 with a flag/break so execution falls through. Remove the current placement at line 638.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/src/session-start-continuity.ts around lines 637 - 641, The call to ensureMemoryDaemon() is placed too late and is skipped when main() returns early for new projects; move the ensureMemoryDaemon() invocation to immediately after projectDir is read (so it always runs regardless of handoff/ledger branches), or instead replace the early return that checks usedHandoffLedger and fs.existsSync(ledgerDir) with a flag/flow control so execution falls through to the existing ensureMemoryDaemon() call; update references in main() and ensureMemoryDaemon() accordingly and remove the stale call at its current location.
372-379:⚠️ Potential issue | 🔴 CriticalTwo open issues from the prior review remain unaddressed: missing
errorlistener andlogFdleak.
Unhandled
errorevent (critical): Ifuvis not inPATH,spawn()emits an async'error'event. Without a listener Node.js re-throws it as an uncaught exception — this is outside the enclosingtry/catch(which only covers synchronous throws) and also outsidemain().catch(). The hook crashes without producing any JSON output.File-descriptor leak:
logFdis opened beforespawn()butfs.closeSync(logFd)is only reachable whenspawn()does not throw synchronously. A synchronous failure leaveslogFdopen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/src/session-start-continuity.ts around lines 372 - 379, The spawned-daemon block leaks the log file descriptor and lacks an 'error' listener on the child process: ensure logFd opened by fs.openSync is closed on all error paths (use try/finally or close in the catch) and add a child.on('error', ...) handler to catch async spawn failures (e.g., when spawn('uv'...) fails) so the hook does not crash; reference the fs.openSync/logFd, spawn(...)/child, fs.closeSync(logFd), and child.on('error', ...) symbols and ensure the error handler logs JSON output and closes logFd before rethrowing or exiting.
369-369:⚠️ Potential issue | 🟠 Major
cwdderivation is still incorrect for wizard/global install paths.Three-level walk from the script file only reaches
opc/for the dev layout (path 1). For paths 2 and 3 it resolves to.claude/or~/.claude/, which almost certainly lack apyproject.toml/uv.lock, causinguv runto use the wrong environment or fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/src/session-start-continuity.ts at line 369, The cwd calculation using path.resolve(daemonScript, '..', '..', '..') is brittle; instead walk upward from path.dirname(daemonScript) until you find project markers (e.g. pyproject.toml or uv.lock) and set cwd to that directory. Update the logic around the daemonScript/cwd variables in session-start-continuity.ts to loop (using fs.existsSync or fs.promises.stat) checking each parent for those files and stop at filesystem root; fall back to the previous heuristic only if no marker is found. Ensure the new cwd is used wherever the old cwd variable was referenced to run uv/run commands.
🧹 Nitpick comments (1)
.claude/hooks/src/session-start-continuity.ts (1)
344-344: PreferfileURLToPathover.pathnamefor ESM-safe path resolution.
new URL(import.meta.url).pathnamehas two known problems: (1) percent-encoded characters (e.g. spaces →%20) are not decoded, so paths containing spaces won't resolve to files on disk; (2) on Windows it produces/C:/...with a leading slash, breaking all downstreampath.resolvecalls. Using.pathnamedirectly is dangerous — spaces in filenames are encoded as%20, which the OS won't match. On Windows,new URL('file:///C:/path/').pathnamegives/C:/path/(incorrect), whilefileURLToPath('file:///C:/path/')givesC:\path\(correct).♻️ Suggested fix
Add to imports at the top:
+import { fileURLToPath } from 'url';Then replace line 344:
- const hookDir = path.dirname(new URL(import.meta.url).pathname); + const hookDir = path.dirname(fileURLToPath(import.meta.url));Or, if the target Node.js version is ≥ 20.11, use the built-in:
- const hookDir = path.dirname(new URL(import.meta.url).pathname); + const hookDir = import.meta.dirname;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/src/session-start-continuity.ts at line 344, Replace the ESM path extraction that uses new URL(import.meta.url).pathname for hookDir with a fileURLToPath-based approach: import fileURLToPath from 'url' (or use the built-in global if Node ≥20.11) and call fileURLToPath(import.meta.url) before passing to path.dirname so hookDir is correctly decoded and Windows-safe; update the reference where hookDir is defined (the const hookDir assignment) to use path.dirname(fileURLToPath(import.meta.url)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.claude/hooks/src/session-start-continuity.ts:
- Around line 637-641: The call to ensureMemoryDaemon() is placed too late and
is skipped when main() returns early for new projects; move the
ensureMemoryDaemon() invocation to immediately after projectDir is read (so it
always runs regardless of handoff/ledger branches), or instead replace the early
return that checks usedHandoffLedger and fs.existsSync(ledgerDir) with a
flag/flow control so execution falls through to the existing
ensureMemoryDaemon() call; update references in main() and ensureMemoryDaemon()
accordingly and remove the stale call at its current location.
- Around line 372-379: The spawned-daemon block leaks the log file descriptor
and lacks an 'error' listener on the child process: ensure logFd opened by
fs.openSync is closed on all error paths (use try/finally or close in the catch)
and add a child.on('error', ...) handler to catch async spawn failures (e.g.,
when spawn('uv'...) fails) so the hook does not crash; reference the
fs.openSync/logFd, spawn(...)/child, fs.closeSync(logFd), and child.on('error',
...) symbols and ensure the error handler logs JSON output and closes logFd
before rethrowing or exiting.
- Line 369: The cwd calculation using path.resolve(daemonScript, '..', '..',
'..') is brittle; instead walk upward from path.dirname(daemonScript) until you
find project markers (e.g. pyproject.toml or uv.lock) and set cwd to that
directory. Update the logic around the daemonScript/cwd variables in
session-start-continuity.ts to loop (using fs.existsSync or fs.promises.stat)
checking each parent for those files and stop at filesystem root; fall back to
the previous heuristic only if no marker is found. Ensure the new cwd is used
wherever the old cwd variable was referenced to run uv/run commands.
---
Nitpick comments:
In @.claude/hooks/src/session-start-continuity.ts:
- Line 344: Replace the ESM path extraction that uses new
URL(import.meta.url).pathname for hookDir with a fileURLToPath-based approach:
import fileURLToPath from 'url' (or use the built-in global if Node ≥20.11) and
call fileURLToPath(import.meta.url) before passing to path.dirname so hookDir is
correctly decoded and Windows-safe; update the reference where hookDir is
defined (the const hookDir assignment) to use
path.dirname(fileURLToPath(import.meta.url)).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.claude/hooks/dist/session-start-continuity.mjsis excluded by!**/dist/**,!**/dist/**,!**/.claude/hooks/dist/**
📒 Files selected for processing (1)
.claude/hooks/src/session-start-continuity.ts
1 participant
Summary
ensure_memory_daemon()from the dead Python hook to the registered TypeScript hookarchival_memorytable will actually populate with extracted learningsRoot Cause
ensure_memory_daemon()existed insession_start_continuity.py(Python) butsettings.jsonregisterssession-start-continuity.mjs(TypeScript) which had zero memory/daemon logic. Dead code since initial releasef8d7173.Changes
session-start-continuity.ts— addensureMemoryDaemon()function + call it inmain()session-start-continuity.mjs— rebuilt distImprovements over Python version
DEVNULL(swallowed)~/.claude/memory-daemon.logreturn Noneconsole.errorwarningsubprocess.Popenspawn()withdetached+unref()Test plan
memory-daemon.pidcreatedkill -0 $(cat ~/.claude/memory-daemon.pid)→ process alive~/.claude/memory-daemon.loghas startup outputFixes #156
Summary by CodeRabbit
New Features
Bug Fixes