Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Codex-driven conversation compaction pipeline and integrates it into request transformation, fetch handling, response finalization, and session storage; introduces fork-aware session/prompt keys, cloning and input-item utilities, new prompts/constants, CONFIG/type extensions, and comprehensive tests. (≤50 words) Changes
Sequence DiagramsequenceDiagram
participant User
participant Transform as Transform Layer
participant Compaction as Compaction Workflow
participant Codex as Codex API
participant ResHandler as Response Handler
participant Session as Session Manager
User->>Transform: request (input items + pluginConfig)
alt compaction enabled or command detected
Transform->>Compaction: detect trigger, serialize conversation
Compaction-->>Transform: compactionDecision + prompt items
Transform->>Codex: send compaction prompt items
else
Transform->>Codex: send normal prompt items
end
Codex-->>ResHandler: response payload
alt compactionDecision present
ResHandler->>Compaction: extract assistant text -> create summary
Compaction-->>ResHandler: finalized summary + codex_compaction metadata
ResHandler->>Session: applyCompactionSummary(context, preservedSystem, summary)
ResHandler-->>User: return updated payload (status/headers preserved)
else
ResHandler-->>User: return original payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Blockers / high-priority review points:
Files/areas to inspect closely:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
- Create shared clone utility (lib/utils/clone.ts) to eliminate 3+ duplicate implementations - Create InputItemUtils (lib/utils/input-item-utils.ts) for centralized text extraction - Centralize magic numbers in constants with SESSION_CONFIG, CONVERSATION_CONFIG, PERFORMANCE_CONFIG - Add ESLint cognitive complexity rules (max: 15) to prevent future issues - Refactor large functions to use shared utilities, reducing complexity - Update all modules to use centralized utilities and constants - Remove dead code and unused imports - All 123 tests pass, no regressions introduced Code quality improved from B+ to A- with better maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/plugin-config.test.ts (1)
90-141: Blocker: duplicated “file read errors” tests, stray assertions, and outdated expectationsThis section has multiple problems that will break CI and/or fail assertions:
Stray code outside any test (syntax error / CI blocker)
Lines 124–126:
expect(consoleSpy).toHaveBeenCalled();consoleSpy.mockRestore();});
These appear after the secondit('should handle file read errors gracefully', ...)block and referenceconsoleSpyout of scope, leaving braces unbalanced. This matches the esbuild “Unexpected '}'” failure.Duplicate test names for file read errors
There are two separateit('should handle file read errors gracefully', ...)blocks (lines 106–122 and 128–140) doing essentially the same thing with different error messages. That’s confusing and makes maintenance harder.Outdated expected config shape in the last test
The last test (lines 128–140) still asserts:expect(config).toEqual({ codexMode: true, enablePromptCaching: true });but
loadPluginConfignow returns default config includingenableCodexCompactionandautoCompactMinMessages, so this will fail.A minimal concrete fix would be:
- Keep a single “file read errors gracefully” test with the new default config shape.
- Move the
expect(consoleSpy).toHaveBeenCalled()inside that test.- Remove the stray lines and the outdated duplicate test.
For example:
- it('should handle invalid JSON gracefully', () => { - ... - expect(config).toEqual({ - codexMode: true, - enablePromptCaching: true, - enableCodexCompaction: true, - autoCompactMinMessages: 8, - }); - consoleSpy.mockRestore(); - }); - - it('should handle file read errors gracefully', () => { - mockExistsSync.mockReturnValue(true); - mockReadFileSync.mockImplementation(() => { - throw new Error('Permission denied'); - }); - - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - const config = loadPluginConfig(); - - expect(config).toEqual({ codexMode: true, enablePromptCaching: true }); - expect(consoleSpy).toHaveBeenCalled(); - consoleSpy.mockRestore(); - }); + it('should handle invalid JSON gracefully', () => { + mockExistsSync.mockReturnValue(true); + mockReadFileSync.mockReturnValue('invalid json'); + + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const config = loadPluginConfig(); + + expect(config).toEqual({ + codexMode: true, + enablePromptCaching: true, + enableCodexCompaction: true, + autoCompactMinMessages: 8, + }); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + + it('should handle file read errors gracefully', () => { + mockExistsSync.mockReturnValue(true); + mockReadFileSync.mockImplementation(() => { + throw new Error('Permission denied'); + }); + + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const config = loadPluginConfig(); + + expect(config).toEqual({ + codexMode: true, + enablePromptCaching: true, + enableCodexCompaction: true, + autoCompactMinMessages: 8, + }); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + });(Adjust the exact diff to match which of the two read-error variants you want to keep, but the key points are: remove stray lines, avoid duplicate
itnames, and update expectations to the extendedPluginConfigdefaults.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (14)
.github/workflows/main-merge-guard.ymlis excluded by none and included by none.github/workflows/opencode.ymlis excluded by none and included by none.github/workflows/pr-auto-base.ymlis excluded by none and included by noneREADME.mdis excluded by none and included by nonedocs/configuration.mdis excluded by none and included by nonedocs/getting-started.mdis excluded by none and included by nonedocs/notes/2025.11.14.20.26.19.mdis excluded by none and included by noneindex.tsis excluded by none and included by nonerelease-analysis.jsonis excluded by none and included by nonespec/auto-compaction-summary.mdis excluded by none and included by nonespec/codex-compaction.mdis excluded by none and included by nonespec/issue-11-docs-package.mdis excluded by none and included by nonespec/issue-4-prompt-cache-key.mdis excluded by none and included by nonespec/open-issues-check.mdis excluded by none and included by none
📒 Files selected for processing (17)
lib/compaction/codex-compaction.ts(1 hunks)lib/compaction/compaction-executor.ts(1 hunks)lib/config.ts(1 hunks)lib/prompts/codex-compaction.ts(1 hunks)lib/request/codex-fetcher.ts(4 hunks)lib/request/fetch-helpers.ts(4 hunks)lib/request/request-transformer.ts(10 hunks)lib/session/session-manager.ts(2 hunks)lib/types.ts(2 hunks)test/README.md(1 hunks)test/codex-compaction.test.ts(1 hunks)test/codex-fetcher.test.ts(2 hunks)test/fetch-helpers.test.ts(1 hunks)test/plugin-config.test.ts(4 hunks)test/request-transformer-tools-normalization.test.ts(5 hunks)test/request-transformer.test.ts(58 hunks)test/session-manager.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
test/request-transformer-tools-normalization.test.ts (2)
lib/types.ts (2)
RequestBody(147-169)UserConfig(40-47)lib/request/request-transformer.ts (1)
transformRequestBody(1064-1236)
test/codex-compaction.test.ts (2)
lib/types.ts (1)
InputItem(136-142)lib/compaction/codex-compaction.ts (6)
detectCompactionCommand(43-59)serializeConversation(61-91)buildCompactionPromptItems(93-105)collectSystemMessages(107-112)createSummaryMessage(114-124)approximateTokenCount(32-41)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(25-47)
lib/compaction/compaction-executor.ts (3)
lib/types.ts (2)
InputItem(136-142)SessionContext(199-205)lib/session/session-manager.ts (1)
SessionManager(123-402)lib/compaction/codex-compaction.ts (1)
createSummaryMessage(114-124)
lib/session/session-manager.ts (1)
lib/types.ts (3)
InputItem(136-142)SessionContext(199-205)RequestBody(147-169)
test/request-transformer.test.ts (2)
lib/types.ts (3)
RequestBody(147-169)UserConfig(40-47)InputItem(136-142)lib/request/request-transformer.ts (2)
transformRequestBody(1064-1236)filterOpenCodeSystemPrompts(554-665)
lib/request/request-transformer.ts (4)
lib/types.ts (4)
InputItem(136-142)RequestBody(147-169)UserConfig(40-47)SessionContext(199-205)lib/compaction/compaction-executor.ts (1)
CompactionDecision(5-15)lib/compaction/codex-compaction.ts (4)
approximateTokenCount(32-41)serializeConversation(61-91)buildCompactionPromptItems(93-105)collectSystemMessages(107-112)lib/logger.ts (1)
logWarn(75-77)
lib/request/codex-fetcher.ts (3)
lib/types.ts (1)
PluginConfig(6-35)lib/request/fetch-helpers.ts (1)
handleSuccessResponse(337-355)lib/compaction/compaction-executor.ts (1)
finalizeCompactionResponse(24-67)
test/session-manager.test.ts (1)
lib/types.ts (2)
SessionContext(199-205)InputItem(136-142)
lib/compaction/codex-compaction.ts (2)
lib/types.ts (1)
InputItem(136-142)lib/prompts/codex-compaction.ts (2)
CODEX_COMPACTION_PROMPT(1-9)CODEX_SUMMARY_PREFIX(11-11)
test/fetch-helpers.test.ts (1)
lib/request/fetch-helpers.ts (1)
transformRequestForCodex(122-213)
lib/request/fetch-helpers.ts (6)
lib/types.ts (4)
InputItem(136-142)PluginConfig(6-35)RequestBody(147-169)SessionContext(199-205)lib/compaction/compaction-executor.ts (1)
CompactionDecision(5-15)lib/compaction/codex-compaction.ts (1)
detectCompactionCommand(43-59)lib/request/request-transformer.ts (1)
transformRequestBody(1064-1236)lib/logger.ts (1)
logRequest(47-64)lib/constants.ts (1)
LOG_STAGES(56-61)
🪛 GitHub Actions: CI
test/plugin-config.test.ts
[error] 213-213: Transform failed: Unexpected '}' detected by esbuild during transformation at line 213.
🔇 Additional comments (12)
lib/types.ts (1)
20-35: Compaction config and session fields look structurally soundThe new compaction-related fields on
PluginConfigandSessionStateare optional, backward compatible, and line up with howSessionManager/compaction are using them. No issues from a typing or lifecycle standpoint.Also applies to: 183-194
lib/config.ts (1)
12-17: Confirm default-on compaction behavior and thresholdsTurning
enableCodexCompactionon by default withautoCompactMinMessages: 8changes existing behavior for all users. Please confirm this default-on rollout and threshold are intentional and acceptable from a UX/compat perspective (vs. starting disabled or with a higher floor).lib/prompts/codex-compaction.ts (1)
1-11: Compaction prompts are well-scoped and safeThe new
CODEX_COMPACTION_PROMPTandCODEX_SUMMARY_PREFIXconstants are simple string exports with clear, constrained instructions. No technical issues here.test/codex-fetcher.test.ts (1)
74-93: Good coverage of pluginConfig propagation into Codex fetch pathAdding
pluginConfigtobaseDepsand asserting it’s forwarded intotransformRequestForCodexexercises the new dependency surface well. The shape (codexMode, enablePromptCaching, enableCodexCompaction, autoCompactMinMessages) matches the productionPluginConfigexpectations.Also applies to: 107-120
test/session-manager.test.ts (1)
3-3: Compacted history test matches SessionManager behaviorThe new test around
applyCompactionSummary/applyCompactedHistorycorrectly captures the expected merged input layout (base system, summary, then latest user slice). ImportingInputItemfor the system message keeps the test type-safe. No issues here.Also applies to: 180-201
lib/request/codex-fetcher.ts (1)
8-10: Compaction finalization wiring in codex-fetcher looks correct; verify all deps supply pluginConfigThe new flow—passing
pluginConfigintotransformRequestForCodex, then post-processing viafinalizeCompactionResponsewhentransformation.compactionDecisionis present—fits cleanly into the existing pipeline and keeps non-compaction behavior unchanged. Using the post-handleSuccessResponsehandledResponsefor both compaction finalization andrecordSessionResponseFromHandledResponseis consistent with the compaction executor’s expectations.Please double-check that every
createCodexFetcher(...)call site has been updated to pass apluginConfigobject, since it’s now a required dependency inCodexFetcherDeps.Also applies to: 21-30, 32-55, 86-101
test/fetch-helpers.test.ts (1)
253-283: Compaction options wiring in transformRequestForCodex test looks correctThe test now correctly mocks
transformRequestBodyto return{ body }, passespluginConfig, and verifies thatoptionsArg.compaction.originalInputis present and array-shaped before asserting the transformed body/updated init. This matches the newtransformRequestForCodexcontract and compaction integration.Note: the main CI blocker in this PR is in
test/plugin-config.test.ts(unbalanced braces / duplicate tests); see that file’s comments for details.test/codex-compaction.test.ts (1)
1-67: New codex compaction helper tests cover the core behaviors wellThese tests align with the helper implementations (command detection, transcript truncation, prompt structure, system/developer extraction, summary prefixing, and token heuristic). They give good coverage of the intended control flow without over-specifying internals.
lib/session/session-manager.ts (1)
34-39: Session compaction state handling and history rebuilding look consistent and safe
cloneInputandcloneValuecentralize deep cloning, soSessionStatedoesn’t hold references to mutable request objects.extractLatestUserSlicecorrectly finds the last user message and returns a cloned tail slice, or an empty array when there’s no user content; this matches the intent of “latest user-originated slice.”applyCompactionSummarystorescompactionBaseSystemand a synthesizedcompactionSummaryItemon the session, andapplyCompactedHistoryrebuildsbody.inputas[baseSystem..., summary, latestUserSlice...], guarded bycontext.enabled,opts?.skip, and presence of both base and summary.Overall this is a clean, side-effect-safe implementation of the compaction history flow.
Also applies to: 47-58, 273-303
lib/compaction/compaction-executor.ts (1)
69-94: Assistant text extraction and rewrite logic is sound.Scanning
payload.outputfor the first assistantoutput_textblock and rewriting only that entry is a reasonable, minimal intervention and degrades gracefully when the expected structure isn’t present (no assistant text → no rewrite, only metadata added).lib/request/request-transformer.ts (1)
6-12: New compaction imports line up with usage.The imports of
approximateTokenCount,buildCompactionPromptItems,collectSystemMessages,serializeConversation, andCompactionDecisionmatch how they’re used later in the file; no issues here.lib/compaction/codex-compaction.ts (1)
32-41: Token estimation, command detection, and serialization are coherent and match usage.
approximateTokenCountandserializeConversationprovide simple, cheap heuristics that are good enough for compaction triggering, anddetectCompactionCommandcorrectly scopes to the last non‑empty user message with well‑defined triggers. Building the compaction prompt as[developer(CODEX_COMPACTION_PROMPT), user(transcript)]aligns with how the executor and transformer expect to work.Also applies to: 43-59, 61-91, 93-105
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
lib/request/fetch-helpers.ts (1)
140-155: Compaction “enabled” flag does not fully disable compaction reuse.Even when
pluginConfig.enableCodexCompaction === false,applyCompactedHistorywill still run (because it’s only skipped whenmanualCommandis present), so previously stored compaction summaries can still be applied to new requests. That’s surprising for a flag namedenableCodexCompactionand makes it impossible to fully turn off compaction behavior once summaries exist.To make the flag authoritative, gate history application on
compactionEnabledas well:- const sessionContext = sessionManager?.getContext(body); - if (!manualCommand) { - sessionManager?.applyCompactedHistory?.(body, sessionContext); - } + const sessionContext = sessionManager?.getContext(body); + if (compactionEnabled && !manualCommand) { + sessionManager?.applyCompactedHistory?.(body, sessionContext); + }lib/request/request-transformer.ts (1)
896-905: Fallback hash computation can throw on non‑JSON‑serializable metadata.
computeFallbackHashForBodyfeedsbody.metadataand a slice ofbody.inputthroughstableStringify, which ultimately callsJSON.stringifyon primitives. If callers ever pass non‑JSON‑serializable values (e.g.,BigInt, or cyclic objects whentransformRequestBodyis used directly in-process rather than via JSON), this will throw and abort the transform, surfacing as a failed request.To harden this, wrap the hashing in a try/catch and fall back to a simpler seed (e.g., just the model string) on error:
-function computeFallbackHashForBody(body: RequestBody): string { - const inputSlice = Array.isArray(body.input) ? body.input.slice(0, 3) : undefined; - const seed = stableStringify({ - model: typeof body.model === "string" ? body.model : undefined, - metadata: body.metadata, - input: inputSlice, - }); - return createHash("sha1").update(seed).digest("hex").slice(0, 12); -} +function computeFallbackHashForBody(body: RequestBody): string { + try { + const inputSlice = Array.isArray(body.input) ? body.input.slice(0, 3) : undefined; + const seed = stableStringify({ + model: typeof body.model === "string" ? body.model : undefined, + metadata: body.metadata, + input: inputSlice, + }); + return createHash("sha1").update(seed).digest("hex").slice(0, 12); + } catch { + const model = typeof body.model === "string" ? body.model : "unknown"; + return createHash("sha1").update(model).digest("hex").slice(0, 12); + } +}Also applies to: 939-951
lib/compaction/codex-compaction.ts (1)
108-129: Tail extraction still treats any lastrole: "user"as the start; be aware of summary messages.
extractTailAfterSummaryreturns a cloned slice starting at the lastrole: "user"message, regardless of whether that item is a real user turn or a synthetic summary (e.g., fromcreateSummaryMessage, which also usesrole: "user"). If you ever feed conversations that already contain the summary message through this helper, the tail will start at the summary itself rather than “after summary,” which may not match your intended semantics.Not urgent today, but future-proofing could look like marking summary items explicitly (e.g., a flag on
InputItem) and teachingextractTailAfterSummaryto skip those when searching for the last “real” user turn.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
.gitignoreis excluded by none and included by nonebiome.jsonis excluded by none and included by nonedocs/code-cleanup-summary.mdis excluded by none and included by nonedocs/code-quality-analysis-report.mdis excluded by none and included by nonespec/codex-compaction.mdis excluded by none and included by nonevitest.config.tsis excluded by none and included by none
📒 Files selected for processing (12)
lib/compaction/codex-compaction.ts(1 hunks)lib/constants.ts(1 hunks)lib/logger.ts(2 hunks)lib/request/fetch-helpers.ts(4 hunks)lib/request/request-transformer.ts(14 hunks)lib/session/session-manager.ts(10 hunks)lib/utils/clone.ts(1 hunks)lib/utils/input-item-utils.ts(1 hunks)test/codex-compaction.test.ts(1 hunks)test/compaction-executor.test.ts(1 hunks)test/plugin-config.test.ts(5 hunks)test/session-manager.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
test/codex-compaction.test.ts (2)
lib/types.ts (1)
InputItem(136-142)lib/compaction/codex-compaction.ts (7)
detectCompactionCommand(37-53)serializeConversation(55-85)buildCompactionPromptItems(87-99)collectSystemMessages(101-106)createSummaryMessage(108-118)approximateTokenCount(26-35)extractTailAfterSummary(120-130)
test/compaction-executor.test.ts (3)
lib/compaction/compaction-executor.ts (2)
CompactionDecision(5-15)finalizeCompactionResponse(24-67)lib/types.ts (1)
SessionContext(199-205)lib/prompts/codex-compaction.ts (1)
CODEX_SUMMARY_PREFIX(11-11)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(25-47)
test/session-manager.test.ts (3)
lib/types.ts (3)
RequestBody(147-169)SessionContext(199-205)InputItem(136-142)lib/session/session-manager.ts (1)
SessionManager(147-430)lib/constants.ts (1)
SESSION_CONFIG(78-83)
lib/request/fetch-helpers.ts (4)
lib/types.ts (4)
InputItem(136-142)PluginConfig(6-35)RequestBody(147-169)SessionContext(199-205)lib/compaction/compaction-executor.ts (1)
CompactionDecision(5-15)lib/compaction/codex-compaction.ts (1)
detectCompactionCommand(37-53)lib/request/request-transformer.ts (1)
transformRequestBody(980-1152)
lib/request/request-transformer.ts (6)
lib/utils/input-item-utils.ts (2)
extractTextFromItem(16-27)countConversationTurns(115-117)lib/types.ts (4)
InputItem(136-142)RequestBody(147-169)UserConfig(40-47)SessionContext(199-205)lib/compaction/compaction-executor.ts (1)
CompactionDecision(5-15)lib/utils/clone.ts (1)
cloneInputItems(26-31)lib/compaction/codex-compaction.ts (4)
approximateTokenCount(26-35)serializeConversation(55-85)buildCompactionPromptItems(87-99)collectSystemMessages(101-106)lib/logger.ts (1)
logWarn(76-78)
lib/compaction/codex-compaction.ts (4)
lib/types.ts (1)
InputItem(136-142)lib/utils/input-item-utils.ts (3)
extractTextFromItem(16-27)formatRole(43-46)formatEntry(54-56)lib/prompts/codex-compaction.ts (2)
CODEX_COMPACTION_PROMPT(1-9)CODEX_SUMMARY_PREFIX(11-11)lib/utils/clone.ts (1)
deepClone(13-19)
lib/session/session-manager.ts (4)
lib/types.ts (3)
InputItem(136-142)RequestBody(147-169)SessionContext(199-205)lib/utils/input-item-utils.ts (1)
isUserMessage(72-74)lib/utils/clone.ts (2)
cloneInputItems(26-31)deepClone(13-19)lib/constants.ts (1)
SESSION_CONFIG(78-83)
lib/utils/input-item-utils.ts (1)
lib/types.ts (1)
InputItem(136-142)
🪛 GitHub Actions: CI
test/session-manager.test.ts
[warning] 131-131: lint/style/useConst: FIXABLE - Use 'const' instead of 'let' when variable is never reassigned.
[warning] 147-147: lint/style/useConst: FIXABLE - Use 'const' instead of 'let' when variable is never reassigned.
[warning] 156-156: lint/style/useConst: FIXABLE - Use 'const' instead of 'let' when variable is never reassigned.
[warning] 176-176: lint/style/useConst: FIXABLE - Use 'const' instead of 'let' when variable is never reassigned.
[warning] 1-1: assist/source/organizeImports: The imports and exports are not sorted. Suggested fix: organize imports.
🔇 Additional comments (6)
lib/logger.ts (1)
11-11: Test-only suppression of non-error logs looks correctThe IS_TEST_ENV flag combined with the guard in
fallbackcleanly prevents non-error console noise in Vitest/NODE_ENV=testwhile still always surfacing errors. Public behavior outside tests is unchanged since the short-circuit requires both logging flags to be false.Also applies to: 103-106
test/compaction-executor.test.ts (1)
1-134: finalizeCompactionResponse behavior is well exercisedThe tests cover the key branches of
finalizeCompactionResponse:
- Auto mode: rewritten assistant output with the meta note +
CODEX_SUMMARY_PREFIX, correctcodex_compactionmetadata, andSessionManager.applyCompactionSummaryinvocation.- Payloads without assistant output: preserved HTTP status, empty output, and sane metadata defaults.
- Command mode: no auto note, still prefixed summary, and
reasonleft undefined.This matches the executor’s responsibilities and should guard against regressions in the compaction pipeline.
lib/constants.ts (1)
77-103: Session/performance constants are clear and align with SessionManager usageThe new
SESSION_CONFIG,CONVERSATION_CONFIG, andPERFORMANCE_CONFIGconstants give a clean, centralized place for TTL and capacity tuning. Their shapes and values match howSessionManagerand the related tests consume them (idle eviction and max session count), which should make future adjustments straightforward.test/session-manager.test.ts (1)
2-28: Fork-aware sessions and compaction history behavior are well coveredThe new helper (
BodyOptions+createBody) and tests validate the key SessionManager behaviors:
- Standard sessions:
conversation_id-based keys, prefix matching, cache key regeneration on divergent prefixes, cached token recording, metrics snapshot, and prompt_cache_key fallback/reuse all line up with the SessionManager implementation.- Fork-specific flows:
creates fork-specific sessions with derived cache keyscorrectly asserts theconv-fork::fork::<forkId>behavior for both session reuse and prompt_cache_key propagation across turns.- Compaction integration:
scopes compaction summaries per fork sessionandapplies compacted history when summary storedaccurately model howapplyCompactionSummaryandapplyCompactedHistoryshould combinebaseSystem, the stored summary, and the latest user tail, and verify that summaries are isolated per fork.- TTL/capacity: tests that use
SESSION_CONFIG.IDLE_TTL_MSandSESSION_CONFIG.MAX_ENTRIESconfirm that idle pruning and capacity enforcement respect the centralized constants.From a correctness perspective, this suite gives good confidence in the new session/fork/compaction behavior.
Also applies to: 39-160, 161-219, 221-271
lib/session/session-manager.ts (1)
301-331: Compaction summary application path looks safe and self-contained.
applyCompactionSummaryandapplyCompactedHistorycorrectly guard oncontext?.enabled, avoid mutating the original arrays viacloneInputItems/deepClone, and fall back to a no-op when compaction state is absent. TheextractLatestUserSliceheuristic (last user turn + tail) is simple but coherent with the idea of keeping only the latest user slice plus the persisted summary.No blocking issues from this compaction/session wiring.
lib/utils/input-item-utils.ts (1)
16-116: InputItem utility set is coherent and matches expected shapes.The shared helpers for text extraction, role classification, and turn counting line up with the
InputItemshape and existing usage patterns (e.g., compaction and request-transform paths), and they correctly treat non-text/empty content as empty. Centralizing these avoids further duplication; no functional issues detected here.
riatzukiza
left a comment
riatzukiza
left a comment
There was a problem hiding this comment.
$Thanks for the thorough review. Here is the plan:\n1. Consolidate the loadPluginConfig error-handling tests so there is a single file read errors spec that checks the current defaults (codexMode, enablePromptCaching, enableCodexCompaction, autoCompactMinMessages) and keeps the warning/spy assertions together.\n2. Guard sessionManager?.applyCompactedHistory under pluginConfig.enableCodexCompaction !== false so the flag can truly disable compaction reuse (it already skips when a manual command is present).\n3. Wrap computeFallbackHashForBody in a try/catch and fall back to hashing just the model name when JSON serialization fails, preventing crashes on atypical metadata.\n\nI will also verify the default-on compaction ship is intentional and keep the new defaults, then run the related tests before pushing an update.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/prompts/codex.ts (2)
49-155: Blocker:getCodexInstructionsexceeds linted cognitive complexity and needs decompositionThe
getCodexInstructionsfunction (lines 49–155) has grown into a large, multi-branch routine (session cache, file cache, TTL logic, GitHub fetch, 304/200 handling, error fallback). The CI failure (noExcessiveCognitiveComplexityat Line 49) means this must be refactored before merge.Consider splitting responsibilities into smaller helpers, e.g.:
- A “fast path” helper that returns from session cache if present.
- A file‑cache helper that:
- Reads metadata, computes
isCacheFresh, and either returns cached content ornullplus(cachedETag, cachedTag, cacheFileExists).- A network fetch helper that:
- Resolves
latestTag,- Handles the 304 vs 200 branches,
- Updates disk metadata and session cache, and
- Applies the GitHub/bundled fallbacks.
Then have
getCodexInstructionsorchestrate those helpers at a higher level, keeping its own branching very shallow. This should bring complexity under the configured limit while making the error paths easier to reason about.
79-88: TTL rate‑limit behavior doesn’t update on 304 responses (more GitHub calls than intended)The comment says “Only checks GitHub if cache is older than 15 minutes”, but with the current logic:
isCacheFreshis based onmetadata.lastCheckedloaded from disk.lastCheckedis only updated in theresponse.ok(200) branch where you rewrite the meta file (lines 118–132).- In the
response.status === 304 && cacheFileExistsbranch (lines 112–116), you return cached file content but never bumplastChecked.Once the TTL window has elapsed, if GitHub keeps returning 304, every subsequent call to
getCodexInstructionswill re-hit GitHub becausecachedTimestampnever advances, undermining the intended rate‑limit protection.To align behavior with the docstring and avoid unnecessary GitHub traffic, you should update the cache metadata (at least
lastChecked, and probablytag/url) in the 304 path as well. One straightforward option is to share a small helper that writes the meta file (etag, tag, lastChecked, url) and call it from both the 200 and 304 branches.Also applies to: 112-132
lib/request/codex-fetcher.ts (1)
44-113: RefactorcodexFetchto satisfy cognitive complexity lintThe
codexFetchfunction now handles auth refresh, request transformation, in-band Codex commands, fetch, error/success handling, compaction finalization, and session recording. CI is flagging this withnoExcessiveCognitiveComplexity(17 > 15), and it’s also getting dense to reason about.You can reduce complexity by extracting the response-handling branch into a helper, leaving
codexFetchfocused on auth + request building:+type CodexTransformationResult = Awaited<ReturnType<typeof transformRequestForCodex>>; + +interface ProcessResponseOptions { + response: Response; + hasTools: boolean; + transformation?: CodexTransformationResult; + sessionManager: SessionManager; + sessionContext?: SessionContext; +} + +async function processCodexResponse({ + response, + hasTools, + transformation, + sessionManager, + sessionContext, +}: ProcessResponseOptions): Promise<Response> { + if (!response.ok) { + return await handleErrorResponse(response); + } + + let handledResponse = await handleSuccessResponse(response, hasTools); + + if (transformation?.compactionDecision) { + handledResponse = await finalizeCompactionResponse({ + response: handledResponse, + decision: transformation.compactionDecision, + sessionManager, + sessionContext, + }); + } + + await recordSessionResponseFromHandledResponse({ + sessionManager, + sessionContext, + handledResponse, + }); + + return handledResponse; +} @@ - const response = await fetch(url, { ...requestInit, headers }); + const response = await fetch(url, { ...requestInit, headers }); @@ - if (!response.ok) { - return await handleErrorResponse(response); - } - - let handledResponse = await handleSuccessResponse(response, hasTools); - - if (transformation?.compactionDecision) { - handledResponse = await finalizeCompactionResponse({ - response: handledResponse, - decision: transformation.compactionDecision, - sessionManager, - sessionContext, - }); - } - - await recordSessionResponseFromHandledResponse({ - sessionManager, - sessionContext, - handledResponse, - }); - - return handledResponse; + return await processCodexResponse({ + response, + hasTools, + transformation, + sessionManager, + sessionContext, + });You’ll need to import
SessionContextand adjust types to match your exacttransformRequestForCodexreturn type, but this pattern should drop complexity under the configured threshold.lib/session/session-manager.ts (1)
155-196: Fix explicitanyin hostprompt_cache_keyfallback (lint blocker)
getContext’s host-cache-key fallback currently uses(body as any), which CI flags withlint/suspicious/noExplicitAnyand is easy to avoid given theRequestBodyindex signature.Consider this adjustment:
- const conversationId = extractConversationId(body); - const forkId = extractForkIdentifier(body); - if (!conversationId) { - // Fall back to host-provided prompt_cache_key if no metadata ID is available - const hostCacheKey = (body as any).prompt_cache_key || (body as any).promptCacheKey; - if (hostCacheKey && typeof hostCacheKey === "string") { + const conversationId = extractConversationId(body); + const forkId = extractForkIdentifier(body); + if (!conversationId) { + // Fall back to host-provided prompt_cache_key if no metadata ID is available + const bodyAny = body as Record<string, unknown>; + const hostCacheKey = + typeof body.prompt_cache_key === "string" + ? body.prompt_cache_key + : typeof bodyAny.promptCacheKey === "string" + ? (bodyAny.promptCacheKey as string) + : undefined; + if (hostCacheKey) { // Use the existing cache key as session identifier to maintain continuityThis removes explicit
anywhile preserving the same behavior.
♻️ Duplicate comments (5)
test/request-transformer-tools-normalization.test.ts (1)
5-14: Theoptions?: anytyping issue was already flagged.The past review comment correctly identified that
optionsshould be typed asTransformRequestOptionsinstead ofany.Additionally, the helper doesn't expose the
sessionContextparameter thattransformRequestBodyaccepts (see line 965 inlib/request/request-transformer.ts). While not needed for current tool normalization tests, this may limit future test coverage if session-aware tool behavior is added.Run the following script to check if any existing tests or upcoming scenarios require
sessionContextfor tool-related testing:#!/bin/bash # Search for sessionContext usage in test files and tool-related code rg -n --type=ts -C3 'sessionContext' test/ lib/request/lib/utils/clone.ts (1)
13-19: deepClone will throw onundefinedand inefficiently handles primitives.The past review comment remains valid:
JSON.parse(JSON.stringify(undefined))throws aSyntaxError, and primitives are unnecessarily serialized. This will cause runtime failures for valid generic inputs.Apply the suggested fix from the past review to hoist the
structuredClonereference and short-circuit non-object values:-export function deepClone<T>(value: T): T { - const globalClone = (globalThis as { structuredClone?: <T>(value: T) => T }).structuredClone; - if (typeof globalClone === "function") { - return globalClone(value); - } - return JSON.parse(JSON.stringify(value)) as T; -} +const STRUCTURED_CLONE = + (globalThis as { structuredClone?: <U>(value: U) => U }).structuredClone; + +export function deepClone<T>(value: T): T { + // Fast path: primitives and null/undefined are already copy-safe. + if (value === null || typeof value !== "object") { + return value; + } + if (typeof STRUCTURED_CLONE === "function") { + return STRUCTURED_CLONE(value); + } + // Fallback: JSON-based deep clone for plain objects/arrays. + return JSON.parse(JSON.stringify(value)) as T; +}Based on learnings.
test/request-transformer.test.ts (1)
15-32: AlignrunTransformhelper types with the real transformer signature
runTransformcurrently declares:async function runTransform( body: RequestBody, codexInstructions: string, userConfig?: UserConfig, codexMode = true, options?: Record<string, unknown>, sessionContext?: any, )Given
transformRequestBodyalready exposes concrete types for itsoptionsand last parameter, keeping this helper onRecord<string, unknown>andanyweakens type safety in a heavily-used test helper.To keep tests aligned with the production API and catch mismatches early, consider:
- Importing and using the real
TransformRequestOptionstype foroptions.- Narrowing
sessionContextto the correct type (e.g.,SessionContext | undefinedor the corresponding parameter type fromtransformRequestBody).This is non-blocking but will make these tests more robust against future API changes.
lib/session/session-manager.ts (1)
87-126: Fork identifier handling is narrower than prompt-cache fork hints
extractForkIdentifieronly considersforkId,fork_id,branchId, andbranch_id. Per earlier feedback,derivePromptCacheKeyFromBodyinlib/request/request-transformer.tsalso looks at other fork-hint keys (e.g.parentConversationId), so there’s a risk that session keys and prompt cache keys diverge for some fork sources.If that’s not intentional, consider extending
forkKeysto include the same set used in prompt cache derivation so session scoping and cache-key derivation stay in lockstep.lib/compaction/codex-compaction.ts (1)
86-129: Prompt construction and tail extraction fit current flow; be aware of summary-as-user semantics
buildCompactionPromptItemsandcreateSummaryMessageproduce the expected developer/user pair and a summary message prefixed withCODEX_SUMMARY_PREFIX.extractTailAfterSummarythen returns a cloned slice starting at the lastrole: "user"message, which matches the “latest user slice” semantics used by the session manager.As previously noted in earlier reviews, if you later decide to keep injected summaries in the client-side conversation as
role: "user"items, you may want to add an explicit summary marker (e.g. a flag on the item or in its metadata) and teachextractTailAfterSummaryto skip those, so the tail always starts from a “real” user instruction.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
biome.jsonis excluded by none and included by none
📒 Files selected for processing (47)
lib/auth/auth.ts(3 hunks)lib/auth/browser.ts(1 hunks)lib/auth/server.ts(1 hunks)lib/cache/cache-metrics.ts(7 hunks)lib/cache/cache-warming.ts(6 hunks)lib/cache/prompt-fingerprinting.ts(7 hunks)lib/cache/session-cache.ts(6 hunks)lib/commands/codex-metrics.ts(1 hunks)lib/compaction/codex-compaction.ts(1 hunks)lib/compaction/compaction-executor.ts(1 hunks)lib/config.ts(2 hunks)lib/logger.ts(2 hunks)lib/prompts/codex.ts(4 hunks)lib/prompts/opencode-codex.ts(6 hunks)lib/request/codex-fetcher.ts(4 hunks)lib/request/response-handler.ts(5 hunks)lib/session/response-recorder.ts(1 hunks)lib/session/session-manager.ts(9 hunks)lib/types.ts(4 hunks)lib/utils/cache-config.ts(2 hunks)lib/utils/clone.ts(1 hunks)lib/utils/file-system-utils.ts(4 hunks)lib/utils/input-item-utils.ts(1 hunks)test/auth-constants.test.ts(1 hunks)test/auth.test.ts(3 hunks)test/browser.test.ts(1 hunks)test/cache-metrics.test.ts(5 hunks)test/cache-warming.test.ts(10 hunks)test/codex-compaction.test.ts(1 hunks)test/codex-fetcher.test.ts(5 hunks)test/codex-metrics-command.test.ts(8 hunks)test/compaction-executor.test.ts(1 hunks)test/config.test.ts(2 hunks)test/constants.test.ts(1 hunks)test/fetch-helpers.test.ts(4 hunks)test/index.test.ts(8 hunks)test/logger.test.ts(3 hunks)test/prompt-fingerprinting.test.ts(1 hunks)test/prompts-codex.test.ts(4 hunks)test/prompts-opencode-codex.test.ts(2 hunks)test/request-transformer-tools-normalization.test.ts(1 hunks)test/request-transformer.test.ts(1 hunks)test/response-handler.test.ts(5 hunks)test/response-recorder.test.ts(2 hunks)test/server.test.ts(4 hunks)test/session-cache-evictions.test.ts(1 hunks)test/session-manager.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (36)
test/compaction-executor.test.ts (4)
lib/compaction/compaction-executor.ts (2)
CompactionDecision(5-15)finalizeCompactionResponse(24-66)lib/session/session-manager.ts (1)
SessionManager(139-417)lib/types.ts (1)
SessionContext(199-205)lib/prompts/codex-compaction.ts (1)
CODEX_SUMMARY_PREFIX(11-11)
test/constants.test.ts (1)
lib/constants.ts (12)
PLUGIN_NAME(7-7)PROVIDER_ID(16-16)CODEX_BASE_URL(10-10)DUMMY_API_KEY(13-13)LOG_STAGES(56-61)ERROR_MESSAGES(49-53)HTTP_STATUS(19-22)OPENAI_HEADERS(25-31)OPENAI_HEADER_VALUES(34-37)URL_PATHS(40-43)JWT_CLAIM_PATH(46-46)AUTH_LABELS(71-75)
test/response-recorder.test.ts (1)
lib/session/response-recorder.ts (1)
isCodexResponsePayload(5-24)
lib/logger.ts (1)
lib/utils/file-system-utils.ts (1)
getOpenCodePath(22-24)
test/response-handler.test.ts (1)
lib/request/response-handler.ts (2)
ensureContentType(94-102)convertSseToJson(36-87)
test/codex-compaction.test.ts (2)
lib/types.ts (1)
InputItem(136-142)lib/compaction/codex-compaction.ts (7)
detectCompactionCommand(36-52)serializeConversation(54-84)buildCompactionPromptItems(86-98)collectSystemMessages(100-105)createSummaryMessage(107-117)approximateTokenCount(25-34)extractTailAfterSummary(119-129)
lib/cache/session-cache.ts (1)
lib/cache/cache-metrics.ts (1)
recordCacheEviction(183-185)
test/browser.test.ts (2)
lib/auth/browser.ts (2)
getBrowserOpener(13-18)openBrowserUrl(25-35)lib/constants.ts (1)
PLATFORM_OPENERS(64-68)
test/server.test.ts (1)
lib/auth/server.ts (1)
startLocalOAuthServer(17-77)
test/request-transformer-tools-normalization.test.ts (2)
lib/types.ts (2)
RequestBody(147-169)UserConfig(40-47)lib/request/request-transformer.ts (1)
transformRequestBody(960-1123)
test/codex-fetcher.test.ts (2)
lib/request/codex-fetcher.ts (1)
createCodexFetcher(32-114)lib/constants.ts (1)
LOG_STAGES(56-61)
test/auth-constants.test.ts (1)
lib/auth/auth.ts (4)
AUTHORIZE_URL(9-9)CLIENT_ID(8-8)REDIRECT_URI(11-11)SCOPE(12-12)
lib/commands/codex-metrics.ts (4)
lib/session/session-manager.ts (2)
SessionManager(139-417)SessionMetricsSnapshot(128-137)lib/cache/cache-metrics.ts (1)
getCachePerformanceReport(224-252)lib/cache/cache-warming.ts (2)
CacheWarmSnapshot(133-136)getCacheWarmSnapshot(138-143)lib/types.ts (1)
RequestBody(147-169)
test/session-manager.test.ts (3)
lib/types.ts (3)
RequestBody(147-169)SessionContext(199-205)InputItem(136-142)lib/session/session-manager.ts (1)
SessionManager(139-417)lib/constants.ts (1)
SESSION_CONFIG(78-83)
test/request-transformer.test.ts (2)
lib/types.ts (3)
RequestBody(147-169)UserConfig(40-47)InputItem(136-142)lib/request/request-transformer.ts (9)
transformRequestBody(960-1123)normalizeModel(221-254)getReasoningConfig(286-344)filterInput(368-401)getModelConfig(263-272)addToolRemapMessage(664-682)isOpenCodeSystemPrompt(410-437)filterOpenCodeSystemPrompts(447-549)addCodexBridgeMessage(585-656)
test/prompt-fingerprinting.test.ts (1)
lib/cache/prompt-fingerprinting.ts (5)
generateContentHash(15-17)hasBridgePromptInConversation(27-46)generateInputHash(124-143)cacheBridgeDecision(109-117)getCachedBridgeDecision(88-101)
test/cache-warming.test.ts (2)
lib/cache/cache-warming.ts (3)
warmCachesOnStartup(32-102)getCacheWarmingStats(145-156)areCachesWarm(113-126)lib/cache/session-cache.ts (2)
codexInstructionsCache(76-76)openCodePromptCache(77-77)
test/index.test.ts (1)
lib/auth/auth.ts (1)
REDIRECT_URI(11-11)
test/prompts-opencode-codex.test.ts (2)
lib/cache/session-cache.ts (1)
openCodePromptCache(77-77)lib/prompts/opencode-codex.ts (2)
getOpenCodeCodexPrompt(31-129)getCachedPromptPrefix(136-146)
lib/session/session-manager.ts (5)
lib/types.ts (3)
InputItem(136-142)RequestBody(147-169)SessionContext(199-205)lib/utils/input-item-utils.ts (1)
isUserMessage(80-82)lib/utils/clone.ts (2)
cloneInputItems(26-31)deepClone(13-19)lib/constants.ts (1)
SESSION_CONFIG(78-83)lib/logger.ts (1)
logDebug(66-69)
lib/cache/cache-warming.ts (2)
lib/logger.ts (2)
logWarn(75-77)logDebug(66-69)lib/cache/session-cache.ts (2)
codexInstructionsCache(76-76)openCodePromptCache(77-77)
lib/compaction/codex-compaction.ts (4)
lib/types.ts (1)
InputItem(136-142)lib/utils/input-item-utils.ts (3)
extractTextFromItem(16-27)formatRole(43-54)formatEntry(62-64)lib/prompts/codex-compaction.ts (2)
CODEX_COMPACTION_PROMPT(1-9)CODEX_SUMMARY_PREFIX(11-11)lib/utils/clone.ts (1)
deepClone(13-19)
lib/request/response-handler.ts (1)
lib/types.ts (1)
SSEEventData(174-178)
lib/utils/input-item-utils.ts (1)
lib/types.ts (1)
InputItem(136-142)
test/fetch-helpers.test.ts (3)
lib/auth/auth.ts (1)
refreshAccessToken(124-166)lib/request/fetch-helpers.ts (8)
shouldRefreshToken(29-31)extractRequestUrl(82-86)rewriteUrlForCodex(104-106)createCodexHeaders(212-235)refreshAndUpdateToken(39-75)transformRequestForCodex(117-203)handleErrorResponse(242-319)handleSuccessResponse(328-343)lib/constants.ts (2)
OPENAI_HEADERS(25-31)OPENAI_HEADER_VALUES(34-37)
test/auth.test.ts (1)
lib/auth/auth.ts (10)
createState(19-21)parseAuthorizationInput(28-52)decodeJWT(107-117)createAuthorizationFlow(172-189)AUTHORIZE_URL(9-9)CLIENT_ID(8-8)REDIRECT_URI(11-11)SCOPE(12-12)exchangeAuthorizationCode(61-100)refreshAccessToken(124-166)
lib/prompts/codex.ts (3)
lib/cache/cache-metrics.ts (2)
recordCacheHit(167-169)recordCacheMiss(175-177)lib/utils/file-system-utils.ts (1)
getOpenCodePath(22-24)lib/utils/cache-config.ts (2)
CACHE_FILES(26-35)CACHE_TTL_MS(11-11)
test/codex-metrics-command.test.ts (1)
lib/commands/codex-metrics.ts (1)
maybeHandleCodexCommand(36-93)
lib/compaction/compaction-executor.ts (2)
lib/types.ts (2)
InputItem(136-142)SessionContext(199-205)lib/compaction/codex-compaction.ts (1)
createSummaryMessage(107-117)
test/logger.test.ts (1)
lib/logger.ts (3)
LOGGING_ENABLED(7-7)logRequest(47-64)logWarn(75-77)
test/config.test.ts (1)
lib/request/request-transformer.ts (2)
getModelConfig(263-272)getReasoningConfig(286-344)
lib/prompts/opencode-codex.ts (2)
lib/cache/cache-metrics.ts (2)
recordCacheHit(167-169)recordCacheMiss(175-177)lib/utils/cache-config.ts (1)
CACHE_TTL_MS(11-11)
test/session-cache-evictions.test.ts (2)
lib/cache/cache-metrics.ts (2)
resetCacheMetrics(206-208)getCacheMetrics(191-193)lib/cache/session-cache.ts (3)
codexInstructionsCache(76-76)openCodePromptCache(77-77)cleanupExpiredCaches(102-114)
test/cache-metrics.test.ts (1)
lib/cache/cache-metrics.ts (8)
recordCacheHit(167-169)getCacheMetrics(191-193)recordCacheMiss(175-177)recordCacheEviction(183-185)getCacheMetricsSummary(199-201)resetCacheMetrics(206-208)autoResetCacheMetrics(214-218)getCachePerformanceReport(224-252)
lib/request/codex-fetcher.ts (3)
lib/types.ts (1)
PluginConfig(6-35)lib/request/fetch-helpers.ts (1)
handleSuccessResponse(328-343)lib/compaction/compaction-executor.ts (1)
finalizeCompactionResponse(24-66)
test/prompts-codex.test.ts (2)
lib/prompts/codex.ts (1)
getCodexInstructions(49-155)lib/cache/session-cache.ts (2)
codexInstructionsCache(76-76)getCodexCacheKey(85-87)
🪛 GitHub Actions: CI
lib/cache/prompt-fingerprinting.ts
[error] 27-27: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
[error] 53-53: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
[error] 124-124: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
lib/commands/codex-metrics.ts
[warning] 300-300: lint/suspicious/useIterableCallbackReturn: This callback passed to forEach() should not return a value.
lib/session/session-manager.ts
[error] 159-159: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
lib/cache/cache-warming.ts
[error] 32-32: lint/complexity/noExcessiveCognitiveComplexity: Excessive complexity of 28 detected (max: 15). Please refactor this function to reduce its complexity.
lib/prompts/codex.ts
[error] 49-49: lint/complexity/noExcessiveCognitiveComplexity: Excessive complexity of 29 detected (max: 15). Refactor to reduce complexity.
lib/compaction/compaction-executor.ts
[error] 31-31: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
[error] 68-68: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type.
lib/prompts/opencode-codex.ts
[error] 31-31: lint/complexity/noExcessiveCognitiveComplexity: Excessive complexity of 16 detected (max: 15). Refactor to reduce complexity.
lib/request/codex-fetcher.ts
[error] 44-44: lint/complexity/noExcessiveCognitiveComplexity: Excessive complexity of 17 detected (max: 15). Refactor to reduce complexity.
🪛 Gitleaks (8.29.0)
test/session-manager.test.ts
[high] 139-139: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (58)
lib/utils/cache-config.ts (1)
1-49: Clean and well-structured configuration module.The changes are minimal and align with best practices: the added comment line improves readability in the JSDoc header, and the EOF newline ensures consistency with linting standards. All exports use
as constfor proper type narrowing, and the module is logically organized with clear sections for TTL, directories, files, and metadata fields.test/request-transformer-tools-normalization.test.ts (4)
18-49: Test correctly verifies tool normalization for non-Codex models.The test properly checks that:
- Native Codex tools (
shell,apply_patch) are passed as type-only entries- String tools are converted to function tools with default schema
- Non-Codex models enable
parallel_tool_calls=true(line 48)This aligns with the implementation in
lib/request/request-transformer.ts(lines 1050-1052).
51-102: Test correctly verifies Codex model tool behavior.The test properly validates:
- Direct function objects use their own fields (lines 86-92)
- Nested function objects prefer nested fields (lines 95-97)
- Codex models disable parallel tool calls (line 101)
- Native Codex tools are preserved (line 83)
The
parallel_tool_calls=falseexpectation for "gpt-5-codex" matches the implementation logic.
104-160: Test correctly verifies tool map and enabled flag handling.The test properly validates:
- Tools defined as object maps are normalized (line 113)
- Disabled tools are filtered out (lines 150-151)
- Boolean
falsevalues are treated as disabled (line 151)- Custom tool types with formats are preserved (lines 153-159)
The enabled flag logic and filtering behavior is correctly tested.
162-178: Test correctly verifies cleanup when no tools remain.The test validates that when all tools are disabled, the implementation properly removes
tools,tool_choice, andparallel_tool_callsfields (lines 175-177). This matches the cleanup logic in the transformer.test/auth-constants.test.ts (1)
1-11: LGTM!Formatting changes align with project standardization (double quotes, import ordering). No functional changes detected.
lib/utils/file-system-utils.ts (1)
1-76: LGTM!Formatting changes only—import reorganization and function signature cleanup. No behavioral changes.
test/prompt-fingerprinting.test.ts (1)
1-134: LGTM!Import centralization and double-quote standardization. Test logic remains unchanged.
test/constants.test.ts (1)
1-55: LGTM!Formatting standardization (double quotes, import ordering). No functional changes.
test/config.test.ts (1)
1-151: LGTM!String literal standardization to double quotes. Test logic unchanged.
test/index.test.ts (1)
1-451: LGTM!Test mock expansion and double-quote standardization. Test coverage appropriately updated for the broader changes.
test/auth.test.ts (1)
1-325: LGTM!Import expansion reflects extended public API surface. String literals standardized to double quotes. Test logic remains sound.
lib/config.ts (1)
15-16: Verification confirms proper typing and integration—no blockers.Both fields are properly typed in the
PluginConfiginterface inlib/types.ts:
enableCodexCompaction?: boolean;(line 24, default: true via JSDoc)autoCompactMinMessages?: number;(line 34)They are actively consumed in
lib/request/fetch-helpers.ts(lines 140 and 144) with sensible fallbacks, and comprehensive test coverage spans multiple test files. The config defaults inlib/config.ts(lines 15–16) align with the implementation.lib/auth/browser.ts (1)
32-32: LGTM: Unused error parameter properly indicated.The underscore prefix correctly signals that the error is intentionally ignored, aligning with the documented behavior of silent failure.
lib/session/response-recorder.ts (1)
33-33: LGTM: Guard condition formatting improved.The single-line conditional is more concise while preserving the same logic.
lib/auth/server.ts (1)
2-2: LGTM: Import organization improved.The import reordering enhances readability with no behavioral impact.
Also applies to: 6-6
test/browser.test.ts (1)
1-76: LGTM: Formatting standardized.The changes improve style consistency (double quotes, import order) without altering test behavior.
test/session-cache-evictions.test.ts (1)
5-43: LGTM: Test formatting standardized.Import organization and quote style updated for consistency without affecting test behavior.
lib/commands/codex-metrics.ts (1)
1-383: LGTM: Formatting standardized throughout.The extensive formatting changes improve consistency (spacing, quotes, alignment) without altering functionality.
test/logger.test.ts (1)
1-134: LGTM: Test formatting standardized.Quote style and spacing updated for consistency without affecting test logic.
lib/auth/auth.ts (1)
2-4: LGTM - Clean organizational improvements.The import reordering and type-only imports improve code organization. The condensed conditionals maintain the same logic while being more concise.
Also applies to: 90-90, 150-150
test/response-recorder.test.ts (1)
2-7: LGTM - Test structure is correct.The import updates and test assertions properly validate the payload structure with typed expectations.
Also applies to: 134-138
lib/logger.ts (1)
10-10: Good improvement - Test environment log suppression.The
IS_TEST_ENVguard effectively reduces noise in test output while preserving error logs. This is a practical enhancement to developer experience.Also applies to: 103-105
test/server.test.ts (1)
1-119: LGTM - Consistent quote style applied.The conversion to double quotes maintains consistency with the broader codebase style standards. No functional changes.
lib/cache/session-cache.ts (1)
21-21: LGTM - Quote style normalization.String literals and type annotations consistently use double quotes, aligning with project conventions.
Also applies to: 48-48, 86-86, 95-95, 107-107, 113-113
test/prompts-codex.test.ts (1)
1-261: LGTM - Quote convention applied consistently.All string literals updated to double quotes, maintaining consistency across the test suite.
test/prompts-opencode-codex.test.ts (1)
1-329: LGTM - Style consistency maintained.Quote style updated throughout to align with project standards. Test logic remains intact.
lib/prompts/opencode-codex.ts (3)
10-13: LGTM - Import cleanup removes unused dependencies.Removing unused imports (
getOpenCodeCacheKey, file utility functions) improves code clarity.
41-146: Quote style updates applied consistently.String literals updated to double quotes throughout, aligning with project conventions.
31-128: Review comment is factually incorrect—threshold mismatch undermines the claim.The review states the complexity exceeds "16/15," but your Biome configuration sets
maxAllowedComplexity: 25. The claimed threshold of 15 does not exist in your linting rules. Even if the function reaches the stated complexity of 16, it would fall well within the configured limit of 25 and would not trigger a pipeline failure.The
noExcessiveCognitiveComplexityrule is also set to"warn"level, not an error, so it wouldn't block the pipeline regardless.Likely an incorrect or invalid review comment.
test/response-handler.test.ts (1)
1-112: LGTM! Formatting changes align with project style.The conversion from single to double quotes throughout the test file is purely cosmetic and preserves all test logic.
lib/utils/clone.ts (2)
26-31: LGTM once deepClone is fixed.This helper correctly handles edge cases and will inherit the safer semantics once the
deepClonefunction is fixed.
38-40: LGTM once deepClone is fixed.Simple wrapper that will inherit the safer behavior once
deepCloneis corrected.test/codex-fetcher.test.ts (1)
1-278: LGTM! Test updates align with compaction integration.The formatting changes and pluginConfig additions correctly reflect the broader PR changes. Test logic is preserved and expectations are properly updated.
lib/types.ts (2)
20-34: LGTM! Clean interface extensions for compaction.The new optional fields for enabling and configuring compaction are well-documented and backward-compatible.
192-193: LGTM! Session state extensions for compaction.The optional fields for compaction base system and summary items are appropriately scoped to session state.
lib/cache/cache-warming.ts (1)
1-156: Formatting changes look good; consider addressing complexity warning.The formatting updates are clean. However, the pipeline flagged
warmCachesOnStartup(line 32) with excessive cognitive complexity (28, max: 15). Consider extracting helper functions for cache warming attempts to improve maintainability.test/cache-warming.test.ts (1)
1-242: LGTM! Test formatting and mock updates are consistent.The quote style changes and logger mock enhancement align with the broader PR updates. All test logic is preserved.
test/codex-metrics-command.test.ts (1)
1-350: LGTM! Import refinements and formatting updates are sound.The type-only import for
RequestBody, addition ofresetCacheMetrics, and quote style changes are all appropriate. Test logic is preserved.test/codex-compaction.test.ts (1)
1-95: Solid test coverage; consider clarifyingextractTailAfterSummarysemantics.The test suite provides comprehensive coverage of the compaction helpers. The past review comment about
extractTailAfterSummarytest naming remains relevant—the tests assert "tail after the last user message" rather than detection of a specific summary marker. If the implementation evolves to key off a summary prefix, updating the test names and expectations would improve clarity.Based on learnings.
test/request-transformer.test.ts (1)
639-1314: Compaction, config, and ID-filtering integration tests are comprehensive and match transformer behaviorThe new and expanded
describe("compaction integration"),describe("CODEX_MODE parameter"), and “Integration: Complete Config Scenarios” blocks exercise:
- Manual
/codex-compactand auto-compaction decisions (modes, token thresholds, tools removal).- Prompt cache key derivation (host key vs metadata, fork-aware keys, deterministic fallbacks, metadata edge cases).
- Model normalization, reasoning config defaults/overrides, text verbosity, and
includelogic for encrypted reasoning.- Message ID stripping vs preservation across single and multi-turn histories, including mixed/custom ID formats.
- Tool remap vs Codex bridge behavior under
codexMode=true/false, including OpenCode prompt filtering rules.- Numerous edge cases (empty/undefined input, malformed content arrays, odd tool arrays, unicode/long content).
These tests provide strong regression coverage for
transformRequestBody’s most complex paths and match the production semantics shown inlib/request/request-transformer.ts. From a behavior standpoint, this suite looks solid.lib/request/response-handler.ts (1)
9-87: SSE parsing and JSON conversion look consistent and safe for the intended stream format
parseSseStreamandconvertSseToJsonnow:
- Consume the entire SSE stream, log it when enabled, and robustly scan for
response.done/response.completedevents.- Fall back to returning the original stream text (with original headers and status) when no final event is found, instead of throwing.
- Set an explicit JSON
content-typewhen a final payload is successfully extracted.Within the constraints of the expected SSE format, this is a reasonable and resilient implementation.
test/cache-metrics.test.ts (1)
17-225: Cache metrics tests thoroughly cover recording, reset, auto-reset, and reportingThe updated suite:
- Uses the public
recordCacheHit/Miss/Eviction,resetCacheMetrics,autoResetCacheMetrics, andgetCachePerformanceReporthelpers as intended.- Validates per-cache and overall counters, hit rates, and evictions for hits/misses/evictions flows.
- Exercises reset behavior (manual and auto via fake timers/system time), including “no reset when interval not passed”.
- Asserts on performance report summaries and all three recommendation conditions, plus dedicated bridge-decision metrics.
This gives good confidence that the metrics collector and helper API behave correctly across normal and edge scenarios.
test/compaction-executor.test.ts (1)
10-132: Compaction executor tests correctly pin auto vs command behavior and session wiringThese tests verify that
finalizeCompactionResponse:
- Rewrites assistant output in auto mode with the “Auto compaction triggered (…)” note and
CODEX_SUMMARY_PREFIX, while preserving HTTP status, status text, and headers.- Populates
metadata.codex_compactionfields (mode, reason, total/dropped turns) correctly for both auto and command modes.- Gracefully handles payloads without assistant output (keeps
output: []while still writing compaction metadata).- Invokes
SessionManager.applyCompactionSummarywith the expectedSessionContextand a summary that includesCODEX_SUMMARY_PREFIX.This looks well aligned with the compaction executor implementation and should guard against regressions in the compaction finalization flow.
lib/utils/input-item-utils.ts (1)
16-125: InputItem helpers look correct and cohesiveThe extraction, role predicates, and turn-counting helpers are consistent with the
InputItemshape and will make downstream compaction/session logic clearer. No functional issues spotted here.lib/request/codex-fetcher.ts (2)
21-42: pluginConfig integration and deps wiring look goodAdding
pluginConfigtoCodexFetcherDepsand threading it through totransformRequestForCodexis consistent with the new compaction-config options and keeps the public surface explicit. No issues here.
53-63: Compaction and session context handling is wired correctlyUsing
transformRequestForCodexto computesessionContextand optionalcompactionDecision, then passingsessionContextthrough to header construction and later tofinalizeCompactionResponseandrecordSessionResponseFromHandledResponse, keeps session and compaction state aligned. ThehasToolsandrequestInitderivation also handles the “no transform” case safely.Also applies to: 72-75
test/session-manager.test.ts (5)
6-28: Session creation and prefix/prompt-cache tests match new behaviorThe
createBodyhelper and the initial tests (returns undefined when disabled,initializes session,maintains prefix across turns) exercise the fork‑agnostic path correctly:
conversation_idmetadata drives sessionKey andprompt_cache_key.applyRequestwiring tobody.prompt_cache_keyandstate.lastInputis asserted.- Prefix reuse vs. regeneration is covered via the “regenerates cache key when prefix differs” test.
No changes needed here.
Also applies to: 30-69
99-159: Hostprompt_cache_keyfallback and reuse tests look correctThe
falls back to prompt_cache_key when metadata missingandreuses session when prompt_cache_key matches existingtests now exercise the host‑cache‑key path explicitly, including reusing the same session when onlyprompt_cache_keyis provided. This matches the updatedgetContextlogic.
161-219: Fork‑specific sessions and compaction scoping are well coveredThe fork tests do a good job of pinning the new semantics:
creates fork-specific sessions with derived cache keysverifiesconv-fork::fork::<id>for bothalphaandbetaforks and shows reuse on repeat alpha requests.scopes compaction summaries per fork sessionvalidates that summaries and compacted history are isolated per fork, and that the merge order (base system → summary → latest user tail) is as intended.These tests should protect against regressions in fork keying and per‑fork compaction behavior.
221-248: TTL and capacity behavior tests align with SESSION_CONFIGThe eviction and capacity tests now rely on
SESSION_CONFIG.IDLE_TTL_MSandSESSION_CONFIG.MAX_ENTRIES, matching the implementation:
- TTL test directly manipulates
lastUpdatedto force eviction.- Capacity test ensures
totalSessionsnever exceeds the configured max and thatrecentSessionsis capped.No functional issues here.
137-140: Gitleaks “generic API key” warning is a false positiveThe hard‑coded
"persistent_key_789"value is a synthetic test cache key, not a real credential. No change needed, but if you want to silence future scanners you could rename it to something like"test_cache_key_persistent"to avoid the “key” heuristic.test/fetch-helpers.test.ts (1)
1-437: Fetch helper tests comprehensively cover new behaviorThe updated tests:
- Validate the URL helpers, header construction (including prompt cache key → conversation/session headers and x-api-key stripping), and token refresh paths.
- Exercise
transformRequestForCodexwith and without body, invalid JSON, and the new compaction options (assertingcompaction.originalInputand pluginConfig propagation).- Cover both response paths: SSE → JSON for non‑tool responses, passthrough for tool responses, and detailed
handleErrorResponseenrichment branches.This suite should give good confidence in the new compaction-aware request/response flow.
lib/session/session-manager.ts (4)
18-33:computeHashandextractLatestUserSlicematch compaction needs
computeHashandextractLatestUserSliceprovide consistent hashing and “latest user slice” extraction usingcloneInputItemsandisUserMessage. This aligns with how compaction later mergesbaseSystem + summary + tail, and avoids mutating original inputs.
195-227: Fork-aware session and prompt cache key derivation look consistentUsing
buildSessionKey(conversationId, forkId)forSessionState.idandbuildPromptCacheKey(conversationId, forkId)forpromptCacheKeyensures:
- Base sessions remain keyed by the conversation ID when no forkId is present.
- Forked sessions get deterministic, namespaced keys (
<conversation>::fork::<forkId>), matching expectations in the tests.This matches the test semantics in
test/session-manager.test.ts.
230-320: Compaction summary/history integration is correct and side-effect scoped
applyCompactionSummaryandapplyCompactedHistory:
- Respect
context?.enabledand an explicitopts?.skipflag.- Store a cloned base system and a deep-cloned summary as a
usermessage.- Merge
baseSystem + summary + latest-user-tailintobody.input, usingextractLatestUserSliceandcloneInputItems.This matches the expectations in both compaction executor tests and
test/session-manager.test.tsand correctly confines compaction state to the active session.
361-397: Pruning logic correctly usesSESSION_CONFIGand logs evictionsThe updated
pruneSessions:
- Evicts sessions idle longer than
SESSION_CONFIG.IDLE_TTL_MS.- Enforces
SESSION_CONFIG.MAX_ENTRIESby evicting the oldest sessions, with clear debug/warn logs.This matches the new tests and centralizes the limits via
SESSION_CONFIG.lib/compaction/codex-compaction.ts (1)
25-84: Compaction command detection and serialization logic look solid
approximateTokenCount,detectCompactionCommand, andserializeConversationbehave coherently:
- Token approximation via total text length / 4 is simple and safe.
- Command detection only inspects the latest user turn, normalized (lowercased, optional
/or?stripped) againstCOMMAND_TRIGGERS.- Transcript serialization walks the conversation, filters to user/assistant, then truncates from the end up to
DEFAULT_TRANSCRIPT_CHAR_LIMIT, preserving chronological order and trackingdroppedTurns.This aligns with the compaction behavior described in tests and is easy to reason about.
|
Handling remaining CodeRabbit review comments:
With these follow-ups tracked, we are resolving the outstanding review threads from PR #20. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/README.md(5 hunks)
🔇 Additional comments (1)
test/README.md (1)
35-85: Well-organized test coverage documentation.The expanded descriptions for each test file (auth, config, request-transformer, response-handler, logger) clearly communicate scope and coverage areas. The formatting is consistent and readable.
… edit workflow files
|
/oc In test/README.md around lines 112 to 117, the docs reference |
|
/oc In test/README.md around lines 19 to 31 (and specifically update the Test |
…paths - Update stale test counts to reflect actual numbers: * auth.test.ts: 16 → 27 tests * config.test.ts: 13 → 16 tests * request-transformer.test.ts: 30 → 123 tests * logger.test.ts: 5 → 7 tests * response-handler.test.ts: unchanged at 10 tests - Fix broken configuration file paths: * config/minimal-opencode.json (was config/minimal-opencode.json) * config/full-opencode.json (was config/full-opencode.json) Both configuration files exist in the config/ directory at repository root.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/workflows/ci.ymlis excluded by none and included by none.github/workflows/formatting.ymlis excluded by none and included by none.github/workflows/opencode.ymlis excluded by none and included by noneeslint.config.mjsis excluded by none and included by nonepackage.jsonis excluded by none and included by nonespec/lint-warnings-nonfatal.mdis excluded by none and included by none
📒 Files selected for processing (1)
test/README.md(4 hunks)
🔇 Additional comments (2)
test/README.md (2)
19-31: pnpm command alignment is complete and consistent.All test running instructions and CI guidance have been successfully migrated from npm to pnpm. The replacements are consistent across all sections (Running Tests, Adding New Tests, CI/CD).
Also applies to: 109-110
35-35: Test count headers verified—all accurate.Cross-check against actual test files confirms all documented counts are correct:
- auth.test.ts: 27 tests ✓
- config.test.ts: 16 tests ✓
- request-transformer.test.ts: 123 tests ✓
- response-handler.test.ts: 10 tests ✓
- logger.test.ts: 7 tests ✓
The documentation is in sync with the test files.
1 participant
No description provided.