From 14491e9f211d411d5e22f84cbaaab55b75ea2161 Mon Sep 17 00:00:00 2001
From: Gergo Magyar <gergomagyar0@gmail.com>
Date: Wed, 26 Aug 2026 19:22:10 +0000
Subject: [PATCH] refactor(ingestion): reuse the shared modifier reader, and
 report the drop where it is decided
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Four cleanup passes over the fix commits. No behaviour change except where
noted; every mutation that guarded these paths still bites.

Reuse — `isRequestHandler` tested `child.type === 'static'`. The repo already
exports `hasKeyword` for exactly this question, uses it at three other sites,
and documents why the type spelling is wrong: `static` reaches the tree as an
anonymous token in some grammar versions and a keyword node in others, so a
type test "silently stops firing on a grammar bump". Here that would have
readmitted precisely the phantom routes 72596b0d0 removes, with the suite green.
`hasKeyword` also skips the `name` field, so a method called `get()` or
`static()` is not misread. Fourth divergent spelling of one predicate, gone.

Altitude — the drop diagnostic moves up to `controllerPrefix`, the single funnel
every whole-controller drop already returns through. It was attached to one
refusal out of four, which made the loudest signal fire for the rarest cause and
left `@Controller(ROUTES.VENUES)` — the case this module's own header calls
motivating — silent. One call site now covers all four. This DELETES
`refuseUnprovableObject`, its constant, and the `filePath` threading through
`literalPaths` and `decoratorLiteralPaths`: better coverage from less code.

The structurally right home is a counted diagnostic that survives the parse
cache, summarized in the routes phase beside the existing duplicate-URL line.
That needs a SCHEMA_BUMP of its own and is a follow-up, not this commit.

Simplification — the three pre-seed sites hand-paired `set` with
`preSeededKeys.add`, making the invariant a convention three call sites had to
remember; a fourth that forgot the `add` would reopen #3049 as silently as the
source-set it replaced. One `preSeed` helper now writes both, which is the shape
`claim()` in call-processor.ts uses and which that comment already cited.

The test file grew three copies of one controller wrapper and two of one
fixture. Now one `memberUrls` helper and one `STATIC_THEN_INSTANCE` constant;
the standalone instance-method control became a row of the table it controls
for, where it reads as the contrast it is.

The ledger paragraph traded ~6 lines re-litigating scan methodology for the
thing a future reader actually needs: why this is still a hand-picked number
when `SCHEMA_FINGERPRINT` next door is a derived digest that cannot collide.
The derivation exists — `resolveAnalyzerRunnerIdentity` computes a build digest
every run — and is rejected because it moves on any build change, including a
comment-only one, forcing a full re-parse for every dev and CI build. That is
the real trade and it was nowhere written down. The residual cost is named too:
a capture change with no bump ships inert and no check can see it, which #2860
does not close.

Declined: merging the two object-refusal tables (they document different rules —
unreadable path value vs unprovable object — and the row labels are the
documentation); extracting a shared object walk into data-route-table.ts (its
`routeFromObject` dedupes only route-token keys and runs
`containsExecutingExpression`, so a shared strict walk would change ITS
behaviour, and a third primitive is not a subtraction).
---
 .../core/ingestion/pipeline-phases/routes.ts  |  34 +++---
 .../core/ingestion/route-extractors/nest.ts   | 108 ++++++++----------
 gitnexus/src/storage/parse-cache.ts           |  27 ++++-
 .../test/unit/nest-decorator-routes.test.ts   |  81 +++++--------
 4 files changed, 117 insertions(+), 133 deletions(-)

diff --git a/gitnexus/src/core/ingestion/pipeline-phases/routes.ts b/gitnexus/src/core/ingestion/pipeline-phases/routes.ts
index ba09263bc..e93fa033c 100644
--- a/gitnexus/src/core/ingestion/pipeline-phases/routes.ts
+++ b/gitnexus/src/core/ingestion/pipeline-phases/routes.ts
@@ -256,35 +256,33 @@ export const routesPhase: PipelinePhase<RoutesOutput> = {
       }
     }
 
+    // One writer for every pre-seeded route, so recording membership cannot be
+    // forgotten. Inlining `has` / `set` / `add` at each site made the invariant
+    // a convention three call sites had to remember — and a fourth source that
+    // forgot the `add` would reopen #3049 exactly as silently as the source-set
+    // it replaced. This is the shape `claim()` in call-processor.ts uses for the
+    // same reason: one helper writes the collection and its key set together.
+    const preSeed = (url: string, entry: Omit<RouteEntry, 'url'>): boolean => {
+      if (routeRegistry.has(url)) return false;
+      routeRegistry.set(url, { ...entry, url });
+      preSeededKeys.add(url);
+      return true;
+    };
+
     for (const p of allPaths) {
       if (expoAppPaths.has(p)) {
         const expoURL = expoFileToRouteURL(p);
-        if (expoURL && !routeRegistry.has(expoURL)) {
-          routeRegistry.set(expoURL, {
-            filePath: p,
-            source: 'expo-filesystem-route',
-            url: expoURL,
-          });
-          preSeededKeys.add(expoURL);
+        if (expoURL && preSeed(expoURL, { filePath: p, source: 'expo-filesystem-route' })) {
           continue;
         }
       }
       const nextjsURL = nextjsFileToRouteURL(p);
-      if (nextjsURL && !routeRegistry.has(nextjsURL)) {
-        routeRegistry.set(nextjsURL, {
-          filePath: p,
-          source: 'nextjs-filesystem-route',
-          url: nextjsURL,
-        });
-        preSeededKeys.add(nextjsURL);
+      if (nextjsURL && preSeed(nextjsURL, { filePath: p, source: 'nextjs-filesystem-route' })) {
         continue;
       }
       if (p.endsWith('.php')) {
         const phpURL = phpFileToRouteURL(p);
-        if (phpURL && !routeRegistry.has(phpURL)) {
-          routeRegistry.set(phpURL, { filePath: p, source: 'php-file-route', url: phpURL });
-          preSeededKeys.add(phpURL);
-        }
+        if (phpURL) preSeed(phpURL, { filePath: p, source: 'php-file-route' });
       }
     }
 
diff --git a/gitnexus/src/core/ingestion/route-extractors/nest.ts b/gitnexus/src/core/ingestion/route-extractors/nest.ts
index e1b90124f..73d34ddcd 100644
--- a/gitnexus/src/core/ingestion/route-extractors/nest.ts
+++ b/gitnexus/src/core/ingestion/route-extractors/nest.ts
@@ -44,6 +44,7 @@
 import type Parser from 'tree-sitter';
 import type { ExtractedDecoratorRoute } from '../workers/parse-worker.js';
 import { plainString, propertyName } from './data-route-table.js';
+import { hasKeyword } from '../field-extractors/configs/helpers.js';
 import { isDev } from '../utils/env.js';
 import { logger } from '../../logger.js';
 
@@ -131,10 +132,7 @@ 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,
-  filePath: string,
-): readonly string[] | null {
+function decoratorLiteralPaths(decorator: Parser.SyntaxNode): 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".
@@ -146,42 +144,7 @@ function decoratorLiteralPaths(
   // 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, 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;
+  return literalPaths(first);
 }
 
 /**
@@ -191,7 +154,7 @@ function refuseUnprovableObject(node: Parser.SyntaxNode, filePath: string): null
  * 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, filePath: string): readonly string[] | null {
+function literalPaths(node: Parser.SyntaxNode): 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
@@ -218,10 +181,10 @@ function literalPaths(node: Parser.SyntaxNode, filePath: string): readonly strin
       // — 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);
+      if (child.type !== 'pair') return null;
       const key = child.childForFieldName('key');
       const value = child.childForFieldName('value');
-      if (key === null || value === null) return refuseUnprovableObject(node, filePath);
+      if (key === null || value === null) return null;
       // 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
@@ -232,7 +195,7 @@ function literalPaths(node: Parser.SyntaxNode, filePath: string): readonly strin
       // 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);
+      if (name === null || values.has(name)) return null;
       values.set(name, value);
     }
 
@@ -245,7 +208,7 @@ function literalPaths(node: Parser.SyntaxNode, filePath: string): readonly strin
     // 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);
+    return path === undefined ? null : literalPaths(path);
   }
 
   // `array` is the node type in all three grammars this extractor runs under —
@@ -313,6 +276,13 @@ function leadingDecorators(node: Parser.SyntaxNode): Parser.SyntaxNode[] {
   return out;
 }
 
+/**
+ * Cap on the decorator text quoted in the dropped-controller log. Long enough
+ * to identify the shape, short enough not to dump a wrapped multi-line
+ * decorator into the operator's terminal.
+ */
+const DROPPED_CONTROLLER_LOG_LIMIT = 160;
+
 /**
  * Every decorator attached to a class, across the two shapes the grammar
  * produces — which differ by whether the class is exported:
@@ -349,11 +319,33 @@ function controllerPrefix(
 ): string | null | undefined {
   for (const decorator of classDecorators(classNode)) {
     if (decoratorName(decorator) !== 'Controller') continue;
-    const paths = decoratorLiteralPaths(decorator, filePath);
+    const paths = decoratorLiteralPaths(decorator);
     // `@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.
-    if (paths === null || paths.length !== 1) return null;
+    if (paths === null || paths.length !== 1) {
+      // The single funnel for EVERY whole-controller drop — an unreadable
+      // constant (`@Controller(ROUTES.VENUES)`), a multi-path array, an
+      // unreadable array element, and an options object whose `path` another
+      // member could override all return null here. Reporting at the refusal
+      // sites instead would make the rarest cause the loudest, and leave the
+      // motivating one from this module's own header silent.
+      //
+      // `isDev` at `info`, not `debug`: the logger's base level IS `info`, so
+      // an isDev-gated `debug` is gated twice and stays silent in exactly the
+      // dev run it exists for. Same shape the routes phase uses.
+      if (isDev) {
+        const shape = decorator.text.replace(/\s+/g, ' ');
+        logger.info(
+          `🗺️ NestJS: dropped @Controller in ${filePath} — its prefix is not provable: ${
+            shape.length > DROPPED_CONTROLLER_LOG_LIMIT
+              ? `${shape.slice(0, DROPPED_CONTROLLER_LOG_LIMIT)}…`
+              : shape
+          }`,
+        );
+      }
+      return null;
+    }
     return paths[0];
   }
   return undefined;
@@ -404,7 +396,7 @@ export function extractNestRoutes(
  * route minted from one is a URL the app does not serve — the invented fact
  * this module refuses everywhere else.
  */
-const NON_HANDLER_MODIFIERS: ReadonlySet<string> = new Set(['static', 'get', 'set']);
+const NON_HANDLER_MODIFIERS: readonly string[] = ['static', 'get', 'set'];
 
 /**
  * Whether Nest could register this `method_definition` as a request handler.
@@ -418,21 +410,21 @@ const NON_HANDLER_MODIFIERS: ReadonlySet<string> = new Set(['static', 'get', 'se
  * `namedChildren` idiom cannot see the modifier at all and every one of the
  * three reads as an ordinary handler.
  *
- * Tests every child's TYPE rather than indexing a position, because the
- * position is not stable: under tree-sitter-javascript a method decorator is a
- * CHILD of `method_definition`, so `children[0]` there is the `decorator` and
- * the modifier sits behind it, while under tree-sitter-typescript the decorator
- * is a preceding sibling and the modifier comes first.
+ * Uses the shared `hasKeyword`, which the TypeScript and JavaScript captures
+ * and the TS/JS method extractor already use to decide this same question. It
+ * matches on child TEXT, not node type: `static` reaches the tree as an
+ * anonymous token in some grammar versions and as a keyword node in others, so
+ * a `child.type === 'static'` test silently stops firing on a grammar bump —
+ * and here that would readmit exactly the phantom routes this function removes,
+ * with the suite still green. It also skips the `name` field, so a method
+ * literally called `get()` or `static()` is not misread as a modifier.
  *
  * The scan is bounded by one method's own children (a handful), so it is not
  * the uncached-getter trap {@link collectClassRoutes} documents — that one bites
  * when a PARENT's child list is re-marshalled once per member.
  */
 function isRequestHandler(member: Parser.SyntaxNode): boolean {
-  for (const child of member.children) {
-    if (NON_HANDLER_MODIFIERS.has(child.type)) return false;
-  }
-  return true;
+  return !NON_HANDLER_MODIFIERS.some((keyword) => hasKeyword(member, keyword));
 }
 
 function collectClassRoutes(
@@ -495,7 +487,7 @@ function collectClassRoutes(
       const httpMethod = NEST_METHOD_DECORATORS.get(name);
       if (httpMethod === undefined) continue;
 
-      const routePaths = decoratorLiteralPaths(decorator, filePath);
+      const routePaths = decoratorLiteralPaths(decorator);
       if (routePaths === null) continue; // unreadable → skip
 
       const handlerName = member.childForFieldName('name')?.text;
diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts
index 689b81cb2..d1ab66d3f 100644
--- a/gitnexus/src/storage/parse-cache.ts
+++ b/gitnexus/src/storage/parse-cache.ts
@@ -619,11 +619,28 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j
 // when you pick the number.
 //
 // 77 is free at this merge: origin/main is 76, and the open PRs touching this
-// constant are #2840 (a stale 71) and #1616 (a stale 2). Note the scan method
-// matters — `gh pr diff` exits non-zero on an inaccessible fork and prints
-// nothing, so a grep over its output silently skips that PR. #2840 was missed
-// exactly that way and had to be read through the contents API at the PR head.
-// An "exhaustive" scan that fails open is how this constant keeps colliding.
+// constant are #2840 (a stale 71) and #1616 (a stale 2). Scan with the contents
+// API at each PR head, not `gh pr diff` — that exits non-zero on an
+// inaccessible fork and prints nothing, so a grep over its output skips the PR
+// silently. #2840 was missed exactly that way this round.
+//
+// WHY THIS IS STILL A HAND-PICKED NUMBER, when `SCHEMA_FINGERPRINT` next door
+// is a derived sha256 that cannot collide. The derivation exists and already
+// runs: `resolveAnalyzerRunnerIdentity` computes `build.digest` over the
+// analyzer build tree on every analyze. It is not used here because it moves on
+// ANY build change — a comment-only edit, this paragraph included — so every
+// dev and CI rebuild would force a full re-parse of every repo. That is the
+// expensive half of the trade, and `PARSE_CACHE_VERSION` already carries the
+// package version, so this counter's only job is separating builds that SHARE
+// one: dev, CI, unreleased. Exactly the population a whole-build digest would
+// punish, and exactly the population that hits the exact-clash failure.
+//
+// So the counter stays, and the open cost is that a bump nobody makes is
+// invisible: a capture change with no bump ships inert and no check can see it.
+// #2860 (a base-branch CI comparison) closes the "not greater than main" axis
+// only. A digest over just the determinant subset — the language queries plus
+// `route-extractors/` and `workers/` module content — would close the missing-
+// bump axis without invalidating on unrelated churn, and is the real follow-up.
 // RE-CHECK AGAINST origin/main AND OPEN PRs IMMEDIATELY BEFORE MERGING.
 const SCHEMA_BUMP = 77;
 const GITNEXUS_PKG_VERSION = (() => {
diff --git a/gitnexus/test/unit/nest-decorator-routes.test.ts b/gitnexus/test/unit/nest-decorator-routes.test.ts
index e6894a271..970fedba1 100644
--- a/gitnexus/test/unit/nest-decorator-routes.test.ts
+++ b/gitnexus/test/unit/nest-decorator-routes.test.ts
@@ -602,6 +602,27 @@ describe('NestJS decorator routes', () => {
 
   // ─── Members Nest never registers as handlers ──────────────────────
 
+  /** Routes a controller emits when `member` is its only member. */
+  const memberUrls = (member: string) =>
+    urls(`
+      @Controller('v')
+      export class C {
+        ${member}
+      }
+    `);
+
+  /** A non-handler followed by a real one — the shape both arms below need. */
+  const STATIC_THEN_INSTANCE = `
+      @Controller('v')
+      export class C {
+        @Get('s')
+        static s() {}
+
+        @Get('i')
+        i() {}
+      }
+    `;
+
   // Nest's `RequestMapping` writes the handler onto the class PROTOTYPE's
   // `descriptor.value`, and `RouterExplorer` scans prototype instance methods
   // for that metadata. A `static` method lives on the constructor and is never
@@ -614,14 +635,7 @@ describe('NestJS decorator routes', () => {
     { label: 'a getter', member: "@Get('s') get s(): string { return ''; }" },
     { label: 'a setter', member: "@Get('s') set s(v: string) {}" },
   ])('mints nothing for $label, which Nest never registers as a handler', ({ member }) => {
-    expect(
-      extract(`
-        @Controller('v')
-        export class C {
-          ${member}
-        }
-      `),
-    ).toEqual([]);
+    expect(memberUrls(member)).toEqual([]);
   });
 
   // The other half of the modifier check, and the half a mutation can actually
@@ -642,29 +656,12 @@ describe('NestJS decorator routes', () => {
     },
     { label: 'a method named get', member: "@Get('s') get() {}" },
     { label: 'a method named static', member: "@Get('s') static() {}" },
-  ])('still emits the route for $label', ({ member }) => {
-    expect(
-      urls(`
-        @Controller('v')
-        export class C {
-          ${member}
-        }
-      `),
-    ).toEqual(['GET /v/s']);
-  });
-
-  it('still emits the route for that same decorator on an instance method', () => {
-    // The control for the table above — identical source minus the modifier.
-    // It is what makes those three empty results evidence of the modifier
+    // The control for the mints-nothing table above: identical source minus the
+    // modifier, which is what makes those three empty results evidence of the
     // check rather than of a fixture that happens to parse to nothing.
-    expect(
-      urls(`
-        @Controller('v')
-        export class C {
-          @Get('s') s() {}
-        }
-      `),
-    ).toEqual(['GET /v/s']);
+    { label: 'a plain instance method', member: "@Get('s') s() {}" },
+  ])('still emits the route for $label', ({ member }) => {
+    expect(memberUrls(member)).toEqual(['GET /v/s']);
   });
 
   it('drops a decorated static method under the JavaScript grammar too', () => {
@@ -674,18 +671,7 @@ describe('NestJS decorator routes', () => {
     // check has to test every child's type. The instance method beside it is
     // the in-fixture control: its route proves this .js arm still measures
     // something rather than passing on a fixture that parses to nothing.
-    expect(
-      jsUrls(`
-        @Controller('v')
-        export class C {
-          @Get('s')
-          static s() {}
-
-          @Get('i')
-          i() {}
-        }
-      `),
-    ).toEqual(['GET /v/i']);
+    expect(jsUrls(STATIC_THEN_INSTANCE)).toEqual(['GET /v/i']);
   });
 
   it("does not donate a non-handler member's decorator run to the method after it", () => {
@@ -697,16 +683,7 @@ describe('NestJS decorator routes', () => {
     // if a future edit adds the early `continue`. Asserted over the routes
     // attributed to `i`, because the whole-output form would instead be
     // measuring the modifier check the table above already covers.
-    const routes = extract(`
-      @Controller('v')
-      export class C {
-        @Get('s')
-        static s() {}
-
-        @Get('i')
-        i() {}
-      }
-    `);
+    const routes = extract(STATIC_THEN_INSTANCE);
 
     expect(format(routes.filter((route) => route.handlerName === 'i'))).toEqual(['GET /v/i']);
   });