Conversation
npm SnapshotWant to try this PR's changes before it merges? Comment |
|
Size Change: 0 B Total Size: 499 kB ℹ️ View Unchanged
|
… literal ship-blocker marker in reviewer docs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Commented — medium-importance findings found; nothing blocks.
Lower-confidence observations (6; top: `.github/aw/review/ROUTING:37` suggestion (non-blocking): The "test and dev surfaces drop back down" block is shadowed inside the two high-risk src subtrees, because the high rul...)
.github/aw/review/ROUTING:37suggestion (non-blocking): The "test and dev surfaces drop back down" block is shadowed inside the two high-risk src subtrees, because the high rules come later and last match wins. (correctness-reviewer).github/aw/review/ROUTING:74question (non-blocking): ROUTING raises.github/workflows/**and.github/aw/review/**totier=high lens=security-auth, but.github/aw/actions-lock.json— the third-party GitHub Action SHA pin file added in this same PR — matches no rule and falls to the default; this run's routing.json confirms".github/aw/actions-lock.json": "Low". (skill-auditor (out-of-lane)).github/aw/review/ROUTING:50suggestion (non-blocking): ROUTING under-tiers two surfaces that risk-classification.md and skills.md declare higher-risk:accessibility-instructions.mdandpackages/*/package.json(first-principles).gitattributes:2note (non-blocking):merge=oursis inert without a[merge "ours"] driver = truegit config, which this repo does not ship (holistic).gitattributes:1question (non-blocking):.gitattributesnow drives review tiering (risk-classification.md: "Files matchinglinguist-generatedpatterns in.gitattributes... (skill-auditor (out-of-lane)).github/aw/review/ci-tooling.md:37question (non-blocking): Does thefix-workflowsSHA pinned in validate-workflows.yml predate v3.4.0, the release that stopped it reformatting*.lock.yml? (holistic)
review details
review-v1.24.0 | schema 2 | depth full | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=full verdict=COMMENT anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiNmY0ZDQxOGFmN2NlNzZjYiJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbImNmY2E3NjEyNTFiOGY3N2EiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyJlMDFiNzM0MzcxOTlhYzkzIl0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19
There was a problem hiding this comment.
Changes requested — see inline comments.
2 of 3 prior review threads resolved; 1 still unaddressed as of 0d52c53:
1 non-blocking thread still open
- suggestion (non-blocking)
.github/aw/review/ci-tooling.md: The Jest bullet overstates CI coverage: node-ci runs Jest full-or-limited, not the full suite, and there is no Codecov i...
Lower-confidence observations (5; top: `.github/aw/review/ROUTING:53` suggestion (non-blocking): `patches/**` has no tier rule, so pnpm-applied dependency patches fall to the default `low` despite being the same "hand...)
.github/aw/review/ROUTING:53suggestion (non-blocking):patches/**has no tier rule, so pnpm-applied dependency patches fall to the defaultlowdespite being the same "hand-patched, no upstream review" category asvendor/**. (correctness-reviewer).github/aw/review/ci-tooling.md:55suggestion (non-blocking, best-practice): Reviewer suppression narrows CLAUDE.md's test-generator defaults rule (skill-auditor).github/aw/review/ROUTING:43suggestion (non-blocking): The agent-config tiers added to ROUTING (.claude/hooks/**,.claude/settings.json,SKILL.md/CLAUDE.md/AGENTS.md,/accessibility-instructions.md) have no counterpart in risk-classification.md (holistic).github/aw/review/ci-tooling.md:10suggestion (non-blocking): ci-tooling.md hand-transcribes node-ci.yml's gate list into standing suppressions with nothing tying the two files together (first-principles).github/aw/review/ROUTING:83nitpick (non-blocking): Both new files end without a trailing newline, against the repo-wideinsert_final_newline = truein.editorconfig(conventions)
review details
review-v1.24.0 | schema 2 | depth scoped | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=scoped verdict=REQUEST_CHANGES anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiMjg1MzkxNWQxMTA0NWY5MiJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbIjFkNzQ3MDA4OTkzYTRjN2IiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyJlMDFiNzM0MzcxOTlhYzkzIl0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19Drop direction-dependent on reviewer config, fix fixture and Chromatic paths, add typetest/patches rules
There was a problem hiding this comment.
All 5 prior review threads are resolved.
Lower-confidence observations (7; top: `.github/aw/review/ROUTING:44` suggestion (non-blocking): The `tier=high` hook rules cover `.claude/hooks/**` only, leaving the tracked `.codex/hooks/**` and `.cursor/hooks/**` c...)
.github/aw/review/ROUTING:44suggestion (non-blocking): Thetier=highhook rules cover.claude/hooks/**only, leaving the tracked.codex/hooks/**and.cursor/hooks/**copies of the same auto-executing code at the defaultlow. (correctness-reviewer).github/aw/review/ci-tooling.md:11suggestion (non-blocking): The lint entry names the wrong switch: full-vs-limited ESLint is driven by changes to eslint config files, not by the PR's base branch, and the full command coverspackages/only. (correctness-reviewer).github/aw/review/ROUTING:44note (non-blocking): PR description says.claude/hooks/*deliberately falls to the router's defaultlow, but ROUTING routes it (and.claude/settings.json) athigh(completeness).github/aw/review/ci-tooling.md:43suggestion (non-blocking):pnpm pre-publish-checkis listed as PR coverage, but its node-ci job only runs on thechangeset-release/mainPR. (correctness-reviewer).github/aw/review/ROUTING:45suggestion (non-blocking):.mcp.jsonis agent-executed config but is left at the router's defaultlowwhile.claude/hooks/**and.claude/settings.jsonwere raised tohigh(first-principles).github/aw/review/ROUTING:72thought (non-blocking): The.github/aw/review/**high tier cannot protect against config tampering, because the router reads this file from the PR's own head branch (first-principles).github/aw/review/ROUTING:44suggestion (non-blocking): Skill reference files fall to Trivial via**/*.md: theSKILL.mdbasename rule doesn't reach.agents/skills/wonder-blocks/references/forms.md(holistic)
review details
review-v1.24.0 | schema 2 | depth scoped | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=scoped verdict=APPROVE anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiOWJiMGU0ODczYzQ0YmY2OSJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbImI5YzliNDUzZjYyYjczZTMiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyJmYWYzYTQzZDZmODlmMTY2Il0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19|
|
||
| Files matching `linguist-generated` patterns in `.gitattributes` | ||
| (`.github/workflows/*.lock.yml`, `agentics-maintenance.yml`), `pnpm-lock.yaml`, and | ||
| `**/__snapshots__/**`. A generated file edited *without* its source changing is a red |
There was a problem hiding this comment.
suggestion (non-blocking): "Classify by the manual edit" cannot be carried out: the router never stages a generated file's contents, so a hand-edited review.lock.yml reaches no reviewer.
context
"Classify by the manual edit" cannot be carried out: the router never stages a generated file's contents, so a hand-edited review.lock.yml reaches no reviewer. In router.ts the isGenerated branch (around line 591) short-circuits before lens and tier resolution: generated files get tier: "trivial", lenses: [], and the comment "contents are not analysed". The shipped checker agrees — check-consumer-config.ts --explain .github/workflows/review.lock.yml prints tier trivial (generated) even though the .github/workflows/** tier=high rule matches. The only workflow that runs on .github/** yml changes is validate-workflows.yml, which just applies fix-workflows-v3 and pushes; nothing recompiles the lock from review.md and fails on a diff. So the sentence describes a control this configuration does not have. Introduced by this change (the .gitattributes mark and this prose land together). Worth either dropping the claim or adding a gh aw compile + git diff --exit-code check so lock drift fails CI.
A sketch, not a committable replacement:
`**/__snapshots__/**`. The router short-circuits generated files (`tier: trivial`, no
lenses, contents not analysed), so a hand-edit to a lock file reaches no reviewer here —
it has to be caught by a human or by a CI step that recompiles the lock from its source
and fails on a diff.
found by correctness-reviewer
Tier Codex/Cursor hooks and .mcp.json high; correct lint and pre-publish claims in ci-tooling
There was a problem hiding this comment.
Commented — medium-importance findings found; nothing blocks.
3 of 3 prior review threads are still unaddressed as of 28349dc:
3 non-blocking threads still open
- suggestion (non-blocking)
.github/aw/review/ROUTING:77:.gitattributescan suppress review of any path but is not raised tohigh/lens=security-authlike the other reviewe... - suggestion (non-blocking)
.github/aw/review/ROUTING:93: Theregression-tests/**trivial rule also swallowsparse-perseus-json-regression.test.ts, the harness CI does not pr... - suggestion (non-blocking)
.github/aw/review/risk-classification.md:18: "Classify by the manual edit" cannot be carried out: the router never stages a generated file's contents, so a hand-edit...
Lower-confidence observations (2; top: `.github/aw/review/ROUTING:40` suggestion (non-blocking): Skill `references/*.md` fall to Trivial via `**/*.md`, but they are loaded into agent context by the same mechanism as t...)
.github/aw/review/ROUTING:40suggestion (non-blocking): Skillreferences/*.mdfall to Trivial via**/*.md, but they are loaded into agent context by the same mechanism as the Medium-tier SKILL.md (first-principles).github/aw/review/ROUTING:22question (non-blocking): No specialist lens is routed for product code — wascontent-i18nconsidered forpackages/**, given Perseus's translated-strings surface? (completeness)
review details
review-v1.24.0 | schema 2 | depth scoped | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=scoped verdict=COMMENT anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiZmU2N2ViMWVmZDY4NGU1OSJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbIjU1ZjVlOGNjN2IxMjE2NjQiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyIxZTAzZjZkMmJkOWRmNDM0Il0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19…view Tier .gitattributes and token setup script high; narrow fixture rule to data dirs; utils to medium
There was a problem hiding this comment.
Commented — medium-importance findings found; nothing blocks.
5 of 6 prior review threads resolved; 1 still unaddressed as of 3a8bbe7:
1 non-blocking thread still open (1 previously reported)
- suggestion (non-blocking)
.github/aw/review/risk-classification.md:18
Lower-confidence observations (2; top: `.github/aw/review/ROUTING:71` suggestion (non-blocking): ROUTING matches `**/action.yml` but not `**/action.yaml`, so an action definition using the equally-valid `.yaml` spelli...)
.github/aw/review/ROUTING:71suggestion (non-blocking): ROUTING matches**/action.ymlbut not**/action.yaml, so an action definition using the equally-valid.yamlspelling gets nosecurity-authlens and the defaultlowtier. (security-auth).github/aw/review/ROUTING:57question (non-blocking): Was routing a compatibility lens (e.g. api-federation-compat / cross-deploy-serialization) onto the schema and parser paths considered, or is security-auth deliberately the only lens? (completeness)
review details
review-v1.24.0 | schema 2 | depth scoped | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=scoped verdict=COMMENT anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiNzBjMjA4ZDk5YmM4YTQ2NyJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbIjY1N2FkMzMxOTA5MDliZTciXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyIxZTAzZjZkMmJkOWRmNDM0Il0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19
There was a problem hiding this comment.
Commented — medium-importance findings found; nothing blocks.
1 of 1 prior review thread is still unaddressed as of 25c6742:
1 non-blocking thread still open (1 previously reported)
- suggestion (non-blocking)
.github/aw/review/risk-classification.md:18
Lower-confidence observations (3; top: `.github/aw/review/ROUTING:44` suggestion (non-blocking): The always-loaded agent prose block misses `.cursor/rules/**`, the Cursor counterpart of the `CLAUDE.md`/`AGENTS.md` rul...)
.github/aw/review/ROUTING:44suggestion (non-blocking): The always-loaded agent prose block misses.cursor/rules/**, the Cursor counterpart of theCLAUDE.md/AGENTS.mdrules it raises to medium. (correctness-reviewer).github/aw/review/ROUTING:25suggestion (non-blocking):config/** tier=lowalso coversconfig/build/, the rollup config that produces the published npm bundles, which the neighbouringutils/**comment's own reasoning would put at medium. (correctness-reviewer).github/aw/review/ROUTING:90suggestion (non-blocking): The trailing test block's unscoped**/globs also downgrade any future test-named file under.claude/hooks/**,vendor/**,patches/**, or the.githubsecurity-auth paths — no instance exists today. (security-auth)
review details
review-v1.24.0 | schema 2 | depth scoped | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=scoped verdict=COMMENT anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiMmZjZTczYTc3MDM2NDkyNSJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbImRmMDgxNWIxOWE5YzA5MGEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyIxZTAzZjZkMmJkOWRmNDM0Il0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19
There was a problem hiding this comment.
Commented — medium-importance findings found; nothing blocks.
3 of 4 prior review threads resolved; 1 still unaddressed as of 58084ab:
1 non-blocking thread still open (1 previously reported)
- suggestion (non-blocking)
.github/aw/review/risk-classification.md:18
Lower-confidence observations (2; top: `.github/aw/review/ci-tooling.md:7` thought (non-blocking): ci-tooling.md hard-codes a snapshot of what node-ci enforces, and nothing fails when CI and this file drift apart.)
.github/aw/review/ci-tooling.md:7thought (non-blocking): ci-tooling.md hard-codes a snapshot of what node-ci enforces, and nothing fails when CI and this file drift apart. (holistic).github/aw/review/ROUTING:32nitpick (non-blocking):data/questions/uses a trailing-slash directory form that this file's documented grammar never describes and no other directory rule uses (conventions)
review details
review-v1.24.0 | schema 2 | depth scoped | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=scoped verdict=COMMENT anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiOGZhZjRjNmE2OWJhODZiMyJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbImIxZWRlNGNhMTY0ODZlMjgiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyJkMjAwZGZlZmExZGY1YWJmIl0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19| `packages/perseus-core/src/parse-perseus-json/regression-tests/{item,article,renderer,user-input}-data`. | ||
| Don't warn about edits there; CI blocks them outright. (Adding new fixtures is | ||
| allowed and worth a look.) | ||
| - **Visual regressions** — Chromatic (`chromatic-pr.yml`, which calls |
There was a problem hiding this comment.
suggestion (non-blocking): ci-tooling.md tells the reviewer Chromatic snapshots "all Storybook stories", but chromatic-build.yml restricts PR snapshots to **/*-regression.stories.tsx
context
ci-tooling.md tells the reviewer Chromatic snapshots "all Storybook stories", but chromatic-build.yml restricts PR snapshots to **/*-regression.stories.tsx. .github/workflows/chromatic-build.yml passes onlyStoryFiles: "**/*-regression.stories.tsx" on both publishing paths (lines 65 and 96, with the comment "TurboSnap cannot run with onlyStoryFiles"), so any story file not named *-regression.stories.tsx is never captured. The added sentence reads "snapshots all Storybook stories *-regression.stories.tsx files" — two claims spliced together, the first of which is false and the wider of the two, so the "Don't flag purely visual styling concerns" instruction attached to it suppresses findings on the majority of components that have no regression story. Same bullet: that job uses exitOnceUploaded: true, so the workflow step returns before comparison and it is Chromatic's own commit status, not a CI job, that surfaces unapproved diffs — "blocks on unapproved visual diffs" overstates the gate. The PR description states round three "tightened the ci-tooling.md claims to match what node-ci actually runs"; the other claims in this file do check out (prettier/knip/stopship/typecheck/tstyche/build:types/jest in node-ci.yml, the "Verify changeset entries" job, pre_publish_check gated on github.head_ref == 'changeset-release/main'), so this bullet is the one left half-edited.
A sketch, not a committable replacement:
- **Visual regressions** — Chromatic (`chromatic-pr.yml`, which calls
`chromatic-build.yml`) snapshots only `**/*-regression.stories.tsx` stories
(`onlyStoryFiles`) and uploads with `exitOnceUploaded`, so Chromatic's own commit
status — not a CI job — reports unapproved diffs, and a human approves them. Don't
re-flag a visual diff on a component that HAS a `*-regression` story; a component
without one has no visual coverage at all, so a visual concern there is worth
raising. DO flag behavioral or a11y consequences of styling changes (contrast,
focus visibility), which Chromatic doesn't judge.
found by completeness | also flagged by correctness-reviewer (at line 34): The Chromatic bullet reads as full Storybook coverage, but onlyStoryFiles limits snapshots to *-regression.stories.tsx — 51 of the repo's 180 story files.; skill-auditor (out-of-lane) (at line 34)
| `vendor/**`. | ||
| - **Agent hooks and settings** — `.claude/hooks/**`, `.codex/hooks/**`, | ||
| `.codex/hooks.json`, `.cursor/hooks/**`, `.cursor/hooks.json`, | ||
| `.cursor/sandbox.json`, `.claude/settings.json`, `.mcp.json`. Code and server |
There was a problem hiding this comment.
thought (non-blocking): The path→tier map is written twice (ROUTING + risk-classification.md) and the two copies already disagree in two places
context
The path→tier map is written twice (ROUTING + risk-classification.md) and the two copies already disagree in two places. ROUTING is the deterministic map; risk-classification.md restates the same assignments as prose. Two concrete divergences already exist in this diff. (1) risk-classification.md:48-51 lists .cursor/sandbox.json in the High Risk "agent hooks and settings" bullet, but ROUTING has rules for .cursor/hooks/** and .cursor/hooks.json and none for .cursor/sandbox.json — the file exists in the checkout (test -e .cursor/sandbox.json → EXISTS), so per the PR description's own account of the remaining dot-configs it resolves to the router's default low. (2) ROUTING:23-24 sets utils/** tier=medium with the comment "utils/ holds the scripts CI runs to gate PRs and publish to npm, so not low", while risk-classification.md:69-72 lists utils/ scripts under Low Risk. Both copies were hand-maintained through three live tuning rounds and still diverged, which suggests the duplication — not either individual line — is the thing to change: the prose could carry only the reasoning per tier (which is what the model actually needs and what ROUTING cannot express) and defer all path→tier statements to ROUTING, leaving one editable source for the map. Was restating the paths in prose a deliberate redundancy for the model's benefit, or a byproduct of writing the two files separately?
found by first-principles
…ication when already defined in ROUTING.md
There was a problem hiding this comment.
Commented — medium-importance findings found; nothing blocks.
1 of 4 prior review threads resolved; 3 still unaddressed as of 7733ead:
3 non-blocking threads still open (1 previously reported)
- suggestion (non-blocking)
.github/aw/review/ci-tooling.md:33: ci-tooling.md tells the reviewer Chromatic snapshots "all Storybook stories", but chromatic-build.yml restricts PR snaps... - thought (non-blocking)
.github/aw/review/risk-classification.md: The path→tier map is written twice (ROUTING + risk-classification.md) and the two copies already disagree in two places - suggestion (non-blocking)
.github/aw/review/risk-classification.md:18
review details
review-v1.24.0 | schema 2 | depth scoped | re-review scoped | enable holistic,completeness,test-adequacy,first-principles,conventionsreview fingerprint
pr-reviewer:rereview v=1 depth=scoped verdict=COMMENT anchor-draft=false hunks=eyIuY2hhbmdlc2V0L3NoYXJwLWxhbXBzLWVhdC5tZCI6WyI3MmRhOGIyNjJiNjRjM2Q4Il0sIi5naXRhdHRyaWJ1dGVzIjpbIjI2OWY5NDAyNGQ4NTMzMWIiXSwiLmdpdGh1Yi9hdy9hY3Rpb25zLWxvY2suanNvbiI6WyIwZWY4MmY2MDAwZGU2YjEwIl0sIi5naXRodWIvYXcvcmV2aWV3L1JPVVRJTkciOlsiMDA2ZTQwYjFiYzUzNzU5NCJdLCIuZ2l0aHViL2F3L3Jldmlldy9jaS10b29saW5nLm1kIjpbIjQ1ZGJlMzhlYWQ5NWFkZDIiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvY29uZmlnLm1kIjpbIjcyYmVhZGRiNDYyYzFmZmEiXSwiLmdpdGh1Yi9hdy9yZXZpZXcvcmlzay1jbGFzc2lmaWNhdGlvbi5tZCI6WyJjNDg0NTgwYzNmYTRkNDcwIl0sIi5naXRodWIvYXcvcmV2aWV3L3NraWxscy5tZCI6WyJmZTNlMGU4NjNkZWZlNmNmIl0sIi5wcmV0dGllcmlnbm9yZSI6WyJkM2VmYmY4YmMzM2VmYTJlIl19| ### Generated files are automatically Trivial | ||
|
|
||
| Files matching `linguist-generated` patterns in `.gitattributes` | ||
| (`.github/workflows/*.lock.yml`, `agentics-maintenance.yml`), `pnpm-lock.yaml`, and |
There was a problem hiding this comment.
suggestion (non-blocking): review.lock.yml — the workflow GitHub actually runs — is stripped from the reviewer's diff by the linguist-generated marking, and no gate verifies it matches review.md.
context
review.lock.yml is the file GitHub actually executes, and this PR makes it the one workflow file the reviewer's whole-change and lens agents never read.
The chain, all verified in this checkout: .gitattributes:1 marks .github/workflows/*.lock.yml linguist-generated=true; the section on this line makes anything matching those patterns automatically Trivial; routing.json for this run lists .github/workflows/review.lock.yml under generatedFiles at tier Trivial, so ROUTING L73's .github/workflows/** lens=security-auth tier=high never applies to it. The shared workflow then stages full-stripped.diff — documented in review.md as "the full diff with the sections of every file the router classified generated (routing.json generatedFiles) removed" — and that is the copy every whole-change reviewer and specialist lens reads; only pattern-triage, whose job is classification, sees the unstripped diff. (Both review.md and review.lock.yml are also absent from this PR's diff with hasPatch: false, but that is GitHub omitting patches for very large files, not the strip — review.md carries it too at tier High.)
The marking itself is what the shared workflow wants: its own consumer-config checker warns when *.lock.yml is not marked linguist-generated, precisely so the reviewer doesn't line-review its own compiler output. The gap is that nothing proves the lock is that compiler output. The activation job's 'Check workflow lock file' step (review.lock.yml:191-201) is check_workflow_timestamp_api.cjs, a commit-timestamp comparison — editing only the lock makes it newer than the .md, which is the passing direction. validate-workflows.yml runs only the fix-workflows ordering autofix, not gh aw compile. So the file's authority ("it's generated, don't read it") rests on a convention no gate enforces, while the sentence two lines below — "A generated file edited without its source changing is a red flag — classify by the manual edit" — asks a reviewer to classify content it is never given.
The cheap fix is a CI step that recompiles and diffs: gh aw compile followed by git diff --exit-code .github/workflows/*.lock.yml. With that in place, stripping the lock from review is sound, because the lock can then only be the deterministic output of the review.md the reviewer does route at high tier.
A sketch, not a committable replacement:
# .github/workflows/validate-workflows.yml — add alongside the existing autofix job
- name: Verify gh-aw lock files are a faithful compile
run: |
gh extension install githubnext/gh-aw --pin v0.85.4
gh aw compile
git diff --exit-code -- '.github/workflows/*.lock.yml' \
|| { echo '::error::review.lock.yml does not match its .md source; run `gh aw compile` and commit the result.'; exit 1; }
found by security-auth | also flagged by holistic (at line 18)
| .github/aw/actions-lock.json lens=security-auth tier=high | ||
| # A linguist-generated line here hides a path from review entirely, so this file is | ||
| # reviewer control input too. | ||
| /.gitattributes lens=security-auth tier=high |
There was a problem hiding this comment.
suggestion (non-blocking): .github/CODEOWNERS — the repo's mandatory-approver control — matches no ROUTING pattern and falls to the default low tier, unlike the /.gitattributes rule right above it.
context
/.gitattributes is raised here for a good reason — it is reviewer control input, and a line in it can hide a path from review. .github/CODEOWNERS is the same class of file and is not routed at all.
I walked every pattern in this file against .github/CODEOWNERS and none matches, so it lands on the router's default tier (low): reduced depth, no security-auth lens. That is the file deciding who is required to approve a PR in this repo, and the PR description itself cites it as the control that makes bot reviewer requests redundant. A PR dropping the @Khan/perseus catch-all, or reassigning a path's owners, would be reviewed as a low-risk config tweak.
One line alongside the /.gitattributes rule would close it:
/.github/CODEOWNERS lens=security-auth tier=high
Same for /.github/REVIEWERS if Gerald is ever turned on here.
A sketch, not a committable replacement:
# A linguist-generated line here hides a path from review entirely, so this file is
# reviewer control input too.
/.gitattributes lens=security-auth tier=high
# Same class: CODEOWNERS decides who is required to approve a PR at all.
/.github/CODEOWNERS lens=security-auth tier=high
found by security-auth
2 participants
Issue: LEMS-4590
Enables the Khan shared AI PR reviewer (
Khan/actionsworkflows/review, pinned atreview-v1.24.0) on this repo.What was auto-generated
Produced by
gh aw add Khan/actions/workflows/review/review.md@review-v1.24.0andgh aw compile(gh-aw v0.85.4 — deliberately pinned: gh-aw ≥ v0.87 rejects the tag's frontmatter under a new strict-mode rule, flagged to the workflow maintainers). Compiler output, never hand-edited:.github/workflows/review.md.github/workflows/review.lock.ymlgh aw compile— the workflow GitHub actually runs.github/aw/actions-lock.jsongh aw compile— third-party action SHA pins (unmarked in.gitattributeson purpose; the pins are worth reading).gitattributesmarks the locklinguist-generatedso it collapses in diffs and the reviewer skips its own compiler output. (It also pre-marksagentics-maintenance.yml, which this gh-aw version doesn't emit yet but newer ones generate unconditionally.)What was hand-written, and why
The five files under
.github/aw/review/plus four labeledPERSEUS LOCAL OVERRIDE:edits inreview.md— that's the whole review surface.Blast radius the tiers derive from: Perseus is a published npm library rendering and scoring every exercise and article on Khan Academy; wrong code ships via changeset releases into webapp/frontend production, breaking rendering, answer scoring (perseus-score grades server-side), or accessibility for learners.
perseus-score, the data-schema + parsers,vendor/andpatches/, publish config (.changeset/config.json,packages/*/package.json), workflows and the reviewer's own config, and agent-executed code (.claude/hooks/**,.codex/hooks/**,.cursor/hooks/**,.claude/settings.json,.mcp.json) arehigh; shipped package src and agent prose (SKILL.md,CLAUDE.md,AGENTS.md,accessibility-instructions.md) aremedium; tests/typetests/stories/dev surfaceslow; docs, CI-frozen parser fixtures, and thedata/questionsanalysis corpustrivial. About 60 remaining dot-config files (.codex/config.toml,.claude/launch.json, etc.) intentionally fall to the router's defaultlow. Skillreferences/*.mdstay trivial on purpose: documentation, not executable prose.enable holistic,completeness,test-adequacy,first-principles,conventions+re-review scoped): mirrors Khan/webapp's eval-justified set minusdocumentation, which webapp enables only to feed its autofix workflow (no autofix here).allowed-team-reviewers: [perseus]as a safety bound: perseus doesn't use Gerald (no.github/REVIEWERS), so the bot finds no owners to request, and CODEOWNERS already auto-requests@Khan/perseuson every PR. The allowlist is kept anyway because in gh-aw an absent allowlist accepts any reviewer the agent names;[perseus]caps a prompt-injected request at one harmless duplicate ping.review.md(all labeledPERSEUS LOCAL OVERRIDE): credit ceiling 1000 → 2500 (+ its env mirror; shipped value dies below a full high-tier run), observability block commented out (noGH_AW_OTEL_SENTRY_*secrets; while present they're hard-required),*.sentry.ioremoved from the network allowlist (only served that exporter), fork guard added to theif:(public repo; shippedroles: alldisables gh-aw's actor gate),source:normalized from the resolved SHA to the tag.Validated with the consumer-config checker from a
review-v1.24.0checkout: 0 errors, 1 warning (the ~60 default-low dot-config files above, deliberately).Three live rounds of the reviewer on this PR shaped the config: round one caught a real ROUTING shadowing bug (the low-tier test block sat before the high-tier src rules, so tests under
perseus-scoreresolved high); round two caught thatdirection-dependenton.github/aw/review/**let the router offer Trivial as a candidate (via**/*.md) and skip this PR's own config files, so that flag is now off with a comment explaining why; round three tightened theci-tooling.mdclaims to match what node-ci actually runs. Every factual claim the reviewer made about this repo checked out on verification.