fix: safe Codex config sync — merge AGENTS.md + add-only MCP servers#723
fix: safe Codex config sync — merge AGENTS.md + add-only MCP servers#723chris-yyau wants to merge 3 commits intoaffaan-m:mainfrom
Conversation
|
📝 WalkthroughWalkthroughAdds a Node-based TOML merge utility and integrates it into the existing sync script to auto-merge ECC-recommended Codex MCP server sections into Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Sync as "sync-ecc-to-codex.sh"
participant Merge as "merge-mcp-config.js"
participant Parser as "TOML Parser (\\@iarna/toml)"
participant FS as "File System"
User->>Sync: run sync-ecc-to-codex.sh [--dry-run] [--update-mcp]
Sync->>Merge: invoke Node merge script with flags & config path
Merge->>FS: read config.toml
FS-->>Merge: return raw TOML
Merge->>Parser: parse TOML -> object
Parser-->>Merge: parsed config
alt Add-only (default)
Merge->>Merge: detect canonical & legacy sections
Merge->>Merge: compare fields, log drift warnings
Merge->>FS: append missing ECC server TOML blocks
else Update mode (--update-mcp)
Merge->>Merge: remove ECC-managed sections from raw TOML
Merge->>Merge: re-append canonical ECC server blocks
Merge->>FS: write updated config.toml
end
FS-->>Merge: write result
Merge-->>Sync: exit code / messages
Sync-->>User: print summary / dry-run output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: |
There was a problem hiding this comment.
Pull request overview
This PR updates the ECC → Codex sync workflow to preserve user customizations by switching from destructive overwrites/text-surgery to marker-based AGENTS merging and TOML-aware MCP server merging.
Changes:
- Update
scripts/sync-ecc-to-codex.shto merge an ECC-managed AGENTS block via<!-- BEGIN ECC -->/<!-- END ECC -->markers. - Replace bash-based TOML manipulation with a new Node script that appends missing ECC MCP servers and supports
--update-mcp. - Document the new merge behaviors in
README.mdand.codex/AGENTS.md, and add the TOML parser dependency.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/sync-ecc-to-codex.sh | Marker-based merge for ~/.codex/AGENTS.md; delegates MCP merging to Node script |
| scripts/codex/merge-mcp-config.js | New TOML parser-based MCP server merge/update implementation |
| README.md | Updates Codex setup instructions and describes add-only MCP merge behavior |
| package.json | Adds @iarna/toml dependency and includes the new script in published files |
| .codex/AGENTS.md | Documents the new automatic config.toml MCP merge behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/codex/merge-mcp-config.js
Outdated
| // Also remove sub-sections (e.g., mcp_servers.supabase.env) | ||
| for (const key of Object.keys(existing)) { | ||
| if (key.startsWith(name + '.') || key.startsWith(resolvedLabel + '.')) { | ||
| raw = removeSectionFromText(raw, `[mcp_servers.${key}]`); | ||
| } | ||
| } |
There was a problem hiding this comment.
In --update-mcp mode, the loop intended to remove sub-sections (e.g. [mcp_servers.supabase.env]) iterates Object.keys(existing), but existing only contains top-level server names (like supabase, github) from the parsed TOML. This means keys like supabase.env are never seen, so nested subtables are not removed even though the comment says they are. If the intent is to remove nested sections, consider scanning/removing them from raw via section-header matching (e.g. remove any [mcp_servers.<name>.*] headers) or by walking the parsed object recursively and reconstructing paths; otherwise, remove this dead code/comment and document that nested subtables (often credentials) are preserved on update.
| // Also remove sub-sections (e.g., mcp_servers.supabase.env) | |
| for (const key of Object.keys(existing)) { | |
| if (key.startsWith(name + '.') || key.startsWith(resolvedLabel + '.')) { | |
| raw = removeSectionFromText(raw, `[mcp_servers.${key}]`); | |
| } | |
| } | |
| // Also remove sub-sections (e.g., mcp_servers.supabase.env, mcp_servers.supabase.env.prod) | |
| const collectSubtablePaths = (obj, prefix) => { | |
| const paths = []; | |
| if (!obj || typeof obj !== 'object') return paths; | |
| for (const [key, value] of Object.entries(obj)) { | |
| const path = prefix ? `${prefix}.${key}` : key; | |
| if (value && typeof value === 'object' && !Array.isArray(value)) { | |
| // This represents a TOML table: [mcp_servers.<base>.<path>] | |
| paths.push(path); | |
| paths.push(...collectSubtablePaths(value, path)); | |
| } | |
| } | |
| return paths; | |
| }; | |
| const baseNames = new Set([resolvedLabel]); | |
| if (name !== resolvedLabel) { | |
| baseNames.add(name); | |
| } | |
| for (const baseName of baseNames) { | |
| const root = existing[baseName]; | |
| const subPaths = collectSubtablePaths(root, ''); | |
| for (const subPath of subPaths) { | |
| raw = removeSectionFromText(raw, `[mcp_servers.${baseName}.${subPath}]`); | |
| } | |
| } |
| awk -v begin="$ECC_BEGIN_MARKER" -v end="$ECC_END_MARKER" -v ecc="$ecc_tmp" ' | ||
| { gsub(/\r$/, "") } | ||
| $0 == begin { skip = 1; while ((getline line < ecc) > 0) print line; close(ecc); next } | ||
| $0 == end { skip = 0; next } | ||
| !skip { print } | ||
| ' "$AGENTS_FILE" > "$tmp" |
There was a problem hiding this comment.
replace_ecc_section strips \r from every processed line (gsub(/\r$/, "")) and then prints the modified line, which will normalize a CRLF AGENTS.md to LF across the entire file (including user-owned content outside the ECC markers). If preserving user content includes preserving line endings, consider comparing marker lines using a copy of $0 with sub(/\r$/, "", tmp) (or similar) while printing the original line unchanged, or restrict CR stripping to marker matching only.
| # Automatic setup: sync ECC assets (AGENTS.md, skills, MCP servers) into ~/.codex | ||
| npm install && bash scripts/sync-ecc-to-codex.sh | ||
|
|
||
| # Or manually: copy the reference config to your home directory | ||
| cp .codex/config.toml ~/.codex/config.toml |
There was a problem hiding this comment.
The README now suggests npm install && bash scripts/sync-ecc-to-codex.sh as an automatic setup path, but the sync script hard-requires an existing ~/.codex/config.toml and exits if it doesn’t exist. Either adjust the README to mention the prerequisite (create/copy a config first), or update the script to bootstrap config.toml from .codex/config.toml when missing.
| "dependencies": { | ||
| "@iarna/toml": "^2.2.5", | ||
| "sql.js": "^1.14.1" |
There was a problem hiding this comment.
package.json adds @iarna/toml, but package-lock.json in this branch does not contain any @iarna/toml entries. If CI uses npm ci (lockfile-driven), the install will fail or the dependency won’t be available at runtime. Please update and commit the lockfile (or switch the repo away from package-lock.json if that’s intentional).
scripts/codex/merge-mcp-config.js
Outdated
| function main() { | ||
| const args = process.argv.slice(2); | ||
| const configPath = args.find(a => !a.startsWith('-')); | ||
| const dryRun = args.includes('--dry-run'); | ||
| const updateMcp = args.includes('--update-mcp'); | ||
|
|
||
| if (!configPath) { | ||
| console.error('Usage: merge-mcp-config.js <config.toml> [--dry-run] [--update-mcp]'); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (!fs.existsSync(configPath)) { | ||
| console.error(`[ecc-mcp] Config file not found: ${configPath}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| let raw = fs.readFileSync(configPath, 'utf8'); | ||
| let parsed; | ||
| try { | ||
| parsed = TOML.parse(raw); | ||
| } catch (err) { | ||
| console.error(`[ecc-mcp] Failed to parse ${configPath}: ${err.message}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const existing = parsed.mcp_servers || {}; | ||
| const toAppend = []; | ||
|
|
||
| for (const [name, spec] of Object.entries(ECC_SERVERS)) { | ||
| // A subtable-only entry (e.g. mcp_servers.supabase.env without |
There was a problem hiding this comment.
The new merge logic in merge-mcp-config.js is non-trivial (TOML parsing + drift detection + --update-mcp section removal) but there are no tests covering it under tests/scripts/ (the repo does have a test harness for scripts). Adding a focused unit/integration test that runs the script against sample TOML fixtures (add-only, legacy alias, drift warning, update behavior) would help prevent regressions.
Greptile SummaryThis PR replaces the destructive overwrite/awk-surgery Codex sync approach with a safe, marker-based merge strategy for Key issues found:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[sync-ecc-to-codex.sh] --> B{Mode?}
B -->|dry-run| C[node merge-mcp-config.js --dry-run]
B -->|apply| D[node merge-mcp-config.js]
D --> E{--update-mcp?}
C --> E2{--update-mcp?}
E -->|no - add-only| F[Parse config.toml via @iarna/toml]
E -->|yes| F
E2 -->|no| F2[Parse + preview missing servers]
E2 -->|yes| F2
F --> G{For each ECC server}
G --> H{Entry exists?}
H -->|no| I[Append server TOML block]
H -->|yes, add-only| J{Drift?}
J -->|yes| K[Warn: run --update-mcp]
J -->|no| L[log ok]
H -->|yes, --update-mcp| M[removeServerFromText raw]
M --> N{Legacy alias present?}
N -->|resolvedLabel != name| O[Also remove legacy alias]
N -->|resolvedLabel == name ⚠️| P[Legacy alias NOT removed - BUG]
O --> Q[Append fresh server TOML]
P --> Q
Q --> R{toAppend empty?}
I --> R
R -->|no, --update-mcp| S[writeFileSync cleaned+appended]
R -->|no, add-only| T[appendFileSync]
S --> U[Done]
T --> U
style P fill:#ff6b6b,color:#fff
Last reviewed commit: "fix: address PR #723..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 5 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="scripts/codex/merge-mcp-config.js">
<violation number="1" location="scripts/codex/merge-mcp-config.js:208">
P2: `--update-mcp` subtable cleanup never runs because it expects dot-qualified keys from parsed TOML, but `parsed.mcp_servers` is nested. As a result `[mcp_servers.<name>.*]` blocks are not removed even though the comment says they should be, leaving stale config in update mode.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:1007">
P2: Codex MCP server counts conflict: Codex section now lists 6 auto-merged servers, but the cross-tool parity table still says 4 command-based servers. Align the parity table with the updated Codex MCP behavior to avoid confusing users.</violation>
</file>
<file name="scripts/sync-ecc-to-codex.sh">
<violation number="1" location="scripts/sync-ecc-to-codex.sh:205">
P2: Legacy unmarked ECC AGENTS content is never removed after migration, so duplicate ECC sections persist across runs despite the code comment claiming next-run deduplication.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 984-986: Update the "Automatic setup: sync ECC assets (AGENTS.md,
skills, MCP servers) into ~/.codex" section so it shows non-npm variants or
links to the package-manager section: replace the single "npm install && bash
scripts/sync-ecc-to-codex.sh" example with equivalent commands (pnpm install,
yarn install, bun install) plus the same bash invocation, or add a short note
linking to the existing package-manager documentation; edit the README header
text and the command block around "scripts/sync-ecc-to-codex.sh" to include
these alternatives or a pointer to the package-manager section.
In `@scripts/codex/merge-mcp-config.js`:
- Around line 38-97: ECC_SERVERS in merge-mcp-config.js has diverged from the
repo's .codex/config.toml (it removed mcp_servers.exa and added supabase), so
update the script to use a single source of truth: read/parse the canonical
Codex config (.codex/config.toml or the project reference) and build ECC_SERVERS
from that data instead of hardcoding the map; ensure the resulting map includes
the mcp_servers.exa entry (and matches keys like supabase, github/GH_BOOTSTRAP,
memory, sequential-thinking, etc.) and remove the duplicated hardcoded literals
so mergeMcpConfig or any code that references ECC_SERVERS derives entries
directly from the parsed config.
- Around line 192-210: The update-mcp branch fails to remove nested TOML
subtable headers because it only iterates Object.keys(existing) (top-level keys)
and never inspects nested properties under existing[name] or
existing[resolvedLabel]; fix this in the updateMcp block by recursively walking
the object at existing[name] and existing[resolvedLabel] to collect full dotted
subkeys (e.g., "supabase.env" from { supabase: { env: {...} } }) and then call
removeSectionFromText(raw, `[mcp_servers.${fullKey}]`) for each collected
fullKey (and also for the base name/resolvedLabel); use the existing symbols
name, resolvedLabel, existing, removeSectionFromText, and raw so the code
removes all nested sub-sections.
- Around line 32-96: The ECC_SERVERS entries currently hardcode "pnpm" and a
bash GH_BOOTSTRAP (symbols: ECC_SERVERS, GH_BOOTSTRAP) which breaks the repo
package-manager abstraction; import and call getPackageManager() from
scripts/lib/package-manager.js and use its returned launcher/flags instead of
literal "pnpm" or "bash -lc" when building each entry's fields.args and toml
strings (update entries: supabase, playwright, context7-mcp, github, memory,
sequential-thinking and the GH_BOOTSTRAP constant); ensure toml uses
JSON.stringify of the computed args, and for the github bootstrap create a
cross-platform invocation using the package manager's dlx invocation rather than
a bash shell so Windows is supported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a2efca8-c755-40b3-a2b1-6b350e3f3b49
📒 Files selected for processing (5)
.codex/AGENTS.mdREADME.mdpackage.jsonscripts/codex/merge-mcp-config.jsscripts/sync-ecc-to-codex.sh
| const ECC_SERVERS = { | ||
| supabase: { | ||
| fields: { | ||
| command: 'pnpm', | ||
| args: ['dlx', '@supabase/mcp-server-supabase@latest', '--features=account,docs,database,debugging,development,functions,storage,branching'], | ||
| startup_timeout_sec: 20.0, | ||
| tool_timeout_sec: 120.0 | ||
| }, | ||
| toml: `[mcp_servers.supabase] | ||
| command = "pnpm" | ||
| args = ["dlx", "@supabase/mcp-server-supabase@latest", "--features=account,docs,database,debugging,development,functions,storage,branching"] | ||
| startup_timeout_sec = 20.0 | ||
| tool_timeout_sec = 120.0` | ||
| }, | ||
| playwright: { | ||
| fields: { | ||
| command: 'pnpm', | ||
| args: ['dlx', '@playwright/mcp@latest'] | ||
| }, | ||
| toml: `[mcp_servers.playwright] | ||
| command = "pnpm" | ||
| args = ["dlx", "@playwright/mcp@latest"]` | ||
| }, | ||
| 'context7-mcp': { | ||
| fields: { | ||
| command: 'pnpm', | ||
| args: ['dlx', '@upstash/context7-mcp'] | ||
| }, | ||
| toml: `[mcp_servers.context7-mcp] | ||
| command = "pnpm" | ||
| args = ["dlx", "@upstash/context7-mcp"]` | ||
| }, | ||
| github: { | ||
| fields: { | ||
| command: 'bash', | ||
| args: ['-lc', GH_BOOTSTRAP] | ||
| }, | ||
| toml: `[mcp_servers.github] | ||
| command = "bash" | ||
| args = ["-lc", ${JSON.stringify(GH_BOOTSTRAP)}]` | ||
| }, | ||
| memory: { | ||
| fields: { | ||
| command: 'pnpm', | ||
| args: ['dlx', '@modelcontextprotocol/server-memory'] | ||
| }, | ||
| toml: `[mcp_servers.memory] | ||
| command = "pnpm" | ||
| args = ["dlx", "@modelcontextprotocol/server-memory"]` | ||
| }, | ||
| 'sequential-thinking': { | ||
| fields: { | ||
| command: 'pnpm', | ||
| args: ['dlx', '@modelcontextprotocol/server-sequential-thinking'] | ||
| }, | ||
| toml: `[mcp_servers.sequential-thinking] | ||
| command = "pnpm" | ||
| args = ["dlx", "@modelcontextprotocol/server-sequential-thinking"]` | ||
| } | ||
| }; |
There was a problem hiding this comment.
ECC_SERVERS has already drifted from the Codex reference config.
The repo's .codex/config.toml still ships mcp_servers.exa, but this map drops Exa and adds Supabase instead. In add-only mode that means copying the template and then running sync yields a different home config than the repo's own baseline. Please collapse this to a single source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/codex/merge-mcp-config.js` around lines 38 - 97, ECC_SERVERS in
merge-mcp-config.js has diverged from the repo's .codex/config.toml (it removed
mcp_servers.exa and added supabase), so update the script to use a single source
of truth: read/parse the canonical Codex config (.codex/config.toml or the
project reference) and build ECC_SERVERS from that data instead of hardcoding
the map; ensure the resulting map includes the mcp_servers.exa entry (and
matches keys like supabase, github/GH_BOOTSTRAP, memory, sequential-thinking,
etc.) and remove the duplicated hardcoded literals so mergeMcpConfig or any code
that references ECC_SERVERS derives entries directly from the parsed config.
- Use package-manager abstraction (scripts/lib/package-manager.js) instead of hardcoding pnpm — respects CLAUDE_PACKAGE_MANAGER, lock files, and project config - Add Yarn 1.x fallback to npx (yarn dlx unsupported in classic) - Add missing exa server to match .codex/config.toml baseline - Wire up findSubSections for --update-mcp nested subtable removal (fixes Greptile P1: Object.keys only returned top-level keys) - Fix resolvedLabel to prefer canonical entry over legacy alias when both exist (fixes context7/context7-mcp spurious warning) - Fix removeSectionFromText to handle inline TOML comments - Fix dry-run + --update-mcp to show removals before early return - Update README parity table: 4 → 7 servers, TOML-parser-based - Add non-npm install variants to README Codex quick start - Update package-lock.json for @iarna/toml Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
|
Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
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="scripts/codex/merge-mcp-config.js">
<violation number="1" location="scripts/codex/merge-mcp-config.js:253">
P1: `--update-mcp` fails to remove legacy alias sections when canonical and alias entries coexist, leaving stale duplicate MCP server definitions.</violation>
</file>
<file name=".codex/AGENTS.md">
<violation number="1" location=".codex/AGENTS.md:54">
P2: New docs conflict on MCP scope (6-server baseline vs 7 sync-managed), creating ambiguous setup expectations.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:1195">
P2: README now has conflicting Codex MCP server counts (7 vs 6), creating source-of-truth drift in setup docs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/sync-ecc-to-codex.sh`:
- Around line 188-205: The logic mixes exact-line awk matching with substring
grep which lets stray mentions slip into the corrupted path; change both
branches to use exact-line marker checks and validate counts: use awk (or
similar) against "$AGENTS_FILE" to count occurrences of ECC_BEGIN_MARKER and
ECC_END_MARKER, ensure exactly one begin and one end and that begin < end to
take the “replace_ecc_section” branch (call replace_ecc_section when counts==1
and ordered), and treat any other combination (e.g., only one marker, multiple
markers, or markers out-of-order) as corrupted—log via log and call
compose_ecc_block to fully replace the file. Ensure you reference
ECC_BEGIN_MARKER, ECC_END_MARKER, AGENTS_FILE, replace_ecc_section,
compose_ecc_block, and log when making the change.
- Around line 16-21: The argument-parsing loop currently ignores unknown flags
(so typos let MODE remain "apply"); modify the case in the for-loop that handles
"$arg" (the --dry-run/--update-mcp switch block) to include a default branch
that rejects unknown arguments: when an arg doesn't match known patterns, print
a clear error mentioning the unknown flag and exit non-zero (e.g., via exit 2)
so the script fails fast rather than silently continuing with MODE="apply".
Ensure the updated logic still sets MODE and UPDATE_MCP as before and runs for
the same symbols (MODE, UPDATE_MCP) used later.
- Around line 441-447: Preflight currently doesn't validate that Node and the
merge script exist before writing AGENTS/skills/prompts, so add checks for the
Node runtime and the MCP_MERGE_SCRIPT file early in the preflight validation
block (the same place you validate AGENTS, SKILLS, and PROMPTS paths);
specifically verify the `node` binary is runnable (e.g., `command -v
node`/equivalent) and that the path referenced by the `MCP_MERGE_SCRIPT`
variable (`scripts/codex/merge-mcp-config.js`) exists and is readable, and abort
with a clear log/error if either check fails so the script can fail fast rather
than leaving partially-applied state when `node "$MCP_MERGE_SCRIPT"
"$CONFIG_FILE"` is invoked later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ce7638d-b0cc-42ef-8201-45e2b877e556
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.codex/AGENTS.mdREADME.mdscripts/codex/merge-mcp-config.jsscripts/sync-ecc-to-codex.sh
✅ Files skipped from review due to trivial changes (1)
- .codex/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/codex/merge-mcp-config.js
- README.md
| for arg in "$@"; do | ||
| case "$arg" in | ||
| --dry-run) MODE="dry-run" ;; | ||
| --update-mcp) UPDATE_MCP="--update-mcp" ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Reject unknown flags before falling back to apply mode.
Because MODE starts as apply, a typo like --dryrun is silently ignored and the script still mutates ~/.codex. Please fail fast on unknown args.
Suggested fix
for arg in "$@"; do
case "$arg" in
--dry-run) MODE="dry-run" ;;
--update-mcp) UPDATE_MCP="--update-mcp" ;;
+ --help|-h)
+ printf 'Usage: %s [--dry-run] [--update-mcp]\n' "$0"
+ exit 0
+ ;;
+ *)
+ log "Unknown argument: $arg"
+ exit 2
+ ;;
esac
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for arg in "$@"; do | |
| case "$arg" in | |
| --dry-run) MODE="dry-run" ;; | |
| --update-mcp) UPDATE_MCP="--update-mcp" ;; | |
| esac | |
| done | |
| for arg in "$@"; do | |
| case "$arg" in | |
| --dry-run) MODE="dry-run" ;; | |
| --update-mcp) UPDATE_MCP="--update-mcp" ;; | |
| --help|-h) | |
| printf 'Usage: %s [--dry-run] [--update-mcp]\n' "$0" | |
| exit 0 | |
| ;; | |
| *) | |
| log "Unknown argument: $arg" | |
| exit 2 | |
| ;; | |
| esac | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-ecc-to-codex.sh` around lines 16 - 21, The argument-parsing loop
currently ignores unknown flags (so typos let MODE remain "apply"); modify the
case in the for-loop that handles "$arg" (the --dry-run/--update-mcp switch
block) to include a default branch that rejects unknown arguments: when an arg
doesn't match known patterns, print a clear error mentioning the unknown flag
and exit non-zero (e.g., via exit 2) so the script fails fast rather than
silently continuing with MODE="apply". Ensure the updated logic still sets MODE
and UPDATE_MCP as before and runs for the same symbols (MODE, UPDATE_MCP) used
later.
…ation) - Add Node.js and merge-mcp-config.js to preflight checks so the script fails fast before partial writes (CodeRabbit) - Validate marker counts: require exactly 1 BEGIN + 1 END in correct order for clean replacement (CodeRabbit) - Corrupted markers: strip all marker lines and re-append fresh block, preserving user content outside markers instead of overwriting - Move MCP_MERGE_SCRIPT to preflight section, remove duplicate Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
|
Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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="scripts/sync-ecc-to-codex.sh">
<violation number="1" location="scripts/sync-ecc-to-codex.sh:216">
P1: Malformed-marker recovery can truncate AGENTS.md by deleting all content after an unmatched BEGIN marker.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The old sync script used awk/sed to remove and re-append MCP server sections in config.toml, causing credential extraction races, duplicate TOML tables, and 3 fragile code paths with 9 remove_section_inplace calls each. Replace with a Node script (scripts/codex/merge-mcp-config.js) that uses @iarna/toml to parse the config, then appends only missing ECC servers — preserving all existing content byte-for-byte. Warns on config drift, supports legacy aliases (context7 → context7-mcp), and adds --update-mcp flag for explicit refresh. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
- Use package-manager abstraction (scripts/lib/package-manager.js) instead of hardcoding pnpm — respects CLAUDE_PACKAGE_MANAGER, lock files, and project config - Add Yarn 1.x fallback to npx (yarn dlx unsupported in classic) - Add missing exa server to match .codex/config.toml baseline - Wire up findSubSections for --update-mcp nested subtable removal (fixes Greptile P1: Object.keys only returned top-level keys) - Fix resolvedLabel to prefer canonical entry over legacy alias when both exist (fixes context7/context7-mcp spurious warning) - Fix removeSectionFromText to handle inline TOML comments - Fix dry-run + --update-mcp to show removals before early return - Update README parity table: 4 → 7 servers, TOML-parser-based - Add non-npm install variants to README Codex quick start - Update package-lock.json for @iarna/toml Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
…ation) - Add Node.js and merge-mcp-config.js to preflight checks so the script fails fast before partial writes (CodeRabbit) - Validate marker counts: require exactly 1 BEGIN + 1 END in correct order for clean replacement (CodeRabbit) - Corrupted markers: strip all marker lines and re-append fresh block, preserving user content outside markers instead of overwriting - Move MCP_MERGE_SCRIPT to preflight section, remove duplicate Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
chris-yyau
force-pushed
the
fix/codex-agents-merge-not-replace
branch
from
be1cbde to
35bf3d8
Compare
|
Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/sync-ecc-to-codex.sh (1)
16-21:⚠️ Potential issue | 🟡 MinorUnknown flags are silently ignored — typos could cause unintended writes.
The argument parsing loop ignores unrecognized flags, so a typo like
--dryrun(missing hyphen) will silently proceed withMODE="apply"and mutate~/.codex. Consider failing fast on unknown arguments.🛡️ Suggested fix to reject unknown flags
for arg in "$@"; do case "$arg" in --dry-run) MODE="dry-run" ;; --update-mcp) UPDATE_MCP="--update-mcp" ;; + --help|-h) + printf 'Usage: %s [--dry-run] [--update-mcp]\n' "$0" + exit 0 + ;; + *) + log "Unknown argument: $arg" + exit 2 + ;; esac done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-ecc-to-codex.sh` around lines 16 - 21, The argument parsing loop currently ignores unknown flags, so update the case in the for-arg loop (the "for arg in \"$@\"; do" / case "$arg" in) to add a default branch that prints an error including the unrecognized flag and exits non‑zero (e.g., echo "Unknown flag: $arg" >&2; exit 1) so typos like --dryrun fail fast instead of silently proceeding; keep existing --dry-run and --update-mcp branches unchanged.
🧹 Nitpick comments (1)
scripts/codex/merge-mcp-config.js (1)
102-104: Avoid mutating the array returned bydlxServer.The
push()call directly mutatesECC_SERVERS.supabase.fields.args, which violates the immutability guideline. While this is initialization-time code, the pattern can cause subtle bugs if this object is ever shared or ifdlxServeris refactored to cache results.♻️ Suggested refactor using spread operator
-// Append --features arg for supabase after dlxServer builds the base -ECC_SERVERS.supabase.fields.args.push('--features=account,docs,database,debugging,development,functions,storage,branching'); -ECC_SERVERS.supabase.toml = ECC_SERVERS.supabase.toml.replace(/^(args = \[.*)\]$/m, '$1, "--features=account,docs,database,debugging,development,functions,storage,branching"]'); +// Build supabase with --features arg included +const SUPABASE_FEATURES = '--features=account,docs,database,debugging,development,functions,storage,branching'; +const supabaseBase = dlxServer('supabase', '@supabase/mcp-server-supabase@latest', { startup_timeout_sec: 20.0, tool_timeout_sec: 120.0 }, 'startup_timeout_sec = 20.0\ntool_timeout_sec = 120.0'); + +/** Each entry: key = section name under mcp_servers, value = { toml, fields } */ +const ECC_SERVERS = { + supabase: { + fields: { ...supabaseBase.fields, args: [...supabaseBase.fields.args, SUPABASE_FEATURES] }, + toml: supabaseBase.toml.replace(/^(args = \[.*)\]$/m, `$1, ${JSON.stringify(SUPABASE_FEATURES)}]`) + },Alternatively, pass the features flag directly to
dlxServer.As per coding guidelines: "Always create new objects, never mutate existing ones."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/codex/merge-mcp-config.js` around lines 102 - 104, The code mutates ECC_SERVERS.supabase.fields.args via push; instead create a new args array and replace the field immutably (or pass the features flag into dlxServer) so no in-place mutation occurs: build a new args array from ECC_SERVERS.supabase.fields.args using spread (or construct the full args when calling dlxServer), then assign the new array back to ECC_SERVERS.supabase.fields.args (or set ECC_SERVERS.supabase = { ...ECC_SERVERS.supabase, fields: { ...ECC_SERVERS.supabase.fields, args: newArgs } }); also update the toml replacement to use the same features string without mutating the original args array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/sync-ecc-to-codex.sh`:
- Around line 16-21: The argument parsing loop currently ignores unknown flags,
so update the case in the for-arg loop (the "for arg in \"$@\"; do" / case
"$arg" in) to add a default branch that prints an error including the
unrecognized flag and exits non‑zero (e.g., echo "Unknown flag: $arg" >&2; exit
1) so typos like --dryrun fail fast instead of silently proceeding; keep
existing --dry-run and --update-mcp branches unchanged.
---
Nitpick comments:
In `@scripts/codex/merge-mcp-config.js`:
- Around line 102-104: The code mutates ECC_SERVERS.supabase.fields.args via
push; instead create a new args array and replace the field immutably (or pass
the features flag into dlxServer) so no in-place mutation occurs: build a new
args array from ECC_SERVERS.supabase.fields.args using spread (or construct the
full args when calling dlxServer), then assign the new array back to
ECC_SERVERS.supabase.fields.args (or set ECC_SERVERS.supabase = {
...ECC_SERVERS.supabase, fields: { ...ECC_SERVERS.supabase.fields, args: newArgs
} }); also update the toml replacement to use the same features string without
mutating the original args array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d7a6c1e-7d26-4622-9c69-8b656fc32d73
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.codex/AGENTS.mdREADME.mdpackage.jsonscripts/codex/merge-mcp-config.jsscripts/sync-ecc-to-codex.sh
✅ Files skipped from review due to trivial changes (2)
- .codex/AGENTS.md
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| awk -v b="$ECC_BEGIN_MARKER" -v e="$ECC_END_MARKER" ' | ||
| { gsub(/\r$/, "") } | ||
| $0 == b { skip = 1; next } | ||
| $0 == e { skip = 0; next } | ||
| !skip { print } | ||
| ' "$AGENTS_FILE" > "$_fix_tmp" |
There was a problem hiding this comment.
The awk on lines 214–219 sets skip = 1 when it sees the BEGIN marker and only resets to skip = 0 when it sees an END marker. When the "corrupted" case is a lone <!-- BEGIN ECC --> with no following <!-- END ECC --> (the most common corruption — accidental marker deletion), the awk enters skip = 1 and never exits it, so every line after the BEGIN marker — including user-authored content — is silently dropped before cat "$_fix_tmp" > "$AGENTS_FILE" overwrites the file.
The comment on line 211 says "This preserves user content outside markers", which is only true when the markers happen to be balanced within a corrupted-but-paired block (e.g. duplicates). For the lone-BEGIN case it is incorrect.
Concrete example:
My custom rules ← preserved ✓
<!-- BEGIN ECC -->
(old ECC content)
My other custom rules ← LOST (skip=1, never reset)
A backup is saved before this runs, but this is silent data loss — no additional warning is logged about lines being dropped.
One safe fix is to only skip lines that are between a matched BEGIN/END pair, and treat any trailing unmatched BEGIN block as recoverable user content:
awk -v b="$ECC_BEGIN_MARKER" -v e="$ECC_END_MARKER" '
{ gsub(/\r$/, "") }
$0 == b { skip = 1; next }
$0 == e { skip = 0; next }
!skip { print }
END { if (skip) { print "WARNING: unclosed BEGIN marker — content after it was dropped" > "/dev/stderr" } }
' "$AGENTS_FILE" > "$_fix_tmp"Or, more conservatively, do a two-pass: first scan to collect paired ranges, then delete only those ranges, leaving any unmatched tail intact.
| function dlxServer(name, pkg, extraFields, extraToml) { | ||
| const args = [...PM_EXEC_PARTS.slice(1), pkg]; | ||
| const fields = { command: PM_EXEC_PARTS[0], args, ...extraFields }; | ||
| const argsStr = JSON.stringify(args).replace(/,/g, ', '); |
There was a problem hiding this comment.
argsStr global comma replacement corrupts quoted string values
JSON.stringify(args).replace(/,/g, ', ') replaces all commas in the serialised output, including those that appear inside quoted string values. For the current set of server package names this is harmless, but the --features argument added to supabase ("--features=account,docs,database,debugging,development,functions,storage,branching") contains commas. That string is not routed through argsStr (it is spliced in via the regex on line 104), so the bug is dormant today.
However, if any future server is added via dlxServer with a package name or argument that contains a comma — or if the supabase --features arg is ever passed directly into dlxServer's extraFields.args — the replacement would corrupt the TOML array by splitting one string token into multiple tokens:
// JSON.stringify produces: ["dlx","--features=a,b,c"]
// after replace: ["dlx", "--features=a", "b", "c"] ← invalid TOML & wrong args
Build the pretty-printed array string element-by-element instead:
| const argsStr = JSON.stringify(args).replace(/,/g, ', '); | |
| const argsStr = '[' + args.map(a => JSON.stringify(a)).join(', ') + ']'; |
| if (updateMcp) { | ||
| for (const label of toRemoveLog) log(` [update] ${label}`); | ||
| const cleaned = raw.replace(/\n+$/, '\n'); | ||
| fs.writeFileSync(configPath, cleaned + appendText, 'utf8'); | ||
| } else { | ||
| fs.appendFileSync(configPath, appendText, 'utf8'); | ||
| } |
There was a problem hiding this comment.
After all removals and appends are assembled, the final content is written to disk without first verifying it is valid TOML. If removeSectionFromText leaves an orphaned key-value line, an empty section header, or any other syntax residue (possible given the text-surgery approach), the corrupt file will be written silently and Codex CLI will fail to start on the next run.
A round-trip parse before the write catches any malformed output and aborts with a clear error rather than silently corrupting the user's config:
const outputContent = updateMcp
? raw.replace(/\n+$/, '\n') + appendText
: raw + appendText;
try {
TOML.parse(outputContent);
} catch (err) {
console.error(`[ecc-mcp] BUG: generated output is invalid TOML — aborting write. ${err.message}`);
process.exit(1);
}
if (updateMcp) {
for (const label of toRemoveLog) log(` [update] ${label}`);
fs.writeFileSync(configPath, outputContent, 'utf8');
} else {
fs.appendFileSync(configPath, appendText, 'utf8');
}| if (dryRun) { | ||
| if (toRemoveLog.length > 0) { | ||
| log('Dry run — would remove and re-add:'); | ||
| for (const label of toRemoveLog) log(` [remove] ${label}`); | ||
| } | ||
| log('Dry run — would append:'); | ||
| console.log(appendText); | ||
| return; |
There was a problem hiding this comment.
--dry-run --update-mcp mutates raw before the early return
When both flags are passed, removeServerFromText is called (and raw is modified in-place in each loop iteration) before the dry-run early-return on line 288. The dry-run output lists what would be appended but says nothing about what would be removed. A user running --dry-run --update-mcp to preview the operation sees an incomplete picture.
The removal log is already collected in toRemoveLog (line 252) before raw is mutated, so showing it in the dry-run output requires no extra work:
| if (dryRun) { | |
| if (toRemoveLog.length > 0) { | |
| log('Dry run — would remove and re-add:'); | |
| for (const label of toRemoveLog) log(` [remove] ${label}`); | |
| } | |
| log('Dry run — would append:'); | |
| console.log(appendText); | |
| return; | |
| if (dryRun) { | |
| if (toRemoveLog.length > 0) { | |
| log('Dry run — would remove and re-add:'); | |
| for (const label of toRemoveLog) log(` [remove] ${label}`); | |
| } | |
| log('Dry run — would append:'); | |
| console.log(appendText); | |
| return; | |
| } |
The existing code already populates toRemoveLog correctly — the only change needed is to emit it before return. (The snippet above is identical to the current code; the issue is that raw is already mutated by the time we reach this block, so the mutation should be deferred to after the dry-run guard, or moved inside the !dryRun branch.)
| if (updateMcp) { | ||
| // --update-mcp: remove existing section (and legacy alias), will re-add below | ||
| toRemoveLog.push(`mcp_servers.${resolvedLabel}`); | ||
| raw = removeServerFromText(raw, resolvedLabel, existing); | ||
| if (resolvedLabel !== name) { | ||
| raw = removeServerFromText(raw, name, existing); | ||
| } | ||
| toAppend.push(spec.toml); |
There was a problem hiding this comment.
--update-mcp mode
When a user's config contains both [mcp_servers.context7] (legacy) and [mcp_servers.context7-mcp] (canonical), --update-mcp removes and re-adds context7-mcp but silently leaves the legacy context7 entry behind.
Trace-through:
hasCanonical = true(canonical entry hascommand)legacyName = 'context7'(still found inexisting)resolvedLabel = name = 'context7-mcp'(canonical wins, line 247)- Line 253 removes
context7-mcp✓ - Line 254:
resolvedLabel !== name→'context7-mcp' !== 'context7-mcp'→false→context7is never removed ✗ - Line 257 appends a fresh
context7-mcp
End result: config contains both context7 and context7-mcp. Codex CLI loads the Context7 server twice under different names.
The guard on line 254 is intended to skip a redundant second removal when the label already equals the canonical name, but it inadvertently prevents the legacy cleanup. The legacy alias should always be removed independently of resolvedLabel:
raw = removeServerFromText(raw, resolvedLabel, existing);
if (legacyName && legacyName !== resolvedLabel) {
raw = removeServerFromText(raw, legacyName, existing);
}
toAppend.push(spec.toml);
Summary
<!-- BEGIN ECC -->/<!-- END ECC -->markers to merge ECC content into existingAGENTS.mdinstead of overwriting with>. User content outside markers is preserved.scripts/codex/merge-mcp-config.js) using@iarna/toml. Add-only by default — missing ECC servers are appended, existing servers are never touched. Warns on config drift and supports legacy aliases (context7→context7-mcp).--update-mcpflag: Explicitly refreshes all ECC-managed servers when users want the latest recommended config..codex/AGENTS.mdandREADME.mdCodex sections to document the new merge behavior.Why
The old sync script (
>overwrite for AGENTS.md, awk section removal for config.toml) destroyed user customizations on every run. The marker approach for AGENTS.md was merged in the first commit; the config.toml approach failed codex review twice due to credential extraction races and duplicate TOML tables from bash text surgery. The Node TOML parser approach passed review on first try.Test plan
[mcp_servers.context7]detected as legacy forcontext7-mcp[mcp_servers.supabase.env]without parent block treated as missing--update-mcp: replaces all ECC servers, preserves user custom serversbash -nandnode --checkpass🤖 Generated with Claude Code
via Happy
Summary by cubic
Make Codex sync safe and non-destructive by merging ECC content into AGENTS.md and add-only merging MCP servers into
~/.codex/config.toml. Uses a Node TOML parser and package‑manager–aware commands, with--update-mcp, Yarn 1.x fallback, and stricter preflight/marker validation.New Features
scripts/codex/merge-mcp-config.jsusing@iarna/toml; respects the repo’s package manager and falls back tonpxfor Yarn 1.x.--update-mcpto refresh;--dry-runpreviews changes.Bug Fixes
<!-- BEGIN ECC -->/<!-- END ECC -->pair; corrupted markers are stripped and a fresh block re-appended; no markers → append; CRLF-safe with backups.--update-mcpremoves nested subtables, handles inline comments, prefers canonical names over legacy aliases, and only warns on drift without touching user config.Written for commit 35bf3d8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation