Add code-review agent skill for Workers code validation

.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 -- <file>`
+- 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 <files>                  # 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.

.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\.' <file> | grep -v 'this\.env\.' | grep -v '//'
+
+# Inside module export handlers: flag this.env.X (should be env.X)
+grep -n 'this\.env\.' <file>
+```
+
+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 <file>
+```
+
+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`.