fix(review): tighten generic secret fixture allowlist

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

@@ -1047,27 +1047,24 @@ function hasLongSequentialRun(value: string): boolean {
 // 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;
+// #4579-followup: these exact live false-positive literals are fixture/enum names, not credentials. Keep
+// this allowlist intentionally closed: a broad suffix rule would suppress plausible human-chosen secrets such
+// as `client_secret = "correct-horse-battery-secret"`.
+const KNOWN_FIXTURE_SECRET_VALUES = new Set([
+  "installation-token",
+  "default-session-token",
+  "beta-session-token",
+  "unsafe_install_or_secret",
+]);
 
 /** 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"). */
+ *  known fixture/enum literal. */
 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;
+  if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true;
   return hasLongSequentialRun(value);
 }
 

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

@@ -142,10 +142,13 @@ test("scanPatch does NOT flag an underscore-separated self-naming enum label (#4
   );
 });
 
+test("scanPatch still flags lowercase segmented credentials even when their suffix names a secret kind (#4579-followup regression)", () => {
+  const findings = scanPatch("src/config.ts", hunk([`const client_secret = "correct-horse-battery-secret";`]));
+  assert.equal(findings.length, 1);
+  assert.equal(findings[0].kind, "generic_secret_assignment");
+});
+
 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");

scripts/check-engine-parity.ts

@@ -113,7 +113,7 @@ export const SECRET_DETECTION_MARKERS = Object.freeze([
   "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;",
+  "if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true;",
   "return hasLongSequentialRun(value);",
   '"github_token"',
   '"github_pat"',

src/review/secret-patterns.ts

@@ -72,29 +72,25 @@ export function hasLongSequentialRun(value: string): boolean {
 // 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;
+// #4579-followup: these exact live false-positive literals are fixture/enum names, not credentials. Keep
+// this allowlist intentionally closed: a broad suffix rule would suppress plausible human-chosen secrets such
+// as `client_secret = "correct-horse-battery-secret"`.
+const KNOWN_FIXTURE_SECRET_VALUES = new Set([
+  "installation-token",
+  "default-session-token",
+  "beta-session-token",
+  "unsafe_install_or_secret",
+]);
 
 /** 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 lowercase identifier whose own last segment self-names as a secret kind
- *  (e.g. "default-session-token", "unsafe_install_or_secret"). Mirrored (drift-checked, not imported) in
+ *  (e.g. "abcdefghijklmnop123"), or a known fixture/enum literal. Mirrored (drift-checked, not imported) in
  *  review-enrichment/src/analyzers/secret-scan.ts — see this file's header. */
 export 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;
+  if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true;
   return hasLongSequentialRun(value);
 }
 

test/unit/check-engine-parity-script.test.ts

@@ -369,7 +369,7 @@ describe("check-engine-parity script", () => {
         "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;",
+        "if (KNOWN_FIXTURE_SECRET_VALUES.has(value)) return true;",
         "return hasLongSequentialRun(value);",
       ].join("\n");
       const hostKinds = SECRET_DETECTION_MARKERS.filter((marker) => marker.startsWith('"')).join("\n");

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

@@ -120,19 +120,22 @@ describe("scanForSecrets", () => {
   });
 
   // #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.
+  // unsafe_install_or_secret"; the same known fixture shape as metagraphed/gittensory#4524's
+  // "token: default-session-token") -- these exact literals are enum/fixture names, not credentials.
   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) => {
+  ])("does NOT flag a known 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");
+  it.each([
+    ["generic passphrase", 'token = "alpha-bravo-charlie-delta"'],
+    ["client secret ending in -secret", 'client_secret = "correct-horse-battery-secret"'],
+    ["password ending in -passwd", 'password: "legacy-system-passwd"'],
+    ["api key ending in -key", 'api_key = "internal-service-key"'],
+  ])("still flags a lowercase segmented credential: %s (regression guard for #4579-followup)", (_name, snippet) => {
+    expect(scanForSecrets(snippet).kinds).toContain("generic_secret_assignment");
   });
 });
 

test/unit/secret-patterns.test.ts

@@ -83,13 +83,21 @@ describe("secret-patterns — shared secret-detection primitives (#4608)", () =>
       expect(isPlaceholderSecretValue("mock-aK9xQ2mZw7Ln4Rv8Pt3Bh6")).toBe(false);
     });
 
-    it("flags a lowercase identifier whose own last segment self-names as a secret kind", () => {
+    it("flags every known fixture/enum literal in the closed allowlist", () => {
+      expect(isPlaceholderSecretValue("installation-token")).toBe(true);
       expect(isPlaceholderSecretValue("default-session-token")).toBe(true);
+      expect(isPlaceholderSecretValue("beta-session-token")).toBe(true);
       expect(isPlaceholderSecretValue("unsafe_install_or_secret")).toBe(true);
     });
 
-    it("does NOT flag a self-naming-suffix-shaped value once digits/mixed case break the ALL-lowercase check", () => {
+    it("does NOT flag a token/secret/key/password-suffixed value that isn't in the closed fixture set (#4579-followup regression)", () => {
+      // Same self-naming SHAPE as the known fixtures above (ends in "-token"/"-secret"/"-key"/"-passwd"),
+      // but none of these exact literals are in KNOWN_FIXTURE_SECRET_VALUES, so a real human-chosen secret
+      // is no longer swept in just because its suffix happens to restate what kind of thing it is.
       expect(isPlaceholderSecretValue("session2024-token")).toBe(false);
+      expect(isPlaceholderSecretValue("correct-horse-battery-secret")).toBe(false);
+      expect(isPlaceholderSecretValue("legacy-system-passwd")).toBe(false);
+      expect(isPlaceholderSecretValue("internal-service-key")).toBe(false);
     });
 
     it("does NOT flag a multi-segment lowercase passphrase that does not self-name as a secret kind", () => {

test/unit/secrets-scan.test.ts

@@ -203,13 +203,12 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => {
   });
 
   // #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".
+  // "missing before/after screenshot table"-unrelated reasons -- a leaked secret that never existed. These
+  // exact session-token literals are known test fixtures.
   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) => {
+  ])("does NOT flag a known multi-segment fixture value: %s (#4579-followup)", (_name, snippet) => {
     expect(scanForSecrets(snippet).kinds).not.toContain("generic_secret_assignment");
   });
 
@@ -226,9 +225,12 @@ describe("scanForSecrets — deterministic secret-pattern scanner", () => {
     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");
+  it.each([
+    ["client_secret", 'client_secret = "correct-horse-battery-secret"'],
+    ["password", 'password = "legacy-system-passwd"'],
+    ["api_key", 'api_key = "internal-service-key"'],
+    ["token with digit", 'token = "session2024-token"'],
+  ])("still flags a self-naming-suffix-shaped credential assigned to %s", (_name, snippet) => {
+    expect(scanForSecrets(snippet).kinds).toContain("generic_secret_assignment");
   });
 });