Skip to content

Commit 182b77e

Browse files
ndycodendycode
committed
Address remaining goal proxy review findings
1 parent 6a1a74a commit 182b77e

4 files changed

Lines changed: 211 additions & 25 deletions

File tree

‎lib/runtime-rotation-proxy.ts‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,10 @@ function readStringSearchParam(searchParams: URLSearchParams, key: string): stri
341341
return value && value.trim().length > 0 ? value.trim() : null;
342342
}
343343

344+
function isThreadGoalFallbackStatus(status: number): boolean {
345+
return status === HTTP_STATUS.FORBIDDEN;
346+
}
347+
344348
function resolveSessionKey(headers: Headers, parsedBody: RequestBody | null): string | null {
345349
const headerKey =
346350
headers.get(OPENAI_HEADERS.SESSION_ID) ??
@@ -938,7 +942,7 @@ export async function startRuntimeRotationProxy(
938942
operation: isModelsRequest
939943
? "models"
940944
: isThreadGoalRequest
941-
? "diagnostic"
945+
? "responses"
942946
: "responses",
943947
model: context.model,
944948
projectKey,
@@ -1136,21 +1140,32 @@ export async function startRuntimeRotationProxy(
11361140
continue;
11371141
}
11381142

1139-
if (isThreadGoalRequest && upstream.status >= 400) {
1143+
if (isThreadGoalRequest && isThreadGoalFallbackStatus(upstream.status)) {
11401144
await readErrorBody(upstream);
11411145
const parsedGoalBody = parseRequestBody(context.body);
1142-
const fallbackKey = context.sessionKey ?? "default";
1146+
const fallbackKey = context.sessionKey;
11431147
const goal =
11441148
typeof parsedGoalBody?.goal === "string" ? parsedGoalBody.goal : null;
1145-
accountManager.recordSuccess(refreshed.account, context.family, context.model);
1146-
await persistRuntimeActiveAccount(
1147-
accountManager,
1148-
refreshed.account,
1149-
context.family,
1150-
);
1149+
if (!fallbackKey) {
1150+
await usageRecorder.record({
1151+
outcome: "failure",
1152+
statusCode: HTTP_STATUS.BAD_REQUEST,
1153+
errorCode: "thread_goal_session_key_required",
1154+
account: refreshed.account,
1155+
});
1156+
writeJson(res, HTTP_STATUS.BAD_REQUEST, {
1157+
error: {
1158+
message:
1159+
"Thread goal fallback requires a thread_id, threadId, or session header.",
1160+
code: "thread_goal_session_key_required",
1161+
},
1162+
});
1163+
return;
1164+
}
11511165
await usageRecorder.record({
1152-
outcome: "success",
1153-
statusCode: HTTP_STATUS.OK,
1166+
outcome: "failure",
1167+
statusCode: upstream.status,
1168+
errorCode: "thread_goal_upstream_blocked",
11541169
account: refreshed.account,
11551170
});
11561171
if (context.upstreamPath.endsWith("/set")) {
@@ -1164,6 +1179,23 @@ export async function startRuntimeRotationProxy(
11641179
return;
11651180
}
11661181

1182+
if (isThreadGoalRequest && upstream.status >= 400) {
1183+
const forwarded = await forwardStreamingResponse(
1184+
upstream,
1185+
res,
1186+
status,
1187+
() => undefined,
1188+
streamStallTimeoutMs,
1189+
);
1190+
await usageRecorder.record({
1191+
outcome: "failure",
1192+
statusCode: upstream.status,
1193+
errorCode: forwarded ? "thread_goal_upstream_error" : "stream_forward_failed",
1194+
account: refreshed.account,
1195+
});
1196+
return;
1197+
}
1198+
11671199
accountManager.recordSuccess(refreshed.account, context.family, context.model);
11681200
const nearExhaustionWaitMs = getQuotaNearExhaustionWaitMs(
11691201
upstream.headers,

‎scripts/codex.js‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ const shadowHomeCleanupRetryMarkerDir =
8989
(process.env.CODEX_MULTI_AUTH_TEST_SHADOW_RETRY_MARKER_DIR ?? "").trim();
9090
let warnedInvalidRuntimeRotationProxyEnv = false;
9191
let warnedPendingAccountReadIdOverflow = false;
92+
let warnedShadowHomeSqliteLinkFailure = false;
9293

9394
async function loadRuntimeConstants() {
9495
const fallback = {
@@ -1906,15 +1907,19 @@ function shouldMaterializeFileIntoShadowHome(name) {
19061907
return /\.sqlite(?:-(?:shm|wal))?$/i.test(name);
19071908
}
19081909

1909-
function materializeFileIntoShadowHome(sourcePath, destinationPath, tightenFile) {
1910+
function materializeFileIntoShadowHome(sourcePath, destinationPath) {
19101911
try {
19111912
linkSync(sourcePath, destinationPath);
1912-
return;
1913+
return true;
19131914
} catch {
1914-
// Hard links make SQLite roots look local without copying an active DB.
1915+
if (!warnedShadowHomeSqliteLinkFailure) {
1916+
warnedShadowHomeSqliteLinkFailure = true;
1917+
console.error(
1918+
"codex-multi-auth: skipped SQLite shadow-home materialization because hard-linking failed; refusing to copy active SQLite state.",
1919+
);
1920+
}
19151921
}
1916-
copyFileSync(sourcePath, destinationPath);
1917-
tightenFile(destinationPath);
1922+
return false;
19181923
}
19191924

19201925
function collectShadowHomeSyncFileNames(shadowCodexHome, syncFileNames) {
@@ -2120,7 +2125,7 @@ function createShadowHomeMirror(
21202125
copyFileSync(sourcePath, destinationPath);
21212126
tightenFile(destinationPath);
21222127
} else if (shouldMaterializeFile) {
2123-
materializeFileIntoShadowHome(sourcePath, destinationPath, tightenFile);
2128+
materializeFileIntoShadowHome(sourcePath, destinationPath);
21242129
} else {
21252130
mirrorFileIntoShadowHome(sourcePath, destinationPath, tightenFile);
21262131
}
@@ -2881,10 +2886,8 @@ async function createRuntimeRotationAppHelperContext(
28812886

28822887
const cleanup = async ({ exitCode } = {}) => {
28832888
const livedMs = Date.now() - startedAt;
2884-
if (
2885-
options.detachOnExit === true ||
2886-
(exitCode === 0 && livedMs < detachGraceMs)
2887-
) {
2889+
const exitedSuccessfully = exitCode === 0 || exitCode === null || exitCode === undefined;
2890+
if (exitedSuccessfully && (options.detachOnExit === true || livedMs < detachGraceMs)) {
28882891
helper.stdout?.destroy();
28892892
helper.stderr?.destroy();
28902893
helper.unref();

‎test/codex-bin-wrapper.test.ts‎

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,14 @@ import {
1414
writeFileSync,
1515
} from "node:fs";
1616
import { tmpdir } from "node:os";
17-
import { delimiter, dirname, join } from "node:path";
17+
import {
18+
delimiter,
19+
dirname,
20+
isAbsolute,
21+
join,
22+
relative,
23+
resolve,
24+
} from "node:path";
1825
import process from "node:process";
1926
import { fileURLToPath, pathToFileURL } from "node:url";
2027
import { afterEach, describe, expect, it } from "vitest";
@@ -1043,9 +1050,11 @@ describe("codex bin wrapper", () => {
10431050
const shadowHomeMatch = output.match(/^CODEX_HOME:(.+)$/m);
10441051
expect(shadowHomeMatch?.[1]).toBeTruthy();
10451052
if (shadowHomeMatch?.[1]) {
1046-
expect(shadowHomeMatch[1]).toContain(
1047-
join(originalHome, "multi-auth", "runtime-shadow-homes"),
1048-
);
1053+
const expectedRoot = resolve(originalHome, "multi-auth", "runtime-shadow-homes");
1054+
const actual = resolve(shadowHomeMatch[1]);
1055+
const shadowRelativePath = relative(expectedRoot, actual);
1056+
expect(shadowRelativePath).not.toMatch(/^\.\.(?:[\\/]|$)/);
1057+
expect(isAbsolute(shadowRelativePath)).toBe(false);
10491058
expect(existsSync(shadowHomeMatch[1])).toBe(false);
10501059
}
10511060
expect(readFileSync(markerPath, "utf8")).toBe(
@@ -1805,6 +1814,42 @@ describe("codex bin wrapper", () => {
18051814
expect(marker).toContain("close\n");
18061815
});
18071816

1817+
it("stops detached TUI app helpers after failed launches", async () => {
1818+
const fixtureRoot = createWrapperFixture();
1819+
createRuntimeRotationProxyFixtureModule(fixtureRoot);
1820+
const fakeBin = createCustomFakeCodexBin(fixtureRoot, [
1821+
"#!/usr/bin/env node",
1822+
'console.error("tui failed");',
1823+
"process.exit(1);",
1824+
]);
1825+
const originalHome = join(fixtureRoot, "codex-home");
1826+
const markerPath = join(fixtureRoot, "proxy-marker.txt");
1827+
mkdirSync(originalHome, { recursive: true });
1828+
writeFileSync(
1829+
join(originalHome, "config.toml"),
1830+
'model_provider = "openai"\n',
1831+
"utf8",
1832+
);
1833+
1834+
const result = runWrapper(fixtureRoot, [], {
1835+
CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin,
1836+
CODEX_HOME: originalHome,
1837+
CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1",
1838+
CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "10000",
1839+
CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath,
1840+
CODEX_MULTI_AUTH_TEST_PROXY_MARKER_PID: "1",
1841+
OPENAI_API_KEY: undefined,
1842+
});
1843+
1844+
expect(result.status).toBe(1);
1845+
const marker = readFileSync(markerPath, "utf8");
1846+
const helperPid = Number(
1847+
marker.match(/^start:http:\/\/127\.0\.0\.1:4567:pid=(\d+)$/m)?.[1] ?? NaN,
1848+
);
1849+
expect(Number.isFinite(helperPid)).toBe(true);
1850+
expect(isProcessAlive(helperPid)).toBe(false);
1851+
});
1852+
18081853
it("writes app router status files with owner-only permissions", async () => {
18091854
const fixtureRoot = createWrapperFixture();
18101855
createRuntimeRotationProxyFixtureModule(fixtureRoot);

‎test/runtime-rotation-proxy.test.ts‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -632,6 +632,30 @@ describe("runtime rotation proxy", () => {
632632
]);
633633
});
634634

635+
it("rejects anonymous blocked thread goal fallbacks instead of sharing state", async () => {
636+
const now = Date.now();
637+
const accountManager = new AccountManager(undefined, createStorage(now));
638+
const { calls, fetchImpl } = createRecordingFetch(
639+
() =>
640+
new Response("<html>blocked</html>", {
641+
status: HTTP_STATUS.FORBIDDEN,
642+
headers: { "content-type": "text/html" },
643+
}),
644+
);
645+
const proxy = await startProxy({ accountManager, fetchImpl });
646+
647+
const response = await postThreadGoal(proxy, { goal: "ship it" }, "/thread/goal/set");
648+
649+
expect(response.status).toBe(HTTP_STATUS.BAD_REQUEST);
650+
expect(await response.json()).toEqual({
651+
error: {
652+
message: "Thread goal fallback requires a thread_id, threadId, or session header.",
653+
code: "thread_goal_session_key_required",
654+
},
655+
});
656+
expect(calls).toHaveLength(1);
657+
});
658+
635659
it("keys blocked GET thread goal fallbacks by query thread id", async () => {
636660
const now = Date.now();
637661
const accountManager = new AccountManager(undefined, createStorage(now));
@@ -659,6 +683,88 @@ describe("runtime rotation proxy", () => {
659683
]);
660684
});
661685

686+
it("passes through non-fallback thread goal client errors", async () => {
687+
const now = Date.now();
688+
const accountManager = new AccountManager(undefined, createStorage(now));
689+
const recordSuccessSpy = vi.spyOn(accountManager, "recordSuccess");
690+
const { calls, fetchImpl } = createRecordingFetch(
691+
() =>
692+
new Response('{"error":{"code":"bad_goal"}}\n', {
693+
status: HTTP_STATUS.BAD_REQUEST,
694+
headers: { "content-type": "application/json" },
695+
}),
696+
);
697+
const proxy = await startProxy({ accountManager, fetchImpl });
698+
699+
const response = await postThreadGoal(
700+
proxy,
701+
{ threadId: "thread-1", goal: "" },
702+
"/thread/goal/set",
703+
);
704+
705+
expect(response.status).toBe(HTTP_STATUS.BAD_REQUEST);
706+
expect(await response.text()).toBe('{"error":{"code":"bad_goal"}}\n');
707+
expect(calls).toHaveLength(1);
708+
expect(recordSuccessSpy).not.toHaveBeenCalled();
709+
});
710+
711+
it("rejects unauthenticated thread goal requests", async () => {
712+
const now = Date.now();
713+
const accountManager = new AccountManager(undefined, createStorage(now));
714+
const { calls, fetchImpl } = createRecordingFetch(
715+
() => new Response('{"ok":true}', { status: HTTP_STATUS.OK }),
716+
);
717+
const proxy = await startProxy({ accountManager, fetchImpl });
718+
719+
const response = await postThreadGoal(
720+
proxy,
721+
{ threadId: "thread-1" },
722+
"/thread/goal/get",
723+
{ authorization: "Bearer caller-token", "x-api-key": "caller-key" },
724+
);
725+
726+
expect(response.status).toBe(HTTP_STATUS.UNAUTHORIZED);
727+
expect(calls).toHaveLength(0);
728+
});
729+
730+
it("isolates local thread goal fallback state across concurrent threads", async () => {
731+
const now = Date.now();
732+
const accountManager = new AccountManager(undefined, createStorage(now));
733+
const { calls, fetchImpl } = createRecordingFetch(
734+
() =>
735+
new Response("<html>blocked</html>", {
736+
status: HTTP_STATUS.FORBIDDEN,
737+
headers: { "content-type": "text/html" },
738+
}),
739+
);
740+
const proxy = await startProxy({ accountManager, fetchImpl });
741+
742+
const [setA, setB] = await Promise.all([
743+
postThreadGoal(proxy, { threadId: "thread-a", goal: "goal-a" }, "/thread/goal/set"),
744+
postThreadGoal(proxy, { threadId: "thread-b", goal: "goal-b" }, "/thread/goal/set"),
745+
]);
746+
const [getA, getB] = await Promise.all([
747+
postThreadGoal(proxy, { threadId: "thread-a" }, "/thread/goal/get"),
748+
postThreadGoal(proxy, { threadId: "thread-b" }, "/thread/goal/get"),
749+
]);
750+
751+
expect(setA.status).toBe(HTTP_STATUS.OK);
752+
expect(setB.status).toBe(HTTP_STATUS.OK);
753+
expect(await getA.json()).toEqual({ goal: "goal-a" });
754+
expect(await getB.json()).toEqual({ goal: "goal-b" });
755+
expect(calls).toHaveLength(4);
756+
expect(
757+
calls.filter(
758+
(call) => call.url === "https://example.test/backend-api/codex/thread/goal/set",
759+
),
760+
).toHaveLength(2);
761+
expect(
762+
calls.filter(
763+
(call) => call.url === "https://example.test/backend-api/codex/thread/goal/get",
764+
),
765+
).toHaveLength(2);
766+
});
767+
662768
it("rejects unauthenticated model discovery requests", async () => {
663769
const now = Date.now();
664770
const accountManager = new AccountManager(undefined, createStorage(now));

0 commit comments

Comments
 (0)