fix(coding-agent): reject unresolvable regex fallback and sanitize friendly-name collision check

Two Codex P2 findings on commit 732f725:

- A default (no custom replacement) mode: "replace" regex that cannot
  escape a 1-2 char match (e.g. ".", "[\\s\\S]", "[\\s\\S]{2}") had its
  key-derived same-length fallback marker returned without checking it
  against the matched value. Since that marker is drawn from an alphabet
  the regex has already proven to match exhaustively, a real 1-2 byte
  secret coinciding with it would ship unredacted. Such entries are now
  rejected: dropped with a warning when loaded from secrets.yml, dropped
  silently as a construction-time backstop otherwise.
- #friendlyNameCollidesWithSecret compared the sanitized (uppercased,
  alnum-only) friendly name against each secret's raw value, so a
  friendlyName that was a lowercase or punctuated variant of its own
  secret slipped through and stamped most of the secret into the
  placeholder. The secret value is now sanitized the same way before
  comparing.

packages/coding-agent/CHANGELOG.md

@@ -27,6 +27,8 @@
 - Fixed a configured secret's `friendlyName` being able to bake another live secret's literal value straight into every placeholder minted for it (e.g. `{ content: "ABCDEFGH", friendlyName: "LEAKTOKEN" }` alongside a secret covering `LEAKTOKEN`), which then read as an already-generated placeholder on an exact match and was never scanned. `obfuscate()` now drops the friendly-name prefix for a given secret whenever the sanitized name contains a configured plain secret's literal or is matched by a configured regex, independent of `entries[]` order (regex patterns are now compiled before any placeholder is minted).
 - Fixed a default (no custom `replacement`) `mode: "replace"` regex with no same-length candidate able to escape it (a pathological match-everything config such as `.`/`[\s\S]`) emitting a 1–2 byte matched value unchanged when it was exactly `Z` or `ZZ`: the fallback reused `#generateReplacement`'s shared `Z`/`ZZ` sentinel for such short values, and a raw match of exactly that sentinel round-tripped to the same bytes, reaching the provider unredacted (plain `mode: "replace"` secrets already avoided this via `ensureDistinctReplacement`, but the regex fallback did not). The fallback now uses a same-length, key-derived run instead of the sentinel for <=2 char values — still a fixed point under re-obfuscation (depends only on the per-install key and the value's length, never its content, so re-matching and re-redacting it reproduces the identical marker), but no longer a public, install-independent constant a regex config could be tuned to bypass.
 - Fixed `getSecretPlaceholderKey()`/`getExistingSecretPlaceholderKey()` defaulting their `keyDir` parameter to `getConfigRootDir()` (`~/.omp`) while `createAgentSession()` always passes the profile-scoped `agentDir` (`~/.omp/agent`, per `docs/secrets.md`) explicitly. A caller relying on the default read or minted a key file at a different path than live sessions use, so placeholders it created were never stable against SDK sessions. Both helpers now default to `getAgentDir()`.
+- Fixed a default (no custom `replacement`) `mode: "replace"` regex that cannot escape a 1–2 byte match (e.g. `.`, `[\s\S]`, `[\s\S]{2}`) still risking an unredacted round-trip: the previous key-derived same-length fallback marker was returned without checking it against the matched value, and since that marker is drawn from an alphabet the regex has already proven to match exhaustively, a real 1–2 byte secret that happened to equal it would ship to the provider unchanged. Such regex entries are now rejected outright — dropped with a warning when loaded from `secrets.yml`, dropped silently as a construction-time backstop otherwise — since no same-length marker can be guaranteed distinct from every possible match once the regex is proven to match every candidate in that alphabet.
+- Fixed a plain secret's `friendlyName` being accepted as a placeholder prefix when it was a lowercase or punctuated variant of the secret's own value (e.g. `friendlyName: "github_pat_abc123"` for a secret of the same content): the collision check compared the already-sanitized (uppercased, alphanumeric-only) friendly name against the secret's raw, case-sensitive value, so a case/punctuation variant slipped through and stamped most of the secret into every placeholder (`#GITHUBPATABC123_…#`). The secret value is now sanitized the same way before the comparison.
 
 ## [16.3.8] - 2026-07-05
 

packages/coding-agent/src/secrets/index.ts

@@ -3,7 +3,7 @@ import * as fs from "node:fs/promises";
 import * as path from "node:path";
 import { getAgentDir, isEnoent, logger } from "@oh-my-pi/pi-utils";
 import { YAML } from "bun";
