feat(planner): accept every read plan against its checked query (RFC 0047)

[ragnorc]

RFC 0047 step 2a: every read plan is accepted against its checked query before it runs.

Builds on #815 (step 2, planner refusal diagnostics); its two commits appear here until it merges.

What changes

One acceptance door. omnigraph-planner/src/validate/ constructs the only executable plan type, AcceptedPlan, whose fields are private. The ordinary run, Session::explain_query, the explain statement and the inspected run all call accept_query / accept_query_explained, which share one traced planning run (gate::plan_traced), so explain validates exactly what a run validates. bind takes an AcceptedPlan; execute takes an AcceptedBoundPlan. A fresh plan failing a check is a planner defect (500); an exhausted validation limit is ResourceLimitExceeded (413); a refusal by design keeps its diagnostic (400).

Requirements from the checked query, not the IR. The compiled-query cache keeps the CheckedQuery (declaration and type context) beside the IR. The validator derives binding identity, search identity and approximation, eligibility conjuncts (checked per rrf arm), correlated blocks, the projection and score origin, order and cut, and the declared search policies, and relates written expressions to the IR through the compiler's lowering patterns (constant folding is checked against the engine's own evaluator). Order is recomputed from the operators over typed OrderKeys; a Sort's declared ordering is now its whole comparator.

Exact fragment. A single-binding query with Boolean/integer/String eligibility, an optional leading bm25() over a String property, property sort keys, a limit and a projection of properties and the score is a member. The optimizer records the rules it applies (absorb_scan_filter, prune_scan_columns, lower, rank_bm25_scan); the validator checks the IR is the declaration's lowering, rebuilds the canonical chain, re-applies every rule against its precondition and requires the plan to match. Members are accepted as exact_subset, everything else as invariants_only; explain reports it as validation.scope, and GQT claims it with validation scope <scope>.

