Skip to content

fix(tests): honor CARGO_TARGET_DIR and clap's singular alias label - #996

Merged
inureyes merged 3 commits into
mainfrom
fix/issue-962-nightly-verify-red
Aug 2, 2026
Merged

fix(tests): honor CARGO_TARGET_DIR and clap's singular alias label#996
inureyes merged 3 commits into
mainfrom
fix/issue-962-nightly-verify-red

Conversation

@inureyes

@inureyes inureyes commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

Two independent defects kept the nightly make verify job red. Both surface in tests/cli_help_consistency.rs; one of them lives in the helper that 13 integration test files share. Neither is a defect in the CLI itself, so nothing operator-facing changes.

Defect A: the binary-path helper ignored CARGO_TARGET_DIR

This is the one CI actually hits, and it does not reproduce locally. repo_binary_path in tests/common/mod.rs read CARGO_BIN_EXE_<name> with std::env::var_os (a compile-time variable that is never set at test runtime, so that branch was dead) and then reconstructed CARGO_MANIFEST_DIR/target/<profile>/<name>. nightly-verify.yml and release.yml both set CARGO_TARGET_DIR=$HOME/.cargo-target/mlxcel so the self-hosted runner keeps a warm build cache outside the checkout, so cargo put the binaries there while the tests looked for them in the workspace. Every test in the file failed at the Command::output() call with No such file or directory.

Cargo does expose a better signal than reconstruction, and the rest of this test suite already uses it: tests/surgery_cli.rs, tests/lang_bias.rs, tests/pipeline_cli_real_models.rs and six other files call env!("CARGO_BIN_EXE_mlxcel") directly, tests/surgery_cli.rs:47 even spelling out why. That compile-time variable already accounts for the profile, the target triple, and CARGO_TARGET_DIR, and cargo guarantees the binary is built before the integration test runs. tests/common/mod.rs was the outlier.

Resolution is now built from cargo-produced signals only, in this order:

  1. The compile-time env!("CARGO_BIN_EXE_<name>") for the three binaries the tests spawn (mlxcel, mlxcel-server, mlxcel-bench-decode). All four bin targets are unconditional, so these cannot fail to compile.
  2. A profile directory derived from std::env::current_exe(), for any binary not named above. Integration tests link into <target-dir>/[<triple>/]<profile>/deps/, so the package binaries sit two levels up. The candidate verifies the parent really is deps rather than assuming it, so a test binary running from a copied location cannot spawn an unrelated sibling that happens to share the name.

Two things were deliberately dropped rather than fixed. The runtime CARGO_BIN_EXE_* lookup is gone: cargo never sets it at test runtime, 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 <manifest>/target/<profile>/<name> reconstruction is gone too: beyond the CARGO_TARGET_DIR bug, 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 (Makefile:236), so it could hand back a stale binary from an unrelated build. A candidate that can be silently wrong is worse than no candidate.

resolve_repo_binary returns the chosen path and a diagnostic report from a single pass, marking which candidate it selected, so the report cannot contradict the choice it is explaining. Both spawn sites in the help test fold it into their panic, so the next occurrence is diagnosable from the panic alone. repo_binary_path keeps its signature and still returns a path unconditionally, so the if !binary.exists() { skip } guards in the real-model tests keep compiling and behaving as before.

One nuance worth recording rather than glossing: because the compile-time candidate is essentially always present under cargo test, those exists() guards now almost always see a real path. That is a no-op in both environments that matter, since every one of them is preceded by a repo_model_dir(...).exists() guard and the nightly runner carries no model weights, so the model guard fires first there and locally the binary guard already passed.

Defect B: the alias assertion pinned a string clap no longer emits

drafter_flag_aliases_are_documented_on_both_binaries asserted on the literal [aliases: X] four times. clap_builder computes that label as pluralize(als.len(), "", "es"), so it renders [alias: X] for exactly one visible alias and [aliases: ...] only for two or more. Each of the four flags carries exactly one visible_alias, so all four render singular. The pluralize call arrived in clap_builder 4.6.2, which entered Cargo.lock on 2026-07-20 in 3ccb693a3 (#828); the test was written on 2026-07-01 in a89f28aa4 (#602) against the older rendering. This is a dependency-bump regression, not a behavior change: both binaries still declare and still document all four aliases, so the #464 contract holds.

The fix is to the test. Adding a second alias so clap emits the plural would change the operator-facing CLI to satisfy a test string, which is the wrong direction.

Which assertion form, and why

The replacement locates the flag's own help entry, parses the alias names out of whatever alias annotation clap rendered there, and matches on the name. Concretely, assert_flag_documents_alias(label, help, "--draft-model <PATH>", "--model-draft").

The two candidate shapes named in the issue each fail on their own:

  • Accept either [alias: or [aliases: on the whole help output. Too loose in the wrong dimension. It does not say which flag carries the alias, so an alias annotation anywhere in a 600-plus-line help page would satisfy it.
  • Search for the alias name near its flag. Too loose in a way that silently un-pins the contract. --draft-model's own description reads "the llama-server-style --model-draft spelling (alias, matches mlxcel-server)". The alias name is already in that entry's prose, so this assertion would pass with the visible_alias attribute deleted. This is exactly the failure mode the issue warns about, and it is not hypothetical for this specific flag.

Combining the two removes both problems: the alias name must appear, it must be inside a real clap alias annotation rather than prose, and that annotation must belong to the flag under assertion. What is deliberately not pinned is clap's alias/aliases label, which is presentation, nor the alias count, so adding a second alias to any of these flags will not break the test.

Both halves of that are anchored rather than substring-matched, because unanchored matching left two live gaps:

  • The entry anchor matches the whole trimmed signature line, not a prefix. Measured on the live help, a prefix match on --metrics also hits the separate --metrics-port <PORT> entry and an example command line in after_long_help, and the short-form branch requires a real two-character short flag so a hyphen-bulleted description line cannot anchor the slice on prose.
  • The alias annotation must begin a line, which is how clap always renders it. A bare substring search would also accept a bracketed span written by hand in a doc comment, and the help already carries such spans on other flags ([llama-server alias for --prefill-chunk-size]), so that gap was a live foot-gun rather than a theoretical one.

Six tests cover the machinery itself, so the guard is guarded: prose-only entries and inline prose brackets do not count, both label spellings and a wrapped annotation parse, the slicer stops at the next flag and at the next section, a prefix sibling does not resolve, a hyphen-bulleted line is not mistaken for an entry, and cutting the rendered annotation out of the live mlxcel serve --help leaves the alias undocumented.

Defect A also gets the regression test it never had. cli_binaries_resolve_to_the_path_cargo_built_them_at asserts 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, and resolve_repo_binary_derives_the_path_for_a_binary_the_compile_time_case_does_not_name covers the other candidate through speculative_bench, the one [[bin]] target no CARGO_BIN_EXE_* case names.

What changed

  • tests/common/mod.rs: binary_path_candidates (private) feeding a new resolve_repo_binary that returns path plus diagnostics from one snapshot; repo_binary_path reduced to a thin wrapper so its 28 existing call sites are untouched.
  • tests/cli_help_consistency.rs: both spawn panics carry the resolution report; drafter_flag_aliases_are_documented_on_both_binaries rewritten on new flag_help_entry / documented_aliases / assert_flag_documents_alias helpers; eight new tests.

Test plan

  • cargo test --release --features metal,accelerate --test cli_help_consistency: 17 passed, 0 failed.
  • The same command with CARGO_TARGET_DIR pointed at a directory outside the checkout, matching what the nightly sets: 17 passed. The new resolution test makes this self-verifying, and the rebuilt test binary embeds <CARGO_TARGET_DIR>/release/mlxcel as its compile-time path.
  • fix(cli): align mlxcel-server drafter flags with mlxcel serve (--draft-model rejected) #464 contract still pinned, demonstrated twice on real rebuilds rather than only synthetically. Removing visible_alias = "model-draft" from src/main.rs:905 fails the --draft-model <PATH> assertion; removing visible_alias = "draft-max" from src/bin/mlx_server.rs:282 fails the --draft <DRAFT> assertion. Both report an empty alias list next to prose that still names the alias. Sources restored afterwards.
  • cargo clippy --release --all-targets --features metal,accelerate -- -D warnings: clean.
  • cargo fmt --all -- --check: clean.
  • cargo test --release --features metal,accelerate --no-fail-fast over the whole root package, three times.

What the --no-fail-fast run surfaced

cargo test is fail-fast, so the nightly stopped at cli_help_consistency and the other integration targets never ran on CI. The point of this run was to find out what that stop had been hiding.

Nothing in this PR's family. The final run is green end to end: across 73 result lines (lib, four bin unit targets, 67 integration targets, doc-tests), 5219 passed, 0 failed, 274 ignored, exit 0, zero compiler warnings. The lib suite is green at 4813 passed, and cli_help_consistency is green at 17.

One unrelated flake did turn up, and it is filed as #997 rather than folded in. text_only_forward_produces_finite_logits failed in tests/granite4_vision_parity.rs:100 and tests/hunyuan_vl_parity.rs:105 on the second of three whole-suite runs, and was green on the first and third. Run on its own, either target passes 3 out of 3 attempts. Both are real-model tests that load a checkpoint from models/ and assert the maximum logit of a text-only forward pass is finite; neither spawns a binary, and both reach only common::repo_model_dir, which this PR does not touch, so the change is not a plausible cause. It does not belong here on scope either: both tests self-skip when the checkpoint directory is absent, and the nightly runner carries no model weights, so they never execute on the run this issue is about. Chasing a non-finite logit in two VLM forward paths is a different investigation from a test-harness path bug, so it gets its own issue.

make verify status

  • make verify-fmt (cargo fmt --all -- --check): green.
  • make verify-test (cargo test --release --features metal,accelerate): green, run as the --no-fail-fast superset above.
  • make verify-clippy: the CI target is cargo clippy --all-targets --features metal,accelerate -- -D warnings, which is the debug profile. This machine has no target/debug at all, so running it verbatim triggers a cold MLX C++ debug build. It was run in the release profile instead, clean. The lint set does not vary by profile, this PR touches only test code, and verify-clippy was already green on the 2026-07-31 nightly after fix(multimodal): gate the Qwen2-VL prefill export on the feature that calls it #961, so the remaining risk is the profile substitution alone.

Closes #962

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/<profile>/<name>`, 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_<name>` 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
@inureyes inureyes added status:review Under review type:bug Bug fixes, error corrections, or issue resolutions priority:high High priority labels Aug 2, 2026
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 `<manifest>/target/<profile>/<name>` 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 <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 <DRAFT>` assertion, confirming the tightened form still pins the contract.

Refs #962
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
@inureyes inureyes added status:done Completed and removed status:review Under review labels Aug 2, 2026
@inureyes
inureyes merged commit fce662f into main Aug 2, 2026
5 checks passed
@inureyes
inureyes deleted the fix/issue-962-nightly-verify-red branch August 2, 2026 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:high High priority status:done Completed type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[nightly-verify] main is red

1 participant