Give builders their own directory, one per published dataset (re-land of #60) - #61
Merged
Conversation
* Give builders their own directory, one per published dataset scripts/ was two unrelated things sharing a name: seven builders that produce published datasets, and the dashboard/catalog toolchain that has nothing to do with data production. Its own README already apologised for the split with a "Not builders" section — while listing 1 of the 7. builders/<stem>.<ext> now builds lectures/<stem>.<ext2>. That is not a new convention: six of the seven already matched their output stem exactly, each with an explicit OUT_FILE = '<stem>.csv'. The move makes a latent pattern enforceable. Where one builder produces a SET of files, it is named for the set and several manifests point at the same path — business_cycle.py writes three, and both incoming notebook builders write two. So the stem rule is the default, not an invariant, and what CI asserts is weaker and truer: * builder_status must be a known value * a `committed*` status must name a builder * a named builder must exist on disk None of that was checked anywhere before — build_audit only records the value and build_catalog only formats it, so a manifest could assert a builder that was never committed, or one that had been moved. Which is exactly what this commit does to six of them. Adds `committed-frozen` to the enum: the builder is here and deliberately will not run, for a dataset built from a source that must not be refreshed. `committed` asserts a runnable four-stage builder and is a false claim for a frozen vintage; `unrecovered` says the builder is absent and is a false claim for one arriving in the same PR. This is the answer to #14's notebook question, and it lets the two high_dim_data builders land as what they are. Also records where a builder reads its input from, since sources/ is about to exist and is easy to misread: the normal case is the third-party upstream at run time — six of seven do that — and sources/ is only for an input that cannot be re-fetched. It is not a general input tree and not "the big-file directory"; the defining property is un-refetchability. Safe to do now, and cheaper now than later: no workflow runs any builder, nothing outside the repo references the paths, and PLAN's warning that the restructure window is spent applies to lectures/, where filenames are the public API — not to scripts/, which is never served. Doing it before the high_dim_data fold means its two builders land in the final shape. Verified: real tree 18/18 hash-checked exit 0 (which also proves the six moved paths resolve), strict audit exit 0, CATALOG.md regenerates byte-identical. Six negative cases exercised — missing builder path, a committed status with no builder, an unknown status, committed-frozen, and the verbatim/not-applicable shape that must keep passing. Part of #14. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Guardrail: type-check the builder fields, and require an in-repo file Copilot review on #60, confirmed by reproduction — the builder checks had four holes, two of which crashed rather than reported. A non-string `builder_status` (a YAML list, say) raised `TypeError: unhashable type` on the set-membership test, and a non-string `builder` raised on `REPO / builder`. Both surfaced as a traceback with no `::error::` line — the same class of defect the review caught in #56, in a file that already handles it that way for `integrity` and for the manifest itself. The path check was satisfiable by things that are not builders in this repo. `REPO / builder` silently discards REPO when `builder` is absolute, so `/etc/hosts` passed; a relative path can climb out with `../`, so a real file outside the repo passed; and `.exists()` is true for directories, so naming `builders` passed. That last one the review did not name. Resolving and asserting `is_relative_to(REPO)` plus `is_file()` closes all three. Framing note: the read is correctness rather than security — the check only calls `.exists()`, never reads — but an assertion satisfiable by a file that is not a builder here establishes nothing, and the likely trigger is a typo'd relative path that happens to resolve on the runner, not an attack. Verified: real tree 18/18 exit 0; the six cases from the PR body unchanged; five new ones — list status, list builder, escaping relative path, absolute path, directory — all now fail cleanly with no traceback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
force-pushed
the
builders/land-on-main
branch
from
August 10, 2026 02:09
bb79de7 to
f8e506f
Compare
There was a problem hiding this comment.
Pull request overview
This pull request re-lands the “builders live in builders/” restructure by moving dataset builder scripts out of scripts/, updating manifests/schema/docs to point at the new locations, and tightening PR guardrails to validate builder metadata in manifests.
Changes:
- Introduces
builders/as the dedicated home for per-dataset builder scripts and updates repository documentation to reflect the split between builders vs repo tooling. - Updates lecture manifests and
manifest-schema.ymlsobuilder:paths referencebuilders/..., and addscommitted-frozento the documentedbuilder_statusenum. - Extends
.github/scripts/check_consumed_files.pyto validatebuilder_statusvalues and ensure any namedbuilderresolves to a file inside the repo.
Reviewed changes
Copilot reviewed 12 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/README.md | Re-documents scripts/ as repo tooling and points builders to builders/. |
| scripts/audit_annotations.yml | Corrects the graph.txt description. |
| README.md | Updates the “Referencing data” section with runtime-specific URL forms and tightens the “never media host” guidance. |
| PLAN.md | Updates references from scripts/... to builders/... and refreshes roadmap/status text accordingly. |
| manifest-schema.yml | Updates the example builder: path and documents committed-frozen in builder_status. |
| AGENTS.md | Adds committed-frozen, clarifies URL-form rules, and documents the builders layout/contract. |
| lectures/us_adult_heights.csv.yml | Updates builder: to builders/us_adult_heights.py. |
| lectures/japan_population_by_age.csv.yml | Updates builder: to builders/japan_population_by_age.py. |
| lectures/japan_earthquakes.csv.yml | Updates builder: to builders/japan_earthquakes.py. |
| lectures/japan_deaths_by_age.csv.yml | Updates builder: to builders/japan_deaths_by_age.py. |
| lectures/epl_match_goals.csv.yml | Updates builder: to builders/epl_match_goals.py. |
| lectures/ames_house_prices.csv.yml | Updates builder: to builders/ames_house_prices.py. |
| builders/README.md | New documentation for the builders directory, naming rule, and builder contract. |
| builders/business_cycle.py | Moves the (manual) business cycle builder into builders/. |
| builders/us_adult_heights.py | Builder script located under builders/. |
| builders/japan_population_by_age.py | Builder script located under builders/. |
| builders/japan_earthquakes.py | Builder script located under builders/. |
| builders/japan_deaths_by_age.py | Builder script located under builders/. |
| builders/epl_match_goals.py | Builder script located under builders/. |
| builders/ames_house_prices.py | Builder script located under builders/. |
| .github/scripts/check_consumed_files.py | Adds builder-status/path consistency checks to the consumed-file guardrail. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Aug 10, 2026
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
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.
#60 merged into its own base branch instead of
main, so none of it is onmain. This re-lands it, unchanged.What happened
#59 squash-merged to
mainat 02:03:51Z. #60 merged 12 seconds later, at 02:04:03Z — intodocs/storage-position-and-url-rule, the branch it was stacked on, which had just become stale. GitHub retargets a stacked PR when its base merges, but only once it notices; #60 went in first.The squash is what makes it invisible:
mainhas #59's changes but not its commits, sodocs/storage-position-and-url-rulestill looked like a live branch with three commits, and merging into it succeeded and reported success. Nothing was lost — the work is all inbb79de7on that branch — butmainhas nobuilders/, nocommitted-frozen, no builder assertions, and not the Copilot fix.Why this diff is exactly #60 and nothing else
git diff --stat origin/main origin/docs/storage-position-and-url-ruleandgit show --stat bb79de7are byte-identical — 19 files, +144/-36.mainalready carries #59's changes via the squash, so the only tree difference between the two is #60's change set. No cherry-pick, no re-authoring, no drift.Contents (unchanged from #60)
builders/<stem>.<ext>buildslectures/<stem>.<ext2>. Seven builders move out ofscripts/, which keeps only repo tooling. Six of the seven already matched their output stem, so this makes a latent convention enforceable.Three assertions added to the PR guardrail, none of which existed: the status must be a known value, a
committed*status must name a builder, and a named builder must resolve to a file inside the repo.committed-frozenjoins the enum — the builder is here and deliberately will not run — which is the answer to #14's notebook question.Includes the Copilot fix from #60 (
d2c02bd): both builder fields are type-checked, because a YAML list raised an uncaughtTypeErroron each; and the path check resolves and assertsis_relative_to(REPO)plusis_file(), closing three ways through it — an absolute path, a../escape, and a directory.Verified on this branch: real tree 18/18 hash-checked exit 0, strict audit exit 0, eleven guardrail cases behaving.
🤖 Generated with Claude Code