fix(persistence): correct tenancy capability advertisements (#369)

[mauripunzueta]

Closes #369.

Corrects a false capability advertisement, and — in a second pass — re-audits
every backend's non-tenancy capability claims, fixes a SQLite cross-tenant
search leak, and hardens S3 bucket-per-tenant validation. The tenancy
advertisement correction removes no isolation mode and places no data
differently; the second-pass search-leak and S3-validation fixes do change
runtime behaviour and are called out under "Round 2" below.

The claim, and why it's wrong

crates/persistence/src/backends/postgres/backend.rs listed
BackendCapability::SchemaPerTenant and DatabasePerTenant in both
supports() and capabilities(). PostgreSQL implements neither. It is
shared-schema only: one resources table keyed by
(tenant_id, resource_type, id) with a tenant_id discriminator every query
filters on. There is no SET search_path, no CREATE SCHEMA, no
CREATE DATABASE, and no per-tenant pool anywhere under src/backends/, and
no configuration knob selects a tenancy strategy for the PostgreSQL backend
(S3's S3TenancyMode is exactly such a knob — see below — but no SQL backend
has one). Design discussion #28 chose shared-schema as the pragmatic approach
and did not commit to schema- or database-per-tenant — so this was a false
claim, not an unimplemented plan.

capabilities() is the surface a caller uses to ask whether a backend gives
them a particular tenant-isolation guarantee. For a healthcare server,
answering "yes" for isolation that does not exist is the dangerous direction to
be wrong in.

Behaviour change — security relevant

PostgreSQL. Every release up to and including v0.2.1 reported
SchemaPerTenant and DatabasePerTenant for the PostgreSQL backend; this is
fixed in the next release, not in 0.2.1 as published (the branch does not bump
the version). It has never implemented either. All PostgreSQL deployments are,
and always have been, shared-schema.
No runtime behaviour changed and no data was ever placed differently — only the
advertisement was wrong. If you gated a deployment, procurement, or
data-segregation attestation on supports(BackendCapability::DatabasePerTenant)
against the PostgreSQL backend, please re-assess it.

S3 — a correction in both directions, per mode:

• PrefixPerTenant instances stop claiming DatabasePerTenant (was false —
  all tenants share one bucket).
• BucketPerTenant instances stop claiming SharedSchema (was false) and keep
  DatabasePerTenant (was, and remains, true).

Source break: S3Backend::declared_capabilities() is removed and
replaced by S3Backend::declared_capabilities_for(&S3TenancyMode). It was a
static associated fn, so it could not answer a question whose truth depends on
the instance's configuration. It is renamed rather than silently redefined so
the change is loud at every call site — silently changing what a same-named
public function returns is the exact drift mechanism this PR exists to remove.

Release ask: this should not ship in a patch release. A public inherent fn
was removed and advertised values changed, so under 0.x conventions it belongs
in the minor slot (0.3.0). No version bump here — 0.2.1 is shared-version
across 19 crates and cargo release owns it.

Classification: a documentation/advertisement defect with security-decision
consequences. Not a vulnerability, no CVE, no RUSTSEC advisory — nothing is
exploitable, nothing regressed, and no isolation was ever weakened.

S3 was the same defect from a subtler cause

Not in the original issue; found while checking every backend. S3TenancyMode
has two genuinely different modes — tenant_location() resolves a dedicated
per-tenant bucket under BucketPerTenant (and errors InvalidTenant for an
unmapped tenant), while PrefixPerTenant shares one bucket. Because the
declaration was static it claimed both SharedSchema and DatabasePerTenant
unconditionally, and was false in one direction whichever mode it ran.

It now derives from self.config.tenancy_mode and declares exactly one.
DatabasePerTenant is kept for BucketPerTenant — dropping it to mirror
the Postgres fix would have erased a real, implemented, operator-paid-for
isolation mode. The match is exhaustive with no wildcard arm, so a third mode
is a compile error rather than a stale claim.

The semantics this pins down

The trait already implied the answer: supports(&self) and capabilities(&self)
both take &self, and S3 — the only backend that discarded it — was the only
other backend that was wrong. A capability now explicitly means "this running
instance, as configured, does X", never "this backend type could be
configured to." The alternative reading is degenerate: with enough code any
backend could do anything, which is how ~2,100 lines of never-executed
strategy/ scaffolding became a Postgres capability claim.

Documented on BackendCapability, on the three tenancy variants (now defined,
and declared mutually exclusive), and on both trait methods. Configuration may
only refine which member of a mutually exclusive group is reported — it may
never motivate a new variant.

Why this drifted, and what stops it recurring

Every backend hand-maintained two capability lists — one matches! ladder
in supports(), one vec![] in capabilities() — so the false claim had to be
written twice and either copy could drift. All five backends now derive both
from a single constructor-free declared_capabilities().

That is also what makes the fix testable at all: PostgresBackend::new is async
and eagerly verifies connectivity, so an instance-based assertion for the very
backend this issue is about could only run against a live database — i.e. never
on an ordinary PR.

New crates/persistence/tests/backend_capability_contract.rs needs no database,
Docker, MinIO, AWS credentials, or network. Its central invariant is that each
backend declares exactly one tenant-placement topology: re-adding
SchemaPerTenant to Postgres cannot be made green by adding to a list — the
only passing edit is to also remove SharedSchema, which is a loud,
reviewable semantic claim rather than a quiet addition.

Deliberately not added: a supports(c) == capabilities().contains(&c) sweep
(tautological after the delegation, and needs live instances), and a
source-scanning test for SET search_path (that SQL already exists in
src/strategy/, so such a test would dictate #370's file layout and red-build
correct future work).

On tests/common/capabilities.rs

The issue asks to reconcile this matrix. Worth stating plainly: cargo never
compiles it. No test target declares mod common; (recorded in-tree at
tests/backend_error_handling.rs:44), so it has never executed — which is
exactly why it and the backends were free to drift.

It is also unfit as a gate in principle: it marks S3 DatabasePerTenant as
Implemented, so a matrix-agreement test would have passed the S3
over-claim; SupportLevel::Partial has no representation in a capability
vector (Postgres FullTextSearch is Partial-and-declared, ChainedSearch is
Partial-and-undeclared); and it is keyed by BackendKind, so it structurally
cannot express a mode-dependent S3.

So the two Postgres tenancy cells are corrected and the S3 mode-dependence noted,
but the file is labelled documentation-only with no enforcement value. Its fate,
and the dead should_run_test / backend_test! harness around it, stays with
#306/#361.

Prose corrections

The same false claim appeared where an operator actually reads it: the
crate-level rustdoc (lib.rs), the PostgreSQL blurb and two feature bullets,
and — not in the issue — the capability matrix row marking SQLite ✓ for
Database-per-Tenant, under a table that promises "actual implementation status,
not aspirational goals." SQLite has no per-tenant file or ATTACH logic and
declares only SharedSchema.

Scope

src/strategy/ is untouched — no deletion, no wiring, no un-exporting. Its
fate is #370. The re-export in lib.rs gains a rustdoc note only (these are SQL
generators that configure nothing); leaving the export in place is deliberate,
not an oversight. The README "Phase 3" checklist is annotated rather than
unchecked for the same reason: saying those generators are unwired is a fact;
deciding whether they should be wired is #370's call.

Deliberately out of scope, to be filed as follow-ups:

• Elasticsearch tenancy classification. index_name() gives each tenant its
  own index ({prefix}_{tenant}_{type}), which looks like a per-tenant
  topology. It keeps SharedSchema here — every document also carries a
  tenant_id field that queries filter on, and the split is a naming convention
  inside one cluster and one credential, with no policy or storage boundary. The
  adjudication is recorded in a doc comment. Reclassifying would strengthen an
  isolation claim, which is not what this PR should do. It also exposes a real
  taxonomy gap: the enum cannot express "namespace-per-tenant within one
  service", which is what both ES indices and S3 buckets actually are.
• BackendCapability::InDbSofRunner and RawSqlQuery are declared by no
  backend, though RawSqlQuery's own doc says "(Postgres, SQLite only)" —
  under-claiming, the safe direction.
• composite/config.rs:205 keeps a parallel, config-declared capability list
  independent of the backends' own.
• Matrix staleness in the under-claiming direction (CursorPagination: Planned while keyset paging is implemented; PessimisticLocking absent) —
  carried with the matrix's fate.

The first pass deliberately did not re-audit non-tenancy capability claims;
Round 2 (below) does. The InDbSofRunner / CursorPagination under-claims
noted above are now fixed.

Verification

There is no C linker in this environment, so nothing here was verified by a
local cargo check/test — CI is the gate. cargo fmt was run. Every symbol,
import, feature name, public path, and intra-doc link target was checked by hand
against the tree.

---

Design settled by a five-persona council (multi-tenancy architect, healthcare
compliance auditor, Rust API/semver steward, regression-prevention engineer,
scope steward), each position adversarially stress-tested. The S3 defect, the
SQLite README row, the uncompiled-matrix finding, and the clippy interaction
that rules out #[deprecated] all came out of that process.

---

Round 2 — capability re-audit, SQLite leak fix, and test-hole closure

A second five-persona council reviewed the first pass, then a per-backend audit
(one auditor per backend) checked every BackendCapability variant against
the actual implementation. Three things came out of it.

1. Non-tenancy capability re-audit (all backends)

Backend::capabilities() under-declared implemented, live-wired behaviour. It is
consumed by the shipped config-advisor and by the README matrix, so
under-declaration misinforms operators. Declarations now match what a deployment
using the backend can actually do, still derived from the single per-backend
declared_capabilities():

Capability	Added to
QuantitySearch	SQLite, Postgres, MongoDB (ES already had it)
ChainedSearch, ReverseChaining	SQLite, Postgres, MongoDB, Elasticsearch
FullTextSearch	SQLite (Postgres/ES already had it)
CursorPagination	SQLite (others already had it)
Include, Revinclude	MongoDB (others already had it)
BulkExport	MongoDB
InDbSofRunner	SQLite, Postgres, MongoDB

Chained/_has and _include/_revinclude are resolved by the persistence
layer's backend-agnostic resolver (helios_persistence::search::resolve_chains)
over each backend's search() — which rewrites the chain into an _id filter
the backend then executes — so every search-capable backend that honours _id
declares them. S3 (no search()) does not. The native per-backend
ChainedSearchProvider is dead code on the live path; the capability reflects
the deployment, not that provider. InDbSofRunner covers an in-DB SQL runner
(SQLite/Postgres) and an aggregation-pipeline runner (MongoDB); S3's in-process
runner and ES (no runner) do not declare it.

No over-claims were found. S3 OptimisticLocking was scrutinised and is a
genuine If-Match/If-None-Match conditional write (ETag CAS → 412 →
VersionConflict), not a read-then-write. The enum docs and the golden lists in
backend_capability_contract.rs were updated to match; each backend's
declaration is cross-checked against its golden list as an exact set.

2. SQLite cross-tenant search leak (runtime fix)

Three SQLite search sub-selects omitted the tenant_id discriminator that
BackendCapability::SharedSchema promises every query carries — the
_text/_content FTS (query_builder.rs), _filter (filter_parser.rs), and
:identifier reference (reference.rs) paths. Each scanned every tenant's
rows and returned a bare resource_id/EXISTS result that the tenant-filtered
outer query then intersected, so tenant A's Patient/123 was returned whenever
tenant B's Patient/123 matched — a cross-tenant match oracle and plainly wrong
results. Postgres already scoped all three correctly.

Fixed by adding tenant_id = ?1 to each sub-select (no new bindings — ?1 is
the tenant in every param layout this builder produces). New symmetric
regression tests in sqlite_tests.rs run one query against two tenants holding
the same resource id: the owning tenant must match, the other must not.

3. S3 BucketPerTenant validation (runtime fix)

declared_capabilities_for returns DatabasePerTenant for any BucketPerTenant
config, but nothing checked the per-tenant buckets were distinct — two tenants
mapped to one bucket wrote byte-identical keys (silent cross-tenant overwrite),
making the claim false. S3BackendConfig::validate() now rejects duplicate
buckets, and a mapped bucket colliding with default_system_bucket for a
non-system tenant, with a deterministic sorted error naming the collision. The
capability is now true by construction.

4. Delegation test hole

supports() and capabilities() both delegate to declared_capabilities(), so
they agree today — but a regression that re-hand-rolls a matches! ladder in
supports() would pass every constructor-free golden test. Added instance-level
agreement tests for SQLite/MongoDB/Elasticsearch (their constructors open no
connection — no infra needed) and extended the live-Postgres
postgres_integration_capabilities test with the tenancy negatives and a
supports↔capabilities agreement loop. Added the missing S3 BucketPerTenant
golden list.

Breaking changes

• Removed public inherent fn S3Backend::declared_capabilities(), replaced
  by S3Backend::declared_capabilities_for(&S3TenancyMode). No in-repo caller
  existed; impact is limited to external crates.io consumers of
  helios-persistence 0.2.x.
• Backend::supports() return values changed for existing deployments:
  PostgreSQL now answers false for SchemaPerTenant/DatabasePerTenant, and
  several backends now answer true for the re-audited capabilities above. An
  external caller branching on these takes a different branch. Together with the
  removed fn, this is why the release belongs in the 0.3.0 minor slot, not a
  patch.

Deliberately deferred to follow-ups

Confirmed but out of scope for an advertisement/correctness PR, now filed:

• Elasticsearch: tenants differing only in case share an index and document _id (cross-tenant overwrite/delete) #384 — Elasticsearch index_name() lowercases the tenant id, so tenants
  ACME and acme share an index and document _id — a real cross-tenant
  read/overwrite/delete on every _id-addressed write path.
• Tenant ids have no canonical validator: divergent charsets and an infallible TenantId::new #385 — no canonical TenantId charset validation (the root cause of Elasticsearch: tenants differing only in case share an index and document _id (cross-tenant overwrite/delete) #384;
  each backend defends inconsistently).
• SQLite: purge paths never delete from resource_fts, so purged content stays searchable #386 — SQLite purge never clears resource_fts, so purged content stays
  searchable (SQLite-only; Postgres deletes it).
• Reconcile README non-tenancy capability matrix with the re-audited Backend::capabilities() #387 — README non-tenancy matrix cells (the finer-grained ◐/○ feature
  rows) were not reconciled with the re-audited boolean capability vector.

Verification (Round 2)

Same as above: no C linker locally, CI is the gate. cargo fmt clean. Each
backend's declaration was cross-checked against its golden list as an exact set;
every new symbol, import, feature gate, and enum variant was hand-verified
against the tree. New tests were written to compile under --all-features (the
CI config) with each behind its backend's feature gate.

[mauripunzueta]

Cross-PR note for reviewers of either: #380 now implements the #370 decision (delete src/strategy/), so the interim scaffolding annotations this PR adds are superseded there.

Overlap and resolution, so neither review is confusing:

• The lib.rs crate-doc "Multitenancy" bullet — refactor(persistence): remove unwired tenancy-strategy scaffolding (#370) #380 uses this PR's exact wording, so that hunk merges clean.
• The [strategy] module-list bullet, the rustdoc note this PR adds above pub use strategy::{...}, and the README "Phase 3" annotations — deleted or replaced by refactor(persistence): remove unwired tenancy-strategy scaffolding (#370) #380, by design: they say "not wired into any backend — see Decide the fate of crates/persistence/src/strategy/: ~2,100 lines of unwired tenancy-isolation code #370", and Decide the fate of crates/persistence/src/strategy/: ~2,100 lines of unwired tenancy-isolation code #370's answer is removal.
• The README capability-matrix rows and the PostgreSQL / PG+ES feature bullets from this PR are untouched by refactor(persistence): remove unwired tenancy-strategy scaffolding (#370) #380 — they describe BackendCapability variants, which survive the deletion.

This PR should merge first. #380 branches from main rather than stacking, because HFS CI only runs for PRs based on main and a 2,100-line deletion must not merge unbuilt; it rebases after this lands. Both are breaking and both belong in the same 0.3.0 slot.

[codecov]

Codecov Report

❌ Patch coverage is 97.27273% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ends/sqlite/search/parameter_handlers/reference.rs	84.21%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!