Skip to content

fix: don't misdetect uv.lock changes from diff text alone - #765

Open
brobro10000 wants to merge 2 commits into
openedx:masterfrom
brobro10000:fix/uv-lock-false-positive-detection
Open

brobro10000 wants to merge 2 commits into
openedx:masterfrom
brobro10000:fix/uv-lock-false-positive-detection

Conversation

@brobro10000

Copy link
Copy Markdown
Member

Summary

compare_pr_differnce decides whether to parse a requirements-upgrade PR's diff as
a uv repo or a pip-tools repo with:

if "uv.lock" in txt:
    reqs = self._parse_uv(pull_request)
else:
    reqs = self._parse_reqs(txt)

This is a bare substring check on the PR's entire diff text, not a check for
whether uv.lock is actually one of the changed files. A plain pip-tools repo can
trip this if the diff happens to contain the literal text "uv.lock" for any other
reason — once that happens, _parse_uv correctly finds no real uv.lock file
changed and returns nothing, so both review comments post completely empty,
silently hiding every real dependency change from the PR.

Real-world impact (confirmed)

openedx/edx-enterprise vendors requirements/edx-platform-constraints.txt — a
file explicitly "copied from edx-platform. See make upgrade" per its own header.
edx-platform recently migrated to pyproject.toml + uv, which changed this
copied file's generated header to include the line:

# Source of truth: [project.dependencies] / [dependency-groups].bundled in pyproject.toml / uv.lock.

That line alone is enough to trip the substring check on every edx-enterprise
upgrade PR since, even though edx-enterprise itself has no uv.lock and is still
entirely pip-tools based. I bisected edx-enterprise's weekly bot PR history and
confirmed this has been silently broken for 6 consecutive weeks:

PR Date Review comments populated?
#2683 2026-08-21 ✅ Yes
#2687 2026-08-28 ❌ Empty (first occurrence)
#2689 2026-09-04 ❌ Empty
#2692 2026-09-11 ❌ Empty
#2694 2026-09-18 ❌ Empty
#2700 2026-09-25 ❌ Empty
#2708 2026-10-02 ❌ Empty

Every one of those PRs posted:

List of packages in the PR without any issue.</br>

These Packages need manual review..</br>

with no package list at all under either heading — despite real changes including
several MAJOR version bumps (e.g. cryptography 49→50, edx-organizations 8→9,
filelock 3→4, python-slugify 8→9 in PR #2708 alone).

I confirmed this isn't a regression in _parse_uv itself — genuinely uv-migrated
repos (openedx/taxonomy-connector, openedx/openedx-ledger) parse correctly via
the same _parse_uv path this week, with full and accurate package lists. The bug
is specifically the false-positive dispatch: the substring fires without an
actual uv.lock file in the diff.

Fix

Guard the dispatch with the PR's actual changed files — the same source of truth
_parse_uv already uses internally (pr.get_files()) — instead of trusting the
diff text alone:

uv_lock_changed = "uv.lock" in txt and any(
    f.filename == "uv.lock" for f in pull_request.get_files()
)

The "uv.lock" in txt fast-path check is kept first so repos whose diffs never
mention "uv.lock" never pay the extra get_files() call. For a real uv.lock diff,
its own diff --git a/uv.lock b/uv.lock header always contains the substring, so
this is a pure narrowing of false positives with no change to true-positive
behavior — correct across the full migration spectrum (pure pip-tools, pure uv, or
a repo transitioning between the two).

Testing

  • Added two regression tests to tests/test_pull_request_creator.py:
    • test_compare_upgrade_difference_uv_lock_mentioned_but_not_changed — reproduces
      the edx-enterprise scenario (diff text mentions "uv.lock" but it isn't a changed
      file); confirmed this test fails against the old code (_parse_uv gets called
      when it shouldn't) and passes against the fix.
    • test_compare_upgrade_difference_uv_lock_actually_changed — confirms a real
      uv.lock file change still routes to _parse_uv.
  • Full existing suite (uv run pytest tests/test_pull_request_creator.py): 24/24
    pass (22 existing + 2 new).
  • Sanity-checked against the real, saved diff from edx-enterprise#2708 (fetched via
    the same API + media type this tool uses) with the fix applied — it now correctly
    surfaces 127 valid and 32 suspicious package changes, including all the
    previously-hidden MAJOR bumps and one DOWNGRADE (django-simple-history).

compare_pr_differnce routed to the uv.lock parser whenever the literal
string "uv.lock" appeared anywhere in a PR's diff text, even when
uv.lock wasn't actually one of the changed files. A plain pip-tools
repo can trip this if a vendored/copied requirements file's generated
header happens to mention "pyproject.toml / uv.lock" after the repo
it's copied from migrates to uv -- the uv parser then finds no real
uv.lock file, returns nothing, and both review comments post empty.

Confirmed this has been silently hiding every dependency-risk comment
on openedx/edx-enterprise's weekly "chore: Upgrade Python requirements"
PRs since 2026-08-28 (6 consecutive PRs): edx-enterprise vendors
requirements/edx-platform-constraints.txt, copied from edx-platform,
and edx-platform's own uv migration added a "pyproject.toml / uv.lock"
line to that file's header.

Guard the dispatch with the PR's actual changed files (the same source
of truth _parse_uv already uses internally) before trusting the diff
text, so the fast path for repos that never mention uv.lock is
unaffected and genuinely uv-migrated repos route correctly either way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant