Skip to content

[Bug] ocx config set / ocx config import bypass the protected 3-way reconcile and can clobber concurrent disk / hand edits #1838

Closed

Description

@trungtaottn

[Bug] ocx config import silently deletes already-saved hand edits (deterministic); ocx config set/unset can drop a concurrent edit (race)

Issue #1838, rewritten from a real user's live experience.

Client or integration

Codex CLI (ocx) — config import, config set, config unset.

Summary

As a user, I hand-edit ~/.opencodex/config.json (I set streamMode, or tweak emptyCompletionRetry, or add a field of my own). Then I run a normal config command. My hand-written values get silently deleted, with no warning, and I only notice when I ocx config show later.

Two distinct ways this happens, in increasing order of seriousness:

  1. ocx config import file.json --yes destroys an edit that is already committed to disk — even with no concurrency. Import is a wholesale overwrite: whatever the import file does not contain, the CLI forgets. I proved this live: hand-edit → import a valid file lacking that field → edit gone. No race involved.
  2. ocx config set / unset uses a read-then-write against a stale snapshot. If something edits the file in the tiny window between the command's read and its write, that edit is dropped. This one is a real race and is narrower.

I expected config management commands to preserve the fields on disk they aren't touching. They don't.

Reproduction — deterministic import clobber (no concurrency needed)

Runs from a fresh temp HOME; CLI invoked via the bundled Bun runtime:

# 1. create a real config file
ocx config set emptyCompletionRetry true
#    -> writes ~/.opencodex/config.json with emptyCompletionRetry=true

# 2. hand-edit it like a normal user (this is committed to disk now)
#    add a field AND toggle an existing one
> edit config.json: add  "streamMode": "eager-relay",  "customFlag": "user-hand-edit"
>                   set  "emptyCompletionRetry": false

ocx config show --json | grep -E 'streamMode|emptyCompletionRetry|customFlag'
# streamMode: eager-relay ; emptyCompletionRetry: false ; customFlag: user-hand-edit   <- present

# 3. import a schema-valid file that happens to omit those edited fields
ocx config import ./fresh.json --yes       # fresh.json has emptyCompletionRetry=true, no streamMode/customFlag
# -> "Imported config from ./fresh.json. Restart or run ocx sync if needed."  (exit 0)

# 4. the on-disk hand edits are gone
ocx config get streamMode          # error: config path not found: streamMode
ocx config get customFlag          # error: config path not found: customFlag
ocx config get emptyCompletionRetry
# -> true     (my hand-toggled false has been reverted)

Observed output (live, Bun 1.3.14, v2.22.0):

  • config import ./fresh.json --yes → Imported config from ./fresh.json. Restart or run ocx sync if needed. (exit 0).
  • After (ocx config show disk state): streamMode, customFlag both deleted; emptyCompletionRetry back to true. All three hand edits destroyed.

There was no concurrency in this reproduction: the hand edits were fully written to disk before the command was invoked.

Caveat discovered live: import is not a blind overwrite — it validates the import file against the config schema first and refuses to write on an invalid input (an import file missing providers errored with schema_invalid and wrote nothing). The data loss happens when the import file is schema-valid but simply doesn't carry the user's extra field, which is exactly the common case.

Reproduction — set/unset race (confirmed with a concurrent writer)

ocx config set some.other.path value reads the current config snapshot, applies only its one path, and writes the whole object back. An edit that lands between the snapshot read and the write is overwritten by the stale snapshot. I reproduced this live by running ocx config set port 10100 in a loop while a separate process continuously rewrote an unrelated field (a monotonic counter) in the same file: 34 of 51 rounds the counter was reverted to a stale lower value — i.e. set wrote a snapshot that predated the concurrent edit and destroyed it. Without an aggressive concurrent writer the window is narrow and often passes, which is why a plain hand-edit-then-set usually looks safe and this hides.

Expected vs Actual

Expected: ocx config set/unset rebase onto whatever is on disk right now, so they never destroy a concurrent hand edit. ocx config import (a deliberate overwrite) diffs its result against the current on-disk config and warns about keys it is about to drop before saving.

Actual:

  • import silently overwrites the whole file; any key the import file lacks is dropped with no diff/warning. Reproduced deterministically.
  • set/unset write a whole-object snapshot taken a moment earlier; a concurrent disk edit in between is lost. Race reproduced live (34/51 counter reverts) under a concurrent writer.

Evidence — what I actually ran

Live CLI (throwaway copy /tmp/opencodex-cli, Bun 1.3.14, source v2.22.0 d9de895):

  • Baseline: ocx config set emptyCompletionRetry true → file written.
  • Hand-edit of streamMode, customFlag, emptyCompletionRetry committed to disk.
  • Schema-valid ocx config import fresh.json --yes (file omitting those fields) → exit 0, then ocx config show confirmed all three edits gone. ─ deterministic, no concurrency.
  • ocx config set port 10100 under a high-frequency concurrent writer with a monotonic disk counter → 51 valid rounds, 34 lost-update reverts of the counter (monotonic counter regressed to a lower snapshot value = stale snapshot written over a newer concurrent edit).

Source facts (read, not inferred):

  • src/cli/config-command.ts:133 — config set/unset snapshot readConfigDiagnostics().config.
  • src/cli/config-command.ts:145 — set/unset persist via raw saveConfig(config) (whole-object write, no disk rebase).
  • src/cli/config-command.ts:178 — saveConfig(validate(loadInput(path))); loadInput (:67) reads only the import file, never the current config → import is structural whole-file overwrite.
  • src/config.ts:2766 saveConfig — only custom-model-catalog reconciliation, then persistConfigUnlocked; no general 3-way on-disk rebase.
  • src/config.ts:3185 saveConfigPreservingClaudeCode — the rebasing writer (re-reads disk, reconciles); used by server-side writers, not by these CLI config commands.
  • src/server/index.ts:562 — baseline is armed at server start.
  • Related open [Bug] ocx sync overwrites hand-edited config.json from stale server memory (GLM-5.3 tune lost); 2.21.0 save path fixed it but sync path question remains #1802 — same clobber family on the service path; its fix covered service saves, not these CLI commands.

Expected behavior (what I'd ask of a fix)

  • ocx config set/unset should merge onto the current on-disk config before writing (like the server-side rebasing writer), so a concurrent hand edit is never dropped.
  • ocx config import should diff its candidate against the current on-disk config and warn about (or require acking) keys it is about to remove, since it is a destructive whole-file overwrite.

Version

2.22.0 (main d9de895)

Operating system

Linux

Checks

Activity

  1. coderabbitai commented on Aug 16, 2026

    @coderabbitai
    Contributor
    ⚠️ Possible Duplicate Issue(s)
    🔗 Related PRs

    #697 - security: close audit findings 1/2/4/7/9/11 and partially 3/16 [merged]
    #770 - fix: research-batch reliability and security-boundary issues [closed]
    #852 - feat(export): client config export for Pi and OpenCode [merged]
    #886 - fix(server,config): extension CORS origin identity + symlink-safe atomic writes [open]
    #892 - fix: bound the six remaining retained-state leaks (#820 wave 2) [merged]
    #1324 - fix(codex): recover incomplete account deletion cleanup [open]
    #1331 - fix(catalog): keep failover combos when live discovery omits members [merged]
    #1744 - feat: land six repaired cherry-picks (MiniMax, picker order, terminal guard, streamAborted, imageInput, droid docs) [merged]


    📝 Issue Planner

    Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.

    • Create Plan

    🧪 Issue enrichment is currently in open beta.

    To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

    issue_enrichment:
      auto_enrich:
        enabled: false

    💬 Have feedback or questions? Drop into our discord!

  2. added
    bugSomething isn't working
    cliCLI, config inject, packaging flags
    on Aug 16, 2026
  3. github-actions commented on Aug 16, 2026

    @github-actions
    Contributor

    Maintainer decision respected

    A maintainer has reopened this issue. The automated closure has been deactivated.

  4. trungtaottn commented on Aug 16, 2026

    @trungtaottn
    Author

    Folded into #1837 — the two defects (routing optimize.latency + CLI config clobber) are now tracked together in #1837 (OPEN). Keeping this record closed to avoid duplicates.

  5. lidge-jun commented on Aug 16, 2026

    @lidge-jun
    Owner

    Reopening: the technical report here is still accurate on dev at 7c348a032, and it is being fixed.

    config set/unset read the disk snapshot at src/cli/config-command.ts:133, outside the mutation lock, and then sent that whole older snapshot through saveConfig at :145. A concurrent edit landing in between is silently reverted while the CLI reports success.

    The fix moves the operation inside mutatePersistedConfig, which already exists (src/config.ts:2884) and reruns the callback against the latest validated disk state. No new mutation primitive is needed.

    import is deliberately left as a whole-document replacement — forcing it through patch semantics would silently merge where the user asked to replace.

  6. added a commit that references this issue on Aug 16, 2026
    50ad068
  7. lidge-jun commented on Aug 16, 2026

    @lidge-jun
    Owner

    Fixed on dev by #1848 (merged as 73440d4bb).

    config set/unset now run inside mutatePersistedConfig, which reruns the operation against the latest validated disk state, so it is applied to what is actually there at commit time rather than to a snapshot read before the lock. No new primitive was needed — that one already existed.

    Two details worth recording, both of which had to be right for this to be a real fix:

    • changed compares a snapshot taken before the mutation. Comparing after the write compares a value with itself, reports every no-op as a change, and bumps the config generation for nothing.
    • The committed object is replaced, not merged. Object.assign alone cannot remove a key, so unset would have reported success while changing nothing. That case was driven red before landing.

    Validation, the saved-value readback and the codexAccountPriorities pin release all moved inside the transaction — leaving the pin clear outside would have reintroduced the same race for the pin.

    import is deliberately unchanged. It is an intentional whole-document replacement, and forcing it through patch semantics would silently merge where you asked to replace. The dropped-key warning idea from this report is recorded as separate follow-up rather than bundled in.

    Closing as completed. The change is on dev and ships with the next release.

  8. added a commit that references this issue on Sep 17, 2026
    0bc5605
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingcliCLI, config inject, packaging flags

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions