[matt-dz]

What does this PR do?

• Preserve leading and adjacent empty fields when unquoted expansions use non-whitespace IFS separators, including command arguments, for-loop items, and redirect targets.
• Split these fields locally before delegating quoting and globbing to the existing expansion library, retaining brace expansion and argument/byte limits without changing dependencies.
• Replace the existing bug-compatible expectation with Bash behavior and document the supported splitting semantics.

Motivation

An expansion such as IFS=: with x=':a::b:' silently lost empty arguments. A redirect through ':input.txt' consequently opened input.txt even though Bash rejects it as ambiguous. Both behaviors were reproduced on the checked-out code against Bash 5.2.21, confirming scan 4d79407d-0ae8-4b25-b965-96a1448afacb.

Testing

The reported argument and redirect regressions, plus the corrected consecutive-separator scenario, failed before the fix and pass afterward. Coverage also exercises mixed whitespace, quoted boundaries, Unicode separators, command substitution, globbing, brace expansion, and resource-limit boundaries.

Passed the full shell scenario suite, interpreter race tests (interp and interp/tests), interpreter symbol checks, all 49 field-splitting scenarios against local Bash 5.2.21, and 5,000 generated Bash comparison cases. Ran make fmt. Validation used Go 1.26.3 because the pinned 1.26.2 is unavailable.

Update (rebased onto main, now targets main instead of the scanner branch): the Docker Bash comparison suite (RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash) now passes. It first caught the IFS=é case: the Debian oracle runs in the POSIX locale and splits on each byte of é, while rshell always uses UTF-8, so that case moved to its own nonws_ifs_unicode_separator.yaml with skip_assert_against_bash (same precedent as the printf/wc Unicode scenarios). The \r\v\f case matches Debian's bash 5.2.15.

Checklist

• Tests added/updated
• Documentation updated (if applicable)

Correctness scan: 4d79407d-0ae8-4b25-b965-96a1448afacb

Finding fingerprint: f1686f31c79151a8229c90dcc383af5e192c60d3640e5747f2bef14303afa9ab

---

PR by Bits - View session in Datadog

Comment @datadog-staging to request changes

[datadog-staging-us1-crawler-test]

View session in Datadog

Bits Code status: ✅ Done

CI Auto-fix: Disabled | Enable

Comment @datadog-staging to request changes

[julesmcrt]

Reviewed 3b9b21ca.

Verdict: Approve. The fix works: across a 44-case matrix (this branch vs current main vs bash 3.2 and 5.3 oracles) the branch matches bash on every case that isn't an intentional rshell restriction, while current main still drops empty fields in 17 of them — including cases where main's dropping mis-executes rather than just mis-splits (IFS=,; v=',printf'; $v hi runs printf on main because the empty leading field vanishes and shifts the command name, where bash gives "command not found"; this branch fails closed with "command not allowed"). Current main is not correct anywhere this branch is wrong, so the PR is not superseded by the #695 rewrite. Empty fields count toward the 16 384-field expansion limit identically to main (20 000-comma probe errors with the same message on both), empty argv[0] can never match an allowlist entry since AllowedCommands rejects empty names at construction and matches exactly, and I found no new panic or OOM surface — unknown word parts error out rather than panicking, and 64 probe scripts produced no crashes. The package's own new tests pass, as does the full interp and scenario suite.

Findings (all P3):

1. interp/runner_fields.go:132 classifies \r, \v, \f as IFS whitespace (hardcoded " \t\n\r\v\f"). With IFS=$(printf '\r\v\f:') and x=\r:\v:\fa:\r, this branch produces <><><a> like bash 5.3, but POSIX defines IFS white space as space/tab/newline only, which makes the 7-field output (what bash 3.2 gives) the strictly correct answer — and sibling #668 implements that, so the two PRs are mutually incompatible on this input. Under the repo's bash-over-POSIX rule matching bash 5 is defensible; just be aware whichever of the two lands second needs to reconcile this (and its scenario coverage) with the other.

2. interp/runner_fields.go:127: strings.ContainsRune(ifs, r) has a utf8.RuneError special case, so any invalid-UTF-8 byte in IFS matches any invalid byte in an expanded value. With IFS=$(printf '\xff:') and x=$(printf 'a\xff\xffb:c'), this branch yields <a><><b><c> where bash 3.2 and 5.3 both yield <a\xff\xffb><c> (no split at \xff). Main already splits at invalid bytes (upstream FieldsSeq does the same), so the byte-splitting itself is pre-existing; this PR adds the empty field between adjacent invalid bytes. If you want to close it: only compare runes when both sides decode cleanly and handle DecodeRuneInString returning RuneError with size == 1 as an opaque byte.

3. analysis/symbols_internal.go conflicts with main (the only conflicting file per merge-tree) but is formatting-only — gofmt of the merge-base blob is byte-identical to gofmt of this head's blob (numstat 111/111). Suggested resolution: take main's blob and run make fmt. As always this area stays gated on the human verified/allowed_symbols label regardless of review outcome.

4. The tools/correctness-scan scaffolding and scanner docs duplicate what's carried on the #629 stack; leaving the authoritative scanner review to that PR is fine (its Codex-sandbox pinning issue is tracked there), just flagging the duplication so it doesn't get reviewed twice.