-import { type SecretEntry, sanitizeSecretFriendlyName } from "./obfuscator";
+import { regexHasUnresolvableShortMatchFallback, type SecretEntry, sanitizeSecretFriendlyName } from "./obfuscator";
 import { compileSecretRegex } from "./regex";
 
 const PLACEHOLDER_KEY_RE = /^[A-Za-z0-9_-]{43}$/;
@@ -216,8 +216,9 @@ function validateEntry(entry: unknown, filePath: string, index: number): entry i
 		return false;
 	}
 	if (e.type === "regex") {
+		let regex: RegExp;
 		try {
-			compileSecretRegex(e.content as string, e.flags as string | undefined);
+			regex = compileSecretRegex(e.content as string, e.flags as string | undefined);
 		} catch (error) {
 			logger.warn(`secrets.yml[${index}]: invalid regex pattern`, {
 				path: filePath,
@@ -226,6 +227,14 @@ function validateEntry(entry: unknown, filePath: string, index: number): entry i
 			});
 			return false;
 		}
+		const mode = (e.mode as "obfuscate" | "replace" | undefined) ?? "obfuscate";
+		if (mode === "replace" && e.replacement === undefined && regexHasUnresolvableShortMatchFallback(regex)) {
+			logger.warn(
+				`secrets.yml[${index}]: regex matches every 1-2 character candidate with no custom replacement, so a match can never be redacted distinctly from itself`,
+				{ path: filePath, pattern: e.content },
+			);
+			return false;
+		}
 	}
 	return true;
 }

packages/coding-agent/src/secrets/obfuscator.ts

@@ -206,6 +206,33 @@ function findWhitespaceFallbackReplacement(
 	return undefined;
 }
 
+/**
+ * Whether a default (no custom `replacement`) replace-mode regex can never
+ * safely redact a 1-2 char match: `findNonMatchingReplacement`'s bounded
+ * search — the same search `#generateRegexReplacement` runs at match time —
+ * finds no candidate the regex fails to re-match. This holds independent of
+ * any actual per-install key: the search already exhausts every character in
+ * `REPLACEMENT_CHARS` (the alphabet `buildKeyedReplacementRun` draws its
+ * fallback marker from) plus punctuation and whitespace, so if none of those
+ * escape the regex, no key-derived marker drawn from the same alphabet can
+ * either — the marker is guaranteed to re-match too, making every such match
+ * unresolvable: the fallback could only ever emit the raw matched text
+ * unchanged. Probed with a value (`"\0".repeat(length)`) the bounded search
+ * never treats as a real candidate, so the result depends only on the
+ * regex's own matching behavior, not on this specific probe.
+ */
+export function regexHasUnresolvableShortMatchFallback(regex: RegExp): boolean {
+	return ([1, 2] as const).some(length => {
+		const probe = "\u0000".repeat(length);
+		const savedLastIndex = regex.lastIndex;
+		try {
+			return findNonMatchingReplacement(probe, regex, { text: probe, start: 0, end: length }) === undefined;
+		} finally {
+			regex.lastIndex = savedLastIndex;
+		}
+	});
+}
+
 // ═══════════════════════════════════════════════════════════════════════════
 // Placeholder format
 // ═══════════════════════════════════════════════════════════════════════════
@@ -586,9 +613,24 @@ export class SecretObfuscator {
 				continue;
 			}
 			try {
+				const regex = compileSecretRegex(entry.content, entry.flags);
+				const mode = entry.mode ?? "obfuscate";
+				// A default (no custom `replacement`) replace-mode regex that can
+				// never redact a 1-2 char match distinctly from itself (see
+				// `regexHasUnresolvableShortMatchFallback`) is dropped rather than
+				// risk a real secret round-tripping unredacted; `secrets/index.ts`
+				// warns loudly for the `secrets.yml`-loaded path — this is the
+				// silent backstop for direct construction.
+				if (
+					mode === "replace" &&
+					entry.replacement === undefined &&
+					regexHasUnresolvableShortMatchFallback(regex)
+				) {
+					continue;
+				}
 				this.#regexEntries.push({
-					regex: compileSecretRegex(entry.content, entry.flags),
-					mode: entry.mode ?? "obfuscate",
+					regex,
+					mode,
 					replacement: entry.replacement,
 					friendlyName: entry.friendlyName,
 				});
