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
Comment thread workspace/src/major-change-check.js
@alfonso-noriega alfonso-noriega force-pushed the fix-breaking-change-check-baseline branch from 7082d87 to 5828156 Compare May 6, 2026 14:17
Per Gonzalo's review: collapses event-payload parsing + per-event-type
branching + GitHub compare API call into a single `git merge-base
origin/<base> HEAD` + `git diff --name-only`. Drops GITHUB_TOKEN
handling and the compare-API fallback path entirely.

- workspace/src/major-change-check.js: resolveContext now reads
  GITHUB_BASE_REF (set on both pull_request and merge_group events) and
  shells out to git via an injectable runGit. On any git failure we
  still degrade *open* (changedFiles=null) so we never mask a real
  breaking change.
- .github/workflows/tests-pr.yml: bump fetch-depth 1 -> 0 on the
  major-change-check job so the merge-base is reachable.
- workspace/src/major-change-check.test.js: rewrite the 4 resolveContext
  tests against a runGit mock; drop fs/event-file fixtures.
@alfonso-noriega alfonso-noriega force-pushed the fix-breaking-change-check-baseline branch from 5828156 to 3f375c0 Compare May 6, 2026 14:21
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.

2 participants