fix(review): stop generic_secret_assignment from auto-closing PRs

review-enrichment/src/analyzers/secret-scan.ts

@@ -1057,14 +1057,48 @@ const KNOWN_FIXTURE_SECRET_VALUES = new Set([
   "unsafe_install_or_secret",
 ]);
 
+// Closed set of grammatical FUNCTION words — articles, negations, prepositions, auxiliary verbs — chosen for
+// having near-zero information content per word. A human-authored placeholder that describes itself in prose
+// (e.g. "present-value-not-a-real-token", "test-value-should-never-appear-in-doctor-output" — both real
+// false-positive literals from gittensory PR #5346/#5341) naturally reaches for these; a deliberately
+// memorable human-CHOSEN passphrase like "correct-horse-battery-secret" is composed of high-entropy CONTENT
+// words (nouns/verbs/adjectives) specifically BECAUSE function words carry little entropy per word, so a
+// diceware-style passphrase essentially never contains one. Their presence is therefore a reliable structural
+// signal for "this is descriptive prose about the value", not "this is someone's chosen secret" — see
+// looksLikeDescriptivePlaceholderPhrase below. (Mirrors src/review/secret-patterns.ts — drift-checked, not
+// imported; see this file's header.)
+const ENGLISH_FUNCTION_WORDS = new Set([
+  "a", "an", "the", "is", "are", "was", "were", "be", "been", "not", "no", "and", "or", "to", "of",
+  "in", "on", "at", "for", "with", "should", "never", "always", "will", "would", "does", "did", "do",
+  "this", "that", "it", "as", "if", "then", "so", "but", "has", "have", "had", "can", "could",
+]);
+
+// A written-prose fixture description needs several words to say something (both PR #5346 literals split into
+// 6 and 8 segments respectively); a memorable diceware-style passphrase conventionally tops out around 4 words
+// for memorability. Requiring 5+ keeps this from colliding with a short, genuinely human-chosen passphrase.
+const MIN_DESCRIPTIVE_PHRASE_SEGMENTS = 5;
+
+/** True when `value` reads as a written sentence fragment (a fixture author's own prose description of the
+ *  value) rather than a credential or a chosen passphrase: split on `-`/`_` into 5+ segments, every segment
+ *  purely lowercase ASCII letters (a real token's mixed case/digits would fail this, correctly leaving it
+ *  flagged), with at least one segment a low-entropy English function word (see ENGLISH_FUNCTION_WORDS). */
+function looksLikeDescriptivePlaceholderPhrase(value: string): boolean {
+  const segments = value.split(/[-_]/);
+  if (segments.length < MIN_DESCRIPTIVE_PHRASE_SEGMENTS) return false;
+  if (!segments.every((segment) => /^[a-z]+$/.test(segment))) return false;
+  return segments.some((segment) => ENGLISH_FUNCTION_WORDS.has(segment));
+}
+
 /** True for an obvious non-secret filler value: a known placeholder phrase, a string built from at most 2
- *  distinct characters, a long monotonic character-code run, a lowercase hyphenated mock fixture name, or a
- *  known fixture/enum literal. */
+ *  distinct characters, a long monotonic character-code run, a lowercase hyphenated mock fixture name, a
+ *  known fixture/enum literal, or a descriptive multi-word prose phrase (see
+ *  looksLikeDescriptivePlaceholderPhrase). */
 function isPlaceholderSecretValue(value: string): boolean {
   if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true;
   if (new Set(value.toLowerCase()).size <= 2) return true;
   if (LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN.test(value)) return true;
   if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true;
+  if (looksLikeDescriptivePlaceholderPhrase(value)) return true;
   return hasLongSequentialRun(value);
 }
 

review-enrichment/test/secret-scan.test.ts

@@ -154,6 +154,33 @@ test("scanPatch still flags a generic multi-segment lowercase passphrase that do
   assert.equal(findings[0].kind, "generic_secret_assignment");
 });
 
