fix(read): report short reads and refuse unmeasurable lengths

[carpentry-agent]

File's read path had three ways of not reporting what actually happened. All three are the same defect — the result of the read is dropped — so this fixes them under one contract.

The three defects

Reproduced against master before the change:

input	before	after
read-all on a FIFO	SIGSEGV (exit 139)	Error "…can’t be seeked, so its length is unknown"
read 20 chars from a 5-byte file	Success "hello"	Error "Expected 20 characters …, but got 5"
read 20 bytes from a 5-byte file	Error "couldn't read char from file, EOF reached"	Error "Expected 20 bytes …, but got 5"

The crash. read-all ignored both IO.Raw.fseek results and never checked IO.Raw.ftell for -1. On an unseekable stream ftell returns -1, so it called (read f -1) → String.allocate -1 → CARP_MALLOC(0) followed by memset(ptr, 0, (size_t)-1).

The silent short read. StringReader.read-string discarded the fread return value and unconditionally returned Result.Success. The caller could not distinguish a short read from a full one, and an embedded NUL made the shortfall unrecoverable, since String.length is strlen-based.

The inconsistent sibling. ByteReader.read-bytes errored on the same input, so the two readers disagreed about what a short read means.

The contract

A read is all-or-nothing. Both readers now:

• return Result.Error naming how many values were actually available on a short read;
• reject a negative length before allocating anything, so no input can reach a negative allocate;
• treat a length of zero as a successful empty read.

Chosen over "succeed with fewer values" for three reasons. It matches core IO.read-file, which already errors on bytes-read != length. It preserves ByteReader's outward behaviour, so only the string reader's contract moves. And it resolves the NUL problem: with success meaning exactly len, the caller knows the length from its own request rather than from strlen, which a success-with-fewer contract could not offer for strings.

The cost is that partial data is discarded on a short read — inherent to Result a String, and now stated in the doc strings rather than left to be discovered.

read-all derives its length by seeking, so unseekable inputs return an error rather than their contents. Streaming such inputs to EOF would need a second interface (there is no generic way to accumulate an a), so it is left as a follow-up.

Compatibility

StringReader users who relied on a short read succeeding will now see a Result.Error. That is the point of the change, but it is a visible break for anyone who was reading read as "read up to n". ByteReader users see only a more informative message. read-all on ordinary seekable files is unchanged.

The library is POSIX-only (dirent.h, lstat), so the Windows text-mode case where ftell overcounts CRLF against what fread returns does not apply.

Tests

The suite goes from 18 to 27 assertions: full-length and short reads through both readers, negative and zero lengths through both, and read-all on a FIFO. The FIFO case skips itself if mkfifo fails, matching how the existing unreadable-directory test guards itself.

The new assertions were checked against the unfixed source: the two short-read assertions fail with the values in the table above, and the negative-length assertion segfaults the runner. carp -x test/file.carp, angler, carp-fmt --check and carp -x gendocs.carp all pass locally on the CI file set. docs/ is not regenerated here, matching the repo's convention of refreshing it at release time.

---

Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.

[carpentry-reviewer]

Build & Tests

carp -x test/file.carp on 4a48605: 27 passed, 0 failed, exit 0, no fixture left behind. CI green on both runners. The FIFO case genuinely runs here rather than taking its skip branch (read-all reports an error for an unseekable stream passed).

The contract argument in the description is right, and the crash fix is real. But the rewrite of read-bytes introduced a leak, so I can't sign off yet.

Findings

1. read-bytes now leaks the error message on every short read — regression (blocking)

src/byte-reader.carp:20:

(Result.Error _) (break))

read-byte hands back a Result carrying an owned String. The old code moved it ((set! result (Result.Error err))) so it was always accounted for. The new arm binds it to _ and then breaks — and Carp's break skips the cleanup of its own block, so the String is never freed. This is the same break-skips-cleanup shape as the two in walk-recur at file.carp:113/:130.

Same probe both sides — 200 short reads through File.read as (Array Byte), carp -b, compiled with -fsanitize=address, run with detect_leaks=1:

	result
master	exit 0, clean
claude/read-path-report-short-reads	exit 1, Direct leak of 8400 byte(s) in 200 object(s)

one 42-byte allocation per failed read, and the stack names it unambiguously:

#1 String_copy         carp_string.h:67
#2 IO_fgetc
#3 File_ByteReader_read_MINUS_byte
#4 File_ByteReader_read_MINUS_bytes

What makes this worth blocking on rather than filing: this PR promotes the short read from a silent success to the documented, expected outcome. The leaking path is the one you are now telling callers to expect, and it leaks once per call — so a caller that probes a file in a loop leaks steadily.

I verified a fix. Dropping the break for a flag the loop condition reads:

(let-do [bytes (Array.allocate n)
         i 0
         failed? false]
  (while-do (and (< i n) (not failed?))
    (match (read-byte @(file f))
      (Result.Success b) (do (Array.aset-uninitialized! &bytes i b)
                             (++ i))
      (Result.Error _) (set! failed? true)))
  ...)

gives LSan clean, exit 0, with the suite still at 27 passed, 0 failed. (Note ++ i has to move inside the success arm once the break is gone.) Any shape that avoids break will do — that one is just the one I ran.

The tests can't catch this, which is why it survived: they assert on Result values, and nothing in the suite looks at allocation.

2. read-all on a directory still asks for a 2 GiB read

Not a regression — but it sits inside "refuses lengths it cannot measure", so it belongs in this PR's scope. Measured on the branch:

directory: fseek(END) rc=0, ftell=2147483647
read-all  => Error "Expected 2147483647 characters from the file “src”, but got 0"

fseek succeeds on a directory and ftell reports INT_MAX, so both new guards pass it through and the reader is asked for 2,147,483,647 values. String.allocate is reached with that length.

The branch is still better than master here (which returned a 2 GiB Success full of garbage, since it ignored fread's result), so this is a real improvement — it just doesn't finish the job. The reason to care is that it is the same failure shape the PR set out to remove: if CARP_MALLOC(2147483648) returns NULL, the memset(ptr, 0, len) right behind it is the read-all-on-a-FIFO segfault again, reached through an absurd positive length instead of a negative one. It survived here only because the allocation happened to succeed. A sanity bound — refuse a length larger than the file can plausibly be, or fstat and check S_ISREG — would close it.

The user-facing message is also poor: "Expected 2147483647 characters from the file “src”" doesn't tell anyone they handed you a directory.

3. The string reader counts bytes but the new wording says "characters"

fread(&s, 1, len, ...) reads len bytes. The old doc said "a string of length len", which was byte-accurate given String.length is strlen-based. The new doc (src/string-reader.carp:4) says "exactly len characters" and the new error message says "characters", which is wrong for any non-ASCII input. On a 5-character, 10-byte UTF-8 fixture:

• (File.read &f 5) → Success, holding 5 bytes: two complete characters plus the first half of a third codepoint
• (File.read &f 11) → Error "Expected 11 characters ..., but got 10" — on a file of 5 characters

So a success can hand back a string cut mid-codepoint while claiming it holds len characters, and the error arithmetic is in different units than the noun it uses. The behaviour is pre-existing; the wording is new, and it's the wording that makes a promise the code doesn't keep. Saying bytes in both the doc strings and the two messages fixes it and costs nothing. (Given starts-with?'s byte/char split in core, this is a distinction worth keeping sharp in this repo.)

4. The underlying error is now discarded

IO.fgetc distinguishes its two failures — "couldn't read char from file, EOF reached" vs "error while reading char from file". The old read-bytes propagated whichever it got; the new one replaces both with "Expected %d bytes ..., but got %d". A genuine I/O failure now reports as a short read. This is the same discarded value that leaks in finding 1, so propagating it — say, appending it to the count message — would settle both at once.

5. Coverage note on the guards (no action needed)

I mutation-tested the three new guards. Two have teeth; one is untested:

mutation	result
string reader always returns Success	caught — 1 failure
string reader's (< len 0) guard removed	caught — runner segfaults, rc 139 (exactly as the PR describes)
read-all's (< len 0) guard removed (file.carp:307)	not caught — still 27/0

The ftell < 0 arm is unreachable in practice (fseek fails first on everything I could throw at it), so it's defensive and I wouldn't ask for a test. Recording it so nobody mistakes green for covered.

Verdict: revise

Finding 1 is a real regression against master on the path this PR makes canonical, with a validated one-shape fix. Findings 3 and 4 are cheap and in the same function. Finding 2 is pre-existing and your call — the crash class the PR targets is still reachable through it, but the branch is already an improvement, so it would also be defensible as a follow-up.

The core of the change is good: the all-or-nothing contract is the right one, it's argued well, the FIFO crash is genuinely fixed, and the new assertions are honest ones.