skills(writing-user-outputs): route --format=json through print_json - #3750
skills(writing-user-outputs): route --format=json through print_json#3750worktrunk-bot wants to merge 3 commits into
Conversation
worktrunk-bot
left a comment
There was a problem hiding this comment.
Self-authored, so this is a COMMENT rather than an approval.
The prose is accurate against the code — I checked print_json's anstream println! and its "pretty, one trailing newline" contract in src/output/json.rs, emit_switch_json's compact single line in src/commands/worktree/switch.rs, and that src/commands/statusline.rs imports only fix_dim_after_color_reset/terminal_width_for_statusline/truncate_visible from worktrunk::styling (so its text-path println! really is std's, while its --format=json path goes through the imported print_json).
One number is off. #3746's commit message distinguishes the total from the converted subset: "Moving it surfaced thirty other call sites that had each open-coded the same two lines … Four printed through anstream's println! …, the other twenty-six through std's", and separately "All thirty-six call sites now go through print_json". So thirty-six is the total after conversion; only thirty were open-coded, with the remaining six already calling the shared printer. #3746's diff agrees — 31 removed to_string_pretty lines, one of which is the definition moving out of src/commands/list/mod.rs. Since the whole point here is a skill that drifted from its evidence, the miscount is worth fixing on the way in.
Thirty-six is the post-#3746 total of `print_json` call sites, not the number that had open-coded the two lines. #3746 removed thirty-one `to_string_pretty` lines, one of which was the definition moving out of `src/commands/list/mod.rs`, so thirty call sites were converted; six already called the shared printer. Twenty-six stands as a subset of the thirty.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Self-authored, so COMMENT rather than approval.
Dropping "thirty-six" was right, and I re-verified the arithmetic behind it independently (6 pre-existing callers at 860fa02ec^, 31 removed to_string_pretty lines = 30 sites + the definition, 37 callers after, the extra one being statusline.rs's schema-1 empty path). That thread is resolved.
But the same sentence carries a second number from #3746's commit message that doesn't survive checking, and this time it's the one the sentence turns on. "Twenty-six of those picked std's macro" and the wt config state get --format=json | head -3 demonstration both fail against the imports at the base commit.
src/commands/config/state.rs imported anstream's macro before #3746 — its use block at 860fa02ec^ reads use worktrunk::styling::{eprintln, format_heading, format_with_gutter, hint_message, info_message, println, success_message, warning_message};, and all seven of its converted sites plus every other --format=json printer for wt config state … live in that file. So no wt config state surface was on std's macro, and the panic attributed to it can't have come from there. The clean half of the pair is right for a different reason than stated: config/show.rs had no module-level import and called worktrunk::styling::println! fully qualified.
Counting the same way across all thirty sites gives eleven on std's macro, not twenty-six — for_each.rs ×2, merge.rs, picker/mod.rs, remove.rs ×2, main.rs ×5. The other nineteen (hints.rs, show.rs, state.rs ×7, eval.rs, hook_commands.rs, copy_ignored.rs ×5, prune.rs ×2, relocate.rs) already had anstream's in scope, so they never panicked. That is still a real bug for the eleven and the fix is still right — it's the size of the blast radius and the worked example that are wrong.
The suggestion drops both numbers rather than swapping in "eleven". The rule doesn't rest on either, the sentence has now been wrong twice on a count, and a per-site tally is the part that drifts the next time a --format=json surface lands — which is the failure mode this PR exists to fix.
How I checked
Imports at the base commit:
$ git show 860fa02ec^:src/commands/config/state.rs | sed -n '81,84p'
use worktrunk::styling::{
eprintln, format_heading, format_with_gutter, hint_message, info_message, println,
success_message, warning_message,
};
$ git show 860fa02ec^:src/main.rs | grep -c 'use worktrunk::styling::.*[^e]println'
0
Per-file split of the thirty converted sites, from git show 860fa02ec -- src | awk '/^\+\+\+ b\//{f=substr($2,3)} /^-.*to_string_pretty/{print f}' | sort | uniq -c cross-referenced against each file's use worktrunk::styling::{…} group at 860fa02ec^. Every removed line is a bare println!, so module scope decides; config/show.rs is the one fully-qualified call.
That anstream's re-exported macro really does shadow the prelude — the premise the whole split rests on — I confirmed by building a standalone binary with a pub use anstream::println; re-export and two modules, one importing it and one not, then piping 20k lines through head -3:
std module → thread 'main' panicked at library/std/src/io/stdio.rs:1166:9:
failed printing to stdout: Broken pipe (os error 32)
anstream module → empty stderr, no panic
which is byte-for-byte the panic #3746 quotes — from a module that did not import the re-export. anstream 1.0.0's _macros.rs:143-148 swallows BrokenPipe and panics on every other error.
… example `src/commands/config/state.rs` imported anstream's `println!` at 860fa02^, and all seven of its converted sites are bare `println!` in that module scope — so no `wt config state` surface was on std's macro and the panic the sentence attributed to it can't have come from there. Counting module scope across the thirty converted sites gives eleven on std's macro (`for_each.rs` x2, `merge.rs`, `picker/mod.rs`, `remove.rs` x2, `main.rs` x5), not twenty-six; `config/show.rs` called `worktrunk::styling::println!` fully qualified. The rule rests on the mechanism, not on a per-site tally that drifts the next time a --format=json surface lands. State the mechanism.
Nightly sweep finding: the
writing-user-outputsskill drifted from the convention #3746 established yesterday.The skill is the load-before-editing gate for anything that writes user-visible output, and its "Printing output" block still labels a bare
println!as the way to emit JSON to stdout ("Primary output to stdout (tables, JSON, pipeable)"). #3746 replaced that at all thirty-six--format=jsoncall sites withcrate::output::print_json, precisely because the hand-rolled two-liner picked std'sprintln!twenty-six times out of thirty-six and std panics onBrokenPipewhere anstream drops it.src/commands/CLAUDE.mdpicked up the new rule under "Adding a CLI Command"; the skill did not.The concrete failure mode is that the next
--format=jsonsurface follows the skill, hand-rollsprintln!("{}", serde_json::to_string_pretty(&v)?), and reintroduceswt … --format=json | headpanicking. Theoutput_system_guardallowlist pruning in #3746 catches a strayprintln!in the files that PR touched, but not a new file that adds itself toSTDOUT_ALLOWED_PATHS.This adds two short paragraphs after the existing code block: which
println!is in scope decides whether a closed pipe panics (with the text statusline named as the deliberate std-macro exception), and--format=jsonanswers go throughprint_json, withwt switch --format=jsonnamed as the one non-caller and why. Every claim is taken from the code or from #3746's own commit message.Docs-only, so there is no regression test. A mechanical guard is possible — extending
output_system_guardto rejectserde_json::to_string_prettypaired withprintln!undersrc/commands/— but that is a new lint rather than a documentation fix, so I left it out; happy to add it if you'd prefer the enforcement over the prose.