diff --git a/.changeset/review-lens-payload-seam.md b/.changeset/review-lens-payload-seam.md new file mode 100644 index 00000000..1f994464 --- /dev/null +++ b/.changeset/review-lens-payload-seam.md @@ -0,0 +1,30 @@ +--- +"review": minor +--- + +Add the per-lens consumer payload seam. A consuming repo may now define +`.github/aw/review/lenses/.md` for any of the eleven specialist lenses, plus +`lenses/correctness.md` for the always-on `correctness-reviewer`; each file is +runtime-imported (optional form) into a new "Repo-specific rules and hunts" section +of the matching reviewer prompt, carrying that repo's surface-specific rules and +extra tri-state hunts. Lens names stay generic and shared; only payloads vary per +repo. Behavior-neutral for every current consumer: no consumer carries a payload +file yet, and a missing optional import inlines nothing at runtime. +`correctness-checks.md` remains imported as a deprecated alias for +`lenses/correctness.md` (frontend carries one today); repos should migrate the file +and carry at most one of the two, and the alias is removed in the next major +release. Payloads are additive by contract: the lens prompts state that payload +rules never relax or override the shared rules, which win on any conflict. The +router now warns (through `routingConfig.warnings`, surfaced in the review body's +note lines) when a payload would be silently inert: a filename matching no imported +payload, a specialist payload no ROUTING rule routes, the correctness alias carried +alongside its replacement, the alias carried at all (a deprecation nudge ahead of +its removal), or a `lenses` path that is not a readable directory (which degrades +to a warning instead of crashing the router CLI). The eval's import resolution now matches +production for the optional form (missing resolves to empty, not the "(not +configured for this eval case)" note), so corpus case trees can carry payloads; the +required-form fallback note is unchanged. README documents the new surface and the +three-way contribution rule (shared skeleton vs lens payload vs skills) and fixes +two stale claims in the consumer-config section (the undocumented +`correctness-checks.md`, and the assertion that the optional import form was +dropped). diff --git a/workflows/review/README.md b/workflows/review/README.md index 678e98ca..273c98d5 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -111,14 +111,18 @@ locally at compile/run time, not from this repo). Create them under | `ci-tooling.md` | **Required** | The lint/format/type/test issues your CI already catches. Imported into `correctness-reviewer` so it doesn't flag them, and into `claim-validator` so it drops any correctness claim that flags a CI-caught issue. | | `skills.md` | **Required** | The catalog of best-practice skill files (and when each applies). Imported into `skill-auditor` to evaluate the diff against, and into `claim-validator` so it can verify a flagged skill violation against the skill's actual rule. | | `ROUTING` | Optional | The machine-readable path map the deterministic router reads (see below). Without it the router spawns no specialist lenses and floors the run budget, and the review notes the missing config on the PR. | +| `lenses/.md` | Optional | Per-lens payloads: your repo's surface-specific review rules and extra hunts, imported into the matching reviewer (see [Per-lens payloads](#per-lens-payloads-lenseslensmd)). Absent files import nothing. | +| `correctness-checks.md` | Deprecated | Alias for `lenses/correctness.md`; still imported for compatibility, and removed in the next major release. Carry only one: when both exist, both are imported (duplicating the checks) and the router warns; an alias carried alone gets a deprecation note on each review. | -All four are **required**, but validated at different times. `config.md` is a +The first four are **required**, but validated at different times. `config.md` is a frontmatter import, embedded and checked at **compile time** — `gh aw compile` fails if it's missing. The other three are `{{#runtime-import}}` body imports inside the sub-agent prompts; they resolve when the workflow **runs**, so a missing one surfaces as a `Runtime import file not found` failure on the next PR — not at compile time. The -optional `{{#runtime-import? … }}` form was dropped either way, so a missing config -fails loudly rather than silently degrading the review. +required configs deliberately avoid the optional `{{#runtime-import? … }}` form, so a +missing one fails loudly rather than silently degrading the review. The lens payloads +(and the deprecated `correctness-checks.md`) use the optional form: a repo that +defines none is valid, and a missing payload file imports nothing. These imported snippets are plain Markdown — they must not contain `${{ }}` expressions (gh-aw rejects those inside imports). `add-reviewer` lives @@ -129,6 +133,52 @@ Repo-specific frontmatter that imports can't merge (e.g. an `if:` condition to s deploy/automation branches or forks) goes directly in your installed `review.md` as a local edit; `gh aw update` preserves it. +### Per-lens payloads (`lenses/.md`) + +A consuming repo may define surface-specific review rules and extra hunts for any +reviewer lens by adding `.github/aw/review/lenses/.md`. Each file is imported +at runtime into the matching reviewer prompt, in a "Repo-specific rules and hunts" +section the reviewer treats exactly like its built-in rules and tri-state hunts. All +imports are optional: a repo with no `lenses/` directory gets exactly the shared +behavior, and lens names never vary per repo; only their payloads do. + +Valid names are the eleven specialist lenses (`security-auth`, +`ai-safety-moderation`, `mass-comms-coppa`, `caching-resource`, `data-migrations`, +`concurrency-async`, `api-federation-compat`, `cross-deploy-serialization`, +`deploy-infra-config`, `money-payments`, `content-i18n`) plus `correctness`, which +feeds the always-on `correctness-reviewer`. `lenses/correctness.md` supersedes the +older `correctness-checks.md` (still imported as a deprecated alias until the next +major release; carry at most one of the two). A payload only reaches a specialist +lens on PRs where the router actually spawns that lens, so a payload without +matching `ROUTING` `lens=` rules is inert. The router warns in the review body's +note lines when a payload would be silently inert: a filename that matches no +imported payload, a specialist payload no `ROUTING` rule routes, the correctness +alias carried alongside its replacement (or, as a deprecation nudge, carried at +all), or a `lenses` path that is not a readable directory. + +Payloads are **additive**: they extend the lens's shared rules and hunts but never +relax or override them, and the shared rules win on any conflict (the lens prompts +state this next to the import). A payload cannot whitelist a defect or lower the +evidence bar; it can only add repo-specific things to check. + +**Where a rule belongs** (the three-way contribution rule): + +- **Shared skeleton** (this repo's `review.md`): rules that hold for every consumer + on every stack. If a rule needs stack-specific phrasing ("Datastore query", + "DOM sink"), it does not belong here; keep only its surface-neutral core. +- **Lens payload** (your repo's `lenses/.md`): rules and hunts specific to + your repo's surface. Server repos carry server-surface checks (query bounds, + datastore idioms); client repos carry client-surface checks (DOM XSS sinks, token + storage, postMessage origins). +- **Skills** (your repo's `skills.md` catalog): house conventions and sanctioned + fixes, audited by `skill-auditor` rather than baked into a lens. + +Payloads are model-facing prose owned by the consuming repo's engineers, like +`risk-classification.md` and the skills catalog before them; the repo's normal code +review is the bar for editing them. Note that a payload changes reviewer behavior +without touching Khan/actions: when in doubt about a large payload change, ask the +workflow maintainers for an eval run over payload-carrying corpus cases. + ### Per-directory `REVIEW.md` contracts (optional) Separately from `.github/aw/review/`, a consuming repo may carry `REVIEW.md` files in diff --git a/workflows/review/eval/live-producer.test.ts b/workflows/review/eval/live-producer.test.ts index 0b918e2c..75a2b2f9 100644 --- a/workflows/review/eval/live-producer.test.ts +++ b/workflows/review/eval/live-producer.test.ts @@ -360,14 +360,14 @@ describe("produceLive", () => { }); describe("resolveRuntimeImports", () => { - it("inlines imports present in the checkout and notes absent ones", () => { + it("inlines imports present in the checkout and notes absent required ones", () => { const vol = Volume.fromJSON({ "/checkout/.github/aw/review/skills.md": "## skills index", }); const fs = volFs(vol); const prompt = [ "Skills:\n{{#runtime-import .github/aw/review/skills.md}}", - "CI:\n{{#runtime-import? .github/aw/review/ci-tooling.md}}", + "CI:\n{{#runtime-import .github/aw/review/ci-tooling.md}}", ].join("\n"); const resolved = resolveRuntimeImports(prompt, "/checkout", fs); expect(resolved).toContain("## skills index"); @@ -375,6 +375,23 @@ describe("resolveRuntimeImports", () => { expect(resolved).not.toMatch(/runtime-import/); }); + it("resolves a missing optional import to nothing, like production", () => { + const vol = Volume.fromJSON({ + "/checkout/.github/aw/review/lenses/security-auth.md": + "## repo security payload", + }); + const fs = volFs(vol); + const prompt = [ + "Payload:\n{{#runtime-import? .github/aw/review/lenses/security-auth.md}}", + "Absent:\n{{#runtime-import? .github/aw/review/lenses/money-payments.md}}", + ].join("\n"); + const resolved = resolveRuntimeImports(prompt, "/checkout", fs); + expect(resolved).toContain("## repo security payload"); + expect(resolved).toContain("Absent:\n"); + expect(resolved).not.toContain("(not configured for this eval case)"); + expect(resolved).not.toMatch(/runtime-import/); + }); + it("reaches the dispatched prompts through produceLive", async () => { const {runner, requests} = scriptedRunner({ "correctness-reviewer": [JSON.stringify({findings: []})], diff --git a/workflows/review/eval/live-producer.ts b/workflows/review/eval/live-producer.ts index 4fbf4f6a..c9499850 100644 --- a/workflows/review/eval/live-producer.ts +++ b/workflows/review/eval/live-producer.ts @@ -18,8 +18,11 @@ * router's `lensesToSpawn`. * - `{{#runtime-import }}` directives are compile-time inlines of * consumer-repo files. Here they resolve against the case's checkout tree - * when the file exists there, else to a fixed "not configured" note, so a - * case can opt into a skills index by carrying the file in its tree. + * when the file exists there, so a case can opt into a skills index or a + * lens payload by carrying the file in its tree. A missing optional + * import (`{{#runtime-import? …}}`) resolves to empty exactly as in + * production; a missing required one resolves to a fixed "not + * configured" note where production would fail the run. * - The investigation-cap CLI the prompts invoke is not staged; sub-agents * run with read-only tools and treat the unavailable cap as a denied * budget (the prompt's own fallback: stop investigating, report what you @@ -201,26 +204,35 @@ const parseReconciliation = (output: string): LiveReconciliation => { /* Prompt resolution */ /* -------------------------------------------------------------------------- */ -const RUNTIME_IMPORT = /\{\{#runtime-import\??\s+([^}\s]+)\s*\}\}/g; +const RUNTIME_IMPORT = /\{\{#runtime-import(\?)?\s+([^}\s]+)\s*\}\}/g; const IMPORT_FALLBACK = "(not configured for this eval case)"; /** * Inline `{{#runtime-import }}` directives from the case's checkout - * tree, falling back to a fixed note when the tree does not carry the file. - * Exported for the A/B runner's reporting (which imports resolved per case). + * tree. A missing OPTIONAL import (`{{#runtime-import? …}}`) resolves to the + * empty string, matching production (gh-aw's runtime_import.cjs warns and + * inlines nothing), so an absent lens payload is behavior-identical to + * production. A missing REQUIRED import falls back to a fixed note; this is + * the one deliberate deviation (production fails the run), so cases need not + * carry every consumer config file. Exported for the A/B runner's reporting + * (which imports resolved per case). */ export const resolveRuntimeImports = ( prompt: string, checkoutDir: string, fs: Pick, ): string => - prompt.replace(RUNTIME_IMPORT, (_match, importPath: string) => { - const full = `${checkoutDir}/${importPath}`; - return fs.existsSync(full) - ? fs.readFileSync(full, "utf8") - : IMPORT_FALLBACK; - }); + prompt.replace( + RUNTIME_IMPORT, + (_match, optional: string | undefined, importPath: string) => { + const full = `${checkoutDir}/${importPath}`; + if (fs.existsSync(full)) { + return fs.readFileSync(full, "utf8"); + } + return optional ? "" : IMPORT_FALLBACK; + }, + ); /* -------------------------------------------------------------------------- */ /* Output parsing: the three sub-agent contracts -> RecordedFinding */ diff --git a/workflows/review/lib/disciplines.test.ts b/workflows/review/lib/disciplines.test.ts index f9b153f8..d604ea71 100644 --- a/workflows/review/lib/disciplines.test.ts +++ b/workflows/review/lib/disciplines.test.ts @@ -163,6 +163,11 @@ describe("each specialist lens definition", () => { expect(section).toContain( "{{#runtime-import .github/aw/review/skills.md}}", ); + // The per-lens consumer payload seam: optional, resolves to + // nothing when the host repo carries no payload file. + expect(section).toContain( + `{{#runtime-import? .github/aw/review/lenses/${lens}.md}}`, + ); expect(section).toContain(`\`lens\` is exactly \`${lens}\``); expect(section).toContain( "Domain notes for §Bounded investigation", @@ -171,6 +176,20 @@ describe("each specialist lens definition", () => { } }); +describe("the correctness payload seam", () => { + it("correctness-reviewer imports lenses/correctness.md and its alias", () => { + const section = lensSection("correctness-reviewer"); + expect(section).toContain( + "{{#runtime-import? .github/aw/review/lenses/correctness.md}}", + ); + // The deprecated alias stays imported until consumers migrate + // (frontend carries one); dropping either import is a regression. + expect(section).toContain( + "{{#runtime-import? .github/aw/review/correctness-checks.md}}", + ); + }); +}); + describe("the label-shape reviewers still carry their own disciplines", () => { // Their variants differ materially (CLI cap invocation with per-agent id // semantics, `discussion` instead of `evidence_trace`), so they were diff --git a/workflows/review/lib/lens-payloads.test.ts b/workflows/review/lib/lens-payloads.test.ts new file mode 100644 index 00000000..1b133d3f --- /dev/null +++ b/workflows/review/lib/lens-payloads.test.ts @@ -0,0 +1,97 @@ +import {describe, it, expect} from "vitest"; + +import { + CORRECTNESS_ALIAS_PATH, + LENS_PAYLOAD_DIR, + lensPayloadWarnings, +} from "./lens-payloads.ts"; +import {SPECIALIST_LENSES} from "./router.ts"; +import type {Lens} from "./finding-schema.ts"; +import type {LensRule} from "./routing-config.ts"; + +/** + * Lens-payload validation tests. The failure mode under test is silence: a + * payload file that nothing imports (typo, unknown name) or that an imported + * lens never reads (no ROUTING rule spawns it) is invisible at runtime, so + * the only surface where the author learns about it is these warnings. The + * CLI wiring (readdir + append to routingConfig.warnings) is pinned in + * router.test.ts. + */ + +const routed = (lens: Lens): LensRule[] => [ + {pattern: "src/**", lenses: [lens]}, +]; + +const warningsFor = ( + payloadFiles: readonly string[], + lensRules: readonly LensRule[] = [], + aliasPresent = false, +): string[] => + lensPayloadWarnings( + payloadFiles, + lensRules, + aliasPresent, + SPECIALIST_LENSES, + ); + +describe("lensPayloadWarnings", () => { + it("is silent for a routed specialist payload and correctness.md", () => { + expect( + warningsFor( + ["security-auth.md", "correctness.md"], + routed("security-auth"), + ), + ).toEqual([]); + }); + + it("warns on a filename matching no imported payload", () => { + const warnings = warningsFor( + ["security_auth.md", "README.md", "notes.txt"], + routed("security-auth"), + ); + expect(warnings).toHaveLength(3); + for (const warning of warnings) { + expect(warning).toContain("matches no imported payload"); + } + expect(warnings[0]).toContain(`${LENS_PAYLOAD_DIR}/security_auth.md`); + }); + + it("warns on a specialist payload no ROUTING rule routes", () => { + const warnings = warningsFor( + ["money-payments.md"], + routed("security-auth"), + ); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("lens=money-payments"); + expect(warnings[0]).toContain("inert"); + }); + + it("never flags correctness.md as unrouted (always-on reviewer)", () => { + expect(warningsFor(["correctness.md"])).toEqual([]); + }); + + it("warns when correctness.md and its deprecated alias coexist", () => { + const warnings = warningsFor(["correctness.md"], [], true); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain(CORRECTNESS_ALIAS_PATH); + }); + + it("nudges on the alias alone (deprecated, dropped next major)", () => { + const warnings = warningsFor([], [], true); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("deprecated alias"); + expect(warnings[0]).toContain(`${LENS_PAYLOAD_DIR}/correctness.md`); + }); + + it("does not double-warn when both correctness files exist", () => { + const warnings = warningsFor(["correctness.md"], [], true); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("both"); + }); + + it("accepts every specialist lens name as a payload filename", () => { + for (const lens of SPECIALIST_LENSES) { + expect(warningsFor([`${lens}.md`], routed(lens))).toEqual([]); + } + }); +}); diff --git a/workflows/review/lib/lens-payloads.ts b/workflows/review/lib/lens-payloads.ts new file mode 100644 index 00000000..fa5ba5e4 --- /dev/null +++ b/workflows/review/lib/lens-payloads.ts @@ -0,0 +1,83 @@ +/** + * Consumer lens-payload validation (the per-lens payload seam). + * + * A consuming repo may carry `.github/aw/review/lenses/.md` files; + * each is runtime-imported (optional form) into the matching reviewer + * prompt. Because the imports are optional, a payload that nothing imports + * fails SILENTLY: the file sits in the tree, the team believes its rules + * are active, and no reviewer ever reads it. This module computes + * fixed-format warnings for those cases; the router CLI appends them to + * `routingConfig.warnings`, the same channel that surfaces ROUTING parse + * warnings in the review body's note lines. + * + * Pure and dependency-light on purpose: the specialist-lens roster is a + * parameter (it is derived in `router.ts`, which imports this module, so + * importing it here would be a cycle). + */ + +import type {Lens} from "./finding-schema"; +import type {LensRule} from "./routing-config"; + +/** Where consumer lens payloads live. */ +export const LENS_PAYLOAD_DIR = ".github/aw/review/lenses"; + +/** The deprecated alias for `lenses/correctness.md`, still imported. */ +export const CORRECTNESS_ALIAS_PATH = ".github/aw/review/correctness-checks.md"; + +/** + * Warnings for lens payloads that would be silently inert: an entry in + * `lenses/` whose name matches no imported payload (a typo or an unknown + * lens), a payload for a specialist lens no ROUTING rule ever routes, the + * deprecated correctness alias carried alongside its replacement, and a + * soft deprecation nudge when the alias is carried alone (so the repo + * migrates before the alias import is dropped at the next major release). + * `correctness` is valid alongside the specialists (the always-on + * `correctness-reviewer` imports it) and is never flagged as unrouted. + */ +export const lensPayloadWarnings = ( + payloadFiles: readonly string[], + lensRules: readonly LensRule[], + correctnessAliasPresent: boolean, + specialistLenses: readonly Lens[], +): string[] => { + const warnings: string[] = []; + const validNames = new Set([...specialistLenses, "correctness"]); + const routed = new Set(lensRules.flatMap((rule) => rule.lenses)); + for (const file of payloadFiles) { + const name = file.endsWith(".md") ? file.slice(0, -3) : file; + if (!validNames.has(name) || !file.endsWith(".md")) { + warnings.push( + `lens payload ${LENS_PAYLOAD_DIR}/${file} matches no ` + + `imported payload (valid names: .md ` + + `or correctness.md); no reviewer will read it`, + ); + } else if (name !== "correctness" && !routed.has(name as Lens)) { + warnings.push( + `lens payload ${LENS_PAYLOAD_DIR}/${file} is inert: no ` + + `ROUTING rule routes lens=${name}, so that lens never ` + + `spawns in this repo`, + ); + } + } + if (correctnessAliasPresent) { + if (payloadFiles.includes("correctness.md")) { + warnings.push( + `both ${LENS_PAYLOAD_DIR}/correctness.md and its deprecated ` + + `alias ${CORRECTNESS_ALIAS_PATH} exist; both are ` + + `imported, duplicating the correctness checks -- carry ` + + `only ${LENS_PAYLOAD_DIR}/correctness.md`, + ); + } else { + // The soft forcing function before the alias cliff: when the + // alias import is dropped at the next major, an unmigrated repo + // would go inert with no signal, so nudge on every review until + // the file moves. + warnings.push( + `${CORRECTNESS_ALIAS_PATH} is a deprecated alias; rename it ` + + `to ${LENS_PAYLOAD_DIR}/correctness.md (the alias ` + + `import is removed in the reviewer's next major release)`, + ); + } + } + return warnings; +}; diff --git a/workflows/review/lib/router.test.ts b/workflows/review/lib/router.test.ts index 5e053176..8aebd17c 100644 --- a/workflows/review/lib/router.test.ts +++ b/workflows/review/lib/router.test.ts @@ -7,6 +7,7 @@ import { DEFAULT_TIER_BUDGETS, ENABLEABLE_REVIEWERS, isGenerated, + LENS_PAYLOAD_DIR, matchesGlob, parseGitattributesGenerated, parseReviewers, @@ -576,8 +577,10 @@ const GITATTRIBUTES_PATH = ".gitattributes"; const REVIEWERS_PATH = ".github/REVIEWERS"; // A structural stand-in for the node:fs subset runCli injects: existsSync + -// readFileSync answer from the supplied map, writes are recorded, and mkdir -// calls are captured -- no real filesystem is touched. +// readFileSync answer from the supplied map (existsSync also answers true for +// a directory some key lives under, and readdirSync lists a directory's +// immediate entries), writes are recorded, and mkdir calls are captured -- no +// real filesystem is touched. const fakeFs = (inputs: Record) => { const written: Record = {}; const mkdirCalls: string[] = []; @@ -592,10 +595,27 @@ const fakeFs = (inputs: Record) => { writeFileSync: (p: string, data: string): void => { written[p] = data; }, - existsSync: (p: string): boolean => p in inputs, + existsSync: (p: string): boolean => + p in inputs || + Object.keys(inputs).some((key) => key.startsWith(`${p}/`)), mkdirSync: (p: string, _opts: {recursive: boolean}): void => { mkdirCalls.push(p); }, + readdirSync: (p: string): string[] => { + if (p in inputs) { + // Mirror node:fs — reading a regular file as a directory + // throws ENOTDIR. + throw new Error(`ENOTDIR: not a directory, scandir '${p}'`); + } + const prefix = p.endsWith("/") ? p : `${p}/`; + const names = new Set(); + for (const key of Object.keys(inputs)) { + if (key.startsWith(prefix)) { + names.add(key.slice(prefix.length).split("/")[0]); + } + } + return [...names]; + }, }; return {fs, written, mkdirCalls}; }; @@ -900,3 +920,69 @@ describe("runCli: re-review mode", () => { expect(runCli(fs).reReviewMode).toBe("full"); }); }); + +// Unit tests for lensPayloadWarnings live in lens-payloads.test.ts; here we +// pin only the CLI wiring (readdir the payload dir, append to warnings). +describe("runCli: lens payload warnings", () => { + it("appends payload warnings to routingConfig.warnings", () => { + const {fs} = fakeFs({ + ["/tmp/gh-aw/review/files.json"]: JSON.stringify([ + {path: "a.ts", status: "modified"}, + ]), + [ROUTING_CONFIG_PATH]: "src/** lens=security-auth", + [`${LENS_PAYLOAD_DIR}/security-auth.md`]: "- repo rule", + [`${LENS_PAYLOAD_DIR}/data-migrations.md`]: "- inert rule", + }); + const json = runCli(fs); + expect(json.routingConfig.present).toBe(true); + expect(json.routingConfig.warnings).toHaveLength(1); + expect(json.routingConfig.warnings[0]).toContain( + "lens=data-migrations", + ); + }); + + it("emits no payload warnings when lenses/ is absent", () => { + const {fs} = fakeFs({ + ["/tmp/gh-aw/review/files.json"]: JSON.stringify([ + {path: "a.ts", status: "modified"}, + ]), + [ROUTING_CONFIG_PATH]: "src/** lens=security-auth", + }); + expect(runCli(fs).routingConfig.warnings).toEqual([]); + }); + + it("wires the alias check: both correctness files warn through runCli", () => { + const {fs} = fakeFs({ + ["/tmp/gh-aw/review/files.json"]: JSON.stringify([ + {path: "a.ts", status: "modified"}, + ]), + [ROUTING_CONFIG_PATH]: "src/** lens=security-auth", + [`${LENS_PAYLOAD_DIR}/correctness.md`]: "- new home", + [".github/aw/review/correctness-checks.md"]: "- old home", + }); + const warnings = runCli(fs).routingConfig.warnings; + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("both"); + expect(warnings[0]).toContain("correctness-checks.md"); + }); + + it("degrades to a warning when the payload dir is a regular file", () => { + const {fs, written} = fakeFs({ + ["/tmp/gh-aw/review/files.json"]: JSON.stringify([ + {path: "a.ts", status: "modified"}, + ]), + [ROUTING_CONFIG_PATH]: "src/** lens=security-auth", + // The payload dir path itself is a file: readdirSync throws + // ENOTDIR, which must not crash the CLI before routing.json. + [LENS_PAYLOAD_DIR]: "oops, a file", + }); + const json = runCli(fs); + expect(json.routingConfig.warnings).toHaveLength(1); + expect(json.routingConfig.warnings[0]).toContain( + "not a readable directory", + ); + expect(JSON.parse(written["/tmp/gh-aw/review/routing.json"])).toEqual( + json, + ); + }); +}); diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index 6c4bfdb9..1a8238d6 100644 --- a/workflows/review/lib/router.ts +++ b/workflows/review/lib/router.ts @@ -34,6 +34,11 @@ import {DEFAULT_MISROUTED_FLOOR_TIER, DEFAULT_TIER_BUDGETS} from "./budgets"; import {clampBudgetToCreditCap, resolveCreditCap} from "./credit-cap"; import {KNOWN_LENSES} from "./finding-schema"; import type {Lens} from "./finding-schema"; +import { + CORRECTNESS_ALIAS_PATH, + LENS_PAYLOAD_DIR, + lensPayloadWarnings, +} from "./lens-payloads"; import { DEFAULT_RE_REVIEW_MODE, ENABLEABLE_REVIEWERS, @@ -62,6 +67,7 @@ export { RISK_TIERS, ROUTING_CONFIG_PATH, }; +export {CORRECTNESS_ALIAS_PATH, LENS_PAYLOAD_DIR, lensPayloadWarnings}; export type { EnableableReviewer, LensRule, @@ -862,6 +868,7 @@ type FsLike = { writeFileSync: (p: string, data: string) => void; existsSync: (p: string) => boolean; mkdirSync: (p: string, opts: {recursive: boolean}) => void; + readdirSync: (p: string) => string[]; }; /** @@ -923,6 +930,34 @@ export const runCli = ( ], }; + // Lens payloads that would be silently inert are worth a PR note (a + // missing optional import inlines nothing at runtime). existsSync is + // also true for a regular file at the payload dir path; that must + // degrade to a warning, not an ENOTDIR crash before routing.json. + const lensesDir = repoPath(LENS_PAYLOAD_DIR); + let payloadFiles: string[] = []; + let payloadDirWarning: string[] = []; + if (fs.existsSync(lensesDir)) { + try { + payloadFiles = fs.readdirSync(lensesDir); + } catch { + payloadDirWarning = [ + `${LENS_PAYLOAD_DIR} exists but is not a readable ` + + `directory; lens payloads were not checked and none ` + + `will be imported`, + ]; + } + } + const payloadWarnings = [ + ...payloadDirWarning, + ...lensPayloadWarnings( + payloadFiles, + routingFileConfig.lensRules, + fs.existsSync(repoPath(CORRECTNESS_ALIAS_PATH)), + SPECIALIST_LENSES, + ), + ]; + const input: RouteInput = {files}; if (fs.existsSync(RESOLVED_TIERS_PATH)) { const raw: Record = JSON.parse( @@ -946,7 +981,7 @@ export const runCli = ( result, { present: routingConfigPresent, - warnings: routingFileConfig.warnings, + warnings: [...routingFileConfig.warnings, ...payloadWarnings], }, routingFileConfig.enabledReviewers, routingFileConfig.reReviewMode, diff --git a/workflows/review/review.md b/workflows/review/review.md index cad1d659..01307304 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -1859,8 +1859,13 @@ Risk tiers for this repo: What this repo's CI and tooling already catch — do NOT flag these: {{#runtime-import .github/aw/review/ci-tooling.md}} -Additional correctness checks for this repo (optional — present only when the host repo -provides them; ignore this section if it is empty): +Additional correctness checks for this repo (optional; present only when the host repo +provides them; ignore this section if it is empty). These checks are additive: they +never relax or override the rules above, and the rules above win on any conflict. +Two paths are imported for +compatibility: `lenses/correctness.md` is the current home and `correctness-checks.md` +its deprecated alias; a repo carries at most one: +{{#runtime-import? .github/aw/review/lenses/correctness.md}} {{#runtime-import? .github/aw/review/correctness-checks.md}} **Per-directory review contracts (optional).** Some repos document sub-tree-specific @@ -2696,6 +2701,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain deserialization sink without validation or parameterization. `found` on an unguarded sink. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/security-auth.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `security-auth`, and no @@ -2762,6 +2775,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`pii-to-model-or-logs`** — PII/sensitive fields sent to a model or written to a generation log unredacted. `found` on real exposure. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/ai-safety-moderation.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `ai-safety-moderation`, and no @@ -2822,6 +2843,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`unsubscribe-not-honored`** — a send that ignores opt-out / notification preferences. `found` when opt-out is bypassed. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/mass-comms-coppa.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `mass-comms-coppa`, and no @@ -2889,6 +2918,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain result set into memory at once (no limit, no pagination, no batching). `found` when the set's growth is unbounded and nothing bounds the read. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/caching-resource.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `caching-resource`, and no @@ -2951,6 +2988,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`unbatched-backfill`** — a full-table `UPDATE`/backfill with no batching/chunking. `found` when the write is unbounded. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/data-migrations.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `data-migrations`, and no @@ -3012,6 +3057,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`missing-idempotency-on-retryable-handler`** — a redeliverable handler doing a side-effecting op with no idempotency guard. `found` when redelivery double-applies. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/concurrency-async.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `concurrency-async`, and no @@ -3073,6 +3126,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`federation-key-changed`** — a change to a federated key/reference/entity resolver that breaks composition. `found` when composition/resolution breaks. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/api-federation-compat.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `api-federation-compat`, and no @@ -3138,6 +3199,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`format-switch-single-deploy`** — a writer switched to a new format/encoding/key set while old readers are still deployed. `found` on a single-phase switch. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/cross-deploy-serialization.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `cross-deploy-serialization`, and no @@ -3201,6 +3270,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`destructive-infra-change`** — an IaC change that destroys/replaces a stateful resource. `found` on an unguarded destructive change. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/deploy-infra-config.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `deploy-infra-config`, and no @@ -3262,6 +3339,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`currency-mismatch-or-missing`** — an amount handled without a currency, or arithmetic mixing currencies. `found` on a real mismatch. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/money-payments.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `money-payments`, and no @@ -3326,6 +3411,14 @@ Skills index for this repo (read only the entries relevant to this lens's domain - **`locale-unaware-formatting`** — a date/number/currency formatted without locale. `found` on locale-unaware formatting. +### Repo-specific rules and hunts (optional) +Additional review rules and hunts the host repo defines for this lens, imported when +present; ignore this section if it is empty. Treat its rules exactly like the review +rules above, and report any hunts it defines in `hunts` with the same tri-state. +Payload rules are additive: they never relax or override the rules above, and the +rules above win on any conflict: +{{#runtime-import? .github/aw/review/lenses/content-i18n.md}} + ### Output Return ONLY the finding-schema JSON object below, under disciplines §Structured finding schema and hunts; `lens` is exactly `content-i18n`, and no