Repository navigation
Claude/r33 scope allowlist retrieval - #443
Conversation
`User::hasDocumentAccess()` has always honoured a membership's scope
allowlist, but enforcement was split: `AccessScopeScope` pushed only the
project constraint and deny-ACL rows into SQL and left the allowlist arm to
`KnowledgeDocumentPolicy::view()`.
That split holds for controller reads, which call the Gate. It does not hold
for retrieval: the RAG path resolves chunks with
`KnowledgeChunk::whereHas('document', ...)` and never calls the policy, so a
member scoped to `hr/policies/**` still received `hr/salaries/**` chunks as
grounding — with citations. Same shape as the H8 role-deny finding, one arm
later.
The allowlist is now SQL, so it reaches every query touching
`KnowledgeDocument`, retrieval and FTS included:
- globs translated by `App\Support\ScopeAllowlistSql` — because `KbPath`
gives `*` single-segment and `**` cross-segment reach, a bare LIKE would be
wrong (`%` crosses `/`), so globs without a cross-segment wildcard also
carry an exact separator count;
- tags as a correlated EXISTS, joined inside the document's own tenant
because slugs are unique only per (tenant_id, project_key) (R30);
- the two OR'd, matching `matchesScope()`; allowlists stay per membership,
so per project.
One glob shape — mixing single- and cross-segment wildcards — narrows to a
superset rather than an exact match; it never hides a readable document, and
`hasDocumentAccess()` remains authoritative. `ScopeAllowlistSql::isExact()`
reports which shape an entry has. A membership with no allowlist generates
the identical query as before, guarded by a regression test.
Also codifies this as review rule R33 — the first of the three long-reserved
slots to be claimed — and corrects the rule count/range the README stated in
three places (it claimed "40 rules (R1-R43)" while 43 rule sections existed,
R36-R46 among them).
Design only, status Proposed; no behaviour ships with this commit. Records the decision for two gaps that share one cause — the ingestion contract stores what a document *is*, never where its authority comes from: - a file shared with three people upstream becomes, on ingest, readable by everyone with the project; - ten connectors ingest internally-authored content while IMAP ingests content written by anyone who can send an email, which then reaches a tool-calling context with no boundary in between. Key decisions captured: opt-in capability interfaces rather than a required argument (eleven connectors and a public template consume this contract); `SourceAccess` carries principals, not decisions; an unresolvable principal fails closed to `restricted-unmapped` instead of falling back to project-wide visibility, which is the bug being fixed; mirrored ACL rows carry an `origin` column and reconciliation deletes on un-share, because a mirror that only adds is a slow leak; and provenance stays orthogonal to the Auto-Wiki curation tier of ADR 0014. Phased v8.32 (labels) / v8.33 (mirroring) / v8.34 (enforcement) so the cheap half is not held behind the expensive one. Enforcement extends the R33 SQL seam added in the previous commit. Surfaces the ADR where the others are already cited: a roadmap row in the README and the decisions narrative + dependency graph on the doc site. It was previously referenced from nowhere.
There was a problem hiding this comment.
🟡 Changes recommended
The new retrieval allowlist test currently inserts knowledge_document_tags rows without tenant_id, which is inconsistent with the test suite’s non-default TenantContext and should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR closes the “scope allowlist enforced only in policy” gap by pushing project_memberships.scope_allowlist (folder globs + tags) into SQL so it constrains retrieval (KnowledgeChunk::whereHas('document', …)) as well as controller reads, and documents the new R33 rule across internal and public docs.
Changes:
- Add
App\Support\ScopeAllowlistSqland wire it intoAccessScopeScopeso folder-glob allowlists are enforced in SQL (including segment-aware matching via separator counts). - Extend
AccessScopeScopeto apply per-project allowlist constraints (globs OR tags) in the globalKnowledgeDocumentscope. - Add regression coverage (
RetrievalScopeAllowlistTest) plus documentation updates (README, CLAUDE.md, docs-site) and a proposed ADR 0028 design doc.
File summaries
| File | Description |
|---|---|
| tests/Feature/Kb/RetrievalScopeAllowlistTest.php | New regression tests proving scope allowlists constrain both document listing and retrieval-shaped whereHas('document') chunk queries. |
| app/Support/ScopeAllowlistSql.php | New helper translating folder globs into portable SQL predicates (LIKE + separator counting; superset fallback for mixed-wildcard shape). |
| app/Scopes/AccessScopeScope.php | Push scope allowlist enforcement into the global scope: per-project arms + glob/tag constraints. |
| README.md | Document R33 behavior and update the codified-rule count references. |
| CLAUDE.md | Add the new R33 rule definition and update the populated-rule range references. |
| docs-site/architecture/security-and-threat-model.mdx | Public deep-doc explanation of why allowlists must reach retrieval, including the “one inexact glob shape” warning. |
| docs-site/projects-registry.mdx | Note that membership scope allowlists constrain retrieval, with a link to the threat model section. |
| docs-site/architecture/decisions.mdx | Register ADR 0028 (proposed) in the decisions index and update the decision graph links. |
| docs/adr/0028-source-acl-mirroring-and-ingest-provenance.md | New proposed ADR describing source ACL mirroring + provenance, building on the new “authz-in-SQL” seam. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $builder->where(function ($outer) use ($allowed, $column, $model, $user): void { | ||
| foreach ($allowed as $projectKey) { | ||
| $outer->orWhere(function ($arm) use ($projectKey, $column, $model, $user): void { | ||
| $arm->where($column, $projectKey); | ||
| $this->constrainByScopeAllowlist($arm, $model, $user, (string) $projectKey); |
Addresses the PR #443 review findings on the R33 scope-allowlist arm. AccessScopeScope asked User for the allowed project list and then for each project's scope_allowlist separately, so the global scope cost 1 + N membership queries on every KnowledgeDocument query — including the retrieval hot path, which reaches documents through whereHas('document'). User::allowedProjectScopes() now resolves the whole tenant-scoped map in a single query and the scope builds its per-project arms from it. Semantics are unchanged: the tenant filter (R30), the PROJECT_WILDCARD sentinel guard, and first-row-wins on duplicate memberships all behave as allowedProjects() plus allowedScopesFor() did. ScopeAllowlistSql spelled its LIKE escape clause with a backslash. That survives SQLite but dies on PostgreSQL through PDO with SQLSTATE[HY093] once a second placeholder is bound in the same statement, which is exactly this class's shape — the separator-count predicate binds one. Both the clause and the operand escaping now go through App\Support\LikeEscaper (escape character '~'), the R19 helper the NoBackslashLikeEscapeTest architecture test enforces. The knowledge_document_tags fixture inserted a row without tenant_id while the suite pins TenantContext to a non-default tenant, seeding a cross-tenant pivot row (R30/R31); it is now stamped explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
A glob mixing a cross-segment wildcard with a single-segment one has no
exact LIKE translation: the whole-string separator count that pins a
single-segment wildcard cannot coexist with a wildcard that crosses
separators. That shape previously fell back to the literal prefix before
the first wildcard, documented as a deliberate superset with the policy
named as the authoritative gate for it.
That reasoning is the one this rule exists to reject. A superset is safe
only when a later gate narrows it, and the retrieval path has none — it
resolves chunks through whereHas('document') and never calls
KnowledgeDocumentPolicy. So a membership scoped to a folder two levels
down collapsed to a prefix match on its first literal segment and admitted
every sibling folder underneath it, salaries included, as grounding the
model then cited. The narrowest correct answer for a constraint that
cannot be expressed is to grant nothing, per SEC-FAILCLOSED-001.
The arm is still emitted as an always-false predicate rather than omitted:
callers OR these together inside one group, and an empty group is dropped
by the query builder, which would read as "no restriction" and invert the
control entirely.
Failing closed is per glob, not per membership, so an allowlist pairing an
inexpressible glob with an expressible one still grants everything the
expressible one covers. Both halves are pinned by tests, including through
the retrieval-shaped query. literalPrefix() is removed with its only
caller, and the rule text, README, doc-site warning and scope docblock are
corrected to describe the behaviour that now ships.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
There was a problem hiding this comment.
🟡 Changes recommended
AccessScopeScope::constrainByProject() introduces avoidable OR-per-project predicates (vs whereIn) for unrestricted memberships, creating unnecessary hot-path query overhead.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| $scopes = $user->allowedProjectScopes(); | ||
|
|
||
| if ($allowed === []) { | ||
| if ($scopes === []) { | ||
| $builder->whereRaw('1=0'); | ||
| return; |
Picks up the MCP tool-count lock fix (#444) so this branch's PHPUnit run reflects its own changes rather than main's inherited failure.
Addressing the second Copilot review pass on PR #443. Pushing the allowlist into the global scope gave every allowed project its own OR arm, because a scope held in one project must not reach into another. Projects that carry no scope at all need no arm of their own, though, and emitting one anyway turned the pre-R33 `whereIn(project_key, …)` into a long OR chain on every KnowledgeDocument query — retrieval's whereHas('document') included — for the common subject who holds several memberships and no allowlist. R33 states that a subject with no restriction of this kind must generate the identical query as before, so this was the code contradicting its own documented promise. The membership map is now partitioned: projects without a scope collapse back into one IN list, and only the projects that actually carry globs or tags contribute an arm. A subject with no allowlist anywhere takes an early return and never builds the OR group at all. Covered by two tests: one asserting the generated SQL keeps an IN list and grows no `project_key = ?` arm, one proving a scope held in one project leaves an unscoped sibling project fully readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
Both would have been inherited by the implementation, since the ADR is the design authority for the v8.32 -> v8.34 work. The connector count was eleven. There are eight ingesting connectors (confluence, evernote, fabric, google-drive, imap, jira, notion, onedrive); `-base` is the framework and `-api` is the v8.27 live-tool paradigm whose ingest mode is still reserved. Seven of the eight carry content authored inside the organisation, with IMAP the one that does not — the asymmetry the ADR is built around, so the number it is stated with matters. README and the doc-site decisions index carried the same figures. The groundwork section claimed `knowledge_documents.provenance` already exists. It does not. The `provenance` columns in the schema are `kb_edges.provenance`, which records how an edge was derived, and the `chat_log_provenance` table, which maps answer tokens to source chunks — neither is authorship origin. Phase 1 therefore adds a column rather than reusing one, which is what the rollout plan already assumed; only the justification was wrong. The Auto-Wiki half of that claim stands: the reranker's human > auto > raw ordering is a real precedent for trust tiers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
There was a problem hiding this comment.
🟡 Changes recommended
The new tag-based allowlist paths (tests + AccessScopeScope) assume kb_tags/knowledge_document_tags have tenant_id columns, but the repo’s migrations currently define those tables without tenant_id, causing SQL errors on fresh databases/CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/Feature/Kb/RetrievalScopeAllowlistTest.php:300
- This test inserts
tenant_idintoknowledge_document_tagsand documents it as tenant-aware, but the migrations forknowledge_document_tagscurrently do not define atenant_idcolumn (database/migrations/2026_04_23_000003_create_knowledge_document_tags_table.php and tests/database/migrations/0001_01_01_000015_create_knowledge_document_tags_table.php). This will fail on insert in a fresh test DB. If the pivot is intended to be tenant-scoped, add the column + constraints in both prod and mirrored test migrations; otherwise remove the column usage and update the surrounding comment to match reality.
// R30/R31 — the pivot is tenant-aware, and the suite pins
// TenantContext to a non-default tenant. Letting the column fall back
// to its 'default' DB default would seed a cross-tenant pivot row.
\DB::table('knowledge_document_tags')->insert([
'tenant_id' => $this->tenantId,
'knowledge_document_id' => $doc->id,
'kb_tag_id' => $tagId,
]);
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
| $tagId = \DB::table('kb_tags')->insertGetId([ | ||
| 'tenant_id' => $this->tenantId, | ||
| 'project_key' => $this->projectKey, | ||
| 'slug' => $slug, | ||
| 'label' => $slug, | ||
| ]); |
| $sub->from('knowledge_document_tags') | ||
| ->join('kb_tags', 'kb_tags.id', '=', 'knowledge_document_tags.kb_tag_id') | ||
| ->whereColumn('knowledge_document_tags.knowledge_document_id', $idColumn) | ||
| ->whereColumn('kb_tags.tenant_id', $tenantColumn) | ||
| ->whereIn('kb_tags.slug', $tags) |
|
This one does not hold either — the premise skips a migration.
Migrations run in order, so by the time any test opens a connection the column is there. Both tables are also on the authoritative R30 tenant-aware list in CLAUDE.md, which is the contract the correlated subquery in The empirical check agrees: Worth saying that the shape of the concern was right: a tenant-scoped join against a table without |
There was a problem hiding this comment.
🟡 Changes recommended
The tag-based allowlist SQL correlation in AccessScopeScope needs tightening (project/tenant correlation in the EXISTS subquery) to avoid unintended access widening.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| $pathColumn = $model->qualifyColumn('source_path'); | ||
| $idColumn = $model->qualifyColumn('id'); | ||
| $tenantColumn = $model->qualifyColumn('tenant_id'); | ||
|
|
||
| $builder->where(function ($q) use ($globs, $tags, $pathColumn, $idColumn, $tenantColumn): void { | ||
| foreach ($globs as $glob) { | ||
| ScopeAllowlistSql::apply($q, $pathColumn, (string) $glob); | ||
| } | ||
|
|
||
| if ($tags !== []) { | ||
| // Mirrors User::documentHasAnyTag(): slugs are unique only | ||
| // per (tenant_id, project_key), so the join stays inside | ||
| // the document's own tenant (R30). | ||
| $q->orWhereExists(function ($sub) use ($tags, $idColumn, $tenantColumn): void { | ||
| $sub->from('knowledge_document_tags') | ||
| ->join('kb_tags', 'kb_tags.id', '=', 'knowledge_document_tags.kb_tag_id') | ||
| ->whereColumn('knowledge_document_tags.knowledge_document_id', $idColumn) | ||
| ->whereColumn('kb_tags.tenant_id', $tenantColumn) | ||
| ->whereIn('kb_tags.slug', $tags) | ||
| ->selectRaw('1'); | ||
| }); | ||
| } | ||
| }); |
A tag slug is unique per (tenant_id, project_key) — not globally. Both the policy check and the SQL that mirrors it correlated only the tag's tenant, so an identically-named tag belonging to a sibling project satisfied a membership's tag allowlist, and a pivot row stamped for another tenant could carry a document across the boundary since the join reaches kb_tags through it. The two matched each other exactly, which is why R33's "never wider than the policy" held and nothing caught it: they were equally wrong. Fixing only the scope would have made retrieval narrower than the Gate and started hiding documents the policy allows, so both move together — the pivot's tenant, the tag's tenant and the tag's project are now all correlated. Two tests, each targeting one hole: a same-named tag in a sibling project, and a pivot row stamped for another tenant while the tag itself is perfectly ordinary. Both fail with the correlations removed, which is what makes them worth having — the first version of the second test passed either way, because the tag's own tenant already blocked it. Found by review on PR #443. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
The row arrived with #443 and covers all three phases, so it moves to in-progress rather than shipped: provenance labelling is out, ACL mirroring (v8.33) and tool-firewall enforcement (v8.34) are not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX
…32) (#448) * feat(kb): add the provenance_tier column (ADR 0028 phase 1, WIP) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * feat(kb): record who authored ingested content (ADR 0028 phase 1) Ingestion recorded what a document is — title, path, mime type, project — and nothing about where the authority of its text comes from. The gap is invisible until you line up what the connectors actually do: Confluence, Jira, Notion, Drive, OneDrive, Evernote and Fabric read systems the organisation administers, so whoever wrote a document had to be granted the ability to write it. IMAP reads a mailbox, which accepts a message from anyone who knows the address. Both produce documents, both become chunks, both are retrieved as grounding on a platform that also exposes tools an agent can call — and until now both were stored as the same kind of fact. No deployment could say how much of its corpus was written by outsiders. A connector declares a ProvenanceTier through the opt-in DeclaresProvenance capability (connector-base ^1.5), resolved per installation. The host asks via instanceof, never by connector name, so a connector added tomorrow participates without a branch. The tier lands on knowledge_documents.provenance_tier at the single ingestion choke point, which means every path — connector, CLI walker, HTTP batch, GitHub action — is covered by one place. Tri-surface over one core (R44): `kb:provenance`, GET /api/admin/kb/provenance (R32 matrix row added), and the MCP KbProvenanceTool (roster 48 -> 49). All three delegate to ProvenanceInsightsService, which scopes to the active tenant (R30) and skips soft-deleted rows (R2). Three decisions that are easy to get backwards: Undeclared reads as the trusted default rather than as untrusted. A "safest" default would relabel every existing corpus overnight and make the label meaningless; external content is the exception a connector declares. The read-out reports `declared` next to the headline for exactly that reason — a corpus reading 100% internal with 0 declared has been assumed, not assessed, and the command says so out loud. A connector whose declaration throws fails CLOSED. Propagating would fail a document's ingestion over a label nothing yet enforces; returning null would read as trusted and silently promote the content. A wrong "untrusted" is recoverable and visible; a wrong "trusted" is noticed by nobody. Provenance is not the Auto-Wiki curation tier. That ranks whether a human vouched for something; this records who wrote it. A human-accepted page summarising an external email is both at once. This phase labels and enforces nothing, deliberately: enforcement (v8.34, an untrusted chunk may be quoted but must never influence a tool call) is only testable against a corpus that is already labelled. Doc-site page + README row ship here (R45). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * fix(kb): make the per-project breakdown agree with the headline `summary()` resolves every stored tier through `ProvenanceTier::fromStorage()`, which fails closed: a value this version does not recognise reads as untrusted. `byProject()` matched the literal `untrusted-external` instead, so a tier written by a newer release during a mixed deployment counted as externally authored in the headline and as internal in the breakdown directly beneath it — and mis-ordered the projects, on exactly the rows an operator is scanning for. The predicate is now the inverse: anything neither absent nor a recognised non-external tier. It is derived from the enum, so a tier added later is covered without editing the query, and the values are bound rather than interpolated. The command also clamps `--limit` to 200 like the HTTP and MCP surfaces. Three surfaces over one core are only worth the name if they answer the same question the same way; the one that forgot the bound was the way to pull the whole project table. Both covered by tests: an unrecognised tier counted identically in both views, and the breakdown bounded regardless of what the caller asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * feat(kb): take the connector releases that actually declare provenance Phase 1 only means something once the connectors ship their declaration: until now the host could store a tier but every connector was silent, so every document fell back to the default and the read-out reported an assumption rather than an assessment. connector-base ^1.4 -> ^1.5 (ProvenanceTier + DeclaresProvenance) imap ^1.5.1 -> ^1.6 (UntrustedExternal) confluence, evernote, fabric, google-drive, jira, notion, onedrive -> ^1.2 (TrustedInternal) The api connector also moves to v1.0.0-beta.3 for the uppercase-method fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * docs: align the docs.json nav entry indentation It sat at a different indent from every sibling in the pages array — valid JSON, but it reads as an accidental paste and makes the next diff noisier than it needs to be. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX * docs(readme): mark v8.32 shipped in the roadmap row The row arrived with #443 and covers all three phases, so it moves to in-progress rather than shipped: provenance labelling is out, ACL mirroring (v8.33) and tool-firewall enforcement (v8.34) are not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QPJodaHYMJ3wppek7aYTBX --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#470) The roadmap table jumped from v8.26.0 straight to v8.32→v8.34, and the changelog's newest entry was v8.30.0. Five versions had shipped without a roadmap row and five without a changelog entry, so the two tables that answer "what is in this product" disagreed with each other and with the code. Backfilled from verifiable sources rather than reconstructed from memory: merge dates come from the merge commits (#397 2026-07-10, #402 2026-08-24, #403/#404 2026-08-06, #424 2026-08-30, #443/#448/#452 2026-08-29, #455/#461 2026-08-30), and the substance from the ADRs that accompanied each cycle plus the CLAUDE.md sections written when the work landed. Where a changelog entry already existed (v8.27, v8.28, v8.30) the roadmap row summarises that entry rather than re-deriving the history. Roadmap gains v8.27.0, v8.28.0, v8.29.0, v8.30.0, v8.31.0 and v8.35.0, so the table now runs unbroken from v8.25 to the present. Changelog gains v8.29.0, v8.31.0, v8.32.0, v8.33.0, v8.34.0 and v8.35.0. v8.27.0 also stops claiming to be "in progress": PR #424 merged on 2026-08-30 and completed it, so the heading was three weeks stale and would have told a prospective consumer the API connector was unfinished. v8.35.0 is this cycle — laravel/ai 0.8→0.11 and laravel-flow 1.1→2.5 landing together, the flow surface closing its R32/R44/R45 gaps, and the empty-tenant fix that came out of review. The entry keeps the parts that are load-bearing for anyone upgrading: that the tenant on converted history was the real risk and why a `default` stamp there would have been undetectable, that four of the v2 breakages fail at boot rather than gradually, and that the first R32 test was theatre because routes register at boot from a flag the test flipped too late. R45 parity verified rather than assumed: every version documented here already has its deep doc-site page — connectors-api, team-management, case-study-email-datasets, system-administration, source-permissions, ingest-provenance, tool-firewall and flow-orchestration all exist. The doc-site was never what fell behind; the README was.
No description provided.