From 6a126a0a825916493347f9196c5af26a0a9b0f4c Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 16:55:01 -0800 Subject: [PATCH 01/13] fix: pre-compute review artifacts and deduplicate into shared module, remove security review from workflow --- .github/workflows/droid-review.yml | 49 +--- review/action.yml | 1 + security/action.yml | 1 + src/create-prompt/index.ts | 6 + src/create-prompt/templates/review-prompt.ts | 34 ++- .../templates/security-review-prompt.ts | 2 +- src/create-prompt/types.ts | 1 + src/entrypoints/generate-review-prompt.ts | 45 ++++ src/github/data/review-artifacts.ts | 171 ++++++++++++++ src/tag/commands/review-validator.ts | 117 +--------- src/tag/commands/review.ts | 132 +---------- .../templates/review-prompt.test.ts | 21 ++ .../templates/security-review-prompt.test.ts | 21 ++ test/github/data/review-artifacts.test.ts | 210 ++++++++++++++++++ 14 files changed, 538 insertions(+), 273 deletions(-) create mode 100644 src/github/data/review-artifacts.ts create mode 100644 test/github/data/review-artifacts.test.ts diff --git a/.github/workflows/droid-review.yml b/.github/workflows/droid-review.yml index 44a8a6f..545c9b5 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 @@ -44,7 +43,7 @@ jobs: - 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 @@ -60,44 +59,11 @@ 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 - uses: actions/upload-artifact@v4 - with: - name: security-results - path: ${{ runner.temp }}/security-results.json - if-no-files-found: ignore - 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 +84,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 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/review/action.yml b/review/action.yml index 257ad9c..552ce6b 100644 --- a/review/action.yml +++ b/review/action.yml @@ -68,6 +68,7 @@ runs: DROID_COMMENT_ID: ${{ inputs.tracking_comment_id }} REVIEW_MODEL: ${{ inputs.review_model }} REVIEW_TYPE: "code" + DROID_OUTPUT_FILE: ${{ inputs.output_file }} - name: Run Code Review id: review 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-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 0f74e89..93e4702 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -5,9 +5,11 @@ */ 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"; @@ -47,12 +49,53 @@ async function run() { currentBranch: prData.headRefName, }; + // 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.warn( + `Failed to checkout PR branch, will use fallback diff method: ${e}`, + ); + } + + 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 const generatePrompt = reviewType === "security" ? generateSecurityReviewPrompt : 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, commentId, @@ -62,6 +105,8 @@ async function run() { headRefOid: prData.headRefOid, }, generatePrompt, + reviewArtifacts, + outputFilePath, }); // Set run type diff --git a/src/github/data/review-artifacts.ts b/src/github/data/review-artifacts.ts new file mode 100644 index 0000000..acbd632 --- /dev/null +++ b/src/github/data/review-artifacts.ts @@ -0,0 +1,171 @@ +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/tag/commands/review-validator.ts b/src/tag/commands/review-validator.ts index 9d574eb..f519987 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, @@ -141,19 +50,17 @@ export async function prepareReviewValidatorMode({ 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, diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index a69ce85..1066a3c 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"; 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/github/data/review-artifacts.test.ts b/test/github/data/review-artifacts.test.ts new file mode 100644 index 0000000..5d6628a --- /dev/null +++ b/test/github/data/review-artifacts.test.ts @@ -0,0 +1,210 @@ +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"); + }); + }); +}); From 7d253d4276e1ad19dfd01669247ae771265980b6 Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 17:07:52 -0800 Subject: [PATCH 02/13] test --- .github/workflows/droid-review.yml | 6 +++--- .github/workflows/droid.yml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/droid-review.yml b/.github/workflows/droid-review.yml index 545c9b5..b546a1a 100644 --- a/.github/workflows/droid-review.yml +++ b/.github/workflows/droid-review.yml @@ -24,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 @@ -46,7 +46,7 @@ jobs: 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 }} @@ -85,7 +85,7 @@ jobs: 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 }} diff --git a/.github/workflows/droid.yml b/.github/workflows/droid.yml index 36c7be8..d36d5d4 100644 --- a/.github/workflows/droid.yml +++ b/.github/workflows/droid.yml @@ -34,6 +34,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 }} From 0ffb8e0f43c1749452075c714b379f66bc292b98 Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 17:43:48 -0800 Subject: [PATCH 03/13] fix: use review-candidates prompt and two-pass validator for automatic review flow --- .github/workflows/droid-review.yml | 11 +++++ action.yml | 5 +-- review/action.yml | 49 ++++++++++++++++++++ src/entrypoints/generate-review-prompt.ts | 55 ++++++++++++++++++----- 4 files changed, 106 insertions(+), 14 deletions(-) diff --git a/.github/workflows/droid-review.yml b/.github/workflows/droid-review.yml index b546a1a..11fa87f 100644 --- a/.github/workflows/droid-review.yml +++ b/.github/workflows/droid-review.yml @@ -59,6 +59,17 @@ jobs: path: ${{ runner.temp }}/code-review-results.json if-no-files-found: ignore + - name: Upload debug artifacts + if: always() + uses: actions/upload-artifact@v4 + with: + 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] if: | diff --git a/action.yml b/action.yml index 457b734..d46a6f2 100644 --- a/action.yml +++ b/action.yml @@ -402,10 +402,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 552ce6b..bd08f16 100644 --- a/review/action.yml +++ b/review/action.yml @@ -20,6 +20,18 @@ inputs: 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,8 +80,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 }} 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 shell: bash @@ -82,3 +105,29 @@ 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 }} + 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/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 93e4702..6a4cc14 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -13,6 +13,7 @@ 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"; @@ -20,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) { @@ -69,8 +73,9 @@ async function run() { `Successfully checked out PR branch: ${execSync("git rev-parse --abbrev-ref HEAD", { encoding: "utf8" }).trim()}`, ); } catch (e) { - console.warn( - `Failed to checkout PR branch, will use fallback diff method: ${e}`, + console.error(`Failed to checkout PR branch: ${e}`); + throw new Error( + `Failed to checkout PR #${context.entityNumber} branch for review`, ); } @@ -86,11 +91,13 @@ async function run() { githubToken, }); - // Select prompt generator based on review type + // 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 @@ -120,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({ @@ -173,8 +205,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}`); From f678487f0098237e97dbc6a6d236eac223ad5b6e Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 22:44:52 -0800 Subject: [PATCH 04/13] add gpt-5.2 default model --- review/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/review/action.yml b/review/action.yml index bd08f16..3f18fd6 100644 --- a/review/action.yml +++ b/review/action.yml @@ -15,7 +15,7 @@ inputs: review_model: description: "Model to use for review" required: false - default: "" + default: "gpt-5.2" output_file: description: "Path to write review results JSON" required: false From 074167ca60bf52116926214804825935e932caa8 Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 22:49:02 -0800 Subject: [PATCH 05/13] fix: default to gpt-5.2 with high reasoning and simplify model config --- action.yml | 2 +- review/action.yml | 5 +++++ src/entrypoints/generate-review-prompt.ts | 11 +++++++---- src/tag/commands/review-validator.ts | 11 +++++------ src/tag/commands/review.ts | 17 +++++------------ test/modes/tag/review-command.test.ts | 7 ++++--- 6 files changed, 27 insertions(+), 26 deletions(-) diff --git a/action.yml b/action.yml index d46a6f2..f06cb53 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 diff --git a/review/action.yml b/review/action.yml index 3f18fd6..4a43f14 100644 --- a/review/action.yml +++ b/review/action.yml @@ -16,6 +16,10 @@ inputs: description: "Model to use for review" required: false 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 @@ -82,6 +86,7 @@ runs: 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 diff --git a/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 6a4cc14..f1534dc 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -188,14 +188,17 @@ async function run() { droidArgParts.push(`--enabled-tools "${builtInTools.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) { diff --git a/src/tag/commands/review-validator.ts b/src/tag/commands/review-validator.ts index f519987..8ba85d0 100644 --- a/src/tag/commands/review-validator.ts +++ b/src/tag/commands/review-validator.ts @@ -118,12 +118,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 1066a3c..f5292ab 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -172,18 +172,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/modes/tag/review-command.test.ts b/test/modes/tag/review-command.test.ts index 6ddedc0..56cccf8 100644 --- a/test/modes/tag/review-command.test.ts +++ b/test/modes/tag/review-command.test.ts @@ -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 () => { From 84caa8c042c812849a4864052f565ec36b4cf25c Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 23:01:06 -0800 Subject: [PATCH 06/13] fix: include MCP tools in --enabled-tools for droid exec --- src/entrypoints/generate-combine-prompt.ts | 6 +----- src/entrypoints/generate-review-prompt.ts | 7 +------ 2 files changed, 2 insertions(+), 11 deletions(-) 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 f1534dc..03b989f 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -181,12 +181,7 @@ 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(",")}"`); const reviewModel = reviewType === "security" From f50bccbc8d8839bd49d4bec3d1e974ea4ccb9f0c Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 23:14:59 -0800 Subject: [PATCH 07/13] add debugging --- .github/workflows/droid-review.yml | 2 ++ .github/workflows/droid.yml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/.github/workflows/droid-review.yml b/.github/workflows/droid-review.yml index 11fa87f..f2e3af7 100644 --- a/.github/workflows/droid-review.yml +++ b/.github/workflows/droid-review.yml @@ -39,6 +39,8 @@ jobs: issues: write id-token: write actions: read + env: + ACTIONS_STEP_DEBUG: true steps: - name: Checkout repository uses: actions/checkout@v5 diff --git a/.github/workflows/droid.yml b/.github/workflows/droid.yml index d36d5d4..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 From c3a464fcc8c7f405ee10dcf7b3b2a6d726d3ed07 Mon Sep 17 00:00:00 2001 From: User Date: Wed, 18 Feb 2026 23:19:43 -0800 Subject: [PATCH 08/13] test with --list-tools --- base-action/src/run-droid.ts | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/base-action/src/run-droid.ts b/base-action/src/run-droid.ts index 015dc98..376f0af 100644 --- a/base-action/src/run-droid.ts +++ b/base-action/src/run-droid.ts @@ -169,6 +169,39 @@ export async function runDroid(promptPath: string, options: DroidOptions) { // Don't continue without MCP if we were expecting it throw new Error(`MCP server registration failed: ${e}`); } + + // List available tools to verify MCP tools are registered + console.log("\nListing available tools after MCP registration..."); + try { + const { stdout: listOutput } = await execAsync( + "droid exec --list-tools", + ); + console.log("Available tools:\n" + listOutput); + } catch (listError: any) { + console.error("Failed to list tools:", listError.message); + } + console.log(""); + + // Check MCP config file content + console.log("Checking MCP config file..."); + try { + const { stdout: mcpConfig } = await execAsync( + "cat ~/.factory/mcp.json 2>/dev/null || echo 'File not found'", + ); + console.log("~/.factory/mcp.json content:\n" + mcpConfig); + } catch (e: any) { + console.error("Failed to read MCP config:", e.message); + } + + // Check registered MCP servers + console.log("\nListing registered MCP servers..."); + try { + const { stdout: mcpList } = await execAsync("droid mcp list"); + console.log("MCP servers:\n" + mcpList); + } catch (e: any) { + console.error("Failed to list MCP servers:", e.message); + } + console.log(""); } const config = prepareRunConfig(promptPath, options); From a18675a4525d1381e59477c12f2154f0d2003747 Mon Sep 17 00:00:00 2001 From: User Date: Thu, 19 Feb 2026 00:02:40 -0800 Subject: [PATCH 09/13] fix: resolve MCP server paths relative to repo root instead of sub-action directory --- .factory/droids/file-group-reviewer.md | 6 +- action.yml | 1 - base-action/src/run-droid.ts | 33 ------- src/entrypoints/prepare-validator.ts | 4 +- src/entrypoints/update-comment-link.ts | 3 +- src/github/data/review-artifacts.ts | 12 ++- .../comments/fetch-droid-comment.ts | 4 +- src/mcp/install-mcp-server.ts | 25 ++---- src/tag/commands/review-validator.ts | 14 ++- src/tag/commands/review.ts | 4 +- test/entrypoints/prepare-validator.test.ts | 2 +- test/fetch-droid-comment.test.ts | 16 ++-- test/github/data/review-artifacts.test.ts | 86 +++++++++---------- test/integration/review-flow.test.ts | 41 ++++----- test/modes/tag/review-command.test.ts | 22 ++--- 15 files changed, 116 insertions(+), 157 deletions(-) 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/action.yml b/action.yml index f06cb53..d31c2bc 100644 --- a/action.yml +++ b/action.yml @@ -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 diff --git a/base-action/src/run-droid.ts b/base-action/src/run-droid.ts index 376f0af..015dc98 100644 --- a/base-action/src/run-droid.ts +++ b/base-action/src/run-droid.ts @@ -169,39 +169,6 @@ export async function runDroid(promptPath: string, options: DroidOptions) { // Don't continue without MCP if we were expecting it throw new Error(`MCP server registration failed: ${e}`); } - - // List available tools to verify MCP tools are registered - console.log("\nListing available tools after MCP registration..."); - try { - const { stdout: listOutput } = await execAsync( - "droid exec --list-tools", - ); - console.log("Available tools:\n" + listOutput); - } catch (listError: any) { - console.error("Failed to list tools:", listError.message); - } - console.log(""); - - // Check MCP config file content - console.log("Checking MCP config file..."); - try { - const { stdout: mcpConfig } = await execAsync( - "cat ~/.factory/mcp.json 2>/dev/null || echo 'File not found'", - ); - console.log("~/.factory/mcp.json content:\n" + mcpConfig); - } catch (e: any) { - console.error("Failed to read MCP config:", e.message); - } - - // Check registered MCP servers - console.log("\nListing registered MCP servers..."); - try { - const { stdout: mcpList } = await execAsync("droid mcp list"); - console.log("MCP servers:\n" + mcpList); - } catch (e: any) { - console.error("Failed to list MCP servers:", e.message); - } - console.log(""); } const config = prepareRunConfig(promptPath, options); 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 index acbd632..c7d7a4f 100644 --- a/src/github/data/review-artifacts.ts +++ b/src/github/data/review-artifacts.ts @@ -39,15 +39,13 @@ export async function computeAndStoreDiff( // Fetch the base branch (it may not exist locally yet) try { - execSync( - `git fetch origin ${baseRef}:refs/remotes/origin/${baseRef}`, - { encoding: "utf8", stdio: "pipe" }, - ); + 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}`, - ); + console.log(`Base branch fetch skipped (may already exist): ${baseRef}`); } const mergeBase = execSync( 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 8ba85d0..dc2c05f 100644 --- a/src/tag/commands/review-validator.ts +++ b/src/tag/commands/review-validator.ts @@ -28,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, }); @@ -36,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", @@ -47,7 +53,9 @@ 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 reviewArtifacts = await computeReviewArtifacts({ diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index f5292ab..9b77f45 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -124,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 ? [] 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 index 5d6628a..3a3b2ae 100644 --- a/test/github/data/review-artifacts.test.ts +++ b/test/github/data/review-artifacts.test.ts @@ -1,11 +1,4 @@ -import { - afterEach, - beforeEach, - describe, - expect, - it, - spyOn, -} from "bun:test"; +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; import { computeAndStoreDiff, fetchAndStoreComments, @@ -33,15 +26,15 @@ describe("review-artifacts", () => { 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, - ); + 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"); @@ -58,17 +51,18 @@ describe("review-artifacts", () => { }); 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, - ); + 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", @@ -84,17 +78,17 @@ describe("review-artifacts", () => { }); 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, + 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", ); - - await expect( - computeAndStoreDiff("main", "/tmp/test"), - ).rejects.toThrow("no fallback credentials"); }); }); @@ -170,14 +164,14 @@ describe("review-artifacts", () => { 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, - ); + 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: { 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 56cccf8..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); From ff964c39b6dd94f1a94750c9c3cd1c246269d479 Mon Sep 17 00:00:00 2001 From: User Date: Thu, 19 Feb 2026 00:30:50 -0800 Subject: [PATCH 10/13] add model and reasoning effort --- review/action.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/review/action.yml b/review/action.yml index 4a43f14..d4a6bad 100644 --- a/review/action.yml +++ b/review/action.yml @@ -122,6 +122,8 @@ runs: 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 From 84c63a3a6f8e822eb1a21c6cdf657b590e05bfa1 Mon Sep 17 00:00:00 2001 From: User Date: Thu, 19 Feb 2026 10:31:36 -0800 Subject: [PATCH 11/13] refactor: remove combine action and related prompt generation files --- .github/workflows/droid-review.yml | 40 ---- combine/action.yml | 81 ------- src/create-prompt/templates/combine-prompt.ts | 101 --------- src/create-prompt/templates/review-prompt.ts | 2 +- src/entrypoints/combine-reviews.ts | 213 ------------------ src/entrypoints/generate-combine-prompt.ts | 108 --------- .../templates/review-prompt.test.ts | 2 +- 7 files changed, 2 insertions(+), 545 deletions(-) delete mode 100644 combine/action.yml delete mode 100644 src/create-prompt/templates/combine-prompt.ts delete mode 100644 src/entrypoints/combine-reviews.ts delete mode 100644 src/entrypoints/generate-combine-prompt.ts diff --git a/.github/workflows/droid-review.yml b/.github/workflows/droid-review.yml index f2e3af7..665e000 100644 --- a/.github/workflows/droid-review.yml +++ b/.github/workflows/droid-review.yml @@ -54,13 +54,6 @@ jobs: tracking_comment_id: ${{ needs.prepare.outputs.comment_id }} output_file: ${{ runner.temp }}/code-review-results.json - - name: Upload Results - uses: actions/upload-artifact@v4 - with: - name: code-review-results - path: ${{ runner.temp }}/code-review-results.json - if-no-files-found: ignore - - name: Upload debug artifacts if: always() uses: actions/upload-artifact@v4 @@ -71,36 +64,3 @@ jobs: ${{ runner.temp }}/droid-prompts/** if-no-files-found: ignore retention-days: 7 - - combine: - needs: [prepare, code-review] - if: | - always() && - needs.prepare.outputs.run_code_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: Download Code Review Results - uses: actions/download-artifact@v4 - with: - name: code-review-results - path: ${{ runner.temp }} - continue-on-error: true - - - name: Combine Results - 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 - code_review_status: ${{ needs.code-review.result }} diff --git a/combine/action.yml b/combine/action.yml deleted file mode 100644 index e35eabc..0000000 --- a/combine/action.yml +++ /dev/null @@ -1,81 +0,0 @@ -name: "Droid Combine Results" -description: "Combine code review and security review results, post inline comments, update summary" - -inputs: - factory_api_key: - description: "Factory API key" - required: true - tracking_comment_id: - description: "ID of the tracking comment to update" - required: true - code_review_results: - description: "Path to code review results JSON (optional)" - required: false - default: "" - security_results: - description: "Path to security results JSON (optional)" - required: false - default: "" - code_review_status: - description: "Code review job status (success/failure/skipped)" - required: false - default: "skipped" - security_review_status: - description: "Security review job status (success/failure/skipped)" - required: false - default: "skipped" - -runs: - using: "composite" - steps: - - name: Install Bun - uses: oven-sh/setup-bun@735343b667d3e6f658f44d0eca948eb6282f2b76 - with: - bun-version: 1.2.11 - - - name: Install Dependencies - shell: bash - run: | - cd ${{ github.action_path }}/.. - bun install - cd ${{ github.action_path }}/../base-action - bun install - - - name: Install Droid CLI - shell: bash - run: | - curl --retry 5 --retry-delay 2 --retry-all-errors -fsSL https://app.factory.ai/cli | sh - echo "$HOME/.local/bin" >> "$GITHUB_PATH" - "$HOME/.local/bin/droid" --version - - - name: Get GitHub Token - id: token - shell: bash - run: | - bun run ${{ github.action_path }}/../src/entrypoints/get-token.ts - env: - FACTORY_API_KEY: ${{ inputs.factory_api_key }} - - - name: Generate Combine Prompt - id: prompt - shell: bash - run: | - bun run ${{ github.action_path }}/../src/entrypoints/generate-combine-prompt.ts - env: - GITHUB_TOKEN: ${{ steps.token.outputs.github_token }} - DROID_COMMENT_ID: ${{ inputs.tracking_comment_id }} - CODE_REVIEW_RESULTS: ${{ inputs.code_review_results }} - SECURITY_RESULTS: ${{ inputs.security_results }} - CODE_REVIEW_STATUS: ${{ inputs.code_review_status }} - SECURITY_REVIEW_STATUS: ${{ inputs.security_review_status }} - - - name: Run Combine - 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.prompt.outputs.droid_args }} - INPUT_MCP_TOOLS: ${{ steps.prompt.outputs.mcp_tools }} - FACTORY_API_KEY: ${{ inputs.factory_api_key }} - GITHUB_TOKEN: ${{ steps.token.outputs.github_token }} diff --git a/src/create-prompt/templates/combine-prompt.ts b/src/create-prompt/templates/combine-prompt.ts deleted file mode 100644 index 73d2de5..0000000 --- a/src/create-prompt/templates/combine-prompt.ts +++ /dev/null @@ -1,101 +0,0 @@ -import type { PreparedContext } from "../types"; - -export function generateCombinePrompt( - context: PreparedContext, - codeReviewResultsPath: string, - securityResultsPath: string, -): string { - const prNumber = context.eventData.isPR - ? context.eventData.prNumber - : context.githubContext && "entityNumber" in context.githubContext - ? String(context.githubContext.entityNumber) - : "unknown"; - - const repoFullName = context.repository; - - return `You are combining code review and security review results for PR #${prNumber} in ${repoFullName}. -The gh CLI is installed and authenticated via GH_TOKEN. - -## Context -- Repo: ${repoFullName} -- PR Number: ${prNumber} -- Code Review Results: ${codeReviewResultsPath} -- Security Review Results: ${securityResultsPath} - -## Task - -1. Read the results files (if they exist): - - ${codeReviewResultsPath} - Code review findings - - ${securityResultsPath} - Security review findings - -2. Combine and deduplicate findings: - - Merge findings from both reviews - - Remove duplicates (same file + line + similar description) - - Prioritize security findings over code review findings for overlaps - -3. Post inline comments for all unique findings using github_inline_comment___create_inline_comment: - - Use side="RIGHT" for new/modified code - - Include severity, description, and suggested fix where available - - For security findings, include CWE reference - -4. Analyze the PR diff to generate: - - A concise 1-2 sentence summary of what the PR does - - 3-5 key changes extracted from the diff - - The most important files changed (up to 5-7 files) - -5. Update the tracking comment with combined summary using github_comment___update_droid_comment: - -IMPORTANT: Do NOT use github_pr___submit_review. Only update the tracking comment and post inline comments. -The tracking comment IS the summary - do not create any other summary comments. - -\`\`\`markdown -## Code review completed - -### Summary -{Brief 1-2 sentence description of what this PR does} - -### Key Changes -- {Change 1} -- {Change 2} -- {Change 3} - -### Important Files Changed -- \`path/to/file1.ts\` - {Brief description of changes} -- \`path/to/file2.ts\` - {Brief description of changes} - -### Code Review -| Type | Count | -|------|-------| -| πŸ› Bugs | X | -| ⚠️ Issues | X | -| πŸ’‘ Suggestions | X | - -### Security Review -| Severity | Count | -|----------|-------| -| 🚨 CRITICAL | X | -| πŸ”΄ HIGH | X | -| 🟑 MEDIUM | X | -| 🟒 LOW | X | - -### Findings -| ID | Type | Severity | File | Description | -|----|------|----------|------|-------------| -| ... | ... | ... | ... | ... | - -[View workflow run](link) -\`\`\` - -## Available Tools -- github_comment___update_droid_comment - Update tracking comment (this is the ONLY place for the summary) -- github_inline_comment___create_inline_comment - Post inline comments on specific lines -- Read, Grep, Glob, LS, Execute - File operations - -DO NOT use github_pr___submit_review - it creates duplicate summary comments. - -## Important -- If no results files exist or they're empty, report "No issues found" -- Maximum 10 inline comments total -- Deduplicate findings that appear in both reviews -`; -} diff --git a/src/create-prompt/templates/review-prompt.ts b/src/create-prompt/templates/review-prompt.ts index 6462eb0..5863ab2 100644 --- a/src/create-prompt/templates/review-prompt.ts +++ b/src/create-prompt/templates/review-prompt.ts @@ -382,7 +382,7 @@ After completing your review, you MUST write your findings to \`${context.output 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. +This file is required for downstream processing of review results. ` : "" }`; diff --git a/src/entrypoints/combine-reviews.ts b/src/entrypoints/combine-reviews.ts deleted file mode 100644 index fe8933f..0000000 --- a/src/entrypoints/combine-reviews.ts +++ /dev/null @@ -1,213 +0,0 @@ -#!/usr/bin/env bun - -/** - * Combine results from code review and security review into a single summary - * Updates the tracking comment with the combined results - */ - -import * as core from "@actions/core"; -import { readFile } from "fs/promises"; -import { createOctokit } from "../github/api/client"; -import { parseGitHubContext } from "../github/context"; -import { GITHUB_SERVER_URL } from "../github/api/config"; - -interface ReviewFinding { - id: string; - type: string; - severity?: string; - file: string; - line: number; - description: string; - cwe?: string; -} - -interface ReviewResults { - type: "code" | "security"; - findings: ReviewFinding[]; - summary?: string; -} - -async function loadResults(filePath: string): Promise { - if (!filePath || filePath === "") { - return null; - } - - try { - const content = await readFile(filePath, "utf-8"); - return JSON.parse(content); - } catch (error) { - console.warn(`Could not load results from ${filePath}:`, error); - return null; - } -} - -function generateCombinedSummary( - codeResults: ReviewResults | null, - securityResults: ReviewResults | null, - codeStatus: string, - securityStatus: string, - jobUrl: string, -): string { - const sections: string[] = []; - - // Header - sections.push("## πŸ” PR Review Summary\n"); - - // Status overview - const statusTable = ["| Review Type | Status |", "|-------------|--------|"]; - - if (codeStatus !== "skipped") { - const codeIcon = codeStatus === "success" ? "βœ…" : "❌"; - statusTable.push(`| Code Review | ${codeIcon} ${codeStatus} |`); - } - - if (securityStatus !== "skipped") { - const securityIcon = securityStatus === "success" ? "βœ…" : "❌"; - statusTable.push(`| Security Review | ${securityIcon} ${securityStatus} |`); - } - - if (statusTable.length > 2) { - sections.push(statusTable.join("\n")); - sections.push(""); - } - - // Code Review Section - if (codeResults && codeResults.findings.length > 0) { - sections.push("### πŸ“ Code Review Findings\n"); - sections.push("| ID | Type | File | Line | Description |"); - sections.push("|----|------|------|------|-------------|"); - - for (const finding of codeResults.findings.slice(0, 10)) { - sections.push( - `| ${finding.id} | ${finding.type} | \`${finding.file}\` | ${finding.line} | ${finding.description} |`, - ); - } - - if (codeResults.findings.length > 10) { - sections.push( - `\n*...and ${codeResults.findings.length - 10} more findings*`, - ); - } - sections.push(""); - } else if (codeStatus === "success") { - sections.push("### πŸ“ Code Review\n"); - sections.push("βœ… No code quality issues found.\n"); - } - - // Security Review Section - if (securityResults && securityResults.findings.length > 0) { - sections.push("### πŸ” Security Review Findings\n"); - - // Severity counts - const severityCounts = { - CRITICAL: 0, - HIGH: 0, - MEDIUM: 0, - LOW: 0, - }; - - for (const finding of securityResults.findings) { - const sev = (finding.severity?.toUpperCase() || - "MEDIUM") as keyof typeof severityCounts; - if (sev in severityCounts) { - severityCounts[sev]++; - } - } - - sections.push("| Severity | Count |"); - sections.push("|----------|-------|"); - if (severityCounts.CRITICAL > 0) - sections.push(`| 🚨 CRITICAL | ${severityCounts.CRITICAL} |`); - if (severityCounts.HIGH > 0) - sections.push(`| πŸ”΄ HIGH | ${severityCounts.HIGH} |`); - if (severityCounts.MEDIUM > 0) - sections.push(`| 🟑 MEDIUM | ${severityCounts.MEDIUM} |`); - if (severityCounts.LOW > 0) - sections.push(`| 🟒 LOW | ${severityCounts.LOW} |`); - sections.push(""); - - // Findings table - sections.push("| ID | Severity | Type | File | Line | Reference |"); - sections.push("|----|----------|------|------|------|-----------|"); - - for (const finding of securityResults.findings.slice(0, 10)) { - const cweLink = finding.cwe - ? `[${finding.cwe}](https://cwe.mitre.org/data/definitions/${finding.cwe.replace("CWE-", "")}.html)` - : "-"; - sections.push( - `| ${finding.id} | ${finding.severity || "MEDIUM"} | ${finding.type} | \`${finding.file}\` | ${finding.line} | ${cweLink} |`, - ); - } - - if (securityResults.findings.length > 10) { - sections.push( - `\n*...and ${securityResults.findings.length - 10} more findings*`, - ); - } - sections.push(""); - } else if (securityStatus === "success") { - sections.push("### πŸ” Security Review\n"); - sections.push("βœ… No security vulnerabilities found.\n"); - } - - // Footer with job link - sections.push(`---\n[View job run](${jobUrl})`); - - return sections.join("\n"); -} - -async function run() { - try { - const githubToken = process.env.GITHUB_TOKEN!; - const commentId = parseInt(process.env.DROID_COMMENT_ID || "0"); - const codeResultsPath = process.env.CODE_REVIEW_RESULTS || ""; - const securityResultsPath = process.env.SECURITY_RESULTS || ""; - const codeStatus = process.env.CODE_REVIEW_STATUS || "skipped"; - const securityStatus = process.env.SECURITY_REVIEW_STATUS || "skipped"; - const runId = process.env.GITHUB_RUN_ID || ""; - - if (!commentId) { - throw new Error("DROID_COMMENT_ID is required"); - } - - const context = parseGitHubContext(); - const { owner, repo } = context.repository; - const octokit = createOctokit(githubToken); - - // Load results from artifacts - const codeResults = await loadResults(codeResultsPath); - const securityResults = await loadResults(securityResultsPath); - - // Generate job URL - const jobUrl = `${GITHUB_SERVER_URL}/${owner}/${repo}/actions/runs/${runId}`; - - // Generate combined summary - const summary = generateCombinedSummary( - codeResults, - securityResults, - codeStatus, - securityStatus, - jobUrl, - ); - - // Update the tracking comment - await octokit.rest.issues.updateComment({ - owner, - repo, - comment_id: commentId, - body: summary, - }); - - console.log( - `βœ… Updated tracking comment ${commentId} with combined summary`, - ); - } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); - core.setFailed(`Combine reviews failed: ${errorMessage}`); - process.exit(1); - } -} - -if (import.meta.main) { - run(); -} diff --git a/src/entrypoints/generate-combine-prompt.ts b/src/entrypoints/generate-combine-prompt.ts deleted file mode 100644 index 232e135..0000000 --- a/src/entrypoints/generate-combine-prompt.ts +++ /dev/null @@ -1,108 +0,0 @@ -#!/usr/bin/env bun - -/** - * Generate combine prompt for finalizing parallel reviews - */ - -import * as core from "@actions/core"; -import { createOctokit } from "../github/api/client"; -import { parseGitHubContext, isEntityContext } from "../github/context"; -import { fetchPRBranchData } from "../github/data/pr-fetcher"; -import { createPrompt } from "../create-prompt"; -import { prepareMcpTools } from "../mcp/install-mcp-server"; -import { generateCombinePrompt } from "../create-prompt/templates/combine-prompt"; -import { normalizeDroidArgs, parseAllowedTools } from "../utils/parse-tools"; - -async function run() { - try { - const githubToken = process.env.GITHUB_TOKEN!; - const commentId = parseInt(process.env.DROID_COMMENT_ID || "0"); - const codeReviewResults = process.env.CODE_REVIEW_RESULTS || ""; - const securityResults = process.env.SECURITY_RESULTS || ""; - - const context = parseGitHubContext(); - - if (!isEntityContext(context)) { - throw new Error("Combine requires entity context (PR or issue)"); - } - - if (!context.isPR) { - throw new Error("Combine is only supported on pull requests"); - } - - const octokit = createOctokit(githubToken); - - const prData = await fetchPRBranchData({ - octokits: octokit, - repository: context.repository, - prNumber: context.entityNumber, - }); - - // Generate combine prompt with paths to result files - await createPrompt({ - githubContext: context, - commentId, - baseBranch: prData.baseRefName, - prBranchData: { - headRefName: prData.headRefName, - headRefOid: prData.headRefOid, - }, - generatePrompt: (ctx) => - generateCombinePrompt(ctx, codeReviewResults, securityResults), - }); - - core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-combine"); - - const rawUserArgs = process.env.DROID_ARGS || ""; - const normalizedUserArgs = normalizeDroidArgs(rawUserArgs); - const userAllowedMCPTools = parseAllowedTools(normalizedUserArgs).filter( - (tool) => tool.startsWith("github_") && tool.includes("___"), - ); - - // Combine step has tools for inline comments and tracking comment update - // NO github_pr___submit_review - it creates duplicate summary comments - const baseTools = [ - "Read", - "Grep", - "Glob", - "LS", - "Execute", - "github_comment___update_droid_comment", - "github_inline_comment___create_inline_comment", - ]; - - const allowedTools = Array.from( - new Set([...baseTools, ...userAllowedMCPTools]), - ); - - const mcpTools = await prepareMcpTools({ - githubToken, - owner: context.repository.owner, - repo: context.repository.repo, - droidCommentId: commentId.toString(), - allowedTools, - mode: "tag", - context, - }); - - const droidArgParts: string[] = []; - droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); - - if (normalizedUserArgs) { - droidArgParts.push(normalizedUserArgs); - } - - core.setOutput("droid_args", droidArgParts.join(" ").trim()); - core.setOutput("mcp_tools", mcpTools); - - console.log(`Generated combine prompt`); - } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); - core.setFailed(`Generate combine prompt failed: ${errorMessage}`); - process.exit(1); - } -} - -if (import.meta.main) { - run(); -} diff --git a/test/create-prompt/templates/review-prompt.test.ts b/test/create-prompt/templates/review-prompt.test.ts index 72b7760..3898c6b 100644 --- a/test/create-prompt/templates/review-prompt.test.ts +++ b/test/create-prompt/templates/review-prompt.test.ts @@ -166,7 +166,7 @@ describe("generateReviewPrompt", () => { 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"); + expect(prompt).toContain("downstream processing of review results"); }); it("does not include output file section when outputFilePath is not set", () => { From 6be386918551e80780b65ba5f2a7287114e8ba15 Mon Sep 17 00:00:00 2001 From: User Date: Thu, 19 Feb 2026 10:41:22 -0800 Subject: [PATCH 12/13] add --tag flags to droid exec calls --- src/entrypoints/generate-review-prompt.ts | 1 + src/tag/commands/fill.ts | 1 + src/tag/commands/review-validator.ts | 1 + src/tag/commands/review.ts | 1 + 4 files changed, 4 insertions(+) diff --git a/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 03b989f..0db78d0 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -182,6 +182,7 @@ async function run() { const droidArgParts: string[] = []; droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); + droidArgParts.push('--tag "code-review"'); const reviewModel = reviewType === "security" diff --git a/src/tag/commands/fill.ts b/src/tag/commands/fill.ts index 1cc7e31..3ae1a5a 100644 --- a/src/tag/commands/fill.ts +++ b/src/tag/commands/fill.ts @@ -91,6 +91,7 @@ export async function prepareFillMode({ const droidArgParts: string[] = []; droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); + droidArgParts.push('--tag "droid-fill"'); // Add model override if specified const fillModel = process.env.FILL_MODEL?.trim(); diff --git a/src/tag/commands/review-validator.ts b/src/tag/commands/review-validator.ts index dc2c05f..6da2d3f 100644 --- a/src/tag/commands/review-validator.ts +++ b/src/tag/commands/review-validator.ts @@ -122,6 +122,7 @@ export async function prepareReviewValidatorMode({ const droidArgParts: string[] = []; droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); + droidArgParts.push('--tag "code-review"'); const reviewModel = process.env.REVIEW_MODEL?.trim(); const reasoningEffort = process.env.REASONING_EFFORT?.trim(); diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index 9b77f45..4fabb33 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -170,6 +170,7 @@ export async function prepareReviewMode({ const droidArgParts: string[] = []; droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); + droidArgParts.push('--tag "code-review"'); const reviewModel = process.env.REVIEW_MODEL?.trim(); const reasoningEffort = process.env.REASONING_EFFORT?.trim(); From 28ea2b4542bf891ba4509b733aadb76f39bde063 Mon Sep 17 00:00:00 2001 From: User Date: Thu, 19 Feb 2026 10:53:07 -0800 Subject: [PATCH 13/13] fix: reuse pre-computed review artifacts in validator step instead of redundant checkout --- src/tag/commands/review-validator.ts | 45 +++++++--------------------- 1 file changed, 10 insertions(+), 35 deletions(-) diff --git a/src/tag/commands/review-validator.ts b/src/tag/commands/review-validator.ts index 6da2d3f..4e9148d 100644 --- a/src/tag/commands/review-validator.ts +++ b/src/tag/commands/review-validator.ts @@ -3,13 +3,12 @@ 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/types"; 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"; export async function prepareReviewValidatorMode({ context, @@ -35,40 +34,16 @@ export async function prepareReviewValidatorMode({ prNumber: context.entityNumber, }); + // The PR branch is already checked out and review artifacts (diff, + // comments, description) were already computed by the generate-review-prompt + // step earlier in this job. Reuse them from disk instead of recomputing. const tempDir = process.env.RUNNER_TEMP || "/tmp"; - - // Ensure PR branch is checked out (same as review mode) - try { - execSync("git reset --hard HEAD", { - encoding: "utf8", - stdio: "pipe", - } as any); - 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" } as any).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, - }); + const promptsDir = `${tempDir}/droid-prompts`; + const reviewArtifacts: ReviewArtifacts = { + diffPath: `${promptsDir}/pr.diff`, + commentsPath: `${promptsDir}/existing_comments.json`, + descriptionPath: `${promptsDir}/pr_description.txt`, + }; await createPrompt({ githubContext: context,