From 3d1cfde1aec486e61374801e0a7a32a50afa6328 Mon Sep 17 00:00:00 2001 From: David Christensen Date: Wed, 4 Nov 2026 19:26:15 -0800 Subject: [PATCH 1/2] docs: specify report option diagnostics for #1892 --- ...-11-04-report-option-diagnostics-design.md | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 docs/workflow/specs/2026-11-04-report-option-diagnostics-design.md diff --git a/docs/workflow/specs/2026-11-04-report-option-diagnostics-design.md b/docs/workflow/specs/2026-11-04-report-option-diagnostics-design.md new file mode 100644 index 000000000..9565d56f9 --- /dev/null +++ b/docs/workflow/specs/2026-11-04-report-option-diagnostics-design.md @@ -0,0 +1,54 @@ +# Report utilization option diagnostics + +## Problem + +Issue #1892: global TLS options are refused, but the error lists five unrelated +options and the guide omits TLS options. The existing refusal is correct. + +## Scope + +One unit: replace report.py's fixed diagnostic list with sorted canonical names +from `current_options().command_line_options`, hyphenating underscores as serve.py +does. Keep report as the diagnostic owner; no shared helper or caller migration. +Update `docs/cli.md` and `tests/app/test_report_cli.py`; no ADR is needed. +The operator excludes acceptance behavior/config policy (owner: outside issue +operator). Preserve #1891's 19 refused and two global environment settings, +validation order, exit status, profile loading and survey behavior. + +### Failure model + +- Actors and deployments: local CLI operators and CI CLI tests on supported Python versions. +- Invariants and assets: name only supplied global option identities; disclose no + argument values; refusal still precedes profile loading and output creation. +- Accepted failure classes: original alias spelling is not retained by the existing + canonical option set; report canonical long names, including positive names for + negative boolean switches, following serve.py. Earlier validation errors remain first. +- Covered elsewhere: CLI parsing and option-source classification belong to app.py; + environment classification and configuration policy remain outside this issue. + +## Success + +- C1: When execution reaches the global-option refusal, its diagnostic lists exactly + the supplied members of the eight-option set, sorted and hyphenated, without values. + That set is host, user, password, verify_ssl, trust_system_store, ca_bundle, + cert_fingerprint and profile. Repeated flags appear once; aliases use canonical names. +- C2: The guide lists those eight global options, the `-u`/`-p` aliases and both boolean + polarities; distinguishes the accepted report-local `--profile` from global `--profile`. +- C3: Existing refusal and accepted report behavior remain unchanged. + +## Validation + +- C1/C3, Mode: focused-test. Parameterize the public `cli.app` runner in + `tests/app/test_report_cli.py` over each global option, aliases and negative booleans; + include mixed and repeated flags, assert exact canonical list and absence of values, + exit 2, no profile inventory/load/survey and no output. Existing report-local profile + and no-global success tests cover accepted paths. Red: TLS-only input omits its name + and includes unrelated names. Green command: + `uv run --no-sync pytest --no-cov tests/app/test_report_cli.py -q`. + Verify tests bite by temporarily restoring the old fixed list, then restore the fix. +- C2, Mode: task-test-not-applicable. Guide prose has no executable consumer for its + option list; manually compare it with app.py's eight callback parameters and aliases, + without adding a prose snapshot test. +- Integration: `just verify` and `uv run --no-sync prek run --all-files`; CI checks the + supported Python matrix. No live HMC operation is needed. Revert this isolated diff + to roll back; the command changes no data format or persistent state. From 761461c4c910ddeb9a6bf334fd3fcb2ac861d2f6 Mon Sep 17 00:00:00 2001 From: David Christensen Date: Wed, 4 Nov 2026 19:32:22 -0800 Subject: [PATCH 2/2] fix: name supplied global options in utilization refusal --- docs/cli.md | 8 +++-- src/hmcpctl/cli_commands/report.py | 8 +++-- tests/app/test_report_cli.py | 49 +++++++++++++++++++++++++----- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 12f6adf74..11c5ad4e2 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -620,8 +620,12 @@ The command classifies every `HMCConfig` field for this report: Unset the per-profile exports and configure those settings in individual TOML profiles. An exported setting would otherwise override every profile's own credentials, trust, attribution or limits. The refusal also covers settings this read-only report does not use. -The command also refuses global connection options -(`--host`, `--user`, `--password`, `--verify-ssl`, `--profile`), which would be ignored. +The command also refuses global connection options, which would be ignored: +`--host`, `--user` (`-u`), `--password` (`-p`), `--verify-ssl`/`--no-verify-ssl`, +`--trust-system-store`/`--no-trust-system-store`, `--ca-bundle`, `--cert-fingerprint` +and `--profile`. The error names only the supplied options using their canonical long +names. A global `--profile` before `report` is refused; `--profile` after +`report utilization` selects profiles for the survey. When profiles fail, the `fleet` row's `notes` says how many and which: a system that only a failed profile manages is missing from the fleet totals. It also says how many systems diff --git a/src/hmcpctl/cli_commands/report.py b/src/hmcpctl/cli_commands/report.py index d2a5c466d..b47038f11 100644 --- a/src/hmcpctl/cli_commands/report.py +++ b/src/hmcpctl/cli_commands/report.py @@ -415,10 +415,14 @@ def report_utilization( "report utilization logs on to each profile's own host with that profile's " f"credentials and TLS setting; unset {', '.join(exported)}" ) - if current_options().command_line_options: + command_line_options = current_options().command_line_options + if command_line_options: + options = ", ".join( + f"--{name.replace('_', '-')}" for name in sorted(command_line_options) + ) usage_error( "report utilization connects to each profile's own host; drop the global " - "--host, --user, --password, --verify-ssl and --profile options" + f"{options} options" ) try: selected = _selected_profiles(profiles) diff --git a/tests/app/test_report_cli.py b/tests/app/test_report_cli.py index 1f62a75eb..5b1d21222 100644 --- a/tests/app/test_report_cli.py +++ b/tests/app/test_report_cli.py @@ -328,15 +328,48 @@ async def fake(profiles, open_client, *, concurrency, hmc_timeout): assert failure["notes"] == "'=cmd|x" -def test_root_connection_option_is_refused(configured, tmp_path) -> None: - result = RUNNER.invoke( - cli.app, - ["--profile", "hmc-a", "report", "utilization", "--csv", - str(tmp_path / "r.csv")], - ) # fmt: skip +@pytest.mark.parametrize( + ("args", "expected"), + [ + (["--host", "other.example.test"], "--host"), + (["--user", "other-user"], "--user"), + (["-u", "other-user"], "--user"), + (["--password", "private-value"], "--password"), + (["-p", "private-value"], "--password"), + (["--verify-ssl"], "--verify-ssl"), + (["--no-verify-ssl"], "--verify-ssl"), + (["--trust-system-store"], "--trust-system-store"), + (["--no-trust-system-store"], "--trust-system-store"), + (["--ca-bundle", "other-ca.pem"], "--ca-bundle"), + (["--cert-fingerprint", "AA:BB:CC"], "--cert-fingerprint"), + (["--profile", "hmc-a"], "--profile"), + (["--host", "first.example.test", "--host", "second.example.test"], "--host"), + ( + ["--profile", "hmc-a", "--no-trust-system-store", "--password", "private-value", + "--ca-bundle", "other-ca.pem"], + "--ca-bundle, --password, --profile, --trust-system-store", + ), + ], +) # fmt: skip +def test_root_connection_option_is_refused( + configured, tmp_path, monkeypatch, args, expected +) -> None: + def unexpected(*args, **kwargs): + pytest.fail("refused report must not inspect profiles or survey") - assert result.exit_code == 2 - assert "drop the global" in result.stderr + monkeypatch.setattr(report, "config_inventory", unexpected) + monkeypatch.setattr(report, "load_profile", unexpected) + monkeypatch.setattr(report, "survey_fleet", unexpected) + out = tmp_path / "r.csv" + result = RUNNER.invoke(cli.app, [*args, "report", "utilization", "--csv", str(out)]) + + assert result.exit_code == 2, result.output + error = " ".join(result.stderr.split("Error: ", 1)[1].split()) + assert error == ( + "report utilization connects to each profile's own host; drop the global " + f"{expected} options" + ) + assert not out.exists() def test_write_failure_keeps_failure_lines_and_leaves_no_file(