Scope SessionStart context injection#1493
Conversation
|
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:
📝 WalkthroughWalkthroughParse SessionStart stdin JSON to determine mode (startup/resume/skip/invalid/null), tighten session selection (no recency fallback; project-match only when worktree absent), and conditionally inject a previous-session summary only when mode and project/worktree match. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as Hook stdin
participant Script as session-start.js
participant FS as Sessions (filesystem)
participant Log as Logger
Hook->>Script: deliver raw stdin payload
Script->>Log: trim input
Script->>Script: getSessionStartMode(rawInput)
alt parse failure
Script->>Log: log "Invalid stdin payload (Length: ...)"
Script-->>Hook: treat as INVALID (skip injection)
else parse ok
Script->>Log: mode = resume|startup|skip|null
Script->>FS: call selectMatchingSession(project, worktree)
FS-->>Script: matching session or null
alt session found and mode allows injection (startup or stdin null)
Script->>FS: read session content
Script->>Script: inject "Previous session summary"
Script->>Log: log "injected"
else no match or mode prevents injection
Script->>Log: log "skipping injection / no match"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
Greptile SummaryThis PR scopes
Confidence Score: 3/5Merge only after resolving the SESSION_START_MODE_SKIP fallthrough for unrecognized startup payloads, which silently breaks context injection for valid sessions. The core fix is correct and well-tested, but the catch-all SESSION_START_MODE_SKIP return creates a silent regression path for any SessionStart payload with a missing or unrecognized source. scripts/hooks/session-start.js — specifically the getSessionStartMode fallthrough logic at lines 46-55. Important Files Changed
Reviews (5): Last reviewed commit: "Fail closed on unrecognized SessionStart..." | Re-trigger Greptile |
![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
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-start.js (1)
132-149:⚠️ Potential issue | 🟠 MajorDon’t treat a different recorded worktree as a project match.
Line 146 can still inject a session from another checkout if both directories share the same basename/project name. To avoid reintroducing cross-worktree context leaks, only use project-name fallback when the session has no worktree metadata, or compare a stronger project identity.
🔒 Proposed fix
// Extract **Worktree:** field const worktreeMatch = content.match(/\*\*Worktree:\*\*\s*(.+)$/m); const sessionWorktree = worktreeMatch ? worktreeMatch[1].trim() : ''; + const normalizedSessionWorktree = sessionWorktree ? normalizePath(sessionWorktree) : ''; // Exact worktree match — best possible, return immediately // Normalize both paths to handle symlinks and case-insensitive filesystems - if (sessionWorktree && normalizePath(sessionWorktree) === normalizedCwd) { + if (normalizedSessionWorktree && normalizedSessionWorktree === normalizedCwd) { return { session, content, matchReason: 'worktree' }; } // Project name match — keep searching for a worktree match - if (!projectMatch && currentProject) { + if (!projectMatch && currentProject && !sessionWorktree) { const projectFieldMatch = content.match(/\*\*Project:\*\*\s*(.+)$/m); const sessionProject = projectFieldMatch ? projectFieldMatch[1].trim() : ''; if (sessionProject && sessionProject === currentProject) {As per coding guidelines, “Never trust external data (API responses, user input, file content)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/session-start.js` around lines 132 - 149, The current project-name fallback can accept sessions that explicitly record a different worktree, causing cross-worktree leaks; in the block that sets projectMatch (referencing projectMatch, currentProject, projectFieldMatch, sessionProject), only treat a session as a project match when it has no Worktree metadata — i.e., check that sessionWorktree is empty (the value produced earlier) before comparing sessionProject to currentProject (or alternatively require a stronger identity), so replace the condition that opens the project-name fallback with one that also verifies sessionWorktree is falsy.
🧹 Nitpick comments (1)
tests/hooks/hooks.test.js (1)
545-547: Assert the resume sentinel is absent too.This test currently checks only the heading. It would still pass if prior session content were injected under another label.
✅ Proposed test hardening
assert.strictEqual(result.code, 0); const additionalContext = getSessionStartAdditionalContext(result.stdout); assert.ok(!additionalContext.includes('Previous session summary'), 'Should skip previous session summary on resume'); + assert.ok( + !additionalContext.includes('RESUME_CONTEXT_SHOULD_NOT_BE_INJECTED'), + 'Should not inject previous session content on resume' + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 545 - 547, The test currently only checks that additionalContext (from getSessionStartAdditionalContext(result.stdout)) does not include the heading 'Previous session summary'; also assert that the resume sentinel string is absent to prevent prior session content being injected under a different label—add an assertion on additionalContext to ensure it does not include the resume sentinel (use the exact sentinel literal used by the codebase or the constant/variable name if available) alongside the existing assert.ok(!additionalContext.includes('Previous session summary'), ...).
🤖 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 34-43: The current parsing of raw stdin silently falls back to
startup on malformed non-empty payloads; change the catch to fail-closed by
checking raw.trim() and, if non-empty, log the validation failure (include raw
or error message) and return a sentinel (e.g., 'skip' or 'invalid') so
previous-summary injection is skipped; specifically update the try/catch around
JSON.parse(raw) that uses raw, input and hookName to log malformed payloads and
return 'skip' instead of allowing the code to proceed to startup.
---
Outside diff comments:
In `@scripts/hooks/session-start.js`:
- Around line 132-149: The current project-name fallback can accept sessions
that explicitly record a different worktree, causing cross-worktree leaks; in
the block that sets projectMatch (referencing projectMatch, currentProject,
projectFieldMatch, sessionProject), only treat a session as a project match when
it has no Worktree metadata — i.e., check that sessionWorktree is empty (the
value produced earlier) before comparing sessionProject to currentProject (or
alternatively require a stronger identity), so replace the condition that opens
the project-name fallback with one that also verifies sessionWorktree is falsy.
---
Nitpick comments:
In `@tests/hooks/hooks.test.js`:
- Around line 545-547: The test currently only checks that additionalContext
(from getSessionStartAdditionalContext(result.stdout)) does not include the
heading 'Previous session summary'; also assert that the resume sentinel string
is absent to prevent prior session content being injected under a different
label—add an assertion on additionalContext to ensure it does not include the
resume sentinel (use the exact sentinel literal used by the codebase or the
constant/variable name if available) alongside the existing
assert.ok(!additionalContext.includes('Previous session summary'), ...).
🪄 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: e7fc5e8c-71fa-4b26-ba65-0ae3d51ed810
📒 Files selected for processing (2)
scripts/hooks/session-start.jstests/hooks/hooks.test.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues 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-start.js">
<violation number="1" location="scripts/hooks/session-start.js:36">
P2: Resume suppression is brittle because it relies only on exact `hookName` parsing; if SessionStart payload uses a different field shape (e.g., `source`), resume sessions will be misclassified and previous-session context may be injected incorrectly.</violation>
<violation number="2" location="scripts/hooks/session-start.js:162">
P2: Unhandled stdin read can abort SessionStart before emitting hook payload.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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 `@scripts/hooks/session-start.js`:
- Around line 36-47: The current parser returns null for valid JSON with a
missing/unknown hookName which lets main() treat it as startup; change the logic
so any non-empty stdin payload that does not explicitly match
'SessionStart:resume' or 'SessionStart:startup' returns SESSION_START_MODE_SKIP
(fail closed) instead of null, and remove/raw-sensitive data from logs by
replacing the raw payload in the catch log with a generic message (use
error.message only or a fixed text) — update the block that parses raw, the
hookName checks, and the catch logging (referencing hookName, raw,
SESSION_START_MODE_SKIP, and main()) accordingly.
🪄 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: 832a37d7-ce9d-436a-b307-64f17232484f
📒 Files selected for processing (2)
scripts/hooks/session-start.jstests/hooks/hooks.test.js
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/hooks/session-start.js">
<violation number="1" location="scripts/hooks/session-start.js:43">
P2: Malformed stdin handling logs raw input content, potentially exposing sensitive data in stderr/log captures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/hooks/session-start.js (1)
393-418: Redundant "no match" logs — minor nit.When
selectMatchingSessionreturnsnullyou log'No worktree/project session match found'(line 212) and then the caller logs'No matching session found'(line 416). Consider dropping one to keep the log scannable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/session-start.js` around lines 393 - 418, The caller in session-start.js logs "No matching session found" after selectMatchingSession already logs a similar "No worktree/project session match found"; remove or demote one of these redundant messages to avoid duplicate output — e.g., delete or change the log call in this file (the log invocation inside the else branch after const result = selectMatchingSession(...)) or change the message level in selectMatchingSession itself; update only the log statement (function: selectMatchingSession and the caller log in session-start.js) so a single clear message remains.
🤖 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 33-49: getSessionStartMode currently only checks input.hookName
(Desktop/SDK shape) so CLI payloads using hook_event_name + source are ignored;
update getSessionStartMode to accept both shapes: if input.hookName is a string
handle 'SessionStart:resume' and 'SessionStart:startup' as before, and
additionally if input.hook_event_name === 'SessionStart' map input.source ===
'resume'|'startup' to return 'resume' or 'startup' (treat 'clear'/'compact' as
SESSION_START_MODE_SKIP or null as appropriate). Also change the catch logging
around log(...) so raw stdin is not printed verbatim—either omit raw, replace it
with a short hash/anonymized token, or only log the truncated length—to avoid
leaking session IDs/paths while still reporting the parse error (use
error.message and the chosen anonymized indicator); keep references to
getSessionStartMode and SESSION_START_MODE_SKIP when making these edits.
---
Nitpick comments:
In `@scripts/hooks/session-start.js`:
- Around line 393-418: The caller in session-start.js logs "No matching session
found" after selectMatchingSession already logs a similar "No worktree/project
session match found"; remove or demote one of these redundant messages to avoid
duplicate output — e.g., delete or change the log call in this file (the log
invocation inside the else branch after const result =
selectMatchingSession(...)) or change the message level in selectMatchingSession
itself; update only the log statement (function: selectMatchingSession and the
caller log in session-start.js) so a single clear message remains.
🪄 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: d5e75ea2-4434-49a4-b98c-6698d0a595dd
📒 Files selected for processing (2)
scripts/hooks/session-start.jstests/hooks/hooks.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/hooks/hooks.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/hooks/session-start.js (1)
33-55:⚠️ Potential issue | 🟠 MajorFail closed for non-empty unrecognized stdin payloads.
Line 55 still returns
nullfor valid JSON that is not explicitlySessionStart:startup,SessionStart:resume,clear, orcompact, somain()treats it as eligible for previous-session injection. For this privacy-sensitive path, keep empty stdin as legacy startup, but skip injection for any non-empty unrecognized payload.🛡️ Proposed fix
const hookName = typeof input?.hookName === 'string' ? input.hookName : ''; if (hookName === 'SessionStart:resume') return 'resume'; if (hookName === 'SessionStart:startup') return 'startup'; + if (hookName === 'SessionStart:clear' || hookName === 'SessionStart:compact') { + return SESSION_START_MODE_SKIP; + } const hookEventName = typeof input?.hook_event_name === 'string' ? input.hook_event_name : ''; const source = typeof input?.source === 'string' ? input.source.toLowerCase() : ''; @@ log(`[SessionStart] Invalid stdin payload (${error.message}); skipping previous session summary injection. Length: ${raw.length}`); return SESSION_START_MODE_SKIP; } - return null; + log('[SessionStart] Unrecognized stdin payload; skipping previous session summary injection.'); + return SESSION_START_MODE_SKIP; }As per coding guidelines, “Always validate all user input before processing at system boundaries” and “Error messages must not leak sensitive data (credentials, system internals, database details)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/session-start.js` around lines 33 - 55, The function getSessionStartMode currently returns null for non-empty JSON payloads that aren't recognized, which lets main() treat them as eligible for injection; change getSessionStartMode so that after parsing JSON (in the try block) any non-empty but unrecognized payload returns SESSION_START_MODE_SKIP instead of falling through to the final return null. Concretely, in getSessionStartMode after the existing hookName/hook_event_name/source checks, add a branch that when raw (or input) is non-empty and no known conditions matched, returns SESSION_START_MODE_SKIP; keep empty/invalid input behavior unchanged and still return null only for truly empty input. Ensure you reference SESSION_START_MODE_SKIP, getSessionStartMode, hookName, hookEventName, and source when updating the logic.
🧹 Nitpick comments (1)
tests/hooks/hooks.test.js (1)
108-112: Preserve explicit empty fixture metadata.
options.worktree || process.cwd()rewritesworktree: '', so the helper cannot model the legacy “session lacks a recorded worktree” case thatselectMatchingSession()now relies on. Use nullish coalescing so tests can intentionally pass blank metadata.🧪 Proposed test-helper fix
function buildSessionStartFixture(content, options = {}) { - const title = options.title || '# Session'; - const project = options.project || path.basename(process.cwd()); - const worktree = options.worktree || process.cwd(); + const title = options.title ?? '# Session'; + const project = options.project ?? path.basename(process.cwd()); + const worktree = options.worktree ?? process.cwd(); return [title, `**Project:** ${project}`, `**Worktree:** ${worktree}`, '', content, ''].join('\n'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 108 - 112, The helper buildSessionStartFixture uses the logical OR fallback for worktree (options.worktree || process.cwd()), which treats an explicit empty string as falsy and prevents tests from modeling a missing worktree that selectMatchingSession expects; change the fallback to use nullish coalescing so an explicit empty string is preserved (replace the worktree expression with options.worktree ?? process.cwd()), leaving other fields unchanged.
🤖 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 403-405: The log message labels valid non-startup events as
"invalid stdin payload"; update the conditional block that checks
sessionStartMode (the if using SESSION_START_MODE_SKIP and the log(...) call) so
it uses neutral wording and includes the actual mode value instead of "invalid
stdin payload" — e.g., log that injection is being skipped and show
sessionStartMode (or explicitly check a parse-failure sentinel if one exists) so
only a true parse failure is described as "invalid stdin payload".
---
Duplicate comments:
In `@scripts/hooks/session-start.js`:
- Around line 33-55: The function getSessionStartMode currently returns null for
non-empty JSON payloads that aren't recognized, which lets main() treat them as
eligible for injection; change getSessionStartMode so that after parsing JSON
(in the try block) any non-empty but unrecognized payload returns
SESSION_START_MODE_SKIP instead of falling through to the final return null.
Concretely, in getSessionStartMode after the existing
hookName/hook_event_name/source checks, add a branch that when raw (or input) is
non-empty and no known conditions matched, returns SESSION_START_MODE_SKIP; keep
empty/invalid input behavior unchanged and still return null only for truly
empty input. Ensure you reference SESSION_START_MODE_SKIP, getSessionStartMode,
hookName, hookEventName, and source when updating the logic.
---
Nitpick comments:
In `@tests/hooks/hooks.test.js`:
- Around line 108-112: The helper buildSessionStartFixture uses the logical OR
fallback for worktree (options.worktree || process.cwd()), which treats an
explicit empty string as falsy and prevents tests from modeling a missing
worktree that selectMatchingSession expects; change the fallback to use nullish
coalescing so an explicit empty string is preserved (replace the worktree
expression with options.worktree ?? process.cwd()), leaving other fields
unchanged.
🪄 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: e25f784f-e881-4380-8887-2bc7d7cf43c8
📒 Files selected for processing (2)
scripts/hooks/session-start.jstests/hooks/hooks.test.js
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/hooks/session-start.js">
<violation number="1" location="scripts/hooks/session-start.js:48">
P3: Returning `SESSION_START_MODE_SKIP` for valid `clear`/`compact` events causes them to be logged as `invalid stdin payload` at the call site. The log message should use neutral wording (e.g., `"non-startup or invalid stdin payload"`) since `SESSION_START_MODE_SKIP` now covers both legitimate skip-worthy events and actual parse failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const hookEventName = typeof input?.hook_event_name === 'string' ? input.hook_event_name : ''; | ||
| const source = typeof input?.source === 'string' ? input.source.toLowerCase() : ''; | ||
| if (hookEventName === 'SessionStart') { | ||
| if (source === 'resume') return 'resume'; | ||
| if (source === 'startup') return 'startup'; | ||
| if (source === 'clear') return 'clear'; | ||
| if (source === 'compact') return 'compact'; | ||
| } | ||
|
|
||
| return SESSION_START_MODE_SKIP; |
There was a problem hiding this comment.
source silently skips injection for hook_event_name: "SessionStart" payloads
When hookEventName === 'SessionStart' but source is missing or an unrecognized value, none of the inner if branches match and execution falls through to return SESSION_START_MODE_SKIP. In main(), 'skip' is truthy and !== 'startup', so context injection is silently suppressed — the same observable symptom as the resume bug this PR fixes, but for startup sessions.
A concrete trigger: any caller that sends {"hook_event_name": "SessionStart"} without a source field would receive no previous-session context with no warning in the log.
| const hookEventName = typeof input?.hook_event_name === 'string' ? input.hook_event_name : ''; | |
| const source = typeof input?.source === 'string' ? input.source.toLowerCase() : ''; | |
| if (hookEventName === 'SessionStart') { | |
| if (source === 'resume') return 'resume'; | |
| if (source === 'startup') return 'startup'; | |
| if (source === 'clear') return 'clear'; | |
| if (source === 'compact') return 'compact'; | |
| } | |
| return SESSION_START_MODE_SKIP; | |
| const hookEventName = typeof input?.hook_event_name === 'string' ? input.hook_event_name : ''; | |
| const source = typeof input?.source === 'string' ? input.source.toLowerCase() : ''; | |
| if (hookEventName === 'SessionStart') { | |
| if (source === 'resume') return 'resume'; | |
| if (source === 'startup') return 'startup'; | |
| if (source === 'clear') return 'clear'; | |
| if (source === 'compact') return 'compact'; | |
| log(`[SessionStart] Unrecognized source '${source}' in SessionStart payload; defaulting to startup injection`); | |
| return 'startup'; | |
| } | |
| return SESSION_START_MODE_SKIP; |
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 `@scripts/hooks/session-start.js`:
- Line 368: The hook currently reads unbounded stdin with fs.readFileSync(0,
'utf8') before validation; change it to perform a bounded read using a new
MAX_SESSION_START_STDIN_BYTES constant (1MB) and reject oversized input by
returning SESSION_START_MODE_INVALID, mirroring the pattern used in
session-end.js and other hooks; specifically, replace the direct fs.readFileSync
call in session-start.js (and the same pattern in session-start-bootstrap.js)
with a bounded read that checks Buffer.byteLength or reads a limited number of
bytes, enforces MAX_SESSION_START_STDIN_BYTES, and only passes the validated
string into getSessionStartMode(), returning SESSION_START_MODE_INVALID on
overflow.
🪄 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: df5f1059-caa0-47cb-a50f-9c8fbb8f36e0
📒 Files selected for processing (2)
scripts/hooks/session-start.jstests/hooks/hooks.test.js
| } | ||
|
|
||
| async function main() { | ||
| const sessionStartMode = getSessionStartMode(fs.readFileSync(0, 'utf8')); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm SessionStart currently reads fd 0 directly and compare existing stdin-limit patterns.
rg -n -C3 --type=js 'fs\.readFileSync\(0|MAX_STDIN|readStdinJson|stdin.*limit'Repository: affaan-m/everything-claude-code
Length of output: 50387
🏁 Script executed:
#!/bin/bash
# Check the current state of session-start.js around line 368
sed -n '360,375p' scripts/hooks/session-start.js
# Also check session-start-bootstrap.js since it also uses fs.readFileSync(0)
sed -n '40,55p' scripts/hooks/session-start-bootstrap.jsRepository: affaan-m/everything-claude-code
Length of output: 1140
Apply bounded stdin reading to match session-end.js and other hooks.
fs.readFileSync(0, 'utf8') on line 368 reads unbounded external input before getSessionStartMode() can validate it. Per coding guidelines, always validate user input at system boundaries. Implement a MAX_SESSION_START_STDIN_BYTES constant (1MB) and reject oversized payloads as SESSION_START_MODE_INVALID, matching the bounded-read pattern already used in session-end.js, check-console-log.js, post-edit-console-warn.js, and 40+ other hooks.
Note: session-start-bootstrap.js line 46 has the same issue.
🛡️ Proposed bounded-read shape
const SESSION_START_MODE_SKIP = 'skip';
const SESSION_START_MODE_INVALID = 'invalid';
+const MAX_SESSION_START_STDIN_BYTES = 1024 * 1024;
+
+function readSessionStartStdin(maxBytes = MAX_SESSION_START_STDIN_BYTES) {
+ const chunks = [];
+ const buffer = Buffer.allocUnsafe(64 * 1024);
+ let totalBytes = 0;
+
+ while (true) {
+ const bytesRead = fs.readSync(0, buffer, 0, buffer.length, null);
+ if (bytesRead === 0) {
+ return {
+ rawInput: Buffer.concat(chunks, totalBytes).toString('utf8'),
+ exceededLimit: false
+ };
+ }
+
+ totalBytes += bytesRead;
+ if (totalBytes > maxBytes) {
+ return { rawInput: '', exceededLimit: true };
+ }
+
+ chunks.push(Buffer.from(buffer.subarray(0, bytesRead)));
+ }
+}
async function main() {
- const sessionStartMode = getSessionStartMode(fs.readFileSync(0, 'utf8'));
+ const { rawInput, exceededLimit } = readSessionStartStdin();
+ if (exceededLimit) {
+ log(`[SessionStart] Stdin payload exceeded ${MAX_SESSION_START_STDIN_BYTES} byte limit; skipping previous session summary injection.`);
+ }
+ const sessionStartMode = exceededLimit
+ ? SESSION_START_MODE_INVALID
+ : getSessionStartMode(rawInput);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/session-start.js` at line 368, The hook currently reads
unbounded stdin with fs.readFileSync(0, 'utf8') before validation; change it to
perform a bounded read using a new MAX_SESSION_START_STDIN_BYTES constant (1MB)
and reject oversized input by returning SESSION_START_MODE_INVALID, mirroring
the pattern used in session-end.js and other hooks; specifically, replace the
direct fs.readFileSync call in session-start.js (and the same pattern in
session-start-bootstrap.js) with a bounded read that checks Buffer.byteLength or
reads a limited number of bytes, enforces MAX_SESSION_START_STDIN_BYTES, and
only passes the validated string into getSessionStartMode(), returning
SESSION_START_MODE_INVALID on overflow.
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/hooks/session-start.js">
<violation number="1" location="scripts/hooks/session-start.js:55">
P2: When `hook_event_name` is `"SessionStart"` but `source` is missing or an unrecognized value, none of the inner `if` branches match and the function falls through to `return SESSION_START_MODE_SKIP`. In `main()`, `'skip'` is truthy and `!== 'startup'`, so context injection is silently suppressed for what could be a legitimate startup session. Consider defaulting to `'startup'` when the event is `SessionStart` but no recognized `source` is provided, and logging a warning for the unrecognized value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (source === 'compact') return 'compact'; | ||
| } | ||
|
|
||
| return SESSION_START_MODE_SKIP; |
There was a problem hiding this comment.
P2: When hook_event_name is "SessionStart" but source is missing or an unrecognized value, none of the inner if branches match and the function falls through to return SESSION_START_MODE_SKIP. In main(), 'skip' is truthy and !== 'startup', so context injection is silently suppressed for what could be a legitimate startup session. Consider defaulting to 'startup' when the event is SessionStart but no recognized source is provided, and logging a warning for the unrecognized value.
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 55:
<comment>When `hook_event_name` is `"SessionStart"` but `source` is missing or an unrecognized value, none of the inner `if` branches match and the function falls through to `return SESSION_START_MODE_SKIP`. In `main()`, `'skip'` is truthy and `!== 'startup'`, so context injection is silently suppressed for what could be a legitimate startup session. Consider defaulting to `'startup'` when the event is `SessionStart` but no recognized `source` is provided, and logging a warning for the unrecognized value.</comment>
<file context>
@@ -39,20 +40,23 @@ function getSessionStartMode(rawInput) {
+ if (source === 'compact') return 'compact';
}
+
+ return SESSION_START_MODE_SKIP;
} catch (error) {
log(`[SessionStart] Invalid stdin payload (${error.message}); skipping previous session summary injection. Length: ${raw.length}`);
</file context>
| return SESSION_START_MODE_SKIP; | |
| if (hookEventName === 'SessionStart') { | |
| log(`[SessionStart] Unrecognized source '${source}' in SessionStart payload; defaulting to startup injection`); | |
| return 'startup'; | |
| } | |
| return SESSION_START_MODE_SKIP; |
1 participant
Summary
SessionStart:resumerecency-fallbackinjection when no worktree/project match existsWhy
In our Claude Desktop integration, we invoke
sdk.query(...)per user turn while passingresumefor the Claude runtime session. Claude emitsSessionStartagain on those resumed turns, and the current ECC hook re-injects previous-session context each time.That led to two concrete issues:
recency-fallbackWe hit a real case where a new
Claudedesktopsession was seeded with asts2sendboxsummary because the startup hook selected a recent unrelated session file.Notes
This keeps startup injection for matching worktree/project sessions, but avoids polluting resume turns and unrelated projects.
Validation
node tests/hooks/hooks.test.jsSummary by cubic
Scopes previous-session context injection to startup only, and only for sessions that match the current worktree (or project if no worktree is recorded). Adds CLI
SessionStartsupport and fails closed for unrecognized modes; skips injection on resume, clear, compact, and on malformed stdin.SessionStartpayloads.Written for commit d729c0d. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests