fix(acl): scope known_metrics — the metric vocabulary leaked existence

[sturlese]

Third iteration of the autonomous bughunt sweep — lead #2 from the backlog (lead #1, the corpus taxonomy ordering, is parked pending a policy decision).

Bug

known_metrics was the one read path in the serving layer that ignored the ACL scope, though observations.acl is right there in the table it queries:

rows = conn.execute("SELECT DISTINCT metric FROM observations WHERE entity = ?",
                    (entity.strip().lower(),)).fetchall()
return [r[0] for r in rows]

Three documents state the contract it breaks:

• answer/index.md:43 — "the ACL scope (ANSWER_AUDIENCES) must filter every read path; out-of-scope pages must look non-existent, not forbidden."
• ADR 010 — "query_metrics filters rows, and even entity existence is scoped."
• docs/answer.md:51 — "their numbers don't resolve, and even entity existence is scoped."

And its own sibling twelve lines up, service.known_entities, carries the comment "existence is also scoped".

Why it matters — the caller makes it worse

service.metrics_text reaches known_metrics on the empty-result path — that is, precisely when the row filter just hid everything from this client — and then answers with the vocabulary anyway:

eng.metrics_text(None, "acme")
-> "no observations for that query. known metrics for acme: total-compensation"

So a client scoped to eng learns the two things the rest of the service goes out of its way to hide: that entity acme exists, and that it has a total-compensation figure. The same value reaches match_metric, so an out-of-scope metric id also resolves while parsing the question.

It leaks ids rather than values — but known_entities proves the intended posture, and all three docs promise scoped existence.

Fix

known_metrics takes audiences and applies visible() itself, selecting acl alongside the metric. The filtering cannot be pushed to the caller: one metric may be carried by several documents with different audiences, and it must stay visible when any of them is in scope. Both service call sites pass self.audiences. Unrestricted clients (audiences=None) are unchanged.

Also matches query_metrics' verified = 1. factstore inserts verified rows only (hardcoded 1 at factstore.py:72, schema default 1), so this changes no result today; it keeps the hand-mirrored read paths saying the same thing. (Noted by the gate: nothing enforces that agreement — known_metrics has no pipeline counterpart, so eval_contract_parity cannot probe it. It is symmetry by convention, not by proof.)

Test

Three commits: the fix, plus two rounds from self-review and the adversarial gate (detail in the gate-verdict comment). Two further hardenings came out of those and are part of this PR:

• A store with no acl column is no longer served open. acl reaches facts.db through an additive migration clean runs on its write path; this package only reads it. Naming the column crashed ask() outright on a legacy store, and degrading to "no acl → open" leaked values, source_refs and existence to scoped clients while the page itself stayed hidden. Now "the store records no audience information" means a scoped client gets nothing — the direction the page index already takes for an unreadable acl (fix(contract): a model-chosen type/date could drop a page's ACL or kill the build #34) and the one brain-page-contract.md states. Unrestricted instances are unaffected, so open corpora and the demo are unchanged. Applied to query_metrics too.

• Unrestricted clients no longer read acl at all, keeping that query inside the covering index idx_obs_metric; selecting the extra column forced a temp B-tree and measured 32x slower on a 300k-row store, and match_metric runs it once per question.

• test_known_metrics_are_scoped — the scoping itself, plus both service paths that reach it (metrics_text, match_metric), and that open facts and unrestricted clients are unaffected.

• test_no_public_read_surface_leaks_an_out_of_scope_document — a probe table over every public read surface (search, search_text, get_page, page_text, query_metrics, current_metric_rows, known_entities, match_metric, metrics_text, ask), asserted in both directions: hidden from eng through all of them, and still reachable by finance through all of them, so an over-denying "fix" cannot pass by hiding everything from everybody. On the unfixed code: reachable through: ['match_metric', 'metrics_text'].

• test_leak_probes_cover_every_public_service_surface — the completeness assertion, and the actual point. My first version of the checklist was a hardcoded list whose docstring claimed a missing surface was "the bug this test exists to catch". That was false: it was structurally blind to a new surface, which is exactly how known_metrics got in unscoped. The gate proved it by adding an unscoped known_periods() — all 48 tests passed while it handed an eng client 'acme/total-compensation@2026'. It now asserts over inspect.getmembers(AnswerService), so adding a public method fails until it is either probed or explicitly classified as not-a-content-surface. Verified against that mutant, and against a get_page-denies-everyone mutant for the positive direction.

• test_refresh_counts_are_index_maintenance_not_a_content_surface — the one deliberate exclusion, with its reasoning pinned rather than waived: refresh() reports what the index did, and the index holds every page because one instance serves one scope. The test asserts no transport hands those counters to a client, so if that ever changes the exclusion breaks loudly instead of silently becoming a count oracle.

• test_pre_acl_facts_db_serves_unrestricted_but_never_a_scoped_client — the legacy-store direction, failing without the guard.

answer 50 (+5) · clean 252 · corpus 48 · graph 40 · slack 13 · fetch 33 · benchmark 3 — bare pytest (CI mode). ruff clean; golden scorecard 25/25. Full CI-equivalent suite green locally before opening.

Found by the gate's sweep — NOT fixed here

Auditing this class exhaustively (it is the third ACL leak in this package) turned up four more, each reproduced with literal output. They are separate bugs with their own fixes, recorded rather than ridden in:

1. ANSWER_AUDIENCES="," silently means UNRESTRICTED (settings.py:31) — tuple(...) or None turns a scope that parses to zero labels into "no scope", so ",", ",,", " , " serve the entire corpus while the operator believes the instance is scoped. ADR 010 states this doctrine for the write side ("a malformed config errors loudly: silently-open is the one failure mode an access-control file must not have") but it is not applied to the read side. Most severe item found so far; next up.
2. A non-list acl: value is encoded as "no ACL" and served open (index.py:128-137) — acl: finance (the intuitive single-label form), acl: '', acl: null all fall through to None. My own fix(contract): a model-chosen type/date could drop a page's ACL or kill the build #34 fix being incomplete: it handled an unreadable block but not a readable block with an unreadable value.
3. A comma inside a label widens access (index.py:129) — clean/acl.py rejects commas because labels are CSV-serialised; answer re-parses the page text with no such check, so acl: ["finance,eng"] grants both.
4. superseded_by echoes the source id of an out-of-scope successor (service.py:100) — existence disclosure, no content.

Plus the sibling bug in the same file: the ACL filter runs after the SQL row cap in both query_metrics (LIMIT ?) and retrieve.search (LIMIT 40), so out-of-scope rows consume candidate slots and can starve a scoped client to zero results — a wrong answer rather than a truncated one. Independent blast radius and a different fix (stream the ordered cursor, stop after N visible rows), so it gets its own PR.

[sturlese]

Gate verdict

Two agents: one told to refute this fix, one told to sweep the whole serving layer for any other unscoped read path (this being the third ACL leak found in answer, the class was worth auditing exhaustively).

Verdicts: REFUTED at f6ab55a, WEAKENED at 7f2c9fe. Both commits under-delivered on their own claims, and both are now fixed in 6d83a16.

What the refuter broke

1. The pre-ACL crash was worse than I described. I'd caught it myself, but the reviewer measured the real blast radius: at f6ab55a, ask() died with OperationalError: no such column: acl on every question, for every client including unrestricted ones — while main and the sibling query_metrics worked fine. A security PR shipped a total-outage regression, its own new checklist test did not notice, and CI was green on it.

2. The degradation direction was wrong — and this is the finding I'd have defended and been wrong about. 7f2c9fe made both facts read paths degrade to "no acl → open" on a pre-ACL store, justified as symmetry with query_metrics. That is symmetry with the wrong half: the page index takes the opposite direction for an acl it cannot read (acl = '', restricted to nobody — #34), and brain-page-contract.md states it outright. On a legacy store a scoped client got values, source_refs, the restricted page path and metric/entity existence, while the page itself stayed hidden. Now: no acl column means no audience information, so a scoped client gets nothing; unrestricted instances are unaffected.

3. My checklist test could not catch what its docstring claimed. It said "a surface missing from it is the bug this test exists to catch." False — the list was hardcoded, so it regression-tested the 8 surfaces already in it and was structurally blind to a new one, which is exactly how known_metrics got in unscoped. The reviewer proved it by adding an unscoped known_periods(): all 48 tests passed while it handed an eng client 'acme/total-compensation@2026'.

It is now a probe table plus a completeness assertion over inspect.getmembers(AnswerService) — adding a public method fails until it is probed or explicitly classified. Verified against that same mutant. The positive direction was also one-sided (3 of 8 surfaces), so an over-denying "fix" could pass; both directions are now asserted over the full table, verified against a get_page-denies-everyone mutant that the old version passed.

4. Perf: selecting acl forced a temp B-tree instead of the covering index — 32x slower on a 300k-row store, and match_metric runs it once per question. Unrestricted clients no longer read acl at all.

Also confirmed by the refuter and worth recording: the ANY-in-scope union logic is correct (a metric shared by an in-scope and an out-of-scope document stays visible; one carried only by out-of-scope documents vanishes); ''/None/CSV encodings all behave; sorted() and SQL ORDER BY agree for every possible TEXT value (UTF-8 byte order = code-point order). And a behavioural delta I had not reported: because match_metric now returns None, out-of-scope refusals travel the search path, so the reason changes from "no entity named 'acme' in the brain" to "no pages matched: …" — arguably better, since it no longer asserts a falsehood about an entity that does exist.

The sweep: four more leaks, none of them this PR's

All 11 public service surfaces are correctly scoped after this PR, and the sweep found no fourth unscoped method. But it found four leaks below the method level, reproduced with literal output. These are separate bugs and are NOT fixed here — they are recorded as the top of the bughunt backlog:

1. ANSWER_AUDIENCES="," silently means UNRESTRICTED (settings.py:31). tuple(...) or None turns a scope that parses to zero labels into "no scope", so ",", ",,", " , " all serve the entire corpus while the operator believes the instance is scoped. One config typo — e.g. a template rendering an empty list — and everything is exposed. ADR 010 states this doctrine for the write side ("A malformed config errors loudly: silently-open is the one failure mode an access-control file must not have"); it is not applied to the read side. One-line fail-loud fix. This is the most severe thing found in three iterations and is next up.
2. A non-list acl: value is encoded as "no ACL" and served open (index.py:128-137). refresh honours acl only when it is a YAML list; acl: finance (the intuitive single-label form), acl: '', acl: null all fall through to None → open. This is my own fix(contract): a model-chosen type/date could drop a page's ACL or kill the build #34 fix being incomplete: I handled an unreadable block but not a readable block with an unreadable value, even though the doc line I wrote covers both.
3. A comma inside a label widens access (index.py:129). clean/acl.py rejects commas precisely because labels are CSV-serialised, but answer re-parses the page text and applies no such check, so acl: ["finance,eng"] grants both.
4. superseded_by echoes the source id of an out-of-scope successor (service.py:100). Existence disclosure, no content.

Clean bill of health for the rest, with evidence: no existence oracle (out-of-scope and nonexistent produce byte-identical messages on every surface), check_citations short-circuits before get_page, superseded_paths never emits a path, FTS bm25 statistics never leave the server, nothing reads brain_md_dir outside the index, and the facts↔page ACL asymmetry is not producible by the pipeline.

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