mirror of
https://github.com/max-sixty/worktrunk.git
synced 2026-09-14 20:00:38 +08:00
Maximilian Roos
e4238e8369
Six guards read `src/`, or the snapshot corpus, as text rather than
compiling it — no stray `println!` outside the allowlist, no `eprintln!`
resolving to std's macro instead of anstream's, no bare
`env!("VERGEN_…")`, no `include_str!` of a path the package won't ship,
no host path in a `.snap`, and nothing but `src/testing/mod.rs` spawning
`wt` through `CARGO_BIN_EXE_wt`. Each asserts *absence* over what its
walk handed it, and that polarity is what makes a swallowed read
dangerous here in a way it isn't for code that acts on what it finds: a
file that drops out of the walk is indistinguishable from a file with
nothing wrong in it, so `violations` stays empty and the test passes
green over less than it claims.
Each carried its own copy of the same recursion — read the directory,
iterate entries, recurse into subdirectories, read and check files with
a matching extension. The copies drifted, and #3911 is the evidence:
closing a swallowed read in one left the identical swallow in the
others, and the review that caught it had to list remaining sites by
hand. `tests/common/source_scan.rs` owns the walk now and all six call
it. The guards keep what is actually theirs: which extension, what
counts as a violation, and their own coverage assertion.
Six swallowed reads go with it. `output_system_guard.rs` had
`entries.flatten()` in both its walks and `Err(_) => return` in both its
file readers; `version_build.rs` had `entries.flatten()` and `let Ok(..)
else { return }`. `flatten()` is the one that hides, dropping a
per-entry `io::Error` without looking like a `return` at all — a
`println!` in a file whose `stat` fails went unreported by
`check_no_unexpected_stdout_writes`, and a bare `env!("VERGEN_…")` in a
file that failed to read went unreported by
`vergen_env_vars_are_read_optionally`, the guard standing between that
and another #3123.
`visit_files` returns how many files it visited, because an absence
assertion also passes over an empty walk. Three guards had nothing that
would fail in that case and now assert the count. The other three
already assert something an empty walk cannot satisfy — a non-empty
asset set, a per-root snapshot count, an offender list equal to one
named file — so a count beside those would be a second mechanism for one
guarantee.
The return is `#[must_use]`, which matters more than it looks. Left
advisory, the classification of who needs the count would live only in
prose, and a seventh caller would inherit none of it — the same shape as
the bug being fixed, a per-caller obligation that four of five copies
quietly skipped. As a `must_use` it is a compile error under `-D
warnings`, and "my proof is elsewhere" becomes a `let _ =` a reviewer
can see and check.
The `scan` label names the calling guard. It previously claimed to and
didn't: three call sites passed the same `"src/ scan"`, so a permissions
failure read identically whichever guard was running, and only libtest's
thread name told them apart. The file-contents panic was likewise worded
exactly like the directory-listing one, separated only by whether the
path happened to be a file.
One behaviour change worth naming: `output_system_guard`'s allowlist
checks now run against a file the walk has already read, rather than
short-circuiting before the read. An allowlisted file that can't be read
is a file in the tree that can't be read, and now says so instead of
being skipped.
`tests/CLAUDE.md` records the rule, beside "a test's setup is part of
its proof" — the same failure in different dress. Nothing wrote the
hazard down, which is why each guard was free to rediscover it.
The rule holds outside the source scans too, so two walks in
`config_show.rs`'s `test_plugin_layout_is_consolidated` now count what
they assert absence over. One walks `plugins/worktrunk/skills` asserting
no entry is a symlink — if the mirror never generated, it visits nothing
and passes; `is_dir()` runs first, but an existing empty directory is
exactly what a failed mirror leaves. The `skills/` loop below it has the
same shape for `SKILL.md`. Neither reads file contents, so `visit_files`
isn't a drop-in and they carry their own counts.
## Test plan
`cargo fmt --check` and `clippy --all-targets` clean; the 15 tests
across the guard files pass.
Green proves least here, since a walker that silently visits nothing
passes every one of those tests. So each behaviour was reached for
directly:
<details><summary>Reach checks</summary>
**The walk recurses and reads.** A `println!` injected three levels deep
in `src/trace/profile.rs`, a file on no allowlist:
```
src/trace/profile.rs:1306: println!("probe");
```
**The walk covers roots outside `src/`.** The `CARGO_BIN_EXE_wt` needle
injected into a file under `tests/`:
```
left: ["src/testing/mod.rs", "tests/integration_tests/list_layout.rs"]
```
**The coverage assert fires.** A scan pointed at a directory that
exists, reads fine, and holds no `.rs` files:
```
scanned no files under …/.github
```
**The per-root snapshot count fires on a partial move.** `tests` alone —
1,088 of the 1,156 snapshots — swapped for a directory holding none,
leaving `src`'s 64 and `docs`' 4 in place:
```
.github/ holds no .snap files — the corpus has moved, so these assertions are now passing over whatever is left of it
```
An aggregate non-empty check survives that case, which is why the count
is per root. Every edit was reverted after its run.
</details>
> _This was written by Claude Code on behalf of max-sixty_
67 lines
2.7 KiB
Rust
67 lines
2.7 KiB
Rust
//! One recursive walk for the guards that read source as text.
|
|
//!
|
|
//! Those guards assert *absence*, which is what makes a swallowed read
|
|
//! dangerous here in a way it isn't for code that acts on what it finds — see
|
|
//! "Guards that scan source text" in `tests/CLAUDE.md` for the shape of that
|
|
//! failure and the rules it sets.
|
|
//!
|
|
//! Every read below panics rather than skipping, so the walk either covers the
|
|
//! tree or says which read stopped it.
|
|
|
|
use std::fs;
|
|
use std::path::Path;
|
|
|
|
/// Visit every `.{extension}` file under `dir`, recursively, passing each
|
|
/// file's path and contents to `f`.
|
|
///
|
|
/// `scan` names the calling guard in panic messages (`"stdout scan"`,
|
|
/// `"snapshot scan"`), so a failure says which guard was running — several
|
|
/// guards walk the same tree, so the path alone doesn't. The three reads that
|
|
/// can fail are worded apart: listing a directory, reading one entry of a
|
|
/// listed directory, and reading a file's contents. The entry case is the one
|
|
/// that most needs saying, because the directory it names checks out healthy
|
|
/// and the obvious next step leads nowhere.
|
|
///
|
|
/// Returns the number of files visited. It is `#[must_use]` because an
|
|
/// absence-asserting guard also passes over an empty walk, so every caller has
|
|
/// to answer for coverage — either by asserting this count, or by discarding it
|
|
/// with `let _ =` where it already asserts something an empty walk cannot
|
|
/// satisfy. Leaving that to prose is how the swallowed reads this walk replaced
|
|
/// survived in four copies.
|
|
#[must_use]
|
|
pub fn visit_files(
|
|
dir: &Path,
|
|
extension: &str,
|
|
scan: &str,
|
|
f: &mut impl FnMut(&Path, &str),
|
|
) -> usize {
|
|
let mut visited = 0usize;
|
|
let entries = fs::read_dir(dir)
|
|
.unwrap_or_else(|e| panic!("{} unreadable during the {scan}: {e}", dir.display()));
|
|
|
|
for entry in entries {
|
|
// `flatten()` here would drop a per-entry error — a file removed
|
|
// mid-walk, a failing `stat` — and skip a file without saying so.
|
|
let entry = entry.unwrap_or_else(|e| {
|
|
panic!(
|
|
"an entry in {} unreadable during the {scan}: {e}",
|
|
dir.display()
|
|
)
|
|
});
|
|
let path = entry.path();
|
|
if path.is_dir() {
|
|
visited += visit_files(&path, extension, scan, f);
|
|
} else if path.extension().and_then(|s| s.to_str()) == Some(extension) {
|
|
let contents = fs::read_to_string(&path).unwrap_or_else(|e| {
|
|
panic!(
|
|
"the contents of {} are unreadable during the {scan}: {e}",
|
|
path.display()
|
|
)
|
|
});
|
|
f(&path, &contents);
|
|
visited += 1;
|
|
}
|
|
}
|
|
visited
|
|
}
|