@@ -938,6 +980,16 @@ export class SecretObfuscator {
 	 * than a universal constant — the same class of accepted risk
 	 * `generateDeterministicReplacement`'s hash collision already carries for longer
 	 * values.
+	 * A regex that is UNCONDITIONALLY pathological for a length <= 2 — matching
+	 * literally every candidate in isolation, independent of context, like `.`
+	 * or `[\s\S]` — is now rejected at construction/config-load time instead
+	 * (see `regexHasUnresolvableShortMatchFallback`), since for that narrow
+	 * case the residual risk above is fully avoidable rather than merely
+	 * unlikely. This branch, and the residual risk above, still applies to a
+	 * length > 2 pathological config and to a length <= 2 pattern that is only
+	 * pathological in a SPECIFIC match's surrounding context (the
+	 * construction-time check tests the regex in isolation, not every context
+	 * it could appear in).
 	 */
 	#generateRegexReplacement(value: string, regex: RegExp, context: RegexMatchContext): string {
 		let replacement = generateDeterministicReplacement(value);
@@ -1106,13 +1158,17 @@ export class SecretObfuscator {
 	// verbatim, model-visible prefix on every placeholder minted for THIS
 	// secret, baked in via an exact `#deobfuscateMap` entry rather than the
 	// alias fallback — so it needs its own check independent of the scan-skip
-	// alias guard in `#isGeneratedPlaceholder`. Reject when the label contains
-	// a configured plain secret's literal value, or when any configured regex
-	// pattern matches the label — either means that text is meant to be
-	// redacted, not stamped unredacted onto every use of this secret.
+	// alias guard in `#isGeneratedPlaceholder`. Reject when the label contains a
+	// sanitized (alnum-only, uppercased) form of a configured plain secret's
+	// value — the same normalization already applied to the label itself, so a
+	// lowercase or punctuated secret cannot smuggle itself through case or
+	// separator noise — or when any configured regex pattern matches the label
+	// — either means that text is meant to be redacted, not stamped unredacted
+	// onto every use of this secret.
 	#friendlyNameCollidesWithSecret(name: string): boolean {
 		for (const secretValue of this.#configuredSecretValues) {
-			if (secretValue.length > 0 && name.includes(secretValue)) return true;
+			const sanitizedSecret = secretValue.replace(/[^A-Za-z0-9]/g, "").toUpperCase();
+			if (sanitizedSecret.length > 0 && name.includes(sanitizedSecret)) return true;
 		}
 		for (const entry of this.#regexEntries) {
 			entry.regex.lastIndex = 0;

packages/coding-agent/test/secrets-obfuscator.test.ts

@@ -317,6 +317,36 @@ describe("SecretObfuscator friendlyName placeholders", () => {
 		expect(obfuscator.deobfuscate(obfuscated)).toBe(input);
 	});
 
+	it("rejects a friendlyName that is a case/punctuation variant of its own secret, but still applies unrelated friendly names", () => {
+		// `#friendlyNameCollidesWithSecret` used to compare the sanitized (uppercased,
+		// alnum-only) friendly name against each secret's RAW value, so a friendlyName
+		// that was merely a case/punctuation variant of its own secret (e.g.
+		// "GitHub_Pat_Abc123" labeling "github_pat_abc123") slipped through: sanitizing
+		// the label produced "GITHUBPATABC123", which never literally appears inside the
+		// lowercase, underscored raw secret string. The fix sanitizes the secret value
+		// the same way before comparing, so this exact-content-under-normalization case
+		// is now caught and the secret falls back to a bare placeholder — while a
+		// genuinely unrelated friendly name is untouched and still gets its prefix.
+		const collidingSecret = "github_pat_abc123";
+		const collidingObfuscator = new SecretObfuscator([
+			{ type: "plain", content: collidingSecret, friendlyName: "GitHub_Pat_Abc123" },
+		]);
+		const collidingObfuscated = collidingObfuscator.obfuscate(collidingSecret);
+
+		expect(collidingObfuscated).not.toMatch(/GITHUBPATABC123_/);
+		expect(collidingObfuscated).toMatch(/^#[A-Z0-9]+:L#$/);
+		expect(collidingObfuscator.deobfuscate(collidingObfuscated)).toBe(collidingSecret);
+
+		const distinctSecret = "github_pat_xyz789";
+		const distinctObfuscator = new SecretObfuscator([
+			{ type: "plain", content: distinctSecret, friendlyName: "GitHub Token" },
+		]);
+		const distinctObfuscated = distinctObfuscator.obfuscate(distinctSecret);
+
+		expect(distinctObfuscated).toMatch(/^#GITHUBTOKEN_[A-Z0-9]+:L#$/);
+		expect(distinctObfuscator.deobfuscate(distinctObfuscated)).toBe(distinctSecret);
+	});
+
 	it("uses regex entry friendly names for discovered matches", () => {
 		const secret = "tok_abc123";
 		const obfuscator = new SecretObfuscator([{ type: "regex", content: "tok_[a-z0-9]+", friendlyName: "API Key" }]);
@@ -1413,36 +1443,26 @@ describe("SecretObfuscator friendlyName placeholders", () => {
 		expect(obf.obfuscate(out)).toBe(out);
 	});
 
-	it("never emits a <=2 char match unchanged when no same-length value avoids the regex", () => {
-		// A match-everything regex has no nonmatching same-length redaction, so the
-		// search exhausts. The fallback must still differ from a <=2 char value that
-		// happens to equal `#generateReplacement`'s own `Z`/`ZZ` sentinel (else the
-		// raw secret would ship unchanged) while staying a fixed point under
-		// re-obfuscation. Such a config redacts every character and is pathological
-		// by construction.
-		for (const content of [".", "[\\s\\S]"]) {
+	it("drops a replace regex entirely when it can never redact a 1-2 char match distinctly from itself", () => {
+		// A match-everything regex has no nonmatching same-length redaction for a
+		// 1-2 char value, so `findNonMatchingReplacement` would exhaust even against
+		// an isolated probe drawn from the fallback's own alphabet — the regex has
+		// already proven it matches literally every candidate that alphabet could
+		// produce. Accepting the entry would mean the only "redaction" available is
+		// the raw match returned unchanged, which is worse than not registering the
+		// entry at all. The constructor now drops such entries silently at
+		// construction (`regexHasUnresolvableShortMatchFallback`), so with no other
+		// configured secret, `hasSecrets()` is false and the input passes through
+		// completely unobfuscated.
+		for (const content of [".", "[\\s\\S]", "[\\s\\S]{2}"]) {
 			const obf = new SecretObfuscator([{ type: "regex", mode: "replace", content }], "Q".repeat(43));
+			const input = content === "[\\s\\S]{2}" ? "ZZ" : "Z";
 
-			const out = obf.obfuscate("Z");
-
-			expect(out).not.toBe("Z");
-			expect(out).toHaveLength(1);
-			expect(obf.obfuscate(out)).toBe(out);
+			expect(obf.hasSecrets()).toBe(false);
+			expect(obf.obfuscate(input)).toBe(input);
 		}
 	});
 
-	it("never emits a 2-char match unchanged when no same-length value avoids the regex", () => {
-		// Same pathology as above, sized to hit the `ZZ` half of `#generateReplacement`'s
-		// sentinel rather than the `Z` half.
-		const obf = new SecretObfuscator([{ type: "regex", mode: "replace", content: "[\\s\\S]{2}" }], "Q".repeat(43));
-
-		const out = obf.obfuscate("ZZ");
-
-		expect(out).not.toBe("ZZ");
-		expect(out).toHaveLength(2);
-		expect(obf.obfuscate(out)).toBe(out);
-	});
-
 	it("resolves a three-character match-everything regex without the removed exhaustive sweep", () => {
 		// Regression: `findNonMatchingReplacement` used to special-case `len <= 3`
 		// with a fully exhaustive search over every `base**length` candidate
@@ -2473,6 +2493,26 @@ describe("SecretObfuscator friendlyName placeholders", () => {
 			await fs.rm(root, { recursive: true, force: true });
 		}
 	});
+
+	it("rejects a secrets.yml replace regex entry that can never redact a 1-2 char match distinctly from itself", async () => {
+		const root = await fs.mkdtemp(path.join(os.tmpdir(), "omp-secret-friendly-"));
+		try {
+			const project = path.join(root, "project");
+			const agentDir = path.join(root, "agent");
+			await fs.mkdir(path.join(project, ".omp"), { recursive: true });
+			await fs.mkdir(agentDir, { recursive: true });
+			await fs.writeFile(
+				path.join(project, ".omp", "secrets.yml"),
+				'- type: regex\n  mode: replace\n  content: "."\n',
+			);
+
+			const entries = await loadSecrets(project, agentDir);
+
+			expect(entries).toEqual([]);
+		} finally {
+			await fs.rm(root, { recursive: true, force: true });
+		}
+	});
 });
 
 describe("SecretObfuscator cross-turn cache stability", () => {