Skip to content

chore: add a tiered review workflow and merge gate - #282

Draft
rafaelha wants to merge 11 commits into
mainfrom
chore/review-workflow
Draft

chore: add a tiered review workflow and merge gate#282
rafaelha wants to merge 11 commits into
mainfrom
chore/review-workflow

Conversation

@rafaelha

@rafaelha rafaelha commented Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Stacked on #281. A review workflow and merge gate, so that changes can merge without the owner reading the code, with the review effort set by the change instead of chosen ad hoc.

Why

Measured on PRs #201–#278 and the agent sessions behind them:

  • Diffs of 600+ lines let bugs through (4 of 5); smaller ones rarely did (about 1 of 40).
  • Under 200 lines, the first review round found everything; second rounds found nits. On large diffs, round 3 still found new real bugs.
  • A second model mostly repeated the first on shallow changes, but found disjoint bugs on deep ones (Keep the error basis sparse in transform_error_basis #270: no overlap).
  • Copilot and Codex comments were real about half the time and almost never wrong, yet three escapes had been flagged by a bot and merged anyway; Move the docs from mkdocs to Mintlify #276 merged minutes after seven bot findings that are still unanswered.
  • Long fuzz campaigns found bugs only in older code, never in the PR under test.

Anthropic's Claude Code team reports the same convergence problem (most re-flags were on code already reviewed) and caps rounds; reviewers given the author's summary find fewer bugs.

What this adds

  • tsim-dev review plan: the tier of the diff from its paths and size.
    • Tier 0, no change in src/tsim: tsim-dev check.
    • Tier 1, other src/tsim changes: plus one fresh-context review.
    • Tier 2, sampling code (statevector/, noise/, compile/, sampler.py, kernels, RNG) or more than 300 source lines: plus a second review from another model family, tsim-dev fuzz, and rounds until one has no findings, at most 3.
    • Independently of the tier, a diff on the compile or sampling path or to the dependencies needs tsim-dev bench --ref <base>, and one that can change the active width needs bench --width. A benchmark that finds a regression still counts as run; review status notes it, and it must be explained in the PR.
    • Notes: SPLIT above 600 source lines, HUMAN for files a person should read (workflows, hook, settings, this gate, the public API), TESTS when tests shrink.
  • tsim-dev review status: exits 0 only when check, and the benchmarks and fuzz that the change needs, ran on exactly the current commit, CI passed, the recorded review rounds meet the tier, and every review thread on the PR is resolved or answered. Exit 1 lists what is owed; exit 3 means waiting on CI or a requested bot.
  • tsim-dev review record --model M --findings N: logs a review round.
  • tsim-dev fuzz: 10 minutes of random small circuits sampled by Tsim and by the dense VecSampler; a mismatch is resampled with 10x the shots before it counts. Also checks bit-packed against unpacked output.
  • reviewing-tsim-changes skill and a tsim-reviewer subagent: the reviewer gets the diff, the task and known findings, not the PR description; it reports only findings it reproduced, with input, expected and observed output; no style or nits.
  • Hook: gh pr merge is blocked unless written as tsim-dev review status && gh pr merge ....
  • AGENTS.md: "Code review rules" for automatic and human reviewers (Codex reads that section).

Verification

  • uv run tsim-dev check: lint passes, 2599 tests pass.
  • tsim-dev fuzz on this branch: 839 circuits in 600 s, 0 failures.
  • With a planted bug (T parsed as T†) the fuzzer reported 7 failing circuits in 90 s.
  • The thread check on merged Move the docs from mkdocs to Mintlify #276 lists its 7 unanswered bot comments.
  • Tier rules, round logic and the merge guard are unit-tested in test/unit/test_dev_cli.py.

Limits

  • Review rounds are self-reported by the agent through review record; the gate cannot prove a review ran. CI, bot threads and the evidence files are checked directly.
  • The fuzzer covers measurement sampling only: no detectors, postselection or PAULI_CHANNEL gates (the dense simulator lacks them). It is a smoke test until the exact oracle lands.
  • Nightly fuzzing on main and periodic whole-repo reviews are not part of this PR.
  • The hook and subagent definition were tested as scripts, not in a live Claude Code session.
  • Files a person should read here: dev/tsim_dev/review.py, .claude/hooks/guard_bash.py.

🤖 Generated with Claude Code

rafaelha and others added 3 commits November 10, 2026 08:10
@rafaelha @claude
chore: add a development harness for agents working on Tsim
6b9c5dd 
- `tsim-dev` CLI (dev/, installed by `uv sync`): `check`, `test --changed`,
  `bench --ref` (interleaved fresh-process A/B, pinned single thread, warm numba
  cache, round-by-round verdicts, tsim-bench corpus), `inspect`, `profile`
  (py-spy, split by phase, time charged to kernel callers), and `run`/`job`
  under systemd memory, CPU and time caps. Short output, logs in .tsim-dev/,
  exit codes 0 pass, 1 failed or slower, 2 harness error, 3 running.
- Benchmark registry of eight circuits in dev/circuits/.
- Developer skills in .claude/skills (also .agents/skills): shipping changes,
  benchmarking, profiling, correctness testing. Not published to users.
- .claude/settings.json: a Bash hook that blocks `pkill -f` and uncapped
  background Python; read-only git/gh allowed, push/PR/merge ask.
- AGENTS.md points to the CLI and skills; heavy-job and PR rules updated.
- Fix test_the_calling_thread_keeps_parallel_kernels_after_run_tasks on TBB
  hosts: TBB runs a 4096-element prange on one thread.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
chore: take benchmark circuits and their postselection from tsim-bench
ff34684 
- `tsim-dev bench` reads circuits from tsim-bench at a pinned commit
  (dev/tsim_dev/corpus.py), fetched into the shared cache on first use, or
  from a local checkout with `--corpus-dir` / TSIM_BENCH_DIR. Each circuit
  there records its arXiv paper and how that paper postselects.
- Postselection follows the recorded protocol by default; `--no-postselect`
  turns it off. Rows show the kept fraction.
- `--suite quick|full|hard|corpus` (suites defined in tsim-bench),
  `--max-width K --width-of tsim|clifft` to keep only circuits up to a
  recorded active width, and circuits named by tsim-bench id or file stem.
- `tsim-dev corpus fetch|list`. The timing worker is tsim-bench's.
- Drop dev/circuits/ and dev/tsim_dev/_worker.py. The .stim files there were
  never committed (`*.stim` is ignored), so a fresh clone could not run them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
chore: compare sides with their own threads, device or strategy in ts…
521f5d4 
…im-dev bench

`--side REF[:threads=N,device=cpu|gpu,strategy=NAME]`, given twice, replaces
`--ref` when the sides differ in more than the checkout: 10 cores against 1,
the GPU executor against the CPU, or two strategies. REF is `.`, a git
revision, a checkout or a Python executable (an environment built elsewhere).
`device=cpu` hides CUDA devices; `device=gpu` fails unless the worker reports
Tsim's CUDA executor usable (tsim-bench 4cb8ca2).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
chore: review fixes for the tsim-bench harness
04413e4 
- Fetching tsim-bench takes a lock and renames a complete export into place,
  so overlapping runs share one export and an interrupted one is redone; a
  pinned commit that is not pushed gives an error, not a tar traceback.
- `tsim-dev profile` accepts `--corpus-dir` (it crashed without it).
- A corpus circuit given by its path keeps its recorded postselection, and
  `--no-postselect` also applies to `.stim` paths outside the corpus.
- A/B runs compare the two sides' kept fractions: if they differ beyond
  binomial noise the rate is marked `kept-differs` and the run fails.
- `k` falls back to the recorded Tsim width (not the `--width-of` backend's);
  the JSON records each side's thread count; malformed manifests are harness
  errors (exit 2).
- Pin tsim-bench 88f3b72 (consistent end-to-end cultivation masks).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha
rafaelha force-pushed the chore/review-workflow branch from 86ad1a7 to bdc685f Compare November 10, 2026 15:26
@github-actions

github-actions Bot commented Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

☂️ Code Coverage

current status: ✅

Overall Coverage

Statements Covered Coverage Threshold Status
4897 4266 87% 0% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 9f07336 by action🐍

rafaelha and others added 5 commits November 10, 2026 08:53
@rafaelha @claude
chore: hide development skills from the skill installer
24575dd 
`npx skills add QuEraComputing/tsim` also searches `.claude/skills` and
`.agents/skills`, so it offered the four development skills next to the user
skill. Mark them `metadata.internal: true` (shown only with
INSTALL_INTERNAL_SKILLS=1), name the user skill in the documented install
command, and test the flag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
feat(dev): tsim-dev bench --width compares active widths without sa…
b88563b 
…mpling

Each side compiles every selected circuit once (tsim-bench's worker in width
mode, 4 jobs in parallel by default) and prints its width next to the one
recorded in tsim-bench; the verdict per circuit is wider, narrower, same or
unknown, and any wider width exits 1. A circuit over Tsim's limit shows
`>=N`, the width from Tsim's refusal. A compile longer than --case-timeout
(300 s) is stopped and recorded as an error; the remaining circuits continue
in a new worker. The default suite is the whole corpus: 190 circuits, both
sides, in 519 s on a Ryzen 9 9900X, all the same as main.

Pin tsim-bench cd0acb6 (width mode; Tsim 1.1 worker).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
chore(dev): remove the GPU options from tsim-dev bench
bbf11ef 
Drop `--side ...:device=cpu|gpu`, the CUDA check and the GPU notes in the
benchmarking and profiling skills until Tsim has a GPU backend on main.
Sides take `threads` and `strategy`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
feat(dev): show and check the active width in shot benchmarks
9d378be 
With a reference, each row compares the two sides' active widths and ends in
`WIDER: k 6 -> 7` or `NARROWER: ...` when they differ; a wider width fails
the run. A width that changes between runs of one side is an error, since
it means compilation is not deterministic. `k` is the sampler's own
`active_width` (Tsim 1.1), without a fallback to the recorded width.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
docs(dev): scale the shipping workflow to the change
e70e43d 
- Benchmark only changes that could affect compile or sampling cost (the
  compile and sampling paths, dependencies, numba settings), and run
  `bench --width` for changes that could change the active width.
- A `slower` verdict is a finding, not a veto: confirm it, profile it, and
  remove it or explain it; a correctness fix may cost performance if stated.
- Distinguish new work (a new branch off main) from updating an existing
  pull request (keep its branch and commit on top).
- Infer acceptance criteria and ask only when they are unclear.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha
rafaelha force-pushed the chore/review-workflow branch from bdc685f to d605c4b Compare November 10, 2026 16:10
rafaelha and others added 2 commits November 10, 2026 09:14
@rafaelha @claude
fix(dev): pyright errors in the width harness; type-check dev/ in pre…
749d4d6 
…-commit

CI runs pyright over the whole repository, the pre-commit hook only over src
and test, so these were missed locally. The width worker stream now yields
lines and records how the worker ended in an `Exit` dataclass, and the
width-change message no longer sorts a set that may hold None. The hook now
also checks dev/.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha @claude
chore: add a tiered review workflow and merge gate
9f07336 
- `tsim-dev review plan|status|record`: the tier of a change comes from the
  paths and size of its diff. Tier 0 (no src change) needs the check gate;
  tier 1 adds a benchmark and one fresh-context review; tier 2 (sampling code or
  more than 300 source lines) adds a second model family, a fuzz run and a last
  round without findings, capped at 3 rounds. `status` reads the evidence that
  `check`, `bench` and `fuzz` record for the current commit, the pull request's
  CI, and its review threads, and exits 0 only when nothing is owed.
- Unanswered review threads (Copilot, Codex, posted reviews) block the gate.
- `tsim-dev fuzz`: random small circuits against the dense VecSampler, with a
  confirmation rerun before a mismatch counts.
- `reviewing-tsim-changes` skill and a `tsim-reviewer` subagent that reports
  only findings it reproduced.
- The Bash hook blocks `gh pr merge` unless it follows `tsim-dev review status &&`.
- AGENTS.md: review rules for automatic and human reviewers.

Rebased on the harness's width mode and scaled shipping rules: a benchmark is
required by path (the compile and sampling paths and the dependencies), not by
tier, and `bench --width` for changes that can alter the active width. A
benchmark that finds a regression counts as run; `review status` notes the
regression to be explained in the pull request. The reviewing skill is marked
internal for the skill installer.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rafaelha
rafaelha force-pushed the chore/review-workflow branch from d605c4b to 9f07336 Compare November 10, 2026 16:16
Base automatically changed from chore/dev-harness to main November 10, 2026 16:57

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@rafaelha