Make the session-commands scenario pass under the hostile tmpdir axis - #726
Merged
Conversation
…container hostile+normal runs pass, shellcheck clean; test-only path-quoting defect) The shared/session-commands scenario roots its whole workspace tree under TMPDIR, so the tmpdir hostile-env axis (space, literal $HOME, backtick `id`) exposed two test-side path-quoting defects that only surface when the full scenario runs in the Linux validator container: - The monitor env-file assertions compared the raw workspace path against output the product writes with printf '%s=%q\n' (write_session_monitor_env_file), so they mismatched once %q escaped the hostile characters. Match the %q-quoted form with grep -qxF. - The docker-desktop monitor fixture hand-wrote its session-monitor.env with a raw-interpolated heredoc, then had the product source it. Emit the file with printf %q, mirroring the product's own format, so it is safe to source. The prior single-quoted printf was also nested inside a single-quoted bash -lc body, which collapsed the newlines. Verified in the validator container under the real hostile TMPDIR: the scenario now runs to completion (set -e, so completion proves no remaining break) and still passes under a normal TMPDIR. %q is a no-op on benign paths, so the normal axes are unchanged.
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 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.
Problem
shared/session-commandsfailed only on the tmpdir hostile-env axis (passed=16 failed=1). That axis pointsTMPDIRat a directory whose name carries a space, a literal$HOME, and a backtick`id`command substitution (scripts/ci/run-validate-in-validator.sh:~83-91), and the scenario roots its entire workspace tree underTMPDIR(TMP_DIR=$(mktemp -d "${TMPDIR:-/tmp}/...")). Two earlier piecemeal fixes (#724 clean filter, #725 diff textconv) did not reach the remaining breaks because the full scenario only executes inside the Linux validator container (macOS takes a different launch path), so they were never reproduced.Method — reproduced in-container
Built the validator image and ran only this scenario inside it under the real hostile
TMPDIR(uid 501,HOME/GOCACHE/GOMODCACHEmirroring the harness), iterating withbash -xuntil it ran to completion. Underset -e, running to completion is proof there is no remaining break.Hostile-path sites found and fixed (both test-side defects)
printf '%s=%q\n'(scripts/workcellwrite_session_monitor_env_file). The assertions grepped the raw workspace path, which mismatched once%qescaped the hostile characters. Now match the%q-quoted form withgrep -qxF.session-monitor.envwith a raw-interpolated heredoc and then had the productsourceit — the unquoted hostile path broke sourcing. Now emitted withprintf "%q", mirroring the product's own format, so it is safe to source. (The intermediate single-quotedprintf '…\n'was additionally nested inside a single-quotedbash -lcbody, which collapsed the newlines into literaln; double-quoted formats — the existing pattern in these blocks — avoid that.)Why spot-quoting, not re-rooting
Sibling scenarios and this one deliberately root
TMP_DIRunder${TMPDIR}and pass under the axis — exercising real code under hostile paths is the axis's purpose. Re-rooting would defeat it. Both breaks were genuine test bugs that now match the product's real contract, and%qis a no-op on benign paths, so the normal axes are unchanged.Verification (in-container)
TMPDIR: scenario runs to completion, exit 0 (was the sole failure before).TMPDIR: scenario exit 0 — no regression.bash -nclean;shellcheck(validator image) clean.run-validate-in-validator.shis unchanged (space +$+ backtick preserved).Why this is comprehensive
The fix was driven by an actual in-container run to completion rather than fixing the first error and guessing, so every hostile-path break the scenario can hit on this axis is covered in one pass.