diff --git a/README-zh.md b/README-zh.md index 3b5038e..314e54b 100644 --- a/README-zh.md +++ b/README-zh.md @@ -14,17 +14,36 @@ skill 就是纯 markdown，任何能遵循指令的 coding agent 都能用：Cla ## 安装 +安装 `old-coder`： + ```sh -npx skills add https://github.com/amazingang/old-coder +npx skills add https://github.com/amazingang/old-coder --skill old-coder ``` 也可以手动安装： -- **Claude Code**——把 skill 拷进 skills 文件夹，然后用 `/old-coder` 调用，或在"证明它能用"这类请求时让它自动触发： +- **Claude Code**——把 skill 拷进 skills 文件夹，然后用 `/old-coder` 调用，或让它在高可靠性任务中自动触发： ```sh - cp -r skills/old-coder ~/.claude/skills/ # 或 /.claude/skills/ + cp -r skills/old-coder ~/.claude/skills/ + # 或拷贝到 /.claude/skills/ ``` -- **其他 agent**——把 `skills/old-coder/SKILL.md` 加进你的 `AGENTS.md`、规则文件或 system prompt，并把 `references/gauntlet.md` 放在旁边备查。 +- **其他 agent**——把 `skills/old-coder/SKILL.md` 加进你的 `AGENTS.md`、规则文件或 system prompt，并将它的 `references/` 目录放在旁边备查。 + +### 可选配套 skill：`old-coder-api` + +仓库还包含一个专门用于 HTTP/JSON API 设计与评审的 skill，在需要兼容性、授权、幂等、分页、限流与可运维性闸门时安装： + +```sh +npx skills add https://github.com/amazingang/old-coder --skill old-coder-api +``` + +同时安装两个 skill： + +```sh +npx skills add https://github.com/amazingang/old-coder --skill old-coder --skill old-coder-api +``` + +两者同时适用时，`old-coder` 负责流程、批准与证据，`old-coder-api` 负责 API 契约；API 闸门结论进入 SPEC，并由 gauntlet 验证。 @@ -84,8 +103,9 @@ agent 是在给自己的作业打分，所以规则很严：不许为通过而 ## 仓库里有什么 ``` -skills/old-coder/ skill 本体（SKILL.md + references/gauntlet.md） -demo-rate-limiter/ 按此 skill 端到端做出来的限流器示例 +skills/old-coder/ 可靠编码流程（SKILL.md + references/） +skills/old-coder-api/ HTTP/JSON API 设计与评审（SKILL.md + references/） +demo-rate-limiter/ 按 old-coder 端到端做出来的限流器示例 ``` demo 的 `evidence.md` 就是重点：41 个测试、100% 覆盖率（49/49 个语句、20/20 个分支），22/22 个埋入的 bug 全部被抓。更重要的是，对此前绿色状态进行的 fresh-context verification 仍发现了真实的行为缺陷和一个不可靠的 mutation runner——这恰好说明，关卡全绿并不能自证其可信。当前报告同时披露了修复情况和最终源码状态的验证状态。整份报告可以重跑： diff --git a/README.md b/README.md index f9e1a14..d4aa4f6 100644 --- a/README.md +++ b/README.md @@ -8,23 +8,42 @@ **An old coder's strategy for the agent era: don't read the code — make it run the gauntlet.** -A skill that makes coding agents **prove their work**. Instead of you reading every line the agent writes, the agent must push its code through a gauntlet of checks — and hand you a test plan before coding and an evidence report after. You review those two documents, not the code. +A skill that makes coding agents **prove their work**. Instead of you reading every line, the agent pushes its code through a gauntlet of checks and hands you a test plan before coding and an evidence report after. You review those two documents, not the code. It's plain markdown, so it works with any coding agent that follows instructions: Claude Code, Codex CLI, Cursor, Aider, or your own agent loop. ## Installation +Install `old-coder`: + ```sh -npx skills add https://github.com/amazingang/old-coder +npx skills add https://github.com/amazingang/old-coder --skill old-coder ``` Or manually: -- **Claude Code** — copy the skill into a skills folder, then invoke `/old-coder` or let it trigger on "prove it works"-style requests: +- **Claude Code** — copy the skill into a skills folder, then invoke `/old-coder` or let it trigger on high-assurance requests: ```sh - cp -r skills/old-coder ~/.claude/skills/ # or /.claude/skills/ + cp -r skills/old-coder ~/.claude/skills/ + # or copy it to /.claude/skills/ ``` -- **Other agents** — add `skills/old-coder/SKILL.md` to your `AGENTS.md`, rules file, or system prompt, and keep `references/gauntlet.md` alongside it. +- **Other agents** — add `skills/old-coder/SKILL.md` to your `AGENTS.md`, rules file, or system prompt, and keep its `references/` directory alongside it. + +### Optional companion: `old-coder-api` + +This repository also includes a focused HTTP/JSON API design and review skill. Install it when you want compatibility, authorization, idempotency, pagination, rate-limit, and operability gates: + +```sh +npx skills add https://github.com/amazingang/old-coder --skill old-coder-api +``` + +To install both skills: + +```sh +npx skills add https://github.com/amazingang/old-coder --skill old-coder --skill old-coder-api +``` + +When both apply, `old-coder` owns workflow, approval, and evidence; `old-coder-api` owns the API contract, and its gate decisions become SPEC constraints and gauntlet checks. ## The idea @@ -79,8 +98,9 @@ And one limit stated plainly: the gauntlet turns the constraints expressed in th ## What's in the repo ``` -skills/old-coder/ the skill (SKILL.md + references/gauntlet.md) -demo-rate-limiter/ a rate limiter built end-to-end under the skill +skills/old-coder/ reliable coding workflow (SKILL.md + references/) +skills/old-coder-api/ HTTP/JSON API design and review (SKILL.md + references/) +demo-rate-limiter/ a rate limiter built end-to-end under old-coder ``` The demo's `evidence.md` is the point of the exercise: 41 tests, 100% coverage (49/49 statements and 20/20 branches), and 22/22 planted bugs caught. More importantly, fresh-context verification of earlier green states still found real behavioral defects and an unsound mutation runner — evidence that a green gauntlet is not self-authenticating. The current report discloses both the fixes and the final state's verification status. Rerun the whole report: diff --git a/demo-rate-limiter/evidence.md b/demo-rate-limiter/evidence.md index c563973..afdf021 100644 --- a/demo-rate-limiter/evidence.md +++ b/demo-rate-limiter/evidence.md @@ -1,34 +1,34 @@ # Evidence Report — Sliding-Window Rate Limiter (Tier 3) -- Spec approval: **obtained** for REVISION 4 (2026-08-09) — the human approved - each contract change item by item before implementation. Earlier revisions - (2026-07-25, 2026-07-27) were autonomous and are still unapproved; treat - them as the weaker part of the spec. +- Spec approval: **obtained** for REVISION 4 (2026-08-09) and REVISION 5 + (2026-08-18) — the human approved each contract change before + implementation. Earlier revisions (2026-07-25, 2026-07-27) were autonomous + and are still unapproved; treat them as the weaker part of the spec. - Independent verification: **not performed against the final source state - `8b88bda`.** Six earlier rounds were performed; the last verified state + `d45cc2f`.** Six earlier rounds were performed; the last verified state `d0b506c` returned `failed`, and the fixes made since — one of them behavioural — are disclosed below as unverified. This report is finalized as a **declared downgrade**, not on the strength of a passing verdict. A verdict attaches to the state a verifier actually saw, and no verifier has seen this one. -- Source state: git commit `8b88bda`; sha256 tree hash `c80e8cccf0a1ed3a` — - reproduce both with `./tools/source_state.sh` (works from any directory; - now includes `.github/workflows`, which decides whether the gauntlet runs - in CI at all). Commits after `8b88bda` touch only paths outside the hashed - tree — `skills/`, the READMEs, `CONTRIBUTING.md`, and this report itself — - hence the same hash at a later HEAD, not a stale binding. +- Source state: source commit `d45cc2f`; sha256 tree hash + `76389992f4e342e2` — reproduce both with `./tools/source_state.sh` from any + directory. The script separately reports current HEAD; commits after + `d45cc2f` that touch only this report or other out-of-scope paths preserve + the source commit and tree hash. The manifest includes `.github/workflows`, + which decides whether the gauntlet runs in CI at all. - Toolchain: pinned in `requirements-dev.txt` (local run: Python 3.14.3; CI runs the same gauntlet on 3.12 via `.github/workflows/gauntlet.yml`). - Entry point: `./tools/gauntlet.sh` reruns every layer below. All numbers are from one final fresh run of the entry point, executed -2026-08-10 after the last code edit. +2026-08-18 at source commit `d45cc2f` after the last code edit. -`spec.md` was deliberately pruned back to a contract afterwards (339 → 255 -lines). Every clause, invariant, obligation and failure-model row survives; -what was removed is the per-revision forensics, which lives in the honest -notes below and in git. The spec is the artifact a human reads before any -code exists, and it had stopped being readable as one. +`spec.md` was deliberately pruned back to a contract before REVISION 5 +(339 → 255 lines). Every clause, invariant, obligation and failure-model row +survived; what was removed is the per-revision forensics, which lives in the +honest notes below and in git. REVISION 5 adds the approved source-binding +contract and tests without changing rate-limiter behaviour. ## Spec → Test mapping @@ -64,24 +64,27 @@ Status legend: pass / fail / unverified / n-a. | Must NOT: denials store nothing (no memory growth) | test_ratelimiter.py::test_must_not_denials_store_nothing + M8 | pass | | Must NOT: the limiter is never driven by a real clock | layer: must-not scan in `tools/gauntlet.sh` over tests/ → no matches | pass | | failure-model row: allow() is atomic | test_ratelimiter.py::test_allow_is_atomic_a_second_caller_cannot_interleave + M13 | pass | +| REVISION 5: source binding is reproducible and fail-closed | test_source_state.py (ignored artifacts, staged/unstaged/untracked/deleted inputs, clean clone, no-Git archive, arbitrary cwd, evidence-only commit) | pass | ## Gauntlet (final fresh run: `./tools/gauntlet.sh`) | Layer | Command | Result | |---|---|---| | Checker self-test | `sh tools/test_gauntlet_checks.sh` (first layer; asserts the must-not scan fails on a planted pattern, passes on a clean tree, and fails closed with a distinct rc 2 when the scan itself breaks) | 3/3 expectations ok | +| Source-state self-test | `pytest -q tests/test_source_state.py` (negative controls for ambient ignored artifacts and every fail-closed branch; clean clone and no-Git archive comparison) | 6/6 passed | | Mutation harness negative control | `python tools/mutants.py --negative-control` (a killer and a strictly-equivalent mutant of identical size under one pinned mtime) | C1 KILLED, C2 SURVIVED — ok | -| Tests | `pytest -q --cov=ratelimiter` | 41 passed, 0 failed | -| Types | `mypy src tests examples tools` (strict) | 0 errors in 6 files | -| Lint + format + complexity | `ruff check . && ruff format --check .` (mccabe ≤ 8) | 0 warnings, 8 files formatted | +| Tests | `pytest -q --cov=ratelimiter` | 47 passed, 0 failed | +| Types | `mypy src tests examples tools` (strict) | 0 errors in 8 files | +| Lint + format + complexity | `ruff check . && ruff format --check .` (mccabe ≤ 8) | 0 warnings, 10 files formatted | | Changed-line coverage | `pytest --cov … --cov-fail-under=100` | 49/49 statements, 20/20 branches (100%). **This layer is a gate**; before 2026-08-09 it printed a percentage and exited 0 no matter how far coverage fell | | Mutation | `python tools/mutants.py` (manual, scripted; only pytest exit 1 counts as a kill; `__pycache__` cleared and `PYTHONDONTWRITEBYTECODE` set per mutant) | 22/22 killed | | Property-based | hypothesis, 2 properties | 100 examples each, 0 falsified | | Real execution | `python examples/demo.py` (real `time.monotonic`) | burst of 5 → `[True, True, True, False, False]`; other key unaffected; allowed again after window | | Supply chain | `pip-audit -r requirements-dev.txt` | no known vulnerabilities; runtime dependencies: **none** (stdlib only; `threading` is stdlib) | | Secret scan | must-not scan in `tools/gauntlet.sh` over src, tests, tools, examples, spec.md, pyproject.toml, requirements-dev.txt and `../.github` | clean, no matches | +| Source binding | `tools/source_state.sh` (last gauntlet layer) | HEAD/source commit `d45cc2f`; tree `76389992f4e342e2` | | License check | — | n-a: zero runtime dependencies, nothing redistributed beyond this repo's own MIT code | -| Suite health | pytest-randomly (order shuffled every run) | 41 passed in randomized order, 10/10 consecutive runs | +| Suite health | pytest-randomly (order shuffled every run) | 47 passed in randomized order, 10/10 consecutive runs | ## Layer attribution @@ -148,10 +151,25 @@ independently verified**: - the six prose corrections listed in commit `66df5cd`; - the prune of `spec.md` from 339 to 255 lines in commit `8b88bda`. No clause was changed, but it is a large edit to the document a verifier attacks - hardest, and it was made after the last verified state. + hardest, and it was made after the last verified state; +- REVISION 5 and the reproducible, fail-closed source-state mechanism in + commits `86bfcf4` and `d45cc2f`. ## Honest notes +- **The previous source binding was invalid.** The reported tree + `c80e8cccf0a1ed3a` included four Git-ignored `*.egg-info` files created by + `pip install -e .`; a clean checkout at the cited commit instead produced + `939188446f61289c`. Because those generated files can also vary with the + setuptools version, neither hash was a trustworthy Git-source binding. The + old `find | sort | xargs | shasum` pipeline could additionally print success + after a missing input. REVISION 5 replaces it with a canonical tracked-file + manifest, explicit dirty/untracked rejection, structured path+content + hashing, a deterministic no-Git fallback, six negative controls, and a + second clean-state check around hashing. The corrected source binding is + `d45cc2f` / `76389992f4e342e2`; it is gauntlet-tested but not independently + verified. + - **The A/B experiment that started this failed.** The design was to plant a defect in one copy and verify a clean copy as a false-positive control. The "clean" arm was not clean: it independently invented the exact mutation that diff --git a/demo-rate-limiter/spec.md b/demo-rate-limiter/spec.md index 2f7bb29..c498d0b 100644 --- a/demo-rate-limiter/spec.md +++ b/demo-rate-limiter/spec.md @@ -245,6 +245,47 @@ table. A row whose catcher cannot be shown to fail is a defect, not a mapping. the gap is visible rather than absent. - **Distributed / multi-process limiting.** In-process state only. +## REVISION 5 — reproducible source-state binding (Tier 3) + +Approved 2026-08-18. This revision repairs the evidence mechanism; it does +not change rate-limiter runtime behaviour or its public API. + +### Behaviour + +- In a Git checkout, `tools/source_state.sh` hashes only version-controlled + files in the declared source scope. Ignored build products such as + `*.egg-info`, bytecode caches and coverage output cannot change the hash. +- The same tracked content produces the same tree hash in the working tree, a + clean checkout and the no-Git archive fallback, regardless of current + working directory. +- In Git, relevant staged changes, unstaged changes, deletions or non-ignored + untracked files make the command fail closed instead of emitting a binding. +- The command reports both current HEAD and the most recent commit that + changed the source scope. A later evidence-only commit may change HEAD while + preserving the source commit and tree hash. +- Missing or unreadable manifest inputs make the command fail non-zero; no + partial hash may be reported. +- The gauntlet runs a negative-control self-test for these properties and then + emits the source-state binding only after every other layer has passed. + +### Must NOT do + +- Do not derive a Git binding from ambient ignored files on disk. +- Do not silently omit a new, non-ignored file inside the source scope. +- Do not use a hashing pipeline whose intermediate read failure can be hidden + by the exit status of its final command. +- Do not add a runtime or development dependency for this repair. + +### Setup plan + +- Modify `tools/source_state.sh`; add its implementation and regression tests + under `tools/` and `tests/`; connect the self-test and binding to + `tools/gauntlet.sh`; clarify the reusable rule in the old-coder evidence + template; update `evidence.md` after the implementation commit is clean. +- Commit cadence: this approved SPEC first; tests plus implementation second; + evidence rebinding third. Independent verification remains `not performed` + unless a separate verifier actually inspects the final source state. + ## Revision history Revisions 1–3 (2026-07-25 → 07-27) were made autonomously during the original diff --git a/demo-rate-limiter/tests/test_source_state.py b/demo-rate-limiter/tests/test_source_state.py new file mode 100644 index 0000000..e8efbff --- /dev/null +++ b/demo-rate-limiter/tests/test_source_state.py @@ -0,0 +1,164 @@ +"""Negative controls for the evidence source-state binding.""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +TOOLS = Path(__file__).resolve().parents[1] / "tools" + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + text=True, + ) + + +def _fixture_repo(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + demo = repo / "demo-rate-limiter" + for directory in ( + repo / ".github/workflows", + demo / "examples", + demo / "src/ratelimiter", + demo / "tests", + demo / "tools", + ): + directory.mkdir(parents=True) + + (repo / ".gitignore").write_text( + "*.egg-info/\n__pycache__/\n*.pyc\n.coverage\ncoverage.xml\n", + encoding="utf-8", + ) + (repo / ".github/workflows/gauntlet.yml").write_text("name: test\n") + (repo / "README.md").write_text("metadata only\n") + (demo / "examples/demo.py").write_text("print('demo')\n") + (demo / "pyproject.toml").write_text("[project]\nname = 'fixture'\n") + (demo / "requirements-dev.txt").write_text("pytest==0\n") + (demo / "spec.md").write_text("# fixture spec\n") + (demo / "src/ratelimiter/__init__.py").write_text("VALUE = 1\n") + (demo / "tests/test_fixture.py").write_text("def test_fixture(): pass\n") + shutil.copy2(TOOLS / "source_state.sh", demo / "tools/source_state.sh") + implementation = TOOLS / "source_state.py" + if implementation.exists(): + shutil.copy2(implementation, demo / "tools/source_state.py") + + _git(repo, "init", "-q") + _git(repo, "config", "user.name", "Source State Test") + _git(repo, "config", "user.email", "source-state@example.invalid") + _git(repo, "add", ".") + _git(repo, "commit", "-qm", "fixture") + return repo + + +def _run_state( + repo: Path, *, cwd: Path | None = None +) -> subprocess.CompletedProcess[str]: + script = repo / "demo-rate-limiter/tools/source_state.sh" + return subprocess.run( + [str(script)], + cwd=cwd or repo, + check=False, + capture_output=True, + text=True, + ) + + +def _state(result: subprocess.CompletedProcess[str]) -> dict[str, str]: + assert result.returncode == 0, result.stderr + state = dict(line.split(":", 1) for line in result.stdout.splitlines()) + return {key.strip(): value.strip() for key, value in state.items()} + + +def test_ignored_build_products_do_not_change_tree_hash(tmp_path: Path) -> None: + repo = _fixture_repo(tmp_path) + before = _state(_run_state(repo)) + generated = repo / "demo-rate-limiter/src/ratelimiter.egg-info" + generated.mkdir() + (generated / "PKG-INFO").write_text("generated metadata\n") + cache = repo / "demo-rate-limiter/tests/__pycache__" + cache.mkdir() + (cache / "test_fixture.pyc").write_bytes(b"generated bytecode") + + after = _state(_run_state(repo)) + + assert after["tree"] == before["tree"] + + +def test_dirty_tracked_source_fails_closed(tmp_path: Path) -> None: + repo = _fixture_repo(tmp_path) + source = repo / "demo-rate-limiter/src/ratelimiter/__init__.py" + source.write_text("VALUE = 2\n") + + result = _run_state(repo) + + assert result.returncode != 0 + assert "dirty" in result.stderr.lower() + + _git(repo, "add", "demo-rate-limiter/src/ratelimiter/__init__.py") + staged_result = _run_state(repo) + assert staged_result.returncode != 0 + assert "dirty" in staged_result.stderr.lower() + + +def test_nonignored_untracked_source_fails_closed(tmp_path: Path) -> None: + repo = _fixture_repo(tmp_path) + (repo / "demo-rate-limiter/src/ratelimiter/new.py").write_text("VALUE = 2\n") + + result = _run_state(repo) + + assert result.returncode != 0 + assert "untracked" in result.stderr.lower() + + +def test_missing_manifest_input_fails_closed(tmp_path: Path) -> None: + repo = _fixture_repo(tmp_path) + (repo / "demo-rate-limiter/pyproject.toml").unlink() + + result = _run_state(repo) + + assert result.returncode != 0 + + +def test_clean_archive_matches_git_from_any_directory(tmp_path: Path) -> None: + repo = _fixture_repo(tmp_path) + git_state = _state(_run_state(repo, cwd=tmp_path)) + clone = tmp_path / "clone" + subprocess.run( + ["git", "clone", "-q", str(repo), str(clone)], + check=True, + capture_output=True, + text=True, + ) + clone_state = _state(_run_state(clone, cwd=clone / "demo-rate-limiter/tests")) + archive = tmp_path / "archive" + for relative in _git(repo, "ls-files").stdout.splitlines(): + source = repo / relative + destination = archive / relative + destination.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(source, destination) + + archive_state = _state(_run_state(archive, cwd=archive / "demo-rate-limiter/src")) + + assert clone_state["tree"] == git_state["tree"] == archive_state["tree"] + assert archive_state["head"] == "(no git)" + assert archive_state["source commit"] == "(no git)" + + +def test_evidence_only_commit_preserves_source_binding(tmp_path: Path) -> None: + repo = _fixture_repo(tmp_path) + before = _state(_run_state(repo)) + (repo / "README.md").write_text("new evidence prose\n") + _git(repo, "add", "README.md") + _git(repo, "commit", "-qm", "evidence only") + + after = _state(_run_state(repo)) + + assert after["head"] != before["head"] + assert after["source commit"] == before["source commit"] + assert after["tree"] == before["tree"] diff --git a/demo-rate-limiter/tools/gauntlet.sh b/demo-rate-limiter/tools/gauntlet.sh index b53c727..b3e9e3f 100755 --- a/demo-rate-limiter/tools/gauntlet.sh +++ b/demo-rate-limiter/tools/gauntlet.sh @@ -13,6 +13,9 @@ PY=.venv/bin echo "=== checker self-test ===" sh tools/test_gauntlet_checks.sh +echo "=== source-state self-test ===" +"$PY/pytest" -q tests/test_source_state.py + echo "=== tests + coverage ===" # --cov-fail-under makes this layer a gate. Without it the layer printed a # percentage and exited 0 no matter how far coverage fell: a fail-open layer @@ -51,4 +54,6 @@ echo "=== mutation ===" "$PY/python" tools/mutants.py echo "=== real execution ===" "$PY/python" examples/demo.py +echo "=== source state ===" +tools/source_state.sh echo "=== gauntlet: all layers green ===" diff --git a/demo-rate-limiter/tools/source_state.py b/demo-rate-limiter/tools/source_state.py new file mode 100644 index 0000000..d6c4e83 --- /dev/null +++ b/demo-rate-limiter/tools/source_state.py @@ -0,0 +1,176 @@ +"""Produce a deterministic, fail-closed binding for the demo source tree.""" + +from __future__ import annotations + +import hashlib +import os +import subprocess +import sys +from pathlib import Path + +DEMO = Path(__file__).resolve().parent.parent +ROOT = DEMO.parent +SCOPES = ( + ".github/workflows", + "demo-rate-limiter/examples", + "demo-rate-limiter/pyproject.toml", + "demo-rate-limiter/requirements-dev.txt", + "demo-rate-limiter/spec.md", + "demo-rate-limiter/src", + "demo-rate-limiter/tests", + "demo-rate-limiter/tools", +) +EXCLUDED_DIRS = { + ".hypothesis", + ".mypy_cache", + ".pytest_cache", + ".ruff_cache", + ".venv", + "__pycache__", +} +EXCLUDED_FILES = {".coverage", ".DS_Store", "coverage.xml"} + + +def _git( + root: Path, *args: str, check: bool = True +) -> subprocess.CompletedProcess[bytes]: + return subprocess.run( + ["git", *args], + cwd=root, + check=check, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + + +def _git_root() -> Path | None: + discovered = _git(DEMO, "rev-parse", "--show-toplevel", check=False) + if discovered.returncode != 0: + return None + root = Path(os.fsdecode(discovered.stdout.rstrip(b"\n"))) + relative_script = (DEMO / "tools/source_state.sh").relative_to(root).as_posix() + tracked = _git( + root, "ls-files", "--error-unmatch", "--", relative_script, check=False + ) + return root if tracked.returncode == 0 else None + + +def _manifest_from_git(root: Path) -> list[str]: + status = _git( + root, + "status", + "--porcelain=v1", + "-z", + "--untracked-files=all", + "--ignored=no", + "--", + *SCOPES, + ).stdout + if status: + records = [record for record in status.split(b"\0") if record] + issue = ( + "untracked files" + if any(record.startswith(b"?? ") for record in records) + else "dirty files" + ) + details = ", ".join(os.fsdecode(record) for record in records) + raise RuntimeError(f"source scope contains {issue}: {details}") + + output = _git(root, "ls-files", "-z", "--", *SCOPES).stdout + files = sorted(os.fsdecode(path) for path in output.split(b"\0") if path) + for scope in SCOPES: + if not any(path == scope or path.startswith(f"{scope}/") for path in files): + raise RuntimeError(f"source scope has no tracked input: {scope}") + return files + + +def _is_generated(relative: Path) -> bool: + return ( + any( + part in EXCLUDED_DIRS or part.endswith(".egg-info") + for part in relative.parts + ) + or relative.name in EXCLUDED_FILES + or relative.suffix == ".pyc" + ) + + +def _manifest_without_git(root: Path) -> list[str]: + files: list[str] = [] + for scope in SCOPES: + candidate = root / scope + if not candidate.exists(): + raise RuntimeError(f"source input is missing: {scope}") + inputs = [candidate] if candidate.is_file() else candidate.rglob("*") + scoped_files = [ + path + for path in inputs + if (path.is_file() or path.is_symlink()) + and not _is_generated(path.relative_to(root)) + ] + if not scoped_files: + raise RuntimeError(f"source scope has no input: {scope}") + files.extend(path.relative_to(root).as_posix() for path in scoped_files) + return sorted(files) + + +def _read_input(path: Path) -> bytes: + if path.is_symlink(): + return os.fsencode(os.readlink(path)) + if not path.is_file(): + raise RuntimeError(f"source input is not a regular file: {path}") + try: + return path.read_bytes() + except OSError as error: + raise RuntimeError(f"cannot read source input {path}: {error}") from error + + +def _tree_hash(root: Path, files: list[str]) -> str: + digest = hashlib.sha256() + for relative in files: + path_bytes = relative.encode("utf-8") + content = _read_input(root / relative) + digest.update(len(path_bytes).to_bytes(8, "big")) + digest.update(path_bytes) + digest.update(len(content).to_bytes(8, "big")) + digest.update(content) + return digest.hexdigest()[:16] + + +def main() -> int: + try: + git_root = _git_root() + root = git_root or ROOT + if git_root: + head_before = os.fsdecode( + _git(root, "rev-parse", "--short", "HEAD").stdout + ).strip() + files = _manifest_from_git(root) + tree = _tree_hash(root, files) + if _manifest_from_git(root) != files: + raise RuntimeError("source manifest changed while hashing") + head = os.fsdecode( + _git(root, "rev-parse", "--short", "HEAD").stdout + ).strip() + if head != head_before: + raise RuntimeError("HEAD changed while hashing") + source_commit = os.fsdecode( + _git(root, "log", "-1", "--format=%h", "--", *SCOPES).stdout + ).strip() + if not source_commit: + raise RuntimeError("no commit contains the source manifest") + else: + head = source_commit = "(no git)" + tree = _tree_hash(root, _manifest_without_git(root)) + except (OSError, subprocess.CalledProcessError, RuntimeError, ValueError) as error: + print(f"source-state error: {error}", file=sys.stderr) + return 2 + + print(f"head: {head}") + print(f"source commit: {source_commit}") + print(f"tree: {tree}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/demo-rate-limiter/tools/source_state.sh b/demo-rate-limiter/tools/source_state.sh index 6d15547..2fa9039 100755 --- a/demo-rate-limiter/tools/source_state.sh +++ b/demo-rate-limiter/tools/source_state.sh @@ -1,18 +1,6 @@ #!/bin/sh -# Print the demo's source state: git commit (if available) and a sha256 tree -# hash over the source files. Works from any working directory — evidence.md -# cites this script so the recorded hash is reproducible without ambiguity. -set -e -cd "$(dirname "$0")/.." -if git rev-parse --short HEAD >/dev/null 2>&1; then - printf "commit: %s\n" "$(git rev-parse --short HEAD)" -else - printf "commit: (no git)\n" -fi -# ../.github/workflows decides whether the gauntlet runs at all in CI, so it -# belongs in the state the evidence binds to; omitting it let CI config change -# silently under an unchanged hash. -tree_hash=$(find src tests tools examples pyproject.toml requirements-dev.txt spec.md \ - ../.github/workflows \ - -type f -not -path "*__pycache__*" | sort | xargs shasum -a 256 | shasum -a 256 | cut -c1-16) -printf "tree: %s\n" "$tree_hash" +# Stable entry point for evidence.md; the Python implementation avoids shell +# pipelines that can hide an intermediate read failure. +set -eu +script_dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +exec python3 "$script_dir/source_state.py" diff --git a/skills/old-coder-api/SKILL.md b/skills/old-coder-api/SKILL.md new file mode 100644 index 0000000..4d9a6f3 --- /dev/null +++ b/skills/old-coder-api/SKILL.md @@ -0,0 +1,138 @@ +--- +name: old-coder-api +description: Design, change, or review an HTTP/JSON API surface — endpoints, request/response shapes, authentication and authorization, pagination, idempotency, rate limits, versioning, and deprecations. Use when adding or modifying an HTTP endpoint, reviewing an OpenAPI spec or HTTP route diff, or deciding whether an HTTP API change breaks consumers. Do not use as a protocol-compatibility checklist for gRPC/protobuf, GraphQL, WebSockets, or other non-HTTP/JSON interfaces. +--- + +# old-coder-api + +Distilled from Sean Goedecke, *Everything I know about good API design* (2025-08-24). + +This skill covers HTTP/JSON contract and operability concerns. Its compatibility rules assume JSON consumers. For gRPC/protobuf, GraphQL, WebSockets, or another protocol, apply the transport-independent principles only alongside that protocol's own compatibility rules. This is not a substitute for a full application-security review. + +**Good APIs are boring.** For the people who build them, an API is a product. For the people who use them, it is a tool in the way of something else. Every minute a consumer spends thinking about your API instead of their goal is waste. An interesting API is a bad API — or would be a better one if it were less interesting. + +Two failure modes an agent falls into by default, and this skill exists to stop both: + +1. **Inventing.** Producing a clever, bespoke interface where the boring conventional one would do. +2. **Breaking.** Renaming, restructuring, or tightening a field because it reads better now — and silently breaking every downstream caller. + +**Composition with `old-coder`:** when both skills apply, this skill owns the +HTTP/JSON contract while `old-coder` owns workflow order, SPEC approval, the +gauntlet, and EVIDENCE. Run Step 0 and the gates before SPEC approval; put the +surviving API constraints and risks into SPEC and verify them through the +gauntlet. For review-only work with no implementation, use this skill's review +format without manufacturing a development loop. + +## Step 0 — establish scope before designing anything + +Answer these three, out loud, before writing a route: + +| Question | Why it changes the work | +|---|---| +| **Public or internal?** Can you ship code for every consumer? | Internal: breaking changes are affordable, complex authentication is fine, non-engineer ergonomics don't matter. Public: none of that holds. | +| **Existing surface or greenfield?** | Existing → run `references/breaking-changes.md` **first**; compatibility outranks every improvement below. | +| **Does the product's resource model support this API?** | API design tracks the product's basic resources. If the resources are awkward (state machines with no name, records that only exist inside a job, parent/child relations that aren't modeled), the API will be awkward no matter how carefully you design it. Say so instead of papering over it. | + +**Honesty rule for step 0:** when the ugliness comes from the underlying model, name it and propose the model fix as the real option. A background-job-polling interface bolted onto a read that *should* be a read is how the worst APIs happen — technical constraints that the UI hides get laid bare in the API, forcing consumers to understand far more of your system than they should have to. + +## The gates + +Run every gate. Use **✓** only for a verified pass, **✗ + concrete fix** for a verified failure, **N/A + reason** only when the gate truly does not apply, and **? + reason** when it remains unverified. Never skip silently. + +### 1. Boring +A competent consumer should be able to guess this endpoint before reading any docs. +- Resources are the product's nouns (`/issues`, `/projects`, `/users`), plural, stable. +- Standard verbs and status codes, following the established convention of this API. Use `400` for a general client error; use `422` only when the content type and syntax are valid but the contained instructions cannot be processed. Use `404` for missing and `429` for rate-limited. +- Standard field names: `id`, `created_at`, `next_page`, `url`. Match the names the rest of *this* API already uses — internal consistency beats external convention when they conflict. +- REST + JSON unless there's a reason. An established, internally consistent HTTP RPC surface can also be boring; do not rename it to resource paths for REST purity. Don't relitigate HATEOAS or JSON-vs-anything; it isn't important. +- **Anything surprising needs a written justification line.** If you can't write one, make it boring. + +### 2. Don't break userspace +Applies only to changes on an existing surface. Full matrix in `references/breaking-changes.md`. +- Additive is fine: new endpoints, new optional params, **new response fields**. Consumers are expected to ignore unknown fields. +- Removing a field, renaming it, changing its type, moving it (`user.address` → `user.details.address`), narrowing an enum, or tightening validation is a break. Don't, even if it's neater. The HTTP `referer` header is a misspelling and it is still there. +- If a break is genuinely unavoidable: versioning, as a **last resort** — see the reference. + +### 3. Authentication: make the simplest safe path easy +Many server-to-server integrations start life as a `curl` or a 20-line script. For developer-facing server-to-server APIs, default to simple, scoped, revocable API keys. +- Use OAuth or another short-lived or sender-constrained flow instead for browser/mobile clients, user-delegated access, high-sensitivity data, or environments where policy requires it. Do not ship long-lived bearer credentials into those clients. +- For every credential type, define scope, rotation, revocation, secure transport, and a way to identify or disable the credential during an incident. +- N/A for internal credential ergonomics: use the mechanism the infrastructure already provides (mTLS, workload identity, service tokens), while still verifying its operational controls. + +### 4. Authorization: enforce who may do what to which resource +Authentication identifies a caller; it does not authorize an action. For every endpoint, identify the actor, action, resource, and tenant boundary. +- Enforce authorization server-side on the resolved resource. Do not trust a caller-supplied `tenant_id`, owner ID, role, or scope without checking it against the authenticated principal. +- Apply the same checks to list, search, bulk, export, nested-resource, and indirect lookup paths; filtering after fetching is not an authorization boundary. +- N/A only for intentionally anonymous public operations, with a one-line reason. For security-sensitive changes, require a dedicated security review in addition to these API gates. + +### 5. Idempotency on anything that takes action +A `500` or a timeout tells the caller nothing about whether the action happened. Without an idempotency key, the caller must choose between a lost operation and a duplicate one. +- Every operation that is not already idempotent and creates, triggers, or applies a relative change accepts an idempotency key (header or param); repeat keys return the original result instead of acting twice. +- Keep it **optional for low-stakes operations** where an occasional duplicate is cheaper than added adoption friction. +- When a duplicate is unacceptable — payments, transfers, medication, irreversible external side effects — require an idempotency key or an intrinsic unique operation ID, and enforce deduplication atomically with the effect. +- Not needed for reads (harmless) or `DELETE /comments/32` (the ID *is* the key — the retry just 404s). Exception: non-ID-scoped operations like "delete the most recent". +- Storage recipe in `references/patterns.md`. + +### 6. Blast radius, rate limits, killswitch +UI users are limited by the speed of their hands. **Anything you expose via API is called at the speed of code**, forever, in a loop, by someone who read no docs. +- Before shipping: write down what one caller in a tight `while true` loop costs you. Fan-outs, `/index` endpoints, bulk imports, and anything doing per-record work in a request are the dangerous ones. +- Rate limit everything, with **tighter limits on expensive operations**. +- Return `X-RateLimit-Remaining` and `Retry-After` so well-behaved clients can back off — that metadata is what lets you set stricter limits than you otherwise could. +- Keep a per-consumer killswitch. You will need it during an incident caused by an integration you never imagined. + +### 7. Pagination +- Any collection that could plausibly grow large: **cursor-based**, always. `WHERE id > :cursor ORDER BY id LIMIT :n` stays fast at record one million; `OFFSET` gets slower every page and the migration away from it later is expensive. +- Bounded-forever collections (a user's API keys, a project's 5 environments): page/offset is fine. +- Never return an unbounded list. Always include `next_page` (URL or cursor) so consumers don't compute it. + +### 8. Expensive fields are optional and off by default +If a field needs an extra service call, a join over a big table, or a computation, don't put it in the default response. +- Gate it behind `?include=subscription` / an `includes[]` array; keep the default response cheap and **constant-cost**. +- This is the useful 20% of the GraphQL idea without the cost. +- **Don't propose GraphQL** unless the user asks or the codebase is already GraphQL: high barrier for non-engineers, arbitrary client-crafted queries complicate caching and multiply edge cases, and the backend is fiddlier. It's a last resort, not a default. + +### 9. No implementation leakage +Read the response as a stranger. Does using it correctly require knowing how you store things? +- Leaks: `next_comment_id` chains the client must walk; a `POST /fetch_job` + poll dance for what should be a `GET`; internal enum values; internal table IDs; pagination whose page size depends on your shard layout. +- Either hide it behind a boring interface, or state the debt explicitly in the PR — don't let it slip into a public contract unremarked. + +## Deliberately not gates + +Guard against over-design as hard as under-design: + +- **Don't build versioning machinery up front.** A `/v1/` prefix is itself a public product choice, not a free placeholder. Adopt path or header versioning only when the product's compatibility policy calls for it; do not build multi-version negotiation before a second version exists. +- **Don't add `includes` or cursors to internal endpoints with one caller and a bounded result set.** The Pagination and Expensive fields gates are about potentially large or expensive responses. For Idempotency, caller count does not remove retry risk: omit it only when the operation is already idempotent or duplicate effects are explicitly acceptable. +- **Don't rewrite a working API to be prettier.** Prettiness is not worth a compatibility break, and it isn't worth the review time either. +- **Remember API quality is marginal.** If the product is valuable, people integrate with a terrible API (Facebook, Jira). If it isn't, a beautiful API won't save it. API quality decides between two roughly equivalent products; having *no* API at all is the real defect. So: apply these gates, don't gold-plate past them. + +## Review output format + +When reviewing rather than writing, report only findings that survive verification and skip taste. For repository code, specs, and diffs, cite `file:line`. For published contracts outside the repository, cite a stable URL and exact section; source-code evidence must use an immutable commit permalink, not a moving branch. A missing public guarantee means consumers cannot rely on the behavior; it does **not** prove that the backend lacks an undocumented implementation. Give a gate `✓` only when the reviewed evidence supports it. When repository context is available, inspect beyond the diff instead of treating silence as a pass. If the input is intentionally limited and further evidence is unavailable, use `? (unverified: )`; reserve `N/A` for a gate that truly does not apply. Gate summaries evaluate the artifact under review, not the hypothetical state after suggested fixes. Order: breaks first, security boundaries second, incidents third, ergonomics last. + +``` +## API review: +Scope: public|internal · greenfield|existing + +### Breaking changes (blocking) +- — — file:line + Fix: + +### Security boundary +- — — file:line + +### Incident risk +- — — file:line + +### Ergonomics +- — file:line + +### Gates: Boring · Compatibility · Authentication · Authorization · Idempotency · Blast radius · Pagination · Expensive fields · No implementation leakage +``` + +If nothing survives, say so plainly — an empty review is a valid result. + +## References + +- `references/breaking-changes.md` — compatibility matrix, versioning playbook, deprecation sequence. **Read before changing any existing endpoint.** +- `references/patterns.md` — implementation recipes: idempotency keys, cursor pagination, rate-limit headers, `includes`. +- `references/examples.md` — three compact, local examples: an existing route/spec diff, a greenfield proposal, and an established HTTP RPC route. Read only when a concrete calibration example is useful. diff --git a/skills/old-coder-api/references/breaking-changes.md b/skills/old-coder-api/references/breaking-changes.md new file mode 100644 index 0000000..2a2bcf5 --- /dev/null +++ b/skills/old-coder-api/references/breaking-changes.md @@ -0,0 +1,86 @@ +# Breaking changes, versioning, deprecation + +Read this before modifying any HTTP/JSON endpoint that already has consumers. These rules do not replace protocol-specific compatibility guidance for protobuf, GraphQL, or other representations. + +> As the maintainer of an API you have something like a sacred duty to your downstream consumers. One careless maintainer far enough upstream breaks hundreds of pieces of software. **WE DO NOT BREAK USERSPACE.** + +## Compatibility matrix + +Assume a consumer that parses your JSON into a typed struct, ignores unknown fields, and was written two years ago by someone who has left the company. + +| Change | Safe? | Notes | +|---|---|---| +| New endpoint | ✅ | | +| New **optional** request param | ✅ | Default must reproduce the old behavior exactly. | +| New response field | ✅ | Consumers are expected to ignore unknown fields. A consumer that explodes on extra fields is being irresponsible — but check for known strict clients before assuming. | +| New optional response field inside an existing object | ✅ | | +| Remove a response field | ❌ | Even if it's always `null`. Even if "nobody uses it" — check logs before believing that. | +| Rename a field | ❌ | This is remove + add. If truly needed: add the new name, keep the old one populated, forever. | +| Move a field (`user.address` → `user.details.address`) | ❌ | Same as rename. | +| Change a field's type (`"3"` → `3`, scalar → array, int → string ID) | ❌ | Silently corrupts typed consumers. | +| Change a field's semantics under the same name/type | ❌ | The worst kind: no error, wrong behavior, undetectable in tests. | +| Add a value to a response enum | ⚠️ | Breaks exhaustive-match consumers. Safe only if the enum was documented as open-ended from day one. | +| Remove a value from a request enum | ❌ | | +| **Tighten** validation (new required param, stricter regex, lower max) | ❌ | Requests that worked yesterday now fail with a client error. | +| **Loosen** validation | ✅ | | +| Make a required param optional | ✅ | | +| Change a default value | ❌ | It changes behavior for every caller who omitted the param. | +| Change default page size | ❌ | Callers hardcode the count or the loop bound. | +| Change a status code (`200`→`201`, `404`→`422`) | ❌ | Callers branch on it. | +| Change error response *shape* | ❌ | Callers parse it. Adding a new field to it is fine. | +| Change ordering of a list without a documented sort | ⚠️ | Formally allowed, practically breaks pagination and diff-based consumers. Treat as breaking. | +| New rate limit / tighter rate limit | ⚠️ | Breaks heavy callers at runtime, not at compile time. Announce, measure top callers first, roll out gradually. | +| Fix a bug consumers may have worked around | ⚠️ | Real judgment call. Measure how many callers depend on the buggy behavior. | + +Rule of thumb: **additive is safe, subtractive and restrictive are not.** When unsure, it's breaking. + +## Before claiming "nobody uses this" + +Don't assert it — check, and cite what you checked: +1. Access logs / analytics per endpoint and, if available, per field. +2. First-party callers in the monorepo or sibling repos (grep the path string). +3. SDKs, docs, examples, support macros, and anything a customer copy-pasted from a blog post. + +If you cannot check, you cannot claim it. Say "unverified" in the PR. + +## Versioning — a necessary evil, and a last resort + +It is honestly hard to find a case where an API genuinely *needs* a breaking change. When the technical value is high enough that you bite the bullet anyway, versioning means: **serve the old and the new version at the same time.** + +Two shapes: +- **URL path** — `/v1/chat/completions` → `/v2/chat/completions`. Simplest, most visible, easiest for consumers to reason about. +- **Header / account default** — Stripe's model: a version header, plus a per-account default set in the UI. Consumers upgrade at their own pace. + +Costs to state plainly before proposing it: +- 30 endpoints × a new version = 30 more surfaces to test, debug, document, and support. +- A translation layer (serialize/deserialize per version, one shared core) keeps the codebase from doubling — but that abstraction always leaks. Some version differences require conditional logic down in the core, and Stripe engineers have said so publicly. +- Docs and search become confusing: users land on the wrong version's page. +- Migration takes **months to years**, with banners, emails, response headers — and you will *still* have angry users on removal day. + +So: exhaust the additive options first. In order — +1. New optional param that opts into the new behavior. +2. New field alongside the old one, both populated. +3. New endpoint (`/users/:id/details`) beside the old one. +4. New version. Only here. + +## Deprecation sequence + +When something must eventually go: + +1. **Ship the replacement first.** Never announce a removal before the alternative is live and documented. +2. **Announce**: docs banner, changelog, direct email to identified callers, and a `Deprecation` / `Sunset` header on responses to the old surface. +3. **Measure.** Track calls to the deprecated surface by consumer. This is the number that decides the date, not the calendar. +4. **Wait.** Months, not weeks. Public and widely used: a year is not excessive. +5. **Brownout** (optional, effective): return errors for the old surface for a few scheduled hours, announced in advance. Wakes up the callers that ignored every email. +6. **Remove** — and expect complaints anyway. At that point you've done what you can. + +For internal APIs, compress this hard: you can grep every caller and ship the fix yourself. Do that instead of a deprecation program. + +## Internal APIs + +The relaxations, stated precisely — internal means *you can ship code for every consumer*: +- Breaking changes are affordable: change the callers in the same PR, or in a two-step ship (add new → migrate callers → remove old). +- Auth can be as complex as your infra wants. +- Consumers are professional engineers; ergonomics for non-engineers doesn't apply. + +What does **not** relax: internal APIs are still a top source of incidents, and key operations still need idempotency. A retrying internal client double-charging something is exactly as bad as an external one doing it. diff --git a/skills/old-coder-api/references/examples.md b/skills/old-coder-api/references/examples.md new file mode 100644 index 0000000..cae5dd2 --- /dev/null +++ b/skills/old-coder-api/references/examples.md @@ -0,0 +1,100 @@ +# Compact review examples + +These examples calibrate decisions, not current facts about third-party APIs. The excerpts are intentionally small enough to resemble normal route or OpenAPI review work. + +## Existing public OpenAPI diff: safety versus compatibility + +```text +41 - display_name: { type: string } +41 + name: { type: string } +73 post: +74 + parameters: +75 + - { name: Idempotency-Key, in: header, required: true } +76 operationId: createTransfer +``` + +```text +## API review: transfer API +Scope: public · existing + +### Breaking changes (blocking) +- `display_name` was renamed, so typed consumers lose a field — openapi.yaml:41 + Fix: add `name` and keep populating `display_name`. +- A formerly valid request now fails without a new header — openapi.yaml:75 + Fix: accept the key optionally in this version, generate it by default in official + SDKs, and require it only in a new major contract or new safer operation. + +### Verified context + +- Idempotency ✓ — supplied keys are atomically claimed with the transfer; repeats replay the original status and body, a different request hash is rejected, and records survive the documented retry window — transfers/idempotency.ts:52-81. +- Blast radius ✓ — `createTransfer` inherits write-tier limits, recovery headers, and a per-consumer killswitch — gateway/limits.yaml:88. + +### Gates: Boring ? (unverified: route convention not in diff) · Compatibility ✗ · Authentication ? (unverified: not in diff) · Authorization ? (unverified: not in diff) · Idempotency ✓ (verified contract) · Blast radius ✓ · Pagination N/A (not a collection) · Expensive fields ? (unverified: response cost not in diff) · No implementation leakage ? (unverified: response shape not in diff) +``` +The submitted diff passes Idempotency but fails Compatibility. Making the key optional preserves compatibility but leaves duplicate protection incomplete until a new operation or major contract can require it. Record both states explicitly: compatibility outranks the tempting retroactive fix, but the additive migration is not the final idempotency solution. + +## Greenfield proposal: fix the resource model first + +Context: report exports are durable, long-running product resources, and `rows` has no fixed upper bound. + +Input excerpt (line-numbered `proposal.yaml`): + +```text +12 GET /reports/export +14 api_key: query string +16 tenant_id: query string +18 response: { rows: [...], billing_profile: {...} } +``` + +```text +## API review: report export proposal +Scope: public · greenfield + +### Security boundary +- A query-string credential leaks through URLs and logs — proposal.yaml:14 + Fix: use a scoped, revocable API key in the Authorization header. +- The caller selects a tenant without a stated principal check — proposal.yaml:16 + Fix: derive the tenant from the credential or verify membership server-side. + +### Incident risk +- The export is an unbounded synchronous read that will be called in loops — proposal.yaml:12 + Fix: model exports as `POST /exports` plus `GET /exports/{id}`, rate-limit creation, + and paginate the exported items with a server-provided `next_page`. + +### Ergonomics +- `billing_profile` makes every row pay for an unrelated expensive join — proposal.yaml:18 + Fix: omit it by default and expose `?include=billing_profile` with a documented cap. + +### Gates: Boring ✗ · Compatibility N/A (greenfield) · Authentication ✗ · Authorization ✗ · Idempotency N/A (read-only input) · Blast radius ✗ · Pagination ✗ · Expensive fields ✗ · No implementation leakage ? (unverified: response mechanics not in excerpt) +``` + +The named export resource is justified by genuinely long-running product work. Do not copy this async shape onto an ordinary read merely because the backend happens to use a job internally. + +## Existing HTTP RPC route: do not review for REST purity + +Context: this is an existing public API whose other routes use the same `family.method` convention. + +Input excerpt (line-numbered `routes.ts`): + +```text +17 router.post("/api/chat.postMessage", retryable(handler)) +18 handler.idempotency = false +``` + +```text +## API review: chat post RPC +Scope: public · existing + +### Incident risk +- The wrapper retries an action-creating request without deduplication — routes.ts:17-18 + Fix: accept an idempotency key and replay the stored result; do not retry ambiguous + failures until that contract exists. + +### Gates: Boring ✓ · Compatibility N/A (no contract change shown) · Authentication ? (unverified: not in excerpt) · Authorization ? (unverified: not in excerpt) · Idempotency ✗ · Blast radius ? (unverified: limits not in excerpt) · Pagination N/A (not a collection) · Expensive fields ? (unverified: response not in excerpt) · No implementation leakage ? (unverified: response mechanics not in excerpt) +``` + +`/api/chat.postMessage` is not a finding when `family.method` is the established, predictable convention. Renaming it to look RESTful would create a compatibility break without reducing incident risk. + +## Evidence boundary + +If published documentation does not guarantee deduplication, conclude only that consumers cannot safely rely on deduplication. Do not claim the backend lacks an undocumented mechanism. For external source code, link to an immutable commit and symbol or line range rather than a moving default branch. diff --git a/skills/old-coder-api/references/patterns.md b/skills/old-coder-api/references/patterns.md new file mode 100644 index 0000000..5fe5b0b --- /dev/null +++ b/skills/old-coder-api/references/patterns.md @@ -0,0 +1,109 @@ +# Implementation recipes + +Boring, known-good shapes. Prefer whatever this codebase already does over anything here; consistency inside one API beats matching an external example. + +## Idempotency keys + +**Contract** + +``` +POST /v1/comments +Idempotency-Key: 8f1e... (client-generated UUID, optional) +``` + +- First request with a given key: perform the action, store `key → (status, response body)`. +- Repeat with the same key: return the **stored response**, don't act again. Same status code as the original. +- Same key, *different* request body: reject it using this API's established `400`/`409`/`422` error convention. Silently returning the first response would hide a client bug. +- In flight when the retry arrives: `409`, telling the client to retry shortly. (Take a lock on the key at the start of processing.) + +**Storage** + +A key/value store (Redis) with the idempotency key as the key is enough for most low-stakes cases. Scope by user (`idem:{user_id}:{key}`) — UUIDs are unique enough that you don't strictly need to, but you may as well. Set expiry from a documented retry window; a few hours may be enough for low-stakes immediate retries, but it is not a universal default. + +Caveat worth stating when it matters: Redis and your database can't be updated atomically together, so under a crash between the two you can still double-act. For payments and other high-risk paths, store the key in the same transaction as the effect — e.g. a unique column/row in the same database. For everything else, bolting Redis idempotency onto a non-idempotent API is much better than nothing. + +**Where it's needed** + +| Operation | Key needed? | +|---|---| +| `GET` anything | No — double reads are harmless | +| `DELETE /comments/32` | No — the resource ID *is* the key; retries just `404` | +| `DELETE` "the most recent X" | **Yes** — not ID-scoped | +| `POST /comments` (create) | Yes | +| `POST /transfers`, `/charges`, irreversible side effects | **Required**, unless an intrinsic unique operation ID provides equivalent atomic deduplication | +| `PUT /users/32` (full replace) | Usually not — replacing with the same body twice is the same end state. Still yes if it fires side effects (emails, webhooks, audit rows). | +| `PATCH` with relative semantics (`increment: 1`) | Yes | + +Keep the key **optional for low-stakes operations**. Document it and default it in your own SDKs. For duplicate-intolerable operations, require it (or an equivalent unique operation ID) rather than accepting an unsafe request. + +## Cursor pagination + +**Request/response** + +``` +GET /v1/tickets?limit=50&cursor=eyJpZCI6MzJ9 + +{ + "data": [ ... ], + "next_page": "/v1/tickets?limit=50&cursor=eyJpZCI6ODJ9", + "has_more": true +} +``` + +**Query** + +```sql +SELECT * FROM tickets +WHERE account_id = :account AND id > :cursor +ORDER BY id +LIMIT :limit +``` + +Fast at any depth because the index locates the cursor row directly. `OFFSET 200000` makes the database count through 200,000 rows every time, so each page is slower than the last. + +**Rules** +- Sort key must be **unique and stable**. `ORDER BY created_at` alone breaks on ties — use `(created_at, id)` and encode both in the cursor. +- Opaque cursors (base64 of a small JSON blob) let you change the sort key later without breaking consumers. A raw `cursor=32` is a contract you'll regret. +- Cap `limit` server-side. Document the cap and the default; changing the default later is a breaking change. +- Always emit `next_page` (or `null`) so consumers never construct it. That's also what lets you switch strategies later. +- Ending condition: `has_more: false` / `next_page: null`. Don't make clients infer it from a short page — a short page is legal mid-collection when you filter after fetching. + +Offset pagination is fine for collections that are bounded forever (a user's API keys, a project's environments). Anything user-generated and unbounded: cursor from day one, because retrofitting it later is a breaking change you'll be forced into at the worst moment. + +## Rate limiting + +**Response headers on every rate-limited endpoint** + +``` +X-RateLimit-Limit: 700 +X-RateLimit-Remaining: 412 +X-RateLimit-Reset: 1755300000 # epoch seconds +Retry-After: 32 # seconds; on 429 responses +``` + +Status `429` when exceeded. (Standardized `RateLimit-*` headers exist; if your ecosystem already uses `X-`-prefixed ones, stay consistent with it.) + +**Tiering** + +Set limits by cost, not by uniform policy: + +| Class | Example | Relative limit | +|---|---|---| +| Cheap read by ID | `GET /tickets/32` | High | +| List / search | `GET /tickets?query=` | Medium | +| Write | `POST /tickets` | Medium | +| Fan-out, bulk, export, anything doing per-record work | `POST /apps/:id/notify_all` | Low, and consider making it async with a job resource | + +**Killswitch.** A per-consumer (account, API key, app) disable that an on-call engineer can flip without a deploy. Incidents caused by third-party integrations are routine — polling an `/index` endpoint with no delay, create/delete loops, imports with no backoff — and you need pressure relief that doesn't require the customer's cooperation. + +## Optional / expensive fields + +``` +GET /v1/users/32 → cheap, constant-cost fields only +GET /v1/users/32?include=subscription,posts → adds the expensive ones +``` + +- One `include` param taking a comma-separated list (or `includes[]`) scales better than a boolean per field. +- Validate the values and return the API's established client-error status (`400` or `422`) on unknown ones — a typo that silently returns less data is a bad debugging afternoon. +- Cap what can be combined. `include=posts` on a user with 100k posts is a fan-out; either paginate the sub-resource or expose it as its own endpoint. +- The default response should be **constant-cost**: no N+1, no cross-service call, no unbounded array. That's the invariant this pattern is protecting. diff --git a/skills/old-coder/SKILL.md b/skills/old-coder/SKILL.md index 4573d5f..d4d4916 100644 --- a/skills/old-coder/SKILL.md +++ b/skills/old-coder/SKILL.md @@ -22,6 +22,12 @@ correlation), and why EVIDENCE reports layered, auditable confidence, never absolute proof. Every shortcut you take against the gauntlet destroys the only basis of trust. +**Composition with `old-coder-api`:** when both skills apply, this skill owns +workflow order, SPEC approval, the gauntlet, and EVIDENCE; `old-coder-api` owns +the HTTP/JSON contract. Run its scope check and API gates while drafting SPEC, +turn the surviving constraints and risks into acceptance criteria and checks, +then map those checks into EVIDENCE. Do not run two parallel workflows. + ## The Loop ``` diff --git a/skills/old-coder/references/templates.md b/skills/old-coder/references/templates.md index d54ad2c..0a75957 100644 --- a/skills/old-coder/references/templates.md +++ b/skills/old-coder/references/templates.md @@ -65,7 +65,10 @@ scenario so the evidence report's spec→test mapping is mechanical. confidence downgraded; spec is the artifact to review after the fact> - Source state: — persist the computation as a script (e.g. tools/source_state.sh); a hash recipe written - in prose is working-directory-sensitive and will fail to reproduce + in prose is working-directory-sensitive and will fail to reproduce. When + Git exists, derive the tree hash from version-controlled inputs, fail on + relevant staged, unstaged, deleted, or non-ignored untracked files, and + never hash ambient ignored build artifacts - Toolchain: - Entry point: - Independent verification: