G1: fence the ingest→ACL chain at the pipeline seam, mark G1/G2/G9 shipped

.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<RawDocument>` deviation from the planned `Stream<Document>` 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.

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<Document>`). 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<Timestamp>) -> Result<Vec<RawDocument>> }` (`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<RawDocument>`, not `Stream<Document>`.** 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**.
 

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<RawDocument> {
+    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<dyn StorageAdapter> = 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::<Vec<_>>()
+    );
+    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"
+    );
+}