Make the session-commands scenario filter survive a hostile TMPDIR - #724
Merged
Conversation
…roduced and eliminated the exact CI filter-127 error locally; fixes the scenario's own fixture, the target of this change) The shared/session-commands scenario builds a real git clean filter fixture: it writes filter.sh under a workspace rooted at TMPDIR and registers it as `clean = <absolute-path>` in a git include file. Git runs external filter commands through `sh -c`, so any shell metacharacter in that unquoted absolute path is reinterpreted by the shell rather than treated as a literal path. The tmpdir hostile-env axis composes TMPDIR with a space, a literal $HOME, a backtick command substitution, and padding. Under that axis the unquoted filter path splits on the space, expands $HOME, and runs the backtick as a command substitution, so the shell can no longer locate filter.sh: `error: external filter '...' failed 127`. This is not a noexec issue: the validator container that runs this scenario does not mount /tmp noexec (only workcell's own spawned session containers do, via container-smoke.sh's `--tmpfs /tmp:...noexec`), and a byte-for-byte local reproduction of the scenario's own git commands under a matching hostile TMPDIR produces the identical `failed 127` text purely from shell word-splitting and command substitution, with no noexec mount involved. Fix: single-quote the filter path in the `clean =` config line so the value git hands to `sh -c` is treated as one literal argument. The same class of bug existed one line up: filter.sh's own body embedded the marker path inside a double-quoted `touch "..."`, which still lets `sh` expand $HOME and the backtick when filter.sh runs; single-quoting that argument closes it too. Neither change alters what the scenario exercises: the filter is still a real external git clean filter, and the existing assertions that `session diff` must not trigger it are unchanged. Verified locally: reproduced the CI-reported `external filter ... failed 127` message character-for-character by extracting and running the scenario's exact git setup under a matching hostile TMPDIR, then confirmed the quoted form both invokes filter.sh successfully and produces no filter or marker errors. Ran bash -n and shellcheck clean on the touched file. Not verified locally: the full scenario's overall exit code inside the actual validator container + Colima, since this host takes a different (real launch) code path past this fixture than the CI validator's non-macOS "launch not supported" branch; that divergence is unrelated to this fix and reproducible with or without it.
Codex Review SummaryThis 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. |
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
This was referenced Sep 9, 2026
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
1 participant
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.
Summary
The advisory hostile-env CI lane's tmpdir axis fails
shared/session-commandswith:Root cause: the scenario registers a real git external clean filter as
clean = <absolute path to filter.sh>in a git include file, with the path built fromTMPDIR. Git runs external filter commands throughsh -c, so any shell metacharacter in an unquoted path is reinterpreted by the shell instead of being treated as a literal filename. The tmpdir hostile-env axis composesTMPDIRwith a space, a literal$HOME, a backtick command substitution, and padding (scripts/ci/run-validate-in-validator.sh:~83-91). Against that path, the unquoted filter command word-splits on the space, expands$HOME, and runs the backtick as a command substitution — so the shell can no longer findfilter.sh, producing the observedfailed 127.This is not a noexec problem: the validator container that runs this scenario does not mount
/tmpnoexec (only workcell's own spawned session containers do that, viacontainer-smoke.sh's--tmpfs /tmp:...noexec). A local, byte-for-byte reproduction of the scenario's own git setup under a matching hostileTMPDIRreproduces the identicalfailed 127text purely from shell word-splitting/command-substitution, with no noexec mount involved.Fix
Single-quote the filter path in the
clean =config line, so the value git hands tosh -cis one literal argument immune to$/backtick/space reinterpretation. The same class of bug existed one line above:filter.sh's own body embedded the marker path inside a double-quotedtouch "...", which still letsshexpand$HOME/backticks whenfilter.shitself runs; single-quoting that argument closes it too.The scenario's intent is unchanged: it still exercises a real external git clean filter, and the existing assertions that
workcell session diffmust not trigger that filter are untouched.Verification
error: external filter '...' failed 127message character-for-character by extracting and running the scenario's exact git setup (git init/git config .../filter.sh/git add) under a hostileTMPDIRmatching the CI axis composition (space,$HOME, backtick, padding).filter.shsuccessfully with zero filter/touch errors under the same hostile path, both in isolation and via the real fixture lines (1-353) extracted verbatim from the scenario file, before and after the fix.bash -nandshellcheckclean on the touched file.Test plan
shared/session-commandsas PASSbash -nandshellcheckclean locallyscripts/check-pr-shape.shpasses