CI: Extend mypy to tests and scripts

.pre-commit-config.yaml

@@ -76,8 +76,26 @@ repos:
     rev: 7ff8d35ae36a7d2b968f2f90b4c723e292e594ee  # frozen: v2.3.1
     hooks:
       - id: mypy
-        files: ^src/.+\.py$
+        # Every Python path in the repository, matching the ruff and
+        # basedpyright hooks above and below so the three cannot drift.
+        files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$
+        # mypy runs in an isolated virtualenv that holds only what is
+        # listed here, so anything the checked files import has to be
+        # present or mypy draws conclusions the project never would. With
+        # pytest absent, for instance, @pytest.fixture reads as an untyped
+        # decorator and pytest.raises stops telling mypy the block can
+        # swallow the exception, which turns every statement after a
+        # `with pytest.raises(...)` into dead code. Installing the real
+        # packages removes that whole class of environment-only findings;
+        # keep this list in step with the imports under src/ and tests/.
+        # pygerrit2 is deliberately absent: it is an optional runtime
+        # dependency and pyproject.toml already gives it
+        # ignore_missing_imports.
         additional_dependencies:
+          - click>=8.2
+          - pytest
+          - responses
+          - typer
           - types-PyYAML
           - types-requests
 

pyproject.toml

@@ -119,6 +119,7 @@ dev = [
 
   # Type checking helpers
   "pytest-mock>=3.15.1",
+  "types-PyYAML>=6.0.12",
   "types-requests>=2.31.0",
   "types-urllib3>=1.26.25.14",
 ]
@@ -166,40 +167,49 @@ warn_unreachable = true
 warn_unused_ignores = true
 disallow_any_generics = true
 disallow_subclassing_any = true
-disallow_untyped_defs = true
-disallow_incomplete_defs = true
-disallow_untyped_calls = true
+# Annotation rules are relaxed here and switched back on for the shipped
+# package by the github2gerrit.* override below. The obvious shape --
+# global strictness plus a tests.* relaxation -- cannot work, because
+# neither half of it matches:
+#
+#   * tests/ has no __init__.py, so mypy names its modules test_cli,
+#     conftest and so on, never tests.test_cli.
+#   * A pattern cannot recover the real names either. mypy accepts "*"
+#     only as a whole dotted component, so "test_*" is rejected outright
+#     ("Patterns must be fully-qualified module names, optionally with
+#     '*' in some components") and the section is discarded in silence.
+#
+# Inverting the default is the only form that both applies and keeps
+# applying when test files are added. It also suits scripts/, which is
+# unannotated by design.
+disallow_untyped_defs = false
+disallow_incomplete_defs = false
+disallow_untyped_calls = false
 no_implicit_optional = true
 show_error_codes = true
 pretty = true
-exclude = [
-  "build/",
-  "dist/",
-  "docs/",
-  "scripts/",
-  "tests/fixtures/",
-  "tests/test_composite_action_coverage.py",
-  "tests/test_action_environment_mapping.py",
-  "tests/test_action_step_validation.py",
-  "tests/test_action_outputs.py",
-  "tests/test_trailers_additional.py",
-]
-
+# exclude skips directory discovery only; files named explicitly on the
+# command line are still checked. The pre-commit hook passes filenames,
+# so anything listed here would be checked there regardless -- keep this
+# to paths that hold no first-party Python.
+exclude = ["build/", "dist/", "docs/"]
+
+# The shipped package is held to full strict-mode annotation rules.
+# Everything else -- tests/, scripts/, sitecustomize.py -- keeps every
+# correctness check (check_untyped_defs, warn_unreachable,
+# strict_equality, no_implicit_reexport) but is not required to annotate
+# signatures. This mirrors the basedpyright stance recorded in
+# [[tool.pyright.executionEnvironments]] below, where #392 disabled
+# reportMissingParameterType for tests/ because pytest already resolves
+# fixtures by parameter name.
 [[tool.mypy.overrides]]
-module = [
-    "tests.test_composite_action_coverage",
-    "tests.test_action_environment_mapping",
-    "tests.test_action_step_validation",
-    "tests.test_action_outputs",
-    "tests.test_trailers_additional",
-]
-disallow_untyped_defs = false
-disallow_incomplete_defs = false
-disallow_untyped_calls = false
-check_untyped_defs = false
+module = ["github2gerrit.*"]
+disallow_untyped_defs = true
+disallow_incomplete_defs = true
+disallow_untyped_calls = true
 
 [[tool.mypy.overrides]]
-module = ["pygerrit2", "pygerrit2.*"]
+module = ["pygerrit2"]
 ignore_missing_imports = true
 
 [[tool.mypy.overrides]]
@@ -210,21 +220,12 @@ ignore_missing_imports = true
 module = ["typer", "typer.*"]
 ignore_missing_imports = true
 
-[[tool.mypy.overrides]]
-module = ["pytest", "pytest.*"]
-ignore_missing_imports = true
-
 [[tool.mypy.overrides]]
 module = ["github2gerrit.rich_display"]
 disallow_untyped_defs = false
 disallow_untyped_calls = false
 check_untyped_defs = false
 
-[[tool.mypy.overrides]]
-module = ["tests.*"]
-disallow_untyped_defs = false
-disallow_untyped_calls = false
-
 [tool.coverage.run]
 branch = true
 # Keep transient coverage data out of the working tree so tests that

src/github2gerrit/cli.py

@@ -30,7 +30,6 @@
 from urllib.parse import urlparse
 
 import click
-import click.core
 import typer
 
 from . import models
@@ -762,10 +761,18 @@ def _save_derived_parameters_after_success(data: Inputs) -> None:
         cfg = load_org_config(org_for_cfg)
 
         # Apply derivation with saving enabled to capture newly derived
-        # parameters
+        # parameters.  The result is written to the configuration file
+        # only: this runs after submission and never calls
+        # apply_config_to_env, so nothing derived here reaches the
+        # process environment and recording it as derived provenance
+        # would describe environment values this call did not set.
         repository = os.getenv("GITHUB_REPOSITORY", "")
         apply_parameter_derivation(
-            cfg, org_for_cfg, repository=repository, save_to_config=True
+            cfg,
+            org_for_cfg,
+            repository=repository,
+            save_to_config=True,
+            mark_derived=False,
         )
 
         log.debug(
@@ -810,7 +817,13 @@ def _resolve_bool_override(
     provided on the command line, so CLI flags always take precedence.
     """
     source = ctx.get_parameter_source(param_name)  # pyright: ignore[reportAttributeAccessIssue]
-    if source == click.core.ParameterSource.COMMANDLINE:  # pyright: ignore[reportAttributeAccessIssue, reportUnnecessaryComparison]
+    # Compare by member name rather than by identity with a ParameterSource
+    # member. typer >= 0.20 ships its own copy of click under typer._click,
+    # so ctx.get_parameter_source returns a
+    # typer._click.core.ParameterSource member, which never compares equal
+    # to the click.core.ParameterSource member of the same name. Matching
+    # on .name is correct whichever copy typer hands back.
+    if source is not None and source.name == "COMMANDLINE":
         return current
     env_val = os.getenv(env_var)
     if env_val is not None:

src/github2gerrit/config.py

@@ -49,6 +49,7 @@
 import logging
 import os
 import re
+from collections.abc import Iterable
 from pathlib import Path
 from typing import Any
 from typing import cast
@@ -476,6 +477,59 @@ def apply_config_to_env(cfg: dict[str, str]) -> None:
             os.environ[k] = v
 
 
+# Provenance for configuration values.
+#
+# Several Gerrit parameters are *derived* when nothing configures them:
+# the host from a `gerrit.[org].org` heuristic, the project from the
+# GitHub repository name.  Once `apply_config_to_env` exports them they
+# are indistinguishable from values an operator set deliberately, so a
+# consumer with a better answer of its own (duplicate detection reads
+# the pull request's `.gitreview`) cannot tell whether overriding the
+# environment would be correcting a guess or discarding intent.
+#
+# Recording the derived keys answers that question.  The record lives in
+# an environment variable because it describes *process-scoped
+# configuration*, established once in `_load_effective_inputs` before
+# the bulk `ThreadPoolExecutor` starts and identical for every pull
+# request the process handles.  That is the opposite of per-pull-request
+# state such as the approved SHA, which `cli.py` deliberately threads
+# through return values precisely because concurrent workers would
+# otherwise overwrite one another's values.
+DERIVED_KEYS_ENV = "G2G_DERIVED_KEYS"
+
+
+def _derived_keys() -> set[str]:
+    """Return the set of config keys currently recorded as derived."""
+    raw = os.getenv(DERIVED_KEYS_ENV, "")
+    return {part.strip().upper() for part in raw.split(",") if part.strip()}
+
+
+def mark_derived_keys(keys: Iterable[str]) -> None:
+    """Record which config keys hold derived fallbacks rather than
+    operator intent.
+
+    Additive: keys already recorded stay recorded, so several derivation
+    passes accumulate rather than the last one winning.
+
+    Args:
+        keys: Config key names (case-insensitive).  Empty names are
+            ignored.
+    """
+    incoming = {k.strip().upper() for k in keys if k and k.strip()}
+    if not incoming:
+        return
+    os.environ[DERIVED_KEYS_ENV] = ",".join(sorted(_derived_keys() | incoming))
+
+
+def is_derived_key(key: str) -> bool:
+    """Return ``True`` when ``key``'s current value came from derivation.
+
+    A ``False`` answer covers both "explicitly configured" and "not set
+    at all", so callers should test the value's presence separately.
+    """
+    return key.strip().upper() in _derived_keys()
+
+
 def filter_known(
     cfg: dict[str, str],
     include_extra: bool = True,
@@ -666,6 +720,8 @@ def apply_parameter_derivation(
     organization: str | None = None,
     repository: str | None = None,
     save_to_config: bool = True,
+    *,
+    mark_derived: bool = True,
 ) -> dict[str, str]:
     """Apply dynamic parameter derivation for missing Gerrit parameters.
 
@@ -689,6 +745,11 @@ def apply_parameter_derivation(
         organization: GitHub organization name for derivation
         repository: GitHub repository in owner/repo format (optional)
         save_to_config: Whether to save derived parameters to config file
+        mark_derived: Whether to record the derived keys as provenance
+            (see :func:`mark_derived_keys`).  Pass ``False`` from call
+            sites that do not follow up with :func:`apply_config_to_env`,
+            since a key that never reaches the environment must not be
+            described as holding a derived value there.
 
     Returns:
         Configuration dictionary with derived values for missing parameters
@@ -737,6 +798,34 @@ def apply_parameter_derivation(
             "GitHub Actions" if is_github_actions else "Local CLI",
             ", ".join(f"{k}={v}" for k, v in newly_derived.items()),
         )
+        if mark_derived:
+            # `newly_derived` records what derivation *supplied*, not
+            # what will end up in the environment.  `apply_config_to_env`
+            # writes a key only when its environment value is currently
+            # empty, so a key derived here can still be shadowed by an
+            # explicit environment value and must not be recorded as
+            # derived.  Apply the same emptiness test the caller will
+            # apply moments later; the two must agree.
+            mark_derived_keys(
+                key
+                for key in newly_derived
+                if (os.getenv(key) or "").strip() == ""
+            )
+
+    # A GERRIT_PROJECT arriving from the configuration file is a
+    # fallback, never intent for this specific repository: the file is
+    # sectioned per-organization, so one project name there is wrong for
+    # every repository in the org but one. That covers entries auto-saved
+    # by releases before such writes stopped, and hand-written ones,
+    # which are equally misscoped. Marking it derived keeps it usable as
+    # a last resort while letting the per-pull-request .gitreview
+    # outrank it.
+    if (
+        mark_derived
+        and cfg.get("GERRIT_PROJECT", "").strip()
+        and (os.getenv("GERRIT_PROJECT") or "").strip() == ""
+    ):
+        mark_derived_keys(["GERRIT_PROJECT"])
     # Save newly derived parameters to configuration file for future use
     # Default to true for local CLI, false for GitHub Actions
     default_auto_save = "false" if _is_github_actions_context() else "true"
@@ -789,6 +878,18 @@ def save_derived_parameters_to_config(
     if not organization or not derived_params:
         return
 
+    # GERRIT_PROJECT is per-repository, but this file section is
+    # per-organization: persisting it would hand every other repository
+    # in the org one repository's project name, and would additionally
+    # come back on the next run as configuration indistinguishable from
+    # operator intent, defeating the provenance tracking that lets
+    # duplicate detection prefer a per-pull-request .gitreview.
+    derived_params = {
+        k: v for k, v in derived_params.items() if k != "GERRIT_PROJECT"
+    }
+    if not derived_params:
+        return
+
     if config_path is None:
         config_path = (
             os.getenv("G2G_CONFIG_PATH", "").strip() or DEFAULT_CONFIG_PATH

src/github2gerrit/core.py

@@ -63,7 +63,7 @@
 from .github_api import get_recent_change_ids_from_comments
 from .github_api import get_repo_from_env
 from .github_api import iter_open_pulls
-from .gitreview import GerritInfo
+from .gitreview import GerritInfo as GerritInfo
 from .gitreview import fetch_gitreview
 from .gitreview import make_gitreview_info
 from .gitutils import CommandError
@@ -273,8 +273,9 @@ def _is_valid_change_id(value: str) -> bool:
 # GerritInfo is imported from .gitreview (aliased from GitReviewInfo).
 # It retains the same frozen-dataclass interface (host, port, project) plus
 # an additional optional ``base_path`` field (default ``None``).
-# The top-level import is sufficient for ``from github2gerrit.core import
-# GerritInfo`` to keep working — no explicit re-export needed.
+# The top-level import uses the redundant ``as GerritInfo`` form so that
+# ``from github2gerrit.core import GerritInfo`` keeps working under
+# no_implicit_reexport, which strict type checking enables.
 
 
 @dataclass(frozen=True)

src/github2gerrit/duplicate_detection.py

@@ -223,16 +223,24 @@ def _resolve_gerrit_info_from_env_or_gitreview(
         :mod:`github2gerrit.gitreview` module which consolidates parsing,
         URL-encoding, branch deduplication, and URL validation.
 
+        Explicit configuration outranks the per-pull-request
+        ``.gitreview`` field by field, since derivation guesses
+        ``GERRIT_PROJECT`` from the repository name. A derived pair is
+        the last resort: a possibly-wrong project beats no query.
+
         Returns:
             Tuple of (host, project) if found, None otherwise
         """
+        from .config import is_derived_key
         from .gitreview import fetch_gitreview_raw
 
-        # First try environment variables (same as core module)
         gerrit_host = os.getenv("GERRIT_SERVER", "").strip()
         gerrit_project = os.getenv("GERRIT_PROJECT", "").strip()
 
-        if gerrit_host and gerrit_project:
+        env_ok = bool(gerrit_host and gerrit_project)
+        host_ok = bool(gerrit_host) and not is_derived_key("GERRIT_SERVER")
+        proj_ok = bool(gerrit_project) and not is_derived_key("GERRIT_PROJECT")
+        if host_ok and proj_ok:
             return (gerrit_host, gerrit_project)
 
         # Skip local .gitreview check in composite action context.
@@ -243,7 +251,7 @@ def _resolve_gerrit_info_from_env_or_gitreview(
 
         repo_full = gh.repository.strip() if gh.repository else ""
         if not repo_full:
-            return None
+            return (gerrit_host, gerrit_project) if env_ok else None
 
         # Collect branch hints from the GitHub context. A fork may name
         # its branch after one that exists in the base repository, so an
@@ -269,9 +277,13 @@ def _resolve_gerrit_info_from_env_or_gitreview(
             default_branches=default_branches,
         )
         if info:
-            return (info.host, info.project)
+            # .gitreview fills non-explicit fields, where it has one.
+            host = gerrit_host if host_ok else info.host or gerrit_host
+            proj = gerrit_project if proj_ok else info.project or gerrit_project
+            if host and proj:
+                return (host, proj)
 
-        return None
+        return (gerrit_host, gerrit_project) if env_ok else None
 
     def _build_gerrit_rest_client(self, gerrit_host: str) -> Any | None:
         """Build a Gerrit REST API client using centralized framework.