fix(ci): scope the spell check to the branch, not the whole divergence

[LHMQ878]

The bug

scripts/ci/changed_lines.py runs git diff <base>, a two-dot diff: base tip vs working tree. Every commit that lands on main after a branch is cut therefore appears in it — and the pre-existing side of each of those appears as an added line, because the working tree still holds it while main no longer does.

The spell-check job scopes cspell to exactly those lines, so on any branch that has fallen behind, cspell is handed repo-wide vocabulary and fails on words the branch never wrote.

Measured on a branch 34 commits behind main (one commit, 2 files, +142):

two-dot   (what CI does):  160102 added lines
three-dot (the real diff):    142 added lines

Every word the job flagged appears zero times in that branch's actual diff:

word              in 2dot  in 3dot
huggingface             5        0
CBRN                    4        0
Sarin                   2        0
IMDA                    6        0
organisations           4        0
ctypes                  7        0
SPENDGUARD              3        0
Deidentification        1        0

The reported line numbers went up to 159873, in a file the branch does not touch. So the failure is not "a contributor used an unknown word" — it is the check reading the base branch's own history as the contributor's work. Currently there is no way for a contributor to make it pass other than rebasing.

--mode changed-files has the same defect; nothing in-tree uses that mode today, but it answers the same question, so it is fixed by the same change and has a test.

The change

The base is resolved to its merge base with HEAD before diffing, which excludes the base-side commits.

Resolving the base rather than switching the diff to git diff base...HEAD is deliberate: the three-dot form compares two commits and would ignore the working tree, and this script is also usable locally before committing. There's a test asserting uncommitted work is still reported.

An unresolvable merge base falls back to the base tip — the previous behaviour — with a warning to the log rather than an exception. Over-reporting is the safe direction for a check built on this: it flags too much rather than missing something.

The --depth=1 in the workflow had to go with it

git fetch origin <base> --depth=1 writes .git/shallow into the full clone that fetch-depth: 0 just made, truncating the history so merge-base cannot reach the divergence point. Verified in an isolated repo — the depth-limited fetch turns a working merge-base into exit code 1:

== full clone ==
shallow file: no
merge-base: 97a8ca19...

== after: git fetch origin main --depth=1 ==
shallow file: YES
merge-base exit: -> FAILED rc=1

and the diff then behaves as the bug describes:

--- current CI: two-dot ---
    +line 1                 <- rewritten on main, never touched by the branch
    +my own added line

--- unshallow, then merge-base ---
    +my own added line

Note the fetch is why the bug survived: with the graft in place merge-base would fail on every run, so the fix would silently do nothing. That is also what the fallback test pins down, so a future reintroduction of --depth degrades visibly instead of quietly.

Verification

• 4 new tests in tests/ci/test_changed_lines.py, over real git repositories with a base that has moved on. Reverting only the script fails exactly those 4 — 3 assertion failures and the resolve_merge_base attribute error — and leaves the 3 existing tests passing.
• pytest tests/ci — 72 passed, 6 skipped.
• ruff check ... --select E,F,W --ignore E501 clean, matching what ci.yml runs. I did not run ruff format: the repo has no ruff config at the root and the committed file does not satisfy the default formatter, so running it would have reformatted lines this PR does not touch. The test file diff is +117/-0.
• generate_workflows.py --check reports no drift — spell-check.yml is hand-authored, not generated.
• End to end on the real 34-commit-behind branch, with the fix applied: 304 added lines (including uncommitted test files) instead of 160102, and cspell@8.17.3 --config .cspell.json finds none of the words above.
• This PR's own added lines through the fixed script and the real cspell: 161 lines, exit 0. No dictionary additions needed.

I ran into this because it is failing my own open PRs, so the "34 commits behind" branch above is mine. The fix is not specific to it — every PR that sits for a while hits this.

I'm happy to split the --depth=1 removal into its own commit if you'd prefer to review them separately, and to add .cspell.json entries instead if you consider the current base-tip behaviour intentional.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

🤖 AI Agent: contributor-guide — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Welcome to the microsoft/agent-governance-toolkit! Thank you for your contribution.

You've done a great job providing a detailed explanation of the issue and the solution in your pull request description.

Before we can merge, please ensure that your code passes all tests and adheres to the project's coding standards. Specifically, make sure to run ruff format if applicable, and include any necessary configuration files if they are missing.

For more guidance, please refer to our CONTRIBUTING.md.

[github-actions]

🤖 AI Agent: breaking-change-detector — API Compatibility

AI-generated review output. Treat it as untrusted analysis and verify before acting.

API Compatibility

Severity	Change	Impact
High	resolve_merge_base function added to scripts/ci/changed_lines.py.	If any external code depends on the previous behavior of run_git_diff without the resolve_merge_base logic, this change could break compatibility.
Medium	run_git_diff function in scripts/ci/changed_lines.py now uses resolve_merge_base to determine the base commit for git diff.	Changes the behavior of run_git_diff by modifying the base commit used for diffs. This could impact any external scripts or tools relying on the previous behavior.

[github-actions]

🤖 AI Agent: code-reviewer — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

TL;DR: 0 blockers, 0 warnings. The fix is robust and improves CI correctness.

#	Sev	Issue	Where

No action items needed. Clean change.

[github-actions]

🤖 AI Agent: test-generator — `scripts/ci/changed_lines.py`

AI-generated review output. Treat it as untrusted analysis and verify before acting.

scripts/ci/changed_lines.py

• test_missing_merge_base_falls_back_to_the_base_tip -- Ensure fallback behavior when git merge-base fails is tested for all edge cases, such as invalid base references or corrupted repositories.
• test_uncommitted_work_is_still_reported -- Add tests for scenarios where uncommitted changes include binary files or files with special characters in their names.
• test_added_lines_exclude_commits_that_landed_on_the_base -- Validate behavior when the base branch includes merge commits or cherry-picked changes.

[github-actions]

🤖 AI Agent: docs-sync-checker — Docs Sync

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Docs Sync

• resolve_merge_base() in scripts/ci/changed_lines.py -- missing docstring
• CHANGELOG.md -- missing entry for the behavioral change in how the base is resolved for diffs in scripts/ci/changed_lines.py

[github-actions]

🤖 AI Agent: security-scanner — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

No security issues found.

[github-actions]

🔴 Contributor Check: HIGH

Check	Result
Profile	HIGH
Credential	LOW
Overall	HIGH

Automated check by AGT Contributor Check.

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[Copilot]

Pull request overview

TL;DR: 0 blockers, 1 warning. This ships; warning is fine as a follow-up.

#	Sev	Issue	Where
1	Warn	Warning message is printed to stdout (can pollute piped output); prefer stderr	resolve_merge_base()

Changes:

• Resolve the diff base to the git merge-base with HEAD to avoid attributing base-branch changes to the PR branch.
• Add regression tests covering diverged base branches, uncommitted work, --mode changed-files, and merge-base failure fallback.
• Update the spell-check workflow to avoid --depth=1 fetching that would break merge-base resolution.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
scripts/ci/changed_lines.py	Resolve base ref to merge-base before diffing to scope “added lines/changed files” to the PR branch’s actual changes.
tests/ci/test_changed_lines.py	Adds git-backed regression tests for diverged branches, uncommitted work inclusion, and merge-base fallback behavior.
.github/workflows/spell-check.yml	Removes depth-limited fetch to preserve history needed for merge-base and prevent repo-wide spell-checking on stale branches.

[liamcrumm]

This is a really good change, I've been seeing this on a variety of PRs.

[liamcrumm]

Move the resolve_merge_base warning to stderr (file=sys.stderr) and update the test to assert on capsys.readouterr().err. That covers both Copilot comments.

Approving the workflow runs now so CI can run. Once that change is in and CI is green, we'll merge.

[LHMQ878]

Thanks — one of the two findings was right and is addressed in 28303f47; the other doesn't hold.

CHANGELOG entry — valid, added

Fair catch: how the base is resolved is user-visible for anyone running changed_lines.py locally, and removing the depth-limited fetch changes the workflow's behaviour too. Added a ### Fixed entry under [Unreleased] (the section didn't exist yet). It records the measured numbers rather than just "fixed a diff bug", and calls out the fetch change separately, since a shallow fetch into the full clone writes .git/shallow and truncates the history git merge-base needs — leaving that in would have made the script change a silent no-op in CI.

resolve_merge_base() missing a docstring — not accurate

It has one. Verified by importing the module rather than reading the diff:

docstring present: True  length: 969
first line: Return the commit where `base` and the working tree's history diverged.

It's the longest docstring in the file — it explains why the two-dot git diff <base> form over-reports, and why this resolves the base instead of switching to git diff base...HEAD (that form compares two commits and would drop uncommitted work, which matters because this script is also run locally before committing).

Everything else is unchanged; Spell-check changed files — the job this PR fixes — is green on the branch.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

scripts/ci/changed_lines.py:75

• The warning in resolve_merge_base() is printed to stdout. When this script is used without --output (or its output is piped/consumed), this warning will be mixed into the functional output and can break downstream consumers. Print warnings to stderr instead.

        message = result.stderr.strip() or f"git merge-base exited {result.returncode}"
        print(f"warning: cannot resolve merge base with {base} ({message}); diffing against its tip instead")
        return base

tests/ci/test_changed_lines.py:172

• This test asserts the merge-base warning appears on stdout, but warnings should go to stderr to avoid corrupting the script's normal stdout output. Update the assertion to check capsys.err.

    assert changed_lines.resolve_merge_base(repo, "refs/heads/no-such-branch") == "refs/heads/no-such-branch"
    assert "cannot resolve merge base" in capsys.readouterr().out

[LHMQ878]

Done in 3bdfbde6 — file=sys.stderr, and the test now asserts on .err.

Worth flagging that this is a bit more than a tidiness fix, which I only saw when writing the test. Without --output the script writes its result to stdout, so the warning was sharing the data channel. With an unresolvable base and stderr discarded, the sole line on stdout is:

$ python scripts/ci/changed_lines.py --base does-not-exist-xyz \
    --extensions .md --mode changed-files 2>/dev/null
warning: cannot resolve merge base with does-not-exist-xyz (fatal: Not a valid object name does-not-exist-xyz); diffing against its tip instead

A caller doing for f in $(changed_lines.py ... --mode changed-files) gets warning:, cannot, resolve as file names, and exit status stays 0. To be clear about the blast radius: the spell-check workflow passes --output, so it was never affected. But --output is optional and the fallback exists for exactly the shallow clone a CI job runs in, so the two do meet in any other stdout consumer.

So I asserted it at both levels rather than only where you suggested:

• test_missing_merge_base_falls_back_to_the_base_tip — now assert "cannot resolve merge base" in captured.err, plus assert captured.out == ""
• test_warning_does_not_contaminate_the_result_on_stdout — new; runs the script as a subprocess and requires every stdout line to be a path the diff actually produced, which is the property the caller depends on

One thing the second test cost me a pass on: my first version used a nonexistent ref, which fails the fallback git diff too (exit status 128), so it never reached the case under test. It now builds two unrelated root commits — the base ref resolves, the fallback diff succeeds, and there is genuinely no merge base to find. The reason is in a comment so nobody simplifies it back.

Discrimination: restoring the print to stdout fails both tests —

tests/ci/test_changed_lines.py:174: AssertionError: assert 'cannot resolve merge base' in ''
tests/ci/test_changed_lines.py:227: AssertionError: assert 'cannot resolve merge base' in ''

tests/ci/test_changed_lines.py is 8 passed. ruff check --select E,F,W --ignore E501 clean on both files, matching what ci.yml:260 runs. (Plain ruff format --check does want to reformat both files, but on pre-existing lines — it defaults to 88 columns where this repo writes wider — and no workflow runs it, so I left that alone rather than reflow code this PR does not touch.)

Ready for CI whenever you approve the runs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

tests/ci/test_changed_lines.py:172

• If the merge-base warning is emitted on stderr (so stdout remains clean for script output), this assertion should check capsys stderr rather than stdout.

    assert changed_lines.resolve_merge_base(repo, "refs/heads/no-such-branch") == "refs/heads/no-such-branch"

scripts/ci/changed_lines.py:75

• The fallback warning is printed to stdout, which will corrupt the script’s normal stdout output when --output is not used (e.g., piping the added-lines output into another tool). Warnings/logs should go to stderr so the data stream stays parseable.

        # checks built on this -- they flag too much rather than miss something.
        message = result.stderr.strip() or f"git merge-base exited {result.returncode}"
        # stderr, not stdout: without `--output` this script emits its result on

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[LHMQ878]

A live instance of this on another of my open PRs, in case it is useful for prioritising: #3512 is failing Spell-check changed files purely from this bug.

Same measurement, exact job command (cspell@8.17.3, --config .cspell.json), on a branch 40 commits behind main whose own diff is 133 lines across 2 files:

two-dot   (what CI does today):  160125 added lines  -> FAIL
                                 SNOMED, huggingface, CBRN, Sarin,
                                 cryptominer, SPENDGUARD, deidentification, ...
                                 reported at lines 155439-159896

merge-base (what this PR does):     133 added lines  -> exit 0, no output

Every flagged word is in a file #3512 does not touch. That is the second branch where I have measured it, and the failure mode is identical: the count is three orders of magnitude off, and the contributor has no way to make it pass except rebasing.

No change requested here — this PR already fixes it. Just recording a second data point on a real red check.

[Imran Siddique (imran-siddique)]

Worth prioritising this one: Spell-check changed files is currently red on 14 open PRs (#3120, #3169, #3199, #3200, #3296, #3362, #3365, #3367, #3375, #3383, #3419, #3440, #3441, #3576), which matches the root cause filed as #3514. Landing a scope fix clears that whole cluster rather than 14 individual rebases.

Note #3568 targets the same gate by scoping the diff to the PR merge-base, so the two overlap. Worth a maintainer picking one and closing the other rather than both landing.

[LHMQ878]

liamcrumm both of your conditions are met — the stderr change went in as 3bdfbde (before your comment, so it may have landed after you last looked):

print(
    f"warning: cannot resolve merge base with {base} ({message}); diffing against its tip instead",
    file=sys.stderr,
)

and the test now asserts on err, plus that stdout stays empty — since stdout is the result channel when --output is omitted, a warning there is read back as a changed file name:

captured = capsys.readouterr()
assert "cannot resolve merge base" in captured.err
assert captured.out == ""

CI on 3bdfbde: 94 success, 5 skipped, 1 neutral, 0 failures across 100 check runs.

Imran Siddique (@imran-siddique) I checked the list you posted. Of the 14, 10 are currently red on Spell-check changed files — #3120, #3199, #3200, #3296, #3362, #3365, #3375, #3383, #3419, #3440. The other four (#3169, #3367, #3441, #3576) have no spell-check run recorded on their head SHA at all, so they're not evidence either way.

The logs confirm it's this root cause rather than real typos. On #3120 — a 2-file, +72/-6 TypeScript change:

ci-diff/spell-check-added-lines.txt:1:15 - Unknown word (dorny)
ci-diff/spell-check-added-lines.txt:3:15 - Unknown word (dorny)
ci-diff/spell-check-added-lines.txt:396:11 - Unknown word (coreutils)

A 2-file PR produced an added-lines file 396 lines long, and dorny is a dorny/paths-filter reference — from a workflow file that PR doesn't touch. Same dorny hit at lines 1–3 on #3199 and #3362 too, neither of which touches a workflow. Those authors are being asked to fix words they didn't write.

The inverse is worth noting as well: #3440 has exactly one hit, serch → search, which is a real typo in the author's own line. That's the actual cost here — the signal works, it's just buried, so a red X on this check currently carries no information.

Ready to merge whenever you are; happy to rebase if anything has moved.