fix(hooks): isolate session-end.js filename using transcript_path UUID (#1494)#1495
fix(hooks): isolate session-end.js filename using transcript_path UUID (#1494)#1495ratorin wants to merge 4 commits intoaffaan-m:mainfrom
Conversation
When session-end.js runs and CLAUDE_SESSION_ID is unset, getSessionIdShort() falls back to the project/worktree name. If any other Stop-hook in the chain spawns a claude subprocess (e.g. an AI-summary generator using 'claude -p'), the subprocess also fires the full Stop chain and writes to the same project- name-based filename, clobbering the parent's valid session summary with a summary of the summarization prompt itself. Fix: when stdin JSON (or CLAUDE_TRANSCRIPT_PATH) provides a transcript_path, extract the first 8 hex chars of the session UUID from the filename and use that as shortId. Falls back to the original getSessionIdShort() when no transcript_path is available, so existing behavior is preserved for all callers that do not set it. Adds a regression test in tests/hooks/hooks.test.js. Refs affaan-m#1494
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDerives the per-session shortId in Changes
Sequence Diagram(s)(omitted — change is localized to filename selection logic and tests; no multi-component flow requiring a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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: 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 `@tests/hooks/hooks.test.js`:
- Around line 648-654: The test relies on CLAUDE_SESSION_ID being unset but
runScript merges process.env, so explicitly set CLAUDE_SESSION_ID to an empty
string in the env passed to runScript to guarantee the fallback path is
exercised; update the call that invokes runScript(path.join(scriptsDir,
'session-end.js'), stdinJson, { HOME: isoHome, USERPROFILE: isoHome, ... }) to
include CLAUDE_SESSION_ID: '' so the session-end.js behavior and project-name
fallback collision are deterministically tested.
🪄 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: e52d959f-9d1c-4c3b-94b5-ce43a09803f1
📒 Files selected for processing (2)
scripts/hooks/session-end.jstests/hooks/hooks.test.js
Greptile SummaryThis PR addresses a Confidence Score: 4/5Safe to merge for the targeted scenario; residual concern from prior review thread about CLAUDE_SESSION_ID priority vs transcript UUID when they differ is unresolved but unlikely to surface in practice. The two concrete P1 issues from prior review threads (missing scripts/hooks/session-end.js — the CLAUDE_SESSION_ID vs transcript-UUID priority ordering (lines 211–220) is the unresolved concern from prior review threads. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[session-end.js main] --> B{Parse stdin JSON}
B -->|valid transcript_path| C[transcriptPath = stdin value]
B -->|missing or malformed| D{Check CLAUDE_TRANSCRIPT_PATH env}
D -->|set and non-empty| E[transcriptPath = env value]
D -->|unset or empty| F[transcriptPath = null]
C --> G{transcriptPath set?}
E --> G
F --> G
G -->|yes| H{Filename matches UUID regex?}
H -->|yes| I[shortId = sanitizeSessionId last 8 hex chars lowercased]
H -->|no| J[shortId = null]
I --> K{shortId truthy?}
J --> K
G -->|no| K
K -->|no| L[shortId = getSessionIdShort]
K -->|yes| M[sessionFile = today-shortId-session.tmp]
L --> M
Reviews (4): Last reviewed commit: "review: broaden CLAUDE_TRANSCRIPT_PATH f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
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/hooks/session-end.js">
<violation number="1" location="scripts/hooks/session-end.js:198">
P1: This unconditionally prefers `transcriptPath` over `CLAUDE_SESSION_ID` for deriving `shortId`. In a normal session where both are present, `getSessionIdShort()` would use the last 8 chars of `CLAUDE_SESSION_ID` — but this new code path bypasses that, silently changing the filename and orphaning existing `.tmp` files written under the old name. Guard this with `!process.env.CLAUDE_SESSION_ID` so the transcript-UUID derivation only activates in the fallback case the fix targets.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Use last-8 chars of transcript UUID instead of first-8, matching getSessionIdShort()'s .slice(-8) convention. Same session now produces the same filename whether shortId comes from CLAUDE_SESSION_ID or transcript_path, so existing .tmp files are not orphaned on upgrade. - Normalize extracted hex prefix to lowercase to avoid case-driven filename divergence from sanitizeSessionId()'s lowercase output. - Explicitly clear CLAUDE_SESSION_ID in the first regression test so the env leak from parent test runs cannot hide the fallback path. - Add regression tests for the lowercase-normalization path and for the case where CLAUDE_SESSION_ID and transcript_path refer to the same UUID (backward compat guarantee). Refs affaan-m#1494
|
Thanks for the reviews! Pushed 93cd5f4 to address the feedback: P1 (cubic, greptile): The concern about unconditional override and orphaned files is valid. Rather than guarding with P2 (greptile): Added P2 (coderabbitai): Added P2 (greptile — missing coexistence test): Added two new regression tests:
Full suite: Please take another look, and let me know if there's anything else to tweak. First time contributing here, so happy to follow your style/scope guidance. |
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 `@tests/hooks/hooks.test.js`:
- Around line 654-655: The inline comment is inverted: clearing the
CLAUDE_SESSION_ID env var prevents parent env leakage and causes the test to
exercise the transcript_path-derived naming (not the getSessionIdShort()
fallback). Update the comment near the CLAUDE_SESSION_ID clear in
tests/hooks/hooks.test.js to state that removing CLAUDE_SESSION_ID forces the
transcript_path branch, and mention getSessionIdShort() only as the
alternative/fallback branch that is not exercised here.
🪄 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: 88b96857-4ea3-40e8-8465-1a04c832a220
📒 Files selected for processing (2)
scripts/hooks/session-end.jstests/hooks/hooks.test.js
✅ Files skipped from review due to trivial changes (1)
- scripts/hooks/session-end.js
- Route the transcript-derived shortId through sanitizeSessionId so the fallback and transcript branches remain byte-for-byte equivalent for any non-UUID session IDs that still land in CLAUDE_SESSION_ID (greptile P1). - Clarify the inline comment in the first regression test: clearing CLAUDE_SESSION_ID exercises the transcript_path branch, not the getSessionIdShort() fallback (coderabbit P2). Refs affaan-m#1494
|
Pushed 01d8167:
Tests still green: 222/222. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/hooks/session-end.js (1)
184-190:⚠️ Potential issue | 🟠 MajorRestore env fallback when parsed stdin lacks a usable transcript path.
Right now
CLAUDE_TRANSCRIPT_PATHis only consulted whenJSON.parse(stdinData)throws. If stdin is valid JSON but omitstranscript_pathor provides an empty/non-string value,transcriptPathstays unusable and Line 212 falls back to the project-name-basedgetSessionIdShort(), which can reintroduce the collision this PR is fixing for the env compatibility path.🐛 Proposed fix
let transcriptPath = null; try { const input = JSON.parse(stdinData); - transcriptPath = input.transcript_path; + if (typeof input?.transcript_path === 'string' && input.transcript_path.length > 0) { + transcriptPath = input.transcript_path; + } } catch { - // Fallback: try env var for backwards compatibility - transcriptPath = process.env.CLAUDE_TRANSCRIPT_PATH; + // Ignore malformed stdin; try env var for backwards compatibility below. + } + + const envTranscriptPath = process.env.CLAUDE_TRANSCRIPT_PATH; + if (!transcriptPath && typeof envTranscriptPath === 'string' && envTranscriptPath.length > 0) { + transcriptPath = envTranscriptPath; }As per coding guidelines, “All user inputs must be validated” and “Always validate all user input before processing at system boundaries.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/session-end.js` around lines 184 - 190, Parsed stdin may be valid JSON but still lack a usable transcript_path, so after JSON.parse(stdinData) check that input.transcript_path is a non-empty string (e.g. typeof === 'string' and trimmed length > 0) before assigning to transcriptPath; if it is missing/empty/falsy then fall back to process.env.CLAUDE_TRANSCRIPT_PATH instead of leaving transcriptPath unusable (this change affects the try block where stdinData is parsed and the variables transcriptPath and input are set, and preserves the existing catch that also falls back to process.env.CLAUDE_TRANSCRIPT_PATH), ensuring downstream getSessionIdShort() fallback only runs when both sources are absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/hooks/session-end.js`:
- Around line 184-190: Parsed stdin may be valid JSON but still lack a usable
transcript_path, so after JSON.parse(stdinData) check that input.transcript_path
is a non-empty string (e.g. typeof === 'string' and trimmed length > 0) before
assigning to transcriptPath; if it is missing/empty/falsy then fall back to
process.env.CLAUDE_TRANSCRIPT_PATH instead of leaving transcriptPath unusable
(this change affects the try block where stdinData is parsed and the variables
transcriptPath and input are set, and preserves the existing catch that also
falls back to process.env.CLAUDE_TRANSCRIPT_PATH), ensuring downstream
getSessionIdShort() fallback only runs when both sources are absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b1534e8-089b-4bb0-b449-2837bfe0deef
📒 Files selected for processing (2)
scripts/hooks/session-end.jstests/hooks/hooks.test.js
✅ Files skipped from review due to trivial changes (1)
- tests/hooks/hooks.test.js
…y JSON fields Previously the env fallback ran only when JSON.parse threw. If stdin was valid JSON but omitted transcript_path or provided a non-string/empty value, the script dropped to the getSessionIdShort() fallback path, re-introducing the collision this PR targets. Validate the parsed transcript_path and apply the env-var fallback for any unusable value, not just malformed JSON. Matches coderabbit's outside-diff suggestion and keeps both input-source paths equivalent. Refs affaan-m#1494
|
Pushed 0c3fc70: broadened the |
1 participant
Summary
Fixes #1494 —
*-session.tmpcorruption when another Stop hook spawns aclaudesubprocess.In my own setup, a custom AI-summarization Stop hook spawned
claude -p ...to generate a session summary. That subprocess also fires the full Stop hook chain, soscripts/hooks/session-end.jsis invoked a second time. BecausegetSessionIdShort()falls back to the project/worktree name whenCLAUDE_SESSION_IDis unset, both invocations produced the same{date}-{shortId}-session.tmppath. The subprocess transcript only contained the summarization prompt, so its### Tasksended up holding the prompt text andTotal user messages: 1, overwriting the parent's valid summary.Type
Fix
When stdin JSON (or
CLAUDE_TRANSCRIPT_PATH) provides atranscript_path, deriveshortIdfrom the first 8 hex characters of the session UUID in the filename instead of always callinggetSessionIdShort(). The existing fallback is preserved for cases withouttranscript_path, so nothing changes for other callers.Parent and subprocess invocations now resolve to different UUIDs, so they write to different files and do not collide.
Testing
New regression test in
tests/hooks/hooks.test.js(derives shortId from transcript_path UUID when available) runssession-end.jswith stdin carrying a fake transcript path/tmp/.../abcdef12-3456-4789-a012-bcdef3456789.jsonl, withoutCLAUDE_SESSION_IDset, and asserts the resulting.tmpfile name containsabcdef12(the UUID prefix) rather than the project-name fallback.Full suite:
node tests/hooks/hooks.test.js→ 220 passed, 0 failed on Windows 11 + Node 20.Notes / request for feedback
claudefrom another Stop hook would hit it. Happy to adjust the scope or style if you prefer a different direction..tmpfiles under~/.claude/sessions/will grow faster because each real session now gets its own UUID-based file instead of reusing the project-name filename. Related open PRs fix(hooks): add ECC_SESSION_STRICT_MATCH to prevent cross-project session fallback #1481 / fix(hooks): scope session-start injection by shortId to prevent cross-cwd leakage #1485 / Scope SessionStart context injection #1493 are already tightening the read-side matching, so this should compose well with them, but if you want me to also touch up a retention or mtime-ordering pass on the read side in the same PR, let me know.path.basename(...)so macOS/Linux behavior should be identical, but I have not tested on those platforms.If anything looks off, please comment and I will revise.
Summary by cubic
Prevents session summary
.tmpcollisions when a Stop hook spawns aclaudesubprocess by deriving the filename shortId from thetranscript_pathUUID when available. Parent and subprocess now write to different files while keeping names compatible withgetSessionIdShort().shortIdfrom the last 8 hex chars of the transcript UUID (from stdin orCLAUDE_TRANSCRIPT_PATH), lowercase it, and route throughsanitizeSessionId()to matchgetSessionIdShort()and avoid collisions.transcript_pathor provides an empty/non-string value (or malformed JSON), fall back toCLAUDE_TRANSCRIPT_PATH; if still absent, fall back togetSessionIdShort().shortId, lowercase normalization, and parity whenCLAUDE_SESSION_IDequals the transcript UUID.Written for commit 0c3fc70. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests