Repository navigation
fix: make runCommand maxBuffer explicit - #747
sylvesterkaczmarek wants to merge 3 commits into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Upstream PR openai#747 (sylvesterkaczmarek), two conflicts resolved. runCommand() passed options.maxBuffer straight through, so an unset value left Node's 1 MiB spawnSync default in place and a large `git diff` came back truncated with ENOBUFS. It now defaults to an explicit 256 MiB, and an explicit override still wins. Conflicts: both are adjacency, not disagreement. In process.mjs the fork had added `timeout`/`killSignal` on the lines around `maxBuffer`; the resolved call keeps all three. In tests/process.test.mjs the PR's import line is dropped (the fork's import block already names runCommand and terminateProcessTree) and its two tests are appended. Verified: node --check on both files; tests/process.test.mjs 13/13. 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
|
Following up after the Group 6 maintenance pass. This branch is current, mergeable, and has no failing checks or unresolved review threads. Could a maintainer review it when convenient? |
912e98d to
1776e37
Compare
|
Refreshed validation for #747, #749, #808 and #809, with additional large Unicode stdout/stderr coverage here. The four changes merge cleanly in a local checkout and pass all 109 tests, the version check and the TypeScript build. The unchanged upstream checkout passes 91 tests and its build. Could a maintainer approve the relevant fork workflows when reviewing? Thanks. |
![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: d2f85dc219
ℹ️ 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".
Summary
Thanks for the detailed report in #744. This makes the intended buffering behaviour explicit instead of relying on an
undefinedoption overriding Node's 1 MiBspawnSyncdefault.runCommandmaxBuffervalues used by measurement gatesValidation
npm test(93 passed)node --test tests/process.test.mjs(4 passed)npm run check-versiongit diff --checkFixes #744
Maintenance validation on 7 November 2026
Fresh validation of exact head
26e52abon macOS arm64 with Node 26.8.1:npm testpassed all 95 tests,node --test tests/process.test.mjspassed all 6 focused tests (including large Unicode stdout/stderr), andnpm run check-versionandgit diff --checkpassed. The previous 93-test count is historical.The four PRs merged without conflicts in a local checkout. All 109 tests passed, together with the version check and TypeScript build. The unchanged upstream checkout passed 91 tests and its build. Runtime tests used fake app-server fixtures and isolated test state, without live model requests or account credentials. Native Windows execution and a live Claude Code session were not tested. Hosted CI has not completed successfully for these fork heads; maintainer approval or reruns remain outstanding.
Validation used Python 3.12/3.14 and Node 26 on macOS arm64 as applicable. Earlier validation counts above are historical. Individual and combined counts overlap and should not be added.