Minor positive notes worth keeping: fragments are re-expanded through synthetic parameters with IFS overridden empty, so variable values are never re-parsed as shell source — tilde, backslash, and glob characters arriving via variables all behave literally, matching bash (probed); and the double field-count check (in-split at runner_fields.go:78 plus the existing per-field check in runner_expand.go) keeps the #695 limit airtight.

[julesmcrt]

Reviewed 3b9b21ca.

Follow-up to my earlier approve on this PR: that verdict stands for the IFS field-splitting fix itself, but I missed blocking on one carried item. This branch also contains the correctness-scanner scaffolding, including commit bfa6899 "fix: use Codex automatic approval sandbox", which removes the Codex --sandbox workspace-write pin while keeping --approve-for-me in tools/correctness-scan/main.go (invokeCodex, now lines 434-435: exec --approve-for-me --ephemeral ... with no --sandbox). That is the same P1 detailed in the #629 review: an auto-approving, unsandboxed Codex child process run over scanned-repository content, whose containment then depends on unverified local Codex defaults.

Merging this PR as-is would land that copy, so I'm flipping to request-changes for exactly this: please rebase the scanner commits out of this PR (they belong to the scanner PR's stack and are reviewed there), or restore the sandbox pin in this branch first. With just the IFS fix remaining, this branch is an approve from me — see my earlier review for the full verification of that fix (44-case matrix vs bash 3.2/5.3, limit accounting, and the fail-closed empty-argv behavior).

[matt-dz]

@julesmcrt thanks, addressed (posted by Matthew's Claude Code agent):

• Scanner commits removed. Rebased onto current main and retargeted this PR from matthew.deguzman/correctness-scanner to main. It now contains only the IFS fix (28c7308) plus one test-only follow-up. 79044bb9, bfa6899b, 4a694921, d3141ecc, and 3734339e are no longer on this branch, so the unpinned --approve-for-me Codex call can't land through here; they're still on feat: add local correctness scanner #629's branch, where that P1 is tracked. The old head is backed up as backup/pr671-pre-rebase-3b9b21ca.
• analysis/symbols_internal.go conflict (your P3 [NETPATH-645] Extract filesystem sandboxing into pathSandbox #3) is gone: it came from the dropped fix: format symbols commit, and the rebase applied cleanly.
• New: 2bfd467. Running the Docker bash comparison locally, which the original PR couldn't do (HTTP 502), turned up a failure in nonws_ifs_delimiter_edges. Debian's bash runs in the POSIX locale and splits IFS=é on each byte, while rshell always uses UTF-8. I moved that case into its own nonws_ifs_unicode_separator.yaml with skip_assert_against_bash, following the printf/wc Unicode precedent. The \r\v\f case already matches Debian's bash 5.2.15 (<> <> <a>), so it stays bash-compared.
• Verification: TestShellScenariosAgainstBash passes. All field-splitting scenarios and the interp package pass. In the full go test ./..., the only failure is TestPSPentestLargePIDListDoesNotHang, which also fails on main on this host (its PIDs exceed the test's "missing" range) and isn't related to this PR.

Your P3s #1 (\r\v\f vs #668) and #2 (invalid-UTF-8 IFS bytes) are left as they were. Whether #671 or #668 lands is Matthew's call.

[julesmcrt]

Reviewed 2bfd4679c97c9cb07fb7d2e0db3705e56f1188eb.

The previous request (drop the carried correctness-scanner commits) is fully addressed. The branch is now retargeted to main and contains exactly two commits beyond it: the IFS fix (28c73087) and a test-only follow-up (2bfd4679). Verified:

• Scanner removal is complete. tools/correctness-scan, docs/CORRECTNESS_SCANNER.md, and docs/BEHAVIOR_CONTRACTS.md are absent from the tree, and no invocation of codex --approve-for-me (or any unpinned-sandbox variant) remains anywhere in the branch.
• The approved implementation is preserved exactly. interp/runner_fields.go is byte-identical to the version approved in the earlier review, and the branch no longer carries the formatting-only analysis/symbols_internal.go rewrite (it takes main's blob, per that review's note). The PR's footprint vs main is now just the fix, its tests, and the field-splitting scenarios.
• Behavior re-verified at the new head. The 44-case field-splitting matrix and the adversarial probe set (argv[0]-in-IFS fail-closed, glob-after-split, expansion-order, security bounds) all produce identical results to the approved tree; TestNonWhitespaceIFS, the full scenario suite, and -race all pass; go vet is clean; the branch merges into current main without conflicts.
• The test-only commit is a sound adjustment, not a weakening. It moves the IFS=é case into its own scenario with skip_assert_against_bash, because the Debian bash oracle runs in the POSIX locale and splits on each byte of a multibyte IFS character while rshell splits on the whole UTF-8 rune — consistent with the existing Unicode precedents for printf/wc. The rshell-side assertion is preserved, and its expected output (<>, <a>, <>, <b > — leading empty preserved, adjacent éé producing an empty field, whitespace retained inside b ) matches the empirically verified behavior from the earlier review.

Two small notes carried forward from that review: the P3 on invalid-UTF-8 IFS bytes matching any invalid byte (pre-existing strings.ContainsRune RuneError quirk, unchanged here), and the single behavioral divergence between this PR and #668 (\r/\v/\f as IFS whitespace — this PR follows bash 5's three-field reading, #668 follows the POSIX letter's seven-field one), which the second of the two PRs to merge will want to reconcile.

This is an approve — the IFS fix was already verified correct and this push removes everything that was blocking it.