diff --git a/.gitignore b/.gitignore index fc175568df68..02142ddcbbb1 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,5 @@ target # Local dev files opencode-dev logs/ +code/ +pr_diff.txt diff --git a/packages/opencode/src/ci.ts b/packages/opencode/src/ci.ts new file mode 100644 index 000000000000..9025d00a929c --- /dev/null +++ b/packages/opencode/src/ci.ts @@ -0,0 +1,109 @@ +/** + * Headless entry point for GitHub Actions / CI environments + * This avoids loading TUI dependencies (React, SolidJS, etc.) + */ +import yargs from "yargs" +import { hideBin } from "yargs/helpers" +import { Log } from "./util/log" +import { UI } from "./cli/ui" +import { Installation } from "./installation" +import { NamedError } from "@opencode-ai/util/error" +import { FormatError } from "./cli/error" +import { GithubCommand } from "./cli/cmd/github" +import { EOL } from "os" + +process.on("unhandledRejection", (e) => { + Log.Default.error("rejection", { + e: e instanceof Error ? e.message : e, + }) +}) + +process.on("uncaughtException", (e) => { + Log.Default.error("exception", { + e: e instanceof Error ? e.message : e, + }) +}) + +const cli = yargs(hideBin(process.argv)) + .parserConfiguration({ "populate--": true }) + .scriptName("opencode") + .wrap(100) + .help("help", "show help") + .alias("help", "h") + .version("version", "show version number", Installation.VERSION) + .alias("version", "v") + .option("print-logs", { + describe: "print logs to stderr", + type: "boolean", + }) + .option("log-level", { + describe: "log level", + type: "string", + choices: ["DEBUG", "INFO", "WARN", "ERROR"], + }) + .middleware(async (opts) => { + await Log.init({ + print: process.argv.includes("--print-logs"), + dev: Installation.isLocal(), + level: (() => { + if (opts.logLevel) return opts.logLevel as Log.Level + if (Installation.isLocal()) return "DEBUG" + return "INFO" + })(), + }) + + process.env.AGENT = "1" + process.env.OPENCODE = "1" + + Log.Default.info("opencode-ci", { + version: Installation.VERSION, + args: process.argv.slice(2), + }) + }) + .usage("\nopencode CI/GitHub Actions runner") + .command(GithubCommand) + .demandCommand(1, "You must specify a command") + .strict() + +try { + await cli.parse() +} catch (e) { + let data: Record = {} + if (e instanceof NamedError) { + const obj = e.toObject() + Object.assign(data, { + ...obj.data, + }) + } + + if (e instanceof Error) { + Object.assign(data, { + name: e.name, + message: e.message, + cause: e.cause?.toString(), + stack: e.stack, + }) + } + + if (e instanceof ResolveMessage) { + Object.assign(data, { + name: e.name, + message: e.message, + code: e.code, + specifier: e.specifier, + referrer: e.referrer, + position: e.position, + importKind: e.importKind, + }) + } + Log.Default.error("fatal", data) + const formatted = FormatError(e) + if (formatted) UI.error(formatted) + if (formatted === undefined) { + UI.error("Unexpected error, check log file at " + Log.file() + " for more details" + EOL) + console.error(e) + } + process.exitCode = 1 +} finally { + process.exit() +} diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 26e0fb73dc33..0e2a69d527b6 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -27,6 +27,12 @@ import { Bus } from "../../bus" import { MessageV2 } from "../../session/message-v2" import { SessionPrompt } from "@/session/prompt" import { $ } from "bun" +import { ContextInjector, ReviewComment } from "../../context" +import { generateObject } from "ai" +import z from "zod" +import { repairAndParseJson } from "../../util/json-repair" +import { parsePatchForValidLines } from "../../util/git-diff" +import { renderReviewMarkdown, filterCommentsByDiff } from "../../util/github-review-logic" type GitHubAuthor = { login: string @@ -176,16 +182,17 @@ export function extractResponseText(parts: MessageV2.Part[]): string | null { const toolParts = parts.filter((p) => p.type === "tool" && p.state.status === "completed") if (toolParts.length > 0) return null - // No usable parts - throw with debug info - const partTypes = parts.map((p) => p.type).join(", ") || "none" - throw new Error(`Failed to parse response. Part types found: [${partTypes}]`) + // Priority 4: Step parts or other unknown parts + // When Gemini 3.0+ uses tools or thinks, it may emit step-start/step-finish or other types. + // We return null to signal summary needed if there is no text. + return null } export const GithubCommand = cmd({ command: "github", describe: "manage GitHub agent", builder: (yargs) => yargs.command(GithubInstallCommand).command(GithubRunCommand).demandCommand(), - async handler() {}, + async handler() { }, }) export const GithubInstallCommand = cmd({ @@ -234,9 +241,14 @@ export const GithubInstallCommand = cmd({ ` 1. Commit the \`${WORKFLOW_FILE}\` file and push`, step2, "", - " 3. Go to a GitHub issue and comment `/oc summarize` to see the agent in action", + " 3. Ensure your self-hosted runner has:", + " - Bun installed (https://bun.sh)", + " - Git installed", + " - Network access to github.com", "", - " Learn more about the GitHub agent - https://opencode.ai/docs/github/#usage-examples", + " 4. Go to a GitHub issue or PR and comment `/oc` to see the agent in action", + "", + " Note: This workflow uses rakamindev/opencode fork with self-hosted runners", ].join("\n"), ) } @@ -363,10 +375,9 @@ export const GithubInstallCommand = cmd({ } async function addWorkflowFiles() { - const envStr = - provider === "amazon-bedrock" - ? "" - : `\n env:${providers[provider].env.map((e) => `\n ${e}: \${{ secrets.${e} }}`).join("")}` + const envSecrets = providers[provider].env + .map((e) => ` ${e}: \${{ secrets.${e} }}`) + .join("\n") await Bun.write( path.join(app.root, WORKFLOW_FILE), @@ -385,20 +396,35 @@ jobs: startsWith(github.event.comment.body, '/oc') || contains(github.event.comment.body, ' /opencode') || startsWith(github.event.comment.body, '/opencode') - runs-on: ubuntu-latest + runs-on: self-hosted permissions: id-token: write contents: read - pull-requests: read - issues: read + pull-requests: write + issues: write steps: - name: Checkout repository uses: actions/checkout@v4 + - name: Clone opencode + run: | + rm -rf /tmp/opencode + git clone --depth 1 https://github.com/rakamindev/opencode.git /tmp/opencode + + - name: Install dependencies + working-directory: /tmp/opencode + run: bun install + - name: Run opencode - uses: sst/opencode/github@latest${envStr} - with: - model: ${provider}/${model}`, + working-directory: \${{ github.workspace }} + env: + GITHUB_TOKEN: \${{ secrets.GITHUB_TOKEN }} + GITHUB_EVENT_NAME: \${{ github.event_name }} + GITHUB_EVENT_PATH: \${{ github.event_path }} + OPENCODE_MODEL: ${provider}/${model} +${envSecrets} + run: bun run /tmp/opencode/packages/opencode/src/index.ts github run +`, ) prompts.log.success(`Added workflow file: "${WORKFLOW_FILE}"`) @@ -464,9 +490,15 @@ export const GithubRunCommand = cmd({ : context.eventName === "issue_comment" || context.eventName === "issues" ? (payload as IssueCommentEvent | IssuesEvent).issue.number : (payload as PullRequestEvent | PullRequestReviewCommentEvent).pull_request.number - const runUrl = `/${owner}/${repo}/actions/runs/${runId}` + const runUrl = `https://github.com/${owner}/${repo}/actions/runs/${runId}` const shareBaseUrl = isMock ? "https://dev.opencode.ai" : "https://opencode.ai" + // Branch-aware strict review: only enforce concurrent rules on specified branches + const strictReviewBranches = (process.env["STRICT_REVIEW_BRANCHES"] || "") + .split(",") + .map((b) => b.trim().toLowerCase()) + .filter(Boolean) + let appToken: string let octoRest: Octokit let octoGraph: typeof graphql @@ -563,30 +595,40 @@ export const GithubRunCommand = cmd({ if (prData.headRepository.nameWithOwner === prData.baseRepository.nameWithOwner) { await checkoutLocalBranch(prData) const head = (await $`git rev-parse HEAD`).stdout.toString().trim() - const dataPrompt = buildPromptDataForPR(prData) + const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) const { dirty, uncommittedChanges } = await branchIsDirty(head) if (dirty) { const summary = await summarize(response) await pushToLocalBranch(summary, uncommittedChanges) } - const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl}/s/${shareId}`)) - await createComment(`${response}${footer({ image: !hasShared })}`) + const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl} / s / ${shareId}`)) + // Try to post inline review comments, fall back to regular comment + await parseAndPostInlineReview( + issueId!, + response, + footer({ image: !hasShared }) + ) await removeReaction(commentType) } // Fork PR else { await checkoutForkBranch(prData) const head = (await $`git rev-parse HEAD`).stdout.toString().trim() - const dataPrompt = buildPromptDataForPR(prData) + const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) const { dirty, uncommittedChanges } = await branchIsDirty(head) if (dirty) { const summary = await summarize(response) await pushToForkBranch(summary, prData, uncommittedChanges) } - const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl}/s/${shareId}`)) - await createComment(`${response}${footer({ image: !hasShared })}`) + const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl} / s / ${shareId}`)) + // Try to post inline review comments, fall back to regular comment + await parseAndPostInlineReview( + issueId!, + response, + footer({ image: !hasShared }) + ) await removeReaction(commentType) } } @@ -645,7 +687,7 @@ export const GithubRunCommand = cmd({ const { providerID, modelID } = Provider.parseModel(value) if (!providerID.length || !modelID.length) - throw new Error(`Invalid model ${value}. Model must be in the format "provider/model".`) + throw new Error(`Invalid model ${value}.Model must be in the format "provider/model".`) return { providerID, modelID } } @@ -660,7 +702,7 @@ export const GithubRunCommand = cmd({ if (!value) return undefined if (value === "true") return true if (value === "false") return false - throw new Error(`Invalid share value: ${value}. Share must be a boolean.`) + throw new Error(`Invalid share value: ${value}.Share must be a boolean.`) } function normalizeUseGithubToken() { @@ -668,7 +710,7 @@ export const GithubRunCommand = cmd({ if (!value) return false if (value === "true") return true if (value === "false") return false - throw new Error(`Invalid use_github_token value: ${value}. Must be a boolean.`) + throw new Error(`Invalid use_github_token value: ${value}.Must be a boolean.`) } function normalizeOidcBaseUrl(): string { @@ -703,6 +745,93 @@ export const GithubRunCommand = cmd({ position: reviewPayload.comment.position, commitId: reviewPayload.comment.commit_id, originalCommitId: reviewPayload.comment.original_commit_id, + // Thread detection: if in_reply_to_id exists, this is a reply in a thread + inReplyToId: (reviewPayload.comment as any).in_reply_to_id as number | undefined, + } + } + + /** + * Check if Phase 1 (clarifying questions) has already been asked for this PR + * OR if user has already used /oc! (direct review) - in either case, skip Phase 1 + */ + async function hasPhase1BeenAsked(): Promise { + if (!issueId) return false + + try { + // Fetch PR comments and reviews to check for previous engagement + const prData = await fetchPR() + + // Patterns to detect: + // 1. Bot's Phase 1 questions were already asked + // 2. User already used /oc! (direct review) = review exists + const phase1Pattern = /πŸ€”.*Before I review.*questions/i + const directReviewPattern = /\/(oc|opencode)!/i + + // Check issue-style comments + const comments = prData.comments?.nodes || [] + for (const comment of comments) { + if (comment.body) { + // Check if Phase 1 was asked (bot comment) + if (phase1Pattern.test(comment.body)) { + return true + } + // Check if user already used /oc! (user comment) + if (directReviewPattern.test(comment.body)) { + return true + } + } + } + + // Check review comments + const reviews = prData.reviews?.nodes || [] + for (const review of reviews) { + if (review.body && phase1Pattern.test(review.body)) { + return true + } + } + + return false + } catch (e) { + console.warn("Failed to check Phase 1 status:", e) + return false + } + } + + /** + * Get review strictness based on PR target branch. + * Returns whether concurrent rules should be blocking or info-only. + */ + async function getReviewStrictness(): Promise<{ + isStrict: boolean + targetBranch: string + message: string + }> { + if (!issueId) return { isStrict: false, targetBranch: "", message: "" } + + try { + const prData = await fetchPR() + const targetBranch = prData.baseRefName?.toLowerCase() || "" + + const isStrict = strictReviewBranches.length > 0 && strictReviewBranches.includes(targetBranch) + + // Debug logging for branch-aware strict review + console.log("=== BRANCH-AWARE STRICT REVIEW DEBUG ===") + console.log(`STRICT_REVIEW_BRANCHES env: "${process.env["STRICT_REVIEW_BRANCHES"] || "(not set)"}"`) + console.log(`Parsed strict branches: [${strictReviewBranches.join(", ")}]`) + console.log(`PR target branch: "${prData.baseRefName}" (normalized: "${targetBranch}")`) + console.log(`Is strict review: ${isStrict}`) + console.log("=========================================") + + const message = isStrict + ? `⚠️ This PR targets **${prData.baseRefName}** (protected branch). Concurrent implementation rules are ENFORCED.` + : strictReviewBranches.length > 0 + ? `ℹ️ This PR targets **${prData.baseRefName}** (non-protected). Concurrent rules shown as INFO only.` + : "" + + return { isStrict, targetBranch, message } + } catch (e) { + console.warn("Failed to get review strictness:", e) + return { isStrict: false, targetBranch: "", message: "" } } } @@ -726,25 +855,333 @@ export const GithubRunCommand = cmd({ .split(",") .map((m) => m.trim().toLowerCase()) .filter(Boolean) - let prompt = (() => { + let prompt = await (async () => { if (!isCommentEvent) { return "Review this pull request" } const body = (payload as IssueCommentEvent | PullRequestReviewCommentEvent).comment.body.trim() const bodyLower = body.toLowerCase() - if (mentions.some((m) => bodyLower === m)) { - if (reviewContext) { - return `Review this code change and suggest improvements for the commented lines:\n\nFile: ${reviewContext.file}\nLines: ${reviewContext.line}\n\n${reviewContext.diffHunk}` + if (mentions.some((m) => bodyLower === m) || mentions.some((m) => bodyLower === m + "!")) { + // Check for direct review trigger (/oc! or /opencode!) + const isDirectReview = mentions.some((m) => bodyLower === m + "!" || bodyLower.startsWith(m + "!")) + + if (isDirectReview) { + // /oc! β†’ Skip Phase 1, go directly to focused review + return `[DIRECT_REVIEW] Review this pull request directly without asking clarifying questions. Provide your complete review now.` } - return "Summarize this thread" + + // /oc β†’ Phase 1: Ask clarifying questions first + return `[PHASE_1] Before reviewing this pull request, analyze the changes and ask 2-4 clarifying questions to understand the context and focus areas. DO NOT provide the actual review yet - just ask focused questions. Example format: + +πŸ€” **Before I review, a few questions:** + +**PR Type Detected:** [Type based on files changed] + +**I noticed:** +- [Observation about what's in the PR] +- [Observation about what seems missing] + +**Questions:** +1. [Context question] +2. [Focus question] + +Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), or use \`/oc!\` to skip questions.` } + + // Handle /oc with additional text (user answering questions or providing context) if (mentions.some((m) => bodyLower.includes(m))) { - if (reviewContext) { - return `${body}\n\nContext: You are reviewing a comment on file "${reviewContext.file}" at line ${reviewContext.line}.\n\nDiff context:\n${reviewContext.diffHunk}` + // Check for recheck/rereview trigger first + const isRecheckTrigger = /\/(oc|opencode)\s*(recheck|rereview|check\s*again)/i.test(body) + if (isRecheckTrigger) { + const userMessage = body.replace(/\/(oc|opencode)\s*(recheck|rereview|check\s*again)/gi, "").trim() + return `[RECHECK] User is requesting a re-review after fixing previous feedback. + +User's context: ${userMessage || "Fixed previous issues"} + +CRITICAL INSTRUCTIONS: +1. You are a CODE REVIEWER, not a fixer. DO NOT attempt to apply code changes. +2. DO NOT return file edits, string replacements, or code arrays. +3. Output ONLY the review JSON format shown below. +4. If you want to suggest code changes, use the "suggestion" field in comments. + +Output ONLY valid JSON. NO explanations. NO text before or after JSON. + +\`\`\`json +{ + "context_summary": "Re-review: [brief context of what was fixed]", + "summary": "1-2 sentence status of previous feedback resolution", + + "checklist": [ + { + "item": "Previous issue name", + "passed": true, + "note": "βœ… Resolved / ❌ Still open / ⏭️ Skipped (if fixed in another PR per context)" + } + ], + + "comments": [ + { + "path": "src/path/to/file.js", + "start_line": 42, + "line": 55, + "body": "Issue description", + "severity": "error|warning|info|suggestion", + "suggestion": "// corrected code - will create committable suggestion" + } + ], + + "not_reviewed": [ + {"item": "Item name", "reason": "Already addressed / Not in scope"} + ], + + "decision": "APPROVE|REQUEST_CHANGES", + "decision_reason": "All issues resolved / X issues remain" +} +\`\`\` + +RULES: +- Output ONLY the JSON block, nothing else +- Use "path" not "file" for file paths +- Use "body" not "note" for comment text +- Use "severity" for each comment (error/warning/info/suggestion) +- If no inline comments needed, use empty array: "comments": [] +- Checklist items should track resolution of PREVIOUS issues +- IMPORTANT: Only comment on lines that are ADDED or MODIFIED in the PR diff. Do NOT comment on unchanged lines far from the changes - those will be rejected by GitHub API. +- CRITICAL: "suggestion" field must contain RAW CODE ONLY - NO markdown formatting, NO triple backticks, NO \`\`\`suggestion blocks. Just the plain replacement code. + +CONCURRENT IMPLEMENTATION RULES (model-migration-sync, controller-service, etc.): +${await (async () => { + const { isStrict, message } = await getReviewStrictness() + if (isStrict) { + return `- ${message} +- ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` + } else { + return ` +=== CRITICAL: NON-STRICT BRANCH - READ THIS FIRST === +${message || "No strict branches configured."} + +YOU MUST FOLLOW THESE RULES FOR THIS NON-PROTECTED BRANCH: +1. Do NOT mark missing concurrent deps (models, hooks, associations, schema definitions) as FAIL +2. If user says migrations/models/tests are in another PR, mark those checklist items as "passed": null (Skipped) +3. THIS INCLUDES "Previous Feedback": If the fix for a previous issue is in a separate PR (per context), mark it as "passed": null (Skipped), NOT as "Still open" or "Fail". +4. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too +5. Focus ONLY on reviewing the actual code IN THIS PR +6. Decision should be APPROVE if the code IN THIS PR is correct +7. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR + +CORRECT OUTPUT FOR NON-STRICT BRANCH: +- Checklist item for items in other PRs: {"item": "Criterion name", "passed": null, "note": "⏭️ Skipped - handled in separate PR per user context"} +- Decision: "APPROVE" (assuming code in this PR is correct) +- not_reviewed: List what was skipped and why +=== END CRITICAL SECTION ===` + } + })()}` + } + + // Check for direct review trigger first (/oc! anywhere in text) + const isDirectReview = mentions.some((m) => bodyLower.includes(m + "!")) + if (isDirectReview) { + const userMessage = body.replace(/\/oc!?|\/opencode!?/gi, "").trim() + return `[DIRECT_REVIEW] Review this pull request directly without asking clarifying questions. +User context: ${userMessage || "None provided"} + +CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. + +\`\`\`json +{ + "context_summary": "${userMessage || "Direct review requested"}", + "summary": "1-2 sentence summary of what this PR does", + + "checklist": [ + { + "item": "Criterion name (generate based on PR type - models, migrations, services, etc.)", + "passed": true, + "note": "Why it passed/failed - be specific" + } + ], + + "comments": [ + { + "path": "src/path/to/file.js", + "start_line": 42, + "line": 55, + "body": "Issue description", + "severity": "error|warning|info|suggestion", + "suggestion": "// corrected code - will create committable suggestion" + } + ], + + "general_observations": ["Any observations not tied to specific lines"], + + "not_reviewed": [ + {"item": "Thing not reviewed", "reason": "Per user context / Not in diff"} + ], + + "decision": "APPROVE|REQUEST_CHANGES", + "decision_reason": "Why this decision" +} +\`\`\` + +CHECKLIST GENERATION RULES: +- Generate 4-7 checklist items RELEVANT to this specific PR type +- If PR adds models: check schema correctness, associations, naming conventions +- If PR adds migrations: check column types, indexes, rollback safety +- If PR adds services: check business logic separation, error handling +- If PR adds controllers: check input validation, output formatting +- Be CONTEXT-AWARE, not generic + +CONCURRENT IMPLEMENTATION RULES (model-migration-sync, controller-service, etc.): +${await (async () => { + const { isStrict, message } = await getReviewStrictness() + if (isStrict) { + return `- ${message} +- ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` + } else { + return ` +=== CRITICAL: NON-STRICT BRANCH - READ THIS FIRST === +${message || "No strict branches configured."} + +YOU MUST FOLLOW THESE RULES FOR THIS NON-PROTECTED BRANCH: +1. Do NOT mark missing concurrent deps (models, hooks, associations, schema definitions) as FAIL +2. If user says migrations/models/tests are in another PR, mark those checklist items as "passed": null (Skipped) +3. THIS INCLUDES "Previous Feedback": If the fix for a previous issue is in a separate PR (per context), mark it as "passed": null (Skipped), NOT as "Still open" or "Fail". +4. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too +5. Focus ONLY on reviewing the actual code IN THIS PR +6. Decision should be APPROVE if the code IN THIS PR is correct +7. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR +=== END CRITICAL SECTION ===` + } + })()}` + } + + const userMessage = body.replace(/\/oc!?|\/opencode!?/gi, "").trim() + const hasNumberedAnswers = /^\s*\d+[\.\)]\s*.+/m.test(userMessage) + + // SMART THREAD DETECTION: + // If this is a threaded reply (in_reply_to_id exists), skip Phase 1 + // and go directly to Phase 2 with focused context + const isThreadedReply = reviewContext?.inReplyToId !== undefined + + // PHASE 1 ALREADY DONE: + // If Phase 1 questions were already asked, skip directly to Phase 2 + const phase1AlreadyAsked = await hasPhase1BeenAsked() + + if (isThreadedReply) { + // Threaded reply β†’ Skip Phase 1, use thread context for focused response + return `[THREAD_REPLY] User is replying in a code review thread. Provide a focused response based on the thread context. + +User's reply: ${userMessage || "Acknowledged"} + +Thread context: +- File: ${reviewContext?.file} +- Line: ${reviewContext?.line} +- Diff: +${reviewContext?.diffHunk} + +Respond directly to the user's message. If they've acknowledged a fix, confirm and close the thread. If they have questions, answer concisely. If they disagree, discuss the trade-offs. + +Keep your response focused on this specific issue only. Do NOT ask Phase 1 questions.` } - return body + + if (hasNumberedAnswers || phase1AlreadyAsked) { + // User is answering questions OR Phase 1 was already asked β†’ Phase 2 + return `[PHASE_2] ${hasNumberedAnswers ? "User has answered your clarifying questions." : "Phase 1 questions were already asked."} Now provide the focused review. + +User's context/answers: +${userMessage} + +CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. + +\`\`\`json +{ + "context_summary": "Summarize user's context/focus from their message", + "summary": "1-2 sentence summary of what this PR does", + + "checklist": [ + { + "item": "Criterion name (generate based on PR type - models, migrations, services, etc.)", + "passed": true, + "note": "Why it passed/failed - be specific" + } + ], + + "comments": [ + { + "path": "src/path/to/file.js", + "start_line": 42, + "line": 55, + "body": "Issue description", + "severity": "error|warning|info|suggestion", + "suggestion": "// corrected code - will create committable suggestion" + } + ], + + "general_observations": ["Any observations not tied to specific lines"], + + "not_reviewed": [ + {"item": "Thing not reviewed", "reason": "Per user context / Not in diff"} + ], + + "decision": "APPROVE|REQUEST_CHANGES", + "decision_reason": "Why this decision" +} +\`\`\` + +CHECKLIST GENERATION RULES: +- Generate 4-7 checklist items RELEVANT to this specific PR type +- If PR adds models: check schema correctness, associations, naming conventions +- If PR adds migrations: check column types, indexes, rollback safety +- If PR adds services: check business logic separation, error handling +- If PR adds controllers: check input validation, output formatting +- Be CONTEXT-AWARE based on user's answers, not generic +- Mark items NOT reviewed (per user context) with "skipped" status and reason + +CONCURRENT IMPLEMENTATION RULES (model-migration-sync, controller-service, etc.): +${await (async () => { + const { isStrict, message } = await getReviewStrictness() + if (isStrict) { + return `- ${message} +- ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` + } else { + return ` +=== CRITICAL: NON-STRICT BRANCH - READ THIS FIRST === +${message || "No strict branches configured."} + +YOU MUST FOLLOW THESE RULES FOR THIS NON-PROTECTED BRANCH: +1. Do NOT mark missing concurrent deps (models, hooks, associations, schema definitions) as FAIL +2. If user says migrations/models/tests are in another PR, mark those checklist items as "passed": null (Skipped) +3. THIS INCLUDES "Previous Feedback": If the fix for a previous issue is in a separate PR (per context), mark it as "passed": null (Skipped), NOT as "Still open" or "Fail". +4. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too +5. Focus ONLY on reviewing the actual code IN THIS PR +6. Decision should be APPROVE if the code IN THIS PR is correct +7. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR +=== END CRITICAL SECTION ===` + } + })()}` + } + + // /oc or /oc without numbered answers β†’ Phase 1 + return `[PHASE_1] Before reviewing this pull request, analyze the changes and ask 2-4 clarifying questions. DO NOT provide the actual review yet - just ask focused questions. + +User's additional context: ${userMessage || "None provided"} + +Example output format: + +πŸ€” **Before I review, a few questions:** + +**PR Type Detected:** [Type based on files changed] + +**I noticed:** +- [Observation about what's in the PR] +- [Observation about what seems missing] + +**Questions:** +1. [Context question] +2. [Focus question] + +Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), or use \`/oc!\` to skip questions.` } - throw new Error(`Comments must mention ${mentions.map((m) => "`" + m + "`").join(" or ")}`) + throw new Error(`Comments must mention ${mentions.map((m) => "\`" + m + "\`").join(" or ")} (add \`!\` for direct review, e.g. \`/oc!\`)`) })() // Handle images @@ -819,7 +1256,7 @@ export const GithubRunCommand = cmd({ function printEvent(color: string, type: string, title: string) { UI.println( - color + `|`, + color + `| `, UI.Style.TEXT_NORMAL + UI.Style.TEXT_DIM + ` ${type.padEnd(7, " ")}`, "", UI.Style.TEXT_NORMAL + title, @@ -858,7 +1295,7 @@ export const GithubRunCommand = cmd({ async function summarize(response: string) { try { - return await chat(`Summarize the following in less than 40 characters:\n\n${response}`) + return await chat(`Summarize the following in less than 40 characters: \n\n${response}`) } catch (e) { const title = issueEvent ? issueEvent.issue.title @@ -877,6 +1314,12 @@ export const GithubRunCommand = cmd({ providerID, modelID, }, + // REVIEW-ONLY MODE: Disable file editing tools + // AI can read files but cannot modify them - suggestions go in JSON output + tools: { + edit: false, + write: false, + }, // agent is omitted - server will use default_agent from config or fall back to "build" parts: [ { @@ -889,7 +1332,7 @@ export const GithubRunCommand = cmd({ id: Identifier.ascending("part"), type: "file" as const, mime: f.mime, - url: `data:${f.mime};base64,${f.content}`, + url: `data: ${f.mime}; base64, ${f.content} `, filename: f.filename, source: { type: "file" as const, @@ -909,7 +1352,7 @@ export const GithubRunCommand = cmd({ if (result.info.role === "assistant" && result.info.error) { console.error(result.info) throw new Error( - `${result.info.error.name}: ${"message" in result.info.error ? result.info.error.message : ""}`, + `${result.info.error.name}: ${"message" in result.info.error ? result.info.error.message : ""} `, ) } @@ -938,7 +1381,7 @@ export const GithubRunCommand = cmd({ if (summary.info.role === "assistant" && summary.info.error) { console.error(summary.info) throw new Error( - `${summary.info.error.name}: ${"message" in summary.info.error ? summary.info.error.message : ""}`, + `${summary.info.error.name}: ${"message" in summary.info.error ? summary.info.error.message : ""} `, ) } @@ -956,7 +1399,7 @@ export const GithubRunCommand = cmd({ } catch (error) { console.error("Failed to get OIDC token:", error) throw new Error( - "Could not fetch an OIDC token. Make sure to add `id-token: write` to your workflow permissions.", + "Could not fetch an OIDC token. Make sure to add `id - token: write` to your workflow permissions.", ) } } @@ -964,18 +1407,18 @@ export const GithubRunCommand = cmd({ async function exchangeForAppToken(token: string) { const response = token.startsWith("github_pat_") ? await fetch(`${oidcBaseUrl}/exchange_github_app_token_with_pat`, { - method: "POST", - headers: { - Authorization: `Bearer ${token}`, - }, - body: JSON.stringify({ owner, repo }), - }) + method: "POST", + headers: { + Authorization: `Bearer ${token}`, + }, + body: JSON.stringify({ owner, repo }), + }) : await fetch(`${oidcBaseUrl}/exchange_github_app_token`, { - method: "POST", - headers: { - Authorization: `Bearer ${token}`, - }, - }) + method: "POST", + headers: { + Authorization: `Bearer ${token}`, + }, + }) if (!response.ok) { const responseJson = (await response.json()) as { error?: string } @@ -1223,6 +1666,12 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` async function createComment(body: string) { // Only called for non-schedule events, so issueId is defined console.log("Creating comment...") + + // If this is a reply to a review comment, use threaded reply + if (commentType === "pr_review" && triggerCommentId) { + return await createReviewCommentReply(body) + } + return await octoRest.rest.issues.createComment({ owner, repo, @@ -1231,6 +1680,20 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` }) } + /** + * Reply to a specific review comment thread + */ + async function createReviewCommentReply(body: string) { + console.log(`Replying to review comment ${triggerCommentId}...`) + return await octoRest.rest.pulls.createReplyForReviewComment({ + owner, + repo, + pull_number: issueId!, // For PR events, issueId is the PR number + comment_id: triggerCommentId!, + body, + }) + } + async function createPR(base: string, branch: string, title: string, body: string) { console.log("Creating pull request...") const pr = await octoRest.rest.pulls.create({ @@ -1244,6 +1707,182 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` return pr.data.number } + /** + * Parse a Git patch string to extract valid line numbers from the NEW file side. + * These are the only lines where GitHub allows inline PR comments on the RIGHT side. + */ + + + /** + * Create a pull request review with inline comments on specific lines + */ + async function createPullRequestReview( + prNumber: number, + summary: string, + comments: Array<{ + path: string + line: number + start_line?: number + side?: "LEFT" | "RIGHT" + body: string + }> + ) { + console.log(`Creating PR review with ${comments.length} inline comments...`) + + // GitHub requires commit_id for review comments + const { data: pr } = await octoRest.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }) + const commitId = pr.head.sha + + try { + await octoRest.rest.pulls.createReview({ + owner, + repo, + pull_number: prNumber, + commit_id: commitId, + event: "COMMENT", + body: summary, + comments: comments.map((c) => ({ + path: c.path, + line: c.line, + ...(c.start_line ? { start_line: c.start_line } : {}), + side: c.side || "RIGHT", + body: c.body, + })), + }) + console.log(`Review created with ${comments.length} inline comments`) + } catch (error: any) { + // If inline comments fail, fall back to regular comment + console.warn("Failed to create inline review, falling back to regular comment:", error.message) + await createComment(`${summary}\n\n---\n\n_Note: Could not post inline comments. Showing feedback here instead._\n\n${comments.map((c) => `**${c.path}:${c.line}**\n${c.body}`).join("\n\n")}`) + } + } + + /** + * Robustly repairs and parses JSON from an LLM that might include: + * 1. Unescaped control characters like newlines inside strings + * 2. Hallucinated markdown blocks (```js) inside strings + * 3. Trailing commas + */ + + + /** + * Parse LLM response for structured review output and post inline comments. + * Returns true if inline review was posted, false if it fell back to regular comment. + */ + async function parseAndPostInlineReview( + prNumber: number, + response: string, + fallbackFooter: string + ): Promise { + // Try to find JSON in the response + const jsonMatch = response.match(/```json\s*([\s\S]*?)\s*```/) || + response.match(/\{[\s\S]*"summary"[\s\S]*"comments"[\s\S]*\}/) + + if (!jsonMatch) { + console.log("No structured JSON found in response, using regular comment") + await createComment(`${response}${fallbackFooter}`) + return false + } + + try { + const jsonStr = jsonMatch[1] || jsonMatch[0] + let parsed: any + let result: ReturnType + + // Phase 1: Try fast repair and parse + try { + parsed = repairAndParseJson(jsonStr) + result = ReviewComment.ReviewOutput.safeParse(parsed) + } catch (repairError) { + console.warn("Fast JSON repair failed, falling back to native structured output:", repairError) + result = { success: false, error: new z.ZodError([]) } as typeof result + } + + // Phase 2: If repair failed, use Gemini's native structured output + if (!result.success) { + console.log("Attempting extraction via native structured output (generateObject)...") + try { + const model = await Provider.getModel(providerID, modelID) + const language = await Provider.getLanguage(model) + + const structuredResult = await generateObject({ + model: language, + schema: ReviewComment.ReviewOutputRaw, + prompt: `Extract the structured review data from the following AI response. Return ONLY the JSON object matching the schema.\n\nAI Response:\n${response}`, + // Use Gemini's native JSON mode for bulletproof extraction + providerOptions: { + google: { + responseMimeType: 'application/json', + }, + }, + }) + + // Use the full schema with transforms for final validation + parsed = structuredResult.object + result = ReviewComment.ReviewOutput.safeParse(parsed) + } catch (structuredError) { + console.error("Native structured output extraction failed:", structuredError) + await createComment(`${response}${fallbackFooter}`) + return false + } + } + + if (!result.success) { + console.warn("Invalid review output structure after all attempts:", result.error.issues) + await createComment(`${response}${fallbackFooter}`) + return false + } + + const reviewData = result.data + + const fullSummary = renderReviewMarkdown(reviewData, { fallbackFooter }) + + if (reviewData.comments && reviewData.comments.length > 0) { + // Fetch changed files in PR + const { data: prFiles } = await octoRest.rest.pulls.listFiles({ + owner, + repo, + pull_number: prNumber, + per_page: 100, + }) + + const { valid: validComments, invalid: invalidComments } = filterCommentsByDiff( + reviewData.comments, + prFiles + ) + + let summaryWithInvalid = fullSummary + if (invalidComments.length > 0) { + summaryWithInvalid = fullSummary + "\n\n**Additional notes (outside diff range):**\n" + + invalidComments.map((c) => `- **${c.path}:${c.line}** - ${c.body}`).join("\n") + } + + if (validComments.length > 0) { + await createPullRequestReview(prNumber, summaryWithInvalid, validComments) + console.log(`Posted inline review with ${validComments.length} comments (${invalidComments.length} skipped)`) + return true + } else { + // All comments were for invalid paths, just post summary with notes + await createComment(summaryWithInvalid) + console.log(`No valid inline comments, posted summary with ${invalidComments.length} notes`) + return false + } + } else { + // No inline comments, just post summary + await createComment(fullSummary) + return false + } + } catch (e: any) { + console.warn("Failed to parse structured review:", e.message) + await createComment(`${response}${fallbackFooter}`) + return false + } + } + function footer(opts?: { image?: boolean }) { const image = (() => { if (!shareId) return "" @@ -1431,7 +2070,7 @@ query($owner: String!, $repo: String!, $number: Int!) { return pr } - function buildPromptDataForPR(pr: GitHubPullRequest) { + async function buildPromptDataForPR(pr: GitHubPullRequest) { // Only called for non-schedule events, so payload is defined const comments = (pr.comments?.nodes || []) .filter((c) => { @@ -1450,6 +2089,15 @@ query($owner: String!, $repo: String!, $number: Int!) { ] }) + // Inject context from repository based on changed files + const changedFilePaths = (pr.files.nodes || []).map((f) => f.path) + const contextResult = await ContextInjector.inject(changedFilePaths) + const contextPrompt = ContextInjector.buildPrompt(contextResult) + + if (contextResult.matchedRules.length > 0) { + console.log(`Context injection: ${contextResult.summary}`) + } + return [ "", "You are running as a GitHub Action. Important:", @@ -1476,6 +2124,9 @@ query($owner: String!, $repo: String!, $number: Int!) { ...(files.length > 0 ? ["", ...files, ""] : []), ...(reviewData.length > 0 ? ["", ...reviewData, ""] : []), "", + "", + // Inject repository context based on changed files + ...(contextPrompt ? [contextPrompt] : []), ].join("\n") } diff --git a/packages/opencode/src/command/template/review-clarification-guide.md b/packages/opencode/src/command/template/review-clarification-guide.md new file mode 100644 index 000000000000..87b083601626 --- /dev/null +++ b/packages/opencode/src/command/template/review-clarification-guide.md @@ -0,0 +1,148 @@ +# PR Review Clarification Guide + +This guide helps the AI reviewer ask relevant clarifying questions before reviewing a PR. + +## Trigger Keywords + +| Trigger | Behavior | +|---------|----------| +| `/oc` | Full two-phase flow (ask questions first, then review) | +| `/oc!` | Direct review (skip questions, go straight to review) | + +## Review Philosophy + +**Every PR deserves context-aware review.** Before diving into code, understand what the PR is trying to achieve and what constraints apply. + +**Exception:** If user uses `/oc!`, respect their intent to skip questions and proceed directly to review. + +--- + +## PR Type Detection + +Analyze the PR diff to detect what type of changes are being made: + +### Migration Only +**Signals:** Files in `**/migrations/**`, no model/controller changes +**Focus areas to ask about:** +- Related model PR (upcoming or existing?) +- Index strategy requirements +- Data migration concerns +- Rollback safety + +### Model Only +**Signals:** Files in `**/models/**`, no migration in this PR +**Focus areas to ask about:** +- Migration location (separate PR? already merged?) +- Association requirements +- Validation rules expected +- Soft delete requirements + +### API/Controller Changes +**Signals:** Files in `**/routes/**`, `**/controllers/**`, `**/services/**` +**Focus areas to ask about:** +- Expected request/response format +- Authentication/authorization requirements +- Rate limiting considerations +- Error handling expectations + +### Test Only +**Signals:** Files in `**/*.test.*`, `**/*.spec.*`, `**/__tests__/**` +**Focus areas to ask about:** +- What behavior is being tested +- Edge cases to cover +- Mock requirements + +### Full Feature (Multiple Types) +**Signals:** Migration + Model + Controller + Tests together +**Focus areas to ask about:** +- Priority of review (what's most critical?) +- Known limitations or shortcuts +- Performance requirements + +--- + +## Clarifying Questions Template + +```markdown +πŸ€” **Before I review, a few questions:** + +**PR Type Detected:** [Migration Only / Model Only / API / Full Feature] + +**I noticed:** +- [Observation 1: e.g., "No model file for this migration"] +- [Observation 2: e.g., "This adds a new endpoint"] + +**Questions:** +1. [Context question: e.g., "Will the model be in a separate PR?"] +2. [Constraint question: e.g., "Any specific performance requirements?"] +3. [Focus question: e.g., "Should I prioritize X or Y?"] + +Please reply with context so I can provide a focused review. +``` + +--- + +## After User Responds + +Once the user provides context: + +1. **Acknowledge** the constraints provided +2. **Generate checklist** based on PR type + user context +3. **Review with inline comments** on specific lines +4. **Provide decision** with reasoning + +--- + +## Example Scenarios + +### Scenario 1: Migration-Only PR + +**PR:** "Add video_post_views table migration" + +**AI asks:** +``` +πŸ€” Before I review, a few questions: + +**PR Type:** Migration Only + +I noticed this adds a new table but no corresponding model file. + +Questions: +1. Will VideoPostView model be in a separate PR? +2. Should I verify the schema matches any existing design doc? +3. Any specific indexing requirements for query performance? +``` + +**User responds:** "Model is in PR #354. Focus on FK constraints and indexes." + +**AI reviews:** Focused checklist on FKs + indexes, inline comments only on those issues. + +--- + +### Scenario 2: Refactoring PR + +**PR:** "Extract helper functions from UserService" + +**AI asks:** +``` +πŸ€” Before I review: + +**PR Type:** Refactoring + +I noticed this moves code without changing behavior. + +Questions: +1. Should I focus on maintaining existing behavior? +2. Any specific naming conventions for the new helpers? +3. Are there tests that should still pass unchanged? +``` + +--- + +## Key Principles + +1. **Don't assume** - Ask if something is unclear +2. **Detect scope** - Migration? Model? API? Mixed? +3. **Focus questions** - Max 3-4 questions, relevant to PR type +4. **Respect user's time** - Be concise, get to the point +5. **Block on confirmation** - Don't review until user responds diff --git a/packages/opencode/src/command/template/review.txt b/packages/opencode/src/command/template/review.txt index 1ffa0fca0b46..c0e1f651576d 100644 --- a/packages/opencode/src/command/template/review.txt +++ b/packages/opencode/src/command/template/review.txt @@ -28,6 +28,86 @@ Use best judgement when processing input. --- +## Two-Phase Review Workflow (For GitHub PRs) + +When reviewing a GitHub PR, follow this smart two-phase approach: + +### Trigger Keywords + +| Trigger | Behavior | +|---------|----------| +| `/oc` | Full two-phase flow (ask questions first) | +| `/oc!` | Direct review (skip questions, go straight to review) | + +### Smart Phase Detection + +Before deciding which phase to run, analyze the PR conversation history: + +1. **Check if `/oc!` was used** β†’ Skip to Phase 2 immediately +2. **Check if this is a follow-up message with answers:** + - Look for your previous "πŸ€” Before I review" comment in the conversation + - If user's message contains numbered answers (e.g., "1. Yes... 2. Focus on...") + - β†’ Proceed to Phase 2 using their answers as context +3. **Otherwise** β†’ Run Phase 1 (ask clarifying questions) + +### Phase 1: Ask Clarifying Questions (BLOCKING) + +Before reviewing, analyze the PR and ask clarifying questions: + +1. **Detect PR Type:** + - Migration Only: Files in `**/migrations/**` only + - Model Only: Files in `**/models/**`, no migration + - API Changes: Files in `**/routes/**`, `**/controllers/**`, `**/services/**` + - Tests Only: Files in `**/*.test.*`, `**/*.spec.*` + - Full Feature: Multiple types combined + +2. **Generate Questions Based on PR Type:** + ``` + πŸ€” **Before I review, a few questions:** + + **PR Type Detected:** [Type] + + **I noticed:** + - [Observation about what's in the PR] + - [Observation about what's missing] + + **Questions:** + 1. [Context question] + 2. [Focus question] + 3. [Constraint question] + + Reply with `/oc` and your answers, or use `/oc!` to skip this next time. + ``` + +3. **STOP and wait for user response.** Do NOT proceed with the review until the user responds. + +### Phase 2: Focused Review (After User Responds or /oc! Used) + +Once user provides context (or if /oc! was used): + +1. If answers provided, **acknowledge** the constraints +2. If /oc! used with no prior context, **infer focus** from PR description and diff +3. **Generate checklist** based on PR type + user context +4. **Post inline comments** on specific lines +5. **Provide decision** (APPROVE/REQUEST_CHANGES) with reasoning + +Output format for Phase 2: +```json +{ + "summary": "Brief summary", + "checklist": [ + {"item": "Criterion", "passed": true, "note": "Why"} + ], + "comments": [ + {"path": "file.js", "line": 42, "body": "Issue", "suggestion": "Fix", "severity": "warning"} + ], + "decision": "APPROVE or REQUEST_CHANGES", + "decision_reason": "Why" +} +``` + +--- + ## Gathering Context **Diffs alone are not enough.** After getting the diff, read the entire file(s) being modified to understand the full context. Code that looks wrong in isolation may be correct given surrounding logicβ€”and vice versa. @@ -95,3 +175,49 @@ If you're uncertain about something and can't verify it with these tools, say "I 4. Your tone should be matter-of-fact and not accusatory or overly positive. It should read as a helpful AI assistant suggestion without sounding too much like a human reviewer. 5. Write so the reader can quickly understand the issue without reading too closely. 6. AVOID flattery, do not give any comments that are not helpful to the reader. Avoid phrasing like "Great job ...", "Thanks for ...". + +--- + +## Structured Output for Inline Comments (GitHub PRs) + +When reviewing a GitHub PR and you want to provide inline comments on specific lines, output a JSON object with this structure: + +```json +{ + "summary": "Brief overall summary of the review (1-2 sentences)", + "comments": [ + { + "path": "src/models/User.js", + "line": 42, + "start_line": 40, + "body": "This field should have a default value to prevent null errors", + "suggestion": "const status = user.status ?? 'pending';", + "severity": "warning" + } + ], + "general_observations": [ + "Any observations not tied to specific lines" + ] +} +``` + +### Field Descriptions: +- **path**: File path relative to repository root (required) +- **line**: Line number to comment on - for the NEW version of the file (required) +- **start_line**: Starting line for multi-line comments/suggestions (optional) +- **body**: The comment text explaining the issue (required) +- **suggestion**: Code that should replace the commented lines - GitHub will show this as a "suggested change" the author can apply with one click (optional) +- **severity**: One of `error`, `warning`, `info`, `suggestion` (optional, defaults to `info`) + +### Severity Guide: +- **error**: Bugs, security issues, will cause failures +- **warning**: Code smells, potential issues, missing error handling +- **info**: Minor improvements, style issues +- **suggestion**: Optional enhancements, alternative approaches + +### Important Rules: +- Line numbers must reference the NEW version of the file (right side of the diff) +- For multi-line suggestions, set `start_line` to the first line and `line` to the last +- If you cannot determine exact line numbers, put the comment in `general_observations` instead +- Keep individual comments focused and actionable + diff --git a/packages/opencode/src/context/index.ts b/packages/opencode/src/context/index.ts new file mode 100644 index 000000000000..ce270099c440 --- /dev/null +++ b/packages/opencode/src/context/index.ts @@ -0,0 +1,3 @@ +export { ReviewRules } from "./rules" +export { ContextInjector } from "./injector" +export { ReviewComment } from "./review-comment" diff --git a/packages/opencode/src/context/injector.ts b/packages/opencode/src/context/injector.ts new file mode 100644 index 000000000000..52cb411479fa --- /dev/null +++ b/packages/opencode/src/context/injector.ts @@ -0,0 +1,226 @@ +import { Log } from "../util/log" +import { Instance } from "../project/instance" +import { Ripgrep } from "../file/ripgrep" +import { File } from "../file" +import { ReviewRules } from "./rules" +import path from "path" + +export namespace ContextInjector { + const log = Log.create({ service: "context-injector" }) + + /** + * Represents a file fetched as context + */ + export interface ContextFile { + path: string + content: string + truncated: boolean + } + + /** + * Result of context injection for a PR review + */ + export interface InjectionResult { + /** Rules that were triggered by the changed files */ + matchedRules: ReviewRules.Rule[] + /** Files fetched as additional context */ + contextFiles: ContextFile[] + /** Additional prompts from matched rules */ + rulePrompts: string[] + /** Summary of what context was injected */ + summary: string + } + + /** + * Fetch context files based on matched rules + */ + async function fetchContextFiles( + rules: ReviewRules.Rule[], + cwd: string + ): Promise { + const seenPaths = new Set() + const contextFiles: ContextFile[] = [] + + for (const rule of rules) { + const patterns = rule.context.include + const maxFiles = rule.context.maxFiles ?? 10 + const maxLines = rule.context.maxLinesPerFile ?? 500 + let fileCount = 0 + + for (const pattern of patterns) { + if (fileCount >= maxFiles) break + + try { + for await (const file of Ripgrep.files({ + cwd, + glob: [pattern], + maxDepth: 10, + })) { + if (fileCount >= maxFiles) break + + const fullPath = path.join(cwd, file) + if (seenPaths.has(fullPath)) continue + seenPaths.add(fullPath) + + try { + const bunFile = Bun.file(fullPath) + if (!(await bunFile.exists())) continue + + // Skip binary files + const type = bunFile.type?.toLowerCase() ?? "" + if (type.startsWith("image/") || type.startsWith("video/") || type.startsWith("audio/")) { + continue + } + + let content = await bunFile.text() + let truncated = false + + // Truncate if too long + const lines = content.split("\n") + if (lines.length > maxLines) { + content = lines.slice(0, maxLines).join("\n") + `\n\n... (truncated, ${lines.length - maxLines} more lines)` + truncated = true + } + + contextFiles.push({ + path: file, + content, + truncated, + }) + fileCount++ + + log.info("fetched context file", { rule: rule.name, file, lines: lines.length, truncated }) + } catch (e) { + log.warn("failed to read context file", { file, error: e }) + } + } + } catch (e) { + log.warn("failed to glob for context files", { pattern, error: e }) + } + } + } + + return contextFiles + } + + /** + * Format context files into a prompt section + */ + function formatContextSection(contextFiles: ContextFile[]): string { + if (contextFiles.length === 0) return "" + + const sections: string[] = [ + "", + "The following files are provided as additional context from the repository:", + "", + ] + + for (const file of contextFiles) { + sections.push(``) + sections.push(file.content) + sections.push("") + sections.push("") + } + + sections.push("") + return sections.join("\n") + } + + /** + * Format rule prompts into a prompt section + */ + function formatRulePrompts(rules: ReviewRules.Rule[]): string { + const prompts = rules + .filter((r) => r.prompt) + .map((r) => `### ${r.name}\n${r.prompt}`) + + if (prompts.length === 0) return "" + + return [ + "", + "Based on the types of files changed, apply these specific review guidelines:", + "", + ...prompts, + "", + ].join("\n") + } + + /** + * Main entry point: inject context for a PR review + */ + export async function inject(changedFiles: string[]): Promise { + using _ = log.time("inject") + const cwd = Instance.directory + + // Load rules (default + custom) + const rules = await ReviewRules.load() + + // Match rules against changed files + const matchedRules = await ReviewRules.match(changedFiles, rules) + + if (matchedRules.length === 0) { + log.info("no rules matched", { changedFiles: changedFiles.length }) + return { + matchedRules: [], + contextFiles: [], + rulePrompts: [], + summary: "No context injection rules matched the changed files.", + } + } + + log.info("rules matched", { count: matchedRules.length, rules: matchedRules.map((r) => r.name) }) + + // Fetch context files based on matched rules + const contextFiles = await fetchContextFiles(matchedRules, cwd) + + // Collect rule prompts + const rulePrompts = matchedRules + .filter((r) => r.prompt) + .map((r) => r.prompt!) + + // Generate summary + const summary = [ + `Context injection applied:`, + `- ${matchedRules.length} rules matched: ${matchedRules.map((r) => r.name).join(", ")}`, + `- ${contextFiles.length} context files fetched`, + contextFiles.length > 0 ? `- Files: ${contextFiles.map((f) => f.path).join(", ")}` : "", + ].filter(Boolean).join("\n") + + log.info("context injection complete", { + rules: matchedRules.length, + files: contextFiles.length, + }) + + return { + matchedRules, + contextFiles, + rulePrompts, + summary, + } + } + + /** + * Build the full context prompt to inject into the review + */ + export function buildPrompt(result: InjectionResult): string { + const sections: string[] = [] + + // Add context files section + const contextSection = formatContextSection(result.contextFiles) + if (contextSection) { + sections.push(contextSection) + } + + // Add rule prompts section + const ruleSection = formatRulePrompts(result.matchedRules) + if (ruleSection) { + sections.push(ruleSection) + } + + if (sections.length === 0) { + return "" + } + + return sections.join("\n\n") + } +} diff --git a/packages/opencode/src/context/review-comment.ts b/packages/opencode/src/context/review-comment.ts new file mode 100644 index 000000000000..cbd2dc0a21a4 --- /dev/null +++ b/packages/opencode/src/context/review-comment.ts @@ -0,0 +1,218 @@ +import z from "zod" + +/** + * Schema for inline review comments that can be posted to specific lines in a PR + */ +export namespace ReviewComment { + /** + * A single inline comment on a specific line or range of lines + * RAW version for generateObject (no transforms/defaults) + */ + export const InlineCommentRaw = z.object({ + path: z.string().describe("The file path where the comment should be placed"), + start_line: z.number().optional().describe("Starting line for multi-line comment range"), + line: z.number().describe("The line number to comment on (end line for multi-line)"), + side: z.enum(["LEFT", "RIGHT"]).optional().describe("Which side of the diff to comment on"), + body: z.string().describe("The comment text in markdown"), + suggestion: z.string().optional().nullable().describe("Code suggestion to replace the commented lines"), + severity: z.enum(["error", "warning", "info", "suggestion"]).optional().describe("Severity of the issue"), + }) + + /** + * A single inline comment on a specific line or range of lines + */ + export const InlineComment = z.object({ + /** File path relative to repository root */ + path: z.string().describe("The file path where the comment should be placed"), + + /** Starting line number for multi-line comments (optional) */ + start_line: z.number().optional().describe("Starting line for multi-line comment range"), + + /** Line number where the comment should appear */ + line: z.number().describe("The line number to comment on (end line for multi-line)"), + + /** Side of the diff to comment on */ + side: z.enum(["LEFT", "RIGHT"]).optional().default("RIGHT").describe("Which side of the diff to comment on"), + + /** The comment body */ + body: z.string().describe("The comment text in markdown"), + + /** Optional code suggestion to replace the commented lines */ + suggestion: z.string().optional().nullable().transform(v => { + if (!v) return undefined + // Strip markdown code blocks if AI accidentally includes them + return v + .replace(/^```\w*\n?/gm, '') // Remove opening ``` or ```js etc + .replace(/\n?```$/gm, '') // Remove closing ``` + .trim() || undefined + }).describe("Code suggestion to replace the commented lines"), + + /** Severity of the issue */ + severity: z.enum(["error", "warning", "info", "suggestion"]).optional().default("info"), + }) + export type InlineComment = z.infer + + /** + * A single checklist item for the review + * RAW version for generateObject (no preprocess) + */ + export const ChecklistItemRaw = z.object({ + item: z.string().describe("The criterion being checked"), + passed: z.boolean().nullable().describe("true=passed, false=failed, null=skipped"), + note: z.string().describe("Why it passed/failed/skipped"), + }) + + /** + * A single checklist item for the review + */ + export const ChecklistItem = z.object({ + /** The criterion being checked */ + item: z.string().describe("The criterion being checked"), + /** Whether it passed */ + passed: z.preprocess( + // Coerce invalid values (like "warning") to null + (v) => (typeof v === 'boolean' ? v : null), + z.boolean().nullable() + ).describe("true=passed, false=failed, null=skipped"), + /** Note explaining the result */ + note: z.string().describe("Why it passed/failed/skipped"), + }) + export type ChecklistItem = z.infer + + /** + * Item that was explicitly not reviewed + */ + export const NotReviewedItem = z.object({ + /** What was not reviewed */ + item: z.string().describe("What was not reviewed (e.g., 'Migrations')"), + /** Why it was skipped */ + reason: z.string().describe("Why it was skipped (e.g., 'In PR #352')"), + }) + export type NotReviewedItem = z.infer + + /** + * The full structured review output from the LLM + * RAW version for generateObject (no transforms/preprocess in nested schemas) + */ + export const ReviewOutputRaw = z.object({ + context_summary: z.string().optional().describe("Restated context from user's answers (e.g., 'Focusing on models only')"), + summary: z.string().describe("Overall summary of the code review"), + checklist: z.array(ChecklistItemRaw).optional().describe("Dynamic checklist based on PR type"), + comments: z.array(InlineCommentRaw).describe("List of inline comments on specific lines"), + general_observations: z.array(z.string()).optional().describe("General observations not tied to specific lines"), + not_reviewed: z.array(NotReviewedItem).optional().describe("Items explicitly not reviewed"), + decision: z.enum(["APPROVE", "REQUEST_CHANGES", "COMMENT"]).optional().describe("Review decision"), + decision_reason: z.string().optional().describe("Why this decision was made"), + }) + + /** + * The full structured review output from the LLM + */ + export const ReviewOutput = z.object({ + /** Restated context from user's answers */ + context_summary: z.string().optional().describe("Restated context from user's answers (e.g., 'Focusing on models only')"), + + /** Overall summary of the review */ + summary: z.string().describe("Overall summary of the code review"), + + /** Dynamic checklist based on PR type */ + checklist: z.array(ChecklistItem).optional().describe("Dynamic checklist based on PR type"), + + /** List of inline comments on specific lines */ + comments: z.array(InlineComment).describe("List of inline comments on specific lines"), + + /** General observations that don't map to specific lines */ + general_observations: z.array(z.string()).optional().describe("General observations not tied to specific lines"), + + /** Items explicitly not reviewed (hybrid: extracted from user answers + inferred) */ + not_reviewed: z.array(NotReviewedItem).optional().describe("Items explicitly not reviewed"), + + /** Review decision */ + decision: z.enum(["APPROVE", "REQUEST_CHANGES", "COMMENT"]).optional().describe("Review decision"), + + /** Reason for the decision */ + decision_reason: z.string().optional().describe("Why this decision was made"), + }) + export type ReviewOutput = z.infer + + /** + * Format an inline comment for GitHub's review API + * Handles the suggestion syntax for auto-fixable changes + */ + export function formatForGitHub(comment: InlineComment): { + path: string + line: number + start_line?: number + side?: "LEFT" | "RIGHT" + body: string + } { + let body = comment.body + + // Add suggestion block if provided + if (comment.suggestion) { + body += `\n\n\`\`\`suggestion\n${comment.suggestion}\n\`\`\`` + } + + // Add severity emoji prefix + const severityEmoji = { + error: "🚨", + warning: "⚠️", + info: "ℹ️", + suggestion: "πŸ’‘", + } + body = `${severityEmoji[comment.severity || "info"]} ${body}` + + return { + path: comment.path, + line: comment.line, + ...(comment.start_line ? { start_line: comment.start_line } : {}), + ...(comment.side ? { side: comment.side } : {}), + body, + } + } + + /** + * Generate the prompt instructions for structured review output + */ + export function getReviewPromptInstructions(): string { + return ` +## Output Format + +You MUST respond with a JSON object matching this exact schema: + +\`\`\`json +{ + "summary": "Overall summary of your review (1-3 sentences)", + "comments": [ + { + "path": "relative/path/to/file.ts", + "line": 42, + "start_line": 40, // optional, for multi-line comments + "body": "Your comment explaining the issue", + "suggestion": "// corrected code here", // optional, for auto-fix suggestions + "severity": "warning" // error | warning | info | suggestion + } + ], + "general_observations": [ + "Any high-level observations not tied to specific lines" + ] +} +\`\`\` + +### Guidelines for Comments: +- **Be specific**: Reference exact variable names, function calls, or patterns +- **Be actionable**: Explain what should be changed and why +- **Use suggestions**: When you know the fix, include a \`suggestion\` with corrected code +- **Set severity appropriately**: + - \`error\`: Bugs, security issues, will cause failures + - \`warning\`: Code smells, potential issues, missing error handling + - \`info\`: Style issues, minor improvements + - \`suggestion\`: Optional enhancements, alternative approaches + +### Important: +- Line numbers must match the NEW version of the file (right side of diff) +- For multi-line suggestions, set \`start_line\` to the first line and \`line\` to the last +- Keep comments concise but informative +` + } +} diff --git a/packages/opencode/src/context/rules.ts b/packages/opencode/src/context/rules.ts new file mode 100644 index 000000000000..bd641ecec10e --- /dev/null +++ b/packages/opencode/src/context/rules.ts @@ -0,0 +1,1296 @@ +import z from "zod" +import { Log } from "../util/log" +import { Instance } from "../project/instance" +import { Ripgrep } from "../file/ripgrep" +import path from "path" +import fs from "fs/promises" + +export namespace ReviewRules { + const log = Log.create({ service: "review-rules" }) + + /** + * Schema for a single review rule + */ + export const Rule = z.object({ + name: z.string().describe("Unique identifier for the rule"), + description: z.string().optional().describe("Human-readable description of what this rule checks"), + trigger: z.object({ + patterns: z.array(z.string()).describe("Glob patterns that trigger this rule when matched"), + }), + context: z.object({ + include: z.array(z.string()).describe("Glob patterns of files to fetch as context"), + maxFiles: z.number().optional().default(10).describe("Maximum number of files to include"), + maxLinesPerFile: z.number().optional().default(500).describe("Maximum lines to include per file"), + }), + prompt: z.string().optional().describe("Additional prompt instructions when this rule is triggered"), + }) + export type Rule = z.infer + + /** + * Schema for the review rules configuration + */ + export const Config = z.object({ + rules: z.array(Rule), + }) + export type Config = z.infer + + /** + * Default review rules shipped with opencode + * Based on layered Node.js/Express and Ruby/Rails patterns + */ + export const DEFAULT_RULES: Rule[] = [ + // ====== ALWAYS ACTIVE ====== + { + name: "conventions-memory-bank", + description: "Inject project-specific coding conventions and institutional knowledge", + trigger: { + patterns: ["**/*"], + }, + context: { + include: [ + "**/MEMORY_BANK.md", + "**/memory-bank.md", + "**/CONVENTIONS.md", + "**/conventions.md", + "**/.opencode/conventions.md", + "**/.github/MEMORY_BANK.md", + "**/.github/memory-bank.md", + "**/.github/runner-scripts/memory-bank.md", + "**/AGENTS.md", + "**/CLAUDE.md", + "**/OPENCODE.md", + "**/RAKAMIND.md", + ], + maxFiles: 3, + maxLinesPerFile: 2000, + }, + prompt: `Use the conventions/memory-bank as ground truth for project-specific standards.`, + }, + + // ====== MODEL ↔ MIGRATION ====== + { + name: "model-migration-sync", + description: "Verify model changes have corresponding migrations and vice versa", + trigger: { + patterns: [ + // Node.js + "**/models/*.js", + "**/models/**/*.js", + "**/src/migrations/*.js", + // Ruby + "**/app/models/*.rb", + "**/db/migrate/*.rb", + // Prisma/TypeORM + "**/schema.prisma", + "**/entities/**/*.ts", + ], + }, + context: { + include: [ + "**/models/*.js", + "**/models/**/*.js", + "**/src/migrations/*.js", + "**/app/models/*.rb", + "**/db/migrate/*.rb", + "**/schema.prisma", + ], + maxFiles: 15, + maxLinesPerFile: 300, + }, + prompt: `When reviewing models or migrations, CHECK THE REVIEW CONTEXT for branch strictness: +- Verify new model fields have corresponding migration columns +- Check column types match between model definition and migration +- Verify foreign key constraints and indexes +- Check paranoid/soft-delete is consistent (deletedAt column) +- For Sequelize: verify associations (belongsTo, hasMany, etc.) match FKs + +IMPORTANT: The ENFORCEMENT LEVEL depends on target branch: +- If targeting PROTECTED branch (main, production): Missing items are BLOCKERS +- If targeting NON-PROTECTED branch: Missing items are INFO/reminders only +- Follow the "CONCURRENT IMPLEMENTATION RULES" in the review context for strictness`, + }, + + // ====== CONTROLLER ↔ INPUT ↔ OUTPUT ====== + { + name: "controller-input-output", + description: "Verify controllers use proper Input validation and Output formatting", + trigger: { + patterns: [ + // Node.js/Express + "**/controllers/*.js", + "**/controllers/**/*.js", + "**/inputs/*.js", + "**/inputs/**/*.js", + "**/outputs/*.js", + "**/outputs/**/*.js", + // Ruby/Rails + "**/app/controllers/**/*.rb", + "**/app/inputs/**/*.rb", + ], + }, + context: { + include: [ + "**/controllers/*.js", + "**/inputs/*.js", + "**/outputs/*.js", + "**/app/controllers/**/*.rb", + "**/app/inputs/**/*.rb", + ], + maxFiles: 12, + maxLinesPerFile: 200, + }, + prompt: `When reviewing controllers, inputs, or outputs: +- Controllers should be thin: validate input β†’ call service β†’ format output +- Input classes must validate with JSON schema or strong params +- Output classes must format response consistently (renderJson, renderJsonArray) +- Check that input.validate() is called before using data +- Verify output fields match API contract`, + }, + + // ====== SERVICE ↔ REPOSITORY ====== + { + name: "service-repository", + description: "Verify service/repository layer separation", + trigger: { + patterns: [ + "**/services/*.js", + "**/services/**/*.js", + "**/repositories/*.js", + "**/app/services/**/*.rb", + "**/app/lib/repositories/**/*.rb", + ], + }, + context: { + include: [ + "**/services/*.js", + "**/services/**/*.js", + "**/repositories/*.js", + "**/app/services/**/*.rb", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `When reviewing services or repositories: +- Services contain business logic, validations, and orchestration +- Repositories handle data access (queries, pagination, filtering) +- Services should NOT contain raw SQL or complex queries +- Repositories should NOT contain business logic +- Check for proper error handling and transactions +- Services should extend AppService/ApplicationService`, + }, + + // ====== JOBS ====== + { + name: "background-jobs", + description: "Review background jobs for proper error handling and idempotency", + trigger: { + patterns: [ + "**/jobs/*.js", + "**/jobs/**/*.js", + "**/app/jobs/*.rb", + "**/workers/**/*.rb", + ], + }, + context: { + include: [ + "**/jobs/*.js", + "**/jobs/index.js", + "**/ApplicationJob.js", + "**/app/jobs/application_job.rb", + ], + maxFiles: 8, + maxLinesPerFile: 200, + }, + prompt: `When reviewing background jobs: +- Jobs must extend ApplicationJob +- Verify idempotency (safe to run multiple times) +- Check error handling and retry configuration +- Verify job is registered in index.js or job registry +- Check for proper queue selection (default, critical, low_priority)`, + }, + + // ====== MAILERS ====== + { + name: "mailers", + description: "Review email mailers for proper configuration", + trigger: { + patterns: [ + "**/mailers/*.js", + "**/app/mailers/*.rb", + ], + }, + context: { + include: [ + "**/mailers/*.js", + "**/mailers/ApplicationMailer.js", + "**/app/mailers/application_mailer.rb", + ], + maxFiles: 6, + maxLinesPerFile: 200, + }, + prompt: `When reviewing mailers: +- Must extend ApplicationMailer +- Check template rendering and variable passing +- Verify subject lines are localized if applicable +- Check for proper error handling +- Verify mailer uses job queue for async sending`, + }, + + // ====== ROUTES ====== + { + name: "routes", + description: "Review route definitions for consistency", + trigger: { + patterns: [ + "**/routes/*.js", + "**/routes/**/*.js", + "**/config/routes.rb", + ], + }, + context: { + include: [ + "**/routes/*.js", + "**/app.js", + "**/config/routes.rb", + "**/middlewares/auth.js", + ], + maxFiles: 10, + maxLinesPerFile: 200, + }, + prompt: `When reviewing routes: +- Verify authentication middleware is applied correctly +- Check rate limiter configuration +- RESTful conventions: index, show, create, update, destroy +- Verify route naming matches controller methods +- Check permissions/authorization middleware`, + }, + + // ====== TESTS ====== + { + name: "test-coverage", + description: "Verify test coverage for changed code", + trigger: { + patterns: [ + "**/src/**/*.js", + "**/app/**/*.rb", + "**/lib/**/*.rb", + ], + }, + context: { + include: [ + // Node.js/Jest + "**/tests/**/*.test.js", + "**/tests/**/*.spec.js", + "**/tests/factories/*.js", + // Ruby/RSpec + "**/spec/**/*_spec.rb", + "**/spec/factories/*.rb", + ], + maxFiles: 10, + maxLinesPerFile: 200, + }, + prompt: `When reviewing source code, check for corresponding tests: +- Controllers should have integration tests +- Services should have unit tests +- Jobs should have tests verifying correct behavior +- Critical business logic MUST have test coverage +- Check that tests use factories, not raw data`, + }, + + // ====== DOCUMENTATION ====== + { + name: "documentation", + description: "Include relevant documentation for context", + trigger: { + patterns: [ + "**/controllers/**/*.js", + "**/services/**/*.js", + "**/models/**/*.js", + "**/jobs/**/*.js", + ], + }, + context: { + include: [ + "**/docs/*.md", + "**/docs/controllers.md", + "**/docs/services.md", + "**/docs/models.md", + "**/docs/repositories.md", + "**/docs/job-system.md", + ], + maxFiles: 5, + maxLinesPerFile: 500, + }, + prompt: `Use the documentation to understand project conventions and patterns.`, + }, + + // ====== MIDDLEWARE ====== + { + name: "middleware", + description: "Review middleware for security and performance", + trigger: { + patterns: [ + "**/middlewares/*.js", + "**/app/middlewares/**/*.rb", + ], + }, + context: { + include: [ + "**/middlewares/*.js", + "**/app/middlewares/**/*.rb", + ], + maxFiles: 8, + maxLinesPerFile: 200, + }, + prompt: `When reviewing middleware: +- Check for proper error handling (next(e) pattern) +- Verify authentication middleware returns proper errors +- Check rate limiting configuration +- Verify middleware order is correct`, + }, + + // ====== ERRORS ====== + { + name: "error-handling", + description: "Review error classes and handling", + trigger: { + patterns: [ + "**/errors/*.js", + "**/app/errors/*.rb", + ], + }, + context: { + include: [ + "**/errors/*.js", + "**/middlewares/errorHandler.js", + ], + maxFiles: 6, + maxLinesPerFile: 200, + }, + prompt: `When reviewing error handling: +- Custom errors should extend base error class +- Verify HTTP status codes are appropriate +- Check error messages are user-friendly +- Sensitive info should not leak in error responses`, + }, + + // ====== CACHES ====== + { + name: "caching", + description: "Review cache implementations", + trigger: { + patterns: [ + "**/caches/*.js", + "**/app/caches/*.rb", + ], + }, + context: { + include: [ + "**/caches/*.js", + "**/config/cache.js", + "**/config/redis.js", + "**/app/caches/*.rb", + ], + maxFiles: 6, + maxLinesPerFile: 200, + }, + prompt: `When reviewing cache code: +- Check cache key generation for uniqueness +- Verify TTL is appropriate +- Check cache invalidation strategy +- Verify Redis connection handling`, + }, + + // ====== TEST β†’ BUSINESS LOGIC ====== + { + name: "test-business-logic", + description: "When reviewing tests, fetch source code to verify business logic coverage", + trigger: { + patterns: [ + // Node.js/Jest + "**/tests/**/*.test.js", + "**/tests/**/*.spec.js", + "**/*.test.js", + "**/*.spec.js", + // Ruby/RSpec + "**/spec/**/*_spec.rb", + ], + }, + context: { + include: [ + // Node.js source + "**/controllers/*.js", + "**/services/*.js", + "**/services/**/*.js", + "**/repositories/*.js", + "**/jobs/*.js", + "**/mailers/*.js", + // Ruby source + "**/app/controllers/**/*.rb", + "**/app/services/**/*.rb", + "**/app/models/*.rb", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `When reviewing tests, verify they properly cover business logic: +- Check edge cases are tested (null, empty, boundary values) +- Verify error scenarios are covered +- Check happy path AND failure paths +- Verify mocks are appropriate (not over-mocking) +- Check assertions are meaningful (not just "it doesn't throw") +- For controllers: test request validation, auth, and response format +- For services: test business rules and side effects +- For jobs: test idempotency and failure handling`, + }, + + // ====== DATABASE TRANSACTIONS ====== + { + name: "database-transactions", + description: "Verify multi-step database operations use transactions", + trigger: { + patterns: [ + "**/services/*.js", + "**/services/**/*.js", + "**/jobs/*.js", + "**/app/services/**/*.rb", + ], + }, + context: { + include: [ + "**/config/sequelize.js", + "**/models/index.js", + ], + maxFiles: 4, + maxLinesPerFile: 200, + }, + prompt: `When reviewing services or jobs with multiple database operations: +- Multi-step writes MUST be wrapped in transaction +- Verify transaction.commit() on success +- Verify transaction.rollback() in catch block +- Check for advisory locks on concurrent operations (transactionWithAdvLock) +- Jobs that queue other jobs should use transaction.afterCommit() +- For Rails: use ActiveRecord::Base.transaction`, + }, + + // ====== N+1 QUERY PREVENTION ====== + { + name: "n-plus-one-queries", + description: "Detect potential N+1 query patterns", + trigger: { + patterns: [ + "**/repositories/*.js", + "**/services/*.js", + "**/controllers/*.js", + "**/app/models/*.rb", + ], + }, + context: { + include: [ + "**/models/*.js", + "**/models/index.js", + "**/repositories/BaseRepository.js", + ], + maxFiles: 8, + maxLinesPerFile: 300, + }, + prompt: `Watch for N+1 query patterns: +- Loops that call findByPk/findOne inside β†’ use include/eager loading +- Check 'include:' arrays for nested associations +- Verify buildIncludeClause is used for dynamic includes +- For bulk operations: batch loading, not individual queries +- For Rails: use .includes(), .preload(), .eager_load()`, + }, + + // ====== SECURITY PATTERNS ====== + { + name: "security-patterns", + description: "Review security-sensitive code", + trigger: { + patterns: [ + "**/auth/**/*.js", + "**/middlewares/auth.js", + "**/inputs/*.js", + "**/repositories/*.js", + "**/app/auth/**/*.rb", + ], + }, + context: { + include: [ + "**/tests/**/*.security.test.js", + "**/middlewares/auth.js", + "**/errors/*.js", + ], + maxFiles: 8, + maxLinesPerFile: 300, + }, + prompt: `Security review checklist: +- SQL injection: use parameterized queries, Sequelize.escape() +- Input validation: sanitize user input, validate ltree paths +- Auth: verify JWT validation, check permission middleware +- Sensitive data: don't log passwords/tokens/API keys +- Response: don't expose internal errors to users +- Hierarchy queries: validate path format before use +- BOLA/IDOR: verify user owns the resource before update/delete`, + }, + + // ====== EXTERNAL API INTEGRATIONS ====== + { + name: "external-api-integration", + description: "Review external API/service integrations", + trigger: { + patterns: [ + "**/services/external/*.js", + "**/services/*Service.js", + "**/lib/*Service.js", + "**/app/services/external/**/*.rb", + ], + }, + context: { + include: [ + "**/services/external/*.js", + "**/tests/mocks/*.js", + "**/config/*.js", + ], + maxFiles: 8, + maxLinesPerFile: 300, + }, + prompt: `When reviewing external API integrations: +- Verify timeout configuration +- Check retry logic with exponential backoff +- Handle API errors gracefully (don't crash on 5xx) +- Validate response data before using +- Check for rate limiting awareness +- Verify API keys come from env, not hardcoded +- Mock exists for testing (tests/mocks/)`, + }, + + // ====== DATABASE INDEXES ====== + { + name: "database-indexes", + description: "Verify migrations add appropriate indexes", + trigger: { + patterns: [ + "**/migrations/*.js", + "**/db/migrate/*.rb", + ], + }, + context: { + include: [ + "**/models/*.js", + "**/repositories/*.js", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `When reviewing migrations, THINK about how data will be queried: + +INDEX DECISION FRAMEWORK: +1. What queries will this table serve? (list the common access patterns) +2. What columns appear in WHERE clauses? (need indexes) +3. What columns appear in ORDER BY? (consider in compound index) +4. Are there JOINs through this table? (foreign keys need indexes) + +QUERY PATTERN ANALYSIS: +- "Get X by Y" β†’ Index on Y +- "Get X by Y ordered by Z" β†’ Compound index (Y, Z) +- "Get children of parent ordered by date" β†’ Index on (parent_fk, created_at) +- "Get recent items" β†’ Index on (created_at) or include in compound + +SELF-REFERENTIAL / HIERARCHICAL TABLES: +- If a table references itself (parent_id pattern), think about: + - How will children be fetched? By parent + sort order? + - Recommend appropriate compound index based on query pattern + +Always explain WHY an index is needed based on expected query patterns.`, + }, + + // ====== FACTORY & TEST DATA CONSISTENCY ====== + { + name: "factory-consistency", + description: "Verify test factories match model definitions", + trigger: { + patterns: [ + "**/tests/factories/*.js", + "**/spec/factories/*.rb", + ], + }, + context: { + include: [ + "**/models/*.js", + "**/app/models/*.rb", + "**/tests/factories/BaseFactory.js", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `When reviewing factories: +- Factory fields must match model definition +- Required fields should have default values +- Associations must use correct factory +- Traits/variations for different states +- Use faker for realistic test data +- Check for circular dependencies in associations`, + }, + + // ====== AUDIT LOGGING ====== + { + name: "audit-logging", + description: "Verify audit logging for sensitive operations", + trigger: { + patterns: [ + "**/services/*.js", + "**/jobs/*.js", + "**/controllers/*.js", + "**/app/services/**/*.rb", + ], + }, + context: { + include: [ + "**/jobs/AuditLogJob.js", + "**/middlewares/auditLog.js", + "**/repositories/AuditLogsRepository.js", + ], + maxFiles: 5, + maxLinesPerFile: 300, + }, + prompt: `When reviewing code with sensitive operations: +- Create/update/delete operations should trigger audit logs +- Use AuditLogJob.performLater() for async logging +- Include actor, action, resource, and changes in audit +- Check namespace.get('auditLogContext') propagation +- Jobs should pass parent_transaction_id for tracing`, + }, + + // ====== FILE UPLOAD SECURITY ====== + { + name: "file-upload-security", + description: "Review file upload and S3 operations", + trigger: { + patterns: [ + "**/services/UploadService.js", + "**/controllers/UploadsController.js", + "**/lib/FileStorageBucket.js", + "**/lib/StoredFile.js", + ], + }, + context: { + include: [ + "**/services/UploadService.js", + "**/lib/FileStorageBucket.js", + "**/inputs/*Upload*.js", + ], + maxFiles: 6, + maxLinesPerFile: 300, + }, + prompt: `When reviewing file upload code: +- Validate file type/extension before upload +- Sanitize filenames (remove special chars, spaces) +- Use presigned URLs with expiration +- Verify content-type matches extension +- Check max file size limits +- S3 bucket should not be public +- Use secure upload paths (user-specific prefixes)`, + }, + + // ====== CI/CD WORKFLOWS ====== + { + name: "ci-cd-workflows", + description: "Review CI/CD pipeline configurations", + trigger: { + patterns: [ + "**/.github/workflows/*.yml", + "**/.github/workflows/*.yaml", + "**/.circleci/config.yml", + ], + }, + context: { + include: [ + "**/.github/workflows/*.yml", + "**/package.json", + "**/Dockerfile", + ], + maxFiles: 6, + maxLinesPerFile: 300, + }, + prompt: `When reviewing CI/CD workflows: +- Secrets should use GitHub secrets, not hardcoded +- Check for proper caching (node_modules, bun cache) +- Verify test step runs before deploy +- Check Docker build context and layer caching +- Verify proper environment variable injection +- Check trigger conditions (branches, paths)`, + }, + + // ====== SOFT DELETE / PARANOID ====== + { + name: "soft-delete-paranoid", + description: "Verify soft delete consistency", + trigger: { + patterns: [ + "**/models/*.js", + "**/repositories/*.js", + "**/migrations/*.js", + "**/app/models/*.rb", + ], + }, + context: { + include: [ + "**/models/*.js", + "**/migrations/*.js", + ], + maxFiles: 10, + maxLinesPerFile: 200, + }, + prompt: `When reviewing models with soft delete (CHECK REVIEW CONTEXT for enforcement level): +- Model should have paranoid: true if soft delete is used +- Migration should include deleted_at column +- Queries should respect soft delete (default behavior) +- Force: true only when GDPR hard delete is needed +- Check cascading deletes for associations +- Restore functionality if needed + +Note: Enforcement level depends on target branch per "CONCURRENT IMPLEMENTATION RULES" in review context`, + }, + + // ====== PERMISSION / AUTHORIZATION ====== + { + name: "permission-authorization", + description: "Review permission and authorization logic", + trigger: { + patterns: [ + "**/middlewares/auth.js", + "**/controllers/*.js", + "**/services/PermissionService.js", + "**/caches/RolePermissionCache.js", + ], + }, + context: { + include: [ + "**/middlewares/auth.js", + "**/services/PermissionService.js", + "**/caches/RolePermissionCache.js", + "**/models/Permission.js", + "**/models/RolePermission.js", + ], + maxFiles: 8, + maxLinesPerFile: 300, + }, + prompt: `When reviewing authorization code: +- All endpoints should have permission checks +- Use role-based access control (RBAC) +- Cache permissions for performance +- Check resource ownership (BOLA prevention) +- Verify permission names match feature names +- Super admin bypass should be intentional`, + }, + + // ====== AI/LLM INTEGRATION ====== + { + name: "ai-llm-integration", + description: "Review AI/LLM service integrations", + trigger: { + patterns: [ + "**/services/external/GoogleAiService.js", + "**/services/*Ai*.js", + "**/services/*Llm*.js", + "**/jobs/*Llm*.js", + "**/jobs/Process*Job.js", + ], + }, + context: { + include: [ + "**/services/external/GoogleAiService.js", + "**/jobs/LogLlmInteractionJob.js", + "**/models/LlmMetadata.js", + "**/tests/mocks/*Ai*.js", + ], + maxFiles: 8, + maxLinesPerFile: 400, + }, + prompt: `When reviewing AI/LLM integrations: +- Log all LLM interactions (input, output, tokens, cost) +- Handle rate limits and quota errors +- Implement retry with exponential backoff +- Validate and sanitize LLM outputs before using +- Check for prompt injection vulnerabilities +- Use structured output (JSON mode) when possible +- Test with mocks, not real API calls`, + }, + + // ====== LOGGING / OBSERVABILITY ====== + { + name: "logging-observability", + description: "Review logging and observability patterns", + trigger: { + patterns: [ + "**/config/logger.js", + "**/middlewares/logging.js", + "**/middlewares/prometheus.js", + "**/services/*.js", + ], + }, + context: { + include: [ + "**/config/logger.js", + "**/middlewares/logging.js", + "**/middlewares/prometheus.js", + ], + maxFiles: 6, + maxLinesPerFile: 300, + }, + prompt: `When reviewing logging code: +- Use structured logging (JSON format) +- Include correlation IDs for tracing +- Don't log sensitive data (passwords, tokens, PII) +- Use appropriate log levels (debug, info, warn, error) +- Add metrics for critical operations (Prometheus) +- Check for log injection vulnerabilities`, + }, + + // ====== MODEL HOOKS / LIFECYCLE ====== + { + name: "model-hooks-lifecycle", + description: "Review model hooks and lifecycle callbacks", + trigger: { + patterns: [ + "**/models/*.js", + "**/app/models/*.rb", + ], + }, + context: { + include: [ + "**/models/AppModel.js", + "**/models/*.js", + ], + maxFiles: 8, + maxLinesPerFile: 300, + }, + prompt: `When reviewing model hooks (beforeCreate, afterUpdate, etc.): +- Hooks should be fast (no heavy operations) +- Avoid circular triggers (update triggers update) +- Use hooks for data normalization (lowercase email, trim) +- Hash passwords in beforeCreate/beforeUpdate +- Don't put business logic in hooks (use services) +- Check options.transaction is passed through +- For Rails: use before_validation, before_save, after_commit`, + }, + + // ====== RATE LIMITING ====== + { + name: "rate-limiting", + description: "Review rate limiting configuration", + trigger: { + patterns: [ + "**/middlewares/rateLimiter.js", + "**/routes/*.js", + "**/app.js", + ], + }, + context: { + include: [ + "**/middlewares/rateLimiter.js", + "**/config/cache.js", + "**/routes/*.js", + ], + maxFiles: 8, + maxLinesPerFile: 300, + }, + prompt: `When reviewing rate limiting: +- Auth endpoints need stricter limits (5 per 15 min) +- Public endpoints need looser limits +- Use Redis for distributed rate limiting +- Check skip paths configuration +- Different limits per endpoint sensitivity +- Return proper 429 status with Retry-After header`, + }, + + // ====== CONFIG / ENVIRONMENT ====== + { + name: "config-environment", + description: "Review configuration and environment handling", + trigger: { + patterns: [ + "**/config/*.js", + "**/config/*.rb", + "**/.env.example", + ], + }, + context: { + include: [ + "**/config/config.js", + "**/config/sequelize.js", + "**/config/redis.js", + "**/.env.example", + ], + maxFiles: 6, + maxLinesPerFile: 300, + }, + prompt: `When reviewing configuration: +- Sensitive values must come from env vars +- Provide sensible defaults for non-sensitive values +- Validate required env vars on startup +- Use different configs for dev/test/prod +- Document all env vars in .env.example +- Check for hardcoded secrets or API keys`, + }, + + // ====== API RESPONSE FORMAT ====== + { + name: "api-response-format", + description: "Ensure consistent API response formatting", + trigger: { + patterns: [ + "**/outputs/*.js", + "**/controllers/*.js", + ], + }, + context: { + include: [ + "**/outputs/ApiOutput.js", + "**/outputs/*.js", + "**/errors/*.js", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `When reviewing API responses: +- Consistent structure: { data, meta, errors } +- Use Output classes for all responses +- Include pagination meta for list endpoints +- Use camelCase for JSON keys +- Don't expose internal IDs if not needed +- Include proper HTTP status codes +- Error responses should have error code and message`, + }, + + // ====== CODE STYLE & CLARITY (from memory-bank) ====== + { + name: "code-style-clarity", + description: "Review code style, imports, and redundancy", + trigger: { + patterns: [ + "**/services/*.js", + "**/controllers/*.js", + "**/repositories/*.js", + ], + }, + context: { + include: [], + maxFiles: 0, + maxLinesPerFile: 0, + }, + prompt: `Code style review (from memory-bank): +- No redundant imports (consolidate from same source) +- No redundant variable declarations (const { ...copy } = data) +- No redundant conditional checks (if user exists when auth middleware guarantees it) +- Explicitly declare all dependencies (no ReferenceError) +- Use singular naming for Service/Input classes (CompanyService, not CompaniesService) +- Remove dead/deprecated code +- Document conceptually overlapping entities (JobGrade vs JobLevel) +- Document complex or non-obvious logic with comments`, + }, + + // ====== INPUT SCHEMA VALIDATION (from memory-bank) ====== + { + name: "input-schema-validation", + description: "Verify input schemas validate all used fields", + trigger: { + patterns: [ + "**/inputs/*.js", + "**/inputs/**/*.js", + ], + }, + context: { + include: [ + "**/services/*.js", + "**/controllers/*.js", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `Input schema review (from memory-bank): +- Schema must validate ALL fields used by business logic +- Use 'enum' to strictly enforce allowed string values +- Schema filters params, query, body - single source of truth +- Remove fields no longer used by business logic +- Use specific Input schemas per endpoint, not generic reuse +- Required fields should be in 'required' array`, + }, + + // ====== RACE CONDITION & LOCKS (from memory-bank) ====== + { + name: "race-condition-locks", + description: "Prevent race conditions in find-then-act patterns", + trigger: { + patterns: [ + "**/services/*.js", + "**/repositories/*.js", + ], + }, + context: { + include: [ + "**/models/index.js", + "**/config/sequelize.js", + ], + maxFiles: 4, + maxLinesPerFile: 200, + }, + prompt: `Race condition prevention (from memory-bank): +- Find-then-create/update needs pessimistic lock (FOR UPDATE) +- Use lock: t.LOCK.UPDATE in Sequelize +- Or use transactionWithAdvLock for advisory locks +- Watch for TOC/TOU (Time-of-Check to Time-of-Use) bugs +- Toggle status operations need locks to prevent overwrites`, + }, + + // ====== OUTPUT FORMATTING (from memory-bank) ====== + { + name: "output-formatting", + description: "Verify API output formatting consistency", + trigger: { + patterns: [ + "**/outputs/*.js", + "**/outputs/**/*.js", + ], + }, + context: { + include: [ + "**/outputs/ApiOutput.js", + "**/models/*.js", + ], + maxFiles: 8, + maxLinesPerFile: 300, + }, + prompt: `Output formatting review (from memory-bank): +- Map camelCase model properties to snake_case JSON keys +- Always include keys even if null (consistent shape) +- Use singular key for belongsTo, plural for hasMany +- Expose only necessary data (no toJSON() dumps) +- Format nested objects via their own Output class +- Provide defaults for missing values (|| null)`, + }, + + // ====== ASSOCIATION NAMING (from memory-bank) ====== + { + name: "association-naming", + description: "Verify model association naming conventions", + trigger: { + patterns: [ + "**/models/*.js", + "**/app/models/*.rb", + ], + }, + context: { + include: [ + "**/models/*.js", + "**/migrations/*.js", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `Association naming review (from memory-bank): +- Exact case-sensitive alias in queries (workAreas not work_areas) +- Always specify foreignKey explicitly +- Use singular alias for belongsTo/hasOne +- Use plural alias for hasMany/belongsToMany +- Every foreign key column needs corresponding association +- Migration FK must match model association foreignKey`, + }, + + // ====== API LAYER SEPARATION (from memory-bank) ====== + { + name: "api-layer-separation", + description: "Ensure separation between data and presentation", + trigger: { + patterns: [ + "**/services/*.js", + "**/controllers/*.js", + "**/outputs/*.js", + ], + }, + context: { + include: [], + maxFiles: 0, + maxLinesPerFile: 0, + }, + prompt: `Layer separation review (from memory-bank): +- Service: fetch data, execute business logic +- Output: format data for API response (mapping, merging) +- Don't do presentation logic (parseInt, mapping) in services +- Controllers should be thin (delegate to services) +- Services can use multiple repositories +- Raw SQL for stats/reports should be in services, not repositories`, + }, + + // ====== DEFENSIVE CODING (from memory-bank) ====== + { + name: "defensive-coding", + description: "Review defensive coding patterns", + trigger: { + patterns: [ + "**/services/*.js", + "**/outputs/*.js", + "**/repositories/*.js", + ], + }, + context: { + include: [], + maxFiles: 0, + maxLinesPerFile: 0, + }, + prompt: `Defensive coding review (from memory-bank): +- Use optional chaining (?.) for nested property access +- Correctly attribute actions in audit trails (this.user.id for current user) +- Use declarative helpers (this.exists, this.authorize, this.assert) +- Use lazy require to break circular dependencies (code smell warning) +- Provide fallback to system config for optional values (not magic numbers) +- Let errors from external services propagate (don't swallow for monitoring)`, + }, + + // ====== RAW SQL SOFT DELETE (from memory-bank) ====== + { + name: "raw-sql-soft-delete", + description: "Verify raw SQL handles soft deletes correctly", + trigger: { + patterns: [ + "**/services/*.js", + "**/repositories/*.js", + ], + }, + context: { + include: [ + "**/models/*.js", + ], + maxFiles: 5, + maxLinesPerFile: 200, + }, + prompt: `Raw SQL soft delete review (from memory-bank): +- ORM adds deleted_at IS NULL automatically +- Raw SQL must MANUALLY add deleted_at IS NULL +- Add to both WHERE clause and JOIN conditions +- Check all tables in query that use paranoid/soft-delete +- Missing filter = data leakage, incorrect calculations`, + }, + + // ====== TEST COMPLETENESS (from memory-bank) ====== + { + name: "test-completeness", + description: "Verify test coverage completeness", + trigger: { + patterns: [ + "**/tests/**/*.test.js", + "**/tests/**/*.spec.js", + "**/spec/**/*_spec.rb", + ], + }, + context: { + include: [ + "**/services/*.js", + "**/controllers/*.js", + ], + maxFiles: 10, + maxLinesPerFile: 300, + }, + prompt: `Test completeness review (from memory-bank): +- Test happy paths AND failure paths +- Test edge cases (null, empty, boundary values) +- Test all enum values/branches (e.g., 'upcoming' AND 'past') +- Test authorization scenarios (own data, subordinate, peer, superior) +- Test input validation failures (missing required params) +- Test aggregate scoping (user A's stats don't include user B's data) +- Test descriptions must be accurate (not copy-pasted wrong) +- Disable rate limiting in test env`, + }, + + // ====== PR QUALITY (from memory-bank) ====== + { + name: "pr-quality", + description: "Review PR for production readiness", + trigger: { + patterns: [ + "**/*", + ], + }, + context: { + include: [], + maxFiles: 0, + maxLinesPerFile: 0, + }, + prompt: `PR quality review (from memory-bank): +- Remove temporary debugging code (console.log, [DEBUG] logger) +- No 'I'll fix this in the next PR' - fix now +- Migrations should be consolidated (no add-then-change-column) +- Keep feature branches up-to-date with main +- Audit and update all call sites for breaking changes +- Check for missing model associations for new FKs`, + }, + ] + + /** + * Load review rules from the project's .opencode directory + */ + export async function load(): Promise { + const configPaths = [ + path.join(Instance.directory, ".opencode", "review-rules.yaml"), + path.join(Instance.directory, ".opencode", "review-rules.json"), + path.join(Instance.directory, "review-rules.yaml"), + path.join(Instance.directory, "review-rules.json"), + ] + + for (const configPath of configPaths) { + try { + const exists = await fs.access(configPath).then(() => true).catch(() => false) + if (!exists) continue + + const content = await fs.readFile(configPath, "utf-8") + let parsed: unknown + + if (configPath.endsWith(".yaml") || configPath.endsWith(".yml")) { + // Dynamic import for YAML parsing + const yaml = await import("js-yaml") + parsed = yaml.load(content) + } else { + parsed = JSON.parse(content) + } + + const result = Config.safeParse(parsed) + if (result.success) { + log.info("loaded review rules", { path: configPath, count: result.data.rules.length }) + return [...DEFAULT_RULES, ...result.data.rules] + } else { + log.warn("invalid review rules config", { path: configPath, issues: result.error.issues }) + } + } catch (e) { + log.warn("failed to load review rules", { path: configPath, error: e }) + } + } + + log.info("using default review rules", { count: DEFAULT_RULES.length }) + return DEFAULT_RULES + } + + /** + * Match changed files against rules to determine which rules apply + */ + export async function match(changedFiles: string[], rules: Rule[]): Promise { + const matchedRules: Rule[] = [] + + for (const rule of rules) { + for (const pattern of rule.trigger.patterns) { + const glob = new Bun.Glob(pattern) + for (const file of changedFiles) { + if (glob.match(file)) { + if (!matchedRules.includes(rule)) { + matchedRules.push(rule) + log.info("rule matched", { rule: rule.name, file, pattern }) + } + break + } + } + } + } + + return matchedRules + } +} diff --git a/packages/opencode/src/util/git-diff.ts b/packages/opencode/src/util/git-diff.ts new file mode 100644 index 000000000000..615fb7a5b71f --- /dev/null +++ b/packages/opencode/src/util/git-diff.ts @@ -0,0 +1,40 @@ + +/** + * Parse a git patch string to identify valid line numbers for inline comments. + * + * GitHub only allows inline PR comments on the RIGHT side for: + * 1. Added lines (starting with +) + * 2. Context lines (starting with space) + * Deleted lines (starting with -) are not valid for RIGHT side comments. + * + * @param patch The raw git patch string + * @returns Set of valid line numbers (typically new/right side line numbers) + */ +export function parsePatchForValidLines(patch: string): Set { + const validLines = new Set() + const lines = patch.split('\n') + let currentNewLine = 0 + + for (const line of lines) { + // Parse hunk header: @@ -oldStart,oldCount +newStart,newCount @@ + const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/) + if (hunkMatch) { + currentNewLine = parseInt(hunkMatch[1], 10) + continue + } + + if (currentNewLine === 0) continue // Before first hunk + + // Lines starting with '+' are additions (valid) + // Lines starting with ' ' are context (valid) + // Lines starting with '-' are deletions (not valid for RIGHT side comments) + if (line.startsWith('+') || line.startsWith(' ')) { + validLines.add(currentNewLine) + currentNewLine++ + } else if (line.startsWith('-')) { + // Deletion - don't increment newLine counter + } + } + + return validLines +} diff --git a/packages/opencode/src/util/github-review-logic.ts b/packages/opencode/src/util/github-review-logic.ts new file mode 100644 index 000000000000..17e24d8bdb64 --- /dev/null +++ b/packages/opencode/src/util/github-review-logic.ts @@ -0,0 +1,131 @@ +import { ReviewComment } from "../context/review-comment" +import { parsePatchForValidLines } from "./git-diff" + +export interface RenderOptions { + fallbackFooter: string +} + +/** + * Renders the structured review data into a comprehensive Markdown body for GitHub. + */ +export function renderReviewMarkdown(reviewData: any, opts: RenderOptions): string { + const reviewParts: string[] = [] + + // Header + reviewParts.push("## πŸ€– AI Code Review") + reviewParts.push("") + + // Context summary (if present) + if (reviewData.context_summary) { + reviewParts.push(`> **Context:** ${reviewData.context_summary}`) + reviewParts.push("") + } + + // Summary + reviewParts.push(`> ${reviewData.summary}`) + reviewParts.push("") + reviewParts.push("---") + reviewParts.push("") + + // Checklist table (if present) + if (reviewData.checklist && reviewData.checklist.length > 0) { + reviewParts.push("### βœ… Review Checklist") + reviewParts.push("") + reviewParts.push("| # | Criterion | Status | Note |") + reviewParts.push("|---|-----------|--------|------|") + reviewData.checklist.forEach((item: any, index: number) => { + const status = item.passed === true ? "βœ… Pass" : + item.passed === false ? "❌ Fail" : + "⏭️ Skipped" + reviewParts.push(`| ${index + 1} | ${item.item} | ${status} | ${item.note} |`) + }) + reviewParts.push("") + reviewParts.push("---") + reviewParts.push("") + } + + // General observations + if (reviewData.general_observations && reviewData.general_observations.length > 0) { + reviewParts.push("### πŸ“ General Observations") + reviewParts.push("") + reviewData.general_observations.forEach((obs: string) => { + reviewParts.push(`- ${obs}`) + }) + reviewParts.push("") + reviewParts.push("---") + reviewParts.push("") + } + + // Decision (if present) + if (reviewData.decision) { + const decisionEmoji = reviewData.decision === "APPROVE" ? "βœ…" : + reviewData.decision === "REQUEST_CHANGES" ? "πŸ”„" : "πŸ’¬" + reviewParts.push(`### 🎯 Decision: **${reviewData.decision}** ${decisionEmoji}`) + reviewParts.push("") + if (reviewData.decision_reason) { + reviewParts.push(`**Reason:** ${reviewData.decision_reason}`) + reviewParts.push("") + } + } + + // Not reviewed section (if present) + if (reviewData.not_reviewed && reviewData.not_reviewed.length > 0) { + reviewParts.push("**Not reviewed (per your context):**") + reviewData.not_reviewed.forEach((item: any) => { + reviewParts.push(`- ~~${item.item}~~ β†’ ${item.reason}`) + }) + reviewParts.push("") + } + + return reviewParts.join("\n") + opts.fallbackFooter +} + +export interface ValidComment { + path: string + line: number + side: "LEFT" | "RIGHT" + body: string + start_line?: number + start_side?: "LEFT" | "RIGHT" +} + +/** + * Filters generated comments against the PR's valid line ranges from the diff. + */ +export function filterCommentsByDiff( + comments: any[], + prFiles: { filename: string, patch?: string }[] +): { valid: ValidComment[], invalid: ValidComment[] } { + // Build a map of valid paths and their line ranges from the diff + const validPathsWithLines = new Map>() + for (const file of prFiles) { + if (!file.patch) continue + const validLines = parsePatchForValidLines(file.patch) + validPathsWithLines.set(file.filename, validLines) + } + + const valid: ValidComment[] = [] + const invalid: ValidComment[] = [] + + const formattedComments = comments.map(c => ReviewComment.formatForGitHub(c)) as ValidComment[] + + for (const comment of formattedComments) { + const validLines = validPathsWithLines.get(comment.path) + if (!validLines) { + invalid.push(comment) + continue + } + + // Check if the comment's line (or range) is in the diff + const lineInDiff = validLines.has(comment.line) + const startLineInDiff = comment.start_line ? validLines.has(comment.start_line) : true + + if (lineInDiff && startLineInDiff) { + valid.push(comment) + } else { + invalid.push(comment) + } + } + + return { valid, invalid } +} diff --git a/packages/opencode/src/util/json-repair.ts b/packages/opencode/src/util/json-repair.ts new file mode 100644 index 000000000000..6cd15ccad54b --- /dev/null +++ b/packages/opencode/src/util/json-repair.ts @@ -0,0 +1,85 @@ + +/** + * Robustly parses JSON that might have common LLM-generated errors. + * + * Features: + * 1. Normalizes trailing commas + * 2. Escapes control characters (newlines, tabs) inside string literals + * 3. Strips markdown code block wrappers (```json ... ```) + * + * @param raw The raw JSON string to parse + * @returns The parsed object + * @throws Error if parsing fails even after repair + */ +export function repairAndParseJson(raw: string): any { + let text = raw.trim() + + // Phase 1: Basic structural cleaning + // Normalize trailing commas in objects and arrays + // Matches , followed by whitespace and a closing brace/bracket + text = text.replace(/,\s*([\]}])/g, '$1') + + // Phase 2: Structural Character Walk + // We walk the string to correctly identify and escape content inside string literals + // without affecting the JSON structure itself. + let inString = false + let escaped = false + let repaired = "" + + for (let i = 0; i < text.length; i++) { + const char = text[i] + + if (char === '"' && !escaped) { + if (!inString) { + inString = true + repaired += char + } else { + // We are in a string and found a quote. Is it a closing quote or an unescaped inner quote? + // We look ahead to see if the next non-whitespace character is a structural delimiter (:, }, ], ,) + // or if we are at the end of the string. + const remainder = text.slice(i + 1) + const isDelimiter = /^(\s*[,}\]:])|^\s*$/.test(remainder) + + if (isDelimiter) { + inString = false + repaired += char + } else { + // It's likely an inner quote (e.g. "some "quoted" text") -> escape it + repaired += '\\"' + } + } + } else if (inString) { + // Inside a string literal - escape or transform problematic chars + if (char === '\n') repaired += '\\n' + else if (char === '\r') repaired += '\\r' + else if (char === '\t') repaired += '\\t' + else if (char === '\\' && !escaped) { + escaped = true + repaired += char + } else { + repaired += char + escaped = false + } + } else { + repaired += char + escaped = false + } + } + + // Phase 3: Content-specific repair (Triple Backticks) + // Now that we have valid JSON-escaped strings, we can specifically strip + // markdown markers that AI hallucinates inside suggestion/body fields. + // We use a simplified regex because control chars are now escaped. + repaired = repaired + .replace(/```[a-z]*\\n?/gi, '') // Remove opening blocks (escaped) + .replace(/\\\\n?```/gi, '') // Remove closing blocks (escaped) + .replace(/```[a-z]*\\n?/gi, '') // Remove opening blocks (raw, though unlikely now) + .replace(/\\n?```/gi, '') // Remove closing blocks (raw, though unlikely now) + + try { + return JSON.parse(repaired) + } catch (e: any) { + console.error("JSON parse failed after repair. Repaired string:", repaired) + throw new Error(`JSON Repair failed: ${e.message}`) + } +} diff --git a/packages/opencode/test/cli/github-review.test.ts b/packages/opencode/test/cli/github-review.test.ts new file mode 100644 index 000000000000..f07616aa5839 --- /dev/null +++ b/packages/opencode/test/cli/github-review.test.ts @@ -0,0 +1,211 @@ +import { describe, expect, test } from "bun:test" +import { repairAndParseJson } from "../../src/util/json-repair" +import { parsePatchForValidLines } from "../../src/util/git-diff" +import { renderReviewMarkdown, filterCommentsByDiff } from "../../src/util/github-review-logic" + +describe("PR Review Utilities", () => { + describe("github-review-logic", () => { + describe("renderReviewMarkdown", () => { + test("renders a basic review correctly", () => { + const data = { + summary: "Great job!", + decision: "APPROVE" + } + const rendered = renderReviewMarkdown(data, { fallbackFooter: "\nFooter" }) + expect(rendered).toContain("## πŸ€– AI Code Review") + expect(rendered).toContain("> Great job!") + expect(rendered).toContain("### 🎯 Decision: **APPROVE** βœ…") + expect(rendered).toContain("Footer") + }) + + test("renders complex review with checklist and observations", () => { + const data = { + summary: "Summary here", + checklist: [ + { item: "Bug check", passed: true, note: "Clean" }, + { item: "Syntax", passed: false, note: "Typo on L10" } + ], + general_observations: ["Good style", "Missing docs"], + decision: "REQUEST_CHANGES", + decision_reason: "Needs typo fix" + } + const rendered = renderReviewMarkdown(data, { fallbackFooter: "" }) + expect(rendered).toContain("| 1 | Bug check | βœ… Pass | Clean |") + expect(rendered).toContain("| 2 | Syntax | ❌ Fail | Typo on L10 |") + expect(rendered).toContain("- Good style") + expect(rendered).toContain("- Missing docs") + expect(rendered).toContain("### 🎯 Decision: **REQUEST_CHANGES** πŸ”„") + expect(rendered).toContain("**Reason:** Needs typo fix") + }) + + test("renders context summary and not_reviewed section", () => { + const data = { + summary: "Summary", + context_summary: "Reviewing PR #1", + not_reviewed: [ + { item: "Tests", reason: "Out of scope" } + ] + } + const rendered = renderReviewMarkdown(data, { fallbackFooter: "" }) + expect(rendered).toContain("> **Context:** Reviewing PR #1") + expect(rendered).toContain("- ~~Tests~~ β†’ Out of scope") + }) + }) + + describe("filterCommentsByDiff", () => { + test("filters comments correctly based on diff match", () => { + const comments = [ + { path: "src/a.ts", line: 10, body: "Valid" }, + { path: "src/a.ts", line: 20, body: "Invalid Line" }, + { path: "src/b.ts", line: 10, body: "Invalid Path" } + ] + const prFiles = [ + { filename: "src/a.ts", patch: "@@ -1,1 +10,1 @@\n+line10\n-old\n+line11" } + ] + + const { valid, invalid } = filterCommentsByDiff(comments, prFiles) + + expect(valid.length).toBe(1) + expect(valid[0].body).toBe("Valid") + expect(invalid.length).toBe(2) + expect(invalid.some(c => c.body === "Invalid Line")).toBe(true) + expect(invalid.some(c => c.body === "Invalid Path")).toBe(true) + }) + + test("handles multi-line comments validation", () => { + const comments = [ + { path: "src/a.ts", line: 10, start_line: 5, body: "Invalid Range" }, + ] + const prFiles = [ + { filename: "src/a.ts", patch: "@@ -1,1 +10,1 @@\n+line10" } + ] + const { valid, invalid } = filterCommentsByDiff(comments, prFiles) + expect(valid.length).toBe(0) + expect(invalid.length).toBe(1) + }) + }) + }) + + describe("git-diff", () => { + test("parses valid lines correctly", () => { + const patch = `@@ -10,1 +10,1 @@ +-old ++new` + const result = parsePatchForValidLines(patch) + expect(result.has(10)).toBe(true) + }) + + test("handles multiple hunks", () => { + const patch = `@@ -1,1 +1,1 @@ ++line1 +@@ -5,1 +5,1 @@ ++line5` + const result = parsePatchForValidLines(patch) + expect(result.has(1)).toBe(true) + expect(result.has(5)).toBe(true) + }) + + test("ignores deleted lines", () => { + const patch = `@@ -1,1 +1,0 @@ +-deleted` + const result = parsePatchForValidLines(patch) + expect(result.size).toBe(0) + }) + + test("handles context lines", () => { + const patch = `@@ -1,3 +1,3 @@ + line1 ++line2 + line3` + const result = parsePatchForValidLines(patch) + expect(result.has(1)).toBe(true) + expect(result.has(2)).toBe(true) + expect(result.has(3)).toBe(true) + }) + }) + + describe("repairAndParseJson", () => { + test("parses valid JSON accurately", () => { + const input = '{"key": "value"}' + const result = repairAndParseJson(input) + expect(result).toEqual({ key: "value" }) + }) + + test("handles unescaped newlines in strings", () => { + // Simulating LLM output where newlines are raw literals + const input = `{"key": "line1 + line2"}` + const result = repairAndParseJson(input) + expect(result.key).toContain("line1") + expect(result.key).toContain("line2") + }) + + test("handles unescaped tabs and quotes", () => { + const input = `{"key": "some "quoted" text and a tab "}` + const result = repairAndParseJson(input) + expect(result.key).toBe('some "quoted" text and a tab ') + }) + + test("normalizes trailing commas", () => { + const input = ` + { + "arr": [1, 2, ], + "obj": {"a": 1, }, + }` + const result = repairAndParseJson(input) + expect(result.arr).toEqual([1, 2]) + expect(result.obj).toEqual({ a: 1 }) + }) + + test("strips markdown code blocks in values", () => { + const input = ` + { + "suggestion": "\`\`\`javascript +const x = 1 +\`\`\`" + }` + const result = repairAndParseJson(input) + expect(result.suggestion.trim()).toBe("const x = 1") + }) + + test("strips simple invalid markdown blocks", () => { + const input = ` + { + "suggestion": "\`\`\` +const x = 1 +\`\`\`" + }` + const result = repairAndParseJson(input) + expect(result.suggestion.trim()).toBe("const x = 1") + }) + + test("handles complex nested objects with mixed issues", () => { + const input = ` + { + "comments": [ + { + "body": "Multiligne + body", + "suggestion": "\`\`\`ts + console.log('hello') + \`\`\`" + }, + ] + }` + const result = repairAndParseJson(input) + expect(result.comments[0].body).toContain("Multiligne") + expect(result.comments[0].suggestion).toContain("console.log") + }) + + test("handles already escaped backslashes", () => { + const input = '{"key": "a\\\\b"}' + const result = repairAndParseJson(input) + expect(result.key).toBe("a\\b") + }) + + test("throws on truly un-repairable JSON", () => { + const input = '{"key": "value" ... partially broken' + expect(() => repairAndParseJson(input)).toThrow("JSON Repair failed") + }) + }) +})