feat(planner): refusals by design carry planner diagnostics

[ragnorc]

What & why

Issue #786: on engine v2, the one query shape the planner refuses by design, a nearest() or bm25() ranking on a binding that a traversal reaches, came back as an internal error (HTTP 500) with a fix that did not answer the query. #795 has since made it a bad request. Two things were still missing:

• It carried no diagnostic, so the HTTP body and the CLI had no code, stage or fix, unlike every parse and type refusal since compiler: the diagnostics contract for refused queries (RFC 0047, PR 1) #759.
• Its fix told the caller to order on the traversal's source binding or to use search(). Neither ranks the rows the traversal reaches.

This is RFC 0047's rollout step 2 (#791).

Fixes #786.

The change

• The planner refusal becomes PlanError::Unsupported(Box<QueryDiagnostic>), using a new diagnostic kind, plan, and the first planner code, P001. The stage is plan and the expression is the refused call, for example nearest($t.embedding, $q). The fix is declare `$t` first in `match`, so the ranking starts the traversal. I checked that this fix answers the query: declaring the ranked binding first makes it the component's scan root, which v2 ranks, and a probe returned the correct row.
• The gate's Unrouted::UnsupportedQuery carries the diagnostic, and plan_source returns it as OmniError::Compiler on both the ordinary and the inspected (explain) path. The server answers 400 with the diagnostic object, and the CLI prints error[P001] with the stage and the fix, through compiler: the diagnostics contract for refused queries (RFC 0047, PR 1) #759's existing paths.
• The one-line text reads plan error: P001: …, next to parse error: … and type error: T33: ….
• Before the planning that runs, load_column_statistics resolves and rewrites the query once to find which column statistics the cost model needs. It sent every error through no_plan, so a refusal raised while resolving came back as an internal error (HTTP 500) with the diagnostic only as text. It now uses the same conversion as the gate's result. Found by checking step 2a's assumptions against the code.
• The edge-selection refusals that GQ: add edge alternation and wildcard traversal #827 added (edge alternation and wildcard traversal) use the same path, through one constructor, PlanError::refused: P002 when there is no finite traversal work limit (fix: set traversal_work_limit), P003 when the limit is out of range or recorded twice, and P004 for CSR traversal mode. Their message text, and their classification as refusals by design, are unchanged, so the existing assertions hold. Ordinary queries rarely reach them: the setting has a finite default, and CSR mode is set only by the test harness.
• The diagnostics page in the user guide lists the P… codes, and a changelog fragment, changelog.d/planner-refusal-diagnostic.fixed.md, carries the release note.

Once RFC 0047's step 4 makes the ranked binding the root of its traversal, this refusal goes away.

Tests

• engine_v2_plan_replay.rs a_search_order_on_a_traversal_destination_is_a_typed_bad_request_issue_786, extended from the old error-kind check. The ordinary path (query) and the inspected path (query_inspected) both return the diagnostic: code P001, stage plan, the refused expression and the fix. It failed before the change, because the ordinary path returned a plain bad request with no diagnostic.
• The same test adds an edge wildcard under CSR traversal mode on the ordinary door, which must return P004 as a diagnostic. It returned an internal error before the statistics-pass fix.
• rrf_prefilter_gate.rs asserts code P001 instead of the error kind.
• query_plan/edge_selections.rs issue_659_selection_admission_precedes_lowering_of_outer_named_edges asserts each edge-selection refusal's code beside its text.
• v2/planner/search_order_on_an_unranked_binding_and_a_destination.gqt expects the new text.
• Compiler: a display test for the plan kind, and the code catalogue test admits the P namespace.

Checklist

• Fixes an issue: A planner refusal by design is returned as an internal error (HTTP 500) #786
• Implements RFC 0047 (rfc: RFC 0047, search plan truth on engine v2 #791), rollout step 2
• Reviewed against docs/dev/invariants.md: a typed diagnostic rather than a string (9), a loud refusal (8), no engine v1 change

[ragnorc]

Recommendation: request changes for the P2 repair-loop defect in the inline comment. Reviewed head ccfe79864ee6062d369a59be2e58f42eb6c9cc67.

This PR helps people and agents repair queries that the planner cannot execute. It gives four existing refusal classes stable codes, a planning stage, and repair advice. The engine preserves that information through ordinary queries, inspected queries, and the earlier statistics pass. The existing HTTP and CLI handlers then display the same diagnostic. Callers can identify a query problem without interpreting an internal-error message.

The main design is sound. The error conversion preserves intended refusals while keeping unresolved names and planner defects internal. It reuses QueryDiagnostic, its code catalog, and the existing transport fields. It adds no storage state, publication path, cache, or backend-specific behavior.

For interactive graph and search queries, a useful refusal is part of the observable contract. The same engine serves embedded use and the cluster HTTP server. File, S3, and Azure storage do not need separate error rules. In agent-driven workloads, incorrect repair advice has a direct cost: repeated requests that cannot succeed.

The tradeoffs are concrete:

• Stable codes help callers handle errors, but their meanings become a compatibility obligation. Rust callers that match the old Unsupported { detail } variant must also change.
• Declaring a single ranked binding first uses the existing reverse traversal. It keeps the requested ranking. Starting there can cost more when the other end has a selective key filter. Nearest search can require wider scans. This PR makes no measured speed improvement.
• The new hint depends on the whole query. RRF combines two ranked searches. It can rank both ends of one component, so moving either binding first cannot repair that shape. The inline finding asks for truthful refusal text, without expanding this PR into a planner rewrite.

On long-term liability, the net addition is 186 lines. Most of that cost buys one shared diagnostic mechanism and focused coverage. Five similar changes can add catalog entries and reuse the same path, without five new error frameworks. The remaining liability is unconditional repair advice tied to compiler root selection. Change that advice before merge. The PR preserves the error class by design, but it does not add support for the refused query shapes.

Validation and limits:

• Local, exact head: the original issue-786 regression passed. All 20 other tests in engine_v2_plan_replay passed using the probe build, with the modified test excluded. Formatting and the documentation check passed.
• Temporary assertions in that existing owner reproduced both opposite P001 hints. Separate nearest and BM25 controls each returned d01 after the proposed rewrite. The assertion against contradictory repair advice failed as expected. I restored all temporary source edits. The checkout is clean.
• Workspace CI passed on merge commit 7176e7baca9538fef67bcc31be15084f272d8314, which includes this exact PR head. Its log records the issue-786, RRF refusal, and edge-selection admission tests as passed. This is merge-commit evidence, separate from the local head checks.
• Code inspection covered the compiler root choice, both planning paths, API conversion, and CLI rendering. I also checked the exact Lance 11 scanner implementation and the relevant full upstream search/index guides. Lance owns per-table nearest and full-text scans. OmniGraph owns this graph-plan refusal and its repair advice.

I did not run fresh HTTP, S3, or Azure tests, or a performance benchmark. No additional blocking finding emerged from the code inspection. An optional improvement is a P001 case in the existing transport tests to preserve the complete error envelope.

[ragnorc]

[P2] Do not offer a declaration-order fix for connected RRF arms

The new fix can send a caller through an endless repair loop. With Doc.text: String @index and Knows: Doc -> Doc, use:

query both() {
    match { $d: Doc $t: Doc $d knows $t }
    return { $d.slug, $t.slug }
    order { rrf(bm25($d.text, "needle"), bm25($t.text, "needle")) }
    limit 1
}

P001 says to declare $t first. After that edit, P001 says to declare $d first. Both RRF arms copy the same traversal plan. Only one binding can start this connected component. Thus, neither order satisfies both arms.

Please omit this fix when the ranked bindings cannot all start their components. Explain that this RRF shape is unsupported. Keep the useful reorder hint for the single-binding case. Extend an existing test to exercise both declaration orders. This does not require the automatic root selection planned for RFC 0047 step 4.

[ragnorc]

Done in 8df5788. An rrf() whose arms rank two bindings that one traversal connects is now refused before either arm is ranked, with a new code P005 that names both bindings and the fused call and offers no fix; the reorder hint stays for a single ranked binding. a_search_order_on_a_traversal_destination_is_a_typed_bad_request_issue_786 runs your query in both declaration orders and requires the same fixless P005 each time. The diagnostics table and the release note describe it. I did not add the optional transport-level P001 case.

[ragnorc]

Recommendation: approve the code change. This change fixes the repair-loop defect from my earlier review. I found no new blocking defect. This follow-up covers ccfe7986..8df57880, at exact head 8df5788072382c110c0922ba7376ee60d3b07b70. The merge CI documentation check still needs attention, as described below.

This PR gives callers useful errors when the planner cannot run a query. The follow-up corrects the advice for reciprocal rank fusion (rrf), which combines two search rankings. Previously, ranking both ends of a traversal could produce an endless repair loop: declare one binding first, then declare the other first. Now the planner returns P005, names the combined ranking, and gives no false repair instruction. A query that ranks one binding still gets the useful P001 reorder hint. This changes error reporting. It does not add support for ranking both connected bindings.

The fix addresses the cause. The compiler chooses one scan root per connected component. Both RRF arms use copies of that traversal plan. The new guard checks connectivity before ranking either arm. It handles chains and excludes traversals inside a negated match. Thus, the diagnostic describes the whole unsupported shape instead of suggesting a local edit that cannot work.

For interactive and agent-driven graph search, truthful repair advice is part of the observable contract. A false hint wastes requests and can prevent completion. The same planner serves embedded use and the HTTP server, across file, S3, and Azure storage. This correction needs no separate backend rule.

The tradeoffs and liability are concrete:

• P005 gives callers a stable distinction, but adds a diagnostic code whose meaning must remain stable.
• The guard adds a temporary walk over the physical plan. It adds no durable state, cache, storage format, or execution primitive. Its cost depends on query structure, not stored row count. I measured no performance improvement.
• This follow-up adds 97 net lines, including tests and documentation. It removes the repair-loop liability but adds a connectivity check to maintain. The check derives its answer from existing Expand nodes and reuses the scope-aware walk. It stores no second source of truth.
• Five similar refusal changes should reuse the diagnostic catalog and error conversion. They should not create five separate connectivity models or error frameworks. Future changes to traversal lowering must keep this check aligned with which bindings can start a ranked scan.

Validation:

• Local, exact head: all 21 engine_v2_plan_replay tests and all 12 rrf_prefilter_gate tests passed. Formatting, AGENTS links, and documentation checks passed.
• Temporary assertions in the existing issue-786 test covered two-hop paths in both declaration orders, two destinations from one source, and mixed nearest/BM25 arms. Ordinary and inspected queries returned P005 without a fix. Independent roots, a negated connection, same-binding fusion, and a reordered single ranking each returned one row.
• With the old optimizer and the new regression, the test failed: it received P001 where it required P005. After restoring the exact head, the regression passed. I restored all temporary source edits. The checkout is clean.
• Workspace CI passed on merge commit de9a651000040675d58c74a99e20d265bb62bd87, which includes this head. Its log records the issue-786 regression as passed. This is separate from the local head results.
• I checked the complete relevant upstream search guides and the pinned Lance 11 scanner. Lance owns per-dataset search. OmniGraph owns graph connectivity, fusion, and this refusal. I also checked the existing engine-to-HTTP diagnostic conversion.

The merge CI documentation check failed because its generated release-note snapshot is stale. The exact-head documentation check passes locally. Refresh the snapshot for the merge result and obtain a green check before merge. I did not run fresh HTTP, S3, or Azure tests locally, or benchmark large query plans.

Optional coverage improvement: retain the chain and negated-scope cases in the existing test owner. They protect the new connectivity walk beyond the direct-edge regression.

[ragnorc]

Recommendation: approve this follow-up. The documentation-gate issue noted in my previous review is resolved. This is a COMMENT review because the authenticated account owns the PR.

This follow-up moves the release-note configuration from the already published v0.12.0 release to the next unreleased version, v0.12.1. It also moves the note-selection base to the v0.12.0 tag. New notes therefore belong to the next release instead of being compared with a frozen published snapshot. It does not alter planner behavior or regenerate published release notes.

I compared the current head 5569283223dc3132afd6c97abcbe700be34fea4c with the previously reviewed 8df5788072382c110c0922ba7376ee60d3b07b70. The intervening merge matches Git's automatic merge tree. The following commit changes only release.json. The planner optimizer, its existing engine regression owner, and the release-note checker are unchanged from the previous review. This is a review of the follow-up delta, not a new claim that all inherited main changes were independently retested.

The fix addresses the cause at the existing configuration owner. Note selection derives the next release's notes from the selected base and refuses changes to published notes. The checker still verifies frozen snapshots; no special-case bypass was added. I confirmed that v0.12.0 is a published, non-draft release and its tag is an ancestor of this head.

Tradeoff and liability: release preparation still requires advancing one explicit configuration after publication. That is a small existing maintenance obligation. This removes the stale-release selection without adding an API, persistent engine state, enforcement path or duplicated release ledger. Five more releases can use the same configuration and checker. This delta has no Lance storage or deployment behavior change and makes no performance claim.

Local before/after proof used the existing documentation checker: /tmp/review-900-0328-docs/bin/python scripts/check-docs.py. Parent 1763c9490b79699831caaa561824e8534eaf5f0e failed with release notes changed after snapshot generation; regenerate it (exit 1). Exact reviewed head passed all 204 Markdown files (exit 0), including a rerun after returning from the parent. AGENTS links and git diff --check HEAD^ HEAD also passed. GQT cannot express release configuration or generated-document validation, so a query test would not guard this fix. No temporary source or test edits were needed; the isolated checkout is clean.

Separately, exact-head CI passed, including workspace, Clippy, RustFS, Azurite and storage-upgrade checks. GQT and DST also passed. I did not rerun Rust, HTTP or cloud suites locally for this configuration-only delta. Earlier planner validation remains the evidence recorded in the prior review; I do not present it as a fresh run.

No new blocking finding or optional request. I rechecked that the PR is open at this head and that no newer substantive review duplicates this follow-up.

[ragnorc]

Recommendation: include the minimal P005 GQT regression before merge. The code passes the focused checks. I found no new runtime defect. The inline comment identifies one required test addition.

Reviewed head: 29a21deb21b7b0be448d1dfa89893010933ca796. This follow-up checks the main merge, its conflict resolutions, and the restored ManifestErrorKind import. Earlier review findings remain resolved.

This PR gives callers structured errors when the planner cannot execute a valid query. Codes, stages, expressions, and useful repair hints pass through the existing diagnostic path. For RRF, which combines two search rankings, connected ranked bindings receive P005 without contradictory reorder advice. This helps interactive clients and agents avoid repeated repairs that cannot succeed. It does not make that query shape executable.

The design fixes the cause at the planner. The compiler selects one root per connected component. The RRF guard checks connectivity before ranking either arm. Its walk follows chains and excludes negated inner scopes. The merge retains this rule with main's boxed arms. It also preserves main's asynchronous planning and original source-error propagation alongside the diagnostic conversion.

Tradeoffs and liability: stable P-codes add a compatibility obligation. The connectivity walk adds work based on query structure. It derives connectivity from the existing plan and adds no persistent state, cache, storage format, or execution primitive. The merge adds no second error system. Five similar refusals should reuse the same catalog and conversion. The remaining maintenance duty is to keep the refusal aligned with supported traversal lowering. I made no performance claim.

The shared engine carries this behavior into embedded and served queries. HTTP conversion preserves a compiler diagnostic in a 400 response. Lance owns per-dataset retrieval, while OmniGraph owns traversal roots and fusion. I checked the relevant upstream search documentation and the exact Lance 11.0.0 scanner against its Cargo.lock-verified package archive. Its source commit is ab6b5bbe46009ed78746b444df8db59a8bc5d842. This follow-up adds no storage-backend or deployment rule.

Local validation:

• Exact head: 33 engine_v2_plan_replay tests and 12 rrf_prefilter_gate tests passed. Command: cargo test --locked -p omnigraph-engine --features failpoints --test engine_v2_plan_replay --test rrf_prefilter_gate -j3.
• The existing GQT case passed. A temporary copy added two P005 queries, with opposite declaration orders, and reused its complete schema and seed. No additional fixture data was needed.
• Before: base db59f0846ac47a4163bd833fb9e87984cb52fd18 failed that case at step 1 in 1.82s. It returned the old traversal-destination error instead of P005. Compilation and fixture setup succeeded.
• After: exact head 29a21deb21b7b0be448d1dfa89893010933ca796 passed the same case in 1.82s. Returning from the base and rebuilding the head passed again in 1.84s.
• Formatting, diff checks, AGENTS links, and all 237 Markdown checks passed. The isolated checkout is clean. No source or test changes were pushed.

The before run used this command. The two head runs changed only the artifact directory to gqt-head and gqt-head-restored:

env RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target \
  RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
  cargo run --locked -p omnigraph-gqt --features omnigraph/failpoints \
  --bin omnigraph-gqt -j3 -- /tmp/review-815-merge/rrf.gqt \
  --target omnigraph-engine --storage local-filesystem \
  --artifacts /tmp/review-815-merge/gqt-before

GQT can check this public error code. The existing Rust owner should retain the structured fix == None assertion, which the GQT error matcher does not express.

Separately, CI for this head passed, including 4,066 workspace tests with 17 skipped. GQT, DST, Clippy, and configured storage checks were green. The workspace job executed merge commit fa8630c970afae8f1679925bfcb0eca6da4aa52e, whose tree differs from the reviewed head. These are merge-build results, separate from the exact-head local checks. I did not rerun the full workspace, HTTP, S3, or Azure suites locally.

No additional optional request. I rechecked the open state, head, reviews, and inline comments before posting.

[ragnorc]

[P2] Retain the connected-RRF refusal in the GQT corpus

Please insert these two queries after the seed, before the existing query steps. The P005 fix has Rust coverage, but its public error is still absent from GQT. The current case only covers P001. Both declaration orders matter because the original advice sent callers back and forth between them.

This adds 22 lines and uses the existing schema and seed unchanged. I ran the same extended case on base db59f0846ac47a4163bd833fb9e87984cb52fd18 and head 29a21deb21b7b0be448d1dfa89893010933ca796. The base failed at the first new query with the old traversal-destination error instead of P005. The head passed both orders. Keep the existing Rust assertions for the structured diagnostic and absent repair hint.

--- query
query connected_rrf_d_first($q: Vector(4)) {
    match { $d: Doc $t: Doc $p: Person $p knows $d $p knows $t }
    return { $d.slug, $t.slug }
    order { rrf(nearest($d.embedding, $q), nearest($t.embedding, $q)) }
    limit 1
}
--- params
{"q": [0.0, 0.0, 0.0, 0.0]}
--- expect error: plan error: P005:

--- query
query connected_rrf_t_first($q: Vector(4)) {
    match { $t: Doc $d: Doc $p: Person $p knows $d $p knows $t }
    return { $d.slug, $t.slug }
    order { rrf(nearest($d.embedding, $q), nearest($t.embedding, $q)) }
    limit 1
}
--- params
{"q": [0.0, 0.0, 0.0, 0.0]}
--- expect error: plan error: P005: