Skip to content

fix(experts): scope-filter claims before ranking entities - #689

Closed
dripsmvcp wants to merge 2 commits into
vouchdev:testfrom
dripsmvcp:fix/experts-scope-filter
Closed

fix(experts): scope-filter claims before ranking entities#689
dripsmvcp wants to merge 2 commits into
vouchdev:testfrom
dripsmvcp:fix/experts-scope-filter

Conversation

@dripsmvcp

Copy link
Copy Markdown
Contributor

closes #684

what

experts.rank_experts filtered candidate claims on status alone:

for claim in store.list_claims():
    if claim.status in _EXCLUDED_STATUSES:
        continue

no is_visible check, so a private or cross-project claim contributed to
every number in the row and had its id handed back under top_claim_ids.
kb.recall, kb.search, kb.context, the salience sidebar and
kb.explain_ranking all scope-filter first; kb.experts did not.

the fix

rank_experts now takes an optional viewer: ViewerContext, defaulting to
viewer_from(config_path=store.config_path) — so a KB read with no explicit
viewer reads as its own project — and drops anything is_visible rejects
before aggregating. this mirrors detect_themes exactly, which had the same
defect on the sibling read surface (#657).

filtering only top_claim_ids would not have been enough. claim_count,
citation_count and score are computed over the same set, so an entity
whose evidence is mostly private would keep outranking one whose evidence the
viewer can actually read, and min_claims would keep being cleared on claims
the caller never gets to see.

the fts candidate fetch goes through scoped_fetch_limit now, matching
kb.search — otherwise a KB whose top hits are mostly private starves the
candidate pool of the claims this viewer can see.

surfaces

viewer scope is plumbed through all three, so a transport that knows who is
asking can say so:

  • mcp kb_expertsproject / agent keyword params, as on kb_audit
  • jsonl _h_expertsviewer_from_params, so both flat project/agent
    and nested viewer_scope work
  • cli vouch experts--project / --agent, worded as on vouch search

tests

five added to tests/test_experts.py, built on the issue's repro (one public
claim, three private/alice claims, all on acme-example):

  • a foreign viewer sees claim_count: 1 and only ['pub'] — the ranking
    itself, not just the ids
  • min_claims=2 yields no row for that foreign viewer, and still yields one
    for the owning agent
  • the owning agent still gets all four — the filter is not a blanket drop
  • visibility: project claims from another project are excluded
  • the jsonl envelope honours params.agent

full gate green locally: pytest tests/ --ignore=tests/embeddings,
mypy src, ruff check src tests.

`rank_experts` filtered candidate claims on status alone, so a private or
cross-project claim contributed to every number in the row and had its id
handed back under `top_claim_ids` — to a viewer that cannot retrieve the
claim itself. `kb.recall`, `kb.search`, `kb.context`, the salience sidebar
and `kb.explain_ranking` all scope-filter first; `kb.experts` did not.

this is the same defect `detect_themes` had (issue vouchdev#657), on the sibling
read surface, and it takes the same fix: resolve a `ViewerContext` (the
config-resolved one when the caller passes none, so a KB read with no
explicit viewer reads as its own project) and drop anything `is_visible`
rejects before aggregating.

filtering only the ids would not have been enough. `claim_count`,
`citation_count` and `score` are all computed over the same set, so an
entity whose evidence is mostly private would keep outranking one whose
evidence the viewer can actually read, and `min_claims` would keep being
cleared on claims the caller will never see.

the fts candidate fetch now goes through `scoped_fetch_limit`, matching
kb.search — otherwise a KB whose top hits are mostly private starves the
pool of the claims this viewer can actually see.

viewer scope is plumbed through all three surfaces: `project`/`agent` on
the mcp tool, `viewer_from_params` in the jsonl handler (flat or nested
`viewer_scope`), and `--project`/`--agent` on the cli mirror.
@dripsmvcp
dripsmvcp requested a review from plind-junior as a code owner July 30, 2026 21:31
@github-actions github-actions Bot added cli command line interface mcp mcp, jsonl, and http surfaces tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 30, 2026
the diff-coverage gate wants 100% of changed python, and the mcp tool
was import-covered only — the decorator ran, the body never did.

two cases, matching what the tool actually promises: a plain ranking,
and the viewer scope this change exists for, where a private claim is
invisible to another agent and visible to its owner.
@github-actions github-actions Bot added size: M 200-499 changed non-doc lines and removed size: S 50-199 changed non-doc lines labels Jul 30, 2026
@plind-junior

Copy link
Copy Markdown
Member

the reasoning for filtering before aggregating rather than only scrubbing top_claim_ids is the load-bearing part of this PR and it is right. claim_count, citation_count and score are all computed over the same set, so post-filtering the id list would have left an entity whose evidence is mostly private outranking one the viewer can actually read, with min_claims cleared on claims they never see — a leak that shows up as a wrong ranking rather than as visible text, which is the harder kind to notice.

routing the FTS candidate fetch through scoped_fetch_limit is the detail that makes it hold under load rather than only in the test. without it a KB whose top hits are mostly private starves the candidate pool before the filter ever runs, and the symptom is "experts got worse on the big KB", which nobody traces back to scoping.

mirroring detect_themes (#657) exactly is the right call — same defect, sibling read surface, and having the two read identically is worth more than a marginally better bespoke shape here.

good to see tests/test_server_tool_surface.py in the diff. plumbing a viewer through three surfaces is precisely where this repo drifts, and a parity test is the thing that keeps --project/--agent on the CLI, the project/agent keywords on MCP, and viewer_from_params on JSONL from diverging later.

one question rather than an objection: viewer=None now defaults to viewer_from(config_path=store.config_path) instead of no filtering, so rank_experts changes behaviour for any existing in-process caller that passed nothing and expected the unfiltered set. for a single-project KB with no private scopes that is a no-op, so the blast radius is small — but if anything internal calls rank_experts for a non-user-facing purpose (metrics, eval), it is now scoped too. worth a quick grep to confirm the only callers are the three surfaces.

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

Labels

cli command line interface mcp mcp, jsonl, and http surfaces size: M 200-499 changed non-doc lines tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: kb.experts ranks on, and names, claims the viewer cannot retrieve

2 participants