Repository navigation
drift-check: judge a non-canonical copy by promotion's own rule - #17
Merged
Merged
Conversation
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. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0155xQ4bHh1dJP2Tt8da7FAN
There was a problem hiding this comment.
🔵 Needs a closer look
It alters drift-detection logic in critical monorepo sync/CI tooling (via dynamic module loading) where a false "converged" result could mask a genuine fork, warranting final human review.
0 open findings
What changed in this PR
This PR fixes a false-positive drift finding (issue #15) in the QuantEcon monorepo sync tooling. Since PR #14, tools/promote admits a same-name copy as a source when it is identical to the canonical copy after each copy's asset references are normalised to their pool paths (/_static/... vs _static/...), recording it with its own digest. However, tools/drift-check still compared such copies byte-for-byte, so an upstream edit landing in both series raised a false diverged drift finding. This change makes drift-check judge a moved non-canonical copy by the same rule promotion admitted it under, by importing promote's own normalised_text rather than re-implementing the asset parser.
Changes:
- Added
promote_module()(lazy, single import oftools/promoteviaSourceFileLoader) andsame_lecture(), which treats a copy as the same lecture when byte-identical or equal undernormalised_text. - Updated both non-canonical comparison call sites (
sourcesanddivergent) so a converged finding reports whether the match is exact or "up to asset path spelling" and carriesexactin its data; thedivergeddetail no longer asserts byte identity at promotion. - Added four scratch-repo tests covering lockstep edits, spelling-only changes, genuine divergence, and a divergent interim copy converging up to spelling.
| File | Description |
|---|---|
tools/drift-check |
Adds lazy import of promote's normalised_text and same_lecture(); updates converged/diverged findings for the normalisation rule. |
tools/tests/test_drift_check_normalised.py |
New tests validating the normalised-equal comparison across lockstep, spelling-only, genuine-edit, and interim-converge scenarios. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
This was referenced Oct 8, 2026
mmcky
added a commit
that referenced
this pull request
Oct 8, 2026
…a consumer series class, and --switch (project-monorepo#74) (#14) * promote: refuse differing same-name copies, consumer series, and the 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> * tests: pytest suite for promote and drift-check, run in CI on pull requests 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> * promote: the canonical map picks a copy, priority picks among its holders 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> * sync: declare dp a consumer series; exclude jax explicitly in the CI 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> * ledger: regenerate the header that documents superseded and switched 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> * canonical map: note that the ifp_egm entry also settles jax's ifp_egm 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> * drift-check.yml: a failed refresh entry no longer blocks the refresh PR 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 * promote: skip switched lectures under --toc; group the collision message 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 * drift-check.yml: open no refresh PR when promote stopped part-way 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> * promote: a stale copy stays a source; only an edited copy is a collision 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> * drift-check: judge a non-canonical copy by promotion's own rule (#17) 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> * promote: take --switch out of this pull request 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> * promote: a refresh fails an entry it cannot use once its pin has moved 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> * promote: judge a stale copy against the recorded canonical copy 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> * promote: validate canonical.yml keys, and never call a live entry removable 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> * drift-check.yml, docs: an empty PROMOTE_EXCLUDE works; refusal text gives 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> * promote: refusals and warnings that say what is true; the stale rule 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> * drift-check.yml, docs: a refused entry stays red every night until settled 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> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: QuantEcon <quantecon-services@users.noreply.github.com>
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.
Fixes #15. Stacked on #14 (base is
promote/74-estate), because the normalisation it relies on only exists there; GitHub retargets this tomainwhen #14 merges.The problem
Since #14,
tools/promoteadmits a same-name copy as asourcewhen it is identical to the canonical copy once each copy's asset references are normalised to their pool paths (/_static/...against_static/...), and records it with its own digest.tools/drift-checkstill compared such a copy byte-for-byte. So the same upstream edit landing in both series raised a falsedivergeddrift finding, and its detail text claimed the copy had been byte-identical at promotion. Latent today, since every source in the committed ledger is byte-identical; live as soon asjaxleavesPROMOTE_EXCLUDEor an intro/intermediate pair that differs only in spelling is promoted.The change
normalised_text.tools/promote(lazily, on the first copy whose digest moved) instead of re-implementing the asset parser, so the two tools cannot disagree about which copies are one lecture.convergedfinding says whether the copy matches exactly or up to asset path spelling, and carriesexactin its data. Thedivergeddetail now describes the rule rather than asserting byte identity.Tests
Four new tests in
tools/tests/test_drift_check_normalised.py: a lockstep edit (refresh only, no drift), a spelling-only change in the non-canonical copy (clean), a real edit (still drift), and a divergent copy converging up to spelling (converged, re-promotable). The full suite passes, 29 tests.Not changed
The ledger format, promote, and the clean path of the drift-check: a copy whose digest has not moved is never parsed.
🤖 Generated with Claude Code
https://claude.ai/code/session_0155xQ4bHh1dJP2Tt8da7FAN
Generated by Claude Code