AI Deep Review#1036
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: AI Deep ReviewBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThis PR is documentation-only: it adds Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
gontzess
left a comment
gontzess
left a comment
There was a problem hiding this comment.
Have you done some test runs with this? Would be good to a/b test a few things. Regardless this seems great
| Before the detailed review, score the change per `docs/BUG_CATCHING.md` §2: | ||
| risk is escape times consequence. Score the failure mode, not the subsystem. | ||
|
|
||
| Escape axes — how likely a defect ships and persists: |
There was a problem hiding this comment.
Should we specify the review agent to delegate a sub agent for each axis? Wonder if that improves depth. Maybe we could a/b test some stuff like this with a complex pr example
| ## 3. The seven mandatory passes | ||
|
|
||
| Every non-trivial change gets each of these as a distinct pass. Separate, | ||
| separately-prompted reviewer agents per pass work well (see §6). Check §2's routing |
There was a problem hiding this comment.
Ah does this wording lead to sub agent spawning reliably?
There was a problem hiding this comment.
Unverified. I typically do this manually.
| @@ -0,0 +1,784 @@ | |||
| # Bug-catching guidance for baton-sdk | |||
There was a problem hiding this comment.
Are we worried about the length of this file? Wonder if the 7 passes should be split into sub pages
There was a problem hiding this comment.
Yes, we are, particularly in the case that you want to give different subagents different tasks. Thats loading a lot of the same context repeatedly.
Yeah, actually. I tested it against 3 active PRs, including two active PRs from the resync epic (see lack of convergence), the [incremental grant expansion pr](this #1013,) and the last 15 or so PRs. The approaches are generally what I learned shipping the perf improvements work (compaction/expansion), getting pebble to go, and from the first attempt at resync, which crashed and burned. Its also where this pr came from. |
Awesome thanks! |
3 participants
Add
docs/BUG_CATCHING.md— an evidence-based review rubric — and wire risk triage into the CI reviewerWhat this is
Two files:
docs/BUG_CATCHING.md— a self-contained rubric for reviewing changes in this repo, built from a retrospective across our recent efforts (fan-out, sync-replay, timeout preservability, incremental expansion, the verification-infrastructure phases). Every claim in it is backed by a shipped bug or a named effort; §7 is an index that resolves the shorthand..claude/skills/ci-review.md— the CI AI reviewer's prompt gains a risk-triage step that mirrors the rubric's §2, so every PR gets scored and HIGH-risk changes get flagged for escalation before merge.The core arguments
Review has no closure criterion. It samples the behavior space with no coverage model, so "review converged" is undefined — the sync-replay effort ran 15+ rounds without converging, and the fan-out branch produced new high-severity bugs for 5 straight rounds. The rubric budgets two to three rounds, then routes to instruments (differential oracles, cross-version harnesses, fault injection, permutation tables). Review's real job is discovering the verification plan, not substituting for one.
Risk is escape × consequence. Triage scores four escape axes (silence, durability, uncontrolled dimensions — including data volume, consumer distance) against a remediation ladder (redeploy → re-sync → migrate → coordinate → irrecoverable). Silent+durable compounds with detection latency; version-pair dependence is auto-HIGH; ladder rung ≥ 4 is auto-HIGH and flips the defense from detection to prevention.
Cost contracts. On a declared list of hot paths (grant expansion, compaction, the per-checkpoint loop, artifact-open-time work) performance is a correctness property: budgets convert latency into failure and fixed memory converts peak-RSS into OOM livelock. The reviewable object is the cost-curve delta; the enforcement is a whale-scale benchmark gate. Migrations are named as the intersection class that hits every risk factor at once.
Fix-time obligations. Failing test first for every confirmed bug, and the recurrence rule: a bug in a class this document already names must ship the ladder climb (§4), not just the patch — evidenced by converter field loss recurring 19 days after its point fix (fixes toPebble converter to preserve discovered at #997 → Add new resource attributes to pebble. #1023).
How it's meant to be used
The doc opens with a routing guide and §6 contains a slicing map: orchestrating agents inline only the sections a pass agent needs, rather than pasting the whole document. The CI reviewer does triage-and-route only — it flags HIGH, names the review-blind class and the instrument, and checks whether the PR contains it; it does not attempt the full pass set itself.
Notes for review
git log --grep.