fix(hooks): scope session-start injection by shortId to prevent cross-cwd leakage#1485
Conversation
…-cwd leakage The SessionStart hook picked the globally-newest *-session.tmp file from ~/.claude/sessions/ and injected it as "Previous session summary" into the context of every new Claude session, regardless of which cwd/project/worktree the session was for. Symptom observed in the wild: a Claude Agent SDK session dispatched in worktree A received worktree B's session summary and began making tool calls as if the task belonged to B — content-level crosstalk across worktrees/projects sharing a single ~/.claude/sessions directory. Fix: import getSessionIdShort() and parseSessionFilename(), compute the current invocation's shortId, and filter recent session files down to only those whose filename shortId matches. If no file matches, skip injection entirely rather than fall back to the globally-newest file — a missing summary is far safer than a wrong one. Tests: - New regression test tests/hooks/session-start-worktree-isolation.test.js simulates two worktrees (A older, B newer on disk) and asserts each invocation gets ONLY its own summary. - Three existing hooks.test.js tests updated to pass CLAUDE_SESSION_ID matching the session filename's shortId so they still exercise their scenarios (real content injection, 7-day maxAge boundary, newest-of-many selection) under the new scoped behaviour. No assertions weakened. All 204 existing hook tests + 2 new isolation tests pass.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
📝 WalkthroughWalkthroughThe pull request modifies the session-start hook to scope session file injection by the current invocation's shortId (parsed from CLAUDE_SESSION_ID or derived from cwd basename) instead of using the globally newest session file. Tests are updated to validate this scoped behavior, and a new regression test verifies worktree isolation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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/session-start-worktree-isolation.test.js`:
- Around line 65-84: The test is flaky because session-start.js's
getSessionIdShort() → getProjectName() calls git rev-parse which can walk up
into an unrelated repo under os.tmpdir(); to fix, when spawning the child that
runs session-start.js (from main() using isoHome, worktreeA/worktreeB and
basenameA/basenameB), neutralize git's repository walk-up so rev-parse fails and
the basename fallback is used — for example set a blocking git env for the child
process (e.g. GIT_DIR to a non-existent path or GIT_CEILING_DIRECTORIES to
isoHome) in the spawn/env passed to session-start.js so getProjectName()/git
rev-parse cannot discover an ancestor repo and the test deterministically falls
back to the cwd basename.
🪄 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: d8490633-e670-4823-87f7-67a7228856d8
📒 Files selected for processing (3)
scripts/hooks/session-start.jstests/hooks/hooks.test.jstests/hooks/session-start-worktree-isolation.test.js
| async function main() { | ||
| const failures = []; | ||
|
|
||
| // Isolated $HOME so the test never touches the real ~/.claude | ||
| const isoHome = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-worktree-isolation-')); | ||
| const sessionsDir = path.join(isoHome, '.claude', 'sessions'); | ||
| fs.mkdirSync(sessionsDir, { recursive: true }); | ||
| fs.mkdirSync(path.join(isoHome, '.claude', 'skills', 'learned'), { recursive: true }); | ||
|
|
||
| // Two fake worktrees as cwds. Each has its own basename which | ||
| // getSessionIdShort() will fall back to when CLAUDE_SESSION_ID is unset. | ||
| // Basenames must be lowercase alphanumeric+hyphen with length >= 8 to | ||
| // match SESSION_FILENAME_REGEX in session-manager.js. Nesting them under | ||
| // isoHome keeps the test hermetic and avoids mkdtempSync's mixed-case suffix. | ||
| const basenameA = 'worktree-a-test'; | ||
| const basenameB = 'worktree-b-test'; | ||
| const worktreeA = path.join(isoHome, basenameA); | ||
| const worktreeB = path.join(isoHome, basenameB); | ||
| fs.mkdirSync(worktreeA, { recursive: true }); | ||
| fs.mkdirSync(worktreeB, { recursive: true }); |
There was a problem hiding this comment.
Potential test flakiness if /tmp (or an ancestor) is inside a git repo.
Both worktree cwds are nested under isoHome in os.tmpdir(). The child process runs session-start.js, which calls getSessionIdShort() → getProjectName() → git rev-parse --show-toplevel. If any ancestor of the temp directory happens to be a git repo (container images, dev sandboxes where / or $HOME is a repo, or tmpdir mounted inside a worktree), rev-parse will succeed and return that outer repo's basename for both invocations — making both shortIds identical and causing the isolation assertion to either spuriously pass (no leakage because both sides resolve to the same scope and each writes/reads its own basename-named file... wait, actually filenames still embed worktree-a-test / worktree-b-test, so they'd fail to match the outer repo's shortId and the test would fail with "no sessions injected" instead of detecting leakage).
Consider hardening by neutralizing the git walk-up so the basename fallback is deterministic:
🛡️ Proposed hardening
function runSessionStart({ cwd, env }) {
return new Promise((resolve, reject) => {
const proc = spawn('node', [sessionStartScript], {
cwd,
- env: { ...process.env, ...env },
+ // GIT_CEILING_DIRECTORIES stops `git rev-parse --show-toplevel` from
+ // walking above the temp worktree and picking up an unrelated outer
+ // repo (e.g. when /tmp or $HOME sits inside a git checkout).
+ env: { ...process.env, GIT_CEILING_DIRECTORIES: cwd, ...env },
stdio: ['pipe', 'pipe', 'pipe']
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/session-start-worktree-isolation.test.js` around lines 65 - 84,
The test is flaky because session-start.js's getSessionIdShort() →
getProjectName() calls git rev-parse which can walk up into an unrelated repo
under os.tmpdir(); to fix, when spawning the child that runs session-start.js
(from main() using isoHome, worktreeA/worktreeB and basenameA/basenameB),
neutralize git's repository walk-up so rev-parse fails and the basename fallback
is used — for example set a blocking git env for the child process (e.g. GIT_DIR
to a non-existent path or GIT_CEILING_DIRECTORIES to isoHome) in the spawn/env
passed to session-start.js so getProjectName()/git rev-parse cannot discover an
ancestor repo and the test deterministically falls back to the cwd basename.
Greptile SummaryThis PR fixes a real cross-worktree session leak in
Confidence Score: 4/5Safe to merge for the common Claude Code use case (CLAUDE_SESSION_ID always provided), but introduces a silent session-injection regression for projects with names shorter than 8 characters when running without CLAUDE_SESSION_ID. The cross-worktree isolation fix is correct and well-tested. One P1 finding remains: projects with names shorter than 8 characters will silently lose session injection due to SESSION_FILENAME_REGEX's {8,} minimum, a regression from pre-PR behavior. This is a real present defect on the fallback code path, warranting a score of 4 rather than 5. scripts/hooks/session-start.js — the shortId filter interacts with SESSION_FILENAME_REGEX's 8-char minimum in a way that silently drops sessions for short project names. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SessionStart hook runs] --> B[findFiles: recent sessions last 7 days]
B --> C{recentSessions.length > 0?}
C -- No --> G[Skip injection]
C -- Yes --> D[getSessionIdShort\nCLAUDE_SESSION_ID slice -8\nor projectName or 'default']
D --> E[Filter: parseSessionFilename\nshortId === currentShortId]
E --> F{scopedSessions.length > 0?}
F -- Yes --> H[Read latest.path\noutput content to stdout]
F -- No --> I{recentSessions existed?}
I -- Yes --> J[log: sessions exist but\nnone match shortId - skip]
I -- No --> G
style E fill:#f96,stroke:#c00
E -.->|null if shortId < 8 chars\nor old-format file| K[❌ Silently excluded]
K -.-> F
Reviews (1): Last reviewed commit: "fix(hooks): scope session-start injectio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const currentShortId = getSessionIdShort(); | ||
| const scopedSessions = recentSessions.filter(f => { | ||
| const parsed = parseSessionFilename(path.basename(f.path)); | ||
| return parsed && parsed.shortId === currentShortId; | ||
| }); |
There was a problem hiding this comment.
When CLAUDE_SESSION_ID is unset, getSessionIdShort() falls back to the full project/git-repo name (e.g. app, api, web, server). session-end.js writes the file as YYYY-MM-DD-<name>-session.tmp. SESSION_FILENAME_REGEX in session-manager.js requires the shortId segment to be [a-z0-9-]{8,} — it returns null for names shorter than 8 characters. Since this filter skips null results, those session files are silently excluded and injection is broken for any project whose name is < 8 chars. This is a regression: before this PR the globally-newest file was injected regardless of parseability.
A minimal fix is to log a warning for files that are recent but unparseable, so operators can detect the issue:
const scopedSessions = recentSessions.filter(f => {
const parsed = parseSessionFilename(path.basename(f.path));
if (!parsed) {
log(`[SessionStart] Could not parse filename (shortId < 8 chars?): ${path.basename(f.path)}`);
return false;
}
return parsed.shortId === currentShortId;
});The root fix is to relax SESSION_FILENAME_REGEX to [a-z0-9-]{1,} or to pad short project names in getSessionIdShort() to always produce ≥ 8 characters.
| } else if (recentSessions.length > 0) { | ||
| log(`[SessionStart] ${recentSessions.length} recent session(s) exist but none match shortId=${currentShortId}; skipping injection`); | ||
| } |
There was a problem hiding this comment.
parseSessionFilename returns shortId: 'no-id' for old-format files (YYYY-MM-DD-session.tmp). They will never match any real currentShortId, so users who previously had old-format session files silently lose session continuity after this update. The else if log message ("recent session(s) exist but none match shortId") also fires for this case, making it hard to distinguish a cross-worktree mismatch from a legacy-format issue. Consider differentiating the two cases in the log or documenting the migration trade-off.
![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 3 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-start.js">
<violation number="1" location="scripts/hooks/session-start.js:44">
P2: Filtering by `shortId` here still allows cross-project leakage when `CLAUDE_SESSION_ID` is unset and two worktrees/repos share the same basename.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Before this filter, the hook picked the globally-newest session file and | ||
| // leaked it cross-worktree — e.g. worktree A received worktree B's summary, | ||
| // causing downstream agents to act on another worktree's task. | ||
| const currentShortId = getSessionIdShort(); |
There was a problem hiding this comment.
P2: Filtering by shortId here still allows cross-project leakage when CLAUDE_SESSION_ID is unset and two worktrees/repos share the same basename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/session-start.js, line 44:
<comment>Filtering by `shortId` here still allows cross-project leakage when `CLAUDE_SESSION_ID` is unset and two worktrees/repos share the same basename.</comment>
<file context>
@@ -33,9 +36,20 @@ async function main() {
+ // Before this filter, the hook picked the globally-newest session file and
+ // leaked it cross-worktree — e.g. worktree A received worktree B's summary,
+ // causing downstream agents to act on another worktree's task.
+ const currentShortId = getSessionIdShort();
+ const scopedSessions = recentSessions.filter(f => {
+ const parsed = parseSessionFilename(path.basename(f.path));
</file context>
1 participant
Summary
Fixes a cross-worktree / cross-cwd content leak in the
SessionStarthook.Before this patch,
scripts/hooks/session-start.jspicked the globally-newest*-session.tmpfile from~/.claude/sessions/and injected it asPrevious session summaryinto the context of every new Claude session, regardless of which cwd/project/worktree the session was for.Symptom I observed in the wild: a Claude Agent SDK session dispatched for worktree A received worktree B's session summary and began making tool calls as if the task belonged to B — content-level crosstalk across worktrees/projects that share
~/.claude/sessions/.Type
Fix
getSessionIdShortfrom../lib/utilsandparseSessionFilenamefrom../lib/session-managershortId(last 8 chars of$CLAUDE_SESSION_ID, else project/worktree basename)recentSessionsdown to only those whose filenameshortIdmatches the current invocationlog(...)level so operators can still see when recent sessions exist but don't belong to this invocationNo public API change, no new dependencies. Pure filter added to an existing code path.
Testing
New regression test
tests/hooks/session-start-worktree-isolation.test.js— simulates two worktrees (A, B) with session summaries where B's is newer on disk, then runssession-start.jsonce from each cwd and asserts:Before the fix, test 1 fails: worktree A receives worktree B's summary (the globally-newest file), which is exactly the crosstalk bug.
Existing tests updated
Three existing tests in
tests/hooks/hooks.test.jspreviously relied on the globally-newest behaviour. They now passCLAUDE_SESSION_IDso the invocation's shortId matches the test's session filename and the scenario still exercises the same code path:injects real session content— asserts real content still flows when shortIds matchexcludes session files older than 7 days— unified shortId across both recent/old files so maxAge filtering is what excludes the old one (the original intent)injects newest session when multiple recent sessions exist— both files share shortId so the newest-by-mtime still wins (the original intent)No assertions weakened; each test still verifies exactly what it originally verified.
Results
Checklist
Summary by cubic
Scopes previous session injection in the
SessionStarthook by shortId to stop cross-cwd/worktree leaks. If no matching session exists, nothing is injected.getSessionIdShortandparseSessionFilenameto compute the current shortId and filter~/.claude/sessions/*-session.tmpto only matching files; pick the newest within scope.tests/hooks/session-start-worktree-isolation.test.jsto cover worktree isolation; update three existing tests to setCLAUDE_SESSION_ID. No API or dependency changes.Written for commit dcb2895. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests