Skip to content

fix(codex): restore reasoning-object whitelist deleted by #13717 - #14065

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
HouMinXi:restore/codex-reasoning-whitelist-13643
Sep 18, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
HouMinXi:restore/codex-reasoning-whitelist-13643

Conversation

@HouMinXi

@HouMinXi HouMinXi commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Restores the Codex reasoning-object key whitelist from #13643, which #13717 (e7999c477b) deleted. Unknown OpenRouter-style keys (enabled, max_tokens, ...) are stripped before the Responses API wire.

Conflict with current tip: keep the post-#13643 force-rule (getForcedReasoningEffort still wins) and keep enabled: false as a connection-default override. File-size freeze for open-sse/executors/codex.ts moved 1530 -> 1552 with a restore note.

Related to #14062. #14062 stays open.

Test plan

  • tests/unit/codex-reasoning-wire-whitelist.test.ts: 8/8.
  • Defect injection: hard-code clientDisabledReasoning = false -> 2 fail; restore -> 8/8.
  • npm run typecheck:core: clean.

⚠️ base-red inherited: #13866 — the 16 unit-test failures reproduce on the clean tip 7cc454d9 (6/6 sampled) and are identical across PRs with disjoint content; none touches what this PR changes.

…zapw#13643)

The Codex executor now whitelists the wire `reasoning` object to `effort`/`summary` before dispatch instead of spreading whatever the client sent, and maps `reasoning.enabled === false` to `effort: "none"` when no more specific effort was requested. OpenRouter-style keys (`enabled`, `max_tokens`, `exclude`) were reaching the Responses API and 400-ing the whole combo target with `Unknown parameter: 'reasoning.<key>'`.

The precedence chain keeps an explicit per-request effort ahead of `enabled: false`, and the strip matches the siblings already removed in the same function (`truncation`, `user`, `prompt_cache_retention`).

Validated as a combined board first (this PR merged with the 11 siblings of the same batch on the release tip): eslint on every changed file with the suppressions file, typecheck:core, check:open-sse-typecheck, complexity, cognitive-complexity, changelog-integrity, i18n new-key coverage, docs-sync, migration-numbering, provider-consistency and a duplicate-identifier audit all green, plus 275 passing / 0 failing focused node:test cases across the 28 test files the batch touches. Then re-validated alone on the fresh tip before this merge: conflicts re-resolved, file sizes rebaselined for this PR's own growth, eslint and this PR's focused tests re-run.

Thanks @HouMinXi!
@diegosouzapw
diegosouzapw merged commit de428ce into diegosouzapw:release/v3.8.51 Sep 18, 2026
11 of 16 checks passed
diegosouzapw added a commit that referenced this pull request Sep 18, 2026
The release tip went red on check:file-size after #14065 (fix(codex): whitelist
reasoning object keys before the wire, #13643) merged with +1 line in
open-sse/executors/codex.ts and no baseline entry — the PR->release fast-gates
do not run check:file-size. Every merge-train boarding after it inherits the
red, so the drift is absorbed once at the tip (owner-approved train-rebaseline
policy, 2026-09-18). Measured clean: check:file-size passes on the tip with
this entry.
@diegosouzapw

Copy link
Copy Markdown
Owner

Reviewed. The restore itself is correct and clean — I diffed your head against the current tip and the only change to codex.ts is the isolated whitelist + clientDisabledReasoning re-addition; every legitimate change that landed on codex.ts after the original #13643 (the getForcedReasoningEffort precedence from #13556, stripCodexPassthroughRejectedParams, the WS onclose hardening) is intact. All 8 new tests pass, and I re-ran 70 pre-existing Codex executor tests plus typecheck:core — all clean.

One concrete issue that will fail CI's file-size gate: the config/quality/file-size-baseline.json update sets open-sse/executors/codex.ts to 1552, but the gate script counts lines via content.split("\n").length (which is wc -l + 1 for a file with a trailing newline), so it actually measures 1553. Please bump both entries (and the 1530->1552 note) to 1553 — that's the only outstanding item; I'll re-check once it's pushed.

Separately, flagging for coordination rather than as something to fix here: #13224 rewrites this same block into a new applyCodexReasoningSelection() helper that spreads the raw client reasoning object without a key whitelist, so if it merges after this without picking up the fix, the OpenRouter-key 400 comes back a third time. I'll note this on that PR's review too.

@HouMinXi
HouMinXi deleted the restore/codex-reasoning-whitelist-13643 branch September 19, 2026 13:08
fenix007 pushed a commit to fenix007/OmniRoute that referenced this pull request Sep 23, 2026
Adapt OmniRoute diegosouzapw#14065 at a932b87 to the frozen v3.8.48 executor. Preserve explicit effort precedence and validate HTTP/WebSocket request bodies.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…zapw#13643) (diegosouzapw#14065)

The Codex executor now whitelists the wire `reasoning` object to `effort`/`summary` before dispatch instead of spreading whatever the client sent, and maps `reasoning.enabled === false` to `effort: "none"` when no more specific effort was requested. OpenRouter-style keys (`enabled`, `max_tokens`, `exclude`) were reaching the Responses API and 400-ing the whole combo target with `Unknown parameter: 'reasoning.<key>'`.

The precedence chain keeps an explicit per-request effort ahead of `enabled: false`, and the strip matches the siblings already removed in the same function (`truncation`, `user`, `prompt_cache_retention`).

Validated as a combined board first (this PR merged with the 11 siblings of the same batch on the release tip): eslint on every changed file with the suppressions file, typecheck:core, check:open-sse-typecheck, complexity, cognitive-complexity, changelog-integrity, i18n new-key coverage, docs-sync, migration-numbering, provider-consistency and a duplicate-identifier audit all green, plus 275 passing / 0 failing focused node:test cases across the 28 test files the batch touches. Then re-validated alone on the fresh tip before this merge: conflicts re-resolved, file sizes rebaselined for this PR's own growth, eslint and this PR's focused tests re-run.

Thanks @HouMinXi!

Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…pw#14065)

The release tip went red on check:file-size after diegosouzapw#14065 (fix(codex): whitelist
reasoning object keys before the wire, diegosouzapw#13643) merged with +1 line in
open-sse/executors/codex.ts and no baseline entry — the PR->release fast-gates
do not run check:file-size. Every merge-train boarding after it inherits the
red, so the drift is absorbed once at the tip (owner-approved train-rebaseline
policy, 2026-09-18). Measured clean: check:file-size passes on the tip with
this entry.
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.

2 participants