fix(server): resolve SDK session ID for headless resume

[dimakis]

Summary

• Headless sessions (created via POST /api/sessions with initialPrompt) fail silently — Claude never responds
• Root cause: query loop passes the worktree session ID (e.g. 2026-05-22-xxxx) to the SDK's --resume, but the SDK expects a UUID
• Add getSessionSdkId() to look up the stored SDK UUID from the session index
• Use it in chat.ts when constructing the resume parameter, falling back to the original value

Context

PR Shepherd agent in mgmt creates per-PR Mitzo sessions via POST /api/sessions. Sessions were created successfully but Claude never executed because the resume call failed with:

Error: --resume requires a valid session ID or session title.
Provided value "2026-05-22-41327e2d562c" is not a UUID.

Test plan

• Create a headless session via POST /api/sessions with initialPrompt
• Verify Claude responds in the session
• Verify session appears on mobile with assistant reply
• Verify normal (non-headless) session resume still works

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/session-index.ts

Correct fix — resolves SDK session ID for headless resume with a safe fallback. Main gap is missing unit tests for session-index.ts (project requires TDD). Minor style and consistency nits.

• 🟡 missing_tests (L163): No test file exists for session-index.ts at all. The new getSessionSdkId function (and the existing updateSessionSdkId) lack unit tests. Given the project's TDD requirement, this should have tests covering: (1) returns undefined when entry doesn't exist, (2) returns undefined when sdk_session_id is not set, (3) returns the stored SDK session ID when present. [fixable]
• 🔵 style (L159): The JSDoc comment on getSessionSdkId is a multi-line block that just restates what the type signature already says (returns string | undefined). Per project conventions, prefer no comment or a single-line comment only when the WHY is non-obvious. [fixable]

server/chat.ts

Correct fix — resolves SDK session ID for headless resume with a safe fallback. Main gap is missing unit tests for session-index.ts (project requires TDD). Minor style and consistency nits.

• 🔵 unsafe_assumptions (L907): getSessionSdkId(BASE_REPO, ...) is called without guarding that BASE_REPO is non-empty. BASE_REPO is process.env.REPO_PATH || ''. Other resume-related code in this file guards with if (options.resume && BASE_REPO) (e.g., line 637). When BASE_REPO is empty, readIndex('') constructs a path relative to CWD (./.claude/sessions/index.yaml), which will return [] and the ?? fallback safely fires — so this isn't a bug, but it performs a needless filesystem read. A guard like (BASE_REPO && getSessionSdkId(BASE_REPO, options.resume)) ?? options.resume would be more consistent with the rest of the file. [fixable]

[dimakis]

🟡 missing_tests: No test file exists for session-index.ts at all. The new getSessionSdkId function (and the existing updateSessionSdkId) lack unit tests. Given the project's TDD requirement, this should have tests covering: (1) returns undefined when entry doesn't exist, (2) returns undefined when sdk_session_id is not set, (3) returns the stored SDK session ID when present. [fixable]

[dimakis]

🔵 style: The JSDoc comment on getSessionSdkId is a multi-line block that just restates what the type signature already says (returns string | undefined). Per project conventions, prefer no comment or a single-line comment only when the WHY is non-obvious. [fixable]

[dimakis]

🔵 unsafe_assumptions: getSessionSdkId(BASE_REPO, ...) is called without guarding that BASE_REPO is non-empty. BASE_REPO is process.env.REPO_PATH || ''. Other resume-related code in this file guards with if (options.resume && BASE_REPO) (e.g., line 637). When BASE_REPO is empty, readIndex('') constructs a path relative to CWD (./.claude/sessions/index.yaml), which will return [] and the ?? fallback safely fires — so this isn't a bug, but it performs a needless filesystem read. A guard like (BASE_REPO && getSessionSdkId(BASE_REPO, options.resume)) ?? options.resume would be more consistent with the rest of the file. [fixable]

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/chat.ts

Clean, well-scoped fix. The SDK session ID resolution logic is correct with proper fallback, the new getSessionSdkId function is well-tested, and the model preference sync addresses a real bug. Main gap is the lack of integration-level tests for the resume resolution path in chat.ts.

• 🟡 missing_tests (L912): The SDK session ID resolution logic in _startChatInner (line 914) is not tested at integration level. The ws-handler-v2 tests mock startChat entirely, so the getSessionSdkId lookup path is never exercised end-to-end. A unit test for _startChatInner (or at minimum a test that verifies the resume value passed to query() is the resolved SDK UUID rather than the raw worktree ID) would catch regressions in this mapping. [fixable]
• 🔵 unsafe_assumptions (L914): BASE_REPO is process.env.REPO_PATH || '' (line 101). The empty-string fallback is falsy, so the && guard works correctly to skip the lookup when REPO_PATH is unset. However, this means the feature silently degrades — resume will pass the worktree ID directly to the SDK (which may fail) instead of surfacing a clear error. This is acceptable for now since REPO_PATH is documented as required, but worth noting.

frontend/src/client-store.ts

Clean, well-scoped fix. The SDK session ID resolution logic is correct with proper fallback, the new getSessionSdkId function is well-tested, and the model preference sync addresses a real bug. Main gap is the lack of integration-level tests for the resume resolution path in chat.ts.

• 🔵 style (L30): The comment // Sync localStorage model preference into the store so sendMessage() includes it explains the 'why' well, which is good. However, this is a module-level side effect that runs on import. If client-store.ts is ever imported in a test or SSR context, it will silently read from localStorage. Current usage is fine (single-page app entry point), but if this file's import surface grows, consider guarding with a typeof window !== 'undefined' check.

[dimakis]

🟡 missing_tests: The SDK session ID resolution logic in _startChatInner (line 914) is not tested at integration level. The ws-handler-v2 tests mock startChat entirely, so the getSessionSdkId lookup path is never exercised end-to-end. A unit test for _startChatInner (or at minimum a test that verifies the resume value passed to query() is the resolved SDK UUID rather than the raw worktree ID) would catch regressions in this mapping. [fixable]

[dimakis]

🔵 unsafe_assumptions: BASE_REPO is process.env.REPO_PATH || '' (line 101). The empty-string fallback is falsy, so the && guard works correctly to skip the lookup when REPO_PATH is unset. However, this means the feature silently degrades — resume will pass the worktree ID directly to the SDK (which may fail) instead of surfacing a clear error. This is acceptable for now since REPO_PATH is documented as required, but worth noting.

[dimakis]

🔵 style: The comment // Sync localStorage model preference into the store so sendMessage() includes it explains the 'why' well, which is good. However, this is a module-level side effect that runs on import. If client-store.ts is ever imported in a test or SSR context, it will silently read from localStorage. Current usage is fine (single-page app entry point), but if this file's import surface grows, consider guarding with a typeof window !== 'undefined' check.

[dimakis]

Centaur Review

Found 3 issue(s) (1 critical) (1 warning).

server/chat.ts

The core getSessionSdkId helper and tests are clean, but the fallback expression in chat.ts has a subtle ?? vs || bug when REPO_PATH is unset (empty string is not coalesced by ??).

• 🔴 bugs (L914): (BASE_REPO && getSessionSdkId(BASE_REPO, options.resume)) ?? options.resume — when BASE_REPO is '' (REPO_PATH unset), the && short-circuits to '', and ?? does NOT coalesce empty strings (only null/undefined), so resume becomes '' instead of falling back to options.resume. Use || instead of ??, or restructure: (BASE_REPO ? getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume. [fixable]
• 🟡 missing_tests (L912): The SDK session ID resolution logic in _startChatInner has no unit test. The ws-handler-v2 tests mock startChat entirely, so the getSessionSdkId lookup is never exercised. A test (even with mocked getSessionSdkId) verifying the correct value reaches query() options would catch the ?? vs || bug above. [fixable]

frontend/src/client-store.ts

The core getSessionSdkId helper and tests are clean, but the fallback expression in chat.ts has a subtle ?? vs || bug when REPO_PATH is unset (empty string is not coalesced by ??).

• 🔵 style (L31): The model preference sync is tangentially related to the headless resume fix. Consider splitting it into its own PR (it already has its own commit c4cfe5e) to keep the PR focused on the session ID resolution.

[dimakis]

🔴 bugs: (BASE_REPO && getSessionSdkId(BASE_REPO, options.resume)) ?? options.resume — when BASE_REPO is '' (REPO_PATH unset), the && short-circuits to '', and ?? does NOT coalesce empty strings (only null/undefined), so resume becomes '' instead of falling back to options.resume. Use || instead of ??, or restructure: (BASE_REPO ? getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume. [fixable]

[dimakis]

🟡 missing_tests: The SDK session ID resolution logic in _startChatInner has no unit test. The ws-handler-v2 tests mock startChat entirely, so the getSessionSdkId lookup is never exercised. A test (even with mocked getSessionSdkId) verifying the correct value reaches query() options would catch the ?? vs || bug above. [fixable]

[dimakis]

🔵 style: The model preference sync is tangentially related to the headless resume fix. Consider splitting it into its own PR (it already has its own commit c4cfe5e) to keep the PR focused on the session ID resolution.

[dimakis]

Centaur Review

Found 4 issue(s) (1 critical) (1 warning).

server/chat.ts

Critical bug: when REPO_PATH is unset, && short-circuits to '' which ?? doesn't catch, so resume gets an empty string instead of the original worktree ID. The rest of the PR is sound — getSessionSdkId is well-tested and the frontend model-sync fix is correct.

• 🔴 bugs (L918): When BASE_REPO is '' (REPO_PATH unset), '' && getSessionSdkId(...) evaluates to '', and '' ?? options.resume returns '' (since ?? only checks null/undefined, not falsy values). This passes an empty string as the resume value instead of falling back to options.resume. Use a ternary instead: (BASE_REPO ? getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume. [fixable]
• 🔵 style (L912): The IIFE (() => { ... })() inside a spread is harder to read than a simple ternary or a local variable computed before the query() call. Consider extracting const resolvedResume = options.resume ? (BASE_REPO ? getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume : undefined above the call and spreading ...(resolvedResume ? { resume: resolvedResume } : {}). [fixable]

server/__tests__/chat.test.ts

Critical bug: when REPO_PATH is unset, && short-circuits to '' which ?? doesn't catch, so resume gets an empty string instead of the original worktree ID. The rest of the PR is sound — getSessionSdkId is well-tested and the frontend model-sync fix is correct.

• 🟡 missing_tests (L335): The 'resume resolves SDK session ID' tests are source-text assertions (string-matching chat.ts source code). While this pattern is established in the file for other hard-to-mock SDK flows, these tests won't catch the ?? vs || bug on line 918 — they verify substring presence but not runtime semantics. A unit test for getSessionSdkId itself (which exists in session-index.test.ts) is solid, but the integration between the lookup and the fallback is only structurally verified, not behaviorally.

frontend/src/client-store.ts

Critical bug: when REPO_PATH is unset, && short-circuits to '' which ?? doesn't catch, so resume gets an empty string instead of the original worktree ID. The rest of the PR is sound — getSessionSdkId is well-tested and the frontend model-sync fix is correct.

• 🔵 missing_tests (L31): The new setModel(getPreferredModel()) bootstrap has no test coverage. A unit test verifying that clientStore.getState().config.modelId matches the localStorage value after module initialization would lock in the behavior. [fixable]

[dimakis]

🔴 bugs: When BASE_REPO is '' (REPO_PATH unset), '' && getSessionSdkId(...) evaluates to '', and '' ?? options.resume returns '' (since ?? only checks null/undefined, not falsy values). This passes an empty string as the resume value instead of falling back to options.resume. Use a ternary instead: (BASE_REPO ? getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume. [fixable]

[dimakis]

🔵 style: The IIFE (() => { ... })() inside a spread is harder to read than a simple ternary or a local variable computed before the query() call. Consider extracting const resolvedResume = options.resume ? (BASE_REPO ? getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume : undefined above the call and spreading ...(resolvedResume ? { resume: resolvedResume } : {}). [fixable]

[dimakis]

🟡 missing_tests: The 'resume resolves SDK session ID' tests are source-text assertions (string-matching chat.ts source code). While this pattern is established in the file for other hard-to-mock SDK flows, these tests won't catch the ?? vs || bug on line 918 — they verify substring presence but not runtime semantics. A unit test for getSessionSdkId itself (which exists in session-index.test.ts) is solid, but the integration between the lookup and the fallback is only structurally verified, not behaviorally.

[dimakis]

🔵 missing_tests: The new setModel(getPreferredModel()) bootstrap has no test coverage. A unit test verifying that clientStore.getState().config.modelId matches the localStorage value after module initialization would lock in the behavior. [fixable]

[dimakis]

Centaur Review

Found 2 issue(s).

server/__tests__/chat.test.ts

Clean, well-scoped fix. The core logic (getSessionSdkId + resolvedResume fallback with ternary guard) is correct, tests follow established source-reading patterns, and backward compatibility is preserved via nullish coalescing fallback. Two minor suggestions, no blocking issues.

• 🔵 style (L335): The earlier source-reading test block (line 285-288) has a rationale comment explaining why behavioral testing is impractical and source-code assertions are used instead. This new describe block follows the same pattern but omits the rationale, making it less obvious to future readers why these tests read raw source instead of exercising the function. [fixable]

frontend/src/client-store.ts

Clean, well-scoped fix. The core logic (getSessionSdkId + resolvedResume fallback with ternary guard) is correct, tests follow established source-reading patterns, and backward compatibility is preserved via nullish coalescing fallback. Two minor suggestions, no blocking issues.

• 🔵 missing_tests (L31): The typeof window guard is a reasonable defensive fix (likely for Vitest imports in a non-browser environment), but there is no corresponding test verifying that importing client-store.ts in a windowless context no longer throws. If this guard was added to fix a specific failure, a regression test would lock it in. [fixable]

[dimakis]

🔵 style: The earlier source-reading test block (line 285-288) has a rationale comment explaining why behavioral testing is impractical and source-code assertions are used instead. This new describe block follows the same pattern but omits the rationale, making it less obvious to future readers why these tests read raw source instead of exercising the function. [fixable]

[dimakis]

🔵 missing_tests: The typeof window guard is a reasonable defensive fix (likely for Vitest imports in a non-browser environment), but there is no corresponding test verifying that importing client-store.ts in a windowless context no longer throws. If this guard was added to fix a specific failure, a regression test would lock it in. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

server/__tests__/chat.test.ts

Clean, well-scoped fix: adds a simple lookup function, correctly handles the empty-string falsy edge case, and has reasonable test coverage via structural assertions consistent with the project's existing patterns. No bugs or regressions found.

• 🔵 style (L350): The comment '// Must use ternary (BASE_REPO ? ...) not && (which returns '' for empty string)' is helpful rationale, but the assertion expect(chatSource).toContain('BASE_REPO ? getSessionSdkId(BASE_REPO') is fragile — any whitespace or formatting change (e.g. Prettier wrapping the line) will break this test without any behavioral change. This is an inherent limitation of all the structural tests here, but this particular assertion is especially tight since it spans across a ternary expression.
• 🔵 missing_tests (L337): The structural tests verify that resume resolution code exists in the source text, but there is no behavioral test confirming that when options.resume is a worktree ID with a stored sdk_session_id, the correct UUID actually reaches the SDK query() call. The structural tests are a reasonable pragmatic choice given the difficulty of mocking the Agent SDK (and the project already uses this pattern), but a comment acknowledging this gap or a future TODO would help.

server/chat.ts

Clean, well-scoped fix: adds a simple lookup function, correctly handles the empty-string falsy edge case, and has reasonable test coverage via structural assertions consistent with the project's existing patterns. No bugs or regressions found.

• 🟡 unsafe_assumptions (L901): getSessionSdkId performs synchronous YAML file I/O (readFileSync inside readIndex) on every resume call. This is fine for the current call volume, but if resume frequency increases, this will block the event loop. The existing updateSessionSdkId/readIndex callers already do synchronous I/O, so this is consistent with the codebase — flagging it as a known trade-off, not a blocker.

frontend/src/__tests__/client-store.test.ts

Clean, well-scoped fix: adds a simple lookup function, correctly handles the empty-string falsy edge case, and has reasonable test coverage via structural assertions consistent with the project's existing patterns. No bugs or regressions found.

• 🔵 style (L12): import.meta.dirname is a Node 21+ feature (and supported by Vitest). The server tests already use it, so this is consistent. Just noting it requires Node >= 21 at test-time.

[dimakis]

🔵 style: The comment '// Must use ternary (BASE_REPO ? ...) not && (which returns '' for empty string)' is helpful rationale, but the assertion expect(chatSource).toContain('BASE_REPO ? getSessionSdkId(BASE_REPO') is fragile — any whitespace or formatting change (e.g. Prettier wrapping the line) will break this test without any behavioral change. This is an inherent limitation of all the structural tests here, but this particular assertion is especially tight since it spans across a ternary expression.

[dimakis]

🔵 missing_tests: The structural tests verify that resume resolution code exists in the source text, but there is no behavioral test confirming that when options.resume is a worktree ID with a stored sdk_session_id, the correct UUID actually reaches the SDK query() call. The structural tests are a reasonable pragmatic choice given the difficulty of mocking the Agent SDK (and the project already uses this pattern), but a comment acknowledging this gap or a future TODO would help.

[dimakis]

🟡 unsafe_assumptions: getSessionSdkId performs synchronous YAML file I/O (readFileSync inside readIndex) on every resume call. This is fine for the current call volume, but if resume frequency increases, this will block the event loop. The existing updateSessionSdkId/readIndex callers already do synchronous I/O, so this is consistent with the codebase — flagging it as a known trade-off, not a blocker.

[dimakis]

🔵 style: import.meta.dirname is a Node 21+ feature (and supported by Vitest). The server tests already use it, so this is consistent. Just noting it requires Node >= 21 at test-time.

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/__tests__/chat.test.ts

Clean, well-structured fix with appropriate fallback semantics. The core logic is correct: worktree IDs are resolved to SDK session UUIDs before being passed to query(), with safe fallback to the raw ID. The structural test pattern matches existing conventions. Minor suggestions around test specificity and observability on the fallback path.

• 🔵 style (L355): The assertion expect(chatSource).toContain('?? options.resume') is fragile — it matches any occurrence of ?? options.resume anywhere in chat.ts, not specifically in the resume resolution block. If another use of ?? options.resume is added elsewhere, this test would still pass even if the actual resolution fallback were removed. Consider matching a more specific string like getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume to anchor it to the correct code path. [fixable]
• 🔵 missing_tests (L337): There is no structural test asserting that the resolved UUID (not the worktree ID) is logged or traceable on fallback. When getSessionSdkId returns undefined and the raw worktree ID is used, the only signal is the absence of the UUID — no log line distinguishes 'resolved successfully' from 'fell back to worktree ID'. A debug/info log on successful resolution would aid troubleshooting and could be structurally asserted. [fixable]

server/chat.ts

Clean, well-structured fix with appropriate fallback semantics. The core logic is correct: worktree IDs are resolved to SDK session UUIDs before being passed to query(), with safe fallback to the raw ID. The structural test pattern matches existing conventions. Minor suggestions around test specificity and observability on the fallback path.

• 🟡 bugs (L900): The getSessionSdkId call is synchronous I/O (reads YAML from disk via readFileSync inside readIndex). This runs on the main event loop inside an async function that could have used an async read. For a single resume this is unlikely to cause issues, but it's worth noting — if the session index file grows large or disk is slow, this blocks the event loop. Not critical for current usage patterns but worth considering if resume frequency increases. [fixable]

[dimakis]

🔵 style: The assertion expect(chatSource).toContain('?? options.resume') is fragile — it matches any occurrence of ?? options.resume anywhere in chat.ts, not specifically in the resume resolution block. If another use of ?? options.resume is added elsewhere, this test would still pass even if the actual resolution fallback were removed. Consider matching a more specific string like getSessionSdkId(BASE_REPO, options.resume) : undefined) ?? options.resume to anchor it to the correct code path. [fixable]

[dimakis]

🔵 missing_tests: There is no structural test asserting that the resolved UUID (not the worktree ID) is logged or traceable on fallback. When getSessionSdkId returns undefined and the raw worktree ID is used, the only signal is the absence of the UUID — no log line distinguishes 'resolved successfully' from 'fell back to worktree ID'. A debug/info log on successful resolution would aid troubleshooting and could be structurally asserted. [fixable]

[dimakis]

🟡 bugs: The getSessionSdkId call is synchronous I/O (reads YAML from disk via readFileSync inside readIndex). This runs on the main event loop inside an async function that could have used an async read. For a single resume this is unlikely to cause issues, but it's worth noting — if the session index file grows large or disk is slow, this blocks the event loop. Not critical for current usage patterns but worth considering if resume frequency increases. [fixable]