Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/branding-drift-baseline.json
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
17 changes: 17 additions & 0 deletions src/selfhost/ai.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string | undefined>): 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");
}
}
Expand Down
20 changes: 12 additions & 8 deletions src/services/ai-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions test/unit/ai-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
46 changes: 46 additions & 0 deletions test/unit/selfhost-ai.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down