activity: harden the data contract before automatic merges - #286
Conversation
The reporter's daily PRs will merge themselves once they pass the data check, so the data contract they must meet is written down first. README's "Activity data" now says each file is named for the UTC day its entries cover, not the run date, and that files are append-only: a re-run adds its new entries after the existing ones and never changes or removes one. It appends them as text after the file's last byte, with no --- line, since re-dumping the file would drop its header comment and Jekyll reads only a file's first YAML document. The older migrated and hand-added files are named for the day they were written. The paragraph also lists what CI checks, including the limit on the reporter's own PRs, and says a correction to a published entry needs an admin to merge it. The definitions table adds updates to existing books to Activity (a new book stays under News), and the data format gains the book type. The comment in _layouts/activity.html matches; nothing renders differently. Part of #284. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Once the reporter's PRs merge themselves, this check is the only gate on the data that reaches the site, and it passed both an overwritten file and a release listed twice. check-activity-data.rb now accepts the book type, and: - lists _data/activity/ itself and fails on anything there but regular .yml and .yaml files. Jekyll would read and show a .json, .csv or .tsv file or a subdirectory, unchecked. Names that start with a dot are skipped, as Jekyll skips them. - fails a file that holds more than one YAML document, since Jekyll reads only the first, so entries after a --- line would be neither checked nor shown. - fails when a release URL or a pull-request URL is listed more than once, in one file or across files, naming every file and entry involved. The GitHub owner and repository are compared without case. - with --base <revision>, fails when a data file at that revision is deleted, or its entries are changed, removed or reordered, or a new entry is put before them. Entries are matched by value (the parsed YAML is compared), so removing one entry names only that entry. The base is read with git, with paths tagged UTF-8 so that a file with a non-ASCII name is compared too, and a revision that can't be read fails the check. Without --base only that comparison is skipped, and the output says so. Every existing rule and message stays; annotation text is now escaped. Part of #284. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
build.yml gets a read-only token and a depth-2 checkout, so the test merge commit's parents are there. A new first step, before setup-ruby runs any of the PR's code, fails a PR opened by the reporter App (bot user ID 294005175, not the login) unless it only adds _data/activity/YYYY-MM-DD-(software|lectures).yml files or appends to them byte for byte. It is plain bash and git, inline, since the App can't change workflow files. When HEAD^2 is missing, it fails and names both causes: HEAD isn't the pull request's merge commit, or the checkout is too shallow to hold its parents. The data check now runs with --base HEAD^1, and the job stays named build, the required check. test-check-activity-data.rb, run by a new step, builds real merge commits, checks them out the way actions/checkout does, and runs the guard's own script from build.yml and the data check on them. It covers each rule of both checks; two appends the guard passes and the data check must fail (one continues the old last line, one starts a second YAML document); a depth-1 checkout; a checkout of the PR's own head commit; and paths that pin both anchors of the guard's path pattern. The first base file holds multi-byte text, so reading the base has to count bytes. Part of #284. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ Deploy Preview for grand-swan-ca5201 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes security-sensitive CI that gates unattended auto-merge to production, and the PR itself flags an unresolved bypass-actor boundary and requires maintainer sign-off on the definitions change, so it needs human review.
Review effort: Balanced
Findings: None
What changed in this PR
This PR hardens the _data/activity/ data contract in the QuantEcon website's CI so that the build check becomes a reliable gate before QuantEcon/reports-activity#91 lets the reporter App's daily Activity PRs merge themselves without human review (closes #284, part of #271). It closes two gaps the current check misses: silently overwritten data files and the same release/PR appearing in two files. It also documents the new day-named, append-only file convention and adds a book type.
Changes:
- Rewrites
check-activity-data.rbto add:booktype, a directory-contents check (only.yml/.yaml), a single-YAML-document rule, duplicate release/PR URL detection (case-insensitive owner/repo), an optional--base <revision>append-only comparison against git, and workflow-command escaping in::error::annotations. - Adds a
build.ymlbash guard step (keyed on the reporter bot user ID294005175, before any PR-controlled code) that restricts reporter PRs to added/appended_data/activity/YYYY-MM-DD-(software|lectures).ymlfiles, plus top-levelpermissions: contents: read,fetch-depth: 2, and a new regression-test step. - Adds a committed 38-case minitest suite that runs in
build, and updates the README and the_layouts/activity.htmlcomment to describe day-named, append-only files and thebooktype.
| File | Description |
|---|---|
.github/scripts/check-activity-data.rb |
Adds book type, directory/single-document validation, duplicate-URL detection, and --base append-only comparison with escaped annotations. |
.github/workflows/build.yml |
Adds read-only permissions, fetch-depth: 2, an inline reporter-PR guard step, --base HEAD^1 on the check, and a regression-test step. |
.github/scripts/test-check-activity-data.rb |
New minitest suite (38 cases) exercising the data check and the guard via real merge commits; runs in CI. |
README.md |
Documents day-named append-only files, the book type, and the new CI checks. |
_layouts/activity.html |
Liquid-comment-only update describing the append-only, day-named file scheme. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Once QuantEcon/reports-activity#91 is live, the reporter's daily Activity PRs merge themselves, so
buildbecomes the only gate on what reaches quantecon.org. This PR makes that gate hold the contract settled on 2026-09-29: day-named, append-only data files, a newbooktype, and a limit on what the reporter's own PRs may touch. Today the check passes an overwritten file (in a probe the entry count fell from 40 to 37 and it still printed "Activity data OK") and the same release listed in two files.What changes
README ("News and Activity") and the
_layouts/activity.htmlcomment. Files are_data/activity/<day>-<stream>.yml, named for the UTC day the entries cover, not the run date.lecturesfiles now also hold book updates. Files are append-only: a re-run adds new entries after the existing ones and never changes or removes one, and a run writes no file for a stream with nothing to report. The README also says how to append: as text after the file's last byte, with no---line, because re-dumping the whole file would drop its header comment and Jekyll reads only a file's first YAML document. The definitions table adds "updates to existing books" to Activity (a new book stays under News), and the field table addsbooktotypeand to thedateandurlrules. The paragraph lists what CI checks: only.ymland.yamlfiles in_data/activity/, one YAML document per file, no repeated release or PR URLs, no changed or removed entries, and, for the reporter's PRs, only added day files or appends that keep every existing byte (the first step after checkout). It also notes that the older migrated and hand-added files are named for the day they were written. The sentence on how the Activity page orders entries is unchanged, since #277 rewrites it. The layout change is a Liquid comment only..github/scripts/check-activity-data.rb. Every existing rule, message and the "Activity data OK (N files)" line stay. New:bookis a valid type._data/activity/may hold only.ymland.yamlfiles. The check lists the directory itself and fails, naming the path, on anything else: Jekyll would read and show a.json,.csvor.tsvfile or a subdirectory there, unchecked. A.ymlname that isn't a regular file fails too (a directory used to crash the check). Names that start with a dot are skipped, as Jekyll skips them.---line would be neither checked nor shown. The check parses the whole stream and fails a file with more than one document.--base <revision>, every data file at the base must still exist, and its entries must be the first entries of the new file, unchanged (the parsed YAML is compared). Entries are matched by value, so each error says what happened: an entry removed (the entries after it aren't reported as changed), changed (with the keys that differ), a new entry added before existing ones, or the existing entries reordered. A deleted file fails too. The base is read with git (rev-parse,ls-tree, onecat-file --batch), with paths read as UTF-8, so a file with a non-ASCII name is compared like any other. A base file is compared with the data file of the same name, or reported as deleted, so none is skipped silently. If the base can't be read, the check fails and says why. Without--base, as in a local run, only this comparison is skipped, and the output says so.::error::lines are escaped (%, CR, LF) so they can't end the annotation or start another workflow command..github/workflows/build.yml. The job id staysbuild, and no job is added.permissions: contents: readat the top level.actions/checkoutgetsfetch-depth: 2. Onpull_requestit checks out GitHub's test merge commit, soHEAD^1is the tip ofmainandHEAD^2the PR's head.github.event.pull_request.user.id == 294005175(the reporter App's bot user ID; never the login or slug, which change with the rename). It sits beforeruby/setup-ruby, whosebundle installevaluates the PR's Gemfile, and before every other step that runs the PR's code. It is inline because the App has noworkflowspermission, so the App can't change it. It uses bash and git, plusmktempandheadfrom the runner image. It diffsHEAD^1againstHEADwith renames off, and fails with an::error::naming each offending path unless every change is an added regular file (mode 100644), or a modification of one whose base content is a byte prefix of its new content, at a path matching_data/activity/YYYY-MM-DD-(software|lectures).yml. Deletions, renames (a deletion plus an add), mode changes, symlinks, submodules and every other path fail. So does a checkout withoutHEAD^2, and the error names both causes:HEADisn't the merge commit, or the checkout is shallower thanfetch-depth: 2.ruby .github/scripts/check-activity-data.rb --base HEAD^1, whoever opened the PR.ruby..github/scripts/test-check-activity-data.rb(new, committed, and run bybuild). It has 38 minitest cases. The PR cases build real merge commits, fetch them at depth 2 and check them out detached, as actions/checkout does. They run the guard's ownruntext, read frombuild.ymlwith YAML at test time, throughbash --noprofile --norc -eo pipefail(as Actions runsshell: bash), and the data check with the arguments parsed from the workflow's step. Covered:bookpasses; an unknown type fails; duplicate release and PR URLs fail, across two files (naming both) and within one file; a file with two YAML documents fails (after---, or after...and a document with a Ruby object tag), while one document that starts with---or ends with...passes; a.json,.csvor.yml.jsonfile, a subdirectory, a directory named like a data file and a dangling symlink each fail and are named, while.DS_Storeis skipped; the existing rules still fail as before.mainafter the branch are not counted; a rewrite, a deletion, a rename, a mode change, an added symlink or executable, an edit to_layouts/orGemfile, and ten paths that aren't day files each fail and are named (two of them, with a suffix after.ymland a prefix before_data/, pin both anchors of the path pattern); a checkout of the PR's own head commit fails although its change alone would pass, and so does a depth-1 checkout; a path containing a newline can't inject a workflow command.---adds a second YAML document. The guard passes both, since the old bytes are a prefix, and the data check fails both.buildis the only job; the guard is step 2, keyed on the bot user ID, with no${{ }}in its script; the data check passes--base HEAD^1.How it was tested
Regression test, locally in three setups. CI runs it under Ruby 3.4 with minitest 5.25 and bash 5.
Each run takes about 12 seconds, mostly git process start-up. minitest 6 counts two more assertions for the same tests.
Data check on this branch's data (
main's 23 files and 40 entries, which have no duplicate URLs, even ignoring case), withcd6a5e2beingmainwhen this branch was made:The first prints the skipped-comparison note and "Activity data OK (23 files)". The second prints "Compared with the base cd6a5e2 (cd6a5e2): the 40 entries in its 23 files are unchanged." and "Activity data OK (23 files)".
A scratch harness built a merge commit for each of 42 PR scenarios on top of the real files. The scenarios covered appends, rewrites, deletions, renames, mode changes, symlinks, submodules, paths outside the data directory, duplicates, book and unknown types, CRLF, a depth-1 checkout and others. The harness ran the guard and the data check on this branch and on its state before the local review's fixes. Only three outcomes differ, all intended: a
.yml.bakfile in_data/activity/now fails the data check; the depth-1 guard error names the cause; and a data file replaced by a symlink to/etc/hostsnow reads as invalid YAML rather than "not a list" (both fail). The local review's cases now fail the data check, each naming the file: an append that starts a second document, a.jsondata file, a.yml.jsonfile, a directory named like a data file, and a changed entry in a file with a non-ASCII name. Removing the second of five entries reports only that entry, and a swap reports the reordering.Writers using the emitter settings in QuantEcon/reports-activity#91 (
yaml.safe_dump(..., sort_keys=False, allow_unicode=True)), in the same harness: appending the dump of the new entries after the file's last byte passes both checks. Re-dumping the whole file fails the guard, since the header comment is gone, though the data check passes it. Appending withexplicit_start=Truepasses the guard and fails the data check (two documents).shellcheck 0.11.0 on the guard's script, extracted from
build.yml, and actionlint 1.7.7 onbuild.yml(with shellcheck): no findings.Mutation check (scratch copies only): each of 28 deliberate breaks made at least one test fail. Sixteen were first run before the local review's fixes, and re-run on the final code: the prefix test always passing; any path allowed; deletions allowed; any added mode accepted; no merge-commit check; no escaping; the guard keyed on the login; the permissions block removed; checkout depth 1; no
--base;bookremoved; no duplicate rule; case-sensitive URL keys; no entry comparison; deleted files allowed; an unreadable base only noted. Twelve were added with the fixes: no^and no$in the guard's path pattern;binmodedropped from the base read; no one-document rule; non-YAML files ignored; dotfiles not skipped; base paths left untagged; entries compared by position; no reorder check; no added-before check; the old guard message; and only theexitremoved from the merge-commit check. That last one first survived, because the test checked out a root commit, wheregit diff-tree HEAD^1fails anyway. The test now checks out the PR's own head commit. One more break, restoring the oldFile.file?test for whether a base file still exists, changes no outcome now that base paths are UTF-8, so no test can tell it apart.No visible change: a production build of this branch (282bf28) diffed against a production build of
main(cd6a5e2), with per-build stamps normalised, differs in exactly one file,README.md, which Jekyll publishes as a static file.Alongside #276:
git merge-treemerges this branch with #276's branch without conflicts. On the merged tree this suite passes (38 runs), andbuild.ymlruns checkout, the guard, Setup Ruby, the data check, this suite, #276's "Test Activity rows", then the Jekyll build.In this PR's own
buildthe guard step is skipped, since a person opened it. It first runs for real on the App's own PRs, starting with the deliberately failing PR that #275's test calls for.Choices for review
main, so it could still merge, without approval, a PR someone else opened (including one from a fork) that passesbuild. Only the reporter's own merge step prevents that: it merges the PR it just opened, at the head commit whosebuildit checked. Commits the App might push onto someone else's PR aren't checked either. Adding|| github.event.sender.id == 294005175to the step's condition would check the run that such a push starts, but not a later push by the PR's author, so it was left out. This boundary is worth weighing before the go-live variable is set to merge.build, and an admin can merge past it, which the README now says. A label or path-based escape hatch was left out._data/activity/holds only YAML files. The check fails on any other file or directory there, including ones Jekyll ignores, such as aREADME.mdor a.bakfile, rather than only on what Jekyll reads (.json,.csv,.tsvand subdirectories). Names that start with a dot are skipped, as Jekyll skips them, so a stray.DS_Storedoesn't fail a local run.quantecon/quantecon.pyandQuantEcon/QuantEcon.pycount as one release. Tags stay case-sensitive..ymlor.yamlname, so a person can add, say, a hand-made backfill file with a suffix.--baseis an explicit argument, not an environment variable. An empty or missing value fails instead of silently skipping the comparison.main. A base file with more than one YAML document is compared by its first, the part Jekyll showed. Every other unreadable base fails.build, rather than kept as a scratch check. That keeps the tested script identical to the deployed one, and exercises it under the runner's bash 5 on every PR (about 12 seconds locally).Notes
/activity/has noqe-badge--bookstyle yet, so abookentry would show an uncoloured badge until Restyle /activity/ with the shared rows, a type filter and day links #277 restyles the page. No book entries exist yet.build.yml, and git merges the two branches cleanly.buildisn't strict, so whichever merges second has so far been tested without the other's step: update its branch once the first has merged, so itsbuildruns both. This PR leaves.github/copilot-instructions.mdto Compute the Activity rows at build time, with tests #276. Its line calling the data check "the one automated check" is now out of date.buildisn't strict. Each PR is checked againstmainas of its test merge. If two PRs each pass alone but together list the same release, the duplicate shows up on the next PR'sbuild._data/activity/2026-09-28-software.ymlkeeps its name: nothing was released or published on 2026-09-28 UTC, so no daily run writes to it, and an append-only write would keep its entries anyway.Closes #284.
Part of #271.
🤖 Generated with Claude Code