fix(shell): decode UTF-8 in the read builtin (Fixes #6543)

[DS-argus]

What

Fixes #6543. The Bash tool's read builtin turned each input byte into a char, so every non-ASCII byte became a separate Latin-1 character (café → café), and read -n 1 split a multibyte character.

Following the triage on #6543, this ports the oh-my-pi fix into crates/brush-builtins-vendored. It does not keep upstream's block read-ahead.

From oh-my-pi:

• f3ce813d: read and mapfile read through a shared LineInput (new line_input.rs). Each line is UTF-8 decoded once, and invalid bytes become U+FFFD.
• 68fb9863: read -n/-N count characters, not bytes.
• b6e8ee12: mapfile into a readonly array fails before reading anything.

Not kept from oh-my-pi:

• The block read-ahead from f3ce813d, which seeks unread bytes back to a regular file.
• The -C give-back from b636b6ae, which only existed for that read-ahead.

Review showed that read-ahead could lose pipe input when a mapfile assignment fails, and could rewind a file offset that another reader had already advanced. So read and mapfile read one byte at a time, as before this PR, and never seek.

Changes beyond the upstream code

• Readiness and cancellation. read polls a stream before every byte instead of trusting a FIONREAD count. It keeps the local cancellation, -t timeout, Interrupted, and -t 0 readiness guards (D-BUILTINS-READ-CANCEL).
• Counting malformed UTF-8. -n/-N count characters the way the line decodes them:
  • Each invalid sequence that becomes one U+FFFD counts as one character.
  • An incomplete character at EOF also counts as one.
• Held input. To know a lead byte is invalid, read -n/-N sometimes reads one byte past its limit. That byte is held instead of being kept in the value, together with the backslash before it if it was escaped. The next builtin read or mapfile of the same input gets it first.
  • The store is a new openfiles::HeldInput inside brush-core's OpenFile. Every try_clone shares it, so it follows the open file description, not the fd number. It is recorded as the keep-local row D-BRUSH-CORE-HELD-INPUT.
  • As with bash's zungetc pushback, an external command that reads next does not see a held byte.
• Headers and inventory. The vendored headers name 68fb9863b377af566d3b56aa882a964654b5760a and describe these local changes. The A-PI-BUILTINS-BASE inventory row records the per-row bump.
• Test helper. Upstream's tests use an execute_captured helper and tempfile, which pi-shell didn't have. I added the helper in the same style as the existing tests (it takes PROCESS_TEST_LOCK), and tempfile as a dev-dependency.

Differences from bash

• An invalid byte becomes U+FFFD; bash keeps the raw byte.
• Valid UTF-8 is stored unchanged, as in bash.

Not yet in this PR

• Windows ACP for native-command pipelines. On a CP1252 system, a native producer's E9 used to come out of read as é and now becomes U+FFFD. I'm fixing this in a follow-up commit on this PR.

Why

Without this fix, a common while IFS= read -r line; do ...; done < file loop garbles any non-ASCII text that passes through it. That includes Korean, accented Latin, and emoji. The only workarounds are mapfile or pty: true.

Testing

All on macOS arm64 with the pinned nightly-2026-08-12, at c74a87a.

• cargo test -p pi-shell --lib: 245 passed. This PR adds 20 of them.

  • 7 tests are ported from upstream.
  • 13 tests cover the review findings.
  • Before changing the code in c74a87a, I confirmed that its tests for review findings fail on the previous head, 257dd83.

• brush-builtins unit tests, run standalone with the brush-core path patch: 43 passed.

  • These include the existing test_read_line_returns_interrupted_when_cancelled_before_input and test_ensure_not_cancelled_returns_interrupted.
  • They also include new tests:
    • -t 0 reports held input as ready without consuming it.
    • Cancellation leaves held input in place.
    • A read times out instead of blocking when another reader takes the held byte first.

• brush-core unit tests: 51 passed.

• cargo check -p pi-shell --lib --target x86_64-pc-windows-msvc passes. This is a compile check only.

• bun run check:rs passes: the rust scope check, cargo fmt --all -- --check, and cargo clippy --workspace -- -D warnings.

• bun scripts/changelog-fragments.ts check and bun scripts/verify-rust-porting-inventory.ts pass.

• Issue repro. "Before" is the native executeShell binding of an installed gjc 0.18.8. "After" is the same binding built from this branch with bun run build:native:

  case	before	after
  IFS= read -r line < file with café (hex)	636166c383c2a9	636166c3a9
  read -r line <<< "$(cat file)"	636166c383c2a9	636166c3a9
  ls | while IFS= read -r f finds é.txt	missing	found
  printf 'éx' | { read -r -n 1 a; read -r b; }	a=c383, b=c2a978	a=c3a9, b=78
  printf '한국어 원래\n' | read -r a b	double-encoded	한국어|원래
  mapfile -t -C cb -c 1 where cb reads the same pipe	cb:0:1:2 cb:1:3:4 arr=1 3	same

• Malformed input through the same binding (read -r <opt> x; read -r y on a pipe):

  input	opt	x	y
  C3 41 42 0A	-n 1 or -N 1	�	AB
  E2 82 41 0A	-n 1	�	A
  61 E2 + EOF	-N 2	a�, rc=0 as in bash	—

• Timeout and readiness still work as before:

  • read -t 1 on an idle pipe returns 142 after about a second.
  • executeShell({ timeoutMs: 1000 }) on sleep 30 | read x returns timedOut after about 1.0s.
  • read -t 0 returns 0 on a ready pipe or file and 1 on an empty pipe, as in bash.

• Speed is the same as before this PR. Median of 5 runs over 5000 lines of 80 bytes:

  case	before	after
  pipe while read	812ms	830ms
  file while read	448ms	461ms
  pipe mapfile	161ms	165ms

• bun check:

  • Not clean on this dev base, for reasons unrelated to this change. check:tools fails check-unsafe-rmrf on packages/coding-agent/test/runtime-mcp/mcp-project-host-values.test.ts (from fix(mcp): keep project config from running host shell or copying secrets #6561). This branch doesn't touch that file.
  • Every other check:ts step passes. check:sdk-closure needs the locally rebuilt addon's digest in diagnostic-artifact.json to embed it. That local edit is not committed.

• Not tested locally: Windows and Linux.

AI disclosure

GJC (Claude) ported the upstream commits, adapted the tests and dependencies, reworked the reading model after review, and ran the checks above. I reviewed the diff against upstream and the before/after output.

Approval

Merges to dev require one approving GitHub review from a write-access maintainer on the current head. Agent reviews (architect/critic) are advisory comments.

---

• Target branch is dev
• bun check passes (check:rs passes; check:ts fails only on the pre-existing check-unsafe-rmrf finding above)
• Tested locally
• Changelog fragment added under packages/<pkg>/changelog.d/ (if user-facing)
• Human approval or the required agent/owner verdict matches the exact PR head, not an earlier commit
• Risk classification above matches the actual review path taken

[probepark]

Code review (CI still running on 82be8ea; approval decision follows once CI finishes).

Review (head 82be8ea, gajae-reviewer on behalf of probepark)

ocr: blocking 0 / nit 1

CI: No checks reported. gajae-approve-gate.py 6572 --head 82be8ea41a5411e3ada9f744129edf66c6008c10 returned HOLD_CI (exit 10) during this review: Affected path validation has not reported yet. Approval is held; the body is unchanged.
Scope: +421 / -101, 10 files — vendored shell builtins, pi-shell regression tests, Cargo dependencies, porting inventory, changelog fragment.
Conventions: User-facing changelog fragment present; released changelogs and generated artifacts unchanged; no labels.

Notable:

• crates/brush-builtins-vendored/src/read.rs:532-610 collects bytes before UTF-8 decoding. CharCount at lines 494-510 waits for continuation bytes before satisfying valid UTF-8 character limits.
• crates/brush-builtins-vendored/src/line_input.rs:52-60,98-114 limits early-stop read-ahead to regular file handles and returns unread bytes through their shared offset. mapfile.rs:132-134,169-173 disables pipe read-ahead for callbacks and returns file read-ahead before invoking them.
• crates/brush-builtins-vendored/src/mapfile.rs:63-69 rejects readonly arrays before obtaining or consuming input.
• crates/brush-builtins-vendored/src/read.rs:445-460,662-669 retains cancellation checks, stream deadline checks, and the non-consuming -t 0 path.

Blocking: No blocking candidates found in the code review.

Note: crates/pi-shell/src/shell.rs:3686-3696 adds a -n UTF-8 regression. Add a matching -N regression with newline data and a three- or four-byte character to cover the other public option.

Issue #6543: The changed path now carries input bytes through LineInput → the line byte buffer → decode_line → variable assignment. The added regressions check UTF-8 output and shared-descriptor handoff. Existing expectations were not weakened.

Checked and clean: valid UTF-8 decoding/counting, callback handoff, readonly preflight, dependency placement, cancellation paths, release-note placement.
Could not assess: exact-head CI/build/test execution; no CI results are reported. The PR reports macOS tests and native-binding repros, but I did not execute them. Linux and Windows runtime behavior remains unverified. Run the required exact-head CI checks; approval will follow only after the approval gate permits it.

[snowykr]

Verdict

CHANGES_REQUESTED

Summary

This PR adds shared UTF-8 line input and read-ahead for shell read/mapfile, addressing valid UTF-8 corruption. Static review found three reachable correctness regressions in character counting for malformed UTF-8, refill-error rollback, and cached pipe readiness. A low-priority concurrent shared-file-offset issue is noted separately. No code was executed for this review.

Findings / Required Changes

1. [P2] Count malformed UTF-8 consistently with decoded characters — crates/brush-builtins-vendored/src/read.rs:495-510

   • For input bytes C0 41 0A, read -r -n 1 x treats C0 as a two-byte lead, consumes the following A while waiting for a continuation, and then returns the lossy-decoded value �A. The one-character limit is exceeded and A is removed from subsequent input.
   • Relative to base, which also decoded bytes incorrectly but stopped after the first byte for -n 1, the new count heuristic introduces the over-consumption. The new decoder explicitly replaces malformed sequences with U+FFFD, and the added shell contract says -n counts UTF-8 characters. Count malformed bytes consistently with the decoder and preserve a following valid byte when it does not belong to the replacement character.

2. [P2] Preserve the actual buffer length when a refill fails — crates/brush-builtins-vendored/src/line_input.rs:81-84,99-103

   • The refill resizes the buffer before the fallible read. If a regular-file read returns a short block (for example, 3000 bytes), those bytes are consumed, and the next refill errors, the buffer remains length 4096. Drop then interprets the 1096 bytes of unused capacity as unread input and seeks back from offset 3000 to 1904, exposing already-consumed data again to the next reader.
   • Base used byte-at-a-time reads and did not rewind on drop, so it could not synthesize unread bytes after an I/O error. Keep buffer state limited to bytes actually read (and handle refill errors without a compensating seek for unused capacity).

3. [P2] Do not let cached pipe readiness authorize a blocking read — crates/brush-builtins-vendored/src/read.rs:451-459

   • With a pipe shared by concurrent readers, FIONREAD can report two bytes and cache readiness; this reader consumes the first byte, then another reader drains the second while the writer remains open. The next iteration trusts the stale count, skips wait_for_input, and blocks in the descriptor read. read -t and cancellation are then not observed until a later write or EOF wakes that syscall.
   • Base polled before each one-byte read, so it would re-enter the timeout/cancellation-aware wait after the competing reader drained the pipe (although the immediate poll/read race already existed). Do not use a cached byte count to permit a potentially blocking read; retain timeout/cancellation responsiveness when another consumer can drain the stream.

Non-blocking Observations

• [P3, non-blocking] crates/brush-builtins-vendored/src/line_input.rs:99-103 seeks backward to return unread bytes on regular files, while cloned File handles share the underlying cursor. If a concurrent child/background reader consumes the same redirected descriptor after this builtin reads ahead but before drop, the compensating seek can rewind over that reader's progress and make bytes available again. This requires concurrent reads on one shared open-file description, which are otherwise unordered, so it is not a merge blocker; avoid seek-back where cursor ownership is not exclusive if this concurrency is meant to be supported.

CI / Verification

The added shell integration tests cover the main valid-UTF-8, character-count, read-ahead, callback, and input-preservation cases. The exact-head Dev CI suite is action_required and awaiting maintainer approval; no check runs/status results are available for this SHA, so the PR's reported local checks are not independently corroborated here. No tests were run as part of this review.

Axis Coverage

Axis	Verdict	Coverage
A1 — Intent / Policy / Contract	APPROVED	Issue #6543's UTF-8/counting intent, porting policy, cancellation/timeout constraints, and changelog requirements checked.
A2 — Architecture / Correctness / Failure	CHANGES_REQUESTED	Findings 1–3 identify reachable regressions in counting and input state/timeout handling.
A3 — Security / Privacy / Trust	APPROVED	Input remains shell data; no new trust-boundary crossing or executable-input path found.
A4 — Verification / Tests / CI	APPROVED	Core integration cases are present; no exact-head CI result is available, as noted above.
A5 — Context / Compatibility / Platform	APPROVED	Builtin registration, descriptor consumers, platform guards, and reuse were traced; no API/persisted-format issue found.

Limitations

The reviewed head is 82be8ea41a5411e3ada9f744129edf66c6008c10. GitHub Actions is awaiting maintainer approval, leaving CI outcomes unavailable; no platform test results were independently available.

[probepark]

Code review (CI completed on 82be8ea; approval is held because the required affected-path validation was not observed).

Review (head 82be8ea, gajae-reviewer on behalf of probepark)

ocr: blocking 0 / nit 1

CI: Approval held. gajae-approve-gate.py 6572 --head 82be8ea41a5411e3ada9f744129edf66c6008c10 returned NEED_LOCAL (exit 11): Affected path validation never reported on this head. No checks are pending, but the required affected-path evidence is missing.
Scope: +421 / -101, 10 files — vendored shell builtins, pi-shell regression tests, Cargo dependencies, porting inventory, changelog fragment.
Conventions: User-facing changelog fragment present; released changelogs and generated artifacts unchanged; no labels.

Notable:

• No new blocking findings; the prior exact-head code review found the UTF-8 read path, cancellation/deadline handling, readonly preflight, and descriptor handoff clean.
• crates/pi-shell/src/shell.rs:3686-3696 adds a -n UTF-8 regression; a matching -N regression with newline data and a three- or four-byte character remains a nit.

Blocking: None from code review. Approval is not issued because the exact-head CI plan lacks affected-path validation.

Action required: Re-run Dev CI for this exact head, or run the package checks required by the CI plan locally, then re-request review. Do not change the body verdict line while the approval gate is NEED_LOCAL.

[Yeachan-Heo]

Thanks for the fix! My lane reviewed 82be8ea (base dev 355f4e2) and found no blocking issues. It checked that read -n counts characters, not bytes, at UTF-8 boundaries, that invalid bytes decode to U+FFFD, that seek-back happens only on regular files (pipes and terminals aren't buffered past the line), and that mapfile hands back unread input before running its callback. It found no unsafe code. Two nits, both documented: a multibyte -d delimiter uses only its first byte, and overlong UTF-8 lead bytes are accepted and then replaced when decoding.
I approved the first-time-contributor CI runs; it still needs a maintainer's approval.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[probepark]

Approval remains held on 82be8ea41a5411e3ada9f744129edf66c6008c10.

The owner's reply confirms CI authorization, not completed exact-head checks. The approval gate now returns HOLD_CI. Native build, Windows regression, and native-addon checks are pending; the affected-path aggregate has not reported.

The peer change request also remains unresolved. The reply does not address its refill-error rollback or cached pipe-readiness findings. It does not show a regression result for malformed UTF-8 character limits.

1. Let the exact-head CI checks finish.
2. Address the three peer findings with fixes or specific counter-evidence.
3. Re-request review after these conditions are met.

The existing code reviews remain advisory. No approval or body verdict change was made.

[Yeachan-Heo]

Correction: my earlier 'no blocking issues' was wrong. snowykr's three P2s on 82be8ea all hold and are what needs fixing:

1. read -n over-consumes after a malformed lead byte (C0 41 0A).
2. The buffer keeps its full 4096-byte size after a failed refill, so drop seeks back over bytes that were never read.
3. A cached FIONREAD count lets a read block past -t or cancellation.
   Once you push fixes I'll re-check.
   —
   [repo owner's gaebal-gajae (clawdbot) 🦞]

[probepark]

revise: insufficient evidence to change the verdict on 82be8ea.

The correction confirms that snowykr's three P2 findings remain unresolved:

• malformed UTF-8 character counting can over-consume the following byte (crates/brush-builtins-vendored/src/read.rs:495-510);
• a failed refill can leave the buffer length larger than the bytes actually read and seek back over consumed data (crates/brush-builtins-vendored/src/line_input.rs:81-84,99-103);
• cached pipe readiness can allow a blocking read after another reader drains the pipe (crates/brush-builtins-vendored/src/read.rs:451-459).

Post fixes or exact-head counter-evidence for all three findings, then re-request review. No approval or body verdict change was made.

[snowykr]

Verdict

CHANGES_REQUESTED

Summary

This PR adds UTF-8-aware line input for the vendored Bash read and mapfile builtins, including regular-file read-ahead and unread-byte restoration. The exact-head review found three reachable regressions: malformed UTF-8 breaks read -n character limits, cached pipe readiness can bypass timeout/cancellation, and Windows ACP bytes from native commands are corrupted when piped to read.

Findings / Required Changes

1. [P2] Count malformed UTF-8 using replacement-decoding semantics — crates/brush-builtins-vendored/src/read.rs:495-503

   • CharCount infers UTF-8 width from lead-byte ranges without validating whether the sequence is legal. With pipe bytes C3 41 and read -rn1, it waits for a continuation after C3, then counts and returns A too; the line decoder instead produces two characters (�A). A lone invalid starter such as C0 can also leave read -n1 waiting for bytes although it already decodes to a replacement character. The base implementation consumed only one byte, so this over-consumption/stall is introduced by the new character counter.
   • Validate incrementally using the same malformed-sequence semantics as the lossy decoder: count invalid bytes as replacement characters and preserve a following valid byte for the next read.

2. [P2] Do not treat cached FIONREAD bytes as reserved — crates/brush-builtins-vendored/src/read.rs:446-460

   • After polling, the new cache skips wait_for_input while its count remains positive. On a pipe shared through cloned open files, reader A can cache several bytes and consume one, then reader B can drain the remainder while the writer stays open. A's next call skips polling and blocks in the read; cancellation and timeout checks before that syscall cannot run until more data arrives or the writer closes. OpenFile clones pipe readers (crates/brush-core-vendored/src/openfiles.rs:117-125), and background shell execution clones shell state (crates/brush-core-vendored/src/interp.rs:511-514), so this is reachable. At base, read polled before each byte; the new cache expands the stale-readiness window and violates the documented D-BUILTINS-READ-CANCEL contract.
   • Treat readiness as advisory and ensure a potentially blocking read can return to the poll/cancellation path (for example, a nonblocking read followed by poll/retry on WouldBlock).

3. [P2] Preserve Windows ACP decoding for internal pipelines — crates/brush-builtins-vendored/src/line_input.rs:18-19

   • External-command stdout is connected directly to raw pipeline Stdio and consumed as OpenFile bytes; it does not pass through pi-shell's outer stdout decoder. The new UTF-8-only lossy decoder therefore turns a native Windows command's CP1252 byte E9 into � when piped to read, whereas the base read returned é. The repository explicitly supports ACP output for native Windows programs in packages/coding-agent/src/...? The applicable decoder is pi-shell/src/output_decode.rs:8-15; its outer-stream fallback does not cover internal pipelines.
   • Apply the established UTF-8-first/Windows-ACP policy at the internal pipeline input boundary or share it from a lower layer, without introducing a dependency from brush-builtins up to pi-shell; add an external-producer-to-read Windows regression case.

CI / Verification

Exact-head CI is green: Rust test/check jobs, the affected-path aggregate, and Windows dev-doctor succeeded; the check-run set reports 26 successes and 4 skipped jobs, with no failed product checks. Skips were conditionally/manual-ineligible, not product failures. Added integration tests cover valid UTF-8, read -n, offsets/read-ahead, mapfile callbacks, and readonly arrays, but do not exercise the malformed-input, stale-readiness, or Windows ACP cases above. This review was static; no PR code was executed.

Axis Coverage

Axis	Verdict	Coverage
A1 — Intent / Policy / Contract	CHANGES_REQUESTED	Finding 2 violates the documented cancellation/timeout contract; no intent_projection was available.
A2 — Architecture / Correctness / Failure	CHANGES_REQUESTED	Findings 1 and 2 demonstrate malformed-character counting and stale-readiness failures.
A3 — Security / Privacy / Trust	APPROVED	No new injection, privilege, permission, or data-exposure path; input remains data.
A4 — Verification / Tests / CI	APPROVED	Exact-head required CI succeeded; missing edge-case tests are not separate merge blockers.
A5 — Context / Compatibility / Platform	CHANGES_REQUESTED	Finding 3 shows ACP data loss through Windows internal pipelines; no existing line-reader abstraction was duplicated.

Limitations

The findings are based on read-only inspection at head 82be8ea41a5411e3ada9f744129edf66c6008c10 against base/merge-base 355f4e23ba0da8e33264deecfa8ca7015ab7fed1. CI did not directly exercise the reported Windows ACP pipeline case; it was established by tracing the pinned producer-to-consumer path and existing decoder contract.

[probepark]

The new peer review requests changes. It does not provide evidence that the held findings are resolved.

Exact-head CI now passes, including Affected path validation. The previous CI hold is resolved. Passing CI does not resolve the reported edge cases.

The head remains 82be8ea41a5411e3ada9f744129edf66c6008c10. The new review retains malformed UTF-8 counting and cached pipe-readiness findings. It adds a Windows ACP pipeline finding. It does not resolve the refill-error rollback finding confirmed in the owner's correction.

1. Fix or provide specific counter-evidence for malformed UTF-8 counting and cached pipe readiness.
2. Address the failed-refill rollback finding and the new Windows ACP pipeline finding.
3. Include regression commands, the tested head, and exit codes. Then re-request review.

This follow-up evaluates the reply evidence; it is not a new code review. No approval, review dismissal, or body verdict change was made.

[DS-argus]

Thanks for the review. e7a41dc fixes all three P2s, each with a regression test.

1. Malformed UTF-8 over-consumed (read.rs CharCount)
CharCount no longer guesses a sequence length from the lead byte. It keeps up to four pending bytes and counts a character once std::str::from_utf8 either decodes them or reports them invalid (error_len() is Some). This matches bash's read_mbchar, which reads another byte only while mbrtowc returns (size_t)-2 (incomplete). The count now ends where the decoder places each U+FFFD.

read -r -n 1 x; read -r y, compared with bash 5.3.20:

input	bash x / y	this branch x / y
C0 41	c0 / A	U+FFFD / A
E0 80 41	e0 80 / A	U+FFFD U+FFFD / A
C3 A9 78	é / x	é / x

New test: read_count_takes_one_invalid_byte_and_leaves_the_rest.

2. Refill error kept the full block length (line_input.rs next_byte)
A read error now clears the block before the error is returned, so drop has nothing to seek back. bash's zreadc does the same: it sets lused = 0 when zread returns <= 0.

In the repro, 5000 bytes are written, read -r x <&1 fails, and then END is written. END now lands at offset 5000, as in bash. Before the fix it landed at 904.

New unit test: failed_refill_gives_nothing_back. It got offset 904 before the fix.

3. Cached FIONREAD count skipped the poll (read.rs read_event)
The cached count, bytes_ready(), and the rustix dependency are gone. A stream is polled before every byte again, as GJC's read did before this port. That way -t and cancellation are still checked after another reader drains the pipe. Regular files still read ahead.

• New unit test: test_read_line_times_out_after_another_reader_drains_the_pipe. A second reader drains a real pipe right after the first byte while the writer stays open. read returns TimedOut(Some("a")) within its 200ms timeout. Before the fix it was still blocked after 60s.
• read -r -t 1 x < fifo, with a second cat fifo draining the same FIFO, timed out at about 1.02s in 20 of 20 runs.
• The race you noted between the poll and the read remains. bash 5.3 has it too, since it waits in pselect before each read.

P3: left as is. bash's zsyncfd seeks a shared descriptor back the same way (lseek(fd, -(lused - lind), SEEK_CUR)), after read and before each mapfile callback.

The read -N regression from the earlier nit is also in: read_exact_count_takes_whole_utf8_characters_across_newlines.

Local checks on e7a41dc all pass:

• cargo test -p pi-shell --lib: 234 passed.
• brush-builtins unit tests, run standalone: 35 passed.
• bun run check:rs, plus the changelog fragment and porting-inventory checks.

@snowykr, could you take another look?

[probepark]

Code review (approval held because the exact-head CI plan is incomplete).

CI: gate pending — Affected path validation never reported on this head, so the approval gate returned NEED_LOCAL.
Scope: +545 / -100, 9 files — shell builtins and pi-shell regression coverage.
Conventions: changelog fragment present; generated files not included; porting inventory updated.
Notable:

• No blocking candidate found in the reviewed Rust diff. The UTF-8 decoding, character-counted read -n/-N, file-offset read-ahead rollback, readonly-array validation, and regression tests were checked.
• The changed behavior is covered by shell-level regression tests and the existing Rust checks described in the PR. Windows and Linux remain untested per the PR description.

Blocking: none.

ocr: blocking 0 / nit 0

Approve held: CI plan did not run re-run Dev CI for this head, or run the package checks below locally on e7a41dc; run the required affected-path/package checks at this head (or add them to the plan) and re-request review.

[DS-argus]

@snowykr Thank you for the second review, and sorry for the slow follow-up. My earlier reply went up a few minutes after your review, before I had seen it.

1. Malformed UTF-8 counting: fixed in 257dd83.

You were right. In e7a41dc I had matched bash, which counts C3 41 as one character, but this PR stores U+FFFD, so the variable still ended up over the limit. read -n/-N now counts characters the way decode_line decodes them:

• Each invalid sequence that becomes one U+FFFD counts as one character.
• The byte that shows a lead byte to be invalid starts the next character.

When that byte falls past the limit, a regular file takes it back so the next reader gets it.

read -r -n N x; read -r y, through the native binding:

input	N	before (e7a41dc)	now
file C3 41	1	x=�A, y empty	x=�, y=A
file C3 41 42	2	x=�AB, y empty	x=�A, y=B
pipe C3 41 42	2	x=�AB, y empty	x=�A, y=B
pipe C0 41	1	x=�, y=A	same

One case I couldn't fully meet: on a pipe or terminal, a byte that has already been read can't be put back. So for C3 41 with -n 1 on a pipe, read keeps the A in the variable (x=�A) rather than dropping it. I couldn't find a way to give it back without adding a pushback buffer to the shell, and even then a later external command wouldn't see it. If you'd prefer a different trade-off here, I'm happy to change it.

New tests:

• read_count_counts_invalid_bytes_as_they_decode covers the pipe cases.
• read_count_gives_a_file_the_byte_past_the_limit_back covers the file cases.
• Two LineInput::unread unit tests: one gives bytes back across a block boundary, and one confirms a pipe byte can't be given back.

2. Cached FIONREAD readiness: fixed in e7a41dc, as described in my earlier comment.

3. Windows ACP for internal pipelines: I'd like to ask your preference before changing anything.

You're right that this PR changes one thing here: a single-byte ANSI code page byte such as CP1252 E9 used to come out of read as é and now becomes U+FFFD. A few things make me unsure whether it belongs in this PR:

• Before this PR, read didn't apply the ACP policy either. It mapped each byte to the Latin-1 character with the same value, so only single-byte code pages whose bytes match Latin-1 came out right. Double-byte code pages such as CP949, GBK and Shift-JIS were already garbled by read.
• mapfile already decoded these lines as UTF-8 only, with replacement characters, before this PR.
• Doing it properly would mean moving or sharing pi-shell's OutputDecoder in a lower crate, deciding how its per-stream ACP switch applies when read is called once per line, and adding a Windows-only regression. I can't run Windows locally, so I'd be relying on Windows CI to verify it.

Would you prefer that I do it in this PR, or open a separate issue for applying the ACP policy to internal pipelines? I'll follow whichever you think is better.

Local checks on 257dd83:

• cargo test -p pi-shell --lib: 235 passed, exit 0.
• brush-builtins unit tests, run standalone with the brush-core path patch: 37 passed, exit 0.
• bun run check:rs, bun scripts/changelog-fragments.ts check, and bun scripts/verify-rust-porting-inventory.ts: exit 0.

Thanks again for the careful review.

[probepark]

Review (head 257dd83, gajae-reviewer on behalf of probepark)

ocr: blocking 2 / nit 0

Notable / required changes:

1. P2 — Pipe character limits still return too many decoded characters. crates/brush-builtins-vendored/src/read.rs:424-428,617-620 retains the lookahead byte when unread fails. For pipe bytes C3 41 42 0A, read -r -n 1 x; read -r y returns x=�A, y=B. The first variable contains two decoded characters despite its one-character limit. -N 1 takes the same path. The new test at crates/pi-shell/src/shell.rs:3714-3729 also accepts two replacement characters for -n 1 with E0 80 41. The author explicitly confirms this remaining limitation in the head's follow-up. Regular-file rollback resolves the file case, not this stream case. Preserve the count contract for pipes and terminals; add a regression that rejects the oversized value.
2. P2 — Windows native-command pipeline input loses previously preserved characters. crates/brush-builtins-vendored/src/line_input.rs:18-19 decodes every line as UTF-8 with replacement. An external producer's stdout goes directly to its redirected descriptor at crates/brush-core-vendored/src/commands.rs:245-249. This bypasses pi-shell's outer output decoder. On CP1252, a native producer's E9 0A previously yielded é in read; it now yields �. crates/pi-shell/src/output_decode.rs:8-21 documents the UTF-8-first/Windows-ACP output policy. Existing DBCS corruption and mapfile's previous behavior do not remove this newly introduced single-byte regression. Preserve this supported input path at an appropriate lower-layer boundary and add a Windows external-producer regression. The author acknowledges the change in the linked follow-up. This also remains a finding in the peer change request.
3. Unclassified CI blocker — Windows regression failed; base equivalence is not established. Windows dev:doctor + session-path regression fails packages/natives/test/walker-pool-unavailable.windows.test.ts:127: expected thread count <=20, received 21. The evidence producer and affected-path aggregate then fail because this required Windows job failed. This assertion is outside the changed files, but that alone does not prove a base failure. The inspected dev run skipped its Windows job. Re-run the failed exact-head job, or provide the same Windows failure on dev before treating it as unrelated.

CI: Three failed checks, with one underlying Windows failure and two dependent aggregate failures. Failure attribution remains unclassified. Rust check/test, native build, and Cargo build checks pass on this head.
Scope: +668 / -101, 9 files — vendored shell builtins, pi-shell shell integration regressions, Cargo dev-dependency, porting inventory, release-note fragment. OCR preview marks six files reviewable; their full diff was inspected.
Conventions: User-facing changelog fragment present; no released changelog or generated-artifact edits; no labels. tempfile is confined to dev-dependencies. No existing test expectations were weakened.
Blocking: Findings 1 and 2; unclassified CI failure in finding 3. The approval-only gate was not needed for this change request.

Issue #6543 / spec axis: Valid UTF-8 now flows through LineInput::next_byte → byte collection → decode_line → assign_input_to_variables (read.rs:143). Shell integration regressions cover valid UTF-8, -n, -N, callback handoff, and file offsets. The malformed-stream count contract is still not met.
Checked and clean: Failed-refill buffer clearing (line_input.rs:84-91), file-only read-ahead and unread handoff, readonly-array preflight, mapfile callback give-back, removal of cached FIONREAD readiness, valid multibyte counting, and cancellation checks before byte reads.
Could not assess: Independent local execution and Windows ACP runtime reproduction; this review is static plus exact-head CI inspection. No builds or tests were run in this pod. The existing immediate poll/read race remains a base limitation, not a new finding. The peer change request was not dismissed.

Verification evidence: At 2026-10-11T09:47:28Z, gh pr checks 6572 --repo Yeachan-Heo/gajae-code and gh run view 38126932931 --repo Yeachan-Heo/gajae-code --log-failed showed the exact-head results above. OCR rules/stamp command exited 0. The binary/full-index three-dot diff hash is 77cffc64a8b62a5947f67a84e5409928614757b4ee3cea93e49a2e843ae658db. The inspected SHA matches this review's pinned head; these results are not a merge-readiness claim.

Verdict: gajae.pr-review-verdict.v1 needs-human sha256:77cffc64a8b62a5947f67a84e5409928614757b4ee3cea93e49a2e843ae658db reviewer:critic reviewer-id:gajae-reviewer evidence:pipe-count-overrun;windows-acp-regression;windows-ci-unclassified

PR body verdict line count=0, not updated. Body verdict line is owned by DS-argus; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:77cffc64a8b62a5947f67a84e5409928614757b4ee3cea93e49a2e843ae658db reviewer:critic reviewer-id:gajae-reviewer evidence:pipe-count-overrun;windows-acp-regression;windows-ci-unclassified

[snowykr]

Verdict

CHANGES_REQUESTED

Summary

This PR adds UTF-8 line decoding and buffered read-ahead for Bash read/mapfile. The shared reader is a justified abstraction, but several reachable regressions remain in stream recovery, character-limit handling, shared file offsets, and Windows native-command pipelines.

Findings / Required Changes

1. [P2] Preserve read -n/-N limits on malformed UTF-8 streams — crates/brush-builtins-vendored/src/read.rs:420-428,617-620

   • With a pipe containing E2 82 41, read -n 1 needs the A byte to determine that the preceding UTF-8 prefix is invalid. It counts that prefix as one replacement character, but when unread cannot restore a pipe byte, A remains in the result: the variable gets �A (two characters) and the next reader cannot consume A. This is introduced by the new decode-aware counting path; the base byte-at-a-time path did not over-read the limit. Preserve the lookahead across builtin reads or otherwise return no more than the requested decoded characters while leaving the next character available.

2. [P2] Do not lose pipe input when mapfile assignment fails — crates/brush-builtins-vendored/src/mapfile.rs:132-134,179-186

   • Unlimited mapfile without -C selects ReadAhead::ToEof, and LineInput block-reads pipes without a way to return unread bytes. A reachable post-read failure such as mapfile -O 1 EPOCHSECONDS (a dynamic scalar that fails indexed-array assignment) occurs after the first line, while later lines may already have been drained into the buffer; dropping the reader loses them to the next consumer. At base, reads were one byte at a time and retained the rest of the pipe. Keep nonseekable read-ahead off when later assignment can fail, or prove/preflight that persistence cannot fail before consuming the block.

3. [P2] Finalize the decoded character count at EOF — crates/brush-builtins-vendored/src/read.rs:548-550

   • For read -N 2 on bytes 61 E2 followed by EOF, lossy decoding produces a� (two characters), but the pending incomplete UTF-8 prefix is not finalized in CharCount; the result is reported as EOF and the builtin returns nonzero despite having produced the requested count. The base byte-count path returned after two bytes. Finalize the pending replacement character before choosing success versus EOF so the assigned value and status agree with the -N character contract.

4. [P2] Avoid rewinding another reader's shared file offset — crates/brush-builtins-vendored/src/line_input.rs:135-143

   • A supported background-builtin schedule can corrupt a shared regular-file descriptor: exec 3<file; mapfile -n 1 -u 3 arr & cat <&3; wait on a file larger than the 4096-byte refill. The background mapfile reads a block and stops after one line; the parent cat can advance the same open-file description before LineInput drops. give_back then seeks backward from the new cursor by the unread buffer size, replaying bytes cat already consumed and skipping earlier bytes. Cloned shell fds share the file cursor, and base reads did not perform this later relative seek. Avoid block read-ahead for concurrently consumable descriptors or coordinate recovery against the shared cursor.

5. [P2] Preserve Windows ACP text in native-command pipelines — crates/brush-builtins-vendored/src/line_input.rs:18-19

   • On Windows with CP1252 ACP, a native producer writing byte E9 to stdout and piping it into read previously assigned é; this path now unconditionally decodes as UTF-8 and assigns �. The pipeline passes raw producer bytes directly to builtin stdin, so pi-shell's UTF-8-then-ACP OutputDecoder for final stdout/stderr never runs before assignment. Preserve the established Windows text policy for native pipeline input without changing Unix/UTF-8 behavior.

Non-blocking Observations

• The PR description says the three builtin files match the upstream commit except for headers/comments, but the pinned read.rs also has local stream-readiness and malformed-character-counting differences. The inventory row is more accurate; align the description with the final implementation.
• Optional coverage gaps: the added mapfile integration cases do not directly pin UTF-8/malformed-byte decoding, and the read -t 0 readiness branch has no committed direct test. These are not blockers independent of the findings above.

CI / Verification

Exact-head Rust tests, Rust checks, and the pi-shell cargo build passed (run 38126932931, jobs 114436656956, 114436656969, 114436656986). The same run failed Windows dev:doctor + session-path regression at the unchanged packages/natives/test/walker-pool-unavailable.windows.test.ts:127 assertion (expected at most 20 threads, observed 21); its evidence producer and affected-path aggregate then failed, and virtual integration was skipped. This does not establish a defect in the changed code. The repository dev ruleset requires an approving review but lists no required status checks. The PR's local check:ts and check:sdk-closure claims were not established by this exact-head run.

Axis Coverage

Axis	Verdict	Coverage
A1 — Intent / Policy / Contract	APPROVED	UTF-8 behavior and retained read guards match the primary intent; the upstream-equivalence statement is a non-blocking description mismatch.
A2 — Architecture / Correctness / Failure	CHANGES_REQUESTED	Findings 1–4: reachable stream loss/over-read, EOF status, and concurrent shared-offset corruption.
A3 — Security / Privacy / Trust	APPROVED	Input remains data assigned to shell variables; no new evaluation or privilege boundary was found.
A4 — Verification / Tests / CI	APPROVED	Exact-head Rust coverage passes; the Windows failure is in an unchanged test and is not a required status check.
A5 — Context / Compatibility / Platform	CHANGES_REQUESTED	Findings 2, 4, and 5; producer/consumer tracing confirms mapfile recovery, shared-fd, and Windows pipeline regressions. LineInput itself is a justified shared abstraction; no redundant existing reader was found.

Limitations

The Windows session-path job failed before its later Windows pi-shell tests, so this run does not independently exercise those platform-specific cases. No local PR tests were executed as part of this review.

[DS-argus]

@snowykr @probepark Thank you both for the careful reviews. I pushed c74a87a. It addresses findings 1–4 from snowykr's review and finding 1 from probepark's. I'll fix the Windows ACP finding in this PR as well, in a follow-up commit.

What changed

• I removed the block read-ahead that I had ported from upstream. read and mapfile now read one byte at a time again, as before this PR, and never seek.
• Sometimes read -n/-N has to read one byte past its limit before it knows a lead byte is invalid. That byte is now held with the open file description, together with the backslash before it if it was escaped.
  • The store is a small openfiles::HeldInput inside brush-core's OpenFile. Every try_clone shares it, so the next builtin read or mapfile of the same input takes the held bytes first.
  • This is close to bash's zungetc pushback. As in bash, an external command that reads next does not see the held byte.

1. -n/-N limits on malformed UTF-8 streams (snowykr 1, probepark 1)

Pipes, through the native binding (read -r <opt> 1 x; read -r y):

input	opt	257dd83	now
C3 41 42 0A	-n	x=�A, y=B	x=�, y=AB
C3 41 42 0A	-N	x=�A, y=B	x=�, y=AB
E2 82 41 0A	-n	x=�A, y empty	x=�, y=A
E0 80 41 0A	-n	x=��, y=A	x=�, y=�A

The test that used to accept �� for E0 80 41 now expects [�][�A].

2. mapfile losing pipe input when assignment fails (snowykr 2)

Without read-ahead, mapfile consumes input only up to each delimiter. printf 'a\nb\nc\n' | { mapfile -O 1 EPOCHSECONDS; cat; } prints the same error and rc=99 as base, and cat then gets b and c, also as on base.

3. Character count at EOF (snowykr 3)

An incomplete character at EOF now counts as one character, because it decodes to one U+FFFD. The count happens before read chooses between success and EOF. The status now matches bash 5.3 in every row I checked:

pipe input	command	257dd83	base	bash 5.3	now
61 E2	read -r -N 2 x	1	0	0	0
61 E2	read -r -n 2 x	1	0	0	0
61 E2	read -r -n 3 x	1	0	1	1
E2	read -r -n 1 x	1	0	0	0
61 E2	read -r x	1	1	1	1

4. Shared file offset (snowykr 4)

Nothing seeks anymore, so read and mapfile can no longer move a file offset that another reader has advanced.

I couldn't reproduce the race itself. I ran it 10 times on 257dd83, on base, and in bash, using a brace-group redirect because the embedded shell has no exec, and saw 0 duplicates. So I added a unit test instead:

• Two descriptors share one open file. One reads a, the other reads bc, and then the first reads d.
• The shared offset must end at 4.
• The test fails on 257dd83.

5. Windows ACP for native-command pipelines (snowykr 5, probepark 2)

This is not in this commit. In a follow-up commit on this PR, I'll apply the existing UTF-8-then-ACP policy to native-command pipeline input. I'll add a Windows regression test whose name includes windows, so the Windows job runs it. I can't run Windows locally, so I'll rely on Windows CI for that part.

CI (probepark 3)

The Windows failure is the assertion at packages/natives/test/walker-pool-unavailable.windows.test.ts:127 (expected at most 20 threads, got 21).

• The same job passed on this PR's first head, 82be8ea.
• The same assertion failed on dev in dev: walker-pool-unavailable.windows.test.ts thread-count assertion fails on Windows dev:doctor after #5980 #6010, which fix(natives): stabilize thread count measurement in walker pool unavailability test #6013 closed.

I can't rerun jobs myself. Once CI is approved for c74a87a, I'll check the fresh Windows run.

Tests

Before changing the code, I added these tests and confirmed that they fail on 257dd83, with the old outputs shown above:

• Finding 1:
  • read_count_counts_invalid_bytes_as_they_decode, whose expectation is now [�][�A] for E0 80 41
  • read_count_holds_the_byte_past_the_limit_for_the_next_read
  • read_count_holds_both_bytes_of_an_escape_past_the_limit
• Finding 2: mapfile_assignment_failure_leaves_the_rest_of_a_pipe
• Finding 3: read_count_counts_an_incomplete_character_at_eof
• Finding 4: line_input::tests::a_reader_takes_only_the_bytes_it_hands_out
• Held input belongs to one input:
  • It doesn't follow an fd number into a here-string: read_held_input_stays_with_its_input_not_the_descriptor_number.
  • A subshell shares it: read_held_input_is_consumed_once_across_a_subshell.
  • read_count_holds_a_file_byte_that_external_readers_do_not_see.
  • mapfile_reads_held_input_first_and_decodes_utf8, which also covers snowykr's optional mapfile UTF-8 gap.

These also pass on 257dd83. They guard behavior that must not change:

• read_from_a_reopened_file_does_not_see_held_input: reopening the file starts fresh.
• read_t_0_reports_ready_input: snowykr's optional -t 0 gap.

New read unit tests use the new store directly:

• A pipe read with -n 1 or -N 1 holds the byte past the limit for the next read.
• -t 0 reports held input as ready without consuming it.
• Cancellation leaves held input in place.
• Held input is served before any wait.
• A read times out instead of blocking when another reader takes the held byte first.

I also updated the PR description to match the final implementation, as snowykr noted.

Local checks on c74a87a (macOS arm64)

cargo test -p pi-shell --lib                                      # 245 passed, exit 0
cargo test --manifest-path crates/brush-builtins-vendored/Cargo.toml --lib \
  --config 'patch.crates-io.brush-core.path="crates/brush-core-vendored"'  # 43 passed, exit 0
cargo test --manifest-path crates/brush-core-vendored/Cargo.toml --lib     # 51 passed, exit 0
cargo check -p pi-shell --lib --target x86_64-pc-windows-msvc     # exit 0 (compile only)
bun run check:rs                                                  # exit 0
bun scripts/changelog-fragments.ts check                          # exit 0
bun scripts/verify-rust-porting-inventory.ts                      # exit 0

Clippy on both vendored crates reports no new diagnostics compared with 257dd83.

Thanks again for the thorough reviews.

[probepark]

Scope-only review (head c74a87a, on behalf of probepark)

Large PR — code review skipped. Human review is required. No approval, change-request verdict, or body update is issued.

Scope: +1006 / -162 across 13 files. ocr delegate preview reports +998 / -160 across 10 reviewable files (1158 changed lines). This exceeds this reviewer's 800-line limit. The embedded shell integration tests are in a source file, so OCR includes them in the reviewable count.

Files by area:

• Vendored builtins: crates/brush-builtins-vendored/src/{lib.rs,line_input.rs,mapfile.rs,read.rs}.
• Shared descriptor/input handling: crates/brush-core-vendored/src/{ioutils.rs,openfiles.rs,shell/history.rs,sys/windows/fd.rs}.
• Shell integration regressions and test dependency: crates/pi-shell/src/shell.rs, crates/pi-shell/Cargo.toml, Cargo.lock.
• Porting ledger and release notes: docs/rust-porting-inventory.md, packages/coding-agent/changelog.d/6543-bash-read-decodes-utf8.md.

CI: gh pr checks reports no checks; the exact-head check-runs API returns total_count=0. No CI pass is established. The PR reports local macOS checks; I did not run builds or tests.
Conventions: User-facing changelog fragment present. No released changelog or generated artifact appears in the changed-file list. No labels. The new tempfile dependency is confined to dev-dependencies.

ocr: blocking 0 / nit 0
These counts cover scope/manifest inspection only. Rust rules were retrieved, but production code was not reviewed because the reviewable size exceeds the limit. Zero is not a code-cleanliness verdict.

Checked: changed-file inventory, OCR size, release-note placement, dependency section, exact-head check availability.
Not assessed: issue #6543 behavior, code correctness, prior findings' resolution, concurrency/cancellation, Windows compatibility, and runtime regression coverage. Existing change-request reviews remain unchanged. The author states that Windows ACP support still needs a follow-up commit in the update.

Have a maintainer review the listed areas and the outstanding findings before merging.