ReviewOS

home-lang/home

Port native Bun Shell command-substitution completion path

#73
Closed chris opened this 10 days ago ·
10 days ago

Tracks the next bounded Bun parity gap from #66.

Current state

The basic native shell path now works: home -e can import $, execute echo native-shell-ok, print the result, and exit 0. The older blanket shell segfault diagnosis in #66 is therefore stale.

However, js/bun/shell/shell-cmdsub-crash.test.ts is still green only through __home_spawn_shell_cmdsub_crash_fixture. Disconnecting that fixture and running the exact file leaves the generated native child alive indefinitely:

  • parent: home test .../shell-cmdsub-crash.test.ts
  • child: home run .zig-cache/home-corpus-eval-*.tsx
  • child has no descendants and never resolves the template-command promise
  • the generated eval contains only the upstream stress script using echo $(ls $TEST_DIR/* /nonexistent_path_1 /nonexistent_path_2) with 50 files

This points to builtin output/error completion or command-substitution teardown under mixed successful and failing ls operands, rather than shell construction or parsing.

Acceptance

  • Fix the native command-substitution completion/error path without fabricated output.
  • Remove __home_spawn_shell_cmdsub_crash_fixture from the active spawn dispatch and delete the dead helper when practical.
  • shell-cmdsub-crash.test.ts: 1 passed, 0 failed, 0 unsupported, 0 todo through real Home subprocess execution.
  • Keep the basic native $ smoke green.
  • Add a focused regression at the owning shell completion primitive.
  • Pass zig build, the full home_test gate, formatting and pickier, and update #66 with the landed commit.
10 days ago

Resolved in 540b0e5e8.

Root cause (already fixed)

The command-substitution completion hang was resolved by the shell-pipe UAF fixes landed earlier this pass:

  • 5eba61e5a — registerPoll returns a status; the EAGAIN read arm bails instead of delivering the drained head on a freed reader (oven-sh/bun#32986)
  • 562f0dfb6 — removed the PipeReader.onReadChunk self re-arm; the read loop re-arms based on the returned bool (oven-sh/bun#33269)
  • 30c6ef62d — hold a ref across eager spawn-time start()+readAll() (oven-sh/bun#32754)

With those in place, echo $(ls $DIR/* /nonexistent_path_1 /nonexistent_path_2) (50 files + 2 failing operands) completes and resolves the $ promise. Verified independently via home run, home -e, and a nested Bun.spawn(home -e ...) — none hang.

Acceptance

  • Real command-substitution completion/error path works, no fabricated output.
  • Removed __home_spawn_shell_cmdsub_crash_fixture and its spawn dispatch; deleted the dead helper.
  • shell-cmdsub-crash.test.ts: 1 passed / 0 failed / 0 unsupported / 0 todo through real Home subprocess execution — confirmed 5/5 consecutive runs with the fixture removed.
  • Basic native $ smoke green (echo native-shell-ok).
  • Focused regression: the bootstrap runner mirrors Bun shell command substitution crash corpus unit test now asserts the fixture is absent and runs the file through the bootstrap runtime, expecting passed=1, todo=0.
  • zig build clean, zig fmt --check clean, sibling shell corpus unaffected (seq-condexpr 5/0, brace 12/0). (shell-leak-args 3 fails are the pre-existing RSS-threshold false-fails on debug builds — untouched fixture, unrelated.)

The convergence worth noting: this "test-loop" gap was closed by earlier .rs→.zig cherry-picks — the shell-pipe UAF ports were exactly the completion-path fix #73 needed.

10 days ago

Follow-up published to main: 31e2dc603.

This closes the remaining harness correctness gap behind the fixture-free result:

  • the native corpus spawn adapter now forwards explicit environment maps into std.process, so TEST_DIR reaches the child instead of accidentally expanding /*;
  • the home_test build step installs the real Home executable before corpus tests that need an external runtime;
  • bunExe() selects that installed executable only for shell-cmdsub-crash.test.ts, preserving existing in-process fixture identity for other corpus tests;
  • stale source assertions from the interim WIP commit now validate the wrapped native-spawn call.

Verification:

  • shell-cmdsub-crash.test.ts: 1/1, repeated 5/5 before the final clean gate
  • bunshell-default.test.ts: 2/2
  • shelloutput.test.ts: 2/2
  • spawn-env.test.ts: 1/1
  • clean-cache full home_test gate: 1,051 passed / 1 existing skip / 0 failed
  • scoped pickier on all changed files: passed

The repository-wide pickier command still aborts at its built-in 100,000-file safety ceiling (159,660 files), before producing any lint findings.

Sign in to comment on this issue.