Differencing phase 14: composition rollout across the chain-walker operations¶
Prompt¶
Plan phase 14 of PLAN-differencing.md: turn the phase 4 refusal
into composition for the operations that already read through the
guest chain walker, so that instar convert, dd, compare,
bench and rebase read a differencing VHD or VHDX against its
parent instead of declining the source by name.
Phases 11 to 13 built the machinery and none of it is reachable from
a user command. The host resolves a chain (c66f8b2, #603), the
guest composes a differencing VHD block (b4ae7fc, #615) and a
differencing VHDX chunk (d677823c, #624) -- and every read entry
point still refuses such a source before any of that runs. This is
the phase that changes what a user sees, which is why the master
plan gives it its own review rather than tacking it onto a guest
phase.
The phase plan is the deliverable; implementation is a separate ask.
Planning effort¶
High. The diff is mostly deletions of if statements, which is
exactly what makes it dangerous: the phase's real content is
deciding when each deletion is safe, and three of the four
preconditions were discovered rather than planned. Two are filed
issues this phase must settle before it deletes anything (#623,
614), one is a reachability change this phase creates (#625), and¶
the fourth is a host-side gate whose 40-line justifying comment stops being true halfway through the phase.
A wrong answer here is silently wrong user data from a command that previously failed loudly, which is a strictly worse failure than the refusal it replaces.
Review effort: high. The master plan does not specify one for this phase.
Scope¶
In scope.
- Issue #623:
vhdx::calculate_bat_layoutsizes a differencing VHDX BAT by the dynamic rule, omitting the chunk-group padding. A gate, per the master plan. - Issue #614:
init_chain_statescannot tell chain boundaries apart from its device count. A gate, per the master plan. - Issue #625: the VHDX chain walker accepts a block offset that overlaps the BAT or metadata region. In scope here because lifting the refusal is the change that makes it reachable from a user command -- see decision 3.
- The host-side composability gate at
src/vmm/src/main.rs:3047, which today refuses to resolve a differencing parent for any composing caller. - Lifting the
init_chain_statesrefusals (src/crates/qcow2/src/lib.rs:13501for VHD,:13529for VHDX) for the operations that read through the chain walker:convert,compare,bench,rebase, andddby inheritance. - Python integration tests proving each of those operations reads a real differencing chain correctly, and that the operations this phase does not cover still refuse.
- Removing the
PLAN-differencing.mdcitation from the user-visible refusal message (src/vmm/src/main.rs:852). docs/andCHANGELOG.mdfor the behaviour change.
Out of scope.
map,measureandcheck. The master plan lists all three as phase 14 work; the survey found they do not use the chain walker at all and composing in them means building chain support rather than lifting a refusal -- see F1 and decision 1. They become phase 15, and the current phases 15 to 17 renumber to 16 to 18.commit. It is a composing caller on the host (src/vmm/src/main.rs:7265) but its guest op reads through its ownbacking_chain_first/backing_chain_countslots rather thaninit_chain_states, so it is neither refused today nor lifted here. Recorded because the survey found it and a reader will otherwise wonder.- Cross-validation against the phase 1 oracle, and coverage fuzzing of the compose path. That is phase 16 (the current phase 15), including the 4096-byte logical sector gap phase 13 left it.
- Issue #626, raised by 14a and filed rather than acted on: a
claim that instar's non-differencing BAT entry count may
over-declare by one entry against SPEC(VHDX) and qemu's
vhdx_calc_bat_entrieswhenevertotal_blocksis an exact multiple ofchunk_ratio. It is out of scope because it was not measurable from the evidence to hand -- the megabyte rounding of the BAT region makes a writer's intended entry count unobservable from the file it produced -- and because instar over-declares rather than under-declares, which is the safe direction on the writer side. The read side is a looseness rather than a defect, sinceVhdxState::init's bound is additionally capped by the declared region. The issue records the geometry that would settle it. - The ~20 other
PLAN-*.mdcitations in comments acrosssrc/operations/andsrc/shared/. Pre-existing, not created by this phase, and a tree-wide sweep does not belong in a phase that changes read behaviour. File an issue.
What the survey found¶
Surveyed against d677823c on 2026-10-05. The master plan's phase
14 bullet (docs/plans/PLAN-differencing.md:530) describes the
phase as "turning the refusals into composition across convert,
compare, dd, bench, map, measure and check" and calls it
"mechanical once 11 to 13 land". Six findings, five of which
contradict that sentence. The master plan's bullet and the
index.md row are corrected in this phase's planning commit.
F1. Three of the seven named operations do not use the chain
walker, and one of the walker's consumers is not named. The
refusal the phase 12 bullet called "a single if in
init_chain_states" is one of four independent families:
| Operation | Chain walker | Refusal site | What a lift means |
|---|---|---|---|
convert |
yes | init_chain_states |
lift, plus the host gate |
dd |
via convert |
inherits | host only, no guest change |
compare |
yes | init_chain_states |
lift; the op that needs #614 |
bench |
yes | init_chain_states |
lift |
rebase |
yes, src/operations/rebase/src/main.rs:1201 |
init_chain_states |
absent from the master plan's list |
check |
partial: validate_chain walks members |
own, :1593 / :1996 |
validates members independently; composes nothing |
measure |
no ChainConfig at all |
own, :422 / :441 |
build chain support it lacks |
map |
no ChainConfig at all |
own, :463 / :531 |
ditto, and src/shared/src/lib.rs:833 defers it to PLAN-map.md |
rebase matters most of the four corrections: it reads through
init_chain_states, so lifting the refusal there changes rebase
whether this phase plans for it or not. Leaving it unnamed would
have shipped an unplanned behaviour change.
grep -c qcow2::init_chain_states src/operations/*/src/main.rs
reproduces the first column; grep -c ChainConfig the second.
F2. dd is not a separate operation. run_dd
(src/vmm/src/main.rs:14629) ends in execute_convert (:14732),
so there is no src/operations/dd and no guest-op work for it. It
does take its own discover_backing_chain with ChainUse::Compose
at :14686, so it needs the host half and nothing else.
F3. The host gate is per-caller-kind, and the rollout is
per-operation. src/vmm/src/main.rs:3047 holds
which records a differencing parent and never resolves it, for
every composing caller. Its justifying comment (:2989-3010)
argues that resolving a parent instar will not read would make the
refusal contingent on the parent's presence -- sound reasoning,
resting on the premise "Nothing in instar can compose a VHD or VHDX
chain yet: every read entry point refuses such a source by name".
This phase makes that premise false for five operations and leaves
it true for three, so the gate cannot stay a property of
ChainUse: it has two variants (:2700), nine Compose call
sites and one Report (run_info, :10715). See decision 5.
F4. #623's blast radius is five consumers, not the writer.
calculate_bat_layout (src/crates/vhdx/src/lib.rs:3357) takes
(virtual_disk_size, block_size, logical_sector_size) and no
has_parent, so it always sizes by the dynamic rule
total_blocks + ceil(total_blocks / chunk_ratio). The
differencing rule is ceil(total_blocks / chunk_ratio) * (chunk_ratio + 1).
Callers: src/crates/create/src/lib.rs:1404,
src/operations/create/src/main.rs:654,
src/crates/measure/src/lib.rs:1025,
src/crates/resize/src/vhdx.rs:85, and
src/operations/convert/src/main.rs:4394. That is wider than
"the writer side" as the master plan's gate paragraph (:537)
implies, and it is why 14a is its own step with its own risk entry.
Partly falsified by 14a's execution. Two predictions in the
paragraph above were wrong, and both are recorded here rather than
silently fixed because a later step would otherwise rely on them.
measure cannot predict a differencing VHDX at all -- VhdxOpts
carries no parent field and MeasureConfig no backing field, so
there is no route to ask it and nothing about its output could
change; its call site passes false and says why. And the bytes
create -f vhdx -b writes do not change at any geometry the suite
already used: the BAT region is rounded up to a megabyte and the
two rules differ by at most chunk_ratio - 1 entries, so both
counts land in the same region. No test expectation moved. That
rounding is also why the defect survived -- it is visible only
where the shortfall escapes the megabyte -- so 14a's tests pick
such a geometry deliberately.
F5. #614 has an in-tree precedent for its fix. ChainConfig
(src/shared/src/lib.rs:4817) is a flat [ChainDeviceInfo; 16]
with a device_count and nothing describing segmentation, which is
the whole of #614. But the information exists twice already, in
per-operation configs: CompareConfig.image1_device_count /
image2_device_count (:2376, :2381), and
CommitConfig.backing_chain_first / backing_chain_count
(:4563, :4566) -- a literal (start, len) segment descriptor
already on the wire. And read_chain_virtual_cluster (:11455)
already takes chain_start and chain_len, so the reader knows
its own chain's bounds; only init_chain_states does not. The fix
is to put the segmentation where the chain is described rather than
deriving it a fourth time. See decision 4.
Corrected by the 14b design gate, which read the tree rather
than this finding and found three things wrong with it. Six host
paths write a ChainConfig, not four: run_commit_guest
(src/vmm/src/main.rs:10461) and run_check (:11798) also do,
and check reads one (src/operations/check/src/main.rs:293,
:422) without ever calling init_chain_states, so tightening
ChainConfig::is_valid reaches two operations this phase does not
otherwise touch. init_chain_states has five call sites, not four
-- convert calls it twice, and the second
(src/operations/convert/src/main.rs:1007) mutates device 0 in
place for the LUKS inner image with device_count 1 and restores
it afterwards, so it must override and restore the segmentation
too. And is_valid (src/shared/src/lib.rs:4853) already checks
device_count > 0 as well as magic, which is what makes
segment_count == 0 an unreachable state rather than merely an
unusual one.
F6. The user-visible refusal message cites a plan file.
src/vmm/src/main.rs:852 renders "composition is deferred (see
PLAN-differencing.md)" into the error a user reads. This phase
rewrites that message anyway, since it stops being the answer for
five operations, so the citation goes with it.
One thing the survey confirmed rather than contradicted: the master plan is right that both gates must be settled before any refusal is lifted, and right about why. Lifting before #623 ships a reader that cannot resolve the sector bitmaps of images instar itself wrote.
Decisions¶
-
Phase 14 covers the chain-walker operations only;
map,measureandcheckbecome phase 15. Decided with Michael on 2026-10-05 after the survey. The two groups are different work: one deletes guards from a code path that already composes, the other builds chain plumbing in operations that have never had any, andmap's belongs to a different master plan. Mixing them would produce a phase whose review could not be scoped. The current phases 15, 16 and 17 renumber to 16, 17 and 18. -
Both gates are steps of this phase, 14a and 14b, before any refusal is lifted. #614 on its own ships nothing a user can observe, so a separate gate phase would have no testable deliverable and no way to demonstrate the thing it enables. Put adjacent to the lift they gate, each is verifiable by the lift failing without it.
-
Issue #625 is in scope, and it is this phase that makes it urgent. Phase 13 added a
MIN_BLOCK_FILE_OFFSETfloor so a zeroed BAT entry cannot name offset 0, but an entry naming 2 MiB -- inside the BAT or metadata region of a small image -- is still accepted. While every read entry point refuses a differencing source, that path is reachable only from crate tests. Lifting the refusal makes it reachable frominstar converton an untrusted image, which turns a latent bound check into a reachable one. Fixing it in the phase that creates the reachability is cheaper than filing it forward, and a reviewer should not have to reconstruct that argument. -
#614 is fixed by giving
ChainConfigthe segmentation, at version 3 -- not by adding a parameter toinit_chain_states. The alternative is to pass a segment list from each of the four call sites, derived from each operation's own config. That works and is a smaller diff, and it is the wrong shape: it asks four operations to compute the same fact consistently, whenChainConfigis the struct whose entire job is describing the chain.ChainConfighas aversion(currently 2, fordata_device_idx) and a_reservedword, so the extension is routine, andCommitConfig's(first, count)pair is the in-tree precedent for the field shape. Making the answer derivable once is also what stops a fifth consumer getting it wrong later.
This is the decision most likely to be argued with, because
it is a guest/host ABI change in a phase whose point is deleting
if statements, and because the per-operation alternative would
let 14b be a two-line change in compare alone. Three things
decide it. compare is not the only op with a multi-chain
future -- a convert with a differencing source and a
differencing output chain has the same shape. The reader already
carries chain_start/chain_len, so the asymmetry being fixed
is that init_chain_states was handed less than its sibling, not
that nobody knew. And a wrong answer composes a child against an
unrelated image, which the refusal currently prevents and no test
that uses a single chain can detect -- exactly the defect class
phase 12 found by writing the two-device case.
-
The host gate becomes a per-call capability, not a third
ChainUsevariant.ChainUsedistinguishes "will attach and launch" from "will print"; whether a particular operation can compose a differencing VHD is orthogonal to that, and encoding it asChainUse::ComposeDifferencingwould make the enum mean two things at once and leave nine call sites to audit for which they meant. Add a separate argument carrying the composability, so a call site states its capability explicitly and a reader can enumerate the five that say yes. The comment at:2989is rewritten to argue the surviving case rather than deleted: the reasoning is still correct for the operations that still refuse. -
Rollout order is
convert(withdd), thencompare, thenbenchandrebase.
Falsified by 14e's execution, in the part that assumed the lift
could be staged at all. init_chain_states is one function with
five callers, so removing its refusal lifts it for convert, dd,
compare, bench and rebase in the same instant. Measured: after
14e, bench -c 4 on a differencing VHD returns a throughput number
and compare reports a verdict, neither of which 14e set out to
change. Staging it per operation would have meant a per-operation
capability on the guest ABI, which decisions 4 and 5 deliberately
keep on the host -- and the host gate from 14d is what actually
achieves the per-operation rollout, because an operation whose call
states Unsupported never has a parent attached and so can only see
a single-device chain. The three operations that still refuse do so
in their own code, never reaching init_chain_states at all. The
consequence is that 14f and 14g carry no code change: they become
the steps that assert the behaviour 14e already shipped, which is
what decision 6's own reasoning about test strength was really
about. convert first because it is the
operation a user reaches for, it has the most read paths
(read_chain_virtual_cluster ten times in
src/operations/convert/src/main.rs), and an integration test
for it is a whole-file byte comparison -- the cheapest strong
proof available. compare second because it is the op that
exercises 14b: it is the one that packs two chains into one
array. bench and rebase last and together: both are single
additional call sites once the first two are right.
-
calculate_bat_layoutgains a required parameter, not a defaulted one or a second function. Every one of the five consumers must state whether it is sizing a differencing image, and the compiler should refuse the ones that do not. Acalculate_bat_layout_differencingsibling would let a consumer keep calling the old name and be silently wrong, which is the failure #623 already is. -
A half-lifted tree must say so. After this phase,
convert,dd,compare,benchandrebasecompose a differencing source andmap,measureandcheckrefuse it. The refusal message must name the operation and not imply that instar cannot compose at all -- a user who has just run a successfulconvertand then a refusedmapneeds the message to explain the difference rather than contradict their last command.
Step plan¶
| Step | Effort | Model | Isolation | Brief for sub-agent |
|---|---|---|---|---|
| 14a | high | opus | none | Fix issue #623. Give vhdx::calculate_bat_layout (src/crates/vhdx/src/lib.rs:3357) a required has_parent: bool parameter and size the BAT by the differencing rule when it is set: a differencing VHDX BAT is padded to whole chunk groups, ceil(total_blocks / chunk_ratio) * (chunk_ratio + 1) entries, where a dynamic one takes total_blocks + ceil(total_blocks / chunk_ratio). Update all five consumers to pass their actual intent: src/crates/create/src/lib.rs:1404, src/operations/create/src/main.rs:654, src/crates/measure/src/lib.rs:1025, src/crates/resize/src/vhdx.rs:85, src/operations/convert/src/main.rs:4394. Do not add a defaulted parameter or a sibling function (decision 7). Three consequences to handle rather than discover: measure's predicted size for a differencing VHDX changes, so its expectation changes with it; resize must be checked for whether it accepts a differencing VHDX at all before its call site is given a true branch; and the bytes create -f vhdx -b writes change, so any golden fixture or round-trip expectation over that output changes too -- find them with make test-rust and the tests/test_create.py and tests/test_differencing.py suites rather than by reading. The phase 13 reader deliberately caps itself at the declared BAT region (sb_bat_entry_bound, added in ec005d57); once the writer is right, confirm that cap no longer truncates instar's own output, and say in the commit message which image geometry you checked it at. Add a mutation to tools/mutate-differencing.sh reverting the padded rule. Closes #623 -- use the Fixes keyword in the pull request body, not only the commit message. Built as 6d8ee05d. |
| 14b | high | opus | none | Fix issue #614. init_chain_states (src/crates/qcow2/src/lib.rs:13404) receives only device_count, a bound on a flat array that may hold several independent chains -- compare diff.vhdx base.raw packs two -- so it cannot tell a differencing child with a real parent behind it from one followed by an unrelated image. Extend ChainConfig (src/shared/src/lib.rs:4817) to carry the segmentation and bump ChainConfig::VERSION to 3, following the (first, count) field shape CommitConfig.backing_chain_first / backing_chain_count (:4563) already ships; a _reserved word is available. Do not add a parameter to init_chain_states instead (decision 4). Populate it on the host for every caller that writes a ChainConfig -- and note that the four line numbers this row originally gave for run_rebase and run_compare were their argument-parsing functions, not their config writers; the writers are run_rebase_guest (src/vmm/src/main.rs:10120) and run_compare (:12965, not the :12674 an earlier draft of this plan gave), and run_commit_guest (:10461) and run_check (:11798) write one too -- and assert in the guest that a chain's segment bounds agree with device_count. No refusal is lifted in this step: the deliverable is that init_chain_states could make the per-chain judgement, proved by a test that builds a two-chain array with a differencing child at index 0 and asserts the function sees chain length 1 for it. The existing vhdx_init_refuses_a_differencing_child_and_admits_a_dynamic_one (src/crates/qcow2/src/lib.rs:9795) documents the hazard in its comment and is the test to extend. Closes #614. Built as 1bd1dac2. |
| 14c | medium | sonnet | none | Fix issue #625. Phase 13 added vhdx::MIN_BLOCK_FILE_OFFSET so a zeroed or truncated BAT entry cannot name file offset 0, but an entry naming any offset inside the BAT or metadata region of a small image is still accepted, and block_lookup / sector_bitmap_lookup will read a block from there. Bound a payload or sector-bitmap block's file offset below by the end of the last declared region rather than by a 1 MiB constant: VhdxState already parses the region table at init, so the bound is derivable from what the image declares about itself. Refuse, do not clamp -- an image whose BAT names a block inside its own metadata is malformed, and the phase 12 and 13 arms both fail closed rather than guess. Add crate tests for an offset inside the BAT region, inside the metadata region, and immediately after the last region as a positive control, plus mutations in tools/mutate-differencing.sh. This is a bound check, not a behaviour change: no image instar writes names such an offset, and the test that proves it is the existing compose suite still passing. Closes #625. Redesigned during execution. This row originally said to bound the offset below by the end of the last declared region, which was wrong: SPEC(VHDX) does not require a region to precede the payload blocks, so a legally placed trailing region would have made the bound refuse every block in the file. The implemented test is the one the issue names -- the block's byte range overlapping a declared region's -- over every entry the table carries, recognised or not. Offset zero is not subsumed by that predicate, because a block at zero overlaps nothing unless a region starts at zero, so it stays a separate guard and both are independently killable. The row's "not a behaviour change" was also wrong, and review round 1 caught it: the overlap test sits in block_lookup's fully-present arm, which runs whatever has_parent says, so it tightens every VHDX read rather than only a differencing one. No conforming image is affected, but a malformed one instar used to read now fails the read, and that belongs in the changelog as a Changed entry rather than being described as a latent fix. Corrected there, and a crate test now pins the refusal on a has_parent == false image so the dynamic path cannot regress unnoticed. Built as 52c3cfbc. |
| 14d | high | opus | none | The host-side composability gate. discover_backing_chain (src/vmm/src/main.rs, the match at :3044) refuses to resolve a differencing VHD or VHDX parent for every ChainUse::Compose caller. Replace the caller-kind test with an explicit per-call capability argument (decision 5) -- not a third ChainUse variant -- so each of the ten call sites states whether its operation can compose a differencing chain. Set it true for run_bench (:5045), run_rebase (:6903, :6916), run_compare (:12407, :12417), execute_convert (:13255) and run_dd (:14686); false for run_commit (:7265) and run_check (:11662); run_info (:10715) stays Report and is untouched. Rewrite the comment at :2989-3010 rather than deleting it: its argument -- that a refusal must never become contingent on whether a parent file happens to exist -- is still exactly right for the callers that still refuse, and is the invariant phase 4 established and phase 11 preserved. State in the commit message which call sites you set each way and why, because that list is the phase's actual policy. No guest change in this step; the refusals are still in init_chain_states, so every operation still fails, and the proof of this step is that the host now resolves the parent and the guest still declines it. Corrected by execution. This row's claim that nothing user-visible changes is false. Resolving a parent means failing when it is missing or outside the allowlist, so convert, dd, compare and bench now fail in the host walk for an orphaned parent instead of reaching the guest's typed refusal; the step is invisible only for a well-formed chain whose parent is present. Three integration tests asserted the old caller-kind scoping and were rewritten along the new policy boundary rather than relaxed, and the six hostile-locator fixtures are now declined by path resolution and the allowlist during discovery, with a per-fixture reason table measured against the built binary and an assertion that no output file is produced. One consequence 14e must close: between these two steps a differencing source fails one way with its parent present and another way without, which is the contingent refusal phase 4 ruled out. Built as 185df853. |
| 14e | high | opus | none | Lift the refusal for convert, and dd with it. Remove the VHD (src/crates/qcow2/src/lib.rs:13501) and VHDX (:13529) refusals from init_chain_states, replacing them with the per-chain judgement 14b made possible: a differencing child with no device behind it in its own chain must still be refused, because composing it would read parent-owned sectors from nothing. That is the condition phase 12 proved unsafe to write against device_count and 14b makes safe to write against the segmentation. Keep the typed refusal and its status codes (shared::DifferencingRefusal) for that case -- it is now a narrower refusal, not a deleted one. Rewrite the host message at src/vmm/src/main.rs:841-857 per decision 8: name the operation, say that the source's parent could not be composed and why, and drop the PLAN-differencing.md citation (F6). Verify with a Python integration test that converts a real differencing VHD and a real differencing VHDX to raw and compares the whole output byte-for-byte against the same chain flattened by the phase 1 oracle or by instar reading the parent directly -- a whole-file comparison, not a spot check. Both formats, and a chain of depth 3 for at least one of them. Execution notes. No depth-3 fixture exists and create -b refuses to stack a child on a child, so the test builds one with qemu-img rebase -u. The expectation came from the recorded *-composed.raw fixtures rather than the phase 1 oracle, which is absent on the development host and is in any case the wrong reference for the mixed-bitmap VHD child. Built as 42a23cc1. |
| 14f | high | opus | none | Lift for compare, which is the operation 14b exists for. compare packs two independent chains into one device array (CompareConfig.image1_device_count / image2_device_count, src/shared/src/lib.rs:2376) and calls init_chain_states once with the total (src/operations/compare/src/main.rs:215). The tests that matter are the ones a single-chain test cannot fail: a differencing child as image1 against a raw image2, where the old dev_idx + 1 >= device_count form would have composed the child against image2; and both images differencing children of different parents. Assert the comparison verdict, not just that the command exits 0 -- a wrongly composed read can still produce "identical" if both sides are wrong the same way, so one test must compare a differencing chain against its own correctly flattened output and a second must compare it against a deliberately different image and expect a difference at a known offset. No production change: 14e's lift already covered compare, so this step is the proof rather than the change. Five tests, including the one the brief's point 3 really asked for -- image2 set to qemu's own flattening of the same child, the one file a reader that composed the child against image2 would call identical -- and a four-device, two-chain case built by copying each parent into its own directory and altering one, so the children stay byte-identical and only per-chain descent can move the verdict. Execution found that collapsing the segmentation does not by itself produce silent wrong data, because read_chain_virtual_cluster's own chain_len bound refuses the descent, which is why each test pins an exact message or an exact offset rather than a non-zero exit. Built as 969ac5de. |
| 14g | medium | sonnet | none | Lift for bench and rebase. Both read through init_chain_states and need no new guest logic once 14e is in: src/operations/bench/src/main.rs:1395 and src/operations/rebase/src/main.rs:1201. rebase is the one the master plan did not name (F1), so it needs the most care in testing rather than the least. Re-scoped by 14e's execution: rebase refuses any overlay that is not qcow2 or vmdk (src/vmm/src/main.rs:7138, rebase: format 'Vhd' does not support rebase), so "rebasing a differencing VHD or VHDX child" is not a thing this tool does and the brief as first written is untestable. What is testable, and what this step must cover, is a differencing VHD or VHDX sitting in the backing chain of a qcow2 overlay being rebased -- the chain is read, so the composition matters, and the test must assert the rebased image's contents rather than that the command completed. 14e also found rebase has no differencing_refusal_error call site, so a guest refusal reaching it renders a generic message; this step decides whether to add one. Execution found three production defects in rebase, so this step did carry code after all. The operation's Cargo.toml never requested vhd-input/vhdx-input from its qcow2 dependency, so the host attached a differencing parent to a binary with no VHD parser; read_chain_cluster's own format allowlist, separate from the crate features, still refused VHD and VHDX once that was fixed; and dummy_buf pointed at CHAIN_CACHES, the first chain device's own L1/BAT cache slot. That last was documented as safe because compressed_buf is never touched in this build -- true for qcow2 and raw, false the moment a VHD chain member exists, because a differencing VHD chunk's mixed-ownership arm uses it as a sub-sector bounce buffer, so the read clobbered the cached BAT sector mid-lookup. It was unreachable before this phase and the first two fixes armed it. A carve of their own replaces it (CHAIN_READ_COMPRESSED): review round 3 noted the first fix had handed the same pointer to both compressed_buf and staging_buf, which was safe only by the order in which the qcow2 crate happens to touch them, so the two now have separate addresses that alias nothing. The refusal call site was added. bench --output json has no raw byte-count field -- count and buffer-size echo the arguments rather than measuring -- so the byte-count assertion the plan asked for -- whose point was that a composed read silently serving zeros would otherwise pass -- does not exist, and position probes at each fixture's validated parent-owned and child-owned sectors stand in for it. Built as c46eef81. |
| 14h | medium | sonnet | none | Integration-test the boundary this phase draws. Add tests asserting that map, measure and check still refuse a differencing source, with their own refusal messages, and that each message satisfies decision 8 -- names its operation and does not claim instar cannot compose. This is the test that stops phase 15 silently inheriting a half-lifted tree, and the test that would have caught rebase being missing from the master plan's list. Put them in tests/test_differencing.py beside the composition tests, so the two halves of the policy are read together. Also add a case per operation to tools/mutate-differencing.sh that re-asserts the refusal, so a future phase cannot lift one of the three by accident. Execution note. The tests assert decision 8's two properties -- the message names its own operation, and never reads as a claim about every operation -- rather than pinning map's sentence verbatim, because 14i rewords it; the verbatim pin stays in the older test. check is asserted both with and without --chain, since it walks a chain only when asked and a gate that differed under the flag would otherwise be invisible. No production change. Built as 416046bd. |
| 14i | low | sonnet | none | Documentation and changelog. CHANGELOG.md: the five operations that now compose a differencing VHD or VHDX, the three that still refuse, and the three issues closed. docs/convert.md, docs/compare.md, docs/bench.md, docs/rebase.md and docs/dd.md each gain or lose a differencing limitation -- check what each currently claims rather than assuming, since phase 4 wrote those limitations and some are now false. docs/map.md:238's VHDX partial-present entry and docs/measure.md stay as they are and should be checked to confirm they still read as a limitation rather than a plan. docs/chain-discovery.md describes the host walk and now needs the per-call capability (decision 5). Do not update ARCHITECTURE.md -- no component or data path changes -- and do not update AGENTS.md unless 14b's ChainConfig version bump creates a convention an agent could not infer, in which case one line about the version is the whole change. Execution notes. Nine user-visible plan citations, not the eight the Definition of done counted: the create --sector-size message spans a non-quoted continuation line that the criterion's own grep misses. Four files beyond the named list -- docs/create.md, docs/guest-architecture.md, docs/format-coverage.md and docs/info.md -- asserted that nothing in instar composes a differencing source, which is false for five operations, and were corrected. docs/quirks.md's claim that info --chain reports a one-image chain was already false before this phase began, which is drift from phase 11 or 13 rather than anything 14a to 14h did. AGENTS.md was left alone: the ChainConfig version bump uses an already-documented versioned-struct pattern and is not a new convention. Built as b0ba64b8. |
Risks and mitigations¶
-
14a changes bytes instar already writes. Fixing the BAT sizing changes the output of
create -f vhdx -band the sizemeasurepredicts for it. A round-trip or golden expectation that encodes the old size will fail, and the failure will look like a regression. Mitigation: 14a's brief requires finding them by running the suites, and the management session reviews the list of changed expectations as a list -- each one must be explained as "the old number was the bug", not adjusted to match. -
14b is a guest/host ABI change. A version-3
ChainConfigread by a guest binary built against version 2, or the reverse, is a silent misparse rather than an error. Mitigation: the version field exists andis_validalready checks magic; 14b must make the guest refuse a version it does not know, and a test must assert the refusal. The management session checks that both halves of the tree are rebuilt before any integration test is believed -- a staleinstar-releaseimage has produced a false result in this project before. -
A half-lifted tree is a new user-visible inconsistency. Five operations compose, three refuse, and a user will hit both in one session. Mitigation: decision 8 and step 14h, which tests the refusal messages rather than only the refusals.
-
The lift's correctness is invisible to a test that uses one chain. The defect #614 describes -- composing a child against an unrelated image -- produces plausible output. Mitigation: 14f's two-chain cases are the specific test, and the
tools/mutate-differencing.shentries from 14b and 14f must kill a mutation that reverts the segmentation. The harness verdict, not the suite passing, is the evidence. -
Phase 15's scope is now defined by this phase's omission. If 14h's boundary tests are weak, phase 15 inherits an unclear starting point. Mitigation: 14h is a step rather than a line in 14i's brief, precisely so it is reviewed on its own.
Definition of done¶
instar convertwrites byte-identical output for a differencing VHD chain and for the same data flattened into a single image. Both outputs are in 14e's commit message, with the command that produced them. Same for VHDX.instar compareof a differencing child against its own correctly flattened output reports identical, and against a deliberately different image reports a difference at the expected offset. Both assertions are intests/test_differencing.py.instar map,instar measureandinstar checkeach still refuse a differencing source, and each refusal message names its own operation. Falsifiable: a test asserts the message text, not just the exit code.- No user-visible string contains
PLAN-. Falsifiable:grep -rn 'PLAN-[a-z0-9-]*\.md' --include=*.rs src/ | grep -v '^\s*//'returns nothing fromformat!,println!,eprintln!or a&strconstant. 14e found this is eight strings, not the one F6 scoped it to: the differencing read refusal (removed in 14e),map's refusal citingPLAN-map.md(src/vmm/src/main.rs:16138),create's differencing-backing refusal citingPLAN-differencing.md(:18443), and fivePLAN-create.md/PLAN-convert-followups.mdcitations between:14799and:18926. The criterion stands as written and 14i clears all eight -- these are error messages a user reads, pointing at a directory of internal planning documents, and leaving seven in place while claiming the criterion is met would be false. Rewording a refusal's text is not the same as changing what it refuses:create's refusal is additionally wrong now, which is issue #628 and out of scope. vhdx::calculate_bat_layoutcannot be called without statinghas_parent. Falsifiable: it takes four parameters, andgrep -c 'calculate_bat_layout(' src/finds the same five consumers it finds today.- A differencing VHDX written by
instar create -f vhdx -band read back byinstar convertresolves every sector bitmap -- that is, the phase 13sb_bat_entry_boundcap no longer truncates instar's own output. Falsifiable: the geometry and the before/after entry counts are in 14a's commit message. init_chain_statesrefuses a differencing child that has no device behind it in its own chain, and admits one that does, with a two-chain array distinguishing them. Both are asserted by tests, and a mutation reverting the segmentation kills one of them.tools/mutate-differencing.shcase count has grown, the new count is derived bycheck_case_countrather than written in prose, anddocs/testing.mdagrees with it. Every new case is PASS or a documented SURVIVOR; no BROKEN.make test-rustpasses with zero failures and the count is stated, against the 2427 phase 13 measured.pre-commit run --all-filesis clean.- No source file or comment added by this phase cites a plan
phase, step or decision number. Falsifiable:
git diff d677823c..HEAD -- 'src/*' | grep '^+' | grep -iE 'PLAN-[a-z0-9-]+\.md|decision [0-9]|phase 1[0-9]|14[a-i]'is empty. CHANGELOG.mdnames which operations compose and which refuse.
Back brief¶
Before implementation starts, confirm:
-
The step order and the gate placement. 14a to 14c settle three issues before 14d touches a policy and 14e deletes a guard. Nothing user-visible changes until 14e.
-
A gate on 14b's wire format, before any code is written. The
ChainConfigversion-3 field shape is cheap to propose and expensive to redo: it is read by five guest operations and written by four host paths, and getting the shape wrong means redoing 14b, 14e, 14f and 14g. The implementer of 14b must bring the exact struct definition -- field names, types, offsets, and what the guest asserts about them -- to the management session before editing. Decision 4 settles that the segmentation goes inChainConfig; it does not settle whether that is a per-device chain id, a per-chain(first, count)table, or achain_starton eachChainDeviceInfo. -
That the two gates really are gates. If 14a or 14b turns out to be larger than planned -- 14a's five consumers are the likeliest source of that -- the phase stops and reports rather than lifting a refusal onto an unsettled gate. The master plan is explicit that lifting before #623 ships a reader that cannot read instar's own output.
-
Model and effort. 14a, 14b, 14d, 14e and 14f are opus at high effort: each decides a policy or a wire format rather than implementing one. 14c, 14g and 14h are sonnet at medium with the briefs above. 14i is low. No step wants
fable; none of them has defeated opus. -
The standing warning. This repository's history is that agents assert plausible-but-wrong format and tool capabilities. Every claim about what a VHDX BAT contains at a given geometry must be measured against a built image, not reasoned about -- phase 13 found its largest defect (
ec005d57) exactly where the arithmetic looked obviously right.
Review round 1¶
The automated review of #630 raised nine items: two fix, one
document, five consider and one informational. Eight were taken.
Two are worth recording here because they say something the plan got
wrong rather than something the code did:
-
14c's "not a behaviour change" was false. Corrected in the 14c row above and in
CHANGELOG.md, which now carries a Changed entry for it. A crate test pins the refusal on ahas_parent == falseimage so the plain dynamic path cannot lose it unnoticed, and a mutation case re-gates the check onhas_parentto prove that test is the one holding it. -
Nothing exercised
rebase's two-segment chain config. Every rebase test in 14g and 14h detaches (-b ''), which produces oneChainSegment; the multi-segment emission that #614's fix exists to make safe inrebasehad no coverage at all. Found while answering the review's note that no test puts a differencing image in a new backing chain, which is the construction that reaches it.test_rebase_onto_a_new_backing_keeps_the_two_chains_apartnow does, and a mutation case drops the second chain's segment to prove it. The host-side device-count agreement check, which the same round turned from adebug_assert_eq!into a real error, is what that mutation trips.
Writing that test also turned up issue #632, which has nothing to do
with differencing: a safe-mode rebase onto a raw new backing always
fails as the overlay's header could not be parsed, whatever the old
chain holds, while the same rebase onto a qcow2 backing succeeds and
-u with the raw backing succeeds. raw is an advertised value for
-F that no test exercises. The test uses a qcow2 new backing and
says why.
Declined, with the reason:
-
Plan citations in
docs/quirks.md. The remaining ones quote error-message text verbatim inside dated records of what instar did at the time. Rewriting them would falsify the record; this phase already reframed the surrounding prose into the past tense and added the current wording beside it. The phase citations in releasedCHANGELOG.mdsections are history and out of scope. Separately verified: no user-visible message containsPLAN-at all, so what was stale was documentation quoting a message that no longer exists. -
A CLI-level differencing-over-differencing chain. Both formats have crate-level coverage of two consecutive composition levels (
src/crates/qcow2/src/lib.rs:7996and:9853), but nothing drives it through the binary, and nothing can until the fixture exists:qemu-imgcannot author a third differencing level in either format. Filed as issue #631 with what the fixture needs.
Review round 2¶
Eight items: two fix, one document, four consider and one
informational. Seven taken, one declined on evidence. Both fix items
were stale documentation this phase's own changes created -- two test
docstrings describing map's message as still due to be reworded when
this phase reworded it, and a sixth cell on the 14g row of a
five-column table.
Three findings are worth recording:
-
The overlap documentation claimed more than the scan enforces.
VhdxState::initreadsentry_count.min(8)region table entries while SPEC(VHDX) permits 2047, so "any entry in the region table" was true only of the first eight. The docs now say so. The cap is sound rather than merely documented: the same scan is what setsfound_batandfound_metadata, andinitreturnsNonewhen either is missing, so an image cannot hide the BAT or the metadata region behind it -- only an unrecognised region, whose bytes nothing reads as structure.vhdx_overlap_check_covers_the_eighth_region_table_entrypins the eighth entry being checked, with a control proving the refusal is that entry's range and not the presence of six more entries. -
The trailing-region test had no mutation case. It is the one test distinguishing an overlap test from a high-water mark -- the regression 14c's fix was shaped to avoid -- and nothing proved it could fail.
vhdx-read-overlap-replaced-by-a-high-water-marknow does, and it is surgical: because the fixture's payload blocks sit immediately above the BAT region's end, the mark only rises past them when a region is declared after the blocks, so every "inside a region" case still passes under it and only the trailing-region test fails. Found by the round's own instruction to mutate rather than read the guard, not by the review. -
benchandcheckhave no chain-depth guard (issue #633). Chased from the review's claim thatcomparelacked a host-side agreement check between itsCompareConfigcounts and its written device slots.comparedoes not::12826refuses the combined depth before KVM is opened, andtotal_devices()counts exactly the slots the entry writer emits, so the two cannot disagree. Sweeping the class foundbenchandcheckare the only chain-attaching operations with no such guard, where an over-long chain is silently truncated and a 17th device's virtqueue memory lands onDMA_POOL_BASE. Pre-existing and unrelated to differencing, so filed rather than fixed here.
Declined, with the reason:
- A host-side count agreement check in
run_compare. Declined because the premise does not hold, as above. The review's first suggestion -- derivingCompareConfig's counts from the written slots -- would be a regression rather than a tightening: it would replace a refusal naming both chain depths with a silently truncated comparison, which is the one answercomparemust never give. A comment at the config write now names the guard that makes the two sources agree, since the guard is 90 lines above and a reader at the write cannot see it.
Review round 3¶
Six items: one fix, one document, four consider. Five taken, one
declined and filed. The round's shape changed: where rounds 1 and 2
found claims the plan or the changelog got wrong, this one found two
genuine defects in code the earlier rounds had added.
-
The overlap check never reached
mapormeasure(issue #634).CHANGELOG.mdsaid it reached "convert,dd,compare,benchandmap", which was wrong twice:mapwalks the BAT throughclassify_vhdx_bat_entryinmap_extentsand applies no overlap test, andrebase-- which does reach it -- was missing from the list.measureis in the same position asmapviascan_allocation. There are exactly three callers ofblock_overlaps_a_declared_region, all insideblock_lookupandsector_bitmap_lookup, so the reach is enumerable rather than argued. Both documents now say which operations apply the rule and that two do not, and the divergence --convertrefusing an imagemapstill reports as data -- is filed rather than closed here, because changingmap's output for a malformed image is a behaviour change to an operation this phase deliberately leaves non-composing. Phase 15 revisits all three readers. -
ranges_overlaprefused on an empty region, inconsistently. Withregion_len == 0the predicate reduced tooffset < region_offset < end: a zero-length entry at or below a block's start was waved through, one whose offset fell strictly inside the block refused it. A region naming no bytes can hide nothing, so the refusal bought no safety, andinitdoes not reject a zero-length entry, so a hostile image can reach it. Empty ranges are now answered before either sum is formed. This also falsified a comment round 2 added, which claimed a zero-length region "intersects nothing" -- true only of the offset-0 entries that comment's own fixture carried. -
The two chain-read bounce buffers shared one address. Round 1 moved them off
CHAIN_CACHES, which was the real defect, but handedread_chain_virtual_clusterthe same pointer forcompressed_bufandstaging_buf. Those are separate parameters with separate lifetimes; sharing was safe only because no arm of the chain reader uses both at once, which is a property of call ordering inside theqcow2crate and invisible fromrebase. They have their own carve now (CHAIN_READ_COMPRESSED,CHAIN_READ_STAGING), above every other carve, aliasing nothing -- not each other, not a device's cache, not the DMG chunk-table scratch. They do not both fit insidePLANNER_SCRATCH_LIMIT, which is 64 KiB short ofCOMPRESSED_BUF_SIZE + MAX_CLUSTER_SIZE, so reusing that carve was not an option. Two static asserts hold the new carve below the allocator heap and above the carve it follows.
No mutation case guards the separation, and the attempt to add one
is the reason why: a case pointing staging_buf back at
compressed_buf was written, run, and came back FAIL -- the
differencing rebase suite still passed against it. That is the same
statement the review made, measured rather than argued: no current
behaviour distinguishes the two layouts, which is exactly why this
was a latent hazard and not a bug. The case was dropped rather than
shipped as a permanent FAIL, since the harness has no survivor
variant for integration cases and building one to hold a single
comment is not worth the machinery. What holds the separation is
the two constants and the static asserts, checked at compile time;
the carve's doc comment records that a mutation survives it, so the
next reader does not collapse them back on the grounds that nothing
fails. The one case this round did add -- the empty-region answer --
takes the harness from 90 to 91.
- The composing refusal said "source". For
rebasethe differencing image is never the source -- the overlay must be qcow2 or vmdk -- so the sentence was wrong every timerebaserendered it, and wrong for aconvertorcompareof a qcow2 overlay whose backing is a parentless differencing image. It now says "a differencing {fmt} image in the chain {op} was given has no parent behind it", which holds whether the differencing image is the source or a backing member. The non-composing arm keeps "source", and that is not an oversight:validate_chainnever runs the VHD/VHDX validators that holdrefuse_differencing, somap,measureandcheckonly ever refuse the image the user named.
Declined, with the reason:
- An integration fixture for a parentless differencing VHDX
(issue #635). The gap is real -- the narrowed refusal is only driven
end to end for VHD -- but it is a fixture gap, not a test gap.
qemu-imgwill not create a differencing VHDX,instar create -brefuses a differencing backing file, and copyingvhdx-diff-childwithout its parent exercises the unresolvable-parent path instead. It needs a hand-built image ininstar-testdata, the same family as #631, and the two are blocked on the same thing.