From 0ff504bd78a2db7957b836b1fd4ee8bb50f373dc Mon Sep 17 00:00:00 2001 From: Nick DiZazzo Date: Sun, 13 Sep 2026 20:14:20 -0400 Subject: [PATCH 1/2] refactor(ci): retire the console-print ratchet for a plain output gate The console-print check no longer approves individual call sites. The allowlist file is deleted, `--regen` is gone, and any print macro in product code fails outright. A stale allowlist file reappearing is itself a failure, so a revert cannot quietly restore per-location approvals. Retiring the macros made `writeln!(io::stdout(), ..)` the obvious way to reintroduce the same debt, so the gate now also rejects direct `io::stdout()` / `io::stderr()` handles in product crates. Exemptions stay category rules: `CONSOLE_OUTPUT_OWNERS` names the files that implement the console output facility itself. A capability probe such as `io::stdout().is_terminal()` reads nothing and is not a handle. That new rule caught one real violation. `mesh_client::models::catalog` wrote its listing straight to stderr from a library crate; it now returns the rendered text and leaves the stream choice to the caller. Failure output points at the conversion patterns instead of a regen command. --- .../manage-ci/references/current-inventory.md | 24 +- AGENTS.md | 16 +- ci/ci.md | 24 +- crates/mesh-client/src/models/catalog.rs | 30 +- just/ci.just | 6 +- tools/xtask/data/console_print_allowlist.json | 1 - tools/xtask/src/main.rs | 2 +- tools/xtask/src/no_console_print.rs | 439 +++++++----------- tools/xtask/src/no_console_print/scope.rs | 33 ++ 9 files changed, 269 insertions(+), 306 deletions(-) delete mode 100644 tools/xtask/data/console_print_allowlist.json diff --git a/.agents/skills/manage-ci/references/current-inventory.md b/.agents/skills/manage-ci/references/current-inventory.md index ec7eb8509c..e18ed6dfc2 100644 --- a/.agents/skills/manage-ci/references/current-inventory.md +++ b/.agents/skills/manage-ci/references/current-inventory.md @@ -827,19 +827,27 @@ had different CPUs. See [retained evidence](../../../../ci/runtime-seed-evidence ## Console-print product scope -`just no-console-print` keeps exact file/line/macro approvals for product -sources. Its scope excludes test paths, parsed `#[cfg(test)]` modules, -examples, benches, auxiliary `src/bin/` targets and the explicit -`NON_PRODUCT_CRATES` list in `tools/xtask/src/no_console_print/scope.rs`. -Build scripts remain excluded because their output contains Cargo directives. -`mesh-llm/src/main.rs` and `mesh-client` remain in scope. +`just no-console-print` forbids the print macros and direct `io::stdout()` / +`io::stderr()` handles in product sources. There is no allowlist: every +exemption is a category rule, so no individual call site can be approved. Its +scope excludes test paths, parsed `#[cfg(test)]` modules, examples, benches, +auxiliary `src/bin/` targets and the explicit `NON_PRODUCT_CRATES` list in +`tools/xtask/src/no_console_print/scope.rs`. Build scripts remain excluded +because their output contains Cargo directives. `mesh-llm/src/main.rs` and +`mesh-client` remain in scope. + +The handle rule exempts only the files that implement the console output +facility, listed as `CONSOLE_OUTPUT_OWNERS` in the same module: the sink-aware +writer and its pre-sink CLI fallback, the inline progress renderers, the TUI +output manager / fd capture / terminal backend, the runtime tracing writer, +skippy-server's stderr telemetry sink, and the CLI presentation surfaces. +A capability probe such as `io::stdout().is_terminal()` reads nothing and is +not a handle. The gate checks Cargo metadata on every invocation and rejects an exempt crate that becomes a transitive normal dependency of `mesh-llm`, including optional and platform-specific dependencies. Tests cover that guard and the scope rules. The Quality workflow still invokes the same `just no-console-print` gate. -Direct stdout/stderr handle detection and deletion of the remaining product -ratchet belong to later stages of issue #1763. The CPU native runtime-event gate selects `family-qwen3-dense` from the `skippy-ci-smoke` manifest for both `pull-request` and `main` cadences. The diff --git a/AGENTS.md b/AGENTS.md index 7e4fc09f4b..5c4dc0eb91 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -585,13 +585,15 @@ Do not rerun otherwise unchanged validation solely because a commit is about to - Format Rust files in a way that preserves the owning crate's edition metadata. Prefer `cargo fmt -p -- path/to/file.rs` for a narrow edit, or `cargo fmt --all` when changes span packages. Do not use `cargo fmt --all -- path/to/file.rs`: workspace-level file arguments can be parsed without the owning crate's Rust 2024 edition metadata and fail on let-chains. - If you must invoke `rustfmt` directly on a standalone file, pass the edition resolved from that manifest lookup, for example `--edition 2024` for the current workspace default; otherwise use `cargo fmt` through the owning package. - Before committing Rust changes, ensure the formatting check passes with `cargo fmt --all --check`. -- After Rust changes, run `just no-console-print`. The allowlist records source - locations, so adding or removing unrelated lines can move an existing - approved occurrence and invalidate the ratchet. If the check reports only - moved existing occurrences, regenerate it with - `cargo run -p xtask -- repo-consistency no-console-print --regen`, review the - allowlist diff to confirm that no new console prints were approved, and - commit the regenerated allowlist with the source change. +- After Rust changes, run `just no-console-print`. It forbids `println!` / + `eprintln!` / `print!` / `eprint!` and direct `io::stdout()` / `io::stderr()` + handles in product code. There is no allowlist and no way to approve an + individual call site: route operational output through + `mesh_llm_events::emit_event` or `tracing`, human-facing CLI prose through + `mesh_llm_events::console_out` / `console_err`, and a `--json` command's + payload through `mesh_llm_events::machine_out`. Only the console output + facility itself may hold a terminal handle; that roster is + `CONSOLE_OUTPUT_OWNERS` in `tools/xtask/src/no_console_print/scope.rs`. - After Rust changes, run `cargo check` and `cargo clippy --all-targets -- -D warnings` for each touched crate (`-p `), and at least `cargo check -p mesh-llm` plus `cargo clippy -p mesh-llm --all-targets -- -D warnings` if the change is reachable from the shipped binary. - Treat Clippy as a required local gate, not a CI-only cleanup step. `cargo check`, `just build`, and formatter success do not catch lints such as `clippy::collapsible-if`; run the warning-denying Clippy command before opening or updating a PR. - If you touched tests, public APIs, routing, inference, gossip, plugin protocol, skippy ABI, or CLI behavior, run the relevant tests before committing. diff --git a/ci/ci.md b/ci/ci.md index aac9fca041..40e877e5ad 100644 --- a/ci/ci.md +++ b/ci/ci.md @@ -944,19 +944,27 @@ preserve the basis beyond remote artifact expiry. ## Console-print product scope -`just no-console-print` keeps exact file/line/macro approvals for product -sources. Its scope excludes test paths, parsed `#[cfg(test)]` modules, -examples, benches, auxiliary `src/bin/` targets and the explicit -`NON_PRODUCT_CRATES` list in `tools/xtask/src/no_console_print/scope.rs`. -Build scripts remain excluded because their output contains Cargo directives. -`mesh-llm/src/main.rs` and `mesh-client` remain in scope. +`just no-console-print` forbids the print macros and direct `io::stdout()` / +`io::stderr()` handles in product sources. There is no allowlist: every +exemption is a category rule, so no individual call site can be approved. Its +scope excludes test paths, parsed `#[cfg(test)]` modules, examples, benches, +auxiliary `src/bin/` targets and the explicit `NON_PRODUCT_CRATES` list in +`tools/xtask/src/no_console_print/scope.rs`. Build scripts remain excluded +because their output contains Cargo directives. `mesh-llm/src/main.rs` and +`mesh-client` remain in scope. + +The handle rule exempts only the files that implement the console output +facility, listed as `CONSOLE_OUTPUT_OWNERS` in the same module: the sink-aware +writer and its pre-sink CLI fallback, the inline progress renderers, the TUI +output manager / fd capture / terminal backend, the runtime tracing writer, +skippy-server's stderr telemetry sink, and the CLI presentation surfaces. +A capability probe such as `io::stdout().is_terminal()` reads nothing and is +not a handle. The gate checks Cargo metadata on every invocation and rejects an exempt crate that becomes a transitive normal dependency of `mesh-llm`, including optional and platform-specific dependencies. Tests cover that guard and the scope rules. The Quality workflow still invokes the same `just no-console-print` gate. -Direct stdout/stderr handle detection and deletion of the remaining product -ratchet belong to later stages of issue #1763. The CPU native runtime-event gate selects `family-qwen3-dense` from the `skippy-ci-smoke` manifest for both `pull-request` and `main` cadences. The diff --git a/crates/mesh-client/src/models/catalog.rs b/crates/mesh-client/src/models/catalog.rs index 27126999af..ee239c8fe1 100644 --- a/crates/mesh-client/src/models/catalog.rs +++ b/crates/mesh-client/src/models/catalog.rs @@ -1,5 +1,4 @@ use serde::Deserialize; -use std::io::Write; use std::sync::LazyLock; #[derive(Clone, Debug, Deserialize)] @@ -106,24 +105,23 @@ pub fn huggingface_repo_url(url: &str) -> Option { Some(format!("https://huggingface.co/{repo}")) } -pub fn list_models() { - let _ = writeln!(std::io::stderr(), "Available models:"); - let _ = writeln!(std::io::stderr()); +/// Renders the built-in catalog as human-readable lines. This crate is a +/// library, so it hands the listing back to the caller instead of choosing a +/// stream: only the console output facility knows whether a JSON sink or the +/// interactive dashboard currently owns the terminal. +pub fn render_model_listing() -> String { + let mut listing = String::from("Available models:\n\n"); for m in MODEL_CATALOG.iter() { - let draft_info = if let Some(d) = m.draft.as_deref() { - format!(" (draft: {})", d) - } else { - String::new() + let draft_info = match m.draft.as_deref() { + Some(draft) => format!(" (draft: {draft})"), + None => String::new(), }; - let _ = writeln!( - std::io::stderr(), - " {:40} {:>6} {}{}", - m.name, - m.size, - m.description, - draft_info - ); + listing.push_str(&format!( + " {:40} {:>6} {}{}\n", + m.name, m.size, m.description, draft_info + )); } + listing } #[cfg(test)] diff --git a/just/ci.just b/just/ci.just index e446871672..0467a67d3d 100644 --- a/just/ci.just +++ b/just/ci.just @@ -26,9 +26,9 @@ ci-crate-lists: publish-crates: just with-lld cargo run -p xtask -- repo-consistency publish-crates -# Ratchet on println!/eprintln!/print!/eprint! in Rust files under crates/ -# (excluding tests, non-product crates, helper bins and build.rs): each hit must be -# explicitly listed in tools/xtask/data/console_print_allowlist.json. +# Gate on println!/eprintln!/print!/eprint! and direct io::stdout()/io::stderr() +# handles in Rust files under crates/ (excluding tests, non-product crates, helper +# bins and build.rs). There is no allowlist: route output through mesh-llm-events. no-console-print: cargo run -p xtask -- repo-consistency no-console-print diff --git a/tools/xtask/data/console_print_allowlist.json b/tools/xtask/data/console_print_allowlist.json deleted file mode 100644 index 9e26dfeeb6..0000000000 --- a/tools/xtask/data/console_print_allowlist.json +++ /dev/null @@ -1 +0,0 @@ -{} \ No newline at end of file diff --git a/tools/xtask/src/main.rs b/tools/xtask/src/main.rs index 1782f00957..2498bfc839 100644 --- a/tools/xtask/src/main.rs +++ b/tools/xtask/src/main.rs @@ -53,7 +53,7 @@ fn run() -> DynResult<()> { attestation::inspect_release_attestation(rest) } _ => Err( - "usage:\n cargo run -p xtask -- repo-consistency release-targets\n cargo run -p xtask -- repo-consistency ci-crate-lists\n cargo run -p xtask -- repo-consistency publish-crates\n cargo run -p xtask -- repo-consistency test-all-rust-crate-coverage\n cargo run -p xtask -- repo-consistency no-console-print [--regen]\n cargo run -p xtask -- release-attestation generate-keypair --private-key-out --public-key-out \n cargo run -p xtask -- release-attestation stamp --binary --signing-key-file [--node-version ] [--build-id ] [--commit ] [--target-triple ] [--protocol-min ] [--protocol-max ]\n cargo run -p xtask -- release-attestation inspect --binary [--public-key-file ] [--json]" + "usage:\n cargo run -p xtask -- repo-consistency release-targets\n cargo run -p xtask -- repo-consistency ci-crate-lists\n cargo run -p xtask -- repo-consistency publish-crates\n cargo run -p xtask -- repo-consistency test-all-rust-crate-coverage\n cargo run -p xtask -- repo-consistency no-console-print\n cargo run -p xtask -- release-attestation generate-keypair --private-key-out --public-key-out \n cargo run -p xtask -- release-attestation stamp --binary --signing-key-file [--node-version ] [--build-id ] [--commit ] [--target-triple ] [--protocol-min ] [--protocol-max ]\n cargo run -p xtask -- release-attestation inspect --binary [--public-key-file ] [--json]" .to_string() .into(), ), diff --git a/tools/xtask/src/no_console_print.rs b/tools/xtask/src/no_console_print.rs index 3916264f90..35aceb7386 100644 --- a/tools/xtask/src/no_console_print.rs +++ b/tools/xtask/src/no_console_print.rs @@ -1,43 +1,49 @@ -//! Ratchet check that product code routes console output through the app's -//! format-aware event facility instead of raw print macros. +//! Check that product code routes console output through the app's +//! format-aware event facility instead of writing to the terminal itself. +//! +//! This is a plain gate, not a ratchet: there is no allowlist and no way to +//! approve an individual call site. Exemptions are category rules that live in +//! `scope`, so an exception always names a surface that legitimately owns +//! terminal output rather than a line someone wanted to keep. -use std::collections::{BTreeMap, BTreeSet}; use std::fs; use std::path::Path; -use crate::command::{DynResult, write_json_file}; +use crate::command::DynResult; mod scope; -const ALLOWLIST_RELATIVE_PATH: &str = "tools/xtask/data/console_print_allowlist.json"; -const REGEN_FLAG: &str = "--regen"; -const REGEN_COMMAND: &str = "cargo run -p xtask -- repo-consistency no-console-print --regen"; +/// The retired ratchet's data file. The gate fails if it reappears so a future +/// change cannot quietly reintroduce per-location approvals. +const RETIRED_ALLOWLIST_RELATIVE_PATH: &str = "tools/xtask/data/console_print_allowlist.json"; -/// Macros the ratchet forbids in product crates. `eprintln!` contains +/// Macros the gate forbids in product crates. `eprintln!` contains /// `println!`, so matches must be boundary-checked (see `is_macro_boundary`). pub(crate) const FORBIDDEN_CONSOLE_MACROS: [&str; 4] = ["println!", "eprintln!", "print!", "eprint!"]; +/// Direct terminal handles the gate forbids outside the surfaces that own +/// console output. Retiring the print macros makes `writeln!(io::stdout(), ..)` +/// the obvious way to reintroduce exactly the debt the macros carried. +pub(crate) const DIRECT_TERMINAL_HANDLES: [&str; 2] = ["io::stdout()", "io::stderr()"]; + #[derive(Debug, PartialEq, Eq)] pub(crate) struct ConsolePrintHit { pub line: usize, pub macro_name: &'static str, } -/// One ratchet-approved console print occurrence. The ratchet approves exact -/// occurrences rather than per-file counts so that retiring one legacy print -/// can never free up allowance for a new one elsewhere in the file. -#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -struct AllowedOccurrence { - line: usize, - macro_name: String, +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct DirectHandleHit { + pub line: usize, + pub handle: &'static str, } /// Finds every forbidden console print macro occurrence in a source file. /// Whole-line comments are skipped; string literal mentions are intentionally -/// counted so the regenerated baseline stays stable and conservative. An -/// invocation counts even when whitespace or comments separate the macro name -/// from `!`, including across line breaks, because such spellings compile too. +/// counted so the gate stays conservative. An invocation counts even when +/// whitespace or comments separate the macro name from `!`, including across +/// line breaks, because such spellings compile too. pub(crate) fn find_console_prints(source: &str) -> Vec { let lines: Vec<&str> = source.lines().collect(); let mut hits = Vec::new(); @@ -160,10 +166,44 @@ fn advance_to_next_line(lines: &[&str], line_index: &mut usize) -> bool { true } +/// Finds every direct terminal handle acquisition in a source file. Whole-line +/// comments are skipped so prose about the rule does not trip it. Capability +/// probes such as `io::stdout().is_terminal()` read nothing and write nothing, +/// so they are not handles for this purpose. +pub(crate) fn find_direct_terminal_handles(source: &str) -> Vec { + let mut hits = Vec::new(); + for (index, raw_line) in source.lines().enumerate() { + if is_comment_only_line(raw_line) { + continue; + } + for handle in DIRECT_TERMINAL_HANDLES { + for (byte_offset, _matched) in raw_line.match_indices(handle) { + if !is_macro_boundary(raw_line, byte_offset) + || is_capability_probe(&raw_line[byte_offset + handle.len()..]) + { + continue; + } + hits.push(DirectHandleHit { + line: index + 1, + handle, + }); + } + } + } + hits.sort_by(|a, b| a.line.cmp(&b.line).then_with(|| a.handle.cmp(b.handle))); + hits +} + +/// True when the handle is immediately consumed by a read-only capability +/// question rather than kept for writing. +fn is_capability_probe(after_handle: &str) -> bool { + after_handle.trim_start().starts_with(".is_terminal()") +} + /// Collects relative paths (slash separated, deterministic order) of every /// product `.rs` file under `crates/`, using the explicit scope rules in /// `scope`. Build scripts use print macros for Cargo directives. Paths carry -/// the `crates/` prefix so they stay stable as repo-relative allowlist keys. +/// the `crates/` prefix so reported violations are repo-relative. fn collect_rs_files(crates_dir: &Path) -> std::io::Result> { let mut files = Vec::new(); collect_rs_files_recursive(crates_dir, "crates/", &mut files)?; @@ -196,27 +236,11 @@ fn collect_rs_files_recursive( Ok(()) } -/// Gates CI: every console print occurrence in a scanned file must have an -/// exact ratchet approval (same file, line, and macro). Prints at unapproved -/// locations fail, as do approvals whose occurrence moved or disappeared — so -/// retiring one legacy print can never hide a new one. New files with prints -/// fail outright, and allowlist entries for deleted files are reported as -/// stale debt to drop via `--regen`. +/// Gates CI: no console print macro may appear in product code, and no product +/// crate outside the console-owning surfaces may take a terminal handle. There +/// is no per-location approval — an exception is a category rule in `scope`. pub(crate) fn check_no_console_prints(repo_root: &Path) -> DynResult<()> { - let allowlist_path = repo_root.join(ALLOWLIST_RELATIVE_PATH); - let raw_allowlist = fs::read_to_string(&allowlist_path).map_err(|error| { - format!( - "missing console print ratchet at {}: run `{REGEN_COMMAND}` to generate it ({error})", - allowlist_path.display() - ) - })?; - let allowed: BTreeMap> = serde_json::from_str(&raw_allowlist) - .map_err(|error| { - format!( - "invalid console print ratchet at {}: {error}", - allowlist_path.display() - ) - })?; + check_retired_allowlist_absent(repo_root)?; let crates_dir = repo_root.join("crates"); let files = collect_rs_files(&crates_dir).map_err(|error| { @@ -225,59 +249,67 @@ pub(crate) fn check_no_console_prints(repo_root: &Path) -> DynResult<()> { crates_dir.display() ) })?; - let mut seen = BTreeSet::new(); - let mut new_violations: Vec = Vec::new(); - let mut drift_violations: Vec = Vec::new(); + let mut macro_violations: Vec = Vec::new(); + let mut handle_violations: Vec = Vec::new(); for file in &files { - seen.insert(file.as_str()); let source = read_source(repo_root, file)?; - let hits = find_console_prints(&scope::without_test_modules(&source)); - if hits.is_empty() && !allowed.contains_key(file.as_str()) { + let product_source = scope::without_test_modules(&source); + for hit in find_console_prints(&product_source) { + macro_violations.push(format!("{file}:{} {}", hit.line, hit.macro_name)); + } + if scope::owns_console_output(file) { continue; } - match allowed.get(file.as_str()) { - None => { - for hit in &hits { - new_violations.push(format!("{file}:{} {}", hit.line, hit.macro_name)); - } - } - Some(approved) => claim_approved_occurrences( - file, - &hits, - approved, - &mut new_violations, - &mut drift_violations, - ), + for hit in find_direct_terminal_handles(&product_source) { + handle_violations.push(format!("{file}:{} {}", hit.line, hit.handle)); } } - for stale_path in allowed.keys().filter(|path| !seen.contains(path.as_str())) { - drift_violations.push(format!( - "{stale_path}: stale allowlist entry (no console prints remain); remove it with `--regen`" - )); - } - - if new_violations.is_empty() && drift_violations.is_empty() { + if macro_violations.is_empty() && handle_violations.is_empty() { return Ok(()); } let mut sections = Vec::new(); - if !new_violations.is_empty() { + if !macro_violations.is_empty() { sections.push(format!( "forbidden console print macros found in product code:\n{}", - new_violations.join("\n") + macro_violations.join("\n") )); } - if !drift_violations.is_empty() { + if !handle_violations.is_empty() { sections.push(format!( - "console print ratchet is out of sync with the tree:\n{}", - drift_violations.join("\n") + "direct terminal handles found outside the console output facility:\n{}", + handle_violations.join("\n") )); } + Err(format!("{}\n\n{}", sections.join("\n\n"), CONVERSION_GUIDANCE).into()) +} + +const CONVERSION_GUIDANCE: &str = "\ +Convert each site to the facility that owns the stream: + - operational or diagnostic output -> mesh_llm_events::emit_event, or tracing + (`tracing::info!` / `warn!` / `error!`) for runtime diagnostics; + - human-facing CLI prose, tables, and prompts -> mesh_llm_events::console_out + / console_err, which discard while a JSON sink or the TUI owns the terminal; + - the machine-readable payload a --json command exists to produce -> + mesh_llm_events::machine_out. +Writing to io::stdout() / io::stderr() directly bypasses all three: it corrupts +the interactive dashboard and puts free-form text on the stream while a JSON +sink is installed. Only the console output facility itself may hold a terminal +handle; see scope::CONSOLE_OUTPUT_OWNERS."; + +/// The gate replaced a ratchet whose approvals lived in a JSON file. Failing +/// when that file returns keeps a revert or a stray merge from silently +/// restoring per-location approvals nothing reads any more. +fn check_retired_allowlist_absent(repo_root: &Path) -> DynResult<()> { + let retired = repo_root.join(RETIRED_ALLOWLIST_RELATIVE_PATH); + if !retired.exists() { + return Ok(()); + } Err(format!( - "{}\n\nRoute output through mesh_llm_events::emit_event instead; retire legacy debt line by \ -line and regenerate the ratchet with `{REGEN_COMMAND}`.", - sections.join("\n\n") + "stale console print allowlist at {}: the ratchet was retired and this file is no longer \ +read. Delete it; console prints are now gated outright, not approved per location.", + retired.display() ) .into()) } @@ -289,90 +321,15 @@ fn read_source(repo_root: &Path, file: &str) -> DynResult { .map_err(|error| format!("failed to read {}: {error}", path.display()))?) } -/// Multiset-matches the observed hits against the file's approved occurrences. -/// Each hit must claim a distinct approval with the same line and macro; an -/// unclaimed hit is a new print, an unspent approval is drift. -fn claim_approved_occurrences( - file: &str, - hits: &[ConsolePrintHit], - approved: &[AllowedOccurrence], - new_violations: &mut Vec, - drift_violations: &mut Vec, -) { - let mut approved_used = vec![false; approved.len()]; - for hit in hits { - let claimed = (0..approved.len()).find(|index| { - !approved_used[*index] - && approved[*index].line == hit.line - && approved[*index].macro_name == hit.macro_name - }); - match claimed { - Some(index) => approved_used[index] = true, - None => new_violations.push(format!("{file}:{} {}", hit.line, hit.macro_name)), - } - } - for (index, occurrence) in approved.iter().enumerate() { - if !approved_used[index] { - drift_violations.push(format!( - "{file}:{} {}: approved occurrence is missing or was replaced", - occurrence.line, occurrence.macro_name - )); - } - } -} - -/// Entry point for `xtask repo-consistency no-console-print [--regen]`. The -/// plain invocation gates CI; `--regen` rewrites the ratchet from the current -/// tree after validating the product scope so the reduced baseline can be committed. -pub(crate) fn check_no_console_print_command(rest: &[String]) -> DynResult<()> { +/// Entry point for `xtask repo-consistency no-console-print`. +pub(crate) fn check_no_console_print_command(_rest: &[String]) -> DynResult<()> { let repo_root = crate::repo_consistency::repo_root()?; scope::check_exempt_crates(&repo_root)?; - if rest.iter().any(|arg| arg == REGEN_FLAG) { - regenerate_allowlist(&repo_root)?; - } else { - check_no_console_prints(&repo_root)?; - } + check_no_console_prints(&repo_root)?; println!("repo consistency checks passed: no-console-print"); Ok(()) } -fn regenerate_allowlist(repo_root: &Path) -> DynResult<()> { - let crates_dir = repo_root.join("crates"); - let files = collect_rs_files(&crates_dir).map_err(|error| { - format!( - "failed to list Rust sources under {}: {error}", - crates_dir.display() - ) - })?; - let mut allowed: BTreeMap> = BTreeMap::new(); - for file in &files { - let source = fs::read_to_string(repo_root.join(file)) - .map_err(|error| format!("failed to read {file}: {error}"))?; - let hits = find_console_prints(&scope::without_test_modules(&source)); - if !hits.is_empty() { - allowed.insert( - file.clone(), - hits.iter() - .map(|hit| AllowedOccurrence { - line: hit.line, - macro_name: hit.macro_name.to_string(), - }) - .collect(), - ); - } - } - let allowlist_path = repo_root.join(ALLOWLIST_RELATIVE_PATH); - write_json_file(&allowlist_path, &allowed)?; - let total: usize = allowed.values().map(Vec::len).sum(); - println!( - "console print ratchet regenerated at {}: {} file(s), {} legacy hit(s)", - allowlist_path.display(), - allowed.len(), - total - ); - Ok(()) -} - #[cfg(test)] mod tests { use super::*; @@ -485,133 +442,98 @@ lines */ !(x); } #[test] - fn ratchet_fails_for_unapproved_print_locations() { + fn finds_direct_terminal_handles_and_skips_capability_probes() { + let source = "use std::io::Write;\n\ + // io::stdout() in prose is not a handle\n\ + fn f() {\n\ + \x20 let interactive = std::io::stdout().is_terminal();\n\ + \x20 let _ = writeln!(std::io::stderr(), \"hi\");\n\ + \x20 let out = io::stdout();\n\ + }\n"; + assert_eq!( + find_direct_terminal_handles(source), + vec![ + DirectHandleHit { + line: 5, + handle: "io::stderr()" + }, + DirectHandleHit { + line: 6, + handle: "io::stdout()" + }, + ] + ); + } + + #[test] + fn gate_fails_for_every_console_print_with_no_way_to_approve_one() { let repo_root = temp_repo_with_files(&[( "crates/demo/src/lib.rs", "fn main() {\n println!(\"one\");\n eprintln!(\"two\");\n}\n", )]); - write_allowlist( - &repo_root, - r#"{"crates/demo/src/lib.rs": [{"line": 2, "macro_name": "println!"}]}"#, - ); let error = check_no_console_prints(&repo_root).unwrap_err().to_string(); assert!( error.contains("forbidden console print macros found"), "{error}" ); - // The approved occurrence is not reported; only the new one is. - assert!(!error.contains(":2 println!"), "{error}"); + assert!( + error.contains("crates/demo/src/lib.rs:2 println!"), + "{error}" + ); assert!( error.contains("crates/demo/src/lib.rs:3 eprintln!"), "{error}" ); + assert!(error.contains("mesh_llm_events::console_out"), "{error}"); } #[test] - fn retiring_an_approved_print_cannot_hide_a_new_one() { - let repo_root = temp_repo_with_files(&[( - "crates/demo/src/lib.rs", - "fn main() {\n eprintln!(\"swapped\");\n}\n", - )]); - write_allowlist( - &repo_root, - r#"{"crates/demo/src/lib.rs": [{"line": 2, "macro_name": "println!"}]}"#, - ); + fn gate_passes_for_product_code_with_no_console_output() { + let repo_root = + temp_repo_with_files(&[("crates/demo/src/lib.rs", "fn main() {\n let _ = 1;\n}\n")]); + check_no_console_prints(&repo_root).expect("clean product code must pass"); + } + + #[test] + fn gate_fails_when_the_retired_allowlist_reappears() { + let repo_root = temp_repo_with_files(&[ + ("crates/demo/src/lib.rs", "fn main() {}\n"), + (RETIRED_ALLOWLIST_RELATIVE_PATH, "{}\n"), + ]); let error = check_no_console_prints(&repo_root).unwrap_err().to_string(); - // Old count-based ratchets pass this (1 print <= allowance of 1); the - // occurrence ratchet must flag both sides of the swap. - assert!( - error.contains("crates/demo/src/lib.rs:2 eprintln!"), - "{error}" - ); - assert!( - error.contains( - "crates/demo/src/lib.rs:2 println!: approved occurrence is missing or was replaced" - ), - "{error}" - ); + assert!(error.contains("stale console print allowlist"), "{error}"); } #[test] - fn ratchet_fails_when_an_approved_occurrence_is_removed() { + fn gate_fails_for_direct_terminal_handles_outside_the_output_facility() { let repo_root = temp_repo_with_files(&[( "crates/demo/src/lib.rs", - "fn main() {\n println!(\"one\");\n}\n", + "fn f() {\n let _ = writeln!(std::io::stderr(), \"bypass\");\n}\n", )]); - write_allowlist( - &repo_root, - r#"{"crates/demo/src/lib.rs": [{"line": 2, "macro_name": "println!"}, {"line": 3, "macro_name": "eprintln!"}]}"#, - ); let error = check_no_console_prints(&repo_root).unwrap_err().to_string(); assert!( - error.contains("console print ratchet is out of sync with the tree"), + error.contains("direct terminal handles found outside the console output facility"), "{error}" ); assert!( - !error.contains("forbidden console print macros found"), - "{error}" - ); - assert!( - error.contains( - "crates/demo/src/lib.rs:3 eprintln!: approved occurrence is missing or was replaced" - ), + error.contains("crates/demo/src/lib.rs:2 io::stderr()"), "{error}" ); } #[test] - fn ratchet_passes_when_occurrences_match_exactly() { - let repo_root = temp_repo_with_files(&[( - "crates/demo/src/lib.rs", - "fn main() {\n println!(\"one\");\n}\n", - )]); - write_allowlist( - &repo_root, - r#"{"crates/demo/src/lib.rs": [{"line": 2, "macro_name": "println!"}]}"#, - ); - check_no_console_prints(&repo_root).expect("matching occurrences must pass"); - - let duplicates = temp_repo_with_files(&[( - "crates/twin/src/lib.rs", - "fn main() {\n println!(\"a\"); println!(\"b\");\n}\n", - )]); - write_allowlist( - &duplicates, - r#"{"crates/twin/src/lib.rs": [{"line": 2, "macro_name": "println!"}, {"line": 2, "macro_name": "println!"}]}"#, - ); - check_no_console_prints(&duplicates) - .expect("duplicate approvals for same-line prints must pass"); - } - - #[test] - fn ratchet_fails_for_new_files_and_stale_entries() { - let repo_root = temp_repo_with_files(&[ - ( - "crates/legacy/src/lib.rs", - "fn f() { println!(\"old\"); }\n", - ), - ( - "crates/fresh/src/lib.rs", - "fn g() { eprintln!(\"new\"); }\n", - ), - ]); - write_allowlist( - &repo_root, - r#"{"crates/legacy/src/lib.rs": [{"line": 1, "macro_name": "println!"}], "crates/gone/src/lib.rs": [{"line": 5, "macro_name": "print!"}]}"#, - ); - let error = check_no_console_prints(&repo_root).unwrap_err().to_string(); - // The approved legacy occurrence passes; only the fresh file and the - // entry for a deleted file are reported. - assert!(!error.contains("crates/legacy/src/lib.rs:1"), "{error}"); - assert!( - error.contains("crates/fresh/src/lib.rs:1 eprintln!"), - "{error}" - ); - assert!(error.contains("stale allowlist entry"), "{error}"); + fn console_output_owners_may_hold_terminal_handles() { + let owner = scope::CONSOLE_OUTPUT_OWNERS + .iter() + .find(|path| path.starts_with("crates/mesh-llm-events/")) + .expect("an events-crate owner"); + let repo_root = + temp_repo_with_files(&[(owner, "fn f() {\n let mut out = std::io::stderr();\n}\n")]); + check_no_console_prints(&repo_root).expect("the output facility owns terminal access"); } #[test] - fn regeneration_and_gate_share_product_scope() { + fn gate_respects_product_scope() { let repo_root = temp_repo_with_files(&[ ( "crates/demo/src/lib.rs", @@ -630,21 +552,19 @@ lines */ !(x); "fn f() { println!(\"bench\"); }", ), ]); - regenerate_allowlist(&repo_root).unwrap(); - let allowed: BTreeMap> = serde_json::from_str( - &fs::read_to_string(repo_root.join(ALLOWLIST_RELATIVE_PATH)).unwrap(), - ) - .unwrap(); - assert_eq!(allowed.len(), 1); - assert_eq!(allowed["crates/demo/src/lib.rs"].len(), 1); - assert_eq!(allowed["crates/demo/src/lib.rs"][0].line, 2); - check_no_console_prints(&repo_root).unwrap(); - fs::write( - repo_root.join("crates/demo/src/lib.rs"), - "fn f() { eprintln!(\"new\"); }", - ) - .unwrap(); - assert!(check_no_console_prints(&repo_root).is_err()); + let error = check_no_console_prints(&repo_root).unwrap_err().to_string(); + assert!( + error.contains("crates/demo/src/lib.rs:2 println!"), + "{error}" + ); + for out_of_scope in [ + "crates/demo/src/lib.rs:1", + "crates/demo/tests/integration.rs", + "crates/demo/src/bin/tool.rs", + "crates/skippy-bench/src/lib.rs", + ] { + assert!(!error.contains(out_of_scope), "{out_of_scope}: {error}"); + } } fn temp_repo_with_files(files: &[(&str, &str)]) -> std::path::PathBuf { @@ -656,9 +576,4 @@ lines */ !(x); } dir } - - fn write_allowlist(repo_root: &Path, raw_json: &str) { - fs::create_dir_all(repo_root.join("tools/xtask/data")).unwrap(); - fs::write(repo_root.join(ALLOWLIST_RELATIVE_PATH), raw_json).unwrap(); - } } diff --git a/tools/xtask/src/no_console_print/scope.rs b/tools/xtask/src/no_console_print/scope.rs index ab8fcd6e75..9131bfd020 100644 --- a/tools/xtask/src/no_console_print/scope.rs +++ b/tools/xtask/src/no_console_print/scope.rs @@ -25,6 +25,39 @@ const NON_PRODUCT_CRATES: &[&str] = &[ "metrics-server", ]; +/// Files that implement the console output facility, and therefore legitimately +/// hold a terminal handle. Every other product file must route output through +/// that facility. These are whole-file category rules: a surface either owns +/// terminal access or it does not, so this list can never grow to excuse one +/// convenient write inside an ordinary module. +/// +/// - the sink-aware console writer and the pre-sink CLI lifecycle fallback, +/// - the inline progress renderers, which paint a transient cursor-addressed +/// redraw that has no structured representation, +/// - the TUI's own output manager, fd capture, and terminal backend, +/// - the runtime's tracing writer, the last-resort path used when event +/// emission itself fails, +/// - skippy-server's stderr telemetry sink, whose entire purpose is writing +/// newline-delimited events to stderr, +/// - CLI presentation surfaces that render to the user's terminal by design. +pub(super) const CONSOLE_OUTPUT_OWNERS: &[&str] = &[ + "crates/mesh-llm-events/src/console.rs", + "crates/mesh-llm-events/src/command_lifecycle.rs", + "crates/mesh-llm-events/src/terminal_progress.rs", + "crates/mesh-llm-tui/src/terminal_progress.rs", + "crates/mesh-llm-tui/src/output/console_capture.rs", + "crates/mesh-llm-tui/src/output/formatting.rs", + "crates/mesh-llm-tui/src/output/terminal_out.rs", + "crates/mesh-llm-host-runtime/src/runtime/tracing_writer.rs", + "crates/skippy-server/src/telemetry.rs", + "crates/mesh-llm-cli/src/pager.rs", + "crates/mesh-llm-commands/src/gpus/tune_runner.rs", +]; + +pub(super) fn owns_console_output(path: &str) -> bool { + CONSOLE_OUTPUT_OWNERS.contains(&path) +} + pub(super) fn is_product_source(path: &str) -> bool { let parts: Vec<_> = path.split('/').collect(); if parts.len() < 3 || parts[0] != "crates" { From 56eca81a8b708d9aaa7717a1b801d54ffc923d3e Mon Sep 17 00:00:00 2001 From: scama Date: Tue, 15 Sep 2026 15:13:56 +1000 Subject: [PATCH 2/2] fix(ci): close console output gate bypasses --- tools/xtask/src/no_console_print.rs | 170 ++++++++++++++++++++++++++-- 1 file changed, 158 insertions(+), 12 deletions(-) diff --git a/tools/xtask/src/no_console_print.rs b/tools/xtask/src/no_console_print.rs index 35aceb7386..a811c545d2 100644 --- a/tools/xtask/src/no_console_print.rs +++ b/tools/xtask/src/no_console_print.rs @@ -25,7 +25,8 @@ pub(crate) const FORBIDDEN_CONSOLE_MACROS: [&str; 4] = /// Direct terminal handles the gate forbids outside the surfaces that own /// console output. Retiring the print macros makes `writeln!(io::stdout(), ..)` /// the obvious way to reintroduce exactly the debt the macros carried. -pub(crate) const DIRECT_TERMINAL_HANDLES: [&str; 2] = ["io::stdout()", "io::stderr()"]; +pub(crate) const DIRECT_TERMINAL_HANDLES: [(&str, &str); 2] = + [("stdout", "stdout()"), ("stderr", "stderr()")]; #[derive(Debug, PartialEq, Eq)] pub(crate) struct ConsolePrintHit { @@ -171,15 +172,22 @@ fn advance_to_next_line(lines: &[&str], line_index: &mut usize) -> bool { /// probes such as `io::stdout().is_terminal()` read nothing and write nothing, /// so they are not handles for this purpose. pub(crate) fn find_direct_terminal_handles(source: &str) -> Vec { + let lines: Vec<&str> = source.lines().collect(); let mut hits = Vec::new(); - for (index, raw_line) in source.lines().enumerate() { + for (index, raw_line) in lines.iter().enumerate() { if is_comment_only_line(raw_line) { continue; } - for handle in DIRECT_TERMINAL_HANDLES { - for (byte_offset, _matched) in raw_line.match_indices(handle) { - if !is_macro_boundary(raw_line, byte_offset) - || is_capability_probe(&raw_line[byte_offset + handle.len()..]) + for (function, handle) in DIRECT_TERMINAL_HANDLES { + for (byte_offset, _matched) in raw_line.match_indices(function) { + let end = byte_offset + function.len(); + if !is_identifier_boundary(raw_line, byte_offset, end) { + continue; + } + let direct_call = terminal_call_end(&lines, index, end); + let alias_import = resolves_to_alias(&lines, index, end); + if direct_call.is_some_and(|cursor| is_capability_probe_at(&lines, cursor)) + || direct_call.is_none() && !alias_import { continue; } @@ -194,10 +202,105 @@ pub(crate) fn find_direct_terminal_handles(source: &str) -> Vec hits } +fn is_identifier_boundary(line: &str, start: usize, end: usize) -> bool { + let identifier = |ch: char| ch.is_alphanumeric() || ch == '_'; + !line[..start].chars().next_back().is_some_and(identifier) + && !line[end..].chars().next().is_some_and(identifier) +} + +#[derive(Clone, Copy)] +struct SourceCursor { + line: usize, + byte: usize, +} + +/// Skip Rust whitespace and comments, including nested block comments. +fn skip_trivia(lines: &[&str], cursor: &mut SourceCursor) { + let mut block_depth = 0u32; + loop { + if cursor.line >= lines.len() { + return; + } + let line = lines[cursor.line]; + if cursor.byte >= line.len() { + cursor.line += 1; + cursor.byte = 0; + continue; + } + let rest = &line[cursor.byte..]; + if block_depth > 0 { + if rest.starts_with("/*") { + block_depth += 1; + cursor.byte += 2; + } else if rest.starts_with("*/") { + block_depth -= 1; + cursor.byte += 2; + } else { + cursor.byte += rest.chars().next().expect("non-empty rest").len_utf8(); + } + continue; + } + if rest.starts_with("//") { + cursor.line += 1; + cursor.byte = 0; + } else if rest.starts_with("/*") { + block_depth = 1; + cursor.byte += 2; + } else if rest.chars().next().expect("non-empty rest").is_whitespace() { + cursor.byte += rest.chars().next().expect("non-empty rest").len_utf8(); + } else { + return; + } + } +} + +fn consume(lines: &[&str], cursor: &mut SourceCursor, token: &str) -> bool { + skip_trivia(lines, cursor); + let Some(rest) = lines + .get(cursor.line) + .and_then(|line| line.get(cursor.byte..)) + else { + return false; + }; + if !rest.starts_with(token) { + return false; + } + cursor.byte += token.len(); + true +} + +/// Return the cursor after a trivia-tolerant empty call to stdout/stderr. +fn terminal_call_end(lines: &[&str], line: usize, byte: usize) -> Option { + let mut cursor = SourceCursor { line, byte }; + if consume(lines, &mut cursor, "(") && consume(lines, &mut cursor, ")") { + Some(cursor) + } else { + None + } +} + +/// A renamed direct import can hide the canonical function name at the call +/// site, so the import itself is enough to fail the conservative gate. +fn resolves_to_alias(lines: &[&str], line: usize, byte: usize) -> bool { + let mut cursor = SourceCursor { line, byte }; + if !consume(lines, &mut cursor, "as") { + return false; + } + skip_trivia(lines, &mut cursor); + lines + .get(cursor.line) + .and_then(|source| source.get(cursor.byte..)) + .and_then(|rest| rest.chars().next()) + .is_some_and(|ch| ch.is_alphabetic() || ch == '_') +} + /// True when the handle is immediately consumed by a read-only capability /// question rather than kept for writing. -fn is_capability_probe(after_handle: &str) -> bool { - after_handle.trim_start().starts_with(".is_terminal()") +fn is_capability_probe_at(lines: &[&str], mut cursor: SourceCursor) -> bool { + consume(lines, &mut cursor, ".") + && consume(lines, &mut cursor, "is_terminal") + && consume(lines, &mut cursor, "(") + && consume(lines, &mut cursor, ")") } /// Collects relative paths (slash separated, deterministic order) of every @@ -322,7 +425,10 @@ fn read_source(repo_root: &Path, file: &str) -> DynResult { } /// Entry point for `xtask repo-consistency no-console-print`. -pub(crate) fn check_no_console_print_command(_rest: &[String]) -> DynResult<()> { +pub(crate) fn check_no_console_print_command(rest: &[String]) -> DynResult<()> { + if !rest.is_empty() { + return Err("usage: cargo run -p xtask -- repo-consistency no-console-print".into()); + } let repo_root = crate::repo_consistency::repo_root()?; scope::check_exempt_crates(&repo_root)?; check_no_console_prints(&repo_root)?; @@ -455,16 +561,56 @@ lines */ !(x); vec![ DirectHandleHit { line: 5, - handle: "io::stderr()" + handle: "stderr()" }, DirectHandleHit { line: 6, - handle: "io::stdout()" + handle: "stdout()" }, ] ); } + #[test] + fn finds_imported_aliased_and_multiline_terminal_handles() { + let source = r#"use std::io::stdout; +use std::io::{stderr as terminal_error}; +fn f() { + stdout /* split */ + ( + ); + terminal_error(); + std::io:: + stderr + /* split */ () + .is_terminal(); +}"#; + assert_eq!( + find_direct_terminal_handles(source), + vec![ + DirectHandleHit { + line: 2, + handle: "stderr()" + }, + DirectHandleHit { + line: 4, + handle: "stdout()" + }, + ] + ); + } + + #[test] + fn no_console_print_command_rejects_trailing_arguments() { + let error = check_no_console_print_command(&["--regen".to_owned()]) + .unwrap_err() + .to_string(); + assert_eq!( + error, + "usage: cargo run -p xtask -- repo-consistency no-console-print" + ); + } + #[test] fn gate_fails_for_every_console_print_with_no_way_to_approve_one() { let repo_root = temp_repo_with_files(&[( @@ -516,7 +662,7 @@ lines */ !(x); "{error}" ); assert!( - error.contains("crates/demo/src/lib.rs:2 io::stderr()"), + error.contains("crates/demo/src/lib.rs:2 stderr()"), "{error}" ); }