feat(models): configure ApplyPatch editing per model - #5533
Sun-GLiang wants to merge 4 commits into
Conversation
Generated-by: OpenAI Codex
Remove the unused Codex ApplyPatch protocol declaration and update the headless DeepSeek wire contract for default-enabled portable patch editing. Generated-by: OpenAI Codex
The hosted patch contract passes. The unrelated Side Chat cleanup scenario and its five disposal tests pass locally; trigger another CI run because upstream rerun permissions are unavailable. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
The core design holds up: single shared APPLY_PATCH_MODELS list (all 22 IDs verified present in the model-metadata snapshot — no typos), override ?? list precedence with strict codec validation, profile frozen per backend generation so mid-turn consistency holds, backend invalidation unconditional on committed mutations, replay normalization covering all six profile-direction pairs, Write/Edit never exposed alongside apply_patch, and plan-mode/agent-catalog gating consistent.
One P1 inline, one P2 and two P3s below.
P2 — the headless-coding-v1 system prompt tells the model to prefer tools that routing just removed (packages/runtime-host/src/server/hosted-execution-tool-profile.ts:40). HEADLESS_CODING_V1_TOOL_NAMES includes Write, Edit and apply_patch, so routing strips Write/Edit on enabled models — but the prompt still says "Prefer Read, Glob, and Grep for inspection, Edit and Write for file changes" and never names apply_patch. Every such session opens pointing at absent tools → wasted invalid calls until the model discovers apply_patch from the schema list. Pre-existing for openai-structured on main; this PR widens it to every enabled model on the path. ① normal path. Fix: make the sentence conditional on the resolved profile, or generalize it ("file-editing tools").
P3 — applyPatchProtocol: 'codex-v4a-freeform' on the DeepSeek adapter is a dead, misleading declaration (provider-registry.ts:945). resolveApplyPatchProfile only consults the field for the 'openai-structured' check; the freeform-vs-portable choice is driven by runtime.customTools, so DeepSeek always resolves portable-v4a despite the declaration. No runtime effect today (open-responses can't carry custom tools anyway — portable is correct), but the declaration invites a future reader to honor it and silently switch the wire format. Remove it, or make the freeform branch consult the field.
P3 — the Settings selector can't return a model to "automatic" (provider-capability-editor.tsx:100-112). The model is tri-state (list-default / explicit on / explicit off) but the UI is binary — once touched, the stored override can never go back to undefined, unlike the vision selector's auto/enabled/disabled at :83-98. A pinned true survives later removal from APPLY_PATCH_MODELS — intended for explicit choices, but the user had no way to say "default". Add an auto option writing undefined.
| description: CODEX_PATCH_DESCRIPTION, | ||
| parameters: | ||
| profile.kind === 'codex-v4a-freeform' ? z.string() : z.object({ patch: z.string() }), | ||
| providerTool: |
There was a problem hiding this comment.
P1 — Code Mode + an enabled profile leaves the model with zero file-editing tools. Chain: this strips Write/Edit (:64) and marks the tool providerTool: {kind:'codex-apply-patch'} here (structured keeps openai-apply-patch). In Code Mode projectToolModePlan reduces the model-facing set to [exec] (ai-sdk-turn.ts:416-435), and nestableToolSnapshot excludes any tool with providerTool !== undefined (:456-462) — so apply_patch appears in neither the provider set nor exec's nested catalog. Repro: enable Code Mode in Settings, open a session on an openai-codex connection with a default-listed model — the model's only tool is exec with no editing capability, silently. Note the same hole exists on main for openai-structured, but this PR turns it on by default for the whole Codex OAuth fleet — the blast-radius expansion is this feature's. Fix belongs at the projection layer: either emit a nestable client-executed apply_patch (portable {patch: string} form, providerTool: undefined) when toolMode === 'code_mode' — the impl already accepts that shape (builtin-tools.ts:281-292) — or keep Write/Edit in the nested catalog when the profile's tool can't nest. ① normal user path.
There was a problem hiding this comment.
Fixed in b5712c2. Code Mode now projects provider-native openai-apply-patch and codex-apply-patch tools into the portable client-executed { patch: string } shape before nestability filtering. This keeps the exclusive ApplyPatch editing surface while making apply_patch available through exec. I added end-to-end Code Mode coverage for both the OpenAI structured and Codex freeform paths.
|
A design thought prompted by the mid-session model-switch question — feel free to take or leave; none of this blocks the review above. Consider making the patch-editing choice session-bound, stamped at creation, instead of per-model. What it buys:
The one thing it needs to answer — mid-session switch to a model that emits worse patches — is benign: patch capability is a quality gradient, not a hard failure, since the impl validates patches and returns tool errors on malformed input. The session keeps its promised surface; a weaker model just recovers more often. That seems strictly better than carrying mixed-history machinery to handle a case that's a soft degradation anyway. If you keep the per-model design, two smaller suggestions: give the selector an |
Keep ApplyPatch available inside Code Mode through a portable nested projection, align the headless prompt with routed tools, remove the dead DeepSeek protocol declaration, and restore the model setting's automatic state.
|
@Astro-Han Addressed the concrete review feedback in b5712c2:
Verification: affected Core/Runtime/Runtime Host/Desktop typechecks passed; 132 focused runtime tests passed; the two headless prompt contract tests passed; Storybook production build and the ModelCapabilities / ModelParameterSave browser scenarios passed. For the session-bound design suggestion: I am keeping the current per-model design in this PR for now while I think through model-switch and wire-protocol compatibility. I will treat that as a separate design decision rather than fold it into this review fix. |
Summary
Model settings could not control ApplyPatch, and the Codex OAuth path filtered it out. Add an Enabled/Disabled setting per connection and model, with defaults from a shared model support list; unlisted models default to disabled and explicit user choices take precedence.
Enabled models receive ApplyPatch instead of Write/Edit: native structured patches where supported, Codex custom freeform patches, or portable function calls for other interfaces including DeepSeek. Preserve tool history across these protocols and when patch editing is disabled.
ApplyPatch defaults by model
ApplyPatch is enabled by default for these 22 model IDs:
gpt-5-codexgpt-5.1gpt-5.1-codexgpt-5.1-codex-minigpt-5.1-codex-maxgpt-5.2gpt-5.2-codexgpt-5.3-codexgpt-5.3-codex-sparkgpt-5.4gpt-5.4-minigpt-5.4-nanogpt-5.4-progpt-5.5gpt-5.6gpt-5.6-solgpt-5.6-terragpt-5.6-lunagpt-6-astradeepseek-v4-flashdeepseek-v4-flash-vision-expdeepseek-v4-proAll other model IDs default to disabled. Dated snapshots ending in
-YYYY-MM-DDinherit the default of their base model ID. An explicit Enabled/Disabled choice saved for a connection and model overrides these defaults.Verification
6401e1297, including desktop E2E, Storybook smoke, layout checks, and installed CLI release-candidate validation.Before / after (production settings components rendered in Storybook):
AI use
Tool(s) and scope: OpenAI Codex implemented the model setting, provider routing, history conversion, tests, and this PR description. The code commit includes
Generated-by: OpenAI Codex.Checklist
Does this PR entail a change in behavior?