Skip to content

fix(graph): tie-breaks leaked os.walk order into node paths and wikilinks - #43

Merged
sturlese merged 3 commits into
mainfrom
fix/bughunt-graph-tiebreak-determinism
Jul 25, 2026
Merged

fix(graph): tie-breaks leaked os.walk order into node paths and wikilinks#43
sturlese merged 3 commits into
mainfrom
fix/bughunt-graph-tiebreak-determinism

Conversation

@sturlese

@sturlese sturlese commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Tenth iteration of the autonomous bughunt sweep. A determinism claim the code did not keep.

Bug

docs/pipeline/graph.md promises "Output is deterministic for a given input" — and the input is the pages' content, not the order the filesystem happened to yield them in. Three tie-breaks read a Counter, which resolves ties by first-encounter, and build_graph fills those Counters straight from os.walk:

ms = [("Initech","company",1), ("Initech","organization",1)]
as walked -> type=company       slug=entities/company/initech
reversed  -> type=organization  slug=entities/organization/initech

That value is the node's file pathentities/<type>/<slug> — and therefore every [[wikilink]] pointing at it. build_graph's stale-removal then deletes the other one. So adding an unrelated page to brain-md could relocate an existing entity and rewrite links across the corpus.

A type tie is not exotic: two documents mentioning the same company, two LLM guesses at its type.

Two more of the same shape:

  • Title tied identically — equal case-ness, equal length, equal count fell through to walk order (Acme Corp vs Acme GmbH).
  • Alias order decided which aliases survive render_node's aliases[:8].

Fix

All three now state a rule rather than relying on stability:

choice rule
entity type most-mentioned, then alphabetical
aliases most-mentioned, then alphabetical
title non-ALL-CAPS → shortest → most-mentioned → alphabetical

Ordering aliases by frequency has a second benefit: the [:8] truncation now keeps the aliases that actually appear in the corpus rather than the ones the walk reached first.

docs/pipeline/graph.md states the rules next to the determinism claim it was making — and, after the gate caught me over-claiming, also names the one thing that is not guaranteed: slugs are assigned in key order, so adding an entity whose key sorts earlier can still relocate an existing node (Foo & Bar and Foo + Bar both slugify to foo-bar). Links are rewritten consistently, so nothing dangles. I had checked that loop and found it stable under reordering — stability under adding a key is a different property, and I asserted the stronger one without testing it.

Migration

A deployed corpus that has a genuine type tie will relocate that node on its next build. Verified end to end: the node moves, the old one is removed, and every wikilink is rewritten consistently — no orphan and nothing dangling. One cosmetic residue: build.py uses os.remove and never rmdir, so an empty entities/<old-type>/ directory is left behind.

Eval risk — checked, not assumed

golden.json pins graph output, so I verified the change cannot move it rather than reasoning about it:

mention names with a TYPE tie in the golden corpus: none
distinct types seen: ['organization']

No ties and a single type, so the pinned entities/organization/globex.md is unreachable by this change. placement: pages at expected paths 8/8, placement: frontmatter contract 30/30, the graph metric and the full 25/25 scorecard all unchanged.

Test

Both new tests confirmed to fail with the source change stashed.

  • test_output_is_independent_of_mention_order — the property itself: build_entities(ms) == build_entities(reversed(ms)) for a type tie, a title tie, and an alias set.

  • test_tie_breaks_are_alphabetical_not_incidental — that the tie-break is a stated rule, not merely stable, so a future change cannot make it deterministic-but-arbitrary.

  • test_build_graph_output_is_byte_identical_under_any_walk_order — the claim asserted on the whole output tree (node pages, rewritten docs, wikilinks), not just at build_entities.

  • test_title_prefers_non_all_caps_over_shortest — pins the ordered title rule this PR documents.

Three tests were passing for the wrong reason, mine included. test_dominant_type_wins picked a case where alphabetical and frequency agree, so an alphabetical-only rule passed it. My own end-to-end determinism test initially caught only one of the three reverts — the title tie needs its spellings in separate files (within one file the frontmatter fixes the order), and the alias order needs three-plus spellings because render_node excludes the title, leaving only one alias with two names. And the title rule's top two keys were unpinned entirely: reordering isupper and len passed all 50 tests, so this PR documented a rule nothing checked. All three now discriminate; the reviewer's independent mutation set is fully killed.

graph 51 (+5) · clean 256 · corpus 48 · slack 13 · fetch 33 · answer 66 · benchmark 3 — bare pytest (CI mode). ruff clean; golden scorecard 25/25.

sturlese and others added 3 commits July 25, 2026 07:19
…inks

docs/pipeline/graph.md promises "Output is deterministic for a given input", and the
input is the pages' CONTENT — not the order the filesystem happened to yield them in.
Three tie-breaks read a Counter, which resolves ties by first-encounter, and
build_graph fills those Counters straight from os.walk:

    ms = [("Initech","company",1), ("Initech","organization",1)]
    as walked -> type=company       slug=entities/company/initech
    reversed  -> type=organization  slug=entities/organization/initech

That value is the node's FILE PATH and therefore every wikilink pointing at it, and
build_graph's stale-removal deletes the other one — so adding an unrelated page to
brain-md could relocate an existing entity and rewrite links across the corpus. A
type tie is not exotic: two documents, two LLM guesses at the same company.

The title tied the same way (equal case-ness, equal length, equal count fell through
to walk order), and alias order decided WHICH aliases survive render_node's
`aliases[:8]`.

All three now state a rule: type and aliases by most-mentioned then alphabetical,
title by non-ALL-CAPS -> shortest -> most-mentioned -> alphabetical. Ordering aliases
by frequency also means the truncation keeps the ones that actually appear in the
corpus rather than the ones the walk reached first.

Verified the golden output cannot move, rather than assuming it: the demo corpus has
NO type ties and exactly one distinct mention type, so the pinned
`entities/organization/globex.md` is unreachable by this change. placement 8/8,
frontmatter 30/30, graph metric and the full 25/25 scorecard unchanged.

Mutation-tested, all five caught. One is worth recording: `min(g["types"])` —
alphabetical while IGNORING frequency — initially SURVIVED, because the existing
test_dominant_type_wins picks a case where alphabetical and frequency agree
(company beats organization both ways). It now also pins the discriminating case,
where the dominant type sorts later.

graph 49 (+3). scorecard 25/25, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ild_entities

The claim is about build_graph's OUTPUT — node pages, rewritten docs, every wikilink —
so it is asserted there: run the real build twice with os.walk yielding entries in
opposite orders and compare the trees byte for byte.

Getting it to actually discriminate took three corrections, each of which says
something about where the order-dependence can and cannot show up:

