SIGN IN SIGN UP

fix(mental-models): store delta documents as verbatim markdown blocks (#3361, #3273) (#3622)

* fix(mental-models): store delta documents as verbatim markdown blocks (#3361, #3273)

A knowledge page could come back with a whole table welded onto one line, in
sections no delta operation had named, and never recover. The delta refresh was
blamed, but the damage was done one refresh earlier.

`structured_content` was only the source of truth on the delta leg. The full leg
stored the LLM candidate markdown verbatim in `content` while deriving the
structure from it with `parse_markdown`, a typed-block parser that flattened
anything its union could not express -- nested lists, list continuation lines,
blockquotes, hard line breaks, horizontal rules, HTML, indented code, table
alignment, a table row missing an outer pipe. 15 of 16 common constructs lost
information, and every loss was a fixed point, so no later refresh could undo
it. The two columns disagreed by construction, and the next delta refresh
published the degraded one over the whole document.

Schema v2 stores each block as a verbatim markdown fragment plus an id. Nothing
parses a table, so nothing can flatten one. `parse_markdown` is deleted;
`split_markdown` replaces it and recognises only ATX headings and blank lines,
both fence-aware, which makes it lossless -- asserted as a property over a
26-case corpus of exactly the constructs v1 destroyed.

Blocks are now addressed by id rather than by index (#3273). An index has to be
counted by the model, and an off-by-one lands in range, silently overwrites an
unrelated block, and is recorded as a success. An id is copied, not derived; one
that does not resolve -- or that names a block in a different section -- is
skipped and reported. Operation payloads are plain markdown strings, so the
model no longer has to emit a typed block union either.

`content` is now always the render of `structured_content`, on both legs, so the
two can no longer drift apart.

Also here:
- `parse_llm_json` escapes `\n \r \t \b \f` inside JSON string values instead of
  blanking them. A model writing a markdown table into a string often forgets to
  escape its line breaks, and replacing them with spaces delivered the table
  already collapsed. Other control characters keep the previous treatment.
- A model that adds a table row as its own block would render a broken table, so
  the prompt asks for `replace_block` and bare rows landing directly after a
  table are folded into it.
- Migration `d1e2f3a4b5c6` clears v1 blobs. They are a lossy projection of the
  row's own `content`, so there is nothing to convert: the next refresh
  re-imports the structure from the markdown, losslessly. `content` is untouched.
- The Gemini eval fixture pinned `gemini-2.0-flash`, which the provider has
  retired (404), so the whole eval class was dead.

Verified against a real model: `test_document_survives_many_delta_rounds_intact`
runs five delta rounds feeding one new fact each, asserting after every round
that content is the render of the structure, that no line welds a table
separator to other cells, that sections no operation named are byte-identical,
and that a section never named across the whole run is unchanged at the end. Run
four times, 20 real rounds, green. It is what caught the orphan table row.

* fix(mental-models): write the structure whenever content is written

`create_mental_model(content=...)` and `create_knowledge_page` inserted the
markdown and left `structured_content` NULL, so a model authored as markdown had
no structure until its first delta refresh -- and that refresh was then the one
to derive it, silently reshaping a document nobody had asked it to touch.
`update_mental_model(content=...)` was worse: it could set the markdown while
leaving the *previous* document's structure in place, so the two columns
described different documents until the next refresh papered over it.

Nothing enforced the pairing; the refresh path just happened to pass both.

Both writes now go through `canonical_document()`, which splits the authored
markdown and hands back the structure together with its render. The insert
stores both, and the update derives the structure whenever a caller supplies
content without one. A refresh still passes both explicitly -- there the
structure is authoritative and the markdown is already its render -- and is
untouched. The derivation is hoisted above the embedding computation so the
embedding, the history snapshot and the UPDATE all see one text.

`content` is therefore the render of `structured_content` from the first byte
rather than from the first refresh, which is what the assertion churn in this
commit is: authored markdown now comes back canonicalised, so a document that
was stored as "v1" reads back as "v1\n".

Also makes the migration test rerunnable: a pg0 instance survives between runs
and alembic will not replay a migration on a DB already stamped past it, so
seeding into it would have left the rows untouched and the test asserting
nothing.

* feat(reflect): answer with a document, render the markdown from it

The mental-model refresh asked the agent for markdown and worked out the
document's structure by reading that markdown back. Reading LLM markdown back is
where #3361 destroyed tables, and it is unnecessary here: the refresh knows it is
producing a document, so it can ask for one.

`done()` gains a document mode. Instead of an `answer` string it takes a
`document` -- an ordered list of sections, each with a heading, a level and its
blocks -- and the markdown that gets stored and shown is rendered from it. The
model no longer writes the markdown that gets persisted, and nothing parses
markdown to find out what the model meant. `answer` is not merely discouraged in
that mode, it is absent from the schema, so there is no escape hatch back to
prose.

The shape is deliberately flat -- an array of sections holding arrays of block
strings, no unions. A tool schema goes to the provider verbatim and not every
provider accepts `oneOf` (Gemini rejects it), and a shape the model can fill
without thinking is one it fills correctly.

`document_from_sections` is tolerant, because a tool call is still model output:
a missing heading, a `##` the model prefixed anyway, an out-of-range level or a
non-string block is coerced rather than rejected. A block holding several
blank-line-separated fragments is split into one block each, so the document
keeps the granularity delta operations address even when the model packs a whole
section into one string.

Downstream is unchanged: the rendered markdown still flows on as `text`, so
structured-output extraction, the length rewrite and the HTTP response all
behave as before. The one place the two could drift is the length rewrite, which
edits the text after the fact -- there the structure is re-derived from the
rewritten markdown, which is lossless and keeps the invariant that the stored
text is exactly what the stored structure renders.

Splitting markdown is now only an import path: a model created from authored
markdown, a restored export, or a run that produced plain text anyway (a
provider that dropped the tool call, the iteration-limit answer).

Verified against a real model: all five `hs_llm_core` refresh evals pass with the
agent emitting structure, including the five-round stability run. The ordered
list that a previous run had rewritten now survives untouched, and the new table
row lands inside the table rather than beside it.

* test(benchmarks): compare two builds on how a document survives being edited

Neither half of "is the new pipeline better" was measurable before this. The
unit tests prove the mechanics in isolation and the refresh evals prove one
build behaves, but nothing compared a build against another one on the thing
that actually broke: a document rewritten by an LLM over and over.

The harness talks HTTP only, so the same code drives a server built from any
revision. That is what makes an A/B possible without a feature flag inside the
code under test: run it once per build, compare the two artifacts. Every
document from every round is stored, and metrics are recomputed at comparison
time, so sharpening a metric costs nothing instead of another few hundred LLM
calls.

Two things it measures, deliberately separately.

Structural, no LLM: collapsed tables (the detector from #3361), rows, nesting,
hard breaks, fences and quotes lost, sections that drifted with no operation
naming them, plus pipeline health and latency. Damage is counted only in
sections no operation named -- a refresh that rewrites a section it targeted may
legitimately restructure it, and scoring that as corruption would punish the
model for doing its job.

Content, judged: each round declares what must be true afterwards and what must
no longer be stated, checked one claim at a time so a miss points at a fact
rather than at a score; plus a blind pairwise preference between the two builds'
final documents, judged in both orderings so position bias cannot decide it.

Three cases, and the third is the point. `api-reference` carries every fragile
construct; `onboarding-playbook` carries none, because a change that fixes
tables while degrading ordinary prose is not an improvement and that is where it
would show; `release-runbook` puts a table with a missing outer pipe in a
section the fact stream never touches. Both details are load-bearing: a
well-formed table never triggered the bug, and a model asked to edit a malformed
table tends to rewrite it correctly, repairing the damage before it can be
measured. The reported failure was in sections the operations never named, where
nothing could repair it.

Token usage is not reported. The stored reflect_response does not carry it, and
a column that is always zero reads as "this is free" rather than "this is not
measured here".

* test(benchmarks): harden the judging, and say what the A/B measured

Running the benchmark against main and the branch turned up three problems in
the benchmark itself, all of which would have made its verdict untrustworthy.

The runner slept a fixed interval instead of awaiting the async operations it
submitted, so its first results were five rounds of "Generating content..."
scored as though they were documents. Retain, create and refresh are all
submit-and-poll; it now polls, and refuses outright if the seed it is about to
measure is still a placeholder.

Damage was attributed to the whole document rather than to the sections nobody
asked to change, which scored a model deliberately restructuring a section it
targeted as if the machinery had corrupted it. Damage is now measured only in
untouched sections, and the metrics are recomputed from the stored documents at
comparison time, so this sharper reading could be applied to results already
collected instead of paying to re-run them.

A single judge call decided each claim, and one pedantic reading moved a build's
score: a document describing an operation as "synthesises stored memories" was
scored as not supporting "answers questions over stored memories" — the same
operation, in the wording the source fact itself used. Claims are now decided by
majority of three, and that claim was rewritten to test the fact rather than one
phrasing of it. A claim a correct document can fail measures the corpus, not the
pipeline.

The report prints mean document length beside the preference column, because
judges favour longer documents and a preference that tracks that column should
be read sceptically rather than counted.

The prompt change is the finding that landed back in the product: stating a
document's structure is more clerical than writing prose, and the model got
terser at it — measurably shorter documents that the judge liked less. Document
mode now says plainly that the structure is the shape of the answer, not a
budget for it.

Baseline recorded in baseline_report.json: 45 refresh rounds per build from
identical seeds. main lost 3 tables to collapse, 9 table rows, 6 levels of list
nesting and 5 hard line breaks across 6 damaged rounds, and drifted 14 sections
nobody had named. The branch lost nothing and drifted nothing. Content came out
level — 100% recall and zero stale claims on both sides.

* fix(mental-models): give the delta leg the document's own token budget

`max_tokens` was enforced in exactly one place: a rewrite of the *synthesis*
answer when it came back longer than the budget. In delta mode that answer is
only context for the operations call and never becomes the document, so the
document that actually gets stored was never measured against the budget at all.

A delta refresh only adds. The document-evolution benchmark measured ~20 tokens
of growth per round across 45 rounds, monotonic, which crosses the 4096-token
knowledge-page default after a couple of hundred refreshes — and knowledge pages
refresh after every consolidation. The configured budget was quietly ignored for
the entire life of a page after its first full build.

Truncating the document here would delete knowledge nobody asked to delete, so
the budget is stated instead: the delta call is told the document's current size
against its budget, and when it is over, asked to make room with the same
operations it uses for everything else — on content that is superseded or
duplicated, never by dropping the facts it is integrating and never by
summarising a section that is still current. Below 80% of the budget nothing is
said at all. Every refresh records document_tokens and document_budget, and
going over adds a warning, so a page that keeps growing is visible rather than
merely large.

Also closes the one path where the model still wrote markdown that got stored:
the over-budget trim. In document mode it is now asked for a document, so the
structure survives the trim instead of being re-derived from prose the model
wrote. A response that is not JSON falls back to the previous split, which is
lossless — the worst case is the old behaviour, not a lost answer.

The real-LLM eval is the part that could not be mocked: told that a document is
over budget, does a model reclaim space or append anyway? It drops the twelve
archived sections and keeps the current process and the checklist — 434 tokens
to 39 against a 200-token budget, stable across four runs. The assertions check
where the space came from, because getting under budget by deleting current
content would pass a naive shrink check and be worse than going over.

* test(mental-models): audit every trigger flag on the delta leg

`max_tokens` looked wired up — read from the model, passed to reflect, enforced
by a rewrite — and was still ignored for the document that actually got stored,
because in delta mode the thing it capped never becomes the document. Reading
the code is how that was missed; nothing asserted the flag at its destination.

So every flag is now exercised through a real delta refresh and asserted where
it lands: retrieval options at the reflect call, document options in the delta
prompt or the persisted row. Full mode is covered by the surrounding modules;
this is the leg where a flag goes to die.

Fifteen flags checked. Fourteen were already honoured. The audit pins them so a
future change to either leg cannot quietly drop one:

- retrieval: fact_types, exclude_mental_models, exclude_mental_model_ids, the
  model's own id (a model must not feed on its previous version), include_chunks,
  recall_max_tokens, recall_chunks_max_tokens, the model's tags, tags_match
- document: max_tokens, response_schema (extracted from the merged document, not
  from reflect's delta-only answer), keep_trace, mode
- and the whole trigger surviving a create/read round trip, since a flag that
  does not persist is not honoured either

The fifteenth is documented behaviour that reads as a bug from outside, so it is
pinned as intended rather than "fixed": `tag_groups` overrides flat tags
entirely, dropping the model's own tags and forcing `tags_match` to `any`,
because each group carries its own match mode. A `tags_match` set alongside
groups is deliberately not forwarded. The default for a tagged model with no
`tags_match` is `all_strict`, not `any` — a model scoped to tags must not widen
its own scope by default.

Scheduling flags (`refresh_cron`, `refresh_after_consolidation`) are honoured
outside the refresh executor — the maintenance loop and the consolidation hook —
and keep their existing coverage there.

* test(benchmarks): type the structural summary, and cover the flag main just added

Two findings from reviewing the branch against a fresh main.

`_structural_summary` returned a raw dict of known keys, which the project
standards forbid for exactly the reason it bit here: the report read it with
`summary["rounds"]` and nothing would have caught a renamed metric until the
table rendered wrong. It is a `StructuralSummary` model now, and the side-by-side
table renders `model_dump()` so a metric added later appears without being
listed twice.

Main added a sixteenth trigger flag while this branch was in flight
(`min_refresh_interval_seconds`, #3621). It gates automatic refreshes rather than
shaping one, so it is honoured in the submit path and covered there — but the
round-trip test enumerates the whole trigger on purpose, because a field that
round-trips as None looks like the flag being ignored rather than like a storage
bug. Adding it keeps that list exhaustive.

* fix(mental-models): teach the retraction prompt the schema it emits into

CI caught what the rebase brought: main added an unsay pass (#3618) whose prompt
documents the operation vocabulary a second time, in prose, and it still told the
model to say `{"op": "remove_block", "section_id": "...", "index": N}` with typed
`block` payloads. Under the id-addressed schema those ops fail validation and are
dropped, so a retracted fact would keep being stated and nothing would say why —
the unsay feature silently doing nothing.

The prompt now describes the schema it actually emits into: blocks addressed by
`block_id`, block payloads as markdown strings, and the note about emitting
removals in descending index order deleted, because ids do not shift when a
sibling is removed and telling a model to order by position invites it to think
in positions again.

Guarded structurally rather than by one more test. The op vocabulary is written
down twice — Pydantic models the applier validates against, and prose in each
system prompt — and a test for the prompt that drifted does not exist by
construction, since it is the one nobody wrote. `test_delta_prompt_schema_parity`
asserts over *every* prompt carrying an operations vocabulary that each op exists,
that no shape names a field the schema rejects, that no v1 typed block survives,
and that blocks are addressed by id; plus a check that a prompt asking for
`{"operations": [...]}` cannot be left off the list. Reverting the prompt fails
three of them.

The rest is the same schema change reaching tests the rebase brought in: canned
ops in the outcome matrix moved to `text`, the retraction tests resolve a real
`block_id` out of the document the prompt shows them (which is what a model does,
and what a hardcoded index cannot express), the stale `parse_markdown`
monkeypatch points at `structured_document_from_stored`, and four assertions on
authored content now expect the canonical render.
N
Nicolò Boschi committed
fe5c25d64c919c771b7bba9540a928be7a3c15be
Parent: 51837b8
Committed by GitHub <noreply@github.com> on 8/20/2026, 9:35:32 AM