From 60021f9ab8dad1c853c1a5b9ca34fb8a4d84d225 Mon Sep 17 00:00:00 2001
From: Gergo Magyar <gergomagyar0@gmail.com>
Date: Wed, 26 Aug 2026 17:46:38 +0000
Subject: [PATCH] fix(ingestion): refuse a @Controller options object whose
 path is not provable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`literalPaths` took the first `pair` named `path` and ignored everything after
it, so a member that overrides `path` at runtime produced a URL the app never
serves:

    @Controller({ path: 'cats', ...options })   ->  /cats, but options.path wins
    @Controller({ path: 'cats', path: 'dogs' }) ->  /cats, but dogs wins

Both are the wrong-fact class this module's header forbids, and both cost every
route on the class rather than one route.

The object branch is now a fail-closed walk, mirroring `routeFromObject` in
data-route-table.ts: skip comments first, then refuse on any non-`pair` child
(spread, computed key, shorthand, method), any unreadable key, and any repeated
key name. Key names compare through `propertyName`, so `path` and `'path'`
collide as duplicates rather than reading as two different keys.

Refusing on a spread positioned BEFORE the literal is deliberate even though JS
evaluation order makes that shape provably safe. The rule is not cost — the walk
already runs in source order, so position tracking would be one boolean — and it
is not blanket parity with the sibling extractor. It is that the shape's real
frequency is unmeasured (this repo contains no NestJS application; all four
`@Controller({` occurrences are its own test fixtures) and the failure directions
are asymmetric: reading it wrong publishes a URL that does not exist, refusing it
omits a route still findable in source. If the new log line shows the shape in
real repos, the position-sensitive rule is the ready upgrade.

`containsExecutingExpression` is deliberately not ported from that sibling. It
guards whole-entry declarativeness for a static route table; here only `path`
needs to be readable, so a computed value on an unrelated key such as
`scope: Scope.REQUEST` stays benign and is pinned as such.

Because a refusal silently costs a whole controller, it now logs under `isDev`
and names the file plus the offending shape. Emitted at `info`, not `debug`:
the logger's base level is already `info`, so an isDev-gated `debug` would be
gated twice and stay silent in exactly the run it exists for. `filePath` is
threaded from `extractNestRoutes` down to the object branch to make that line
useful; without it the message names a shape but not where to find it.

7 of the new cases were red on base for the right reason — each published a
prefix the application never mounts.
---
 .../core/ingestion/route-extractors/nest.ts   | 115 +++++++++++++++---
 .../test/unit/nest-decorator-routes.test.ts   |  83 +++++++++++++
 2 files changed, 181 insertions(+), 17 deletions(-)

diff --git a/gitnexus/src/core/ingestion/route-extractors/nest.ts b/gitnexus/src/core/ingestion/route-extractors/nest.ts
index 1220b5a66..1f052bb25 100644
--- a/gitnexus/src/core/ingestion/route-extractors/nest.ts
+++ b/gitnexus/src/core/ingestion/route-extractors/nest.ts
@@ -44,6 +44,8 @@
 import type Parser from 'tree-sitter';
 import type { ExtractedDecoratorRoute } from '../workers/parse-worker.js';
 import { plainString, propertyName } from './data-route-table.js';
+import { isDev } from '../utils/env.js';
+import { logger } from '../../logger.js';
 
 /**
  * NestJS method decorators → HTTP verb. A Map rather than an object literal
@@ -129,7 +131,10 @@ function decoratorName(decorator: Parser.SyntaxNode): string | null {
  * `:id(d+)`, and `@Get('/v\u0069ews')` came out as `/vews`. Both are paths the
  * app never serves, i.e. the wrong-URL outcome the paragraph above forbids.
  */
