Skip to content

fix(explain): scope-filtered candidates leak their summary to the viewer - #640

Merged
plind-junior merged 1 commit into
vouchdev:testfrom
kai392:fix/critical-issue-explain-ranking-scope-leak
Jul 30, 2026
Merged

fix(explain): scope-filtered candidates leak their summary to the viewer#640
plind-junior merged 1 commit into
vouchdev:testfrom
kai392:fix/critical-issue-explain-ranking-scope-leak

Conversation

@kai392

@kai392 kai392 commented Jul 30, 2026

Copy link
Copy Markdown

What changed

explain_ranking sources candidate summaries from the scoped hit set
instead of the pre-scope fused set. one word in the dict comprehension; the
candidate list itself is unchanged.

Why

the candidate loop deliberately walks every hit the pipeline saw, so a dropped
artifact is still explained rather than silently missing. but summaries was
keyed off that same pre-scope set, so the summary rode along for candidates
filter_hits had just removed. the result is that a viewer gets back the text
of artifacts kb.search and kb.context withhold from it — the module
docstring promises exactly the opposite:

Viewer scoping runs through the same filter_hits as kb.context, so a
caller cannot see a candidate it could not already retrieve.

same viewer, same query, on test before this patch:

=== kb.search as an outsider viewer ===
    (nothing)
=== kb.explain_ranking as the SAME viewer ===
    id=c-secret  gate=scope-filtered  summary='acme-example quarterly revenue projection is 4.2 million'

kb.explain_ranking is registered on all four surfaces — the mcp tool, the
jsonl handler, METHODS, and the cli — and on the remote transports the
viewer arrives as a request param, so this is the read path that answers with
content the sibling read paths refuse.

worth being precise about the severity: scoping.py says retrieval surfaces
treat scope as a relevance filter and artifacts stay readable as plaintext
on disk, so this is not a local privilege boundary. it matters for the
multi-project and remote-transport shapes — the same reason kb.audit grew
viewer scoping and tests/test_audit_scoping.py — and for the scoped-token
work in #608, where the viewer stops being caller-declared.

What might break

nothing on disk, no method signature changes, no kb.* shape change — the
summary key is still always present, just "" for a candidate the viewer
could not retrieve.

the deliberate part of the design is preserved on purpose. i read
test_scope_filtered_candidate_is_attributed_to_scope as pinning intent:
listing the hidden candidate and naming its gate is what makes the report
useful for tuning. so the row, the scope-filtered gate and the stage list
all stay exactly as they were — that test passes untouched. only the text is
withheld. if you'd rather drop such candidates from the report entirely, say
so and i'll switch it, but that would reverse a choice #628 looks to have
made on purpose.

VEP

not a surface change — no vep.

Tests

  • make check passes locally (lint + mypy + pytest)
  • New / changed behaviour has a test
  • CHANGELOG.md updated under ## [Unreleased]

tests/test_explain_ranking.py::test_scope_filtered_candidate_does_not_carry_its_summary
— asserts the gate is still reported, the summary is "", the secret string
appears nowhere in the serialised result, and a retrievable candidate still
carries its summary. it fails on the previous code with the leaked text in the
assertion diff.

validation run:

python -m ruff check src tests
python -m mypy src
python -m pytest tests/test_explain_ranking.py tests/test_scoping.py \
                 tests/test_audit_scoping.py tests/test_context.py \
                 tests/test_capabilities.py -q

no ui files touched, so no screenshots.

@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance tests tests and fixtures size: XS less than 50 changed non-doc lines labels Jul 30, 2026
@kai392
kai392 force-pushed the fix/critical-issue-explain-ranking-scope-leak branch 4 times, most recently from a5af5d7 to a270074 Compare July 30, 2026 15:21
the candidate list is built from every hit the pipeline saw, and summaries
were keyed off that same pre-scope fused set. so a viewer scoped to one
project got back the text of claims `kb.search` and `kb.context` withhold
from it — the module docstring promises the opposite, and the surface is
exposed over mcp, jsonl and the cli.

listing the candidate is deliberate: naming the gate that hid it is what
makes the report useful for tuning, and
`test_scope_filtered_candidate_is_attributed_to_scope` pins that. so this
keeps the row and its `scope-filtered` gate and withholds only the text,
by sourcing summaries from the scoped set rather than the fused one. a
candidate the viewer can retrieve is unaffected.
@plind-junior

Copy link
Copy Markdown
Member

Checked this against test and the diagnosis holds. Summaries was keyed off the pre-scope hits while scoped sat ten lines above it, so explain_ranking was the one read path returning text for artifacts the same viewer's kb.search had already dropped.

One followup, not for this PR: The report still exposes a filtered candidate's id and ranks. It's fine while scope is a relevance filter, but #608 makes the viewer no longer caller-declared, and that turns existence disclosure into the next question here.
LGTM!

@plind-junior
plind-junior merged commit 4e075fc into vouchdev:test Jul 30, 2026
15 checks passed
plind-junior pushed a commit that referenced this pull request Jul 30, 2026
a retracted/superseded/redacted claim or archived page correctly
reported gate: "status-filtered", but explain_ranking sourced
summaries from `scoped` (post scope-filter, pre status-filter), so
its full live text still came back regardless of which stage actually
dropped it.

#640 fixed the equivalent leak for the scope_filter stage, moving
summaries off the raw fused set. that fix's chosen source, `scoped`,
is itself still upstream of `_filter_live_hits`, so the same
withholding was never extended to status_filter.

source summaries from `live` instead — the set that survived both the
scope and status gates, matching what kb.search/kb.context expose.
new tests mirror #640's scope-filtered regression test for a
superseded claim and an archived page, asserting an empty summary and
that the secret text is absent from the dumped response.

Fixes #650
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs documentation, specs, examples, and repo guidance size: XS less than 50 changed non-doc lines tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants