fix(best-practices): restore canonical content, dedupe, sync script, spec exemplar
- best-practices/v1: replace 9 stale symlinks (into planning/v1) with real files synced byte-identical from the canonical best-practices project; add INDEX.md, scripting.md, mechanical-test-generation.md (canonical had drifted heavily, e.g. api-design.md 463->807 lines) - planning/v1: delete duplicated best-practices/ copy (requires: inheritance confirmed via spec/harness.md HC-1/HC-7) - scripts/sync-best-practices.sh: idempotent re-sync from canonical checkout - code-methodology/v1: INDEX.md + scripting.md references now resolve; point test-writing tasks at mechanical-test-generation.md - spec-writing/v1: worked spec exemplar (module layout table, Why: lines, exact error messages, parametrize pattern table) + CLAUDE.md pointer + mount entry
This commit is contained in:
Paul O'Reilly
@@ -20,6 +20,14 @@ Before starting any spec, read these from `/workspace/best-practices/`:
|
||||
|
||||
Read at minimum the four "Always" files. Read others based on the task domain.
|
||||
|
||||
Before drafting, also read the worked exemplar at
|
||||
`/opt/harness/context/spec-writing/spec-exemplar.md`. It shows the full section
|
||||
structure (module layout table, numbered requirements with Why: lines and exact
|
||||
error messages, a pattern table usable as a parametrize matrix, GIVEN/WHEN/THEN
|
||||
scenarios) applied to a small invented subsystem. A spec must let a test-writer
|
||||
agent produce imports, assertions, and mocks with no other input — the exemplar is
|
||||
the bar for that level of concreteness.
|
||||
|
||||
## Spec Structure
|
||||
|
||||
Every spec must follow this structure:
|
||||
|
||||
@@ -9,3 +9,5 @@ provides: [spec-writer]
|
||||
context_files:
|
||||
- source: ./CLAUDE.md
|
||||
target: /opt/harness/context/spec-writing/CLAUDE.md
|
||||
- source: ./spec-exemplar.md
|
||||
target: /opt/harness/context/spec-writing/spec-exemplar.md
|
||||
|
||||
133
harnesses/contexts/spec-writing/v1/spec-exemplar.md
Normal file
133
harnesses/contexts/spec-writing/v1/spec-exemplar.md
Normal file
@@ -0,0 +1,133 @@
|
||||
# Spec Exemplar: Rate-Limit Config Validator
|
||||
|
||||
This is a compact worked example of a spec written for **mechanical test generation**
|
||||
(see `mechanical-test-generation.md` in `/workspace/best-practices/`). It covers a small,
|
||||
self-contained subsystem end-to-end so every section can be read in full. Use it as the
|
||||
structural template for real specs — same section order, same requirement shape, same
|
||||
level of concreteness. The subsystem itself (a rate-limit config validator) is invented
|
||||
for this exemplar; it is not part of any real codebase in this project.
|
||||
|
||||
A test-writer agent given ONLY this document should be able to produce correct imports,
|
||||
correct assertion values, and correct mock boundaries with no other input — no reading
|
||||
the codebase, no guessing module paths, no inventing error message text.
|
||||
|
||||
---
|
||||
|
||||
## Overview
|
||||
|
||||
The rate-limit config validator loads a service's rate-limit policy from a YAML file,
|
||||
validates it against structural and semantic rules, and produces a `RateLimitPolicy`
|
||||
object that the gateway middleware consumes at startup. It exists to catch
|
||||
misconfiguration (overlapping rules, invalid windows, negative limits) before the
|
||||
gateway starts serving traffic, rather than failing silently at request time.
|
||||
|
||||
## Responsibilities
|
||||
|
||||
- Parse a rate-limit policy YAML file into typed Pydantic models
|
||||
- Validate structural correctness (required fields, types, value ranges)
|
||||
- Validate semantic correctness (no two rules match the same route + method with
|
||||
different limits; window units are one of a fixed set)
|
||||
- Produce a single validation report listing every error found (not just the first)
|
||||
- Expose `load_policy(path) -> RateLimitPolicy` as the sole public entry point
|
||||
|
||||
## Dependencies
|
||||
|
||||
- `pydantic` v2 (data models and field validation)
|
||||
- `pyyaml` (YAML parsing) — errors from malformed YAML are caught and re-raised as
|
||||
`PolicyLoadError`, never allowed to propagate as raw `yaml.YAMLError`
|
||||
- No network calls, no filesystem writes — this is a pure load-and-validate module
|
||||
|
||||
## Module Layout
|
||||
|
||||
| Module | Location | Key exports |
|
||||
|---|---|---|
|
||||
| Policy models | `gateway/ratelimit/models.py` | `RateLimitRule`, `RateLimitPolicy`, `WindowUnit` (enum: `second`, `minute`, `hour`) |
|
||||
| Validator | `gateway/ratelimit/validator.py` | `load_policy(path: str) -> RateLimitPolicy`, `PolicyLoadError`, `PolicyValidationError` |
|
||||
| Overlap check | `gateway/ratelimit/overlap.py` | `find_overlapping_rules(rules: list[RateLimitRule]) -> list[tuple[RateLimitRule, RateLimitRule]]` |
|
||||
|
||||
| Test file | Spec requirements covered |
|
||||
|---|---|
|
||||
| `tests/ratelimit/test_models.py` | RLV-1, RLV-2 |
|
||||
| `tests/ratelimit/test_validator.py` | RLV-3, RLV-4, RLV-5 |
|
||||
| `tests/ratelimit/test_overlap.py` | RLV-6 |
|
||||
|
||||
## Requirements
|
||||
|
||||
- **RLV-1:** `RateLimitRule.limit` MUST be a positive integer (`>= 1`). A value of `0`
|
||||
or negative raises `pydantic.ValidationError` at model construction time.
|
||||
- Why: A zero or negative limit is not a valid rate limit — it either blocks
|
||||
everything or is meaningless. Catching it at the model layer means every caller
|
||||
gets the same guarantee for free, without re-checking in the validator.
|
||||
- **Scenario:** GIVEN a rule dict `{"route": "/api/orders", "method": "POST", "limit": 0, "window": 60, "window_unit": "second"}`, WHEN `RateLimitRule(**rule)` is constructed, THEN `pydantic.ValidationError` is raised mentioning field `limit`.
|
||||
|
||||
- **RLV-2:** `RateLimitRule.window_unit` MUST be one of `WindowUnit.second`,
|
||||
`WindowUnit.minute`, `WindowUnit.hour`. Any other string value raises
|
||||
`pydantic.ValidationError` at construction time — no case-insensitive matching,
|
||||
no aliasing (`"secs"`, `"s"` are rejected).
|
||||
- Why: Silent unit aliasing is how a `window: 5, window_unit: "s"` rule quietly
|
||||
becomes a 5-hour window instead of 5 seconds. Reject anything not in the enum.
|
||||
- **Scenario:** GIVEN a rule dict with `"window_unit": "seconds"` (plural, not in the enum), WHEN `RateLimitRule(**rule)` is constructed, THEN `pydantic.ValidationError` is raised mentioning field `window_unit`.
|
||||
|
||||
- **RLV-3:** `load_policy(path)` MUST raise `PolicyLoadError` with the message
|
||||
`"failed to parse YAML: <path>"` when the file exists but contains malformed YAML.
|
||||
The underlying `yaml.YAMLError` MUST NOT propagate directly.
|
||||
- Why: The gateway's startup code catches `PolicyLoadError` specifically to produce
|
||||
a clean "refusing to start: bad rate-limit config" message. Letting a raw
|
||||
`yaml.YAMLError` through breaks that error handling and dumps a parser traceback
|
||||
on operators instead.
|
||||
- **Scenario:** GIVEN a file containing `route: [unclosed`, WHEN `load_policy(path)` is called, THEN `PolicyLoadError` is raised with message `"failed to parse YAML: <path>"`.
|
||||
|
||||
- **RLV-4:** `load_policy(path)` MUST raise `FileNotFoundError` (not `PolicyLoadError`)
|
||||
when `path` does not exist on disk.
|
||||
- Why: Missing file and malformed file are different failure classes for an
|
||||
operator — missing file usually means a deploy/mount problem, malformed file
|
||||
means a config authoring problem. Callers need to tell them apart.
|
||||
- **Scenario:** GIVEN `path = "/etc/gateway/does-not-exist.yaml"`, WHEN `load_policy(path)` is called, THEN `FileNotFoundError` is raised.
|
||||
|
||||
- **RLV-5:** `load_policy(path)` MUST raise `PolicyValidationError` collecting ALL
|
||||
semantic validation failures (not just the first) when the parsed rules contain
|
||||
overlapping routes (see RLV-6) or duplicate `(route, method)` pairs with identical
|
||||
limits. `PolicyValidationError.errors` is a `list[str]`, one entry per problem
|
||||
found, each in the form `"<route> <method>: <reason>"`.
|
||||
- Why: Config authors iterate faster when they see every problem in one pass
|
||||
instead of fixing one error, rerunning, hitting the next error.
|
||||
- **Scenario:** GIVEN a policy YAML with two rules both matching `POST /api/orders` at different limits, and a third rule with `window_unit: "hour"` and `window: 0`, WHEN `load_policy(path)` is called, THEN `PolicyValidationError` is raised with `len(errors) == 2`.
|
||||
|
||||
- **RLV-6:** `find_overlapping_rules(rules)` MUST return every pair of rules that
|
||||
share the same `route` and `method` but have a different `limit`, `window`, or
|
||||
`window_unit`. Rules with identical `(route, method)` AND identical
|
||||
`(limit, window, window_unit)` are NOT considered overlapping (harmless
|
||||
duplication, not a conflict) — implementation note: dedupe on the full tuple
|
||||
before pairing, not just `(route, method)`.
|
||||
- Why: Two rules for the same route+method with different limits is ambiguous —
|
||||
which one applies at request time is undefined. Two identical rules are just a
|
||||
copy-paste no-op and shouldn't block a deploy.
|
||||
- **Scenario:** GIVEN rules `[{route: "/x", method: "GET", limit: 10, window: 60, window_unit: "second"}, {route: "/x", method: "GET", limit: 20, window: 60, window_unit: "second"}]`, WHEN `find_overlapping_rules(rules)` is called, THEN it returns one pair containing both rules.
|
||||
|
||||
## Pattern Table (parametrize matrix)
|
||||
|
||||
| `window_unit` input | Valid? | Why |
|
||||
|---|---|---|
|
||||
| `"second"` | ✓ | matches `WindowUnit.second` |
|
||||
| `"minute"` | ✓ | matches `WindowUnit.minute` |
|
||||
| `"hour"` | ✓ | matches `WindowUnit.hour` |
|
||||
| `"seconds"` | ✗ | plural not aliased (RLV-2) |
|
||||
| `"SECOND"` | ✗ | no case-insensitive matching (RLV-2) |
|
||||
| `"s"` | ✗ | no abbreviation aliasing (RLV-2) |
|
||||
| `"day"` | ✗ | not a supported unit |
|
||||
| `""` | ✗ | empty string is not a valid enum member |
|
||||
|
||||
## Scenarios
|
||||
|
||||
### Happy path: valid policy loads cleanly
|
||||
**GIVEN** a policy YAML with three non-overlapping rules, all fields valid.
|
||||
**WHEN** `load_policy(path)` is called.
|
||||
**THEN** it returns a `RateLimitPolicy` with `len(policy.rules) == 3` and no exception
|
||||
is raised.
|
||||
|
||||
### Atomic failure: partial policy never returned on validation error
|
||||
**GIVEN** a policy YAML where rule 1 is valid and rule 2 has `limit: -5`.
|
||||
**WHEN** `load_policy(path)` is called.
|
||||
**THEN** `pydantic.ValidationError` propagates from rule 2's construction and
|
||||
`load_policy` does not return a partially-built `RateLimitPolicy` — the caller gets
|
||||
either a complete, fully-valid policy or an exception, never a partial object.
|
||||
Reference in New Issue
Block a user