fix(bash,tests): real #315 multi-input bug + test infra path/ESM bugs - #555
Merged
Conversation
## scripts/capture_bash.sh, scripts/validate_bash.sh — REAL execution-verified bug invoke_entry()'s argument-passing pipeline writes args via `jq -r`, then reads them back via `mapfile -t ARGS < "$args_file"`. jq.exe (Windows build) emits CRLF line endings on -r text output (same root cause as the confirmed jq-stack bug, #519/#529), and `mapfile -t` only strips the trailing `\n`, leaving every argument contaminated with a trailing `\r` (e.g. "Hello, World!" becomes "Hello, World!\r"). This silently defeats exact string comparisons in user code -- discovered via tests/bash-stack.test.js's #315 multi-input regression test, which mutates a function to special-case input #2 ("Hello, World!") and expects validate to catch it. It didn't: the live arg never matched the literal string the mutation checked for, so the mutated branch was never taken, and validate silently PASSed a real behavioral regression. Initial fix attempt (strip \r from each array element via `${ARGS[i]%$'\r'}` after mapfile) reproducibly does NOT work when nested inside this file's `output=$( { ... } )` command-substitution structure -- confirmed via isolated minimal repro that the exact same suffix-removal pattern works at top level and inside a plain `( )` subshell, but silently no-ops here. Root cause of THAT specific bash behavior is not fully understood; worked around it by stripping \r from the file via `tr -d '\r'` (through process substitution) BEFORE mapfile reads it, which does work reliably. ## tests/bash-stack.test.js — two test-infrastructure bugs (Windows-only) 1. REPO_ROOT/SCRIPTS (computed via path.resolve()/join()) get embedded unquoted into bash -c command strings throughout this file. On native Windows these are backslash-separated paths, and bash's own command- string parser treats every unquoted '\X' as an escaped literal X -- silently dropping every backslash ("C:\Users\user\foo" becomes "C:Usersuserfoo"), which then fails to resolve as a file path at all (status 127, "No such file or directory"). Fixed by normalizing both to forward slashes once at the top (valid path separator on Windows too) -- fixes every call site at once. 2. `await import(join(SCRIPTS, 'validate.js'))` passes a raw absolute path string to dynamic import(). Node's ESM loader on Windows requires a file:// URL for absolute-path imports, not a raw "c:/..." string (ERR_UNSUPPORTED_ESM_URL_SCHEME). Fixed with pathToFileURL(). ## Verification - Full manual repro outside the test suite: fresh capture -> 2/2 PASS -> mutate slugify() to special-case input #2 only -> validate -> exactly 1/2 FAIL ("bash-slugify: INPUTS[1] hash mismatch", bash-greet still PASS) -> restore -> 2/2 PASS again. - `node --test tests/bash-stack.test.js`: 13/13 pass, 0 fail (was 13 failures before this fix, all stemming from the two test-infra bugs cascading into every test in the file). These three bugs were found while investigating why a from-scratch `npm test` run on Windows showed 91 failures despite this session's direct, isolated manual verification of the Bash stack passing cleanly. Cross-checked against GitHub Actions CI (Linux, green on main) to confirm the bulk of those 91 failures are Windows-test-runner-specific artifacts (SIGINT/SIGTERM semantics, this exact path-escaping class of bug) rather than product regressions -- but this particular one (#315 multi-input silently not caught) is a genuine product bug that happened to only surface because the test assertion exercises a multi-input scenario that this session's manual spot-checks did not happen to construct.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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.
scripts/capture_bash.sh, scripts/validate_bash.sh — REAL execution-verified bug
invoke_entry()'s argument-passing pipeline writes args viajq -r, then reads them back viamapfile -t ARGS < "$args_file".jq.exe(Windows build) emits CRLF line endings on-rtext output (same root cause as the confirmed jq-stack bug, #519/#529), andmapfile -tonly strips the trailing\n, leaving every argument contaminated with a trailing\r.This silently defeats exact string comparisons in user code — discovered via
tests/bash-stack.test.js's #315 multi-input regression test, which mutates a function to special-case input #2 and expects validate to catch it. It didn't: the live arg never matched the literal string the mutation checked for, so validate silently PASSed a real behavioral regression.Fixed by stripping
\rfrom the file viatr -d '\r'(through process substitution) beforemapfilereads it — a post-hoc${ARGS[i]%$'\r'}strip aftermapfilereproducibly does NOT take effect when nested inside this file'soutput=$( { ... } )command-substitution structure (confirmed via isolated minimal repro; root cause of that specific bash behavior not fully understood, worked around instead).tests/bash-stack.test.js — two test-infrastructure bugs (Windows-only)
REPO_ROOT/SCRIPTSget embedded unquoted intobash -ccommand strings. On native Windows these are backslash-separated paths, and bash's own command-string parser treats every unquoted\Xas an escaped literalX— silently dropping every backslash. Fixed by normalizing both to forward slashes once at the top.await import(join(SCRIPTS, 'validate.js'))passes a raw absolute path string to dynamicimport(). Node's ESM loader on Windows requires afile://URL. Fixed withpathToFileURL().Verification
INPUTS[1] hash mismatch) → restore → 2/2 PASS again.node --test tests/bash-stack.test.js: 13/13 pass, 0 fail (was 13 failures before this fix).Cross-checked against GitHub Actions CI (Linux, green on main) to confirm the bulk of unrelated Windows-only
npm testfailures seen this session are test-runner artifacts (SIGINT/SIGTERM semantics) rather than product regressions — this one is a genuine product bug.