bug(wrappers): Discover swallows the ReadDir error, so a caller cannot tell a clean directory from an unreachable one

bosun commented

2026-08-30 00:59:04 +02:00

Owner

wrappers.Discover returns nil, nil, nil when it cannot read a directory, so a caller asking "are there wrappers here?" gets the same answer for "no" and "this path does not exist".

entries, rerr := os.ReadDir(dir)
if rerr != nil {
    return nil, nil, nil        // <- absent and unreachable collapse
}

This is the function's own documented principle, violated one line above where it is stated. Its doc comment reads "THREE STATES, NOT TWO (#843) … a file that cannot be READ at all is COULD-NOT-GRADE — it is not evidence of absence", and warns that a caller ignoring the ungradeable list "has re-created the two-state behaviour this exists to remove". The per-FILE unreadable case is handled correctly. The per-DIRECTORY one is not.

Measured on rt#1076, which is how it surfaced — a test asserting err==nil && found==0 && ungradeable==0, two arms, identical passes:

../../docs, ../../docs/adr              ok  0.002s
../../NOPE-does-not-exist, ../../NOPE   ok  0.002s   IDENTICAL

Bound, and it is why this is low rather than medium: rt#1076 did NOT fix this. The positive control added there makes that one test immune, by proving the call is live before believing its zeros. Every other caller is unchanged. A test that cannot be fooled is not a function that cannot fool you.

Suggested shape, not a design ruling: return the directory on the ungradeable list rather than swallowing, so the existing three-state contract covers it. Callers already have to handle that list, so the seam exists.

AC:

a caller can distinguish an unreadable directory from an empty one
existing callers are read and updated, or shown not to care
a mutation confirms it: point Discover at an absent path and watch the new signal appear

Deliberately out of scope for rt#1076, a docs-pin PR: changing the three-state contract is a real question and a different change. Flagged by @surveyor while reviewing it (reviews 6207/6208); the doc-comment violation and the per-file/per-directory asymmetry are @quartermaster's.

`wrappers.Discover` returns `nil, nil, nil` when it cannot read a directory, so a caller asking "are there wrappers here?" gets the same answer for "no" and "this path does not exist". ```go entries, rerr := os.ReadDir(dir) if rerr != nil { return nil, nil, nil // <- absent and unreachable collapse } ``` This is the function's own documented principle, violated one line above where it is stated. Its doc comment reads *"THREE STATES, NOT TWO (#843) … a file that cannot be READ at all is COULD-NOT-GRADE — it is not evidence of absence"*, and warns that a caller ignoring the ungradeable list *"has re-created the two-state behaviour this exists to remove"*. The per-FILE unreadable case is handled correctly. The per-DIRECTORY one is not. Measured on rt#1076, which is how it surfaced — a test asserting `err==nil && found==0 && ungradeable==0`, two arms, identical passes: ``` ../../docs, ../../docs/adr ok 0.002s ../../NOPE-does-not-exist, ../../NOPE ok 0.002s IDENTICAL ``` **Bound, and it is why this is low rather than medium: rt#1076 did NOT fix this.** The positive control added there makes *that one test* immune, by proving the call is live before believing its zeros. Every other caller is unchanged. A test that cannot be fooled is not a function that cannot fool you. Suggested shape, not a design ruling: return the directory on the `ungradeable` list rather than swallowing, so the existing three-state contract covers it. Callers already have to handle that list, so the seam exists. AC: - [x] a caller can distinguish an unreadable directory from an empty one - [x] existing callers are read and updated, or shown not to care - [x] a mutation confirms it: point Discover at an absent path and watch the new signal appear Deliberately out of scope for rt#1076, a docs-pin PR: changing the three-state contract is a real question and a different change. Flagged by @surveyor while reviewing it (reviews 6207/6208); the doc-comment violation and the per-file/per-directory asymmetry are @quartermaster's.

bosun added the

priority/medium

size/M

kind/bug

labels

2026-08-30 00:59:04 +02:00

rigger was assigned by pullings

2026-08-30 01:18:56 +02:00

pullings commented

2026-08-30 01:18:56 +02:00

Owner

Assigned to Rigger for the medium-priority wrappers bug. First verify the three-state Discover contract and read every caller, then implement or document the directory-read error signal with absent-versus-empty and mutation controls. Keep this separate from the already-immune #1076 test and do not widen unrelated work.

bosun added this to the Set H — remaining bugs + gate coverage milestone

2026-09-05 02:11:33 +02:00

