Skip to content

Commit 370f2ac

Browse files
duyetduyetbot
duyet
and
duyetbot
authored
docs(plans): track the plans directory and add a status index (#371)
* docs(plans): track the plans directory and add a status index The 10 implementation plans were untracked, so they existed only on one machine while every PR referenced them by path, and no worktree could see them. Each plan also instructs its executor to "update this plan's row in plans/README.md" — a file that never existed, which three separate executors hit and correctly flagged rather than fabricating. Adds plans/README.md: status table with issue links, the cross-plan dependency graph, and a note that plan 008 is superseded (its target design preserves the /watch deadlock in #360, so its own regression test would hang). Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net> * docs(plans): add plans 011-014 for the held product/architecture issues Plans for the four issues deliberately held out of the parallel fix batch because they are product decisions rather than defined fixes: - 011 dashboard UI for the 4 unexposed primitives (#284) - 012 webhooks docs + dashboard UI (#285) - 013 demo sandbox / pre-signup try-it (#287) - 014 write atomicity via db.batch() (#342) 014 confirms D1 batch() semantics against Cloudflare docs: statements execute sequentially as one all-or-nothing SQL transaction, but all statements must be prepared before the call — no reading a result mid-batch and branching on it. All three call sites are batchable as-is; updateConversationMessageCount is already a blind relative SQL update rather than a read-then-write. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net> * docs(plans): mark 006 superseded and 012 docs-done; record merge order Plan 006 is superseded: main shipped a stronger fix for the same issue (#289/#290) in aaf05ac (PR #355) after this plan's ce9a1fa baseline. The Idempotency-Key claim is now embedded atomically in the same d1.batch() as the mutation, rather than plan 006's separate claim-then-mutate step. Plan 007 was unaffected (#355 guarded a different code path) and ships as #382. Plan 012's hard blocker dissolved: PR #380 implemented #344 and wrote docs/webhooks.md with the timestamped signature scheme in the same PR, so the docs were written against the correct format. Plan 012 is now UI-only. Three plans went stale within hours of being written, each differently: 008 was wrong on arrival, 006 was overtaken by unrelated work, 012 by its own dependency being solved better elsewhere. The drift check guards only the first, and only when the executor's baseline is the pinned commit. Also records that #370 and #380 both edit createConversation/deleteConversation and will conflict, plus a suggested merge order. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net> --------- Co-authored-by: duyetbot <bot@duyet.net>
1 parent 943f2d8 commit 370f2ac

15 files changed

Lines changed: 2842 additions & 0 deletions

‎plans/001-rate-limit-coordination-routes.md‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
# Plan 001: Rate-limit the states/leases/claims/MCP routers
2+
3+
> **Executor instructions**: Follow this plan step by step. Run every
4+
> verification command and confirm the expected result before moving to the
5+
> next step. If anything in the "STOP conditions" section occurs, stop and
6+
> report — do not improvise. When done, update the status row for this plan
7+
> in `plans/README.md`.
8+
>
9+
> **Drift check (run first)**: `git diff --stat ce9a1fa..HEAD -- packages/api/src/routes/states packages/api/src/routes/leases packages/api/src/routes/claims packages/api/src/routes/mcp packages/api/src/middleware/rate-limit.ts`
10+
> If any in-scope file changed since this plan was written, compare the
11+
> "Current state" excerpts against the live code before proceeding; on a
12+
> mismatch, treat it as a STOP condition.
13+
14+
## Status
15+
16+
- **Priority**: P1
17+
- **Effort**: S
18+
- **Risk**: LOW
19+
- **Depends on**: none
20+
- **Category**: security
21+
- **Planned at**: commit `ce9a1fa`, 2026-07-17
22+
23+
## Why this matters
24+
25+
Every conversation-facing router (`conversations`, `tags`, `keys`, `v1-keys`, `ai`, `webhooks`, `capability-tokens`, `analytics-public`) chains `rateLimitMiddleware`, but the write-heavy coordination surface — states, leases, claims, and the MCP endpoint — has **no rate limiting at all**. Each `PUT /api/v1/states/:key` inserts a `state_events` row and fans out a Durable Object notify, so one authenticated key (or a leaked capability token) can drive unlimited D1 writes and DO traffic: cost abuse and a noisy-neighbor path across the shared database.
26+
27+
## Current state
28+
29+
- `packages/api/src/middleware/rate-limit.ts` — the existing limiter. Default path is an atomic D1 fixed-window (100 req / 60 s per `apiKeyHash`); an opt-in KV sliding window sits behind `USE_SLIDING_WINDOW`. It only needs `apiKeyHash` in the Hono context, which the auth middlewares below already set.
30+
- `packages/api/src/routes/states/index.ts` — routes attach auth per-route, no limiter:
31+
```ts
32+
// states/index.ts:50
33+
router.get("/watch", scopedAuth({ scope: "state:watch", allowQueryToken: true }), async (c) => {
34+
// :101 router.post("/query", scopedAuth({ scope: "state:read" }), ...)
35+
// :125 router.post("/:state_key/lease", scopedAuth({ scope: "lease:write" }), ...)
36+
// :173 router.put("/:state_key", scopedAuth({ scope: "state:write" }), ...)
37+
// :240 router.delete("/:state_key", scopedAuth({ scope: "state:write" }), ...)
38+
```
39+
- `packages/api/src/routes/leases/index.ts:16,29` — `POST /:id/renew` and `DELETE /:id`, per-route `scopedAuth`, no limiter.
40+
- `packages/api/src/routes/claims/index.ts:86` — `router.use("*", scopedAuth({ scope: "claim:write" }))`, no limiter.
41+
- `packages/api/src/routes/mcp/index.ts:57` — `router.use("*", mcpAuth)`, no limiter.
42+
- Convention exemplar — `packages/api/src/routes/conversations/index.ts:16`:
43+
```ts
44+
import { rateLimitMiddleware } from "../../middleware/rate-limit";
45+
router.use("*", rateLimitMiddleware);
46+
```
47+
- `scopedAuth` (`packages/api/src/middleware/scoped-auth.ts`) sets `c.set("apiKeyHash", hash)` for both API keys and capability tokens; `mcpAuth` (`packages/api/src/middleware/mcp-auth.ts`) — verify it also sets `apiKeyHash` before relying on it (see STOP conditions).
48+
- Ordering constraint: the limiter keys on `apiKeyHash`, so it must run **after** auth. Because states/leases attach auth per-route (not `router.use`), a bare `router.use("*", rateLimitMiddleware)` at the top would run *before* auth with no `apiKeyHash`. Read how `rateLimitMiddleware` behaves when `apiKeyHash` is unset before choosing placement — the safe pattern is to add the limiter as a second per-route middleware after `scopedAuth(...)`, or convert the router to `router.use("*", scopedAuth-equivalent)` where all routes share a scope (claims already does this).
49+
50+
## Commands you will need
51+
52+
| Purpose | Command | Expected on success |
53+
|-----------|---------|---------------------|
54+
| Lint | `bunx biome check packages/api/src/` | exit 0 |
55+
| Typecheck | `bunx tsc --noEmit -p packages/api/tsconfig.json` | exit 0 |
56+
| Tests | `cd packages/api && bunx vitest run` | all pass |
57+
58+
## Scope
59+
60+
**In scope** (the only files you should modify):
61+
- `packages/api/src/routes/states/index.ts`
62+
- `packages/api/src/routes/leases/index.ts`
63+
- `packages/api/src/routes/claims/index.ts`
64+
- `packages/api/src/routes/mcp/index.ts`
65+
- New/extended tests under `packages/api/test/` (e.g. `rate-limit.test.ts` or a new `states-rate-limit.test.ts`)
66+
67+
**Out of scope** (do NOT touch):
68+
- `packages/api/src/middleware/rate-limit.ts` — do not change limits or algorithm; wiring only. (Exception: if a per-router limit override is truly required for MCP, STOP and report instead of refactoring the middleware.)
69+
- All other routers — they already have the limiter.
70+
- The SSE `/watch` route's streaming behavior: the limiter must count the connection request once, not per event. Do not wrap the stream.
71+
72+
## Git workflow
73+
74+
- Branch: `claude/improvement-<timestamp>` (repo convention, see CLAUDE.md)
75+
- Semantic commits, e.g. `fix(api): rate-limit states/leases/claims/mcp routers`
76+
- Include both co-author trailers from CLAUDE.md. Do NOT push or open a PR unless the operator instructed it.
77+
78+
## Steps
79+
80+
### Step 1: Wire the limiter into the states router
81+
82+
In `packages/api/src/routes/states/index.ts`, import `rateLimitMiddleware` and add it after `scopedAuth(...)` on every route (or as a `router.use` placed so it runs after auth — only if you verify auth runs first). Example target shape:
83+
84+
```ts
85+
router.put("/:state_key", scopedAuth({ scope: "state:write" }), rateLimitMiddleware, async (c) => {
86+
```
87+
88+
**Verify**: `bunx tsc --noEmit -p packages/api/tsconfig.json` → exit 0
89+
90+
### Step 2: Wire leases, claims, MCP
91+
92+
- `leases/index.ts`: same per-route pattern on both routes.
93+
- `claims/index.ts`: add `router.use("*", rateLimitMiddleware)` on the line **after** the existing `router.use("*", scopedAuth(...))` at :86.
94+
- `mcp/index.ts`: add `router.use("*", rateLimitMiddleware)` after `router.use("*", mcpAuth)` at :57 — only after confirming `mcpAuth` sets `apiKeyHash` (STOP if it doesn't).
95+
96+
**Verify**: `bunx tsc --noEmit -p packages/api/tsconfig.json` → exit 0
97+
98+
### Step 3: Add tests
99+
100+
Model after the existing rate-limit assertions in `packages/api/test/` (grep for `429` / `RATE_LIMIT` to find the pattern; `packages/api/test/conversations.test.ts` and any `rate-limit` suite are exemplars). Add, for at least the states PUT route and one MCP call: after exceeding the limit within a window, the route returns 429 with the standard error envelope `{ error: { code, message } }`.
101+
102+
**Verify**: `cd packages/api && bunx vitest run` → all pass, including new tests
103+
104+
## Test plan
105+
106+
- New: states PUT returns 429 past the limit; a request under the limit still succeeds; MCP endpoint returns 429 past the limit (or its JSON-RPC error equivalent — match how other middleware errors surface on the MCP route).
107+
- Existing suites must stay green — especially `states`, `leases`, `claims`, `mcp` test files, which will now pass through the limiter (if existing tests hammer an endpoint >100 times in one window, raise the per-test key variety rather than the limit).
108+
109+
## Done criteria
110+
111+
- [ ] `bunx biome check packages/api/src/` exits 0
112+
- [ ] `bunx tsc --noEmit -p packages/api/tsconfig.json` exits 0
113+
- [ ] `cd packages/api && bunx vitest run` exits 0, new 429 tests pass
114+
- [ ] `grep -L rateLimitMiddleware packages/api/src/routes/states/index.ts packages/api/src/routes/leases/index.ts packages/api/src/routes/claims/index.ts packages/api/src/routes/mcp/index.ts` prints nothing (all four files reference it)
115+
- [ ] No files outside the in-scope list modified (`git status`)
116+
- [ ] `plans/README.md` status row updated
117+
118+
## STOP conditions
119+
120+
- `mcpAuth` does not set `apiKeyHash` in context — the limiter would key on undefined; report instead of inventing a key.
121+
- `rateLimitMiddleware` errors or behaves unexpectedly when placed per-route rather than via `router.use` (e.g. relies on being registered once).
122+
- Existing test suites fail with 429s that can't be resolved by varying test keys — a design decision on limits is needed.
123+
- The SSE `/watch` route breaks (stream closed early / limiter counts events).
124+
125+
## Maintenance notes
126+
127+
- Any new router added under `/api/v1/*` must chain `rateLimitMiddleware` after its auth — reviewers should check this on every new-route PR.
128+
- Follow-up deliberately deferred: a separate (lower) limit bucket for MCP tool calls, and migrating the limiter to a Durable Object / Cloudflare rate-limiting binding (noted in the middleware's own comments).

‎plans/002-webhook-event-exact-match.md‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
# Plan 002: Match webhook events by exact array membership, not substring LIKE
2+
3+
> **Executor instructions**: Follow this plan step by step. Run every
4+
> verification command and confirm the expected result before moving on. If
5+
> anything in "STOP conditions" occurs, stop and report — do not improvise.
6+
> When done, update this plan's status row in `plans/README.md`.
7+
>
8+
> **Drift check (run first)**: `git diff --stat ce9a1fa..HEAD -- packages/api/src/services/webhooks.ts packages/api/test/webhooks.test.ts`
9+
> On any in-scope change, compare "Current state" excerpts to live code; on a
10+
> mismatch, STOP.
11+
12+
## Status
13+
14+
- **Priority**: P1
15+
- **Effort**: S
16+
- **Risk**: LOW
17+
- **Depends on**: none
18+
- **Category**: bug
19+
- **Planned at**: commit `ce9a1fa`, 2026-07-17
20+
21+
## Why this matters
22+
23+
`getActiveWebhooksForEvent` decides which webhooks receive an event by running a substring `LIKE` over the JSON-serialized events array. Substring matching cross-matches overlapping event names: an event `state.update` would match a webhook subscribed only to `["state.updated"]`, and a webhook subscribed to `["conversation.created"]` would match a hypothetical `conversation.create` query. Only one event type exists today (`conversation.created`), so the bug is latent — but it fires the moment a second overlapping event name ships, delivering webhooks to endpoints that never subscribed. Fix it now while it's cheap and testable.
24+
25+
## Current state
26+
27+
- `packages/api/src/services/webhooks.ts:219-244` — the matcher:
28+
```ts
29+
export async function getActiveWebhooksForEvent(
30+
db: DrizzleD1Database,
31+
projectId: string,
32+
event: string,
33+
): Promise<Array<{ id: string; url: string; secret: string }>> {
34+
const pattern = `%${escapeLikePattern(event)}%`;
35+
const rows = await db
36+
.select({ id: webhooks.id, url: webhooks.url, secret: webhooks.secret })
37+
.from(webhooks)
38+
.where(
39+
and(
40+
eq(webhooks.projectId, projectId),
41+
eq(webhooks.active, true),
42+
sql`json_extract(${webhooks.events}, '$') LIKE ${pattern}`,
43+
),
44+
);
45+
return rows;
46+
}
47+
```
48+
- `webhooks.events` is a TEXT column holding a JSON array (see `serializeEvents`/`parseEvents` in the same file, lines 48-63).
49+
- The DB is D1 (SQLite) — `json_each` is available.
50+
- Valid event names: `WEBHOOK_EVENTS = ["conversation.created"]` in `packages/api/src/lib/webhook.ts:7`.
51+
- Tests live in `packages/api/test/webhooks.test.ts` (currently covers URL safety + HMAC helpers).
52+
- Convention: Drizzle with raw `sql` fragments for JSON functions is already the local pattern (see the excerpt) — keep using bound parameters, never string interpolation of user input.
53+
54+
## Commands you will need
55+
56+
| Purpose | Command | Expected on success |
57+
|-----------|---------|---------------------|
58+
| Lint | `bunx biome check packages/api/src/` | exit 0 |
59+
| Typecheck | `bunx tsc --noEmit -p packages/api/tsconfig.json` | exit 0 |
60+
| Tests | `cd packages/api && bunx vitest run webhooks` | all pass |
61+
62+
## Scope
63+
64+
**In scope**:
65+
- `packages/api/src/services/webhooks.ts` (only `getActiveWebhooksForEvent`; the `escapeLikePattern` import becomes unused — remove the import if nothing else in the file uses it)
66+
- `packages/api/test/webhooks.test.ts` (add matcher tests)
67+
68+
**Out of scope**:
69+
- `packages/api/src/services/conversation-search.ts` — `escapeLikePattern`'s home; still used by search. Do not touch.
70+
- Webhook delivery/retry logic (`lib/webhook.ts`) — covered by plans 003/004.
71+
- The events column format — do not migrate the schema.
72+
73+
## Git workflow
74+
75+
- Branch: `claude/improvement-<timestamp>`; semantic commit, e.g. `fix(api): exact-match webhook event subscriptions via json_each`; include both co-author trailers from CLAUDE.md. No push/PR unless instructed.
76+
77+
## Steps
78+
79+
### Step 1: Replace the LIKE predicate with json_each membership
80+
81+
Target shape (bound parameter for the event string):
82+
83+
```ts
84+
sql`EXISTS (SELECT 1 FROM json_each(${webhooks.events}) WHERE json_each.value = ${event})`
85+
```
86+
87+
Remove the `pattern` variable; drop the now-unused `escapeLikePattern` import if unused elsewhere in the file.
88+
89+
**Verify**: `bunx tsc --noEmit -p packages/api/tsconfig.json` → exit 0
90+
91+
### Step 2: Add matcher tests
92+
93+
In `packages/api/test/webhooks.test.ts`, using the existing test setup pattern in that file (or `conversations.test.ts` if webhooks.test.ts lacks a DB harness), insert webhooks rows and assert:
94+
95+
1. Subscribed `["conversation.created"]` + event `conversation.created` → matched.
96+
2. Subscribed `["state.updated"]` + event `state.update` → **not** matched (the regression this plan fixes).
97+
3. Subscribed `["a.b","conversation.created"]` + event `conversation.created` → matched (multi-entry arrays).
98+
4. Inactive webhook with matching subscription → not matched.
99+
100+
**Verify**: `cd packages/api && bunx vitest run webhooks` → all pass, 4 new tests
101+
102+
## Test plan
103+
104+
As Step 2. Structural pattern: whichever existing suite in `packages/api/test/` seeds D1 rows directly (grep for `db.insert(webhooks)` or raw INSERT usage) — mirror it.
105+
106+
## Done criteria
107+
108+
- [ ] `bunx biome check packages/api/src/` exits 0
109+
- [ ] `bunx tsc --noEmit -p packages/api/tsconfig.json` exits 0
110+
- [ ] `cd packages/api && bunx vitest run` exits 0, incl. the substring-negative test
111+
- [ ] `grep -n "LIKE" packages/api/src/services/webhooks.ts` returns no matches
112+
- [ ] No files outside scope modified; `plans/README.md` row updated
113+
114+
## STOP conditions
115+
116+
- `json_each` with a Drizzle column reference fails to compile/execute against the vitest-pool-workers D1 — report the error rather than reverting to LIKE.
117+
- `escapeLikePattern` turns out to be used elsewhere in `webhooks.ts` beyond the matcher.
118+
- The webhooks test file has no DB harness and adding one would require touching shared test setup files not listed in scope.
119+
120+
## Maintenance notes
121+
122+
- When a second webhook event type is added (e.g. `state.updated`), add it to `WEBHOOK_EVENTS` in `lib/webhook.ts` and extend test case 2's overlap coverage.
123+
- Reviewer should confirm the event string is passed as a bound parameter, not interpolated.

0 commit comments

Comments
 (0)