diff --git a/.factory/droids/file-group-reviewer.md b/.factory/droids/file-group-reviewer.md index 7ba489b..7da7902 100644 --- a/.factory/droids/file-group-reviewer.md +++ b/.factory/droids/file-group-reviewer.md @@ -10,6 +10,7 @@ You are a senior staff software engineer and expert code reviewer. Your task: Review the assigned files from the PR and generate a JSON array of **high-confidence, actionable** review comments that pinpoint genuine issues. + - You are currently checked out to the PR branch. - Review ALL files assigned to you thoroughly. - Focus on: functional correctness, syntax errors, logic bugs, broken dependencies/contracts/tests, security issues, and performance problems. @@ -24,7 +25,7 @@ Your task: Review the assigned files from the PR and generate a JSON array of ** - Type-assumption bugs (e.g., numeric ops on datetime/strings, ordering key type mismatches) - Offset/cursor/pagination semantic mismatches (off-by-one, prev/next behavior, commit semantics) - Only flag issues you are confident about—avoid speculative or stylistic nitpicks. - + 1. Read each assigned file in full to understand the context @@ -57,6 +58,7 @@ Return your findings as a JSON array (no wrapper object, just the array): If no issues found, return an empty array: `[]` Field definitions: + - `path`: Relative file path (must match exactly as provided in your assignment) - `body`: Comment text starting with priority tag [P0|P1|P2], then title, then 1 paragraph explanation - P0: Critical bugs (crashes, security vulnerabilities, data loss) @@ -65,7 +67,7 @@ Field definitions: - `line`: Target line number (single-line) or end line number (multi-line). Must be ≥ 0. - `startLine`: `null` for single-line comments, or start line number for multi-line comments - `side`: "RIGHT" for new/modified code (default), "LEFT" only for commenting on removed code - + - Output ONLY the JSON array—no additional commentary or markdown formatting around it. diff --git a/.github/workflows/droid-review.yml b/.github/workflows/droid-review.yml index 44a8a6f..f2e3af7 100644 --- a/.github/workflows/droid-review.yml +++ b/.github/workflows/droid-review.yml @@ -16,7 +16,6 @@ jobs: outputs: comment_id: ${{ steps.prepare.outputs.comment_id }} run_code_review: ${{ steps.prepare.outputs.run_code_review }} - run_security_review: ${{ steps.prepare.outputs.run_security_review }} steps: - name: Checkout repository uses: actions/checkout@v5 @@ -25,7 +24,7 @@ jobs: - name: Prepare id: prepare - uses: Factory-AI/droid-action/prepare@v2 + uses: Factory-AI/droid-action/prepare@vn/bug-fixes with: factory_api_key: ${{ secrets.FACTORY_API_KEY }} automatic_review: true @@ -40,14 +39,16 @@ jobs: issues: write id-token: write actions: read + env: + ACTIONS_STEP_DEBUG: true steps: - name: Checkout repository uses: actions/checkout@v5 with: - fetch-depth: 1 + fetch-depth: 0 - name: Run Code Review - uses: Factory-AI/droid-action/review@v2 + uses: Factory-AI/droid-action/review@vn/bug-fixes with: factory_api_key: ${{ secrets.FACTORY_API_KEY }} tracking_comment_id: ${{ needs.prepare.outputs.comment_id }} @@ -60,44 +61,22 @@ jobs: path: ${{ runner.temp }}/code-review-results.json if-no-files-found: ignore - security-review: - needs: prepare - if: needs.prepare.outputs.run_security_review == 'true' - runs-on: ubuntu-latest - permissions: - contents: write - pull-requests: write - issues: write - id-token: write - actions: read - steps: - - name: Checkout repository - uses: actions/checkout@v5 - with: - fetch-depth: 1 - - - name: Run Security Review - uses: Factory-AI/droid-action/security@v2 - with: - factory_api_key: ${{ secrets.FACTORY_API_KEY }} - tracking_comment_id: ${{ needs.prepare.outputs.comment_id }} - security_severity_threshold: medium - output_file: ${{ runner.temp }}/security-results.json - - - name: Upload Results + - name: Upload debug artifacts + if: always() uses: actions/upload-artifact@v4 with: - name: security-results - path: ${{ runner.temp }}/security-results.json + name: droid-review-debug-${{ github.run_id }} + path: | + ~/.factory/** + ${{ runner.temp }}/droid-prompts/** if-no-files-found: ignore + retention-days: 7 combine: - needs: [prepare, code-review, security-review] - # Run combine when EITHER code review OR security review was executed + needs: [prepare, code-review] if: | always() && - (needs.prepare.outputs.run_code_review == 'true' || - needs.prepare.outputs.run_security_review == 'true') + needs.prepare.outputs.run_code_review == 'true' runs-on: ubuntu-latest permissions: contents: write @@ -118,19 +97,10 @@ jobs: path: ${{ runner.temp }} continue-on-error: true - - name: Download Security Results - uses: actions/download-artifact@v4 - with: - name: security-results - path: ${{ runner.temp }} - continue-on-error: true - - name: Combine Results - uses: Factory-AI/droid-action/combine@v2 + uses: Factory-AI/droid-action/combine@vn/bug-fixes with: factory_api_key: ${{ secrets.FACTORY_API_KEY }} tracking_comment_id: ${{ needs.prepare.outputs.comment_id }} code_review_results: ${{ runner.temp }}/code-review-results.json - security_results: ${{ runner.temp }}/security-results.json code_review_status: ${{ needs.code-review.result }} - security_review_status: ${{ needs.security-review.result }} diff --git a/.github/workflows/droid.yml b/.github/workflows/droid.yml index 36c7be8..7a69275 100644 --- a/.github/workflows/droid.yml +++ b/.github/workflows/droid.yml @@ -27,6 +27,8 @@ jobs: issues: write id-token: write actions: read + env: + ACTIONS_STEP_DEBUG: true steps: - name: Checkout repository uses: actions/checkout@v5 @@ -34,6 +36,6 @@ jobs: fetch-depth: 1 - name: Run Droid Exec - uses: Factory-AI/droid-action@v2 + uses: Factory-AI/droid-action@vn/bug-fixes with: factory_api_key: ${{ secrets.FACTORY_API_KEY }} diff --git a/action.yml b/action.yml index 457b734..d31c2bc 100644 --- a/action.yml +++ b/action.yml @@ -98,7 +98,7 @@ inputs: reasoning_effort: description: "Override reasoning effort for review flows (passed to Droid Exec as --reasoning-effort). If empty and review_model is also empty, the action defaults internally to gpt-5.2 at high reasoning." required: false - default: "" + default: "high" review_use_validator: description: "Enable two-pass review: generate candidate comments to JSON, then validate and post only approved ones." required: false @@ -373,7 +373,6 @@ runs: DETAILED_PERMISSION_MESSAGES: "1" FACTORY_API_KEY: ${{ inputs.factory_api_key }} - - name: Update comment with job link if: steps.prepare.outputs.contains_trigger == 'true' && steps.prepare.outputs.droid_comment_id && always() shell: bash @@ -402,10 +401,7 @@ runs: with: name: droid-review-debug-${{ github.run_id }} path: | - ~/.factory/logs/droid-log-single.log - ~/.factory/logs/console.log - ~/.factory/sessions/* - ~/.factory/droids/* + ~/.factory/** ${{ runner.temp }}/droid-prompts/** if-no-files-found: ignore retention-days: 7 diff --git a/review/action.yml b/review/action.yml index 257ad9c..d4a6bad 100644 --- a/review/action.yml +++ b/review/action.yml @@ -15,11 +15,27 @@ inputs: review_model: description: "Model to use for review" required: false - default: "" + default: "gpt-5.2" + reasoning_effort: + description: "Reasoning effort for review (passed to Droid Exec as --reasoning-effort)" + required: false + default: "high" output_file: description: "Path to write review results JSON" required: false default: "" + review_use_validator: + description: "Enable two-pass review: generate candidate comments, then validate and post only approved ones." + required: false + default: "true" + review_candidates_path: + description: "Path to write review candidates JSON (pass 1)." + required: false + default: "${{ runner.temp }}/droid-prompts/review_candidates.json" + review_validated_path: + description: "Path to write review validated JSON (pass 2)." + required: false + default: "${{ runner.temp }}/droid-prompts/review_validated.json" outputs: conclusion: @@ -68,6 +84,19 @@ runs: DROID_COMMENT_ID: ${{ inputs.tracking_comment_id }} REVIEW_MODEL: ${{ inputs.review_model }} REVIEW_TYPE: "code" + REVIEW_USE_VALIDATOR: ${{ inputs.review_use_validator }} + REVIEW_CANDIDATES_PATH: ${{ inputs.review_candidates_path }} + REASONING_EFFORT: ${{ inputs.reasoning_effort }} + DROID_OUTPUT_FILE: ${{ inputs.output_file }} + + - name: Setup Custom Droids + shell: bash + run: | + mkdir -p ~/.factory/droids + if [ -d "${{ github.action_path }}/../.factory/droids" ]; then + cp -r ${{ github.action_path }}/../.factory/droids/* ~/.factory/droids/ + echo "Copied custom droids to ~/.factory/droids/" + fi - name: Run Code Review id: review @@ -81,3 +110,31 @@ runs: FACTORY_API_KEY: ${{ inputs.factory_api_key }} GITHUB_TOKEN: ${{ steps.token.outputs.github_token }} DROID_OUTPUT_FILE: ${{ inputs.output_file }} + + - name: Prepare Validator + id: prepare_validator + if: steps.prompt.outputs.review_use_validator == 'true' + shell: bash + run: | + bun run ${{ github.action_path }}/../src/entrypoints/prepare-validator.ts + env: + GITHUB_TOKEN: ${{ steps.token.outputs.github_token }} + REVIEW_USE_VALIDATOR: ${{ inputs.review_use_validator }} + REVIEW_VALIDATED_PATH: ${{ inputs.review_validated_path }} + REVIEW_CANDIDATES_PATH: ${{ inputs.review_candidates_path }} + REVIEW_MODEL: ${{ inputs.review_model }} + REASONING_EFFORT: ${{ inputs.reasoning_effort }} + DROID_COMMENT_ID: ${{ inputs.tracking_comment_id }} + + - name: Run Validator + id: validator + if: steps.prompt.outputs.review_use_validator == 'true' + shell: bash + run: | + bun run ${{ github.action_path }}/../base-action/src/index.ts + env: + INPUT_PROMPT_FILE: ${{ runner.temp }}/droid-prompts/droid-prompt.txt + INPUT_DROID_ARGS: ${{ steps.prepare_validator.outputs.droid_args }} + INPUT_MCP_TOOLS: ${{ steps.prepare_validator.outputs.mcp_tools }} + FACTORY_API_KEY: ${{ inputs.factory_api_key }} + GITHUB_TOKEN: ${{ steps.token.outputs.github_token }} diff --git a/security/action.yml b/security/action.yml index 84655d4..45e1c4e 100644 --- a/security/action.yml +++ b/security/action.yml @@ -115,6 +115,7 @@ runs: SECURITY_BLOCK_ON_CRITICAL: ${{ inputs.security_block_on_critical }} SECURITY_BLOCK_ON_HIGH: ${{ inputs.security_block_on_high }} REVIEW_TYPE: "security" + DROID_OUTPUT_FILE: ${{ inputs.output_file }} - name: Run Security Review id: review diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index 0cd8d22..415013f 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -303,6 +303,7 @@ export type PromptCreationOptions = { disallowedTools?: string[]; includeActionsTools?: boolean; reviewArtifacts?: ReviewArtifacts; + outputFilePath?: string; }; export async function createPrompt({ @@ -316,6 +317,7 @@ export async function createPrompt({ disallowedTools = [], includeActionsTools = false, reviewArtifacts, + outputFilePath, }: PromptCreationOptions) { try { const droidCommentId = commentId?.toString(); @@ -328,6 +330,10 @@ export async function createPrompt({ reviewArtifacts, ); + if (outputFilePath) { + preparedContext.outputFilePath = outputFilePath; + } + await mkdir(`${process.env.RUNNER_TEMP || "/tmp"}/droid-prompts`, { recursive: true, }); diff --git a/src/create-prompt/templates/review-prompt.ts b/src/create-prompt/templates/review-prompt.ts index 2d53eb8..6462eb0 100644 --- a/src/create-prompt/templates/review-prompt.ts +++ b/src/create-prompt/templates/review-prompt.ts @@ -353,5 +353,37 @@ In the submitted review body: * State whether the changes are correct or incorrect * Provide a 1-3 sentence overall assessment -`; +${ + context.outputFilePath + ? ` +--- + +## Output File (REQUIRED) + +After completing your review, you MUST write your findings to \`${context.outputFilePath}\` as a JSON file with this structure: + +\`\`\`json +{ + "type": "code-review", + "findings": [ + { + "id": "CR-001", + "severity": "P0|P1|P2|P3", + "file": "path/to/file.ts", + "line": 55, + "side": "RIGHT", + "description": "Brief description of the issue", + "suggestion": "Optional suggested fix" + } + ], + "summary": "Brief overall summary of the review" +} +\`\`\` + +If no issues were found, write: \`{"type": "code-review", "findings": [], "summary": "No issues found"}\` + +This file is required for the combine step to aggregate results. +` + : "" +}`; } diff --git a/src/create-prompt/templates/security-review-prompt.ts b/src/create-prompt/templates/security-review-prompt.ts index f71e3dc..7e4425f 100644 --- a/src/create-prompt/templates/security-review-prompt.ts +++ b/src/create-prompt/templates/security-review-prompt.ts @@ -109,7 +109,7 @@ You have access to these Factory security skills (installed in ~/.factory/skills IMPORTANT: Do NOT post inline comments directly. Instead, write findings to a JSON file. The finalize step will post all inline comments to avoid overlapping with code review comments. -1. Write findings to \`security-review-results.json\` with this structure: +1. Write findings to \`${context.outputFilePath || "security-review-results.json"}\` with this structure: \`\`\`json { "type": "security", diff --git a/src/create-prompt/types.ts b/src/create-prompt/types.ts index edcfdd0..cf110c3 100644 --- a/src/create-prompt/types.ts +++ b/src/create-prompt/types.ts @@ -117,4 +117,5 @@ export type PreparedContext = CommonFields & { headRefOid: string; }; reviewArtifacts?: ReviewArtifacts; + outputFilePath?: string; }; diff --git a/src/entrypoints/generate-combine-prompt.ts b/src/entrypoints/generate-combine-prompt.ts index bcafe32..232e135 100644 --- a/src/entrypoints/generate-combine-prompt.ts +++ b/src/entrypoints/generate-combine-prompt.ts @@ -86,11 +86,7 @@ async function run() { }); const droidArgParts: string[] = []; - // Only include built-in tools in --enabled-tools - const builtInTools = allowedTools.filter((t) => !t.includes("___")); - if (builtInTools.length > 0) { - droidArgParts.push(`--enabled-tools "${builtInTools.join(",")}"`); - } + droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); if (normalizedUserArgs) { droidArgParts.push(normalizedUserArgs); diff --git a/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 0f74e89..03b989f 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -5,12 +5,15 @@ */ import * as core from "@actions/core"; +import { execSync } from "child_process"; import { createOctokit } from "../github/api/client"; import { parseGitHubContext, isEntityContext } from "../github/context"; import { fetchPRBranchData } from "../github/data/pr-fetcher"; +import { computeReviewArtifacts } from "../github/data/review-artifacts"; import { createPrompt } from "../create-prompt"; import { prepareMcpTools } from "../mcp/install-mcp-server"; import { generateReviewPrompt } from "../create-prompt/templates/review-prompt"; +import { generateReviewCandidatesPrompt } from "../create-prompt/templates/review-candidates-prompt"; import { generateSecurityReviewPrompt } from "../create-prompt/templates/security-review-prompt"; import { normalizeDroidArgs, parseAllowedTools } from "../utils/parse-tools"; @@ -18,6 +21,9 @@ async function run() { try { const githubToken = process.env.GITHUB_TOKEN!; const reviewType = process.env.REVIEW_TYPE || "code"; + const reviewUseValidator = + reviewType === "code" && + (process.env.REVIEW_USE_VALIDATOR ?? "true").trim() !== "false"; const commentId = parseInt(process.env.DROID_COMMENT_ID || "0"); if (!commentId) { @@ -47,11 +53,55 @@ async function run() { currentBranch: prData.headRefName, }; - // Select prompt generator based on review type + // Pre-compute review artifacts (diff, existing comments, PR description) + // so the Droid can read them directly instead of fetching via gh CLI + const tempDir = process.env.RUNNER_TEMP || "/tmp"; + + // Checkout the PR branch before computing diff to ensure HEAD points + // to the PR head commit, not the merge commit + console.log( + `Checking out PR #${context.entityNumber} branch for diff computation...`, + ); + try { + execSync("git reset --hard HEAD", { encoding: "utf8", stdio: "pipe" }); + execSync(`gh pr checkout ${context.entityNumber}`, { + encoding: "utf8", + stdio: "pipe", + env: { ...process.env, GH_TOKEN: githubToken }, + }); + console.log( + `Successfully checked out PR branch: ${execSync("git rev-parse --abbrev-ref HEAD", { encoding: "utf8" }).trim()}`, + ); + } catch (e) { + console.error(`Failed to checkout PR branch: ${e}`); + throw new Error( + `Failed to checkout PR #${context.entityNumber} branch for review`, + ); + } + + const reviewArtifacts = await computeReviewArtifacts({ + baseRef: prData.baseRefName, + tempDir, + octokit, + owner: context.repository.owner, + repo: context.repository.repo, + prNumber: context.entityNumber, + title: prData.title, + body: prData.body, + githubToken, + }); + + // Select prompt generator based on review type and validator mode const generatePrompt = reviewType === "security" ? generateSecurityReviewPrompt - : generateReviewPrompt; + : reviewUseValidator + ? generateReviewCandidatesPrompt + : generateReviewPrompt; + + // Pass the output file path so the prompt can instruct the Droid + // to write structured findings for the combine step + const outputFilePath = process.env.DROID_OUTPUT_FILE || undefined; await createPrompt({ githubContext: context, @@ -62,6 +112,8 @@ async function run() { headRefOid: prData.headRefOid, }, generatePrompt, + reviewArtifacts, + outputFilePath, }); // Set run type @@ -75,22 +127,47 @@ async function run() { (tool) => tool.startsWith("github_") && tool.includes("___"), ); - // Base tools for analysis only - NO inline comment tools - // Inline comments will be posted by the finalize step to avoid overlaps + // Base tools for analysis const baseTools = [ "Read", "Grep", "Glob", "LS", "Execute", + "Edit", + "Create", + "ApplyPatch", "github_comment___update_droid_comment", ]; - // Review tools for reading existing comments only - const reviewTools = ["github_pr___list_review_comments"]; + // Task tool is needed for parallel subagent reviews in candidate generation phase. + // FetchUrl is needed to fetch linked tickets from the PR description. + const candidateGenerationTools = reviewUseValidator + ? ["Task", "FetchUrl"] + : []; + + // When validator is enabled, the candidate generation phase should NOT + // have access to PR mutation tools. When disabled, allow them. + const reviewTools = reviewUseValidator + ? [] + : ["github_pr___list_review_comments"]; + + const safeUserAllowedMCPTools = reviewUseValidator + ? userAllowedMCPTools.filter( + (tool) => + tool === "github_comment___update_droid_comment" || + (!tool.startsWith("github_pr___") && + tool !== "github_inline_comment___create_inline_comment"), + ) + : userAllowedMCPTools; const allowedTools = Array.from( - new Set([...baseTools, ...reviewTools, ...userAllowedMCPTools]), + new Set([ + ...baseTools, + ...candidateGenerationTools, + ...reviewTools, + ...safeUserAllowedMCPTools, + ]), ); const mcpTools = await prepareMcpTools({ @@ -104,21 +181,19 @@ async function run() { }); const droidArgParts: string[] = []; - // Only include built-in tools in --enabled-tools - // MCP tools are discovered dynamically from registered servers - const builtInTools = allowedTools.filter((t) => !t.includes("___")); - if (builtInTools.length > 0) { - droidArgParts.push(`--enabled-tools "${builtInTools.join(",")}"`); - } + droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); - // Add model override if specified - const model = + const reviewModel = reviewType === "security" ? process.env.SECURITY_MODEL?.trim() || process.env.REVIEW_MODEL?.trim() : process.env.REVIEW_MODEL?.trim(); + const reasoningEffort = process.env.REASONING_EFFORT?.trim(); - if (model) { - droidArgParts.push(`--model "${model}"`); + if (reviewModel) { + droidArgParts.push(`--model "${reviewModel}"`); + } + if (reasoningEffort) { + droidArgParts.push(`--reasoning-effort "${reasoningEffort}"`); } if (normalizedUserArgs) { @@ -128,8 +203,11 @@ async function run() { // Output for next step - use core.setOutput which handles GITHUB_OUTPUT internally core.setOutput("droid_args", droidArgParts.join(" ").trim()); core.setOutput("mcp_tools", mcpTools); + core.setOutput("review_use_validator", reviewUseValidator.toString()); - console.log(`Generated ${reviewType} review prompt`); + console.log( + `Generated ${reviewType} review prompt (validator=${reviewUseValidator})`, + ); } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); core.setFailed(`Generate prompt failed: ${errorMessage}`); diff --git a/src/entrypoints/prepare-validator.ts b/src/entrypoints/prepare-validator.ts index 52c18f7..b27d3a7 100644 --- a/src/entrypoints/prepare-validator.ts +++ b/src/entrypoints/prepare-validator.ts @@ -16,7 +16,9 @@ async function run() { // This entrypoint only makes sense when the workflow input is enabled. if ((process.env.REVIEW_USE_VALIDATOR ?? "true").trim() === "false") { - throw new Error("reviewUseValidator must be true to run prepare-validator"); + throw new Error( + "reviewUseValidator must be true to run prepare-validator", + ); } const githubToken = await setupGitHubToken(); diff --git a/src/entrypoints/update-comment-link.ts b/src/entrypoints/update-comment-link.ts index ad57b16..b066a8a 100644 --- a/src/entrypoints/update-comment-link.ts +++ b/src/entrypoints/update-comment-link.ts @@ -44,7 +44,8 @@ async function run() { owner, repo, commentId, - isPullRequestReviewCommentEvent: isPullRequestReviewCommentEvent(context), + isPullRequestReviewCommentEvent: + isPullRequestReviewCommentEvent(context), }); comment = result.comment; isPRReviewComment = result.isPRReviewComment; diff --git a/src/github/data/review-artifacts.ts b/src/github/data/review-artifacts.ts new file mode 100644 index 0000000..c7d7a4f --- /dev/null +++ b/src/github/data/review-artifacts.ts @@ -0,0 +1,169 @@ +import { execSync } from "child_process"; +import { writeFile, mkdir } from "fs/promises"; +import type { Octokits } from "../api/client"; +import type { ReviewArtifacts } from "../../create-prompt/types"; + +const DIFF_MAX_BUFFER = 50 * 1024 * 1024; // 50MB buffer for large diffs + +/** + * Compute the PR diff and store it on disk. + * + * Tries git merge-base first (requires sufficient history). When that + * fails (e.g. shallow clone without unshallow support) it falls back + * to `gh pr diff` which always works. + */ +export async function computeAndStoreDiff( + baseRef: string, + tempDir: string, + options?: { githubToken?: string; prNumber?: number }, +): Promise { + const promptsDir = `${tempDir}/droid-prompts`; + await mkdir(promptsDir, { recursive: true }); + + let diff: string; + try { + // Unshallow the repo if it's a shallow clone (needed for merge-base) + try { + execSync("git rev-parse --is-shallow-repository", { + encoding: "utf8", + stdio: "pipe", + }).trim() === "true" && + execSync("git fetch --unshallow", { + encoding: "utf8", + stdio: "pipe", + }); + console.log("Unshallowed repository"); + } catch { + console.log("Repository already has full history"); + } + + // Fetch the base branch (it may not exist locally yet) + try { + execSync(`git fetch origin ${baseRef}:refs/remotes/origin/${baseRef}`, { + encoding: "utf8", + stdio: "pipe", + }); + console.log(`Fetched base branch: ${baseRef}`); + } catch { + console.log(`Base branch fetch skipped (may already exist): ${baseRef}`); + } + + const mergeBase = execSync( + `git merge-base HEAD refs/remotes/origin/${baseRef}`, + { encoding: "utf8" }, + ).trim(); + + diff = execSync(`git --no-pager diff ${mergeBase}..HEAD`, { + encoding: "utf8", + maxBuffer: DIFF_MAX_BUFFER, + }); + } catch { + // Fallback: use gh CLI to get the diff (works even with shallow clones) + if (options?.githubToken && options?.prNumber) { + console.log( + "Git merge-base failed, falling back to gh pr diff for PR diff", + ); + diff = execSync(`gh pr diff ${options.prNumber}`, { + encoding: "utf8", + maxBuffer: DIFF_MAX_BUFFER, + env: { ...process.env, GH_TOKEN: options.githubToken }, + }); + } else { + throw new Error( + "Git merge-base failed and no fallback credentials provided", + ); + } + } + + const diffPath = `${promptsDir}/pr.diff`; + await writeFile(diffPath, diff); + console.log(`Stored PR diff (${diff.length} bytes) at ${diffPath}`); + return diffPath; +} + +export async function fetchAndStoreComments( + octokit: Octokits, + owner: string, + repo: string, + prNumber: number, + tempDir: string, +): Promise { + const promptsDir = `${tempDir}/droid-prompts`; + await mkdir(promptsDir, { recursive: true }); + + const [issueComments, reviewComments] = await Promise.all([ + octokit.rest.issues.listComments({ + owner, + repo, + issue_number: prNumber, + per_page: 100, + }), + octokit.rest.pulls.listReviewComments({ + owner, + repo, + pull_number: prNumber, + per_page: 100, + }), + ]); + + const comments = { + issueComments: issueComments.data, + reviewComments: reviewComments.data, + }; + + const commentsPath = `${promptsDir}/existing_comments.json`; + await writeFile(commentsPath, JSON.stringify(comments, null, 2)); + console.log( + `Stored existing comments (${issueComments.data.length} issue, ${reviewComments.data.length} review) at ${commentsPath}`, + ); + return commentsPath; +} + +export async function storeDescription( + title: string, + body: string, + tempDir: string, +): Promise { + const promptsDir = `${tempDir}/droid-prompts`; + await mkdir(promptsDir, { recursive: true }); + + const content = `# ${title}\n\n${body}`; + const descriptionPath = `${promptsDir}/pr_description.txt`; + await writeFile(descriptionPath, content); + console.log( + `Stored PR description (${content.length} bytes) at ${descriptionPath}`, + ); + return descriptionPath; +} + +/** + * Pre-compute all review artifacts (diff, comments, description) in parallel. + */ +export async function computeReviewArtifacts(opts: { + baseRef: string; + tempDir: string; + octokit: Octokits; + owner: string; + repo: string; + prNumber: number; + title: string; + body: string; + githubToken?: string; +}): Promise { + const [diffPath, commentsPath, descriptionPath] = await Promise.all([ + computeAndStoreDiff(opts.baseRef, opts.tempDir, { + githubToken: opts.githubToken, + prNumber: opts.prNumber, + }), + fetchAndStoreComments( + opts.octokit, + opts.owner, + opts.repo, + opts.prNumber, + opts.tempDir, + ), + storeDescription(opts.title, opts.body, opts.tempDir), + ]); + + return { diffPath, commentsPath, descriptionPath }; +} diff --git a/src/github/operations/comments/fetch-droid-comment.ts b/src/github/operations/comments/fetch-droid-comment.ts index 98eb071..bbc1b25 100644 --- a/src/github/operations/comments/fetch-droid-comment.ts +++ b/src/github/operations/comments/fetch-droid-comment.ts @@ -62,9 +62,7 @@ export async function fetchDroidComment( } catch (issueError) { // If event wasn't a PR review comment event, try review comment API as fallback if (!isPullRequestReviewCommentEvent) { - console.log( - "Issue comment fetch failed, trying PR review comment API", - ); + console.log("Issue comment fetch failed, trying PR review comment API"); try { const { data: prComment } = await octokit.rest.pulls.getReviewComment( { diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 0714523..80adde5 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -1,4 +1,5 @@ import * as core from "@actions/core"; +import path from "path"; import { GITHUB_API_URL, GITHUB_SERVER_URL } from "../github/api/config"; import type { GitHubContext } from "../github/context"; import { isEntityContext } from "../github/context"; @@ -61,6 +62,10 @@ export async function prepareMcpTools( try { const allowedToolsList = allowedTools || []; + // GITHUB_ACTION_PATH points to the sub-action directory (e.g., review/, combine/) + // but src/ lives at the repo root, one level up. + const repoRoot = path.resolve(process.env.GITHUB_ACTION_PATH || ".", ".."); + const hasGitHubMcpTools = allowedToolsList.some((tool) => tool.startsWith("github___"), ); @@ -79,10 +84,7 @@ export async function prepareMcpTools( baseMcpTools.mcpServers.github_comment = { command: "bun", - args: [ - "run", - `${process.env.GITHUB_ACTION_PATH}/src/mcp/github-comment-server.ts`, - ], + args: ["run", `${repoRoot}/src/mcp/github-comment-server.ts`], env: { GITHUB_TOKEN: githubToken, REPO_OWNER: owner, @@ -101,10 +103,7 @@ export async function prepareMcpTools( ) { baseMcpTools.mcpServers.github_inline_comment = { command: "bun", - args: [ - "run", - `${process.env.GITHUB_ACTION_PATH}/src/mcp/github-inline-comment-server.ts`, - ], + args: ["run", `${repoRoot}/src/mcp/github-inline-comment-server.ts`], env: { GITHUB_TOKEN: githubToken, REPO_OWNER: owner, @@ -136,10 +135,7 @@ export async function prepareMcpTools( } baseMcpTools.mcpServers.github_ci = { command: "bun", - args: [ - "run", - `${process.env.GITHUB_ACTION_PATH}/src/mcp/github-actions-server.ts`, - ], + args: ["run", `${repoRoot}/src/mcp/github-actions-server.ts`], env: { // Use workflow github token, not app token GITHUB_TOKEN: process.env.DEFAULT_WORKFLOW_TOKEN, @@ -154,10 +150,7 @@ export async function prepareMcpTools( if (isEntityContext(context) && context.isPR && hasGitHubPRTools) { baseMcpTools.mcpServers.github_pr = { command: "bun", - args: [ - "run", - `${process.env.GITHUB_ACTION_PATH}/src/mcp/github-pr-server.ts`, - ], + args: ["run", `${repoRoot}/src/mcp/github-pr-server.ts`], env: { GITHUB_TOKEN: githubToken, REPO_OWNER: owner, diff --git a/src/tag/commands/review-validator.ts b/src/tag/commands/review-validator.ts index 9d574eb..dc2c05f 100644 --- a/src/tag/commands/review-validator.ts +++ b/src/tag/commands/review-validator.ts @@ -3,104 +3,13 @@ import type { GitHubContext } from "../../github/context"; import { isEntityContext } from "../../github/context"; import type { Octokits } from "../../github/api/client"; import { fetchPRBranchData } from "../../github/data/pr-fetcher"; +import { computeReviewArtifacts } from "../../github/data/review-artifacts"; import { createPrompt } from "../../create-prompt"; -import type { ReviewArtifacts } from "../../create-prompt"; import { prepareMcpTools } from "../../mcp/install-mcp-server"; import { normalizeDroidArgs, parseAllowedTools } from "../../utils/parse-tools"; import type { PrepareResult } from "../../prepare/types"; import { generateReviewValidatorPrompt } from "../../create-prompt/templates/review-validator-prompt"; import { execSync } from "child_process"; -import { mkdir, writeFile } from "fs/promises"; - -const DIFF_MAX_BUFFER = 50 * 1024 * 1024; - -async function computeAndStoreDiff( - baseRef: string, - tempDir: string, -): Promise { - const promptsDir = `${tempDir}/droid-prompts`; - await mkdir(promptsDir, { recursive: true }); - - try { - execSync("git fetch --unshallow", { encoding: "utf8", stdio: "pipe" }); - console.log("Unshallowed repository"); - } catch { - console.log("Repository already has full history"); - } - - try { - execSync(`git fetch origin ${baseRef}:refs/remotes/origin/${baseRef}`, { - encoding: "utf8", - stdio: "pipe", - }); - console.log(`Fetched base branch: ${baseRef}`); - } catch (e) { - console.error(`Failed to fetch base branch ${baseRef}: ${e}`); - throw new Error(`Failed to fetch base branch ${baseRef} for review`); - } - - const mergeBase = execSync(`git merge-base HEAD origin/${baseRef}`, { - encoding: "utf8", - stdio: "pipe", - }).trim(); - - const diff = execSync(`git --no-pager diff ${mergeBase}..HEAD`, { - encoding: "utf8", - stdio: "pipe", - maxBuffer: DIFF_MAX_BUFFER, - }); - - const diffPath = `${promptsDir}/pr.diff`; - await writeFile(diffPath, diff); - console.log(`Stored PR diff (${diff.length} bytes) at ${diffPath}`); - - return diffPath; -} - -async function fetchAndStoreComments( - octokit: Octokits, - owner: string, - repo: string, - prNumber: number, - tempDir: string, -): Promise { - const promptsDir = `${tempDir}/droid-prompts`; - await mkdir(promptsDir, { recursive: true }); - - const [issueComments, reviewComments] = await Promise.all([ - octokit.rest.issues.listComments({ owner, repo, issue_number: prNumber }), - octokit.rest.pulls.listReviewComments({ owner, repo, pull_number: prNumber }), - ]); - - const commentsPath = `${promptsDir}/existing_comments.json`; - const payload = { - issueComments: issueComments.data, - reviewComments: reviewComments.data, - }; - await writeFile(commentsPath, JSON.stringify(payload, null, 2)); - console.log( - `Stored existing comments (${issueComments.data.length} issue, ${reviewComments.data.length} review) at ${commentsPath}`, - ); - - return commentsPath; -} - -async function storeDescription( - title: string, - body: string, - tempDir: string, -): Promise { - const promptsDir = `${tempDir}/droid-prompts`; - await mkdir(promptsDir, { recursive: true }); - - const content = `# ${title}\n\n${body}`; - const descriptionPath = `${promptsDir}/pr_description.txt`; - await writeFile(descriptionPath, content); - console.log( - `Stored PR description (${content.length} bytes) at ${descriptionPath}`, - ); - return descriptionPath; -} export async function prepareReviewValidatorMode({ context, @@ -119,7 +28,10 @@ export async function prepareReviewValidatorMode({ const prData = await fetchPRBranchData({ octokits: octokit, - repository: { owner: context.repository.owner, repo: context.repository.repo }, + repository: { + owner: context.repository.owner, + repo: context.repository.repo, + }, prNumber: context.entityNumber, }); @@ -127,7 +39,10 @@ export async function prepareReviewValidatorMode({ // Ensure PR branch is checked out (same as review mode) try { - execSync("git reset --hard HEAD", { encoding: "utf8", stdio: "pipe" } as any); + execSync("git reset --hard HEAD", { + encoding: "utf8", + stdio: "pipe", + } as any); execSync(`gh pr checkout ${context.entityNumber}`, { encoding: "utf8", stdio: "pipe", @@ -138,22 +53,22 @@ export async function prepareReviewValidatorMode({ ); } catch (e) { console.error(`Failed to checkout PR branch: ${e}`); - throw new Error(`Failed to checkout PR #${context.entityNumber} branch for review`); + throw new Error( + `Failed to checkout PR #${context.entityNumber} branch for review`, + ); } - const [diffPath, commentsPath, descriptionPath] = await Promise.all([ - computeAndStoreDiff(prData.baseRefName, tempDir), - fetchAndStoreComments( - octokit, - context.repository.owner, - context.repository.repo, - context.entityNumber, - tempDir, - ), - storeDescription(prData.title, prData.body, tempDir), - ]); - - const reviewArtifacts: ReviewArtifacts = { diffPath, commentsPath, descriptionPath }; + const reviewArtifacts = await computeReviewArtifacts({ + baseRef: prData.baseRefName, + tempDir, + octokit, + owner: context.repository.owner, + repo: context.repository.repo, + prNumber: context.entityNumber, + title: prData.title, + body: prData.body, + githubToken, + }); await createPrompt({ githubContext: context, @@ -211,12 +126,11 @@ export async function prepareReviewValidatorMode({ const reviewModel = process.env.REVIEW_MODEL?.trim(); const reasoningEffort = process.env.REASONING_EFFORT?.trim(); - if (!reviewModel && !reasoningEffort) { - droidArgParts.push(`--model "gpt-5.2"`); - droidArgParts.push(`--reasoning-effort "high"`); - } else { - if (reviewModel) droidArgParts.push(`--model "${reviewModel}"`); - if (reasoningEffort) droidArgParts.push(`--reasoning-effort "${reasoningEffort}"`); + if (reviewModel) { + droidArgParts.push(`--model "${reviewModel}"`); + } + if (reasoningEffort) { + droidArgParts.push(`--reasoning-effort "${reasoningEffort}"`); } if (normalizedUserArgs) { diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index a69ce85..9b77f45 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -1,10 +1,9 @@ import * as core from "@actions/core"; import { execSync } from "child_process"; -import { writeFile, mkdir } from "fs/promises"; import type { GitHubContext } from "../../github/context"; import { fetchPRBranchData } from "../../github/data/pr-fetcher"; +import { computeReviewArtifacts } from "../../github/data/review-artifacts"; import { createPrompt } from "../../create-prompt"; -import type { ReviewArtifacts } from "../../create-prompt"; import { prepareMcpTools } from "../../mcp/install-mcp-server"; import { createInitialComment } from "../../github/operations/comments/create-initial"; import { normalizeDroidArgs, parseAllowedTools } from "../../utils/parse-tools"; @@ -14,107 +13,6 @@ import { generateReviewCandidatesPrompt } from "../../create-prompt/templates/re import type { Octokits } from "../../github/api/client"; import type { PrepareResult } from "../../prepare/types"; -const DIFF_MAX_BUFFER = 50 * 1024 * 1024; // 50MB buffer for large diffs - -async function computeAndStoreDiff( - baseRef: string, - tempDir: string, -): Promise { - const promptsDir = `${tempDir}/droid-prompts`; - await mkdir(promptsDir, { recursive: true }); - - // Unshallow the repo if it's a shallow clone (needed for merge-base to work) - try { - execSync("git fetch --unshallow", { encoding: "utf8", stdio: "pipe" }); - console.log("Unshallowed repository"); - } catch (e) { - // Already unshallowed or not a shallow clone, continue - console.log("Repository already has full history"); - } - - // Fetch the base branch (it may not exist locally yet) - try { - execSync( - `git fetch origin ${baseRef}:refs/remotes/origin/${baseRef}`, - { encoding: "utf8", stdio: "pipe" }, - ); - console.log(`Fetched base branch: ${baseRef}`); - } catch (e) { - // Branch might already exist, continue - console.log(`Base branch fetch skipped (may already exist): ${baseRef}`); - } - - const mergeBase = execSync( - `git merge-base HEAD refs/remotes/origin/${baseRef}`, - { encoding: "utf8" }, - ).trim(); - - const diff = execSync(`git --no-pager diff ${mergeBase}..HEAD`, { - encoding: "utf8", - maxBuffer: DIFF_MAX_BUFFER, - }); - - const diffPath = `${promptsDir}/pr.diff`; - await writeFile(diffPath, diff); - console.log(`Stored PR diff (${diff.length} bytes) at ${diffPath}`); - return diffPath; -} - -async function fetchAndStoreComments( - octokit: Octokits, - owner: string, - repo: string, - prNumber: number, - tempDir: string, -): Promise { - const promptsDir = `${tempDir}/droid-prompts`; - await mkdir(promptsDir, { recursive: true }); - - const [issueComments, reviewComments] = await Promise.all([ - octokit.rest.issues.listComments({ - owner, - repo, - issue_number: prNumber, - per_page: 100, - }), - octokit.rest.pulls.listReviewComments({ - owner, - repo, - pull_number: prNumber, - per_page: 100, - }), - ]); - - const comments = { - issueComments: issueComments.data, - reviewComments: reviewComments.data, - }; - - const commentsPath = `${promptsDir}/existing_comments.json`; - await writeFile(commentsPath, JSON.stringify(comments, null, 2)); - console.log( - `Stored existing comments (${issueComments.data.length} issue, ${reviewComments.data.length} review) at ${commentsPath}`, - ); - return commentsPath; -} - -async function storeDescription( - title: string, - body: string, - tempDir: string, -): Promise { - const promptsDir = `${tempDir}/droid-prompts`; - await mkdir(promptsDir, { recursive: true }); - - const content = `# ${title}\n\n${body}`; - const descriptionPath = `${promptsDir}/pr_description.txt`; - await writeFile(descriptionPath, content); - console.log( - `Stored PR description (${content.length} bytes) at ${descriptionPath}`, - ); - return descriptionPath; -} - type ReviewCommandOptions = { context: GitHubContext; octokit: Octokits; @@ -175,23 +73,17 @@ export async function prepareReviewMode({ // Pre-compute review artifacts (diff, existing comments, and PR description) const tempDir = process.env.RUNNER_TEMP || "/tmp"; - const [diffPath, commentsPath, descriptionPath] = await Promise.all([ - computeAndStoreDiff(prData.baseRefName, tempDir), - fetchAndStoreComments( - octokit, - context.repository.owner, - context.repository.repo, - context.entityNumber, - tempDir, - ), - storeDescription(prData.title, prData.body, tempDir), - ]); - - const reviewArtifacts: ReviewArtifacts = { - diffPath, - commentsPath, - descriptionPath, - }; + const reviewArtifacts = await computeReviewArtifacts({ + baseRef: prData.baseRefName, + tempDir, + octokit, + owner: context.repository.owner, + repo: context.repository.repo, + prNumber: context.entityNumber, + title: prData.title, + body: prData.body, + githubToken, + }); const reviewUseValidator = (process.env.REVIEW_USE_VALIDATOR ?? "true").trim() !== "false"; @@ -232,7 +124,9 @@ export async function prepareReviewMode({ // Task tool is needed for parallel subagent reviews in candidate generation phase. // FetchUrl is needed to fetch linked tickets from the PR description. - const candidateGenerationTools = reviewUseValidator ? ["Task", "FetchUrl"] : []; + const candidateGenerationTools = reviewUseValidator + ? ["Task", "FetchUrl"] + : []; const reviewTools = reviewUseValidator ? [] @@ -280,18 +174,11 @@ export async function prepareReviewMode({ const reviewModel = process.env.REVIEW_MODEL?.trim(); const reasoningEffort = process.env.REASONING_EFFORT?.trim(); - // action.yml defaults review_model to gpt-5.2, so reviewModel is - // normally always set. The fallback keeps things working outside the action. - if (!reviewModel && !reasoningEffort) { - droidArgParts.push(`--model "gpt-5.2"`); - droidArgParts.push(`--reasoning-effort "high"`); - } else { - if (reviewModel) { - droidArgParts.push(`--model "${reviewModel}"`); - } - if (reasoningEffort) { - droidArgParts.push(`--reasoning-effort "${reasoningEffort}"`); - } + if (reviewModel) { + droidArgParts.push(`--model "${reviewModel}"`); + } + if (reasoningEffort) { + droidArgParts.push(`--reasoning-effort "${reasoningEffort}"`); } if (normalizedUserArgs) { diff --git a/test/create-prompt/templates/review-prompt.test.ts b/test/create-prompt/templates/review-prompt.test.ts index 7bcb90c..72b7760 100644 --- a/test/create-prompt/templates/review-prompt.test.ts +++ b/test/create-prompt/templates/review-prompt.test.ts @@ -155,4 +155,25 @@ describe("generateReviewPrompt", () => { expect(prompt).toContain("pr_description.txt"); }); + + it("includes output file instructions when outputFilePath is set", () => { + const context = createBaseContext({ + outputFilePath: "/tmp/results/code-review-results.json", + }); + + const prompt = generateReviewPrompt(context); + + expect(prompt).toContain("## Output File (REQUIRED)"); + expect(prompt).toContain("/tmp/results/code-review-results.json"); + expect(prompt).toContain('"type": "code-review"'); + expect(prompt).toContain("combine step to aggregate results"); + }); + + it("does not include output file section when outputFilePath is not set", () => { + const context = createBaseContext(); + + const prompt = generateReviewPrompt(context); + + expect(prompt).not.toContain("## Output File (REQUIRED)"); + }); }); diff --git a/test/create-prompt/templates/security-review-prompt.test.ts b/test/create-prompt/templates/security-review-prompt.test.ts index 5014fcd..a888d60 100644 --- a/test/create-prompt/templates/security-review-prompt.test.ts +++ b/test/create-prompt/templates/security-review-prompt.test.ts @@ -60,6 +60,27 @@ describe("generateSecurityReviewPrompt", () => { expect(prompt).toContain("LOW"); }); + it("uses outputFilePath when provided", () => { + const context = createBaseContext({ + outputFilePath: "/tmp/results/security-results.json", + }); + + const prompt = generateSecurityReviewPrompt(context); + + expect(prompt).toContain("/tmp/results/security-results.json"); + expect(prompt).not.toContain( + "Write findings to `security-review-results.json`", + ); + }); + + it("falls back to default filename when outputFilePath is not set", () => { + const context = createBaseContext(); + + const prompt = generateSecurityReviewPrompt(context); + + expect(prompt).toContain("security-review-results.json"); + }); + it("includes security configuration from context", () => { const contextWithConfig = createBaseContext({ githubContext: { diff --git a/test/entrypoints/prepare-validator.test.ts b/test/entrypoints/prepare-validator.test.ts index 6b3bc80..28f9d96 100644 --- a/test/entrypoints/prepare-validator.test.ts +++ b/test/entrypoints/prepare-validator.test.ts @@ -45,7 +45,7 @@ describe("prepare-validator entrypoint", () => { } as any); const mod = await import( - `../../src/entrypoints/prepare-validator.ts?test=${Math.random()}`, + `../../src/entrypoints/prepare-validator.ts?test=${Math.random()}` ); await expect(mod.default()).rejects.toBeTruthy(); diff --git a/test/fetch-droid-comment.test.ts b/test/fetch-droid-comment.test.ts index eeb67e5..308b527 100644 --- a/test/fetch-droid-comment.test.ts +++ b/test/fetch-droid-comment.test.ts @@ -26,8 +26,7 @@ describe("fetchDroidComment", () => { data: { id: 123456, body: "Test issue comment", - html_url: - "https://github.com/owner/repo/issues/1#issuecomment-123456", + html_url: "https://github.com/owner/repo/issues/1#issuecomment-123456", }, }; @@ -59,8 +58,7 @@ describe("fetchDroidComment", () => { data: { id: 789012, body: "Test PR review comment", - html_url: - "https://github.com/owner/repo/pull/1#discussion_r789012", + html_url: "https://github.com/owner/repo/pull/1#discussion_r789012", }, }; @@ -146,8 +144,7 @@ describe("fetchDroidComment", () => { data: { id: 456789, body: "Test issue comment fetched via fallback", - html_url: - "https://github.com/owner/repo/issues/1#issuecomment-456789", + html_url: "https://github.com/owner/repo/issues/1#issuecomment-456789", }, }; @@ -183,9 +180,7 @@ describe("fetchDroidComment", () => { comment_id: 456789, }); - expect(result.comment.body).toBe( - "Test issue comment fetched via fallback", - ); + expect(result.comment.body).toBe("Test issue comment fetched via fallback"); expect(result.isPRReviewComment).toBe(false); }); @@ -266,8 +261,7 @@ describe("fetchDroidComment", () => { data: { id: 111222, body: null, - html_url: - "https://github.com/owner/repo/issues/1#issuecomment-111222", + html_url: "https://github.com/owner/repo/issues/1#issuecomment-111222", }, }; diff --git a/test/github/data/review-artifacts.test.ts b/test/github/data/review-artifacts.test.ts new file mode 100644 index 0000000..3a3b2ae --- /dev/null +++ b/test/github/data/review-artifacts.test.ts @@ -0,0 +1,204 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import { + computeAndStoreDiff, + fetchAndStoreComments, + storeDescription, + computeReviewArtifacts, +} from "../../../src/github/data/review-artifacts"; +import * as childProcess from "child_process"; +import * as fsPromises from "fs/promises"; + +describe("review-artifacts", () => { + let execSyncSpy: ReturnType; + let writeFileSpy: ReturnType; + let mkdirSpy: ReturnType; + + beforeEach(() => { + writeFileSpy = spyOn(fsPromises, "writeFile").mockResolvedValue(); + mkdirSpy = spyOn(fsPromises, "mkdir").mockResolvedValue(undefined); + }); + + afterEach(() => { + execSyncSpy?.mockRestore(); + writeFileSpy.mockRestore(); + mkdirSpy.mockRestore(); + }); + + describe("computeAndStoreDiff", () => { + it("computes diff via git merge-base and writes to disk", async () => { + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + ) => { + if (cmd.includes("is-shallow-repository")) return "false\n"; + if (cmd.includes("fetch origin main")) return ""; + if (cmd.includes("merge-base")) return "abc123\n"; + if (cmd.includes("diff")) return "diff --git a/f.ts b/f.ts\n+line\n"; + return ""; + }) as typeof childProcess.execSync); + + const result = await computeAndStoreDiff("main", "/tmp/test"); + + expect(result).toBe("/tmp/test/droid-prompts/pr.diff"); + expect(mkdirSpy).toHaveBeenCalledWith("/tmp/test/droid-prompts", { + recursive: true, + }); + const writeCall = writeFileSpy.mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && (c[0] as string).includes("pr.diff"), + ); + expect(writeCall).toBeDefined(); + expect(writeCall![1]).toContain("diff --git"); + }); + + it("falls back to gh pr diff when merge-base fails", async () => { + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + opts?: any, + ) => { + if (cmd.includes("is-shallow-repository")) return "false\n"; + if (cmd.includes("merge-base")) throw new Error("no merge base"); + if (cmd.includes("gh pr diff")) { + expect(opts?.env?.GH_TOKEN).toBe("test-token"); + return "diff from gh cli\n"; + } + return ""; + }) as typeof childProcess.execSync); + + const result = await computeAndStoreDiff("main", "/tmp/test", { + githubToken: "test-token", + prNumber: 42, + }); + + expect(result).toBe("/tmp/test/droid-prompts/pr.diff"); + const writeCall = writeFileSpy.mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && (c[0] as string).includes("pr.diff"), + ); + expect(writeCall![1]).toContain("diff from gh cli"); + }); + + it("throws when merge-base fails and no fallback credentials", async () => { + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + ) => { + if (cmd.includes("is-shallow-repository")) return "false\n"; + if (cmd.includes("merge-base")) throw new Error("no merge base"); + return ""; + }) as typeof childProcess.execSync); + + await expect(computeAndStoreDiff("main", "/tmp/test")).rejects.toThrow( + "no fallback credentials", + ); + }); + }); + + describe("fetchAndStoreComments", () => { + it("fetches issue and review comments and writes JSON", async () => { + const mockOctokit = { + rest: { + issues: { + listComments: () => + Promise.resolve({ + data: [{ id: 1, body: "issue comment" }], + }), + }, + pulls: { + listReviewComments: () => + Promise.resolve({ + data: [{ id: 2, body: "review comment" }], + }), + }, + }, + } as any; + + const result = await fetchAndStoreComments( + mockOctokit, + "owner", + "repo", + 42, + "/tmp/test", + ); + + expect(result).toBe("/tmp/test/droid-prompts/existing_comments.json"); + const writeCall = writeFileSpy.mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && + (c[0] as string).includes("existing_comments.json"), + ); + expect(writeCall).toBeDefined(); + const written = JSON.parse(writeCall![1] as string); + expect(written.issueComments).toHaveLength(1); + expect(written.reviewComments).toHaveLength(1); + }); + }); + + describe("storeDescription", () => { + it("writes PR title and body as markdown", async () => { + const result = await storeDescription( + "My Feature", + "Adds cool stuff", + "/tmp/test", + ); + + expect(result).toBe("/tmp/test/droid-prompts/pr_description.txt"); + const writeCall = writeFileSpy.mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && + (c[0] as string).includes("pr_description.txt"), + ); + expect(writeCall).toBeDefined(); + expect(writeCall![1]).toBe("# My Feature\n\nAdds cool stuff"); + }); + + it("handles empty body", async () => { + await storeDescription("Title Only", "", "/tmp/test"); + + const writeCall = writeFileSpy.mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && + (c[0] as string).includes("pr_description.txt"), + ); + expect(writeCall![1]).toBe("# Title Only\n\n"); + }); + }); + + describe("computeReviewArtifacts", () => { + it("runs all three artifact computations in parallel", async () => { + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + ) => { + if (cmd.includes("is-shallow-repository")) return "false\n"; + if (cmd.includes("merge-base")) return "abc123\n"; + if (cmd.includes("diff")) return "some diff\n"; + return ""; + }) as typeof childProcess.execSync); + + const mockOctokit = { + rest: { + issues: { + listComments: () => Promise.resolve({ data: [] }), + }, + pulls: { + listReviewComments: () => Promise.resolve({ data: [] }), + }, + }, + } as any; + + const result = await computeReviewArtifacts({ + baseRef: "main", + tempDir: "/tmp/test", + octokit: mockOctokit, + owner: "owner", + repo: "repo", + prNumber: 99, + title: "Test PR", + body: "Test body", + githubToken: "token", + }); + + expect(result.diffPath).toContain("pr.diff"); + expect(result.commentsPath).toContain("existing_comments.json"); + expect(result.descriptionPath).toContain("pr_description.txt"); + }); + }); +}); diff --git a/test/integration/review-flow.test.ts b/test/integration/review-flow.test.ts index 7d8f64b..e8c51d5 100644 --- a/test/integration/review-flow.test.ts +++ b/test/integration/review-flow.test.ts @@ -40,15 +40,15 @@ describe("review command integration", () => { setOutputSpy = spyOn(core, "setOutput").mockImplementation(() => {}); exportVarSpy = spyOn(core, "exportVariable").mockImplementation(() => {}); - execSyncSpy = spyOn(childProcess, "execSync").mockImplementation( - ((cmd: string) => { - if (cmd.includes("merge-base")) return "abc123def456\n"; - if (cmd.includes("git --no-pager diff")) { - return "diff --git a/file.ts b/file.ts\n+added line\n"; - } - return ""; - }) as typeof childProcess.execSync, - ); + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + ) => { + if (cmd.includes("merge-base")) return "abc123def456\n"; + if (cmd.includes("git --no-pager diff")) { + return "diff --git a/file.ts b/file.ts\n+added line\n"; + } + return ""; + }) as typeof childProcess.execSync); }); afterEach(async () => { @@ -103,7 +103,7 @@ describe("review command integration", () => { } as any, }); - const octokit = { + const octokit = { rest: { issues: { listComments: () => Promise.resolve({ data: [] }), @@ -111,16 +111,17 @@ describe("review command integration", () => { pulls: { listReviewComments: () => Promise.resolve({ data: [] }), }, - }, - graphql: () => Promise.resolve({ - repository: { - pullRequest: { - baseRefName: "main", - headRefName: "feature/review", - headRefOid: "def456", - } - } - }) + }, + graphql: () => + Promise.resolve({ + repository: { + pullRequest: { + baseRefName: "main", + headRefName: "feature/review", + headRefOid: "def456", + }, + }, + }), } as any; graphqlSpy = spyOn(octokit, "graphql").mockResolvedValue({ diff --git a/test/modes/tag/review-command.test.ts b/test/modes/tag/review-command.test.ts index 6ddedc0..5b8b8af 100644 --- a/test/modes/tag/review-command.test.ts +++ b/test/modes/tag/review-command.test.ts @@ -58,17 +58,17 @@ describe("prepareReviewMode", () => { () => {}, ); // Mock execSync for git commands - execSyncSpy = spyOn(childProcess, "execSync").mockImplementation( - ((cmd: string) => { - if (cmd.includes("merge-base")) { - return "abc123def456\n"; - } - if (cmd.includes("diff")) { - return "diff --git a/file.ts b/file.ts\n+added line\n"; - } - return ""; - }) as typeof childProcess.execSync, - ); + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + ) => { + if (cmd.includes("merge-base")) { + return "abc123def456\n"; + } + if (cmd.includes("diff")) { + return "diff --git a/file.ts b/file.ts\n+added line\n"; + } + return ""; + }) as typeof childProcess.execSync); // Mock file system operations writeFileSpy = spyOn(fsPromises, "writeFile").mockResolvedValue(); mkdirSpy = spyOn(fsPromises, "mkdir").mockResolvedValue(undefined); @@ -389,9 +389,10 @@ describe("prepareReviewMode", () => { const droidArgsCall = setOutputSpy.mock.calls.find( (call: unknown[]) => call[0] === "droid_args", ) as [string, string] | undefined; - // When neither REVIEW_MODEL nor REASONING_EFFORT is provided, we default to gpt-5.2 at high reasoning. - expect(droidArgsCall?.[1]).toContain('--model "gpt-5.2"'); - expect(droidArgsCall?.[1]).toContain('--reasoning-effort "high"'); + // When neither REVIEW_MODEL nor REASONING_EFFORT is provided, no --model or --reasoning-effort + // flags are added. Defaults are handled by the action.yml inputs (gpt-5.2 / high). + expect(droidArgsCall?.[1]).not.toContain("--model"); + expect(droidArgsCall?.[1]).not.toContain("--reasoning-effort"); }); it("stores PR description as an artifact file", async () => {