From 1cee567865284be756e6676de297bee680b068b2 Mon Sep 17 00:00:00 2001
From: JSONbored <49853598+JSONbored@users.noreply.github.com>
Date: Fri, 10 Jul 2026 04:28:16 -0700
Subject: [PATCH] refactor(review): consolidate 3 duplicate helper
 implementations (#4611)

Three independent, low-risk duplicate cleanups from the review-stack
architecture audit's duplication dimension:

- isCommentLine: extracted the byte-identical body shared by unsafe-any.ts,
  complexity.ts, and floating-promise.ts into a new isBasicCommentLine
  export in review-enrichment's diff-lines.ts. hardcoded-url.ts and
  a11y-regression.ts keep their own isCommentLine override, now composing
  the shared base plus their deliberate per-language extras (shell/Python
  #, HTML <!--, JSX import/from) instead of re-deriving the whole regex.
- authoritativeContributorRepoStats: exported the single implementation
  from services/decision-pack.ts and pointed api/routes.ts, mcp/server.ts,
  and queue/processors.ts at it instead of each carrying its own copy.
- hasUnsafeWildcardCount: packages/gittensory-engine/src/scoring/preview.ts
  now imports the wildcard-safety check from its own package's
  signals/change-guardrail.ts instead of re-deriving a third hand-
  maintained copy; the guarded host/engine change-guardrail.ts pair is
  untouched.

No behavior change. Validated with typecheck, the engine package's own
build and test suite, the engine-parity contract test, review-enrichment's
node:test suite, and targeted vitest runs (coverage scoped to the changed
files) against the decision-pack, api, mcp, queue, and scoring tests.
---
 .../gittensory-engine/src/scoring/preview.ts  | 30 ++++---------------
 .../src/analyzers/a11y-regression.ts          |  6 +++-
 review-enrichment/src/analyzers/complexity.ts | 10 ++-----
 review-enrichment/src/analyzers/diff-lines.ts | 16 ++++++++++
 .../src/analyzers/floating-promise.ts         |  8 ++---
 .../src/analyzers/hardcoded-url.ts            |  6 +++-
 review-enrichment/src/analyzers/unsafe-any.ts |  8 ++---
 review-enrichment/test/diff-lines.test.ts     | 17 ++++++++++-
 src/api/routes.ts                             |  9 +-----
 src/mcp/server.ts                             | 10 +------
 src/queue/processors.ts                       | 12 +-------
 src/services/decision-pack.ts                 |  6 +++-
 12 files changed, 63 insertions(+), 75 deletions(-)

diff --git a/packages/gittensory-engine/src/scoring/preview.ts b/packages/gittensory-engine/src/scoring/preview.ts
index c9f93ed0df..b7aa3cd185 100644
--- a/packages/gittensory-engine/src/scoring/preview.ts
+++ b/packages/gittensory-engine/src/scoring/preview.ts
@@ -1,10 +1,14 @@
 import type { ContributorEvidenceRecord, JsonValue, RepositoryRecord, RepoTimeDecayOverrides, ScoringModelSnapshotRecord, ScorePreviewRecord } from "./types.js";
 import { DEFAULT_SCORING_CONSTANTS } from "./model.js";
+import { hasUnsafeWildcardCount } from "../signals/change-guardrail.js";
 
 // Deterministic score-preview builder extracted verbatim from the backend's `src/scoring/preview.ts`
 // (#2282) — this file has no D1/network/env dependency in the original, so it ports unchanged aside from
-// its imports and the two tiny pure helpers (`nowIso`, `hasUnsafeWildcardCount`) inlined below, which the
-// backend sources from `src/utils/json.ts` and `src/signals/change-guardrail.ts` respectively.
+// its imports and one tiny pure helper (`nowIso`) inlined below, which the backend sources from
+// `src/utils/json.ts`. `hasUnsafeWildcardCount` is imported from this package's own
+// `signals/change-guardrail.ts` (#4611) rather than re-derived here — that file is a verbatim port of the
+// backend's `src/signals/change-guardrail.ts`, kept in sync by the engine-parity contract test, so importing
+// it carries the same ReDoS-safety guarantee without a third hand-maintained copy.
 
 // The package's tsconfig sets `types: []` (no ambient DOM/Node globals, keeping the engine's type surface
 // independent of any consumer's lib config), so the Web Crypto global needs a minimal local declaration.
@@ -16,28 +20,6 @@ function nowIso(): string {
   return new Date().toISOString();
 }
 
-// Mirrors `src/signals/change-guardrail.ts`'s `hasUnsafeWildcardCount`/wildcard-group counting exactly —
-// see that file for the full ReDoS-safety rationale. Duplicated here (rather than imported) because this
-// package cannot reach into `src/`; keep the two in sync by hand.
-const MAX_GLOB_WILDCARD_GROUPS = 2;
-
-function countWildcardGroups(glob: string): number {
-  let count = 0;
-  for (let i = 0; i < glob.length; i += 1) {
-    if (glob.charAt(i) !== "*") continue;
-    count += 1;
-    if (glob.charAt(i + 1) === "*") {
-      i += 1; // consume the second star of the "**" pair — one group, not two
-      if (glob.charAt(i + 1) === "/") i += 1; // `**/` also matches zero segments, mirroring globToRegExp
-    }
-  }
-  return count;
-}
-
-function hasUnsafeWildcardCount(glob: string): boolean {
-  return countWildcardGroups(glob) > MAX_GLOB_WILDCARD_GROUPS;
-}
-
 export type ScorePreviewInput = {
   repoFullName: string;
   targetType?: ScorePreviewRecord["targetType"];
diff --git a/review-enrichment/src/analyzers/a11y-regression.ts b/review-enrichment/src/analyzers/a11y-regression.ts
index 0d4b2beeaf..a7b39a470a 100644
--- a/review-enrichment/src/analyzers/a11y-regression.ts
+++ b/review-enrichment/src/analyzers/a11y-regression.ts
@@ -4,6 +4,7 @@
 import type { A11yFinding, EnrichRequest } from "../types.js";
 import { isTestPath } from "./test-ratio.js";
 import { DEFAULT_MAX_FINDINGS, DEFAULT_MAX_LINE_CHARS } from "./limits.js";
+import { isBasicCommentLine } from "./diff-lines.js";
 
 const MAX_FINDINGS = DEFAULT_MAX_FINDINGS;
 const MAX_LINE_CHARS = DEFAULT_MAX_LINE_CHARS;
@@ -24,9 +25,12 @@ const NON_INTERACTIVE_CLICK_TARGET_RE =
 const FORM_CONTROL_RE = /<(?:input|select|textarea)\b/i;
 const LABEL_ASSOC_RE = /\b(?:aria-label|aria-labelledby|id)\s*=|<label\b/i;
 
+// Layers the HTML `<!--` comment form and JSX-adjacent `import`/`from` statements on top of the shared
+// `isBasicCommentLine` base (#4611) — this analyzer scans markup (.jsx/.tsx/.html/.vue) where an import
+// line is boilerplate, not a markup regression candidate, and HTML comments are common.
 function isCommentLine(line: string): boolean {
   const trimmed = line.trimStart();
-  return /^(?:\/\/|\/\*|\*|<!--|import\b|from\b)/.test(trimmed);
+  return isBasicCommentLine(line) || /^(?:<!--|import\b|from\b)/.test(trimmed);
 }
 
 function isMarkupPath(path: string): boolean {
diff --git a/review-enrichment/src/analyzers/complexity.ts b/review-enrichment/src/analyzers/complexity.ts
index 9e8d248e97..1ac5435085 100644
--- a/review-enrichment/src/analyzers/complexity.ts
+++ b/review-enrichment/src/analyzers/complexity.ts
@@ -28,6 +28,7 @@ import type { ComplexityFinding, EnrichRequest } from "../types.js";
 import { codeOnly } from "./secret-log.js";
 import { isTestPath } from "./test-ratio.js";
 import { DEFAULT_MAX_FINDINGS, DEFAULT_MAX_LINE_CHARS } from "./limits.js";
+import { isBasicCommentLine } from "./diff-lines.js";
 
 export const DEFAULT_MAX_COMPLEXITY = 10;
 const MAX_FINDINGS = DEFAULT_MAX_FINDINGS;
@@ -59,11 +60,6 @@ function isJsTsPath(path: string): boolean {
   return JS_TS_PATH_RE.test(path) && !isTestPath(path);
 }
 
-function isCommentLine(line: string): boolean {
-  const trimmed = line.trimStart();
-  return /^(?:\/\/|\/\*|\*)/.test(trimmed);
-}
-
 /** Count decision-point tokens (if/for/while/case/catch/&&/||/??) in one code fragment. Pure. */
 export function countDecisionPoints(code: string): number {
   let total = 0;
@@ -77,7 +73,7 @@ export function countDecisionPoints(code: string): number {
 /** The declared/assigned name when a line opens a named function declaration or an arrow function assigned to a
  *  const/let/var -- the same structural scope size-smell.ts's function detection uses. Pure. */
 export function functionNameFromLine(line: string): string | undefined {
-  if (isCommentLine(line)) return undefined;
+  if (isBasicCommentLine(line)) return undefined;
   const match = FUNCTION_OPEN_RE.exec(codeOnly(line));
   return match?.[1] ?? match?.[2];
 }
@@ -177,7 +173,7 @@ export function scanPatchForComplexity(
     if (line.startsWith("+")) {
       const body = line.slice(1);
       if (body.length <= MAX_LINE_CHARS) {
-        const commented = isCommentLine(body);
+        const commented = isBasicCommentLine(body);
         const code = codeOnly(body);
         pending = advancePendingFunction(pending, body, commented, code, newLine);
         if (pending && pending.depth <= 0) flushFunction();
diff --git a/review-enrichment/src/analyzers/diff-lines.ts b/review-enrichment/src/analyzers/diff-lines.ts
index 19b70d80c7..6949e72a55 100644
--- a/review-enrichment/src/analyzers/diff-lines.ts
+++ b/review-enrichment/src/analyzers/diff-lines.ts
@@ -15,3 +15,19 @@
 export function isDiffFileHeaderLine(line: string): boolean {
   return /^(?:\+\+\+|---) (?:[ab]\/|\/dev\/null)/.test(line);
 }
+
+/**
+ * True for a line whose visible content opens with a `//` line comment, a `/* ` block-comment opener, or a
+ * `*` block-comment continuation — the baseline "this added line is not real code" check shared by analyzers
+ * that skip comment lines before pattern-matching (#4611).
+ *
+ * This is the common BASE only. `hardcoded-url.ts` and `a11y-regression.ts` each layer additional
+ * language-specific comment forms on top of it (shell/Python `#`, HTML `<!--`, JSX-adjacent `import`/`from`)
+ * via their own local `isCommentLine` override — those two are deliberately not folded in here, since e.g. a
+ * `#` line-start is a real comment in Python but a real (and common) Markdown heading / hex-color / URL
+ * fragment elsewhere, so it isn't a safe universal default.
+ */
+export function isBasicCommentLine(line: string): boolean {
+  const trimmed = line.trimStart();
+  return /^(?:\/\/|\/\*|\*)/.test(trimmed);
+}
diff --git a/review-enrichment/src/analyzers/floating-promise.ts b/review-enrichment/src/analyzers/floating-promise.ts
index f8e2886bc6..baf16e559f 100644
--- a/review-enrichment/src/analyzers/floating-promise.ts
+++ b/review-enrichment/src/analyzers/floating-promise.ts
@@ -6,6 +6,7 @@ import type { EnrichRequest, FloatingPromiseFinding } from "../types.js";
 import { codeOnly } from "./secret-log.js";
 import { isTestPath } from "./test-ratio.js";
 import { DEFAULT_MAX_FINDINGS, DEFAULT_MAX_LINE_CHARS } from "./limits.js";
+import { isBasicCommentLine } from "./diff-lines.js";
 
 const MAX_FINDINGS = DEFAULT_MAX_FINDINGS;
 const MAX_LINE_CHARS = DEFAULT_MAX_LINE_CHARS;
@@ -22,11 +23,6 @@ function isJsTsPath(path: string): boolean {
   return JS_TS_PATH_RE.test(path) && !isTestPath(path);
 }
 
-function isCommentLine(line: string): boolean {
-  const trimmed = line.trimStart();
-  return /^(?:\/\/|\/\*|\*)/.test(trimmed);
-}
-
 function truncateCall(call: string): string {
   if (call.length <= MAX_CALL_CHARS) return call;
   return `${call.slice(0, MAX_CALL_CHARS - 3)}...`;
@@ -58,7 +54,7 @@ function extractLeadingCallCallee(line: string): string | null {
 
 /** Classify one added line for a floating promise call, or null. Pure. */
 export function detectFloatingPromise(line: string): string | null {
-  if (isCommentLine(line) || HANDLED_PREFIX.test(line)) {
+  if (isBasicCommentLine(line) || HANDLED_PREFIX.test(line)) {
     return null;
   }
 
diff --git a/review-enrichment/src/analyzers/hardcoded-url.ts b/review-enrichment/src/analyzers/hardcoded-url.ts
index 56ce031997..ea7ed1ece1 100644
--- a/review-enrichment/src/analyzers/hardcoded-url.ts
+++ b/review-enrichment/src/analyzers/hardcoded-url.ts
@@ -5,6 +5,7 @@
 import type { EnrichRequest, HardcodedUrlFinding } from "../types.js";
 import { isMagicNumberSourcePath } from "./magic-number.js";
 import { DEFAULT_MAX_FINDINGS, DEFAULT_MAX_LINE_CHARS } from "./limits.js";
+import { isBasicCommentLine } from "./diff-lines.js";
 
 const MAX_FINDINGS = DEFAULT_MAX_FINDINGS;
 const MAX_LINE_CHARS = DEFAULT_MAX_LINE_CHARS;
@@ -44,9 +45,12 @@ function hostFromHttpUrl(url: string): string {
   return match?.[1] ?? url;
 }
 
+// Layers the shell/Python `#` and HTML `<!--` comment forms on top of the shared `isBasicCommentLine` base
+// (#4611) — this analyzer scans non-TS source (Dockerfiles, shell scripts, YAML) where `#` is a real comment
+// marker, unlike the shared base's TS/JS-only `//`/`/* `/`*` forms.
 function isCommentLine(line: string): boolean {
   const trimmed = line.trimStart();
-  return /^(?:\/\/|#|\/\*|\*|<!--)/.test(trimmed);
+  return isBasicCommentLine(line) || /^(?:#|<!--)/.test(trimmed);
 }
 
 function isImportLine(line: string): boolean {
diff --git a/review-enrichment/src/analyzers/unsafe-any.ts b/review-enrichment/src/analyzers/unsafe-any.ts
index 5245177396..7a11db9ec1 100644
--- a/review-enrichment/src/analyzers/unsafe-any.ts
+++ b/review-enrichment/src/analyzers/unsafe-any.ts
@@ -5,6 +5,7 @@ import type { EnrichRequest, UnsafeAnyFinding } from "../types.js";
 import { codeOnly } from "./secret-log.js";
 import { isTestPath } from "./test-ratio.js";
 import { DEFAULT_MAX_FINDINGS, DEFAULT_MAX_LINE_CHARS } from "./limits.js";
+import { isBasicCommentLine } from "./diff-lines.js";
 
 const MAX_FINDINGS = DEFAULT_MAX_FINDINGS;
 const MAX_LINE_CHARS = DEFAULT_MAX_LINE_CHARS;
@@ -15,14 +16,9 @@ function isTsPath(path: string): boolean {
   return TS_PATH_RE.test(path) && !isTestPath(path);
 }
 
-function isCommentLine(line: string): boolean {
-  const trimmed = line.trimStart();
-  return /^(?:\/\/|\/\*|\*)/.test(trimmed);
-}
-
 /** Classify one added line for an unsafe `any` pattern, or null. Pure. */
 export function detectUnsafeAny(line: string): UnsafeAnyFinding["kind"] | null {
-  if (isCommentLine(line) || line.length > MAX_LINE_CHARS) return null;
+  if (isBasicCommentLine(line) || line.length > MAX_LINE_CHARS) return null;
   const code = codeOnly(line);
   if (/\bas any\b/.test(code)) return "cast";
   if (/<any>/.test(code)) return "assertion";
diff --git a/review-enrichment/test/diff-lines.test.ts b/review-enrichment/test/diff-lines.test.ts
index b186be5f39..b638f2947a 100644
--- a/review-enrichment/test/diff-lines.test.ts
+++ b/review-enrichment/test/diff-lines.test.ts
@@ -2,7 +2,7 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
 
-import { isDiffFileHeaderLine } from "../dist/analyzers/diff-lines.js";
+import { isBasicCommentLine, isDiffFileHeaderLine } from "../dist/analyzers/diff-lines.js";
 
 test("isDiffFileHeaderLine matches real file headers only, not ++/--- content", () => {
   // Real unified-diff file headers → skipped.
@@ -15,3 +15,18 @@ test("isDiffFileHeaderLine matches real file headers only, not ++/--- content",
     assert.equal(isDiffFileHeaderLine(content), false, content);
   }
 });
+
+test("isBasicCommentLine matches //, /*, and * comment openers, leading whitespace included", () => {
+  for (const line of ["// a note", "  // indented", "/* block open", "* jsdoc continuation", "   * indented continuation"]) {
+    assert.equal(isBasicCommentLine(line), true, line);
+  }
+  // Not real code either, but outside this shared base's scope — analyzers that need these layer their own
+  // override on top (hardcoded-url.ts's `#`/`<!--`, a11y-regression.ts's `<!--`/`import`/`from`).
+  for (const line of ["# shell/python comment", "<!-- html comment -->", "import x from 'y'", "from y import x"]) {
+    assert.equal(isBasicCommentLine(line), false, line);
+  }
+  // Real code → never flagged.
+  for (const line of ["const x = 1;", "  return a && b;", "export function run() {"]) {
+    assert.equal(isBasicCommentLine(line), false, line);
+  }
+});
diff --git a/src/api/routes.ts b/src/api/routes.ts
index f6293414b6..e074d17300 100644
--- a/src/api/routes.ts
+++ b/src/api/routes.ts
@@ -166,6 +166,7 @@ import { decidePendingAgentAction } from "../services/agent-approval-queue";
 import { explainScoreBreakdown } from "../services/score-breakdown";
 import { buildMcpClientTelemetry } from "../services/client-telemetry";
 import {
+  authoritativeContributorRepoStats,
   buildAndPersistContributorDecisionPack,
   CONTRIBUTOR_DECISION_PACK_SIGNAL,
   loadContributorDecisionPackForServing,
@@ -5204,14 +5205,6 @@ function parseBackfillSegment(value: unknown): Extract<JobMessage, { type: "back
   return value === "labels" || value === "open_issues" || value === "open_pull_requests" || value === "recent_merged_pull_requests" ? value : null;
 }
 
-function authoritativeContributorRepoStats(
-  gittensorSnapshot: Awaited<ReturnType<typeof fetchGittensorContributorSnapshot>>,
-  cachedRepoStats: Awaited<ReturnType<typeof listContributorRepoStats>>,
-) {
-  const officialRepoStats = contributorRepoStatsFromGittensor(gittensorSnapshot);
-  return officialRepoStats.length > 0 ? officialRepoStats : cachedRepoStats;
-}
-
 async function persistSignal(
   env: Env,
   signalType: string,
diff --git a/src/mcp/server.ts b/src/mcp/server.ts
index 9de690e595..e9c3c0df35 100644
--- a/src/mcp/server.ts
+++ b/src/mcp/server.ts
@@ -86,7 +86,7 @@ import {
   preparePrPacketWithAgent,
   startAgentRun,
 } from "../services/agent-orchestrator";
-import { loadContributorDecisionPackForServing, repoDecisionFromPack } from "../services/decision-pack";
+import { authoritativeContributorRepoStats, loadContributorDecisionPackForServing, repoDecisionFromPack } from "../services/decision-pack";
 import { buildPublicPrBodyDraft } from "../services/pr-body-draft";
 import { buildRemediationPlan } from "../services/remediation-plan";
 import { deriveEligibilityPlan } from "../services/eligibility-plan";
@@ -3782,14 +3782,6 @@ function redactSensitiveForMcp(value: unknown): unknown {
   );
 }
 
-function authoritativeContributorRepoStats(
-  gittensorSnapshot: Awaited<ReturnType<typeof fetchGittensorContributorSnapshot>>,
-  cachedRepoStats: Awaited<ReturnType<typeof listContributorRepoStats>>,
-) {
-  const officialRepoStats = contributorRepoStatsFromGittensor(gittensorSnapshot);
-  return officialRepoStats.length > 0 ? officialRepoStats : cachedRepoStats;
-}
-
 async function authenticateMcpRequest(c: AppContext): Promise<AuthIdentity | null> {
   const identity = await authenticatePrivateToken(c.env, extractBearerToken(c.req.header("authorization")));
   if (!identity || identity.kind !== "session") return identity;
diff --git a/src/queue/processors.ts b/src/queue/processors.ts
index c83e6abc66..a16f5898f2 100644
--- a/src/queue/processors.ts
+++ b/src/queue/processors.ts
@@ -228,6 +228,7 @@ import {
   refreshScoringModelSnapshot,
 } from "../scoring/model";
 import {
+  authoritativeContributorRepoStats,
   buildAndPersistContributorDecisionPack,
   loadDecisionPackSharedInputs,
 } from "../services/decision-pack";
@@ -15558,17 +15559,6 @@ function officialGittensorContributorDetection(
   };
 }
 
-function authoritativeContributorRepoStats(
-  gittensorSnapshot: Awaited<
-    ReturnType<typeof fetchGittensorContributorSnapshot>
-  >,
-  cachedRepoStats: Awaited<ReturnType<typeof listContributorRepoStats>>,
-) {
-  const officialRepoStats =
-    contributorRepoStatsFromGittensor(gittensorSnapshot);
-  return officialRepoStats.length > 0 ? officialRepoStats : cachedRepoStats;
-}
-
 /** Split `owner/name` into the project/repo key shape shared by RAG indexing and retrieval. */
 export function splitRepoForRag(repoFullName: string): [string, string] {
   const slash = repoFullName.indexOf("/");
diff --git a/src/services/decision-pack.ts b/src/services/decision-pack.ts
index 27a4adb390..3bb323cffc 100644
--- a/src/services/decision-pack.ts
+++ b/src/services/decision-pack.ts
@@ -1794,7 +1794,11 @@ function snapshotAgeMs(generatedAt: string): number {
   return Number.isFinite(parsed) ? Date.now() - parsed : Number.POSITIVE_INFINITY;
 }
 
-function authoritativeContributorRepoStats(
+/** The gittensor-official snapshot's repo stats when present, falling back to the last cached copy —
+ *  gittensor is the authoritative source when reachable, the cache is only a degrade-gracefully fallback for
+ *  when it isn't. Shared by every site that resolves a contributor's repo stats (#4611) — mcp/server.ts,
+ *  api/routes.ts, and queue/processors.ts all import this rather than redefining it. */
+export function authoritativeContributorRepoStats(
   gittensorSnapshot: Awaited<ReturnType<typeof fetchGittensorContributorSnapshot>>,
   cachedRepoStats: ContributorRepoStatRecord[],
 ) {