fix: persist review gate config outside transient state - #731
fscfede-beep wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e123511d2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Moving stopReviewGate out of transient plugin state fixes the authority boundary: Setup and Stop may legitimately run with different CLAUDE_PLUGIN_DATA, but the workspace policy must remain the same. Reusing one canonical workspace key for both paths avoids a second identity scheme, and preferring durable config while mirroring legacy state gives a reasonable compatibility transition. The cross-root integration test pins the actual fail-open bug rather than only the storage helper.
|
Thanks — giving the fixtures their own |
Upstream PR openai#731 (fscfede-beep), two conflicts resolved. The review-gate flag lived in the workspace state file under CLAUDE_PLUGIN_DATA, so a different plugin data root (or a cleared state dir) silently reverted the gate to off while /codex:setup still reported it as enabled. It now lives in a durable per-workspace file under CODEX_HOME, with the state copy kept in sync as a cache. Conflicts: both in tests/state.test.mjs, both unions, and the second one cut through the fork's last test again — its closing `});` is restored in the resolution. One change beyond the PR: writeDurableConfig() created the file with fs.writeFileSync and a plain mkdirSync, while every other artifact this module writes goes through ensurePrivateDir()/writeJsonFileAtomic(). It now does too, so the new config file is 0600 like the rest and a torn write cannot silently disable the gate on the next read. Verified: node --check on state.mjs; tests/state.test.mjs, tests/runtime.test.mjs and tests/commands.test.mjs 134/134. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Adds openai#731, openai#737, openai#747 and openai#763 to "Differences From Upstream", and notes in Requirements that Node no longer has to be on the system PATH now that the hooks go through scripts/run-node.sh (including CODEX_COMPANION_NODE for pinning one). Verified: full npm test 254/254. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
|
Picked this up on a fork — the durable config it introduces is the one artifact in function writeDurableConfig(cwd, config) {
const configFile = resolveConfigFile(cwd);
fs.mkdirSync(path.dirname(configFile), { recursive: true });
const nextConfig = { ...defaultState().config, ...(config ?? {}) };
fs.writeFileSync(configFile, `${JSON.stringify(nextConfig, null, 2)}\n`, "utf8");
return nextConfig;
}Everything else the module creates goes through function writeDurableConfig(cwd, config) {
const configFile = resolveConfigFile(cwd);
- fs.mkdirSync(path.dirname(configFile), { recursive: true });
+ ensurePrivateDir(path.dirname(configFile));
const nextConfig = { ...defaultState().config, ...(config ?? {}) };
- fs.writeFileSync(configFile, `${JSON.stringify(nextConfig, null, 2)}\n`, "utf8");
+ writeJsonFileAtomic(configFile, nextConfig);
return nextConfig;
}The PR's tests pass unchanged with it. |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked current a81f52f9. writeDurableConfig() still writes the persistent review-gate policy directly with writeFileSync, so an interrupted write can truncate/corrupt the only durable copy, and the file mode is left to the caller umask. Since this file contains persistent trust-root/review policy, please write via a same-directory temporary file and atomic rename, enforce a restrictive mode (for example 0600), and add coverage for permissions plus preservation of the previous config when a replacement write fails.
…eplacement The review on openai#731 asks for exactly this coverage, and this fork already carries the hardening it requests (ensurePrivateDir plus writeJsonFileAtomic instead of mkdirSync plus writeFileSync), so the tests belong here too. - "the durable review-gate config is private" pins 0600 on the file and 0700 on its directory. It discriminates: reverting writeDurableConfig() to the plain writeFileSync the PR shipped with fails it. - "a durable config write that fails mid-write leaves the previous config intact" fails the replacement from inside writeJsonFileAtomic(), after it has created its temporary file, using a value whose toJSON() throws. It asserts the previous config still reads back enabled and that no temporary file is left beside it. This one does not discriminate against the naive implementation (which throws before touching the file either way); what it guards is the regression class where a future rewrite truncates the target before serializing, and the cleanup path of the atomic write. Verified: full npm test 302/302; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Summary
Fixes #684.
CODEX_HOME/plugin-cc/config/<workspace>.jsonCLAUDE_PLUGIN_DATA/ temp rootsgetConfig()prefer the durable config while retaining the existing state config as a legacy fallbacksetConfig()into the current legacy state for compatibilityThis removes
CLAUDE_PLUGIN_DATAfrom the authority boundary forstopReviewGate, so Setup and Stop can run with different plugin-data environments without silently disagreeing about whether the gate is enabled.Validation
Windows 11 / Node 26.3.1:
main: enable under plugin-data root A, read under root B ->false(fail-open)true, and disabling under B is observed under ACODEX_HOMEand correctly returns a blocking review decisionnode --check plugins/codex/scripts/lib/state.mjsgit diff --check