diff --git a/.changeset/gitattributes-negation.md b/.changeset/gitattributes-negation.md new file mode 100644 index 00000000..8d0cc136 --- /dev/null +++ b/.changeset/gitattributes-negation.md @@ -0,0 +1,44 @@ +--- +"review": minor +--- + +review: honour `linguist-generated` negations the way git does, so deliberately un-marked files get reviewed + +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 placed after a broad glob is how a repo says "this +subtree is generated, except this part". The reviewer discarded those negations +while parsing, which meant it silently skipped review of exactly the files a repo +had gone out of its way to keep visible. + +`parseGitattributesGenerated` now returns ordered `GeneratedRule[]` +(`{pattern, generated}`) 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. + +All three of git's negation forms count as rules, since each one shadows an +earlier `=true` by last match: `-linguist-generated` (Unset), +`linguist-generated=false` (the value `false`), and `!linguist-generated` +(Unspecified). Unspecified is not generally the same as false (it is where +Linguist falls back to its content heuristic), but this router has no content +heuristic and treats an unmatched path as source, so the two reach the same +verdict here. A negation form the parser did not recognise was dropped as +not-a-rule, letting the broad glob win and skipping the path: the same silent +skip, by a different spelling. + +Found while 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 in diffs. Its +`new-repo-config` skill — executable prose that steers every Claude session in that +repo — resolved to `trivial (generated)` and was excluded from review; it now +resolves to `medium` and is reviewed. + +Blast radius is narrow and one-directional (strictly more review, never less): +only a repo that actually writes a negation after a broader `=true` glob changes at +all. `Khan/actions` and `Khan/frontend` have no negations, and webapp's single one +(`services/ai-guide/cmd/componentgen/main.go linguist-generated=false`) exists to +counter Linguist's content heuristic rather than an earlier `.gitattributes` rule, +so no earlier rule matches that path and its classification is unchanged. diff --git a/.changeset/review-onboarding-checker.md b/.changeset/review-onboarding-checker.md new file mode 100644 index 00000000..0fb04b53 --- /dev/null +++ b/.changeset/review-onboarding-checker.md @@ -0,0 +1,93 @@ +--- +"review": minor +--- + +review: an onboarding playbook and a consumer-config checker for new consumer repos + +Onboarding a repo (`Khan/kore-marketplace#3` is the current template) 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 the problem both halves of this change address. + +`lib/check-consumer-config.ts` validates an install through the **production** +parsers rather than a reimplementation: tiers, lenses, generated-file +classification and the budget come from `route()`, `ROUTING` from +`parseRoutingConfig`, `.gitattributes` from `parseGitattributesGenerated`. A +release that changes those semantics changes the checker's answers for free, +which is why it ships in `lib/` and is run from the tag the consumer pins (a +mismatch between the two is itself one of its warnings). It reports, as errors: +a missing or empty required config, a `${{ }}` expression inside a runtime +import or lens payload (gh-aw rejects those), `add-reviewer` defined in +`review.md` as well as `config.md` (the main workflow wins, silently discarding +the consumer's allowlist), a dropped `imports:` line, an empty +`allowed-team-reviewers` **in a repo that has a `.github/REVIEWERS` ownership +map**, and a missing `.lock.yml`. And as warnings: an unpinned or stale +`source:`, a live `observability:` block (both `GH_AW_OTEL_SENTRY_*` secrets are +hard-required while it is present, and a missing one kills the agent job at +startup), the shipped 1000-credit ceiling that `tier=high` runs have died at, +`ROUTING` parse warnings, inert lens payloads, a lock not marked +`linguist-generated`, an unmarked `agentics-maintenance.yml`, leftover `gh aw` +Copilot scaffolding, and reviewer config that does not itself route to `high`. + +Two of those checks are deliberately shaped by what the router can actually do. +`.github/REVIEWERS` is its only source of team ownership, so in a repo without +one, Step 8 requests nobody no matter what `add-reviewer` allows: an empty +allowlist there is an accurate "this repo does not request reviewers" rather than +a dropped request, and it reports `reviewer-requests-inert` instead of failing. +Erroring would only get an inert team invented to satisfy the check. Separately, +`agentics-maintenance.yml` is `gh aw compile` output whose name is *not* +`*.lock.yml`, so the `.gitattributes` marker consumers were told to add misses it +and ~600 generated lines get line-reviewed until it gets its own. + +Two views answer the question a tier map actually raises. `--files-from` +(`git ls-files | …`) prints the resolved tier of every tracked file with +per-tier samples and names the files no `tier=` rule matches at all, and +`--explain ` lists every matching rule in file order so last-match-wins +ordering is visible instead of inferred. That replaces the hand-verification +`kore-marketplace#3` did by importing the parser ad hoc. + +`lib/frontmatter.ts` is the minimal structural frontmatter reader the checker +needs (indentation, `key:`, `- item`; no YAML dependency in `lib/`). One +behaviour is load-bearing: comment lines are dropped, so a commented-out block +reads as absent, which is exactly what disabling `observability:` means. + +It also **normalises values**, because for a tool whose contract is "errors must +be zero" a false error is the worst failure mode, and each of these is valid YAML +a consumer can legitimately write: + +- **Inline comments are stripped** from values and list items (respecting quotes + and `url#frag`). This one fired on the skill's own prescribed flow, which tells + authors to label every local edit with a comment: `max-ai-credits: 2500 # LOCAL + OVERRIDE` made `Number()` return `NaN`, silently suppressing the credit-ceiling + check; a labelled `source:` reported a spurious `source-ref-mismatch`; and a + labelled `imports` item produced a false `workflow-missing-config-import` + error. +- **Surrounding quotes are stripped** by `scalar()`, so `max-ai-credits: "1000"` + no longer reads as `NaN` (and a quoted `imports` item no longer reads as a + missing import). +- **Flow-style lists are read**: `allowed-team-reviewers: [kore]` is the spelling + the shipped `review.md` itself uses for `toolsets: [pull_requests, repos]`, and + it previously reported a false `config-empty-team-allowlist` error on a working + allowlist. + +The new `list()` returns undefined only when a key is **absent**, which is what +lets the empty-allowlist check tell a deliberate omission from a key that yielded +nothing. Those are now separate outcomes: a present-but-empty key is always an +error (someone wrote the field and got nothing, and calling that deliberate would +read as an all-clear over a dropped allowlist), while an absent key defers to +`.github/REVIEWERS` as described above. + +Still unsupported, and documented as such: multi-line flow sequences, +anchors/aliases, and block scalars. + +The judgment half is `.claude/skills/review-onboarding/SKILL.md`: what stays the +operator's call (the team allowlist, the `enable` roster and `re-review` mode, +every admin blocker), the preflight inventory that becomes `ci-tooling.md` and +`skills.md`, the two local edits and when each applies, how to derive risk prose +and tiers from the repo's own blast radius rather than another consumer's file +(the three known consumers' radii differ completely), and the PR-body structure +that separates what is generated from what was decided, including what the author +did not verify. + +No change to the shipped review workflow: `review.md`, the dispatcher, and every +routing default are untouched. diff --git a/.claude/skills/review-onboarding/SKILL.md b/.claude/skills/review-onboarding/SKILL.md new file mode 100644 index 00000000..327793f9 --- /dev/null +++ b/.claude/skills/review-onboarding/SKILL.md @@ -0,0 +1,501 @@ +--- +name: review-onboarding +description: Onboard a repo onto Khan's shared AI PR reviewer (`Khan/actions` `workflows/review`): install the workflow at a pinned tag, author the five consumer config files against that repo's own blast radius, validate the install with the consumer-config checker, and open the disclosure PR. Use when a repo should start getting automated PR reviews, when refreshing or auditing an existing install, or when a review is misbehaving in a way that smells like config. Invoke with the target repo. +--- + +# Onboard a repo onto the shared PR reviewer + +Produce **one PR** that turns the shared reviewer on in a consuming repo, with +config written for that repo rather than copied from another one. + +The template is [`Khan/kore-marketplace#3`](https://github.com/Khan/kore-marketplace/pull/3); +the contract it satisfies is `workflows/review/README.md` in `Khan/actions` +(sections *Consumer configuration*, *The `ROUTING` file*, *Required secrets / +variables*). Read those before writing anything; this skill choreographs the +work and carries the judgment calls, it does not restate the format spec. + +## What stays human + +Confirm all of these with the operator before writing config. Do not infer them +from the repo and do not carry them over from another consumer: + +1. **The `add-reviewer` team allowlist**: which team owns this repo — or whether any + does. "No reviewer requests" is a legitimate answer, and for a repo with no + single owning team it is usually the right one. Do not invent an inert team to + fill the field: leave `allowed-team-reviewers` empty, add no `.github/REVIEWERS`, + and the checker reports `reviewer-requests-inert` rather than an error. Ownership + comes from `.github/REVIEWERS` and nowhere else, so without it Step 8 requests + nobody whatever the allowlist says — the two are inert alone and only work + together. +2. **The `enable` roster and `re-review` mode.** Policy is that an opt-in + reviewer earns its line through the eval suite. The defensible default is to + mirror an existing consumer's eval-justified set and say in the PR that that + is what you did; anything else is the operator's call. +3. **Anything that needs a repo admin or a secret value**: adding + `ANTHROPIC_API_KEY`, granting a team repo access, creating the opt-out label. + Inventory them, put them in the PR body as ordered blockers, and say which one + is hard. + + **Never handle the key's value.** Do not read it out of Keeper, another repo, + the environment, or the operator's chat, and never echo it into a command you + run. Print the command for the operator to run themselves and stop: + + ```sh + gh api -X PUT /orgs/Khan/teams//repos/Khan/ -f permission=push + gh label create skip-ai-review -R / -d "Opt this PR out of AI review" + ``` + + The `ANTHROPIC_API_KEY` itself is requested from IT, who create it and add + it to the repo's Actions secrets — see *Provisioning the `ANTHROPIC_API_KEY`* + at the end of this skill. + + Afterwards confirm by name only (`gh secret list -R `), never by value. + + **You cannot copy the key from a repo that already has it, so do not try.** + Actions secrets are write-only: values are encrypted against the repo's public + key on write and decrypted only into a runner at job time. `GET + /repos/{owner}/{repo}/actions/secrets/{name}` returns the name and timestamps + with no value field, there is no `gh secret get`, and `gh secret list` is names + only. "Pull it from webapp" is not a step that exists. + + **Prefer an org secret over a per-repo paste.** A repo-level + `ANTHROPIC_API_KEY` makes every onboarding wait on someone handling key + material. An organisation secret with *selected repositories* visibility turns + that into an admin adding one repo to a list, with no value moving and nothing + for this skill to touch. Check what the target repo can already see: + + ```sh + gh api /repos///actions/organization-secrets --jq '.secrets[].name' + ``` + + If `ANTHROPIC_API_KEY` is there, it is already provisioned and there is no + blocker. If an org secret exists but this repo is not in its selected list, + the blocker is "add this repo to it" (org-admin, no key material) rather than + a new key, and the PR body should say so. Either way the request goes through + IT (see *Provisioning the `ANTHROPIC_API_KEY`*). + +You *do* own the risk prose, the tiers, and the CI-tooling list, but derive them +from **this** repo (its CI workflows, its test setup, its docs, its actual blast +radius), never from another repo's file. A `ci-tooling.md` that names a lint rule +this repo doesn't run teaches the reviewer to stay silent about real defects. + +## Step 0: preflight + +Gather, and record for the PR body: + +- The `gh aw` extension, and its version (the compiled lock is version-specific, + and the PR body should name the version that produced it): + + ```sh + gh extension install github/gh-aw # or: gh extension upgrade gh-aw + gh aw version + ``` +- The tag to pin: the newest `review-v..` in `Khan/actions` + (`git tag --list 'review-v*' | sort -V | tail`). Pin the full semver, not the + moving major, so the install is reproducible. +- Secrets: `gh secret list -R ` plus + `gh api /repos///actions/organization-secrets --jq '.secrets[].name'`. + You need `ANTHROPIC_API_KEY` (repo-level at Khan) and `KHAN_ACTIONS_BOT_TOKEN` + (usually an org secret). `GH_AW_OTEL_SENTRY_ENDPOINT` / + `GH_AW_OTEL_SENTRY_AUTHORIZATION` decide Step 2's observability edit. +- Public or private (`gh repo view --json isPrivate`), which decides the fork guard. +- What CI already exists: every workflow in `.github/workflows/`, the lint/format + config, the test runner and whether a suite actually runs on PRs. This *is* + `ci-tooling.md`; read it now, not later. +- Ownership and convention surfaces: `.github/REVIEWERS` (Gerald), + `.github/NOTIFIED`, `.claude/skills/`, `AGENTS.md`/`CLAUDE.md`, `REVIEW.md`, + READMEs that state contracts. These are the raw material for `skills.md`. +- **Write down the blast radius in one sentence**: what breaks, and for whom, + when code in this repo is wrong. Every tier and deep-check decision below + descends from it. The three known answers differ completely, which is the point: + webapp = product runtime for learners; `Khan/actions` = CI supply chain running + in other repos with their tokens; `kore-marketplace` = agent supply chain + running on engineers' laptops as them. + +## Step 1: install the workflow + +Work on a branch in the consumer repo from the start; never on its default +branch. + +```sh +cd && git switch -c enable-shared-pr-reviewer +gh aw add Khan/actions/workflows/review/review.md@review-v.. +gh aw compile +``` + +This writes `.github/workflows/review.md` (verbatim from the tag, with a +`source:` field), compiles `review.lock.yml`, and may also emit +`.github/aw/actions-lock.json` and `agentics-maintenance.yml` (gh-aw scaffolding +for `cache-memory`; keep it, and explain it in the PR body). Commit all of them. + +If `gh aw add` fails with *failed to inspect repository initialization state*, look +for a zero-byte non-directory sitting where it wants a directory (`.vscode`, +`.idea`): some sandbox setups create read-only placeholder files to block those +paths. Do the install in a clean `git worktree` rather than deleting them. + +Three cleanups: + +- **Remove the Copilot scaffolding** `gh aw` writes on first init in a repo + (`.github/mcp.json`, `.github/agents/`, `.github/skills/`, + `.github/workflows/copilot-setup-steps.yml`) unless the repo actually uses + Copilot Agent. No Khan consumer carries it. +- **`.gitattributes`**: mark **both** generated workflows, or the reviewer + line-reviews its own compiler output: + + ``` + .github/workflows/*.lock.yml linguist-generated=true merge=ours + .github/workflows/agentics-maintenance.yml linguist-generated=true merge=ours + ``` + + The second is easy to miss because its name is not `*.lock.yml`, but it is + `gh aw compile` output all the same (~600 lines, regenerated unconditionally — + deleting it does not stick). Do **not** mark `review.md`: that is the + hand-editable source your Step 2 local edits live in. The checker flags either + omission. If the repo's `.gitattributes` has a machine-managed block (a generator + that owns part of the file, as in `Khan/agent-settings`), put these lines + **outside** it and verify they round-trip before trusting them. +- **Normalise `source:` to the tag.** `gh aw add ...@review-v1.11.0` records the + *resolved commit SHA*, not the tag you asked for. Rewrite it to + `@review-v..`; that is what webapp and `Khan/actions` carry, + it is what makes the pin legible in a diff, and the checker compares against it + (a SHA reads as `source-ref-mismatch`). + +Never hand-edit `review.lock.yml`; regenerate with `gh aw compile`. + +`gh aw compile` may also stop with *safe update mode detected unapproved changes*, +listing the secrets the workflow references. Review each one before passing +`--approve`, and put that review in the PR body — Step 7 has a section for it. + +## Step 2: local edits to the installed `review.md` + +`gh aw update` preserves local edits via 3-way merge, so these survive upgrades. +Label each one with a ` LOCAL OVERRIDE:` comment saying *why*, so the next +reader and the next merge conflict both have the reasoning. + +| Edit | When | Why | +| --- | --- | --- | +| `max-ai-credits: 2500` **and** the `REVIEW_MAX_AI_CREDITS: "2500"` env mirror | Any repo where paths route to `tier=high` | The shipped 1000 sits below a full-depth full-roster run: observed runs died at 1001-1024 metered credits *after* computing a verdict but before posting it. A ceiling, not a spend. | +| Comment out the `observability:` block | No `GH_AW_OTEL_SENTRY_*` secrets | While the block is present both secrets are hard-required: a missing one makes the agent job die at startup rather than skip trace export. Restore verbatim once they exist. | +| Fork guard in the `if:` (`head.repo.full_name == github.repository`) | Public repos | The shipped `roles: all` disables gh-aw's own actor gate, so this `if:` is what keeps untrusted fork heads from triggering runs. Private repos need nothing. | +| `REVIEW_BOT_LOGIN` | Only if the repo posts reviews under its own GitHub App | Getting it wrong files the bot's own threads as human ones, which puts their lines in `skipLines` and **drops** fresh findings there. | + +Do **not** add `add-reviewer` here. gh-aw lets the main workflow override an +imported safe output of the same type, so defining it here silently discards the +consumer's team allowlist. + +## Step 3: the five config files (`.github/aw/review/`) + +Author all five. Four are required; `ROUTING` is nominally optional but a repo +without it gets no specialist lens, a floored budget, and a missing-config note +on every review. Keep every file plain Markdown with **no `${{ }}` expressions** +(gh-aw rejects those inside imports). `config.md` is the sole exception, because +it is a frontmatter import and carries the bot token. + +Head each file with an HTML comment saying which repo it belongs to, which +sub-agent consumes it, and when it is read (compile vs. runtime). That comment is +how the next person knows what editing it changes. + +- **`config.md`**: frontmatter only; the body is ignored. Owns `add-reviewer`: + `allowed-team-reviewers` (bare team slugs) and + `github-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}` (the default + `GITHUB_TOKEN` cannot request an org team). Note in the comment whether Gerald + also requests reviewers here, and that GitHub silently drops a team reviewer + request when the team lacks repo access. Still required even when this repo + requests no reviewers (see *What stays human*): it is the compile-time + `imports:` target, so `gh aw compile` fails without it. In that case leave the + allowlist empty and say in the comment *why* requests are off and what would turn + them on, because nothing else on the PR makes that visible. +- **`risk-classification.md`**: the model-facing prose about file *contents*. + Assign High/Medium/Low/Trivial using real paths from this repo, then add a + "what to verify that CI cannot" section. This is where the blast-radius + sentence pays off; write the deep-checks that follow from it (for + `kore-marketplace`: pre-authorization creep in `allowed-tools:`, shell + injection in plugin scripts, credential flow, manifest agreement). Also state + which files are generated, and that a generated file edited *without* its + source changing is a red flag. +- **`ci-tooling.md`**: what CI already catches, so the reviewer doesn't spend + comments on it; every claim must name a real command or job. Then a + "don't raise these false alarms" section for this repo's established patterns. + **Invert it when the repo has no CI**: say so explicitly and set the bar for + what is worth a comment anyway (don't nitpick unenforced formatting; don't + treat missing tests as a finding in a repo with no test infrastructure). +- **`skills.md`**: the catalog `skill-auditor` audits against. Each entry is a + `### - ` heading (path in backticks) followed by an + **Evaluate when:** clause. The agent reads each path off its checked-out + workspace, so every path must exist. A repo with no `.claude/skills/` catalog points + at the READMEs that actually state its conventions. Say the quiet part in the + header comment: a convention not written in one of these files is not + auditable, so don't flag from memory or from another Khan repo. +- **`ROUTING`**: see Step 4. + +## Step 4: `ROUTING` + +Format spec is in the README; the judgment is in the ordering and the tiers. +`ROUTING` is the only config file whose effect is fully mechanical, so it is the +only one you can be *sure* about before the first PR. Get it right by derivation +and then by checking, not by analogy to another repo's file. + +**Derive the tiers.** Enumerate what the repo actually contains +(`git ls-files | cut -d/ -f1 | sort -u` for the top level, then descend into +anything large), and for each group ask the Step 0 blast-radius question: *when +this file is wrong, who is harmed and how far does it reach?* Ship code that +executes elsewhere (in another repo's CI, on an engineer's machine, in +production) is `high`. Repo-local dev surfaces (tests, fixtures, eval harnesses) +are `low`. Docs are `trivial`. Anything that decides what gets published, or that +steers an agent, is `high` regardless of file extension. + +Then write the rules in this order: + +1. **Broad rules first, exceptions after.** Last matching rule wins + (gitignore-style), so `**/*.md tier=trivial` goes above the re-raises. +2. **Re-raise executable prose.** `SKILL.md`, agentic-workflow `.md`, and prompt + imports are programs that happen to be Markdown; the broad docs rule would + route them `trivial`. Then drop READMEs and changelogs back down. +3. **Tier the reviewer's own config `high`** (`.github/aw/review/**`, + `.github/workflows/*.md`). A PR that tampers with the reviewer must not be + reviewed at the trivial or default budget. The checker warns when it isn't. +4. **`direction-dependent`** (only ever with `tier=`) for a path whose risk + depends on which way the diff moves, such as a permissions or allowlist file + where widening is dangerous and tightening is not. The router then asks rather + than guesses, and it applies only when its own rule is the winning one for the + path. Use it sparingly; it costs a model call. +5. **`enable` and `re-review`** are the operator's decisions from *What stays + human*. Whatever you write, justify it in a comment above the line. + +**The glob dialect has one real trap.** A pattern with **no `/` matches the +basename in any directory**: `README.md` also matches `docs/sub/README.md`. +Prefix `/` to anchor to the repo root (`/README.md`). Otherwise: `**` crosses +directories, `*` and `?` never do (`plugins/*/skills/*/SKILL.md` will not match +`plugins/a/b/c/SKILL.md`), and a trailing `/` matches a directory and everything +beneath it. + +**Then check it, per path, until nothing surprises you.** A malformed line is +skipped with a warning and a typo'd pattern parses perfectly while routing +nothing, so neither is visible in the file itself: + +- `--files-from` gives the resolved tier of every tracked file, names every file + no rule matched (those fall to the router's default `low`), and names every + pattern that matches nothing at all. +- `--explain ` lists every rule matching that path in file order, so you + can see which one won instead of assuming. + +Run both before the PR, and treat "a file I expected to be `high` came out +`low`" as a rule bug, not a rounding error. + +**Lenses are opt-in and mostly won't apply.** The eleven specialist lenses target +webapp's backend domains. Enable one only when a real path here maps to it; a +lens with no matching path is inert, and a payload with no routing rule is +silently inert (the checker warns). "None maps here, and why" is a legitimate +outcome to record in the header comment. + +## Step 5: validate + +Run the checker **from a checkout of the tag the repo pins**, so the semantics +you validate are the ones its reviews will run. Get one (skip the clone if you +are already working inside a `Khan/actions` checkout at that tag): + +```sh +git clone --depth 1 --branch review-v.. \ + https://github.com/Khan/actions.git /tmp/review-checker +cd /tmp/review-checker + +# Stage the new config first: git ls-files lists TRACKED files, so an unstaged +# .github/aw/review/ reads as absent and every rule pointing at it is reported as +# matching nothing. `git add -A` in the consumer, or add the paths explicitly. +git -C ls-files | npx -y tsx workflows/review/lib/check-consumer-config.ts \ + --repo --files-from - + +# and, for any path whose tier you are unsure about: +npx -y tsx workflows/review/lib/check-consumer-config.ts \ + --repo --explain +``` + +Pass `` as an absolute path, since the checker runs from the clone. + +It asks the real router, `ROUTING` parser, and `.gitattributes` parser (not a +reimplementation), so it catches the whole class of mistakes that otherwise +surface as a red run on someone's PR: a missing runtime import, a `${{ }}` in an +import, `add-reviewer` defined in both places, a dropped `imports:` line, an +empty team allowlist, a missing lock, an unmarked lock, a live `observability:` +block, the default credit ceiling, `ROUTING` parse warnings, inert lens payloads, +under-tiered reviewer config, and every tracked file's resolved tier. + +Errors must be zero. Every remaining warning is either fixed or **explained in +the PR body**; `--explain` output is the evidence that a tier is deliberate. + +## Step 6: optional surfaces + +Decide each explicitly and record the decision: + +- **`lenses/.md` payloads**: only for lenses `ROUTING` actually spawns. +- **`REVIEW.md` contracts**: per-directory calibration of Important vs. nit, + read from the PR head at run time. Cheap to add later. +- **`.github/NOTIFIED`**: honoured automatically if present; approval-time + pings, not on-touch. +- **Thumbs sweep and live counters**: two small scheduled plain-Actions + workflows that turn on the production feedback signal (see the README). + Reasonable to defer to a follow-up PR; say so rather than leaving it silent. +- **The opt-out label** (`skip-ai-review`): the `if:` guard works without it, + but the label must exist to be selectable in the UI. + +## Step 7: the PR + +**Keep the body short.** Its whole job is three things, and a reviewer should be +able to read it in under a minute. Do not narrate the onboarding process, restate +the README, or argue every decision at length — a body nobody finishes reading +reviews nothing. + +1. **What was auto-generated.** One table — file, approximate line count, what + regenerates it — and one sentence saying these are compiler output, never + hand-edited: + + | File | Lines | Regenerated by | + | --- | --- | --- | + | `.github/workflows/review.md` | ~3000 | `gh aw update` (verbatim from the tag, except the local edits) | + | `.github/workflows/review.lock.yml` | ~1900 | `gh aw compile` — the workflow GitHub actually runs | + | `.github/workflows/agentics-maintenance.yml` | ~600 | `gh aw compile` — gh-aw housekeeping | + | `.github/aw/actions-lock.json` | ~15 | `gh aw compile` — third-party action SHA pins | + + Note that the `.gitattributes` markers make GitHub collapse these in the + diff, and that `actions-lock.json` is deliberately unmarked because the SHA + pins are worth reading. +2. **What was hand-written, and why.** Name the human-written files (the five + under `.github/aw/review/`, the `.gitattributes` lines, the Step 2 local edits + to `review.md`) — that is the whole of the review. Then give the rationale for + each judgment call, one or two sentences apiece: the blast-radius sentence and + the tier decisions that follow from it, each local edit's reason, and anything + copied from another consumer rather than derived here. This is the only section + a reviewer should need to argue with. +3. **Before this works**: ordered blockers, naming which are hard (a missing + `ANTHROPIC_API_KEY` fails the agent job immediately; a team without repo + access makes `add-reviewer` a no-op but breaks nothing else). For the key, + link the request flow in *Provisioning the `ANTHROPIC_API_KEY`* below. + +Anything else (pinned-version link to the shared README, a security note, what +you did not verify) earns at most a sentence each. + +Apply the opt-out label to this PR only if you don't want it self-reviewed; +otherwise the PR is the install's own first test. + +Commit everything the install touched in one commit, write the body to a file +rather than fighting shell quoting, and open the PR: + +```sh +git add .github .gitattributes README.md +git commit -m "Enable the Khan shared AI PR reviewer" +git push -u origin enable-shared-pr-reviewer +gh pr create --title "Enable the Khan shared AI PR reviewer" --body-file +``` + +Then report the PR URL back. Do not merge it: the install's blockers are the +operator's to clear, and the PR is also the first live test of the reviewer. + +## Step 8: confirm the first run + +Watch the run (`gh run watch`, then the job summary and the posted review): + +- Did it post a verdict, and does the risks/patterns comment carry the + `` marker? That marker is the + attribution and rollback handle. +- Metered credits vs. the cap. A run that dies at the ceiling emits nothing. +- Failure triage: `Runtime import file not found` → a missing required config; + agent job dead at startup → a missing secret (or the live `observability:` + block); reviewer request silently absent → team lacks repo access; verdict but + no comments → check the change-provenance gate in the run artifact before + assuming the reviewer had nothing to say. + +Then hand back: the PR link, the first run's verdict and cost, the outstanding +admin blockers, and the deferred items from Step 6. + +Two follow-ups worth naming, not doing now: pricing a cheaper `re-review` mode +for this repo, and (for an architecture-class question, or before graduating a +repo to automatic mode) a seeded-defect live trial +(`.claude/skills/review-trial/SKILL.md`). + +## Updating an existing install + +A repo that is already on the reviewer needs a different, much shorter flow: not +authoring config, but moving a pin and checking what moved with it. Use this when +bumping to a newer `review-v*`, and also as the audit path when a review is +misbehaving in a way that smells like config. + +**Do it as its own PR**, never folded into unrelated work: the whole point is that +the diff shows what a version bump changed. + +```sh +cd && git switch -c bump-shared-pr-reviewer +gh aw update # 3-way merge; preserves the Step 2 local edits +gh aw compile # --approve only after reviewing any new secret +``` + +Then, in this order, because each step can invalidate the next: + +1. **Read the release notes between the two versions** + (`workflows/review/CHANGELOG.md` in `Khan/actions`, from the pinned version to + the new one). Semver is a behaviour contract here: a minor can change what gets + reviewed, what a label means, or what the router does with an existing `ROUTING` + line. This is the step that tells you what to look for in the rest. +2. **Confirm every local edit survived the merge.** `gh aw update` preserves them, + but "preserved" is not "still correct", and a conflict resolved badly is silent. + Diff the installed `review.md` against the new tag's copy and check that what + differs is exactly the Step 2 list and nothing else: + + ```sh + git -C diff HEAD~1 -- .github/workflows/review.md + ``` + + Check `max-ai-credits` **and** its `REVIEW_MAX_AI_CREDITS` mirror together (a + merge can update one), that a commented-out `observability:` block is still + commented out, that a public repo's fork guard is still in the `if:`, and that + `source:` names the new tag rather than a resolved SHA. +3. **Look for newly generated files.** A new gh-aw version can start emitting a + file the old one did not, and an unmarked generated file gets line-reviewed. + Check `git status` for additions under `.github/`, and that `.gitattributes` + covers each one (this is exactly how `agentics-maintenance.yml` arrived). +4. **Re-run the checker from the NEW tag**, not the old one — the semantics you are + validating are the ones the repo is about to run. Compare its tier preview to + the previous run: a routing-semantics change can move files between tiers + without any `ROUTING` edit, and that is worth knowing before the next review + rather than after. +5. **Re-read `ci-tooling.md` and `skills.md` against the repo as it is now.** These + rot silently and nothing else checks them: a lint rule the repo dropped, a test + command that changed, a skill path that moved or was deleted. A `skills.md` + entry pointing at a file that no longer exists degrades the `skill-auditor` on + every review. This is the highest-value part of an audit and the easiest to skip. + +The PR body is much smaller than an install's, but keep the same shape: old +version → new version, what the changelog says changed, that the local edits are +unchanged (or which moved and why), the checker's before/after tier counts, and +anything you did not verify. If the tier counts or enabled roster changed, say so +explicitly — that is the part a reviewer cannot see from the diff. + +If the pinned version is far behind, bump one **minor** at a time rather than +jumping several: each step's changelog is then the explanation for that step's +diff, and a behaviour change that needs a config response is attributable. + +## Provisioning the `ANTHROPIC_API_KEY` + +When preflight (Step 0) shows the repo cannot see an `ANTHROPIC_API_KEY` — not +as a repo secret and not via an org secret's selected-repositories list — the +key has to be requested. The operator does not create it and never handles its +value (see *What stays human*); IT does both: + +1. **Request it in the `#it` Slack channel.** Name the repo + (`/`), say it is for the shared AI PR reviewer, and ask IT to + create an Anthropic API key and add it to the repo's Actions secrets as + `ANTHROPIC_API_KEY`. If an org-level `ANTHROPIC_API_KEY` secret already + exists, ask instead that this repo be added to its selected-repositories + list — no key material moves at all. +2. **IT creates the key and adds the secret.** The key value goes straight from + IT into GitHub; it never passes through the operator, the PR, or a chat + message to you. +3. **Confirm by name only** once IT says it is done: + + ```sh + gh secret list -R / + gh api /repos///actions/organization-secrets --jq '.secrets[].name' + ``` + +List the request as a blocker in the Step 7 PR body until the confirmation +succeeds — it is the hard one: without the key the agent job fails immediately +on the first run. diff --git a/workflows/review/README.md b/workflows/review/README.md index a50edd67..bdff77ee 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -147,6 +147,57 @@ flow rewrites it; see [Versioning](#versioning)), so after `gh aw add` or `gh aw update` the imported file already fetches the matching lib code and needs no manual fix-up of the ref. +### Onboarding a whole repo + +`gh aw add` is the mechanical half. The judgment half (writing the five consumer +config files for *that* repo's blast radius, the two local edits to the installed +`review.md`, the admin blockers only a repo admin can clear, and a PR body that +discloses what is generated versus decided) is choreographed by the +[`review-onboarding` skill](../../.claude/skills/review-onboarding/SKILL.md) +in this repo. [`Khan/kore-marketplace#3`](https://github.com/Khan/kore-marketplace/pull/3) +is the template it produces. + +Validate any install (new or existing) with the consumer-config checker, run from +a checkout of the tag the consumer pins: + +```sh +git -C ls-files | npx -y tsx workflows/review/lib/check-consumer-config.ts \ + --repo --files-from - + +# Why does this path get this tier? (every matching ROUTING rule, last one wins) +npx -y tsx workflows/review/lib/check-consumer-config.ts --repo --explain +``` + +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 +`${{ }}` expression inside one, `add-reviewer` defined in both `review.md` and +`config.md` (the main workflow wins, discarding the allowlist), a dropped +`imports:` line, an empty team allowlist in a repo that *has* a `.github/REVIEWERS` +ownership map, a missing or unmarked `.lock.yml`, an unmarked +`agentics-maintenance.yml`, a live `observability:` block, the shipped credit +ceiling, `ROUTING` parse warnings, inert lens payloads, reviewer config that does +not route to `high`, and the resolved tier of every tracked file. Errors exit 1; +`--strict` also fails on warnings, and `--json` emits the report for tooling. + +Two of those deserve a note, because both are "valid, but invisible" rather than +broken. An empty `allowed-team-reviewers` is only an **error** when +`.github/REVIEWERS` exists: that file is the router's only source of ownership, so +without it Step 8 requests nobody regardless, and the empty allowlist is an accurate +statement that the repo does not do reviewer requests (reported as +`reviewer-requests-inert`). Requiring a team there would only get an inert one +invented to satisfy the check. And `agentics-maintenance.yml` is `gh aw compile` +output that is *not* named `*.lock.yml`, so the marker every consumer was told to add +misses it, and ~600 generated lines get line-reviewed until it has its own. + +Follow-up, not yet built: the failure class the checker targets (config failing +late and quietly) recurs on every later edit to ROUTING or `config.md`, not just +at onboarding. `checkConsumerConfig` is pure with an injected filesystem and +already ships `--json` and `--strict`, so a consumer CI job gating PRs that +touch the config paths is the natural next layer; nothing in the checker +blocks it. + ## Consumer configuration The workflow imports the following files **from the consuming repo** (they resolve diff --git a/workflows/review/eval/live-producer.ts b/workflows/review/eval/live-producer.ts index cae79669..46e65379 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -737,7 +737,7 @@ export const produceLive = async ( // diff the staging already wrote); `flip-gated` keeps only the correctness // pass; `fast` keeps none. const routerConfig: RouterConfig = { - generatedPatterns: [], + generatedRules: [], ...(corpusCase.routerConfig as Partial), }; const routing = route({files: corpusCase.changedFiles}, routerConfig); diff --git a/workflows/review/eval/live-stage.ts b/workflows/review/eval/live-stage.ts index b498cf2e..c314378a 100644 --- a/workflows/review/eval/live-stage.ts +++ b/workflows/review/eval/live-stage.ts @@ -291,7 +291,7 @@ export const stageCase = ( // Deterministic routing, exactly as the no-post runner computes it. const routerConfig: RouterConfig = { - generatedPatterns: [], + generatedRules: [], ...(corpusCase.routerConfig as Partial), }; const routing = route({files: corpusCase.changedFiles}, routerConfig); diff --git a/workflows/review/eval/runner.ts b/workflows/review/eval/runner.ts index d39abd46..8bb1b975 100644 --- a/workflows/review/eval/runner.ts +++ b/workflows/review/eval/runner.ts @@ -350,7 +350,7 @@ export const runCase = ( ): RunResult => { // 1. Deterministic routing over the changed files. const routerConfig: RouterConfig = { - generatedPatterns: [], + generatedRules: [], ...(corpusCase.routerConfig as Partial), }; const routing = route({files: corpusCase.changedFiles}, routerConfig); diff --git a/workflows/review/lib/check-consumer-config-report.ts b/workflows/review/lib/check-consumer-config-report.ts new file mode 100644 index 00000000..567e7fc5 --- /dev/null +++ b/workflows/review/lib/check-consumer-config-report.ts @@ -0,0 +1,134 @@ +/** + * Text rendering for the consumer-config checker, split out of + * `check-consumer-config.ts` by concern and its max-lines budget (the same split + * `glob-match.ts` took from `router.ts`). The checker produces the report as + * *data* and this module is the only place that decides how it reads, so a new + * check needs no rendering change and a formatting change cannot alter a verdict. + * + * Determinism boundary: a pure function of the report. No filesystem, no model + * call, no process exit — the caller owns those. + */ + +import {RISK_TIERS} from "./router"; +import type {ConfigIssue, ConsumerConfigReport} from "./check-consumer-config"; + +/** Human-readable report. Errors first, then warnings, then the summary. */ +export const renderReport = (report: ConsumerConfigReport): string => { + const lines: string[] = []; + const errors = report.issues.filter((issue) => issue.severity === "error"); + const warnings = report.issues.filter( + (issue) => issue.severity === "warning", + ); + + lines.push(`Reviewer config check: ${report.repoRoot}`); + lines.push(""); + + const section = (title: string, issues: ConfigIssue[]): void => { + if (issues.length === 0) { + return; + } + lines.push(`${title} (${issues.length})`); + for (const issue of issues) { + lines.push(` [${issue.code}] ${issue.message}`); + if (issue.fix !== undefined) { + lines.push(` fix: ${issue.fix}`); + } + } + lines.push(""); + }; + section("ERRORS", errors); + section("WARNINGS", warnings); + + const {routing, installedWorkflow: wf} = report; + lines.push("Install"); + lines.push( + ` source ${wf.source ?? "(none)"}${ + wf.present ? "" : " [workflow not installed]" + }`, + ); + lines.push(` lock ${wf.lockPresent ? "present" : "MISSING"}`); + lines.push(` max-ai-credits ${wf.maxAiCredits ?? "(shipped default)"}`); + lines.push( + ` observability ${wf.observabilityActive ? "active" : "disabled"}`, + ); + lines.push( + ` reviewer teams ${ + report.reviewerRouting.allowedTeamReviewers.join(", ") || "(none)" + }`, + ); + lines.push(""); + lines.push("ROUTING"); + lines.push(` present ${routing.present ? "yes" : "NO"}`); + lines.push( + ` tier rules ${routing.tierRules} lens rules ${routing.lensRules}`, + ); + lines.push( + ` enabled reviewers ${ + routing.enabledReviewers.join(", ") || "(none)" + }`, + ); + lines.push(` re-review ${routing.reReviewMode}`); + + if (report.tierPreview !== undefined) { + const preview = report.tierPreview; + lines.push(""); + lines.push( + `Tier preview over ${preview.fileCount} tracked files (${preview.generated} generated)`, + ); + for (const tier of RISK_TIERS) { + const count = preview.counts[tier]; + const sample = preview.samples[tier].join(", "); + lines.push( + ` ${tier.padEnd(8)} ${String(count).padStart(5)}${ + sample === "" ? "" : ` e.g. ${sample}` + }`, + ); + } + if (preview.deadPatterns.length > 0) { + lines.push( + ` dead patterns (match nothing): ${preview.deadPatterns.join( + ", ", + )}`, + ); + } + lines.push( + ` lenses on a whole-repo change: ${ + preview.lensesToSpawn.join(", ") || "(none)" + }`, + ); + } + + if (report.explanation !== undefined) { + const explanation = report.explanation; + lines.push(""); + lines.push(`Explanation: ${explanation.path}`); + lines.push( + ` tier ${explanation.tier}${ + explanation.tierPending ? " (pending: direction-dependent)" : "" + }${explanation.generated ? " (generated)" : ""}`, + ); + lines.push(` lenses ${explanation.lenses.join(", ") || "(none)"}`); + if (explanation.matchingTierRules.length === 0) { + lines.push(" rules (none matched; router default tier)"); + } else { + lines.push(" rules (last one wins)"); + for (const rule of explanation.matchingTierRules) { + lines.push( + ` ${rule.pattern} tier=${rule.tier}${ + rule.diffDirectionDependent + ? " direction-dependent" + : "" + }`, + ); + } + } + } + + lines.push(""); + lines.push( + errors.length === 0 + ? `PASS with ${warnings.length} warning(s).` + : `FAIL: ${errors.length} error(s), ${warnings.length} warning(s).`, + ); + return `${lines.join("\n")}\n`; +}; diff --git a/workflows/review/lib/check-consumer-config.test.ts b/workflows/review/lib/check-consumer-config.test.ts new file mode 100644 index 00000000..a69c4fe2 --- /dev/null +++ b/workflows/review/lib/check-consumer-config.test.ts @@ -0,0 +1,717 @@ +import {describe, it, expect} from "vitest"; + +import { + checkConsumerConfig, + CONFIG_IMPORT_PATH, + INSTALLED_LOCK_PATH, + INSTALLED_WORKFLOW_PATH, + MAINTENANCE_WORKFLOW_PATH, + parseArgs, + REQUIRED_RUNTIME_IMPORTS, + renderReport, +} from "./check-consumer-config.ts"; +import type {ConsumerConfigFs} from "./check-consumer-config.ts"; +import {ROUTING_CONFIG_PATH} from "./routing-config.ts"; +import {REVIEWERS_PATH} from "./router.ts"; + +/** + * Consumer-config checker tests. Each case is one way an install fails silently + * in production; the whole point of the checker is that none of these surfaces + * at `gh aw compile` time, so the test is the only place they are pinned besides + * a real PR going wrong. + * + * Routing semantics themselves are NOT re-tested here (router.test.ts owns + * those); what these assert is that the checker asks the real router and reports + * its answer, so the tier assertions below are deliberately thin. + */ + +/** In-memory fs. A directory "exists" when some key sits beneath it. */ +const fakeFs = (inputs: Record): ConsumerConfigFs => ({ + readFileSync: (p: string): string => { + const content = inputs[p]; + if (content === undefined) { + throw new Error(`unexpected read: ${p}`); + } + return content; + }, + existsSync: (p: string): boolean => + p in inputs || + Object.keys(inputs).some((key) => key.startsWith(`${p}/`)), + readdirSync: (p: string): string[] => { + if (p in inputs) { + throw new Error(`ENOTDIR: not a directory, scandir '${p}'`); + } + return Object.keys(inputs) + .filter((key) => key.startsWith(`${p}/`)) + .map((key) => key.slice(p.length + 1).split("/")[0]); + }, +}); + +const WORKFLOW_MD = `--- +imports: + - ${CONFIG_IMPORT_PATH} +permissions: + contents: read +safe-outputs: + submit-pull-request-review: + max: 1 +max-ai-credits: 2500 +env: + REVIEW_MAX_AI_CREDITS: "2500" +source: Khan/actions/workflows/review/review.md@review-v1.11.0 +--- + +Prompt body. +`; + +const CONFIG_MD = `--- +safe-outputs: + add-reviewer: + target: "triggering" + max: 2 + allowed-team-reviewers: + - kore + github-token: \${{ secrets.KHAN_ACTIONS_BOT_TOKEN }} +--- +`; + +// Realistic ordering: the broad docs rule first, its executable-prose +// exceptions after it (last matching rule wins). +const ROUTING = `enable holistic,completeness +re-review scoped +**/*.md tier=trivial +plugins/** tier=high +plugins/*/skills/*/SKILL.md tier=high +.github/workflows/** tier=high +.github/workflows/*.md tier=high +.github/aw/review/** tier=high +`; + +/** A fully valid install: every later case mutates one thing out of this. */ +const validInstall = (): Record => ({ + [INSTALLED_WORKFLOW_PATH]: WORKFLOW_MD, + [INSTALLED_LOCK_PATH]: "# compiled\n", + [CONFIG_IMPORT_PATH]: CONFIG_MD, + [REQUIRED_RUNTIME_IMPORTS[0]]: "### High Risk\n\n- plugins/**\n", + [REQUIRED_RUNTIME_IMPORTS[1]]: "- **Lint**: eslint.\n", + [REQUIRED_RUNTIME_IMPORTS[2]]: "### conventions - `README.md`\n", + [ROUTING_CONFIG_PATH]: ROUTING, + ".gitattributes": ".github/workflows/*.lock.yml linguist-generated=true\n", +}); + +const codes = ( + report: ReturnType, + severity: "error" | "warning", +): string[] => + report.issues + .filter((issue) => issue.severity === severity) + .map((issue) => issue.code); + +const check = ( + inputs: Record, + options: Parameters[1] = {}, +) => checkConsumerConfig(fakeFs(inputs), options); + +describe("a valid install", () => { + it("reports no errors and reads the install back", () => { + const report = check(validInstall(), {checkerVersion: "1.11.0"}); + expect(codes(report, "error")).toEqual([]); + expect(codes(report, "warning")).toEqual([]); + expect(report.installedWorkflow.pinnedRef).toBe("review-v1.11.0"); + expect(report.installedWorkflow.maxAiCredits).toBe(2500); + expect(report.installedWorkflow.observabilityActive).toBe(false); + expect(report.reviewerRouting.allowedTeamReviewers).toEqual(["kore"]); + expect(report.reviewerRouting.hasGithubToken).toBe(true); + expect(report.routing.enabledReviewers).toEqual([ + "holistic", + "completeness", + ]); + expect(report.routing.reReviewMode).toBe("scoped"); + }); + + it("renders a PASS line", () => { + const rendered = renderReport( + check(validInstall(), {checkerVersion: "1.11.0"}), + ); + expect(rendered).toContain("PASS with 0 warning(s)."); + }); +}); + +describe("required config files", () => { + it("errors once per missing file, naming when it would fail", () => { + const report = check({}); + expect(codes(report, "error")).toEqual([ + "missing-required-config", + "missing-required-config", + "missing-required-config", + "missing-required-config", + "workflow-not-installed", + ]); + const compileTime = report.issues.find((issue) => + issue.message.startsWith(CONFIG_IMPORT_PATH), + ); + expect(compileTime?.message).toContain("gh aw compile"); + }); + + it("errors on an empty required file", () => { + const inputs = validInstall(); + inputs[REQUIRED_RUNTIME_IMPORTS[2]] = "\n \n"; + expect(codes(check(inputs), "error")).toEqual([ + "empty-required-config", + ]); + }); + + it("errors on a template expression inside a runtime import", () => { + const inputs = validInstall(); + inputs[ + REQUIRED_RUNTIME_IMPORTS[1] + ] = `- Lint runs on \${{ github.sha }}\n`; + expect(codes(check(inputs), "error")).toEqual([ + "template-expression-in-import", + ]); + }); + + it("exempts config.md, whose bot token is a template expression by design", () => { + // CONFIG_MD carries ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }} and is a + // frontmatter import, not a runtime one. + expect(codes(check(validInstall()), "error")).toEqual([]); + }); + + it("flags a template expression in a lens payload", () => { + const inputs = validInstall(); + inputs[".github/aw/review/lenses/security-auth.md"] = + "- check ${{ secrets.FOO }}\n"; + expect(codes(check(inputs), "error")).toEqual([ + "template-expression-in-import", + ]); + }); +}); + +describe("the add-reviewer contract", () => { + it("errors when the main workflow defines add-reviewer too", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + " submit-pull-request-review:\n max: 1\n", + " add-reviewer:\n max: 2\n", + ); + expect(codes(check(inputs), "error")).toEqual([ + "workflow-defines-add-reviewer", + ]); + }); + + it("errors when the workflow drops the config import", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + `imports:\n - ${CONFIG_IMPORT_PATH}\n`, + "", + ); + expect(codes(check(inputs), "error")).toEqual([ + "workflow-missing-config-import", + ]); + }); + + it("errors on an empty team allowlist when the repo has an ownership map", () => { + const inputs = validInstall(); + inputs[REVIEWERS_PATH] = "plugins/** @Khan/kore\n"; + inputs[CONFIG_IMPORT_PATH] = CONFIG_MD.replace( + " allowed-team-reviewers:\n - kore\n", + "", + ); + expect(codes(check(inputs), "error")).toEqual([ + "config-empty-team-allowlist", + ]); + }); + + // The distinction the error above rests on. With no REVIEWERS file the router + // derives no owners and no ranked fallback, so Step 8 requests nobody whatever + // the allowlist says: "no teams" is then an accurate configuration rather than a + // dropped request, and erroring would only get an inert team invented to satisfy + // the checker. + it("treats an empty allowlist as deliberate when there is no ownership map", () => { + const inputs = validInstall(); + inputs[CONFIG_IMPORT_PATH] = CONFIG_MD.replace( + " allowed-team-reviewers:\n - kore\n", + "", + ); + const report = check(inputs); + expect(codes(report, "error")).toEqual([]); + expect(codes(report, "warning")).toEqual(["reviewer-requests-inert"]); + }); + + // Present-but-empty is a different animal from absent, and the checker must + // not report it as the deliberate no-requests configuration: that would read + // as an all-clear over an allowlist the safe output is dropping. + it("errors on an allowlist key that yields no teams, with no ownership map", () => { + const inputs = validInstall(); + inputs[CONFIG_IMPORT_PATH] = CONFIG_MD.replace(" - kore\n", ""); + const report = check(inputs); + expect(codes(report, "error")).toEqual(["config-empty-team-allowlist"]); + expect(codes(report, "warning")).not.toContain( + "reviewer-requests-inert", + ); + }); + + // Flow style is valid YAML and the shipped review.md uses it for `toolsets`, + // so reading it as an empty allowlist was a false error on a working install. + it("reads a flow-style allowlist", () => { + const inputs = validInstall(); + inputs[CONFIG_IMPORT_PATH] = CONFIG_MD.replace( + " allowed-team-reviewers:\n - kore\n", + " allowed-team-reviewers: [kore, web]\n", + ); + const report = check(inputs); + expect(codes(report, "error")).toEqual([]); + expect(report.reviewerRouting.allowedTeamReviewers).toEqual([ + "kore", + "web", + ]); + }); + + it("stays quiet about the bot token when no request can be made", () => { + const inputs = validInstall(); + inputs[CONFIG_IMPORT_PATH] = CONFIG_MD.replace( + " allowed-team-reviewers:\n - kore\n", + "", + ).replace( + " github-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}\n", + "", + ); + expect(codes(check(inputs), "warning")).toEqual([ + "reviewer-requests-inert", + ]); + }); + + it("errors when config.md carries no add-reviewer at all", () => { + const inputs = validInstall(); + inputs[CONFIG_IMPORT_PATH] = + "---\nsafe-outputs:\n add-comment:\n---\n"; + expect(codes(check(inputs), "error")).toEqual([ + "config-missing-add-reviewer", + ]); + }); + + it("warns when add-reviewer names no bot token", () => { + const inputs = validInstall(); + inputs[CONFIG_IMPORT_PATH] = CONFIG_MD.replace( + " github-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}\n", + "", + ); + expect(codes(check(inputs), "warning")).toEqual([ + "config-no-bot-token", + ]); + }); +}); + +// Every local edit the skill prescribes carries a ` LOCAL OVERRIDE:` +// comment, so an inline one on the very lines the checker reads is the expected +// shape rather than an exotic case. +describe("labelled local edits", () => { + it("still reads max-ai-credits, source and imports through inline comments", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + "max-ai-credits: 2500", + "max-ai-credits: 2500 # KHAN/REPO LOCAL OVERRIDE: raised", + ) + .replace( + "source: Khan/actions/workflows/review/review.md@review-v1.11.0", + "source: Khan/actions/workflows/review/review.md@review-v1.11.0 # pinned", + ) + .replace( + ` - ${CONFIG_IMPORT_PATH}`, + ` - ${CONFIG_IMPORT_PATH} # consumer config`, + ); + const report = check(inputs, {checkerVersion: "1.11.0"}); + expect(codes(report, "error")).toEqual([]); + expect(codes(report, "warning")).toEqual([]); + expect(report.installedWorkflow.maxAiCredits).toBe(2500); + expect(report.installedWorkflow.pinnedRef).toBe("review-v1.11.0"); + }); + + it("does not let a quoted credit ceiling suppress the default warning", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + "max-ai-credits: 2500", + 'max-ai-credits: "1000"', + ).replace( + 'REVIEW_MAX_AI_CREDITS: "2500"', + 'REVIEW_MAX_AI_CREDITS: "1000"', + ); + expect(codes(check(inputs), "warning")).toEqual([ + "max-ai-credits-default", + ]); + }); + + it("accepts a quoted config import", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + ` - ${CONFIG_IMPORT_PATH}`, + ` - "${CONFIG_IMPORT_PATH}"`, + ); + expect(codes(check(inputs), "error")).toEqual([]); + }); +}); + +describe("the compiled lock", () => { + it("errors when the lock is missing (nothing runs)", () => { + const inputs = validInstall(); + delete inputs[INSTALLED_LOCK_PATH]; + expect(codes(check(inputs), "error")).toEqual(["lock-missing"]); + }); + + // Cause-then-effect: a missing lock is the error above, not additionally + // a nag to mark the nonexistent file as generated (which would also flip + // the --strict exit code). + it("does not ask a lock-less repo to mark the lock generated", () => { + const inputs = validInstall(); + delete inputs[INSTALLED_LOCK_PATH]; + delete inputs[".gitattributes"]; + expect(codes(check(inputs), "warning")).not.toContain( + "lock-not-marked-generated", + ); + }); + + it("derives the lock path from a renamed --workflow and checks it", () => { + const renamed = ".github/workflows/pr-review.md"; + const inputs = validInstall(); + inputs[renamed] = inputs[INSTALLED_WORKFLOW_PATH]; + delete inputs[INSTALLED_WORKFLOW_PATH]; + delete inputs[INSTALLED_LOCK_PATH]; + // Lock absent under the derived name: the error names it. + const missing = check(inputs, {workflowPath: renamed}); + expect(codes(missing, "error")).toEqual(["lock-missing"]); + expect( + missing.issues.find((issue) => issue.code === "lock-missing") + ?.message, + ).toContain(".github/workflows/pr-review.lock.yml"); + // Lock present under the derived name (and marked generated): quiet. + inputs[".github/workflows/pr-review.lock.yml"] = "# compiled\n"; + expect(codes(check(inputs, {workflowPath: renamed}), "error")).toEqual( + [], + ); + }); + + it("warns when the lock is not marked linguist-generated", () => { + const inputs = validInstall(); + delete inputs[".gitattributes"]; + expect(codes(check(inputs), "warning")).toEqual([ + "lock-not-marked-generated", + ]); + }); + + // The gap the documented `*.lock.yml` marker leaves: gh-aw's maintenance + // workflow is compiler output too, but its name does not end in `.lock.yml`, so + // a repo that followed the instructions still gets ~600 generated lines + // line-reviewed. + it("warns when the gh-aw maintenance workflow is not marked generated", () => { + const inputs = validInstall(); + inputs[MAINTENANCE_WORKFLOW_PATH] = "# generated by gh aw\n"; + expect(codes(check(inputs), "warning")).toEqual([ + "maintenance-workflow-not-marked-generated", + ]); + }); + + it("is quiet once the maintenance workflow is marked generated", () => { + const inputs = validInstall(); + inputs[MAINTENANCE_WORKFLOW_PATH] = "# generated by gh aw\n"; + const marker = `${MAINTENANCE_WORKFLOW_PATH} linguist-generated=true\n`; + inputs[".gitattributes"] += marker; + expect(codes(check(inputs), "warning")).toEqual([]); + }); + + it("says nothing about a maintenance workflow the repo does not have", () => { + expect(codes(check(validInstall()), "warning")).toEqual([]); + }); +}); + +describe("the pinned source", () => { + it("warns when the install tracks a branch instead of a tag", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + "@review-v1.11.0", + "", + ); + expect(codes(check(inputs), "warning")).toEqual(["source-unpinned"]); + }); + + it("warns when the checker's own version is not the pinned one", () => { + const report = check(validInstall(), {checkerVersion: "1.12.0"}); + expect(codes(report, "warning")).toEqual(["source-ref-mismatch"]); + }); + + it("stays quiet about the version when the checker does not know its own", () => { + expect(codes(check(validInstall()), "warning")).toEqual([]); + }); +}); + +describe("the source field", () => { + it("warns when review.md carries no source: field at all", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + /^source: .*$\n/m, + "", + ); + expect(codes(check(inputs), "warning")).toEqual(["source-missing"]); + }); +}); + +describe("the credit-cap mirror", () => { + it("warns when the env mirror is missing while the cap is raised", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + /^env:\n {2}REVIEW_MAX_AI_CREDITS: "2500"\n/m, + "", + ); + const report = check(inputs); + expect(codes(report, "warning")).toEqual([ + "max-ai-credits-mirror-stale", + ]); + expect( + report.issues.find( + (issue) => issue.code === "max-ai-credits-mirror-stale", + )?.message, + ).toContain("no REVIEW_MAX_AI_CREDITS env mirror"); + }); + + it("warns when the mirror disagrees with the frontmatter cap", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + 'REVIEW_MAX_AI_CREDITS: "2500"', + 'REVIEW_MAX_AI_CREDITS: "1000"', + ); + const report = check(inputs); + expect(codes(report, "warning")).toEqual([ + "max-ai-credits-mirror-stale", + ]); + expect( + report.issues.find( + (issue) => issue.code === "max-ai-credits-mirror-stale", + )?.message, + ).toContain("says 1000"); + }); +}); + +describe("the shipped credit ceiling", () => { + it("compares against the live shipped value when the CLI provides one", () => { + // The shipped ceiling rose to meet the consumer's: the default warning + // fires again, instead of comparing against the stale constant. + const report = check(validInstall(), {shippedMaxAiCredits: 2500}); + expect(codes(report, "warning")).toContain("max-ai-credits-default"); + }); +}); + +describe("lens payloads", () => { + it("forwards the real lens-payload warnings", () => { + const inputs = validInstall(); + // A specialist-lens payload no ROUTING rule routes: inert. + inputs[".github/aw/review/lenses/security-auth.md"] = + "### security-auth - extra rules\n"; + const report = check(inputs); + expect(codes(report, "warning")).toContain("lens-payload-warning"); + expect( + report.issues.find((issue) => issue.code === "lens-payload-warning") + ?.message, + ).toContain("inert"); + }); +}); + +describe("local edits the README prescribes", () => { + it("warns when the observability block is live", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + "max-ai-credits: 2500\n", + "observability:\n otlp:\n exporters: []\nmax-ai-credits: 2500\n", + ); + expect(codes(check(inputs), "warning")).toEqual([ + "observability-active", + ]); + }); + + it("warns when max-ai-credits is still the shipped ceiling", () => { + const inputs = validInstall(); + inputs[INSTALLED_WORKFLOW_PATH] = WORKFLOW_MD.replace( + "max-ai-credits: 2500", + "max-ai-credits: 1000", + ).replace( + 'REVIEW_MAX_AI_CREDITS: "2500"', + 'REVIEW_MAX_AI_CREDITS: "1000"', + ); + expect(codes(check(inputs), "warning")).toEqual([ + "max-ai-credits-default", + ]); + }); +}); + +describe("ROUTING", () => { + it("warns when absent, and does not then nag about tiers it cannot know", () => { + const inputs = validInstall(); + delete inputs[ROUTING_CONFIG_PATH]; + const warnings = codes(check(inputs), "warning"); + expect(warnings).toContain("routing-missing"); + expect(warnings).not.toContain("reviewer-config-not-high"); + }); + + it("forwards the real parser's warnings", () => { + const inputs = validInstall(); + inputs[ROUTING_CONFIG_PATH] = `${ROUTING}src/** lens=not-a-lens\n`; + const report = check(inputs); + expect(codes(report, "warning")).toContain("routing-parse-warning"); + expect( + report.issues.find( + (issue) => issue.code === "routing-parse-warning", + )?.message, + ).toContain("not-a-lens"); + }); + + it("warns when no opt-in reviewer is enabled and the mode is still full", () => { + const inputs = validInstall(); + inputs[ROUTING_CONFIG_PATH] = ".github/aw/review/** tier=high\n"; + const warnings = codes(check(inputs), "warning"); + expect(warnings).toContain("no-enabled-reviewers"); + expect(warnings).toContain("re-review-full"); + }); + + it("warns when the reviewer's own config does not route to high", () => { + const inputs = validInstall(); + inputs[ROUTING_CONFIG_PATH] = "enable holistic\nre-review scoped\n"; + const report = check(inputs); + expect(codes(report, "warning")).toContain("reviewer-config-not-high"); + expect(report.configFileTiers[ROUTING_CONFIG_PATH]).toBe("low"); + }); +}); + +describe("tier preview", () => { + // Every pattern in ROUTING matches at least one of these, so a dead pattern + // in a later case is the case's own doing. This is the shape of the real + // invocation: the whole tracked set, from `git ls-files`. + const files = [ + "plugins/kore/skills/support/SKILL.md", + "plugins/kore/scripts/run.py", + "README.md", + "docs/notes.md", + ".github/workflows/review.md", + ".github/workflows/review.lock.yml", + ".github/aw/review/ROUTING", + "Makefile", + ]; + + it("counts tiers from the real router and lists rule-less files", () => { + const report = check(validInstall(), {files}); + const preview = report.tierPreview; + expect(preview?.fileCount).toBe(8); + // The lock is linguist-generated, so it is classified, not tiered. + expect(preview?.generated).toBe(1); + expect(preview?.counts.high).toBe(4); + expect(preview?.counts.trivial).toBe(2); + // Only the file no `tier=` rule matches at all. + expect(preview?.unmatched).toEqual(["Makefile"]); + expect(preview?.deadPatterns).toEqual([]); + expect(codes(report, "warning")).toEqual(["files-without-tier-rule"]); + }); + + it("names patterns that match nothing, typo or not", () => { + const inputs = validInstall(); + // `plugin/**` is a plausible typo for `plugins/**`, and the misspelling + // parses perfectly: only the file list reveals that it routes nothing. + inputs[ROUTING_CONFIG_PATH] = `${ROUTING}plugin/** tier=high\n`; + const report = check(inputs, {files}); + expect(report.tierPreview?.deadPatterns).toEqual(["plugin/**"]); + expect(codes(report, "warning")).toContain( + "routing-pattern-matches-nothing", + ); + expect(renderReport(report)).toContain( + "dead patterns (match nothing): plugin/**", + ); + }); + + it("treats a no-slash pattern as a basename match, and a leading slash as root-anchored", () => { + // `notes.md` (no slash) matches the basename in ANY directory, while + // `/notes.md` matches only the root file. Pinned here because Step 4 of + // the onboarding skill tells authors to rely on both. + const anchored = validInstall(); + anchored[ROUTING_CONFIG_PATH] = `${ROUTING}/notes.md tier=high\n`; + // No root notes.md in the file list, so the anchored rule is dead... + expect(check(anchored, {files}).tierPreview?.deadPatterns).toEqual([ + "/notes.md", + ]); + // ...and the nested file it looks like it names keeps its docs tier. + expect( + check(anchored, {explainPath: "docs/notes.md"}).explanation?.tier, + ).toBe("trivial"); + + const bare = validInstall(); + bare[ROUTING_CONFIG_PATH] = `${ROUTING}notes.md tier=high\n`; + expect(check(bare, {files}).tierPreview?.deadPatterns).toEqual([]); + expect( + check(bare, {explainPath: "docs/notes.md"}).explanation?.tier, + ).toBe("high"); + }); + + it("is absent when no file list is supplied", () => { + expect(check(validInstall()).tierPreview).toBeUndefined(); + }); +}); + +describe("explain", () => { + it("lists every matching rule in file order, last one winning", () => { + const report = check(validInstall(), { + explainPath: "plugins/kore/skills/support/SKILL.md", + }); + expect(report.explanation?.tier).toBe("high"); + expect( + report.explanation?.matchingTierRules.map((rule) => rule.pattern), + ).toEqual(["**/*.md", "plugins/**", "plugins/*/skills/*/SKILL.md"]); + expect(renderReport(report)).toContain("rules (last one wins)"); + }); + + it("reports a generated file as generated", () => { + const report = check(validInstall(), { + explainPath: ".github/workflows/review.lock.yml", + }); + expect(report.explanation?.generated).toBe(true); + expect(report.explanation?.tier).toBe("trivial"); + }); +}); + +describe("leftover scaffolding", () => { + it("warns about the gh aw init Copilot files", () => { + const inputs = validInstall(); + inputs[".github/mcp.json"] = "{}\n"; + inputs[".github/agents/example.md"] = "# agent\n"; + const report = check(inputs); + expect(codes(report, "warning")).toEqual([ + "copilot-scaffolding-present", + ]); + expect( + report.issues.find( + (issue) => issue.code === "copilot-scaffolding-present", + )?.message, + ).toContain(".github/mcp.json, .github/agents"); + }); +}); + +describe("parseArgs", () => { + it("reads the flags the CLI documents", () => { + expect( + parseArgs([ + "--repo", + "../consumer", + "--files-from", + "-", + "--explain", + "src/a.ts", + "--json", + "--strict", + ]), + ).toEqual({ + repoRoot: "../consumer", + filesFrom: "-", + explainPath: "src/a.ts", + json: true, + strict: true, + }); + }); + + it("rejects an unknown flag rather than ignoring it", () => { + expect(() => parseArgs(["--nope"])).toThrow("unknown argument: --nope"); + }); +}); diff --git a/workflows/review/lib/check-consumer-config.ts b/workflows/review/lib/check-consumer-config.ts new file mode 100644 index 00000000..9b38dcc1 --- /dev/null +++ b/workflows/review/lib/check-consumer-config.ts @@ -0,0 +1,1000 @@ +/** + * Consumer-config checker: validates a repo's `.github/aw/review/` install of + * the shared PR reviewer against the *same* code the workflow runs. + * + * Onboarding a repo means writing five files by hand (`config.md`, + * `risk-classification.md`, `ci-tooling.md`, `skills.md`, `ROUTING`) and making + * two local edits to the installed `review.md`. Every mistake in that set fails + * *late* and quietly: a missing `{{#runtime-import}}` target surfaces as + * `Runtime import file not found` on the next PR rather than at `gh aw compile` + * time; `add-reviewer` defined in `review.md` as well as `config.md` silently + * discards the consumer's team allowlist (the main workflow wins); a `ROUTING` + * typo degrades to fewer lenses and a floored budget, visible only as a `Note:` + * on a review that already happened; an active `observability:` block without + * both `GH_AW_OTEL_SENTRY_*` secrets kills the agent job at startup. + * + * So this module answers "would this install work, and does it route the way + * the author intended?" *before* the first PR. It never reimplements routing + * semantics: tiers, lenses, generated-file classification and the budget all + * come from {@link route}, `ROUTING` is read by {@link parseRoutingConfig}, and + * `.gitattributes` by {@link parseGitattributesGenerated}. A release that + * changes those semantics changes this checker's answers for free, which is why + * it ships in `lib/` beside them and is run from the tag the repo pins. + * + * Usage (from a checkout of this repo at the tag the consumer pins): + * + * git ls-files | npx -y tsx workflows/review/lib/check-consumer-config.ts \ + * --repo ../consumer-repo --files-from - + * npx -y tsx workflows/review/lib/check-consumer-config.ts \ + * --repo ../consumer-repo --explain plugins/foo/skills/bar/SKILL.md + * + * Errors exit 1; warnings exit 0 unless `--strict`. `--json` prints the report + * instead of the text rendering. + */ + +import { + isGenerated, + parseGitattributesGenerated, + parseRoutingConfig, + ROUTING_CONFIG_PATH, + route, + matchesGlob, + lensPayloadWarnings, + SPECIALIST_LENSES, + CORRECTNESS_ALIAS_PATH, + LENS_PAYLOAD_DIR, + DEFAULT_RE_REVIEW_MODE, + REVIEWERS_PATH, +} from "./router"; +import type {ChangedFile, RoutingResult} from "./router"; +import { + frontmatterBlock, + hasKey, + items, + list, + nested, + nestedPath, + scalar, + yamlLines, +} from "./frontmatter"; +import type { + EnableableReviewer, + ReReviewMode, + RiskRule, + RiskTier, +} from "./routing-config"; +import type {Lens} from "./finding-schema"; +import {renderReport} from "./check-consumer-config-report"; + +/* -------------------------------------------------------------------------- */ +/* Paths the install contract fixes */ +/* -------------------------------------------------------------------------- */ + +/** Where every consumer config file lives. */ +export const CONSUMER_CONFIG_DIR = ".github/aw/review"; + +/** + * The compile-time frontmatter import (`imports:` in `review.md`). A missing one + * fails `gh aw compile`; a *wrong* one (no `add-reviewer`, empty allowlist) + * compiles fine and never requests a reviewer. + */ +export const CONFIG_IMPORT_PATH = `${CONSUMER_CONFIG_DIR}/config.md`; + +/** + * The required `{{#runtime-import}}` bodies. They resolve when the workflow + * *runs*, so a missing one is a red run on someone's PR, not a compile error. + */ +export const REQUIRED_RUNTIME_IMPORTS = [ + `${CONSUMER_CONFIG_DIR}/risk-classification.md`, + `${CONSUMER_CONFIG_DIR}/ci-tooling.md`, + `${CONSUMER_CONFIG_DIR}/skills.md`, +] as const; + +/** Default location `gh aw add Khan/actions/workflows/review/review.md` writes. */ +export const INSTALLED_WORKFLOW_PATH = ".github/workflows/review.md"; + +/** Compiled output of the installed workflow (`gh aw compile`). */ +export const INSTALLED_LOCK_PATH = ".github/workflows/review.lock.yml"; + +/** + * gh-aw's scheduled housekeeping workflow (gh-aw v0.83+). Generated like a lock + * file but NOT named `*.lock.yml`, so the documented marker misses it. + */ +export const MAINTENANCE_WORKFLOW_PATH = + ".github/workflows/agentics-maintenance.yml"; + +const GITATTRIBUTES_PATH = ".gitattributes"; + +/** + * Copilot Agent scaffolding `gh aw` writes on first init. Unused at Khan, and it + * reads as part of the reviewer install; both known consumers removed it. + */ +export const COPILOT_SCAFFOLDING = [ + ".github/mcp.json", + ".github/agents", + ".github/skills", + ".github/workflows/copilot-setup-steps.yml", +] as const; + +/** + * The `max-ai-credits` the shared workflow ships. Both known consumers raise it: + * `tier=high` runs have died at ~1001-1024 metered credits *after* computing a + * verdict but before posting it. + * + * FALLBACK ONLY: the CLI reads the live value from this checkout's own + * `review.md` and passes it as `options.shippedMaxAiCredits`; this constant is + * used when that read fails, so a release that raises the shipped ceiling does + * not silently strand the check on a stale number. + */ +export const SHIPPED_MAX_AI_CREDITS = 1000; + +/* -------------------------------------------------------------------------- */ +/* Report shape */ +/* -------------------------------------------------------------------------- */ + +export type IssueSeverity = "error" | "warning"; + +/** + * One finding. `code` is a fixed machine token; `message` states the defect and + * `fix` the action that clears it. + */ +export type ConfigIssue = { + severity: IssueSeverity; + code: string; + message: string; + fix?: string; +}; + +/** What the installed `review.md` frontmatter says. */ +export type InstalledWorkflow = { + present: boolean; + lockPresent: boolean; + /** The `source:` field gh-aw records, verbatim (undefined when absent). */ + source?: string; + /** The `@` half of `source:`, when it carries one. */ + pinnedRef?: string; + importsConfig: boolean; + /** True when the main workflow also defines `add-reviewer` (overrides the import). */ + definesAddReviewer: boolean; + maxAiCredits?: number; + /** The `env.REVIEW_MAX_AI_CREDITS` mirror `resolveCreditCap` reads. */ + creditMirror?: number; + /** True when the `observability:` block is live (not commented out). */ + observabilityActive: boolean; +}; + +/** What `config.md`'s frontmatter says. */ +export type ReviewerRouting = { + present: boolean; + definesAddReviewer: boolean; + /** + * Whether `allowed-team-reviewers` appears at all, as distinct from + * appearing and yielding no teams. Absent is a deliberate no-requests + * install; present-but-empty is a defect. + */ + allowlistKeyPresent: boolean; + allowedTeamReviewers: string[]; + /** True when `add-reviewer` names a `github-token:` (org teams need one). */ + hasGithubToken: boolean; +}; + +/** Tier resolution over a supplied file list, from {@link route}. */ +export type TierPreview = { + fileCount: number; + generated: number; + counts: Record; + /** Up to {@link SAMPLE_LIMIT} paths per tier, in input order. */ + samples: Record; + /** Source files no `tier=` rule matched: the router's default tier applies. */ + unmatched: string[]; + /** + * `ROUTING` patterns that match no tracked file at all. A dead pattern parses + * clean and routes nothing, so a typo is invisible without this. + */ + deadPatterns: string[]; + lensesToSpawn: Lens[]; + highestTier: RiskTier; +}; + +/** One path's routing decision plus the rules that produced it. */ +export type PathExplanation = { + path: string; + generated: boolean; + tier: RiskTier; + tierPending: boolean; + /** Every `tier=` rule matching this path, in file order (last one wins). */ + matchingTierRules: RiskRule[]; + lenses: Lens[]; +}; + +export type ConsumerConfigReport = { + repoRoot: string; + issues: ConfigIssue[]; + routing: { + present: boolean; + enabledReviewers: EnableableReviewer[]; + reReviewMode: ReReviewMode; + tierRules: number; + lensRules: number; + }; + installedWorkflow: InstalledWorkflow; + reviewerRouting: ReviewerRouting; + /** Tier of each config file that steers the reviewer (self-tamper check). */ + configFileTiers: Record; + tierPreview?: TierPreview; + explanation?: PathExplanation; +}; + +const SAMPLE_LIMIT = 5; + +/* -------------------------------------------------------------------------- */ +/* Checks */ +/* -------------------------------------------------------------------------- */ + +/** The filesystem surface this module needs (injected, so tests stay in memory). */ +export type ConsumerConfigFs = { + readFileSync: (p: string, enc: "utf8") => string; + existsSync: (p: string) => boolean; + readdirSync: (p: string) => string[]; +}; + +export type CheckOptions = { + /** Consumer repo root. */ + repoRoot?: string; + /** Tracked paths to preview tier routing over (e.g. `git ls-files`). */ + files?: readonly string[]; + /** One path to explain in full. */ + explainPath?: string; + /** Installed workflow path, when the repo renamed it. */ + workflowPath?: string; + /** + * The `review` package version this checker was run from. When set, a + * consumer pinned to a different `review-v*` tag is warned: the semantics + * validated here are this version's, not the one their PRs will run. + */ + checkerVersion?: string; + /** + * The `max-ai-credits` the shared workflow ships, read from the checker's + * own checkout of `workflows/review/review.md` (the CLI does this). Falls + * back to {@link SHIPPED_MAX_AI_CREDITS} so a release that raises the + * shipped ceiling cannot leave the check comparing against a stale + * constant. + */ + shippedMaxAiCredits?: number; +}; + +const emptyTierRecord = (make: () => T): Record => ({ + trivial: make(), + low: make(), + medium: make(), + high: make(), +}); + +const readInstalledWorkflow = ( + fs: ConsumerConfigFs, + path: string, +): InstalledWorkflow => { + if (!fs.existsSync(path)) { + return { + present: false, + lockPresent: false, + importsConfig: false, + definesAddReviewer: false, + observabilityActive: false, + }; + } + const content = fs.readFileSync(path, "utf8"); + const block = frontmatterBlock(content); + const lines = block === undefined ? [] : yamlLines(block); + + const source = scalar(lines, "source"); + const atIndex = source?.lastIndexOf("@") ?? -1; + const pinnedRef = + source !== undefined && atIndex > 0 + ? source.slice(atIndex + 1) + : undefined; + + const imports = nested(lines, "imports") ?? []; + const credits = scalar(lines, "max-ai-credits"); + // The env mirror the run budget actually reads (resolveCreditCap in + // credit-cap.ts): the frontmatter cap is enforced proxy-side and invisible + // to the agent process, so the two must agree or the router plans at the + // stale ceiling. + const mirror = scalar(nested(lines, "env") ?? [], "REVIEW_MAX_AI_CREDITS"); + + return { + present: true, + lockPresent: false, // filled by the caller (path may be renamed) + source, + pinnedRef, + // Compared through `items()` so a quoted entry (`- ".github/…"`, valid + // YAML) is not read as a missing import, which would be a false error. + importsConfig: items(imports).includes(CONFIG_IMPORT_PATH), + definesAddReviewer: + nestedPath(lines, ["safe-outputs", "add-reviewer"]) !== undefined, + maxAiCredits: credits === undefined ? undefined : Number(credits), + creditMirror: mirror === undefined ? undefined : Number(mirror), + observabilityActive: hasKey(lines, "observability"), + }; +}; + +const readReviewerRouting = ( + fs: ConsumerConfigFs, + path: string, +): ReviewerRouting => { + if (!fs.existsSync(path)) { + return { + present: false, + definesAddReviewer: false, + allowedTeamReviewers: [], + hasGithubToken: false, + }; + } + const block = frontmatterBlock(fs.readFileSync(path, "utf8")); + const lines = block === undefined ? [] : yamlLines(block); + const addReviewer = nestedPath(lines, ["safe-outputs", "add-reviewer"]); + // `list()` reads both block and flow style, and returns undefined only when + // the key is absent. That distinction is the whole point here: an absent key + // is a deliberate "this repo requests no reviewers", while a key that yields + // nothing is a mistake or a spelling this reader cannot read. Collapsing the + // two would let a broken allowlist report as a deliberate choice. + const teams = + addReviewer === undefined + ? undefined + : list(addReviewer, "allowed-team-reviewers"); + return { + present: true, + definesAddReviewer: addReviewer !== undefined, + allowlistKeyPresent: teams !== undefined, + allowedTeamReviewers: teams ?? [], + hasGithubToken: + addReviewer !== undefined && hasKey(addReviewer, "github-token"), + }; +}; + +/** + * Run every check. Pure apart from the injected `fs`: the caller supplies the + * file list (so the checker never shells out to git) and the report is data, + * rendered separately. + */ +export const checkConsumerConfig = ( + fs: ConsumerConfigFs, + options: CheckOptions = {}, +): ConsumerConfigReport => { + const repoRoot = options.repoRoot ?? "."; + const at = (p: string): string => + repoRoot === "." ? p : `${repoRoot}/${p}`; + const issues: ConfigIssue[] = []; + const error = (code: string, message: string, fix?: string): void => { + issues.push({severity: "error", code, message, fix}); + }; + const warn = (code: string, message: string, fix?: string): void => { + issues.push({severity: "warning", code, message, fix}); + }; + + /* --- required config files --------------------------------------------- */ + + const requireFile = (path: string, why: string): string | undefined => { + if (!fs.existsSync(at(path))) { + error( + "missing-required-config", + `${path} is missing. ${why}`, + `Create ${path}.`, + ); + return undefined; + } + const content = fs.readFileSync(at(path), "utf8"); + if (content.trim() === "") { + error( + "empty-required-config", + `${path} is empty. ${why}`, + `Write ${path}, or the reviewer runs with no repo-specific guidance.`, + ); + return undefined; + } + return content; + }; + + requireFile( + CONFIG_IMPORT_PATH, + "It is a compile-time `imports:` target, so `gh aw compile` fails without it.", + ); + for (const path of REQUIRED_RUNTIME_IMPORTS) { + const content = requireFile( + path, + "It is a required `{{#runtime-import}}`, so the next PR's review fails at run time without it.", + ); + // gh-aw rejects Actions template expressions inside imported bodies. + // `config.md` is exempt: it is a frontmatter import and legitimately + // carries `${{ secrets.* }}` for the bot token. + if (content !== undefined && content.includes("${{")) { + error( + "template-expression-in-import", + `${path} contains a \`\${{ }}\` expression; gh-aw rejects those inside runtime imports.`, + "Remove the expression, or move the value into the installed review.md.", + ); + } + } + + /* --- ROUTING ----------------------------------------------------------- */ + + const routingPresent = fs.existsSync(at(ROUTING_CONFIG_PATH)); + const routingConfig = routingPresent + ? parseRoutingConfig(fs.readFileSync(at(ROUTING_CONFIG_PATH), "utf8")) + : undefined; + + if (!routingPresent) { + warn( + "routing-missing", + `${ROUTING_CONFIG_PATH} is absent: no specialist lens spawns, the run budget is floored, and every review carries a missing-config note.`, + "Write ROUTING with this repo's tier rules and `enable` roster.", + ); + } + for (const warning of routingConfig?.warnings ?? []) { + warn( + "routing-parse-warning", + warning, + "Fix the line; a skipped rule routes fewer reviewers, silently.", + ); + } + if ( + routingConfig !== undefined && + routingConfig.enabledReviewers.length === 0 + ) { + warn( + "no-enabled-reviewers", + "ROUTING has no `enable` line: only the default roster runs (pattern-triage, correctness-reviewer, skill-auditor, thread-reconciler, claim-validator).", + "Both known consumers enable holistic,completeness,test-adequacy,first-principles,conventions.", + ); + } + if ( + routingConfig !== undefined && + routingConfig.reReviewMode === DEFAULT_RE_REVIEW_MODE + ) { + warn( + "re-review-full", + "Re-review mode is `full` (the default, and the most expensive setting; the parser cannot tell an explicit `re-review full` from an absent line): every push re-runs the whole roster over the whole diff.", + "`re-review scoped` is the recommended first step down; see the README's re-review table.", + ); + } + + const lensRules = routingConfig?.lensRules ?? []; + const riskRules = routingConfig?.riskRules ?? []; + + /* --- lens payloads ----------------------------------------------------- */ + + const lensesDir = at(LENS_PAYLOAD_DIR); + let payloadFiles: string[] = []; + if (fs.existsSync(lensesDir)) { + try { + payloadFiles = fs.readdirSync(lensesDir); + } catch { + warn( + "lens-dir-unreadable", + `${LENS_PAYLOAD_DIR} exists but is not a readable directory; no payload is imported.`, + ); + } + } + for (const warning of lensPayloadWarnings( + payloadFiles, + lensRules, + fs.existsSync(at(CORRECTNESS_ALIAS_PATH)), + SPECIALIST_LENSES, + )) { + warn("lens-payload-warning", warning); + } + for (const path of payloadFiles) { + const full = `${LENS_PAYLOAD_DIR}/${path}`; + if ( + path.endsWith(".md") && + fs.readFileSync(at(full), "utf8").includes("${{") + ) { + error( + "template-expression-in-import", + `${full} contains a \`\${{ }}\` expression; gh-aw rejects those inside runtime imports.`, + ); + } + } + + /* --- installed workflow ------------------------------------------------ */ + + const workflowPath = options.workflowPath ?? INSTALLED_WORKFLOW_PATH; + const lockPath = + workflowPath === INSTALLED_WORKFLOW_PATH + ? INSTALLED_LOCK_PATH + : workflowPath.replace(/\.md$/, ".lock.yml"); + const installed = readInstalledWorkflow(fs, at(workflowPath)); + installed.lockPresent = fs.existsSync(at(lockPath)); + + if (!installed.present) { + error( + "workflow-not-installed", + `${workflowPath} is missing: the reviewer is not installed.`, + "Run `gh aw add Khan/actions/workflows/review/review.md@review-v..`.", + ); + } else { + if (!installed.lockPresent) { + error( + "lock-missing", + `${lockPath} is missing, so nothing runs: the compiled lock is the workflow GitHub executes.`, + "Run `gh aw compile` and commit the lock.", + ); + } + if (!installed.importsConfig) { + error( + "workflow-missing-config-import", + `${workflowPath} does not import ${CONFIG_IMPORT_PATH}, so this repo's add-reviewer allowlist never reaches the workflow.`, + `Restore \`imports:\` with \`- ${CONFIG_IMPORT_PATH}\`.`, + ); + } + if (installed.definesAddReviewer) { + error( + "workflow-defines-add-reviewer", + `${workflowPath} defines \`add-reviewer\` itself; gh-aw lets the main workflow override an imported safe output of the same type, so the allowlist in ${CONFIG_IMPORT_PATH} is discarded.`, + `Delete the \`add-reviewer\` block from ${workflowPath}; it belongs only in config.md.`, + ); + } + if (installed.source === undefined) { + warn( + "source-missing", + `${workflowPath} carries no \`source:\` field, so \`gh aw update\` cannot find its upstream.`, + ); + } else if (installed.pinnedRef === undefined) { + warn( + "source-unpinned", + `${workflowPath} tracks \`${installed.source}\` with no \`@\`: it follows the default branch instead of a released tag.`, + "Re-add at `@review-v..`.", + ); + } else if ( + options.checkerVersion !== undefined && + installed.pinnedRef !== `review-v${options.checkerVersion}` + ) { + warn( + "source-ref-mismatch", + `${workflowPath} pins \`${installed.pinnedRef}\`, but this checker ran from review v${options.checkerVersion}: the semantics validated here may not be the ones this repo's reviews run.`, + `Re-run the checker from a checkout of \`${installed.pinnedRef}\`.`, + ); + } + if (installed.observabilityActive) { + warn( + "observability-active", + "The `observability:` block is live, which hard-requires GH_AW_OTEL_SENTRY_ENDPOINT and GH_AW_OTEL_SENTRY_AUTHORIZATION: a missing secret kills the agent job at startup rather than skipping trace export.", + "Confirm both secrets exist, or comment the block out as a local edit and recompile.", + ); + } + const shippedCredits = + options.shippedMaxAiCredits ?? SHIPPED_MAX_AI_CREDITS; + if ( + installed.maxAiCredits !== undefined && + installed.maxAiCredits <= shippedCredits + ) { + warn( + "max-ai-credits-default", + `max-ai-credits is ${installed.maxAiCredits}: runs that route to tier=high have died at ~1001-1024 metered credits after computing a verdict but before posting it.`, + "Both known consumers raise it to 2500 (a ceiling, not a spend) in review.md and its REVIEW_MAX_AI_CREDITS mirror.", + ); + } + // The frontmatter cap is enforced by the firewall api-proxy; the run + // budget reads only the env mirror (resolveCreditCap). A raised cap + // with a stale or missing mirror still PLANS at the old ceiling: the + // exact late-and-quiet failure this checker exists to catch. + if ( + installed.maxAiCredits !== undefined && + installed.creditMirror !== installed.maxAiCredits + ) { + warn( + "max-ai-credits-mirror-stale", + installed.creditMirror === undefined + ? `max-ai-credits is ${installed.maxAiCredits} but the frontmatter sets no REVIEW_MAX_AI_CREDITS env mirror; the router reads only the mirror, so runs plan against the shipped default instead of the raised cap.` + : `max-ai-credits is ${installed.maxAiCredits} but its REVIEW_MAX_AI_CREDITS env mirror says ${installed.creditMirror}; the router reads only the mirror, so the two must agree.`, + `Set env.REVIEW_MAX_AI_CREDITS to "${installed.maxAiCredits}" in the same frontmatter (KEEP THE TWO VALUES IN SYNC, per the shipped review.md).`, + ); + } + } + + /* --- reviewer routing (config.md) -------------------------------------- */ + + // Whether a reviewer request can happen here at all. The router reads ownership + // from `.github/REVIEWERS` and nowhere else, so without it Step 8 has no owners + // and no ranked fallback and requests nobody, whatever the allowlist says. + const ownershipMapPresent = fs.existsSync(at(REVIEWERS_PATH)); + + const reviewerRouting = readReviewerRouting(fs, at(CONFIG_IMPORT_PATH)); + if (reviewerRouting.present) { + if (!reviewerRouting.definesAddReviewer) { + error( + "config-missing-add-reviewer", + `${CONFIG_IMPORT_PATH} defines no \`safe-outputs.add-reviewer\`, which is the one thing it exists to carry.`, + ); + } else if (reviewerRouting.allowedTeamReviewers.length === 0) { + // Three cases, and conflating any two of them either fails a working + // install or blesses a broken one. + // + // The key is PRESENT but yields no teams: always an error, whatever + // `.github/REVIEWERS` says. Someone wrote the field and got nothing + // out of it — an empty list, or a spelling this reader cannot read. + // Reporting that as a deliberate choice would be the worst outcome, + // because it reads as an all-clear over a dropped allowlist. + // + // The key is ABSENT: now `.github/REVIEWERS` decides. It is the + // router's only source of ownership, so without it Step 8 computes + // nobody to request and the omission is an accurate statement that + // this repo makes no reviewer requests. With it, ownership exists and + // nothing is allowed through, so the requests are computed and then + // silently dropped. + if (reviewerRouting.allowlistKeyPresent) { + error( + "config-empty-team-allowlist", + `${CONFIG_IMPORT_PATH} has an \`allowed-team-reviewers\` key that yields no teams, so every reviewer request is dropped by the safe output.`, + "Name the owning team(s) as `- team` lines or `[team]`, or delete the key entirely if this repo should request no reviewers.", + ); + } else if (ownershipMapPresent) { + error( + "config-empty-team-allowlist", + `${CONFIG_IMPORT_PATH} names no \`allowed-team-reviewers\`, but ${REVIEWERS_PATH} gives the router team ownership, so Step 8 computes teams to request and the safe output drops every one.`, + `Name the owning team(s), or delete ${REVIEWERS_PATH} if this repo should not request reviewers at all.`, + ); + } else { + warn( + "reviewer-requests-inert", + `${CONFIG_IMPORT_PATH} names no \`allowed-team-reviewers\` and there is no ${REVIEWERS_PATH}, so this repo requests no reviewers. That is a valid configuration; it is reported because it is invisible on the PR.`, + `If reviewer requests are wanted, add ${REVIEWERS_PATH} and name the owning team(s) here — either alone is inert.`, + ); + } + } + // Only matters for a request that can actually be made, so a deliberate + // no-requests install reports one finding rather than two. + if ( + reviewerRouting.definesAddReviewer && + !reviewerRouting.hasGithubToken && + (ownershipMapPresent || + reviewerRouting.allowedTeamReviewers.length > 0) + ) { + warn( + "config-no-bot-token", + `${CONFIG_IMPORT_PATH}'s add-reviewer names no \`github-token:\`; the default GITHUB_TOKEN cannot request an organization team as a reviewer.`, + "Set `github-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }}`.", + ); + } + } + + /* --- .gitattributes ---------------------------------------------------- */ + + const generatedRules = fs.existsSync(at(GITATTRIBUTES_PATH)) + ? parseGitattributesGenerated( + fs.readFileSync(at(GITATTRIBUTES_PATH), "utf8"), + ) + : []; + // Guarded on the lock actually existing: a repo with no reviewer installed + // (or one the errors above just told has no lock) should not additionally + // be told to mark a nonexistent file as generated — the report reads + // cause-then-effect, and under --strict the extra warning would flip the + // exit code. + if (installed.lockPresent && !isGenerated(lockPath, generatedRules)) { + warn( + "lock-not-marked-generated", + `${lockPath} is not marked \`linguist-generated\` in ${GITATTRIBUTES_PATH}, so the reviewer line-reviews its own compiled output.`, + `Add \`.github/workflows/*.lock.yml linguist-generated=true merge=ours\`.`, + ); + } + // The generated workflow the usual `*.lock.yml` marker misses: `gh aw compile` + // writes it unconditionally (deleting it does not stick) under a name that does + // not match, so a repo that followed the documented marker still gets ~600 lines + // of compiler output line-reviewed. Installs predating gh-aw v0.83 lack the file. + if ( + fs.existsSync(at(MAINTENANCE_WORKFLOW_PATH)) && + !isGenerated(MAINTENANCE_WORKFLOW_PATH, generatedRules) + ) { + warn( + "maintenance-workflow-not-marked-generated", + `${MAINTENANCE_WORKFLOW_PATH} is \`gh aw compile\` output but is not marked \`linguist-generated\` in ${GITATTRIBUTES_PATH}, so the reviewer line-reviews it. The \`*.lock.yml\` marker does not cover it.`, + `Add \`${MAINTENANCE_WORKFLOW_PATH} linguist-generated=true merge=ours\`.`, + ); + } + + /* --- leftover scaffolding ---------------------------------------------- */ + + const scaffolding = COPILOT_SCAFFOLDING.filter((path) => + fs.existsSync(at(path)), + ); + if (scaffolding.length > 0) { + warn( + "copilot-scaffolding-present", + `\`gh aw\` init scaffolding is still present: ${scaffolding.join( + ", ", + )}.`, + "Both known consumers removed it; keep it only if this repo uses Copilot Agent.", + ); + } + + /* --- routing over the reviewer's own config ---------------------------- */ + + const selfPaths = [ + ROUTING_CONFIG_PATH, + CONFIG_IMPORT_PATH, + ...REQUIRED_RUNTIME_IMPORTS, + workflowPath, + ]; + const selfRouting = route( + {files: selfPaths.map((path) => ({path, status: "modified" as const}))}, + {generatedRules, lensRules, riskRules}, + ); + const configFileTiers: Record = {}; + for (const path of selfPaths) { + configFileTiers[path] = selfRouting.perFileTier[path]; + } + const underTiered = selfPaths.filter( + (path) => configFileTiers[path] !== "high", + ); + if (routingPresent && underTiered.length > 0) { + warn( + "reviewer-config-not-high", + `These files steer the reviewer but do not route to tier=high: ${underTiered + .map((path) => `${path} (${configFileTiers[path]})`) + .join( + ", ", + )}. A PR editing them would be reviewed at a lower budget than the reviewer it rewrites.`, + "Add `.github/aw/review/** tier=high` and `.github/workflows/*.md tier=high` to ROUTING.", + ); + } + + /* --- tier preview ------------------------------------------------------ */ + + let tierPreview: TierPreview | undefined; + if (options.files !== undefined && options.files.length > 0) { + const files: ChangedFile[] = options.files.map((path) => ({ + path, + status: "modified", + })); + const result: RoutingResult = route( + {files}, + {generatedRules, lensRules, riskRules}, + ); + const counts = emptyTierRecord(() => 0); + const samples = emptyTierRecord(() => []); + let generated = 0; + const unmatched: string[] = []; + for (const file of result.perFile) { + if (file.classification === "generated") { + generated++; + continue; + } + counts[file.tier]++; + if (samples[file.tier].length < SAMPLE_LIMIT) { + samples[file.tier].push(file.path); + } + if ( + !riskRules.some((rule) => matchesGlob(file.path, rule.pattern)) + ) { + unmatched.push(file.path); + } + } + // A pattern nothing matches is the ROUTING analogue of an inert lens + // payload: it parses, it routes nothing, and only the file list can + // reveal it. Deliberately dead patterns exist (a rule written ahead of + // the directory it guards), hence a warning rather than an error. + const deadPatterns = [ + ...new Set( + [...riskRules, ...lensRules] + .map((rule) => rule.pattern) + .filter( + (pattern) => + !options.files?.some((path) => + matchesGlob(path, pattern), + ), + ), + ), + ]; + + tierPreview = { + fileCount: files.length, + generated, + counts, + samples, + unmatched, + deadPatterns, + lensesToSpawn: result.lensesToSpawn, + highestTier: result.runBudget.tier, + }; + if (deadPatterns.length > 0) { + warn( + "routing-pattern-matches-nothing", + `${deadPatterns.length} ROUTING pattern(s) match none of the ${ + files.length + } supplied paths, so they route nothing: ${deadPatterns.join( + ", ", + )}`, + "Check for a typo or wrong glob dialect (a pattern with no `/` matches the basename in any directory; prefix `/` to anchor to the repo root), or confirm the rule guards a path this repo does not have yet.", + ); + } + if (unmatched.length > 0) { + warn( + "files-without-tier-rule", + `${unmatched.length} of ${ + files.length + } tracked files match no \`tier=\` rule and fall to the router's default tier (low): ${unmatched + .slice(0, SAMPLE_LIMIT) + .join(", ")}${ + unmatched.length > SAMPLE_LIMIT ? ", …" : "" + }`, + "Confirm `low` is the tier you want for these, or add rules.", + ); + } + } + + /* --- single-path explanation ------------------------------------------ */ + + let explanation: PathExplanation | undefined; + if (options.explainPath !== undefined) { + const path = options.explainPath; + const result = route( + {files: [{path, status: "modified"}]}, + {generatedRules, lensRules, riskRules}, + ); + const decision = result.perFile[0]; + explanation = { + path, + generated: decision.classification === "generated", + tier: decision.tier, + tierPending: decision.tierPending, + matchingTierRules: riskRules.filter((rule) => + matchesGlob(path, rule.pattern), + ), + lenses: decision.lenses, + }; + } + + return { + repoRoot, + issues, + routing: { + present: routingPresent, + enabledReviewers: routingConfig?.enabledReviewers ?? [], + reReviewMode: routingConfig?.reReviewMode ?? DEFAULT_RE_REVIEW_MODE, + tierRules: riskRules.length, + lensRules: lensRules.length, + }, + installedWorkflow: installed, + reviewerRouting, + configFileTiers, + tierPreview, + explanation, + }; +}; + +/* -------------------------------------------------------------------------- */ +/* Rendering */ +/* -------------------------------------------------------------------------- */ + +// The text rendering lives in its own module (split by concern, and to keep this +// file inside its max-lines budget). Imported for the CLI below and re-exported so +// existing importers still treat this module as the checker's single entry point. +export {renderReport}; + +/* -------------------------------------------------------------------------- */ +/* CLI */ +/* -------------------------------------------------------------------------- */ + +/** Parse `--flag value` arguments. Unknown flags are an error, not ignored. */ +export const parseArgs = ( + argv: readonly string[], +): { + repoRoot?: string; + filesFrom?: string; + explainPath?: string; + workflowPath?: string; + json: boolean; + strict: boolean; +} => { + const out = {json: false, strict: false} as { + repoRoot?: string; + filesFrom?: string; + explainPath?: string; + workflowPath?: string; + json: boolean; + strict: boolean; + }; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + switch (arg) { + case "--repo": + out.repoRoot = argv[++i]; + break; + case "--files-from": + out.filesFrom = argv[++i]; + break; + case "--explain": + out.explainPath = argv[++i]; + break; + case "--workflow": + out.workflowPath = argv[++i]; + break; + case "--json": + out.json = true; + break; + case "--strict": + out.strict = true; + break; + default: + throw new Error(`unknown argument: ${arg}`); + } + } + return out; +}; + +const main = (): void => { + /* eslint-disable-next-line no-undef */ + const nodeFs = require("node:fs") as ConsumerConfigFs & { + readFileSync: (p: string | number, enc: "utf8") => string; + }; + const args = parseArgs(process.argv.slice(2)); + + const files = + args.filesFrom === undefined + ? undefined + : nodeFs + .readFileSync( + args.filesFrom === "-" ? 0 : args.filesFrom, + "utf8", + ) + .split("\n") + .map((line) => line.trim()) + .filter((line) => line !== ""); + + // The version this checker ships with, so a consumer pinned elsewhere is + // told the semantics validated here are not the ones its reviews run. + let checkerVersion: string | undefined; + try { + checkerVersion = JSON.parse( + nodeFs.readFileSync(`${__dirname}/../package.json`, "utf8"), + ).version; + } catch { + checkerVersion = undefined; + } + + // The ceiling the shared workflow ships, read from this checkout's own + // review.md so a release that raises it cannot strand the hardcoded + // fallback constant. + let shippedMaxAiCredits: number | undefined; + try { + const shipped = frontmatterBlock( + nodeFs.readFileSync(`${__dirname}/../review.md`, "utf8"), + ); + const raw = + shipped === undefined + ? undefined + : scalar(yamlLines(shipped), "max-ai-credits"); + const parsed = raw === undefined ? NaN : Number(raw); + shippedMaxAiCredits = Number.isFinite(parsed) ? parsed : undefined; + } catch { + shippedMaxAiCredits = undefined; + } + + const report = checkConsumerConfig(nodeFs, { + repoRoot: args.repoRoot, + files, + explainPath: args.explainPath, + workflowPath: args.workflowPath, + checkerVersion, + shippedMaxAiCredits, + }); + + process.stdout.write( + args.json + ? `${JSON.stringify(report, null, 2)}\n` + : renderReport(report), + ); + + const errors = report.issues.filter((issue) => issue.severity === "error"); + const warnings = report.issues.filter( + (issue) => issue.severity === "warning", + ); + if (errors.length > 0 || (args.strict && warnings.length > 0)) { + process.exitCode = 1; + } +}; + +// Run only when invoked directly, never on import (tests). +if (typeof require !== "undefined" && require.main === module) { + main(); +} diff --git a/workflows/review/lib/credit-cap.test.ts b/workflows/review/lib/credit-cap.test.ts index 6c247fa4..26d51488 100644 --- a/workflows/review/lib/credit-cap.test.ts +++ b/workflows/review/lib/credit-cap.test.ts @@ -18,7 +18,7 @@ import { */ /** The smallest router config the clamp path needs; the tier is explicit. */ -const minimalConfig: RouterConfig = {generatedPatterns: []}; +const minimalConfig: RouterConfig = {generatedRules: []}; /** In-memory fs seam, mirroring router.test.ts's helper. */ const fakeFs = (inputs: Record) => { diff --git a/workflows/review/lib/frontmatter.test.ts b/workflows/review/lib/frontmatter.test.ts new file mode 100644 index 00000000..d44fa979 --- /dev/null +++ b/workflows/review/lib/frontmatter.test.ts @@ -0,0 +1,245 @@ +import {describe, it, expect} from "vitest"; + +import { + frontmatterBlock, + hasKey, + items, + nested, + nestedPath, + scalar, + yamlLines, + list, + stripInlineComment, + unquote, +} from "./frontmatter.ts"; + +/** + * The frontmatter reader's contract. Two behaviours here are load-bearing for + * the consumer-config checker rather than incidental: a commented-out block must + * read as absent (that is how a consumer disables `observability:`), and nesting + * must be indent-scoped, so an `add-reviewer` under `safe-outputs` is only found + * at that path and not by a substring hit anywhere in the file. + */ + +const block = (content: string) => yamlLines(frontmatterBlock(content) ?? ""); + +const WORKFLOW = `--- +description: > + Reviews PR code changes. +imports: + - .github/aw/review/config.md +permissions: + contents: read +safe-outputs: + create-pull-request-review-comment: + max: 20 + add-comment: + target: "triggering" +# observability is disabled here because this repo has no OTEL secrets. +# observability: +# otlp: +# exporters: +# - url: \${{ secrets.GH_AW_OTEL_SENTRY_ENDPOINT }} +max-ai-credits: 2500 +source: Khan/actions/workflows/review/review.md@review-v1.11.0 +--- + +# Prompt body + +safe-outputs: this line is prose, not frontmatter. +`; + +describe("frontmatterBlock", () => { + it("returns the lines between the leading and closing ---", () => { + expect(frontmatterBlock("---\na: 1\n---\nbody\n")).toBe("a: 1"); + }); + + it("is undefined without a leading --- or a closing one", () => { + expect(frontmatterBlock("# just markdown\n")).toBeUndefined(); + expect(frontmatterBlock("---\na: 1\nno terminator\n")).toBeUndefined(); + }); +}); + +describe("yamlLines", () => { + it("reads keys, scalars, items and indentation, dropping comments", () => { + expect(yamlLines("a: 1\n# note\n - x\n\nb:\n")).toEqual([ + {indent: 0, key: "a", value: "1"}, + {indent: 2, item: "x"}, + {indent: 0, key: "b", value: ""}, + ]); + }); +}); + +describe("reading a gh-aw workflow's frontmatter", () => { + const lines = block(WORKFLOW); + + it("reads top-level scalars", () => { + expect(scalar(lines, "source")).toBe( + "Khan/actions/workflows/review/review.md@review-v1.11.0", + ); + expect(scalar(lines, "max-ai-credits")).toBe("2500"); + }); + + it("reads a key with no inline value as undefined but present", () => { + expect(scalar(lines, "safe-outputs")).toBeUndefined(); + expect(hasKey(lines, "safe-outputs")).toBe(true); + }); + + it("reads list items under a key", () => { + expect(items(nested(lines, "imports") ?? [])).toEqual([ + ".github/aw/review/config.md", + ]); + }); + + // YAML allows a block sequence at the parent key's own indentation; a + // hand-restyled but valid install must not read as import-less (a false + // error, the checker's worst failure mode). + it("reads same-indent list items directly after the key", () => { + const restyled = block( + "---\nimports:\n- .github/aw/review/config.md\n- other.md\nsource: x@y\n---\n", + ); + expect(items(nested(restyled, "imports") ?? [])).toEqual([ + ".github/aw/review/config.md", + "other.md", + ]); + // The same-indent `source:` key still ends the block. + expect(scalar(restyled, "source")).toBe("x@y"); + }); + + it("treats a commented-out block as absent", () => { + expect(hasKey(lines, "observability")).toBe(false); + }); + + it("does not see frontmatter keys spelled in the markdown body", () => { + // `safe-outputs:` also appears as prose below the closing ---; the + // block boundary is what keeps it out. + expect(nested(lines, "safe-outputs")?.length).toBe(4); + }); + + it("scopes nesting to the given path", () => { + expect( + nestedPath(lines, ["safe-outputs", "add-comment"]), + ).not.toBeUndefined(); + expect( + nestedPath(lines, ["safe-outputs", "add-reviewer"]), + ).toBeUndefined(); + // `contents` is nested under permissions, not at the top level. + expect(hasKey(lines, "contents")).toBe(false); + expect(hasKey(nested(lines, "permissions") ?? [], "contents")).toBe( + true, + ); + }); + + it("returns undefined for a missing hop rather than throwing", () => { + expect(nestedPath(lines, ["nope", "deeper"])).toBeUndefined(); + expect(nested([], "anything")).toBeUndefined(); + expect(scalar([], "anything")).toBeUndefined(); + expect(hasKey([], "anything")).toBe(false); + }); +}); + +describe("items", () => { + it("strips surrounding quotes and skips non-item lines", () => { + expect( + items( + yamlLines(`- "kore"\n- 'github-actions'\nkey: value\n- bare`), + ), + ).toEqual(["kore", "github-actions", "bare"]); + }); +}); + +/** + * Value normalisation. Each case below is a valid YAML spelling that previously + * read as absent or unparseable, which for a checker whose contract is "errors + * must be zero" meant a false error or a silently-suppressed check on a working + * install. + */ +describe("stripInlineComment", () => { + it("drops a trailing comment", () => { + expect(stripInlineComment("2500 # LOCAL OVERRIDE")).toBe("2500"); + expect(stripInlineComment("2500\t# note")).toBe("2500"); + }); + + it("keeps a # that is inside quotes or has no leading space", () => { + expect(stripInlineComment('"a # b"')).toBe('"a # b"'); + expect(stripInlineComment("https://x/y#frag")).toBe("https://x/y#frag"); + }); + + it("treats a value that is only a comment as empty", () => { + expect(stripInlineComment("# just a note")).toBe(""); + }); +}); + +describe("unquote", () => { + it("strips one layer of matching quotes and nothing else", () => { + expect(unquote('"1000"')).toBe("1000"); + expect(unquote("'kore'")).toBe("kore"); + expect(unquote("plain")).toBe("plain"); + expect(unquote('"mismatched')).toBe('"mismatched'); + }); +}); + +describe("inline comments through yamlLines", () => { + // The onboarding skill tells authors to label every local edit with a + // comment, so this is the checker's own prescribed flow. + const block = [ + "max-ai-credits: 2500 # KHAN/REPO LOCAL OVERRIDE", + "source: Khan/actions/x.md@review-v1.11.0 # pinned", + "imports:", + " - .github/aw/review/config.md # the consumer import", + ].join("\n"); + const lines = yamlLines(block); + + it("leaves a labelled numeric value parseable", () => { + expect(Number(scalar(lines, "max-ai-credits"))).toBe(2500); + }); + + it("leaves a labelled source ref intact", () => { + expect(scalar(lines, "source")).toBe( + "Khan/actions/x.md@review-v1.11.0", + ); + }); + + it("leaves a labelled list item matchable", () => { + expect(items(nested(lines, "imports") ?? [])).toEqual([ + ".github/aw/review/config.md", + ]); + }); +}); + +describe("scalar unquoting", () => { + it("makes a quoted number parse rather than yield NaN", () => { + const lines = yamlLines('max-ai-credits: "1000"'); + expect(Number(scalar(lines, "max-ai-credits"))).toBe(1000); + }); +}); + +describe("list", () => { + const blockStyle = yamlLines( + ["teams:", " - kore", ' - "web"'].join("\n"), + ); + const flowStyle = yamlLines('teams: [kore, "web"]'); + + it("reads block style", () => { + expect(list(blockStyle, "teams")).toEqual(["kore", "web"]); + }); + + it("reads flow style, which the shipped review.md itself uses", () => { + expect(list(flowStyle, "teams")).toEqual(["kore", "web"]); + }); + + it("reads an empty flow list as empty", () => { + expect(list(yamlLines("teams: []"), "teams")).toEqual([]); + }); + + // The distinction the caller needs: absent means "deliberately none", + // present-but-empty means "someone wrote this and got nothing". + it("returns undefined only when the key is absent", () => { + expect(list(yamlLines("other: 1"), "teams")).toBeUndefined(); + expect(list(yamlLines("teams:"), "teams")).toEqual([]); + }); + + it("reports a scalar where a list belongs as empty, not absent", () => { + expect(list(yamlLines("teams: kore"), "teams")).toEqual([]); + }); +}); diff --git a/workflows/review/lib/frontmatter.ts b/workflows/review/lib/frontmatter.ts new file mode 100644 index 00000000..af28399b --- /dev/null +++ b/workflows/review/lib/frontmatter.ts @@ -0,0 +1,242 @@ +/** + * A minimal structural reader for gh-aw markdown frontmatter: indentation, + * `key:` and `- item`, nothing else. + * + * Deliberately not a YAML parser. `lib/` carries no YAML dependency (its + * `package.json` pins exactly what the thumbs sweep needs), and every question + * asked of frontmatter here (is this key present, what scalar does it hold, + * what list items sit under it) is answerable from the shape of gh-aw + * frontmatter, which is plain block-style mappings. + * + * One behaviour is load-bearing rather than incidental: comment lines are + * dropped, so a commented-out block reads as **absent**. That is exactly the + * intended meaning, because commenting a block out is how a consumer disables + * one (the `observability:` local edit the reviewer's README prescribes for a + * repo without the `GH_AW_OTEL_SENTRY_*` secrets). + * + * Values are normalised on the way out — inline comments stripped, surrounding + * quotes removed, flow-style lists (`[a, b]`) read as lists — because the + * checker's contract is "errors must be zero", which makes a FALSE error its + * worst failure mode. Every one of those spellings is valid YAML that a consumer + * can legitimately write (and the shipped `review.md` itself uses flow style for + * `toolsets: [pull_requests, repos]`), so treating any of them as absent would + * fail a working install. What is still not supported: multi-line flow + * sequences, anchors/aliases, and block scalars (`|`, `>`). + */ + +/** One frontmatter line, reduced to what the callers ask about. */ +export type YamlLine = { + indent: number; + key?: string; + value?: string; + item?: string; +}; + +/** + * The frontmatter block of a markdown file: the lines between the leading `---` + * and the next `---`. Undefined when the file has no frontmatter at all. + */ +export const frontmatterBlock = (content: string): string | undefined => { + const lines = content.split(/\r?\n/); + if (lines[0]?.trim() !== "---") { + return undefined; + } + const end = lines.findIndex( + (line, index) => index > 0 && line.trim() === "---", + ); + return end === -1 ? undefined : lines.slice(1, end).join("\n"); +}; + +/** + * Drop a YAML inline comment from a value or list item. A `#` opens a comment + * only at the start of the value or after whitespace, and never inside a quoted + * scalar — so `1000 # LOCAL OVERRIDE` loses the comment while `"a # b"` and + * `https://x/y#frag` keep every character. + * + * Not cosmetic: the onboarding skill tells authors to label each local edit with + * a comment, so without this `Number("2500 # LOCAL OVERRIDE")` is `NaN` and the + * credit-ceiling check silently never fires, a commented `source:` reports a + * spurious ref mismatch, and a commented `imports` item reads as a missing + * import — a false error on a valid install. + */ +export const stripInlineComment = (raw: string): string => { + let quote: string | undefined; + for (let i = 0; i < raw.length; i++) { + const ch = raw[i]; + if (quote !== undefined) { + if (ch === quote) { + quote = undefined; + } + continue; + } + if (ch === '"' || ch === "'") { + quote = ch; + continue; + } + if (ch === "#" && (i === 0 || /\s/.test(raw[i - 1]))) { + return raw.slice(0, i).trimEnd(); + } + } + return raw.trimEnd(); +}; + +/** Strip one layer of matching surrounding quotes. */ +export const unquote = (raw: string): string => { + const trimmed = raw.trim(); + const first = trimmed[0]; + return (first === '"' || first === "'") && + trimmed.length > 1 && + trimmed.endsWith(first) + ? trimmed.slice(1, -1) + : trimmed; +}; + +/** Reduce a block to {@link YamlLine}s, dropping blanks and comments. */ +export const yamlLines = (block: string): YamlLine[] => + block + .split(/\r?\n/) + .filter((line) => line.trim() !== "" && !line.trim().startsWith("#")) + .map((line) => { + const indent = line.length - line.trimStart().length; + const trimmed = line.trim(); + if (trimmed.startsWith("- ")) { + return { + indent, + item: stripInlineComment(trimmed.slice(2)).trim(), + }; + } + const match = /^([A-Za-z0-9_.-]+):\s*(.*)$/.exec(trimmed); + return match + ? { + indent, + key: match[1], + value: stripInlineComment(match[2]).trim(), + } + : {indent}; + }); + +/** The base (outermost) indent of a block's own keys. */ +const baseIndent = (lines: readonly YamlLine[]): number => + Math.min(...lines.map((line) => line.indent)); + +/** + * The lines nested under `key` at this block's own indent level. A `- item` + * line at the SAME indent as the key, directly after it, also belongs to the + * key: YAML allows a block sequence at the parent key's indentation, so + * `imports:` followed by an unindented `- …` is a valid spelling of a working + * install, and reading it as absent would raise a false error (this checker's + * worst failure mode). A same-indent `key:` line still ends the block. + */ +export const nested = ( + lines: readonly YamlLine[], + key: string, +): YamlLine[] | undefined => { + if (lines.length === 0) { + return undefined; + } + const base = baseIndent(lines); + for (let i = 0; i < lines.length; i++) { + if (lines[i].indent === base && lines[i].key === key) { + const out: YamlLine[] = []; + for (let j = i + 1; j < lines.length; j++) { + const line = lines[j]; + const sameIndentItem = + line.indent === base && line.item !== undefined; + if (line.indent <= base && !sameIndentItem) { + break; + } + out.push(line); + } + return out; + } + } + return undefined; +}; + +/** Walk a key path, returning the deepest block (undefined if any hop misses). */ +export const nestedPath = ( + lines: readonly YamlLine[], + path: readonly string[], +): YamlLine[] | undefined => { + let current: YamlLine[] | undefined = [...lines]; + for (const key of path) { + if (current === undefined) { + return undefined; + } + current = nested(current, key); + } + return current; +}; + +/** + * The scalar `key` holds at this block's own indent level, with surrounding + * quotes stripped. A key with no inline value (a nested block, or an empty + * value) reads as undefined. + * + * Unquoting matters for the numeric reads: `max-ai-credits: "1000"` is valid + * YAML, and passing the raw `"1000"` to `Number()` yields `NaN`, which compares + * false against every threshold and silently suppresses the check. + */ +export const scalar = ( + lines: readonly YamlLine[], + key: string, +): string | undefined => { + if (lines.length === 0) { + return undefined; + } + const base = baseIndent(lines); + const hit = lines.find((line) => line.indent === base && line.key === key); + return hit?.value === undefined || hit.value === "" + ? undefined + : unquote(hit.value); +}; + +/** True when `key` exists at this block's own indent level. */ +export const hasKey = (lines: readonly YamlLine[], key: string): boolean => { + if (lines.length === 0) { + return false; + } + const base = baseIndent(lines); + return lines.some((line) => line.indent === base && line.key === key); +}; + +/** The `- item` values of a block, in order, with surrounding quotes stripped. */ +export const items = (lines: readonly YamlLine[]): string[] => + lines + .map((line) => line.item) + .filter((item): item is string => item !== undefined) + .map(unquote); + +/** + * The list `key` holds, in either spelling: block style (`- item` lines nested + * under the key) or flow style (`key: [a, b]`). Undefined only when the key is + * absent, which is what lets a caller tell "no list here" from "a list that came + * out empty" — the difference between a deliberate omission and a spelling this + * reader could not read. + * + * Flow style is not an exotic case to skip: the shipped `review.md` writes + * `toolsets: [pull_requests, repos]`, so a consumer copying that style into an + * allowlist is writing perfectly ordinary frontmatter. + */ +export const list = ( + lines: readonly YamlLine[], + key: string, +): string[] | undefined => { + if (!hasKey(lines, key)) { + return undefined; + } + const inline = scalar(lines, key); + if (inline !== undefined) { + if (!inline.startsWith("[") || !inline.endsWith("]")) { + // A scalar where a list belongs: report it as unreadable rather than + // as an empty list, so the caller does not mistake it for absence. + return []; + } + return inline + .slice(1, -1) + .split(",") + .map((entry) => unquote(entry)) + .filter((entry) => entry !== ""); + } + return items(nested(lines, key) ?? []); +}; diff --git a/workflows/review/lib/gitattributes.test.ts b/workflows/review/lib/gitattributes.test.ts new file mode 100644 index 00000000..3c9bd895 --- /dev/null +++ b/workflows/review/lib/gitattributes.test.ts @@ -0,0 +1,98 @@ +import {describe, it, expect} from "vitest"; + +import {isGenerated, parseGitattributesGenerated} from "./router.ts"; + +/** + * `.gitattributes` generated-file classification, split from router.test.ts by + * concern and its max-lines budget (as credit-cap and lens-payloads are). + * + * What these pin is one semantic: git resolves an attribute per path by the LAST + * matching line, so `linguist-generated` is not a set of "generated globs" but an + * ordered list of verdicts. Reading it as any-match-wins silently skipped review + * of paths a repo had deliberately un-marked, which is why the ordering cases + * below are the point of the file rather than edge cases in it. + */ + +describe("parseGitattributesGenerated", () => { + it("keeps every rule that mentions the attribute, in file order", () => { + const content = [ + "# comment", + "", + "dist/** linguist-generated=true", + "vendor/** linguist-generated", + "generated/keep.ts -linguist-generated", + "src/*.ts text", + "other/*.js linguist-generated=false", + "third/*.js !linguist-generated", + ].join("\n"); + // Negations are retained rather than dropped: isGenerated resolves per + // path by last match, so a later `=false` has to be able to win. A line + // that never mentions the attribute (`src/*.ts text`) is not a rule. + expect(parseGitattributesGenerated(content)).toEqual([ + {pattern: "dist/**", generated: true}, + {pattern: "vendor/**", generated: true}, + {pattern: "generated/keep.ts", generated: false}, + {pattern: "other/*.js", generated: false}, + {pattern: "third/*.js", generated: false}, + ]); + }); +}); + +describe("isGenerated", () => { + // Git resolves an attribute per path by LAST matching line. The reviewer read + // it as "any matching =true line wins", which silently skipped review of paths + // a repo had deliberately un-marked (observed in Khan/agent-settings, whose + // .gitattributes marks `.claude/**` generated and then un-marks + // `.claude/skills/**` so its skills stay visible and reviewable). + const rules = parseGitattributesGenerated( + [ + ".claude/** linguist-generated=true", + ".claude/skills/** linguist-generated=false", + ].join("\n"), + ); + + it("lets a later negation un-mark an earlier broad glob", () => { + expect(isGenerated(".claude/hooks/git-filter.mjs", rules)).toBe(true); + expect(isGenerated(".claude/skills/foo/SKILL.md", rules)).toBe(false); + }); + + it("keeps ordering load-bearing: reversing the lines reverses the answer", () => { + const reversed = parseGitattributesGenerated( + [ + ".claude/skills/** linguist-generated=false", + ".claude/** linguist-generated=true", + ].join("\n"), + ); + expect(isGenerated(".claude/skills/foo/SKILL.md", reversed)).toBe(true); + }); + + it("treats a path no rule matches as source", () => { + expect(isGenerated("src/index.ts", rules)).toBe(false); + }); + + // Git has three ways to spell "stop being generated", and each one shadows an + // earlier `=true` by last match; verified against `git check-attr`, which + // reports `.claude/skills/foo/SKILL.md` as unset / false / unspecified for + // these three respectively, and `.claude/**` as true in all three. A form the + // parser fails to recognise is not treated as a negation at all: its line is + // dropped, the broad glob still wins, and the path is silently skipped. That + // is the same failure this file exists to pin, so all three are covered. + it.each([ + ["-linguist-generated", "unset"], + ["linguist-generated=false", "false"], + ["!linguist-generated", "unspecified"], + ])("lets `%s` (git: %s) shadow an earlier =true glob", (negation) => { + const withNegation = parseGitattributesGenerated( + [ + ".claude/** linguist-generated=true", + `.claude/skills/** ${negation}`, + ].join("\n"), + ); + expect(isGenerated(".claude/skills/foo/SKILL.md", withNegation)).toBe( + false, + ); + expect(isGenerated(".claude/hooks/git-filter.mjs", withNegation)).toBe( + true, + ); + }); +}); diff --git a/workflows/review/lib/investigation-cap.test.ts b/workflows/review/lib/investigation-cap.test.ts index ee816bce..25b78728 100644 --- a/workflows/review/lib/investigation-cap.test.ts +++ b/workflows/review/lib/investigation-cap.test.ts @@ -156,7 +156,7 @@ describe("DEFAULT_TOOL_CALL_CAPS", () => { describe("capsFromRunBudget", () => { // Build a real RunBudget via the production path rather than a hand literal. - const config: RouterConfig = {generatedPatterns: []}; + const config: RouterConfig = {generatedRules: []}; it("projects exactly the two cap fields out of a full RunBudget", () => { const budget = computeRunBudget("medium", false, config); @@ -305,7 +305,7 @@ describe("InvestigationCap", () => { }); it("builds a guard from a RunBudget whose caps match that budget", () => { - const budget = computeRunBudget("high", false, {generatedPatterns: []}); + const budget = computeRunBudget("high", false, {generatedRules: []}); const guard = InvestigationCap.fromRunBudget(budget); expect(guard.getCaps()).toEqual(capsFromRunBudget(budget)); }); diff --git a/workflows/review/lib/router.test.ts b/workflows/review/lib/router.test.ts index 8aebd17c..b109a8c6 100644 --- a/workflows/review/lib/router.test.ts +++ b/workflows/review/lib/router.test.ts @@ -50,7 +50,7 @@ const file = ( // map lives in its own .github/aw/review/ROUTING file), so the tests supply // the rules they exercise, shaped like a typical consumer config. const baseConfig: RouterConfig = { - generatedPatterns: [], + generatedRules: [], lensRules: [ {pattern: "**/*.sql", lenses: ["data-migrations"]}, {pattern: "**/migrations/**", lenses: ["data-migrations"]}, @@ -117,24 +117,6 @@ describe("patternSpecificity", () => { /* Config parsers */ /* -------------------------------------------------------------------------- */ -describe("parseGitattributesGenerated", () => { - it("collects linguist-generated patterns and honours negation", () => { - const content = [ - "# comment", - "", - "dist/** linguist-generated=true", - "vendor/** linguist-generated", - "generated/keep.ts -linguist-generated", - "src/*.ts text", - "other/*.js linguist-generated=false", - ].join("\n"); - expect(parseGitattributesGenerated(content)).toEqual([ - "dist/**", - "vendor/**", - ]); - }); -}); - describe("teamSlug", () => { it("strips @, org prefix, trailing !, and lowercases", () => { expect(teamSlug("@Khan/Security!")).toBe("security"); @@ -165,14 +147,14 @@ describe("parseReviewers", () => { describe("route: classification", () => { it("marks linguist-generated files generated with no lenses/teams/tier", () => { - const generatedPatterns = parseGitattributesGenerated( + const generatedRules = parseGitattributesGenerated( "dist/** linguist-generated=true", ); - expect(isGenerated("dist/bundle.js", generatedPatterns)).toBe(true); + expect(isGenerated("dist/bundle.js", generatedRules)).toBe(true); const result = route( {files: [file("dist/bundle.js")]}, - {...baseConfig, generatedPatterns}, + {...baseConfig, generatedRules}, ); expect(result.perFile[0]).toMatchObject({ path: "dist/bundle.js", @@ -543,7 +525,11 @@ describe("toRoutingJson", () => { const reviewerRules = parseReviewers("src/auth/ @Khan/Security"); const result = route( {files: [file("src/auth/login.ts"), file("dist/bundle.js")]}, - {...baseConfig, generatedPatterns: ["dist/**"], reviewerRules}, + { + ...baseConfig, + generatedRules: [{pattern: "dist/**", generated: true}], + reviewerRules, + }, ); const json = toRoutingJson(result); // Every tier is emitted in the display casing review.md consumes. diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index 7764a558..78aef505 100644 --- a/workflows/review/lib/router.ts +++ b/workflows/review/lib/router.ts @@ -214,8 +214,12 @@ export type RunBudget = { }; export type RouterConfig = { - /** linguist-generated globs (from `.gitattributes`). */ - generatedPatterns: string[]; + /** + * `linguist-generated` rules from `.gitattributes`, in file order. Order is + * load-bearing: {@link isGenerated} resolves per path by last match, so a + * later negation un-marks an earlier glob. + */ + generatedRules: GeneratedRule[]; /** * path->lens rules, from the consumer's `ROUTING` file * ({@link parseRoutingConfig}). No default: absent config means no @@ -303,13 +307,37 @@ export type RouteInput = { /* -------------------------------------------------------------------------- */ /** - * Extract the linguist-generated globs from `.gitattributes` content. A line - * assigns attributes to a pattern: ` attr1 attr2 ...`. We collect the - * pattern when it sets `linguist-generated` truthy, and honour explicit - * negation (`-linguist-generated` or `linguist-generated=false`). + * One `.gitattributes` line's verdict on `linguist-generated` for the paths its + * glob matches. Negations are kept rather than dropped, because git resolves the + * attribute per path by LAST matching line, so a later `=false` has to be able to + * override an earlier `=true` — see {@link isGenerated}. + */ +export type GeneratedRule = { + pattern: string; + generated: boolean; +}; + +/** + * Extract the `linguist-generated` rules from `.gitattributes` content, in file + * order. A line assigns attributes to a pattern: ` attr1 attr2 ...`; a + * line that mentions the attribute either way becomes a rule, set by + * `linguist-generated` / `linguist-generated=true` and cleared by + * `-linguist-generated` / `linguist-generated=false` / `!linguist-generated`. + * Lines that never mention it are not rules at all and are skipped, so they + * cannot shadow a real one. + * + * Git has four attribute states, and all three of the non-set forms have to + * become rules so they can shadow an earlier broad `=true` glob by last match: + * `-attr` resolves to Unset, `attr=false` to the value `false`, and `!attr` to + * Unspecified. Unspecified is not the same as false in general (it is where + * Linguist falls back to its content heuristic), but this router has no content + * heuristic and treats an unmatched path as source, so Unspecified and false + * reach the same verdict here and share the branch below. */ -export const parseGitattributesGenerated = (content: string): string[] => { - const patterns: string[] = []; +export const parseGitattributesGenerated = ( + content: string, +): GeneratedRule[] => { + const rules: GeneratedRule[] = []; for (const rawLine of content.split(/\r?\n/)) { const line = rawLine.trim(); if (line === "" || line.startsWith("#")) { @@ -320,9 +348,8 @@ export const parseGitattributesGenerated = (content: string): string[] => { if (pattern === undefined) { continue; } - const attrs = tokens.slice(1); - let generated = false; - for (const attr of attrs) { + let generated: boolean | undefined; + for (const attr of tokens.slice(1)) { if ( attr === "linguist-generated" || attr === "linguist-generated=true" @@ -330,16 +357,18 @@ export const parseGitattributesGenerated = (content: string): string[] => { generated = true; } else if ( attr === "-linguist-generated" || - attr === "linguist-generated=false" + attr === "linguist-generated=false" || + // Unspecified; source as far as this router is concerned. + attr === "!linguist-generated" ) { generated = false; } } - if (generated) { - patterns.push(pattern); + if (generated !== undefined) { + rules.push({pattern, generated}); } } - return patterns; + return rules; }; /** @@ -392,11 +421,29 @@ export const parseReviewers = (content: string): ReviewerRule[] => { /* Per-file decisions */ /* -------------------------------------------------------------------------- */ -/** Whether `path` matches any linguist-generated glob. */ +/** + * Whether `path` is linguist-generated, resolved the way git resolves an + * attribute: the LAST matching rule wins, so a `linguist-generated=false` line + * placed after a broad `=true` glob un-marks the paths it covers. A path no rule + * matches is source. + * + * Scanning in reverse rather than collecting only the `=true` patterns is the + * whole point: a repo that marks `.claude/**` generated and then un-marks + * `.claude/skills/**` means its skills to be reviewed, and the earlier + * any-match-wins reading silently skipped them. + */ export const isGenerated = ( path: string, - generatedPatterns: string[], -): boolean => generatedPatterns.some((pattern) => matchesGlob(path, pattern)); + generatedRules: readonly GeneratedRule[], +): boolean => { + for (let i = generatedRules.length - 1; i >= 0; i--) { + const rule = generatedRules[i]; + if (matchesGlob(path, rule.pattern)) { + return rule.generated; + } + } + return false; +}; /** Specialist lenses for one path (union of all matching rules, deduped). */ const lensesForFile = (path: string, lensRules: LensRule[]): Lens[] => { @@ -539,7 +586,7 @@ export const route = ( let hasSource = false; for (const file of input.files) { - const generated = isGenerated(file.path, config.generatedPatterns); + const generated = isGenerated(file.path, config.generatedRules); if (generated) { // Generated files: contents are not analysed, so no lenses, no team @@ -778,7 +825,14 @@ const ROUTING_OUT = `${REVIEW_DIR}/routing.json`; /** Optional second-pass input: {path: tier} answers for pending questions. */ const RESOLVED_TIERS_PATH = `${REVIEW_DIR}/resolved-tiers.json`; const GITATTRIBUTES_PATH = ".gitattributes"; -const REVIEWERS_PATH = ".github/REVIEWERS"; +/** + * The consumer's team-ownership map, and the router's ONLY source of it: with no + * such file both `teams.owners` and the ranked `fallbackTeams` come out empty, so + * the prompt's Step 8 has nobody to request no matter what `add-reviewer` allows. + * Exported because the config checker needs that same fact to tell an empty + * reviewer allowlist that is accurate from one that is broken. + */ +export const REVIEWERS_PATH = ".github/REVIEWERS"; type FsLike = { readFileSync: (p: string, enc: "utf8") => string; @@ -822,7 +876,7 @@ export const runCli = ( }; }); - const generatedPatterns = fs.existsSync(repoPath(GITATTRIBUTES_PATH)) + const generatedRules = fs.existsSync(repoPath(GITATTRIBUTES_PATH)) ? parseGitattributesGenerated(readText(repoPath(GITATTRIBUTES_PATH))) : []; const reviewerRules = fs.existsSync(repoPath(REVIEWERS_PATH)) @@ -889,7 +943,7 @@ export const runCli = ( } const result = route(input, { - generatedPatterns, + generatedRules, reviewerRules, lensRules: routingFileConfig.lensRules, riskRules: routingFileConfig.riskRules,