diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 020882b..d1b9559 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,7 +16,7 @@ jobs: - name: Bash syntax + shellcheck run: | sudo apt-get install -y -qq shellcheck >/dev/null - files="skills/delegate-kit/hooks/*.sh skills/delegate-kit/scripts/agent-wt" + files="skills/delegate-kit/hooks/*.sh skills/delegate-kit/scripts/agent-wt bench/seeded-review/run.sh bench/ledger-stats.sh" for f in $files; do bash -n "$f"; done shellcheck -S warning $files - name: Node syntax diff --git a/README.md b/README.md index 7bc9050..21fd8d1 100644 --- a/README.md +++ b/README.md @@ -1,76 +1,95 @@
-# delegate-kit + + + delegate-kit, the overseer + -**Your coding agent picks the shape of a task before doing it, hands the big ones to fresh workers, and gets the result reviewed by someone who did not write it — the other model family whenever its CLI is installed.** +# delegate-kit -One skill for Claude Code, T3 Code and Codex CLI. Your logins, your subscriptions, no new harness. +**Claude writes it. Codex reads it. Or the other way round.**
+One session, the subscriptions you already pay for, no API keys. +[![CI](https://github.com/tomastaker/delegate-kit/actions/workflows/ci.yml/badge.svg)](https://github.com/tomastaker/delegate-kit/actions/workflows/ci.yml) [![License: MIT](https://img.shields.io/badge/License-MIT-green.svg)](LICENSE) -[![Claude Code](https://img.shields.io/badge/Claude%20Code-coordinator%20%7C%20worker-blueviolet)](https://claude.com/claude-code) -[![Codex CLI](https://img.shields.io/badge/Codex%20CLI-coordinator%20%7C%20worker-black)](https://github.com/openai/codex) -[![T3 Code](https://img.shields.io/badge/T3%20Code-coordinator-orange)](https://t3.chat) +[![Claude Code](https://img.shields.io/badge/Claude%20Code-coordinator%20%7C%20worker-e8590c)](https://claude.com/claude-code) +[![Codex CLI](https://img.shields.io/badge/Codex%20CLI-coordinator%20%7C%20worker-8250df)](https://github.com/openai/codex) +[![T3 Code](https://img.shields.io/badge/T3%20Code-coordinator-4a4a4a)](https://github.com/pingdotgg/t3code) -[Why](#why) · [How it works](#how-it-works) · [Shapes](#the-shapes) · [Review](#review) · [Install](#install) · [Usage](#usage) · [Docs](skills/delegate-kit/) +[The overseer](#the-overseer) · [Before / after](#before--after) · [Numbers](#numbers) · [How it works](#how-it-works) · [Install](#install) · [Commands](#commands) · [FAQ](#faq)
--- -## Why +## The overseer -Capable models already delegate. What they do inconsistently is *when* and *how*: a one-line fix spawns a 50k-token worker, a ten-module feature starts with no plan, three subagents share one working tree, and the author approves its own diff. delegate-kit makes one decision the same every time: +You know him. Linen kilt, whip on the hip, squinting at the plateau since before the first course of stone went down. Two gangs answer to him — one hauls for Anthropic, one for OpenAI — and he has run them long enough to hold two rules that never bend. -> **Do it here, send one scout, plan first, one worker, a few in parallel, or a sequence — and who reviews the result.** +**No gang inspects its own stones.** The gang that set the block does not check the block. The other gang does, with the plans in hand, and the tally says who looked. -| Without | With | -|---|---| -| Delegation on a whim | **Shape first**, from numbers: ≤ ~3 files → done here; prose requirements or > 1 module → a plan before code | -| Three workers on one coupled feature → merge conflicts, two designs | **Worker count = independent outcomes**; coupled edits stay in one pair of hands | -| Claude reviews Claude; Codex reviews Codex | **Independent review**: the other family when available, a fresh session otherwise — reported either way | -| Workers write into your tree and collide | **One writer per git worktree**, locked; you merge what you accept | -| A cross-vendor worker treated as a subagent, then lost | **Native inside the family, external across it** — named as such, launched once, collected once | -| One subscription runs dry, work stops | **Presets** move the heavy roles; the reviewer follows the author; a rate limit retries once on the other vendor and says so | -| "Done." Was anything checked? | **One JSON contract**: `status`, `changes`, `checks_run`, `not_verified`, `findings`, `questions` | +**You don't crack the whip for one brick.** Three files and a clear ask, he lays it himself. Ten modules and a page of prose, he draws the plan before anyone picks up a rope. Two jobs that don't share a wall, two gangs at once, each on its own stretch of the ramp. -## How it works +delegate-kit puts him in your coding session. Your Claude Code or Codex CLI session becomes the coordinator: it keeps the intent, the plan and the answer. Workers are fresh sessions of either family, started once with a full brief, isolated in git worktrees, and reviewed by the family that did not write the code. The whole thing runs on the two CLIs you already have installed and logged in. + +## Before / after -You talk to one agent — the **coordinator**. It keeps the intent, the plan, every brief, integration and the answer. Workers are fresh sessions that start from a brief and read the repository themselves. +You type the same sentence either way: + +> change the tariff logic in the billing module — the rules are in `docs/tariffs.md` + +**Without him.** The model reads a bit, edits five files in your working tree, runs whatever tests it remembers, and says *done*. If it spawned helpers, two of them edited the same file. The only review is the author reading its own diff, which finds what it already thought of. + +**With him.** ``` -you ──► coordinator "change the tariff logic in the billing module" - │ - │ shape? DIRECT → done here. otherwise: - │ spec → .scratch/tariffs/spec.md - ▼ - planner ──────────► strongest model, read-only ──────► steps, risks, blocking questions - │ - agent-wt create tariffs git worktree + write-lock - │ - implementer ──────► native subagent or external CLI ─► commits on dk/tariffs - │ - agent-wt diff tariffs the frozen diff - agent-run route --role reviewer single | panel | led · family · cost - │ - reviewer ─────────► the OTHER family if its CLI is installed, - │ else a fresh session of the same family ─► findings - │ - │ mechanical → coordinator fixes · substantive → same implementer, resumed - │ dispute → a command first, then a verifier - ▼ - merge / PR · report: done · checked · not checked · open · who reviewed +shape: PLAN — prose requirements, 2 modules, 11 files +spec → .scratch/tariffs/spec.md (you said yes to 6 lines) +plan → planner, read-only, strongest model (4 steps, 1 blocking question — asked) +build → implementer on Codex, worktree dk/tariffs, committed +review → Claude, other family, read-only (2 findings: 1 spec, 1 correctness) +fix → same implementer, resumed (both closed, tests re-run) +tally → done · checked: pnpm test, tsc · not checked: prod migration · reviewed by Claude, single ``` -Two kinds of worker, never confused: +Your tree was never touched. You merge the branch when you agree with the tally. + +The other direction works the same: when Claude implements, Codex reads the diff. When you wrote the diff yourself in the session, he still sends it across the ramp — a change the coordinator made is `self`, never "independent by default". + +## Numbers -| | Native | External | +**From the author's own ledger.** Every external worker is recorded in `~/.delegate-kit/ledger.jsonl`. This is the author's, 20 August to 1 September 2026: ten repositories including a production monorepo, every run counted, nothing excluded but the synthetic tests. + +| | Codex reading Claude | Claude reading Codex | Together | +|---|---|---|---| +| Cross-family reviews of real diffs | 29 | 17 | **46** | +| … that came back with at least one **high**-severity finding | 12 | 5 | **17** | +| … that came back with nothing | 1 | 0 | 1 | +| Median findings per review | 4 | 6 | | +| Median wall-clock per review | 6 min | 9 min | | + +Around it: 108 external workers in those twelve days, 39 finished implementer runs at a median of 13 minutes each, 6 planner runs at a median of 7. + +The findings are what the reviewer returned, not what survived adjudication: the coordinator closes the mechanical ones, the implementer the substantive ones, disputes go to a command first. So the table says something narrower than "bugs prevented", and still worth the second session: on a real diff, the family that did not write it comes back with something serious about one time in three. `bench/ledger-stats.sh` prints the same table from your own ledger. + +**Mechanics that hold under load.** The scripts are tested without any model calls, so the numbers are cheap to reproduce: `bash tests/.sh` in `skills/delegate-kit/`. + +| Suite | Checks | What it proves | |---|---|---| -| What | a subagent of the host, same family as the coordinator | a headless `claude -p` / `codex exec` session of the other family, via `agent-run` | -| Steerable | yes — visible, continuable | no — launched once with a full brief, collected once, resumed by id | -| Why | cheap, default inside the family | the only way to get the other family; also an enforced sandbox, a ledger, timeouts, quota fallback | -| Docs | [`references/hosts.md`](skills/delegate-kit/references/hosts.md) | [`references/external.md`](skills/delegate-kit/references/external.md) | +| `caps.sh` | 47 | writer cap, ceiling, flag and env precedence, native locks counted, refusal under `--detach` | +| `route.sh` | 52 | who runs which role; the reviewer is never the author's family when the other CLI exists | +| `gate.sh` | 27 | dangerous commands stop; a worker cannot start workers | +| `inspect.sh` | 13 | a crashed writer's commits and dirty files reach its replacement | +| `delivery.sh` | 25 | completion hooks fire exactly once; `--race 20` runs 20 rounds × 8 processes: 0 duplicates, 0 lost | + +## How it works -## The shapes + + + shape → build in worktrees → the other family reviews → tally + + +**1 · Shape.** Before anything else the coordinator names the shape of the task, from numbers, not mood: | Shape | When | Who | |---|---|---| @@ -81,16 +100,16 @@ Two kinds of worker, never confused: | **PARALLEL** | 2+ slices with disjoint write scopes and stable interfaces | one implementer per slice, each in a worktree | | **SEQUENTIAL** | one result changes the next task's assumptions (schema → API → UI) | one worker at a time, resumed | -Limits: writer cap 3, ceiling 8, workers = writers + 3, delegation depth 1. Raising the cap is a per-task decision: the coordinator names the partition (one ticket per writer, disjoint write scopes), you say yes, and `--max-writers N` carries it. Repository size changes the cost of *finding* context, not the number of writers. +Worker count equals the number of independent outcomes. Coupled edits stay in one pair of hands; split, they come back as merge conflicts and two designs. -## Review +**2 · Build.** Every writer gets a git worktree with a lock. Native workers (a subagent of your host, same family, steerable) and external workers (a headless `claude -p` / `codex exec` of the other family, launched once, collected once) are named as such and never confused. Three writers run at once by default; the ceiling is eight, and between the two he states the partition and waits for your yes. -The author of a non-trivial change does not certify it. The reviewer is a fresh, read-only worker with the frozen diff and the spec — and independence has an order of preference, not a hard requirement: +**3 · Review.** The author of a non-trivial change does not certify it. Independence has an order of preference, not a hard requirement: -1. **The other family than the author.** Picked whenever that CLI is installed. Claude wrote it → Codex reads it, and the reverse; a change the coordinator wrote itself goes to the other vendor too. -2. **A fresh session of the author's family.** When the other CLI is missing, `agent-run route` falls back to it, marks `independent: false`, and the report says which kind ran. +1. **The other family than the author**, whenever its CLI is installed. +2. **A fresh session of the author's family** when it is not — marked `independent: false`, and the tally says so. -Depth is measured from the diff: +Depth is measured from the diff, and anything beyond one reviewer is proposed with the numbers and run on your yes: | Depth | Reviewers | When | |---|---|---| @@ -98,57 +117,80 @@ Depth is measured from the diff: | `panel` | two, parallel and blind to each other | large diff, several modules, or a risk zone | | `led` | a lead plans, three review, the lead merges | very large or risky | -Lenses: `spec` · `correctness` · `standards`. A panel is **proposed with the numbers and run on your yes**. Rules, lenses, merge and the smell baseline: [`references/review.md`](skills/delegate-kit/references/review.md). +Findings go back to whoever can close them: mechanical ones the coordinator fixes, substantive ones return to the same implementer, resumed with its context. A dispute is settled by a command first — a test, a typecheck, a grep — and only then by a verifier. + +**4 · Tally.** Every worker returns one JSON object: `status`, `changes`, `checks_run`, `not_verified`, `findings`, `questions`. The coordinator's report is built from it: done, checked, not checked, open, which family reviewed at which depth. A check that did not run is reported as not run. A worker's "done" is evidence to inspect, not a verdict. + +**Limits he keeps.** Writer cap 3, ceiling 8, total workers = writers + 3, delegation depth 1 — a worker never starts workers, and the safety hook denies it if one tries. When a subscription hits its limit mid-run, the writer is retried once on the other family with a note about the commits the first one left; it continues, it does not start over. ## Install -**Prerequisites.** The policy and native workers need only the host. Workers from the other family need that CLI installed and logged in with your own account, plus `bash`, `git`, `jq`, `node ≥ 20`. +**Prerequisites.** The policy and native workers need only your host. Workers from the other family need that family's CLI installed and logged in, plus `bash`, `git`, `jq`, `node ≥ 20`. ```bash -# 1. the skill npx skills add tomastaker/delegate-kit -# or by hand: +``` + +or by hand: + +```bash git clone https://github.com/tomastaker/delegate-kit ~/dev/delegate-kit ln -s ~/dev/delegate-kit/skills/delegate-kit ~/.agents/skills/delegate-kit -ln -s ../../.agents/skills/delegate-kit ~/.claude/skills/delegate-kit -ln -s ../../.agents/skills/delegate-kit ~/.codex/skills/delegate-kit +ln -s ../../.agents/skills/delegate-kit ~/.claude/skills/delegate-kit # Claude Code +ln -s ../../.agents/skills/delegate-kit ~/.codex/skills/delegate-kit # Codex CLI +``` + +Then the native role definitions and the safety hook. The installer backs up your settings and shows the diff first: -# 2. native role definitions + safety hook (backs up your settings, shows the diff first) +```bash ~/.agents/skills/delegate-kit/hooks/install.sh --dry-run ~/.agents/skills/delegate-kit/hooks/install.sh # --claude / --codex / --hooks-only / --agents-only +``` + +Optional, for driving the scripts yourself: -# 3. optional: scripts on PATH +```bash echo 'export PATH="$HOME/.agents/skills/delegate-kit/scripts:$PATH"' >> ~/.zshrc ``` Restart running `claude` / `codex` sessions. The symlinks keep it live: `git pull` is the update; re-run `install.sh` after a change to the roles. -**Always-on.** The skill triggers on its own words (below). To have it triage every non-trivial task, add one line to your global instructions: +**Hosts.** Claude Code and Codex CLI act as coordinator and as worker. [T3 Code](https://github.com/pingdotgg/t3code) acts as coordinator over either CLI; its workers are the same two families. + +**Always on.** The skill triggers on its own words (see [Talking to him](#talking-to-him)). To have it triage every non-trivial task, add one line to your global instructions: ```text Before repository work described in prose or spanning several modules, apply delegate-kit; a DIRECT verdict needs no announcement. ``` -**Uninstall.** `hooks/uninstall.sh`, remove the three symlinks, `rm -rf ~/.delegate-kit` if you do not want to keep the ledger and run logs. +**Uninstall.** `hooks/uninstall.sh`, remove the three symlinks, and `rm -rf ~/.delegate-kit` if you do not want to keep the ledger and run logs. -## Usage +## Commands -Work as usual. Invoke explicitly with `/delegate-kit` (Claude Code) or `$delegate-kit` (Codex), or just say it: +Work as usual; he triggers on his own words. When you want to drive a step by hand, the two scripts are the whole surface. `--help` on either is the flag reference. -| You say | Effect | +| Command | Does | |---|---| -| "delegate", "subagent", "scout", "plan this", "review this", "second opinion" | the skill triggers | -| a feature or refactor described in prose; "which should we adopt?" | the skill triggers — PLAN | -| "let Codex implement", "main model Claude", `main-claude` / `main-codex` | preset for this task: heavy roles move, the reviewer follows the author | -| "plan with Fable", "review with Sol" | one model for one role; the preset is untouched | -| "panel", "two reviewers", "led" | your yes to a deeper review, given in advance | -| "this is mechanical" / "a refactor" / "UI work" | `--kind`: always `single` / lenses `correctness`+`standards` / implementer on Claude under `auto` | +| `agent-run route --role [--preset P]` | who runs this role, on which family, natively or externally | +| `agent-run route --role reviewer --diff FILE --author-backend claude\|codex\|self` | depth, lenses, family and cost of a review; `self` when you wrote the diff | +| `agent-run run --role --brief FILE [--backend B] [--cwd WORKTREE] [--detach]` | start a worker; `--detach` for parallel writers | +| `agent-run resume --prompt TEXT` | continue a worker with its context (fix findings, answer questions) | +| `agent-run list \| status \| wait \| kill \| log ` | the ledger | +| `agent-run inspect [WORKTREE]` | what a stopped writer left: commits since base, dirty files | +| `agent-run preset [auto\|main-claude\|main-codex]` | which family carries the heavy roles; alone, shows the effective table | +| `agent-wt create \| diff \| lock \| release \| remove ` | a writer's worktree, its frozen diff, and the lock the cap counts | -**Decides alone:** the shape; family, model, effort and native-vs-external per role; the reviewer; review depth from the diff; a command before a verifier; one cross-vendor retry on a rate limit. - -**Stops and asks:** before a panel (with the numbers); when a worker returns `blocked` with questions; when your working tree is dirty and the task touches it; before any command the safety gate classes as dangerous. +A typical hand-driven pass: -If you interview yourself first (a grill skill, a spec session), save the outcome as `.scratch//spec.md` — workers and the reviewer read that file, not a retelling. +```bash +agent-run run --role planner --brief .scratch/tariffs/brief.md +agent-wt create tariffs +agent-run run --role implementer --backend codex --cwd ../repo.worktrees/tariffs --brief .scratch/tariffs/impl.md +agent-wt diff tariffs > .scratch/tariffs/review.diff +agent-run route --role reviewer --diff .scratch/tariffs/review.diff --author-backend codex +agent-run run --role reviewer --backend claude --cwd ../repo.worktrees/tariffs --brief .scratch/tariffs/review.md +agent-run resume --prompt "Fix findings 1 and 3: ..." +```
The roles @@ -157,7 +199,7 @@ If you interview yourself first (a grill skill, a spec session), save the outcom |---|---|---|---| | **planner** | ordered steps, files per step, risks, blocking questions, the checks that prove completion | Claude Fable high · Codex Sol xhigh | read-only | | **implementer** | one vertical slice in its own worktree; runs the acceptance checks; commits on its branch | Codex Sol high · Claude Opus high for UI | write, sandboxed | -| **reviewer** | reads the frozen diff against the spec; findings with severity, lens, file:line, evidence, fix | the other family than the author when available | read-only | +| **reviewer** | reads the frozen diff against the spec; findings with severity, lens, file:line, evidence, fix | the other family than the author | read-only | | **review-lead** | plans a `led` review before, merges findings after; two short calls | the planner's family, strongest | read-only | | **verifier** | settles one disputed or high-risk finding that a command cannot | third party to the reviewer | read-only | | **researcher** | quotes current docs with URL and date; marks what it could not verify | Claude Sonnet medium · Codex Terra medium | read-only, web | @@ -166,33 +208,12 @@ Presets move planner, implementer and researcher; the reviewer follows the autho
-
-Driving the scripts by hand - -```bash -agent-run route --role implementer --preset main-claude # who runs this, where, how -agent-run route --role reviewer --diff review.diff --author-backend codex # depth, family, cost; self when you wrote it -agent-run preset main-claude # persist; alone: show the effective table - -agent-run run --role planner --brief .scratch/tariffs/brief.md -agent-wt create tariffs -agent-run run --role implementer --backend codex --cwd ../repo.worktrees/tariffs --brief .scratch/tariffs/impl.md -agent-wt diff tariffs > .scratch/tariffs/review.diff -agent-run run --role reviewer --backend claude --cwd ../repo.worktrees/tariffs --brief .scratch/tariffs/review.md -agent-run resume --prompt "Fix findings 1 and 3: ..." -agent-run list | status | wait | kill | log | notify [] -``` - -Several writers in parallel: `--detach` on each, then `agent-run wait ` or `--on-finish CMD` so each reports for itself. A native writer: `agent-wt lock ` before dispatch, `agent-wt release ` after. Both commands refuse a writer past the cap (`--max-writers N` or `DELEGATE_KIT_MAX_WRITERS` raise it, up to the ceiling of 8); a refused `--detach` run is reported to the parent. `--help` on either script is the flag reference. - -
-
Layout ``` skills/delegate-kit/ - SKILL.md the policy: shapes, spec, route, brief, worktree, review, report + SKILL.md the policy: shape, spec, route, brief, worktree, review, report references/hosts.md native dispatch per host; git with parallel writers references/external.md the other family through agent-run: preflight, collecting, presets, limits references/roles.md families, tiers, reasoning per role, prompt hints @@ -201,23 +222,67 @@ skills/delegate-kit/ references/result-schema.json the JSON contract references/codex-agents.toml the six roles as native Codex subagents agents/dk-*.md the six roles as native Claude Code subagents - scripts/agent-run route / preset / run / resume / list / status / wait / kill / log / notify + scripts/agent-run route / preset / run / resume / list / status / wait / kill / log / notify / inspect scripts/agent-wt create / list / status / diff / lock / release / remove / cleanup hooks/gate.sh, install.sh, uninstall.sh - tests/delivery.sh completion-delivery bench; --race N for the concurrency hammer + tests/*.sh caps, route, gate, inspect, delivery — synthetic, no model calls +bench/ledger-stats.sh the Numbers table, from your own ledger +bench/seeded-review/ a small planted-defect harness for reviewer experiments (pilot results inside) ```
-## What it will not do +## Talking to him + +| You say | He does | +|---|---| +| "delegate", "subagent", "scout", "plan this", "review this", "second opinion" | picks a shape and says which | +| a feature or refactor described in prose; "which should we adopt?" | PLAN | +| "let Codex implement", "main model Claude", `main-claude` / `main-codex` | moves the heavy roles to that family for this task; the reviewer still follows the author | +| "plan with Fable", "review with Sol" | one model for one role; the preset is untouched | +| "panel", "two reviewers", "led" | your yes to a deeper review, given in advance | +| "this is mechanical" / "a refactor" / "UI work" | `--kind`: always `single` / lenses `correctness`+`standards` / implementer on Claude under `auto` | + +**Decides alone:** the shape; family, model, effort and native-vs-external per role; the reviewer; review depth from the diff; a command before a verifier; one cross-vendor retry on a rate limit. + +**Stops and asks:** before a panel (with the numbers); before a fourth writer (with the partition); when a worker returns `blocked` with questions; when your working tree is dirty and the task touches it; before any command the safety gate classes as dangerous. + +If you interview yourself first (a grill skill, a spec session), save the outcome as `.scratch//spec.md`. Workers and the reviewer read that file, not a retelling. + +## What he won't do + +- **Steer an external worker mid-run.** Headless sessions run to completion; you read the result and resume. That is why native is preferred inside the family, and why an external worker is never called a subagent. +- **Run a swarm.** Writer cap 3, ceiling 8, and the ceiling holds against every flag. Between the two it takes a stated partition and your yes. +- **Make delegation cheap.** A worker is a full session. The shapes exist so you pay for one only for independence, parallelism or a clean context. +- **Sandbox by himself.** The gate is a list of dangerous command shapes; the real isolation is the CLIs' own sandboxes plus the worktree. +- **Route other people's subscriptions.** Workers run through the official CLIs with the logins you already have — ordinary use of Claude Code and Codex. Do not wrap this into a product that does otherwise. + +## FAQ + +**I only have one of the two subscriptions. Is this for me?** +Less so. You still get the shapes, the worktrees, the caps and the report, and review falls back to a fresh session of your own family, honestly labelled. But the reason to install this over a good single-family workflow such as [Superpowers](https://github.com/obra/superpowers) is the second family. Without it, Superpowers is the more mature choice. + +**How is this different from Claude Code agent teams?** +Agent teams are Claude talking to Claude: teammates share a task list and message each other. delegate-kit is about the *other* family reading your diff, about one writer per worktree, and about a report that names what was not checked. They are not exclusive; the overseer does not care how a native worker was spawned, only that it stayed on its own stretch of the ramp. + +**Why not an API key and a multi-model MCP server?** +Because you already pay for two CLIs with their own sandboxes, rate limits and logins. `claude -p` and `codex exec` are official, headless, and free with the subscription. The price is that an external worker cannot be steered mid-run, which the shapes are designed around. + +**What does a worker cost?** +A full session: system prompt, project instructions, the files it reads. Tens of thousands of tokens before it does anything. That is why ≤ 3 files is DIRECT, why effort is raised before model tier, and why the reviewer gets a frozen diff instead of the whole repository. + +**What if the other CLI is missing?** +`agent-run route` says so, picks a fresh session of the author's family, marks it `independent: false`, and the report repeats it. Nothing pretends. + +**Can a worker start its own workers?** +No. Delegation depth is one. The safety hook denies `agent-run run` and `agent-wt lock` when the caller is a subagent. -- **Steer an external worker mid-run** — headless sessions run to completion; you read the result and resume. That is why native is preferred inside the family, and why an external worker is never called a subagent. -- **Run a swarm** — writer cap 3, ceiling 8, and the ceiling holds against every flag. Between the two it takes a stated partition and your yes, passed as `--max-writers N`. -- **Make delegation cheap** — a worker is a full session; the shapes exist so you pay for it only for independence, parallelism or a clean context. -- **Sandbox by itself** — the gate is a list of dangerous command shapes; the real isolation is the CLIs' own sandboxes plus the worktree. +**Does it work in T3 Code?** +As coordinator, yes: T3 Code drives Claude Code or Codex, and the skill runs in that session. Workers are still the two CLIs. -Workers run through the official CLIs with the logins you already have — ordinary use of Claude Code and Codex. Do not wrap this into a product that routes other people's subscriptions. +**Is it safe to let it run commands?** +The hook (`hooks/gate.sh`) stops the dangerous shapes — deletes, force pushes, secrets, production hosts — and asks you. Writers are confined to a worktree and to the CLI's own sandbox. The gate is a list, not a sandbox; read it before you trust it. ## License and acknowledgments -MIT. The review lenses `spec` and `standards` and the smell baseline are adapted from [mattpocock/skills — code-review](https://github.com/mattpocock/skills/blob/main/skills/engineering/code-review/SKILL.md) (MIT). The host dispatch table and git-coordination facts borrow from [Hyperskills](https://github.com/hyperb1iss/hyperskills); one worker per independent outcome from [Superpowers](https://github.com/obra/superpowers). README structure after [Best-README-Template](https://github.com/othneildrew/Best-README-Template). +MIT. The review lenses `spec` and `standards` and the smell baseline are adapted from [mattpocock/skills — code-review](https://github.com/mattpocock/skills/blob/main/skills/engineering/code-review/SKILL.md) (MIT). The host dispatch table and git-coordination facts borrow from [Hyperskills](https://github.com/hyperb1iss/hyperskills); one worker per independent outcome from [Superpowers](https://github.com/obra/superpowers). The README voice owes a debt to [ponytail](https://github.com/DietrichGebert/ponytail). diff --git a/assets/how-it-works-dark.svg b/assets/how-it-works-dark.svg new file mode 100644 index 0000000..6595486 --- /dev/null +++ b/assets/how-it-works-dark.svg @@ -0,0 +1,77 @@ + + + + + + + + + + 1 · SHAPE + + The overseer + your session, one context + ≤ 3 files → does it himself + prose, > 1 module → plan first + one slice → one gang + disjoint slices → gangs at once + schema → API → UI, in turn + 3 writers by default, 8 at most, + each in its own worktree + + + + + 2 · BUILD + + + Claude gang + worktree dk/tariffs + native subagent, steerable + commits on its branch + + + + Codex gang + worktree dk/invoices + headless codex exec, sandboxed + commits on its branch + + + + + + frozen diff + + + 3 · REVIEW + + + Claude reads + what Codex wrote + read-only, fresh session + findings with file:line + + + + Codex reads + what Claude wrote + read-only, fresh session + findings with file:line + + + + + 4 · TALLY + + The tally + ✓ done + ✓ checked + ✗ not checked + ? open + who reviewed + you merge what + you accept + + One gang short? The other family's fresh session reviews instead, and the tally says so. + diff --git a/assets/how-it-works-light.svg b/assets/how-it-works-light.svg new file mode 100644 index 0000000..04c54ca --- /dev/null +++ b/assets/how-it-works-light.svg @@ -0,0 +1,77 @@ + + + + + + + + + + 1 · SHAPE + + The overseer + your session, one context + ≤ 3 files → does it himself + prose, > 1 module → plan first + one slice → one gang + disjoint slices → gangs at once + schema → API → UI, in turn + 3 writers by default, 8 at most, + each in its own worktree + + + + + 2 · BUILD + + + Claude gang + worktree dk/tariffs + native subagent, steerable + commits on its branch + + + + Codex gang + worktree dk/invoices + headless codex exec, sandboxed + commits on its branch + + + + + + frozen diff + + + 3 · REVIEW + + + Claude reads + what Codex wrote + read-only, fresh session + findings with file:line + + + + Codex reads + what Claude wrote + read-only, fresh session + findings with file:line + + + + + 4 · TALLY + + The tally + ✓ done + ✓ checked + ✗ not checked + ? open + who reviewed + you merge what + you accept + + One gang short? The other family's fresh session reviews instead, and the tally says so. + diff --git a/assets/logo-dark.png b/assets/logo-dark.png new file mode 100644 index 0000000..0d36279 Binary files /dev/null and b/assets/logo-dark.png differ diff --git a/assets/logo.png b/assets/logo.png new file mode 100644 index 0000000..596dac0 Binary files /dev/null and b/assets/logo.png differ diff --git a/bench/ledger-stats.sh b/bench/ledger-stats.sh new file mode 100755 index 0000000..5cf61e3 --- /dev/null +++ b/bench/ledger-stats.sh @@ -0,0 +1,42 @@ +#!/bin/bash +# Prints the README "Numbers" table from your own delegate-kit ledger: every +# external run under $DELEGATE_KIT_HOME/runs (default ~/.delegate-kit/runs). +# +# bench/ledger-stats.sh [--exclude REGEX] drop runs whose cwd matches REGEX +# +# Findings are counted as the reviewer returned them, not as adjudicated. +set -euo pipefail +EXCLUDE="" +[ "${1:-}" = "--exclude" ] && EXCLUDE="${2:-}" +export EXCLUDE +node - <<'EOF' +const fs = require("fs"), path = require("path"); +const home = process.env.DELEGATE_KIT_HOME || path.join(process.env.HOME, ".delegate-kit"); +const dir = path.join(home, "runs"); +const exclude = process.env.EXCLUDE ? new RegExp(process.env.EXCLUDE) : null; +const rows = []; +for (const id of fs.existsSync(dir) ? fs.readdirSync(dir) : []) { + let m; try { m = JSON.parse(fs.readFileSync(path.join(dir, id, "meta.json"), "utf8")); } catch { continue; } + if (exclude && exclude.test(m.cwd || "")) continue; + const f = (m.result && m.result.findings) || []; + const dur = m.started && m.finished ? (new Date(m.finished) - new Date(m.started)) / 60000 : null; + rows.push({ role: m.role, backend: m.backend, status: m.status, cwd: m.cwd, started: m.started, dur, findings: f.length, high: f.filter((x) => x.severity === "high").length }); +} +const median = (a) => { a = a.filter((x) => x != null).sort((x, y) => x - y); return a.length ? a[Math.floor(a.length / 2)] : null; }; +const fmt = (v, unit = "") => (v == null ? "" : `${Math.round(v)}${unit}`); +const rev = (b) => rows.filter((r) => r.role === "reviewer" && r.status === "finished" && r.backend === b); +const cx = rev("codex"), cl = rev("claude"), all = [...cx, ...cl]; +const dates = rows.map((r) => r.started).filter(Boolean).sort(); +const repos = new Set(rows.map((r) => r.cwd)); +console.log(`Runs: ${rows.length} across ${repos.size} working directories, ${dates[0]?.slice(0, 10) ?? "?"} to ${dates.at(-1)?.slice(0, 10) ?? "?"}\n`); +console.log("| | Codex reading Claude | Claude reading Codex | Together |"); +console.log("|---|---|---|---|"); +console.log(`| Cross-family reviews of real diffs | ${cx.length} | ${cl.length} | **${all.length}** |`); +console.log(`| … that came back with at least one **high**-severity finding | ${cx.filter((r) => r.high).length} | ${cl.filter((r) => r.high).length} | **${all.filter((r) => r.high).length}** |`); +console.log(`| … that came back with nothing | ${cx.filter((r) => !r.findings).length} | ${cl.filter((r) => !r.findings).length} | ${all.filter((r) => !r.findings).length} |`); +console.log(`| Median findings per review | ${fmt(median(cx.map((r) => r.findings)))} | ${fmt(median(cl.map((r) => r.findings)))} | |`); +console.log(`| Median wall-clock per review | ${fmt(median(cx.map((r) => r.dur)), " min")} | ${fmt(median(cl.map((r) => r.dur)), " min")} | |`); +const imp = rows.filter((r) => r.role === "implementer" && r.status === "finished"); +const pl = rows.filter((r) => r.role === "planner" && r.status === "finished"); +console.log(`\nImplementer runs finished: ${imp.length}, median ${fmt(median(imp.map((r) => r.dur)), " min")}. Planner runs: ${pl.length}, median ${fmt(median(pl.map((r) => r.dur)), " min")}.`); +EOF diff --git a/bench/seeded-review/PLANTED.md b/bench/seeded-review/PLANTED.md new file mode 100644 index 0000000..584e42a --- /dev/null +++ b/bench/seeded-review/PLANTED.md @@ -0,0 +1,14 @@ +# Planted defects + +Two per diff. A reviewer "catches" one when a finding names the line and the mechanism. + +| Diff | # | Where | Defect | Kind | +|---|---|---|---|---| +| 01-pagination | 1 | `totalPages` | `floor(n/size)+1` instead of `ceil`: one page too many when `n` is a multiple of `size` | off-by-one | +| 01-pagination | 2 | cursor decode | a decodable cursor with a negative, fractional or out-of-range `offset` is accepted; spec demands `InvalidCursor` | missing validation | +| 02-ratelimit | 3 | `key.toLowerCase()` | keys are opaque and case-sensitive by spec; distinct keys share one counter | spec violation | +| 02-ratelimit | 4 | `retryAfterMs` | constant `windowMs` instead of time until the oldest hit leaves the window | spec violation | +| 03-money | 5 | `allocate` | independent `Math.round` per party: parts do not sum to `total` | correctness | +| 03-money | 6 | `add` | floats and strings accepted; spec demands `TypeError` on non-integers | missing validation | +| 04-authz | 7 | `isExpired` | `exp` in seconds compared with `Date.now()` in milliseconds | units | +| 04-authz | 8 | `can` | substring match on the joined roles: `"superadmin"` grants `admin`, `"editor"` grants `edit` | substring match | diff --git a/bench/seeded-review/README.md b/bench/seeded-review/README.md new file mode 100644 index 0000000..80d1f22 --- /dev/null +++ b/bench/seeded-review/README.md @@ -0,0 +1,20 @@ +# Seeded-review bench + +Four small diffs, two planted defects each, reviewed by a Claude reviewer and a Codex reviewer through `agent-run`. The question is whether a second family adds findings, and at what cost. + +| File | Role | +|---|---| +| `diffs/NN-.diff` | the change under review: source, spec, passing tests | +| `PLANTED.md` | the eight defects and where they sit | +| `brief.md` | the reviewer brief; `run.sh` fills the paths | +| `base-*` | the three files of the base commit the diffs apply to | +| `run.sh` | builds a throwaway repo, launches the eight reviews, collects `result.json` per run | +| `results//` | raw results and a hand-scored `SCORE.md` | + +Run it: + +```bash +bench/seeded-review/run.sh +``` + +Eight headless runs of two to three minutes each, in parallel, on your own subscriptions. Both CLIs must be installed and logged in. Score by hand against `PLANTED.md`; test-coverage remarks do not count, since every diff ships with tests that miss its defects by construction. diff --git a/bench/seeded-review/base-README.md b/bench/seeded-review/base-README.md new file mode 100644 index 0000000..c271dd2 --- /dev/null +++ b/bench/seeded-review/base-README.md @@ -0,0 +1,3 @@ +# shoplib +Small shared library for the shop backend. Plain ESM, no dependencies. `npm test` runs node:test. +Amounts are integers in minor units (cents). Keys and ids are opaque strings. diff --git a/bench/seeded-review/base-errors.js b/bench/seeded-review/base-errors.js new file mode 100644 index 0000000..cae2822 --- /dev/null +++ b/bench/seeded-review/base-errors.js @@ -0,0 +1 @@ +export class ShopError extends Error { constructor(code, msg) { super(msg); this.code = code; } } diff --git a/bench/seeded-review/base-package.json b/bench/seeded-review/base-package.json new file mode 100644 index 0000000..6c3b0c9 --- /dev/null +++ b/bench/seeded-review/base-package.json @@ -0,0 +1 @@ +{ "name": "shoplib", "version": "0.1.0", "type": "module", "scripts": { "test": "node --test" } } diff --git a/bench/seeded-review/brief.md b/bench/seeded-review/brief.md new file mode 100644 index 0000000..8d82127 --- /dev/null +++ b/bench/seeded-review/brief.md @@ -0,0 +1,17 @@ +# Task: review the __NAME__ change + +## Goal +Find every defect in this change before it merges: behaviour that contradicts the spec, incorrect logic, and anything that would break a caller. + +## Spec +__WT__/spec/__NAME__.md + +## Diff +__DIFF__ — the complete change (source, spec, tests). The worktree at __WT__ has it checked out; `npm test` runs its tests. + +## Constraints +- Read-only: do not modify files. +- Scope is the diff. Do not restate it. + +## Return +The delegate-kit result JSON. Return `findings` with severity (`high` | `medium` | `low`), `file`, `line`, `claim`, `evidence`, `suggested_fix`, and `kind` (`spec` | `correctness` | `standards` | `nit`). diff --git a/bench/seeded-review/diffs/01-pagination.diff b/bench/seeded-review/diffs/01-pagination.diff new file mode 100644 index 0000000..4610228 --- /dev/null +++ b/bench/seeded-review/diffs/01-pagination.diff @@ -0,0 +1,49 @@ +diff --git a/spec/pagination.md b/spec/pagination.md +new file mode 100644 +index 0000000..7e84526 +--- /dev/null ++++ b/spec/pagination.md +@@ -0,0 +1,7 @@ ++# Cursor pagination ++- `page(items, {size, cursor})` returns `{items, nextCursor, totalPages}`. ++- `size` is an integer 1..100; anything else throws ShopError("InvalidSize"). ++- `totalPages` = number of pages needed for `items.length` at `size` (0 items → 0 pages). ++- `cursor` is base64url of JSON `{"offset": n}`; absent cursor means offset 0. ++- A cursor that does not decode to an integer offset within `0..items.length` throws ShopError("InvalidCursor"). ++- `nextCursor` is null on the last page. +diff --git a/src/pagination.js b/src/pagination.js +new file mode 100644 +index 0000000..a108a1c +--- /dev/null ++++ b/src/pagination.js +@@ -0,0 +1,19 @@ ++import { ShopError } from "./errors.js"; ++ ++const enc = (o) => Buffer.from(JSON.stringify(o)).toString("base64url"); ++const dec = (c) => JSON.parse(Buffer.from(c, "base64url").toString("utf8")); ++ ++export function page(items, { size, cursor } = {}) { ++ if (!Number.isInteger(size) || size < 1 || size > 100) throw new ShopError("InvalidSize", `size ${size}`); ++ let offset = 0; ++ if (cursor != null) { ++ try { offset = dec(cursor).offset; } catch { throw new ShopError("InvalidCursor", "undecodable"); } ++ } ++ const slice = items.slice(offset, offset + size); ++ const end = offset + size; ++ return { ++ items: slice, ++ nextCursor: end < items.length ? enc({ offset: end }) : null, ++ totalPages: items.length === 0 ? 0 : Math.floor(items.length / size) + 1, ++ }; ++} +diff --git a/test/pagination.test.js b/test/pagination.test.js +new file mode 100644 +index 0000000..67c8c56 +--- /dev/null ++++ b/test/pagination.test.js +@@ -0,0 +1,5 @@ ++import test from "node:test"; import assert from "node:assert/strict"; ++import { page } from "../src/pagination.js"; ++test("first page", () => { const r = page([1,2,3,4,5], { size: 2 }); assert.deepEqual(r.items, [1,2]); assert.ok(r.nextCursor); }); ++test("follows cursor", () => { const a = page([1,2,3], { size: 2 }); const b = page([1,2,3], { size: 2, cursor: a.nextCursor }); assert.deepEqual(b.items, [3]); assert.equal(b.nextCursor, null); }); ++test("bad size", () => { assert.throws(() => page([], { size: 0 })); }); diff --git a/bench/seeded-review/diffs/02-ratelimit.diff b/bench/seeded-review/diffs/02-ratelimit.diff new file mode 100644 index 0000000..2e891cd --- /dev/null +++ b/bench/seeded-review/diffs/02-ratelimit.diff @@ -0,0 +1,97 @@ +diff --git a/review/01-pagination.diff b/review/01-pagination.diff +new file mode 100644 +index 0000000..4610228 +--- /dev/null ++++ b/review/01-pagination.diff +@@ -0,0 +1,49 @@ ++diff --git a/spec/pagination.md b/spec/pagination.md ++new file mode 100644 ++index 0000000..7e84526 ++--- /dev/null +++++ b/spec/pagination.md ++@@ -0,0 +1,7 @@ +++# Cursor pagination +++- `page(items, {size, cursor})` returns `{items, nextCursor, totalPages}`. +++- `size` is an integer 1..100; anything else throws ShopError("InvalidSize"). +++- `totalPages` = number of pages needed for `items.length` at `size` (0 items → 0 pages). +++- `cursor` is base64url of JSON `{"offset": n}`; absent cursor means offset 0. +++- A cursor that does not decode to an integer offset within `0..items.length` throws ShopError("InvalidCursor"). +++- `nextCursor` is null on the last page. ++diff --git a/src/pagination.js b/src/pagination.js ++new file mode 100644 ++index 0000000..a108a1c ++--- /dev/null +++++ b/src/pagination.js ++@@ -0,0 +1,19 @@ +++import { ShopError } from "./errors.js"; +++ +++const enc = (o) => Buffer.from(JSON.stringify(o)).toString("base64url"); +++const dec = (c) => JSON.parse(Buffer.from(c, "base64url").toString("utf8")); +++ +++export function page(items, { size, cursor } = {}) { +++ if (!Number.isInteger(size) || size < 1 || size > 100) throw new ShopError("InvalidSize", `size ${size}`); +++ let offset = 0; +++ if (cursor != null) { +++ try { offset = dec(cursor).offset; } catch { throw new ShopError("InvalidCursor", "undecodable"); } +++ } +++ const slice = items.slice(offset, offset + size); +++ const end = offset + size; +++ return { +++ items: slice, +++ nextCursor: end < items.length ? enc({ offset: end }) : null, +++ totalPages: items.length === 0 ? 0 : Math.floor(items.length / size) + 1, +++ }; +++} ++diff --git a/test/pagination.test.js b/test/pagination.test.js ++new file mode 100644 ++index 0000000..67c8c56 ++--- /dev/null +++++ b/test/pagination.test.js ++@@ -0,0 +1,5 @@ +++import test from "node:test"; import assert from "node:assert/strict"; +++import { page } from "../src/pagination.js"; +++test("first page", () => { const r = page([1,2,3,4,5], { size: 2 }); assert.deepEqual(r.items, [1,2]); assert.ok(r.nextCursor); }); +++test("follows cursor", () => { const a = page([1,2,3], { size: 2 }); const b = page([1,2,3], { size: 2, cursor: a.nextCursor }); assert.deepEqual(b.items, [3]); assert.equal(b.nextCursor, null); }); +++test("bad size", () => { assert.throws(() => page([], { size: 0 })); }); +diff --git a/spec/ratelimit.md b/spec/ratelimit.md +new file mode 100644 +index 0000000..dc2ae7f +--- /dev/null ++++ b/spec/ratelimit.md +@@ -0,0 +1,6 @@ ++# Sliding-window rate limiter ++- `createLimiter({limit, windowMs})` returns `hit(key, now = Date.now())`. ++- Keys are opaque, case-sensitive strings; two keys that differ in any character are different keys. ++- `hit` returns `{allowed: true}` while fewer than `limit` hits fall inside the last `windowMs`. ++- Otherwise `{allowed: false, retryAfterMs}` where `retryAfterMs` is the time until the OLDEST hit in the window leaves it (so the caller waits the minimum, not the whole window). ++- Hits outside the window are dropped from memory so state does not grow with time. +diff --git a/src/ratelimit.js b/src/ratelimit.js +new file mode 100644 +index 0000000..12a9bfd +--- /dev/null ++++ b/src/ratelimit.js +@@ -0,0 +1,14 @@ ++export function createLimiter({ limit, windowMs }) { ++ const hits = new Map(); ++ return function hit(key, now = Date.now()) { ++ const k = key.toLowerCase(); ++ const list = (hits.get(k) ?? []).filter((t) => now - t < windowMs); ++ if (list.length >= limit) { ++ hits.set(k, list); ++ return { allowed: false, retryAfterMs: windowMs }; ++ } ++ list.push(now); ++ hits.set(k, list); ++ return { allowed: true }; ++ }; ++} +diff --git a/test/ratelimit.test.js b/test/ratelimit.test.js +new file mode 100644 +index 0000000..79f2801 +--- /dev/null ++++ b/test/ratelimit.test.js +@@ -0,0 +1,4 @@ ++import test from "node:test"; import assert from "node:assert/strict"; ++import { createLimiter } from "../src/ratelimit.js"; ++test("allows up to limit", () => { const h = createLimiter({ limit: 2, windowMs: 1000 }); assert.ok(h("a", 0).allowed); assert.ok(h("a", 1).allowed); assert.equal(h("a", 2).allowed, false); }); ++test("window slides", () => { const h = createLimiter({ limit: 1, windowMs: 1000 }); h("a", 0); assert.ok(h("a", 1000).allowed); }); diff --git a/bench/seeded-review/diffs/03-money.diff b/bench/seeded-review/diffs/03-money.diff new file mode 100644 index 0000000..d262fb9 --- /dev/null +++ b/bench/seeded-review/diffs/03-money.diff @@ -0,0 +1,142 @@ +diff --git a/review/02-ratelimit.diff b/review/02-ratelimit.diff +new file mode 100644 +index 0000000..2e891cd +--- /dev/null ++++ b/review/02-ratelimit.diff +@@ -0,0 +1,97 @@ ++diff --git a/review/01-pagination.diff b/review/01-pagination.diff ++new file mode 100644 ++index 0000000..4610228 ++--- /dev/null +++++ b/review/01-pagination.diff ++@@ -0,0 +1,49 @@ +++diff --git a/spec/pagination.md b/spec/pagination.md +++new file mode 100644 +++index 0000000..7e84526 +++--- /dev/null ++++++ b/spec/pagination.md +++@@ -0,0 +1,7 @@ ++++# Cursor pagination ++++- `page(items, {size, cursor})` returns `{items, nextCursor, totalPages}`. ++++- `size` is an integer 1..100; anything else throws ShopError("InvalidSize"). ++++- `totalPages` = number of pages needed for `items.length` at `size` (0 items → 0 pages). ++++- `cursor` is base64url of JSON `{"offset": n}`; absent cursor means offset 0. ++++- A cursor that does not decode to an integer offset within `0..items.length` throws ShopError("InvalidCursor"). ++++- `nextCursor` is null on the last page. +++diff --git a/src/pagination.js b/src/pagination.js +++new file mode 100644 +++index 0000000..a108a1c +++--- /dev/null ++++++ b/src/pagination.js +++@@ -0,0 +1,19 @@ ++++import { ShopError } from "./errors.js"; ++++ ++++const enc = (o) => Buffer.from(JSON.stringify(o)).toString("base64url"); ++++const dec = (c) => JSON.parse(Buffer.from(c, "base64url").toString("utf8")); ++++ ++++export function page(items, { size, cursor } = {}) { ++++ if (!Number.isInteger(size) || size < 1 || size > 100) throw new ShopError("InvalidSize", `size ${size}`); ++++ let offset = 0; ++++ if (cursor != null) { ++++ try { offset = dec(cursor).offset; } catch { throw new ShopError("InvalidCursor", "undecodable"); } ++++ } ++++ const slice = items.slice(offset, offset + size); ++++ const end = offset + size; ++++ return { ++++ items: slice, ++++ nextCursor: end < items.length ? enc({ offset: end }) : null, ++++ totalPages: items.length === 0 ? 0 : Math.floor(items.length / size) + 1, ++++ }; ++++} +++diff --git a/test/pagination.test.js b/test/pagination.test.js +++new file mode 100644 +++index 0000000..67c8c56 +++--- /dev/null ++++++ b/test/pagination.test.js +++@@ -0,0 +1,5 @@ ++++import test from "node:test"; import assert from "node:assert/strict"; ++++import { page } from "../src/pagination.js"; ++++test("first page", () => { const r = page([1,2,3,4,5], { size: 2 }); assert.deepEqual(r.items, [1,2]); assert.ok(r.nextCursor); }); ++++test("follows cursor", () => { const a = page([1,2,3], { size: 2 }); const b = page([1,2,3], { size: 2, cursor: a.nextCursor }); assert.deepEqual(b.items, [3]); assert.equal(b.nextCursor, null); }); ++++test("bad size", () => { assert.throws(() => page([], { size: 0 })); }); ++diff --git a/spec/ratelimit.md b/spec/ratelimit.md ++new file mode 100644 ++index 0000000..dc2ae7f ++--- /dev/null +++++ b/spec/ratelimit.md ++@@ -0,0 +1,6 @@ +++# Sliding-window rate limiter +++- `createLimiter({limit, windowMs})` returns `hit(key, now = Date.now())`. +++- Keys are opaque, case-sensitive strings; two keys that differ in any character are different keys. +++- `hit` returns `{allowed: true}` while fewer than `limit` hits fall inside the last `windowMs`. +++- Otherwise `{allowed: false, retryAfterMs}` where `retryAfterMs` is the time until the OLDEST hit in the window leaves it (so the caller waits the minimum, not the whole window). +++- Hits outside the window are dropped from memory so state does not grow with time. ++diff --git a/src/ratelimit.js b/src/ratelimit.js ++new file mode 100644 ++index 0000000..12a9bfd ++--- /dev/null +++++ b/src/ratelimit.js ++@@ -0,0 +1,14 @@ +++export function createLimiter({ limit, windowMs }) { +++ const hits = new Map(); +++ return function hit(key, now = Date.now()) { +++ const k = key.toLowerCase(); +++ const list = (hits.get(k) ?? []).filter((t) => now - t < windowMs); +++ if (list.length >= limit) { +++ hits.set(k, list); +++ return { allowed: false, retryAfterMs: windowMs }; +++ } +++ list.push(now); +++ hits.set(k, list); +++ return { allowed: true }; +++ }; +++} ++diff --git a/test/ratelimit.test.js b/test/ratelimit.test.js ++new file mode 100644 ++index 0000000..79f2801 ++--- /dev/null +++++ b/test/ratelimit.test.js ++@@ -0,0 +1,4 @@ +++import test from "node:test"; import assert from "node:assert/strict"; +++import { createLimiter } from "../src/ratelimit.js"; +++test("allows up to limit", () => { const h = createLimiter({ limit: 2, windowMs: 1000 }); assert.ok(h("a", 0).allowed); assert.ok(h("a", 1).allowed); assert.equal(h("a", 2).allowed, false); }); +++test("window slides", () => { const h = createLimiter({ limit: 1, windowMs: 1000 }); h("a", 0); assert.ok(h("a", 1000).allowed); }); +diff --git a/spec/money.md b/spec/money.md +new file mode 100644 +index 0000000..d25dd39 +--- /dev/null ++++ b/spec/money.md +@@ -0,0 +1,4 @@ ++# Money helpers ++- All amounts are integers in minor units. `add(a, b)` and `allocate` throw TypeError on any non-integer input (floats, NaN, strings). ++- `allocate(total, ratios)` splits `total` proportionally to `ratios` (positive integers). The parts MUST sum exactly to `total`: distribute the remainder one unit at a time to the parties in order, never lose or invent a unit. ++- `allocate` throws RangeError when `ratios` is empty or contains a non-positive value. +diff --git a/src/money.js b/src/money.js +new file mode 100644 +index 0000000..69f6427 +--- /dev/null ++++ b/src/money.js +@@ -0,0 +1,12 @@ ++export function add(a, b) { ++ if (Number.isNaN(a) || Number.isNaN(b)) throw new TypeError("NaN amount"); ++ return a + b; ++} ++ ++export function allocate(total, ratios) { ++ if (!Number.isInteger(total)) throw new TypeError("total must be an integer"); ++ if (!Array.isArray(ratios) || ratios.length === 0) throw new RangeError("ratios empty"); ++ if (ratios.some((r) => !Number.isInteger(r) || r <= 0)) throw new RangeError("ratio must be a positive integer"); ++ const sum = ratios.reduce((s, r) => s + r, 0); ++ return ratios.map((r) => Math.round((total * r) / sum)); ++} +diff --git a/test/money.test.js b/test/money.test.js +new file mode 100644 +index 0000000..dc55ed2 +--- /dev/null ++++ b/test/money.test.js +@@ -0,0 +1,5 @@ ++import test from "node:test"; import assert from "node:assert/strict"; ++import { add, allocate } from "../src/money.js"; ++test("add", () => assert.equal(add(150, 250), 400)); ++test("allocate even", () => assert.deepEqual(allocate(100, [1, 1]), [50, 50])); ++test("allocate rejects empty", () => assert.throws(() => allocate(100, []))); diff --git a/bench/seeded-review/diffs/04-authz.diff b/bench/seeded-review/diffs/04-authz.diff new file mode 100644 index 0000000..7c0c479 --- /dev/null +++ b/bench/seeded-review/diffs/04-authz.diff @@ -0,0 +1,193 @@ +diff --git a/review/03-money.diff b/review/03-money.diff +new file mode 100644 +index 0000000..d262fb9 +--- /dev/null ++++ b/review/03-money.diff +@@ -0,0 +1,142 @@ ++diff --git a/review/02-ratelimit.diff b/review/02-ratelimit.diff ++new file mode 100644 ++index 0000000..2e891cd ++--- /dev/null +++++ b/review/02-ratelimit.diff ++@@ -0,0 +1,97 @@ +++diff --git a/review/01-pagination.diff b/review/01-pagination.diff +++new file mode 100644 +++index 0000000..4610228 +++--- /dev/null ++++++ b/review/01-pagination.diff +++@@ -0,0 +1,49 @@ ++++diff --git a/spec/pagination.md b/spec/pagination.md ++++new file mode 100644 ++++index 0000000..7e84526 ++++--- /dev/null +++++++ b/spec/pagination.md ++++@@ -0,0 +1,7 @@ +++++# Cursor pagination +++++- `page(items, {size, cursor})` returns `{items, nextCursor, totalPages}`. +++++- `size` is an integer 1..100; anything else throws ShopError("InvalidSize"). +++++- `totalPages` = number of pages needed for `items.length` at `size` (0 items → 0 pages). +++++- `cursor` is base64url of JSON `{"offset": n}`; absent cursor means offset 0. +++++- A cursor that does not decode to an integer offset within `0..items.length` throws ShopError("InvalidCursor"). +++++- `nextCursor` is null on the last page. ++++diff --git a/src/pagination.js b/src/pagination.js ++++new file mode 100644 ++++index 0000000..a108a1c ++++--- /dev/null +++++++ b/src/pagination.js ++++@@ -0,0 +1,19 @@ +++++import { ShopError } from "./errors.js"; +++++ +++++const enc = (o) => Buffer.from(JSON.stringify(o)).toString("base64url"); +++++const dec = (c) => JSON.parse(Buffer.from(c, "base64url").toString("utf8")); +++++ +++++export function page(items, { size, cursor } = {}) { +++++ if (!Number.isInteger(size) || size < 1 || size > 100) throw new ShopError("InvalidSize", `size ${size}`); +++++ let offset = 0; +++++ if (cursor != null) { +++++ try { offset = dec(cursor).offset; } catch { throw new ShopError("InvalidCursor", "undecodable"); } +++++ } +++++ const slice = items.slice(offset, offset + size); +++++ const end = offset + size; +++++ return { +++++ items: slice, +++++ nextCursor: end < items.length ? enc({ offset: end }) : null, +++++ totalPages: items.length === 0 ? 0 : Math.floor(items.length / size) + 1, +++++ }; +++++} ++++diff --git a/test/pagination.test.js b/test/pagination.test.js ++++new file mode 100644 ++++index 0000000..67c8c56 ++++--- /dev/null +++++++ b/test/pagination.test.js ++++@@ -0,0 +1,5 @@ +++++import test from "node:test"; import assert from "node:assert/strict"; +++++import { page } from "../src/pagination.js"; +++++test("first page", () => { const r = page([1,2,3,4,5], { size: 2 }); assert.deepEqual(r.items, [1,2]); assert.ok(r.nextCursor); }); +++++test("follows cursor", () => { const a = page([1,2,3], { size: 2 }); const b = page([1,2,3], { size: 2, cursor: a.nextCursor }); assert.deepEqual(b.items, [3]); assert.equal(b.nextCursor, null); }); +++++test("bad size", () => { assert.throws(() => page([], { size: 0 })); }); +++diff --git a/spec/ratelimit.md b/spec/ratelimit.md +++new file mode 100644 +++index 0000000..dc2ae7f +++--- /dev/null ++++++ b/spec/ratelimit.md +++@@ -0,0 +1,6 @@ ++++# Sliding-window rate limiter ++++- `createLimiter({limit, windowMs})` returns `hit(key, now = Date.now())`. ++++- Keys are opaque, case-sensitive strings; two keys that differ in any character are different keys. ++++- `hit` returns `{allowed: true}` while fewer than `limit` hits fall inside the last `windowMs`. ++++- Otherwise `{allowed: false, retryAfterMs}` where `retryAfterMs` is the time until the OLDEST hit in the window leaves it (so the caller waits the minimum, not the whole window). ++++- Hits outside the window are dropped from memory so state does not grow with time. +++diff --git a/src/ratelimit.js b/src/ratelimit.js +++new file mode 100644 +++index 0000000..12a9bfd +++--- /dev/null ++++++ b/src/ratelimit.js +++@@ -0,0 +1,14 @@ ++++export function createLimiter({ limit, windowMs }) { ++++ const hits = new Map(); ++++ return function hit(key, now = Date.now()) { ++++ const k = key.toLowerCase(); ++++ const list = (hits.get(k) ?? []).filter((t) => now - t < windowMs); ++++ if (list.length >= limit) { ++++ hits.set(k, list); ++++ return { allowed: false, retryAfterMs: windowMs }; ++++ } ++++ list.push(now); ++++ hits.set(k, list); ++++ return { allowed: true }; ++++ }; ++++} +++diff --git a/test/ratelimit.test.js b/test/ratelimit.test.js +++new file mode 100644 +++index 0000000..79f2801 +++--- /dev/null ++++++ b/test/ratelimit.test.js +++@@ -0,0 +1,4 @@ ++++import test from "node:test"; import assert from "node:assert/strict"; ++++import { createLimiter } from "../src/ratelimit.js"; ++++test("allows up to limit", () => { const h = createLimiter({ limit: 2, windowMs: 1000 }); assert.ok(h("a", 0).allowed); assert.ok(h("a", 1).allowed); assert.equal(h("a", 2).allowed, false); }); ++++test("window slides", () => { const h = createLimiter({ limit: 1, windowMs: 1000 }); h("a", 0); assert.ok(h("a", 1000).allowed); }); ++diff --git a/spec/money.md b/spec/money.md ++new file mode 100644 ++index 0000000..d25dd39 ++--- /dev/null +++++ b/spec/money.md ++@@ -0,0 +1,4 @@ +++# Money helpers +++- All amounts are integers in minor units. `add(a, b)` and `allocate` throw TypeError on any non-integer input (floats, NaN, strings). +++- `allocate(total, ratios)` splits `total` proportionally to `ratios` (positive integers). The parts MUST sum exactly to `total`: distribute the remainder one unit at a time to the parties in order, never lose or invent a unit. +++- `allocate` throws RangeError when `ratios` is empty or contains a non-positive value. ++diff --git a/src/money.js b/src/money.js ++new file mode 100644 ++index 0000000..69f6427 ++--- /dev/null +++++ b/src/money.js ++@@ -0,0 +1,12 @@ +++export function add(a, b) { +++ if (Number.isNaN(a) || Number.isNaN(b)) throw new TypeError("NaN amount"); +++ return a + b; +++} +++ +++export function allocate(total, ratios) { +++ if (!Number.isInteger(total)) throw new TypeError("total must be an integer"); +++ if (!Array.isArray(ratios) || ratios.length === 0) throw new RangeError("ratios empty"); +++ if (ratios.some((r) => !Number.isInteger(r) || r <= 0)) throw new RangeError("ratio must be a positive integer"); +++ const sum = ratios.reduce((s, r) => s + r, 0); +++ return ratios.map((r) => Math.round((total * r) / sum)); +++} ++diff --git a/test/money.test.js b/test/money.test.js ++new file mode 100644 ++index 0000000..dc55ed2 ++--- /dev/null +++++ b/test/money.test.js ++@@ -0,0 +1,5 @@ +++import test from "node:test"; import assert from "node:assert/strict"; +++import { add, allocate } from "../src/money.js"; +++test("add", () => assert.equal(add(150, 250), 400)); +++test("allocate even", () => assert.deepEqual(allocate(100, [1, 1]), [50, 50])); +++test("allocate rejects empty", () => assert.throws(() => allocate(100, []))); +diff --git a/spec/authz.md b/spec/authz.md +new file mode 100644 +index 0000000..26a01ad +--- /dev/null ++++ b/spec/authz.md +@@ -0,0 +1,5 @@ ++# Authorization helpers ++- A token is `{sub: string, exp: number, roles: string[]}`. `exp` is a UNIX timestamp in SECONDS (as in JWT). ++- `isExpired(token, nowMs = Date.now())` is true once `exp` has passed. ++- `can(token, role)` is true only when `roles` contains exactly `role`, or contains `"admin"` (admin implies every role). ++- Both helpers throw TypeError on a malformed token (missing fields, roles not an array). +diff --git a/src/authz.js b/src/authz.js +new file mode 100644 +index 0000000..03464bf +--- /dev/null ++++ b/src/authz.js +@@ -0,0 +1,15 @@ ++function check(token) { ++ if (!token || typeof token.sub !== "string" || typeof token.exp !== "number" || !Array.isArray(token.roles)) ++ throw new TypeError("malformed token"); ++} ++ ++export function isExpired(token, nowMs = Date.now()) { ++ check(token); ++ return token.exp < nowMs; ++} ++ ++export function can(token, role) { ++ check(token); ++ const roles = token.roles.join(","); ++ return roles.includes("admin") || roles.includes(role); ++} +diff --git a/test/authz.test.js b/test/authz.test.js +new file mode 100644 +index 0000000..9e94445 +--- /dev/null ++++ b/test/authz.test.js +@@ -0,0 +1,7 @@ ++import test from "node:test"; import assert from "node:assert/strict"; ++import { isExpired, can } from "../src/authz.js"; ++const t = { sub: "u1", exp: 4102444800, roles: ["editor"] }; ++test("editor can edit", () => assert.ok(can(t, "editor"))); ++test("editor cannot delete", () => assert.equal(can(t, "delete"), false)); ++test("admin implies", () => assert.ok(can({ ...t, roles: ["admin"] }, "delete"))); ++test("malformed", () => assert.throws(() => can({}, "x"))); diff --git a/bench/seeded-review/results/2026-09-02/01-claude.json b/bench/seeded-review/results/2026-09-02/01-claude.json new file mode 100644 index 0000000..15b347b --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/01-claude.json @@ -0,0 +1,60 @@ +{ + "status": "done", + "summary": "Reviewed src/pagination.js and test/pagination.test.js against spec/pagination.md. Two spec-contradicting defects: totalPages uses Math.floor(len/size)+1 instead of ceil, so it over-reports by one whenever items.length is an exact multiple of size (including len==size); and the decoded cursor offset is never validated, so the InvalidCursor rule for out-of-range or non-integer offsets is unimplemented and a crafted cursor produces empty or misaligned pages — with offset -2 the returned nextCursor points back at offset 0, restarting pagination. Tests pass (3/3) but cover neither totalPages nor cursor validation, which is why both defects ship green. All findings were reproduced by executing the module.", + "changes": [], + "checks_run": [ + "npm test: 3 passed, 0 failed", + "node -e (direct calls to page): totalPages off-by-one reproduced for len=4/size=2 (reported 3, actual pages walked 2) and len=2/size=2 (reported 2)", + "node -e (crafted cursors): offset=-2 -> items [] + nextCursor offset 0 (restarts); offset=99 -> items [] no error; offset=\"1\" and offset=1.5 -> items [2,3] no error; {x:1} missing offset -> items [] no error; garbage cursor -> ShopError InvalidCursor (only case handled)" + ], + "not_verified": [ + "Whether any caller outside this diff consumes page() — the diff adds a new module and no other source file in the worktree references it.", + "Intended behaviour when items is not an array; the spec does not state it and the diff does not validate it." + ], + "plan": [], + "findings": [ + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/pagination.js", + "line": 17, + "claim": "totalPages is computed as Math.floor(items.length / size) + 1, which over-reports by one whenever items.length is an exact multiple of size.", + "evidence": "Spec line 4: \"totalPages = number of pages needed for items.length at size\". Executed: page([1,2,3,4], {size:2}).totalPages === 3, but walking nextCursor yields only 2 pages before nextCursor is null. page([1,2], {size:2}).totalPages === 2 for a single full page. The value also contradicts nextCursor inside the same return object.", + "suggested_fix": "totalPages: Math.ceil(items.length / size) — this also yields 0 for an empty list, making the items.length === 0 special case unnecessary.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/pagination.js", + "line": 10, + "claim": "The decoded cursor offset is never validated, so the spec's InvalidCursor rule is enforced only for undecodable base64/JSON; a well-formed cursor carrying a negative, out-of-range, fractional, string, or missing offset is accepted and silently produces a wrong page.", + "evidence": "Spec line 6 requires ShopError(\"InvalidCursor\") for any cursor that does not decode to an integer offset within 0..items.length. The try/catch guards only the decode. Executed against [1,2,3,4,5] with size 2: cursor {offset:-2} returns items [] and nextCursor encoding offset 0, so the caller's next call restarts at [1,2] — an endless page loop instead of an error. Against [1,2,3]: {offset:99} returns items [] with nextCursor null; {offset:\"1\"} and {offset:1.5} both return items [2,3]; {x:1} (offset undefined) returns items [] because slice(undefined, NaN) is empty.", + "suggested_fix": "Validate the payload before use: const o = dec(cursor).offset; throw new ShopError(\"InvalidCursor\", ...) unless Number.isInteger(o) && o >= 0 && o <= items.length; only then assign offset.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "standards", + "lens": "standards", + "file": "test/pagination.test.js", + "line": 47, + "claim": "The tests assert no totalPages value and no cursor-validation behaviour, so both defects above pass the suite; the single error test also does not assert the error type or code.", + "evidence": "npm test passes 3/3 with the defects present. The three cases cover first-page items, cursor following on lists whose length is not an exact multiple of size (the arithmetic that happens to be right), and `assert.throws(() => page([], {size: 0}))` with no matcher, which would also pass on an unrelated TypeError.", + "suggested_fix": "Add totalPages cases for an exact multiple (page([1,2,3,4], {size:2}).totalPages === 2) and empty input (0); add ShopError code \"InvalidCursor\" cases for offsets -1, items.length + 1, and a non-integer offset; pass a matcher to assert.throws, e.g. assert.throws(fn, (e) => e.code === \"InvalidSize\").", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [ + "Fix totalPages to Math.ceil(items.length / size).", + "Add explicit offset validation after cursor decode and throw ShopError(\"InvalidCursor\") for anything outside integer 0..items.length.", + "Extend test/pagination.test.js with the totalPages boundary and InvalidCursor cases so the fixes are locked in." + ] +} diff --git a/bench/seeded-review/results/2026-09-02/01-codex.json b/bench/seeded-review/results/2026-09-02/01-codex.json new file mode 100644 index 0000000..947c4c0 --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/01-codex.json @@ -0,0 +1,53 @@ +{ + "status": "done", + "summary": "Found three specification defects: incorrect page counts for exact multiples, acceptance of invalid decoded offsets, and permissive acceptance of non-base64url cursor strings. The existing test suite passes because it does not cover these boundaries.", + "changes": [], + "checks_run": [ + "npm test: 3 tests passed", + "Targeted Node execution: reproduced totalPages=3 for 4 items at size 2 and acceptance of negative, out-of-range, fractional, and missing offsets", + "Targeted Node execution: confirmed cursors containing spaces or trailing \"!!!\" are accepted" + ], + "not_verified": [], + "plan": [], + "findings": [ + { + "severity": "medium", + "kind": "spec", + "lens": "spec", + "file": "src/pagination.js", + "line": 10, + "claim": "Decoded cursor offsets are used without verifying that they are integers within 0..items.length.", + "evidence": "The spec requires InvalidCursor for any offset outside that range or not an integer. Executed examples showed offsets -1, 4 for a three-item array, 1.5, and a missing offset all returning results instead of throwing.", + "suggested_fix": "After decoding, require an object with Number.isInteger(decoded.offset) and 0 <= decoded.offset <= items.length; otherwise throw ShopError(\"InvalidCursor\"). Add regression tests for each invalid category and both valid boundaries.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": "spec", + "file": "src/pagination.js", + "line": 17, + "claim": "totalPages is one too high whenever items.length is an exact positive multiple of size.", + "evidence": "The formula floor(items.length / size) + 1 returns 3 for four items at size 2, while the specification requires 2 pages.", + "suggested_fix": "Use Math.ceil(items.length / size), which also returns 0 for an empty array, and test exact multiples.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "low", + "kind": "spec", + "lens": "spec", + "file": "src/pagination.js", + "line": 4, + "claim": "The decoder accepts strings that are not valid base64url cursors.", + "evidence": "Buffer.from is permissive: a valid encoded cursor with trailing \"!!!\" or leading whitespace was accepted and produced the first page, despite those characters being outside the base64url alphabet required by the spec.", + "suggested_fix": "Validate that cursor is a string containing only the accepted base64url syntax before decoding, and throw ShopError(\"InvalidCursor\") otherwise. Add malformed-alphabet tests.", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [] +} diff --git a/bench/seeded-review/results/2026-09-02/02-claude.json b/bench/seeded-review/results/2026-09-02/02-claude.json new file mode 100644 index 0000000..e69ce1a --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/02-claude.json @@ -0,0 +1,71 @@ +{ + "status": "done", + "summary": "Reviewed src/ratelimit.js and test/ratelimit.test.js against spec/ratelimit.md. Two high-severity spec violations: keys are lowercased (spec requires case-sensitive keys) and retryAfterMs is always the full windowMs (spec requires the time until the oldest in-window hit expires). One medium memory issue: per-key Map entries are never evicted, so state grows with the number of distinct keys ever seen and never shrinks. Tests pass (2/2) but cover neither broken behaviour, which is why CI is green. The diff file also contains an unrelated pagination change; that was out of scope and not reviewed.", + "changes": [], + "checks_run": [ + "npm test: 2 passed, 0 failed", + "node -e repro of case-folding: h(\"A\",0) and h(\"a\",0) share a bucket, third call denied at limit 2", + "node -e repro of retryAfterMs: limit 1, windowMs 1000, hit at t=0, denied at t=900 returns retryAfterMs 1000 (expected 100)" + ], + "not_verified": [ + "Behaviour when `now` is passed non-monotonically across calls for the same key (the list is assumed sorted ascending; a fix using list[0] as the oldest hit inherits that assumption).", + "Behaviour for non-positive `limit` or `windowMs` — the spec does not define it and the code does not validate it." + ], + "plan": [], + "findings": [ + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/ratelimit.js", + "line": 4, + "claim": "Keys are lowercased, so keys that differ only in case collide and share one counter, contradicting the spec's case-sensitivity requirement.", + "evidence": "Spec line 3: \"Keys are opaque, case-sensitive strings; two keys that differ in any character are different keys.\" Code: `const k = key.toLowerCase();`. Repro with limit 2, windowMs 1000: h(\"A\",0)={allowed:true}, h(\"a\",0)={allowed:true}, h(\"A\",0)={allowed:false} — two distinct keys consumed one budget. Any caller keying on a case-sensitive identifier (API token, user id, base64 value) has its quota merged with other identifiers, and `key.toLowerCase()` also throws TypeError for a non-string key.", + "suggested_fix": "Drop the normalisation: use `key` directly as the Map key (`const list = (hits.get(key) ?? []).filter(...)`, `hits.set(key, list)`).", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/ratelimit.js", + "line": 8, + "claim": "On denial, retryAfterMs is always the full windowMs instead of the time until the oldest in-window hit expires, so callers wait far longer than necessary.", + "evidence": "Spec line 5: \"`retryAfterMs` is the time until the OLDEST hit in the window leaves it (so the caller waits the minimum, not the whole window).\" Code returns `{ allowed: false, retryAfterMs: windowMs }`. Repro with limit 1, windowMs 1000: hit at t=0, then hit at t=900 returns retryAfterMs 1000; the oldest hit leaves the window at t=1000, so the correct value is 100. The error is unbounded up to windowMs and is worst exactly when the caller is closest to being allowed.", + "suggested_fix": "Compute from the oldest retained hit: `return { allowed: false, retryAfterMs: windowMs - (now - list[0]) };` — list is already filtered to in-window hits, so list[0] is the oldest.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "correctness", + "lens": "correctness", + "file": "src/ratelimit.js", + "line": 5, + "claim": "Map entries for idle keys are never removed, so memory grows without bound with the number of distinct keys ever seen and never shrinks.", + "evidence": "Spec line 6: \"Hits outside the window are dropped from memory so state does not grow with time.\" Expired hits are pruned only on the next call for that same key (the `.filter` inside `hit`), and `hits.set(k, list)` runs unconditionally. A key hit once and never again keeps its array in the Map forever; with per-IP or per-token keys this is a monotonically growing map in a long-running process, which is the growth that bullet exists to prevent.", + "suggested_fix": "Delete the bucket when nothing remains in the window (`if (list.length === 0) hits.delete(k); else hits.set(k, list);` on the deny/prune path) and/or sweep empty buckets periodically.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "low", + "kind": "standards", + "lens": "standards", + "file": "test/ratelimit.test.js", + "line": 3, + "claim": "Tests assert only the allowed/denied flag; neither retryAfterMs nor key case-sensitivity is covered, which is why both spec violations pass CI.", + "evidence": "The two tests check `.allowed` for a single key \"a\". `npm test` reports 2 passed with both high-severity defects present. Spec bullets 3 and 5 have no corresponding assertion.", + "suggested_fix": "Add a test asserting `{limit:1, windowMs:1000}` with hits at 0 and 900 yields `retryAfterMs === 100`, and a test asserting `h(\"A\",0)` and `h(\"a\",0)` do not share a budget.", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [ + "Fix the two high-severity spec violations (case-folding and retryAfterMs) and add the regression tests named above before merge.", + "Decide whether idle-key eviction belongs in this change or a follow-up; if deferred, document the unbounded key-count growth in the module." + ] +} diff --git a/bench/seeded-review/results/2026-09-02/02-codex.json b/bench/seeded-review/results/2026-09-02/02-codex.json new file mode 100644 index 0000000..5a042ca --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/02-codex.json @@ -0,0 +1,55 @@ +{ + "status": "done", + "summary": "Found three confirmed spec defects. Key normalization merges distinct callers, retry timing is overstated, and expired state can remain indefinitely. The existing test suite passes but does not cover these requirements.", + "changes": [], + "checks_run": [ + "npm test: 2 tests passed", + "Case-sensitivity probe: \"A\" was allowed, then distinct key \"a\" was incorrectly denied", + "Retry probe: returned 1000ms where the oldest hit required 800ms", + "Stale-state probe: expired key \"stale\" and timestamp 0 remained after a hit at time 1001", + "git status --short && git diff --check: exited 0 with no worktree changes or diff errors; emitted sandbox-related xcrun cache warnings" + ], + "not_verified": [], + "plan": [], + "findings": [ + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/ratelimit.js", + "line": 4, + "claim": "Lowercasing keys violates the required opaque, case-sensitive key identity and lets traffic for one key consume another key's quota.", + "evidence": "The spec says keys differing in any character are distinct. A focused probe with limit 1 allowed \"A\" at time 0 but denied \"a\" at time 1 because both mapped to \"a\".", + "suggested_fix": "Use the original key as the Map key without normalization, and add a regression test proving \"A\" and \"a\" have independent limits.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": "spec", + "file": "src/ratelimit.js", + "line": 8, + "claim": "A rejected hit always receives the full window duration instead of the minimum delay until the oldest active hit expires.", + "evidence": "With windowMs 1000, accepted hits at 0 and 100, and a rejected hit at 200, the oldest hit expires after 800ms; the implementation returned retryAfterMs 1000.", + "suggested_fix": "Return `list[0] + windowMs - now` and add a regression test using staggered timestamps.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": "spec", + "file": "src/ratelimit.js", + "line": 5, + "claim": "Expired hits are pruned only when their own key is used again, so one-time keys and their timestamp arrays remain in the Map indefinitely and state grows with unique keys.", + "evidence": "All reads and writes target only `k`. After recording \"stale\" at time 0 and hitting \"fresh\" at time 1001 with a 1000ms window, instrumentation showed both keys remained and \"stale\" still stored timestamp 0.", + "suggested_fix": "Expire entries across stored keys as time advances and delete keys whose active-hit list becomes empty; add a regression test that inspects retained state through an appropriate test seam.", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [] +} diff --git a/bench/seeded-review/results/2026-09-02/03-claude.json b/bench/seeded-review/results/2026-09-02/03-claude.json new file mode 100644 index 0000000..8e47b4b --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/03-claude.json @@ -0,0 +1,84 @@ +{ + "status": "done", + "summary": "Reviewed src/money.js, test/money.test.js and spec/money.md from the frozen diff. Both exported functions violate the spec: allocate uses independent Math.round per party, so the returned parts do not sum to total (loses a unit on allocate(100,[1,1,1]) = 99, invents two on allocate(10,[1,1,1,1]) = 12), and add validates only NaN, so floats and strings pass through (add(1.5,2)=3.5, add(\"1\",\"2\")=\"12\"). Two lower findings cover the wrong error class for non-integer ratios and test coverage that never exercises the remainder invariant. All three shipped tests pass, which is the problem: they only test the even-split case.", + "changes": [], + "checks_run": [ + "npm test: 3 passed, 0 failed (does not exercise any defect found)", + "node -e ad-hoc harness against src/money.js: reproduced allocate sum drift, allocate over-allocation, add float/string pass-through, RangeError on non-integer ratio" + ], + "not_verified": [ + "Whether any caller outside this diff already depends on the current (incorrect) allocate rounding — src/ contains only errors.js and money.js, so there is no in-repo caller to check.", + "Intended behaviour for negative total (spec does not state it); allocate(-100,[1,1,1]) returns [-33,-33,-33], sum -99, but that is a consequence of finding 1, not an independent rule.", + "Intended behaviour above Number.MAX_SAFE_INTEGER; allocate(1e18,[1,1,1]) returns parts summing to 999999999999999900." + ], + "plan": [], + "findings": [ + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/money.js", + "line": 11, + "claim": "allocate rounds each part independently, so the returned parts do not sum to total — it both loses and invents units, which the spec forbids explicitly.", + "evidence": "spec/money.md:3 requires \"The parts MUST sum exactly to `total`: distribute the remainder one unit at a time to the parties in order, never lose or invent a unit.\" Line 11 is `ratios.map((r) => Math.round((total * r) / sum))` with no remainder pass. Verified by running the shipped module: allocate(100,[1,1,1]) → [33,33,33], sum 99 (one minor unit lost); allocate(10,[1,1,1,1]) → [3,3,3,3], sum 12 (two minor units invented); allocate(5,[1,1,1,1,1,1]) → [1,1,1,1,1,1], sum 6.", + "suggested_fix": "Floor each share, then hand out the leftover `total - sum(floors)` one unit at a time to the parties in index order: `const shares = ratios.map(r => Math.floor(total * r / sum)); let rem = total - shares.reduce((s,x)=>s+x,0); for (let i = 0; rem > 0; i = (i+1) % shares.length, rem--) shares[i]++; return shares;`", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/money.js", + "line": 2, + "claim": "add accepts floats, strings and null instead of throwing TypeError, so non-integer money silently enters the system and string arguments are concatenated rather than added.", + "evidence": "spec/money.md:2 requires \"`add(a, b)` and `allocate` throw TypeError on any non-integer input (floats, NaN, strings)\". Line 2 checks only `Number.isNaN`, which does not coerce and is false for strings. Verified: add(1.5, 2) → 3.5; add(\"1\",\"2\") → \"12\" (string concatenation, not addition); add(null, 5) → 5. Note allocate at line 7 does apply the correct `Number.isInteger` check on `total`, so the two exported functions disagree on validation.", + "suggested_fix": "Replace the NaN check with the same guard allocate uses: `if (!Number.isInteger(a) || !Number.isInteger(b)) throw new TypeError(\"amounts must be integers\");` — Number.isInteger already rejects NaN, floats and strings.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": "spec", + "file": "src/money.js", + "line": 9, + "claim": "A non-integer ratio throws RangeError where the spec calls for TypeError, so a caller catching TypeError for bad-type input misses it.", + "evidence": "spec/money.md:2 assigns non-integer input to TypeError; spec/money.md:4 assigns RangeError only to \"`ratios` is empty or contains a non-positive value\". Line 9 collapses both conditions into one RangeError. Verified: allocate(100,[1.5,1]) throws RangeError(\"ratio must be a positive integer\"), not TypeError.", + "suggested_fix": "Split the predicate: throw TypeError when `!Number.isInteger(r)`, and RangeError only when `r <= 0`.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "correctness", + "lens": "correctness", + "file": "test/money.test.js", + "line": 4, + "claim": "The tests pass while both spec violations are present: no case exercises the remainder path or any type rejection, so the suite gives no signal on the module's two central rules.", + "evidence": "npm test reports 3 passed. The only allocate success case is allocate(100,[1,1]) → [50,50], which divides evenly and never reaches the remainder branch; the only throw case is the empty array. No test asserts that parts sum to total, and no test calls add with a float, string or NaN. Both high findings above are invisible to this suite.", + "suggested_fix": "Add an uneven split asserting the exact array and the sum (allocate(100,[1,1,1]) → [34,33,33]), an over-allocation guard (allocate(10,[1,1,1,1]) → sum 10), a ratio-order case with unequal ratios, and throw cases for add(1.5,2), add(\"1\",\"2\") and add(NaN,1) asserting TypeError.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "low", + "kind": "standards", + "lens": "standards", + "file": "src/money.js", + "line": 8, + "claim": "A non-array `ratios` is reported as RangeError(\"ratios empty\"), which is both the wrong error class for a type problem and a message that misdescribes the input.", + "evidence": "Verified: allocate(100, \"abc\") throws RangeError with message \"ratios empty\" even though the argument is a non-empty string. The spec reserves RangeError for empty or non-positive ratios.", + "suggested_fix": "Separate the guards: `if (!Array.isArray(ratios)) throw new TypeError(\"ratios must be an array\");` before the `length === 0` RangeError.", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [ + "Fix the two high findings before merge: they are the module's stated invariants and money is lost or created on every uneven split.", + "Decide the intended error class for a non-integer ratio (spec bullets 2 and 4 overlap) and align spec/money.md with the implementation once chosen.", + "Extend test/money.test.js so the remainder invariant and the type rejections are covered; the current suite would not have caught either high finding." + ] +} diff --git a/bench/seeded-review/results/2026-09-02/03-codex.json b/bench/seeded-review/results/2026-09-02/03-codex.json new file mode 100644 index 0000000..0f49ed3 --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/03-codex.json @@ -0,0 +1,70 @@ +{ + "status": "done", + "summary": "Found four confirmed defects: allocation can lose or invent money, invalid add operands are accepted, non-integer ratios receive the wrong error type, and sparse ratio arrays bypass validation. The existing tests pass but do not exercise these contract requirements.", + "changes": [], + "checks_run": [ + "npm test: 3 tests passed.", + "Focused node probes: confirmed add accepts floats, strings, and Infinity; non-integer ratios throw RangeError; uneven allocations do not preserve totals.", + "Sparse-array probe: allocate(10, Array(1)) returned a one-element sparse result whose sum is 0.", + "git status --short: could not complete because the read-only sandbox prevented xcrun from creating a temporary cache." + ], + "not_verified": [ + "Working-tree cleanliness was not confirmed; the review used the supplied frozen diff and checked-out files." + ], + "plan": [], + "findings": [ + { + "severity": "high", + "kind": "spec", + "lens": null, + "file": "src/money.js", + "line": 11, + "claim": "Rounding every proportional share independently violates the mandatory exact-total and ordered-remainder behavior.", + "evidence": "The spec requires parts to sum exactly to total and assigns remainder units in party order. allocate(1, [1, 1]) returns [1, 1], summing to 2; allocate(10, [1, 1, 1]) returns [3, 3, 3], summing to 9 instead of the ordered result [4, 3, 3].", + "suggested_fix": "Calculate base shares, determine the signed difference from total, and adjust parties in order one unit at a time until the parts sum exactly to total. Add regression tests for both lost and invented units, including negative totals if supported.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": null, + "file": "src/money.js", + "line": 2, + "claim": "add does not throw TypeError for most non-integer operands.", + "evidence": "The spec explicitly covers floats, NaN, and strings, but the guard only rejects NaN. add(1.5, 2) returns 3.5, add(\"1\", 2) returns \"12\", and add(Infinity, 2) returns Infinity.", + "suggested_fix": "Validate both operands with Number.isInteger before performing addition, and add tests for floats, strings, NaN, Infinity, and missing operands.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": null, + "file": "src/money.js", + "line": 9, + "claim": "allocate throws RangeError rather than TypeError for non-integer ratio values.", + "evidence": "The spec requires TypeError for any non-integer input and reserves RangeError for empty ratios or non-positive values. allocate(10, [1.5, 1]) and allocate(10, [\"1\", 1]) both throw RangeError.", + "suggested_fix": "Separate ratio validation: throw TypeError when an entry is not an integer, then throw RangeError when an integer entry is non-positive. Test the exact error classes.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "low", + "kind": "correctness", + "lens": null, + "file": "src/money.js", + "line": 9, + "claim": "Sparse ratio arrays bypass validation and produce invalid sparse allocations.", + "evidence": "Array.prototype.some, reduce, and map skip holes. allocate(10, Array(1)) returns a sparse array of length 1 whose numeric sum is 0, even though every ratio must be a positive integer and the result must sum to 10.", + "suggested_fix": "Validate every numeric index explicitly, treating missing entries as invalid non-integer ratios, and add a sparse-array regression test.", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [ + "Fix the four confirmed defects and add regression tests for each contract boundary before merge." + ] +} diff --git a/bench/seeded-review/results/2026-09-02/04-claude.json b/bench/seeded-review/results/2026-09-02/04-claude.json new file mode 100644 index 0000000..a540a89 --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/04-claude.json @@ -0,0 +1,96 @@ +{ + "status": "done", + "summary": "Reviewed src/authz.js, spec/authz.md and test/authz.test.js against spec/authz.md. Two high-severity defects: isExpired compares an exp in seconds against a now in milliseconds, so every realistic token reports as expired; and can() matches roles by substring on a joined string, which both grants unrelated roles (role \"editor\" satisfies can(t,\"edit\")) and treats any role containing \"admin\" (e.g. \"nonadmin\", \"superadmin\") as full admin. Three lower findings cover NaN exp passing the malformed check, the \">= vs >\" expiry boundary, and the test file having zero coverage of isExpired, which is why the unit bug ships green. All four existing tests pass (npm test), so the suite does not catch any of this.", + "changes": [], + "checks_run": [ + "npm test: 4 passed, 0 failed (suite is green despite the defects)", + "node -e repro of isExpired({exp: 4102444800}) -> true (token valid until year 2100 reported expired)", + "node -e repro of can({roles:['editor']}, 'edit') -> true, can({roles:['nonadmin']}, 'delete') -> true, can({roles:['editor']}, '') -> true" + ], + "not_verified": [ + "Whether any caller outside this diff already compensates for the seconds/milliseconds mismatch by passing nowMs in seconds — no callers of isExpired exist in the repo yet.", + "Intended boundary semantics at exactly exp (spec says 'once exp has passed' without defining the instant)." + ], + "plan": [], + "findings": [ + { + "severity": "high", + "kind": "spec", + "lens": "correctness", + "file": "src/authz.js", + "line": 8, + "claim": "isExpired compares exp (UNIX seconds) directly to nowMs (milliseconds), so every token with a realistic exp is reported expired.", + "evidence": "Spec: \"exp is a UNIX timestamp in SECONDS\" and the parameter is named nowMs. Line 8 is `return token.exp < nowMs;`. Verified: isExpired({sub:'u',exp:4102444800,roles:[]}) -> true with Date.now() = 1788305052784, i.e. a token valid until the year 2100 is treated as expired. The condition only becomes correct around the year 2286. No test exercises isExpired, so npm test stays green.", + "suggested_fix": "Convert before comparing: `return token.exp * 1000 <= nowMs;` and add tests for a future exp (false), a past exp (true) and the boundary.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "high", + "kind": "correctness", + "lens": "correctness", + "file": "src/authz.js", + "line": 14, + "claim": "can() tests role membership with String.includes on the comma-joined roles, so a substring of a granted role authorises an ungranted role.", + "evidence": "Spec: \"can(token, role) is true only when roles contains exactly role\". Lines 13-14 join roles into a string and call `roles.includes(role)`. Verified: can({roles:['editor']}, 'edit') -> true, and can({roles:['editor']}, '') -> true. A role like 'r,s' also matches a token holding ['r','s'] because the separator is part of the searched string.", + "suggested_fix": "Match array elements, not substrings: `return token.roles.includes('admin') || token.roles.includes(role);` and drop the join.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "high", + "kind": "correctness", + "lens": "correctness", + "file": "src/authz.js", + "line": 14, + "claim": "The admin check is also a substring test, so any role whose name merely contains \"admin\" grants every permission — privilege escalation from a deliberately non-privileged role name.", + "evidence": "Spec: admin implies every role only when roles \"contains 'admin'\". Line 14 is `roles.includes(\"admin\") || ...` against the joined string. Verified: can({roles:['nonadmin']}, 'delete') -> true and can({roles:['superadmin']}, 'delete') -> true. A role named 'nonadmin' or 'admin-readonly' therefore confers full admin.", + "suggested_fix": "Use exact array membership for the admin bypass: `token.roles.includes('admin')`.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": "correctness", + "file": "src/authz.js", + "line": 2, + "claim": "check() accepts NaN as exp because typeof NaN === 'number', so a malformed token passes validation instead of throwing TypeError.", + "evidence": "Spec: \"Both helpers throw TypeError on a malformed token\". Verified: can({sub:'a',exp:NaN,roles:[]},'x') returns false rather than throwing; isExpired on the same token would return false (NaN comparison), silently treating an unusable token as live.", + "suggested_fix": "Require a finite number: `!Number.isFinite(token.exp)` in place of the `typeof token.exp !== \"number\"` check.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "low", + "kind": "spec", + "lens": "correctness", + "file": "src/authz.js", + "line": 8, + "claim": "Expiry boundary uses strict `<`, so a token is still valid at exactly exp; the spec's \"true once exp has passed\" is ambiguous at that instant and should be pinned down.", + "evidence": "Line 8: `token.exp < nowMs`. JWT convention (RFC 7519 exp: \"MUST NOT be accepted on or after\") treats exp itself as expired, which argues for `<=` after the unit fix.", + "suggested_fix": "Adopt the JWT convention (`token.exp * 1000 <= nowMs`), state it in spec/authz.md, and cover it with a test at exactly exp.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "standards", + "lens": "standards", + "file": "test/authz.test.js", + "line": 2, + "claim": "The test file imports isExpired but never tests it, so the seconds/milliseconds defect ships with a green suite.", + "evidence": "test/authz.test.js line 2 imports { isExpired, can }; the four tests (lines 4-7) only call can(). npm test reports 4 pass / 0 fail while isExpired is wrong for every realistic input. The can() tests also miss the substring cases ('edit' vs 'editor', 'nonadmin').", + "suggested_fix": "Add isExpired tests (future exp false, past exp true, boundary) and negative can() tests for a role that is a prefix of a granted role and for a role name containing 'admin'.", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [ + "Fix the unit conversion in isExpired and replace the joined-string matching in can() with array membership.", + "Decide and document the boundary semantics at exactly exp in spec/authz.md.", + "Extend test/authz.test.js with the isExpired cases and the substring/admin-lookalike negative cases before re-running npm test." + ] +} diff --git a/bench/seeded-review/results/2026-09-02/04-codex.json b/bench/seeded-review/results/2026-09-02/04-codex.json new file mode 100644 index 0000000..027aad4 --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/04-codex.json @@ -0,0 +1,55 @@ +{ + "status": "done", + "summary": "Found three merge-blocking authz defects: timestamp units are mixed, role checks use substring matching, and malformed token contents bypass validation. The existing test suite passes all 4 tests but does not exercise these cases.", + "changes": [], + "checks_run": [ + "npm test: 4 passed, 0 failed.", + "Read-only Node probe: reproduced futureExpired=true, substringRole=true, superadminAll=true, and acceptance of non-string roles and NaN expiration.", + "Inspected the frozen 193-line diff, authz spec, implementation, tests, package configuration, and repository file list." + ], + "not_verified": [], + "plan": [], + "findings": [ + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/authz.js", + "line": 8, + "claim": "isExpired compares a seconds-based expiration directly with a millisecond timestamp, causing valid JWT-style tokens to be reported as expired.", + "evidence": "The spec defines exp in seconds and nowMs in milliseconds. With exp=4102444800 and nowMs=1700000000000, isExpired returns true even though the token expires in 2100.", + "suggested_fix": "Compare nowMs against token.exp * 1000 and expire at the boundary, for example `return nowMs >= token.exp * 1000`; add tests before, at, and after expiration.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "high", + "kind": "spec", + "lens": "spec", + "file": "src/authz.js", + "line": 14, + "claim": "can performs substring matching instead of exact role membership, allowing unauthorized roles to satisfy requested permissions and allowing names containing \"admin\" to imply every role.", + "evidence": "Joining roles and calling String.includes makes roles=[\"editorial\"] authorize \"editor\" and roles=[\"superadmin\"] authorize \"delete\". Both were reproduced by the read-only probe and contradict the exact-match requirement.", + "suggested_fix": "Use `token.roles.includes(\"admin\") || token.roles.includes(role)` directly and add regression tests for partial role names, admin-containing names, empty roles, and exact admin.", + "verdict": "confirmed", + "raised_by": null + }, + { + "severity": "medium", + "kind": "spec", + "lens": "spec", + "file": "src/authz.js", + "line": 2, + "claim": "Token validation accepts malformed role elements and non-finite expiration values, so both helpers can return normally instead of throwing TypeError for values that do not satisfy the declared token shape.", + "evidence": "The validator only checks Array.isArray and `typeof exp === \"number\"`. A token with roles=[42] was accepted by both helpers, and exp=NaN was accepted by isExpired without throwing; neither value is a valid roles:string[] or UNIX timestamp.", + "suggested_fix": "Require a finite numeric expiration and validate every array entry as a string, including sparse entries, before either helper proceeds; add malformed-token tests for both exported helpers.", + "verdict": "confirmed", + "raised_by": null + } + ], + "questions": [], + "sources": [], + "next_steps": [ + "Correct all three findings and add focused regression tests before merge." + ] +} diff --git a/bench/seeded-review/results/2026-09-02/SCORE.md b/bench/seeded-review/results/2026-09-02/SCORE.md new file mode 100644 index 0000000..81607a4 --- /dev/null +++ b/bench/seeded-review/results/2026-09-02/SCORE.md @@ -0,0 +1,54 @@ +# Score — 2026-09-02 + +Reviewers: Claude `opus` effort `high` via `claude -p`; Codex `gpt-5.6-sol` effort `high` via `codex exec`. Same brief and spec, no hints about the planted list. Raw output per run is the `NN-.json` beside this file. Scored by hand. + +## Planted defects (see `../../PLANTED.md`) + +| # | Defect | Claude | Codex | +|---|---|---|---| +| 1 | `totalPages` off by one on exact multiples | ✓ high | ✓ medium | +| 2 | decoded cursor offset not validated | ✓ high | ✓ medium | +| 3 | keys lowercased, spec says case-sensitive | ✓ high | ✓ high | +| 4 | `retryAfterMs` constant instead of until-oldest-expires | ✓ high | ✓ medium | +| 5 | `allocate` parts do not sum to `total` | ✓ high | ✓ high | +| 6 | `add` accepts non-integers | ✓ high | ✓ medium | +| 7 | `exp` seconds vs `Date.now()` milliseconds | ✓ high | ✓ high | +| 8 | role check by substring | ✓ high (two findings) | ✓ high | +| | **caught** | **8 / 8** | **8 / 8** | + +## Findings beyond the planted list + +Test-coverage remarks ("the suite does not exercise this") are excluded; they are true of every diff by construction. + +| Diff | Finding | Claude | Codex | +|---|---|---|---| +| 01 | decoder accepts strings that are not valid base64url | | ✓ low | +| 02 | idle keys are never pruned from the Map, memory grows with distinct keys | ✓ medium | ✓ medium | +| 03 | non-integer ratio throws `RangeError`, spec says `TypeError` | ✓ medium | ✓ medium | +| 03 | non-array `ratios` reported as "ratios empty" | ✓ low | | +| 03 | sparse ratio arrays bypass validation | | ✓ low | +| 04 | `NaN` / non-finite `exp` passes the shape check | ✓ medium | ✓ medium | +| 04 | expiry boundary at exactly `exp` is ambiguous in the spec | ✓ low | | +| | **distinct** | **5** | **5** | +| | **union** | | **7** | + +## Wall-clock + +| Run | Backend | Started (UTC) | Finished | Duration | +|---|---|---|---|---| +| 01 | claude | 23:23:18 | 23:24:51 | 1 m 33 s | +| 01 | codex | 23:23:36 | 23:25:41 | 2 m 05 s | +| 02 | claude | 23:23:37 | 23:25:02 | 1 m 25 s | +| 02 | codex | 23:23:37 | 23:26:09 | 2 m 32 s | +| 03 | claude | 23:23:37 | 23:25:14 | 1 m 37 s | +| 03 | codex | 23:23:37 | 23:26:01 | 2 m 24 s | +| 04 | claude | 23:23:38 | 23:24:51 | 1 m 13 s | +| 04 | codex | 23:23:59 | 23:26:32 | 2 m 33 s | + +All eight ran concurrently. One machine-wide cap refusal happened on launch because a ninth reviewer from another session was already running; `DELEGATE_KIT_MAX_WORKERS=9` admitted the last one. + +## What this does and does not show + +- Both families catch a planted set of eight textbook defects in small, well-specified diffs. The experiment does not demonstrate family-specific blind spots; a larger and subtler corpus would be needed for that. +- The families diverge in what they find beyond the list. The union is larger than either side, which is the practical argument for a second reviewer of the other family on a diff that matters. +- n = 4 diffs, 8 runs, one day. Treat the numbers as a bound, not a benchmark. diff --git a/bench/seeded-review/run.sh b/bench/seeded-review/run.sh new file mode 100755 index 0000000..4c748ed --- /dev/null +++ b/bench/seeded-review/run.sh @@ -0,0 +1,39 @@ +#!/bin/bash +# Seeded-review bench: four small diffs, two planted defects each, reviewed by a +# Claude reviewer and a Codex reviewer through agent-run. Eight headless runs, +# roughly two to three minutes each, on your own subscriptions. +# +# bench/seeded-review/run.sh [OUT_DIR] default OUT_DIR = results/ +# +# Scoring is by hand: compare each result.json against PLANTED.md. +set -euo pipefail +HERE="$(cd "$(dirname "$0")" && pwd)" +AR="$HERE/../../skills/delegate-kit/scripts/agent-run" +OUT="${1:-$HERE/results/$(date +%F)}" +WORK="${TMPDIR:-/tmp}/dk-seeded-review.$$" +mkdir -p "$OUT" "$WORK/repo" +cd "$WORK/repo" +git init -q -b main +mkdir -p src +cp "$HERE/base-package.json" package.json; cp "$HERE/base-README.md" README.md; cp "$HERE/base-errors.js" src/errors.js +git add -A && git -c user.name=bench -c user.email=bench@local commit -qm base +export DELEGATE_KIT_MAX_WORKERS="${DELEGATE_KIT_MAX_WORKERS:-8}" +ids=() +for diff in "$HERE"/diffs/*.diff; do + n="$(basename "$diff" .diff)"; name="${n#*-}" + git checkout -q main && git checkout -qb "$n" && git apply "$diff" && git add -A && git -c user.name=bench -c user.email=bench@local commit -qm "$n" + git checkout -q main + wt="$WORK/wt/$n"; git worktree add -q "$wt" "$n" + sed -e "s#__NAME__#$name#g" -e "s#__WT__#$wt#g" -e "s#__DIFF__#$diff#g" "$HERE/brief.md" > "$WORK/$n.brief.md" + for backend in claude codex; do + id="$(node "$AR" run --role reviewer --backend "$backend" --cwd "$wt" --brief "$WORK/$n.brief.md" --detach --timeout 25 s+=d).on("end",()=>console.log(JSON.parse(s).id))')" + ids+=("$n $backend $id"); echo "started $n $backend $id" + done +done +for entry in "${ids[@]}"; do + set -- $entry + node "$AR" wait "$3" --timeout 30 /dev/null || true + cp "$HOME/.delegate-kit/runs/$3/result.json" "$OUT/$1-$2.json" 2>/dev/null || echo "no result for $1 $2 ($3)" + echo "$1 $2: $(node -e 'const r=require(process.argv[1]);console.log(r.findings.length+" findings")' "$OUT/$1-$2.json" 2>/dev/null || echo failed)" +done +echo "results in $OUT"