From f2a5718e49f195b11ce5afe5158614693f09d369 Mon Sep 17 00:00:00 2001 From: jwvanderstam Date: Wed, 11 Nov 2026 23:49:21 +0100 Subject: [PATCH 1/2] =?UTF-8?q?feat(plugins):=20PC-1=20=E2=80=94=20a=20man?= =?UTF-8?q?ifest,=20services,=20and=20the=20core's=20role=20gate=20on=20to?= =?UTF-8?q?ols?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plugin defines setup(services) and registers its tools through services.tools.register, importing nothing from src. Its manifest declares PLUGIN_SCOPE, PLUGIN_MIN_ROLE, PLUGIN_HOOKS and PLUGIN_CONFIG; a malformed one disables that plugin alone, and any tool it registered before failing is removed. Decided with the maintainer, departing from the ticket: - retrieval takes no workspace_id. It searches the workspace the request was authorised for; the tier is capped by PLUGIN_SCOPE and a workspace API key gets local only. - PLUGIN_MIN_ROLE is enforced by the core. The workspace guard records the caller's role (viewer/editor/owner, or admin), the chat route binds it for the stream, and the tool executor hides a tool below its role and refuses it if the model names it anyway. identity only reads. - storage and llm.complete are deferred until a consumer needs them. - A plugin without setup() still loads, listed as legacy, with a warning. Fixed on the way: a loader given an empty registry used the global one (an empty registry is falsy), and a reload snapshotted the tools before unloading the old ones, so it recorded none and a later unload left them registered. Co-Authored-By: Claude Opus 5.5 --- .claude/rules/file-map.md | 9 +- .claude/rules/plugins.md | 38 ++- CHANGELOG.md | 19 ++ CLAUDE.md | 2 +- docs/CONFIGURATION.md | 1 + docs/ROADMAP.md | 22 +- plugins/README.md | 98 +++++-- plugins/example_plugin.py | 99 ++++--- pyproject.toml | 4 + src/config.py | 12 + src/plugins/__init__.py | 74 +++++ src/plugins/manifest.py | 124 +++++++++ src/plugins/services.py | 156 +++++++++++ src/routes_fastapi/api_routes.py | 10 +- src/security_fastapi.py | 8 + src/tools/executor.py | 16 +- src/tools/plugin_loader.py | 51 +++- src/tools/registry.py | 5 + src/utils/caller.py | 68 +++++ src/utils/workspace.py | 14 + tests/unit/test_plugin_contract.py | 432 +++++++++++++++++++++++++++++ 21 files changed, 1144 insertions(+), 118 deletions(-) create mode 100644 src/plugins/__init__.py create mode 100644 src/plugins/manifest.py create mode 100644 src/plugins/services.py create mode 100644 src/utils/caller.py create mode 100644 tests/unit/test_plugin_contract.py diff --git a/.claude/rules/file-map.md b/.claude/rules/file-map.md index ec8d6226..c4ce62ca 100644 --- a/.claude/rules/file-map.md +++ b/.claude/rules/file-map.md @@ -174,8 +174,12 @@ Full module index for LocalChat. **Keep this current** — update in the same co | `src/tools/executor.py` | Ollama tool-call loop (multi-turn until final response) | | `src/tools/registry.py` | Tool registration with JSON schemas | | `src/tools/builtin.py` | Built-in tools: document search, calculator, datetime | -| `src/tools/plugin_loader.py` | Loads `.py` plugins from `plugins/` at startup | -| `plugins/example_plugin.py` | Reference plugin demo (`word_count`, `reverse_text` tools) | +| `src/tools/plugin_loader.py` | Loads `.py` plugins from `plugins/` at startup; one with `setup(services)` gets its manifest validated and its services built, one without loads as legacy. A failing plugin is disabled alone, with any tool it registered removed | +| `src/plugins/manifest.py` | PC-1 — `read_manifest()`: `PLUGIN_SCOPE`, `PLUGIN_MIN_ROLE`, `PLUGIN_HOOKS` (checked against the catalogue), `PLUGIN_CONFIG`; `PLUGIN_CONTRIBUTES` must be False. Raises `ManifestError` | +| `src/plugins/services.py` | PC-1 — the handles a plugin's `setup()` receives: `retrieval` (the request's workspace only, tier capped by the manifest), `llm.embed`, `config`, `identity` (read only), `tools.register` (refuses a taken name) | +| `src/plugins/__init__.py` | `build_services()` — types the plugin's settings from `PLUGIN__` and assembles its handles | +| `plugins/example_plugin.py` | Example plugin on the PC-1 contract — `setup(services)`, a manifest and a setting (`word_count`, `reverse_text` tools) | +| `tests/unit/test_plugin_contract.py` | PC-1 — the manifest (every malformed field refused by name), each handle (retrieval reads the request's workspace and refuses a tier beyond its reach or a key's), the executor hiding and refusing a tool below its role, the loader disabling one plugin alone and removing its half-registered tools, and the guard recording who it admitted | | `plugins/README.md` | Plugin authoring guide | | **Graph / Memory / Performance** | | | `src/graph/store.py` | `GraphStore` ABC + `PostgresGraphStore`, which reaches the entities through `db.entities` | @@ -242,6 +246,7 @@ Full module index for LocalChat. **Keep this current** — update in the same co | `tests/unit/test_injection_flags.py` | GR-1c — each kind is found by the text it names and stays quiet on the near-misses business documents carry (`System: 2`, "you are now logged in") | | `tests/unit/test_frontend_injection_flag_badge.py` | GR-1c — the Documents page badges a flagged document, names the kinds, and leaves an unflagged or pre-scan one alone | | `tests/integration/test_injection_fixtures.py` | GR-1's acceptance — eleven adversarial documents (text, Markdown, Word paragraph and table cell, spreadsheet cells, slide, white-on-white PDF) ingested for real: each flagged with its kind, a control not, the instruction kept inside its fence, nothing reaching memory extraction; PPTX speaker notes asserted not ingested | +| `src/utils/caller.py` | `RequestCaller` and `ROLE_LEVELS` (viewer < editor < owner < admin) — who the request was authorised as, bound for the chat stream like the scope, so the tool executor can check a tool's minimum role and a plugin's `identity` handle can read it (PC-1) | | `src/utils/text.py` | `STOP_WORDS` and `content_terms()` — a question's topic words, any script, operator-free; what the lexical arm searches for, and the stop-word set active learning shares | | **Infra / Config** | | | `requirements.in` | Runtime dependencies, hand-written — the input `requirements.txt` is compiled from | diff --git a/.claude/rules/plugins.md b/.claude/rules/plugins.md index 70ced308..50c9c50c 100644 --- a/.claude/rules/plugins.md +++ b/.claude/rules/plugins.md @@ -3,25 +3,20 @@ How plugins extend LocalChat without destabilising the core. Read alongside the "Plugin Contract" section in `CLAUDE.md` — that states the principle; this is the rule. -> ## ⚠ Status: designed, not built (verified 2026-08-27, re-checked 2026-10-06) -> -> **Everything below describes PC-1..PC-3, which have not been implemented.** They are -> ROADMAP Sprints 10–11, which now run after the quality plan (Initiative 12) has built the -> service layer they wrap. PC-4, the `core-without-plugins` job, was built on 2026-10-23 -> (QP-3). Apart from it, none of the following exists in the repository today: +> ## ⚠ Status: partly built (PC-1 built 2026-11-11; PC-4 2026-10-23) > > | Described here | Reality | > |---|---| -> | The services catalogue (`retrieval`, `llm`, `storage`, `config`, `identity`) | No service handles; a plugin imports `src.tools.registry` directly | -> | The hooks catalogue (`on_document_ingested`, `on_scheduler_tick`, …) | No hook bus | -> | The manifest (`PLUGIN_SCOPE`, `PLUGIN_MIN_ROLE`, `PLUGIN_HOOKS`, `PLUGIN_CONFIG`) | `plugin_loader.py` reads none of these; it reads an optional `PLUGIN_META` dict for display only | -> | `plugins/_echo/` reference plugin | Absent. `plugins/` holds `example_plugin.py` | +> | The manifest (`PLUGIN_SCOPE`, `PLUGIN_MIN_ROLE`, `PLUGIN_HOOKS`, `PLUGIN_CONFIG`) | **Built** (PC-1) in `src/plugins/manifest.py`; a malformed one disables that plugin only | +> | The services catalogue | **Built** (PC-1) in `src/plugins/services.py`: `retrieval`, `llm` (`embed` only), `config`, `identity`, and `tools` for registering. `storage` and `llm.complete` are deferred until a consumer needs them | +> | `PLUGIN_MIN_ROLE` | **Enforced by the core** (PC-1): the tool executor hides and refuses a tool below its minimum role | +> | The hooks catalogue (`on_document_ingested`, `on_scheduler_tick`, …) | No hook bus (PC-2). `PLUGIN_HOOKS` is validated against the names below and fires nothing | +> | `plugins/_echo/` reference plugin | Absent (PC-3). `plugins/` holds `example_plugin.py`, on the PC-1 contract | > | The `core-without-plugins` CI job | **Built** (2026-10-23) in `.github/workflows/tests.yml`; a required check since 2026-10-30 | > -> **What actually ships** is a much smaller thing, and it is documented accurately in -> [`plugins/README.md`](../../plugins/README.md): drop a `.py` file in `plugins/`, decorate -> functions with `@tool_registry.register`, and they become LLM-callable tools. That is the -> live contract. This file is the target. +> A plugin without `setup(services)` still loads as a *legacy* plugin that imports +> `src.tools.registry` directly. [`plugins/README.md`](../../plugins/README.md) documents +> what ships; the sections below are the target, and say where PC-1 departed from it. > > The document is kept in present tense below because it is a specification, and rewriting a > spec into the conditional makes it harder to build against. Read it as "shall", not "does". @@ -53,11 +48,12 @@ with a stable method surface and never imports the core module behind it. | Service | Surface the plugin gets | Behind it (plugin never touches) | |---------|------------------------|----------------------------------| -| `retrieval` | `retrieve(query, scope, workspace_id)` -> ranked chunks. `scope` is `local`/`global`/`hybrid`. | `src/rag/retrieval.py`, pgvector, reranker | -| `llm` | `complete(prompt, ...)`, `embed(text)` | `src/ollama_client.py`, model routing, warm-up | -| `storage` | A namespaced handle for the plugin's own tables; optional Clark-Wilson soft-delete helpers | `src/db/connection.py` pool, psycopg3 | -| `config` | Validated access to the plugin's declared keys + defaults | `src/config.py`, `.env` loading | -| `identity` | `require_role(min_role)` -> ok / 403 | `src/security_fastapi.py`, JWT claims | +| `retrieval` | `retrieve(query, knowledge_scope, top_k)` -> ranked chunks. `knowledge_scope` is `local`/`global`/`hybrid`, capped by `PLUGIN_SCOPE`; a workspace API key gets `local` only. **No workspace argument:** it searches the workspace the request was authorised for (decided 2026-11-11; the earlier `workspace_id` parameter would have let a plugin choose its target) | `src/rag/retrieval.py`, pgvector, reranker | +| `llm` | `embed(text)`. `complete(prompt, ...)` is deferred: tools run synchronously inside the chat stream, so a completion from one needs that changed first | `src/ollama_client.py`, model routing, warm-up | +| `storage` | *Deferred until a consumer needs it (2026-11-11).* A namespaced handle for the plugin's own tables; optional Clark-Wilson soft-delete helpers. Open: who migrates those tables, and whether RLS applies | `src/db/connection.py` pool, psycopg3 | +| `config` | Validated access to the plugin's declared keys + defaults, from `PLUGIN__` | `src/config.py` (`plugin_setting`), `.env` loading | +| `identity` | `current()` -> the caller, `has_role(min_role)` -> bool. Reads only: `PLUGIN_MIN_ROLE` is enforced by the core before a tool runs | `src/utils/caller.py`, bound from the workspace guard | +| `tools` | `register(name, description, parameters)`, tagged with the plugin and its minimum role; a taken name is refused | `src/tools/registry.py` | **Adding a service.** Only when a real consumer needs it, and only with a name that describes the capability generically. A `pricing_lookup` service is a leak; a @@ -84,8 +80,8 @@ Every plugin declares a manifest the core reads at load time. Minimal shape: ```python PLUGIN_SCOPE = "hybrid" # local | global | hybrid (retrieval reach) -PLUGIN_MIN_ROLE = "viewer" # viewer | user | admin (who may invoke) -PLUGIN_CONTRIBUTES = False # may it write to the Global Knowledge Base? +PLUGIN_MIN_ROLE = "viewer" # viewer | editor | owner | admin (who may invoke) +PLUGIN_CONTRIBUTES = False # may it write to the GKB? Must be False: no service writes PLUGIN_HOOKS = ["on_document_ingested", "on_scheduler_tick", "on_tool_invocation"] PLUGIN_CONFIG = { # keys the core loads into the config service "FEEDBACK_THRESHOLD": ("float", 0.30), diff --git a/CHANGELOG.md b/CHANGELOG.md index 73bca45b..2e3646c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,25 @@ reasoning attached, in [docs/history/LESSONS_LEARNED.md](docs/history/LESSONS_LE ## [Unreleased] +### Added + +- **Plugins get a manifest and services (PC-1).** A plugin defines `setup(services)` and + registers its tools through `services.tools.register`, importing nothing from `src`. Its + manifest declares how far its retrieval reaches (`PLUGIN_SCOPE`), who may use it + (`PLUGIN_MIN_ROLE`) and its settings (`PLUGIN_CONFIG`, read from + `PLUGIN__`). `services.retrieval` searches only the workspace the request was + authorised for. The core enforces the minimum role: a tool is not offered to a caller below + it, and is refused if the model names it anyway. A malformed manifest disables that plugin + alone. **Upgrading:** nothing to do. A plugin without `setup()` still loads as before, now + listed as legacy in `/api/plugins` with a warning in the log. + +### Fixed + +- **A failing plugin no longer leaves tools behind, and a reloaded one can be unloaded.** A + plugin that raised after registering a tool kept that tool. A reload recorded none of the + plugin's tools, so unloading it afterwards left them registered. And a loader given an + empty registry used the global one instead, since an empty registry is falsy. + ## [3.6.0] — 2026-11-11 The release of the QP-9 review's open findings (QP-10): R-8 to R-12 and item 13 are fixed, diff --git a/CLAUDE.md b/CLAUDE.md index b3ba4816..fa4f714b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,7 +156,7 @@ LocalChat supports plugins (LLM tools, connectors, background work) that extend The enforcing procedure is a CI job, `core-without-plugins`, that verifies architectural integrity: the core builds, lints, and passes its full test suite **with the `plugins/` directory and all private plugin code absent.** If that job goes red, a plugin dependency has leaked into the core. -> **It exists since 2026-10-23** (PC-4, built with QP-3): it deletes everything in `plugins/` but its README, then runs `ruff` and the fast suite. It has been a required check since 2026-10-30 (see Pull Requests). The rest of the contract — the services catalogue, the hook bus, the manifest, the `_echo` reference plugin — is likewise designed and unbuilt; what actually ships is the simpler tool-plugin loader described in [`plugins/README.md`](plugins/README.md). See the status banner in [.claude/rules/plugins.md](.claude/rules/plugins.md). +> **It exists since 2026-10-23** (PC-4, built with QP-3): it deletes everything in `plugins/` but its README, then runs `ruff` and the fast suite. It has been a required check since 2026-10-30 (see Pull Requests). The manifest and the services catalogue were built on 2026-11-11 (PC-1, `src/plugins/`), with `PLUGIN_MIN_ROLE` enforced by the core; `storage` and `llm.complete` wait for a consumer. The hook bus (PC-2) and the `_echo` reference plugin (PC-3) are designed and unbuilt. What ships is described in [`plugins/README.md`](plugins/README.md). See the status banner in [.claude/rules/plugins.md](.claude/rules/plugins.md). Full rules -> [.claude/rules/plugins.md](.claude/rules/plugins.md) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 0733cf03..1d093a8d 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -463,6 +463,7 @@ to empty, in which case the router falls back to the active model. > fixes this product at one node and one process; scale with a second node instead. | `LOG_FILE` | `logs/app.log` | Path for the file sink | | `PLUGINS_DIR` | `plugins` | Directory scanned for plugins | +| `PLUGIN__` | the plugin's declared default | A setting a plugin declares in `PLUGIN_CONFIG`. `` is its file name upper-cased, with anything but letters and digits made `_`. Read by `plugin_setting()` in `config.py`; see [plugins/README.md](../plugins/README.md) | | `PRESENCE_TTL_SECONDS` | `30` | How long a workspace presence entry stays live | | `CONNECTOR_LOCAL_ROOTS` | *(empty)* | Comma-separated absolute directories the `local_folder` connector may watch. **Empty disables that connector type** — see below | diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 18a44dc4..b6cd9cfc 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -594,7 +594,25 @@ The mechanism that lets plugins extend LocalChat without the core ever depending --- -### PC-1 — Service catalogue and manifest loader +### PC-1 — Service catalogue and manifest loader ✅ (built 2026-11-11) + +> **As built.** Four of the spec's points were decided differently by the maintainer on +> 2026-11-11, and `.claude/rules/plugins.md` was amended to match: +> - `retrieval` takes no `workspace_id`. It searches the workspace the request was +> authorised for, so a plugin cannot aim a search at one its caller was never admitted to. +> The tier is capped by `PLUGIN_SCOPE`, and a workspace API key gets `local` only. +> - `PLUGIN_MIN_ROLE` is enforced by the core, not by the plugin: the tool executor hides a +> tool from the model below its minimum role and refuses it if named anyway. The roles are +> the real ones (viewer < editor < owner, then admin); the workspace guard records the +> caller's, and the chat route binds it for the stream. `identity` only reads. +> `require_workspace_role_dep`, which the spec named, was deleted by QP-1. +> - `storage` is deferred until a consumer needs it, and so is `llm.complete`: tools run +> synchronously inside the chat stream, so a completion from one needs that changed first. +> - A plugin without `setup(services)` still loads, as *legacy*, so the release stays a minor. +> +> The built-in tools stay on the core directly: they are core code, and the contract governs +> plugins. Building it found two loader bugs, fixed here: an empty registry was swapped for +> the global one, and a reload recorded no tools, so a later unload left them registered. **Current foundation (do not rebuild from scratch).** The following already exists and works: - `src/tools/registry.py` — `ToolRegistry` with `@register` decorator, JSON-schema export, execute-by-name, `unregister` for hot-reload. @@ -2038,7 +2056,7 @@ the generation path is a deterministic pipeline unless an answer-level eval says | PG-0..PG-8 | **Production-grade hardening** ✅ — see [PRODUCTION_PLAN.md](history/PRODUCTION_PLAN.md). Gated everything below; **gate lifted 2026-08-31**. | done | | 8 | GKB-1 (schema + two-tier retrieval) ✅ 2026-10-09 — read-only global tier under RLS (0019) | 1 week | | 9 | GKB-2 (contribution workflow) ✓ backend 2026-10-19 (admin-only contribute, migration 0020), UI 2026-10-20 | 1 week | -| 10 | PC-1 + PC-2 (services, hooks, scheduler) | 1 week | +| 10 | PC-1 ✅ 2026-11-11 (manifest, services, the core's role gate) + PC-2 (hooks, scheduler) | 1 week | | 11 | PC-3 + PC-4 (echo plugin, CI gate) | 1 week | | 12 | PR-1 (pricing plugin — private repo) | 1–2 weeks | | 13 | CONN-1 (connector authorisation model — decision, no code) | 2–3 days | diff --git a/plugins/README.md b/plugins/README.md index 9fd813bd..255ce232 100644 --- a/plugins/README.md +++ b/plugins/README.md @@ -1,8 +1,8 @@ # LocalChat Plugin System -> Verified against `src/tools/` and `src/app_bootstrap.py` at `a04da4d` on 2026-11-10. This -> is the plugin mechanism that ships; the contract in `.claude/rules/plugins.md` is the -> designed, unbuilt target. +> Verified against `src/tools/`, `src/plugins/` and `src/app_bootstrap.py` on 2026-11-11. +> PC-1 of the contract in `.claude/rules/plugins.md` ships: the manifest and the services +> below. The hook bus (PC-2) and the `_echo` reference plugin (PC-3) do not exist yet. Drop a `.py` file here — LocalChat loads it automatically at startup. @@ -11,7 +11,7 @@ Drop a `.py` file here — LocalChat loads it automatically at startup. ## How it works 1. On startup, `PluginLoader.load_all()` scans this directory for `*.py` files (skips `_`-prefixed files). -2. Each file is dynamically imported. Any `@tool_registry.register` decorated functions in the file are registered as LLM-callable tools. +2. Each file is imported. If it defines `setup(services)`, the loader validates its manifest, builds its services and calls `setup`, where the plugin registers its tools. 3. The LLM can call your tools during chat just like the built-in ones (`search_documents`, `list_documents`, `calculate`, `get_current_datetime`). The tool loop runs only when retrieval found no document or web context for the question, and only with `TOOL_CALLING_ENABLED=true` (the default). No source-code changes needed — just drop a file and restart (or call `POST /api/plugins/reload`). @@ -21,40 +21,72 @@ No source-code changes needed — just drop a file and restart (or call `POST /a ## Plugin file contract ```python -# plugins/my_plugin.py +# plugins/my_plugin.py — imports nothing from src -from src.tools.registry import tool_registry +PLUGIN_META = {"name": "My Plugin", "version": "1.0.0", "description": "Shown in GET /api/plugins."} -# Optional — shown in GET /api/plugins -PLUGIN_META = { - "name": "My Plugin", - "version": "1.0.0", - "author": "Your Name", - "description": "What this plugin does.", +# The manifest. Every name is optional; these are the defaults. +PLUGIN_SCOPE = "local" # retrieval reach: local | global | hybrid +PLUGIN_MIN_ROLE = "viewer" # who may use its tools: viewer | editor | owner | admin +PLUGIN_HOOKS = [] # validated against the catalogue; nothing fires them until PC-2 +PLUGIN_CONFIG = { # KEY: (type, default); type is str | int | float | bool + "TOP_K": ("int", 5), } -@tool_registry.register( - name="my_tool", # unique name the LLM uses - description="What this tool does.", # shown to the LLM - parameters={ # JSON Schema for arguments - "type": "object", - "properties": { - "input": {"type": "string", "description": "The input value."}, - }, - "required": ["input"], - }, -) -def my_tool(input: str) -> str: - return f"You passed: {input}" + +def setup(services): + top_k = services.config.get("TOP_K") + + @services.tools.register( + name="my_tool", # unique; a name already taken is refused + description="What this tool does.", # shown to the LLM + parameters={"type": "object", "properties": {"q": {"type": "string"}}, "required": ["q"]}, + ) + def my_tool(q: str) -> str: + hits = services.retrieval.retrieve(q, top_k=top_k) + return "\n".join(h["text"] for h in hits) ``` +A malformed manifest, a setting that does not parse, or a `setup` that raises disables +**that plugin only**: the error shows in `GET /api/plugins`, any tool it registered before +failing is removed, and the application starts normally. + +### The services + +| Handle | What it does | What it cannot do | +|---|---|---| +| `services.tools.register(name, description, parameters)` | Registers a tool, tagged with this plugin and its `PLUGIN_MIN_ROLE` | Replace a tool that already exists | +| `services.retrieval.retrieve(query, knowledge_scope="local", top_k=5)` | Searches the documents the current request may read; returns `text`, `filename`, `chunk_index`, `similarity`, `tier` per chunk | Name a workspace: it always searches the one the request was authorised for. A tier beyond `PLUGIN_SCOPE`, or any but `local` for a workspace API key, raises `PermissionError` | +| `services.llm.embed(text)` | The embedding the core would store for `text` | Chat completion: not offered yet (see below) | +| `services.identity.current()`, `.has_role(role)` | Who is asking, and at what role | Grant anything: it only reads | +| `services.config.get(KEY)` | A declared setting, typed, from `PLUGIN__` or its default | Read anything the manifest did not declare | + +`PLUGIN__` is the plugin's file name upper-cased, with anything but letters and +digits made `_`: `TOP_K` for `my_plugin.py` is `PLUGIN_MY_PLUGIN_TOP_K`. + +**Who may use a tool is the core's decision.** A caller below `PLUGIN_MIN_ROLE` is never +offered the plugin's tools, and if the model names one anyway it is refused as an unknown +tool. Roles run viewer < editor < owner (the caller's role in the workspace, or a workspace +API key's) and then admin (a global administrator). + +**Not offered yet:** `storage` (tables of the plugin's own) and `llm.complete()`. Each waits +for a plugin that needs it. Tools run synchronously inside the chat stream, so a completion +from inside one needs that changed first. `PLUGIN_CONTRIBUTES` must be `False`. + +### Legacy plugins + +A file without `setup(services)` loads the way every plugin did before PC-1: it imports +`src.tools.registry` and registers at import time. It still works, is listed with +`"legacy": true` in `GET /api/plugins`, and logs a warning. Its tools are open to every +role. Move it to `setup(services)` — `example_plugin.py` shows the result. + --- ## API endpoints | Endpoint | Method | Description | |---|---|---| -| `/api/plugins` | `GET` | List all loaded plugins and their tools — admin only | +| `/api/plugins` | `GET` | List loaded plugins with their tools, manifest, `legacy` flag and any load error — admin only | | `/api/plugins/reload` | `POST` | Hot-reload all plugins from disk (no restart needed) — admin only | --- @@ -65,18 +97,24 @@ def my_tool(input: str) -> str: |---|---|---| | `PLUGINS_ENABLED` | `true` | Set to `false` to disable plugin loading | | `PLUGINS_DIR` | `plugins` | Directory to scan (relative to repo root) | +| `PLUGIN__` | the manifest's default | A plugin's declared setting | --- ## Rules -- Tool names must be **unique** across all plugins and builtins. A plugin that reuses an existing name **replaces** it, with only a warning logged — including a built-in such as `search_documents`, whose workspace scoping the replacement would then have to re-implement. Treat a plugin as code with the application's full authority (SECURITY.md, risk 4). +- The contract governs dependencies, not trust. A plugin runs **in the application's + process** and Python cannot stop it importing `src` or anything else. Treat a plugin as + code with the application's full authority (SECURITY.md, risk 4). What the contract buys + is that the core never depends on a plugin, and that a well-behaved plugin cannot reach a + workspace or a role its caller does not have. +- Tool names must be **unique**. Through `services.tools.register` a taken name is refused. + A legacy plugin that reuses a name still **replaces** it, with only a warning logged. - Plugin files starting with `_` (e.g. `__init__.py`) are **skipped**. -- Plugins run in the same process — they have full access to the Python environment. -- Keep tool handlers **fast and side-effect-free** where possible; the LLM waits for the result. +- Keep tool handlers **fast**: they run inside the chat stream, and the LLM waits for the result. --- ## Example -See [`example_plugin.py`](example_plugin.py) for a working two-tool example (`word_count`, `reverse_text`). +See [`example_plugin.py`](example_plugin.py) for a working two-tool example (`word_count`, `reverse_text`) with a manifest and a setting. diff --git a/plugins/example_plugin.py b/plugins/example_plugin.py index 4021f37d..e988e681 100644 --- a/plugins/example_plugin.py +++ b/plugins/example_plugin.py @@ -6,8 +6,10 @@ LocalChat discovers and loads it automatically on the next startup, or immediately via ``POST /api/plugins/reload``. -Required: at least one ``@tool_registry.register`` decorated function. -Optional: a ``PLUGIN_META`` dict for display in ``GET /api/plugins``. +A plugin defines ``setup(services)`` and registers its tools there, through +``services.tools.register``. It imports nothing from ``src``: everything it needs +from the core arrives on ``services``. The ``PLUGIN_*`` names below are its manifest; +each is optional and shown with its default. See ``plugins/README.md``. To write your own plugin: 1. Copy this file to ``plugins/my_plugin.py``. @@ -15,66 +17,63 @@ 3. Restart or call ``POST /api/plugins/reload`` — done. """ -from src.tools.registry import tool_registry - # --------------------------------------------------------------------------- -# Metadata (optional — shown in GET /api/plugins) +# Manifest # --------------------------------------------------------------------------- PLUGIN_META = { "name": "Example Plugin", - "version": "1.0.0", + "version": "2.0.0", "author": "LocalChat Team", - "description": "Demonstrates the plugin system with two sample tools.", + "description": "Demonstrates the plugin contract with two sample tools.", } +PLUGIN_SCOPE = "local" # retrieval reach: local | global | hybrid +PLUGIN_MIN_ROLE = "viewer" # who may use its tools: viewer | editor | owner | admin +PLUGIN_CONFIG = { # read from PLUGIN_EXAMPLE_PLUGIN_; the default applies when unset + "MAX_CHARS": ("int", 10_000), +} -# --------------------------------------------------------------------------- -# Tool 1 — word_count -# --------------------------------------------------------------------------- -@tool_registry.register( - name="word_count", - description=( - "Count the number of words in the provided text. " - "Useful when the user asks how long a passage or document excerpt is." - ), - parameters={ - "type": "object", - "properties": { - "text": { - "type": "string", - "description": "The text whose words should be counted.", +def setup(services): + max_chars = services.config.get("MAX_CHARS") + + @services.tools.register( + name="word_count", + description=( + "Count the number of words in the provided text. " + "Useful when the user asks how long a passage or document excerpt is." + ), + parameters={ + "type": "object", + "properties": { + "text": { + "type": "string", + "description": "The text whose words should be counted.", + }, }, + "required": ["text"], }, - "required": ["text"], - }, -) -def word_count(text: str) -> str: - """Return word and character counts for *text*.""" - words = len(text.split()) - chars = len(text) - return f"{words} word(s), {chars} character(s)." - - -# --------------------------------------------------------------------------- -# Tool 2 — reverse_text -# --------------------------------------------------------------------------- + ) + def word_count(text: str) -> str: + """Return word and character counts for *text*.""" + text = text[:max_chars] + return f"{len(text.split())} word(s), {len(text)} character(s)." -@tool_registry.register( - name="reverse_text", - description="Reverse the characters in the provided text.", - parameters={ - "type": "object", - "properties": { - "text": { - "type": "string", - "description": "The text to reverse.", + @services.tools.register( + name="reverse_text", + description="Reverse the characters in the provided text.", + parameters={ + "type": "object", + "properties": { + "text": { + "type": "string", + "description": "The text to reverse.", + }, }, + "required": ["text"], }, - "required": ["text"], - }, -) -def reverse_text(text: str) -> str: - """Return *text* with its characters in reverse order.""" - return text[::-1] + ) + def reverse_text(text: str) -> str: + """Return *text* with its characters in reverse order.""" + return text[:max_chars][::-1] diff --git a/pyproject.toml b/pyproject.toml index 1b98fe26..a464bcbc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -134,6 +134,10 @@ module = [ "src.services.users", "src.services.workspaces", "src.routes_fastapi._services", + "src.plugins", + "src.plugins.manifest", + "src.plugins.services", + "src.utils.caller", ] # `strict = true` is global-only; these are the per-module flags it turns on. disallow_untyped_defs = true diff --git a/src/config.py b/src/config.py index b52dc41b..1485b159 100644 --- a/src/config.py +++ b/src/config.py @@ -520,6 +520,18 @@ def _production_errors(errors: list[str]) -> None: # Directory (relative to repo root) that is scanned for .py plugin files. PLUGINS_DIR: str = os.environ.get('PLUGINS_DIR', 'plugins') + +def plugin_setting(plugin: str, key: str) -> str | None: + """The raw value of a plugin's declared setting, ``PLUGIN__``, or None. + + A plugin declares its keys and defaults in ``PLUGIN_CONFIG`` and never reads the + environment itself; the loader asks here and types the value (PC-1). The names are + built from the manifest, so they are documented as a family in CONFIGURATION.md + rather than one by one. + """ + name = "".join(c if c.isalnum() else "_" for c in plugin) + return os.environ.get(f"PLUGIN_{name}_{key}".upper()) + # ============================================================================ # DOCS SYSTEM CONFIGURATION # ============================================================================ diff --git a/src/plugins/__init__.py b/src/plugins/__init__.py new file mode 100644 index 00000000..ffbde4a8 --- /dev/null +++ b/src/plugins/__init__.py @@ -0,0 +1,74 @@ +"""The plugin contract's core side: the manifest and the services a plugin receives (PC-1).""" + +from __future__ import annotations + +from typing import Any + +from .. import config +from ..tools.registry import ToolRegistry +from .manifest import ( + HOOKS, + SCOPES, + ConfigValue, + Manifest, + ManifestError, + parse_setting, + read_manifest, +) +from .services import ( + ConfigHandle, + IdentityHandle, + LlmHandle, + PluginServices, + RetrievalHandle, + ToolsHandle, +) + + +def build_services( + name: str, + manifest: Manifest, + registry: ToolRegistry, + *, + processor: Any = None, # Any: DocumentProcessor; the singleton unless a test passes one + client: Any = None, # Any: OllamaClient; likewise +) -> PluginServices: + """Assemble the handles for plugin *name*. ManifestError when a setting does not parse.""" + values: dict[str, ConfigValue] = {} + for key, (type_name, default) in manifest.config.items(): + raw = config.plugin_setting(name, key) + try: + values[key] = default if raw is None else parse_setting(raw, type_name) + except ManifestError as exc: + raise ManifestError(f"setting {key}: {exc}") from exc + + if processor is None: + from ..rag import doc_processor as processor + if client is None: + from ..ollama_client import ollama_client as client + + return PluginServices( + name=name, + manifest=manifest, + config=ConfigHandle(values), + llm=LlmHandle(client), + retrieval=RetrievalHandle(processor, manifest.scope), + identity=IdentityHandle(), + tools=ToolsHandle(registry, source=name, min_role=manifest.min_role), + ) + + +__all__ = [ + "HOOKS", + "SCOPES", + "ConfigHandle", + "IdentityHandle", + "LlmHandle", + "Manifest", + "ManifestError", + "PluginServices", + "RetrievalHandle", + "ToolsHandle", + "build_services", + "read_manifest", +] diff --git a/src/plugins/manifest.py b/src/plugins/manifest.py new file mode 100644 index 00000000..1a4b7de4 --- /dev/null +++ b/src/plugins/manifest.py @@ -0,0 +1,124 @@ +"""A plugin's manifest: what it may reach, who may use it, and what it configures. + +Read from module-level names once the plugin file has been imported. Anything wrong +raises :class:`ManifestError`, and the loader disables that plugin and nothing else. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass, field +from types import ModuleType +from typing import Any + +from ..utils.caller import ROLE_LEVELS + +#: How far the plugin's retrieval reaches: its workspace, the global tier, or both. +SCOPES = ("local", "global", "hybrid") +#: The hooks PC-2 will fire. Named now so a manifest is validated against the catalogue. +HOOKS = ("on_document_ingested", "on_scheduler_tick", "on_tool_invocation", "on_route_mount") +_CONFIG_TYPES: dict[str, type] = {"str": str, "int": int, "float": float, "bool": bool} +_KEY = re.compile(r"^[A-Z][A-Z0-9_]*$") + +ConfigValue = str | int | float | bool + + +class ManifestError(ValueError): + """The manifest is malformed; the plugin is disabled.""" + + +@dataclass(frozen=True) +class Manifest: + scope: str = "local" + min_role: str = "viewer" + hooks: tuple[str, ...] = () + #: key -> (type name, default) + config: dict[str, tuple[str, ConfigValue]] = field(default_factory=dict) + + def to_dict(self) -> dict[str, Any]: + return { + "scope": self.scope, + "min_role": self.min_role, + "hooks": list(self.hooks), + "config": {k: {"type": t, "default": d} for k, (t, d) in self.config.items()}, + } + + +def read_manifest(module: ModuleType) -> Manifest: + """Validate the ``PLUGIN_*`` names on *module*; an absent one takes its default.""" + scope = getattr(module, "PLUGIN_SCOPE", "local") + if scope not in SCOPES: + raise ManifestError(f"PLUGIN_SCOPE must be one of {', '.join(SCOPES)}, not {scope!r}") + + min_role = getattr(module, "PLUGIN_MIN_ROLE", "viewer") + if min_role not in ROLE_LEVELS: + raise ManifestError( + f"PLUGIN_MIN_ROLE must be one of {', '.join(ROLE_LEVELS)}, not {min_role!r}" + ) + + # Writing to the Global Knowledge Base is an admin's decision (GKB-2), and no service + # lets a plugin write anything yet. Accepting True would promise what nothing honours. + contributes = getattr(module, "PLUGIN_CONTRIBUTES", False) + if contributes is not False: + raise ManifestError("PLUGIN_CONTRIBUTES must be False: no contribution service exists") + + hooks = getattr(module, "PLUGIN_HOOKS", []) + if not isinstance(hooks, (list, tuple)) or not all(isinstance(h, str) for h in hooks): + raise ManifestError("PLUGIN_HOOKS must be a list of hook names") + unknown = sorted(set(hooks) - set(HOOKS)) + if unknown: + raise ManifestError(f"PLUGIN_HOOKS names unknown hook(s): {', '.join(unknown)}") + + return Manifest( + scope=scope, + min_role=min_role, + hooks=tuple(hooks), + config=_read_config(getattr(module, "PLUGIN_CONFIG", {})), + ) + + +def _read_config(declared: object) -> dict[str, tuple[str, ConfigValue]]: + if not isinstance(declared, dict): + raise ManifestError("PLUGIN_CONFIG must be a dict of KEY: (type, default)") + out: dict[str, tuple[str, ConfigValue]] = {} + for key, spec in declared.items(): + if not isinstance(key, str) or not _KEY.match(key): + raise ManifestError(f"PLUGIN_CONFIG key {key!r} must be UPPER_SNAKE_CASE") + if not (isinstance(spec, tuple) and len(spec) == 2 and spec[0] in _CONFIG_TYPES): + raise ManifestError( + f"PLUGIN_CONFIG[{key!r}] must be (type, default) with type one of " + f"{', '.join(_CONFIG_TYPES)}" + ) + type_name, default = spec + if not _is_type(default, type_name): + raise ManifestError(f"PLUGIN_CONFIG[{key!r}] default {default!r} is not a {type_name}") + out[key] = (type_name, default) + return out + + +def _is_type(value: object, type_name: str) -> bool: + # bool is an int in Python; a flag must not pass as a number, nor a number as a flag. + if type_name == "bool": + return isinstance(value, bool) + if isinstance(value, bool): + return False + if type_name == "float": + return isinstance(value, (int, float)) + return isinstance(value, _CONFIG_TYPES[type_name]) + + +def parse_setting(raw: str, type_name: str) -> ConfigValue: + """Type an environment value for a declared key; raise ManifestError if it does not parse.""" + if type_name == "str": + return raw + if type_name == "bool": + lowered = raw.strip().lower() + if lowered in ("true", "1", "yes"): + return True + if lowered in ("false", "0", "no"): + return False + raise ManifestError(f"{raw!r} is not a bool") + try: + return int(raw) if type_name == "int" else float(raw) + except ValueError as exc: + raise ManifestError(f"{raw!r} is not a {type_name}") from exc diff --git a/src/plugins/services.py b/src/plugins/services.py new file mode 100644 index 00000000..8447fd95 --- /dev/null +++ b/src/plugins/services.py @@ -0,0 +1,156 @@ +"""The capabilities a plugin is handed at load time (PC-1). + +A plugin defines ``setup(services)`` and reaches the core only through the handles on +:class:`PluginServices`. It never imports ``src``. Each handle is named for what it does, +not for any plugin that uses it (`.claude/rules/plugins.md`). + +What a handle cannot do matters as much as what it can: + +* ``retrieval`` reads the workspace the request was authorised for. It takes no + workspace argument, so a plugin cannot point a search at a workspace the caller was + never admitted to (P0-1, P0-2). +* ``identity`` only reads. Whether the caller may use a plugin's tools at all is the + core's decision, made from ``PLUGIN_MIN_ROLE`` before the tool runs. +* There is no ``storage`` handle and no ``llm.complete`` yet. Both wait for a consumer + that needs them; see the status banner in `.claude/rules/plugins.md`. +""" + +from __future__ import annotations + +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + +from ..rag.retrieval import KnowledgeScope +from ..tools.registry import ToolRegistry +from ..utils.caller import RequestCaller, caller_has_role, current_request_caller +from ..utils.logging_config import get_logger +from ..utils.scope import current_request_scope +from .manifest import ConfigValue, Manifest + +logger = get_logger(__name__) + +#: The tiers each PLUGIN_SCOPE may ask for. +_REACH: dict[str, frozenset[str]] = { + "local": frozenset({"local"}), + "global": frozenset({"global"}), + "hybrid": frozenset({"local", "global", "hybrid"}), +} + + +class ConfigHandle: + """The plugin's own declared settings, typed, with their defaults applied.""" + + def __init__(self, values: dict[str, ConfigValue]) -> None: + self._values = dict(values) + + def get(self, key: str) -> ConfigValue: + """The value of a key the manifest declared; KeyError for one it did not.""" + if key not in self._values: + raise KeyError(f"{key!r} is not declared in PLUGIN_CONFIG") + return self._values[key] + + def as_dict(self) -> dict[str, ConfigValue]: + return dict(self._values) + + +class LlmHandle: + """Embeddings from the core's model server.""" + + def __init__(self, client: Any) -> None: # Any: OllamaClient, imported lazily by the loader + self._client = client + + def embed(self, text: str) -> list[float]: + """The embedding the core would store for *text*; RuntimeError when none can be made.""" + model = self._client.get_embedding_model() + if not model: + raise RuntimeError("no embedding model is available") + ok, vector = self._client.generate_embedding(model, text) + if not ok: + raise RuntimeError("the embedding could not be generated") + return list(vector) + + +class RetrievalHandle: + """Search the documents the current request may read.""" + + def __init__(self, processor: Any, reach: str) -> None: # Any: DocumentProcessor, lazily imported + self._processor = processor + self._allowed = _REACH[reach] + + def retrieve( + self, query: str, knowledge_scope: KnowledgeScope = "local", top_k: int = 5, + ) -> list[dict[str, Any]]: + """Ranked chunks for *query*, from the request's workspace and the tiers allowed. + + PermissionError for a tier beyond the manifest's PLUGIN_SCOPE, or any tier but + ``local`` for a workspace API key, refused rather than narrowed as GKB-3 does: + a narrowed answer could be taken for the one asked for. + """ + if knowledge_scope not in self._allowed: + raise PermissionError(f"PLUGIN_SCOPE does not reach the {knowledge_scope!r} tier") + caller = current_request_caller() + if knowledge_scope != "local" and (caller is None or caller.via_api_key): + raise PermissionError("the global knowledge tier is not open to this caller") + results = self._processor.retrieve_context( + query, top_k=top_k, scope=current_request_scope(), knowledge_scope=knowledge_scope, + ) + return [ + { + "text": r.chunk_text, + "filename": r.filename, + "chunk_index": r.chunk_index, + "similarity": r.similarity, + "tier": r.metadata.get("knowledge_tier", "local"), + } + for r in results + ] + + +class IdentityHandle: + """Who the current request is from. Read only.""" + + def current(self) -> RequestCaller | None: + """The caller, or None outside a request.""" + return current_request_caller() + + def has_role(self, min_role: str) -> bool: + """True when the caller is at least *min_role*: viewer < editor < owner < admin.""" + return caller_has_role(min_role) + + +class ToolsHandle: + """Register LLM-callable tools, tagged with this plugin and its minimum role.""" + + def __init__(self, registry: ToolRegistry, source: str, min_role: str) -> None: + self._registry = registry + self._source = source + self._min_role = min_role + + def register( + self, name: str, description: str, parameters: dict[str, Any], + ) -> Callable[[Callable[..., Any]], Callable[..., Any]]: + """Decorator, as ``tool_registry.register``. A name already taken is refused. + + Refused rather than overwritten: a plugin replacing ``search_documents`` would + decide what every chat's document search returns. + """ + if name in self._registry: + raise ValueError(f"tool {name!r} is already registered") + return self._registry.register( + name=name, description=description, parameters=parameters, + source=self._source, min_role=self._min_role, + ) + + +@dataclass(frozen=True) +class PluginServices: + """Everything a plugin may ask the core for, handed to its ``setup()``.""" + + name: str + manifest: Manifest + config: ConfigHandle + llm: LlmHandle + retrieval: RetrievalHandle + identity: IdentityHandle + tools: ToolsHandle diff --git a/src/routes_fastapi/api_routes.py b/src/routes_fastapi/api_routes.py index 89d9fde8..b3bd63fb 100644 --- a/src/routes_fastapi/api_routes.py +++ b/src/routes_fastapi/api_routes.py @@ -19,6 +19,7 @@ require_auth, ) from ..services import chat +from ..utils.caller import RequestCaller, bind_request_caller, reset_request_caller from ..utils.logging_config import get_logger from ..utils.scope import ( Scope, @@ -26,7 +27,7 @@ request_scope, reset_request_scope, ) -from ..utils.workspace import get_scope, get_workspace_id +from ..utils.workspace import get_caller, get_scope, get_workspace_id from ._authz import deny as _deny from ._authz import deny_additional as _deny_additional from ._services import chat_service @@ -195,6 +196,7 @@ async def _generate_sse( agent_result: Any, routed_rationale: str | None, workspace_scope: Scope, + caller: RequestCaller | None = None, ) -> AsyncGenerator[str, None]: full_response: list[str] = [] model_used = "local" @@ -203,6 +205,9 @@ async def _generate_sse( # the request rather than from their arguments. Unbound they refuse, which is # the point — an unscoped retrieval used to mean every workspace (audit C3). scope_token = bind_request_scope(workspace_scope) + # The caller too: a tool below its minimum role is hidden from the model and refused + # if called anyway, and a plugin reads who is asking through its identity handle. + caller_token = bind_request_caller(caller) try: if plan is not None: yield f"data: {json.dumps({'plan': plan.to_dict()})}\n\n" @@ -242,6 +247,7 @@ async def _generate_sse( logger.exception("[CHAT API] Unexpected error generating response") yield f"data: {json.dumps({'error': 'GenerationError', 'message': 'Failed to generate response', 'done': True})}\n\n" finally: + reset_request_caller(caller_token) reset_request_scope(scope_token) # also ensures cleanup on client disconnect @@ -403,7 +409,7 @@ async def api_chat(request: Request, chat_svc: Chat) -> Any: _generate_sse( plan, tool_executor, active_model, app_state, messages, fields, sources, cloud_client, conversation_id, agent_result, routed_rationale, - scope, + scope, get_caller(request), ), media_type="text/event-stream", headers={"Cache-Control": "no-cache", "X-Accel-Buffering": "no"}, diff --git a/src/security_fastapi.py b/src/security_fastapi.py index e73c019b..dc48ec81 100644 --- a/src/security_fastapi.py +++ b/src/security_fastapi.py @@ -491,6 +491,10 @@ def _check_api_key_access( # default workspace after authorising against the key's. request.state.resolved_workspace_id = key_workspace request.state.resolved_scope = key_workspace + # The role and identity a tool the model calls is checked against (PC-1). + request.state.resolved_role = key_role + request.state.resolved_user_id = None + request.state.resolved_via_api_key = True return None @@ -549,6 +553,8 @@ def check_workspace_access( # forgetting to pass anything (C1/C2). named = workspace_id or get_workspace_id(request) request.state.resolved_scope = named or ALL_WORKSPACES + request.state.resolved_role = "admin" + request.state.resolved_user_id = principal.user_id return None ws_id = workspace_id or get_workspace_id(request) db = getattr(request.app.state, "db", None) @@ -583,6 +589,8 @@ def check_workspace_access( # Set only after the checks pass: a refusal has no authorised workspace to name. request.state.resolved_workspace_id = ws_id request.state.resolved_scope = ws_id + request.state.resolved_role = role + request.state.resolved_user_id = principal.user_id return None diff --git a/src/tools/executor.py b/src/tools/executor.py index 92516d73..0b1cf1d4 100644 --- a/src/tools/executor.py +++ b/src/tools/executor.py @@ -25,6 +25,7 @@ from .. import config from ..ollama_client import OllamaClient +from ..utils.caller import caller_has_role from ..utils.logging_config import get_logger from .registry import ToolRegistry @@ -131,7 +132,11 @@ async def execute( Yields: Text chunks of the model's final answer. """ - schemas = self._registry.get_ollama_schemas() + # A tool above the caller's role is not offered to the model at all. + schemas = [ + s for s in self._registry.get_ollama_schemas() + if self._permitted(s["function"]["name"]) + ] if not schemas: async for chunk in self._client.generate_chat_response( model, messages, stream=stream @@ -240,6 +245,10 @@ def _append_tool_results( else: working_messages.append({"role": "tool", "content": result}) + def _permitted(self, name: str) -> bool: + spec = self._registry.get(name) + return spec is not None and caller_has_role(spec.min_role) + def _run_tool(self, name: str, arguments: dict[str, Any]) -> str: """Execute a single tool and return its result as a string.""" logger.info( @@ -247,7 +256,10 @@ def _run_tool(self, name: str, arguments: dict[str, Any]) -> str: f"{json.dumps(arguments, default=str)[:200]})" ) - if name not in self._registry: + # A model can name a tool it was never offered. Below the tool's minimum role + # the answer is the same as for a tool that does not exist, so the reply does + # not tell the caller what a higher role would unlock. + if name not in self._registry or not self._permitted(name): error = f"Unknown tool: {name}" logger.warning(f"[TOOLS] {error}") return error diff --git a/src/tools/plugin_loader.py b/src/tools/plugin_loader.py index 6809d718..0e4ed8eb 100644 --- a/src/tools/plugin_loader.py +++ b/src/tools/plugin_loader.py @@ -9,10 +9,16 @@ Plugin contract --------------- -A plugin file **must** be importable as a top-level Python module (i.e. it -can use absolute imports such as ``from src.tools.registry import -tool_registry``). The only required content is at least one -``@tool_registry.register`` decorated function. +A plugin defines ``setup(services)`` and may declare a manifest (``PLUGIN_SCOPE``, +``PLUGIN_MIN_ROLE``, ``PLUGIN_HOOKS``, ``PLUGIN_CONFIG``). The loader validates the +manifest, builds the services (``src/plugins``) and calls ``setup``, where the plugin +registers its tools through ``services.tools.register``. A malformed manifest, a setting +that does not parse, or a ``setup`` that raises disables that plugin only, and any tool +it registered before failing is removed. See ``plugins/README.md``. + +A plugin without ``setup`` is *legacy*: it imports ``src.tools.registry`` and registers +at import time, as every plugin did before PC-1. It still loads, with a warning, and is +listed as legacy. Optionally, a plugin can declare metadata at module level:: @@ -63,7 +69,9 @@ class PluginLoader: """ def __init__(self, registry: ToolRegistry | None = None) -> None: - self._registry = registry or tool_registry + # `is None`, not `or`: an empty registry has length 0, so `or` swapped a fresh + # one for the global singleton. + self._registry = registry if registry is not None else tool_registry # plugin_stem -> {"path": str, "tools": [name, ...], "meta": {...}, "error": str|None} self._plugins: dict[str, dict[str, Any]] = {} @@ -129,13 +137,14 @@ def load_file(self, path: Path) -> list[str]: path = Path(path).resolve() plugin_key = path.stem - # Snapshot existing tools before loading - before: set = set(self._registry.names) - # If reloading, cleanly remove the previous version first if plugin_key in self._plugins: self._unload_tools(plugin_key) + # Snapshot after the unload: taken before it, a reloaded plugin's own tools counted + # as already present, so it recorded none and a later unload left them registered. + before: set = set(self._registry.names) + # Ensure the repo root is on sys.path so plugins can do # ``from src.tools.registry import tool_registry`` repo_root = str(path.parent.parent) @@ -152,8 +161,12 @@ def load_file(self, path: Path) -> list[str]: try: spec.loader.exec_module(module) # type: ignore[union-attr] + manifest = self._set_up(plugin_key, module) except Exception: sys.modules.pop(module_name, None) + # A plugin that fails part-way must not leave half its tools behind. + for tool_name in set(self._registry.names) - before: + self._registry.unregister(tool_name) raise # Determine which tools this plugin registered @@ -178,6 +191,8 @@ def load_file(self, path: Path) -> list[str]: "tools": new_tools, "meta": meta, "error": None, + "legacy": manifest is None, + "manifest": manifest, } logger.info( @@ -254,6 +269,8 @@ def list_plugins(self) -> list[dict[str, Any]]: * ``tool_count`` — ``len(tools)`` * ``meta`` — contents of ``PLUGIN_META`` (may be ``{}``) * ``error`` — load error message, or ``None`` on success + * ``legacy`` — True for a plugin without ``setup(services)`` + * ``manifest`` — the validated manifest, or ``None`` for a legacy plugin """ return [ { @@ -263,6 +280,8 @@ def list_plugins(self) -> list[dict[str, Any]]: "tool_count": len(info["tools"]), "meta": info["meta"], "error": info["error"], + "legacy": info.get("legacy", False), + "manifest": info.get("manifest"), } for key, info in self._plugins.items() ] @@ -276,6 +295,22 @@ def loaded_count(self) -> int: # Internals # ------------------------------------------------------------------ + def _set_up(self, plugin_key: str, module: Any) -> dict[str, Any] | None: + """Read the manifest and call ``setup(services)``; None for a legacy plugin.""" + setup = getattr(module, "setup", None) + if not callable(setup): + logger.warning( + "[PLUGINS] '%s' has no setup(services): loaded as a legacy plugin, which " + "imports core modules directly. See plugins/README.md.", plugin_key, + ) + return None + + from ..plugins import build_services, read_manifest + + manifest = read_manifest(module) + setup(build_services(plugin_key, manifest, self._registry)) + return manifest.to_dict() + def _unload_tools(self, plugin_name: str) -> None: """Remove all tools registered by *plugin_name* from the registry.""" tools = self._plugins.get(plugin_name, {}).get("tools", []) diff --git a/src/tools/registry.py b/src/tools/registry.py index d6f6e3fc..4b17faaa 100644 --- a/src/tools/registry.py +++ b/src/tools/registry.py @@ -46,6 +46,8 @@ class ToolSpec: parameters: dict[str, Any] handler: Callable[..., Any] source: str = "builtin" + #: The lowest role that may call it; the executor hides and refuses it below that. + min_role: str = "viewer" # --------------------------------------------------------------------------- @@ -71,6 +73,7 @@ def register( description: str, parameters: dict[str, Any], source: str = "builtin", + min_role: str = "viewer", ) -> Callable[[Callable[..., Any]], Callable[..., Any]]: """ Decorator that registers a function as a tool. @@ -80,6 +83,7 @@ def register( description: Human-readable description shown to the model. parameters: JSON-Schema object describing accepted arguments. source: Origin label — ``"builtin"`` or the plugin stem name. + min_role: The lowest caller role that may use the tool. Returns: The original function, unmodified. @@ -94,6 +98,7 @@ def decorator(func: Callable[..., Any]) -> Callable[..., Any]: parameters=parameters, handler=func, source=source, + min_role=min_role, ) logger.debug(f"Registered tool: {name} (source={source!r})") return func diff --git a/src/utils/caller.py b/src/utils/caller.py new file mode 100644 index 00000000..48925a18 --- /dev/null +++ b/src/utils/caller.py @@ -0,0 +1,68 @@ +"""The caller of the current request, bound for code the model calls. + +A tool runs because the model asked for it, so it has no request to read the caller +from, the same reason the workspace scope is bound in :mod:`src.utils.scope`. The chat +route binds both for the life of its stream. A plugin reads the caller through its +``identity`` handle, and the tool executor checks each tool's minimum role against it. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from contextlib import contextmanager +from contextvars import ContextVar, Token +from dataclasses import dataclass + +#: Workspace roles in ascending order, then the global administrator above all of them. +ROLE_LEVELS: dict[str, int] = {"viewer": 0, "editor": 1, "owner": 2, "admin": 3} + + +@dataclass(frozen=True) +class RequestCaller: + """Who the request was authorised as, and at what role.""" + + #: None for a workspace API key, which is not a user. + user_id: str | None + #: The role the workspace guard admitted: a workspace role, or ``admin``. + role: str + via_api_key: bool = False + + def has_role(self, min_role: str) -> bool: + """True when this caller's role is at least *min_role*; an unknown role is never met.""" + if min_role not in ROLE_LEVELS: + return False + return ROLE_LEVELS.get(self.role, -1) >= ROLE_LEVELS[min_role] + + +_REQUEST_CALLER: ContextVar[RequestCaller | None] = ContextVar("localchat_request_caller", default=None) + + +def bind_request_caller(caller: RequestCaller | None) -> Token[RequestCaller | None]: + return _REQUEST_CALLER.set(caller) + + +def reset_request_caller(token: Token[RequestCaller | None]) -> None: + _REQUEST_CALLER.reset(token) + + +@contextmanager +def request_caller(caller: RequestCaller | None) -> Iterator[None]: + """Bind *caller* for the duration of the block, then restore the previous one.""" + token = _REQUEST_CALLER.set(caller) + try: + yield + finally: + _REQUEST_CALLER.reset(token) + + +def current_request_caller() -> RequestCaller | None: + """The bound caller, or None outside a request; callers treat None as the lowest role.""" + return _REQUEST_CALLER.get() + + +def caller_has_role(min_role: str) -> bool: + """True when the bound caller meets *min_role*. With no caller bound only ``viewer`` is met.""" + caller = _REQUEST_CALLER.get() + if caller is None: + return min_role == "viewer" + return caller.has_role(min_role) diff --git a/src/utils/workspace.py b/src/utils/workspace.py index 170a275d..0accbc91 100644 --- a/src/utils/workspace.py +++ b/src/utils/workspace.py @@ -4,6 +4,7 @@ from fastapi import Request +from .caller import RequestCaller from .scope import Scope @@ -44,3 +45,16 @@ def get_scope(request: Request) -> Scope: "guard before reading the scope" ) return scope # type: ignore[no-any-return] + + +def get_caller(request: Request) -> RequestCaller | None: + """Return who the workspace guard admitted and at what role, or None when no guard ran.""" + role = getattr(request.state, "resolved_role", None) + if not isinstance(role, str): + return None + user_id = getattr(request.state, "resolved_user_id", None) + return RequestCaller( + user_id=user_id if isinstance(user_id, str) else None, + role=role, + via_api_key=getattr(request.state, "resolved_via_api_key", False) is True, + ) diff --git a/tests/unit/test_plugin_contract.py b/tests/unit/test_plugin_contract.py new file mode 100644 index 00000000..46228da8 --- /dev/null +++ b/tests/unit/test_plugin_contract.py @@ -0,0 +1,432 @@ +"""PC-1 — the plugin contract's core side: manifest, services, and the role gate on tools. + +Every plugin file here is written into a temporary directory, never read from +``plugins/``: the ``core-without-plugins`` job deletes that directory, and the contract +has to hold without it. +""" + +from __future__ import annotations + +import textwrap +from types import ModuleType, SimpleNamespace +from typing import Any +from unittest.mock import MagicMock + +import pytest + +from src.plugins import ( + ManifestError, + RetrievalHandle, + ToolsHandle, + build_services, + read_manifest, +) +from src.plugins.manifest import parse_setting +from src.plugins.services import ConfigHandle, IdentityHandle, LlmHandle +from src.security_fastapi import check_workspace_access, create_access_token +from src.tools.executor import ToolExecutor +from src.tools.plugin_loader import PluginLoader +from src.tools.registry import ToolRegistry +from src.utils.caller import RequestCaller, caller_has_role, request_caller +from src.utils.scope import ScopeUnavailableError, request_scope +from src.utils.workspace import get_caller + +pytestmark = pytest.mark.unit + + +def _module(**names: Any) -> ModuleType: + mod = ModuleType("plugin_under_test") + for k, v in names.items(): + setattr(mod, k, v) + return mod + + +# --------------------------------------------------------------------------- +# Roles +# --------------------------------------------------------------------------- + +class TestRoleLadder: + def test_an_editor_meets_editor_but_not_owner(self): + editor = RequestCaller(user_id="u", role="editor") + assert (editor.has_role("viewer"), editor.has_role("editor"), editor.has_role("owner")) == ( + True, True, False, + ) + + def test_a_global_admin_meets_every_workspace_role(self): + admin = RequestCaller(user_id="u", role="admin") + assert all(admin.has_role(r) for r in ("viewer", "editor", "owner", "admin")) + + def test_an_owner_does_not_meet_admin(self): + assert RequestCaller(user_id="u", role="owner").has_role("admin") is False + + def test_an_unknown_caller_role_meets_nothing(self): + assert RequestCaller(user_id="u", role="wizard").has_role("viewer") is False + + def test_an_unknown_minimum_is_never_met(self): + """A typo in a minimum must lock the tool, not open it.""" + assert RequestCaller(user_id="u", role="admin").has_role("superuser") is False + + def test_with_no_caller_bound_only_viewer_is_met(self): + assert (caller_has_role("viewer"), caller_has_role("editor")) == (True, False) + + +# --------------------------------------------------------------------------- +# Manifest +# --------------------------------------------------------------------------- + +class TestManifest: + def test_an_empty_manifest_takes_every_default(self): + m = read_manifest(_module()) + assert (m.scope, m.min_role, m.hooks, m.config) == ("local", "viewer", (), {}) + + def test_declared_values_are_kept(self): + m = read_manifest(_module( + PLUGIN_SCOPE="hybrid", PLUGIN_MIN_ROLE="owner", + PLUGIN_HOOKS=["on_scheduler_tick"], PLUGIN_CONFIG={"LIMIT": ("int", 3)}, + )) + assert (m.scope, m.min_role, m.hooks, m.config) == ( + "hybrid", "owner", ("on_scheduler_tick",), {"LIMIT": ("int", 3)}, + ) + + @pytest.mark.parametrize("names, fragment", [ + ({"PLUGIN_SCOPE": "everywhere"}, "PLUGIN_SCOPE"), + ({"PLUGIN_MIN_ROLE": "user"}, "PLUGIN_MIN_ROLE"), + ({"PLUGIN_CONTRIBUTES": True}, "PLUGIN_CONTRIBUTES"), + ({"PLUGIN_HOOKS": "on_scheduler_tick"}, "PLUGIN_HOOKS must be a list"), + ({"PLUGIN_HOOKS": ["on_everything"]}, "unknown hook"), + ({"PLUGIN_CONFIG": [("LIMIT", "int", 3)]}, "must be a dict"), + ({"PLUGIN_CONFIG": {"limit": ("int", 3)}}, "UPPER_SNAKE_CASE"), + ({"PLUGIN_CONFIG": {"LIMIT": ("decimal", 3)}}, "(type, default)"), + ({"PLUGIN_CONFIG": {"LIMIT": ("int", "3")}}, "is not a int"), + ({"PLUGIN_CONFIG": {"LIMIT": ("int", True)}}, "is not a int"), + ({"PLUGIN_CONFIG": {"ON": ("bool", 1)}}, "is not a bool"), + ]) + def test_a_malformed_manifest_is_refused_naming_the_field(self, names, fragment): + with pytest.raises(ManifestError, match=__import__("re").escape(fragment)): + read_manifest(_module(**names)) + + def test_a_float_setting_accepts_an_integer_default(self): + assert read_manifest(_module(PLUGIN_CONFIG={"T": ("float", 1)})).config == {"T": ("float", 1)} + + @pytest.mark.parametrize("raw, type_name, expected", [ + ("yes", "bool", True), ("0", "bool", False), ("42", "int", 42), + ("0.25", "float", 0.25), ("plain", "str", "plain"), + ]) + def test_a_setting_is_typed(self, raw, type_name, expected): + assert parse_setting(raw, type_name) == expected + + @pytest.mark.parametrize("raw, type_name", [("maybe", "bool"), ("4.5", "int"), ("x", "float")]) + def test_a_setting_that_does_not_parse_is_refused(self, raw, type_name): + with pytest.raises(ManifestError): + parse_setting(raw, type_name) + + +# --------------------------------------------------------------------------- +# Services +# --------------------------------------------------------------------------- + +def _services(name: str = "my-plugin", **manifest: Any): + return build_services( + name, read_manifest(_module(**manifest)), ToolRegistry(), + processor=MagicMock(), client=MagicMock(), + ) + + +class TestConfigHandle: + def test_an_unset_key_takes_its_default(self, monkeypatch): + monkeypatch.delenv("PLUGIN_MY_PLUGIN_LIMIT", raising=False) + assert _services(PLUGIN_CONFIG={"LIMIT": ("int", 3)}).config.get("LIMIT") == 3 + + def test_the_environment_overrides_the_default_typed(self, monkeypatch): + """The plugin's stem is upper-cased and its dash made an underscore.""" + monkeypatch.setenv("PLUGIN_MY_PLUGIN_LIMIT", "7") + assert _services(PLUGIN_CONFIG={"LIMIT": ("int", 3)}).config.get("LIMIT") == 7 + + def test_a_value_that_does_not_parse_refuses_the_plugin(self, monkeypatch): + monkeypatch.setenv("PLUGIN_MY_PLUGIN_LIMIT", "seven") + with pytest.raises(ManifestError, match="setting LIMIT"): + _services(PLUGIN_CONFIG={"LIMIT": ("int", 3)}) + + def test_an_undeclared_key_is_a_key_error(self): + with pytest.raises(KeyError): + ConfigHandle({"A": 1}).get("B") + + +class _FakeProcessor: + def __init__(self) -> None: + self.calls: list[dict[str, Any]] = [] + + def retrieve_context(self, query: str, **kwargs: Any) -> list[Any]: + self.calls.append({"query": query, **kwargs}) + return [SimpleNamespace( + chunk_text="body", filename="a.md", chunk_index=2, similarity=0.9, + metadata={"knowledge_tier": "global"}, chunk_id=11, + )] + + +class TestRetrievalHandle: + def test_it_searches_the_workspace_the_request_was_authorised_for(self): + proc = _FakeProcessor() + with request_scope("ws-a"), request_caller(RequestCaller("u", "viewer")): + out = RetrievalHandle(proc, "local").retrieve("q", top_k=3) + assert proc.calls == [{"query": "q", "top_k": 3, "scope": "ws-a", "knowledge_scope": "local"}] + assert out == [{"text": "body", "filename": "a.md", "chunk_index": 2, + "similarity": 0.9, "tier": "global"}] + + def test_with_no_request_scope_it_refuses_rather_than_searching_everything(self): + proc = _FakeProcessor() + with pytest.raises(ScopeUnavailableError): + RetrievalHandle(proc, "local").retrieve("q") + assert proc.calls == [] + + def test_a_tier_beyond_the_manifest_scope_is_refused(self): + proc = _FakeProcessor() + with request_scope("ws-a"), request_caller(RequestCaller("u", "admin")): + with pytest.raises(PermissionError, match="PLUGIN_SCOPE"): + RetrievalHandle(proc, "local").retrieve("q", knowledge_scope="global") + assert proc.calls == [] + + def test_a_workspace_api_key_is_refused_the_global_tier(self): + proc = _FakeProcessor() + with request_scope("ws-a"), request_caller(RequestCaller(None, "owner", via_api_key=True)): + with pytest.raises(PermissionError, match="not open"): + RetrievalHandle(proc, "hybrid").retrieve("q", knowledge_scope="hybrid") + assert proc.calls == [] + + def test_a_user_within_the_manifest_scope_reaches_the_global_tier(self): + """Over-correction guard: the refusal above is about the key, not the tier.""" + proc = _FakeProcessor() + with request_scope("ws-a"), request_caller(RequestCaller("u", "viewer")): + RetrievalHandle(proc, "hybrid").retrieve("q", knowledge_scope="hybrid") + assert proc.calls[0]["knowledge_scope"] == "hybrid" + + +class TestLlmHandle: + def test_embed_returns_the_vector(self): + client = MagicMock() + client.get_embedding_model.return_value = "nomic" + client.generate_embedding.return_value = (True, [0.1, 0.2]) + assert LlmHandle(client).embed("hi") == [0.1, 0.2] + client.generate_embedding.assert_called_once_with("nomic", "hi") + + def test_a_failed_embedding_raises_rather_than_returning_empty(self): + client = MagicMock() + client.get_embedding_model.return_value = "nomic" + client.generate_embedding.return_value = (False, []) + with pytest.raises(RuntimeError): + LlmHandle(client).embed("hi") + + +class TestIdentityHandle: + def test_it_reads_the_bound_caller(self): + caller = RequestCaller("u-1", "editor") + with request_caller(caller): + handle = IdentityHandle() + assert (handle.current(), handle.has_role("editor"), handle.has_role("owner")) == ( + caller, True, False, + ) + + +class TestToolsHandle: + def test_a_tool_carries_the_plugin_and_its_minimum_role(self): + reg = ToolRegistry() + ToolsHandle(reg, source="p", min_role="owner").register("t", "d", {"type": "object"})(lambda: "x") + spec = reg.get("t") + assert (spec.source, spec.min_role) == ("p", "owner") + + def test_a_name_already_taken_is_refused_and_the_original_kept(self): + reg = ToolRegistry() + reg.register("search_documents", "core", {"type": "object"})(lambda: "core") + with pytest.raises(ValueError, match="already registered"): + ToolsHandle(reg, source="p", min_role="viewer").register("search_documents", "d", {}) + assert reg.get("search_documents").handler() == "core" + + +# --------------------------------------------------------------------------- +# The executor's role gate +# --------------------------------------------------------------------------- + +def _gated_registry(calls: list[str]) -> ToolRegistry: + reg = ToolRegistry() + reg.register("open_tool", "anyone", {"type": "object", "properties": {}})( + lambda: calls.append("open_tool") or "open") + reg.register("owner_tool", "owners", {"type": "object", "properties": {}}, min_role="owner")( + lambda: calls.append("owner_tool") or "secret") + return reg + + +class TestExecutorRoleGate: + async def test_a_tool_above_the_callers_role_is_not_offered(self): + offered: list[list[str]] = [] + + async def completion(model, messages, tools=None): + offered.append([t["function"]["name"] for t in tools or []]) + return {"message": {"content": "done"}} + + client = MagicMock() + client.generate_chat_completion = completion + executor = ToolExecutor(client, _gated_registry([])) + with request_caller(RequestCaller("u", "editor")): + _ = [c async for c in executor.execute("m", [{"role": "user", "content": "hi"}])] + assert offered == [["open_tool"]] + + def test_a_tool_called_below_its_role_is_refused_as_unknown(self): + calls: list[str] = [] + executor = ToolExecutor(MagicMock(), _gated_registry(calls)) + with request_caller(RequestCaller("u", "editor")): + result = executor._run_tool("owner_tool", {}) + assert (result, calls) == ("Unknown tool: owner_tool", []) + + def test_a_tool_called_at_its_role_runs(self): + calls: list[str] = [] + executor = ToolExecutor(MagicMock(), _gated_registry(calls)) + with request_caller(RequestCaller("u", "owner")): + result = executor._run_tool("owner_tool", {}) + assert (result, calls) == ("secret", ["owner_tool"]) + + def test_with_no_caller_a_viewer_tool_still_runs(self): + calls: list[str] = [] + assert ToolExecutor(MagicMock(), _gated_registry(calls))._run_tool("open_tool", {}) == "open" + + +# --------------------------------------------------------------------------- +# The loader +# --------------------------------------------------------------------------- + +_CONTRACT = ''' +PLUGIN_MIN_ROLE = "editor" +PLUGIN_CONFIG = {"GREETING": ("str", "hello")} + +def setup(services): + greeting = services.config.get("GREETING") + + @services.tools.register(name="greet", description="d", parameters={"type": "object"}) + def greet(): + return greeting +''' + +_LEGACY = ''' +from src.tools.registry import tool_registry + +@tool_registry.register(name="legacy_tool", description="d", parameters={"type": "object"}) +def legacy_tool(): + return "old" +''' + + +def _write(directory, name: str, body: str) -> None: + (directory / f"{name}.py").write_text(textwrap.dedent(body), encoding="utf-8") + + +class TestLoader: + def test_a_contract_plugin_registers_through_setup_with_its_manifest(self, tmp_path): + _write(tmp_path, "greeter", _CONTRACT) + reg = ToolRegistry() + loader = PluginLoader(registry=reg) + assert loader.load_all(tmp_path) == 1 + [info] = loader.list_plugins() + spec = reg.get("greet") + assert (info["legacy"], info["manifest"]["min_role"], info["tools"]) == (False, "editor", ["greet"]) + assert (spec.source, spec.min_role, spec.handler()) == ("greeter", "editor", "hello") + + def test_a_malformed_manifest_disables_only_that_plugin(self, tmp_path): + _write(tmp_path, "bad", 'PLUGIN_SCOPE = "everywhere"\ndef setup(services):\n pass\n') + _write(tmp_path, "greeter", _CONTRACT) + reg = ToolRegistry() + loader = PluginLoader(registry=reg) + assert loader.load_all(tmp_path) == 1 + by_name = {p["name"]: p for p in loader.list_plugins()} + assert "PLUGIN_SCOPE" in by_name["bad"]["error"] + assert reg.names == ["greet"] + + def test_a_setup_that_fails_part_way_leaves_no_tool_behind(self, tmp_path): + _write(tmp_path, "half", ''' +def setup(services): + services.tools.register(name="first", description="d", parameters={})(lambda: 1) + raise RuntimeError("second tool failed") +''') + reg = ToolRegistry() + loader = PluginLoader(registry=reg) + assert loader.load_all(tmp_path) == 0 + assert (reg.names, loader.list_plugins()[0]["error"]) == ([], "second tool failed") + + def test_a_plugin_without_setup_loads_as_legacy(self, tmp_path): + from src.tools.registry import tool_registry + + _write(tmp_path, "oldstyle", _LEGACY) + loader = PluginLoader(registry=tool_registry) + try: + loader.load_all(tmp_path) + [info] = loader.list_plugins() + assert (info["legacy"], info["manifest"], info["tools"]) == (True, None, ["legacy_tool"]) + finally: + loader.unload("oldstyle") + assert "legacy_tool" not in tool_registry + + def test_a_reload_still_records_the_plugins_tools(self, tmp_path): + """The snapshot used to precede the unload, so a reload recorded no tools and a + later unload left them registered.""" + _write(tmp_path, "greeter", _CONTRACT) + reg = ToolRegistry() + loader = PluginLoader(registry=reg) + loader.load_all(tmp_path) + assert loader.reload("greeter") == ["greet"] + loader.unload("greeter") + assert reg.names == [] + + +# --------------------------------------------------------------------------- +# The guard records who it admitted +# --------------------------------------------------------------------------- + +_USER = "88888888-8888-8888-8888-888888888888" + + +def _request(*, token: str | None = None, api_key: str | None = None, db: Any) -> MagicMock: + req = MagicMock() + headers: dict[str, str] = {} + if token: + headers["Authorization"] = f"Bearer {token}" + if api_key: + headers["X-API-Key"] = api_key + req.headers.get = lambda k, default="": headers.get(k, default) + req.cookies = {} + req.app.state.db = db + req.query_params = {} + req.path_params = {} + req.state = SimpleNamespace(resolved_workspace_id=None) + return req + + +def _db(*, member_role: str | None = "editor", global_role: str | None = "user") -> MagicMock: + db = MagicMock() + db.is_connected = True + db.members.role.return_value = member_role + db.members.for_user.return_value = [] + db.workspaces.default_id.return_value = "ws-1" + db.api_keys.resolve.return_value = ("ws-1", "viewer") + db.tokens.is_revoked.return_value = False + db.users.get_role.return_value = global_role + return db + + +class TestGuardRecordsTheCaller: + def test_a_member_is_recorded_with_their_workspace_role(self): + req = _request(token=create_access_token(_USER, {"role": "user"}), db=_db(member_role="owner")) + assert check_workspace_access(req, "ws-1", "viewer") is None + assert get_caller(req) == RequestCaller(user_id=_USER, role="owner", via_api_key=False) + + def test_a_global_admin_is_recorded_as_admin(self): + req = _request(token=create_access_token(_USER, {"role": "admin"}), db=_db(global_role="admin")) + assert check_workspace_access(req, "ws-1", "viewer") is None + assert get_caller(req) == RequestCaller(user_id=_USER, role="admin", via_api_key=False) + + def test_an_api_key_is_recorded_with_its_role_and_no_user(self): + req = _request(api_key="lcw_abc", db=_db()) + assert check_workspace_access(req, "ws-1", "viewer") is None + assert get_caller(req) == RequestCaller(user_id=None, role="viewer", via_api_key=True) + + def test_a_refused_caller_is_not_recorded(self): + req = _request(token=create_access_token(_USER, {"role": "user"}), db=_db(member_role=None)) + assert check_workspace_access(req, "ws-1", "viewer") is not None + assert get_caller(req) is None From 867bf4dfeaf6d73f76fa260748eeeab607a3234e Mon Sep 17 00:00:00 2001 From: jwvanderstam Date: Wed, 11 Nov 2026 23:54:11 +0100 Subject: [PATCH 2/2] =?UTF-8?q?fix(plugins):=20answer=20the=20security=20r?= =?UTF-8?q?eview=20of=20#541=20=E2=80=94=20L1=20and=20L2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit L1: a legacy plugin that overwrote an existing tool and then raised kept its handler, because rollback removed only new names. Rollback now restores the registry as it was before the load. L2: a plugin could keep its tools handle and register after setup() returned, leaving tools the loader never recorded. The handle closes when setup() returns. Tests for both fail on the previous commit. Also a test that a token claiming admin is recorded at the role the database holds. Co-Authored-By: Claude Opus 5.5 --- plugins/README.md | 1 + src/plugins/services.py | 7 +++++ src/tools/plugin_loader.py | 18 ++++++++--- tests/unit/test_plugin_contract.py | 48 ++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 4 deletions(-) diff --git a/plugins/README.md b/plugins/README.md index 255ce232..ec4c7690 100644 --- a/plugins/README.md +++ b/plugins/README.md @@ -109,6 +109,7 @@ role. Move it to `setup(services)` — `example_plugin.py` shows the result. is that the core never depends on a plugin, and that a well-behaved plugin cannot reach a workspace or a role its caller does not have. - Tool names must be **unique**. Through `services.tools.register` a taken name is refused. +- Register tools **inside `setup()`**. The handle closes when `setup()` returns, so the loader knows every tool a plugin owns and can unload it. A legacy plugin that reuses a name still **replaces** it, with only a warning logged. - Plugin files starting with `_` (e.g. `__init__.py`) are **skipped**. - Keep tool handlers **fast**: they run inside the chat stream, and the LLM waits for the result. diff --git a/src/plugins/services.py b/src/plugins/services.py index 8447fd95..4c8d379b 100644 --- a/src/plugins/services.py +++ b/src/plugins/services.py @@ -126,6 +126,11 @@ def __init__(self, registry: ToolRegistry, source: str, min_role: str) -> None: self._registry = registry self._source = source self._min_role = min_role + self._closed = False + + def close(self) -> None: + """Refuse registration from now on; the loader calls this when ``setup()`` returns.""" + self._closed = True def register( self, name: str, description: str, parameters: dict[str, Any], @@ -135,6 +140,8 @@ def register( Refused rather than overwritten: a plugin replacing ``search_documents`` would decide what every chat's document search returns. """ + if self._closed: + raise RuntimeError("tools are registered in setup(); this handle is closed") if name in self._registry: raise ValueError(f"tool {name!r} is already registered") return self._registry.register( diff --git a/src/tools/plugin_loader.py b/src/tools/plugin_loader.py index 0e4ed8eb..5362355a 100644 --- a/src/tools/plugin_loader.py +++ b/src/tools/plugin_loader.py @@ -144,6 +144,9 @@ def load_file(self, path: Path) -> list[str]: # Snapshot after the unload: taken before it, a reloaded plugin's own tools counted # as already present, so it recorded none and a later unload left them registered. before: set = set(self._registry.names) + # The whole table, not just the names: a legacy plugin can overwrite a built-in + # before it fails, and rolling back only new names would keep its handler. + previous = dict(self._registry._tools) # Ensure the repo root is on sys.path so plugins can do # ``from src.tools.registry import tool_registry`` @@ -164,9 +167,10 @@ def load_file(self, path: Path) -> list[str]: manifest = self._set_up(plugin_key, module) except Exception: sys.modules.pop(module_name, None) - # A plugin that fails part-way must not leave half its tools behind. - for tool_name in set(self._registry.names) - before: - self._registry.unregister(tool_name) + # A plugin that fails part-way must not leave half its tools behind, nor a + # tool it overwrote replaced. + self._registry._tools.clear() + self._registry._tools.update(previous) raise # Determine which tools this plugin registered @@ -308,7 +312,13 @@ def _set_up(self, plugin_key: str, module: Any) -> dict[str, Any] | None: from ..plugins import build_services, read_manifest manifest = read_manifest(module) - setup(build_services(plugin_key, manifest, self._registry)) + services = build_services(plugin_key, manifest, self._registry) + try: + setup(services) + finally: + # A tool registered later, from a kept handle, would be one the loader never + # recorded: listed nowhere, and left behind by unload and reload. + services.tools.close() return manifest.to_dict() def _unload_tools(self, plugin_name: str) -> None: diff --git a/tests/unit/test_plugin_contract.py b/tests/unit/test_plugin_contract.py index 46228da8..6f8d8125 100644 --- a/tests/unit/test_plugin_contract.py +++ b/tests/unit/test_plugin_contract.py @@ -363,6 +363,46 @@ def test_a_plugin_without_setup_loads_as_legacy(self, tmp_path): loader.unload("oldstyle") assert "legacy_tool" not in tool_registry + def test_a_failing_legacy_plugin_does_not_keep_a_builtin_it_overwrote(self, tmp_path): + """Security review of #541, L1: rolling back only new names kept the handler a + legacy plugin had put over an existing tool before it raised.""" + from src.tools.registry import tool_registry + + tool_registry.register("probe_builtin", "core", {"type": "object"})(lambda: "core") + _write(tmp_path, "hijack", ''' +from src.tools.registry import tool_registry + +@tool_registry.register(name="probe_builtin", description="d", parameters={}) +def probe_builtin(): + return "plugin" + +raise RuntimeError("fails after overwriting") +''') + try: + assert PluginLoader(registry=tool_registry).load_all(tmp_path) == 0 + assert tool_registry.get("probe_builtin").handler() == "core" + finally: + tool_registry.unregister("probe_builtin") + + def test_a_tools_handle_kept_past_setup_cannot_register(self, tmp_path): + """Security review of #541, L2: a tool registered after setup() would be one the + loader never recorded, so unload and reload would leave it behind.""" + _write(tmp_path, "keeper", ''' +KEPT = [] + +def setup(services): + KEPT.append(services.tools) +''') + reg = ToolRegistry() + loader = PluginLoader(registry=reg) + assert loader.load_all(tmp_path) == 1 + import sys + kept = sys.modules["localchat_plugin_keeper"].KEPT[0] + with pytest.raises(RuntimeError, match="closed"): + kept.register("late", "d", {}) + assert reg.names == [] + loader.unload("keeper") + def test_a_reload_still_records_the_plugins_tools(self, tmp_path): """The snapshot used to precede the unload, so a reload recorded no tools and a later unload left them registered.""" @@ -421,6 +461,14 @@ def test_a_global_admin_is_recorded_as_admin(self): assert check_workspace_access(req, "ws-1", "viewer") is None assert get_caller(req) == RequestCaller(user_id=_USER, role="admin", via_api_key=False) + def test_a_token_claiming_admin_is_recorded_at_its_database_role(self): + """The role comes from the database, not the token's claim (H2): a demoted admin + holding an old token is recorded as the member they now are.""" + req = _request(token=create_access_token(_USER, {"role": "admin"}), + db=_db(member_role="viewer", global_role="user")) + assert check_workspace_access(req, "ws-1", "viewer") is None + assert get_caller(req) == RequestCaller(user_id=_USER, role="viewer", via_api_key=False) + def test_an_api_key_is_recorded_with_its_role_and_no_user(self): req = _request(api_key="lcw_abc", db=_db()) assert check_workspace_access(req, "ws-1", "viewer") is None