fix(server): non-numeric route ids answer the legacy not-found, never NaN (tokens, categories, tags); mint path back to two statements; identity-map regression pinned

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

@@ -1,5 +1,5 @@
 import { McpTokens } from '../entities/McpTokens.entity';
-import { toRow, type AssertRowKeys } from './_shared/rows';
+import type { AssertRowKeys } from './_shared/rows';
 import { columnRef, currentTimestamp } from '../dialect/sql-functions';
 import { EntityRepository } from '@mikro-orm/sql';
 
@@ -169,12 +169,21 @@ export class McpTokensRepository extends EntityRepository<McpTokens> {
    * transaction (a later task's caller), a `flush()` here would commit
    * whatever else that transaction was still assembling. `em.insert()` fires
    * one native INSERT with no side effects on the context/identity map,
-   * matching `AuditLogRepository.insertEntry`'s precedent. Followed by a
-   * `disableIdentityMap: true` re-select (D4's create rule, this
-   * repository's isolated-read shape) rather than `em.refresh`, which needs
-   * a managed entity `em.insert` never produces.
+   * matching `AuditLogRepository.insertEntry`'s precedent.
+   *
+   * Returns only the generated `id` (`EntityManager#insert`'s own return
+   * type, `Primary<McpTokens>` — a bare `number` for this entity's PK,
+   * confirmed against the installed 7.2.1 typings), not a re-selected row
+   * (Plan 3b Task 2 review, F4): an earlier version re-queried the full row
+   * here via a `disableIdentityMap: true` `findOne` and discarded it, which
+   * made `TokenService.createToken`'s own follow-up `findBasic` call a
+   * *third* statement for what the legacy code did in two (INSERT, then one
+   * re-select) — and the discarded middle read was the only place on the
+   * mint path that loaded `token_hash` into memory for nothing. `findBasic`
+   * (TK4) stays the one post-insert read; this method fires the INSERT and
+   * nothing else.
    */
-  async insertToken(row: NewMcpTokenRow): Promise<McpTokenRow> {
+  async insertToken(row: NewMcpTokenRow): Promise<{ id: number }> {
     const id = await this.getEntityManager().insert(McpTokens, {
       user: row.user_id,
       name: row.name,
@@ -184,23 +193,30 @@ export class McpTokensRepository extends EntityRepository<McpTokens> {
       scope_mode: row.scope_mode,
       api_scopes: row.api_scopes,
     });
-    const inserted = await this.findOne({ id }, { disableIdentityMap: true });
-    if (!inserted) {
-      throw new Error(`McpTokensRepository.insertToken: row ${String(id)} not found immediately after insert`);
-    }
-    return toRow(inserted) as McpTokenRow;
+    return { id };
   }
 
   /**
    * `SELECT id, name, token_prefix, created_at, last_used_at FROM
    *  mcp_tokens WHERE id = ?` (TK4, `createToken`'s re-select) and
    * `SELECT id, user_id FROM mcp_tokens WHERE id = ?` (TK9,
-   * `adminDeleteMcpToken`'s 404 check) — ONE method: TK9's two columns
-   * (`id`, `user_id`) are a subset of TK4's five, and both statements are a
-   * plain by-id lookup with no other filter, so a single narrower-than-`*`
-   * projection covering the union of both column sets serves both call
-   * sites without a second near-identical statement (same economy as
-   * `deleteById` below covering TK6/TK10). PK-only `findOne` ->
+   * `adminDeleteMcpToken`'s 404 check) — ONE method, but NOT the same
+   * byte-identical-statement merge `deleteById` below does for TK6/TK10 (a
+   * real D4 deviation, corrected here per the Plan 3b Task 2 review, F3):
+   * TK4 and TK9 select two genuinely different column sets (`id, name,
+   * token_prefix, created_at, last_used_at` vs. `id, user_id`), and this
+   * method returns their UNION — a superset projection that answers both
+   * call shapes rather than one of them plus an extra column nobody asked
+   * for. That is safe here because the client-visible payload is unchanged
+   * (`createToken` destructures `user_id` back out before responding, and
+   * `adminDeleteMcpToken` reads only `user_id`) and both call sites are
+   * plain by-id lookups with no other filter — two near-identical
+   * `findOne`s here is exactly the Sonar-duplication shape the plan warns
+   * about. This precedent does NOT transfer automatically to two methods
+   * whose difference is an order of magnitude larger (e.g. `SELECT *` vs. a
+   * narrow named set) — evaluate the actual column-set delta each time,
+   * don't cite this merge to justify collapsing a bigger gap. PK-only
+   * `findOne` ->
    * `disableIdentityMap: true`, NOT `refresh: true` (controller ruling on
    * Task 1's review, superseding Plan 3a's "PK-only `findOne` -> `refresh:
    * true`" ruling everywhere in this repository): `refresh` still re-queries

server/src/nest/categories/categories.service.ts

@@ -3,6 +3,7 @@ import { InjectRepository } from '@mikro-orm/nestjs';
 import type { Category } from '@trek/shared';
 import { Categories } from '../../db/entities/Categories.entity';
 import type { CategoriesRepository } from '../../db/repositories/Categories.repository';
+import { toRowId } from '../common/row-id';
 
 /**
  * Categories domain service — owns the category business rules (moved off
@@ -27,7 +28,16 @@ export class CategoriesService {
   }
 
   async getById(id: string | number): Promise<Category | undefined> {
-    const category = await this.categories.findById(Number(id));
+    // Convert, VALIDATE, and answer the legacy not-found before any
+    // repository call (program rule 15): the legacy raw-SQL statement bound
+    // `id` straight into `WHERE id = ?` and let SQLite's affinity rules miss
+    // on a non-numeric string, returning `undefined`; a typed repository
+    // filter has no such leniency — a bare `Number()` renders `NaN` as an
+    // unquoted column reference and 500s (Plan 3b Task 2 review, F1's class
+    // of bug, same fix applied here per the fix round's item 1b).
+    const rowId = toRowId(id);
+    if (rowId === null) return undefined;
+    const category = await this.categories.findById(rowId);
     return (category as unknown as Category | null) ?? undefined;
   }
 
@@ -51,14 +61,27 @@ export class CategoriesService {
     if (name) changes.name = name;
     if (color) changes.color = color;
     if (icon) changes.icon = icon;
-    // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2);
-    // the legacy re-select returned `undefined` for the same case, and every
-    // caller pre-checks with a 404 first, so `?? undefined` restores that
-    // exact parity for the race-condition path the pre-check doesn't cover.
-    return ((await this.categories.patch(Number(id), changes)) ?? undefined) as unknown as Category;
+    // Every caller pre-checks with `getById` (a 404 for both a missing row
+    // and a non-numeric id) first, so this path only runs for an id already
+    // known valid — but a non-numeric id here still must not reach the
+    // repository as `NaN` (F1's class of bug): the legacy `UPDATE ... WHERE
+    // id = ?` bound the raw string, matched nothing via SQLite's affinity
+    // rules, and the re-select likewise returned `undefined`. `toRowId`
+    // reproduces that exact no-op for the race-condition path the pre-check
+    // doesn't cover, and `patch` returns `null` for a non-existent numeric
+    // id (Task 3 review, Minor 2) — both fold into the same `?? undefined`
+    // parity the legacy re-select had.
+    const rowId = toRowId(id);
+    if (rowId === null) return undefined as unknown as Category;
+    return ((await this.categories.patch(rowId, changes)) ?? undefined) as unknown as Category;
   }
 
   async remove(id: string | number): Promise<void> {
-    await this.categories.remove(Number(id));
+    // Same guard: a non-numeric id must no-op (matching the legacy `DELETE
+    // ... WHERE id = ?` binding a string that never matched any row) rather
+    // than reach the repository as `NaN`.
+    const rowId = toRowId(id);
+    if (rowId === null) return;
+    await this.categories.remove(rowId);
   }
 }

server/src/nest/common/row-id.ts

@@ -0,0 +1,38 @@
+/**
+ * The one place an external, string-shaped route/tool id is turned into a
+ * numeric primary key before it reaches a typed repository filter.
+ *
+ * The legacy raw-SQL layer bound a route param straight into `WHERE id = ?`
+ * and let SQLite's column-affinity rules do the work: a well-formed numeric
+ * string ('5', ' 5', '5.0') converts and matches, anything else ('abc', '',
+ * '0x10', '1e3') stays TEXT, can never equal an INTEGER column, and the
+ * lookup simply misses — answered with the route's ordinary not-found. A
+ * typed MikroORM filter has no such affinity: `Number('abc')` is `NaN`, and
+ * the ORM renders that as the bare, unquoted token `NaN` in the generated
+ * SQL, which SQLite parses as a column reference and throws
+ * `InvalidFieldNameException: no such column: NaN` — a 500 where the legacy
+ * route 404'd (Plan 3b Task 2 review, F1).
+ *
+ * `toRowId` is the guard every converted service calls instead of a bare
+ * `Number(id)`: it accepts only what the legacy binding actually matched —
+ * digits only, no sign, no decimal point, no exponent, no hex — and returns
+ * `null` for everything else, which the caller answers with the route's own
+ * legacy not-found result BEFORE any repository call (program rule 15,
+ * `docs/superpowers/plans/2026-09-21-orm-migration-program.md`).
+ *
+ * A `/^\d+$/` string match is deliberately stricter than `Number.isSafeInteger`
+ * alone: `Number('0x10')` is `16` and `Number('1e3')` is `1000`, both safe
+ * integers a bare `Number()` conversion would have accepted even though
+ * SQLite's affinity conversion never would have matched either shape against
+ * an INTEGER column. Requiring the digits-only shape first closes that gap.
+ */
+export function toRowId(value: unknown): number | null {
+  if (typeof value === 'number') {
+    return Number.isSafeInteger(value) ? value : null;
+  }
+  if (typeof value === 'string' && /^\d+$/.test(value)) {
+    const parsed = Number(value);
+    return Number.isSafeInteger(parsed) ? parsed : null;
+  }
+  return null;
+}

server/src/nest/tags/tags.service.ts

@@ -3,6 +3,7 @@ import { InjectRepository } from '@mikro-orm/nestjs';
 import type { Tag } from '@trek/shared';
 import { Tags } from '../../db/entities/Tags.entity';
 import type { TagsRepository } from '../../db/repositories/Tags.repository';
+import { toRowId } from '../common/row-id';
 
 /**
  * Tags domain service — owns the tag business rules (moved off
@@ -27,7 +28,17 @@ export class TagsService {
   }
 
   async getByIdAndUser(id: string | number, userId: number): Promise<Tag | undefined> {
-    const tag = await this.tags.findByIdAndUser(Number(id), userId);
+    // Convert, VALIDATE, and answer the legacy not-found before any
+    // repository call (program rule 15): the legacy raw-SQL statement bound
+    // `id` straight into `WHERE id = ? AND user_id = ?` and let SQLite's
+    // affinity rules miss on a non-numeric string, returning `undefined`; a
+    // typed repository filter has no such leniency — a bare `Number()`
+    // renders `NaN` as an unquoted column reference and 500s (Plan 3b Task 2
+    // review, F1's class of bug, same fix applied here per the fix round's
+    // item 1b).
+    const rowId = toRowId(id);
+    if (rowId === null) return undefined;
+    const tag = await this.tags.findByIdAndUser(rowId, userId);
     return (tag as unknown as Tag | null) ?? undefined;
   }
 
@@ -49,14 +60,27 @@ export class TagsService {
     const changes: { name?: string; color?: string } = {};
     if (name) changes.name = name;
     if (color) changes.color = color;
-    // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2);
-    // the legacy re-select returned `undefined` for the same case, and every
-    // caller pre-checks with a 404 first, so `?? undefined` restores that
-    // exact parity for the race-condition path the pre-check doesn't cover.
-    return ((await this.tags.patch(Number(id), changes)) ?? undefined) as unknown as Tag;
+    // Every caller pre-checks with `getByIdAndUser` (a 404 for both a
+    // missing/foreign row and a non-numeric id) first, so this path only
+    // runs for an id already known valid — but a non-numeric id here still
+    // must not reach the repository as `NaN` (F1's class of bug): the
+    // legacy `UPDATE ... WHERE id = ?` bound the raw string, matched
+    // nothing via SQLite's affinity rules, and the re-select likewise
+    // returned `undefined`. `toRowId` reproduces that exact no-op for the
+    // race-condition path the pre-check doesn't cover, and `patch` returns
+    // `null` for a non-existent numeric id (Task 3 review, Minor 2) — both
+    // fold into the same `?? undefined` parity the legacy re-select had.
+    const rowId = toRowId(id);
+    if (rowId === null) return undefined as unknown as Tag;
+    return ((await this.tags.patch(rowId, changes)) ?? undefined) as unknown as Tag;
   }
 
   async remove(id: string | number): Promise<void> {
-    await this.tags.remove(Number(id));
+    // Same guard: a non-numeric id must no-op (matching the legacy `DELETE
+    // ... WHERE id = ?` binding a string that never matched any row) rather
+    // than reach the repository as `NaN`.
+    const rowId = toRowId(id);
+    if (rowId === null) return;
+    await this.tags.remove(rowId);
   }
 }

server/src/nest/tokens/token.service.ts

@@ -11,6 +11,7 @@ import type { McpTokensRepository, McpTokenBasicRow } from '../../db/repositorie
 import { Users } from '../../db/entities/Users.entity';
 import type { UsersRepository } from '../../db/repositories/Users.repository';
 import { EphemeralTokenService } from '../auth/ephemeral-token.service';
+import { toRowId } from '../common/row-id';
 // Import from sessionManager directly, NOT the ../../mcp barrel: the barrel pulls
 // the whole tools fan-out (and via the domain bridges, the Nest services) into
 // every consumer of this module — a nest→mcp→nest module cycle.
@@ -133,10 +134,11 @@ export class TokenService {
     });
 
     // A separate re-select, matching the legacy INSERT-then-SELECT shape
-    // exactly (TK3 then TK4) rather than reusing `inserted`'s full row —
-    // `findBasic` also serves TK9's admin lookup, so its `user_id` field is
-    // dropped here: TK4's response never carried it, and leaking it would be
-    // a new field on the client-facing token payload, not a refactor.
+    // exactly (TK3 then TK4 — two statements, `insertToken` returns only
+    // the generated id and never re-queries the row itself) — `findBasic`
+    // also serves TK9's admin lookup, so its `user_id` field is dropped
+    // here: TK4's response never carried it, and leaking it would be a new
+    // field on the client-facing token payload, not a refactor.
     const basic = (await this.tokens.findBasic(inserted.id)) as McpTokenBasicRow;
     const { user_id: _userId, ...token } = basic;
 
@@ -160,7 +162,14 @@ export class TokenService {
    * missing from a screen they never opened.
    */
   private async deleteToken(userId: number, tokenId: string, kind: TokenKind): Promise<{ error?: string; status?: number; success?: boolean }> {
-    const id = Number(tokenId);
+    // Convert, VALIDATE, and answer the legacy not-found before any
+    // repository call (program rule 15): the legacy statement bound
+    // `tokenId` straight into `WHERE id = ?` and let SQLite's affinity rules
+    // miss on a non-numeric string; a typed repository filter has no such
+    // leniency, so a bare `Number()` turned a 404 into a 500 (Plan 3b Task 2
+    // review, F1).
+    const id = toRowId(tokenId);
+    if (id === null) return { error: 'Token not found', status: 404 };
     const token = await this.tokens.findOwnedByKind(id, userId, kind);
     if (!token) return { error: 'Token not found', status: 404 };
     await this.tokens.deleteById(id);
@@ -197,7 +206,7 @@ export class TokenService {
   // Admin view
   //
   // The same table, seen across all users. Moved here from AdminService, which
-  // only owned a second copy of the delete purely because the admin route lived
+  // owned a second copy of the delete purely because the admin route lived
   // there. Note the two are genuinely different queries, not duplicates: the
   // user-facing ones scope every statement by user_id, these deliberately do
   // not, and the admin delete revokes sessions unconditionally where the
@@ -209,7 +218,10 @@ export class TokenService {
   }
 
   async adminDeleteMcpToken(id: string) {
-    const numericId = Number(id);
+    // Same guard as `deleteToken` above — convert, VALIDATE, answer the
+    // legacy 404 before any repository call (F1).
+    const numericId = toRowId(id);
+    if (numericId === null) return { error: 'Token not found', status: 404 };
     const token = await this.tokens.findBasic(numericId);
     if (!token) return { error: 'Token not found', status: 404 };
     await this.tokens.deleteById(numericId);
@@ -260,8 +272,8 @@ export class TokenService {
     if (row) {
       await this.tokens.touchLastUsedByHash(hash);
       // `role` is `users.role TEXT`, narrower at runtime than the repository's
-      // row type states — same trust boundary the legacy `this.db.get<User>()`
-      // generic asserted without a runtime check.
+      // row type states — same trust boundary the pre-ORM raw-SQL lookup's
+      // generic type parameter asserted without a runtime check.
       return row as User;
     }
     return null;