brain/gateway: prohibit mission_tasks.status as a write source (M4-3a phase 1) (#1479)
ci/woodpecker/push/publish Pipeline was canceled
ci/woodpecker/push/publish Pipeline was canceled
This commit was merged in pull request #1479.
This commit is contained in:
@@ -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,
|
||||||
|
|||||||
@@ -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` /
|
||||||
|
|||||||
@@ -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']);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -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];
|
||||||
|
|||||||
Reference in New Issue
Block a user