fix(responses): make reasoning replay restart-safe and observable (#950) - #1126
fix(responses): make reasoning replay restart-safe and observable (#950)#1126ZachDreamZ wants to merge 4 commits into
Conversation
…dge-jun#950) DeepSeek thinking mode requires the assistant's original reasoning_content on every tool-call continuation. The lidge-jun#971 replay cache is in-memory only, so a proxy restart mid-round still loses recovery, and there is no privacy-safe way to see when a bare tool-call continuation is about to be serialized. - Opt-in disk spill (OPENCODEX_REASONING_REPLAY_PERSIST=1, optional OPENCODEX_REASONING_REPLAY_FILE override): bounded, TTL'd, atomically written with best-effort 0600 perms; rehydrated at boot. Default stays memory-only. - Privacy-safe diagnostics: getReasoningReplayStats() exposes counters and bounds only; recordBareToolCallSerialization() counts the exact 400 shape per model; openai-chat logs a throttled counter line (never reasoning text) when a bare tool-call continuation is serialized for a preserveReasoningContentModels provider. - Regression tests: restart round-trip, TTL filter on reload, corrupt-file tolerance, entry-cap on reload, stats privacy, and the wire-level bare serialization counter.
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe reasoning replay cache now supports optional persistent storage, bounded and TTL-filtered rehydration, privacy-safe diagnostics, and serialization counters. OpenAI Chat continuation paths report missing cached reasoning through throttled warnings. Robustness tests cover persistence and diagnostics. ChangesReasoning replay robustness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAIChatAdapter
participant ReasoningReplayCache
participant WarningLogger
OpenAIChatAdapter->>ReasoningReplayCache: Record bare tool-call serialization
ReasoningReplayCache-->>OpenAIChatAdapter: Return aggregate cache counters
OpenAIChatAdapter->>WarningLogger: Emit throttled privacy-safe warning
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/responses/reasoning-replay-cache.ts`:
- Around line 78-86: Replace the duplicated path logic in defaultPersistPath
with a dependency-free shared config-directory helper, then update both
defaultPersistPath and config.getConfigDir() to call that helper. Preserve the
existing OPENCODEX_HOME and home-directory resolution behavior while ensuring
both locations use the same shared routing path.
- Around line 153-157: The reasoning replay cache must enforce TTL in both
memory and persisted spill data. Update the expiry and startup-validation paths
around the cache eviction logic and spill-loading code to reject future-dated
timestamps, mark the spill dirty when entries expire or fail validation, and
rewrite the spill after loading so removed records are not retained on disk. Add
regressions covering spill contents after expiry and loading a record with a
future finite entry timestamp.
- Around line 202-205: Update the spill-file write flow around tmpPath,
writeFileSync, and renameSync to use a unique temporary filename and create it
with mode 0o600. Apply restrictive permissions at creation time, then rename the
secured temporary file into persistPath; do not rely on the post-rename
chmodSync for protection.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b81d6fa3-6021-4561-8014-a37cd07222ac
📒 Files selected for processing (3)
src/adapters/openai-chat.tssrc/responses/reasoning-replay-cache.tstests/reasoning-replay-robustness.test.ts
| /** Mirror config.getConfigDir() resolution (OPENCODEX_HOME or ~/.opencodex) without importing config. */ | ||
| function defaultPersistPath(): string { | ||
| const raw = process.env.OPENCODEX_HOME?.trim(); | ||
| let base: string; | ||
| if (!raw) base = join(homedir(), ".opencodex"); | ||
| else if (raw === "~") base = homedir(); | ||
| else if (raw.startsWith("~/") || raw.startsWith("~\\")) base = join(homedir(), raw.slice(2)); | ||
| else base = raw; | ||
| return join(base, "reasoning-replay-cache.json"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the shared configuration directory resolver.
Line 78 duplicates config.getConfigDir() behavior instead of using the shared configuration layer. If that resolver changes, config and the replay spill can resolve OPENCODEX_HOME to different directories.
Extract a dependency-free config-directory helper and use it from both locations. As per path instructions, changes must not bypass shared routing/config layers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/responses/reasoning-replay-cache.ts` around lines 78 - 86, Replace the
duplicated path logic in defaultPersistPath with a dependency-free shared
config-directory helper, then update both defaultPersistPath and
config.getConfigDir() to call that helper. Preserve the existing OPENCODEX_HOME
and home-directory resolution behavior while ensuring both locations use the
same shared routing path.
Source: Path instructions
| if (now() - entry.at >= TTL_MS) { | ||
| entries.delete(key); | ||
| totalBytes -= entry.bytes; | ||
| misses += 1; | ||
| return undefined; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce TTL for persisted records at rest.
Line 153 removes an expired entry only from memory. It does not schedule a spill rewrite. Lines 229-233 also skip expired records during startup without rewriting the file. The reasoning text can therefore remain on disk indefinitely.
Line 231 also accepts a future finite timestamp. now() - entryAt is negative, so that record can bypass expiry until the system clock reaches that future value.
Mark the spill dirty when records expire or fail validation. Rewrite it after loading. Reject timestamps where entryAt > now(). Add regressions that inspect the spill after expiry and load a future-dated record. Based on PR objectives, persisted entries must remain TTL-bounded.
Also applies to: 229-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/responses/reasoning-replay-cache.ts` around lines 153 - 157, The
reasoning replay cache must enforce TTL in both memory and persisted spill data.
Update the expiry and startup-validation paths around the cache eviction logic
and spill-loading code to reject future-dated timestamps, mark the spill dirty
when entries expire or fail validation, and rewrite the spill after loading so
removed records are not retained on disk. Add regressions covering spill
contents after expiry and loading a record with a future finite entry timestamp.
| const tmpPath = `${persistPath}.tmp`; | ||
| writeFileSync(tmpPath, JSON.stringify(payload), "utf8"); | ||
| renameSync(tmpPath, persistPath); | ||
| try { chmodSync(persistPath, 0o600); } catch { /* best-effort on platforms that ignore chmod */ } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'reasoning-replay-cache\.ts$' . || true
echo "== relevant file excerpt =="
file="$(fd 'reasoning-replay-cache\.ts$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,260p' "$file" | cat -n
fi
echo "== search related fs.writeFile/writeFileSync/renameSync usage =="
rg -n "writeSync|writeFileSync|renameSync|chmodSync|uniqueFile|tmp|temporary|persist" src || true
echo "== package/runtime/package manager hints =="
for f in package.json bun.lockb bun.lock package-lock.json pnpm-lock.yaml yarn.lock deno.json; do
[ -e "$f" ] && { echo "--- $f"; sed -n '1,120p' "$f"; }
doneRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -u
target="src/responses/reasoning-replay-cache.ts"
echo "== file exists =="
if [ -f "$target" ]; then
echo yes
echo "== line count =="
wc -l "$target"
echo "== excerpt lines 180-220 =="
sed -n '180,220p' "$target" | nl -ba -v180
else
echo no
echo "== tracked files named similar =="
git ls-files | rg 'reasoning-replay-cache\.ts$|reasoning.*replay.*cache' || true
fi
echo "== focused fs calls in target =="
rg -n "writeFileSync|renameSync|chmodSync|persistPath|tmpPath|create|crypto|randomUUID" "$target" || true
echo "== package/runtime hints =="
for f in package.json bun.lockb bun.lock; do
[ -e "$f" ] && { echo "--- $f"; sed -n '1,120p' "$f"; }
doneRepository: lidge-jun/opencodex
Length of output: 16243
🌐 Web query:
Bun 1.3 Buffer writeFileSync mode option string encoding third argument
💡 Result:
In Bun, there is no native Bun.writeFileSync function. For operations that require synchronous file writing or Node.js compatibility, Bun provides an implementation of node:fs.writeFileSync [1][2]. When using node:fs.writeFileSync in Bun, the third argument follows standard Node.js fs.writeFileSync behavior [1][3]: 1. Mode: The mode option (e.g., 0o666) is provided within an options object as the third argument. It affects the file permissions only when the file is newly created [1][3]. 2. Encoding: If the third argument is provided as a string, it is interpreted as the encoding (e.g., 'utf8', 'ascii') [4][5][3]. 3. Options Object: Alternatively, you can provide an object as the third argument to specify both mode and encoding (along with other flags) [1][5][3]. If you are using Bun's native, optimized file I/O API (Bun.write), it is asynchronous and does not use a writeFileSync style signature. Instead, Bun.write() accepts a destination and data, and returns a promise [2][6]. Example using node:fs: import { writeFileSync } from 'node:fs'; // Using string as third argument for encoding writeFileSync('file.txt', 'data', 'utf8'); // Using object for mode and encoding writeFileSync('file.txt', 'data', { encoding: 'utf8', mode: 0o644 }); Example using native Bun.write (asynchronous): await Bun.write('file.txt', 'data');
Citations:
- 1: https://bun.com/reference/node/fs/writeFileSync
- 2: https://bun.com/docs/runtime/file-io
- 3: https://nodejs.org/api/fs.html
- 4: https://bun.com/reference/node/fs/writeFile
- 5: https://bun.com/reference/node/fs/promises/writeFile
- 6: https://bun.com/docs/guides/write-file/basic
Set restrictive permissions before publishing the spill file.
In src/responses/reasoning-replay-cache.ts:202-205, writeFileSync creates ${persistPath}.tmp without an explicit mode, so a local 022 umask can make it readable. renameSync then publishes that file before chmodSync(persistPath, 0o600) runs. Use a unique temp filename with mode: 0o600 and then rename it into place.
Proposed fix
- const tmpPath = `${persistPath}.tmp`;
- writeFileSync(tmpPath, JSON.stringify(payload), "utf8");
- renameSync(tmpPath, persistPath);
- try { chmodSync(persistPath, 0o600); } catch { /* best-effort on platforms that ignore chmod */ }
+ const tmpPath = `${persistPath}.${crypto.randomUUID()}.tmp`;
+ writeFileSync(tmpPath, JSON.stringify(payload), { encoding: "utf8", mode: 0o600 });
+ try { chmodSync(tmpPath, 0o600); } catch { /* best-effort on platforms that ignore chmod */ }
+ renameSync(tmpPath, persistPath);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tmpPath = `${persistPath}.tmp`; | |
| writeFileSync(tmpPath, JSON.stringify(payload), "utf8"); | |
| renameSync(tmpPath, persistPath); | |
| try { chmodSync(persistPath, 0o600); } catch { /* best-effort on platforms that ignore chmod */ } | |
| const tmpPath = `${persistPath}.${crypto.randomUUID()}.tmp`; | |
| writeFileSync(tmpPath, JSON.stringify(payload), { encoding: "utf8", mode: 0o600 }); | |
| try { chmodSync(tmpPath, 0o600); } catch { /* best-effort on platforms that ignore chmod */ } | |
| renameSync(tmpPath, persistPath); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/responses/reasoning-replay-cache.ts` around lines 202 - 205, Update the
spill-file write flow around tmpPath, writeFileSync, and renameSync to use a
unique temporary filename and create it with mode 0o600. Apply restrictive
permissions at creation time, then rename the secured temporary file into
persistPath; do not rely on the post-rename chmodSync for protection.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb683abd3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const entryAt = typeof entry.at === "number" && Number.isFinite(entry.at) ? entry.at : at; | ||
| if (at - entryAt >= TTL_MS) continue; | ||
| rememberReasoningAt(entry.callId, entry.text, typeof entry.scope === "string" ? entry.scope : undefined, entryAt); |
There was a problem hiding this comment.
When persistence is enabled, entries that have passed the 60-minute TTL are only skipped while loading; the JSON spill file is left untouched. A normal sequence of recording reasoning, flushing, and then restarting after the TTL leaves the raw reasoning text on disk indefinitely until some unrelated successful write happens, so the opt-in disk spill is not actually TTL-bounded. Rewrite or unlink the spill file when expired entries are dropped, or schedule a purge at the expiry time.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| persistLastError = err instanceof Error ? err.message : String(err); | ||
| } |
There was a problem hiding this comment.
If persistence is enabled and a failure happens after the temp file is written but before the rename completes (for example, another proxy sharing the same OPENCODEX_HOME races on the fixed .tmp path or the target is locked), this catch only records lastError and leaves reasoning-replay-cache.json.tmp containing raw reasoning text. That temp file is never TTL-swept and is written before the final chmod, so use the existing atomic-write pattern with a unique 0600 temp file and truncate/unlink cleanup on errors.
Useful? React with 👍 / 👎.
| persistTimer = setTimeout(() => { | ||
| persistTimer = undefined; | ||
| writePersisted(); | ||
| }, PERSIST_DEBOUNCE_MS); |
There was a problem hiding this comment.
With persistence enabled, rememberReasoningForCall() only schedules a debounced write, and production code never calls flushReasoningReplayCache() (only the tests do). If the proxy is restarted or terminated in the 750 ms window after the model emits the tool call, the reasoning is still only in memory and the next process cannot replay it, so the restart-safe path still loses exactly the mid-round continuation this option is meant to protect; write synchronously here or hook the existing restart/shutdown paths to flush first.
Useful? React with 👍 / 👎.
| mkdirSync(dirname(persistPath), { recursive: true }); | ||
| } catch { /* best-effort */ } | ||
| const tmpPath = `${persistPath}.tmp`; | ||
| writeFileSync(tmpPath, JSON.stringify(payload), "utf8"); |
There was a problem hiding this comment.
When the default spill path is under OPENCODEX_HOME, this direct write creates reasoning-replay-cache.json without going through recordOwnedConfigPath, and that filename is not in the initial uninstall manifest. Users who enable persistence can therefore leave a raw-reasoning cache behind after ocx uninstall (or turn cleanup into a partial/refused removal if this file created the config dir first). Register this path with the config ownership manifest before writing, like the other config-dir state files.
Useful? React with 👍 / 👎.
1 participant
What
Continues the #950 fix (PR #971) on the Codex /
/v1/responsespath: the reasoning replay cache is currently in-memory only, so a proxy restart mid-round still loses recovery for a DeepSeek thinking-mode tool round, and there is no privacy-safe signal when a bare tool-call continuation is about to be serialized.Changes
OPENCODEX_REASONING_REPLAY_PERSIST=1(optionalOPENCODEX_REASONING_REPLAY_FILEoverride, default<config dir>/reasoning-replay-cache.json, mirroringgetConfigDir()resolution). Bounded by the same entry/byte/TTL caps as memory, written atomically (tmp + rename) with best-effort 0600 perms, rehydrated at boot. Default stays memory-only — the privacy stance of fix(responses): keep DeepSeek reasoning_content on tool-call continuations (#950) #971 is unchanged unless explicitly opted in.getReasoningReplayStats()exposes counters and bounds only (entries, bytes, hits, misses, bare-serialization counts per model, persistence state) — never reasoning text.recordBareToolCallSerialization(model)counts the exact 400 shape (assistant message withtool_calls, preserved model, noreasoning_contentre-attached) in both the compacted-history path and the orphan-repair path, with a throttledconsole.warncontaining counters only.tests/reasoning-replay-robustness.test.ts): restart round-trip, TTL filter on reload, corrupt-file tolerance, entry-cap on reload, stats never contain reasoning text, persistence off by default, and the wire-level bare-serialization counter.Verification
bun teston the reasoning suites: 45 pass / 0 fail.bun run typecheckclean;bun run privacy:scanpasses.opencode-free/deepseek-v4-flash-free(tool-call continuation replay 200, no 400s).Notes
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Diagnostics
Performance & Reliability