From 789a94f80fd0190aabe204fba82b89c43a525960 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 17:22:17 +0200 Subject: [PATCH 1/8] docs: add plugin best-practices reference guide Extracts patterns from the 5 core plugins (analytics, genie, files, lakebase, server) into a structured reference with 9 sections and three severity tiers (NEVER/MUST/SHOULD). Signed-off-by: Atila Fassina --- .claude/references/plugin-best-practices.md | 178 ++++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 .claude/references/plugin-best-practices.md diff --git a/.claude/references/plugin-best-practices.md b/.claude/references/plugin-best-practices.md new file mode 100644 index 00000000..978c3f6c --- /dev/null +++ b/.claude/references/plugin-best-practices.md @@ -0,0 +1,178 @@ +# Plugin Best Practices + +Reference guide for building AppKit plugins. Every guideline is prefixed with a severity tier: + +- **NEVER** — Security or breakage blocker. Violating this will cause data leaks, crashes, or silent corruption. +- **MUST** — Correctness requirement. Violating this produces bugs, broken APIs, or inconsistent behavior. +- **SHOULD** — Quality recommendation. Violating this degrades DX, performance, or maintainability. + +--- + +## 1. Manifest Design + +**MUST** include all four required top-level fields: `name`, `displayName`, `description`, `resources`. + +**MUST** use lowercase kebab-case for `name` (pattern: `^[a-z][a-z0-9-]*$`). This becomes the route prefix (`/api/{name}/`) and the key on the AppKit instance. + +**MUST** declare both `resources.required` and `resources.optional` arrays, even if empty. + +**MUST** give every resource a unique `resourceKey` (lowercase kebab-case). The `alias` is for display only; `resourceKey` drives deduplication, env naming, and bundle config. + +**MUST** use the correct permission enum per resource type (e.g. `CAN_USE` for `sql_warehouse`, `WRITE_VOLUME` for `volume`). The schema validates this with `allOf`/`if-then` rules. + +**SHOULD** add `fields` with `env` entries so that `appkit plugin sync` and `appkit init` can auto-generate `.env` templates and `app.yaml` resource blocks. + +**SHOULD** set `hidden: true` on infrastructure plugins (like `server`) that should not appear in the template manifest. + +**MUST** use `getResourceRequirements(config)` for resources that depend on runtime config. Declare them as `optional` in the static manifest, then return them as `required: true` from the static method. See `FilesPlugin.getResourceRequirements` for the canonical pattern. + +--- + +## 2. Plugin Class Structure + +**MUST** extend `Plugin` where `TConfig extends BasePluginConfig`. + +**MUST** declare a static `manifest` property typed with `PluginManifest<"your-name">`: + +```ts +static manifest = manifest as PluginManifest<"my-plugin">; +``` + +**MUST** export a `toPlugin`-wrapped factory as the public API. The class itself is `@internal`: + +```ts +class MyPlugin extends Plugin { ... } + +/** @internal */ +export const myPlugin = toPlugin(MyPlugin); +``` + +**MUST** re-declare config with `protected declare config: IMyConfig` if extending the base config type. Call `super(config)` first, then assign `this.config = config`. + +**SHOULD** keep the barrel `index.ts` minimal: re-export the plugin factory and types only. + +**SHOULD** use `static phase: PluginPhase` only when initialization order matters. Use `"core"` for config-only plugins, `"deferred"` for server (starts last). Default is `"normal"`. + +**MUST** implement `shutdown()` and call `this.streamManager.abortAll()` if the plugin uses streaming or long-lived connections. + +--- + +## 3. Route Design + +**MUST** register routes via `this.route(router, config)` inside `injectRoutes()`. This auto-registers endpoints under `/api/{pluginName}{path}` and tracks them for the client config endpoint map. + +**MUST** include a `name` in every route config. This becomes the key in `getEndpoints()` and is used by the frontend to discover URLs. + +**NEVER** register routes directly on `router.get(...)` without going through `this.route()` — the endpoint will be invisible to the server plugin's endpoint map and client config. + +**MUST** set `skipBodyParsing: true` on routes that receive raw streams (e.g. file uploads). The server plugin uses this to bypass `express.json()` for those paths. + +**SHOULD** use RESTful conventions: `GET` for reads, `POST` for mutations and queries with bodies, `DELETE` for deletions. + +**SHOULD** validate required params early and return `400` before doing any work. + +--- + +## 4. Interceptor Usage + +**MUST** define execution defaults in a separate `defaults.ts` file as `PluginExecuteConfig` constants. This keeps route handlers clean and makes defaults testable. + +**MUST** pass defaults via the `default` key in `PluginExecutionSettings`. User overrides go in `user`. The merge order is: method defaults <- plugin config <- user override. + +**NEVER** enable cache without providing a `cacheKey` array. The cache interceptor silently skips caching when `cacheKey` is empty/missing, so you get no caching and no error. + +**MUST** scope cache keys to include the plugin name, operation, and all varying parameters: + +```ts +cache: { + cacheKey: ["analytics:query", queryKey, JSON.stringify(params), executorKey], +} +``` + +**SHOULD** disable retry for non-idempotent operations (mutations, chat messages, stream creation). See `genieStreamDefaults` for the pattern. + +**SHOULD** disable cache for write operations and streaming downloads. See `FILES_WRITE_DEFAULTS` and `FILES_DOWNLOAD_DEFAULTS`. + +**SHOULD** set appropriate timeouts: short for reads (5-30s), long for writes/uploads (600s), very long for streaming conversations (120s+). + +--- + +## 5. asUser / OBO Patterns + +**MUST** call `this.asUser(req)` to execute operations with user credentials. The returned proxy wraps every method call in `runInUserContext(userContext, ...)`. + +**NEVER** call `asUser()` on lifecycle methods (`setup`, `shutdown`, `injectRoutes`). These are excluded from the proxy via `EXCLUDED_FROM_PROXY`. + +**MUST** use `.obo.sql` file suffix for analytics queries that should execute as the user. The `_handleQueryRoute` method checks `isAsUser` and conditionally wraps with `this.asUser(req)`. + +**SHOULD** use `isInUserContext()` to detect whether the current call is running in a user context. Use this for audit logging or to enforce OBO-only access: + +```ts +// Warn pattern (analytics reads): +if (!isInUserContext()) { logger.warn("..."); } + +// Enforce pattern (file writes): +if (!isInUserContext()) { throw new Error("...use OBO..."); } +``` + +**MUST** scope cache keys differently for OBO vs service principal. Use `getCurrentUserId()` for OBO and `"global"` for service principal to avoid cross-user cache pollution. + +**SHOULD** prefer strict enforcement (`throwIfNoUserContext`) for write operations. Use warn-only (`warnIfNoUserContext`) for read operations where service principal fallback is acceptable. + +--- + +## 6. Client Config + +**MUST** return only JSON-serializable plain data from `clientConfig()`. No functions, Dates, classes, Maps, Sets, BigInts, or circular references. + +**NEVER** expose secrets, tokens, or internal URLs in `clientConfig()`. The server plugin runs `sanitizeClientConfig()` which redacts values matching non-public env vars, but defense in depth means not returning them at all. + +**SHOULD** return an empty object `{}` (the default) if the plugin has no client-facing config. Only override `clientConfig()` when the frontend needs server-side values at boot time. + +**SHOULD** keep client config minimal: feature flags, resource IDs, available volume keys. Avoid large payloads. + +--- + +## 7. SSE Streaming + +**MUST** use `this.executeStream(res, handler, settings)` for SSE responses. This wires up the interceptor chain, stream management, abort signals, and reconnection support. + +**MUST** return an `AsyncGenerator` from the stream handler for chunked event delivery. Non-generator return values are auto-wrapped in a single yield. + +**SHOULD** pass a stable `streamId` (e.g. from `requestId` query param or `randomUUID()`) in `stream.streamId` to support client reconnection via `Last-Event-ID`. + +**SHOULD** configure `stream.bufferSize` for event replay on reconnection. Default is fine for most cases; increase for high-throughput streams. + +**MUST** call `this.streamManager.abortAll()` in `shutdown()` to cancel all active streams during graceful shutdown. + +--- + +## 8. Testing Expectations + +**MUST** co-locate tests in a `tests/` directory inside the plugin folder (e.g. `plugins/analytics/tests/analytics.test.ts`). + +**MUST** mock external connectors and the Databricks SDK. Use `vi.mock()` for connector classes. Never make real API calls in unit tests. + +**SHOULD** write both unit tests (`.test.ts`) and integration tests (`.integration.test.ts`). Integration tests exercise the full interceptor chain with mocked connectors. + +**SHOULD** test error paths: missing params (400), not-found (404), upstream failures (500), auth errors (401). + +**SHOULD** test cache key scoping: verify that different users, different params, and different query keys produce distinct cache entries. + +**SHOULD** test `asUser` proxy behavior: verify that route handlers correctly delegate to `this.asUser(req)` for OBO endpoints. + +--- + +## 9. Type Safety + +**MUST** type the config interface extending `BasePluginConfig` and export it from `types.ts`. + +**MUST** type `exports()` with an explicit return type or interface when the public API is non-trivial. This ensures the registry type generation produces accurate types. + +**MUST** type route handler request/response bodies. Use `req.body as IMyRequest` with a defined interface, not `any`. + +**SHOULD** use `PluginManifest<"name">` generic to tie the manifest type to the plugin name literal. This enables type-safe access on the AppKit instance. + +**SHOULD** use `protected declare config: IMyConfig` instead of redefining the property. The `declare` keyword preserves the base class field while narrowing the type. + +**NEVER** use `any` for connector responses or SDK return types without documenting why. Prefer `unknown` and narrow with type guards, or define response interfaces. From fd5119b2df1b48cea479aea2103109268d133d84 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 17:23:27 +0200 Subject: [PATCH 2/8] docs: integrate best-practices reference into create-core-plugin skill Adds a "Load Best Practices Reference" step before scaffolding decisions so guidelines are applied during plugin creation. Signed-off-by: Atila Fassina --- .claude/commands/create-core-plugin.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.claude/commands/create-core-plugin.md b/.claude/commands/create-core-plugin.md index bb234471..56e172ce 100644 --- a/.claude/commands/create-core-plugin.md +++ b/.claude/commands/create-core-plugin.md @@ -2,6 +2,16 @@ User input: $ARGUMENTS +## 0. Load Best Practices Reference + +Before making any scaffolding decisions, read the plugin best-practices reference: + +``` +.claude/references/plugin-best-practices.md +``` + +This document defines NEVER/MUST/SHOULD guidelines for manifest design, plugin class structure, route design, interceptor usage, asUser/OBO patterns, client config, SSE streaming, testing, and type safety. Apply these guidelines throughout the scaffolding process — especially when deciding interceptor defaults, cache key scoping, OBO enforcement, and route registration patterns. + ## 1. Gather Requirements The user may have provided a plugin name and/or description above in `$ARGUMENTS`. Parse what was given. From a4b31e2ae1928c252479568a1d8d2678282a7610 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 17:26:29 +0200 Subject: [PATCH 3/8] fix: correct inaccuracies in plugin best-practices reference Fixes 3 issues found during dry-run validation against analytics and files plugins: - Plugin extends Plugin (not Plugin) - @internal goes on toPlugin export, not the class - isInUserContext() patterns clarified per actual usage Signed-off-by: Atila Fassina --- .claude/references/plugin-best-practices.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/.claude/references/plugin-best-practices.md b/.claude/references/plugin-best-practices.md index 978c3f6c..de75d7ac 100644 --- a/.claude/references/plugin-best-practices.md +++ b/.claude/references/plugin-best-practices.md @@ -30,7 +30,7 @@ Reference guide for building AppKit plugins. Every guideline is prefixed with a ## 2. Plugin Class Structure -**MUST** extend `Plugin` where `TConfig extends BasePluginConfig`. +**MUST** extend `Plugin` (the base class accepts an optional generic but core plugins use the plain form). **MUST** declare a static `manifest` property typed with `PluginManifest<"your-name">`: @@ -38,10 +38,10 @@ Reference guide for building AppKit plugins. Every guideline is prefixed with a static manifest = manifest as PluginManifest<"my-plugin">; ``` -**MUST** export a `toPlugin`-wrapped factory as the public API. The class itself is `@internal`: +**MUST** export a `toPlugin`-wrapped factory as the public API. Mark the factory (not the class) as `@internal`: ```ts -class MyPlugin extends Plugin { ... } +export class MyPlugin extends Plugin { ... } /** @internal */ export const myPlugin = toPlugin(MyPlugin); @@ -105,13 +105,16 @@ cache: { **MUST** use `.obo.sql` file suffix for analytics queries that should execute as the user. The `_handleQueryRoute` method checks `isAsUser` and conditionally wraps with `this.asUser(req)`. -**SHOULD** use `isInUserContext()` to detect whether the current call is running in a user context. Use this for audit logging or to enforce OBO-only access: +**SHOULD** use `isInUserContext()` in the programmatic API to detect whether the call is running in a user context. Two patterns exist: + +- **File-naming convention** (analytics): Use `.obo.sql` suffix to determine whether a query runs as user or service principal. No runtime `isInUserContext()` check needed in route handlers. +- **Runtime enforcement** (files programmatic API): Call `isInUserContext()` to warn or throw: ```ts -// Warn pattern (analytics reads): +// Warn pattern (read operations in route handlers): if (!isInUserContext()) { logger.warn("..."); } -// Enforce pattern (file writes): +// Enforce pattern (programmatic API, e.g. createVolumeAPI): if (!isInUserContext()) { throw new Error("...use OBO..."); } ``` From e1eb2c6187ef6445df2e064397e865560718a688 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 18:01:42 +0200 Subject: [PATCH 4/8] docs: add review-core-plugin and audit-core-plugin skills Signed-off-by: Atila Fassina --- .claude/commands/audit-core-plugin.md | 144 +++++++++++++++++++++++++ .claude/commands/review-core-plugin.md | 129 ++++++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 .claude/commands/audit-core-plugin.md create mode 100644 .claude/commands/review-core-plugin.md diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md new file mode 100644 index 00000000..48c8160e --- /dev/null +++ b/.claude/commands/audit-core-plugin.md @@ -0,0 +1,144 @@ +--- +description: Full audit of a core plugin against all best-practices categories with scorecard +argument-hint: +--- + +# Audit Core Plugin + +Perform a full audit of the named plugin against all AppKit plugin best-practices categories and produce a scorecard. + +**Plugin name:** $ARGUMENTS + +## Step 1: Validate Input + +If `$ARGUMENTS` is empty or missing, stop and output: + +> Usage: /audit-core-plugin + +Otherwise, set `PLUGIN_NAME` to the value of `$ARGUMENTS` (trimmed, kebab-case). + +Check whether the plugin directory exists at either of these paths: + +- `packages/appkit/src/plugins/{PLUGIN_NAME}/` +- `packages/appkit/src/connectors/{PLUGIN_NAME}/` + +If **neither** path exists, stop and output: + +> Error: Plugin "{PLUGIN_NAME}" not found. Checked: +> - `packages/appkit/src/plugins/{PLUGIN_NAME}/` +> - `packages/appkit/src/connectors/{PLUGIN_NAME}/` + +If at least one path exists, proceed. + +## Step 2: Load Best Practices Reference + +Read the full contents of: + +``` +.claude/references/plugin-best-practices.md +``` + +This defines the 9 audit categories and their NEVER/MUST/SHOULD guidelines. You will evaluate the plugin against every guideline in every category. + +## Step 3: File Discovery + +Read **all** files under: + +- `packages/appkit/src/plugins/{PLUGIN_NAME}/` (recursively, including `tests/` subdirectory) +- `packages/appkit/src/connectors/{PLUGIN_NAME}/` (recursively, if this directory exists) + +Collect the full contents of every file. You need the complete source to evaluate all 9 categories. + +## Step 4: Structural Completeness Check + +Verify the following expected files exist inside `packages/appkit/src/plugins/{PLUGIN_NAME}/`: + +| Expected file | Required? | +|---|---| +| `manifest.json` | MUST | +| Main plugin class file (any `.ts` file containing a class extending `Plugin`) | MUST | +| `types.ts` | MUST | +| `defaults.ts` | MUST | +| `index.ts` | MUST | +| `tests/` directory with at least one `.test.ts` file | MUST | + +Each missing file is a **MUST**-severity finding under the "Structural Completeness" category. + +## Step 5: Full Best-Practices Review + +Evaluate the plugin code against **all 9 categories** from the best-practices reference: + +1. **Manifest Design** — Check `manifest.json` against all NEVER/MUST/SHOULD rules +2. **Plugin Class Structure** — Check the main plugin class against all NEVER/MUST/SHOULD rules +3. **Route Design** — Check `injectRoutes()` and route handlers against all NEVER/MUST/SHOULD rules +4. **Interceptor Usage** — Check `execute()`/`executeStream()` calls and `defaults.ts` against all NEVER/MUST/SHOULD rules +5. **asUser / OBO Patterns** — Check OBO usage, cache key scoping, context enforcement against all NEVER/MUST/SHOULD rules +6. **Client Config** — Check `clientConfig()` return value against all NEVER/MUST/SHOULD rules +7. **SSE Streaming** — Check streaming usage and shutdown behavior against all NEVER/MUST/SHOULD rules +8. **Testing Expectations** — Check test files for coverage, mocking patterns, error path tests against all NEVER/MUST/SHOULD rules +9. **Type Safety** — Check types, exports, config interface, `any` usage against all NEVER/MUST/SHOULD rules + +For each guideline in each category, determine whether the plugin **passes**, **violates**, or is **not applicable** (e.g., SSE rules for a non-streaming plugin). Record findings with: + +- **Severity**: NEVER, MUST, or SHOULD (from the guideline prefix) +- **Category**: Which of the 9 categories +- **Description**: What the guideline requires and how the plugin violates it +- **Location**: Specific `file:line` reference(s) + +A category with no findings is a pass. A category with only SHOULD findings is a warn. A category with any MUST or NEVER finding is a fail. + +## Step 6: Produce Output + +### Scorecard Table (output first) + +``` +## Scorecard + +| # | Category | Status | Findings | +|---|----------|--------|----------| +| 0 | Structural Completeness | {status} | {count} | +| 1 | Manifest Design | {status} | {count} | +| 2 | Plugin Class Structure | {status} | {count} | +| 3 | Route Design | {status} | {count} | +| 4 | Interceptor Usage | {status} | {count} | +| 5 | asUser / OBO Patterns | {status} | {count} | +| 6 | Client Config | {status} | {count} | +| 7 | SSE Streaming | {status} | {count} | +| 8 | Testing Expectations | {status} | {count} | +| 9 | Type Safety | {status} | {count} | +``` + +Where `{status}` is one of: +- Pass — no findings +- Warn — SHOULD-only findings +- Fail — any NEVER or MUST findings + +And `{count}` is the number of findings (0 if pass). + +### Detailed Findings (output second, severity-first) + +Group all findings across all categories and sort by severity: + +1. **NEVER** findings first (most critical) +2. **MUST** findings second +3. **SHOULD** findings last + +For each finding, output: + +``` +### [{severity}] {category}: {short description} + +**File:** `{file_path}:{line_number}` + +{Explanation of what the guideline requires, what the code does wrong, and how to fix it.} +``` + +If there are zero findings across all categories, output: + +> All checks passed. No findings. + +### Summary (output last) + +End with a one-line summary: + +> **Audit result: {total_findings} findings ({never_count} NEVER, {must_count} MUST, {should_count} SHOULD) across {failing_categories} failing and {warning_categories} warning categories.** diff --git a/.claude/commands/review-core-plugin.md b/.claude/commands/review-core-plugin.md new file mode 100644 index 00000000..9c53b182 --- /dev/null +++ b/.claude/commands/review-core-plugin.md @@ -0,0 +1,129 @@ +--- +description: Review plugin changes against AppKit best practices (composes with review-pr) +argument-hint: [plugin-name or base-branch] +--- + +# Review Core Plugin Changes + +User input: $ARGUMENTS + +## Step 0: Parse Input + +Parse `$ARGUMENTS` to determine: + +- **Plugin name filter** — if `$ARGUMENTS` looks like a plugin name (kebab-case word, no slashes, not a branch name), use it to filter the diff to that specific plugin. +- **Base branch** — if `$ARGUMENTS` looks like a branch name (contains `/` or matches known branch patterns like `main`, `develop`, `origin/*`), use it as the base branch. Otherwise default to `origin/main`. +- If `$ARGUMENTS` is empty, review all plugins found in the diff against `origin/main`. + +## Step 1: Core Principles Review + +First, invoke the `review-pr` skill to run the standard Core Principles review. Pass the base branch as the argument (not the plugin name). + +Use the Skill tool: +- skill: `review-pr` +- args: the base branch determined in Step 0 (or empty to use default) + +Wait for this review to complete before continuing. + +## Step 2: Diff Analysis + +Run `git diff ...HEAD --name-only` to get all changed files. + +Filter the file list to plugin-relevant paths: +- `packages/appkit/src/plugins/**` +- `packages/appkit/src/connectors/**` + +If a specific plugin name was provided in Step 0, further filter to only files matching that plugin name in the path. + +**If no plugin files are found in the diff**, output: + +> No plugin files were changed in this branch. Plugin best-practices review is not applicable. Only the Core Principles review above applies. + +Then stop. Do not continue to subsequent steps. + +## Step 3: Multi-Plugin Detection + +If no specific plugin name was provided, detect all distinct plugins touched in the diff by extracting the plugin directory name from each changed path: +- From `packages/appkit/src/plugins/{name}/...` extract `{name}` +- From `packages/appkit/src/connectors/{name}/...` extract `{name}` + +Deduplicate the list. You will run Steps 4-6 for **each** detected plugin. + +## Step 4: Category Scoping + +For each plugin being reviewed, map the changed files to the relevant best-practices categories: + +| Changed file pattern | Category | +|---|---| +| `manifest.json` | 1. Manifest Design | +| Main plugin class file (the primary `.ts` file, not index/types/defaults) | 2. Plugin Class Structure | +| Main plugin class file (the primary `.ts` file, not index/types/defaults) | 9. Type Safety | +| Any file containing route registration (`this.route(`, `injectRoutes`) | 3. Route Design | +| `defaults.ts` | 4. Interceptor Usage | +| Any file containing `asUser(`, `.obo.sql`, `isInUserContext`, `runInUserContext` | 5. asUser / OBO Patterns | +| Any file containing `clientConfig()` | 6. Client Config | +| Any file containing `executeStream(`, `streamManager`, `AsyncGenerator`, SSE-related code | 7. SSE Streaming | +| Files under `tests/` or files ending in `.test.ts` | 8. Testing Expectations | +| `types.ts`, config interfaces, `exports()` return types | 9. Type Safety | + +Read the actual changed file contents with `git diff ...HEAD -- ` to determine which patterns are present. A single file may trigger multiple categories. + +Record which of the 9 categories are **relevant** (at least one changed file maps to them) and which are **skipped** (no changed files map to them). + +## Step 5: Load Best Practices Reference + +Read the file `.claude/references/plugin-best-practices.md`. + +For each **relevant** category identified in Step 4, extract all NEVER, MUST, and SHOULD guidelines from that category section. + +## Step 6: Best-Practices Review + +For each plugin detected in Step 3, review the changed code against the scoped guidelines from Step 5. + +For each finding: +- Identify the **severity** (NEVER, MUST, or SHOULD) +- Identify the **category** (e.g., "Manifest Design", "Route Design") +- Cite the specific guideline being violated or satisfied +- Reference the exact file and line(s) involved +- Provide a concrete fix if it is a violation + +## Step 7: Output + +### Format + +For each plugin reviewed, output a section with the plugin name as the heading. + +**Order findings by severity, not by category:** + +1. **NEVER violations** (most critical) — these are security or breakage blockers +2. **MUST violations** — these are correctness requirements +3. **SHOULD violations** — these are quality recommendations + +Each finding should follow this format: + +``` +### [SEVERITY] Category Name: Brief description +- **File:** `path/to/file.ts:L42` +- **Guideline:** +- **Finding:** +- **Fix:** +``` + +If a plugin has **no findings** (all scoped guidelines are satisfied), state that explicitly. + +### Skipped Categories + +At the end of the output (after all plugin reviews), list the categories that were **not relevant** to this diff: + +``` +### Skipped Categories (not relevant to this diff) +- Category N: — no changed files matched this category +- ... +``` + +### Summary + +End with an overall summary: +- Total findings by severity (e.g., "0 NEVER, 2 MUST, 3 SHOULD") +- Whether the changes are ready to merge from a plugin best-practices perspective +- Any categories that deserve attention even though they were skipped (e.g., "No tests were changed — consider adding tests for the new route") From b69100e75029149693cc892d8b1085f76b2c837d Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 18:06:03 +0200 Subject: [PATCH 5/8] fix: address dry-run findings in audit and review skills - Add cacheKey two-stage pattern guidance to prevent false positives - Add N/A status option for non-applicable scorecard categories - Clarify connector scope in structural completeness check - Add deduplication guidance for cross-category findings - Add recovery hint to plugin-not-found error message Signed-off-by: Atila Fassina --- .claude/commands/audit-core-plugin.md | 9 ++++++++- .claude/commands/review-core-plugin.md | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md index 48c8160e..18a13014 100644 --- a/.claude/commands/audit-core-plugin.md +++ b/.claude/commands/audit-core-plugin.md @@ -27,6 +27,8 @@ If **neither** path exists, stop and output: > Error: Plugin "{PLUGIN_NAME}" not found. Checked: > - `packages/appkit/src/plugins/{PLUGIN_NAME}/` > - `packages/appkit/src/connectors/{PLUGIN_NAME}/` +> +> Available plugins can be listed with: `ls packages/appkit/src/plugins/` If at least one path exists, proceed. @@ -64,14 +66,18 @@ Verify the following expected files exist inside `packages/appkit/src/plugins/{P Each missing file is a **MUST**-severity finding under the "Structural Completeness" category. +> **Note:** The structural completeness check applies only to the `plugins/{PLUGIN_NAME}/` directory. Connector directories (`connectors/{PLUGIN_NAME}/`) serve a different architectural role and are read as supporting context for the best-practices review, not audited for structural completeness. + ## Step 5: Full Best-Practices Review +> **Deduplication:** If the same code issue is covered by guidelines in multiple categories, report it once under the most specific category and note that it also relates to the other category. Do not count it as separate findings in each category. + Evaluate the plugin code against **all 9 categories** from the best-practices reference: 1. **Manifest Design** — Check `manifest.json` against all NEVER/MUST/SHOULD rules 2. **Plugin Class Structure** — Check the main plugin class against all NEVER/MUST/SHOULD rules 3. **Route Design** — Check `injectRoutes()` and route handlers against all NEVER/MUST/SHOULD rules -4. **Interceptor Usage** — Check `execute()`/`executeStream()` calls and `defaults.ts` against all NEVER/MUST/SHOULD rules +4. **Interceptor Usage** — Check `execute()`/`executeStream()` calls and `defaults.ts` against all NEVER/MUST/SHOULD rules. **Important:** When evaluating cache configuration in `defaults.ts`, the codebase uses a two-stage pattern where `defaults.ts` defines partial cache config (e.g., `enabled: true, ttl: 3600`) and the `cacheKey` is injected at the call site in the plugin class. Before flagging a NEVER violation for missing `cacheKey`, trace the cache config through to `execute()`/`executeStream()` call sites to verify whether a `cacheKey` is provided there. Only flag as NEVER if no `cacheKey` is provided at any point in the execution path. 5. **asUser / OBO Patterns** — Check OBO usage, cache key scoping, context enforcement against all NEVER/MUST/SHOULD rules 6. **Client Config** — Check `clientConfig()` return value against all NEVER/MUST/SHOULD rules 7. **SSE Streaming** — Check streaming usage and shutdown behavior against all NEVER/MUST/SHOULD rules @@ -112,6 +118,7 @@ Where `{status}` is one of: - Pass — no findings - Warn — SHOULD-only findings - Fail — any NEVER or MUST findings +- N/A — category does not apply to this plugin (e.g., SSE Streaming for a non-streaming plugin) And `{count}` is the number of findings (0 if pass). diff --git a/.claude/commands/review-core-plugin.md b/.claude/commands/review-core-plugin.md index 9c53b182..6477f549 100644 --- a/.claude/commands/review-core-plugin.md +++ b/.claude/commands/review-core-plugin.md @@ -83,7 +83,7 @@ For each plugin detected in Step 3, review the changed code against the scoped g For each finding: - Identify the **severity** (NEVER, MUST, or SHOULD) - Identify the **category** (e.g., "Manifest Design", "Route Design") -- Cite the specific guideline being violated or satisfied +- Cite the specific guideline being violated or satisfied. **Important:** For Interceptor Usage findings (Category 4), when evaluating cache configuration, trace `cacheKey` through to `execute()`/`executeStream()` call sites before flagging. The codebase commonly defines partial cache config in `defaults.ts` and injects `cacheKey` at the call site. Only flag as NEVER if no `cacheKey` is provided anywhere in the execution path. - Reference the exact file and line(s) involved - Provide a concrete fix if it is a violation From b8df39807c7ebd492038756b705298a77596b827 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 19:02:26 +0200 Subject: [PATCH 6/8] fix: address review feedback on plugin skills - Remove trailing slash from route prefix example in best-practices - Use deterministic directory-existence check instead of name-pattern heuristic for plugin vs branch disambiguation in review-core-plugin - Downgrade defaults.ts from MUST to SHOULD in audit-core-plugin since not all plugins (e.g. server, lakebase) require it Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .claude/commands/audit-core-plugin.md | 6 ++++-- .claude/commands/review-core-plugin.md | 21 ++++++++++++++++----- .claude/references/plugin-best-practices.md | 2 +- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md index 18a13014..0a9ffd64 100644 --- a/.claude/commands/audit-core-plugin.md +++ b/.claude/commands/audit-core-plugin.md @@ -60,11 +60,13 @@ Verify the following expected files exist inside `packages/appkit/src/plugins/{P | `manifest.json` | MUST | | Main plugin class file (any `.ts` file containing a class extending `Plugin`) | MUST | | `types.ts` | MUST | -| `defaults.ts` | MUST | +| `defaults.ts` | SHOULD | | `index.ts` | MUST | | `tests/` directory with at least one `.test.ts` file | MUST | -Each missing file is a **MUST**-severity finding under the "Structural Completeness" category. +Treat each missing `MUST` file as a **MUST**-severity finding under the "Structural Completeness" category. Treat a missing `SHOULD` file as a **SHOULD**-severity finding. + +`defaults.ts` is not universally required for every plugin. It should be present when the plugin exposes execution settings or defines behavior that depends on `execute()` / `executeStream()` defaults, but its absence alone should not be reported as a MUST failure for plugins that do not use those defaults. > **Note:** The structural completeness check applies only to the `plugins/{PLUGIN_NAME}/` directory. Connector directories (`connectors/{PLUGIN_NAME}/`) serve a different architectural role and are read as supporting context for the best-practices review, not audited for structural completeness. diff --git a/.claude/commands/review-core-plugin.md b/.claude/commands/review-core-plugin.md index 6477f549..d701f35b 100644 --- a/.claude/commands/review-core-plugin.md +++ b/.claude/commands/review-core-plugin.md @@ -9,11 +9,22 @@ User input: $ARGUMENTS ## Step 0: Parse Input -Parse `$ARGUMENTS` to determine: - -- **Plugin name filter** — if `$ARGUMENTS` looks like a plugin name (kebab-case word, no slashes, not a branch name), use it to filter the diff to that specific plugin. -- **Base branch** — if `$ARGUMENTS` looks like a branch name (contains `/` or matches known branch patterns like `main`, `develop`, `origin/*`), use it as the base branch. Otherwise default to `origin/main`. -- If `$ARGUMENTS` is empty, review all plugins found in the diff against `origin/main`. +Parse `$ARGUMENTS` deterministically: + +- If `$ARGUMENTS` is empty: + - Use no plugin name filter. + - Use `origin/main` as the base branch. +- Otherwise, check whether either of these paths exists: + - `packages/appkit/src/plugins/$ARGUMENTS` + - `packages/appkit/src/connectors/$ARGUMENTS` +- If either path exists: + - Treat `$ARGUMENTS` as the **plugin name filter**. + - Use `origin/main` as the base branch. +- Otherwise: + - Treat `$ARGUMENTS` as the **base branch**. + - Use no plugin name filter. + +Do not use a name-pattern heuristic such as "kebab-case with no slashes" to decide whether `$ARGUMENTS` is a plugin name, because common branch names like `feature-x` and `bugfix-foo` would be ambiguous. ## Step 1: Core Principles Review diff --git a/.claude/references/plugin-best-practices.md b/.claude/references/plugin-best-practices.md index de75d7ac..e0f5f970 100644 --- a/.claude/references/plugin-best-practices.md +++ b/.claude/references/plugin-best-practices.md @@ -12,7 +12,7 @@ Reference guide for building AppKit plugins. Every guideline is prefixed with a **MUST** include all four required top-level fields: `name`, `displayName`, `description`, `resources`. -**MUST** use lowercase kebab-case for `name` (pattern: `^[a-z][a-z0-9-]*$`). This becomes the route prefix (`/api/{name}/`) and the key on the AppKit instance. +**MUST** use lowercase kebab-case for `name` (pattern: `^[a-z][a-z0-9-]*$`). This becomes the route prefix (`/api/{name}`) and the key on the AppKit instance. **MUST** declare both `resources.required` and `resources.optional` arrays, even if empty. From 65d58054d5921a896073a962bdea274d1559a8e5 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 19:18:15 +0200 Subject: [PATCH 7/8] chore: address code review --- .claude/commands/audit-core-plugin.md | 4 +++- .claude/commands/create-core-plugin.md | 22 ++++++--------------- .claude/commands/review-core-plugin.md | 2 +- .claude/references/plugin-best-practices.md | 10 +++++++--- 4 files changed, 17 insertions(+), 21 deletions(-) diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md index 0a9ffd64..fc093dd9 100644 --- a/.claude/commands/audit-core-plugin.md +++ b/.claude/commands/audit-core-plugin.md @@ -53,7 +53,9 @@ Collect the full contents of every file. You need the complete source to evaluat ## Step 4: Structural Completeness Check -Verify the following expected files exist inside `packages/appkit/src/plugins/{PLUGIN_NAME}/`: +If `packages/appkit/src/plugins/{PLUGIN_NAME}/` does not exist (connector-only package), mark Structural Completeness as **N/A** in the scorecard and proceed to Step 5. + +Otherwise, verify the following expected files exist inside `packages/appkit/src/plugins/{PLUGIN_NAME}/`: | Expected file | Required? | |---|---| diff --git a/.claude/commands/create-core-plugin.md b/.claude/commands/create-core-plugin.md index 56e172ce..a72c6e19 100644 --- a/.claude/commands/create-core-plugin.md +++ b/.claude/commands/create-core-plugin.md @@ -75,6 +75,8 @@ connectors/{name}/ ## 4. Plugin Patterns to Follow +> The scaffolding templates below implement the guidelines from `.claude/references/plugin-best-practices.md`. Refer to that document for the full NEVER/MUST/SHOULD rules and rationale behind each pattern. + ### 4a. Config Interface (`types.ts`) Extend `BasePluginConfig` from `shared`: @@ -176,7 +178,7 @@ const logger = createLogger("{name}"); export class {PascalName}Plugin extends Plugin { name = "{name}"; - static manifest = manifest as PluginManifest; + static manifest = manifest as PluginManifest<"{name}">; protected declare config: I{PascalName}Config; private connector: {PascalName}Connector; @@ -330,15 +332,7 @@ export * from "./{name}"; ## 6. Key Conventions -### Route Registration -Routes mount at `/api/{pluginName}/...`. Use `this.route(router, { name, method, path, handler })`. - -### User-Scoped Execution (OBO) -`this.asUser(req)` returns a proxy where all method calls use the user's Databricks credentials. Use it in route handlers for user-scoped operations. Inside the proxied method, `getWorkspaceClient()` automatically returns the user-scoped client. **Important:** any plugin using `asUser()` must declare the appropriate `user_api_scopes` in the bundle config — see section 4f. - -### Execution Methods -- `this.execute(fn, options)` — single request-response with interceptors (telemetry, timeout, retry, cache) -- `this.executeStream(res, fn, options)` — SSE streaming with interceptors +For full NEVER/MUST/SHOULD rules on routes, interceptors, OBO, streaming, and type safety, see `.claude/references/plugin-best-practices.md`. ### Context Utilities Import from `../../context`: @@ -349,12 +343,8 @@ Import from `../../context`: ### Logging Use `createLogger("plugin-name")` from `../../logging/logger`. -### Setup/Teardown -- Override `setup()` for async init (e.g., connection pools). Called by AppKit during startup. -- Override `abortActiveOperations()` for cleanup. Call `super.abortActiveOperations()` first. - -### Plugin Phases -Set `static phase: PluginPhase` if needed: `"core"` (first), `"normal"` (default), `"deferred"` (last). +### OBO Scopes Reminder +Any plugin using `this.asUser(req)` must declare `user_api_scopes` in the bundle config — see section 4f. ## 7. After Scaffolding diff --git a/.claude/commands/review-core-plugin.md b/.claude/commands/review-core-plugin.md index d701f35b..2c441874 100644 --- a/.claude/commands/review-core-plugin.md +++ b/.claude/commands/review-core-plugin.md @@ -70,7 +70,7 @@ For each plugin being reviewed, map the changed files to the relevant best-pract | Main plugin class file (the primary `.ts` file, not index/types/defaults) | 2. Plugin Class Structure | | Main plugin class file (the primary `.ts` file, not index/types/defaults) | 9. Type Safety | | Any file containing route registration (`this.route(`, `injectRoutes`) | 3. Route Design | -| `defaults.ts` | 4. Interceptor Usage | +| `defaults.ts`, or any file containing `execute(` / `executeStream(` calls with interceptor settings | 4. Interceptor Usage | | Any file containing `asUser(`, `.obo.sql`, `isInUserContext`, `runInUserContext` | 5. asUser / OBO Patterns | | Any file containing `clientConfig()` | 6. Client Config | | Any file containing `executeStream(`, `streamManager`, `AsyncGenerator`, SSE-related code | 7. SSE Streaming | diff --git a/.claude/references/plugin-best-practices.md b/.claude/references/plugin-best-practices.md index e0f5f970..007ccb62 100644 --- a/.claude/references/plugin-best-practices.md +++ b/.claude/references/plugin-best-practices.md @@ -6,13 +6,15 @@ Reference guide for building AppKit plugins. Every guideline is prefixed with a - **MUST** — Correctness requirement. Violating this produces bugs, broken APIs, or inconsistent behavior. - **SHOULD** — Quality recommendation. Violating this degrades DX, performance, or maintainability. +> **Scope:** These guidelines target **core plugins within the AppKit monorepo** (`packages/appkit/src/plugins/`). Custom plugins built outside the monorepo have more flexibility — see `docs/docs/plugins/custom-plugins.md` for lighter-weight patterns (inline manifests, camelCase names, etc.). + --- ## 1. Manifest Design **MUST** include all four required top-level fields: `name`, `displayName`, `description`, `resources`. -**MUST** use lowercase kebab-case for `name` (pattern: `^[a-z][a-z0-9-]*$`). This becomes the route prefix (`/api/{name}`) and the key on the AppKit instance. +**MUST** use lowercase kebab-case for `name` (pattern: `^[a-z][a-z0-9-]*$`). This becomes the route prefix (`/api/{name}`) and the key on the AppKit instance. (This is a monorepo convention; custom plugins may use camelCase per the official docs.) **MUST** declare both `resources.required` and `resources.optional` arrays, even if empty. @@ -32,12 +34,14 @@ Reference guide for building AppKit plugins. Every guideline is prefixed with a **MUST** extend `Plugin` (the base class accepts an optional generic but core plugins use the plain form). -**MUST** declare a static `manifest` property typed with `PluginManifest<"your-name">`: +**MUST** declare a static `manifest` property. Core plugins import `manifest.json` as a JSON module, which requires `as` (JSON imports cannot use `satisfies`): ```ts static manifest = manifest as PluginManifest<"my-plugin">; ``` +> **Note:** The `<"my-plugin">` generic is a **SHOULD** (see Section 9: Type Safety). Inline manifests — as shown in the custom-plugins docs — should use `satisfies PluginManifest<"name">` instead, which is stricter and catches structural errors at compile time. + **MUST** export a `toPlugin`-wrapped factory as the public API. Mark the factory (not the class) as `@internal`: ```ts @@ -75,7 +79,7 @@ export const myPlugin = toPlugin(MyPlugin); ## 4. Interceptor Usage -**MUST** define execution defaults in a separate `defaults.ts` file as `PluginExecuteConfig` constants. This keeps route handlers clean and makes defaults testable. +**SHOULD** define execution defaults in a separate `defaults.ts` file as `PluginExecuteConfig` constants. This keeps route handlers clean and makes defaults testable. (Required when the plugin uses `execute()` or `executeStream()` with non-trivial settings; not needed for plugins that don't use execution interceptors.) **MUST** pass defaults via the `default` key in `PluginExecutionSettings`. User overrides go in `user`. The merge order is: method defaults <- plugin config <- user override. From dd3f954581a16acae3626b44ecc82aa90ea7ae46 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 20:28:08 +0200 Subject: [PATCH 8/8] refactor: extract shared plugin review guidance to reduce duplication Move category index, severity ordering, deduplication rules, and cache-key tracing instructions into a shared reference file so audit and review skills stay in sync from a single source of truth. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .claude/commands/audit-core-plugin.md | 20 ++-------- .claude/commands/review-core-plugin.md | 29 ++++---------- .claude/references/plugin-review-guidance.md | 40 ++++++++++++++++++++ 3 files changed, 50 insertions(+), 39 deletions(-) create mode 100644 .claude/references/plugin-review-guidance.md diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md index fc093dd9..64ed39d2 100644 --- a/.claude/commands/audit-core-plugin.md +++ b/.claude/commands/audit-core-plugin.md @@ -74,19 +74,9 @@ Treat each missing `MUST` file as a **MUST**-severity finding under the "Structu ## Step 5: Full Best-Practices Review -> **Deduplication:** If the same code issue is covered by guidelines in multiple categories, report it once under the most specific category and note that it also relates to the other category. Do not count it as separate findings in each category. +Before evaluating, read the shared review rules in `.claude/references/plugin-review-guidance.md` and apply them throughout this step (deduplication, cache-key tracing). -Evaluate the plugin code against **all 9 categories** from the best-practices reference: - -1. **Manifest Design** — Check `manifest.json` against all NEVER/MUST/SHOULD rules -2. **Plugin Class Structure** — Check the main plugin class against all NEVER/MUST/SHOULD rules -3. **Route Design** — Check `injectRoutes()` and route handlers against all NEVER/MUST/SHOULD rules -4. **Interceptor Usage** — Check `execute()`/`executeStream()` calls and `defaults.ts` against all NEVER/MUST/SHOULD rules. **Important:** When evaluating cache configuration in `defaults.ts`, the codebase uses a two-stage pattern where `defaults.ts` defines partial cache config (e.g., `enabled: true, ttl: 3600`) and the `cacheKey` is injected at the call site in the plugin class. Before flagging a NEVER violation for missing `cacheKey`, trace the cache config through to `execute()`/`executeStream()` call sites to verify whether a `cacheKey` is provided there. Only flag as NEVER if no `cacheKey` is provided at any point in the execution path. -5. **asUser / OBO Patterns** — Check OBO usage, cache key scoping, context enforcement against all NEVER/MUST/SHOULD rules -6. **Client Config** — Check `clientConfig()` return value against all NEVER/MUST/SHOULD rules -7. **SSE Streaming** — Check streaming usage and shutdown behavior against all NEVER/MUST/SHOULD rules -8. **Testing Expectations** — Check test files for coverage, mocking patterns, error path tests against all NEVER/MUST/SHOULD rules -9. **Type Safety** — Check types, exports, config interface, `any` usage against all NEVER/MUST/SHOULD rules +Evaluate the plugin code against **all 9 categories** from the Category Index in `plugin-review-guidance.md`. Check each category's NEVER/MUST/SHOULD rules from the best-practices reference. For each guideline in each category, determine whether the plugin **passes**, **violates**, or is **not applicable** (e.g., SSE rules for a non-streaming plugin). Record findings with: @@ -128,11 +118,7 @@ And `{count}` is the number of findings (0 if pass). ### Detailed Findings (output second, severity-first) -Group all findings across all categories and sort by severity: - -1. **NEVER** findings first (most critical) -2. **MUST** findings second -3. **SHOULD** findings last +Group all findings across all categories and sort by severity per the Severity Ordering rule in `plugin-review-guidance.md`. For each finding, output: diff --git a/.claude/commands/review-core-plugin.md b/.claude/commands/review-core-plugin.md index 2c441874..6db26038 100644 --- a/.claude/commands/review-core-plugin.md +++ b/.claude/commands/review-core-plugin.md @@ -62,22 +62,9 @@ Deduplicate the list. You will run Steps 4-6 for **each** detected plugin. ## Step 4: Category Scoping -For each plugin being reviewed, map the changed files to the relevant best-practices categories: - -| Changed file pattern | Category | -|---|---| -| `manifest.json` | 1. Manifest Design | -| Main plugin class file (the primary `.ts` file, not index/types/defaults) | 2. Plugin Class Structure | -| Main plugin class file (the primary `.ts` file, not index/types/defaults) | 9. Type Safety | -| Any file containing route registration (`this.route(`, `injectRoutes`) | 3. Route Design | -| `defaults.ts`, or any file containing `execute(` / `executeStream(` calls with interceptor settings | 4. Interceptor Usage | -| Any file containing `asUser(`, `.obo.sql`, `isInUserContext`, `runInUserContext` | 5. asUser / OBO Patterns | -| Any file containing `clientConfig()` | 6. Client Config | -| Any file containing `executeStream(`, `streamManager`, `AsyncGenerator`, SSE-related code | 7. SSE Streaming | -| Files under `tests/` or files ending in `.test.ts` | 8. Testing Expectations | -| `types.ts`, config interfaces, `exports()` return types | 9. Type Safety | - -Read the actual changed file contents with `git diff ...HEAD -- ` to determine which patterns are present. A single file may trigger multiple categories. +For each plugin being reviewed, use the Category Index from `.claude/references/plugin-review-guidance.md` as the canonical list of categories. Map changed files to relevant categories using the "What to check" column as a guide — match file names and code patterns (e.g., `this.route(` → Route Design, `asUser(` → asUser / OBO Patterns). A single file may trigger multiple categories. + +Read the actual changed file contents with `git diff ...HEAD -- ` to determine which patterns are present. Record which of the 9 categories are **relevant** (at least one changed file maps to them) and which are **skipped** (no changed files map to them). @@ -89,12 +76,14 @@ For each **relevant** category identified in Step 4, extract all NEVER, MUST, an ## Step 6: Best-Practices Review +Before evaluating, read the shared review rules in `.claude/references/plugin-review-guidance.md` and apply them throughout this step (deduplication, cache-key tracing). + For each plugin detected in Step 3, review the changed code against the scoped guidelines from Step 5. For each finding: - Identify the **severity** (NEVER, MUST, or SHOULD) - Identify the **category** (e.g., "Manifest Design", "Route Design") -- Cite the specific guideline being violated or satisfied. **Important:** For Interceptor Usage findings (Category 4), when evaluating cache configuration, trace `cacheKey` through to `execute()`/`executeStream()` call sites before flagging. The codebase commonly defines partial cache config in `defaults.ts` and injects `cacheKey` at the call site. Only flag as NEVER if no `cacheKey` is provided anywhere in the execution path. +- Cite the specific guideline being violated or satisfied - Reference the exact file and line(s) involved - Provide a concrete fix if it is a violation @@ -104,11 +93,7 @@ For each finding: For each plugin reviewed, output a section with the plugin name as the heading. -**Order findings by severity, not by category:** - -1. **NEVER violations** (most critical) — these are security or breakage blockers -2. **MUST violations** — these are correctness requirements -3. **SHOULD violations** — these are quality recommendations +Order findings by severity per the Severity Ordering rule in `plugin-review-guidance.md`. Each finding should follow this format: diff --git a/.claude/references/plugin-review-guidance.md b/.claude/references/plugin-review-guidance.md new file mode 100644 index 00000000..89c0d255 --- /dev/null +++ b/.claude/references/plugin-review-guidance.md @@ -0,0 +1,40 @@ +# Plugin Review Guidance + +Shared evaluation rules for any skill that reviews plugin code against the best-practices categories. + +## Category Index + +The canonical list of best-practices categories (defined in `plugin-best-practices.md`): + +| # | Category | What to check | +|---|----------|---------------| +| 1 | Manifest Design | `manifest.json` fields, resource declarations, naming | +| 2 | Plugin Class Structure | Base class, static manifest, factory export, config, shutdown | +| 3 | Route Design | `injectRoutes()`, `this.route()`, endpoint naming, body parsing | +| 4 | Interceptor Usage | `execute()`/`executeStream()` calls, `defaults.ts`, cache keys | +| 5 | asUser / OBO Patterns | `asUser(req)`, `.obo.sql`, cache key scoping, context enforcement | +| 6 | Client Config | `clientConfig()` return value, serialization, secret exposure | +| 7 | SSE Streaming | `executeStream()`, `AsyncGenerator`, `streamManager`, shutdown | +| 8 | Testing Expectations | Co-located tests, mocking, error paths, cache key coverage | +| 9 | Type Safety | Config interface, `exports()` return type, `any` usage | + +## Severity Ordering + +Always order findings by severity, not by category: + +1. **NEVER** findings first — security or breakage blockers +2. **MUST** findings second — correctness requirements +3. **SHOULD** findings last — quality recommendations + +## Deduplication + +If the same code issue is covered by guidelines in multiple categories, report it once under the most specific category and note that it also relates to the other category. Do not count it as separate findings in each category. + +## Cache-Key Tracing + +When evaluating cache configuration (Interceptor Usage, Category 4), the codebase uses a two-stage pattern: + +1. `defaults.ts` defines partial cache config (e.g., `enabled: true, ttl: 3600`) **without** a `cacheKey`. +2. The `cacheKey` is injected at the `execute()` / `executeStream()` call site in the plugin class. + +Before flagging a NEVER violation for missing `cacheKey`, trace the cache config through to every call site. Only flag if no `cacheKey` is provided at any point in the execution path.