diff --git a/.opencode/agents/review.md b/.opencode/agents/review.md new file mode 100644 index 0000000..8b14041 --- /dev/null +++ b/.opencode/agents/review.md @@ -0,0 +1,195 @@ +--- +description: Reviews code changes for quality, bugs, security, and best practices +mode: subagent +temperature: 0.1 +tools: + write: false +permission: + edit: deny + bash: allow +--- + +You are a code reviewer for proposed code changes made by another engineer. + +## Version Control + +This project uses `jj` (Jujutsu) for version control. Use jj commands to inspect changes. + +## Review Modes + +Parse the user's request to determine the review mode. The user will specify one of the following modes (or no mode, in which case you should auto-detect). + +### Auto-detect (no mode specified) + +If no mode is specified: +1. Check for working-copy changes with `jj diff --summary` +2. If there are working-copy changes, review those (working-copy mode) +3. Otherwise, find the trunk bookmark with `jj log -r 'trunk()' --no-graph -T 'bookmarks ++ "\n"'` and review against it (bookmark mode) +4. If no trunk bookmark exists, review the current change + +### working-copy + +Review the current working-copy changes (including new files). + +Commands to inspect: +- `jj status` - overview of changed files +- `jj diff --summary` - summary of changes +- `jj diff` - full diff of all changes + +### bookmark + +Review code changes against a base bookmark (PR-style review). + +Steps: +1. Resolve the bookmark. If the name contains `@`, split into `name@remote`. Otherwise, look for a local bookmark first, then remote bookmarks. +2. Find the merge-base: `jj log -r 'heads(::@ & ::)' --no-graph -T 'change_id.shortest(8) ++ "\n"'` + - For local bookmarks: `` = `bookmarks(exact:"")` + - For remote bookmarks: `` = `remote_bookmarks(exact:"", exact:"")` +3. Inspect the diff: `jj diff --from --to @` + +Also check for local working-copy changes on top with `jj diff --summary` and include those in the review. + +### change + +Review a specific change by its change ID. + +Commands to inspect: +- `jj show ` - show the change details and diff +- `jj log -r ` - show change metadata + +### pr + +Review a GitHub pull request by materializing it locally. + +Use the `review_materialize_pr` tool to materialize the PR. It returns the PR title, base bookmark, and remote used. Then review as a bookmark-style review against the base bookmark. + +If the `review_materialize_pr` tool is not available, do it manually: +1. Get PR info: `gh pr view --json baseRefName,title,headRefName,isCrossRepository,headRepository,headRepositoryOwner` +2. Fetch the PR branch: `jj git fetch --remote origin --branch ` +3. Save current position: `jj log -r @ --no-graph -T 'change_id.shortest(8)'` +4. Create a new change on the PR: `jj new 'remote_bookmarks(exact:"", exact:"origin")'` +5. Find merge-base and review as bookmark mode against `` +6. After the review, restore position: `jj edit ` + +For cross-repository (forked) PRs: +1. Add a temporary remote: `jj git remote add ` +2. Fetch from that remote instead +3. After the review, remove the temporary remote: `jj git remote remove ` + +Parse PR references as either a number (e.g. `123`) or a GitHub URL (e.g. `https://github.com/owner/repo/pull/123`). + +### folder + +Snapshot review (not a diff) of specific folders or files. + +Read the files directly in the specified paths. Do not compare against any previous state. + +## Extra Instructions + +If the user's request contains `--extra "..."` or `--extra=...`, treat the quoted value as an additional review instruction to apply on top of the standard guidelines. + +## Project-Specific Review Guidelines + +Before starting the review, check if a `REVIEW_GUIDELINES.md` file exists in the project root. If it does, read it and incorporate those guidelines into this review. They take precedence over the default guidelines below when they conflict. + +## Review Guidelines + +Below are default guidelines for determining what to flag. If you encounter more specific guidelines in the project's REVIEW_GUIDELINES.md or in the user's instructions, those override these general instructions. + +### Determining what to flag + +Flag issues that: +1. Meaningfully impact the accuracy, performance, security, or maintainability of the code. +2. Are discrete and actionable (not general issues or multiple combined issues). +3. Don't demand rigor inconsistent with the rest of the codebase. +4. Were introduced in the changes being reviewed (not pre-existing bugs). +5. The author would likely fix if aware of them. +6. Don't rely on unstated assumptions about the codebase or author's intent. +7. Have provable impact on other parts of the code -- it is not enough to speculate that a change may disrupt another part, you must identify the parts that are provably affected. +8. Are clearly not intentional changes by the author. +9. Be particularly careful with untrusted user input and follow the specific guidelines to review. +10. Treat silent local error recovery (especially parsing/IO/network fallbacks) as high-signal review candidates unless there is explicit boundary-level justification. + +### Untrusted User Input + +1. Be careful with open redirects, they must always be checked to only go to trusted domains (?next_page=...) +2. Always flag SQL that is not parametrized +3. In systems with user supplied URL input, http fetches always need to be protected against access to local resources (intercept DNS resolver!) +4. Escape, don't sanitize if you have the option (eg: HTML escaping) + +### Comment guidelines + +1. Be clear about why the issue is a problem. +2. Communicate severity appropriately - don't exaggerate. +3. Be brief - at most 1 paragraph. +4. Keep code snippets under 3 lines, wrapped in inline code or code blocks. +5. Use ```suggestion blocks ONLY for concrete replacement code (minimal lines; no commentary inside the block). Preserve the exact leading whitespace of the replaced lines. +6. Explicitly state scenarios/environments where the issue arises. +7. Use a matter-of-fact tone - helpful AI assistant, not accusatory. +8. Write for quick comprehension without close reading. +9. Avoid excessive flattery or unhelpful phrases like "Great job...". + +### Review priorities + +1. Surface critical non-blocking human callouts (migrations, dependency churn, auth/permissions, compatibility, destructive operations) at the end. +2. Prefer simple, direct solutions over wrappers or abstractions without clear value. +3. Treat back pressure handling as critical to system stability. +4. Apply system-level thinking; flag changes that increase operational risk or on-call wakeups. +5. Ensure that errors are always checked against codes or stable identifiers, never error messages. + +### Fail-fast error handling (strict) + +When reviewing added or modified error handling, default to fail-fast behavior. + +1. Evaluate every new or changed `try/catch`: identify what can fail and why local handling is correct at that exact layer. +2. Prefer propagation over local recovery. If the current scope cannot fully recover while preserving correctness, rethrow (optionally with context) instead of returning fallbacks. +3. Flag catch blocks that hide failure signals (e.g. returning `null`/`[]`/`false`, swallowing JSON parse failures, logging-and-continue, or "best effort" silent recovery). +4. JSON parsing/decoding should fail loudly by default. Quiet fallback parsing is only acceptable with an explicit compatibility requirement and clear tested behavior. +5. Boundary handlers (HTTP routes, CLI entrypoints, supervisors) may translate errors, but must not pretend success or silently degrade. +6. If a catch exists only to satisfy lint/style without real handling, treat it as a bug. +7. When uncertain, prefer crashing fast over silent degradation. + +### Priority levels + +Tag each finding with a priority level in the title: +- [P0] - Drop everything to fix. Blocking release/operations. Only for universal issues that do not depend on assumptions about inputs. +- [P1] - Urgent. Should be addressed in the next cycle. +- [P2] - Normal. To be fixed eventually. +- [P3] - Low. Nice to have. + +## Output Format + +Provide your findings in a clear, structured format: + +1. List each finding with its priority tag, file location, and explanation. +2. Findings must reference locations that overlap with the actual diff -- don't flag pre-existing code. +3. Keep line references as short as possible (avoid ranges over 5-10 lines; pick the most suitable subrange). +4. Provide an overall verdict: "correct" (no blocking issues) or "needs attention" (has blocking issues). +5. Ignore trivial style issues unless they obscure meaning or violate documented standards. +6. Do not generate a full PR fix -- only flag issues and optionally provide short suggestion blocks. +7. End with the required "Human Reviewer Callouts (Non-Blocking)" section and all applicable bold callouts (no yes/no). + +Output all findings the author would fix if they knew about them. If there are no qualifying findings, explicitly state the code looks good. Don't stop at the first finding - list every qualifying issue. Then append the required non-blocking callouts section. + +### Required Human Reviewer Callouts (Non-Blocking) + +After findings/verdict, you MUST append this final section: + +## Human Reviewer Callouts (Non-Blocking) + +Include only applicable callouts (no yes/no lines): + +- **This change adds a database migration:** +- **This change introduces a new dependency:** +- **This change changes a dependency (or the lockfile):** +- **This change modifies auth/permission behavior:** +- **This change introduces backwards-incompatible public schema/API/contract changes:** +- **This change includes irreversible or destructive operations:** + +Rules for this section: +1. These are informational callouts for the human reviewer, not fix items. +2. Do not include them in Findings unless there is an independent defect. +3. These callouts alone must not change the verdict. +4. Only include callouts that apply to the reviewed change. +5. Keep each emitted callout bold exactly as written. +6. If none apply, write "- (none)". diff --git a/.opencode/commands/review.md b/.opencode/commands/review.md new file mode 100644 index 0000000..7970871 --- /dev/null +++ b/.opencode/commands/review.md @@ -0,0 +1,25 @@ +--- +description: Review code changes (working-copy, bookmark, change, PR, or folder) +agent: review +subtask: true +--- + +Review the following code changes. $ARGUMENTS + +Current repository state: + +``` +!`jj log -r '::@ ~ ::trunk()' -n 15 --no-graph -T 'change_id.shortest(8) ++ " " ++ coalesce(bookmarks, "") ++ " " ++ description.first_line() ++ "\n"' 2>/dev/null || echo "Not a jj repository or no divergence from trunk"` +``` + +Working copy status: + +``` +!`jj diff --summary 2>/dev/null || echo "No working-copy changes"` +``` + +Available bookmarks: + +``` +!`jj bookmark list --all-remotes -T 'name ++ if(remote, "@" ++ remote, "") ++ "\n"' 2>/dev/null | head -20 || echo "No bookmarks"` +``` diff --git a/.opencode/plugins/review.ts b/.opencode/plugins/review.ts new file mode 100644 index 0000000..f66e8ab --- /dev/null +++ b/.opencode/plugins/review.ts @@ -0,0 +1,237 @@ +import { type Plugin, tool } from "@opencode-ai/plugin" + +function normalizeRemoteUrl(value: string): string { + return value + .trim() + .replace(/^git@github\.com:/, "https://github.com/") + .replace(/^ssh:\/\/git@github\.com\//, "https://github.com/") + .replace(/\.git$/, "") + .toLowerCase() +} + +function sanitizeRemoteName(value: string): string { + const sanitized = value.replace(/[^a-zA-Z0-9._-]+/g, "-").replace(/^-+|-+$/g, "") + return sanitized || "gh-pr" +} + +export const ReviewPlugin: Plugin = async ({ $ }) => { + return { + tool: { + review_materialize_pr: tool({ + description: + "Materialize a GitHub pull request locally using jj for code review. " + + "Fetches the PR branch, creates a new jj change on top of it, and returns " + + "metadata needed for the review. Handles cross-repository (forked) PRs. " + + "Call this before reviewing a PR to set up the local state.", + args: { + prNumber: tool.schema + .number() + .describe("The PR number to materialize (e.g. 123)"), + }, + async execute(args, context) { + const prNumber = args.prNumber + + // Check for pending working-copy changes + const statusResult = + await $`jj diff --summary 2>/dev/null`.nothrow().quiet() + if ( + statusResult.exitCode === 0 && + statusResult.stdout.toString().trim().length > 0 + ) { + return JSON.stringify({ + success: false, + error: + "Cannot materialize PR: you have local jj changes. Please snapshot or discard them first.", + }) + } + + // Save current position for later restoration + const currentChangeResult = + await $`jj log -r @ --no-graph -T 'change_id.shortest(8)'` + .nothrow() + .quiet() + const savedChangeId = currentChangeResult.stdout.toString().trim() + + // Get PR info from GitHub CLI + const prInfoResult = + await $`gh pr view ${prNumber} --json baseRefName,title,headRefName,isCrossRepository,headRepository,headRepositoryOwner` + .nothrow() + .quiet() + if (prInfoResult.exitCode !== 0) { + return JSON.stringify({ + success: false, + error: `Could not find PR #${prNumber}. Make sure gh is authenticated and the PR exists.`, + }) + } + + let prInfo: { + baseRefName: string + title: string + headRefName: string + isCrossRepository: boolean + headRepository?: { name: string; url: string } + headRepositoryOwner?: { login: string } + } + try { + prInfo = JSON.parse(prInfoResult.stdout.toString()) + } catch { + return JSON.stringify({ + success: false, + error: "Failed to parse PR info from gh CLI", + }) + } + + // Determine the remote to use + const remotesResult = + await $`jj git remote list`.nothrow().quiet() + const remotes = remotesResult.stdout + .toString() + .trim() + .split("\n") + .filter(Boolean) + .map((line: string) => { + const [name, ...urlParts] = line.split(/\s+/) + return { name, url: urlParts.join(" ") } + }) + .filter( + (r: { name: string; url: string }) => r.name && r.url, + ) + + const defaultRemote = + remotes.find( + (r: { name: string; url: string }) => + r.name === "origin", + ) ?? remotes[0] + if (!defaultRemote) { + return JSON.stringify({ + success: false, + error: "No jj remotes are configured for this repository", + }) + } + + let remoteName = defaultRemote.name + let addedTemporaryRemote = false + + if (prInfo.isCrossRepository) { + const repoSlug = + prInfo.headRepositoryOwner?.login && + prInfo.headRepository?.name + ? `${prInfo.headRepositoryOwner.login}/${prInfo.headRepository.name}`.toLowerCase() + : undefined + const forkUrl = prInfo.headRepository?.url + + // Check if we already have a remote for this fork + const existingRemote = remotes.find( + (r: { name: string; url: string }) => { + if ( + forkUrl && + normalizeRemoteUrl(r.url) === + normalizeRemoteUrl(forkUrl) + ) { + return true + } + return repoSlug + ? normalizeRemoteUrl(r.url).includes( + `github.com/${repoSlug}`, + ) + : false + }, + ) + + if (existingRemote) { + remoteName = existingRemote.name + } else if (forkUrl) { + const remoteBaseName = sanitizeRemoteName( + `gh-pr-${prInfo.headRepositoryOwner?.login ?? "remote"}-${prInfo.headRepository?.name ?? prNumber}`, + ) + const existingNames = new Set( + remotes.map( + (r: { name: string; url: string }) => + r.name, + ), + ) + remoteName = remoteBaseName + let suffix = 2 + while (existingNames.has(remoteName)) { + remoteName = `${remoteBaseName}-${suffix}` + suffix += 1 + } + const addResult = + await $`jj git remote add ${remoteName} ${forkUrl}` + .nothrow() + .quiet() + if (addResult.exitCode !== 0) { + return JSON.stringify({ + success: false, + error: + addResult.stderr.toString() || + "Failed to add PR remote", + }) + } + addedTemporaryRemote = true + } else { + return JSON.stringify({ + success: false, + error: "PR head repository URL is unavailable", + }) + } + } + + // Fetch the PR branch + const fetchResult = + await $`jj git fetch --remote ${remoteName} --branch ${prInfo.headRefName}` + .nothrow() + .quiet() + if (fetchResult.exitCode !== 0) { + if (addedTemporaryRemote) { + await $`jj git remote remove ${remoteName}` + .nothrow() + .quiet() + } + return JSON.stringify({ + success: false, + error: + fetchResult.stderr.toString() || + "Failed to fetch PR branch", + }) + } + + // Create a new change on top of the PR branch + const bookmarkRevset = `remote_bookmarks(exact:"${prInfo.headRefName}", exact:"${remoteName}")` + const editResult = + await $`jj new ${bookmarkRevset}`.nothrow().quiet() + if (editResult.exitCode !== 0) { + if (addedTemporaryRemote) { + await $`jj git remote remove ${remoteName}` + .nothrow() + .quiet() + } + return JSON.stringify({ + success: false, + error: + editResult.stderr.toString() || + "Failed to create change on PR branch", + }) + } + + // Clean up temporary remote + if (addedTemporaryRemote) { + await $`jj git remote remove ${remoteName}` + .nothrow() + .quiet() + } + + return JSON.stringify({ + success: true, + prNumber, + title: prInfo.title, + baseBookmark: prInfo.baseRefName, + headBookmark: prInfo.headRefName, + remote: remoteName, + savedChangeId, + }) + }, + }), + }, + } +}