Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3046ee550c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
*
*
7 Eyl 2026 Pzt 14:30 tarihinde めがねこ ***@***.***> şunu yazdı:
… ***@***.**** commented on this pull request.
------------------------------
In plugins/codex/scripts/session-lifecycle-hook.mjs
<#748 (comment)>
:
> + if (fs.readFileSync(process.env.CLAUDE_ENV_FILE, "utf8").includes(line)) {
+ return;
CLAUDE_ENV_FILE only set once.
—
Reply to this email directly, view it on GitHub
<#748?email_source=notifications&email_token=B3WU6VV5PGJILAXWDGSF6QT5N2L5ZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJTGE2DMOJRGA32M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r3949290453>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B3WU6VRQLK6BMDPLSYL63IT5N2L5ZAVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJWGMZDCMJWGI5US43TOVSTWNJTGY4TGNBUGYYTNILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/B3WU6VWVRGS4RIZ65427TPT5N2L5ZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJTGE2DMOJRGA32M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ>
and Android
<https://github.com/notifications/mobile/android/B3WU6VVOMB3TVMORQCIH5ZL5N2L5ZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJTGE2DMOJRGA32M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
MeGaNeKoS
force-pushed
the
fix/idempotent-appendenvvar
branch
from
September 7, 2026 12:18
3046ee5 to
4ab1fa2
Compare
MeGaNeKoS
changed the title
Author
|
Switching this from "append, skip if the line already exists" to set semantics, and it's worth saying why, since the append version looks simpler. The env file is sourced, so the effective value of a key is whatever its last
Tradeoff, to be upfront: set does a read plus a full rewrite per call instead of one append. That's negligible here, the file is well under 1 KB and SessionStart is an occasional event (startup/resume/clear/compact), not a hot path. On Windows, rename over an open file can rarely throw EPERM in that same narrow double-fire window; happy to add a small retry if you'd prefer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ab1fa24bd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+53
to
+55
| const tmp = `${envFile}.${process.pid}.tmp`; | ||
| fs.writeFileSync(tmp, lines.join("\n") + "\n", "utf8"); | ||
| fs.renameSync(tmp, envFile); |
There was a problem hiding this comment.
When another matching SessionStart hook writes to the shared CLAUDE_ENV_FILE after this read, the subsequent whole-file rename silently discards that hook's export. This can occur in installations with additional user or plugin SessionStart hooks, leaving later commands without their persisted environment variables; use an append-based update or otherwise serialize read-modify-write access.
Useful? React with 👍 / 👎.
Comment on lines
+53
to
+55
| const tmp = `${envFile}.${process.pid}.tmp`; | ||
| fs.writeFileSync(tmp, lines.join("\n") + "\n", "utf8"); | ||
| fs.renameSync(tmp, envFile); |
There was a problem hiding this comment.
When Claude created the existing environment file with a restrictive mode such as 0600 and its parent directory is traversable, writeFileSync creates the replacement using the default 0666 & umask mode—commonly 0644—and the rename installs those broader permissions. Because this file may contain exports from other hooks, including credentials, replacing it should preserve the original mode or explicitly create the temporary file with equally restrictive permissions.
Useful? React with 👍 / 👎.
|
*
*
7 Eyl 2026 Pzt 15:22 tarihinde chatgpt-codex-connector[bot] <
***@***.***> şunu yazdı:
… ***@***.***[bot]* commented on this pull request.
💡 Codex Review
Here are some automated review suggestions for this pull request.
*Reviewed commit:* 4ab1fa2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo.
Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment ***@***.*** <https://github.com/codex> review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT <https://openai.com/codex>,
Codex can also answer questions or update the PR, like ***@***.***
<https://github.com/codex> address that feedback".
------------------------------
In plugins/codex/scripts/session-lifecycle-hook.mjs
<#748 (comment)>
:
> + const tmp = `${envFile}.${process.pid}.tmp`;
+ fs.writeFileSync(tmp, lines.join("\n") + "\n", "utf8");
+ fs.renameSync(tmp, envFile);
*[image: P1 Badge]
<https://camo.githubusercontent.com/c595229c0ecb6ee85b9c7804144d495f131a495ec87091fea2b262d954c9a92d/68747470733a2f2f696d672e736869656c64732e696f2f62616467652f50312d6f72616e67653f7374796c653d666c6174>
Avoid overwriting concurrent SessionStart hook updates*
When another matching SessionStart hook writes to the shared
CLAUDE_ENV_FILE after this read, the subsequent whole-file rename
silently discards that hook's export. This can occur in installations with
additional user or plugin SessionStart hooks, leaving later commands
without their persisted environment variables; use an append-based update
or otherwise serialize read-modify-write access.
Useful? React with 👍 / 👎.
------------------------------
In plugins/codex/scripts/session-lifecycle-hook.mjs
<#748 (comment)>
:
> + const tmp = `${envFile}.${process.pid}.tmp`;
+ fs.writeFileSync(tmp, lines.join("\n") + "\n", "utf8");
+ fs.renameSync(tmp, envFile);
*[image: P2 Badge]
<https://camo.githubusercontent.com/f2c1aacb361ddd3a0e9f9cacdb84fab050de434017f6747bb916e31e29bdf03d/68747470733a2f2f696d672e736869656c64732e696f2f62616467652f50322d79656c6c6f773f7374796c653d666c6174>
Preserve restrictive permissions on the environment file*
When Claude created the existing environment file with a restrictive mode
such as 0600 and its parent directory is traversable, writeFileSync
creates the replacement using the default 0666 & umask mode—commonly 0644—and
the rename installs those broader permissions. Because this file may
contain exports from other hooks, including credentials, replacing it
should preserve the original mode or explicitly create the temporary file
with equally restrictive permissions.
Useful? React with 👍 / 👎.
—
Reply to this email directly, view it on GitHub
<#748?email_source=notifications&email_token=B3WU6VWT3AHNJTNDWIYQZXL5N2SAXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJTGE4TENJVGU2KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-5131925554>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B3WU6VXT6FSPYLU6ZC47F2T5N2SAXAVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJWGMZDCMJWGI5US43TOVSTWNJTGY4TGNBUGYYTNILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/B3WU6VWP563JIUTVFD4QVWD5N2SAXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJTGE4TENJVGU2KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/B3WU6VV5AZSXQGBCD2EDEFT5N2SAXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJTGE4TENJVGU2KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you commented.Message ID:
***@***.***>
|
Edo771977
pushed a commit
to Edo771977/codex-plugin-cc
that referenced
this pull request
Sep 17, 2026
…ted code The remaining findings from the independent review. None came from this branch's grafts; all four arrived with imported PRs and are now this fork's. - state.mjs: the cross-root prune rewrote another root's state.json while holding only the primary's lock, so a process whose primary IS that root could lose its whole update to the rename. It now takes that root's own lock and never waits for it (timeoutMs: 0): two processes each holding the other's primary lock would deadlock, and a skipped prune is harmless — the next save redoes it. - state.mjs: loadState() merges booleans across roots with OR, so a stranded `stopReviewGate: true` outvoted an explicit disable forever whenever the durable config could not be read, because setConfig() only ever wrote the primary. It now writes the new config into every existing root (same non-blocking lock discipline), keeping the fail-safe OR without making "disable" unreachable. The merge also folds candidates in reverse so the primary wins for non-boolean keys — the old "first writer wins" branch was dead for every key the defaults define, which is all of them. - session-lifecycle-hook.mjs: setEnv() rewrote the shared CLAUDE_ENV_FILE (read, filter, rename), which drops any export another plugin's SessionStart hook appended in between and discards the file's mode with the replaced file. It appends again, and skips the append when the value the file already resolves to is ours — the shell takes the last export for a key, so openai#748's point (no growth on every session) survives without the data loss. - claude-session-transfer.mjs: a process attaching to a staged copy whose creator had not yet written the marker took no lease, and the creator's release() then deleted the file under it. The lease is now taken unconditionally, and cleanup belongs to whoever leaves last (marker present, no leases left) rather than to whoever created the copy. Regression tests for the first three; each fails with only its own fix reverted. The staging race has no deterministic test — it needs an interleaving between two processes at a specific point — so it rests on the reasoning above. Verified: full npm test 310/310; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Edo771977
pushed a commit
to Edo771977/codex-plugin-cc
that referenced
this pull request
Sep 17, 2026
…ound of fixes - The retained orphan kept its dead pid, which made it reapable: another session's SessionEnd failed the record without an interrupt and it stopped pinning the broker, tearing the runtime down under the turn the retain protects. The verdict is persisted instead — pid: null plus workerExited — and reconcileJobLiveness() reads that flag, so the record stays truthful in /codex:status while a pid-less active record keeps the broker up under the existing staleness bound. The test now also ends a second session and asserts the job survives it. - Skipping assertResumedSandbox() for scoped resumes also dropped the escalation check. A dedicated one replaces it: a thread running with the sandbox disabled cannot be scoped by a permission profile, so --read-root on it is refused rather than silently promising a scope. - saveState() deleted a dropped job's files from every root while the prune of another root's state.json is best-effort, so a contended prune left a record to be merged back — and rewritten into the primary — with its detail file, claim and log already gone. Each root's files now go with its own record. - setEnv() appended without ensuring the file ends in a newline, so a preceding hook's unterminated line and ours would run together and lose both exports. - The append's own comment (and the README's line for openai#748) claimed the file no longer grows per session, which is false for values that change every session — the session id and transcript path. Both now say what actually holds: unchanged values are skipped, changed ones append, and the shell takes the last export. Bounded growth is the price of never destroying another plugin's export. - release() took the 5s staging lock even when there was nothing to clean up, from a finally, so a busy lock replaced the import error that was unwinding. It is wrapped now, with a lock-free unlink of our own lease as the fallback. Verified: full npm test 310/310; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
handleSessionStartcallsappendEnvVarforCODEX_COMPANION_SESSION_ID,CODEX_COMPANION_TRANSCRIPT_PATH, andCLAUDE_PLUGIN_DATAon every fire.hooks/hooks.jsonregisters SessionStart with no matcher, so it runs on startup, resume, /clear, and compact. Those three values are constant for the life of a session, so every fire after the first re-appends bytes already inCLAUDE_ENV_FILE. The file grows without bound.Claude Code inlines that file into the
bash -cpreamble of every Bash tool call, so past a size threshold every command in the session breaks. On Windows (Git Bash) I saw one env file reach 1248 lines / 130 KB containing only 3 unique lines, with three failure shapes by size:unexpected EOF while looking for matching 'ENAMETOOLONG ... uv_spawnQuote escaping in
shellEscapeis correct; the bug is purely the unbounded append.Fix
Replace the unconditional append with set semantics.
setEnvremoves any existingexport <name>=line and writes the current value, so each key appears once with its latest value. This bounds the file and, unlike dedup-by-line, correctly handles a key returning to an earlier value (A -> B -> A ends at A, not a stale B). The write goes through a temp file plus rename so a concurrent SessionStart can't read a half-written file.Test
Repeated identical fires keep the file at one line per key; a fire with a changed value replaces the line instead of appending.