Repository navigation
refactor(migrate): one step-class entry point; the generator names each carried step's move (architecture pass on #636) - #641
Merged
0x054 merged 3 commits intoOct 12, 2026
Conversation
The generator hands `step_class` the step it lays out (`LaidOutStep`: its phase, its planned ops with their verdicts, the schemas on either side, its rendering's headers) and the module decides the class; the seven exported entry points become one. `index_step_class` folds into the op-kind rule it repeated, and whether a step adds a constraint `NOT VALID` is read inside the module from the step's ops through `staging::stages_not_valid`, the predicate `staged_constraints` now filters by. The apply-time path classifies data and hand-written steps through the same entry point. Behaviour is unchanged. Part of #623 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
…ntry `Carried` names the position of the step it carries in `GenerateOptions.carry` instead of echoing the step's body back through the JSON, beside whether it won over a namesake. `--replace`'s Python side maps that index to the files it read and renders the per-file lines; the re-matching of carried steps to generated steps by name and kind in order is gone. The printed lines are unchanged. Part of #623 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
0x054
commented
Oct 12, 2026
0x054
left a comment
Contributor
Author
There was a problem hiding this comment.
Spec: PASS
Spec: the PR body's Card 1 and Card 4; the invariant is that behaviour does not change. Closes #N is waived because this PR is Part of #623.
Checks run against 0f96c19:
- Schema step. The old
stages_onwas computed over labels+schema ops. It now comes fromstages(SchemaStep{placed.schema}). The two are the same because labels ops are onlyAddEnumLabel(commits_alone, lib.rs:504), andstages_not_validreturns false for those. Sides (before/expanded) and the demanded-column relaxing are unchanged. - Contract.
stages(expand) || !demands.is_empty()is the oldvalidates || !layout.demands.is_empty(), built over the same ops and sides. stages_not_validvs the old filter. It is the old guard ofstaged_constraintslifted out unchanged: Postgres only, the table on both sides, and the same four op kinds plus the riders ofAddColumn. The old staged list was non-empty exactly when some op passes this predicate.staged_constraintsnow filters by it (staging.rs:257, 321).- Index steps.
index_opsonly emitsAddIndex,DropIndexandRedefineIndex(stages.rs:293-358).kind_classgives each the same class as the oldindex_step_class. TheRebuildIndexdifference cannot be reached. - Other step kinds. Labels, constraint/validate (
not_applicable), data, guard and carried-data steps, and hand SQL with or without-- ferro: expandall map one-to-one. - Apply time (F1 rule).
migration_classesstill gates on the checksum and falls back todeclared_class. Only the data arm and the hand arm changed, and both now go throughstep_class. - Tests kept. C1, F1 and the
classes()golden tests are present and pass. - Card 4.
carryis the index into the same list that_carried_stepsbuilds and the generator receives. The files written come from that entry'sbodyanddown, which hold the same bytes the old echo returned. The order-matching code is gone.test_migrate_replace.pypasses unchanged. - Generated bytes. No rendering path changed (the class is never written to a file). Pin (a) in
test_cross_emitter_parity.pypasses.
Cannot verify: none.
Quality: PASS
- Minor · crates/ferro-migrate/src/run_plan/step_class.rs:344 at 0f96c19 · The rule in the
storage_factscomment is now "changing it alone cannot refuse a write". The rule it replaces was "renders no storage". The listed fields satisfy both, but a future editor could read the new rule as permission to drop a field that renders storage. - Minor · src/ferro/migrations/layout.py:31 at 0f96c19 ·
collections.abcis imported afterpathlib, which is out of stdlib order.
Other checks: I-1 holds (the class is decided once, in the core). I-3: the change adds no unwrap. The pub(crate) widening is needed for Demand and stages_not_valid. The four changed tests now assert through step_class or the JSON shape. Nothing outside the five files changed.
Merge previews:
- Against
origin/feat/pinch-migration-needs: no-op. - Against #640 (
dc93200): merges cleanly with no conflicts, includingsrc/migrations_ffi.rs.cargo check --all-targetsexits 0.
Verify: passed on 0f96c19. All six commands exited 0: maturin; cargo check; cargo test -p ferro-migrate 435 passed; cargo test --no-default-features --features testing 354 passed; pytest 943 passed, 13 skipped, 1 xfailed; prek.
Part of #623 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
0x054
marked this pull request as ready for review
October 12, 2026 10:41
1 of 3 tasks
0x054
added a commit
that referenced
this pull request
Oct 12, 2026
…ch carried step's move (architecture pass on #636) (#641) Architecture pass on PR #636, cards 1 and 4; behaviour unchanged. The step class has one entry point: the generator describes each laid-out step and the run planner's module decides, deriving the `constrains` flag from the step's own ops through the same function the staging code uses; the duplicated unique-index rule is gone; the apply-time path classes data and hand steps through the same entry point. `Carried` names which carry entry a step came from, so the Python side maps files by index instead of re-matching steps by name. Part of #623 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
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.
Part of #623 (deepening pass on PR #636; behaviour unchanged)
Two decisions that were spread across callers now each live in one module. Every refusal text, generated file, recorded class and printed line is the same as before.
Card 1: one step-class entry point
Before.
run_plan/step_class.rsexported seven ways to get a class:op_class,step_class,schema_step_class,index_step_class,constraint_step_class,hand_sql_classandDATA_STEP_CLASS. At each step it laid out, the generator chose one of them and worked out theconstrainsflag on its own. It did this through astages_onmap built fromstaged_constraints(...).is_empty(), and for the contract it added|| !layout.demands.is_empty().index_step_classrepeatedop_class'sAddIndex/RedefineIndexrule. The table test calledop_classdirectly, so it would still pass if a call site picked the wrong sibling or passed the wrongconstrains.After. The interface is one function,
step_class(&LaidOutStep) -> StepClass. The generator describes the step it laid out and the module decides the class.LaidOutStephas these variants:LabelsSchema(SchemaStep { ops, sides, dialect })Index { op, target }Constraints { headers }Contract { ops, sides, expand: SchemaStep, demands }DataHandSql { headers }How the old pieces map onto it:
NOT VALID, usingstaging::stages_not_valid.staged_constraintsnow filters by that same predicate, so the class and the staged list cannot disagree.contractwhen the expand it finishes staged a constraint, or when it holds a demanded column to its values.index_step_classbecame the privatekind_class, whichop_classalso uses.migration_classeskeeps its three inputs (regenerated text checksum, file checksum, headers). For a data step and for a hand-written step it now goes throughstep_class.storage_facts. The IR crate has no DDL-bearing projection to derive it from, so it stays a single list. Its comment now states the rule it follows.step_class,LaidOutStep,SchemaStepandSidesare nowpub(crate); nothing outside the crate used them. To let the module nameDemandandstages_not_valid,generate::backfillandgenerate::stagingare nowpub(crate).Interface tests. These now go through
step_class:every_op_kind_has_its_class_under_a_quiet_verdict: every op kind from ADR-0054's table, each alone in a schema step, on both dialects. The verdict overrides, the SQLite rebuild row and the demanded column all stay. A demanded column is now assertedexpandin the schema step andcontractin the contract, which the oldop_classtable could not tell apart.every_op_reads_its_class_off_its_verdict_and_kind: the whole plan carried by one contract step.a_hand_written_sql_step_is_contract_unless_it_says_expand.a_data_step_is_expand,a_constraint_step_is_contract_wherever_it_runs_a_statement,an_index_step_is_contract_only_when_it_builds_a_unique_index, anda_schema_step_staging_a_constraint_not_valid_is_contract_and_so_is_its_contract. The last one is a nullable FK column added to an existing table:contracton Postgres,expandon SQLite, and the same for its contract.The
classes()golden table (with the C1 rowan_added_label_beside_a_retired_one_is_expand), the F1 testa_generated_step_edited_by_hand_is_classed_by_its_file, theunclassifiabletest andtests/test_step_class.pyare unchanged. The only change is that theclasses()helper reads every step's class from the generator, data steps included, instead of usingDATA_STEP_CLASS. No test was deleted: the tests that exercised the removed siblings were rewritten against the entry point.Changed test. One generator test in
generate/mod.rscompared againstDATA_STEP_CLASS, which no longer exists. It now compares againstStepClass::Expand.RED. The rewritten tests came first and failed to compile against the old module: 24 errors, because
LaidOutStepandSchemaStepdid not exist andstep_classtook three arguments. GREEN:cargo test -p ferro-migratewent from 431 to 435 passing.Card 4: the generator names each carried step's move
Before.
Carriedsent the whole carried step (name, kind, body, down) back through the JSON._with_movesthen matched carried steps to generated steps again by(name, kind)in order, which repeated a placementcarried_stephad already decided.After.
Carriedis{ carry: usize, over_namesake: bool }, which serializes as{"carry": N, "over_namesake": …}.carryis the step's position inGenerateOptions.carry. On the Python side:GeneratedMigration.from_generated(..., carry=carry)writes a carried step's files from the carry entry it names.GeneratedStep.carry(the index, orNone) replaces the booleancarried._with_moveslooks upcarry[step.carry]["old"]for the old files and renders the per-file lines. The order-matching code is gone.The FFI result is still a JSON string, so neither
src/migrations_ffi.rsnor_core.pyichanged.Interface tests.
tests/test_migrate_replace.pypins the printed lines (kept, renumbered, "carried, review it", and the namesake line) and passes without changes.Changed tests. Three generator tests asserted
Carried.step == options.carry[i], the old echoed body. That was testing past the interface, so they now assert the index.a_carried_step_wins_over_…also pins the JSON shape the Python side reads.RED. The updated tests failed to compile (4 ×
no field carry on Carried). GREEN after the change.Verify
Run as written on the committed state (
FERRO_POSTGRES_URLset,UV_FROZEN=1,set -o pipefail):uv run maturin developcargo check --all-targetscargo test -p ferro-migratecargo test --no-default-features --features testinguv run pytest --db-backends=sqlite,postgres tests/test_step_class.py tests/test_migrations_api.py tests/test_migrate_up.py tests/test_migrate_status.py tests/test_migrate_rerecord.py tests/test_migrate_baseline.py tests/test_migrate_down.py tests/test_migrate_replace.py tests/test_migrate_new.py tests/test_migrate_check.py tests/test_generate_columns.py tests/test_cross_emitter_parity.py tests/test_docs_migrations.pyuv run prek run --all-filesOutside Files owned
None. This PR does not touch
src/migrations_ffi.rs,_core.pyi,runner.pyor any of the other worker's files.🤖 Generated with Claude Code
https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR