-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(responses): omit canonical Codex user metadata and hop request-local 400s #4563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c90d66d
fbf49ea
2530f4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # 260914 — Carry contributor PR #4528 | ||
|
|
||
| ## Why a carry | ||
|
|
||
| PR #4528 (branch fix/codex-forward-user-4527, head d1d8d45f22c8, author RHODIZSECURITY) | ||
| fixes issue #4527. It was stuck for reasons unrelated to its code: a contributor draft | ||
| whose four-box readiness checklist resets on every push, and a Cross-platform CI run | ||
| (34774339026) that failed on exactly one test, "release version line > the in-tree | ||
| version is never behind a released one". | ||
|
|
||
| That failure is stale base, not a defect. The test compares package.json against the | ||
| highest local release tag. The branch sat at in-tree 2.54.0 while v2.54.0 was already | ||
| tagged, so ordering === 0 and the tag does not point at the branch head. dev has since | ||
| opened 2.55.0 against a highest tag of v2.54.0, so the same test passes on a fresh base. | ||
|
|
||
| ## Defect being fixed | ||
|
|
||
| Claude Code sends metadata.user_id; src/claude/inbound.ts maps it onto the Responses | ||
| top-level user field. src/adapters/openai-responses.ts stripped other unsupported | ||
| native-forward fields but left that one on the canonical ChatGPT Codex wire, which | ||
| rejects it with 400 "Unsupported parameter: user". Because a generic 400 was terminal | ||
| for a combo, a request that had already taken a 429 on an earlier target ended the turn | ||
| rather than trying the next healthy one. | ||
|
|
||
| ## Scope boundary | ||
|
|
||
| Only a clear pre-output, target-local incompatibility may fall through to the next | ||
| target. Widening this to retry all 400s would be a defect, not an improvement. | ||
| Cancellation, policy refusals, context overflow, other invalid requests and anything | ||
| after output commitment must stay terminal. | ||
|
|
||
| ## Proof policy | ||
|
|
||
| Local product suite, typecheck, build and install are NOT RUN. Focused runs are | ||
| debugging only and are never cited. The only proof is hosted Cross-platform CI at the | ||
| exact final head SHA. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| # Phase 1 — Reimplement on current dev, audit, publish | ||
|
|
||
| ## Diff level | ||
|
|
||
| Branch codex/carry-4528-codex-forward-user off origin/dev 246b5cab43. The upstream diff | ||
| applies cleanly at that base (39 files, 613+/61-), which is itself the evidence that the | ||
| CI red was the version test alone and not a code conflict. | ||
|
|
||
| Source changes carried: | ||
|
|
||
| - src/adapters/openai-responses.ts — stripCanonicalForwardUser, called only inside the | ||
| existing isCanonicalOpenAiForwardProvider gate, after the prompt-cache strips. | ||
| - src/combos/failover.ts — isRequestLocalTargetIncompatibility: HTTP 400 only, 16,384 | ||
| char bound, generic outer code required, strict JSON parse, error object required, | ||
| inner code string-or-null and generic, leaf type invalid_request_error, only the exact | ||
| "Provider error 400: " wrapper unwrapped with a depth budget of 3. Three accepted | ||
|
Comment on lines
+13
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Document optional error codes accurately.
🤖 Prompt for AI Agents |
||
| shapes. Wired into both comboFailureDecision (hop) and comboFailureCooldownScope. | ||
| - src/vision/plan.ts — requiresVisionPreprocessing replaces the isModelTextOnly call | ||
| sites. Proven-negative capability preprocesses; unknown custom models stay sighted. | ||
| - src/vision/eligibility.ts — canonical Codex consults the generated openai-codex bundle | ||
| before generic row metadata; shallow copy plus cloned vision maps so an injected fetch | ||
| survives enrichment. | ||
| - scripts/generate-model-metadata.ts and src/generated/model-metadata.ts — openai-codex | ||
| retained as a capability-only bundle. | ||
| - src/providers/registry.ts — OPENCODE_ZEN_IMAGE_MODELS records mimo-v2.5-free and | ||
| longcat-2.0-free as positive modality evidence instead of blacklist absence. | ||
| - src/server/responses/core.ts, chat-native.ts, chat-completions.ts and | ||
| src/web-search/index.ts — thread providerName through so one gate applies everywhere. | ||
|
|
||
| ## Audit plan | ||
|
|
||
| Six parallel read-only recon agents: failover classifier bounds, the sanitation | ||
| boundary, vision capability routing and generated-file consistency, structure ownership, | ||
| docs-site locale fidelity, plus one adversarial counter-read of both security boundaries. | ||
|
|
||
| ## Fold list beyond the upstream diff | ||
|
|
||
| Turkish combos.md wording (cikti baslamadan ONCE sonraki uygun hedefe); the stale | ||
| noVisionModels activation row in structure/ops/service-and-sidecars.md; the | ||
| transports/responses.md contract pointer and its whitespace churn; a Zen positive-modality | ||
| note in transports/inventory.md; the English cross-reference; the French before-output | ||
| timing; seven locale failover-table exception rows; and two comment blocks that wrongly | ||
| claimed unknown capability fails closed. | ||
|
|
||
| ## Known limitation, documented not fixed | ||
|
|
||
| On the combo path the classifier never sees more than 500 characters: consumeComboFailure | ||
| passes normalized.safeText, which is redactSecretString(text).slice(0, 500) at | ||
| src/server/responses/core.ts:954. The classifier's own 16,384 bound is the outer belt. | ||
| An envelope fatter than 500 bytes truncates mid-JSON, fails the parse and does not hop. | ||
| That fails closed, and raising it would touch shared redaction and byte-accounting | ||
| contracts outside this carry's scope. | ||
|
|
||
| ## Exit | ||
|
|
||
| Push --no-verify fast-forward only, open against dev with the full template, report the | ||
| PR number, head SHA and the CI run id whose head_sha equals the final head. Do not merge; | ||
| do not close #4528 or #4527. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,7 +200,8 @@ Les échecs d’un combo se répartissent entre ceux qui entraînent un **bascul | |
| | --- | --- | | ||
| | HTTP 401, 403, 404, 408, 429, ou n'importe quel 5xx | Refroidissez la cible et passez à la prochaine cible éligible. | | ||
| | Erreur classée comme erreur d’authentification, d’abonnement, de quota, de limitation de débit, de surcharge ou de serveur en amont | Place la cible en période de refroidissement et bascule, même si le statut seul ne suffit pas. | | ||
| | Annulation client (499), `origin_rejected`, refus de cyber-politique, débordement de contexte ou demande invalide | Arrêtez et renvoyez l'erreur ; une autre cible ne rendrait pas la demande valide. | | ||
| | Annulation client (499), `origin_rejected`, refus de cyber-politique, débordement de contexte ou autre demande invalide | Arrêtez et renvoyez l'erreur ; une autre cible ne rendrait pas la demande valide. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Document the model-lifecycle HTTP 410 exception. The French failure table omits the supported HTTP 410 case where an explicit model end-of-life signal causes a hop and cooldown. Unrelated HTTP 410 responses remain terminal. Add this row before the terminal invalid-request row. 🧰 Tools🪛 LanguageTool[typographical] ~203-~203: Caractère d’apostrophe incorrect. (APOS_INCORRECT) [typographical] ~203-~203: Le préfixe « cyber » est généralement associé au terme qu’il précède. (PAS_DE_TRAIT_UNION) 🤖 Prompt for AI Agents |
||
| | Rejet structuré de `user`, valeur non prise en charge pour `reasoning.effort`/`reasoning_effort`, ou rejet d'entrée d'image propre à un modèle (`param: input`) | Bascule vers la cible admissible suivante avant le début de la sortie, sans délai de refroidissement ; voir Compatibilité des paramètres facultatifs ci-dessous. | | ||
| | Toute autre erreur non classifiée | Arrêtez et renvoyez l'erreur. | | ||
|
|
||
| Une cible sautée entre en temps de recharge pendant 60 secondes par défaut. Si la réponse en amont inclut un | ||
|
|
@@ -365,3 +366,9 @@ message de validation. | |
|
|
||
| L’erreur était terminale plutôt que spécifique à la cible. Corriger une entrée invalide, réduire un contexte surdimensionné, | ||
| gérer un refus de politique ou corriger l’origine de la demande rejetée. Les combos ne sautent pas dans ces cas-là. | ||
|
|
||
| ## Compatibilité des paramètres facultatifs | ||
|
|
||
| Exception aux erreurs 400 terminales : un rejet structuré de `user`, une valeur non prise en charge pour `reasoning.effort`/`reasoning_effort`, ou un rejet d’entrée d’image propre à un modèle (`param: input`) peut faire passer le combo à la cible admissible suivante avant le début de la sortie, sans délai de refroidissement. Le refus de sécurité, l’annulation et une sortie déjà commencée restent non rejouables. | ||
|
|
||
| [Canonical compatibility details](/guides/combos/#request-local-target-compatibility). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,7 +213,8 @@ Combo failures are divided into **hop** failures and **terminal** failures. | |
| | HTTP 401, 403, 404, 408, 429, or any 5xx | Cool the target and hop to the next eligible target. | | ||
| | HTTP 410 with an explicit model end-of-life, retired, deprecated, sunset, decommissioned, or no-longer-available signal | Cool that target and hop. Unrelated 410 responses remain terminal. | | ||
| | Classified authentication, subscription, quota, rate-limit, overload, or upstream-server error | Cool the target and hop, even when the status alone is not sufficient. | | ||
| | Client cancellation (499), `origin_rejected`, cyber-policy refusal, context overflow, or invalid request | Stop and return the error; another target would not make the request valid. | | ||
| | Client cancellation (499), `origin_rejected`, cyber-policy refusal, context overflow, or other invalid request | Stop and return the error; another target would not make the request valid. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Distinguish target-local context overflow from terminal invalid input. The runtime hops for provider-target context overflow because another combo target may have a larger context window. These tables classify every context overflow as terminal. Limit the terminal wording to generic request-level overflow and document the target-local hop case.
📍 Affects 4 files
🤖 Prompt for AI AgentsSource: Path instructions |
||
| | Structured HTTP 400 rejecting optional `user`, an unsupported reasoning effort, or model-scoped image input | Hop before output commitment without cooling; see request-local target compatibility below. | | ||
| | Any other unclassified error | Stop and return the error. | | ||
|
|
||
| When `cooldownMs` is unset, a hopped target uses an upstream fallback: 5 seconds for request-rate | ||
|
|
@@ -227,8 +228,8 @@ fallback for upstream rate-limit codes `1302`/`1305` → the 60-second default. | |
| `Retry-After: 0` remains an immediate upstream directive rather than being replaced by a configured | ||
| cooldown. | ||
|
|
||
| The current request never retries the same attempted target. Later requests skip it until its | ||
| cooldown expires. A `Retry-After` HTTP-date that is already in the past is also preserved as an | ||
| The current request never retries the same attempted target. Later requests skip a cooled target until its | ||
| cooldown expires; request-local compatibility rejections do not cool the target. A `Retry-After` HTTP-date that is already in the past is also preserved as an | ||
| immediate upstream directive, just like `Retry-After: 0`. Set `waitForCooldownMs` to allow a later | ||
| request to wait for the earliest eligible target cooldown, up to that cap on each selection attempt, | ||
| and then make one fresh selection. A request may therefore wait up to `hops × waitForCooldownMs` | ||
|
|
@@ -254,6 +255,12 @@ another provider, which prevents duplicate text and tool execution. If the pre-o | |
| its safety cap without a terminal or output boundary, OpenCodex also commits the current target | ||
| instead of growing memory without a bound. | ||
|
|
||
| ## Request-local target compatibility | ||
|
|
||
| When routing Claude Code to the canonical ChatGPT Codex backend, OpenCodex removes the unsupported top-level `user` metadata field without changing the session/cache key, input messages, tool schemas, or safety identifiers. Public Responses API and noncanonical forward gateways keep that field. | ||
|
|
||
| A combo can also advance after an intact HTTP 400 `invalid_request_error` that specifically rejects `user`, reports `unsupported_value` for `reasoning.effort`/`reasoning_effort`, or reports `param: input` with an exact model-scoped `does not support image inputs` rejection. This is a mismatch for that request, not evidence that the target is unhealthy, so it records no cooldown. This compatibility recovery does not silently change `none` into a different effort or broaden this exception to arbitrary invalid requests. Policy refusals, cancellation and already-committed output remain non-replayable. A single-target request still returns an unresolved upstream rejection. | ||
|
|
||
| ## Default reasoning effort | ||
|
|
||
| `defaultEffort` fills an absent `reasoning.effort` when the combo has a non-null default and the selected target has a known, nonempty supported ladder. If the target supports the configured value, it is retained; otherwise the highest supported rung at or below it is used, or the lowest supported rung when none is lower. Unknown or empty ladders omit the default. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Separate patch application from CI diagnosis.
A clean patch application proves only that the patch has no textual conflict at that base. It does not prove that the failed CI run was caused only by the release-version test, or that no code conflict affected behavior. Cite the failed job output and the hosted run at the final head SHA, or rewrite these lines to state only the patch-application fact.
🤖 Prompt for AI Agents