fix(acl): the index trusted the acl value it re-derived from page text

[sturlese]

Fifth iteration of the autonomous bughunt sweep — the last two "silently open" items from the full ACL audit that #36's gate ran. Both live in the same encoding step in index.refresh, so they go together.

Bug 1 — a non-list acl: value meant "no ACL"

refresh honoured acl only when it was a YAML list. Every other shape fell through to acl = None, which means "this page carries no ACL" — open to everyone:

acl: finance          stored None   eng client sees it: True   <== the intuitive form
acl: "finance"        stored None   True
acl: ''               stored None   True
acl: null             stored None   True
acl: {finance: true}  stored None   True

acl: finance is what anyone writing a single label by hand produces. clean always emits the list form, so the trigger is a hand-authored or hand-edited page — but the whole point of an ACL is that it holds for pages the pipeline did not write, and ADR 010 explicitly foresees other writers feeding this same field.

This is #34's fix being incomplete, not a new bug. #34 taught this block that an unreadable block means restricted-to-nobody, and the contract line it added says "only a page with genuinely no acl key is open" — but the code only ever checked the block, never the value. "acl" in fm closes it, and that distinction is load-bearing: fm.get("acl") cannot tell acl: null from an absent key.

Bug 2 — a comma inside a label widened access

Labels are joined into one CSV column, and nothing checked them, so a label containing a comma split back into two audiences at enforcement time:

acl: ["finance,eng"]  stored 'finance,eng'   eng client sees it: True

clean.acl._check_labels rejects a comma or a blank label at config load, calling it "the exact silent-corruption failure mode an access-control config must not have". The index re-derives the same encoding from page text and cannot assume the page came through that validation. _label_ok mirrors that rule.

Bug 3 — recognising the block required it to be CLOSED

Found by the gate, and the larger half of the same hole. carries_frontmatter_block ran the full block regex despite a docstring (mine, from #34) claiming "whether or not it parses". So a page carrying acl: [finance] was indexed NULL — open — whenever its block was unclosed, truncated mid-write, ended with YAML's ..., preceded by a BOM, or preceded by a blank line. A truncated page leaked its entire ACL, where bugs 1 and 2 leaked only a malformed one.

Bug 4 — the label check renamed labels instead of rejecting them

Also from the gate. _label_ok validated str(label) rather than requiring a str, and a renamed label is grantable: YAML 1.1 reads acl: [010] as the int 8 and [12:30] as 750, so the index stored audiences nobody granted. It also collided [~] with ["None"] and [1.50] with [1.5].

Fix

One condition and one predicate:

• elif "acl" in fm or (not fm and carries_frontmatter_block(text)) — an acl this encoding cannot represent is still an acl, so the audience is unknown, and unknown is not open.

• _label_ok — a label must have no comma and be non-blank, matching clean.acl._check_labels. A label the encoding cannot represent makes the whole ACL unreadable rather than quietly dropping the bad label.

• carries_frontmatter_block matches the opener alone, tolerating a BOM and leading blank lines. The BOM case is handled better than merely closing it: split_frontmatter strips the BOM, so that page's real ACL is read — a BOM is encoding noise editors add invisibly, not content, and it must never be able to hide an acl.

• _label_ok requires isinstance(label, str). Verified this cannot reject anything the pipeline writes: clean.page._yaml emits a scalar plain only when it round-trips as the identical string, so every label it writes reads back as a str.

Everything unreadable now stores '' — restricted to nobody: no scoped client sees the page, an unrestricted operator still does, so the breakage is discoverable rather than silent.

And a migration, or none of this reaches a deployed index. refresh re-reads a page only when its (mtime, size) changed, so a row stored as NULL under the old rule would stay open for the life of the index — measured added: 0, updated: 0 on an existing index. PRAGMA user_version = 2 sets mtime = -1 to force the re-encode, and closes rows meanwhile rather than after. That closes every page to scoped clients until refresh completes (AnswerService refreshes immediately after connect()); unrestricted clients are unaffected.

Test

Both confirmed to fail with the source change stashed, and verified across ten acl: shapes:

acl: value	stored	eng	finance	unrestricted
[finance]	'finance'	no	yes	yes
finance · "finance" · '' · null · {finance: true}	''	no	no	yes
["finance,eng"] · [finance, " "] · [true]	''	no	no	yes
[]	''	no	no	yes
(no acl key)	NULL	yes	yes	yes

The last two rows are the ones that matter for regressions: acl: [] still round-trips through the list branch to '' (PR #30's semantics, unchanged), and a page with genuinely no acl key is still the one open case.

answer 56 (+6) · clean 252 · corpus 48 · graph 40 · slack 13 · fetch 33 · benchmark 3 — bare pytest (CI mode). ruff clean; golden scorecard 25/25; ACL visibility parity 11/11.

Parity is unaffected: eval_contract_parity builds the CSV itself rather than going through refresh, and the two hand-mirrored halves agree exactly on the valid-label domain that this fix now enforces at index time.

Docs

brain-page-contract.md's tolerance rule now covers the acl value as well as the block, and states the separator constraint for any consumer that serialises labels into one field — the assumption that cost this bug, twice.

Still open

• clean.acl.dossier_acl iterates a string member ACL into characters (found by this PR's gate, verified): dossier_acl(["finance"]) == ['a','c','e','f','i','n'], so a dossier is emitted with six single-character audiences its members never granted. The same value-shape assumption the index just stopped making, on the pipeline side.
• clean.acl._check_labels accepts whitespace-padded labels — it rejects commas and blanks but not surrounding whitespace, so "audiences": [" finance "] mints a label no ANSWER_AUDIENCES value can ever match (the scope side strips, the page side does not). Silently over-restrictive, so fail-closed, but it means an ACL config can be quietly inert. Deliberately not fixed here: normalising on the read side alone would make the two mirrored halves disagree, so it wants fixing on the write side, in its own PR.
• The ACL filter runs after the SQL row cap in query_metrics and retrieve.search, which can starve a scoped client to zero results.

[sturlese]

Gate verdict

VERDICT: WEAKENED. (A first reviewer stalled; a relaunched one completed. Its two lead findings are fixed in 3ac9073.) It also found and correctly dismissed the attack I most expected to land.

The same disclosure, one line above the code I changed

carries_frontmatter_block required the closing ---. Its docstring — which I wrote in #34 — says "True when the page OPENS a frontmatter block, whether or not it parses." It did not; it ran the full block regex. So a page carrying acl: [finance] was indexed NULL — open to everyone — whenever its block was unclosed, truncated mid-write, ended with YAML's ..., preceded by a BOM, or preceded by a blank line:

case               parsed?  opens?    -> stored acl
unclosed           False    False     None  <== OPEN
truncated          False    False     None  <== OPEN
dots terminator    False    False     None  <== OPEN
BOM prefix         False    False     None  <== OPEN
leading blank      False    False     None  <== OPEN

Byte-identical on main, so a miss of the same class rather than a regression — and the larger half of it: a truncated page leaked its entire ACL, where the values my first commit closed leaked only a non-list one. It now matches on the opener alone. The BOM case is handled better than merely closing it: split_frontmatter strips the BOM, so that page's real ACL is read. A BOM is encoding noise editors add invisibly; it should never be able to hide an acl.

A predicate that renamed labels instead of rejecting them

_label_ok validated str(label) rather than requiring a str — and a renamed label is grantable. YAML 1.1 reads acl: [010] as the int 8 and acl: [12:30] as 750, so the index stored audiences "8" and "750" that nobody granted, visible to any client holding them. It also collided [~] with ["None"] (both "None", and matchable) and [1.50] with [1.5], and accepted or rejected nested containers by whether repr() happened to contain a comma. Excluding bool while coercing int/float/None/list/dict was three different answers to one question.

isinstance(label, str) is the whole fix, and the reviewer verified it cannot reject anything the pipeline writes: clean.page._yaml emits a scalar plain only when it round-trips as the identical string, so every label it writes reads back as a str.

The attack I expected to land, dismissed on evidence

I expected "you fixed the page half and left the facts half open". The reviewer traced resolve_acl → ",".join(acl) → factstore.replace_facts → observations.acl and showed there is no path from page frontmatter into observations.acl: every value comes from load_acl_config, which _check_labels-validates commas and blanks on both default and every rule. So the mirrored writer is genuinely safe, and this PR is not half a fix.

Also corrected

The v2 migration comment claimed "nothing goes dark". False: it closes every page to scoped clients until the next refresh, including pages whose ACL was fine. AnswerService refreshes immediately after connect(), but a caller that connects without refreshing serves a scoped client nothing — and if the UPDATE cannot run (another process holds a write lock) connect() raises rather than proceeding, which is the right trade: no service beats a leaky one. The reviewer also measured the migration at 1.2s over a 100k-row / 410 MB index, and confirmed pages.mtime has exactly one reader (refresh's skip check), so -1 cannot leak into a response.

Verified sound

Not over-restrictive against the real pipeline: indexing the evals' produced brain-md with main's index.py and with this one gives zero differences across all 8 pages. "acl" in fm cannot raise (split_frontmatter returns {} for a non-dict). Duplicate acl: keys resolve last-wins and fail closed. Nine mutants across both rounds all caught, including the over-restriction direction and dropping the migration.

Recorded for its own PR

• clean.acl.dossier_acl iterates a string member ACL into characters — verified: dossier_acl(["finance"]) == ['a','c','e','f','i','n'], so the dossier is emitted with six single-character audiences its members never granted. The same value-shape assumption the index just stopped making, on the pipeline side.
• load_acl_config str()s default but not rules[].audiences, so a numeric or null rule label passes validation and then crashes ",".join(acl) per document. Fails closed and loudly, but it is an asymmetry in the file this PR cites as its authority.
• Case-variant keys (ACL:), a trailing space in the key name, and a nested acl are still read as "no acl" — lower severity, since the contract names the key exactly.

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