fix(codex): broaden context7 config checks#1000
Conversation
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR updates Codex configuration documentation and validation logic to support both legacy and current MCP server section naming conventions while marking multi-agent collaboration as stable. Configuration changes document role-specific agent setup, and script validation logic now handles dual naming support with improved error reporting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Analysis Failed
Troubleshooting
Retry: |
Greptile SummaryThis PR broadens the Codex global sanity check to accept either Two issues need to be resolved before merging:
Confidence Score: 4/5Not safe to merge as-is: the duplicate TOML header breaks spec compliance and the loop still hard-fails for Two P1 issues remain: a duplicate TOML table header that strict parsers will reject, and a logical gap where the legacy compatibility path the PR intends to enable still triggers a script failure because the old loop entry for
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[check-codex-global-state.sh] --> B{config.toml exists?}
B -- no --> Z[exit 1]
B -- yes --> C[Loop: check github / memory / sequential-thinking / context7]
C -->|context7 missing| D["fail 'MCP section context7 missing' ❌\nfailures++"]
C -->|all found| E[New OR-check block]
D --> E
E --> F{has_context7_legacy=1 OR has_context7_current=1?}
F -- yes --> G["ok 'context7 or context7-mcp exists' ✅"]
F -- no --> H["fail 'context7 or context7-mcp missing' ❌"]
G --> I{failures == 0?}
H --> I
D -. failures already > 0 .-> I
I -- yes --> J[PASS / exit 0]
I -- no --> K[FAIL / exit 1]
style D fill:#f88,stroke:#c00
style H fill:#f88,stroke:#c00
style G fill:#8f8,stroke:#080
style J fill:#8f8,stroke:#080
style K fill:#f88,stroke:#c00
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| [agents] | ||
| [agents] |
There was a problem hiding this comment.
[agents] TOML table header
The [agents] section header appears twice in a row (lines 104 and 105). The TOML specification prohibits redefining a table; strict TOML parsers (including the reference implementation) will reject this with a parse error such as Cannot define table 'agents': key already defined. Even lenient parsers may silently drop the first definition.
The first [agents] on line 104 should be removed — the comment and the keys (max_threads, max_depth) belong under the single [agents] header.
| [agents] | |
| [agents] | |
| [agents] | |
| # Multi-agent role limits and local role definitions. | |
| # These map to `.codex/agents/*.toml` and mirror the repo's explorer/reviewer/docs workflow. | |
| max_threads = 6 | |
| max_depth = 1 |
| has_context7_legacy=0 | ||
| has_context7_current=0 | ||
|
|
||
| if rg -n '^\[mcp_servers\.context7\]' "$CONFIG_FILE" >/dev/null 2>&1; then | ||
| has_context7_legacy=1 | ||
| fi | ||
|
|
||
| if rg -n '^\[mcp_servers\.context7-mcp\]' "$CONFIG_FILE" >/dev/null 2>&1; then | ||
| warn "Legacy [mcp_servers.context7-mcp] exists (context7 is preferred)" | ||
| has_context7_current=1 | ||
| fi |
There was a problem hiding this comment.
has_context7_legacy is set when [mcp_servers.context7] is found (the canonical/preferred name per the comment on line 48 of config.toml), while has_context7_current is set for [mcp_servers.context7-mcp] (the name that was previously warned as legacy). This is the opposite of what the names suggest, which makes the code harder to maintain. Consider renaming:
has_context7_legacy→has_context7(canonical preferred name)has_context7_current→has_context7_mcp(oldcontext7-mcpalias)
| if ( | ||
| test('sync and global sanity checks accept the legacy context7 MCP section', () => { | ||
| const homeDir = createTempDir('codex-sync-home-'); | ||
| const codexDir = path.join(homeDir, '.codex'); | ||
| const configPath = path.join(codexDir, 'config.toml'); | ||
| const config = [ | ||
| 'approval_policy = "on-request"', | ||
| 'sandbox_mode = "workspace-write"', | ||
| 'web_search = "live"', | ||
| 'persistent_instructions = ""', | ||
| '', | ||
| '[features]', | ||
| 'multi_agent = true', | ||
| '', | ||
| '[profiles.strict]', | ||
| 'approval_policy = "on-request"', | ||
| 'sandbox_mode = "read-only"', | ||
| 'web_search = "cached"', | ||
| '', | ||
| '[profiles.yolo]', | ||
| 'approval_policy = "never"', | ||
| 'sandbox_mode = "workspace-write"', | ||
| 'web_search = "live"', | ||
| '', | ||
| '[mcp_servers.context7]', | ||
| 'command = "npx"', | ||
| 'args = ["-y", "@upstash/context7-mcp"]', | ||
| '', | ||
| '[mcp_servers.github]', | ||
| 'command = "npx"', | ||
| 'args = ["-y", "@modelcontextprotocol/server-github"]', | ||
| '', | ||
| '[mcp_servers.memory]', | ||
| 'command = "npx"', | ||
| 'args = ["-y", "@modelcontextprotocol/server-memory"]', | ||
| '', | ||
| '[mcp_servers.sequential-thinking]', | ||
| 'command = "npx"', | ||
| 'args = ["-y", "@modelcontextprotocol/server-sequential-thinking"]', | ||
| '', | ||
| ].join('\n'); | ||
|
|
||
| try { | ||
| fs.mkdirSync(codexDir, { recursive: true }); | ||
| fs.writeFileSync(configPath, config); | ||
|
|
||
| const syncResult = runBash(syncScript, [], { HOME: homeDir, CODEX_HOME: codexDir }); | ||
| assert.strictEqual(syncResult.status, 0, syncResult.stderr || syncResult.stdout); | ||
|
|
||
| const checkResult = runBash(checkScript, [], { HOME: homeDir, CODEX_HOME: codexDir }); | ||
| assert.strictEqual(checkResult.status, 0, checkResult.stderr || checkResult.stdout); | ||
| assert.match(checkResult.stdout, /MCP section \[mcp_servers\.context7\] or \[mcp_servers\.context7-mcp\] exists/); | ||
| } finally { | ||
| cleanup(homeDir); | ||
| } | ||
| }) | ||
| ) | ||
| passed++; | ||
| else failed++; |
There was a problem hiding this comment.
context7 (canonical name), not the context7-mcp legacy path
The PR description says the goal is to accept either mcp_servers.context7 or mcp_servers.context7-mcp. The new test config on line 97 uses [mcp_servers.context7] (the canonical name), which already passed before this PR. The regression case — a user whose global config only has [mcp_servers.context7-mcp] — is never exercised, so the test would pass even with the broken loop logic described above.
Adding a second sub-test (or a separate test case) that writes a config with only [mcp_servers.context7-mcp] and asserts the check script exits 0 would make this a true regression guard.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".codex/config.toml">
<violation number="1" location=".codex/config.toml:105">
P1: Duplicate `[agents]` table declaration makes the TOML invalid and breaks config parsing.</violation>
</file>
<file name="tests/scripts/codex-hooks.test.js">
<violation number="1" location="tests/scripts/codex-hooks.test.js:37">
P2: Normalize `scriptPath` before invoking bash; passing raw OS paths here can break bash-based tests on win32.</violation>
</file>
<file name="scripts/codex/check-codex-global-state.sh">
<violation number="1" location="scripts/codex/check-codex-global-state.sh:126">
P1: The new context7 compatibility block is ineffective because `[mcp_servers.context7]` is still required earlier, so legacy-only `[mcp_servers.context7-mcp]` configs still fail.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| [agents] | ||
| # Multi-agent role limits and local role definitions. |
There was a problem hiding this comment.
P1: Duplicate [agents] table declaration makes the TOML invalid and breaks config parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .codex/config.toml, line 105:
<comment>Duplicate `[agents]` table declaration makes the TOML invalid and breaks config parsing.</comment>
<file context>
@@ -101,6 +102,9 @@ sandbox_mode = "workspace-write"
web_search = "live"
[agents]
+[agents]
+# Multi-agent role limits and local role definitions.
+# These map to `.codex/agents/*.toml` and mirror the repo's explorer/reviewer/docs workflow.
</file context>
| [agents] | |
| # Multi-agent role limits and local role definitions. | |
| # Multi-agent role limits and local role definitions. |
| has_context7_current=1 | ||
| fi | ||
|
|
||
| if [[ "$has_context7_legacy" -eq 1 || "$has_context7_current" -eq 1 ]]; then |
There was a problem hiding this comment.
P1: The new context7 compatibility block is ineffective because [mcp_servers.context7] is still required earlier, so legacy-only [mcp_servers.context7-mcp] configs still fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/codex/check-codex-global-state.sh, line 126:
<comment>The new context7 compatibility block is ineffective because `[mcp_servers.context7]` is still required earlier, so legacy-only `[mcp_servers.context7-mcp]` configs still fail.</comment>
<file context>
@@ -112,10 +112,25 @@ if [[ -f "$CONFIG_FILE" ]]; then
+ has_context7_current=1
+ fi
+
+ if [[ "$has_context7_legacy" -eq 1 || "$has_context7_current" -eq 1 ]]; then
+ ok "MCP section [mcp_servers.context7] or [mcp_servers.context7-mcp] exists"
else
</file context>
|
|
||
| function runBash(scriptPath, args = [], env = {}, cwd = repoRoot) { | ||
| return spawnSync('bash', [toBashPath(scriptPath), ...args], { | ||
| return spawnSync('bash', [scriptPath, ...args], { |
There was a problem hiding this comment.
P2: Normalize scriptPath before invoking bash; passing raw OS paths here can break bash-based tests on win32.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/scripts/codex-hooks.test.js, line 37:
<comment>Normalize `scriptPath` before invoking bash; passing raw OS paths here can break bash-based tests on win32.</comment>
<file context>
@@ -32,50 +33,30 @@ function cleanup(dirPath) {
-
function runBash(scriptPath, args = [], env = {}, cwd = repoRoot) {
- return spawnSync('bash', [toBashPath(scriptPath), ...args], {
+ return spawnSync('bash', [scriptPath, ...args], {
cwd,
env: {
</file context>
| return spawnSync('bash', [scriptPath, ...args], { | |
| const bashScriptPath = process.platform === 'win32' | |
| ? String(scriptPath).replace(/^([A-Za-z]):/, (_, driveLetter) => `/${driveLetter.toLowerCase()}`).replace(/\\/g, '/') | |
| : scriptPath; | |
| return spawnSync('bash', [bashScriptPath, ...args], { |
1 participant
Summary
mcp_servers.context7ormcp_servers.context7-mcpin the global Codex sanity checkTesting
Supersedes the remaining relevant parts of #860.
Summary by cubic
Broadened Codex sanity checks to accept both
mcp_servers.context7andmcp_servers.context7-mcp, and clarified that multi‑agent is enabled by default. Added regression tests to validate role wiring and legacycontext7compatibility, plus safer hook path handling.Bug Fixes
check-codex-global-state.shnow succeeds with eithermcp_servers.context7ormcp_servers.context7-mcp, and warns if both are present..codex/config.tomldocuments multi‑agent as on by default and clarifies local role mapping.Refactors
[mcp_servers.context7]passes sync and global checks.Written for commit 0ebcfc3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes