feat(authz): shared filter derivation, snapshot decision boundary, and adapter conformance suite

[tlyyxjz]

feat(authz): shared filter derivation, snapshot decision boundary, and adapter conformance suite

Draft PR promised in #1398 (comment), following the review answers from @Teingi. Target scope as agreed there: shared filter derivation, a defined snapshot/read boundary with the deliberately-interleaving regression, the conformance extensions, and an explicit capability statement for the native-writer question. Branch is a single draft; every claim below is backed by a test in this PR.

What changed

1. Shared filter derivation — one derivation, zero drift risk.

resolve_resource_filter was duplicated between BuiltinAuthorizationProvider (service.py L395-433) and CasbinAuthorizationProvider (casbin.py L125-159) with a latent semantic difference: the Casbin copy's parent test (_is_parent) accepted a SERVER resource as a parent for any requested type, while the built-in copy (_resource_is_parent + _parent_binding_grants) constrains the child type. Both now call one package-internal derivation (_derive_authorized_resource_filter in service.py), keeping the built-in (stricter) semantics. The private helpers remain package-internal; nothing is promoted to public adapter API.

2. Snapshot decision boundary — the read gap is closed two ways.

The read-consistency gap demonstrated on this thread (revision read at 3, grant commits at 4, bindings evaluated from 4 while the decision is labeled 3) came from policy_revision(), active_bindings() and ownership reads running in separate transactions. Both providers now read decision inputs through one boundary (read_decision_state in service.py):

• Primary: RelationalAccessRepository.decision_snapshot() reads revision + active bindings + artifact ownership (+ owned resources when needed) inside one transaction; backends with snapshot isolation (SQLite read transactions, OceanBase REPEATABLE READ) supply the consistency. One captured evaluation time is used for every expiry comparison in the decision.
• Fallback: repositories without the capability get bounded revision-check-and-retry (revision → bindings → re-read revision; retry while unstable, max 3), and fail closed (AccessUnavailableError("policy-snapshot-unstable")) when no stable read is obtained.
• AccessRepository protocol is unchanged — decision_snapshot is an optional capability detected by the providers, so existing implementations keep working through the fallback.

3. Native-writer capability statement (as promised, measured not asserted).

No second policy store is added. With the composition in this repo — canonical five-table schema as the only writable copy, enforcer materialized from committed state at evaluation time — a persistent Casbin policy table would add storage without a demonstrated need, and this PR contains no workload showing adapter translation falling short. The concrete deltas this draft delivers are the three the review identified: shared derivation, reusable conformance tests, and a defined snapshot boundary. If a large-policy-set workload appears, the same single-writer + revision-equality rule from the earlier storage-layout comment remains the agreed shape for adding one.

4. Conformance suite.

New tests/test_access_snapshot_boundary.py:

• test_providers_read_decision_inputs_from_one_snapshot — one transaction per check/resolve_resource_filter (was 3+), both providers;
• test_interleaved_mutation_cannot_label_decision_with_stale_revision — the controlled interleaving from the review, replayed: a grant committed between the reads can no longer produce allowed=true under the pre-mutation revision, and a stable retry labels it with the matching revision;
• test_unstable_revision_fails_closed — no stable snapshot within the budget → AccessUnavailableError, never a possibly-stale allow;
• test_providers_agree_on_point_and_list_for_direct_and_inherited_grants — point/list agreement plus cross-provider filter parity;
• test_idempotency_ledger_conflicts_across_operations_and_payloads — repository-level, shared by both adapters: changed-payload conflict on the same key, cross-operation conflict, replay-after-revocation returning the revoked binding without resurrection, and post-revocation re-grant via a fresh key.

Test evidence

Tests were written first and run against the pre-fix tree; they failed with exactly the gap under review, which is the mutation check for this suite:

assert counting.transactions == 1, "check must read from one snapshot transaction"
E  AssertionError: assert 3 == 1                     <- pre-fix: 3 separate transactions

assert not (decision.allowed and decision.policy_revision == pre_mutation_revision)
E  AssertionError: assert not (True and '3' == '3')  <- pre-fix: the exact stale-label shape from the review

with pytest.raises(AccessUnavailableError): ...
E  Failed: DID NOT RAISE AccessUnavailableError      <- pre-fix: unstable reads proceeded silently

After the fix (this PR), the authz and access suites at the current head:

tests/test_access_snapshot_boundary.py tests/test_access_control.py tests/test_access_adapters.py \
tests/test_access_http.py tests/test_access_mcp.py tests/test_docs_remote_access_contract.py
54 passed in 10.81s

ruff check and ruff format clean on all touched files. ty check reports the same 10 diagnostics as master on this machine, none of them in a touched file.

Snapshot read boundary (review follow-up)

Review found that the single SQLAlchemy transaction decision_snapshot reads through is not a snapshot on SQLite: sqlite3's legacy transaction control does not BEGIN for a bare SELECT, and read_decision_state delegated on the mere presence of decision_snapshot, which left the revision-check-and-retry fallback unreachable. Reproduced with two connections against one file-backed database — the reader captured revision N, a commit landed, and the binding read returned the new grant labelled with revision N.

• _pin_read_snapshot opens the read transaction explicitly with SAVEPOINT. Not BEGIN: the read may already run inside a transaction this repository did not open (a shared in-memory connection, or with_connection), where a second BEGIN raises.
• decision_snapshot re-reads the revision before the snapshot is released and raises policy-snapshot-unstable rather than returning the mixture; read_decision_state retries inside the budget it already had. A READ COMMITTED profile therefore fails closed instead of returning a stale label — see the OceanBase note below for what that does and does not buy.
• Two regressions, neither of which hides the snapshot capability: test_snapshot_path_survives_two_connection_interleaving runs the interleaving against the real path (the earlier test raised AttributeError on decision_snapshot, so it only ever exercised the fallback) and asserts the snapshot holds on the first attempt; test_snapshot_read_fails_closed_when_isolation_cannot_pinned covers a profile that cannot pin isolation. Disabling the pin makes the first one fail, so it is a regression rather than decoration.

Rebased onto master

Rebased onto master at 346fef79 (the previous base was 103 commits behind) so the branch merges cleanly: mergeable is now true. Two conflicts, both the same shape — each side added a distinct method or import at the same anchor, no semantic overlap:

• repository.py — master added with_connection (the bound-connection support); this PR adds decision_snapshot. Both are kept. One line of this PR had to move with the refactor: decision_snapshot was written against self._database.transaction() and now uses self._database.connection(self._bound_connection). AsyncDatabase.connection is defined as transaction() if bound is None else nullcontext(bound), so this is identical when unbound and additionally joins the caller's transaction when the repository is bound to one. That single line was the whole rebase diff of decision_snapshot; the review follow-up above changes the method further.
• service.py — import block is the union of both sides (AsyncIterator/asynccontextmanager/ContextVar from master, Mapping from this PR).

casbin.py and tests/test_access_snapshot_boundary.py are byte-identical to the pre-rebase head. The change set is still exactly 4 files, +678/−81.

What this draft does not claim

• OceanBase concurrency run — the interleaving regression runs on SQLite here. It is written against the DatabaseConfig abstraction and backend-agnostic, but per the review the same interleaving should be exercised on OceanBase in your CI; I cannot run that from this environment and would rather say so than claim a green I did not produce. What the fix buys there is worth stating precisely: it does not give a READ COMMITTED connection a consistent snapshot — it guarantees the read fails closed instead of returning data labelled with a revision it was not read at. Forcing an explicit consistent-snapshot/REPEATABLE READ transaction in the profile would be a separate change, not smuggled in here.
• load_filtered_policy — untouched; it remains an optional optimization and is not load-bearing for decision correctness or pagination safety.
• Persistent Casbin policy table — intentionally not added (see §3).
• Evaluation-time materialization failure — still fails closed per the corrected consistency framing; pre-commit validation semantics are unchanged.

[CLAassistant]

All committers have signed the CLA.

[tlyyxjz]

Note on the CI result: the tests (3.11) failure is a sqlite concurrency flake — database is locked in test_concurrent_default_bootstrap_returns_one_scope (tests/builtin/), which is unrelated to the authz changes in this PR. The same commit passes the full suite on 3.12/3.13/3.14. Happy to rerun if needed.

[tlyyxjz]

@Teingi — this is ready for review. Two things changed since you last saw it, and both were the reason it was sitting unreviewed:

1. Rebased onto master at 346fef79. The branch was 103 commits behind, so it was conflicted and mergeable_state was dirty — that, plus the draft flag, is most of why this PR never reached your queue. mergeable is now true. The two conflict resolutions are written up in the PR description; both were the same shape (each side added a distinct method or import at the same anchor), and the only line of this PR that had to move with master's refactor is decision_snapshot's connection acquisition, which now follows the new bound-connection contract instead of the older transaction() call.
2. Marked ready for review, so it is no longer filtered out as a draft.

Verified before pushing: the authz and access suites pass at the new head (52 passed), ruff check and ruff format are clean on all touched files, and ty check reports no new diagnostics against the master baseline — same 10 as master, none of them in a file this PR touches. The change set is still exactly 4 files, +678/−81.

I could not set a formal reviewer request — this account does not have the permission on this repo — so this comment is doing that job.

The one item from your review I still cannot close from here is the OceanBase concurrency run for the interleaving regression; it runs against SQLite in this PR and is written to be backend-agnostic, but per your note it should be exercised on OceanBase in your CI. If you can point me at how that is normally invoked, I would rather wire it than leave it as a caveat — failing that, the PR body states it plainly as the thing I could not produce.

[Teingi]

[P1] Establish a real snapshot before trusting this read boundary

Using one SQLAlchemy transaction here does not give SQLite a consistent read snapshot: the current SQLite profile uses legacy transaction handling, where SELECT does not issue BEGIN. Since decision_snapshot exists, read_decision_state also skips the revision-check-and-retry fallback.

I reproduced this at f9effd44 with a file-backed SQLite database and two independent connections: the reader captures revision 3, the writer commits a new grant at revision 4, and the reader returns allowed=true with policy_revision="3". Both Builtin and Casbin reproduce this for point checks and resource filters. Adding an explicit read transaction in the probe keeps all four cases consistent.

The new transaction-count test does not catch this, and the interleaving test hides decision_snapshot, so it exercises only the fallback. Please add a two-connection interleaving regression against the actual snapshot path and establish a real snapshot, or use revision validation when that guarantee is unavailable. Also account for OceanBase connections using READ COMMITTED: the profile does not enforce REPEATABLE READ, so sharing a transaction alone is insufficient there too. I have not run the OceanBase interleaving probe.

[tlyyxjz]

CI status at f9effd44, so the one red X does not cost you a detour.

Green: validate, quality, tests (3.11 / 3.12 / 3.13 / 3.14), Acceptance (sqlite), Acceptance (oceanbase), linux-systemd-user, windows-task-scheduler, windows-unit-portability, website-link-validation-windows, check-website, Check License Header, and the five package jobs. (The cancelled entries are the first trigger being superseded by the second, not failures.)

Red: macos-launchagent, on tests/native/test_personal_service_lifecycle.py — "the personal service was registered but did not become live: cannot reach http://127.0.0.1:49210". The same assertion is failing right now on unrelated branches (codex/rfc-desktop-control-center at 03:33, codex/layer-powercontext-skills at 01:55 and 17:52 yesterday), so I read it as a flaky launchd lifecycle test on the macOS runner rather than as something this PR caused — the four files here are all under server/authz/. Correct me if that reading is wrong.

One thing that did improve: tests (3.11) is green here. On the previous head it failed once with the sqlite concurrency flake I flagged on 9/8; it has not recurred.

[tlyyxjz]

Confirmed — you are right on both counts. Reproduced here, then fixed at the snapshot boundary.

I reproduced it before changing anything, with a file-backed SQLite database and two independent connections, driven through the repository the way read_decision_state drives it:

	reader captured	writer committed	reader's binding read
current head f9effd44	revision 2	revision 3	saw the revision-3 grant → allowed=true labelled policy_revision="2"
with an explicit read transaction	4	5	did not see it → consistent

Both Builtin and Casbin reproduce it, point checks and resource filters alike.

Root cause (two independent halves)

1. The shared transaction is not a snapshot on SQLite. decision_snapshot reads through AsyncDatabase.connection(bound), which is transaction() when nothing is bound — i.e. engine.begin(). With pysqlite's legacy transaction control that emits no BEGIN SQL (SQLAlchemy logs BEGIN (implicit) as a logical marker only; the only statement actually sent is the SELECT). A bare SELECT therefore never opens a read transaction, so a concurrent commit is visible mid-read. The docstring's "a SQLite read transaction … provide[s] it" was simply wrong.
2. The retry path was unreachable. read_decision_state delegates on the mere presence of decision_snapshot, so nothing re-checked the revision afterwards.

You were also right about the tests. test_providers_read_decision_inputs_from_one_snapshot asserts transactions == 1 — that counts transaction openings, it does not assert consistency — and _InterleavingRepository.__getattribute__ raises AttributeError("decision_snapshot"), so the interleaving test could only ever exercise the fallback. The snapshot path had no interleaving coverage at all.

Fix — both options you offered, because they cover different backends

• A real snapshot, pinned explicitly: _pin_read_snapshot() issues SAVEPOINT powercontext_decision_read_snapshot before the first read. SAVEPOINT rather than BEGIN on purpose — the read may already run inside a transaction this repository did not open (shared in-memory connection, or with_connection), and a second BEGIN fails with cannot start a transaction within a transaction. SAVEPOINT composes. (builtin/persistence/tags.py:127 already pins with BEGIN for the same reason; that call site owns its transaction, this one cannot assume that.)
• Revision validation where a snapshot cannot be pinned. The revision is re-read before the snapshot is released; if it moved, the read raises policy-snapshot-unstable instead of returning the mixture, and read_decision_state retries it inside the same bounded budget it already uses for the separate-read fallback.

That second half is what answers your READ COMMITTED / OceanBase point, and I want to be precise about it rather than let it look stronger than it is: we do not make OceanBase consistent here, and I have not run an interleaving probe against a real OceanBase instance (same gap you flagged). What changes is the failure mode — a READ COMMITTED profile can no longer return data labelled with a revision it was not read at; it fails closed, or succeeds on a retry that saw a stable revision. If you would rather have the guarantee than the detection there, forcing an explicit consistent-snapshot/REPEATABLE READ transaction in the profile is the follow-up, and I would rather do that as its own change than smuggle a session-level isolation change in here.

Regression tests against the actual snapshot path

Two new tests, neither of which hides decision_snapshot:

• test_snapshot_path_survives_two_connection_interleaving — same two-connection setup as the reproduction above, but on the real path: the grant is committed immediately after the revision read that opens each snapshot, and the test asserts no decision pairs the late grant with the pre-mutation revision and that the snapshot holds on the first attempt (reads == 2), i.e. it does not silently spend the retry budget.
• test_snapshot_read_fails_closed_when_isolation_cannot_pinned — pins to a no-op to emulate a profile that cannot establish a snapshot (your READ COMMITTED case), commits on every attempt, and asserts AccessUnavailableError rather than a stale-labelled decision.

I checked the first one actually fails without the fix (pin disabled → AccessUnavailableError on the first attempt, no stale result), so it is a regression and not decoration.

Fix is at fdb8fd8d (on top of the f9effd44 you reviewed, so your repro still applies unchanged to the parent). Thanks for the two-connection framing — it is what made this reproducible instead of arguable.

[Teingi]

https://github.com/oceanbase/powercontext/actions/runs/35057736578/job/104864716938?pr=1483 failed

[Teingi]

LGTM

[tlyyxjz]

Post-merge note on e7271e2a — the quality failure and what it was hiding.

Thanks for merging this. Recording the diagnosis behind the last commit, since the red X was resolved in e7271e2a rather than being waved through.

quality failed on ty check with three invalid-assignment diagnostics in tests/test_access_snapshot_boundary.py. Taken at face value that reads as a typing complaint about monkeypatching, and the tempting fix is to annotate the assignment. The actual problem was one layer down.

What it was. The interleaving tests rebound _read_policy_revision and _pin_read_snapshot on the module, and decision_snapshot also called them as module-level functions. So the patch landed on a seam that was only coincidentally the one the reader went through — nothing bound the interception to the code path under test. Had RelationalAccessRepository called a different implementation (a subclass override, or a bound-connection route), the patch would still have "worked" while silently testing nothing. That is the same defect you already caught from the other direction, where the interleaving test hid decision_snapshot and therefore only exercised the fallback. One layer down, same class of bug.

The fix. Both helpers are now overridable methods on RelationalAccessRepository, called via self from decision_snapshot:

• _pin_read_snapshot(connection) / _read_policy_revision(connection) are thin methods delegating to the existing module functions, which remain the default implementations. No behaviour change for any caller.
• Both interleaving tests now patch that seam with patch.object(reader_repository, ...) instead of rebinding a module attribute, so the interception is bound to the instance the reader actually uses. This also drops the manual try/finally restore, which is where the assignment typing was leaking in the first place.

Verified before pushing: uv run ty check reports no diagnostic in a file this PR touches (Found 12 → Found 9, and the 9 are the pre-existing master baseline — os.WNOHANG and the spawn-handle argument types in builtin/runtime/artifact_processing.py). ruff check and ruff format --check clean. pytest tests/test_access_snapshot_boundary.py → 7 passed; the wider access/authz/snapshot selection → 22 passed.

Two things I want to leave stated rather than buried.

1. The patch changed what the tests prove. They now exercise the self._read_policy_revision seam rather than the module binding alone. The assertions are unchanged and still fail without the snapshot fix — I re-verified by disabling the pin, which reproduces AccessUnavailableError on the first attempt instead of a stale-labelled decision. But read as evidence that the module-level functions are the ones drifted on, they no longer demonstrate that, and the green check should not be read as covering it.

2. The OceanBase interleaving probe is still not done, and this merge does not change that. The fix makes the failure mode safe under a READ COMMITTED profile — it fails closed, or retries on a stable revision, instead of returning data labelled with a revision it was not read at. It does not make OceanBase a consistent read. If you would rather have the guarantee than the detection there, forcing an explicit consistent-snapshot/REPEATABLE READ transaction in the profile is the follow-up worth doing as its own change. Still happy to wire that in if you can point me at how the OceanBase concurrency run is normally invoked.