diff --git a/src/types/api.ts b/src/types/api.ts index 9c98d2d3..23a97594 100644 --- a/src/types/api.ts +++ b/src/types/api.ts @@ -113,22 +113,6 @@ export function createErrorResponse(code: ApiErrorCode, details?: string): ApiRe }; } -/** - * Response for quick start operation - */ -export interface QuickStartResponse { - /** Whether the request succeeded */ - success: boolean; - /** Created session ID */ - sessionId?: string; - /** Path to case folder */ - casePath?: string; - /** Case name */ - caseName?: string; - /** Error message if failed */ - error?: string; -} - /** * Information about a case folder */ diff --git a/src/web/public/ralph-panel.js b/src/web/public/ralph-panel.js index 25898f4f..5dc8c630 100644 --- a/src/web/public/ralph-panel.js +++ b/src/web/public/ralph-panel.js @@ -1035,7 +1035,7 @@ Object.assign(CodemanApp.prototype, { onclick="app.rollbackToPlanVersion(${item.version})">
v${item.version} - ${item.taskCount || 0} tasks + ${item.stats?.total ?? 0} tasks
${this.formatRelativeTime(item.timestamp)} diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 3896d9a6..0d7ac772 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -304,7 +304,7 @@ Object.assign(CodemanApp.prototype, { try { // Get case path first const caseRes = await fetch(`/api/cases/${caseName}`); - let caseData = (await caseRes.json()).data; + let caseData = (await caseRes.json())?.data ?? {}; // Create the case if it doesn't exist if (!caseData.path) { @@ -452,7 +452,7 @@ Object.assign(CodemanApp.prototype, { try { // Get the case path const caseRes = await fetch(`/api/cases/${caseName}`); - let caseData = (await caseRes.json()).data; + let caseData = (await caseRes.json())?.data ?? {}; // Create the case if it doesn't exist if (!caseData.path) { diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 43b45058..4b7d8d07 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -569,7 +569,7 @@ Object.assign(CodemanApp.prototype, { const res = await this._apiPost('/api/system/update', {}); if (!res || !res.ok) { let msg = 'Failed to start the update.'; - try { const j = await res.json(); if (j?.error?.message) msg = j.error.message; } catch {} + try { const j = await res.json(); if (typeof j?.error === 'string' && j.error) msg = j.error; } catch {} this._setUpdateProgress(`${escapeHtml(msg)}`); if (btn) { btn.disabled = false; btn.textContent = 'Update now'; } return; diff --git a/src/web/routes/clipboard-routes.ts b/src/web/routes/clipboard-routes.ts index fe8b90be..32f22c4b 100644 --- a/src/web/routes/clipboard-routes.ts +++ b/src/web/routes/clipboard-routes.ts @@ -6,13 +6,14 @@ import { FastifyInstance } from 'fastify'; import { SseEvent } from '../sse-events.js'; import type { EventPort } from '../ports/index.js'; +import { createErrorResponse, ApiErrorCode } from '../../types.js'; export function registerClipboardRoutes(app: FastifyInstance, ctx: EventPort): void { app.post('/api/clipboard', async (req) => { const body = req.body as { text?: string; sessionId?: string }; const text = body?.text; if (typeof text !== 'string' || text.length === 0) { - return { success: false, error: 'Missing or empty "text" field' }; + return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Missing or empty "text" field'); } ctx.broadcast(SseEvent.ClipboardWrite, { text, diff --git a/src/web/routes/file-routes.ts b/src/web/routes/file-routes.ts index ef9a9842..2b8051df 100644 --- a/src/web/routes/file-routes.ts +++ b/src/web/routes/file-routes.ts @@ -375,12 +375,15 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort): void }); }); - // Close a file stream + // Close a file stream. Returns { closed } rather than { success: closed } — + // a top-level `success` key would collide with the envelope discriminator + // (the preSerialization hook would pass `{success:false}` through as a + // malformed error envelope instead of wrapping it). app.delete('/api/sessions/:id/tail-file/:streamId', async (req) => { const { id, streamId } = req.params as { id: string; streamId: string }; findSessionOrFail(ctx, id); // Validates session exists const closed = fileStreamManager.closeStream(streamId); - return { success: closed }; + return { closed }; }); // Session-scoped file download. // Uses the same realpath-based workspace boundary as file preview/raw routes; diff --git a/src/web/routes/plan-routes.ts b/src/web/routes/plan-routes.ts index a6fcec53..e1fe3b46 100644 --- a/src/web/routes/plan-routes.ts +++ b/src/web/routes/plan-routes.ts @@ -408,7 +408,7 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; return createErrorResponse(ApiErrorCode.OPERATION_FAILED, 'Ralph tracker not available'); } - return { success: true, data: tracker.getPlanHistory() }; + return { success: true, data: { history: tracker.getPlanHistory(), currentVersion: tracker.planVersion } }; }); // ========== Rollback to Version ========== diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 23a0b14e..9e8d8653 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -1086,9 +1086,10 @@ export function registerSessionRoutes( await ctx.cleanupSession(session.id, true, 'run_prompt_complete'); return { sessionId: session.id, ...result }; } catch (err) { - // Clean up session on error too + // Clean up session on error too. The session is destroyed here, so its id + // is only useful for log correlation — carry it in the error message. await ctx.cleanupSession(session.id, true, 'run_prompt_error'); - return { success: false, sessionId: session.id, error: getErrorMessage(err) }; + return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `${getErrorMessage(err)} (session ${session.id})`); } }); diff --git a/src/web/server.ts b/src/web/server.ts index a0a1c1c0..67f776c2 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -104,6 +104,7 @@ function rewriteApiV1Url(url: string): string { import { getErrorMessage, httpStatusForErrorCode, + createErrorResponse, ApiErrorCode, type PersistedRespawnConfig, type NiceConfig, @@ -738,7 +739,7 @@ export class WebServer extends EventEmitter { this.app.post('/api/events/subscribe', (req, reply) => { const body = (req.body || {}) as { clientId?: string; sessions?: string[] | null }; if (typeof body.clientId !== 'string' || !SSE_CLIENT_ID_RE.test(body.clientId)) { - reply.code(400).send({ error: 'clientId required' }); + reply.code(400).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'clientId required')); return; } const sessions = Array.isArray(body.sessions) @@ -752,6 +753,18 @@ export class WebServer extends EventEmitter { // parseBody. Shared with the route test harness so test behavior matches prod. installRouteErrorHandler(this.app); + // Stable-contract 404 for unknown /api routes — without this, Fastify's + // default not-found payload {message,error,statusCode} would be wrapped by + // the envelope hook into a contradictory HTTP 404 {success:true,...}. + this.app.setNotFoundHandler((req, reply) => { + const notFound = `Route ${req.method}:${req.url} not found`; + if (req.url.startsWith('/api')) { + reply.code(404).send(createErrorResponse(ApiErrorCode.NOT_FOUND, notFound)); + return; + } + reply.code(404).send({ message: notFound, error: 'Not Found', statusCode: 404 }); + }); + // Crash diagnostics beacon — frontend POSTs breadcrumbs, GET to read them. // text/plain is used ONLY by this beacon (navigator.sendBeacon sends text/plain). // Keep the body as a RAW STRING and parse it inside the handler — a global diff --git a/test/http-contract.test.ts b/test/http-contract.test.ts new file mode 100644 index 00000000..74d44e4d --- /dev/null +++ b/test/http-contract.test.ts @@ -0,0 +1,92 @@ +/** + * Live-server tests for the stable HTTP contract (docs/api-reference.md): + * the uniform {success,data} envelope, error envelopes with conventional + * HTTP statuses, the /api/v1 alias, and the /api not-found handler. + * + * These behaviors live in server.ts (preSerialization hook, setNotFoundHandler), + * which the route-test harness does not install — so they need a real WebServer. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { WebServer } from '../src/web/server.js'; + +const PORT = 3168; + +describe('Stable HTTP contract (live server)', () => { + let server: WebServer; + const base = `http://localhost:${PORT}`; + + beforeAll(async () => { + server = new WebServer(PORT, false, true); + await server.start(); + }); + + afterAll(async () => { + await server.stop(); + }); + + it('wraps bare payloads as { success: true, data }', async () => { + const res = await fetch(`${base}/api/status`); + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.success).toBe(true); + expect(body.data).toBeDefined(); + expect(body.data.version).toBeDefined(); + }); + + it('serves the same envelope on the /api/v1 alias', async () => { + const res = await fetch(`${base}/api/v1/status`); + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.success).toBe(true); + expect(body.data.version).toBeDefined(); + }); + + it('maps error envelopes to conventional HTTP statuses', async () => { + const res = await fetch(`${base}/api/sessions/nonexistent/terminal`); + expect(res.status).toBe(404); + const body = await res.json(); + expect(body.success).toBe(false); + expect(typeof body.error).toBe('string'); + expect(body.errorCode).toBe('NOT_FOUND'); + }); + + it('returns a contract-shaped 404 for unknown /api routes', async () => { + const res = await fetch(`${base}/api/this-route-does-not-exist`); + expect(res.status).toBe(404); + const body = await res.json(); + expect(body.success).toBe(false); + expect(body.errorCode).toBe('NOT_FOUND'); + }); + + it('returns a contract-shaped 404 for unknown /api/v1 routes', async () => { + const res = await fetch(`${base}/api/v1/this-route-does-not-exist`); + expect(res.status).toBe(404); + const body = await res.json(); + expect(body.success).toBe(false); + expect(body.errorCode).toBe('NOT_FOUND'); + }); + + it('rejects a bad /api/events/subscribe body with an error envelope', async () => { + const res = await fetch(`${base}/api/events/subscribe`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({}), + }); + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.success).toBe(false); + expect(body.errorCode).toBe('INVALID_INPUT'); + }); + + it('keeps validation errors on the envelope with HTTP 400', async () => { + const res = await fetch(`${base}/api/clipboard`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ text: '' }), + }); + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.success).toBe(false); + expect(body.errorCode).toBe('INVALID_INPUT'); + }); +}); diff --git a/test/routes/file-routes.test.ts b/test/routes/file-routes.test.ts index 903dc7f3..3bc1dc8a 100644 --- a/test/routes/file-routes.test.ts +++ b/test/routes/file-routes.test.ts @@ -363,11 +363,11 @@ describe('file-routes', () => { }); expect(res.statusCode).toBe(200); const body = JSON.parse(res.body); - expect(body.success).toBe(true); + expect(body.closed).toBe(true); expect(mockedFileStreamManager.closeStream).toHaveBeenCalledWith('stream-1'); }); - it('returns false for unknown stream', async () => { + it('returns closed: false for unknown stream', async () => { mockedFileStreamManager.closeStream.mockReturnValue(false); const res = await harness.app.inject({ @@ -376,7 +376,7 @@ describe('file-routes', () => { }); expect(res.statusCode).toBe(200); const body = JSON.parse(res.body); - expect(body.success).toBe(false); + expect(body.closed).toBe(false); }); }); diff --git a/test/routes/plan-routes.test.ts b/test/routes/plan-routes.test.ts index 390a8966..d2f952bf 100644 --- a/test/routes/plan-routes.test.ts +++ b/test/routes/plan-routes.test.ts @@ -235,13 +235,14 @@ describe('plan-routes', () => { expect(body.error).toContain('Ralph tracker'); }); - it('returns plan version history', async () => { + it('returns plan version history with the current version', async () => { const mockHistory = [ { version: 1, timestamp: Date.now() - 60000, itemCount: 10 }, { version: 2, timestamp: Date.now(), itemCount: 12 }, ]; harness.ctx._session.ralphTracker = { getPlanHistory: vi.fn(() => mockHistory), + planVersion: 2, } as never; const res = await harness.app.inject({ @@ -251,8 +252,9 @@ describe('plan-routes', () => { expect(res.statusCode).toBe(200); const body = JSON.parse(res.body); expect(body.success).toBe(true); - expect(body.data).toHaveLength(2); - expect(body.data[1].version).toBe(2); + expect(body.data.history).toHaveLength(2); + expect(body.data.history[1].version).toBe(2); + expect(body.data.currentVersion).toBe(2); }); }); diff --git a/test/scheduled-runs.test.ts b/test/scheduled-runs.test.ts index 67a6aab3..4e1ee55d 100644 --- a/test/scheduled-runs.test.ts +++ b/test/scheduled-runs.test.ts @@ -205,8 +205,14 @@ describe('Quick Run API', () => { const data = await response.json(); // On success the payload is wrapped ({ success:true, data:{ sessionId, ... } }); - // on failure the handler returns a bare { success:false, sessionId, error }. - expect(data.data?.sessionId ?? data.sessionId).toBeDefined(); + // on failure the handler returns the standard error envelope + // ({ success:false, error, errorCode }) with the dead session id in the message. + if (data.success) { + expect(data.data?.sessionId).toBeDefined(); + } else { + expect(data.errorCode).toBeDefined(); + expect(data.error).toMatch(/session /); + } // Note: success/failure depends on Claude actually running });