From 8b8cd1d3136ff0ce8783cadda6c2e51427d31ac5 Mon Sep 17 00:00:00 2001 From: Nizar Alrifai Date: Tue, 10 Mar 2026 15:25:33 -0700 Subject: [PATCH 1/2] feat: add @droid fix command for PR code fixes Add a new 'fix' command that triggers Droid to commit actual code changes when users @tag droid in PR comments. Includes: - Command parser detection for '@droid fix' - Fix prompt template with comment hierarchy logic: - Top-level PR comment (issue_comment): fix all review findings in scope - Review thread reply (pull_request_review_comment): fix specific issue - Fix command handler with PR branch checkout, file-editing tools, and git commit/push capability - fix_model action input for model override - Full test coverage for parser, command handler, and prompt generation Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- action.yml | 5 + bun.lock | 1 + src/create-prompt/index.ts | 14 +- src/create-prompt/templates/fix-prompt.ts | 145 +++++++++ src/create-prompt/types.ts | 7 + src/github/utils/command-parser.test.ts | 57 ++++ src/github/utils/command-parser.ts | 11 + src/mcp/github-pr-server.ts | 7 +- src/tag/commands/fix.ts | 189 ++++++++++++ src/tag/index.ts | 12 + .../templates/fix-prompt.test.ts | 130 ++++++++ test/modes/tag/fix-command.test.ts | 277 ++++++++++++++++++ 12 files changed, 853 insertions(+), 2 deletions(-) create mode 100644 src/create-prompt/templates/fix-prompt.ts create mode 100644 src/tag/commands/fix.ts create mode 100644 test/create-prompt/templates/fix-prompt.test.ts create mode 100644 test/modes/tag/fix-command.test.ts diff --git a/action.yml b/action.yml index ef1d568..6580a85 100644 --- a/action.yml +++ b/action.yml @@ -111,6 +111,10 @@ inputs: description: "Override the model used for PR description fill (e.g., 'claude-sonnet-4-5-20250929', 'gpt-5.1-codex'). Only applies to fill flows." required: false default: "" + fix_model: + description: "Override the model used for code fix (e.g., 'claude-sonnet-4-5-20250929', 'gpt-5.1-codex'). Only applies to @droid fix flows." + required: false + default: "" experimental_allowed_domains: description: "Restrict network access to these domains only (newline-separated). If not set, no restrictions are applied. Provider domains are auto-detected." required: false @@ -188,6 +192,7 @@ runs: REVIEW_CANDIDATES_PATH: ${{ inputs.review_candidates_path }} REVIEW_VALIDATED_PATH: ${{ inputs.review_validated_path }} FILL_MODEL: ${{ inputs.fill_model }} + FIX_MODEL: ${{ inputs.fix_model }} ADDITIONAL_PERMISSIONS: ${{ inputs.additional_permissions }} DROID_ARGS: ${{ inputs.droid_args }} ALL_INPUTS: ${{ toJson(inputs) }} diff --git a/bun.lock b/bun.lock index b0a562a..cbb05c9 100644 --- a/bun.lock +++ b/bun.lock @@ -1,5 +1,6 @@ { "lockfileVersion": 1, + "configVersion": 0, "workspaces": { "": { "name": "@Factory-AI/droid-action", diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index 415013f..0032afe 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -20,9 +20,15 @@ import type { PreparedContext, EventData, ReviewArtifacts, + FixContext, } from "./types"; -export type { CommonFields, PreparedContext, ReviewArtifacts } from "./types"; +export type { + CommonFields, + PreparedContext, + ReviewArtifacts, + FixContext, +} from "./types"; const BASE_ALLOWED_TOOLS = [ "Execute", @@ -303,6 +309,7 @@ export type PromptCreationOptions = { disallowedTools?: string[]; includeActionsTools?: boolean; reviewArtifacts?: ReviewArtifacts; + fixContext?: FixContext; outputFilePath?: string; }; @@ -317,6 +324,7 @@ export async function createPrompt({ disallowedTools = [], includeActionsTools = false, reviewArtifacts, + fixContext, outputFilePath, }: PromptCreationOptions) { try { @@ -330,6 +338,10 @@ export async function createPrompt({ reviewArtifacts, ); + if (fixContext) { + preparedContext.fixContext = fixContext; + } + if (outputFilePath) { preparedContext.outputFilePath = outputFilePath; } diff --git a/src/create-prompt/templates/fix-prompt.ts b/src/create-prompt/templates/fix-prompt.ts new file mode 100644 index 0000000..d40ddca --- /dev/null +++ b/src/create-prompt/templates/fix-prompt.ts @@ -0,0 +1,145 @@ +import type { PreparedContext, FixContext } from "../types"; + +export function generateFixPrompt(context: PreparedContext): string { + const prNumber = context.eventData.isPR + ? context.eventData.prNumber + : "unknown"; + + const repoFullName = context.repository; + const commentBody = + "commentBody" in context.eventData ? context.eventData.commentBody : ""; + + const userInstructions = extractUserInstructions(commentBody); + + if (context.eventData.eventName === "pull_request_review_comment") { + return generateThreadFixPrompt({ + prNumber, + repoFullName, + commentBody, + userInstructions, + reviewCommentContext: context.fixContext, + }); + } + + return generateTopLevelFixPrompt({ + prNumber, + repoFullName, + commentBody, + userInstructions, + }); +} + +function extractUserInstructions(commentBody: string): string { + const cleaned = commentBody.replace(/@droid\s+fix/i, "").trim(); + return cleaned || ""; +} + +type ThreadFixPromptOptions = { + prNumber: string; + repoFullName: string; + commentBody: string; + userInstructions: string; + reviewCommentContext?: FixContext; +}; + +function generateThreadFixPrompt({ + prNumber, + repoFullName, + userInstructions, + reviewCommentContext, +}: ThreadFixPromptOptions): string { + const filePath = reviewCommentContext?.filePath ?? "unknown"; + const line = reviewCommentContext?.line; + const parentBody = reviewCommentContext?.parentCommentBody ?? ""; + + const lineContext = line ? ` around line ${line}` : ""; + + return `You are fixing a specific code review issue on PR #${prNumber} in ${repoFullName}. +The gh CLI is installed and authenticated via GH_TOKEN. + +## Review Issue to Fix + +The following review comment identified an issue in \`${filePath}\`${lineContext}: + +\`\`\` +${parentBody} +\`\`\` +${userInstructions ? `\n## Additional Instructions from User\n\n${userInstructions}\n` : ""} +## Procedure + +1. **Understand the issue**: Read the review comment above carefully. Read the file at the specified path to understand the surrounding code context. +2. **Check the PR diff** for additional context: + - Run: \`gh pr diff ${prNumber} --repo ${repoFullName}\` +3. **Implement the fix**: Edit the file(s) to resolve the issue identified in the review comment. +4. **Verify the fix**: + - Read the modified file(s) to confirm correctness. + - If the project has tests, try running them: look for test scripts in package.json, Makefile, or similar config files. +5. **Commit and push**: + - Run: \`git add -A\` + - Run: \`git commit -m "fix: address review comment in ${filePath}"\` + - Run: \`git push\` + +## Rules + +- Only fix the specific issue mentioned in the review comment. Do not make unrelated changes. +- Keep changes minimal and focused. +- Follow the existing code style and conventions in the repository. +- If you cannot determine the correct fix with confidence, explain what you found and suggest a fix in a comment instead of making a wrong change. +- Never introduce new lint errors or break existing tests. +- Update the tracking comment with progress using the github_comment___update_droid_comment tool. +`; +} + +type TopLevelFixPromptOptions = { + prNumber: string; + repoFullName: string; + commentBody: string; + userInstructions: string; +}; + +function generateTopLevelFixPrompt({ + prNumber, + repoFullName, + userInstructions, +}: TopLevelFixPromptOptions): string { + return `You are fixing code issues on PR #${prNumber} in ${repoFullName}. +The gh CLI is installed and authenticated via GH_TOKEN. + +## Procedure + +1. **Gather context**: + - Run: \`gh pr view ${prNumber} --repo ${repoFullName} --json title,body\` + - Run: \`gh pr view ${prNumber} --repo ${repoFullName} --json comments,reviews\` + - Run: \`gh pr diff ${prNumber} --repo ${repoFullName}\` + - Run: \`gh api repos/${repoFullName}/pulls/${prNumber}/reviews --paginate --jq '.[] | {user: .user.login, state: .state, body: .body}'\` + - Run: \`gh api repos/${repoFullName}/pulls/${prNumber}/comments --paginate --jq '.[] | {path: .path, line: .line, body: .body, user: .user.login}'\` + +2. **Identify issues to fix**: + - Review all review comments and feedback on the PR. + - ${userInstructions ? `The user specifically asked: "${userInstructions}". Prioritize these instructions.` : "Identify all actionable review findings that require code changes."} + - Categorize issues by file and severity. + +3. **Implement fixes**: + - Address each identified issue systematically, file by file. + - Read each file before editing to understand the full context. + - Make minimal, focused changes that directly address the review feedback. + +4. **Verify fixes**: + - Read modified files to confirm correctness. + - If the project has tests, try running them: look for test scripts in package.json, Makefile, or similar config files. + - Check for lint/format scripts and run them if available. + +5. **Commit and push**: + - Run: \`git add -A\` + - Run: \`git commit -m "fix: address review feedback on PR #${prNumber}"\` + - Run: \`git push\` + +## Rules + +- Follow the existing code style and conventions in the repository. +- Keep changes focused on addressing review feedback. Do not refactor unrelated code. +- If a review comment is unclear or you cannot determine the correct fix, skip it and note it in the tracking comment. +- Never introduce new lint errors or break existing tests. +- Update the tracking comment with progress using the github_comment___update_droid_comment tool. +`; +} diff --git a/src/create-prompt/types.ts b/src/create-prompt/types.ts index cf110c3..54d6938 100644 --- a/src/create-prompt/types.ts +++ b/src/create-prompt/types.ts @@ -109,6 +109,12 @@ export type ReviewArtifacts = { descriptionPath: string; }; +export type FixContext = { + parentCommentBody: string; + filePath: string; + line: number | null; +}; + export type PreparedContext = CommonFields & { eventData: EventData; githubContext?: GitHubContext; @@ -117,5 +123,6 @@ export type PreparedContext = CommonFields & { headRefOid: string; }; reviewArtifacts?: ReviewArtifacts; + fixContext?: FixContext; outputFilePath?: string; }; diff --git a/src/github/utils/command-parser.test.ts b/src/github/utils/command-parser.test.ts index 8d30c0f..e2892d8 100644 --- a/src/github/utils/command-parser.test.ts +++ b/src/github/utils/command-parser.test.ts @@ -85,6 +85,30 @@ describe("Command Parser", () => { expect(result?.raw).toBe("@droid review"); }); + it("should detect @droid fix", () => { + const result = parseDroidCommand("@droid fix this bug"); + expect(result?.command).toBe("fix"); + expect(result?.raw).toBe("@droid fix"); + }); + + it("should be case insensitive for @droid fix", () => { + const result = parseDroidCommand("@DROID FIX the issue"); + expect(result?.command).toBe("fix"); + expect(result?.raw).toBe("@DROID FIX"); + }); + + it("should detect @droid fix with extra spaces", () => { + const result = parseDroidCommand("@droid fix"); + expect(result?.command).toBe("fix"); + expect(result?.raw).toBe("@droid fix"); + }); + + it("should detect standalone @droid fix", () => { + const result = parseDroidCommand("@droid fix"); + expect(result?.command).toBe("fix"); + expect(result?.raw).toBe("@droid fix"); + }); + it("should parse @droid security review as security", () => { const result = parseDroidCommand("@droid security review"); expect(result?.command).toBe("security"); @@ -244,6 +268,39 @@ describe("Command Parser", () => { expect(result?.timestamp).toBe("2024-01-01T00:00:00Z"); }); + it("should extract fix from PR review comment", () => { + const context = createContext("pull_request_review_comment", { + action: "created", + comment: { + body: "@droid fix this issue", + created_at: "2024-01-01T00:00:00Z", + }, + pull_request: { + number: 1, + }, + } as unknown as PullRequestReviewCommentEvent); + const result = extractCommandFromContext(context); + expect(result?.command).toBe("fix"); + expect(result?.location).toBe("comment"); + }); + + it("should extract fix from issue comment on PR", () => { + const context = createContext("issue_comment", { + action: "created", + comment: { + body: "@droid fix", + created_at: "2024-01-01T00:00:00Z", + }, + issue: { + number: 1, + pull_request: { url: "" }, + }, + } as unknown as IssueCommentEvent); + const result = extractCommandFromContext(context); + expect(result?.command).toBe("fix"); + expect(result?.location).toBe("comment"); + }); + it("should extract from PR review body", () => { const context = createContext("pull_request_review", { action: "submitted", diff --git a/src/github/utils/command-parser.ts b/src/github/utils/command-parser.ts index 5a9ca27..a894d6e 100644 --- a/src/github/utils/command-parser.ts +++ b/src/github/utils/command-parser.ts @@ -6,6 +6,7 @@ import type { GitHubContext } from "../context"; export type DroidCommand = | "fill" + | "fix" | "review" | "security" | "security-full" @@ -49,6 +50,16 @@ export function parseDroidCommand(text: string): ParsedCommand | null { }; } + // Check for @droid fix command (case insensitive) + const fixMatch = text.match(/@droid\s+fix/i); + if (fixMatch) { + return { + command: "fix", + raw: fixMatch[0], + location: "body", + }; + } + // Check for @droid security --full command (case insensitive) const securityFullMatch = text.match(/@droid\s+security\s+--full/i); if (securityFullMatch) { diff --git a/src/mcp/github-pr-server.ts b/src/mcp/github-pr-server.ts index 5a57268..d36ab1d 100644 --- a/src/mcp/github-pr-server.ts +++ b/src/mcp/github-pr-server.ts @@ -624,7 +624,12 @@ export function createGitHubPRServer({ path: z .string() .describe("The file path to comment on (e.g., 'src/index.js')"), - body: z.string().min(1).describe("The comment text (supports markdown and GitHub code suggestion blocks)"), + body: z + .string() + .min(1) + .describe( + "The comment text (supports markdown and GitHub code suggestion blocks)", + ), line: z .number() .int() diff --git a/src/tag/commands/fix.ts b/src/tag/commands/fix.ts new file mode 100644 index 0000000..9bac58c --- /dev/null +++ b/src/tag/commands/fix.ts @@ -0,0 +1,189 @@ +import * as core from "@actions/core"; +import { execSync } from "child_process"; +import type { GitHubContext } from "../../github/context"; +import { + isEntityContext, + isPullRequestReviewCommentEvent, +} from "../../github/context"; +import { fetchPRBranchData } from "../../github/data/pr-fetcher"; +import { createPrompt } from "../../create-prompt"; +import type { FixContext } from "../../create-prompt/types"; +import { generateFixPrompt } from "../../create-prompt/templates/fix-prompt"; +import { prepareMcpTools } from "../../mcp/install-mcp-server"; +import { createInitialComment } from "../../github/operations/comments/create-initial"; +import { normalizeDroidArgs, parseAllowedTools } from "../../utils/parse-tools"; +import type { Octokits } from "../../github/api/client"; +import type { PrepareResult } from "../../prepare/types"; + +type FixCommandOptions = { + context: GitHubContext; + octokit: Octokits; + githubToken: string; + trackingCommentId?: number; +}; + +export async function prepareFixMode({ + context, + octokit, + githubToken, + trackingCommentId, +}: FixCommandOptions): Promise { + if (!isEntityContext(context)) { + throw new Error("Fix command requires an entity event context"); + } + + if (!context.isPR) { + throw new Error("Fix command is only supported on pull requests"); + } + + const commentId = + trackingCommentId ?? (await createInitialComment(octokit.rest, context)).id; + + const prData = await fetchPRBranchData({ + octokits: octokit, + repository: context.repository, + prNumber: context.entityNumber, + }); + + const branchInfo = { + baseBranch: prData.baseRefName, + droidBranch: undefined, + currentBranch: prData.headRefName, + }; + + // Checkout the PR branch so Droid can commit and push fixes + console.log(`Checking out PR #${context.entityNumber} branch for fix...`); + try { + execSync("git reset --hard HEAD", { encoding: "utf8", stdio: "pipe" }); + execSync(`gh pr checkout ${context.entityNumber}`, { + encoding: "utf8", + stdio: "pipe", + env: { ...process.env, GH_TOKEN: githubToken }, + }); + console.log( + `Successfully checked out PR branch: ${execSync("git rev-parse --abbrev-ref HEAD", { encoding: "utf8" }).trim()}`, + ); + } catch (e) { + console.error(`Failed to checkout PR branch: ${e}`); + throw new Error( + `Failed to checkout PR #${context.entityNumber} branch for fix`, + ); + } + + // Build fix context for review thread replies + const fixContext = await buildFixContext(context, octokit); + + await createPrompt({ + githubContext: context, + commentId, + baseBranch: branchInfo.baseBranch, + droidBranch: branchInfo.droidBranch, + prBranchData: { + headRefName: prData.headRefName, + headRefOid: prData.headRefOid, + }, + generatePrompt: generateFixPrompt, + fixContext, + }); + core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-fix"); + + const rawUserArgs = process.env.DROID_ARGS || ""; + const normalizedUserArgs = normalizeDroidArgs(rawUserArgs); + const userAllowedMCPTools = parseAllowedTools(normalizedUserArgs).filter( + (tool) => tool.startsWith("github_") && tool.includes("___"), + ); + + const baseTools = [ + "Read", + "Grep", + "Glob", + "LS", + "Execute", + "Edit", + "Create", + "ApplyPatch", + "FetchUrl", + "github_comment___update_droid_comment", + ]; + + const safeUserAllowedMCPTools = userAllowedMCPTools.filter( + (tool) => + tool === "github_comment___update_droid_comment" || + !tool.startsWith("github_pr___"), + ); + + const allowedTools = Array.from( + new Set([...baseTools, ...safeUserAllowedMCPTools]), + ); + + 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(",")}"`); + droidArgParts.push('--tag "droid-fix"'); + + const fixModel = process.env.FIX_MODEL?.trim(); + if (fixModel) { + droidArgParts.push(`--model "${fixModel}"`); + } + + if (normalizedUserArgs) { + droidArgParts.push(normalizedUserArgs); + } + + core.setOutput("droid_args", droidArgParts.join(" ").trim()); + core.setOutput("mcp_tools", mcpTools); + + return { + commentId, + branchInfo, + mcpTools, + }; +} + +async function buildFixContext( + context: GitHubContext, + octokit: Octokits, +): Promise { + if (!isPullRequestReviewCommentEvent(context)) { + return undefined; + } + + const comment = context.payload.comment; + const filePath = comment.path || "unknown"; + const line = comment.line ?? comment.original_line ?? null; + + // If this comment is a reply in a thread, fetch the parent comment + // to get the original review finding + const inReplyToId = (comment as { in_reply_to_id?: number }).in_reply_to_id; + let parentCommentBody = comment.body || ""; + + if (inReplyToId) { + try { + const parentComment = await octokit.rest.pulls.getReviewComment({ + owner: context.repository.owner, + repo: context.repository.repo, + comment_id: inReplyToId, + }); + parentCommentBody = parentComment.data.body || comment.body || ""; + } catch (e) { + console.warn( + `Failed to fetch parent review comment ${inReplyToId}: ${e}`, + ); + } + } + + return { + parentCommentBody, + filePath, + line, + }; +} diff --git a/src/tag/index.ts b/src/tag/index.ts index f17e07d..436909c 100644 --- a/src/tag/index.ts +++ b/src/tag/index.ts @@ -5,6 +5,7 @@ import { createInitialComment } from "../github/operations/comments/create-initi import { isEntityContext, type ParsedGitHubContext } from "../github/context"; import { extractCommandFromContext } from "../github/utils/command-parser"; import { prepareFillMode } from "./commands/fill"; +import { prepareFixMode } from "./commands/fix"; import { prepareReviewMode } from "./commands/review"; import { prepareSecurityReviewMode } from "./commands/security-review"; import { prepareSecurityScanMode } from "./commands/security-scan"; @@ -184,6 +185,17 @@ export async function prepareTagExecution({ }); } + if (commandContext?.command === "fix") { + core.setOutput("run_code_review", "false"); + core.setOutput("run_security_review", "false"); + return prepareFixMode({ + context, + octokit, + githubToken, + trackingCommentId: commentId, + }); + } + if (commandContext?.command === "security") { core.setOutput("run_code_review", "false"); core.setOutput("run_security_review", "true"); diff --git a/test/create-prompt/templates/fix-prompt.test.ts b/test/create-prompt/templates/fix-prompt.test.ts new file mode 100644 index 0000000..c46a0ff --- /dev/null +++ b/test/create-prompt/templates/fix-prompt.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it } from "bun:test"; +import { generateFixPrompt } from "../../../src/create-prompt/templates/fix-prompt"; +import type { PreparedContext } from "../../../src/create-prompt/types"; + +function createBaseContext( + overrides: Partial = {}, +): PreparedContext { + return { + repository: "test-owner/test-repo", + droidCommentId: "123", + triggerPhrase: "@droid", + eventData: { + eventName: "issue_comment", + commentId: "456", + prNumber: "42", + isPR: true, + commentBody: "@droid fix", + }, + githubContext: undefined, + ...overrides, + } as PreparedContext; +} + +describe("generateFixPrompt", () => { + describe("top-level fix (issue_comment)", () => { + it("generates prompt to fix all review findings", () => { + const context = createBaseContext(); + const prompt = generateFixPrompt(context); + + expect(prompt).toContain("PR #42"); + expect(prompt).toContain("test-owner/test-repo"); + expect(prompt).toContain("gh pr diff 42"); + expect(prompt).toContain("Identify issues to fix"); + expect(prompt).toContain("git commit"); + expect(prompt).toContain("git push"); + }); + + it("includes user instructions when provided", () => { + const context = createBaseContext({ + eventData: { + eventName: "issue_comment", + commentId: "456", + prNumber: "42", + isPR: true as const, + commentBody: "@droid fix the null pointer issue in auth.ts", + }, + }); + const prompt = generateFixPrompt(context); + + expect(prompt).toContain("the null pointer issue in auth.ts"); + }); + + it("fetches review comments via gh CLI", () => { + const context = createBaseContext(); + const prompt = generateFixPrompt(context); + + expect(prompt).toContain( + "gh api repos/test-owner/test-repo/pulls/42/comments", + ); + expect(prompt).toContain( + "gh api repos/test-owner/test-repo/pulls/42/reviews", + ); + }); + }); + + describe("thread fix (pull_request_review_comment)", () => { + it("generates prompt focused on specific file and issue", () => { + const context = createBaseContext({ + eventData: { + eventName: "pull_request_review_comment", + prNumber: "42", + isPR: true as const, + commentBody: "@droid fix", + }, + fixContext: { + parentCommentBody: "This has a race condition in the mutex lock", + filePath: "src/server/handler.ts", + line: 55, + }, + }); + const prompt = generateFixPrompt(context); + + expect(prompt).toContain("src/server/handler.ts"); + expect(prompt).toContain("around line 55"); + expect(prompt).toContain("race condition in the mutex lock"); + expect(prompt).toContain( + "Only fix the specific issue mentioned in the review comment", + ); + }); + + it("handles missing line number", () => { + const context = createBaseContext({ + eventData: { + eventName: "pull_request_review_comment", + prNumber: "42", + isPR: true as const, + commentBody: "@droid fix", + }, + fixContext: { + parentCommentBody: "Missing error handling", + filePath: "src/api.ts", + line: null, + }, + }); + const prompt = generateFixPrompt(context); + + expect(prompt).toContain("src/api.ts"); + expect(prompt).not.toContain("around line"); + }); + + it("includes user instructions for thread fix", () => { + const context = createBaseContext({ + eventData: { + eventName: "pull_request_review_comment", + prNumber: "42", + isPR: true as const, + commentBody: "@droid fix use try-catch instead", + }, + fixContext: { + parentCommentBody: "Missing error handling here", + filePath: "src/api.ts", + line: 10, + }, + }); + const prompt = generateFixPrompt(context); + + expect(prompt).toContain("use try-catch instead"); + }); + }); +}); diff --git a/test/modes/tag/fix-command.test.ts b/test/modes/tag/fix-command.test.ts new file mode 100644 index 0000000..ea547d4 --- /dev/null +++ b/test/modes/tag/fix-command.test.ts @@ -0,0 +1,277 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import * as core from "@actions/core"; +import { prepareFixMode } from "../../../src/tag/commands/fix"; +import { createMockContext } from "../../mockContext"; +import * as prFetcher from "../../../src/github/data/pr-fetcher"; +import * as promptModule from "../../../src/create-prompt"; +import * as mcpInstaller from "../../../src/mcp/install-mcp-server"; +import * as childProcess from "child_process"; + +const MOCK_PR_DATA = { + title: "Test PR", + body: "Existing description", + baseRefName: "main", + headRefName: "feature/branch", + headRefOid: "abcdef", +}; + +describe("prepareFixMode", () => { + const originalArgs = process.env.DROID_ARGS; + const originalFixModel = process.env.FIX_MODEL; + let fetchPRSpy: ReturnType; + let promptSpy: ReturnType; + let mcpSpy: ReturnType; + let setOutputSpy: ReturnType; + let exportVariableSpy: ReturnType; + let execSyncSpy: ReturnType; + + beforeEach(() => { + process.env.DROID_ARGS = ""; + delete process.env.FIX_MODEL; + + fetchPRSpy = spyOn(prFetcher, "fetchPRBranchData").mockResolvedValue({ + baseRefName: MOCK_PR_DATA.baseRefName, + headRefName: MOCK_PR_DATA.headRefName, + headRefOid: MOCK_PR_DATA.headRefOid, + title: "Test PR", + body: "Test description", + }); + + promptSpy = spyOn(promptModule, "createPrompt").mockResolvedValue(); + mcpSpy = spyOn(mcpInstaller, "prepareMcpTools").mockResolvedValue( + "mock-config", + ); + setOutputSpy = spyOn(core, "setOutput").mockImplementation(() => {}); + exportVariableSpy = spyOn(core, "exportVariable").mockImplementation( + () => {}, + ); + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + ) => { + if (typeof cmd === "string" && cmd.includes("rev-parse")) { + return "feature/branch\n"; + } + return ""; + }) as typeof childProcess.execSync); + }); + + afterEach(() => { + fetchPRSpy.mockRestore(); + promptSpy.mockRestore(); + mcpSpy.mockRestore(); + setOutputSpy.mockRestore(); + exportVariableSpy.mockRestore(); + execSyncSpy.mockRestore(); + process.env.DROID_ARGS = originalArgs; + if (originalFixModel !== undefined) { + process.env.FIX_MODEL = originalFixModel; + } else { + delete process.env.FIX_MODEL; + } + }); + + it("prepares file-editing toolset and prompt for fix command", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 100, + body: "@droid fix", + }, + } as any, + entityNumber: 42, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + const result = await prepareFixMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 99, + }); + + expect(fetchPRSpy).toHaveBeenCalledWith({ + octokits: octokit, + repository: context.repository, + prNumber: 42, + }); + expect(promptSpy).toHaveBeenCalled(); + expect(result.branchInfo.baseBranch).toBe("main"); + expect(result.branchInfo.currentBranch).toBe("feature/branch"); + expect(result.branchInfo.droidBranch).toBeUndefined(); + expect(result.commentId).toBe(99); + + // Verify allowed tools include file-editing tools + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).toContain("Edit"); + expect(droidArgsCall?.[1]).toContain("Create"); + expect(droidArgsCall?.[1]).toContain("ApplyPatch"); + expect(droidArgsCall?.[1]).toContain("Execute"); + expect(droidArgsCall?.[1]).toContain("FetchUrl"); + expect(droidArgsCall?.[1]).toContain('--tag "droid-fix"'); + + expect(exportVariableSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + "droid-fix", + ); + }); + + it("throws when invoked on non-PR context", async () => { + const context = createMockContext({ isPR: false }); + + await expect( + prepareFixMode({ + context, + octokit: { rest: {}, graphql: () => {} } as any, + githubToken: "token", + }), + ).rejects.toThrow("Fix command is only supported on pull requests"); + }); + + it("adds --model flag when FIX_MODEL is set", async () => { + process.env.FIX_MODEL = "gpt-5.1-codex"; + + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 101, + body: "@droid fix", + }, + } as any, + entityNumber: 43, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareFixMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 100, + }); + + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).toContain('--model "gpt-5.1-codex"'); + }); + + it("does not add --model flag when FIX_MODEL is empty", async () => { + process.env.FIX_MODEL = ""; + + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 102, + body: "@droid fix", + }, + } as any, + entityNumber: 44, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareFixMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 101, + }); + + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).not.toContain("--model"); + }); + + it("passes fix context for pull_request_review_comment events", async () => { + const context = createMockContext({ + eventName: "pull_request_review_comment", + isPR: true, + payload: { + action: "created", + comment: { + id: 200, + body: "@droid fix", + path: "src/utils/algorithm.js", + line: 42, + in_reply_to_id: 199, + created_at: "2024-01-01T00:00:00Z", + user: { login: "reviewer", id: 123 }, + }, + pull_request: { + number: 50, + title: "Test PR", + body: "", + user: { login: "dev", id: 456 }, + }, + } as any, + entityNumber: 50, + }); + + const mockGetReviewComment = async () => ({ + data: { + body: "This function has a null pointer dereference bug", + node_id: "node-1", + }, + }); + + const octokit = { + rest: { + pulls: { + getReviewComment: mockGetReviewComment, + }, + }, + graphql: () => {}, + } as any; + + await prepareFixMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 201, + }); + + // Verify createPrompt was called with fixContext + const promptCall = promptSpy.mock.calls[0] as [any]; + expect(promptCall[0].fixContext).toEqual({ + parentCommentBody: "This function has a null pointer dereference bug", + filePath: "src/utils/algorithm.js", + line: 42, + }); + }); + + it("does not pass fix context for issue_comment events", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 300, + body: "@droid fix all the things", + }, + } as any, + entityNumber: 60, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareFixMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 301, + }); + + const promptCall = promptSpy.mock.calls[0] as [any]; + expect(promptCall[0].fixContext).toBeUndefined(); + }); +}); From 184ee93fa8005ad1c10275bb770281c631765355 Mon Sep 17 00:00:00 2001 From: Nizar Alrifai Date: Tue, 10 Mar 2026 15:59:09 -0700 Subject: [PATCH 2/2] test: add integration tests for @droid fix flow Tests end-to-end through prepareTagExecution: - Top-level fix on PR comment (fix all review findings) - Thread reply fix on review comment (fix specific issue) - Routing: @droid without fix routes to review, not fix Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- test/integration/fix-flow.test.ts | 357 ++++++++++++++++++++++++++++++ 1 file changed, 357 insertions(+) create mode 100644 test/integration/fix-flow.test.ts diff --git a/test/integration/fix-flow.test.ts b/test/integration/fix-flow.test.ts new file mode 100644 index 0000000..ba6b492 --- /dev/null +++ b/test/integration/fix-flow.test.ts @@ -0,0 +1,357 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import path from "node:path"; +import os from "node:os"; +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { prepareTagExecution } from "../../src/tag"; +import { createMockContext } from "../mockContext"; +import * as createInitial from "../../src/github/operations/comments/create-initial"; +import * as mcpInstaller from "../../src/mcp/install-mcp-server"; +import * as actorValidation from "../../src/github/validation/actor"; +import * as core from "@actions/core"; +import * as childProcess from "node:child_process"; + +describe("fix command integration", () => { + const originalRunnerTemp = process.env.RUNNER_TEMP; + const originalDroidArgs = process.env.DROID_ARGS; + let tmpDir: string; + let graphqlSpy: ReturnType; + let createCommentSpy: ReturnType; + let mcpSpy: ReturnType; + let actorSpy: ReturnType; + let setOutputSpy: ReturnType; + let exportVarSpy: ReturnType; + let execSyncSpy: ReturnType; + + beforeEach(async () => { + tmpDir = await mkdtemp(path.join(os.tmpdir(), "fix-int-")); + process.env.RUNNER_TEMP = tmpDir; + process.env.DROID_ARGS = ""; + + createCommentSpy = spyOn( + createInitial, + "createInitialComment", + ).mockResolvedValue({ id: 301 } as any); + + mcpSpy = spyOn(mcpInstaller, "prepareMcpTools").mockResolvedValue("{}"); + actorSpy = spyOn(actorValidation, "checkHumanActor").mockResolvedValue(); + setOutputSpy = spyOn(core, "setOutput").mockImplementation(() => {}); + exportVarSpy = spyOn(core, "exportVariable").mockImplementation(() => {}); + execSyncSpy = spyOn(childProcess, "execSync").mockImplementation((( + cmd: string, + ) => { + if (cmd.includes("rev-parse")) return "feature/fix-branch\n"; + return ""; + }) as typeof childProcess.execSync); + }); + + afterEach(async () => { + graphqlSpy?.mockRestore(); + createCommentSpy.mockRestore(); + mcpSpy.mockRestore(); + actorSpy.mockRestore(); + setOutputSpy.mockRestore(); + exportVarSpy.mockRestore(); + execSyncSpy.mockRestore(); + + if (process.env.RUNNER_TEMP) { + await rm(process.env.RUNNER_TEMP, { recursive: true, force: true }); + } + + if (originalRunnerTemp) { + process.env.RUNNER_TEMP = originalRunnerTemp; + } else { + delete process.env.RUNNER_TEMP; + } + + if (originalDroidArgs !== undefined) { + process.env.DROID_ARGS = originalDroidArgs; + } else { + delete process.env.DROID_ARGS; + } + }); + + it("prepares top-level fix flow end-to-end via @droid fix on PR comment", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + actor: "human-user", + entityNumber: 10889, + repository: { + owner: "Factory-AI", + repo: "factory-mono", + full_name: "Factory-AI/factory-mono", + }, + payload: { + comment: { + id: 555, + body: "@droid fix", + user: { login: "reviewer" }, + created_at: "2024-01-02T00:00:00Z", + }, + issue: { + number: 10889, + pull_request: {}, + }, + } as any, + }); + + const octokit = { + rest: {}, + graphql: () => + Promise.resolve({ + repository: { + pullRequest: { + baseRefName: "dev", + headRefName: "ci-fail-review-test", + headRefOid: "55cf61f", + title: "[TEST] CI failure review validation", + body: "Test PR with intentional type error", + }, + }, + }), + } as any; + + graphqlSpy = spyOn(octokit, "graphql").mockResolvedValue({ + repository: { + pullRequest: { + baseRefName: "dev", + headRefName: "ci-fail-review-test", + headRefOid: "55cf61f", + title: "[TEST] CI failure review validation", + body: "Test PR with intentional type error", + }, + }, + }); + + const result = await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + // Verify the fix flow was selected (not review, fill, etc.) + expect(exportVarSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + "droid-fix", + ); + expect(result.commentId).toBe(301); + expect(result.branchInfo.baseBranch).toBe("dev"); + expect(result.branchInfo.currentBranch).toBe("ci-fail-review-test"); + + // Verify output flags: no code review, no security review + const runCodeReview = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "run_code_review", + ) as [string, string] | undefined; + const runSecurityReview = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "run_security_review", + ) as [string, string] | undefined; + expect(runCodeReview?.[1]).toBe("false"); + expect(runSecurityReview?.[1]).toBe("false"); + + // Verify prompt was written to disk + const promptPath = path.join( + tmpDir, + "droid-prompts", + "droid-prompt.txt", + ); + const prompt = await readFile(promptPath, "utf8"); + + // Top-level fix: should instruct to fix all review findings + expect(prompt).toContain("PR #10889"); + expect(prompt).toContain("Factory-AI/factory-mono"); + expect(prompt).toContain("gh pr diff 10889"); + expect(prompt).toContain("Identify issues to fix"); + expect(prompt).toContain("git commit"); + expect(prompt).toContain("git push"); + + // Verify droid_args include file-editing tools and fix tag + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).toContain("Edit"); + expect(droidArgsCall?.[1]).toContain("Create"); + expect(droidArgsCall?.[1]).toContain("ApplyPatch"); + expect(droidArgsCall?.[1]).toContain("Execute"); + expect(droidArgsCall?.[1]).toContain('--tag "droid-fix"'); + }); + + it("prepares thread-reply fix flow via @droid fix on review comment", async () => { + const context = createMockContext({ + eventName: "pull_request_review_comment", + isPR: true, + actor: "human-user", + entityNumber: 10889, + repository: { + owner: "Factory-AI", + repo: "factory-mono", + full_name: "Factory-AI/factory-mono", + }, + payload: { + action: "created", + comment: { + id: 2914349333, + body: "@droid fix", + path: "packages/common/src/shared/constants.ts", + line: 1, + in_reply_to_id: 2914349332, + user: { login: "reviewer", id: 123 }, + created_at: "2024-01-02T00:00:00Z", + }, + pull_request: { + number: 10889, + title: "[TEST] CI failure review validation", + body: "Test PR with intentional type error", + user: { login: "dev", id: 456 }, + }, + } as any, + }); + + const mockGetReviewComment = async () => ({ + data: { + body: "[P1] DEV_FACTORY_APP_BASE_URL is typed as `number` but assigned a string\n\n`export const DEV_FACTORY_APP_BASE_URL: number = 'https://dev.app.factory.ai';` is a TypeScript type error (string is not assignable to number) and will break typechecking.", + node_id: "node-1", + }, + }); + + const octokit = { + rest: { + pulls: { + getReviewComment: mockGetReviewComment, + }, + }, + graphql: () => + Promise.resolve({ + repository: { + pullRequest: { + baseRefName: "dev", + headRefName: "ci-fail-review-test", + headRefOid: "55cf61f", + title: "[TEST] CI failure review validation", + body: "Test PR with intentional type error", + }, + }, + }), + } as any; + + graphqlSpy = spyOn(octokit, "graphql").mockResolvedValue({ + repository: { + pullRequest: { + baseRefName: "dev", + headRefName: "ci-fail-review-test", + headRefOid: "55cf61f", + title: "[TEST] CI failure review validation", + body: "Test PR with intentional type error", + }, + }, + }); + + const result = await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + // Verify fix flow was selected + expect(exportVarSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + "droid-fix", + ); + expect(result.commentId).toBe(301); + + // Verify prompt was written and contains thread-specific context + const promptPath = path.join( + tmpDir, + "droid-prompts", + "droid-prompt.txt", + ); + const prompt = await readFile(promptPath, "utf8"); + + // Thread fix: should reference specific file and review comment + expect(prompt).toContain("packages/common/src/shared/constants.ts"); + expect(prompt).toContain("around line 1"); + expect(prompt).toContain("DEV_FACTORY_APP_BASE_URL"); + expect(prompt).toContain("TypeScript type error"); + expect(prompt).toContain( + "Only fix the specific issue mentioned in the review comment", + ); + expect(prompt).toContain("git commit"); + expect(prompt).toContain("git push"); + }); + + it("routes @droid (without fix) to review, not fix", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + actor: "human-user", + entityNumber: 42, + payload: { + comment: { + id: 555, + body: "@droid please look at this", + user: { login: "reviewer" }, + created_at: "2024-01-02T00:00:00Z", + }, + issue: { + number: 42, + pull_request: {}, + }, + } as any, + }); + + // For review flow, createPrompt is called (which we need to mock differently) + // We just verify it does NOT route to fix + const promptSpy = spyOn( + await import("../../src/create-prompt"), + "createPrompt", + ).mockResolvedValue(); + + const octokit = { + rest: { + issues: { listComments: () => Promise.resolve({ data: [] }) }, + pulls: { listReviewComments: () => Promise.resolve({ data: [] }) }, + }, + graphql: () => + Promise.resolve({ + repository: { + pullRequest: { + baseRefName: "main", + headRefName: "feature/test", + headRefOid: "abc", + title: "Test", + body: "", + }, + }, + }), + } as any; + + graphqlSpy = spyOn(octokit, "graphql").mockResolvedValue({ + repository: { + pullRequest: { + baseRefName: "main", + headRefName: "feature/test", + headRefOid: "abc", + title: "Test", + body: "", + }, + }, + }); + + await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + // Should be review, NOT fix + expect(exportVarSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + "droid-review", + ); + expect(exportVarSpy).not.toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + "droid-fix", + ); + + promptSpy.mockRestore(); + }); +});