fix(cli): make coverage add exit non-zero when it stores nothing

[NgoQuocViet2001]

Fixes #1745.

The problem

Every no-op path through coverage add printed a message and returned, so the process exited 0:

path	before
auto-discovery found no report	0
no index yet (repowise init not run)	0
no indexed files	0
report parsed but nothing mapped to the repo tree	0

A refresh scripted as repowise coverage add ... \|\| exit 1 therefore could not tell a complete ingest from one that stored nothing, and CI recorded success over a stale or empty coverage store.

The change

_do() now answers whether anything was stored, and the command turns that answer into the exit status. The discovery miss raises directly, since it happens before the ingest starts and has no _do() to report through.

Partial ingests

Kept at 0 by default and put behind --strict, which is the second option the issue offers. A report that maps most of its files is still a real refresh, and failing it by default would break every setup with a build-prefix mismatch it has already learned to live with — whereas a --strict run is opt-in and says what it found:

--strict: 248 report file(s) did not map to the repo tree.

Happy to flip that to fail-by-default if you would rather; it is a one-line change and I asked on the issue before picking a side.

Not addressed here: the machine-readable requested/mapped/skipped/stored counts, and the .coverage-contexts documentation ambiguity. Both are worth doing and neither belongs in an exit-status fix.

Test plan

• Ran: uv run pytest tests/unit/cli/test_coverage_cmd.py -q — 7 passed (3 pre-existing + 4 new).
• Ran: uv run pytest tests/unit/cli/ tests/unit/health/test_coverage_discovery.py -q — 2192 passed, 2 skipped, 2 xfailed.
• Ran: uv run ruff check and uv run ruff format --check on both changed files — clean, and the tree was already format-clean before the change.
• Checked: with only coverage_cmd.py reverted, 3 of the new assertions fail — so they are pinned to the behaviour, not to the test setup.

New coverage: the discovery miss exits 1 with its message intact, and a parametrised case pins the wiring in both directions (stored → 0, nothing stored → 1), which is the branch every one of the four no-op paths funnels through. Plus --help documenting --strict.

[repowise-bot]

✅ Health of changed files: 6.7 → 7.2 (+0.4)
_{⚠️ Change risk: moderate, riskier than 37% of this repo's commits.}

📋 At a glance
1 hotspot touched · 1 new finding introduced. Scoped to packages.

✅ Health gate: passed

📌 Before you merge

• Run .../cli/test_format_json_rollout.py: they import the changed files

🔎 More signals (2)

🗺️ Change map

flowchart LR
  subgraph PR ["Changed in this PR (1 with dependents)"]
    f_packages_cli_src_repowise_cli_commands_coverage_cmd_py[".../commands/coverage_cmd.py 🔥"]:::changed
  end
  t_tests_unit_cli_test_format_json_rollout_py(["✅ .../cli/test_format_json_rollout.py"]):::guard
  t_tests_unit_cli_test_format_json_rollout_py -.-> f_packages_cli_src_repowise_cli_commands_coverage_cmd_py
  classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
  classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
  classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d

Loading

_{Solid arrows: code that imports the changed files (0 direct dependents, from the last indexed snapshot). Dashed: history/tests.}

🔥 Hotspot touched (1)

• .../commands/coverage_cmd.py: 6 commits/90d, 2 dependents · primary owner: Raghav Chamadiya (95%)

👀 Suggested reviewers @RaghavChamadiya

---

📊 See the full report for this PR
_{Your repo map with this PR's blast radius lit up, every caller of the contracts it changes, and health before and after. No sign-in. · ⭐ Star Repowise · 📥 Install bot · Silence on a single PR with [skip repowise] in the title · Per-repo toggle on repowise.dev/settings?tab=bot · Updated 2026-08-24 04:19 UTC (since the last push: health 7.0 to 7.2)}

[NgoQuocViet2001]

Ran the dependent suite the bot flagged:

uv run pytest tests/unit/cli/test_format_json_rollout.py -q
43 passed

It touches coverage add only through the shared --format rollout, which this change does not go near — the new option is --strict and the only other edit is the exit path.

[RaghavChamadiya]

Thanks @NgoQuocViet2001, and thanks for asking on the issue before picking a side on the partial case rather than guessing.

Two things you got right that I want to name. Returning a bool from _do() and letting the command turn it into the exit status keeps the decision in one place instead of scattering Exit(1) through the branches, and raising directly for the discovery miss is correct because that one happens before there is any _do() to report through. You also found a fourth no-op path that the issue does not list, the auto-discovery miss, and I had missed it too when I wrote up the mechanism.

To answer your question: keep --strict opt-in, do not flip it to fail-by-default. Your reasoning in the description is the right one. A monorepo that ingests a report covering one package is a legitimate partial, and failing that by default would break setups that work today. There is also a better discriminator available than the ratio, which is that matched_exact and matched_suffix are already tracked separately, so "200 files matched exactly" and "1 file matched by suffix" can be told apart without a threshold at all. That belongs in #1746 rather than here, and this PR should not wait on it.

One thing I would like fixed before merge, because it is in the new flag's main use case.

unmapped is assigned inside if resolved.files::

                if resolved.files:
                    await save_coverage_files(...)
                    agg_matched = resolved.matched
                    console.print(...)
                    skipped = resolved.unmatched + resolved.ambiguous
                    unmapped = len(skipped)

So when the aggregate leg maps nothing at all, resolved.files is empty, unmapped stays 0, and --strict cannot fire. Concretely: repowise coverage add --strict .coverage lcov.info where the .coverage has contexts and every path in lcov.info fails to map. The per-test map builds, so map_records is non-empty and the "nothing mapped" branch is skipped; unmapped is 0, so the strict branch is skipped too; the command exits 0. That is total mapping loss on the aggregate leg passing a strict run, and it is exactly the case in #1746.

Hoisting the count out of the if resolved.files: block so it is computed from resolved whenever agg_paths ran should cover it.

Two non-blocking notes, take them or leave them:

The strict failure prints after the success guidance, so a failing --strict run says "Run repowise health to fold coverage into the defect scores" and then reports that it failed. Moving the strict check above that console.print reads better.

The second test monkeypatches run_async and closes the coroutine, so it pins the wiring rather than the branches. Your docstring says exactly that, which is the right call, and I mention it only so nobody later reads it as covering the four no-op paths themselves.

On CI: it has not run the matrix here, and that is not your doing. Main has been red since #1755 for an unrelated reason, two generated files edited at their output instead of their source. #1762 fixes it. Once that lands, rebase on main and I will approve the workflow run so you get a clean signal. Ping me when it is rebased with the unmapped hoist and I will merge.

[RaghavChamadiya]

#1762 is merged, so main is green again and the rebase I mentioned is unblocked. git fetch origin && git rebase origin/main should be clean; it only touched generated files, none of them yours.

Once that is pushed with the unmapped hoist, ping me and I will approve the workflow run so the Python matrix actually reports, then merge.

[NgoQuocViet2001]

@RaghavChamadiya rebased on main and the unmapped hoist is pushed — ready for the workflow approval.

The hoist. You're right, and the repro you wrote out is the flag's main use case, which makes it worse than an edge case. unmapped now comes off resolved as soon as agg_paths ran:

for path, err in errors:
    console.print(f"[yellow]  {path.name}: {err}[/yellow]")
# Counted from `resolved`, not from inside the `resolved.files`
# branch below: total mapping loss leaves `files` empty, so
# reading the count in there reported 0 report files unmapped
# for the one run where every single one of them was.
skipped = resolved.unmatched + resolved.ambiguous
unmapped = len(skipped)
if resolved.files:
    ...

The shape of the bug is worth naming: the count was only read on the success path, so the total-loss case was invisible precisely because it had nothing to succeed with. --strict now fires on your coverage add --strict .coverage lcov.info case.

Both non-blocking notes taken.

The strict check moved above the success guidance, so a failing run no longer says "Run repowise health…" and then reports that it failed.

On the second test — agreed, and I'd rather not leave that gap unmarked. The two new guards are explicitly structural, and say so in the docstring: they assert unmapped = len(skipped) precedes if resolved.files:, and that the strict branch precedes the success print. They pin the ordering that makes the branch reachable, not the branch itself, because it lives in the async closure that needs a session. They do fail on the pre-fix code, so they are not decorative — but nobody should read them as covering the four no-op paths.

If you'd rather have a real behavioural test, it is doable by mocking the session plus build_coverage_map / parse_contexts_file / resolve_test_reports; I left it out because that is about nine mocks to pin one ordering, and I did not want to smuggle a test-shaped refactor into a fix you are waiting on. Happy to add it here or as a follow-up, your call.

On --strict staying opt-in — noted, and thanks for settling it with a reason rather than a preference. The matched_exact / matched_suffix split being a better discriminator than any ratio is a much nicer answer than a threshold; I'll pick that up in #1746 if it's still unclaimed.

Verification

uv run python -m pytest tests/unit/cli/test_coverage_cmd.py tests/unit/cli/test_format_json_rollout.py -q
52 passed

ruff check and ruff format clean on both changed files. The rebase onto main was clean, as you said it would be. Local CI still cannot run the matrix here, so your approval is the first real signal.

[RaghavChamadiya]

The unmapped hoist is exactly what I was after, and the comment above it explains the failure better than my ask did: the one run where every report file failed to map was also the run that counted zero unmapped files. Verified on current main, applies clean, tests/unit/cli/test_coverage_cmd.py green locally, CI green on all three versions.

Merging. Two things worth saying out loud rather than holding you for.

test_strict_counts_unmapped_before_the_resolved_files_branch and test_strict_failure_is_reported_before_the_success_guidance assert on inspect.getsource text ordering. You flagged that yourself as a structural guard, which is the honest framing, and I would rather have them than nothing. They will fail on a rename or a reflow that changes nothing about behaviour, so if the closure is ever lifted out of _do() into something a test can call with a fake session, those two should be replaced rather than repaired.

The exit-status change is a CLI contract change. Anyone running repowise coverage add in CI who was tolerating "no reports found" will start failing, which is the point of #1745, but it belongs in the release notes rather than arriving quietly. That is ours to write, not yours.

Thanks for coming back to this one.

[RaghavChamadiya]

Verified against current main: applies clean, targeted tests green locally, CI green. Merging.