review: an onboarding playbook and a consumer-config checker for new consumer repos - #317
Merged
Conversation
…and a consumer-config checker for new consumer repos Onboarding a repo onto the shared reviewer is five hand-written config files, two local edits to the installed review.md, and a handful of admin blockers only a repo admin can clear. Every mistake in that set fails late and quietly, which is what both halves of this change address. lib/check-consumer-config.ts validates an install through the production parsers rather than a reimplementation: route(), parseRoutingConfig, and parseGitattributesGenerated. It ships in lib/ and is run from the tag the consumer pins, so a release that changes those semantics changes its answers for free (and a mismatch between checker and pin is itself one of its warnings). lib/frontmatter.ts is the minimal structural frontmatter reader it needs; no YAML dependency in lib/. Comment lines are dropped, so a commented-out block reads as absent, which is exactly what disabling observability: means. .claude/skills/review-onboarding/SKILL.md is the judgment half: what stays the operator's call, the preflight inventory that becomes ci-tooling.md and skills.md, the local edits and when each applies, how to derive tiers from the repo's own blast radius, and the PR-body structure that separates what is generated from what was decided. No change to the shipped review workflow.
🦋 Changeset detectedLatest commit: 7d1f4c1 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 |
Contributor
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
khan-actions-bot
requested review from
a team,
jaredly and
somewhatabstract
and removed request for
a team
August 3, 2026 19:31
jwbron
marked this pull request as draft
August 3, 2026 19:37
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| It reads the install through the *production* parsers (`route()`, | ||
| `parseRoutingConfig`, `parseGitattributesGenerated`), so it never drifts from | ||
| what a review actually does, and it reports the whole class of mistakes that | ||
| otherwise surface as a red run on someone's PR: a missing runtime import, a |
Contributor
There was a problem hiding this comment.
thought (non-blocking): The check catalog is restated in prose in four places (changeset, README, SKILL Step 5, module docstring). Since the issue codes are fixed tokens, a single generated enumeration (e.g. a --list-checks flag) would keep the docs from drifting as checks are added or renamed.
…ng team say so, and mark the other generated workflow Two fixes from onboarding Khan/agent-settings (Khan/agent-settings#48), the first real consumer this skill and checker drove end to end. Both are cases where the checker was right about the mechanism and wrong about the verdict. An empty `allowed-team-reviewers` was a hard error. But `.github/REVIEWERS` is the router's ONLY source of team ownership, so in a repo without one, Step 8 derives no owners and no ranked fallback and requests nobody no matter what the allowlist says. There the empty allowlist is an accurate "this repo does not request reviewers", not a dropped request, and erroring only pressures the author into naming an inert team to satisfy the check -- which is what agent-settings did on its first pass, entering a team with no access to that repo. It is now an error only when REVIEWERS exists (requests get computed, then silently dropped) and a `reviewer-requests-inert` warning when it does not, reported because the state is otherwise invisible on the PR. `config-no-bot-token` goes quiet in the same case, so a deliberate no-requests install reports one finding rather than two. REVIEWERS_PATH is exported from router.ts so the checker asks the router for that fact rather than restating the path. The `.gitattributes` marker the skill prescribes covers `*.lock.yml`, but `gh aw compile` also writes `agentics-maintenance.yml` (~600 lines, regenerated unconditionally -- deleting it does not stick), and that name does not match. So a repo that followed the instructions exactly still had the reviewer line-reviewing compiler output. The checker now flags it, and the skill prescribes both markers and warns not to mark `review.md`, which is the hand-editable source. Also from the same onboarding, in the skill: `gh aw add` records the resolved commit SHA rather than the tag you asked for, so `source:` needs normalising or it reads as `source-ref-mismatch`; `git ls-files` lists only TRACKED files, so an unstaged `.github/aw/review/` makes every rule pointing at it look like it matches nothing; `.gitattributes` markers must sit outside any machine-managed block in repos that have one; `gh aw compile` can stop on its safe-update secret gate; and `gh aw add` fails outright when a placeholder file sits where it wants a directory.
… checker's rendering into its own module
The previous commit's additions pushed check-consumer-config.ts to 1026 lines,
over the repo's 1000-line max-lines budget, and carried two prettier/escape
errors in the test. CI caught all three.
Rather than shave comments to squeeze under the cap by a few lines, this takes the
split the file's own docstring already implies ("the report is data, rendered
separately"): renderReport moves to check-consumer-config-report.ts, the same
by-concern split glob-match.ts took from router.ts for the same reason. The
checker re-exports it, so it stays the single entry point and no importer changes.
That leaves 901 lines and headroom for the next check, instead of a file that
tips over again on the next addition.
Verified with the repo's own eslint and prettier this time (the earlier run could
not resolve plugins from inside a nested worktree, which is why the lint errors
reached CI): eslint clean over workflows/, utils/, actions/, config/ and types/,
prettier clean, 113 tests passing, tsc clean.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
12 of 12 prior review threads are still unaddressed as of 3cbfc7b:
- issue (blocking)
workflows/review/lib/frontmatter.ts:52: Inline#comments aren't stripped from values or list items, so a labeled local edit silently breaks the check. `yamlL...
11 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/README.md:160: The checker runs by hand at onboarding, but the failure it targets (config failing late and quietly) recurs on every lat... - thought (non-blocking)
workflows/review/README.md:174: The check catalog is restated in prose in four places (changeset, README, SKILL Step 5, module docstring). Since the iss... - suggestion (non-blocking)
workflows/review/lib/check-consumer-config.ts:123:SHIPPED_MAX_AI_CREDITS = 1000is hardcoded even though the shippedreview.md(one dir up, at the pinned tag) declare... - suggestion (non-blocking)
workflows/review/lib/check-consumer-config.ts:280:importsConfigcompares rawline.item, so a quoted entry- ".github/aw/review/config.md"(valid YAML) produces a fa... - suggestion (non-blocking)
workflows/review/lib/check-consumer-config.ts:308: A flow-style listallowed-team-reviewers: [kore](valid YAML — the shippedreview.mduses `toolsets: [pull_requests,... - nitpick (non-blocking)
workflows/review/lib/check-consumer-config.ts:415: This warns "ROUTING sets nore-reviewmode" wheneverreReviewMode === DEFAULT_RE_REVIEW_MODE, but `parseRoutingConfi... - suggestion (non-blocking)
workflows/review/lib/check-consumer-config.ts:444: ThelensPayloadWarnings()forwarding loop has no test, unlike the sibling ROUTING-parser forwarding (which has a "forw... - note (non-blocking)
workflows/review/lib/check-consumer-config.ts:465: The renamed-workflow--workflowpath and its.lock.ymlderivation are untested; a refactor of the lock-path regex wo... - note (non-blocking)
workflows/review/lib/check-consumer-config.ts:498: Thesource-missingbranch (areview.mdwith nosource:field at all) isn't covered — the pinned-source tests exerc... - question (non-blocking)
workflows/review/lib/check-consumer-config.ts:525: Themax-ai-creditscheck reads only thereview.mdfrontmatter scalar, butresolveCreditCap(credit-cap.ts) reads t... - question (non-blocking)
workflows/review/lib/frontmatter.ts:5: This hand-rolled reader reimplements the layer gh-aw parses as real YAML, which is in tension with the checker's "never...
| installed.maxAiCredits <= SHIPPED_MAX_AI_CREDITS | ||
| ) { | ||
| warn( | ||
| "max-ai-credits-default", |
Contributor
There was a problem hiding this comment.
thought (non-blocking): Two of the checker's warnings compensate for shipped defaults this repo owns and could just fix upstream. I checked the shipped workflows/review/review.md: it carries max-ai-credits: 1000 (line 313) and a live observability: block (line 163). The PR's own evidence says 1000 is known to kill every tier=high run after computing a verdict, that both known consumers raise it to 2500, and that 2500 is 'a ceiling, not a spend' — by that argument raising the shipped default is strictly safe and removes the failure class, the local edit, the env mirror, and this warning for all consumers at once. Likewise, shipping observability: commented out (or documented as opt-in) would turn 'a missing secret kills the agent job at startup' from a per-consumer trap into a non-event. The PR lists bumping this repo's own pin as a follow-up but not fixing the defaults that generate the checker's work.
A sketch, not a committable replacement:
Keep the checks as a backstop, but file/land the upstream fixes: ship `max-ai-credits: 2500` and an inert-by-default `observability:` block in review.md, so these two warnings become legacy-install-only.
…not be copied, and prefer an org secret Two additions to the onboarding skill's secret handling, prompted by "can you pull the key from webapp and push it to agent-settings?". It cannot be done, and saying so explicitly is worth a paragraph: Actions secrets are write-only (encrypted against the repo public key on write, decrypted only into a runner), the single-secret GET returns name and timestamps with no value field, and there is no `gh secret get`. A future reader should not spend time looking for that path or assume the prohibition is only policy. More usefully, the per-repo paste is avoidable. An organisation secret with selected-repository visibility turns every future onboarding's hardest blocker into an org admin adding one repo to a list, with no key material moving and nothing for this skill to handle. The skill now checks what the target repo can already see via the organization-secrets endpoint, and distinguishes three outcomes: already provisioned (no blocker), org secret exists but this repo is not scoped to it (an org-admin list change), or repo-level paste as the last resort.
…to read, and document the update flow Two gaps in the onboarding skill, both found by using it. Step 7 asked for a "what's autogenerated" table but never told the author to say what is LEFT. An install PR is a few thousand lines of which all but a few hundred are compiler output, so a reviewer who is not handed the split either reviews the wrong file or skims everything. It now requires naming the human-written part (the five config files, the .gitattributes lines, the local edits), pointing at where the judgment is concentrated, and flagging two counter-intuitive details: the .gitattributes markers make GitHub collapse the generated files, so a small-looking diff is the marker working rather than a small change; and actions-lock.json is generated but deliberately not marked, because which third-party action code runs in CI is worth reading. The skill's own description claimed it covered "refreshing or auditing an existing install" while having no steps for it. There is now an Updating section: read the changelog between the two versions first (semver is a behaviour contract, so a minor can change what gets reviewed), then verify each local edit survived the 3-way merge (checking max-ai-credits and its env mirror together, since a merge can update one), then look for newly generated files needing a .gitattributes marker (how agentics-maintenance.yml arrived), then re-run the checker from the NEW tag and diff its tier preview against the old one, then re-read ci-tooling.md and skills.md against the repo as it is now -- the parts that rot silently, where a stale skills.md path degrades skill-auditor on every review. Also: bump one minor at a time when far behind, so each changelog explains its own diff.
jwbron
added a commit
that referenced
this pull request
Aug 4, 2026
…ng to the router's default The consumer-config checker (#317) reported that 27 of this repo's 430 tracked files matched no `tier=` rule and silently took the router's default `low`. Most were harmless, but one group was not. `workflows/autofix/lib/*.ts` was `low`. That code decides what the autofix workflow does and it PUSHES COMMITS to PRs, so it has at least the blast radius of `workflows/review/lib/**` sitting next to it at `high`. It was low purely because no rule named it. Also now `high`: `.github/aw/actions-lock.json` (the SHA pins for every third-party action our compiled workflows run), `.github-staging/**` (a workflow staged for a human `git mv` into .github/workflows/ is CI the moment it moves, and is reviewed here or nowhere), and `workflows/*/package{,-lock}.json` (review.md's pre-agent step runs `npm ci` against the released lockfile inside a consumer's CI, so a dependency added here executes in every consuming repo). Medium for config that shapes releases, review routing, or what the reviewer can see, without being executable: .changeset/config.json, .github/REVIEWERS (the only source of the router's team ownership), .gitattributes (which files the reviewer skips as generated), root package.json, and tsconfig.json. Low, explicitly rather than by default, for the dev-only remainder: lint/format/test wiring, type shims, .github/NOTIFIED, and eval output. Checker now reports 0 errors and 0 warnings over all 430 files; high goes 100 -> 109. Each tier is spot-checked with --explain rather than inferred, including that autofix's own tests still fall to low via `**/*.test.ts`.
…alues, so valid YAML stops reading as broken Addresses the review feedback on this PR. The blocking finding and three of the non-blocking ones are one root cause: the frontmatter reader returned raw text, so several valid YAML spellings read as absent or unparseable. For a checker whose contract is "errors must be zero", a false error is the worst failure mode, and one of these fired on the flow the skill itself prescribes. - Inline comments are stripped from values and list items, respecting quotes and `url#frag`. SKILL.md Step 2 tells authors to label every local edit with a comment, so `max-ai-credits: 2500 # LOCAL OVERRIDE` was the expected shape -- and it made Number() return NaN, silently suppressing the credit-ceiling check (NaN <= 1000 is false). A labelled `source:` reported a spurious source-ref-mismatch, and a labelled `imports` item produced a false workflow-missing-config-import error, which exits 1 on a valid install. - scalar() strips surrounding quotes, so `max-ai-credits: "1000"` parses instead of yielding NaN. - importsConfig compares through items(), so a quoted import item is not read as a missing import. - New list() reads both block and flow style. `allowed-team-reviewers: [kore]` is the spelling the shipped review.md uses for `toolsets: [pull_requests, repos]`, and it was reporting a false config-empty-team-allowlist error. list() returns undefined only when the key is ABSENT, and that distinction fixes something the previous commit got wrong. It treated any empty allowlist as the deliberate no-requests configuration, which meant a flow-style allowlist the reader could not parse would have been reported as "that is a valid configuration" -- an all-clear over an allowlist the safe output was dropping. Now a present-but-empty key is always an error, and only an absent key defers to .github/REVIEWERS. 13 new frontmatter tests and 5 new checker tests, each one a spelling that previously produced a false error or a silently-suppressed check. Verified against the real Khan/agent-settings install: still 0 errors, and the one warning is the deliberate reviewer-requests-inert note. Not addressed here, and left as open threads: whether lib/ should take a real YAML dependency rather than hand-rolling this, the four-way restatement of the check catalog, and several test-coverage notes.
jwbron
added a commit
that referenced
this pull request
Aug 5, 2026
…322) * [review-tier-unrated-paths] review: tier the 27 paths that were falling to the router's default The consumer-config checker (#317) reported that 27 of this repo's 430 tracked files matched no `tier=` rule and silently took the router's default `low`. Most were harmless, but one group was not. `workflows/autofix/lib/*.ts` was `low`. That code decides what the autofix workflow does and it PUSHES COMMITS to PRs, so it has at least the blast radius of `workflows/review/lib/**` sitting next to it at `high`. It was low purely because no rule named it. Also now `high`: `.github/aw/actions-lock.json` (the SHA pins for every third-party action our compiled workflows run), `.github-staging/**` (a workflow staged for a human `git mv` into .github/workflows/ is CI the moment it moves, and is reviewed here or nowhere), and `workflows/*/package{,-lock}.json` (review.md's pre-agent step runs `npm ci` against the released lockfile inside a consumer's CI, so a dependency added here executes in every consuming repo). Medium for config that shapes releases, review routing, or what the reviewer can see, without being executable: .changeset/config.json, .github/REVIEWERS (the only source of the router's team ownership), .gitattributes (which files the reviewer skips as generated), root package.json, and tsconfig.json. Low, explicitly rather than by default, for the dev-only remainder: lint/format/test wiring, type shims, .github/NOTIFIED, and eval output. Checker now reports 0 errors and 0 warnings over all 430 files; high goes 100 -> 109. Each tier is spot-checked with --explain rather than inferred, including that autofix's own tests still fall to low via `**/*.test.ts`. * [review-tier-unrated-paths] review: re-raise staged .md workflows, and align the prose with ROUTING Two review findings on #322. `.github-staging/** tier=high` was silently defeated for `.md` files. Tier resolution is last-match-wins, and the broad `**/*.md tier=trivial` rule sits below it, so a staged agentic workflow's `.md` source (the frontmatter carrying permissions, secrets and network, plus the prompt) routed as trivial while only the compiled `.lock.yml` got `high`. That is backwards, and it defeats the rule's whole reason for existing. `.github/workflows/*.md tier=high` already exists to re-raise workflow sources after the docs rule; this adds the same re-raise for the staging pen. Verified with the repo's own parseRoutingConfig + matchesGlob: `.github-staging/workflows/review.md` went trivial -> high, and `.github-staging/**/*.md` matches at depth 1 as well as deeper. The sweep over all 430 tracked files is byte-identical before and after (111 high, or 109 after the generated-file exemption), confirming the rule is purely prospective: nothing under `.github-staging/` is a `.md` today. risk-classification.md (model-facing prose, injected into the same reviewer) had drifted from ROUTING and this PR would have codified the split. Aligned: the autofix lib, the staging pen, `workflows/*/package{,-lock}.json` and the gh-aw action lockfile move to High; `config/`, `types/`, `.eslintrc.js` and `pnpm-workspace.yaml` drop to Low, where ROUTING already puts them and where their actual contents belong (vitest setup and ambient `.d.ts` shims); `tsconfig.json` stays Medium on its own since it decides typecheck coverage; `.changeset/config.json`, `.github/REVIEWERS`, `.gitattributes` and the agent-steering prose dirs are named at Medium. Left alone deliberately: `workflows/review/eval/*.ts` is Medium in the prose and `low` in ROUTING. That divergence predates this PR and resolving it is a real tier decision about eval review depth, not a consistency edit.
* [review-gitattributes-negation] review: honour linguist-generated=false the way git does The router read .gitattributes as a set of "generated globs" and asked whether a path matched any of them. Git resolves an attribute per path by the LAST matching line, so a negation after a broad glob is how a repo says "this subtree is generated, except this part". Parsing discarded those negations, so the reviewer silently skipped review of exactly the files a repo had gone out of its way to keep visible. parseGitattributesGenerated now returns ordered GeneratedRule[] instead of a flat pattern list, keeping negations rather than dropping them, and isGenerated scans in reverse and returns the first matching rule's verdict. A line that never mentions the attribute is not a rule at all, so it cannot shadow one. RouterConfig.generatedPatterns is renamed to generatedRules to match what it now carries. Found onboarding Khan/agent-settings (Khan/agent-settings#48), whose .gitattributes marks the installer-written .claude/**, .codex/**, .cursor/** and .pi/** output generated and then un-marks .claude/skills/** and .pi/git/** with a comment saying to keep them visible. Its new-repo-config skill -- executable prose steering every Claude session in that repo -- resolved to `trivial (generated)` and was excluded from review. It now resolves to `medium`. Blast radius is narrow and one-directional (strictly more review, never less): only a repo that writes a negation after a broader =true glob changes at all. Khan/actions and Khan/frontend have none, and webapp's single negation counters Linguist's content heuristic rather than an earlier .gitattributes rule, so no earlier rule matches that path and its classification is unchanged. The gitattributes tests move to their own file, as credit-cap and lens-payloads did, because adding the ordering cases put router.test.ts over its 1000-line cap. Stacked on #317: the rename touches check-consumer-config.ts, which only exists on that branch. * [review-gitattributes-negation] review: treat `!linguist-generated` as a negation too Git has four attribute states, and three of them mean "not generated": `-attr` (Unset), `attr=false` (the value `false`), and `!attr` (Unspecified). The parser recognised the first two but not the third, so a `!linguist-generated` line mentioned the attribute without becoming a rule: it was dropped, an earlier broad `=true` glob still won by last match, and the path stayed excluded from review. That is the same silent skip this branch fixes for the other two forms, reached by a different spelling. Verified against `git check-attr` (2.55.0) rather than inferred: with `.claude/** linguist-generated=true` followed by `.claude/skills/** !linguist-generated`, git reports SKILL.md as `unspecified` and `.claude/hooks/h.mjs` as `true`, and the router returned `true` for both before this change. Unspecified is not the same as false in general, since it is where Linguist falls back to its content heuristic. This router has no content heuristic and treats an unmatched path as source, so the two reach the same verdict here and share a branch; the doc comment says so, because otherwise the collapse reads as a semantic error. The isGenerated ordering test becomes a table over all three negation forms, so no form can regress to being silently unrecognised.
Contributor
There was a problem hiding this comment.
Approved — no blocking issues found.
Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable).
Note: test-adequacy not assessed this run (test-adequacy output unavailable).
Note: completeness not assessed this run (completeness output unavailable).
Note: holistic not assessed this run (holistic output unavailable).
Note: first-principles not assessed this run (first-principles output unavailable).
Note: conventions not assessed this run (conventions output unavailable).
Note: documentation not assessed this run (documentation output unavailable).
Note: thread reconciliation not assessed this run (thread-reconciler output unavailable).
Note: divergence tripwire re-armed a full review (unreviewed share 0.89).
…R body, and route key provisioning through IT Two fixes from reading the PR bodies this skill actually produces (Khan/agent-settings#48). Step 7 prescribed seven sections and got them: the resulting install PR body was long enough that nobody reads it, which defeats its one purpose. It now asks for exactly three things -- what was auto-generated (the table), what was hand-written and the rationale for each judgment call, and the ordered blockers -- with everything else capped at a sentence. The key handling told the operator to hand a repo admin a 'gh secret set' command, but at Khan the key is created and installed by IT, not by whoever is onboarding. A new 'Provisioning the ANTHROPIC_API_KEY' section at the end states the actual flow: request in #it naming the repo, IT creates the key and adds it to the repo's Actions secrets (or scopes the repo to an existing org secret), operator confirms by name only. 'What stays human' now points there instead of at the paste command.
Contributor
|
tested it out on the mobile repo here https://github.com/Khan/mobile/pull/4114 looks very cool! |
…on feedback Review feedback on #317 (the inline-comment/quote/flow-list normalization was already fixed at head; this covers the rest): - nested() reads a same-indent block sequence directly after its key, so a hand-restyled but valid imports list no longer raises the exit-1 workflow-missing-config-import false error. - New max-ai-credits-mirror-stale warning: the run budget reads only the REVIEW_MAX_AI_CREDITS env mirror (resolveCreditCap), so a raised cap with a stale or missing mirror still plans at the old ceiling; the checker now validates the pair. - The CLI reads the shipped max-ai-credits from its own checkout's review.md; SHIPPED_MAX_AI_CREDITS is demoted to a fallback, so a release that raises the shipped ceiling cannot strand the check. - lock-not-marked-generated is guarded on the lock existing: a repo the errors just told has no lock is not also told to mark it generated (and --strict no longer flips on the spurious warning). - re-review-full message no longer claims ROUTING 'sets no re-review mode' when the parser cannot distinguish explicit full from absent. - FsLike renamed ConsumerConfigFs per the module-named injected-fs convention. - Test gaps closed: lens-payload warning forwarding, source-missing, the renamed --workflow lock-path derivation, plus the new checks. - README records the consumer-CI gate as an explicit follow-up.
Contributor
Author
|
Feedback triage. The blocking finding (frontmatter.ts inline comments) and the quote/flow-list/NaN normalization suggestions were already fixed at head (stripInlineComment, unquote, flow-style list(), unquoted scalar()); the review's line references predate those commits. Commit 7d1f4c1 (push pending; branch is outside the sandbox push allowlist) covers the rest:
Also fixed CI: the failing agent job was attempt 2 of run 31027736888 reusing an expired activation artifact; a full rerun regenerates it. |
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
15 of 17 prior review threads resolved; 2 still unaddressed as of 7d1f4c1:
2 non-blocking threads still open
- thought (non-blocking)
workflows/review/README.md:174: The check catalog is restated in prose in four places (changeset, README, SKILL Step 5, module docstring). Since the iss... - thought (non-blocking)
workflows/review/lib/check-consumer-config.ts:572: Two of the checker's warnings compensate for shipped defaults this repo owns and could just fix upstream. I checked the...
| path: string, | ||
| ): ReviewerRouting => { | ||
| if (!fs.existsSync(path)) { | ||
| return { |
Contributor
There was a problem hiding this comment.
note (non-blocking): readReviewerRouting's missing-file early return omits the required allowlistKeyPresent field of ReviewerRouting, a type error that pnpm typecheck cannot catch because tsconfig excludes workflows/**. Introduced by this change. ReviewerRouting declares allowlistKeyPresent: boolean as required (check-consumer-config.ts line 175), but the !fs.existsSync(path) early return at lines 326-331 constructs the object without it — I confirmed the field is absent on the checkout, and that downstream reads don't touch it when present is false, so the impact today is limited to the --json report shape. It survives review and CI because tsconfig.json excludes workflows/** from pnpm typecheck (a documented pre-existing gap) and vitest does not type-check; a one-line allowlistKeyPresent: false closes it and keeps the type honest before some future caller reads the field on the not-present path. Contrast: the sibling readInstalledWorkflow early return (lines 277-285) correctly supplies every required field of its type.
Suggested change
| return { | |
| return { | |
| present: false, | |
| definesAddReviewer: false, | |
| allowlistKeyPresent: false, | |
| allowedTeamReviewers: [], | |
| hasGithubToken: false, | |
| }; |
Lower-confidence observations (1)
workflows/review/lib/frontmatter.ts:5suggestion (non-blocking): The 'no YAML dependency' premise is a choice, not a constraint — and the hand parser's growth pattern already shows the cost.
| case "--explain": | ||
| out.explainPath = argv[++i]; | ||
| break; | ||
| case "--workflow": |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): parseArgs --workflow flag has no coverage. The functional path is covered via the workflowPath option (line 373 test), but the CLI flag that populates it is not. Grep of the test file confirms --workflow appears in no parseArgs case.
A sketch, not a committable replacement:
Add a parseArgs assertion including `--workflow .github/workflows/pr-review.md` and expect `workflowPath` to be set.
| ): boolean => generatedPatterns.some((pattern) => matchesGlob(path, pattern)); | ||
| generatedRules: readonly GeneratedRule[], | ||
| ): boolean => { | ||
| for (let i = generatedRules.length - 1; i >= 0; i--) { |
Contributor
There was a problem hiding this comment.
note (non-blocking): Description says "nothing here runs in a review," but the bundled router.ts negation change alters production generated-file classification. isGenerated (reverse last-match-wins) is called from route() on the production review path; a repo that un-marks a subtree after a broad linguist-generated=true glob now gets those files reviewed where before they were skipped. It's captured in the separate gitattributes-negation changeset, but the PR description's stated intent (onboarding checker + skill, "routing defaults untouched") does not mention this shipped-behavior change and explicitly claims the opposite. Surfacing as a scope note so the reviewer weighs the production blast radius, not to block.
Also flagged by first-principles.
A sketch, not a committable replacement:
Split the gitattributes-negation router change into its own PR, or amend this PR's description to surface it as the live behavior change it is.
| const warnings = report.issues.filter( | ||
| (issue) => issue.severity === "warning", | ||
| ); | ||
| if (errors.length > 0 || (args.strict && warnings.length > 0)) { |
Contributor
There was a problem hiding this comment.
note (non-blocking): --strict exit-code behavior is untested. checkConsumerConfig is pure and well-tested, but the strict-vs-non-strict exit code decision is CLI glue in main() and is never asserted. Consider extracting the exit-code predicate into an exported pure helper so it can be unit-tested.
| }; | ||
|
|
||
| const main = (): void => { | ||
| /* eslint-disable-next-line no-undef */ |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): Unnecessary eslint-disable-next-line no-undef on the require("node:fs") idiom. Every other CLI main() in this directory acquires node fs with the identical bare idiom and no disable comment — e.g. stage-pr.ts:717 const nodeFs = require("node:fs") as StagePrFs;, router.ts:970 const fs = require("node:fs") as FsLike; (11 siblings total, all lint-clean per the PR body). This file adds /* eslint-disable-next-line no-undef */ immediately above the same call: 926| /* eslint-disable-next-line no-undef */ / 927| const nodeFs = require("node:fs") as ConsumerConfigFs & {.
Suggested change
| /* eslint-disable-next-line no-undef */ | |
| const nodeFs = require("node:fs") as ConsumerConfigFs & { |
jwbron
marked this pull request as ready for review
August 13, 2026 20:48
Contributor
Review Guidancegithub-actions (5 files)
Common patterns7 files: RouterConfig field renamed from - generatedPatterns: [],
+ generatedRules: [],Excluded from review (5 files)Not individually reviewed — generated, formatting-only, or
|
Contributor
There was a problem hiding this comment.
Approved — no blocking issues found.
7 of 7 prior review threads are still unaddressed as of 7d1f4c1:
7 non-blocking threads still open
- thought (non-blocking)
workflows/review/README.md:174: The check catalog is restated in prose in four places (changeset, README, SKILL Step 5, module docstring). Since the iss... - note (non-blocking)
workflows/review/lib/check-consumer-config.ts:326: readReviewerRouting's missing-file early return omits the requiredallowlistKeyPresentfield ofReviewerRouting, a t... - thought (non-blocking)
workflows/review/lib/check-consumer-config.ts:572: Two of the checker's warnings compensate for shipped defaults this repo owns and could just fix upstream. I checked the... - suggestion (non-blocking)
workflows/review/lib/check-consumer-config.ts:909: parseArgs--workflowflag has no coverage. The functional path is covered via theworkflowPathoption (line 373 test... - nitpick (non-blocking)
workflows/review/lib/check-consumer-config.ts:926: Unnecessaryeslint-disable-next-line no-undefon therequire("node:fs")idiom. Every other CLImain()in this dire... - note (non-blocking)
workflows/review/lib/check-consumer-config.ts:992:--strictexit-code behavior is untested. checkConsumerConfig is pure and well-tested, but the strict-vs-non-strict exi... - note (non-blocking)
workflows/review/lib/router.ts:439: Description says "nothing here runs in a review," but the bundled router.ts negation change alters production generated-...
sxkosone
approved these changes
Aug 20, 2026
sxkosone
left a comment
sxkosone
left a comment
There was a problem hiding this comment.
I didn't read the PR description or the code, as they're not readable for humans. But I'll rubber-stamp-approve this to keep the reviewer work moving forward.
jeresig
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two pieces of onboarding infrastructure for standing the shared reviewer up in a new
consumer repo, in the shape of
Khan/kore-marketplace#3:workflows/review/lib/check-consumer-config.ts(pluslib/frontmatter.ts): achecker that validates a consumer's install before its first PR.
.claude/skills/review-onboarding/SKILL.md: the playbook that produces the install,carrying the judgment calls the commands can't.
No change to the shipped review workflow.
review.md, the dispatcher, and every routingdefault are untouched; nothing here runs in a review.
Why a checker at all
Onboarding is five hand-written config files, two local edits to the installed
review.md, and a few admin blockers. Almost every mistake in that set fails late andquietly:
{{#runtime-import}}Runtime import file not found, on someone's PR. Not atgh aw compiletime.add-reviewerdefined inreview.mdtoo${{ }}expression inside a runtime importobservability:block without bothGH_AW_OTEL_SENTRY_*secretsmax-ai-credits: 1000on atier=highrepoROUTINGtypo (plugin/**forplugins/**).lock.ymlnot markedlinguist-generatedHow it stays honest
It never reimplements routing semantics. Tiers, lenses, generated-file classification
and the budget come from
route();ROUTINGfromparseRoutingConfig;.gitattributesfromparseGitattributesGenerated. So a release that changes thosesemantics changes the checker's answers for free.
That only holds if the checker and the consumer agree on a version, so it ships in
lib/(beside the code it calls, released under the same tag) and is documented asrun from the tag the consumer pins. A mismatch is itself one of its warnings:
Two views answer the question a tier map actually raises.
--files-from(fed bygit ls-files) resolves every tracked file, names the files no rule matched, and namespatterns that match nothing.
--explain <path>lists every matching rule in file order,so last-match-wins ordering is visible rather than inferred:
This replaces the hand-verification
kore-marketplace#3did by importing the parserad hoc, which its own PR body flagged as the step it could not show.
lib/frontmatter.tsis the minimal structural reader the checker needs (indentation,key:,- item; no YAML dependency inlib/). One behaviour is load-bearing ratherthan incidental: comment lines are dropped, so a commented-out block reads as absent,
which is exactly what disabling
observability:means.The skill
The mechanical half of onboarding is
gh aw add; the rest is judgment, and that is whatthe skill carries: what stays the operator's call (team allowlist,
enableroster,re-reviewmode, every admin blocker), the preflight inventory that becomesci-tooling.mdandskills.md, the local edits and when each applies, how to derivetiers from the repo's own blast radius rather than another consumer's file, and a PR-body
structure that separates what is generated from what was decided (including what the
author did not verify).
Two boundaries worth flagging for review, because they are deliberate:
ANTHROPIC_API_KEYvalue. Not from Keeper, another repo, theenvironment, or chat, and never echoed into a command. It prints
gh secret setforthe operator to run (which prompts, so the value never enters a transcript) and
confirms afterwards by name only. Granting team access and creating the opt-out label
are in the same print-and-stop block.
the PR is the install's own first live test.
What this does not do
The checker proves your rules parse, fire, and resolve where you think. It cannot tell
you a tier is wrong:
plugins/** tier=lowis a defensible-looking line it willhappily confirm. That judgment stays with the author and the reviewer, and Step 4 of the
skill says so.
Testing
check-consumer-config.test.ts,frontmatter.test.ts). Each checkercase is one way an install fails silently in production; routing semantics are not
re-tested here, since
router.test.tsowns those.trivial, 3 generated) and against
kore-marketplace's real 48-lineROUTINGoverits actual tree (zero dead patterns, 14 high, README medium, plugin README trivial,
3 falling to the default
low).after checking
lib/glob-match.ts: a pattern with no/matches the basename in anydirectory (so
README.mdalso matchesdocs/sub/README.md), and a leading/anchorsto the root.
pnpm typecheckandeslint workflows/review/lib/are clean.files:
eval/live-ab.test.tsandeval/rereview-sweep.test.ts(need fetched tags) andlib/dispatch-runner.test.ts(Cannot find package 'zod'underlib/node_modules).Follow-ups, not in scope here
review-v1.7.0while the package is at 1.11.0. Thechecker surfaced it. Bumping a live reviewer install seemed like its own decision.
templates, delete the
gh awCopilot leftovers, write.gitattributes, apply the tworeview.mdlocal edits), which would shrink the skill to judgment only.skill's optional-surfaces step rather than automated.