Fix: Stop mixing typer's click with the real one - #399
Draft
ModeSevenIndustrialSolutions wants to merge 6 commits into
Draft
Fix: Stop mixing typer's click with the real one#399ModeSevenIndustrialSolutions wants to merge 6 commits into
ModeSevenIndustrialSolutions wants to merge 6 commits into
Conversation
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>
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>
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>
Closes issue lfreleng-actions#398. typer vendors a complete private copy of click from 0.26 onward, so every symbol the two packages appear to share is a distinct object and any identity, isinstance or except relationship between them silently fails to hold. The project floor of typer 0.20.1 spans both regimes, which is how the difference went unnoticed. _SingleUsageGroup subclassed click.Group and was passed as the app's cls to force a simplified usage line. It was never instantiated: typer builds a TyperCommand for a single-command app, so a Group subclass could not apply even before vendoring made the class hierarchies unrelated. format_usage never ran, and --help is byte-identical with the class removed. It goes, along with the two Protocol classes that existed only to type its parameters and the cast(Any, ...) that stopped a type checker pointing any of this out. A test now asserts the usage line, so the intent is checked rather than merely expressed. Confirmed to detect drift: adding a second command makes typer build a Group, the line gains COMMAND, and the test fails. That is the case the deleted class was written for, now covered by something that runs. A second test records the vendoring executably, asserting where typer's symbols come from and that raising typer.Exit is not caught by an except naming click's. It asserts through __module__ rather than an identity comparison, which a type checker folds to a constant and reports as non-overlapping, and it skips below typer 0.26 rather than failing where the two really are the same object. click stops being a dependency. Its declaration named the Group subclass and click.core.ParameterSource as the reasons, and both were things that did not work; nothing imports click now. Verified by installing without the dev extra, where click is absent entirely and the CLI still runs. The dev extra keeps click>=8.2, whose reason is now recorded accurately: typer.testing.CliRunner subclasses click's up to typer 0.25, so result.stderr comes from the installed click there and raises below 8.2. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
Copilot started reviewing on behalf of
ModeSevenIndustrialSolutions
August 27, 2026 15:25
View session
There was a problem hiding this comment.
Pull request overview
Removes invalid Click/Typer interoperability and replaces dead usage formatting with regression coverage.
Changes:
- Removes the unused
click.Groupcustomization. - Drops Click as a runtime dependency while retaining its test-only pin.
- Adds help-output and vendored-Click compatibility tests.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pyproject.toml |
Removes runtime Click dependency. |
src/github2gerrit/cli.py |
Deletes ineffective Click-based formatting. |
tests/test_cli_helpers.py |
Adds usage and namespace regression tests. |
uv.lock |
Updates locked runtime dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Stacked on #393 → #396 → #397. Review only the last commit,
Fix: Stop mixing typer's click with the real one. The earlier three belong to those PRs and drop out as they merge.Closes #398
The mechanism, pinned down
Writing #398 I knew typer vendored click. Building the fix turned up the part that actually explains everything: the vendoring began mid-range, and this project's floor spans both regimes.
ctx.get_parameter_source(...)returns== click.core.ParameterSource.COMMANDLINEclick.core.ParameterSourcetyper._clicktyper._click.core.ParameterSourcetyper>=0.20.1admits both. So the flag-precedence bug fixed in #397 appeared on a typer upgrade, with no code change, no error and no test to catch it. The.namecomparison used there is verified working under both regimes.typer 0.27.1 declares no click dependency at all; 0.23.0 declares
click>=8.0.0.Instance 2 — the live one, removed
passed as
cls=cast(Any, _SingleUsageGroup). Instrumented:Two independent reasons it could never work: typer builds a
Command, not aGroup, for a single-command app; and under vendoring the class hierarchies are unrelated anyway.cast(Any, ...)is what stopped a type checker saying so.Deleted, along with
BaseGroup,_FormatterProtoand_ContextProto— the latter two existed solely to type a method nobody called.Proof it was inert:
--helpoutput is byte-identical before and after (diffclean).What replaces it: a test that runs
The intent — pin the usage line — is now expressed as an assertion rather than as dead code. Verified to detect drift using the exact scenario the class was written for; adding a second command makes typer build a
Group:A second test records the vendoring executably: where typer's symbols come from, and that raising
typer.Exitis not caught by anexcept click.exceptions.Exit. Itskips below typer 0.26 rather than failing where the two genuinely are the same object.That test went through two corrections worth noting, both caught by the gates rather than by me:
typer.Exit is not click.exceptions.Exit. mypy rejected it as a non-overlapping identity check — it already knows the types are unrelated, making the assertion statically vacuous. It now asserts__module__, which is a runtime fact a checker cannot fold.import clickalone is not enough to reachclick.testingorclick.exceptions; under vendoring nothing else imports those submodules, so it fails withAttributeError: testing.clickis no longer a dependencyIts declaration named the two things that do not work:
Nothing imports click now. Verified end-to-end with a runtime-only install:
One thing I nearly got wrong
I set out to remove the
click>=8.2dev pin too, reasoning that sincetyper.testingis independent of installed click, the pin was pointless. It is load-bearing, and I checked before acting:Up to typer 0.25,
typer.testing.CliRunnersubclasses click's, soresult.stderrcomes from the installed click. The pin stays — with a comment that now states the real reason. My original rationale for it in #392 was wrong (it namedclick.testing.CliRunner, but the tests usetyper.testing); the conclusion happened to be right.Validation
pytestmypy(124 files)basedpyright(124 files)ruff check/formataislop ciprek run --all-files--helpbefore vs afterSuppressions: net −1. The
cast(Any, ...)is gone and nothing replaced it. Combined with #397, both suppressions that existed incli.pyhave now been removed, and each had been masking a real bug.Not done here
The
Contextdivergence (typer.Contextistyper.models.Context, notclick.Context) is untouched: the codebase already annotates withtyper.Contextthroughout, which is correct. Nothing else mixes the namespaces — the twoclick.mentions remaining intests/are inside docstrings and an assertion about an error string, not imports.