Repository navigation
ci: add TypeScript public interface breaking-change-check project - #9825
danieljbruce wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new TypeScript-based breaking change check tool under ci/breaking-change-check and integrates it into the monorepo workspace. It also updates ci/run_conditional_tests.sh to support a new --list-dirs option and includes the ci directory in the test execution paths while excluding the breaking change check from triggering full CI runs. Feedback on the shell script points out a critical bug where pathspecs in GIT_DIFF_ARG can cause incorrect git diff evaluations, falsely triggering full test runs; a suggestion is provided to isolate the commit range and correctly scope the diff to the ci directory.
| if [[ "${GIT_DIFF_ARG}" == *" -- "* ]]; then | ||
| ci_diff_sep="" | ||
| else | ||
| ci_diff_sep="--" | ||
| fi | ||
| set +e | ||
| git diff --quiet ${GIT_DIFF_ARG} ci | ||
| git diff --quiet ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | ||
| changed=$? | ||
| set -e | ||
| if [[ "${changed}" -eq 0 ]]; then | ||
| echo "no change detected in ci" | ||
| else | ||
| echo "change detected in ci, we should test everything" | ||
| echo "result of git diff ${GIT_DIFF_ARG} ci:" | ||
| git diff ${GIT_DIFF_ARG} ci | ||
| echo "result of git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh':" | ||
| git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | ||
| GIT_DIFF_ARG="" | ||
| fi |
There was a problem hiding this comment.
When GIT_DIFF_ARG contains pathspecs (e.g., upstream/main...HEAD -- :!packages), appending ci directly to the command results in multiple pathspecs: :!packages, ci, :!ci/breaking-change-check, and :!ci/run_conditional_tests.sh.
In Git, multiple pathspecs are evaluated as a logical OR. This means any change in a file that is NOT in packages (such as files in core/) will match the :!packages pathspec, causing git diff to exit with a non-zero status even if there are absolutely no changes in the ci directory. This completely breaks the conditional test logic, causing any change in core/ to falsely trigger a full test run ("change detected in ci, we should test everything").
By extracting only the commit range from GIT_DIFF_ARG (using ${GIT_DIFF_ARG%% -- *}) and explicitly using -- ci ..., we ensure that we only check for changes in the ci directory, completely isolating the check from other pathspecs.
| if [[ "${GIT_DIFF_ARG}" == *" -- "* ]]; then | |
| ci_diff_sep="" | |
| else | |
| ci_diff_sep="--" | |
| fi | |
| set +e | |
| git diff --quiet ${GIT_DIFF_ARG} ci | |
| git diff --quiet ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| changed=$? | |
| set -e | |
| if [[ "${changed}" -eq 0 ]]; then | |
| echo "no change detected in ci" | |
| else | |
| echo "change detected in ci, we should test everything" | |
| echo "result of git diff ${GIT_DIFF_ARG} ci:" | |
| git diff ${GIT_DIFF_ARG} ci | |
| echo "result of git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh':" | |
| git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| GIT_DIFF_ARG="" | |
| fi | |
| commit_range="${GIT_DIFF_ARG%% -- *}" | |
| set +e | |
| git diff --quiet ${commit_range} -- ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| changed=$? | |
| set -e | |
| if [[ "${changed}" -eq 0 ]]; then | |
| echo "no change detected in ci" | |
| else | |
| echo "change detected in ci, we should test everything" | |
| echo "result of git diff ${commit_range} -- ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh':" | |
| git diff ${commit_range} -- ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| GIT_DIFF_ARG="" | |
| fi |
There was a problem hiding this comment.
Addressed in ff1dcf2: extracted commit_range="${GIT_DIFF_ARG%% -- *}" before running the ci directory diff check so any pathspecs present in GIT_DIFF_ARG are not combined with the ci pathspec.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new breaking-change-check package under the ci/ directory and integrates it into the workspace. It also updates ci/run_conditional_tests.sh to support a --list-dirs option and refactors how changes in the ci directory are detected and tested. The review feedback suggests improving the robustness of the git diff command in the shell script by isolating the revision range from any pathspecs in GIT_DIFF_ARG, and recommends removing the unused c8 dependency from the new package's configuration.
| if [[ "${GIT_DIFF_ARG}" == *" -- "* ]]; then | ||
| ci_diff_sep="" | ||
| else | ||
| ci_diff_sep="--" | ||
| fi | ||
| set +e | ||
| git diff --quiet ${GIT_DIFF_ARG} ci | ||
| git diff --quiet ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | ||
| changed=$? | ||
| set -e | ||
| if [[ "${changed}" -eq 0 ]]; then | ||
| echo "no change detected in ci" | ||
| else | ||
| echo "change detected in ci, we should test everything" | ||
| echo "result of git diff ${GIT_DIFF_ARG} ci:" | ||
| git diff ${GIT_DIFF_ARG} ci | ||
| echo "result of git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh':" | ||
| git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | ||
| GIT_DIFF_ARG="" | ||
| fi |
There was a problem hiding this comment.
When checking for changes in the ci directory, using GIT_DIFF_ARG directly can lead to incorrect results if it contains pathspecs for other packages (e.g., upstream/main...HEAD -- packages/foo). In such cases, any change in those packages will trigger a full test run, defeating the purpose of conditional testing. Extracting only the revision range using bash parameter expansion ${GIT_DIFF_ARG%% -- *} and explicitly specifying the ci pathspecs is much more robust and correct.
| if [[ "${GIT_DIFF_ARG}" == *" -- "* ]]; then | |
| ci_diff_sep="" | |
| else | |
| ci_diff_sep="--" | |
| fi | |
| set +e | |
| git diff --quiet ${GIT_DIFF_ARG} ci | |
| git diff --quiet ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| changed=$? | |
| set -e | |
| if [[ "${changed}" -eq 0 ]]; then | |
| echo "no change detected in ci" | |
| else | |
| echo "change detected in ci, we should test everything" | |
| echo "result of git diff ${GIT_DIFF_ARG} ci:" | |
| git diff ${GIT_DIFF_ARG} ci | |
| echo "result of git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh':" | |
| git diff ${GIT_DIFF_ARG} ${ci_diff_sep} ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| GIT_DIFF_ARG="" | |
| fi | |
| set +e | |
| git diff --quiet ${GIT_DIFF_ARG%% -- *} -- ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| changed=$? | |
| set -e | |
| if [[ "${changed}" -eq 0 ]]; then | |
| echo "no change detected in ci" | |
| else | |
| echo "change detected in ci, we should test everything" | |
| echo "result of git diff ${GIT_DIFF_ARG%% -- *} -- ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh':" | |
| git diff ${GIT_DIFF_ARG%% -- *} -- ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' | |
| GIT_DIFF_ARG="" | |
| fi |
There was a problem hiding this comment.
Addressed in ff1dcf2: replaced ${GIT_DIFF_ARG} ${ci_diff_sep} with commit_range="${GIT_DIFF_ARG%% -- *}" and explicit -- ci ':!ci/breaking-change-check' ':!ci/run_conditional_tests.sh' so package pathspecs in GIT_DIFF_ARG cannot cause false-positive full test runs.
| "@types/node": "^22.13.8", | ||
| "c8": "^10.1.3", | ||
| "gts": "^6.0.2", |
There was a problem hiding this comment.
The c8 package is listed as a devDependency but is not used in any of the scripts (which explicitly pass --no-c8 to disable coverage). Removing it keeps the dependencies clean and avoids unnecessary package installations.
| "@types/node": "^22.13.8", | |
| "c8": "^10.1.3", | |
| "gts": "^6.0.2", | |
| "@types/node": "^22.13.8", | |
| "gts": "^6.0.2", |
There was a problem hiding this comment.
Addressed in ff1dcf2: removed the unused c8 devDependency from ci/breaking-change-check/package.json and updated pnpm-lock.yaml.
|
I think we should close this in favour of #9826 |
Description
Adds a compiler-driven TypeScript breaking-change check project and GitHub Actions workflow.
Impact
Catches unannounced breaking public TypeScript interface changes while allowing marked major releases.
Testing
>= 28,000compiled consumer probes,< 1%invalid base probes).feat!:,fix!:,BREAKING CHANGE:footers, squash-merge title warnings, shallow-clone safety, and strictexportsmap handling) across 5 consecutive runs on Node.js and 5 consecutive runs on Bun (16 passingeach).GIT_DIFF_ARG="upstream/main...HEAD -- :!packages" node ./bin/linter.mjs --strictandGIT_DIFF_ARG="upstream/main...HEAD -- :!packages" ./ci/run_conditional_tests.sh --strict.