Repository navigation
fix: keep Codex MCP identity outside native settings - #3157
Open
sama Pyb (Pybsama) wants to merge 3 commits into
Open
sama Pyb (Pybsama) wants to merge 3 commits into
sama Pyb (Pybsama) wants to merge 3 commits into
Conversation
sama Pyb (Pybsama)
requested review from
Daniel Meppiel (danielmeppiel) and
Sergio Sisternes (sergio-sisternes-epam)
as code owners
October 3, 2026 05:54
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The changelog entry does not use the required pull-request-number suffix.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Removes unsupported Codex MCP id settings while preserving registry identity and ownership-safe migration.
Changes:
- Stores registry IDs in escaped TOML comments.
- Migrates only APM-owned legacy entries.
- Adds lifecycle coverage and consumer guidance.
| File | Description |
|---|---|
src/apm_cli/adapters/client/codex.py |
Implements comment metadata and migration. |
src/apm_cli/core/conflict_detector.py |
Reads Codex registry metadata. |
src/apm_cli/install/mcp/ownership.py |
Preserves conservative legacy adoption. |
src/apm_cli/integration/mcp_integrator_install.py |
Runs owned-entry migration. |
src/apm_cli/registry/operations.py |
Detects installed Codex UUIDs. |
tests/unit/test_codex_mcp_native_schema.py |
Covers schema and migration behavior. |
tests/unit/test_codex_adapter_phase3.py |
Updates native-setting expectations. |
tests/unit/test_codex_adapter_compatibility.py |
Updates compatibility expectations. |
tests/integration/test_codex_mcp_schema_lifecycle.py |
Tests CLI regeneration and convergence. |
docs/src/content/docs/consumer/install-mcp-servers.md |
Documents metadata and migration limits. |
CHANGELOG.md |
Records the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| ### Fixed | ||
|
|
||
| - Codex MCP entries no longer emit the unsupported `id` setting; reinstalling corrects recorded APM-owned entries while preserving registry identity, user settings, and ownership safeguards. (refs #3087) |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
APM writes an
idsetting into Codex MCP server entries, but Codex's native schema rejects that field. Stop emitting it for stdio and HTTP servers, and retain registry UUID matching in an escaped TOML comment. Conflict detection and installed-server lookup read that metadata, including legacy IDs, without treating it as proof of ownership.On MCP regeneration, remove legacy IDs only from entries recorded as APM-owned. Preserve the existing narrow adoption rule for legacy self-defined entries that exactly match the saved baseline; a nonempty UUID disqualifies that adoption. Unrelated user entries and settings stay outside migration. Inline MCP containers being written and owned inline entries being repaired may expand into regular tables, retaining their values and comments. Reparse the serialized TOML before atomically writing it. The consumer documentation explains regeneration and ownership limits.
Issue and approved scope
Fixes #3087.
Human scope approval: #3087 (comment)
This completes the bounded Codex adapter repair. It adds no shared MCP schema, target, credential model, or trust policy.
Type of change
Testing
Regression tests cover both transports, save/reparse UUID alias matching, owned migration, unowned and edited-entry preservation, and legacy baseline adoption. Hermetic project- and user-scope CLI tests verify regeneration and convergence.
Generated stdio and HTTP configurations were validated against the official Codex schema and the installed Codex CLI
0.159.0-alpha.12.1in strict mode. MCP entries were disabled during initialization, so no MCP server was launched. This validates that client version, not all Codex releases.Final focused adapter/conflict/ownership/integrator/lifecycle suite: 569 passed. The schema/ownership/lifecycle subset also passes with the declared minimum
tomlkit==0.13.0: 74 passed.Eight final runtime fixtures pass official-schema validation and strict Codex initialization with exit code 0 and empty stderr: two fresh transport configurations in an empty inline MCP container, plus legacy stdio/HTTP entries in standard, out-of-order, inline-child, nested-inline-parent, dotted-container, and dotted-root layouts. Regression tests also cover deleting a legacy ID at the beginning, middle, or end of an inline table.
Full-source Ruff lint/format and pylint duplication checks, architecture/auth boundaries, YAML I/O/file-length/portable-path guards, and assertion/duplicate-test quality checks pass. The full functional suite, Windows runtime, and other Codex versions were not tested locally.
Spec conformance (OpenAPM v0.1)
No normative requirement changes. This repairs output for an existing client adapter while preserving APM's ownership and conflict safeguards; it introduces no new manifest, lockfile, or native configuration setting.