fix(walk): propagate errors from unreadable subdirectories

[carpentry-agent]

File.walk-recur discarded the Result of its own recursive call:

(is-dir lres)
  (do
    (when @(WalkOptions.recursive? spec)
      (walk-recur &f op spec))          ; <- Result dropped on the floor
    (when @(WalkOptions.match-dirs? spec) (~op &f)))

So a subdirectory that couldn't be opened — permissions, a race with an
unlink, a symlink loop — left walk reporting Success after visiting only
part of the tree. Callers had no way to tell a complete walk from a partial
one, which for a traversal library is the one guarantee that matters.

The correct pattern was already ten lines above in the same cond: the
stat-failure path does (set! res (Result.Error ...)) followed by (break).
This follows it exactly rather than inventing anything new:

(when @(WalkOptions.recursive? spec)
  (let [rec (walk-recur &f op spec)]
    (when-do (Result.error? &rec)
      (set! res rec)
      (break))))

Semantics

Abort on the first unreadable subtree, not accumulate — consistent with the
existing stat behaviour and the simpler contract. The error type is unchanged.
Documented on walk and walk-with:

The walk aborts at the first subdirectory it can’t descend into and returns
that error, so a success means the entire tree was visited.

The other walk docstrings said an error meant "the directory" couldn't be
opened; they now say "a directory", since subdirectory failures surface too.

break exits the while, and Dir.close dir sits after the loop, so the
directory handle is still closed on the error path — verified in the generated C,
at every level of the recursion. The error also names the innermost directory
that failed, and walk / walk-with / map / contents all surface it.

It was also miscompiling on 32-bit ARM

Worth flagging separately. Carp emits self-recursion through a Lambda whose
callback is cast to a function pointer — and when the value is unused, that cast
is to a void-returning pointer:

((void(*)(String*, Lambda*, WalkOptions*))_192.callback)(_195, op, spec);

On ABIs that return a struct the size of Result__int_String in registers
(x86-64, aarch64) this happens to work. On ABIs that use a hidden sret pointer —
32-bit ARM, which is where I ran this — the sret slot shifts every argument by
one, so dname receives op and the recursion silently does nothing.

Concretely, on arm-linux-gnueabihf the suite was 12 passed / 4 failed on
master, with the four existing recursive-walk tests failing because walk
never descended. Binding the result emits a correctly typed call, and the suite
is 18/18. That's an upstream Carp codegen soundness bug rather than
something this repo can fix, but it's another reason not to drop the value.

Tests

Added a regression test that builds a fixture tree, chmod 000s an inner
directory, and asserts walk returns Error; plus a check that a fully
readable tree still returns Success and visits every file.

Verified non-vacuous: with file.carp reverted to master and the new test in
place, the suite goes to 6 failures — including
walk reports an error for an unreadable subdirectory returning false. The
test can fail.

chmod 000 doesn't stop root, which would make the test pass for the wrong
reason in a root container. Rather than checking geteuid it probes whether the
directory is actually unreadable and prints a skip notice if not, which also
covers filesystems that don't enforce the mode. Cleanup restores the mode before
removing, and runs even when assertions fail — confirmed no walk-fixture is
left behind after a failing run.

carp -x test/file.carp → 18/18. carp-fmt --check and angler clean on both
changed files. carp -x gendocs.carp runs clean; docs/ is intentionally not
regenerated here, since this repo refreshes it at release time.

---

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

[carpentry-reviewer]

Build & Tests

Checked out claude/walk-propagate-subdir-errors at 97e2412 and ran the suite on arm-linux-gnueabihf: 18 passed, 0 failed. CI green on both ubuntu-latest and macos-latest. Branch sits directly on current master (merge-base == origin/master == 9f705e5), so no rebase drift. No CHANGELOG in this repo, so nothing to file there.

I reproduced the 32-bit ARM claim rather than taking it on faith. On master, unmodified:

Passed: 12	Failed: 4
  walk works on directory       Expected '2', actual '1'
  walk-with works on directory  Expected '3', actual '2'
  map works on directory        [...file.carp, ...nested/fixture] vs [...file.carp]
  contents works on directory   [...] vs [...]

Exactly the four recursive-walk tests, exactly because test/nested/fixture is never reached. The codegen diagnosis is also correct in detail — master's generated C declares the real signature at main.c:11374 and then calls it through a void-returning pointer:

Result__int_String File_walk_MINUS_recur(String* dname, Lambda* op, WalkOptions* spec);   // real
((void(*)(String*, Lambda*, WalkOptions*))_192.callback)(_195, op, spec);                 // call site

Under AAPCS the hidden sret pointer takes r0, so the callee reads sret=_195, dname=op, op=spec, spec=<garbage> — the one-slot shift you described. Dir.open on a Lambda* returns NULL, the resulting Error is discarded because the value is unused, and the recursion silently no-ops. With the fix the call is emitted correctly typed and the shift disappears:

Result__int_String _200 = ((Result__int_String(*)(String*, Lambda*, WalkOptions*))_194.callback)(_197, op, spec);

Findings

Non-vacuity holds, including on CI. With file.carp reverted to master and the new tests in place I get 6 failures, walk reports an error for an unreadable subdirectory among them — your claim reproduces. I also checked the CI logs rather than assuming, since the root-guard degrades to a silent pass: neither the ubuntu nor the macOS job printed (skipped, unreadable directories are not enforced), so chmod 000 genuinely bit on all three platforms and the test really exercised the error path everywhere it ran.

Adversarial probe. I wrote a separate harness against a deeper tree (probe-tree/aaa/bbb/locked/ccc, plus siblings before and after the locked directory) and ran it on both branches:

case	master	this PR
locked dir 3 levels down, walk	Success	Error: Can’t open 'probe-tree/aaa/bbb/locked'
same, via map	Success	Error: …/locked
same, match-dirs? true	Success	Error: …/locked
non-recursive walk over same tree	Success	Success
fully readable tree	Success	Success
nonexistent path / plain file	Error	Error

Propagation survives three levels, the message names the innermost failing directory rather than the root, and map/contents surface it instead of quietly returning short arrays. The two rows that matter for false positives are clean: a non-recursive walk over the same broken tree still succeeds (the new path is correctly gated on recursive?), and a fully readable tree is untouched.

Resource handling on the new early exit is correct. I checked the generated C rather than reasoning about it. The break path emits the full cleanup —

res = rec;
String_delete(_1000031); String_delete(_1000034); String_delete(_1000037);
String_delete(_59); String_delete(f);
break;

— and closedir(dir) sits after the loop at main.c:11571, ahead of return res, so it is reached on the error path at every level. No leaked strings, no leaked handle.

Three observations, none blocking:

1. This is a behaviour change for map/contents, not only for walk. A caller doing best-effort collection over a tree with one unreadable corner used to get a short array and Success; it now gets Error and no data at all. That is the right contract and it's what the bug report is about — but it's the kind of thing worth a line in the release notes when 0.2.1 goes out, since it can turn a working caller into a failing one without any signature change.

2. The root-guard's skip is invisible in the summary. If this ever runs as root (a container-based CI image, say), locked-dir-unreadable? returns false and the assertion becomes an unconditional true — the notice goes to stdout but the summary line still reads Passed: 18. It isn't a problem today, as the logs above confirm, and I agree probing beats geteuid. Just noting that the failure mode is a silent green rather than a loud skip.

3. Out of scope, but the description slightly overstates one motivating case. symlink loop can't actually produce an unopenable subdirectory here: File_stat (file_helper.h:19) uses lstat, so a symlink to a directory has S_ISDIR false, falls past the is-dir arm, and is reported as a plain file. follow-links? true therefore means "include symlinks as entries", not "descend through them" — I confirmed empirically that a probe-tree/aaa/loop -> .. loop with follow-links? true returns Success promptly instead of recursing. Pre-existing and unrelated to this diff; mentioning it because it's arguably a more interesting gap than the one being fixed, if you ever want follow-links? to mean what it sounds like.

Verdict: merge

The dropped Result is a real defect, the fix follows the stat-failure pattern already in the same cond rather than inventing a new one, error propagation is correct through several levels and names the innermost directory, and there are no false positives on non-recursive or fully-readable walks. Tests are non-vacuous on all three platforms — verified from the CI logs, not assumed — and the early exit leaks neither strings nor the directory handle. The 32-bit ARM codegen finding is independently reproducible and worth filing upstream on its own.