fix(556): gate schema-echo dicts at the persistence boundary#803
Merged
Conversation
Schema-echo dicts reach target_data verbatim via the carry-forward/resume path (no LLM call, so the in-flight _reject_schema_echo_items guard never runs). Asserts write_target replaces the namespace with the parse-error sentinel and records FAILED/PARSE_ERROR — across a varied action name so a hardcoded-name gate cannot pass.
A resume/carry-forward record can reach write_target with its action namespace set to the compiled JSON Schema (Ollama format shape) instead of LLM output — the LLM was never invoked, so the in-flight _reject_schema_echo_items guard never ran and the schema dict was persisted verbatim to target_data. StorageBackend.write_target now runs _gate_schema_echo_deltas after delta extraction and before _write_target_raw: a namespace matching is_schema_echo is replaced with the make_schema_echo_error sentinel and the record gets a FAILED/PARSE_ERROR disposition. Backend-agnostic (lives on the abstract base). Deviations from the spec: set_disposition now clears prior dispositions for (action, record), so the gate's FAILED replaces a stale SUCCESS rather than coexisting; test path is tests/unit/storage/ (spec said tests/storage/); the real-DB before-count is now 0 (the 11 rows from 2026-06-17 were cleaned) — the gate remains a forward-looking invariant (RED proves the hole was open).
Owner
Author
Real-project verification (added)Ran the gate against a copy of the actual qanalabs Confirms the gate replaces the namespace and records FAILED/PARSE_ERROR end-to-end against the real project DB schema. Run on a copy; the project DB was not mutated. Note: the spec's original before-check (11 rows on 2026-06-17) now returns 0 on the live DB — those rows were cleaned since. The gate is a forward-looking invariant (RED proves |
Post-review cleanup (no behavior change): - Collect the FAILED/PARSE_ERROR rows and write them via set_dispositions_batch (one transaction) instead of a per-record set_disposition call. Same DELETE-then-INSERT semantics, so a stale prior SUCCESS is still cleared. - Move the record.reasons / utils.schema_echo imports to the module top-level; neither imports storage, so there is no cycle to defer around, and it matches the file's import convention.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
StorageBackend.write_targetnow refuses to persist a record whose action namespace is the compiled JSON Schema (the Ollamaformatshape:{"title","type":"object","properties",...}) instead of LLM output. A resume / carry-forward record reacheswrite_targetwithout an LLM call, so the in-flight_reject_schema_echo_itemsguard never runs and the schema dict was written verbatim intotarget_data.The new
_gate_schema_echo_deltasruns after delta extraction and before_write_target_raw: a namespace matchingis_schema_echois replaced with themake_schema_echo_errorsentinel and the record gets aFAILED/PARSE_ERRORdisposition, so the failure surfaces on the next read exactly like the live-LLM parse-error path. It lives on the abstract base, so every backend inherits the invariant. Reusesis_schema_echo/make_schema_echo_error/PARSE_ERRORas-is — no new constants, no collector change.Verification
main(echo persisted verbatim, no disposition), including a varied-action-name test so a hardcoded-name gate can't pass.ruff+mypyclean.force_full+ delta branches), guards mirror_extract_delta, replacement reaches_write_target_raw,NotImplementedErrorcaught.RLockis re-entrant (callingset_dispositionfrom insidewrite_targetis deadlock-safe); everywrite_targetcaller checked for false-positives (none carrytitle+type:object+properties); no SUCCESS-write race.test_gate_fires_for_any_action_name(recorded in RED). Real SQLite round-trip, non-tautological assertions, collector untouched.scan-projectcanary divergence is pre-existing (baseline.shconfirms it fails onmaintoo; stale project action registry, unrelated to storage). The gate is a no-op on clean data; the realwrite_target→_write_target_raw→_read_target_rawround trip + disposition writes are covered by the 7 unit tests.Deviations from the spec (verify-first)
set_dispositionclears priors. The spec assumed a prior SUCCESS and the gate's FAILED would coexist (UNIQUE(action,record,disposition)). Currentset_disposition(sqlite_backend.py)DELETEs all prior dispositions for(action, record)first, so the gate's FAILED replaces a stale SUCCESS — the record ends unambiguously failed (better). Test asserts the real behavior.tests/unit/storage/(spec saidtests/storage/).write_targetstill persists a schema-echo namespace verbatim onmain, i.e. the hole is open.Out of scope (follow-up)
The upstream planter that stamps
compile_unified_schemaoutput as record content on the resume/checkpoint path — suspect sitesprocessing/strategies/online_llm.py:_checkpoint_recordandllm/batch/services/processing_recovery.py. This gate is the safety net regardless of which path is wrong.