review-trial skill: operational lessons from the sonnet-low trial - #291
Conversation
…al 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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: |
There was a problem hiding this comment.
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 therepo-defaultarm 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.
| 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 |
There was a problem hiding this comment.
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.
… skill lessons doc change
Review Guidance`@Khan/github-actions` — 1 file worth a closer human look
|
There was a problem hiding this comment.
2 of 2 prior review threads are still unaddressed as of 3eeaa3d:
2 non-blocking threads still open
- suggestion (non-blocking)
.claude/skills/review-trial/SKILL.md:47: This budget guidance switches to proxy-credit units and warns the run-summary figure reads ~2.5x lower, but the untouched Steps 2-3 still collect the figure from the run summary. - note (non-blocking)
.claude/skills/review-trial/SKILL.md:89: These measured figures (1,050-1,077 credits, and the 2500 cap) are a single point-in-time measurement of the memory-expiry behaviour.
| 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 |
There was a problem hiding this comment.
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.jsonexposes 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.
…t; make `stampSource` the authoritative anchoring signal The Lints job failed for want of a changeset. Skill-doc changes take a `review` patch, matching #291. Review feedback, both non-blocking: - The recap carried the force-push ban but not the rule the section says actually matters, leaving a skimming operator unprotected against record loss, which is the real anchor-loss cause. Adds the `stampSource` line. - The carrier internals (hunk-hash tolerance, `commitSha` written but never read back) are accurate today but are incidental implementation properties, not contracts. They now sit under the `stampSource`-is-authoritative rule, dated, with a note saying which half goes stale if a later change starts validating `commitSha` on restore.
#306) * [jwies/trial-protocol-hygiene] review-trial: record three operational lessons from the preview trials Three things the last two trials taught the hard way, none of which the skill said: - Never force-push a trial branch between rounds. A round that cannot anchor on a prior fingerprint reports `stampSource: null` and executes `depth: full` with `staging: whole-diff` even where ROUTING says `scoped`, which looks identical to a genuine carrier gap and voids the scoped-cost sample. The carrier itself is content-hashed and rebase stable (`rereview-mode.ts`) and its `commitSha` is never read back, so the thing that actually loses the anchor is losing the record; a force-pushed round is just where that goes unnoticed. Read `stampSource` out of the plan artifact every round instead of inferring it. - Give each trial's compiled workflow a fresh lock filename. GitHub keys the workflow display name to the file path and keeps the stale one, so two trials that reused `review-preview.lock.yml` both listed as "PR Reviewer" while their files declared "PR Reviewer Preview v1.8.0" and "...v1.9.0". Concurrency was unaffected, but the run list invites the "did production just run?" scare the naming rule exists to prevent. - A lifecycle plan that means to test open-thread suppression has to leave one finding unfixed; if the fixer repairs everything the reconciler resolves every thread, `threads.json` is empty, and the round proves nothing either way. * [jwies/trial-protocol-hygiene] review-trial: add the missing changeset; make `stampSource` the authoritative anchoring signal The Lints job failed for want of a changeset. Skill-doc changes take a `review` patch, matching #291. Review feedback, both non-blocking: - The recap carried the force-push ban but not the rule the section says actually matters, leaving a skimming operator unprotected against record loss, which is the real anchor-loss cause. Adds the `stampSource` line. - The carrier internals (hunk-hash tolerance, `commitSha` written but never read back) are accurate today but are incidental implementation properties, not contracts. They now sit under the `stampSource`-is-authoritative rule, dated, with a note saying which half goes stale if a later change starts validating `commitSha` on restore. * [jwies/trial-protocol-hygiene] review-trial: two more lessons; count runs by events, keep injections out of the diff From the 2026-07-31 suppression trial on webapp#41204. Counting runs by events, not pushes. A closed PR fires no pull_request event, so pushes onto it are free; `reopened` is in the reviewer's trigger list, so a reopen fires exactly one run; pushing to an open PR fires `synchronize`, one more each time. Push-while-closed then reopen therefore buys a round for one run. The larger saving is not building the round at all: a closed PR whose last round's comments are still unresolved already IS a partially-fixed round, and reopening webapp#41204 produced the round that made open-thread suppression fire in production for the first time (run 30650642317) against a two-run budget. Unresolving a thread is a GraphQL mutation an agent's tooling may refuse, so it belongs to the operator. Keeping fault injections out of the diff under review. Mis-staging to trip a fail-open reporter works (run 30654454047 reported unusableThreads: 9), but injecting through the prompt or lock file on the trial branch makes that edit the newly-changed code: all five of that run's candidates landed on the prompt file and none on the Go fixture, so the duplicate re-posting the reporter warns about went unobserved, and the round was spent correctly flagging the injected prose as agent-directing instructions. Cleanup gains the corollary the closed-PR shortcut needs. Keeping a resumable branch is fine, but it is reverted to a safe state in the pass that closes the PR, with the resumable details (unresolved threads, run IDs, what a reopen fires) in the closing comment: a surviving branch that still carries a planted panic is a trap, and the reopen reviews immediately.
2 participants
Codifies what the 2026-07-22 sonnet-low trial paid to learn (full context: the round-1 report and closing note on #290). Four changes to the skill doc:
ai_credits_this_responsein the run's token-usage log), not the run-summary AI-credits figure, which reads about 2.5x lower. Measured basis updated: a full-depth, full-roster run of the memory-expiration shape meters 1,050-1,077 proxy credits.max-ai-creditsplus theREVIEW_MAX_AI_CREDITSmirror). The shared default of 1000 sits below the measured cost of exactly the runs this skill choreographs; my first attempt died at 1,016 with the review emitted but the cache record and artifact upload unfinished.skip-ai-reviewgate as the installed reviewer, so the label skips the trial arm itself (observed as 2-second skipped runs). Suppression is done on the scaffolding branch instead: remove the installed reviewer files and any pull_request-triggered slash-command shim (webapp'sreview-kore-prs.ymlauto-posted/reviewand launched the production reviewer onto a trial PR before this was caught).pr-context.json, so the skill's old instruction to prefix[reviewer-trial]and describe the trial in the body taints the run. The realistic surface goes on the arm PR; the disclosure, the label, and the trial framing go on the scaffolding PR, whose branch the arm workflow'sif:now explicitly excludes so it burns no credits.Doc-only change; no code or workflow files touched.