docs(specs): Knowledge Provider Interface v1 — connecting any knowledge base - #184
Closed
zaridan wants to merge 4 commits into
Closed
docs(specs): Knowledge Provider Interface v1 — connecting any knowledge base#184zaridan wants to merge 4 commits into
zaridan wants to merge 4 commits into
Conversation
The public, free-core contract by which any knowledge base -- first-party module, third-party product, or an operator's own wiki -- can serve as a knowledge source for Helpthread. Per the maintainer, 2026-08-02: "we are not going to limit Helpthread to using our KB module only." Pull-based: Helpthread queries providers at request time and stores no content. Core-side indexing was rejected because it duplicates operator knowledge, needs a resident indexing process the deployment posture rules out, and would advantage providers matching the core's indexing assumptions. The load-bearing invariant is that internal knowledge never reaches a customer, protected by two independent gates: the provider filters on a required audience parameter, and the core drops any result whose audience classification is not permitted in the requesting context. Gate two exists because gate one is implemented by software the project does not control. Missing or malformed classification fails closed. Capability declaration keeps the minimum viable provider genuinely small -- one endpoint, whole documents, no scores -- which is the concrete test of extensibility without privilege. Translation grouping is explicitly an optional provider capability, never a prerequisite. Draft for maintainer review. Four open questions in section 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eview Two adversarial passes found the first draft's central safety claim was false. It promised "two independent gates" against internal content reaching customers; both gates rested on the same provider-supplied audience label, so a provider that mislabelled internal content defeated both at once. Withdrawn. Replaced with registration-level exposure profiles routed by where an answer can end up rather than who is asking. A provider registers as customer_safe or agent_only; the profile is immutable and no request parameter can widen it. Any Assistant that can produce customer-directed output gets customer_safe sources only, regardless of its read access -- the auto-answers module reads with agent credentials and writes toward customers, so "the reader is trusted" was wrong for it. The boundary is the credential's reach, not the URL. customer_safe is the minimum useful provider, which keeps the cheap path the safe one. Section 2.3 states what this does and does not guarantee, because no contract can stop a provider publishing the wrong thing. Other corrections: the "minimum viable provider" was not required to return any article text and so could not serve its purpose; the widget's safe path was asserted but never specified; customer-path caching could serve reclassified content and is removed; metadata, counts, scores, and translation-group keys leaked existence and are stripped from the customer projection; the spec claimed to leave the substrate unchanged while changing it; admin authorization was understated; replay protection was advisory; the charter was overstated to justify the pull design; and private address ranges were forbidden while an operator's own wiki was named as supported. Section 2.7 records a gap this spec does not close: Assistants can read internal conversation notes, so a safe knowledge path does not deliver a general "internal never reaches a customer" property. Maintainer decisions, 2026-08-02: the corrected safety split; and module-first with no third-party connectors, but with machine-readable schemas and a conformance suite the first-party module passes unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A third adversarial pass confirmed the narrowed safety guarantee holds and that the factual claims about shipped code and the charter check out. It found two further errors, both mine: Secret rotation did not work as written. The core signs and the provider verifies, so the core retaining a previous secret achieved nothing -- a provider still holding the old secret would have rejected every request. The core now emits both signatures during the overlap window, the multi-signature scheme Stripe uses. The claim that the core "never sends conversation content" was false. The query field is caller-supplied and a consumer may build it from a conversation. Narrowed to the core's own request construction, with the consumer obligation stated explicitly, because the absolute version would have been relied on. Also fixed: Assistant callers had no assigned projection or caching class, leaving a path where a cached result outlives its article's reclassification and reaches customer output; knowledge:read_internal is unreachable under the shipped Assistant model, since every Assistant can post drafts, and is now documented as such rather than shipped as a dead grant; the no-leak invariant is narrowed to explicit metadata with cardinality, ordering, URLs, and timing named as residual signals; the conformance suite is marked a planned deliverable rather than an existing one; and a new section lists what remains unspecified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
conformance requirement A review of section 2 specifically as a public commitment judged the design defensible and the disclosure posture appropriate, but found the wording still read as a stronger promise than delivered. Section 2 no longer opens with "internal knowledge must not reach a customer" -- that reads as a whole-product confidentiality guarantee, which section 2.7 says outright Helpthread does not deliver. It is now scoped to results returned through this interface. Section 2.3 is restated as a security property required for conformance rather than a guarantee about shipped code. Nothing has been built, and categorical "never" language under a heading reading "what this actually guarantees" is written assurance language a reader could try to rely on notwithstanding the AGPL warranty disclaimer. "Customer-egress path" is now defined. Agent inbox search is explicitly not one: an Agent can read an internal article and paste it into a reply, and no interface prevents that. The real line is automated or direct customer egress versus a trusted human workflow, which the earlier "route by where the answer can end up" slogan blurred. Also: section 2.6's invariants are scoped to customer-egress credentials, gain the missing no-cache invariant, and cover every customer-egress path rather than only the widget; section 2.7 no longer offers human review as a confidentiality control, which contradicted section 2.2's own reasoning; section 5.4 no longer claims the residual signals disclose only existence; and "no contract can prevent that" is narrowed to what this contract cannot do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change adds a draft v1 contract for provider-neutral knowledge retrieval. It defines provider administration, authenticated retrieval, consumer and widget routes, exposure controls, caching, conformance requirements, and unresolved implementation details. ChangesKnowledge provider v1
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/modules/knowledge-provider-v1.md`:
- Around line 561-570: Expand “5.3 The widget route” into an implementable
public contract by naming the route and defining its request and response
schemas, credential class, CORS/CSRF requirements, and error behavior; preserve
the stated customer_safe-only projection and empty response when no provider is
configured. If these details are not decided for v1, explicitly mark the widget
route as an unresolved decision instead of presenting an incomplete contract.
- Around line 65-68: Clarify the v1 caching contract in the non-goals section
and the agent-path cache definition: explicitly classify provider-result cache
entries as ephemeral operational data rather than core-side content storage, and
specify retention limits, eviction triggers, access isolation, and deletion
behavior. Ensure the rules reconcile the no-storage/no-mirroring scope with the
cache while preserving provider filtering boundaries.
- Around line 256-263: Update the knowledge_providers registration schema to
include allow_private_network and the setting that enables
context.conversationId, documenting each field’s default, mutability, audit
requirements, and PATCH behavior. Apply the same definitions consistently to the
related registration, retrieval, and update sections referenced by the comment,
using the existing §3.2 and §4.3 terminology and preserving their security
semantics.
- Around line 436-448: Resolve the contradictory audience contract across the
specification: choose either to keep audience required and update §4.2 to record
a contract failure and reject responses missing it, or make audience optional
and align §2.5 and §8.2 with the documented per-result drop behavior. Ensure the
required-fields statement and the optional-field rules consistently describe the
same outcome.
- Around line 399-404: The documentation’s POST privacy claim is too strong
because query text can still appear in request-body captures. Update the
affected sections around the core request-construction guarantee to state only
that POST excludes query text from URLs and referrers, and explicitly require
redaction or suppression of request bodies for this route in logs, proxies,
tracing, and error reports.
- Around line 3-8: Update the knowledge-provider specification prose to refer to
extension artifacts consistently as “Modules,” including the cited sections,
while preserving existing machine-readable field names such as module and any
allowed legal or charter quotations.
- Around line 413-434: Update the §4.4 provider response example and the related
definitions at §4.4, §5.5, and the references around lines 461-463 and 619-623
to clearly separate provider-produced fields from core-computed locale metadata.
Define for both returnedLocale and localeFallback their producer, exact response
location, requiredness, and client projection; remove returnedLocale from the
provider payload if the core owns it, and ensure all examples and adapter/client
requirements use one consistent response shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 628e8ca1-8741-4e23-a9c0-02dcd20d3fc6
📒 Files selected for processing (1)
specs/modules/knowledge-provider-v1.md
Comment on lines
+3
to
+8
| Vocabulary (fixed, like Agents/Assistants/modules): a **knowledge provider** is any HTTP | ||
| endpoint implementing this contract. A **document** is one retrievable unit of knowledge in | ||
| one language. A **chunk** is an addressable passage of a document. Helpthread is always the | ||
| **consumer**; the provider is always the **source**. An **adapter** is a provider | ||
| implementation that fronts an existing product (Notion, Confluence, a static site) — §1.3 | ||
| explains why adapters are the normal case, not the exception. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Modules for extension artifacts.
The prose uses lowercase module or modules for extension artifacts. Use Modules consistently. Keep machine field names such as module unchanged when they are existing schema identifiers.
As per coding guidelines, call extension artifacts Modules, never plugins, except within the legal phrase plugin exception or charter quotations.
Also applies to: 35-41, 92-93, 144-146, 576-578, 703-717, 756-760
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/modules/knowledge-provider-v1.md` around lines 3 - 8, Update the
knowledge-provider specification prose to refer to extension artifacts
consistently as “Modules,” including the cited sections, while preserving
existing machine-readable field names such as module and any allowed legal or
charter quotations.
Source: Coding guidelines
Comment on lines
+65
to
+68
| ### 1.2 Non-goals for v1 | ||
|
|
||
| - **No core-side indexing, ingestion, or content storage.** | ||
| - **No write path.** Authoring, publishing, and management are provider capabilities |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Resolve the contradiction between the no-storage scope and agent caching.
Lines 65-68 prohibit core-side content storage and mirroring. Lines 661-667 permit agent-path cache entries containing filtered provider results. Clarify that this is an ephemeral operational cache, and define retention, eviction, access isolation, and deletion behavior. Otherwise implementers cannot determine whether caching violates the public contract or what privacy guarantees apply.
Also applies to: 661-667
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/modules/knowledge-provider-v1.md` around lines 65 - 68, Clarify the v1
caching contract in the non-goals section and the agent-path cache definition:
explicitly classify provider-result cache entries as ephemeral operational data
rather than core-side content storage, and specify retention limits, eviction
triggers, access isolation, and deletion behavior. Ensure the rules reconcile
the no-storage/no-mirroring scope with the cache while preserving provider
filtering boundaries.
Comment on lines
+256
to
+263
| `knowledge_providers` table: `id`, `name`, `base_url` (https; see §3.2), `exposure_profile` | ||
| (`customer_safe` | `agent_only`, required, immutable after creation — changing exposure | ||
| means a new registration), `secret` (server-generated, returned once, encrypted at rest via | ||
| the existing token-crypto AES-256-GCM envelope — signing needs the plaintext back), | ||
| `previous_secret` + `secret_rotated_at` (nullable; §3.1), `module text NULL` (attribution | ||
| slug, mirroring `assistants.module` / `webhook_endpoints.module` per substrate-v1 §1's | ||
| additive-forward rule), `capabilities jsonb`, `contract_version`, `priority int` (operator | ||
| ordering, §5.5), `status`, failure counters (§6.2), timestamps. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add all security-relevant registration options to the registration schema.
The knowledge_providers field list omits allow_private_network, although §3.2 says an operator can set it. It also omits the registration-level setting that enables context.conversationId, referenced in §4.3. Define both fields, their defaults, mutability, audit behavior, and PATCH semantics. Otherwise admin clients cannot persist or inspect controls that change SSRF and data-disclosure behavior.
Also applies to: 313-318, 389-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/modules/knowledge-provider-v1.md` around lines 256 - 263, Update the
knowledge_providers registration schema to include allow_private_network and the
setting that enables context.conversationId, documenting each field’s default,
mutability, audit requirements, and PATCH behavior. Apply the same definitions
consistently to the related registration, retrieval, and update sections
referenced by the comment, using the existing §3.2 and §4.3 terminology and
preserving their security semantics.
Comment on lines
+399
to
+404
| **What the core sends, stated accurately.** The core never *itself* adds conversation | ||
| content, message bodies, customer identity, or attachments to a retrieval request — the | ||
| fields above are the entire request. But `query` is caller-supplied text, and a caller can | ||
| put anything in it: an agent may paste a customer's words, and an automated consumer may | ||
| build a query from a conversation. **So the guarantee is on the core's own construction, not | ||
| on the bytes that ultimately leave.** |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not claim that POST removes query text from logs.
POST keeps query text out of URLs and referrers, but request bodies can still be captured by application logs, reverse proxies, tracing, and error reports. Replace this claim with the actual guarantee and require request-body redaction or body suppression for this route. This matters because query may contain caller-supplied conversation content.
Also applies to: 504-508
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/modules/knowledge-provider-v1.md` around lines 399 - 404, The
documentation’s POST privacy claim is too strong because query text can still
appear in request-body captures. Update the affected sections around the core
request-construction guarantee to state only that POST excludes query text from
URLs and referrers, and explicitly require redaction or suppression of request
bodies for this route in logs, proxies, tracing, and error reports.
Comment on lines
+413
to
+434
| ```json | ||
| { | ||
| "contractVersion": 1, | ||
| "results": [ | ||
| { | ||
| "documentId": "provider-defined-stable-id", | ||
| "title": "Resetting your password", | ||
| "snippet": "…the passage that matched, as plain text…", | ||
| "audience": "public", | ||
| "revision": "opaque-version-token", | ||
| "chunkId": "optional-passage-id", | ||
| "url": "https://help.example.com/de/passwort-zuruecksetzen", | ||
| "locale": "de-DE", | ||
| "score": 0.87, | ||
| "freshness": "current", | ||
| "reviewDueAt": "2026-11-01T00:00:00Z", | ||
| "translationGroupId": "optional-provider-defined-key" | ||
| } | ||
| ], | ||
| "returnedLocale": "de-DE" | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Separate provider response fields from core-computed locale metadata.
The §4.4 example places returnedLocale in the provider response, but Lines 461-463 say the core computes it and ignores provider input. Section 5.5 then introduces localeFallback without defining whether it is a result field or an envelope field. Define the producer, location, requiredness, and projection of both fields. Otherwise adapters and clients can implement incompatible response shapes.
Also applies to: 461-463, 619-623
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/modules/knowledge-provider-v1.md` around lines 413 - 434, Update the
§4.4 provider response example and the related definitions at §4.4, §5.5, and
the references around lines 461-463 and 619-623 to clearly separate
provider-produced fields from core-computed locale metadata. Define for both
returnedLocale and localeFallback their producer, exact response location,
requiredness, and client projection; remove returnedLocale from the provider
payload if the core owns it, and ensure all examples and adapter/client
requirements use one consistent response shape.
Comment on lines
+436
to
+448
| **Required on every result: `documentId`, `title`, `snippet`, `audience`.** | ||
|
|
||
| `snippet` is required because an earlier draft made it optional while claiming a | ||
| title-and-link-only provider was "fully supported." It is not: a provider returning no text | ||
| supplies nothing an AI consumer can ground an answer in and nothing an agent can read | ||
| without leaving the page. A contract whose stated minimum cannot serve its stated purpose is | ||
| a broken contract. `snippet` is **plain text** — providers send no markup, and the core | ||
| renders none (§4.6). | ||
|
|
||
| Optional fields, and what their absence means: | ||
|
|
||
| - **`audience`** — `public` | `internal`. Tripwire only (§2.3), not authorization. Missing | ||
| or unrecognised is treated as `internal` and dropped. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make audience requiredness and failure behavior consistent.
Line 436 makes audience required on every result. Lines 447-448 place it under optional fields and define missing values as internal and dropped. If missing audience makes the response malformed, §4.2 must record a contract failure and reject the response. If the intended behavior is to drop only that result, remove it from the required-field rule and align §2.5 and §8.2.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/modules/knowledge-provider-v1.md` around lines 436 - 448, Resolve the
contradictory audience contract across the specification: choose either to keep
audience required and update §4.2 to record a contract failure and reject
responses missing it, or make audience optional and align §2.5 and §8.2 with the
documented per-result drop behavior. Ensure the required-fields statement and
the optional-field rules consistently describe the same outcome.
Comment on lines
+561
to
+570
| ### 5.3 The widget route | ||
|
|
||
| The widget does **not** call §5's endpoint. It calls a distinct route with a distinct | ||
| credential class, on which: | ||
|
|
||
| - exposure is hard-wired to `customer_safe` — the concept of an agent-only provider does not | ||
| exist on this code path; | ||
| - there is no audience, provider-selection, or profile parameter in the request schema; | ||
| - the response uses the customer projection (§5.4); | ||
| - with no `customer_safe` provider configured, it returns empty (§2.5). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Give the widget route a concrete public contract.
This section says the widget uses a distinct route, but it does not name the route or define its request, response, credential, CORS/CSRF, or error behavior. The widget cannot implement this API from the current specification. Add the path and schemas, or explicitly defer the route as an unresolved v1 decision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/modules/knowledge-provider-v1.md` around lines 561 - 570, Expand “5.3
The widget route” into an implementable public contract by naming the route and
defining its request and response schemas, credential class, CORS/CSRF
requirements, and error behavior; preserve the stated customer_safe-only
projection and empty response when no provider is configured. If these details
are not decided for v1, explicitly mark the widget route as an unresolved
decision instead of presenting an incomplete contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🟡 NEEDS YOUR DECISION
Adds a public, free-core contract letting any knowledge base serve as a knowledge source. This is a public security model, and most of its design is INFERRED — one maintainer decision authorizes the interface's existence; the rest is this spec's own work and needs review before it becomes something others build against.
Spec only. No code, no schema migration, no route implemented.
Adversarial review: Codex, four passes — 30+ findings; the first invalidated the spec's central safety claim. All adjudicated and applied, summarized below rather than pasted. This was an additional pass, not a substitution: CodeRabbit is installed on this org and is reviewing this PR; its findings will be adjudicated before the verdict moves off 🟡.
What this adds
specs/modules/knowledge-provider-v1.md— provider registration, a retrieval contract, and one consumer-facing search API, so the agent inbox, the embeddable widget, and AI modules all reach knowledge the same way regardless of who provides it.Charter basis, Application and module model: "The knowledge base is one such application: the core may expose public knowledge interfaces and integration hooks, while authoring, publishing, presentation, and management remain module capabilities."
Review history — what the passes found
Worth stating plainly, because the spec changed shape twice:
snippetis now required (§4.4).Also fixed: customer-path caching could serve reclassified content; metadata and translation keys leaked the existence of internal articles; the spec claimed to leave the substrate unchanged while changing it; admin authorization was understated; replay protection was advisory; the charter was overstated to justify a design choice; private network ranges were forbidden while an operator's own wiki was advertised as supported.
Factual claims about shipped code — admin authorization,
signWebhookPayload, the DNS-timeout gap, the substrate capability set — and every charter quotation were verified against the tree.Decision provenance
Project decisions (verbatim provenance held privately, not published):
SELECTED: the corrected safety split — route by customer egress, isolate by credential reach,
customer_safeas the minimum provider; and machine-readable schemas plus a conformance suite shipping with the module.substrate-v1.md§3allow_private_networkas an admin-only escape hatchThis is a public contract in an AGPL repository. Once third parties build against it, changing it breaks them — that is the point of specifying it, and the reason to read §2 before merging.
A public security claim. §2 states a property about internal knowledge not reaching customers. It has been deliberately reframed as "a security property required for conformance" rather than a guarantee about shipped code, because nothing is built. The AGPL disclaims warranties "except when otherwise stated in writing," and a public document headed "what this guarantees" using absolute language is arguably that writing. Not legal advice; flagged because it is the kind of sentence that gets quoted back.
Do not market this as implemented or tested until the conformance suite exists.
It amends
substrate-v1.md§3 by adding the substrate's first named capabilities, default-deny.knowledge:read_internalis documented as unreachable in v1 — every Assistant can post drafts, so every Assistant is a customer-output principal and none may hold it.A gap this records but does not close
Assistants can already read internal conversation notes. So a conforming knowledge path does not deliver a general "internal information never reaches a customer" property — a drafting Assistant can disclose a note instead of an article.
Pre-existing in the shipped substrate, not created here. §2.7 records it; §10.2 recommends closing it before the auto-answers module ships, not before this interface.
What to read if you read one thing
§2, about two minutes. It is the security model, it is public, and it is the part that would be quoted back at us. Everything else is mechanics.
🤖 Generated with Claude Code
Summary by CodeRabbit