From 358d167cff096cf434a953e1efde1ecb3fa5f92d Mon Sep 17 00:00:00 2001 From: Rudy Mizrahi Celekli <47457359+rudycelekli@users.noreply.github.com> Date: Wed, 21 Oct 2026 01:21:40 -0400 Subject: [PATCH] fix(upstream-triage): distinguish subject hints from patch matches (#569) * fix(upstream-triage): require patch identity before skipping commits * fix(upstream-triage): retain subject-only ports as verification hints * changelog: place the #569 entry under [Unreleased] Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Mads Lorentzen Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ tests/test_upstream_triage.py | 55 +++++++++++++++++++++++++++++++++++ tools/upstream_triage.py | 17 ++++++++--- 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8eae222..0e66df58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -426,6 +426,8 @@ per-file diff commands. most cards and `height:0` alone is a spacer. Three new `Bypasses` cases, seven subtests failing on master, plus a calibration case pinning that either half alone stays visible. +- **Upstream triage distinguishes subject-only matches from verified patch matches** (#569) - matching subjects with different patches now name the fork commit and request verification instead of claiming the change was already cherry-picked. + ### Security - **CI no longer leaves the job token in `.git/config` or expands a PR-controlled name into a diff --git a/tests/test_upstream_triage.py b/tests/test_upstream_triage.py index c07faaa9..e3ff42c1 100644 --- a/tests/test_upstream_triage.py +++ b/tests/test_upstream_triage.py @@ -205,6 +205,61 @@ class AlreadyAppliedTests(TriageRepoFixture): self.assertIn("**1** worth reviewing", result.stdout) +class SubjectCollisionTests(TriageRepoFixture): + def test_different_patch_with_same_subject_requests_verification(self): + self.write("kept.py", "print('upstream fix')\n") + upstream_sha = self.commit("Fix configuration") + self.set_upstream_to_head() + git(self.root, "reset", "--hard", "HEAD~1") + self.write("shared.txt", "fork configuration\n") + fork_sha = self.commit("Fix configuration") + + result = self.run_triage() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("**0** worth reviewing", result.stdout) + self.assertIn(f"subject matches fork commit {fork_sha[:9]}, patch differs: verify", result.stdout) + self.assertNotIn(f"git cherry-pick {upstream_sha}", result.stdout) + self.assertNotIn("already applied (cherry-picked)", result.stdout) + + def test_conflict_resolved_personalized_port_has_distinct_verify_reason(self): + profile = ".claude/skills/job-application-assistant/01-candidate-profile.md" + self.write(profile, "Name: template\nGuidance: old\n") + self.commit("Add template profile") + self.write(profile, "Name: template\nGuidance: improved\n") + upstream_sha = self.commit("Improve profile guidance") + self.set_upstream_to_head() + git(self.root, "reset", "--hard", "HEAD~1") + self.write(profile, "Name: personal\nGuidance: old\n") + self.commit("Personalize profile") + picked = subprocess.run(["git", "cherry-pick", upstream_sha], cwd=self.root, + capture_output=True, text=True) + self.assertNotEqual(picked.returncode, 0, "fixture must exercise a real conflict") + self.write(profile, "Name: personal\nGuidance: improved\n") + git(self.root, "add", profile) + git(self.root, "cherry-pick", "--continue") + fork_sha = git(self.root, "rev-parse", "HEAD").strip() + result = self.run_triage() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("**0** worth reviewing", result.stdout) + self.assertIn(f"subject matches fork commit {fork_sha[:9]}, patch differs: verify", result.stdout) + self.assertNotIn("already applied (cherry-picked)", result.stdout) + + def test_same_patch_with_renamed_subject_is_still_already_applied(self): + self.write("kept.py", "print('upstream fix')\n") + upstream_sha = self.commit("Fix configuration") + self.set_upstream_to_head() + git(self.root, "reset", "--hard", "HEAD~1") + self.write("fork_only.txt", "fork\n") + self.commit("Fork configuration") + git(self.root, "cherry-pick", upstream_sha) + git(self.root, "commit", "--amend", "-m", "Port upstream configuration fix") + + result = self.run_triage() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("**0** worth reviewing", result.stdout) + self.assertIn("already applied (cherry-picked)", result.stdout) + + class SuggestedCherryPickOrderTests(TriageRepoFixture): def test_suggested_commands_apply_dependent_changes_in_ancestry_order(self): self.write("kept.py", "print('first upstream change')\n") diff --git a/tools/upstream_triage.py b/tools/upstream_triage.py index 7504ca28..2afe2669 100755 --- a/tools/upstream_triage.py +++ b/tools/upstream_triage.py @@ -18,8 +18,10 @@ Two signals drive the sort: 1. Already applied? A cherry-pick lands with a NEW sha but the same patch, so a raw sha comparison misreports it as missing. We compute git patch-ids for the - fork-only commits and treat any upstream commit whose patch-id (or exact - subject) matches as already applied. + fork-only commits and treat any upstream commit whose patch-id + matches as already applied. A matching subject with a different patch may be a + conflict-resolved port or an unrelated change: keep it as a distinct verify + reason in the advisory skip list, never as proof of a cherry-pick. 2. Relevant to this fork? A commit that only touches files this fork deleted (e.g. removed demo portals) is almost certainly N/A. We check each commit's @@ -175,7 +177,9 @@ def main() -> int: fork_only = rev_list(f"{ref}..HEAD") fork_patch_ids = {p for p in (patch_id(s) for s in fork_only) if p} - fork_subjects = {subject(s) for s in fork_only} + fork_subjects = {} + for fork_sha in fork_only: + fork_subjects.setdefault(subject(fork_sha), fork_sha) try: common_base = git("merge-base", "HEAD", ref).strip() except subprocess.CalledProcessError: @@ -188,9 +192,14 @@ def main() -> int: for sha in behind: subj = subject(sha) short = sha[:9] - if patch_id(sha) in fork_patch_ids or subj in fork_subjects: + if patch_id(sha) in fork_patch_ids: skip.append((short, sha, subj, "already applied (cherry-picked)")) continue + if subj in fork_subjects: + match = fork_subjects[subj][:9] + skip.append((short, sha, subj, + f"subject matches fork commit {match}, patch differs: verify")) + continue if any(sha.startswith(e) for e in wontport): skip.append((short, sha, subj, "on the fork's won't-port list")) continue