Skip to content

fix(codex): sanitize Responses replay state - #1868

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.7.8from
dhaern:fix/codex-responses-internal-replay
May 2, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.7.8from
dhaern:fix/codex-responses-internal-replay

Conversation

@dhaern

@dhaern dhaern commented May 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Sanitizes remembered Codex Responses replay state so internal assistant frames such as phase: \"commentary\" are not stored and re-injected into later /responses requests.
  • Keeps Responses stream accounting intact while only treating response.output_text.delta as visible assistant content for logs/replay payloads.
  • Updates the Codex CLI fingerprint body ordering to match the actual Responses payload shape used by Codex OAuth requests.

Why

Recent Codex Responses fixes improved several real edge cases, but together they exposed a gap in replay hygiene:

That replay state should contain durable conversation/tool state, not local runtime phases. If a client sends assistant-side internal frames such as phase: \"commentary\", storing and replaying them can leak hidden working notes back into the next Codex request. This PR keeps the replay behavior from #1750/#1791, but filters non-final assistant phases at the state boundary.

While adding a full pipeline regression for this, the Codex CLI fingerprint also showed a real mismatch: the codex profile still prioritized chat-completions fields like messages, while the active Codex OAuth path uses Responses fields such as input, instructions, store, reasoning, prompt_cache_key, and client_metadata.

Changes

  • Adds a small sanitizer around remembered Responses conversation items:
    • drops assistant messages with explicit non-final phases (commentary, analysis-like runtime frames, etc.);
    • handles both typed Responses messages and role-based message items without type;
    • preserves visible assistant messages, user messages, function calls, and tool outputs.
  • Adjusts Responses SSE accumulation:
    • generic delta values still count toward fallback usage estimates;
    • only response.output_text.delta is accumulated as visible assistant content.
  • Aligns the Codex CLI fingerprint body order with the Responses request shape.
  • Extends tests for replay sanitization, reasoning/function-argument deltas, OAuth Codex pipeline fingerprinting, and existing no-any test helpers.

Validation

  • node --import tsx/esm --test tests/unit/executor-codex.test.ts tests/unit/stream-utilities.test.ts tests/integration/chat-pipeline.test.ts --test-name-pattern 'Codex|codex|reasoning deltas|passthrough'
  • npm run typecheck:core
  • npx eslint --quiet open-sse/config/cliFingerprints.ts open-sse/utils/stream.ts open-sse/services/responsesToolCallState.ts tests/unit/executor-codex.test.ts tests/unit/stream-utilities.test.ts tests/integration/chat-pipeline.test.ts
  • git diff --check

Compatibility

This keeps the replay repairs from #1750 and #1791 in place. The sanitizer only removes assistant messages that explicitly declare a non-final phase, so normal visible assistant messages and tool state continue to replay.

/responses/compact remains JSON-only as handled by #1777; this PR targets the normal Codex /responses replay/fingerprint path.

@dhaern
dhaern requested a review from diegosouzapw as a code owner May 2, 2026 04:10

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the CLI fingerprint configuration by expanding the bodyFieldOrder and introduces a sanitization mechanism to filter out internal assistant commentary from conversation history. It also refines SSE stream processing to ensure only visible text deltas are accumulated for assistant content in logs. Additionally, the test suite is expanded with new integration and unit tests covering OAuth fingerprinting and commentary filtering. I have no feedback to provide.

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.7.8 May 2, 2026 04:16
@diegosouzapw
diegosouzapw merged commit 1e3c085 into diegosouzapw:release/v3.7.8 May 2, 2026
2 checks passed
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks @dhaern for this excellent contribution! 🎉 The replay sanitization and tests are incredibly thorough and safe. It has been successfully merged into the release/v3.7.8 branch and will be included in the upcoming release. We appreciate your effort!

@AveryanAlex

Copy link
Copy Markdown
Contributor

@dhaern @diegosouzapw why this is implemented and merged? Agent forgot what he says and says the same comments again and again:

• Executing Task 1 through a fresh worker subagent first, then I’ll run spec-compliance and code-quality review before moving to the
  next slice.

• Executing Task 1 first through a dedicated worker subagent, then I’ll run the spec-compliance and code-quality review gates before
  moving on.

• Using subagent-driven-development to execute the plan. I’m dispatching Task 1 first because the store contract is the dependency for
  every later setup UI slice.

• Using subagent-driven-development to execute the plan. I’m dispatching Task 1 first so the store contract is corrected before
  touching the setup UI.

Maybe we should revert it? No other routers does it AFAIK.

@dhaern

dhaern commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

@dhaern @diegosouzapw why this is implemented and merged? Agent forgot what he says and says the same comments again and again:

• Executing Task 1 through a fresh worker subagent first, then I’ll run spec-compliance and code-quality review before moving to the
  next slice.

• Executing Task 1 first through a dedicated worker subagent, then I’ll run the spec-compliance and code-quality review gates before
  moving on.

• Using subagent-driven-development to execute the plan. I’m dispatching Task 1 first because the store contract is the dependency for
  every later setup UI slice.

• Using subagent-driven-development to execute the plan. I’m dispatching Task 1 first so the store contract is corrected before
  touching the setup UI.

Maybe we should revert it? No other routers does it AFAIK.

Im gonna check this but this is a simple sanitizer because last fixes added a regression where many internal messages from codex (tested with gpt 5.5 high and xhigh) were escaping and showing during the responses. What platform are you using? And what model?

Anyways next time show proofs, logs and more info before accusing without have a single idea. This PR is mandatory because model was escaping useless output text.

@dhaern

dhaern commented May 9, 2026

Copy link
Copy Markdown
Contributor Author

I checked this again @diegosouzapw @AveryanAlex

The accusation still doesn’t prove that this PR caused the repeated planning text.

#1868 is a narrow sanitizer for Codex Responses replay state. It only drops assistant replay items that explicitly carry phase: "commentary" before they are remembered/re-injected. It does not filter normal assistant text, does not rewrite visible responses, and does not touch unrelated providers.

The original issue was real: internal Codex/OpenCode commentary frames were being stored and sent back upstream in later /responses requests. Reverting this would reintroduce that leak.

The snippets you posted look like normal visible assistant/planning output, not evidence that sanitized phase:"commentary" replay items are leaking. Without a redacted request.input, replay state, provider/model info, or logs showing those lines being re-injected after this sanitizer, there is no basis to blame this PR.

So no, this is not a reason to revert. If you can provide actual payload evidence, I’ll check it. Otherwise this looks like a separate model/prompt/replay-loop issue, not this sanitizer.

@AveryanAlex

Copy link
Copy Markdown
Contributor

Hello @dhaern, sorry for the delay. I don’t think phase: "commentary" items are a leak. They are normal Responses/Codex assistant items, and removing them from replay/request input looks more like the regression.

Evidence:

I also tested this by reverting the commentary stripping and am currently running a patched version with this commit: AveryanAlex@436f2d0. It fixes the issue where the assistant sends near-identical commentary/progress messages repeatedly.

So “reverting would reintroduce a leak” is not accurate. The leak would be showing commentary as final visible output, not sending commentary back to the model with its phase. Stripping commentary from replay may actually explain repeated preamble/progress text like reported here: #1868 (comment)

@dhaern

dhaern commented May 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @AveryanAlex, after re-checking I agree that stripping phase: "commentary" from Responses input/replay was too aggressive.

However, this is already fixed in current release/v3.8.0: the responsesInputSanitizer / sanitizeResponsesInputItems path is gone, so commentary is no longer stripped from replay/input.

The remaining behavior only prevents non-response.output_text.delta deltas from being accumulated as final visible assistant content, which is the correct boundary: preserve structured commentary, but don’t render it as final output.

HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
Poid-ZA pushed a commit to Poid-ZA/OmniRoute that referenced this pull request Aug 5, 2026
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants