Add corpus regeneration diff CI - #284
Open
sirreal wants to merge 11 commits into
Open
Conversation
7.0.4 is the latest WordPress release. Pinning the point release rather than the 7.0 major tag tracks the patches to wp-includes, so the corpus matches what is shipped. WordPress 7.0 raised core's minimum PHP to 7.4, which is exactly this job's PHP floor. Verified against wp-includes@7.0.4: 1039 PHP files, all of them present in the export, no parse errors, ~49 MB of JSON. Both of the job's guards (>= 500 files, >= 1 MB of JSON) still hold.
Records what the corpus-diff check is, how to run it by hand with the repo's own tools/export-corpus.php and prep-diff.php, and the policy the workflow already states: the head checkout's tooling drives both sides.
sirreal
commented
Aug 20, 2026
sirreal
left a comment
sirreal
left a comment
Member
Author
There was a problem hiding this comment.
Note
This is an agentic review, generated by Claude Code at the repository owner's request.
Ready to land. Recommend landing this one first, so the parser-output changes queued behind it (#277, #282) get corpus evidence automatically.
Verified
- CI green:
test-php (7.4),test-php (8.4), and the newWordPress corpus regeneration diffjob. Mergeable, clean, based onmaster. - The corpus-diff job really ran end to end (run 32167418014, 40s): corpus verified at 1039 PHP files,
base.jsonandhead.jsonboth 48,979,143 bytes — byte-identical, as expected for a PR that changes only tooling — and the uploadedcorpus.diffartifact is 138 bytes, i.e. the zipped empty file. 0 hunks confirmed, and both size guards (>= 500 files, >= 1 MB) are exercised rather than nominal. This matches the numbers in the PR body. prep-diff.phpexists onmaster, so the head-drives-both-sides invocation resolves.tools/export-corpus.phpis 7.4-clean (no arrow functions, match, nullsafe, named args);$argc/$argvandini_set( 'display_errors', 'stderr' )are valid under the CLI SAPI.diff ... || [ $? -eq 1 ]is correct —$?on the right-hand side of||is stilldiff's status, with no intervening command to clobber it. Under the defaultbash -eshell the||is what keeps a non-empty diff from failing the job.fetch-depth: 0makesactions/checkoutfetch+refs/heads/*:refs/remotes/origin/*, soorigin/${{ github.base_ref }}exists for the merge-base step. Action pinning (SHA forsetup-php, major tag for theactions/*ones) matches the existingunit-test.ymlconvention.- README procedure matches what the workflow actually does, including the tooling policy.
Worth noting
- The step is better than its name. On
pull_request, checkout gives yourefs/pull/N/merge, whose parents are the base tip and the PR head — sogit merge-base origin/<base> HEADresolves to the base branch tip, not the historical branch point. That is the more useful comparison (base-as-it-is vs. base-plus-this-PR, always exactly one PR apart, no false hunks from other work landed since branching), but the step name and the header comment both say "merge base". Consider rewording so the next reader doesn't "fix" it. - "The job always succeeds" (PR body) is imprecise. Several steps fail deliberately: corpus < 500 files, either export < 1 MB,
diffexit 2, composer install failure. The workflow header states it correctly — "the job succeeds even when the diff is non-empty" — which is the property that actually matters. Only the PR body overstates it. - Stale text in the PR body. The opening paragraph still says "the WordPress 6.8 tag"; the Update section and
WP_CORPUS_TAGsay 7.0.4. Cosmetic, but worth a squash-message fix so the merged history isn't self-contradicting. export-corpus.php'sif ( ! is_array( $files ) )guard is effectively dead:get_wp_files()signals failure by returningnew \WP_Error( ... ), andWP_Errordoes not exist outside WordPress, so that path fatals before it can return. Harmless (only reachable on an unreadable directory) but the guard does not do what it appears to.- No
concurrencygroup, so every push to a PR queues another full run. At ~40s that is cheap; acancel-in-progressgroup would still be tidy.
Follow-ups already anticipated
- #286 stacks on this branch (
add-corpus-diff-ci) to keep the pin current — it cannot land until this merges. - Making the job blocking, and adding
wp-admin/includesto the corpus, are both correctly deferred.
This was referenced Aug 20, 2026
- Name the comparison for what it is: on pull_request HEAD is the PR merged into the base branch, so the merge base resolves to the base branch tip. Rename the step, explain it in the workflow, and say the same in the README, including how to reproduce that comparison locally. - Drop the unreachable is_array() guard in tools/export-corpus.php: get_wp_files() returns a WP_Error on failure, which does not exist under plain PHP, so that path fatals with a non-zero exit before it can return. - Add a concurrency group so a new push cancels the in-flight run for the same PR.
Both sides run under the same binary, so the PHP version is not part of the comparison; use the newest runtime in the unit-test matrix.
Both sides share the corpus, the PHP binary, and the exporter, so the export is already deterministic. prep-diff.php reconciles exports from different environments: it zeroes line numbers, strips global-namespace prefixes, and sorts collections. Measured against three open PRs, the raw diff was equal or smaller in every case and kept each change at its source location, while the collection sort relocated changed records and the erasures hid classes of real change. Diff the exports as emitted.
tools/corpus-diff-comment.sh renders the diff as Markdown: hunk and line counts, the compared commits, a download link, and the diff in a collapsed diff block when the body fits under GitHub's comment limit; otherwise the counts and the link alone. The workflow writes the same report to the job summary and creates or updates a single PR comment, found by a marker on its first line. pull_request runs for forks and Dependabot get a read-only token, so the comment step is skipped for them.
Corpus diff0 hunks. No behavior change over WordPress 7.0.4, parser at |
Instead of dropping the diff from the comment when it exceeds the size budget, keep the leading whole hunks that fit, and say how many of the total are shown. The download link still has the full diff.
wp-admin, the root files, and the bundled themes and plugins join wp-includes: 1880 PHP files instead of 1039, 64 MB of JSON instead of 49 MB, about 1.4 seconds more per export locally. More shapes of real code mean more regressions caught, and every hunk is still a parser behavior change. Only PHP files are kept in the cache; the cache key carries a layout suffix so the old wp-includes-only entry is not reused.
Member
Author
|
This is quite useful, I plan to merge soon. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a CI check that diffs the parser's output over a fixed corpus of WordPress source, so every PR shows exactly what it changes.
What it does
corpus.diffartifact. Oversize diffs show the first hunks that fit and say so.Why
Decisions
prep-diff.phpoutput. Both sides share the corpus, the PHP binary, and the exporter, so output is already deterministic.prep-diff.phpzeroes line numbers, strips namespace prefixes, and sorts collections: on real PRs that relocated changed records and would hide a line-number regression. It stays for cross-environment comparisons.tools/export-corpus.phpexports both sides, so a tooling change never reads as a parser change.workflow_runrelay for them is a follow-up.Whole job: 45 s. Demo of a dirty diff: #289.