Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/commands/config/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use worktrunk::config::{
};
use worktrunk::git::Repository;
use worktrunk::styling::{
eprintln, format_bash_with_gutter, hint_message, info_message, print, println, success_message,
suggest_command_in_dir,
eprint, eprintln, format_bash_with_gutter, hint_message, info_message, print, println,
success_message, suggest_command_in_dir,
};

use crate::output::prompt::{PromptResponse, prompt_yes_no_preview};
Expand Down
11 changes: 9 additions & 2 deletions src/commands/for_each.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,16 @@
//!
//! For now, we keep `for-each` under `step` as a pragmatic choice.

use std::io::{Write as _, stderr};
use std::io::Write as _;
use std::process::Stdio;

use color_print::cformat;
use worktrunk::config::{UserConfig, VarScope};
use worktrunk::git::{ErrorExt, Repository, WorktreeInfo, WorktrunkError};
use worktrunk::shell_exec::{Cmd, ShellEscapeMode};
use worktrunk::styling::{
eprintln, error_message, format_with_gutter, progress_message, success_message, warning_message,
eprint, eprintln, error_message, format_with_gutter, progress_message, stderr, success_message,
warning_message,
};

use crate::commands::command_executor::{CommandContext, build_hook_context};
Expand Down Expand Up @@ -220,6 +221,12 @@ fn run_argv(
argv: Vec<String>,
stdin_json: &str,
) -> anyhow::Result<()> {
// Reset ANSI codes on stderr so our color doesn't bleed into the child's
// output, the same three lines `execute_shell_command` runs before its own
// spawn. Both `eprint!` and `stderr` are anstream's: the flushes have to
// name the stream the reset was written to, and on a redirected stderr
// anstream drops the reset rather than writing a literal `ESC[0m` into the
// file.
stderr().flush()?;
eprint!("{}", anstyle::Reset);
stderr().flush().ok();
Expand Down
4 changes: 2 additions & 2 deletions src/commands/list/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ use anstyle::Style;
use model::{BranchScope, ItemKind, ListData, ListItem};
use progressive::RenderTarget;
use worktrunk::git::Repository;
use worktrunk::styling::INFO_SYMBOL;
use worktrunk::styling::{INFO_SYMBOL, eprintln};

use crate::output::print_json;

Expand Down Expand Up @@ -215,7 +215,7 @@ pub(crate) fn resolve_json_schema(repo: &Repository) -> u8 {
use std::sync::Once;

use color_print::cformat;
use worktrunk::styling::{eprintln, hint_message, warning_message};
use worktrunk::styling::{hint_message, warning_message};

static WARNED: Once = Once::new();
match repo.config().list.json_schema {
Expand Down
2 changes: 1 addition & 1 deletion src/config/deprecation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use shell_escape::unix::escape;
use crate::config::WorktrunkConfig;
use crate::shell_exec::Cmd;
use crate::styling::{
eprintln, format_with_gutter, hint_message, info_message, suggest_command_in_dir,
eprint, eprintln, format_with_gutter, hint_message, info_message, suggest_command_in_dir,
warning_message,
};

Expand Down
2 changes: 1 addition & 1 deletion src/output/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ use worktrunk::shell_exec::scrub_git_discovery_env_vars;
use worktrunk::shell_exec::{
DIRECTIVE_CD_FILE_ENV_VAR, DIRECTIVE_EXEC_FILE_ENV_VAR, RETIRED_DIRECTIVE_FILE_ENV_VAR,
};
use worktrunk::styling::{hint_message, warning_message};
use worktrunk::styling::{eprintln, hint_message, warning_message};

/// Issue tracking whether to relax the EXEC scrub further for project aliases
/// and hooks. (User aliases already pass EXEC through.) Emitted in the warning
Expand Down
4 changes: 2 additions & 2 deletions src/output/prompt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use std::io::{self, Write};

use color_print::cformat;
use worktrunk::styling::PROMPT_SYMBOL;
use worktrunk::styling::{PROMPT_SYMBOL, eprint, eprintln};

/// Response from a `[y/N/?]` prompt.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Expand Down Expand Up @@ -42,7 +42,7 @@ pub fn prompt_yes_no_preview(
show_preview: impl Fn(),
) -> io::Result<PromptResponse> {
// Blank line before first prompt for visual separation
worktrunk::styling::eprintln!();
eprintln!();

loop {
eprint!(
Expand Down
212 changes: 211 additions & 1 deletion tests/integration_tests/output_system_guard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
//! This test enforces: **No accidental stdout writes in command code**
//!
//! Allowed:
//! - `eprintln!` / `eprint!` (stderr is safe)
//! - `eprintln!` / `eprint!` — stderr is not the answer stream, so writing to
//! it is safe; *which* macro of that name is in scope is a separate rule,
//! enforced by [`check_stderr_macros_come_from_styling`]
//! - `println!` / `print!` in files listed in `STDOUT_ALLOWED_PATHS`
//!
//! When adding stdout output:
Expand All @@ -21,7 +23,14 @@
//!
//! Neither printer panics when the consumer closes the pipe, which
//! [`test_stdout_surfaces_survive_a_closed_consumer`] checks from the outside.
//!
//! The stderr counterpart is two tests: [`check_stderr_macros_come_from_styling`]
//! requires every bare `eprint!`/`eprintln!` under `src/` to resolve to
//! anstream's, statically and for every site; and
//! [`test_stderr_narration_strips_ansi_when_piped`] proves from the outside
//! what that buys — narration redirected to a file carries no escapes.

use std::collections::HashSet;
use std::fs;
use std::path::Path;
use std::process::Stdio;
Expand Down Expand Up @@ -185,6 +194,169 @@ fn check_file(path: &Path, tokens: &[&str], violations: &mut Vec<String>, comman
}
}

/// Paths (relative to `src/`) allowed to reach std's `eprint!`/`eprintln!`.
///
/// Every other file that writes to stderr must have the macro of that name in
/// scope from `worktrunk::styling`, or qualify the call — see
/// [`check_stderr_macros_come_from_styling`].
///
/// An entry exempts the **whole file**, not the call its comment names, so an
/// entry added for one narrow site also covers whatever that file grows later.
/// Keep the list to files where std's macro is right throughout.
const STD_STDERR_ALLOWED_PATHS: &[&str] = &[
// Relays a stub's captured stderr verbatim; the bytes are the fixture's
// data, so stripping their escapes would change what the test replays.
"testing/mock_stub.rs",
// A `#[cfg(test)]` skip diagnostic, not narration a user ever redirects.
"remove_dir.rs",
];

/// Narration on stderr goes out through anstream, at every site.
///
/// `worktrunk::styling`'s `eprint!`/`eprintln!` are anstream's and strip ANSI
/// when stderr isn't a terminal; std's prelude macros of the same name keep it.
/// The two are one import apart and indistinguishable at the call site, so a
/// file that imported `eprintln` but not `eprint` printed a warning and the
/// hint beneath it through different printers — one line of `wt list 2>log`
/// carrying escapes and the next not.
///
/// No runtime test can cover this: it is a property of every stderr write in
/// the binary, and the suite's own `CLICOLOR_FORCE=1` forces color on both
/// printers, so a snapshot agrees no matter which macro is in scope. Hence a
/// static scan. A call may satisfy it either way — importing the macro from
/// `styling`, or qualifying the call as `styling::eprintln!(…)`.
#[test]
fn check_stderr_macros_come_from_styling() {
let src_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("src");
let mut violations = Vec::new();
scan_stderr_macros(&src_dir, &src_dir, &mut violations);
violations.sort();

assert!(
violations.is_empty(),
"stderr writes that resolve to std's macro instead of anstream's:\n\n{}\n\n\
std's `eprint!`/`eprintln!` keep ANSI escapes when stderr is redirected, so a\n\
file mixing them with anstream's emits color on some lines of a log and not others.\n\
Import the macro from `worktrunk::styling`, qualify the call as\n\
`worktrunk::styling::eprintln!(…)`, or add the file to STD_STDERR_ALLOWED_PATHS\n\
with a comment explaining why std's macro is the right one there.",
violations.join("\n")
);
}

fn scan_stderr_macros(dir: &Path, src_dir: &Path, violations: &mut Vec<String>) {
let entries = match fs::read_dir(dir) {
Ok(e) => e,
Err(_) => return,
};

for entry in entries.flatten() {
let path = entry.path();
if path.is_dir() {
scan_stderr_macros(&path, src_dir, violations);
} else if path.extension().and_then(|s| s.to_str()) == Some("rs") {
check_stderr_macros_in_file(&path, src_dir, violations);
}
}
}

fn check_stderr_macros_in_file(path: &Path, src_dir: &Path, violations: &mut Vec<String>) {
let relative_path = path
.strip_prefix(src_dir)
.map(|p| p.to_slash_lossy())
.unwrap_or_default();
if STD_STDERR_ALLOWED_PATHS.contains(&relative_path.as_ref()) {
return;
}

let contents = match fs::read_to_string(path) {
Ok(c) => c,
Err(_) => return,
};
let imported = styling_imports(&contents);

for (line_num, line) in contents.lines().enumerate() {
// A doc comment quoting `eprintln!` is prose, not a write.
let code = match line.find("//") {
Some(pos) => &line[..pos],
None => line,
};

for macro_name in ["eprint", "eprintln"] {
if imported.contains(macro_name) {
continue;
}
let token = format!("{macro_name}!");
for (pos, _) in code.match_indices(&token) {
// `eprintln!` never matches the `eprint!` token — the `!` is
// part of it — so only a leading identifier char can be a
// false positive here.
let preceded_by_ident = pos > 0
&& (code.as_bytes()[pos - 1].is_ascii_alphanumeric()
|| code.as_bytes()[pos - 1] == b'_');
// A qualified `styling::eprintln!(…)` picks anstream's
// regardless of what the file imported.
if preceded_by_ident || code[..pos].ends_with("styling::") {
continue;
}
violations.push(format!(
"src/{relative_path}:{}: {}",
line_num + 1,
line.trim()
));
}
}
}
}

/// Names a file imports from `worktrunk::styling` / `crate::styling`.
///
/// Only `use` statements count. A qualified call site (`styling::eprintln!(…)`)
/// carries the same `styling::` prefix but binds nothing, so counting it would
/// let a file's *bare* calls silently fall through to std's macro.
///
/// **Only *top-level* `use` statements count** — ones at column 0. The caller
/// tests coverage per file while Rust resolves imports per scope, so a
/// function-local `use worktrunk::styling::eprintln;` would otherwise mark
/// every other function in the file as covered when none of them is. Every
/// styling import in `src/` is top-level, so the narrower rule costs nothing;
/// a future function-local one fails this scan and gets hoisted.
Comment thread
worktrunk-bot marked this conversation as resolved.
Resolved
fn styling_imports(contents: &str) -> HashSet<String> {
Comment thread
worktrunk-bot marked this conversation as resolved.
Resolved
let mut imports = HashSet::new();
let mut statement = String::new();

for line in contents.lines() {
if statement.is_empty() && !(line.starts_with("use ") || line.starts_with("pub use ")) {
continue;
}
let trimmed = line.trim();
statement.push_str(trimmed);
if !trimmed.ends_with(';') {
continue;
}

// `use worktrunk::styling::{a, b as c};` or `use crate::styling::a;`
if let Some(pos) = statement.find("styling::") {
let tail = &statement[pos + "styling::".len()..];
let names = match tail.strip_prefix('{') {
Some(braced) => braced.split_once('}').map(|(inner, _)| inner).unwrap_or(""),
// The single-import form ends at the statement's `;`.
None => tail.trim_end_matches(';'),
};
for name in names.split(',') {
// `foo as bar` binds `bar`, which is not the macro this checks.
let name = name.trim().split(" as ").next().unwrap_or("").trim();
if !name.is_empty() {
imports.insert(name.to_string());
}
}
}
statement.clear();
}

imports
}

/// Every stdout surface exits cleanly when its consumer stops reading.
///
/// std's `print!`/`println!` panic on a `BrokenPipe`, so a command whose
Expand Down Expand Up @@ -240,3 +412,41 @@ fn test_stdout_surfaces_survive_a_closed_consumer(repo: TestRepo) {
);
}
}

/// Narration on stderr strips its color when stderr isn't a terminal.
///
/// The stdout rule above has a stderr counterpart, and it is the same mistake:
/// `worktrunk::styling`'s `eprint!` is anstream's and strips ANSI off a
/// non-tty, std's prelude macro of the same name keeps it. A file that
/// imported `eprintln` from `styling` but not `eprint` got one of each, so a
/// deprecation warning and the hint printed directly beneath it disagreed
/// about whether `wt list 2>log` should carry escapes.
///
/// The suite's own `CLICOLOR_FORCE=1` is what hides this — it forces color on
/// both printers, so every snapshot agrees no matter which macro is in scope.
/// This test clears it and sets `NO_COLOR`, which only anstream honors.
///
/// The trigger is a deprecated `[ci]` block, whose warning is emitted by the
/// pre-deserialization deprecation pass on every command that loads config.
#[rstest]
fn test_stderr_narration_strips_ansi_when_piped(repo: TestRepo) {
repo.write_project_config("[ci]\nplatform = \"github\"\n");

let output = repo
.wt_command()
.arg("list")
.env_remove("CLICOLOR_FORCE")
.env("NO_COLOR", "1")
.output()
.expect("failed to run wt list");

let stderr = String::from_utf8_lossy(&output.stderr);
assert!(
stderr.contains("deprecated"),
"expected the [ci] deprecation warning on stderr; got: {stderr:?}"
);
assert!(
!stderr.contains('\x1b'),
"wt narration redirected to a file must not contain ANSI escapes; got: {stderr:?}"
);
}
Loading