-function decoratorLiteralPaths(decorator: Parser.SyntaxNode): readonly string[] | null {
+function decoratorLiteralPaths(
+  decorator: Parser.SyntaxNode,
+  filePath: string,
+): readonly string[] | null {
   const call = decorator.namedChild(0);
   // A bare `@Injectable` with no call, or `@Get()` with no argument — legal,
   // and both mean "no path segment of my own".
@@ -141,7 +146,42 @@ function decoratorLiteralPaths(decorator: Parser.SyntaxNode): readonly string[]
   // Reading it as a route would mint a URL the app never serves, which is the
   // invented fact this module refuses; an unreadable shape drops instead.
   if (first.type === 'object' && decoratorName(decorator) !== 'Controller') return null;
-  return literalPaths(first);
+  return literalPaths(first, filePath);
+}
+
+/**
+ * How much of a refused options object to quote in the dev line. Enough to
+ * recognise the shape, bounded because an options object is arbitrary source.
+ */
+const REFUSED_OBJECT_LOG_LIMIT = 160;
+
+/**
+ * Decline an options object whose `path` is not provable, and say so.
+ *
+ * Gated on `isDev` exactly as the routes phase gates its own registry line
+ * (`pipeline-phases/routes.ts`), and emitted at `info` rather than `debug`
+ * because the logger's base level IS `info`: an `isDev`-gated `logger.debug`
+ * would be gated twice and stay silent in the very dev run it exists for.
+ *
+ * Names the FILE, not just the shape — a controller dropped without a path to
+ * look at is only marginally louder than one dropped in silence, and this
+ * refusal costs every route on the class. The line number is deliberately
+ * absent: `lineOffset` (a Vue SFC `<script>` block shifts every row) is not
+ * threaded this deep, and a row that is wrong for embedded scripts would send
+ * the reader to a line the decorator is not on. The quoted shape locates it.
+ */
+function refuseUnprovableObject(node: Parser.SyntaxNode, filePath: string): null {
+  if (isDev) {
+    const shape = node.text.replace(/\s+/g, ' ');
+    const quoted =
+      shape.length > REFUSED_OBJECT_LOG_LIMIT
+        ? `${shape.slice(0, REFUSED_OBJECT_LOG_LIMIT)}…`
+        : shape;
+    logger.info(
+      `🗺️ NestJS: dropped @Controller in ${filePath} — a member of its options object could override \`path\`: ${quoted}`,
+    );
+  }
+  return null;
 }
 
 /**
@@ -151,23 +191,61 @@ function decoratorLiteralPaths(decorator: Parser.SyntaxNode): readonly string[]
  * what keeps `@Controller({ path: ['a', 'b'] })` from being read by a second,
  * laxer set of rules that has drifted from this one.
  */
-function literalPaths(node: Parser.SyntaxNode): readonly string[] | null {
+function literalPaths(node: Parser.SyntaxNode, filePath: string): readonly string[] | null {
   // `@Controller({ path: 'cats', version: '1' })` is the documented form for
   // URI/header versioning, and its path is a plain literal sitting right there.
   // Worth reading rather than dropping, because the asymmetry is severe: an
   // unreadable METHOD path costs one route, an unreadable PREFIX costs every
   // route on the class.
   if (node.type === 'object') {
-    // `propertyName` reads both spellings that carry a name — `{ path: … }` and
-    // `{ 'path': … }` — so the class is not dropped over a pair of quotes. A
-    // computed key (`{ [KEY]: … }`) has none, and keeps the drop, as does a
-    // computed value.
-    const path = node.namedChildren.find((child) => {
-      const key = child.type === 'pair' ? child.childForFieldName('key') : null;
-      return key !== null && propertyName(key) === 'path';
-    });
-    const value = path?.childForFieldName('value');
-    return value ? literalPaths(value) : null;
+    // But a `path` pair only PROVES the mount when nothing else in the object
+    // can replace it, and the first match proves nothing on its own:
+    // `{ path: 'cats', ...options }` mounts wherever `options.path` says, and
+    // `{ path: 'cats', path: 'dogs' }` mounts at `dogs` — last write wins in
+    // both. Either one publishes `/cats`, a URL the app never serves, and it
+    // looks exactly like a correct one, which is the wrong-answer-dressed-as-
+    // fact this module refuses. So the object is read only when EVERY member is
+    // a named, non-repeated pair. That whole-entry fail-closed walk is the
+    // shape `routeFromObject` uses in `data-route-table.ts`.
+    const values = new Map<string, Parser.SyntaxNode>();
+    for (const child of node.namedChildren) {
+      // Skipped FIRST. A comment between two pairs is ordinary formatting; run
+      // through the not-a-pair test below it would refuse the object and cost
+      // the class every route it has, over a comment.
+      if (child.type === 'comment') continue;
+      // `spread_element` (`{ ...options }`), `shorthand_property_identifier`
+      // (`{ path }`) and `method_definition` (`{ getFoo() {} }`) all land here
+      // — probed and identical across the three grammars this extractor runs
+      // under. None offers a key/value this file can read, and the first can
+      // introduce or overwrite `path` from a value declared elsewhere.
+      if (child.type !== 'pair') return refuseUnprovableObject(node, filePath);
+      const key = child.childForFieldName('key');
+      const value = child.childForFieldName('value');
+      if (key === null || value === null) return refuseUnprovableObject(node, filePath);
+      // Compared through `propertyName`, the same judge used to READ the key —
+      // so `{ path: … }` and `{ 'path': … }` are one key and collide as
+      // duplicates. Comparing raw key text instead makes them two distinct
+      // keys, and `{ path: 'cats', 'path': 'dogs' }` silently mounts the loser.
+      const name = propertyName(key);
+      // No readable name means a computed key (`{ [dynamicKey]: 'b' }`), which
+      // could evaluate to `path` and take the mount with it — refused, not
+      // ignored. A repeated key is refused wherever it appears, not only on
+      // `path`: a duplicate anywhere is evidence the object is not the fixed
+      // literal it reads as, and cost is one controller against a wrong URL.
+      if (name === null || values.has(name)) return refuseUnprovableObject(node, filePath);
+      values.set(name, value);
+    }
+
+    // Deliberately NOT `containsExecutingExpression` (data-route-table.ts): that
+    // guards whole-entry declarativeness for a static route table, a different
+    // invariant. Here only `path` has to be provable, so a non-literal value on
+    // an unrelated key — `{ path: 'a', scope: Scope.REQUEST }`, ordinary Nest —
+    // stays benign and keeps its controller.
+    const path = values.get('path');
+    // A missing `path` keeps the existing drop and must never read as `''`:
+    // `@Controller({ version: '1' })` mounts at a prefix this decorator does
+    // not state, and `''` would publish every one of its methods at the root.
+    return path === undefined ? null : literalPaths(path, filePath);
   }
 
   // `array` is the node type in all three grammars this extractor runs under —
@@ -265,10 +343,13 @@ function classDecorators(classNode: Parser.SyntaxNode): Parser.SyntaxNode[] {
  * extractors solve the same shape and should not disagree about which half of
  * it is supported.
  */
-function controllerPrefix(classNode: Parser.SyntaxNode): string | null | undefined {
+function controllerPrefix(
+  classNode: Parser.SyntaxNode,
+  filePath: string,
+): string | null | undefined {
   for (const decorator of classDecorators(classNode)) {
     if (decoratorName(decorator) !== 'Controller') continue;
-    const paths = decoratorLiteralPaths(decorator);
+    const paths = decoratorLiteralPaths(decorator, filePath);
     // `@Controller([])` lands here too and needs no answer of its own: a
     // controller mounted at no path serves no route, so "emit nothing for this
     // class" is what both readings of it come to.
@@ -297,7 +378,7 @@ export function extractNestRoutes(
 
   const visit = (node: Parser.SyntaxNode): void => {
     if (CLASS_DECLARATION_TYPES.has(node.type)) {
-      const prefix = controllerPrefix(node);
+      const prefix = controllerPrefix(node, filePath);
       // `undefined` — not a controller at all. `null` — a controller whose
       // prefix could not be read, so its routes' URLs are unknowable.
       if (prefix !== undefined) {
@@ -362,7 +443,7 @@ function collectClassRoutes(
       const httpMethod = NEST_METHOD_DECORATORS.get(name);
       if (httpMethod === undefined) continue;
 
-      const routePaths = decoratorLiteralPaths(decorator);
+      const routePaths = decoratorLiteralPaths(decorator, filePath);
       if (routePaths === null) continue; // unreadable → skip
 
       const handlerName = member.childForFieldName('name')?.text;
diff --git a/gitnexus/test/unit/nest-decorator-routes.test.ts b/gitnexus/test/unit/nest-decorator-routes.test.ts
index 927199bb8..d471981d8 100644
--- a/gitnexus/test/unit/nest-decorator-routes.test.ts
+++ b/gitnexus/test/unit/nest-decorator-routes.test.ts
@@ -403,9 +403,92 @@ describe('NestJS decorator routes', () => {
     ).toEqual([]);
   });
 
+  // A `path` pair proves the mount point only when nothing ELSE in the object
+  // can replace it. `{ path: 'cats', ...options }` reads as `cats` under a
+  // first-match scan and mounts wherever `options.path` says at runtime;
+  // `{ path: 'cats', path: 'dogs' }` mounts at `dogs`. Both are the wrong-URL
+  // outcome this module calls worse than a missing one, and both are silent —
+  // a published `/cats` looks exactly like a correct one. So the object is read
+  // only when every member is a named, non-repeated pair: the whole-entry
+  // fail-closed shape `routeFromObject` uses in data-route-table.ts.
+  it.each([
+    // `options.path` overrides the pair above it, so the extracted prefix and
+    // the served prefix disagree with nothing in the file to say so.
+    { label: 'a trailing spread', argument: "{ path: 'cats', ...options }" },
+    // Deterministically SAFE under JS evaluation order — a later `path` pair
+    // always wins over an earlier spread — and refused anyway. Reading member
+    // order as proof makes the verdict turn on which side of the spread the
+    // author happened to type `path`, and how often each spelling occurs in
+    // real controllers is unmeasured. Meanwhile the two failure directions are
+    // not symmetric: reading it wrong publishes a URL the app never serves, and
+    // `route_map`/`api_impact` present that as fact, while refusing omits a
+    // route that is still findable in source.
+    { label: 'a leading spread', argument: "{ ...options, path: 'cats' }" },
+    { label: 'nothing but a spread', argument: '{ ...options }' },
+    // Last write wins at runtime, so a first-match scan names the loser.
+    { label: 'a repeated path key', argument: "{ path: 'cats', path: 'dogs' }" },
+    // Visible only through `propertyName`: compared as raw key text, `path` and
+    // `'path'` are two different keys and the duplicate check never fires.
+    {
+      label: 'a repeated path key in its quoted spelling',
+      argument: "{ path: 'cats', 'path': 'dogs' }",
+    },
+    // Refusal is on ANY repeated key, not only `path` — a duplicate anywhere is
+    // evidence the object is not the fixed literal it reads as.
+    {
+      label: 'a repeated key other than path',
+      argument: "{ path: 'a', version: '1', version: '2' }",
+    },
+    // A computed key could evaluate to `path` and take the mount with it.
+    { label: 'a computed key beside the path', argument: "{ path: 'a', [dynamicKey]: 'b' }" },
+    // `shorthand_property_identifier` and `method_definition`; neither is a
+    // `pair`, so neither offers a key/value this file can read.
+    { label: 'a shorthand property', argument: '{ path }' },
+    { label: 'a method', argument: "{ path: 'a', getFoo() {} }" },
+  ])('refuses an object form whose path another member could override: $label', ({ argument }) => {
+    expect(
+      extract(`
+        @Controller(${argument})
+        export class C {
+          @Get('b') b() {}
+        }
+      `),
+    ).toEqual([]);
+  });
+
+  it.each([
+    // A comment between two pairs is ordinary formatting. It has to be skipped
+    // BEFORE the not-a-pair test above, or the refusal fires on it and costs
+    // the controller every route it has.
+    {
+      label: 'a comment between its pairs',
+      argument: "{ path: 'a', /* URI versioning */ version: '1' }",
+    },
+    // Only `path` has to be provable. A non-literal value on an unrelated key
+    // is benign: `containsExecutingExpression` in data-route-table.ts refuses
+    // these, but it guards whole-entry declarativeness for a static route
+    // table — a different invariant from "can this member move the mount".
+    {
+      label: 'non-literal values on keys other than path',
+      argument: "{ path: 'a', host: 'x', scope: Scope.REQUEST, durable: true }",
+    },
+  ])('still reads the prefix out of an object form with $label', ({ argument }) => {
+    expect(
+      urls(`
+        @Controller(${argument})
+        export class C {
+          @Get('b') b() {}
+        }
+      `),
+    ).toEqual(['GET /a/b']);
+  });
+
   it.each([
     { label: 'a single path', argument: "{ path: 'a' }" },
     { label: 'an array of paths', argument: "{ path: ['a', 'b'] }" },
+    // The verb gate short-circuits on the object form before the object is
+    // walked at all, so the class-form refusal never gets a say here.
+    { label: 'an object the class form would also refuse', argument: "{ path: 'a', ...options }" },
   ])('mints nothing from the object form on a VERB decorator ($label)', ({ argument }) => {
     // `@Controller` takes the object form; `@Get` and friends take
     // `string | string[]`. Nest mounts nothing here, so emitting a route would