Merge pull request #1557 from BradGroux/fix/attempt-ownership-1521

fix: preserve managed attempt ownership during task edits

.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.

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": "<jwt-token>",
   "role": "admin",
   "expiresIn": "24h"
 }

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 () => {

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' });
+    });
+  });
+}

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');
     }

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<TaskAttempt, 'id'> & Partial<Omit<TaskAttempt, 'id'>>;
   lastBoardMove?: TaskBoardMoveReceipt;
   boardRank?: string | null;
@@ -781,10 +783,17 @@ export class TaskService {
     return task;
   }
 
-  async updateTask(id: string, input: UpdateTaskInput): Promise<Task | null> {
+  async updateTask(
+    id: string,
+    input: UpdateTaskInput,
+    options: { protectManagedAttempt?: boolean } = {}
+  ): Promise<Task | null> {
     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;

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');
+  }
+}