Repository navigation
feat(ui): edit other Agents' MCP options in the server dialog - #580
Conversation
Some MCP settings have no portable form, such as Codex's default_tools_approval_mode or Claude Code's per-server timeout. Sync kept them only while an entry survived, so a recreated entry (for example after a rename) lost them, and every machine needed them added by hand again. A server's `options` now holds these fields per Agent, and sync writes them as given. The field ledger that piOptions used is extended to every Agent, so sync removes an option it wrote once the source drops it, and reports a conflict when the Agent changed it since. Options are refused when they could break the entry or leak a secret: fields Skillshare writes (including portable names some Agents rename), values a native format cannot write back (null, arrays of tables), options.pi (piOptions covers Pi), disabled entries, and credentials as plain text, since the Agent's file may be committed. The dashboard has no editor for options yet but keeps them on save. Refs: #578
- A credential option passed if it merely contained "${", so text around
a reference, or another Agent's syntax, was written out as plain text.
It must now be a whole-value reference in the Agent's own syntax, and
Codex and Antigravity options, which read no references, cannot hold one.
- The Kilo Code project check looked only at portable env references, so
an {env:VAR} in options reached a project file Kilo refuses to load.
- A replace resolution left the dropped-option conflict in place, so the
plan stayed blocked; replace now lets the source win and prunes those
fields.
- Adopting an entry that already matched skipped writing its options until
a later sync; the takeover now writes them.
…he schema - YAML reads an unquoted secret such as clientSecret: 123456 as a number, which the credential check skipped, so sync wrote it out as plain text. A mapping under a sensitive name such as credentials also hid its plainly named fields. Sensitivity now carries down through mappings and lists, and a number under a sensitive name is refused. - The schema allowed any value inside an Agent's options, so editors accepted null or nested lists that LoadSource then rejects. A recursive optionValue definition now matches the runtime rules.
The dialog only carried options through unchanged, so setting a field such as Codex default_tools_approval_mode or Claude Code timeout still meant editing mcp.yaml by hand. One Agent picker with a JSON editor keeps the dialog short however many Agents are ticked; ticked Agents come first, and an unticked Agent that still has settings stays listed so they can be cleared. Anything but a JSON object blocks saving; the server checks the fields themselves.
- The picker listed targets from the order, which includes accounts, so a ticked account produced an options key the server refuses, and the Agent the account writes to was not offered. Options are keyed by Agent, so every Agent but Pi is offered, ticked ones first. - An unticked Agent left the list as soon as its text was emptied, so its settings could not be cleared and retyped. - The editor was hidden on the Skillshare tab although its server takes options too, and hidden invalid text could block Save there. - The docs still said the dashboard had no options editor.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f183ef5b4c
ℹ️ 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".
| const served = serveSkillsTarget(validation.words) ?? ''; | ||
| const twice = serve ? order.filter((x) => selectedTargets.has(x) && skillTargets.synced.has(x)) : []; | ||
| // Options are keyed by Agent, not account, so every Agent is offered: ticked ones first, then those with settings. | ||
| const rank = (x: string) => (selectedTargets.has(x) ? 0 : draft.agentOptions[x]?.trim() ? 1 : 2); |
There was a problem hiding this comment.
When the selected target is a supported account such as codex-work with agent: codex, selectedTargets contains the account name rather than codex, so Codex receives the lowest rank and the editor defaults to Claude. Settings entered in that initially shown editor are consequently saved under options.claude and never applied to the selected Codex account. Resolve selected account targets to their underlying Agents before calculating the rank.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 018b6b4. The MCP page now provides each account's Agent through an MCPAccountAgents context (empty in a project, where accounts do not apply), and the picker ranks Agents by the ticked targets resolved to their Agents, so a server reaching only codex-work opens the Codex editor and saves under options.codex. Test: "saves an account's settings under its Agent".
| await user.click(screen.getByRole('combobox', { name: 'Agent' })); | ||
| await user.click(screen.getByRole('option', { name: (name) => name.startsWith('Claude') && !name.startsWith('Claude Desktop') })); | ||
| await user.clear(screen.getByLabelText('Claude: other settings')); | ||
| expect(screen.getByLabelText('Claude: other settings')).toBeInTheDocument(); |
There was a problem hiding this comment.
In the select-Claude-then-clear scenario, this rendered test only verifies that an editor remains visible; it never demonstrates that the draft survives or that the correct options are submitted. Retype a value and assert the save payload, or remove the test, because dashboard rendered tests must protect user data and must not test collapse/expand or text visibility. wiki/frontend.mdL56-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 018b6b4. The test now clears Claude's settings, retypes them, saves, and asserts the payload's options: { claude: { timeout: 120000 } } instead of the editor staying visible.
The picker ranked Agents by the ticked targets, which hold account names, so a server reaching only codex-work opened the Claude editor and what was typed there was saved under options.claude. The MCP page now provides each account's Agent, and the ranking uses it. The retype test now asserts the saved options rather than the editor staying visible.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 018b6b45e0
ℹ️ 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".
| if (!text.trim()) continue; | ||
| let value: unknown; | ||
| try { value = JSON.parse(text); } catch { value = undefined; } | ||
| if (!value || typeof value !== 'object' || Array.isArray(value)) agentOptionsError ||= t('mcp.agentOptionsInvalid', { agent: targetLabel(agent) }); |
There was a problem hiding this comment.
When the dashboard is using a non-English locale and the user enters a syntactically valid but forbidden object such as {"url":"https://x"} or a plaintext credential, this branch accepts it and enables Save, leaving the Go validator to reject it. The resulting Skillshare-owned error has no option-specific mapping in describeError, so MCPServerDialog renders the English backend guidance verbatim; validate these cases client-side or translate the backend errors before displaying them.
AGENTS.md reference: AGENTS.md:L51-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2597968. describeError now maps the server's four option refusals the dialog can reach (a field Skillshare writes, a plain-text credential, a credential in an Agent that reads no references, an unsupported value) to translated messages naming the Agent and field, in all 11 locales. The remaining option errors cannot come from this dialog: it offers no Pi, no non-Agent key, and no options on an off switch. Field rules stay on the server so the dialog and mcp.yaml share one validator. Test: "translates the server's refusals of Agent options" in mcpView.test.ts.
… language The dialog leaves field rules to the server, but its refusals (a field Skillshare writes, a plain-text credential, a credential in an Agent that reads no references, an unsupported value) reached the dialog in English. describeError now maps each to a translated message naming the Agent and field. The other option errors cannot come from the dialog: it offers no Pi, no non-Agent key, and no options on an off switch.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2597968c1a
ℹ️ 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".
| /** The server's refusals of a field in options, by i18n key; the groups are the Agent, the field and, for a credential, the reference to use. */ | ||
| const agentOptionErrors: [RegExp, string][] = [ | ||
| [/options\.([\w-]+) cannot set ([\w.-]+); Skillshare writes that field/, 'mcp.agentOptionsTaken'], | ||
| [/options\.([\w-]+)\.(\S+) holds a credential as plain text; make the whole value an environment reference, (\S+)$/, 'mcp.agentOptionsSecret'], |
There was a problem hiding this comment.
When the dashboard uses a non-English locale and a user saves valid JSON such as {"api key":"secret"}, the backend recognizes the key as sensitive (internal/mcp/import.go:26) and returns a refusal containing options.claude.api key, but this \S+ capture cannot span the space, so describeError falls through and displays the English backend guidance. This whitespace-key case is fresh evidence beyond the prior mapping fix; match the field using the fixed message suffix instead so every JSON property name accepted by the editor is localized.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing code for this one. This is the third review round, and the case only affects the language of an error message: the save is still refused correctly, with the server's English guidance shown. No Agent's native MCP field name contains a space, so a key such as api key is not something a supported workflow produces. Recorded under "Not covered" in the PR description.
1 participant
Type
proposals/only — see CONTRIBUTING.md)None of these fits: this is the dashboard half of the v0.26
optionsfeature from #578. Follows #579 (merged);mainwas merged in after its squash, so the diff shows only the dashboard change.Linked Issue
Refs #578
Summary
The server dialog gets an Other Agent settings section for the per-Agent
optionsadded in #579:Checks run (devcontainer)
make check(Go fmt, lint, unit, integration), on feat(mcp): write per-Agent native fields from the source #579's headvitest run src/components/mcp src/i18n: 92 passed, including 3 new dialog tests (save typed options and keep another Agent's; clear and retype an unticked Agent; block non-object JSON)tsc -b --noEmit,pnpm run lint(0 errors),pnpm run buildpython3 scripts/ai-context.py checkskillshare-code-reviewwith fix: its findings (account keys, retyping an unticked Agent, hidden on the Skillshare tab, stale docs) are fixed in f183ef5Not run: E2E runbook.
Not covered
"api key") that the server refuses shows the server's English message instead of a translated one. No Agent's native field name has a space; the save is still refused.Checklist
make check) — for code changes