Register the MCP server with Codex, and unblock the initializr suite - #5724
Merged
Conversation
Codex was detected and then skipped: MCPClientRegistrar only knew how to edit a JSON "mcpServers" object, so ~/.codex/config.toml was marked non-writable and "Install in MCP Hosts" could neither add the entry nor remove it (discussion #5700). One file covers all three Codex clients - the ChatGPT desktop app, the CLI and the IDE extension share it. MCPToml edits that file as TEXT rather than parsing and writing it back. It walks the document far enough to know where every table and key assignment begins and ends - tracking strings, multi-line strings, arrays, inline tables and comments, so a "[" at the head of a line inside an array is never mistaken for a table header - and then rewrites only the byte range belonging to one server. Everything else survives exactly as the user left it: other servers, settings, comments, key order, formatting, line endings, a BOM. A parse and reserialize round trip could not promise that. The other half of the promise is refusing. A document that is not valid TOML, or one that declares this server in a shape the editor does not rewrite - a dotted key, an inline table, an array of tables, or twice - is reported and left untouched. Losing a Codex configuration is worse than not registering. Server names and env keys are quoted when they are not bare keys: written bare, a dot in a name would silently become two nested tables instead of one server. Verified against the real thing, not only against ourselves. 800 generated documents were cross-checked with Python's tomllib (output parses, the entry is exact, every other key unchanged, removal restores the original), and the whole cycle was round-tripped with the codex CLI: it reads what we write, and we update and remove an entry it wrote without disturbing its neighbours. The registrar's host table now carries a config format instead of a writable flag, and the desktop menu names any host it could have written and did not, so a refusal reaches the user rather than only the log. Generated projects ship the pointer an agent needs: a new skill reference on driving the RUNNING simulator over MCP - serve, register, then ui_snapshot / ui_find / ui_set_text / ui_activate - indexed from SKILL.md and AGENTS.md. That loop answers whether a flow BEHAVES right, which a screenshot cannot. Making that verifiable meant repairing the initializr's javase module, which has not compiled since #5591 added downloadProject to WebsiteThemeNative and left the simulator implementation behind. With it building again the test suite runs, which surfaced three stale assumptions in GeneratorModelIntegrationBuildTest: the build client jar was written to the home root instead of .codenameone/, it was an empty placeholder when the mojo loads a class out of it, and the build stopped at "compile" while the css goal that produces theme.res is bound to process-classes. All six tests pass now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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: 0bb2fa5e42
ℹ️ 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".
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
The Ant core build compiles against the Codename One runtime, not the JDK, and StringBuilder.replace(int,int,String) exists in neither vm/JavaAPI nor Ports/CLDC11 - so build-test (8) and javase-simulator-tests failed on a call that compiled cleanly on a desktop. The splice now copies front to back with append and substring. Verified the way CI does it, with -bootclasspath ../cn1-binaries/CLDC11.jar, and with a negative control that puts the replace() call back and fails. Windows paths were wrong for the dotfile hosts. KnownClient.absolutePath prefixed EVERY Windows entry with %APPDATA%, so Codex resolved to ~/AppData/Roaming/.codex/config.toml when CODEX_HOME is ~/.codex on every platform - %USERPROFILE% on Windows. Claude Code had the same bug for ~/.claude.json. Both are silent: the config is never found, so the host is never detected and nothing is ever written. The table now says which base each host uses; only Claude Desktop, an installed application with a per user directory of its own, keeps %APPDATA%. appDataPath now takes the home as an argument instead of reading it back, so a path is a pure function of the home it is resolved against - the asymmetry the new test caught first. A multi-line string may legally end in four or five quotes, because the VALUE itself may end in one or two. Consuming the first three as the delimiter left a stray quote and refused a valid Codex configuration. A container has to close with the delimiter it opened with. One depth counter accepted `value = [}` as balanced, so a file that is not valid TOML was edited anyway, against the one promise this editor makes. The walk now tracks the closers it owes. Python's tomllib confirms both premises: it parses the quote-run document and rejects all three mismatched-delimiter ones. Removal no longer claims "no matching entries were found" when it does not know that: a refusal produces the same empty list. It does NOT list the hosts that were not updated the way Install does, because on removal that set is every host the tool was never registered with, which would report the normal case as a failure; the reason is in the code beside the message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 747a9e2e56
ℹ️ 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".
Collaborator
Author
|
Compared 12 screenshots: 12 matched. |
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
…failure Registering a second time reported failure. writeTomlEntry treats "the document did not change" as "nothing was updated", which is right for a removal with no entry to remove and wrong for a registration whose entry is already exactly what would be written - the host IS registered. Install then listed Codex under "Not updated" and pointed at a log that says nothing, on the ordinary path of clicking Install twice. The check now answers per operation. A neighbouring server declared with a root dotted key blocked everything. The refusal was written for `mcp_servers = <value>`, after which TOML forbids a later [mcp_servers.x] header - but it fired on any root key whose FIRST segment was mcp_servers, so `mcp_servers.docs.command = "d"` refused both registration and removal of an unrelated entry. Dotted keys leave mcp_servers defined by dotted keys, and TOML explicitly allows a [table] header to add a sub-table to one of those; only the whole-table assignment is fatal, and a dotted key naming THIS server is already caught as a second declaration of it. tomllib settles both boundaries: it parses `mcp_servers.docs.command = "d"` followed by our table, and rejects the same document when the dotted key names mcp_servers itself or this server. The cross-check now asserts that every refusal on a valid document is justified by appending being illegal there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5963bb3a15
ℹ️ 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".
Deleting the config and renaming a staged file over it turned a symlinked config - a common dotfiles arrangement - into a regular file, and reset the file's mode to whatever the process umask said. Observed, not theorised: on the previous commit a 600 symlink to ~/dotfiles/codex-config.toml comes back as a 644 regular file and the dotfiles source never sees the change. These files carry other tools' API keys in their env blocks, so widening a hand-set 600 is not cosmetic, and it happened on every write rather than on some unlucky one. An existing config is now written through, which keeps the inode and therefore keeps both the link and the mode. FileSystemStorage cannot read a link or copy a mode - it has no API for either - so preserving the original file is the only portable way to preserve them, and the suggestion to resolve the link or copy the metadata explicitly cannot be implemented where this code has to live. Nothing here was atomic to begin with, and the comment claiming it has gone: rename cannot overwrite on every platform, so the old path deleted first and had a window where the config was missing entirely. The staged sibling file remains the recovery path - it holds the complete new content until the write through succeeds - so the failure story is unchanged while the metadata story is fixed. This applies to the JSON hosts too; ~/.claude.json holds tokens. Verified end to end through the real JavaSE port: symlink still a symlink, mode still 600, neighbouring server's secret env intact, no staging file left behind, and the codex CLI reads the result and still reads it after removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d970fa8bff
ℹ️ 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".
Codex really is relocatable with CODEX_HOME - the review is right about the behaviour - but the registrar cannot follow it. The Codename One runtime has no System.getenv: it is absent from ../cn1-binaries/CLDC11.jar, the bootclasspath the Ant core build compiles against, and from the java-runtime jar the bytecode compliance check uses, so the call does not compile no matter what a desktop JVM would do at runtime. Wiring it up would mean a desktop port, which does have an environment, handing it to core through new public API, which is a bigger decision than a Codex writer should make on its own. Worth recording how nearly this went the other way: javap on either jar reports a getenv that core cannot call, because it resolves java.lang.System from the JDK rather than from the jar on the classpath. Only javac -bootclasspath tells the truth, and it is what caught this after the plumbing was already written. The note in the code says so, so the next reader does not repeat the check and reach the wrong answer. The affected user is told instead: the guide now says to add the entry by hand when CODEX_HOME has moved the configuration, and the code records what such a machine sees - Codex not detected at all, or a stale ~/.codex updated if one survived the move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
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
1 participant
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.
Fixes the request in discussion #5700: Install in MCP Hosts now registers and unregisters a Codename One application's MCP server in Codex, instead of detecting Codex and asking the user to edit
config.tomlby hand. One file covers all three Codex clients - the ChatGPT desktop app, the CLI and the IDE extension share~/.codex/config.toml.How the TOML is edited
MCPTomledits the file as text rather than parsing and writing it back. It walks the document far enough to know where every table and key assignment begins and ends - tracking strings, multi-line strings, arrays, inline tables and comments, so a[at the head of a line inside an array is never mistaken for a table header - and then rewrites only the byte range belonging to one server. Everything else survives exactly as the user left it: other servers, settings, comments, key order, formatting, line endings, a BOM. A parse-and-reserialize round trip could not promise that.The other half of the promise is refusing. A document that is not valid TOML, or one that declares this server in a shape the editor does not rewrite - a dotted key, an inline table, an array of tables, or twice - is reported and left untouched. Losing a Codex configuration is a worse outcome than not registering. Server names and env keys are quoted when they are not bare keys: written bare, a dot in a name would silently become two nested tables instead of one server.
Registering twice replaces the entry rather than duplicating it, removing takes the server's
envsub-table with it, and tables the user wrote out of order are consolidated.Verification
Cross-checked against independent references, not only against itself:
tomllib: the output parses, the entry is exact, every other key is unchanged, and removal restores the original. 0 failures.codexCLI (0.153.0):codex mcp listreads the entry we write with the right command/args/env, and the editor updates and removes an entrycodex mcp addwrote while leaving its other server,[tui]and comments intact.FileSystemStorage/JavaSE port against a temporary home: detect, register, re-register (byte-identical), unregister (file restored exactly, no temp file left).core-unittestssuite is green, with SpotBugs, PMD and Checkstyle at zero.Generated projects
Every generated project already ships an authoring skill, but nothing in it told an agent that the running app is drivable - so an agent could only ever look at screenshots. A new reference covers the loop: serve from the simulator's
MCPmenu, register with one click, thenui_snapshot/ui_find/ui_set_text/ui_activate, with the release-build gate spelled out. Indexed fromSKILL.mdandAGENTS.md.The initializr suite
Making that verifiable meant repairing
scripts/initializr/javase, which has not compiled since #5591 addeddownloadProjecttoWebsiteThemeNativeand left the simulator implementation behind. With the module building again its test suite runs, which surfaced three stale assumptions inGeneratorModelIntegrationBuildTest: the build client jar was written to the home root instead of.codenameone/, it was an empty placeholder when the mojo loads a class out of it, and the build stopped atcompilewhile thecssgoal that producestheme.resis bound toprocess-classes. All six tests pass now, including the one that generates a real project, builds it on JDK 17 and verifies the localization bundles are baked intotheme.res.🤖 Generated with Claude Code