diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md new file mode 100644 index 00000000..0a9ffd64 --- /dev/null +++ b/.claude/commands/audit-core-plugin.md @@ -0,0 +1,153 @@ +--- +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 + +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 + +> **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. **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 + +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: + +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/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. diff --git a/.claude/commands/review-core-plugin.md b/.claude/commands/review-core-plugin.md new file mode 100644 index 00000000..d701f35b --- /dev/null +++ b/.claude/commands/review-core-plugin.md @@ -0,0 +1,140 @@ +--- +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, 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. **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 + +## 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") diff --git a/.claude/references/plugin-best-practices.md b/.claude/references/plugin-best-practices.md new file mode 100644 index 00000000..e0f5f970 --- /dev/null +++ b/.claude/references/plugin-best-practices.md @@ -0,0 +1,181 @@ +# 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` (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">`: + +```ts +static manifest = manifest as PluginManifest<"my-plugin">; +``` + +**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 + +**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()` 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.