diff --git a/.changeset/ingest-acl-contract-fence.md b/.changeset/ingest-acl-contract-fence.md new file mode 100644 index 00000000..50ab832a --- /dev/null +++ b/.changeset/ingest-acl-contract-fence.md @@ -0,0 +1,33 @@ +--- +"@smooai/smooth-operator": patch +--- + +test: fence the ingest→ACL chain at the pipeline seam, not just the GitHub connector + +The guarantee that a connector's document ACL survives ingestion — `RawDocument::acl` +→ chunk → structured `DocAcl` → `AclKnowledgeStore` side table → `AclReader` — was +asserted end to end in exactly one place: `github_connector.rs`. That test is real, but +it is a *connector* test. Delete or rewrite the GitHub connector and the ingest half of +G3 loses its only fence, silently, with the ingestion contract test still green. + +`ingestion_contract.rs::ingested_acls_gate_retrieval_for_every_connector` asserts the +same chain at the pipeline seam over a `MockConnector`, so it holds for every connector +present and future: a document ingested for `group-eng` is readable by a principal +carrying that group and returns **nothing** for `group-fin` or for anonymous, while a +document ingested with no ACL stays org-public. Each negative assertion is paired with +the entitled-principal positive control on the same query, so a pipeline that stored +nothing cannot satisfy "nothing leaked" vacuously — the failure mode this repo has +shipped before. + +Verified red before green: with `DocAcl::for_groups(...).attach_to(document)` reverted +in `pipeline.rs`, the new test fails with `group-fin must not read the group-eng doc, +got 1 hits` — the exact G3 cross-user leak — while the pre-existing contract test stays +green, which is what made the gap invisible. + +No production behavior changes. `docs/Planning/Feature Gaps.md` §G1/§G2/§G9 are updated +to record what actually shipped (the `Connector` seam, `MockConnector`, and the file / +web / github connectors landed some time ago and were never marked), the `pull` → +`Vec` deviation from the planned `Stream` and why it should be +re-shaped before the SaaS connectors rather than after, and what remains: the connector +long tail, format extraction, and the nightly job that would actually run the gated +`external` tier. diff --git a/docs/Planning/Feature Gaps.md b/docs/Planning/Feature Gaps.md index 797861e6..10cf75b9 100644 --- a/docs/Planning/Feature Gaps.md +++ b/docs/Planning/Feature Gaps.md @@ -41,9 +41,13 @@ What we **lack**: ingestion/connectors, a document-processing pipeline, access-c Ordered by leverage. Each item: **write the test first (red), then implement (green)**. -### G1. Knowledge ingestion + connectors (biggest gap) +### G1. Knowledge ingestion + connectors — ✅ seam, mock, and 3 connectors shipped; the SaaS long tail remains Mature platforms ship 50+ connectors (confluence, jira, github, gmail, google_drive, notion, salesforce, sharepoint, slack, zendesk, web, …) + a `mock_connector` for testing. We have only manual/seeded knowledge. - **TDD**: define a `Connector` trait (`async fn pull(&self, since) -> Stream`). Write `tests/connector_contract.rs` against a **`MockConnector`** first (asserts the ingest→chunk→embed→store pipeline lands documents in the `StorageAdapter` knowledge slice + they're retrievable). Then implement the trait + 2–3 real connectors (web, file, github) each with an `external_dependency`-gated test mirroring that split. +- ✅ **Done — `rust/ingestion` (`smooai-smooth-operator-ingestion`).** The seam is `Connector { fn name(); async fn pull(&self, since: Option) -> Result> }` (`src/connector.rs`), driven by `ingest(connector, chunker, embedder, knowledge, options)` (`src/pipeline.rs`): pull → chunk → embed → `KnowledgeBase::ingest`, idempotent on `(document id, content hash)` via an `IngestLedger` so a re-run stores nothing new. Shipped connectors: **file**, **web**, **github** (`src/connectors/`), plus the credential-free `MockConnector`. Background / incremental re-indexing (per-connector cursor + per-run status) is `src/indexing.rs`. The contract test is **`tests/ingestion_contract.rs`** (not `connector_contract.rs` as sketched above); the GitHub connector runs fully offline against a `wiremock` server (`tests/github_connector.rs`). See [[Ingestion]] + [[Connectors]]. +- ✅ **ACLs survive ingestion, connector-agnostically.** A connector's `RawDocument::acl` propagates through the chunker and is written as a structured `DocAcl` (under `DocAcl::ACL_METADATA_KEY`) — the ingest half of the G3 chain. Enforcement is **adapter-side, not opt-in**: Postgres parses that key at ingest into the `knowledge_vectors.acl` column and filters in SQL, DynamoDB into an `acl` attribute post-filtered at read, and `InMemoryStorageAdapter` wraps its knowledge slice in an `AclKnowledgeStore` whose `knowledge()` returns the ACL-recording ingest handle. So the pipeline writing that metadata is the single load-bearing link every backend depends on — which is what the test below fences. `tests/ingestion_contract.rs::ingested_acls_gate_retrieval_for_every_connector` fences it at the **pipeline** seam (a doc ingested for `group-eng` is unreadable by `group-fin` and by anonymous, while a no-ACL doc stays org-public), so the guarantee no longer rides on the GitHub-specific test alone and cannot be deleted with any one connector. Every negative assertion is paired with the entitled-principal positive control, so an empty run cannot satisfy it vacuously. +- ⚠️ **Deviation from the sketch above: `pull` returns `Vec`, not `Stream`.** A `Vec` materializes the whole source per pull. That is fine for the three shipped connectors and wrong for a large Confluence space or Jira project — which is precisely what the next connectors are. Re-shaping `pull` into a stream (or a paged `pull_page(cursor)`) is a **breaking change to the trait**, so it should land *before* the SaaS connectors, not after them. +- **Remains**: the SaaS long tail — confluence, jira, notion, slack, zendesk, google_drive, salesforce, sharepoint. Confluence and Jira are the hardest shape (deep pagination, incremental `since`, per-document permissions) and should be designed against first; their per-document permissions must map onto `RawDocument::acl` the way `GithubConnectorConfig::acl_groups` already does, or ingesting them reopens G3. ### G2. Document processing / chunking pipeline Mature knowledge platforms have a tested chunking + metadata-extraction pipeline. Our knowledge store assumes pre-chunked text. @@ -74,9 +78,11 @@ Mature knowledge platforms support multi-tenant schemas. Our org scoping is row- Mature knowledge platforms have a dedicated, tested model server (embeddings + rerank + intent). We have a pluggable `Embedder` + RRF; the rerank stage is now implemented as a pluggable seam mirroring the `Embedder` pattern. - **TDD (done)**: the `Reranker` trait (`smooth_operator::rerank`) ships `NoopReranker` (identity default) + `LexicalReranker` (deterministic, network-free) + the production **`GatewayReranker`** (adapter crate, alongside `GatewayEmbedder`): a Cohere/Voyage-style `/v1/rerank` cross-encoder over the SmooAI gateway, key from `SMOOAI_GATEWAY_*`. It reorders candidates by returned relevance, truncates to `top_k`, and falls back to input order on any API error (never panics, never drops the turn). A `RerankBackend` seam lets unit tests inject a stub so reorder/truncate/error-fallback are exercised offline (mirrors `GithubSearchBackend`). The server's `build_reranker` selector (mirrors `build_embedder`) picks gateway-when-keyed / lexical / noop from `SMOOTH_AGENT_RERANK`, defaulting **off** so existing behavior is unchanged. Wired into the retrieval path via `KnowledgeSearchTool::with_reranker(...)` (over-fetch → rerank → truncate) in both the reference server and the lambda. A live test is gated on `SMOOTH_AGENT_E2E=1` + a real `/v1/rerank` route (`#[ignore]`). -### G9. Connector mock + external-dependency split (test infra) +### G9. Connector mock + external-dependency split (test infra) — ✅ mock shipped; the tier split is convention, with no nightly running it Formalize the platform.s `mock_connector` + `external_dependency_unit` vs `unit` split so connectors are testable credential-free in CI and fully nightly. - **TDD**: ship the `MockConnector` (G1) and a CI convention: `unit` (no creds, every PR) vs `external` (gated, nightly), matching our `SMOOTH_AGENT_E2E` gate. +- ✅ **Done (the mock + the credential-free tier).** `MockConnector` (`src/connector.rs`) is the fixture behind the ingestion contract test. Every ingestion test in CI is credential-free and runs on **every PR**: GitHub goes through `wiremock`, embeddings through `DeterministicEmbedder`, files through `tempfile`. The one test that touches the network (`connectors::web` live fetch) is `#[ignore]` **and** gated on `SMOOTH_AGENT_E2E=1`, and skips loudly rather than passing silently. +- **Remains**: the **nightly half of the split**. `.github/workflows/rust.yml` has no `schedule:` trigger, so the gated `external` tier is currently a convention that nothing ever executes — a live-API break (auth change, response-schema change, rate limit) in the web or GitHub connector is invisible until a user hits it. Wiring a scheduled job that supplies creds and runs the `#[ignore]`d tests is what actually closes G9. ## 4. TDD working agreement (applies to all of the above and beyond) @@ -87,10 +93,10 @@ Formalize the platform.s `mock_connector` + `external_dependency_unit` vs `unit` 5. **Gated, never skipped silently.** External/LLM tests `skip` (not pass) without creds and log why; nightly CI supplies creds. ## 5. Suggested next TDD increments (priority order) -1. **G3 access-control leak test** (highest severity) → ACL filter on all adapters. -2. **G1 `MockConnector` + ingestion-pipeline contract test** → connector trait + web/file/github connectors. +1. ~~**G3 access-control leak test** (highest severity) → ACL filter on all adapters.~~ ✅ shipped, including the live-path hole — see §G3. +2. ~~**G1 `MockConnector` + ingestion-pipeline contract test** → connector trait + web/file/github connectors.~~ ✅ shipped — next in this line is the `pull` streaming/pagination decision, then Confluence + Jira. 3. **G4 retrieval-quality eval** (deterministic recall@k) alongside the LLM-judge evals. -4. **G5 widget Playwright e2e**, then **G2/G7/G9**. (G6 and G8 are done; G3 is done.) +4. **G5 widget Playwright e2e**, then **G7** (multi-tenancy), plus the specific remainders of **G2** (see §G2) and **G9** — a `schedule:` job that actually runs the gated `external` tier; the mock and the credential-free tier are done. (G1, G3, G6 and G8 are done.) Tracked against the [[Roadmap]]; these become Phase 4 (tools/ingestion), Phase 6 (deploy CI), and a new **Phase 10 — connectors & quality regression**. diff --git a/rust/ingestion/tests/ingestion_contract.rs b/rust/ingestion/tests/ingestion_contract.rs index da24f663..9a0bac62 100644 --- a/rust/ingestion/tests/ingestion_contract.rs +++ b/rust/ingestion/tests/ingestion_contract.rs @@ -131,3 +131,149 @@ async fn ingest_chunks_embeds_stores_and_retrieves_then_is_idempotent() { broad_second.len() ); } + +/// Three documents whose ACLs differ, for the entitlement half of the contract: +/// one unrestricted (the org-public control), one scoped to `group-eng`, one to +/// `group-fin`. Salient terms are mutually distinct so retrieval is unambiguous. +fn acl_fixture_docs() -> Vec { + vec![ + RawDocument::new( + "doc-open", + "mock", + "The grimwald handbook is open to everyone in the company. \ + Grimwald procedures apply to all staff without restriction.", + ) + .with_title("Grimwald Handbook"), + RawDocument::new( + "doc-eng", + "mock", + "The snarflex incident postmortem is engineering-only. \ + Snarflex retries were exhausted before the pager fired.", + ) + .with_title("Snarflex Postmortem") + .with_acl(vec!["group-eng".to_string()]), + RawDocument::new( + "doc-fin", + "mock", + "The brindlewick revenue forecast is finance-only. \ + Brindlewick margins are reported quarterly to the board.", + ) + .with_title("Brindlewick Forecast") + .with_acl(vec!["group-fin".to_string()]), + ] +} + +/// Access control must survive ingestion — connector-agnostically. +/// +/// The end-to-end ACL chain (connector → `DocAcl` → `AclKnowledgeStore` side +/// table → `AclReader`) is also covered in `github_connector.rs`, but only for +/// that one connector. This asserts the same guarantee at the *pipeline* seam, +/// so it holds for every connector and cannot be deleted along with any single +/// one of them. G3 was reopened once already by an ACL layer that existed but +/// was not on the live path; this is the regression fence for the ingest half. +/// +/// Every negative assertion below is paired with the positive control that +/// makes it non-vacuous: the same query, run as an entitled principal, must +/// return the document. Otherwise "nothing leaked" would also pass on a +/// pipeline that stored nothing at all. +#[tokio::test] +async fn ingested_acls_gate_retrieval_for_every_connector() { + use smooth_operator::access_control::{AccessContext, AclKnowledgeStore}; + + let storage: Arc = Arc::new(InMemoryStorageAdapter::new()); + // Wrapping the knowledge slice is what records the ACL at ingest and + // enforces it at read; the pipeline writes the `DocAcl` this store reads. + let acl_store = AclKnowledgeStore::new(storage.knowledge()); + let connector = MockConnector::new(acl_fixture_docs()); + + let report = ingest( + &connector, + &Chunker::default(), + &DeterministicEmbedder::new(), + acl_store.ingest_handle(), + IngestOptions::for_org("org-acme"), + ) + .await + .expect("ingest through the ACL store"); + + // Positive control on the run itself: an empty run must not be able to + // satisfy the "no leak" assertions vacuously. + assert_eq!(report.documents_pulled, 3, "pulled all three fixture docs"); + assert!( + report.chunks_stored >= 3, + "expected at least one chunk per doc, got {}", + report.chunks_stored + ); + + let engineer = acl_store.reader(AccessContext::new( + Some("alice".into()), + vec!["group-eng".into()], + )); + let financier = acl_store.reader(AccessContext::new( + Some("bob".into()), + vec!["group-fin".into()], + )); + let anon = acl_store.reader(AccessContext::anonymous()); + + // --- the restricted doc is readable by its group (positive control) ------ + let eng_hits = engineer.query("snarflex", 10).expect("engineer query"); + assert!( + eng_hits + .iter() + .any(|h| h.chunk.to_lowercase().contains("snarflex")), + "group-eng must be able to read the engineering-only doc" + ); + + // --- ...and NOT by a principal outside it (the leak assertion) ----------- + let outsider_hits = financier.query("snarflex", 10).expect("outsider query"); + assert!( + outsider_hits.is_empty(), + "group-fin must not read the group-eng doc, got {} hits: {:?}", + outsider_hits.len(), + outsider_hits.iter().map(|h| &h.chunk).collect::>() + ); + let anon_hits = anon.query("snarflex", 10).expect("anonymous query"); + assert!( + anon_hits.is_empty(), + "an anonymous requester must not read a group-restricted doc, got {} hits", + anon_hits.len() + ); + + // --- symmetric: the finance doc is gated the other way ------------------- + assert!( + financier + .query("brindlewick", 10) + .expect("financier query") + .iter() + .any(|h| h.chunk.to_lowercase().contains("brindlewick")), + "group-fin must be able to read the finance-only doc" + ); + assert!( + engineer + .query("brindlewick", 10) + .expect("engineer query") + .is_empty(), + "group-eng must not read the group-fin doc" + ); + + // --- a doc ingested with no ACL stays org-public ------------------------- + // (Confirms the gate is an opt-in restriction, not a blanket denial that + // would make the assertions above pass for the wrong reason.) + for (who, reader) in [("engineer", &engineer), ("financier", &financier)] { + assert!( + reader + .query("grimwald", 10) + .expect("open-doc query") + .iter() + .any(|h| h.chunk.to_lowercase().contains("grimwald")), + "{who} must still read the unrestricted doc" + ); + } + assert!( + anon.query("grimwald", 10) + .expect("anon open-doc query") + .iter() + .any(|h| h.chunk.to_lowercase().contains("grimwald")), + "an anonymous requester must still read the unrestricted doc" + ); +}