Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
e0d0f76
docs(search): plan the provider-list rework
graycyrus Sep 11, 2026
11ae5e7
feat(search): one credential slot per provider, not one per company
graycyrus Sep 11, 2026
2f9d948
feat(search): provider-list routes
graycyrus Sep 11, 2026
528ec78
feat(search): resolve the harness connection through the provider list
graycyrus Sep 11, 2026
e5148b5
feat(console): the Search provider list, at parity with the LLM page
graycyrus Sep 11, 2026
18bbe39
test(search): move the console tests onto the provider list
graycyrus Sep 11, 2026
e4a4cb3
docs(search): correct the plan where the code disagreed with it
graycyrus Sep 11, 2026
b123766
test(search): pin the harness seam, and say what the docs do not cover
graycyrus Sep 11, 2026
1a0657b
fix(search): the per-provider routes never matched their own path
graycyrus Sep 11, 2026
9fddeb1
test(search): register the new routes in the auth matrix
graycyrus Sep 11, 2026
2467796
test(auth-matrix): bless the snapshot for the six new search routes
graycyrus Sep 12, 2026
e8fc6f7
test(e2e): move the Search authority assertions onto the provider list
graycyrus Sep 12, 2026
55da91a
fix(console): one list testid across both states, and run prettier
graycyrus Sep 12, 2026
d12aafd
fix(search): the two CI failures, both mine
graycyrus Sep 12, 2026
86509e6
fix(search): Managed is always a row, including where it resolves to …
graycyrus Sep 12, 2026
3fea37b
fix(search): cap the probe body while reading it, not after
graycyrus Sep 12, 2026
730f6dd
fix(search): selecting managed actually stops searching through the a…
graycyrus Sep 12, 2026
22cb686
fix(search): serialise the provider index's read-modify-write per com…
graycyrus Sep 12, 2026
9270af1
fix(search): judge an instance hostname by what it resolves to, and p…
graycyrus Sep 12, 2026
79537cc
fix(search): a class this console cannot name must not become undefined
graycyrus Sep 12, 2026
4ae922d
fix(search): one validator for an instance address, applied on every …
graycyrus Sep 12, 2026
47a60fe
style(search): rustfmt the review-round changes
graycyrus Sep 12, 2026
2fcffe4
fix(search): four more from review — the claim, the ULA, the marker, …
graycyrus Sep 12, 2026
efa4351
Merge upstream/main into feat/search-providers
graycyrus Sep 12, 2026
c55b1b2
fix(search): the storing paths get the resolve check, and two more TO…
graycyrus Sep 12, 2026
1d0bbb5
fix(search): a check cannot aim a stored key elsewhere; legacy SearXN…
graycyrus Sep 14, 2026
4cc8fb4
fix(search): a Test that finishes after the row changed writes nothin…
graycyrus Sep 14, 2026
c71d8b1
Merge remote-tracking branch 'upstream/main' into feat/search-providers
graycyrus Sep 14, 2026
25ccfb0
fix(search): nothing stored and unlisted — removal order, bulk remova…
graycyrus Sep 14, 2026
67e6200
fix(search): an edit keeps its row's place; default and legacy writes…
graycyrus Sep 14, 2026
c7e8e9d
Merge upstream/main into feat/search-providers
graycyrus Sep 14, 2026
f34e571
fix(search): the legacy select is one critical section; validate befo…
graycyrus Sep 14, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions docs/modules/search/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Search providers

`Connections → API Keys → Search`: which index this company's teammates search
the web through.

This surface is being brought to the shape the LLM/inference surface reached in
`docs/modules/inference/` (landing in PR #2262, so those cross-references
resolve only once that branch merges) — a list of connected providers, an add-provider modal,
one credential per provider, one marked default, and per-provider controls. The
goal is that an operator who has used the LLM page finds this one obvious.

## The files

| File | What it answers |
|---|---|
| [`current-state.md`](current-state.md) | what exists today, and the one-key-slot bug that makes a list worth building |
| [`data-model.md`](data-model.md) | the record, where the credential lives, entry-zero convergence, what "default" means here |
| [`connect-flow.md`](connect-flow.md) | the page, the modal, the two dialogs, and the classified probe |
| [`catalogue.md`](catalogue.md) | the four providers — endpoints, auth headers, failure shapes |
| [`architecture.md`](architecture.md) | the module seams and how each is tested |
| [`known-defects.md`](known-defects.md) | what is deliberately **not** inherited from the inference design |

## The two rules that outrank the redesign

Both are already in the code and neither is negotiable.

**The key is per company and never from the environment.** A BYO search key is
billed to whoever pasted it, so an environment fallback would let one company's
searches ride on a credential somebody else pays for. With nothing stored the
company falls back to `managed`, which is metered and daily-capped against the
platform. The module header on `src/company/search.rs` is the canonical statement
and it survives this change unedited.

**The configuration surface is not feature-gated; the harness is.**
`src/harness/built_in/search_byo.rs` is behind `openhuman`;
`src/company/search` and `src/server/ops/search.rs` are always compiled, so a
build with no agent harness renders "this build has no search tools" rather than
a 404.

## The shortest statement of the change

One credential slot became many, keyed by the provider it authenticates.

Today a company has one `search/api_key` and a separate `search/provider` field
that selects which API it is presented to. Changing the provider without
re-pasting the key leaves the old key authenticating against the new provider,
and every layer — the status route, the console badge, the harness — agrees the
company is correctly configured until an agent's first search returns a 401 that
nothing on the page can explain.

Everything else here follows from fixing that honestly.
225 changes: 225 additions & 0 deletions docs/modules/search/architecture.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
# Architecture

How the code is arranged, and how each seam is tested. The organising rule is the
inference rework's, unchanged: **every decision this feature makes should be
testable without a host, a browser, or a network.**

That is not a testing preference. It is what keeps the interesting logic — which
failure class is this? which provider is active? is this instance URL safe to
fetch? — out of components and handlers where it can only be exercised end to
end.

## The seams

```
┌───────────────────────────────────────┐
HTTP ───────────────────│ server/ops/search.rs │ handlers only:
│ routes, extractors, DTO mapping │ auth + shape
└──────────────────┬────────────────────┘
┌────────────────────────────────┼────────────────────────────────┐
▼ ▼ ▼
┌───────────────────┐ ┌───────────────────┐ ┌───────────────────┐
│ search/store │ │ search/resolve │ │ search/probe │
│ │ │ │ │ │
│ list / put / │ │ active provider │ │ one search call │
│ delete / enable │ │ effective slug │ │ classify per │
│ credential slots │ │ default marker │ │ provider │
│ entry-zero + the │ │ │ │ address guard │
│ convergence write │ │ PURE │ │ │
│ │ │ │ │ IO at the edge, │
│ PURE over a port │ │ │ │ classifier PURE │
└───────────────────┘ └─────────┬─────────┘ └───────────────────┘
│ │
│ ├──────────────▶ harness/built_in/search_byo.rs
└──────────────┬───────────────┘ TenantSearch::resolve
▼ (feature = "openhuman")
┌───────────────────────┐
│ search/catalogue │ four entries, asserted against
│ (data, no behaviour) │ its TypeScript mirror
└───────────────────────┘
```

`src/company/search.rs` becomes `src/company/search/` with `mod.rs` keeping the
module header — the per-company-never-from-environment rule and the
configuration-is-not-feature-gated rule are the two things a reader must meet
first, and they must not move.

**Real modules, not `include!`d halves of one file.** That is the eighth defect
in the borrowed design, and the split here is on responsibility seams anyway.

### `catalogue` — data only

Four entries, no behaviour beyond lookup by slug.
[`catalogue.md`](catalogue.md) is the table.

**Tested by:** a table test asserting every entry has a parseable endpoint and a
known auth style, and a cross-language test that fails when the Rust table and
`frontend/src/search-providers/catalogue.ts` diverge.

### `store` — persistence over the `SecretStore` port

Reads and writes provider records and credential slots. Knows about entry zero —
the legacy flat `search/provider` / `search/api_key` / `search/endpoint` — and
about the `search/provider/<slug>/…` namespace. Knows nothing about HTTP or about
which provider is *right*.

**Tested by:** unit tests against an in-memory `SecretStore`. No host. The cases
that must exist: a company with nothing configured; entry zero round-trips; the
legacy flat keys still read after the change; adding a second provider; **a write
for the entry-zero slug clears the flat key it converged from**; deleting a
provider clears its credential; clearing is a write-of-empty because the port has
no delete; and the one that is the whole point — **two providers hold two
independent credentials, and writing one does not touch the other.**

### `resolve` — the decisions, all pure

```rust
fn active(providers: &[SearchProvider], marked: Option<&str>) -> Option<&SearchProvider>;
fn effective_slug(active: Option<&SearchProvider>, key_present: bool) -> &str;
```

`active` is the marker rule: the marked provider when it exists and is enabled,
else the first enabled one, else `None` — which means managed.
`effective_slug` is today's `effective_provider` with a list behind it, and it
stays **the one derivation** of "which index actually answers", called by the
status route, the capabilities panel and the harness alike. It is the function
whose doc comment already warns that two surfaces mirroring each other's rule
would drift.

**Tested by:** pure unit tests, no async. Marked-and-enabled, marked-but-disabled
falls through, marked-but-deleted falls through, nothing marked uses first
enabled, everything disabled resolves to managed, and a provider whose credential
is missing resolves to managed rather than reporting itself connected.

### `probe` — IO at the edge, classification pure

```rust
async fn probe(entry: &SearchProviderInfo, credential: Option<&str>, endpoint: Option<&str>)
-> Result<(), ProbeError>; // IO
fn classify(slug: &str, status: u16, body: &str) -> ProbeClass; // PURE
fn describe(class: ProbeClass, provider: &str) -> String; // PURE
```

`classify` takes the **slug** as well as the status and body, which is the
signature difference that matters: classification is per provider here, because
Brave signals a rejected key with `422` and a body code while everything else
uses `401`. See [`connect-flow.md`](connect-flow.md).

**Tested by:** a table test with a real response per provider per class —
Brave `422 SUBSCRIPTION_TOKEN_INVALID` asserting `auth`, Brave `403` asserting
`unknown` (a WAF, not a key), Exa `401 INVALID_API_KEY` asserting `auth`, Exa
`402` asserting `quota`, Querit's string-typed `error_code` asserting `auth`,
SearXNG `403` asserting `format` **not** `auth`, `407 Proxy Authentication
Required` asserting `unknown` not `auth`, and an id containing `1403` asserting
no match. Those are the cases the ordering and the per-provider dispatch exist
for.

The address guard **is** written here, against the plan's intention, and
[`connect-flow.md`](connect-flow.md) records why: `guard_link` in
`src/server/ops/memory_ingest.rs` refuses every private address including
`.internal` hostnames, which is exactly where a self-hosted SearXNG instance
lives, and it is `#[cfg(feature = "documents")]` while this surface is ungated.
`guard_instance_url` is therefore a narrower rule — metadata and link-local only —
rather than a copy of a stricter one.

### The harness seam barely moves

`TenantSearch::resolve` keeps its signature and its contract — `Ok(None)` means
"search through the managed surface", a store read failure is `Err` and not
`Ok(None)`, and `fingerprint` still covers the credential so a rotation rebuilds
the roster. What changes is its middle: it asks `search::resolve` for the active
provider instead of reading three flat keys.

`byo_search_tools` is untouched. `BYO_SEARCH_TOOLS` is untouched — it is the
closed set of tool names the `search` namespace must account for, and a provider
list does not add a name.

**This is the ordering hazard.** The harness reader, the capabilities reader and
the store's convergence write must land together; a console that writes the new
address while a reader is still on the flat keys silently drops the company to
managed search. [`data-model.md`](data-model.md) states it; the tests that hold
it are the harness resolve tests, which must be run under
`--features openhuman,mcp` and therefore must be named in a CI lane that enables
them (issue #770 — a feature-gated test nothing runs reports nothing).

## The console side

```
frontend/src/search-providers/
catalogue.ts the mirror; data only
types.ts SearchProvider, ProbeClass, the DTOs
resolve.ts active provider, row model, control set per kind PURE
classify.ts probe class -> copy PURE
connect.ts the add/connect state machine PURE
ProviderList.tsx the connected rows
AddProviderDialog.tsx
ProviderConnectDialog.tsx
use-search-providers.ts
```

`frontend/src/views/SearchView.tsx` becomes layout plus handlers and nothing
else. Every branch worth a test moves into `resolve.ts`, `classify.ts` or
`connect.ts` — including **which controls a row offers**, which is per kind and
is exactly the sort of conditional that rots inside a component:

```ts
controlsFor(row) // managed -> [] no toggle, no remove
// account -> [enable, test, replace key, remove, default]
// searxng -> [enable, test, edit address, remove, default]
```

"Remove key" is not offered where there is no key. That is the brief's rule and
it is one function.

## Testing

| Layer | Kind | Must cover |
|---|---|---|
| `catalogue` | unit + cross-language | every entry valid; Rust and TS agree |
| `store` | unit, in-memory port | entry zero, convergence clear, independent credentials, delete clears key |
| `resolve` | unit, pure | the marker rule, fall-through, managed fallback |
| `probe::classify` | unit, table | every provider's auth shape, the 422/403/407 traps, SearXNG `format` |
| address guard | unit | loopback, link-local, metadata, post-DNS recheck, redirect target |
| routes | integration | authority (admin vs scoped), **key never in any response body** |
| harness | unit, `--features openhuman,mcp` | active provider reaches `byo_search_tools`; fingerprint covers rotation |
| console pure | vitest | control sets per kind, copy selection, connect state machine |
| console UI | Playwright | the flows below |

### The test that must never be deleted

The leak test drives a real BYO key through every route and asserts no response
body contains it. **Extend it to each new route as that route is added**, before
there is anything to leak — and assert on the **value**, so a field rename cannot
make it pass.

### The browser flows

Not shipped as Playwright specs in this change — they were driven by hand
against a real host, and the list is here as the matrix that pass covers rather
than as a claim about CI. Adding them as specs needs a lane that selects them
(issue #475), which is a separate change.

1. Add an account provider with a good key → row appears, marked ok.
2. Add one with a bad key → rejected, **no row, no credential stored**.
3. Add one behind a non-auth failure → row created, amber advisory, **key kept**.
4. Add a SearXNG instance whose JSON output is off → row created, `format`
advisory naming `search.formats`, endpoint kept.
5. Add a second provider → both rows, both credentials independent.
6. Set the second as default → the effective provider changes and survives reload.
7. Delete the default → the marker moves and the UI says which provider is active now.
8. Disable a provider → it is not eligible as active, credential retained.

## Rules for the implementation

- **No decision in a component or a handler.** If it has a branch worth a test,
it belongs in a pure module beside the others.
- **No `Serialize` on anything holding a credential.** Private field, redacting
`Debug`, boolean on the DTO. `TenantSearch` is the existing example.
- **Additive DTO changes only.** `SearchStatus` is read by the capabilities panel
as well as this page. Add fields; do not reshape it.
- **`AdminScopedCompany` on every write, and on probe.** Reads stay
`ScopedCompany`, which is what the existing routes do.
- **Load-bearing comments.** Brave's 422, SearXNG's 403, the convergence clear
and the entry-zero special case each deserve one where the code enforces them.
- **Small commits on a seam.** One per module.
Loading
Loading