fix(paths): shell-quote gstack-paths output so eval round-trips values - #2376
Open
fangearhq-boop wants to merge 1 commit into
Open
fix(paths): shell-quote gstack-paths output so eval round-trips values#2376fangearhq-boop wants to merge 1 commit into
fangearhq-boop wants to merge 1 commit into
Conversation
`bin/gstack-paths` is consumed as `eval "$(gstack-paths)"`, but emitted bare `KEY=VALUE` lines with an unquoted RHS. eval re-parses those, so backslashes become escape sequences and spaces become word separators. On Windows this breaks every skill. $TMP is a backslash path, so: TMP=C:\Users\me\AppData\Local\Temp eval "$(gstack-paths)" echo "$TMP_ROOT" # C:UsersmeAppDataLocalTemp The following `mktemp "$TMP_ROOT/codex-err-XXXXXX.txt"` fails, TMPERR is empty, and `2>"$TMPERR"` is a redirect to an empty filename — so the bash block exits 1 before Codex ever runs and the user sees no review and no obvious cause. A value containing a space (C:\Program Files\Temp) is worse: eval word-splits it, prints `FilesTemp: command not found`, and leaves the variable empty. This is the same user-visible cascade as garrytan#2091 / garrytan#2370, which are filed as macOS/BSD-mktemp bugs. The root cause here is different, so those fixes don't help — a Windows user finds them, applies them, and is still broken. Not Windows-only: any POSIX TMPDIR containing a space or backslash corrupts the same way. Fix: emit with `printf %q` so eval reproduces values byte-for-byte. The script is already `#!/usr/bin/env bash`, so %q is available. Plain POSIX paths are unchanged by %q, so existing callers and tests see identical output. The header's existing "callers should quote expansions" note did not cover this: the corruption happens during eval, before the caller has a variable to quote. Reworded to say what the emitter now guarantees. Tests: three eval-round-trip cases (backslash, space, embedded quote) added to test/gstack-paths.test.ts. They fail against the old `echo` and pass with %q. The backslash case is skipped on win32 — MSYS rewrites backslashes to forward slashes in env values, so it cannot be injected through the environment on a Git Bash runner; the escape-eating it guards is pure eval semantics and is exercised on Linux/macOS CI. Same reasoning as the existing HOME-unset skips in that file. Fixes garrytan#2374 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
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
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.
Fixes #2374.
The bug
bin/gstack-pathsis consumed aseval "$(gstack-paths)"(the documented contract, usedat Step 0.6 of every skill), but emits bare
KEY=VALUElines with an unquoted right-handside.
evalre-parses those: backslashes become escape sequences, spaces become wordseparators.
On Windows
$TMPis always a backslash path, so this fires every time:The
mktemp "$TMP_ROOT/codex-err-XXXXXX.txt"that follows then fails,TMPERRis empty,and
2>"$TMPERR"is a redirect to an empty filename, so the bash block exits 1 beforeCodex ever runs:
A value containing a space is worse —
evalword-splits it, printsFilesTemp: command not found, and leaves the variable empty, somktemptargets thefilesystem root and fails with
Permission denied.This is the same user-visible cascade as #2091 and #2370, both filed as macOS/BSD-
mktempbugs. The root cause here is different, so those fixes don't help: a Windows user searching
the symptom finds them, applies them, and is still broken.
Not Windows-only — any POSIX
TMPDIRcontaining a space or a backslash corrupts the same way.The fix
Emit with
printf %qsoevalreproduces values byte-for-byte:The script is already
#!/usr/bin/env bash, so%qis available. Plain POSIX paths areunchanged by
%q, so existing callers and the existing assertions see identical output.I also reworded the header comment. The old note said callers should quote expansions,
which doesn't cover this failure — the corruption happens during
eval, before thecaller has a variable to quote. Callers here already quote correctly and were still broken.
Tests
Three
eval-round-trip cases added totest/gstack-paths.test.ts: backslash, space, andembedded single quote. They assert the full documented calling convention rather than the
raw stdout, which is what the existing tests check.
Verified they are real regression tests — against the old
echothey fail, with%qthey pass:echoprintf %qtest/gstack-paths.test.tstest/gstack-paths.test.ts+test/spec-template-invariants.test.ts(2 rather than 3 failures on the old code because the backslash case is skipped on this
Windows runner — see below. On Linux CI all three fail without the fix.)
Test-environment note worth flagging for review: the backslash case is skipped on
win32.MSYS/Git Bash rewrites both
C:\...→/c/...and backslashes → forward slashes in envvalues, so a literal backslash cannot be injected through the environment on a Git Bash
runner. What it guards is pure
evalsemantics, so exercising it on Linux/macOS CI issufficient. This follows the same reasoning as the existing
HOME-unset skips in thatfile. The space and quote cases do run on Windows, and both catch the bug there.
The three test files that reference
gstack-pathsorTMP_ROOTaretest/gstack-paths.test.ts,test/spec-template-invariants.test.ts, andtest/skill-e2e-plan-tune-cathedral.test.ts(evals tier). The first two pass in full.No templates are touched, so generated
SKILL.mdoutput is unaffected.Alternative considered
Keeping
echoand changing the contract toset -a; . <(gstack-paths)also fixes it andpreserves the plain human-readable output documented on line 4. I didn't take it because
it changes Step 0.6 in every skill template plus each host variant, where this is three
lines in one file. Happy to switch if you'd rather have the unescaped display form.
Found while dogfooding
/codex reviewon Windows: with a literalTMP_ROOT=/tmpsubstituted before the
mktemp,/codex reviewworks normally on 1.60.1.0, which isolatesthis as the only blocker on that path.