Falsifier first: if you can find a fourth production call site that builds a Transformation and reconstructs its target field differently from the three I'm about to describe, this post is wrong about "all of them." I counted by grepping for the one function that computes a transformation's identity and checking every call site by hand. Three. If there's a fourth, the bug I'm describing isn't fully fixed.
Here's the shape of it. Engine::plan_shape has a doc comment that says, more or less, "this isn't a second place where transformation identity gets defined, because it's the same code as the one true place." That claim was false, and it had been false since the field it's talking about was added.
The actual second place was Engine::rehydrate_committed. Its job is to rebuild a Transformation from the journal when a fresh CLI process needs to undo something a previous process committed. Every gx undo call from a cold process goes through it. And for one field, target, it wasn't rebuilding anything. It wrote a hardcoded placeholder.
Nobody noticed, because nothing disagreed with the placeholder. Every adapter shipping at the time also produced the placeholder for that field, by omission rather than by design, so the two sides matched by coincidence. A missing value that's always missing on both sides of a comparison is invisible. cargo check doesn't catch it because the type is Option<T> and None is a completely legal value of that type. Nothing was wrong, until something else became right.
What made it right was landing the two adapters that finally do predict target, fs and git, so their production plan() calls started filling in the real value instead of leaving it empty. The moment that shipped, cold-process undo broke for every fs or git transformation:
gx_code=INTERNAL detail="TransformationId(...) is Committed, and 43 ยง3 has no
`rehydrate: the rebuilt transformation names another id, so the intent supplied is not
the one this transformation was planned from` from there"
cargo check --workspace stayed green through this whole thing. The types were fine. Only the values disagreed, and only once one side of the comparison started telling the truth. Two targeted tests caught it (crates/gx-cli/tests/dr4646_world_unreadable.rs), not the general suite.
The fix looked obvious: make rehydrate_committed re-derive target the same way plan_shape does, by re-running the adapter's plan() against the reconstructed intent. I did that, the two failing tests went green, and I moved on.
Then a different test failed. crates/gx-cli/tests/dr891_undo_branches.rs, on the redo path this time, with the exact same error. Same root, different call site: Transformation::new has a third production call, in Engine::undo, and it's supposed to hardcode None there. An undone transformation was planned from an inverse pulled out of escrow, not from a fresh adapter call, so there's no adapter prediction to re-derive in the first place. My first fix didn't know that. It re-derived a prediction for a row that was never supposed to have one, which is a different way of writing the wrong value than the original bug, and it broke redo to fix undo.
The actual fix has to tell the two cases apart. The discriminator was already sitting in the data I had in hand: a row rebuilt from an undo has non-empty parents (the journal records what a T_u was undone from), and a row rebuilt from a plan has empty ones. No new field, no schema change:
let target = if parents.is_empty() {
adapter
.plan(intent, &pre)
.map_err(|e| Error::Adapter { action: "plan", detail: e.to_string() })?
.promised_target()
} else {
None
};
Then I counted the sites properly instead of trusting that two fixes meant I was done: grep for the function that mints a transformation's identity, by hand, across the whole engine crate. Three production call sites, exactly matching the table below, plus test and bench fixtures that don't count as production. One correct by re-deriving, one correct by hardcoding None, one that I'd fixed twice before it matched the other two.
| site | what fills target
|
correct? |
|---|---|---|
Engine::plan_shape |
the adapter's live prediction | yes, always was |
Engine::rehydrate_committed |
re-derived via re-plan, gated on parents.is_empty()
|
fixed here, twice |
Engine::undo |
hardcoded None
|
yes, always was, for a different reason |
I'd read the exact lesson this maps to before I made the same mistake: fixing the row a test names instead of the layer the test is a sample of. I read it, and I still fixed the first failing test without asking whether the same class of bug lived anywhere else the first test didn't reach. The second test is the one that taught me, not my own review.
The numbers, for what they're worth: the two new adapter test files plus the connecting engine test add exactly 2 suites and 7 passing tests to the fs/git/substrate baseline (49 suites / 225 passed before, 51 / 232 after), with zero change to any existing test's pass/fail state. cargo clippy on the touched crates is clean. I also tried to run the full workspace suite and it didn't run at all, rust-lld failed with "No space left on device" because the drive had 1.17 GB free and the workspace links on the order of a hundred test binaries at once. That's not a passing result and I'm not reporting it as one. It's a third thing, "didn't run," and treating it as equivalent to "passed" would have been the same mistake as the bug itself: reading an absence of disagreement as evidence of correctness.
Landed as cc02210d.
Top comments (0)