From 499451b837a65e788fc2828cb33f5575e2d69409 Mon Sep 17 00:00:00 2001 From: hexcon Date: Mon, 6 Jul 2026 10:37:07 +0300 Subject: [PATCH 1/2] Ship a worked example session (001-api-rate-limiter) A complete six-phase session copied as-if from a project's artifacts folder: requirement, research with a recorded decision, a critiqued three-step plan, self-contained step files per references/step-format.md, and an execution log where every step closes on a named check. README links it from Quickstart and the repo tree. --- README.md | 7 +- .../001-api-rate-limiter/execution-log.md | 18 +++ examples/001-api-rate-limiter/plan.md | 44 +++++++ examples/001-api-rate-limiter/requirement.md | 68 +++++++++++ .../research/decisions.md | 17 +++ .../research/express-middleware-patterns.md | 39 ++++++ .../001-api-rate-limiter/research/notes.md | 11 ++ .../001-api-rate-limiter/research/summary.md | 15 +++ examples/001-api-rate-limiter/session.json | 49 ++++++++ .../001-api-rate-limiter/steps/step-001.md | 94 +++++++++++++++ .../001-api-rate-limiter/steps/step-002.md | 112 ++++++++++++++++++ .../001-api-rate-limiter/steps/step-003.md | 87 ++++++++++++++ examples/README.md | 34 ++++++ 13 files changed, 594 insertions(+), 1 deletion(-) create mode 100644 examples/001-api-rate-limiter/execution-log.md create mode 100644 examples/001-api-rate-limiter/plan.md create mode 100644 examples/001-api-rate-limiter/requirement.md create mode 100644 examples/001-api-rate-limiter/research/decisions.md create mode 100644 examples/001-api-rate-limiter/research/express-middleware-patterns.md create mode 100644 examples/001-api-rate-limiter/research/notes.md create mode 100644 examples/001-api-rate-limiter/research/summary.md create mode 100644 examples/001-api-rate-limiter/session.json create mode 100644 examples/001-api-rate-limiter/steps/step-001.md create mode 100644 examples/001-api-rate-limiter/steps/step-002.md create mode 100644 examples/001-api-rate-limiter/steps/step-003.md create mode 100644 examples/README.md diff --git a/README.md b/README.md index a4e0de5..24ccb2a 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,10 @@ Phase 6 Writes the docs you keep (optional), then cleans up the session The workflow stops at every phase boundary and waits for you. Say `status` to see where a session stands, `back` to revisit a phase, `pause` to save and exit, or `abort` to discard. +A complete finished session — every artifact from a real run, unedited — lives +in [`examples/001-api-rate-limiter`](examples/001-api-rate-limiter). Read it to +see what each phase produces before you run one. + ## How it works Six phases run in order. Each one produces artifacts, and each transition waits for your approval. @@ -140,6 +144,7 @@ The skill repository itself: ├── GUIDED-TOUR.md # Named overview that points to SKILL.md ├── phases/01-06 # Instructions per phase ├── templates/ # Output formats (requirement, plan, step, log) +├── examples/ # A complete worked session (001-api-rate-limiter) └── references/ ├── step-format.md # Self-containment specification ├── research-fanout.md # Subagent dispatch contract @@ -162,7 +167,7 @@ The skill repository itself: - [x] Self-contained step files and artifact memory - [x] Quality loops in research, planning, and execution - [ ] Commit a `LICENSE` file -- [ ] Ship a worked example session in the repo +- [x] Ship a worked example session in the repo - [ ] Optional hook-enforced checks for the execution verify loop ## Contributing diff --git a/examples/001-api-rate-limiter/execution-log.md b/examples/001-api-rate-limiter/execution-log.md new file mode 100644 index 0000000..68fe5e2 --- /dev/null +++ b/examples/001-api-rate-limiter/execution-log.md @@ -0,0 +1,18 @@ +# Execution Log + +`Verified` records the check that closed the step — the command run and its result, not "looks done." + +| Step | Status | Verified | Started | Completed | Notes | +|------|--------|----------|---------|-----------|-------| +| 001 | complete | `npx jest token-bucket` → 8 passed, 0 failed | 10:05 | 10:31 | Tests written first; initial run failed on missing module as expected | +| 002 | complete | `npx jest --detectOpenHandles` → 51 passed, no open handles | 10:33 | 11:02 | Two discoveries folded back (fake-timer options, `.unref()` on sweep) — see research/notes.md | +| 003 | complete | `npx jest` → 54 passed, 0 failed | 11:04 | 11:27 | Skipped RateLimit-Reset deliberately; noted as follow-up in docs | + +## Final Review + +Full diff reviewed in a fresh context against requirement.md (2 rounds max): + +- Round 1: one correctness gap — `RateLimit-Remaining` was set from + `floor(tokens)` *before* the decrement on the allow branch, off by one + versus the header contract. Fixed; `npx jest` → 54 passed. +- Round 2: no correctness or requirements gaps. Converged, review closed. diff --git a/examples/001-api-rate-limiter/plan.md b/examples/001-api-rate-limiter/plan.md new file mode 100644 index 0000000..4276601 --- /dev/null +++ b/examples/001-api-rate-limiter/plan.md @@ -0,0 +1,44 @@ +# Implementation Plan + +**Session**: 001-api-rate-limiter +**Granularity**: medium +**Total Steps**: 3 + +## Overview + +Build a token-bucket limiter as a standalone module, wrap it in an Express +middleware following the project's factory convention, then add the standard +`RateLimit-*` response headers. Each step lands with its own tests; the bucket +logic is testable without HTTP. + +## Prerequisites + +- [ ] `npx jest` green on main before starting (confirmed: 42 passed) +- [ ] Node 20 toolchain available + +## Steps + +| # | Title | Dependencies | Scope | +|---|-------|--------------|-------| +| 001 | Token-bucket module | - | `src/lib/token-bucket.js`, `tests/token-bucket.test.js` | +| 002 | Rate-limit middleware, wired into the app | 001 | `src/middleware/rate-limit.js`, `src/app.js`, `tests/rate-limit.test.js` | +| 003 | Standard RateLimit headers and Retry-After | 002 | `src/middleware/rate-limit.js`, `tests/rate-limit.test.js` | + +## Dependency Graph + +``` +001 ─► 002 ─► 003 +``` + +## Rollback Strategy + +Each step is one commit on branch `feat/rate-limiter`; roll back a step with +`git revert ` or drop the branch entirely. No migrations, no config +changes outside the repo, so reverting the code fully reverts the feature. + +## Critique + +Reviewed for blocking gaps before execution: 1 gap found and fixed +- Gap: plan tested refill timing against wall-clock `Date.now()`, which makes + the bucket untestable with fake timers → fix: the bucket takes an injectable + `now` function (defaults to `Date.now`), steps 001-002 updated. diff --git a/examples/001-api-rate-limiter/requirement.md b/examples/001-api-rate-limiter/requirement.md new file mode 100644 index 0000000..220fdbd --- /dev/null +++ b/examples/001-api-rate-limiter/requirement.md @@ -0,0 +1,68 @@ +# API Rate Limiter + +## Problem Statement + +The orders-api has no request throttling. One misbehaving client script +recently sent 40 requests per second for an hour and degraded response times +for everyone. We need a per-client rate limit that rejects excess requests +early, before they reach the database. + +## Scope + +### In Scope +- Token-bucket limiter applied to all `/api/*` routes +- Client identity: `X-API-Key` header when present, source IP otherwise +- `429 Too Many Requests` with standard `RateLimit-*` headers and `Retry-After` +- Unit tests for the bucket, integration tests through the HTTP layer + +### Out of Scope +- Distributed rate limiting (Redis or similar) — the API runs as a single instance +- Per-route or per-plan limit tiers — one global limit for now +- Client dashboards or usage reporting +- Changes to authentication + +## Success Criteria + +- [ ] A client staying under 60 requests/minute is never throttled +- [ ] A burst above the bucket capacity gets `429` with `Retry-After` +- [ ] Every response on `/api/*` carries `RateLimit-Limit` and `RateLimit-Remaining` +- [ ] `npx jest` passes with the new tests included +- [ ] Existing endpoints behave unchanged below the limit + +## Context + +### Relevant Files +- `src/app.js` - Express app assembly; middleware order lives here +- `src/routes/orders.js` - the main API surface being protected +- `tests/orders.test.js` - existing supertest patterns to follow + +### Related Systems +- None. The limiter is in-process; no external store. + +## Constraints + +- Node 20, Express 4, Jest + supertest already in the project — no new runtime dependencies +- Must not add measurable latency to requests under the limit +- Memory bounded: idle client buckets must be evicted + +## Q&A Transcript + +
+Q&A + +**Q:** What are we building or fixing, and why? +**A:** Rate limiting for orders-api; a runaway client degraded the service last week. + +**Q:** Limit per what — API key, IP, or both? +**A:** API key when the header is present, otherwise IP. + +**Q:** What limit and burst? +**A:** 60 requests/minute sustained, bursts up to 60 allowed (bucket capacity 60, refill 1/sec). + +**Q:** External store acceptable? +**A:** No, keep it in-process; we run one instance. + +**Q:** Standard headers? +**A:** Yes, RateLimit-Limit/Remaining/Reset plus Retry-After on 429. + +
diff --git a/examples/001-api-rate-limiter/research/decisions.md b/examples/001-api-rate-limiter/research/decisions.md new file mode 100644 index 0000000..d4d8b6f --- /dev/null +++ b/examples/001-api-rate-limiter/research/decisions.md @@ -0,0 +1,17 @@ +# Decisions + +## Decision: Limiter algorithm and storage + +**Context**: Need per-client throttling with burst tolerance, single-instance API, no new runtime dependencies allowed. +**Options**: A: token bucket in an in-process Map | B: sliding-window counter in Redis | C: fixed-window counter in-process +**Chosen**: A +**Rationale**: Token bucket gives burst-then-sustain semantics that match the stated limit (burst 60, refill 1/sec). Redis is out of scope (single instance, no new dependencies). Fixed windows allow 2x bursts at window edges. + +--- + +## Decision: Idle bucket eviction + +**Context**: An in-process Map grows with every distinct client key; memory must stay bounded. +**Options**: A: sweep on a timer every 10 minutes | B: evict lazily on access when a bucket is full and stale +**Chosen**: A +**Rationale**: Lazy eviction never removes buckets for clients that stop calling. A 10-minute sweep dropping buckets idle for more than 2 minutes keeps the Map proportional to active clients; the sweep is O(entries) over a small set. diff --git a/examples/001-api-rate-limiter/research/express-middleware-patterns.md b/examples/001-api-rate-limiter/research/express-middleware-patterns.md new file mode 100644 index 0000000..9a63dc8 --- /dev/null +++ b/examples/001-api-rate-limiter/research/express-middleware-patterns.md @@ -0,0 +1,39 @@ +# Express Middleware Patterns in orders-api + +**Researched**: 2026-07-05T09:41:00Z +**Source**: codebase + +## Question + +Where does middleware mount in this app, and what conventions do existing +middleware and tests follow? + +## Key Findings + +- `src/app.js` mounts middleware in a fixed order: `express.json()`, then + `requestId`, then the routers under `/api`. A limiter must mount after + `requestId` (so 429s carry a request id) and before the routers. +- Existing middleware lives one file per concern in `src/middleware/` + (`request-id.js`, `error-handler.js`), exported as a factory function + taking an options object: `module.exports = (options = {}) => (req, res, next) => {...}`. +- Tests build the app per suite via `require('../src/app')` and drive it with + supertest; no server listens during tests. Time-sensitive tests use + `jest.useFakeTimers({ doNotFake: ['nextTick'] })` in `tests/orders.test.js:12`. +- Client IP: `app.set('trust proxy', 1)` is already configured, so + `req.ip` is the real client behind the reverse proxy. + +## Sources + +- `src/app.js:14-22` - middleware mount order +- `src/middleware/request-id.js:1` - factory-function convention +- `tests/orders.test.js:8-16` - supertest + fake-timers pattern + +## Relevance + +The limiter becomes `src/middleware/rate-limit.js`, a factory following the +existing convention, mounted between `requestId` and the routers. Tests can +control refill timing with the fake-timers pattern already in use. + +## Open Questions + +- None blocking. Eviction cadence for idle buckets decided in decisions.md. diff --git a/examples/001-api-rate-limiter/research/notes.md b/examples/001-api-rate-limiter/research/notes.md new file mode 100644 index 0000000..a2b2d5a --- /dev/null +++ b/examples/001-api-rate-limiter/research/notes.md @@ -0,0 +1,11 @@ +# Notes + +Running discoveries during execution, folded into the step files they affect. + +- 2026-07-05 10:22 — `jest.useFakeTimers()` without options broke supertest + (requests never resolved because `setImmediate` was faked). The working call + is `jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] })`. + Folded into step-001 and step-002 Validation sections. +- 2026-07-05 10:54 — The eviction sweep timer kept the Jest process alive + after tests. Fixed by calling `.unref()` on the interval; folded into + step-002 Instructions. diff --git a/examples/001-api-rate-limiter/research/summary.md b/examples/001-api-rate-limiter/research/summary.md new file mode 100644 index 0000000..d745172 --- /dev/null +++ b/examples/001-api-rate-limiter/research/summary.md @@ -0,0 +1,15 @@ +# Research Summary + +## Key Findings +- Middleware convention: factory functions in `src/middleware/`, mounted in `src/app.js` between `requestId` and the `/api` routers +- Tests use supertest against the exported app plus `jest.useFakeTimers` for time control — the limiter's refill logic can be tested the same way +- `trust proxy` is already set, so `req.ip` is safe as the fallback client key + +## Decisions Made +| Decision | Choice | Rationale | +|----------|--------|-----------| +| Algorithm and storage | Token bucket, in-process Map | Burst-then-sustain matches the requirement; no new dependencies | +| Idle bucket eviction | Timed sweep every 10 min | Bounded memory even for clients that vanish | + +## Open Questions +- None blocking. diff --git a/examples/001-api-rate-limiter/session.json b/examples/001-api-rate-limiter/session.json new file mode 100644 index 0000000..efd17da --- /dev/null +++ b/examples/001-api-rate-limiter/session.json @@ -0,0 +1,49 @@ +{ + "slug": "001-api-rate-limiter", + "phase": "wrapup", + "status": "complete", + "created": "2026-07-05T09:12:04Z", + "updated": "2026-07-05T11:48:31Z", + "requirement": { + "summary": "Per-client token-bucket rate limiting for the orders-api, keyed by API key with IP fallback, returning 429 with standard RateLimit headers.", + "minimal": false + }, + "research": { + "findings_count": 1, + "decisions_count": 1, + "last_topic": "express-middleware-patterns" + }, + "planning": { + "total_steps": 3, + "steps_generated": 3, + "granularity": "medium" + }, + "execution": { + "current_step": 3, + "completed_steps": [1, 2, 3], + "step_results": { + "1": { + "status": "complete", + "notes": "token-bucket module, 8 tests green", + "started": "2026-07-05T10:05:12Z", + "completed": "2026-07-05T10:31:40Z" + }, + "2": { + "status": "complete", + "notes": "middleware wired before routes, 5 tests green", + "started": "2026-07-05T10:33:02Z", + "completed": "2026-07-05T11:02:19Z" + }, + "3": { + "status": "complete", + "notes": "RateLimit headers + Retry-After, full suite green", + "started": "2026-07-05T11:04:55Z", + "completed": "2026-07-05T11:27:08Z" + } + } + }, + "documentation": { + "type": "feature", + "path": "docs/features/rate-limiting/README.md" + } +} diff --git a/examples/001-api-rate-limiter/steps/step-001.md b/examples/001-api-rate-limiter/steps/step-001.md new file mode 100644 index 0000000..a8ad59c --- /dev/null +++ b/examples/001-api-rate-limiter/steps/step-001.md @@ -0,0 +1,94 @@ +# Step 001: Token-bucket module + +**Session**: 001-api-rate-limiter +**Step**: 1 of 3 +**Dependencies**: none +**Scope**: create `src/lib/token-bucket.js`, create `tests/token-bucket.test.js` + +## Context + +### Requirement Summary +The orders-api needs per-client rate limiting: 60 requests/minute sustained +with bursts up to 60, keyed by API key or IP. Excess requests get `429`. This +step builds only the bucket logic — no HTTP, no Express. + +### Relevant Decisions +- **Algorithm and storage**: Token bucket in an in-process Map — burst-then-sustain + semantics match the requirement (capacity 60, refill 1 token/sec); no new + dependencies allowed. +- **Testability (from plan critique)**: The bucket takes an injectable `now` + function defaulting to `Date.now`, so tests control time without fake timers. + +### Prior Step Outputs +- None. This is the first step. + +### Files to Read +- `tests/orders.test.js` - Jest conventions in this project (describe/it naming, no snapshots) + +### Patterns to Follow +```js +// From src/lib/request-id.js — modules in src/lib export plain functions or +// classes via module.exports, no default-export interop: +module.exports = { generateRequestId }; +``` + +## Task + +### Objective +Implement a token bucket with take/refill semantics and an idle-entry sweep, +driven by an injectable clock. + +### Instructions +1. Create `src/lib/token-bucket.js` exporting a class `TokenBucketMap`: + - `constructor({ capacity = 60, refillPerSecond = 1, now = Date.now } = {})` + - `take(key)` → `{ allowed, remaining, retryAfterSeconds }`. Lazily creates + a bucket per key at full capacity. Refill is continuous: + `tokens = min(capacity, tokens + elapsedSeconds * refillPerSecond)`, + computed from the stored `lastRefill` timestamp on each call. + - When `tokens >= 1`: decrement, `allowed: true`, `remaining: floor(tokens)`. + - When `tokens < 1`: `allowed: false`, `remaining: 0`, + `retryAfterSeconds: ceil((1 - tokens) / refillPerSecond)`. + - `sweep(maxIdleMs = 120000)` → deletes entries whose `lastRefill` is older + than `maxIdleMs`; returns the number evicted. +2. Create `tests/token-bucket.test.js` with a manual fake clock + (`let t = 0; const now = () => t;`) covering: + - a fresh key allows `capacity` consecutive takes, then denies + - a denied take reports `retryAfterSeconds: 1` at refill 1/sec + - advancing the clock 1000ms allows exactly one more take + - tokens never exceed capacity after a long idle period + - `sweep` evicts only entries idle past the threshold + - two keys do not share tokens +3. Write the tests first and run them against an empty module to confirm they + fail for the right reason (missing implementation, not typos), then implement. + +### Expected Output +- `src/lib/token-bucket.js` - the bucket, no Express imports +- `tests/token-bucket.test.js` - 6-8 focused unit tests + +## Validation + +### Commands +```bash +npx jest token-bucket --verbose +npx jest # full suite still green +``` + +### Expected Results +- All new tests pass; suite total rises from 42 passed, 0 failures +- No open handles warning (the module itself starts no timers) + +### Manual Checks +- [ ] `take` never returns negative `remaining` + +## Acceptance Criteria + +- [ ] `npx jest token-bucket` exits 0 with all listed cases covered +- [ ] Full `npx jest` run stays green +- [ ] `src/lib/token-bucket.js` imports nothing from Express or `src/middleware` + +## Rollback + +```bash +git checkout -- src/lib/token-bucket.js tests/token-bucket.test.js 2>/dev/null || \ + rm -f src/lib/token-bucket.js tests/token-bucket.test.js +``` diff --git a/examples/001-api-rate-limiter/steps/step-002.md b/examples/001-api-rate-limiter/steps/step-002.md new file mode 100644 index 0000000..a39243c --- /dev/null +++ b/examples/001-api-rate-limiter/steps/step-002.md @@ -0,0 +1,112 @@ +# Step 002: Rate-limit middleware, wired into the app + +**Session**: 001-api-rate-limiter +**Step**: 2 of 3 +**Dependencies**: 001 +**Scope**: create `src/middleware/rate-limit.js`, create `tests/rate-limit.test.js`, modify `src/app.js` + +## Context + +### Requirement Summary +The orders-api needs per-client rate limiting: 60 requests/minute sustained +with bursts up to 60. Client identity is the `X-API-Key` header when present, +source IP otherwise. Requests over the limit get `429`. This step turns the +step-001 bucket into mounted Express middleware; response headers come in +step 003. + +### Relevant Decisions +- **Algorithm and storage**: Token bucket in an in-process `TokenBucketMap` + (built in step 001) — no external store, single-instance API. +- **Idle bucket eviction**: A sweep every 10 minutes evicts buckets idle for + more than 2 minutes, keeping memory proportional to active clients. + +### Prior Step Outputs +- Step 001 created: `src/lib/token-bucket.js` — `TokenBucketMap` with + `take(key) → { allowed, remaining, retryAfterSeconds }` and `sweep(maxIdleMs)`. + +### Files to Read +- `src/app.js` - current middleware order: `express.json()` → `requestId` → routers +- `src/middleware/request-id.js` - the factory convention to copy + +### Patterns to Follow +```js +// From src/middleware/request-id.js — middleware are factories taking options: +module.exports = (options = {}) => { + return (req, res, next) => { + // ... + next(); + }; +}; +``` + +```js +// From tests/orders.test.js:8-16 — supertest against the exported app, +// fake timers scoped so supertest still resolves: +jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }); +const request = require('supertest'); +const app = require('../src/app'); +``` + +## Task + +### Objective +Mount a rate-limit middleware on `/api` that rejects over-limit clients with +`429`, keyed by API key with IP fallback. + +### Instructions +1. Create `src/middleware/rate-limit.js` as a factory: + `module.exports = ({ capacity = 60, refillPerSecond = 1, sweepIntervalMs = 600000, now } = {}) => ...` + - Instantiate one `TokenBucketMap` per factory call (not per request). + - Client key: `req.get('x-api-key') || req.ip`. `trust proxy` is already + set in `src/app.js`, so `req.ip` is the real client address. + - On `take(key).allowed === false`: respond + `res.status(429).json({ error: 'rate_limited' })` and do not call `next()`. + - Start the eviction sweep with + `setInterval(() => buckets.sweep(), sweepIntervalMs).unref()` — the + `.unref()` matters: without it the interval keeps Jest (and any short-lived + process) alive after the suite finishes. +2. Mount it in `src/app.js` between `requestId` and the routers: + ```js + const rateLimit = require('./middleware/rate-limit'); + app.use('/api', rateLimit()); + ``` +3. Create `tests/rate-limit.test.js` (write the failing tests first, confirm + they fail because the middleware is absent, then implement): + - 60 requests with one API key all return 200; the 61st returns 429 + - a second API key still gets 200 while the first is throttled + - no API key → keyed by IP: the 61st anonymous request returns 429 + - a throttled key gets 200 again after advancing fake timers 1000ms +4. Build the middleware with an injectable `now` passed through to + `TokenBucketMap`, matching the step-001 design, so tests drive time. + +### Expected Output +- `src/middleware/rate-limit.js` - factory following the project convention +- `src/app.js` - one new `app.use('/api', rateLimit())` line, order preserved +- `tests/rate-limit.test.js` - 4-5 integration tests through supertest + +## Validation + +### Commands +```bash +npx jest rate-limit --verbose +npx jest --detectOpenHandles # full suite; confirms the .unref() sweep leaks nothing +``` + +### Expected Results +- New tests pass; full suite green; no open-handles report + +### Manual Checks +- [ ] `curl -i localhost:3000/api/orders` (dev server) shows 200 and normal body under the limit + +## Acceptance Criteria + +- [ ] `npx jest rate-limit` exits 0 covering the four listed cases +- [ ] Full `npx jest --detectOpenHandles` run is green with no leaked handles +- [ ] `src/app.js` mounts the limiter after `requestId`, before the routers + +## Rollback + +```bash +git checkout -- src/app.js +rm -f src/middleware/rate-limit.js tests/rate-limit.test.js +``` diff --git a/examples/001-api-rate-limiter/steps/step-003.md b/examples/001-api-rate-limiter/steps/step-003.md new file mode 100644 index 0000000..623db94 --- /dev/null +++ b/examples/001-api-rate-limiter/steps/step-003.md @@ -0,0 +1,87 @@ +# Step 003: Standard RateLimit headers and Retry-After + +**Session**: 001-api-rate-limiter +**Step**: 3 of 3 +**Dependencies**: 002 +**Scope**: modify `src/middleware/rate-limit.js`, modify `tests/rate-limit.test.js` + +## Context + +### Requirement Summary +The orders-api rate limiter (built in steps 001-002) must expose its state to +clients: every `/api/*` response carries `RateLimit-Limit` and +`RateLimit-Remaining`, and a `429` additionally carries `Retry-After` so +well-behaved clients can back off instead of retrying blind. + +### Relevant Decisions +- **Algorithm and storage**: Token bucket (`TokenBucketMap`, step 001); + `take(key)` already returns `remaining` and `retryAfterSeconds` — this step + only surfaces them as headers, no bucket changes. + +### Prior Step Outputs +- Step 001 created: `src/lib/token-bucket.js` — `take(key) → { allowed, remaining, retryAfterSeconds }` +- Step 002 created: `src/middleware/rate-limit.js` mounted on `/api` in + `src/app.js`; `tests/rate-limit.test.js` with 4 passing integration tests + +### Files to Read +- `src/middleware/rate-limit.js` - the `take()` call site where headers attach + +### Patterns to Follow +```js +// Headers set before either branch responds, so 200s and 429s both carry them: +const result = buckets.take(key); +res.set('RateLimit-Limit', String(capacity)); +res.set('RateLimit-Remaining', String(result.remaining)); +``` + +## Task + +### Objective +Attach `RateLimit-Limit`/`RateLimit-Remaining` to every limited response and +`Retry-After` to 429s. + +### Instructions +1. In `src/middleware/rate-limit.js`, after `take(key)`: + - always set `RateLimit-Limit: ` and + `RateLimit-Remaining: ` + - on the deny branch, also set `Retry-After: ` + (integer seconds, per RFC 9110) before sending the 429 body +2. Extend `tests/rate-limit.test.js` (failing first, then implement): + - a 200 response carries `RateLimit-Limit: 60` and a numeric + `RateLimit-Remaining` + - `RateLimit-Remaining` decreases by 1 across two consecutive requests + - a 429 response carries `Retry-After: 1` at refill 1/sec +3. Do not add `RateLimit-Reset`: the requirement's success criteria name only + Limit/Remaining plus Retry-After, and the draft-standard Reset semantics + (window vs delay) invite client confusion. Note it as a possible follow-up + in the wrap-up docs instead. + +### Expected Output +- `src/middleware/rate-limit.js` - header lines added, logic otherwise unchanged +- `tests/rate-limit.test.js` - 3 new assertions/tests + +## Validation + +### Commands +```bash +npx jest rate-limit --verbose +npx jest # full suite +``` + +### Expected Results +- All rate-limit tests green, including the three new header cases +- Full suite green + +### Manual Checks +- [ ] `curl -i` against the dev server shows both headers on a normal 200 + +## Acceptance Criteria + +- [ ] `npx jest` exits 0 with the header assertions included +- [ ] A throttled response includes all three headers: Limit, Remaining (0), Retry-After + +## Rollback + +```bash +git checkout -- src/middleware/rate-limit.js tests/rate-limit.test.js +``` diff --git a/examples/README.md b/examples/README.md new file mode 100644 index 0000000..e8d204b --- /dev/null +++ b/examples/README.md @@ -0,0 +1,34 @@ +# Examples + +A complete, finished Guided-Tour session, copied verbatim from a project's +`.claude/workflows/guided-tour/artifacts/` folder after Phase 6 chose "Keep". +Use it to see what the workflow produces before you run it, and as a reference +for the shape each artifact should take. + +## 001-api-rate-limiter + +Adds a token-bucket rate limiter to a small Express API (`orders-api`). The +session ran all six phases: requirements in two rounds, research with one +recorded decision, a three-step plan that survived one critique pass, execution +with a test-first loop per step, and a wrap-up that kept the artifacts. + +Read it in the order the workflow wrote it: + +| Order | File | What to look at | +|-------|------|-----------------| +| 1 | `session.json` | Final state: phase `wrapup`, status `complete` | +| 2 | `requirement.md` | Scope kept small; success criteria are checkable | +| 3 | `research/` | One topic file, one decision, a summary, and `notes.md` from execution | +| 4 | `plan.md` | Step table, dependency graph, and the critique result | +| 5 | `steps/step-001..003.md` | Self-containment: each file repeats the context it needs | +| 6 | `execution-log.md` | Every step closed by a named check, not an assertion | + +The step files are the most useful part. Each one copies the requirement +summary and relevant decisions inline, names exact paths, and shows the +patterns to follow — so a fresh agent with no history can execute the step +from the file alone. That is the standard `references/step-format.md` sets; +these files show what it looks like in practice. + +The target codebase (`orders-api`) is fictional but consistent across all +artifacts: an Express app with `src/app.js`, an orders router, and a +Jest + supertest test suite. From e995f5e10ae732f79541e16e68e125968d87103b Mon Sep 17 00:00:00 2001 From: hexcon Date: Mon, 6 Jul 2026 10:40:55 +0300 Subject: [PATCH 2/2] Add optional hook-enforced checks for the execution verify loop A PreToolUse guard (hooks/check_step_verified.py) blocks marking a step complete in execution-log.md unless a .verified/step-NNN marker exists in the session folder. The marker is created by chaining it onto the step's real Validation command, so it exists only when the check exited 0. Fails open on anything it does not understand. Verified with six stdin cases (block/allow/other-file/in-progress/ malformed) against python3. --- README.md | 3 +- SKILL.md | 1 + hooks/README.md | 98 ++++++++++++++++++++++++++++++++++++ hooks/check_step_verified.py | 81 +++++++++++++++++++++++++++++ phases/05-execution.md | 16 ++++++ 5 files changed, 198 insertions(+), 1 deletion(-) create mode 100644 hooks/README.md create mode 100644 hooks/check_step_verified.py diff --git a/README.md b/README.md index a4e0de5..0c0f522 100644 --- a/README.md +++ b/README.md @@ -140,6 +140,7 @@ The skill repository itself: ├── GUIDED-TOUR.md # Named overview that points to SKILL.md ├── phases/01-06 # Instructions per phase ├── templates/ # Output formats (requirement, plan, step, log) +├── hooks/ # Optional harness-enforced verify checks └── references/ ├── step-format.md # Self-containment specification ├── research-fanout.md # Subagent dispatch contract @@ -163,7 +164,7 @@ The skill repository itself: - [x] Quality loops in research, planning, and execution - [ ] Commit a `LICENSE` file - [ ] Ship a worked example session in the repo -- [ ] Optional hook-enforced checks for the execution verify loop +- [x] Optional hook-enforced checks for the execution verify loop ## Contributing diff --git a/SKILL.md b/SKILL.md index 0182127..73f4695 100644 --- a/SKILL.md +++ b/SKILL.md @@ -113,3 +113,4 @@ When in doubt, ask the user: *"This looks non-trivial — want me to run Guided- - `phases/01-06` — Phase instructions - `templates/` — Output formats - `references/` — Step format specification, session schema +- `hooks/` — Optional PreToolUse guard enforcing the evidence rule (see `hooks/README.md`) diff --git a/hooks/README.md b/hooks/README.md new file mode 100644 index 0000000..dee7aa0 --- /dev/null +++ b/hooks/README.md @@ -0,0 +1,98 @@ +# Hook-enforced checks (optional) + +The execution phase already tells the agent to close a step only on a real, +observed check. That rule lives in prose, so a rushed agent can still write +`complete` into `execution-log.md` on an assertion. This optional hook moves +the rule from prose into the harness: the edit is blocked unless evidence +exists on disk. + +## How it works + +1. `check_step_verified.py` runs as a Claude Code **PreToolUse** hook on every + Edit/Write call. +2. It ignores everything except writes to a file named `execution-log.md`. +3. When the new content marks step `NNN` as `complete`, the hook requires + `.verified/step-NNN` in the session folder (next to the log). Missing + marker → the tool call is blocked (exit 2) and the agent is told to run the + step's Validation command first. +4. The marker is created by chaining it onto the step's real check, so it can + only exist when that check exited 0: + + ```bash + npx jest rate-limit && mkdir -p .verified && touch .verified/step-002 + ``` + + (Paths relative to the session folder; use the session's absolute path when + running from the project root.) + +The hook fails open: malformed input, other files, or log edits that mark +nothing `complete` all pass through. It can block exactly one thing — claiming +completion without a marker — and nothing else. + +## Install + +Per project. Copy the script in and register it: + +```bash +mkdir -p .claude/hooks +cp ~/.claude/skills/guided-tour/hooks/check_step_verified.py .claude/hooks/ +``` + +Add to `.claude/settings.json` (project) or `.claude/settings.local.json`: + +```json +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Edit|Write", + "hooks": [ + { + "type": "command", + "command": "python3 \"$CLAUDE_PROJECT_DIR/.claude/hooks/check_step_verified.py\"", + "timeout": 10 + } + ] + } + ] + } +} +``` + +On Windows, hooks run through `cmd`, so use `python` and `%CLAUDE_PROJECT_DIR%`: + +```json +"command": "python \"%CLAUDE_PROJECT_DIR%\\.claude\\hooks\\check_step_verified.py\"" +``` + +Requires Python 3 on PATH; the script uses only the standard library. On +Windows, `python` may be a Microsoft Store stub that opens the store instead +of running anything — install Python from python.org (or use the `py` +launcher) and verify `python --version` prints a version before enabling the +hook. + +## Test it + +From a project with the hook installed and a session folder in place: + +```bash +# No marker: this hook invocation must exit 2 +echo '{"tool_name":"Edit","tool_input":{"file_path":"/abs/path/to/session/execution-log.md","new_string":"| 001 | complete | test | 10:00 | 10:05 | ok |"}}' \ + | python3 .claude/hooks/check_step_verified.py; echo "exit: $?" + +# With marker: must exit 0 +mkdir -p /abs/path/to/session/.verified && touch /abs/path/to/session/.verified/step-001 +echo '{"tool_name":"Edit","tool_input":{"file_path":"/abs/path/to/session/execution-log.md","new_string":"| 001 | complete | test | 10:00 | 10:05 | ok |"}}' \ + | python3 .claude/hooks/check_step_verified.py; echo "exit: $?" +``` + +## Honest limits + +The hook checks that a marker exists, not that the check truly ran — an agent +could `touch` the marker directly. Phase 5 forbids that, and the marker's +mtime leaves an audit trail, but this is friction plus evidence, not proof. +Keep the final review loop; the hook narrows the gap between "said done" and +"is done", it does not close it alone. + +Markers live inside the session folder, so Phase 6 cleanup removes them with +everything else. diff --git a/hooks/check_step_verified.py b/hooks/check_step_verified.py new file mode 100644 index 0000000..36e1107 --- /dev/null +++ b/hooks/check_step_verified.py @@ -0,0 +1,81 @@ +#!/usr/bin/env python3 +"""PreToolUse guard: block marking a step complete without a verify marker. + +Enforces the Loop Discipline evidence rule mechanically. When the agent edits +an execution-log.md to mark a step ``complete``, this hook requires a marker +file ``.verified/step-{NNN}`` next to the log. The marker is created by +chaining it onto the step's real Validation command, so it exists only when +that command has exited 0: + + npx jest rate-limit && mkdir -p .verified && touch .verified/step-002 + +Fail-open by design: anything unexpected (malformed input, a different file, +no ``complete`` rows in the new content) allows the tool call. The hook only +blocks the one action it understands — writing ``complete`` for a step whose +marker is missing — so it can never wedge unrelated work. + +Install and settings snippet: see hooks/README.md. +""" +import json +import os +import re +import sys + + +def main(): + try: + payload = json.load(sys.stdin) + except Exception: + return 0 # malformed input: never block on our own bug + + tool_input = payload.get("tool_input") or {} + file_path = tool_input.get("file_path") or "" + if os.path.basename(file_path) != "execution-log.md": + return 0 + + # Write sends the whole file as `content`; Edit sends the replacement + # string as `new_string` (older builds: `new_str`). Check all shapes. + new_text = ( + tool_input.get("content") + or tool_input.get("new_string") + or tool_input.get("new_str") + or "" + ) + + # Table rows marking a step complete: | 002 | complete | ... + steps = re.findall(r"\|\s*(\d{3})\s*\|\s*complete\b", new_text) + if not steps: + return 0 + + session_dir = os.path.dirname(os.path.abspath(file_path)) + missing = sorted( + { + step + for step in steps + if not os.path.exists( + os.path.join(session_dir, ".verified", "step-" + step) + ) + } + ) + if not missing: + return 0 + + marker_dir = os.path.join(session_dir, ".verified") + sys.stderr.write( + "Blocked: step(s) {steps} marked complete without a verify marker.\n" + "Run the step's Validation command and let success create the marker, " + "for example:\n" + " && mkdir -p \"{marker_dir}\" && " + "touch \"{marker_dir}/step-{first}\"\n" + "then retry this edit. Do not create the marker without running the " + "check; that defeats the audit trail.\n".format( + steps=", ".join(missing), + marker_dir=marker_dir, + first=missing[0], + ) + ) + return 2 # exit 2 blocks the tool call; stderr is fed back to the agent + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/phases/05-execution.md b/phases/05-execution.md index c0b3ee6..2fae655 100644 --- a/phases/05-execution.md +++ b/phases/05-execution.md @@ -31,6 +31,22 @@ Each step runs as a verify loop, not a single pass. The step is done when its ch 8. Present checkpoint options. 9. Repeat until every step's check has passed. +## Optional Hook Enforcement + +Projects can install the guard hook from the skill's `hooks/` directory (see +`hooks/README.md`). With it enabled, the harness blocks any edit that marks a +step `complete` in `execution-log.md` unless `.verified/step-{NNN}` exists in +the session folder. Create the marker only by chaining it onto the step's real +check, so it exists only when the check exited 0: + +```bash + && mkdir -p {session}/.verified && touch {session}/.verified/step-{NNN} +``` + +Never touch a marker without running the check — that defeats the audit trail +the hook exists to keep. Without the hook installed, nothing changes; the +evidence rule still applies, enforced by discipline instead of the harness. + ## Execution Log Update after each step: