review-consumer-bump: a skill for rolling reviewer releases out to consumers - #357
Conversation
… rolling reviewer releases out to consumers
🦋 Changeset detectedLatest commit: 3aa6f35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…gin/main' into jwbron/review-consumer-bump-skill
This comment has been minimized.
This comment has been minimized.
… review's composability fixes Three of the review's non-blocking findings were real, so take them: - The .gitattributes bullets collided: git checkout -- .gitattributes after a compile also discards the stale agentics-maintenance line removal three lines below. State the ordering: the removal happens after the final compile. - The consumer-discovery search read one 100-item page with no truncation check, so a rollout past 100 consumers would silently drop the overflow. Guard on .total_count in the jq. - review-onboarding still prescribed gh aw update in four places for the exact operation this skill documents failing twice (repins review-v* tags to main's head SHA; emptied a review.md to 0 bytes). Point all four at the review-consumer-bump merge flow instead. Also number the procedural headings (Step 1..6) to match the three sibling skills, per the conventions nitpick.
|
Took three of the review's findings in 0fb321e, plus the nitpick:
Two follow-ups, deliberately not in this PR:
On the changeset question: repo precedent settles it. The Lints job fails without a changeset (baa5fc7 records exactly this), every prior skill-doc PR took a patch bump, and the changeset already says the shipped workflow is unchanged, which is the signal to skip a rollout for it. |
There was a problem hiding this comment.
Changes requested — see inline comments.
All 2 prior review threads are resolved.
Note: divergence tripwire re-armed a full review (unreviewed share 1.00).
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…viders gate, guard the merge inputs, propagate the gh aw update ban Takes the #357 review: - the Step 5 providers gate now counts the two awf-config payloads separately: the second payload is written with backslash-escaped quotes, so the old single grep counted GH_AW_INFO_MODEL_COSTS instead and its answer of 2 was a coincidence of the lock's shape (blocking) - Step 2 gains git fetch --tags and a test -s guard on both extracted files: a missing tag left a 0-byte theirs that git merge-file reads as an upstream deletion, which no downstream gate catches - dropped the 'four commands' count that miscounted the hand edit - added the 'What stays human' gate the sibling repo-mutating skills open with (consumer set and version confirmation, force-push approval) - the gh aw update ban now also covers the shipped docs: the README's install and local-edit sections and one frontmatter comment in review.md pointed consumers at the tool this skill bans
|
Took all five in d4496b4. The blocking one first: the providers gate now counts the two payloads separately, exactly per the sketch. Verified against this repo's lock: The rest:
|
…re, not just the README The review on #371 found the onboarding skill still recommended gh aw update in 4 places and the shipped review.md observability comment in 1 more, so fixing the README alone relocated the inconsistency instead of removing it. Port #357's wording for the skill's 4 spots and the shipped comment verbatim (its rebase then collapses to just the new consumer-bump skill), fix the same comment in this repo's own install and recompile the lock with gh-aw v0.85.4 (the diff is the frontmatter hash line only), point the dangling skill path at #357, and give the README ban a revisit condition: neither failure is filed upstream (searched github/gh-aw issues, no reports), so re-test on a scratch install before trusting a newer gh-aw release.
Guidance for reviewersTriage notes for reviewers: risky files by owning team, repeated changes, and files excluded from review. github-actions (2 files)
Common patterns3 files: Replace every reference to - a local edit; `gh aw update` preserves it.
+ a local edit; the 3-way merge update flow preserves it.Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or
review detailsreview-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation |
There was a problem hiding this comment.
All 5 prior review threads are resolved.
Note: divergence tripwire re-armed a full review (unreviewed share 0.67).
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| (a 3-way merge that preserves your local edits). | ||
| plus the consumer config files below. Pull future updates with the 3-way merge | ||
| flow in [`review-consumer-bump`](../../.claude/skills/review-consumer-bump/SKILL.md), | ||
| **not** `gh aw update`: the tool does not recognize the `review-v<version>` tag |
There was a problem hiding this comment.
note (non-blocking): The gh aw update doc sweep misses the checker's user-facing source-missing warning. Grepped the tracked tree for gh aw update: after this PR the only remaining live prescriptions are workflows/review/lib/check-consumer-config.ts:540 and .github/workflows/review.md:168. The latter is correct to leave alone (it is this repo's installed copy, frozen to its pinned tag by review-pins.test.ts), but line 540 is emitted output the new skill's own Step 5 tells operators to read and quote into PR bodies.
Lower-confidence observations (2)
.claude/skills/review-consumer-bump/SKILL.md:89suggestion (non-blocking): The merge procedure is deterministic and guard-heavy — it reads like a script that was written as prose. (first-principles)workflows/review/README.md:182thought (non-blocking): The upgrade procedure now lives in a maintainer agent skill while the consumer-facing doc only points at it. (first-principles)
review details
found by completeness | also flagged by holistic| 2. Its 3-way merge emptied the consumer's `review.md` to 0 bytes in one run | ||
| against `Khan/kore-marketplace` (gh-aw v0.85.4). | ||
|
|
||
| The manual merge below is what the tool would do if it worked. |
There was a problem hiding this comment.
question (non-blocking): The gh aw update ban is unconditioned and the root cause may not be upstream's. Failure (1) is that gh-aw does not read review-v<version> as a release tag because it is not bare semver — that is a consequence of this repo's own tag scheme, and failure (2) (merge emptying a file to 0 bytes) is a data-loss bug worth reporting since other Khan repos use the same tool. Was an upstream issue filed, and does the ban carry a version bound plus a condition under which it is re-evaluated?
review details
found by first-principles
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0441b7c6a581 (10 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…onvention Two review catches on the new skill. Step 3 prescribed an unconditional cp for autofix on the strength of a dated snapshot (no consumer carried local autofix edits as of 2026-08-20); the skill is written to be re-run on every future rollout, nothing downstream catches a clobbered autofix override (review-pins.test.ts covers review.md only), and autofix pushes commits to consumer PRs, so the bump is now diff-first: confirm the installed copy is clean against its pinned tag, cp when it is, merge-file with the override inventory when it is not. Step 6 named a plans-repo convention it never defined; the jira-link format and its enforcement by the gh wrapper are now spelled out inline.
There was a problem hiding this comment.
Approved — no blocking issues found.
3 of 5 prior review threads resolved; 2 still unaddressed as of 3aa6f35:
2 non-blocking threads still open
- question (non-blocking)
.claude/skills/review-consumer-bump/SKILL.md:71: Thegh aw updateban is unconditioned and the root cause may not be upstream's. Failure (1) is that gh-aw does not rea... - note (non-blocking)
workflows/review/README.md:183: Thegh aw updatedoc sweep misses the checker's user-facingsource-missingwarning. Grepped the tracked tree for `gh...
Non-blocking observations (2)
.claude/skills/review-consumer-bump/SKILL.md:41question (non-blocking): Step 1's discovery query matches only...review.md@review-vpins, so it cannot find a consumer whosesource:line is a raw SHA or an unpinned default-branch install — including the exact damage the skill's own "Do not usegh aw update" section says the tool leaves behind ("repins to the head commit of main as a raw SHA instead of the target tag"). (skill-auditor (out-of-lane)).claude/skills/review-consumer-bump/SKILL.md:170suggestion (non-blocking):--repo (consumer-path)renames the placeholder the repo's other checker invocations call(consumer). (conventions)
review details
review-v1.18.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…th (#372) * [jwbron/checker-repo-path-guard] review: check-consumer-config fails loudly on a nonexistent --repo path --repo takes a path to the consumer checkout, but an owner/name argument parses fine and resolves as a relative path: every check reported missing and the report read as a catastrophically broken install instead of a typo. Hit live during the 2026-08-20 rollout; the review-consumer-bump skill documents the footgun. The checker now throws naming the bad path before running any check. Two tests: the throw, and a positive case proving an existing root prefixes every checked path. The file sat exactly at the 1000-line max-lines cap, so the guard is paid for by deduplicating parseArgs's twice-declared inline arg type into one CliArgs alias. Follow-up from the #357 review. * review: assert warnings empty too in the repo-root prefix test The positive repo-root test asserted only errors, so warning-only paths (the .gitattributes lookup) escaped its 'prefixes every path' claim.
…ine a comment Two findings from a reviewer run the dispatch gate blocked before posting (run 32758584548; the gate report carries them). The changeset still described the repo-wide sweep #357 already landed, crediting this release with edits it did not make; it now describes only this PR's remaining diff. And the <STOP: ...> line inside the sh fence was shell-hostile: bash parses it as a redirect, so a paste errors on that line and runs gh aw compile anyway. It is now a # comment, paste-safe and matching the two comment lines above it.
* [jwbron/review-readme-manual-merge] review: stop recommending gh aw update in the consumer README The README told consumers to pull updates with gh aw update in 4 places. The review-consumer-bump skill (#357) documents it failing twice against this repo's tag scheme, both observed live on 2026-08-20: it treats review-v<version> as a branch and repins to main's head SHA, and its 3-way merge emptied a consumer's installed review.md to 0 bytes (gh-aw v0.85.4). All 4 spots now describe the maintainer-driven bump flow (a manual git merge-file 3-way merge, one PR per consumer) and the install section names the failure modes so nobody rediscovers them. Follow-up from the #357 review; lands independently of #357, the skill-path reference just dangles for whichever merges first. * [jwbron/review-readme-manual-merge] review: ban gh aw update everywhere, not just the README The review on #371 found the onboarding skill still recommended gh aw update in 4 places and the shipped review.md observability comment in 1 more, so fixing the README alone relocated the inconsistency instead of removing it. Port #357's wording for the skill's 4 spots and the shipped comment verbatim (its rebase then collapses to just the new consumer-bump skill), fix the same comment in this repo's own install and recompile the lock with gh-aw v0.85.4 (the diff is the frontmatter hash line only), point the dangling skill path at #357, and give the README ban a revisit condition: neither failure is filed upstream (searched github/gh-aw issues, no reports), so re-test on a scratch install before trusting a newer gh-aw release. * review: sweep the last runtime gh aw update mention; give the update block a stop line Two review catches: check-consumer-config's source-missing warning still framed gh aw update as the update mechanism (the one place a consumer met the stale recommendation at runtime rather than in prose), and the onboarding skill's update block had lost its only merge step, so following it literally recompiled an unmerged install and every later step silently passed on an empty diff. The warning now cites the reason source: still matters (the manual bump flow reads it), and the block carries an explicit unrunnable stop line where the merge command used to be. * review: rewrite the changeset for the post-#357 diff; make the stop line a comment Two findings from a reviewer run the dispatch gate blocked before posting (run 32758584548; the gate report carries them). The changeset still described the repo-wide sweep #357 already landed, crediting this release with edits it did not make; it now describes only this PR's remaining diff. And the <STOP: ...> line inside the sh fence was shell-hostile: bash parses it as a redirect, so a paste errors on that line and runs gh aw compile anyway. It is now a # comment, paste-safe and matching the two comment lines above it. * review: soften the stop-comment claim, put the revisit condition in the bump skill, guard the ban in CI Three from the latest round plus the earlier fold: the changeset claimed a shell comment makes running the block straight through impossible (it cannot; the claim ships verbatim as the CHANGELOG entry, so it is softened to what the comment does), the no-expiry/re-test-on-scratch-install condition now lives in the bump skill the operator actually follows rather than only the consumer README, and a new test pins every tracked gh aw update mention to the files that ban it, so re-recommending the tool anywhere fails red. * review: the ban sweep scans every tracked file and spares the CHANGELOG The blocking catch: changeset version copies changeset bodies verbatim into workflows/review/CHANGELOG.md, so excluding .changeset/ but not the file it concatenates into just deferred the failure to the release PR. Also from the round: the eval corpus is excluded (case fixtures quote arbitrary text), the extension glob is gone entirely (the residue has turned up in .md, .ts, and a compiled .lock.yml, so an extension list is a bet on where the next one lands; reading a binary as utf8 cannot match), the ALLOWED docstring names all five entries including this file's own search string, and the header states the file-granularity limit so nobody over-trusts the backstop.
Adds
.claude/skills/review-consumer-bump/SKILL.md: the playbook for rolling areview-v<version>(and optionallyautofix-v<version>) release out to every consuming repo, one PR per consumer. Written from the 2026-08-20 rollout (kore-marketplace#11, agent-settings#76, webapp#41661, #356), where every pitfall it records was hit live.The judgment calls it carries:
source:line each time, never from a list in the skill (lists drift; the search found exactly the 4 current consumers when tested, and the command in the skill is the tested percent-encoded form since the raw query 400s intermittently)gh aw updateis explicitly banned with the two observed failures: it treatsreview-v<version>tags as branches and repins to main's head SHA, and its 3-way merge emptied a consumer's review.md to 0 bytesgit merge-file, with the LOCAL OVERRIDE inventory check before and aftermerge=oursgitattribute: revert; gh-aw v0.85.x deleting agentics-maintenance.yml: keep)providersin both awf-config payloads of the lock, and the consumer-config checker run from a version-matched tag checkout with a path (not a name) as--repo, both of which failed silently or confusingly when done wrong this weekMostly skill-doc,
"review": patchchangeset per the #306 precedent. Review feedback also had thegh aw updateban propagated to the shipped docs:workflows/review/README.mdnow points updates at the merge flow instead of the tool, and one frontmatter comment inworkflows/review/review.mddrops the "gh aw update preserves it" claim. No behavior change to the shipped workflow.