Skip to content

Upstream the fork's fixes to reubeno/brush: what, and in what order #18

Description

@cataggar

Ten commits sit on k that are not upstream. All of them fix real defects found by running an 88k-line production bash script (kenji-cli) and its 98-suite BATS corpus under brush. This tracks getting them into reubeno/brush and in what order.

Verified against reubeno/brush@3a1de7ad (2026-08-21). k is 8 commits behind upstream at time of writing.

Before sending anything

Strip the (#N) suffixes from commit subjects. They are this fork's PR numbers, and they collide with unrelated real PRs upstream:

our commit says reubeno/brush#N actually is
heredoc deadlock (#2) Bump prettydiff 0.6.4 → 0.7.0
procsub deadlock (#5) Fix deny error
real-time signals (#15) Implement more builtins
Windows quoting (#17) Bump serde 1.0.201 → 1.0.202

GitHub renders these as links to those closed PRs. Left as-is, every PR opens with a confusing wrong reference.

Also rebase onto current main first, and re-run the suite per PR — applying cleanly is not the same as passing.

Status against current upstream

Every bug below is still live upstream; none was independently fixed.

  • setup_open_file_with_contents still does writer.write_all(bytes) before any reader exists
  • there is no brush-core/src/sys/windows/commands.rs
  • no real-time signal support in sys/unix/signal.rs

Cherry-pick results onto 3a1de7ad: all ten apply, with conflicts only where a commit depends on an earlier one in its own chain (#2→#3, #13→#14→#15). Those chains apply cleanly in order. No conflicts with upstream drift.

Every functional commit carries compat tests with a bash oracle.

Order

Wave 1 — docs

docs(core): clarify process substitution task detachment 10 lines
docs(core): document builtin worker invariant 6 lines

Zero risk, and they show the code was read closely rather than patched blindly. Cheap first contact that makes the later reviews easier.

Wave 2 — deadlocks (highest value)

  1. fix(core): avoid deadlock on here-documents larger than the pipe buffer
    A hang, not a wrong answer. The entire here-document is written into a pipe before any reader exists, so the write only completes while the payload fits in the buffer. Bisected to an exact threshold: fine at 4096 bytes, hangs at 4160. Linux masks it via F_SETPIPE_SZ; Windows keeps the 4 KiB default. Also repairs two latent Linux gaps — the ?-propagated fcntl fails outright for payloads above /proc/sys/fs/pipe-max-size and for zero-length payloads.

  2. fix(parser): honor backslash-newline line continuations in here-documents
    Must follow (1) — same here.yaml. heredoc_escape_sequence handled \$, \` and \\ but not \<newline>, so continuations were emitted verbatim instead of joining lines.

  3. fix(builtins): avoid deadlock when a builtin reads a process substitution
    Independent of the two above. Another hang.

Deadlocks first because they are severe, self-contained, and each ships a bash-oracle test. Easiest "yes" a maintainer can give.

Wave 3 — signals, strictly in order

  1. fix: report signals ignored at shell entry
  2. fix: preserve ignored signals at shell entry
  3. fix: support Linux real-time signals — 319 lines; send only after the first two land

Wave 4 — Windows argument quoting

fix(core): quote arguments passed to external commands on Windows

Highest review burden: ~300 lines of new platform code a maintainer may not be able to exercise locally. Kept last for that reason, not because it matters least — the bug is severe and silent. Arguments containing braces or escaped parens were corrupted en route to an external process, so grep -oE '[0-9]{3}' matched 23 instead of 123 and still exited 0. Wrong answers with a success status are worse than crashes.

Not upstreaming as a PR

chore: check out all text files with LF via .gitattributes — the only commit here that is not a bug fix. Adding a .gitattributes changes every contributor's working copy, which is a maintainer policy decision rather than a patch to drop on them. Raised as an issue upstream instead so they can decide. Stays on k regardless.

Caveat

The verification above confirms these apply to current upstream and that the bugs are still present. Each PR still needs a build and full-suite run after rebasing.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions