Measure successful Initializr downloads - #5591
Merged
Merged
Conversation
![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: a4c40e1fc9
ℹ️ 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
Cloudflare Preview
|
shai-almog
added a commit
that referenced
this pull request
Sep 7, 2026
…5724) * Register the MCP server with Codex, and unblock the initializr suite 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> * Fix the core build, and four defects the review found 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> * Do not let a neighbour's dotted key, or a no-op install, look like a 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> * Write through an existing config instead of replacing it 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> * Say why CODEX_HOME is not followed, since it cannot be from here 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
Verification
translated_app.jscontains the acknowledgeddownloadProjectbridge/initializr/page containcn1-initializr-project-downloadednode docs/website/scripts/test-cn1-crisp-events.mjsnode --check scripts/initializr/javascript/src/main/javascript/com_codename1_initializr_WebsiteThemeNative.jsThe generated Initializr bundle is intentionally not committed; the website workflow rebuilds it from repository sources for previews and production deployment.