From eb64e4929273daee9fd789a3e879fab53afa0a80 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 2 Aug 2026 12:54:11 +0900 Subject: [PATCH 1/3] fix(tests): honor CARGO_TARGET_DIR and clap's singular alias label The nightly `make verify` job has been red on `main` because of two independent defects in the integration test suite, both surfacing in `tests/cli_help_consistency.rs`. `repo_binary_path` in `tests/common/mod.rs` reconstructed the binary location as `CARGO_MANIFEST_DIR/target//`, which is wrong whenever `CARGO_TARGET_DIR` is set. Both `nightly-verify.yml` and `release.yml` set it to `$HOME/.cargo-target/mlxcel` so the self-hosted runner keeps a warm build cache outside the checkout, so cargo built the binaries there while the tests looked for them in the workspace. Every test in the file failed on CI with `No such file or directory` while passing locally, and the helper is shared by 12 other integration test files. It now prefers the compile-time `CARGO_BIN_EXE_` path cargo hands every integration test target, which already accounts for the profile, the target triple, and `CARGO_TARGET_DIR`, then falls back to a `current_exe()`-derived profile directory for binaries it does not name by hand, then to the previous reconstruction. The existing `if !binary.exists()` skip guards in the real-model tests keep working because the helper still returns a path unconditionally; the new `binary_resolution_report` lists every candidate and whether it exists, and the two spawn sites in the help test fold it into their panic so the next occurrence is diagnosable from the panic alone. `drafter_flag_aliases_are_documented_on_both_binaries` asserted on the literal `[aliases: X]`. `clap_builder` 4.6.2 introduced a `pluralize` call in its help template that renders `[alias: X]` for exactly one visible alias, and each of the four flags carries exactly one, so all four assertions broke on a dependency bump (`3ccb693a3`, #828) rather than a code change. All four aliases are still declared and still documented, so the fix belongs in the assertion, not in the CLI. It now locates the flag's own help entry, parses the alias names out of whatever alias annotation clap rendered, and matches on the name. Accepting either label keeps the assertion working across clap's singular/plural rendering and across a future second alias; scoping it to one entry and requiring a real annotation keeps it from being satisfied by prose, which matters because `--draft-model`'s description names `--model-draft` in its own text. Three tests cover the new assertion: a prose-only entry must not count as documented, the entry slicer must not spill into the neighbouring flag, and cutting the rendered annotation out of the live `mlxcel serve --help` must leave the alias undocumented. Validated with `cargo test --release --features metal,accelerate --test cli_help_consistency` (14 passed), the same command with `CARGO_TARGET_DIR` pointed at a directory outside the checkout, `cargo clippy --release --all-targets --features metal,accelerate -- -D warnings`, and `cargo fmt --all -- --check`. Rebuilding the CLI with `visible_alias = "model-draft"` removed turns the test red as intended, so the #464 contract is still pinned. Refs #962 --- tests/cli_help_consistency.rs | 244 +++++++++++++++++++++++++++++++--- tests/common/mod.rs | 114 ++++++++++++++-- 2 files changed, 333 insertions(+), 25 deletions(-) diff --git a/tests/cli_help_consistency.rs b/tests/cli_help_consistency.rs index ede7a8d6c..acb54fdc0 100644 --- a/tests/cli_help_consistency.rs +++ b/tests/cli_help_consistency.rs @@ -40,7 +40,7 @@ use std::process::Command; mod common; -use common::repo_binary_path; +use common::{binary_resolution_report, repo_binary_path}; const HEADING: &str = "KV Cache (TurboQuant) Options"; @@ -95,9 +95,12 @@ fn help_output(bin_name: &str, args: &[&str]) -> String { for key in ENV_TO_CLEAR_FOR_HELP { cmd.env_remove(key); } - let output = cmd - .output() - .unwrap_or_else(|e| panic!("failed to spawn {} from {:?}: {e}", bin_name, path)); + let output = cmd.output().unwrap_or_else(|e| { + panic!( + "failed to spawn {bin_name} from {path:?}: {e}\n{}", + binary_resolution_report(bin_name) + ) + }); assert!( output.status.success(), "{} {:?} exited with status {:?}: stderr=\n{}", @@ -358,9 +361,12 @@ fn help_output_for_speculative(bin_name: &str, args: &[&str]) -> String { for key in SPECULATIVE_ENV_TO_CLEAR_FOR_HELP { cmd.env_remove(key); } - let output = cmd - .output() - .unwrap_or_else(|e| panic!("failed to spawn {} from {:?}: {e}", bin_name, path)); + let output = cmd.output().unwrap_or_else(|e| { + panic!( + "failed to spawn {bin_name} from {path:?}: {e}\n{}", + binary_resolution_report(bin_name) + ) + }); assert!( output.status.success(), "{} {:?} exited with status {:?}: stderr=\n{}", @@ -467,28 +473,232 @@ fn speculative_flag_signatures_match_across_binaries() { // discovers the alternate spelling without reading the source. Unit tests // in `src/main_tests.rs` and `src/bin/mlx_server.rs` cover that the // aliases resolve to the identical parsed value. + +/// Leading-space count of a help line. +fn indent_width(line: &str) -> usize { + line.len() - line.trim_start_matches(' ').len() +} + +/// Slice the `--help` entry clap rendered for `flag_signature`: the signature +/// line plus every following line indented deeper than it, which is the +/// flag's description and its bracketed annotations. Stops before the next +/// flag entry (same or shallower indent) and before the next section heading. +/// +/// Scoping to one entry matters: the prose on `--draft-model` names the +/// alternate spelling in its own description, so a whole-help substring search +/// for the alias name would pass even with the `visible_alias` removed. +fn flag_help_entry<'a>(help: &'a str, flag_signature: &str) -> Option<&'a str> { + let mut offset = 0usize; + let mut entry_start = None; + for line in help.split_inclusive('\n') { + let trimmed = line.trim_start(); + // clap renders long-only flags as ` --flag ` and flags + // carrying a short form as ` -s, --flag `; accept both. + let starts_entry = trimmed.starts_with(flag_signature) + || trimmed.split_once(", ").is_some_and(|(short, rest)| { + short.starts_with('-') && rest.starts_with(flag_signature) + }); + if starts_entry { + entry_start = Some(offset); + break; + } + offset += line.len(); + } + let start = entry_start?; + let rest = &help[start..]; + + let mut lines = rest.split_inclusive('\n'); + let signature_line = lines.next()?; + let signature_indent = indent_width(signature_line); + + let mut consumed = signature_line.len(); + let mut body_indent: Option = None; + for line in lines { + if line.trim().is_empty() { + consumed += line.len(); + continue; + } + let indent = indent_width(line); + match body_indent { + // The first non-blank line fixes the body indent. A line no + // deeper than the signature means this flag has no body at all. + None if indent <= signature_indent => break, + None => body_indent = Some(indent), + Some(body) if indent < body => break, + Some(_) => {} + } + consumed += line.len(); + } + Some(&rest[..consumed]) +} + +/// Alias names clap documented for one flag entry, parsed out of its +/// `[alias: ...]` / `[aliases: ...]` annotation. +/// +/// Both labels are accepted because the label is not part of the contract: +/// `clap_builder` renders `[alias: ...]` for exactly one visible alias and +/// `[aliases: ...]` for two or more (the `pluralize` call in its help +/// template, added in 4.6.2). Pinning the singular literal instead would +/// break again the moment a second alias is added to any of these flags. +/// What the contract needs is the alias NAME, and that it is carried by a +/// real clap alias annotation rather than by prose. +fn documented_aliases(entry: &str) -> Vec { + for label in ["[aliases: ", "[alias: "] { + let Some(start) = entry.find(label) else { + continue; + }; + let inner_start = start + label.len(); + let Some(len) = entry[inner_start..].find(']') else { + continue; + }; + return entry[inner_start..inner_start + len] + .split(',') + // The annotation wraps across lines when it is long, so collapse + // whitespace rather than trimming spaces only. + .map(|alias| alias.split_whitespace().collect::>().join(" ")) + .filter(|alias| !alias.is_empty()) + .collect(); + } + Vec::new() +} + +/// Assert one binary's `--help` documents `alias` as a clap alias of the flag +/// rendered as `flag_signature`. +fn assert_flag_documents_alias(label: &str, help: &str, flag_signature: &str, alias: &str) { + let entry = flag_help_entry(help, flag_signature).unwrap_or_else(|| { + panic!("{label} --help has no flag entry for {flag_signature:?}.\nHelp was:\n{help}") + }); + let aliases = documented_aliases(entry); + assert!( + aliases.iter().any(|documented| documented == alias), + "{label} --help must document {flag_signature:?} with a {alias:?} alias (issue #464). \ + The alias annotation clap rendered for that flag listed {aliases:?}.\n\ + Entry was:\n{entry}" + ); +} + #[test] fn drafter_flag_aliases_are_documented_on_both_binaries() { let serve_help = help_output("mlxcel", &["serve", "--help"]); let server_help = help_output("mlxcel-server", &["--help"]); + assert_flag_documents_alias( + "mlxcel serve", + &serve_help, + "--draft-model ", + "--model-draft", + ); + assert_flag_documents_alias( + "mlxcel serve", + &serve_help, + "--draft-max ", + "--draft", + ); + + assert_flag_documents_alias( + "mlxcel-server", + &server_help, + "--model-draft ", + "--draft-model", + ); + assert_flag_documents_alias( + "mlxcel-server", + &server_help, + "--draft ", + "--draft-max", + ); +} + +/// Guard for the guard: the alias assertion above must reject an entry whose +/// clap alias annotation is missing, even when the alias name still appears +/// in that flag's prose. `--draft-model`'s real help does exactly that, so a +/// whole-help substring search would not catch a dropped `visible_alias`. +#[test] +fn documented_aliases_ignores_alias_names_that_only_appear_in_prose() { + let without_annotation = " --draft-model \n \ + Accepts the mlx-lm-style `--draft-model` spelling (primary) and the \ + llama-server-style `--model-draft` spelling.\n\n [default: none]\n"; + assert!( + documented_aliases(without_annotation).is_empty(), + "prose mentioning an alias must not count as a documented alias" + ); + + let singular = + " --draft-max \n Max draft tokens\n\n [alias: --draft]\n"; + assert_eq!(documented_aliases(singular), vec!["--draft".to_string()]); + + let plural = " --draft-max \n Max draft tokens\n\n [aliases: --draft, --n-draft]\n"; + assert_eq!( + documented_aliases(plural), + vec!["--draft".to_string(), "--n-draft".to_string()] + ); +} + +/// Mutation check on the real help text: dropping a `visible_alias` from the +/// CLI removes exactly one rendered `[alias: ...]` / `[aliases: ...]` +/// annotation, so cut that annotation out of the live `mlxcel serve --help` +/// and confirm the alias is then undocumented. Answers "would a build that +/// dropped one of the four aliases still fail?" against real output, without +/// rebuilding the CLI with the attribute removed. +/// +/// `--draft-model` is the interesting case: its own description names +/// `--model-draft` in prose, so this also shows the prose alone does not +/// satisfy the contract. +#[test] +fn removing_the_rendered_alias_annotation_leaves_the_alias_undocumented() { + let serve_help = help_output("mlxcel", &["serve", "--help"]); + let entry = flag_help_entry(&serve_help, "--draft-model ") + .expect("mlxcel serve --help has no --draft-model entry"); + assert_eq!( + documented_aliases(entry), + vec!["--model-draft".to_string()], + "precondition: the live help must document exactly the one alias" + ); + + let annotation_start = entry + .find("[alias") + .expect("precondition: the entry carries an alias annotation"); + let annotation_end = annotation_start + + entry[annotation_start..] + .find(']') + .expect("alias annotation is unterminated") + + 1; + let without_annotation = format!("{}{}", &entry[..annotation_start], &entry[annotation_end..]); + assert!( - serve_help.contains("--draft-model ") - && serve_help.contains("[aliases: --model-draft]"), - "mlxcel serve --help must document --draft-model with a --model-draft alias.\nHelp was:\n{serve_help}" + without_annotation.contains("--model-draft"), + "precondition: the alias name still appears in the flag's prose" ); assert!( - serve_help.contains("--draft-max ") && serve_help.contains("[aliases: --draft]"), - "mlxcel serve --help must document --draft-max with a --draft alias.\nHelp was:\n{serve_help}" + documented_aliases(&without_annotation).is_empty(), + "a dropped visible_alias must leave the alias undocumented, but the \ + assertion still found: {:?}", + documented_aliases(&without_annotation) ); +} +/// The entry slicer must not spill into the neighbouring flag: an alias +/// annotation on the *next* flag must never satisfy the assertion for this +/// one. +#[test] +fn flag_help_entry_stops_at_the_next_flag() { + let help = "Options:\n \ + --draft-model \n Path to drafter checkpoint\n\n \ + --draft-max \n Max draft tokens\n\n [alias: --draft]\n\n\ + Other Options:\n --unrelated\n"; + + let entry = flag_help_entry(help, "--draft-model ").expect("entry not found"); + assert!(entry.contains("Path to drafter checkpoint")); assert!( - server_help.contains("--model-draft ") - && server_help.contains("[aliases: --draft-model]"), - "mlxcel-server --help must document --model-draft with a --draft-model alias.\nHelp was:\n{server_help}" + !entry.contains("--draft-max"), + "entry leaked into the next flag:\n{entry}" ); + assert!(documented_aliases(entry).is_empty()); + + let entry = flag_help_entry(help, "--draft-max ").expect("entry not found"); + assert_eq!(documented_aliases(entry), vec!["--draft".to_string()]); assert!( - server_help.contains("--draft ") && server_help.contains("[aliases: --draft-max]"), - "mlxcel-server --help must document --draft with a --draft-max alias.\nHelp was:\n{server_help}" + !entry.contains("Other Options:"), + "entry leaked into the next section:\n{entry}" ); } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 23f1ba13c..d9ce99699 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; #[allow(dead_code)] pub fn repo_model_dir(name: &str) -> PathBuf { @@ -19,20 +19,118 @@ pub fn repo_model_dir(name: &str) -> PathBuf { primary } -#[allow(dead_code)] -pub fn repo_binary_path(name: &str) -> PathBuf { - let env_key = format!("CARGO_BIN_EXE_{name}"); - if let Some(path) = std::env::var_os(&env_key) { - return PathBuf::from(path); +/// Candidate locations for one of the crate's binaries, most authoritative +/// first. Each entry carries a short label naming where the path came from so +/// a failure can be diagnosed from the panic message alone. +/// +/// The list exists because `/target//` is not where +/// the binaries live whenever `CARGO_TARGET_DIR` is set. Both `release.yml` +/// and `nightly-verify.yml` set it to `$HOME/.cargo-target/mlxcel` so the +/// self-hosted runner keeps a warm build cache outside the checkout, which is +/// how every test in `tests/cli_help_consistency.rs` came to fail on CI with +/// `No such file or directory` while passing locally (see issue #962). +fn binary_path_candidates(name: &str) -> Vec<(&'static str, PathBuf)> { + let mut candidates: Vec<(&'static str, PathBuf)> = Vec::new(); + + // 1. Runtime override. Cargo never sets this at test runtime, so it only + // fires when a harness or an operator points the tests at a + // pre-built binary on purpose. + if let Some(path) = std::env::var_os(format!("CARGO_BIN_EXE_{name}")) { + candidates.push(("CARGO_BIN_EXE_* (runtime env)", PathBuf::from(path))); } - let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + // 2. The compile-time path cargo hands every integration test target. It + // already accounts for the profile, the target triple, and + // `CARGO_TARGET_DIR`, so it needs no reconstruction and cannot drift. + let compile_time: Option<&'static str> = match name { + "mlxcel" => Some(env!("CARGO_BIN_EXE_mlxcel")), + "mlxcel-server" => Some(env!("CARGO_BIN_EXE_mlxcel-server")), + "mlxcel-bench-decode" => Some(env!("CARGO_BIN_EXE_mlxcel-bench-decode")), + _ => None, + }; + if let Some(path) = compile_time { + candidates.push(("CARGO_BIN_EXE_* (compile time)", PathBuf::from(path))); + } + + // 3. Derived from this test binary's own location. Integration tests are + // linked into `/[/]/deps/`, so the profile + // directory holding the package binaries is two levels up. This covers + // binaries not named above without hardcoding a target directory. + let derived_profile_dir = std::env::current_exe().ok().and_then(|test_exe| { + test_exe + .parent() + .and_then(|deps| deps.parent()) + .map(Path::to_path_buf) + }); + if let Some(profile_dir) = derived_profile_dir { + candidates.push(("derived from current_exe()", profile_dir.join(name))); + } + + // 4. Last resort: reconstruct from `CARGO_TARGET_DIR` when set, else from + // the manifest directory. Kept so a stripped-down invocation still has + // something to try, not relied on by the paths above. + let target_dir = std::env::var_os("CARGO_TARGET_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("target")); let profile = if cfg!(debug_assertions) { "debug" } else { "release" }; - manifest_dir.join("target").join(profile).join(name) + candidates.push(( + "reconstructed from CARGO_TARGET_DIR / CARGO_MANIFEST_DIR", + target_dir.join(profile).join(name), + )); + + candidates +} + +/// Resolve the path to one of the crate's binaries for an integration test. +/// +/// Returns the first candidate from [`binary_path_candidates`] that exists on +/// disk, falling back to the most authoritative candidate when none do. The +/// caller-visible behaviour of returning a path unconditionally is preserved +/// so the existing `if !binary.exists() { skip }` guards in the real-model +/// integration tests keep working; [`binary_resolution_report`] supplies the +/// diagnostics for the paths that spawn unconditionally. +#[allow(dead_code)] +pub fn repo_binary_path(name: &str) -> PathBuf { + let candidates = binary_path_candidates(name); + for (_, path) in &candidates { + if path.exists() { + return path.clone(); + } + } + candidates + .into_iter() + .next() + .map(|(_, path)| path) + .unwrap_or_else(|| PathBuf::from(name)) +} + +/// Human-readable account of how [`repo_binary_path`] resolved `name`, for +/// use in panic messages when spawning the binary fails. Names every +/// candidate, whether it exists, and the target directory the reconstruction +/// fallback derived, so a CI failure is diagnosable without reproducing the +/// environment. +#[allow(dead_code)] +pub fn binary_resolution_report(name: &str) -> String { + let mut report = format!("binary resolution for {name:?}:\n"); + for (source, path) in binary_path_candidates(name) { + let state = if path.exists() { "exists" } else { "missing" }; + report.push_str(&format!(" [{state}] {source}: {}\n", path.display())); + } + let configured_target_dir = std::env::var_os("CARGO_TARGET_DIR").map(PathBuf::from); + report.push_str(&match configured_target_dir { + Some(dir) => format!(" CARGO_TARGET_DIR is set to {}", dir.display()), + None => format!( + " CARGO_TARGET_DIR is unset; reconstruction assumed {}", + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("target") + .display() + ), + }); + report } #[allow(dead_code)] From d32f684ca889f0117adb95cf1d9ece0921839d50 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 2 Aug 2026 13:21:07 +0900 Subject: [PATCH 2/3] fix(tests): tighten binary resolution and alias-entry matching Review hardening on top of the previous commit. Both halves had ways to be silently wrong that the passing tests did not expose. Binary resolution now consults only signals cargo itself produces. The runtime `CARGO_BIN_EXE_*` lookup is gone: cargo never sets it at test runtime, so it was dead code, and keeping it as an override hook would let a stale exported value beat the binary cargo just built, which is the same shape as the bug this ordering exists to fix. The `/target//` reconstruction is gone too: besides being wrong under `CARGO_TARGET_DIR`, it cannot know the target triple under `--target`, and its `cfg!(debug_assertions)` profile guess is wrong for a custom profile such as `--profile test-fast`, so it could hand back a stale binary from an unrelated build. What remains is the compile-time `CARGO_BIN_EXE_*` path and a `current_exe()`-derived profile directory that now verifies its parent really is `deps` rather than assuming the layout, so a test binary running from a copied location cannot spawn an unrelated sibling that happens to share the name. `resolve_repo_binary` returns the path and the diagnostic report from one pass, marking the candidate it selected, so the report can no longer contradict the choice it is explaining, and the help test stats the filesystem once instead of twice. `flag_help_entry` matched the signature by prefix against any line. That is not sound as a general helper: measured against the live help, `--metrics` prefix-matches the separate `--metrics-port ` entry and an example command line in `after_long_help`. It now matches the whole trimmed line, which is exactly how clap's long help renders a signature, and the short-form branch requires a real two-character short flag so a hyphen-bulleted description line cannot anchor the slice on prose. `documented_aliases` searched the whole entry for a bare `[alias: ` / `[aliases: ` substring, so a bracketed span written by hand in a doc comment would have satisfied the #464 contract with no `visible_alias` attribute behind it. The help output already carries prose brackets on other flags, so this was a live foot-gun rather than a theoretical one. The annotation must now begin a line, which is how clap always renders it, and the parser still handles an annotation that wraps across lines. New coverage: `cli_binaries_resolve_to_the_path_cargo_built_them_at` is the regression test the resolution defect never had, asserting the resolved path is the one cargo built rather than a reconstruction, which holds under `CARGO_TARGET_DIR`, a custom profile, and a cross build alike. `flag_help_entry_ignores_hyphen_bulleted_prose` and a prefix-sibling case in `flag_help_entry_stops_at_the_next_flag` cover the matcher; an inline-prose bracket case and a wrapped-annotation case cover the parser. The mutation test now splices the annotation-stripped entry back into the full help and re-runs the entry lookup, so it exercises the whole path rather than only the parser. Validated with `cargo test --release --features metal,accelerate --test cli_help_consistency` (16 passed), the same under `CARGO_TARGET_DIR` outside the checkout, and `cargo clippy --release --all-targets --features metal,accelerate -- -D warnings`. Rebuilding `mlxcel-server` with `visible_alias = "draft-max"` removed turns the test red on the `--draft ` assertion, confirming the tightened form still pins the contract. Refs #962 --- tests/cli_help_consistency.rs | 169 ++++++++++++++++++++++++++-------- tests/common/mod.rs | 165 ++++++++++++++++++--------------- 2 files changed, 220 insertions(+), 114 deletions(-) diff --git a/tests/cli_help_consistency.rs b/tests/cli_help_consistency.rs index acb54fdc0..b6582574b 100644 --- a/tests/cli_help_consistency.rs +++ b/tests/cli_help_consistency.rs @@ -37,10 +37,11 @@ //! When a future flag or mode is added to either shared group, all three //! binaries fail the test together, no drift is possible. +use std::path::Path; use std::process::Command; mod common; -use common::{binary_resolution_report, repo_binary_path}; +use common::resolve_repo_binary; const HEADING: &str = "KV Cache (TurboQuant) Options"; @@ -89,18 +90,15 @@ const ENV_TO_CLEAR_FOR_HELP: &[&str] = &["LLAMA_ARG_CACHE_TYPE_K", "LLAMA_ARG_CA /// Run `--help` on a binary and return the resulting stdout. Panics with a /// descriptive message when the binary fails to execute. fn help_output(bin_name: &str, args: &[&str]) -> String { - let path = repo_binary_path(bin_name); + let (path, resolution) = resolve_repo_binary(bin_name); let mut cmd = Command::new(&path); cmd.args(args); for key in ENV_TO_CLEAR_FOR_HELP { cmd.env_remove(key); } - let output = cmd.output().unwrap_or_else(|e| { - panic!( - "failed to spawn {bin_name} from {path:?}: {e}\n{}", - binary_resolution_report(bin_name) - ) - }); + let output = cmd + .output() + .unwrap_or_else(|e| panic!("failed to spawn {bin_name} from {path:?}: {e}\n{resolution}")); assert!( output.status.success(), "{} {:?} exited with status {:?}: stderr=\n{}", @@ -355,18 +353,15 @@ fn extract_speculative_block(help: &str) -> &str { /// Run `--help` on a binary with the speculative env vars cleared and /// return stdout. fn help_output_for_speculative(bin_name: &str, args: &[&str]) -> String { - let path = repo_binary_path(bin_name); + let (path, resolution) = resolve_repo_binary(bin_name); let mut cmd = Command::new(&path); cmd.args(args); for key in SPECULATIVE_ENV_TO_CLEAR_FOR_HELP { cmd.env_remove(key); } - let output = cmd.output().unwrap_or_else(|e| { - panic!( - "failed to spawn {bin_name} from {path:?}: {e}\n{}", - binary_resolution_report(bin_name) - ) - }); + let output = cmd + .output() + .unwrap_or_else(|e| panic!("failed to spawn {bin_name} from {path:?}: {e}\n{resolution}")); assert!( output.status.success(), "{} {:?} exited with status {:?}: stderr=\n{}", @@ -484,6 +479,10 @@ fn indent_width(line: &str) -> usize { /// flag's description and its bracketed annotations. Stops before the next /// flag entry (same or shallower indent) and before the next section heading. /// +/// `flag_signature` must be the complete rendered signature including the +/// value name, for example `--draft-model `, because the anchor line is +/// matched in full. +/// /// Scoping to one entry matters: the prose on `--draft-model` names the /// alternate spelling in its own description, so a whole-help substring search /// for the alias name would pass even with the `visible_alias` removed. @@ -491,12 +490,17 @@ fn flag_help_entry<'a>(help: &'a str, flag_signature: &str) -> Option<&'a str> { let mut offset = 0usize; let mut entry_start = None; for line in help.split_inclusive('\n') { - let trimmed = line.trim_start(); - // clap renders long-only flags as ` --flag ` and flags - // carrying a short form as ` -s, --flag `; accept both. - let starts_entry = trimmed.starts_with(flag_signature) + let trimmed = line.trim(); + // clap's long help puts the signature alone on its line, as + // ` --flag ` for a long-only flag or ` -s, --flag ` + // for one carrying a short form. Match the whole line rather than a + // prefix: `--metrics` is a prefix of the separate `--metrics-port + // ` entry, and a description line that merely mentions the flag + // would otherwise anchor the slice on prose. A short form is always + // exactly two characters, which rules out a hyphen-bulleted line. + let starts_entry = trimmed == flag_signature || trimmed.split_once(", ").is_some_and(|(short, rest)| { - short.starts_with('-') && rest.starts_with(flag_signature) + short.len() == 2 && short.starts_with('-') && rest == flag_signature }); if starts_entry { entry_start = Some(offset); @@ -535,6 +539,12 @@ fn flag_help_entry<'a>(help: &'a str, flag_signature: &str) -> Option<&'a str> { /// Alias names clap documented for one flag entry, parsed out of its /// `[alias: ...]` / `[aliases: ...]` annotation. /// +/// The annotation must START a line, which is how clap renders it (on its own +/// line at the description indent). A bare substring search would also accept +/// a bracketed span someone wrote by hand inside a doc comment, and the help +/// output already contains such spans on other flags, so the anchor is what +/// keeps a prose `[alias: --foo]` from silently satisfying the contract. +/// /// Both labels are accepted because the label is not part of the contract: /// `clap_builder` renders `[alias: ...]` for exactly one visible alias and /// `[aliases: ...]` for two or more (the `pluralize` call in its help @@ -543,21 +553,27 @@ fn flag_help_entry<'a>(help: &'a str, flag_signature: &str) -> Option<&'a str> { /// What the contract needs is the alias NAME, and that it is carried by a /// real clap alias annotation rather than by prose. fn documented_aliases(entry: &str) -> Vec { - for label in ["[aliases: ", "[alias: "] { - let Some(start) = entry.find(label) else { - continue; - }; - let inner_start = start + label.len(); - let Some(len) = entry[inner_start..].find(']') else { - continue; - }; - return entry[inner_start..inner_start + len] - .split(',') - // The annotation wraps across lines when it is long, so collapse - // whitespace rather than trimming spaces only. - .map(|alias| alias.split_whitespace().collect::>().join(" ")) - .filter(|alias| !alias.is_empty()) - .collect(); + let mut offset = 0usize; + for line in entry.split_inclusive('\n') { + let trimmed = line.trim_start(); + let trimmed_offset = offset + (line.len() - trimmed.len()); + offset += line.len(); + for label in ["[aliases: ", "[alias: "] { + if !trimmed.starts_with(label) { + continue; + } + let inner_start = trimmed_offset + label.len(); + let Some(len) = entry[inner_start..].find(']') else { + continue; + }; + return entry[inner_start..inner_start + len] + .split(',') + // The annotation wraps across lines when it is long, so + // collapse whitespace rather than trimming spaces only. + .map(|alias| alias.split_whitespace().collect::>().join(" ")) + .filter(|alias| !alias.is_empty()) + .collect(); + } } Vec::new() } @@ -577,6 +593,28 @@ fn assert_flag_documents_alias(label: &str, help: &str, flag_signature: &str, al ); } +/// Regression test for the defect that actually took the nightly down: the +/// CLI binaries must be resolved from the path cargo built them at, never from +/// a `/target//` reconstruction. `CARGO_BIN_EXE_*` accounts +/// for `CARGO_TARGET_DIR`, the profile name, and the target triple, so this +/// holds for a plain run, for the nightly's external target directory, for +/// `--profile test-fast`, and for a cross build (issue #962). +#[test] +fn cli_binaries_resolve_to_the_path_cargo_built_them_at() { + for (name, built_by_cargo) in [ + ("mlxcel", env!("CARGO_BIN_EXE_mlxcel")), + ("mlxcel-server", env!("CARGO_BIN_EXE_mlxcel-server")), + ] { + let (resolved, report) = resolve_repo_binary(name); + assert_eq!( + resolved, + Path::new(built_by_cargo), + "{name} must resolve to the binary cargo built, not a reconstructed path.\n{report}" + ); + assert!(resolved.exists(), "{name} was not built.\n{report}"); + } +} + #[test] fn drafter_flag_aliases_are_documented_on_both_binaries() { let serve_help = help_output("mlxcel", &["serve", "--help"]); @@ -632,6 +670,25 @@ fn documented_aliases_ignores_alias_names_that_only_appear_in_prose() { documented_aliases(plural), vec!["--draft".to_string(), "--n-draft".to_string()] ); + + // A bracketed span written by hand mid-sentence must not count either. + // The real help already carries prose brackets on other flags (for + // example `[llama-server alias for --prefill-chunk-size]`), so an + // unanchored substring search would let a doc comment satisfy the + // contract with no `visible_alias` attribute behind it. + let inline_prose = + " --draft-max \n Max draft tokens [alias: --draft] per step\n"; + assert!( + documented_aliases(inline_prose).is_empty(), + "a bracketed span inside prose must not count as a clap alias annotation" + ); + + // The annotation still parses when clap wraps it across lines. + let wrapped = " --draft-max \n Max draft tokens\n\n [aliases: --draft,\n --n-draft]\n"; + assert_eq!( + documented_aliases(wrapped), + vec!["--draft".to_string(), "--n-draft".to_string()] + ); } /// Mutation check on the real help text: dropping a `visible_alias` from the @@ -665,15 +722,21 @@ fn removing_the_rendered_alias_annotation_leaves_the_alias_undocumented() { + 1; let without_annotation = format!("{}{}", &entry[..annotation_start], &entry[annotation_end..]); + // Splice the mutated entry back into the full help so the whole + // resolution path runs, not just the annotation parser. + let mutated_help = serve_help.replace(entry, &without_annotation); + let mutated_entry = flag_help_entry(&mutated_help, "--draft-model ") + .expect("the flag entry itself must survive the mutation"); + assert!( - without_annotation.contains("--model-draft"), + mutated_entry.contains("--model-draft"), "precondition: the alias name still appears in the flag's prose" ); assert!( - documented_aliases(&without_annotation).is_empty(), + documented_aliases(mutated_entry).is_empty(), "a dropped visible_alias must leave the alias undocumented, but the \ - assertion still found: {:?}", - documented_aliases(&without_annotation) + assertion still found: {:?}\nEntry was:\n{mutated_entry}", + documented_aliases(mutated_entry) ); } @@ -701,4 +764,32 @@ fn flag_help_entry_stops_at_the_next_flag() { !entry.contains("Other Options:"), "entry leaked into the next section:\n{entry}" ); + + // A signature that is only a prefix of a real entry must not resolve to + // it. `--draft` is a prefix of `--draft-max `, and in the live + // help `--metrics` is a prefix of the separate `--metrics-port `. + assert!( + flag_help_entry(help, "--draft").is_none(), + "a prefix of another flag's signature must not match its entry" + ); +} + +/// A hyphen-bulleted description line must not be mistaken for a flag entry. +/// The short-form branch of the matcher accepts `-s, --flag `, and +/// without the two-character constraint a bullet such as `- foo, --draft-max +/// ...` would anchor the entry on prose and pick up whatever +/// followed it. +#[test] +fn flag_help_entry_ignores_hyphen_bulleted_prose() { + let help = "Options:\n \ + --other \n Describes something:\n \ + - see also, --draft-max for the token budget\n\n \ + --draft-max \n Max draft tokens\n\n [alias: --draft]\n"; + + let entry = flag_help_entry(help, "--draft-max ").expect("entry not found"); + assert!( + entry.starts_with(" --draft-max "), + "matcher anchored on a prose bullet instead of the flag entry:\n{entry}" + ); + assert_eq!(documented_aliases(entry), vec!["--draft".to_string()]); } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index d9ce99699..b208b650e 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,3 +1,4 @@ +use std::ffi::OsStr; use std::path::{Path, PathBuf}; #[allow(dead_code)] @@ -20,28 +21,36 @@ pub fn repo_model_dir(name: &str) -> PathBuf { } /// Candidate locations for one of the crate's binaries, most authoritative -/// first. Each entry carries a short label naming where the path came from so -/// a failure can be diagnosed from the panic message alone. +/// first, each labelled with where the path came from so a failure is +/// diagnosable from the panic message alone. /// -/// The list exists because `/target//` is not where -/// the binaries live whenever `CARGO_TARGET_DIR` is set. Both `release.yml` -/// and `nightly-verify.yml` set it to `$HOME/.cargo-target/mlxcel` so the -/// self-hosted runner keeps a warm build cache outside the checkout, which is -/// how every test in `tests/cli_help_consistency.rs` came to fail on CI with -/// `No such file or directory` while passing locally (see issue #962). +/// The list is deliberately short, and every entry is a signal cargo itself +/// produces. What used to be here, `/target//`, is +/// gone: it is wrong whenever `CARGO_TARGET_DIR` is set, which is how every +/// test in `tests/cli_help_consistency.rs` came to fail on CI with `No such +/// file or directory` while passing locally (issue #962). `release.yml` and +/// `nightly-verify.yml` both set it to `$HOME/.cargo-target/mlxcel` so the +/// self-hosted runner keeps a warm build cache outside the checkout. It is +/// also silently wrong under `--target`, where the binaries live under an +/// extra triple segment it cannot know, and its `cfg!(debug_assertions)` +/// profile guess is only a proxy for the real profile name. A candidate that +/// can hand back a stale binary from an unrelated build is worse than no +/// candidate at all. +/// +/// The runtime environment is deliberately not consulted either. Cargo never +/// sets `CARGO_BIN_EXE_*` at test runtime, so the previous `var_os` lookup was +/// dead code, and honouring it would let a stale exported value win over the +/// binary cargo just built, which is the same shape as the bug this ordering +/// exists to fix. fn binary_path_candidates(name: &str) -> Vec<(&'static str, PathBuf)> { let mut candidates: Vec<(&'static str, PathBuf)> = Vec::new(); - // 1. Runtime override. Cargo never sets this at test runtime, so it only - // fires when a harness or an operator points the tests at a - // pre-built binary on purpose. - if let Some(path) = std::env::var_os(format!("CARGO_BIN_EXE_{name}")) { - candidates.push(("CARGO_BIN_EXE_* (runtime env)", PathBuf::from(path))); - } - - // 2. The compile-time path cargo hands every integration test target. It + // 1. The compile-time path cargo hands every integration test target. It // already accounts for the profile, the target triple, and - // `CARGO_TARGET_DIR`, so it needs no reconstruction and cannot drift. + // `CARGO_TARGET_DIR`, and cargo builds the binary before running the + // test, so for the binaries named here it is authoritative and needs no + // reconstruction. This is the same mechanism `tests/surgery_cli.rs` and + // `tests/lang_bias.rs` already use directly. let compile_time: Option<&'static str> = match name { "mlxcel" => Some(env!("CARGO_BIN_EXE_mlxcel")), "mlxcel-server" => Some(env!("CARGO_BIN_EXE_mlxcel-server")), @@ -52,85 +61,91 @@ fn binary_path_candidates(name: &str) -> Vec<(&'static str, PathBuf)> { candidates.push(("CARGO_BIN_EXE_* (compile time)", PathBuf::from(path))); } - // 3. Derived from this test binary's own location. Integration tests are - // linked into `/[/]/deps/`, so the profile - // directory holding the package binaries is two levels up. This covers - // binaries not named above without hardcoding a target directory. + // 2. Derived from this test binary's own location, covering any binary not + // named above. Integration tests link into + // `/[/]/deps/`, so the directory holding + // the package binaries is two levels up. The `deps` check enforces that + // layout rather than assuming it: a test binary running from a copied + // location (a nextest archive, an extracted CI artifact) must not spawn + // whatever sibling file happens to share the binary's name. let derived_profile_dir = std::env::current_exe().ok().and_then(|test_exe| { test_exe .parent() - .and_then(|deps| deps.parent()) + .filter(|deps| deps.file_name() == Some(OsStr::new("deps"))) + .and_then(Path::parent) .map(Path::to_path_buf) }); if let Some(profile_dir) = derived_profile_dir { candidates.push(("derived from current_exe()", profile_dir.join(name))); } - // 4. Last resort: reconstruct from `CARGO_TARGET_DIR` when set, else from - // the manifest directory. Kept so a stripped-down invocation still has - // something to try, not relied on by the paths above. - let target_dir = std::env::var_os("CARGO_TARGET_DIR") - .map(PathBuf::from) - .unwrap_or_else(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("target")); - let profile = if cfg!(debug_assertions) { - "debug" - } else { - "release" - }; - candidates.push(( - "reconstructed from CARGO_TARGET_DIR / CARGO_MANIFEST_DIR", - target_dir.join(profile).join(name), - )); - candidates } -/// Resolve the path to one of the crate's binaries for an integration test. +/// Resolve one of the crate's binaries, returning both the path and a report +/// of how it was chosen. /// -/// Returns the first candidate from [`binary_path_candidates`] that exists on -/// disk, falling back to the most authoritative candidate when none do. The -/// caller-visible behaviour of returning a path unconditionally is preserved -/// so the existing `if !binary.exists() { skip }` guards in the real-model -/// integration tests keep working; [`binary_resolution_report`] supplies the -/// diagnostics for the paths that spawn unconditionally. +/// Both halves come from a single pass over the candidates, so the report can +/// never contradict the choice it is explaining, and the caller stats the +/// filesystem once rather than twice. #[allow(dead_code)] -pub fn repo_binary_path(name: &str) -> PathBuf { +pub fn resolve_repo_binary(name: &str) -> (PathBuf, String) { let candidates = binary_path_candidates(name); - for (_, path) in &candidates { - if path.exists() { - return path.clone(); - } - } - candidates - .into_iter() - .next() - .map(|(_, path)| path) - .unwrap_or_else(|| PathBuf::from(name)) -} + let selected = candidates + .iter() + .position(|(_, path)| path.exists()) + .or(if candidates.is_empty() { None } else { Some(0) }); -/// Human-readable account of how [`repo_binary_path`] resolved `name`, for -/// use in panic messages when spawning the binary fails. Names every -/// candidate, whether it exists, and the target directory the reconstruction -/// fallback derived, so a CI failure is diagnosable without reproducing the -/// environment. -#[allow(dead_code)] -pub fn binary_resolution_report(name: &str) -> String { let mut report = format!("binary resolution for {name:?}:\n"); - for (source, path) in binary_path_candidates(name) { + if candidates.is_empty() { + report.push_str( + " (no candidate: this binary is not one the helper names, and the \ + running test binary is not inside a cargo `deps` directory)\n", + ); + } + for (index, (source, path)) in candidates.iter().enumerate() { let state = if path.exists() { "exists" } else { "missing" }; - report.push_str(&format!(" [{state}] {source}: {}\n", path.display())); + let marker = if Some(index) == selected { + " <- selected" + } else { + "" + }; + report.push_str(&format!( + " [{state}] {source}: {}{marker}\n", + path.display() + )); } - let configured_target_dir = std::env::var_os("CARGO_TARGET_DIR").map(PathBuf::from); - report.push_str(&match configured_target_dir { - Some(dir) => format!(" CARGO_TARGET_DIR is set to {}", dir.display()), - None => format!( - " CARGO_TARGET_DIR is unset; reconstruction assumed {}", - PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .join("target") - .display() + report.push_str(&match std::env::var_os("CARGO_TARGET_DIR") { + Some(dir) => format!( + " CARGO_TARGET_DIR is set to {}", + PathBuf::from(dir).display() ), + None => " CARGO_TARGET_DIR is unset".to_string(), }); - report + + let path = selected + .map(|index| candidates[index].1.clone()) + // Nothing to go on. Hand back a path that deliberately does not exist, + // so the caller's spawn fails with this report attached, rather than a + // bare name, which would be resolved through `PATH`. + .unwrap_or_else(|| { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("target") + .join(name) + }); + (path, report) +} + +/// Path to one of the crate's binaries. +/// +/// Returns the first candidate that exists, and otherwise the most +/// authoritative one. It still returns a path unconditionally, so the +/// `if !binary.exists() { skip }` guards in the real-model integration tests +/// keep compiling and behaving the same way. Callers that spawn without such a +/// guard should use [`resolve_repo_binary`] and put the report in their panic. +#[allow(dead_code)] +pub fn repo_binary_path(name: &str) -> PathBuf { + resolve_repo_binary(name).0 } #[allow(dead_code)] From 0285b6f0c335488ac120677284fccf4efd9ce8dd Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 2 Aug 2026 13:37:13 +0900 Subject: [PATCH 3/3] test(cli): cover the deps-derived binary resolution branch resolve_repo_binary's second candidate, a binary path derived from the running test binary's own deps/ location, only fires for a binary name the compile-time CARGO_BIN_EXE_* match arm does not list. Every existing test in this file, including the new cli_binaries_resolve_to_the_path_cargo_built_them_at regression test, only calls resolve_repo_binary with "mlxcel" or "mlxcel-server", both of which always resolve through the compile-time candidate, so the deps-parent layout check added alongside this branch was never exercised. speculative_bench is a real fourth [[bin]] target in Cargo.toml that no CARGO_BIN_EXE_* case names and that no integration test spawns, making it the live binary that takes this code path. The new test resolves it and asserts the result sits in the same profile directory as the binaries CARGO_BIN_EXE_* names directly. Also reviewed CONTRIBUTING.md, docs/architecture.md, and the docs/ tree for anything documenting CARGO_TARGET_DIR or how integration tests locate binaries; none exists, so no documentation needed updating for this test-harness-only change. Validation: - cargo fmt --all -- --check: clean. - DEVELOPER_DIR=/Applications/Xcode-26.6.0.app/Contents/Developer cargo test --release --features metal,accelerate --test cli_help_consistency: 17 passed. Refs #962 --- tests/cli_help_consistency.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/cli_help_consistency.rs b/tests/cli_help_consistency.rs index b6582574b..99439b98c 100644 --- a/tests/cli_help_consistency.rs +++ b/tests/cli_help_consistency.rs @@ -615,6 +615,34 @@ fn cli_binaries_resolve_to_the_path_cargo_built_them_at() { } } +/// Coverage for the other candidate `binary_path_candidates` produces: a +/// binary the compile-time match arm does not name by hand must still +/// resolve, derived from this test binary's own `deps/` location rather than +/// a `CARGO_BIN_EXE_*` constant. `speculative_bench` is the live example, a +/// real fourth `[[bin]]` target in `Cargo.toml` that no `CARGO_BIN_EXE_*` case +/// names and that no other integration test spawns, so nothing else in the +/// suite exercises this branch, including the `deps`-parent layout check +/// added alongside it. +#[test] +fn resolve_repo_binary_derives_the_path_for_a_binary_the_compile_time_case_does_not_name() { + let (resolved, report) = resolve_repo_binary("speculative_bench"); + + // Same profile directory the compile-time candidates live in, just a + // name `binary_path_candidates` does not special-case. + let profile_dir = Path::new(env!("CARGO_BIN_EXE_mlxcel")) + .parent() + .expect("CARGO_BIN_EXE_mlxcel has a parent directory"); + assert_eq!( + resolved, + profile_dir.join("speculative_bench"), + "speculative_bench must resolve next to the binaries CARGO_BIN_EXE_* names.\n{report}" + ); + assert!( + resolved.exists(), + "speculative_bench was not built.\n{report}" + ); +} + #[test] fn drafter_flag_aliases_are_documented_on_both_binaries() { let serve_help = help_output("mlxcel", &["serve", "--help"]);