feat: frontmatter resilience: degrade cleanly and surface parse reasons#998
Merged
Conversation
Member
Author
|
Ships with engram-app/Engram-obsidian#239 (the plugin consumer). Merge together. |
Rasbandit
added
the
phase/expand
Rasbandit
force-pushed
the
feat/frontmatter-resilience
branch
from
July 13, 2026 08:49
b90c414 to
f140e16
Compare
encode_values/1 now returns {values, bad_keys} instead of {:ok, map} | :error.
Every encodable key is kept; a value (or an exotic non-binary map key from
YAML like date:YYYY-MM-DD) that Jason cannot encode is collected into bad_keys
rather than raising or aborting the whole batch. safe_encode/1 runs deep_sort
inside its rescue so a deep_sort guard raise on a non-binary key is trapped too.
parse/1's call site is updated minimally to keep its existing {:ok,...} | :error
contract (any bad key means :error); Task 2 rewrites parse/1 to surface bad_keys.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
parse/1 now returns {:ok, order, values, degraded} | :error: keys that
fail to JSON-encode are dropped from order/values and reported in
degraded as %{key, line, snippet} instead of collapsing the whole
block to :error. :error is reserved for non-map YAML.
OkfFields.extract/1 and CrdtBridge.ingest_plaintext/2 + normalize_doc/1
updated to the 4-tuple, ignoring degraded for now (raw passthrough is
a later task).
A flow-style complex mapping key (e.g. `[a, b]:`) parses to a non-binary map key. degraded_entry/2 called Regex.escape/1 on it, which raises FunctionClauseError, breaking the TOTAL guarantee. Guard non-binary keys to report the inspected form with no source line.
Preserve a degraded frontmatter key (valid YAML map key whose value cannot be JSON-encoded) as a raw-passthrough marker in the CRDT Y.Map at its source position, so emit/2 re-renders it verbatim instead of dropping it on the next Y.Map emit. Good keys stay structured; the degraded key round-trips byte for byte (single-line and multi-line spans). Frontmatter.parse_for_ingest/1 captures each degraded key's full source span by column-0 line tiling, and only when that tiling is a bijection with the parsed map keys (else :error, and the bridge falls back to whole-text-as-body, also lossless). parse/1 is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
A good, encodable frontmatter value can JSON-encode to a map containing the
__engram_raw__ marker key (e.g. {"__engram_raw__":"x"}), which emit/2's
raw_from_marker/1 mistook for a passthrough marker and rendered as its inner
raw, silently DROPPING the key and any siblings. The exact single-key collision
is byte-identical to a real marker, so no marker format can disambiguate.
Fix at ingest: any good value raw_from_marker/1 would falsely match is routed
through the same verbatim span-passthrough as degraded keys, so it round-trips
byte-identical regardless. Falls back to whole-text-as-body when a span can't
be captured losslessly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
Adds Frontmatter.reason_for/1 (per-key degrade -> frontmatter_unparseable_key) and Frontmatter.invalid_yaml_reason/1 (whole-block malformed YAML -> frontmatter_invalid_yaml), and a shared put_parse_status/2 helper in Notes wired into every content-persisting path: insert_new_note, move_note, do_rewrite_note (shared by single-note update and batch update), and build_batch_insert_row. Also adds parse_status/parse_reason to @batch_insert_columns so the raw insert_all row actually carries them. Status is derived from the CRDT-merged text (same input inject_okf_fields already uses), not the raw incoming push, and is recomputed from scratch on every write so a clean re-write of a previously degraded note resets parse_status to ok / parse_reason to nil.
process_batch_entry wraps its body in process_batch_entry_rescued/3, a thunk-taking rescue boundary: any future raise inside per-entry processing (parser/crypto/CRDT) degrades that one entry to a note_processing_failed error result instead of propagating out of Repo.with_tenant and rolling back every sibling in the batch. Logged at error with the note path (Sentry-visible via prod's metadata: :all JSON formatter). Since Task 1 made frontmatter parsing total, there's no longer a natural input that raises in this path, so the isolation is proven via process_batch_entry_rescued/3 directly: a raising thunk degrades only its own entry while a sibling's already-accumulated row/result is untouched. process_batch_entry_rescued/3 is @doc false but public for exactly this reason (no test-only branch in prod code).
Review follow-ups (no mechanism rework): - Rewrite process_batch_entry_rescued/3 comment: it isolates pre-DB-write raises per entry (covers every raise reachable today); a future failed-SQL raise in the UPDATE branch would poison the shared tx and needs per-entry SAVEPOINTs/deferred writes (out of scope, not built). Drop the blanket "any raise, siblings commit" claim. - Interpolate Exception.message(e) into the log message string; :error is not in the Sentry LoggerHandler metadata allowlist, so the reason never reached Sentry from metadata alone. - Relabel the Task 5 regression test (not raise-isolation).
Threads Task 5/6's parse_status/parse_reason columns through the three client-facing serializers: POST /api/notes/batch per-note results, the single note read (note_json), and each /sync/changes entry (change_json). Also widens the shared @note_meta_fields select-list so the fields=meta sync path (metadata-only pull) carries the two fields too, and adds them to the corresponding OpenApiSpex response schemas.
Narrow invalid_yaml_reason/1's @SPEC to its actual literal return shape (was map(), dialyzer flagged contract_supertype). decode_value/1's non-binary catch-all triggers pattern_match_cov because every lib/ call site happens to route values through our own JSON-string encoding, but the clause is load-bearing defense against a Y.Map value written directly over the Yjs wire protocol by a buggy/hostile peer, bypassing our encode path entirely; deleting it breaks two existing frontmatter tests, so it's suppressed with a documented ignore entry instead (matching the crypto.ex/key_provider.ex precedent for the same class of issue). Also drops the now-stale clerk_strategy.ex ignore entry that no longer matches any warning.
A pathologically long single-line frontmatter value stored the whole line in parse_reason jsonb and re-echoed it on every /sync/changes fetch. Truncate the diagnostic snippet only; raw passthrough spans used for lossless round-trip are untouched.
…dact parse_reason
Addresses a high-effort review of the frontmatter-resilience branch.
MUST-FIX 1 (data loss): normalize_doc lifted a legacy fence via parse/1, which
now DROPS degraded keys, then stripped the whole fence -> degraded keys in a
legacy body fence were permanently lost. Route normalize through the same
lossless parse_for_ingest machinery; bail (leave doc as-is) when a block can't
be captured losslessly.
MUST-FIX 2 (data corruption): emit_key interpreted any value decoding to a map
containing __engram_raw__ as a passthrough marker and dropped the key. This
fired on the PROJECTION path with client-controlled Y.Map values that never
passed the ingest guard. Eliminate the in-band marker: store degraded keys'
verbatim spans out of band in a dedicated "frontmatter_raw" Y.Map, consulted by
emit/3 + project/4. A normal value is never mistaken for a marker. Removed
raw_marker/raw_from_marker/@raw_key. parse_for_ingest now returns
{:ok, order, values, raws}.
MUST-FIX 3 (plaintext content leak): degraded_entry/invalid_yaml_reason put the
raw frontmatter source line (full key: value) into snippet, persisted PLAINTEXT
in parse_reason and echoed on /sync/changes. snippet now carries only the KEY
("date:") or a generic "<frontmatter>" marker; detail.key/line and the generic
message stay. The lossless raw span is captured separately, so fidelity is
unaffected.
FOLD IN 6: left the put_parse_status double-parse (tiny block, threading would
couple OKF extraction to parse-status); added a ponytail comment noting it.
ADD TEST 5: lock OKF resilience (a good OKF key survives a degraded sibling).
Dialyzer: the decode_value pattern_match_cov ignore became stale (emit_key now
calls decode_value with a term() value, so the catch-all is genuinely
reachable); pruned it. The clause itself stays.
Full gate green: format, credo --strict (no issues), dialyzer (0 unnecessary
skips), mix test 3706 pass (seed 0).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
The batch upsert runs every entry inside one Repo.with_tenant tx. The per-entry rescue caught a raise but could not contain a raise that came from a FAILED SQL statement in the UPDATE branch (next_seq!'s UPDATE ... RETURNING, or Repo.update): Postgres flips the whole tx into 25P02 in_failed_sql_transaction, so every subsequent entry's SQL fails too and the tx rolls back at COMMIT while the client already got a 200. A bare nested Repo.transaction does NOT recover a failed raw query (DBConnection disconnects). Only mode: :savepoint on the failing operation rolls back to a per-statement savepoint and keeps the outer tx healthy. So thread db_mode: :savepoint through the batch UPDATE path (update_batch_entry -> do_update_note -> do_rewrite_note -> next_seq!/Repo.update). The single-note path passes no db_mode and is unchanged. Rewrote the honest-scope comment to match. Test drives the real batch path end-to-end: one entry's next_seq! hits a genuine failed statement (bigint overflow, SQLSTATE 22003) while a sibling idempotent re-push commits and the batch returns real partial success. RED (fix off) poisons the tx and raises 25P02 at COMMIT. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
Code-review finding on #4 (savepoint isolation): 7 comment/docstring lines added by the savepoint work used em dashes, violating the repo's no-em-dash convention. Cosmetic only; savepoint semantics unchanged (verified RED/GREEN against a real 25P02 in review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
The savepoint db_mode threading (b8fd26b) added `opts \\ []` to both, but every caller passes opts explicitly, so the defaults are dead and trip mix compile --warnings-as-errors. mix test compiles without that flag, which is why 3707/0 stayed green; the pre-push gate caught it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
The new frontmatter parse fields on note/batch/change responses changed the generated OpenAPI spec; the unit-tests staleness gate requires the committed openapi.json to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
Rasbandit
force-pushed
the
feat/frontmatter-resilience
branch
from
July 13, 2026 19:01
f140e16 to
6b2af13
Compare
Two concurrent xdist workers each drive their own Obsidian instance doing live Clerk auth + CRDT joins. That parallelism races identity-rebind and auth timing, tipping a different clerk test almost every run (#355, e.g. test_84 crdtJoinFailedReason=unauthorized). Cross-suite serialization fixed contention between suites but not this within-suite churn. Default E2E_WORKERS to 1 (override =2 to restore). crdt stays at its default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
Even single-worker + suite-serialized, e2e's 30s live-delivery budgets get starved when unit-tests (~6min) and n1-compat (~3min) run concurrently on the shared runner VM (#355 — e.g. test_34 materialized=no, test_edit_after_discovery). Cross-suite serialization only orders e2e suites against each other, not against these heavy non-e2e jobs. Gate e2e-clerk + e2e-crdt (the chain heads) on unit-tests + n1-compat so the whole e2e chain runs after them, on a quiet VM. Costs wall-clock; buys a reliably green e2e. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
E2E_WORKERS=1 was added to quiet the #355 clerk churn, but the actual clerk flake (test_84 unauthorized) is fixed at the source by the plugin identity-rebind guards (#240) and passes at -n 2. Single-worker instead broke test_34's folder-rename mint-resurrection race (shared-instance ordering under --dist=loadfile), materialized=no on two runs. The -n 2 + quiet-VM-gate config is what the plugin-fix validations proved green (95/95). Back to -n 2. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PZq4ryBTf4rGPKrqPwkeCX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Clobbered frontmatter now degrades cleanly instead of 500ing the sync pipeline, and the server surfaces why a note's frontmatter failed to parse so clients can show it.
Why
Prod incident: a note with exotic YAML frontmatter (yamerl key tuples/charlists that
Jason.encoderaises on) 500'd the wholePOST /api/notes/batch, which the plugin then retried forever. Root cause:Frontmatter.encode_values/1letJason.encoderaise uncaught, taking down the batch.Changes (8 tasks, subagent-driven + reviewed)
encode_values/1is now total and lenient: returns{values, bad_keys}, never raises.parse/1returns{:ok, order, values, degraded}; degraded keys carry%{key, line, snippet}with the snippet redacted to key-only (<=200 chars), so no plaintext frontmatter leaks into the reason.frontmatter_rawY.Map (CRDT), so they round-trip verbatim through emit/project instead of being dropped.parse_status+parse_reasoncolumns (expand-phase migration). Plaintext on purpose: they describe our parser's behavior, not note content, so no encryption/HMAC.okf_fieldsextraction made lenient (a degraded sibling no longer drops good keys).Review
Built-in code review surfaced 3 must-fixes beyond the per-task gates (normalize_doc data loss, emit projection collision, plaintext leak in the reason). All fixed. SAVEPOINT isolation independently verified RED/GREEN against a real 25P02.
Ships with
The plugin PR (engram-app/Engram-obsidian) consumes
parse_status/parse_reason+ thefrontmatter_rawmap. Merge together — the producer here is inert-to-harmful without the consumer.Test
mix test3707/0; dialyzer clean;mix compile --warnings-as-errorsclean.🤖 Generated with Claude Code