refactor(file-diff): refactor changedFiles to do 3 ref diffing with intersection by JuanGalilea · Pull Request #209 · apify/apify-test-tools

JuanGalilea · 2026-10-19T11:52:27Z

My take on the diffing.
Tried to keep the changes to a minimum and didnt go into how isCosmeticOnlyJsonSchemaChange is doing its thing.
As mentioned in a comment in the code, that will be done as a followup but the idea is kinda the same as the current behavior

Solves #196

JuanGalilea · 2026-10-19T11:55:41Z

+        return { changedFiles, mergeBaseSha, resolutionType: 'simple', currentRef };
+    }
+    const changesSince = await getDiffChangedFiles(baseCommit, currentRef);
+    if (changesSince.length === 0) {


idk what other case could do [] with exit code 0.
So i'd say its pretty safe to assume its the same ref (as in <headRefSha>..HEAD) and return the whole PR diff.
This makes it so we don't have to rev-parse HEAD just to compare it with since.

Not sure if it is this line but according to Claude, if since is not in the branch (force push), your git call with crash

true.
Did some digging and we could do something to get around it by fetching that one sha or just ignoring baseCommit if not found.
For a longer term solution of this it seems like a reasonable thing to do (since the whole diffing we do already solves force-pushes).

JuanGalilea · 2026-10-19T11:59:01Z

+    const output = await runGitCommand(['diff', '--name-only', '-z', fromRef, toRef], {
+        operation: `list files changed between refs ${fromRef} and ${toRef}`,
+    });
+    return output.split('\0').filter(Boolean);


-z
When --raw, --numstat, --name-only or --name-status has been
given, do not munge pathnames and use NULs as output field
terminators.
Without this option, pathnames with "unusual" characters are
quoted as explained for the configuration variable
core.quotePath (see git-config(1)).

Codex's idea to play it safe around stupid names. Doesn't hurt to have it imo.

metalwarrior665

There are significant issues here. So either we just go with #204 or we update this and then decide which implementation is nicer or extract some code from it.

Here is Claude's analysis

The two PRs fix the same problem (issue #196) with the same idea: compare the branch against git merge-base, then narrow the result using the last validated commit. They differ in what they intersect, how they decide functional vs cosmetic, and which branch they build on.

Big picture

	apify/apify-test-tools#204	apify/apify-test-tools#209
Author / state	metalwarrior665, draft	JuanGalilea, open, reviewers requested
Base branch	`claude/remove-composite-commands`. It is stacked on the `get-affected-actors` refactor (`--base`/`--since`, separate functional and cosmetic lists), so it can't land until that stack does.	`master`. It works inside the current `resolveChangedActors` / `build` flow.
Size (`bin/` only)	About +93/−132, all in `affected-actors.ts` and `git.ts`	Adds `bin/git-file-diff.ts` and makes small edits to `main.ts`, `git.ts` and `diff-changes.ts`
Old merge-from-target code	Deleted: `hasMergeFromTarget` and `getBranchOnlyChangedFiles`	Callers removed, but both functions are still exported from `git.ts` as dead code

The core difference: what gets intersected

Both compute two diffs: merge-base..HEAD and since..HEAD.

#209 intersects files. resolveGitFileDiff keeps only the files that appear in both diffs, then classifies those files.
#204 intersects Actors. It classifies each diff into functional and cosmetic Actors separately. It then keeps the Actors that changed in both, and an Actor counts as functional only if both diffs say so (narrowToChangedSince).

This matters when changes to different files meet in one Actor's shared context. Example: the branch's validated change is in src/foo.ts, then it merges a master commit that changed src/bar.ts, used by the same Actor.

#209: the two file sets don't overlap, so the Actor is not tested.
#204: the Actor shows up in both diffs, so it is tested. This is the "can interact" case that the old comment on master ("individually validated changes can have novel interactions") was protecting, and #204's scenario table lists it.

Functional vs cosmetic (the open question in #196)

#204 picks the least impactful classification. A validated functional change followed by only cosmetic changes is cosmetic. The .actor JSON check runs once per diff, comparing that diff's own endpoints.
#209 is stricter for .actor JSON files. A file in the intersection counts as cosmetic only if both mergeBase→HEAD and baseCommit→HEAD are cosmetic. So a JSON schema that had a functional change, already validated, followed by a description-only tweak gets re-tested. The PR has a TODO saying this will be reworked in a follow-up. Other files in the intersection behave the same in both PRs.

Edge cases

Case	#204	#209
Rerun	Detected when `since === HEAD`	Detected when `git diff since HEAD` is empty, which also catches a push with no file changes
`since` not in local history (force-push)	Checks with `rev-parse --verify`, falls back to the whole branch	`git diff <sha> HEAD` throws, so the command fails
`--base-commit` sent as a JSON commit object	Not applicable, since the stack changed the input	⚠️ Likely a bug, see below
`get-changed-files` command	Left to the stack's redesign	Now returns the merge-base or intersected file list instead of the commit-range diff

Probable bug in #209: the PR workflow fills base_commit.txt from get-latest-commit, which writes the whole commit as JSON ({"sha":…,"author":…}). Master runs this through parseBaseCommit, which pulls out the SHA. resolveGitFileDiff passes config.baseCommit straight to git diff, so from the second push on a PR the build would probably fail with a git error. I found this by reading the code and didn't run it.

What's the same

Both pass {fromRef, toRef} refs instead of a commit list into isCosmeticOnlyJsonSchemaChange, which settles the old "validate this is the right commit range" TODO.
Both use git merge-base (three-dot) semantics for the branch diff, like GitHub's "Files changed" tab.
Both keep --since-only (release) behaviour as it is.
Both check out and inspect the literal HEAD. #209 ignores --source-branch for the diff.

Bottom line: #209 is the smaller change against today's master and can merge on its own. It has a coarser, file-level intersection, misses the case where the since commit has been force-pushed away, and probably breaks on the JSON-formatted base commit. #204 handles cross-file interactions per Actor and covers the rerun and force-push fallbacks, but it depends on the unmerged stack under it.

metalwarrior665 · 2026-10-19T16:50:16Z

+
+    const filesSince = new Set(changesSince);
+    return {
+        changedFiles: changedFiles.filter((file) => filesSince.has(file)),


This is a big logic bug. The intersection should be per Actor (like in #204), not per file since the whole Actor context can interact. E.g. if local branch changes /src/main.ts and master pulls /src/crawler.ts, this needs to intersect.

I wonder how this slipped through the agent (unless I'm wrong), not enough tests? This kinda already pulls me in direction to prefer mine unless there are bugs too :)

this is an oversight on my part. I did it deliberately this way due to not thinking 😅

this alone makes it worth closing and i will in favor of #204
But now that i solved it and gained some insight on the whole thing, there are several things we can streamline there.

I'll do a thorough review pointing those out and then we can go over them (or add them as improvements later)

metalwarrior665 · 2026-10-19T16:53:54Z

+ * currentRef is the literal HEAD ref; this function assumes the desired source commit is checked out.
+ * For more info, see https://github.com/apify/apify-test-tools/issues/196
+ */
+export const resolveGitFileDiff = async ({


According to Claude, this crashes with base commit being an object since you skippied parseBaseCommit

had no idea of this, i was under the impression we just got either SHA's or refs.

metalwarrior665 · 2026-10-19T16:56:58Z

+        return { changedFiles, mergeBaseSha, resolutionType: 'simple', currentRef };
+    }
+    const changesSince = await getDiffChangedFiles(baseCommit, currentRef);
+    if (changesSince.length === 0) {


Not sure if it is this line but according to Claude, if since is not in the branch (force push), your git call with crash

JuanGalilea added 2 commits October 19, 2026 12:55

resolve range

3042b15

minimal inclusion of new diffing

fb0dc9b

JuanGalilea requested review from metalwarrior665 and ruocco-l as code owners October 19, 2026 11:52

JuanGalilea self-assigned this Oct 19, 2026

JuanGalilea commented Oct 19, 2026

View reviewed changes

JuanGalilea changed the title ~~Git diff~~ refactor(file-diff): refactor changedFiles to do 3 ref diffing with intersection Oct 19, 2026

JuanGalilea mentioned this pull request Oct 19, 2026

refactor(isCosmeticOnlyJsonSchemaChange): handle 3 ref diffs and add some more smartness #210

Closed

JuanGalilea added this pull request to stack #211 October 19, 2026 15:29

metalwarrior665 requested changes Oct 19, 2026

View reviewed changes

ilker-ekin mentioned this pull request Oct 19, 2026

Diff files logic refactor #196

Open

Navigation Menu

Repository navigation

refactor(file-diff): refactor changedFiles to do 3 ref diffing with intersection - #209

refactor(file-diff): refactor changedFiles to do 3 ref diffing with intersection#209
JuanGalilea wants to merge 2 commits into
masterfrom
git-diff

JuanGalilea commented Oct 19, 2026

Uh oh!

JuanGalilea Oct 19, 2026 •

edited

Loading

Uh oh!

metalwarrior665 Oct 19, 2026

Uh oh!

JuanGalilea Oct 20, 2026

Uh oh!

JuanGalilea Oct 19, 2026

Uh oh!

metalwarrior665 left a comment

Uh oh!

metalwarrior665 Oct 19, 2026

Uh oh!

JuanGalilea Oct 20, 2026

Uh oh!

JuanGalilea Oct 20, 2026

Uh oh!

metalwarrior665 Oct 19, 2026

Uh oh!

JuanGalilea Oct 20, 2026

Uh oh!

metalwarrior665 Oct 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

JuanGalilea commented Oct 19, 2026

Uh oh!

JuanGalilea Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metalwarrior665 left a comment

Choose a reason for hiding this comment

Here is Claude's analysis

Big picture

The core difference: what gets intersected

Functional vs cosmetic (the open question in #196)

Edge cases

What's the same

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JuanGalilea Oct 19, 2026 •

edited

Loading