diff --git a/.env.example b/.env.example index 95a7506..30dc12c 100644 --- a/.env.example +++ b/.env.example @@ -3,6 +3,8 @@ ANTHROPIC_API_KEY=your_anthropic_api_key_here # GitHub GITHUB_TOKEN=your_github_personal_access_token_here TARGET_GITHUB_USERNAME=your_github_username_here +# PR review: absolute path to a local clone of the repo whose PRs you review +REPO_PATH=/absolute/path/to/local/clone/of/the/repo # Your personal oh-my-workers DB DATABASE_URL=postgresql://user:password@localhost:5432/work_coordinator # Company DB (update these when you change jobs) diff --git a/.gitignore b/.gitignore index 3c5a511..8e84dff 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ data/ wiki/ src/assets/slides_to_video/ src/scripts/test-registry.ts +dist/ \ No newline at end of file diff --git a/package.json b/package.json index 0a42408..37f9d9a 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,7 @@ "news": "pnpm run dev --job=news", "jobs": "pnpm run dev --list-jobs", "seed-mock": "node --loader ts-node/esm src/scripts/seed-mock-users.ts", + "review:dev": "node --loader ts-node/esm src/scripts/run-pr-review.ts", "format": "prettier --write \"src/**/*.ts\"", "format:check": "prettier --check \"src/**/*.ts\"" }, diff --git a/src/agent/pr-review.agent.ts b/src/agent/pr-review.agent.ts new file mode 100644 index 0000000..0132e69 --- /dev/null +++ b/src/agent/pr-review.agent.ts @@ -0,0 +1,68 @@ +import { createAgent, toolCallLimitMiddleware } from 'langchain' +import { ChatAnthropic } from '@langchain/anthropic' +import { AIMessage } from '@langchain/core/messages' +import { getPrDiffTool, readFileTool, searchCodeTool } from '../tools/pr-review.tool.js' +import { PR_REVIEW_PROMPT } from './prompt.js' +// import { REVIEW_LLM, MAX_REVIEW_TOOL_CALLS } from '../constants/index.js' +import { DEFAULT_LLM, MAX_REVIEW_TOOL_CALLS } from '../constants/index.js' +import { ReviewResultSchema, type ReviewResult } from '../schemas/index.js' +import { parsePrUrl } from '../utils/pr-url.js' + +const llm = new ChatAnthropic({ model: DEFAULT_LLM, temperature: 0 }) + +// Unlike the single-call fetch agents, the reviewer loops: read the diff, pull in +// context with read_file/search_code, then judge. The tool-call limit caps that loop. +// +// NOTE: we deliberately do NOT use createAgent's `responseFormat` here. In this langchain +// build, combining structured-output machinery with an afterModel middleware produces +// concurrent writes to the internal `jumpTo` channel (INVALID_CONCURRENT_GRAPH_UPDATE). +// Instead the agent returns a plain-text review, and we convert it to the typed +// ReviewResult in a separate withStructuredOutput call below. +export const prReviewAgent = createAgent({ + model: llm, + tools: [getPrDiffTool, readFileTool, searchCodeTool], + systemPrompt: PR_REVIEW_PROMPT, + middleware: [toolCallLimitMiddleware({ runLimit: MAX_REVIEW_TOOL_CALLS, exitBehavior: 'end' })], +}) + +// A separate, single-shot model call that turns the agent's free-text review into the +// typed ReviewResult. Runs outside the agent graph, so it avoids the jumpTo conflict. +const structuredLlm = new ChatAnthropic({ model: DEFAULT_LLM, temperature: 0 }).withStructuredOutput(ReviewResultSchema) + +// Extract the text of the agent's final assistant message (content may be a string or +// an array of content blocks). +function finalMessageText(messages: Array<{ content: unknown }>): string { + const last = [...messages].reverse().find((m) => AIMessage.isInstance(m)) + const content = last?.content + + if (typeof content === 'string') return content + if (Array.isArray(content)) { + return content.map((block) => (typeof block === 'object' && block !== null && 'text' in block ? (block as { text: string }).text : '')).join('') + } + + return '' +} + +// Orchestrator shared by the Task 1 dev script and the future Task 2 CLI. +export async function reviewPullRequest(prUrl: string): Promise { + const { owner, repo, number } = parsePrUrl(prUrl) + + console.log(`๐Ÿ” Reviewing ${owner}/${repo} PR #${number} ...\n`) + + const result = await prReviewAgent.invoke({ + messages: [ + { + role: 'user', + content: `Review this pull request. owner="${owner}", repo="${repo}", prNumber=${number}. Start by calling get_pr_diff, then investigate the surrounding code before reporting any issues.`, + }, + ], + }) + + const reviewText = finalMessageText(result.messages) + + console.log('\n๐Ÿงฉ Structuring findings...\n') + + return structuredLlm.invoke( + `Convert the following code-review notes into the structured schema. Preserve every finding exactly; do not invent new ones.\n\n${reviewText}` + ) +} diff --git a/src/agent/prompt.ts b/src/agent/prompt.ts index f36d7db..97ab276 100644 --- a/src/agent/prompt.ts +++ b/src/agent/prompt.ts @@ -54,3 +54,29 @@ When curating: Call curate_trending_repos with the result immediately.` export const TRENDING_TELEGRAM_PROMPT = `You are a Telegram delivery agent. Your only job is to call send_trending_telegram with the repos provided. Send it immediately and return the result.` + +export const PR_REVIEW_PROMPT = `You are a senior code reviewer with deep expertise in TypeScript, JavaScript, Node.js, Vue.js and NestJS. You review a single pull request and report only genuine BUGS, SECURITY issues, and PERFORMANCE problems. Do NOT comment on style, formatting, or naming. + +You have three tools: +- get_pr_diff โ€” fetch the PR's changed files and diffs. Call this FIRST. +- read_file โ€” read any file from the local checkout (callers, definitions, types, config). +- search_code โ€” find where a symbol is defined or used across the codebase. + +How to review โ€” investigate before judging: +1. Call get_pr_diff to see exactly what changed. +2. For each meaningful change, DO NOT judge from the diff alone. Pull in context first: + - read_file the file being changed (the diff only shows a slice) and the files/functions/types it touches. + - search_code for callers and definitions to understand how the changed code is actually used. +3. Only after you understand the surrounding code, decide whether something is a real issue. Examples worth reporting: + - Bugs: null/undefined dereferences, wrong/edge-case logic, broken contracts with callers, unhandled promise rejections, incorrect async/await, off-by-one, type mismatches that survive at runtime. + - Security: injection (SQL/command/XSS), missing authz/authn, secret/PII leakage, unsafe deserialization, SSRF, path traversal. + - Performance: N+1 queries, unbounded loops/allocations, blocking the event loop, missing pagination, redundant network/db calls. + +Rules: +- Report a finding ONLY if you are confident it is a real problem after inspecting the surrounding code. When in doubt, leave it out โ€” false positives waste the reviewer's time. +- Every finding must cite the specific file and line in the changed code. +- Explain WHY it is a problem, referencing the context you read (e.g. "parseUser() returns null when the row is missing โ€” see src/db/user.ts:42"). +- Give a concrete suggested fix. +- If you find no genuine issues, say so clearly. + +When done investigating, write your final answer as plain text: a one-paragraph overall summary, then a numbered list of findings. For EACH finding include severity (critical/high/medium/low), category (bug/security/performance), the file path and line number, a short title, an explanation of why it is a real problem, and a concrete suggested fix.` diff --git a/src/constants/index.ts b/src/constants/index.ts index 10d1fa5..c27632f 100644 --- a/src/constants/index.ts +++ b/src/constants/index.ts @@ -1,4 +1,6 @@ -export const DEFAULT_LLM = 'claude-haiku-4-5-20251001' +export const DEFAULT_LLM = 'claude-haiku-4-5' +export const REVIEW_LLM = 'claude-sonnet-4-6' // stronger model for code review reasoning (bugs/security/performance) +export const MAX_REVIEW_TOOL_CALLS = 30 // cap how many times the review agent can read/search context before deciding export const COMPANY_CLEANUP_TABLE = 'mockTestUsers' export const COMPANY_CLEANUP_THRESHOLD_DAYS = '30' export const DEFAULT_CRONJOB_TIME = '0 17 * * *' diff --git a/src/schemas/index.ts b/src/schemas/index.ts index 9a5122f..f11c307 100644 --- a/src/schemas/index.ts +++ b/src/schemas/index.ts @@ -78,6 +78,22 @@ export const TrendingRepoLogSchema = TrendingRepoSchema.extend({ updated_at: z.string(), }) +// --- PR code review (bugs + security + performance) --- +export const ReviewFindingSchema = z.object({ + severity: z.enum(['critical', 'high', 'medium', 'low']), + category: z.enum(['bug', 'security', 'performance']), + file: z.string().describe('Repo-relative path of the file the issue is in'), + line: z.number().describe('Line number in the changed file where the issue occurs'), + title: z.string().describe('Short one-line summary of the issue'), + explanation: z.string().describe('Why this is a real issue, referencing the surrounding code that was inspected'), + suggestion: z.string().describe('Concrete fix or change to resolve the issue'), +}) + +export const ReviewResultSchema = z.object({ + summary: z.string().describe('One-paragraph overall assessment of the PR'), + findings: z.array(ReviewFindingSchema), +}) + // TypeScript types inferred from schemas export type GitHubDigest = z.infer export type GithubKpiInput = z.infer @@ -87,3 +103,5 @@ export type DiaryEntry = z.infer export type CleanupResult = z.infer export type TrendingRepo = z.infer export type TrendingRepoLog = z.infer +export type ReviewFinding = z.infer +export type ReviewResult = z.infer diff --git a/src/scripts/run-pr-review.ts b/src/scripts/run-pr-review.ts new file mode 100644 index 0000000..f1abf49 --- /dev/null +++ b/src/scripts/run-pr-review.ts @@ -0,0 +1,17 @@ +import 'dotenv/config' +import { reviewPullRequest } from '../agent/pr-review.agent.js' +import { formatReview } from '../utils/review-formatter.js' + +// Task 1: hardcode the PR URL here. CLI arg parsing comes in Task 2. +// Use a real PR you can access; make sure REPO_PATH points at a local clone with the PR's branch checked out. +const PR_URL = 'https://github.com/DamengRandom/oh-my-workers/pull/3/changes' + +async function main(): Promise { + const result = await reviewPullRequest(PR_URL) + console.log('\n' + formatReview(result) + '\n') +} + +main().catch((err) => { + console.error('Fatal error during PR review:', err instanceof Error ? err.message : err) + process.exit(1) +}) diff --git a/src/tools/pr-review.tool.ts b/src/tools/pr-review.tool.ts new file mode 100644 index 0000000..ac8ea91 --- /dev/null +++ b/src/tools/pr-review.tool.ts @@ -0,0 +1,148 @@ +import { DynamicStructuredTool } from '@langchain/core/tools' +import { Octokit } from '@octokit/rest' +import { execFile } from 'node:child_process' +import { promisify } from 'node:util' +import { readFile } from 'node:fs/promises' +import { realpathSync } from 'node:fs' +import path from 'node:path' +import { z } from 'zod' + +const execFileAsync = promisify(execFile) + +const octokit = new Octokit({ auth: process.env.GITHUB_TOKEN }) + +// Resolve the local clone root once. Every file/search tool is sandboxed to this directory. +function repoRoot(): string { + const root = process.env.REPO_PATH + if (!root) throw new Error('REPO_PATH is not set โ€” point it at the local clone of the repo being reviewed.') + return path.resolve(root) +} + +// Resolve a repo-relative path and refuse anything that escapes the clone (path-traversal guard). +// path.resolve() is purely lexical and does NOT follow symlinks, so we resolve the real path +// (via realpathSync) before checking containment โ€” otherwise a symlink committed inside the repo +// and pointing outside (e.g. link -> /etc) could be used to read files beyond REPO_PATH. +function resolveInsideRepo(relativePath: string): string { + const root = realpathSync(repoRoot()) + const resolved = path.resolve(root, relativePath) + + // Follow symlinks for paths that exist. If the path doesn't exist there is nothing to read, + // so the lexical check on `resolved` is sufficient (path.resolve already normalises any '..'). + let real = resolved + try { + real = realpathSync(resolved) + } catch { + // non-existent path โ€” fall through to the lexical check below + throw new Error(`Refusing to read "${relativePath}" โ€” it does not exist.`) + } + + if (real !== root && !real.startsWith(root + path.sep)) { + throw new Error(`Refusing to read "${relativePath}" โ€” it resolves outside REPO_PATH.`) + } + return resolved +} + +// โ”€โ”€ get_pr_diff: the one network call โ€” fetch what changed in the PR โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ +export const getPrDiffTool = new DynamicStructuredTool({ + name: 'get_pr_diff', + description: + 'Fetches the changed files and their diffs (patches) for a pull request, plus the base/head branches. Call this FIRST to see what the PR changed.', + schema: z.object({ + owner: z.string().describe('Repository owner (org or user)'), + repo: z.string().describe('Repository name'), + prNumber: z.number().describe('Pull request number'), + }), + func: async ({ owner, repo, prNumber }) => { + const { data: pr } = await octokit.rest.pulls.get({ owner, repo, pull_number: prNumber }) + const files = await octokit.paginate(octokit.rest.pulls.listFiles, { owner, repo, pull_number: prNumber, per_page: 100 }) + + const result = { + title: pr.title, + baseBranch: pr.base.ref, + headBranch: pr.head.ref, + changedFiles: files.map((f) => ({ + filename: f.filename, + status: f.status, + additions: f.additions, + deletions: f.deletions, + patch: f.patch ?? '(no textual diff โ€” binary or too large)', + })), + } + + console.log(`โœ… get_pr_diff: ${result.changedFiles.length} changed file(s) in PR #${prNumber} (${result.headBranch} โ†’ ${result.baseBranch})`) + + return JSON.stringify(result) + }, +}) + +// โ”€โ”€ read_file: read any file from the local clone to gather context โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ +export const readFileTool = new DynamicStructuredTool({ + name: 'read_file', + description: + 'Reads a file from the local checkout of the repository so you can inspect the surrounding code (callers, definitions, types) before deciding whether a change is a real issue. Path is relative to the repo root.', + schema: z.object({ + path: z.string().describe('Repo-relative path of the file to read, e.g. "src/utils/parser.ts"'), + }), + func: async ({ path: relativePath }) => { + const absolutePath = resolveInsideRepo(relativePath) + + let contents: string + try { + contents = await readFile(absolutePath, 'utf8') + } catch { + console.log(`โš ๏ธ read_file: could not read "${relativePath}"`) + return JSON.stringify({ path: relativePath, error: 'File not found or unreadable in the local clone.' }) + } + + // Prefix line numbers so the agent can cite accurate file:line references. + const numbered = contents + .split('\n') + .map((line, i) => `${i + 1}\t${line}`) + .join('\n') + + console.log(`โœ… read_file: ${relativePath} (${contents.split('\n').length} lines)`) + + return JSON.stringify({ path: relativePath, contents: numbered }) + }, +}) + +// โ”€โ”€ search_code: grep the local clone to find callers / definitions โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ +export const searchCodeTool = new DynamicStructuredTool({ + name: 'search_code', + description: + 'Searches the local checkout for a string or symbol (e.g. a function name) to locate its definition and callers. Use this to understand how changed code is used elsewhere before judging it.', + schema: z.object({ + query: z.string().describe('Literal text or symbol to search for'), + glob: z.string().optional().describe('Optional file glob to narrow the search, e.g. "*.ts"'), + }), + func: async ({ query, glob }) => { + const root = repoRoot() + // --no-follow is ripgrep's default; we pin it explicitly so the search can never be made to + // traverse a symlink out of the repo (defence-in-depth, matching the read_file guard). + const args = ['--line-number', '--no-heading', '--color', 'never', '--max-count', '50', '--fixed-strings', '--no-follow'] + if (glob) args.push('--glob', glob) + args.push('--', query, root) + + try { + const { stdout } = await execFileAsync('rg', args, { maxBuffer: 1024 * 1024 }) + // Trim absolute prefix back to repo-relative paths for readability. + const matches = stdout + .split('\n') + .filter(Boolean) + .slice(0, 100) + .map((line) => line.replace(`${root}${path.sep}`, '')) + + console.log(`โœ… search_code: "${query}" โ†’ ${matches.length} match(es)`) + return JSON.stringify({ query, matches }) + } catch (err) { + // ripgrep exits with code 1 when there are no matches โ€” that's not an error for us. + const e = err as { code?: number } + if (e.code === 1) { + console.log(`โœ… search_code: "${query}" โ†’ 0 matches`) + return JSON.stringify({ query, matches: [] }) + } + console.log(`โš ๏ธ search_code failed for "${query}"`) + return JSON.stringify({ query, error: 'Search failed.' }) + } + }, +}) diff --git a/src/utils/pr-url.ts b/src/utils/pr-url.ts new file mode 100644 index 0000000..a54b5f1 --- /dev/null +++ b/src/utils/pr-url.ts @@ -0,0 +1,22 @@ +// Parses a GitHub pull request URL into its owner / repo / number parts. +// Basic validation only โ€” full guardrails (size limits, richer input handling) are deferred to a later task. + +export type ParsedPr = { + owner: string + repo: string + number: number +} + +const PR_URL_REGEX = /github\.com\/([^/\s]+)\/([^/\s]+)\/pull\/(\d+)/i + +export function parsePrUrl(url: string): ParsedPr { + const match = url.trim().match(PR_URL_REGEX) + + if (!match) { + throw new Error(`Invalid GitHub PR URL: "${url}". Expected a URL like https://github.com///pull/`) + } + + const [, owner, repo, number] = match + + return { owner, repo, number: Number(number) } +} diff --git a/src/utils/review-formatter.ts b/src/utils/review-formatter.ts new file mode 100644 index 0000000..05e6651 --- /dev/null +++ b/src/utils/review-formatter.ts @@ -0,0 +1,57 @@ +import type { ReviewResult, ReviewFinding } from '../schemas/index.js' + +// Minimal ANSI colors โ€” no extra dependency. +const COLORS = { + reset: '\x1b[0m', + bold: '\x1b[1m', + dim: '\x1b[2m', + red: '\x1b[31m', + yellow: '\x1b[33m', + blue: '\x1b[34m', + gray: '\x1b[90m', +} as const + +const SEVERITY_ORDER: ReviewFinding['severity'][] = ['critical', 'high', 'medium', 'low'] + +const SEVERITY_COLOR: Record = { + critical: COLORS.red, + high: COLORS.red, + medium: COLORS.yellow, + low: COLORS.blue, +} + +function color(text: string, c: string): string { + return `${c}${text}${COLORS.reset}` +} + +export function formatReview(result: ReviewResult): string { + const lines: string[] = [] + + lines.push(color('PR Review', COLORS.bold)) + lines.push('') + lines.push(result.summary) + lines.push('') + + if (result.findings.length === 0) { + lines.push(color('โœ… No bugs, security, or performance issues found.', COLORS.blue)) + return lines.join('\n') + } + + // Stable ordering: most severe first. + const sorted = [...result.findings].sort((a, b) => SEVERITY_ORDER.indexOf(a.severity) - SEVERITY_ORDER.indexOf(b.severity)) + + lines.push(color(`Found ${sorted.length} issue(s):`, COLORS.bold)) + lines.push('') + + sorted.forEach((f, i) => { + const tag = color(`${f.severity.toUpperCase()} ยท ${f.category}`, SEVERITY_COLOR[f.severity]) + const location = color(`${f.file}:${f.line}`, COLORS.gray) + lines.push(`${color(`${i + 1}.`, COLORS.bold)} ${tag} โ€” ${color(f.title, COLORS.bold)}`) + lines.push(` ${location}`) + lines.push(` ${f.explanation}`) + lines.push(` ${color('Suggestion:', COLORS.dim)} ${f.suggestion}`) + lines.push('') + }) + + return lines.join('\n') +}