fix(tests): close the route guard's factory hole and two regex warnings (#307)

Three of the items on the cleanup issue, and the first is the one that mattered.

routesAreWrapped.test.ts could not see a handler built by a factory. `router.post('/x', rotationRoute('left'))` carries no async token of its own, so the guard read those two lines, found nothing to object to, and passed — which is not the same as finding them wrapped. That is how the rotation routes added in #301 went through a test that exists precisely because this convention had already been half-forgotten once, when thirty handlers were added unwrapped after the wrapper existed. It now follows a call to a function declared in the same file and reads its body the same way it reads a registration, so an unwrapped handler inside a factory is an offender. Proved rather than assumed: unwrapping rotationRoute's handler makes the suite fail naming admin.ts, where before it passed.

Only same-file functions are followed, deliberately. app.ts registers express.json(), cookieParser() and uploadsRouter(), none of which is a handler factory and none of which can be resolved from the file being read — treating an unresolvable name as an offender would trade one hole for a permanently red test, so there is a case asserting those are left alone.

The brace and paren walking is now one function rather than two. Adding the factory reader as a near-copy of registrationAt is what a cleanup commit should not do, and the duplicate carried its own cognitive-complexity and loop-counter warnings with it; parameterising the delimiter pair removes both the copy and the warnings it added.

The schema mirror's table regex used `\s*` where kysely-codegen emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. SonarQube flagged both, and the looser form bought nothing and backtracked for it.

An empty status list would have compiled to `in ()`, which is a Postgres syntax error, where the `= ANY($n::text[])` it replaced in #308 was valid and matched nothing. It is unreachable through parseItemFilters, which refuses a list that names nothing — but the obvious guard is wrong in the opposite direction, because dropping the clause entirely would make an empty status filter match every status rather than none, so the empty case is spelled out as false.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/itemFilters.ts

@@ -307,7 +307,17 @@ export function itemFilterExpressions(
     // `in` replaces the `= ANY($n::text[])` this used to build. Kysely emits
     // the placeholder list itself, so one status and several use the same
     // expression and the explicit ::text[] cast is no longer needed.
-    clauses.push(eb('i.status', 'in', filters.status));
+    //
+    // The empty list is spelled out rather than left to `in`, which would emit
+    // `in ()` — a Postgres syntax error, where `= ANY` on an empty array was
+    // valid and matched nothing. Unreachable through parseItemFilters, which
+    // refuses a list that names nothing, but the obvious alternative is wrong
+    // in the opposite direction: dropping the clause entirely would make an
+    // empty status filter match *every* status, where the behaviour this
+    // replaced matched none. See #307.
+    clauses.push(
+      filters.status.length ? eb('i.status', 'in', filters.status) : sql<SqlBool>`false`
+    );
   }
 
   if (filters.favoritesOnly) {

backend/tests/integration/schemaMirror.integration.test.ts

@@ -30,7 +30,11 @@ const SCHEMA = readFileSync(
 function mirroredColumns(): Map<string, Set<string>> {
   const block = /export interface DB \{([^}]*)\}/.exec(SCHEMA)?.[1] ?? '';
   const byTable = new Map<string, Set<string>>();
-  for (const [, table, iface] of block.matchAll(/^\s*([a-z0-9_]+):\s*([A-Za-z0-9_]+);/gm)) {
+  // Literal spaces rather than `\s*`, and `\w` rather than `[A-Za-z0-9_]`.
+  // kysely-codegen indents two spaces and puts exactly one after the colon, so
+  // the looser form bought nothing and backtracked for it — SonarQube flagged
+  // both (#307).
+  for (const [, table, iface] of block.matchAll(/^ {2}([a-z0-9_]+): (\w+);/gm)) {
     const declaration =
       new RegExp(`export interface ${iface!} \\{([^}]*)\\}`).exec(SCHEMA)?.[1] ?? '';
     byTable.set(

backend/tests/unit/routesAreWrapped.test.ts

@@ -23,14 +23,18 @@ const SRC = join(__dirname, '..', '..', 'src');
 const REGISTRATION = /\b(?:app|\w*[Rr]outer)\.(?:get|post|put|patch|delete|all|use)\s*\(/g;
 
 /**
- * Returns the full text of the registration call starting at `open` (the index
+ * Returns the text from `start` to the delimiter matching the one it opens
- * of its `(`), by counting parens to the matching close. Quotes and comments
+ * with, counting depth. Quotes and comments are skipped so a route path like
- * are skipped so a path like `'/items/:id'` or a `)` inside a string cannot
+ * `'/items/:id'`, or a brace inside a string, cannot throw the count off.
- * throw the count off.
+ *
+ * Parameterised over the delimiter pair because two callers need the same walk:
+ * a registration is bounded by parens and a function body by braces, and #307
+ * added the second one as a near-copy of the first before this was factored.
  */
-function registrationAt(source: string, open: number): string {
+function matchedSpan(source: string, start: number, open: string, close: string): string {
   let depth = 0;
-  for (let i = open; i < source.length; i++) {
+
+  for (let i = start; i < source.length; i++) {
     const c = source[i];
 
     if (c === "'" || c === '"' || c === '`') {
@@ -47,13 +51,22 @@ function registrationAt(source: string, open: number): string {
       continue;
     }
 
-    if (c === '(') depth++;
+    if (c === open) depth++;
-    if (c === ')') {
+    if (c === close) {
       depth--;
-      if (depth === 0) return source.slice(open, i + 1);
+      if (depth === 0) return source.slice(start, i + 1);
     }
   }
-  return source.slice(open);
+
+  return source.slice(start);
+}
+
+/**
+ * The full text of the registration call starting at `open`, the index of its
+ * `(`.
+ */
+function registrationAt(source: string, open: number): string {
+  return matchedSpan(source, open, '(', ')');
 }
 
 /** Returns the index of the closing quote of the string opening at `start`. */
@@ -69,8 +82,53 @@ function skipString(source: string, start: number): number {
   return source.length;
 }
 
+/** The body of the function declared at `declared`, braces included. */
+function bodyOf(source: string, declared: number): string {
+  const start = source.indexOf('{', declared);
+  return start === -1 ? '' : matchedSpan(source, start, '{', '}');
+}
+
 /**
- * Every `async` inside a registration must sit directly behind `asyncRoute(`.
+ * The bodies of any handler factories a registration calls.
+ *
+ * `router.post('/x', rotationRoute('left'))` carries no `async` token of its
+ * own, so the token check below passes it by finding nothing at all. That is
+ * exactly how the two rotation routes added in #301 sailed through a guard that
+ * exists because this convention had already been half-forgotten once — it did
+ * not find them wrapped, it found nothing and said yes. The handler lives in
+ * the factory, so the factory is what has to be read.
+ *
+ * Only functions declared in the same file are followed. `express.json()` and
+ * `cookieParser()` are imported and are not handler factories at all; there is
+ * no honest way to resolve those textually, and guessing at them would trade
+ * this hole for false positives on every library call in app.ts.
+ */
+function factoryBodies(source: string, call: string): string[] {
+  const bodies: string[] = [];
+
+  for (const [, name] of call.matchAll(/\b([a-zA-Z_]\w*)\s*\(/g)) {
+    if (name === 'asyncRoute') continue;
+
+    const declared = new RegExp(`function\\s+${name}\\s*\\(`).exec(source);
+    if (!declared) continue;
+
+    bodies.push(bodyOf(source, declared.index));
+  }
+
+  return bodies;
+}
+
+/** Whether `text` contains an `async` that is not directly behind `asyncRoute(`. */
+function hasUnwrappedAsync(text: string): boolean {
+  for (const found of text.matchAll(/\basync\b/g)) {
+    if (!text.slice(0, found.index!).trimEnd().endsWith('asyncRoute(')) return true;
+  }
+  return false;
+}
+
+/**
+ * Every `async` inside a registration must sit directly behind `asyncRoute(`,
+ * and so must every `async` inside a factory that registration calls.
  * Checking the token rather than the line catches a handler whose `async`
  * lands on its own line, which a line-oriented grep would wave through.
  */
@@ -81,11 +139,9 @@ function unwrappedHandlers(source: string): string[] {
     const open = match.index! + match[0].length - 1;
     const call = registrationAt(source, open);
 
-    for (const found of call.matchAll(/\basync\b/g)) {
+    const suspect = [call, ...factoryBodies(source, call)];
-      const before = call.slice(0, found.index!).trimEnd();
+    if (suspect.some(hasUnwrappedAsync)) {
-      if (!before.endsWith('asyncRoute(')) {
+      offenders.push(`line ${lineOf(source, match.index!)}: ${match[0].trim()}`);
-        offenders.push(`line ${lineOf(source, match.index!)}: ${match[0].trim()}`);
-      }
     }
   }
 
@@ -151,4 +207,40 @@ describe('the guard itself', () => {
 
     expect(unwrappedHandlers(sync)).toEqual([]);
   });
+
+  // The hole #307 closed. A registration built by a factory carries no `async`
+  // of its own, so before this the guard looked at these two lines, found
+  // nothing to object to, and passed — which is not the same as finding them
+  // wrapped. The rotation routes in #301 were the first factory in src/routes
+  // and went through exactly this way.
+  it('follows a factory to the handler it returns', () => {
+    const bad = `
+      function rotationRoute(direction) {
+        return async (req, res) => { res.json({ direction }); };
+      }
+      router.post('/rotate-left', rotationRoute('left'));`;
+
+    expect(unwrappedHandlers(bad)).toHaveLength(1);
+  });
+
+  it('accepts a factory that returns a wrapped handler', () => {
+    const good = `
+      function rotationRoute(direction) {
+        return asyncRoute(async (req, res) => { res.json({ direction }); });
+      }
+      router.post('/rotate-left', rotationRoute('left'));`;
+
+    expect(unwrappedHandlers(good)).toEqual([]);
+  });
+
+  // Imported calls are left alone deliberately. app.ts registers
+  // `express.json()`, `cookieParser()` and `uploadsRouter()`, none of which is
+  // a handler factory and none of which can be read from this file — treating
+  // an unresolvable name as an offender would trade one hole for a permanently
+  // red test.
+  it('ignores a call it cannot resolve in the same file', () => {
+    const imported = `router.use('/uploads', uploadsRouter());`;
+
+    expect(unwrappedHandlers(imported)).toEqual([]);
+  });
 });