fix(git): pin trusted executables across scan hosts - #467
Conversation
|
@codex review |
![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: 5dc0f1c298
ℹ️ 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".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9be47a60bb
ℹ️ 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".
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49000ab013
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…/codex/portfolio-pr-467-20260815
|
@codex review Please review the current head, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 743e32f028
ℹ️ 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".
| const git = await inspectTrustedExecutable( | ||
| "git", | ||
| pluginEnvironment, | ||
| protectedGitRoot, | ||
| ); |
There was a problem hiding this comment.
When the SDK environment sets CODEX_SECURITY_GIT="" to disable optional Git, this lookup ignores the binding, discovers Git from PATH, and later replaces the empty value with that executable. Codebase scans therefore use a Git snapshot instead of the requested directory fallback, while diff scans proceed rather than reporting Git as unavailable. Check the binding before lookup, including case-insensitive aliases on Windows, as is already done for ripgrep.
AGENTS.md reference: sdk/typescript/AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review Please review the current head, |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0142c5739
ℹ️ 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".
| let ripgrep = await inspectTrustedExecutable( | ||
| "rg", | ||
| git.environment, | ||
| protectedGitRoot, | ||
| ); |
There was a problem hiding this comment.
On Windows, when PATH contains an rg.exe symlink targeting an rg.cmd or rg.bat file and no later trusted ripgrep exists, inspectTrustedExecutable treats the .exe invocation as runnable and this call therefore skips the bundled-ripgrep fallback. The Python resolver subsequently rejects the canonical batch target in _is_native_executable, causing inventory generation—and thus the scan—to fail. Reject canonical batch targets during host inspection while continuing to allow extensionless native targets.
AGENTS.md reference: sdk/typescript/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62bfcd13d4
ℹ️ 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".
| `, | ||
| join(PLUGIN_ROOT, "scripts"), | ||
| ], | ||
| { encoding: "utf8", env: childEnvironment(dirname(python!)) }, |
There was a problem hiding this comment.
When Bun.which returns a pyenv/asdf-style shim implemented with #!/usr/bin/env bash, replacing PATH with only dirname(python) prevents /usr/bin/env from locating bash, so every runPythonMocks case exits with status 127 before Python starts. Start the interpreter with a PATH capable of executing the shim and narrow os.environ["PATH"] inside Python, or resolve the underlying interpreter first, so these tests remain deterministic across developer installations.
AGENTS.md reference: sdk/typescript/AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Draft feedback: preserve explicitly configured trusted executable selections.
| modelProvider, | ||
| ); | ||
| const protectedGitRoot = await outermostGitMarkerRoot(repo, signal); | ||
| const git = await inspectTrustedExecutable( |
There was a problem hiding this comment.
[P2] Honor nonempty explicit Git and ripgrep executable bindings
Could we validate and preserve nonempty CODEX_SECURITY_GIT and CODEX_SECURITY_RG selections before searching PATH? The workbench supports explicit trusted absolute executable paths, but this code always resolves literal git and rg, then overwrites the configured bindings. A valid custom Git outside PATH is silently disabled or replaced, and an explicitly selected ripgrep can be replaced by the bundled fallback. The existing empty-disable behavior should stay unchanged.
2 participants
Summary
Use one trusted Git executable consistently across SDK scans, the bundled workbench, and MCP hosts.
Fixes #129. Extends the still-open proposal in #140.
Changes
PATHis omitted without broadening an explicitly empty or rejected path.CODEX_SECURITY_GITbinding while still sanitizingPATHfor ripgrep. Emit one effective binding on Windows and retain case-distinct POSIX variables.PATHaliases consistently.0.1.22so cached hosts refresh the helper changes.Testing
Current head
b83d18e5:bun --no-env-file test --timeout 30000 tests-ts/workbench-tool-environment.test.ts— nine passed, zero failures, and 17 Bun assertions with Bun 1.3.14. The existing Python mock cases and assertions are unchanged; no interpreter shim or executable fixture was added.pnpm run types,pnpm run format, andgit diff --check— passed on the committed head.bun --no-env-file test --randomize --seed 12345 --timeout 30000 ./tests-ts— 1,317 passed, 11 skipped, zero failures, and 9,287 assertions with Bun 1.3.14. Live integration and machine-policy opt-ins were disabled.pnpm run build,pnpm pack,pnpm run check:package, and the separatepnpm run test:packageon the resultingopenai-codex-security-0.1.14.tgz— passed. The package validator checked 218 archive entries and 106 bundled-plugin files. Both installed checks ran the actual nested worker with its original timeout. The archive is byte-identical to the preceding published-head archive.The follow-up changes only the test helper; production source and bundled-plugin files are unchanged. The source head and tree stayed clean through every current-head local gate. Remote CI has not yet run on this new head.
Historical published head
62bfcd13:bun --no-env-file test --randomize --seed 12345 --timeout 30000 ./tests-ts— 1,317 passed, 11 skipped, zero failures, and 9,287 assertions with Bun 1.3.14. Live integration and machine-policy opt-ins were disabled.pnpm run build,pnpm pack,pnpm run check:package, and the separatepnpm run test:package— passed. The package validator checked 218 archive entries and 106 bundled-plugin files. Both installed checks ran the actual nested worker with its original timeout.The earlier
743e32f0CI attempt exposed a preexisting parallel-test assertion race corrected inc0142c57and an unlocalized Windows credential-lock owner-fileEPERM. The laterc0142c57and62bfcd13matrices passed. Production locking, ownership, and ACL behavior remain unchanged, and no old-head retry was run. Earlier source-head records are preserved; no historical result is claimed for the current head.Risk and rollout
Git remains optional, safe user settings remain available, and the existing snapshot and scan-completion contracts are unchanged. The packaged-ripgrep fallback uses the installed application's native-package provenance and existing runtime cleanup; it does not relax the Python repository exclusion or add a persistent executable cache. Explicit disable bindings remain effective on POSIX and Windows. The missing-
PATHbehavior follows the documented Node command-lookup defaults.Integrate #449 first and retain its streamed Git command support, private-spool pipe handling, distinct cancellation and writable-state error handling, committed-object immutability, and offline-read policy. Preserve its shared-runner routing and immutable blob-read flags. Reconcile the shared inventory and containment changes in #320 and #472 when they land. This branch uses paired plugin version
0.1.22; the recorded #449, #462, and #472 heads carried competing0.1.21updates. Before merging, update both version fields together to the next monotonic version after the preceding plugin changes, then rerun the integration checks on the resulting head.Public disclosure review
Newly authored material was reviewed for public disclosure. Existing automated review comments contain access-restricted report references; maintainer cleanup remains outstanding.