Skip to content

Remove shell expansion for git commands - #447

Merged
dkundel-openai merged 2 commits into
mainfrom
bryane/git-remove-shell-expansion
Jul 8, 2026
Merged

dkundel-openai merged 2 commits into
mainfrom
bryane/git-remove-shell-expansion

Conversation

@bryane-oai

Copy link
Copy Markdown
Contributor

Summary

Ensure Git commands are executed directly on Windows instead of being routed through a shell.

Git reference names can contain characters that Windows shells interpret specially. Passing those references through shell: true can therefore change the meaning of an otherwise valid Git command. This change keeps Git arguments as a literal argv array by forcing shell: false in the shared Git helpers.

Historical context

Windows npm installations expose tools such as npm and codex through .cmd shims, which Node cannot launch directly in some configurations.

  • #13 enabled shell: true for the shared synchronous command runner so Windows could resolve those shims.
  • #55 added equivalent handling for the Codex app-server process, along with hidden-window and process-tree cleanup behavior.
  • #178 made Windows launches respect SHELL, preserving compatibility with Git Bash.

Those changes fixed important Windows behavior, but the shared command runner also handles Git. Unlike npm and codex, Git is directly executable on Windows and does not need shell-based .cmd resolution.

Changes

  • Allow callers of runCommand to override its shell setting.
  • Force shell: false in both Git command helpers.
  • Apply the Git override after caller-provided options so it cannot be re-enabled accidentally.
  • Add regression coverage for an automatically detected default branch containing shell-special characters.
  • Update the test command helper to support an explicit shell setting when constructing the fixture.

The regression test verifies that the branch name is passed to Git literally and that an adjacent command helper is not executed.

Compatibility

The existing Windows behavior remains unchanged for commands that require it:

  • npm and Codex .cmd discovery continues to use the configured Windows shell.
  • Codex app-server startup continues to respect SHELL for Git Bash users.
  • Existing Windows process-tree cleanup and hidden-window behavior are unaffected.
  • macOS and Linux behavior is unchanged.

This approach avoids restricting valid Git reference names and confines the change to the command path that does not require shell expansion.

Validation

  • npm test: 91 tests passed
  • Focused Git tests: 11 tests passed
  • git diff --check: clean

@bryane-oai
bryane-oai requested a review from a team July 7, 2026 23:18
@dkundel-openai
dkundel-openai merged commit db52e28 into main Jul 8, 2026
1 check passed
HerbertGao added a commit to HerbertGao/codex-plugin-cc that referenced this pull request Jul 8, 2026
- v1.0.6 (PR openai#447): git commands no longer pass through a shell (shell: false)
- Resolved version conflicts to 1.0.6-herbertgao.1 (fork suffix convention)
- Preserved fork's codex-rescue foreground-forcing customization (untouched by v1.0.6)

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
DKalien added a commit to DKalien/codex-plugin-cc that referenced this pull request Jul 8, 2026
- Merge upstream security fix: remove shell expansion for git commands (openai#447)
- Add options.shell override support in runCommand
- Update version to 1.0.6.1
- Keep local Node.js v25 compatibility improvements
Jafee added a commit to Jafee/codex-plugin-cc-stable that referenced this pull request Jul 8, 2026
Port of openai/codex-plugin-cc v1.0.6 (net diff 80c31f9..db52e28, PR openai#447).
On win32 runCommand defaulted to shell: process.env.SHELL || true, so
git()/gitChecked() ran repository-derived arguments (baseRef, branch
names, paths) through cmd.exe or the user's shell — a malicious repo
could inject commands via a crafted default-branch name. git commands
now force shell: false (after the options spread, so callers cannot
override it back); runCommand and the test helper honor an explicit
options.shell via ?? while keeping the old platform default otherwise.
Regression test: a branch named main&branch-helper&x must reach git
literally and never execute the repo-local branch-helper.cmd.

Fork notes: tests/helpers.mjs merged with the fork's timeout option;
upstream version hunks replaced by the fork's own versioning. This
supersedes HOLD item openai#294 (same root cause, fixed at the source).

Dual review: Claude (Fable 5) APPROVE + Codex (gpt-5.5 xhigh, fresh
read-only, pinned-SHA static audit) APPROVE. Clean-env suite 124/124.

Co-Authored-By: bryane-oai <[email protected]>
Co-Authored-By: Claude Fable 5 <[email protected]>
Crazytieguy added a commit to Crazytieguy/codex-plugin-cc that referenced this pull request Jul 9, 2026
…ture

Adopted:
- Remove shell expansion for git commands (upstream openai#447): git spawns now
  pass shell: false via an options.shell passthrough in lib/process.mjs
  and tests/helpers.mjs, composed with our resolveWindowsShell() on win32.
  Includes upstream's regression test for branch names with shell
  metacharacters.
- app-server.mjs: append captured stderr to "app-server exited
  unexpectedly" errors.

Rejected:
- /codex:transfer Claude-session-to-Codex-thread feature (upstream openai#374):
  transferring Claude-generated assistant turns into a Codex thread puts
  Codex out of distribution; also couples to Codex's private import ledger.
  All transfer code, command, protocol types, fixtures, and tests dropped.
- Upstream version bumps and identity churn (package.json, marketplace,
  plugin.json, README) per fork policy.

Post-merge fixes:
- Dropped upstream's requestAttestation: false capability — the checked-in
  generated InitializeCapabilities type doesn't define it, so it broke the
  tsc build (found by Codex review).

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01LE4prC9U5Snku24TXbUfYr
zknpr added a commit to zknpr/codex-plugin-cc that referenced this pull request Jul 16, 2026
Brings the branch onto v1.0.6: session transfer (openai#374), 1.0.5/1.0.6 bumps,
and no-shell-expansion for git commands (openai#447). Conflicts resolved in
codex-companion.mjs (workspace-boundary notice + transfer dispatch coexist)
and lib/app-server.mjs (child exit deregisters the host-exit reaper AND
carries captured stderr in protocol errors). Full suite green: 124/124.

Co-Authored-By: Claude Fable 5 <[email protected]>
robinmordasiewicz added a commit to f5-sales-demo/codex-plugin-cc that referenced this pull request Jul 24, 2026
* Fix working-tree review crash on untracked directories (openai#166)

* fix: skip untracked directories in review context

* fix: skip broken untracked symlinks in reviews

* fix: respect SHELL on Windows for Git Bash (openai#178)

* Use app-server auth status for Codex readiness (openai#177)

* Use app-server auth status for Codex readiness

* fix: reuse existing app server for auth checks

* fix: inherit process.env in app-server spawn when no explicit env is provided (openai#159)

`SpawnedCodexAppServerClient.initialize()` passes `this.options.env` to
`spawn()`, but no caller ever sets `env` in options. In Node.js, passing
`undefined` for `env` gives the child process **no** environment variables,
breaking any model provider that relies on env vars (e.g. DATABRICKS_TOKEN).

Fall back to `process.env` when `this.options.env` is not set, matching the
existing pattern in `broker-lifecycle.mjs` and `codex-companion.mjs`.

Co-authored-by: Isaac

Co-authored-by: Bhuvanesh Sridharan <[email protected]>

* fix: gracefully handle unsupported thread/name/set on older Codex CLI (openai#126)

* fix: gracefully handle unsupported thread/name/set on older Codex CLI

Codex CLI v0.118.0 does not recognize the thread/name/set JSON-RPC method,
causing startThread() to throw. Thread naming is cosmetic (for job log
labels) and should not block thread creation. Wraps the call in try/catch
so it fails silently on older CLI versions.

Fixes openai#119

* refactor: only suppress unsupported-method errors for thread/name/set

Address Codex review feedback: the bare catch swallowed all errors
including auth, network, and server failures. Now only suppresses
errors containing 'unknown variant' or 'unknown method' (the specific
error older CLI versions return) and rethrows everything else.

* codex: scope implicit resume-last selection to the current Claude session (openai#83)

Co-authored-by: VOIDXAI <[email protected]>

* codex: scope default cancel selection to the current Claude session (openai#84)

Co-authored-by: VOIDXAI <[email protected]>

* fix: avoid embedding large adversarial review diffs (openai#179)

* fix: avoid embedding large adversarial review diffs

* fix: preserve untracked content in lightweight review

* address comments

* fix: handle ENOBUFS type check in git diff sizing

* bump: update plugin version to 1.0.3 (openai#180)

* codex: honor --cwd when reporting session runtime (openai#35)

* codex: honor --cwd when reporting session runtime

* codex: keep session runtime lookup scoped to callers

---------

Co-authored-by: VOIDXAI <[email protected]>

* fix: declare model in codex-rescue agent frontmatter (openai#169)

* fix: declare model in codex-rescue agent frontmatter

The codex-rescue agent had no model field, leaving Claude Code to assign
whatever default it chooses. As a thin forwarding wrapper that issues a
single Bash call, this agent is well-suited to the haiku tier; declaring
it explicitly ensures a predictable cost profile and tier guarantee.

Co-Authored-By: Claude Code <[email protected]>

* Update codex-rescue.md

---------

Co-authored-by: claude[bot] <[email protected]>
Co-authored-by: Claude Code <[email protected]>
Co-authored-by: Dominik Kundel <[email protected]>

* fix: correct invalid 'xhigh' reasoning effort in README (openai#99)

Closes openai#77

Updated README to replace unsupported 'xhigh' value with 'high' to prevent configuration errors.

* fix: quote \$ARGUMENTS in cancel, result, and status commands (openai#168)

Unquoted \$ARGUMENTS in the ! shell commands allowed shell metacharacters
in user-supplied job IDs to be expanded before Node received them (e.g.,
`task-123; malicious-cmd` would execute the trailing command). This is
inconsistent with review.md and adversarial-review.md, which both wrap
"$ARGUMENTS" in double quotes.

Co-authored-by: claude[bot] <[email protected]>
Co-authored-by: Claude Code <[email protected]>

* fix: route /codex:rescue through the Agent tool to stop Skill recursion (openai#234) (openai#235)

* fix: route /codex:rescue through the Agent tool to stop Skill recursion (openai#234)

`/codex:rescue` previously combined two things that together caused a hang:

- `context: fork` in the frontmatter, which spawns a `general-purpose`
  subagent for the command body.
- Body prose "Route this request to the `codex:codex-rescue` subagent."
  without naming the transport.

When the main agent called `Skill(codex:rescue)` programmatically, the
fork resolved the ambiguous prose by trying `Skill(codex:codex-rescue)`
(unknown skill) and then falling back to `Skill(codex:rescue)`, which
re-entered this command and hung the session until the user cancelled.
No Codex job was ever created.

Naming the transport as `Agent(codex:codex-rescue)` alone is not enough:
forked general-purpose subagents do not expose the `Agent` tool, so the
forked runner cannot reach the subagent that way either. The minimal fix
is therefore two coordinated changes:

- Drop `context: fork` so the command body runs inline in the calling
  agent's context, where `Agent` is in scope.
- Say explicitly "use the `Agent` tool with `subagent_type:
  "codex:codex-rescue"`", and call out that `Skill(codex:codex-rescue)`
  and `Skill(codex:rescue)` are not valid routing paths. Add `Agent`
  to `allowed-tools` so the call does not prompt for permission.

Everything else in rescue.md (resume-candidate check, flag handling,
background/foreground semantics, operating rules) is unchanged. The
`codex:codex-rescue` subagent itself is unchanged.

Tests pin the new allow-list, the explicit `subagent_type`, the ban on
`Skill(codex:codex-rescue)`, and the absence of `context: fork`. The
existing "run the `codex:codex-rescue` subagent in the background"
assertion continues to hold since that sentence still reads correctly
with the Agent-tool transport.

Fixes openai#234

* test: match quoted result and cancel command arguments

---------

Co-authored-by: Dominik Kundel <[email protected]>

* fix: bump plugin version to 1.0.4 (openai#244)

* feat: add Claude session transfer command (openai#374)

* fix: bump plugin version to 1.0.5 (openai#398)

* Remove shell expansion for git commands (openai#447)

* Remove shell expansion for git commands

* Version bump

---------

Co-authored-by: Dominik Kundel <[email protected]>
Co-authored-by: Bhuvanesh Sridharan <[email protected]>
Co-authored-by: Bhuvanesh Sridharan <[email protected]>
Co-authored-by: Trevin Chow <[email protected]>
Co-authored-by: VOIDXAI <[email protected]>
Co-authored-by: VOIDXAI <[email protected]>
Co-authored-by: xiaolai <[email protected]>
Co-authored-by: claude[bot] <[email protected]>
Co-authored-by: Claude Code <[email protected]>
Co-authored-by: Manav Agarwal <[email protected]>
Co-authored-by: Friende <[email protected]>
Co-authored-by: stefanstokic-oai <[email protected]>
Co-authored-by: bryane-oai <[email protected]>
raghavagrawal-x added a commit to raghavagrawal-x/grok-build-plugin-cc that referenced this pull request Aug 4, 2026
Match openai/codex-plugin-cc#447: keep Windows shell defaults in
runCommand for .cmd shim resolution, but force shell:false in git
helpers (applied last) so repository-derived refs never go through
cmd.exe. Add integration coverage for a default branch containing &
and a tracked branch-helper.cmd marker.
MikeTV added a commit to MikeTV/codex-plugin-cc that referenced this pull request Oct 1, 2026
…l-expanded

Cherry-picked from upstream openai/codex-plugin-cc db52e28 (openai#447). On
Windows, runCommand sent git through a shell, so a crafted file or branch
name in a reviewed repo could be read as shell syntax. Upstream's version
bump to 1.0.6 is left out because the fork keeps its own version line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants