fix: address platform source review feedback
Tighten platform source persistence so legacy callers cannot silently relabel existing sessions, repair migration 24 when schema_versions drifts from the real schema, and polish the follow-up UI/error-handler review nits. - only backfill platform_source when it is blank and raise on explicit source conflicts for an existing session - make migration 24 verify both the sdk_sessions column and its index before treating it as applied - expose platform_source from the functional session getters and add regression tests for source preservation and schema drift recovery - add the required APPROVED OVERRIDE annotation for centralized HTTP error translation - keep mobile source pills on a single horizontal row
This commit is contained in:
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -1646,6 +1646,7 @@
|
||||
|
||||
.source-tabs {
|
||||
width: 100%;
|
||||
flex-wrap: nowrap;
|
||||
overflow-x: auto;
|
||||
padding-bottom: 2px;
|
||||
scrollbar-width: none;
|
||||
|
||||
@@ -19,10 +19,10 @@ import { DEFAULT_PLATFORM_SOURCE, normalizePlatformSource, sortPlatformSources }
|
||||
function resolveCreateSessionArgs(
|
||||
customTitle?: string,
|
||||
platformSource?: string
|
||||
): { customTitle?: string; platformSource: string } {
|
||||
): { customTitle?: string; platformSource?: string } {
|
||||
return {
|
||||
customTitle,
|
||||
platformSource: platformSource ?? DEFAULT_PLATFORM_SOURCE
|
||||
platformSource: platformSource ? normalizePlatformSource(platformSource) : undefined
|
||||
};
|
||||
}
|
||||
|
||||
@@ -892,11 +892,13 @@ export class SessionStore {
|
||||
* Add platform_source column to sdk_sessions for Claude/Codex isolation (migration 24)
|
||||
*/
|
||||
private addSessionPlatformSourceColumn(): void {
|
||||
const applied = this.db.prepare('SELECT version FROM schema_versions WHERE version = ?').get(24) as SchemaVersion | undefined;
|
||||
if (applied) return;
|
||||
|
||||
const tableInfo = this.db.query('PRAGMA table_info(sdk_sessions)').all() as TableColumnInfo[];
|
||||
const hasColumn = tableInfo.some(col => col.name === 'platform_source');
|
||||
const indexInfo = this.db.query('PRAGMA index_list(sdk_sessions)').all() as IndexInfo[];
|
||||
const hasIndex = indexInfo.some(index => index.name === 'idx_sdk_sessions_platform_source');
|
||||
const applied = this.db.prepare('SELECT version FROM schema_versions WHERE version = ?').get(24) as SchemaVersion | undefined;
|
||||
|
||||
if (applied && hasColumn && hasIndex) return;
|
||||
|
||||
if (!hasColumn) {
|
||||
this.db.run(`ALTER TABLE sdk_sessions ADD COLUMN platform_source TEXT NOT NULL DEFAULT '${DEFAULT_PLATFORM_SOURCE}'`);
|
||||
@@ -908,7 +910,10 @@ export class SessionStore {
|
||||
SET platform_source = '${DEFAULT_PLATFORM_SOURCE}'
|
||||
WHERE platform_source IS NULL OR platform_source = ''
|
||||
`);
|
||||
this.db.run('CREATE INDEX IF NOT EXISTS idx_sdk_sessions_platform_source ON sdk_sessions(platform_source)');
|
||||
|
||||
if (!hasIndex) {
|
||||
this.db.run('CREATE INDEX IF NOT EXISTS idx_sdk_sessions_platform_source ON sdk_sessions(platform_source)');
|
||||
}
|
||||
|
||||
this.db.prepare('INSERT OR IGNORE INTO schema_versions (version, applied_at) VALUES (?, ?)').run(24, new Date().toISOString());
|
||||
}
|
||||
@@ -1558,12 +1563,12 @@ export class SessionStore {
|
||||
const now = new Date();
|
||||
const nowEpoch = now.getTime();
|
||||
const resolved = resolveCreateSessionArgs(customTitle, platformSource);
|
||||
const normalizedPlatformSource = normalizePlatformSource(resolved.platformSource);
|
||||
const normalizedPlatformSource = resolved.platformSource ?? DEFAULT_PLATFORM_SOURCE;
|
||||
|
||||
// Session reuse: Return existing session ID if already created for this contentSessionId.
|
||||
const existing = this.db.prepare(`
|
||||
SELECT id FROM sdk_sessions WHERE content_session_id = ?
|
||||
`).get(contentSessionId) as { id: number } | undefined;
|
||||
SELECT id, platform_source FROM sdk_sessions WHERE content_session_id = ?
|
||||
`).get(contentSessionId) as { id: number; platform_source: string | null } | undefined;
|
||||
|
||||
if (existing) {
|
||||
// Backfill project if session was created by another hook with empty project
|
||||
@@ -1580,11 +1585,24 @@ export class SessionStore {
|
||||
WHERE content_session_id = ? AND custom_title IS NULL
|
||||
`).run(resolved.customTitle, contentSessionId);
|
||||
}
|
||||
this.db.prepare(`
|
||||
UPDATE sdk_sessions SET platform_source = ?
|
||||
WHERE content_session_id = ?
|
||||
AND COALESCE(platform_source, '') != ?
|
||||
`).run(normalizedPlatformSource, contentSessionId, normalizedPlatformSource);
|
||||
|
||||
if (resolved.platformSource) {
|
||||
const storedPlatformSource = existing.platform_source?.trim()
|
||||
? normalizePlatformSource(existing.platform_source)
|
||||
: undefined;
|
||||
|
||||
if (!storedPlatformSource) {
|
||||
this.db.prepare(`
|
||||
UPDATE sdk_sessions SET platform_source = ?
|
||||
WHERE content_session_id = ?
|
||||
AND COALESCE(platform_source, '') = ''
|
||||
`).run(resolved.platformSource, contentSessionId);
|
||||
} else if (storedPlatformSource !== resolved.platformSource) {
|
||||
throw new Error(
|
||||
`Platform source conflict for session ${contentSessionId}: existing=${storedPlatformSource}, received=${resolved.platformSource}`
|
||||
);
|
||||
}
|
||||
}
|
||||
return existing.id;
|
||||
}
|
||||
|
||||
|
||||
@@ -871,11 +871,13 @@ export class MigrationRunner {
|
||||
* Add platform_source column to sdk_sessions for Claude/Codex isolation (migration 24)
|
||||
*/
|
||||
private addSessionPlatformSourceColumn(): void {
|
||||
const applied = this.db.prepare('SELECT version FROM schema_versions WHERE version = ?').get(24) as SchemaVersion | undefined;
|
||||
if (applied) return;
|
||||
|
||||
const tableInfo = this.db.query('PRAGMA table_info(sdk_sessions)').all() as TableColumnInfo[];
|
||||
const hasColumn = tableInfo.some(col => col.name === 'platform_source');
|
||||
const indexInfo = this.db.query('PRAGMA index_list(sdk_sessions)').all() as IndexInfo[];
|
||||
const hasIndex = indexInfo.some(index => index.name === 'idx_sdk_sessions_platform_source');
|
||||
const applied = this.db.prepare('SELECT version FROM schema_versions WHERE version = ?').get(24) as SchemaVersion | undefined;
|
||||
|
||||
if (applied && hasColumn && hasIndex) return;
|
||||
|
||||
if (!hasColumn) {
|
||||
this.db.run(`ALTER TABLE sdk_sessions ADD COLUMN platform_source TEXT NOT NULL DEFAULT '${DEFAULT_PLATFORM_SOURCE}'`);
|
||||
@@ -887,7 +889,10 @@ export class MigrationRunner {
|
||||
SET platform_source = '${DEFAULT_PLATFORM_SOURCE}'
|
||||
WHERE platform_source IS NULL OR platform_source = ''
|
||||
`);
|
||||
this.db.run('CREATE INDEX IF NOT EXISTS idx_sdk_sessions_platform_source ON sdk_sessions(platform_source)');
|
||||
|
||||
if (!hasIndex) {
|
||||
this.db.run('CREATE INDEX IF NOT EXISTS idx_sdk_sessions_platform_source ON sdk_sessions(platform_source)');
|
||||
}
|
||||
|
||||
this.db.prepare('INSERT OR IGNORE INTO schema_versions (version, applied_at) VALUES (?, ?)').run(24, new Date().toISOString());
|
||||
}
|
||||
|
||||
@@ -10,10 +10,10 @@ import { DEFAULT_PLATFORM_SOURCE, normalizePlatformSource } from '../../../share
|
||||
function resolveCreateSessionArgs(
|
||||
customTitle?: string,
|
||||
platformSource?: string
|
||||
): { customTitle?: string; platformSource: string } {
|
||||
): { customTitle?: string; platformSource?: string } {
|
||||
return {
|
||||
customTitle,
|
||||
platformSource: platformSource ?? DEFAULT_PLATFORM_SOURCE
|
||||
platformSource: platformSource ? normalizePlatformSource(platformSource) : undefined
|
||||
};
|
||||
}
|
||||
|
||||
@@ -39,12 +39,12 @@ export function createSDKSession(
|
||||
const now = new Date();
|
||||
const nowEpoch = now.getTime();
|
||||
const resolved = resolveCreateSessionArgs(customTitle, platformSource);
|
||||
const normalizedPlatformSource = normalizePlatformSource(resolved.platformSource);
|
||||
const normalizedPlatformSource = resolved.platformSource ?? DEFAULT_PLATFORM_SOURCE;
|
||||
|
||||
// Check for existing session
|
||||
const existing = db.prepare(`
|
||||
SELECT id FROM sdk_sessions WHERE content_session_id = ?
|
||||
`).get(contentSessionId) as { id: number } | undefined;
|
||||
SELECT id, platform_source FROM sdk_sessions WHERE content_session_id = ?
|
||||
`).get(contentSessionId) as { id: number; platform_source: string | null } | undefined;
|
||||
|
||||
if (existing) {
|
||||
// Backfill project if session was created by another hook with empty project
|
||||
@@ -61,11 +61,24 @@ export function createSDKSession(
|
||||
WHERE content_session_id = ? AND custom_title IS NULL
|
||||
`).run(resolved.customTitle, contentSessionId);
|
||||
}
|
||||
db.prepare(`
|
||||
UPDATE sdk_sessions SET platform_source = ?
|
||||
WHERE content_session_id = ?
|
||||
AND COALESCE(platform_source, '') != ?
|
||||
`).run(normalizedPlatformSource, contentSessionId, normalizedPlatformSource);
|
||||
|
||||
if (resolved.platformSource) {
|
||||
const storedPlatformSource = existing.platform_source?.trim()
|
||||
? normalizePlatformSource(existing.platform_source)
|
||||
: undefined;
|
||||
|
||||
if (!storedPlatformSource) {
|
||||
db.prepare(`
|
||||
UPDATE sdk_sessions SET platform_source = ?
|
||||
WHERE content_session_id = ?
|
||||
AND COALESCE(platform_source, '') = ''
|
||||
`).run(resolved.platformSource, contentSessionId);
|
||||
} else if (storedPlatformSource !== resolved.platformSource) {
|
||||
throw new Error(
|
||||
`Platform source conflict for session ${contentSessionId}: existing=${storedPlatformSource}, received=${resolved.platformSource}`
|
||||
);
|
||||
}
|
||||
}
|
||||
return existing.id;
|
||||
}
|
||||
|
||||
|
||||
@@ -17,7 +17,9 @@ import type {
|
||||
*/
|
||||
export function getSessionById(db: Database, id: number): SessionBasic | null {
|
||||
const stmt = db.prepare(`
|
||||
SELECT id, content_session_id, memory_session_id, project, user_prompt, custom_title
|
||||
SELECT id, content_session_id, memory_session_id, project,
|
||||
COALESCE(platform_source, 'claude') as platform_source,
|
||||
user_prompt, custom_title
|
||||
FROM sdk_sessions
|
||||
WHERE id = ?
|
||||
LIMIT 1
|
||||
@@ -38,7 +40,9 @@ export function getSdkSessionsBySessionIds(
|
||||
|
||||
const placeholders = memorySessionIds.map(() => '?').join(',');
|
||||
const stmt = db.prepare(`
|
||||
SELECT id, content_session_id, memory_session_id, project, user_prompt, custom_title,
|
||||
SELECT id, content_session_id, memory_session_id, project,
|
||||
COALESCE(platform_source, 'claude') as platform_source,
|
||||
user_prompt, custom_title,
|
||||
started_at, started_at_epoch, completed_at, completed_at_epoch, status
|
||||
FROM sdk_sessions
|
||||
WHERE memory_session_id IN (${placeholders})
|
||||
|
||||
@@ -12,6 +12,7 @@ export interface SessionBasic {
|
||||
content_session_id: string;
|
||||
memory_session_id: string | null;
|
||||
project: string;
|
||||
platform_source: string;
|
||||
user_prompt: string;
|
||||
custom_title: string | null;
|
||||
}
|
||||
@@ -24,6 +25,7 @@ export interface SessionFull {
|
||||
content_session_id: string;
|
||||
memory_session_id: string;
|
||||
project: string;
|
||||
platform_source: string;
|
||||
user_prompt: string;
|
||||
custom_title: string | null;
|
||||
started_at: string;
|
||||
|
||||
@@ -79,6 +79,8 @@ export abstract class BaseRouteHandler {
|
||||
* Checks headersSent to avoid "Cannot set headers after they are sent" errors
|
||||
*/
|
||||
protected handleError(res: Response, error: Error, context?: string): void {
|
||||
// [APPROVED OVERRIDE]: Worker routes need centralized AppError translation so
|
||||
// status/code/details stay consistent across every HTTP handler.
|
||||
logger.failure('WORKER', context || 'Request failed', {}, error);
|
||||
if (!res.headersSent) {
|
||||
const statusCode = error instanceof AppError ? error.statusCode : 500;
|
||||
|
||||
@@ -1646,6 +1646,7 @@
|
||||
|
||||
.source-tabs {
|
||||
width: 100%;
|
||||
flex-wrap: nowrap;
|
||||
overflow-x: auto;
|
||||
padding-bottom: 2px;
|
||||
scrollbar-width: none;
|
||||
|
||||
@@ -22,6 +22,10 @@ interface TableColumnInfo {
|
||||
notnull: number;
|
||||
}
|
||||
|
||||
interface IndexInfo {
|
||||
name: string;
|
||||
}
|
||||
|
||||
interface SchemaVersion {
|
||||
version: number;
|
||||
}
|
||||
@@ -46,6 +50,11 @@ function getSchemaVersions(db: Database): number[] {
|
||||
return rows.map(r => r.version);
|
||||
}
|
||||
|
||||
function getIndexNames(db: Database, table: string): string[] {
|
||||
const rows = db.prepare(`PRAGMA index_list(${table})`).all() as IndexInfo[];
|
||||
return rows.map(r => r.name);
|
||||
}
|
||||
|
||||
describe('MigrationRunner', () => {
|
||||
let db: Database;
|
||||
|
||||
@@ -158,6 +167,43 @@ describe('MigrationRunner', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('schema drift recovery for migration 24', () => {
|
||||
it('should repair platform_source column and index even when version 24 is already recorded', () => {
|
||||
db.run(`
|
||||
CREATE TABLE IF NOT EXISTS schema_versions (
|
||||
id INTEGER PRIMARY KEY,
|
||||
version INTEGER UNIQUE NOT NULL,
|
||||
applied_at TEXT NOT NULL
|
||||
)
|
||||
`);
|
||||
db.prepare('INSERT INTO schema_versions (version, applied_at) VALUES (?, ?)').run(24, new Date().toISOString());
|
||||
|
||||
db.run(`
|
||||
CREATE TABLE sdk_sessions (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
content_session_id TEXT UNIQUE NOT NULL,
|
||||
memory_session_id TEXT UNIQUE,
|
||||
project TEXT NOT NULL,
|
||||
user_prompt TEXT,
|
||||
started_at TEXT NOT NULL,
|
||||
started_at_epoch INTEGER NOT NULL,
|
||||
completed_at TEXT,
|
||||
completed_at_epoch INTEGER,
|
||||
status TEXT NOT NULL CHECK(status IN ('active','completed','failed'))
|
||||
)
|
||||
`);
|
||||
|
||||
const runner = new MigrationRunner(db);
|
||||
expect(() => runner.runAllMigrations()).not.toThrow();
|
||||
|
||||
const columnNames = getColumns(db, 'sdk_sessions').map(column => column.name);
|
||||
expect(columnNames).toContain('platform_source');
|
||||
|
||||
const indexNames = getIndexNames(db, 'sdk_sessions');
|
||||
expect(indexNames).toContain('idx_sdk_sessions_platform_source');
|
||||
});
|
||||
});
|
||||
|
||||
describe('issue #979 — old DatabaseManager version conflict', () => {
|
||||
it('should create core tables even when old migration versions 1-7 are in schema_versions', () => {
|
||||
// Simulate the old DatabaseManager having applied its migrations 1-7
|
||||
|
||||
@@ -130,6 +130,38 @@ describe('Sessions Module', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('platform_source', () => {
|
||||
it('should default new sessions to claude when platformSource is omitted', () => {
|
||||
const sessionId = createSDKSession(db, 'session-platform-1', 'project', 'prompt');
|
||||
const session = getSessionById(db, sessionId);
|
||||
|
||||
expect(session?.platform_source).toBe('claude');
|
||||
});
|
||||
|
||||
it('should preserve a non-default platform_source for legacy callers that omit platformSource', () => {
|
||||
const sessionId = createSDKSession(db, 'session-platform-2', 'project', 'prompt', undefined, 'codex');
|
||||
let session = getSessionById(db, sessionId);
|
||||
expect(session?.platform_source).toBe('codex');
|
||||
|
||||
createSDKSession(db, 'session-platform-2', 'project', 'prompt');
|
||||
session = getSessionById(db, sessionId);
|
||||
expect(session?.platform_source).toBe('codex');
|
||||
});
|
||||
|
||||
it('should reject explicit platform_source conflicts for the same session', () => {
|
||||
createSDKSession(db, 'session-platform-3', 'project', 'prompt', undefined, 'codex');
|
||||
|
||||
expect(() => createSDKSession(
|
||||
db,
|
||||
'session-platform-3',
|
||||
'project',
|
||||
'prompt',
|
||||
undefined,
|
||||
'claude'
|
||||
)).toThrow(/Platform source conflict/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('updateMemorySessionId', () => {
|
||||
it('should update memory_session_id for existing session', () => {
|
||||
const contentSessionId = 'content-session-update';
|
||||
|
||||
Reference in New Issue
Block a user