Skip to content

Fix breaking-change check: compare against merge-base, scope to PR diff#7467

Open
alfonso-noriega wants to merge 1 commit intomainfrom
fix-breaking-change-check-baseline
Open

Fix breaking-change check: compare against merge-base, scope to PR diff#7467
alfonso-noriega wants to merge 1 commit intomainfrom
fix-breaking-change-check-baseline

Conversation

@alfonso-noriega
Copy link
Copy Markdown
Contributor

@alfonso-noriega alfonso-noriega commented May 5, 2026

WHY are these changes introduced?

The "Breaking change detection" check in tests-pr.yml was reporting false-positive breaking changes whenever main had progressed past a PR's branch point. Concrete repro: PR #7466 only edits .github/workflows/tests-pr.yml, yet the check reports packages/app/src/cli/models/extensions/specifications/type-generation.ts removed a Zod field value. The field wasn't removed by the PR — it was added on main in commit bc9ca2e after the PR branched, and the script was diffing the PR (which is 7 commits behind) against main's current tip.

This pattern silently inflates over time: any PR that sits unrebased while schema or manifest edits land on main accumulates phantom breaking-change reports.

WHAT is this pull request doing?

Replaces "compare against main HEAD" with "compare against the merge-base of the PR head and the base branch", and scopes the schema/manifest scan to files actually present in the PR's diff.

workspace/src/major-change-check.js

  • New resolveContext() reads GITHUB_EVENT_PATH and resolves:
    • pull_request / pull_request_reviewmerge_base_commit.sha from the GitHub compare API; changedFiles = the compare's files[]
    • merge_groupmerge_group.base_sha (already the right baseline); changedFiles from the same compare API
    • Anything else (e.g. local invocation) → falls back to main HEAD with changedFiles=null so legacy behavior is preserved
  • checkSchemas and checkManifest now accept {changedFiles}. When provided, they skip files outside the diff. The OCLIF manifest check short-circuits entirely if packages/cli/oclif.manifest.json isn't in the diff (a removed command always shows up there by definition).
  • Compare-API failures degrade open, not closed: changedFiles=null so the scan widens rather than silently skipping potential removals. We never want to mask a real breaking change because of a transient API blip.

workspace/src/utils/git.js

  • cloneCLIRepository(tmpDir, {ref}) now takes a ref so callers can pin the baseline to the merge-base SHA (default still main).

workspace/src/major-change-check.test.js

  • 4 new tests covering the new context resolution: pull_request uses merge-base, merge_group uses base_sha, compare-API failure widens the scan, missing event payload falls through to main.

How to test your changes?

cd workspace
node --test src/major-change-check.test.js

All 13 tests pass locally, including the new 4. The existing schema-extractor tests are unchanged.

In CI, the false-positive on PR #7466 will disappear once this lands on main (the check will use the merge-base, which contains the same value field).

Post-release steps

None.

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows) — pure Node + git CLI, no platform-specific calls
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact — N/A (CI tooling)
  • The change is user-facing — N/A (internal CI tooling, no changeset needed)

Copy link
Copy Markdown
Contributor Author

alfonso-noriega commented May 5, 2026

@github-actions github-actions Bot added the no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. label May 5, 2026
@alfonso-noriega alfonso-noriega marked this pull request as ready for review May 5, 2026 12:24
@alfonso-noriega alfonso-noriega requested a review from a team as a code owner May 5, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant