[metalwarrior665]

The core logic seems correct and clear improvement. Keeping in draft for further polish.

This is #196's "Solution 2" implemented on top of the simplified get-affected-actors, so we can discuss a concrete diff. It changes behaviour compared to master. The changes are listed below.

What it does

• --base: inspects git diff $(git merge-base base HEAD) HEAD, which is the same as base...HEAD and GitHub's "Files changed" tab. That is what the branch changes, not what it merged in from base.
• --base and --since: also classifies since..HEAD, which is everything since the last validation, including what came in through a merge or a rebase. It then keeps only the Actors that changed in both diffs. An Actor counts as functional only when both diffs say functional. On the question left open in Diff files logic refactor #196, that means taking the least impactful classification; taking the most impactful would re-test scenario A on every push.
• --since alone (releases): unchanged.
• Rerun and missing --since: when --since is HEAD (a rerun) or isn't in the local history (a force-push), it is ignored and the whole branch is checked.

What goes away

• hasMergeFromTarget, getBranchOnlyChangedFiles and getOwnChangesIfMergedFromBase, together with the merge-from-target log messages and the per-Actor TODO, which this resolves.
• Change detection now takes a { fromRef, toRef } range instead of a commit list. The cosmetic .actor JSON check therefore compares the right endpoints for each diff, which also resolves its "validate this is the right commit range" TODO.

bin/: 93 insertions, 132 deletions. With tests and README: 318 insertions, 410 deletions.

Behaviour on a scratch repo

I ran this PR's getAffectedActors on a scratch repo where Actors a and c share src/ and b is isolated. The "Before" column is the code in #203, which matches master.

Scenario	Before	This PR
Branch change already validated, then merged a master that only changed README, CHANGELOG and .vscode	–	– (a, c cosmetic)
Validated foo, then merged a master bar in the same shared context	a, c	a, c
Branch's own commits only change a README, but the merge commit itself edits src	– (missed)	a, c
Branch changes src, then merged a master that only changed b	a, c, b	a, c
Rebased onto a master that only changed b	a, c (full check)	–
Rebased onto a master that changed shared src	a, c	a, c
Only README edits since validation	–	– (a cosmetic)
Rerun (--since = HEAD)	a, c	a, c

Open points

• After a force-push, the old --since commit usually isn't in a fresh checkout, so the rebase improvement only kicks in if the workflow fetches that commit (git fetch origin <sha>). I haven't checked whether GitHub serves force-pushed commits that way. Without the fetch, it falls back to checking the whole branch, which is what master does today.
• With criss-cross merges, git merge-base picks one of several merge bases.
• The workflows are untouched; the PR workflow already passes --base and --since.

Checks

tsc, eslint, prettier and vitest (399 passed) all pass locally. Knip shows only the existing false positives caused by bin/main.ts not being a knip entry.

Refs #196

🤖 Generated with Claude Code

https://claude.ai/code/session_012dp3mSDDgp5u2QVBT54EpX

---

Generated by Claude Code

[JuanGalilea]

main thing we could do after (not here) is some accommodation for rebasing (fetch then diff) so we can process whatever --baseCommit we get

[JuanGalilea]

do we need this anymore?

[metalwarrior665]

From Claude

It was still needed: get-affected-actors skipped the commit list when the range was empty, because getCommitsBetween couldn’t parse git’s empty output and threw. I fixed that at the source instead:

getCommitsBetween now returns [] for an empty range, so the special case in get-affected-actors is gone.
getCommits is shared by get-commits, get-latest-commit and get-changed-files, which all need at least one commit. It still fails on an empty range, but now with a clear message (“ has no commits that are not on ”) instead of “Failed to parse commit string”.
The two mocked tests that checked the special case are gone. Real-git tests replace them:
a branch with no commits of its own, and a release with nothing new, both report nothing;
getCommitsBetween('HEAD', 'HEAD') returns [];
getCommits fails with the new message.

I will go through the refactored code (it tried to satisfy you :) ) once I address everything

[ruocco-l]

Personally this function is a bit too cryptic, I would never be able to understand what it is trying to do just by looking at the code. The JSDoc doesn't really help much

[JuanGalilea]

yeah, same.
I remember spending quite a while reading it until i got it because i was so sure there was a bug there somewhere (there isnt) but its quite a smell.

[metalwarrior665]

I hand polished it:

1. Rewrote JSDoc to be closer to how we see it (hopefully)
2. Renamed variables to be more explicit and added more comments. more verbose code.
3. Collapsed some if statements. We redo one git command because of that because we intersect equal sets but I think it is cleaner this way. Basically, since defaults base if no validated commit.

There is still room for improvement but I'm quite happy now

[JuanGalilea]

😂

[JuanGalilea]

kinda sus all the git manipulations.
Seems like it should work even for worktrees tho.