Skip to content

fix(knowledge): expose honest search score provenance - #2317

Closed
ohdearquant wants to merge 5 commits into
mainfrom
codex/knowledge-search-score-provenance
Closed

fix(knowledge): expose honest search score provenance#2317
ohdearquant wants to merge 5 commits into
mainfrom
codex/knowledge-search-score-provenance

Conversation

@ohdearquant

@ohdearquant ohdearquant commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • keep a genuine FTS miss empty instead of ranking the newest eligible corpus rows as a lexical source
  • distinguish matched, filtered, no-match, and timeout lexical outcomes while preserving existing fail-open timeout and ANN behavior
  • carry lexical/ANN source attribution through RRF fusion and successful embedding reranks, then expose additive candidate and per-result score provenance
  • retire the stale 0.46/0.42 absolute score bands in the pack README, API guide, ADR-047, and the live verb vocabulary

Contract

knowledge.search now returns top-level candidate_provenance with the lexical candidate state and whether the returned set is ANN fallback. Each result adds score_provenance with stable lexical/ANN sources, embedding-rerank use, s_over_s_plus_1 normalization, and calibrated: false.

Scores remain useful for response-local ordering and min_score, but they are request-relative hybrid ranking values rather than probabilities or absolute corpus-presence signals. knowledge.suggest keeps its existing result shape so its strict knowledge.fold handoff remains compatible.

Verification

  • cargo test -p khive-pack-knowledge (403 passed, 1 ignored)
  • cargo check --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --check --all
  • git diff --check

This PR is independent of the search-concurrency and query-cache branches.

Closes #1982

Keep genuine lexical misses empty instead of ranking newest corpus rows. Track lexical and ANN score sources, expose candidate and score provenance, and retire stale absolute score bands across the public contract docs.
@ohdearquant

Copy link
Copy Markdown
Owner Author

The behaviour change is scoped correctly and the provenance is derived rather than asserted, which are the two things most likely to be wrong in a change of this shape. One test gap is worth closing before merge; one stale comment is worth fixing on the way past.

The set difference is the right one

Before this diff, fetch_fts_candidates ran bounded per-term FTS, then an unfiltered raw-FTS existence probe, and only if that probe also came back empty did it run a bounded scan of non-deleted, status/type-eligible atoms ordered created_at DESC, slug (search.rs:551-621 at the merge base). Those rows were scored and returned exactly like lexical hits, with nothing in the response marking them as fallback.

The new path keeps the raw probe, but only to distinguish "matched then filtered to empty" from "matched nothing" (search.rs:674-713). So the behaviour difference is precisely: a non-timeout true FTS miss with at least one eligible corpus row. That set previously received newest-corpus rows presented as matches; it now receives an empty lexical set, or ANN-only results when the dense leg is healthy.

That is the correct scope, and the two conditions that would have made a partial fix are both handled: a raw match whose rows are all ineligible does not take the miss path, and a timeout does not either.

One correction to the framing

The wire contract is five-valued, not four: matched, filtered, no_match, partial_timeout, timed_out (search.rs:461-507). partial_timeout is a real state — some decomposed stage completed before the deadline — and it is carried in both the ADR text and the API reference, so the documents are complete. Timeout states dominate the decomposed merge (:485-506), so a timeout cannot surface as no_match or filtered. Given that conflating a timeout with an empty result is a defect this codebase has hit before, that ordering is the load-bearing part and it holds.

Filtered-to-empty and matched-nothing are distinguishable through candidate_provenance.lexical, so the new vocabulary earns its place rather than decorating. Worth noting for callers that a final total: 0 still has causes outside the lexical outcome — min_score removing a matched set, or ANN hydration failures, the latter surfaced separately via degraded.hydration_failures. The documents promise an empty lexical set on a genuine miss, not that every empty response means no match, and that narrower promise is what the code delivers.

Provenance is computed, not inferred from context

Lexical hits carry lexical provenance at construction (:839-853), ANN shells carry ANN provenance (:1291-1305), and fusion merges both flags for a shared id (:252-260) — so a result contributed by both legs is honestly ["lexical", "ann"] rather than taking the label of whichever stage ran last. Rerank happens after fusion and sets embedding_rerank=true (:2091-2101), which correctly describes the score actually being reported rather than the one it replaced. This is the item most likely to be silently wrong in this class of change, and it is right.

Additivity holds: existing result keys and results/total keep their names and shapes, and score arithmetic is unchanged for queries that reach the existing paths. Values change only for the intended true-miss set difference.

[Medium] No end-to-end coverage of the fused-plus-reranked case

rrf_fusion_preserves_per_hit_score_sources_and_ann_fallback (:3465-3495) exercises the internal sources() helper after fuse_ann_hits. It does not assert the handler's serialized output, and it does not cover a result that survives a rerank. Since the claim is specifically about the source of the reported score, and since a single-source result is labelled correctly under almost any implementation, helper-level and single-source coverage cannot detect the failure that matters: a fused or reranked result carrying a label that no longer describes its score.

A regression in the caller-visible projection, in the fused-source merge, or in the rerank flag would pass the current suite. Suggested shape: one handler-level test with an id contributed by both legs and a successful fake embedding rerank, asserting score_provenance.sources == ["lexical", "ann"] and embedding_rerank == true in the emitted JSON, plus an ANN-only result staying ["ann"] through the same rerank.

lexical_candidate_state_distinguishes_filtered_match (:3263-3301) has the same shape of gap — it covers the raw-probe distinction at the helper layer, not the public response — though that one matters less because true_lexical_miss_does_not_return_newest_rows (:3180-3260) does reach the public JSON.

[Low] Two comments still describe the removed fallback

tests/smoke_knowledge.py:466-471 and crates/khive-pack-knowledge/src/knowledge/ann_degrade_tests.rs:309-310 describe the public search path as falling back to a full scan. The assertions are still valid; the comments now teach the old contract, which makes a future failure harder to read. Worth updating in this diff while the context is fresh.

One ordering note

The implementation fetches ANN before lexical (:2018-2024, and the suggest path at :2215-2219). The provenance merge itself is source-derived and order-independent, but the fail-open guarantee — ANN results surviving a lexical timeout — does depend on that order. Worth a comment at the call site, since it currently reads as incidental sequencing and is not.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Approving the change. Not merging yet — CI has never run on this PR, and I am not
merging 426 lines of retrieval logic on a reading. Details at the bottom.

The fix is the right one

The defect being removed is real and it is the interesting kind. On a genuine FTS miss the
old path fell back to a bounded scan ordered by a.created_at DESC and handed those rows
to reciprocal-rank fusion as a lexical rank source. Corpus recency is not query evidence,
so a zero-overlap query came back with confident-looking ranked results drawn from
whatever happened to be newest. That is worse than an empty result, because the failure is
invisible at the call site: the caller cannot distinguish "nothing matched" from "these
matched".

Deleting that fallback while keeping the raw-FTS existence probe is the right shape — it
preserves the distinction between a true miss and a match removed by kind/status
eligibility, which is exactly the information a caller needs to act differently.

Retiring the score bands is overdue and correctly justified

Retiring the 0.46 / 0.42 bands is the part I would have asked for if it were not here.
The ADR amendment gives the actual reason rather than just deleting them: the bands
predated the s / (s + 1) squash, so they had been describing a scale that no longer
existed. A stale numeric threshold is worse than none, because it reads as calibration and
gets applied as a filter.

calibrated: false in score_provenance is the right thing to state explicitly. Scores
here are request-relative ranking values; publishing that as a field rather than as prose
means a consumer that treats them as cross-query comparable is contradicting a value it
can read, not just a doc it did not read.

Tests

Three tests, one per behavioural claim, and the important one is not vacuous — I checked,
because an absence-assertion is exactly where a test passes for the wrong reason.
true_lexical_miss_does_not_return_newest_rows seeds newest-unrelated at created_at 999, which is precisely the row the deleted fallback would have surfaced, then queries a
nonsense term and asserts both that the result is empty and that the state is NoMatch.
The fixture contains the bait, so the test genuinely reddens against the old behaviour
rather than passing on an empty corpus.

I ran the suite locally at this head: cargo test -p khive-pack-knowledge → 255 passed,
0 failed across six binaries, including all three new tests by name.

Treat that as supporting evidence, not as a substitute for CI: my local toolchain is
1.98.0 while CI pins 1.95.0, so a local pass is not a CI pass.

Why this is not merged

No CI has run on this PR at all. The only workflow run on the branch is the auto-merge
enabler (pull_request_target, skipped); every pull_request-event workflow is missing.
For comparison, another branch opened the same day has the full battery.

This is not a filter: the CI trigger is a bare on: pull_request: with no branches and
no paths, and its own comment warns about precisely this failure mode leaving a PR
mergeStateStatus clean with zero checks. So the runs were simply never created.

I am flipping this out of draft to fire a ready_for_review event and get the battery to
run. That is safe here — the auto-merge workflow is scoped to bot-authored PRs and gated
behind a repo variable, so it will not arm on this one.

Merge waits on a green wall.

@ohdearquant
ohdearquant marked this pull request as ready for review August 30, 2026 12:29
@ohdearquant ohdearquant reopened this Aug 30, 2026
@ohdearquant

Copy link
Copy Markdown
Owner Author

Correcting my previous note: marking this ready did not get CI to run, and neither did
a close/reopen. The reason is more useful than the trigger theory I gave.

This branch conflicts with main, in one file: docs/adr/ADR-047-knowledge-pack.md. I
confirmed it independently of the API message with a git merge-tree against current
main, which reports a single content conflict in that path and nothing else. Both sides
amended the same ADR, which is the expected collision — the amendment list and status line
sit at the top of the file where every amendment lands.

The branch is 9 commits behind main and 1 ahead. Six of the paths this PR touches or
neighbours have moved on main since its base, including vocab.rs and
docs/guide/api-reference.md, so the merge is worth doing on its own merits rather than
only to clear the conflict.

Resolving that conflict and pushing will also fix the CI situation as a side effect, since
the resulting push is a synchronize event — and synchronize demonstrably does create
runs on this repo right now, which is what I could not achieve with ready_for_review or
reopened.

So the sequence is: merge main in, resolve the ADR-047 amendment collision, push. Nothing
in my review of the code changes — the fix and its tests still look right to me, and the
local suite passed at this head. This is purely about getting a green wall in front of it.

@ohdearquant

Copy link
Copy Markdown
Owner Author

This is blocked on a merge conflict, and it explains the missing CI too. Details below so the
resolution is mechanical rather than a re-derivation.

The conflict

One file: docs/adr/ADR-047-knowledge-pack.md. The branch is 10 commits behind main, and the
collision is entirely in the header region — both sides appended to the same place.

  • main (via fix(knowledge): support explicit-null atom patches #2304) bumped the status line to ... 2026-08-06, 2026-08-29 and inserted
    ## Amendment (2026-08-29): tri-state atom upsert patches.
  • This branch bumped the same status line to ... 2026-08-06, 2026-08-30 and inserted
    ## Amendment (2026-08-30): honest lexical fallback and score provenance.

Correct resolution is to keep both: the status line becomes
... 2026-08-06, 2026-08-29, 2026-08-30, and both amendment sections stay, in date order.

The risk worth naming: resolving this by taking this branch's side of the hunk is
syntactically clean and silently deletes #2304's amendment — both its date on the status line
and the whole tri-state upsert section — from an ADR that already shipped it. Nothing downstream
would flag that; the file would simply no longer document behaviour that is live in main. Worth
a targeted check after resolving that ## Amendment (2026-08-29) is still present.

The other two hunks on this branch (the search(...) signature around line 259 and the score
interpretation around line 298) do not overlap main's remaining change, which lands near line
184 in upsert_atoms(...). So the header is the only place that needs judgment.

Why there is no CI

on: pull_request: here declares no types:, so it uses the default set
[opened, synchronize, reopened]. This PR missed its runs and flipping states will not recover
them — but that does not need any separate action, because pushing the conflict resolution
produces a synchronize, which is in that set and does create runs.
The author's normal next
step clears both problems at once; there is no re-trigger ceremony to perform and no reason for
anyone else to touch the branch.

One thing to expect on that first run: the secret scan was repository-wide until recently and is
now scoped to the change under review, so a first green run here is the scoped behaviour, not a
weakened check.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Both required contexts are absent, and re-triggering will not produce them

Secret scan (gitleaks) and CI gate have never reported at this head. Nothing is queued or
running, so this is not a wait — the workflow that produces both has never been scheduled for
this branch:

every workflow run for this branch
  Auto-merge (bot PRs)   pull_request_target   skipped
  Auto-merge (bot PRs)   pull_request_target   skipped
  Auto-merge (bot PRs)   pull_request_target   skipped

Only the pull_request_target workflow has ever run. The CI workflow triggers on
pull_request, and a pull_request workflow checks out the pull request's merge ref.
That ref does not exist here:

refs/pull/2317/merge   ABSENT
refs/pull/2317/head    present

That is the whole cause. GitHub cannot compute a merge commit while the branch conflicts with
the base, so there is nothing for a pull_request run to check out and no run is scheduled.
pull_request_target uses the base ref instead, which is why the auto-merge workflow ran
normally and nothing else did.

Worth stating what this is not, because the obvious reading is wrong: a conflicted state alone
does not do this. Two other branches are equally conflicted right now and both still have CI
runs and a merge ref, because their merge ref was computed before the conflict appeared and is
simply stale. The discriminator is an absent merge ref, not DIRTY.

The practical consequence: re-running, reopening, or re-marking ready cannot help, since all of
them still have no merge ref to check out. Resolving the conflict is the only thing that
restores it, and the resolving push is itself the synchronize that schedules CI.

The conflict is one file:

CONFLICT (content): Merge conflict in docs/adr/ADR-047-knowledge-pack.md

crates/khive-pack-knowledge/src/vocab.rs and docs/guide/api-reference.md auto-merge cleanly.
One caution on that: a clean auto-merge is not agreement. Where both sides edited far apart in
the same file, git keeps both edits, which can leave two definitions of the same thing with the
last one winning and no test noticing. Worth reading the merged result of those two files rather
than trusting the absence of a conflict marker.

@ohdearquant

Copy link
Copy Markdown
Owner Author

No CI is running on this branch, and the empty check list is the symptom

This branch conflicts with main — the conflict is in docs/adr/ADR-047-knowledge-pack.md
so GitHub cannot build the merge ref that pull_request workflows run against. Those workflows
are therefore never instantiated. The only entries on this head are Enable auto-merge runs,
which are triggered against the base ref and report skipped.

The consequence is worth stating plainly, because it reads the wrong way round: neither required
check has ever run here. CI gate and Secret scan (gitleaks) are both absent, not passing.
The pull request displays no failing checks, but that is the absence of any check rather than a
green result, and a sweep that looks for red builds will never surface this branch.

The direction of the error is safe — a required check that is missing blocks the merge, so
nothing can land unverified. The cost is silence: the branch sits with no signal either way.

Resolving the conflict and pushing is what produces a check surface. Nothing else will; in
particular, re-running the existing jobs replays the same stale merge ref and cannot create one.

@ohdearquant ohdearquant left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head 8e4ab94: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

@ohdearquant ohdearquant left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head 21dcc88: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

…-out in search

A query whose every token falls below the minimum scoreable term length
(e.g. "AI") reached FTS only as the raw phrase, which the trigram tokenizer
cannot match below three characters. Removing the recency-fallback lexical
path left such short exact names undiscoverable without ANN. search_core now
probes the unique (namespace, slug) index as an indexed exact-name fallback
when no query term is scoreable, and reports a distinct exact_name lexical
candidate state so callers can tell it apart from an ordinary FTS match.

The lexical candidate stage also bounds the number of distinct scoreable
terms that each issue their own FTS MATCH statement. Without this bound, a
query with many distinct terms turned one request into a proportionally
unbounded number of index probes and retained-row memory, checked only by
the request read deadline. A query at or under the bound sees identical
candidate generation and ranking to the unbounded behavior; a query over the
bound now reports terms_truncated in candidate_provenance.

Also documents the upsert_atoms source_uri/source_type patch fields that
were missing from the API reference, and adds a deterministic-embedder test
covering the embedding_rerank=true provenance path, which previously had no
coverage.

@ohdearquant ohdearquant left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head c4c1c11: REQUEST-CHANGES, 2 blocking findings. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Superseded by three stacked changes that separate the score-provenance vocabulary, the per-request FTS term bound, and the exact-name probe: #2381, #2382, #2383.

@ohdearquant ohdearquant closed this Sep 3, 2026
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.

knowledge.search: documented 0.46/0.42 score bands are stale since the s/(s+1) squash; full-scan fallback scores off-topic queries in the on-topic band

1 participant