Repository navigation
fix: preserve existing Codex settings during setup - #138
PatrickSys wants to merge 1 commit into
Conversation
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: 19ddb1c6c5
ℹ️ 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".
|
|
||
| const dottedEntry = new RegExp( | ||
| `^\\s*${mcpServersKey}\\s*\\.\\s*${codebaseContextKey}\\s*\\.`, | ||
| `^\\s*${mcpServersKey}\\s*\\.\\s*${codebaseContextKey}\\s*(?:\\.|=)`, |
There was a problem hiding this comment.
When an existing Codex config has a multiline string such as developer_instructions containing an example line beginning mcp_servers.codebase-context =, this unanchored textual scan matches content inside the string and aborts initialization even though no such server entry exists. Restrict detection to actual root-level TOML key definitions or parse the TOML structure so unrelated saved settings do not prevent setup.
Useful? React with 👍 / 👎.
| ``` | ||
|
|
||
| The interactive wizard limitations described above are still present in the 2.4.0 source. A successful wizard exit alone does not establish working client registration. | ||
| A successful setup run writes the config but does not confirm that the client connected. |
There was a problem hiding this comment.
In interactive mode, answering no to Apply MCP config? still lets handleInitCli print the next steps and return successfully without calling writeFile, so a successful setup run does not necessarily write a config. This sentence should distinguish a successful config application from a successful wizard exit rather than assuring users that the file was written.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
 [Medium impact] The change reaches Codex project setup and its saved config workflow. Fix the false conflict on multiline string contents before merging. Findings
SummarySetup now detects existing Codex servers written as dotted inline assignments.
Reviews (1) · Last reviewed commit: "fix: refuse root dotted Codex init entri..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const dottedEntry = new RegExp( | ||
| `^\\s*${mcpServersKey}\\s*\\.\\s*${codebaseContextKey}\\s*\\.`, | ||
| `^\\s*${mcpServersKey}\\s*\\.\\s*${codebaseContextKey}\\s*(?:\\.|=)`, |
There was a problem hiding this comment.
The new = match also catches text inside TOML multiline strings. If a user includes mcp_servers.codebase-context = { command = "example" } on its own line in saved instructions, setup reports a conflicting server and exits without adding one, even though that line is only string content. Check actual TOML keys rather than matching every line, and add a saved-file test for this case.
1 participant
Parked: setup mistakes an example inside saved instructions for a real server entry.
The new regression test confirms that valid settings can block setup.
Two guard revisions still leave valid TOML cases uncovered, so this follow-up should not merge tonight.
The earlier project setup change remains merged; this PR only adds more protection for existing settings.
Text preview: C:/Users/bitaz/AppData/Local/Temp/job-J11/agy-mockups/mockup_1792167096015.html.