CI: Extend mypy to tests and scripts by ModeSevenIndustrialSolutions · Pull Request #397 · lfreleng-actions/github2gerrit-action

ModeSevenIndustrialSolutions · 2026-08-27T13:59:21Z

Important

Stacked on #393 and #396. Review only the last two commits — Fix: Let explicit CLI flags outrank the environment and CI: Extend mypy to tests and scripts. The earlier two belong to those PRs and drop out once they merge.

Closes #395

The headline is not the mypy config

Turning mypy on over tests/ surfaced a live production bug that had been silenced behind a pyright: ignore. It gets its own commit, because it changes behaviour.

`_resolve_bool_override` never let a CLI flag win

cli.py:821 decided whether a boolean option came from the command line:

source = ctx.get_parameter_source(param_name)
if source == click.core.ParameterSource.COMMANDLINE:   # never true
    return current

typer ≥ 0.20 vendors its own copy of click at typer._click, so the context returns a typer._click.core.ParameterSource member. Members of different enum classes never compare equal:

typer 0.27.1   click 8.4.2
typer._click is click:                                    False
tc.core.ParameterSource is click.core.ParameterSource:    False

So the branch was unreachable and every explicit CLI flag lost to the environment variable behind it — the exact opposite of the docstring, which promises "CLI flags always take precedence". Confirmed end-to-end:

PRE-FIX   explicit --flag, G2G_PROBE=false  ->  False   (wrong)
POST-FIX  explicit --flag, G2G_PROBE=false  ->  True
          no flag,         G2G_PROBE=false  ->  False   (unchanged)

Two things about how this survived are worth dwelling on:

Nothing tested it. The suite passes 1553/0/0 with the bug and without it. There was no test of _resolve_bool_override at all. The regression test added here drives a real Typer invocation, since the parameter source is only populated by genuine argument parsing, and covers both flag directions plus the GitHub Actions case the helper exists for, where the string "false" must parse rather than be coerced to True.
Both checkers had already reported it, and it was silenced. The line carried # pyright: ignore[reportAttributeAccessIssue, reportUnnecessaryComparison] — and reportUnnecessaryComparison was basedpyright saying, in as many words, that this comparison can never be true.

The mypy work

Why the config was dead

tests/ has no __init__.py, so mypy names its modules test_cli and conftest, never tests.test_cli. The [[tool.mypy.overrides]] blocks targeting tests.* matched nothing, and mypy said so on every run.

Correcting the names does not fix it either. mypy accepts * only as a whole dotted component, so test_* is rejected outright:

probe.toml: [module = "test_*"]: Patterns must be fully-qualified module names,
  optionally with '*' in some components (e.g spam.*.eggs.*)

That complaint goes to stderr, mypy still exits 0, and the entire block — including valid entries sitting alongside the bad one — is discarded silently. Since test modules are single-component, no pattern can match them all.

Approach	Outcome
`module = ["test_*"]`	Pattern rejected; whole block dropped
`explicit_package_bases` + `tests.*`	Fixes naming, but breaks `from fixtures.make_repo import …` — `tests/fixtures/` does have `__init__.py`
Rewrite to `tests.fixtures.make_repo`	Fails at runtime: pytest puts `…/tests` on `sys.path`, not the repo root
Invert the default	Chosen

So: relax annotation rules globally, switch them back on for github2gerrit.* — a structured wildcard that always matches and cannot go inert. strict = true still supplies check_untyped_defs, warn_unreachable, strict_equality and no_implicit_reexport everywhere; tests lose only the demand to annotate, never a correctness check. This mirrors the basedpyright stance #392 recorded for tests/.

Verified in both directions rather than assumed — the same untyped function added to each tree:

src/github2gerrit/gitreview.py  ->  error: ... [no-untyped-def]
tests/test_gitreview.py         ->  Success: no issues found

A second misleading setting

exclude filters directory discovery only; files named explicitly on the command line are still checked. The hook passes filenames, so every path in that list was already being checked. Trimmed to build/, dist/, docs/ — which brought five more test files under the checker.

The hook needed packages, not stubs

Widening files: produced 42 errors that never appear locally: 31 × untyped-decorator (with pytest absent, @pytest.fixture is Any) and 10 × unreachable (without pytest, pytest.raises no longer tells mypy the block can swallow an exception). Silencing those would have meant disabling two checks and dropping warn_unused_configs. Installing click, pytest, responses and typer as additional_dependencies removes the class at its root.

On the `comparison-overlap` and `unreachable` findings

I asked for these to be scrutinised as possible vacuous assertions. They were investigated individually, and all 13 turned out to be mypy limitations rather than broken tests:

comparison-overlap ×10 — ExitCode is an IntEnum, so ExitCode.SUCCESS == 0 is genuinely True; mypy narrows the member to a Literal and declines to overlap it with Literal[0]. Rewritten to drive the identical comparisons from a typed list, so mypy sees the declared type. Same checks, same count, no suppression.
unreachable ×3 — mypy does not widen a narrowed member expression when a method mutates it, so assert m.pid is None after m.cleanup() narrowed to Never. Routed through a helper so the attributes keep their declared optional types. Assertions untouched.

The nearest thing to a vacuous assertion was in production code, not tests: the cli.py comparison above.

Validation

Gate	Result
`uv run mypy src/ tests/ scripts/ sitecustomize.py`	Success: no issues found in 124 source files, no unused sections
`prek run --all-files mypy`	passed — the real gate, not just the local run
`pytest`	1558 passed, 0 failed, 0 errors, 0 skipped
`basedpyright` (124 files)	0 errors, 0 warnings
`ruff check` / `format`	clean
`aislop ci`	100 / 100, 0 findings

For scale: mypy over src/ tests/ went from 486 errors in 40 files with 7 unused sections, to zero, with the checked set growing from 112 to 124 files.

Suppressions added: none. Net change is −3 # type: ignore and −1 pyright ignore code.

Regression test proven non-vacuous — reverting the cli.py fix fails exactly the two explicit-flag cases, while the environment-only cases correctly pass either way:

FAILED tests/test_cli_helpers.py::test_resolve_bool_override_precedence[argv0-false-True]
FAILED tests/test_cli_helpers.py::test_resolve_bool_override_precedence[argv1-true-False]

Worth a reviewer's eye

typer vendoring click is a latent hazard beyond this one line. typer.Context is not click.Context and typer.testing.Result is not click.testing.Result. cli.py still does import click and uses click.Group. Worth auditing for other cross-copy identity assumptions — out of scope here, happy to raise it.
types-PyYAML moved into the dev extra. uv run mypy src/ was not clean before this PR — it reported missing yaml stubs, because the stub package existed only inside the pre-commit hook's isolated environment.
The hook's additional_dependencies are unpinned, so pre-commit.ci resolves latest pytest/typer/click rather than the lockfile versions. Inherent to mirrors-mypy. A language: system hook running uv run mypy would be tidier, but pre-commit.ci is the only place mypy runs in CI and system hooks must be skipped there, which would lose the gate entirely.

Completes issue lfreleng-actions#385. An autouse fixture already cleared gerrit_urls._BASE_PATH_CACHE; the sibling caches it named were left to a follow-up. ssh_config_parser memoises four values, and each derives from state tests routinely monkeypatch. Two are lru_cache(maxsize=1) over zero-argument functions reading G2G_RESPECT_USER_SSH and git config user.email, so with one slot and no key *any* two tests collide and the second sees the first one's environment. derive_gerrit_credentials keys on (host, organization, port) while its answer also depends on those two caches and on ~/.ssh/config, none of which appear in the key. _ssh_config_cache keys on the config path, never its contents, so rewriting the file under an unchanged HOME serves the previous parse. The module already exposed clear_credential_cache(), which resets all four, but only two test modules called it and only from setup_method: that guards those files on the way in while leaving the caches populated for whatever runs next. Calling it from an autouse fixture covers every test, and clearing on both sides stops a dirty cache reaching a test as well as leaving one behind. No src/ change was needed. No leak between existing tests was demonstrable, so this is latent rather than live. Every test reaching derive_gerrit_credentials either patches it wholesale or clears in setup_method, which is why it has held so far. It would bite whenever someone adds a test that derives credentials for real, and would surface far from its cause. The new tests pair a polluter with a victim for each cache. Verified non-vacuous: with the autouse fixtures disabled all five victims fail, with stale environment, stale git identity and a stale SSH config parse respectively. The explicit clear in test_gerrit_urls_more.py at module scope is now redundant and goes. The one mid-test survives, because it is not a boundary: without it the second half of that test never probes at all and its assertions pass off the value cached by the first half. Its comment claimed the fixture covered it, which was wrong. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

Closes issue lfreleng-actions#386. Duplicate detection short-circuited on GERRIT_SERVER and GERRIT_PROJECT from the environment, but by the time it runs those are usually derived fallbacks rather than operator intent: _load_effective_inputs derives them and writes them to the environment first. Two consequences. The .gitreview resolution below that early return was effectively dead on the normal path, including the base-ref pinning added for fork pull requests. And the derived project is a guess taken from the GitHub repository name, so the OpenDaylight repository integration-distribution yielded that same string where Gerrit holds integration/distribution. Detection queried a project that does not exist and reported no duplicates, silently. Config values now carry provenance. mark_derived_keys records which keys hold a derived fallback and is_derived_key reports it, backed by G2G_DERIVED_KEYS. That is process-scoped configuration, settled once before the bulk thread pool starts, so unlike per-pull-request state it is safe to hold in the environment. apply_parameter_derivation marks only those keys whose environment value is empty, matching the test apply_config_to_env applies moments later; a key derivation supplies but the environment already defines explicitly stays explicit. The save-after-success call site opts out, since it persists to the config file without ever touching the environment. Duplicate detection now short-circuits only when both host and project are explicit, letting the per-pull-request .gitreview resolve otherwise. Where .gitreview yields nothing the derived pair still applies, because returning nothing would disable detection entirely wherever it previously ran against a derived project; a query against a possibly wrong project is no worse than no query. The fork-trust handling is untouched. Provenance is tracked per field, so a partially configured target keeps operator intent: where only one of the pair is derived, .gitreview supplies that field alone and the explicit one survives, and only where it carries a value. A .gitreview naming a host but no project parses successfully, and taking its empty project would drop the project qualifier from the query and search every project on the server, letting an unrelated change of the same title block a legitimate submission. A GERRIT_PROJECT arriving from the configuration file counts as derived, whichever release or hand wrote it. That file is sectioned per-organization while a project is per-repository, so a value there is wrong for every repository in the org but one; treating it as a fallback keeps it usable while letting .gitreview outrank it, and covers configurations written before such saves stopped. Derived parameters persisted to the configuration file now omit GERRIT_PROJECT. That value is per-repository while the file section is per-organization, so saving it handed every other repository in the org one repository's project name, and it returned on the next run as configuration indistinguishable from operator intent. Verified against the unmodified detector: the new test for a project containing a path separator fails there, showing the hyphenated name in the live query URL. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

Copilot

Pull request overview

Extends mypy coverage across all Python code and fixes CLI boolean flags being incorrectly overridden by environment variables. The branch also contains stacked prerequisite changes from #393 and #396.

Changes:

Expands mypy’s pre-commit scope and dependencies.
Resolves newly exposed typing errors without suppressions.
Fixes and regression-tests CLI boolean precedence.

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`.pre-commit-config.yaml`	Expands the mypy hook and dependencies.
`pyproject.toml`	Configures strict package typing and relaxed test annotations.
`uv.lock`	Locks the new PyYAML stubs.
`src/github2gerrit/cli.py`	Fixes CLI flag precedence.
`src/github2gerrit/core.py`	Makes the `GerritInfo` re-export explicit.
`src/github2gerrit/config.py`	Adds stacked configuration provenance handling.
`src/github2gerrit/duplicate_detection.py`	Uses stacked configuration provenance.
`src/github2gerrit/ssh_config_parser.py`	Adds stacked cache-correctness changes.
`tests/conftest.py`	Resets stacked provenance state.
`tests/test_cache_isolation.py`	Tests stacked cache isolation.
`tests/test_cli.py`	Adds precise Typer result typing.
`tests/test_cli_helpers.py`	Regression-tests boolean precedence.
`tests/test_composite_action_coverage.py`	Annotates environment mappings.
`tests/test_config_helpers.py`	Tests stacked provenance behavior.
`tests/test_duplicate_detection.py`	Tests stacked resolution behavior.
`tests/test_error_codes.py`	Preserves enum checks without false positives.
`tests/test_gerrit_ssh_abandon.py`	Uses a type-checker-friendly patch target.
`tests/test_gerrit_urls_more.py`	Documents required mid-test cache clearing.
`tests/test_mapping_comment_digest_and_backref.py`	Removes a stale suppression.
`tests/test_misc_small_coverage.py`	Removes a stale suppression.
`tests/test_reconciliation_plan_and_orphans.py`	Types decoded JSON mappings.
`tests/test_ssh_agent_ownership.py`	Avoids incorrect mypy narrowing.
`tests/unit/test_config_integration.py`	Annotates configuration dictionaries.
`tests/unit/test_ssh_config_parser.py`	Tests stacked cache behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Closes issue lfreleng-actions#394. derive_gerrit_credentials memoised on (gerrit_host, organization, gerrit_port) while its answer depended on three things absent from that key: the G2G_RESPECT_USER_SSH environment variable, the SSH config on disk, and the git identity. Since that flag selects between the user's own credentials and the organisation service account, a stale entry served personal SSH credentials where the service account was required. The failure was latent rather than live: cli._load_effective_inputs sets the environment variable at line 2330 and the only chain reaching the cache starts at line 2344, so the first read happened after the mutation. It sat one new caller or one reordering away from serving the wrong identity, with no error to point at a cache. Both helper memos were lru_cache(maxsize=1) over zero-argument functions, which is a global variable rather than a cache: with no key there is a single slot, so any second caller collides with the first. All three memos are gone, and the misleading _get_cached_git_user_email is now _read_git_user_email. The parsed SSH config is no longer cached either. Keying on the path served a stale parse once the file was rewritten, and keying on stat metadata still collides when an in-place rewrite preserves size and inode on a filesystem with coarse timestamps; a sound key means hashing the contents, which costs more than the parse it saves. The eviction it needed was also not thread-safe, in a module that advertises thread safety and a tool that runs a thread pool. Parsing costs around 0.2ms for a fifty-host config, once or twice per process, so the cache was never worth a correctness risk. Holding no process-wide state is what makes the thread-safety claim true rather than aspirational. Removing the memos costs one extra git config lookup per process, around 12ms, and only in local mode: GitHub Actions runs never read these sources at all. The parsed SSH config still loads once. Set against _load_effective_inputs at 236ms and a run that clones over the network, the memo was buying noise at the price of a wrong-identity failure mode. The autouse fixture clearing these caches around every test goes with them, along with clear_credential_cache and clear_ssh_config_cache and the setup_method calls to them: with no state to reset there is nothing left to clear. Tests that asserted the caching directly now assert that each source is re-read. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

_resolve_bool_override decides whether a boolean option came from the command line, so that an explicit flag wins and the environment applies only as a default. It asked Click for the parameter source and compared the answer against click.core.ParameterSource.COMMANDLINE. typer 0.20 and later vendor their own copy of click at typer._click, so the context returns a typer._click.core.ParameterSource member. That is a different enum class from click.core.ParameterSource, and members of different classes never compare equal, so the branch was unreachable and every explicit flag lost to the variable behind it. The docstring promised the opposite. Comparing the member name instead is correct whichever copy typer hands back, and does not depend on the two packages sharing an import. Nothing tested this, which is how it shipped: the suite passes either way. The regression test drives a real Typer invocation, since the parameter source is only populated by genuine argument parsing, and covers both flag directions along with the GitHub Actions case that motivated the helper, where the string "false" must parse rather than be coerced to True. Both type checkers had already reported it. The line carried a pyright: ignore listing reportUnnecessaryComparison, which was basedpyright saying this comparison can never be true. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

@pytest

Closes issue lfreleng-actions#395. The mypy hook ran on src/ alone, and the settings written for tests/ never applied: mypy reported them as unused sections, because the module names they targeted do not exist. tests/ has no __init__.py, so mypy names its modules test_cli and conftest, never tests.test_cli. Correcting the names is not enough either. A pattern cannot express "every test module", because mypy accepts * only as a whole dotted component; test_* is rejected outright with "Patterns must be fully-qualified module names", the complaint goes to stderr, mypy still exits 0, and the whole section is discarded in silence. Enumerating the names would work but would under-cover every file added afterwards. Inverting the default is the only form that both applies and keeps applying: annotation rules relax globally and switch back on for github2gerrit.*, a structured wildcard that always matches. Tests and scripts keep every correctness check that strict mode brings, losing only the demand to annotate signatures, which mirrors the basedpyright stance already recorded for tests/. The exclude list was misleading in the same way. It skips directory discovery only, so files named on the command line are still checked; since the hook passes filenames, every path listed there was already being checked. Trimmed to paths holding no first-party Python, which brought five more test files under the checker. Widening the hook needed real packages rather than more stubs. Without pytest installed, every @pytest.fixture reads as untyped and each pytest.raises block turns the code after it unreachable, for 42 errors that never appear locally. Suppressing those would have meant disabling two checks and dropping warn_unused_configs; installing the packages removes the class at its root. Fixed along the way: GerritInfo is now re-exported explicitly from core, which no_implicit_reexport requires and which 21 tests and a comment at core.py already depended on. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated no new comments.

ModeSevenIndustrialSolutions and others added 2 commits August 26, 2026 09:09

Copilot AI balanced review requested due to automatic review settings August 27, 2026 13:59

github-actions Bot added the CI CI and tests updates label Aug 27, 2026

Copilot started reviewing on behalf of ModeSevenIndustrialSolutions August 27, 2026 13:59 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

ModeSevenIndustrialSolutions and others added 3 commits August 27, 2026 15:17

ModeSevenIndustrialSolutions force-pushed the fix/mypy-tests branch from 64af213 to 734783a Compare August 27, 2026 14:20

Copilot AI review requested due to automatic review settings August 27, 2026 14:20

Copilot started reviewing on behalf of ModeSevenIndustrialSolutions August 27, 2026 14:20 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

This was referenced Aug 27, 2026

typer vendors its own click, so click identity checks silently fail #398

Open

Fix: Stop mixing typer's click with the real one #399

Draft

ModeSevenIndustrialSolutions self-assigned this Aug 28, 2026

Navigation Menu

Uh oh!

CI: Extend mypy to tests and scripts - #397

CI: Extend mypy to tests and scripts#397
ModeSevenIndustrialSolutions wants to merge 5 commits into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:fix/mypy-tests

ModeSevenIndustrialSolutions commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ModeSevenIndustrialSolutions commented Aug 27, 2026

The headline is not the mypy config

_resolve_bool_override never let a CLI flag win

The mypy work

Why the config was dead

A second misleading setting

The hook needed packages, not stubs

On the comparison-overlap and unreachable findings

Validation

Worth a reviewer's eye

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`_resolve_bool_override` never let a CLI flag win

On the `comparison-overlap` and `unreachable` findings