diff --git a/scripts/branding-drift-baseline.json b/scripts/branding-drift-baseline.json index ea0f5f2284..14dabb7fcb 100644 --- a/scripts/branding-drift-baseline.json +++ b/scripts/branding-drift-baseline.json @@ -25,7 +25,7 @@ "src/review/repo-doc-render.ts": 2, "src/review/repo-skill-render.ts": 2, "src/review/selftune-wire.ts": 1, - "src/selfhost/ai.ts": 6, + "src/selfhost/ai.ts": 9, "src/selfhost/health.ts": 3, "src/selfhost/monitored-work.ts": 1, "src/selfhost/orb-collector.ts": 1, diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index 4549c38d84..9157c8254a 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -473,12 +473,29 @@ export function subscriptionCliEnv( return child; } +// The pre-rebrand name of the unsafe-reviewer opt-in flag. #5652 retired dual-read of GITTENSORY_-prefixed vars +// repo-wide, so an operator whose .env still uses this name silently reverts to fully-disabled. We deliberately do +// NOT honor it (the rebrand is intentional), but we still recognize it here purely to emit an actionable "rename it" +// error instead of the same generic message an operator who never configured anything at all would get. Accessed +// via this constant (not `env.GITTENSORY_...`) so the retired name stays out of the generated self-host env +// reference — it is not a var operators should configure, only one we detect to redirect them. +const LEGACY_UNSAFE_CODEX_REVIEWER_FLAG = "GITTENSORY_ENABLE_UNSAFE_CODEX_REVIEWER"; + function assertCodexCredentialIsolation(env: Record): void { // `codex exec` receives attacker-controlled PR title/body/diff text. Its read-only sandbox prevents writes, but not // reads, so a self-hosted OAuth home mounted into the same filesystem can be prompt-injected into public output. // Fail closed until Codex exposes a brokered credential mode that does not put auth.json in the review sandbox. // Strict "1"-only, matching health.ts's codexAuthReadinessProbe and this flag's narrow opt-in convention. if (env.CODEX_HOME || env.LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER !== "1") { + // An operator still on the retired flag name gets a specific, actionable signal to rename it — but only when the + // current flag isn't already correctly set (a mounted CODEX_HOME with a valid opt-in is a different failure and + // must not be mislabeled a rename problem). Keep the `codex_credential_isolation_required` prefix so the + // structural circuit breaker in ai-review.ts still recognizes this deterministic failure and backs off. + if (env.LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER !== "1" && env[LEGACY_UNSAFE_CODEX_REVIEWER_FLAG] === "1") { + throw new Error( + `codex_credential_isolation_required: ${LEGACY_UNSAFE_CODEX_REVIEWER_FLAG} is set but was retired in #5652; rename it to LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER`, + ); + } throw new Error("codex_credential_isolation_required"); } } diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 093a7335d1..6e3d213236 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -1022,15 +1022,19 @@ export function isRateLimitError(error: unknown): boolean { } /** True for a provider's own STRUCTURAL misconfiguration signal (`src/selfhost/ai.ts`'s - * `codex_auth_not_configured` / `codex_no_auth` — a missing or expired credential file). Unlike a transient - * timeout or rate limit, this will fail identically on every future attempt until an operator re-runs - * `codex auth` -- confirmed live (GITTENSORY-K/8: 2094 + 544 events over 16 days from one unfixed - * misconfiguration, the credential file was never present the whole time). Mirrors - * {@link isSubscriptionCliTimeout}/{@link isRateLimitError}'s identical non-transient-error short-circuit. - * Exported so `src/selfhost/ai.ts`'s circuit breaker can give this failure class a much longer cooldown - * than a genuinely transient one. */ + * `codex_auth_not_configured` / `codex_no_auth` — a missing or expired credential file — or + * `codex_credential_isolation_required` — the fail-closed opt-in guard, thrown either bare or with a + * `: rename …` detail suffix). Unlike a transient timeout or rate limit, these fail identically on every future + * attempt until an operator re-runs `codex auth` / fixes the opt-in flag -- confirmed live (GITTENSORY-K/8: + * 2094 + 544 events over 16 days from one unfixed misconfiguration, the credential file was never present the + * whole time). Mirrors {@link isSubscriptionCliTimeout}/{@link isRateLimitError}'s identical non-transient-error + * short-circuit. Exported so `src/selfhost/ai.ts`'s circuit breaker can give this failure class a much longer + * cooldown than a genuinely transient one. */ export function isStructuralProviderConfigError(error: unknown): boolean { - return error instanceof Error && /^codex_(?:auth_not_configured|no_auth):/.test(error.message); + return ( + error instanceof Error && + /^codex_(?:auth_not_configured|no_auth|credential_isolation_required)(?::|$)/.test(error.message) + ); } /** Cap on the diagnostic prefix logged for an unparseable model response (#observability-unparseable) -- long diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 6531f0c16b..0c7f0593ea 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3077,6 +3077,18 @@ describe("pure helpers", () => { it("isStructuralProviderConfigError matches only codex's own structural-config error messages, not other Errors or non-Error throws (GITTENSORY-K/8)", () => { expect(isStructuralProviderConfigError(new Error("codex_auth_not_configured: ~/.codex/auth.json not found"))).toBe(true); expect(isStructuralProviderConfigError(new Error("codex_no_auth: auth.json missing or expired"))).toBe(true); + // The fail-closed credential-isolation guard is equally deterministic, thrown either bare (never opted in) or + // with a `: rename …` detail suffix (legacy flag name still set) -- both must earn the structural cooldown (#7466). + expect(isStructuralProviderConfigError(new Error("codex_credential_isolation_required"))).toBe(true); + expect( + isStructuralProviderConfigError( + new Error( + "codex_credential_isolation_required: GITTENSORY_ENABLE_UNSAFE_CODEX_REVIEWER is set but was retired in #5652; rename it to LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER", + ), + ), + ).toBe(true); + // Prefix-anchored, but must not match a longer look-alike token that merely starts with the same characters. + expect(isStructuralProviderConfigError(new Error("codex_no_auth_pending"))).toBe(false); expect(isStructuralProviderConfigError(new Error("connection reset"))).toBe(false); // Anchored ("^codex_...") -- a wrapped/rethrown message doesn't match, only the exact provider-level throw does. expect(isStructuralProviderConfigError(new Error("wrapped: codex_auth_not_configured: nested"))).toBe(false); diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index 6008e445bc..4f592e2596 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -1945,6 +1945,52 @@ describe("subscription CLI helpers + fail-safe", () => { ).rejects.toThrow(/codex_credential_isolation_required/); }); + it("credential isolation: an operator still on the retired GITTENSORY_ flag name gets an actionable rename error, not the generic one (#7466)", async () => { + const shouldNotSpawn: StubSpawn = async () => { + throw new Error("spawned"); + }; + // Legacy flag set (and the current LOOPOVER_ one absent) — the operator opted in under the pre-rebrand name and + // silently reverted to disabled. The error must name both the retired var and its replacement so it is fixable + // without reading the source, and must still carry the codex_credential_isolation_required prefix. + await expect( + createCodexAi( + { GITTENSORY_ENABLE_UNSAFE_CODEX_REVIEWER: "1" }, + shouldNotSpawn, + ).run("gpt-5", { prompt: "x" }), + ).rejects.toThrow( + /codex_credential_isolation_required: GITTENSORY_ENABLE_UNSAFE_CODEX_REVIEWER is set but was retired in #5652; rename it to LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER/, + ); + }); + + it("credential isolation: never-configured still throws the plain generic error, distinct from the legacy-rename one (#7466)", async () => { + const shouldNotSpawn: StubSpawn = async () => { + throw new Error("spawned"); + }; + // Neither the current nor the retired flag is set — this must stay the bare generic message so it is + // distinguishable from the legacy-rename case above. + await expect(createCodexAi({}, shouldNotSpawn).run("gpt-5", { prompt: "x" })).rejects.toThrow( + /^codex_credential_isolation_required$/, + ); + }); + + it("credential isolation: a set CODEX_HOME with a valid opt-in is not mislabeled a legacy-rename problem even when the legacy flag is also present (#7466)", async () => { + const shouldNotSpawn: StubSpawn = async () => { + throw new Error("spawned"); + }; + // CODEX_HOME is mounted (a distinct failure) while the current opt-in is correctly "1" — the rename branch must + // not fire; the operator sees the generic isolation error, not a spurious "rename your flag" instruction. + await expect( + createCodexAi( + { + CODEX_HOME: "/home/node/.codex", + LOOPOVER_ENABLE_UNSAFE_CODEX_REVIEWER: "1", + GITTENSORY_ENABLE_UNSAFE_CODEX_REVIEWER: "1", + }, + shouldNotSpawn, + ).run("gpt-5", { prompt: "x" }), + ).rejects.toThrow(/^codex_credential_isolation_required$/); + }); + it("resolveCodexAuthPath: CODEX_HOME wins, else HOME/.codex, else ~/.codex", () => { expect(resolveCodexAuthPath({ CODEX_HOME: "/data/codex", HOME: "/home/node" })).toBe( "/data/codex/auth.json",