Declared search policy. A nearest scan declares its NearestPolicy (probe factor, flat rescan of unreached rows, uncapped fail-closed rescan, flat start); every pre-pass declares on_empty and coverage_admits. The ladder and both gates read them from the plan (the rrf gate's coverage guard is now the recorded fact, not a run-time dataset read). The report and the profile record each gate verdict with its counts and each probe attempt per rung.

Replay through acceptance. Session::replay_bound_plan takes a serialized ReplayEnvelope (query source and name, scope, schema digest, bound plan, a member's derivation), checks its byte limit and versions before decoding, re-establishes every recorded full-text coverage fact from the pinned snapshot, recompiles the query, and accepts the plan again. Version mismatch, another schema and an unpinned dataset are conflicts (409); an edited plan, derivation or coverage claim is invalid evidence (400).

Behavior changes

• BM25 scores on partially indexed data no longer depend on filters. Lance scores fragments no full-text segment covers flat from statistics over the rows its prefilter admits. A bm25 scan now filters before scoring only under the recorded full coverage of its property, and otherwise runs Lance's postfilter. Regression cases/v2/bm25_score_ignores_a_filter_over_an_unindexed_fragment.gqt (the old prefilter scored the tail row 0.41299206 under a filter, 0.3903352 without), Lance guard fts_prefilter_changes_unindexed_scores_and_postfilter_keeps_them.
• Ties order by declared bindings first. The implicit tie-break put anonymous endpoints (__anon_*) before named bindings, contrary to RFC 0047's comparator; the validator refused that plan. Regression cases/v2/anonymous_endpoint_ties_order_by_declared_bindings.gqt.
• explain gains validation, a bm25 scan's eligibility, a nearest scan's policy and the assumptions' full_text coverage; saved plans move to bound_plan_version 2.
• A replay whose dataset pins no longer match is a conflict (was an internal error), and a plan accepted under an older schema is not replayed under a newer one: wildcard_replay_keeps_captured_members_and_pins_every_member_issue_659 now asserts that refusal.

RFC 0047 amendments

Recorded in the RFC's text and decision log: the canonical form is the planner's resolve order; the derivation stores rule applications, not successors; the rule catalogue as built; BM25 eligibility before scoring only under full coverage; three tie-break omissions as comparator equivalences; correlated blocks required as blocks; evidence memory bounded by byte and node limits (no planning pool exists); the fusion and search-ordered aggregate order checks land with step 5, which implements that order.

Verification

• cargo test --workspace --exclude omnigraph-gqt --exclude omnigraph-dst --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints: 122 test targets green (run before the last review fix; the planner suite, the replay owners and clippy pass after it)
• cargo test -p omnigraph-gqt --locked: 238 of 239 cases pass locally; issue_782_multi_hop_limit_stops_early hits its 10 s budget under local machine load (it takes 8.33 s in main's own CI, so it is near its budget there too)
• cargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro, cargo fmt --all --check, scripts/check-docs.py, scripts/check-agents-md.sh, typos: clean
• Planner validator tests (validate/tests.rs): one broken plan per check, membership over admitted and excluded forms, derivation rejections, entry parity under an exhausted limit; derivation_cost_grows_with_the_query (instrument:) prints evidence growth: 256 conjuncts take about 58 KB, 261 rule applications and 11 ms.
• Codex review (GPT-6-Astra) of the branch found two defects, both reproduced and fixed with regressions: an order key bound to a return item that lowers equally but is written differently failed as a planner defect (cases/v2/order_key_bound_to_an_equally_lowered_return_item.gqt), and a replay trusted a forged full-text coverage claim (a_replay_rechecks_recorded_full_text_coverage). A second review of the fix found nothing.
• A third Codex review (GPT-6-Astra, against main 445f50f) found two more, both fixed with regressions. A constant order key that return projects under an alias (return { $x as x } order { $x }, also now() and a single edge type's grouped @type) failed acceptance as a planner defect, because the planner binds the key to the alias and the order check did not resolve the alias before discounting constants; it now resolves an alias to the return item it names on the written side and on the planned side alike, so order { x } and order { $x } are judged the same way (cases/v2/constant_order_key_bound_to_a_return_alias.gqt, validate/tests.rs::a_constant_key_bound_to_a_return_alias_is_accepted). And a fusion returned without recording its nearest arms' probe attempts, so inspection and replay could not see them; every nearest scan now reports under its own plan node and a fusion records each arm's attempts after its gate (engine_v2_plan_replay.rs::a_fusion_replays, extended with a two-arm rrf(nearest, nearest)).

[ragnorc]

Recommendation: request changes before merge. I found two reproducible gaps in the new acceptance checks. This is a COMMENT review because the authenticated account is also the PR author.

This PR adds a check between a user's query and the plan that runs it. It aims to catch translation errors before they produce wrong answers. Ordinary queries, explain, and saved-plan replay use the same acceptance path. A small set of single-table queries gets checked rewrite evidence. Other queries get narrower checks, reported as invariants_only. The PR also keeps BM25 scores independent of filters on unindexed rows and puts named bindings first in implicit tie breaks.

Two findings need changes:

1. P2 — An unwritten row limit passes acceptance. A saved count_people plan with an added Limit(0) replays successfully. It returns zero rows instead of the required count row. The inline comment identifies the missing check.
2. P2 — The checker retains quadratic copies of predicate data. A 526,559-byte query retained 68,717,622 bytes of string literals in its derivation arena. Acceptance still succeeded within every default counter. The inline comment includes the fixture and a control.

The design has a sound starting point: an accepted wrapper closes the execution boundary, and requirements come from the checked declaration. That can catch an error shared by later planning stages. It adds protection by construction. The BM25 change also addresses a cause. In pinned Lance 11, fts_search_source uses an empty filter plan for postfilter search. plan_flat_match_query otherwise filters input before flat scoring. The new Lance surface guard passed locally. See Lance scanner and OmniGraph's scan adapter.

The concrete tradeoffs are:

• Assurance costs planning work on every read. Compiled queries retain the checked declaration, but acceptance and derivation reconstruction still run per request. This matters most for frequent small reads. I did not measure production latency or throughput.
• Stable BM25 scores can require more work. Appends leave rows outside index coverage. Postfiltering scores those rows before removing ineligible results, and loses scalar-index filtering at that stage. This favors correct scores over selective-read speed. Full coverage permits prefiltering again.
• Replay favors a fixed contract over portability. Schema and dataset checks reject stale evidence. Format, rule, and semantics versions require future compatibility decisions. This is a hidden Rust replay API, not a new HTTP endpoint.
• The stronger guarantee has a narrow scope. Traversal, aggregation, ANN, and RRF remain outside the exact subset. Fusion and search-aggregate ordering checks are explicitly deferred in RFC 0047. These limits must remain visible when more rules land.

My liability assessment is mixed. The shared acceptance path, typed ordering, and plan-owned search policy remove places where execution and explain can disagree. But this change adds 7,719 lines and removes 627 across 62 files. It adds another account of lowering semantics, a rule catalogue, retained evidence, and versioned replay obligations. The two findings show real costs of that new responsibility. Five similar extensions should compose through the same small rule set and bounded evidence store. Separate copies of each query shape's semantics would increase long-term liability. Fixing the retained-node issue with existing arena slots would reduce that liability without another abstraction.

Validation on ec8b43a25b7a14ea78312693182413d49cbca19b:

• Local clean baseline: planner suite, 121 passed and one ignored instrument.
• Local engine baseline: engine_v2, engine_v2_plan_replay, engine_v2_scrubbed_replay, and lance_surface_guards: 103 passed and two ignored tests.
• Temporary assertions in existing owners reproduced the accepted extra cut at both planner and replay boundaries. A focused rejection check made the replay assertion pass.
• The existing derivation instrument measured retained string data. Releasing obsolete arena nodes reduced it to 524,562 bytes for the same query. This is retained payload, not a process-RSS measurement.
• Exact-head CI reports successful Test Workspace and GQ Logic Tests. I did not rerun the full workspace or cloud suites locally.

I restored all temporary source edits. The restored planner suite passed all 121 tests, and the restored replay owner passed all 24 tests. The checkout is clean. The review leaves the PR unchanged.

[ragnorc]

[P2] Reject row cuts that the query does not require

This branch checks a limit only when the query declares one. For an unordered invariants_only query without a limit, an added PhysicalNode::Limit passes acceptance. I reproduced this with the existing count_people replay fixture: add Limit { input: old_root, rows: 0 }, copy the root properties, and select the new root. replay_bound_plan succeeds with zero rows. The original query returns one count row. A planner assertion using a contains predicate also confirms acceptance. This violates the stated row-cut check, even within the narrower validation scope. Reject cuts that the query scope does not justify, and check their placement. Please extend the existing row-cut and replay owners. A temporary check rejecting a limit when the query has none made the replay regression pass. This is a validation/replay gap, not evidence that the current optimizer emits this plan.

[ragnorc]

Done in 9f37eca. A read plan now carries exactly the query's row cut: one Limit at the root when the query writes limit, and no Limit or Page anywhere when it does not, a correlated block's tree included. validate/tests.rs::an_unwritten_row_cut_fails_the_row_cut covers an added cut over a query without limit and an extra cut below a written one, and engine_v2_plan_replay.rs::an_unwritten_row_cut_refuses_the_replay_of_a_count is your count_people case: the saved plan with an added Limit(0) is now refused instead of replaying as no rows.

[ragnorc]

[P2] Release replaced derivation nodes before retaining successors

Each AbsorbScanFilter clones the remaining filter and the growing scan filter. These lines retain both predecessors after replacing their current references. For N conjuncts, the arena therefore retains O(N²) predicate data. Arena::node already refuses references to these obsolete nodes, so they cannot support a later rule. I extended the existing derivation instrument with 128 distinct $d.title != "<index><4096 x characters>" clauses. The 526,559-byte query passes acceptance with 261 charged nodes, 133 steps, and 17,832 visits. Yet its arena retains 68,717,622 bytes of literal strings alone. The serialized derivation is only 553,713 bytes. These copies occur before the execution memory pool exists, on every ordinary query acceptance. The server's 1 MiB request limit admits this query size. A control that clears replaced arena slots reduces retained literals to 524,562 bytes. Release those nodes or share immutable expressions, and charge retained bytes before allocation. The current counters do not provide the stated evidence-memory bound.

[ragnorc]

Done in 9f37eca. A replaced node is now released before its successor is stored, so the arena keeps only each role's current node and its expressions partition the query's own. validate/tests.rs::the_derivation_arena_keeps_only_current_nodes absorbs 64 conjuncts one at a time and requires the arena to end with at most six nodes; it fails without the release. I did not add a separate retained-bytes counter: with the release, retained data is linear in the compiled query (bounded by the request limits), the node limit bounds slots and the visit charge bounds the copying work. RFC 0047's statement that the byte and node limits bound evidence memory is replaced by that bound, with a decision-log entry, and execution.md states it.

[ragnorc]

Recommendation: request changes for the remaining P2 row-cut gap in the inline comment. The memory finding is fixed. I reviewed exact head 9f37eca717a122882b842630583f524594a65f9d, including the changes since my review of ec8b43a2.

This PR checks that a physical read plan matches the user's checked query before execution. A small set of single-table queries gets checked rewrite evidence. Other queries get narrower checks, reported as invariants_only. The follow-up addresses two failures in that protection: saved plans could add a row limit, and the evidence checker kept obsolete copies of predicates. It now counts explicit limits and releases replaced arena nodes. The memory change works. The row-cut change still misses limits stored in sort nodes.

The remaining defect has an observable result. The existing liked traversal query returns three rows. A saved plan with an extra Sort { fetch: Some(0) } below its final sort passes acceptance and returns zero rows. The new check rejects equivalent Limit and Page nodes, including those inside correlated blocks. It must also check capped sorts. I did not observe the optimizer generate this bad shape. This is a gap in the acceptance and replay contract.

The arena change fixes the memory cause. Old references were already invalid. Clearing their slots now removes the data that those references could no longer use. With 128 large predicates, the same 526,559-byte query retains 524,562 bytes of literal strings, compared with 68,717,622 without the release. The arena holds four live nodes. Measurements at 32 and 64 predicates also follow linear retained growth.

For frequent small reads, acceptance adds work before useful execution. For larger generated predicates, retained copies and concurrent requests become more important. The check runs in the shared engine, so embedded, file, S3, and Azure use need the same contract. These fixes change planner bookkeeping. They need no new storage mechanism.

The tradeoffs and liability are explicit:

• The memory fix reduces retained data by design. It keeps existing slots, role ownership, and stale-reference checks. It adds no cache, memory manager, or production API.
• Each absorption still copies expressions. Linear retained memory does not imply linear total copying work. The measurements cover literal payload, not peak RSS or production latency. The corrected RFC makes the bound relative to compiled query size.
• The row-cut check adds a charged walk over plan nodes. Its cost follows plan size, not stored row count. Strict placement limits future optimizer freedom until a new rewrite has appropriate evidence.
• The corrective commit adds 147 net lines, mostly tests and documentation. It removes substantial memory liability despite the positive line count. The remaining liability is incomplete coverage of row-reducing fields. Five similar extensions should share the same acceptance rules and arena ownership rules, with one regression owner per contract.

Validation:

• Clean local baseline: 123 planner tests passed, with one ignored instrument. The four engine targets passed 104 tests, with two ignored tests. These include replay, scrubbed replay, and the Lance surface guards.
• Temporary probes confirmed that extra Limit and Page nodes fail both at the root and inside a correlated block. The capped-sort replay returned the wrong rows. A focused rejection control refused that replay.
• Disabling the arena release reproduced the 68,717,622-byte retained payload and failed the new retained-node regression. I restored all temporary edits. The restored planner and replay suites passed, and the checkout is clean.
• Formatting, AGENTS links, and documentation checks passed locally. Workspace CI passed on merge commit 13154c7f7dea2087c508cafaccf137a552e21afd, which includes this head. Its log records all three new regressions as passed.
• The complete upstream search documentation and pinned Lance 11 implementation remain consistent with the earlier review. The dependency pin and scan adapter did not change. The reproduced cut occurs in OmniGraph's own sort operator.

The merge CI documentation check still fails because its release-note snapshot is stale. The exact-head documentation check passes locally. Refresh that merge snapshot before merge. I did not rerun the full workspace, HTTP, or cloud suites locally, or measure production performance.

[ragnorc]

[P2] Include capped sorts in the row-cut check

This count misses PhysicalNode::Sort { fetch: Some(_), .. }. The later check compares only the final sort's fetch, so another capped sort below it can still remove required rows.

I reproduced this in the existing replay owner with PEOPLE_QUERIES::liked. The query traverses Likes, orders by person and document, and has no limit. It returns three rows. Insert a sort with fetch: Some(0) beneath its existing final sort, preserve the input properties, and replay the envelope. Acceptance succeeds under invariants_only, and replay returns zero rows. A temporary control that rejects capped sorts when the query has no limit makes the same replay fail with row cut.

Please validate every capped sort and its position, including additional sorts below a legitimate final sort. Keep the final sort's valid top-k optimization when the query requires it. Extend the existing planner and replay regressions with this shape. This is an acceptance/replay defect, not evidence that the current optimizer generates the bad plan.

[ragnorc]

Fixed in 3426d7c, by design rather than by adding Sort to the count.

PhysicalNode::row_cut() now names each node's count cut in one match over every variant: a Limit's or Page's rows, a Sort's fetch, a RankFuse's limit, and a ranked scan's candidate cap. A node kind that gains a cut cannot compile without declaring it there.

check_row_cuts runs right after the search check. It walks every live node and accepts a declared cut only where the query requires it:

• the root Limit at the query's limit;
• the final sort's fetch at the query's limit (the final sort is the one below the root Limit and its projections, so its top-k stays);
• the fusion at that limit;
• a ranked scan's cap, which check_access already matches to its declared policy.

It refuses every other cut as row cut, including a capped sort below the final one and any cut in a correlated block's tree. It replaces the earlier Limit/Page count.

Regressions:

• validate/tests.rs::an_unwritten_row_cut_fails_the_row_cut now also splices a Sort { fetch: Some(0) } below the final sort of an ordered query and of a limited query. It requires the planned plan to be accepted and the spliced one refused with row cut.
• engine_v2_plan_replay.rs::an_unwritten_row_cut_refuses_the_replay covers your liked shape (three rows, capped sort below the final sort) beside the count's root Limit(0), and asserts that liked takes the sort arm.

The planner suite, the replay, engine_v2, search and query owners, the GQT corpus, clippy and docs all pass. RFC 0047's decision log and execution.md now describe declared cuts.

[ragnorc]

Recommendation: approve. The remaining P2 capped-sort defect is fixed. I found no new blocking defect. The inline P3 wording note is optional. This is a COMMENT review because the authenticated account is also the PR author.

I reviewed 3426d7cbb8b0eebf86c14ff4b81eb4f7534801d8, including the changes since my previous review.

This PR checks a read plan against the user's query before execution. That protects users from translation errors and invalid saved plans. The earlier check missed limits stored inside sort nodes. A saved traversal plan could pass acceptance, then return no rows instead of three. This fix gives each physical node one place to declare its row cap. The checker then checks the cap's size and position against the query. It rejects the hidden sort and preserves the final sort's valid top-k optimization.

The node classification covers Limit, Page, Sort, RankFuse, and ranked Scan. The shared check replaces the incomplete count of Limit and Page nodes. Existing search checks still own candidate caps. Existing order checks still require the query's root limit. This addresses the cause across node kinds.

For frequent small reads, acceptance costs planning work before useful execution. For larger scans, a misplaced cap can discard required answers. Embedded and server execution need the same contract on file, S3, and Azure storage. This correction changes shared plan acceptance and requires no new storage behavior.

The tradeoffs and liability are:

• Correctness before optimizer freedom. Only justified positions may carry a cap. Future rewrites that move caps need matching evidence and acceptance rules.
• Work follows plan size. The checker replaces a charged node walk with classification and placement checks. It does not scan stored rows. I did not measure production latency or throughput.
• One classification reduces omissions. The fix adds a Rust helper and extends existing regression owners. It adds no durable state, cache, execution primitive, or wire format. The follow-up adds 216 lines and removes 58 across six files. Despite that increase, it reduces maintenance liability by replacing an incomplete rule with a shared classification. Five similar extensions should use this classification and the same acceptance boundary.
• The guarantee remains bounded. invariants_only still provides selected checks, not full plan equivalence. ANN candidate caps retain their declared search semantics. The broader PR's evidence and replay maintenance costs from the earlier review remain.

Validation:

• Exact-head local baseline: 123 planner tests passed, with one ignored instrument. engine_v2, engine_v2_plan_replay, and search passed all 64 tests.
• I temporarily restored only the old validator, keeping the new tests. The replay regression failed because liked still accepted the hidden capped sort. The planner regression also failed: its exact-subset check caught the added sort, but the required shared row cut check did not.
• I restored the exact reviewed source. Both focused regressions passed again. The checkout is clean. Formatting, whitespace, AGENTS links, and documentation checks passed locally.
• Workspace CI passed on merge commit 04a0813, which includes this head. Its log records both updated regressions as passed. GQ Logic Tests and Clippy also passed in CI.
• I checked the relevant upstream documentation and the pinned Lance 11 scanner. The dependency pin and scan adapter are unchanged. Lance's query limit and nearest-candidate count remain distinct from OmniGraph's local sort cap.

I did not rerun the full workspace or cloud suites locally. The earlier memory fix remains unchanged. This follow-up closes the remaining reproduced acceptance defect.

[ragnorc]

[P3] Narrow the compile-time guarantee to new variants

Optional: say that adding a new node variant requires an explicit classification here. The exhaustive match gives that guarantee. It does not force a change when an existing variant gains a field: patterns such as Self::Expand { .. } still compile and return None. The same stronger claim appears in RFC 0047's new decision entry. Narrowing the wording keeps future reviewers aware that changes to existing operators still need a cut audit. The current listed cuts are handled correctly.

[ragnorc]

Narrowed in d76469b: the doc comment on row_cut() and RFC 0047's decision-log entry now say the exhaustive match forces a classification only for a new node variant, and that a field giving an existing variant a cut still needs its arm audited by hand.

[ragnorc]

Recommendation: approve at d76469b511d821648a690da575ef5357ebd5d67d. The optional wording finding is addressed in both places. No new blocking defect or optional change to request. This is a COMMENT review because the authenticated account owns the PR.

This PR checks a read plan against the checked query before execution, including when a saved plan is replayed. Its row-cap check prevents an unjustified limit from silently removing answers. This follow-up corrects what the compiler guarantees when developers extend that check. It changes documentation, not query results.

The helper’s comment and RFC decision entry now match the actual mechanism. The exhaustive match requires an arm for a new PhysicalNode variant. A field added to an existing variant can still match { .. }. A developer must therefore audit that arm when the field adds a row cap. Correcting both claims removes the false assurance at its source.

The shared validator still checks every declared cap against its allowed size and position. The existing planner and replay regressions still cover the hidden capped sort. The Lance 11.0.0 pin, scan adapter, callers, and deployment boundaries are unchanged from the previous review.

Liability is lower: maintainers now see the manual obligation instead of relying on a guarantee Rust does not provide. The change adds no API, state, invariant, abstraction, or test fixture. Five similar operator changes can use the same classification and acceptance check. They still require an audit of any new fields. For ordinary reads and saved-plan replay, the existing correctness-versus-planning-cost tradeoff is unchanged. This revision makes no performance improvement claim.

Validation on the clean isolated head:

• Compared against parent 3426d7cbb8b0eebf86c14ff4b81eb4f7534801d8. Only the two passages above differ. physical.rs is identical after excluding doc-comment lines. All executable code, tests, and dependencies are unchanged.
• /tmp/review-916-docs/bin/python scripts/check-docs.py passed for 207 Markdown files, using the existing documentation environment.
• bash scripts/check-agents-md.sh, cargo +1.97.1 fmt --all --check, and git diff --check HEAD^ passed.

GQT cannot observe a documentation guarantee or inject the malformed saved plan used by the existing replay regression. No new behavioral regression is appropriate for this wording-only revision. The previous review records the runtime defect’s failure proof and passing Rust owners at the parent. Those are retained evidence, not fresh test runs at this head.

The current head reports successful PR Title and Fix Regression Gate checks. I found no fresh workspace or GQT CI result for this head, and did not rerun those suites locally. No source edits were retained. No new storage or performance claim was tested.