-
Notifications
You must be signed in to change notification settings - Fork 1
Enable the automatic AI PR reviewer on this repo #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
22d3853
0fb0987
b5c203f
cd2b891
f3b902f
13f143e
5fa4d22
9de5611
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,3 @@ | ||
| pnpm-lock.yaml binary linguist-generated | ||
|
|
||
| .github/workflows/*.lock.yml linguist-generated=true merge=ours |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "entries": { | ||
| "github/gh-aw-actions/setup@v0.81.6": { | ||
| "repo": "github/gh-aw-actions/setup", | ||
| "version": "v0.81.6", | ||
| "sha": "ba6380cc6e5be5d21677bebe04d52fb48e3abec7" | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # Machine-readable routing for the PR-reviewer workflow. Parsed deterministically by | ||
| # the shared router (workflows/review/lib in this repo; format spec in | ||
| # workflows/review/README.md). One rule per line: | ||
| # <pattern> [lens=<lens>,...] [tier=trivial|low|medium|high] [direction-dependent] | ||
| # enable <reviewer>[,<reviewer>...] | ||
| # Later tier rules override earlier ones (gitignore-style); lenses union across | ||
| # matching rules. This file complements risk-classification.md (model-facing prose); | ||
| # team ownership stays in .github/REVIEWERS. | ||
| # | ||
| # No specialist lenses are enabled: the eleven lens prompts target webapp's backend | ||
| # domains (auth resolvers, migrations, federation, ...) and none maps to this repo's | ||
| # supply-chain surface. Whole-change reviewers mirror webapp's eval-justified roster. | ||
|
|
||
| enable holistic,completeness,test-adequacy,first-principles,conventions | ||
|
|
||
| # Shipped composite actions run inside consuming repos' CI with their credentials. | ||
| actions/** tier=high | ||
| # This repo's own CI, the publish pipeline, and the compiled agentic workflows. | ||
| .github/workflows/** tier=high | ||
| utils/** tier=high | ||
| # The shared reviewer's deterministic enforcement layer. | ||
| workflows/review/lib/** tier=high | ||
|
|
||
| # Dev-only surfaces. | ||
| workflows/review/eval/** tier=low | ||
| **/*.test.ts tier=low | ||
|
|
||
| # Docs are trivial; agentic-workflow .md files are executable, not docs, so they | ||
| # are re-raised after the broad docs rule (last match wins). | ||
| **/*.md tier=trivial | ||
| .changeset/*.md tier=trivial | ||
| workflows/review/review.md tier=high | ||
| .github/workflows/*.md tier=high | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| <!-- | ||
| Khan/actions-specific list of issues the CI/tooling already catches, so the reviewer | ||
| doesn't flag them. Injected into the shared workflow (workflows/review/review.md in | ||
| this repo) at runtime. Keep this plain Markdown with no GitHub Actions template | ||
| expressions. | ||
|
|
||
| CI here is `node-ci.yml` (every PR) plus `check-for-changeset`. The reviewer's value | ||
| is what none of this sees: shell/template injection in action.yml files, publish/tag | ||
| integrity, gh-aw frontmatter security surfaces, and semver-as-behavior-contract | ||
| judgment (see risk-classification.md "What to verify"). | ||
| --> | ||
|
|
||
| - **Formatting / lint**: `eslint` with `@khanacademy/eslint-config` + | ||
| `eslint-plugin-prettier` (Prettier runs as a lint rule). Never comment on | ||
| formatting, import order, or style eslint enforces. | ||
| - **Type errors**: `pnpm typecheck` (`tsc --noEmit`) and `pnpm build` | ||
| (`tsc -p actions/tsconfig.json`). If it wouldn't typecheck or compile, CI fails. | ||
| - **Unit tests**: `pnpm test --run` (vitest) runs the full suite, including the | ||
| reviewer lib tests under `workflows/review/lib/` and the eval smoke test. | ||
| - **Action wiring**: `utils/check-action-dependencies.ts` verifies intra-repo | ||
| `uses:` references between actions; `node-ci.yml` also functionally smoke-tests | ||
| `filter-files`, `get-changed-files`, and `json-args` against fixed inputs. | ||
| - **Changeset presence**: `check-for-changeset` fails a PR that changes published | ||
| action code without a changeset (`.github/`, `utils`, and lint configs are | ||
| excluded). Don't flag a *missing* changeset; DO review whether the changeset's | ||
| semver level matches the change; CI can't judge that (see risk-classification.md). | ||
|
|
||
| ### Don't raise these false alarms | ||
|
|
||
| - `action.yml` files are not covered by eslint/tsc; but don't flag YAML syntax or | ||
| schema errors speculatively; a broken action.yml fails its smoke test or the | ||
| consumer's first use. Focus on injection and logic, not syntax. | ||
| - `.lock.yml` workflow files are compiled output; don't review their contents | ||
| line-by-line; verify the source `.md` changed with them (pairing check). | ||
| - Vitest tests here import from `node:` builtins and use `memfs`; that's the | ||
| established pattern, not a smell. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| --- | ||
| # Khan/actions-specific reviewer configuration, merged into the shared review workflow | ||
| # (workflows/review/review.md in this repo) at COMPILE time via its `imports:` field. | ||
| # | ||
| # Only the frontmatter below is merged; this file's markdown body is ignored by | ||
| # `imports:`. This file owns the `add-reviewer` safe output; the repo-specific | ||
| # `allowed-team-reviewers` allowlist and the bot token used to request teams. It is | ||
| # defined ONLY here (never in review.md): gh-aw lets the main workflow override an | ||
| # imported safe-output of the same type, so defining it there would drop the allowlist. | ||
| # | ||
| # This repo also requests reviewers via Gerald (`.github/REVIEWERS`), which routes | ||
| # everything to @Khan/github-actions. This `add-reviewer` runs in addition to Gerald; | ||
| # the shared workflow's Step 8 dedups against teams already requested or reviewed. | ||
| safe-outputs: | ||
| add-reviewer: | ||
| target: "triggering" | ||
| max: 2 | ||
| allowed-team-reviewers: | ||
| - github-actions | ||
| github-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }} | ||
| --- | ||
|
|
||
| <!-- | ||
| This file is a compile-time frontmatter import for the shared PR-reviewer workflow. | ||
| Its body is intentionally empty: `imports:` merges frontmatter only. | ||
| --> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| <!-- | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine for now - I'm curious how much of this file we still need wit the new ROUTING classifier (probably this should only be relevant for files that weren't previously classified by ROUTING)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on the destination. ROUTING should be the single source of truth for path-to-tier, with this file refocused on what globs can't express (deep-checks, direction-dependent judgments, unclassified paths). I'd hold off on a shared-prompt change for now though. Router tiers already win where it counts (budget and verdict), ROUTING is optional while this file is required, and taking tiers as given is a behavior change per the semver contract, so it wants an eval run once the A/B tooling lands. In the meantime this repo's file already leans that way (mostly deep-checks, thin tier prose), and we can treat that as the convention for new installs. |
||
| Khan/actions-specific risk-tier file patterns AND the deep-checks for the PR-reviewer | ||
| workflow. Injected into the shared workflow (workflows/review/review.md in this repo) | ||
| at runtime. Keep this plain Markdown with no GitHub Actions template expressions; | ||
| gh-aw rejects those inside imported files. | ||
|
|
||
| The blast radius in this repo is supply-chain shaped: the composite actions under | ||
| `actions/` and the shared agentic workflows under `workflows/` execute inside OTHER | ||
| Khan repos' CI, with those repos' tokens and secrets, pinned by release tag. The most | ||
| severe changes here don't break this repo; they break or compromise every consumer | ||
| on its next version bump. | ||
| --> | ||
|
|
||
| ### Generated files are automatically Trivial | ||
|
|
||
| - `pnpm-lock.yaml` (marked `linguist-generated` in `.gitattributes`). | ||
| - `.github/workflows/*.lock.yml`; compiled by `gh aw compile` from the sibling | ||
| `.md` agentic workflow. Don't deep-review compiled lock contents; instead verify | ||
| the pairing (see "What to verify"). A `.lock.yml` edited **without** a matching | ||
| `.md` change is a red flag; classify by the manual edit. | ||
| - `actions/*/index.js` bundles produced by the build. A compiled bundle edited | ||
| without a source change is a red flag. | ||
|
|
||
| ### High Risk | ||
|
|
||
| Runs in consuming repos' CI (with their credentials), or controls what gets | ||
| published to them: | ||
|
|
||
| - **Shipped composite actions**: `actions/*/action.yml` and the code they invoke. | ||
| These execute in webapp/frontend/perseus/etc. CI with those repos' `GITHUB_TOKEN` | ||
| (and sometimes admin/bot tokens passed as inputs). Injection via untrusted | ||
| interpolation, a new outbound network call, loosened token handling, or a subtly | ||
| wrong filter (e.g. `filter-files` matching too much/little gates other repos' CI | ||
| decisions) all propagate on the next release. | ||
| - **The publish pipeline**: `utils/run-publish.ts`, `utils/check-action-dependencies.ts`, | ||
| `.github/workflows/release.yml`. Cuts and force-moves the release tags every | ||
| consumer pins (`<action>-vX`, `review-vX`). A bug can publish the wrong tree to a | ||
| tag or rewrite a moving major tag out from under consumers. | ||
| - **This repo's workflow files**: `.github/workflows/**`: `permissions:` blocks, | ||
| secret usage, event triggers, action pinning. The compiled agentic locks | ||
| (`*.lock.yml`) are generated, but their frontmatter sources (`review.md` here and | ||
| in `workflows/review/`) define real permission/secret/network surfaces. | ||
| - **The shared PR reviewer**: `workflows/review/review.md` (the prompt IS the | ||
| reviewer's behavior in every consuming repo; its frontmatter carries security | ||
| surfaces: `safe-outputs` caps, `allowed-domains`, `network.allowed`, `roles`, | ||
| `permissions`) and `workflows/review/lib/**` (the deterministic enforcement | ||
| layer: finding schema, change-provenance gate, verdict computation, | ||
| investigation cap, routing parser; a bug here silently changes what gets | ||
| blocked or posted on every consumer PR). | ||
|
|
||
| ### Medium Risk | ||
|
|
||
| - **The eval suite**: `workflows/review/eval/*.ts` (judge, gates, metrics, | ||
| runner). Dev-only, but it decides which reviewers/lenses "earn their line" in | ||
| consumer ROUTING files; a wrong judge or gate corrupts those decisions. | ||
| - **Build/lint/type configuration**: `config/`, `types/`, `tsconfig.json`, | ||
| `.eslintrc.js`, `pnpm-workspace.yaml`: shapes the compiled output of shipped | ||
| actions. | ||
| - **Staged workflows**: `.github-staging/**` (not yet active, but will be | ||
| promoted verbatim). | ||
|
|
||
| ### Low Risk | ||
|
|
||
| - Test files (`**/*.test.ts`) and eval corpus fixtures | ||
| (`workflows/review/eval/corpus/**`). | ||
| - Internal refactors with test coverage; logging/output-formatting tweaks in | ||
| utils. | ||
|
|
||
| ### Trivial Risk | ||
|
|
||
| - Generated files (above); Markdown docs and READMEs (except | ||
| `workflows/review/review.md`, which is executable prompt, not docs); | ||
| `.changeset/*.md` entries; comments/naming/formatting-only changes. | ||
|
|
||
| ### Disambiguation | ||
|
|
||
| - Highest applicable level wins. | ||
| - **Diff direction matters**: read the hunk, not just the path. In gh-aw | ||
| frontmatter, widening (`allowed-domains` additions, a raised safe-output `max`, | ||
| a new secret, a permission upgrade, loosening `roles:` or the fork guard) is the | ||
| risky direction; tightening is usually safe. In an action's `action.yml`, a new | ||
| input interpolated into a `run:` block is riskier than a removed one. | ||
| - If the PR description justifies a risky change (evaluated against the eval | ||
| suite, security-reviewed frontmatter, a documented rollback), you may lower the | ||
| risk one tier but note the justification. | ||
|
|
||
| --- | ||
|
|
||
| ## What to verify (CI cannot catch these) | ||
|
|
||
| CI here runs eslint/prettier, `tsc`, vitest, a build, changeset presence, and smoke | ||
| tests of a few actions. It has no actionlint/shellcheck, no security scanning, and | ||
| no check that compiled artifacts match their sources. Findings here are candidates | ||
| for **blocking** comments: | ||
|
|
||
| **Template injection in composite actions.** Any dollar-brace GitHub Actions | ||
| expression over `inputs.*`, `github.event.*`, or another attacker-influenceable | ||
| value, interpolated directly into a `run:` script or `github-script` body, is a | ||
| command/JS injection in the CONSUMING repo's CI. Require env-var indirection | ||
| (`env:` + `"$VAR"`) for untrusted values. Same check applies to workflow files in | ||
| this repo. (Spelled without the literal expression syntax here because gh-aw | ||
| rejects runtime imports containing it; see this file's header.) | ||
|
|
||
| **Action pinning.** Every new or changed `uses:` of a third-party action must be | ||
| pinned to a full commit SHA (with a version comment), not a tag or branch. | ||
| Intra-repo `uses:` references are rewritten at publish; verify a new one is known | ||
| to `utils/check-action-dependencies.ts` conventions. | ||
|
|
||
| **Publish/tag integrity.** For `utils/run-publish.ts` and `release.yml` changes: | ||
| verify the tag-rewriting logic still publishes exactly the action's subtree, that | ||
| moving major tags advance only on release, and that a dry-run path stays dry. | ||
|
|
||
| **gh-aw frontmatter security surfaces.** For `review.md` (shared or installed): | ||
| treat a widened `allowed-domains` list, a raised safe-output `max`, a new | ||
| `network.allowed` entry, a new secret reference, added `permissions:`, or a | ||
| weakened trigger guard (`if:` fork/branch exclusions, `roles:`) as a security | ||
| regression needing explicit justification in the PR description. This repo is | ||
| public: the fork-PR guard in the installed `.github/workflows/review.md` must not | ||
| be loosened. | ||
|
|
||
| **Compiled-artifact pairing.** A change to an agentic `.md` workflow must ship the | ||
| recompiled `.lock.yml` (and vice versa); a change to reviewer `lib/` code that the | ||
| prompt's pinned `gh-aw-review-lib` checkout ref doesn't cover only takes effect on | ||
| the next release; flag a PR that claims immediate effect for a lib change without | ||
| a release/pin bump. | ||
|
|
||
| **Reviewer behavior contract.** Semver on the `review` package is a behavior | ||
| contract (see `workflows/review/README.md`): a release that changes reviewer | ||
| behavior must bump the major version. Flag a behavior-changing `workflows/review/` | ||
| change whose changeset is patch/minor, and any change to `lib/finding-schema.ts`'s | ||
| `FINDING_SCHEMA_VERSION` semantics without the version bump. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <!-- | ||
| Khan/actions-specific best-practice catalog for the PR-reviewer workflow. Injected at | ||
| runtime into the shared workflow's `skill-auditor` sub-agent (workflows/review/ | ||
| review.md in this repo). That agent has no GitHub API access; it reads each entry's | ||
| file directly from its checked-out workspace on disk, so the paths here must match | ||
| real files. Keep this plain Markdown with no GitHub Actions template expressions. | ||
|
|
||
| This repo has no `.claude/skills/` catalog; its documented conventions live in two | ||
| READMEs. Audit against those only. A deviation is *blocking* only when it is itself | ||
| a correctness, security, or breaking-change issue; a merely non-idiomatic deviation | ||
| is a non-blocking suggestion. A convention not written in one of the files below is | ||
| not auditable; do not flag from memory or from other Khan repos' conventions. | ||
| --> | ||
|
|
||
| ### release-and-versioning - `README.md` (repo root) | ||
|
|
||
| **Evaluate when:** anything under `actions/` changes, or `utils/run-publish.ts` / | ||
| `.github/workflows/release.yml` change. Covers the monorepo publish model (bare | ||
| tags per action), the changeset flow (a code change to a published action needs a | ||
| changeset; never hand-edit an action's `package.json` version or `CHANGELOG.md`), | ||
| and the dry-run testing convention for the publish pipeline. | ||
|
|
||
| ### review-workflow-contract - `workflows/review/README.md` | ||
|
|
||
| **Evaluate when:** anything under `workflows/review/` changes. Covers the consumer | ||
| configuration contract (which files a consuming repo must provide, and that | ||
| `add-reviewer` lives only in the consumer's `config.md`), the `ROUTING` file format | ||
| and its last-match-wins tier semantics, the model/effort-per-role table, the | ||
| versioning policy (semver as behavior contract; the `pr-reviewer:version` marker), | ||
| and the policy that opt-in reviewers and lenses earn their `ROUTING` line through | ||
| the eval suite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want this to match any workflow .md file (I'm sure we'll have others)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in f3b902f:
workflows/*/*.mdis now tier=high, with the co-located README/CHANGELOG dropped back to trivial. Single-star segments don't cross into subdirectories, so nothing under eval/ is re-raised. Re-validated againstlib/routing-config.ts: zero warnings, tiers unchanged for every currently checked-in path.