- a type tie is enough on its own (it decides the node's file path);
- the title tie needs its two spellings in SEPARATE files — within one file the mention
  order is fixed by the frontmatter, so only cross-file order can flip it;
- the alias order needs THREE or more spellings of one entity: render_node excludes the
  title from the alias list, so with two names only one alias remains and its order
  cannot reach the output at all.

Verified each of the three reverts now fails this test on its own; before the last two
corrections, two of them passed it.

Also checked and not changed: slug-collision assignment (`base-2`) is already stable,
because build_entities iterates `for key in sorted(groups)` rather than walk order —
verified under both orders. And `min(g["types"], ...)` cannot see an empty Counter: a
group only exists once a mention has been counted into it.

graph 50 (+1).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…as unpinned

Gate review found two problems with what this PR SAYS rather than what it does.

The sentence I added — "adding an unrelated page never relocates an existing entity" —
is false, and the reviewer reproduced it on the branch:

    before (only 'Foo + Bar')          : entities/company/foo-bar
    after  (added 'Foo & Bar' pages)   : foo-bar -> 'foo & bar'
                                         'foo + bar' MOVED to foo-bar-2

Two different keys can slugify identically, slugs are assigned in key order, and '&'
(0x26) sorts before '+' (0x2b) — so a genuinely unrelated entity took the bare slug and
the pre-existing one's wikilinks were rewritten corpus-wide. That is the same harm this
PR describes as the bug, through the one order-sensitive path in the same function I had
checked and (correctly) found stable under REORDERING — but it is not stable under
ADDING a key, which is a different property than the one I verified. The doc now states
what is actually guaranteed and names the caveat.

Also corrected: "alphabetical" is codepoint order, which for equal-length names
systematically prefers the more-ASCII spelling ('IPhone 15' over 'iPhone 15', 'Inigo
Ruiz' over 'Íñigo Ruiz'). Deterministic-but-sometimes-ugly is the honest description;
any total order has to pick something, but the doc should not imply locale collation.

And the title rule this PR promotes to documented public contract had its top two keys
unpinned: reordering `isupper` and `len` passed all 50 tests. That is precisely the
"test passing for the wrong reason" defect I identified for the type rule in this same
PR — `test_dedup_variants_into_one_entity` cannot pin the order because there the
non-caps name is also the shortest. `_best_title({"IBM": 5, "Ibm Global": 1})` is the
case that discriminates: IBM is shorter AND more frequent, and must still lose. Both
reorderings now fail.

graph 51 (+1). scorecard 25/25, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sturlese

Copy link
Copy Markdown
Owner Author

Gate verdict

VERDICT: WEAKENED. The fix itself held up under heavy attack; what did not hold up were two things this PR says. Both corrected in 7517f68.

The claim, verified hard

The reviewer ran the real build_graph with os.walk monkeypatched across forward, reverse and 6 seeded shuffles, diffing the entire output tree byte for byte, then fuzzed 40 randomised corpora × 7 walk orders including CJK/accented names, None types, registries and colliding slugs:

BRANCH: 40 random corpora x 7 walk orders: 0 order-dependent differences
MAIN:   40 random corpora x 7 walk orders: 185 order-dependent differences

With a paired control on main showing real relocations (entities/product/widget-systems.mdentities/software/…), so the harness demonstrably sees the bug. Golden graph output byte-identical between main and branch; scorecard 25/25. min never disagrees with sorted(...)[0], and an empty types Counter is unreachable because g["types"][typ or "other"] += count always creates a key.

The doc sentence I added was false

"adding an unrelated page never relocates an existing entity"

Reproduced on the branch:

before (only 'Foo + Bar')        : entities/company/foo-bar
after  (added 'Foo & Bar' pages) : foo-bar -> 'foo & bar'
                                   'foo + bar' MOVED to foo-bar-2

Two different keys can slugify identically, slugs are assigned in key order, and & (0x26) sorts before + (0x2b) — so a genuinely unrelated entity took the bare slug and the pre-existing one's wikilinks were rewritten corpus-wide. That is the same harm this PR calls the bug.

Worth being precise about my error: I did check the slug-collision loop and correctly found it stable — under reordering. Stability under adding a key is a different property, and I asserted the stronger claim without testing it. The doc now states what is actually guaranteed and names the caveat.

Also corrected: "alphabetical" is codepoint order, which for equal-length names systematically prefers the more-ASCII spelling (IPhone 15 over iPhone 15, Inigo Ruiz over Íñigo Ruiz). Any total order must pick something, but the doc shouldn't imply locale collation.

The rule I promoted to public contract was unpinned

Mutant (d) — reordering isupper and len in the title key — passed all 50 tests. So this PR documented "non-ALL-CAPS → shortest" in docs/pipeline/graph.md while nothing checked that order.

That is exactly the defect I identified for the type rule two paragraphs into my own PR body: a test passing for the wrong reason. test_dedup_variants_into_one_entity cannot pin it, because there the non-caps name is also the shortest. _best_title({"IBM": 5, "Ibm Global": 1}) discriminates — IBM is shorter and more frequent and must still lose. Both reorderings now fail.

Recorded, not fixed

  • Slug-collision assignment depends on the set of keys, so a new entity whose key sorts earlier can relocate an existing node. Links are rewritten consistently, so nothing dangles — but making paths independent of sibling keys would change existing paths for every corpus, which is a migration decision rather than a bugfix. Backlog.
  • Migration: a deployed corpus with a genuine type tie relocates that node on the next build. Handled correctly — no orphan .md, no dangling links — but build.py uses os.remove and never rmdir, so an empty entities/<old-type>/ directory is left behind. Cosmetic; noted.
  • isupper() is False for caseless scripts, so a CJK name always reads as non-ALL-CAPS and outranks Latin caps, and 3M is penalised as shouty. Pre-existing, not introduced here.

Corrected in the PR body: the test count (graph 51, main was 47).

graph 51 · clean 256 · corpus 48 · slack 13 · fetch 33 · answer 66 · benchmark 3, bare pytest (CI mode) · ruff clean · scorecard 25/25.

@sturlese
sturlese merged commit 30796ed into main Jul 25, 2026
10 checks passed
@sturlese
sturlese deleted the fix/bughunt-graph-tiebreak-determinism branch July 25, 2026 05:32
sturlese added a commit that referenced this pull request Jul 25, 2026
…I needed

Gate review: I removed the `.lower()` from the expected-name comparison on the grounds
that "the mutation set showed nothing distinguished it from the exact form". The premise
was false. The mutation set was blind to case-folding filesystems — macOS APFS, Windows,
Docker Desktop bind mounts — which is precisely the environment `content_name`'s docstring
and `_clobbered_by_sidecar`'s samefile() dance exist for, and which two tests in this file
already cover.

On such a filesystem `D.md` and `D.MD` are ONE file, so a case-only difference is not a
format change, and treating it as one is worse than missing it. Measured on this machine
(verified case-folding) with GOOGLE_DOCS_FORMAT=MD — a typo in the very knob this PR
fixes:

    pass 2: downloads=['D'] skipped=0  disk=['D.json']            <- content GONE
    pass 3: downloads=['D'] skipped=0  disk=['D.MD', 'D.json']
    pass 4: downloads=[]    skipped=1

The download writes `D.MD`, then the pre-existing rename cleanup unlinks `D.md` — the same
inode — so the mirror loses the document for a full poll interval (1800s default) and
re-downloads twice. On main that config is a silent no-op; the branch made it destructive.

So the guard is back, with a comment that says why in terms of the filesystem rather than
of "any difference is a real change". And this time it is pinned:
test_a_case_only_name_difference_is_not_a_format_change asserts zero downloads AND that
the document is still in the mirror.

Also pinned the `.exists()` half, which nothing covered: a manifest entry whose file is
gone must re-download. It is what limits any name-comparison mistake to one cycle instead
of permanent silent absence, so it is load-bearing, not belt-and-braces. Mutants now
caught: dropping the expected-name check, the case-sensitive form (the one I had shipped),
and dropping `.exists()`.

Worth naming the mistake, because it is the inverse of PR #43's lesson. There I documented
a rule no test pinned. Here I had a rule that mattered, my tests could not see it, and I
resolved that by DELETING the rule instead of extending the tests. Same root cause —
trusting the test set to define what matters — opposite and worse outcome.

The pre-existing "unlink your own download on a case-folding fs" bug this exposed is
already on the bughunt backlog as its own lead; this change no longer triggers it.

fetch 36 (+3). scorecard 25/25, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sturlese added a commit that referenced this pull request Jul 25, 2026
…#45)

* fix(fetch): changing GOOGLE_DOCS_FORMAT was a silent, permanent no-op

The skip decision compared the remote fingerprint and "some file is at the recorded
path" — never whether that path is the name the CURRENT config would write.
`fingerprint` is modifiedTime|size|md5, purely remote, so a LOCAL export-format change
is invisible to it:

    GOOGLE_DOCS_FORMAT=md   -> {'added': 1}   on disk D.md   localPath D.md
    GOOGLE_DOCS_FORMAT=pdf  -> {'skipped': 1} on disk D.md   localPath D.md
                               downloads: []

So switching md -> pdf did nothing, forever. No download, no log line, no error: the
operator believes PDFs are flowing while clean keeps converting the old Markdown, and
no recipe in docs/operations/runbook.md tells you to wipe raw/ for it. The knob is
documented in both docs/pipeline/fetch.md and pipeline/.env.example, and
fetch/index.md even lists "New export format / MIME mapping" as a supported task.

The skip now also requires `prev_local == expected_local`, computed with the same
content_name() the download uses. That comparison subsumes the old truthiness check —
`None == "D.pdf"` is False, so a missing recorded path still falls through — and it is
exact rather than case-insensitive: this path is always written from content_name, so
any difference is a real change, and a hand-edited manifest differing only in case
costs one re-download and then records the canonical name.

Verified it settles: the pass after the switch re-downloads nothing, and the stale
export is unlinked by the existing rename cleanup rather than left in the mirror.

I dropped a case-insensitive comparison I had written first, along with the comment
justifying it: the mutation set showed nothing distinguished it from the exact form, and
per PR #43's lesson a documented rule no test pins is worse than not claiming it.

Debugging note for the record: I spent a while chasing "3 failed" runs that turned out to
be my own mutation harness leaving a mutated file in place, not a defect. Re-ran with a
hash-verified restore — 34/34 five times consecutively, and main fails only the new test.

fetch 34 (+1). scorecard 25/25, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(fetch): restore the case-insensitive compare — I deleted a guard I needed

Gate review: I removed the `.lower()` from the expected-name comparison on the grounds
that "the mutation set showed nothing distinguished it from the exact form". The premise
was false. The mutation set was blind to case-folding filesystems — macOS APFS, Windows,
Docker Desktop bind mounts — which is precisely the environment `content_name`'s docstring
and `_clobbered_by_sidecar`'s samefile() dance exist for, and which two tests in this file
already cover.

On such a filesystem `D.md` and `D.MD` are ONE file, so a case-only difference is not a
format change, and treating it as one is worse than missing it. Measured on this machine
(verified case-folding) with GOOGLE_DOCS_FORMAT=MD — a typo in the very knob this PR
fixes:

    pass 2: downloads=['D'] skipped=0  disk=['D.json']            <- content GONE
    pass 3: downloads=['D'] skipped=0  disk=['D.MD', 'D.json']
    pass 4: downloads=[]    skipped=1

The download writes `D.MD`, then the pre-existing rename cleanup unlinks `D.md` — the same
inode — so the mirror loses the document for a full poll interval (1800s default) and
re-downloads twice. On main that config is a silent no-op; the branch made it destructive.

So the guard is back, with a comment that says why in terms of the filesystem rather than
of "any difference is a real change". And this time it is pinned:
test_a_case_only_name_difference_is_not_a_format_change asserts zero downloads AND that
the document is still in the mirror.

Also pinned the `.exists()` half, which nothing covered: a manifest entry whose file is
gone must re-download. It is what limits any name-comparison mistake to one cycle instead
of permanent silent absence, so it is load-bearing, not belt-and-braces. Mutants now
caught: dropping the expected-name check, the case-sensitive form (the one I had shipped),
and dropping `.exists()`.

Worth naming the mistake, because it is the inverse of PR #43's lesson. There I documented
a rule no test pinned. Here I had a rule that mattered, my tests could not see it, and I
resolved that by DELETING the rule instead of extending the tests. Same root cause —
trusting the test set to define what matters — opposite and worse outcome.

The pre-existing "unlink your own download on a case-folding fs" bug this exposed is
already on the bughunt backlog as its own lead; this change no longer triggers it.

fetch 36 (+3). scorecard 25/25, ruff clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant