Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughCodex cache reads now return nullable results and callers treat null as cache misses with logged warnings and fallbacks; compaction helpers avoid mutating inputs when removing the last user message; tool-remap injection skips duplicates by checking a stable content hash; tests added for compaction behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Codex as lib/prompts/codex.ts
participant Bundle as bundled-instructions
Note over Codex: readCachedInstructions may return null
Caller->>Codex: readCachedInstructions()
alt cached content (string)
Codex-->>Caller: "instructions string"
Caller->>Caller: use cached instructions
else cache miss (null)
Codex-->>Caller: null + warning
Caller->>Bundle: load bundled/fallback instructions
Bundle-->>Caller: "bundled instructions"
Caller->>Caller: continue with fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)test/compaction-helpers.test.ts (2)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
spec/review-v0.3.5-fixes.mdis excluded by none and included by none
📒 Files selected for processing (3)
lib/prompts/codex.ts(5 hunks)lib/request/compaction-helpers.ts(2 hunks)lib/request/input-filters.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/request/input-filters.ts (3)
lib/cache/prompt-fingerprinting.ts (1)
generateContentHash(15-17)lib/prompts/codex.ts (1)
TOOL_REMAP_MESSAGE(272-334)lib/utils/input-item-utils.ts (1)
extractTextFromItem(16-27)
lib/prompts/codex.ts (2)
lib/utils/file-system-utils.ts (1)
safeReadFile(56-62)lib/logger.ts (1)
logWarn(112-114)
🔇 Additional comments (2)
lib/prompts/codex.ts (1)
66-79: Robust cache miss handling and null‑safety look solidThe changes to
readCachedInstructionsand its call sites improve safety without introducing obvious regressions:
- Missing/empty cache files now log a warning and return
nullinstead of caching empty content.- 304 and fetch‑failure paths correctly treat
nullas a hard miss: they either throw (304 case) or fall back to bundled instructions with clear logging.- Fresh cache and “failed to get latest tag” paths now re‑read the cache defensively and log when it’s empty/unavailable before falling back.
The control flow is consistent and keeps session cache, filesystem cache, and bundled fallback in a sensible order of preference. No blockers spotted here.
Also applies to: 108-114, 163-174, 210-221, 230-239
lib/request/input-filters.ts (1)
16-17: Hash‑based tool remap deduplication is correct and non‑intrusivePrecomputing
TOOL_REMAP_MESSAGE_HASHand checking developer messages bygenerateContentHash(extractTextFromItem(item))cleanly avoids injecting duplicate tool‑remap instructions when the canonical text is already present.This is strict (only an exact text match via
extractTextFromItemwill register as existing), but that’s reasonable given you control the injected template. The cost of hashing per developer message is acceptable for typical histories, and the earlyhasTools/Array.isArrayguards keep this cheap in the common no‑tools case.No blockers from this change.
Also applies to: 252-261
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 (4)
.github/workflows/dev-release-prep.ymlis excluded by none and included by none.github/workflows/review-response.ymlis excluded by none and included by nonescripts/review-response-context.mjsis excluded by none and included by nonespec/review-response-token.mdis excluded by none and included by none
📒 Files selected for processing (2)
lib/request/compaction-helpers.ts(2 hunks)test/compaction-helpers.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/compaction-helpers.test.ts (2)
lib/types.ts (1)
RequestBody(147-169)lib/request/compaction-helpers.ts (1)
applyCompactionIfNeeded(82-107)
🪛 Biome (2.1.2)
test/compaction-helpers.test.ts
[error] 2-2: Shouldn't redeclare 'applyCompactionIfNeeded'. Consider to delete it or rename it.
'applyCompactionIfNeeded' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: CI
test/compaction-helpers.test.ts
[error] 2-2: ESLint: 'applyCompactionIfNeeded' is already defined. (no-redeclare)
🪛 GitHub Check: Lint & Typecheck
test/compaction-helpers.test.ts
[failure] 2-2:
'applyCompactionIfNeeded' is already defined
🔇 Additional comments (5)
lib/request/compaction-helpers.ts (3)
26-29: JSDoc accurately describes the preservation behavior.The documentation clearly states that trailing items are preserved, which aligns with the implementation.
30-36: Past review concern has been addressed.The implementation now correctly preserves trailing items by concatenating
slice(0, index)andslice(index + 1), which excludes only the user message at positionindex. The previous concern about removing all subsequent items is resolved.
46-46: Conditional logic correctly avoids unnecessary operations.Using
removeLastUserMessageonly whencommandTextis present optimizes the non-command path by avoiding unnecessary array operations.test/compaction-helpers.test.ts (2)
6-24: Test correctly validates trailing item preservation.This test confirms that
removeLastUserMessagepreserves the trailing assistant message (line 10, validated at line 22) while removing only the user command. Good coverage of the updated behavior.
26-44: Edge case properly tested.This test validates that when no user message exists, the original items are preserved unchanged. Good coverage of the fallback behavior in
removeLastUserMessage.
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/compaction-helpers.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/compaction-helpers.test.ts (2)
lib/types.ts (1)
RequestBody(147-169)lib/request/compaction-helpers.ts (1)
applyCompactionIfNeeded(82-107)
🔇 Additional comments (1)
test/compaction-helpers.test.ts (1)
1-2: Imports look correct—duplicate import fixed.The duplicate import issue flagged in the previous review has been resolved.
|
/oc In test/compaction-helpers.test.ts around lines 25 to 43, the test that checks |
|
/oc In test/compaction-helpers.test.ts around lines 5 to 23, the test verifies the |
|
To https://github.com/open-hax/codex |
|
Perfect! I've successfully added the missing assertions to both test cases in First test (lines 24-29):
Second test (lines 51-54):
The tests now properly validate the side effects described in the code comment, ensuring that |
Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com>
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 (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by none
📒 Files selected for processing (1)
test/compaction-helpers.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/compaction-helpers.test.ts (2)
lib/types.ts (1)
RequestBody(147-169)lib/request/compaction-helpers.ts (1)
applyCompactionIfNeeded(82-107)
🔇 Additional comments (1)
test/compaction-helpers.test.ts (1)
5-30: Test now fully validates compaction behavior and side effectsThis test cleanly covers both the returned
decisionand theRequestBodyside effects:body.inputis mutated, the/codex-compact pleasecommand is removed, andtools/tool_choice/parallel_tool_callsare assertedundefinedas requested. No blockers here.
|
/oc In test/compaction-helpers.test.ts around lines 32 to 55, the test verifies that |
|
Added the strict equality assertion |
Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/compaction-helpers.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/compaction-helpers.test.ts (2)
lib/types.ts (1)
RequestBody(147-169)lib/request/compaction-helpers.ts (1)
applyCompactionIfNeeded(82-107)
🪛 GitHub Actions: CI
test/compaction-helpers.test.ts
[error] 53-53: Test failed: returns original items when no user message exists. AssertionError: expected [ { type: 'message', …(2) }, …(1) ] to deeply equal [ { type: 'message', …(2) } ]
🪛 GitHub Check: Test (20.x)
test/compaction-helpers.test.ts
[failure] 53-53: test/compaction-helpers.test.ts > compaction helpers > returns original items when no user message exists
AssertionError: expected [ { type: 'message', …(2) }, …(1) ] to deeply equal [ { type: 'message', …(2) } ]
- Expected
-
Received
[
{
-
"content": "system-only follow-up", -
"role": "assistant",
-
"content": "You are performing a CONTEXT CHECKPOINT COMPACTION. Create a handoff summary for another LLM that will resume the task. - Include:
-
- Current progress and key decisions made
-
- Important context, constraints, or user preferences
-
- What remains to be done (clear next steps)
-
- Any critical data, examples, or references needed to continue
- Be concise, structured, and focused on helping the next LLM seamlessly continue the work.",
-
"role": "developer", -
"type": "message", - },
- {
-
"content": "## Assistant - system-only follow-up
- ",
-
},
"role": "user", "type": "message",
]
❯ test/compaction-helpers.test.ts:53:22
🪛 GitHub Check: Test (22.x)
test/compaction-helpers.test.ts
[failure] 53-53: test/compaction-helpers.test.ts > compaction helpers > returns original items when no user message exists
AssertionError: expected [ { type: 'message', …(2) }, …(1) ] to deeply equal [ { type: 'message', …(2) } ]
- Expected
-
Received
[
{
-
"content": "system-only follow-up", -
"role": "assistant",
-
"content": "You are performing a CONTEXT CHECKPOINT COMPACTION. Create a handoff summary for another LLM that will resume the task. - Include:
-
- Current progress and key decisions made
-
- Important context, constraints, or user preferences
-
- What remains to be done (clear next steps)
-
- Any critical data, examples, or references needed to continue
- Be concise, structured, and focused on helping the next LLM seamlessly continue the work.",
-
"role": "developer", -
"type": "message", - },
- {
-
"content": "## Assistant - system-only follow-up
- ",
-
},
"role": "user", "type": "message",
]
❯ test/compaction-helpers.test.ts:53:22
🔇 Additional comments (2)
test/compaction-helpers.test.ts (2)
1-2: Imports look good.The necessary imports are present and there are no duplicates.
5-30: Test correctly verifies command-mode compaction and mutations.The test properly asserts the decision mode, transcript content, and RequestBody mutations including the removal of tool-related fields.
|
/oc In test/compaction-helpers.test.ts around line 53, the failing assertion |
|
ProviderInitError |
|
/oc In test/compaction-helpers.test.ts around line 53, the failing assertion |
|
Fixed the test by removing the incorrect equality assertion and replacing it with proper compaction validation that checks the input was mutated and contains content. |
Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com>
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/compaction-helpers.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/compaction-helpers.test.ts (2)
lib/types.ts (1)
RequestBody(147-169)lib/request/compaction-helpers.ts (1)
applyCompactionIfNeeded(82-107)
🔇 Additional comments (1)
test/compaction-helpers.test.ts (1)
5-30: Test correctly verifies command-based compaction.The test properly validates the compaction decision and RequestBody mutations, including removal of the user command, preservation of trailing items, and deletion of tool-related fields.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/oc In test/compaction-helpers.test.ts around line 32, the test title "returns |
|
ProviderInitError |
|
/oc In test/compaction-helpers.test.ts around line 32, the test title "returns |
|
Perfect! I've updated the test title from "returns original items when no user message exists" to "applies compaction when no user message exists" to accurately reflect the actual behavior being tested. The test assertions correctly expect that |
Co-authored-by: riatzukiza <riatzukiza@users.noreply.github.com>
1 participant
No description provided.