From 4d046b666d091a7388574e78b2702f104f33e8e8 Mon Sep 17 00:00:00 2001 From: barlind Date: Wed, 3 Jun 2026 10:48:57 +0200 Subject: [PATCH] chore: address pre-PR review findings Signed-off-by: barlind --- .claude/agents/slopsmith-reviewer.md | 8 ++++---- .claude/skills/plugin-scaffold/SKILL.md | 2 +- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/workflows/validate-plugins.yml | 8 ++++++++ docs/capability-domains.md | 2 +- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/.claude/agents/slopsmith-reviewer.md b/.claude/agents/slopsmith-reviewer.md index a810cec..80e268c 100644 --- a/.claude/agents/slopsmith-reviewer.md +++ b/.claude/agents/slopsmith-reviewer.md @@ -33,7 +33,7 @@ Run each item; structure the output as `PASS` / `FAIL` / `N/A` with file:line ci python -c "import json,jsonschema; s=json.load(open('schema/plugin.schema.json')); jsonschema.validate(json.load(open('plugins//plugin.json')), s); print('OK')" ``` 2. **Manifest `id` matches the directory name.** `tests/test_plugin_schema.py::test_in_tree_manifest_id_matches_directory` enforces this — but call it out in review. -3. **Declared files exist.** For every path-bearing field in `plugin.json` (`script`, `routes`, `tour`, `settings.html`, `settings.server_files`, `diagnostics.server_files`), `test -f plugins//` must succeed (or the path must be a directory if it ends with `/`). +3. **Declared plugin files exist.** For plugin-root-relative file fields (`script`, `routes`, `tour`, `settings.html`), `test -f plugins//` must succeed. 4. **License is on the curated allowlist** if present. Cross-check `plugin.json.license` against the SPDX list in [`CONTRIBUTING.md`](../../CONTRIBUTING.md) "Plugin licensing". 5. **`type: "visualization"` ↔ `window.slopsmithViz_` factory.** If `type == "visualization"`, grep `script` for the factory declaration. 6. **Backend logging.** Grep `plugins//*.py` for `print(`, `traceback.print_exc(`, `logging.getLogger(`. Suggest `context["log"]` replacements. @@ -41,7 +41,7 @@ Run each item; structure the output as `PASS` / `FAIL` / `N/A` with file:line ci 8. **Frontend IIFE.** If `script` exists, check the top of the file isn't running top-level statements that leak to global scope. Wrapping in `(function () { 'use strict'; ... })();` is the convention. 9. **Capability-first frontend integration.** If the script wraps host globals or polls another plugin's globals, flag it as a capability gap unless the PR explicitly documents why no active domain can model the integration yet. 10. **`localStorage` prefix.** Grep for `localStorage.` usage; keys must start with ``. -11. **`settings.server_files` paths are safe.** Each entry must be a relpath — no leading `/`, no `..`, no backslashes. The schema enforces this but call it out. +11. **Server-file allowlists are safe.** `settings.server_files` and `diagnostics.server_files` entries are relpaths under `context["config_dir"]`, not files under the plugin directory. Check for no leading `/`, no `..`, no backslashes, no `//`, no `./`, and no leading dotfiles. The schema enforces this but call it out. ## Output format @@ -50,12 +50,12 @@ plugin-review: ========================= 1. manifest validates PASS 2. id matches directory PASS -3. declared files exist FAIL — settings.server_files lists "missing_db.sqlite" but plugins//missing_db.sqlite is absent +3. declared plugin files exist FAIL — script lists "missing.js" but plugins//missing.js is absent ... Total: 10 PASS / 1 FAIL / 1 N/A Action items: -- Remove the dangling settings.server_files entry, or create the file. +- Remove the dangling script entry, or create the file. - Replace `print(...)` calls at routes.py:42, routes.py:88 with `context["log"].info(...)`. ``` diff --git a/.claude/skills/plugin-scaffold/SKILL.md b/.claude/skills/plugin-scaffold/SKILL.md index 9fab731..98bb0aa 100644 --- a/.claude/skills/plugin-scaffold/SKILL.md +++ b/.claude/skills/plugin-scaffold/SKILL.md @@ -54,7 +54,7 @@ If the plugin slug or type is missing, ask once. **`type=routes-only`** — adds: - `"routes": "routes.py"` to manifest -- a conservative `"capabilities"` declaration only when the route participates in a known domain such as `library`, `jobs`, or `privileged-capabilities`; otherwise leave capability participation out and ask what workflow the route owns +- a conservative `"capabilities"` declaration for the route's Slopsmith-facing workflow; if the domain is unclear, ask what capability domain the route owns, provides, requests, or observes before finalizing the scaffold - `routes.py` with `def setup(app, context):` that registers one example route and uses `context["log"].info("plugin ready")` (never `print()`) - `tests/test__routes.py` — FastAPI TestClient stub diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 5af73fb..2838fd4 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -23,7 +23,7 @@ Thanks for the PR! A few quick reminders before you hit submit: - [ ] `npm run test:js` passes locally - [ ] `npm test` (Playwright) passes locally — *or* CI will run it - [ ] Verified in `docker compose up` (live-reload working directory) -- [ ] Plugin manifest validates against `schema/plugin.schema.json` (legacy fields + capability metadata) +- [ ] Plugin manifest validates against `schema/plugin.schema.json` (including capability metadata) - [ ] Not applicable — explain below ## Screenshots / recordings diff --git a/.github/workflows/validate-plugins.yml b/.github/workflows/validate-plugins.yml index 9c03eec..219af67 100644 --- a/.github/workflows/validate-plugins.yml +++ b/.github/workflows/validate-plugins.yml @@ -5,16 +5,24 @@ on: branches: [main] paths: - 'plugins/**/plugin.json' + - 'plugins/__init__.py' - 'schema/plugin.schema.json' + - 'docs/plugin-manifest.schema.json' + - 'static/capabilities.js' - 'CONTRIBUTING.md' + - 'requirements-test.txt' - 'tests/test_plugin_schema.py' - '.github/workflows/validate-plugins.yml' pull_request: branches: [main] paths: - 'plugins/**/plugin.json' + - 'plugins/__init__.py' - 'schema/plugin.schema.json' + - 'docs/plugin-manifest.schema.json' + - 'static/capabilities.js' - 'CONTRIBUTING.md' + - 'requirements-test.txt' - 'tests/test_plugin_schema.py' - '.github/workflows/validate-plugins.yml' diff --git a/docs/capability-domains.md b/docs/capability-domains.md index 6c1e5c0..2321c0c 100644 --- a/docs/capability-domains.md +++ b/docs/capability-domains.md @@ -234,7 +234,7 @@ if (!runtime.registered) { window.slopsmith.capabilities.registerParticipant('my_plugin.runtime', { 'example.plugin-domain': { roles: ['provider'], - operations: ['inspect'], + commands: ['inspect'], handlers: { inspect: () => runtime.impl.inspect(), },