refactor(server): plan 4 — narrow TripMembersRepository.remove/TripsRepository.setOwner to number

Both accepted trip_id: number | string per 3c's carry note (their "stays
loose for other callers" reasoning), pending the guard-level single parse
landing first. Re-checked every caller directly: setOwner's one production
caller (trip-members.service.ts#transferOwnership) already passes trip.id,
a real number resolved earlier in the same transaction; remove's callers
are trips.rpc.ts (num(), already a validated number), trips.mcp.ts (Zod-
typed tool input, already a number) and TripMembersController.removeMember
(the one genuine raw-string caller, :id off the route with no class-level
TripAccessGuard on this controller by design).

TripMembersController.removeMember now parses :id once (toRowId, not
Number() — rule 15's NaN-into-SQL trap) and threads the parsed number to
both its own access check and removeMember, instead of passing the raw
string through twice. TripMembersService.removeMember and both repository
methods narrow to number accordingly; setOwner's WHERE also moves off its
raw-bind escape hatch onto nativeUpdate now that trip_id is typed, and
remove's trip_id half moves onto a typed .where({ trip: trip_id }) filter
(user_id keeps its own raw bind — a different, still-live NaN-safety seam,
unrelated to this narrowing).

TRIPREPO-018 and TMEMREPO-023, which pinned the retired string-bind seam
on setOwner/remove, no longer typecheck against the narrowed signatures
and are removed with an explanatory comment in their place.

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

@@ -231,12 +231,16 @@ export class TripMembersRepository extends TrekRepository<TripMembers> {
    * `DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?`
    * (`trip-members.service.ts:166`, TM8 `removeMember`; `:198`, TM14
    * `transferOwnership`'s "no longer a plain member" step —
-   * security-sensitive). Raw-bind on BOTH `trip_id` and `user_id` — not just
-   * `trip_id` (D4's T5 escape hatch, the same `number | string` seam
-   * `rosterUserIds`/`listUserIdsByTrip` above preserve): `TripMembersController
-   * .removeMember` still does a bare `Number.parseInt(userId)` on the route's
-   * `:userId` before calling this, unvalidated (rule 15's exact trap — a
-   * non-numeric id parses to `NaN`).
+   * security-sensitive). `trip_id: number` (Plan 4 Task 8a, narrowed from
+   * `number | string` — its last raw-string caller,
+   * `TripMembersController.removeMember`, now parses its route param once
+   * itself before calling this), so `trip_id` is a typed `.where({ trip:
+   * trip_id })` filter below; `user_id` stays a raw `m.user_id = ?`
+   * placeholder — NOT the same seam, a different reason entirely:
+   * `TripMembersController.removeMember` still does a bare
+   * `Number.parseInt(userId)` on the route's `:userId`, unvalidated (rule
+   * 15's exact trap — a non-numeric id parses to `NaN`), and a typed filter
+   * cannot carry a `NaN` through this ORM layer intact (see below).
    *
    * **`Number.isFinite` guard, checked directly against a compiled boot, not
    * assumed:** a typed `.andWhere({ user: user_id })` filter renders `NaN` as
@@ -257,11 +261,11 @@ export class TripMembersRepository extends TrekRepository<TripMembers> {
    * that, byte for byte, since neither the typed filter nor the raw
    * placeholder route can pass `NaN` through the ORM layer intact.
    */
-  async remove(trip_id: number | string, user_id: number): Promise<void> {
+  async remove(trip_id: number, user_id: number): Promise<void> {
     if (!Number.isFinite(user_id)) return;
     await this.qb('m')
       .delete()
-      .where('m.trip_id = ?', [trip_id])
+      .where({ trip: trip_id })
       .andWhere('m.user_id = ?', [user_id])
       .execute('run');
   }

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

@@ -309,16 +309,16 @@ export class TripsRepository extends TrekRepository<Trips> {
   /**
    * `UPDATE trips SET user_id = ? WHERE id = ?` (`trip-members.service.ts:196`,
    * TM13) — security-sensitive: the ownership handover itself, the first of
-   * `transferOwnership`'s three transactional statements. `qb().update()`
-   * rather than `nativeUpdate`: `nativeUpdate`'s typed `FilterQuery` rejects
-   * a `string` against `id`'s branded `number` type the same way `find`'s
-   * does, and `trip_id` here is still the route's unconverted string.
+   * `transferOwnership`'s three transactional statements. `trip_id: number`
+   * (Plan 4 Task 8a, narrowed from `number | string`): its one production
+   * caller already passes `trip.id`, the real row `findIdTitleOwner`
+   * resolved earlier in the same transaction, never the route's raw string
+   * (see `transferOwnership`'s own comment on why). `nativeUpdate`, typed
+   * filter throughout — nothing here needs the raw-bind escape hatch any
+   * more.
    */
-  async setOwner(trip_id: number | string, user_id: number): Promise<void> {
-    await this.qb('t')
-      .update({ user: user_id })
-      .where('t.id = ?', [trip_id])
-      .execute('run');
+  async setOwner(trip_id: number, user_id: number): Promise<void> {
+    await this.nativeUpdate({ id: trip_id }, { user: user_id });
   }
 
   /**

server/src/nest/trip-members/trip-members.controller.ts

@@ -21,6 +21,7 @@ import { RequireTripOwner, TripOwnerGuard } from '../permissions/trip-owner.guar
 import { getClientIp } from '../audit/client-ip';
 import { AuditService } from '../audit/audit.service';
 import { NotFoundError, ValidationError } from '../common/domain-errors';
+import { toRowId } from '../common/row-id';
 import { TripAddMemberDto, TripTransferOwnershipDto, TripCreateGuestDto, TripRenameGuestDto } from '../trips/trips.dto';
 
 /**
@@ -80,7 +81,16 @@ export class TripMembersController {
 
   @Delete(':id/members/:userId')
   async removeMember(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) {
-    const access = await this.roster.canAccessTrip(id, user.id);
+    // Plan 4 Task 8a — parsed ONCE here (toRowId, not Number(): rule 15's
+    // NaN-into-SQL trap), and the parsed number is what both the access
+    // check and the removal itself use below — this route has no class-level
+    // TripAccessGuard (this controller's own docstring explains why), so it
+    // owns this parse itself rather than inheriting the guard's.
+    const tripId = toRowId(id);
+    if (tripId === null) {
+      throw new HttpException({ error: 'Trip not found' }, 404);
+    }
+    const access = await this.roster.canAccessTrip(tripId, user.id);
     if (!access) {
       throw new HttpException({ error: 'Trip not found' }, 404);
     }
@@ -88,7 +98,7 @@ export class TripMembersController {
     if (targetId !== user.id && !(await this.roster.can('member_manage', user.role, access.user_id, user.id, access.user_id !== user.id))) {
       throw new HttpException({ error: 'No permission to remove members' }, 403);
     }
-    await this.roster.removeMember(id, targetId);
+    await this.roster.removeMember(tripId, targetId);
     return { success: true };
   }
 

server/src/nest/trip-members/trip-members.service.ts

@@ -170,7 +170,14 @@ export class TripMembersService {
     };
   }
 
-  async removeMember(tripId: string | number, targetUserId: number): Promise<void> {
+  /**
+   * `tripId: number` (Plan 4 Task 8a — narrowed from `string | number`):
+   * every caller now passes an already-parsed id — `trips.rpc.ts`'s `num()`,
+   * `trips.mcp.ts`'s Zod-typed tool input, and `TripMembersController
+   * .removeMember`'s own `toRowId` parse (added alongside this narrowing,
+   * the route's one remaining raw-string caller).
+   */
+  async removeMember(tripId: number, targetUserId: number): Promise<void> {
     await this.tripMembersRepo.remove(tripId, targetUserId);
   }
 
@@ -202,15 +209,15 @@ export class TripMembersService {
 
     // Task 9 fix wave (B-L4 / A-L1): all THREE writes below now take
     // `trip.id` — the real `number` `findIdTitleOwner` already resolved
-    // above, not the route's raw `tripId` string. The comment this replaces
+    // above, not the route's raw `tripId` string. The comment this replaced
     // claimed "`Number(tripId)` at the two writes below", but only
     // `addIgnoringConflict` (which requires a real `Primary<Trips>`) ever
-    // did that; `setOwner`/`remove` kept binding the unconverted string —
-    // not a live divergence (both repository methods document and test
-    // that raw-bind seam for their OTHER callers, so their signatures stay
-    // `number | string`), but the letter of rule 21 says one id, parsed
-    // once, threaded everywhere — and `trip.id` is already sitting right
-    // here, already proven to be the real row.
+    // did that; `setOwner`/`remove` used to keep binding the unconverted
+    // string for their OTHER callers' sake. Plan 4 Task 8a narrowed both to
+    // `number` (their last raw-string caller, `TripMembersController
+    // .removeMember`, now parses once itself) — the letter of rule 21 says
+    // one id, parsed once, threaded everywhere, and `trip.id` is already
+    // sitting right here, already proven to be the real row.
     await this.uow.transactional(async () => {
       await this.tripsRepo.setOwner(trip.id, newOwnerId);
       // The new owner is no longer a plain member…

server/tests/unit/db/repositories/TripMembers.repository.test.ts

@@ -306,14 +306,14 @@ describe('TripMembersRepository.addIgnoringConflict / remove (TM8/TM14/TM15, sec
     await expect(tripMembers.remove(trip.id, 999999)).resolves.toBeUndefined();
   });
 
-  it('TMEMREPO-023: remove binds a string trip id raw, the same seam rosterUserIds/listUserIdsByTrip document', async () => {
-    const { user: owner } = createUser(testDb);
-    const { user: member } = createUser(testDb);
-    const trip = createTrip(testDb, owner.id);
-    addTripMember(testDb, trip.id, member.id);
-    await tripMembers.remove(String(trip.id), member.id);
-    expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id)).toBeUndefined();
-  });
+  // TMEMREPO-023 used to pin remove's `number | string` raw-bind seam on
+  // `trip_id` (`remove(String(trip.id), ...)`) — retired by Plan 4 Task 8a's
+  // narrowing to `trip_id: number` (its own docstring covers why: its last
+  // raw-string caller, TripMembersController.removeMember, now parses its
+  // route param once itself). `String(trip.id)` no longer typechecks
+  // against `remove`'s signature, so there is nothing left to pin here.
+  // `user_id` keeps its own raw bind (TMEMREPO-024 below), a different,
+  // still-live seam — see remove's own docstring.
 });
 
 // Rule 15's exact trap, caught on a compiled boot (`DELETE /api/trips/:id/

server/tests/unit/db/repositories/Trips.repository.test.ts

@@ -193,13 +193,12 @@ describe('TripsRepository — TripMembersService / TripReadModelService (Plan 3c
     expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(newOwner.id);
   });
 
-  it('TRIPREPO-018: setOwner binds a string trip id raw — the same seam its docstring documents', async () => {
-    const { user: owner } = createUser(testDb);
-    const { user: newOwner } = createUser(testDb);
-    const trip = createTrip(testDb, owner.id);
-    await trips.setOwner(String(trip.id), newOwner.id);
-    expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(newOwner.id);
-  });
+  // TRIPREPO-018 used to pin setOwner's `number | string` raw-bind seam
+  // (`setOwner(String(trip.id), ...)`) — retired by Plan 4 Task 8a's
+  // narrowing to `trip_id: number` (its own docstring covers why: its one
+  // production caller already passes a real `trip.id` number, never the
+  // route's raw string). `String(trip.id)` no longer typechecks against
+  // `setOwner`'s signature, so there is nothing left to pin here.
 
   describe('findRaw (TR-B, shared with TripsService.getRaw once Task 7 lands)', () => {
     it('TRIPREPO-019: every scalar column comes back, feed_token included (the JS strip is the caller\'s job)', async () => {