From cd99cfbc2510e03989369e22fa67b829b950a7a5 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Mon, 8 Jun 2026 06:28:00 +0100 Subject: [PATCH] fix(hub): preserve session metadata across archive transitions (#825) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hub): preserve flavor session ids in metadata across archive transitions When a session ends (terminate, crash, local-launch failure, handoff), the runner's archive write replaces sessions.metadata wholesale. If the CLI's locally cached Metadata is null (e.g. Zod parse failed at bootstrap and api.ts nulled it out) or stale, the spread in archiveAndClose ships a sparse blob and the resume token (cursorSessionId, codexSessionId, claudeSessionId, etc.) gets cleared from the row even though the on-disk chat data is still intact. Fix at the hub layer because update-metadata is the single chokepoint for every metadata write surface (CLI, web, future): in the store-level updateSessionMetadata, read the prior row's metadata inside a transaction and carry forward a small allowlist of flavor resume tokens when the incoming write omits them. Explicit overwrites still win. The allowlist mirrors pickExistingSessionMetadata in sessionFactory.ts which already preserves the same fields on bootstrap. Closes tiann/hapi#820 Co-authored-by: Cursor * fix(hub): address cold-review findings on metadata merge Three bot findings on the initial patch: 1. (P1) Sparse archive payloads still resulted in metadata blobs that failed MetadataSchema parse downstream — required `path`/`host` were not in the carry-forward set, so even though the resume token survived, hub session cache and CLI getSession nulled-out the row and resume_unavailable came back. Add PARSE_IDENTITY_FIELDS = `path`, `host` to the carry-forward. 2. (P2) Preserving `cursorSessionProtocol` whenever it was omitted carried a stale protocol over to a freshly written `cursorSessionId`, misrouting a future remote resume. Pair-aware logic: drop the prior protocol when next sets a new id; preserve the protocol only when next is silent on both id and protocol. 3. (P2) The successful update-metadata broadcast emitted the pre-merge payload to other CLIs in the session room, so even though the DB row was preserved, peer caches diverged. Switch the broadcast value to `result.value` (the persisted merged value) so live caches stay in sync with the truth. Refactor preserveProtocolResumeFields into mergeSessionMetadata with two tiers (PARSE_IDENTITY_FIELDS + SIMPLE_RESUME_TOKENS) plus the cursor pair handler. 6 new tests cover the regressions; existing 16 still pass plus 1 new socket-level test for the broadcast. Co-authored-by: Cursor * fix(hub): preserve flavor + machineId across sparse metadata merges Bot P2 on the prior fix: PARSE_IDENTITY_FIELDS (path, host) made the blob parseable and SIMPLE_RESUME_TOKENS preserved the chat-id, but flavor and machineId were still being dropped by sparse archive payloads. Consequences: - flavor: hub/src/web/routes/sessions.ts and sync/syncEngine.ts read `metadata?.flavor ?? 'claude'` to pick which session id field to resume. With flavor missing, a Cursor/Codex/Gemini session was routed as Claude and the preserved cursorSessionId was ignored. - machineId: telegram/bot.ts and the CLI's resumable listing read `metadata?.machineId` to scope sessions to the current host. With machineId missing, the row dropped out of the resume picker. Add a third carry-forward tier ROUTING_FIELDS = [flavor, machineId] between PARSE_IDENTITY_FIELDS and SIMPLE_RESUME_TOKENS in mergeSessionMetadata. 3 new tests cover preservation, no-invention, and explicit override. Co-authored-by: Cursor * fix(hub,cli): support explicit-clear sentinel for carry-forward fields Upstream cold-review (Major): the carry-forward semantics introduced in the prior commits ("omit field → preserve from prior") collide with cli/src/codex/session.ts resetCodexThread(), which intentionally clears codexSessionId by deleting it from the metadata blob before calling updateMetadata. With omit-as-preserve, the cleared id was restored from the prior row and /clear on a Codex session no longer dropped the persisted thread. Add an explicit-clear sentinel: when next sets a carry-forward field to `null`, the merge drops the key entirely from the persisted blob (key removed; not stored as null since MetadataSchema fields are `string().optional()`). `undefined` (key missing from next) keeps its "carry forward" meaning. The two semantics now compose cleanly: - next.field = "x" → next wins (caller sets a new value) - next.field = null → drop the field (caller intentionally clears) - next omits field → carry forward prior (caller didn't touch it) Update resetCodexThread() to send `codexSessionId: null` so the reset actually drops the persisted thread under the new merge. 4 new hub tests cover: explicit clear of a single token, clear-one- preserve-others independence, no-op clear on a never-set field, and the success-ack value reflects the cleared blob. cli/src/codex tests (224/224) and hub suite (301/301) green; bun typecheck clean. Co-authored-by: Cursor --------- Co-authored-by: Cursor --- cli/src/codex/session.ts | 13 +- .../handlers/cli/sessionHandlers.test.ts | 64 +- .../socket/handlers/cli/sessionHandlers.ts | 8 +- hub/src/store/sessions.test.ts | 759 ++++++++++++++++++ hub/src/store/sessions.ts | 183 ++++- 5 files changed, 996 insertions(+), 31 deletions(-) create mode 100644 hub/src/store/sessions.test.ts diff --git a/cli/src/codex/session.ts b/cli/src/codex/session.ts index 6c53fb17..c8892419 100644 --- a/cli/src/codex/session.ts +++ b/cli/src/codex/session.ts @@ -102,9 +102,16 @@ export class CodexSession extends AgentSessionBase { this.sessionId = null; this.resetTranscriptPath(); this.client.updateMetadata((metadata: Metadata) => { - const updated = { ...metadata }; - delete updated.codexSessionId; - return updated; + // Explicit-clear sentinel: `null` instructs the hub merge to + // drop `codexSessionId` from the persisted blob. Plain + // `delete` arrives at the hub as an omitted field, which the + // carry-forward path then restores from the prior row — + // defeating the reset. See hub/src/store/sessions.ts + // mergeSessionMetadata. The value is `null` on the wire only; + // MetadataSchema parses `string().optional()`, so the + // post-merge persisted blob carries no key. + const updated: Record = { ...metadata, codexSessionId: null }; + return updated as unknown as Metadata; }); } diff --git a/hub/src/socket/handlers/cli/sessionHandlers.test.ts b/hub/src/socket/handlers/cli/sessionHandlers.test.ts index 54d6c761..428299ce 100644 --- a/hub/src/socket/handlers/cli/sessionHandlers.test.ts +++ b/hub/src/socket/handlers/cli/sessionHandlers.test.ts @@ -6,9 +6,9 @@ import { registerSessionHandlers } from './sessionHandlers' class FakeSocket { readonly roomEvents: Array<{ room: string; event: string; data: unknown }> = [] - private readonly handlers = new Map void>() + private readonly handlers = new Map void) => void>() - on(event: string, handler: (data: unknown) => void): this { + on(event: string, handler: (data: unknown, ack?: (response: unknown) => void) => void): this { this.handlers.set(event, handler) return this } @@ -21,8 +21,8 @@ class FakeSocket { } } - trigger(event: string, data: unknown): void { - this.handlers.get(event)?.(data) + trigger(event: string, data: unknown, ack?: (response: unknown) => void): void { + this.handlers.get(event)?.(data, ack) } } @@ -64,4 +64,60 @@ describe('cli session handlers', () => { expect(socket.roomEvents).toHaveLength(0) expect(webEvents).toHaveLength(0) }) + + it('update-metadata broadcasts the merged value, not the pre-merge payload', () => { + const store = new Store(':memory:') + const session = store.sessions.getOrCreateSession( + 'broadcast-merged', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'broadcast-survives' + }, + null, + 'default' + ) + const socket = new FakeSocket() + + registerSessionHandlers(socket as unknown as CliSocketWithData, { + store, + resolveSessionAccess: () => ({ ok: true, value: session as StoredSession }), + emitAccessError: () => { + throw new Error('unexpected access error') + } + }) + + let ackResponse: unknown = null + socket.trigger( + 'update-metadata', + { + sid: session.id, + expectedVersion: session.metadataVersion, + metadata: { + lifecycleState: 'archived', + archivedBy: 'cli', + archiveReason: 'Session crashed' + } + }, + (response) => { + ackResponse = response + } + ) + + // Ack: success and the version bumps; the persisted value carries the + // merged metadata so other CLIs can update their cache to the truth. + const ack = ackResponse as { result: string; version: number; metadata: unknown } + expect(ack.result).toBe('success') + const ackMetadata = ack.metadata as Record + expect(ackMetadata.cursorSessionId).toBe('broadcast-survives') + expect(ackMetadata.path).toBe('/tmp/project') + + // Broadcast: the room event must carry the same merged value. + const broadcast = socket.roomEvents.find((event) => event.event === 'update') + expect(broadcast).toBeDefined() + const broadcastBody = (broadcast?.data as { body: { metadata: { value: Record } } }).body + expect(broadcastBody.metadata.value.cursorSessionId).toBe('broadcast-survives') + expect(broadcastBody.metadata.value.path).toBe('/tmp/project') + expect(broadcastBody.metadata.value.lifecycleState).toBe('archived') + }) }) diff --git a/hub/src/socket/handlers/cli/sessionHandlers.ts b/hub/src/socket/handlers/cli/sessionHandlers.ts index b2809003..67def89c 100644 --- a/hub/src/socket/handlers/cli/sessionHandlers.ts +++ b/hub/src/socket/handlers/cli/sessionHandlers.ts @@ -202,7 +202,13 @@ export function registerSessionHandlers(socket: CliSocketWithData, deps: Session body: { t: 'update-session' as const, sid, - metadata: { version: result.version, value: metadata }, + // Broadcast the persisted (merged) value, not the pre-merge + // payload — otherwise other CLIs in the session room would + // overwrite their local cache with a tokenless metadata + // snapshot even though the DB row was preserved. + // See store.sessions.mergeSessionMetadata for the merge + // contract. + metadata: { version: result.version, value: result.value }, agentState: null } } diff --git a/hub/src/store/sessions.test.ts b/hub/src/store/sessions.test.ts new file mode 100644 index 00000000..f00a0377 --- /dev/null +++ b/hub/src/store/sessions.test.ts @@ -0,0 +1,759 @@ +import { describe, expect, it } from 'bun:test' +import { Store } from './index' + +function makeStore(): Store { + return new Store(':memory:') +} + +function getMetadata(store: Store, id: string): Record | null { + const row = store.sessions.getSession(id) + return (row?.metadata ?? null) as Record | null +} + +describe('updateSessionMetadata: protocol resume token preservation', () => { + it('preserves cursorSessionId when archive payload omits it (Cursor crash-archive)', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-archive-cursor-id', + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + cursorSessionId: 'cursor-thread-abc', + cursorSessionProtocol: 'stream-json', + lifecycleState: 'running' + }, + null, + 'default' + ) + + const result = store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + lifecycleState: 'archived', + lifecycleStateSince: 2, + archivedBy: 'cli', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + expect(result.result).toBe('success') + + const metadata = getMetadata(store, session.id) + expect(metadata).not.toBeNull() + expect(metadata?.cursorSessionId).toBe('cursor-thread-abc') + expect(metadata?.cursorSessionProtocol).toBe('stream-json') + expect(metadata?.lifecycleState).toBe('archived') + expect(metadata?.archiveReason).toBe('Session crashed') + expect(metadata?.archivedBy).toBe('cli') + }) + + it('preserves codexSessionId when archive payload omits it (Codex generic flavor)', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'codex-archive', + { + path: '/tmp/project', + host: 'example', + flavor: 'codex', + codexSessionId: 'codex-thread-1', + lifecycleState: 'running' + }, + null, + 'default' + ) + + const result = store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + flavor: 'codex', + lifecycleState: 'archived', + archivedBy: 'cli', + archiveReason: 'User terminated' + }, + session.metadataVersion, + 'default' + ) + expect(result.result).toBe('success') + + const metadata = getMetadata(store, session.id) + expect(metadata?.codexSessionId).toBe('codex-thread-1') + }) + + it.each([ + ['claudeSessionId', 'claude-thread-x'], + ['codexSessionId', 'codex-thread-x'], + ['geminiSessionId', 'gemini-thread-x'], + ['opencodeSessionId', 'opencode-thread-x'], + ['cursorSessionId', 'cursor-thread-x'], + ['kimiSessionId', 'kimi-thread-x'] + ])('preserves %s across an archive metadata replacement', (field, value) => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + `archive-${field}`, + { + path: '/tmp/project', + host: 'example', + [field]: value + }, + null, + 'default' + ) + + const result = store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + lifecycleState: 'archived', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + expect(result.result).toBe('success') + + const metadata = getMetadata(store, session.id) + expect(metadata?.[field]).toBe(value) + }) + + it('preserves cursorSessionProtocol independently of cursorSessionId', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-protocol-only', + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + cursorSessionProtocol: 'acp' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { path: '/tmp/project', host: 'example' }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) + expect(metadata?.cursorSessionProtocol).toBe('acp') + }) + + it('lets the next write override a flavor session id when it explicitly sets a different value', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-overwrite', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'old-thread' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'new-thread' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) + expect(metadata?.cursorSessionId).toBe('new-thread') + }) + + it('does not invent fields when the prior row had no resume token', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'no-prior-token', + { path: '/tmp/project', host: 'example' }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + lifecycleState: 'archived', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) + expect(metadata).not.toBeNull() + expect('cursorSessionId' in (metadata as Record)).toBe(false) + expect('codexSessionId' in (metadata as Record)).toBe(false) + }) + + it('preserves resume token when CLI sends an empty payload (stale-cache failure mode)', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-empty-payload', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'survives-empty-payload' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + lifecycleState: 'archived', + archivedBy: 'cli', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) + expect(metadata?.cursorSessionId).toBe('survives-empty-payload') + expect(metadata?.lifecycleState).toBe('archived') + }) + + it('preserves resume token across multiple consecutive metadata writes', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-multi-write', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'persistent-thread' + }, + null, + 'default' + ) + + const v1 = store.sessions.updateSessionMetadata( + session.id, + { path: '/tmp/project', host: 'example', name: 'renamed' }, + session.metadataVersion, + 'default' + ) + expect(v1.result).toBe('success') + + const v2 = store.sessions.updateSessionMetadata( + session.id, + { path: '/tmp/project', host: 'example', name: 'renamed', tools: ['read_file'] }, + v1.result === 'success' ? v1.version : -1, + 'default' + ) + expect(v2.result).toBe('success') + + const metadata = getMetadata(store, session.id) + expect(metadata?.cursorSessionId).toBe('persistent-thread') + expect(metadata?.name).toBe('renamed') + expect(metadata?.tools).toEqual(['read_file']) + }) + + it('returns version-mismatch unchanged when the expected version is stale', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-version-mismatch', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'stable-id' + }, + null, + 'default' + ) + + const result = store.sessions.updateSessionMetadata( + session.id, + { path: '/tmp/project', host: 'example' }, + session.metadataVersion + 99, + 'default' + ) + expect(result.result).toBe('version-mismatch') + if (result.result === 'version-mismatch') { + const value = result.value as Record | null + expect(value?.cursorSessionId).toBe('stable-id') + } + }) + + it('returns error when the session row does not exist', () => { + const store = makeStore() + const result = store.sessions.updateSessionMetadata( + 'no-such-session', + { path: '/tmp/project', host: 'example' }, + 0, + 'default' + ) + expect(result.result).toBe('error') + }) + + it('archive then read-back ships a payload that legacy resume routing can use', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-roundtrip', + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + cursorSessionId: 'legacy-uuid', + lifecycleState: 'running' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + lifecycleState: 'archived', + archiveReason: 'Session crashed', + archivedBy: 'cli' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) + // Legacy routing in cursorProtocol.isLegacyCursorSession() defaults to + // legacy when cursorSessionProtocol is unset and cursorSessionId is + // truthy. Preserving the id alone is enough for resume to route + // correctly even if the protocol marker was never persisted. + expect(metadata?.cursorSessionId).toBe('legacy-uuid') + expect(metadata?.flavor).toBe('cursor') + }) + + // P1 from cold review: a sparse archive payload must result in a + // metadata blob that still parses against MetadataSchema (path/host + // are required). Without these, downstream consumers null-out the + // metadata and resume cannot find the session even though the + // resume token survived in the DB. + it('preserves required path and host when archive payload is sparse (sparse-cache failure mode)', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-sparse-archive', + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + cursorSessionId: 'parse-required' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + lifecycleState: 'archived', + archivedBy: 'cli', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.path).toBe('/tmp/project') + expect(metadata?.host).toBe('example') + expect(metadata?.cursorSessionId).toBe('parse-required') + expect(metadata?.lifecycleState).toBe('archived') + }) + + it('does not invent path or host when prior had none', () => { + const store = makeStore() + // create with minimal raw metadata (path is technically required by + // the schema, but the store accepts any JSON; this exercises the + // edge case where prior is missing identity fields) + const session = store.sessions.getOrCreateSession( + 'no-prior-identity', + { flavor: 'cursor' }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { lifecycleState: 'archived' }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.lifecycleState).toBe('archived') + expect('path' in (metadata ?? {})).toBe(false) + expect('host' in (metadata ?? {})).toBe(false) + }) + + // P2 from cold review: flavor + machineId are routing fields. Without + // flavor, hub/src/web/routes/sessions.ts and syncEngine fall through + // to the `?? 'claude'` default and ignore the preserved Cursor/Codex + // token. Without machineId, the CLI's resumable listing filters the + // row out of the resume picker. Both must survive sparse archive. + it('preserves flavor and machineId across sparse archive (resume routing)', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-routing-survives', + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + machineId: 'mach-xyz', + cursorSessionId: 'cursor-thread-routed' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + lifecycleState: 'archived', + archivedBy: 'cli', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.flavor).toBe('cursor') + expect(metadata?.machineId).toBe('mach-xyz') + expect(metadata?.cursorSessionId).toBe('cursor-thread-routed') + }) + + it('does not invent flavor or machineId when prior had none', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'no-prior-routing', + { path: '/tmp/project', host: 'example' }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { lifecycleState: 'archived' }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.lifecycleState).toBe('archived') + expect('flavor' in (metadata ?? {})).toBe(false) + expect('machineId' in (metadata ?? {})).toBe(false) + }) + + it('lets the next write override flavor and machineId when explicitly set', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'override-routing', + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + machineId: 'mach-old' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + flavor: 'codex', + machineId: 'mach-new' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.flavor).toBe('codex') + expect(metadata?.machineId).toBe('mach-new') + }) + + // P2 from cold review: cursorSessionProtocol must NOT carry over + // when the next write explicitly sets a different cursorSessionId. + // The protocol is tied to the id, and a different id may use a + // different protocol (e.g. legacy stream-json id under an old + // ACP marker would be misrouted). + it('drops cursorSessionProtocol when a new cursorSessionId is written', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-protocol-pair-drop', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'old-id', + cursorSessionProtocol: 'acp' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'new-id' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.cursorSessionId).toBe('new-id') + expect(metadata?.cursorSessionProtocol).toBeUndefined() + }) + + it('preserves cursorSessionProtocol when neither id nor protocol is in the next write', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-protocol-pair-preserve', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'stable-id', + cursorSessionProtocol: 'acp' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + lifecycleState: 'archived', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.cursorSessionId).toBe('stable-id') + expect(metadata?.cursorSessionProtocol).toBe('acp') + }) + + it('respects an explicit cursorSessionProtocol on the next write even when the id is unchanged', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-protocol-pair-explicit', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'stable-id' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + cursorSessionProtocol: 'stream-json' + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata?.cursorSessionId).toBe('stable-id') + expect(metadata?.cursorSessionProtocol).toBe('stream-json') + }) + + // P2 from cold review: the broadcast on a successful update must + // ship the merged value so other CLIs in the session room update + // their local cache to the persisted state. This is enforced in the + // socket handler (see hub/src/socket/handlers/cli/sessionHandlers.ts); + // the store-level guarantee here is that result.value reflects the + // merged state and not the pre-merge input. + it('returns the merged value in the success ack, not the pre-merge input', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'cursor-ack-merged', + { + path: '/tmp/project', + host: 'example', + cursorSessionId: 'should-survive-ack' + }, + null, + 'default' + ) + + const result = store.sessions.updateSessionMetadata( + session.id, + { + lifecycleState: 'archived', + archivedBy: 'cli', + archiveReason: 'Session crashed' + }, + session.metadataVersion, + 'default' + ) + + expect(result.result).toBe('success') + if (result.result === 'success') { + const value = result.value as Record | null + expect(value?.path).toBe('/tmp/project') + expect(value?.host).toBe('example') + expect(value?.cursorSessionId).toBe('should-survive-ack') + expect(value?.lifecycleState).toBe('archived') + } + }) + + // Upstream cold-review (Major): preserve-on-omit must not block + // intentional clears. `cli/src/codex/session.ts resetCodexThread()` + // is the existing site that needs to drop `codexSessionId` (called + // from /clear in codexRemoteLauncher.ts). The explicit-clear + // sentinel: `null` in `next` means "drop this field entirely from + // the merged blob" (key removed, not stored as null) — distinct + // from omitted (`undefined`) which carries forward. + it('drops a carry-forward field when next sets it to null (explicit clear)', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'codex-explicit-clear', + { + path: '/tmp/project', + host: 'example', + flavor: 'codex', + codexSessionId: 'old-thread' + }, + null, + 'default' + ) + + const result = store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + flavor: 'codex', + codexSessionId: null + }, + session.metadataVersion, + 'default' + ) + + expect(result.result).toBe('success') + const metadata = getMetadata(store, session.id) as Record | null + expect(metadata).not.toBeNull() + expect('codexSessionId' in (metadata ?? {})).toBe(false) + expect(metadata?.flavor).toBe('codex') + }) + + it('treats null as clear for any carry-forward field, independently of others', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'multi-token-explicit-clear', + { + path: '/tmp/project', + host: 'example', + flavor: 'cursor', + cursorSessionId: 'cursor-keep', + codexSessionId: 'codex-clear-me' + }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + codexSessionId: null + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect('codexSessionId' in (metadata ?? {})).toBe(false) + expect(metadata?.cursorSessionId).toBe('cursor-keep') + expect(metadata?.flavor).toBe('cursor') + }) + + it('null on a never-set field is a no-op (does not introduce the key)', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'null-on-absent', + { path: '/tmp/project', host: 'example' }, + null, + 'default' + ) + + store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + codexSessionId: null + }, + session.metadataVersion, + 'default' + ) + + const metadata = getMetadata(store, session.id) as Record | null + expect('codexSessionId' in (metadata ?? {})).toBe(false) + }) + + it('explicit clear leaves the merged value in the success ack', () => { + const store = makeStore() + const session = store.sessions.getOrCreateSession( + 'explicit-clear-ack', + { + path: '/tmp/project', + host: 'example', + codexSessionId: 'thread-x' + }, + null, + 'default' + ) + + const result = store.sessions.updateSessionMetadata( + session.id, + { + path: '/tmp/project', + host: 'example', + codexSessionId: null + }, + session.metadataVersion, + 'default' + ) + + expect(result.result).toBe('success') + if (result.result === 'success') { + const value = result.value as Record | null + expect('codexSessionId' in (value ?? {})).toBe(false) + expect(value?.path).toBe('/tmp/project') + } + }) +}) diff --git a/hub/src/store/sessions.ts b/hub/src/store/sessions.ts index 82264b83..44921403 100644 --- a/hub/src/store/sessions.ts +++ b/hub/src/store/sessions.ts @@ -5,6 +5,130 @@ import type { StoredSession, VersionedUpdateResult } from './types' import { safeJsonParse } from './json' import { updateVersionedField } from './versionedUpdates' +// Carry-forward fields that the hub preserves across any metadata +// replacement when the incoming write omits them. +// +// The CLI's archive transition (cli/src/agent/runnerLifecycle.ts +// archiveAndClose) spreads `currentMetadata` from the session client's +// local cache; if that cache is `null` (e.g. the row's metadata failed +// Zod parse at bootstrap and got nulled out in cli/src/api/api.ts) or +// stale, the resulting payload is sparse and the unconditional REPLACE +// in updateSessionMetadata wipes whatever it omits. That breaks resume +// even though the on-disk chat data still exists. +// +// Three preservation tiers cover the failure modes: +// +// - PARSE_IDENTITY_FIELDS: required by MetadataSchema in +// shared/src/schemas.ts. Without these, hub session cache and CLI +// getSession reject the row with safeParse → metadata becomes null +// downstream and resume cannot find a path even when the resume +// token survived. +// +// - ROUTING_FIELDS: flavor + machineId. `flavor` is what +// hub/src/web/routes/sessions.ts and hub/src/sync/syncEngine.ts use +// to pick which session id field to read; if it's dropped, the +// `?? 'claude'` fallback misroutes a Cursor/Codex/Gemini session as +// Claude and the preserved token is ignored. `machineId` is the +// filter the CLI's resumable listing uses to scope rows to the +// current host; without it the row drops out of the resume picker. +// +// - SIMPLE_RESUME_TOKENS: flavor-specific resume identifiers that are +// write-once-keep semantics. Mirror of pickExistingSessionMetadata +// in cli/src/agent/sessionFactory.ts. +// +// `cursorSessionProtocol` is paired with `cursorSessionId`: protocol is +// tied to a specific chat id, so a write that explicitly sets a new +// `cursorSessionId` must drop a stale prior protocol. Handled in +// preserveCursorProtocolPair below. +// +// Explicit-clear sentinel: when `next` sets a carry-forward field to +// `null`, the merge drops the key entirely from the output (the +// resulting blob has neither the prior value nor `null`). This lets +// callers intentionally remove a preserved field — e.g. +// `cli/src/codex/session.ts` `resetCodexThread()` clears the codex +// thread id with `codexSessionId: null` so a `/clear` command actually +// drops the persisted thread. `undefined` (key missing from `next`) +// continues to mean "carry forward". +const PARSE_IDENTITY_FIELDS = ['path', 'host'] as const + +const ROUTING_FIELDS = ['flavor', 'machineId'] as const + +const SIMPLE_RESUME_TOKENS = [ + 'claudeSessionId', + 'codexSessionId', + 'geminiSessionId', + 'opencodeSessionId', + 'cursorSessionId', + 'kimiSessionId' +] as const + +function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +function carryForwardIfMissing( + prior: Record, + next: Record, + merged: Record | null, + fields: ReadonlyArray +): Record | null { + let result = merged + for (const field of fields) { + // Explicit-clear sentinel: `null` in next means "drop this field". + // Strip it from the merged output so the persisted blob stays + // schema-clean (MetadataSchema fields are `string().optional()` + // — string|undefined, not nullable). + if (next[field] === null) { + if (result === null) { + result = { ...next } + } + delete result[field] + continue + } + if (next[field] === undefined && prior[field] !== undefined) { + if (result === null) { + result = { ...next } + } + result[field] = prior[field] + } + } + return result +} + +function preserveCursorProtocolPair( + prior: Record, + next: Record, + merged: Record | null +): Record | null { + // If next explicitly sets cursorSessionId, the protocol is tied to + // the new id — never carry over the prior protocol. The next write + // can include its own cursorSessionProtocol if it knows the protocol. + if (next.cursorSessionId !== undefined) { + return merged + } + // Otherwise next is silent on the id (and possibly the protocol); + // carry over the prior protocol so it stays paired with the prior id + // (which is preserved via SIMPLE_RESUME_TOKENS above). + if (next.cursorSessionProtocol === undefined && prior.cursorSessionProtocol !== undefined) { + const result = merged ?? { ...next } + result.cursorSessionProtocol = prior.cursorSessionProtocol + return result + } + return merged +} + +export function mergeSessionMetadata(prior: unknown, next: unknown): unknown { + if (!isPlainObject(prior) || !isPlainObject(next)) { + return next + } + let merged: Record | null = null + merged = carryForwardIfMissing(prior, next, merged, PARSE_IDENTITY_FIELDS) + merged = carryForwardIfMissing(prior, next, merged, ROUTING_FIELDS) + merged = carryForwardIfMissing(prior, next, merged, SIMPLE_RESUME_TOKENS) + merged = preserveCursorProtocolPair(prior, next, merged) + return merged ?? next +} + type DbSessionRow = { id: string tag: string | null @@ -128,29 +252,42 @@ export function updateSessionMetadata( const now = Date.now() const touchUpdatedAt = options?.touchUpdatedAt !== false - return updateVersionedField({ - db, - table: 'sessions', - id, - namespace, - field: 'metadata', - versionField: 'metadata_version', - expectedVersion, - value: metadata, - encode: (value) => { - const json = JSON.stringify(value) - return json === undefined ? null : json - }, - decode: safeJsonParse, - setClauses: [ - 'updated_at = CASE WHEN @touch_updated_at = 1 THEN @updated_at ELSE updated_at END', - 'seq = seq + 1' - ], - params: { - updated_at: now, - touch_updated_at: touchUpdatedAt ? 1 : 0 - } - }) + try { + return db.transaction((): VersionedUpdateResult => { + const priorRow = db.prepare( + 'SELECT metadata FROM sessions WHERE id = ? AND namespace = ?' + ).get(id, namespace) as { metadata: string | null } | undefined + + const prior = priorRow ? safeJsonParse(priorRow.metadata) : null + const merged = mergeSessionMetadata(prior, metadata) + + return updateVersionedField({ + db, + table: 'sessions', + id, + namespace, + field: 'metadata', + versionField: 'metadata_version', + expectedVersion, + value: merged, + encode: (value) => { + const json = JSON.stringify(value) + return json === undefined ? null : json + }, + decode: safeJsonParse, + setClauses: [ + 'updated_at = CASE WHEN @touch_updated_at = 1 THEN @updated_at ELSE updated_at END', + 'seq = seq + 1' + ], + params: { + updated_at: now, + touch_updated_at: touchUpdatedAt ? 1 : 0 + } + }) + })() + } catch { + return { result: 'error' } + } } export function updateSessionAgentState(