fix: a command whose redirect fails should not run at all - #60
Merged
Conversation
davydog187
force-pushed
the
fix/preflight-redirect-open-59
branch
from
July 31, 2026 14:23
977ca25 to
744047b
Compare
davydog187
force-pushed
the
fix/preflight-redirect-open-59
branch
from
August 3, 2026 18:17
744047b to
c48dcbc
Compare
ivarvong
approved these changes
Aug 3, 2026
Closes #59. bash opens every redirect target before it forks the command, so a target it cannot open means the command never runs. JustBash ran the command and applied redirections to the result afterwards, so the command's side effects survived a failed redirect: b = JustBash.new(files: %{"/m/j" => "f\n"}) # /m/j is a regular file {r, _} = JustBash.exec(b, "mkdir /made > /m/j/x; [ -d /made ] && echo MADE") r.stdout #=> "MADE\n" (bash: "absent") #56 fixed the observable-output half; this is the body half. ## The shape `Redirection.preflight/2` expands, resolves, and *opens* every redirection target left to right, before the body. It returns the opened redirections for `apply_redirections/3` to consume, or the shell's own error result for the first target that would not open — in which case the caller returns it without executing the body. `Executor.with_redirections/3` wraps all six call sites that can carry a redirection (simple commands, `for`, `while`, `until`, subshells, groups), so each of them skips its body on a failed open. The compound cases open once for the whole body rather than per iteration, which is what bash does and what matches the existing behavior of applying the redirection once to the accumulated output. Because the targets are now resolved up front and threaded through to the write, `apply_redirections/3` takes preflighted `{type, path}` pairs instead of AST nodes, and a target containing a command substitution is expanded exactly once. `redirect_failed/4` stays for failures the open cannot predict, because they depend on what the command produced — a write past `Limit`'s file-size cap. ## Behaviors that changed All verified against GNU bash 3.2.57: | script (`/m/j` is a regular file) | now | |---|---| | `mkdir /made > /m/j/x` | exit 1, no `/made` | | `touch /t > /m/j/x` | exit 1, no `/t` | | `for i in 1 2 3; do mkdir /L$i; done > /m/j/x` | exit 1, no `/L*` | | `( mkdir /S ) > /m/j/x`, `{ mkdir /G ; } > /m/j/x` | exit 1, nothing made | | `cd /elsewhere > /m/j/x` | exit 1, cwd unchanged | Opening the target is also what truncates it, so two more cases move toward bash: `cat f > f` now leaves `f` empty (`O_TRUNC` happens before the body reads it), and `true >> f` leaves an existing `f`'s mtime alone (`O_APPEND` with nothing to append is not a write). With several redirections, the ones left of a failure are still created or truncated and the ones to its right are never expanded, so `echo hi > /bad > $(gen)` does not run `gen`. UPGRADING.md item 7 gains the body half; item 8 states the truncate-at-open and expand-once changes. ## Tests `test/just_bash/redirect_preflight_test.exs`: 30 tests — side effects suppressed for each of the six call sites and for functions and state-mutating builtins, open-time truncate/append semantics, targets either side of a failing one, a command substitution proven to run exactly once via a custom command that returns a new path per call, and regression guards for the redirections that already worked. 18 of the 30 fail without this change.
davydog187
force-pushed
the
fix/preflight-redirect-open-59
branch
from
August 3, 2026 20:20
c48dcbc to
4730320
Compare
This was referenced Aug 3, 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
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.
Closes #59. Rebased onto
mainnow that #56 has merged — this is a single commit againstmain.Root cause
bash opens every redirect target before it forks the command, so a target it cannot open means the command never runs. JustBash ran the command and applied redirections to the result afterwards, so the command's side effects survived a failed redirect:
Real bash:
#56 fixed the observable-output half —
$?,&&/||, stdout suppression, and truncate-on-open already matched. This is the body half: the only remaining divergence was that the command executed.The shape
Redirection.preflight/2expands, resolves, and opens every redirection target, left to right, before the body. It returns either the opened redirections forapply_redirections/3to consume, or the shell's own error result for the first target that would not open — in which case the caller returns it without executing the body.Opening follows the
open(2)flags bash uses:>,2>,&>create or truncate;>>,&>>create only if missing. Redirections that touch no file (/dev/null,>&,<) are classified and passed through.Executor.with_redirections/3wraps all six call sites that can carry a redirection —do_execute_simple_command/6,AST.For,AST.While,AST.Until,AST.Subshell,AST.Group— so each skips its body on a failed open. The compound cases open once for the whole body rather than per iteration, which is what bash does and what already matched the existing behavior of applying the redirection once to the accumulated output:Because targets are now resolved up front and threaded through to the write,
apply_redirections/3takes preflighted{type, resolved_path}pairs instead of AST nodes, so a target containing a command substitution is expanded exactly once.redirect_failed/4stays for failures the open cannot predict, because they depend on what the command produced — a write pastLimit's file-size cap.The subshell case keeps discarding the subshell's own state, but now runs the body against the preflighted outer shell, since opening the target creates or truncates it.
Behaviors that changed
All verified against GNU bash 3.2.57. With
/m/ja regular file, every one of these was exit 1 with the side effect applied and is now exit 1 with no side effect:mkdir /made > /m/j/x/madetouch /t > /m/j/x/tmkdir /made 2> /m/j/x,>> …,&> …/mademkdir -p /d && mkdir /made > /d(:eisdir)/madef() { mkdir /made; }; f > /m/j/x/madecd /elsewhere > /m/j/xfor i in 1 2 3; do mkdir /L$i; done > /m/j/x/L*while/untilequivalents( mkdir /S ) > /m/j/x,{ mkdir /G ; } > /m/j/xOpening the target is also what truncates it, which moves two more cases toward bash:
cat f > fleavesfempty.O_TRUNChappens before the body reads the file; previouslyfwas rewritten with its own contents.true >> fleaves an existingf's mtime alone.O_APPENDwith nothing to append is not a write, so the fourapplywrite paths now no-op on empty content instead of rewriting the file.And ordering across several redirections now matches bash: the ones left of a failure are still created or truncated, and the ones to its right are never expanded —
echo hi > /bad > $(gen)does not rungen.UPGRADING.md item 7 gains the body half; a new item 8 states the truncate-at-open, expand-once, and ordering changes.
Tests
New
test/just_bash/redirect_preflight_test.exs, 30 tests:cd);>>mtime case;/dev/null,2>&1, heredocs,<, truncate-on-failure).18 of the 30 fail on the parent commit (confirmed by stashing the
lib/changes); the other 12 are the deliberate no-regression guards, which pass on both sides.Full suite: 3855 tests / 56 properties, 0 failures — also green with
--include bash_comparison.mix compile --warnings-as-errors,mix credo --strict(only the known intentional test-fixture finding),mix format --check-formatted, andmix dialyzerall clean. The one "Unnecessary Skip" dialyzer reports pre-exists on the parent commit.Out of scope
<also blocks the command in bash (cat < nosuchfile→ exit 1,catnever runs); JustBash still substitutes empty stdin. A command whose redirect fails should not run at all #59 scoped preflight to non-stdin redirections, soextract_heredoc_stdin/2is untouched.$(...)discards filesystem writes in JustBash, so a command substitution cannot be counted via a file it writes — which is why the expand-once test uses an Agent-backed custom command. Unrelated to this issue; happy to file it separately.echo hi > a > bwritesaand emptiesb; bash emptiesaand writesb(last wins for the same fd). Pre-existing, not made worse by preflight.