//patches:audition reports success after writing nothing #69

New issue

Closed

opened 2026-09-05 23:17:49 +00:00 by bcox · 1 comment

bcox commented

2026-09-05 23:17:49 +00:00

Owner

bazel run //patches:audition -- /some/dir exits 0 when /some/dir does not
exist, having written no files. The only sign is a line on stdout per patch:

cannot write /some/dir/hello.wav: could not write /some/dir/hello.wav

...and then a clean exit. A caller checking the exit status -- a script, CI, or an
agent -- sees success.

This is the same shape as the --test_filter traps CLAUDE.md documents at length: a
green run that did nothing. It cost one wasted render cycle while verifying that branch
fm-followups left the factory bank byte-identical, and it would silently produce an
empty comparison for anyone doing the same.

Two fixes, either acceptable:

std::filesystem::create_directories on the output path before rendering.
Exit non-zero on the first write failure.

Probably both: create the directory, and still fail loudly if a write fails for another
reason (full disk, permissions).

While in there: the message says the same thing twice ("cannot write X: could not write
X"), and neither half carries the errno that would explain why.

`bazel run //patches:audition -- /some/dir` exits **0** when `/some/dir` does not exist, having written no files. The only sign is a line on stdout per patch: cannot write /some/dir/hello.wav: could not write /some/dir/hello.wav ...and then a clean exit. A caller checking the exit status -- a script, CI, or an agent -- sees success. This is the same shape as the `--test_filter` traps `CLAUDE.md` documents at length: a green run that did nothing. It cost one wasted render cycle while verifying that branch `fm-followups` left the factory bank byte-identical, and it would silently produce an empty comparison for anyone doing the same. Two fixes, either acceptable: - `std::filesystem::create_directories` on the output path before rendering. - Exit non-zero on the first write failure. Probably both: create the directory, and still fail loudly if a write fails for another reason (full disk, permissions). While in there: the message says the same thing twice ("cannot write X: could not write X"), and neither half carries the `errno` that would explain why.

bcox referenced this issue

2026-09-09 23:56:08 +00:00

//patches:audition --patch is not repeatable, so an A/B needs two invocations #88

bcox commented

2026-09-09 23:56:18 +00:00

Author

Owner

Fixed on worktree-audition-flags, together with #88, #63 and #81 — but the headline of this issue was already stale when it was picked up, and that is worth recording.

audition.cc already returns 1 on the first write failure (patches/audition.cc:218 before this branch). So the tool did not report success after writing nothing; it failed fast on the first patch, with no explanation. What was actually left:

The directory is now created, missing parents included — std::filesystem::create_directories, the first of the two fixes this issue proposed. It runs before the first render rather than at the first write, so a mistyped path costs no render at all.

The message says why, and says the path once. The old line named the path twice because resynth/audio_file.h:95 already puts the path in the error string ("could not write " + path) and the call site prefixed it with the same path again. The call site no longer does. Where the reason is available, it is carried:

audition: /tmp/kicks/blocker: File exists
exit=1

On errno, deliberately not: resynth/audio_file.h reports only that ma_encoder_init_file failed — miniaudio does not hand back an errno, so the write path has no reason to carry. Widening that header to invent one would change a subsystem with its own test for a message this tool prints, so it is left alone; the directory is the one write failure whose cause can actually be reported, which is why the reason lives there. Same posture as #85's report-rather-than-widen.

Covered by //patches:audition_options_test. The message shape is asserted structurally rather than by matching text: the error must start with the path and ": ", the remainder must not contain the path a second time, and the remainder must equal std::error_code::message() for the same failure — so a canned non-empty string cannot satisfy it. Verified by injecting an EnsureDirectory that accepts any existing path; only its own test failed.

Fixed on `worktree-audition-flags`, together with #88, #63 and #81 — but **the headline of this issue was already stale when it was picked up**, and that is worth recording. `audition.cc` already returns 1 on the first write failure (`patches/audition.cc:218` before this branch). So the tool did not report success after writing nothing; it failed fast on the first patch, with no explanation. What was actually left: **The directory is now created, missing parents included** — `std::filesystem::create_directories`, the first of the two fixes this issue proposed. It runs *before the first render* rather than at the first write, so a mistyped path costs no render at all. **The message says why, and says the path once.** The old line named the path twice because `resynth/audio_file.h:95` already puts the path in the error string (`"could not write " + path`) and the call site prefixed it with the same path again. The call site no longer does. Where the reason is available, it is carried: ``` audition: /tmp/kicks/blocker: File exists exit=1 ``` **On errno, deliberately not:** `resynth/audio_file.h` reports only that `ma_encoder_init_file` failed — miniaudio does not hand back an errno, so the write path has no reason to carry. Widening that header to invent one would change a subsystem with its own test for a message this tool prints, so it is left alone; the directory is the one write failure whose cause can actually be reported, which is why the reason lives there. Same posture as #85's report-rather-than-widen. Covered by `//patches:audition_options_test`. The message shape is asserted structurally rather than by matching text: the error must start with the path and `": "`, the remainder must not contain the path a second time, and the remainder must equal `std::error_code::message()` for the same failure — so a canned non-empty string cannot satisfy it. Verified by injecting an `EnsureDirectory` that accepts any existing path; only its own test failed.

bcox referenced this issue

2026-09-09 23:56:25 +00:00

patches/audition.cc: stale bank count in the allocation comment #63

bcox referenced this issue

2026-09-09 23:56:35 +00:00

CLAUDE.md says 91 test targets; bazel query says 98, and did before the last branch #81

bcox referenced this issue from a commit

2026-09-10 00:09:21 +00:00

Make --patch repeatable; create the output directory it names

bcox closed this issue

2026-09-10 00:09:21 +00:00