mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-09 08:59:40 +02:00
fix(api): close contract gaps found by post-merge adversarial audit
A 15-agent audit of the merged tree confirmed 9 envelope/contract bugs;
all fixed here, with live-server contract tests added:
Blockers (fresh-install quick start broken):
- session-ui.js runClaude/runShell unwrapped .data from the /api/cases/:name
404 error envelope (which has no data key), so a not-yet-created case threw
TypeError instead of triggering the auto-create fallback. Now '?.data ?? {}'.
Contract violations on the new stable surface:
- Unknown /api routes returned HTTP 404 with {success:true,...} (Fastify's
default not-found payload was wrapped by the envelope hook). Added a
setNotFoundHandler returning the standard error envelope for /api paths.
- POST /api/events/subscribe 400 body became {success:true,data:{error}};
now createErrorResponse(INVALID_INPUT).
- POST /api/clipboard validation error lacked errorCode and shipped HTTP 200;
now createErrorResponse(INVALID_INPUT) -> 400.
- POST /api/run catch path returned bare {success:false,sessionId,error}
(HTTP 200, no errorCode); now OPERATION_FAILED envelope -> 422 with the
dead session id in the message.
- DELETE tail-file/:streamId returned {success: closed}, colliding with the
envelope discriminator; now returns {closed}.
Dead/regressed UI paths:
- Plan history modal could never open: route returned the bare history array
under data while the frontend read data.data.history/currentVersion. Route
now returns {history, currentVersion}; modal task count fixed to stats.total.
- Self-update error toast read j.error.message from the string-typed envelope
error, always falling back to the generic message; now reads the string.
Cleanup:
- Removed the stale QuickStartResponse type (unreferenced; documented the
pre-envelope shape and invited success-key collisions).
Tests: new test/http-contract.test.ts boots a real WebServer (port 3168) and
pins the envelope, /api/v1 alias, error statuses, and the /api 404 shape —
the route-test harness does not install the server-level hook, so these need
the live server. Updated file-routes/plan-routes/scheduled-runs tests to the
fixed shapes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
* Information about a case folder
|
||||||
*/
|
*/
|
||||||
|
|||||||
@@ -1035,7 +1035,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
onclick="app.rollbackToPlanVersion(${item.version})">
|
onclick="app.rollbackToPlanVersion(${item.version})">
|
||||||
<div>
|
<div>
|
||||||
<span class="plan-history-version">v${item.version}</span>
|
<span class="plan-history-version">v${item.version}</span>
|
||||||
<span class="plan-history-tasks">${item.taskCount || 0} tasks</span>
|
<span class="plan-history-tasks">${item.stats?.total ?? 0} tasks</span>
|
||||||
</div>
|
</div>
|
||||||
<span class="plan-history-time">${this.formatRelativeTime(item.timestamp)}</span>
|
<span class="plan-history-time">${this.formatRelativeTime(item.timestamp)}</span>
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -304,7 +304,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
try {
|
try {
|
||||||
// Get case path first
|
// Get case path first
|
||||||
const caseRes = await fetch(`/api/cases/${caseName}`);
|
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
|
// Create the case if it doesn't exist
|
||||||
if (!caseData.path) {
|
if (!caseData.path) {
|
||||||
@@ -452,7 +452,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
try {
|
try {
|
||||||
// Get the case path
|
// Get the case path
|
||||||
const caseRes = await fetch(`/api/cases/${caseName}`);
|
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
|
// Create the case if it doesn't exist
|
||||||
if (!caseData.path) {
|
if (!caseData.path) {
|
||||||
|
|||||||
@@ -569,7 +569,7 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
const res = await this._apiPost('/api/system/update', {});
|
const res = await this._apiPost('/api/system/update', {});
|
||||||
if (!res || !res.ok) {
|
if (!res || !res.ok) {
|
||||||
let msg = 'Failed to start the update.';
|
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(`<span style="color:var(--danger,#e5534b)">${escapeHtml(msg)}</span>`);
|
this._setUpdateProgress(`<span style="color:var(--danger,#e5534b)">${escapeHtml(msg)}</span>`);
|
||||||
if (btn) { btn.disabled = false; btn.textContent = 'Update now'; }
|
if (btn) { btn.disabled = false; btn.textContent = 'Update now'; }
|
||||||
return;
|
return;
|
||||||
|
|||||||
@@ -6,13 +6,14 @@
|
|||||||
import { FastifyInstance } from 'fastify';
|
import { FastifyInstance } from 'fastify';
|
||||||
import { SseEvent } from '../sse-events.js';
|
import { SseEvent } from '../sse-events.js';
|
||||||
import type { EventPort } from '../ports/index.js';
|
import type { EventPort } from '../ports/index.js';
|
||||||
|
import { createErrorResponse, ApiErrorCode } from '../../types.js';
|
||||||
|
|
||||||
export function registerClipboardRoutes(app: FastifyInstance, ctx: EventPort): void {
|
export function registerClipboardRoutes(app: FastifyInstance, ctx: EventPort): void {
|
||||||
app.post('/api/clipboard', async (req) => {
|
app.post('/api/clipboard', async (req) => {
|
||||||
const body = req.body as { text?: string; sessionId?: string };
|
const body = req.body as { text?: string; sessionId?: string };
|
||||||
const text = body?.text;
|
const text = body?.text;
|
||||||
if (typeof text !== 'string' || text.length === 0) {
|
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, {
|
ctx.broadcast(SseEvent.ClipboardWrite, {
|
||||||
text,
|
text,
|
||||||
|
|||||||
@@ -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) => {
|
app.delete('/api/sessions/:id/tail-file/:streamId', async (req) => {
|
||||||
const { id, streamId } = req.params as { id: string; streamId: string };
|
const { id, streamId } = req.params as { id: string; streamId: string };
|
||||||
findSessionOrFail(ctx, id); // Validates session exists
|
findSessionOrFail(ctx, id); // Validates session exists
|
||||||
const closed = fileStreamManager.closeStream(streamId);
|
const closed = fileStreamManager.closeStream(streamId);
|
||||||
return { success: closed };
|
return { closed };
|
||||||
});
|
});
|
||||||
// Session-scoped file download.
|
// Session-scoped file download.
|
||||||
// Uses the same realpath-based workspace boundary as file preview/raw routes;
|
// Uses the same realpath-based workspace boundary as file preview/raw routes;
|
||||||
|
|||||||
@@ -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 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 ==========
|
// ========== Rollback to Version ==========
|
||||||
|
|||||||
@@ -1086,9 +1086,10 @@ export function registerSessionRoutes(
|
|||||||
await ctx.cleanupSession(session.id, true, 'run_prompt_complete');
|
await ctx.cleanupSession(session.id, true, 'run_prompt_complete');
|
||||||
return { sessionId: session.id, ...result };
|
return { sessionId: session.id, ...result };
|
||||||
} catch (err) {
|
} 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');
|
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})`);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
+14
-1
@@ -104,6 +104,7 @@ function rewriteApiV1Url(url: string): string {
|
|||||||
import {
|
import {
|
||||||
getErrorMessage,
|
getErrorMessage,
|
||||||
httpStatusForErrorCode,
|
httpStatusForErrorCode,
|
||||||
|
createErrorResponse,
|
||||||
ApiErrorCode,
|
ApiErrorCode,
|
||||||
type PersistedRespawnConfig,
|
type PersistedRespawnConfig,
|
||||||
type NiceConfig,
|
type NiceConfig,
|
||||||
@@ -738,7 +739,7 @@ export class WebServer extends EventEmitter {
|
|||||||
this.app.post('/api/events/subscribe', (req, reply) => {
|
this.app.post('/api/events/subscribe', (req, reply) => {
|
||||||
const body = (req.body || {}) as { clientId?: string; sessions?: string[] | null };
|
const body = (req.body || {}) as { clientId?: string; sessions?: string[] | null };
|
||||||
if (typeof body.clientId !== 'string' || !SSE_CLIENT_ID_RE.test(body.clientId)) {
|
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;
|
return;
|
||||||
}
|
}
|
||||||
const sessions = Array.isArray(body.sessions)
|
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.
|
// parseBody. Shared with the route test harness so test behavior matches prod.
|
||||||
installRouteErrorHandler(this.app);
|
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.
|
// Crash diagnostics beacon — frontend POSTs breadcrumbs, GET to read them.
|
||||||
// text/plain is used ONLY by this beacon (navigator.sendBeacon sends text/plain).
|
// 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
|
// Keep the body as a RAW STRING and parse it inside the handler — a global
|
||||||
|
|||||||
@@ -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');
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -363,11 +363,11 @@ describe('file-routes', () => {
|
|||||||
});
|
});
|
||||||
expect(res.statusCode).toBe(200);
|
expect(res.statusCode).toBe(200);
|
||||||
const body = JSON.parse(res.body);
|
const body = JSON.parse(res.body);
|
||||||
expect(body.success).toBe(true);
|
expect(body.closed).toBe(true);
|
||||||
expect(mockedFileStreamManager.closeStream).toHaveBeenCalledWith('stream-1');
|
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);
|
mockedFileStreamManager.closeStream.mockReturnValue(false);
|
||||||
|
|
||||||
const res = await harness.app.inject({
|
const res = await harness.app.inject({
|
||||||
@@ -376,7 +376,7 @@ describe('file-routes', () => {
|
|||||||
});
|
});
|
||||||
expect(res.statusCode).toBe(200);
|
expect(res.statusCode).toBe(200);
|
||||||
const body = JSON.parse(res.body);
|
const body = JSON.parse(res.body);
|
||||||
expect(body.success).toBe(false);
|
expect(body.closed).toBe(false);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -235,13 +235,14 @@ describe('plan-routes', () => {
|
|||||||
expect(body.error).toContain('Ralph tracker');
|
expect(body.error).toContain('Ralph tracker');
|
||||||
});
|
});
|
||||||
|
|
||||||
it('returns plan version history', async () => {
|
it('returns plan version history with the current version', async () => {
|
||||||
const mockHistory = [
|
const mockHistory = [
|
||||||
{ version: 1, timestamp: Date.now() - 60000, itemCount: 10 },
|
{ version: 1, timestamp: Date.now() - 60000, itemCount: 10 },
|
||||||
{ version: 2, timestamp: Date.now(), itemCount: 12 },
|
{ version: 2, timestamp: Date.now(), itemCount: 12 },
|
||||||
];
|
];
|
||||||
harness.ctx._session.ralphTracker = {
|
harness.ctx._session.ralphTracker = {
|
||||||
getPlanHistory: vi.fn(() => mockHistory),
|
getPlanHistory: vi.fn(() => mockHistory),
|
||||||
|
planVersion: 2,
|
||||||
} as never;
|
} as never;
|
||||||
|
|
||||||
const res = await harness.app.inject({
|
const res = await harness.app.inject({
|
||||||
@@ -251,8 +252,9 @@ describe('plan-routes', () => {
|
|||||||
expect(res.statusCode).toBe(200);
|
expect(res.statusCode).toBe(200);
|
||||||
const body = JSON.parse(res.body);
|
const body = JSON.parse(res.body);
|
||||||
expect(body.success).toBe(true);
|
expect(body.success).toBe(true);
|
||||||
expect(body.data).toHaveLength(2);
|
expect(body.data.history).toHaveLength(2);
|
||||||
expect(body.data[1].version).toBe(2);
|
expect(body.data.history[1].version).toBe(2);
|
||||||
|
expect(body.data.currentVersion).toBe(2);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -205,8 +205,14 @@ describe('Quick Run API', () => {
|
|||||||
const data = await response.json();
|
const data = await response.json();
|
||||||
|
|
||||||
// On success the payload is wrapped ({ success:true, data:{ sessionId, ... } });
|
// On success the payload is wrapped ({ success:true, data:{ sessionId, ... } });
|
||||||
// on failure the handler returns a bare { success:false, sessionId, error }.
|
// on failure the handler returns the standard error envelope
|
||||||
expect(data.data?.sessionId ?? data.sessionId).toBeDefined();
|
// ({ 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
|
// Note: success/failure depends on Claude actually running
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user