review: honour Gerald .github/NOTIFIED in the reviewer - #264
Merged
Conversation
…ub/NOTIFIED in the reviewer Add `.github/NOTIFIED` handling so the reviewer pings the people and teams a repo has asked to be notified about matching changes, distinct from the `.github/REVIEWERS` reviewer ownership the router already handles. New `workflows/review/lib/notified.ts` (+ tests) parses the file's `[ON PULL REQUEST]` notify rules (ignoring the docs above the `----...ignored!----` marker and the `[ON PUSH WITHOUT PULL REQUEST]` section) and matches them against the PR: path globs against the changed-file list, quoted `"/body/flags"` diff regexes against each file's staged unified diff (so a rule can fire on added content). It groups matches by `@mention`, carries each rule's label, and renders a ready-to-insert `### Notified` block. Matching is a self-contained micromatch subset anchored at the repo root, keeping the lib dependency-free (it runs under `npx tsx` with no install); an unsupported construct or malformed rule degrades to no match plus a `Note:` on the PR, never a crashed review. review.md Step 7 runs the CLI and pastes the block into the Review Guidance comment on approval, telling each matched person the label and which changed files matched; notifications also feed the comment's post trigger and its `risksPatternsKey` idempotency signature, so a repost re-pings only when the match set changes. Most of the logic lives in the script; the prompt just runs it and inserts the result. Ships only in the shared source + lib + changeset (per the review-feature convention); the release-pinned consumer copy and lock are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 56c407f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
khan-actions-bot
requested review from
a team,
jaredly and
somewhatabstract
and removed request for
a team
July 16, 2026 20:18
Contributor
Review Guidancegithub-actions (2 files)
Excluded from review (2 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…viewer feedback (parse fixes + honest docs)
- Fix a section-header false positive: an *unlabeled* rule whose glob leads
with a character class (`[Dd]ockerfile`, `[1-5]`, `[[:digit:]]`) was matched
by the `startsWith("[")` header guard, silently dropping that rule and every
rule after it in `[ON PULL REQUEST]`. Match only genuine all-caps `[SECTION]`
headers now (`SECTION_HEADER_RE`).
- Fix a silent drop: a quoted diff-regex rule with an invalid body compiled to
null and was dropped at match time with no warning, contradicting the "a
malformed rule surfaces a Note:" promise. Validate the regex at parse time so
it flows through the existing `warnings[]` -> Step 7 `Note:` path.
- Tests: the char-class-header case (rule + trailing rules survive), a leading
char-class matchGlob, and the malformed-regex warning end-to-end through
parseNotified and computeNotifiedResult.
- Docs: describe the glob dialect honestly as a practical subset of Gerald's
micromatch (wildcards match dotfiles; `!(...)` unsupported), and document the
approval-time delivery model and Gerald coexistence in the README.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kevinb-khan
approved these changes
Jul 21, 2026
kevinb-khan
left a comment
kevinb-khan
left a comment
Contributor
There was a problem hiding this comment.
The changes seem reasonable to me. I tried to focus on reviewing the high-level logic in notified.ts and review.md as well as all of the tests in notified.test.ts.
I like a lot of the logic for this change to the reviewer was encoded using a formal programming language as opposed to trying to do everything within the review.md.
I think there's likely more logic within the review.md that we could likely extract into .ts files. This something to think about for future iterations on the review. I see a couple of potential benefits of having more of the logic living in .ts files:
- it can be unit tested
- its deterministic
- it's easier for humans, or at least myself, to review (I find that reviewing large blocks of logic encoded in English takes longer to parse especially when the sentences include a lot of clauses and parentheticals which is often how LLMs tend to write this kind of stuff).
… review feedback (tests + docs) Addresses the follow-up review on #264 (all non-blocking): - notified.ts: import `./diff` without the `.ts` extension, matching the module's other importers (provenance.ts, rereview-mode.ts). - notified.test.ts: cover the `+(...) *(...) @(...)` extglob branches and invalid globs; add a case proving `@mentions` inside a trailing `#` comment are ignored; split the combined glob and render/signature describe blocks; drive the glob matcher with `it.each`; and snapshot the rendered `### Notified` block and the full `runCli` result with `toMatchInlineSnapshot` so the shape review.md consumes is visible at a glance. - review.md: render the notified.json field descriptions as a bulleted list and break the long "Notified" rendering instruction into nested bullets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… gaps + fix repost-wording (review feedback) Follow-up review on #264 (all non-blocking): - notified.test.ts: add matchGlob rows for the trailing-slash directory glob (`deploy/`) and for negated character classes (`[!a-z]`, `[^abc]`) — both branches of the glob compiler were previously unexercised. - notified.ts: correct the delivery-note wording. It claimed review.md re-posts "only when the notification signature changes"; in fact Step 7 folds that signature into the combined Review Guidance key, so a repost (and its re-pings) fires when any component — risk files, patterns, exclusions, or the notification set — changes. Reworded to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Teaches the AI PR reviewer to honour a repo's Gerald
.github/NOTIFIEDfile: when a PR touches files (or introduces diff content) that someone asked to be notified about, the reviewer adds a### Notifiedsection to its Review Guidance comment that@-mentions them and tells them the rule label and which changed files matched. This is the notify half of Gerald (the router already handles the reviewer-ownership half via.github/REVIEWERS).What changed
workflows/review/lib/notified.ts(new, +notified.test.ts, 34 tests) — parses.github/NOTIFIEDand does the matching, following the same shape asrouter.ts/provenance.ts(pure functions + an fs-injectedrunCli). It:[ON PULL REQUEST]section (discards the docs above the----…ignored!----marker and the[ON PUSH WITHOUT PULL REQUEST]section);label:prefixes,#comments, path globs (matched against changed-file paths) and quoted"/body/flags"diff regexes (matched against each file's staged unified diff, so a rule fires on added content);@mention, keeps each rule's label, and renders a ready-to-insert### Notifiedblock plus a stablesignature.workflows/review/review.md— minimal Step 7 wiring: run the CLI, paste itsmarkdownverbatim into the Review Guidance comment, add notifications as a post trigger, and fold the notificationsignatureinto therisksPatternsKeyidempotency key (Steps 7 & 9). Almost all logic lives in the script; the prompt just runs it and inserts the result.workflows/review/README.md— documents the.github/NOTIFIEDsupport..changeset/review-notified.md—reviewminor bump.Design notes
npx -y tsxwith no install step, so the glob matcher is self-contained — a micromatch subset (**,*,?,{a,b,c},[…],(a|b),?(…) *(…) +(…) @(…)) anchored at the repo root (Gerald/micromatch semantics, not the router's basename-anywhere gitattributes convention). An unsupported construct or malformed rule degrades to no match plus aNote:on the PR review — never a crashed review..github/NOTIFIEDis read fromREVIEW_REPO_ROOT(the base-branch copy, like the router reads.github/REVIEWERS), so a PR can't inject notify rules that fire before it merges.gh-aw's safe-output sanitizer neutralizes
@mentions(@user→`@user`) unless they're allow-listed. The defaultmentions.allow-team-members: truealready lets repo-collaborator users through, so individual folks are pinged — but team mentions (e.g.@Khan/infra-platform) and non-collaborators are shown-but-not-pinged unless the workflow adds asafe-outputs.mentions:block (allowed-teamsneeds a token withread:org). Since the targets are dynamic and this is a shared-workflow + token-scope decision, I left that config out of this PR. Happy to add it as a follow-up if we want teams pinged.Scope
Per the review-feature convention (cf. #262, #225), this ships in the shared source + lib + changeset only. The release-pinned consumer copy (
.github/workflows/review.md) and compiled lock are intentionally untouched — they pick this up on the next release + re-pin.Verification
notified.tsexercised end-to-end exactly as the workflow invokes it, against a webapp-styleNOTIFIED+ staged diff: correctly matched a team via both adeploy/**glob and a model diff-regex, and a user via a*.graphqlglob; correctly ignored the push-only rule and above-marker docs; the diff-regex fired on the added line, not the+++header.prettier --checkclean,tsc --noEmitclean,eslintclean.🤖 Generated with Claude Code