Repository navigation
feat(mcp): write per-Agent native fields from the source - #579
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
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: 2bd8f337eb
ℹ️ 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".
| func literalSecret(key string, value any) string { | ||
| switch v := value.(type) { | ||
| case string: | ||
| if v != "" && (sensitiveKey.MatchString(key) || hasURLPassword(v)) && !hasInterpolation(v) { |
There was a problem hiding this comment.
A sensitive option is accepted whenever its value contains any interpolation-looking substring, rather than when the whole value is a valid reference for the selected Agent. For example, options.opencode.oauth.clientSecret: "actual-secret-{env:DOCS_SECRET}" passes and writes the plaintext prefix into a potentially committed native file; Claude also accepts OpenCode's {env:...} syntax and receives an unusable literal. Validate the complete value against the target Agent's supported reference syntax instead of using hasInterpolation.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 719aa39. literalSecret now accepts a credential only when the whole value is an environment reference in that Agent's own syntax (envRefPrefix: ${VAR} for Claude/Grok, {env:VAR} for OpenCode/Kilo, ${env:VAR} for clients that use it). Text around a reference, another Agent's syntax, and any credential in Codex or Antigravity options are refused. Tests: TestOptionsRefusePlainTextSecrets ("text around a reference", "another Agent's syntax", "an Agent without references"). Docs, schema and skill reference updated.
| // Options are other Agents' native per-server fields Skillshare has no setting for, keyed | ||
| // by Agent, such as Codex's default_tools_approval_mode. They are written as given. | ||
| Options map[string]map[string]any `yaml:"options,omitempty" json:"options,omitempty"` |
There was a problem hiding this comment.
When a project-mode Kilo server puts a credential reference under options.kilocode, the new field is never considered by Server.usesEnv(), so checkScope allows the sync. For example, options.kilocode.oauth.clientSecret: "{env:DOCS_SECRET}" is written even though Kilo ignores the entire project config when it contains such a reference, causing every server in that file to disappear. Include native option references in the Kilo project restriction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 719aa39. The Kilo Code project check now also refuses when options.kilocode holds a reference (hasReference), not only the portable env/headers. Test: TestKiloCodeProjectRefusesReferencesInOptions.
| // Fields this config wrote earlier and no longer sets go, unless Pi changed them since. | ||
| if target == "pi" && want != nil && managed && owned.Owner == source.ConfigPath { | ||
| // Fields this config wrote earlier and no longer sets go, unless the Agent changed them since. | ||
| if want != nil && managed && owned.Owner == source.ConfigPath { |
There was a problem hiding this comment.
When an Agent changes an option after sync and the source then removes it, this generalized check sets pruneConflict before conflict resolutions are processed. A replace resolution resets ownership later but never clears that boolean, so preview returns the same conflict and the dashboard's Replace action cannot unblock synchronization; importing also omits Agent-specific fields. Recompute or suppress the prune conflict after an explicit replacement resolution.
AGENTS.md reference: AGENTS.md:L36-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 719aa39. When a replace resolution takes over the entry, the fields changed in the Agent since sync are pruned and the conflict is cleared, so the source wins. Test: TestDroppedOptionChangedInAgentConflicts now replaces through PreviewMutation and expects timeout in Fields.Removed.
| if err != nil || s.Disabled { | ||
| return out, err | ||
| } | ||
| return withOptions(target, s, out) |
There was a problem hiding this comment.
If an unmanaged native entry already has matching portable fields, the adoption path compares only managedEntry, reports adopt, and performs no write even though this rendered entry contains new or different options. For example, an existing Claude entry with the same URL but no timeout remains unchanged when the source first specifies options.claude.timeout; only a second sync applies it. Adoption should consider these option differences and write them during the first sync.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 719aa39. When sync adopts a matching entry whose options differ, the takeover becomes an update that writes them, instead of waiting for the next sync. Test: TestAdoptionWritesOptions.
- 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 719aa3911d
ℹ️ 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".
| switch v := value.(type) { | ||
| case string: | ||
| prefix := envRefPrefix(agent) | ||
| ref := prefix != "" && strings.HasPrefix(v, prefix) && strings.HasSuffix(v, "}") && envName.MatchString(v[len(prefix):len(v)-1]) | ||
| if v != "" && (sensitiveKey.MatchString(key) || hasURLPassword(v)) && !ref { |
There was a problem hiding this comment.
Fresh evidence after the prior string-reference fix: when YAML decodes an unquoted credential such as options.opencode.oauth.clientSecret: 123456 as an integer, optionValue accepts it but this switch never checks it, so sync writes the plaintext credential into the potentially committed Agent file. Recursing into mappings also drops a sensitive parent key such as credentials, allowing nonsensitive child names to bypass the same check; reject non-string scalars under sensitive keys and preserve sensitivity through nested mappings.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9824397. literalSecret now carries sensitivity down through mappings and lists, so fields under a sensitive name such as credentials are checked too, and a number under a sensitive name is refused as a plain-text credential. Tests: TestOptionsRefusePlainTextSecrets ("a number", "under a sensitive name").
| "directTools": {"$ref": "#/$defs/directTools"}, | ||
| "tools": {"$ref": "#/$defs/tools"}, | ||
| "piOptions": {"type": "object", "description": "Pi built-in per-server options. Known built-in fields are validated; unknown fields are preserved. pi-mcp-adapter-only fields are refused: set tool lists in tools; exposure and toolExposure cannot be combined with tools."}, | ||
| "options": {"type": "object", "propertyNames": {"enum": ["claude", "codex", "cursor", "vscode", "opencode", "kilocode", "grok", "antigravity", "amp", "claude-desktop", "cline", "copilot", "factory", "gemini", "goose", "junie", "kiro", "lmstudio", "warp", "windsurf", "omp"]}, "additionalProperties": {"type": "object"}, "description": "Native per-server fields of other Agents, keyed by Agent, written into that Agent's entry as given, such as codex: {default_tools_approval_mode: approve} or claude: {timeout: 600000}. Fields Skillshare writes from the server's own settings are refused; values are strings, numbers, booleans, lists of those, or mappings of them. Credentials must be a whole-value environment reference in the Agent's own syntax, such as ${VAR} or {env:VAR}, never plain text; Codex and Antigravity options cannot hold one. Set Pi's fields in piOptions. Sync removes an option it wrote once the source drops it."}, |
There was a problem hiding this comment.
The schema constrains each Agent value only to an object, leaving every nested value unrestricted even though the runtime and this description reject null, nested lists, and lists of mappings. Consequently, schema-backed editors accept configurations such as options.claude.timeout: null, but LoadSource later rejects them through optionValue; add a recursive option-value definition that matches the runtime rules.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9824397. Added recursive optionScalar/optionValue definitions that match optionValue at runtime (no null; lists hold only scalars; mappings recurse), and each Agent's fields now reference them. Test: TestSchema_MCPOptionValuesMatchRuntime.
…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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98243970e2
ℹ️ 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".
| file may be committed. Codex and Antigravity read no references there, so their options | ||
| cannot hold a credential. |
There was a problem hiding this comment.
This says only Codex and Antigravity cannot hold credential references, but envRefPrefix also returns an empty prefix for Claude Desktop, Goose, Junie, LM Studio, and Warp, so literalSecret rejects them too. A supported source such as options.goose.apiKey: "${GOOSE_KEY}" therefore appears valid here—and in the schema description, built-in skill, and translated copies—but LoadSource fails; describe the rule generically or list every affected client in all copies.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf95e37 (docs only). The reference, its ja/ko/zh-Hans/zh-Hant copies, the built-in skill reference and the schema description now name every Agent whose options cannot hold a credential: Codex, Antigravity, Claude Desktop, Goose, Junie, LM Studio and Warp, matching envRefPrefix. Website build passes for all locales.
The docs named only Codex and Antigravity, but Claude Desktop, Goose,
Junie, LM Studio and Warp read no environment references there either,
so a source such as options.goose.apiKey: "${GOOSE_KEY}" looked valid
while LoadSource refused it.
1 participant
Type
proposals/only — see CONTRIBUTING.md)None of these fits: this is a maintainer feature for the v0.26 milestone, implemented from #578.
Linked Issue
Closes #578
Summary
A server's new
optionsfield holds native per-server fields that have no portable form, keyed by Agent, and sync writes them as given:args/env),nulland arrays of tables,options.pi(usepiOptions), disabled entries, and credentials as plain text (environment references such as${VAR}/{env:VAR}are required, since the Agent's file may be committed).Native fields checked against official docs: Codex
default_tools_approval_modeandtools.<tool>.approval_mode, OpenCodetimeout, Claude Codetimeout(documented for.mcp.json).Checks run (devcontainer)
make check(fmt, lint, unit, integration)go test ./internal/mcp/..., including newoptions_test.go(written failing first)vitestforMCPServerDialog.test.tsxandmcpView.test.tspython3 scripts/ai-context.py checktools.<tool>.approval_modesubtable written and removed, OpenCodetimeoutwritten, plain-text OpenCodeoauth.clientSecretrefused and{env:…}acceptedNot run: the full UI test suite and UI build, E2E runbooks. Not verified: whether Claude Code reads
timeoutfrom~/.claude.json(the docs only name.mcp.json).Not covered
--optionsCLI flag, a dashboard editor, and import of these fields (deferred).config.toml, so Codex credentials cannot go inoptions; its dedicated fields such asbearer_token_env_varremain the way.Checklist
make check) — for code changes