fix(#843): session-manager hooks — ecc-sessions dir + hookSpecificOutput format#948
fix(#843): session-manager hooks — ecc-sessions dir + hookSpecificOutput format#948anuragg-saxenaa wants to merge 4 commits intoaffaan-m:mainfrom
Conversation
…ormat - Rename sessions dir from 'sessions' to 'ecc-sessions' to prevent Claude Code's built-in cleanup from deleting session-end hook files - Update session-start.js to output hookSpecificOutput JSON format so context is actually injected into Claude's session (plain stdout strings are silently discarded by SessionStart hooks)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReworks session-start hook output and session-file discovery: session-start now accumulates context, emits a single JSON payload via safe stdout writes and avoids blocking exit; session-file search deduplicates across canonical and legacy session dirs; utilities add sanitized session ID logic and new session-directory helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Greptile SummaryThis PR fixes two root causes that prevented the session-manager hooks from working out of the box: (1) ECC session files were stored in Changes:
Notable discrepancy: The PR title and description consistently reference Confidence Score: 4/5Safe to merge with one targeted fix: reconcile the The two core bug fixes are correctly implemented and the prior P0 double-JSON-write concern is resolved. Remaining comments are non-blocking P2s with no data-loss, security, or runtime correctness issues. Both files are low-risk; only the naming discrepancy between the PR description and the actual Important Files Changed
Reviews (3): Last reviewed commit: "refactor(session-start): dedupe sessions..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (1)
scripts/lib/utils.js (1)
34-37: Extract'ecc-sessions'into a shared constant/config key.This works functionally, but the directory name is now a magic string. Pull it into a module-level constant (or config) to avoid drift across hook scripts.
Proposed refactor
+const ECC_SESSIONS_DIRNAME = 'ecc-sessions'; + function getSessionsDir() { // Use 'ecc-sessions' instead of 'sessions' to avoid Claude Code's built-in // session directory cleanup which removes files written by session-end hooks. // See: https://github.com/affaan-m/everything-claude-code/issues/843 - return path.join(getClaudeDir(), 'ecc-sessions'); + return path.join(getClaudeDir(), ECC_SESSIONS_DIRNAME); }As per coding guidelines, "Do not use hardcoded values; use constants or configuration instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/utils.js` around lines 34 - 37, Replace the magic string 'ecc-sessions' with a shared constant: add a module-level constant (e.g., const ECC_SESSIONS_DIR = 'ecc-sessions') or import it from a config module, then use path.join(getClaudeDir(), ECC_SESSIONS_DIR) in the function that returns the sessions path; update any other scripts that rely on the same literal to use the new constant to avoid drift (reference the return using getClaudeDir() in scripts/lib/utils.js).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/hooks/session-start.js`:
- Around line 54-55: Replace the forced process.exit(0) calls so stdout can
flush after writing the JSON payload: locate the occurrences of process.exit(0)
in scripts/hooks/session-start.js (the two places referenced in the review) and
change the first one to return (so the function returns and lets the event loop
finish flushing output) and change the second one to set process.exitCode = 0
(so the process exits with success without forcing immediate termination); keep
the JSON write (process.stdout.write(JSON.stringify(payload) + '\n')) unchanged.
- Around line 95-102: The hook currently writes two separate JSON objects via
process.stdout.write (one for the previous session summary and one as
projPayload with hookSpecificOutput), so consolidate by accumulating all
additionalContext strings into a single hookSpecificOutput.additionalContext
array (e.g., collect the previous session summary context and the detected
project information into the same array) and then emit one JSON object once all
contexts are gathered by calling
process.stdout.write(JSON.stringify(projPayload) + '\n'); update
projPayload.hookSpecificOutput to hold an array instead of a single string so
both contexts are included in the single output.
---
Nitpick comments:
In `@scripts/lib/utils.js`:
- Around line 34-37: Replace the magic string 'ecc-sessions' with a shared
constant: add a module-level constant (e.g., const ECC_SESSIONS_DIR =
'ecc-sessions') or import it from a config module, then use
path.join(getClaudeDir(), ECC_SESSIONS_DIR) in the function that returns the
sessions path; update any other scripts that rely on the same literal to use the
new constant to avoid drift (reference the return using getClaudeDir() in
scripts/lib/utils.js).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7af093f4-853f-428a-9d06-8d9b72fe4f5b
📒 Files selected for processing (2)
scripts/hooks/session-start.jsscripts/lib/utils.js
|
thanks for the PR. quick triage: maintainer review pending. if there are docs, screenshots, or repro steps, please drop them here. |
…envelope - Rename sessions dir from 'sessions' to 'ecc-sessions' to avoid Claude Code's built-in cleanup which deletes session-end hook files - Emit exactly one JSON object with hookSpecificOutput at the end (was writing two separate JSON objects in the happy path) - Accumulate all context into one additionalContext string - Fixes CodeRabbit review comments on PR affaan-m#948
…t end Resolves conflict markers from linter re-introducing broken early-JSON pattern. Both the session content and project detection blocks now defer to the emitContext() call at the end, which produces exactly one JSON envelope. Parent commit: 25a1c45
…ns dirs - Use dedupeRecentSessions() to merge results from both session directories without duplicating by filename; canonical dir wins on tie - Replace inline getSessionSearchDirs with explicit import - Swap process.stdout.write for writeSessionStartPayload() Promise wrapper to handle stream errors gracefully - Align with linter auto-fix on PR affaan-m#948 review
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/lib/utils.js">
<violation number="1" location="scripts/lib/utils.js:17">
P2: Backward compatibility regression: previous `~/.claude/ecc-sessions` data is no longer searched after directory rename, so existing sessions may appear missing after upgrade.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Closing as already landed on current main. The session-start payload/session-data work and the utils session-directory/sanitization changes from this PR are already present upstream in scripts/hooks/session-start.js and scripts/lib/utils.js, so keeping this branch open would duplicate code that has already shipped. |
2 participants
Summary
Fixes #843 — session hooks installed but not working out of the box.
Two root causes identified and fixed:
Fix 1: Session directory collision with Claude Code cleanup
Claude Code performs cleanup on its built-in
~/.claude/sessions/directory on exit, which removes files written by thesession-endhook before the next session can read them.Fix: Renamed the sessions directory from
sessionstoecc-sessionsso ECC-managed files live outside Claude Code's cleanup scope.Fix 2: SessionStart hook output format
Claude Code's
SessionStarthooks require JSON output with ahookSpecificOutputenvelope. Plainconsole.log(string)output is silently discarded — no context is injected.Fix: Updated
session-start.jsto useprocess.stdout.write(JSON.stringify({hookSpecificOutput: {hookEventName, additionalContext}}))for context injection.Test plan
~/.claude/ecc-sessions/contains a session file after session ends~/.claude/ecc-sessions/still exists (not cleaned up by Claude Code)Summary by cubic
Makes session hooks work out of the box by moving session storage out of Claude Code’s cleanup, deduping recent sessions across directories, and emitting a single
hookSpecificOutputJSON with accumulated context at session start. Fixes #843.~/.claude/session-data/and search both canonical and legacysessionsdirs to dedupe recent files, avoiding cleanup-related losses.hookSpecificOutputJSON via a safe final write; remove early stdout writes and sanitize session IDs for valid, stable filenames.Written for commit 2546f96. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Bug Fixes