diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md new file mode 100644 index 00000000000..693e4fb2fa3 --- /dev/null +++ b/.agents/skills/code-review/SKILL.md @@ -0,0 +1,128 @@ +--- +name: code-review +description: Reviews Workers and Cloudflare Developer Platform code for type correctness, API usage, and configuration validity. Load when reviewing TypeScript/JavaScript using Workers APIs, wrangler.jsonc/toml config, or Cloudflare bindings (KV, R2, D1, Durable Objects, Queues, Vectorize, AI, Hyperdrive). +--- + +Your knowledge of Cloudflare Workers APIs, types, and wrangler configuration may be outdated. **Prefer retrieval over pre-training** for any Workers code review task. + +## Retrieval Sources + +Fetch the **latest** versions before reviewing. The current repo may have outdated dependencies — always validate against what is currently published, not what is locally installed. + +| Source | How to retrieve | Use for | +| ---------------------- | ------------------------------------------------------- | ------------------------------------------------------ | +| Wrangler config schema | See `references/wrangler-config.md` for retrieval steps | Config fields, binding shapes, allowed values | +| Workers types | See `references/workers-types.md` for retrieval steps | API usage, handler signatures, binding types | +| Cloudflare docs | `https://developers.cloudflare.com/workers/` | API reference, compatibility dates/flags, binding docs | + +## FIRST: Fetch Latest References + +Retrieve the latest wrangler schema and workers types before reviewing. See the reference files for specific retrieval commands. If the current repo's `node_modules` has an older version, **prefer the latest published version** — documentation should reflect the current state of the platform, not a pinned dependency. + +## Review Process + +### 1. Build Context + +Read full files, not just diffs or isolated snippets. Code that looks wrong in isolation may be correct given surrounding logic. + +- Identify the purpose of the code: is it a complete Worker, a snippet, a configuration example? +- Check git history for context: `git log --oneline -5 -- ` +- Understand which bindings, types, and patterns the code depends on + +### 2. Categorize the Code + +Every code block falls into one of three categories. Review **in the context** of its category. + +| Category | Definition | Expectations | +| ----------------- | -------------------------------------------------------------------- | ---------------------------------------------------- | +| **Illustrative** | Demonstrates a concept; uses comments for most logic | Correct API names, realistic signatures | +| **Demonstrative** | Functional but incomplete; would work if placed in the right context | Syntactically valid, correct APIs and binding access | +| **Executable** | Standalone and complete; runs without modification | Compiles, runs, includes imports and config | + +### 3. Validate with Tools + +Run type-checking and linting. Tool output is evidence, not opinion. + +```bash +npx tsc --noEmit # TypeScript errors +npx eslint # Lint issues +``` + +For config files, validate against the latest wrangler config schema (see `references/wrangler-config.md` for retrieval) and check that all fields, binding types, and values conform. + +### 4. Check Against Rules + +See `references/workers-types.md` for type system rules, `references/wrangler-config.md` for config validation, and `references/common-patterns.md` for correct API patterns. + +**Quick-reference rules:** + +| Rule | Detail | +| ----------------------- | ---------------------------------------------------------------------------------------------------------------------------------------- | +| Binding access | `env.X` in module export handlers; `this.env.X` in classes extending platform base classes. See `references/common-patterns.md`. | +| No `any` | Never use `any` for binding types, handler params, or API responses. Use proper generics. | +| No type-system cheats | Flag `as unknown as T`, unjustified `@ts-ignore`, unsafe assertions. See `references/workers-types.md`. | +| Config-code consistency | Binding names in wrangler config must match `env.X` usage in code. See `references/wrangler-config.md`. | +| Required config fields | Verify against the wrangler config schema — do not assume which fields are required. | +| Concise examples | Examples should focus on core logic. Minimize boilerplate that distracts from what the code teaches. | +| Floating promises | Every `Promise` must be `await`ed, `return`ed, `void`ed, or passed to `ctx.waitUntil()`. See `references/common-patterns.md`. | +| Serialization | Data crossing Queue, Workflow step, or DO storage boundaries must be structured-clone serializable. See `references/common-patterns.md`. | +| Streaming | Large/unknown payloads must stream, not buffer. Flag `await response.text()` on unbounded data. | +| Error handling | Minimal but present — null checks on nullable returns, basic fetch error handling. Do not distract with verbose try/catch. | + +### 5. Assess Risk + +| Risk | Triggers | +| ---------- | ---------------------------------------------------------------------------------------------------------- | +| **HIGH** | Auth, crypto, external calls, value transfer, validation removal, access control, binding misconfiguration | +| **MEDIUM** | Business logic, state changes, new public APIs, error handling, config changes | +| **LOW** | Comments, logging, formatting, minor style | + +Focus deeper analysis on HIGH risk. For critical paths, check blast radius: how many other files reference this code? + +**Security logic escalation**: for crypto, auth, and timing-sensitive code, do not stop at verifying API calls are correct. Examine the surrounding logic for flaws that undermine the security property (e.g., correct `timingSafeEqual` call but early return on length mismatch). See `references/common-patterns.md` Security section. + +## Anti-patterns to Flag + +| Anti-pattern | Why it matters | +| ----------------------------------------------------------------------------- | -------------------------------------------------------------------------- | +| `any` on `Env` or handler params | Defeats type safety for every binding access downstream | +| `as unknown as T` double-cast | Hides real type incompatibilities — fix the underlying design | +| `@ts-ignore` / `@ts-expect-error` without explanation | Masks errors silently; require a comment justifying each suppression | +| Buffering unbounded data (`await res.text()`, `await res.json()` on streams) | Memory exhaustion on large payloads; use streaming | +| Hardcoded secrets or API keys | Use `env` bindings and `wrangler secret` | +| `blockConcurrencyWhile` on every request | Only for initialization; blocks all concurrent requests | +| Single global Durable Object | Creates a bottleneck; shard by coordination atom | +| In-memory-only state in DOs | Lost on eviction; persist to SQLite storage | +| Missing DO migrations in config | New DO classes require migration entries or deployment fails | +| Floating promises (`step.do()`, `fetch()` without `await`) | Silent bugs — drops results, breaks Workflow durability, ignores errors | +| Non-serializable values across boundaries (`Response`, `Error` in step/queue) | Compiles but fails at runtime; extract plain data before crossing boundary | +| `implements` instead of `extends` on platform base classes | Legacy pattern — loses `this.ctx`, `this.env` access from base class | + +## What NOT to Flag + +- Style not enforced by linters +- "Could be cleaner" when code is correct and clear +- Theoretical performance concerns without evidence +- Missing features not in scope of the example +- Pre-existing issues in unchanged code +- TOML config in existing docs (only flag for new content) + +## Output Format + +``` +**[SEVERITY]** Brief description +`file.ts:42` — explanation with evidence (tool output, type error, config mismatch) +Suggested fix: `code` (if applicable) +``` + +Severity: **CRITICAL** (security, data loss, crash) | **HIGH** (type error, wrong API, broken config) | **MEDIUM** (missing validation, edge case, outdated pattern) | **LOW** (style, minor improvement) + +End with a summary count by severity. If no issues found, say so directly. + +## Principles + +- **Be certain.** Investigate before flagging. If you cannot confirm an API, binding pattern, or config field, retrieve the docs or schema first. +- **Provide evidence.** Reference line numbers, tool output, schema fields, or type definitions. +- **Correctness over completeness.** A concise example that works is better than a comprehensive one with errors. +- **Respect existing patterns.** Do not flag conventions already established in the codebase unless actively harmful. +- **Focus on what developers will copy.** Code in documentation gets pasted into production. Treat it accordingly. diff --git a/.agents/skills/code-review/references/common-patterns.md b/.agents/skills/code-review/references/common-patterns.md new file mode 100644 index 00000000000..4c8bb844378 --- /dev/null +++ b/.agents/skills/code-review/references/common-patterns.md @@ -0,0 +1,134 @@ +# Common Patterns — What to Verify + +Do not memorize patterns. Retrieve current examples from Cloudflare docs and verify code against the current type definitions. This file describes _what to check_, not _what the code should look like_. + +## Retrieval + +When reviewing a pattern you are unsure about, fetch the current reference: + +| Topic | Where to look | +| --------------------- | ------------------------------------------------------------------------------------------------------------------------------- | +| Handler signatures | Latest `@cloudflare/workers-types` (see `references/workers-types.md`) — search for `ExportedHandler`, `WorkerEntrypoint`, etc. | +| Binding APIs | Same types file — search for the binding type name (e.g., `KVNamespace`, `R2Bucket`) | +| Wrangler config shape | Latest wrangler config schema (see `references/wrangler-config.md`) | +| Platform base classes | Search for imports from `"cloudflare:workers"` in the type definitions | +| Cloudflare docs | `https://developers.cloudflare.com/workers/` for guides, examples, and API reference | + +## Binding Access + +The single most common error in Workers code. The rule is stable: + +- **Module export handlers** (`fetch`, `scheduled`, `queue`, `email`): bindings via `env.X` parameter +- **Platform base classes** (WorkerEntrypoint, DurableObject, Workflow, Agent, and subclasses): bindings via `this.env.X` + +Flag any code that uses `env.X` inside a class extending a platform base class, or `this.env.X` inside a module export handler. + +**Mechanical check**: do not rely on reading alone. Search the file: + +```bash +# Inside class bodies: flag bare env.X (should be this.env.X) +grep -n 'env\.' | grep -v 'this\.env\.' | grep -v '//' + +# Inside module export handlers: flag this.env.X (should be env.X) +grep -n 'this\.env\.' +``` + +Cross-reference each match against its surrounding context (class body vs module export) to confirm. + +## Stale Class and API Patterns + +Cloudflare platform classes evolve. Old patterns survive in docs long after APIs change. Before approving class-based code, verify against the latest type definitions: + +- **`extends` vs `implements`**: platform classes (DurableObject, WorkerEntrypoint, WorkflowEntrypoint) use `extends`, not `implements`. The `implements` pattern is legacy and loses access to `this.ctx` and `this.env`. Flag any `implements DurableObject` or similar. +- **Import paths**: verify the module specifier matches what the types actually export. Common mistake: importing from `"cloudflare:workflows"` when the correct path is `"cloudflare:workers"` (or vice versa as APIs evolve). Always check. +- **Renamed properties**: APIs rename properties across versions (e.g., `this.state` to `this.ctx` in Durable Objects). Search the type definitions for the class and confirm which properties exist. +- **Constructor signatures**: base class constructors change. Verify the expected parameters against the type definition rather than assuming `(state, env)` or `(ctx, env)`. + +Do not trust your training data for these. Read the types. + +## Floating Promises + +Unawaited async calls are a frequent source of silent bugs — especially in Workflow `step.*` methods where a missing `await` breaks durability guarantees. + +A floating promise is a Promise-valued expression that is not handled via `await`, `return`, `.then()/.catch()`, `void`, or `ctx.waitUntil()`. The `typescript/no-floating-promises` rule from oxlint catches these mechanically when type-aware linting is available. + +Check for: + +- **`step.do()` and `step.sleep()` without `await`**: silently drops the step result and breaks the Workflow execution order. This is the most common instance in docs. +- **`fetch()` without `await`**: fires the request but never reads the response. +- **`.map(async ...)` producing `Promise[]`**: must be wrapped in `Promise.all()` or similar. +- **Fire-and-forget**: if intentional, use `ctx.waitUntil(promise)` in handlers or `void promise` to signal intent. Bare promise statements are bugs until proven otherwise. + +**Mechanical check**: if the project supports it, run oxlint with type-aware linting: + +```bash +npx oxlint --type-aware --deny typescript/no-floating-promises +``` + +Otherwise, search manually for async method calls (especially `step.do`, `step.sleep`, `step.sleepUntil`, `fetch`) and verify each is `await`ed or explicitly handled. + +## Serialization Boundaries + +Data that crosses a serialization boundary must be structured-clone serializable. This is a runtime constraint — the code compiles fine but fails at runtime. + +Serialization boundaries in Workers: + +- **Queue messages**: the body passed to `.send()` or `.sendBatch()` +- **Workflow step return values**: whatever `step.do()` callback returns is persisted to durable storage +- **DO storage**: values passed to `storage.put()` or returned from SQL queries +- **`postMessage()`**: data sent to/from Web Workers or WebSocket messages + +**Non-serializable types to flag:** + +- `Response` and `Request` objects — common mistake in `step.do()` callbacks that `return await fetch(...)` +- `Error` objects — native `Error` is not structured-clone serializable; extract `.message` and `.stack` into a plain object +- Functions, class instances with methods, `Map`/`Set` (unless using structured clone explicitly) +- Symbols + +When reviewing code that returns data across these boundaries, verify the value is a plain object, array, string, number, boolean, null, `ArrayBuffer`, or `Date`. + +## Streaming + +These are runtime constraints, not API-specific. They do not go stale. + +- Large or unknown-size payloads must stream. Flag `await response.text()`, `await response.json()`, or `await response.arrayBuffer()` on unbounded data. +- R2 object bodies are streams by default. Flag `.text()` or `.arrayBuffer()` on R2 objects unless the code has already verified the object is small. +- When proxying or transforming a response, pipe the stream rather than buffering it. +- Small, bounded payloads (config files, API responses with known limits) are fine to buffer. + +## Error Handling + +Minimal but present. The goal is correctness without distracting from the example's purpose. + +- **Nullable returns**: APIs that return `T | null` (e.g., KV `.get()`, R2 `.get()`, D1 `.first()`) need null checks before use. Flag code that asserts non-null (`!`) without justification. +- **Network requests**: basic error handling or at minimum a status check. +- **Workflow steps**: validation failures should use `NonRetryableError` (from `"cloudflare:workers"`) to prevent infinite retries. Verify this import path against current types. +- **Verbose try/catch**: flag excessive error handling that obscures the example's core logic. A docs example should show the happy path clearly. + +## Security + +These are principles, not API-specific. + +- **No hardcoded secrets.** API keys, tokens, passwords must come from env bindings set via `wrangler secret put`. Flag any string literal that looks like a credential. +- **No weak hashes for security.** MD5 and SHA-1 must not be used in security contexts (signing, auth, integrity). SHA-256 minimum via the Web Crypto API. +- **Auth as a stub.** If auth is not the point of the example, it should be omitted or shown as a minimal stub. Flag examples that implement full auth flows when the page topic is something else. +- **Correct API usage does not mean correct logic.** When reviewing code involving crypto, timing-sensitive comparisons, auth, or access control, do not stop at verifying the API calls are correct. Examine the surrounding logic for flaws that undermine the security property. Example: calling `timingSafeEqual` correctly but short-circuiting with an early return on length mismatch leaks the secret's length via timing side-channel. The API call is right; the logic is wrong. Escalate security-sensitive code to a higher scrutiny level — verify the invariant the code claims to provide, not just the function signatures. + +## Conciseness + +Examples should teach one thing clearly. Flag: + +- Boilerplate that distracts from the core concept (full error handling, logging, CORS, routing) when those are not the topic +- Unused imports or bindings +- Comments that restate what the code does instead of explaining _why_ +- Multiple patterns crammed into a single example when they should be separate + +## Durable Objects — Specific Checks + +These constraints are architectural, not API-version-specific: + +- **Persist state to storage, not in-memory fields.** Class properties are lost on eviction. SQLite storage (via `ctx.storage.sql`) persists. +- **`blockConcurrencyWhile` is for initialization only.** It blocks all concurrent requests. Flag code that calls it on every request or across external I/O. +- **One alarm per DO instance.** Setting a new alarm replaces the previous one. Alarm handlers must be idempotent. +- **Shard by coordination atom.** A single global DO is a bottleneck. Each DO should represent one entity (room, user, session, document). +- **DO migrations required.** New DO classes need a migration entry in the wrangler config. See `references/wrangler-config.md`. diff --git a/.agents/skills/code-review/references/workers-types.md b/.agents/skills/code-review/references/workers-types.md new file mode 100644 index 00000000000..bc878296c08 --- /dev/null +++ b/.agents/skills/code-review/references/workers-types.md @@ -0,0 +1,86 @@ +# Workers Types + +## How to Retrieve Current Types + +Prefer generated types over baked-in knowledge. Types change with compatibility dates and new bindings. The repo you are working in may have an older `@cloudflare/workers-types` version — **always validate against the latest published types**. + +### Primary: fetch latest `@cloudflare/workers-types` + +```bash +# Fetch the latest type definitions to a temp directory +mkdir -p /tmp/workers-types-latest && \ + npm pack @cloudflare/workers-types --pack-destination /tmp/workers-types-latest && \ + tar -xzf /tmp/workers-types-latest/cloudflare-workers-types-*.tgz -C /tmp/workers-types-latest +# Types are now at /tmp/workers-types-latest/package/index.d.ts +``` + +Search this file for the specific type, class, or interface under review. Do not guess type names — look them up. + +### Alternative: `wrangler types` + +If working in a project with a wrangler config, `npx wrangler types` generates a `.d.ts` file with a typed `Env` interface derived from the config. This is useful for binding types but reflects the locally installed wrangler version. + +### Fallback: local `node_modules` + +Read `node_modules/@cloudflare/workers-types/index.d.ts` if the above approaches are unavailable. Note the installed version and flag if it appears significantly outdated. + +The package provides date-versioned entrypoints (e.g., `2023-07-01/`) and `latest/`. Check the project's `tsconfig.json` to determine which entrypoint is in use. + +## What to Validate + +### Env interface + +- Every binding must have a specific type. Flag `any`, `unknown`, `object`, or `Record` on bindings. +- Binding types that accept generic parameters (like Durable Object namespaces, Queues, Service bindings for RPC) must include them. Read the type definition to confirm which types are generic. +- Binding names must match the wrangler config exactly (see `references/wrangler-config.md`). + +### Handler and class signatures + +Workers code uses several base classes and handler patterns. The correct signatures change over time — **always verify against the current type definitions** rather than assuming. + +Verify: + +- The correct import path (most Workers platform classes import from `"cloudflare:workers"`) +- The generic type parameter on base classes (e.g., ``) +- Binding access pattern: `env.X` in module export handlers, `this.env.X` in classes extending platform base classes +- `ExecutionContext` as the third param in module export handlers (needed for `ctx.waitUntil()`) + +### Return types + +- `fetch()` handlers must return `Promise` +- `scheduled()` handlers must return `Promise` +- Verify other handler return types against the type definitions + +## Type Integrity Rules + +These are principles, not API-specific. They do not go stale. + +### No `any` + +Never use `any` for binding types, handler parameters, or API responses. The type system exists to catch binding mismatches, incorrect API usage, and missing null checks. Using `any` defeats all of this. + +### No double-casting + +`as unknown as T` hides real type incompatibilities. If a cast is needed, the underlying design needs fixing — a missing generic parameter, a wrong import, or an actual API mismatch. Investigate instead of casting. + +### Justify every suppression + +`@ts-ignore` and `@ts-expect-error` must include a comment explaining why suppression is necessary. Prefer `@ts-expect-error` (fails when the error is fixed) over `@ts-ignore` (silently persists forever). + +### Prefer `satisfies` over `as` + +`satisfies` validates structure at compile time without widening the type. `as` silently allows incorrect structure. Use `satisfies` for config objects, handler exports, and any value where you want to confirm a shape. + +### Validate, do not assert + +When receiving untyped data (JSON responses, parsed bodies, external input), validate it with a schema or narrow it with type guards. Do not assert it into a type with `as`. + +## Common Mistakes to Check + +Look for these patterns and verify against the current type definitions: + +- Generic parameters omitted on binding types that require them +- `Response` or `Request` type conflicts between Workers types and DOM `lib.dom.d.ts` — check tsconfig `lib` and `types` settings +- Missing handler parameters (especially `ExecutionContext` as third param) +- `JSON.parse()` results used without validation or type narrowing +- Binding access using `env.X` inside a class body (should be `this.env.X`) or `this.env.X` in a module export handler (should be `env.X`) diff --git a/.agents/skills/code-review/references/wrangler-config.md b/.agents/skills/code-review/references/wrangler-config.md new file mode 100644 index 00000000000..cb409c29c4d --- /dev/null +++ b/.agents/skills/code-review/references/wrangler-config.md @@ -0,0 +1,79 @@ +# Wrangler Config Validation + +## Schema Retrieval + +The authoritative schema is bundled with the `wrangler` npm package as `config-schema.json`. It is a JSON Schema (draft-07) document defining the full `RawConfig` type. + +**Always fetch the latest published schema** — the repo you are working in may have an older wrangler version pinned. To retrieve the latest: + +```bash +# Fetch the latest schema to a temp directory +mkdir -p /tmp/wrangler-latest && \ + npm pack wrangler --pack-destination /tmp/wrangler-latest && \ + tar -xzf /tmp/wrangler-latest/wrangler-*.tgz -C /tmp/wrangler-latest +# Schema is now at /tmp/wrangler-latest/package/config-schema.json +``` + +If that fails, fall back to reading `./node_modules/wrangler/config-schema.json` from the current project — but note the version may be outdated. + +Do not guess field names or structures — look them up in the schema. + +## Format + +- **JSONC** (`wrangler.jsonc`) — preferred for new projects. Supports comments. +- **JSON** (`wrangler.json`) — valid but no comments. +- **TOML** (`wrangler.toml`) — legacy format. Acceptable in existing content; only flag in new content. + +## What to Validate + +### Required fields + +For executable examples, verify the config includes `name`, `compatibility_date`, and `main`. Check the schema for current required fields — these may change. + +### Binding declarations + +Every binding type has a specific top-level key and required sub-fields defined in the schema. Do not memorize these — read the schema's `RawConfig.properties` to find the correct key name and shape for each binding type. + +Common validation errors: + +- Wrong top-level key (e.g., `kv` instead of the correct key — check the schema) +- Missing required sub-fields within a binding declaration +- Mixing up singular vs plural key names + +### Binding-code consistency + +The `binding` or `name` field value in config must exactly match the property name used in code via `env.X`. + +Check: + +1. Every `env.X` reference in code has a corresponding binding declaration in config +2. Every binding in config is referenced in code (warn on unused) +3. Names match exactly (case-sensitive) +4. For Durable Objects: the `class_name` value matches the exact exported class name + +### Durable Object migrations + +New DO classes require a `migrations` entry in the config. Missing migrations cause deployment failures. Read the schema to confirm the current migration format and required fields. + +### Secrets + +Secrets must never appear in config files. They are set via `wrangler secret put`. If a `vars` block contains values that look like secrets (API keys, tokens, passwords), flag it. + +### Environment overrides + +The `env` key supports per-environment config overrides. Some fields inherit from the top level; others must be redeclared. Consult the schema for inheritance rules rather than assuming. + +## Common Config Mistakes + +These are procedural checks — verify each when reviewing config: + +| Check | What to look for | +| -------------------------- | --------------------------------------------------------------- | +| Missing `$schema` | Config should reference the wrangler schema via `$schema` field | +| Stale `compatibility_date` | In docs, use `$today` placeholder for auto-replacement | +| Missing DO migrations | Every new DO class needs a migration entry | +| Binding name mismatch | Config `binding`/`name` must exactly match `env.X` in code | +| Secrets in config | Never in `vars` — use `wrangler secret put` | +| Wrong binding key | Verify the top-level key name against the schema | +| Missing entrypoint | `main` is required for executable examples | +| `class_name` mismatch | Must match the exported class name, not the binding name |