Skip to content

skills(writing-user-outputs): route --format=json through print_json - #3750

Open
worktrunk-bot wants to merge 3 commits into
mainfrom
nightly/clean-31079892636
Open

skills(writing-user-outputs): route --format=json through print_json#3750
worktrunk-bot wants to merge 3 commits into
mainfrom
nightly/clean-31079892636

Conversation

@worktrunk-bot

Copy link
Copy Markdown
Collaborator

Nightly sweep finding: the writing-user-outputs skill 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=json call sites with crate::output::print_json, precisely because the hand-rolled two-liner picked std's println! twenty-six times out of thirty-six and std panics on BrokenPipe where anstream drops it. src/commands/CLAUDE.md picked up the new rule under "Adding a CLI Command"; the skill did not.

The concrete failure mode is that the next --format=json surface follows the skill, hand-rolls println!("{}", serde_json::to_string_pretty(&v)?), and reintroduces wt … --format=json | head panicking. The output_system_guard allowlist pruning in #3746 catches a stray println! in the files that PR touched, but not a new file that adds itself to STDOUT_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=json answers go through print_json, with wt switch --format=json named 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_guard to reject serde_json::to_string_pretty paired with println! under src/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.

@worktrunk-bot worktrunk-bot added the nightly-cleanup Issues found by nightly code quality sweep label Aug 6, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .claude/skills/writing-user-outputs/SKILL.md Outdated
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 worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .claude/skills/writing-user-outputs/SKILL.md Outdated
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nightly-cleanup Issues found by nightly code quality sweep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant