fix: rewrite session env exports instead of appending them - #657
fix: rewrite session env exports instead of appending them#657lukehutch wants to merge 1 commit into
Conversation
handleSessionStart appended three export lines to CLAUDE_ENV_FILE on every firing, and SessionStart fires on startup, on resume and on every compaction. Nothing ever pruned the file, and only the last assignment of a name has any effect, so every earlier copy was dead weight. Claude Code inlines the whole env file into the single `bash -c <script>` argument it spawns, so a long-running session eventually pushed that one argument past the operating system's limit on the length of a single argument (MAX_ARG_STRLEN, 131072 bytes on Linux). At that point execve fails with E2BIG and no shell can be started at all: every Bash tool call fails regardless of how short the command itself is. The same growth truncates the command line at 8191 characters on Windows. Drop this plugin's own export lines, keep every other line verbatim, append the current values, and skip the write entirely when the result is unchanged. Write to a temp file and rename so a reader never sees a half-written file. Rewriting, rather than only skipping duplicate appends, also repairs a file that has already grown - which is the state every affected session is already in, and where a skip-if-unchanged check never fires. Fixes openai#528. openai#322 reported the same defect earlier.
![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: 22a79e972c
ℹ️ 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".
| // Write and rename, so that a reader never sees a half-written file. | ||
| const tmpFile = `${envFile}.${process.pid}.tmp`; | ||
| fs.writeFileSync(tmpFile, next, "utf8"); | ||
| fs.renameSync(tmpFile, envFile); |
There was a problem hiding this comment.
When another SessionStart hook writes to the same CLAUDE_ENV_FILE concurrently, this read-modify-rename sequence can silently discard that hook's exports: if it appends after this process reads the file but before this rename, its write lands on the old inode or is overwritten by the stale snapshot. Claude Code may run matching hooks concurrently, so the rewrite needs cross-process synchronization or another update strategy that cannot lose concurrent additions.
Useful? React with 👍 / 👎.
| fs.writeFileSync(tmpFile, next, "utf8"); | ||
| fs.renameSync(tmpFile, envFile); |
There was a problem hiding this comment.
When CLAUDE_ENV_FILE was created with restrictive permissions such as 0600, this temporary file is created with Node's default 0666 & umask mode (typically 0644) and then replaces the original. In a traversable directory, exports written by any hook—including credentials—therefore become readable by other local users after SessionStart; create the temporary file using the original file's mode before renaming it.
Useful? React with 👍 / 👎.
1 participant
Fixes #528. #322 reported the same defect earlier and was closed without the fix landing, so this adds regression tests as well.
The defect
handleSessionStartappends threeexportlines toCLAUDE_ENV_FILEevery time it runs, andSessionStartfires on startup, on resume, and on every compaction. Nothing prunes. Only the last assignment of a name has any effect, so every earlier copy is dead weight.Claude Code inlines the whole env file into the single
bash -c <script>argument it spawns. On Linux a single argv element is capped atMAX_ARG_STRLEN= 32 pages = 131072 bytes — a separate limit fromARG_MAX, andulimitdoesn't move it. Once the accumulated prelude crosses it,execvefails withE2BIGand no shell starts at all: every Bash tool call fails regardless of how short the command is.The env file involved is
~/.claude/session-env/<session-id>/sessionstart-hook-0.sh, one per session; the one that broke my session had reached 136770 bytes. On Windows the same growth truncates the command line at 8191 characters, which is how #528 was originally reported.The change
Rewrite this plugin's exports instead of appending them: drop lines that assign one of the three names this plugin owns, keep every other line verbatim, append the current values, and skip the write entirely when the result is byte-identical to what's already there. The write goes to a temp file and is renamed, so a reader never sees a half-written file.
Why a rewrite rather than a skip-if-duplicate append
This is the part that matters for anyone already affected. Both #558 and #386 keep the append and return early when the last
export <name>=line already matches. That stops future growth on a fresh file, but it never removes anything — and because the env file is per-session, the last assignment of each name in an already-bloated file is byte-identical to what the hook would write. So the early return fires, nothing is pruned, and a session that is already over the limit stays broken until the user deletes the file by hand.Measured by firing the real hook against a seeded env file (one unrelated co-tenant line, same
session_idevery time, since the file is keyed by session id in its own path):mainTrade-off worth knowing about
This is a read-modify-write on a file other plugins may also append to, so a sibling
SessionStarthook appending between the read and the rename would lose that write. The current append is atomic and has no such window. The exposure is small — the read-filter-write-rename is microseconds, it happens at most once per session-start event, and the skip-if-unchanged check means the steady state performs no write at all — but it is real, and it is presumably why the other two PRs stayed append-only.If that window is unacceptable, the two approaches can be combined: prune the plugin's own lines once, then keep the values in a plugin-owned file under
CLAUDE_PLUGIN_DATAand leave a single constant[ -f … ] && . …line in the shared file. Steady state then never writes to the shared file, and a changed value is picked up by rewriting the owned file, which nobody else touches. I did not do that here because it makes the shared file depend on being executed as shell rather than parsed, which is a stronger assumption about the host than the current plainexportlines need. Happy to switch if you'd prefer it.Tests
Four tests added to
tests/runtime.test.mjs, all of which fail onmainand pass with this change:The existing
session start hook exports the Claude session id, transcript path, and plugin data dirtest is unchanged and still passes — the output for a fresh env file is byte-identical to before.Local
npm test: 87 pass / 4 fail out of 91 onmain, and 91 pass / 4 fail out of 95 with this change — four new tests, four new passes, and the same four failures before and after (status shows phases…,status preserves adversarial review kind labels,result returns the stored output…,resolveStateDir uses a temp-backed per-workspace directory). Those four are unrelated to this change and look like they need the real Codex CLI, which CI installs and my machine doesn't have.