feat(codex): add session ID auto-completion for Codex requests#598
feat(codex): add session ID auto-completion for Codex requests#598
Conversation
Implement CodexSessionIdCompleter to automatically synchronize session IDs across headers (session_id, x-session-id), body (prompt_cache_key), and metadata fields. Uses fingerprint-based Redis caching to maintain session continuity for clients that don't provide explicit session IDs. - Add enable_codex_session_id_completion system setting (default: true) - Generate UUID v7 for new sessions when no ID is provided - Support multiple completion actions: copy, align, generate - Include comprehensive unit tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough添加了新的 Codex 会话完成工具,通过在 system_settings 表中新增 enable_codex_session_id_completion 列,并配合数据库快照和单元测试。该工具管理会话 ID 在请求头、请求体和 Redis 之间的传播和持久化。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust session ID auto-completion mechanism for Codex requests. The primary goal is to ensure consistent session tracking by automatically detecting, synchronizing, or generating session IDs across different parts of an incoming request (headers, body, and metadata). For requests lacking an explicit session ID, a new UUID v7 is generated, and a fingerprinting system backed by Redis helps maintain session continuity across subsequent requests from the same client, improving user experience and data integrity. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for session ID auto-completion and synchronization for Codex requests, which is a valuable feature for ensuring session continuity. The core logic in CodexSessionIdCompleter is comprehensive, handling various scenarios where a session ID might be present in headers, the request body, or metadata. The use of fingerprinting with Redis to maintain sessions for clients that don't provide an explicit ID is a clever solution. The implementation is well-tested, covering most critical paths. My feedback focuses on improving code clarity, maintainability, and test coverage with a few targeted suggestions.
| const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10); | ||
|
|
||
| function getClientIp(headers: Headers): string | null { | ||
| // 取链路上的首个 IP |
| const realIp = headers.get("x-real-ip"); | ||
| const ip = | ||
| forwardedFor?.split(",").map((part) => part.trim())[0] || (realIp ? realIp.trim() : null); | ||
| return ip || null; |
| for (const item of input) { | ||
| const obj = ensureObject(item); | ||
| if (!obj) continue; | ||
| const role = typeof obj.role === "string" ? obj.role.toLowerCase() : null; | ||
| if (role !== "system") continue; | ||
|
|
||
| const texts = extractTextParts(obj.content); | ||
| const joined = texts.join("\n").trim(); | ||
| if (joined) { | ||
| systemText = joined; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| for (const item of input) { | ||
| const obj = ensureObject(item); | ||
| if (!obj) continue; | ||
| const role = typeof obj.role === "string" ? obj.role.toLowerCase() : null; | ||
| if (role !== "user") continue; | ||
|
|
||
| const texts = extractTextParts(obj.content); | ||
| const joined = texts.join("\n").trim(); | ||
| if (joined) { | ||
| userText = joined; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This function iterates over the input array twice to find systemText and userText. This can be optimized by using a single loop to find both, which would improve performance, especially for requests with many messages.
for (const item of input) {
if (systemText && userText) break;
const obj = ensureObject(item);
if (!obj) continue;
const role = typeof obj.role === "string" ? obj.role.toLowerCase() : null;
if (role === "system" && !systemText) {
const texts = extractTextParts(obj.content);
const joined = texts.join("\n").trim();
if (joined) {
systemText = joined;
}
} else if (role === "user" && !userText) {
const texts = extractTextParts(obj.content);
const joined = texts.join("\n").trim();
if (joined) {
userText = joined;
}
}
}| static async complete( | ||
| keyId: number, | ||
| headers: Headers, | ||
| requestBody: Record<string, unknown> | ||
| ): Promise<CodexSessionIdCompletionResult> { | ||
| const headerSessionId = normalizeCodexSessionId(headers.get("session_id")); | ||
| const headerXSessionId = normalizeCodexSessionId(headers.get("x-session-id")); | ||
| const bodyPromptCacheKey = normalizeCodexSessionId(requestBody.prompt_cache_key); | ||
| const metadataSessionId = readMetadataSessionId(requestBody); | ||
|
|
||
| const fingerprint = calculateFingerprint(keyId, headers, requestBody); | ||
|
|
||
| // Case: both prompt_cache_key and any session header are present | ||
| if (bodyPromptCacheKey && (headerSessionId || headerXSessionId)) { | ||
| const canonicalHeader = headerSessionId ?? headerXSessionId; | ||
|
|
||
| // If they differ, align to header to avoid ambiguity | ||
| if (canonicalHeader && canonicalHeader !== bodyPromptCacheKey) { | ||
| const wroteSessionIdHeader = setIfMissingOrDifferent(headerSessionId, canonicalHeader, () => | ||
| headers.set("session_id", canonicalHeader) | ||
| ); | ||
| const wroteXSessionIdHeader = setIfMissingOrDifferent( | ||
| headerXSessionId, | ||
| canonicalHeader, | ||
| () => headers.set("x-session-id", canonicalHeader) | ||
| ); | ||
| const wroteHeader = wroteSessionIdHeader || wroteXSessionIdHeader; | ||
|
|
||
| const wroteBody = setIfMissingOrDifferent(bodyPromptCacheKey, canonicalHeader, () => { | ||
| requestBody.prompt_cache_key = canonicalHeader; | ||
| }); | ||
|
|
||
| const wroteMetadata = setIfMissingOrDifferent(metadataSessionId, canonicalHeader, () => { | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = canonicalHeader; | ||
| }); | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
|
|
||
| return { | ||
| applied: wroteHeader || wroteBody || wroteMetadata, | ||
| action: "aligned_mismatch", | ||
| sessionId: canonicalHeader, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| // Already consistent, no-op (but keep mapping warm if possible) | ||
| if (canonicalHeader) { | ||
| // Some clients may only send x-session-id without session_id. | ||
| // If prompt_cache_key is present, we treat it as having the session id and fill session_id. | ||
| if (!headerSessionId) { | ||
| headers.set("session_id", canonicalHeader); | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = canonicalHeader; | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
|
|
||
| return { | ||
| applied: true, | ||
| action: "copied_body_to_header", | ||
| sessionId: canonicalHeader, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| applied: false, | ||
| action: "noop", | ||
| sessionId: canonicalHeader ?? bodyPromptCacheKey, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| // Case: header exists but body missing | ||
| if (headerSessionId || headerXSessionId) { | ||
| const canonical = headerSessionId ?? headerXSessionId; | ||
| if (!canonical) { | ||
| return { | ||
| applied: false, | ||
| action: "noop", | ||
| sessionId: null, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| const wroteSessionIdHeader = setIfMissingOrDifferent(headerSessionId, canonical, () => | ||
| headers.set("session_id", canonical) | ||
| ); | ||
| const wroteXSessionIdHeader = setIfMissingOrDifferent(headerXSessionId, canonical, () => | ||
| headers.set("x-session-id", canonical) | ||
| ); | ||
| const wroteHeader = wroteSessionIdHeader || wroteXSessionIdHeader; | ||
|
|
||
| const wroteBody = setIfMissingOrDifferent(bodyPromptCacheKey, canonical, () => { | ||
| requestBody.prompt_cache_key = canonical; | ||
| }); | ||
|
|
||
| const wroteMetadata = setIfMissingOrDifferent(metadataSessionId, canonical, () => { | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = canonical; | ||
| }); | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonical); | ||
| } | ||
|
|
||
| return { | ||
| applied: wroteHeader || wroteBody || wroteMetadata, | ||
| action: wroteBody ? "copied_header_to_body" : "noop", | ||
| sessionId: canonical, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| // Case: body prompt_cache_key exists but header missing | ||
| if (bodyPromptCacheKey) { | ||
| const canonical = bodyPromptCacheKey; | ||
|
|
||
| const wroteSessionIdHeader = setIfMissingOrDifferent(headerSessionId, canonical, () => | ||
| headers.set("session_id", canonical) | ||
| ); | ||
| const wroteXSessionIdHeader = setIfMissingOrDifferent(headerXSessionId, canonical, () => | ||
| headers.set("x-session-id", canonical) | ||
| ); | ||
| const wroteHeader = wroteSessionIdHeader || wroteXSessionIdHeader; | ||
|
|
||
| const wroteMetadata = setIfMissingOrDifferent(metadataSessionId, canonical, () => { | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = canonical; | ||
| }); | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonical); | ||
| } | ||
|
|
||
| return { | ||
| applied: wroteHeader || wroteMetadata, | ||
| action: wroteHeader ? "copied_body_to_header" : "noop", | ||
| sessionId: canonical, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| // Case: metadata.session_id exists but both header/body missing | ||
| if (metadataSessionId) { | ||
| const canonical = metadataSessionId; | ||
|
|
||
| const wroteSessionIdHeader = setIfMissingOrDifferent(headerSessionId, canonical, () => | ||
| headers.set("session_id", canonical) | ||
| ); | ||
| const wroteXSessionIdHeader = setIfMissingOrDifferent(headerXSessionId, canonical, () => | ||
| headers.set("x-session-id", canonical) | ||
| ); | ||
| const wroteHeader = wroteSessionIdHeader || wroteXSessionIdHeader; | ||
|
|
||
| const wroteBody = setIfMissingOrDifferent(bodyPromptCacheKey, canonical, () => { | ||
| requestBody.prompt_cache_key = canonical; | ||
| }); | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonical); | ||
| } | ||
|
|
||
| return { | ||
| applied: wroteHeader || wroteBody, | ||
| action: "copied_metadata_to_header_and_body", | ||
| sessionId: canonical, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| // Case: none provided → generate UUID v7 (with Redis-backed fingerprint reuse when possible) | ||
| const { sessionId, redisUsed, redisHit } = | ||
| await CodexSessionIdCompleter.getOrCreateSessionIdFromFingerprint(fingerprint); | ||
|
|
||
| headers.set("session_id", sessionId); | ||
| headers.set("x-session-id", sessionId); | ||
| requestBody.prompt_cache_key = sessionId; | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = sessionId; | ||
|
|
||
| return { | ||
| applied: true, | ||
| action: "generated", | ||
| sessionId, | ||
| fingerprint, | ||
| redis: { used: redisUsed, hit: redisHit }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The complete method is quite long and contains complex conditional logic. To improve readability and maintainability, consider refactoring it by breaking it down into smaller, more focused private helper methods for each of the main cases (e.g., handling mismatches, copying from header to body, generating a new ID). This would make the overall flow easier to follow and the individual pieces easier to test and reason about.
|
|
||
| return { | ||
| applied: true, | ||
| action: "copied_body_to_header", |
There was a problem hiding this comment.
The action name copied_body_to_header is misleading in this context. Here, the canonical ID is derived from a header (session_id or x-session-id), and the logic ensures the session_id header is set if it was missing. A more accurate action name like completed_missing_header would improve clarity. You'll also need to add this to the CodexSessionIdCompletionAction type.
action: "completed_missing_header",| describe("CodexSessionIdCompleter", () => { | ||
| beforeEach(() => { | ||
| mocks.store.clear(); | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| test("completes prompt_cache_key from existing session_id header", async () => { | ||
| const headers = new Headers({ | ||
| session_id: VALID_SESSION_ID, | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = createBaseCodexBody("hello"); | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(true); | ||
| expect(result.action).toBe("copied_header_to_body"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | ||
| }); | ||
|
|
||
| test("completes session_id header from existing prompt_cache_key", async () => { | ||
| const headers = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(true); | ||
| expect(result.action).toBe("copied_body_to_header"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | ||
| }); | ||
|
|
||
| test("noop when both prompt_cache_key and session_id are present and consistent", async () => { | ||
| const headers = new Headers({ | ||
| session_id: VALID_SESSION_ID, | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(false); | ||
| expect(result.action).toBe("noop"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect(body.metadata).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("aligns mismatch by preferring header session_id", async () => { | ||
| const headers = new Headers({ | ||
| session_id: VALID_SESSION_ID, | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: OTHER_VALID_SESSION_ID }; | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(true); | ||
| expect(result.action).toBe("aligned_mismatch"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | ||
| }); | ||
|
|
||
| test("generates UUID v7 when both are missing and reuses via fingerprint", async () => { | ||
| const headers1 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body1 = createBaseCodexBody("hello"); | ||
|
|
||
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | ||
|
|
||
| expect(first.applied).toBe(true); | ||
| expect(first.action).toBe("generated"); | ||
| expect(first.sessionId).toMatch( | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ | ||
| ); | ||
| expect(first.redis.used).toBe(true); | ||
| expect(first.redis.hit).toBe(false); | ||
| expect(body1.prompt_cache_key).toBe(first.sessionId); | ||
| expect(headers1.get("session_id")).toBe(first.sessionId); | ||
|
|
||
| const headers2 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body2 = createBaseCodexBody("hello"); | ||
|
|
||
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | ||
|
|
||
| expect(second.action).toBe("generated"); | ||
| expect(second.redis.used).toBe(true); | ||
| expect(second.redis.hit).toBe(true); | ||
| expect(second.sessionId).toBe(first.sessionId); | ||
| expect(body2.prompt_cache_key).toBe(first.sessionId); | ||
| expect(headers2.get("session_id")).toBe(first.sessionId); | ||
| expect(second.fingerprint).toBe(first.fingerprint); | ||
| }); | ||
|
|
||
| test("treats invalid session_id as missing and generates a new one", async () => { | ||
| const headers = new Headers({ | ||
| session_id: "short", | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = createBaseCodexBody("hello"); | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.action).toBe("generated"); | ||
| expect(headers.get("session_id")).not.toBe("short"); | ||
| expect(body.prompt_cache_key).toBe(headers.get("session_id")); | ||
| }); | ||
|
|
||
| test("falls back to local generation when Redis is unavailable", async () => { | ||
| mocks.getRedisClient.mockReturnValueOnce(null); | ||
| mocks.getRedisClient.mockReturnValueOnce(null); | ||
|
|
||
| const headers1 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body1 = createBaseCodexBody("hello"); | ||
|
|
||
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | ||
|
|
||
| const headers2 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body2 = createBaseCodexBody("hello"); | ||
|
|
||
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | ||
|
|
||
| expect(first.redis.used).toBe(false); | ||
| expect(second.redis.used).toBe(false); | ||
| expect(first.sessionId).not.toBeNull(); | ||
| expect(second.sessionId).not.toBeNull(); | ||
| expect(second.sessionId).not.toBe(first.sessionId); | ||
| }); | ||
| }); | ||
|
|
||
| describe("generateUuidV7", () => { | ||
| test("returns UUID v7 string format", () => { | ||
| const id = generateUuidV7(); | ||
| expect(id).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite is comprehensive, which is great! However, there are a couple of edge cases handled in CodexSessionIdCompleter.complete that are not explicitly tested:
- When
metadata.session_idis the only session identifier present in the request. - When
x-session-idis present and consistent withprompt_cache_key, butsession_idis missing.
Adding tests for these scenarios would make the test suite even more robust and ensure all logic paths are covered.
Here's an example for the first case:
test("completes from metadata.session_id when other sources are missing", async () => {
const headers = new Headers({
"user-agent": "codex_cli_rs/0.50.0",
"x-real-ip": "1.2.3.4",
});
const body = {
...createBaseCodexBody("hello"),
metadata: { session_id: VALID_SESSION_ID }
};
const result = await CodexSessionIdCompleter.complete(1, headers, body);
expect(result.applied).toBe(true);
expect(result.action).toBe("copied_metadata_to_header_and_body");
expect(headers.get("session_id")).toBe(VALID_SESSION_ID);
expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID);
expect(body.prompt_cache_key).toBe(VALID_SESSION_ID);
});| redis: { used: boolean; hit: boolean }; | ||
| } | ||
|
|
||
| const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10); |
There was a problem hiding this comment.
the parseInt function returns NaN if process.env.SESSION_TTL is set to a non-numeric string (e.g., "invalid"). This NaN value will be passed to Redis setex/expire operations, which could cause silent failures or unexpected behavior
| const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10); | |
| const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10) || 300; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 24:24
Comment:
the `parseInt` function returns `NaN` if `process.env.SESSION_TTL` is set to a non-numeric string (e.g., "invalid"). This `NaN` value will be passed to Redis `setex`/`expire` operations, which could cause silent failures or unexpected behavior
```suggestion
const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10) || 300;
```
How can I resolve this? If you propose a fix, please make it concise.| if (headerSessionId || headerXSessionId) { | ||
| const canonical = headerSessionId ?? headerXSessionId; | ||
| if (!canonical) { | ||
| return { | ||
| applied: false, | ||
| action: "noop", | ||
| sessionId: null, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
the check if (!canonical) on line 291 is unreachable because the outer condition if (headerSessionId || headerXSessionId) guarantees at least one is truthy, making canonical = headerSessionId ?? headerXSessionId always truthy. This dead code block will never execute and returns a misleading result
| if (headerSessionId || headerXSessionId) { | |
| const canonical = headerSessionId ?? headerXSessionId; | |
| if (!canonical) { | |
| return { | |
| applied: false, | |
| action: "noop", | |
| sessionId: null, | |
| fingerprint, | |
| redis: { used: false, hit: false }, | |
| }; | |
| } | |
| // Case: header exists but body missing | |
| if (headerSessionId || headerXSessionId) { | |
| const canonical = headerSessionId ?? headerXSessionId!; // Non-null assertion safe here |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 289:299
Comment:
the check `if (!canonical)` on line 291 is unreachable because the outer condition `if (headerSessionId || headerXSessionId)` guarantees at least one is truthy, making `canonical = headerSessionId ?? headerXSessionId` always truthy. This dead code block will never execute and returns a misleading result
```suggestion
// Case: header exists but body missing
if (headerSessionId || headerXSessionId) {
const canonical = headerSessionId ?? headerXSessionId!; // Non-null assertion safe here
```
How can I resolve this? If you propose a fix, please make it concise.| // Already consistent, no-op (but keep mapping warm if possible) | ||
| if (canonicalHeader) { | ||
| // Some clients may only send x-session-id without session_id. | ||
| // If prompt_cache_key is present, we treat it as having the session id and fill session_id. | ||
| if (!headerSessionId) { | ||
| headers.set("session_id", canonicalHeader); | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = canonicalHeader; | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
|
|
||
| return { | ||
| applied: true, | ||
| action: "copied_body_to_header", | ||
| sessionId: canonicalHeader, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
| } |
There was a problem hiding this comment.
the comment on line 252 says "Already consistent, no-op" but the code below (lines 254-271) is not actually a no-op when headerSessionId is missing. It sets the session_id header and metadata, then returns action: "copied_body_to_header" which contradicts the "no-op" intent. Additionally, when headerSessionId exists (lines 273-277), metadata is NOT set, creating inconsistent behavior where metadata is sometimes populated and sometimes not in the "noop" path
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 252:277
Comment:
the comment on line 252 says "Already consistent, no-op" but the code below (lines 254-271) is not actually a no-op when `headerSessionId` is missing. It sets the `session_id` header and metadata, then returns `action: "copied_body_to_header"` which contradicts the "no-op" intent. Additionally, when `headerSessionId` exists (lines 273-277), metadata is NOT set, creating inconsistent behavior where metadata is sometimes populated and sometimes not in the "noop" path
How can I resolve this? If you propose a fix, please make it concise.| private static async getOrCreateSessionIdFromFingerprint( | ||
| fingerprint: string | null | ||
| ): Promise<{ sessionId: string; redisUsed: boolean; redisHit: boolean }> { | ||
| const redis = getRedisClient(); | ||
| if (!redis || redis.status !== "ready" || !fingerprint) { | ||
| return { sessionId: generateUuidV7(), redisUsed: false, redisHit: false }; | ||
| } | ||
|
|
||
| const key = getRedisKey(fingerprint); | ||
| try { | ||
| const existing = await redis.get(key); | ||
| const normalized = normalizeCodexSessionId(existing); | ||
| if (normalized) { | ||
| await redis.expire(key, SESSION_TTL_SECONDS); | ||
| return { sessionId: normalized, redisUsed: true, redisHit: true }; | ||
| } | ||
|
|
||
| const created = generateUuidV7(); | ||
| await redis.setex(key, SESSION_TTL_SECONDS, created); | ||
| return { sessionId: created, redisUsed: true, redisHit: false }; | ||
| } catch (error) { | ||
| logger.warn("[CodexSessionIdCompleter] Redis error, falling back to local UUID", { | ||
| error, | ||
| }); | ||
| return { sessionId: generateUuidV7(), redisUsed: false, redisHit: false }; | ||
| } |
There was a problem hiding this comment.
this function has a race condition between checking if a session exists (line 419) and creating a new one (line 427). If two concurrent requests with the same fingerprint arrive simultaneously, both could pass the existence check and generate different UUIDs, breaking session continuity. Consider using Redis SET NX (set if not exists) with GET option, or implement proper distributed locking
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 409:434
Comment:
this function has a race condition between checking if a session exists (line 419) and creating a new one (line 427). If two concurrent requests with the same fingerprint arrive simultaneously, both could pass the existence check and generate different UUIDs, breaking session continuity. Consider using Redis `SET NX` (set if not exists) with `GET` option, or implement proper distributed locking
How can I resolve this? If you propose a fix, please make it concise.| function calculateFingerprint( | ||
| keyId: number, | ||
| headers: Headers, | ||
| requestBody: Record<string, unknown> | ||
| ) { | ||
| const ip = getClientIp(headers); | ||
| const userAgent = headers.get("user-agent"); | ||
| const initialHash = calculateInitialMessageHash(requestBody); | ||
|
|
||
| const parts = [`key:${keyId}`, ip ? `ip:${ip}` : null, userAgent ? `ua:${userAgent}` : null]; | ||
| if (initialHash) { | ||
| parts.push(`init:${initialHash}`); | ||
| } | ||
|
|
||
| const filtered = parts.filter(Boolean); | ||
| if (filtered.length === 0) return null; | ||
|
|
||
| return crypto.createHash("sha256").update(filtered.join("|"), "utf8").digest("hex").slice(0, 32); | ||
| } |
There was a problem hiding this comment.
the fingerprint calculation relies heavily on client-controlled data (x-forwarded-for, user-agent) which can be easily spoofed. An attacker could hijack another user's session by:
- Spoofing the x-forwarded-for header to match the victim's IP
- Using the same user-agent string
- Sending the same initial message
This allows the attacker to reuse the victim's cached session ID from Redis. Consider including server-side data like the API key hash or a server-generated nonce in the fingerprint to prevent spoofing
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 109:127
Comment:
the fingerprint calculation relies heavily on client-controlled data (x-forwarded-for, user-agent) which can be easily spoofed. An attacker could hijack another user's session by:
1. Spoofing the x-forwarded-for header to match the victim's IP
2. Using the same user-agent string
3. Sending the same initial message
This allows the attacker to reuse the victim's cached session ID from Redis. Consider including server-side data like the API key hash or a server-generated nonce in the fingerprint to prevent spoofing
How can I resolve this? If you propose a fix, please make it concise.| import "server-only"; | ||
|
|
||
| import crypto from "node:crypto"; | ||
| import { normalizeCodexSessionId } from "@/app/v1/_lib/codex/session-extractor"; | ||
| import { logger } from "@/lib/logger"; | ||
| import { getRedisClient } from "@/lib/redis"; | ||
|
|
||
| export type CodexSessionIdCompletionAction = | ||
| | "noop" | ||
| | "copied_header_to_body" |
There was a problem hiding this comment.
the CodexSessionIdCompleter class is implemented but is never actually integrated into the request handling pipeline. Searching the codebase shows that extractCodexSessionId is used in SessionManager.extractClientSessionId, but there are no calls to CodexSessionIdCompleter.complete() anywhere. This means the auto-completion feature is not functional - session IDs will never be automatically completed, synchronized, or generated. The feature needs to be integrated into the Codex request handling flow (likely in the chat-completions-handler or similar middleware) before requests are processed
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 1:10
Comment:
the `CodexSessionIdCompleter` class is implemented but is never actually integrated into the request handling pipeline. Searching the codebase shows that `extractCodexSessionId` is used in `SessionManager.extractClientSessionId`, but there are no calls to `CodexSessionIdCompleter.complete()` anywhere. This means the auto-completion feature is not functional - session IDs will never be automatically completed, synchronized, or generated. The feature needs to be integrated into the Codex request handling flow (likely in the chat-completions-handler or similar middleware) before requests are processed
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1 @@ | |||
| ALTER TABLE "system_settings" ADD COLUMN "enable_codex_session_id_completion" boolean DEFAULT true NOT NULL; No newline at end of file | |||
There was a problem hiding this comment.
the database setting enable_codex_session_id_completion is added but is never checked or used anywhere in the codebase. Searching for this setting reveals it only exists in the migration files. This means:
- The feature cannot be disabled even though a toggle was created for it
- The setting serves no purpose in its current state
- There's likely missing code that should check this setting before calling
CodexSessionIdCompleter.complete()
The integration code should check this setting (probably from system_settings table) before applying session ID completion
Prompt To Fix With AI
This is a comment left during a code review.
Path: drizzle/0054_shiny_quentin_quire.sql
Line: 1:1
Comment:
the database setting `enable_codex_session_id_completion` is added but is never checked or used anywhere in the codebase. Searching for this setting reveals it only exists in the migration files. This means:
1. The feature cannot be disabled even though a toggle was created for it
2. The setting serves no purpose in its current state
3. There's likely missing code that should check this setting before calling `CodexSessionIdCompleter.complete()`
The integration code should check this setting (probably from system_settings table) before applying session ID completion
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/app/v1/_lib/codex/session-completer.ts:
- Line 24: SESSION_TTL_SECONDS may become NaN or non-positive from parseInt and
later break Redis expire/setex calls; validate and sanitize it when defined:
parse the env value into a number, check Number.isFinite/Number.isNaN and that
it is >= 1, and if invalid fall back to a safe default (e.g., 300) or clamp to a
minimum (1); update the constant SESSION_TTL_SECONDS to use this validated value
so Redis calls never receive NaN/<=0.
In @tests/unit/codex/session-completer.test.ts:
- Around line 52-206: Add a unit test to cover the symmetric case where
session_id is present but x-session-id is missing: create Headers with
session_id: VALID_SESSION_ID but no "x-session-id", call
CodexSessionIdCompleter.complete(1, headers, body) (use
createBaseCodexBody("hello") for body), then assert result.applied is true,
result.action is "copied_header_to_body" (or the expected action for filling
headers), headers.get("x-session-id") equals VALID_SESSION_ID,
headers.get("session_id") remains VALID_SESSION_ID, and body.prompt_cache_key
and body.metadata.session_id are set to VALID_SESSION_ID to ensure the completer
fills the missing x-session-id from session_id.
🧹 Nitpick comments (1)
src/app/v1/_lib/codex/session-completer.ts (1)
252-272: action 命名可能误导(从 x-session-id 补齐 session_id,并非“body → header”)此处
canonicalHeader可能来自x-session-id,但返回action: "copied_body_to_header";如果 action 用于统计/观测,建议更精确(例如新增"copied_x_header_to_session_header"或复用"aligned_mismatch"/"copied_header_to_body"的语义)。
(不影响主流程正确性,但会影响可观测性解读)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (4)
drizzle/0054_shiny_quentin_quire.sqldrizzle/meta/0054_snapshot.jsonsrc/app/v1/_lib/codex/session-completer.tstests/unit/codex/session-completer.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use emoji characters in any code, comments, or string literals
Files:
tests/unit/codex/session-completer.test.tssrc/app/v1/_lib/codex/session-completer.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
All new features must have unit test coverage of at least 80%
Files:
tests/unit/codex/session-completer.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text
Use path alias@/to reference files in./src/directory
Format code with Biome using: double quotes, trailing commas, 2-space indent, 100 character line width
Files:
tests/unit/codex/session-completer.test.tssrc/app/v1/_lib/codex/session-completer.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer named exports over default exports
Files:
tests/unit/codex/session-completer.test.tssrc/app/v1/_lib/codex/session-completer.ts
tests/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest for unit testing and happy-dom for DOM testing
Files:
tests/unit/codex/session-completer.test.ts
🧠 Learnings (3)
📚 Learning: 2026-01-10T17:53:25.066Z
Learnt from: CR
Repo: ding113/claude-code-hub PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:53:25.066Z
Learning: Applies to src/drizzle/schema.ts : Modify database schema in `src/drizzle/schema.ts`, then run `bun run db:generate` to generate migrations. Never create SQL migration files manually
Applied to files:
drizzle/meta/0054_snapshot.json
📚 Learning: 2026-01-10T17:53:25.066Z
Learnt from: CR
Repo: ding113/claude-code-hub PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:53:25.066Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : All new features must have unit test coverage of at least 80%
Applied to files:
tests/unit/codex/session-completer.test.ts
📚 Learning: 2026-01-05T03:01:39.354Z
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 539
File: src/types/user.ts:158-170
Timestamp: 2026-01-05T03:01:39.354Z
Learning: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited (e.g., matches DB schema or special UI logic) and undefined signifies 'inherit default'. This pattern should be consistently reflected in type definitions across related fields to preserve semantic clarity between database constraints and UI behavior.
Applied to files:
src/app/v1/_lib/codex/session-completer.ts
🧬 Code graph analysis (1)
tests/unit/codex/session-completer.test.ts (1)
src/app/v1/_lib/codex/session-completer.ts (2)
CodexSessionIdCompleter(201-450)generateUuidV7(142-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pr-review
- GitHub Check: check-codex-status
- GitHub Check: pr-description
- GitHub Check: 🌐 API Tests
- GitHub Check: Docker Build Test
- GitHub Check: dev-build-deploy
🔇 Additional comments (6)
drizzle/0054_shiny_quentin_quire.sql (2)
1-1: 确认迁移生成流程符合仓库约定(避免手写 SQL 迁移)这条
ALTER TABLE ... ADD COLUMN ... DEFAULT true NOT NULL在 Postgres 语义上没问题;但根据仓库 learnings,建议确认该文件是由bun run db:generate生成、且src/drizzle/schema.ts已同步更新,避免后续漂移。Based on learnings, ...
1-1: 默认值选择合理(开启为 true)默认开启能保持向后兼容(老客户端不传 session id 也能获得连续性),符合 PR 目标描述。
drizzle/meta/0054_snapshot.json (1)
1952-1958: 快照与迁移保持一致
system_settings.enable_codex_session_id_completion的类型/默认值/非空约束与迁移一致,作为 drizzle 元数据快照也符合预期。src/app/v1/_lib/codex/session-completer.ts (2)
409-435: Redis API/状态判断请与 ioredis 版本能力对齐这里依赖
redis.status === "ready"以及get/setex/expire;若getRedisClient()返回的是 ioredis client,通常成立,但建议确认封装层不会返回不同实现或 status 取值不同,否则会导致意外走本地 UUID 分支。
142-169: UUID v7 生成实现清晰且符合变体/版本位设置时间戳写入 + 随机位填充 + version/variant bitmask 的处理方式合理;测试也覆盖了格式校验。
tests/unit/codex/session-completer.test.ts (1)
1-212: 请跑一次覆盖率校验,确认新特性单测覆盖 >= 80%已具备较完整的行为覆盖(含 Redis 命中/未命中/不可用回退),但仍建议在 CI/本地用 vitest coverage 明确验证达到仓库要求。As per coding guidelines, ...
#!/bin/bash set -euo pipefail # 期望:tests/unit/codex/session-completer.test.ts 覆盖率达到仓库阈值(>=80%) bunx vitest run --coverage tests/unit/codex/session-completer.test.ts
| redis: { used: boolean; hit: boolean }; | ||
| } | ||
|
|
||
| const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10); |
There was a problem hiding this comment.
为 SESSION_TTL_SECONDS 增加 NaN/边界保护,避免 Redis 调用异常
parseInt(...) 可能得到 NaN(或 <= 0),随后传给 expire/setex 会导致运行期错误或不可预测行为。
建议修改
-const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10);
+const SESSION_TTL_SECONDS_RAW = Number.parseInt(process.env.SESSION_TTL || "300", 10);
+const SESSION_TTL_SECONDS =
+ Number.isFinite(SESSION_TTL_SECONDS_RAW) && SESSION_TTL_SECONDS_RAW > 0
+ ? SESSION_TTL_SECONDS_RAW
+ : 300;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10); | |
| const SESSION_TTL_SECONDS_RAW = Number.parseInt( | |
| process.env.SESSION_TTL || "300", | |
| 10, | |
| ); | |
| const SESSION_TTL_SECONDS = | |
| Number.isFinite(SESSION_TTL_SECONDS_RAW) && | |
| SESSION_TTL_SECONDS_RAW > 0 | |
| ? SESSION_TTL_SECONDS_RAW | |
| : 300; |
🤖 Prompt for AI Agents
In @src/app/v1/_lib/codex/session-completer.ts at line 24, SESSION_TTL_SECONDS
may become NaN or non-positive from parseInt and later break Redis expire/setex
calls; validate and sanitize it when defined: parse the env value into a number,
check Number.isFinite/Number.isNaN and that it is >= 1, and if invalid fall back
to a safe default (e.g., 300) or clamp to a minimum (1); update the constant
SESSION_TTL_SECONDS to use this validated value so Redis calls never receive
NaN/<=0.
| // Case: both prompt_cache_key and any session header are present | ||
| if (bodyPromptCacheKey && (headerSessionId || headerXSessionId)) { | ||
| const canonicalHeader = headerSessionId ?? headerXSessionId; | ||
|
|
||
| // If they differ, align to header to avoid ambiguity | ||
| if (canonicalHeader && canonicalHeader !== bodyPromptCacheKey) { | ||
| const wroteSessionIdHeader = setIfMissingOrDifferent(headerSessionId, canonicalHeader, () => | ||
| headers.set("session_id", canonicalHeader) | ||
| ); | ||
| const wroteXSessionIdHeader = setIfMissingOrDifferent( | ||
| headerXSessionId, | ||
| canonicalHeader, | ||
| () => headers.set("x-session-id", canonicalHeader) | ||
| ); | ||
| const wroteHeader = wroteSessionIdHeader || wroteXSessionIdHeader; | ||
|
|
||
| const wroteBody = setIfMissingOrDifferent(bodyPromptCacheKey, canonicalHeader, () => { | ||
| requestBody.prompt_cache_key = canonicalHeader; | ||
| }); | ||
|
|
||
| const wroteMetadata = setIfMissingOrDifferent(metadataSessionId, canonicalHeader, () => { | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = canonicalHeader; | ||
| }); | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
|
|
||
| return { | ||
| applied: wroteHeader || wroteBody || wroteMetadata, | ||
| action: "aligned_mismatch", | ||
| sessionId: canonicalHeader, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| // Already consistent, no-op (but keep mapping warm if possible) | ||
| if (canonicalHeader) { | ||
| // Some clients may only send x-session-id without session_id. | ||
| // If prompt_cache_key is present, we treat it as having the session id and fill session_id. | ||
| if (!headerSessionId) { | ||
| headers.set("session_id", canonicalHeader); | ||
| const metadata = ensureMetadataObject(requestBody); | ||
| metadata.session_id = canonicalHeader; | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
|
|
||
| return { | ||
| applied: true, | ||
| action: "copied_body_to_header", | ||
| sessionId: canonicalHeader, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } | ||
|
|
||
| if (fingerprint) { | ||
| await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| applied: false, | ||
| action: "noop", | ||
| sessionId: canonicalHeader ?? bodyPromptCacheKey, | ||
| fingerprint, | ||
| redis: { used: false, hit: false }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
“已一致”分支未补齐缺失的 x-session-id,导致“跨 header 同步”不完整
当 session_id + prompt_cache_key 同时存在且一致、但 x-session-id 缺失时,目前会直接 noop 返回,x-session-id 不会被补齐(与 PR 目标“同步 session_id 与 x-session-id”不一致)。
建议修改
// Already consistent, no-op (but keep mapping warm if possible)
if (canonicalHeader) {
// Some clients may only send x-session-id without session_id.
// If prompt_cache_key is present, we treat it as having the session id and fill session_id.
if (!headerSessionId) {
headers.set("session_id", canonicalHeader);
const metadata = ensureMetadataObject(requestBody);
metadata.session_id = canonicalHeader;
@@
return {
applied: true,
action: "copied_body_to_header",
sessionId: canonicalHeader,
fingerprint,
redis: { used: false, hit: false },
};
}
+
+ // Some clients may only send session_id without x-session-id.
+ if (!headerXSessionId) {
+ headers.set("x-session-id", canonicalHeader);
+ if (fingerprint) {
+ await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader);
+ }
+ return {
+ applied: true,
+ action: "copied_header_to_body",
+ sessionId: canonicalHeader,
+ fingerprint,
+ redis: { used: false, hit: false },
+ };
+ }
if (fingerprint) {
await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader);
}
}| describe("CodexSessionIdCompleter", () => { | ||
| beforeEach(() => { | ||
| mocks.store.clear(); | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| test("completes prompt_cache_key from existing session_id header", async () => { | ||
| const headers = new Headers({ | ||
| session_id: VALID_SESSION_ID, | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = createBaseCodexBody("hello"); | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(true); | ||
| expect(result.action).toBe("copied_header_to_body"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | ||
| }); | ||
|
|
||
| test("completes session_id header from existing prompt_cache_key", async () => { | ||
| const headers = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(true); | ||
| expect(result.action).toBe("copied_body_to_header"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | ||
| }); | ||
|
|
||
| test("noop when both prompt_cache_key and session_id are present and consistent", async () => { | ||
| const headers = new Headers({ | ||
| session_id: VALID_SESSION_ID, | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(false); | ||
| expect(result.action).toBe("noop"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect(body.metadata).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("aligns mismatch by preferring header session_id", async () => { | ||
| const headers = new Headers({ | ||
| session_id: VALID_SESSION_ID, | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: OTHER_VALID_SESSION_ID }; | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.applied).toBe(true); | ||
| expect(result.action).toBe("aligned_mismatch"); | ||
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | ||
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | ||
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | ||
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | ||
| }); | ||
|
|
||
| test("generates UUID v7 when both are missing and reuses via fingerprint", async () => { | ||
| const headers1 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body1 = createBaseCodexBody("hello"); | ||
|
|
||
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | ||
|
|
||
| expect(first.applied).toBe(true); | ||
| expect(first.action).toBe("generated"); | ||
| expect(first.sessionId).toMatch( | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ | ||
| ); | ||
| expect(first.redis.used).toBe(true); | ||
| expect(first.redis.hit).toBe(false); | ||
| expect(body1.prompt_cache_key).toBe(first.sessionId); | ||
| expect(headers1.get("session_id")).toBe(first.sessionId); | ||
|
|
||
| const headers2 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body2 = createBaseCodexBody("hello"); | ||
|
|
||
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | ||
|
|
||
| expect(second.action).toBe("generated"); | ||
| expect(second.redis.used).toBe(true); | ||
| expect(second.redis.hit).toBe(true); | ||
| expect(second.sessionId).toBe(first.sessionId); | ||
| expect(body2.prompt_cache_key).toBe(first.sessionId); | ||
| expect(headers2.get("session_id")).toBe(first.sessionId); | ||
| expect(second.fingerprint).toBe(first.fingerprint); | ||
| }); | ||
|
|
||
| test("treats invalid session_id as missing and generates a new one", async () => { | ||
| const headers = new Headers({ | ||
| session_id: "short", | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body = createBaseCodexBody("hello"); | ||
|
|
||
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | ||
|
|
||
| expect(result.action).toBe("generated"); | ||
| expect(headers.get("session_id")).not.toBe("short"); | ||
| expect(body.prompt_cache_key).toBe(headers.get("session_id")); | ||
| }); | ||
|
|
||
| test("falls back to local generation when Redis is unavailable", async () => { | ||
| mocks.getRedisClient.mockReturnValueOnce(null); | ||
| mocks.getRedisClient.mockReturnValueOnce(null); | ||
|
|
||
| const headers1 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body1 = createBaseCodexBody("hello"); | ||
|
|
||
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | ||
|
|
||
| const headers2 = new Headers({ | ||
| "user-agent": "codex_cli_rs/0.50.0", | ||
| "x-real-ip": "1.2.3.4", | ||
| }); | ||
| const body2 = createBaseCodexBody("hello"); | ||
|
|
||
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | ||
|
|
||
| expect(first.redis.used).toBe(false); | ||
| expect(second.redis.used).toBe(false); | ||
| expect(first.sessionId).not.toBeNull(); | ||
| expect(second.sessionId).not.toBeNull(); | ||
| expect(second.sessionId).not.toBe(first.sessionId); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
补一条用例覆盖“缺失 x-session-id 也要补齐”的对称场景
当前覆盖了“缺失 session_id 时从 x-session-id 补齐”的隐含路径,但缺少“缺失 x-session-id 时从 session_id 补齐”。建议加一条回归用例,能直接锁定 complete() 的“已一致但 header 不全”问题。
建议新增测试
describe("CodexSessionIdCompleter", () => {
@@
test("noop when both prompt_cache_key and session_id are present and consistent", async () => {
@@
});
+
+ test("fills missing x-session-id when session_id and prompt_cache_key are consistent", async () => {
+ const headers = new Headers({
+ session_id: VALID_SESSION_ID,
+ "user-agent": "codex_cli_rs/0.50.0",
+ "x-real-ip": "1.2.3.4",
+ });
+ const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID };
+
+ const result = await CodexSessionIdCompleter.complete(1, headers, body);
+
+ expect(result.applied).toBe(true);
+ expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID);
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("CodexSessionIdCompleter", () => { | |
| beforeEach(() => { | |
| mocks.store.clear(); | |
| vi.clearAllMocks(); | |
| }); | |
| test("completes prompt_cache_key from existing session_id header", async () => { | |
| const headers = new Headers({ | |
| session_id: VALID_SESSION_ID, | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = createBaseCodexBody("hello"); | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(true); | |
| expect(result.action).toBe("copied_header_to_body"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | |
| }); | |
| test("completes session_id header from existing prompt_cache_key", async () => { | |
| const headers = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(true); | |
| expect(result.action).toBe("copied_body_to_header"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | |
| }); | |
| test("noop when both prompt_cache_key and session_id are present and consistent", async () => { | |
| const headers = new Headers({ | |
| session_id: VALID_SESSION_ID, | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(false); | |
| expect(result.action).toBe("noop"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect(body.metadata).toBeUndefined(); | |
| }); | |
| test("aligns mismatch by preferring header session_id", async () => { | |
| const headers = new Headers({ | |
| session_id: VALID_SESSION_ID, | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: OTHER_VALID_SESSION_ID }; | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(true); | |
| expect(result.action).toBe("aligned_mismatch"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | |
| }); | |
| test("generates UUID v7 when both are missing and reuses via fingerprint", async () => { | |
| const headers1 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body1 = createBaseCodexBody("hello"); | |
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | |
| expect(first.applied).toBe(true); | |
| expect(first.action).toBe("generated"); | |
| expect(first.sessionId).toMatch( | |
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ | |
| ); | |
| expect(first.redis.used).toBe(true); | |
| expect(first.redis.hit).toBe(false); | |
| expect(body1.prompt_cache_key).toBe(first.sessionId); | |
| expect(headers1.get("session_id")).toBe(first.sessionId); | |
| const headers2 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body2 = createBaseCodexBody("hello"); | |
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | |
| expect(second.action).toBe("generated"); | |
| expect(second.redis.used).toBe(true); | |
| expect(second.redis.hit).toBe(true); | |
| expect(second.sessionId).toBe(first.sessionId); | |
| expect(body2.prompt_cache_key).toBe(first.sessionId); | |
| expect(headers2.get("session_id")).toBe(first.sessionId); | |
| expect(second.fingerprint).toBe(first.fingerprint); | |
| }); | |
| test("treats invalid session_id as missing and generates a new one", async () => { | |
| const headers = new Headers({ | |
| session_id: "short", | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = createBaseCodexBody("hello"); | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.action).toBe("generated"); | |
| expect(headers.get("session_id")).not.toBe("short"); | |
| expect(body.prompt_cache_key).toBe(headers.get("session_id")); | |
| }); | |
| test("falls back to local generation when Redis is unavailable", async () => { | |
| mocks.getRedisClient.mockReturnValueOnce(null); | |
| mocks.getRedisClient.mockReturnValueOnce(null); | |
| const headers1 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body1 = createBaseCodexBody("hello"); | |
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | |
| const headers2 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body2 = createBaseCodexBody("hello"); | |
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | |
| expect(first.redis.used).toBe(false); | |
| expect(second.redis.used).toBe(false); | |
| expect(first.sessionId).not.toBeNull(); | |
| expect(second.sessionId).not.toBeNull(); | |
| expect(second.sessionId).not.toBe(first.sessionId); | |
| }); | |
| }); | |
| describe("CodexSessionIdCompleter", () => { | |
| beforeEach(() => { | |
| mocks.store.clear(); | |
| vi.clearAllMocks(); | |
| }); | |
| test("completes prompt_cache_key from existing session_id header", async () => { | |
| const headers = new Headers({ | |
| session_id: VALID_SESSION_ID, | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = createBaseCodexBody("hello"); | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(true); | |
| expect(result.action).toBe("copied_header_to_body"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | |
| }); | |
| test("completes session_id header from existing prompt_cache_key", async () => { | |
| const headers = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(true); | |
| expect(result.action).toBe("copied_body_to_header"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | |
| }); | |
| test("noop when both prompt_cache_key and session_id are present and consistent", async () => { | |
| const headers = new Headers({ | |
| session_id: VALID_SESSION_ID, | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(false); | |
| expect(result.action).toBe("noop"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect(body.metadata).toBeUndefined(); | |
| }); | |
| test("fills missing x-session-id when session_id and prompt_cache_key are consistent", async () => { | |
| const headers = new Headers({ | |
| session_id: VALID_SESSION_ID, | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: VALID_SESSION_ID }; | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(true); | |
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | |
| }); | |
| test("aligns mismatch by preferring header session_id", async () => { | |
| const headers = new Headers({ | |
| session_id: VALID_SESSION_ID, | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = { ...createBaseCodexBody("hello"), prompt_cache_key: OTHER_VALID_SESSION_ID }; | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.applied).toBe(true); | |
| expect(result.action).toBe("aligned_mismatch"); | |
| expect(headers.get("session_id")).toBe(VALID_SESSION_ID); | |
| expect(headers.get("x-session-id")).toBe(VALID_SESSION_ID); | |
| expect(body.prompt_cache_key).toBe(VALID_SESSION_ID); | |
| expect((body.metadata as any)?.session_id).toBe(VALID_SESSION_ID); | |
| }); | |
| test("generates UUID v7 when both are missing and reuses via fingerprint", async () => { | |
| const headers1 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body1 = createBaseCodexBody("hello"); | |
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | |
| expect(first.applied).toBe(true); | |
| expect(first.action).toBe("generated"); | |
| expect(first.sessionId).toMatch( | |
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ | |
| ); | |
| expect(first.redis.used).toBe(true); | |
| expect(first.redis.hit).toBe(false); | |
| expect(body1.prompt_cache_key).toBe(first.sessionId); | |
| expect(headers1.get("session_id")).toBe(first.sessionId); | |
| const headers2 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body2 = createBaseCodexBody("hello"); | |
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | |
| expect(second.action).toBe("generated"); | |
| expect(second.redis.used).toBe(true); | |
| expect(second.redis.hit).toBe(true); | |
| expect(second.sessionId).toBe(first.sessionId); | |
| expect(body2.prompt_cache_key).toBe(first.sessionId); | |
| expect(headers2.get("session_id")).toBe(first.sessionId); | |
| expect(second.fingerprint).toBe(first.fingerprint); | |
| }); | |
| test("treats invalid session_id as missing and generates a new one", async () => { | |
| const headers = new Headers({ | |
| session_id: "short", | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body = createBaseCodexBody("hello"); | |
| const result = await CodexSessionIdCompleter.complete(1, headers, body); | |
| expect(result.action).toBe("generated"); | |
| expect(headers.get("session_id")).not.toBe("short"); | |
| expect(body.prompt_cache_key).toBe(headers.get("session_id")); | |
| }); | |
| test("falls back to local generation when Redis is unavailable", async () => { | |
| mocks.getRedisClient.mockReturnValueOnce(null); | |
| mocks.getRedisClient.mockReturnValueOnce(null); | |
| const headers1 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body1 = createBaseCodexBody("hello"); | |
| const first = await CodexSessionIdCompleter.complete(1, headers1, body1); | |
| const headers2 = new Headers({ | |
| "user-agent": "codex_cli_rs/0.50.0", | |
| "x-real-ip": "1.2.3.4", | |
| }); | |
| const body2 = createBaseCodexBody("hello"); | |
| const second = await CodexSessionIdCompleter.complete(1, headers2, body2); | |
| expect(first.redis.used).toBe(false); | |
| expect(second.redis.used).toBe(false); | |
| expect(first.sessionId).not.toBeNull(); | |
| expect(second.sessionId).not.toBeNull(); | |
| expect(second.sessionId).not.toBe(first.sessionId); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @tests/unit/codex/session-completer.test.ts around lines 52 - 206, Add a unit
test to cover the symmetric case where session_id is present but x-session-id is
missing: create Headers with session_id: VALID_SESSION_ID but no "x-session-id",
call CodexSessionIdCompleter.complete(1, headers, body) (use
createBaseCodexBody("hello") for body), then assert result.applied is true,
result.action is "copied_header_to_body" (or the expected action for filling
headers), headers.get("x-session-id") equals VALID_SESSION_ID,
headers.get("session_id") remains VALID_SESSION_ID, and body.prompt_cache_key
and body.metadata.session_id are set to VALID_SESSION_ID to ensure the completer
fills the missing x-session-id from session_id.
|
|
||
| const fingerprint = calculateFingerprint(keyId, headers, requestBody); | ||
|
|
||
| // Case: both prompt_cache_key and any session header are present |
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] Feature is not integrated - CodexSessionIdCompleter.complete() is never called
Why this is a problem: The entire feature is non-functional. The CodexSessionIdCompleter.complete() method is never called in any request handler, meaning session ID completion will never execute. This makes the feature completely dead code.
Evidence:
- Searched entire codebase for
CodexSessionIdCompleterusage - only found in test file - Not integrated into
chat-completions-handler.tsor any guard in the proxy pipeline - The guard pipeline documented in CLAUDE.md (auth → sensitive → client → model → version → probe → session → warmup → requestFilter → rateLimit → provider → providerRequestFilter → messageContext) has no codex session completion step
Suggested fix:
// In src/app/v1/_lib/codex/chat-completions-handler.ts or appropriate guard
// Add integration before forwarding to provider:
import { CodexSessionIdCompleter } from "./session-completer";
// Before proxy forwarding:
if (session.request.apiType === "codex") {
const completionResult = await CodexSessionIdCompleter.complete(
session.key.id,
session.headers,
session.request.message
);
if (completionResult.applied) {
logger.debug("[Codex] Session ID completion applied", {
action: completionResult.action,
sessionId: completionResult.sessionId
});
}
}| @@ -0,0 +1 @@ | |||
| ALTER TABLE "system_settings" ADD COLUMN "enable_codex_session_id_completion" boolean DEFAULT true NOT NULL; No newline at end of file | |||
There was a problem hiding this comment.
[CRITICAL] [STANDARD-VIOLATION] Database setting added but never used
Why this is a problem: The migration adds enable_codex_session_id_completion to system_settings, but this setting is never checked in the code. This violates CLAUDE.md's requirement that migrations must be generated from schema changes, and indicates the toggle functionality is non-functional.
Evidence:
- CLAUDE.md line 54-62: "IMPORTANT: Never create SQL migration files manually. Always follow this workflow: 1. Modify schema - Edit src/drizzle/schema.ts"
- Searched codebase for
enable_codex_session_id_completion- only found in migration and snapshot, never used in TypeScript code - The
CodexSessionIdCompleter.complete()method has no conditional check for this setting
Suggested fix:
// 1. First add to src/drizzle/schema.ts:
export const systemSettings = pgTable('system_settings', {
// ... existing fields
enableCodexSessionIdCompletion: boolean('enable_codex_session_id_completion')
.notNull()
.default(true),
});
// 2. Then in session-completer.ts:
import { systemSettingsRepo } from "@/repository/system-settings";
static async complete(
keyId: number,
headers: Headers,
requestBody: Record<string, unknown>
): Promise<CodexSessionIdCompletionResult> {
const settings = await systemSettingsRepo.getSettings();
if (!settings.enableCodexSessionIdCompletion) {
return {
applied: false,
action: "noop",
sessionId: null,
fingerprint: null,
redis: { used: false, hit: false },
};
}
// ... rest of existing logic
}| const existing = await redis.get(key); | ||
| const normalized = normalizeCodexSessionId(existing); | ||
| if (normalized) { | ||
| await redis.expire(key, SESSION_TTL_SECONDS); |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Race condition in Redis fingerprint-to-session mapping
Why this is a problem: Lines 422-428 have a classic check-then-set race condition. When multiple concurrent requests with the same fingerprint arrive (common for CLI users), they will race between the GET and SETEX operations, potentially generating different UUIDs for the same session fingerprint.
Evidence:
const existing = await redis.get(key); // Request A and B both get null
const normalized = normalizeCodexSessionId(existing);
if (normalized) {
// ...
}
// Request A generates UUID-1
const created = generateUuidV7();
// Request B generates UUID-2
await redis.setex(key, SESSION_TTL_SECONDS, created); // Last writer winsThe second request will overwrite the first, breaking session continuity.
Suggested fix:
private static async getOrCreateSessionIdFromFingerprint(
fingerprint: string | null
): Promise<{ sessionId: string; redisUsed: boolean; redisHit: boolean }> {
const redis = getRedisClient();
if (\!redis || redis.status \!== "ready" || \!fingerprint) {
return { sessionId: generateUuidV7(), redisUsed: false, redisHit: false };
}
const key = getRedisKey(fingerprint);
try {
// Use SET NX GET to atomically set only if not exists
const created = generateUuidV7();
const previous = await redis.set(
key,
created,
'NX', // Only set if not exists
'EX',
SESSION_TTL_SECONDS,
'GET' // Return previous value if exists
);
if (previous) {
// Key already existed, use the existing session ID
const normalized = normalizeCodexSessionId(previous);
if (normalized) {
return { sessionId: normalized, redisUsed: true, redisHit: true };
}
}
// We successfully created a new mapping
return { sessionId: created, redisUsed: true, redisHit: false };
} catch (error) {
logger.warn("[CodexSessionIdCompleter] Redis error, falling back to local UUID", {
error,
});
return { sessionId: generateUuidV7(), redisUsed: false, redisHit: false };
}
}| redis: { used: boolean; hit: boolean }; | ||
| } | ||
|
|
||
| const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10); |
There was a problem hiding this comment.
[HIGH] [ERROR-NO-USER-FEEDBACK] parseInt can return NaN causing silent Redis failures
Why this is a problem: Line 24 uses parseInt(process.env.SESSION_TTL || "300", 10) without validating the result. If SESSION_TTL contains invalid data (e.g., "abc", "30s", "5.5"), parseInt returns NaN. This causes Redis setex and expire calls to fail silently or behave unexpectedly, breaking session persistence.
Evidence:
const SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10);
// If SESSION_TTL="abc", SESSION_TTL_SECONDS = NaN
await redis.setex(key, SESSION_TTL_SECONDS, sessionId); // Redis error: invalid TTL
await redis.expire(key, SESSION_TTL_SECONDS); // Redis error: invalid TTLRedis will reject NaN TTL values, causing operations to fail silently in try/catch blocks.
Suggested fix:
function parseSessionTTL(): number {
const raw = process.env.SESSION_TTL || "300";
const parsed = parseInt(raw, 10);
if (isNaN(parsed) || parsed <= 0) {
logger.warn(
"[CodexSessionIdCompleter] Invalid SESSION_TTL value, using default 300s",
{ value: raw, parsed }
);
return 300;
}
return parsed;
}
const SESSION_TTL_SECONDS = parseSessionTTL();There was a problem hiding this comment.
Code Review Summary
This PR implements session ID auto-completion for Codex requests with Redis-backed fingerprint caching. While the core logic and test coverage are well-designed, the PR has 2 critical blockers and 2 high-priority bugs that prevent the feature from functioning.
PR Size: XL
- Lines changed: 3,051 (3,051 additions, 0 deletions)
- Files changed: 4
Split recommendations: This PR mixes feature implementation with database migrations. Consider splitting into:
- Database schema change + migration (with actual schema.ts modification per CLAUDE.md)
- Core session completer implementation + integration
- Test suite
This would make review easier and allow partial merging if issues arise.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 1 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Standards | 1 | 0 | 0 | 0 |
Critical Issues (Must Fix)
1. Feature Not Integrated (session-completer.ts:214)
Confidence: 95/100 | Severity: CRITICAL
CodexSessionIdCompleter.complete() is never called in the request pipeline. The entire feature is non-functional dead code.
Impact: Zero functionality - session ID completion will never execute in production.
Evidence: Searched entire codebase - only usage is in test file. Not integrated into chat-completions-handler.ts or any guard in the proxy pipeline documented in CLAUDE.md.
2. Database Setting Never Used (0054_shiny_quentin_quire.sql:1)
Confidence: 85/100 | Severity: CRITICAL
Migration adds enable_codex_session_id_completion but:
- Setting is never checked in code (no conditional logic)
- Violates CLAUDE.md line 54-62: "Never create SQL migration files manually. Always follow this workflow: 1. Modify schema"
- Schema file (
src/drizzle/schema.ts) does not contain this field on the PR branch
Impact: Toggle is non-functional; violates project standards; migration likely to cause schema drift.
High Priority Issues (Should Fix)
3. Race Condition in Redis (session-completer.ts:422)
Confidence: 90/100 | Severity: HIGH
Classic check-then-set race condition in getOrCreateSessionIdFromFingerprint(). When concurrent requests with the same fingerprint arrive:
- Both read
nullfrom Redis - Both generate different UUIDs
- Last writer wins, overwriting the first
Impact: Breaks session continuity for CLI users making rapid successive requests.
Fix: Use Redis SET NX GET for atomic set-if-not-exists operation.
4. parseInt NaN Handling (session-completer.ts:24)
Confidence: 80/100 | Severity: HIGH
SESSION_TTL_SECONDS = parseInt(process.env.SESSION_TTL || "300", 10) can return NaN if environment variable contains invalid data ("abc", "5.5", "30s").
Impact: Redis setex and expire calls fail silently, breaking session persistence. Errors caught by try/catch blocks but not logged at appropriate level.
Fix: Validate parseInt result and log warning if invalid, falling back to default.
Positive Observations
- Test coverage is comprehensive (8 test cases covering main paths)
- UUID v7 implementation follows RFC 9562 correctly
- Code is well-structured with clear helper functions
- Fingerprint calculation includes multiple factors (keyId, IP, UA, message hash)
- Redis fallback handling prevents hard failures
Review Coverage
- Logic and correctness - 2 critical bugs found
- Security (OWASP Top 10) - Clean (fingerprint spoofing risk accepted as design tradeoff)
- Error handling - 1 high priority issue found
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (80%+ estimated)
- Code clarity - Good
- Standards compliance - 1 critical violation found (CLAUDE.md)
Recommendation
DO NOT MERGE until critical issues are resolved:
- Integrate the feature: Add
CodexSessionIdCompleter.complete()call in the appropriate request handler or guard - Fix schema workflow: Add field to
src/drizzle/schema.tsfirst, then regenerate migration, then implement setting check - Fix race condition: Use atomic Redis operations
- Add validation: Validate
SESSION_TTLenvironment variable
After fixes, this will be a solid addition to the Codex integration.
Automated review by Claude AI (Sonnet 4.5)
| } | ||
|
|
||
| return { | ||
| applied: false, |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] x-session-id conflicts can slip through as noop
Evidence: src/app/v1/_lib/codex/session-completer.ts:252 and src/app/v1/_lib/codex/session-completer.ts:279 return action: "noop" without reconciling headerXSessionId when bodyPromptCacheKey matches headerSessionId:
// Already consistent, no-op (but keep mapping warm if possible)
if (canonicalHeader) {
// ...
}
return {
applied: false,
action: "noop",
// ...
};This can leave session_id, x-session-id, and prompt_cache_key inconsistent (e.g. session_id == prompt_cache_key but x-session-id is different), which undermines the purpose of completion.
Why this is a problem: Downstream components and/or upstream providers may read x-session-id. Leaving it stale/incorrect makes session continuity behavior non-deterministic and harder to debug.
Suggested fix:
// In the canonicalHeader block (before returning noop), align any missing/different fields.
const wroteSessionIdHeader = setIfMissingOrDifferent(headerSessionId, canonicalHeader, () =>
headers.set("session_id", canonicalHeader)
);
const wroteXSessionIdHeader = setIfMissingOrDifferent(headerXSessionId, canonicalHeader, () =>
headers.set("x-session-id", canonicalHeader)
);
const wroteMetadata = setIfMissingOrDifferent(metadataSessionId, canonicalHeader, () => {
const metadata = ensureMetadataObject(requestBody);
metadata.session_id = canonicalHeader;
});
if (wroteSessionIdHeader || wroteXSessionIdHeader || wroteMetadata) {
if (fingerprint) {
await CodexSessionIdCompleter.storeFingerprintMapping(fingerprint, canonicalHeader);
}
return {
applied: true,
action: "aligned_mismatch",
sessionId: canonicalHeader,
fingerprint,
redis: { used: false, hit: false },
};
}Also add a unit test that covers session_id + prompt_cache_key consistent but x-session-id missing/different.
| } | ||
|
|
||
| export class CodexSessionIdCompleter { | ||
| static async complete( |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] CodexSessionIdCompleter is not invoked in the runtime request pipeline
Evidence: src/app/v1/_lib/codex/session-completer.ts:201 defines the completer, but the only import in this PR is the unit test (tests/unit/codex/session-completer.test.ts:34). There are no production call sites, so session IDs are never actually completed for real requests.
Why this is a problem: The migration + completion logic land, but behavior never runs in production. Codex clients that omit stable session identifiers will still see broken continuity because headers/body/metadata remain uncompleted.
Suggested fix:
// Example wiring point: ProxySessionGuard.ensure (runs early, before forwarding).
import { CodexSessionIdCompleter } from "@/app/v1/_lib/codex/session-completer";
const settings = await getCachedSystemSettings();
if (settings.enableCodexSessionIdCompletion && Array.isArray(session.request.message.input)) {
try {
await CodexSessionIdCompleter.complete(keyId, session.headers, session.request.message);
} catch (error) {
logger.warn("[ProxySessionGuard] Codex session completion failed", { error });
}
}Wire this behind the new system setting (enable_codex_session_id_completion) so it can be disabled if needed.
There was a problem hiding this comment.
Code Review Summary
This PR introduces Codex session ID completion logic plus a system-setting migration, but there are functional gaps that prevent the feature from behaving as intended. Specifically, one header-consistency case currently returns noop while leaving conflicting session headers unchanged, and the completer is not wired into the production request pipeline at all.
PR Size: XL
- Lines changed: 3051
- Files changed: 4
- Split suggestion: move the Drizzle artifacts (
drizzle/meta/0054_snapshot.json,drizzle/0054_*.sql) into a separate PR/commit from the runtime logic/tests to keep reviewable diffs small.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 2 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- None.
High Priority Issues (Should Fix)
- [HIGH] [LOGIC-BUG]
src/app/v1/_lib/codex/session-completer.ts:252returnsnoopwithout reconciling conflicting/missingx-session-id(Confidence: 85) - [HIGH] [LOGIC-BUG]
src/app/v1/_lib/codex/session-completer.ts:201is not invoked anywhere in production code, so the feature does not take effect (Confidence: 85)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
| } | ||
|
|
||
| export class CodexSessionIdCompleter { | ||
| static async complete( |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] CodexSessionIdCompleter is not invoked in production
Why this is a problem: This new implementation appears to be unused at runtime. The only reference is the unit test import:
tests/unit/codex/session-completer.test.ts:34
import { CodexSessionIdCompleter, generateUuidV7 } from "@/app/v1/_lib/codex/session-completer";As a result, Codex requests will not actually get session_id / x-session-id / prompt_cache_key auto-completed.
Suggested fix:
// Example: wire into the proxy pipeline after auth (keyId known), before session extraction.
// src/app/v1/_lib/proxy/session-guard.ts
import { CodexSessionIdCompleter } from "@/app/v1/_lib/codex/session-completer";
// ... inside ProxySessionGuard.ensure(session)
if (keyId && Array.isArray(session.request.message.input)) {
try {
await CodexSessionIdCompleter.complete(keyId, session.headers, session.request.message);
} catch (error) {
logger.warn("[ProxySessionGuard] Codex session completion failed, continuing", { error });
}
}There was a problem hiding this comment.
- Reviewed PR
#598and applied the size labelsize/XL. - Left 2 inline comments on
src/app/v1/_lib/codex/session-completer.ts:252andsrc/app/v1/_lib/codex/session-completer.ts:201. - Reported 2 High issues:
x-session-idcan remain missing/conflicting while the code returnsnoop(src/app/v1/_lib/codex/session-completer.ts:252).CodexSessionIdCompleterisn’t wired into any production request path, so the feature won’t run in production (src/app/v1/_lib/codex/session-completer.ts:201).
- Submitted the required PR review summary via
gh pr review --comment.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE "system_settings" ADD COLUMN "enable_codex_session_id_completion" boolean DEFAULT true NOT NULL; No newline at end of file | |||
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] enable_codex_session_id_completion is added to DB but not usable from code
Why this is a problem: This migration introduces a new flag:
ALTER TABLE "system_settings" ADD COLUMN "enable_codex_session_id_completion" boolean DEFAULT true NOT NULL;…but there is no corresponding field in the Drizzle schema / SystemSettings types, and no runtime code reads it. That makes the flag effectively dead configuration and risks schema drift (future migration generation may try to revert it).
Suggested fix:
// src/drizzle/schema.ts (systemSettings table)
enableCodexSessionIdCompletion: boolean("enable_codex_session_id_completion").notNull().default(true),// src/types/system-config.ts
export interface SystemSettings {
// ...
enableCodexSessionIdCompletion: boolean;
}
export interface UpdateSystemSettingsInput {
// ...
enableCodexSessionIdCompletion?: boolean;
}// Example usage once wired (e.g. src/app/v1/_lib/proxy/session-guard.ts)
const settings = await getCachedSystemSettings();
if (settings.enableCodexSessionIdCompletion) {
await CodexSessionIdCompleter.complete(keyId, session.headers, session.request.message);
}There was a problem hiding this comment.
Code Review Summary
This PR introduces a Codex session ID auto-completion helper plus a system setting migration, but the implementation is not currently wired into the runtime request path. The new DB flag also isn’t represented/used in code, so it can’t actually control behavior and may introduce schema drift.
PR Size: XL
- Lines changed: 3051
- Files changed: 4
- Split suggestions:
- Move the Drizzle migration + snapshot (
drizzle/0054_shiny_quentin_quire.sql,drizzle/meta/0054_snapshot.json) into a dedicated PR. - Keep “runtime wiring + feature flag usage” separate from “helper implementation + unit tests” to make behavior changes reviewable.
- Move the Drizzle migration + snapshot (
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 1 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- None.
High Priority Issues (Should Fix)
- (90)
src/app/v1/_lib/codex/session-completer.ts:202:CodexSessionIdCompleteris only referenced from tests (tests/unit/codex/session-completer.test.ts:34) and is never invoked in production, so the intended auto-completion won’t run. - (88)
drizzle/0054_shiny_quentin_quire.sql:1: Addsenable_codex_session_id_completion, but there is no corresponding Drizzle schema /SystemSettingstype field and no code reads it, making the flag ineffective and risking schema drift.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Summary
CodexSessionIdCompleterto automatically synchronize session IDs across headers (session_id,x-session-id), body (prompt_cache_key), and metadata fieldsenable_codex_session_id_completionsystem setting (default: true)Changes
drizzle/0054_shiny_quentin_quire.sql: Database migration for new settingsrc/app/v1/_lib/codex/session-completer.ts: Core completion logictests/unit/codex/session-completer.test.ts: Comprehensive unit testsTest plan
bunx vitest run tests/unit/codex/session-completer.test.tsGreptile Overview
Greptile Summary
This PR implements auto-completion logic for Codex session IDs across multiple fields (headers, body, metadata) with Redis-backed fingerprint caching. While the core implementation approach is sound, the PR has critical integration issues and multiple logic bugs that prevent it from functioning:
Critical Issues Found
Integration Problems (Blocking)
CodexSessionIdCompleter.complete()is never called in the request pipeline - the feature is completely non-functionalenable_codex_session_id_completiondatabase setting is added but never used, making the toggle uselessLogic Bugs (High Priority)
getOrCreateSessionIdFromFingerprint()have a race condition where concurrent requests can generate different UUIDs for the same fingerprintif (!canonical)can never be trueSESSION_TTL_SECONDScan become NaN if the environment variable contains invalid data, breaking Redis operationsSecurity Concerns
Style Issues
Architecture Review
The session completion logic follows a clear priority system (header > body > metadata) and includes fingerprint-based session reuse, which is well-designed. However, the implementation needs significant fixes before merge.
Recommendations
CodexSessionIdCompleter.complete()into the Codex request handler and implement the setting checkSET NX GETor distributed lockingConfidence Score: 1/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Client as Codex Client participant Handler as Request Handler participant Completer as CodexSessionIdCompleter participant Redis as Redis Cache participant Fingerprint as Fingerprint Calculator Note over Client,Redis: Session ID Auto-Completion Flow Client->>Handler: Codex Request<br/>(may have partial/missing session IDs) Handler->>Completer: complete(keyId, headers, body) Completer->>Completer: Extract session IDs from:<br/>- headers.session_id<br/>- headers.x-session-id<br/>- body.prompt_cache_key<br/>- body.metadata.session_id alt All sources consistent Completer->>Redis: Store fingerprint mapping Completer-->>Handler: action: "noop" else Header exists, body missing Completer->>Completer: Copy header → body & metadata Completer->>Redis: Store fingerprint mapping Completer-->>Handler: action: "copied_header_to_body" else Body exists, header missing Completer->>Completer: Copy body → headers & metadata Completer->>Redis: Store fingerprint mapping Completer-->>Handler: action: "copied_body_to_header" else Mismatch detected Completer->>Completer: Align all to header value<br/>(header has priority) Completer->>Redis: Store fingerprint mapping Completer-->>Handler: action: "aligned_mismatch" else All missing Completer->>Fingerprint: Calculate fingerprint<br/>(keyId, IP, UA, message hash) Fingerprint-->>Completer: fingerprint hash Completer->>Redis: Check if fingerprint exists alt Fingerprint found in Redis Redis-->>Completer: Return cached session ID Completer->>Redis: Refresh TTL else Fingerprint not found Completer->>Completer: Generate UUID v7 Completer->>Redis: Store new mapping<br/>(fingerprint → UUID) end Completer->>Completer: Set session ID in all locations<br/>(headers + body + metadata) Completer-->>Handler: action: "generated" end Handler->>Handler: Continue with request<br/>(all session fields synchronized) Handler->>Client: Response with consistent session IDs