Skip to content

🤖 feat: add custom model parameter override fields to custom model editing#3453

Open
Neppkun wants to merge 4 commits into
coder:mainfrom
Neppkun:max_output_token_ui
Open

🤖 feat: add custom model parameter override fields to custom model editing#3453
Neppkun wants to merge 4 commits into
coder:mainfrom
Neppkun:max_output_token_ui

Conversation

@Neppkun
Copy link
Copy Markdown
Contributor

@Neppkun Neppkun commented Jun 4, 2026

Summary

Adds max_output_tokens, temperature, and top_p editing to the Custom Models edit flow in Settings, so users can configure per-model provider overrides in-app instead of editing ~/.mux/providers.jsonc manually.

Background

providers.jsonc already supports per-model parameter overrides and runtime consumption, but the Settings UI had no controls for these fields. This expands on #2743 by exposing those existing capabilities in the custom model editing UX.

Implementation

  • Added modelParameters to provider config payloads returned to the UI.
  • Added a new typed provider mutation: providers.setModelParameters for setting/clearing per-model overrides.
  • Extended ProviderService to:
    • surface filtered modelParameters in getConfig
    • persist/clear overrides at provider.modelParameters[modelId]
    • remove empty entries/maps when cleared
  • Extended custom model editing UI (ModelsSection + ModelRow) to:
    • show edit fields for max_output_tokens, temperature, top_p
    • validate ranges (>0 int, 0..2, 0..1)
    • save overrides along with model metadata updates
    • clear stale override entries when model IDs are renamed

Demo

https://owo.whats-th.is/4nedYg5.mp4

Validation

  • make typecheck
  • bun test src/node/services/providerService.test.ts src/browser/features/Settings/Sections/ModelsSection.test.ts
  • Attempted make static-check in this environment, but it is blocked by missing local tools: shfmt, shellcheck, and hadolint.

Risks

  • Main risk is save sequencing when both model metadata and model-parameter overrides are updated in one edit operation.
  • Mitigated by targeted service/UI helper tests and by falling back to config refresh on mutation failures.

Generated with mux • Model: openai:gpt-5.3-codex • Thinking: high • Cost: $4.54

@Neppkun
Copy link
Copy Markdown
Contributor Author

Neppkun commented Jun 4, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1870af287e

ℹ️ 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".

Comment thread src/node/services/providerService.ts Outdated
Comment thread src/browser/features/Settings/Sections/ModelsSection.tsx
Comment thread src/node/services/providerService.ts
@Neppkun
Copy link
Copy Markdown
Contributor Author

Neppkun commented Jun 4, 2026

Addressed Codex findings in a follow-up commit (a1c80adfc):

  1. discussion_r3357996320 (preserve non-editable parameters when clearing UI fields)

    • Updated ProviderService.setModelParameters to remove only editable keys (max_output_tokens, temperature, top_p) and keep any remaining passthrough/non-editable keys when the model is still configured.
    • Added regression test: clears editable keys without dropping non-editable model parameters.
  2. discussion_r3357996326 (clear overrides when removing custom models)

    • Updated custom model remove flow in ModelsSection to clear the matching model-parameter entry (optimistic + persisted cleanup call to setModelParameters with null overrides after model removal).
    • Added helper coverage for removing model-parameter entries.
  3. discussion_r3357996329 (do not drop all overrides because one entry is malformed)

    • Hardened normalizeModelParameters to recover valid per-model entries when the full object parse fails.
    • Added regression test: retains valid model parameter entries when another entry is malformed.

Validation run:

  • make typecheck
  • bun test src/node/services/providerService.test.ts src/browser/features/Settings/Sections/ModelsSection.test.ts

@Neppkun
Copy link
Copy Markdown
Contributor Author

Neppkun commented Jun 4, 2026

@codex review

Addressed all prior findings and resolved those threads; please re-review.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a1c80adfc1

ℹ️ 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".

Comment thread src/browser/features/Settings/Sections/ModelsSection.tsx Outdated
@Neppkun
Copy link
Copy Markdown
Contributor Author

Neppkun commented Jun 4, 2026

Addressed new Codex finding discussion_r3358046353 in commit 8c715feb0.

  • Added migrateModelParameterEntry(...) in ModelsSection and used it in rename flow so legacy non-editable overrides (e.g. top_k, seed) are migrated to the renamed model key before legacy cleanup.
  • Kept editable-field update/clear semantics intact via existing buildUpdatedModelParameters(...) behavior.
  • Added helper tests:
    • rename preserves non-editable overrides and removes legacy key
    • rename merge behavior when destination key already exists

Validation:

  • bun test src/browser/features/Settings/Sections/ModelsSection.test.ts
  • make typecheck

@Neppkun
Copy link
Copy Markdown
Contributor Author

Neppkun commented Jun 4, 2026

@codex review

Addressed the latest finding about preserving hidden parameter overrides on model rename and resolved that thread. Please re-review.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c715feb0c

ℹ️ 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".

Comment on lines +543 to +547
const setOverridesResult = await api.providers.setModelParameters({
provider: providerId,
modelId: trimmedModelId,
overrides,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve extra overrides when persisting renames

When a custom model with hand-edited modelParameters such as top_k or seed is renamed in Settings, the optimistic state migrates those extra keys, but the persisted API sequence does not: this call only sends the three editable fields for the new ID, and the later legacy clear removes the old unconfigured entry entirely. After refresh/restart, the non-editable overrides are lost despite being preserved in the temporary UI state; persist the migrated entry or add a backend rename path before clearing the legacy key.

Useful? React with 👍 / 👎.

@Neppkun
Copy link
Copy Markdown
Contributor Author

Neppkun commented Jun 4, 2026

Addressed the rename persistence bug in commit 29845a7.

Fix summary

  • Added renameFromModelId to providers.setModelParameters so backend can atomically migrate model-parameter entries during rename.
  • Updated ProviderService.setModelParameters to migrate and merge the source entry into the destination before applying editable overrides, preserving non-editable keys (e.g. top_k, seed).
  • Updated the Settings save flow to send renameFromModelId instead of issuing a separate legacy-clear call.
  • Added/updated regression test coverage for rename + non-editable key preservation.

Validation:

  • bun test src/node/services/providerService.test.ts src/browser/features/Settings/Sections/ModelsSection.test.ts
  • make typecheck

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Neppkun
Copy link
Copy Markdown
Contributor Author

Neppkun commented Jun 4, 2026

oh ok

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.

1 participant