From 239dad775b1a08c3ad8f1ba614c29cb8cb619301 Mon Sep 17 00:00:00 2001 From: Rudy Mizrahi Celekli <47457359+rudycelekli@users.noreply.github.com> Date: Sun, 18 Oct 2026 01:24:01 -0400 Subject: [PATCH] fix(upstream): distinguish new paths from fork deletions (#585) * fix(upstream): distinguish new paths from fork deletions * docs(changelog): record fix for #585 --- CHANGELOG.md | 2 ++ tests/test_upstream_triage.py | 49 ++++++++++++++++++++++++++++++----- tools/upstream_triage.py | 27 ++++++++++++------- 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8927cc0d..637f76c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -194,6 +194,8 @@ per-file diff commands. the Jobtype item and a description paragraph (fails on master), and an entity-encoded `
` in a description that stays in the text. +- **Upstream checks distinguish new files from fork deletions** (#585) - a path absent from the base is reported as newly added upstream instead of incorrectly treated as a deletion in the fork. + - **`/add-portal` includes the `.agents` prefix in generated `allowed-tools`** (#545, `.claude/commands/add-portal.md`, `tests/test_add_portal_command.py`) - the specification template for the new portal's `SKILL.md` frontmatter wrote `allowed-tools: Bash(bun run skills//cli/src/cli.ts *)` diff --git a/tests/test_upstream_triage.py b/tests/test_upstream_triage.py index de91a40a..198c9e82 100644 --- a/tests/test_upstream_triage.py +++ b/tests/test_upstream_triage.py @@ -71,13 +71,15 @@ class UpToDateTests(TriageRepoFixture): class RelevanceFilterTests(TriageRepoFixture): def test_commit_touching_only_removed_files_is_skipped(self): - # Upstream edits a file this fork never had -> not relevant. + # Both start with the portal; upstream edits it while the fork drops it. self.write("portals/removed_portal.py", "x = 1\n") - self.commit("upstream: add removed_portal") + self.commit("shared: add portal") + self.write("portals/removed_portal.py", "x = 2\n") + self.commit("upstream: update portal") self.set_upstream_to_head() - # Fork drops back to before that commit and deletes nothing extra; - # the file simply is not in fork HEAD. git(self.root, "reset", "--hard", "HEAD~1") + git(self.root, "rm", "portals/removed_portal.py") + self.commit("fork: remove demo portal") result = self.run_triage() self.assertEqual(result.returncode, 0, result.stderr) @@ -98,19 +100,54 @@ class RelevanceFilterTests(TriageRepoFixture): self.assertIn("git cherry-pick", result.stdout) def test_changelog_only_footprint_is_skipped(self): + self.write("portals/gone.py", "y = 1\n") + self.write("CHANGELOG.md", "- base\n") + self.commit("shared: add portal and changelog") self.write("portals/gone.py", "y = 2\n") self.write("CHANGELOG.md", "- did a thing\n") self.commit("upstream: feature living in removed area + changelog") self.set_upstream_to_head() # Fork ships CHANGELOG.md but not the removed portal file. git(self.root, "reset", "--hard", "HEAD~1") - self.write("CHANGELOG.md", "- fork changelog\n") - self.commit("fork changelog") + git(self.root, "rm", "portals/gone.py") + self.commit("fork: remove demo portal") result = self.run_triage() self.assertEqual(result.returncode, 0, result.stderr) self.assertIn("changelog-only footprint in this fork", result.stdout) + def test_new_upstream_tool_is_worth_reviewing(self): + self.write("tools/new_workflow.py", "print('new workflow')\n") + added = self.commit("upstream: add a new workflow tool") + self.set_upstream_to_head() + git(self.root, "reset", "--hard", "HEAD~1") + before = git(self.root, "status", "--porcelain") + + result = self.run_triage() + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("**1** worth reviewing", result.stdout) + self.assertIn("tools/new_workflow.py", result.stdout) + self.assertIn(f"git cherry-pick {added}", result.stdout) + self.assertEqual(git(self.root, "status", "--porcelain"), before) + self.assertFalse((self.root / "tools/new_workflow.py").exists()) + + def test_new_tool_with_changelog_is_not_changelog_only(self): + self.write("CHANGELOG.md", "- base\n") + self.commit("shared: add changelog") + self.write("tools/new_workflow.py", "print('new workflow')\n") + self.write("CHANGELOG.md", "- new workflow tool\n") + added = self.commit("upstream: ship a new tool and changelog") + self.set_upstream_to_head() + git(self.root, "reset", "--hard", "HEAD~1") + + result = self.run_triage() + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("**1** worth reviewing", result.stdout) + self.assertIn(f"git cherry-pick {added}", result.stdout) + self.assertNotIn("changelog-only footprint in this fork", result.stdout) + class AlreadyAppliedTests(TriageRepoFixture): def test_cherry_picked_commit_drops_off_via_patch_id(self): diff --git a/tools/upstream_triage.py b/tools/upstream_triage.py index 9f6a83a2..4de0f800 100755 --- a/tools/upstream_triage.py +++ b/tools/upstream_triage.py @@ -23,7 +23,8 @@ Two signals drive the sort: 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 - touched paths against the working tree and flag accordingly. + touched paths against HEAD and the shared base: a genuinely new upstream + path is worth reviewing, rather than a file the fork chose to remove. Usage: python tools/upstream_triage.py [--remote upstream] [--branch master] Exits 0 always (a report, not a gate). Prints a note to stderr and exits 0 if @@ -81,10 +82,10 @@ def files_touched(sha: str) -> list[str]: return [f for f in out.splitlines() if f] -def path_exists(path: str) -> bool: - # ls-tree against HEAD is authoritative for "does this fork still ship it". +def path_exists(path: str, ref: str = "HEAD") -> bool: + # Read committed trees, rather than uncommitted files in the working tree. r = subprocess.run( - ["git", "cat-file", "-e", f"HEAD:{path}"], capture_output=True + ["git", "cat-file", "-e", f"{ref}:{path}"], capture_output=True ) return r.returncode == 0 @@ -154,6 +155,11 @@ 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} + try: + common_base = git("merge-base", "HEAD", ref).strip() + except subprocess.CalledProcessError: + # Without shared history, absence cannot establish a deliberate deletion. + common_base = None review: list[tuple[str, str, str, list[str]]] = [] skip: list[tuple[str, str, str, str]] = [] @@ -168,14 +174,17 @@ def main() -> int: skip.append((short, sha, subj, "on the fork's won't-port list")) continue touched = files_touched(sha) - present = [f for f in touched if path_exists(f)] + relevant = [ + f for f in touched + if path_exists(f) or common_base is None or not path_exists(f, common_base) + ] # A commit whose only surviving footprint is the changelog is one whose # real change lives in files this fork removed - the code doesn't apply, # only a doc line would. Low signal; demote it. - substantive = [f for f in present if f != "CHANGELOG.md"] - if touched and not present: + substantive = [f for f in relevant if f != "CHANGELOG.md"] + if touched and not relevant: skip.append((short, sha, subj, "touches only files not in this fork")) - elif present and not substantive: + elif relevant and not substantive: skip.append((short, sha, subj, "changelog-only footprint in this fork")) else: review.append((short, sha, subj, substantive)) @@ -190,7 +199,7 @@ def main() -> int: lines.append("### Worth reviewing") if review: lines.append("") - lines.append("| Commit | Subject | Fork files it touches |") + lines.append("| Commit | Subject | Relevant files it touches |") lines.append("|---|---|---|") for short, sha, subj, present in review: shown = ", ".join(f"`{p}`" for p in present[:4]) or "_(new/shared paths)_"