Fix: Resolve open cache-isolation and config-provenance issues

.pre-commit-config.yaml

@@ -67,10 +67,10 @@ repos:
     rev: aab412d509121cb5f7533134b7e67f9fab59c682  # frozen: v0.16.4
     hooks:
       - id: ruff
-        files: ^(src|scripts|tests)/.+\.py$
+        files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$
         args: [--fix, --exit-non-zero-on-fix]
       - id: ruff-format
-        files: ^(src|scripts|tests)/.+\.py$
+        files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$
 
   - repo: https://github.com/pre-commit/mirrors-mypy
     rev: 7ff8d35ae36a7d2b968f2f90b4c723e292e594ee  # frozen: v2.3.1
@@ -123,7 +123,9 @@ repos:
     rev: 3c94d276aa3005fa1f5a977946c9dc106135c083  # frozen: 1.39.10
     hooks:
       - id: basedpyright
-        files: ^src/.+\.py$
+        # Every Python path in the repository. Keep in sync with the
+        # `include` list under [tool.pyright] in pyproject.toml.
+        files: ^(src|scripts|tests)/.+\.py$|^sitecustomize\.py$
 
   - repo: https://github.com/lfreleng-actions/gha-workflow-linter
     rev: 42aa4db931ab4c9da0930ae5ca2259ab08da79cb  # frozen: v1.5.2

pyproject.toml

@@ -107,6 +107,12 @@ dev = [
   "coverage[toml]>=7.10.6",
   "responses>=0.25.8",
 
+  # Several tests read CliRunner's result.stderr, which click only
+  # captures separately from 8.2 onward; before that it raises
+  # ValueError. This is a test-only requirement, so the runtime floor
+  # above stays where it is: the package itself still works on 8.1.7.
+  "click>=8.2",
+
   # Lint/format
   "ruff>=0.6.3",
   "mypy>=1.17.1",
@@ -254,7 +260,10 @@ markers = [
 
 [tool.pyright]
 pythonVersion = "3.11"
-include = ["src"]
+# Every Python path in the repository, so an editor's language server and
+# the pre-commit hook agree on the checked set. Keep in sync with the
+# basedpyright hook's `files` pattern in .pre-commit-config.yaml.
+include = ["src", "tests", "scripts", "sitecustomize.py"]
 typeCheckingMode = "strict"
 reportMissingImports = "none"
 reportMissingTypeStubs = "none"
@@ -278,6 +287,21 @@ reportIncompatibleVariableOverride = "error"
 reportOverlappingOverload = "error"
 reportUnsupportedDunderAll = "error"
 
+# Test code is held to the same correctness rules as src/, but not to the
+# same annotation rules. This mirrors the mypy stance above, where tests
+# set disallow_untyped_defs = false.
+[[tool.pyright.executionEnvironments]]
+root = "tests"
+# pytest resolves fixtures by parameter name at collection time, so
+# annotating every `monkeypatch`, `tmp_path` and `mock_*` parameter adds
+# no checking that the collection step does not already perform.
+reportMissingParameterType = "none"
+# @pytest.fixture and @pytest.mark.* read as untyped whenever pytest is
+# not importable. The pre-commit hook runs basedpyright without a project
+# virtualenv, so this fires there but not locally. The same tolerance is
+# already applied repository-wide via reportMissingImports = "none".
+reportUntypedFunctionDecorator = "none"
+
 
 [tool.uv]
 # uv will manage installation based on this pyproject and lockfile.
@@ -289,4 +313,11 @@ managed = true
 dev = [
     "hatch-vcs>=0.5.0",
     "hatchling>=1.28.0",
+    # Pull in the dev extra defined above. uv installs the `dev` group by
+    # default, so a bare `uv sync` provisions pytest and friends; without
+    # this, `uv run pytest` silently falls through to whatever pytest sits
+    # on PATH. Declared by reference rather than duplicated because CI
+    # installs the extra directly (`uv pip install ./[test,dev]`) and
+    # cannot see dependency groups.
+    "github2gerrit[dev]",
 ]

src/github2gerrit/cli.py

@@ -762,10 +762,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(

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/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.

tests/conftest.py

@@ -231,6 +231,74 @@ def reset_gerrit_base_path_cache() -> Iterable[None]:
     gerrit_urls._BASE_PATH_CACHE.clear()
 
 
+@pytest.fixture(autouse=True)
+def reset_credential_caches() -> Iterable[None]:
+    """Clear the process-wide credential caches around every test.
+
+    ⚠️  IMPORTANT: autouse=True is REQUIRED for test suite stability
+
+    ``ssh_config_parser`` memoises four things for the lifetime of the
+    process, and every one of them derives its value from state that
+    tests routinely monkeypatch:
+
+    * ``_get_respect_user_ssh_setting`` — ``lru_cache(maxsize=1)`` over a
+      zero-argument function reading ``G2G_RESPECT_USER_SSH``.  With no
+      arguments there is a single cache slot, so *any* two tests collide
+      on it and the second receives the first one's environment.
+    * ``_get_cached_git_user_email`` — ``lru_cache(maxsize=1)`` over a
+      zero-argument function that shells out to ``git config``.  Same
+      single-slot collision, over the git identity that
+      ``isolate_git_environment`` takes care to control.
+    * ``derive_gerrit_credentials`` — ``lru_cache(maxsize=32)`` keyed on
+      ``(host, organization, port)``.  Tests share fixture hosts and
+      organisations, so the key collides while the answer depends on
+      the two caches above plus the SSH config on disk.
+    * ``_ssh_config_cache`` — keyed on ``~/.ssh/config`` as a path
+      string, never on that file's contents, so rewriting the file
+      under an unchanged ``HOME`` serves the previous parse.
+
+    ``clear_credential_cache()`` resets all four.  Two test modules call
+    it from ``setup_method``, which protects them on the way in but
+    leaves the caches populated for whatever runs next.  Doing it here
+    instead covers every test, and clearing on both sides stops a dirty
+    cache reaching a test as well as leaving one behind.
+    """
+    from github2gerrit.ssh_config_parser import clear_credential_cache
+
+    clear_credential_cache()
+    yield
+    clear_credential_cache()
+
+
+@pytest.fixture(autouse=True)
+def reset_derived_key_provenance() -> Iterable[None]:
+    """Clear the derived-config-key record around every test.
+
+    ⚠️  IMPORTANT: autouse=True is REQUIRED for test suite stability
+
+    ``config.mark_derived_keys`` records provenance in the
+    ``G2G_DERIVED_KEYS`` environment variable, and is deliberately
+    additive so repeated derivation passes accumulate.  Any test that
+    calls ``apply_parameter_derivation`` therefore leaves a record
+    behind, and it outlives ``monkeypatch`` teardown whenever the test
+    reached ``os.environ`` directly rather than through the fixture.
+
+    A stale record makes ``config.is_derived_key`` answer ``True`` for a
+    key a later test set explicitly, which is exactly the distinction
+    duplicate detection relies on -- so the leak would surface as a
+    short-circuit that silently stopped happening, in an unrelated
+    module, depending on test order.
+
+    Clearing on both sides keeps a dirty record from reaching a test and
+    stops one leaving a record behind.
+    """
+    from github2gerrit.config import DERIVED_KEYS_ENV
+
+    os.environ.pop(DERIVED_KEYS_ENV, None)
+    yield
+    os.environ.pop(DERIVED_KEYS_ENV, None)
+
+
 @pytest.fixture(autouse=True)
 def isolate_git_environment(monkeypatch: pytest.MonkeyPatch) -> None:
     """

tests/fixtures/make_repo.py

@@ -87,16 +87,9 @@ def _run_git(
     )
     if check and proc.returncode != 0:
         cmd_str = " ".join(full_cmd)
-        stderr = (
-            proc.stderr.decode("utf-8", errors="replace")
-            if proc.stderr is not None and isinstance(proc.stderr, bytes)
-            else proc.stderr
-        )
-        stdout = (
-            proc.stdout.decode("utf-8", errors="replace")
-            if proc.stdout is not None and isinstance(proc.stdout, bytes)
-            else proc.stdout
-        )
+        # capture_output=True with text=False always yields bytes.
+        stderr = proc.stderr.decode("utf-8", errors="replace")
+        stdout = proc.stdout.decode("utf-8", errors="replace")
         raise RuntimeError(
             f"Git command failed: {cmd_str}\n"
             f"Return code: {proc.returncode}\n"