Repository navigation
🤖 fix: load managed plugin skills and MCP servers only after their review - #6587
Conversation
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
86efdd7 to
2a92074
Compare
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
2a92074 to
685cbe5
Compare
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
15a711b to
a2d05f6
Compare
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
a2d05f6 to
3b84a2c
Compare
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 3b84a2ca2d
ℹ️ 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".
Managed registry rows in tests now carry the consent an install review would store, computed with the production plugin discovery, SKILL.md parse and MCP loader (reviewedComponentConsent). Without it the #6498 load gate correctly refuses their skills and MCP servers. Signed-off-by: Thomas Kosiewski <tk@coder.com>
Extract named helpers instead of growing the gated functions: pluginSkillConsentGate and noComponentConsent (componentConsent.ts), ownerMcpPolicy/isReviewedServer/registryRowName/mcpPolicySnapshot (registry.ts), managedPluginFields (discovery.ts), the SKILL.md reads and readCandidateSkill (agentSkillsService.ts), and readContainedSkillFile (agent_skill_list.ts). Behavior is unchanged. Signed-off-by: Thomas Kosiewski <tk@coder.com>
A managed skill or MCP server with no record, or a record for another fingerprint, does not load; a matching record loads it; a frontmatter or parser-visible server change under the same name is refused; unmanaged plugins are not gated. One install-service test strips the records of a real install and restores them through confirmComponentReview. Signed-off-by: Thomas Kosiewski <tk@coder.com>
Now that the load gate exists, the Settings notice states the effect: the skills and MCP servers of a plugin marked "needs review" do not load until the user reviews them. Signed-off-by: Thomas Kosiewski <tk@coder.com>
…rade and how to review them
3b84a2c to
5faba1f
Compare
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…stom_header without headerName (coder#6487) ## Summary Xum now loads three Codex `.mcp.json` shapes that it used to skip or misreport. A stdio server with `env_vars` loads and receives the named host variables. A remote entry with a `url` and no `type` loads as streamable HTTP. An `api_key` auth block with `headerScheme: custom_header` and no `headerName` is reported as malformed. Refs coder#6297. This PR covers three of its items: "Codex `env_vars` on stdio servers", "URL entries without `type`" and "`custom_header` without `headerName`". ## Background Codex review found these gaps after track A merged main (review threads `PRRT_kwDOPxxmWM6rv0UV`, `PRRT_kwDOPxxmWM6rv0Ua` and `PRRT_kwDOPxxmWM6rv0Uh`). 1. Codex defines `env_vars` as the way to pass selected host environment variables to a stdio server ([OpenAI plugins guide](https://developers.openai.com/api/docs/guides/agents-api/tools/plugins)). Xum rejected the field as unknown, so the server did not load. 2. The Codex plugin parser reads a `url` entry without `type` as streamable HTTP ([`codex-mcp/src/plugin_config.rs`](https://github.com/openai/codex/blob/main/codex-rs/codex-mcp/src/plugin_config.rs)). Xum skipped it as an unknown type. 3. OpenAI's auth reference requires `headerName` for `custom_header` ([fixed bearer tokens and API keys](https://developers.openai.com/plugins/build/plugins#fixed-bearer-tokens-and-api-keys)). Xum listed the block as an unsupported setting instead of a malformed one. ## Implementation All three changes apply to the Codex dialect only. A root `mcp.json` stays strict: `env_vars` and an untyped `url` are still errors there. A new test covers that. **`env_vars`.** The Codex translator turns `env_vars` into a new `forwardEnv` list of names on the stdio server record. Xum copies the semantics from Codex: - `env_vars: null` and `source: null` count as absent, as Codex's optional fields do. - An item is a bare name or `{ name, source }` (`McpServerEnvVar` in [`config/src/mcp_types.rs`](https://github.com/openai/codex/blob/main/codex-rs/config/src/mcp_types.rs)). - Xum refuses `source: "remote"`. Codex refuses it for a plugin that runs in the local environment (`bind_environment_env_vars` in `plugin_config.rs`). - Xum refuses `env_vars` on a remote server. Codex does the same (`throw_if_set` in `mcp_types.rs`). - At launch, `prepareStdioLaunch` copies only the named variables from the host. It reads only own properties of the host environment, so a name such as `constructor` is not an inherited value. It skips a name that the host does not set, as Codex's `create_env_for_mcp_server` does ([`rmcp-client/src/utils.rs`](https://github.com/openai/codex/blob/main/codex-rs/rmcp-client/src/utils.rs)). A configured `env` value wins over a host value. - Xum refuses `PLUGIN_ROOT` and `PLUGIN_DATA` in `env_vars`, as it already does in `env`. Xum reads only the names at load time. It never stores or shows a host value. The install consent preview shows the names, for example `node (host env: 'API_TOKEN')`. The consent fingerprint includes the names in the order they first appear (duplicates are dropped), so an update that adds, removes or reorders a name needs new consent. The server signature includes them too, so a changed list restarts the server. **Untyped `url`.** The translator infers `streamable-http` when an entry has a `url` and no `type`. This mirrors how it already infers `stdio` from a `command`. **`custom_header` without `headerName`.** `readAuth` now adds a warning that `headerName` is required, and it drops `headerScheme`. A bad `headerName` already used this warning class. ## Validation - I wrote each test first and saw it fail on `origin/main`. Then I made it pass. - `bun test` (pinned Bun 1.4.3) of `mcpConfig.test.ts`, `openaiMcpAuth.test.ts`, `installService.test.ts` and `mcpServerManager.test.ts`: 558 pass, 0 fail. - `make static-check` passes. ## Resolved since the hold - Consent for newly recognized servers is fixed on main by coder#6498 (stack coder#6534, coder#6557, coder#6570, coder#6587). A managed plugin's server loads only when its consent fingerprint matches what the user reviewed. This branch merges that main: `forwardEnv` is an included field in the fingerprint table (`componentConsent.ts`), so adding an `env_vars` name changes the fingerprint and needs a review. The old update-only `stdioFingerprint` helper is gone, because main's update review uses the same consent fingerprint. - The launch environment is fixed on main by coder#6581 (coder#6509). A plugin stdio server starts from a minimal host environment. `env_vars` names are copied into the server's declared `env`, so they reach the server on top of that base. A new test launches a plugin server through the real host runtime and checks that a named variable arrives and an unnamed one does not. - Docs: docs/config/mcp-servers.mdx ("Plugin server environment") now describes `env_vars`. ## Risks The risk is low. The `env_vars` and untyped-`url` changes apply only to Codex `.mcp.json` files, and those entries failed to load before. A stdio server without `env_vars` keeps the same launch environment, fingerprint and signature, because each of them leaves out an unset `forwardEnv`. Adding `forwardEnv` to the consent fingerprint does not change the fingerprint of an existing server, because `pick` skips an unset field. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$0.00`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=0.00 -->
…ed instances (coder#6765) ## Summary On the Coder AI Gateway route, Xum now sends Anthropic's `drop_block` thinking-binding control to anthropic-typed instances. Anthropic then drops a replayed thinking block whose prefix changed and answers 200, instead of rejecting the whole request with a signature 400 that Xum must repair and retry. Before this PR, only the direct Anthropic route sent it. ## Background Opus, Sonnet and Haiku 5.5 and Fable 5.1 bind replayed thinking to the system prompt and tool list before it. When either changes, a replayed block fails its signature check. On one host the 400s ran at about 0.128 per Anthropic turn, and they clustered in the 20 minutes after each app start (31, 20, 28, 17 and 14 per start on Oct 14). Comparison of each workspace's last request before an app start with its first request after it (8 starts, 281 workspaces, from `turn-envelope` rows in `durable-events.jsonl`): the system prompt or tool list changed in 95 of 281 (34%), against 195 of 2,590 (7.5%) between two turns in one process. | Cause | Workspaces | Status | |---|---|---| | Hot and pinned memory sections rebuilt from changed files | 52 | Fixed by coder#6519 | | Notion MCP server changed its tool schemas while the app was down | 18 | External | | AGENTS.md or other context files edited | 15 | Real content change | | `mcp_prompt_get` missing or added on the first turn | 10 | Open (coder#6406) | | Plugin skills and an MCP server hidden until the coder#6587 review | 5 | One-time upgrade effect | | An MCP server failed or was missing | about 10 | Open | This PR does not stop these prefix changes. It removes the rejected request and the retry that each of them cost. ## Implementation - `anthropicBlockBindingRouteAvailable` keeps the direct-route check unchanged. On the `coder` route it allows `drop_block` when the model reaches an instance whose type is exactly `anthropic`, and the request-level beta opt-out still turns it off. Bedrock-typed and other instances stay excluded. - The instance-type check moved into `anthropicTypedCoderRoute`, which `between_tools` eligibility already used, so both decisions share it. - The AI SDK sends the `thinking-binding-controls-2026-08-01` beta header together with `thinking.blockBinding`. ## Validation - `providerOptions.test.ts`: `coder:anthropic/…` ids and `anthropic:` ids routed onto Coder through `canonicalRoutes` get `drop_block`. Bedrock-typed instances and the beta opt-out do not. The test fails with the source change reverted. - **Live probe** through dogfood `claude-aws-us-east-2` (raw Messages API, about $0.10), for Opus 5.5, Sonnet 5.5, Haiku 5.5 and Fable 5.1: - A changed system prompt or tool list without `drop_block` returns the same 400 as in the app log. - The same request with `drop_block` and the beta header returns 200 and reports `input_transformations: [{type: thinking_dropped, reason: prefix_binding_mismatch}]`. - An unchanged history with `drop_block` returns 200 and drops nothing. - Adding `block_binding` to an unchanged conversation read the whole cache (Haiku: 21,710 of 21,710 tokens), so turning it on costs no cache miss. ## Risks Low to medium. The change affects every Claude 5.5 and Fable 5.1 request with thinking on the `coder` route to an anthropic-typed instance. - Without the beta header, Anthropic rejects `block_binding` with "Extra inputs are not permitted". The gateway passes the header through for Anthropic upstreams, and its Bedrock path converts or strips the header and the field together (coder/coder#29792). Bedrock-typed instances are excluded anyway. An older gateway was not tested live. - A dropped block still costs the cache miss of the changed prefix. Only the 400 and the retry go away. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$437.19`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=437.19 -->
1 participant
Upgrade effect (read first)
After this PR, every managed plugin's skills and MCP servers stop loading once, right after the upgrade, until the user reviews them. "Managed" means a plugin installed from Settings → Plugins (a row in
plugins.json). Unmanaged plugins (~/.agents/plugins, repo plugins) are not affected.Settings → Plugins shows a "Needs review" badge on each affected plugin and one notice that says the components do not load until reviewed, with a Review button (🤖 feat: show plugins that need a component review in Settings #6570). The command palette has the same review command.
One review per plugin restores loading. The review uses the existing update review panel and lists each skill and MCP server.
Agents in a workspace see fewer skills and MCP tools until then. Nothing is deleted: imports, enablement and plugin data stay as they are.
Downgrade: older builds ignore the consent records and load by name, as before. Upgrading again re-applies the gate.
Release text: the settings notice says why the components stop loading and that each plugin needs one review after the upgrade. The Agent Skills docs have a "Component review" section, and the MCP servers docs link to it.
Why no backfill
#6498 is about a newer Xum parsing more of an installed package than the user saw at install time: an extra Codex skills root, or a stdio server that an older parser skipped. Those components load under a name the user approved. A first-run backfill would fingerprint whatever the current parser sees, which is exactly the content the user never reviewed. It would turn the bug into a stored approval. So an install without consent records counts as "nothing reviewed", and the user reviews once.
Summary
A managed plugin's skill or MCP server now loads only when a stored consent record matches its current fingerprint. Closes #6498 (part 4 of the stack, on top of #6570).
Implementation
isPluginSkillConsented(componentConsent.ts), fingerprints the skill's directory (relative to the plugin root) plus its parsed frontmatter, as the install review does. The skill index,agent_skill_read,agent_skill_listandskill://reads all call it. A refused skill is skipped, so a same-name skill from another source can still serve.loadPluginMcpServersstoresconsentFingerprinton the plugin provenance.isPluginMcpServerAllowed(registry.ts) requires a matching record, in addition to the import and enablement checks. The provider drops an unreviewed server from the list, and the manager checks the same rule again at launch.Validation
New tests cover refusal and acceptance in the registry, the real MCP provider (including an
argschange under a reviewed name), the skill index,agent_skill_listand an end-to-end install. The end-to-end test installs a plugin, sees its skill and server load, strips the consent records, sees both stop loading, runsconfirmComponentReview, and sees both load again. Each new test fails with the gate turned off. Existing fixtures now record real consent through the production fingerprint code.Risks
The upgrade effect above is intended. It is user-visible and needs one review per managed plugin.
A fingerprint mismatch between the install review and the load path would block a component the user approved. The fixtures compute consent through the same production functions as the load path, and the end-to-end test covers the full cycle.
A body-only update of a skill (its
SKILL.mdinstructions) still applies without a review, as on main. The consent fingerprint covers the skill's folder and frontmatter, which closes the 🤖 fix: bind saved plugin imports and enablement to the reviewed component fingerprint #6498 case. Body review is a separate product proposal: 🤖 feat: review changed SKILL.md bodies in plugin updates #6617.Size: about 400 production lines changed, the rest is tests and fixtures.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$42.85