From 9399c5de56194f1398d4e719e7a714200139b533 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 16:51:17 -0400 Subject: [PATCH 1/7] test(cli): stop the tree tests depending on ambient colour detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dependable tree` styles its labels through `if_supports_color`, which asks the environment whether stdout can take ANSI. Both sets of tree tests assumed the answer would be no — the unit tests because a test harness captures stdout, the end-to-end ones because a pipe is not a TTY — and asserted on the plain text. That is a default, not a guarantee. `FORCE_COLOR` overrides it, and anything exporting it puts it in scope: a terminal multiplexer, a task runner, a CI image. Under `FORCE_COLOR=3` three tests fail on an unmodified checkout — `ascii_points_a_member_at_its_own_tree`, `tree_distinguishes_workspace_and_ external`, and `a_member_used_by_another_member_points_at_its_own_tree` — which makes `mise run test` fail for a contributor whose shell sets it while CI, which does not, stays green. A test that only holds in some terminals reports the terminal, not the code. The assertions are about the shape of the tree, never its colour, so both are made to say so: the unit tests strip any styling from `ascii` before matching, and the end-to-end `run` helper pins the child's colour environment rather than inferring it from the pipe. `dependable` itself is unchanged — honouring `FORCE_COLOR` through a pipe is what the variable is for. --- crates/dependable/src/output/tree.rs | 40 ++++++++++++++++++++++--- crates/dependable/tests/fixture_tree.rs | 16 ++++++++-- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/crates/dependable/src/output/tree.rs b/crates/dependable/src/output/tree.rs index a3bbadf..9f30b2a 100644 --- a/crates/dependable/src/output/tree.rs +++ b/crates/dependable/src/output/tree.rs @@ -294,10 +294,42 @@ source = "registry+https://x" DependencyGraph::from_resolved(&resolved, &names, &["app".to_owned()]) } + /// `ascii` output with any terminal styling removed. + /// + /// [`label`] colours through `if_supports_color`, which asks the ambient + /// environment whether stdout can take ANSI — and `FORCE_COLOR`, which a + /// terminal multiplexer or a task runner may well export, answers yes even + /// under a captured test harness. These tests assert on the shape of the + /// tree, never on its colour, so a styled run must not fail them; stripping + /// here is what states that, rather than leaving it to whatever the process + /// happened to inherit. + fn plain(graph: &DependencyGraph, opts: &TreeOptions) -> String { + let raw = ascii(graph, opts); + let mut out = String::with_capacity(raw.len()); + let mut chars = raw.chars(); + while let Some(c) = chars.next() { + if c != '\u{1b}' { + out.push(c); + continue; + } + // Only CSI sequences (`ESC [ … final`) are ever emitted here, and a + // CSI's final byte is the first in `@..=~` after the `[`. + if chars.next() != Some('[') { + continue; + } + for c in chars.by_ref() { + if matches!(c, '@'..='~') { + break; + } + } + } + out + } + #[test] fn ascii_marks_workspace_and_dedupe() { // Color is disabled in the test harness (not a TTY), so labels are plain. - let out = ascii(&sample(), &TreeOptions::default()); + let out = plain(&sample(), &TreeOptions::default()); assert!(out.contains("app v0.1.0 (workspace)")); assert!(out.contains("├── serde v1.0.0")); assert!(out.contains("└── ")); // last-child connector @@ -329,7 +361,7 @@ source = "registry+https://x" #[test] fn ascii_points_a_member_at_its_own_tree() { - let out = ascii(&workspace(), &TreeOptions::default()); + let out = plain(&workspace(), &TreeOptions::default()); assert!( out.contains("└── lib v0.1.0 (workspace) (see root)"), "under `app`, `lib` is a pointer rather than a copy; {out}" @@ -352,7 +384,7 @@ source = "registry+https://x" collapse_roots: false, ..TreeOptions::default() }; - let out = ascii(&workspace(), &opts); + let out = plain(&workspace(), &opts); assert!(!out.contains("(see root)"), "{out}"); assert_eq!(out.matches("serde v1.0.0").count(), 2, "{out}"); } @@ -364,7 +396,7 @@ source = "registry+https://x" dedupe: true, ..TreeOptions::default() }; - let out = ascii(&sample(), &opts); + let out = plain(&sample(), &opts); assert!(out.contains("app v0.1.0 (workspace)")); assert!(!out.contains("serde")); } diff --git a/crates/dependable/tests/fixture_tree.rs b/crates/dependable/tests/fixture_tree.rs index 063ec75..2ce5266 100644 --- a/crates/dependable/tests/fixture_tree.rs +++ b/crates/dependable/tests/fixture_tree.rs @@ -1,6 +1,7 @@ //! End-to-end: `dependable tree` over a committed workspace fixture. Fully -//! offline — the graph comes from the fixture's `Cargo.lock`. Piped stdout is -//! not a TTY, so labels are plain (uncolored) and assertable as text. +//! offline — the graph comes from the fixture's `Cargo.lock`. Labels are +//! asserted as plain text, which [`run`] pins the child's environment to make +//! true rather than inferring it from the pipe. use std::path::PathBuf; use std::process::{Command, Output}; @@ -9,9 +10,20 @@ fn fixture() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/sample-workspace") } +/// `dependable` with the given arguments, its output left unstyled. +/// +/// A pipe is not a TTY, so the labels come out plain by default — but that is a +/// default, not a guarantee. `FORCE_COLOR` overrides it, and the child inherits +/// whatever exported it: a terminal multiplexer, a task runner, a CI image. The +/// assertions below are about the shape of the tree rather than its colour, so +/// the environment is pinned here instead of assumed, and the tests state the +/// same thing in a coloured terminal as in a bare one. fn run(args: &[&str]) -> Output { Command::new(env!("CARGO_BIN_EXE_dependable")) .args(args) + .env_remove("FORCE_COLOR") + .env_remove("CLICOLOR_FORCE") + .env("NO_COLOR", "1") .output() .expect("run dependable") } From ff41756efd110ecedf6b208abafc1343afcc4262 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:07:54 -0400 Subject: [PATCH 2/7] ci: run the test suite a second time with colour forced on Making the three failing tests colour-independent fixes today's failure but keeps nothing that way. `crates/dependable/src/output/table.rs` styles the manifest path and every status cell through the same `if_supports_color`, and no test file that spawns the binary pins its colour environment; the next assertion written across a style boundary reintroduces the same flake, and it reintroduces it silently, because the default run is the configuration where colour is off. `FORCE_COLOR` outranks the TTY check inside `supports-color`, so forcing it on is a genuinely different configuration rather than a repeat of the first run. A `test:color` task runs the suite under it, and CI runs that task after the ordinary one on the Linux leg. The suite is hermetic and takes seconds, so the second run costs little and is the only thing that holds the property. Also drops a comment in `ascii_marks_workspace_and_dedupe` that still claimed colour is disabled because a test harness is not a TTY. That is the assumption this branch exists to remove, and leaving it ten lines below the helper doc that contradicts it invites someone to conclude the stripping is redundant. --- .github/workflows/ci.yml | 6 ++++++ crates/dependable/src/output/tree.rs | 1 - mise.toml | 11 +++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eef5a97..3093125 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,12 @@ jobs: run: mise run lint - name: Test run: mise run test + # A second run with color forced on. The tree assertions match unstyled + # text, and `FORCE_COLOR` outranks the TTY check the default run relies on, + # so this is the leg that catches an assertion straddling a style boundary + # before it reaches a contributor whose shell exports the variable. + - name: Test (color forced) + run: mise run test:color # Run the hermetic test suite on Windows and macOS so those targets are # officially supported. Format/lint are platform-agnostic and stay on the Linux diff --git a/crates/dependable/src/output/tree.rs b/crates/dependable/src/output/tree.rs index 9f30b2a..02cbee6 100644 --- a/crates/dependable/src/output/tree.rs +++ b/crates/dependable/src/output/tree.rs @@ -328,7 +328,6 @@ source = "registry+https://x" #[test] fn ascii_marks_workspace_and_dedupe() { - // Color is disabled in the test harness (not a TTY), so labels are plain. let out = plain(&sample(), &TreeOptions::default()); assert!(out.contains("app v0.1.0 (workspace)")); assert!(out.contains("├── serde v1.0.0")); diff --git a/mise.toml b/mise.toml index bca6759..67051e5 100644 --- a/mise.toml +++ b/mise.toml @@ -28,6 +28,17 @@ run = "cargo install --path crates/dependable --locked --force" description = "Run all tests (live network tests are #[ignore]d and skipped)" run = "cargo test --workspace" +# The tree output is styled through `if_supports_color`, which consults the +# environment. The suite asserts on unstyled text, so a run with color forced on +# is the only thing that proves those assertions do not quietly depend on the +# terminal they happen to be run from. `FORCE_COLOR` outranks the TTY check in +# `supports-color`, which is what makes this a real second configuration rather +# than a repeat of the first. +[tasks."test:color"] +description = "Run the test suite with terminal color forced on" +run = "cargo test --workspace" +env = { FORCE_COLOR = "3" } + [tasks."test:live"] description = "Run the live network smoke tests (hits crates.io + OSV)" run = "cargo test --workspace -- --ignored" From d58c3bb84ec5fc459520a592675e40e88157366c Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 19:02:53 -0400 Subject: [PATCH 3/7] ci: put the colour-forced run behind the local gate too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The colour-forced suite ran only in CI, so the guard caught a colour-dependent assertion one round-trip later than it needed to: `mise run ci` and the pre-push hook both passed, and the failure arrived from the remote. `[tasks.ci]` and the pre-push gate now include it, and the Quality block lists it beside the plain run. The exposure is wider than the three tests this branch repairs — `output/table.rs` styles the manifest path and every status cell through the same `if_supports_color`, and no test that spawns the binary pins its colour environment, so they pass today only because their assertions happen not to straddle a style boundary. This is what makes that prospective rather than permanent. The extra run costs seconds: the suite is hermetic and already built by then, so the second pass compiles nothing. --- AGENTS.md | 1 + hk.pkl | 9 +++++++++ mise.toml | 4 ++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e8f5f2a..c31d9f9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,6 +27,7 @@ Validate changes before committing: ```bash mise run test # correctness (cargo test --workspace) +mise run test:color # the same suite with terminal color forced on mise run fmt:check # formatting mise run lint # clippy -D warnings mise run coverage # coverage (informational, no threshold) diff --git a/hk.pkl b/hk.pkl index e6bd0fb..9fc9905 100644 --- a/hk.pkl +++ b/hk.pkl @@ -22,6 +22,14 @@ local test = new Step { check = "cargo test --workspace" } +// The same suite with color forced on. `FORCE_COLOR` outranks the TTY check inside +// `supports-color`, so this is a second configuration rather than a repeat: it is what +// catches an assertion that matches unstyled text and would otherwise pass here and +// fail for whoever runs the suite from a terminal that exports the variable. +local testColor = new Step { + check = "mise run test:color" +} + local coverage = new Step { check = "cargo llvm-cov --workspace" } @@ -61,6 +69,7 @@ hooks { ["fmt"] = fmt ["clippy"] = clippy ["test"] = test + ["test-color"] = testColor ["coverage"] = coverage } } diff --git a/mise.toml b/mise.toml index 67051e5..cc21704 100644 --- a/mise.toml +++ b/mise.toml @@ -68,5 +68,5 @@ description = "Check that commits on this branch follow Conventional Commits" run = "convco check master..HEAD" [tasks.ci] -description = "Run the full local CI gate: format check, lint, and tests" -depends = ["fmt:check", "lint", "test"] +description = "Run the full local CI gate: format check, lint, and tests (both color modes)" +depends = ["fmt:check", "lint", "test", "test:color"] From fe3fa20e2d1f137abcb05a84d84422a649ee479d Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:18:06 -0400 Subject: [PATCH 4/7] ci: invoke cargo directly from the colour-forced hook step The step shelled out to `mise run test:color`. This file's own header states the convention -- explicit, stable workspace commands rather than delegation -- and every other step here follows it. Delegating also made the hook depend on `mise` being resolvable in the hook environment, which nothing guarantees. `Step` carries its own `env` block, so the forced-colour configuration the task supplied can be expressed here directly. --- hk.pkl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hk.pkl b/hk.pkl index 9fc9905..75ffd67 100644 --- a/hk.pkl +++ b/hk.pkl @@ -27,7 +27,10 @@ local test = new Step { // catches an assertion that matches unstyled text and would otherwise pass here and // fail for whoever runs the suite from a terminal that exports the variable. local testColor = new Step { - check = "mise run test:color" + check = "cargo test --workspace" + env { + ["FORCE_COLOR"] = "3" + } } local coverage = new Step { From 091c0318bff9b2d67599d645841e0f230bdd43a4 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:18:21 -0400 Subject: [PATCH 5/7] ci: run the colour-forced suite after the plain one, not beside it The colour leg was a second, byte-identical `cargo test --workspace` scheduled alongside the first in the same target directory. hk runs hook steps in parallel up to `HK_JOBS` (default: core count) and mise runs a task's `depends` list concurrently, and cargo releases its build-directory lock before any test binary starts -- so the two test phases overlapped rather than queued. That is not safe here. The CLI integration tests keep fixed-name scratch fixtures under `CARGO_TARGET_TMPDIR` and wipe each with `remove_dir_all` on entry, so two concurrent runs delete each other's fixtures. `github_summary` is the sharpest case: if the second run recreates the directory between the first run's two `check()` calls, `summary.md` comes back empty and the "appended, not clobbered" assertion fails on a count of 1 -- an unexplainable local failure of exactly the kind this branch exists to remove. Serialising costs almost nothing: `FORCE_COLOR` is in no fingerprint, so the second run's build is a no-op and only its test execution is added. A separate `CARGO_TARGET_DIR` would isolate the two as well, but it forces a full second build and roughly ten more gigabytes of target output for a run whose only difference is one environment variable. CI was unaffected either way -- its two legs are sequential job steps -- so the green checks were never evidence about this. --- hk.pkl | 8 ++++++++ mise.toml | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/hk.pkl b/hk.pkl index 75ffd67..618b0f2 100644 --- a/hk.pkl +++ b/hk.pkl @@ -26,7 +26,15 @@ local test = new Step { // `supports-color`, so this is a second configuration rather than a repeat: it is what // catches an assertion that matches unstyled text and would otherwise pass here and // fail for whoever runs the suite from a terminal that exports the variable. +// +// `depends` on the plain run, because hk schedules steps in parallel up to `HK_JOBS` +// and cargo drops its build lock before any test binary starts: two concurrent runs +// share one target directory, and the integration tests keep fixed-name scratch +// fixtures under `CARGO_TARGET_TMPDIR` that they wipe on entry, so they would delete +// each other's. Serializing is enough — `FORCE_COLOR` is in no fingerprint, so the +// second run rebuilds nothing and costs only its own test execution. local testColor = new Step { + depends = List("test") check = "cargo test --workspace" env { ["FORCE_COLOR"] = "3" diff --git a/mise.toml b/mise.toml index cc21704..5afd8b2 100644 --- a/mise.toml +++ b/mise.toml @@ -34,8 +34,15 @@ run = "cargo test --workspace" # terminal they happen to be run from. `FORCE_COLOR` outranks the TTY check in # `supports-color`, which is what makes this a real second configuration rather # than a repeat of the first. +# +# It `depends` on `test` rather than running beside it: both invocations share one +# Cargo target directory, and the CLI integration tests keep fixed-name scratch +# fixtures under `CARGO_TARGET_TMPDIR` that they wipe on entry, so two concurrent +# runs delete each other's. `FORCE_COLOR` is in no fingerprint, so the serialized +# second run rebuilds nothing and costs only its own test execution. [tasks."test:color"] description = "Run the test suite with terminal color forced on" +depends = ["test"] run = "cargo test --workspace" env = { FORCE_COLOR = "3" } From 1fe5ab049404b43227c9050727cd57908fd8f866 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:18:26 -0400 Subject: [PATCH 6/7] ci: gate the colour-forced suite in the hk check hook too `pre-push`, `mise run ci` and the Linux CI job all run the colour leg; `hk check` did not. That hook is documented in this file as the manual/CI entry point, so a contributor could pass `hk check`, believe the gate satisfied, and still be blocked at push -- two gates that claim to be the same telling them different things. It carries the same `depends` on the plain run, so it queues behind it here as well. --- hk.pkl | 1 + 1 file changed, 1 insertion(+) diff --git a/hk.pkl b/hk.pkl index 618b0f2..fac66a5 100644 --- a/hk.pkl +++ b/hk.pkl @@ -91,6 +91,7 @@ hooks { ["fmt"] = fmt ["clippy"] = clippy ["test"] = test + ["test-color"] = testColor } } ["fix"] { From b049cc0213f339323163e589dc041eba83a666f1 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:18:33 -0400 Subject: [PATCH 7/7] test(cli): split the ANSI stripper into a directly testable function `plain` took `(&DependencyGraph, &TreeOptions)`, so the only input it could ever receive was `ascii`'s own output -- and `ascii` emits nothing but CSI colour sequences. Three of the stripper's branches were therefore unreachable by any test: the non-CSI escape, a CSI final byte other than `m`, and a lone trailing `ESC`. The non-CSI branch also holds a latent wrong answer. It consumes the byte after `ESC` and drops it, then emits the remainder as text -- correct for a two-character escape, wrong for a string escape. If `label` ever wrapped a crate name in an OSC 8 hyperlink, the URL would be spliced onto the label and present as a tree-shape regression rather than as a limit of the stripper. Taking `&str` instead lets the branches be exercised with synthetic input, and states the contract the function actually implements rather than the narrower one its comment claimed. --- crates/dependable/src/output/tree.rs | 62 ++++++++++++++++++++++++---- 1 file changed, 53 insertions(+), 9 deletions(-) diff --git a/crates/dependable/src/output/tree.rs b/crates/dependable/src/output/tree.rs index 02cbee6..a456453 100644 --- a/crates/dependable/src/output/tree.rs +++ b/crates/dependable/src/output/tree.rs @@ -294,7 +294,7 @@ source = "registry+https://x" DependencyGraph::from_resolved(&resolved, &names, &["app".to_owned()]) } - /// `ascii` output with any terminal styling removed. + /// `raw` with any terminal styling removed. /// /// [`label`] colours through `if_supports_color`, which asks the ambient /// environment whether stdout can take ANSI — and `FORCE_COLOR`, which a @@ -303,8 +303,20 @@ source = "registry+https://x" /// tree, never on its colour, so a styled run must not fail them; stripping /// here is what states that, rather than leaving it to whatever the process /// happened to inherit. - fn plain(graph: &DependencyGraph, opts: &TreeOptions) -> String { - let raw = ascii(graph, opts); + /// + /// The contract is two escape forms, which is what `owo-colors` emits: + /// + /// - a CSI sequence, `ESC [` then parameter and intermediate bytes then a + /// final byte in `@..=~` — dropped whole, final byte included; + /// - any other escape, treated as the two-character form `ESC` + one byte + /// and dropped whole. + /// + /// A string escape (OSC, DCS, APC …) carries a payload terminated by BEL or + /// `ESC \\` rather than a single byte, so it is *not* in the contract: this + /// would drop its introducer and leave the payload as text. Nothing here + /// emits one; a styling path that starts to (a hyperlink, say) has to teach + /// this function about it. + fn strip_ansi(raw: &str) -> String { let mut out = String::with_capacity(raw.len()); let mut chars = raw.chars(); while let Some(c) = chars.next() { @@ -312,8 +324,6 @@ source = "registry+https://x" out.push(c); continue; } - // Only CSI sequences (`ESC [ … final`) are ever emitted here, and a - // CSI's final byte is the first in `@..=~` after the `[`. if chars.next() != Some('[') { continue; } @@ -326,9 +336,43 @@ source = "registry+https://x" out } + #[test] + fn strip_ansi_drops_a_csi_sequence_whole() { + assert_eq!( + strip_ansi("\u{1b}[36;1mserde\u{1b}[0m v1.0.0"), + "serde v1.0.0" + ); + } + + /// A CSI whose final byte is not `m`: still terminated by the first byte in + /// `@..=~`, because parameter (`0x30..=0x3F`) and intermediate (`0x20..=0x2F`) + /// bytes all sort below that range. + #[test] + fn strip_ansi_ends_a_csi_at_a_final_byte_other_than_m() { + assert_eq!(strip_ansi("a\u{1b}[2Kb\u{1b}[1;31Hc"), "abc"); + } + + /// A two-character escape — `ESC c` (RIS) — loses both characters. + #[test] + fn strip_ansi_drops_a_two_character_escape() { + assert_eq!(strip_ansi("a\u{1b}cb"), "ab"); + } + + /// A lone trailing `ESC` has nothing after it: the iterator ends and the + /// escape is dropped rather than emitted as text. + #[test] + fn strip_ansi_drops_a_lone_trailing_escape() { + assert_eq!(strip_ansi("serde\u{1b}"), "serde"); + } + + #[test] + fn strip_ansi_leaves_unstyled_text_alone() { + assert_eq!(strip_ansi("├── serde v1.0.0 (*)"), "├── serde v1.0.0 (*)"); + } + #[test] fn ascii_marks_workspace_and_dedupe() { - let out = plain(&sample(), &TreeOptions::default()); + let out = strip_ansi(&ascii(&sample(), &TreeOptions::default())); assert!(out.contains("app v0.1.0 (workspace)")); assert!(out.contains("├── serde v1.0.0")); assert!(out.contains("└── ")); // last-child connector @@ -360,7 +404,7 @@ source = "registry+https://x" #[test] fn ascii_points_a_member_at_its_own_tree() { - let out = plain(&workspace(), &TreeOptions::default()); + let out = strip_ansi(&ascii(&workspace(), &TreeOptions::default())); assert!( out.contains("└── lib v0.1.0 (workspace) (see root)"), "under `app`, `lib` is a pointer rather than a copy; {out}" @@ -383,7 +427,7 @@ source = "registry+https://x" collapse_roots: false, ..TreeOptions::default() }; - let out = plain(&workspace(), &opts); + let out = strip_ansi(&ascii(&workspace(), &opts)); assert!(!out.contains("(see root)"), "{out}"); assert_eq!(out.matches("serde v1.0.0").count(), 2, "{out}"); } @@ -395,7 +439,7 @@ source = "registry+https://x" dedupe: true, ..TreeOptions::default() }; - let out = plain(&sample(), &opts); + let out = strip_ansi(&ascii(&sample(), &opts)); assert!(out.contains("app v0.1.0 (workspace)")); assert!(!out.contains("serde")); }