review: stop recommending gh aw update in the consumer README - #371
Conversation
…pdate 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.
🦋 Changeset detectedLatest commit: fa9ee8f 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 |
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
…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.
|
All four review findings addressed in 9c901e3:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
All 4 prior review threads are resolved.
Note: divergence tripwire re-armed a full review (unreviewed share 0.78).
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…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.
# Conflicts: # .github/workflows/review.lock.yml
There was a problem hiding this comment.
Approved — no blocking issues found.
4 of 6 prior review threads resolved; 2 still unaddressed as of 7d260c3:
2 non-blocking threads still open
- note (non-blocking)
.claude/skills/review-onboarding/SKILL.md:172: Every replaced recommendation points at.claude/skills/review-consumer-bump/SKILL.md, which does not exist on this bra... - suggestion (non-blocking)
workflows/review/README.md:184: PR citations elsewhere in the docs are bareKhan/actions#NNN, not markdown links. Every tracked-markdown PR reference...
Non-blocking observations (4)
.claude/skills/review-onboarding/SKILL.md:431suggestion (non-blocking): Manual step written as<STOP: ...>inside a ```sh block rather than as prose between blocks. (conventions)workflows/review/README.md:183question (non-blocking): The bump playbook the SKILL.md pointers name does not exist in the repo yet, and those pointers don't name #357. (completeness)workflows/review/lib/check-consumer-config.ts:540suggestion (non-blocking): Changeset says "Docs only" but this PR also changes a shipped checker warning string. (completeness)workflows/review/lib/check-consumer-config.ts:540suggestion (non-blocking): The ban is enforced only in prose, in the same shipped file that could detect the failure it warns about. (first-principles)
review details
review-v1.18.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation# Conflicts: # .claude/skills/review-onboarding/SKILL.md # workflows/review/README.md
…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.
Guidance for reviewersTriage notes for reviewers: risky files by owning team, repeated changes, and files excluded from review. github-actions (3 files)
Common patterns4 files: Replace - a local edit `gh aw update` preserves
+ a local edit the 3-way merge update flow preservesExcluded from review (1 file)Not individually reviewed — generated, formatting-only, or
review detailsreview-v1.18.0 | 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 2 prior review threads are resolved.
Note: divergence tripwire re-armed a full review (unreviewed share 0.60).
review details
review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…he 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.
There was a problem hiding this comment.
Changes requested — see inline comments.
All 5 prior review threads are resolved.
Note: divergence tripwire re-armed a full review (unreviewed share 0.43).
review details
review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentationThe 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.
There was a problem hiding this comment.
Approved — no blocking issues found.
All 5 prior review threads are resolved.
Non-blocking observations (4)
workflows/review/gh-aw-update-ban.test.ts:68suggestion (non-blocking): The sweep matches the literal substring, so a line-wrapped mention slips past the ban. (correctness-reviewer)workflows/review/gh-aw-update-ban.test.ts:76suggestion (non-blocking): The stale-allowlist detector's custom message never renders for the missing-file case. (correctness-reviewer)workflows/review/gh-aw-update-ban.test.ts:70suggestion (non-blocking): the sweep has no positive control, so a broadened exclusion would pass silently. (test-adequacy)workflows/review/gh-aw-update-ban.test.ts:51suggestion (non-blocking):git grep -ldoes this sweep in the one subprocess the test already spawns. (first-principles)
review details
review-v1.18.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…ce (#375) Run 32758584548 (PR #371) was blocked by rule 7 on a fully conforming review: the staged plan quoted <STOP: ...> entity-escaped in a footer, the ingest sanitizer decoded it and parenthesised the resulting pseudo-tag, and the normalized comparison saw different bodies. Entity decoding was on the documented-not-absorbed residual list; this absorbs it by mirroring sanitize_content_core.cjs's decodeHtmlEntities (v0.85.4) on both sides of the comparison, before the invisible strips, in the sanitizer's order.
The consequential one is sequencing: the dispatcher-death prose said 'report the run incomplete, then continue at Step 9', and reporting incomplete can end the turn, so the cache compensation the death path exists to trigger could be skipped with it. Step 9 now runs first, and the guidance-repost claim is scoped to the next full-depth run. The ban test folds whitespace before matching (a wrapped 'gh aw update' counted as no mention), gains a positive control so a broadened exclusion fails rather than passing vacuously, and checks existence before reading so the stale-allowlist message renders; git grep -l was considered and declined in a comment, since line-based matching is exactly what misses the wrapped form. foldToken's three length thresholds are documented and the empty-subject-tokens early return is pinned.
Follow-up to #357's
gh aw updateban. Most of this PR's original diff (the README and onboarding-skill rewording) landed on main via #357 itself, so what remains is the residue that PR did not carry, plus hardening from the review rounds:source-missingwarning no longer framesgh aw updateas the update mechanism; it cites the reasonsource:still matters (the manual bump flow reads it to tell which release the install was copied from)gh aw compile, telling the reader to run the 3-way merge and commit it before compiling (advisory; the first round's<STOP: ...>pseudo-tag form was not valid shell)workflows/review/gh-aw-update-ban.test.tspins every trackedgh aw updatemention to the files that ban it, so re-recommending the tool anywhere fails CIreview.mdpicks up the frontmatter comment fix (the observability local-edit note no longer namesgh aw updateas the merger); lock recompiled with gh-aw v0.85.4, comment-only, no behavior changeKORE-2498