feat: auto-complete Codex session identifiers#599
Conversation
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 enhances the handling of Codex API requests by implementing an automatic session ID completion mechanism. This ensures that all Codex requests consistently carry a session identifier, which is crucial for maintaining conversation context and optimizing provider resource utilization. The system intelligently fills in missing IDs or generates new ones, leveraging a Redis-based fingerprinting system to ensure these generated IDs remain stable across a user's conversation, thereby improving the overall reliability and efficiency of Codex interactions. 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 new feature for auto-completing Codex session identifiers, enhancing session stickiness and provider reuse stability. The changes include database schema updates, integration into the system settings UI, and core logic for session ID completion using client-provided IDs or generating UUID v7 with Redis fingerprint caching. Comprehensive unit tests have been added to cover various scenarios and edge cases, ensuring the robustness of the new functionality. The localization files have also been updated to support the new setting.
| return 300; | ||
| } | ||
| return parsed; |
There was a problem hiding this comment.
| if (texts.length >= 3) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
The if (texts.length >= 3) { break; } condition limits the number of initial messages considered for the hash. Adding a brief comment explaining the rationale behind choosing 3 (e.g., to keep the fingerprint concise or focus on the immediate conversation start) would improve code clarity and maintainability.
| if (headerSessionId && bodyPromptCacheKey && existing) { | ||
| return { | ||
| applied: false, | ||
| action: "none", | ||
| sessionId: existing.sessionId, | ||
| source: existing.source, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The early return when both headerSessionId and bodyPromptCacheKey exist doesn't validate that they have the same value. If a client sends conflicting session IDs (e.g., session_id: "sess_A" in header and prompt_cache_key: "sess_B" in body), this code will return with applied: false and use the header value per the priority chain at lines 252-260, but the body will keep the different value.
This creates an inconsistency where the request proceeds with mismatched session identifiers, which could cause issues with session tracking and provider affinity.
Recommendation: Add a validation check:
if (headerSessionId && bodyPromptCacheKey) {
if (headerSessionId !== bodyPromptCacheKey) {
// Log warning about conflicting session IDs
// Option 1: Normalize to use header value for both
// Option 2: Reject the request
}
// ... existing early return
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 265:272
Comment:
The early return when both `headerSessionId` and `bodyPromptCacheKey` exist doesn't validate that they have the **same value**. If a client sends conflicting session IDs (e.g., `session_id: "sess_A"` in header and `prompt_cache_key: "sess_B"` in body), this code will return with `applied: false` and use the header value per the priority chain at lines 252-260, but the body will keep the different value.
This creates an inconsistency where the request proceeds with mismatched session identifiers, which could cause issues with session tracking and provider affinity.
**Recommendation**: Add a validation check:
```typescript
if (headerSessionId && bodyPromptCacheKey) {
if (headerSessionId !== bodyPromptCacheKey) {
// Log warning about conflicting session IDs
// Option 1: Normalize to use header value for both
// Option 2: Reject the request
}
// ... existing early return
}
```
How can I resolve this? If you propose a fix, please make it concise.| await redis.set(redisKey, candidate, "EX", ttlSeconds); | ||
| return { sessionId: candidate, source: "generated_uuid_v7", action: "generated_uuid_v7" }; |
There was a problem hiding this comment.
The fallback SET without NX at line 202 can overwrite an existing value in Redis if the second GET also returns null (possibly due to Redis instability or race conditions).
While this is a rare edge case and the code handles the NX race at lines 193-199, this final fallback could theoretically replace a valid session ID that was set by another concurrent request between the second GET and this SET.
Impact: Low probability but could cause session ID instability in high-concurrency scenarios with unreliable Redis.
Consideration: This may be acceptable as a last-resort fallback to ensure the request can proceed, but worth documenting the trade-off.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 202:203
Comment:
The fallback `SET` without `NX` at line 202 can overwrite an existing value in Redis if the second `GET` also returns null (possibly due to Redis instability or race conditions).
While this is a rare edge case and the code handles the NX race at lines 193-199, this final fallback could theoretically replace a valid session ID that was set by another concurrent request between the second GET and this SET.
**Impact**: Low probability but could cause session ID instability in high-concurrency scenarios with unreliable Redis.
**Consideration**: This may be acceptable as a last-resort fallback to ensure the request can proceed, but worth documenting the trade-off.
How can I resolve this? If you propose a fix, please make it concise.| if (headerSessionId && bodyPromptCacheKey && existing) { | ||
| return { | ||
| applied: false, | ||
| action: "none", | ||
| sessionId: existing.sessionId, | ||
| source: existing.source, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The early return doesn't check if x-session-id header or metadata.session_id need synchronization.
If headerSessionId and bodyPromptCacheKey both exist and match, but x-session-id is missing or metadata.session_id differs, the function returns early without synchronizing these fields.
While this may be intentional for idempotency, it means that x-session-id (used for downstream compatibility) and metadata.session_id might remain inconsistent with the canonical session ID.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/codex/session-completer.ts
Line: 265:272
Comment:
The early return doesn't check if x-session-id header or metadata.session_id need synchronization.
If headerSessionId and bodyPromptCacheKey both exist and match, but x-session-id is missing or metadata.session_id differs, the function returns early without synchronizing these fields.
While this may be intentional for idempotency, it means that x-session-id (used for downstream compatibility) and metadata.session_id might remain inconsistent with the canonical session ID.
How can I resolve this? If you propose a fix, please make it concise.| test("no-op when both session_id and prompt_cache_key already exist", async () => { | ||
| const { completeCodexSessionIdentifiers } = await import( | ||
| "@/app/v1/_lib/codex/session-completer" | ||
| ); | ||
|
|
||
| const sessionId = "sess_123456789012345678901"; | ||
| const headers = new Headers({ session_id: sessionId }); | ||
| const body = makeCodexRequestBody({ prompt_cache_key: sessionId }); | ||
|
|
||
| const result = await completeCodexSessionIdentifiers({ | ||
| keyId: 1, | ||
| headers, | ||
| requestBody: body, | ||
| userAgent: "codex_cli_rs/0.50.0", | ||
| }); | ||
|
|
||
| expect(result.applied).toBe(false); | ||
| expect(result.sessionId).toBe(sessionId); | ||
| expect(headers.get("session_id")).toBe(sessionId); | ||
| expect(body.prompt_cache_key).toBe(sessionId); | ||
| }); |
There was a problem hiding this comment.
The test only covers the case where both have the same value. Missing test case: What happens when the header session_id and body prompt_cache_key have different values?
This is a critical edge case that could expose the logic issue where conflicting session IDs aren't validated. Consider adding a test that verifies the behavior when these values differ.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/codex/session-completer.test.ts
Line: 116:136
Comment:
The test only covers the case where both have the same value. Missing test case: What happens when the header session_id and body prompt_cache_key have different values?
This is a critical edge case that could expose the logic issue where conflicting session IDs aren't validated. Consider adding a test that verifies the behavior when these values differ.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Code Review Summary
This PR implements automatic completion of Codex session identifiers to improve session stickiness and provider affinity. The implementation is well-structured with comprehensive test coverage (15 test cases) and proper i18n support across all 5 languages.
PR Size: XL
- Lines changed: 3336 (3318 additions, 18 deletions)
- Files changed: 28
Split suggestion: For future similar features, consider splitting into:
- Core session completion logic + tests
- Database migration + repository changes
- UI/i18n changes
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 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 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues
1. [LOGIC-BUG] Missing validation for conflicting session IDs (src/app/v1/_lib/codex/session-completer.ts:265)
When both headerSessionId and bodyPromptCacheKey are present but have different values, the code returns early without checking if they match. The existing variable will always be set to headerSessionId (due to the priority chain), so the function returns headerSessionId while bodyPromptCacheKey retains its different value. This could lead to inconsistent session tracking.
Suggested fix: Add a warning log when conflicting IDs are detected:
if (headerSessionId && bodyPromptCacheKey && existing) {
if (headerSessionId !== bodyPromptCacheKey) {
logger.warn("[CodexSessionCompleter] Conflicting session IDs detected", {
headerSessionId,
bodyPromptCacheKey,
using: existing.sessionId,
});
}
return { applied: false, action: "none", sessionId: existing.sessionId, source: existing.source };
}2. [TEST-MISSING-CRITICAL] No test for conflicting session ID scenario (tests/unit/codex/session-completer.test.ts)
The test suite covers many edge cases but lacks a test for when headerSessionId and bodyPromptCacheKey have different values. This is the scenario identified in issue #1.
Suggested fix: Add a test case:
test("handles conflicting session_id and prompt_cache_key gracefully", async () => {
const { completeCodexSessionIdentifiers } = await import(
"@/app/v1/_lib/codex/session-completer"
);
const headerSessionId = "sess_header_123456789012345";
const bodySessionId = "sess_body_9876543210987654321";
const headers = new Headers({ session_id: headerSessionId });
const body = makeCodexRequestBody({ prompt_cache_key: bodySessionId });
const result = await completeCodexSessionIdentifiers({
keyId: 1,
headers,
requestBody: body,
userAgent: "codex_cli_rs/0.50.0",
});
// Should use header session_id (priority) and not modify anything
expect(result.applied).toBe(false);
expect(result.sessionId).toBe(headerSessionId);
expect(body.prompt_cache_key).toBe(bodySessionId); // Body unchanged
});Positive Observations (not reported as issues per guidelines)
The implementation demonstrates several good practices:
- Proper error handling with graceful degradation (falls back to UUID generation when Redis unavailable)
- Race condition handling for concurrent requests with SET NX pattern
- Well-documented code with clear comments explaining behavior
- Consistent with existing patterns (similar to thinking-signature-rectifier feature)
- Complete i18n translations across all 5 supported languages
Review Coverage
- Logic and correctness
- Security (OWASP Top 10) - No issues found
- Error handling - Properly logs and falls back
- Type safety - Clean TypeScript types
- Documentation accuracy - Comments match code behavior
- Test coverage - 15 test cases, 80%+ coverage gate
- Code clarity - Well-structured and readable
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
This PR implements automatic completion of Codex session identifiers to improve session stickiness and provider affinity. The implementation is well-structured with comprehensive test coverage (15 test cases) and proper i18n support across all 5 languages.
PR Size: XL
- Lines changed: 3336 (3318 additions, 18 deletions)
- Files changed: 28
Split suggestion: For future similar features, consider splitting into:
- Core session completion logic + tests
- Database migration + repository changes
- UI/i18n changes
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 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 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues
1. [LOGIC-BUG] Missing validation for conflicting session IDs (src/app/v1/_lib/codex/session-completer.ts:265)
When both headerSessionId and bodyPromptCacheKey are present but have different values, the code returns early without checking if they match. The existing variable will always be set to headerSessionId (due to the priority chain), so the function returns headerSessionId while bodyPromptCacheKey retains its different value. This could lead to inconsistent session tracking.
Suggested fix: Add a warning log when conflicting IDs are detected:
if (headerSessionId && bodyPromptCacheKey && existing) {
if (headerSessionId !== bodyPromptCacheKey) {
logger.warn("[CodexSessionCompleter] Conflicting session IDs detected", {
headerSessionId,
bodyPromptCacheKey,
using: existing.sessionId,
});
}
return { applied: false, action: "none", sessionId: existing.sessionId, source: existing.source };
}2. [TEST-MISSING-CRITICAL] No test for conflicting session ID scenario (tests/unit/codex/session-completer.test.ts)
The test suite covers many edge cases but lacks a test for when headerSessionId and bodyPromptCacheKey have different values. This is the scenario identified in issue #1.
Suggested fix: Add a test case:
test("handles conflicting session_id and prompt_cache_key gracefully", async () => {
const { completeCodexSessionIdentifiers } = await import(
"@/app/v1/_lib/codex/session-completer"
);
const headerSessionId = "sess_header_123456789012345";
const bodySessionId = "sess_body_9876543210987654321";
const headers = new Headers({ session_id: headerSessionId });
const body = makeCodexRequestBody({ prompt_cache_key: bodySessionId });
const result = await completeCodexSessionIdentifiers({
keyId: 1,
headers,
requestBody: body,
userAgent: "codex_cli_rs/0.50.0",
});
// Should use header session_id (priority) and not modify anything
expect(result.applied).toBe(false);
expect(result.sessionId).toBe(headerSessionId);
expect(body.prompt_cache_key).toBe(bodySessionId); // Body unchanged
});Review Coverage
- Logic and correctness
- Security (OWASP Top 10) - No issues found
- Error handling - Properly logs and falls back
- Type safety - Clean TypeScript types
- Documentation accuracy - Comments match code behavior
- Test coverage - 15 test cases, 80%+ coverage gate
- Code clarity - Well-structured and readable
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. This is an XL change set touching migrations, runtime session handling, admin settings/i18n, and tests; consider splitting to reduce review risk and simplify rollbacks.
PR Size: XL
- Lines changed: 3336
- Files changed: 28
- Split suggestions:
- PR 1: DB migration + schema/types for
enableCodexSessionIdCompletion - PR 2: Runtime
completeCodexSessionIdentifiers()integration + auditing - PR 3: Admin UI toggle + i18n strings
- PR 4: Unit tests + dedicated coverage config/script
- PR 1: DB migration + schema/types for
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Codex AI
There was a problem hiding this comment.
- PR detected:
#599(feat: auto-complete Codex session identifiers),3336lines changed across28files. - Applied PR size label:
size/XL. - Completed a 6‑perspective diff-only review (comments, tests, error handling, types, general correctness/security, simplification) and found no issues meeting the reporting threshold, so no inline review comments were posted.
- Submitted the required summary review via
gh pr review(includes split suggestions due to XL size).
* PR: 修复 Edge Runtime 下 `process.once` 构建告警(AsyncTaskManager 导入链) (#589) * fix: skip async task manager init on edge * fix: avoid static async task manager import * test: cover edge runtime task scheduling * chore: document edge runtime process.once fix * chore: record edge runtime warning baseline * fix: drop NEXT_PHASE and lazy-init async task manager * test: isolate NEXT_RUNTIME in cloud price sync tests * docs: stabilize edge process.once repro baseline * docs: make rollback instructions hashless * docs: add grep checklist for edge warning audit * chore: run regression gate and align docs * test: cover edge runtime guard on register * Update src/lib/async-task-manager.ts 补充 NEXT_PHASE === "phase-production-build" 检查 Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * chore: format code (fix-edge-runtime-process-once-bee7e19) * PR:i18n settings 拆分与翻译质量门禁 (#588) * refactor(i18n): split settings json into smaller files * refactor(i18n): load settings from split module * refactor(i18n): remove legacy settings.json * chore(i18n): update sync-settings-keys for split layout * test(i18n): add split settings guards * chore: align biome schema version * chore(i18n): document messages loading contract * chore(i18n): add settings split verification notes * chore: format code (refactor-i18n-split-settings-3f48fec) * chore: fix i18n request formatting * chore: format code (refactor-i18n-split-settings-a1eff62) * fix: replace settings placeholder translations * chore: verify settings sync script is idempotent * test: run i18n settings split guards * test: add audit for zh-CN placeholder settings strings * chore: apply biome formatting * chore: document manual i18n settings verification * fix: translate all providers filter in ja * fix: translate all providers filter in zh-TW * fix: translate providers section copy in zh-TW * fix: translate providers section copy in ja * feat: extend placeholder audit output * feat: add allowlist for placeholder audit * docs: define i18n translation quality rules * chore: add i18n audit fail commands * docs: add i18n PR checklist * chore: format i18n audit tests * fix: translate dashboard placeholders * fix: translate myUsage placeholders * fix: enforce locale-specific parentheses * fix: start translating provider form strings * fix: translate provider form strings * fix: translate provider guide content * test: add ja dashboard parentheses guard * test: add zh-TW dashboard parentheses guard * test: add zh-TW myUsage parentheses guard * chore: translate ja provider form strings * chore: translate zh-TW provider form strings * chore: translate ja providers guide * chore: translate zh-TW providers guide * chore: refine zh-TW dashboard strings * chore: translate ja providers strings * chore: translate zh-TW providers strings * chore: refine zh-TW api test strings * chore: translate zh-TW settings small modules * chore: translate ja settings small modules * chore: clear i18n placeholders in settings * chore: format code (refactor-i18n-split-settings-2437d19) * test: fix biome formatting in i18n test * chore: verify Biome lint gate (I18NE-030) * chore: add messages emoji audit script (I18NE-010) * fix: remove emoji from messages warnings (I18NE-040) * test: add messages no-emoji audit gate (I18NE-050) * docs: add zh-CN i18n docs (I18NE-020) * docs: add messages no-emoji rule (I18NE-060) * chore: run full regression checks (I18NE-070) * docs: add i18n PR evidence template (I18NE-080) * fix: make messages no-emoji audit path-sep safe * docs: add bun alias for messages no-emoji audit * fix: detect keycap and flag emoji sequences in i18n message audits * fix(provider): allow removing custom whitelisted models (#592) (#593) * fix(rectifier): detect 'signature: Field required' error and trigger rectifier (#594) - Extend detectThinkingSignatureRectifierTrigger to match 'signature: Field required' - Add Rule 72 for friendly error message when signature field is missing - Add comprehensive test cases for the new detection logic This fixes an issue where switching from non-Anthropic to Anthropic channels with thinking blocks missing signature fields would fail without proper handling. * feat(users): increase provider group length to 200 (#591) close #590 * feat(usage-doc): update OpenCode config example with GPT-5.2 and Gemini v1beta (#597) - Add OpenAI GPT-5.2 model configuration with reasoningEffort options - Add GPT-5.2-small variant using medium reasoning effort - Fix Gemini baseURL to use /v1beta endpoint - Update i18n strings to reflect different baseURLs per provider * feat: auto-complete Codex session identifiers (#599) * fix: Codex session completion must not inject metadata (#601) * feat: auto-complete Codex session identifiers * fix: avoid Codex metadata injection --------- Co-authored-by: YangQing-Lin <56943790+YangQing-Lin@users.noreply.github.com> Co-authored-by: Hwwwww-dev <47653238+Hwwwww-dev@users.noreply.github.com>
What\n- Add Codex Session ID completion toggle (default: enabled)\n- When Codex requests provide only one of header
session_id/ bodyprompt_cache_key, auto-complete the missing value using the client-provided id\n- When both are missing, generate a UUID v7 and reuse it stably within the same conversation via a Redis fingerprint cache (keyId + IP + UA + initial message hash)\n\n## Where\n- System setting:enableCodexSessionIdCompletion(DB + cache + admin UI + i18n)\n- Runtime: runs early inProxySessionGuard.ensure()for Codex (Responses API) requests\n- Audit: recordscodex_session_id_completionin special settings when applied\n\n## Tests\n- Unit tests:tests/unit/codex/session-completer.test.ts\n- Coverage gate (>=80% for the module):bun run test:coverage:codex-session-id-completer\n\n## Migration\n- Addssystem_settings.enable_codex_session_id_completion(default true)Greptile Overview
Greptile Summary
This PR implements automatic completion of Codex session identifiers to improve session stickiness and provider affinity. When Codex requests provide only one of
session_id(header) orprompt_cache_key(body), the system auto-completes the missing value. If both are missing, it generates a UUID v7 and reuses it stably within the same conversation via a Redis fingerprint cache based on keyId, IP, user-agent, and initial message hash.Key Changes
Core Implementation (
session-completer.ts):header.session_id→header.x-session-id→body.prompt_cache_key→body.metadata.session_idSESSION_TTL)Integration Points:
ProxySessionGuard.ensure()before session extractionenableCodexSessionIdCompletion(default: true)special_settingsfield in message requestsDatabase & Configuration:
system_settings.enable_codex_session_id_completioncolumnConcerns Identified
headerSessionIdandbodyPromptCacheKeyhave different values, which could lead to inconsistent session trackingx-session-idheader andmetadata.session_idaren't checked for consistencyPositive Aspects
Confidence Score: 3/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Client as Codex Client participant Guard as ProxySessionGuard participant Completer as SessionCompleter participant Redis as Redis Cache participant DB as Database Client->>Guard: Request (may have session_id/prompt_cache_key) Guard->>Guard: Check enableCodexSessionIdCompletion flag alt Feature Enabled & Codex Request Guard->>Completer: completeCodexSessionIdentifiers() Completer->>Completer: Extract headerSessionId, bodyPromptCacheKey alt Both Present & Match Completer->>Guard: Return (applied=false, action=none) else One Present Completer->>Completer: Use existing ID, complete missing field Completer->>Guard: Return (applied=true, action=completed_missing_fields) else Both Missing Completer->>Completer: Calculate fingerprint hash<br/>(keyId + IP + UA + message) Completer->>Redis: GET codex:fingerprint:{hash}:session_id alt Cache Hit Redis->>Completer: Return cached session ID Completer->>Guard: Return (action=reused_fingerprint_cache) else Cache Miss Completer->>Completer: Generate UUID v7 Completer->>Redis: SET NX with TTL alt SET Success Redis->>Completer: OK Completer->>Guard: Return (action=generated_uuid_v7) else Race Condition Redis->>Completer: NULL (another request won) Completer->>Redis: GET again Redis->>Completer: Return existing session ID Completer->>Guard: Return (action=reused_fingerprint_cache) end end end Guard->>Guard: Record audit trail in special_settings end Guard->>DB: Create message_request with special_settings Guard->>Client: Continue proxy flow