+// gittensory PR #5346/#5341: two inert test-fixture strings ("present-value-not-a-real-token",
+// "test-value-should-never-appear-in-doctor-output") matched the generic_secret_assignment SHAPE but neither
+// contained a PLACEHOLDER_VALUE_PATTERN keyword, so both auto-closed a legitimate contributor PR twice in a
+// row. looksLikeDescriptivePlaceholderPhrase (mirrors src/review/secret-patterns.ts) recognizes both as
+// written-prose fixture descriptions instead.
+test("scanPatch does NOT flag the two real false-positive fixture values from gittensory PR #5346/#5341 (regression)", () => {
+  const first = scanPatch("src/config.ts", hunk([`const token = "present-value-not-a-real-token";`]));
+  assert.equal(
+    first.some((f) => f.kind === "generic_secret_assignment"),
+    false,
+  );
+  const second = scanPatch(
+    "src/config.ts",
+    hunk([`const secretToken = "test-value-should-never-appear-in-doctor-output";`]),
+  );
+  assert.equal(
+    second.some((f) => f.kind === "generic_secret_assignment"),
+    false,
+  );
+});
+
+test("scanPatch still flags a 5+ all-lowercase-word passphrase with no function word (a genuine diceware-style secret)", () => {
+  const findings = scanPatch("src/config.ts", hunk([`const secret = "correct-horse-battery-staple-secret";`]));
+  assert.equal(findings.length, 1);
+  assert.equal(findings[0].kind, "generic_secret_assignment");
+});
+
 test("scanAddedLinesForSecrets applies the same self-naming-fixture exclusion as scanPatch (#4579-followup)", () => {
   const findings = scanAddedLinesForSecrets([{ file: "src/config.ts", line: 1, text: `const token = "default-session-token";` }]);
   assert.equal(

src/review/content-lane/security-scan.ts

@@ -67,24 +67,34 @@ function firstLineMatching(text: string, re: RegExp): { n: number; text: string
 }
 
 function firstSecretLine(text: string): { n: number; kinds: string[] } | null {
-  // Per-line scan first — O(n): catches every LINE-CONTAINED hard kind (github_token, jwt, …) and cites its
+  // Per-line scan — O(n): catches every LINE-CONTAINED concrete kind (github_token, jwt, …) and cites its
   // exact line. lines[i] is defined for an in-range index (split never yields holes) — assert past
-  // noUncheckedIndexedAccess.
+  // noUncheckedIndexedAccess. HARD_SECRET_KINDS no longer includes generic_secret_assignment (see
+  // ../secret-patterns.ts's doc comment) — that kind is handled separately by
+  // firstGenericSecretAssignmentLine below and routes to MANUAL, not this function's auto-close caller.
   const lines = text.split(/\r?\n/);
   for (let i = 0; i < lines.length; i += 1) {
     const hits = scanForSecrets(lines[i]!).kinds.filter((k) => HARD_SECRET_KINDS.has(k));
     if (hits.length) return { n: i + 1, kinds: hits };
   }
-  // The only HARD kind whose keyword-to-value span can WRAP across lines is generic_secret_assignment, so the
-  // per-line scan above can miss it (`client_secret =\n"…"`). Recover it with ONE whole-blob pass over just that
-  // detector — LINEAR, not the quadratic prefix-rescan — citing the line where the non-placeholder match
-  // COMPLETES, so scanSubmissionContent's auto-close stays in parity with the whole-blob PR-diff gate.
+  return null;
+}
+
+/**
+ * generic_secret_assignment is a keyword-plus-quoted-value SHAPE heuristic, not a concrete credential format
+ * (see ../secret-patterns.ts's HARD_SECRET_KINDS doc comment — split out post-gittensory-PR-#5346, which
+ * auto-closed a legitimate contributor PR over two inert test-fixture strings). Per this file's own header
+ * ("only ONE signal is unambiguous enough to hard-close... every other heuristic routes to MANUAL"), a hit
+ * here routes to MANUAL, never scanSubmissionContent's auto-close. Its keyword-to-value span can wrap across
+ * lines (`client_secret =\n"…"`), so this is a single whole-blob pass — LINEAR, not a quadratic prefix-rescan
+ * — citing the line where the non-placeholder match COMPLETES.
+ */
+function firstGenericSecretAssignmentLine(text: string): number | null {
   GENERIC_SECRET_ASSIGNMENT_PATTERN.lastIndex = 0;
   let match: RegExpExecArray | null;
   while ((match = GENERIC_SECRET_ASSIGNMENT_PATTERN.exec(text)) !== null) {
     if (!isPlaceholderSecretValue(match[1]!)) {
-      const n = text.slice(0, match.index + match[0].length).split(/\r?\n/).length;
-      return { n, kinds: ["generic_secret_assignment"] };
+      return text.slice(0, match.index + match[0].length).split(/\r?\n/).length;
     }
   }
   return null;
@@ -93,6 +103,8 @@ function firstSecretLine(text: string): { n: number; kinds: string[] } | null {
 /**
  * Deterministic security scan of the SUBMITTED content. Returns:
  *  - `close` (embedded_secret) on a concrete embedded credential — cited to a line; or
+ *  - `manual` (possible_secret_assignment) on a secret-shaped-but-not-concrete-format assignment — cited to
+ *    a line (see firstGenericSecretAssignmentLine's doc comment for why this is MANUAL, not close); or
  *  - `manual` (unsafe_install_pipeline) on a pipe-to-shell install in an executable category; or
  *  - null otherwise.
  * Prompt-injection / exfiltration prose is intentionally NOT matched here: it is indistinguishable
@@ -111,6 +123,15 @@ export function scanSubmissionContent(params: { content: string; category: strin
     };
   }
 
+  const genericLine = firstGenericSecretAssignmentLine(content);
+  if (genericLine !== null) {
+    return {
+      verdict: "manual",
+      reasonCode: "possible_secret_assignment",
+      summary: `Submission contains a secret-shaped assignment (generic_secret_assignment) at line ${genericLine} that doesn't match a concrete credential format — routing to maintainer review to verify it isn't a real secret.`,
+    };
+  }
+
   if (EXECUTABLE_CATEGORIES.has(category)) {
     const pipe = firstLineMatching(content, PIPED_INSTALL_RE);
     if (pipe) {
@@ -124,8 +145,9 @@ export function scanSubmissionContent(params: { content: string; category: strin
   return null;
 }
 
-/** A concrete credential exposed in a LINKED third-party body → manual (flag for a human; don't
- *  auto-close someone's submission over the linked artifact's own leak). */
+/** A concrete credential — or a secret-shaped-but-not-concrete-format assignment — exposed in a LINKED
+ *  third-party body → manual either way (flag for a human; don't auto-close someone's submission over the
+ *  linked artifact's own leak). */
 export function scanLinkedBodiesForSecrets(bodies: string[]): SecurityFinding | null {
   for (const body of bodies) {
     const hits = scanForSecrets(body).kinds.filter((k) => HARD_SECRET_KINDS.has(k));
@@ -136,6 +158,13 @@ export function scanLinkedBodiesForSecrets(bodies: string[]): SecurityFinding |
         summary: `The linked source appears to expose a credential (${hits.join(", ")}) — routing to maintainer review.`,
       };
     }
+    if (hasGenericSecretAssignment(body)) {
+      return {
+        verdict: "manual",
+        reasonCode: "possible_secret_assignment",
+        summary: "The linked source contains a secret-shaped assignment (generic_secret_assignment) that doesn't match a concrete credential format — routing to maintainer review.",
+      };
+    }
   }
   return null;
 }

src/review/safety.ts

@@ -7,8 +7,8 @@
 
 import type { AdvisoryFinding } from "../types";
 import { neutralizePromptInjection, safeReviewTitle } from "./prompt-injection";
-import { HARD_SECRET_KINDS } from "./secret-patterns";
-import { scanDiffForSecretsWithLocations } from "./secrets-scan";
+import { ADVISORY_ONLY_SECRET_KINDS, HARD_SECRET_KINDS } from "./secret-patterns";
+import { scanDiffForSecretsWithLocations, type SecretScanLocationMatch } from "./secrets-scan";
 
 // Concrete credential formats only — NOT the weak heuristics (`seed_or_mnemonic` / `bittensor_key`) that
 // false-positive on legitimate config/workflow content. A `coldkey:` / `hotkey =` line or the word
@@ -78,47 +78,70 @@ export function defangReviewInput(input: SafetyReviewInput): {
 // produces a readable comment; anything past the cap is summarized as an omitted count instead of listed.
 const MAX_REPORTED_SECRET_LOCATIONS = 5;
 
+function locationSummaryFor(hits: SecretScanLocationMatch[]): string {
+  const locations = hits.map((hit) =>
+    hit.line === 0 ? `${hit.path} (filename)` : `${hit.path}:${hit.line}`,
+  );
+  const uniqueLocations = [...new Set(locations)];
+  const shown = uniqueLocations.slice(0, MAX_REPORTED_SECRET_LOCATIONS);
+  const omitted = uniqueLocations.length - shown.length;
+  return omitted > 0
+    ? `Found at: ${shown.join(", ")} (+${omitted} more location${omitted === 1 ? "" : "s"})`
+    : `Found at: ${shown.join(", ")}`;
+}
+
 /**
- * Scan the PR diff for leaked secrets and, on a hit, return ONE critical `secret_leak` advisory finding (else
- * null). Mapped to gittensory's {@link AdvisoryFinding} shape. The gate treats this code as a hard blocker
- * (see rules/advisory.ts) so a leaked secret holds the PR. Only CONCRETE credential formats
- * ({@link HARD_SECRET_KINDS}) qualify — the weak `seed_or_mnemonic` / `bittensor_key` heuristics are ignored
- * here because they false-positive on legitimate config/workflow content (e.g. `coldkey:` / `hotkey =` lines
- * in *.toml, .github/workflows/**, or wrangler/workers config). This is UNCONDITIONAL (#audit-3.4): a concrete,
- * real-format committed credential is a leak on any repo, so the caller runs it regardless of the safety flag /
- * review allowlist (unlike the prompt-injection defang, which stays flag-gated).
+ * Scan the PR diff for leaked secrets and, on a hit, return ONE `AdvisoryFinding` (else null). Mapped to
+ * gittensory's {@link AdvisoryFinding} shape.
+ *
+ * Only CONCRETE credential formats ({@link HARD_SECRET_KINDS}) produce the critical `secret_leak` code that
+ * `rules/advisory.ts`'s `isConfiguredGateBlocker` treats as an unconditional hard blocker — the weak
+ * `seed_or_mnemonic` / `bittensor_key` heuristics are ignored entirely here because they false-positive on
+ * legitimate config/workflow content (e.g. `coldkey:` / `hotkey =` lines in *.toml, .github/workflows/**, or
+ * wrangler/workers config). This is UNCONDITIONAL (#audit-3.4): a concrete, real-format committed credential
+ * is a leak on any repo, so the caller runs it regardless of the safety flag / review allowlist (unlike the
+ * prompt-injection defang, which stays flag-gated).
+ *
+ * `ADVISORY_ONLY_SECRET_KINDS` (currently just `generic_secret_assignment`) is a keyword-plus-quoted-value
+ * SHAPE heuristic, not a concrete format — see `secret-patterns.ts`'s `HARD_SECRET_KINDS` doc comment for why
+ * it was split out (PR #5346 auto-closed a legitimate contributor PR on two inert test-fixture strings). A
+ * hit on ONLY this kind (no concrete kind present) instead returns a warning-severity `possible_secret_
+ * assignment` finding, which `isConfiguredGateBlocker` does not recognize as a blocker code — it surfaces in
+ * the PR panel for a human/AI reviewer to verify, exactly as REES's own "medium confidence" rating for this
+ * same signal already treats it, without risking another auto-close false positive.
  *
  * #3041: scans the RAW diff directly — `scanDiffForSecretsWithLocations` does its own +/- line-type
  * distinguishing (only added lines and added/renamed file paths are scanned, matching the previous
  * added-only-text behavior) while also tracking each hit's file:line, so the finding can point a maintainer
  * straight at the flagged content instead of forcing them to re-derive it from the whole diff.
  */
 export function secretLeakFinding(diff: string): AdvisoryFinding | null {
+  const allHits = scanDiffForSecretsWithLocations(diff);
   // Only CONCRETE credential formats hard-block. The raw scanner also returns the weak `seed_or_mnemonic` /
   // `bittensor_key` heuristics, which false-positive on `coldkey:` / `hotkey =` / "mnemonic" lines in
-  // legitimate config/workflow files (RC6); those are filtered out here so they never produce a `secret_leak`
-  // blocker. A real token (github_token, aws_access_key, …) still blocks regardless of which file it is in.
-  const hits = scanDiffForSecretsWithLocations(diff).filter((match) =>
-    HARD_SECRET_KINDS.has(match.kind),
-  );
-  if (hits.length === 0) return null;
-  const kinds = [...new Set(hits.map((hit) => hit.kind))].sort();
-  const locations = hits.map((hit) =>
-    hit.line === 0 ? `${hit.path} (filename)` : `${hit.path}:${hit.line}`,
-  );
-  const uniqueLocations = [...new Set(locations)];
-  const shown = uniqueLocations.slice(0, MAX_REPORTED_SECRET_LOCATIONS);
-  const omitted = uniqueLocations.length - shown.length;
-  const locationSummary =
-    omitted > 0
-      ? `Found at: ${shown.join(", ")} (+${omitted} more location${omitted === 1 ? "" : "s"})`
-      : `Found at: ${shown.join(", ")}`;
-  return {
-    code: "secret_leak",
-    severity: "critical",
-    title: `Possible leaked secret in the diff (${kinds.join(", ")})`,
-    detail: `The PR diff matches secret pattern(s): ${kinds.join(", ")}. ${locationSummary}. A committed credential must be rotated and removed from the change before merge.`,
-    action:
-      "Remove the secret from the diff, rotate the exposed credential, then re-run the gate.",
-  };
+  // legitimate config/workflow files (RC6); those are filtered out here so they never produce a finding at
+  // all. A real token (github_token, aws_access_key, …) still blocks regardless of which file it is in.
+  const concreteHits = allHits.filter((match) => HARD_SECRET_KINDS.has(match.kind));
+  if (concreteHits.length > 0) {
+    const kinds = [...new Set(concreteHits.map((hit) => hit.kind))].sort();
+    return {
+      code: "secret_leak",
+      severity: "critical",
+      title: `Possible leaked secret in the diff (${kinds.join(", ")})`,
+      detail: `The PR diff matches secret pattern(s): ${kinds.join(", ")}. ${locationSummaryFor(concreteHits)}. A committed credential must be rotated and removed from the change before merge.`,
+      action:
+        "Remove the secret from the diff, rotate the exposed credential, then re-run the gate.",
+    };
+  }
+  const advisoryHits = allHits.filter((match) => ADVISORY_ONLY_SECRET_KINDS.has(match.kind));
+  if (advisoryHits.length > 0) {
+    return {
+      code: "possible_secret_assignment",
+      severity: "warning",
+      title: "Possible secret-shaped assignment in the diff (generic_secret_assignment)",
+      detail: `The PR diff contains a keyword-plus-quoted-value assignment that resembles a credential but doesn't match a concrete credential format. ${locationSummaryFor(advisoryHits)}. This is a medium-confidence heuristic (it also matches inert test/fixture values) and does not block the gate on its own.`,
+      action: "Verify the value is not a real credential.",
+    };
+  }
+  return null;
 }