From 1c95b2f6cd9874b7446d3cf850ceec6ae65d14df Mon Sep 17 00:00:00 2001 From: Emil Simeonov Date: Sat, 19 Sep 2026 04:31:44 +0200 Subject: [PATCH] fix: create_dependency_pr clones target repo instead of editing producer checkout --- docs/specs/DEVX-166.md | 50 +++++++++++++++++-------- src/devx/ci/create_dependency_pr.py | 33 ++++++++++++---- tests/unit/test_create_dependency_pr.py | 8 ++++ 3 files changed, 68 insertions(+), 23 deletions(-) diff --git a/docs/specs/DEVX-166.md b/docs/specs/DEVX-166.md index aca4d00..68bc4e4 100644 --- a/docs/specs/DEVX-166.md +++ b/docs/specs/DEVX-166.md @@ -1,27 +1,47 @@ -# DEVX-166: Exclude docs/plans/* from PR size check +# DEVX-166: create_dependency_pr must clone the target repo ## Problem -Planning docs in `docs/plans/` are legitimately large (700+ lines) but -fail the PR size check (max 500 lines). This blocks PRs that only add -planning documents. + +`create_dependency_pr` resolves the pinned-version file and runs all +git operations in the current working directory. Producer post-merge +workflows (grm, sso-bridge) invoke it from the *producer* checkout, so +it searches/modifies the wrong repository: `find_pinned_version` reads +files that do not exist there, and the git fetch/checkout/commit/push +sequence runs in the producer clone. The failure is silent — producer +workflows append `|| echo warning`, so a no-op looks like success. ## Approach -REQ-1: Add `docs/plans/*` to `DEFAULT_EXCLUDED_PATTERNS` in - `src/devx/ci/check_pr_size.py` -REQ-2: Add test coverage for the new exclusion pattern + +REQ-1: Clone the target repo (`--repo`) into a temporary directory with +an authenticated `http.extraHeader`, then run every file lookup and git +operation (fetch, checkout, add, commit, push) inside that clone. The +push uses the same auth header config. + +REQ-2: Tests mock `subprocess.run` so no real clone happens in the unit +suite (test-isolation gate). + +## Files Affected + +- `src/devx/ci/create_dependency_pr.py` +- `tests/unit/test_create_dependency_pr.py` ## Test Plan -- `make pytest-cov` passes with 100% coverage -- `make lint-all` passes + +- Existing CLI tests keep passing with the subprocess mock in place. +- Verify the clone command targets the `--repo` URL and that git ops + run with `cwd=` (asserted via the mock's call list). ## Deploy Plan -- Merge to master → post-merge auto-publishes new devx version -- Infra PR #1179 picks up the fix once devx is bumped + +Merge via auto-merge after green CI. The fix takes effect the next time +a producer post-merge workflow invokes `create_dependency_pr`. ## Rollback Plan -- Revert the merge commit + +Revert the squash-merge commit on master; the previous (broken) CWD +behavior returns, which is strictly worse — no state is created. ## Acceptance Criteria -- [x] REQ-1: Add `docs/plans/*` to `DEFAULT_EXCLUDED_PATTERNS` in - `src/devx/ci/check_pr_size.py` -- [x] REQ-2: Add test coverage for the new exclusion pattern + +- [x] REQ-1: target repo cloned to tempdir; all file/git ops run in the clone +- [x] REQ-2: unit tests never spawn a real git subprocess diff --git a/src/devx/ci/create_dependency_pr.py b/src/devx/ci/create_dependency_pr.py index 615564b..45dbd28 100644 --- a/src/devx/ci/create_dependency_pr.py +++ b/src/devx/ci/create_dependency_pr.py @@ -21,6 +21,7 @@ from __future__ import annotations import re import subprocess # nosec B404 +import tempfile from pathlib import Path import click @@ -128,11 +129,23 @@ def cli( owner, repo_name = repo.split("/", 1) client = GiteaClient(GITEA_API_URL, token, owner, repo_name) + # Implements: REQ-1 — this tool runs from the *producer* repo's CI, so + # every file lookup and git operation must happen inside a clone of the + # target repo, not the producer checkout in CWD. + workdir = Path(tempfile.mkdtemp(prefix="dep-pr-")) + clone_url = f"{GITEA_API_URL.removesuffix('/api/v1')}/{repo}.git" + auth_cfg = f"http.extraHeader=Authorization: token {token}" + subprocess.run( # nosec B603 B607 + ["git", "-c", auth_cfg, "clone", "--depth", "50", clone_url, str(workdir)], + check=True, + capture_output=True, + ) + # Find current pinned version old_version = None changed_file = None for f in [PYPROJECT_PATH, IMAGES_YML_PATH, ROLE_DEFAULTS_PATH]: - old_version = find_pinned_version(package, f) + old_version = find_pinned_version(package, str(workdir / f)) if old_version: changed_file = f break @@ -184,17 +197,21 @@ def cli( else: raise click.ClickException(_("Failed to create branch: {error}", error=str(e))) from None - # Clone, update file, commit, push - subprocess.run(["git", "fetch", "origin", f"{branch_name}"], check=False, capture_output=True) # nosec B603 B607 - subprocess.run(["git", "checkout", branch_name], check=False, capture_output=True) # nosec B603 B607 + # Check out the API-created branch inside the target clone. + subprocess.run(["git", "fetch", "origin", f"{branch_name}"], check=False, capture_output=True, cwd=workdir) # nosec B603 B607 + subprocess.run(["git", "checkout", branch_name], check=False, capture_output=True, cwd=workdir) # nosec B603 B607 - if not changed_file or not update_pinned_version(changed_file, package, old_version, new_version): + if not changed_file or not update_pinned_version(str(workdir / changed_file), package, old_version, new_version): raise click.ClickException(_("Failed to update {file}", file=changed_file)) - subprocess.run(["git", "add", changed_file], check=True) # nosec B603 B607 + subprocess.run(["git", "add", changed_file], check=True, cwd=workdir) # nosec B603 B607 commit_msg = f"deps: bump {package} from {old_version} to {new_version}" - subprocess.run(["git", "commit", "-m", commit_msg], check=True) # nosec B603 B607 - subprocess.run(["git", "push", "origin", branch_name], check=True) # nosec B603 B607 + subprocess.run(["git", "commit", "-m", commit_msg], check=True, cwd=workdir) # nosec B603 B607 + subprocess.run( # nosec B603 B607 + ["git", "-c", auth_cfg, "push", "origin", branch_name], + check=True, + cwd=workdir, + ) # Create Vikunja task for tracking task_title = f"Bump {package} to {new_version}" diff --git a/tests/unit/test_create_dependency_pr.py b/tests/unit/test_create_dependency_pr.py index 9449160..541cc99 100644 --- a/tests/unit/test_create_dependency_pr.py +++ b/tests/unit/test_create_dependency_pr.py @@ -4,6 +4,7 @@ from pathlib import Path from unittest.mock import MagicMock, patch import click +import pytest from click.testing import CliRunner from devx.ci.create_dependency_pr import ( @@ -15,6 +16,13 @@ from devx.ci.create_dependency_pr import ( ) +@pytest.fixture(autouse=True) +def _mock_subprocess(): + """Mock subprocess so CLI tests never run a real git clone.""" + with patch("devx.ci.create_dependency_pr.subprocess.run") as m: + yield m + + class TestFindPinnedVersion: def test_finds_pip_git_pin(self, tmp_path: Path) -> None: content = "grm @ git+https://git.example.com/repo.git@v0.5.1" -- 2.54.0