refactor(server): plan 4 — U6 gate-level id parsing for todo item ids

Rule 21's carry from 3e Task 8's review: TodoController.update/.remove
passed the raw :id route string all the way to TodoItemsRepository,
relying on SQLite's column-affinity CAST to match it instead of a typed
filter. Parse :id ONCE at the controller (toRowId, matching Task 8a's
TripAccessGuard/removeMember shape), 404 on a non-canonical id, and thread
the number down through TodoService.updateItem/deleteItem into
TodoItemsRepository.findInTrip/existsInTrip/update/deleteById, all
narrowed from number | string to number. todo.rpc.ts's num()-derived
todoId no longer needs its String() wrapper. tripId stays string | number
— a separate, already-accepted carry (23c292f2f). Two new e2e tests pin
the non-numeric-id 404 at the gate.

server/src/db/repositories/TodoItems.repository.ts

@@ -98,14 +98,21 @@ export class TodoItemsRepository extends TrekRepository<TodoItems> {
     return await this.kysely<TodoItemsKyselyDB>().selectFrom('todo_items').selectAll().where('id', '=', id as number).executeTakeFirst();
   }
 
-  /** TD5 (`updateItem`'s trip-scoping guard) — `SELECT * FROM todo_items WHERE id = ? AND trip_id = ?`. */
-  async findInTrip(id: number | string, trip_id: number | string): Promise<TodoItemRow | undefined> {
-    return await this.kysely<TodoItemsKyselyDB>().selectFrom('todo_items').selectAll().where('id', '=', id as number).where('trip_id', '=', trip_id as number).executeTakeFirst();
+  /**
+   * TD5 (`updateItem`'s trip-scoping guard) — `SELECT * FROM todo_items
+   * WHERE id = ? AND trip_id = ?`. `id: number` (Plan 4 Task 8b, U6 — the
+   * program's own gate-level id parsing carry: narrowed from `number |
+   * string` now that `TodoController.update`/`.remove` parse `:id` once via
+   * `toRowId` and thread the number down; `trip_id` stays `number | string`,
+   * a separate, still-accepted carry — see `todo.service.ts`'s own note).
+   */
+  async findInTrip(id: number, trip_id: number | string): Promise<TodoItemRow | undefined> {
+    return await this.kysely<TodoItemsKyselyDB>().selectFrom('todo_items').selectAll().where('id', '=', id).where('trip_id', '=', trip_id as number).executeTakeFirst();
   }
 
-  /** TD8 (`deleteItem`'s trip-scoping guard) — `SELECT id FROM todo_items WHERE id = ? AND trip_id = ?`. */
-  async existsInTrip(id: number | string, trip_id: number | string): Promise<{ id: number } | undefined> {
-    return await this.kysely<TodoItemsKyselyDB>().selectFrom('todo_items').select('id').where('id', '=', id as number).where('trip_id', '=', trip_id as number).executeTakeFirst();
+  /** TD8 (`deleteItem`'s trip-scoping guard) — `SELECT id FROM todo_items WHERE id = ? AND trip_id = ?`. `id: number`, same Plan 4 Task 8b narrowing as {@link findInTrip}. */
+  async existsInTrip(id: number, trip_id: number | string): Promise<{ id: number } | undefined> {
+    return await this.kysely<TodoItemsKyselyDB>().selectFrom('todo_items').select('id').where('id', '=', id).where('trip_id', '=', trip_id as number).executeTakeFirst();
   }
 
   /**
@@ -117,7 +124,7 @@ export class TodoItemsRepository extends TrekRepository<TodoItems> {
    * exactly); every other column is a true presence sentinel bound off the
    * legacy `bodyKeys` array (`present = bodyKeys.includes('<col>')`).
    */
-  async update(id: number | string, write: {
+  async update(id: number, write: {
     name?: readonly [present: boolean, value: string];
     checked?: readonly [present: boolean, value: number];
     category?: readonly [present: boolean, value: string | null];
@@ -131,12 +138,12 @@ export class TodoItemsRepository extends TrekRepository<TodoItems> {
       description: string | null; assigned_user_id: number | null; priority: number;
     }>(write);
     if (Object.keys(data).length === 0) return;
-    await this.kysely<TodoItemsKyselyDB>().updateTable('todo_items').set(data).where('id', '=', id as number).execute();
+    await this.kysely<TodoItemsKyselyDB>().updateTable('todo_items').set(data).where('id', '=', id).execute();
   }
 
-  /** TD9 (`deleteItem`) — `DELETE FROM todo_items WHERE id = ?` (no trip scoping in the statement itself — relies on the caller's prior {@link existsInTrip} gate, matching legacy). */
-  async deleteById(id: number | string): Promise<void> {
-    await this.kysely<TodoItemsKyselyDB>().deleteFrom('todo_items').where('id', '=', id as number).execute();
+  /** TD9 (`deleteItem`) — `DELETE FROM todo_items WHERE id = ?` (no trip scoping in the statement itself — relies on the caller's prior {@link existsInTrip} gate, matching legacy). `id: number`, same Plan 4 Task 8b narrowing as {@link findInTrip}. */
+  async deleteById(id: number): Promise<void> {
+    await this.kysely<TodoItemsKyselyDB>().deleteFrom('todo_items').where('id', '=', id).execute();
   }
 
   /** TD10 (`reorderItems`, looped) — `UPDATE todo_items SET sort_order = ? WHERE id = ? AND trip_id = ?`. */

server/src/nest/todo/todo.controller.ts

@@ -16,6 +16,7 @@ import { TodoCreateItemDto, TodoUpdateItemDto, TodoReorderDto, TodoCategoryAssig
 import { JwtAuthGuard } from '../auth/jwt-auth.guard';
 import { CurrentUser } from '../auth/current-user.decorator';
 import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard';
+import { toRowId } from '../common/row-id';
 
 /**
  * /api/trips/:tripId/todo — trip-scoped task list.
@@ -77,12 +78,20 @@ export class TodoController {
     @Body() body: TodoUpdateItemDto,
     @Headers('x-socket-id') socketId?: string,
   ) {
+    // Plan 4 Task 8b (U6) — :id is parsed ONCE here (toRowId, not Number():
+    // rule 15's NaN-into-SQL trap), and the parsed number is what
+    // updateItem uses below, instead of the raw string reaching the
+    // repository and relying on SQLite's affinity CAST to match it.
+    const itemId = toRowId(id);
+    if (itemId === null) {
+      throw new HttpException({ error: 'Item not found' }, 404);
+    }
     const { name, checked, category, due_date, description, assigned_user_id, priority } = body;
     // bodyKeys carries which keys the request actually provided (the null-clear
     // protocol); the parsed body only ever holds known schema keys.
     const updated = await this.todo.updateItem(
       tripId,
-      id,
+      itemId,
       // checked arrives as boolean or legacy 0/1 — normalize to the 0/1 the SQL binds.
       { name, checked: checked === undefined ? undefined : checked ? 1 : 0, category, due_date, description, assigned_user_id, priority },
       Object.keys(body),
@@ -102,10 +111,15 @@ export class TodoController {
     @Param('id') id: string,
     @Headers('x-socket-id') socketId?: string,
   ) {
-    if (!(await this.todo.deleteItem(tripId, id))) {
+    // Plan 4 Task 8b (U6) — same single gate-level parse as update above.
+    const itemId = toRowId(id);
+    if (itemId === null) {
       throw new HttpException({ error: 'Item not found' }, 404);
     }
-    this.todo.broadcast(tripId, 'todo:deleted', { itemId: Number(id) }, socketId);
+    if (!(await this.todo.deleteItem(tripId, itemId))) {
+      throw new HttpException({ error: 'Item not found' }, 404);
+    }
+    this.todo.broadcast(tripId, 'todo:deleted', { itemId }, socketId);
     return { success: true };
   }
 

server/src/nest/todo/todo.rpc.ts

@@ -54,7 +54,7 @@ export class TodoRpc {
     const actor = this.guards.requireActor(ctx, 'todo');
     await this.guards.requireTripEdit(tripId, actor, TODO_EDIT_ACTION);
     const input = asPayload(params.input);
-    const updated = await this.todos.updateItem(String(tripId), String(todoId), input as never, Object.keys(input));
+    const updated = await this.todos.updateItem(String(tripId), todoId, input as never, Object.keys(input));
     if (!updated) throw new ForbiddenResource(`no todo ${todoId} on trip ${tripId}`);
     this.realtime.broadcast(tripId, 'todo:updated', { item: updated }, undefined);
     return updated;
@@ -66,7 +66,7 @@ export class TodoRpc {
     const todoId = num(params.todoId, 'todoId');
     const actor = this.guards.requireActor(ctx, 'todo');
     await this.guards.requireTripEdit(tripId, actor, TODO_EDIT_ACTION);
-    if (!(await this.todos.deleteItem(String(tripId), String(todoId)))) {
+    if (!(await this.todos.deleteItem(String(tripId), todoId))) {
       throw new ForbiddenResource(`no todo ${todoId} on trip ${tripId}`);
     }
     this.realtime.broadcast(tripId, 'todo:deleted', { itemId: todoId }, undefined);

server/src/nest/todo/todo.service.ts

@@ -76,9 +76,22 @@ export class TodoService {
     return this.todoItemsRepo.findById(id);
   }
 
+  /**
+   * `id: number` (Plan 4 Task 8b, U6 — program rule 21's gate-level id
+   * parsing carry: `TodoController.update` now parses `:id` once via
+   * `toRowId` and threads the number here, instead of the raw route string
+   * reaching `TodoItemsRepository.findInTrip`/`.update` and relying on
+   * SQLite's column-affinity CAST to match it; `todos.rpc.ts`'s `num()` and
+   * `todo.mcp.ts`'s Zod-typed `itemId` were already real numbers). `tripId`
+   * stays `string | number` — a separate, still-accepted carry (the trip id
+   * is gated for ACCESS by `TripAccessGuard`, not threaded as a parsed
+   * number to every downstream call — see `places.service.ts
+   * #verifyTripAccess`'s own docstring, Plan 4's 23c292f2f, for why that
+   * second parse is out of scope here too).
+   */
   async updateItem(
     tripId: string | number,
-    id: string | number,
+    id: number,
     data: { name?: string; checked?: number; category?: string | null; due_date?: string | null; description?: string | null; assigned_user_id?: number | null; priority?: number | null },
     bodyKeys: string[]
   ) {
@@ -98,7 +111,8 @@ export class TodoService {
     return this.todoItemsRepo.findById(id);
   }
 
-  async deleteItem(tripId: string | number, id: string | number): Promise<boolean> {
+  /** `id: number` — same Plan 4 Task 8b (U6) narrowing as {@link updateItem}. */
+  async deleteItem(tripId: string | number, id: number): Promise<boolean> {
     const item = await this.todoItemsRepo.existsInTrip(id, tripId);
     if (!item) return false;
 

server/tests/e2e/todo.e2e.test.ts

@@ -195,6 +195,23 @@ describe('To-do e2e (real auth guard + real SQL over temp SQLite)', () => {
     expect(res.body).toEqual({ error: 'Item not found' });
   });
 
+  // Plan 4 Task 8b (U6) — :id is now parsed ONCE at the controller gate
+  // (toRowId), so a non-numeric id 404s cleanly through that guard instead
+  // of falling through to the repository and depending on SQLite's
+  // column-affinity CAST to simply not match (the legacy outcome was also a
+  // 404, same status — this pins the gate itself, not just the status).
+  it('404 (not 500) on update with a non-numeric :id', async () => {
+    const res = await request(server).put(`/api/trips/${tripId}/todo/abc`).set('Cookie', sessionCookie(1)).send({ name: 'X' });
+    expect(res.status).toBe(404);
+    expect(res.body).toEqual({ error: 'Item not found' });
+  });
+
+  it('404 (not 500) on delete with a non-numeric :id', async () => {
+    const res = await request(server).delete(`/api/trips/${tripId}/todo/abc`).set('Cookie', sessionCookie(1));
+    expect(res.status).toBe(404);
+    expect(res.body).toEqual({ error: 'Item not found' });
+  });
+
   it('200 on reorder, persisting the new sort_order', async () => {
     const a = insertItem(tripId, 'A', { sort_order: 0 });
     const b = insertItem(tripId, 'B', { sort_order: 1 });