brain/gateway: prohibit mission_tasks.status as a write source (M4-3a phase 1) #1479

Merged
fred merged 2 commits from feat/m4-3a-p1-stop-mission-task-status-writes into next 2026-08-29 21:59:13 +00:00
5 changed files with 125 additions and 4 deletions
@@ -108,11 +108,13 @@ export class MissionsController {
) { ) {
const mission = await this.brain.missions.findByIdAndUser(missionId, user.id); const mission = await this.brain.missions.findByIdAndUser(missionId, user.id);
if (!mission) throw new NotFoundException('Mission not found'); if (!mission) throw new NotFoundException('Mission not found');
// dto.status is deliberately not forwarded: mission_tasks.status is
// write-prohibited through the N-1 window (SHARED-CONTRACT §5.1 phase 1);
// the repo strips it as well.
return this.brain.missionTasks.create({ return this.brain.missionTasks.create({
missionId, missionId,
taskId: dto.taskId, taskId: dto.taskId,
userId: user.id, userId: user.id,
status: dto.status,
description: dto.description, description: dto.description,
notes: dto.notes, notes: dto.notes,
pr: dto.pr, pr: dto.pr,
+12
View File
@@ -77,6 +77,12 @@ export class CreateMissionTaskDto {
@IsUUID() @IsUUID()
taskId?: string; taskId?: string;
/**
* @deprecated Accepted for N-1 wire compatibility but ignored: mission_tasks.status
* is write-prohibited through the migration window (SHARED-CONTRACT §5.1 phase 1).
* The field stays declared because the global ValidationPipe runs with
* forbidNonWhitelisted, and removing it would 400 frozen legacy consumers.
*/
@IsOptional() @IsOptional()
@IsIn(taskStatuses) @IsIn(taskStatuses)
status?: 'not-started' | 'in-progress' | 'blocked' | 'done' | 'cancelled'; status?: 'not-started' | 'in-progress' | 'blocked' | 'done' | 'cancelled';
@@ -102,6 +108,12 @@ export class UpdateMissionTaskDto {
@IsUUID() @IsUUID()
taskId?: string; taskId?: string;
/**
* @deprecated Accepted for N-1 wire compatibility but ignored: mission_tasks.status
* is write-prohibited through the migration window (SHARED-CONTRACT §5.1 phase 1).
* The field stays declared because the global ValidationPipe runs with
* forbidNonWhitelisted, and removing it would 400 frozen legacy consumers.
*/
@IsOptional() @IsOptional()
@IsIn(taskStatuses) @IsIn(taskStatuses)
status?: 'not-started' | 'in-progress' | 'blocked' | 'done' | 'cancelled'; status?: 'not-started' | 'in-progress' | 'blocked' | 'done' | 'cancelled';
@@ -73,12 +73,22 @@ SHARED-CONTRACT.md §5.1 phase 1 requires an N-1 patch that stops
`mission_tasks.status` as a write source, plus a writer inventory, before any `mission_tasks.status` as a write source, plus a writer inventory, before any
expand DDL. expand DDL.
- **Sole status write path:** `packages/brain/src/mission-tasks.ts` - **Sole authoring write path:** `packages/brain/src/mission-tasks.ts`
`create`/`update` (Drizzle insert/update on `mission_tasks`), invoked by `create`/`update` (Drizzle insert/update on `mission_tasks`), invoked by
`apps/gateway/src/missions/missions.controller.ts`. `update` accepts `apps/gateway/src/missions/missions.controller.ts`. `update` accepts
`Partial<NewMissionTask>`, so `status` is writable through both DTOs today. `Partial<NewMissionTask>`, so `status` is writable through both DTOs today.
The same module also exposes `remove`/`removeByMission` DELETE paths — The same module also exposes `remove`/`removeByMission` DELETE paths —
immaterial to `status` writes, listed for inventory completeness. immaterial to `status` writes, listed for inventory completeness.
- **Storage-layer surfaces that touch the column without authoring it**
(added 2026-08-29 after independent review of the phase-1 patch):
`packages/storage/src/migrate-tier.ts` copies whole `mission_tasks` rows
between storage tiers and must preserve the stored `status` verbatim — row
transport, exempt from the write prohibition (stripping there would corrupt
data inside the N-1 window). The generic table-keyed storage adapters
(`adapters/postgres.ts`, `adapters/pglite.ts`) register `mission_tasks` in
their table maps but have no caller that targets it: measured at this head,
every runtime adapter caller passes a fixed collection constant
(preferences/insights). Neither surface authors a new `status` value.
- **Read-only consumers of `mission_tasks`:** federation verb services - **Read-only consumers of `mission_tasks`:** federation verb services
(`get-query.service.ts`, `list-query.service.ts`) select only. The MCP (`get-query.service.ts`, `list-query.service.ts`) select only. The MCP
`brain_*` tools do not touch `mission_tasks` at all; `brain_create_task` / `brain_*` tools do not touch `mission_tasks` at all; `brain_create_task` /
+79
View File
@@ -0,0 +1,79 @@
import { describe, it, expect, vi } from 'vitest';
import { createMissionTasksRepo } from './mission-tasks.js';
/**
* SHARED-CONTRACT §5.5 "mission_tasks.status write prohibition": this repo is
* the sole path that authors mission_tasks.status from caller input (storage
* tier migration is row transport and preserves stored values; the generic
* storage adapters have no mission_tasks caller), and it must never forward a
* caller-supplied status to the database on create or update. Callers keep
* working (the field is accepted and ignored), so these tests assert on what
* reaches the Drizzle chain, not on rejection.
*/
function makeInsertDb(returned: unknown[]) {
const values = vi.fn((_v: unknown) => ({ returning: vi.fn().mockResolvedValue(returned) }));
return { db: { insert: vi.fn(() => ({ values })) }, values };
}
function makeUpdateDb(returned: unknown[]) {
const set = vi.fn((_v: unknown) => ({
where: vi.fn(() => ({ returning: vi.fn().mockResolvedValue(returned) })),
}));
return { db: { update: vi.fn(() => ({ set })) }, set };
}
describe('createMissionTasksRepo — status write prohibition', () => {
it('create strips a caller-supplied status before insert', async () => {
const { db, values } = makeInsertDb([{ id: 'mt1', status: 'not-started' }]);
const repo = createMissionTasksRepo(db as never);
const result = await repo.create({
missionId: 'm1',
userId: 'u1',
status: 'done',
description: 'd',
} as never);
expect(values).toHaveBeenCalledTimes(1);
const inserted = values.mock.calls[0]![0] as Record<string, unknown>;
expect('status' in inserted).toBe(false);
expect(inserted.missionId).toBe('m1');
expect(inserted.description).toBe('d');
expect(result.id).toBe('mt1');
});
it('create without status still inserts (DB default applies)', async () => {
const { db, values } = makeInsertDb([{ id: 'mt2' }]);
const repo = createMissionTasksRepo(db as never);
await repo.create({ missionId: 'm1', userId: 'u1' } as never);
const inserted = values.mock.calls[0]![0] as Record<string, unknown>;
expect('status' in inserted).toBe(false);
});
it('update strips a caller-supplied status but keeps the other fields', async () => {
const { db, set } = makeUpdateDb([{ id: 'mt1', notes: 'n' }]);
const repo = createMissionTasksRepo(db as never);
const result = await repo.update('mt1', { status: 'done', notes: 'n' } as never);
expect(set).toHaveBeenCalledTimes(1);
const updated = set.mock.calls[0]![0] as Record<string, unknown>;
expect('status' in updated).toBe(false);
expect(updated.notes).toBe('n');
expect(updated.updatedAt).toBeInstanceOf(Date);
expect(result?.id).toBe('mt1');
});
it('update with only status degenerates to a timestamp-only update', async () => {
const { db, set } = makeUpdateDb([{ id: 'mt1' }]);
const repo = createMissionTasksRepo(db as never);
await repo.update('mt1', { status: 'blocked' } as never);
const updated = set.mock.calls[0]![0] as Record<string, unknown>;
expect(Object.keys(updated)).toEqual(['updatedAt']);
});
});
+20 -2
View File
@@ -3,6 +3,24 @@ import { eq, and, type Db, missionTasks } from '@mosaicstack/db';
export type MissionTask = typeof missionTasks.$inferSelect; export type MissionTask = typeof missionTasks.$inferSelect;
export type NewMissionTask = typeof missionTasks.$inferInsert; export type NewMissionTask = typeof missionTasks.$inferInsert;
// SHARED-CONTRACT §5.1 phase 1 / §5.4: mission_tasks.status is prohibited as a
// write source through the N-1 window. This repo is the sole path that authors
// status from caller input, so the field is stripped here — accepted and
// ignored rather than rejected, because the legacy surface is frozen with
// existing consumers kept working (tool-gateway-mapping.md §3.2). Two other
// surfaces touch the column and are deliberately NOT stripped:
// packages/storage/migrate-tier.ts copies whole rows between storage tiers and
// must preserve the stored value verbatim, and the generic table-keyed storage
// adapters register mission_tasks but have no caller that targets it (runtime
// callers use fixed collection constants). Neither authors a new status. The
// column keeps its DB default, stays declared and readable, and is retired
// only after no readers remain.
function stripStatus<T extends { status?: unknown }>(data: T): Omit<T, 'status'> {
const rest = { ...data };
delete rest.status;
return rest;
}
export function createMissionTasksRepo(db: Db) { export function createMissionTasksRepo(db: Db) {
return { return {
async findByMission(missionId: string): Promise<MissionTask[]> { async findByMission(missionId: string): Promise<MissionTask[]> {
@@ -30,14 +48,14 @@ export function createMissionTasksRepo(db: Db) {
}, },
async create(data: NewMissionTask): Promise<MissionTask> { async create(data: NewMissionTask): Promise<MissionTask> {
const rows = await db.insert(missionTasks).values(data).returning(); const rows = await db.insert(missionTasks).values(stripStatus(data)).returning();
return rows[0]!; return rows[0]!;
}, },
async update(id: string, data: Partial<NewMissionTask>): Promise<MissionTask | undefined> { async update(id: string, data: Partial<NewMissionTask>): Promise<MissionTask | undefined> {
const rows = await db const rows = await db
.update(missionTasks) .update(missionTasks)
.set({ ...data, updatedAt: new Date() }) .set({ ...stripStatus(data), updatedAt: new Date() })
.where(eq(missionTasks.id, id)) .where(eq(missionTasks.id, id))
.returning(); .returning();
return rows[0]; return rows[0];