Visitar URL original
fix(parser): a blank line no longer eats a directive's tags by robcohen · Pull Request #2189 · rustledger/rustledger · GitHub
Skip to content

fix(parser): a blank line no longer eats a directive's tags - #2189

Merged
robcohen merged 3 commits into
mainfrom
fix/blank-line-directive-tags
Aug 30, 2026
Merged

robcohen merged 3 commits into
mainfrom
fix/blank-line-directive-tags

Conversation

@robcohen

Copy link
Copy Markdown
Member

The bug

convert_note and convert_document collected a directive's #tag / ^link tokens by walking the node's direct tokens and breaking at the first NEWLINE. A directive node begins with the trivia that precedes it, so when a blank line sits above the directive, its first token is a NEWLINE — the loop broke immediately and every tag and link was dropped.

That is the common case, not a corner. A blank line between directives is how ledgers are normally written. Of the three tagged document directives in the compatibility corpus, two lost their tags and one kept them — the one written directly under another directive.

document has carried this since long before note had any tags to lose (#2160). Nothing caught it because every fixture placed the directive immediately after another one.

Fix

Both call sites now share header_tags_and_links, which starts collecting only once the header's STRING has been seen and breaks on the newline after it.

Cache versions

Parser output changed, so both cache versions move: loader 31 → 32, wasm 15 → 16.

The wasm bump is overdue. #2160 moved the loader version and the pin but left CACHE_VERSION at 15, so v15 is now ambiguous — a blob written before #2160 holds the two-field-shorter ArchivedNote, one written by a dev build after it holds the new shape, and both claim v15. Rejecting every v15 blob is the only reading that is safe for either.

The guard that should have caught it

LOADER_CACHE_VERSION_PIN can only force a question; it cannot distinguish a loader-internal change from one that moves our archived directives. On #2160 the question got the wrong answer and every test still passed.

directive_archived_form_is_pinned can tell. It parses one of every directive kind and hashes the archived bytes, so a layout change (new/reordered field) and an output change (different value) both fail it, with a message naming the bump to make.

Verification

Both negative controls fire:

mutation kind result
revert this parser fix output FAILS
swap Note's tags/links declaration order layout, values identical FAILS

The first control initially passed — the fixture's note and document followed other directives directly, so it never exercised the blank-line path. The blank lines in that fixture are now load-bearing and documented as such. The completion test made the identical mistake and is fixed the same way.

  • Baseline drift is exactly the 2 corpus files containing tagged documents. Probed before/after: old code kept tags on 1 of 3 tagged documents, new code keeps 3 of 3.
  • corpus_parity divergences: 20, unchanged from main (pre-existing).
  • clippy 1.97 workspace clean; fmt, typos, cargo doc clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr

`convert_note` and `convert_document` collected the `#tag` / `^link`
tokens on the header line by walking the node's direct tokens and
breaking at the first NEWLINE. A directive node begins with the trivia
that precedes it, so when a blank line sits above the directive its
first token is a NEWLINE and the loop broke before reaching anything --
every tag and link dropped.

That is the common case, not a corner: a blank line between directives
is how ledgers are normally written. Of the three tagged `document`
directives in the compatibility corpus, two lost their tags and one
kept them, the one written directly under another directive.

`document` has had this since long before `note` had any tags to lose,
and nothing caught it because every fixture placed the directive
immediately after another one. The completion test added here made the
same mistake first and passed while the bug was live; its fixture now
has the blank line that makes it real.

Both directives now share `header_tags_and_links`, which starts
collecting only once the header's STRING has been seen and breaks on
the newline after it.

Cache versions move because parser OUTPUT changed: loader 31 -> 32, and
wasm 15 -> 16. The wasm bump is overdue -- #2160 moved the loader
version and the pin and left this one at 15, so v15 is now ambiguous
between the old and new `ArchivedNote` shapes and every v15 blob has to
be rejected.

That miss is why `directive_archived_form_is_pinned` exists. The
version pin can only force a question; it cannot tell a loader-internal
change from one that moves our archived directives, and on #2160 the
question got the wrong answer with every test still passing. The new
test hashes the archived bytes of one of every directive kind, so a
layout change and an output change both fail it. Verified against both:
reverting this parser fix and swapping `Note`'s two field declarations
each move the digest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr
@robcohen
robcohen requested a review from a team as a code owner August 30, 2026 04:15
Copilot AI lite review requested due to automatic review settings August 30, 2026 04:15
The parse-side pin added with the blank-line fix covers what
`ParsedLedgerPayload` holds. `LedgerPayload` holds something else: the
output of the shared `process()` pipeline, sorted by
`canonical_sort_key` and booked.

That gap is not hypothetical. #2187 swaps `DirectivePriority` for `Pad`
and `Balance`, which reorders a cached ledger without changing a single
parsed directive -- the parse-side digest does not move, and a v16 blob
would deserialize into an order that build no longer produces.

The fixture puts a pad and a balance on the same date on purpose: that
pair is the one whose relative order the priorities decide, and on
different dates the date component of the sort key settles it and a
priority swap changes nothing. Verified by applying #2187's swap, which
fails this test and passes the parse-side one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr
@robcohen

Copy link
Copy Markdown
Member Author

Pushed a second commit: processed_ledger_archived_form_is_pinned.

The parse-side pin covers ParsedLedgerPayload. LedgerPayload holds something else — the output of the shared process() pipeline, sorted by canonical_sort_key and booked. A DirectivePriority change reorders that list without touching a single parsed directive, so the parse-side digest does not move while every cached blob goes stale.

Not hypothetical: #2187 swaps Pad and Balance priorities. Applying that swap fails the new test and passes the parse-side one, which is the control for it. #2187 needs a wasm CACHE_VERSION bump it does not currently have; I will add it there.

robcohen added a commit that referenced this pull request Aug 30, 2026
`LedgerPayload` archives the output of the shared `process()` pipeline,
which sorts by `canonical_sort_key` -- and that key reads
`DirectivePriority`. Swapping `Pad` and `Balance` therefore changes the
LIST this cache holds without changing a single directive in it: every
one archives to the same bytes, in a different order, with the pad
amounts that went with the old order.

A v15 blob read by this build would serve exactly the arrangement the
fix removes, which is the bug back from disk on a fixed binary.

Nothing in the suite catches this class today: the version pin only
asks the question, and it is answerable "loader-internal" when it is
not. #2189 adds `processed_ledger_archived_form_is_pinned`, which fails
on this very swap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix is well-scoped, includes targeted regression coverage for the previously-missed blank-line path, and correctly bumps the relevant cache versions/pins to prevent serving stale parsed output.

Pull request overview

Fixes a CST→model conversion bug where a blank line (leading trivia) before a note/document directive caused header #tag / ^link tokens to be dropped, ensuring tags/links are preserved in the common “blank line between directives” formatting pattern.

Changes:

  • Introduces a shared header_tags_and_links helper and updates convert_note / convert_document to correctly collect tags/links past leading trivia.
  • Bumps loader and WASM cache versions to reflect parser output changes (and resolves the previously-ambiguous WASM v15 situation).
  • Adds/adjusts regression tests (parser integration + WASM completion + cache digest pins) and updates parser corpus baselines.
File summaries
File Description
tests/baselines/parser-corpus.manifest Updates expected parser-corpus baseline hashes for affected fixtures.
crates/rustledger-parser/src/cst/convert.rs Fixes tag/link collection for note/document headers by skipping leading trivia and stopping at the header newline.
crates/rustledger-parser/tests/parser_integration_test.rs Adds regression test proving tags/links survive when a blank line precedes note/document.
crates/rustledger-loader/src/cache.rs Bumps loader cache version to invalidate stale cached parser output (v31 → v32).
crates/rustledger-wasm/src/cache.rs Bumps WASM cache version (v15 → v16), updates loader pin, and adds archive-form pin tests to guard future cache-version misses.
crates/rustledger-wasm/src/editor/completions.rs Adds regression test ensuring tag completion includes tags that exist only on notes (and exercises the blank-line case).
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

📊 Benchmark Results

Shape rustledger beancount Speedup RSS (rledger / beancount)
simple 47.6ms ± 4.2ms 710.9ms ± 3.2ms 14.9x 55.1 MB / 50.1 MB
investment 67.2ms ± 1.5ms 2734.6ms ± 41.8ms 40.7x 59.0 MB / 57.9 MB
fifo 66.8ms ± 4.3ms 15756.8ms ± 84.6ms 235.8x 61.1 MB / 57.7 MB

vs base, measured in this run

Shape base this PR Change RSS base → PR
simple 47.7ms ± 5.9ms 47.6ms ± 4.2ms -0.1% ✅ within noise 55.0 MB → 55.1 MB (+0.2%)
investment 68.1ms ± 2.9ms 67.2ms ± 1.5ms -1.2% ✅ within noise 59.0 MB → 59.0 MB (+0.1%)
fifo 66.6ms ± 3.2ms 66.8ms ± 4.3ms +0.4% ✅ within noise 61.1 MB → 61.1 MB (+0.1%)

A difference counts only when it clears the combined spread of the two measurements, per shape (2σ: simple 3.2ms, investment 1.5ms, fifo 2.4ms), and is at least 5%.
10,000 transactions per shape · 20 runs, 3 warmup · hyperfine

What do these numbers mean?
  • simple — Two-posting transfers, one currency. The historical workload — keep as the floor, not as the benchmark.
  • investment — Cost specs, per-unit prices and lot reductions. Exercises booking and inventory, which simple leaves at 0.3%.
  • fifo — Bare {} reductions against a FIFO account — how FIFO sells are normally written. The other four shapes reach the ordered-selection path twice in 2k transactions between them, so it was effectively unprofiled until it turned out to be quadratic (plan_ordered scans every lot per reduction: FIFO ledgers are quadratic (8.8s at 20k transactions) #2083).
  • vs base: the PR and the base-branch commit it is built on, built and measured in the same job
  • RSS: peak resident set size, a single sample — reported without a verdict because there is no spread to test it against
  • A shape with no beancount column is one bean-check reports errors on; it is timed for rledger only

Both call sites still described stopping "at the first NEWLINE" as the
thing that captures the header, which is the bug the commit before this
one removed. They now point at `header_tags_and_links`, which is where
the rule lives.

Also records that the formatter already guarded its header walks on a
`seen_content` flag for exactly this reason. The converter was the
outlier, not the pioneer.
@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

Copy link
Copy Markdown
Contributor

✅ Compatibility Test Results

Metric Result
Check Compatibility 100%
BQL Compatibility 100% (raw 93%, 170 masked)
Full AST Match 100%
Total Files 811
What's tested
  • Check: Exit codes and error counts match bean-check
  • BQL: Query results match bean-query
    The headline is the EFFECTIVE rate, after pairs where beanquery
    itself is wrong are masked. raw is the same run with nothing
    masked. A widening gap means the registry grew, not that rledger
    changed.
  • Files: ~800 test files downloaded from 10+ beancount repositories

Full results available in workflow artifacts

@robcohen
robcohen merged commit 2377b90 into main Aug 30, 2026
69 of 70 checks passed
@robcohen
robcohen deleted the fix/blank-line-directive-tags branch August 30, 2026 06:35
robcohen added a commit that referenced this pull request Aug 30, 2026
`LedgerPayload` archives the output of the shared `process()` pipeline,
which sorts by `canonical_sort_key` -- and that key reads
`DirectivePriority`. Swapping `Pad` and `Balance` therefore changes the
LIST this cache holds without changing a single directive in it: every
one archives to the same bytes, in a different order.

What a stale blob costs is narrower than it first looks, and the first
draft of this message got it wrong by saying the pad AMOUNTS come back
stale. They do not: `process_pads` re-sorts its input by
`booking_sort_key`, the same `(date, priority)`, so amounts computed
from a v16 blob still come out right. What differs is the order of the
directives the cache hands back -- what journal output, `SELECT` row
order and the FFI directive stream all read. A cache that returns a
different sequence than a fresh run is stale by definition, which is
reason enough and is the reason recorded.

`processed_ledger_archived_form_is_pinned`, which landed on main with
#2189 for exactly this class, failed on the rebase and had to be
re-pinned. That is the guard working: the version bump was argued for in
review before, and is now also demanded by a test.

Also folds in two review corrections: `pad.rs` no longer claims to sort
"by date" directly above the comment explaining that it sorts by the
booking key, and the variant census in `cache.rs` identifies variants by
`Directive::type_name` rather than `Debug` on `mem::Discriminant`, whose
format is not a documented guarantee -- it also names the missing
variant when it fails.
robcohen added a commit that referenced this pull request Aug 30, 2026
* fix(format): stop deleting a note's tags and links

`rledger format` uses the CST formatter, not the model formatter #2184
points at. There, `emit_document` carried a direct-child token walk that
re-emits the header's TAG / LINK tokens and `emit_note` carried nothing,
so formatting a note silently deleted whatever the author had written on
its header. Formatting is supposed to preserve meaning.

Both now share `emit_header_tags_and_links`. The walk is the document
one unchanged, including its two hard-won details: skip LEADING trivia,
because a blank line before a non-first directive attaches its NEWLINE
inside the node, and skip leading COMMENT lines for the same reason. The
converter had the identical bug from the identical cause (#2189).

The formatter's own IDEMPOTENCE_MATRIX could never have caught this:
dropping a note's tags is perfectly idempotent, and the matrix asserts
stability, not fidelity.

So the new test asserts fidelity -- parse, format, parse, compare
models. That alone is still not enough, and not hypothetically: run it
on a tree whose CONVERTER also drops these tags and both sides come back
tagless and equal while every tag in the file has been deleted. It
therefore also asserts on the formatter's output text, which is the
thing under change and cannot be satisfied by a matching pair of empty
models.

Verified: reverting the `emit_note` line fails the test; corpus scripts
report checked=693 non_idempotent=0 dropped_comments_in=0.

The same fidelity check over the WHOLE matrix fails on
`balance_with_arithmetic_and_tolerance`, where the formatter emits a
balance that no longer parses. That is a parser inconsistency, filed as
#2191, and the matrix-wide harness lands with its fix rather than behind
an allow-list here.

Closes #2184

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr

* refactor(format): a child node no longer stops the header walk

Review feedback: `emit_header_tags_and_links` bailed out of the loop on
the first `NodeOrToken::Node`, so it did not walk all direct-child
tokens the way its name claims.

Harmless for the two callers it has -- a note's or document's only child
node is a META_ENTRY, which sits past the header newline the walk has
already broken on -- but the failure mode if a header ever gains a child
node is that every tag after it disappears with nothing to say so. This
helper exists because exactly that kind of silent drop shipped twice.

Behavior is unchanged today: full parser suite, and 693 corpus files
still report non_idempotent=0 and dropped_comments_in=0.

* fix(core): the typed-directive emitter keeps tags and links too

There are TWO live formatters, and the commit before this one fixed one
of them. `format_source` is what `rledger format` runs.
`canonicalize_directives` is what `rledger add`, `rledger extract` and
the FFI `format.entry` / `format.entries` endpoints run, and it
synthesizes its intermediate text through
`rustledger_core::format::format_directives` -- where
`format_note_lines` and `format_document_lines` emitted a header with no
tags and no links.

So a note built in memory and canonicalized still lost them, and a
DOCUMENT lost them here as well: the CST emitter at least kept a
document's tags, which is why `rledger format` looked correct for
documents while this path was not.

#2184's own text warned about this -- "worth establishing which
formatter actually runs before changing either". Establishing that
`rledger format` uses the CST one answered which to fix first, not which
was the only one that mattered.

Both now share `write_tags_and_links`, in the order the transaction
emitter already used.

Verified through the shim rather than the function: a fixture with a
tagged note, document and transaction canonicalizes with every marker
intact and re-parses to the same directives. Reverting the note line
alone fails it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr

* test(format): give the typed emitter a fidelity harness of its own

The fidelity test added with the note-tag fix exercises `format_source`.
It cannot see `canonicalize_directives`, which renders through a
different emitter, so the second data-loss path this branch fixes went
unnoticed by it.

This is the same check aimed at the other emitter: parse every fixture,
render it typed, re-parse, compare models.

It needed a fixture change to be worth anything. `note_directive` was
`2024-01-15 note Assets:Cash "a note"` -- no tags -- so no harness in
this file could see a note losing them, which is precisely how the bug
survived. It now carries a tag and a link, and `document_directive`
gains a link next to the tag it already had.

That thin fixture is the whole reason to write this down: the harnesses
were sound and the input made them blind. Verified in both directions --
dropping the note or document tags from the typed emitter fails this
test, dropping them from the CST emitter fails the other one, and each
control fails only its own.

693 corpus files still report non_idempotent=0.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
robcohen added a commit that referenced this pull request Aug 30, 2026
* fix(booking): stop padding a balance on the pad's own date

A `pad` and the `balance` it targets on the same date synthesized a
padding transaction beancount does not. On the issue's fixture the
account ended 60.00 USD richer -- 100.00 where beancount reports 40.00 --
and `check` reported the pad as UNUSED in the same run that booked it.

beancount checks a balance at the START of its day: `entry_sortkey` gives
Balance -1 and Pad the default 0, so a same-date balance is evaluated
before the pad exists and consumes nothing. Ours had `DirectivePriority`
Pad=2 before Balance=3, with a comment asserting "padding before balance
assertions", and `process_pads` sorted by DATE ALONE -- so a pad written
above its balance was processed first and the balance consumed it.

Both are fixed: Balance now precedes Pad, and `process_pads` sorts by the
canonical `booking_sort_key` rather than by date.

Two tests pinned the old behavior and are inverted, with the evidence in
their comments. One asserted a same-date synth exists and precedes the
balance; the ordering guarantee it was really protecting is preserved in
a new test on the day-earlier case, which is where a synth actually
exists. The other was named `test_sort_directives_pad_before_balance`.

Verified:
  - the fixture now returns 40.00, matching beancount; a pad one day
    earlier still pads to 100.00, also matching
  - `check` still emits E2003 and E2001, which is what bean-check emits
    for the same file -- the difference is that we no longer book the pad
    we call unused
  - booked values UNCHANGED on all four corpus files that contain a
    same-date pad+balance, compared against a main-built binary
  - directive order changes only in files containing pads: 59 pad-free
    files produce byte-identical `#entries` output
  - the four new `corpus_parity` divergences are the prebuilt component
    still using the old order (`native=(balance) component=(pad)`), which
    is what a correct engine change looks like against a stale component

Not fixed here, and worth stating: beancount interleaves same-date pads
and transactions by lineno (both sort at 0) while we still group by type.
That is the ordering divergence in #2149, unchanged by this.

The booked-value gate reports clean, but its axis is prices; it does not
compare balances and so does not exercise this.

Closes #2150

* test(booking): pin the pad that follows a same-date balance

Review find on #2187. Sweeping the pad scenarios against beancount turned
up a second broken case, worse than the reported one: a pad written after
a balance on the same date, serving a LATER balance.

    2024-06-15 balance Assets:A   0.00 USD
    2024-06-15 pad     Assets:A   Equity:O
    2024-07-01 balance Assets:A  50.00 USD

bean-check accepts that file. On main the account came back with NO
balance at all and `check` reported nothing -- silently empty rather than
silently wrong. The same-date balance consumed the pad, so the later
assertion had nothing left.

The fix already covers it, because Balance now precedes Pad and the pad
stays pending. This adds the test.

Both halves of the fix are confirmed by mutation: restoring
Pad-before-Balance fails two tests, restoring the date-only sort in
`process_pads` fails one.

* style(booking): count instead of collecting in the new pad test

* test(ffi): cover the same-date pad+balance on the FFI path

Review find on #2187. The issue explicitly left the FFI path unverified,
noting that `expand_pads` runs its own date-only sort and could diverge
from the CLI either way. Nothing there covered the case: the existing
`SAME_DATE_LEDGER` shares a date between a pad and an UNRELATED
transaction, with the balance a day later.

It is correct -- `expand_pads` gets its pads from `process_pads`, which
this branch fixed, so the decision is shared rather than duplicated --
but that was unverified rather than untrue. Now asserted, and confirmed
to fail when Pad-before-Balance is restored.

The date-only sort at the top of `expand_pads` is left alone. It orders
the OUTPUT pairs, not the pad decision, and changing it would reorder
same-date directives on the FFI path only. Whether the two paths should
share an ordering rule is worth deciding on its own, not as a rider here.

* docs(booking): mark the stale same-date-balance rule in pad_insertion_index

* fix(wasm): bump the cache version the priority swap invalidates

`LedgerPayload` archives the output of the shared `process()` pipeline,
which sorts by `canonical_sort_key` -- and that key reads
`DirectivePriority`. Swapping `Pad` and `Balance` therefore changes the
LIST this cache holds without changing a single directive in it: every
one archives to the same bytes, in a different order.

What a stale blob costs is narrower than it first looks, and the first
draft of this message got it wrong by saying the pad AMOUNTS come back
stale. They do not: `process_pads` re-sorts its input by
`booking_sort_key`, the same `(date, priority)`, so amounts computed
from a v16 blob still come out right. What differs is the order of the
directives the cache hands back -- what journal output, `SELECT` row
order and the FFI directive stream all read. A cache that returns a
different sequence than a fresh run is stale by definition, which is
reason enough and is the reason recorded.

`processed_ledger_archived_form_is_pinned`, which landed on main with
#2189 for exactly this class, failed on the rebase and had to be
re-pinned. That is the guard working: the version bump was argued for in
review before, and is now also demanded by a test.

Also folds in two review corrections: `pad.rs` no longer claims to sort
"by date" directly above the comment explaining that it sorts by the
booking key, and the variant census in `cache.rs` identifies variants by
`Directive::type_name` rather than `Debug` on `mem::Discriminant`, whose
format is not a documented guarantee -- it also names the missing
variant when it fails.
robcohen added a commit that referenced this pull request Aug 30, 2026
Four PRs shipped user-visible behavior changes without an entry: the
two formatter tag losses (#2192), the blank-line tag drop (#2189), the
same-date pad (#2187), and this branch's tolerance grammar (#2195).

The changelog already carries the direct sibling of the first one --
"`rledger format` no longer deletes a posting's currency, cost or price
when it has no units number" (#2142) -- so the omission was mine, not a
question of whether this class belongs here.

Each entry records the cache versions that moved with it, which is the
part a reader upgrading actually needs and the part not recoverable from
the diff.
robcohen added a commit that referenced this pull request Aug 30, 2026
* fix(parser): a balance tolerance may follow an arithmetic amount

`directive_arithmetic_value` scanned to the first top-level CURRENCY and
treated everything before it as the amount expression. With no currency
between the amount and the tolerance, that swallowed the tolerance:
`0.25 + 0.75 ~ 0.01 USD` was handed to the expression parser as
`0.25 + 0.75 ~ 0.01`, which is not arithmetic, so the whole value was
reported malformed.

Only the currency-less arithmetic form reached that state. With a
currency after the expression the scan stopped there and parsed, which
is why `100.00 ~ 0.01 USD` and `0.25 + 0.75 USD ~ 0.01 USD` both worked
and `0.25 + 0.75 ~ 0.01 USD` did not. `~` now ends the expression, which
is what it means; the tolerance is read separately as it always was.

This is what made `rledger format` emit a balance that no longer parses
(#2191): the formatter emits the single-trailing-currency form, which
is the beancount form and the one the parser already accepted for a
plain number.

Tested for VALUES, not just for parsing. An earlier bug in this same
scan fell back to "take the first NUMBER" and turned `(1 + 5) / 2.1`
into a silent assertion against 1 -- parsed cleanly, asserted the wrong
thing -- so each of the seven accepted forms is checked against the
amount it should evaluate to, with a ledger holding 0.25 confirming that
`0.25 + 0.75` does not match it.

Brings in the matrix-wide format-fidelity harness that #2192 could not
land: it fails on `balance_with_arithmetic_and_tolerance` without this
fix, which is how the bug was found.

Closes #2191

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr

* fix(parser): a price rejects a tolerance instead of discarding it

Review of the commit before this one caught a regression it introduced.
`directive_arithmetic_value` is shared by `balance` and `price`, so
making `~` end the amount expression applied to both -- and a price has
no tolerance. `price USD 1.10 + 0.05 ~ 0.01 EUR` went from a
malformed-value error to silently evaluating 1.15 and dropping the
tolerance on the floor. Trading a diagnosed error for a silent
half-reading of the author's input is a bad trade.

`ToleranceForm` now says which grammar is being parsed: `balance` stops
at `~`, `price` does not.

That restores the old handling of the arithmetic form and leaves the
plain one, `price USD 1.10 ~ 0.01 EUR`, which main ALREADY accepted the
same silent way -- `~` is not an arithmetic operator, so the scan took
the fast path and fell back to the first NUMBER, yielding 1.10 with the
rest discarded. Fixing only the half I broke would leave the identical
silent truncation one keystroke away, so a top-level `~` on a price is
now diagnosed outright.

Both forms are covered, and so are the three tolerance-free price forms
with the values they must evaluate to -- an error path that also stopped
accepting valid input would pass a test that only asserted rejection.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012nNGPA44Dt32NyxWem26xr

* test(format): raise the round-trip floor to what actually round-trips

The fidelity harness guards against going quiet by asserting a minimum
number of fixtures reached the comparison. `> 20` was a third of the way
down from the real figure: 37 of the 42 fixtures round-trip today, the
other 5 being error fixtures, so ten could start failing to parse and be
skipped without the floor noticing. Skipping is exactly how this test
would go quiet.

Measured rather than guessed, and set just under the measured value so
adding fixtures does not churn it.

* test(parser): census which directives keep tags and which refuse them

The failure this session kept turning up is a third state between the
two: accept the tag, then drop it because the model has nowhere to put
it. `note` was in it (#2160), both formatters were in it (#2184), and a
price's tolerance was in it.

So this counts the directives rather than testing one. Seven refuse,
three keep, and each refusal is checked against its tag-free form too --
a rejection that also rejected valid input would satisfy an
assertion that only looked for an error.

`query` is in neither set: it accepts a tag and discards it. That is
asserted here as CURRENT behavior, not as correct behavior. Which way it
should go needs `bean-check` (beancount's `Query` has no tags field,
which argues for refusing; a parser-lima corpus file uses the form,
which argues it appears in the wild) and the container is not built
here. Filed as #2194. Written as an assertion so the day it changes this
fails and the census gets updated rather than quietly going stale.

I implemented the refusal and backed it out: it makes
`parser-lima/examples_data_full.beancount` error, and an unverifiable
strictness increase does not belong in a PR about tolerance parsing.

* chore(cache): bump the versions the tolerance changes invalidate

Loader 32 -> 33, wasm 17 -> 18, pin 33.

Parser OUTPUT moves in both directions here, which is why both caches
have to: a v32/v17 blob holds a parse error where this build now
produces a balance directive, and holds a silently truncated 1.10 where
this build now produces an error. Either way the cache would serve a
reading the binary no longer agrees with.

Both `CostNumber` and `MetaValue` fixture tripwires move version only --
balance and price amounts are `Decimal`s on the directive and take part
in neither archived layout, and the assertions after each comment prove
that rather than take the comment's word for it.

* fix(parser): a child node no longer stops the tilde scan

Review feedback on #2192 was that `emit_header_tags_and_links` bailed
out of its walk at the first child NODE, so it did not scan the whole
header. I fixed that there and then wrote `has_top_level_tilde` with the
identical `break` a few hours later.

Harmless today for the same reason it was harmless there -- a price's
only child node is a META_ENTRY, whose tokens are not direct children,
so nothing here can see a `~` written inside metadata. But a header that
ever gains a child node would stop being scanned at all, and the whole
point of this function is to refuse a tolerance rather than skip past
one.

Also records a blind spot in the format-fidelity test, found while
checking whether the tolerance clause is validated at all (it is not,
#2193): a model comparison cannot see data the model never held. `balance
X 1.00 USD ~ 0.01 EUR` formats to `1.00 ~ 0.01 USD`, erasing the EUR
from the file, and the test passes -- both sides lost it in the parser.
Which is exactly why the note-tag test also asserts on output text.

* test(parser): match the price-tolerance error on its kind, not its Debug

Review feedback: the assertion stringified the whole `ParseError` with
`{e:?}` and searched that. Debug output carries the span and context
fields too, so the test was coupled to formatting it does not care
about.

It now matches `ParseErrorKind::SyntaxError(m)` and searches `m`, which
is the part the test is actually about. Still pins the wording --
rewording the message fails it -- so this narrows what the assertion is
brittle to rather than weakening it.

* docs: record this run's fixes in the changelog

Four PRs shipped user-visible behavior changes without an entry: the
two formatter tag losses (#2192), the blank-line tag drop (#2189), the
same-date pad (#2187), and this branch's tolerance grammar (#2195).

The changelog already carries the direct sibling of the first one --
"`rledger format` no longer deletes a posting's currency, cost or price
when it has no units number" (#2142) -- so the omission was mine, not a
question of whether this class belongs here.

Each entry records the cache versions that moved with it, which is the
part a reader upgrading actually needs and the part not recoverable from
the diff.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
robcohen added a commit that referenced this pull request Aug 30, 2026
…2196)

* ci: report corpus baseline drift on a cadence instead of by accident

The manifest is current -- a fresh fetch of the corpus regenerates to
zero diff, and all 735 files have an entry. #2185/#2189 carried the
drift #2186 describes. What is left is the reason it accumulated.

The issue attributes it to a path filter on the baseline gate. There is
none: `parser-baselines.yml` runs on every PR. The actual mechanism is
two deliberate decisions meeting:

  * A downloaded corpus file with no manifest entry is a WARNING there,
    not a failure, and the comment saying so is right -- upstream can
    add a file at any moment and failing would fire on unrelated PRs.
    But the warning goes to a log nobody reads, so the file sits with no
    parser-output coverage at all.

  * That job restores the corpus from a cache keyed on the FETCH SCRIPT
    and skips the fetch on an exact key hit. CI's corpus is frozen at
    whatever was fetched when the key was created, while the upstream
    repositories it mirrors keep moving. Every PR sees the same stale
    corpus and agrees there is no drift.

Neither is wrong on its own, and tightening either has the cost its
comment already explains. So this adds the missing third thing: a weekly
job that fetches the corpus FRESH -- no cache, which is the entire point
-- and turns drift into an issue. It reports, never gates, and closes
its own issue when the manifest matches again.

The oracle is the repository's own `BASELINE_UPDATE=1` path plus `git
diff`, not a Python reimplementation of the fingerprint, which would be
a second implementation free to disagree with the Rust one in exactly
the cases that matter.

Modeled on `nightly-health.yml`, including the self-test that runs first
and IS allowed to fail the job: the live check exits 0 whatever it
finds, so a broken classifier would otherwise report a permanently
reassuring "no drift" -- the same silence this exists to break, wearing
a green check. Verified by breaking it two ways (rehash detection
disabled, everything reported clean); the self-test catches both.

Refs #2186

* fix(ci): do not report a short fetch as upstream deletions

Review of the commit before this one. Fetching FRESH is what makes this
workflow able to see drift, and it is also what makes it able to invent
some.

The fetch tolerates up to 15 best-effort clone failures. On the PR gate
that is harmless: it restores a cached corpus, so a failed clone still
leaves the previous copy on disk -- `fetch-compat-test-files.sh` only
`mkdir -p`s its destination and never clears it. This job deliberately
skips that cache, so it fetches into an EMPTY checkout, where every
tolerated failure is simply missing files. Regenerating against that
drops their manifest entries, and the report would have called them
"upstream deleted or renamed these" -- a confident lie, longer and more
alarming the more repositories happened to fail.

So the corpus is counted first, against the same 50-file slack the
"Verify corpus is populated" step in `parser-baselines.yml` uses for the
same reason. Short by more than that and the run reports nothing and
says why. A partial fetch cannot manufacture a false "added" or
"rehashed", but it does mean the run saw an unrepresentative corpus, and
silence with a reason beats a plausible wrong answer.

Two more fixes to `--dry-run`, which regenerates and therefore rewrites
the manifest: it now restores the file afterward, because a flag named
dry-run must not leave a modified file for someone to commit by
accident -- and it does NOT restore it when the manifest was already
modified before the run, because `git checkout --` would delete the
caller's edits. A reporting script has no business destroying someone's
work to keep its own promise.

The guard is self-tested in both directions: always-complete (the bug
above) and always-refuse (the over-correction that would silence the
workflow) each fail it.

* docs(baselines): explain why there are two baseline jobs

`tests/baselines/README.md` documents the CI behavior for this manifest
and is where someone goes to understand it, so a second workflow acting
on the same file belongs there. Without it the cadence job reads as
duplicate coverage, and the natural next move is to delete one.

The section names the two reasons the PR gate cannot see this class --
an unmanifested downloaded file warns rather than fails, and the corpus
cache is keyed on the fetch script rather than on upstream state -- so
the distinction survives past whoever remembers the incident.

Also two guards on the script: it refuses to run outside the repository
root, where its relative paths and its `git checkout --` would either
fail confusingly or resolve somewhere unintended; and the comment about
always exiting 0 now says what that covers. Drift is reported rather
than gated, but `regenerate()` runs under `check=True`, so a corpus file
that panics the parser still fails the step -- that is a real problem on
main and nightly-health picks it up from there.

* fix(ci): a gh failure must not read as "nothing to report"

`gh()` returned an empty string when the command failed, which is
indistinguishable from a successful empty result. For a job whose only
output is an issue, that conflation is the worst bug available:

  * a failed `issue list` looks like "no existing issue", so the next
    run opens a duplicate, every week;
  * a failed `issue create` yields a blank URL, prints "opened ", and
    leaves the workflow GREEN having reported nothing at all.

The second is exactly the silence this workflow exists to break, wearing
a green check -- the same failure `nightly-health.yml` was written for,
reproduced inside its sibling.

`gh()` now raises `GhError`, and `main` turns that into a non-zero exit.
Finding drift stays exit 0, because a red X for a successful report
would be one more failing scheduled workflow for nobody to notice;
failing to report is the opposite case. A job that cannot say what it
found must not look like a job that found nothing.

Self-tested in both directions by stubbing `subprocess.run`: returning
"" on failure fails the test, and so does raising on a successful empty
result, which is a real and different answer.

Also makes the two list-element string concatenations explicit --
CodeQL's `py/implicit-string-concatenation-in-list`, flagged in review.
Adjacent literals on consecutive lines inside a list read as a missing
comma, and in a list of issue-body paragraphs that is a plausible typo
rather than a pedantic one.

* test(ci): execute the part of the drift reporter that does the work

`_report` decides between opening, updating and closing the issue -- it
is the only code in the script that changes anything -- and nothing had
ever run it. `--dry-run` returns before reaching it, and a scheduled run
exercises exactly one branch per week, so three of its four paths would
have shipped unexecuted and the fourth would have been tried in
production.

All four now run against a recording stub: drift with no open issue
creates, drift with an open issue edits THAT issue number, clean with an
open issue comments then closes in that order, and clean with no issue
touches nothing at all.

Each is verified to fail for its own reason -- swapping edit for create,
dropping the close, and passing drift to the no-op case each break
exactly one assertion. The last one matters most: a `_report` that
opened an issue every clean week would still have passed a test that
only checked the drift paths.

Stdout is captured while they run. Left alone the stub prints "opened
https://github.com/x/y/issues/1" into a self-test log, which is the sort
of line someone skims and believes.

* docs(ci): record why this guard is stricter than the test's own

The baseline test already refuses to write a manifest from fewer than
`MIN_FULL_CORPUS_SIZE` (100) files, so a partial fetch cannot silently
produce a truncated manifest and a false "no drift" -- checked, and it
panics rather than skipping when `BASELINE_UPDATE` is set.

That backstop and this check are not redundant, and the note says why:
at a corpus of ~735 this one trips at ~685, so a merely partial fetch
gets a sentence explaining itself instead of a cargo stack trace, and
the test's assert stays what it was written to be -- a last resort
against truncating the committed manifest, not the routine path.

Someone reading either guard alone would reasonably conclude the other
is dead weight.

* fix(ci): say which hash moved instead of telling the reader to go look

The "Changed hashes" section told the reader "the diff says which: a
changed source hash is upstream churn; a changed output hash with an
unchanged source hash is a parser change that reached main without
regenerating" -- and then printed a list of paths and no diff. It knew
the answer and sent the reader to find it.

`classify` already had both sides of every changed line, so it now
compares the source hashes and splits the group in two. They are not a
matter of emphasis:

  * source hash moved   -> the file's contents changed upstream, a
                           different output hash is expected, routine.
  * source hash IDENTICAL, output hash moved -> this repository produces
                           different output for the same bytes than the
                           committed baseline records. That is a parser
                           change that reached main unregenerated, and it
                           is the reason this job exists.

Collapsed into one list they look the same and read as churn, which is
how the second kind gets skimmed past.

Self-tested in both directions: forcing every rehash to read as upstream
fails, and so does forcing every one to read as a parser change. The
second control is the one that matters -- a split that always cries
"parser change" would be noise nobody acts on within a month.

* style(ci): explicit string concatenation, and say why the except is empty

Two CodeQL findings from review, both in code this branch added.

`py/implicit-string-concatenation-in-list`: nine runs of adjacent string
literals now join with an explicit `+`. Wrapping them in parentheses was
not enough -- adjacency inside parentheses is still adjacency, and the
concern is real for these in particular, since they are paragraphs of an
issue body where a missing comma would silently merge two of them rather
than raise.

Verified the rewrite changed nothing: rendering the same diff through
the pre-rewrite and post-rewrite modules gives a byte-identical body
(sha e0194f7e3082a81a both sides).

`py/empty-except`: the self-test's `except GhError: pass` is the
assertion -- reaching that arm is the pass condition and the
`failures.append` above it is what runs otherwise -- but nothing said
so, and a bare `pass` under an `except` is worth explaining every time.
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.

2 participants