Enable the automatic AI PR reviewer on this repo#241
Conversation
Install the shared PR-reviewer agentic workflow (workflows/review/review.md) as a consumer of its own repo, pinned to review-v1.4.0, with the default automatic pull_request trigger (opened/synchronize/reopened/ready_for_review); no slash-command opt-in. - .github/workflows/review.md + review.lock.yml: the installed prompt and its gh-aw compile output. Local overrides on top of the shared source: a fork-PR guard added to the job-level if: (this repo is public, unlike webapp), and the gh-aw-review-lib checkout ref bumped from the stale review-v1.2.2 to review-v1.4.0 so prompt and lib run from one release. - .github/aw/review/: the required consumer config; add-reviewer allowlist (github-actions, via Gerald's REVIEWERS), risk tiers and deep-checks written for this repo's supply-chain blast radius, the CI-caught issue list (eslint/tsc/vitest/changeset), a skills catalog pointing at the two convention READMEs, and ROUTING enabling the same whole-change reviewer roster as webapp (no specialist lenses; they target webapp domains). - .gitattributes: gh aw compile marks *.lock.yml linguist-generated. - node-ci.yml: exclude .gitattributes from check-for-changeset; metadata changes should not require cutting a release.
|
khan-actions-bot
requested review from
a team,
jaredly and
jeresig
and removed request for
a team
…classification runtime import gh-aw validates runtime-imported files and rejects unauthorized GitHub Actions expressions; the template-injection deep-check quoted the very syntax it warns about, which failed the review workflow's activation job on this PR. Describe the syntax in prose instead.
…try secrets The first agent run failed at startup: with no GH_AW_OTEL_SENTRY_ENDPOINT secret, the compiled lock feeds an empty URL into the MCP gateway's OTLP config, whose schema requires a non-empty https:// endpoint. A missing secret is a hard failure, not a graceful degrade. Comment out the shared source's observability block as a marked local override; restore it once the two GH_AW_OTEL_SENTRY_* secrets exist here.
Review Guidancegithub-actions (3 files)
Excluded from review (4 files)Not individually reviewed — generated or trivial-tier reviewer-config prose (the config prose was still seen by the whole-change reviewers):
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # so the prompt and the lib it invokes come from one version. Even though this | ||
| # IS Khan/actions, the reviewer runs the released lib, not the PR head; a PR | ||
| # must not be able to change the code that reviews it. | ||
| ref: review-v1.4.0 |
There was a problem hiding this comment.
suggestion (non-blocking): Bumping ref: to review-v1.4.0 here is correct, but the shared source at the review-v1.4.0 tag still self-pins its lib checkout to review-v1.2.2 (git show review-v1.4.0:workflows/review/review.md), so any other repo installing v1.4.0 unmodified runs the v1.4.0 prompt against the v1.2.2 lib. Worth having the release pipeline stamp this embedded ref: to the tag being cut, so the local override isn't needed on every consumer.
| # agent job fails at startup (observed on the first run of PR #241). Restore the block | ||
| # below verbatim once the two secrets exist in this repo. | ||
| # | ||
| # observability: |
There was a problem hiding this comment.
suggestion (non-blocking): Disabling the block here is the right call for this repo, but the underlying issue — a missing GH_AW_OTEL_SENTRY_ENDPOINT being a hard startup failure rather than a graceful degrade — lives in the shared source, so the next consumer without the Sentry secrets hits the same opaque failure. Consider documenting the two secrets as an install prerequisite upstream, or making OTLP export skip when the endpoint is empty.
There was a problem hiding this comment.
Documented upstream in #254: the two secrets are now in the README's required-secrets section, and the comment on the observability: block itself carries the same warning plus the workaround (comment the block out as a preserved local edit and recompile). A skip-when-empty would need gh-aw support, so prose for now.
| # (-1) so reviews are never skipped on a busy PR day; the per-run cap below | ||
| # still bounds the cost of any single review. | ||
| max-daily-ai-credits: -1 | ||
| source: Khan/actions/workflows/review/review.md@review-v1.4.0 |
There was a problem hiding this comment.
note (non-blocking): This install now carries two coupled pins — ref: (line 227) and source: here — plus hand-applied fork-guard and observability overrides, with nothing in CI verifying they stay in lockstep across a future gh aw update. A cheap assertion (that source: ref equals the lib checkout ref:, and that the file diffs against git show <tag>:workflows/review/review.md only in the allowlisted override hunks) would catch a drifted or under-guarded update.
There was a problem hiding this comment.
Addressed the cheap half in 13f143e: .github/workflows/review-pins.test.ts fails any PR where the source: ref and the lib checkout ref: in the installed file differ (the shared source's own ref is covered separately by #242's sync + backstop). Skipped the fuller check (diffing the installed file against the tagged source with an override-hunk allowlist); it needs release tags fetched in CI and feels disproportionate for now.
| # are re-raised after the broad docs rule (last match wins). | ||
| **/*.md tier=trivial | ||
| .changeset/*.md tier=trivial | ||
| workflows/review/review.md tier=high |
There was a problem hiding this comment.
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.
Done in f3b902f: workflows/*/*.md is 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 against lib/routing-config.ts: zero warnings, tiers unchanged for every currently checked-in path.
| @@ -0,0 +1,131 @@ | |||
| <!-- | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
First release cut with the ref sync in place, so the imported file's own ref line now matches the source tag; the local pin comment stays as rationale (comment only) since the reviewer must run the released lib, not the PR head. Recompiled review.lock.yml.
…gh risk Review feedback: the high-tier rule named workflows/review/review.md specifically, but future shared workflows will have prompt files of their own. Match workflows/*/*.md and drop the co-located README/CHANGELOG back to trivial; single-star segments stay out of subdirectories, so eval/ is not re-raised. Verified against lib/routing-config.ts: zero warnings, tiers unchanged for every currently-checked-in path.
…ckstep Review feedback: the install carries two coupled pins (the source: ref and the gh-aw-review-lib checkout ref:) with nothing verifying they move together across a future gh aw update or hand edit. This backstop fails any PR where they differ. The suggested fuller check (diffing the file against the tagged source with an override-hunk allowlist) needs release tags fetched in CI and is skipped as disproportionate.
…apped run The reviewer's run on the previous push computed an APPROVE verdict with two confirmed non-blocking findings, then died posting them at the per-run AI-credit cap (1024/1000; the budget-discipline fix is upstream in the review stack). Recovered from the run log and applied: - ROUTING left .github/aw/review/** at trivial/default via the docs rule, yet config.md carries the add-reviewer allowlist and bot token, and ROUTING itself sets the tiers. Re-raised the directory to tier=high. - review-pins.test.ts read only review.md, but review.lock.yml bakes in its own copies of the ref/source pins and .gitattributes marks it merge=ours, so a pin bump could go green while a stale lock keeps executing the old release. The test now checks every review-v literal in both files against the source: ref.
… webapp With nearly every file here tier=high, a full review of even a modest PR sits right at the 1000-credit gh-aw default: two runs on this PR died at 1001 and 1024 credits after computing an APPROVE verdict but before posting it. Webapp measured the same truncation and runs 2500; adopt the same bound ($25/run) as a marked local override, recompiled with gh aw compile v0.81.6 (cap change only; body hash unchanged).
2 participants
Summary
Enables the shared PR reviewer (
workflows/review/review.md) on this repo itself, pinned toreview-v1.4.1. Unlike webapp's/reviewslash-command setup, this install keeps the shared prompt's automaticpull_requesttrigger: every non-draft PR push gets reviewed, with theskip-ai-reviewlabel (created, matching webapp's) as the per-PR opt-out.Installed workflow (
review.md/review.lock.yml)gh aw addrefuses same-repo installs, so the prompt is copied from thereview-v1.4.1tag with asource:pin recorded, then compiled withgh aw compile(v0.81.6). Local overrides on top of the shared source, all markedKHAN/ACTIONS LOCAL OVERRIDEinline:github.event.pull_request.head.repo.full_name == github.repositoryprepended to the job-levelif:. This repo is public (webapp is private); with the shared prompt'sroles: alldisabling gh-aw's own actor gate, this condition is what keeps untrusted fork heads from triggering the workflow. Same-repo branches require write access to create.ref:line now matches the release (review: keep the pinned review.md ref in sync with the released version #242/Version Packages #253 wired the sync into the release flow), so no hand-bump is needed; the inline comment keeps the self-hosting rationale that the reviewer runs the released lib, not the PR head; a PR must not be able to change the code that reviews it.Consumer config (
.github/aw/review/)config.md:add-reviewerallowlisted togithub-actions(the sole owning team per.github/REVIEWERS),max: 2, usingKHAN_ACTIONS_BOT_TOKEN.risk-classification.md: written for this repo's supply-chain blast radius; the composite actions and shared workflows execute inside consuming repos' CI with their credentials. High tier:actions/**, the publish pipeline (utils/,release.yml),.github/workflows/**, and the reviewer prompt + lib. Deep-checks cover template injection inaction.ymlrun:blocks, action SHA-pinning, publish/tag integrity, gh-aw frontmatter security surfaces,.md/.lock.ymlpairing, and semver-as-behavior-contract onreviewreleases.ci-tooling.md: what node-ci already catches (eslint/prettier, tsc, vitest, the action smoke tests, changeset presence) so the reviewer stays silent on those.skills.md: this repo has no.claude/skills/; the catalog points the skill-auditor at the two real convention docs (rootREADME.mdfor the release/changeset flow,workflows/review/README.mdfor the reviewer contract) and forbids flagging conventions from memory.ROUTING: enables the same eval-justified whole-change roster as webapp (holistic,completeness,test-adequacy,first-principles,conventions); no specialist lenses, since all eleven target webapp's backend domains. Tier rules verified against the lib parser (lib/routing-config.ts): parses with zero warnings.Supporting changes
.gitattributes:gh aw compileitself added.github/workflows/*.lock.yml linguist-generated=true merge=ours.node-ci.yml:.gitattributesadded to thecheck-for-changesetexclude list, alongside the lint configs already there; metadata changes should not force cutting a release. No changeset on this PR for the same reason (everything else is under.github/, already excluded).Security review note (gh-aw compile)
ANTHROPIC_API_KEY,GH_AW_GITHUB_MCP_SERVER_TOKEN,GH_AW_GITHUB_TOKEN,GH_AW_OTEL_SENTRY_AUTHORIZATION,GH_AW_OTEL_SENTRY_ENDPOINT,GITHUB_TOKEN,KHAN_ACTIONS_BOT_TOKEN; the identical set webapp's productionreview.lock.ymluses. Availability here:ANTHROPIC_API_KEYexists at repo level (review-eval-full already uses it);KHAN_ACTIONS_BOT_TOKENis org-level (gerald-pr and release already use it); the MCP tokens fall back through|| secrets.GITHUB_TOKENchains, so their absence is fine. The twoGH_AW_OTEL_SENTRY_*secrets are not configured in this repo, and that turned out to be a hard failure, not a graceful degrade: the MCP gateway's OTLP config schema rejects the empty endpoint URL and the agent job dies at startup (observed on this PR's first run). Theobservability:block is therefore disabled as a marked local override, removing both secrets from the lock's manifest; restore it verbatim once the two secrets exist here.actions/checkout,cache,github-script,setup-node,upload/download-artifact,github/gh-aw-actions/setup@v0.81.6); no actions new to Khan's review workflows.contents: read+pull-requests: readat the job; all writes go through gh-aw safe outputs with the caps from the shared v1.4.1 prompt (reviewed in review: require a concrete failure_scenario on every finding #226/review: enforce the change-provenance gate in code; stage generated-stripped diffs #227/review: prompt disciplines and measured cost fixes #228/review: add allowed-domains allowlist for safe-output sanitization #231). The only trigger-surface change relative to webapp is the fork guard described above, which tightens, not widens.Testing
gh aw compileclean (0 errors, 0 warnings); the lock carries the fork guard and thegithub-actionsallowlist (verified by inspection of the compiledif:and safe-output handler config).ROUTINGparsed with the repo's ownworkflows/review/lib/routing-config.ts: expected tiers/roster,warnings: [].pull_requestworkflow executes from the merge ref, so a PR that adds one exercises it). The first push's activation failed becauserisk-classification.mdquoted literal template-expression syntax in its injection deep-check; gh-aw validates runtime imports and rejects that. Rewritten in prose. The second run then failed at agent startup on the missing OTLP secrets (see the security note); the observability block is now disabled locally. The run on the current push is the live smoke test.