From c7a7fd07cc3c4de54b2f37e7a5cd29c203faba0a Mon Sep 17 00:00:00 2001 From: fred Date: Sat, 29 Aug 2026 21:59:13 +0000 Subject: [PATCH] brain/gateway: prohibit mission_tasks.status as a write source (M4-3a phase 1) (#1479) --- .../src/missions/missions.controller.ts | 4 +- apps/gateway/src/missions/missions.dto.ts | 12 +++ .../P0-MAP-CURRENCY-2026-08-29.md | 12 ++- packages/brain/src/mission-tasks.spec.ts | 79 +++++++++++++++++++ packages/brain/src/mission-tasks.ts | 22 +++++- 5 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 packages/brain/src/mission-tasks.spec.ts diff --git a/apps/gateway/src/missions/missions.controller.ts b/apps/gateway/src/missions/missions.controller.ts index f422d900..ae7bfd0c 100644 --- a/apps/gateway/src/missions/missions.controller.ts +++ b/apps/gateway/src/missions/missions.controller.ts @@ -108,11 +108,13 @@ export class MissionsController { ) { const mission = await this.brain.missions.findByIdAndUser(missionId, user.id); 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({ missionId, taskId: dto.taskId, userId: user.id, - status: dto.status, description: dto.description, notes: dto.notes, pr: dto.pr, diff --git a/apps/gateway/src/missions/missions.dto.ts b/apps/gateway/src/missions/missions.dto.ts index d425e9ba..16908d3a 100644 --- a/apps/gateway/src/missions/missions.dto.ts +++ b/apps/gateway/src/missions/missions.dto.ts @@ -77,6 +77,12 @@ export class CreateMissionTaskDto { @IsUUID() 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() @IsIn(taskStatuses) status?: 'not-started' | 'in-progress' | 'blocked' | 'done' | 'cancelled'; @@ -102,6 +108,12 @@ export class UpdateMissionTaskDto { @IsUUID() 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() @IsIn(taskStatuses) status?: 'not-started' | 'in-progress' | 'blocked' | 'done' | 'cancelled'; diff --git a/docs/native-kanban-sot/P0-MAP-CURRENCY-2026-08-29.md b/docs/native-kanban-sot/P0-MAP-CURRENCY-2026-08-29.md index b06f5746..c50b4f30 100644 --- a/docs/native-kanban-sot/P0-MAP-CURRENCY-2026-08-29.md +++ b/docs/native-kanban-sot/P0-MAP-CURRENCY-2026-08-29.md @@ -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 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 `apps/gateway/src/missions/missions.controller.ts`. `update` accepts `Partial`, so `status` is writable through both DTOs today. The same module also exposes `remove`/`removeByMission` DELETE paths — 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 (`get-query.service.ts`, `list-query.service.ts`) select only. The MCP `brain_*` tools do not touch `mission_tasks` at all; `brain_create_task` / diff --git a/packages/brain/src/mission-tasks.spec.ts b/packages/brain/src/mission-tasks.spec.ts new file mode 100644 index 00000000..805207c7 --- /dev/null +++ b/packages/brain/src/mission-tasks.spec.ts @@ -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; + 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; + 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; + 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; + expect(Object.keys(updated)).toEqual(['updatedAt']); + }); +}); diff --git a/packages/brain/src/mission-tasks.ts b/packages/brain/src/mission-tasks.ts index acd4235b..22b21ba2 100644 --- a/packages/brain/src/mission-tasks.ts +++ b/packages/brain/src/mission-tasks.ts @@ -3,6 +3,24 @@ import { eq, and, type Db, missionTasks } from '@mosaicstack/db'; export type MissionTask = typeof missionTasks.$inferSelect; 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(data: T): Omit { + const rest = { ...data }; + delete rest.status; + return rest; +} + export function createMissionTasksRepo(db: Db) { return { async findByMission(missionId: string): Promise { @@ -30,14 +48,14 @@ export function createMissionTasksRepo(db: Db) { }, async create(data: NewMissionTask): Promise { - const rows = await db.insert(missionTasks).values(data).returning(); + const rows = await db.insert(missionTasks).values(stripStatus(data)).returning(); return rows[0]!; }, async update(id: string, data: Partial): Promise { const rows = await db .update(missionTasks) - .set({ ...data, updatedAt: new Date() }) + .set({ ...stripStatus(data), updatedAt: new Date() }) .where(eq(missionTasks.id, id)) .returning(); return rows[0];