Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -140,6 +144,8 @@ 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)
├── hooks/ # Optional harness-enforced verify checks
└── references/
├── step-format.md # Self-containment specification
├── research-fanout.md # Subagent dispatch contract
Expand All @@ -161,9 +167,9 @@ The skill repository itself:
- [x] Six-phase workflow with human-gated checkpoints (documentation and cleanup merged into one wrap-up phase)
- [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
- [ ] Optional hook-enforced checks for the execution verify loop
- [x] Commit a `LICENSE` file
- [x] Ship a worked example session in the repo
- [x] Optional hook-enforced checks for the execution verify loop

## Contributing

Expand Down
1 change: 1 addition & 0 deletions SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
18 changes: 18 additions & 0 deletions examples/001-api-rate-limiter/execution-log.md
Original file line number Diff line number Diff line change
@@ -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.
44 changes: 44 additions & 0 deletions examples/001-api-rate-limiter/plan.md
Original file line number Diff line number Diff line change
@@ -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 <sha>` 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.
68 changes: 68 additions & 0 deletions examples/001-api-rate-limiter/requirement.md
Original file line number Diff line number Diff line change
@@ -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

<details>
<summary>Q&A</summary>

**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.

</details>
17 changes: 17 additions & 0 deletions examples/001-api-rate-limiter/research/decisions.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
11 changes: 11 additions & 0 deletions examples/001-api-rate-limiter/research/notes.md
Original file line number Diff line number Diff line change
@@ -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.
15 changes: 15 additions & 0 deletions examples/001-api-rate-limiter/research/summary.md
Original file line number Diff line number Diff line change
@@ -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.
49 changes: 49 additions & 0 deletions examples/001-api-rate-limiter/session.json
Original file line number Diff line number Diff line change
@@ -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"
}
}
Loading