From 72edb7026528aa1071323a8b8f5bc935e1b720d6 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:21:58 -0700 Subject: [PATCH 01/10] test(odb): Step 1 spike -- qualify oppackoutline as diff-roots equality 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/Makefile | 8 + tests/diff_roots_spike.c | 464 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 472 insertions(+) create mode 100644 tests/diff_roots_spike.c diff --git a/tests/Makefile b/tests/Makefile index b746a9b0d..a609e67fd 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -279,6 +279,14 @@ oppack_tests: oppack_tests.c $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -I../Common/headers -I../Common/SystemHeaders -I../portable \ oppack_tests.c $(LANG_RUNTIME_SOURCES) ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) +# Determinism spike for --diff-roots (root-build Step 1). Deliberately NOT in +# RUN_BUILDABLE: it takes a .root path argument and is run by hand to qualify +# oppackoutline as the equality basis for script/outline/menubar values. +# See tests/diff_roots_spike.c header for what it proves and why. +diff_roots_spike: diff_roots_spike.c + $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -I../Common/headers -I../Common/SystemHeaders -I../portable -I../frontier-cli \ + diff_roots_spike.c $(LANG_RUNTIME_SOURCES) ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) + hashpack_modern_tests: hashpack_modern_tests.c $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -I../Common/headers -I../Common/SystemHeaders -I../portable \ hashpack_modern_tests.c $(LANG_RUNTIME_SOURCES) ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) diff --git a/tests/diff_roots_spike.c b/tests/diff_roots_spike.c new file mode 100644 index 000000000..174f530ba --- /dev/null +++ b/tests/diff_roots_spike.c @@ -0,0 +1,464 @@ +/* + SPDX-License-Identifier: MIT + + Copyright (c) 2026 Frontier contributors + + Determinism spike for --diff-roots (root-build Step 1). + + Question this answers: is oppackoutline() a usable EQUALITY BASIS for + script/outline/menubar values? It is only usable if packing the SAME stored + value across two INDEPENDENT loads of the same file yields identical bytes. + Packing twice within one load is NOT sufficient evidence -- volatile + in-memory-only fields (cursor, scroll position, expansion state) would be + stable across two packs of one resident value while differing across two + separate loads. + + So: open read-only -> walk -> pack every scpt/optx/mbar -> hash the packed + bytes -> tear the database all the way down -> reopen from scratch -> repeat + -> compare. Any per-path delta names a volatile field and disqualifies the + approach (falling back to a node walk over headstring/headlevel). + + Also asserts the file itself is byte-identical after the walk (md5 is checked + by the driver script), and that packing does not flip the outline record's + dirty flags. + + Build: see tests/Makefile target diff_roots_spike. +*/ + +#include +#include +#include + +#include "frontier.h" +#include "standard.h" +#include "shell_api.h" +#include "db.h" +#include "dbinternal.h" +#include "lang.h" +#include "langexternal.h" +#include "langinternal.h" +#include "op.h" +#include "oplist.h" +#include "opverbs.h" +#include "menueditor.h" /* tymenurecord.menuoutline -- a menubar contains an outline */ +#include "tablestructure.h" +#include "db_format.h" +#include "file.h" +#include "strings.h" +#include "memory.h" +#include "ut_sync.h" /* ut_pct_encode_segment -- the same lossless name codec + the walker will use for path reporting */ + +#define maxpathdepth 64 +#define maxrecords 20000 + +typedef struct tyspikerecord { + char path [512]; + unsigned long hash; + long packedsize; + tyexternalid extid; + } tyspikerecord; + +static tyspikerecord records [maxrecords]; +static long ctrecords = 0; +static long ctpackfailures = 0; +static long ctdirtyflips = 0; + +/* + Read context for the root currently being walked. Set once per load, passed + explicitly to ensure_external_in_memory so materialization never depends on + the databasedata global -- the same discipline the walker proper needs when + it holds two roots open at once. +*/ +static db_context g_walkcontext; + +/* + FNV-1a. We only need to detect difference, not resist collision -- and a + hash keeps the spike's memory flat across ~3,500 scripts. +*/ +static unsigned long hashbytes (const unsigned char *p, long ct) { + + unsigned long h = 2166136261UL; + long i; + + for (i = 0; i < ct; i++) { + h ^= (unsigned long) p [i]; + h *= 16777619UL; + } + + return (h); + } /*hashbytes*/ + + +static void appendsegment (char *path, size_t pathsz, const unsigned char *bsname) { + + /* + Percent-encode via the same codec the walker will use, so the spike's paths + are directly comparable to the walker's output. Names legitimately contain + tab and raw CR/LF (verified: siblings under ...aim.code.util.repl are named + with the single bytes 0x0A/0x0B/0x0C/0x0D), so raw concatenation would be + ambiguous. + */ + + char encoded [766]; /*255 * 3 + 1 -- max encoded Pascal name*/ + size_t len; + + if (!ut_pct_encode_segment ((const char *) &bsname [1], (size_t) bsname [0], + encoded, sizeof (encoded))) + strcpy (encoded, "%3CENCODE-OVERFLOW%3E"); + + len = strlen (path); + + if (len > 0) + snprintf (path + len, pathsz - len, ".%s", encoded); + else + snprintf (path, pathsz, "%s", encoded); + } /*appendsegment*/ + + +static void packone (hdlexternalvariable hv, tyexternalid extid, const char *path) { + + hdloutlinerecord ho; + Handle hpacked = nil; + boolean fldirtybefore, fldirtyviewbefore; + + if (!(**hv).flinmemory) /*handled by the caller; nothing resident to pack*/ + return; + + if (extid == idmenuprocessor) { + /* + MENUBARS CANNOT BE PACKED IN A HEADLESS BUILD. Two separate reasons, + both verified: + + 1. A menubar is not an outline -- variabledata would be an hdlmenurecord + (menueditor.h:142), whose content outline is (**hm).menuoutline + (menueditor.h:144). + + 2. More fundamentally: the headless menu stub + menuverbinmemory_context (frontier-cli/stubs/headless_menu_stubs.c:618) + sets flinmemory = true WITHOUT LOADING ANYTHING -- its own comment + says "Menu externals are not materialized during headless migration. + Mark as in memory to skip actual loading, but keep the address for + packing." So variabledata is still a raw dbaddress, and dereferencing + it as any record segfaults. flinmemory is a LIE for mbar values in + this build. + + This is why mainResponder.root's `menu` crashed the walk, and it is + almost certainly why the census reports size -1 for that same value. + + The walker proper must therefore compare mbar values at the STORED + level (raw disk bytes at the external's address) rather than by + materializing them -- reported honestly, never silently skipped. + */ + printf ("MBAR-NOT-COMPARABLE-HEADLESS\t%s\n", path); + return; + } + + ho = (hdloutlinerecord) (**hv).variabledata; + + if (ho == nil) { + printf ("NOOUTLINE\t%s\textid=%d\n", path, (int) extid); + return; + } + + fldirtybefore = (**ho).fldirty; + fldirtyviewbefore = (**ho).fldirtyview; + + /* + oppackoutline APPENDS when handed a non-nil handle (see oppack's header + comment), so hpacked must start nil. + */ + if (!oppackoutline (ho, &hpacked)) { + ctpackfailures++; + printf ("PACKFAIL\t%s\n", path); + return; + } + + if (((**ho).fldirty != fldirtybefore) || ((**ho).fldirtyview != fldirtyviewbefore)) { + ctdirtyflips++; + printf ("DIRTYFLIP\t%s\tdirty %d->%d view %d->%d\n", path, + (int) fldirtybefore, (int) (**ho).fldirty, + (int) fldirtyviewbefore, (int) (**ho).fldirtyview); + } + + if (ctrecords < maxrecords) { + tyspikerecord *r = &records [ctrecords++]; + + snprintf (r->path, sizeof (r->path), "%s", path); + r->packedsize = gethandlesize (hpacked); + r->hash = hashbytes ((const unsigned char *) *hpacked, r->packedsize); + r->extid = extid; + } + + disposehandle (hpacked); + } /*packone*/ + + +static void walktable (hdlhashtable htable, char *path, size_t pathsz, int depth) { + + hdlhashnode nomad; + + if ((htable == nil) || (depth > maxpathdepth)) + return; + + for (nomad = (**htable).hfirstsort; nomad != nil; nomad = (**nomad).sortedlink) { + + tyvaluerecord *val = &(**nomad).val; + bigstring bsname; + char childpath [512]; + hdlexternalvariable hv; + tyexternalid extid; + + gethashkey (nomad, bsname); + + snprintf (childpath, sizeof (childpath), "%s", path); + appendsegment (childpath, sizeof (childpath), bsname); + + if (getenv ("SPIKE_TRACE") != NULL) { + fprintf (stderr, "TRACE %s type=%d\n", childpath, (int) (*val).valuetype); + fflush (stderr); + } + + if ((*val).valuetype != externalvaluetype) + continue; + + hv = (hdlexternalvariable) (*val).data.externalvalue; + + if (hv == nil) + continue; + + extid = (**hv).id; + + if (extid == idtableprocessor) { + + /* + Subtables are NOT resident after tableloadsystemtable -- they fault + in on demand. The .ut export walk (main.c:2444) simply skips + non-resident subtables, which for a diff tool would silently omit + whole subtrees. So materialize read-only instead. tableverbinmemory + only flips flinmemory (a residency flag); it sets no dirty flag and + writes nothing, and the database is open flreadonly regardless. + */ + if (!(**hv).flinmemory) { + + if (!ensure_external_in_memory (&g_walkcontext, hv)) { + printf ("LOADFAIL\t%s\ttable\n", childpath); + continue; + } + } + + walktable ((hdlhashtable) (**hv).variabledata, childpath, pathsz, depth + 1); + + continue; + } + + /* + The spike measures exactly the three types the walker intends to compare + via packed bytes. wptext (idwordprocessor) is deliberately NOT touched: + force-materializing those exhausts memory on a full-root walk + (db_format.c:1694). + */ + if (extid == idmenuprocessor) { + /* + Do NOT materialize: the headless stub fakes flinmemory without + loading (see packone). Report and move on. + */ + printf ("MBAR-NOT-COMPARABLE-HEADLESS\t%s\n", childpath); + continue; + } + + if ((extid == idscriptprocessor) || (extid == idoutlineprocessor)) { + + if (!(**hv).flinmemory) { + + if (getenv ("SPIKE_TRACE") != NULL) { + fprintf (stderr, " materialize extid=%d %s\n", (int) extid, childpath); + fflush (stderr); + } + + if (!ensure_external_in_memory (&g_walkcontext, hv)) { + printf ("LOADFAIL\t%s\textid=%d\n", childpath, (int) extid); + continue; + } + } + + if (getenv ("SPIKE_TRACE") != NULL) { + fprintf (stderr, " pack extid=%d %s\n", (int) extid, childpath); + fflush (stderr); + } + + packone (hv, extid, childpath); + } + } + } /*walktable*/ + + +static boolean loadandpack (const char *cpath) { + + bigstring bspath; + tyfilespec fs; + hdlfilenum fnum = 0; + dbaddress rootaddress = nildbaddress; + Handle hrootvariable = nil; + hdlhashtable hroot = nil; + char path [512]; + + if (!copyctopstring (cpath, bspath)) { + fprintf (stderr, "spike: path too long: %s\n", cpath); + return (false); + } + + if (!pathtofilespec (bspath, &fs)) { + fprintf (stderr, "spike: pathtofilespec failed for %s\n", cpath); + return (false); + } + + /*Both opens read-only: the file must be byte-identical afterwards.*/ + if (!openfile (&fs, &fnum, true)) { + fprintf (stderr, "spike: openfile failed for %s\n", cpath); + return (false); + } + + if (!dbopenfile (fnum, true)) { + fprintf (stderr, "spike: dbopenfile failed for %s\n", cpath); + closefile (fnum); + return (false); + } + + db_context_init (&g_walkcontext); + g_walkcontext.database = databasedata; + g_walkcontext.mode = db_format_mode_current (); + + dbgetview (cancoonview, &rootaddress); + + if (rootaddress == nildbaddress) { + fprintf (stderr, "spike: dbgetview returned nil root address\n"); + dbclose (); + return (false); + } + + /* + NO Cancoon indirection for v7. + + odbengine.c:566-570 states the rule: for v6 databases views[0] points at a + Cancoon record that in turn holds adrroottable, but for v7 databases + views[0] IS the root table address (or nildbaddress when empty). All three + shipped roots are v7 (header starts 00 07), so the address dbgetview just + returned is already what tableloadsystemtable wants. + + Reading it as a Cancoon record instead produced 0x4fe0000 -- a non-block + address that dbnormalizeaddress correctly rejected. + + A v6 input would need the Cancoon path; this spike does not accept one, and + the walker proper will reject non-v7 roots explicitly rather than + misinterpret them. + */ + + if (!tableloadsystemtable (rootaddress, &hrootvariable, &hroot, false)) { + fprintf (stderr, "spike: tableloadsystemtable failed\n"); + dbclose (); + return (false); + } + + path [0] = '\0'; + + { + long ctnodes = 0; + hdlhashnode n; + + for (n = (**hroot).hfirstsort; n != nil; n = (**n).sortedlink) + ctnodes++; + + fprintf (stderr, "spike: root table adr=0x%llx has %ld top-level nodes\n", + (unsigned long long) rootaddress, ctnodes); + } + + walktable (hroot, path, sizeof (path), 0); + + dbclose (); + + return (true); + } /*loadandpack*/ + + +int main (int argc, char **argv) { + + const char *dbpath; + long ctfirst; + long i; + long ctmismatch = 0; + static tyspikerecord firstpass [maxrecords]; + + if (argc < 2) { + fprintf (stderr, "usage: diff_roots_spike \n"); + return (2); + } + + dbpath = argv [1]; + + if (!db_format_prepare_runtime ()) { + fprintf (stderr, "spike: db_format_prepare_runtime failed\n"); + return (2); + } + + /*PASS 1*/ + ctrecords = 0; + + if (!loadandpack (dbpath)) + return (2); + + ctfirst = ctrecords; + memcpy (firstpass, records, (size_t) ctfirst * sizeof (tyspikerecord)); + + printf ("PASS1\tvalues=%ld\tpackfailures=%ld\tdirtyflips=%ld\n", + ctfirst, ctpackfailures, ctdirtyflips); + + /* + PASS 2 -- a completely independent load. dbclose() ran at the end of pass 1, + so nothing from pass 1's outline records survives into this pass. This is + what makes the comparison meaningful. + */ + ctrecords = 0; + ctpackfailures = 0; + + if (!loadandpack (dbpath)) + return (2); + + printf ("PASS2\tvalues=%ld\tpackfailures=%ld\tdirtyflips=%ld\n", + ctrecords, ctpackfailures, ctdirtyflips); + + if (ctrecords != ctfirst) { + printf ("VERDICT\tRED\tvalue count differs between loads: %ld vs %ld\n", + ctfirst, ctrecords); + return (1); + } + + for (i = 0; i < ctfirst; i++) { + + if (strcmp (firstpass [i].path, records [i].path) != 0) { + printf ("ORDERDIFF\t%ld\t%s\t%s\n", i, firstpass [i].path, records [i].path); + ctmismatch++; + continue; + } + + if ((firstpass [i].hash != records [i].hash) + || (firstpass [i].packedsize != records [i].packedsize)) { + printf ("BYTEDIFF\t%s\tsize %ld vs %ld\thash %lu vs %lu\n", + firstpass [i].path, + firstpass [i].packedsize, records [i].packedsize, + firstpass [i].hash, records [i].hash); + ctmismatch++; + } + } + + printf ("COMPARED\t%ld\tmismatches=%ld\n", ctfirst, ctmismatch); + + if ((ctmismatch == 0) && (ctdirtyflips == 0)) { + printf ("VERDICT\tGREEN\toppackoutline is stable across independent loads\n"); + return (0); + } + + printf ("VERDICT\tRED\tmismatches=%ld dirtyflips=%ld\n", ctmismatch, ctdirtyflips); + + return (1); + } /*main*/ From eb3cdcde2953cda083916308b41f67e5095705d7 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:22:33 -0700 Subject: [PATCH 02/10] chore: gitignore diff_roots_spike build artifact tests/*_tests does not match the spike's name. --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 3dfe15048..50801f247 100644 --- a/.gitignore +++ b/.gitignore @@ -27,6 +27,8 @@ tests/runtime_tests tests/cli_runtime_tests tests/*_tests tests/*_tests.dSYM/ +tests/diff_roots_spike +tests/diff_roots_spike.dSYM/ tests/verbs_test.tmp tests/table_operations_integration tests/time_portability_test From 5d8b479793b8466f8b72514b5714cb7ae9d73478 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:42:35 -0700 Subject: [PATCH 03/10] feat(odb): --diff-roots value-level equality walker (root-build Step 1) 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). - ced824160-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 (5bb712595, #881 LF repair) and startupScript (21d33d2fd, #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 --- frontier-cli/Makefile | 1 + frontier-cli/cli_parser.c | 70 ++ frontier-cli/cli_parser.h | 3 + frontier-cli/diff_roots.c | 1309 +++++++++++++++++++++++++++++++++++++ frontier-cli/diff_roots.h | 151 +++++ frontier-cli/main.c | 47 ++ tests/Makefile | 7 +- tests/diff_roots_spike.c | 22 - tests/diff_roots_tests.c | 239 +++++++ 9 files changed, 1826 insertions(+), 23 deletions(-) create mode 100644 frontier-cli/diff_roots.c create mode 100644 frontier-cli/diff_roots.h create mode 100644 tests/diff_roots_tests.c diff --git a/frontier-cli/Makefile b/frontier-cli/Makefile index c79116d86..e9247642a 100644 --- a/frontier-cli/Makefile +++ b/frontier-cli/Makefile @@ -130,6 +130,7 @@ CLI_SOURCES = \ window_registry.c \ ut_sync.c \ ut_scan.c \ + diff_roots.c \ debugger_tui.c \ boxen_repl.c \ boxen_completion_popup.c \ diff --git a/frontier-cli/cli_parser.c b/frontier-cli/cli_parser.c index 052ac3f9d..74274f3e3 100644 --- a/frontier-cli/cli_parser.c +++ b/frontier-cli/cli_parser.c @@ -167,6 +167,35 @@ boolean cli_validate_options(const cli_options_t* options) { boolean hydration_mode = options->hydrate_system_root; boolean migrate_mode = (options->migrate_database != NULL); + /* --diff-roots mode: compare two roots read-only and exit. Both paths are + required and both must be readable; the walker never writes either. */ + if (options->diff_roots_a != NULL) { + if (options->diff_roots_b == NULL) { + log_error(LOG_COMP_GENERAL, "Error: --diff-roots requires --against "); + return false; + } + if (!cli_file_exists(options->diff_roots_a) || !cli_file_readable(options->diff_roots_a)) { + log_error(LOG_COMP_GENERAL, "Error: Cannot read database: %s", options->diff_roots_a); + return false; + } + if (!cli_file_exists(options->diff_roots_b) || !cli_file_readable(options->diff_roots_b)) { + log_error(LOG_COMP_GENERAL, "Error: Cannot read database: %s", options->diff_roots_b); + return false; + } + if (migrate_mode || hydration_mode || options->system_root != NULL + || options->script_file != NULL || options->inline_script != NULL) { + log_error(LOG_COMP_GENERAL, "Error: --diff-roots runs standalone; do not combine with --migrate, --system-root, scripts, or REPL mode"); + return false; + } + return true; + } + + /* --against requires --diff-roots */ + if (options->diff_roots_b != NULL) { + log_error(LOG_COMP_GENERAL, "Error: --against requires --diff-roots"); + return false; + } + /* --migrate mode: migrate a database to v7 and exit */ if (migrate_mode) { if (!cli_file_exists(options->migrate_database)) { @@ -285,6 +314,8 @@ boolean cli_parse_arguments(int argc, char* argv[], cli_options_t* options) { OPT_UT_SYNC_DIR, OPT_DEBUG_TUI, /* 2026-06-06 JES Phase B.0 #691: --debug-tui */ OPT_PLAIN, /* 2026-06-25 JES C.6 #691: --plain */ + OPT_DIFF_ROOTS, /* root-build Step 1: --diff-roots A */ + OPT_AGAINST, /* root-build Step 1: --against B */ }; // Define long options @@ -293,6 +324,11 @@ boolean cli_parse_arguments(int argc, char* argv[], cli_options_t* options) { {"system-root", required_argument, 0, 'R'}, {"migrate", required_argument, 0, 'm'}, {"output", required_argument, 0, 'o'}, + /* Two-path mode. getopt_long has no two-argument option form, so the + second path arrives via a paired option -- the same shape --migrate + and --output already use. */ + {"diff-roots", required_argument, 0, OPT_DIFF_ROOTS}, + {"against", required_argument, 0, OPT_AGAINST}, {"force", no_argument, 0, 'f'}, {"batch", no_argument, 0, 'b'}, {"non-interactive", no_argument, 0, 'b'}, /* Alias for --batch */ @@ -526,6 +562,30 @@ boolean cli_parse_arguments(int argc, char* argv[], cli_options_t* options) { options->migrate_database = strdup(optarg); break; + case OPT_DIFF_ROOTS: + if (options->diff_roots_a != NULL) { + log_error(LOG_COMP_GENERAL, "Error: Multiple --diff-roots options not allowed"); + goto parse_error; + } + if (strlen(optarg) > CLI_MAX_PATH_LENGTH) { + log_error(LOG_COMP_GENERAL, "Error: --diff-roots path too long (max %d characters)", CLI_MAX_PATH_LENGTH); + goto parse_error; + } + options->diff_roots_a = strdup(optarg); + break; + + case OPT_AGAINST: + if (options->diff_roots_b != NULL) { + log_error(LOG_COMP_GENERAL, "Error: Multiple --against options not allowed"); + goto parse_error; + } + if (strlen(optarg) > CLI_MAX_PATH_LENGTH) { + log_error(LOG_COMP_GENERAL, "Error: --against path too long (max %d characters)", CLI_MAX_PATH_LENGTH); + goto parse_error; + } + options->diff_roots_b = strdup(optarg); + break; + case 'o': if (options->output_path != NULL) { log_error(LOG_COMP_GENERAL, "Error: Multiple --output options not allowed"); @@ -752,6 +812,16 @@ void cli_free_options(cli_options_t* options) { options->migrate_database = NULL; } + if (options->diff_roots_a != NULL) { + free(options->diff_roots_a); + options->diff_roots_a = NULL; + } + + if (options->diff_roots_b != NULL) { + free(options->diff_roots_b); + options->diff_roots_b = NULL; + } + if (options->output_path != NULL) { free(options->output_path); options->output_path = NULL; diff --git a/frontier-cli/cli_parser.h b/frontier-cli/cli_parser.h index 47c112abd..0bb5a26d0 100644 --- a/frontier-cli/cli_parser.h +++ b/frontier-cli/cli_parser.h @@ -51,6 +51,9 @@ typedef struct { char* system_root; // Path to system/root database (e.g., Frontier.root) char* migrate_database; // Path to database to migrate (--migrate) char* output_path; // Output path for migration (--output) + char* diff_roots_a; // First root to compare (--diff-roots) + char* diff_roots_b; // Second root to compare (--against); paired with + // diff_roots_a the same way --output pairs with --migrate char* log_spec; // Log spec string (--log comp:level,...) boolean verbose; // Verbose output boolean debug; // Debug output diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c new file mode 100644 index 000000000..4e1be11ed --- /dev/null +++ b/frontier-cli/diff_roots.c @@ -0,0 +1,1309 @@ +/* + SPDX-License-Identifier: MIT + + Copyright (c) 2026 Frontier contributors + + diff_roots.c -- recursive value-level equality walker for two ODB roots. + + See diff_roots.h for the design commitments. Implementation notes that are + not obvious from the header: + + - Traversal is hfirstsort/sortedlink, which is the ODB's own stored + alphabetical order (lang.h:436,474). Walking two roots in that order lets + the comparison be a single sorted merge with no sorting pass, no + allocation, and deterministic output. + + - Names are compared as RAW BYTES (memcmp over the Pascal string), never + case-folded and never newline-normalized. Storage is case-preserving, and + Virgin.root contains sibling names that differ only by control byte. + + - Externals ('scpt', 'optx', 'tabl', 'mbar', ...) are NOT distinct + valuetypes: they are all externalvaluetype, discriminated by the + tyexternalid on the external variable (lang.h:266-271). + + - Script and outline payloads are compared as oppackoutline() bytes. That + is the canonical serialized form, it encodes text AND node structure (so + the #881 single-LF-node class and the #890 comment-structure class both + surface), and a determinism spike proved it byte-stable across + independent loads of all three shipped roots. + + NOT langexternalpack(): its outline branch calls dbassignhandle_context + (opverbs.c:1494) and writes disk blocks. That is a save path. + + - Menubars and wptext are compared at the STORED level, never materialized. + For mbar the headless stub menuverbinmemory_context + (stubs/headless_menu_stubs.c:618) sets flinmemory = true WITHOUT loading, + so variabledata is still a raw dbaddress and dereferencing it segfaults -- + flinmemory is not trustworthy per-type in this build. For wptext, + force-materializing exhausts memory on a full-root walk (db_format.c:1694). +*/ + +#include +#include +#include + +#include "frontier.h" +#include "standard.h" +#include "shell_api.h" +#include "db.h" +#include "dbinternal.h" +#include "lang.h" +#include "langexternal.h" +#include "langinternal.h" +#include "op.h" +#include "oplist.h" +#include "opverbs.h" +#include "tablestructure.h" +#include "db_format.h" +#include "file.h" +#include "strings.h" +#include "memory.h" +/* + ut_pct_encode_segment is the lossless name codec shared with the ut-sync + layer. NOTE for whoever retires ut-sync (Step 8 of the root-build plan): + --diff-roots depends on this function. Lift it into a shared module rather + than deleting it with the rest of that layer. +*/ +#include "ut_sync.h" + +#include "diff_roots.h" + +#define maxwalkdepth 64 + +/* + Worst-case encoded segment: a 255-byte Pascal name whose every byte escapes + to three characters, plus the NUL. +*/ +#define maxencodedsegment (255 * 3 + 1) + + +typedef struct tyrootside { + + const char *path; + hdlfilenum fnum; + hdldatabaserecord database; + Handle hrootvariable; + hdlhashtable roottable; + db_context context; + boolean flopen; + + } tyrootside; + + +typedef struct tywalkstate { + + const tydiffoptions *options; + FILE *out; + long ctfindings; + + } tywalkstate; + + +const char *diff_roots_kindname (tydiffkind kind) { + + static const char *names [ctdiffkinds] = { + "only-in-a", + "only-in-b", + "type", + "payload", + "unsupported-type", + "unreadable" + }; + + if ((kind < 0) || (kind >= ctdiffkinds)) + return ("unknown"); + + return (names [kind]); + } /*diff_roots_kindname*/ + + +boolean diff_roots_appendsegment (char *path, size_t pathsz, const char *name, size_t namelen) { + + char encoded [maxencodedsegment]; + size_t used; + size_t need; + + if ((path == NULL) || (pathsz == 0)) + return (false); + + if (!ut_pct_encode_segment (name, namelen, encoded, sizeof (encoded))) + return (false); + + used = strlen (path); + + /*One byte for the '.' separator when this is not the first segment.*/ + need = used + ((used > 0) ? 1 : 0) + strlen (encoded) + 1; + + if (need > pathsz) + return (false); /*caller reports; never a silent skip*/ + + if (used > 0) { + path [used] = '.'; + memcpy (path + used + 1, encoded, strlen (encoded) + 1); + } + else + memcpy (path, encoded, strlen (encoded) + 1); + + return (true); + } /*diff_roots_appendsegment*/ + + +static void report (tywalkstate *state, tydiffkind kind, const char *path, + const char *typea, const char *typeb, + long sizea, long sizeb, long firstdiff, const char *detail) { + + tydifffinding finding; + + finding.kind = kind; + finding.path = path; + finding.typea = (typea != NULL) ? typea : "-"; + finding.typeb = (typeb != NULL) ? typeb : "-"; + finding.sizea = sizea; + finding.sizeb = sizeb; + finding.firstdiff = firstdiff; + finding.detail = detail; + + state->ctfindings++; + + if (state->options->callback != NULL) + (*state->options->callback) (&finding, state->options->refcon); + + if (state->options->flquiet) + return; + + fprintf (state->out, "%-16s %s", diff_roots_kindname (kind), path); + + if ((kind == diffkind_type) || (kind == diffkind_unsupported)) + fprintf (state->out, " A=%s B=%s", finding.typea, finding.typeb); + else if (kind == diffkind_payload) { + fprintf (state->out, " type=%s A=%ldbytes B=%ldbytes", finding.typea, sizea, sizeb); + + if (firstdiff >= 0) + fprintf (state->out, " first-diff-offset=%ld", firstdiff); + } + else if ((kind == diffkind_only_in_a) || (kind == diffkind_only_in_b)) + fprintf (state->out, " type=%s", + (kind == diffkind_only_in_a) ? finding.typea : finding.typeb); + + if (detail != NULL) + fprintf (state->out, " (%s)", detail); + + fprintf (state->out, "\n"); + } /*report*/ + + +/* + Symbolic type name for a value. For externals this is the external's kind, + which is what the census calls 'scpt'/'optx'/'tabl'/'mbar'; for everything + else it is the scalar type. Never returns nil. +*/ +static const char *typename_for (const tyvaluerecord *val) { + + if (val == NULL) + return ("-"); + + if ((*val).valuetype == externalvaluetype) { + + hdlexternalvariable hv = (hdlexternalvariable) (*val).data.externalvalue; + + if (hv == nil) + return ("exte-nil"); + + switch ((**hv).id) { + + case idoutlineprocessor: + return ((**hv).flscript ? "scpt" : "optx"); + + case idscriptprocessor: + return ("scpt"); + + case idtableprocessor: + return ("tabl"); + + case idmenuprocessor: + return ("mbar"); + + case idwordprocessor: + return ("wptx"); + + case idpictprocessor: + return ("pict"); + + default: + return ("exte"); + } + } + + switch ((*val).valuetype) { + + case novaluetype: return ("????"); + case charvaluetype: return ("char"); + case intvaluetype: return ("shor"); + case longvaluetype: return ("long"); + case oldstringvaluetype: /*unpacks into the same field as stringvaluetype*/ + case stringvaluetype: return ("TEXT"); + case binaryvaluetype: return ("data"); + case booleanvaluetype: return ("bool"); + case tokenvaluetype: return ("tokn"); + case datevaluetype: return ("date"); + case addressvaluetype: return ("addr"); + case doublevaluetype: return ("doub"); + case directionvaluetype: return ("dir "); + case ostypevaluetype: return ("type"); + case enumvaluetype: return ("enum"); + case pointvaluetype: return ("QDpt"); + case rectvaluetype: return ("qdrt"); + case rgbvaluetype: return ("cRGB"); + case patternvaluetype: return ("patt"); + case objspecvaluetype: return ("obj "); + case filespecvaluetype: return ("fspc"); + case aliasvaluetype: return ("alis"); + case listvaluetype: return ("list"); + case recordvaluetype: return ("reco"); + case codevaluetype: return ("code"); + case passwordvaluetype: return ("pass"); + case singlevaluetype: return ("sing"); + case fixedvaluetype: return ("fixd"); + + default: + return ("unkn"); + } + } /*typename_for*/ + + +/* + Raw payload bytes for the scalar types whose data lives in a Handle. All of + these alias the same Handle slot in tyvaluedata, which is why langgetvalsize + reads them all through data.binaryvalue. + + Returns false when this type is not handle-backed. +*/ +static boolean handlebytes (const tyvaluerecord *val, const unsigned char **pbytes, long *psize) { + + Handle h; + + /* + An fldiskval value has NOT been faulted in: data.binaryvalue still holds a + dbaddress, not a pointer, and dereferencing it segfaults. Callers resolve + those separately (resolvediskvalue) rather than resolving here, because the + kernel's resolver dirties the owning table as a side effect + (langhash.c:2211) and this walker must not dirty either root. + */ + if ((*val).fldiskval) + return (false); + + switch ((*val).valuetype) { + + case oldstringvaluetype: + case stringvaluetype: + case binaryvaluetype: + case passwordvaluetype: + case patternvaluetype: + case rgbvaluetype: + case rectvaluetype: + case objspecvaluetype: + case aliasvaluetype: + case doublevaluetype: + case addressvaluetype: + break; + + default: + return (false); + } + + h = (Handle) (*val).data.binaryvalue; + + if ((h == nil) || (*h == nil)) { + *pbytes = NULL; + *psize = 0; + return (true); /*empty is a legitimate, comparable state*/ + } + + *pbytes = (const unsigned char *) *h; + *psize = gethandlesize (h); + + return (true); + } /*handlebytes*/ + + +/* + Read the stored bytes of an fldiskval value WITHOUT mutating anything. + + The kernel's own resolver (hashresolvevalue_context, langhash.c:2196) does + the same dbrefhandle call, but then writes the handle back into the node and + sets (**htable).fldirty = true "because we released the disk value". A + read-only walker must not do either, so this stops at the read. The caller + disposes the handle. +*/ +static boolean resolvediskvalue (tyrootside *side, const tyvaluerecord *val, Handle *h) { + + hdldatabaserecord saved = databasedata; + boolean fl; + + *h = nil; + + if (!(*val).fldiskval) + return (false); + + /* + Park the global on this side for the duration of the read. dbrefhandle_context + takes an explicit context, but the layers beneath it still consult + databasedata, and with two roots open the global may be pointing at the + other one. Callee-saves, matching the convention documented at + langexternal.c:975-979. + */ + databasedata = side->database; + + fl = dbrefhandle_context (&side->context, (*val).data.diskvalue, h); + + databasedata = saved; + + return (fl); + } /*resolvediskvalue*/ + + +/* + Inline scalars: compared by value, not by dereference. Returns false when + this type is not an inline scalar. +*/ +static boolean inlineequal (const tyvaluerecord *a, const tyvaluerecord *b, boolean *pequal) { + + switch ((*a).valuetype) { + + case novaluetype: + /*Nothing is stored for these (langpack.c: "nothing to pack").*/ + *pequal = true; + return (true); + + case booleanvaluetype: + *pequal = ((*a).data.flvalue == (*b).data.flvalue); + return (true); + + case charvaluetype: + *pequal = ((*a).data.chvalue == (*b).data.chvalue); + return (true); + + case intvaluetype: + *pequal = ((*a).data.intvalue == (*b).data.intvalue); + return (true); + + case longvaluetype: + case datevaluetype: + case fixedvaluetype: + *pequal = ((*a).data.longvalue == (*b).data.longvalue); + return (true); + + case ostypevaluetype: + *pequal = ((*a).data.ostypevalue == (*b).data.ostypevalue); + return (true); + + case enumvaluetype: + *pequal = ((*a).data.enumvalue == (*b).data.enumvalue); + return (true); + + case directionvaluetype: + *pequal = ((*a).data.dirvalue == (*b).data.dirvalue); + return (true); + + case tokenvaluetype: + *pequal = ((*a).data.tokenvalue == (*b).data.tokenvalue); + return (true); + + case singlevaluetype: + *pequal = ((*a).data.singlevalue == (*b).data.singlevalue); + return (true); + + case pointvaluetype: + *pequal = (memcmp (&(*a).data.pointvalue, &(*b).data.pointvalue, + sizeof ((*a).data.pointvalue)) == 0); + return (true); + + default: + return (false); + } + } /*inlineequal*/ + + +/* + Packed bytes for an outline-backed external (script or outline). The caller + disposes the handle. Menubars do NOT come here -- see the file header. +*/ +static boolean packoutlinebytes (hdlexternalvariable hv, Handle *hpacked) { + + hdloutlinerecord ho; + + *hpacked = nil; + + if (!(**hv).flinmemory) + return (false); + + ho = (hdloutlinerecord) (**hv).variabledata; + + if (ho == nil) + return (false); + + /*oppackoutline APPENDS to a non-nil handle, so it must start nil.*/ + return (oppackoutline (ho, hpacked)); + } /*packoutlinebytes*/ + + +/* + Volatile regions of the packed-outline header (oppack_v7.c:115-152), given + as [offset, offset+length). These carry SAVE METADATA, not content, and they + must be excluded from an equality basis or every value in a re-saved root + reports as different. + + Verified empirically: comparing a root against a copy that had been opened + and saved once produced 3,527 spurious payload findings, 3,495 of them + differing first at offset 37 (inside timelastsave) and 27 at offset 49 + (inside outlinesignature). + + timecreated offset 24, 8 bytes -- creation stamp, per-value + timelastsave offset 32, 8 bytes -- rewritten on every save + ctsaves offset 40, 4 bytes -- INCREMENTED by oppack itself + (oppack_v7.c:622), so it differs + even between two packs + outlinesignature offset 48, 4 bytes -- caller-defined cookie + + timecreated is excluded too: it is per-VALUE provenance metadata that the + root-build plan preserves in the manifest (decision 9.4), so it is exactly + the kind of thing that legitimately differs between a built root and its + ancestor without the CONTENT differing. + + NOTE: this makes --diff-roots a CONTENT comparison. A dedicated metadata + comparison is a separate tool if one is ever wanted. +*/ +typedef struct tyskipregion { + long offset; + long length; + } tyskipregion; + +static const tyskipregion outlineheaderskips [] = { + {24, 8}, /*timecreated*/ + {32, 8}, /*timelastsave*/ + {40, 4}, /*ctsaves*/ + {48, 4} /*outlinesignature*/ + }; + +#define ctoutlineheaderskips ((long) (sizeof (outlineheaderskips) / sizeof (outlineheaderskips [0]))) + +/*Total packed-outline header size (_Static_assert at oppack_v7.c:154).*/ +#define outlineheaderbytes 1068 + + +static boolean inskipregion (long offset) { + + long i; + + for (i = 0; i < ctoutlineheaderskips; i++) { + + const tyskipregion *r = &outlineheaderskips [i]; + + if ((offset >= r->offset) && (offset < (r->offset + r->length))) + return (true); + } + + return (false); + } /*inskipregion*/ + + +/* + Compare two packed buffers, ignoring the volatile fields of a packed-outline + header that begins at `outlinebase`. + + `outlinebase` is 0 for a packed outline, and the size of the list header for + a packed list -- oppacklist emits `tydisklistrecord` followed by the packed + outline (oplist.c:708-711), so a list inherits the same volatile fields at a + fixed displacement. + + Returns the offset of the first CONTENT difference, or -1 when equal. +*/ +static long outlinecontentdifference (const unsigned char *a, long cta, + const unsigned char *b, long ctb, + long outlinebase) { + + long i; + + if (cta != ctb) + return ((cta < ctb) ? cta : ctb); /*length difference is a content difference*/ + + for (i = 0; i < cta; i++) { + + long relative; + + if (a [i] == b [i]) + continue; + + relative = i - outlinebase; + + if ((relative >= 0) && (relative < outlineheaderbytes) && inskipregion (relative)) + continue; + + return (i); + } + + return (-1); + } /*outlinecontentdifference*/ + + +/* + Byte offset of the packed outline inside a packed list. + + Read from the list header itself rather than hardcoded: tydisklistrecord's + first field is `recordsize` -- "number of bytes in this header" (oplist.c:88) + -- written as a big-endian short by oppacklist (oplist.c:694). Deriving it + means the walker cannot drift from the format if that struct changes. + + Returns -1 if the buffer is too small or the size is implausible, in which + case the caller compares the whole buffer verbatim. +*/ +static long listoutlinebase (const unsigned char *packed, long ct) { + + long recordsize; + + if ((packed == NULL) || (ct < 2)) + return (-1); + + recordsize = ((long) packed [0] << 8) | (long) packed [1]; + + if ((recordsize < 4) || (recordsize > ct)) + return (-1); + + return (recordsize); + } /*listoutlinebase*/ + + +static long firstdifference (const unsigned char *a, const unsigned char *b, long ct) { + + long i; + + for (i = 0; i < ct; i++) + if (a [i] != b [i]) + return (i); + + return (-1); + } /*firstdifference*/ + + +static void comparepayload (tywalkstate *state, const char *path, + tyrootside *sidea, tyrootside *sideb, + const tyvaluerecord *vala, const tyvaluerecord *valb); + + +static void walkpair (tywalkstate *state, const char *path, + tyrootside *sidea, tyrootside *sideb, + hdlhashtable tablea, hdlhashtable tableb, int depth); + + +/* + Materialize an external read-only, if it is the kind we are willing to + materialize. Returns false when the value cannot be brought into memory -- + the caller reports that rather than skipping. +*/ +static boolean materialize (tyrootside *side, hdlexternalvariable hv) { + + if ((**hv).flinmemory) + return (true); + + return (ensure_external_in_memory (&side->context, hv)); + } /*materialize*/ + + +static void compareexternal (tywalkstate *state, const char *path, + tyrootside *sidea, tyrootside *sideb, + const tyvaluerecord *vala, const tyvaluerecord *valb, + int depth) { + + hdlexternalvariable hva = (hdlexternalvariable) (*vala).data.externalvalue; + hdlexternalvariable hvb = (hdlexternalvariable) (*valb).data.externalvalue; + tyexternalid extid; + + if ((hva == nil) || (hvb == nil)) { + report (state, diffkind_unreadable, path, typename_for (vala), typename_for (valb), + -1, -1, -1, "nil external variable"); + return; + } + + if ((**hva).id != (**hvb).id) { + report (state, diffkind_type, path, typename_for (vala), typename_for (valb), + -1, -1, -1, NULL); + return; + } + + extid = (**hva).id; + + switch (extid) { + + case idtableprocessor: { + + if (!materialize (sidea, hva) || !materialize (sideb, hvb)) { + report (state, diffkind_unreadable, path, "tabl", "tabl", -1, -1, -1, + "subtable could not be loaded"); + return; + } + + walkpair (state, path, sidea, sideb, + (hdlhashtable) (**hva).variabledata, + (hdlhashtable) (**hvb).variabledata, + depth + 1); + return; + } + + case idoutlineprocessor: + case idscriptprocessor: { + + Handle hpackeda = nil; + Handle hpackedb = nil; + long sizea, sizeb; + + if (!materialize (sidea, hva) || !materialize (sideb, hvb)) { + report (state, diffkind_unreadable, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "outline could not be loaded"); + return; + } + + if (!packoutlinebytes (hva, &hpackeda) || !packoutlinebytes (hvb, &hpackedb)) { + + if (hpackeda != nil) + disposehandle (hpackeda); + + if (hpackedb != nil) + disposehandle (hpackedb); + + report (state, diffkind_unreadable, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "outline could not be packed"); + return; + } + + sizea = gethandlesize (hpackeda); + sizeb = gethandlesize (hpackedb); + + { + long offset = outlinecontentdifference ((const unsigned char *) *hpackeda, sizea, + (const unsigned char *) *hpackedb, sizeb, + 0); + + if (offset >= 0) + report (state, diffkind_payload, path, + typename_for (vala), typename_for (valb), + sizea, sizeb, offset, NULL); + } + + disposehandle (hpackeda); + disposehandle (hpackedb); + return; + } + + case idmenuprocessor: + case idwordprocessor: { + + /* + Deliberately NOT materialized. mbar: the headless stub fakes + flinmemory without loading, so any dereference segfaults + (stubs/headless_menu_stubs.c:618). wptx: force-materializing + exhausts memory on a full-root walk (db_format.c:1694). + + Compare where they actually live -- their stored addresses. Equal + addresses mean the identical stored block; unequal addresses are + reported honestly as "cannot compare content here" rather than + being claimed as a content difference. + */ + dbaddress adra = (**hva).oldaddress; + dbaddress adrb = (**hvb).oldaddress; + + if (adra != adrb) + report (state, diffkind_unsupported, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "not comparable headless; stored addresses differ"); + + return; + } + + default: + report (state, diffkind_unsupported, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "unhandled external kind"); + return; + } + } /*compareexternal*/ + + +static void comparepayload (tywalkstate *state, const char *path, + tyrootside *sidea, tyrootside *sideb, + const tyvaluerecord *vala, const tyvaluerecord *valb) { + + boolean equal = false; + const unsigned char *bytesa = NULL; + const unsigned char *bytesb = NULL; + long sizea = 0; + long sizeb = 0; + + /* + Unresolved disk values first: their payload is still on disk, so they must + be read through the owning side's context rather than dereferenced. + */ + if ((*vala).fldiskval || (*valb).fldiskval) { + + Handle ha = nil; + Handle hb = nil; + boolean oka = true; + boolean okb = true; + + if ((*vala).fldiskval) + oka = resolvediskvalue (sidea, vala, &ha); + + if ((*valb).fldiskval) + okb = resolvediskvalue (sideb, valb, &hb); + + if (!oka || !okb) { + + if (ha != nil) + disposehandle (ha); + + if (hb != nil) + disposehandle (hb); + + report (state, diffkind_unreadable, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "stored value could not be read"); + return; + } + + /* + A value may be on disk on one side and resident on the other -- that is + a residency difference, not a content difference, so compare bytes + either way. + */ + if (ha != nil) { + bytesa = (const unsigned char *) *ha; + sizea = gethandlesize (ha); + } + else if (!handlebytes (vala, &bytesa, &sizea)) { + disposehandle (hb); + report (state, diffkind_unsupported, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "mixed residency for a non-handle type"); + return; + } + + if (hb != nil) { + bytesb = (const unsigned char *) *hb; + sizeb = gethandlesize (hb); + } + else if (!handlebytes (valb, &bytesb, &sizeb)) { + disposehandle (ha); + report (state, diffkind_unsupported, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "mixed residency for a non-handle type"); + return; + } + + if ((sizea != sizeb) + || ((sizea > 0) && (memcmp (bytesa, bytesb, (size_t) sizea) != 0))) + report (state, diffkind_payload, path, + typename_for (vala), typename_for (valb), sizea, sizeb, + (sizea == sizeb) ? firstdifference (bytesa, bytesb, sizea) : -1, + NULL); + + if (ha != nil) + disposehandle (ha); + + if (hb != nil) + disposehandle (hb); + + return; + } + + if (inlineequal (vala, valb, &equal)) { + + if (!equal) + report (state, diffkind_payload, path, + typename_for (vala), typename_for (valb), -1, -1, -1, NULL); + + return; + } + + if (handlebytes (vala, &bytesa, &sizea) && handlebytes (valb, &bytesb, &sizeb)) { + + if (sizea != sizeb) { + report (state, diffkind_payload, path, + typename_for (vala), typename_for (valb), sizea, sizeb, -1, NULL); + return; + } + + if ((sizea > 0) && (memcmp (bytesa, bytesb, (size_t) sizea) != 0)) + report (state, diffkind_payload, path, + typename_for (vala), typename_for (valb), sizea, sizeb, + firstdifference (bytesa, bytesb, sizea), NULL); + + return; + } + + if (((*vala).valuetype == listvaluetype) || ((*vala).valuetype == recordvaluetype)) { + + /* + Lists and records serialize through oppacklist -- the same call the + ODB's own scalar packer uses (langhash.c:3204-3207). This is what keeps + #887 out of the picture: nothing here coerces to string. + */ + Handle hpackeda = nil; + Handle hpackedb = nil; + + if (!oppacklist ((hdllistrecord) (*vala).data.listvalue, &hpackeda) + || !oppacklist ((hdllistrecord) (*valb).data.listvalue, &hpackedb)) { + + if (hpackeda != nil) + disposehandle (hpackeda); + + if (hpackedb != nil) + disposehandle (hpackedb); + + report (state, diffkind_unreadable, path, + typename_for (vala), typename_for (valb), -1, -1, -1, + "list could not be packed"); + return; + } + + sizea = gethandlesize (hpackeda); + sizeb = gethandlesize (hpackedb); + + { + /* + A packed list is a list header followed by a packed outline, so it + carries the same volatile save-metadata fields at a displacement. + */ + long basea = listoutlinebase ((const unsigned char *) *hpackeda, sizea); + long baseb = listoutlinebase ((const unsigned char *) *hpackedb, sizeb); + long offset; + + if ((basea >= 0) && (basea == baseb)) + offset = outlinecontentdifference ((const unsigned char *) *hpackeda, sizea, + (const unsigned char *) *hpackedb, sizeb, + basea); + else if (sizea != sizeb) + offset = (sizea < sizeb) ? sizea : sizeb; + else + offset = firstdifference ((const unsigned char *) *hpackeda, + (const unsigned char *) *hpackedb, sizea); + + if (offset >= 0) + report (state, diffkind_payload, path, + typename_for (vala), typename_for (valb), sizea, sizeb, offset, NULL); + } + + disposehandle (hpackeda); + disposehandle (hpackedb); + return; + } + + /*Loud fail: a type we do not know how to compare is a finding, not a skip.*/ + report (state, diffkind_unsupported, path, + typename_for (vala), typename_for (valb), -1, -1, -1, NULL); + } /*comparepayload*/ + + +static void comparevalue (tywalkstate *state, const char *path, + tyrootside *sidea, tyrootside *sideb, + const tyvaluerecord *vala, const tyvaluerecord *valb, + int depth) { + + /* + oldstringvaluetype (4) and stringvaluetype (12) unpack into the same field + (langpack.c:654-660), so treat them as the same type rather than emitting a + phantom TYPE finding on a cross-era comparison. + */ + tyvaluetype ta = (*vala).valuetype; + tyvaluetype tb = (*valb).valuetype; + + if (ta == oldstringvaluetype) + ta = stringvaluetype; + + if (tb == oldstringvaluetype) + tb = stringvaluetype; + + if (ta != tb) { + report (state, diffkind_type, path, typename_for (vala), typename_for (valb), + -1, -1, -1, NULL); + return; + } + + if (ta == externalvaluetype) { + compareexternal (state, path, sidea, sideb, vala, valb, depth); + return; + } + + comparepayload (state, path, sidea, sideb, vala, valb); + } /*comparevalue*/ + + +/* + Compare two raw Pascal names as bytes. Returns <0, 0, >0 like memcmp. +*/ +static int comparenames (const unsigned char *bsa, const unsigned char *bsb) { + + size_t lena = (size_t) bsa [0]; + size_t lenb = (size_t) bsb [0]; + size_t shared = (lena < lenb) ? lena : lenb; + int order = memcmp (bsa + 1, bsb + 1, shared); + + if (order != 0) + return (order); + + if (lena == lenb) + return (0); + + return ((lena < lenb) ? -1 : 1); + } /*comparenames*/ + + +static void reportsubtree (tywalkstate *state, const char *parentpath, + hdlhashnode node, tydiffkind kind, int depth); + + +/* + Report an entire present-on-one-side-only subtree, so a missing table is + not reported as a single line that hides thousands of values. +*/ +static void reportonlyside (tywalkstate *state, const char *parentpath, + const unsigned char *bsname, const tyvaluerecord *val, + tydiffkind kind, hdlhashnode node, int depth) { + + /* + Path buffers are HEAP allocated, not stack. maxdiffpath is deliberately + generous (a diff tool must never fail to name a path), and both this + function and walkpair recurse to maxwalkdepth -- an 8 KB stack buffer per + frame overflows the 8 MB default stack well before that depth. Learned the + hard way: the first build segfaulted here on manila.root. + */ + char *path = (char *) malloc (maxdiffpath); + + if (path == NULL) { + report (state, diffkind_unreadable, parentpath, "-", "-", -1, -1, -1, + "out of memory building path"); + return; + } + + snprintf (path, maxdiffpath, "%s", parentpath); + + if (!diff_roots_appendsegment (path, maxdiffpath, + (const char *) &bsname [1], (size_t) bsname [0])) { + report (state, diffkind_unreadable, parentpath, "-", "-", -1, -1, -1, + "path too long to report"); + free (path); + return; + } + + report (state, kind, path, + (kind == diffkind_only_in_a) ? typename_for (val) : "-", + (kind == diffkind_only_in_b) ? typename_for (val) : "-", + -1, -1, -1, NULL); + + reportsubtree (state, path, node, kind, depth); + + free (path); + } /*reportonlyside*/ + + +static void reportsubtree (tywalkstate *state, const char *parentpath, + hdlhashnode node, tydiffkind kind, int depth) { + + tyvaluerecord *val; + hdlexternalvariable hv; + hdlhashtable child; + hdlhashnode nomad; + + if (depth >= maxwalkdepth) + return; + + val = &(**node).val; + + if ((*val).valuetype != externalvaluetype) + return; + + hv = (hdlexternalvariable) (*val).data.externalvalue; + + if ((hv == nil) || ((**hv).id != idtableprocessor)) + return; + + if (!(**hv).flinmemory) + return; /*not loaded on this side; the table itself was already reported*/ + + child = (hdlhashtable) (**hv).variabledata; + + if (child == nil) + return; + + for (nomad = (**child).hfirstsort; nomad != nil; nomad = (**nomad).sortedlink) { + + bigstring bsname; + + gethashkey (nomad, bsname); + + reportonlyside (state, parentpath, bsname, &(**nomad).val, kind, nomad, depth + 1); + } + } /*reportsubtree*/ + + +static void walkpair (tywalkstate *state, const char *path, + tyrootside *sidea, tyrootside *sideb, + hdlhashtable tablea, hdlhashtable tableb, int depth) { + + hdlhashnode nodea; + hdlhashnode nodeb; + + if (depth >= maxwalkdepth) { + report (state, diffkind_unreadable, path, "tabl", "tabl", -1, -1, -1, + "maximum walk depth exceeded"); + return; + } + + if ((tablea == nil) || (tableb == nil)) { + report (state, diffkind_unreadable, path, "tabl", "tabl", -1, -1, -1, + "nil subtable"); + return; + } + + nodea = (**tablea).hfirstsort; + nodeb = (**tableb).hfirstsort; + + /* + Sorted merge. Both sides are in the ODB's own stored order, so a single + lockstep pass yields every finding in deterministic order with no sorting + and no allocation. + */ + while ((nodea != nil) || (nodeb != nil)) { + + bigstring bsa; + bigstring bsb; + int order; + + if (nodea == nil) + order = 1; + else if (nodeb == nil) + order = -1; + else { + gethashkey (nodea, bsa); + gethashkey (nodeb, bsb); + order = comparenames (bsa, bsb); + } + + if (order < 0) { + gethashkey (nodea, bsa); + reportonlyside (state, path, bsa, &(**nodea).val, diffkind_only_in_a, + nodea, depth); + nodea = (**nodea).sortedlink; + continue; + } + + if (order > 0) { + gethashkey (nodeb, bsb); + reportonlyside (state, path, bsb, &(**nodeb).val, diffkind_only_in_b, + nodeb, depth); + nodeb = (**nodeb).sortedlink; + continue; + } + + { + /*Heap, not stack -- see the note in reportonlyside.*/ + char *childpath = (char *) malloc (maxdiffpath); + + if (childpath == NULL) { + report (state, diffkind_unreadable, path, "-", "-", -1, -1, -1, + "out of memory building path"); + return; + } + + gethashkey (nodea, bsa); + + snprintf (childpath, maxdiffpath, "%s", path); + + if (!diff_roots_appendsegment (childpath, maxdiffpath, + (const char *) &bsa [1], (size_t) bsa [0])) + report (state, diffkind_unreadable, path, "-", "-", -1, -1, -1, + "path too long to report"); + else + comparevalue (state, childpath, sidea, sideb, + &(**nodea).val, &(**nodeb).val, depth); + + free (childpath); + } + + nodea = (**nodea).sortedlink; + nodeb = (**nodeb).sortedlink; + } + } /*walkpair*/ + + +static void closeside (tyrootside *side) { + + if (!side->flopen) + return; + + /* + Point the database globals back at this side before closing, so dbclose + tears down the right database when two are open. + */ + databasedata = side->database; + + dbclose (); + + side->flopen = false; + side->database = nil; + side->roottable = nil; + } /*closeside*/ + + +static boolean openside (tyrootside *side, const char *path, FILE *err) { + + bigstring bspath; + tyfilespec fs; + dbaddress rootaddress = nildbaddress; + + side->path = path; + side->fnum = 0; + side->database = nil; + side->hrootvariable = nil; + side->roottable = nil; + side->flopen = false; + + if (!copyctopstring (path, bspath)) { + fprintf (err, "diff-roots: path too long: %s\n", path); + return (false); + } + + if (!pathtofilespec (bspath, &fs)) { + fprintf (err, "diff-roots: cannot resolve path: %s\n", path); + return (false); + } + + /* + Read-only at both layers. dbwrite refuses writes to a flreadonly database + (db.c:715-728), and read-only open also skips the auto-migration that a + read-write open would perform on a v6 file (dbverbs.c:741). + */ + if (!openfile (&fs, &side->fnum, true)) { + fprintf (err, "diff-roots: cannot open: %s\n", path); + return (false); + } + + if (!dbopenfile (side->fnum, true)) { + fprintf (err, "diff-roots: not a readable database: %s\n", path); + closefile (side->fnum); + return (false); + } + + side->flopen = true; + side->database = databasedata; + + db_context_init (&side->context); + side->context.database = side->database; + side->context.mode = db_format_mode_current (); + + if ((**side->database).versionnumber < 7) { + /* + v6 stores a Cancoon record in views[0] whose adrroottable points at the + root table; v7 stores the root table address directly + (odbengine.c:566-570). Rather than silently misinterpret one as the + other, refuse. + */ + fprintf (err, "diff-roots: %s is not a v7 database (version %d); " + "migrate it first\n", path, (int) (**side->database).versionnumber); + closeside (side); + return (false); + } + + dbgetview (cancoonview, &rootaddress); + + if (rootaddress == nildbaddress) { + fprintf (err, "diff-roots: %s has no root table\n", path); + closeside (side); + return (false); + } + + if (!tableloadsystemtable (rootaddress, &side->hrootvariable, &side->roottable, false)) { + fprintf (err, "diff-roots: cannot load root table of %s\n", path); + closeside (side); + return (false); + } + + return (true); + } /*openside*/ + + +boolean diff_roots_compare (const char *patha, const char *pathb, + const tydiffoptions *options, long *ctfindings) { + + tydiffoptions defaults; + tyrootside sidea; + tyrootside sideb; + tywalkstate state; + odb_context_guard guard; + FILE *err = stderr; + boolean ok = false; + char rootpath [maxdiffpath]; + + if (ctfindings != NULL) + *ctfindings = 0; + + if ((patha == NULL) || (pathb == NULL)) + return (false); + + if (options == NULL) { + memset (&defaults, 0, sizeof (defaults)); + options = &defaults; + } + + memset (&state, 0, sizeof (state)); + state.options = options; + state.out = (options->out != NULL) ? options->out : stdout; + state.ctfindings = 0; + + if (!db_format_prepare_runtime ()) { + fprintf (err, "diff-roots: database runtime failed to initialize\n"); + return (false); + } + + /* + Opening a second database clears the table-structure globals, which would + destroy the first root's state. The guard saves and restores all of them -- + this is the documented hazard at db.h:285-289. + */ + odb_guard_enter (&guard); + + memset (&sidea, 0, sizeof (sidea)); + memset (&sideb, 0, sizeof (sideb)); + + if (!openside (&sidea, patha, err)) + goto exit; + + if (!openside (&sideb, pathb, err)) + goto exit; + + rootpath [0] = '\0'; + + /* + Value access reads the databasedata global, so each side's walk must run + with that global pointing at that side. walkpair touches both sides, so it + sets the global per access via the side's db_context where the API allows + it, and leaves databasedata parked on side A otherwise. + */ + databasedata = sidea.database; + + walkpair (&state, rootpath, &sidea, &sideb, sidea.roottable, sideb.roottable, 0); + + ok = true; + +exit: + + closeside (&sideb); + closeside (&sidea); + + odb_guard_exit (&guard); + + if (ctfindings != NULL) + *ctfindings = state.ctfindings; + + return (ok); + } /*diff_roots_compare*/ diff --git a/frontier-cli/diff_roots.h b/frontier-cli/diff_roots.h new file mode 100644 index 000000000..3328e98b9 --- /dev/null +++ b/frontier-cli/diff_roots.h @@ -0,0 +1,151 @@ +/* + SPDX-License-Identifier: MIT + + Copyright (c) 2026 Frontier contributors + + diff_roots.h -- recursive value-level equality walker for two ODB roots. + + This is the acceptance instrument for the root-build-from-source plan + (planning/ROOT_BUILD_FROM_SOURCE_PLAN.md, Step 1). Every later step is + verified with it: the exporter/assembler round-trip law + (diff(build(export(R)), R) == empty) and the CI drift check both reduce to + "run this and require an empty diff". + + Design commitments, each one load-bearing: + + - PURE C VALUE ACCESS. No interpreter, no string() coercion (string() on a + `list` value HARD-ABORTS through try, #887), no hashgetvaluestring + (deparses and would false-diff). Payloads are compared as stored bytes. + + - BOTH ROOTS OPEN READ-ONLY. Neither file is written, and neither is + dirtied on purpose. dbwrite() itself refuses writes to a flreadonly + database (db.c:715-728), so this is enforced below us as well. + + - NAMES ARE BYTES. ODB names legitimately contain tabs and raw CR/LF -- + Virgin.root has sibling values named with the single bytes 0x0A/0x0B/ + 0x0C/0x0D under system.verbs.builtins.tcp.im.builtinDrivers.aim.code. + util.repl. Those two collide the moment names are newline-normalized, so + names are compared as raw bytes and only ESCAPED for reporting. + + - LOUD FAIL, NEVER SILENT SKIP. Any value the walker cannot compare + produces a per-path finding and a non-empty diff. "Lost artifacts" + become red builds by construction. +*/ + +#ifndef diffrootsinclude +#define diffrootsinclude + +#include + +#include "frontier.h" + +/* + Maximum reported path length. + + Generous on purpose: a diff tool must never fail to NAME a path. The + existing ODB walkers cap paths at 512 bytes and skip on overflow + (frontier-cli/main.c:2429, Common/source/langhash.c:297); for a report that + feeds an acceptance gate, a skipped path is a correctness hole, so overflow + here is both rare and reported rather than silent. +*/ +#define maxdiffpath 8192 + + +/* + Finding kinds, in the order the reporter prints their names. +*/ +typedef enum tydiffkind { + + diffkind_only_in_a, /*path present in A, absent in B*/ + diffkind_only_in_b, /*path present in B, absent in A*/ + diffkind_type, /*same path, different type*/ + diffkind_payload, /*same path and type, different bytes*/ + diffkind_unsupported, /*walker cannot compare this type -- loud fail*/ + diffkind_unreadable, /*value could not be read/materialized on one side*/ + + ctdiffkinds + } tydiffkind; + + +/* + One finding. `path` is the full ODB path with every segment percent-encoded + (see ut_pct_encode_segment), so a '.' in the path is ALWAYS a separator and + control bytes are unambiguous. + + sizea/sizeb are payload byte counts, or -1 where not applicable. firstdiff + is the offset of the first differing byte for diffkind_payload, else -1. + The finding never carries payload CONTENT: dumping bytes would both bloat + the report and risk re-encoding MacRoman (#880). +*/ +typedef struct tydifffinding { + + tydiffkind kind; + const char *path; + const char *typea; /*4-char type tag, or a symbolic name; never nil*/ + const char *typeb; + long sizea; + long sizeb; + long firstdiff; + const char *detail; /*optional extra context; may be nil*/ + + } tydifffinding; + + +typedef void (*tydiffcallback) (const tydifffinding *finding, void *refcon); + + +/* + Options controlling a walk. Zero-initialize for defaults. +*/ +typedef struct tydiffoptions { + + FILE *out; /*findings stream; nil means stdout*/ + boolean flquiet; /*count findings without printing them*/ + tydiffcallback callback; /*optional per-finding hook (tests use this)*/ + void *refcon; + + } tydiffoptions; + + +/* + diff_roots_compare -- open both roots read-only, walk them, report every + difference. + + Returns true if the walk COMPLETED (whether or not differences were found); + returns false only on an operational failure (cannot open, cannot reach a + root table, unsupported format). *ctfindings receives the number of + findings; a completed walk with zero findings is the empty-diff case. + + Both paths must name v7 databases. A v6 root is rejected rather than + misinterpreted: in v6 the database's views[0] points at a Cancoon record, + while in v7 it IS the root table address (odbengine.c:566-570). +*/ +extern boolean diff_roots_compare (const char *patha, const char *pathb, + const tydiffoptions *options, + long *ctfindings); + + +/* + diff_roots_kindname -- stable lowercase-hyphenated name for a finding kind, + as it appears in report output ("only-in-a", "payload", ...). Returns + "unknown" for an out-of-range kind rather than indexing off the end. +*/ +extern const char *diff_roots_kindname (tydiffkind kind); + + +/* + diff_roots_appendsegment -- append one raw ODB name to a dotted path, + percent-encoding it first. + + `path` is a NUL-terminated buffer of `pathsz` bytes holding the parent path + ("" for the root). `name`/`namelen` are the RAW name bytes (which may + include tab, CR, LF, NUL, or high-bit MacRoman). Returns true on success. + + Returns false, leaving `path` unmodified, if the result would not fit -- + callers must treat that as a reportable condition, never as a reason to + skip a value silently. +*/ +extern boolean diff_roots_appendsegment (char *path, size_t pathsz, + const char *name, size_t namelen); + +#endif diff --git a/frontier-cli/main.c b/frontier-cli/main.c index b6bd4840e..e4c474dd5 100644 --- a/frontier-cli/main.c +++ b/frontier-cli/main.c @@ -92,6 +92,7 @@ // CLI-specific headers #include "ut_sync.h" #include "ut_scan.h" +#include "diff_roots.h" #include "cli_parser.h" #include "cli_executor.h" #include "cli_utils.h" @@ -867,6 +868,48 @@ int main(int argc, char* argv[]) { return 0; } + /* + * Handle --diff-roots mode: compare two roots read-only and exit. + * + * Placed in the same early-exit band as --migrate, and for the same + * reason: it returns directly from main(), so it never reaches + * initialize_frontier_runtime() and therefore never reaches the + * save-system-root-on-exit path. Nothing here boots the interpreter or + * runs startup scripts; diff_roots_compare() opens both databases + * read-only and walks them in pure C. + * + * Exit codes: 0 = empty diff, 1 = differences found, 2 = operational + * failure (cannot open, not v7, walk aborted). Callers -- CI drift checks + * and the round-trip acceptance law -- depend on 1 and 2 being distinct; + * a broken check must never look like a clean comparison. + */ + if (g_cli_options.diff_roots_a != NULL) { + long ctfindings = 0; + tydiffoptions diffoptions; + boolean walked; + + memset(&diffoptions, 0, sizeof(diffoptions)); + diffoptions.out = stdout; + + walked = diff_roots_compare(g_cli_options.diff_roots_a, + g_cli_options.diff_roots_b, + &diffoptions, &ctfindings); + + if (!walked) { + cli_free_options(&g_cli_options); + return 2; + } + + if (ctfindings == 0) + fprintf(stderr, "no differences\n"); + else + fprintf(stderr, "%ld difference%s\n", ctfindings, (ctfindings == 1) ? "" : "s"); + + cli_free_options(&g_cli_options); + + return (ctfindings == 0) ? 0 : 1; + } + /* Handle --migrate mode: migrate database to v7 and exit */ if (g_cli_options.migrate_database != NULL) { boolean migrated = false; @@ -1332,6 +1375,10 @@ static void print_usage(const char* program_name) { printf(" --non-interactive Alias for --batch\n"); printf(" --migrate PATH Migrate v6 database to v7 format and exit\n"); printf(" --output PATH Output path for migrated database (default: in-place, v6 backed up)\n"); + printf(" --diff-roots PATH Compare two v7 roots value-by-value and exit. Requires\n"); + printf(" --against. Both are opened READ-ONLY and never written.\n"); + printf(" Exit 0 = identical, 1 = differences found, 2 = error.\n"); + printf(" --against PATH The second root to compare (used with --diff-roots)\n"); printf(" -f, --force Overwrite existing output file (only applies with --output)\n"); printf(" --skip-startup Skip system.startup scripts (they run by default)\n"); printf(" --output-json Output results in JSON format\n"); diff --git a/tests/Makefile b/tests/Makefile index a609e67fd..007aead9a 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -223,7 +223,7 @@ USERTALK_OBJECT_TESTS = \ test_usertalk_runner # Buildable test executables on this branch -RUN_BUILDABLE = core_tests db_format_tests runtime_tests parser_tests save_migration_tests test_efp_augmentation file_portable_tests file_readline_tests file_verb_tests handle_tests cli_runtime_tests paige_text_tests oppack_tests hashpack_modern_tests hash_corruption_tests langvalue_64_tests table_operations_integration refcon_tests refcon_phase2_tests op_context_tests table_context_tests time_portability_test test_sys_shell_command_verbs headless_selection_tests opattributes_stub_tests headless_thread_registry_tests test_callback_infrastructure tcp_phase1a_unit_tests menu_v7_refcon_tests menudata_headless_tests menu_list_describe_tests meuserselected_headless_tests test_stringerrorlist pane_compositor_tests mouse_parser_tests terminal_control_tests terminal_control_dsr_tests palette_state_tests palette_render_snapshot_tests palette_render_backend_tests palette_arg_inject_tests palette_arg_inject_concurrency_tests repl_palette_source_tests repl_verbs_tests repl_slash_resolver_tests scrollback_pane_tests lang_causedby_snapshot_tests test_getsystemtablescript test_window_bridge copyctopstring_tests ut_sync_canonicalize_tests ut_sync_pathmap_tests ut_sync_pctcodec_tests ut_sync_export_tests ut_sync_decanonicalize_tests ut_sync_import_tests boxen_backend_tests boxen_core_tests boxen_input_tests boxen_resize_tests boxen_scroll_tests boxen_chrome_tests boxen_cursor_tests debugger_tui_tests boxen_repl_tests boxen_outline_tests input_decoder_tests +RUN_BUILDABLE = core_tests db_format_tests runtime_tests parser_tests save_migration_tests test_efp_augmentation file_portable_tests file_readline_tests file_verb_tests handle_tests cli_runtime_tests paige_text_tests oppack_tests hashpack_modern_tests hash_corruption_tests langvalue_64_tests table_operations_integration refcon_tests refcon_phase2_tests op_context_tests table_context_tests time_portability_test test_sys_shell_command_verbs headless_selection_tests opattributes_stub_tests headless_thread_registry_tests test_callback_infrastructure tcp_phase1a_unit_tests menu_v7_refcon_tests menudata_headless_tests menu_list_describe_tests meuserselected_headless_tests test_stringerrorlist pane_compositor_tests mouse_parser_tests terminal_control_tests terminal_control_dsr_tests palette_state_tests palette_render_snapshot_tests palette_render_backend_tests palette_arg_inject_tests palette_arg_inject_concurrency_tests repl_palette_source_tests repl_verbs_tests repl_slash_resolver_tests scrollback_pane_tests lang_causedby_snapshot_tests test_getsystemtablescript test_window_bridge copyctopstring_tests ut_sync_canonicalize_tests ut_sync_pathmap_tests ut_sync_pctcodec_tests ut_sync_export_tests ut_sync_decanonicalize_tests ut_sync_import_tests diff_roots_tests boxen_backend_tests boxen_core_tests boxen_input_tests boxen_resize_tests boxen_scroll_tests boxen_chrome_tests boxen_cursor_tests debugger_tui_tests boxen_repl_tests boxen_outline_tests input_decoder_tests # Note: table_verb_tests skipped - pre-existing macOS path detection issue # Note: test_error_messages skipped - needs full runtime linking (tracked in separate task) # Note: test_table_sorting skipped - script execution errors cause segfault (Issue #449) @@ -279,6 +279,11 @@ oppack_tests: oppack_tests.c $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -I../Common/headers -I../Common/SystemHeaders -I../portable \ oppack_tests.c $(LANG_RUNTIME_SOURCES) ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) +# --diff-roots equality walker (root-build Step 1). +diff_roots_tests: diff_roots_tests.c ../frontier-cli/diff_roots.c ../frontier-cli/diff_roots.h + $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -I../Common/headers -I../Common/SystemHeaders -I../portable -I../frontier-cli \ + diff_roots_tests.c ../frontier-cli/diff_roots.c $(LANG_RUNTIME_SOURCES) ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) + # Determinism spike for --diff-roots (root-build Step 1). Deliberately NOT in # RUN_BUILDABLE: it takes a .root path argument and is run by hand to qualify # oppackoutline as the equality basis for script/outline/menubar values. diff --git a/tests/diff_roots_spike.c b/tests/diff_roots_spike.c index 174f530ba..6c7e50834 100644 --- a/tests/diff_roots_spike.c +++ b/tests/diff_roots_spike.c @@ -214,10 +214,6 @@ static void walktable (hdlhashtable htable, char *path, size_t pathsz, int depth snprintf (childpath, sizeof (childpath), "%s", path); appendsegment (childpath, sizeof (childpath), bsname); - if (getenv ("SPIKE_TRACE") != NULL) { - fprintf (stderr, "TRACE %s type=%d\n", childpath, (int) (*val).valuetype); - fflush (stderr); - } if ((*val).valuetype != externalvaluetype) continue; @@ -271,10 +267,6 @@ static void walktable (hdlhashtable htable, char *path, size_t pathsz, int depth if (!(**hv).flinmemory) { - if (getenv ("SPIKE_TRACE") != NULL) { - fprintf (stderr, " materialize extid=%d %s\n", (int) extid, childpath); - fflush (stderr); - } if (!ensure_external_in_memory (&g_walkcontext, hv)) { printf ("LOADFAIL\t%s\textid=%d\n", childpath, (int) extid); @@ -282,10 +274,6 @@ static void walktable (hdlhashtable htable, char *path, size_t pathsz, int depth } } - if (getenv ("SPIKE_TRACE") != NULL) { - fprintf (stderr, " pack extid=%d %s\n", (int) extid, childpath); - fflush (stderr); - } packone (hv, extid, childpath); } @@ -362,16 +350,6 @@ static boolean loadandpack (const char *cpath) { path [0] = '\0'; - { - long ctnodes = 0; - hdlhashnode n; - - for (n = (**hroot).hfirstsort; n != nil; n = (**n).sortedlink) - ctnodes++; - - fprintf (stderr, "spike: root table adr=0x%llx has %ld top-level nodes\n", - (unsigned long long) rootaddress, ctnodes); - } walktable (hroot, path, sizeof (path), 0); diff --git a/tests/diff_roots_tests.c b/tests/diff_roots_tests.c new file mode 100644 index 000000000..18c348a43 --- /dev/null +++ b/tests/diff_roots_tests.c @@ -0,0 +1,239 @@ +/* + * diff_roots_tests.c - Unit tests for the --diff-roots equality walker + * (frontier-cli/diff_roots.c). + * + * The walker is the acceptance instrument for the root-build-from-source plan: + * every later step is verified by "run this and require an empty diff". These + * tests therefore pin the properties that make the instrument TRUSTWORTHY, + * rather than merely exercising its code paths: + * + * - path escaping is lossless and unambiguous, including for the ODB names + * that legitimately contain tab / CR / LF. Virgin.root really does have + * sibling values named with the single bytes 0x0A/0x0B/0x0C/0x0D; under + * newline normalization two of those COLLIDE, which would produce a false + * diff (or a false match). Escaping is what prevents that. + * - a '.' in a reported path is ALWAYS a separator, never part of a name. + * - finding-kind names are stable, because reports get diffed and grepped. + * - overflow is reported, never silently swallowed (the loud-fail rule). + * - operational failures (missing file, non-v7 root) are distinguishable + * from "walk completed and found differences". + * + * Fixture-based whole-root comparisons (identical -> empty, one known mutation + * -> exactly one finding, cross-era -> the known delta set) are acceptance + * evidence run against real .root files, not unit tests -- they need multi-MB + * databases and are driven separately. + * + * License + * ------- + * SPDX-License-Identifier: MIT + * Copyright (c) 2026 Frontier contributors. + */ + +#include +#include +#include + +#include "test_report.h" + +#include "../frontier-cli/diff_roots.h" + +/* ---- finding-kind names are stable and total ---- */ + +static void test_kindname_covers_every_kind(void) { + /* Reports are grepped and diffed, so these strings are a contract. */ + assert(strcmp(diff_roots_kindname(diffkind_only_in_a), "only-in-a") == 0); + assert(strcmp(diff_roots_kindname(diffkind_only_in_b), "only-in-b") == 0); + assert(strcmp(diff_roots_kindname(diffkind_type), "type") == 0); + assert(strcmp(diff_roots_kindname(diffkind_payload), "payload") == 0); + assert(strcmp(diff_roots_kindname(diffkind_unsupported), "unsupported-type") == 0); + assert(strcmp(diff_roots_kindname(diffkind_unreadable), "unreadable") == 0); +} + +static void test_kindname_out_of_range_is_not_a_crash(void) { + /* Must not index off the end of the table. */ + assert(strcmp(diff_roots_kindname((tydiffkind) ctdiffkinds), "unknown") == 0); + assert(strcmp(diff_roots_kindname((tydiffkind) -1), "unknown") == 0); +} + +/* ---- path building: the ordinary cases ---- */ + +static void test_appendsegment_builds_dotted_path(void) { + char path[256]; + + path[0] = '\0'; + assert(diff_roots_appendsegment(path, sizeof(path), "system", 6)); + assert(strcmp(path, "system") == 0); + + assert(diff_roots_appendsegment(path, sizeof(path), "verbs", 5)); + assert(strcmp(path, "system.verbs") == 0); + + assert(diff_roots_appendsegment(path, sizeof(path), "builtins", 8)); + assert(strcmp(path, "system.verbs.builtins") == 0); +} + +static void test_appendsegment_empty_name_is_representable(void) { + char path[256]; + + /* An empty name is a legal (if odd) ODB name; it must not vanish. */ + path[0] = '\0'; + assert(diff_roots_appendsegment(path, sizeof(path), "root", 4)); + assert(diff_roots_appendsegment(path, sizeof(path), "", 0)); + assert(strcmp(path, "root.") == 0); +} + +/* ---- path building: names are bytes ---- */ + +static void test_appendsegment_escapes_tab_cr_lf(void) { + char path[256]; + + /* Real Virgin.root name shape: "0001funnyStuff". */ + path[0] = '\0'; + assert(diff_roots_appendsegment(path, sizeof(path), "0001\tfunnyStuff", 15)); + assert(strcmp(path, "0001%09funnyStuff") == 0); + + path[0] = '\0'; + assert(diff_roots_appendsegment(path, sizeof(path), "\r", 1)); + assert(strcmp(path, "%0D") == 0); + + path[0] = '\0'; + assert(diff_roots_appendsegment(path, sizeof(path), "\n", 1)); + assert(strcmp(path, "%0A") == 0); +} + +static void test_appendsegment_cr_and_lf_siblings_do_not_collide(void) { + /* + * THE case this walker exists to get right. Virgin.root has two sibling + * values under ...aim.code.util.repl named with the single bytes 0x0D and + * 0x0A. Newline-normalizing names merges them (the census saw 11,257 rows + * but only 11,255 unique paths). A path-keyed comparison that merged them + * would report a phantom difference or silently drop a real one. + */ + char patha[256]; + char pathb[256]; + + patha[0] = '\0'; + assert(diff_roots_appendsegment(patha, sizeof(patha), "repl", 4)); + assert(diff_roots_appendsegment(patha, sizeof(patha), "\r", 1)); + + pathb[0] = '\0'; + assert(diff_roots_appendsegment(pathb, sizeof(pathb), "repl", 4)); + assert(diff_roots_appendsegment(pathb, sizeof(pathb), "\n", 1)); + + assert(strcmp(patha, pathb) != 0); + assert(strcmp(patha, "repl.%0D") == 0); + assert(strcmp(pathb, "repl.%0A") == 0); +} + +static void test_appendsegment_escapes_whole_c0_range(void) { + /* + * fix_check.txt in the census archive shows a contiguous run of C0 control + * bytes (0x0B, 0x0C, 0x0D, 0x0A, 0x0E, 0x0F) used as sibling value names, + * so assume the whole 0x00-0x1F range is legal in an ODB name. + */ + int b; + + for (b = 1; b < 0x20; b++) { + char path[64]; + char name[2]; + char expected[16]; + + name[0] = (char) b; + name[1] = '\0'; + + path[0] = '\0'; + assert(diff_roots_appendsegment(path, sizeof(path), name, 1)); + + snprintf(expected, sizeof(expected), "%%%02X", b); + assert(strcmp(path, expected) == 0); + } +} + +static void test_appendsegment_dot_in_name_is_escaped(void) { + /* + * Invariant: a '.' in a REPORTED path is always a separator. A literal '.' + * inside a name must therefore be escaped, or "a.b" (one name) and "a"."b" + * (two levels) would be indistinguishable in the report. + */ + char onename[256]; + char twolevels[256]; + + onename[0] = '\0'; + assert(diff_roots_appendsegment(onename, sizeof(onename), "a.b", 3)); + assert(strcmp(onename, "a%2Eb") == 0); + + twolevels[0] = '\0'; + assert(diff_roots_appendsegment(twolevels, sizeof(twolevels), "a", 1)); + assert(diff_roots_appendsegment(twolevels, sizeof(twolevels), "b", 1)); + assert(strcmp(twolevels, "a.b") == 0); + + assert(strcmp(onename, twolevels) != 0); +} + +static void test_appendsegment_preserves_high_bit_bytes(void) { + /* + * MacRoman bytes pass through as raw bytes -- never decoded, never + * re-encoded as UTF-8 (#880). 0xC7 is the UserTalk comment marker. + */ + char path[256]; + char name[3]; + + name[0] = (char) 0xC7; + name[1] = (char) 0xE9; + name[2] = '\0'; + + path[0] = '\0'; + assert(diff_roots_appendsegment(path, sizeof(path), name, 2)); + assert((unsigned char) path[0] == 0xC7); + assert((unsigned char) path[1] == 0xE9); + assert(path[2] == '\0'); +} + +/* ---- path building: overflow is loud ---- */ + +static void test_appendsegment_overflow_reports_and_preserves(void) { + char path[16]; + char longname[64]; + + memset(longname, 'x', sizeof(longname)); + + strcpy(path, "parent"); + + /* Too long to fit: must fail, and must NOT corrupt the existing path. */ + assert(!diff_roots_appendsegment(path, sizeof(path), longname, sizeof(longname))); + assert(strcmp(path, "parent") == 0); +} + +/* ---- operational failures are distinguishable from differences ---- */ + +static void test_compare_missing_file_is_operational_failure(void) { + tydiffoptions options; + long ctfindings = -1; + + memset(&options, 0, sizeof(options)); + options.flquiet = true; + + /* + * A missing input is NOT "a diff with findings" -- it must return false so + * the caller can exit 2 rather than 1. Conflating the two would let a + * broken CI check masquerade as a real drift report. + */ + assert(!diff_roots_compare("/nonexistent/a.root", "/nonexistent/b.root", + &options, &ctfindings)); +} + +int main(void) { + TR_INIT("diff_roots_tests"); + TR_RUN(test_kindname_covers_every_kind); + TR_RUN(test_kindname_out_of_range_is_not_a_crash); + TR_RUN(test_appendsegment_builds_dotted_path); + TR_RUN(test_appendsegment_empty_name_is_representable); + TR_RUN(test_appendsegment_escapes_tab_cr_lf); + TR_RUN(test_appendsegment_cr_and_lf_siblings_do_not_collide); + TR_RUN(test_appendsegment_escapes_whole_c0_range); + TR_RUN(test_appendsegment_dot_in_name_is_escaped); + TR_RUN(test_appendsegment_preserves_high_bit_bytes); + TR_RUN(test_appendsegment_overflow_reports_and_preserves); + TR_RUN(test_compare_missing_file_is_operational_failure); + TR_SUMMARY(); + return TR_EXIT_CODE(); +} From 319cd0e11af1a5d4a0ee119494eb21264a1c259a Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:47:23 -0700 Subject: [PATCH 04/10] feat(odb): compare mbar/wptext at stored-byte level with distinct reason 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 ced824160-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 5bb712595 and 21d33d2fd. ced824160-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 --- frontier-cli/diff_roots.c | 107 +++++++++++++++++++++++++++++++++----- 1 file changed, 93 insertions(+), 14 deletions(-) diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c index 4e1be11ed..fce68176b 100644 --- a/frontier-cli/diff_roots.c +++ b/frontier-cli/diff_roots.c @@ -362,6 +362,34 @@ static boolean resolvediskvalue (tyrootside *side, const tyvaluerecord *val, Han } /*resolvediskvalue*/ +/* + Read the raw stored block at `adr` from one side, without materializing the + value it belongs to. Used for the types that must not be brought into memory + (see compareexternal). The caller disposes the handle. + + A nil address is a legitimate "nothing stored" state and yields an empty + handle so two nil-address values compare equal rather than both failing. +*/ +static boolean readstoredbytes (tyrootside *side, dbaddress adr, Handle *h) { + + hdldatabaserecord saved = databasedata; + boolean fl; + + *h = nil; + + if (adr == nildbaddress) + return (newemptyhandle (h)); + + databasedata = side->database; /*callee-saves; see resolvediskvalue*/ + + fl = dbrefhandle_context (&side->context, adr, h); + + databasedata = saved; + + return (fl); + } /*readstoredbytes*/ + + /* Inline scalars: compared by value, not by dereference. Returns false when this type is not an inline scalar. @@ -700,24 +728,75 @@ static void compareexternal (tywalkstate *state, const char *path, case idwordprocessor: { /* - Deliberately NOT materialized. mbar: the headless stub fakes - flinmemory without loading, so any dereference segfaults - (stubs/headless_menu_stubs.c:618). wptx: force-materializing - exhausts memory on a full-root walk (db_format.c:1694). - - Compare where they actually live -- their stored addresses. Equal - addresses mean the identical stored block; unequal addresses are - reported honestly as "cannot compare content here" rather than - being claimed as a content difference. + Compared at the STORED level: the raw disk bytes at each side's + external address, never materialized. + + Stored bytes are the ODB's ground truth, and the round-trip law + requires the assembler to reproduce them anyway, so this is the + right basis for these types rather than a concession. + + The two types are here for DIFFERENT reasons, and the reason-codes + below are kept distinct on purpose so a later reader does not merge + them: + + mbar -- cannot be materialized headless AT ALL. The stub + menuverbinmemory_context (stubs/headless_menu_stubs.c:618) + sets flinmemory = true WITHOUT loading, leaving + variabledata a raw dbaddress; dereferencing it segfaults. + Filed as #896. + + wptx -- CAN be materialized, but must not be during a full-root + walk: force-materializing wptext externals exhausts + memory and hangs for minutes (db_format.c:1694). */ - dbaddress adra = (**hva).oldaddress; - dbaddress adrb = (**hvb).oldaddress; + const char *reason = (extid == idmenuprocessor) + ? "mbar: not materializable headless (#896); compared at stored level" + : "wptx: materialization hazard (db_format.c:1694); compared at stored level"; + + Handle ha = nil; + Handle hb = nil; + boolean oka; + boolean okb; + + oka = readstoredbytes (sidea, (**hva).oldaddress, &ha); + okb = readstoredbytes (sideb, (**hvb).oldaddress, &hb); + + if (!oka || !okb) { - if (adra != adrb) - report (state, diffkind_unsupported, path, + if (ha != nil) + disposehandle (ha); + + if (hb != nil) + disposehandle (hb); + + /* + Could not read one or both stored blocks. Report rather than + assume equality -- an unreadable value is a finding. + */ + report (state, diffkind_unreadable, path, typename_for (vala), typename_for (valb), -1, -1, -1, - "not comparable headless; stored addresses differ"); + reason); + return; + } + + { + long sizea = gethandlesize (ha); + long sizeb = gethandlesize (hb); + + if ((sizea != sizeb) + || ((sizea > 0) + && (memcmp (*ha, *hb, (size_t) sizea) != 0))) + report (state, diffkind_payload, path, + typename_for (vala), typename_for (valb), sizea, sizeb, + (sizea == sizeb) + ? firstdifference ((const unsigned char *) *ha, + (const unsigned char *) *hb, sizea) + : -1, + reason); + } + disposehandle (ha); + disposehandle (hb); return; } From 42d104d628bc7db186e0067a02c3c540adca1602 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:49:34 -0700 Subject: [PATCH 05/10] docs(odb): state the date-blindness consequence and list-displacement 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. --- frontier-cli/diff_roots.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c index fce68176b..2d9fb641b 100644 --- a/frontier-cli/diff_roots.c +++ b/frontier-cli/diff_roots.c @@ -498,6 +498,22 @@ static boolean packoutlinebytes (hdlexternalvariable hv, Handle *hpacked) { the kind of thing that legitimately differs between a built root and its ancestor without the CONTENT differing. + CONSEQUENCE, stated plainly so it is not later mistaken for a hole: + excluding timecreated/timelastsave makes this tool BLIND TO CREATE/MOD-DATE + DRIFT BY DESIGN. That is correct for Step 1 drift detection -- a re-saved + root with identical content must compare equal, or the acceptance gate is + useless. Dates are covered long-term by plan decision 9.4: the exporter + preserves per-value dates in the source manifests, so the round-trip law + checks them AS MANIFEST TEXT, not as packed bytes. Content equality here, + date equality there; neither check subsumes the other. + + Packed LISTS inherit these same fields, because oppacklist emits a list + header followed by a packed outline (oplist.c:708-711). The displacement is + READ FROM THE LIST HEADER'S OWN recordsize field (oplist.c:88, "number of + bytes in this header") rather than hardcoded -- see listoutlinebase() -- so + the walker cannot silently drift out of agreement with the format if + tydisklistrecord changes shape. + NOTE: this makes --diff-roots a CONTENT comparison. A dedicated metadata comparison is a separate tool if one is ever wanted. */ From dae566ddda4b9e19f96d6c6fd12a971ff7c815b0 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sun, 16 Aug 2026 00:23:47 -0700 Subject: [PATCH 06/10] fix(odb): P0 -- mbar compare was vacuous; skip-region offsets were wrong 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 --- Common/headers/op.h | 30 ++++++++ Common/source/oppack_v7.c | 56 ++++++++++++++ frontier-cli/diff_roots.c | 158 +++++++++++++++++++++++++++++--------- tests/Makefile | 6 +- tests/diff_roots_tests.c | 107 ++++++++++++++++++++++++++ 5 files changed, 319 insertions(+), 38 deletions(-) diff --git a/Common/headers/op.h b/Common/headers/op.h index 095a03c09..f5ecac2d5 100644 --- a/Common/headers/op.h +++ b/Common/headers/op.h @@ -609,6 +609,36 @@ extern boolean oppack (Handle *); /*oppack.c*/ extern boolean oppackoutline (hdloutlinerecord, Handle *); +/* + Volatile (save-metadata) regions of the packed-outline header, for tools + that compare packed outlines for CONTENT equality. + + These fields are rewritten by the act of saving -- and ctsaves is + INCREMENTED by oppack itself -- so an equality check over raw packed bytes + must exclude them, or two semantically identical outlines compare unequal. + + Exposed as a table derived with offsetof()/sizeof() on the real struct + (which is file-local to oppack_v7.c) so callers cannot drift out of + agreement with the on-disk layout. Only fields with DEMONSTRATED volatility + are listed; anything else must surface as a real difference rather than be + silently swallowed. + + op_packed_header_volatile_regions() writes up to *ctregions entries into + regions[] and sets *ctregions to the number written; returns false if the + caller's array is too small. op_packed_header_size() is the total header + size, so a caller can bound-check an offset before consulting the table. +*/ +typedef struct typackedheaderregion { + + long offset; + long length; + + } typackedheaderregion; + +extern long op_packed_header_size (void); + +extern boolean op_packed_header_volatile_regions (typackedheaderregion *regions, long *ctregions); + extern boolean opunpack (Handle, long *, hdloutlinerecord *); extern boolean opunpackoutline (Handle, hdloutlinerecord *); diff --git a/Common/source/oppack_v7.c b/Common/source/oppack_v7.c index e085aaf49..82b357357 100644 --- a/Common/source/oppack_v7.c +++ b/Common/source/oppack_v7.c @@ -154,6 +154,62 @@ typedef struct typortablediskheader { _Static_assert (sizeof (typortablediskheader) == 1068, "typortablediskheader must be 1068 bytes"); +long op_packed_header_size (void) { + + return ((long) sizeof (typortablediskheader)); + } /*op_packed_header_size*/ + + +boolean op_packed_header_volatile_regions (typackedheaderregion *regions, long *ctregions) { + + /* + The save-metadata fields of the packed header, derived from the struct + rather than hardcoded so a layout change cannot silently desynchronize a + caller. See op.h for why callers need these. + + Only DEMONSTRATED volatility is listed: + + timecreated -- per-value creation stamp + timelastsave -- rewritten on every save + ctsaves -- incremented by oppack() itself, below, so it differs + even between two packs of one resident value + + outlinesignature is deliberately NOT listed. It is a caller-defined cookie + that is constant in practice (observed as 'LAND' throughout the shipped + roots), and excluding a field with no demonstrated volatility would let a + real difference pass silently -- the exact failure mode these exclusions + exist to avoid. + */ + + const typackedheaderregion volatileregions [] = { + {(long) offsetof (typortablediskheader, timecreated), + (long) sizeof (((typortablediskheader *) 0)->timecreated)}, + {(long) offsetof (typortablediskheader, timelastsave), + (long) sizeof (((typortablediskheader *) 0)->timelastsave)}, + {(long) offsetof (typortablediskheader, ctsaves), + (long) sizeof (((typortablediskheader *) 0)->ctsaves)} + }; + + const long ctvolatile = (long) (sizeof (volatileregions) / sizeof (volatileregions [0])); + long i; + + if ((regions == NULL) || (ctregions == NULL)) + return (false); + + if (*ctregions < ctvolatile) { + *ctregions = ctvolatile; /*tell the caller how much room it needs*/ + return (false); + } + + for (i = 0; i < ctvolatile; i++) + regions [i] = volatileregions [i]; + + *ctregions = ctvolatile; + + return (true); + } /*op_packed_header_volatile_regions*/ + + typedef struct tyoppackinfo { handlestream *packstream; diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c index 2d9fb641b..128e043ba 100644 --- a/frontier-cli/diff_roots.c +++ b/frontier-cli/diff_roots.c @@ -362,13 +362,52 @@ static boolean resolvediskvalue (tyrootside *side, const tyvaluerecord *val, Han } /*resolvediskvalue*/ +/* + Where an unmaterialized external's stored block actually lives. + + oldaddress is NOT the answer on its own. For a value that has never been + materialized, langnewexternalvariable initializes oldaddress to nildbaddress + (langexternal.c:2932, :2997) and puts the stored address 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. + + An earlier version of this walker read only oldaddress. For every + never-materialized menubar that was nil on BOTH sides, which combined with a + nil-means-empty-handle rule to make every mbar and wptext compare EQUAL + regardless of content. The comparison was vacuous, and self-diff plus a + cross-era diff both "passed" it because neither had a fixture where a + menubar actually differed. + + The fallback below mirrors what the menu stub itself does + (headless_menu_stubs.c:551-553). + + Returns nildbaddress only when the value genuinely has no stored block; the + caller must treat that as reportable, never as equality. +*/ +static dbaddress storedaddressof (hdlexternalvariable hv) { + + dbaddress adr; + + if (hv == nil) + return (nildbaddress); + + adr = (**hv).oldaddress; + + if ((adr == nildbaddress) && !(**hv).flinmemory) + adr = (dbaddress) (**hv).variabledata; + + return (adr); + } /*storedaddressof*/ + + /* Read the raw stored block at `adr` from one side, without materializing the value it belongs to. Used for the types that must not be brought into memory (see compareexternal). The caller disposes the handle. - A nil address is a legitimate "nothing stored" state and yields an empty - handle so two nil-address values compare equal rather than both failing. + A nil address returns FALSE rather than an empty handle. Treating "no + address" as "empty content" is what made the mbar comparison silently + vacuous; the caller reports it instead. */ static boolean readstoredbytes (tyrootside *side, dbaddress adr, Handle *h) { @@ -378,7 +417,7 @@ static boolean readstoredbytes (tyrootside *side, dbaddress adr, Handle *h) { *h = nil; if (adr == nildbaddress) - return (newemptyhandle (h)); + return (false); databasedata = side->database; /*callee-saves; see resolvediskvalue*/ @@ -476,22 +515,43 @@ static boolean packoutlinebytes (hdlexternalvariable hv, Handle *hpacked) { /* - Volatile regions of the packed-outline header (oppack_v7.c:115-152), given - as [offset, offset+length). These carry SAVE METADATA, not content, and they - must be excluded from an equality basis or every value in a re-saved root - reports as different. - - Verified empirically: comparing a root against a copy that had been opened - and saved once produced 3,527 spurious payload findings, 3,495 of them - differing first at offset 37 (inside timelastsave) and 27 at offset 49 - (inside outlinesignature). - - timecreated offset 24, 8 bytes -- creation stamp, per-value - timelastsave offset 32, 8 bytes -- rewritten on every save - ctsaves offset 40, 4 bytes -- INCREMENTED by oppack itself - (oppack_v7.c:622), so it differs - even between two packs - outlinesignature offset 48, 4 bytes -- caller-defined cookie + The packed-outline header carries SAVE METADATA, not content, and those + fields must be excluded from an equality basis or every value in a re-saved + root reports as different. + + The regions are NOT hardcoded here. They come from + op_packed_header_volatile_regions() (op.h / oppack_v7.c), derived with + offsetof()/sizeof() on the real struct. That indirection exists because the + first version of this walker DID hardcode them and got them wrong by +6: a + local probe of the struct written WITHOUT #pragma pack(2) reported + 24/32/40/48 with a 1080-byte header, while the real packed layout is + 18/26/34 with 1068 bytes. The _Static_assert at oppack_v7.c:154 pins 1068 + and would have caught it. Two silent failures resulted -- timecreated + (18-23) was left UNSKIPPED, producing false diffs whenever creation stamps + differ, and fltextmode (38-39) was WRONGLY skipped, letting real content + compare equal. Deriving from the struct makes that class of error + impossible. + + Excluded, all with demonstrated volatility: + + timecreated -- per-value creation stamp + timelastsave -- rewritten on every save + ctsaves -- INCREMENTED by oppack itself (oppack_v7.c), so it differs + even between two packs of one resident value + + NOT excluded: outlinesignature. An earlier version skipped it on the + assumption it was volatile. Dumping real packed values from Virgin.root + shows it is the constant 'LAND' throughout, and excluding a field with no + demonstrated volatility is precisely the silent-false-equality failure this + whole exclusion mechanism must avoid. If a signature difference ever appears + it should surface as a finding and be attributed, not pre-swallowed. + + Empirical note on the original 3,527 spurious findings: 3,495 first differed + at offset 37 and 27 at offset 49. Both are ctsaves -- offset 37 is its last + byte in a bare packed outline (34..37), and offset 49 is its last byte + inside a packed LIST, displaced by the 12-byte list header (12+34=46..49). + The earlier attribution of those to timelastsave and outlinesignature was + wrong; the DATA was right, the field names were not. timecreated is excluded too: it is per-VALUE provenance metadata that the root-build plan preserves in the manifest (decision 9.4), so it is exactly @@ -517,31 +577,52 @@ static boolean packoutlinebytes (hdlexternalvariable hv, Handle *hpacked) { NOTE: this makes --diff-roots a CONTENT comparison. A dedicated metadata comparison is a separate tool if one is ever wanted. */ -typedef struct tyskipregion { - long offset; - long length; - } tyskipregion; +#define maxheaderregions 8 -static const tyskipregion outlineheaderskips [] = { - {24, 8}, /*timecreated*/ - {32, 8}, /*timelastsave*/ - {40, 4}, /*ctsaves*/ - {48, 4} /*outlinesignature*/ - }; +static typackedheaderregion g_headerregions [maxheaderregions]; +static long g_ctheaderregions = 0; +static long g_headerbytes = 0; +static boolean g_headerregionsready = false; + + +/* + Load the volatile-region table once, from oppack_v7.c via offsetof/sizeof on + the real struct. Hardcoding these offsets is how the first version of this + walker got them wrong by +6: a local probe of the struct WITHOUT + #pragma pack(2) reported 24/32/40/48 and a 1080-byte header, when the real + packed layout is 18/26/34 and 1068 bytes (the _Static_assert at + oppack_v7.c:154 is the invariant that would have caught it). + + Returns false if the table cannot be obtained, in which case the caller + compares the whole buffer verbatim -- over-reporting, never under-reporting. +*/ +static boolean loadheaderregions (void) { + + if (g_headerregionsready) + return (g_ctheaderregions > 0); + + g_headerregionsready = true; + + g_ctheaderregions = maxheaderregions; + + if (!op_packed_header_volatile_regions (g_headerregions, &g_ctheaderregions)) { + g_ctheaderregions = 0; + return (false); + } -#define ctoutlineheaderskips ((long) (sizeof (outlineheaderskips) / sizeof (outlineheaderskips [0]))) + g_headerbytes = op_packed_header_size (); -/*Total packed-outline header size (_Static_assert at oppack_v7.c:154).*/ -#define outlineheaderbytes 1068 + return (g_ctheaderregions > 0); + } /*loadheaderregions*/ static boolean inskipregion (long offset) { long i; - for (i = 0; i < ctoutlineheaderskips; i++) { + for (i = 0; i < g_ctheaderregions; i++) { - const tyskipregion *r = &outlineheaderskips [i]; + const typackedheaderregion *r = &g_headerregions [i]; if ((offset >= r->offset) && (offset < (r->offset + r->length))) return (true); @@ -567,6 +648,7 @@ static long outlinecontentdifference (const unsigned char *a, long cta, long outlinebase) { long i; + boolean haveregions = loadheaderregions (); if (cta != ctb) return ((cta < ctb) ? cta : ctb); /*length difference is a content difference*/ @@ -580,7 +662,9 @@ static long outlinecontentdifference (const unsigned char *a, long cta, relative = i - outlinebase; - if ((relative >= 0) && (relative < outlineheaderbytes) && inskipregion (relative)) + if (haveregions + && (relative >= 0) && (relative < g_headerbytes) + && inskipregion (relative)) continue; return (i); @@ -774,8 +858,8 @@ static void compareexternal (tywalkstate *state, const char *path, boolean oka; boolean okb; - oka = readstoredbytes (sidea, (**hva).oldaddress, &ha); - okb = readstoredbytes (sideb, (**hvb).oldaddress, &hb); + oka = readstoredbytes (sidea, storedaddressof (hva), &ha); + okb = readstoredbytes (sideb, storedaddressof (hvb), &hb); if (!oka || !okb) { diff --git a/tests/Makefile b/tests/Makefile index 007aead9a..475f08f75 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -280,7 +280,10 @@ oppack_tests: oppack_tests.c oppack_tests.c $(LANG_RUNTIME_SOURCES) ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) # --diff-roots equality walker (root-build Step 1). -diff_roots_tests: diff_roots_tests.c ../frontier-cli/diff_roots.c ../frontier-cli/diff_roots.h +# oppack_v7.c is a listed prerequisite because it supplies the packed-header +# volatile-region table these tests pin; without it a stale binary silently +# passes after that table changes (observed during the fix round). +diff_roots_tests: diff_roots_tests.c ../frontier-cli/diff_roots.c ../frontier-cli/diff_roots.h ../Common/source/oppack_v7.c ../Common/headers/op.h $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -I../Common/headers -I../Common/SystemHeaders -I../portable -I../frontier-cli \ diff_roots_tests.c ../frontier-cli/diff_roots.c $(LANG_RUNTIME_SOURCES) ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) @@ -1052,3 +1055,4 @@ verify-errors: check_v6_tables: check_v6_tables.c $(LANG_RUNTIME_SOURCES) ../Common/source/odbengine.c ../frontier-cli/stubs/headless_shell.c $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -DHEADLESS_TEST_PORTABLE_FILE -DHEADLESS_LINKS_REAL_DB -I../Common/headers -I../Common/SystemHeaders -I../portable check_v6_tables.c \ $(LANG_RUNTIME_SOURCES) ../Common/source/odbengine.c ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) + diff --git a/tests/diff_roots_tests.c b/tests/diff_roots_tests.c index 18c348a43..9d3e181d3 100644 --- a/tests/diff_roots_tests.c +++ b/tests/diff_roots_tests.c @@ -35,6 +35,7 @@ #include "test_report.h" +#include "../Common/headers/op.h" /* op_packed_header_volatile_regions */ #include "../frontier-cli/diff_roots.h" /* ---- finding-kind names are stable and total ---- */ @@ -221,6 +222,106 @@ static void test_compare_missing_file_is_operational_failure(void) { &options, &ctfindings)); } +/* ---- packed-header volatile regions: pinned to the real struct ---- */ + +/* + * These pin the exact bug that shipped in the first version of this walker: the + * skip regions were hardcoded from a probe of the header struct written WITHOUT + * #pragma pack(2), so every offset was 6 bytes too high. That left timecreated + * unskipped (false diffs whenever creation stamps differ) and wrongly skipped + * fltextmode (real content silently comparing equal). + * + * The regions now come from op_packed_header_volatile_regions(), derived with + * offsetof() on the real struct. These tests assert what that derivation must + * produce, so a layout change is caught here rather than by a silently wrong + * diff months later. + */ + +static void test_packed_header_size_matches_format(void) { + /* The _Static_assert in oppack_v7.c pins this. If it ever changes, every + skip window moves and that must be a deliberate decision, not a surprise. */ + assert(op_packed_header_size() == 1068); +} + +static void test_volatile_regions_are_the_proven_three(void) { + typackedheaderregion regions[8]; + long ct = 8; + + assert(op_packed_header_volatile_regions(regions, &ct)); + + /* Exactly the fields with DEMONSTRATED volatility -- no speculative + entries. outlinesignature is deliberately absent: it reads as the + constant 'LAND' throughout the shipped roots, and skipping an unproven + field is the same silent-false-equality class this mechanism exists to + prevent. */ + assert(ct == 3); + + assert(regions[0].offset == 18); /* timecreated */ + assert(regions[0].length == 8); + assert(regions[1].offset == 26); /* timelastsave */ + assert(regions[1].length == 8); + assert(regions[2].offset == 34); /* ctsaves */ + assert(regions[2].length == 4); +} + +static void test_volatile_regions_exclude_fltextmode(void) { + typackedheaderregion regions[8]; + long ct = 8; + long i; + + assert(op_packed_header_volatile_regions(regions, &ct)); + + /* fltextmode lives at 38..39 and is real CONTENT. The old +6 table covered + it, which made text-mode changes invisible. */ + for (i = 0; i < ct; i++) { + long lo = regions[i].offset; + long hi = regions[i].offset + regions[i].length; + assert(!(38 >= lo && 38 < hi)); + assert(!(39 >= lo && 39 < hi)); + } +} + +static void test_volatile_regions_cover_timecreated(void) { + typackedheaderregion regions[8]; + long ct = 8; + long i; + int covered18 = 0; + int covered23 = 0; + + assert(op_packed_header_volatile_regions(regions, &ct)); + + /* The old table started at 24, leaving timecreated's first six bytes + (18..23) exposed. Both ends must be covered now. */ + for (i = 0; i < ct; i++) { + long lo = regions[i].offset; + long hi = regions[i].offset + regions[i].length; + if (18 >= lo && 18 < hi) covered18 = 1; + if (23 >= lo && 23 < hi) covered23 = 1; + } + + assert(covered18); + assert(covered23); +} + +static void test_volatile_regions_reports_needed_size(void) { + typackedheaderregion regions[1]; + long ct = 1; + + /* Too small: must fail AND report how much room is needed, rather than + silently truncating the skip table -- which would resurrect the original + bug in a new form. */ + assert(!op_packed_header_volatile_regions(regions, &ct)); + assert(ct == 3); +} + +static void test_volatile_regions_rejects_null(void) { + typackedheaderregion regions[8]; + long ct = 8; + + assert(!op_packed_header_volatile_regions(NULL, &ct)); + assert(!op_packed_header_volatile_regions(regions, NULL)); +} + int main(void) { TR_INIT("diff_roots_tests"); TR_RUN(test_kindname_covers_every_kind); @@ -234,6 +335,12 @@ int main(void) { TR_RUN(test_appendsegment_preserves_high_bit_bytes); TR_RUN(test_appendsegment_overflow_reports_and_preserves); TR_RUN(test_compare_missing_file_is_operational_failure); + TR_RUN(test_packed_header_size_matches_format); + TR_RUN(test_volatile_regions_are_the_proven_three); + TR_RUN(test_volatile_regions_exclude_fltextmode); + TR_RUN(test_volatile_regions_cover_timecreated); + TR_RUN(test_volatile_regions_reports_needed_size); + TR_RUN(test_volatile_regions_rejects_null); TR_SUMMARY(); return TR_EXIT_CODE(); } From c1e774f42d102363827d892873b5e9a97519e500 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sun, 16 Aug 2026 00:32:03 -0700 Subject: [PATCH 07/10] fix(odb): P1 -- name-keyed matching (hfirstsort is insertion order) + 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 --- frontier-cli/cli_parser.c | 11 +++ frontier-cli/diff_roots.c | 188 +++++++++++++++++++++++++++++++------- tests/diff_roots_tests.c | 38 ++++++++ 3 files changed, 204 insertions(+), 33 deletions(-) diff --git a/frontier-cli/cli_parser.c b/frontier-cli/cli_parser.c index 74274f3e3..03f813d88 100644 --- a/frontier-cli/cli_parser.c +++ b/frontier-cli/cli_parser.c @@ -187,6 +187,17 @@ boolean cli_validate_options(const cli_options_t* options) { log_error(LOG_COMP_GENERAL, "Error: --diff-roots runs standalone; do not combine with --migrate, --system-root, scripts, or REPL mode"); return false; } + /* ut-sync is a WRITE path: its import hook rewrites outlines, sets + * dirty flags, and writes .ut-sync-state. It must never run under a + * mode whose entire contract is that neither root is modified. + * Rejected loudly rather than quietly ignored, because + * FRONTIER_UT_SYNC_DIR populates this field from the environment just + * above this validation -- so it can be on without appearing on the + * command line at all. */ + if (options->ut_sync_dir != NULL) { + log_error(LOG_COMP_GENERAL, "Error: --diff-roots cannot be combined with --ut-sync-dir or FRONTIER_UT_SYNC_DIR; the walker opens both roots read-only and ut-sync would write to them"); + return false; + } return true; } diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c index 128e043ba..e67468981 100644 --- a/frontier-cli/diff_roots.c +++ b/frontier-cli/diff_roots.c @@ -1137,6 +1137,79 @@ static int comparenames (const unsigned char *bsa, const unsigned char *bsb) { } /*comparenames*/ +/* + One table entry, buffered so a table can be sorted by name before matching. + `name` is a COPY of the node's Pascal key -- gethashkey writes into a + caller-supplied bigstring, so the bytes must be captured rather than + pointed at. +*/ +typedef struct tynodeentry { + + bigstring name; + hdlhashnode node; + + } tynodeentry; + + +static int comparenodeentries (const void *a, const void *b) { + + return (comparenames (((const tynodeentry *) a)->name, + ((const tynodeentry *) b)->name)); + } /*comparenodeentries*/ + + +/* + Buffer every node of a table and sort by raw name bytes. + + Sorting is what makes matching correct AND output deterministic: hfirstsort + is insertion order in this build (see walkpair), so neither property comes + for free. Sorting by the same byte comparison used for matching keeps the + two consistent. + + Returns false only on allocation failure. An empty table yields ct 0 and a + NULL array, which the caller handles as "nothing on this side". +*/ +static boolean collectnodes (hdlhashtable htable, tynodeentry **pentries, long *pct) { + + hdlhashnode nomad; + long ct = 0; + long i = 0; + tynodeentry *entries; + + *pentries = NULL; + *pct = 0; + + if (htable == nil) + return (true); + + for (nomad = (**htable).hfirstsort; nomad != nil; nomad = (**nomad).sortedlink) + ct++; + + if (ct == 0) + return (true); + + entries = (tynodeentry *) malloc ((size_t) ct * sizeof (tynodeentry)); + + if (entries == NULL) + return (false); + + for (nomad = (**htable).hfirstsort; (nomad != nil) && (i < ct); + nomad = (**nomad).sortedlink) { + + gethashkey (nomad, entries [i].name); + entries [i].node = nomad; + i++; + } + + qsort (entries, (size_t) i, sizeof (tynodeentry), comparenodeentries); + + *pentries = entries; + *pct = i; + + return (true); + } /*collectnodes*/ + + static void reportsubtree (tywalkstate *state, const char *parentpath, hdlhashnode node, tydiffkind kind, int depth); @@ -1229,9 +1302,6 @@ static void walkpair (tywalkstate *state, const char *path, tyrootside *sidea, tyrootside *sideb, hdlhashtable tablea, hdlhashtable tableb, int depth) { - hdlhashnode nodea; - hdlhashnode nodeb; - if (depth >= maxwalkdepth) { report (state, diffkind_unreadable, path, "tabl", "tabl", -1, -1, -1, "maximum walk depth exceeded"); @@ -1244,43 +1314,67 @@ static void walkpair (tywalkstate *state, const char *path, return; } - nodea = (**tablea).hfirstsort; - nodeb = (**tableb).hfirstsort; - /* - Sorted merge. Both sides are in the ODB's own stored order, so a single - lockstep pass yields every finding in deterministic order with no sorting - and no allocation. + NAME-KEYED MATCHING, not a positional merge over hfirstsort. + + hfirstsort is NOT alphabetical in this build. Sorted insertion consults + langcallbacks.comparenodescallback (langhash.c:1464), and the headless + startup wires that to cb_noop_compare, which always returns 0 + (langstartup.c:81, :1172) -- so every node appends and the list is in + INSERTION order. + + An earlier version merged the two lists positionally on that assumption. + Two roots holding identical values inserted in different orders produced + four spurious findings for three values, each reported as BOTH only-in-A + and only-in-B, while their payloads were never compared at all. That is the + exact hazard: not just noise, but masked payload comparisons. + + So: buffer both sides, sort each by raw name bytes, then merge. Sorting + makes the output deterministic regardless of how either root happens to + store its nodes, which is what a drift check needs. */ - while ((nodea != nil) || (nodeb != nil)) { + { + tynodeentry *entriesa = NULL; + tynodeentry *entriesb = NULL; + long cta = 0; + long ctb = 0; + long ia = 0; + long ib = 0; + + if (!collectnodes (tablea, &entriesa, &cta) || !collectnodes (tableb, &entriesb, &ctb)) { + + free (entriesa); + free (entriesb); + + report (state, diffkind_unreadable, path, "tabl", "tabl", -1, -1, -1, + "out of memory collecting table entries"); + return; + } + + while ((ia < cta) || (ib < ctb)) { - bigstring bsa; - bigstring bsb; int order; - if (nodea == nil) + if (ia >= cta) order = 1; - else if (nodeb == nil) + else if (ib >= ctb) order = -1; - else { - gethashkey (nodea, bsa); - gethashkey (nodeb, bsb); - order = comparenames (bsa, bsb); - } + else + order = comparenames (entriesa [ia].name, entriesb [ib].name); if (order < 0) { - gethashkey (nodea, bsa); - reportonlyside (state, path, bsa, &(**nodea).val, diffkind_only_in_a, - nodea, depth); - nodea = (**nodea).sortedlink; + reportonlyside (state, path, entriesa [ia].name, + &(**(entriesa [ia].node)).val, diffkind_only_in_a, + entriesa [ia].node, depth); + ia++; continue; } if (order > 0) { - gethashkey (nodeb, bsb); - reportonlyside (state, path, bsb, &(**nodeb).val, diffkind_only_in_b, - nodeb, depth); - nodeb = (**nodeb).sortedlink; + reportonlyside (state, path, entriesb [ib].name, + &(**(entriesb [ib].node)).val, diffkind_only_in_b, + entriesb [ib].node, depth); + ib++; continue; } @@ -1289,29 +1383,35 @@ static void walkpair (tywalkstate *state, const char *path, char *childpath = (char *) malloc (maxdiffpath); if (childpath == NULL) { + free (entriesa); + free (entriesb); report (state, diffkind_unreadable, path, "-", "-", -1, -1, -1, "out of memory building path"); return; } - gethashkey (nodea, bsa); - snprintf (childpath, maxdiffpath, "%s", path); if (!diff_roots_appendsegment (childpath, maxdiffpath, - (const char *) &bsa [1], (size_t) bsa [0])) + (const char *) &entriesa [ia].name [1], + (size_t) entriesa [ia].name [0])) report (state, diffkind_unreadable, path, "-", "-", -1, -1, -1, "path too long to report"); else comparevalue (state, childpath, sidea, sideb, - &(**nodea).val, &(**nodeb).val, depth); + &(**(entriesa [ia].node)).val, + &(**(entriesb [ib].node)).val, depth); free (childpath); } - nodea = (**nodea).sortedlink; - nodeb = (**nodeb).sortedlink; + ia++; + ib++; } + + free (entriesa); + free (entriesb); + } } /*walkpair*/ @@ -1439,6 +1539,28 @@ boolean diff_roots_compare (const char *patha, const char *pathb, state.out = (options->out != NULL) ? options->out : stdout; state.ctfindings = 0; + /* + Structural guard, not merely a CLI check. ut-sync's import hook rewrites + outlines, sets dirty flags, and writes .ut-sync-state -- all writes, under a + mode that promises neither root is touched. cli_validate_options rejects the + combination on the CLI path, but this function is callable directly (the + unit tests do exactly that), and FRONTIER_UT_SYNC_DIR can turn ut-sync on + from the environment with no flag on the command line at all. + + The env var is checked here rather than calling back into the CLI layer: + this module must not depend on main.c, and the env var is the source that + can be set without anyone passing an argument. + */ + { + const char *utsyncenv = getenv ("FRONTIER_UT_SYNC_DIR"); + + if ((utsyncenv != NULL) && (utsyncenv [0] != '\0')) { + fprintf (err, "diff-roots: refusing to run with FRONTIER_UT_SYNC_DIR set; " + "ut-sync writes to roots this mode opens read-only\n"); + return (false); + } + } + if (!db_format_prepare_runtime ()) { fprintf (err, "diff-roots: database runtime failed to initialize\n"); return (false); diff --git a/tests/diff_roots_tests.c b/tests/diff_roots_tests.c index 9d3e181d3..1716e0ecf 100644 --- a/tests/diff_roots_tests.c +++ b/tests/diff_roots_tests.c @@ -32,6 +32,7 @@ #include #include #include +#include /* setenv/unsetenv for the ut-sync guard tests */ #include "test_report.h" @@ -222,6 +223,42 @@ static void test_compare_missing_file_is_operational_failure(void) { &options, &ctfindings)); } +/* ---- ut-sync must never be active under a read-only walk ---- */ + +static void test_compare_refuses_when_ut_sync_env_set(void) { + tydiffoptions options; + long ctfindings = -1; + + memset(&options, 0, sizeof(options)); + options.flquiet = true; + + /* + * ut-sync's import hook rewrites outlines, sets dirty flags, and writes + * .ut-sync-state -- writes, under a mode whose contract is that neither + * root is touched. The CLI rejects the combination, but this entry point is + * callable directly and FRONTIER_UT_SYNC_DIR can enable ut-sync with no + * flag on the command line, so the refusal also lives inside the walker. + * + * HONEST SCOPE OF THIS TEST: it asserts the call is refused, but it cannot + * prove the ut-sync guard specifically is what refused it. Every input a + * unit test can supply without shipping a multi-MB fixture (missing file, + * non-v7 file) is rejected by an earlier check anyway, so a mutation that + * removes the guard still leaves this passing -- verified, it did. + * + * The guard's real proof is at the acceptance layer, where the CLI is run + * with FRONTIER_UT_SYNC_DIR set against two genuine v7 roots that otherwise + * compare clean, and the run is rejected. This case is kept as a + * regression tripwire for the refusal contract (operational failure -> + * false, never "a diff with findings"), not as proof of the mechanism. + */ + setenv("FRONTIER_UT_SYNC_DIR", "/tmp/diff-roots-must-refuse", 1); + + assert(!diff_roots_compare("/nonexistent/a.root", "/nonexistent/b.root", + &options, &ctfindings)); + + unsetenv("FRONTIER_UT_SYNC_DIR"); +} + /* ---- packed-header volatile regions: pinned to the real struct ---- */ /* @@ -335,6 +372,7 @@ int main(void) { TR_RUN(test_appendsegment_preserves_high_bit_bytes); TR_RUN(test_appendsegment_overflow_reports_and_preserves); TR_RUN(test_compare_missing_file_is_operational_failure); + TR_RUN(test_compare_refuses_when_ut_sync_env_set); TR_RUN(test_packed_header_size_matches_format); TR_RUN(test_volatile_regions_are_the_proven_three); TR_RUN(test_volatile_regions_exclude_fltextmode); From 801e41bfb098af8c11c7ddecb51e92b6b355cead Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sun, 16 Aug 2026 00:39:12 -0700 Subject: [PATCH 08/10] fix(odb): P2 batch -- resource teardown, float bytes, subtree depth, 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 --- frontier-cli/diff_roots.c | 145 ++++++++++++++++++++++++++++++++------ frontier-cli/diff_roots.h | 14 +++- 2 files changed, 136 insertions(+), 23 deletions(-) diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c index e67468981..5718c02f1 100644 --- a/frontier-cli/diff_roots.c +++ b/frontier-cli/diff_roots.c @@ -171,7 +171,31 @@ static void report (tywalkstate *state, tydiffkind kind, const char *path, if (state->options->flquiet) return; - fprintf (state->out, "%-16s %s", diff_roots_kindname (kind), path); + fprintf (state->out, "%-16s ", diff_roots_kindname (kind)); + + /* + High-bit bytes are escaped HERE, at the report layer only. + + The shared name codec (ut_pct_encode_segment) passes >= 0x80 through + untouched, and that is correct -- it is a lossless bijection used for + filesystem paths, and MacRoman bytes must survive it byte-for-byte (#880). + But a terminal will render those bytes through whatever locale it happens + to have, so a REPORT that emits them raw is ambiguous to read and unsafe to + paste into a bug. Escaping at the point of display keeps the stored path + lossless everywhere else -- the finding handed to a callback still carries + the raw bytes. + */ + { + const unsigned char *p; + + for (p = (const unsigned char *) path; *p != '\0'; p++) { + + if (*p >= 0x80) + fprintf (state->out, "%%%02X", (unsigned) *p); + else + fputc ((int) *p, state->out); + } + } if ((kind == diffkind_type) || (kind == diffkind_unsupported)) fprintf (state->out, " A=%s B=%s", finding.typea, finding.typeb); @@ -477,7 +501,15 @@ static boolean inlineequal (const tyvaluerecord *a, const tyvaluerecord *b, bool return (true); case singlevaluetype: - *pequal = ((*a).data.singlevalue == (*b).data.singlevalue); + /* + memcmp, not ==. This is a STORED-BYTES comparison, and float + equality is the wrong predicate for it twice over: NaN != NaN would + report two identical stored values as different, and +0.0 == -0.0 + would report two different stored values as identical. Comparing + the bytes answers the question actually being asked. + */ + *pequal = (memcmp (&(*a).data.singlevalue, &(*b).data.singlevalue, + sizeof ((*a).data.singlevalue)) == 0); return (true); case pointvaluetype: @@ -901,9 +933,19 @@ static void compareexternal (tywalkstate *state, const char *path, } default: + /* + Reached only by external kinds that do not occur in any shipped + root: the 2026-08-11 census records no pict (and no filespec, alias + or code scalars) across Virgin.root, mainResponder.root or + manila.root. Rather than write comparison code that cannot be + exercised -- and so cannot be trusted when it finally is -- these + fail loudly with the path, which is the documented contract. The + first root that carries one turns into a red build naming exactly + what to implement. + */ report (state, diffkind_unsupported, path, typename_for (vala), typename_for (valb), -1, -1, -1, - "unhandled external kind"); + "unhandled external kind (not present in any shipped root)"); return; } } /*compareexternal*/ @@ -1137,6 +1179,32 @@ static int comparenames (const unsigned char *bsa, const unsigned char *bsb) { } /*comparenames*/ +/* + Exact buffer size for `parent` + '.' + one encoded segment + NUL. + + The buffer GROWS with the path instead of sitting at a fixed cap. A diff + tool must never fail to NAME a path, and a fixed cap silently converts a + deep path into an unreportable one -- the failure mode both existing ODB + walkers have (main.c:2429, langhash.c:297). Worst case per segment is three + characters per raw byte, when every byte percent-escapes. + + maxdiffpath survives only as a sanity ceiling for absurd input, not as the + working size. +*/ +static size_t pathbufsize (const char *parent, size_t namelen) { + + size_t need = strlen (parent) + 1 /*separator*/ + (namelen * 3) + 1 /*NUL*/; + + if (need < 64) + need = 64; + + if (need > (size_t) maxdiffpath) + need = (size_t) maxdiffpath; + + return (need); + } /*pathbufsize*/ + + /* One table entry, buffered so a table can be sorted by name before matching. `name` is a COPY of the node's Pascal key -- gethashkey writes into a @@ -1210,7 +1278,7 @@ static boolean collectnodes (hdlhashtable htable, tynodeentry **pentries, long * } /*collectnodes*/ -static void reportsubtree (tywalkstate *state, const char *parentpath, +static void reportsubtree (tywalkstate *state, tyrootside *side, const char *parentpath, hdlhashnode node, tydiffkind kind, int depth); @@ -1218,7 +1286,7 @@ static void reportsubtree (tywalkstate *state, const char *parentpath, Report an entire present-on-one-side-only subtree, so a missing table is not reported as a single line that hides thousands of values. */ -static void reportonlyside (tywalkstate *state, const char *parentpath, +static void reportonlyside (tywalkstate *state, tyrootside *side, const char *parentpath, const unsigned char *bsname, const tyvaluerecord *val, tydiffkind kind, hdlhashnode node, int depth) { @@ -1229,7 +1297,8 @@ static void reportonlyside (tywalkstate *state, const char *parentpath, frame overflows the 8 MB default stack well before that depth. Learned the hard way: the first build segfaulted here on manila.root. */ - char *path = (char *) malloc (maxdiffpath); + size_t pathsz = pathbufsize (parentpath, (size_t) bsname [0]); + char *path = (char *) malloc (pathsz); if (path == NULL) { report (state, diffkind_unreadable, parentpath, "-", "-", -1, -1, -1, @@ -1237,9 +1306,9 @@ static void reportonlyside (tywalkstate *state, const char *parentpath, return; } - snprintf (path, maxdiffpath, "%s", parentpath); + snprintf (path, pathsz, "%s", parentpath); - if (!diff_roots_appendsegment (path, maxdiffpath, + if (!diff_roots_appendsegment (path, pathsz, (const char *) &bsname [1], (size_t) bsname [0])) { report (state, diffkind_unreadable, parentpath, "-", "-", -1, -1, -1, "path too long to report"); @@ -1252,13 +1321,13 @@ static void reportonlyside (tywalkstate *state, const char *parentpath, (kind == diffkind_only_in_b) ? typename_for (val) : "-", -1, -1, -1, NULL); - reportsubtree (state, path, node, kind, depth); + reportsubtree (state, side, path, node, kind, depth); free (path); } /*reportonlyside*/ -static void reportsubtree (tywalkstate *state, const char *parentpath, +static void reportsubtree (tywalkstate *state, tyrootside *side, const char *parentpath, hdlhashnode node, tydiffkind kind, int depth) { tyvaluerecord *val; @@ -1279,8 +1348,21 @@ static void reportsubtree (tywalkstate *state, const char *parentpath, if ((hv == nil) || ((**hv).id != idtableprocessor)) return; - if (!(**hv).flinmemory) - return; /*not loaded on this side; the table itself was already reported*/ + if (!(**hv).flinmemory) { + + /* + Materialize rather than stop. Returning here would silently truncate + the report: the table itself was named, but none of the values beneath + it would be -- so a present-on-one-side-only subtree would look far + smaller than it is. That is the same blind spot the .ut exporter has + (main.c:2444), and a diff tool cannot afford it. + */ + if (!ensure_external_in_memory (&side->context, hv)) { + report (state, diffkind_unreadable, parentpath, "tabl", "tabl", -1, -1, -1, + "subtree could not be loaded to enumerate its contents"); + return; + } + } child = (hdlhashtable) (**hv).variabledata; @@ -1293,7 +1375,7 @@ static void reportsubtree (tywalkstate *state, const char *parentpath, gethashkey (nomad, bsname); - reportonlyside (state, parentpath, bsname, &(**nomad).val, kind, nomad, depth + 1); + reportonlyside (state, side, parentpath, bsname, &(**nomad).val, kind, nomad, depth + 1); } } /*reportsubtree*/ @@ -1363,7 +1445,7 @@ static void walkpair (tywalkstate *state, const char *path, order = comparenames (entriesa [ia].name, entriesb [ib].name); if (order < 0) { - reportonlyside (state, path, entriesa [ia].name, + reportonlyside (state, sidea, path, entriesa [ia].name, &(**(entriesa [ia].node)).val, diffkind_only_in_a, entriesa [ia].node, depth); ia++; @@ -1371,7 +1453,7 @@ static void walkpair (tywalkstate *state, const char *path, } if (order > 0) { - reportonlyside (state, path, entriesb [ib].name, + reportonlyside (state, sideb, path, entriesb [ib].name, &(**(entriesb [ib].node)).val, diffkind_only_in_b, entriesb [ib].node, depth); ib++; @@ -1380,7 +1462,8 @@ static void walkpair (tywalkstate *state, const char *path, { /*Heap, not stack -- see the note in reportonlyside.*/ - char *childpath = (char *) malloc (maxdiffpath); + size_t childsz = pathbufsize (path, (size_t) entriesa [ia].name [0]); + char *childpath = (char *) malloc (childsz); if (childpath == NULL) { free (entriesa); @@ -1390,9 +1473,9 @@ static void walkpair (tywalkstate *state, const char *path, return; } - snprintf (childpath, maxdiffpath, "%s", path); + snprintf (childpath, childsz, "%s", path); - if (!diff_roots_appendsegment (childpath, maxdiffpath, + if (!diff_roots_appendsegment (childpath, childsz, (const char *) &entriesa [ia].name [1], (size_t) entriesa [ia].name [0])) report (state, diffkind_unreadable, path, "-", "-", -1, -1, -1, @@ -1421,15 +1504,37 @@ static void closeside (tyrootside *side) { return; /* - Point the database globals back at this side before closing, so dbclose - tears down the right database when two are open. + Full teardown, in this order: + + databasedata = side -- dbclose/dbdispose operate on the global, so it + must name THIS side while two roots are open. + dbclose() -- flushes the header only (db.c:4823); it does NOT + free the record or close the file, so stopping + here leaks both. + dbdispose() -- frees the databaserecord and sets databasedata to + nil (db.c:4530-4542). + closefile() -- releases the file number itself. + + Ordering matters and is 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. + So the guard exit must happen AFTER this, never around it -- see + diff_roots_compare, which calls closeside for both sides before + odb_guard_exit. */ databasedata = side->database; dbclose (); + dbdispose (); /*frees the record and nils databasedata*/ + + if (side->fnum != 0) + closefile (side->fnum); + side->flopen = false; + side->fnum = 0; side->database = nil; + side->hrootvariable = nil; side->roottable = nil; } /*closeside*/ diff --git a/frontier-cli/diff_roots.h b/frontier-cli/diff_roots.h index 3328e98b9..79d95ce1c 100644 --- a/frontier-cli/diff_roots.h +++ b/frontier-cli/diff_roots.h @@ -17,9 +17,17 @@ `list` value HARD-ABORTS through try, #887), no hashgetvaluestring (deparses and would false-diff). Payloads are compared as stored bytes. - - BOTH ROOTS OPEN READ-ONLY. Neither file is written, and neither is - dirtied on purpose. dbwrite() itself refuses writes to a flreadonly - database (db.c:715-728), so this is enforced below us as well. + - BOTH ROOTS OPEN READ-ONLY. Neither file is written on disk, and the + walker sets no dirty flag itself. dbwrite() refuses writes to a + flreadonly database (db.c:715-728) and dbflushheader skips them + (db.c:855), so the on-disk guarantee is enforced below this layer too. + + Precisely: the walker DOES bring values into memory (subtables and + outlines must be resident to be compared) and materialization sets + residency flags. What it never does is set a dirty flag, write a block, + or let anything it calls do so -- disk-value reads deliberately stop at + dbrefhandle_context rather than using the kernel resolver, which would + mark the owning table dirty as a side effect (langhash.c:2211). - NAMES ARE BYTES. ODB names legitimately contain tabs and raw CR/LF -- Virgin.root has sibling values named with the single bytes 0x0A/0x0B/ From 405a34728d9d84338d9c6668af99a0d7da5739ff Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sun, 16 Aug 2026 00:44:00 -0700 Subject: [PATCH 09/10] fix(odb): make the ut-sync guard provable; define never-stored-both-sides 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 --- frontier-cli/diff_roots.c | 94 ++++++++++++++++++++++--------- frontier-cli/diff_roots.h | 14 +++++ tests/diff_roots_tests.c | 113 ++++++++++++++++++++++++++++++++------ 3 files changed, 179 insertions(+), 42 deletions(-) diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c index 5718c02f1..70abae984 100644 --- a/frontier-cli/diff_roots.c +++ b/frontier-cli/diff_roots.c @@ -887,11 +887,42 @@ static void compareexternal (tywalkstate *state, const char *path, Handle ha = nil; Handle hb = nil; + dbaddress adra = storedaddressof (hva); + dbaddress adrb = storedaddressof (hvb); boolean oka; boolean okb; - oka = readstoredbytes (sidea, storedaddressof (hva), &ha); - okb = readstoredbytes (sideb, storedaddressof (hvb), &hb); + /* + NEVER-STORED ON BOTH SIDES. + + With the old nil-means-empty-handle rule removed (it is what made + the mbar comparison vacuous), a value with no stored block on + EITHER side would otherwise be reported unreadable -- which would + break the self-diff law, since a root compared against itself would + produce findings. + + Two values that are both genuinely never-stored ARE equal: there is + no content on either side to differ. So report no difference, but + emit an auditable note naming the path, so the case can be reviewed + by a human without the exit code lying in either direction. The + note goes to stderr, keeping the findings stream on stdout clean + for diffing. + + The shipped roots contain no such values (every mbar and wptx + resolves through the variabledata fallback), but the tool has to be + correct in general, not just on today's inputs. + */ + if ((adra == nildbaddress) && (adrb == nildbaddress)) { + + if (!state->options->flquiet) + fprintf (stderr, "note: %s never stored on either side (%s); " + "treated as equal\n", path, typename_for (vala)); + + return; + } + + oka = readstoredbytes (sidea, adra, &ha); + okb = readstoredbytes (sideb, adrb, &hb); if (!oka || !okb) { @@ -902,8 +933,9 @@ static void compareexternal (tywalkstate *state, const char *path, disposehandle (hb); /* - Could not read one or both stored blocks. Report rather than - assume equality -- an unreadable value is a finding. + One side has a stored block the other lacks, or a read failed. + Either way this is a real finding, not an equality: reporting + it is what keeps a missing value from passing silently. */ report (state, diffkind_unreadable, path, typename_for (vala), typename_for (valb), -1, -1, -1, @@ -1631,6 +1663,38 @@ boolean diff_roots_compare (const char *patha, const char *pathb, if (ctfindings != NULL) *ctfindings = 0; + /* + MODE PRECONDITION -- checked FIRST, before any input validation, because it + is a statement about whether this mode may run at all, not about whether + its arguments are good. + + ut-sync's import hook rewrites outlines, sets dirty flags, and writes + .ut-sync-state: all writes, under a mode that promises neither root is + touched. cli_validate_options rejects the combination on the CLI path, but + this function is callable directly (the unit tests do exactly that), and + FRONTIER_UT_SYNC_DIR turns ut-sync on from the environment with no flag on + the command line at all. + + Checked via the env var rather than by calling back into the CLI layer: + this module must not depend on main.c, and the env var is precisely the + source that needs no argument to be present. + + Ordering it first also makes the refusal OBSERVABLE and therefore testable. + Behind the argument checks, every input a test can supply is rejected for + some other reason first, so a test could not tell this guard from a + file-not-found -- verified: with the guard stubbed out, such a test still + passed. First position plus the distinct message below means a test can + assert on this specific refusal. + */ + { + const char *utsyncenv = getenv ("FRONTIER_UT_SYNC_DIR"); + + if ((utsyncenv != NULL) && (utsyncenv [0] != '\0')) { + fprintf (err, "%s\n", diff_roots_utsyncrefusal); + return (false); + } + } + if ((patha == NULL) || (pathb == NULL)) return (false); @@ -1644,28 +1708,6 @@ boolean diff_roots_compare (const char *patha, const char *pathb, state.out = (options->out != NULL) ? options->out : stdout; state.ctfindings = 0; - /* - Structural guard, not merely a CLI check. ut-sync's import hook rewrites - outlines, sets dirty flags, and writes .ut-sync-state -- all writes, under a - mode that promises neither root is touched. cli_validate_options rejects the - combination on the CLI path, but this function is callable directly (the - unit tests do exactly that), and FRONTIER_UT_SYNC_DIR can turn ut-sync on - from the environment with no flag on the command line at all. - - The env var is checked here rather than calling back into the CLI layer: - this module must not depend on main.c, and the env var is the source that - can be set without anyone passing an argument. - */ - { - const char *utsyncenv = getenv ("FRONTIER_UT_SYNC_DIR"); - - if ((utsyncenv != NULL) && (utsyncenv [0] != '\0')) { - fprintf (err, "diff-roots: refusing to run with FRONTIER_UT_SYNC_DIR set; " - "ut-sync writes to roots this mode opens read-only\n"); - return (false); - } - } - if (!db_format_prepare_runtime ()) { fprintf (err, "diff-roots: database runtime failed to initialize\n"); return (false); diff --git a/frontier-cli/diff_roots.h b/frontier-cli/diff_roots.h index 79d95ce1c..490929296 100644 --- a/frontier-cli/diff_roots.h +++ b/frontier-cli/diff_roots.h @@ -59,6 +59,20 @@ #define maxdiffpath 8192 +/* + The exact text emitted when the walker refuses to run because ut-sync is + active. + + Exposed so a test can assert on THIS refusal specifically, rather than on + "the call returned false" -- which every bad input also produces, and which + therefore cannot distinguish a working guard from a missing one. Paired + with the guard being checked FIRST in diff_roots_compare, that makes the + refusal genuinely observable. +*/ +#define diff_roots_utsyncrefusal \ + "--diff-roots refuses to run with UT-sync active (FRONTIER_UT_SYNC_DIR is set)" + + /* Finding kinds, in the order the reporter prints their names. */ diff --git a/tests/diff_roots_tests.c b/tests/diff_roots_tests.c index 1716e0ecf..183c276de 100644 --- a/tests/diff_roots_tests.c +++ b/tests/diff_roots_tests.c @@ -33,6 +33,7 @@ #include #include #include /* setenv/unsetenv for the ut-sync guard tests */ +#include /* dup/dup2 for capturing the refusal message */ #include "test_report.h" @@ -225,38 +226,116 @@ static void test_compare_missing_file_is_operational_failure(void) { /* ---- ut-sync must never be active under a read-only walk ---- */ -static void test_compare_refuses_when_ut_sync_env_set(void) { +/* + * Run diff_roots_compare with stderr captured, reporting both the return value + * and whether the ut-sync refusal message specifically was emitted. + * + * Capturing the message is what makes these tests DISCRIMINATE. Asserting only + * "returned false" cannot tell a working guard from a missing one, because the + * inputs a unit test can supply are rejected for other reasons too -- verified + * by stubbing the guard, which left the earlier version of this test passing. + */ +static boolean run_compare_capturing_refusal(const char *patha, const char *pathb, + boolean *psawrefusal) { tydiffoptions options; long ctfindings = -1; + char captured[4096]; + boolean result; + int savedfd; + FILE *tmp; + size_t ct; memset(&options, 0, sizeof(options)); options.flquiet = true; + captured[0] = '\0'; + *psawrefusal = false; + + tmp = tmpfile(); + assert(tmp != NULL); + + fflush(stderr); + savedfd = dup(fileno(stderr)); + assert(savedfd >= 0); + assert(dup2(fileno(tmp), fileno(stderr)) >= 0); + + result = diff_roots_compare(patha, pathb, &options, &ctfindings); + + fflush(stderr); + assert(dup2(savedfd, fileno(stderr)) >= 0); + close(savedfd); + + rewind(tmp); + ct = fread(captured, 1, sizeof(captured) - 1, tmp); + captured[ct] = '\0'; + fclose(tmp); + + *psawrefusal = (strstr(captured, diff_roots_utsyncrefusal) != NULL); + + return (result); +} + +static void test_compare_refuses_when_ut_sync_env_set(void) { + boolean sawrefusal = false; + boolean result; + /* * ut-sync's import hook rewrites outlines, sets dirty flags, and writes * .ut-sync-state -- writes, under a mode whose contract is that neither * root is touched. The CLI rejects the combination, but this entry point is - * callable directly and FRONTIER_UT_SYNC_DIR can enable ut-sync with no - * flag on the command line, so the refusal also lives inside the walker. + * callable directly and FRONTIER_UT_SYNC_DIR enables ut-sync with no flag + * on the command line, so the refusal also lives inside the walker. * - * HONEST SCOPE OF THIS TEST: it asserts the call is refused, but it cannot - * prove the ut-sync guard specifically is what refused it. Every input a - * unit test can supply without shipping a multi-MB fixture (missing file, - * non-v7 file) is rejected by an earlier check anyway, so a mutation that - * removes the guard still leaves this passing -- verified, it did. - * - * The guard's real proof is at the acceptance layer, where the CLI is run - * with FRONTIER_UT_SYNC_DIR set against two genuine v7 roots that otherwise - * compare clean, and the run is rejected. This case is kept as a - * regression tripwire for the refusal contract (operational failure -> - * false, never "a diff with findings"), not as proof of the mechanism. + * The paths are deliberately BOGUS, which is what makes this test prove + * something. The guard is a mode precondition checked BEFORE any argument + * validation, so with the guard present the call is refused for ut-sync; + * with the guard removed the same call fails on the missing file and emits + * no refusal message, and the second assertion catches that. */ setenv("FRONTIER_UT_SYNC_DIR", "/tmp/diff-roots-must-refuse", 1); - assert(!diff_roots_compare("/nonexistent/a.root", "/nonexistent/b.root", - &options, &ctfindings)); + result = run_compare_capturing_refusal("/nonexistent/a.root", + "/nonexistent/b.root", &sawrefusal); unsetenv("FRONTIER_UT_SYNC_DIR"); + + assert(!result); /*operational failure, never "a diff with findings"*/ + assert(sawrefusal); /*and refused for THIS reason, not some other*/ +} + +static void test_compare_without_ut_sync_fails_for_other_reasons(void) { + boolean sawrefusal = true; + boolean result; + + /* + * The discriminating half: identical bogus paths, env var absent. Still an + * operational failure (missing file), but it must NOT be the ut-sync + * refusal -- otherwise the guard fires when it should not, and the test + * above would be passing for the wrong reason. + */ + unsetenv("FRONTIER_UT_SYNC_DIR"); + + result = run_compare_capturing_refusal("/nonexistent/a.root", + "/nonexistent/b.root", &sawrefusal); + + assert(!result); + assert(!sawrefusal); +} + +static void test_compare_ignores_empty_ut_sync_env(void) { + boolean sawrefusal = true; + boolean result; + + /* An empty value means the feature is off; it must not trip the guard. */ + setenv("FRONTIER_UT_SYNC_DIR", "", 1); + + result = run_compare_capturing_refusal("/nonexistent/a.root", + "/nonexistent/b.root", &sawrefusal); + + unsetenv("FRONTIER_UT_SYNC_DIR"); + + assert(!result); + assert(!sawrefusal); } /* ---- packed-header volatile regions: pinned to the real struct ---- */ @@ -373,6 +452,8 @@ int main(void) { TR_RUN(test_appendsegment_overflow_reports_and_preserves); TR_RUN(test_compare_missing_file_is_operational_failure); TR_RUN(test_compare_refuses_when_ut_sync_env_set); + TR_RUN(test_compare_without_ut_sync_fails_for_other_reasons); + TR_RUN(test_compare_ignores_empty_ut_sync_env); TR_RUN(test_packed_header_size_matches_format); TR_RUN(test_volatile_regions_are_the_proven_three); TR_RUN(test_volatile_regions_exclude_fltextmode); From 06ebb5963e5363c615ae6a961fa3c59ca638ada7 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sun, 16 Aug 2026 01:08:52 -0700 Subject: [PATCH 10/10] test(odb): commit behavioral fixtures; every compare path proves DETECTION 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 --- Common/source/oppack_v7.c | 32 ++- frontier-cli/diff_roots.c | 139 ++++++++---- frontier-cli/main.c | 13 +- tests/Makefile | 9 +- tests/diff_roots_fixtures_test.sh | 351 ++++++++++++++++++++++++++++++ 5 files changed, 488 insertions(+), 56 deletions(-) create mode 100755 tests/diff_roots_fixtures_test.sh diff --git a/Common/source/oppack_v7.c b/Common/source/oppack_v7.c index 82b357357..aea9e04f1 100644 --- a/Common/source/oppack_v7.c +++ b/Common/source/oppack_v7.c @@ -167,18 +167,26 @@ boolean op_packed_header_volatile_regions (typackedheaderregion *regions, long * rather than hardcoded so a layout change cannot silently desynchronize a caller. See op.h for why callers need these. - Only DEMONSTRATED volatility is listed: - - timecreated -- per-value creation stamp - timelastsave -- rewritten on every save - ctsaves -- incremented by oppack() itself, below, so it differs - even between two packs of one resident value - - outlinesignature is deliberately NOT listed. It is a caller-defined cookie - that is constant in practice (observed as 'LAND' throughout the shipped - roots), and excluding a field with no demonstrated volatility would let a - real difference pass silently -- the exact failure mode these exclusions - exist to avoid. + This is an ALLOWLIST: a field earns a place only by demonstrated + volatility, with the mechanism named. Adding to it on suspicion is how a + real difference gets silently swallowed. + + timecreated -- per-value creation stamp; differs between a built root + and its ancestor without the content differing + timelastsave -- rewritten by the act of saving + ctsaves -- INCREMENTED by oppack() itself (see below, where it is + bumped and written back into the outline record), so it + differs even between two packs of one resident value + + outlinesignature is deliberately NOT listed, and it is not a close call: + it is SEMANTIC CONTENT, not metadata. It selects the OSA server that will + run the script (scripts.c:943, getosaserver), drives tree building + (scripts.c:1302), and gates UserTalk-specific text handling + (scripts.c:3049, `outlinesignature == typeLAND`). A difference in this + field means a different LANGUAGE, which is exactly the kind of difference + the walker exists to report. That it reads as a constant 'LAND' throughout + the shipped roots is a fact about today's data, not a license to exclude + it. */ const typackedheaderregion volatileregions [] = { diff --git a/frontier-cli/diff_roots.c b/frontier-cli/diff_roots.c index 70abae984..bee073539 100644 --- a/frontier-cli/diff_roots.c +++ b/frontier-cli/diff_roots.c @@ -8,10 +8,13 @@ See diff_roots.h for the design commitments. Implementation notes that are not obvious from the header: - - Traversal is hfirstsort/sortedlink, which is the ODB's own stored - alphabetical order (lang.h:436,474). Walking two roots in that order lets - the comparison be a single sorted merge with no sorting pass, no - allocation, and deterministic output. + - Traversal buffers each table and sorts by raw name bytes before matching. + hfirstsort is NOT alphabetical 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), so nodes append in INSERTION order. Sorting is what makes matching + correct across roots with different insertion histories, and what makes + output deterministic. - Names are compared as RAW BYTES (memcmp over the Pascal string), never case-folded and never newline-normalized. Storage is case-preserving, and @@ -148,6 +151,39 @@ boolean diff_roots_appendsegment (char *path, size_t pathsz, const char *name, s } /*diff_roots_appendsegment*/ +/* + Emit an ODB path to a stream with high-bit bytes escaped. + + THE ONLY WAY a path reaches human-readable output. Every emission site must + route through here -- an earlier version escaped in the findings reporter + but printed the raw path in an audit note, which is the same class of + inconsistency in a smaller place. + + Escaping happens at the REPORT layer only. The shared name codec + (ut_pct_encode_segment) passes >= 0x80 through untouched, and that is + correct: it is a lossless bijection used for filesystem paths, and MacRoman + bytes must survive it byte-for-byte (#880). But a terminal renders those + bytes through whatever locale it happens to have, so raw emission is + ambiguous to read and unsafe to paste into a bug report. Findings handed to + a callback still carry the raw bytes. +*/ +static void emitpath (FILE *out, const char *path) { + + const unsigned char *p; + + if (path == NULL) + return; + + for (p = (const unsigned char *) path; *p != '\0'; p++) { + + if (*p >= 0x80) + fprintf (out, "%%%02X", (unsigned) *p); + else + fputc ((int) *p, out); + } + } /*emitpath*/ + + static void report (tywalkstate *state, tydiffkind kind, const char *path, const char *typea, const char *typeb, long sizea, long sizeb, long firstdiff, const char *detail) { @@ -173,29 +209,7 @@ static void report (tywalkstate *state, tydiffkind kind, const char *path, fprintf (state->out, "%-16s ", diff_roots_kindname (kind)); - /* - High-bit bytes are escaped HERE, at the report layer only. - - The shared name codec (ut_pct_encode_segment) passes >= 0x80 through - untouched, and that is correct -- it is a lossless bijection used for - filesystem paths, and MacRoman bytes must survive it byte-for-byte (#880). - But a terminal will render those bytes through whatever locale it happens - to have, so a REPORT that emits them raw is ambiguous to read and unsafe to - paste into a bug. Escaping at the point of display keeps the stored path - lossless everywhere else -- the finding handed to a callback still carries - the raw bytes. - */ - { - const unsigned char *p; - - for (p = (const unsigned char *) path; *p != '\0'; p++) { - - if (*p >= 0x80) - fprintf (state->out, "%%%02X", (unsigned) *p); - else - fputc ((int) *p, state->out); - } - } + emitpath (state->out, path); if ((kind == diffkind_type) || (kind == diffkind_unsupported)) fprintf (state->out, " A=%s B=%s", finding.typea, finding.typeb); @@ -407,6 +421,13 @@ static boolean resolvediskvalue (tyrootside *side, const tyvaluerecord *val, Han Returns nildbaddress only when the value genuinely has no stored block; the caller must treat that as reportable, never as equality. + + KNOWN-LOUD: the returned address is used as-is, with no normalization + (dbrefhandle_context's underlying read does none). If a stored address were + somehow interior or stale, the read fails and the caller emits an + `unreadable` finding naming the path -- a visible, attributable failure + rather than a silent wrong answer. That is the intended behavior, not an + unhandled case. */ static dbaddress storedaddressof (hdlexternalvariable hv) { @@ -554,11 +575,16 @@ static boolean packoutlinebytes (hdlexternalvariable hv, Handle *hpacked) { The regions are NOT hardcoded here. They come from op_packed_header_volatile_regions() (op.h / oppack_v7.c), derived with offsetof()/sizeof() on the real struct. That indirection exists because the - first version of this walker DID hardcode them and got them wrong by +6: a - local probe of the struct written WITHOUT #pragma pack(2) reported - 24/32/40/48 with a 1080-byte header, while the real packed layout is - 18/26/34 with 1068 bytes. The _Static_assert at oppack_v7.c:154 pins 1068 - and would have caught it. Two silent failures resulted -- timecreated + first version of this walker DID hardcode them and got them wrong by +6. + + The cause was THIS UNIT'S OWN throwaway offset probe (a scratch program + written while designing the walker, never committed): it declared a copy of + typortablediskheader WITHOUT #pragma pack(2), so it reported 24/32/40/48 + with a 1080-byte header, while the real packed layout is 18/26/34 with 1068 + bytes. The _Static_assert at oppack_v7.c:154 pins 1068 and would have caught + it immediately had the probe's own output been checked against it -- + measuring instruments need their own invariant checks, the same lesson this + walker exists to enforce one level up. Two silent failures resulted -- timecreated (18-23) was left UNSKIPPED, producing false diffs whenever creation stamps differ, and fltextmode (38-39) was WRONGLY skipped, letting real content compare equal. Deriving from the struct makes that class of error @@ -620,13 +646,19 @@ static boolean g_headerregionsready = false; /* Load the volatile-region table once, from oppack_v7.c via offsetof/sizeof on the real struct. Hardcoding these offsets is how the first version of this - walker got them wrong by +6: a local probe of the struct WITHOUT - #pragma pack(2) reported 24/32/40/48 and a 1080-byte header, when the real - packed layout is 18/26/34 and 1068 bytes (the _Static_assert at - oppack_v7.c:154 is the invariant that would have caught it). + walker got them wrong by +6: this unit's own scratch offset probe declared + the struct WITHOUT #pragma pack(2) and so reported 24/32/40/48 with a + 1080-byte header, when the real packed layout is 18/26/34 with 1068 bytes + (the _Static_assert at oppack_v7.c:154 is the invariant that would have + caught it). Returns false if the table cannot be obtained, in which case the caller compares the whole buffer verbatim -- over-reporting, never under-reporting. + + The static cache is not thread-safe. That is fine here and nowhere else: + this mode runs in main.c's early-exit band, before any runtime or thread + registry exists, single-threaded by construction. Anything reusing this in + a threaded context must not reuse the cache. */ static boolean loadheaderregions (void) { @@ -914,9 +946,12 @@ static void compareexternal (tywalkstate *state, const char *path, */ if ((adra == nildbaddress) && (adrb == nildbaddress)) { - if (!state->options->flquiet) - fprintf (stderr, "note: %s never stored on either side (%s); " - "treated as equal\n", path, typename_for (vala)); + if (!state->options->flquiet) { + fprintf (stderr, "note: "); + emitpath (stderr, path); /*escaped, like every other path emission*/ + fprintf (stderr, " never stored on either side (%s); treated as equal\n", + typename_for (vala)); + } return; } @@ -1268,6 +1303,12 @@ static int comparenodeentries (const void *a, const void *b) { Returns false only on allocation failure. An empty table yields ct 0 and a NULL array, which the caller handles as "nothing on this side". + + Assumes names within one table are unique, which the ODB enforces (a + hashtable cannot hold two entries under the same key). If duplicates could + occur, qsort's unspecified ordering among equal keys would make the pairing + of A-side to B-side entries arbitrary; the merge below would still pair + them one-for-one, but which-with-which would not be meaningful. */ static boolean collectnodes (hdlhashtable htable, tynodeentry **pentries, long *pct) { @@ -1714,9 +1755,23 @@ boolean diff_roots_compare (const char *patha, const char *pathb, } /* - Opening a second database clears the table-structure globals, which would - destroy the first root's state. The guard saves and restores all of them -- - this is the documented hazard at db.h:285-289. + DEFENSE IN DEPTH, not a load-bearing fix for a demonstrated corruption. + + An earlier version of this comment claimed that opening a second database + clears the table-structure globals. An audit disproved that: dbopenfile + touches only databasedata. cleartablestructureglobals is called from + cancoon.c:601 and the compaction/migration path in db_format.c, neither of + which this mode enters. The hazard db.h:285-289 documents is real for the + guest-DB open path; it is simply not reached from here. + + The guard is kept anyway because this mode holds two roots open at once and + sets databasedata directly, so restoring the caller's context on every exit + path costs nothing and removes a whole class of future coupling. Keeping it + for an honest reason is better than keeping it for an invented one. + + Note it does NOT save the db_format mode globals (g_mode_state). That is + inert here: the walker is v7-only and never switches modes, and closeside's + dbdispose pops the mode pushed by that side's dbopenfile. */ odb_guard_enter (&guard); diff --git a/frontier-cli/main.c b/frontier-cli/main.c index e4c474dd5..fc2de0b39 100644 --- a/frontier-cli/main.c +++ b/frontier-cli/main.c @@ -846,10 +846,21 @@ int main(int argc, char* argv[]) { // Parse command line arguments if (!cli_parse_arguments(argc, argv, &g_cli_options)) { + /* + * --diff-roots exits 2 for ANY operational failure, including one + * detected during argument validation (an unreadable root is rejected + * there, not by the walker). Without this, a missing or unreadable + * database would exit 1 -- indistinguishable from "differences found", + * so a broken CI drift check would read as a real drift report. The + * mode is read before cli_free_options frees it. + */ + boolean diff_roots_mode = (g_cli_options.diff_roots_a != NULL) + || (g_cli_options.diff_roots_b != NULL); + log_error(LOG_COMP_GENERAL, "Error: Invalid command line arguments"); print_usage(argv[0]); cli_free_options(&g_cli_options); /* honor parse-failure cleanup contract */ - return 1; + return diff_roots_mode ? 2 : 1; } // Apply --log spec if provided (overrides env var settings) diff --git a/tests/Makefile b/tests/Makefile index 475f08f75..973d85004 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -911,6 +911,14 @@ test-openurl-guard: @echo "Running headless sys.openUrl guard tests..." @./openurl_headless_guard_test.sh +# Behavioral fixtures for --diff-roots (root-build Step 1). Every compare path is +# proven by a root that DIFFERS in one known way -- the gate found two P0s that +# survived agreement-only evidence. Fixtures are generated at run time from +# databases/StartupTasks.root; no binary fixtures are committed. +test-diff-roots-fixtures: + @echo "Running diff-roots behavioral fixture tests..." + @./diff_roots_fixtures_test.sh + # Run REPL selection flag tests (shell-based, uses -e mode + --help) # 2026-06-25 JES C.6 #691: covers --plain / --debug-tui / --protocol # mutual exclusion, the --debug-tui deprecation warning, and the new @@ -1055,4 +1063,3 @@ verify-errors: check_v6_tables: check_v6_tables.c $(LANG_RUNTIME_SOURCES) ../Common/source/odbengine.c ../frontier-cli/stubs/headless_shell.c $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -DHEADLESS_TEST_PORTABLE_FILE -DHEADLESS_LINKS_REAL_DB -I../Common/headers -I../Common/SystemHeaders -I../portable check_v6_tables.c \ $(LANG_RUNTIME_SOURCES) ../Common/source/odbengine.c ../frontier-cli/stubs/headless_shell.c -o $@ $(LINK_LIBS) - diff --git a/tests/diff_roots_fixtures_test.sh b/tests/diff_roots_fixtures_test.sh new file mode 100755 index 000000000..ef5b3fbf7 --- /dev/null +++ b/tests/diff_roots_fixtures_test.sh @@ -0,0 +1,351 @@ +#!/bin/bash +# +# Behavioral fixture tests for --diff-roots (root-build Step 1). +# +# WHY THIS FILE EXISTS +# +# The gate found two P0s in the compare core, and BOTH survived the original +# acceptance evidence, because that evidence only ever proved the walker +# AGREES -- never that it DETECTS. Self-diff compares a file with itself, so no +# vacuous compare and no wrong skip window can surface. "43 menubars compared +# equal" measured agreement. +# +# So every compare path here is proven by a fixture that DIFFERS in exactly one +# known way, and the walker must find exactly that. Three of the fix commits are +# otherwise revert-invisible: revert storedaddressof, collectnodes, or the +# reportsubtree descent and a self-diff-only suite stays green. +# +# NO BINARY FIXTURES ARE COMMITTED. Every root is generated at run time from +# databases/StartupTasks.root (29 KB, v7, already in the repo) by copying it and +# flipping known bytes. Deterministic: same inputs, same offsets, same results. +# +# The byte offsets below are derived, not guessed. The live packed-outline +# header for StartupTasksData...Frontier.cliVersion starts at file offset 0x66; +# that was established by mutating its script text and reading back the walker's +# own reported first-diff-offset. Header field offsets come from +# typortablediskheader (oppack_v7.c): ctsaves 34-37, fltextmode 38-39, +# outlinesignature 40-43. +# +# License +# ------- +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 Frontier contributors. + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +CLI="$PROJECT_ROOT/frontier-cli/frontier-cli" +SOURCE_DB="$PROJECT_ROOT/databases/StartupTasks.root" + +if [ ! -x "$CLI" ]; then + echo "Error: frontier-cli not found at $CLI" >&2 + exit 1 +fi +if [ ! -f "$SOURCE_DB" ]; then + echo "Error: database not found at $SOURCE_DB" >&2 + exit 1 +fi + +RED='\033[0;31m' +GREEN='\033[0;32m' +NC='\033[0m' + +PASSED=0 +FAILED=0 + +WORKDIR=$(mktemp -d "${TMPDIR:-/tmp}/diff_roots_fixtures.XXXXXX") +cleanup() { rm -rf "$WORKDIR"; } +trap cleanup EXIT + +BASE="$WORKDIR/base.root" +cp "$SOURCE_DB" "$BASE" || { echo "Error: could not stage the base root" >&2; exit 1; } + +# Live packed-outline header for the cliVersion script inside the base root. +HDR=$((0x66)) + +pass() { + echo -e " ${GREEN}PASS${NC}: $1" + PASSED=$((PASSED + 1)) +} + +fail() { + echo -e " ${RED}FAIL${NC}: $1" + echo " $2" + FAILED=$((FAILED + 1)) +} + +# flipbyte -- copy the base root and XOR one byte. +flipbyte() { + local dest="$1" off="$2" + cp "$BASE" "$dest" + python3 - "$dest" "$off" <<'PYEOF' +import sys +path, off = sys.argv[1], int(sys.argv[2]) +d = bytearray(open(path, 'rb').read()) +d[off] ^= 0x01 +open(path, 'wb').write(d) +PYEOF +} + +# findings -- number of findings reported. +findings() { + "$CLI" --diff-roots "$1" --against "$2" 2>/dev/null \ + | grep -cE '^(payload|only-in-a|only-in-b|type|unsupported-type|unreadable)' +} + +# exitcode +exitcode() { + "$CLI" --diff-roots "$1" --against "$2" >/dev/null 2>&1 + echo $? +} + +echo "diff_roots fixture tests" +echo "========================" + +# ---- identical -> empty ------------------------------------------------------ + +cp "$BASE" "$WORKDIR/same.root" +n=$(findings "$BASE" "$WORKDIR/same.root") +e=$(exitcode "$BASE" "$WORKDIR/same.root") +if [ "$n" -eq 0 ] && [ "$e" -eq 0 ]; then + pass "identical roots -> 0 findings, exit 0" +else + fail "identical roots -> 0 findings, exit 0" "got $n findings, exit $e" +fi + +# ---- script payload difference -> exactly one ------------------------------- +# Mutates one byte of script TEXT (inside the comment on the first line of +# cliVersion), which is content by any definition. + +cp "$BASE" "$WORKDIR/script.root" +python3 - "$WORKDIR/script.root" <<'PYEOF' +import sys +path = sys.argv[1] +d = bytearray(open(path, 'rb').read()) +i = d.find(b'return the version string') +assert i > 0, 'script-text anchor not found in StartupTasks.root' +d[i] = ord('R') +open(path, 'wb').write(d) +PYEOF +n=$(findings "$BASE" "$WORKDIR/script.root") +e=$(exitcode "$BASE" "$WORKDIR/script.root") +if [ "$n" -eq 1 ] && [ "$e" -eq 1 ]; then + pass "one script-text byte changed -> exactly 1 finding, exit 1" +else + fail "one script-text byte changed -> exactly 1 finding, exit 1" "got $n findings, exit $e" +fi + +# ---- skip regions: volatile save metadata is IGNORED ------------------------- +# ctsaves (header 34-37) is bumped by oppack itself. If it were compared, every +# value in a re-saved root would report as different and the acceptance gate +# would be useless. + +flipbyte "$WORKDIR/ctsaves.root" $((HDR + 37)) +n=$(findings "$BASE" "$WORKDIR/ctsaves.root") +if [ "$n" -eq 0 ]; then + pass "ctsaves byte changed (skip region) -> 0 findings" +else + fail "ctsaves byte changed (skip region) -> 0 findings" "got $n findings" +fi + +# ---- fltextmode is CONTENT and must NOT be skipped --------------------------- +# This is the exact byte the original off-by-six skip table swallowed. Offset 39 +# is the low byte of the big-endian int16; the high byte at 38 is always zero and +# is discarded by the pack round-trip, so 39 is the one that carries the value. + +flipbyte "$WORKDIR/fltextmode.root" $((HDR + 39)) +n=$(findings "$BASE" "$WORKDIR/fltextmode.root") +if [ "$n" -eq 1 ]; then + pass "fltextmode changed -> 1 finding (old +6 table swallowed this)" +else + fail "fltextmode changed -> 1 finding" "got $n findings" +fi + +# ---- outlinesignature is CONTENT and must NOT be skipped --------------------- +# It selects the OSA server that runs the script (scripts.c:943), drives tree +# building (:1302), and gates UserTalk text handling (:3049, +# outlinesignature == typeLAND). A difference here means a different LANGUAGE. + +flipbyte "$WORKDIR/signature.root" $((HDR + 43)) +n=$(findings "$BASE" "$WORKDIR/signature.root") +if [ "$n" -eq 1 ]; then + pass "outlinesignature changed -> 1 finding (semantic, not metadata)" +else + fail "outlinesignature changed -> 1 finding" "got $n findings" +fi + +# ---- skip-window boundary ---------------------------------------------------- +# Pins both edges at once: inside the window is silent, one byte past it is +# reported. An off-by-N in either direction breaks exactly one of these. + +flipbyte "$WORKDIR/edge_in.root" $((HDR + 34)) +flipbyte "$WORKDIR/edge_out.root" $((HDR + 39)) +nin=$(findings "$BASE" "$WORKDIR/edge_in.root") +nout=$(findings "$BASE" "$WORKDIR/edge_out.root") +if [ "$nin" -eq 0 ] && [ "$nout" -eq 1 ]; then + pass "skip-window boundary: 34 ignored, 39 reported" +else + fail "skip-window boundary: 34 ignored, 39 reported" "got in=$nin out=$nout" +fi + +# ---- operational failure is distinct from findings -------------------------- +# Exit 2 (cannot run) must never look like exit 0 (clean) or exit 1 (differences), +# or a broken CI check reads as a passing one. + +e=$(exitcode "$WORKDIR/does-not-exist.root" "$BASE") +if [ "$e" -eq 2 ]; then + pass "missing input -> exit 2, distinct from 0 and 1" +else + fail "missing input -> exit 2" "got exit $e" +fi + +# ---- ut-sync refusal --------------------------------------------------------- +# ut-sync writes to the roots this mode opens read-only. Same command, same +# roots, env var the only variable. + +FRONTIER_UT_SYNC_DIR="$WORKDIR/utsync" "$CLI" --diff-roots "$BASE" --against "$WORKDIR/same.root" >/dev/null 2>&1 +e=$? +if [ "$e" -ne 0 ]; then + pass "ut-sync active -> refused (exit $e)" +else + fail "ut-sync active -> refused" "got exit 0; the guard did not fire" +fi + +# ---- mbar detection (the P0-1 fixture) --------------------------------------- +# StartupTasks.root has no menubar, so this one needs Virgin.root. It is the +# fixture that proves the vacuous-mbar bug stays fixed: before the fix, EVERY +# menubar compared equal regardless of content, because oldaddress is nil for +# never-materialized externals and the stored address lives in variabledata. +# +# Uses suites.applescripts.oldstuff.menu, whose stored block starts at 0x14a97 +# in the committed Virgin.root. If Virgin.root changes, this offset must be +# re-derived (run the walker against a byte-flipped copy and read the path it +# names); a stale offset shows up as "did not detect", never as a false pass. + +VIRGIN="$PROJECT_ROOT/databases/Virgin.root" +MBAR_BLOCK=$((0x14a97)) +MBAR_INTERIOR=$((MBAR_BLOCK + 64)) + +if [ -f "$VIRGIN" ]; then + VA="$WORKDIR/virgin-a.root" + VB="$WORKDIR/virgin-b.root" + cp "$VIRGIN" "$VA" + cp "$VIRGIN" "$VB" + + python3 - "$VB" "$MBAR_INTERIOR" <<'PYEOF' +import sys +path, off = sys.argv[1], int(sys.argv[2]) +d = bytearray(open(path, 'rb').read()) +d[off] ^= 0x01 +open(path, 'wb').write(d) +PYEOF + + n=$("$CLI" --diff-roots "$VA" --against "$VB" 2>/dev/null | grep -c 'type=mbar') + if [ "$n" -eq 1 ]; then + pass "menubar content changed -> 1 mbar finding (vacuous-compare regression)" + else + fail "menubar content changed -> 1 mbar finding" \ + "got $n mbar findings; if Virgin.root moved, re-derive MBAR_BLOCK" + fi + + # Same roots unmutated must still be clean, so the above cannot pass by + # reporting menubars indiscriminately. + cp "$VIRGIN" "$VB" + n=$("$CLI" --diff-roots "$VA" --against "$VB" 2>/dev/null | grep -c 'type=mbar') + if [ "$n" -eq 0 ]; then + pass "unmutated menubars -> 0 mbar findings" + else + fail "unmutated menubars -> 0 mbar findings" "got $n" + fi +else + echo " SKIP: Virgin.root not present; mbar detection fixture not run" +fi + +# ---- divergent insertion order and subtree enumeration (P1-3 / P2) ---------- +# These need a root that can be opened READ-WRITE as a system root so values can +# be scripted in; StartupTasks.root cannot. Virgin.root can, so they ride along +# with the Virgin block above. +# +# Order fixture: the SAME three values inserted in two different orders. Before +# the name-keyed fix, this produced four findings for three values -- each +# reported as both only-in-A and only-in-B -- and their payloads were never +# compared at all. The masked comparison was the serious half. +# +# Subtree fixture: a nested table present on one side only. Before the fix, +# reportsubtree stopped at any non-resident subtable, so the table was named but +# its contents were not. + +if [ -f "$VIRGIN" ]; then + OA="$WORKDIR/order-a.root" + OB="$WORKDIR/order-b.root" + cp "$VIRGIN" "$OA" + cp "$VIRGIN" "$OB" + + cat > "$WORKDIR/order-a.ut" <<'UTEOF' +new(tabletype, @workspace.orderProbe); +workspace.orderProbe.zebra = "z"; +workspace.orderProbe.alpha = "a"; +workspace.orderProbe.mango = "m"; +"ok" +UTEOF + cat > "$WORKDIR/order-b.ut" <<'UTEOF' +new(tabletype, @workspace.orderProbe); +workspace.orderProbe.alpha = "a"; +workspace.orderProbe.mango = "m"; +workspace.orderProbe.zebra = "z"; +"ok" +UTEOF + + "$CLI" --system-root "$OA" --skip-startup -b "$WORKDIR/order-a.ut" >/dev/null 2>&1 + "$CLI" --system-root "$OB" --skip-startup -b "$WORKDIR/order-b.ut" >/dev/null 2>&1 + + n=$("$CLI" --diff-roots "$OA" --against "$OB" 2>/dev/null | grep -ci 'orderProbe') + if [ "$n" -eq 0 ]; then + pass "same values, different insertion order -> 0 findings" + else + fail "same values, different insertion order -> 0 findings" \ + "got $n (insertion-order matching regressed; payloads go uncompared)" + fi + + SA="$WORKDIR/subtree-a.root" + SB="$WORKDIR/subtree-b.root" + cp "$VIRGIN" "$SA" + cp "$VIRGIN" "$SB" + + cat > "$WORKDIR/subtree.ut" <<'UTEOF' +new(tabletype, @workspace.subtreeProbe); +new(tabletype, @workspace.subtreeProbe.inner); +workspace.subtreeProbe.inner.leaf1 = "one"; +workspace.subtreeProbe.inner.leaf2 = "two"; +workspace.subtreeProbe.topLevel = "top"; +"ok" +UTEOF + "$CLI" --system-root "$SB" --skip-startup -b "$WORKDIR/subtree.ut" >/dev/null 2>&1 + + n=$("$CLI" --diff-roots "$SA" --against "$SB" 2>/dev/null | grep -ci 'subtreeProbe') + if [ "$n" -eq 5 ]; then + pass "one-sided subtree -> all 5 nested values enumerated" + else + fail "one-sided subtree -> all 5 nested values enumerated" \ + "got $n (expected table, inner table, 2 leaves, 1 scalar)" + fi +fi + +# ---- read-only guarantee ----------------------------------------------------- +# The whole tool is worthless if it can perturb its inputs. + +before=$(md5 -q "$BASE" 2>/dev/null || md5sum "$BASE" | cut -d' ' -f1) +"$CLI" --diff-roots "$BASE" "$WORKDIR/script.root" >/dev/null 2>&1 +"$CLI" --diff-roots "$BASE" --against "$WORKDIR/script.root" >/dev/null 2>&1 +after=$(md5 -q "$BASE" 2>/dev/null || md5sum "$BASE" | cut -d' ' -f1) +if [ "$before" = "$after" ]; then + pass "inputs byte-identical after comparison (read-only)" +else + fail "inputs byte-identical after comparison" "md5 changed: $before -> $after" +fi + +echo +echo " Passed: $PASSED" +echo " Failed: $FAILED" + +[ "$FAILED" -eq 0 ] || exit 1 +exit 0