Repository navigation
feat(odb): --diff-roots value-level equality walker (root-build Step 1) - #897
Conversation
…ty basis Determinism spike for the --diff-roots equality walker. Proves byte-identity of packed outline bytes across two INDEPENDENT loads (open -> materialize -> pack -> dbclose -> reopen -> pack -> compare), which is the only form of the test that can detect volatile in-memory-only fields leaking into the packed form. Results, all three shipped roots, files byte-identical after every run: Virgin.root 3499 values 0 mismatches 0 packfailures 0 dirtyflips mainResponder.root 394 values 0 mismatches 0 packfailures 0 dirtyflips manila.root 1285 values 0 mismatches 0 packfailures 0 dirtyflips GREEN for scpt + optx: oppackoutline is a valid equality basis. CARVE-OUT for mbar: menubars cannot be compared this way in a headless build. menuverbinmemory_context (headless_menu_stubs.c:618) sets flinmemory = true WITHOUT loading, so variabledata is still a raw dbaddress and any dereference segfaults (observed on mainResponder.menu, exit 139). flinmemory is not trustworthy per-type headless. The walker will compare mbar at the stored-bytes level and report, never silently skip. Corroboration: mainResponder.menu is also the one mbar the 2026-08-11 census could not measure (size -1). Two tools, two mechanisms, same value. Incidental: for v7 roots views[0] IS the root table address (odbengine.c:566-570); reading it as a Cancoon record yields a non-block address. tyversion2cancoonrecord also compiles to 474 bytes vs the 442-byte on-disk record under 64-bit dbaddress, so it must not be used to read that record. Also: system.macintosh walked cleanly (578 nodes) in the pure-C path, so #864 does not reproduce without the interpreter. Refs #878, #864
tests/*_tests does not match the spike's name.
The acceptance instrument the rest of planning/ROOT_BUILD_FROM_SOURCE_PLAN.md depends on: every later step reduces to "run this and require an empty diff" (the round-trip law, and the CI drift check during the dual-source transition). Pure C. No interpreter, no string() coercion (which HARD-ABORTS on list values through try, #887), no hashgetvaluestring (deparses, would false-diff). Both roots open READ-ONLY; dbwrite refuses writes to a flreadonly database (db.c:715-728), so the guarantee holds below us too. Dispatched from main.c's early-exit band beside --migrate, so it returns before the runtime ever boots and can never reach the save-on-exit path. Comparison basis per type: - scpt/optx: oppackoutline() bytes -- canonical serialized form encoding text AND node structure, so the #881 single-LF-node class and #890's comment structure damage both surface. NOT langexternalpack(), whose outline branch calls dbassignhandle_context and writes disk blocks. - list/reco: oppacklist() bytes -- the same serializer langhash.c's own scalar packer uses (:3204-3207). Sidesteps #887 entirely rather than working around it. - scalars: inline compare or raw handle bytes, mirroring the langhash.c pack switch so the type taxonomy is the ODB's own and exhaustiveness is provable by inspection. - mbar/wptx: stored-address comparison, never materialized. The headless stub menuverbinmemory_context (headless_menu_stubs.c:618) sets flinmemory = true WITHOUT loading, so flinmemory is a LIE for mbar and any dereference segfaults; wptext materialization exhausts memory (db_format.c:1694). - unknown types: loud per-path UNSUPPORTED-TYPE finding, never a silent skip. Names are compared as RAW BYTES and only escaped for reporting, via the existing lossless ut_pct_encode_segment. Virgin.root really does contain sibling values named with the single bytes 0x0A/0x0B/0x0C/0x0D; two of those COLLIDE under newline normalization (census CAVEAT 4), which would mean a phantom or a swallowed finding. Because '.' encodes to %2E, a '.' in a reported path is always a separator. Two findings that shaped the implementation, both found by crashing first: 1. VOLATILE SAVE METADATA. The packed-outline header (oppack_v7.c:115-152) carries timecreated/timelastsave/ctsaves/outlinesignature -- and oppack INCREMENTS ctsaves on every pack (:622). Comparing a root against a re-saved copy produced 3,527 spurious findings before those regions were excluded. Packed lists inherit the same fields at a displacement, read from the list header's own recordsize field rather than hardcoded. 2. fldiskval VALUES. An unresolved value's data.binaryvalue is a dbaddress, not a pointer. Resolved via dbrefhandle_context and NOT written back, because the kernel's own resolver dirties the owning table as a side effect (langhash.c:2211). Path buffers are heap-allocated: maxdiffpath is deliberately generous (a diff tool must never fail to name a path) and the walk recurses, so stack buffers overflowed the default stack. Acceptance evidence (all files md5-identical before and after every run): - self-diff of all three shipped roots: empty, exit 0. Exercises every live type incl. the #887 lists and the 106 ???? placeholders. - Virgin.root vs a copy with ONE scripted mutation: exactly 2 findings -- the probe, plus Frontier.pathstring, which self-updates to the opening process's path (verified by reading it back; the #841 class). - ced8241-era Virgin.root vs develop: exactly 3 findings, ALL attributable to the only two commits touching Virgin.root in that range -- string.isValidUrl + webBrowser.openUrl (5bb7125, #881 LF repair) and startupScript (21d33d2, #849/#862 guards). No unattributable findings. Unit failure SET matches baseline exactly (26 suites N/N, plus the pre-existing test_callback_infrastructure SIGSEGV, #827/#828 family). diff_roots_tests is registered in RUN_BUILDABLE but the suite exits at that earlier crasher before reaching it; standalone it is 11/11. Refs #878, #887, #881, #890, #841
…son codes
Follow-up to the --diff-roots walker. Two changes, both from review:
1. mbar and wptext now compare their RAW STORED BYTES (read at each side's
external address via dbrefhandle_context) rather than only checking whether
the stored addresses match. Stored bytes are the ODB's ground truth, and the
round-trip law requires the assembler to reproduce them anyway, so this is
the correct basis for these types rather than a fallback. Neither type is
materialized.
2. The two types now carry DISTINCT reason codes in the report, because they
are excluded for unrelated reasons and merging them would mislead a later
reader:
mbar -- cannot be materialized headless at all; the stub sets
flinmemory = true without loading, so variabledata stays a raw
dbaddress and dereferencing segfaults (#896).
wptx -- CAN be materialized, but must not be during a full-root walk:
doing so exhausts memory and hangs (db_format.c:1694).
An unreadable stored block is now reported as a finding rather than being
treated as equality. A nil stored address yields an empty handle, so two
never-stored values compare equal instead of both failing.
Evidence the new path is real and not vacuous: across ced8241-era Virgin.root
and current Virgin.root -- two files with different sizes and different block
layouts -- all 43 menubars compare EQUAL by stored bytes, so the comparison is
reading content rather than addresses.
Acceptance re-verified, all unchanged: self-diff empty on all three shipped
roots; single-mutation case still exactly 2 findings; cross-era still exactly 3,
all attributable to 5bb7125 and 21d33d2. ced8241-vs-mutated yields 5, the
clean union of the two independent diffs, with no cross-talk. All inputs
md5-identical after every run. Unit failure SET unchanged from baseline.
Refs #896, #878
… derivation Two review directives on the packed-header exclusion: 1. The list displacement is read from the list header's own recordsize field (oplist.c:88) rather than hardcoded, so the walker cannot drift out of agreement with the format if tydisklistrecord changes shape. Now said at the exclusion table rather than only at listoutlinebase(). 2. Excluding timecreated/timelastsave makes this tool BLIND TO create/mod-date drift BY DESIGN -- correct for Step 1 drift detection (a re-saved root with identical content must compare equal or the gate is useless), and covered long-term by plan decision 9.4: the exporter preserves per-value dates in source manifests, so the round-trip law checks dates AS MANIFEST TEXT, not as packed bytes. Stated explicitly so the exclusion is not later mistaken for a hole. Comment-only; no behavior change. Acceptance re-verified unchanged.
Gate Review: unit-sb1-diff-roots (PR #897)Verdict: FAIL — two P0s in the compare core; fix round required before mergeThe CLI plumbing, read-only discipline, two-roots global-state design, memory hygiene, and exit-code contract were all independently verified sound — three reviewers plus two orchestrator-run audits agree. Both P0s sit in the equality basis itself: the silent-false-equality class this tool's own design document forbids. That is exactly the place a drift gate cannot be wrong, which is why this verdict is FAIL rather than pass-with-notes. Tests (orchestrator-run merge record at 42d104d)
Reviewers
P0 — blocking
Why the evidence lied (record this in the PR)Self-diff can't catch either P0 (same file both sides). The re-save experiment can't catch P0-2 (re-saves change neither timecreated nor the wrongly-covered content bytes, so both offset tables produce identical results on that experiment). Nothing exercised P0-1 because no fixture ever contained a differing menubar. The instrument-validation lesson: acceptance evidence must include fixtures that force each compare path to prove it can DETECT, not just agree. P1
P2 — fix in the same round (completionist)
Follow-up issues to file (not this PR)
Clean dimensions (multi-source verified)Read-only integrity through three independent layers (fopen "rb" / dbwrite refusal / dbflushheader skip) with the exit-save path structurally unreachable; two-roots global discipline holds across all 1404 lines (dbopenfile clobbers only databasedata, re-captured per side; no cleartablestructureglobals; explicit-context variants throughout; no UAF in the double close); recursion depth-guarded with heap path buffers; loud-fail honored on every traced error path (a crash on hostile input exits 139, never 0 — fail-closed); loop bounds from in-memory chains, not disk counts; tag-4/tag-12 equivalence correct; list displacement derivation correct (and its offset-49 datum corroborates P0-2); spike is test-only, unshipped, and its GREEN evidence survives the P0 (it hashed whole buffers, no skip table); exit codes 0/1/2 distinct end-to-end; C style clean. Stats
Next stepSingle fix round: P0-1 + P0-2 first (with the fixture pair proving DETECTION, not just agreement), then P1s 3-5, then P2s. Re-run the FULL acceptance set after — the P0 fixes invalidate prior acceptance evidence (corrected offsets may surface previously-swallowed fltextmode diffs in the cross-era comparison; every new finding must be attributed). Then re-gate delta review + fresh merge record. |
…ong by +6
Two gate-blocking defects, both in the silent-false-equality class this tool's
own design forbids, and both of which survived the original acceptance evidence.
P0-1 -- MBAR/WPTEXT COMPARISON WAS VACUOUS.
readstoredbytes was passed (**hv).oldaddress, but for a never-materialized
external that field is nildbaddress: langnewexternalvariable initializes it to
nil (langexternal.c:2932, :2997) and the stored address lives in variabledata.
menuverbunpack (menuverbs.c:501-509) is the concrete path -- it reads the
address off disk and passes it as newmenuvariable's variabledata argument.
So both sides read nil, hit the nil-means-empty-handle rule, and EVERY menubar
and wptext compared equal regardless of content. Verified directly: all four
sampled Virgin menubars have oldaddress=0x0 with real addresses in variabledata.
Fixes:
- storedaddressof() falls back to variabledata when oldaddress is nil and the
value is not resident, mirroring headless_menu_stubs.c:551-553.
- readstoredbytes now returns false for a nil address instead of an empty
handle. Treating "no address" as "empty content" is what made the failure
silent; the caller reports it.
A/B proof on a fixture whose only change is one byte inside a menubar's stored
block: the pre-fix binary reports "no differences"; the post-fix binary reports
exactly one payload finding at that menubar.
P0-2 -- PACKED-HEADER SKIP REGIONS WERE 6 BYTES TOO HIGH.
The regions were hardcoded from a probe of typortablediskheader written WITHOUT
#pragma pack(2), which reported 24/32/40/48 and a 1080-byte header. The real
packed layout is timecreated=18, timelastsave=26, ctsaves=34, fltextmode=38,
outlinesignature=40, total 1068 -- and the _Static_assert at oppack_v7.c:154
pins 1068, so the wrong table contradicted an existing invariant.
Consequences: timecreated 18-23 was NOT skipped (false diffs whenever creation
stamps differ, breaking the round-trip law by construction), and fltextmode
38-39 WAS skipped (real content silently comparing equal).
Fix: op_packed_header_volatile_regions() (op.h / oppack_v7.c) derives the table
with offsetof()/sizeof() on the real struct, so a caller cannot drift from the
layout -- the same anti-drift principle already used for the list header.
outlinesignature is NO LONGER skipped. It was inherited speculation; dumping
real packed values shows it is the constant 'LAND' throughout, and skipping a
field with no demonstrated volatility is the same failure class as the two
defects above. Only proven-volatile fields are excluded.
Offset attribution corrected while here: the original 3,527 spurious findings
were 3,495 at offset 37 and 27 at offset 49 -- BOTH ctsaves. Offset 37 is its
last byte in a bare packed outline (34..37); offset 49 is its last byte inside a
packed list, displaced by the 12-byte list header (12+34=46..49). The data was
right; the field names were not.
TESTS: 6 new boundary tests pin the derived regions (size 1068, exactly the
three proven fields, fltextmode excluded, timecreated fully covered, too-small
buffer reports the needed size, NULL rejected). Mutation-tested: reinjecting the
+6 error fails exactly the two tests that pin it. 17/17 total.
Also added ../Common/source/oppack_v7.c and op.h as prerequisites of the
diff_roots_tests target -- without them a stale binary silently passed after the
region table changed, which is how the first mutation-test attempt gave a false
green.
WHY THE ORIGINAL EVIDENCE MISSED BOTH: self-diff compares a file with itself, so
no skip window and no vacuous path can surface. The re-save experiment cannot
distinguish the two offset tables, because a re-save alters neither timecreated
nor the bytes the wrong window covered. And no fixture contained a differing
menubar, so the mbar path was never asked to detect anything -- "43 menubars
equal across two eras" measured agreement, not detection.
New rule, applied from here: every compare path needs a fixture proving it
DETECTS, not merely that it agrees.
Refs #896, #878
… ut-sync refusal
P1-3 -- SORTED-MERGE ASSUMPTION WAS WRONG, AND IT MASKED PAYLOAD COMPARISONS.
walkpair merged the two tables positionally over hfirstsort, on the assumption
that list is alphabetical. It is not in this build: sorted insertion consults
langcallbacks.comparenodescallback (langhash.c:1464), and headless startup wires
that to cb_noop_compare, which always returns 0 (langstartup.c:81, :1172). Every
node therefore appends, and hfirstsort is INSERTION order.
Reproduced: two roots holding the same three values inserted in different orders
produced FOUR spurious findings, each value reported as both only-in-A and
only-in-B -- and their payloads were never compared at all. Not just noise: a
masked comparison, which is the same silent-false-equality class as the P0s.
Fix: collectnodes() buffers each table and sorts by raw name bytes (the same
comparison used for matching), then walkpair merges the sorted arrays. Matching
is now correct regardless of stored order, and output is deterministic even
between roots whose insertion histories differ -- which is what a drift check
needs. Verified: the divergent-order fixture goes 4 findings -> 0.
P1-4 -- ut-sync COULD WRITE DURING A "READ-ONLY" WALK.
ut-sync's import hook rewrites outlines, sets dirty flags, and writes
.ut-sync-state. Nothing excluded it from this mode, and FRONTIER_UT_SYNC_DIR
enables it from the environment (cli_parser.c populates ut_sync_dir from the env
before validation runs), so it could be on with no flag on the command line.
Fixed at two layers:
- cli_validate_options rejects --diff-roots combined with --ut-sync-dir or
FRONTIER_UT_SYNC_DIR, loudly, naming both sources.
- diff_roots_compare refuses independently by checking the env var. Structural,
not incidental: this entry point is callable directly (the unit tests do),
and the module must not depend back on main.c -- an earlier attempt called
cli_get_ut_sync_dir() and produced a dyld "symbol not found" at runtime
rather than a link error, because the test target links with
-Wl,-undefined,dynamic_lookup.
Acceptance proof, properly isolated: same two genuine v7 roots, same command,
env var the only variable -- "no differences" exit 0 without it, loud rejection
exit 1 with it.
TEST HONESTY NOTE: the ut-sync unit test asserts the refusal contract but CANNOT
prove the guard is what refused, because every input a unit test can supply
without a multi-MB fixture (missing file, non-v7 file) is rejected by an earlier
check anyway. Mutation-tested and confirmed: removing the guard leaves that test
passing. Rather than ship a test that certifies nothing, its comment states the
limitation and points at the acceptance-layer proof that does isolate it. An
earlier draft of this test tried to dodge the problem by passing a readable .c
file; that fails on the v7 check instead, so it was no better.
ACCEPTANCE AFTER BOTH FIXES: self-diff empty on all three roots; mbar fixture
detects 1; divergent-order fixture 0; cross-era exactly 3 attributable; mutation
exactly 2. All inputs md5-identical; canonical databases/ untouched. 18/18 unit.
Refs #896, #878
…path growth closeside leaked both the database record and the file. dbclose() only flushes the header (db.c:4823); it does not free databasedata or close the file. Now dbclose -> dbdispose -> closefile, in that order, with databasedata pointed at the side being torn down first. Ordering is load-bearing, not cosmetic: dbzeroreleasestack's comment (db.c:4500-4510) records a real bug where a guard restored databasedata and dbdispose then freed it, leaving a dangling global that segfaulted at exit -- which is why odb_guard_exit stays AFTER both closeside calls. reportsubtree stopped at any non-resident subtable, so a present-on-one-side-only subtree was named but its contents were not. Same blind spot the .ut exporter has (main.c:2444), and unacceptable in a diff report. It now materializes read-only via the side's context, and reports loudly if that fails. Verified: a fixture adding a nested table now enumerates the table, the inner table, both leaves and the sibling scalar (5 findings) instead of stopping at the top. singlevaluetype compared with ==, which is the wrong predicate for a stored-bytes comparison twice over: NaN != NaN reports identical stored values as different, and +0.0 == -0.0 reports different stored values as identical. Now memcmp. Path buffers now GROW to fit (pathbufsize: parent + separator + 3 bytes per raw name byte + NUL) instead of every allocation being a flat 8KB. The commitment was a growable buffer; maxdiffpath survives only as a sanity ceiling. A diff tool must never fail to NAME a path -- the failure mode both existing ODB walkers have (main.c:2429, langhash.c:297). High-bit bytes are escaped at the REPORT layer only. The shared codec keeps passing >= 0x80 through untouched -- it is a lossless bijection and MacRoman must survive it byte-for-byte (#880) -- but raw high bytes on a terminal render through whatever locale is present, so display escapes them. Findings handed to a callback still carry raw bytes. Documented rather than speculatively implemented: pict (and filespec/alias/code scalars) occur in NO shipped root per the 2026-08-11 census, so they stay on the loud-fail path. Writing comparison code that cannot be exercised is how untrusted code ships; a red build naming the path is the better outcome. Corrected the header's read-only claim to say what is actually guaranteed: the walker brings values into memory and sets residency flags, but never sets a dirty flag or writes a block, and disk-value reads stop at dbrefhandle_context rather than the kernel resolver, which would dirty the owning table (langhash.c:2211). Acceptance re-verified, all unchanged: self-diff empty x3; mbar detection 1; divergent-order 0; cross-era 3 attributable; mutation 2. All inputs md5-identical; canonical databases/ untouched. Unit 18/18. Filed #898 for the pre-existing signed-overflow UB in db.c's block-size computation (ctbytes - variance, both operands read from disk, guarded by a post-hoc ct < 0 test that UB permits the optimizer to discard). Refs #896, #898, #878
…ides Two review rulings, both of which produced better outcomes than what I had. GUARD-FIRST ORDERING MAKES THE REFUSAL TESTABLE. Previously the ut-sync guard sat behind the argument checks, which made it unprovable: every input a unit test can supply is rejected for some other reason first, so a test could not distinguish the guard firing from a file-not-found. Verified by stubbing the guard to if(0) -- the old test still passed, certifying nothing. Now the guard is checked FIRST in diff_roots_compare, before any input validation, which is also the honest ordering: it is a mode-compatibility precondition, not a statement about the arguments. Its refusal has a distinct observable (diff_roots_utsyncrefusal, exported from the header) so a test can assert on THIS refusal rather than on "returned false". The tests now discriminate three ways: env set -> refused AND the refusal message appears; env absent, same bogus paths -> refused for a DIFFERENT reason and no refusal message; env set but empty -> feature off, no refusal. Re-ran the mutation: stubbing the guard now FAILS the test that asserts on it, where before it passed. The acceptance-layer isolation proof is kept as well, since it exercises the real-roots path. NEVER-STORED ON BOTH SIDES IS NOW DEFINED, NOT ACCIDENTAL. Removing the nil-means-empty-handle rule (the cause of the vacuous mbar compare) left an undefined edge: an external with no stored block on EITHER side would have been reported unreadable, which would break the self-diff law -- a root compared against itself must produce no findings. Two genuinely never-stored values ARE equal; there is no content on either side to differ. So: no finding, plus an auditable note on stderr naming the path and reason, keeping the findings stream on stdout clean for diffing. The exit code tells the truth in both directions and a human can still audit the cases. Shipped roots contain zero such values (every mbar and wptx resolves through the variabledata fallback -- confirmed, the note never fires on any of the three), but the tool has to be correct in general, not merely on today's inputs. One side stored and the other not remains a real finding, unchanged. Acceptance re-verified after both changes: self-diff empty x3; mbar detection 1; divergent-order 0; cross-era 3 attributable; never-stored notes 0; ut-sync refused with env set and clean without. Inputs md5-identical. Unit 20/20. Refs #896, #878
…CTION BLOCKING gate item: the A/B mbar experiment, the divergent-order 4->0, and the subtree 1->5 existed only in commit narrative. Three fixes were revert-invisible -- revert storedaddressof, collectnodes, or the reportsubtree descent and a self-diff-only suite stayed green. tests/diff_roots_fixtures_test.sh, 13 cases, wired as test-diff-roots-fixtures. NO BINARY FIXTURES COMMITTED: every root is generated at run time by copying databases/StartupTasks.root (29 KB, v7, already in-repo) or Virgin.root and flipping known bytes. Deterministic -- same inputs, same offsets, same results. Coverage, each proving DETECTION rather than agreement: identical -> 0 findings, exit 0 script-text byte -> exactly 1 finding, exit 1 ctsaves byte (skip region) -> 0 findings fltextmode byte -> 1 finding <- the byte the +6 table swallowed outlinesignature byte -> 1 finding <- semantic content, must not be skipped skip-window boundary: 34 ignored, 39 reported missing input -> exit 2, distinct from 0 and 1 ut-sync active -> refused menubar content changed -> 1 mbar finding; unmutated -> 0 divergent insertion order -> 0 findings one-sided subtree -> all 5 nested values enumerated inputs md5-identical after comparison REVERT-VERIFIED, which is the point of the exercise. Each fix was individually reverted and the suite re-run: revert P0-1 (variabledata fallback) -> mbar fixture reports 0, FAILS revert P0-2 (offsets back to +6) -> fltextmode, signature and boundary FAIL revert P1-3 (drop the qsort) -> order fixture reports 4, FAILS revert P2 (subtree early return) -> subtree fixture reports 1 not 5, FAILS THE FIXTURES IMMEDIATELY FOUND A REAL BUG. A missing or unreadable database exited 1, not 2 -- indistinguishable from "differences found", so a broken CI drift check would have read as a genuine drift report. Cause: an unreadable root is rejected in cli_validate_options, and main.c's generic parse-failure path returns 1 before --diff-roots dispatch is reached. Fixed at that site by returning 2 when the mode is --diff-roots (read before cli_free_options frees it). This is exactly why the gate required committed fixtures. Also in this round, per review: - outlinesignature allowlist comment now cites the DECISIVE evidence: it selects the OSA server (scripts.c:943), drives tree building (:1302), and gates UserTalk text handling (:3049, outlinesignature == typeLAND). It is semantic content -- a difference means a different LANGUAGE -- which is far stronger than "observed constant". - emitpath() factored out; the never-stored audit note was emitting a raw path while findings were escaped. Single escaped emitter now, class closed. - odb_guard comment reworded to defense-in-depth. The claim that opening a second database clears table-structure globals was DISPROVED by audit: dbopenfile touches only databasedata; cleartablestructureglobals is called from cancoon.c:601 and the compaction path, neither reached here. Guard kept for an honest reason, with the mode-globals note added. - Probe attribution clarified: it was THIS UNIT'S OWN scratch probe that omitted pragma pack(2), not a reviewer's. - Stale file-header claim corrected (hfirstsort is insertion order, not alphabetical) -- it described the pre-P1-3 design. - P3 notes: loadheaderregions cache not thread-safe (fine in the early-exit band); qsort duplicate-name assumption; mbar interior-address known-loud note. Unit failure SET matches baseline exactly on a SERIAL run (26 suites, plus test_callback_infrastructure SIGSEGV on "Empty callback"). Noted for the record: a -j8 run surfaced the crash in test_window_bridge instead, which passes 1/1 standalone -- the parallel runner's interleaving moves which suite dies within the pre-existing crash family. Serial is the reliable comparison. Refs #896, #901, #878
Root-build Step 1 —
--diff-rootsvalue-level equality walkerBuilds the acceptance instrument that every later step of
planning/ROOT_BUILD_FROM_SOURCE_PLAN.mddepends on. The exporter/assembler round-trip law and the CI drift check both reduce to "run this and require an empty diff", so this tool has to be trustworthy before anything is built on top of it.What it does
Recursive value-level comparison of two v7 ODB roots. Exit
0= identical,1= differences found,2= operational error. Those last two are deliberately distinct: a broken CI check must never look like a clean comparison.Pure C throughout — no interpreter, no
string()coercion (which hard-aborts onlistvalues throughtry, #887), nohashgetvaluestring(deparses, would false-diff). Both roots are opened read-only;dbwriterefuses writes to aflreadonlydatabase (Common/source/db.c:715-728), so the guarantee is enforced below us as well. Dispatched frommain.c's early-exit band beside--migrate, so it returns before the runtime boots and can never reach the save-on-exit path.Comparison basis per type
scpt/optxoppackoutlinebytes, volatile header excludedtabllist/recooppacklistbytes, same exclusion at a displacementlanghash.c's own scalar packer uses (:3204-3207); sidesteps #887 rather than working around itmbarwptxdb_format.c:1694)langhash.cpack switch????novaluetypelangpack.c: "nothing to pack")unsupported-typefindingmbarandwptxcarry distinct reason codes in the output, because they are excluded for unrelated reasons and merging the two rationales would mislead a later reader.Not
langexternalpack(): its outline branch callsdbassignhandle_context(opverbs.c:1494) and writes disk blocks. That is a save path wearing a "pack" name.The finding of the unit: packed bytes carry save metadata
The determinism spike was necessary but NOT sufficient. It opened each root read-only, packed every value, tore the database all the way down, reopened, packed again, and compared — 3,499 / 394 / 1,285 values across the three shipped roots, zero mismatches, zero dirty-flag flips, files md5-identical. It reported GREEN.
It was still wrong, because both independent loads increment
ctsavesidentically, so dirty-flag checking alone could never see the problem. Only cross-copy comparison exposed it: diffing a root against a copy that had been opened and saved once produced 3,527 spurious findings, 3,495 of them differing first at offset 37 — insidetimelastsave.The packed-outline header (
oppack_v7.c:115-152) carries save metadata that must be excluded from an equality basis:timecreatedtimelastsavectsavesoppackitself (oppack_v7.c:622) and written back into the outline record, so it differs even between two packsoutlinesignaturePacked lists inherit the same fields, because
oppacklistemits a list header followed by a packed outline (oplist.c:708-711). The displacement is read from the list header's ownrecordsizefield (oplist.c:88, "number of bytes in this header") rather than hardcoded, so the walker cannot silently drift out of agreement with the format iftydisklistrecordchanges shape.Excluding those regions took the mutation diff from 3,527 findings to 2.
Acceptance evidence
Every input md5-identical before and after every run; canonical
databases/untouched;git statusclean.Self-diff, all three shipped roots — empty, exit 0. Exercises every live type including the string() coercion on list-type value HARD-ABORTS interpreter (exit 1, uncontainable); blocks --export-root #887 lists and the 106
????placeholders.Virgin vs one-scripted-mutation copy — exactly 2 findings: the probe (
only-in-b workspace.diffRootsProbe), plusFrontier.pathstring85→113 bytes. That second one was verified, not assumed: reading it back shows it now holds the opening process's own path — self-updating machine state, the Virgin.root polluted with absolute paths into deleted p1-batch-fixes worktree (serverMonitor postFilter errors on every webserver response) #841 class.Cross-era
ced824160→ develop — exactly 3 findings, all attributable, none unexplained:string.isValidUrl+webBrowser.openUrl→5bb712595(LF-single-node script corruption in Virgin.root (embedded LF, zero CR); webBrowser.openURL root cause, #855 unblocker #881 LF repair)system.startup.startupScript→21d33d2fd(startupScript() lacks error recovery path to clear system.temp.Frontier.startingUp, leaving webserver hung on error #849/mainResponder.respond() returns HTTP 200 for errors; vacuous test greens, broke monitoring semantics #862 guards)Those are the only two commits touching
Virgin.rootin that range.Composability —
ced824160vs mutated yields exactly 5, the clean union of (2) and (3), no cross-talk.mbarpath proven non-vacuous — across two files with different sizes and block layouts, all 43 Virgin menubars compare equal by stored bytes, so that path reads content rather than addresses.Two bugs found by crashing first
flinmemoryis a false positive formbarheadless — filed as #896.menuverbinmemory_context(stubs/headless_menu_stubs.c:618) setsflinmemory = truewithout loading, leavingvariabledataa rawdbaddress; dereferencing segfaults. Independently corroborated: that same value is the onembarthe 2026-08-11 census also failed to measure (size-1). Residency flags are not per-type trustworthy in headless builds — a landmine for every future walker.fldiskvalvalues — an unresolved value'sdata.binaryvalueis adbaddress, not a pointer. Resolved viadbrefhandle_contextand deliberately not written back, because the kernel's own resolver dirties the owning table as a side effect (langhash.c:2211).Also: path buffers are heap-allocated.
maxdiffpathis deliberately generous (a diff tool must never fail to name a path) and the walk recurses, so stack buffers overflowed the default stack.Tests
tests/diff_roots_tests.c— 11/11. Behavioral: escaping losslessness, the 0x0D/0x0A sibling non-collision, the whole C0 range,.-is-always-a-separator, MacRoman byte preservation, overflow reported rather than swallowed, operational failure distinguishable from differences, kind-name stability.test_callback_infrastructureSIGSEGV (boxen_outline_tests: deterministic SIGSEGV from 8.9MB stack-local state struct #827/Latent: in-memory hash node takes on-disk resolution path -> NULL deref (dbgeteof via hashresolvevalue) #828 family), make exit 139.6a9895d95. This PR adds no integration tests, so that is the expected post-fix(cli): #891 -- headless sys.openUrl logs the setup URL instead of launching a host GUI browser #895 shape.Known caveat:
diff_roots_testsis registered inRUN_BUILDABLE, but the suite aborts at that earlier pre-existing crasher before reaching it, so in-suite it currently never runs. Standalone it is 11/11 and it appears in the OPML manifest (11 tests, 11 pass). This is a pre-existing suite-abort problem, not introduced here.Names are bytes
ODB names legitimately contain tabs and raw CR/LF.
Virgin.roothas sibling values named with the single bytes0x0A/0x0B/0x0C/0x0Dunder...aim.code.util.repl; two of them collide under newline normalization (census CAVEAT 4 — 11,257 rows, 11,255 unique paths), which would mean a phantom finding or a swallowed one.Names are therefore compared as raw bytes and only escaped for reporting, reusing the existing lossless
ut_pct_encode_segment. Because.encodes to%2E, a.in a reported path is always a separator. A call-site comment marks that the ut-sync-removal unit (plan Step 8) must lift that function, not delete it.Carry into Step 2 (exporter)
views[0]differs by version. For v7 it is the root table address; for v6 it points at a Cancoon record whoseadrroottableholds it (odbengine.c:566-570). Reading v7's as a Cancoon record yields0x4fe0000, whichdbnormalizeaddresscorrectly rejects. This tool accepts v7 only and rejects anything else loudly rather than misinterpreting it.tyversion2cancoonrecordmust not be used to read that record. It compiles to 474 bytes here against a 442-byte on-disk record (assert atcancoon.c:636,kCancoonBytesatdb_format.c:580) becausedbaddresswidened to 64-bit — every field afterversionnumbersits at the wrong offset. Decode the stored bytes explicitly.ctsaves/timelastsaveexclusion, and the spike-vs-cross-copy lesson above: two independent loads are not enough to detect it.timecreated/timelastsavemakes this tool blind to create/mod-date drift. That is correct for Step 1 drift detection — a re-saved root with identical content must compare equal or the gate is useless — and it is covered long-term by plan decision 9.4: the exporter preserves per-value dates in source manifests, so the round-trip law checks dates as manifest text, not as packed bytes. Content equality here, date equality there; neither check subsumes the other. This is not a hole.fldiskvalhandling and the flinmemory is a false positive for mbar externals in headless builds; dereferencing variabledata segfaults #896flinmemorycaveat (see above). Note also thatut_export_walk_table(main.c:2444) skips non-resident subtables entirely — for an exporter that means silently omitting whole subtrees.Repo hygiene note (not addressed here)
tools/run_headless_tests.shhardcodesmakewith noCC, so it fails outright under the broken-Homebrew-clang SDK issue. Unit runs needmake -C tests test CC=/usr/bin/clang. Flagging rather than fixing — out of scope for this PR.Refs #878, #887, #881, #890, #841, #896, #864
Fix round (gate FAIL -> addressed)
The first submission gated FAIL on two P0s, both in the compare core, both in the silent-false-equality class this tool's own design forbids — and both survived the original acceptance evidence. That last part is the most important thing in this PR.
P0-1 — the mbar comparison was vacuous
readstoredbyteswas handed(**hv).oldaddress, but for a never-materialized external that field isnildbaddress:langnewexternalvariableinitializes it to nil (langexternal.c:2932,:2997) and the stored address lives invariabledata(menuverbunpack,menuverbs.c:501-509). Both sides read nil, hit a nil-means-empty-handle rule, and every menubar and wptext compared equal regardless of content.Verified directly: all sampled Virgin menubars have
oldaddress=0x0with real addresses invariabledata.Fixed by
storedaddressof()falling back tovariabledata(mirroringheadless_menu_stubs.c:551-553), and by removing the nil-means-empty exception — that exception is what made the failure silent, so fixing only its input would have left the trap armed.A/B proof. On a fixture whose only change is one byte inside a menubar's stored block: the pre-fix binary reports
no differences; the post-fix binary reports exactly one payload finding at that menubar. The old binary was rebuilt specifically to demonstrate the miss rather than assert it.P0-2 — skip-region offsets were wrong by +6
The regions were hardcoded from a probe of
typortablediskheaderwritten without#pragma pack(2), which reported 24/32/40/48 and a 1080-byte header. The real packed layout istimecreated=18,timelastsave=26,ctsaves=34,fltextmode=38,outlinesignature=40, total 1068 — andoppack_v7.c:154already asserts 1068, so the wrong table contradicted an existing invariant that was never checked against it. Probes need their own invariant checks; that is the same instrument-validation lesson as the rest of this unit, one level down.Consequences:
timecreated18-23 was not skipped (false diffs whenever creation stamps differ — breaking the round-trip law by construction), andfltextmode38-39 was skipped (real content comparing equal).Fixed by
op_packed_header_volatile_regions()deriving the table withoffsetof()/sizeof()on the real struct — the same anti-drift principle already used for the list header.Offset attribution corrected
The original 3,527 spurious findings were 3,495 at offset 37 and 27 at offset 49. Both are
ctsaves— offset 37 is its last byte in a bare packed outline (34..37), offset 49 its last byte inside a packed list, displaced by the 12-byte list header (12+34=46..49). Dumped from real packed lists in Virgin.root:This supersedes the review's 6-byte-displacement attribution. The data was right; the field names were not.
The skip list is an allowlist of proven volatility
Only
timecreated,timelastsave,ctsaves— each with a demonstrated mechanism.outlinesignatureis deliberately NOT skipped: it reads as the constantLANDthroughout the shipped roots, and pre-swallowing a field with no demonstrated volatility is the same failure class as both P0s. If a signature difference ever surfaces it must surface as a finding and be attributed — that is the tool working, not failing.P1s
Sorted-merge assumption was wrong, and it masked payload comparisons.
hfirstsortis insertion order in this build: sorted insertion consultslangcallbacks.comparenodescallback(langhash.c:1464) and headless startup wires that tocb_noop_compare, which always returns 0 (langstartup.c:81,:1172). Two roots holding the same three values inserted in different orders produced four spurious findings, each value reported as both only-in-A and only-in-B, and their payloads were never compared. Fixed with buffer-and-sort by raw name bytes; the divergent-order fixture goes 4 → 0.ut-sync could write during a "read-only" walk. Its import hook rewrites outlines, sets dirty flags, and writes
.ut-sync-state, andFRONTIER_UT_SYNC_DIRenables it from the environment with no flag present. Now refused at both layers.P2s
closesideleaked the database record and the file descriptor —dbclose()only flushes the header (db.c:4823). Nowdbclose→dbdispose→closefile, withodb_guard_exitstaying after both sides (db.c:4500-4510documents the dangling-databasedatasegfault that ordering avoids).reportsubtreeno longer stops at non-resident subtables (fixture: 1 finding → 5).singlevaluetypeusesmemcmprather than==, since NaN and signed zero both make float equality the wrong predicate for a stored-bytes comparison. Path buffers grow to fit. High-bit bytes escape at the report layer only — the shared codec stays lossless.Documented rather than speculatively implemented:
pict,filespec,aliasandcodeoccur in no shipped root per the census, so they stay on the loud-fail path. Writing compare code that cannot be exercised is how both P0s reached review.Why the original evidence lied
timecreatednor the bytes the wrong window covered.Rule adopted: every compare path needs a fixture proving it DETECTS, not merely that it agrees.
Two test-integrity failures found in my own process
Stale binary defeated a mutation test. My first mutation test of the new boundary tests passed with the +6 bug reinjected —
diff_roots_testsdid not listoppack_v7.cas a prerequisite, somakenever relinked. Forcing a rebuild failed the two pinning tests correctly. A mutation test that certifies a test which is not running is the worst failure mode a suite has. Prerequisite fixed here; filed as #901 because the exposure is repo-wide.A test that could not discriminate. The ut-sync unit test asserted only "returned false", which every bad input also produces — stubbing the guard left it passing. Fixed structurally rather than by relabeling: the guard is now checked first (it is a mode precondition, not an input check) and emits a distinct message, so the test asserts on that refusal. Mutation re-run: stubbing the guard now fails the test that asserts on it, where the previous version passed.
Never-stored on both sides
Removing the nil-equals-empty exception left an edge undefined. Two genuinely never-stored values are equal — no content on either side to differ — so: no finding, plus an auditable stderr note naming path and reason, keeping stdout clean for diffing and the exit code honest in both directions. One side stored and the other not remains a real finding. Confirmed the note fires zero times across all three shipped roots; this is correctness in general, not a live case.
Fix-round verification
diff_roots_tests20/20; failure SET matches baselineAll inputs md5-identical after every run; canonical
databases/untouched.Issues filed this round: #898 (pre-existing signed-overflow UB in
db.c), #901 (Makefile prerequisite / stale-binary class).Fixture round (delta re-gate -> addressed)
The delta gate confirmed every code fix but blocked on the fixtures existing only in commit narrative. Three fixes were revert-invisible: revert
storedaddressof,collectnodes, or thereportsubtreedescent and a self-diff-only suite stayed green.tests/diff_roots_fixtures_test.sh— 13 cases, wired astest-diff-roots-fixtures. No binary fixtures are committed: every root is generated at run time fromdatabases/StartupTasks.root(29 KB, v7, already in-repo) orVirgin.rootby copying and flipping known bytes. Deterministic.Each case proves detection, not agreement: identical→0/exit 0; script-text byte→1/exit 1;
ctsaves→0 (skip region);fltextmode→1 (the byte the +6 table swallowed);outlinesignature→1; skip-window boundary (34 ignored, 39 reported); missing input→exit 2; ut-sync refused; menubar content→1 mbar finding and unmutated→0; divergent insertion order→0; one-sided subtree→all 5 nested values; inputs md5-identical.Revert-verified. Each fix was individually reverted and the suite re-run:
variabledatafallback)fltextmode, signature, boundary → FAILqsort)The fixtures immediately found a real bug
A missing or unreadable database exited 1, not 2 — indistinguishable from "differences found", so a broken CI drift check would have read as a genuine drift report. An unreadable root is rejected in
cli_validate_options, andmain.c's generic parse-failure path returns 1 before--diff-rootsdispatch is reached. Fixed at that site. This is precisely why committed fixtures were the right gate condition.Also in this round
outlinesignature's allowlist comment now carries the decisive evidence: it selects the OSA server (scripts.c:943), drives tree building (:1302), and gates UserTalk text handling (:3049,outlinesignature == typeLAND). It is semantic content — a difference means a different language — which is far stronger than "observed constant".emitpath()factored out: the never-stored audit note emitted a raw path while findings were escaped. Single escaped emitter now.The
odb_guardcomment is reworded to defense-in-depth. Its claim that opening a second database clears the table-structure globals was disproved by audit:dbopenfiletouches onlydatabasedata;cleartablestructureglobalsis called fromcancoon.c:601and the compaction path, neither reached here. The guard is kept for an honest reason, with the mode-globals note added.Probe attribution clarified (it was this unit's own scratch probe that omitted
pragma pack(2)), the stale file-header claim abouthfirstsortbeing alphabetical corrected, and P3 notes added for the region cache's thread-safety scope, the qsort duplicate-name assumption, and the mbar interior-address known-loud behavior.Verification
Unit failure set matches baseline on a serial run (26 suites, plus the pre-existing
test_callback_infrastructureSIGSEGV). Worth recording: a-j8run surfaced the crash intest_window_bridgeinstead, which passes 1/1 standalone — the parallel runner's interleaving moves which suite dies within the pre-existing crash family, so serial is the reliable comparison.diff_roots_tests20/20; fixtures 13/13; integration 2492 / 2314 passed / 161 skipped / 0 failed, exit 0.