Repository navigation
promote: differing same-name copies are an error, jax is considered, and a consumer series class (project-monorepo#74) - #14
Conversation
…switch tools/promote no longer settles a same-name collision by run order. A copy whose bytes differ from the anchor copy, even after its asset references are normalised to their pool paths, is an error that names the lecture and every series holding a copy. The one exception is the committed canonical map, sync/canonical.yml, which names the winning series per slug; the losing copies are then recorded under divergent and the entry is interim, as before. Byte-identical copies keep the existing behaviour. ALWAYS_EXCLUDED is gone, so jax takes part in dedup by default and --exclude remains the per-run way to leave a series out. A manifest stanza may now carry class: consumer. A consumer is never canonical, contributes no lecture to the pool, and its copies are recorded under a new superseded field. tools/drift-check reads the class and never watches a consumer's copies, including those an older ledger lists as sources or divergent. promote --switch SERIES sets canonical: pool, with a switched_at stamp of the series pin and the date, on every ledger entry the series is canonical for, and on each asset once every lecture using it has switched. It refuses an unknown or consumer series and any series with a refresh pending, and it is idempotent. The drift check then only confirms the pool file exists, refresh passes the entry over, and promotion refuses to overwrite it. The canonical map is seeded with the 24 entries that were interim in the ledger, keeping each canonical choice already made, so the pool and the ledger body regenerate unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…quests tools/tests/ builds small scratch repositories (a mirror, a manifest, a state file and an empty pool in a temp dir) and runs copies of the real tools against them as subprocesses. The 23 tests cover the same-name collision error and the series it names, the canonical map, jax being considered by default, copies that differ only in asset path spelling, the consumer class and the drift check ignoring its copies, the switch and its idempotence, and a byte-stable ledger write. The tests workflow runs them with uv on pull requests and manual dispatch only, never on push. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ders A canonical map entry now chooses which copy wins a collision, and the canonical series is still the highest-priority series holding that copy, as for any set of identical copies. An entry that settles nothing therefore changes nothing, and deleting it after a rename leaves the ledger as it was. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…refresh dp's manifest stanza now carries class: consumer, as the plan has it. The ledger is unaffected: the CI refresh still excludes dp while dp-test stands in for it, and a refresh that does consider dp only adds superseded records for dp's copies, leaving every other field as it was. jax was excluded inside tools/promote until now. The drift-check workflow's refresh keeps that posture by adding jax to PROMOTE_EXCLUDE, because two pool lectures, ifp_egm and short_path, share a name with a different jax lecture and would otherwise fail the refresh until jax's renames land. The README's layout lists the canonical map, the tests and the tests workflow, and its drift-check section says which copies are never watched and how promote handles a same-name collision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…entries Written by tools/promote itself (a no-op re-promotion of networks with dp and jax excluded, as the CI refresh runs), so the committed ledger matches what the tool now writes and the next refresh starts from a zero diff. Only the comment header changes; every entry is byte-identical. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A map entry covers a slug, not a pair of series, so the entry seeded for the dp-test collision also picks intermediate's ifp_egm over jax's different lecture of that name whenever jax takes part. The CI refresh excludes jax, so nothing changes today; the note keeps that from being a surprise. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a large, intricate change to core provenance tooling with subtle byte-stability and collision-resolution invariants plus operational CI impact (the daily refresh can now go red), which warrants final human review despite the extensive test coverage.
Review effort: Balanced
Findings: None
What changed in this PR
This PR reworks the tools/promote and tools/drift-check tooling that maintains the lecture pool and its provenance ledger, implementing the first work item of QuantEcon/project-monorepo#74. It changes how same-name collisions are resolved, how the jax series is treated, introduces a consumer series class, and adds a --switch cutover mode — plus a new pytest suite and CI job.
Changes:
- A same-name copy whose bytes differ (after asset-path normalisation) is now a hard error naming every series involved, resolvable only via a committed
sync/canonical.ymlmap; run order never decides a winner.ALWAYS_EXCLUDEDis removed sojaxparticipates like any series. - A
class: consumerseries (declared fordp) is never canonical, records copies under a newsuperseded:field, and is never watched by the drift-check;promote --switch SERIESstampscanonical: pool/switched_at:on a series' entries so the pool copy becomes source of truth. - Adds
tools/tests/(24 scratch-repo pytest tests) with atests.ymlPR workflow, and switches the daily refresh to--exclude dp jax.
| File | Description |
|---|---|
tools/promote |
Core logic: load_manifest/load_canonical_map, collision handling in resolve_sources, normalised_text, switch_series, superseded ledger field, CLI --switch. |
tools/drift-check |
Skips canonical: pool and consumer-series copies in series_in_ledger/check_lecture/check_asset. |
sync/manifest.yml |
Documents class: and marks dp as consumer. |
sync/canonical.yml |
New map seeded with 24 previously-interim dp-test collisions. |
sync/ledger.yml |
Header comments updated for superseded/switched_at. |
README.md |
Documents consumer/switch/canonical-map behaviour and the new test suite. |
.github/workflows/tests.yml |
New PR/dispatch job running tools/tests via uv. |
.github/workflows/drift-check.yml |
PROMOTE_EXCLUDE: dp jax. |
tools/tests/*.py |
Scratch-repo harness and tests for collisions, consumer class, and switch. |
I reviewed the collision-resolution logic (anchor selection, normalised_text equivalence, divergent/superseded recording), the --switch flow (stale-pin refusal, idempotency, asset-switching gated on all referencing lectures being switched), the overwrite-guard (blocked) interaction with switched assets, and cross-checked each test's assertions against the code paths. I did not find concrete defects, and the tests align with the implemented behaviour.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The refresh step let a non-zero exit from tools/promote fail the step, so the pull-request step never ran and one entry that promote refused (a same-name copy that came to differ) withheld every other refreshed entry. That is the normal course of events in the live estate: a drifted non-canonical copy, then the canonical copy moving. The step now keeps promote's exit status and its output. The pull request is opened with whatever promote did refresh and lists the FAILED lines under "Not refreshed", with the two remedies (a sync/canonical.yml entry or an upstream fix). A last step then turns the run red while any entry failed, so the collision stays visible until it is resolved. The README's refresh outcome says the same. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0155xQ4bHh1dJP2Tt8da7FAN
A series' toc still lists its lectures after the switch, so `promote --toc` on a switched series failed every one of them. They are now skipped, as product pages are; a switched lecture named explicitly is still refused. The same-name collision error now says which copies agree with the anchor and which differ, instead of one undifferentiated list of digests. The manifest documents `class: source` as the explicit spelling of the default, and the dp-test stanza no longer claims the series is excluded from canonical-home consideration: it is canonical-eligible, and sync/canonical.yml settles its collisions until it is retired. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0155xQ4bHh1dJP2Tt8da7FAN
quantecon-services
left a comment
There was a problem hiding this comment.
Review summary
Read every changed file in full and ran the suite plus three extra scenarios against the branch's tools. The design holds: collisions are order-independent, the map picks a copy while priority picks the holder, consumers are handled consistently across promote, drift-check and the ledger, and the switch refuses stale pins and is idempotent. No blocking defects.
Addressed on this branch (two follow-on commits):
c052dccdrift-check.yml: a refresh entry that promote refuses no longer withholds the refresh PR. The PR is opened with whatever did refresh, lists the failures under "Not refreshed", and the run stays red. Without this, the first drifted non-canonical copy whose canonical then moved would have blocked every other refresh.c23890cpromote: switched lectures met via--tocare skipped rather than failed; the collision error groups copies into same/different; manifest comments updated (class: source, dp-test stanza). One test added, 25 pass.
Filed as follow-ups:
- #15: drift-check still compares non-canonical sources byte-for-byte, so a normalised-equal source moving in lockstep with its canonical copy raises a false
divergedfinding. Latent today. - #16: a new lecture sharing a single-consumer asset with a switched lecture cannot be promoted, and the error's remedy is impossible after the switch. Mid-transition only.
Not reproduced here: the zero-diff ledger regeneration and the 297-name scratch run, both of which need the reconstructed mirror.
Generated by Claude Code
promote writes the pool lectures before the ledger, and since c052dcc the workflow commits whatever is on disk even when promote exits non-zero. A crash between the two writes would have put a half-written pool into the refresh pull request. A failing promote prints its summary only after every write, so a non-zero exit without the summary line now stops the job before anything is staged. The comments and the PR body's "Not refreshed" text now say a refused copy was edited to differ; a stale one no longer fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
resolve_sources compared every eligible series' current copy with the anchor, so a copy that had simply not moved while the canonical copy did was refused as a same-name collision. That is the common case, not an edge: the 2026-09-07 refresh (#9) re-copied 14 lectures and in all 14 the dp-test copy was unchanged since promotion. Under the collision rule every one would have failed, and the 25 pool entries still held identically by a source series and dp-test would each turn the refresh red on their next upstream edit. The drift-check already calls this state `stale-copy` (info). When the anchor is the entry's recorded canonical series, a differing copy recorded under `sources` whose digest equals its own recorded digest (it has not moved) or the canonical copy's recorded digest (it caught up, then the canonical copy moved on) is now stale: it stays a source, the refresh goes through, and the output names it. A copy edited to anything else is still refused. The drift-check gets the matching rule for the caught-up case, which it reported as `diverged`. Three tests: the stale refresh (and a clean drift-check after it), an edited copy still failing, and the caught-up-then-behind case. 28 pass. The ledger header documents stale sources and is regenerated to match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up: a stale copy was refused as a collisionThe finding.
I reproduced it with this PR's own harness before changing anything: two series, one identical lecture, the canonical copy edited, Pushed to this branch:
Tests. Three new tests: a stale copy refreshes cleanly (and the drift-check is clean afterwards), an edited copy still fails, and the caught-up-then-behind case. 28 pass locally and in CI; the stale and caught-up tests fail on c23890c. Unchanged on purpose. The 24 Generated by Claude Code |
Since the collision rework, tools/promote admits a same-name copy as a source when it is identical to the canonical copy once each copy's asset references are normalised to their pool paths, and records it with its own digest. The drift-check still compared such a copy byte-for-byte, so the same upstream edit landing in both series raised a false `diverged` drift finding, with a detail text claiming the copy had been byte-identical. Non-canonical sources and divergent copies are now compared by the rule that admitted them: byte-identical, or equal under promote's normalised_text, which the drift-check imports from tools/promote rather than re-implementing, so the two tools cannot disagree about which copies are one lecture. A converged copy says whether it matches exactly or up to asset path spelling, and the diverged detail describes the rule. Four tests cover the lockstep edit, a spelling-only change, a real edit (still drift), and a divergent copy converging up to spelling. Closes #15. Claude-Session: https://claude.ai/code/session_0155xQ4bHh1dJP2Tt8da7FAN Co-authored-by: Claude <noreply@anthropic.com>
The adversarial review of this pull request (2026-10-08) confirmed twelve defects in --switch, six of them major: it leaves the switched series named in guarded entries and assets, so its stanza cannot be turned off; a shared asset with a switched co-user deadlocks the refresh once its bytes change; a refresh can prune a figure an edited switched lecture uses; a lagging mirror satisfies its pin check; --toc of another series silently skips a different lecture under a switched name; and other series' copies stop being watched. None can bite before the first cutover, while the rest of this pull request is needed sooner, so the switch moves to its own work item and lands, fixed, before the first cutover. Removed: the --switch flag and switch_series, the canonical: pool handling in promote (refresh, --toc, explicit slugs, asset records) and drift-check, the reserved 'pool' series name, test_switch.py and the docs. The reviewed implementation is kept on the branch promote/switch (7128fb1). The ledger header is regenerated; its body re-dumps byte for byte. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
refresh_targets skipped every entry whose canonical series the run could not use, excluded or a consumer, whatever its pin. With jax now considered by default, a local run makes jax canonical for jax-only lectures; CI then refreshes with --exclude dp jax, so each of those entries was a SKIP, promote exited 0, the workflow logged "changed nothing" and the run stayed green while drift-check reported the canonical copy moved every night. The pool copy silently stopped following upstream. Such an entry is still skipped while its pin stands, and now fails once the pin has moved, naming the exclusion and where CI sets it, so the run goes red and the refresh pull request lists it. Three tests: excluded and unmoved (skip), excluded and moved (fail, then refresh once considered), and a consumer-canonical entry from an older ledger. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7128fb1 recognised a stale copy only when the run anchored on the entry's recorded canonical series. Two cases slipped through. `promote --toc S`, where S's copy lags, anchored on S and refused the moved canonical copy as "different", even after the nightly refresh had taken it, and the error blamed the up-to-date copy. A canonical.yml line naming a lower-priority holder of identical copies (one left in place after a merge) anchored the refresh on that holder once the canonical copy moved, silently kept its lagging text, flipped the recorded canonical and marked the entry interim. Every copy is now compared with the recorded canonical copy first, and a recorded source that differs from it while still at a version the pool already took (its own recorded digest, or the canonical copy's) is stale whatever anchors the run. A stale requested copy gives way to the canonical copy; a map line naming a stale copy is refused with a message saying it no longer settles a collision. A copy equal to the canonical one once asset paths are normalised is the same lecture, never stale. Five tests: --toc of a lagging series before and after a refresh, the stale map line, a map line over spelling-only copies, a second canonical move, and a copy never recorded as a source. Each half of the rule and the recorded- source guard is now pinned: dropping any of them fails a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ovable load_canonical_map validated only the series side. A repeated key, or `lake_model` beside `lake_model.md`, silently collapsed to the last line, so file order picked the pool copy; a misspelt slug was silently unused, contrary to its docstring. The map is now read with a loader that refuses repeated keys, two keys naming one lecture are an error, and a slug that no series mirror holds is reported on every run (reported rather than refused, so one dead line cannot stop every promotion). The "settles nothing and can be removed" warning looked only at the series in the run, so under CI's --exclude dp jax it called a line that settles a collision with jax removable; deleting it would fail the first run that considers jax. With an excluded holder it now says to keep the entry. Three tests cover the key checks, the unheld slug and the excluded holder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ives promote's reason - The refresh passed `--exclude $PROMOTE_EXCLUDE` unquoted to a flag that takes one or more values, so emptying the list, as its comment directs, made argparse exit 2 on every refresh. The flag is now left off when the list is empty, in the command and in the pull request text. - The PROMOTE_EXCLUDE comment said ifp_egm and short_path would fail with jax considered; only short_path does, as ifp_egm's canonical.yml line records jax's copy as divergent. It now also says jax must leave the list before any jax-only lecture is promoted, since such an entry fails the refresh once its pin moves. - "Not refreshed", the workflow header, the red step and the README blamed every refusal on an edited same-name copy settled by canonical.yml, and promised the next run would refresh it. Refusals have other causes with other remedies, so the text now points at promote's reason on each line, and says a refused entry is retried when the refresh job next runs. - The guard for a run that stopped before its summary now names the likely cause, a configuration error, before a crash. - Guidance that showed a bare `promote --refresh` now names CI's exclusions; the ledger header no longer calls a normalised-equal source stale; the drift-check docstring's drift example is dp-test, dp being a consumer; README: the workflow also goes red on a refused refresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…for any anchor in step From an adversarial check of the previous five commits (2026-10-08). None changed pool or ledger data on the real estate; these make the messages and two edge cases right. - The stale exemption applied only when the run anchored on the recorded canonical series. It now applies whenever the anchor's copy is that series' lecture: `--toc` of a holder in step with the canonical copy, or a canonical.yml line naming one, no longer refuses or marks interim a third copy that is merely stale. - A refusal of a canonical.yml line naming a stale copy offers pointing the line at the recorded canonical series, and says "has moved on" only when that copy has. - A refresh entry whose canonical series has left sync/manifest.yml is no longer called "excluded"; a consumer-canonical entry is told to re-home or retire; a missing pin prints as none. - A canonical copy gone upstream is named as such, pointing at the drift-check's canonical-removed issue, instead of "not found in mirror/". - "Settles nothing and can be removed" is given only when every canonical- eligible series was checked. An excluded copy is compared before the line is called needed, and an excluded series with no mirror on disk is reported as unchecked. When the line names a lower-priority holder, the warning says to remove it now, since it is refused once that copy falls behind. - A map slug held only by a consumer, or in the wrong case, is reported. - The canonical.yml loader resolves merge keys and reports an unhashable key as a parse error, as safe_load did. Tests: 16 new, including the pool-edit violation and the _shared placement checks that test_switch.py used to carry, and tests that each pin a mutant the suite let through. 51 pass; 12 fail on the previous commit, and each targeted mutant fails its test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ttled 3d0fe00 said a refused entry is retried "when a canonical copy next moves". That holds only for an entry refused when just its series' pin moved (#20). A refusal leaves the ledger record as it was, so while the entry's canonical copy differs from it the drift-check asks for a refresh every night and the run stays red until the entry is settled; a settlement applies the next night. The workflow header, the "Not refreshed" note and the README now say so, and name the pin-only exception. - The texts now say a canonical.yml line naming a lagging copy is refused, and that a line should be deleted once promote reports it settles nothing. The canonical.yml header says the same. - "Not refreshed" no longer promises a remedy in the FAILED line for a canonical copy removed upstream; it points at the canonical-removed issue. - README: a refused entry is listed in the pull request when one is opened, and always in the red run's refresh step. - The refresh step's status line tells refused entries apart from a run that stopped before its summary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Update after the adversarial review (2026-10-08) and a check of the fixes (2026-10-09)What changed on this branch since my last comment
Checked on real data (the mirror at the pinned SHAs):
Filed from the review: #18, #19, #20, #21, #22, #23 and #24. None of them blocks this pull request. The description above is updated to match. Generated by Claude Code |
Implements the promote-tool changes of QuantEcon/project-monorepo#74, the first work item of the plan of record, except the switch operation, which moved to its own work item (QuantEcon/project-monorepo#104) after the adversarial review of 2026-10-08. Includes #17 (merged into this branch), which closes #15.
What changes
sync/canonical.yml, settles it, by naming the winning copy; manifest priority still chooses among byte-identical holders, and run order never decides. The map is seeded with the 24 entries that wereinterimtoday, alldp-testcollisions; they go when thedp-teststanza is removed (QuantEcon/project-monorepo#24).--tocof the lagging series, or an explicit promotion. A map line naming such a stale copy is refused, since it no longer settles a collision. The drift-check calls the same statestale-copy(info), and judges a non-canonical copy by the rule promotion admitted it under (drift-check: judge a non-canonical copy by promotion's own rule #17).jaxis considered like any series:ALWAYS_EXCLUDEDis removed and--excludestays a per-run flag.class: consumerseries in the manifest is never canonical and contributes no lecture; its copies are recorded under a newsuperseded:field and the drift-check never watches them.dpis declared one.tools/tests/: 51 pytest tests on scratch repositories, run by a newtests.ymlon pull requests and manual dispatch.CI changes to note
--exclude $PROMOTE_EXCLUDE, nowdp jax(wasdp): with jax considered,short_path(intro against jax) would otherwise fail today. Dropjaxwhen its renames land (QuantEcon/project-monorepo#76); an empty list now works.Proof
dp-testexcluded, jax considered): the same 16 named collisions as before,about,arellano,autodiff,ifp_egm,inventory_dynamics,jax_intro,kesten_processes,lake_model,lln_clt,lucas_model,markov_asset,mle,pandas_panel,schelling,short_pathandwealth_dynamics, and nothing else. In one batch with a map for them, all 297 promote, a repeat run gives a byte-identical ledger, and the drift-check is clean. (Run series by series,rob_markov_perfalso needs its shared asset re-homed in the same batch, the hardening deferred to QuantEcon/project-monorepo#11.)promote --refresh --exclude dp jaxupdates 41promoted_atpins and nothing else, the pool stays byte-identical, a second run has nothing to do, and the drift-check stays clean.Review. An adversarial review on 2026-10-08 (three rounds, 17 reviewers, every finding checked by three independent verifiers) confirmed 39 findings. This branch addresses the ones that belong here; the switch findings moved with the switch, and the rest are filed as #18 to #23. A second adversarial check of the fix commits (2026-10-09; 51 agents) confirmed 18 minor findings: 17 are fixed here, and one is filed as #24.
Opening or updating this pull request runs two jobs:
drift-check(report-only on pull requests) and the test job. Neither writes anywhere.🤖 Generated with Claude Code