feat(mcp): Add local mode support for Codex CLI#1109
feat(mcp): Add local mode support for Codex CLI#1109cm-dyoshikawa merged 1 commit intodyoshikawa:mainfrom
Conversation
There was a problem hiding this comment.
@r1bilski Thank you! I'll be glad if work on the points:
Code Review & Security Review
Code Review
Overall: Good - The implementation follows existing patterns (GeminiCliMcp, OpencodeMcp) well, and the intent is clear.
Issues to Address
-
README.md not updated (High priority)
- The
Supported Tools and Featuressection still shows🌏 🔧(global-only) for Codex CLI's mcp column. Since this PR adds local mode support, it should be updated to✅ 🌏 🔧. - This is also documented in
feature-change-guidelines.md.
- The
-
Consider
.codex/config.tomlin gitignore.ts (Medium priority)- Other tools' MCP config files (
.mcp.json,.gemini/settings.json, etc.) are added to gitignore, but.codex/config.tomlis not. - If this is intentional (since
config.tomlmay contain other Codex settings, similar to theisDeletable(): falsereasoning), please document the rationale in the PR description. - Per
feature-change-guidelines.md: "Evaluate whethergitignore.tsneeds additions or changes in its generated output."
- Other tools' MCP config files (
Minor Suggestions
getSettablePathsparameter naming (Low priority)- Changing
{ global }to_optionsis inconsistent with other tools likeGeminiCliMcp. Consider using{ global: _global }or keeping the destructuring pattern for consistency.
- Changing
Positives
readFileContent→readFileContentOrNull+smolToml.stringify({})fallback is appropriateisDeletable(): falsefollows the same pattern asGeminiCliMcp/OpencodeMcp— correct defensive design- Good test coverage: local mode full workflow integration test (
fromFile → toRulesyncMcp → fromRulesyncMcp), E2E test, andisDeletabletest added - Existing test bug fix (
mcpServers→mcp_serversreference) is a nice catch
Security Review
No security issues found. This PR is safe to merge from a security perspective.
- Path traversal/injection: Only hardcoded relative paths (
.codex,config.toml) are used.checkPathTraversal()exists as additional protection. - File system security: TOCTOU window in
readFileContentOrNullis a pre-existing pattern across the project, with negligible risk in a CLI tool context. - TOML parsing:
smolToml.parse()is a safe data parser with type checking inconvertFromCodexFormat. - Global/local mode switch: Controlled by
baseDirparameter — no new attack surface introduced. - Malicious code/backdoors: None detected.
🤖 Generated with Claude Code
dyoshikawa
left a comment
There was a problem hiding this comment.
Code Review & Security Review
Code Review
Overall: Good - The implementation follows existing patterns (GeminiCliMcp, OpencodeMcp) well, and the intent is clear.
Issues to Address
-
README.md not updated (High priority)
- The
Supported Tools and Featuressection still shows global-only for Codex CLI's mcp column. Since this PR adds local mode support, it should be updated accordingly. - This is also documented in
feature-change-guidelines.md.
- The
-
Consider
.codex/config.tomlin gitignore.ts (Medium priority)- Other tools' MCP config files (
.mcp.json,.gemini/settings.json, etc.) are added to gitignore, but.codex/config.tomlis not. - If this is intentional (since
config.tomlmay contain other Codex settings, similar to theisDeletable(): falsereasoning), please document the rationale in the PR description. - Per
feature-change-guidelines.md: "Evaluate whether gitignore.ts needs additions or changes in its generated output."
- Other tools' MCP config files (
Minor Suggestions
getSettablePathsparameter naming (Low priority)- Changing
{ global }to_optionsis inconsistent with other tools likeGeminiCliMcp. Consider using{ global: _global }or keeping the destructuring pattern for consistency.
- Changing
Positives
readFileContenttoreadFileContentOrNull+smolToml.stringify({})fallback is appropriateisDeletable(): falsefollows the same pattern asGeminiCliMcp/OpencodeMcp- correct defensive design- Good test coverage: local mode full workflow integration test, E2E test, and
isDeletabletest added - Existing test bug fix (
mcpServerstomcp_serversreference) is a nice catch
Security Review
No security issues found. This PR is safe to merge from a security perspective.
- Path traversal/injection: Only hardcoded relative paths are used.
checkPathTraversal()exists as additional protection. - File system security: TOCTOU window in
readFileContentOrNullis a pre-existing pattern across the project, with negligible risk in a CLI tool context. - TOML parsing:
smolToml.parse()is a safe data parser with type checking inconvertFromCodexFormat. - Global/local mode switch: Controlled by
baseDirparameter - no new attack surface introduced. - Malicious code/backdoors: None detected.
6038d56 to
7141b5e
Compare
7141b5e to
cc098b2
Compare
|
Whoops, I forgot to update the docs :)
|
This implements generating project-level config for mcp for codexcli.
closes #1103