fix(review): stop generic_secret_assignment false-flagging self-naming fixture/enum values

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

@@ -1012,12 +1012,75 @@ const RULES: Rule[] = [
     confidence: "medium",
   },
   {
+    // Captures the VALUE (group 1) so ruleMatches below can reject an obvious non-secret filler before
+    // counting this as a hit -- see isPlaceholderSecretValue.
     kind: "generic_secret_assignment",
-    re: /(?:api[_-]?key|secret|token|password|passwd|access[_-]?key|client[_-]?secret)["']?\s*[:=]\s*["'][A-Za-z0-9+/=_-]{16,}["']/i,
+    re: /(?:api[_-]?key|secret|token|password|passwd|access[_-]?key|client[_-]?secret)["']?\s*[:=]\s*["']([A-Za-z0-9+/=_-]{16,})["']/i,
     confidence: "medium",
   },
 ];
 
+const PLACEHOLDER_VALUE_PATTERN = /placeholder|change[_-]?me|your[_-]|<[^>]*>|\bexample\b|redacted|dummy|\bsample\b|\btodo\b|\bfixme\b|\binsert\b|replace[_-]?me|\bfake\b/i;
+
+// A string with NO repeated characters (e.g. "abcdefghijklmnop123") has HIGH Shannon entropy by raw
+// character-frequency counting, but is obviously not a real secret -- entropy alone only measures frequency,
+// not ORDER, so a keyboard-sequential/alphabetical run slips past a pure distinct-character-count check. Detect
+// the longest run of consecutive ascending or descending character codes and treat a long one as a
+// human-constructed test value, not a randomly generated credential -- real API keys/tokens essentially never
+// contain a 6+ character monotonic run. (Mirrors src/review/secrets-scan.ts and
+// src/review/content-lane/security-scan.ts in the main Worker repo -- REES deploys standalone so this is a
+// deliberate third copy, not a cross-package import; see this repo's own header comment for why.)
+const MIN_SEQUENTIAL_RUN_LENGTH = 6;
+function hasLongSequentialRun(value: string): boolean {
+  let ascendingRun = 1;
+  let descendingRun = 1;
+  for (let i = 1; i < value.length; i += 1) {
+    const diff = value.charCodeAt(i) - value.charCodeAt(i - 1);
+    ascendingRun = diff === 1 ? ascendingRun + 1 : 1;
+    descendingRun = diff === -1 ? descendingRun + 1 : 1;
+    if (ascendingRun >= MIN_SEQUENTIAL_RUN_LENGTH || descendingRun >= MIN_SEQUENTIAL_RUN_LENGTH) return true;
+  }
+  return false;
+}
+
+// Lowercase hyphenated mock names are fixtures; mixed-case/digit-bearing values containing "mock" remain
+// plausible credentials and must still be reported.
+const LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN = /^(?:[a-z]+-)*mock(?:-[a-z]+)*$/;
+
+// All-lowercase-letters value check, shared by the self-naming-suffix exclusion below.
+const ALL_LOWERCASE_SEGMENTS_PATTERN = /^[a-z]+(?:[-_][a-z]+)*$/;
+
+// #4579-followup (confirmed live false positives: metagraphed/gittensory#4524 "token = default-session-token"/
+// "beta-session-token", awesome-claude#4758 "embedded_secret: unsafe_install_or_secret" -- none a real secret):
+// a value whose OWN last hyphen/underscore-separated segment is itself one of the same secret-shaped trigger
+// words reads as a NAME for a concept ("this is a kind of token/secret"), not an opaque credential -- a real
+// generated token/key value never ends by literally restating what kind of thing it is. Deliberately NARROWER
+// than "any multi-segment lowercase phrase": a Diceware-style passphrase like "alpha-bravo-charlie-delta"
+// doesn't end in a trigger word, so it still correctly flags -- only values that self-identify as a
+// token/secret/key/password NAME are excluded.
+const SELF_NAMING_FIXTURE_SUFFIX_PATTERN = /[-_](?:token|secret|key|password|passwd)$/i;
+
+/** 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
+ *  lowercase identifier whose own last segment self-names as a secret kind (e.g. "default-session-token"). */
+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 (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true;
+  return hasLongSequentialRun(value);
+}
+
+/** True when `rule` actually matches `text` -- for every format-specific rule this is a plain `.test()`, but
+ *  `generic_secret_assignment` also requires its captured VALUE to clear {@link isPlaceholderSecretValue}
+ *  first, since a keyword-plus-quoted-value SHAPE also matches plenty of non-secrets (a Zod schema field, a
+ *  TypeScript type declaration, a test fixture, an enum/category label). */
+function ruleMatches(rule: Rule, text: string): boolean {
+  if (rule.kind !== "generic_secret_assignment") return rule.re.test(text);
+  const match = rule.re.exec(text);
+  return match !== null && !isPlaceholderSecretValue(match[1]!);
+}
+
 /** Extract the inner text of every quoted string literal (single/double/backtick) on a line. Used to catch a
  *  secret whose literal value is split across two adjacent added lines and joined at runtime (e.g.
  *  `const a = "AKIA..."; const b = a + "REST";`) — pure per-line regex matching never sees the runtime-joined
@@ -1054,7 +1117,7 @@ export function scanPatch(path: string, patch: string): SecretFinding[] {
       const content = line.slice(1);
       let matched = false;
       for (const rule of RULES) {
-        if (rule.re.test(content)) {
+        if (ruleMatches(rule, content)) {
           findings.push({ file: path, line: newLine, kind: rule.kind, confidence: rule.confidence });
           matched = true;
           break; // one finding per line — first (most specific) rule wins
@@ -1068,7 +1131,7 @@ export function scanPatch(path: string, patch: string): SecretFinding[] {
       if (!matched && lastPrevious !== undefined && firstCurrent !== undefined) {
         const joined = lastPrevious + firstCurrent;
         for (const rule of RULES) {
-          if (rule.re.test(joined)) {
+          if (ruleMatches(rule, joined)) {
             // "medium" regardless of the rule's own confidence — a joined pair is a heuristic, not a direct match.
             findings.push({ file: path, line: newLine, kind: rule.kind, confidence: "medium" });
             break;
@@ -1095,7 +1158,7 @@ export function scanAddedLinesForSecrets(
   const findings: SecretFinding[] = [];
   for (const line of addedLines) {
     for (const rule of RULES) {
-      if (rule.re.test(line.text)) {
+      if (ruleMatches(rule, line.text)) {
         findings.push({
           file: line.file,
           line: line.line,

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

@@ -2,7 +2,7 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
 
-import { scanPatch } from "../dist/analyzers/secret-scan.js";
+import { scanAddedLinesForSecrets, scanPatch } from "../dist/analyzers/secret-scan.js";
 
 const hunk = (lines) => `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`;
 
@@ -123,6 +123,42 @@ test("scanPatch flags a generic secret assignment", () => {
   assert.equal(findings[0].confidence, "medium");
 });
 
+// #4579-followup: confirmed live false positives (metagraphed/gittensory#4524 "token = default-session-token"/
+// "beta-session-token", awesome-claude#4758 "embedded_secret: unsafe_install_or_secret") -- none was a real
+// secret, both were test fixtures / enum-category labels whose own last segment self-names as a secret kind.
+test("scanPatch does NOT flag a self-naming multi-segment fixture value (#4579-followup)", () => {
+  const findings = scanPatch("src/config.ts", hunk([`const token = "default-session-token";`]));
+  assert.equal(
+    findings.some((f) => f.kind === "generic_secret_assignment"),
+    false,
+  );
+});
+
+test("scanPatch does NOT flag an underscore-separated self-naming enum label (#4579-followup)", () => {
+  const findings = scanPatch("src/config.ts", hunk([`const embedded_secret = "unsafe_install_or_secret";`]));
+  assert.equal(
+    findings.some((f) => f.kind === "generic_secret_assignment"),
+    false,
+  );
+});
+
+test("scanPatch still flags a generic multi-segment lowercase passphrase that does NOT self-name as a secret kind (regression guard for #4579-followup)", () => {
+  // Same shape as the excluded fixtures above (all-lowercase, hyphen-separated, no digits) but the value's
+  // own last segment is "delta", not token/secret/key/password/passwd -- a real Diceware-style passphrase
+  // must not be swept in by the new self-naming-suffix exclusion.
+  const findings = scanPatch("src/config.ts", hunk([`const token = "alpha-bravo-charlie-delta";`]));
+  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(
+    findings.some((f) => f.kind === "generic_secret_assignment"),
+    false,
+  );
+});
+
 test("scanPatch reports nothing for clean code", () => {
   const findings = scanPatch("src/app.ts", hunk(['const greeting = "hello world";', "export function run() {}"]));
   assert.equal(findings.length, 0);

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

@@ -63,12 +63,27 @@ function hasLongSequentialRun(value: string): boolean {
   return false;
 }
 
+// All-lowercase-letters value check, shared by the self-naming-suffix exclusion below.
+const ALL_LOWERCASE_SEGMENTS_PATTERN = /^[a-z]+(?:[-_][a-z]+)*$/;
+
+// #4579-followup (metagraphed/gittensory#4524 "token = default-session-token"/"beta-session-token",
+// awesome-claude#4758 "embedded_secret: unsafe_install_or_secret" -- both confirmed live, no real secret
+// present): a value whose OWN last hyphen/underscore-separated segment is itself one of the same secret-shaped
+// trigger words reads as a NAME for a concept ("this is a kind of token/secret"), not an opaque credential --
+// a real generated token/key value never ends by literally restating what kind of thing it is. Deliberately
+// NARROWER than "any multi-segment lowercase phrase": a Diceware-style passphrase like
+// "alpha-bravo-charlie-delta" doesn't end in a trigger word, so it still correctly flags -- only values that
+// self-identify as a token/secret/key/password NAME are excluded.
+const SELF_NAMING_FIXTURE_SUFFIX_PATTERN = /[-_](?:token|secret|key|password|passwd)$/i;
+
 /** True for an obvious non-secret filler value: a known placeholder phrase, a string built from at most 2
- *  distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), or a long monotonic character-code run
- *  (e.g. "abcdefghijklmnop123") — real high-entropy secrets never look like any of these. */
+ *  distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), a long monotonic character-code run
+ *  (e.g. "abcdefghijklmnop123"), or a lowercase identifier whose own last segment self-names as a secret kind
+ *  (e.g. "default-session-token", "unsafe_install_or_secret") — real high-entropy secrets never look like any of these. */
 function isPlaceholderSecretValue(value: string): boolean {
   if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true;
   if (new Set(value.toLowerCase()).size <= 2) return true;
+  if (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true;
   return hasLongSequentialRun(value);
 }
 

src/review/secrets-scan.ts

@@ -67,22 +67,32 @@ function hasLongSequentialRun(value: string): boolean {
   return false;
 }
 
-// #3041: fixture names like "installation-token" are common in this repo and should not trip the
-// generic token-assignment heuristic. Keep that carve-out key-aware and two-word-only: lowercase
-// hyphenated values assigned to password/passwd/client_secret remain plausible passphrase-style credentials.
-const LOWERCASE_HYPHENATED_TOKEN_FIXTURE_PATTERN = /^[a-z]+-[a-z]+$/;
 // Lowercase hyphenated mock names are fixtures; mixed-case/digit-bearing values containing "mock" remain
 // plausible credentials and must still be reported by the generic assignment scanner.
 const LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN = /^(?:[a-z]+-)*mock(?:-[a-z]+)*$/;
 
+// All-lowercase-letters value check, shared by the self-naming-suffix exclusion below.
+const ALL_LOWERCASE_SEGMENTS_PATTERN = /^[a-z]+(?:[-_][a-z]+)*$/;
+
+// #4579-followup (metagraphed/gittensory#4524 "token = default-session-token"/"beta-session-token",
+// awesome-claude#4758 "embedded_secret: unsafe_install_or_secret" -- both confirmed live, no real secret
+// present): a value whose OWN last hyphen/underscore-separated segment is itself one of the same secret-shaped
+// trigger words reads as a NAME for a concept ("this is a kind of token/secret"), not an opaque credential --
+// a real generated token/key value never ends by literally restating what kind of thing it is. Deliberately
+// NARROWER than "any multi-segment lowercase phrase": a Diceware-style passphrase like
+// "alpha-bravo-charlie-delta" doesn't end in a trigger word, so it still correctly flags (regression guard
+// below) -- only values that self-identify as a token/secret/key/password NAME are excluded.
+const SELF_NAMING_FIXTURE_SUFFIX_PATTERN = /[-_](?:token|secret|key|password|passwd)$/i;
+
 /** True for an obvious non-secret filler value: a known placeholder phrase, a string built from at most 2
  *  distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), a long monotonic character-code run
- *  (e.g. "abcdefghijklmnop123"), or a narrow token fixture name (e.g. "installation-token"). */
-function isPlaceholderSecretValue(key: string, value: string): boolean {
+ *  (e.g. "abcdefghijklmnop123"), or a lowercase identifier whose own last segment self-names as a secret kind
+ *  (e.g. "default-session-token", "unsafe_install_or_secret"). */
+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 (key.toLowerCase() === "token" && LOWERCASE_HYPHENATED_TOKEN_FIXTURE_PATTERN.test(value)) return true;
+  if (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true;
   return hasLongSequentialRun(value);
 }
 
@@ -94,7 +104,7 @@ function hasGenericSecretAssignment(text: string): boolean {
   while ((match = GENERIC_SECRET_ASSIGNMENT_PATTERN.exec(text)) !== null) {
     // The key and value groups are mandatory (not `?`/`*`-wrapped), so both are always present
     // whenever the overall match succeeds -- non-null by construction, not runtime branches.
-    if (!isPlaceholderSecretValue(match[1]!, match[2]!)) return true;
+    if (!isPlaceholderSecretValue(match[2]!)) return true;
   }
   return false;
 }

test/unit/content-lane-security-scan.test.ts

@@ -79,6 +79,22 @@ describe("scanForSecrets", () => {
   it("does NOT flag a schema field declaration with no literal value", () => {
     expect(scanForSecrets("password: z.string()").kinds).not.toContain("generic_secret_assignment");
   });
+
+  // #4579-followup: confirmed live false positives (awesome-claude#4758 "embedded_secret:
+  // unsafe_install_or_secret"; the same self-naming shape as metagraphed/gittensory#4524's
+  // "token: default-session-token") -- neither is a real secret, both are enum/fixture NAMES whose own last
+  // segment restates the kind of thing they are.
+  it.each([
+    ["session-token fixture", 'token: "default-session-token"'],
+    ["enum label ending in _secret", 'embedded_secret: "unsafe_install_or_secret"'],
+    ["password ending in -passwd", 'password: "legacy-system-passwd"'],
+  ])("does NOT flag a self-naming multi-segment fixture/enum value: %s (#4579-followup)", (_name, snippet) => {
+    expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment");
+  });
+
+  it("still flags a generic multi-segment lowercase passphrase that does NOT self-name as a secret kind (regression guard for #4579-followup)", () => {
+    expect(scanForSecrets('token = "alpha-bravo-charlie-delta"').kinds).toContain("generic_secret_assignment");
+  });
 });
 
 describe("scanSubmissionContent", () => {

test/unit/secrets-scan.test.ts

@@ -201,4 +201,34 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => {
     const singleWord = "qwzxvbnmalskdjfhgpoiu";
     expect(scanForSecrets(`token = "${singleWord}"`).kinds).toContain("generic_secret_assignment");
   });
+
+  // #4579-followup: confirmed live false positives (metagraphed/gittensory#4524, #4224) closed PRs for
+  // "missing before/after screenshot table"-unrelated reasons -- a leaked secret that never existed. Both
+  // values are test FIXTURES (a session-token mock) whose own last segment self-names as "token".
+  it.each([
+    ["default-session-token", 'token: "default-session-token"'],
+    ["beta-session-token", 'token: "beta-session-token"'],
+    ["three-segment self-naming secret", 'client_secret = "some-embedded-secret"'],
+  ])("does NOT flag a self-naming multi-segment fixture value: %s (#4579-followup)", (_name, snippet) => {
+    expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment");
+  });
+
+  // #4579-followup: confirmed live false positive (awesome-claude#4758) -- an enum/category LABEL, not a
+  // credential, assigned to a key that itself contains "secret" (embedded_secret).
+  it("does NOT flag an underscore-separated self-naming enum label (#4579-followup)", () => {
+    expect(scanForSecrets('embedded_secret: "unsafe_install_or_secret"').kinds).not.toContain("generic_secret_assignment");
+  });
+
+  it("still flags a generic multi-segment lowercase passphrase that does NOT self-name as a secret kind (regression guard for #4579-followup)", () => {
+    // Same shape as the excluded fixtures above (all-lowercase, hyphen-separated, no digits) but the value's
+    // own last segment is "delta", not token/secret/key/password/passwd -- a real Diceware-style passphrase
+    // must not be swept in by the new self-naming-suffix exclusion.
+    expect(scanForSecrets('token = "alpha-bravo-charlie-delta"').kinds).toContain("generic_secret_assignment");
+  });
+
+  it("still flags a self-naming-suffix-shaped value that also has mixed case or digits (the exclusion requires ALL-lowercase)", () => {
+    // Ends in "-token" like the excluded fixtures, but the earlier segment has a digit -- not a plausible
+    // human-authored fixture name, so it must still be reported.
+    expect(scanForSecrets('token = "session2024-token"').kinds).toContain("generic_secret_assignment");
+  });
 });