Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitattributes
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
14 changes: 14 additions & 0 deletions .github/aw/actions-lock.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"entries": {
"github/gh-aw-actions/setup-cli@v0.81.6": {
"repo": "github/gh-aw-actions/setup-cli",
"version": "v0.81.6",
"sha": "ba6380cc6e5be5d21677bebe04d52fb48e3abec7"
},
"github/gh-aw-actions/setup@v0.81.6": {
"repo": "github/gh-aw-actions/setup",
"version": "v0.81.6",
"sha": "ba6380cc6e5be5d21677bebe04d52fb48e3abec7"
}
}
}
44 changes: 44 additions & 0 deletions .github/aw/review/ROUTING
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# 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). Any shared workflow's
# prompt (and prompt imports beside it) is high; the docs living in the same
# directory drop back to trivial. Single-star segments do not cross into
# subdirectories, so nothing under eval/ gets re-raised.
**/*.md tier=trivial
.changeset/*.md tier=trivial
workflows/*/*.md tier=high
workflows/*/README.md tier=trivial
workflows/*/CHANGELOG.md tier=trivial
.github/workflows/*.md tier=high

# The reviewer's own consumer config: config.md carries the add-reviewer team
# allowlist and bot token, ROUTING sets these tiers, and the prose imports steer
# every sub-agent. A PR tampering with any of them must not run at the trivial
# or default budget.
.github/aw/review/** tier=high
36 changes: 36 additions & 0 deletions .github/aw/review/ci-tooling.md
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.
26 changes: 26 additions & 0 deletions .github/aw/review/config.md
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.
-->
131 changes: 131 additions & 0 deletions .github/aw/review/risk-classification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
<!--

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.
31 changes: 31 additions & 0 deletions .github/aw/review/skills.md
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.
2 changes: 1 addition & 1 deletion .github/workflows/node-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,4 @@ jobs:

- uses: ./actions/check-for-changeset
with:
exclude: .github/,utils,.eslintrc.js,.prettierrc.js
exclude: .github/,utils,.eslintrc.js,.prettierrc.js,.gitattributes
58 changes: 58 additions & 0 deletions .github/workflows/review-pins.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* CI backstop for the pins in this repo's installed reviewer (review.md here,
* distinct from the shared source at workflows/review/review.md).
*
* The install carries coupled pins that no release automation touches:
* `source:`, recording which Khan/actions release the prompt was copied from,
* the `ref:` of the gh-aw-review-lib checkout that fetches the deterministic
* lib the prompt invokes at runtime, and the copies of both that `gh aw
* compile` bakes into review.lock.yml (which is what actually executes, and
* which .gitattributes marks merge=ours, so a merge can silently keep a stale
* lock). If a future `gh aw update`, a hand edit, or a skipped recompile moves
* one without the others, the prompt from one release runs the lib of another;
* this test fails that PR. (The shared source's own ref is kept true by
* utils/sync-workflow-versions.ts and its backstop in
* workflows/review/version-sync.test.ts; neither covers these files.)
*/
import * as fs from "fs";
import {describe, expect, it} from "vitest";

const reviewMd = fs.readFileSync(
new URL("./review.md", import.meta.url),
"utf-8",
);
const reviewLock = fs.readFileSync(
new URL("./review.lock.yml", import.meta.url),
"utf-8",
);

const sourceRef = reviewMd.match(
/^source:\s*Khan\/actions\/workflows\/review\/review\.md@(\S+)\s*$/m,
)?.[1];
const checkoutRefs = [...reviewMd.matchAll(/^\s*ref:\s*(\S+)\s*$/gm)].map(
(m) => m[1],
);

describe("installed review.md pins", () => {
it("records the review release the prompt was copied from", () => {
expect(sourceRef).toMatch(/^review-v\d+\.\d+\.\d+$/);
});

it("checks out the lib at exactly the source: release", () => {
expect(checkoutRefs).toEqual([sourceRef]);
});

it("names no other release anywhere in the file", () => {
const literals = reviewMd.match(/review-v\d+\.\d+\.\d+/g) ?? [];
expect(literals.length).toBeGreaterThan(0);
expect(new Set(literals)).toEqual(new Set([sourceRef]));
});
});

describe("compiled review.lock.yml pins", () => {
it("was recompiled from this review.md (every baked literal matches)", () => {
const literals = reviewLock.match(/review-v\d+\.\d+\.\d+/g) ?? [];
expect(literals.length).toBeGreaterThan(0);
expect(new Set(literals)).toEqual(new Set([sourceRef]));
});
});
Loading
Loading