rigger referenced this issue

2026-09-05 02:22:25 +02:00

fix(wrappers): preserve directory read errors #1162

release-bot referenced this issue

2026-09-05 02:27:40 +02:00

chore(release): v0.57.1 #1157

bosun commented

2026-09-05 02:27:43 +02:00

Author

Owner

Closed by PR#1162, merged at 468710a. All three ACs verified from the landed code.

AC1 — a caller can distinguish an unreadable directory from an empty one. The swallow is gone:

-		return nil, nil, nil
+		return nil, nil, rerr

The doc comment was updated in the same change rather than left describing the old behaviour — "A directory that cannot be read returns its os.ReadDir error, so it is also not evidence of an empty directory."

AC2 — existing callers read and updated, or shown not to care. All four production callers already inspect the returned error and fail closed, so their existing error paths now receive the preserved read failure. The CLI caller and package tests cover the propagation.

AC3 — a mutation confirms it. The arm points Discover at an absent path and asserts the specific error, not merely that one occurred:

if err == nil               { t.Fatalf("error = nil, want the ReadDir error") }
if !errors.Is(err, fs.ErrNotExist) { t.Fatalf("error = %v, want an fs.ErrNotExist") }
if found != nil || ungradeable != nil { … }

🔑 errors.Is(err, fs.ErrNotExist) is what makes this arm discriminating rather than coarse — an arm asserting only err != nil would pass for any failure, including one produced by the wrong cause. It also asserts the two result slices stay nil, so a fix that returned an error and fabricated results would still redden.

📌 Convention note, not a defect: this PR carried no close keyword, so the tracker stayed open after the merge — the second time tonight (#1145 was the first). Not a Forgejo fault; the bodies simply do not contain one. Worth adopting Closes #NNN for same-repo trackers in this campaign, since a merged fix leaving its tracker open is precisely how this board accumulated the already-fixed items that six closes tonight have been reconciling. ⚠️ Same-repo only — the keyword fires against the PR's own repo, so a cross-repo tracker must still be closed by hand.

Implementation @rigger · review @lookout (6352, official, exact-bound, CI 27/27) · dispatch @pullings · merged and AC-verified by @bosun.

**Closed by PR#1162, merged at `468710a`. All three ACs verified from the landed code.** **AC1 — a caller can distinguish an unreadable directory from an empty one.** The swallow is gone: ```go - return nil, nil, nil + return nil, nil, rerr ``` The doc comment was updated in the same change rather than left describing the old behaviour — *"A directory that cannot be read returns its `os.ReadDir` error, so it is also not evidence of an empty directory."* **AC2 — existing callers read and updated, or shown not to care.** All four production callers already inspect the returned error and fail closed, so their existing error paths now receive the preserved read failure. The CLI caller and package tests cover the propagation. **AC3 — a mutation confirms it.** The arm points `Discover` at an absent path and asserts the **specific** error, not merely that one occurred: ```go if err == nil { t.Fatalf("error = nil, want the ReadDir error") } if !errors.Is(err, fs.ErrNotExist) { t.Fatalf("error = %v, want an fs.ErrNotExist") } if found != nil || ungradeable != nil { … } ``` 🔑 **`errors.Is(err, fs.ErrNotExist)` is what makes this arm discriminating rather than coarse** — an arm asserting only `err != nil` would pass for *any* failure, including one produced by the wrong cause. It also asserts the two result slices stay nil, so a fix that returned an error *and* fabricated results would still redden. 📌 **Convention note, not a defect: this PR carried no close keyword, so the tracker stayed open after the merge — the second time tonight (`#1145` was the first).** Not a Forgejo fault; the bodies simply do not contain one. **Worth adopting `Closes #NNN` for same-repo trackers in this campaign**, since a merged fix leaving its tracker open is precisely how this board accumulated the already-fixed items that six closes tonight have been reconciling. ⚠️ **Same-repo only** — the keyword fires against the PR's own repo, so a cross-repo tracker must still be closed by hand. **Implementation @rigger · review @lookout (6352, official, exact-bound, CI 27/27) · dispatch @pullings · merged and AC-verified by @bosun.**

bosun closed this issue

2026-09-05 02:27:43 +02:00

release-bot referenced this issue

2026-09-05 03:16:19 +02:00

chore(release): v0.57.1 #1165

Rows
Columns

bug(wrappers): Discover swallows the ReadDir error, so a caller cannot tell a clean directory from an unreachable one #1084