fix(acl): out-of-scope rows consumed slots in the row cap and starved scoped clients

[sturlese]

Sixth iteration of the autonomous bughunt sweep. Both hunters and two gate reviewers flagged this one; it is the last high-confidence ACL item on the answer side.

Bug

Both read paths capped in SQL and applied the ACL filter in Python afterwards, so invisible rows occupied slots in the cap:

metrics.query_metrics   ... ORDER BY entity, metric, period, source_ref LIMIT ?    then filter
retrieve.search         ... ORDER BY bm25 LIMIT 40                                then filter

retrieve.py's own comment states the rule this breaks — "not an annotation: an invisible page simply isn't there" — and a row that isn't there must not occupy a candidate slot. With enough out-of-scope rows sorting ahead, the visible tail was silently truncated, down to nothing:

FACTS: eng-scoped client asking for arr-usd
   unrestricted     -> 50 rows
   eng-scoped       -> 0 rows        <== STARVED
PAGES: eng-scoped client searching "widget detail"
   unrestricted     -> 5 hits
   eng-scoped       -> 0 hits        <== an OPEN matching page existed

Why this matters more than a truncation

It is a wrong answer, not a short one. ask() refuses a question the brain can answer from content the client is fully entitled to — the same observable outcome as "the brain doesn't know", which is what the refusal path is supposed to mean.

And it is scope-dependent: an unrestricted client never sees it, so it cannot be reproduced by an operator testing the deployment without adopting a client's exact scope. That is precisely the asymmetry the ACL layer exists to avoid.

The trigger is not an extreme corpus. 60 restricted rows sorting ahead of 3 open ones is ordinary — the facts store holds one row per figure per document, and entity sorts first, so a single well-documented restricted entity can bury everything alphabetically after it.

Fix

The scope goes into the WHERE clause, so SQL's own LIMIT counts rows the client may see. That is the only shape that is both correct and bounded — cap-then-filter starves, and filter-then-cap unbounds the sort (see below).

                                main    branch     x
scoped, metric filter          0.008s   0.059s   7.3x   (and now a CORRECT answer)
scoped, NO filter              0.016s   0.018s   1.1x
unrestricted, metric filter    0.008s   0.008s   1.0x   <== byte-identical query
unrestricted, NO filter        0.016s   0.016s   1.0x

visible_sql returns the literal "1" with no parameters when audiences is None, so an unrestricted deployment executes exactly the query it executed before. The residual 7.3x is a scoped client on a store where every row shares the filtered metric — the pathological case, since the LIKE is a residual filter applied after the index seek.

I tried the other shape first, and the gate destroyed it. My first attempt dropped the SQL LIMIT and filtered while "streaming" the cursor, specifically to avoid a third hand-mirrored copy of the visibility rule (the thing that produced the five ACL bugs already fixed here). That justification was empirically false: with ORDER BY and no LIMIT, SQLite materialises the entire sort before yielding row one, so nothing streams and breaking out of the loop saves nothing. It measured 11x slower for unrestricted clients on a path the MCP tool reaches with no arguments, and up to 72x for scoped ones — trading a correctness bug for a DoS amplification.

So the objection had to be answered rather than avoided. visible_sql sits immediately beside visible() in the module that owns the rule, and test_visible_sql_agrees_with_visible proves the two answer identically over 12 acl encodings × 13 scopes — including NULL, '', multi-label, empty scope, and labels containing the LIKE metacharacters %, _ and \, which are escaped. Unescaped, an audience label of "%" would have matched every restricted row. It is a second form of one rule, with the agreement proven rather than assumed.

Incidental: the search pool is now the best 40 candidates the client can actually see rather than whatever survived a pool chosen without regard to them, so scoped ranking is better informed; and the now-redundant visible() check in the ranking loop is gone.

Test

Both confirmed to fail with the source change stashed (assert 0 >= 3, and no hits at all).

• test_out_of_scope_rows_do_not_starve_a_scoped_client — 60 finance rows for an entity sorting first, 3 open rows after: the eng client gets exactly the 3 open values. Also asserts the cap still caps, for both a scoped and an unrestricted caller — an over-eager "fix" that ignored limit would pass the first assertion alone.
• test_out_of_scope_pages_do_not_starve_a_scoped_search — 45 finance pages matching the query plus one open one: the open page is returned and none of the restricted ones are.
• test_visible_sql_agrees_with_visible — the truth-table agreement above.

Also verified and not otherwise asserted: for an unrestricted client retrieve.search is byte-identical to main — same paths, scores to 6 dp, same factors — across three queries over a 60-page corpus with more than _POOL matching pages, with and without exact bm25 ties.

answer 59 (+3) · clean 252 · corpus 48 · graph 40 · slack 13 · fetch 33 · benchmark 3 — bare pytest (CI mode). ruff clean; golden scorecard 25/25 including all four end-to-end Q&A cases and the ACL metric; ACL visibility parity 11/11.

A serious sibling, found by the gate and NOT fixed here

The same cap-then-filter shape exists for the current-truth filter, in three live places:

• service.current_metric_rows caps at 100 then drops superseded, and current or rows falls back to the 100 stale rows. Verified with real code: 100 rows returned, all from a superseded page, while 3 current rows existed. That is worse than the bug this PR fixes — confidently wrong numbers rather than none.
• service.metrics_text renders rows[:30] of that same stale list, so the MCP query_metrics tool shows 30 superseded lines.
• retrieve.search(include_superseded=False) drops superseded after the pool — three lines below the check this PR removed — giving zero hits while a current matching page exists.

Same defect shape, different invariant, so it gets its own PR. Confirmed clean (no cap before filter): known_metrics, known_entities, superseded_paths, search_text, check_citations, synthesis evidence assembly.

Not shipped this iteration, and why

The queued lead was clean.acl.dossier_acl iterating a string member ACL into characters (dossier_acl(["finance"]) == ['a','c','e','f','i','n']). It reproduces, but I traced the callers before writing anything: dossier_acl's only caller feeds it lastResult["acl"], which worker.py always writes as a list, and clean.acl.visible has no production caller at all — only the parity eval and unit tests. So it is defensive-only, and this sweep's bar excludes that. It stays recorded as a robustness item rather than being shipped as a bugfix.

[sturlese]

Gate verdict

VERDICT: REFUTED — and correctly. The reviewer attacked my fix on its own stated justification and demolished it. Reworked in 4da21cf.

My justification was empirically false

Both new comments claimed the scan stayed bounded: "the WHERE clause still bounds how much can be scanned", "stream the bm25-ordered cursor". Neither is true. With ORDER BY and no LIMIT, SQLite materialises the entire sort before yielding row one — so nothing streams, and breaking out of the Python loop after N rows saves nothing. LIMIT was what made the sorter bounded; removing it removed the very bound the comment claimed still existed.

Measured on a 200k-row store, against main (I reproduced this independently before acting):

scoped, metric filter          72.3x slower
scoped, NO filter              48.1x
unrestricted, NO filter        11.0x     <== ordinary clients, no ACL involved
unrestricted, metric filter     1.8x

The last two are the damning ones. Dropping LIMIT cost every open deployment, on a path the MCP query_metrics tool reaches with no arguments at all. Trading a correctness bug for a DoS amplification — worst for exactly the scoped deployments the fix protects — is not a trade worth making.

The rework

The scope now goes in the WHERE clause, so SQL's own LIMIT counts rows the client may see. That is the only shape that is both correct and bounded: cap-then-filter starves, filter-then-cap unbounds the sort.

scoped, metric filter           7.3x     (0.008s -> 0.059s, and now a CORRECT answer)
scoped, NO filter               1.1x
unrestricted, metric filter     1.0x     <== byte-identical query, zero cost
unrestricted, NO filter         1.0x

visible_sql returns the literal "1" with no parameters when audiences is None, so an unrestricted deployment executes exactly the query it executed before. The residual 7.3x is a scoped client on a store where every row shares the filtered metric — the pathological case, since the LIKE is a residual filter after the index seek.

On my own objection. I had argued against this shape because it creates a third hand-mirrored copy of the visibility rule — the thing that has produced five ACL bugs in this package. That objection is answered rather than dropped: visible_sql sits immediately beside visible() in the module that owns the rule, and test_visible_sql_agrees_with_visible proves the two answer identically over 12 acl encodings × 13 scopes — including NULL, '', multi-label, empty scope, and labels containing the LIKE metacharacters %, _ and \, which are escaped. Unescaped, an audience label of "%" would have matched every restricted row. It is a second form of one rule, with agreement proven rather than assumed.

Also fixed, from my own review between rounds

Replacing LIMIT ? with a len(out) >= limit break had silently changed two edge semantics SQL gave for free: limit=0 returned one row instead of none, and limit=-1 returned one instead of unlimited. Moot now that LIMIT is back, but it was real in the reviewed commit.

A serious sibling, NOT fixed here

The reviewer found the same cap-then-filter shape in the current-truth filter, in three live places, and proved each with real code:

• service.current_metric_rows caps at 100 then drops superseded, and current or rows falls back to the 100 stale rows. Verified: 100 rows returned, all from a superseded page, while 3 current rows existed in the store. This is worse than the bug this PR fixes — it returns confidently wrong numbers rather than no numbers.
• service.metrics_text then renders rows[:30] of that same stale list, so the MCP query_metrics tool shows 30 superseded lines.
• retrieve.search(include_superseded=False) drops superseded after the pool — three lines below the check this PR removed — giving zero hits while a current matching page exists.

Same defect shape, different invariant (current truth, not ACL), so it gets its own PR rather than riding on this one. It is now the top item in the backlog.

Confirmed clean, with no cap before the filter: known_metrics, known_entities, superseded_paths, search_text, check_citations/verify_answer, synthesis evidence assembly.

Mutation testing

Six of seven mutants killed in the reviewer's run; the survivor was len(rows) > _POOL (a pool of 41 instead of 40) — the exact pool size is only loosely constrained, which I am leaving as is since _POOL is a tuning constant rather than a correctness boundary.

answer 59 (+3) · clean 252 · corpus 48 · graph 40 · slack 13 · fetch 33 · benchmark 3, bare pytest (CI mode) · ruff clean · scorecard 25/25 · parity 11/11.