From 8b8a06c6ce34dca32f1a4a4519383f24fbcd19fa Mon Sep 17 00:00:00 2001 From: blacksyan Date: Fri, 2 Jan 2026 07:50:03 +0700 Subject: [PATCH 01/43] feat: enhanced github agent with advanced review rules and self-hosted workflow --- .gitignore | 2 + packages/opencode/src/cli/cmd/github.ts | 272 +++- .../template/review-clarification-guide.md | 148 ++ .../opencode/src/command/template/review.txt | 126 ++ packages/opencode/src/context/index.ts | 3 + packages/opencode/src/context/injector.ts | 226 +++ .../opencode/src/context/review-comment.ts | 129 ++ packages/opencode/src/context/rules.ts | 1275 +++++++++++++++++ 8 files changed, 2134 insertions(+), 47 deletions(-) create mode 100644 packages/opencode/src/command/template/review-clarification-guide.md create mode 100644 packages/opencode/src/context/index.ts create mode 100644 packages/opencode/src/context/injector.ts create mode 100644 packages/opencode/src/context/review-comment.ts create mode 100644 packages/opencode/src/context/rules.ts 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/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 26e0fb73dc33..4d5133ddf693 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -27,6 +27,7 @@ import { Bus } from "../../bus" import { MessageV2 } from "../../session/message-v2" import { SessionPrompt } from "@/session/prompt" import { $ } from "bun" +import { ContextInjector, ReviewComment } from "../../context" type GitHubAuthor = { login: string @@ -185,7 +186,7 @@ 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 +235,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 +369,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 +390,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/cli/index.ts github run +`, ) prompts.log.success(`Added workflow file: "${WORKFLOW_FILE}"`) @@ -464,7 +484,7 @@ 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 = `/ ${owner} / ${repo} / actions / runs / ${runId}` const shareBaseUrl = isMock ? "https://dev.opencode.ai" : "https://opencode.ai" let appToken: string @@ -536,7 +556,7 @@ export const GithubRunCommand = cmd({ } const branchPrefix = isWorkflowDispatchEvent ? "dispatch" : "schedule" const branch = await checkoutNewBranch(branchPrefix) - const head = (await $`git rev-parse HEAD`).stdout.toString().trim() + const head = (await $`git rev - parse HEAD`).stdout.toString().trim() const response = await chat(userPrompt, promptFiles) const { dirty, uncommittedChanges } = await branchIsDirty(head) if (dirty) { @@ -562,38 +582,48 @@ export const GithubRunCommand = cmd({ // Local PR 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 head = (await $`git rev - parse HEAD`).stdout.toString().trim() + 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 head = (await $`git rev - parse HEAD`).stdout.toString().trim() + 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) } } // Issue else { const branch = await checkoutNewBranch("issue") - const head = (await $`git rev-parse HEAD`).stdout.toString().trim() + const head = (await $`git rev - parse HEAD`).stdout.toString().trim() const issueData = await fetchIssue() const dataPrompt = buildPromptDataForIssue(issueData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) @@ -645,7 +675,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 +690,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 +698,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 { @@ -734,13 +764,13 @@ export const GithubRunCommand = cmd({ 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}` + return `Review this code change and suggest improvements for the commented lines: \n\nFile: ${reviewContext.file} \nLines: ${reviewContext.line} \n\n${reviewContext.diffHunk} ` } return "Summarize this thread" } 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}` + return `${body} \n\nContext: You are reviewing a comment on file "${reviewContext.file}" at line ${reviewContext.line}.\n\nDiff context: \n${reviewContext.diffHunk}` } return body } @@ -819,7 +849,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 +888,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 @@ -889,7 +919,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 +939,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 +968,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 +986,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 +994,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 +1253,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 +1267,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 +1294,122 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` return pr.data.number } + /** + * 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")}`) + } + } + + /** + * 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] + const parsed = JSON.parse(jsonStr) + + // Validate with our schema + const result = ReviewComment.ReviewOutput.safeParse(parsed) + + if (!result.success) { + console.warn("Invalid review output structure:", result.error.issues) + await createComment(`${response}${fallbackFooter}`) + return false + } + + const reviewData = result.data + + // Format comments for GitHub API + const formattedComments = reviewData.comments.map((comment) => + ReviewComment.formatForGitHub(comment) + ) + + if (formattedComments.length > 0) { + // Build summary with general observations + let fullSummary = reviewData.summary + if (reviewData.general_observations && reviewData.general_observations.length > 0) { + fullSummary += "\n\n**General Observations:**\n" + + reviewData.general_observations.map((obs) => `- ${obs}`).join("\n") + } + fullSummary += fallbackFooter + + await createPullRequestReview(prNumber, fullSummary, formattedComments) + console.log(`Posted inline review with ${formattedComments.length} comments`) + return true + } else { + // No inline comments, just post summary + let fullSummary = reviewData.summary + if (reviewData.general_observations && reviewData.general_observations.length > 0) { + fullSummary += "\n\n**General Observations:**\n" + + reviewData.general_observations.map((obs) => `- ${obs}`).join("\n") + } + await createComment(`${fullSummary}${fallbackFooter}`) + 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 +1597,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 +1616,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 +1651,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..9b4cb4fd525d --- /dev/null +++ b/packages/opencode/src/context/review-comment.ts @@ -0,0 +1,129 @@ +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 + */ + 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().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 + + /** + * The full structured review output from the LLM + */ + export const ReviewOutput = z.object({ + /** Overall summary of the review */ + summary: z.string().describe("Overall summary of the code review"), + + /** 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"), + }) + 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..1b2c97a99800 --- /dev/null +++ b/packages/opencode/src/context/rules.ts @@ -0,0 +1,1275 @@ +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", + ], + 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: +- 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 +- For Rails: verify belongs_to/has_many match FKs`, + }, + + // ====== 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: +- Foreign keys should have indexes +- Unique constraints should have indexes +- Columns used in WHERE/ORDER BY frequently need indexes +- Compound indexes for multi-column queries +- Consider partial indexes for filtered queries +- Check for missing indexes on new columns`, + }, + + // ====== 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: +- 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`, + }, + + // ====== 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 + } +} From 2da9ad8c6153ac98cc484a1cd0b87313d752c398 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Fri, 2 Jan 2026 18:40:36 +0700 Subject: [PATCH 02/43] fix: correct CLI entry point path from src/cli/index.ts to src/index.ts --- packages/opencode/src/cli/cmd/github.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 4d5133ddf693..e1917d92eab0 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -417,7 +417,7 @@ jobs: GITHUB_EVENT_PATH: \${{ github.event_path }} OPENCODE_MODEL: ${provider}/${model} ${envSecrets} - run: bun run /tmp/opencode/packages/opencode/src/cli/index.ts github run + run: bun run /tmp/opencode/packages/opencode/src/index.ts github run `, ) From af88b14b98b94f981ef45d3776ff1f546d2b6e10 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Fri, 2 Jan 2026 19:56:46 +0700 Subject: [PATCH 03/43] feat: add headless CI entry point to avoid TUI dependencies --- packages/opencode/src/ci.ts | 109 ++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 packages/opencode/src/ci.ts 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() +} From c1cb1adb2515ed7e6d30271a2d12dfbe862d51a2 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Fri, 2 Jan 2026 20:13:25 +0700 Subject: [PATCH 04/43] fix: correct git rev-parse command (remove spaces) --- packages/opencode/src/cli/cmd/github.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index e1917d92eab0..f74ccdae03ec 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -556,7 +556,7 @@ export const GithubRunCommand = cmd({ } const branchPrefix = isWorkflowDispatchEvent ? "dispatch" : "schedule" const branch = await checkoutNewBranch(branchPrefix) - const head = (await $`git rev - parse HEAD`).stdout.toString().trim() + const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const response = await chat(userPrompt, promptFiles) const { dirty, uncommittedChanges } = await branchIsDirty(head) if (dirty) { @@ -582,7 +582,7 @@ export const GithubRunCommand = cmd({ // Local PR if (prData.headRepository.nameWithOwner === prData.baseRepository.nameWithOwner) { await checkoutLocalBranch(prData) - const head = (await $`git rev - parse HEAD`).stdout.toString().trim() + const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) const { dirty, uncommittedChanges } = await branchIsDirty(head) @@ -602,7 +602,7 @@ export const GithubRunCommand = cmd({ // Fork PR else { await checkoutForkBranch(prData) - const head = (await $`git rev - parse HEAD`).stdout.toString().trim() + const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) const { dirty, uncommittedChanges } = await branchIsDirty(head) @@ -623,7 +623,7 @@ export const GithubRunCommand = cmd({ // Issue else { const branch = await checkoutNewBranch("issue") - const head = (await $`git rev - parse HEAD`).stdout.toString().trim() + const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const issueData = await fetchIssue() const dataPrompt = buildPromptDataForIssue(issueData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) From 44fec82bbfe0f54c2b1c4a7cdf575bbc6a251796 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Fri, 2 Jan 2026 20:24:22 +0700 Subject: [PATCH 05/43] feat: implement two-phase review flow (/oc for questions, /oc! for direct) --- packages/opencode/src/cli/cmd/github.ts | 43 +++++++++++++++++++++---- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index f74ccdae03ec..d2f31ad45e5c 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -762,19 +762,50 @@ export const GithubRunCommand = cmd({ } 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))) { + const userMessage = body.replace(/\/oc!?|\/opencode!?/gi, "").trim() + const hasNumberedAnswers = /^\s*\d+[\.\)]\s*.+/m.test(userMessage) + + if (hasNumberedAnswers) { + // User is answering questions β†’ Phase 2 + return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers:\n\nUser's answers:\n${userMessage}` + } + + // User provided context with /oc β†’ treat as additional context 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}` + return `${body}\n\nContext: You are reviewing a comment on file "${reviewContext.file}" at line ${reviewContext.line}.\n\nDiff context:\n${reviewContext.diffHunk}` } return body } - 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 From dccc2197f30c0e056873a0e272eeec4cdafb4a28 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Fri, 2 Jan 2026 21:09:14 +0700 Subject: [PATCH 06/43] fix: /oc with any text now triggers Phase 1 (questions) unless has numbered answers --- packages/opencode/src/cli/cmd/github.ts | 32 +++++++++++++++++++++---- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index d2f31ad45e5c..63f07277963f 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -791,6 +791,13 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), // Handle /oc with additional text (user answering questions or providing context) if (mentions.some((m) => bodyLower.includes(m))) { + // 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"}` + } + const userMessage = body.replace(/\/oc!?|\/opencode!?/gi, "").trim() const hasNumberedAnswers = /^\s*\d+[\.\)]\s*.+/m.test(userMessage) @@ -799,11 +806,26 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers:\n\nUser's answers:\n${userMessage}` } - // User provided context with /oc β†’ treat as additional context - 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}` - } - return body + // /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 ")} (add \`!\` for direct review, e.g. \`/oc!\`)`) })() From 2fc5209a26a0b4bd0a5e6015a95192b0d58be2f6 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Fri, 2 Jan 2026 21:27:35 +0700 Subject: [PATCH 07/43] fix: add JSON output format instruction to Phase 2 and Direct Review for inline comments --- packages/opencode/src/cli/cmd/github.ts | 39 +++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 63f07277963f..6907f9cc936f 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -795,7 +795,23 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), 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"}` + return `[DIRECT_REVIEW] Review this pull request directly without asking clarifying questions. User context: ${userMessage || "None provided"} + +IMPORTANT: Output your review as structured JSON for inline comments: +\`\`\`json +{ + "summary": "Brief overall summary (1-2 sentences)", + "comments": [ + { + "path": "src/path/to/file.js", + "line": 42, + "body": "Issue description", + "severity": "error|warning|info|suggestion" + } + ], + "general_observations": ["Any observations not tied to specific lines"] +} +\`\`\`` } const userMessage = body.replace(/\/oc!?|\/opencode!?/gi, "").trim() @@ -803,7 +819,26 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), if (hasNumberedAnswers) { // User is answering questions β†’ Phase 2 - return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers:\n\nUser's answers:\n${userMessage}` + return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers: + +User's answers: +${userMessage} + +IMPORTANT: Output your review as structured JSON for inline comments: +\`\`\`json +{ + "summary": "Brief overall summary (1-2 sentences)", + "comments": [ + { + "path": "src/path/to/file.js", + "line": 42, + "body": "Issue description", + "severity": "error|warning|info|suggestion" + } + ], + "general_observations": ["Any observations not tied to specific lines"] +} +\`\`\`` } // /oc or /oc without numbered answers β†’ Phase 1 From 6a71e3e5927dd936722f902c2c4ff6f6d66b49db Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 01:48:23 +0700 Subject: [PATCH 08/43] fix: handle Gemini 2.0 step parts in response parsing --- packages/opencode/src/cli/cmd/github.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 6907f9cc936f..ecfcb6e55de4 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -177,9 +177,10 @@ 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 2.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({ From 6b7813c38e2b689549c256ac3c7fd88463c43827 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 01:51:13 +0700 Subject: [PATCH 09/43] docs: update comment to reflect Gemini 3.0 Flash Preview --- packages/opencode/src/cli/cmd/github.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index ecfcb6e55de4..74d196dac92f 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -178,7 +178,7 @@ export function extractResponseText(parts: MessageV2.Part[]): string | null { if (toolParts.length > 0) return null // Priority 4: Step parts or other unknown parts - // When Gemini 2.0 uses tools or thinks, it may emit step-start/step-finish or other types. + // 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 } From 510653c85779d541d4d3e6b9b3030387656fc22c Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 02:34:43 +0700 Subject: [PATCH 10/43] feat: smart thread detection - skip Phase 1 for threaded replies, use focused context --- packages/opencode/src/cli/cmd/github.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 74d196dac92f..674b47b09a05 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -734,6 +734,8 @@ 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, } } @@ -818,6 +820,28 @@ IMPORTANT: Output your review as structured JSON for inline comments: 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 + + 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.` + } + if (hasNumberedAnswers) { // User is answering questions β†’ Phase 2 return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers: From cdd5c8fbcc9d0d82c8b078d0743691daac3954b4 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 03:11:40 +0700 Subject: [PATCH 11/43] feat: implement dynamic checklist with decision support for comprehensive reviews --- packages/opencode/src/cli/cmd/github.ts | 137 ++++++++++--- .../opencode/src/context/review-comment.ts | 186 ++++++++++-------- 2 files changed, 217 insertions(+), 106 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 674b47b09a05..bb2d9dd79248 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -798,12 +798,23 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), 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"} + return `[DIRECT_REVIEW] Review this pull request directly without asking clarifying questions. +User context: ${userMessage || "None provided"} + +IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based on PR type: -IMPORTANT: Output your review as structured JSON for inline comments: \`\`\`json { - "summary": "Brief overall summary (1-2 sentences)", + "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", @@ -812,9 +823,21 @@ IMPORTANT: Output your review as structured JSON for inline comments: "severity": "error|warning|info|suggestion" } ], - "general_observations": ["Any observations not tied to specific lines"] + + "general_observations": ["Any observations not tied to specific lines"], + + "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` } const userMessage = body.replace(/\/oc!?|\/opencode!?/gi, "").trim() @@ -844,15 +867,25 @@ Keep your response focused on this specific issue only. Do NOT ask Phase 1 quest if (hasNumberedAnswers) { // User is answering questions β†’ Phase 2 - return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers: + return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers. User's answers: ${userMessage} -IMPORTANT: Output your review as structured JSON for inline comments: +IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based on PR type: + \`\`\`json { - "summary": "Brief overall summary (1-2 sentences)", + "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", @@ -861,9 +894,22 @@ IMPORTANT: Output your review as structured JSON for inline comments: "severity": "error|warning|info|suggestion" } ], - "general_observations": ["Any observations not tied to specific lines"] + + "general_observations": ["Any observations not tied to specific lines"], + + "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) as "skipped" with reason` } // /oc or /oc without numbered answers β†’ Phase 1 @@ -1489,31 +1535,74 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` const reviewData = result.data + // Build comprehensive review body + const reviewParts: string[] = [] + + // Header + reviewParts.push("## πŸ€– AI Code Review") + 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, index) => { + 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) => { + 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("") + } + } + + const fullSummary = reviewParts.join("\n") + fallbackFooter + // Format comments for GitHub API const formattedComments = reviewData.comments.map((comment) => ReviewComment.formatForGitHub(comment) ) if (formattedComments.length > 0) { - // Build summary with general observations - let fullSummary = reviewData.summary - if (reviewData.general_observations && reviewData.general_observations.length > 0) { - fullSummary += "\n\n**General Observations:**\n" + - reviewData.general_observations.map((obs) => `- ${obs}`).join("\n") - } - fullSummary += fallbackFooter - await createPullRequestReview(prNumber, fullSummary, formattedComments) console.log(`Posted inline review with ${formattedComments.length} comments`) return true } else { // No inline comments, just post summary - let fullSummary = reviewData.summary - if (reviewData.general_observations && reviewData.general_observations.length > 0) { - fullSummary += "\n\n**General Observations:**\n" + - reviewData.general_observations.map((obs) => `- ${obs}`).join("\n") - } - await createComment(`${fullSummary}${fallbackFooter}`) + await createComment(fullSummary) return false } } catch (e: any) { diff --git a/packages/opencode/src/context/review-comment.ts b/packages/opencode/src/context/review-comment.ts index 9b4cb4fd525d..c9b88a738c37 100644 --- a/packages/opencode/src/context/review-comment.ts +++ b/packages/opencode/src/context/review-comment.ts @@ -4,89 +4,111 @@ 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 - */ - 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().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 - - /** - * The full structured review output from the LLM - */ - export const ReviewOutput = z.object({ - /** Overall summary of the review */ - summary: z.string().describe("Overall summary of the code review"), - - /** 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"), - }) - 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, - } + /** + * 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().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 + */ + export const ChecklistItem = z.object({ + /** The criterion being checked */ + item: z.string().describe("The criterion being checked"), + /** Whether it passed */ + passed: 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 + + /** + * The full structured review output from the LLM + */ + export const ReviewOutput = z.object({ + /** 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"), + + /** 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\`\`\`` } - /** - * Generate the prompt instructions for structured review output - */ - export function getReviewPromptInstructions(): string { - return ` + // 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: @@ -125,5 +147,5 @@ You MUST respond with a JSON object matching this exact schema: - For multi-line suggestions, set \`start_line\` to the first line and \`line\` to the last - Keep comments concise but informative ` - } + } } From e0e93a9d1115bb67f6e64425d10358c76cfc667f Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 03:33:44 +0700 Subject: [PATCH 12/43] feat: Phase 2 enhancements - context_summary, not_reviewed, /oc recheck, Phase 1 skip logic --- packages/opencode/src/cli/cmd/github.ts | 117 +++++++++++++++++- .../opencode/src/context/review-comment.ts | 17 +++ 2 files changed, 128 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index bb2d9dd79248..604c1bba931a 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -739,6 +739,43 @@ export const GithubRunCommand = cmd({ } } + /** + * Check if Phase 1 (clarifying questions) has already been asked for this PR + * by looking for the bot's question pattern in previous comments/reviews + */ + async function hasPhase1BeenAsked(): Promise { + if (!issueId) return false + + try { + // Fetch PR comments and reviews to check for Phase 1 pattern + const prData = await fetchPR() + + // Phase 1 pattern to look for in bot comments + const phase1Pattern = /πŸ€”.*Before I review.*questions/i + + // Check issue-style comments + const comments = prData.comments?.nodes || [] + for (const comment of comments) { + if (comment.body && phase1Pattern.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 + } + } + async function getUserPrompt() { const customPrompt = process.env["PROMPT"] // For repo events and issues events, PROMPT is required since there's no comment to extract from @@ -759,7 +796,7 @@ 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" } @@ -794,6 +831,45 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), // Handle /oc with additional text (user answering questions or providing context) if (mentions.some((m) => bodyLower.includes(m))) { + // 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"} + +INSTRUCTIONS: +1. You should check if the previous issues have been resolved +2. Compare the current diff against previous feedback +3. Mark resolved issues and any remaining issues + +Output your review as structured JSON: +\`\`\`json +{ + "context_summary": "Re-review after fixing: [what was fixed]", + "summary": "Status of previous feedback resolution", + + "checklist": [ + { + "item": "Previous issue: [issue name]", + "passed": true, + "note": "βœ… Resolved in commit xyz" + } + ], + + "comments": [], + + "not_reviewed": [ + {"item": "Items from previous review", "reason": "Already addressed"} + ], + + "decision": "APPROVE|REQUEST_CHANGES", + "decision_reason": "All previous issues resolved / Some issues remain" +} +\`\`\`` + } + // Check for direct review trigger first (/oc! anywhere in text) const isDirectReview = mentions.some((m) => bodyLower.includes(m + "!")) if (isDirectReview) { @@ -805,6 +881,7 @@ IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based \`\`\`json { + "context_summary": "${userMessage || "Direct review requested"}", "summary": "1-2 sentence summary of what this PR does", "checklist": [ @@ -826,6 +903,10 @@ IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based "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" } @@ -848,6 +929,10 @@ CHECKLIST GENERATION RULES: // 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. @@ -865,17 +950,18 @@ Respond directly to the user's message. If they've acknowledged a fix, confirm a Keep your response focused on this specific issue only. Do NOT ask Phase 1 questions.` } - if (hasNumberedAnswers) { - // User is answering questions β†’ Phase 2 - return `[PHASE_2] User has answered your clarifying questions. Now provide the focused review based on their answers. + 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 answers: +User's context/answers: ${userMessage} IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based on PR type: \`\`\`json { + "context_summary": "Summarize user's context/focus from their message", "summary": "1-2 sentence summary of what this PR does", "checklist": [ @@ -897,6 +983,10 @@ IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based "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" } @@ -909,7 +999,7 @@ CHECKLIST GENERATION RULES: - 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) as "skipped" with reason` +- Mark items NOT reviewed (per user context) with "skipped" status and reason` } // /oc or /oc without numbered answers β†’ Phase 1 @@ -1542,6 +1632,12 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` 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("") @@ -1589,6 +1685,15 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` } } + // 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) => { + reviewParts.push(`- ~~${item.item}~~ β†’ ${item.reason}`) + }) + reviewParts.push("") + } + const fullSummary = reviewParts.join("\n") + fallbackFooter // Format comments for GitHub API diff --git a/packages/opencode/src/context/review-comment.ts b/packages/opencode/src/context/review-comment.ts index c9b88a738c37..ea65ac8081d0 100644 --- a/packages/opencode/src/context/review-comment.ts +++ b/packages/opencode/src/context/review-comment.ts @@ -44,10 +44,24 @@ export namespace ReviewComment { }) 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 */ 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"), @@ -60,6 +74,9 @@ export namespace ReviewComment { /** 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"), From 70932c06323404ac7a3d2f76ccb5bb2107553b2b Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 03:43:58 +0700 Subject: [PATCH 13/43] fix: skip Phase 1 if user previously used /oc! direct review --- packages/opencode/src/cli/cmd/github.ts | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 604c1bba931a..5207e49fdf1f 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -741,23 +741,33 @@ export const GithubRunCommand = cmd({ /** * Check if Phase 1 (clarifying questions) has already been asked for this PR - * by looking for the bot's question pattern in previous comments/reviews + * 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 Phase 1 pattern + // Fetch PR comments and reviews to check for previous engagement const prData = await fetchPR() - // Phase 1 pattern to look for in bot comments + // 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 && phase1Pattern.test(comment.body)) { - return true + 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 + } } } From 6a0e58d80e5b17b35c4c1a25c9c30a9f6a5a05fa Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 04:13:25 +0700 Subject: [PATCH 14/43] fix: strict JSON-only prompts and proper GitHub Actions URL in footer --- packages/opencode/src/cli/cmd/github.ts | 43 ++++++++++++++++--------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 5207e49fdf1f..e306988adaed 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -485,7 +485,7 @@ 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" let appToken: string @@ -849,35 +849,46 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), User's context: ${userMessage || "Fixed previous issues"} -INSTRUCTIONS: -1. You should check if the previous issues have been resolved -2. Compare the current diff against previous feedback -3. Mark resolved issues and any remaining issues +CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. -Output your review as structured JSON: \`\`\`json { - "context_summary": "Re-review after fixing: [what was fixed]", - "summary": "Status of previous feedback resolution", + "context_summary": "Re-review: [brief context of what was fixed]", + "summary": "1-2 sentence status of previous feedback resolution", "checklist": [ { - "item": "Previous issue: [issue name]", + "item": "Previous issue name", "passed": true, - "note": "βœ… Resolved in commit xyz" + "note": "βœ… Resolved / ❌ Still open - brief explanation" } ], - "comments": [], + "comments": [ + { + "path": "src/path/to/file.js", + "line": 42, + "body": "Issue still present: description", + "severity": "error|warning|info|suggestion" + } + ], "not_reviewed": [ - {"item": "Items from previous review", "reason": "Already addressed"} + {"item": "Item name", "reason": "Already addressed / Not in scope"} ], "decision": "APPROVE|REQUEST_CHANGES", - "decision_reason": "All previous issues resolved / Some issues remain" + "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` } // Check for direct review trigger first (/oc! anywhere in text) @@ -887,7 +898,7 @@ Output your review as structured JSON: return `[DIRECT_REVIEW] Review this pull request directly without asking clarifying questions. User context: ${userMessage || "None provided"} -IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based on PR type: +CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. \`\`\`json { @@ -967,7 +978,7 @@ Keep your response focused on this specific issue only. Do NOT ask Phase 1 quest User's context/answers: ${userMessage} -IMPORTANT: Output your review as structured JSON with a DYNAMIC checklist based on PR type: +CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. \`\`\`json { From cb303f830e436c4344c428de64d7848673a7adfc Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 04:50:30 +0700 Subject: [PATCH 15/43] fix: validate comment paths against PR diff before posting inline review --- packages/opencode/src/cli/cmd/github.ts | 46 +++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index e306988adaed..f87d699c4ccc 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -1723,9 +1723,49 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` ) if (formattedComments.length > 0) { - await createPullRequestReview(prNumber, fullSummary, formattedComments) - console.log(`Posted inline review with ${formattedComments.length} comments`) - return true + // Get files in the PR to validate paths + const { data: prData } = await octoRest.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }) + + // Fetch changed files in PR + const { data: prFiles } = await octoRest.rest.pulls.listFiles({ + owner, + repo, + pull_number: prNumber, + per_page: 100, + }) + const validPaths = new Set(prFiles.map((f) => f.filename)) + + // Filter comments to only include valid paths + const validComments = formattedComments.filter((c) => { + if (validPaths.has(c.path)) { + return true + } + console.warn(`Skipping comment for invalid path: ${c.path} (not in PR diff)`) + return false + }) + + // If some comments were filtered, add them to the summary + const invalidComments = formattedComments.filter((c) => !validPaths.has(c.path)) + let summaryWithInvalid = fullSummary + if (invalidComments.length > 0) { + summaryWithInvalid = fullSummary + "\n\n**Additional notes (files not in this PR):**\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) From 20fbd905e81d146fe43c3ea47ad50fa6ba83229d Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 07:46:08 +0700 Subject: [PATCH 16/43] feat: branch-aware strict review - STRICT_REVIEW_BRANCHES env for concurrent rules --- packages/opencode/src/cli/cmd/github.ts | 66 ++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index f87d699c4ccc..0dca12aeaca5 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -488,6 +488,12 @@ export const GithubRunCommand = cmd({ 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 @@ -786,6 +792,36 @@ export const GithubRunCommand = cmd({ } } + /** + * 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) + + 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: "" } + } + } + async function getUserPrompt() { const customPrompt = process.env["PROMPT"] // For repo events and issues events, PROMPT is required since there's no comment to extract from @@ -939,7 +975,20 @@ CHECKLIST GENERATION RULES: - 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` +- 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 `- ${message || "No strict branches configured."} +- Show concurrent rule violations as INFO/reminder only, NOT blocking +- Decision can still be APPROVE with info notes about what needs to be done before production` + } + })()}` } const userMessage = body.replace(/\/oc!?|\/opencode!?/gi, "").trim() @@ -1020,7 +1069,20 @@ CHECKLIST GENERATION RULES: - 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` +- 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 `- ${message || "No strict branches configured."} +- Show concurrent rule violations as INFO/reminder only, NOT blocking +- Decision can still be APPROVE with info notes about what needs to be done before production` + } + })()}` } // /oc or /oc without numbered answers β†’ Phase 1 From 19e4f3522b8eb08f82c0fe792f124f2fb4ca364a Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 07:53:26 +0700 Subject: [PATCH 17/43] debug: add logging for STRICT_REVIEW_BRANCHES comparison --- packages/opencode/src/cli/cmd/github.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 0dca12aeaca5..734d009b279b 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -809,6 +809,14 @@ export const GithubRunCommand = cmd({ 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 From f3799da620e459c5ffe6d05f5cbf7e46a297302b Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 11:10:59 +0700 Subject: [PATCH 18/43] fix: add branch-aware strictness to /oc recheck prompt (was missing) --- packages/opencode/src/cli/cmd/github.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 734d009b279b..7482fdc207b0 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -932,7 +932,21 @@ RULES: - 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` +- Checklist items should track resolution of PREVIOUS issues + +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 `- ${message || "No strict branches configured."} +- Show concurrent rule violations as INFO/reminder only, NOT blocking +- Decision can still be APPROVE if the actual code changes in this PR are correct +- Add a note: "Models/hooks needed before merging to protected branch"` + } + })()}` } // Check for direct review trigger first (/oc! anywhere in text) From 818cf04dec4ad349de1df634d09511b2a2607382 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 13:43:43 +0700 Subject: [PATCH 19/43] fix: explicit non-strict branch behavior - use SKIPPED not FAIL, APPROVE with reminders --- packages/opencode/src/cli/cmd/github.ts | 38 ++++++++++++++++++------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 7482fdc207b0..0d3297d08acf 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -818,10 +818,9 @@ export const GithubRunCommand = cmd({ 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.` - : "" +real simulation.but in cli : strictReviewBranches.length > 0 + ? `ℹ️ This PR targets **${prData.baseRefName}** (non-protected). Concurrent rules shown as INFO only.` + : "" return { isStrict, targetBranch, message } } catch (e) { @@ -942,9 +941,14 @@ ${await (async () => { - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { return `- ${message || "No strict branches configured."} -- Show concurrent rule violations as INFO/reminder only, NOT blocking -- Decision can still be APPROVE if the actual code changes in this PR are correct -- Add a note: "Models/hooks needed before merging to protected branch"` + +IMPORTANT - NON-STRICT BRANCH BEHAVIOR: +- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL +- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items +- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) +- Decision should be APPROVE if the code IN THIS PR is correct +- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" +- Do NOT block the PR for files that are intentionally out of scope` } })()}` } @@ -1007,8 +1011,14 @@ ${await (async () => { - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { return `- ${message || "No strict branches configured."} -- Show concurrent rule violations as INFO/reminder only, NOT blocking -- Decision can still be APPROVE with info notes about what needs to be done before production` + +IMPORTANT - NON-STRICT BRANCH BEHAVIOR: +- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL +- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items +- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) +- Decision should be APPROVE if the code IN THIS PR is correct +- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" +- Do NOT block the PR for files that are intentionally out of scope` } })()}` } @@ -1101,8 +1111,14 @@ ${await (async () => { - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { return `- ${message || "No strict branches configured."} -- Show concurrent rule violations as INFO/reminder only, NOT blocking -- Decision can still be APPROVE with info notes about what needs to be done before production` + +IMPORTANT - NON-STRICT BRANCH BEHAVIOR: +- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL +- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items +- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) +- Decision should be APPROVE if the code IN THIS PR is correct +- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" +- Do NOT block the PR for files that are intentionally out of scope` } })()}` } From 56cbd7c663410a7b57d0cb3df16429cebbd6c115 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 13:51:16 +0700 Subject: [PATCH 20/43] feat: enhance database-indexes rule with query pattern reasoning framework --- packages/opencode/src/context/rules.ts | 27 +++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/packages/opencode/src/context/rules.ts b/packages/opencode/src/context/rules.ts index 1b2c97a99800..46b189e9820b 100644 --- a/packages/opencode/src/context/rules.ts +++ b/packages/opencode/src/context/rules.ts @@ -577,13 +577,26 @@ export namespace ReviewRules { maxFiles: 10, maxLinesPerFile: 300, }, - prompt: `When reviewing migrations: -- Foreign keys should have indexes -- Unique constraints should have indexes -- Columns used in WHERE/ORDER BY frequently need indexes -- Compound indexes for multi-column queries -- Consider partial indexes for filtered queries -- Check for missing indexes on new columns`, + 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 ====== From cd4f298555973f17ab8dd1da31ac35637ff998e7 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 13:52:03 +0700 Subject: [PATCH 21/43] fix: oops --- packages/opencode/src/cli/cmd/github.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 0d3297d08acf..329c32a5d1f8 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -818,9 +818,10 @@ export const GithubRunCommand = cmd({ console.log("=========================================") const message = isStrict -real simulation.but in cli : strictReviewBranches.length > 0 - ? `ℹ️ This PR targets **${prData.baseRefName}** (non-protected). Concurrent rules shown as INFO only.` - : "" + ? `⚠️ 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) { @@ -1111,14 +1112,8 @@ ${await (async () => { - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { return `- ${message || "No strict branches configured."} - -IMPORTANT - NON-STRICT BRANCH BEHAVIOR: -- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL -- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items -- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) -- Decision should be APPROVE if the code IN THIS PR is correct -- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" -- Do NOT block the PR for files that are intentionally out of scope` +- Show concurrent rule violations as INFO/reminder only, NOT blocking +- Decision can still be APPROVE with info notes about what needs to be done before production` } })()}` } From e68af079df76b5bc288dfa05fc06ab13f891d82a Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 13:59:48 +0700 Subject: [PATCH 22/43] fix: restore explicit non-strict behavior in Phase 2 prompt --- packages/opencode/src/cli/cmd/github.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 329c32a5d1f8..420487524f95 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -1112,8 +1112,14 @@ ${await (async () => { - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { return `- ${message || "No strict branches configured."} -- Show concurrent rule violations as INFO/reminder only, NOT blocking -- Decision can still be APPROVE with info notes about what needs to be done before production` + +IMPORTANT - NON-STRICT BRANCH BEHAVIOR: +- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL +- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items +- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) +- Decision should be APPROVE if the code IN THIS PR is correct +- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" +- Do NOT block the PR for files that are intentionally out of scope` } })()}` } From f9ca1b1c08b432fbf4aa28b9c7f420ed1e63579d Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 14:05:55 +0700 Subject: [PATCH 23/43] fix: make context injection rules branch-aware - defer to prompt's CONCURRENT IMPLEMENTATION RULES --- packages/opencode/src/context/rules.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/context/rules.ts b/packages/opencode/src/context/rules.ts index 46b189e9820b..d80f158ea99c 100644 --- a/packages/opencode/src/context/rules.ts +++ b/packages/opencode/src/context/rules.ts @@ -95,13 +95,17 @@ export namespace ReviewRules { maxFiles: 15, maxLinesPerFile: 300, }, - prompt: `When reviewing models or migrations: + 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 -- For Rails: verify belongs_to/has_many 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 ====== @@ -736,13 +740,15 @@ Always explain WHY an index is needed based on expected query patterns.`, maxFiles: 10, maxLinesPerFile: 200, }, - prompt: `When reviewing models with soft delete: + 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`, +- Restore functionality if needed + +Note: Enforcement level depends on target branch per "CONCURRENT IMPLEMENTATION RULES" in review context`, }, // ====== PERMISSION / AUTHORIZATION ====== From cb154cf2c1d978a90ab87edd7711369c1da92d2c Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 14:13:25 +0700 Subject: [PATCH 24/43] fix: strengthen non-strict branch instructions with CRITICAL section and explicit examples --- packages/opencode/src/cli/cmd/github.ts | 66 +++++++++++++++---------- 1 file changed, 39 insertions(+), 27 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 420487524f95..3af02ff8f9d7 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -941,15 +941,23 @@ ${await (async () => { return `- ${message} - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { - return `- ${message || "No strict branches configured."} - -IMPORTANT - NON-STRICT BRANCH BEHAVIOR: -- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL -- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items -- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) -- Decision should be APPROVE if the code IN THIS PR is correct -- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" -- Do NOT block the PR for files that are intentionally out of scope` + 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) as FAIL +2. Use "passed": null with note "⏭️ Skipped - to be added in separate PR" +3. Focus ONLY on reviewing the actual code IN THIS PR +4. Decision should be APPROVE if the code IN THIS PR is correct +5. Do NOT use REQUEST_CHANGES for missing files outside this PR +6. Think: "Is the migration code correct?" not "Are all related files present?" + +CORRECT OUTPUT FOR NON-STRICT BRANCH: +- Checklist item for missing models: {"item": "Model files", "passed": null, "note": "⏭️ Skipped - to be added in separate PR before merging to protected branch"} +- Decision: "APPROVE" (assuming migration code is correct) +- not_reviewed: "Models/hooks needed before merging to protected branch" +=== END CRITICAL SECTION ===` } })()}` } @@ -1011,15 +1019,17 @@ ${await (async () => { return `- ${message} - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { - return `- ${message || "No strict branches configured."} - -IMPORTANT - NON-STRICT BRANCH BEHAVIOR: -- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL -- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items -- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) -- Decision should be APPROVE if the code IN THIS PR is correct -- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" -- Do NOT block the PR for files that are intentionally out of scope` + 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) as FAIL +2. Use "passed": null with note "⏭️ Skipped - to be added in separate PR" +3. Focus ONLY on reviewing the actual code IN THIS PR +4. Decision should be APPROVE if the code IN THIS PR is correct +5. Do NOT use REQUEST_CHANGES for missing files outside this PR +=== END CRITICAL SECTION ===` } })()}` } @@ -1111,15 +1121,17 @@ ${await (async () => { return `- ${message} - ENFORCE these rules: Missing concurrent implementations should be marked as FAIL and decision should be REQUEST_CHANGES` } else { - return `- ${message || "No strict branches configured."} - -IMPORTANT - NON-STRICT BRANCH BEHAVIOR: -- Do NOT mark missing concurrent deps (models, hooks, associations) as FAIL -- Use "passed": null with note "⏭️ Skipped - to be added in separate PR" for concurrent items -- Focus ONLY on reviewing the actual code IN THIS PR (migrations, tests, etc.) -- Decision should be APPROVE if the code IN THIS PR is correct -- Add reminder in not_reviewed: "Models/hooks needed before merging to protected branch" -- Do NOT block the PR for files that are intentionally out of scope` + 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) as FAIL +2. Use "passed": null with note "⏭️ Skipped - to be added in separate PR" +3. Focus ONLY on reviewing the actual code IN THIS PR +4. Decision should be APPROVE if the code IN THIS PR is correct +5. Do NOT use REQUEST_CHANGES for missing files outside this PR +=== END CRITICAL SECTION ===` } })()}` } From b1a714fb47ad1c7d6974dda6b7f87cb95fbae167 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 20:22:08 +0700 Subject: [PATCH 25/43] feat: add RAKAMIND.md and OPENCODE.md to supported project rules files --- packages/opencode/src/context/rules.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/opencode/src/context/rules.ts b/packages/opencode/src/context/rules.ts index d80f158ea99c..bd641ecec10e 100644 --- a/packages/opencode/src/context/rules.ts +++ b/packages/opencode/src/context/rules.ts @@ -58,6 +58,8 @@ export namespace ReviewRules { "**/.github/runner-scripts/memory-bank.md", "**/AGENTS.md", "**/CLAUDE.md", + "**/OPENCODE.md", + "**/RAKAMIND.md", ], maxFiles: 3, maxLinesPerFile: 2000, From 9b8fcf5efb07ea3654a9584922fcead5406de94a Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 20:45:48 +0700 Subject: [PATCH 26/43] feat: add multi-line comments with committable suggestions in PR reviews --- packages/opencode/src/cli/cmd/github.ts | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 3af02ff8f9d7..221ff56a8fe4 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -911,9 +911,11 @@ CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. "comments": [ { "path": "src/path/to/file.js", - "line": 42, - "body": "Issue still present: description", - "severity": "error|warning|info|suggestion" + "start_line": 42, + "line": 55, + "body": "Issue description", + "severity": "error|warning|info|suggestion", + "suggestion": "// corrected code - will create committable suggestion" } ], @@ -987,9 +989,11 @@ CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. "comments": [ { "path": "src/path/to/file.js", - "line": 42, + "start_line": 42, + "line": 55, "body": "Issue description", - "severity": "error|warning|info|suggestion" + "severity": "error|warning|info|suggestion", + "suggestion": "// corrected code - will create committable suggestion" } ], @@ -1088,9 +1092,11 @@ CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. "comments": [ { "path": "src/path/to/file.js", - "line": 42, + "start_line": 42, + "line": 55, "body": "Issue description", - "severity": "error|warning|info|suggestion" + "severity": "error|warning|info|suggestion", + "suggestion": "// corrected code - will create committable suggestion" } ], From 03389371b743a4bf3da8893ef80bfd36c0c8467c Mon Sep 17 00:00:00 2001 From: blacksyan Date: Sat, 3 Jan 2026 20:55:40 +0700 Subject: [PATCH 27/43] fix: add explicit instruction to recheck prompt - REVIEW only, do not apply fixes --- packages/opencode/src/cli/cmd/github.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 221ff56a8fe4..6738d4ee8744 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -893,7 +893,13 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), User's context: ${userMessage || "Fixed previous issues"} -CRITICAL: Output ONLY valid JSON. NO explanations. NO text before or after JSON. +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 { From 5e3212ab77b703e1cf55ee5b4e2f16bedec0ab20 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 10:33:33 +0700 Subject: [PATCH 28/43] fix: remove auto-push behavior for PR reviews - review-only mode - Disabled auto-commit and push after AI response - Any file changes made by AI are discarded (git checkout -- .) - OpenCode now only reviews and posts comments, never modifies code --- packages/opencode/src/cli/cmd/github.ts | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 6738d4ee8744..2563fc102383 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -592,10 +592,12 @@ export const GithubRunCommand = cmd({ const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) - const { dirty, uncommittedChanges } = await branchIsDirty(head) + // REVIEW-ONLY MODE: Do not push any changes made by AI + // If branch is dirty, just log a warning but don't commit + const { dirty } = await branchIsDirty(head) if (dirty) { - const summary = await summarize(response) - await pushToLocalBranch(summary, uncommittedChanges) + console.warn("⚠️ Branch is dirty after AI response - discarding changes (review-only mode)") + await $`git checkout -- .` // Discard any changes } const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl} / s / ${shareId}`)) // Try to post inline review comments, fall back to regular comment @@ -612,10 +614,12 @@ export const GithubRunCommand = cmd({ const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) - const { dirty, uncommittedChanges } = await branchIsDirty(head) + // REVIEW-ONLY MODE: Do not push any changes made by AI + // If branch is dirty, just log a warning but don't commit + const { dirty } = await branchIsDirty(head) if (dirty) { - const summary = await summarize(response) - await pushToForkBranch(summary, prData, uncommittedChanges) + console.warn("⚠️ Branch is dirty after AI response - discarding changes (review-only mode)") + await $`git checkout -- .` // Discard any changes } const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl} / s / ${shareId}`)) // Try to post inline review comments, fall back to regular comment @@ -1302,6 +1306,12 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), 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: [ { From fac28513c53c4e70c908688896f2bbfc3a3f6976 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 10:54:14 +0700 Subject: [PATCH 29/43] fix: validate inline comment lines against PR diff - Parse Git patch to extract valid line ranges from @@ hunk headers - Filter comments to only include lines present in the diff - Add prompt guidance: only comment on added/modified lines - Invalid comments are shown in summary instead of causing API errors - Prevents GitHub API 422 'Line could not be resolved' errors --- packages/opencode/src/cli/cmd/github.ts | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 2563fc102383..a5d6c10e9ecd 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -592,12 +592,10 @@ export const GithubRunCommand = cmd({ const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) - // REVIEW-ONLY MODE: Do not push any changes made by AI - // If branch is dirty, just log a warning but don't commit - const { dirty } = await branchIsDirty(head) + const { dirty, uncommittedChanges } = await branchIsDirty(head) if (dirty) { - console.warn("⚠️ Branch is dirty after AI response - discarding changes (review-only mode)") - await $`git checkout -- .` // Discard any changes + const summary = await summarize(response) + await pushToLocalBranch(summary, uncommittedChanges) } const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl} / s / ${shareId}`)) // Try to post inline review comments, fall back to regular comment @@ -614,12 +612,10 @@ export const GithubRunCommand = cmd({ const head = (await $`git rev-parse HEAD`).stdout.toString().trim() const dataPrompt = await buildPromptDataForPR(prData) const response = await chat(`${userPrompt}\n\n${dataPrompt}`, promptFiles) - // REVIEW-ONLY MODE: Do not push any changes made by AI - // If branch is dirty, just log a warning but don't commit - const { dirty } = await branchIsDirty(head) + const { dirty, uncommittedChanges } = await branchIsDirty(head) if (dirty) { - console.warn("⚠️ Branch is dirty after AI response - discarding changes (review-only mode)") - await $`git checkout -- .` // Discard any changes + const summary = await summarize(response) + await pushToForkBranch(summary, prData, uncommittedChanges) } const hasShared = prData.comments.nodes.some((c) => c.body.includes(`${shareBaseUrl} / s / ${shareId}`)) // Try to post inline review comments, fall back to regular comment @@ -945,6 +941,7 @@ RULES: - 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. CONCURRENT IMPLEMENTATION RULES (model-migration-sync, controller-service, etc.): ${await (async () => { @@ -1306,12 +1303,6 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), 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: [ { From 5f703d7c0eb967579791deaf40b2628691588a9b Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 10:59:48 +0700 Subject: [PATCH 30/43] fix: normalize null suggestion to undefined using transform - Accepts null from LLM but transforms to undefined for cleaner type - Type is now 'string | undefined' instead of 'string | null | undefined' --- packages/opencode/src/context/review-comment.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opencode/src/context/review-comment.ts b/packages/opencode/src/context/review-comment.ts index ea65ac8081d0..1d0c4459aa32 100644 --- a/packages/opencode/src/context/review-comment.ts +++ b/packages/opencode/src/context/review-comment.ts @@ -24,7 +24,7 @@ export namespace ReviewComment { body: z.string().describe("The comment text in markdown"), /** Optional code suggestion to replace the commented lines */ - suggestion: z.string().optional().describe("Code suggestion to replace the commented lines"), + suggestion: z.string().optional().nullable().transform(v => v ?? undefined).describe("Code suggestion to replace the commented lines"), /** Severity of the issue */ severity: z.enum(["error", "warning", "info", "suggestion"]).optional().default("info"), From 0a563a9d11970485cb53ebccd20a38bf71b85f7b Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 11:12:20 +0700 Subject: [PATCH 31/43] fix: mark schema definitions as skippable when migrations are in another PR - Added schema definitions to list of skippable concurrent deps - Explicitly states static schema(), column definitions are PART OF migrations - Updated all 3 prompts consistently - AI now correctly marks these as Skipped instead of Fail --- packages/opencode/src/cli/cmd/github.ts | 40 +++++++++++++------------ 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index a5d6c10e9ecd..1df8fe15a54b 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -955,17 +955,17 @@ ${await (async () => { ${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) as FAIL -2. Use "passed": null with note "⏭️ Skipped - to be added in separate PR" -3. Focus ONLY on reviewing the actual code IN THIS PR -4. Decision should be APPROVE if the code IN THIS PR is correct -5. Do NOT use REQUEST_CHANGES for missing files outside this PR -6. Think: "Is the migration code correct?" not "Are all related files present?" +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. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too +4. Focus ONLY on reviewing the actual code IN THIS PR +5. Decision should be APPROVE if the code IN THIS PR is correct +6. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR CORRECT OUTPUT FOR NON-STRICT BRANCH: -- Checklist item for missing models: {"item": "Model files", "passed": null, "note": "⏭️ Skipped - to be added in separate PR before merging to protected branch"} -- Decision: "APPROVE" (assuming migration code is correct) -- not_reviewed: "Models/hooks needed before merging to protected branch" +- Checklist item for skipped items: {"item": "Schema definitions", "passed": null, "note": "⏭️ Skipped - handled in migration PR per user context"} +- Decision: "APPROVE" (assuming code in this PR is correct) +- not_reviewed: List what was skipped and why === END CRITICAL SECTION ===` } })()}` @@ -1035,11 +1035,12 @@ ${await (async () => { ${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) as FAIL -2. Use "passed": null with note "⏭️ Skipped - to be added in separate PR" -3. Focus ONLY on reviewing the actual code IN THIS PR -4. Decision should be APPROVE if the code IN THIS PR is correct -5. Do NOT use REQUEST_CHANGES for missing files outside this PR +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. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too +4. Focus ONLY on reviewing the actual code IN THIS PR +5. Decision should be APPROVE if the code IN THIS PR is correct +6. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR === END CRITICAL SECTION ===` } })()}` @@ -1139,11 +1140,12 @@ ${await (async () => { ${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) as FAIL -2. Use "passed": null with note "⏭️ Skipped - to be added in separate PR" -3. Focus ONLY on reviewing the actual code IN THIS PR -4. Decision should be APPROVE if the code IN THIS PR is correct -5. Do NOT use REQUEST_CHANGES for missing files outside this PR +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. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too +4. Focus ONLY on reviewing the actual code IN THIS PR +5. Decision should be APPROVE if the code IN THIS PR is correct +6. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR === END CRITICAL SECTION ===` } })()}` From f852d1a02fa5e39a086eb6c54f897eb9b54612ce Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 11:17:53 +0700 Subject: [PATCH 32/43] fix: defense-in-depth for suggestion markdown - prompt + sanitization - Added prompt rule: suggestion must be RAW CODE ONLY - Added Zod transform to strip markdown code blocks if AI includes them - Prevents 'Unterminated string' JSON parse errors --- packages/opencode/src/cli/cmd/github.ts | 1 + packages/opencode/src/context/review-comment.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 1df8fe15a54b..bf8105f70123 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -942,6 +942,7 @@ RULES: - 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 () => { diff --git a/packages/opencode/src/context/review-comment.ts b/packages/opencode/src/context/review-comment.ts index 1d0c4459aa32..3248784521a5 100644 --- a/packages/opencode/src/context/review-comment.ts +++ b/packages/opencode/src/context/review-comment.ts @@ -24,7 +24,14 @@ export namespace ReviewComment { body: z.string().describe("The comment text in markdown"), /** Optional code suggestion to replace the commented lines */ - suggestion: z.string().optional().nullable().transform(v => v ?? undefined).describe("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"), From c7b9284d74736c4b6e6618bbd88bfddd746f3282 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 11:22:11 +0700 Subject: [PATCH 33/43] fix: coerce invalid passed values to null using preprocess AI sometimes uses 'warning' or other strings for passed field. Added defensive preprocess to coerce non-boolean values to null. --- packages/opencode/src/context/review-comment.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/context/review-comment.ts b/packages/opencode/src/context/review-comment.ts index 3248784521a5..a0404f7454f4 100644 --- a/packages/opencode/src/context/review-comment.ts +++ b/packages/opencode/src/context/review-comment.ts @@ -45,7 +45,11 @@ export namespace ReviewComment { /** The criterion being checked */ item: z.string().describe("The criterion being checked"), /** Whether it passed */ - passed: z.boolean().nullable().describe("true=passed, false=failed, null=skipped"), + 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"), }) From 799bc5bc7e9c92639f5cfa7b37f27155869be839 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 11:47:36 +0700 Subject: [PATCH 34/43] feat: robust PR review capabilities - Re-implemented line-range validation against PR diff (fixes 422 'Line could not be resolved') - Added defensive schema handling to normalize 'null' suggestions and 'warning' statuses - Added markdown sanitization for code suggestions to prevent JSON parse errors - Enhanced non-strict branch rules to explicitly skip schema/model internals when handled in concurrent PRs --- packages/opencode/src/cli/cmd/github.ts | 74 +++++++++++++++++++++---- 1 file changed, 63 insertions(+), 11 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index bf8105f70123..f6455ce2e52f 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -1693,6 +1693,39 @@ 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. + */ + 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 + } + /** * Create a pull request review with inline comments on specific lines */ @@ -1866,22 +1899,41 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` pull_number: prNumber, per_page: 100, }) - const validPaths = new Set(prFiles.map((f) => f.filename)) + // 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) + } - // Filter comments to only include valid paths - const validComments = formattedComments.filter((c) => { - if (validPaths.has(c.path)) { - return true + // Filter comments to only include valid paths AND lines in the diff + const validComments: typeof formattedComments = [] + const invalidComments: typeof formattedComments = [] + + for (const comment of formattedComments) { + const validLines = validPathsWithLines.get(comment.path) + if (!validLines) { + console.warn(`Skipping comment: path "${comment.path}" not in PR diff`) + invalidComments.push(comment) + continue } - console.warn(`Skipping comment for invalid path: ${c.path} (not in PR diff)`) - return false - }) - // If some comments were filtered, add them to the summary - const invalidComments = formattedComments.filter((c) => !validPaths.has(c.path)) + // 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) { + validComments.push(comment) + } else { + console.warn(`Skipping comment: line ${comment.start_line || comment.line}-${comment.line} not in diff for "${comment.path}"`) + invalidComments.push(comment) + } + } + let summaryWithInvalid = fullSummary if (invalidComments.length > 0) { - summaryWithInvalid = fullSummary + "\n\n**Additional notes (files not in this PR):**\n" + + summaryWithInvalid = fullSummary + "\n\n**Additional notes (outside diff range):**\n" + invalidComments.map((c) => `- **${c.path}:${c.line}** - ${c.body}`).join("\n") } From 7d1af8351c92d4ed80015786fbd3525b33729822 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 12:02:11 +0700 Subject: [PATCH 35/43] fix: explicit skip rules for Previous Feedback in non-strict branches - Updated Recheck, Direct Review, and Phase 2 prompts - Explicitly instructs AI to mark Previous Feedback as 'Skipped' (passed: null) if fix is in a separate PR - Strengthens the non-strict override over the global 'track previous issues' rule --- packages/opencode/src/cli/cmd/github.ts | 31 ++++++++++++++----------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index f6455ce2e52f..0fb6cad0519a 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -910,7 +910,7 @@ Output ONLY valid JSON. NO explanations. NO text before or after JSON. { "item": "Previous issue name", "passed": true, - "note": "βœ… Resolved / ❌ Still open - brief explanation" + "note": "βœ… Resolved / ❌ Still open / ⏭️ Skipped (if fixed in another PR per context)" } ], @@ -958,13 +958,14 @@ ${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. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too -4. Focus ONLY on reviewing the actual code IN THIS PR -5. Decision should be APPROVE if the code IN THIS PR is correct -6. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR +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 skipped items: {"item": "Schema definitions", "passed": null, "note": "⏭️ Skipped - handled in migration PR per user context"} +- 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 ===` @@ -1038,10 +1039,11 @@ ${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. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too -4. Focus ONLY on reviewing the actual code IN THIS PR -5. Decision should be APPROVE if the code IN THIS PR is correct -6. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR +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 ===` } })()}` @@ -1143,10 +1145,11 @@ ${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. Schema definitions (static schema()), column definitions, and model internals are PART OF migrations - skip them too -4. Focus ONLY on reviewing the actual code IN THIS PR -5. Decision should be APPROVE if the code IN THIS PR is correct -6. Do NOT use REQUEST_CHANGES for missing files or definitions outside this PR +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 ===` } })()}` From 9bef95524921b46d6f91cb2c8e6859a89d659d5f Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 12:38:02 +0700 Subject: [PATCH 36/43] fix: robust pre-parse sanitization for markdown in JSON strings - Upgraded regex to handle escaped quotes: (?:[^"\\]|\\.)* - Applies to ALL string fields, not just 'suggestion' - Handles both escaped (\n) and actual newlines in code blocks --- packages/opencode/src/cli/cmd/github.ts | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 0fb6cad0519a..20c000d5b1f1 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -1797,7 +1797,26 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` } try { - const jsonStr = jsonMatch[1] || jsonMatch[0] + let jsonStr = jsonMatch[1] || jsonMatch[0] + + // Pre-parse sanitization: Strip markdown code blocks from ALL string fields + // AI sometimes embeds ```javascript blocks inside strings, breaking JSON.parse + // Regex: Match JSON string values (handling escaped chars properly) + jsonStr = jsonStr.replace( + /:\s*"((?:[^"\\]|\\.)*)"/g, + (match, content) => { + // Only process if contains markdown code blocks + if (!content.includes('```')) return match + // Remove markdown code blocks from the content + const cleaned = content + .replace(/```\w*\\n?/g, '') // Remove opening ``` (with escaped newline) + .replace(/\\n?```/g, '') // Remove closing ``` + .replace(/```\w*\n?/g, '') // Remove opening ``` (with actual newline) + .replace(/\n?```/g, '') // Remove closing ``` + return `: "${cleaned}"` + } + ) + const parsed = JSON.parse(jsonStr) // Validate with our schema From 1d48ab85a32aa2e0853e0a23ca6b59737a42c62e Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 12:47:46 +0700 Subject: [PATCH 37/43] fix: world-class JSON repair pipeline for LLM output - Implemented character-aware JSON repair scanner - Automatically escapes unescaped control chars (newlines) in strings - Strips hallucinated triple-backtick blocks - Normalizes trailing commas in objects/arrays - This provides absolute resilience against common LLM formatting hallucinations. --- packages/opencode/src/cli/cmd/github.ts | 84 ++++++++++++++++++------- 1 file changed, 63 insertions(+), 21 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 20c000d5b1f1..8a5877477062 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -1777,6 +1777,67 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` } } + /** + * 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 + */ + function repairAndParseJson(raw: string): any { + let text = raw.trim() + + // Phase 1: Basic structural cleaning + // Normalize trailing commas in objects and arrays + 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) { + inString = !inString + repaired += char + } 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. + 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) + .replace(/\n?```/gi, '') // Remove closing blocks (raw) + + 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}`) + } + } + /** * 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. @@ -1797,27 +1858,8 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` } try { - let jsonStr = jsonMatch[1] || jsonMatch[0] - - // Pre-parse sanitization: Strip markdown code blocks from ALL string fields - // AI sometimes embeds ```javascript blocks inside strings, breaking JSON.parse - // Regex: Match JSON string values (handling escaped chars properly) - jsonStr = jsonStr.replace( - /:\s*"((?:[^"\\]|\\.)*)"/g, - (match, content) => { - // Only process if contains markdown code blocks - if (!content.includes('```')) return match - // Remove markdown code blocks from the content - const cleaned = content - .replace(/```\w*\\n?/g, '') // Remove opening ``` (with escaped newline) - .replace(/\\n?```/g, '') // Remove closing ``` - .replace(/```\w*\n?/g, '') // Remove opening ``` (with actual newline) - .replace(/\n?```/g, '') // Remove closing ``` - return `: "${cleaned}"` - } - ) - - const parsed = JSON.parse(jsonStr) + const jsonStr = jsonMatch[1] || jsonMatch[0] + const parsed = repairAndParseJson(jsonStr) // Validate with our schema const result = ReviewComment.ReviewOutput.safeParse(parsed) From b49dc0d878f1132a6006b1c0c926ad79e9d6b9b4 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 13:04:07 +0700 Subject: [PATCH 38/43] feat: native Gemini structured output fallback for PR review - Added generateObject fallback when JSON repair fails - Uses Gemini's responseMimeType: 'application/json' for bulletproof JSON extraction - Two-tier approach: fast repair first, native structured output as fallback - This implements Google's structured output spec per user request --- packages/opencode/src/cli/cmd/github.ts | 45 ++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 8a5877477062..4e6b36c360d4 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -28,6 +28,8 @@ 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" type GitHubAuthor = { login: string @@ -1859,13 +1861,48 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` try { const jsonStr = jsonMatch[1] || jsonMatch[0] - const parsed = repairAndParseJson(jsonStr) + 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.ReviewOutput, + 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', + }, + }, + }) - // Validate with our schema - const result = ReviewComment.ReviewOutput.safeParse(parsed) + 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:", result.error.issues) + console.warn("Invalid review output structure after all attempts:", result.error.issues) await createComment(`${response}${fallbackFooter}`) return false } From 829152c5719e1a2e3d27f5d96c9c281b6dee1bd5 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 13:09:33 +0700 Subject: [PATCH 39/43] fix: restore edit/write tool restrictions for review-only mode Previous commit 5e3212ab added these restrictions but they were accidentally removed in a later refactor. Re-adding to ensure AI cannot modify files during PR reviews. --- packages/opencode/src/cli/cmd/github.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 4e6b36c360d4..d9aaa7d9cd46 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -1311,6 +1311,12 @@ Reply with \`/oc\` followed by your answers (e.g., "/oc 1. Yes 2. Models only"), 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: [ { From 1e1dfe42b05f6cb92b5eb197c589362934d47346 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 13:13:33 +0700 Subject: [PATCH 40/43] fix: add ReviewOutputRaw schema for generateObject compatibility - Zod transforms/preprocess cannot be represented in JSON Schema - Added *Raw versions of schemas without transforms for generateObject - generateObject uses ReviewOutputRaw, then validates with full schema - Fixes: 'Transforms cannot be represented in JSON Schema' error --- packages/opencode/src/cli/cmd/github.ts | 3 +- .../opencode/src/context/review-comment.ts | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index d9aaa7d9cd46..f26faed82d54 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -1888,7 +1888,7 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` const structuredResult = await generateObject({ model: language, - schema: ReviewComment.ReviewOutput, + 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: { @@ -1898,6 +1898,7 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` }, }) + // Use the full schema with transforms for final validation parsed = structuredResult.object result = ReviewComment.ReviewOutput.safeParse(parsed) } catch (structuredError) { diff --git a/packages/opencode/src/context/review-comment.ts b/packages/opencode/src/context/review-comment.ts index a0404f7454f4..cbd2dc0a21a4 100644 --- a/packages/opencode/src/context/review-comment.ts +++ b/packages/opencode/src/context/review-comment.ts @@ -4,6 +4,20 @@ 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 */ @@ -38,6 +52,16 @@ export namespace ReviewComment { }) 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 */ @@ -66,6 +90,21 @@ export namespace ReviewComment { }) 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 */ From 36d74d596627ceddd194bd8a0ef6597dce32fe62 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 14:33:32 +0700 Subject: [PATCH 41/43] feat: robust PR review test suite and util refactor - Extracted repairAndParseJson to util/json-repair.ts with Enhanced Smart Quote logic - Extracted parsePatchForValidLines to util/git-diff.ts - Added comprehensive unit tests (github-review.test.ts) covering: - JSON repair edge cases (trailing commas, unescaped quotes/newlines, markdown blocks) - Diff parsing (hunks, context lines, deletions) - Verified all tests pass --- packages/opencode/src/cli/cmd/github.ts | 82 +----------- packages/opencode/src/util/git-diff.ts | 40 ++++++ packages/opencode/src/util/json-repair.ts | 85 +++++++++++++ .../opencode/test/cli/github-review.test.ts | 118 ++++++++++++++++++ 4 files changed, 245 insertions(+), 80 deletions(-) create mode 100644 packages/opencode/src/util/git-diff.ts create mode 100644 packages/opencode/src/util/json-repair.ts create mode 100644 packages/opencode/test/cli/github-review.test.ts diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index f26faed82d54..85d58c320137 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -30,6 +30,8 @@ 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" type GitHubAuthor = { login: string @@ -1708,34 +1710,7 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` * 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. */ - 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 - } /** * Create a pull request review with inline comments on specific lines @@ -1791,60 +1766,7 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` * 2. Hallucinated markdown blocks (```js) inside strings * 3. Trailing commas */ - function repairAndParseJson(raw: string): any { - let text = raw.trim() - - // Phase 1: Basic structural cleaning - // Normalize trailing commas in objects and arrays - 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) { - inString = !inString - repaired += char - } 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. - 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) - .replace(/\n?```/gi, '') // Remove closing blocks (raw) - 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}`) - } - } /** * Parse LLM response for structured review output and post inline comments. 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/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..d8d2afb063e6 --- /dev/null +++ b/packages/opencode/test/cli/github-review.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, test } from "bun:test" +import { repairAndParseJson } from "../../src/util/json-repair" + +import { parsePatchForValidLines } from "../../src/util/git-diff" + +describe("PR Review Utilities", () => { + 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") + }) + }) +}) From 3744a8f2a7f2af22b6c520bcaaf2ae61e27b7c2a Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 14:49:36 +0700 Subject: [PATCH 42/43] test: achieve 100% coverage for PR review utilities --- packages/opencode/test/cli/github-review.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/opencode/test/cli/github-review.test.ts b/packages/opencode/test/cli/github-review.test.ts index d8d2afb063e6..d5e469dbb9a5 100644 --- a/packages/opencode/test/cli/github-review.test.ts +++ b/packages/opencode/test/cli/github-review.test.ts @@ -114,5 +114,16 @@ const x = 1 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") + }) }) }) From 322926cc27c477eca62adb24dbd175d2edb940d3 Mon Sep 17 00:00:00 2001 From: blacksyan Date: Mon, 5 Jan 2026 15:22:46 +0700 Subject: [PATCH 43/43] refactor: extract PR review logic for 100% coverage - Extracted renderReviewMarkdown and filterCommentsByDiff to github-review-logic.ts - Achieved 100% line coverage for all extracted PR review modules - Simplified github.ts orchestration logic - Added unit tests for markdown rendering and diff-based filtering --- packages/opencode/src/cli/cmd/github.ts | 120 +--------------- .../opencode/src/util/github-review-logic.ts | 131 ++++++++++++++++++ .../opencode/test/cli/github-review.test.ts | 84 ++++++++++- 3 files changed, 221 insertions(+), 114 deletions(-) create mode 100644 packages/opencode/src/util/github-review-logic.ts diff --git a/packages/opencode/src/cli/cmd/github.ts b/packages/opencode/src/cli/cmd/github.ts index 85d58c320137..0e2a69d527b6 100644 --- a/packages/opencode/src/cli/cmd/github.ts +++ b/packages/opencode/src/cli/cmd/github.ts @@ -32,6 +32,7 @@ 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 @@ -1838,90 +1839,9 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` const reviewData = result.data - // Build comprehensive review body - 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, index) => { - 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) => { - 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) => { - reviewParts.push(`- ~~${item.item}~~ β†’ ${item.reason}`) - }) - reviewParts.push("") - } - - const fullSummary = reviewParts.join("\n") + fallbackFooter - - // Format comments for GitHub API - const formattedComments = reviewData.comments.map((comment) => - ReviewComment.formatForGitHub(comment) - ) - - if (formattedComments.length > 0) { - // Get files in the PR to validate paths - const { data: prData } = await octoRest.rest.pulls.get({ - owner, - repo, - pull_number: prNumber, - }) + 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, @@ -1929,37 +1849,11 @@ Co-authored-by: ${actor} <${actor}@users.noreply.github.com>"` pull_number: prNumber, per_page: 100, }) - // 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) - } - // Filter comments to only include valid paths AND lines in the diff - const validComments: typeof formattedComments = [] - const invalidComments: typeof formattedComments = [] - - for (const comment of formattedComments) { - const validLines = validPathsWithLines.get(comment.path) - if (!validLines) { - console.warn(`Skipping comment: path "${comment.path}" not in PR diff`) - invalidComments.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) { - validComments.push(comment) - } else { - console.warn(`Skipping comment: line ${comment.start_line || comment.line}-${comment.line} not in diff for "${comment.path}"`) - invalidComments.push(comment) - } - } + const { valid: validComments, invalid: invalidComments } = filterCommentsByDiff( + reviewData.comments, + prFiles + ) let summaryWithInvalid = fullSummary if (invalidComments.length > 0) { 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/test/cli/github-review.test.ts b/packages/opencode/test/cli/github-review.test.ts index d5e469dbb9a5..f07616aa5839 100644 --- a/packages/opencode/test/cli/github-review.test.ts +++ b/packages/opencode/test/cli/github-review.test.ts @@ -1,9 +1,91 @@ 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 @@