From 35d3e8f692cac09d3e3ae07d04dfe50e078a66ad Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Wed, 26 Aug 2026 08:18:09 +0100 Subject: [PATCH 1/4] Fix: Correct type errors in the test suite basedpyright reported 104 errors across tests/ that src/ was already free of, because both the pre-commit hook and the [tool.pyright] include list covered src/ alone. Most were genuine. Tests passed a str where Inputs.gerrit_server_port is declared int, passed "" where a Path | None was expected, and set Orchestrator.workspace to a str immediately after the constructor had already stored the Path. Thirteen loops searched action.yaml for a named step and then used the result without checking that the search had found anything, so a renamed step surfaced as an opaque AttributeError on None rather than a message naming what was missing. Test stand-ins for GerritInfo and GitHubContext now build the real dataclasses, which checks their field names too. Inputs keeps its SimpleNamespace stand-in behind a cast, because reconstructing ~30 required fields to set four reconciliation switches would bury what each test actually varies. Also drops a click compatibility shim from _get_combined_output. It guarded against click below 8.2, where CliRunner mixes stderr into stdout and result.stderr raises ValueError. Three other CliRunner call sites already read result.stderr unguarded, so the suite has required click 8.2 for some time; one helper compensating on its own bought nothing. No assertion was removed or weakened, and no suppression comment was added. The suite collects and passes the same 1518 tests as before. Co-authored-by: Claude Signed-off-by: Matthew Watkins --- tests/fixtures/make_repo.py | 13 +-- tests/test_action_environment_mapping.py | 27 ++++++ tests/test_action_outputs.py | 3 + tests/test_cli.py | 20 ++-- tests/test_composite_action_coverage.py | 9 ++ tests/test_core_integration_fixture_repo.py | 2 +- tests/test_core_prepare_commits.py | 2 +- tests/test_core_ssh_setup.py | 10 +- tests/test_gerrit_change_id_footer.py | 2 +- tests/test_gerrit_pr_closer.py | 1 + tests/test_mapping_comment_additional.py | 91 ++++++++++++------- ...test_mapping_comment_digest_and_backref.py | 5 +- tests/test_orphan_rest_side_effects.py | 21 +++-- tests/test_pr_commands.py | 4 +- tests/test_pr_content_filter_integration.py | 2 +- tests/test_reconciliation_extracted_module.py | 66 +++++++++----- tests/test_reconciliation_plan_and_orphans.py | 66 +++++++++----- tests/test_ssh_artifact_prevention.py | 10 ++ 18 files changed, 226 insertions(+), 128 deletions(-) diff --git a/tests/fixtures/make_repo.py b/tests/fixtures/make_repo.py index f45724a..a27d553 100644 --- a/tests/fixtures/make_repo.py +++ b/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" diff --git a/tests/test_action_environment_mapping.py b/tests/test_action_environment_mapping.py index a8bafa5..e106ffd 100644 --- a/tests/test_action_environment_mapping.py +++ b/tests/test_action_environment_mapping.py @@ -81,6 +81,9 @@ def test_github_context_mapping(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Test GitHub context mappings @@ -113,6 +116,9 @@ def test_computed_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # These are computed by the upstream "Extract PR number" step and @@ -135,6 +141,9 @@ def test_issue_id_conditional_mapping(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) issue_id_env = env_mapping.get("ISSUE_ID", "") @@ -149,6 +158,9 @@ def test_test_mode_environment(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # G2G_TEST_MODE should be explicitly set to false in production @@ -296,6 +308,9 @@ def test_required_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Essential variables that must be present @@ -320,6 +335,9 @@ def test_boolean_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Boolean input variables @@ -348,6 +366,9 @@ def test_optional_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Optional variables that may be empty @@ -442,6 +463,9 @@ def test_secret_environment_variables(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Secret variables that should reference inputs or context @@ -463,6 +487,9 @@ def test_no_hardcoded_secrets(self, action_config): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) # Check that no environment values contain hardcoded secrets diff --git a/tests/test_action_outputs.py b/tests/test_action_outputs.py index 38bcecd..7c34745 100644 --- a/tests/test_action_outputs.py +++ b/tests/test_action_outputs.py @@ -119,6 +119,9 @@ def test_capture_step_script(self, action_config): None, ) + assert capture_step is not None, ( + "No step whose name contains 'Capture outputs' in action.yaml" + ) script = capture_step["run"] # Should use GitHub Actions multiline output format diff --git a/tests/test_cli.py b/tests/test_cli.py index b25b156..46bb880 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -14,23 +14,15 @@ from github2gerrit.cli import app -# Try to use mix_stderr=False for better error separation, but fall back to default -# if the parameter is not supported in this version of typer. -# The mix_stderr parameter was added in typer 0.9.0+ (requires click 8.1+). -# Older versions of typer/click will raise TypeError for unknown parameters. -try: - runner = CliRunner(mix_stderr=False) -except TypeError: - runner = CliRunner() +# The test suite requires click >= 8.2, pinned in the dev extra, where +# CliRunner always captures stderr separately and the mix_stderr +# parameter no longer exists. +runner = CliRunner() def _get_combined_output(result: Any) -> str: - """Get combined stdout and stderr, handling version differences.""" - try: - return result.stdout + (result.stderr or "") - except ValueError: - # stderr not separately captured, use stdout only - return result.stdout + """Get combined stdout and stderr from an invocation result.""" + return result.stdout + result.stderr def test_conflicting_options_error_message_in_stderr(tmp_path: Path) -> None: diff --git a/tests/test_composite_action_coverage.py b/tests/test_composite_action_coverage.py index a809293..1659039 100644 --- a/tests/test_composite_action_coverage.py +++ b/tests/test_composite_action_coverage.py @@ -354,6 +354,9 @@ def test_github_context_mapping(self, action_tester): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) github_vars = [ @@ -396,6 +399,9 @@ def test_issue_id_priority(self, action_tester): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) issue_id_env = env_mapping.get("ISSUE_ID", "") issue_id_lookup_json_env = env_mapping.get("ISSUE_ID_LOOKUP_JSON", "") @@ -642,6 +648,9 @@ def test_github_token_usage(self, action_tester): cli_step = step break + assert cli_step is not None, ( + "No step named 'Run github2gerrit Python CLI' in action.yaml" + ) env_mapping = cli_step.get("env", {}) assert env_mapping.get("GITHUB_TOKEN") == "${{ github.token }}" diff --git a/tests/test_core_integration_fixture_repo.py b/tests/test_core_integration_fixture_repo.py index 4273d76..bf7174f 100644 --- a/tests/test_core_integration_fixture_repo.py +++ b/tests/test_core_integration_fixture_repo.py @@ -36,7 +36,7 @@ def _minimal_inputs(*, dry_run: bool = False) -> Inputs: dry_run=dry_run, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="example/project", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_core_prepare_commits.py b/tests/test_core_prepare_commits.py index 1f6ae5f..1ba9654 100644 --- a/tests/test_core_prepare_commits.py +++ b/tests/test_core_prepare_commits.py @@ -30,7 +30,7 @@ def _minimal_inputs() -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="example/project", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_core_ssh_setup.py b/tests/test_core_ssh_setup.py index 2c44d7a..3a5eb82 100644 --- a/tests/test_core_ssh_setup.py +++ b/tests/test_core_ssh_setup.py @@ -87,7 +87,7 @@ def minimal_inputs() -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", @@ -185,7 +185,7 @@ def test_ssh_setup_skips_when_credentials_missing(tmp_path: Path) -> None: dry_run=False, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="example/project", issue_id="", issue_id_lookup_json="", @@ -348,7 +348,7 @@ def test_ssh_auto_discovery_integration( dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", @@ -417,7 +417,7 @@ def test_ssh_auto_discovery_fallback_when_discovery_fails( dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", @@ -481,7 +481,7 @@ def test_ssh_setup_augments_provided_known_hosts_with_autodiscovery( dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_gerrit_change_id_footer.py b/tests/test_gerrit_change_id_footer.py index df4e1a7..1e7e49b 100644 --- a/tests/test_gerrit_change_id_footer.py +++ b/tests/test_gerrit_change_id_footer.py @@ -57,7 +57,7 @@ def _inputs(*, use_pr_as_commit: bool = True) -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="", - gerrit_server_port="", + gerrit_server_port=29418, gerrit_project="", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_gerrit_pr_closer.py b/tests/test_gerrit_pr_closer.py index df69336..f9a0ae7 100644 --- a/tests/test_gerrit_pr_closer.py +++ b/tests/test_gerrit_pr_closer.py @@ -173,6 +173,7 @@ def test_handles_numeric_pr_numbers(self): result = parse_pr_url(url) + assert result is not None, "Expected a parsed (owner, repo, num)" assert result == ("test", "test", 99999) assert isinstance(result[2], int) diff --git a/tests/test_mapping_comment_additional.py b/tests/test_mapping_comment_additional.py index 11a0ba5..e0180d5 100644 --- a/tests/test_mapping_comment_additional.py +++ b/tests/test_mapping_comment_additional.py @@ -298,30 +298,45 @@ def test_reconciliation_all_new_ids_no_topic_no_comment(monkeypatch): is enabled, all commits receive brand new Change-Ids. """ from types import SimpleNamespace + from typing import cast + from github2gerrit.gitreview import GerritInfo + from github2gerrit.models import GitHubContext + from github2gerrit.models import Inputs from github2gerrit.orchestrator import perform_reconciliation from github2gerrit.reconcile_matcher import LocalCommit - class GH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 9 - - class Gerrit: - host = "gerrit.example.org" - port = 29418 + gh = GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="1", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=9, + ) + gerrit = GerritInfo(host="gerrit.example.org", port=29418) # Force empty topic results from github2gerrit.orchestrator import reconciliation as rmod monkeypatch.setattr(rmod, "query_changes_by_topic", lambda *a, **k: []) - inputs = SimpleNamespace( - reuse_strategy="topic", - allow_orphan_changes=False, - similarity_subject=0.7, - log_reconcile_json=False, + # ``Inputs`` carries ~30 required fields; this stand-in supplies only + # the reconciliation switches that ``perform_reconciliation`` reads, + # so the cast records a deliberately partial implementation. + inputs = cast( + Inputs, + SimpleNamespace( + reuse_strategy="topic", + allow_orphan_changes=False, + similarity_subject=0.7, + log_reconcile_json=False, + ), ) commits = [ LocalCommit( @@ -336,8 +351,8 @@ class Gerrit: ] result = perform_reconciliation( inputs=inputs, - gh=GH(), - gerrit=Gerrit(), + gh=gh, + gerrit=gerrit, local_commits=commits, ) assert len(result) == 3 @@ -351,31 +366,43 @@ def test_reconciliation_topic_query_exception(monkeypatch): (fallback path). """ from types import SimpleNamespace + from typing import cast + from github2gerrit.gitreview import GerritInfo + from github2gerrit.models import GitHubContext + from github2gerrit.models import Inputs from github2gerrit.orchestrator import perform_reconciliation from github2gerrit.orchestrator import reconciliation as rmod from github2gerrit.reconcile_matcher import LocalCommit - class GH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 11 - - class Gerrit: - host = "gerrit.example.org" - port = 29418 + gh = GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="1", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=11, + ) + gerrit = GerritInfo(host="gerrit.example.org", port=29418) def _boom(*_a, **_k): raise RuntimeError("boom") monkeypatch.setattr(rmod, "query_changes_by_topic", _boom) - inputs = SimpleNamespace( - reuse_strategy="topic", - allow_orphan_changes=False, - similarity_subject=0.7, - log_reconcile_json=False, + inputs = cast( + Inputs, + SimpleNamespace( + reuse_strategy="topic", + allow_orphan_changes=False, + similarity_subject=0.7, + log_reconcile_json=False, + ), ) commits = [ LocalCommit( @@ -389,8 +416,8 @@ def _boom(*_a, **_k): ] result = perform_reconciliation( inputs=inputs, - gh=GH(), - gerrit=Gerrit(), + gh=gh, + gerrit=gerrit, local_commits=commits, ) assert len(result) == 1 diff --git a/tests/test_mapping_comment_digest_and_backref.py b/tests/test_mapping_comment_digest_and_backref.py index 332354b..e4ea974 100644 --- a/tests/test_mapping_comment_digest_and_backref.py +++ b/tests/test_mapping_comment_digest_and_backref.py @@ -64,14 +64,13 @@ def test_idempotent_backref_skip_and_force( the back-reference for that commit should be skipped unless forced. """ # Prepare orchestrator with minimal required attributes - orch = Orchestrator(workspace=str(tmp_path)) - orch.workspace = str(tmp_path) + orch = Orchestrator(workspace=tmp_path) # Fake GitHub context gh = GitHubContext( event_name="pull_request_target", event_action="synchronize", - event_path="", + event_path=None, repository="org/repo", repository_owner="org", server_url="https://github.com", diff --git a/tests/test_orphan_rest_side_effects.py b/tests/test_orphan_rest_side_effects.py index 696423b..3ee07da 100644 --- a/tests/test_orphan_rest_side_effects.py +++ b/tests/test_orphan_rest_side_effects.py @@ -9,8 +9,11 @@ from __future__ import annotations +from typing import Any + import pytest +from github2gerrit.gitreview import GerritInfo from github2gerrit.orchestrator.reconciliation import _abandon_orphan_changes from github2gerrit.orchestrator.reconciliation import _comment_orphan_changes @@ -25,7 +28,7 @@ def __init__(self): # an authenticated client that can perform mutating REST calls. self.is_authenticated = True - def post(self, path: str, data: dict | None = None): + def post(self, path: str, data: dict[str, Any] | None = None): """Record POST calls and optionally simulate failures.""" self.post_calls.append((path, data)) @@ -42,11 +45,9 @@ def make_fail(self, path: str, error_msg: str): self.should_fail[path] = error_msg -class MockGerrit: - """Mock Gerrit info object.""" - - def __init__(self, host: str = "gerrit.example.org"): - self.host = host +def _gerrit_info(host: str = "gerrit.example.org") -> GerritInfo: + """Build genuine Gerrit connection info (only ``host`` is read).""" + return GerritInfo(host=host) @pytest.fixture @@ -57,8 +58,8 @@ def mock_client(): @pytest.fixture def mock_gerrit(): - """Provide a mock Gerrit info object.""" - return MockGerrit() + """Provide Gerrit connection info.""" + return _gerrit_info() def test_abandon_orphan_changes_success(mock_client, mock_gerrit, monkeypatch): @@ -202,7 +203,7 @@ def mock_build_client(host, **kwargs): def test_abandon_orphan_changes_no_ids(): """Test abandon with empty orphan list.""" - result = _abandon_orphan_changes([], MockGerrit()) + result = _abandon_orphan_changes([], _gerrit_info()) assert result == [] @@ -216,7 +217,7 @@ def test_abandon_orphan_changes_no_gerrit(): def test_comment_orphan_changes_no_ids(): """Test comment with empty orphan list.""" - result = _comment_orphan_changes([], MockGerrit()) + result = _comment_orphan_changes([], _gerrit_info()) assert result == [] diff --git a/tests/test_pr_commands.py b/tests/test_pr_commands.py index 949f05b..eae7c8d 100644 --- a/tests/test_pr_commands.py +++ b/tests/test_pr_commands.py @@ -414,7 +414,7 @@ def _make_inputs(self, create_missing: bool = False, **kwargs: Any): """Create a minimal Inputs object for testing.""" from github2gerrit.models import Inputs - defaults = { + defaults: dict[str, Any] = { "submit_single_commits": False, "use_pr_as_commit": False, "fetch_depth": 10, @@ -441,7 +441,7 @@ def _make_inputs(self, create_missing: bool = False, **kwargs: Any): defaults.update(kwargs) return Inputs(**defaults) - def _make_gh(self, pr_number: int = 42): + def _make_gh(self, pr_number: int | None = 42): """Create a minimal GitHubContext for testing.""" from github2gerrit.models import GitHubContext diff --git a/tests/test_pr_content_filter_integration.py b/tests/test_pr_content_filter_integration.py index 83b5898..7a7895e 100644 --- a/tests/test_pr_content_filter_integration.py +++ b/tests/test_pr_content_filter_integration.py @@ -33,7 +33,7 @@ def _create_inputs(self, use_pr_as_commit: bool = True) -> Inputs: dry_run=False, normalise_commit=True, gerrit_server="gerrit.example.org", - gerrit_server_port="29418", + gerrit_server_port=29418, gerrit_project="test/project", issue_id="", issue_id_lookup_json="", diff --git a/tests/test_reconciliation_extracted_module.py b/tests/test_reconciliation_extracted_module.py index 6207496..3637a9a 100644 --- a/tests/test_reconciliation_extracted_module.py +++ b/tests/test_reconciliation_extracted_module.py @@ -14,10 +14,14 @@ import logging from types import SimpleNamespace +from typing import cast import pytest from github2gerrit.gerrit_query import GerritChange +from github2gerrit.gitreview import GerritInfo +from github2gerrit.models import GitHubContext +from github2gerrit.models import Inputs from github2gerrit.orchestrator import perform_reconciliation from github2gerrit.orchestrator import reconciliation as recon_mod from github2gerrit.reconcile_matcher import LocalCommit @@ -56,16 +60,24 @@ def _gerrit_change( ) -class _DummyGH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 5 +def _gh_context() -> GitHubContext: + return GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="1", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=5, + ) -class _DummyGerrit: - host = "gerrit.example.org" - port = 29418 +def _gerrit_info() -> GerritInfo: + return GerritInfo(host="gerrit.example.org", port=29418) def _inputs( @@ -74,12 +86,18 @@ def _inputs( allow_orphan_changes: bool = False, similarity_subject: float = 0.7, log_reconcile_json: bool = False, -): - return SimpleNamespace( - reuse_strategy=reuse_strategy, - allow_orphan_changes=allow_orphan_changes, - similarity_subject=similarity_subject, - log_reconcile_json=log_reconcile_json, +) -> Inputs: + # ``Inputs`` carries ~30 required fields; this stand-in supplies only + # the reconciliation switches that ``perform_reconciliation`` reads, + # so the cast records a deliberately partial implementation. + return cast( + Inputs, + SimpleNamespace( + reuse_strategy=reuse_strategy, + allow_orphan_changes=allow_orphan_changes, + similarity_subject=similarity_subject, + log_reconcile_json=log_reconcile_json, + ), ) @@ -89,8 +107,8 @@ def _inputs( def test_reconciliation_strategy_none_returns_empty(): - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="none") local_commits = [_local_commit(0, "a1", "feat: one")] result = perform_reconciliation( @@ -103,8 +121,8 @@ def test_reconciliation_strategy_none_returns_empty(): def test_reconciliation_topic_reuse_path(monkeypatch: pytest.MonkeyPatch): - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic") pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "abc123hash" @@ -137,8 +155,8 @@ def test_reconciliation_queries_canonical_topic( monkeypatch: pytest.MonkeyPatch, ): """Topic discovery must query the topic format the push side uses.""" - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic") monkeypatch.delenv("G2G_TOPIC_PREFIX", raising=False) @@ -183,8 +201,8 @@ def test_reconciliation_comment_fallback_extension( When topic path yields no changes and comment mapping exists with fewer entries than local commits, new IDs must extend mapping. """ - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic+comment") # Empty topic result @@ -227,8 +245,8 @@ def test_reconciliation_json_summary_emitted( Enabling log_reconcile_json should emit a RECONCILE_SUMMARY log line. """ caplog.set_level(logging.DEBUG) - gh = _DummyGH() - gerrit = _DummyGerrit() + gh = _gh_context() + gerrit = _gerrit_info() inputs = _inputs(reuse_strategy="topic", log_reconcile_json=True) pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "def456hash" diff --git a/tests/test_reconciliation_plan_and_orphans.py b/tests/test_reconciliation_plan_and_orphans.py index 6fea6db..6fcf770 100644 --- a/tests/test_reconciliation_plan_and_orphans.py +++ b/tests/test_reconciliation_plan_and_orphans.py @@ -18,11 +18,16 @@ import json import re from types import SimpleNamespace +from typing import Any +from typing import cast # (removed unused List import) import pytest from github2gerrit.gerrit_query import GerritChange +from github2gerrit.gitreview import GerritInfo +from github2gerrit.models import GitHubContext +from github2gerrit.models import Inputs from github2gerrit.orchestrator import reconciliation as recon_mod from github2gerrit.orchestrator.reconciliation import perform_reconciliation from github2gerrit.reconcile_matcher import LocalCommit @@ -33,17 +38,24 @@ # --------------------------------------------------------------------------- -class _GH: - server_url = "https://github.com" - repository = "org/repo" - repository_owner = "org" - pr_number = 42 - run_id = "99999" +def _gh_context() -> GitHubContext: + return GitHubContext( + event_name="pull_request_target", + event_action="opened", + event_path=None, + repository="org/repo", + repository_owner="org", + server_url="https://github.com", + run_id="99999", + sha="deadbeef", + base_ref="master", + head_ref="feature", + pr_number=42, + ) -class _Gerrit: - host = "gerrit.example.org" - port = 29418 +def _gerrit_info() -> GerritInfo: + return GerritInfo(host="gerrit.example.org", port=29418) def _lc(idx: int, sha: str, subject: str, files: list[str]) -> LocalCommit: @@ -80,16 +92,22 @@ def _inputs( log_reconcile_json: bool = True, orphan_policy: str = "comment", similarity_subject: float = 0.7, -): - return SimpleNamespace( - reuse_strategy=reuse_strategy, - log_reconcile_json=log_reconcile_json, - orphan_policy=orphan_policy, - similarity_subject=similarity_subject, +) -> Inputs: + # ``Inputs`` carries ~30 required fields; this stand-in supplies only + # the reconciliation switches that ``perform_reconciliation`` reads, + # so the cast records a deliberately partial implementation. + return cast( + Inputs, + SimpleNamespace( + reuse_strategy=reuse_strategy, + log_reconcile_json=log_reconcile_json, + orphan_policy=orphan_policy, + similarity_subject=similarity_subject, + ), ) -def _extract_summary(caplog) -> dict: +def _extract_summary(caplog) -> dict[str, Any]: """ Find and parse the most recent RECONCILE_SUMMARY json line. """ @@ -104,7 +122,7 @@ def _extract_summary(caplog) -> dict: return json.loads(m.group(1)) -def _extract_orphan_actions(caplog) -> dict | None: +def _extract_orphan_actions(caplog) -> dict[str, Any] | None: lines = [r.message for r in caplog.records if "ORPHAN_ACTIONS" in r.message] if not lines: return None @@ -127,8 +145,8 @@ def test_topic_reuse_and_orphan_policy_comment( Orphan policy 'comment' should log commented array with orphan id. """ caplog.set_level("DEBUG") - gh = _GH() - gerrit = _Gerrit() + gh = _gh_context() + gerrit = _gerrit_info() pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "1234abcdhash" # Local commits -> only first Gerrit change reused. @@ -205,8 +223,8 @@ def test_orphan_policy_variants(policy, expected_field, caplog, monkeypatch): Validate that the orphan action bucket changes with policy. """ caplog.set_level("DEBUG") - gh = _GH() - # (removed unused variable; _Gerrit() passed inline) + gh = _gh_context() + # (removed unused variable; _gerrit_info() passed inline) pr_url = f"{gh.server_url}/{gh.repository}/pull/{gh.pr_number}" gh_hash = "abcd5678hash" local_commits = [_lc(0, "sha-x", "feat: one", ["x.py"])] @@ -250,7 +268,7 @@ def mock_build_client_for_host(host, **kwargs): _ = perform_reconciliation( inputs=inputs, gh=gh, - gerrit=_Gerrit(), + gerrit=_gerrit_info(), local_commits=local_commits, expected_github_hash=gh_hash, ) @@ -269,8 +287,8 @@ def test_comment_based_reuse_extension_digest(caplog, monkeypatch): appended with new Change-Id; digest reflects both. """ caplog.set_level("DEBUG") - gh = _GH() - gerrit = _Gerrit() + gh = _gh_context() + gerrit = _gerrit_info() existing_ids = ["I1234567890abcdef1234567890abcdef12345678"] # Force empty topic results. monkeypatch.setattr(recon_mod, "query_changes_by_topic", lambda *a, **k: []) diff --git a/tests/test_ssh_artifact_prevention.py b/tests/test_ssh_artifact_prevention.py index 46d2aa8..c7eb952 100644 --- a/tests/test_ssh_artifact_prevention.py +++ b/tests/test_ssh_artifact_prevention.py @@ -151,6 +151,13 @@ def test_ssh_files_use_secure_location( orch1._setup_ssh(mock_inputs, mock_gerrit_info) orch2._setup_ssh(mock_inputs, mock_gerrit_info) + assert orch1._ssh_temp_dir is not None, ( + "_setup_ssh should have created an SSH temp dir on orch1" + ) + assert orch2._ssh_temp_dir is not None, ( + "_setup_ssh should have created an SSH temp dir on orch2" + ) + # Verify different secure paths are used assert orch1._ssh_temp_dir != orch2._ssh_temp_dir assert "g2g_ssh_" in str(orch1._ssh_temp_dir) @@ -176,6 +183,9 @@ def test_ssh_cleanup_removes_temp_directory( orch._setup_ssh(mock_inputs, mock_gerrit_info) ssh_temp_dir = orch._ssh_temp_dir + assert ssh_temp_dir is not None, ( + "_setup_ssh should have created an SSH temp dir to clean up" + ) assert ssh_temp_dir.exists() # Cleanup SSH From 13394e3d7ecf28dea936c858df8c10d14267a0a5 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Wed, 26 Aug 2026 08:20:01 +0100 Subject: [PATCH 2/4] CI: Extend static analysis to tests and scripts basedpyright and ruff covered src/ alone, so tests/, scripts/ and sitecustomize.py accumulated errors that no gate would catch. An editor language server reads [tool.pyright] directly and checks whatever file is open, so those errors surfaced during development while CI stayed green. The include list and the hook's files pattern now name every Python path in the repository, and each cross-references the other so the two stay together. Tests keep the annotation latitude they already had under mypy, through an executionEnvironment that disables reportMissingParameterType. Annotating several hundred monkeypatch and tmp_path parameters would add no checking beyond what pytest's name-based fixture resolution performs at collection time. Correctness rules still apply to tests in full. reportUntypedFunctionDecorator is off for tests as well, because it fires on every @pytest.fixture whenever pytest is not importable, and the hook runs basedpyright without a project virtualenv. The repository already tolerates that condition through reportMissingImports. The dev extra now pins click>=8.2. Several tests read CliRunner's result.stderr, which click captures separately only from 8.2 onward and otherwise raises ValueError, so this records a requirement the suite already had. The runtime floor stays at 8.1.7, which remains accurate for the package itself. Finally, the dev extra is referenced from the default dependency group. uv installs that group for a bare `uv sync`, so `uv run pytest` picks up the project's pinned pytest; previously the extra went uninstalled and the call fell through to whichever pytest sat on PATH. The extra stays an extra because CI installs it by name and cannot see dependency groups. Co-authored-by: Claude Signed-off-by: Matthew Watkins --- .pre-commit-config.yaml | 8 +++++--- pyproject.toml | 33 ++++++++++++++++++++++++++++++++- uv.lock | 4 ++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d4e3e8c..7d6f265 100644 --- a/.pre-commit-config.yaml +++ b/.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 diff --git a/pyproject.toml b/pyproject.toml index df74537..dc85d59 100644 --- a/pyproject.toml +++ b/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]", ] diff --git a/uv.lock b/uv.lock index ec2b57c..3f18d56 100644 --- a/uv.lock +++ b/uv.lock @@ -395,6 +395,7 @@ dependencies = [ [package.optional-dependencies] dev = [ + { name = "click" }, { name = "coverage", extra = ["toml"] }, { name = "mypy" }, { name = "pytest" }, @@ -408,6 +409,7 @@ dev = [ [package.dev-dependencies] dev = [ + { name = "github2gerrit", extra = ["dev"] }, { name = "hatch-vcs" }, { name = "hatchling" }, ] @@ -415,6 +417,7 @@ dev = [ [package.metadata] requires-dist = [ { name = "click", specifier = ">=8.1.7" }, + { name = "click", marker = "extra == 'dev'", specifier = ">=8.2" }, { name = "coverage", extras = ["toml"], marker = "extra == 'dev'", specifier = ">=7.10.6" }, { name = "cryptography", specifier = ">=48.0.1" }, { name = "git-review", specifier = ">=2.5.0" }, @@ -438,6 +441,7 @@ provides-extras = ["dev"] [package.metadata.requires-dev] dev = [ + { name = "github2gerrit", extras = ["dev"] }, { name = "hatch-vcs", specifier = ">=0.5.0" }, { name = "hatchling", specifier = ">=1.28.0" }, ] From 8da2264c26224678cd1af3adabb7978941bdd211 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Wed, 26 Aug 2026 09:09:24 +0100 Subject: [PATCH 3/4] Fix: Reset process-wide caches between tests Completes issue #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 Signed-off-by: Matthew Watkins --- tests/conftest.py | 39 +++++ tests/test_cache_isolation.py | 250 +++++++++++++++++++++++++++++++++ tests/test_gerrit_urls_more.py | 21 +-- 3 files changed, 296 insertions(+), 14 deletions(-) create mode 100644 tests/test_cache_isolation.py diff --git a/tests/conftest.py b/tests/conftest.py index efdc1a4..943568a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -231,6 +231,45 @@ 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 isolate_git_environment(monkeypatch: pytest.MonkeyPatch) -> None: """ diff --git a/tests/test_cache_isolation.py b/tests/test_cache_isolation.py new file mode 100644 index 0000000..2900108 --- /dev/null +++ b/tests/test_cache_isolation.py @@ -0,0 +1,250 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2025 The Linux Foundation + +"""Regression tests for process-wide cache isolation between tests. + +Several modules memoise expensive lookups in module-level state that +outlives an individual test: ``gerrit_urls._BASE_PATH_CACHE`` and the +four caches in ``ssh_config_parser``. ``tests/conftest.py`` clears all +of them around every test via the ``reset_gerrit_base_path_cache`` and +``reset_credential_caches`` autouse fixtures. + +Each pair below is a polluter followed by a victim. The polluter +populates a cache with a value the victim must not see; the victim then +asserts the answer it would get from a clean process. Tests within a +module run in definition order, so the pairing is deterministic. Remove +either autouse fixture and the corresponding victim fails. +""" + +from __future__ import annotations + +from collections.abc import Callable +from pathlib import Path +from typing import Any + +import pytest + +from github2gerrit import gerrit_urls as urls_mod +from github2gerrit import ssh_config_parser as ssh_mod +from github2gerrit.gerrit_urls import create_gerrit_url_builder +from github2gerrit.ssh_config_parser import SSHConfig + + +# The fixture host shared by roughly twenty test modules, and therefore +# the one most likely to carry a leaked base path. +_SHARED_HOST = "gerrit.example.org" + + +class _FakeResp: + def __init__(self, code: int, headers: dict[str, str]) -> None: + self.status = code + self.headers = headers + + def getcode(self) -> int: + return self.status + + +class _FakeOpener: + def __init__( + self, decide: Callable[[str], _FakeResp | BaseException] + ) -> None: + self._decide = decide + self.addheaders: list[tuple[str, str]] = [] + + def open(self, url: str, timeout: float | None = None) -> _FakeResp: + result = self._decide(url) + if isinstance(result, BaseException): + raise result + return result + + +def _install_opener( + monkeypatch: pytest.MonkeyPatch, + decide: Callable[[str], _FakeResp | BaseException], +) -> None: + opener = _FakeOpener(decide) + + def _build(*_args: Any, **_kwargs: Any) -> _FakeOpener: + return opener + + monkeypatch.setattr( + "github2gerrit.gerrit_urls.urllib.request.build_opener", + _build, + raising=True, + ) + + +def test_base_path_cache_polluter_records_r_for_shared_host( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Populate the base-path cache with 'r' for the shared host. + + This mirrors what the base-path discovery tests do: a 302 whose + Location carries a ``/r/`` prefix teaches the module that the host + serves Gerrit under that base path, for the rest of the process. + """ + monkeypatch.delenv("GERRIT_HTTP_BASE_PATH", raising=False) + + def decide(url: str) -> _FakeResp | BaseException: + if url.endswith("/dashboard/self"): + return _FakeResp(302, {"Location": "/r/dashboard/self"}) + return _FakeResp(404, {}) + + _install_opener(monkeypatch, decide) + + builder = create_gerrit_url_builder(_SHARED_HOST) + + assert builder.base_path == "r" + assert urls_mod._BASE_PATH_CACHE[_SHARED_HOST] == "r" + + +def test_base_path_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A later, independent build for the same host sees no base path. + + Discovery here cannot reach the server, so the honest answer is an + empty base path. Without the autouse reset the builder short + circuits on the leaked ``'r'`` from the previous test and never + probes at all, yielding ``/r/`` URLs. + """ + monkeypatch.delenv("GERRIT_HTTP_BASE_PATH", raising=False) + + probed: list[str] = [] + + def decide(url: str) -> _FakeResp | BaseException: + probed.append(url) + return OSError("unreachable") + + _install_opener(monkeypatch, decide) + + builder = create_gerrit_url_builder(_SHARED_HOST) + + assert probed, "discovery short-circuited on a leaked cache entry" + assert builder.base_path == "" + assert ( + builder.change_url("releng/builder", 12345) + == "https://gerrit.example.org/c/releng/builder/+/12345" + ) + + +def test_credential_cache_polluter_records_ssh_derived_identity( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Populate the credential caches from a user-SSH-respecting run.""" + monkeypatch.setenv("G2G_RESPECT_USER_SSH", "true") + monkeypatch.setattr( + ssh_mod, + "get_ssh_user_for_gerrit", + lambda _host, _port=29418: "sshuser", + raising=True, + ) + monkeypatch.setattr( + ssh_mod, + "get_git_user_email", + lambda: "dev@example.org", + raising=True, + ) + + user, email = ssh_mod.derive_gerrit_credentials(_SHARED_HOST, "leakorg") + + assert user == "sshuser" + assert email == "dev@example.org" + assert ssh_mod._get_respect_user_ssh_setting() is True + + +def test_respect_user_ssh_setting_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The env-var cache reflects this test's environment, not the last. + + ``_get_respect_user_ssh_setting`` takes no arguments, so its + ``lru_cache(maxsize=1)`` has exactly one slot that every test in the + suite collides on, over an environment variable tests monkeypatch + freely. + """ + monkeypatch.delenv("G2G_RESPECT_USER_SSH", raising=False) + + assert ssh_mod._get_respect_user_ssh_setting() is False + + +def test_git_user_email_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The git-email cache reflects this test's git identity. + + ``_get_cached_git_user_email`` is the same shape as + ``_get_respect_user_ssh_setting``: zero arguments, one cache slot, + shared by the whole suite. What it memoises is the output of + ``git config user.email``, which ``isolate_git_environment`` and + individual tests both take pains to control. + """ + monkeypatch.setattr( + ssh_mod, + "get_git_user_email", + lambda: "victim@example.org", + raising=True, + ) + + assert ssh_mod._get_cached_git_user_email() == "victim@example.org" + + +def test_credential_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The same host and organisation derive fallback credentials. + + ``derive_gerrit_credentials`` is keyed only on host, organisation + and port. The polluter above used the same key, so without the + reset its answer is replayed here even though it was computed from + an environment and an SSH config this test does not share. + """ + monkeypatch.delenv("G2G_RESPECT_USER_SSH", raising=False) + + user, email = ssh_mod.derive_gerrit_credentials(_SHARED_HOST, "leakorg") + + assert user == "leakorg.gh2gerrit" + assert email == "releng+leakorg-gh2gerrit@linuxfoundation.org" + + +def _install_ssh_config( + monkeypatch: pytest.MonkeyPatch, config_path: Path, user: str +) -> None: + """Point ``SSHConfig()`` at a config file naming ``user``. + + ``get_ssh_user_for_gerrit`` keys its cache on ``~/.ssh/config`` as a + path string, so the key is the same for both tests below regardless + of where the file they parse actually lives. + """ + config_path.write_text( + f"Host {_SHARED_HOST}\n User {user}\n", encoding="utf-8" + ) + + def _factory() -> SSHConfig: + return SSHConfig(config_path=config_path) + + monkeypatch.setattr(ssh_mod, "SSHConfig", _factory, raising=True) + + +def test_ssh_config_cache_polluter_parses_a_config_file( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Parse and cache an SSH config naming a distinctive user.""" + _install_ssh_config(monkeypatch, tmp_path / "config", "polluter") + + assert ssh_mod.get_ssh_user_for_gerrit(_SHARED_HOST) == "polluter" + assert ssh_mod._ssh_config_cache + + +def test_ssh_config_cache_does_not_leak_into_later_test( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A later test parses its own config rather than the cached one. + + The cache is keyed on the config path and never on that file's + contents, so without the reset the previous test's parsed instance + is returned and this test never sees its own config at all. + """ + _install_ssh_config(monkeypatch, tmp_path / "config", "victim") + + assert ssh_mod.get_ssh_user_for_gerrit(_SHARED_HOST) == "victim" diff --git a/tests/test_gerrit_urls_more.py b/tests/test_gerrit_urls_more.py index 47a30ce..26c25aa 100644 --- a/tests/test_gerrit_urls_more.py +++ b/tests/test_gerrit_urls_more.py @@ -12,17 +12,6 @@ from github2gerrit.gerrit_urls import create_gerrit_url_builder -@pytest.fixture(autouse=True) -def clear_cache_between_tests() -> None: - """Clear the base path cache between tests to prevent pollution.""" - _clear_builder_cache() - - -def _clear_builder_cache() -> None: - # Ensure base-path discovery cache does not bleed across tests - urls_mod._BASE_PATH_CACHE.clear() # pyright: ignore[reportPrivateUsage] - - class _FakeResp: def __init__( self, code: int, headers: dict[str, str] | None = None @@ -206,9 +195,13 @@ def decide(url: str) -> _FakeResp | BaseException: assert b1.base_path == "r" assert b1.web_url("dashboard").startswith("https://gerrit.example.org/r/") - # Now simulate absolute URL in Location header - # Cache is cleared automatically by the autouse fixture between test sections - # when we create a new builder + # Now simulate absolute URL in Location header. This clear is + # deliberate and mid-test: the first half cached 'r' for this host, + # and without discarding it the second builder would short-circuit + # on the cache and never reach decide_abs, leaving the + # absolute-Location branch untested and the assertions below + # vacuously true. The autouse reset in conftest.py runs around the + # test, not inside it, so it cannot do this. urls_mod._BASE_PATH_CACHE.clear() # pyright: ignore[reportPrivateUsage] def decide_abs(url: str) -> _FakeResp | BaseException: From 972613c10763716c19dc66a971b133b70c8c846a Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Wed, 26 Aug 2026 09:19:38 +0100 Subject: [PATCH 4/4] Fix: Separate derived Gerrit config from explicit Closes issue #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 Signed-off-by: Matthew Watkins --- src/github2gerrit/cli.py | 12 +- src/github2gerrit/config.py | 101 +++++ src/github2gerrit/duplicate_detection.py | 22 +- tests/conftest.py | 29 ++ tests/test_config_helpers.py | 262 +++++++++++++ tests/test_duplicate_detection.py | 455 +++++++++++++++++++++++ 6 files changed, 874 insertions(+), 7 deletions(-) diff --git a/src/github2gerrit/cli.py b/src/github2gerrit/cli.py index f8463c6..5648bc5 100644 --- a/src/github2gerrit/cli.py +++ b/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( diff --git a/src/github2gerrit/config.py b/src/github2gerrit/config.py index 727cde9..d40f569 100644 --- a/src/github2gerrit/config.py +++ b/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 diff --git a/src/github2gerrit/duplicate_detection.py b/src/github2gerrit/duplicate_detection.py index 40b932a..860fa85 100644 --- a/src/github2gerrit/duplicate_detection.py +++ b/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. diff --git a/tests/conftest.py b/tests/conftest.py index 943568a..5699dc3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -270,6 +270,35 @@ def reset_credential_caches() -> Iterable[None]: 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: """ diff --git a/tests/test_config_helpers.py b/tests/test_config_helpers.py index af5f67e..e243876 100644 --- a/tests/test_config_helpers.py +++ b/tests/test_config_helpers.py @@ -22,7 +22,9 @@ from github2gerrit.config import apply_parameter_derivation from github2gerrit.config import derive_gerrit_parameters from github2gerrit.config import filter_known +from github2gerrit.config import is_derived_key from github2gerrit.config import load_org_config +from github2gerrit.config import mark_derived_keys from github2gerrit.config import overlay_missing @@ -874,3 +876,263 @@ def test_sanitize_ssh_key_content_strips_embedded_whitespace() -> None: assert parts[1] == "b3BlbnNzaC1rZXktdjE" assert parts[2] == "AAAABBBBCCCC" assert parts[3] == "-----END OPENSSH PRIVATE KEY-----" + + +def test_mark_derived_keys_round_trip() -> None: + """Marked keys report as derived; unmarked ones do not.""" + assert not is_derived_key("GERRIT_PROJECT") + + mark_derived_keys(["GERRIT_PROJECT"]) + + assert is_derived_key("GERRIT_PROJECT") + # Key names are normalised, so callers need not match case. + assert is_derived_key("gerrit_project") + assert is_derived_key(" GERRIT_PROJECT ") + # An unrelated key is unaffected. + assert not is_derived_key("GERRIT_SERVER") + + +def test_mark_derived_keys_is_additive() -> None: + """A second call must not discard the first call's record.""" + mark_derived_keys(["GERRIT_SERVER"]) + mark_derived_keys(["GERRIT_PROJECT"]) + + assert is_derived_key("GERRIT_SERVER") + assert is_derived_key("GERRIT_PROJECT") + + # Empty and whitespace-only names are ignored rather than recorded. + mark_derived_keys(["", " "]) + assert is_derived_key("GERRIT_SERVER") + assert not is_derived_key("") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_marks_only_unset_env_keys( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Only keys that apply_config_to_env actually writes are marked. + + apply_config_to_env skips keys whose environment value is already + non-empty, so such a key keeps its explicit value and must not be + described as derived. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + # Setting these through monkeypatch registers them for teardown even + # though apply_config_to_env writes os.environ directly. + monkeypatch.setenv("GERRIT_SERVER", "explicit.gerrit.example.org") + monkeypatch.setenv("GERRIT_PROJECT", "") + monkeypatch.setenv("GERRIT_SSH_USER_G2G", "") + monkeypatch.setenv("GERRIT_SSH_USER_G2G_EMAIL", "") + + cfg = apply_parameter_derivation( + {}, "onap", repository="onap/integration-distribution" + ) + apply_config_to_env(cfg) + + # Derivation supplied a host, but the environment already held one, + # so the explicit value survives and provenance stays explicit. + assert os.environ["GERRIT_SERVER"] == "explicit.gerrit.example.org" + assert not is_derived_key("GERRIT_SERVER") + + # The project had no explicit value, so the guess landed and is + # recorded as derived. + assert os.environ["GERRIT_PROJECT"] == "integration-distribution" + assert is_derived_key("GERRIT_PROJECT") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_can_skip_marking( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """mark_derived=False records nothing. + + Used by call sites that only persist derived values to the + configuration file and never export them to the environment. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_SERVER", "") + monkeypatch.setenv("GERRIT_PROJECT", "") + + result = apply_parameter_derivation( + {}, + "onap", + repository="onap/sandbox", + save_to_config=False, + mark_derived=False, + ) + + # Derivation still happened; only the provenance record is skipped. + assert result["GERRIT_PROJECT"] == "sandbox" + assert not is_derived_key("GERRIT_PROJECT") + assert not is_derived_key("GERRIT_SERVER") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_marks_config_file_project( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A GERRIT_PROJECT already in cfg is a fallback, not intent. + + Releases that auto-saved derived values wrote them to the + configuration file, whose sections are per-organization while a + project name is per-repository. On a later run load_org_config + hands that name back in cfg, so it is never missing, never enters + newly_derived, and nothing above records it -- leaving an old + repository-name guess indistinguishable from operator intent. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_PROJECT", "") + + result = apply_parameter_derivation( + {"GERRIT_PROJECT": "stale-org-project"}, + "onap", + repository="onap/integration-distribution", + save_to_config=False, + ) + + assert is_derived_key("GERRIT_PROJECT") + + # Demoted, not discarded: a misscoped project still scopes the + # Gerrit query, which beats searching every project on the server + # whenever no .gitreview resolves. + assert result["GERRIT_PROJECT"] == "stale-org-project" + apply_config_to_env(result) + assert os.environ["GERRIT_PROJECT"] == "stale-org-project" + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_keeps_explicit_env_project( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An environment GERRIT_PROJECT keeps its explicit provenance. + + Unlike the per-organization file, the environment is scoped to this + run, so an operator naming a project there means it for this + repository. apply_config_to_env leaves such a value untouched, and + the provenance record must agree with what it actually wrote. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_PROJECT", "operator/project") + + result = apply_parameter_derivation( + {"GERRIT_PROJECT": "stale-org-project"}, + "onap", + repository="onap/integration-distribution", + save_to_config=False, + ) + apply_config_to_env(result) + + assert os.environ["GERRIT_PROJECT"] == "operator/project" + assert not is_derived_key("GERRIT_PROJECT") + + +@patch("github2gerrit.config._read_gitreview_host") +@patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") +def test_apply_parameter_derivation_config_project_opt_out( + mock_derive_creds, + mock_gitreview_host, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """mark_derived=False covers the configuration-file project too. + + The opt-out serves call sites that never follow up with + apply_config_to_env, so no key they handle may be described as + holding a derived value there -- whatever its source. + """ + mock_derive_creds.return_value = (None, None) + mock_gitreview_host.return_value = None + + monkeypatch.setenv("GERRIT_PROJECT", "") + + result = apply_parameter_derivation( + {"GERRIT_PROJECT": "stale-org-project"}, + "onap", + repository="onap/integration-distribution", + save_to_config=False, + mark_derived=False, + ) + + assert result["GERRIT_PROJECT"] == "stale-org-project" + assert not is_derived_key("GERRIT_PROJECT") + + +def test_save_derived_parameters_omits_per_repository_project( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """GERRIT_PROJECT never reaches the per-organization section. + + The section is shared by every repository in the organization, so + persisting one repository's project name would misroute all the + others. It would also return on the next run indistinguishable + from operator intent, defeating the provenance tracking that lets + duplicate detection prefer a per-pull-request .gitreview. + """ + from github2gerrit.config import save_derived_parameters_to_config + + config_file = tmp_path / "config.txt" + config_file.write_text( + '[default]\nGERRIT_HTTP_USER = "existing_user"\n', encoding="utf-8" + ) + + monkeypatch.setenv("G2G_CONFIG_PATH", str(config_file)) + monkeypatch.delenv("DRY_RUN", raising=False) + + save_derived_parameters_to_config( + "onap", + { + "GERRIT_SERVER": "gerrit.onap.org", + "GERRIT_PROJECT": "integration/distribution", + "GERRIT_SSH_USER_G2G": "onap.gh2gerrit", + }, + ) + + updated_content = config_file.read_text(encoding="utf-8") + assert 'GERRIT_SERVER = "gerrit.onap.org"' in updated_content + assert 'GERRIT_SSH_USER_G2G = "onap.gh2gerrit"' in updated_content + assert "GERRIT_PROJECT" not in updated_content + assert "integration/distribution" not in updated_content + + +def test_save_derived_parameters_project_only_writes_nothing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Filtering out the only parameter must leave the file untouched. + + The file has to exist for this to prove anything: the function + already returns early when it does not, so a missing file would + make the assertion pass for the wrong reason. + """ + from github2gerrit.config import save_derived_parameters_to_config + + config_file = tmp_path / "config.txt" + original_content = '[default]\nGERRIT_HTTP_USER = "existing_user"\n' + config_file.write_text(original_content, encoding="utf-8") + + monkeypatch.setenv("G2G_CONFIG_PATH", str(config_file)) + monkeypatch.delenv("DRY_RUN", raising=False) + + save_derived_parameters_to_config( + "onap", {"GERRIT_PROJECT": "integration/distribution"} + ) + + assert config_file.read_text(encoding="utf-8") == original_content diff --git a/tests/test_duplicate_detection.py b/tests/test_duplicate_detection.py index d0a6190..8de1e2d 100644 --- a/tests/test_duplicate_detection.py +++ b/tests/test_duplicate_detection.py @@ -3,6 +3,8 @@ """Tests for duplicate change detection.""" +import os +import re from datetime import UTC from datetime import datetime from datetime import timedelta @@ -10,10 +12,16 @@ from typing import Any from unittest.mock import Mock from unittest.mock import patch +from urllib.error import URLError +from urllib.parse import unquote +from urllib.parse import urlsplit import pytest import responses +from github2gerrit.config import apply_config_to_env +from github2gerrit.config import apply_parameter_derivation +from github2gerrit.config import mark_derived_keys from github2gerrit.duplicate_detection import ChangeFingerprint from github2gerrit.duplicate_detection import DuplicateChangeError from github2gerrit.duplicate_detection import DuplicateDetector @@ -21,6 +29,68 @@ from github2gerrit.models import GitHubContext +def _gitreview_urlopen(content: bytes | None) -> Any: + """Build a ``urllib.request.urlopen`` side effect for tests. + + Serves *content* only for an ``https://raw.githubusercontent.com`` + origin and refuses everything else, so an unexpected network call + fails loudly rather than resolving through the real network. The + origin is compared after parsing rather than by substring, since a + substring test would also accept + ``https://raw.githubusercontent.com.example.invalid/...`` or a URL + carrying the name only in its path or query string. Pass ``None`` + to make the ``.gitreview`` fetch itself unresolvable. + """ + + def _open(url: Any, *_args: Any, **_kwargs: Any) -> Any: + target = url if isinstance(url, str) else getattr(url, "full_url", "") + parsed = urlsplit(target) + from_raw_github = ( + parsed.scheme == "https" + and parsed.netloc == "raw.githubusercontent.com" + ) + if content is not None and from_raw_github: + response = Mock() + response.read.return_value = content + response.__enter__ = Mock(return_value=response) + response.__exit__ = Mock(return_value=None) + return response + raise URLError(f"blocked in test: {target}") + + return _open + + +@pytest.mark.parametrize( + "url", + [ + "https://raw.githubusercontent.com.example.invalid/o/r/b/.gitreview", + "https://example.invalid/?raw.githubusercontent.com", + "https://example.invalid/raw.githubusercontent.com/.gitreview", + "http://raw.githubusercontent.com/o/r/b/.gitreview", + ], +) +def test_gitreview_urlopen_rejects_lookalike_origins(url: str) -> None: + """The test guard matches the origin, not merely a substring. + + A substring check would serve mocked content to a lookalike host, a + path segment or a query string, so a malformed or redirected fetch + URL could pass these tests silently. + """ + opener = _gitreview_urlopen(b"[gerrit]\nhost=gerrit.example.org\n") + + with pytest.raises(URLError): + opener(url) + + +def test_gitreview_urlopen_accepts_expected_origin() -> None: + """The guard still serves the origin the fetcher actually uses.""" + opener = _gitreview_urlopen(b"[gerrit]\nhost=gerrit.example.org\n") + + response = opener("https://raw.githubusercontent.com/o/r/main/.gitreview") + + assert b"gerrit.example.org" in response.read() + + class TestChangeFingerprint: """Test ChangeFingerprint functionality.""" @@ -381,6 +451,391 @@ def test_resolve_gerrit_info_from_remote_gitreview( result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) assert result == ("gerrit.example.org", "test/project") + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_explicit_env_skips_gitreview( + self, mock_urlopen: Any + ) -> None: + """Explicit configuration short-circuits before any fetch. + + Values the operator set deliberately outrank the pull request's + ``.gitreview``, and resolving them must cost no network I/O. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.wrong.org\nproject=wrong/project\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.example.org", + "GERRIT_PROJECT": "test/project", + }, + ): + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.example.org", "test/project") + mock_urlopen.assert_not_called() + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_gitreview_outranks_derived_env( + self, mock_urlopen: Any + ) -> None: + """A derived environment pair loses to a resolvable .gitreview. + + Derivation guesses the project from the GitHub repository name, + so the per-pull-request ``.gitreview`` is the better answer. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.real.org\nport=29418\n" + b"project=real/project.git\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.real.org", "real/project") + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_keeps_explicit_host_takes_project( + self, mock_urlopen: Any + ) -> None: + """A derived project must not drag an explicit host down with it. + + Provenance is per field. Treating the pair as a unit made one + derived key demote both, so ``.gitreview`` replaced a host the + operator had configured deliberately. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + b"project=gitreview/project.git\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.explicit.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.explicit.org", "gitreview/project") + + @patch("github2gerrit.config._read_gitreview_host") + @patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") + @patch("urllib.request.urlopen") + def test_legacy_config_file_project_loses_to_gitreview( + self, + mock_urlopen: Any, + mock_derive_creds: Any, + mock_config_gitreview_host: Any, + ) -> None: + """The same outcome, reached through the configuration file. + + The case above marks the project derived directly. This one + earns the mark the way a real local CLI run does: releases that + auto-saved derived values put GERRIT_PROJECT in the + per-organization section, so load_org_config returns an old + repository-name guess, derivation finds the key already filled, + and without provenance duplicate detection would short-circuit + on it and query the wrong project. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context( + repository="onap/integration-distribution" + ) + + # Keep derivation off the SSH config and off the network. + mock_derive_creds.return_value = (None, None) + mock_config_gitreview_host.return_value = None + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + b"project=integration/distribution.git\n" + ) + + with patch.dict( + "os.environ", + { + # Set by the operator for this run. + "GERRIT_SERVER": "gerrit.explicit.org", + # Unset, as on a local CLI run that leans on the + # per-organization configuration file. + "GERRIT_PROJECT": "", + }, + ): + cfg = apply_parameter_derivation( + # As load_org_config returns it from the stored section. + {"GERRIT_PROJECT": "integration-distribution"}, + "onap", + repository=gh.repository, + save_to_config=False, + ) + apply_config_to_env(cfg) + + # The stale name is in play, alongside the explicit host. + assert os.environ["GERRIT_SERVER"] == "gerrit.explicit.org" + assert os.environ["GERRIT_PROJECT"] == "integration-distribution" + + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.explicit.org", "integration/distribution") + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_keeps_explicit_project_takes_host( + self, mock_urlopen: Any + ) -> None: + """The mirror case: a derived host must not discard an explicit + project. + + Only the field lacking operator intent may come from + ``.gitreview``. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + b"project=gitreview/project.git\n" + ) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "explicit/project", + }, + ): + mark_derived_keys(["GERRIT_SERVER"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.gitreview.org", "explicit/project") + + @patch("urllib.request.urlopen") + def test_host_only_gitreview_keeps_derived_project_in_query( + self, mock_urlopen: Any + ) -> None: + """A ``.gitreview`` without ``project=`` must not blank the + project. + + ``parse_gitreview`` needs only ``host=``, so a host-only file + parses successfully and leaves ``info.project`` empty. Taking + that empty string over the derived environment value drops the + ``project:`` qualifier from the Gerrit query, which then + searches every project on the server: an unrelated change that + happens to share a title would block a legitimate submission. + A derived project is a guess, but a scoped guess beats an + unscoped search. + """ + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.gitreview.org\nport=29418\n" + ) + + responses.start() + try: + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context( + repository="org/derived-project" + ) + + target_pr = Mock() + target_pr.number = 123 + target_pr.title = "Fix authentication" + target_pr.body = "" + target_pr.get_files.return_value = [] + + responses.add( + responses.GET, + re.compile(r"https://gerrit\.gitreview\.org/.*"), + json=[], + status=200, + ) + + with patch.dict( + "os.environ", + { + # As parameter derivation would leave them. + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + # Pin the base path so no discovery probe runs. + "GERRIT_HTTP_BASE_PATH": "r", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + + resolved = detector._resolve_gerrit_info_from_env_or_gitreview( + gh + ) + assert resolved == ( + "gerrit.gitreview.org", + "derived-project", + ) + + detector.check_for_duplicates( + target_pr, allow_duplicates=False, gh=gh + ) + + queried = [ + unquote(call.request.url or "") for call in responses.calls + ] + assert queried, "expected at least one Gerrit query" + assert all("project:derived-project " in url for url in queried), ( + f"Gerrit query was not scoped to a project: {queried}" + ) + finally: + responses.stop() + responses.reset() + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_falls_back_to_derived_env( + self, mock_urlopen: Any + ) -> None: + """An unresolvable .gitreview leaves the derived pair in play. + + Returning None instead would turn a possibly-wrong query into no + duplicate detection at all, which is strictly worse. + """ + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context() + + mock_urlopen.side_effect = _gitreview_urlopen(None) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.derived.org", "derived-project") + + @patch("urllib.request.urlopen") + def test_resolve_gerrit_info_falls_back_without_repository( + self, mock_urlopen: Any + ) -> None: + """With no repository to fetch from, the derived pair still wins + over returning nothing.""" + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context(repository="") + + mock_urlopen.side_effect = _gitreview_urlopen(None) + + with patch.dict( + "os.environ", + { + "GERRIT_SERVER": "gerrit.derived.org", + "GERRIT_PROJECT": "derived-project", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + result = detector._resolve_gerrit_info_from_env_or_gitreview(gh) + + assert result == ("gerrit.derived.org", "derived-project") + mock_urlopen.assert_not_called() + + @patch("urllib.request.urlopen") + def test_duplicate_query_uses_gitreview_path_not_derived_name( + self, mock_urlopen: Any + ) -> None: + """Acceptance criterion for a Gerrit project with a path separator. + + ``opendaylight/integration-distribution`` derives the project + ``integration-distribution``, but Gerrit hosts it at + ``integration/distribution``. Querying the derived name asks + about a project that does not exist, so every duplicate is + silently missed. The ``.gitreview`` must win. + """ + mock_urlopen.side_effect = _gitreview_urlopen( + b"[gerrit]\nhost=gerrit.opendaylight.org\nport=29418\n" + b"project=integration/distribution.git\n" + ) + + responses.start() + try: + detector = DuplicateDetector(Mock()) + gh = self._create_mock_github_context( + repository="opendaylight/integration-distribution" + ) + + target_pr = Mock() + target_pr.number = 123 + target_pr.title = "Fix authentication" + target_pr.body = "" + target_pr.get_files.return_value = [] + + # Answer any query, so the only thing that can distinguish + # the two projects is the query Gerrit was actually asked. + responses.add( + responses.GET, + re.compile(r"https://gerrit\.opendaylight\.org/.*"), + json=[ + { + "_number": 12345, + "subject": "Fix authentication", + "project": "integration/distribution", + "current_revision": "abc123", + "revisions": { + "abc123": { + "commit": {"message": "Fix authentication"}, + "files": {}, + } + }, + } + ], + status=200, + ) + + with patch.dict( + "os.environ", + { + # As parameter derivation would leave them. + "GERRIT_SERVER": "gerrit.opendaylight.org", + "GERRIT_PROJECT": "integration-distribution", + # Pin the base path so no discovery probe runs. + "GERRIT_HTTP_BASE_PATH": "r", + }, + ): + mark_derived_keys(["GERRIT_SERVER", "GERRIT_PROJECT"]) + + with pytest.raises(DuplicateChangeError): + detector.check_for_duplicates( + target_pr, allow_duplicates=False, gh=gh + ) + + queried = [ + unquote(call.request.url or "") for call in responses.calls + ] + assert queried, "expected at least one Gerrit query" + assert any( + "project:integration/distribution" in url for url in queried + ), f"Gerrit was not queried for the .gitreview project: {queried}" + assert not any( + "project:integration-distribution" in url for url in queried + ), f"Gerrit was queried for the derived project: {queried}" + finally: + responses.stop() + responses.reset() + def test_check_for_duplicates_with_gerrit_duplicate( self, ) -> None: