[max-sixty]

print_json — the pretty-JSON printer behind --format=json — lived in src/commands/list/mod.rs, under the one command that happened to need it first, while wt list statusline and wt config approvals list reached across for it. It now lives in src/output/json.rs, re-exported as crate::output::print_json.

Moving it surfaced thirty other call sites that had each open-coded the same two lines, and that those two lines were not equivalent everywhere. Four printed through anstream's println! (via worktrunk::styling), the other twenty-six through std's. anstream drops a BrokenPipe write error; std panics. So on main today:

$ wt config state get --format=json | head -3
{
  "ci_status": [
    {
thread 'main' panicked at library/std/src/io/stdio.rs:1165:9:
failed printing to stdout: Broken pipe (os error 32)

while wt config show --format=json | head -3 exits cleanly — same command family, different behavior, decided by which macro happened to be imported in that file. EPIPE depends on whether a reader is still attached, not on how much is written, so this is a race rather than a size threshold: an output that fits the pipe buffer usually lands before the consumer goes away, which is why it stayed hidden rather than why it is safe. A three-byte write panics just as readily once the reader has closed.

All thirty-six call sites now go through print_json, and print_json prints through anstream, so no --format=json surface panics on a closed pipe. The command above now exits 0 with an empty stderr; also checked by hand on wt list, wt step prune --dry-run, wt config show, wt step eval, wt config approvals list, and wt list statusline.

wt switch --format=json is the one surface that isn't a print_json caller: it emits its single result as one compact line, which is what a shell loop reads, so converting it would change a machine-readable format for no gain. It gets the same anstream printer instead, which is the part that matters here.

wt list statusline --format=json had a third instance hiding in its schema-1 empty path, a bare println!("[]") on std's macro — on the surface that runs on every prompt redraw. It now serializes an empty array through print_json, which emits the same two bytes; test_statusline_json_outside_worktree already asserted them. A module-level println import would have been wrong there, since the text statusline's println! deliberately bypasses anstream so its pre-rendered ANSI survives a non-tty stdout.

Pruned eight now-dead entries from STDOUT_ALLOWED_PATHS in tests/integration_tests/output_system_guard.rs. Centralizing these writes left seven allowlisted files with no direct println! at all, so their exemptions covered no code and the guard quietly stopped protecting exactly the files this branch touched; a stray println! in merge.rs or remove.rs fails the test again. (alias.rs's entry was already dead.)

Serialization is otherwise byte-identical: serde escapes control characters, so raw ANSI never reaches this output and anstream's stripping is a no-op on it.

src/commands/CLAUDE.md gains one line under "Adding a CLI Command" stating the rule — stdout through anstream, print_json as the printer, switch's compact line as the one shape difference — so the next JSON surface doesn't open-code it again.

Two print_json calls the diff rewrote were uncovered on the base commit, which pulled their misses into codecov/patch without any behavior changing. Rather than hand over a waiver, both now have tests: wt remove --format=json with no branch argument (the single-worktree path, distinct from the named removals already tested) and wt step for-each --format=json interrupted mid-loop, which still owes its consumer the results collected before the signal. Each was mutation-checked — deleting the line under test fails it — because a one-item array is what both the abort and completion paths produce, so a weaker assertion would pass either way.

This was written by Claude Code on behalf of max-sixty

[worktrunk-bot]

Two findings, then why this is a comment rather than an approval.

STDOUT_ALLOWED_PATHS has seven entries that just went dead. Routing these files through print_json removed their last direct println!, so the matching exemptions in tests/integration_tests/output_system_guard.rs (the STDOUT_ALLOWED_PATHS const — the entries commented "JSON output for wt list --format=json", "…wt merge…", "…wt remove…", "…wt step for-each…", "…wt config show…", "…wt step relocate…", and "Dry-run cache inventory dump") now cover no code at all: list/mod.rs, merge.rs, remove.rs, for_each.rs, config/show.rs, step/relocate.rs, and picker/mod.rs each contain zero println!/print! after this change. The guard quietly stops protecting exactly the seven files whose stdout writes this PR centralized, so a future accidental println! in merge.rs or remove.rs passes it silently. (alias.rs's entry is dead too, but that predates this PR.)

Worth pairing the prune with the positive rule while the invariant is fresh: the thing this PR wants to stay fixed is serde_json::to_string_pretty + println! reappearing under src/commands/, and src/output/json.rs now sits outside the tree that guard scans, which makes it the one sanctioned writer — expressible as a second check in the same test. That also seems like the cheaper protection than a real broken-pipe integration test, which is hard to make deterministic: with a sub-buffer payload the child's write usually lands before the reader goes away. (If you do try one, it has to spawn the built binary — anstream::println! delegates to ::std::println! under cfg!(test), so an in-process test can't reproduce the fix either way.) Happy to push a commit with the prune plus the guard rule if you want it.

wt switch --format=json is the one surface that didn't get converted. emit_switch_json in src/commands/worktree/switch.rs still open-codes serde_json::to_string(&json) followed by println!("{json}"), and switch.rs's worktrunk::styling import brings in eprintln but not println — so that's std's macro, the panicking one. It's compact rather than pretty, so it isn't a print_json drop-in, but it means "every --format=json surface tolerates a consumer that stops reading" isn't quite true yet, and the CLAUDE.md line this PR adds doesn't describe it. Not a live panic — a single switch result never fills the pipe buffer, same as the schema-1 empty-path println!("[]") in statusline.rs — so this is about the map matching the territory. Suggestion inline on the doc line; routing emit_switch_json through print_json instead (and dropping the caveat) is the other way to close it, at the cost of switching that output from one line to pretty.

Not approving: the diff edits src/commands/remove.rs, src/commands/step/prune.rs, and src/commands/picker/mod.rs, all on the removal path, so per this repo's review policy the merge call is a human's — even though the edits there are confined to the JSON print and can't reach deletion behavior.

[worktrunk-bot]

One surface left, and a correction to what I said last time.

wt list statusline --format=json still writes through std's println!. run_json's print_empty closure emits println!("[]") on the schema-1 branch, and statusline.rs's worktrunk::styling import brings in fix_dim_after_color_reset, terminal_width_for_statusline, and truncate_visible — no println — so that's std's macro. It's the last --format=json write left on the panicking path, on the one surface that runs on every prompt redraw.

Correcting myself, because it's what makes this a bug rather than a doc nit: I wrote last time that the switch and statusline cases were "not a live panic — a single result never fills the pipe buffer." That's wrong. EPIPE turns on whether a reader is still attached, not on how much you write — a three-byte println!("[]") panics with failed printing to stdout: Broken pipe (os error 32) the moment the consumer has closed, which I verified with a standalone binary rather than reasoning about it a second time.

The fix wants to be one qualified call, not a module-level import: importing println into statusline.rs would also route the text statusline's println!("{}", output) through anstream, and that one deliberately bypasses it — "Output uses println! for raw stdout (bypasses anstream color detection)" — so its pre-rendered ANSI survives a non-tty stdout.

worktrunk::styling::println!("[]");

Same qualified form config/show.rs carried before this PR. It also makes the new src/commands/CLAUDE.md line true as written — as it stands, wt switch --format=json isn't the only exception the line claims it is. Happy to push it.

Not approving, unchanged from last time: the diff edits src/commands/remove.rs, src/commands/step/prune.rs, and src/commands/picker/mod.rs, all on the removal path, so per this repo's review policy the merge call is a human's.

[max-sixty]

Both findings verified against head and fixed in 270d063.

Dead guard entries. Confirmed by replicating the guard's own line logic over each allowlisted path: eight entries matched zero lines — the seven you named plus alias.rs. Pruned all eight, including the pre-existing one. The prune is the enforcement, so I skipped the extra positive rule: with merge.rs, remove.rs, for_each.rs, config/show.rs, step/relocate.rs, picker/mod.rs, and list/mod.rs off the allowlist, a reappearing println!("{}", serde_json::to_string_pretty(…)?) in any of them fails check_no_stdout_in_commands on the println! alone — a second check keyed on to_string_pretty would be a second mechanism for the same guarantee. It would still add something for files allowlisted on other grounds (config/state.rs, step/prune.rs, step/copy_ignored.rs), which is worth doing separately if you think that gap is live.

emit_switch_json. Confirmed: serde_json::to_string plus std's println!, with no println in switch.rs's worktrunk::styling import. I took neither branch of your suggestion exactly. Converting it to print_json turns a machine-readable one-liner into pretty JSON, which is a format change for consumers that read a line — the compact shape is the right one for a command reporting a single result. But the caveat alone would leave the panic shape in place, and unreachable-in-practice isn't the same as unreachable: it's a race with the reader, not a function of payload size.

So switch.rs now imports anstream's println! and keeps its compact line. That closes the panic on the last surface without touching the format, and it makes the shape the only thing switch does differently.

The doc line reads as the rule with switch falling out of it, rather than as an exception to it:

A --format=json mode prints its answer to stdout through anstream's println!, which drops a BrokenPipe write error where std's panics. crate::output::print_json is that printer for every surface but wt switch --format=json, which emits its single result as one compact line.

Agreed on the broken-pipe integration test, and thanks for the cfg!(test) detail — anstream delegating to ::std::println! under cfg!(test) means an in-process test asserts the opposite of the fix. Not adding one.

On the removal-path files: remove.rs, step/prune.rs, and picker/mod.rs are edited only where they serialize their JSON answer, and the guard prune is a test-file change. Flagging the human merge call to the repo owner along with the codecov/patch arithmetic — its two misses are pre-existing lines this diff relocated.

This was written by Claude Code on behalf of max-sixty

[worktrunk-bot]

Both first-round findings check out at 270d063 — eight allowlist entries gone, and switch.rs's worktrunk::styling import now carries println, so its compact line goes through anstream. Agreed on skipping the to_string_pretty check for the pruned files; the prune is the stronger enforcement there.

Two things your replies don't cover, both from the second review (posted 00:31, ~30 min before them) rather than the first.

statusline.rs still has one. println!("[]") on the schema-1 branch of run_json's print_empty — statusline.rs's styling import brings in fix_dim_after_color_reset, terminal_width_for_statusline, and truncate_visible, no println, so that's std's. I swept src/commands/ at head: it's the only --format=json write left on std's macro. Payload size doesn't save it — EPIPE turns on whether a reader is still attached, not on how much you write.

The fix wants to be one qualified call rather than a module-level import, since line 848's println!("{}", output) deliberately bypasses anstream so its pre-rendered ANSI survives a non-tty stdout:

worktrunk::styling::println!("[]");

That also makes the new CLAUDE.md line true as written — as it stands, the printer rule isn't absolute and wt switch isn't the only exception. Happy to push it if you'd rather not.

On the gap you asked about (config/state.rs, step/prune.rs, step/copy_ignored.rs — allowlisted on other grounds): it's live, and statusline.rs is the instance. A file allowlisted for its text stdout can grow a JSON write that nothing checks, which is what happened there. A to_string_pretty-keyed rule wouldn't have caught this particular one, though, since the payload is a literal "[]" — so if you do add a check later, keying it on the printer rather than the serializer is what would have flagged it.

codecov/patch. Your read is right; here's the arithmetic. Both misses are lines this diff rewrote in place and that were already uncovered on bdd7113c9.

Verified against the Codecov compare API

GET /api/v2/github/max-sixty/repos/worktrunk/compare/?base=bdd7113c95af199346feb2c02643ad051d880c03&head=270d0635a239e852a22de2dd74f8202db44e7386

Summing .files[] | select(.has_diff) | .totals.patch: 39 patch lines, 37 hits, 2 misses — 94.87%. The two:

head	line	base line	base coverage
src/commands/remove.rs	368 print_json(&json)?;	368 (println!("{}", serde_json::to_string_pretty(&json)?))	miss
src/commands/for_each.rs	154 print_json(&json_results)?;	153 (same shape, the interrupted branch)	miss

Base coverage from file_report/<path>?sha=bdd7113c9… — line_coverage has remove.rs:368 and for_each.rs:153 as misses on the merge-base. for_each.rs shifts by one because the use crate::output::print_json; line is an addition; remove.rs doesn't because its import line was a replacement.

The remaining sixteen files with patch lines are all 100%.

Unchanged on approval: the diff touches remove.rs, step/prune.rs, and picker/mod.rs on the removal path, so the merge call is yours either way.

[max-sixty]

Verified and fixed in bb351ba — and thank you for the correction, which was the load-bearing part. I had the same wrong reasoning in the PR description ("payloads under the 64KB pipe buffer never reach the failing write"); it's now corrected there too. The size effect is real but it only shifts the odds of the race — it decides whether the write lands before the consumer goes away, not whether EPIPE is possible.

I took a different fix from the one you suggested. Rather than worktrunk::styling::println!("[]"), the schema-1 empty path now goes through print_json(&serde_json::json!([])). to_string_pretty renders an empty array as the same two bytes, so the output is unchanged — test_statusline_json_outside_worktree's assert_eq!(output.trim(), "[]") passes untouched, which is what proves it rather than my reading of serde's formatter.

The reason to prefer it: your version leaves wt list statusline --format=json as a second surface the src/commands/CLAUDE.md line has to carve out, and you're right that the line was false as written. Routing it through print_json removes the exception instead of documenting it, so the line is true again with wt switch --format=json's compact shape as the only difference.

Your caveat about the module-level import was exactly right and is why this is a call-site change either way: statusline.rs:848 bypasses anstream deliberately, documented at line 726, and importing println would have silently rerouted the text statusline's pre-rendered ANSI.

src/commands/statusline.rs now has one println! left, the text one at 848, and it is not on a --format=json path.

Understood on the removal-path policy — I'm taking the merge call to the repo owner along with the codecov/patch arithmetic, not waiting on an approval from you.

This was written by Claude Code on behalf of max-sixty

[worktrunk-bot]

Last finding is addressed. Routing the schema-1 empty branch through print_json is the better of the two fixes — it leaves the text statusline's deliberate std println! alone, and serde_json::to_string_pretty renders an empty array as the same bare [] that test_statusline_json_outside_worktree asserts.

I also checked the STDOUT_ALLOWED_PATHS entries that survive and still carry a --format=json path — step/prune.rs, step/copy_ignored.rs, config/state.rs, hook_commands.rs, eval.rs. Each imports worktrunk::styling::println at module level, so their bare println! calls are anstream's too. statusline.rs was the one file where the guard's file-level granularity could hide a std write on a JSON path, and it no longer does.

Still not approving, unchanged: the diff edits src/commands/remove.rs, src/commands/step/prune.rs, and src/commands/picker/mod.rs, all on the removal path, so per this repo's review policy the merge call is a human's.

[worktrunk-bot]

One finding on the new remove test, plus a note on the for-each one.

test_remove_json_current_worktree_no_args will fail on Windows. It's the fifth test in remove.rs to run a --foreground removal of the current worktree from inside it, and the other four are all #[cfg_attr(windows, ignore)] with the same reason attached — e.g. on test_remove_main_worktree_vs_linked_worktree: "Tests run as subprocesses which can't change directory via shell integration. Real users are fine — shell integration cds to main before removing. But subprocess tests stay in the worktree, causing Windows file locking errors." That still holds at head: src/ has no set_current_dir anywhere, so the wt process deleting the worktree is cwd'd inside it, and Windows won't unlink a directory that's a live process's cwd.

The background counterpart is the reason the gate isn't blanket: test_remove_from_detached_head_in_worktree also removes the current worktree from within and is ungated, because without --foreground the deletion happens in the detached child. Your test needs --foreground for the JSON to be deterministic, so it lands on the gated side. Suggestion inline.

test (windows) is still pending on this head, so this is from the file's own pattern rather than a CI failure I've watched go red.

Nothing on the for-each test's substance — the SIGTERM path pushes exactly one result before break, so items.len() == 1 is deterministic, and deleting the target print_json leaves stdout empty and fails the parse.

[worktrunk-bot]

The Windows finding is closed, and the fix you took is the better of the two.

Dropping --foreground moves the test to the ungated side of the same contrast the file already draws — test_remove_from_detached_head_in_worktree removes the current worktree from within and runs on Windows, while test_remove_foreground_detached_head carries #[cfg_attr(windows, ignore)] for exactly the cwd-lock reason. Keeping the assertion running on Windows beats skipping it there, which is what my suggestion would have done.

I checked the determinism claim rather than taking the commit message for it, since my last review asserted the opposite. It holds: remove.rs's single-worktree branch calls print_json in the parent, immediately after handle_remove_output returns, and the fate it serializes comes back synchronously — spawn_background_removal runs execute_instant_removal_or_fallback (rename-into-trash plus the branch deletion) in-process and only spawns a detached child for the directory cleanup. The mode changes who unlinks the directory, not who computes or prints the result. test_remove_json_branch_outcome_reflects_execution is the existing proof of the same thing: backgrounded, --format=json, and it asserts branch_outcome down to retained_unmerged. Everything on the current-worktree path that background mode adds — prepare_remove_directory_change, print_switch_message_if_changed — writes to stderr, so stdout stays parseable. test (windows) hasn't started on this head yet, so that's from the code rather than from a green run I watched.

The one open thread left is the for_each.rs marker dir, which was non-blocking and still is. Not approving is unchanged and unrelated to any of the above: the diff edits src/commands/remove.rs, src/commands/step/prune.rs, and src/commands/picker/mod.rs on the removal path, so the merge call stays yours.

[worktrunk-bot]

Last open thread is closed, and codecov/patch has moved since the arithmetic I handed over.

The for-each simplification holds. I checked the mutation claim from the code rather than the commit message, since the assertion changed shape. Deleting for_each.rs:154 leaves the interrupted branch returning Err(Interrupted) before the completion path's own print_json at line 166 can run, so stdout is empty and serde_json::from_str fails the test — the surviving print_json isn't a fallback for it. And items.len() == 1 stays a real assertion without the marker dir: the standard fixture carries main plus feature-{a,b,c}, noted in the sibling test two above, so a loop that didn't abort would report four.

codecov/patch is green on this head — 100.00% of diff hit, target 98.11%. My 94.87% / two-misses arithmetic was measured on 270d0635a, before the two tests in 54246cb77 landed; both lines it named are now covered, so the waiver conversation is moot and there's nothing to weigh at merge time on that axis.

test (windows) is the only check still running on f25692249; everything else is terminal and green, test (linux), test (macos), code-coverage, and lint included. The Windows question from my earlier review is already answered rather than pending on it: test (windows) went green on d492cc3aa, which is where test_remove_json_current_worktree_no_args moved to the ungated side of the cwd-lock contrast, and f25692249's only change sits inside a #[cfg(unix)] test.

Not approving, unchanged and unrelated to any finding — no review concerns are outstanding. The diff edits src/commands/remove.rs, src/commands/step/prune.rs, and src/commands/picker/mod.rs, all on the removal path, so per this repo's review policy the merge call is yours.