fix(codex): repair legacy MCP launchers - #2108
Conversation
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|---|---|
| PR | #2108 |
| HEAD checked | af3a8c03a9479191cb56430cd68ea46e6d7f8c89 |
| Request ID | req_1786674076_1750 |
| Review record | 660a386b-e974-42c8-989c-7de090fa5e2c |
What Improved
- Repairs the narrowly identifiable legacy Codex direct MCP launcher while preserving URL endpoints, mismatched environments, custom binaries, and entries with additional process controls.
- Makes doctor remediation render the literal
ouroboros-ai[mcp]extra and recommends the managed stdio repair path. - Documents the migration boundary and adds focused regression coverage.
Issue Requirements
| Requirement | Status |
|---|---|
Repair the narrow legacy PATH-selected ouroboros mcp serve entry with the canonical Codex environment |
Met |
| Migrate the repairable legacy entry to setup’s managed MCP launcher | Met |
| Preserve URL endpoints, genuinely custom executable paths, mismatched environments, and custom process controls | Met |
Render literal package extras in ouroboros codex doctor output |
Met |
Recommend the working --mcp-mode stdio repair path |
Met |
| Document the migration boundary | Met |
Prior Findings Status
No prior ouroboros-agent review concerns were present. Human and inline review-comment artifacts were empty.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
| 1 | docs/cli-reference.md:294 | Documentation | Consider qualifying “custom executable paths remain untouched” as “user-managed custom executable paths”; setup-owned entries carrying the managed comment are intentionally refreshed even when their executable path is stale or nonstandard. |
Test Coverage Notes
- Reviewed both changed unit modules.
tests/unit/cli/test_setup.pypassed with the required MCP profile: 408 passed, 1 skipped. tests/unit/cli/test_codex_command.pypassed: 211 passed.- Focused migration and diagnostic tests passed, including canonical migration, process-control preservation, and literal optional-extra rendering.
- Ruff passed for all changed source and test files; MyPy passed for both changed source modules.
- Direct probes confirmed bare and absolute PATH-selected executables match, while a distinct custom executable does not.
Design Notes
The change extends the existing setup-ownership classifier rather than adding a separate migration path. Exact argv, environment equality, executable identity, managed-comment provenance, and rejection of every additional entry field keep the mutation boundary conservative.
Design / Roadmap Gate
The affected boundary is ~/.codex/config.toml ownership and replacement. Unannotated direct entries are migrated only when they use base MCP argv, the exact canonical environment, no additional configuration fields, and resolve to the currently PATH-selected ouroboros executable. URL, environment, process-control, and distinct-command overrides remain preserved. Writes continue through the existing validated atomic-write and setup rollback path, maintaining failure and replay safety.
Directional Notes
Review focused on optional MCP dependency parity, preservation of operator-managed configuration, executable identity, and safe persistent setup mutation. Maintainer memory was used only to direct inspection; the approval is grounded in the current source, diff, and executed tests.
Test Coverage
- Reviewed both changed unit modules.
tests/unit/cli/test_setup.pypassed with the required MCP profile: 408 passed, 1 skipped. tests/unit/cli/test_codex_command.pypassed: 211 passed.- Focused migration and diagnostic tests passed, including canonical migration, process-control preservation, and literal optional-extra rendering.
- Ruff passed for all changed source and test files; MyPy passed for both changed source modules.
- Direct probes confirmed bare and absolute PATH-selected executables match, while a distinct custom executable does not.
Merge Recommendation
Merge. No current-HEAD blocking runtime, ownership, persistence, compatibility, or coverage defects were found, and the affected unit suites plus lint and type checks pass.
Review-Metadata:
verdict: APPROVE
head_sha: af3a8c0
request_id: req_1786674076_1750
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Draft review (current HEAD The migration boundary is appropriately narrow: it repairs only the canonical legacy direct launcher while preserving URLs, custom executables, mismatched environments, and custom process controls. The doctor-output escaping and I did not find a blocking code issue in this draft. Before marking ready, please make the PR Hygiene check green (the current HEAD still reports |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|---|---|
| PR | #2108 |
| HEAD checked | af3a8c03a9479191cb56430cd68ea46e6d7f8c89 |
| Request ID | req_1786808053_1850 |
| Review record | 67be0e7d-b86c-4ffd-a863-d22668b08e2e |
What Improved
- Repairs the narrow legacy Codex direct MCP configuration when it uses the PATH-selected
ouroborosexecutable, basemcp servearguments, and canonical Codex environment. - Preserves URL endpoints, custom commands, mismatched environments, and entries with additional process-control fields.
- Escapes Rich diagnostics so
ouroboros-ai[mcp]remains visible and recommends the working--mcp-mode stdiorepair path. - Documents the migration boundary in the CLI reference.
Issue Requirements
| Requirement | Status |
|---|---|
Refs #1964 |
N/A — linked issue contents were not provided in the review artifacts |
Migrate only the narrow PATH-selected ouroboros mcp serve launcher with the canonical Codex environment |
Met |
| Preserve URL endpoints, custom executable paths, mismatched environments, and custom process controls | Met |
Preserve the literal ouroboros-ai[mcp] extra in doctor output |
Met |
Recommend ouroboros setup --runtime codex --mcp-mode stdio for repair |
Met |
| Document the migration boundary | Met |
Prior Findings Status
Prior approval concerns remain maintained. The current snapshot independently confirms the previously reviewed migration ownership boundary, optional MCP dependency remediation, preservation behavior, and Rich output fix. The prior PR-hygiene comment about the issue-link check is not a code defect and does not change this review’s merge classification.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | tests/unit/cli/test_setup.py:563 |
Low | High | Add focused preservation tests for a direct launcher with canonical arguments but a mismatched environment and for a base-argument launcher that does not resolve to the PATH-selected executable, making the ownership boundary explicit against future regressions. |
Non-blocking Suggestions
| 1 | tests/unit/cli/test_setup.py:563 | Test coverage | The implementation explicitly rejects mismatched environments and non-PATH-selected executables, but the new tests primarily cover the positive migration case and process-control preservation. |
Test Coverage Notes
- Reviewed the changed setup and Codex call paths, including TOML parsing, launcher selection, PATH identity checks, Rich rendering, and preservation logic.
- Ran
619 passed, 1 skippedacrosstests/unit/cli/test_setup.pyandtests/unit/cli/test_codex_command.pywith the declared MCP extra enabled. - Ran targeted doctor tests separately:
3 passed. ruff checkpassed andruff format --checkreported all four changed Python files already formatted.- The initial setup test invocation without the MCP extra had two environment-dependent failures because the source-tree fallback could not import
mcp; rerunning with--extra mcppassed all changed test modules.
Design Notes
The change keeps setup ownership conservative: only structurally canonical, unannotated direct entries are newly adopted, while managed entries continue to refresh and custom entries remain untouched. The doctor remediation now matches the actual isolated MCP dependency contract.
Design / Roadmap Gate
The affected boundary is persistent ~/.codex/config.toml MCP configuration. The new predicate rejects URLs, missing endpoints, extra configuration fields, non-canonical environments, and commands that do not resolve to the PATH-selected ouroboros executable; accepted entries flow through the existing TOML validation and atomic-write path. The replacement launcher uses the isolated ouroboros-ai[mcp] profile, while direct and module launchers with managed comments retain their existing compatibility behavior. Replay and rollback behavior remain covered by the existing registration machinery and the changed setup suite.
Directional Notes
Review focus followed the PR’s stated optional-dependency parity and configuration-ownership boundary. Advisory maintainer memory also directed inspection of literal MCP extras, truthful runtime remediation, preservation of custom configuration, and safe persistent setup mutation; current source and tests provided the evidence for the classification.
Test Coverage
- Reviewed the changed setup and Codex call paths, including TOML parsing, launcher selection, PATH identity checks, Rich rendering, and preservation logic.
- Ran
619 passed, 1 skippedacrosstests/unit/cli/test_setup.pyandtests/unit/cli/test_codex_command.pywith the declared MCP extra enabled. - Ran targeted doctor tests separately:
3 passed. ruff checkpassed andruff format --checkreported all four changed Python files already formatted.- The initial setup test invocation without the MCP extra had two environment-dependent failures because the source-tree fallback could not import
mcp; rerunning with--extra mcppassed all changed test modules.
Merge Recommendation
No blocking runtime, API-contract, security, persistence, compatibility, or coverage defect was found in the current source snapshot. The changed test modules pass with the MCP extra enabled, static checks pass, and the implementation satisfies the PR-body requirements. Merge is recommended.
Review-Metadata:
verdict: APPROVE
head_sha: af3a8c0
request_id: req_1786808053_1850
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
2 participants
Refs #1964
Summary
ouroboros mcp serveplus the canonical Codex environment—to the isolated managedouroboros-ai[mcp]launcher during setup.ouroboros codex doctorpreserve literal package extras in Rich output and recommend the working--mcp-mode stdiorepair path.Root cause
A base
uv toolinstallation can expose theouroborosexecutable without installing the optional MCP SDK. Older or manually-created Codex entries that launch that executable directly therefore exit before the initialize response, which Codex surfaces only asconnection closed: initialize response.Current setup correctly generates an isolated
uvx --from ouroboros-ai[mcp]launcher, but auto mode treated the unannotated legacy direct entry as user-managed and preserved it. The doctor detected the missing import, but Rich interpreted[mcp]as markup and removed the most important part of the remediation.User impact
Re-running Codex setup now self-heals the safe legacy shape without taking ownership of genuinely custom MCP configurations. Diagnostics show copyable commands and the literal optional-dependency profile.
Verification
ouroboros-ai 0.51.4:ouroboros setup --runtime codex --non-interactive --mcp-mode stdioouroboros codex doctor --live-mcp(initialize/list_tools passed)tests/unit/cli: 1969 passed, 7 skippedupstream/main: passedgit diff --check: passed