diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 89a0af6..2b07ac7 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -196,8 +196,6 @@ jobs: - name: Build review prompt id: prompt run: npx ts-node .tooling/scripts/build-review-prompt.ts - env: - PR_AUTHOR: ${{ github.event.pull_request.user.login }} - name: Cleanup previous AI review comments uses: actions/github-script@v7 diff --git a/docs/actions.md b/docs/actions.md index f591b14..7dcba24 100644 --- a/docs/actions.md +++ b/docs/actions.md @@ -304,12 +304,6 @@ prompt: | Ignore style and formatting issues entirely. ``` -**Personal Review Styles:** - -Individuals can customize the *tone* of reviews on their own PRs, across every -repo that uses this workflow, without touching any repo's `.claude-review.yml`. -See `review-styles/README.md` for setup and details. - **Security Notes:** - Requires approval for external contributors to prevent prompt injection diff --git a/review-styles/README.md b/review-styles/README.md deleted file mode 100644 index 145f39a..0000000 --- a/review-styles/README.md +++ /dev/null @@ -1,54 +0,0 @@ -# Personal review styles - -The [Claude AI PR review](../docs/actions.md#run-claude-reviewyml) uses one default -prompt for everyone. If you want the review of *your* PRs to have a different tone -or emphasis, regardless of which repo the PR is in, add a file here. - -## Setup - -Create `review-styles/.md` (lowercase) in this repo, e.g. -`review-styles/amonkhouse.md`. It's picked up automatically the next time a PR you -author is reviewed, in any repo that uses this workflow. No other setup needed. - -## Format - -Optional YAML frontmatter, then your style content in plain prose or bullets: - -```markdown ---- -mode: augment ---- -Be blunt and terse. Skip the summary on small PRs. -I care most about data-pipeline correctness and idempotency; flag anything -that could double-write or silently drop rows. UK English. -``` - -If you omit the frontmatter, `mode` defaults to `augment`. - -## Modes - -- **`augment`** (default) - keeps the full default prompt (guardrails, review - format, priority emojis) and appends your content as a "Reviewer Style - Preferences" section, with a note that it wins on tone conflicts. Safest option; - right for most people who just want a different voice or emphasis. -- **`replace_style`** - keeps the default guardrails and review format, but swaps - out the "How to write" tone guidance for your content. Use this if you have - strong, complete opinions on tone and don't want the default tone advice - competing with yours. -- **`override`** - your content becomes the *entire* prompt. Nothing else is kept: - no false-positive guardrails, no required review format, no posting - instructions, unless you write them yourself. Only use this if you're - deliberately writing a full replacement prompt (mirrors the repo-level `prompt:` - field in `.claude-review.yml`, but scoped to you). - -## Precedence - -1. A repo's `.claude-review.yml` `prompt:` field (a full override for that repo) - always wins over your personal style - it's a deliberate repo-wide decision. -2. Otherwise, your personal style is applied per its `mode`. -3. A repo's `focus_areas`, `context`, and `ignore_paths` still apply in `augment` - and `replace_style` modes - scope stays with the repo, tone stays with you. - They do not apply under `override`, since there is no base prompt left to - append them to. - -See `amonkhouse.md` in this directory for a worked example. diff --git a/review-styles/amonkhouse.md b/review-styles/amonkhouse.md deleted file mode 100644 index 16d0042..0000000 --- a/review-styles/amonkhouse.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -mode: augment ---- -UK English. Be direct: state the issue and the fix, then stop, no closing summary. -I love emojis. Use more of them. Especially the frog. diff --git a/scripts/build-review-prompt.test.ts b/scripts/build-review-prompt.test.ts index 099efa7..3358f17 100644 --- a/scripts/build-review-prompt.test.ts +++ b/scripts/build-review-prompt.test.ts @@ -1,14 +1,8 @@ import * as fs from "fs" import { - assemblePrompt, buildPrompt, DEFAULT_PROMPT, - HOW_TO_WRITE, - loadPersonalStyle, loadRepoConfig, - PROMPT_CLOSING, - PROMPT_HEADER, - parsePersonalStyle, } from "./build-review-prompt" jest.mock("fs") @@ -110,7 +104,6 @@ prompt: | describe("buildPrompt", () => { beforeEach(() => { jest.clearAllMocks() - delete process.env.PR_AUTHOR }) it("returns default prompt when no config exists", () => { @@ -185,223 +178,6 @@ ignore_paths: expect(result).toContain("## Files to Skip") expect(result).toContain("- **/*.generated.ts") }) - - it("applies the PR author's personal style when PR_AUTHOR is set", () => { - process.env.PR_AUTHOR = "Amonkhouse" - mockFs.existsSync.mockImplementation( - p => typeof p === "string" && p.endsWith("amonkhouse.md") - ) - mockFs.readFileSync.mockImplementation(p => { - if (typeof p === "string" && p.endsWith("amonkhouse.md")) { - return "Be blunt and terse." - } - throw new Error(`unexpected read: ${String(p)}`) - }) - - const result = buildPrompt() - - expect(result).toContain("## Reviewer Style Preferences (amonkhouse)") - expect(result).toContain("Be blunt and terse.") - expect(result).toContain("### Summary") // default prompt structure retained - }) - - it("ignores personal style when no PR_AUTHOR is set", () => { - mockFs.existsSync.mockReturnValue(false) - - const result = buildPrompt() - - expect(result).not.toContain("Reviewer Style Preferences") - expect(result).toBe(DEFAULT_PROMPT) - }) - - it("repo-level prompt override still beats a personal style", () => { - process.env.PR_AUTHOR = "amonkhouse" - mockFs.existsSync.mockReturnValue(true) - mockFs.readFileSync.mockImplementation(p => { - if (typeof p === "string" && p.endsWith(".claude-review.yml")) { - return ` -prompt: | - You are a custom security reviewer. -` - } - return "Be blunt and terse." - }) - - const result = buildPrompt() - - expect(result).toContain("You are a custom security reviewer.") - expect(result).not.toContain("Reviewer Style Preferences") - }) -}) - -describe("parsePersonalStyle", () => { - it("defaults to augment mode with no frontmatter", () => { - const result = parsePersonalStyle("amonkhouse", "Be blunt and terse.") - - expect(result).toEqual({ - login: "amonkhouse", - mode: "augment", - content: "Be blunt and terse.", - }) - }) - - it("reads mode from frontmatter", () => { - const result = parsePersonalStyle( - "amonkhouse", - "---\nmode: replace_style\n---\nBe blunt and terse.\n" - ) - - expect(result.mode).toBe("replace_style") - expect(result.content).toBe("Be blunt and terse.") - }) - - it("falls back to augment and warns on an unknown mode", () => { - const consoleSpy = jest.spyOn(console, "error").mockImplementation() - - const result = parsePersonalStyle( - "amonkhouse", - "---\nmode: nonsense\n---\nBe blunt and terse.\n" - ) - - expect(result.mode).toBe("augment") - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining("Unknown mode") - ) - consoleSpy.mockRestore() - }) - - it("falls back to augment and warns on malformed frontmatter", () => { - const consoleSpy = jest.spyOn(console, "error").mockImplementation() - - const result = parsePersonalStyle( - "amonkhouse", - "---\nmode: [unterminated\n---\nBe blunt and terse.\n" - ) - - expect(result.mode).toBe("augment") - expect(consoleSpy).toHaveBeenCalled() - consoleSpy.mockRestore() - }) - - it("supports override mode", () => { - const result = parsePersonalStyle( - "amonkhouse", - "---\nmode: override\n---\nYou are a custom reviewer.\n" - ) - - expect(result.mode).toBe("override") - expect(result.content).toBe("You are a custom reviewer.") - }) -}) - -describe("loadPersonalStyle", () => { - beforeEach(() => { - jest.clearAllMocks() - }) - - it("returns null when no author is given", () => { - expect(loadPersonalStyle(undefined)).toBeNull() - }) - - it("returns null when no style file exists for the author", () => { - mockFs.existsSync.mockReturnValue(false) - - expect(loadPersonalStyle("amonkhouse")).toBeNull() - }) - - it("lowercases the login when resolving the file path", () => { - mockFs.existsSync.mockImplementation( - p => typeof p === "string" && p.endsWith("review-styles/amonkhouse.md") - ) - mockFs.readFileSync.mockReturnValue("Be blunt and terse.") - - const result = loadPersonalStyle("AmonKHouse") - - expect(result?.login).toBe("amonkhouse") - expect(result?.content).toBe("Be blunt and terse.") - }) - - it("returns null and logs a warning on read error", () => { - const consoleSpy = jest.spyOn(console, "error").mockImplementation() - mockFs.existsSync.mockReturnValue(true) - mockFs.readFileSync.mockImplementation(() => { - throw new Error("Read error") - }) - - const result = loadPersonalStyle("amonkhouse") - - expect(result).toBeNull() - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining("Failed to read review-styles/amonkhouse.md") - ) - consoleSpy.mockRestore() - }) -}) - -describe("assemblePrompt", () => { - it("returns the default prompt when there is no repo config or personal style", () => { - expect(assemblePrompt(null, null)).toBe(DEFAULT_PROMPT) - }) - - it("augment mode keeps the default prompt and appends a style section", () => { - const result = assemblePrompt(null, { - login: "amonkhouse", - mode: "augment", - content: "Be blunt and terse.", - }) - - expect(result).toContain(DEFAULT_PROMPT) - expect(result).toContain("## Reviewer Style Preferences (amonkhouse)") - expect(result).toContain("Be blunt and terse.") - expect(result).toContain("prefer these") - }) - - it("replace_style mode keeps guardrails and format but swaps the tone block", () => { - const result = assemblePrompt(null, { - login: "amonkhouse", - mode: "replace_style", - content: "Be blunt and terse.", - }) - - expect(result).toContain(PROMPT_HEADER) - expect(result).toContain(PROMPT_CLOSING) - expect(result).toContain("## How to write\nBe blunt and terse.") - expect(result).not.toContain(HOW_TO_WRITE) - expect(result).not.toContain("Reviewer Style Preferences") - }) - - it("override mode returns only the personal content", () => { - const result = assemblePrompt( - { focus_areas: ["Watch for N+1 queries"] }, - { - login: "amonkhouse", - mode: "override", - content: "You are a custom reviewer.", - } - ) - - expect(result).toBe("You are a custom reviewer.") - }) - - it("augment mode still applies repo focus areas and ignore paths", () => { - const result = assemblePrompt( - { - focus_areas: ["Watch for N+1 queries"], - ignore_paths: ["**/*.generated.ts"], - }, - { login: "amonkhouse", mode: "augment", content: "Be blunt and terse." } - ) - - expect(result).toContain("## Additional Focus Areas") - expect(result).toContain("- Watch for N+1 queries") - expect(result).toContain("## Files to Skip") - expect(result).toContain("- **/*.generated.ts") - expect(result).toContain("## Reviewer Style Preferences (amonkhouse)") - // Style preferences should come after repo customizations - expect(result.indexOf("## Files to Skip")).toBeLessThan( - result.indexOf("## Reviewer Style Preferences") - ) - }) }) describe("DEFAULT_PROMPT", () => { diff --git a/scripts/build-review-prompt.ts b/scripts/build-review-prompt.ts index e35e6c1..428e767 100644 --- a/scripts/build-review-prompt.ts +++ b/scripts/build-review-prompt.ts @@ -3,21 +3,13 @@ import * as yaml from "js-yaml" import * as path from "path" /** - * Build a review prompt by merging default Artsy guidelines with repo-specific - * configuration and the PR author's personal review style. + * Build a review prompt by merging default Artsy guidelines with repo-specific configuration. * * Repos can create a .claude-review.yml file with: * - prompt: Complete custom prompt (overrides everything else) * - focus_areas: Array of specific things to watch for (added to default prompt) * - ignore_paths: Glob patterns for files to skip * - context: Additional context about the codebase - * - * Individuals can create a review-styles/.md file (in this repo) with - * optional frontmatter choosing how it merges with the default prompt: - * - mode: augment (default) - keep the default prompt, append the style as a - * preference section - * - mode: replace_style - keep guardrails/format, swap only the "How to write" tone - * - mode: override - the personal file becomes the entire prompt */ interface ExcludeConfig { @@ -33,21 +25,7 @@ interface RepoConfig { exclude?: ExcludeConfig } -export type PersonalStyleMode = "augment" | "replace_style" | "override" - -const VALID_PERSONAL_STYLE_MODES: PersonalStyleMode[] = [ - "augment", - "replace_style", - "override", -] - -export interface PersonalStyle { - login: string - mode: PersonalStyleMode - content: string -} - -export const PROMPT_HEADER = `You are a senior staff engineer conducting a code review. +export const DEFAULT_PROMPT = `You are a senior staff engineer conducting a code review. You have access to the full codebase. The PR branch has been checked out. ## Critical: Avoid False Positives @@ -105,9 +83,7 @@ Briefly note any concerns in these areas (skip if nothing notable): ### Questions for Author List anything unclear that needs clarification before you can fully assess the PR. -` - -export const HOW_TO_WRITE = `## How to write +## How to write - Lead with the problem. No preamble like "I noticed that" or "It might be worth considering". - Short words, active voice: "this leaks the handle", not "a resource leak may be introduced". - Cut every word that adds nothing. "Because", not "due to the fact that"; "to", not "in order to"; "before", not "prior to". @@ -117,16 +93,12 @@ export const HOW_TO_WRITE = `## How to write - Avoid words like leverage, robust, comprehensive, crucial, seamless, delve, streamline. Use everyday words. - Go easy on em-dashes; prefer commas and full stops. -` - -export const PROMPT_CLOSING = `--- +--- Be constructive and explain your reasoning. Focus on substantive issues, not style nitpicks. Remember: An empty "Issues Found" section is a valid and often correct outcome. The goal is accurate review, not comprehensive critique. ` -export const DEFAULT_PROMPT = PROMPT_HEADER + HOW_TO_WRITE + PROMPT_CLOSING - export const loadRepoConfig = (): RepoConfig | null => { const configPath = path.join(process.cwd(), ".claude-review.yml") @@ -144,88 +116,16 @@ export const loadRepoConfig = (): RepoConfig | null => { } } -// Parse a personal style file's contents into a mode + body. -export const parsePersonalStyle = ( - login: string, - raw: string -): PersonalStyle => { - const frontmatterMatch = raw.match(/^---\r?\n([\s\S]*?)\r?\n---\r?\n?/) - - let rawMode: unknown - let content = raw - - if (frontmatterMatch) { - content = raw.slice(frontmatterMatch[0].length) - try { - const frontmatter = yaml.load(frontmatterMatch[1]) as { mode?: unknown } - rawMode = frontmatter?.mode - } catch (error) { - const message = error instanceof Error ? error.message : String(error) - console.error( - `Warning: Failed to parse frontmatter in review-styles/${login}.md: ${message}` - ) - } - } - - let mode: PersonalStyleMode = "augment" - if (rawMode !== undefined) { - if (VALID_PERSONAL_STYLE_MODES.includes(rawMode as PersonalStyleMode)) { - mode = rawMode as PersonalStyleMode - } else { - console.error( - `Warning: Unknown mode "${String(rawMode)}" in review-styles/${login}.md, falling back to "augment"` - ) - } - } - - return { login, mode, content: content.trim() } -} - -// Load the PR author's personal review style, if they have one. - -export const loadPersonalStyle = ( - author: string | undefined -): PersonalStyle | null => { - if (!author) { - return null - } - - const login = author.toLowerCase() - const stylePath = path.join(__dirname, "..", "review-styles", `${login}.md`) - - if (!fs.existsSync(stylePath)) { - return null - } - - try { - const raw = fs.readFileSync(stylePath, "utf8") - return parsePersonalStyle(login, raw) - } catch (error) { - const message = error instanceof Error ? error.message : String(error) - console.error( - `Warning: Failed to read review-styles/${login}.md: ${message}` - ) - return null - } -} +export const buildPrompt = (): string => { + const repoConfig = loadRepoConfig() -// Assemble the final prompt from repo config and personal style. -export const assemblePrompt = ( - repoConfig: RepoConfig | null, - personalStyle: PersonalStyle | null -): string => { - if (personalStyle?.mode === "override") { - return personalStyle.content + // If repo provides a complete custom prompt, use it directly + if (repoConfig?.prompt) { + return repoConfig.prompt } - const base = - personalStyle?.mode === "replace_style" - ? PROMPT_HEADER + - `## How to write\n${personalStyle.content}\n\n` + - PROMPT_CLOSING - : DEFAULT_PROMPT - - const sections = [base] + // Otherwise, build from default + customizations + const sections = [DEFAULT_PROMPT] if (repoConfig) { // Add repo-specific context @@ -254,31 +154,9 @@ export const assemblePrompt = ( } } - if (personalStyle?.mode === "augment") { - sections.push( - `\n## Reviewer Style Preferences (${personalStyle.login})\n\n` + - `The PR author has personal review-style preferences below. Where these conflict with the guidance above, prefer these.\n\n` + - `${personalStyle.content}\n` - ) - } - return sections.join("") } -export const buildPrompt = (): string => { - const repoConfig = loadRepoConfig() - - // If repo provides a complete custom prompt, use it directly - this is a - // deliberate repo-wide decision and beats any personal style. - if (repoConfig?.prompt) { - return repoConfig.prompt - } - - const personalStyle = loadPersonalStyle(process.env.PR_AUTHOR) - - return assemblePrompt(repoConfig, personalStyle) -} - const main = (): void => { const prompt = buildPrompt()