Skip to content

Commit 72be1b3

Browse files
committed
refactor(auth): extract shared provider auth helpers (C1-C7)
~250 lines of near-identical callback/login/session/store/lock twins across codex and xai collapse onto seven shared helpers, each taking a small provider spec of explicit hooks. Deliberate provider differences stay hooks, not knobs: the codex inter-process refresh lock is a serializeRefresh hook (xai has none by design), accountId merge and token expiry stay per-provider, the divergent stale horizons carry must-not-unify comments, and exact-code vs substring error matching is untouched. Lock contention still never surfaces as credential_failure. One deliberate delta: the unified file lock propagates non-ENOENT fs errors where the codex refresh lock used to swallow every fs error; the store contract was the stronger one. The propagation is pinned at both levels — this suite exercises the refresh path and file-lock.test pins the primitive.
1 parent fe35dce commit 72be1b3

22 files changed

Lines changed: 1391 additions & 691 deletions

‎src/auth/bearer-headers.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
export interface BearerToken {
2+
readonly access: string;
3+
}
4+
5+
// The one header every provider carries on an inference request; provider
6+
// extras (account ids, client identifiers) ride alongside via the spread.
7+
export function bearerHeaders(
8+
token: BearerToken,
9+
extra: Record<string, string> = {},
10+
): Record<string, string> {
11+
return { authorization: `Bearer ${token.access}`, ...extra };
12+
}

‎src/auth/codex/auth-headers.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { bearerHeaders } from "../bearer-headers.js";
12
import { CODEX_CLIENT_VERSION, CODEX_ORIGINATOR } from "./constants.js";
23

34
export function codexAuthHeadersForToken(
@@ -7,11 +8,10 @@ export function codexAuthHeadersForToken(
78
},
89
commandName: string,
910
): Record<string, string> {
10-
const headers: Record<string, string> = {
11-
authorization: `Bearer ${token.access}`,
11+
const headers = bearerHeaders(token, {
1212
originator: CODEX_ORIGINATOR,
1313
"user-agent": `${commandName} (${CODEX_ORIGINATOR}/${CODEX_CLIENT_VERSION})`,
14-
};
14+
});
1515
// An empty account id carries no identity — sending it as a header value
1616
// would label the request with a meaningless id. Only a non-empty id rides.
1717
if (token.accountId) headers["chatgpt-account-id"] = token.accountId;

‎src/auth/codex/callback-server.ts‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
1-
import { startCallbackServer, type CallbackServer } from "@corbits/oauth-core";
2-
31
import {
4-
authorizationDoneHtml,
5-
callbackPageHtml,
6-
type CallbackPageCopy,
7-
} from "../callback-page.js";
2+
startProviderCallbackServer,
3+
type CallbackServer,
4+
} from "../provider-callback-server.js";
5+
6+
import type { CallbackPageCopy } from "../callback-page.js";
87
import {
98
CODEX_CALLBACK_HOST,
109
CODEX_CALLBACK_PATH,
@@ -19,13 +18,15 @@ export async function startCodexCallbackServer(
1918
expectedState: string,
2019
copy: CallbackPageCopy,
2120
): Promise<CodexCallbackServer> {
22-
return startCallbackServer(expectedState, {
23-
port: CODEX_CALLBACK_PORT,
24-
// Codex's registered redirect_uri uses localhost (not 127.0.0.1).
25-
host: CODEX_CALLBACK_HOST,
26-
path: CODEX_CALLBACK_PATH,
27-
doneHtml: authorizationDoneHtml("Codex", copy),
28-
failedHtml: (reason) =>
29-
callbackPageHtml({ subject: "Codex", error: reason }, copy),
30-
});
21+
return startProviderCallbackServer(
22+
"Codex",
23+
{
24+
port: CODEX_CALLBACK_PORT,
25+
// Codex's registered redirect_uri uses localhost (not 127.0.0.1).
26+
host: CODEX_CALLBACK_HOST,
27+
path: CODEX_CALLBACK_PATH,
28+
},
29+
expectedState,
30+
copy,
31+
);
3132
}

‎src/auth/codex/login.ts‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
import {
2-
buildAuthorizeUrl,
2+
exchangeCodexCode,
3+
codexOAuthConfig,
4+
type CodexTokens,
5+
} from "@corbits/codex-provider";
6+
import {
37
openInBrowser,
4-
startOAuthLogin,
58
type OAuthLoginHandle,
69
type StartOAuthLoginOptions,
710
} from "@corbits/oauth-core";
8-
import {
9-
codexOAuthConfig,
10-
exchangeCodexCode,
11-
type CodexTokens,
12-
} from "@corbits/codex-provider";
11+
import { startProviderLogin } from "../provider-login.js";
1312

1413
import type { CallbackPageCopy } from "../callback-page.js";
1514
import { saveCodexProfile } from "../../config/oauth-stores.js";
@@ -28,10 +27,10 @@ export async function startCodexLogin(
2827
opts: StartCodexLoginOptions,
2928
): Promise<CodexLoginHandle> {
3029
const { home, copy, ...loginOpts } = opts;
31-
return startOAuthLogin(loginOpts, {
30+
return startProviderLogin<CodexTokens>(loginOpts, {
31+
oauthConfig: codexOAuthConfig,
3232
startCallbackServer: (state) => startCodexCallbackServer(state, copy),
33-
buildAuthorizeUrl: (pkce, state) =>
34-
buildAuthorizeUrl(codexOAuthConfig, pkce, state),
33+
// The mapper leaves expiresAt unset when the response omits expires_in.
3534
exchangeCode: async (code, verifier, now) =>
3635
withDefaultCodexExpiry(await exchangeCodexCode(code, verifier, now), now),
3736
saveProfile: (profile) => saveCodexProfile(profile, home),

‎src/auth/codex/refresh-lock.test.ts‎

Lines changed: 103 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,36 @@ import {
99
import { tmpdir } from "node:os";
1010
import { join } from "node:path";
1111
import { describe, expect, test } from "bun:test";
12-
import {
13-
CodexRefreshLockTimeoutError,
14-
withCodexRefreshLock,
15-
} from "./refresh-lock.js";
12+
import { CodexRefreshLockTimeoutError } from "./refresh-lock.js";
13+
14+
import { withMockedModule } from "../../../testkit/mock-module.js";
15+
16+
// Non-ENOENT lock-file failures must surface raw instead of being swallowed
17+
// (the pre-unification codex lock caught every fs error). Flags gate each
18+
// failure to its own test; undefined keeps every call on the real module.
19+
let failingReadPath: string | undefined;
20+
let failingStatPath: string | undefined;
21+
let failingUnlinkPath: string | undefined;
22+
await withMockedModule(
23+
import.meta.resolve("node:fs/promises"),
24+
(real: typeof import("node:fs/promises")) => ({
25+
...real,
26+
readFile: async (...args: Parameters<typeof real.readFile>) => {
27+
if (args[0] === failingReadPath) throw new Error("lock read failed");
28+
return real.readFile(...args);
29+
},
30+
stat: async (...args: Parameters<typeof real.stat>) => {
31+
if (args[0] === failingStatPath) throw new Error("lock stat failed");
32+
return real.stat(...args);
33+
},
34+
unlink: async (...args: Parameters<typeof real.unlink>) => {
35+
if (args[0] === failingUnlinkPath) throw new Error("lock release failed");
36+
return real.unlink(...args);
37+
},
38+
}),
39+
);
40+
41+
const { withCodexRefreshLock } = await import("./refresh-lock.js");
1642

1743
async function tempDir(): Promise<string> {
1844
return mkdtemp(join(tmpdir(), "cl8628-lock-"));
@@ -214,4 +240,77 @@ describe("codex refresh lock", () => {
214240
await rm(dir, { recursive: true, force: true });
215241
}
216242
});
243+
244+
test("a non-ENOENT read failure while contending propagates raw", async () => {
245+
const dir = await tempDir();
246+
try {
247+
const lock = join(dir, "refresh.lock");
248+
await writeFile(lock, "");
249+
failingReadPath = lock;
250+
try {
251+
// The unified lock re-reads the holder content while contending; a
252+
// permission/disk failure there must surface, not read as "no lock".
253+
await expect(
254+
withCodexRefreshLock(lock, async () => "never", {
255+
timeoutMs: 1_000,
256+
retryMs: 10,
257+
}),
258+
).rejects.toThrow("lock read failed");
259+
} finally {
260+
failingReadPath = undefined;
261+
}
262+
} finally {
263+
await rm(dir, { recursive: true, force: true });
264+
}
265+
});
266+
267+
test("a non-ENOENT stat failure while deciding staleness propagates raw", async () => {
268+
const dir = await tempDir();
269+
try {
270+
const lock = join(dir, "refresh.lock");
271+
// Untagged content routes the staleness decision through mtime stat;
272+
// a non-ENOENT stat failure must surface, not read as "not stale".
273+
await writeFile(lock, "");
274+
failingStatPath = lock;
275+
try {
276+
await expect(
277+
withCodexRefreshLock(lock, async () => "never", {
278+
timeoutMs: 1_000,
279+
retryMs: 10,
280+
}),
281+
).rejects.toThrow("lock stat failed");
282+
} finally {
283+
failingStatPath = undefined;
284+
}
285+
} finally {
286+
await rm(dir, { recursive: true, force: true });
287+
}
288+
});
289+
290+
test("a non-ENOENT release failure surfaces after the work value", async () => {
291+
const dir = await tempDir();
292+
try {
293+
const lock = join(dir, "refresh.lock");
294+
failingUnlinkPath = lock;
295+
try {
296+
// The refresh succeeded, but a release that fails for a real reason
297+
// (not a steal's ENOENT) must not vanish: the caller has to know the
298+
// lock file is still on disk.
299+
await expect(
300+
withCodexRefreshLock(lock, async () => "done", {
301+
timeoutMs: 1_000,
302+
retryMs: 10,
303+
}),
304+
).rejects.toThrow("lock release failed");
305+
// The failed release leaves the claim file in place for recovery.
306+
await expect(readFile(lock, "utf8")).resolves.toContain(
307+
String(process.pid),
308+
);
309+
} finally {
310+
failingUnlinkPath = undefined;
311+
}
312+
} finally {
313+
await rm(dir, { recursive: true, force: true });
314+
}
315+
});
217316
});

0 commit comments

Comments
 (0)