diff --git a/.coderabbit.yaml b/.coderabbit.yaml index b21c49bca..f88bd9b19 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -13,19 +13,19 @@ reviews: pre_merge_checks: title: mode: "error" - requirements: "Apply CONTRIBUTING.md → Pull requests → PR title. Fail titles that are non-English, vague, placeholder-like, or do not identify the actual change." + requirements: "Apply CONTRIBUTING.md → Pull requests → PR title. Request a clearer title when it is non-English, vague, placeholder-like, or does not identify the actual change." description: mode: "warning" custom_checks: - - name: "PR Hygiene: English" + - name: "PR Readability: English" mode: "error" - instructions: "Use CONTRIBUTING.md → Pull requests as the source of truth. Fail when the PR title or body is not primarily English, except for code identifiers, file paths, logs, error messages, and short quoted examples." - - name: "PR Hygiene: Template" + instructions: "Use CONTRIBUTING.md → Pull requests as the source of truth. Request English for the PR title and body when they are not primarily English, except for code identifiers, file paths, logs, error messages, and short quoted examples." + - name: "PR Description: Template" mode: "warning" - instructions: "Use CONTRIBUTING.md → Pull requests as the source of truth. Warn when the PR body contains unfilled template placeholders, dangling issue references, TODO/TBD text, empty headings, or template comments. Do not treat missing exact headings as a failure when the author otherwise explains the change." - - name: "PR Hygiene: Substance" + instructions: "Use CONTRIBUTING.md → Pull requests as the source of truth. Suggest cleanup when the PR body contains unfilled template placeholders, dangling issue references, TODO/TBD text, empty headings, or template comments. Do not treat missing exact headings as a problem when the author otherwise explains the change." + - name: "PR Description: Context" mode: "warning" - instructions: "Use CONTRIBUTING.md → Pull requests as the source of truth. Warn when the PR body does not explain what changed, why it changed, or how it was validated. Do not call a focused PR low-effort solely because it needs template cleanup." + instructions: "Use CONTRIBUTING.md → Pull requests as the source of truth. Suggest adding context when the PR body does not explain what changed, why it changed, or how it was validated. Do not call a focused PR low-effort solely because it needs template cleanup." chat: auto_reply: true diff --git a/.github/scripts/monitor-pr-hygiene.ts b/.github/scripts/monitor-pr-hygiene.ts deleted file mode 100644 index 0d84d1cd2..000000000 --- a/.github/scripts/monitor-pr-hygiene.ts +++ /dev/null @@ -1,129 +0,0 @@ -import { readFile } from 'node:fs/promises' - -const owner = process.env.GITHUB_REPOSITORY_OWNER -const repo = process.env.GITHUB_REPOSITORY?.split('/')[1] -const eventPath = process.env.GITHUB_EVENT_PATH -const token = process.env.GITHUB_TOKEN -const githubAPIURL = process.env.GITHUB_API_URL ?? 'https://api.github.com' - -if (!owner || !repo || !eventPath || !token) { - throw new Error('Missing required GitHub Actions environment') -} - -interface GitHubEvent { - sender?: { login?: string } - review?: { state?: string; body?: string } - pull_request?: { number?: number } - issue?: { number?: number; pull_request?: unknown } - comment?: { body?: string } -} - -interface PullRequestResponse { - state: string - author_association: string - title: string - user: { login: string } -} - -class GitHubAPIError extends Error { - readonly status: number - - constructor(message: string, status: number) { - super(message) - this.status = status - } -} - -const event = JSON.parse(await readFile(eventPath, 'utf8')) as GitHubEvent -const sender = event.sender?.login ?? '' -const coderabbitAuthors = new Set(['coderabbitai[bot]', 'coderabbitai']) - -if (!coderabbitAuthors.has(sender)) { - console.log(`Ignoring sender: ${sender}`) - process.exit(0) -} - -let issueNumber: number | undefined -let text = '' -let shouldInspect = false - -if (event.review && event.pull_request) { - issueNumber = event.pull_request.number - text = `${event.review.state ?? ''}\n${event.review.body ?? ''}` - shouldInspect = event.review.state === 'changes_requested' -} else if (event.comment && event.issue?.pull_request) { - issueNumber = event.issue.number - text = event.comment.body ?? '' - shouldInspect = true -} - -if (!issueNumber || !shouldInspect) { - console.log('No actionable PR hygiene signal found.') - process.exit(0) -} - -function tableCells(line: string): string[] { - return line - .trim() - .replace(/^\|/, '') - .replace(/\|$/, '') - .split('|') - .map((cell) => cell.trim()) -} - -function normalizedCheckName(value: string): string { - return value.replace(/[^a-z0-9]+/gi, ' ').trim().toLowerCase() -} - -function prHygieneFailureName(line: string): string | null { - const cells = tableCells(line) - const checkName = cells[0] ?? '' - const status = cells[1] ?? '' - if (checkName.toLowerCase().includes('[ignored]')) return null - if (!normalizedCheckName(checkName).startsWith('pr hygiene')) return null - if (!/❌/u.test(status) && !/\berror\b/i.test(status)) return null - - return checkName -} - -const hygieneFailures = text.split('\n').map(prHygieneFailureName).filter((name): name is string => Boolean(name)) - -if (hygieneFailures.length === 0) { - console.log('CodeRabbit signal did not reference a failed PR Hygiene check.') - process.exit(0) -} - -async function github(path: string, options: RequestInit = {}): Promise { - const response = await fetch(`${githubAPIURL}${path}`, { - ...options, - headers: { - Accept: 'application/vnd.github+json', - Authorization: `Bearer ${token}`, - 'X-GitHub-Api-Version': '2022-11-28', - ...options.headers - } - }) - - if (!response.ok) { - const body = await response.text() - throw new GitHubAPIError( - `${options.method ?? 'GET'} ${path} failed: ${response.status} ${body}`, - response.status - ) - } - - if (response.status === 204) return null - return response.json() as Promise -} - -const pr = await github(`/repos/${owner}/${repo}/pulls/${issueNumber}`) -if (!pr) throw new Error(`PR #${issueNumber} returned no data`) - -console.log( - [ - `Detected CodeRabbit PR Hygiene failure on #${issueNumber}: ${hygieneFailures.join(', ')}`, - `Author: ${pr.user.login} (${pr.author_association})`, - `Title: ${pr.title}`, - 'No automatic close/label/comment was applied. Treat this as maintainer review signal only.' - ].join('\n') -) diff --git a/.github/workflows/monitor-pr-hygiene.yml b/.github/workflows/pr-review-guidance.yml similarity index 59% rename from .github/workflows/monitor-pr-hygiene.yml rename to .github/workflows/pr-review-guidance.yml index a99aa5950..9d7a130bd 100644 --- a/.github/workflows/monitor-pr-hygiene.yml +++ b/.github/workflows/pr-review-guidance.yml @@ -1,4 +1,4 @@ -name: Monitor PR hygiene +name: PR review guidance on: pull_request_review: @@ -7,13 +7,13 @@ on: types: [created, edited] concurrency: - group: monitor-pr-hygiene-${{ github.event.pull_request.number || github.event.issue.number }} + group: pr-review-guidance-${{ github.event.pull_request.number || github.event.issue.number }} cancel-in-progress: false jobs: - # This job only logs CodeRabbit PR Hygiene pre-merge failures and never runs contributor code. - monitor-pr-hygiene: - name: Monitor PR hygiene + # This job only records CodeRabbit PR review guidance and never runs contributor code. + pr-review-guidance: + name: Record PR review guidance runs-on: ubuntu-latest permissions: contents: read @@ -30,12 +30,12 @@ jobs: with: node-version: 24 - - name: Log CodeRabbit PR Hygiene signal + - name: Record CodeRabbit review guidance env: GITHUB_TOKEN: ${{ github.token }} run: | - if [ ! -f .github/scripts/monitor-pr-hygiene.ts ]; then - echo "Trusted PR hygiene monitor is not present on the default branch yet." + if [ ! -f tools/pr-review-guidance/src/index.ts ]; then + echo "Trusted PR review guidance helper is not present on the default branch yet." exit 0 fi - node --experimental-strip-types .github/scripts/monitor-pr-hygiene.ts + node --experimental-strip-types tools/pr-review-guidance/src/index.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2281c87e8..ba69795ed 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,11 +37,11 @@ Pull requests must be reviewable without guessing the author's intent. - Document validation, such as `bun run check`, targeted tests, docs-only review, or an explicit reason validation was not run. - Keep the body primarily in English. Code identifiers, file paths, logs, error messages, and short quoted examples may use their original language. -### Low-effort PRs +### Reviewability Do not submit placeholder PRs. Remove template comments before opening a PR. Do not leave dangling issue references such as `Fixes #`, `TODO`, `TBD`, empty headings, unfilled sections, or similar unfinished text. -CodeRabbit may flag PR Hygiene issues for maintainers to review. Missing template sections or validation details are normal review feedback; they are not, by themselves, a personal judgment on the contributor. Maintainers may close PRs manually when they are clearly low-effort, automated, not written in English, unrelated to the project, or impossible to review without substantial guesswork. If you are unsure how to fix something, please open a detailed issue instead of submitting a placeholder PR. +CodeRabbit may flag PR description or readability issues for maintainers to review. Missing template sections or validation details are normal review feedback; they are not, by themselves, a personal judgment on the contributor. Maintainers may close PRs manually when they are clearly automated, not written in English, unrelated to the project, or impossible to review without substantial guesswork. If you are unsure how to fix something, please open a detailed issue instead of submitting a placeholder PR. ## Quality checks diff --git a/package.json b/package.json index 4781c221d..cff13a9f2 100644 --- a/package.json +++ b/package.json @@ -13,10 +13,10 @@ "preview": "vite preview", "tauri": "tauri", "lint": "bun run lint:structure && oxlint -c oxlint.json --type-aware --type-check src/ packages/core/src/ packages/vue/src/ packages/cli/src/ packages/mcp/src/", - "lint:structure": "oxlint -c oxlint.json vite.config.ts vite/ src/ packages/core/src/ packages/vue/src/ packages/cli/src/ packages/mcp/src/ tests/ scripts/", - "format": "oxfmt --write .oxfmtrc.json vite.config.ts vite/ src/ packages/core/src/ packages/cli/src/ packages/mcp/src/ packages/vue/src/ tests scripts/", + "lint:structure": "oxlint -c oxlint.json vite.config.ts vite/ src/ packages/core/src/ packages/vue/src/ packages/cli/src/ packages/mcp/src/ tests/ scripts/ tools/", + "format": "oxfmt --write .oxfmtrc.json vite.config.ts vite/ src/ packages/core/src/ packages/cli/src/ packages/mcp/src/ packages/vue/src/ tests scripts/ tools/", "format:check": "bun run format && status=$(git status --porcelain -uall) && test -z \"$status\" || (echo \"$status\" && exit 1)", - "check": "bun run build:packages && bun run lint && tsgo --noEmit && bun run check:vue && bun run check:i18n && bun run check:packages && bun run check:arch && bun run test:type-shapes && bun run test:dupes", + "check": "bun run build:packages && bun run lint && tsgo --noEmit && bun run check:vue && bun run check:i18n && bun run check:packages && bun run check:arch && bun run test:type-shapes && bun run test:tools && bun run test:dupes", "check:i18n": "bun scripts/check-locales.ts", "check:packages": "bun scripts/check-package-metadata.ts", "check:arch": "steiger .", @@ -28,6 +28,7 @@ "test:unit": "bun test ./tests/engine", "test:coverage": "bun test --coverage ./tests/engine", "test:type-shapes": "bun scripts/type-shapes.ts", + "test:tools": "bun --cwd tools/pr-review-guidance test", "test:dupes": "jscpd packages/core/src packages/cli/src src --min-lines 5 --min-tokens 50 --format typescript --threshold 0", "test:packages": "bun scripts/check-package-metadata.ts && bun scripts/smoke-packages.ts", "build:packages": "bun --filter @open-pencil/core build && bun --filter @open-pencil/vue build && bun --filter @open-pencil/mcp build && bun --filter @open-pencil/cli build", diff --git a/tools/pr-review-guidance/package.json b/tools/pr-review-guidance/package.json new file mode 100644 index 000000000..41a845347 --- /dev/null +++ b/tools/pr-review-guidance/package.json @@ -0,0 +1,8 @@ +{ + "name": "@open-pencil/pr-review-guidance", + "private": true, + "type": "module", + "scripts": { + "test": "bun test tests" + } +} diff --git a/tools/pr-review-guidance/src/index.ts b/tools/pr-review-guidance/src/index.ts new file mode 100644 index 000000000..334ad7abe --- /dev/null +++ b/tools/pr-review-guidance/src/index.ts @@ -0,0 +1,196 @@ +import { readFile } from 'node:fs/promises' +import { pathToFileURL } from 'node:url' + +const CODE_RABBIT_AUTHORS = new Set(['coderabbitai[bot]', 'coderabbitai']) +const REVIEW_GUIDANCE_PREFIXES = ['pr hygiene', 'pr readability', 'pr description'] + +export interface GitHubEvent { + sender?: { login?: string } + review?: { state?: string; body?: string } + pull_request?: { number?: number } + issue?: { number?: number; pull_request?: unknown } + comment?: { body?: string } +} + +export interface PullRequestSummary { + state: string + author_association: string + title: string + user: { login: string } +} + +interface EventContext { + issueNumber?: number + shouldInspect: boolean + text: string +} + +interface GitHubActionsEnvironment { + GITHUB_REPOSITORY_OWNER?: string + GITHUB_REPOSITORY?: string + GITHUB_EVENT_PATH?: string + GITHUB_TOKEN?: string + GITHUB_API_URL?: string +} + +interface MonitorOptions { + env?: GitHubActionsEnvironment + fetchImpl?: typeof fetch + log?: (message: string) => void +} + +class GitHubAPIError extends Error { + readonly status: number + + constructor(message: string, status: number) { + super(message) + this.name = 'GitHubAPIError' + this.status = status + } +} + +export function markdownTableCells(line: string): string[] { + return line + .trim() + .replace(/^\|/, '') + .replace(/\|$/, '') + .split('|') + .map((cell) => cell.trim()) +} + +export function normalizedCheckName(value: string): string { + return value.replace(/[^a-z0-9]+/gi, ' ').trim().toLowerCase() +} + +export function reviewGuidanceCheckName(line: string): string | null { + const [rawCheckName = '', status = ''] = markdownTableCells(line) + const checkName = rawCheckName.replace(/^\[ignored\]\s*/i, '') + const normalizedName = normalizedCheckName(checkName) + const isRelevantCheck = REVIEW_GUIDANCE_PREFIXES.some((prefix) => normalizedName.startsWith(prefix)) + const needsMaintainerAttention = /❌/u.test(status) || /\berror\b/i.test(status) + + if (!isRelevantCheck || !needsMaintainerAttention) return null + return checkName +} + +export function reviewGuidanceChecks(text: string): string[] { + return text.split('\n').flatMap((line) => { + const name = reviewGuidanceCheckName(line) + return name ? [name] : [] + }) +} + +export function eventContext(event: GitHubEvent): EventContext { + if (event.review && event.pull_request) { + return { + issueNumber: event.pull_request.number, + shouldInspect: event.review.state === 'changes_requested', + text: `${event.review.state ?? ''}\n${event.review.body ?? ''}` + } + } + + if (event.comment && event.issue?.pull_request) { + return { + issueNumber: event.issue.number, + shouldInspect: true, + text: event.comment.body ?? '' + } + } + + return { shouldInspect: false, text: '' } +} + +async function github( + apiURL: string, + token: string, + path: string, + fetchImpl: typeof fetch, + options: RequestInit = {} +): Promise { + const response = await fetchImpl(`${apiURL}${path}`, { + ...options, + headers: { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${token}`, + 'X-GitHub-Api-Version': '2022-11-28', + ...options.headers + } + }) + + if (!response.ok) { + const body = await response.text() + throw new GitHubAPIError( + `${options.method ?? 'GET'} ${path} failed: ${response.status} ${body}`, + response.status + ) + } + + if (response.status === 204) return null + return response.json() as Promise +} + +function repositoryName(repository?: string): string | undefined { + return repository?.split('/')[1] +} + +function requiredEnvironment(env: GitHubActionsEnvironment) { + const owner = env.GITHUB_REPOSITORY_OWNER + const repo = repositoryName(env.GITHUB_REPOSITORY) + const eventPath = env.GITHUB_EVENT_PATH + const token = env.GITHUB_TOKEN + const apiURL = env.GITHUB_API_URL ?? 'https://api.github.com' + + if (!owner || !repo || !eventPath || !token) { + throw new Error('Missing required GitHub Actions environment') + } + + return { apiURL, eventPath, owner, repo, token } +} + +export async function monitorPRReviewGuidance(options: MonitorOptions = {}): Promise { + const env = requiredEnvironment(options.env ?? process.env) + const log = options.log ?? ((message: string) => process.stdout.write(`${message}\n`)) + const fetchImpl = options.fetchImpl ?? fetch + const event = JSON.parse(await readFile(env.eventPath, 'utf8')) as GitHubEvent + const sender = event.sender?.login ?? '' + + if (!CODE_RABBIT_AUTHORS.has(sender)) { + log(`No action needed: event sender is ${sender || 'unknown'}, not CodeRabbit.`) + return + } + + const context = eventContext(event) + if (!context.issueNumber || !context.shouldInspect) { + log('No PR review guidance signal found.') + return + } + + const checks = reviewGuidanceChecks(context.text) + if (checks.length === 0) { + log('CodeRabbit did not report a PR description/readability check that needs maintainer attention.') + return + } + + const pr = await github( + env.apiURL, + env.token, + `/repos/${env.owner}/${env.repo}/pulls/${context.issueNumber}`, + fetchImpl + ) + if (!pr) throw new Error(`PR #${context.issueNumber} returned no data`) + + log( + [ + `CodeRabbit review guidance noted on #${context.issueNumber}: ${checks.join(', ')}`, + `Author: ${pr.user.login} (${pr.author_association})`, + `Title: ${pr.title}`, + 'No automatic label, comment, or close was applied. This is only a maintainer review note.' + ].join('\n') + ) +} + +const isDirectRun = process.argv[1] ? import.meta.url === pathToFileURL(process.argv[1]).href : false + +if (isDirectRun) { + await monitorPRReviewGuidance() +} diff --git a/tools/pr-review-guidance/tests/index.test.ts b/tools/pr-review-guidance/tests/index.test.ts new file mode 100644 index 000000000..de7792ced --- /dev/null +++ b/tools/pr-review-guidance/tests/index.test.ts @@ -0,0 +1,144 @@ +import { mkdtemp, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { describe, expect, test } from 'bun:test' + +import { + eventContext, + monitorPRReviewGuidance, + reviewGuidanceCheckName, + reviewGuidanceChecks +} from '../src/index' + +function response(body: unknown, init: ResponseInit = {}) { + return new Response(JSON.stringify(body), { + headers: { 'content-type': 'application/json' }, + ...init + }) +} + +async function writeEvent(event: unknown) { + const dir = await mkdtemp(join(tmpdir(), 'open-pencil-pr-guidance-')) + const path = join(dir, 'event.json') + await writeFile(path, JSON.stringify(event), 'utf8') + return path +} + +describe('reviewGuidanceCheckName', () => { + test('accepts legacy PR Hygiene errors from CodeRabbit tables', () => { + expect(reviewGuidanceCheckName('| PR Hygiene: Template | ❌ Error | Missing template |')).toBe( + 'PR Hygiene: Template' + ) + }) + + test('accepts softer PR Description check names', () => { + expect(reviewGuidanceCheckName('| PR Description: Context | Error | Add validation |')).toBe( + 'PR Description: Context' + ) + }) + + test('ignores unrelated or passing checks', () => { + expect(reviewGuidanceCheckName('| Security | ❌ Error | Something else |')).toBeNull() + expect(reviewGuidanceCheckName('| PR Description: Context | ✅ Pass | Fine |')).toBeNull() + }) +}) + +describe('reviewGuidanceChecks', () => { + test('extracts only relevant failed checks', () => { + const checks = reviewGuidanceChecks(` +| Check name | Status | Explanation | +|---|---|---| +| PR Description: Template | ❌ Error | Missing template | +| Security | ❌ Error | Not relevant | +| PR Readability: English | Error | Needs English | +`) + + expect(checks).toEqual(['PR Description: Template', 'PR Readability: English']) + }) +}) + +describe('eventContext', () => { + test('reads pull request review events', () => { + expect( + eventContext({ + pull_request: { number: 42 }, + review: { state: 'changes_requested', body: 'review body' } + }) + ).toEqual({ issueNumber: 42, shouldInspect: true, text: 'changes_requested\nreview body' }) + }) + + test('reads pull request issue comments', () => { + expect( + eventContext({ + issue: { number: 42, pull_request: {} }, + comment: { body: 'comment body' } + }) + ).toEqual({ issueNumber: 42, shouldInspect: true, text: 'comment body' }) + }) +}) + +describe('monitorPRReviewGuidance', () => { + test('logs a maintainer note and only reads PR data', async () => { + const eventPath = await writeEvent({ + sender: { login: 'coderabbitai[bot]' }, + issue: { number: 294, pull_request: {} }, + comment: { + body: '| Check name | Status | Explanation |\n|---|---|---|\n| PR Description: Template | ❌ Error | Missing template |' + } + }) + const requests: string[] = [] + const messages: string[] = [] + + await monitorPRReviewGuidance({ + env: { + GITHUB_API_URL: 'https://example.test', + GITHUB_EVENT_PATH: eventPath, + GITHUB_REPOSITORY: 'open-pencil/open-pencil', + GITHUB_REPOSITORY_OWNER: 'open-pencil', + GITHUB_TOKEN: 'token' + }, + fetchImpl: (async (input, init) => { + requests.push(`${init?.method ?? 'GET'} ${String(input)}`) + return response({ + author_association: 'CONTRIBUTOR', + state: 'open', + title: 'feat(dev-install): add script', + user: { login: 'joeycumines' } + }) + }) satisfies typeof fetch, + log: (message) => messages.push(message) + }) + + expect(requests).toEqual(['GET https://example.test/repos/open-pencil/open-pencil/pulls/294']) + expect(messages.join('\n')).toContain('No automatic label, comment, or close was applied') + }) + + test('ignores non-CodeRabbit comments', async () => { + const eventPath = await writeEvent({ + sender: { login: 'contributor' }, + issue: { number: 294, pull_request: {} }, + comment: { body: '| PR Description: Template | ❌ Error | Missing template |' } + }) + const requests: string[] = [] + const messages: string[] = [] + + await monitorPRReviewGuidance({ + env: { + GITHUB_API_URL: 'https://example.test', + GITHUB_EVENT_PATH: eventPath, + GITHUB_REPOSITORY: 'open-pencil/open-pencil', + GITHUB_REPOSITORY_OWNER: 'open-pencil', + GITHUB_TOKEN: 'token' + }, + fetchImpl: (async (input, init) => { + requests.push(`${init?.method ?? 'GET'} ${String(input)}`) + return response({}) + }) satisfies typeof fetch, + log: (message) => messages.push(message) + }) + + expect(requests).toEqual([]) + expect(messages).toEqual(['No action needed: event sender is contributor, not CodeRabbit.']) + }) +})