diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e4653c4ab4..54059580aa 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -279,6 +279,7 @@ import { runGittensoryAiSlopAdvisory } from "../services/ai-slop"; import { decidePublicSurface } from "../signals/settings-preview"; import { buildFocusManifestGuidance, + composeRepoReviewContext, excludeReviewPaths, resolveReviewPathInstructions, resolveReviewPreMergeChecks, @@ -287,7 +288,10 @@ import { type ReviewPathInstruction, type ReviewProfile, } from "../signals/focus-manifest"; -import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; +import { + loadRepoFocusManifest, + loadRepoReviewContext, +} from "../signals/focus-manifest-loader"; import { resolveRepositorySettings } from "../settings/repository-settings"; import type { LocalBranchAnalysisInput } from "../signals/local-branch"; import { @@ -4471,7 +4475,7 @@ async function maybePublishPrPublicSurface( profile: reviewProfile, inlineComments: reviewInlineComments, pathInstructions: reviewPathInstructions, - instructions: reviewInstructions, + instructions: manifestReviewInstructions, excludePaths: reviewExcludePaths, } = resolveReviewPromptOverrides( await loadRepoFocusManifest(env, repoFullName).catch(() => null), @@ -4481,6 +4485,21 @@ async function maybePublishPrPublicSurface( repoFullName, reviewInlineComments, ); + // Per-repo review CONTEXT (#review-skills): fold the container-private review/CLAUDE.md guide + the matching + // review/skills/*.md modules into the SAME review-instructions slot, so reviews follow each repo's conventions. + // Glob-gated for cost (only skills matching the changed files are injected); absent config dir ⇒ empty ⇒ + // byte-identical prompt. getReviewFiles() is memoized, so the second call reuses the loaded diff. + const reviewInstructions = + [ + manifestReviewInstructions, + composeRepoReviewContext( + await loadRepoReviewContext(repoFullName), + (await getReviewFiles()).map((file) => file.path), + ), + ] + .map((part) => part?.trim()) + .filter(Boolean) + .join("\n\n") || null; aiReview = await runAiReviewForAdvisory(env, { settings, advisory, diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index 65eb391270..b3a12ccd83 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -15,9 +15,16 @@ // `.yaml` / `.json` are accepted everywhere `.yml` is. The first existing candidate wins outright (a present // per-repo file fully REPLACES the global fallback — "fallback" means "used only when no per-repo file exists", // not a deep merge). The slug is lowercased (GitHub repo full-names are case-insensitive; #1390 already lowercased). -import { readFile } from "node:fs/promises"; +import { readFile, readdir } from "node:fs/promises"; import { join, resolve } from "node:path"; -import type { RepoFocusManifestFetcher } from "../signals/focus-manifest-loader"; +import type { + RepoReviewContext, + RepoReviewSkill, +} from "../signals/focus-manifest"; +import type { + RepoFocusManifestFetcher, + RepoReviewContextReader, +} from "../signals/focus-manifest-loader"; /** The bare config filenames tried inside a per-repo folder and at the dir root (global fallback), in priority order. */ const CONFIG_BASENAMES = [".gittensory.yml", ".gittensory.yaml", ".gittensory.json"] as const; @@ -76,3 +83,61 @@ export function makeLocalManifestReader(dir: string | undefined): RepoFocusManif return null; }; } + +/** Per-repo review-context candidate FOLDERS (relative to GITTENSORY_REPO_CONFIG_DIR): `{owner}__{repo}/review` then + * `{repo}/review`. Same owner/repo validation as localConfigCandidates; an invalid full name yields none. (#review-skills) */ +function reviewContextFolders(repoFullName: string): string[] { + const slash = repoFullName.indexOf("/"); + if (slash <= 0 || slash === repoFullName.length - 1 || slash !== repoFullName.lastIndexOf("/")) return []; + const owner = repoFullName.slice(0, slash).toLowerCase(); + const repo = repoFullName.slice(slash + 1).toLowerCase(); + if (!GITHUB_OWNER_SEGMENT.test(owner) || !isSafeRepoSegment(repo)) return []; + return [join(`${owner}__${repo}`, "review"), join(repo, "review")]; +} + +/** Parse a skill markdown file into {name, when, body}. YAML frontmatter (`---\nname:\nwhen:\n---`) is optional; name + * defaults to the filename and `when` to "always". */ +export function parseReviewSkill(filename: string, text: string): RepoReviewSkill { + const fm = /^---\s*\n([\s\S]*?)\n---\s*\n?([\s\S]*)$/.exec(text); + const head = fm?.[1] ?? ""; + const body = (fm?.[2] ?? text).trim(); + const name = /(?:^|\n)name:\s*(.+)/.exec(head)?.[1]?.trim() || filename.replace(/\.md$/i, ""); + const whenRaw = /(?:^|\n)when:\s*(.+)/.exec(head)?.[1]?.trim(); + const when = (whenRaw ?? "always").replace(/^["']|["']$/g, "") || "always"; + return { name, when, body }; +} + +/** Build the container-local review-context reader over GITTENSORY_REPO_CONFIG_DIR, or null when the dir is unset. Per + * repo (first existing folder wins) reads `review/CLAUDE.md` (the guide) + every `review/skills/*.md` (rubric modules, + * sorted). Missing files/dir degrade to nulls/empty; a per-file read error skips that file. (#review-skills) */ +export function makeLocalReviewContextReader(dir: string | undefined): RepoReviewContextReader | null { + const trimmed = (dir ?? "").trim(); + if (!trimmed) return null; + const base = resolve(trimmed); + return async (repoFullName: string): Promise => { + for (const folder of reviewContextFolders(repoFullName)) { + const abs = resolve(base, folder); + let guide: string | null = null; + try { + guide = await readFile(resolve(abs, "CLAUDE.md"), "utf8"); + } catch { + // no per-repo review guide + } + const skills: RepoReviewSkill[] = []; + try { + const entries = (await readdir(resolve(abs, "skills"))).filter((f) => f.toLowerCase().endsWith(".md")).sort(); + for (const f of entries) { + try { + skills.push(parseReviewSkill(f, await readFile(resolve(abs, "skills", f), "utf8"))); + } catch { + // unreadable skill file → skip it + } + } + } catch { + // no skills/ dir + } + if (guide !== null || skills.length > 0) return { guide, skills }; + } + return { guide: null, skills: [] }; + }; +} diff --git a/src/server.ts b/src/server.ts index 7855f6d89e..83c8b05559 100644 --- a/src/server.ts +++ b/src/server.ts @@ -46,9 +46,15 @@ import { createPgVectorize, initPgVectorize } from "./selfhost/pg-vectorize"; import { createSqliteQueue } from "./selfhost/sqlite-queue"; import { createSqliteVectorize } from "./selfhost/vectorize"; import { createFsBlobStore } from "./selfhost/blob-store"; -import { makeLocalManifestReader } from "./selfhost/private-config"; +import { + makeLocalManifestReader, + makeLocalReviewContextReader, +} from "./selfhost/private-config"; import { captureError, flushSentry, initSentry } from "./selfhost/sentry"; -import { setLocalManifestReader } from "./signals/focus-manifest-loader"; +import { + setLocalManifestReader, + setLocalReviewContextReader, +} from "./signals/focus-manifest-loader"; import type { JobMessage } from "./types"; /** Resolve `_FILE` env vars (Docker secrets / multi-line keys) into `` at startup. */ @@ -225,6 +231,11 @@ async function main(): Promise { setLocalManifestReader( makeLocalManifestReader(process.env.GITTENSORY_REPO_CONFIG_DIR), ); + // Per-repo review CONTEXT (#review-skills): the same config dir also holds `/review/CLAUDE.md` + skills/*.md, + // injected into the reviewer prompt so reviews follow each repo's conventions. Unset dir ⇒ null reader ⇒ no change. + setLocalReviewContextReader( + makeLocalReviewContextReader(process.env.GITTENSORY_REPO_CONFIG_DIR), + ); // Error tracking (#1468): opt-in via SENTRY_DSN — a complete no-op when unset. When on, capture uncaught crashes // + unhandled rejections (flush before exit for the fatal case); per-subsystem captures (queue dead-letter, // review failures) are wired at their sites. diff --git a/src/signals/focus-manifest-loader.ts b/src/signals/focus-manifest-loader.ts index 90710224b8..191860e395 100644 --- a/src/signals/focus-manifest-loader.ts +++ b/src/signals/focus-manifest-loader.ts @@ -1,7 +1,7 @@ import { listSignalSnapshots, persistSignalSnapshot } from "../db/repositories"; import type { JsonValue } from "../types"; import { nowIso } from "../utils/json"; -import { featuresConfigToJson, gateConfigToJson, MAX_FOCUS_MANIFEST_BYTES, parseFocusManifest, parseFocusManifestContent, reviewConfigToJson, settingsOverrideToJson, type FocusManifest, type FocusManifestSource } from "./focus-manifest"; +import { featuresConfigToJson, gateConfigToJson, MAX_FOCUS_MANIFEST_BYTES, parseFocusManifest, parseFocusManifestContent, reviewConfigToJson, settingsOverrideToJson, type FocusManifest, type FocusManifestSource, type RepoReviewContext } from "./focus-manifest"; import { GITTENSORY_REPO_FOCUS_MANIFEST_YAML, resolveGittensorySelfRepoFullName } from "../config/gittensory-repo-focus-manifest"; export const REPO_FOCUS_MANIFEST_SIGNAL = "repo-focus-manifest"; @@ -33,6 +33,35 @@ export function setLocalManifestReader(reader: RepoFocusManifestFetcher | null): localManifestReader = reader; } +/** + * Async source for a repo's review CONTEXT (#review-skills): the `review/CLAUDE.md` guide + `review/skills/*.md` rubric + * modules from the container-private config dir. Registered once at boot by the Node entry (server.ts); the filesystem + * access lives inside that injected closure, keeping THIS module Workers-safe. Unset (cloud, or a self-host without the + * dir) ⇒ the loader returns an empty context and the reviewer prompt is byte-identical. + */ +export type RepoReviewContextReader = ( + repoFullName: string, +) => Promise; +let localReviewContextReader: RepoReviewContextReader | null = null; +export function setLocalReviewContextReader( + reader: RepoReviewContextReader | null, +): void { + localReviewContextReader = reader; +} + +/** Load the per-repo review context via the registered reader. Local file reads are cheap, so this is NOT cached. + * Unset reader ⇒ empty context; a read error degrades to empty (the reviewer prompt stays byte-identical). */ +export async function loadRepoReviewContext( + repoFullName: string, +): Promise { + if (!localReviewContextReader) return { guide: null, skills: [] }; + try { + return await localReviewContextReader(repoFullName); + } catch { + return { guide: null, skills: [] }; + } +} + /** * Fetch a maintainer-owned manifest file from the public GitHub raw endpoint. Network or HTTP * failures resolve to null so the loader falls back to deterministic signals. diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 8d06eeee50..8fc4b4a3e8 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -844,6 +844,54 @@ export function resolveReviewPreMergeChecks(manifest: FocusManifest | null): Pre return manifest?.review.preMergeChecks ?? []; } +/** One per-repo review SKILL (#review-skills): a maintainer-maintained rubric module loaded from the container-private + * config dir (`/review/skills/*.md`). `when` is "always" (repo-wide) or a path glob / brace-list that gates it to + * matching changed files (cost: only relevant skills are injected). */ +export type RepoReviewSkill = { name: string; when: string; body: string }; +/** The per-repo review CONTEXT (#review-skills): an always-on `review/CLAUDE.md` guide + the skill rubric modules. */ +export type RepoReviewContext = { guide: string | null; skills: RepoReviewSkill[] }; + +/** Hard cap on the injected per-repo review context — a cost guard so a runaway guide/skills set can't bloat every + * prompt. The maintained files are concise by design; this only bites pathological inputs. */ +const MAX_REVIEW_CONTEXT_CHARS = 16_000; + +/** True when a skill's `when` applies to this PR: "always"/empty ⇒ yes; otherwise the (possibly brace-listed) glob must + * match at least one changed path. Reuses the manifest path matcher so it behaves exactly like path_instructions. */ +function reviewSkillApplies(when: string, changedPaths: string[]): boolean { + const w = when.trim(); + if (!w || w.toLowerCase() === "always") return true; + const patterns = w + .replace(/^\{|\}$/g, "") + .split(",") + .map((p) => p.trim()) + .filter(Boolean); + return patterns.some((pat) => + changedPaths.some((path) => matchesManifestPath(path, pat)), + ); +} + +/** Compose the per-repo review context into a prompt section (#review-skills): the always-on guide + every skill whose + * `when` applies to this PR's changed files. Bounded for cost. Null/empty ⇒ "" (byte-identical reviewer prompt). The + * caller folds the result into the `review.instructions` slot, so it inherits the same prompt wrapper + public-safe + * handling. */ +export function composeRepoReviewContext( + context: RepoReviewContext | null, + changedPaths: string[], +): string { + if (!context) return ""; + const parts: string[] = []; + if (context.guide?.trim()) parts.push(context.guide.trim()); + for (const skill of context.skills) { + if (reviewSkillApplies(skill.when, changedPaths) && skill.body.trim()) + parts.push(`## skill: ${skill.name}\n${skill.body.trim()}`); + } + if (parts.length === 0) return ""; + const joined = parts.join("\n\n"); + return joined.length > MAX_REVIEW_CONTEXT_CHARS + ? joined.slice(0, MAX_REVIEW_CONTEXT_CHARS) + : joined; +} + /** Filter a PR's changed files down to the set the AI review should see — dropping any whose path matches a * `review.exclude_paths` glob (generated/vendored/lockfiles). Empty `excludePaths` ⇒ the same array (byte-identical * review). Pure; the gate/slop/secret-scan operate on the unfiltered files. (#review-exclude-paths) */ diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index ded633b296..3d79fa3255 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -13,6 +13,7 @@ import { excludeReviewPaths, resolveReviewPathInstructions, resolveReviewPreMergeChecks, + composeRepoReviewContext, resolveReviewPromptOverrides, reviewConfigToJson, settingsOverrideToJson, @@ -1365,3 +1366,45 @@ describe("review.pre_merge_checks (#review-pre-merge-checks)", () => { expect(resolveReviewPreMergeChecks(null)).toEqual([]); }); }); + +describe("composeRepoReviewContext (#review-skills)", () => { + it("returns '' for null/empty/whitespace-only context", () => { + expect(composeRepoReviewContext(null, ["a.ts"])).toBe(""); + expect(composeRepoReviewContext({ guide: null, skills: [] }, ["a.ts"])).toBe(""); + expect(composeRepoReviewContext({ guide: " ", skills: [] }, ["a.ts"])).toBe(""); + }); + + it("includes the guide + always/blank-when/glob-matched skills, excluding non-matching ones", () => { + const ctx = { + guide: "Review THIS repo carefully.", + skills: [ + { name: "voice", when: "always", body: "Be decisive." }, + { name: "blank", when: "", body: "Blank-when is always-on." }, + { name: "sql", when: "**/*.sql", body: "Check the index usage." }, + { name: "schema", when: "{**/db/schema.ts,**/*.sql}", body: "Migration parity." }, + { name: "ui", when: "app/**", body: "Should not appear." }, + ], + }; + const out = composeRepoReviewContext(ctx, ["migrations/0079_x.sql"]); + expect(out).toContain("Review THIS repo carefully."); + expect(out).toContain("## skill: voice"); + expect(out).toContain("## skill: blank"); + expect(out).toContain("## skill: sql"); // **/*.sql matched the .sql file + expect(out).toContain("## skill: schema"); // brace-list matched the .sql file + expect(out).not.toContain("## skill: ui"); // app/** did not match + expect(out).not.toContain("Should not appear."); + }); + + it("drops empty-body and non-matching skills (⇒ '' when nothing applies)", () => { + const out = composeRepoReviewContext( + { guide: null, skills: [{ name: "empty", when: "always", body: " " }, { name: "x", when: "src/**", body: "nope" }] }, + ["README.md"], + ); + expect(out).toBe(""); + }); + + it("bounds the injected context to the cost cap", () => { + const out = composeRepoReviewContext({ guide: "x".repeat(20_000), skills: [] }, []); + expect(out.length).toBeLessThanOrEqual(16_000); + }); +}); diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index 16d0871e93..fcf8bf3766 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -2,7 +2,8 @@ import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { describe, expect, it } from "vitest"; -import { GLOBAL_CONFIG_CANDIDATES, localConfigCandidates, makeLocalManifestReader } from "../../src/selfhost/private-config"; +import { GLOBAL_CONFIG_CANDIDATES, localConfigCandidates, makeLocalManifestReader, makeLocalReviewContextReader, parseReviewSkill } from "../../src/selfhost/private-config"; +import { loadRepoReviewContext, setLocalReviewContextReader } from "../../src/signals/focus-manifest-loader"; describe("localConfigCandidates (container-private config paths)", () => { it("builds owner-folder → repo-folder → flat candidates (lowercased), each in .yml/.yaml/.json order", () => { @@ -104,3 +105,61 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { expect(await reader!("owner/..")).toBeNull(); }); }); + +describe("parseReviewSkill (#review-skills)", () => { + it("parses frontmatter name + when (quotes stripped); body is the remainder", () => { + expect(parseReviewSkill("sql.md", '---\nname: sql-rubric\nwhen: "**/*.sql"\n---\nCheck the index.\n')).toEqual({ name: "sql-rubric", when: "**/*.sql", body: "Check the index." }); + }); + it("defaults name to the filename and when to 'always' with no frontmatter", () => { + expect(parseReviewSkill("voice.md", "Be decisive.")).toEqual({ name: "voice", when: "always", body: "Be decisive." }); + }); + it("treats a quotes-only/empty when as 'always'", () => { + expect(parseReviewSkill("x.md", '---\nwhen: ""\n---\nbody').when).toBe("always"); + }); +}); + +describe("makeLocalReviewContextReader (#review-skills)", () => { + it("returns null when the dir is unset/blank", () => { + expect(makeLocalReviewContextReader(undefined)).toBeNull(); + expect(makeLocalReviewContextReader(" ")).toBeNull(); + }); + + it("reads the owner-qualified review/CLAUDE.md + skills/*.md (sorted, .md only)", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-review-")); + const rev = join(dir, "jsonbored__gittensory", "review"); + mkdirSync(join(rev, "skills"), { recursive: true }); + writeFileSync(join(rev, "CLAUDE.md"), "Review gittensory carefully.\n"); + writeFileSync(join(rev, "skills", "b-second.md"), "---\nname: second\nwhen: always\n---\nSecond.\n"); + writeFileSync(join(rev, "skills", "a-first.md"), "First with no frontmatter.\n"); + writeFileSync(join(rev, "skills", "notes.txt"), "ignored — not .md\n"); + const reader = makeLocalReviewContextReader(dir)!; + const ctx = await reader("JSONbored/gittensory"); + expect(ctx.guide).toContain("Review gittensory carefully."); + expect(ctx.skills.map((s) => s.name)).toEqual(["a-first", "second"]); // sorted by filename; .txt ignored + }); + + it("falls back to the bare repo-name folder; returns empty for a missing or invalid repo", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-review-")); + mkdirSync(join(dir, "metagraphed", "review"), { recursive: true }); + writeFileSync(join(dir, "metagraphed", "review", "CLAUDE.md"), "Bare-folder guide.\n"); + const reader = makeLocalReviewContextReader(dir)!; + expect((await reader("JSONbored/metagraphed")).guide).toContain("Bare-folder guide."); + expect(await reader("JSONbored/unknown-repo")).toEqual({ guide: null, skills: [] }); // no folder + expect(await reader("owner/..")).toEqual({ guide: null, skills: [] }); // invalid repo segment → no candidates + expect(await reader("noslash")).toEqual({ guide: null, skills: [] }); // invalid full name (no slash) + }); +}); + +describe("loadRepoReviewContext + setLocalReviewContextReader (#review-skills)", () => { + it("empty with no reader; uses the registered reader; degrades to empty on error", async () => { + setLocalReviewContextReader(null); + expect(await loadRepoReviewContext("o/r")).toEqual({ guide: null, skills: [] }); + setLocalReviewContextReader(async () => ({ guide: "G", skills: [{ name: "s", when: "always", body: "B" }] })); + expect(await loadRepoReviewContext("o/r")).toEqual({ guide: "G", skills: [{ name: "s", when: "always", body: "B" }] }); + setLocalReviewContextReader(async () => { + throw new Error("read failed"); + }); + expect(await loadRepoReviewContext("o/r")).toEqual({ guide: null, skills: [] }); + setLocalReviewContextReader(null); // reset for other tests + }); +});