diff --git a/.gitleaksignore b/.gitleaksignore index 4453c9a2..5344fdf2 100644 --- a/.gitleaksignore +++ b/.gitleaksignore @@ -2,7 +2,6 @@ cli/src/__tests__/snapshot.test.ts:generic-api-key:220 # API documentation contains non-functional response examples. -docs/API-REFERENCE.md:generic-api-key:1043 docs/API-WORKFLOWS.md:generic-api-key:1460 # Operator documentation uses placeholders in curl authentication examples. diff --git a/docs/API-REFERENCE.md b/docs/API-REFERENCE.md index 786f1996..3ce6a75d 100644 --- a/docs/API-REFERENCE.md +++ b/docs/API-REFERENCE.md @@ -316,6 +316,8 @@ PATCH /api/tasks/:id **Body**: Partial task fields to update (title, description, status, priority, assignee, etc.). +Managed attempt state is owned by the run lifecycle APIs. Generic task updates reject an `attempt` replacement when the current attempt contains runtime, launch, admission, supervision, or other server-owned evidence. Ordinary task fields remain editable. Historical attempts containing only the legacy editor fields remain editable through this endpoint. + **Headers**: ```http @@ -1040,7 +1042,7 @@ POST /api/auth/login ```json { - "token": "eyJhbGciOiJIUzI1NiIs...", + "token": "", "role": "admin", "expiresIn": "24h" } diff --git a/server/src/__tests__/routes/tasks-coverage.test.ts b/server/src/__tests__/routes/tasks-coverage.test.ts index 7078847f..6fdf5445 100644 --- a/server/src/__tests__/routes/tasks-coverage.test.ts +++ b/server/src/__tests__/routes/tasks-coverage.test.ts @@ -116,6 +116,7 @@ vi.mock('../../middleware/cache-control.js', async () => { // Import after mocking import { taskRoutes } from '../../routes/tasks.js'; import { errorHandler } from '../../middleware/error-handler.js'; +import { taskAccess } from '../../routes/v1/permissions.js'; describe('Tasks Routes (actual module)', () => { let app: express.Express; @@ -635,7 +636,8 @@ describe('Tasks Routes (actual module)', () => { model: 'llama3.2', threadId: 'thread_docs_refresh', }), - }) + }), + { protectManagedAttempt: true } ); }); @@ -691,7 +693,7 @@ describe('Tasks Routes (actual module)', () => { expect(mockTaskService.updateTask).not.toHaveBeenCalled(); }); - it('preserves authoritative run contracts when patching the same attempt', async () => { + it('rejects generic same-ID changes to managed attempt authority and status', async () => { const taskEnvelope = { digest: 'immutable-envelope' }; const completionResult = { status: 'success' }; mockTaskService.getTask.mockResolvedValue({ @@ -708,17 +710,26 @@ describe('Tasks Routes (actual module)', () => { }); mockTaskService.updateTask.mockImplementation(async (_id, input) => input); - const res = await request(app) - .patch('/api/tasks/t1') - .send({ attempt: { id: 'attempt_1', agent: 'codex', status: 'complete' } }); - - expect(res.status).toBe(200); - expect(mockTaskService.updateTask).toHaveBeenCalledWith( - 't1', - expect.objectContaining({ - attempt: expect.objectContaining({ taskEnvelope, completionResult }), - }) - ); + const scopedApp = express(); + scopedApp.use(express.json()); + scopedApp.use((req, _res, next) => { + (req as import('../../middleware/auth.js').AuthenticatedRequest).auth = { + role: 'agent', + isLocalhost: false, + permissions: ['task:write'], + }; + next(); + }); + scopedApp.use(['/api/tasks', '/api/v1/tasks'], taskAccess, taskRoutes); + scopedApp.use(errorHandler); + for (const prefix of ['/api/tasks', '/api/v1/tasks']) { + const res = await request(scopedApp) + .patch(`${prefix}/t1`) + .send({ attempt: { id: 'attempt_1', agent: 'codex', status: 'complete' } }); + expect(res.status).toBe(400); + expect(res.body.message).toContain('run lifecycle APIs'); + } + expect(mockTaskService.updateTask).not.toHaveBeenCalled(); }); it('rejects replacing an attempt that owns authoritative run contracts', async () => { diff --git a/server/src/__tests__/task-attempt-edit.test.ts b/server/src/__tests__/task-attempt-edit.test.ts new file mode 100644 index 00000000..713f097c --- /dev/null +++ b/server/src/__tests__/task-attempt-edit.test.ts @@ -0,0 +1,113 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { DEFAULT_FEATURE_SETTINGS, type TaskAttempt } from '@veritas-kanban/shared'; +import { TaskService } from '../services/task-service.js'; +import { TelemetryService } from '../services/telemetry-service.js'; +import { + createTestSqliteDatabase, + type TestSqliteDatabase, +} from '../storage/sqlite/test-helpers.js'; + +for (const storageType of ['file', 'sqlite'] as const) { + describe(`generic attempt edits (${storageType})`, () => { + let root: string; + let database: TestSqliteDatabase | undefined; + let service: TaskService; + const legacy: TaskAttempt = { id: 'attempt_legacy', agent: 'codex', status: 'running' }; + + function openService() { + return new TaskService({ + storageType, + sqliteDatabase: database?.database, + tasksDir: path.join(root, 'active'), + archiveDir: path.join(root, 'archive'), + telemetryService: new TelemetryService({ + telemetryDir: path.join(root, 'telemetry'), + config: { enabled: false }, + }), + configService: { getFeatureSettings: async () => DEFAULT_FEATURE_SETTINGS }, + }); + } + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'veritas-attempt-edit-')); + database = storageType === 'sqlite' ? createTestSqliteDatabase() : undefined; + service = openService(); + }); + + afterEach(async () => { + service.dispose(); + database?.cleanup(); + await fs.rm(root, { recursive: true, force: true }); + }); + + it('denies same-ID status changes and replacements without changing durable evidence', async () => { + const task = await service.createTask({ title: 'Managed attempt' }); + const managed: TaskAttempt = { + ...legacy, + runSupervisorId: 'supervisor_test', + admissionReservationId: 'reservation_test', + }; + await service.updateTask(task.id, { attempt: managed, attempts: [managed] }); + const before = await service.getTask(task.id); + for (const id of [managed.id, 'attempt_replacement']) { + for (const status of ['running', 'complete', 'failed'] as const) { + await expect( + service.updateTask( + task.id, + { + title: 'Must not be written', + attempt: { ...legacy, id, status }, + }, + { protectManagedAttempt: true } + ) + ).rejects.toThrow('run lifecycle APIs'); + } + } + service.dispose(); + service = openService(); + expect(await service.getTask(task.id)).toEqual(before); + const renamed = await service.updateTask(task.id, { title: 'Ordinary edit' }); + expect(renamed?.attempt).toEqual(managed); + expect(renamed?.attempts).toEqual([managed]); + }); + + it('checks the current stored attempt after a launch replaces the route snapshot', async () => { + const task = await service.createTask({ title: 'Concurrent launch' }); + await service.updateTask(task.id, { attempt: legacy }); + const routeSnapshot = await service.getTask(task.id); + await service.updateTask(task.id, { + attempt: { ...legacy, runSupervisorId: 'supervisor_new' }, + }); + await expect( + service.updateTask( + task.id, + { + attempt: { ...routeSnapshot!.attempt!, status: 'complete' }, + }, + { protectManagedAttempt: true } + ) + ).rejects.toThrow('run lifecycle APIs'); + expect((await service.getTask(task.id))?.attempt?.status).toBe('running'); + }); + + it('preserves legacy edits and dedicated lifecycle updates', async () => { + const task = await service.createTask({ title: 'Legacy attempt' }); + await service.updateTask(task.id, { attempt: legacy }, { protectManagedAttempt: true }); + const completed = await service.updateTask( + task.id, + { + attempt: { ...legacy, status: 'complete' }, + }, + { protectManagedAttempt: true } + ); + expect(completed?.attempt?.status).toBe('complete'); + const managed = { ...legacy, runSupervisorId: 'supervisor_lifecycle' }; + await service.updateTask(task.id, { attempt: managed }); + await service.patchTaskAttempt(task.id, managed.id, { status: 'complete' }); + expect((await service.getTask(task.id))?.attempt).toEqual({ ...managed, status: 'complete' }); + }); + }); +} diff --git a/server/src/routes/tasks.ts b/server/src/routes/tasks.ts index 26439576..9bf6b4df 100644 --- a/server/src/routes/tasks.ts +++ b/server/src/routes/tasks.ts @@ -1,3 +1,4 @@ +import { assertLegacyAttemptEditable } from '../utils/task-attempt-edit.js'; import { Router, type NextFunction, type Response, type Router as RouterType } from 'express'; import { z } from 'zod'; import { getTaskService } from '../services/task-service.js'; @@ -1014,27 +1015,7 @@ router.patch( if (!oldTask) { throw new NotFoundError('Task not found'); } - const authoritativeAttempt = oldTask.attempt; - if ( - input.attempt && - authoritativeAttempt && - (authoritativeAttempt.taskEnvelope || authoritativeAttempt.completionResult) - ) { - if (input.attempt.id !== authoritativeAttempt.id) { - throw new ValidationError( - 'Generic task updates cannot replace an attempt with an authoritative run contract' - ); - } - input.attempt = { - ...input.attempt, - ...(authoritativeAttempt.taskEnvelope - ? { taskEnvelope: authoritativeAttempt.taskEnvelope } - : {}), - ...(authoritativeAttempt.completionResult - ? { completionResult: authoritativeAttempt.completionResult } - : {}), - }; - } + if (input.attempt) assertLegacyAttemptEditable(oldTask.attempt); assertFreshRevision(req, 'task', oldTask.id, oldTask); const authReq = req as AuthenticatedRequest; @@ -1100,7 +1081,11 @@ router.patch( input.blockedReason = null; } - const task = await taskService.updateTask(req.params.id as string, input); + const task = input.attempt + ? await taskService.updateTask(req.params.id as string, input, { + protectManagedAttempt: true, + }) + : await taskService.updateTask(req.params.id as string, input); if (!task) { throw new NotFoundError('Task not found'); } diff --git a/server/src/services/task-service.ts b/server/src/services/task-service.ts index 9696143b..85d4ebc5 100644 --- a/server/src/services/task-service.ts +++ b/server/src/services/task-service.ts @@ -1,3 +1,4 @@ +import { assertLegacyAttemptEditable } from '../utils/task-attempt-edit.js'; import { nanoid } from 'nanoid'; import type { Task, @@ -93,6 +94,7 @@ interface BoardStatusConfig { } type TaskMutationInput = UpdateTaskInput & { + protectManagedAttempt?: boolean; attemptPatch?: Pick & Partial>; lastBoardMove?: TaskBoardMoveReceipt; boardRank?: string | null; @@ -781,10 +783,17 @@ export class TaskService { return task; } - async updateTask(id: string, input: UpdateTaskInput): Promise { + async updateTask( + id: string, + input: UpdateTaskInput, + options: { protectManagedAttempt?: boolean } = {} + ): Promise { const affectsBoard = input.position !== undefined || input.status !== undefined; - const mutationInput: TaskMutationInput = - input.position !== undefined ? { ...input, boardRank: null } : input; + const mutationInput: TaskMutationInput = { + ...input, + ...(input.position !== undefined ? { boardRank: null } : {}), + protectManagedAttempt: options.protectManagedAttempt, + }; if (affectsBoard) { return this.withBoardMoveMutex((commitStorage) => this.withTaskMutex(id, () => @@ -838,6 +847,7 @@ export class TaskService { boardRank: boardRankUpdate, expectedRevision: _expectedRevision, attemptPatch, + protectManagedAttempt, ...restInput } = input; @@ -874,6 +884,12 @@ export class TaskService { ? ((await this.sqliteTasks.findById(id)) ?? task) : (fileMutationTask ?? task); + // Check inside the storage lock: launch may have installed a managed + // attempt after the generic route read the previous task revision. + if (protectManagedAttempt && input.attempt) { + assertLegacyAttemptEditable(freshTask.attempt); + } + if (attemptPatch && freshTask.attempt?.id !== attemptPatch.id) { updatedTask = freshTask; return; diff --git a/server/src/utils/task-attempt-edit.ts b/server/src/utils/task-attempt-edit.ts new file mode 100644 index 00000000..942617ff --- /dev/null +++ b/server/src/utils/task-attempt-edit.ts @@ -0,0 +1,24 @@ +import type { TaskAttempt } from '@veritas-kanban/shared'; +import { ValidationError } from '../middleware/error-handler.js'; + +// The historical generic task editor supports only these legacy fields. Any +// additional field identifies server-owned evidence, including future contracts. +const LEGACY_ATTEMPT_FIELDS = new Set([ + 'id', + 'agent', + 'status', + 'started', + 'ended', + 'provider', + 'model', + 'threadId', + 'cloudUrl', + 'cloudTarget', + 'orchestration', +]); + +export function assertLegacyAttemptEditable(attempt: TaskAttempt | undefined): void { + if (attempt && Object.keys(attempt).some((key) => !LEGACY_ATTEMPT_FIELDS.has(key))) { + throw new ValidationError('Managed attempts can only be changed through run lifecycle APIs'); + } +}