Repository navigation
fix(codex): use ACP recommended model for compatibility - #631
Conversation
madawei2699
left a comment
There was a problem hiding this comment.
Independent review at PR head d6a82154e89c3f5e54bfabecd1fe79ccbe120478: CHANGES REQUIRED — DO NOT MERGE YET. The overall ACP metadata approach is correct, and the CI checks are green, but two model-selection safety cases need fixes.
P1: explicit Human selection can still be overwritten. ApplySessionConfigFallbacksFor skips only when selectedValue != option.CurrentValue. If a Human explicitly selected the uncataloged model that is currently active, the new recommended-value fallback silently changes it. Upstream codex-acp intentionally exposes such uncataloged current models as selectable options. For this new fallback skip when the Human config key is present at all, regardless of equality. Cover explicit same-as-current, explicit different, and unconfigured/Keep-current.
P1: empty description is not proof of a synthetic unknown model. Upstream createModelConfigOption emits description:null for a synthetic uncataloged entry, but catalog entries have description:model.description and that string can be empty. The new Go ACP parser reduces null and empty string to ""; the guard !currentHasDescription can therefore auto-switch a known catalog model with an empty description to the recommended default. Preserve null-vs-empty wire semantics and apply this fallback only with positive uncataloged-entry evidence; otherwise preserve the current model. Add tests: uncataloged null → recommended, catalog "" → unchanged, explicit selection → unchanged, missing/malformed metadata and unadvertised recommendation → unchanged.
Acceptance evidence still missing: the direct Codex CLI probe verifies a manually chosen model, not that the current installed codex-acp emits the negotiated metadata or the Free4Chat Runtime applies it successfully. Please record a sanitized installed-bridge, exact-version ACP handshake/model-options/result and one harmless successful scoped turn for the originally failing scenario. Do not restart the live resident Runtime. If the failing model is present in the catalog but not permitted for the account, this fallback won't solve it; don't claim otherwise.
No new model-ID mappings, config schema expansion, polling or transport changes. Update the SAME PR. DO NOT MERGE. Return updated head SHA and validation results.
|
Addressed at
Validation:
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbb7596fd0
ℹ️ 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".
| JetBrains struct { | ||
| Air struct { | ||
| Version int `json:"version"` | ||
| RecommendedValue string `json:"recommendedValue"` |
There was a problem hiding this comment.
Ignore malformed recommendations without dropping session controls
When an ACP provider sends a non-string recommendedValue (for example the numeric value exercised by the new malformed-metadata test), unmarshalling it into this string returns an error, so parseSessionControls returns nil and discards every otherwise-valid mode and config option. The test only verifies that no fallback is applied and therefore misses that explicit Human selections and the controls UI become unavailable; decode this optional extension independently so malformed recommendation metadata alone is ignored.
Useful? React with 👍 / 👎.
Cause
A direct Codex CLI request using the configured model
gpt-6.1-solreturned HTTP 400 with the authenticated ChatGPT account. The same minimal request succeeded with another model. Runtime previously relied on model-ID mappings instead of the current ACP session's recommended model.Change
Remove Codex model-ID compatibility mappings from the Runtime provider registry. During ACP initialization, advertise the JetBrains AIR
recommendedValuecapability. For a current model that the bridge explicitly marks withdescription: nullas synthesized and uncataloged, Runtime applies the recommendation only when it is advertised in that same session. Explicit Human model selections are always preserved, even when equal to the current value. Catalog models with an empty description, missing/malformed metadata, and unadvertised recommendations are also preserved. Null-vs-empty description evidence remains internal to the ACP adapter.Verification
go test ./internal/harness ./internal/runtime -count=1— pass.@agentclientprotocol/codex-acp@1.12.0: the negotiated session reported currentgpt-6.1-solas a synthesizeddescription: nulloption, advertised recommendationgpt-6-astra, and Runtime applied that selection. A harmless no-tools Codex turn returned the expected token — pass.