fix(coder-labs/modules/codex): prevent config.toml overwrite on restart#896
Conversation
35C4n0r
force-pushed
the
35C4n0r/idempotent-codex-config
branch
from
9be9f74 to
f980245
Compare
35C4n0r
changed the title
After a dasel roundtrip, TOML values use single quotes instead of double quotes. Update the codex-with-ai-gateway and ai-gateway-with-custom-base-config tests to use regex matching that accepts either quote style. Also fix idempotent-run-twice-no-change to read the config file directly from the container instead of piping TOML strings through shell echo (which breaks on single quotes).
The idempotent-run-twice-no-change test was calling dasel in a separate execContainer shell where the PATH export from the install script is not available. Instead, compare the raw config output after runs 2 and 3 (both post-roundtrip, so serialization is stable and byte-comparison is valid).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…le dasel conversion Replace TOML string concatenation with jq-native JSON building: - Extract write_minimal_default_config() back as its own function, now returning JSON on stdout via jq. - populate_config_toml() assembles all config sources as JSON, deep-merges with jq, and does a single dasel JSON-to-TOML conversion at the end. - Remove merge_toml_config() and all TOML string building. - Update test assertions to accept either quote style since all output now goes through dasel.
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
/coder-agents-review |
Script fixes: - Rename write_minimal_default_config to build_minimal_default_config (no longer writes to disk, emits JSON to stdout). - Guard corrupted existing config: if dasel cannot parse the existing TOML, error out and exit instead of silently proceeding. - Atomic config write: write to a temp file and mv, preventing data loss if the process is interrupted mid-write. - Add jq availability check before populate_config_toml, consistent with how other registry modules handle hard dependencies. - Normalize blank lines between function definitions. Test fixes: - idempotent-mcp-deep-merge: use sed address range to only replace the github server command, assert filesystem command is still npx. - workdir-trusted-project: tighten regex to require bracket syntax instead of matching any line containing the path. - Rename idempotent-run-twice-no-change to idempotent-stable-after-roundtrip (test runs 3 times, not 2). - Remove unnecessary regex escaping of forward slashes. - Strengthen combination test assertions to check values, not just key presence.
There was a problem hiding this comment.
why are we doing toml to json to toml? a few real costs with this:
- comments in
~/.codex/config.tomlget stripped on every restart (dasel can't preserve them). - two new binary deps (dasel + jq), and the dasel download isn't checksum-verified.
two cleaner options:
marker block: module owns a fenced region, user owns everything outside.
# >>> coder-managed: codex module >>>
preferred_auth_method = "apikey"
[mcp_servers.github]
command = "npx"
# <<< coder-managed: codex module <<<sed strips and re-emits the block on each run. no deps, comments preserved, byte-stable. overrides go through terraform variables, which matches the rest of the registry.
yq (mikefarah) native toml: if we keep merging, yq -p toml -o toml eval-all '. as $i ireduce ({}; . *+ $i)' a.toml b.toml is the same thing in one binary instead of two. still loses comments but a strict win over dasel + jq.
prefer marker block. happy to be wrong if there's a reason.
|
yq dosen't seem to work. |
|
as for the block marker, everything written by the module will go inside the block marker (this includes the mcps provided by the user and the base_toml or user_provided_toml). |
…enhance idempotency
…ser bare keys and sections
… in config handling
35C4n0r
left a comment
35C4n0r
left a comment
There was a problem hiding this comment.
The marker-block approach is a solid design: switching from destructive overwrite to idempotent merge is the right call, and the atomic write (temp-file then mv) is good practice. The test suite has good breadth.
Two correctness bugs in the script and two test gaps need attention before merge. 2 P1 (script bugs), 2 P1 (test gaps), 4 P2, 2 P3, 2 nits across 10 inline comments.
Generated by Coder Agents (deep-review)
… deduplicate bare keys on upgrade
… instead of hoisting legacy content
…ines to prevent newline drift
…re/user_after awk extraction
…ument file structure
|
@andrewdennis117 PR is ready for review, I'll do a version bump in README once the code review passes. |
35C4n0r
changed the title
4 participants
Description
Replace the destructive overwrite of
~/.codex/config.tomlwith a marker-block merge. The module owns a fenced region of the file; everything outside the markers is user-owned and preserved across restarts.Sample Template: https://dev.coder.com/templates/product/codex-pr-896/docs
Type of Change
Module Information
Path:
registry/coder-labs/modules/codexNew version: v5.1.1
Breaking change: [ ] Yes [x] No
Testing & Validation
bun test)bun fmt)Closes: REG-11