Skip to content

Bring the AI-review callers up to main, and stop any label triggering a review - #839

Merged
cb1kenobi merged 7 commits into
mainfrom
kris/bump-ai-review-pin
Sep 21, 2026
Merged

cb1kenobi merged 7 commits into
mainfrom
kris/bump-ai-review-pin

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 6, 2026 •

Copy link
Copy Markdown
Member

This updates the Claude and Gemini review callers from the July ai-review-prompts revision to the September revision, restricts label events to each provider's own opt-in label, and replays a labeled draft when it becomes ready. Review feedback removed every pnpm change from this PR, leaving package-manager policy to #844 and the final diff limited to the two review callers plus their validator.

For the human reviewer

  1. Cancellation ownership: cancellation moves from the caller workflow to the reusable's provider-and-PR-scoped review job after authorization. This prevents an unrelated or unauthorized event from cancelling an admitted review. The pinned Claude reusable and Gemini reusable implement the claimed job-level groups. The tradeoff is deliberate: in opt-in mode, a later push does not invoke the reusable and therefore does not cancel an already-admitted review of the prior SHA. Restoring caller concurrency is easy, but would restore the unrelated-event cancellation bug.
  2. Opt-in label lifetime: claude-review and gemini-review remain one-shot gestures, with ready_for_review as the draft replay point, rather than sticky subscriptions to every later push or reopen. Making them sticky is a one-line expression change per provider, but increases paid review frequency and should remain symmetric.
  3. Atomic reusable upgrade: one SHA upgrades workflow logic, authorization, scripts, and review layers together instead of selectively adopting pieces. The 19-commit comparison is the review surface, and af81872e was verified as identical to ai-review-prompts main. Rollback is a paired one-SHA change in each caller.
  4. Verification strategy: this PR relies on the reusable repository's workflow-contract tests and this repository's live caller validator rather than adding a second truth-table harness. The pinned validator covers both claude-* and gemini-* callers; adding consumer-side tests later is straightforward if that upstream contract proves insufficient.

Changes

Verification

  • pnpm build — production bundle and native binding built successfully.
  • pnpm check — type-check, oxlint, and oxfmt checks passed.
  • pnpm test — 65 files passed, 1 skipped; 906 tests passed, 9 skipped.
  • pnpm test:native — 194 tests passed across 30 suites.
  • Static workflow checks — all three changed workflows parse as YAML; six reusable/input refs match; both providers contain the provider-specific label and ready_for_review gates; no caller-level concurrency block remains.
  • Scope check — all six pnpm workflow files are byte-for-byte identical to the merge base, and the final PR diff contains only the three AI-review workflow files described above.
  • Upstream contract check — af81872e is identical to ai-review-prompts main; both reusable review jobs use distinct provider-and-PR concurrency groups after authorization.
  • Final independent pre-push review (Claude + Gemini + Harper-domain) covered 6aad6c36 and converged with no unaddressed defect. Its one in-scope tradeoff and one pre-existing policy item are captured above.

The end-to-end route is the live Validate caller workflows check on this pushed head; the runtime/library tests are corroboration because this PR does not change shipped code.

Complexity: medium

🤖 Generated with Claude Code; updated with Codex

Review-Coverage: authored=codex; ran=gemini,claude; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=6 @ 6aad6c3

Human-Review-Need: 3 (decisions: cancellation-ownership, opt-in-label-lifetime, atomic-reusable-upgrade, verification-strategy) @ 6aad6c3

@kriszyp
kriszyp requested a review from cb1kenobi as a code owner September 6, 2026 13:29
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@github-actions

github-actions Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 24.51K ops/sec 40.81 39.41 545.023 0.112 122,529
🥈 rocksdb 2 11.13K ops/sec 89.81 86.11 24,207.151 0.979 55,674

getSync() > sequential keys - small key size (100 records)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 28.66K ops/sec 34.89 33.70 536.907 0.104 143,302
🥈 rocksdb 2 11.87K ops/sec 84.25 81.79 3,710.212 0.149 59,349

ranges.bench.ts

getRange() > small range (100 records, 50 range)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 24.22K ops/sec 41.30 37.87 1,731.846 0.285 121,076
🥈 rocksdb 2 15.25K ops/sec 65.57 57.69 1,128.905 0.123 76,260

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 345.78 ops/sec 2,892.009 54.86 65,559.929 18.85 692
🥈 lmdb 2 26.64 ops/sec 37,537.981 52.72 1,179,294.532 136.72 64.00

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 37.48K ops/sec 26.68 12.28 20,899.456 0.851 187,406
🥈 lmdb 2 443.97 ops/sec 2,252.406 172.395 14,656.027 1.40 2,220

Transaction log > read one entry from random position from log with 1000 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 704.48K ops/sec 1.42 1.22 4,116.191 0.229 3,522,413
🥈 lmdb 2 475.76K ops/sec 2.10 1.07 946.604 0.284 2,378,808

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 836.90 ops/sec 1,194.881 1,049.6 2,055.418 0.279 1,674
🥈 lmdb 2 1.16 ops/sec 864,078.691 816,431.742 927,677.786 3.04 10.00

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 21.45K ops/sec 46.62 29.42 20,372.085 2.07 42,899
🥈 lmdb 2 801.87 ops/sec 1,247.079 142.573 13,923.424 5.40 1,605

Results from commit ef6fd3d

kriszyp and others added 6 commits September 8, 2026 17:20
… a review

Two problems, found while auditing CI review coverage for the 2026-09-05 DLC
post-mortem.

**The pin is two months stale.** These callers pin ai-review-prompts at
1255f2e1 (main, 2026-07-10) while harper and harper-pro are both on af81872e
(main, 2026-09-01). Everything that moves together moves through that pin —
workflow logic, layer files, bash scripts, auth-gate behavior — so this repo has
been reviewing against July's lenses and July's auth gate. Bumped in all three
callers (claude-review, gemini-review, validate-caller-workflows), both the
`uses:` ref and the paired `ai-review-prompts-ref`.

Checked before bumping: between the two refs the reusables add only OPTIONAL
inputs (effort, effort-by-size, debounce-seconds, skip-when-only,
expected-review-author). No new required input and no new required secret, and
these callers already pass every required one.

**Any label fired a full review.** The gate was:

    types: [..., labeled]
    if: vars.CLAUDE_ALWAYS_ON == 'true' || github.event.action == 'labeled'

so adding ANY label to ANY PR — `dependencies`, `wip`, anything — triggered a
complete AI review. The label gesture is supposed to admit the `claude-review` /
`gemini-review` label specifically. Replaced with harper's maintained gate, which
matches the label by NAME and additionally admits `ready_for_review` while the
opt-in label is still on the PR, so a draft that opted in resumes review when it
flips ready even with the always-on variable unset.

Note this does NOT turn on always-on review here: `CLAUDE_ALWAYS_ON` /
`GEMINI_ALWAYS_ON` are still unset at repo level, so both remain opt-in via
label. That is a repo-variable change, deliberately left to a human.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep GitHub Actions on the last known-green pnpm version instead of resolving the mutable latest tag across major versions.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Remove legacy caller-level concurrency after adopting the reusable workflows' post-authorization cancellation. This prevents unrelated label events from cancelling an admitted review before the caller job gate skips the replacement.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Document that callers prefilter provider labels while the reusable workflows retain final authorization.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp
kriszyp force-pushed the kris/bump-ai-review-pin branch from 8ba4702 to f2e2d85 Compare September 8, 2026 23:44
Comment thread .github/workflows/benchmark-asan.yml Outdated

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the ai-review-prompts updates, but as evident by #844, we don't need to touch the pnpm stuff.

Remove the twelve workflow-local pnpm pins so this PR stays focused on the AI-review callers and leaves the package-manager policy to PR #844.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@cb1kenobi
cb1kenobi merged commit 46bcd51 into main Sep 21, 2026
44 of 45 checks passed
@cb1kenobi
cb1kenobi deleted the kris/bump-ai-review-pin branch September 21, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants