diff --git a/.github/review-layers/harper/common.md b/.github/review-layers/harper/common.md new file mode 100644 index 0000000..18de2c4 --- /dev/null +++ b/.github/review-layers/harper/common.md @@ -0,0 +1,38 @@ +# Harper common conventions + +Version-agnostic review guidance for repos in the Harper ecosystem. The repo's own `CLAUDE.md` may duplicate or extend this — when in conflict, the repo's CLAUDE.md wins (it's closer to the source). + +## Non-obvious behavior + +- **`GenericTrackedObject` + spread.** `{ ...obj }` on a Harper _generic_ tracked object (the shape for `session.oauth`, `session.oauthUser`, and any tracked object without a declared schema) copies nothing — properties are served through a Proxy `get` trap, not as own enumerable properties. `Object.keys(obj)` returns `[]`. Use explicit property access: `{ provider: obj.provider, ... }`. Typed TrackedObjects (table rows with declared attributes) may behave differently; verify against the specific tracked type rather than assuming either way. Any test using spread on session fields is testing a plain object, not the production shape. +- **`session.oauth` vs `session.delete()`.** Some session objects expose `.delete(id)` (Harper production shape — destroys the DB record). Some don't (in-memory test mocks). Code calling `clearOAuthSession` or similar may mutate `.oauth`/`.oauthUser` in-memory on test shapes but destroy the entire session record in production. Tests that exercise only one shape hide the divergence. +- **`npm run build` tolerance.** Many Harper repos use `tsc || true` so the build passes even with type errors. "Build passes" ≠ "types are sound." Type correctness must be verified separately (editor diagnostics, a dedicated typecheck script, or CI step). +- **`npm run lint` is ESLint-only**, not a typecheck. + +## Code conventions + +- TypeScript strict mode; ES modules with `.js` extensions in imports; named exports only (no default exports). +- Logging uses the optional-chain pattern `logger?.info?.()` — the `logger` argument is frequently undefined in library code, so code must not assume it's present. +- Error classes come from each repo's `src/errors.ts` OR the framework package (`harper` v5 / `harperdb` v4). All custom errors include a `statusCode` property. +- Tokens, passwords, session IDs — **never** log, never return in responses, never include in error messages. + +## Review checks specific to Harper + +- **`scope.resources` / `scope.server` usage.** Declared optional in the Harper type but always assigned in the Scope constructor. Code should either guard once at entry or use narrowed locals, not sprinkle `?.` / `!` throughout. +- **`static loadAsInstance = false` (Resource API v2).** Harper instantiates the class per request; do NOT rely on shared mutable instance state across requests. If per-request state is stored, it belongs on the context (`this.getContext()`). +- **Unused runtime dependencies.** Harper repos target minimal runtime deps. New ones require explicit justification in the PR description (some repos maintain a `dependencies.md` for this). +- **Dev/prod dependency mismatch.** Production code paths must only import from `dependencies` or `peerDependencies`. A `devDependencies` package referenced by runtime code (e.g. `await import('undici')` in an HTTPS path, a runtime helper imported from a test-only utility) breaks deployment for any consumer that doesn't share the dev environment. Caveat: some packages are also Node built-ins on recent Node versions (`undici` on Node 22+); using them without an explicit dep is OK ONLY if `engines.node` declares the floor that makes them built-in. When the runtime requirement and the declared engine floor disagree, flag both — the missing dep AND the permissive `engines.node`. + +## Documentation boundary (defer to Harper docs) + +Harper maintains its own documentation at [docs.harperdb.io](https://docs.harperdb.io) covering core, pro, and fabric. App, sample, and plugin repos should: + +- Document what is specific to the app/plugin itself — env var names, config shape, setup flow, integration API. +- **Link** to the Harper documentation for anything not app/component-specific: deployment mechanics, runtime env var handling, Fabric configuration, core database behavior, SQL translator, replication, etc. + +Flag PRs that re-explain Harper behavior in-repo when a link to the authoritative Harper docs would be more maintainable. Re-explanation creates drift — Harper updates its docs, the copy doesn't. + +Borderline calls (judgment, not a blocker): + +- Short factual reminders where a link alone is too thin (e.g. "Harper reads env vars directly from the process environment — see [...]") are fine. +- Duplicating whole Harper docs sections inline is not — link. diff --git a/.github/review-layers/harper/v5.md b/.github/review-layers/harper/v5.md new file mode 100644 index 0000000..1a54d90 --- /dev/null +++ b/.github/review-layers/harper/v5.md @@ -0,0 +1,73 @@ +# Harper v5 conventions + +Review guidance for repos that target Harper v5. Apply this layer when the repo's `peerDependencies` declares `harper`. + +> **Authoritative source.** `HarperFast/skills/harper-best-practices/` is the customer-facing guidance for building on Harper. This document is a **review lens** — what to CHECK when reviewing changes. If anything here contradicts the skills, the skills win for user-facing guidance; this file's job is reviewer discipline. + +## Package and CLI + +- **Package**: `harper`. Imports: `from 'harper'`. Peer dep range: `harper >=5.0.0`. +- **CLI binary**: `harper` (e.g., `harper deploy_component`). PR diffs introducing v4-era `harperdb` imports or `harperdb` CLI calls are stale — flag them. + +## Resource API v2 + +v5 has two co-existing dispatch patterns. A review of any Resource — or anything that wraps a Resource — must account for BOTH. + +- **Instance-method pattern**: + + ```ts + class MyResource extends Resource { + static loadAsInstance = false; + async get(target) { + const request = this.getContext(); + ... + } + } + ``` + + Harper's Resource base static `get` is `transactional(fn)` — it creates a per-request instance and calls the instance method. + +- **Static-method pattern** (v5 migration guide explicitly recommends this): + + ```ts + class MyResource extends Resource { + static async get(target, request) { ... } + } + ``` + + Harper's REST dispatcher calls `Class.get(target, request)` at the STATIC level. The user's static shadows the Resource base's transactional static — no instance is created. + +- **Wrappers must cover both surfaces.** A wrapper that intercepts only instance methods is silently bypassed when the user adopts the static-method pattern. Full coverage wraps both and de-duplicates work (e.g. via a WeakSet keyed by the request context) so a single dispatch doesn't run validation twice. + +- **405 vs 204.** Harper's REST dispatcher returns `405 Method Not Allowed` when the method is undefined on the resource. A wrapper that defines all five verbs unconditionally — even when the parent doesn't implement them — replaces `undefined` with a function that returns `undefined`, and Harper serializes that as `204 No Content`. Only wrap verbs the parent actually implements. + +## Context + `getContext()` + +- Request-scoped state lives on the context returned by `this.getContext()` (inside an instance method). For static methods, Harper passes the request as the second arg: `static async get(target, request)`. +- `this.getCurrentUser()` returns the authenticated user off the current context. Checks against `user.role.permission.super_user` are the standard "require superuser" guard. +- `scope.resources` and `scope.server` are typed as optional in v5 but always assigned in the Scope constructor (`components/Scope.ts:70-71`). Plugins should guard once at `handleApplication` entry and use narrowed locals. + +## `config.yaml` and resource loading + +- `jsResource.files` controls which files Harper loads as resources. A change to this glob / pattern / directory / extension IS a meaningful review concern — it changes what actually gets served. +- `config.yaml` also carries plugin entry point (`pluginModule`), `graphqlSchema.files`, and runtime defaults (e.g. `redirectUri`, `scope`, `defaultRole`). Treat `config.yaml` changes as code, not docs — a typo breaks the plugin/app at runtime. + +## Environment variables and `.env` + +- **Harper runtime** reads `process.env.*`. It does not itself parse a `.env` file on startup. +- **Harper app template** (`npm create harper`) scaffolds `dotenv-cli` in `package.json` scripts. `npm run dev` / `npm run deploy` invokes `dotenv -- npm run