test(cli): cover the diagnose command's exit codes, loading paths and flags

[Nitjsefnie]

Description

Closes #104. Adds tests/unit/test_cli/test_diagnose.py — 13 tests for the diagnose CLI, following the existing typer.testing.CliRunner patterns in this repo.

DiagnosisAnalyzer is mocked throughout, so the suite runs fully offline with no model call and no API key.

Coverage is aimed at behaviour rather than line count — the exit codes, the input-validation paths, and the flags that must actually reach the analyzer:

Exit 2 paths	missing argument · non-existent file · non-.json file · malformed JSON
Exit 0 paths	empty result list warns and exits clean · an unrecognised JSON shape yields no results and never calls the analyzer
Loading	a PipelineResult wrapper with a results key is unpacked and handed to the analyzer
Output	default terminal rendering · --output json emits a serialisable DiagnosisReport · the global --json flag forces JSON when --output is omitted
Flags reaching the analyzer	--threshold and --max-recs arrive at the DiagnosisAnalyzer constructor · --verbose renders detailed failures

Type of Change

• Test update

Related Issues

Closes #104

How Has This Been Tested?

• Unit tests pass (uv run pytest) — CI's own command, uv run pytest tests/unit -v --tb=short

Before: 1015 passed, 4 skipped. After: 1028 passed, 4 skipped — the +13 are these tests, and nothing pre-existing changed.

No production code was touched; the commit is that one test file. (uv sync --all-extras modified uv.lock locally while setting up to run your suite — deliberately left unstaged.)

One courtesy note: @bityodha commented "assigne to me" on #104 back on 2026-07-14. I checked before starting — there is no assignee, no claimed label, and no PR referencing the issue — so I read it as never having got started rather than as work in progress. Happy to close this if they are in fact mid-way through.

Generated by Claude Opus 5 (brief, review), Kimi K2.7 Code (implementation)

[Copilot]

🟡 Not ready to approve

The new tests include a likely failing help assertion and a context-reset fixture that can leak global CLI state across modules, making the suite order-dependent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds a new unit-test module to increase coverage of the oaeval diagnose CLI command, focusing on exit-code behavior, report-loading shapes, output modes, and option wiring to the DiagnosisAnalyzer.

Changes:

• Added tests/unit/test_cli/test_diagnose.py with 13 unit tests using typer.testing.CliRunner.
• Introduced a DiagnosisAnalyzer monkeypatch fixture to keep tests fully offline while validating flag/arg wiring and output.

File summaries

File	Description
tests/unit/test_cli/test_diagnose.py	Adds CLI-layer tests for diagnose covering validation paths, report-loading variants, output formats, and analyzer option wiring.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Nitjsefnie]

Two of the three land; the first does not, and I checked it against the rendered output before changing anything.

Metavar case — leaving it as-is. Typer is not Click on this point. TyperArgument.make_metavar (typer/core.py, commented "Modified version of click.core.Argument.make_metavar() to include Argument name") uses var = self.name or "" with no .upper(). So on typer 0.27.0 as locked here, oaeval diagnose --help prints {report_path} in the usage line and report_path in the Arguments panel: "report_path" in output is True and "REPORT_PATH" in output is False. The suggested change would turn a currently-green test red.

Context fixture — fixed (f06a320), and the "can become order-dependent" wording was exactly right. Under default ordering the --json test is followed by two more in the same module whose before-yield reset scrubs the dirt, so a naive probe passes even unfixed. Forcing that test last — -k, --lf, -x, xdist, or just adding a 14th test — leaks json_output=True into the next module, and none of test_main.py / test_run.py / test_synth.py calls reset_context. The fixture now resets on both sides, matching what test_audit.py:37-47 already does.

Unused import — fixed in the same commit; ruff check on the file goes from F401 to All checks passed!.

One thing worth knowing while I was in there: the lint job cannot fail the build — ci.yml runs uv run ruff check . || true under continue-on-error: true — so that F401 shipped green, and ruff check . is repo-wide red at HEAD today. Happy to file that separately if it is not deliberate.

[Nitjsefnie]

The 3.11 red here is unrelated to this change — test_corpus/test_auditor.py::TestCorpusAuditor::test_audit_with_valid_checks_still_works hit a ConnectionError reaching the HF CAS server (cas-server.xethub.hf.co), and 3.12 is green. This diff only touches test_diagnose.py (1027 passed, that one network call the sole failure), so a re-run should clear it.

[Copilot]

🟡 Not ready to approve

One of the new help-output assertions is likely brittle (argument metavar casing) and may fail across Click/Typer help rendering.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

tests/unit/test_cli/test_diagnose.py:103

• Typer/Click help output typically renders argument metavars uppercased (e.g., REPORT_PATH), so asserting on the Python parameter name report_path is likely to be brittle and may fail across Click/Typer versions.

    assert result.exit_code == 0
    output = strip_ansi(result.output)
    assert "report_path" in output
    for flag in ("--output", "--threshold", "--max-recs", "--verbose"):

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Nitjsefnie]

Followed up on the lint-gate note: it turned out to be deliberate rather than an oversight — the MyPy step's own comment about || true makes that clear — so I filed it as #272 framed around the accumulated debt (228 ruff check findings, 122 unformatted files at 2ad932b) and whether permanent non-gating is intended, rather than as a bug.