fix(cli): unreachable 2D-parallelism guard in validate_pipeline_parallel_args, decide whether to delete it or widen the early return #1131

Summary

validate_pipeline_parallel_args in src/commands/generate.rs contains an ensure! whose condition is the exact logical negation of an early return eight lines above it. The guard can never fail and its message can never reach a user. This was found while reviewing #1129 (which closes #1112) and deliberately left out of scope there, because #1112's acceptance criteria required the validation logic to be unchanged.

The fix is not obvious, because the dead guard is evidence of one of two different intents. This issue asks a maintainer to decide which reading is correct before any code is written.

Background

The tautology

src/commands/generate.rs, validate_pipeline_parallel_args:

if pp.pp_layers.is_none() && pp.pp_size <= 1 {
    return Ok(());
}

// ... comment block ...
let tp_size = args.tensor_parallel.tp_size;
if tp_size > 1 {
    ensure!(
        pp.pp_size >= 2 || pp.pp_layers.is_some(),
        "2D parallelism requires --pp-size >= 2 (or an explicit --pp-layers spec) \
         alongside --tp-size > 1"
    );

The early return fires when pp.pp_layers.is_none() && pp.pp_size <= 1. Past that point the negation holds: pp.pp_layers.is_some() || pp.pp_size >= 2, which is precisely the ensure! condition. The guard is therefore always satisfied whenever it is evaluated, so it is dead code and its error message is unreachable.

#1129 corrected the flag name inside that message (it previously said --tensor-parallel-size, which no binary accepts) but deliberately did not touch the logic, since #1112 scoped the change to the message text only.

The test records the same tautology without naming it

validate_pipeline_parallel_args_rejects_2d_without_pp_enabled in src/commands/generate_tests.rs sets pp_size = 1, tp_size = 2, and asserts is_ok(). Its comment reads: "The validator returns early when PP is disabled, so this case is harmless". So the test name claims the validator rejects this input while the assertion confirms it accepts it. The name has to change whichever way the logic question is resolved.

Proposed Solution

This is a decision to make first, not a patch to write first. The two readings lead to entirely different changes, and picking between them requires knowing the intended semantics of --tp-size without a pipeline topology.

Reading (a): the guard is redundant and should be deleted. Under this reading, --tp-size 2 --pp-size 1 is TP-only by design and is correctly accepted today. The early return is doing exactly what it should, the ensure! adds nothing, and the correct change is to remove the dead ensure! and rename the misleading test to describe what it actually asserts (that a TP-only invocation is accepted). No user-visible behaviour changes.

Reading (b): the early return is too broad. Under this reading, a run with tp_size > 1 that names no pipeline topology was meant to be rejected rather than silently downgraded to TP-only, and the early return should never have swallowed that case. The fix is to add a tp_size condition to the early return (so it only short-circuits when TP is also disabled), which makes the existing ensure! reachable and lets its message finally surface. The test then keeps its current name and flips to asserting an error.

Note that (b) is a behaviour change: command lines that work today (--tp-size 2 with default --pp-size) would start failing. That needs a deprecation thought, for example a warning first and a hard error in a later release, rather than an immediate hard failure.

Implementation Notes

• Whichever reading wins, validate_pipeline_parallel_args_rejects_2d_without_pp_enabled in src/commands/generate_tests.rs must be renamed or re-asserted so that its name and its assertion agree, and the comment explaining the early return should be updated to match the resolved intent.
• Under (a), the total_ranks > 0 sanity check immediately after the dead ensure! should be reviewed at the same time: saturating_mul of two values that are already known to be at least 1 and at least 2 respectively cannot produce 0, so it may be dead for the same reason.
• Under (b), check the cluster-TOML validator layer as well, since the in-code comment says the full pp_size * tp_size == nodes check lives there. A rejection added at the CLI layer should not contradict or duplicate what the cluster layer already enforces for remote topologies.

Separable follow-up: machine-check flag names in error strings

The sweep that surfaced the original --tensor-parallel-size defect exists only as a shell pipeline in #1129's description:

grep -o -- '--[a-z][a-z0-9-]*' src/commands/generate.rs | sort -u | comm -23 - <(mlxcel generate --help | ...)

tests/cli_help_consistency.rs is the established home for machine-checked CLI-surface invariants. A test asserting that every --flag named in an error-message string literal actually appears in --help would close the whole class of defect instead of the one instance #1112 fixed. This is genuinely separable from the logic decision above and could be its own issue if a maintainer prefers to split it.

Acceptance Criteria

• A maintainer records a decision on the issue: reading (a) (guard is redundant, TP-only is intended) or reading (b) (early return is too broad, TP without a pipeline topology should be rejected).
• The chosen change is implemented in src/commands/generate.rs: under (a), the unreachable ensure! is removed; under (b), the early return gains a tp_size condition so the guard becomes reachable.
• Under (b) only: a deprecation path is agreed and implemented, so command lines that are accepted today do not start hard-failing without warning.
• validate_pipeline_parallel_args_rejects_2d_without_pp_enabled in src/commands/generate_tests.rs is renamed or re-asserted so its name matches its assertion, with the stale comment updated.
• No remaining branch in validate_pipeline_parallel_args is unreachable by construction, including the total_ranks > 0 check if reading (a) is chosen.
• The flag-name-in-error-strings invariant is either added to tests/cli_help_consistency.rs or split into its own issue and linked from here.