diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md new file mode 100644 index 00000000..64ed39d2 --- /dev/null +++ b/.claude/commands/audit-core-plugin.md @@ -0,0 +1,141 @@ +--- +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}/` +> +> Available plugins can be listed with: `ls packages/appkit/src/plugins/` + +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 + +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? | +|---|---| +| `manifest.json` | MUST | +| Main plugin class file (any `.ts` file containing a class extending `Plugin`) | MUST | +| `types.ts` | MUST | +| `defaults.ts` | SHOULD | +| `index.ts` | MUST | +| `tests/` directory with at least one `.test.ts` file | MUST | + +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. + +## Step 5: Full 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). + +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: + +- **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 +- 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). + +### Detailed Findings (output second, severity-first) + +Group all findings across all categories and sort by severity per the Severity Ordering rule in `plugin-review-guidance.md`. + +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/create-core-plugin.md b/.claude/commands/create-core-plugin.md index bb234471..a72c6e19 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. @@ -65,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`: @@ -166,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; @@ -320,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`: @@ -339,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 new file mode 100644 index 00000000..6db26038 --- /dev/null +++ b/.claude/commands/review-core-plugin.md @@ -0,0 +1,125 @@ +--- +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` 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 + +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, 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). + +## 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 + +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 +- 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 per the Severity Ordering rule in `plugin-review-guidance.md`. + +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") diff --git a/.claude/references/plugin-best-practices.md b/.claude/references/plugin-best-practices.md new file mode 100644 index 00000000..007ccb62 --- /dev/null +++ b/.claude/references/plugin-best-practices.md @@ -0,0 +1,185 @@ +# 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. + +> **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. (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. + +**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` (the base class accepts an optional generic but core plugins use the plain form). + +**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 +export 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 + +**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. + +**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()` 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 (read operations in route handlers): +if (!isInUserContext()) { logger.warn("..."); } + +// Enforce pattern (programmatic API, e.g. createVolumeAPI): +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. 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.