Skip to content

CI: symbol-order check validates unrelated files when a PR is behind main #151

Description

@KakarottoCake

Summary

The Validate symbol order step in build.yml picks its file list with a two-dot diff against the base branch tip. For any PR that is behind main, that list includes every file main has changed since the branch point — and those files are then validated in the PR's stale copies. The check fails on work the PR never touched.

Where

.github/workflows/build.yml, lines 30-48:

- name: Changed .cpp files
  run: |
    if [ "${{ github.event_name }}" = "pull_request" ]; then
      base="${{ github.event.pull_request.base.sha }}"
    ...
    git diff --name-only --diff-filter=d "$base" HEAD -- '*.cpp' \
      > changed_cpp.txt

git diff A B reports every path where the two trees differ, in both directions. It cannot distinguish "the PR changed this" from "main changed this and the PR hasn't caught up". The comment on line 29 already states the intent — "PR: vs base branch" — so this looks like an implementation detail rather than a deliberate choice.

Observed

On #130, which was 32 commits behind main:

Symbol-order check: 74 passed, 31 failed

The 31 failures were JKRHeap, J3DModel, J3DMaterial, JPADraw, EventWatcher, Yoshi, MarNameRefGen and similar — i.e. the units touched by the recent JSystem/J3D sweep. The PR modifies none of them:

$ git diff --shortstat origin/main HEAD -- src/JSystem/JKernel/JKRHeap.cpp
 1 file changed, 4 insertions(+)
$ git diff --shortstat $(git merge-base HEAD origin/main) HEAD -- src/JSystem/JKernel/JKRHeap.cpp
                       # empty - the PR does not touch this file

74 + 31 = 105, which is exactly git diff --name-only origin/main HEAD -- '*.cpp' | wc -l. Rebasing onto current main with no other change took the list to 10 files, all passing:

Symbol-order check: 10 passed, 0 failed, 0 skipped.

The same PR passed this step on 2026-07-27 only because the branch happened to sit exactly on main's tip that day. The step itself has been in place since 32a9cad (#129).

Suggested fix

Use the merge base, which is what "changed by this PR" means:

base="$(git merge-base "$base" HEAD || echo "$base")"

immediately before the git diff. actions/checkout already runs with fetch-depth: 0 (line 22), so the merge base resolves. Equivalently, git diff "$base"...HEAD — note the three dots.

The push branch (github.event.before) is unaffected either way, since that base is already an ancestor.

Why it is worth fixing

Rebasing works, but it makes a green CI contingent on how recently the contributor rebased rather than on the contents of their change. It is noisy on long-running branches, and it trains reviewers to skim past a red check — which is how a real symbol-order regression gets missed. It also means an unrelated merge to main can turn a previously-green PR red without anyone touching it.

Happy to send a PR if the approach looks right.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions