Skip to content

review-trial skill: operational lessons from the sonnet-low trial #291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 3 commits into from
Jul 30, 2026
Merged

review-trial skill: operational lessons from the sonnet-low trial #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13ecd4b
[jwies/review-trial-skill-lessons] review-trial skill: four operation…
jwbron Jul 22, 2026
6b406ea
[jwies/review-trial-skill-lessons] Merge main into jwies/review-trial…
jwbron Jul 30, 2026
3eeaa3d
[jwies/review-trial-skill-lessons] Add changeset for the review-trial…
jwbron Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/review-trial-skill-lessons.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
68 changes: 53 additions & 15 deletions .claude/skills/review-trial/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

@github-actions github-actions Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): This budget guidance switches to proxy-credit units and warns the run-summary figure reads ~2.5x lower, but the untouched Step 2 collection instruction (line 119) still records cost as "billed AI credits from the run summary," and Step 4's table reports that. The trial is then projected in one currency and scored in another differing ~2.5x, with no reconciliation given. Consider updating Step 2 to collect the proxy-metered figure (or record both, labeled).

Lower-confidence notes (2)
  • .claude/skills/review-trial/SKILL.md:59 — "removal of competing reviewer surfaces" reads unconditionally, but for the repo-default arm the installed reviewer is the arm; a clause exempting that arm ("its own reviewer is not a competing surface") would remove the ambiguity.
  • .claude/skills/review-trial/SKILL.md:88 — if the shared default of 1000 sits below a full-depth run's metered cost, the durable fix may be upstream (raise the default / reconcile the meter) rather than a per-trial override.

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/<slug>/<arm>` 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

@github-actions github-actions Bot Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): Restructured Step 1 drops the trial/<slug>/* branch-naming convention that Step 6 cleanup still relies on.

The old Step 1 named branches via create trial/<slug>/<arm> (removed). The new two-branch text names only a "scaffolding branch" and a "content branch" with no naming scheme, yet Step 6 (line 179, untouched) still deletes trial/<slug>/* branches and the if: guard references a <scaffolding branch> placeholder. The naming scheme is referenced but no longer defined.

Name the two branches under the trial/<slug>/ prefix — e.g. trial/<slug>/<arm>-scaffold and trial/<slug>/<arm> — so Step 6's glob-delete and the if: exclusion have a defined target. Otherwise an operator who names the branches outside that prefix runs Step 6's delete trial/<slug>/* and the glob matches nothing, leaving the trial branches and the compiled arm workflow alive on the repo — the exact residue Step 6 warns against.

1 lower-confidence note
  • thought (non-blocking) .claude/skills/review-trial/SKILL.md:68 — The realistic-title lesson removes every human-visible trial marker from the arm PR itself. pr-context.json exposes only title/description, so a label would be invisible to the reviewer anyway; a neutral non-opt-out label on the arm PR would still let a repo member who stumbles on it see it is synthetic without tainting the run. Worth considering only after confirming the arm workflow gate keys solely on the opt-out label.

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 != '<scaffolding branch>'`), 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

@github-actions github-actions Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

note (non-blocking): These measured figures (1,050-1,077 credits, and the 2500 cap) are a single point-in-time measurement of the memory-expiration shape, now baked into three places in the file. A larger PR shape, roster change, or repricing invalidates them with no signal in the doc. Consider stating the durable rule (cap must exceed the Step-5 projection with margin, verified against a recent run's proxy meter) and citing the 2026-07-22 numbers as a dated example.

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.

Expand Down
Loading