From 13ecd4b8936c22b2f50d8b34844a0b0b56c7b500 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Wed, 22 Jul 2026 11:20:47 -0700 Subject: [PATCH 1/2] [jwies/review-trial-skill-lessons] review-trial skill: four operational lessons from the sonnet-low trial - Budget in enforcement units: the api-proxy meter, not the run-summary figure (~2.5x lower); measured basis updated to 1,050-1,077 proxy credits per full-depth run. - Arm workflows must replicate the 2500-credit trial cap; the shared 1000 default killed a round mid-tail. - Never label the arm PR with the repo opt-out: the trial workflow carries the same label gate and skips itself. Suppress by removing the installed reviewer files and any slash-command shim (review-kore-prs) from the scaffolding branch instead. - The arm PR must look real: realistic title/body; trial disclosure lives on the scaffolding PR only. Codifies the scaffolding+content two-PR pattern, including the scaffolding-branch exclusion in the arm workflow's if-gate. --- .claude/skills/review-trial/SKILL.md | 68 ++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 15 deletions(-) diff --git a/.claude/skills/review-trial/SKILL.md b/.claude/skills/review-trial/SKILL.md index 08c33dd8..74f2f7dd 100644 --- a/.claude/skills/review-trial/SKILL.md +++ b/.claude/skills/review-trial/SKILL.md @@ -41,30 +41,68 @@ missing, stop and ask; do not improvise a defect table from the branch diff. 4. **Lifecycle plan** (optional): the push-2 content (fixes mixed with fresh seeds, plus their defect-table rows) and the push-3 content (everything fixed). -5. **Budget approval**: project the cost FIRST and confirm. Measured basis: - roughly $7-10 per workflow-arm run; total is arms x (1 + lifecycle +5. **Budget approval**: project the cost FIRST and confirm. Project in the + units the cap enforces: the firewall api-proxy's credit meter + (`ai_credits_this_response` sums in the run's token-usage log), NOT the + run-summary "AI credits" figure, which reads ~2.5x lower. Measured basis: + a full-depth, full-roster run of the memory-expiration shape meters + 1,050-1,077 proxy credits (~$10.50); total is arms x (1 + lifecycle pushes), hosted-arm runs billed separately by the app. Print the projection and get an explicit yes before creating any PR. ## Step 1: isolated arm PRs -For each arm, create `trial//` from the seeded branch and open a -PR in the consumer repo (title prefixed `[reviewer-trial]`, body stating it -is a trial and will be closed unmerged). One PR per arm; never point two arms -at the same PR, or their comments contaminate each other. +Each arm is TWO branches and TWO PRs (the trial3 preview pattern): + +- A **scaffolding branch** cut from the seeded content's parent commit. It + carries everything trial-specific: the arm's compiled workflow, any + ROUTING override, and the removal of competing reviewer surfaces (below). + Open a scaffolding PR against the default branch (title prefixed + `[reviewer-trial]`, body disclosing the trial, the repo's opt-out label, + e.g. `skip-ai-review`, applied); it exists for review/audit only and is + closed unmerged. +- A **content branch** stacking the operator's seeded commits on the + scaffolding, with its PR based on the scaffolding branch, so the diff + under review is ONLY the seeded change. + +**The arm PR must look like a real PR.** The reviewer reads the PR title +and description (`pr-context.json`), so a `[reviewer-trial]` prefix, a +body describing the trial, or any mention of seeded defects taints the +run. Give the arm PR the seeded change's own realistic title and body +(reuse the prior trial PR's body verbatim when replicating); the trial +disclosure lives on the scaffolding PR and in the closing comment only. +One PR per arm; never point two arms at the same PR, or their comments +contaminate each other. Per-arm trigger setup: - `repo-default`: nothing extra; the repo's reviewer triggers normally. -- `workflow @ ref`: commit the compiled workflow for that ref onto the arm - branch. **Give it a workflow name distinct from the repo's own reviewer - and from every other arm**: same-named gh-aw workflows share a per-PR - concurrency group and silently cancel each other (this ate a run in the - original trial). Then suppress the repo's default reviewer on this PR via - its documented opt-out (`skip-ai-review` label in Khan repos), so exactly - one reviewer runs per PR. -- `hosted`: suppress the default reviewer the same way, then trigger with an - `@claude review` comment. +- `workflow @ ref`: commit the compiled workflow for that ref onto the + scaffolding branch. **Give it a workflow name distinct from the repo's + own reviewer and from every other arm**: same-named gh-aw workflows share + a per-PR concurrency group and silently cancel each other (this ate a run + in the original trial). Its `if:` must exclude the scaffolding branch + itself (`head.ref != ''`), so the scaffolding PR + burns no credits. **Replicate the trial credit cap**: set + `max-ai-credits: 2500` and the `REVIEW_MAX_AI_CREDITS: "2500"` env mirror + in the arm workflow; the shared default of 1000 sits BELOW the + proxy-metered cost of a full-depth, full-roster run (1,050-1,077 measured + on the memory-expiration shape), and a capped run dies mid-tail with the + review emitted but the cache record and artifact upload unfinished. +- `hosted`: trigger with an `@claude review` comment after suppression. + +**Suppressing every other reviewer: do NOT label the arm PR.** The shared +review workflow carries the same opt-out label gate as the repo's installed +reviewer, so the repo's documented opt-out label (`skip-ai-review` in Khan +repos) skips the TRIAL arm too (observed: 2-second skipped runs). Instead, +remove the competing surfaces from the scaffolding branch so they are absent +from the PR's merge ref: the repo's installed reviewer workflow files AND +any pull_request-triggered shim that posts the reviewer's slash command +(webapp's `review-kore-prs.yml` auto-posts `/review`, which launches the +PRODUCTION reviewer from the default branch onto the trial PR). Note the +limit: command-triggered workflows execute from the default branch, so file +removal cannot stop a human typing the slash command; the shim removal +plus nobody commenting is the actual protection. Draft PRs generally do not trigger reviewers; open the PRs ready-for-review. From 3eeaa3d7522a8abcc1909c1745933d79cb9805fe Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Thu, 30 Jul 2026 11:40:03 -0700 Subject: [PATCH 2/2] [jwies/review-trial-skill-lessons] Add changeset for the review-trial skill lessons doc change --- .changeset/review-trial-skill-lessons.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/review-trial-skill-lessons.md diff --git a/.changeset/review-trial-skill-lessons.md b/.changeset/review-trial-skill-lessons.md new file mode 100644 index 00000000..98d9e9b7 --- /dev/null +++ b/.changeset/review-trial-skill-lessons.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Skill-doc only: fold four operational lessons from the 2026-07-22 sonnet-low trial into the `review-trial` skill. (1) Cost projections and cap sizing now use the enforcement unit, the firewall api-proxy's credit meter (the sum of `ai_credits_this_response` in the run's token-usage log), rather than the run-summary AI-credits figure, which reads ~2.5x lower; the measured basis is restated as 1,050-1,077 proxy credits for a full-depth, full-roster run of the memory-expiration shape. (2) Arm workflows must carry the 2500-credit trial cap (`max-ai-credits` plus the `REVIEW_MAX_AI_CREDITS` mirror), because the shared default of 1000 sits below the metered cost of exactly the runs this skill choreographs and a capped run dies mid-tail with the review emitted but the cache record and artifact upload unfinished. (3) Never label the arm PR with the repo's opt-out: the trial workflow ships the same `skip-ai-review` gate as the installed reviewer, so the label skips the trial arm itself; suppression moves to the scaffolding branch (remove the installed reviewer files and any pull_request-triggered slash-command shim). (4) The arm PR must present a realistic title and body, since the reviewer reads both via `pr-context.json`; the trial disclosure, the label, and the `[reviewer-trial]` prefix move to the scaffolding PR, whose branch the arm workflow's `if:` now excludes so it burns no credits. No change to the shipped review workflow.