From f15188f4261644d5a8bd291c48c077a5598035e2 Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Tue, 13 Jan 2026 13:37:06 -0800 Subject: [PATCH 1/8] chore: code formatting and style cleanup - Apply consistent formatting across base-action tests - Format README.md bullet points consistently - Minor whitespace and formatting fixes in various files Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- README.md | 42 +++++++------ base-action/src/run-droid.ts | 43 +++++++++---- base-action/test/parse-shell-args.test.ts | 19 +++--- base-action/test/run-droid-mcp.test.ts | 63 +++++++++++-------- base-action/test/run-droid.test.ts | 6 +- base-action/test/setup-droid-settings.test.ts | 4 +- base-action/test/validate-env.test.ts | 4 +- src/entrypoints/prepare.ts | 1 - src/github/token.ts | 38 +++++++---- src/mcp/github-inline-comment-server.ts | 6 +- src/mcp/install-mcp-server.ts | 5 +- src/tag/commands/fill.ts | 2 +- src/utils/parse-tools.ts | 27 ++++---- test/comment-logic.test.ts | 4 +- test/github/token.test.ts | 13 ++-- test/integration/fill-flow.test.ts | 36 +++++------ test/modes/tag/fill-command.test.ts | 9 +-- test/prepare-context.test.ts | 8 ++- test/trigger-validation.test.ts | 1 - test/utils/retry.test.ts | 20 +++--- 20 files changed, 191 insertions(+), 160 deletions(-) diff --git a/README.md b/README.md index b3cfb4d..be17208 100644 --- a/README.md +++ b/README.md @@ -2,8 +2,8 @@ This GitHub Action powers the Factory **Droid** app. It watches your pull requests for the two supported commands and runs a full Droid Exec session to help you ship faster: -* `@droid fill` — turns a bare pull request into a polished description that matches your template or our opinionated fallback. -* `@droid review` — performs an automated code review, surfaces potential bugs, and leaves inline comments directly on the diff. +- `@droid fill` — turns a bare pull request into a polished description that matches your template or our opinionated fallback. +- `@droid review` — performs an automated code review, surfaces potential bugs, and leaves inline comments directly on the diff. Everything runs inside GitHub Actions using your Factory API key, so the bot never leaves your repository and operates with the permissions you grant. @@ -18,11 +18,11 @@ Everything runs inside GitHub Actions using your Factory API key, so the bot nev ## Installation 1. **Install the Droid GitHub App** - * Install from the Factory dashboard and grant it access to the repositories where you want Droid to operate. + - Install from the Factory dashboard and grant it access to the repositories where you want Droid to operate. 2. **Create a Factory API Key** - * Generate a token at [https://app.factory.ai/settings/api-keys](https://app.factory.ai/settings/api-keys) and save it as `FACTORY_API_KEY` in your repository or organization secrets. + - Generate a token at [https://app.factory.ai/settings/api-keys](https://app.factory.ai/settings/api-keys) and save it as `FACTORY_API_KEY` in your repository or organization secrets. 3. **Add the Action Workflows** - * Create two workflow files under `.github/workflows/` to separate on-demand tagging from automatic PR reviews. + - Create two workflow files under `.github/workflows/` to separate on-demand tagging from automatic PR reviews. `droid.yml` (responds to explicit `@droid` mentions): @@ -105,26 +105,28 @@ Once committed, tagging `@droid fill` or `@droid review` on an open PR will trig ## Using the Commands ### `@droid fill` -* Place the command in the PR description or in a top-level comment. -* Droid searches for common PR template locations (`.github/pull_request_template.md`, etc.). When a template exists, it fills the sections; otherwise it writes a structured summary (overview, changes, testing, rollout). -* The original request is replaced with the generated description so reviewers can merge immediately. + +- Place the command in the PR description or in a top-level comment. +- Droid searches for common PR template locations (`.github/pull_request_template.md`, etc.). When a template exists, it fills the sections; otherwise it writes a structured summary (overview, changes, testing, rollout). +- The original request is replaced with the generated description so reviewers can merge immediately. ### `@droid review` -* Mention `@droid review` in a PR comment. -* Droid inspects the diff, prioritizes potential bugs or high-impact issues, and leaves inline comments directly on the changed lines. -* A short summary comment is posted in the original thread highlighting the findings and linking to any inline feedback. + +- Mention `@droid review` in a PR comment. +- Droid inspects the diff, prioritizes potential bugs or high-impact issues, and leaves inline comments directly on the changed lines. +- A short summary comment is posted in the original thread highlighting the findings and linking to any inline feedback. ## Configuration Essentials -| Input | Purpose | -| --- | --- | -| `factory_api_key` | **Required.** Grants Droid Exec permission to run via Factory. | -| `github_token` | Optional override if you prefer a custom GitHub App/token. By default the installed app token is used. | -| `review_model` | Optional. Override the model used for code review (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to review flows. | -| `fill_model` | Optional. Override the model used for PR description fill (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to fill flows. | +| Input | Purpose | +| ----------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| `factory_api_key` | **Required.** Grants Droid Exec permission to run via Factory. | +| `github_token` | Optional override if you prefer a custom GitHub App/token. By default the installed app token is used. | +| `review_model` | Optional. Override the model used for code review (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to review flows. | +| `fill_model` | Optional. Override the model used for PR description fill (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to fill flows. | ## Troubleshooting & Support -* Check the workflow run linked from the Droid tracking comment for execution logs. -* Verify that the workflow file and repository allow the GitHub App to run (branch protections can block bots). -* Need more detail? Start with the [Setup Guide](./docs/setup.md) or [FAQ](./docs/faq.md). +- Check the workflow run linked from the Droid tracking comment for execution logs. +- Verify that the workflow file and repository allow the GitHub App to run (branch protections can block bots). +- Need more detail? Start with the [Setup Guide](./docs/setup.md) or [FAQ](./docs/faq.md). diff --git a/base-action/src/run-droid.ts b/base-action/src/run-droid.ts index d7cb999..015dc98 100644 --- a/base-action/src/run-droid.ts +++ b/base-action/src/run-droid.ts @@ -68,6 +68,7 @@ function sanitizeJsonOutput( export type DroidOptions = { droidArgs?: string; + reasoningEffort?: string; pathToDroidExecutable?: string; allowedTools?: string; disallowedTools?: string; @@ -90,6 +91,11 @@ export function prepareRunConfig( ): PreparedConfig { const droidArgs = [...BASE_ARGS]; + // Add reasoning effort only when explicitly requested + if (options.reasoningEffort?.trim()) { + droidArgs.push("--reasoning-effort", options.reasoningEffort.trim()); + } + // Parse and add user's custom Droid arguments if (options.droidArgs?.trim()) { const parsed = parseShellArgs(options.droidArgs); @@ -121,10 +127,12 @@ export async function runDroid(promptPath: string, options: DroidOptions) { const cfg = JSON.parse(options.mcpTools); const servers = cfg?.mcpServers || {}; const serverNames = Object.keys(servers); - + if (serverNames.length > 0) { - console.log(`Registering ${serverNames.length} MCP servers: ${serverNames.join(", ")}`); - + console.log( + `Registering ${serverNames.length} MCP servers: ${serverNames.join(", ")}`, + ); + for (const [name, def] of Object.entries(servers)) { const cmd = [def.command, ...(def.args || [])] .filter(Boolean) @@ -143,12 +151,15 @@ export async function runDroid(promptPath: string, options: DroidOptions) { .join(" "); const addCmd = `droid mcp add ${name} "${cmd}" ${envFlags}`.trim(); - + try { await execAsync(addCmd, { env: { ...process.env } }); console.log(` ✓ Registered MCP server: ${name}`); } catch (e: any) { - console.error(` ✗ Failed to register MCP server ${name}:`, e.message); + console.error( + ` ✗ Failed to register MCP server ${name}:`, + e.message, + ); throw e; } } @@ -184,15 +195,19 @@ export async function runDroid(promptPath: string, options: DroidOptions) { // Log custom arguments if any if (options.droidArgs && options.droidArgs.trim() !== "") { console.log(`Custom Droid arguments: ${options.droidArgs}`); - + // Check for deprecated MCP tool naming - const enabledToolsMatch = options.droidArgs.match(/--enabled-tools\s+["\']?([^"\']+)["\']?/); + const enabledToolsMatch = options.droidArgs.match( + /--enabled-tools\s+["\']?([^"\']+)["\']?/, + ); if (enabledToolsMatch && enabledToolsMatch[1]) { - const tools = enabledToolsMatch[1].split(",").map(t => t.trim()); - const oldStyleTools = tools.filter(t => t.startsWith("mcp__")); - + const tools = enabledToolsMatch[1].split(",").map((t) => t.trim()); + const oldStyleTools = tools.filter((t) => t.startsWith("mcp__")); + if (oldStyleTools.length > 0) { - console.warn(`Warning: Found ${oldStyleTools.length} tools with deprecated mcp__ prefix. Update to new pattern (e.g., github_comment___update_droid_comment)`); + console.warn( + `Warning: Found ${oldStyleTools.length} tools with deprecated mcp__ prefix. Update to new pattern (e.g., github_comment___update_droid_comment)`, + ); } } } @@ -247,7 +262,10 @@ export async function runDroid(promptPath: string, options: DroidOptions) { const parsed = JSON.parse(line); if (!sessionId && typeof parsed === "object" && parsed !== null) { const detectedSessionId = parsed.session_id; - if (typeof detectedSessionId === "string" && detectedSessionId.trim()) { + if ( + typeof detectedSessionId === "string" && + detectedSessionId.trim() + ) { sessionId = detectedSessionId; console.log(`Detected Droid session: ${sessionId}`); } @@ -272,7 +290,6 @@ export async function runDroid(promptPath: string, options: DroidOptions) { // In non-full-output mode, suppress non-JSON output } }); - }); // Handle stdout errors diff --git a/base-action/test/parse-shell-args.test.ts b/base-action/test/parse-shell-args.test.ts index b6f0dc4..297d5e3 100644 --- a/base-action/test/parse-shell-args.test.ts +++ b/base-action/test/parse-shell-args.test.ts @@ -8,14 +8,8 @@ describe("shell-quote parseShellArgs", () => { }); test("should parse simple arguments", () => { - expect(parseShellArgs("--auto medium")).toEqual([ - "--auto", - "medium", - ]); - expect(parseShellArgs("-s session-123")).toEqual([ - "-s", - "session-123", - ]); + expect(parseShellArgs("--auto medium")).toEqual(["--auto", "medium"]); + expect(parseShellArgs("-s session-123")).toEqual(["-s", "session-123"]); }); test("should handle double quotes", () => { @@ -27,10 +21,11 @@ describe("shell-quote parseShellArgs", () => { }); test("should handle single quotes", () => { - expect(parseShellArgs("--file '/tmp/prompt.md'")) - .toEqual(["--file", "/tmp/prompt.md"]); - expect(parseShellArgs("'arg with spaces'")) - .toEqual(["arg with spaces"]); + expect(parseShellArgs("--file '/tmp/prompt.md'")).toEqual([ + "--file", + "/tmp/prompt.md", + ]); + expect(parseShellArgs("'arg with spaces'")).toEqual(["arg with spaces"]); }); test("should handle escaped characters", () => { expect(parseShellArgs("arg\\ with\\ spaces")).toEqual(["arg with spaces"]); diff --git a/base-action/test/run-droid-mcp.test.ts b/base-action/test/run-droid-mcp.test.ts index ea652a2..98cde42 100644 --- a/base-action/test/run-droid-mcp.test.ts +++ b/base-action/test/run-droid-mcp.test.ts @@ -74,11 +74,15 @@ const mockSpawn = mock( mock.module("child_process", () => ({ exec: ( command: string, - options?: Record | ((err: Error | null, result?: any) => void), + options?: + | Record + | ((err: Error | null, result?: any) => void), maybeCallback?: (err: Error | null, result?: any) => void, ) => { const callback = - typeof options === "function" ? options : maybeCallback ?? (() => undefined); + typeof options === "function" + ? options + : (maybeCallback ?? (() => undefined)); setImmediate(async () => { try { @@ -98,7 +102,7 @@ let runDroid: RunDroidModule["runDroid"]; beforeAll(async () => { const module = (await import( - `../src/run-droid?mcp-test=${Math.random().toString(36).slice(2)}`, + `../src/run-droid?mcp-test=${Math.random().toString(36).slice(2)}` )) as RunDroidModule; prepareRunConfig = module.prepareRunConfig; runDroid = module.runDroid; @@ -139,23 +143,23 @@ describe("MCP Server Registration", () => { env: { GITHUB_TOKEN: "test-token", REPO_OWNER: "owner", - REPO_NAME: "repo" - } + REPO_NAME: "repo", + }, }, github_ci: { command: "bun", args: ["run", "/path/to/github-actions-server.ts"], env: { GITHUB_TOKEN: "test-token", - PR_NUMBER: "123" - } - } - } + PR_NUMBER: "123", + }, + }, + }, }); const options: DroidOptions = { mcpTools, - pathToDroidExecutable: "droid" + pathToDroidExecutable: "droid", }; const promptPath = await createPromptFile(); const tempDir = process.env.RUNNER_TEMP!; @@ -180,14 +184,14 @@ describe("MCP Server Registration", () => { mcpTools: "", }; const prepared = prepareRunConfig("/tmp/test-prompt.txt", options); - + expect(prepared.droidArgs).not.toContain("--mcp-config"); }); test("should handle invalid JSON in MCP config", () => { const options: DroidOptions = { mcpTools: "{ invalid json }", - pathToDroidExecutable: "droid" + pathToDroidExecutable: "droid", }; // prepareRunConfig doesn't parse MCP config, so it won't throw @@ -205,7 +209,8 @@ describe("MCP Server Registration", () => { console.warn = warnSpy as unknown as typeof console.warn; const options: DroidOptions = { - droidArgs: '--enabled-tools "mcp__github_comment__update_droid_comment,Execute"' + droidArgs: + '--enabled-tools "mcp__github_comment__update_droid_comment,Execute"', }; const promptPath = await createPromptFile(); @@ -216,8 +221,10 @@ describe("MCP Server Registration", () => { const warningMessages = warnSpy.mock.calls.map((args) => args[0]); expect( - warningMessages.some((msg) => - typeof msg === "string" && msg.includes("deprecated mcp__ prefix"), + warningMessages.some( + (msg) => + typeof msg === "string" && + msg.includes("deprecated mcp__ prefix"), ), ).toBe(true); } finally { @@ -232,7 +239,8 @@ describe("MCP Server Registration", () => { console.warn = warnSpy as unknown as typeof console.warn; const options: DroidOptions = { - droidArgs: '--enabled-tools "github_comment___update_droid_comment,Execute"' + droidArgs: + '--enabled-tools "github_comment___update_droid_comment,Execute"', }; const promptPath = await createPromptFile(); @@ -249,14 +257,17 @@ describe("MCP Server Registration", () => { test("should detect MCP tools with triple underscore pattern", () => { const options: DroidOptions = { - droidArgs: '--enabled-tools "github_ci___get_ci_status,github_comment___update_droid_comment"' + droidArgs: + '--enabled-tools "github_ci___get_ci_status,github_comment___update_droid_comment"', }; const prepared = prepareRunConfig("/tmp/test-prompt.txt", options); - + // The args should be passed through correctly expect(prepared.droidArgs).toContain("--enabled-tools"); - expect(prepared.droidArgs).toContain("github_ci___get_ci_status,github_comment___update_droid_comment"); + expect(prepared.droidArgs).toContain( + "github_ci___get_ci_status,github_comment___update_droid_comment", + ); }); }); @@ -267,14 +278,14 @@ describe("MCP Server Registration", () => { failing_server: { command: "nonexistent", args: ["command"], - env: {} - } - } + env: {}, + }, + }, }); const options: DroidOptions = { mcpTools, - pathToDroidExecutable: "droid" + pathToDroidExecutable: "droid", }; const promptPath = await createPromptFile(); const tempDir = process.env.RUNNER_TEMP!; @@ -299,18 +310,18 @@ describe("MCP Server Registration", () => { describe("Environment Variables", () => { test("should include GITHUB_ACTION_INPUTS when present", () => { process.env.INPUT_ACTION_INPUTS_PRESENT = "true"; - + const options: DroidOptions = {}; const prepared = prepareRunConfig("/tmp/test-prompt.txt", options); expect(prepared.env.GITHUB_ACTION_INPUTS).toBe("true"); - + delete process.env.INPUT_ACTION_INPUTS_PRESENT; }); test("should not include GITHUB_ACTION_INPUTS when not present", () => { delete process.env.INPUT_ACTION_INPUTS_PRESENT; - + const options: DroidOptions = {}; const prepared = prepareRunConfig("/tmp/test-prompt.txt", options); diff --git a/base-action/test/run-droid.test.ts b/base-action/test/run-droid.test.ts index 4bb95d2..36f7cb6 100644 --- a/base-action/test/run-droid.test.ts +++ b/base-action/test/run-droid.test.ts @@ -43,7 +43,7 @@ describe("prepareRunConfig", () => { "exec", "--output-format", "stream-json", -"--skip-permissions-unsafe", + "--skip-permissions-unsafe", "--max-turns", "10", "--model", @@ -63,7 +63,7 @@ describe("prepareRunConfig", () => { "exec", "--output-format", "stream-json", - "--skip-permissions-unsafe", + "--skip-permissions-unsafe", "-f", "/tmp/test-prompt.txt", ]); @@ -79,7 +79,7 @@ describe("prepareRunConfig", () => { "exec", "--output-format", "stream-json", - "--skip-permissions-unsafe", + "--skip-permissions-unsafe", "--system-prompt", "You are a helpful assistant", "-f", diff --git a/base-action/test/setup-droid-settings.test.ts b/base-action/test/setup-droid-settings.test.ts index 00dc661..0ae22a4 100644 --- a/base-action/test/setup-droid-settings.test.ts +++ b/base-action/test/setup-droid-settings.test.ts @@ -99,7 +99,9 @@ describe("setupDroidSettings", () => { }); test("should throw error for non-existent file path", async () => { - expect(() => setupDroidSettings("/non/existent/file.json", testHomeDir)).toThrow(); + expect(() => + setupDroidSettings("/non/existent/file.json", testHomeDir), + ).toThrow(); }); test("should handle empty string input", async () => { diff --git a/base-action/test/validate-env.test.ts b/base-action/test/validate-env.test.ts index 67b5a28..0989d29 100644 --- a/base-action/test/validate-env.test.ts +++ b/base-action/test/validate-env.test.ts @@ -16,13 +16,13 @@ describe("validateEnvironmentVariables", () => { }); test("passes when FACTORY_API_KEY is set", () => { - process.env.FACTORY_API_KEY = 'test-factory-key'; + process.env.FACTORY_API_KEY = "test-factory-key"; expect(() => validateEnvironmentVariables()).not.toThrow(); }); test("throws when FACTORY_API_KEY is missing", () => { expect(() => validateEnvironmentVariables()).toThrow( - 'FACTORY_API_KEY is required to run Droid Exec. Please provide the factory_api_key input.' + "FACTORY_API_KEY is required to run Droid Exec. Please provide the factory_api_key input.", ); }); }); diff --git a/src/entrypoints/prepare.ts b/src/entrypoints/prepare.ts index cb09303..5763f35 100644 --- a/src/entrypoints/prepare.ts +++ b/src/entrypoints/prepare.ts @@ -73,7 +73,6 @@ async function run() { if (result?.mcpTools) { core.setOutput("mcp_tools", result.mcpTools); } - } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); core.setFailed(`Prepare step failed with error: ${errorMessage}`); diff --git a/src/github/token.ts b/src/github/token.ts index 986f0b6..471f8ba 100644 --- a/src/github/token.ts +++ b/src/github/token.ts @@ -41,45 +41,57 @@ async function exchangeForAppToken(oidcToken: string): Promise { // Handle the simplified flat error response format const errorCode = responseJson.error || `http_${response.status}`; - const errorMessage = responseJson.message || responseJson.detail || responseJson.error || "Unknown error"; + const errorMessage = + responseJson.message || + responseJson.detail || + responseJson.error || + "Unknown error"; const specificErrorCode = responseJson.error_code; const repository = responseJson.repository; // Check for specific error codes that should skip the action - if (errorCode === "workflow_validation_failed" || - specificErrorCode === "workflow_not_found_on_default_branch") { - core.warning(`Skipping action due to workflow validation: ${errorMessage}`); + if ( + errorCode === "workflow_validation_failed" || + specificErrorCode === "workflow_not_found_on_default_branch" + ) { + core.warning( + `Skipping action due to workflow validation: ${errorMessage}`, + ); console.log( "Action skipped due to workflow validation error. This is expected when adding Droid workflows to new repositories or on PRs with workflow changes. If you're seeing this, your workflow will begin working once you merge your PR.", ); core.setOutput("skipped_due_to_workflow_validation_mismatch", "true"); process.exit(0); } - + // Handle GitHub App not installed error with helpful message if (errorCode === "app_not_installed") { const repo = repository || "this repository"; console.error( `The Factory GitHub App is not installed for ${repo}. ` + - `Please install it at: https://github.com/apps/factory-ai` + `Please install it at: https://github.com/apps/factory-ai`, ); throw new Error(errorMessage); } - + // Handle rate limiting with retry suggestion if (errorCode === "rate_limited") { console.error( - `GitHub API rate limit exceeded. Please wait a few minutes and try again.` + `GitHub API rate limit exceeded. Please wait a few minutes and try again.`, ); throw new Error(errorMessage); } - + // Handle OIDC verification errors if (errorCode === "oidc_verification_failed") { if (specificErrorCode === "token_expired") { - console.error("OIDC token has expired. The workflow may be taking too long."); + console.error( + "OIDC token has expired. The workflow may be taking too long.", + ); } else if (specificErrorCode === "audience_mismatch") { - console.error("OIDC token audience mismatch. This is likely a configuration issue."); + console.error( + "OIDC token audience mismatch. This is likely a configuration issue.", + ); } else if (specificErrorCode === "invalid_signature") { console.error("OIDC token signature verification failed."); } @@ -89,7 +101,7 @@ async function exchangeForAppToken(oidcToken: string): Promise { console.error( `App token exchange failed: ${response.status} ${response.statusText} - ${errorMessage}`, errorCode !== errorMessage ? `(Code: ${errorCode})` : "", - specificErrorCode ? `(Specific: ${specificErrorCode})` : "" + specificErrorCode ? `(Specific: ${specificErrorCode})` : "", ); throw new Error(errorMessage); } @@ -99,7 +111,7 @@ async function exchangeForAppToken(oidcToken: string): Promise { token?: string; expires_at?: string; }; - + if (!appTokenData.token) { throw new Error("App token not found in response"); } diff --git a/src/mcp/github-inline-comment-server.ts b/src/mcp/github-inline-comment-server.ts index 3b9ae31..2a51185 100644 --- a/src/mcp/github-inline-comment-server.ts +++ b/src/mcp/github-inline-comment-server.ts @@ -59,9 +59,9 @@ server.tool( .optional() .default("RIGHT") .describe( - "Side of the diff to comment on: LEFT (old code) or RIGHT (new code). " + - "IMPORTANT: Use RIGHT for comments on new/modified code. " + - "Only use LEFT when specifically referencing code being removed." + "Side of the diff to comment on: LEFT (old code) or RIGHT (new code). " + + "IMPORTANT: Use RIGHT for comments on new/modified code. " + + "Only use LEFT when specifically referencing code being removed.", ), commit_id: z .string() diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 44b5c46..0714523 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -93,7 +93,6 @@ export async function prepareMcpTools( }, }; - // Include inline comment server for PRs when requested via allowed tools if ( isEntityContext(context) && @@ -118,9 +117,7 @@ export async function prepareMcpTools( const hasWorkflowToken = !!process.env.DEFAULT_WORKFLOW_TOKEN; const shouldIncludeCIServer = - isEntityContext(context) && - context.isPR && - hasWorkflowToken; + isEntityContext(context) && context.isPR && hasWorkflowToken; if (shouldIncludeCIServer) { // Verify the token actually has actions:read permission diff --git a/src/tag/commands/fill.ts b/src/tag/commands/fill.ts index 67a5e19..1cc7e31 100644 --- a/src/tag/commands/fill.ts +++ b/src/tag/commands/fill.ts @@ -33,7 +33,7 @@ export async function prepareFillMode({ const commentId = trackingCommentId ?? (await createInitialComment(octokit.rest, context)).id; - + const prData = await fetchPRBranchData({ octokits: octokit, repository: context.repository, diff --git a/src/utils/parse-tools.ts b/src/utils/parse-tools.ts index 9f6e065..9020401 100644 --- a/src/utils/parse-tools.ts +++ b/src/utils/parse-tools.ts @@ -22,7 +22,10 @@ export function parseAllowedTools(args: string): string[] { return []; } - return value.split(",").map((tool) => tool.trim()).filter(Boolean); + return value + .split(",") + .map((tool) => tool.trim()) + .filter(Boolean); } export function normalizeDroidArgs(args: string): string { @@ -30,14 +33,16 @@ export function normalizeDroidArgs(args: string): string { return ""; } - return args - .replace(/--allowedTools/g, "--enabled-tools") - .replace(/--allowed-tools/g, "--enabled-tools") - .replace(/--enabledTools/g, "--enabled-tools") - .replace(/--disallowedTools/g, "--disabled-tools") - .replace(/--disabled-tools/g, "--disabled-tools") - .replace(/--disallowed-tools/g, "--disabled-tools") - // Strip unsupported MCP inline config flags to avoid CLI errors - .replace(/--mcp-config\s+(?:"[^"]*"|'[^']*'|[^\s]+)/g, "") - .trim(); + return ( + args + .replace(/--allowedTools/g, "--enabled-tools") + .replace(/--allowed-tools/g, "--enabled-tools") + .replace(/--enabledTools/g, "--enabled-tools") + .replace(/--disallowedTools/g, "--disabled-tools") + .replace(/--disabled-tools/g, "--disabled-tools") + .replace(/--disallowed-tools/g, "--disabled-tools") + // Strip unsupported MCP inline config flags to avoid CLI errors + .replace(/--mcp-config\s+(?:"[^"]*"|'[^']*'|[^\s]+)/g, "") + .trim() + ); } diff --git a/test/comment-logic.test.ts b/test/comment-logic.test.ts index 33bb2e6..b068949 100644 --- a/test/comment-logic.test.ts +++ b/test/comment-logic.test.ts @@ -279,9 +279,7 @@ describe("updateCommentBody", () => { }; const result = updateCommentBody(input); - expect(result).toContain( - "**Droid finished @testuser's task in 1m 15s**", - ); + expect(result).toContain("**Droid finished @testuser's task in 1m 15s**"); }); it("includes duration in error header", () => { diff --git a/test/github/token.test.ts b/test/github/token.test.ts index d5983a6..a4d8a5a 100644 --- a/test/github/token.test.ts +++ b/test/github/token.test.ts @@ -34,15 +34,14 @@ describe("setupGitHubToken", () => { process.env.OVERRIDE_GITHUB_TOKEN = "override-token"; const setOutputSpy = spyOn(core, "setOutput").mockImplementation(() => {}); - const getIdTokenSpy = spyOn(core, "getIDToken").mockResolvedValue("oidc-token"); + const getIdTokenSpy = spyOn(core, "getIDToken").mockResolvedValue( + "oidc-token", + ); const result = await setupGitHubToken(); expect(result).toBe("override-token"); - expect(setOutputSpy).toHaveBeenCalledWith( - "GITHUB_TOKEN", - "override-token", - ); + expect(setOutputSpy).toHaveBeenCalledWith("GITHUB_TOKEN", "override-token"); expect(getIdTokenSpy).not.toHaveBeenCalled(); setOutputSpy.mockRestore(); @@ -59,7 +58,9 @@ describe("setupGitHubToken", () => { global.fetch = fetchMock as unknown as typeof fetch; const setOutputSpy = spyOn(core, "setOutput").mockImplementation(() => {}); - const getIdTokenSpy = spyOn(core, "getIDToken").mockResolvedValue("oidc-token"); + const getIdTokenSpy = spyOn(core, "getIDToken").mockResolvedValue( + "oidc-token", + ); const retrySpy = spyOn(retryModule, "retryWithBackoff").mockImplementation( (operation: () => Promise) => operation(), ); diff --git a/test/integration/fill-flow.test.ts b/test/integration/fill-flow.test.ts index 21ea50c..ed41bc5 100644 --- a/test/integration/fill-flow.test.ts +++ b/test/integration/fill-flow.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 path from "node:path"; import os from "node:os"; import { mkdtemp, readFile, rm } from "node:fs/promises"; @@ -93,17 +86,18 @@ describe("fill command integration", () => { } as any, }); - const octokit = { - rest: {}, - graphql: () => Promise.resolve({ - repository: { - pullRequest: { - baseRefName: "main", - headRefName: "feature/fill", - headRefOid: "abc123", - } - } - }) + const octokit = { + rest: {}, + graphql: () => + Promise.resolve({ + repository: { + pullRequest: { + baseRefName: "main", + headRefName: "feature/fill", + headRefOid: "abc123", + }, + }, + }), } as any; graphqlSpy = spyOn(octokit, "graphql").mockResolvedValue({ @@ -112,8 +106,8 @@ describe("fill command integration", () => { baseRefName: "main", headRefName: "feature/fill", headRefOid: "abc123", - } - } + }, + }, }); const result = await prepareTagExecution({ diff --git a/test/modes/tag/fill-command.test.ts b/test/modes/tag/fill-command.test.ts index 46b6cdf..ad3f061 100644 --- a/test/modes/tag/fill-command.test.ts +++ b/test/modes/tag/fill-command.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 * as core from "@actions/core"; import { prepareFillMode } from "../../../src/tag/commands/fill"; import { createMockContext } from "../../mockContext"; diff --git a/test/prepare-context.test.ts b/test/prepare-context.test.ts index e228822..d391956 100644 --- a/test/prepare-context.test.ts +++ b/test/prepare-context.test.ts @@ -72,7 +72,9 @@ describe("parseEnvVarsWithContext", () => { test("should throw error when DROID_BRANCH is missing", () => { expect(() => prepareContext(mockIssueCommentContext, "12345", "main"), - ).toThrow("issue_comment on issues requires droidBranch and baseBranch"); + ).toThrow( + "issue_comment on issues requires droidBranch and baseBranch", + ); }); test("should throw error when BASE_BRANCH is missing", () => { @@ -83,7 +85,9 @@ describe("parseEnvVarsWithContext", () => { undefined, "droid/issue-67890-20240101-1200", ), - ).toThrow("issue_comment on issues requires droidBranch and baseBranch"); + ).toThrow( + "issue_comment on issues requires droidBranch and baseBranch", + ); }); }); diff --git a/test/trigger-validation.test.ts b/test/trigger-validation.test.ts index 3ca63f0..46bdf8f 100644 --- a/test/trigger-validation.test.ts +++ b/test/trigger-validation.test.ts @@ -21,7 +21,6 @@ import type { import type { ParsedGitHubContext } from "../src/github/context"; describe("checkContainsTrigger", () => { - // TODO: Assignee and label triggers are disabled until instruct mode is implemented. // These tests verify the triggers are no-ops for now. describe("assignee trigger (disabled)", () => { diff --git a/test/utils/retry.test.ts b/test/utils/retry.test.ts index 36a58f0..84a584d 100644 --- a/test/utils/retry.test.ts +++ b/test/utils/retry.test.ts @@ -5,14 +5,14 @@ describe("retryWithBackoff", () => { let timeoutSpy: ReturnType; beforeEach(() => { - timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation( - ((handler: Parameters[0]) => { - if (typeof handler === "function") { - handler(); - } - return 0 as unknown as ReturnType; - }) as unknown as typeof setTimeout, - ); + timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation((( + handler: Parameters[0], + ) => { + if (typeof handler === "function") { + handler(); + } + return 0 as unknown as ReturnType; + }) as unknown as typeof setTimeout); }); afterEach(() => { @@ -64,7 +64,9 @@ describe("retryWithBackoff", () => { expect(attempts).toBe(2); expect(timeoutSpy).toHaveBeenCalledTimes(1); - const firstCall = timeoutSpy.mock.calls[0] as Parameters | undefined; + const firstCall = timeoutSpy.mock.calls[0] as + | Parameters + | undefined; expect(firstCall?.[1]).toBe(5); }); }); From f607dfd548c4286e8b6fa4aed66faca4e81b1b87 Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Tue, 13 Jan 2026 13:39:55 -0800 Subject: [PATCH 2/8] feat: add @droid security commands Add support for security-focused code review commands: - @droid security - Security review on PR changes - @droid security --full - Full repository security scan New features: - Security command parser (security, review-security, security-full) - Security review prompt with STRIDE methodology - Security scan prompt with threat model generation - Security configuration inputs in action.yml - Security-specific tracking comment message Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- action.yml | 40 +++ src/create-prompt/index.ts | 6 +- src/create-prompt/templates/fill-prompt.ts | 4 +- .../templates/security-report-prompt.ts | 210 +++++++++++ .../templates/security-review-prompt.ts | 171 +++++++++ src/create-prompt/types.ts | 2 +- src/entrypoints/collect-inputs.ts | 8 + src/github/api/queries/github.ts | 10 + src/github/context.ts | 18 + src/github/data/pr-fetcher.ts | 45 ++- src/github/operations/comments/common.ts | 10 +- .../operations/comments/create-initial.ts | 9 +- src/github/utils/command-parser.test.ts | 247 ++++++++----- src/github/utils/command-parser.ts | 89 ++++- .../validation/trigger-commands.test.ts | 40 ++- src/github/validation/trigger.ts | 6 +- src/tag/commands/security-review.ts | 128 +++++++ src/tag/commands/security-scan.ts | 107 ++++++ src/tag/index.ts | 56 ++- test/create-prompt.test.ts | 8 + .../templates/fill-prompt.test.ts | 8 +- .../templates/security-report-prompt.test.ts | 156 ++++++++ .../templates/security-review-prompt.test.ts | 82 +++++ test/mockContext.ts | 8 + test/modes/tag.test.ts | 13 + .../modes/tag/security-review-command.test.ts | 335 ++++++++++++++++++ test/permissions.test.ts | 8 + 27 files changed, 1688 insertions(+), 136 deletions(-) create mode 100644 src/create-prompt/templates/security-report-prompt.ts create mode 100644 src/create-prompt/templates/security-review-prompt.ts create mode 100644 src/tag/commands/security-review.ts create mode 100644 src/tag/commands/security-scan.ts create mode 100644 test/create-prompt/templates/security-report-prompt.test.ts create mode 100644 test/create-prompt/templates/security-review-prompt.test.ts create mode 100644 test/modes/tag/security-review-command.test.ts diff --git a/action.yml b/action.yml index 7218ff0..f259fef 100644 --- a/action.yml +++ b/action.yml @@ -59,6 +59,38 @@ inputs: description: "Automatically run the review flow for pull request contexts without requiring an explicit @droid review command. Only supported for PR-related events." required: false default: "false" + automatic_security_review: + description: "Automatically run the security review flow for pull request contexts without requiring an explicit @droid security command. Only supported for PR-related events." + required: false + default: "false" + security_model: + description: "Override the model used for security review (e.g., 'claude-sonnet-4-5-20250929', 'gpt-5.1-codex'). Only applies to security review flows." + required: false + default: "" + security_severity_threshold: + description: "Minimum severity to report in security reviews (critical, high, medium, low). Findings below this threshold will be filtered out." + required: false + default: "medium" + security_block_on_critical: + description: "Submit REQUEST_CHANGES review when critical severity findings are detected." + required: false + default: "true" + security_block_on_high: + description: "Submit REQUEST_CHANGES review when high severity findings are detected." + required: false + default: "false" + security_notify_team: + description: "GitHub team to @mention on critical findings (e.g., '@org/security-team')." + required: false + default: "" + security_scan_schedule: + description: "Enable scheduled security scans. Set to 'true' for schedule events to trigger full repository scans." + required: false + default: "false" + security_scan_days: + description: "Number of days of commits to scan for scheduled security scans. Only applies when security_scan_schedule is enabled." + required: false + default: "7" review_model: description: "Override the model used for code review (e.g., 'claude-sonnet-4-5-20250929', 'gpt-5.1-codex'). Only applies to review flows." required: false @@ -131,6 +163,14 @@ runs: DEFAULT_WORKFLOW_TOKEN: ${{ github.token }} TRACK_PROGRESS: ${{ inputs.track_progress }} AUTOMATIC_REVIEW: ${{ inputs.automatic_review }} + AUTOMATIC_SECURITY_REVIEW: ${{ inputs.automatic_security_review }} + SECURITY_MODEL: ${{ inputs.security_model }} + SECURITY_SEVERITY_THRESHOLD: ${{ inputs.security_severity_threshold }} + SECURITY_BLOCK_ON_CRITICAL: ${{ inputs.security_block_on_critical }} + SECURITY_BLOCK_ON_HIGH: ${{ inputs.security_block_on_high }} + SECURITY_NOTIFY_TEAM: ${{ inputs.security_notify_team }} + SECURITY_SCAN_SCHEDULE: ${{ inputs.security_scan_schedule }} + SECURITY_SCAN_DAYS: ${{ inputs.security_scan_days }} REVIEW_MODEL: ${{ inputs.review_model }} FILL_MODEL: ${{ inputs.fill_model }} ADDITIONAL_PERMISSIONS: ${{ inputs.additional_permissions }} diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index 511fd34..b2ea5af 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -66,7 +66,7 @@ export function buildDisallowedToolsString( export function prepareContext( context: ParsedGitHubContext, - droidCommentId: string, + droidCommentId?: string, baseBranch?: string, droidBranch?: string, prBranchData?: { headRefName: string; headRefOid: string }, @@ -288,7 +288,7 @@ export type PromptGenerator = ( export type PromptCreationOptions = { githubContext: ParsedGitHubContext; - commentId: number; + commentId?: number; baseBranch?: string; droidBranch?: string; prBranchData?: { headRefName: string; headRefOid: string }; @@ -310,7 +310,7 @@ export async function createPrompt({ includeActionsTools = false, }: PromptCreationOptions) { try { - const droidCommentId = commentId.toString(); + const droidCommentId = commentId?.toString(); const preparedContext = prepareContext( githubContext, droidCommentId, diff --git a/src/create-prompt/templates/fill-prompt.ts b/src/create-prompt/templates/fill-prompt.ts index cc105d3..86bad58 100644 --- a/src/create-prompt/templates/fill-prompt.ts +++ b/src/create-prompt/templates/fill-prompt.ts @@ -1,8 +1,6 @@ import type { PreparedContext } from "../types"; -export function generateFillPrompt( - context: PreparedContext, -): string { +export function generateFillPrompt(context: PreparedContext): string { const prNumber = context.eventData.isPR ? context.eventData.prNumber : context.githubContext && "entityNumber" in context.githubContext diff --git a/src/create-prompt/templates/security-report-prompt.ts b/src/create-prompt/templates/security-report-prompt.ts new file mode 100644 index 0000000..7a1a0be --- /dev/null +++ b/src/create-prompt/templates/security-report-prompt.ts @@ -0,0 +1,210 @@ +import type { PreparedContext } from "../types"; +import type { ScanScope } from "../../tag/commands/security-scan"; + +export function generateSecurityReportPrompt( + context: PreparedContext, + scanScope: ScanScope, + branchName: string, +): string { + const date = new Date().toISOString().split("T")[0]; + const repoFullName = context.repository; + + const scopeDescription = + scanScope.type === "full" + ? "Entire repository" + : `Last ${scanScope.days} days of commits`; + + const scanTypeLabel = + scanScope.type === "full" ? "Full Repository" : "Weekly Scheduled"; + + const scanInstructions = + scanScope.type === "full" + ? `- Scan all source files in the repository +- Focus on: TypeScript, JavaScript, Python, Go, Java, Ruby, PHP files +- Use: \`find . -type f \\( -name "*.ts" -o -name "*.js" -o -name "*.py" -o -name "*.go" -o -name "*.java" -o -name "*.rb" -o -name "*.php" \\) -not -path "./node_modules/*" -not -path "./.git/*"\`` + : `- Get commits from the last ${scanScope.days} days: \`git log --since="${scanScope.days} days ago" --name-only --pretty=format:""\` +- Focus analysis on files changed in recent commits +- Scan each changed file for security vulnerabilities`; + + // Extract security configuration from context + const securityConfig = context.githubContext?.inputs; + const severityThreshold = + securityConfig?.securitySeverityThreshold ?? "medium"; + const notifyTeam = securityConfig?.securityNotifyTeam ?? ""; + + return `You are performing a ${scanTypeLabel.toLowerCase()} security scan for ${repoFullName}. +The gh CLI is installed and authenticated via GH_TOKEN. + +## Scan Configuration +- Scope: ${scopeDescription} +- Severity Threshold: ${severityThreshold} (only report findings at or above this level) +- Output Branch: ${branchName} +- Report Path: .factory/security/reports/security-report-${date}.md + +## Security Skills Available + +You have access to these Factory security skills (installed in ~/.factory/skills/): + +1. **threat-model-generation** - Generate STRIDE-based threat model for the repository +2. **commit-security-scan** - Scan code for security vulnerabilities +3. **vulnerability-validation** - Validate findings, assess exploitability, filter false positives +4. **security-review** - Comprehensive security review and patch generation + +## Workflow + +### Step 1: Create Branch +\`\`\`bash +git checkout -b ${branchName} +\`\`\` + +### Step 2: Threat Model Check +- Check if \`.factory/threat-model.md\` exists in the repository +- If missing: Invoke the **threat-model-generation** skill to create one +- If exists: Check the file's last modified date + - If >90 days old: Regenerate and update the threat model + - If current: Use it as context for the security scan + +### Step 3: Security Scan +${scanInstructions} + +For each file: +- Invoke the **commit-security-scan** skill +- Look for STRIDE vulnerabilities (Spoofing, Tampering, Repudiation, Information Disclosure, Denial of Service, Elevation of Privilege) + +### Step 4: Validate Findings +- For each finding from Step 3, invoke the **vulnerability-validation** skill +- Assess: + - Reachability: Is the vulnerable code reachable from user input? + - Exploitability: How easy is it to exploit? + - Impact: What's the potential damage? +- Filter out false positives and findings below the severity threshold (${severityThreshold}) + +### Step 5: Generate Patches +- For each confirmed finding that can be auto-fixed: + - Invoke **security-review** skill to generate patches + - Apply the patch to the codebase + - Commit the fix with message: \`fix(security): [VULN-XXX] Brief description\` + +### Step 6: Generate Report +- Create directory: \`mkdir -p .factory/security/reports\` +- Write report to: \`.factory/security/reports/security-report-${date}.md\` + +### Step 7: Create PR +\`\`\`bash +git add . +git commit -m "fix(security): Security scan report - ${date}" +git push origin ${branchName} +gh pr create --title "fix(security): Security scan report - ${date} (N findings)" \\ + --body "## Security Scan Report + +See \`.factory/security/reports/security-report-${date}.md\` for details. + +### Summary +| Severity | Count | Auto-fixed | Manual Required | +|----------|-------|------------|-----------------| +| CRITICAL | X | X | X | +| HIGH | X | X | X | +| MEDIUM | X | X | X | +| LOW | X | X | X | + +${notifyTeam ? `cc ${notifyTeam}` : ""}" +\`\`\` + +## Report Format + +The report file should follow this structure: + +\`\`\`markdown +# Security Scan Report + +**Generated:** ${date} +**Scan Type:** ${scanTypeLabel} +**Repository:** ${repoFullName} +**Severity Threshold:** ${severityThreshold} + +## Executive Summary + +| Severity | Count | Auto-fixed | Manual Required | +|----------|-------|------------|-----------------| +| CRITICAL | X | X | X | +| HIGH | X | X | X | +| MEDIUM | X | X | X | +| LOW | X | X | X | + +**Total Findings:** X +**Auto-fixed:** X +**Manual Review Required:** X + +## Critical Findings + +### VULN-001: [Vulnerability Title] + +| Attribute | Value | +|-----------|-------| +| **Severity** | CRITICAL | +| **STRIDE Category** | [Tampering/Spoofing/etc] | +| **CWE** | CWE-XXX | +| **File** | path/to/file.ts:line | +| **Status** | Patched / Manual fix required | + +**Description:** +[Clear explanation of the vulnerability] + +**Evidence:** +\\\`\\\`\\\` +[Code snippet showing the vulnerability] +\\\`\\\`\\\` + +**Fix Applied:** (if auto-patched) +\\\`\\\`\\\`diff +- vulnerable code ++ secure code +\\\`\\\`\\\` + +**Recommended Fix:** (if manual) +[Step-by-step remediation guidance] + +--- + +## High Findings +[Same format as Critical] + +## Medium Findings +[Same format as Critical] + +## Low Findings +[Same format as Critical] + +## Appendix + +### Threat Model +- Version: [date or "newly generated"] +- Location: .factory/threat-model.md + +### Scan Metadata +- ${scanScope.type === "full" ? "Files" : "Commits"} Scanned: N +- Scan Duration: Xm Ys +- Skills Used: threat-model-generation, commit-security-scan, vulnerability-validation, security-review + +### References +- [CWE Database](https://cwe.mitre.org/) +- [STRIDE Threat Model](https://docs.microsoft.com/en-us/azure/security/develop/threat-modeling-tool-threats) +\`\`\` + +## Severity Definitions + +| Severity | Criteria | Examples | +|----------|----------|----------| +| **CRITICAL** | Immediately exploitable, high impact, no auth required | RCE, hardcoded secrets, auth bypass | +| **HIGH** | Exploitable with conditions, significant impact | SQL injection, stored XSS, IDOR | +| **MEDIUM** | Requires specific conditions, moderate impact | CSRF, info disclosure, missing rate limits | +| **LOW** | Difficult to exploit, low impact | Verbose errors, missing security headers | + +## Important Notes + +1. **Accuracy**: Only report high-confidence findings. False positives waste developer time. +2. **Patches**: Test all generated patches before committing. Ensure they don't break functionality. +3. **PR Description**: Update the PR body with actual finding counts before creating. +4. **Commit Messages**: Use semantic commit format: \`fix(security): [VULN-XXX] Description\` +`; +} diff --git a/src/create-prompt/templates/security-review-prompt.ts b/src/create-prompt/templates/security-review-prompt.ts new file mode 100644 index 0000000..6b294a8 --- /dev/null +++ b/src/create-prompt/templates/security-review-prompt.ts @@ -0,0 +1,171 @@ +import type { PreparedContext } from "../types"; + +export function generateSecurityReviewPrompt(context: PreparedContext): string { + const prNumber = context.eventData.isPR + ? context.eventData.prNumber + : context.githubContext && "entityNumber" in context.githubContext + ? String(context.githubContext.entityNumber) + : "unknown"; + + const repoFullName = context.repository; + const headRefName = context.prBranchData?.headRefName ?? "unknown"; + const headSha = context.prBranchData?.headRefOid ?? "unknown"; + const baseRefName = context.eventData.baseBranch ?? "unknown"; + + // Extract security configuration from context + const securityConfig = context.githubContext?.inputs; + const severityThreshold = + securityConfig?.securitySeverityThreshold ?? "medium"; + const blockOnCritical = securityConfig?.securityBlockOnCritical ?? true; + const blockOnHigh = securityConfig?.securityBlockOnHigh ?? false; + const notifyTeam = securityConfig?.securityNotifyTeam ?? ""; + + return `You are performing a security-focused code review for PR #${prNumber} in ${repoFullName}. +The gh CLI is installed and authenticated via GH_TOKEN. + +## Context +- Repo: ${repoFullName} +- PR Number: ${prNumber} +- PR Head Ref: ${headRefName} +- PR Head SHA: ${headSha} +- PR Base Ref: ${baseRefName} + +## Security Configuration +- Severity Threshold: ${severityThreshold} (only report findings at or above this level) +- Block on Critical: ${blockOnCritical} +- Block on High: ${blockOnHigh} +${notifyTeam ? `- Notify Team: ${notifyTeam} (mention on critical findings)` : ""} + +## Security Skills Available + +You have access to these Factory security skills (installed in ~/.factory/skills/): + +1. **threat-model-generation** - Generate STRIDE-based threat model for the repository +2. **commit-security-scan** - Scan code changes for security vulnerabilities +3. **vulnerability-validation** - Validate findings, assess exploitability, filter false positives +4. **security-review** - Comprehensive security review and patch generation + +## Review Workflow + +### Step 1: Threat Model Check +- Check if \`.factory/threat-model.md\` exists in the repository +- If missing: Invoke the **threat-model-generation** skill to create one, then commit it to the PR branch +- If exists: Check the file's last modified date + - If >90 days old: Post a warning comment suggesting regeneration, but proceed with scan + - If current: Use it as context for the security scan + +### Step 2: Security Scan +- Invoke the **commit-security-scan** skill on the PR diff +- Gather the PR diff using: \`gh pr diff ${prNumber} --repo ${repoFullName}\` +- Get file changes: \`gh api repos/${repoFullName}/pulls/${prNumber}/files --paginate\` +- Focus analysis on: + - New code introduced by this PR + - Modified code that may introduce vulnerabilities + - Changes that expose existing vulnerabilities + +### Step 3: Validate Findings +- For each finding from Step 2, invoke the **vulnerability-validation** skill +- Assess: + - Reachability: Is the vulnerable code reachable from user input? + - Exploitability: How easy is it to exploit? + - Impact: What's the potential damage? +- Filter out false positives and findings below the severity threshold (${severityThreshold}) + +### Step 4: Report & Patch +- For each confirmed finding at or above ${severityThreshold} severity: + - Post inline comment using \`github_inline_comment___create_inline_comment\` + - Include: severity, STRIDE category, CWE ID, clear explanation, suggested fix +- For auto-fixable issues: Invoke **security-review** skill to generate patches +- Commit any generated patches to the PR branch + +## Security Scope (STRIDE Categories) + +**Spoofing** (S): +- Weak authentication, session hijacking, token exposure + +**Tampering** (T): +- SQL/NoSQL/command injection, XSS, mass assignment, unsafe deserialization + +**Repudiation** (R): +- Missing audit logs, unsigned transactions + +**Information Disclosure** (I): +- IDOR, verbose errors, hardcoded secrets, sensitive data in logs + +**Denial of Service** (D): +- Missing rate limits, resource exhaustion, ReDoS + +**Elevation of Privilege** (E): +- Missing authorization checks, role manipulation, privilege escalation + +## Severity Definitions + +| Severity | Criteria | Examples | +|----------|----------|----------| +| **CRITICAL** | Immediately exploitable, high impact, no auth required | RCE, hardcoded secrets, auth bypass | +| **HIGH** | Exploitable with conditions, significant impact | SQL injection, stored XSS, IDOR | +| **MEDIUM** | Requires specific conditions, moderate impact | CSRF, info disclosure, missing rate limits | +| **LOW** | Difficult to exploit, low impact | Verbose errors, missing security headers | + +## Output Requirements + +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: +\`\`\`json +{ + "type": "security", + "findings": [ + { + "id": "SEC-001", + "severity": "CRITICAL|HIGH|MEDIUM|LOW", + "type": "SQL Injection", + "stride": "T", + "cwe": "CWE-89", + "file": "path/to/file.ts", + "line": 55, + "side": "RIGHT", + "description": "Brief description of the vulnerability", + "suggestion": "Optional code fix" + } + ], + "summary": "Brief overall summary", + "block_pr": ${blockOnCritical || blockOnHigh ? "true if CRITICAL/HIGH found" : "false"} +} +\`\`\` + +2. Update the tracking comment using \`github_comment___update_droid_comment\` + +## Summary Format (for tracking comment update) + +Use \`github_comment___update_droid_comment\` to update the tracking comment with this format: + +\`\`\`markdown +## 🔐 Security Review Summary + +| Severity | Count | +|----------|-------| +| 🚨 CRITICAL | X | +| 🔴 HIGH | X | +| 🟡 MEDIUM | X | +| 🟢 LOW | X | + +### Findings +| ID | Severity | Type | File | Line | Reference | +|----|----------|------|------|------|-----------| +| SEC-001 | CRITICAL | SQL Injection | auth.ts | 55 | [CWE-89](https://cwe.mitre.org/data/definitions/89.html) | +| SEC-002 | HIGH | XSS | client.ts | 98 | [CWE-79](https://cwe.mitre.org/data/definitions/79.html) | + +${notifyTeam ? `cc ${notifyTeam}` : ""} +\`\`\` + +## Accuracy Requirements + +- Base findings strictly on the current diff and repository context +- False positives are very costly—only report high-confidence findings +- If confidence is low, ask a clarifying question instead of asserting a vulnerability +- Never raise purely stylistic concerns +- Cap at 10 inline comments total +`; +} diff --git a/src/create-prompt/types.ts b/src/create-prompt/types.ts index 75af3a4..cb1d8df 100644 --- a/src/create-prompt/types.ts +++ b/src/create-prompt/types.ts @@ -2,7 +2,7 @@ import type { GitHubContext } from "../github/context"; export type CommonFields = { repository: string; - droidCommentId: string; + droidCommentId?: string; triggerPhrase: string; triggerUsername?: string; droidBranch?: string; diff --git a/src/entrypoints/collect-inputs.ts b/src/entrypoints/collect-inputs.ts index 6eb8b3b..442da29 100644 --- a/src/entrypoints/collect-inputs.ts +++ b/src/entrypoints/collect-inputs.ts @@ -26,6 +26,14 @@ export function collectActionInputsPresence(): void { experimental_allowed_domains: "", track_progress: "false", automatic_review: "false", + automatic_security_review: "false", + security_model: "", + security_severity_threshold: "medium", + security_block_on_critical: "true", + security_block_on_high: "false", + security_notify_team: "", + security_scan_schedule: "false", + security_scan_days: "7", }; const allInputsJson = process.env.ALL_INPUTS; diff --git a/src/github/api/queries/github.ts b/src/github/api/queries/github.ts index 2341a55..36efc22 100644 --- a/src/github/api/queries/github.ts +++ b/src/github/api/queries/github.ts @@ -123,3 +123,13 @@ export const USER_QUERY = ` } } `; + +export const REPO_DEFAULT_BRANCH_QUERY = ` + query($owner: String!, $repo: String!) { + repository(owner: $owner, name: $repo) { + defaultBranchRef { + name + } + } + } +`; diff --git a/src/github/context.ts b/src/github/context.ts index 9b4306d..ae52994 100644 --- a/src/github/context.ts +++ b/src/github/context.ts @@ -90,6 +90,14 @@ type BaseContext = { allowedNonWriteUsers: string; trackProgress: boolean; automaticReview: boolean; + automaticSecurityReview: boolean; + securityModel: string; + securitySeverityThreshold: string; + securityBlockOnCritical: boolean; + securityBlockOnHigh: boolean; + securityNotifyTeam: string; + securityScanSchedule: boolean; + securityScanDays: number; }; }; @@ -140,6 +148,16 @@ export function parseGitHubContext(): GitHubContext { allowedNonWriteUsers: process.env.ALLOWED_NON_WRITE_USERS ?? "", trackProgress: process.env.TRACK_PROGRESS === "true", automaticReview: process.env.AUTOMATIC_REVIEW === "true", + automaticSecurityReview: process.env.AUTOMATIC_SECURITY_REVIEW === "true", + securityModel: process.env.SECURITY_MODEL ?? "", + securitySeverityThreshold: + process.env.SECURITY_SEVERITY_THRESHOLD ?? "medium", + securityBlockOnCritical: + process.env.SECURITY_BLOCK_ON_CRITICAL !== "false", + securityBlockOnHigh: process.env.SECURITY_BLOCK_ON_HIGH === "true", + securityNotifyTeam: process.env.SECURITY_NOTIFY_TEAM ?? "", + securityScanSchedule: process.env.SECURITY_SCAN_SCHEDULE === "true", + securityScanDays: parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10), }, }; diff --git a/src/github/data/pr-fetcher.ts b/src/github/data/pr-fetcher.ts index 6cc04fb..28e7188 100644 --- a/src/github/data/pr-fetcher.ts +++ b/src/github/data/pr-fetcher.ts @@ -1,5 +1,5 @@ import type { Octokits } from "../api/client"; -import { PR_QUERY } from "../api/queries/github"; +import { PR_QUERY, REPO_DEFAULT_BRANCH_QUERY } from "../api/queries/github"; import type { GitHubPullRequest } from "../types"; /** @@ -58,3 +58,46 @@ export async function fetchPRBranchData({ throw new Error(`Failed to fetch PR branch data for PR #${prNumber}`); } } + +type RepoDefaultBranchQueryResponse = { + repository: { + defaultBranchRef: { + name: string; + } | null; + }; +}; + +/** + * Fetches the repository's default branch name. + * Used by security-scan which operates without a PR context. + */ +export async function fetchRepoDefaultBranch({ + octokits, + repository, +}: { + octokits: Octokits; + repository: { owner: string; repo: string }; +}): Promise { + try { + const result = await octokits.graphql( + REPO_DEFAULT_BRANCH_QUERY, + { + owner: repository.owner, + repo: repository.repo, + }, + ); + + if (!result.repository.defaultBranchRef) { + throw new Error( + `Default branch not found for ${repository.owner}/${repository.repo}`, + ); + } + + return result.repository.defaultBranchRef.name; + } catch (error) { + console.error(`Failed to fetch default branch:`, error); + throw new Error( + `Failed to fetch default branch for ${repository.owner}/${repository.repo}`, + ); + } +} diff --git a/src/github/operations/comments/common.ts b/src/github/operations/comments/common.ts index 0ff5429..d3ab2be 100644 --- a/src/github/operations/comments/common.ts +++ b/src/github/operations/comments/common.ts @@ -18,11 +18,19 @@ export function createBranchLink( return `\n[View branch](${branchUrl})`; } +export type CommentType = "default" | "security"; + export function createCommentBody( jobRunLink: string, branchLink: string = "", + type: CommentType = "default", ): string { - return `Droid is working… + const message = + type === "security" + ? "Droid is running a security check…" + : "Droid is working…"; + + return `${message} ${jobRunLink}${branchLink}`; } diff --git a/src/github/operations/comments/create-initial.ts b/src/github/operations/comments/create-initial.ts index 46f2047..2292fb8 100644 --- a/src/github/operations/comments/create-initial.ts +++ b/src/github/operations/comments/create-initial.ts @@ -6,7 +6,11 @@ */ import { appendFileSync } from "fs"; -import { createJobRunLink, createCommentBody } from "./common"; +import { + createJobRunLink, + createCommentBody, + type CommentType, +} from "./common"; import { isPullRequestReviewCommentEvent, isPullRequestEvent, @@ -19,11 +23,12 @@ const DROID_APP_BOT_ID = 209825114; export async function createInitialComment( octokit: Octokit, context: ParsedGitHubContext, + commentType: CommentType = "default", ) { const { owner, repo } = context.repository; const jobRunLink = createJobRunLink(owner, repo, context.runId); - const initialBody = createCommentBody(jobRunLink); + const initialBody = createCommentBody(jobRunLink, "", commentType); try { let response; diff --git a/src/github/utils/command-parser.test.ts b/src/github/utils/command-parser.test.ts index b0be23f..602a13b 100644 --- a/src/github/utils/command-parser.test.ts +++ b/src/github/utils/command-parser.test.ts @@ -27,6 +27,14 @@ const baseContext: Omit = { allowedNonWriteUsers: "", trackProgress: false, automaticReview: false, + automaticSecurityReview: false, + securityModel: "", + securitySeverityThreshold: "medium", + securityBlockOnCritical: true, + securityBlockOnHigh: false, + securityNotifyTeam: "", + securityScanSchedule: false, + securityScanDays: 7, }, entityNumber: 1, isPR: true, @@ -71,6 +79,61 @@ describe("Command Parser", () => { expect(result?.raw).toBe("@droid review"); }); + it("should detect @droid review security (combined)", () => { + const result = parseDroidCommand("@droid review security"); + expect(result?.command).toBe("review-security"); + expect(result?.raw).toBe("@droid review security"); + }); + + it("should detect @droid security review (combined)", () => { + const result = parseDroidCommand("@droid security review"); + expect(result?.command).toBe("review-security"); + expect(result?.raw).toBe("@droid security review"); + }); + + it("should be case insensitive for combined commands", () => { + const result = parseDroidCommand("@DROID REVIEW SECURITY"); + expect(result?.command).toBe("review-security"); + }); + + it("should prioritize combined over individual review", () => { + const result = parseDroidCommand("@droid review security please"); + expect(result?.command).toBe("review-security"); + }); + + it("should detect @droid security", () => { + const result = parseDroidCommand("Please @droid security this PR"); + expect(result?.command).toBe("security"); + expect(result?.raw).toBe("@droid security"); + }); + + it("should detect @droid security at end of text", () => { + const result = parseDroidCommand("Please run @droid security"); + expect(result?.command).toBe("security"); + expect(result?.raw).toBe("@droid security"); + }); + + it("should be case insensitive for @droid security", () => { + const result = parseDroidCommand("@DROID SECURITY"); + expect(result?.command).toBe("security"); + }); + + it("should detect @droid security --full", () => { + const result = parseDroidCommand("@droid security --full"); + expect(result?.command).toBe("security-full"); + expect(result?.raw).toBe("@droid security --full"); + }); + + it("should be case insensitive for @droid security --full", () => { + const result = parseDroidCommand("@DROID SECURITY --FULL"); + expect(result?.command).toBe("security-full"); + }); + + it("should prioritize security-full over security", () => { + const result = parseDroidCommand("@droid security --full this repo"); + expect(result?.command).toBe("security-full"); + }); + it("should prioritize specific commands over default", () => { // If text has both @droid fill and just @droid, should detect fill const result = parseDroidCommand("@droid please @droid fill this"); @@ -125,17 +188,14 @@ describe("Command Parser", () => { describe("extractCommandFromContext", () => { it("should extract from PR body", () => { - const context = createContext( - "pull_request", - { - action: "opened", - pull_request: { - body: "PR description\n\n@droid fill", - number: 1, - title: "PR", - }, - } as unknown as PullRequestEvent, - ); + const context = createContext("pull_request", { + action: "opened", + pull_request: { + body: "PR description\n\n@droid fill", + number: 1, + title: "PR", + }, + } as unknown as PullRequestEvent); const result = extractCommandFromContext(context); expect(result?.command).toBe("fill"); expect(result?.location).toBe("body"); @@ -160,20 +220,17 @@ describe("Command Parser", () => { }); it("should extract from issue comment", () => { - const context = createContext( - "issue_comment", - { - action: "created", - comment: { - body: "@droid fill please", - created_at: "2024-01-01T00:00:00Z", - }, - issue: { - number: 1, - pull_request: { url: "" }, - }, - } as unknown as IssueCommentEvent, - ); + const context = createContext("issue_comment", { + action: "created", + comment: { + body: "@droid fill please", + 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("fill"); expect(result?.location).toBe("comment"); @@ -181,19 +238,16 @@ describe("Command Parser", () => { }); it("should extract from PR review comment", () => { - const context = createContext( - "pull_request_review_comment", - { - action: "created", - comment: { - body: "Can you @droid review this section?", - created_at: "2024-01-01T00:00:00Z", - }, - pull_request: { - number: 1, - }, - } as unknown as PullRequestReviewCommentEvent, - ); + const context = createContext("pull_request_review_comment", { + action: "created", + comment: { + body: "Can you @droid review this section?", + created_at: "2024-01-01T00:00:00Z", + }, + pull_request: { + number: 1, + }, + } as unknown as PullRequestReviewCommentEvent); const result = extractCommandFromContext(context); expect(result?.command).toBe("review"); expect(result?.location).toBe("comment"); @@ -201,37 +255,62 @@ describe("Command Parser", () => { }); it("should extract from PR review body", () => { - const context = createContext( - "pull_request_review", - { - action: "submitted", - review: { - body: "LGTM but @droid fill the description", - submitted_at: "2024-01-01T00:00:00Z", - }, - pull_request: { - number: 1, - }, - } as unknown as PullRequestReviewEvent, - ); + const context = createContext("pull_request_review", { + action: "submitted", + review: { + body: "LGTM but @droid fill the description", + submitted_at: "2024-01-01T00:00:00Z", + }, + pull_request: { + number: 1, + }, + } as unknown as PullRequestReviewEvent); const result = extractCommandFromContext(context); expect(result?.command).toBe("fill"); expect(result?.location).toBe("comment"); expect(result?.timestamp).toBe("2024-01-01T00:00:00Z"); }); + it("should extract security from PR body", () => { + const context = createContext("pull_request", { + action: "opened", + pull_request: { + body: "PR description\n\n@droid security", + number: 1, + title: "PR", + }, + } as unknown as PullRequestEvent); + const result = extractCommandFromContext(context); + expect(result?.command).toBe("security"); + expect(result?.location).toBe("body"); + }); + + it("should extract security-full from issue comment", () => { + const context = createContext("issue_comment", { + action: "created", + comment: { + body: "@droid security --full", + 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("security-full"); + expect(result?.location).toBe("comment"); + }); + it("should return null for events without commands", () => { - const context = createContext( - "pull_request", - { - action: "opened", - pull_request: { - body: "Regular PR description", - number: 1, - title: "PR", - }, - } as unknown as PullRequestEvent, - ); + const context = createContext("pull_request", { + action: "opened", + pull_request: { + body: "Regular PR description", + number: 1, + title: "PR", + }, + } as unknown as PullRequestEvent); const result = extractCommandFromContext(context); expect(result).toBeNull(); }); @@ -247,17 +326,14 @@ describe("Command Parser", () => { }); it("should handle missing body gracefully", () => { - const context = createContext( - "pull_request", - { - action: "opened", - pull_request: { - body: null, - number: 1, - title: "PR", - }, - } as unknown as PullRequestEvent, - ); + const context = createContext("pull_request", { + action: "opened", + pull_request: { + body: null, + number: 1, + title: "PR", + }, + } as unknown as PullRequestEvent); const result = extractCommandFromContext(context); expect(result).toBeNull(); }); @@ -273,20 +349,17 @@ describe("Command Parser", () => { }); it("should extract default command when no specific command", () => { - const context = createContext( - "issue_comment", - { - action: "created", - comment: { - body: "@droid can you help with this?", - created_at: "2024-01-01T00:00:00Z", - }, - issue: { - number: 1, - pull_request: { url: "" }, - }, - } as unknown as IssueCommentEvent, - ); + const context = createContext("issue_comment", { + action: "created", + comment: { + body: "@droid can you help with this?", + 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("default"); expect(result?.location).toBe("comment"); diff --git a/src/github/utils/command-parser.ts b/src/github/utils/command-parser.ts index a1250ca..182cac5 100644 --- a/src/github/utils/command-parser.ts +++ b/src/github/utils/command-parser.ts @@ -4,12 +4,18 @@ import type { GitHubContext } from "../context"; -export type DroidCommand = 'fill' | 'review' | 'default'; +export type DroidCommand = + | "fill" + | "review" + | "security" + | "review-security" + | "security-full" + | "default"; export interface ParsedCommand { command: DroidCommand; raw: string; - location: 'body' | 'comment'; + location: "body" | "comment"; timestamp?: string | null; } @@ -27,9 +33,22 @@ export function parseDroidCommand(text: string): ParsedCommand | null { const fillMatch = text.match(/@droid\s+fill/i); if (fillMatch) { return { - command: 'fill', + command: "fill", raw: fillMatch[0], - location: 'body', // Will be set by caller + location: "body", // Will be set by caller + }; + } + + // Check for @droid review security OR @droid security review (both reviews) + // Must check before individual review/security to avoid false matches + const combinedMatch = text.match( + /@droid\s+(?:review\s+security|security\s+review)/i, + ); + if (combinedMatch) { + return { + command: "review-security", + raw: combinedMatch[0], + location: "body", // Will be set by caller }; } @@ -37,9 +56,30 @@ export function parseDroidCommand(text: string): ParsedCommand | null { const reviewMatch = text.match(/@droid\s+review/i); if (reviewMatch) { return { - command: 'review', + command: "review", raw: reviewMatch[0], - location: 'body', // Will be set by caller + location: "body", // Will be set by caller + }; + } + + // Check for @droid security --full command (case insensitive) + const securityFullMatch = text.match(/@droid\s+security\s+--full/i); + if (securityFullMatch) { + return { + command: "security-full", + raw: securityFullMatch[0], + location: "body", // Will be set by caller + }; + } + + // Check for @droid security command (case insensitive) + // Must check after security-full to avoid false matches + const securityMatch = text.match(/@droid\s+security(?:\s|$|[^-\w])/i); + if (securityMatch) { + return { + command: "security", + raw: securityMatch[0].trim(), + location: "body", // Will be set by caller }; } @@ -47,9 +87,9 @@ export function parseDroidCommand(text: string): ParsedCommand | null { const droidMatch = text.match(/@droid/i); if (droidMatch) { return { - command: 'default', + command: "default", raw: droidMatch[0], - location: 'body', // Will be set by caller + location: "body", // Will be set by caller }; } @@ -61,43 +101,48 @@ export function parseDroidCommand(text: string): ParsedCommand | null { * @param context The GitHub context from the event * @returns ParsedCommand with location info, or null if no command found */ -export function extractCommandFromContext(context: GitHubContext): ParsedCommand | null { +export function extractCommandFromContext( + context: GitHubContext, +): ParsedCommand | null { // Handle missing payload if (!context.payload) { return null; } // Check PR body for commands (pull_request events) - if (context.eventName === 'pull_request' && 'pull_request' in context.payload) { + if ( + context.eventName === "pull_request" && + "pull_request" in context.payload + ) { const body = context.payload.pull_request.body; if (body) { const command = parseDroidCommand(body); if (command) { - return { ...command, location: 'body' }; + return { ...command, location: "body" }; } } } // Check issue body for commands (issues events) - if (context.eventName === 'issues' && 'issue' in context.payload) { + if (context.eventName === "issues" && "issue" in context.payload) { const body = context.payload.issue.body; if (body) { const command = parseDroidCommand(body); if (command) { - return { ...command, location: 'body' }; + return { ...command, location: "body" }; } } } // Check comment body for commands (issue_comment events) - if (context.eventName === 'issue_comment' && 'comment' in context.payload) { + if (context.eventName === "issue_comment" && "comment" in context.payload) { const comment = context.payload.comment; if (comment.body) { const command = parseDroidCommand(comment.body); if (command) { return { ...command, - location: 'comment', + location: "comment", timestamp: comment.created_at, }; } @@ -105,14 +150,17 @@ export function extractCommandFromContext(context: GitHubContext): ParsedCommand } // Check review comment body (pull_request_review_comment events) - if (context.eventName === 'pull_request_review_comment' && 'comment' in context.payload) { + if ( + context.eventName === "pull_request_review_comment" && + "comment" in context.payload + ) { const comment = context.payload.comment; if (comment.body) { const command = parseDroidCommand(comment.body); if (command) { return { ...command, - location: 'comment', + location: "comment", timestamp: comment.created_at, }; } @@ -120,14 +168,17 @@ export function extractCommandFromContext(context: GitHubContext): ParsedCommand } // Check review body (pull_request_review events) - if (context.eventName === 'pull_request_review' && 'review' in context.payload) { + if ( + context.eventName === "pull_request_review" && + "review" in context.payload + ) { const review = context.payload.review; if (review.body) { const command = parseDroidCommand(review.body); if (command) { return { ...command, - location: 'comment', + location: "comment", timestamp: review.submitted_at, }; } diff --git a/src/github/validation/trigger-commands.test.ts b/src/github/validation/trigger-commands.test.ts index 1d44a46..aa0197d 100644 --- a/src/github/validation/trigger-commands.test.ts +++ b/src/github/validation/trigger-commands.test.ts @@ -9,8 +9,9 @@ import type { PullRequestReviewEvent, } from "@octokit/webhooks-types"; -type ContextOverrides = - Partial> & { payload?: unknown }; +type ContextOverrides = Partial> & { + payload?: unknown; +}; const defaultPayload = { action: "created", @@ -27,7 +28,9 @@ const defaultPayload = { } as unknown as IssueCommentEvent; // Helper function to create a mock context -function createMockContext(overrides: ContextOverrides = {}): ParsedGitHubContext { +function createMockContext( + overrides: ContextOverrides = {}, +): ParsedGitHubContext { return { runId: "run-1", eventName: "issue_comment", @@ -50,6 +53,7 @@ function createMockContext(overrides: ContextOverrides = {}): ParsedGitHubContex botName: "droid[bot]", trackProgress: false, automaticReview: false, + automaticSecurityReview: false, useStickyComment: false, }, payload: defaultPayload, @@ -70,7 +74,7 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as PullRequestEvent, }); - + expect(checkContainsTrigger(context)).toBe(true); }); @@ -84,7 +88,7 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as IssueCommentEvent, }); - + expect(checkContainsTrigger(context)).toBe(true); }); @@ -101,7 +105,7 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as PullRequestReviewCommentEvent, }); - + expect(checkContainsTrigger(context)).toBe(true); }); @@ -118,7 +122,21 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as PullRequestReviewEvent, }); - + + expect(checkContainsTrigger(context)).toBe(true); + }); + + it("should trigger on @droid security in issue comment", () => { + const context = createMockContext({ + eventName: "issue_comment", + eventAction: "created", + payload: { + comment: { + body: "Can you @droid security this PR?", + }, + } as unknown as IssueCommentEvent, + }); + expect(checkContainsTrigger(context)).toBe(true); }); @@ -132,7 +150,7 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as IssueCommentEvent, }); - + // This should still trigger because of the existing trigger phrase logic // but now it will be handled as default command expect(checkContainsTrigger(context)).toBe(true); @@ -148,7 +166,7 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as IssueCommentEvent, }); - + expect(checkContainsTrigger(context)).toBe(true); }); @@ -162,7 +180,7 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as IssueCommentEvent, }); - + expect(checkContainsTrigger(context)).toBe(false); }); @@ -179,7 +197,7 @@ describe("checkContainsTrigger with commands", () => { }, } as unknown as IssuesEvent, }); - + expect(checkContainsTrigger(context)).toBe(true); }); }); diff --git a/src/github/validation/trigger.ts b/src/github/validation/trigger.ts index 51e8d25..a39ce0d 100644 --- a/src/github/validation/trigger.ts +++ b/src/github/validation/trigger.ts @@ -18,8 +18,10 @@ export function checkContainsTrigger(context: ParsedGitHubContext): boolean { // Check for specific @droid commands (fill, review) const command = extractCommandFromContext(context); - if (command && ['fill', 'review'].includes(command.command)) { - console.log(`Detected @droid ${command.command} command, triggering action`); + if (command && ["fill", "review", "security"].includes(command.command)) { + console.log( + `Detected @droid ${command.command} command, triggering action`, + ); return true; } diff --git a/src/tag/commands/security-review.ts b/src/tag/commands/security-review.ts new file mode 100644 index 0000000..4033217 --- /dev/null +++ b/src/tag/commands/security-review.ts @@ -0,0 +1,128 @@ +import * as core from "@actions/core"; +import type { GitHubContext } from "../../github/context"; +import { fetchPRBranchData } from "../../github/data/pr-fetcher"; +import { createPrompt } 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"; +import { isEntityContext } from "../../github/context"; +import { generateSecurityReviewPrompt } from "../../create-prompt/templates/security-review-prompt"; +import type { Octokits } from "../../github/api/client"; +import type { PrepareResult } from "../../prepare/types"; + +type SecurityReviewCommandOptions = { + context: GitHubContext; + octokit: Octokits; + githubToken: string; + trackingCommentId?: number; +}; + +export async function prepareSecurityReviewMode({ + context, + octokit, + githubToken, + trackingCommentId, +}: SecurityReviewCommandOptions): Promise { + if (!isEntityContext(context)) { + throw new Error("Security review command requires an entity event context"); + } + + if (!context.isPR) { + throw new Error( + "Security review 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, + }; + + await createPrompt({ + githubContext: context, + commentId, + baseBranch: branchInfo.baseBranch, + droidBranch: branchInfo.droidBranch, + prBranchData: { + headRefName: prData.headRefName, + headRefOid: prData.headRefOid, + }, + generatePrompt: generateSecurityReviewPrompt, + }); + core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-security-review"); + + // Signal that security skills should be installed + core.setOutput("install_security_skills", "true"); + + 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", + "github_comment___update_droid_comment", + "github_inline_comment___create_inline_comment", + ]; + + const reviewTools = [ + "github_pr___list_review_comments", + "github_pr___submit_review", + "github_pr___delete_comment", + "github_pr___minimize_comment", + "github_pr___reply_to_comment", + "github_pr___resolve_review_thread", + ]; + + const allowedTools = Array.from( + new Set([...baseTools, ...reviewTools, ...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(",")}"`); + + // Add model override if specified (prefer SECURITY_MODEL, fallback to REVIEW_MODEL) + const securityModel = + process.env.SECURITY_MODEL?.trim() || process.env.REVIEW_MODEL?.trim(); + if (securityModel) { + droidArgParts.push(`--model "${securityModel}"`); + } + + if (normalizedUserArgs) { + droidArgParts.push(normalizedUserArgs); + } + + core.setOutput("droid_args", droidArgParts.join(" ").trim()); + core.setOutput("mcp_tools", mcpTools); + + return { + commentId, + branchInfo, + mcpTools, + }; +} diff --git a/src/tag/commands/security-scan.ts b/src/tag/commands/security-scan.ts new file mode 100644 index 0000000..5f474fc --- /dev/null +++ b/src/tag/commands/security-scan.ts @@ -0,0 +1,107 @@ +import * as core from "@actions/core"; +import type { GitHubContext } from "../../github/context"; +import { fetchRepoDefaultBranch } from "../../github/data/pr-fetcher"; +import { createPrompt } from "../../create-prompt"; +import { prepareMcpTools } from "../../mcp/install-mcp-server"; +import { normalizeDroidArgs, parseAllowedTools } from "../../utils/parse-tools"; +import { isEntityContext } from "../../github/context"; +import { generateSecurityReportPrompt } from "../../create-prompt/templates/security-report-prompt"; +import type { Octokits } from "../../github/api/client"; +import type { PrepareResult } from "../../prepare/types"; + +export type ScanScope = { type: "full" } | { type: "scheduled"; days: number }; + +type SecurityScanCommandOptions = { + context: GitHubContext; + octokit: Octokits; + githubToken: string; + scanScope: ScanScope; +}; + +export async function prepareSecurityScanMode({ + context, + octokit, + githubToken, + scanScope, +}: SecurityScanCommandOptions): Promise { + if (!isEntityContext(context)) { + throw new Error("Security scan command requires an entity event context"); + } + + // Fetch the repository's default branch (could be main, master, develop, etc.) + const defaultBranch = await fetchRepoDefaultBranch({ + octokits: octokit, + repository: context.repository, + }); + + const date = new Date().toISOString().split("T")[0]; + const branchName = `droid/security-report-${date}`; + + const branchInfo = { + baseBranch: defaultBranch, + droidBranch: branchName, + currentBranch: branchName, + }; + + await createPrompt({ + githubContext: context, + baseBranch: branchInfo.baseBranch, + droidBranch: branchInfo.droidBranch, + generatePrompt: (ctx) => + generateSecurityReportPrompt(ctx, scanScope, branchName), + }); + core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-security-scan"); + + // Signal that security skills should be installed + core.setOutput("install_security_skills", "true"); + + 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", + "github_comment___update_droid_comment", + ]; + + const allowedTools = Array.from( + new Set([...baseTools, ...userAllowedMCPTools]), + ); + + const mcpTools = await prepareMcpTools({ + githubToken, + owner: context.repository.owner, + repo: context.repository.repo, + allowedTools, + mode: "tag", + context, + }); + + const droidArgParts: string[] = []; + droidArgParts.push(`--enabled-tools "${allowedTools.join(",")}"`); + + // Add model override if specified (prefer SECURITY_MODEL, fallback to REVIEW_MODEL) + const securityModel = + process.env.SECURITY_MODEL?.trim() || process.env.REVIEW_MODEL?.trim(); + if (securityModel) { + droidArgParts.push(`--model "${securityModel}"`); + } + + if (normalizedUserArgs) { + droidArgParts.push(normalizedUserArgs); + } + + core.setOutput("droid_args", droidArgParts.join(" ").trim()); + core.setOutput("mcp_tools", mcpTools); + + return { + branchInfo, + mcpTools, + }; +} diff --git a/src/tag/index.ts b/src/tag/index.ts index 7f036c1..a231476 100644 --- a/src/tag/index.ts +++ b/src/tag/index.ts @@ -5,6 +5,8 @@ import { isEntityContext } from "../github/context"; import { extractCommandFromContext } from "../github/utils/command-parser"; import { prepareFillMode } from "./commands/fill"; import { prepareReviewMode } from "./commands/review"; +import { prepareSecurityReviewMode } from "./commands/security-review"; +import { prepareSecurityScanMode } from "./commands/security-scan"; import type { GitHubContext } from "../github/context"; import type { PrepareResult } from "../prepare/types"; import type { Octokits } from "../github/api/client"; @@ -13,7 +15,10 @@ export function shouldTriggerTag(context: GitHubContext): boolean { if (!isEntityContext(context)) { return false; } - if (context.inputs.automaticReview) { + if ( + context.inputs.automaticReview || + context.inputs.automaticSecurityReview + ) { return context.isPR; } return checkContainsTrigger(context); @@ -36,15 +41,31 @@ export async function prepareTagExecution({ await checkHumanActor(octokit.rest, context); - const commentData = await createInitialComment(octokit.rest, context); - const commentId = commentData.id; - if (context.inputs.automaticReview && !context.isPR) { throw new Error("automatic_review requires a pull request context"); } + if (context.inputs.automaticSecurityReview && !context.isPR) { + throw new Error( + "automatic_security_review requires a pull request context", + ); + } + const commandContext = extractCommandFromContext(context); + // Determine if this is a security-related command for the initial comment + const isSecurityCommand = + context.inputs.automaticSecurityReview || + commandContext?.command === "security" || + commandContext?.command === "security-full"; + + const commentData = await createInitialComment( + octokit.rest, + context, + isSecurityCommand ? "security" : "default", + ); + const commentId = commentData.id; + if (context.inputs.automaticReview) { return prepareReviewMode({ context, @@ -54,6 +75,15 @@ export async function prepareTagExecution({ }); } + if (context.inputs.automaticSecurityReview) { + return prepareSecurityReviewMode({ + context, + octokit, + githubToken, + trackingCommentId: commentId, + }); + } + if (commandContext?.command === "fill") { return prepareFillMode({ context, @@ -63,6 +93,24 @@ export async function prepareTagExecution({ }); } + if (commandContext?.command === "security") { + return prepareSecurityReviewMode({ + context, + octokit, + githubToken, + trackingCommentId: commentId, + }); + } + + if (commandContext?.command === "security-full") { + return prepareSecurityScanMode({ + context, + octokit, + githubToken, + scanScope: { type: "full" }, + }); + } + if ( commandContext?.command === "review" || !commandContext || diff --git a/test/create-prompt.test.ts b/test/create-prompt.test.ts index 4d9543b..f4176ff 100644 --- a/test/create-prompt.test.ts +++ b/test/create-prompt.test.ts @@ -26,6 +26,14 @@ describe("prepareContext", () => { allowedNonWriteUsers: "", trackProgress: false, automaticReview: false, + automaticSecurityReview: false, + securityModel: "", + securitySeverityThreshold: "medium", + securityBlockOnCritical: true, + securityBlockOnHigh: false, + securityNotifyTeam: "", + securityScanSchedule: false, + securityScanDays: 7, }, } as const; diff --git a/test/create-prompt/templates/fill-prompt.test.ts b/test/create-prompt/templates/fill-prompt.test.ts index 5d42bf4..baff84d 100644 --- a/test/create-prompt/templates/fill-prompt.test.ts +++ b/test/create-prompt/templates/fill-prompt.test.ts @@ -2,7 +2,9 @@ import { describe, expect, it } from "bun:test"; import { generateFillPrompt } from "../../../src/create-prompt/templates/fill-prompt"; import type { PreparedContext } from "../../../src/create-prompt/types"; -function createBaseContext(overrides: Partial = {}): PreparedContext { +function createBaseContext( + overrides: Partial = {}, +): PreparedContext { return { repository: "test-owner/test-repo", droidCommentId: "123", @@ -26,7 +28,9 @@ describe("generateFillPrompt", () => { const prompt = generateFillPrompt(context); expect(prompt).toContain("Procedure:"); - expect(prompt).toContain("gh pr view 42 --repo test-owner/test-repo --json title,body"); + expect(prompt).toContain( + "gh pr view 42 --repo test-owner/test-repo --json title,body", + ); expect(prompt).toContain("gh pr diff 42 --repo test-owner/test-repo"); expect(prompt).toContain("github_pr___update_pr_description"); expect(prompt).toContain("Do not proceed if required commands fail"); diff --git a/test/create-prompt/templates/security-report-prompt.test.ts b/test/create-prompt/templates/security-report-prompt.test.ts new file mode 100644 index 0000000..f2698d3 --- /dev/null +++ b/test/create-prompt/templates/security-report-prompt.test.ts @@ -0,0 +1,156 @@ +import { describe, expect, test } from "bun:test"; +import { generateSecurityReportPrompt } from "../../../src/create-prompt/templates/security-report-prompt"; +import type { PreparedContext } from "../../../src/create-prompt/types"; + +describe("generateSecurityReportPrompt", () => { + const baseContext: PreparedContext = { + repository: "test-owner/test-repo", + triggerPhrase: "@droid", + eventData: { + eventName: "issue_comment", + commentId: "123", + issueNumber: "1", + isPR: false, + baseBranch: "main", + droidBranch: "droid/security-report-2024-01-15", + commentBody: "@droid security --full", + }, + githubContext: { + runId: "1234567890", + eventName: "issue_comment", + eventAction: "created", + repository: { + owner: "test-owner", + repo: "test-repo", + full_name: "test-owner/test-repo", + }, + actor: "test-user", + payload: {} as any, + entityNumber: 1, + isPR: false, + inputs: { + triggerPhrase: "@droid", + assigneeTrigger: "", + labelTrigger: "", + useStickyComment: false, + allowedBots: "", + allowedNonWriteUsers: "", + trackProgress: false, + automaticReview: false, + automaticSecurityReview: false, + securityModel: "", + securitySeverityThreshold: "high", + securityBlockOnCritical: true, + securityBlockOnHigh: false, + securityNotifyTeam: "@org/security-team", + securityScanSchedule: false, + securityScanDays: 7, + }, + }, + }; + + test("includes scan configuration for full repository scan", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "full" }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain("full repository"); + expect(prompt).toContain("Entire repository"); + expect(prompt).toContain("droid/security-report-2024-01-15"); + expect(prompt).toContain(".factory/security/reports/security-report-"); + }); + + test("includes scan configuration for scheduled scan", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "scheduled", days: 7 }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain("weekly scheduled"); + expect(prompt).toContain("Last 7 days of commits"); + expect(prompt).toContain("git log --since="); + }); + + test("includes security skills workflow", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "full" }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain("threat-model-generation"); + expect(prompt).toContain("commit-security-scan"); + expect(prompt).toContain("vulnerability-validation"); + expect(prompt).toContain("security-review"); + }); + + test("includes PR creation instructions", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "full" }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain("git checkout -b"); + expect(prompt).toContain("git push origin"); + expect(prompt).toContain("gh pr create"); + expect(prompt).toContain("fix(security):"); + }); + + test("includes report format template", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "full" }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain("# Security Scan Report"); + expect(prompt).toContain("Executive Summary"); + expect(prompt).toContain( + "| Severity | Count | Auto-fixed | Manual Required |", + ); + expect(prompt).toContain("VULN-001"); + expect(prompt).toContain("STRIDE"); + expect(prompt).toContain("CWE"); + }); + + test("includes severity definitions", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "full" }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain("CRITICAL"); + expect(prompt).toContain("HIGH"); + expect(prompt).toContain("MEDIUM"); + expect(prompt).toContain("LOW"); + expect(prompt).toContain("Immediately exploitable"); + }); + + test("includes security configuration from context", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "full" }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain("Severity Threshold: high"); + expect(prompt).toContain("@org/security-team"); + }); + + test("includes threat model check instructions", () => { + const prompt = generateSecurityReportPrompt( + baseContext, + { type: "full" }, + "droid/security-report-2024-01-15", + ); + + expect(prompt).toContain(".factory/threat-model.md"); + expect(prompt).toContain("Threat Model Check"); + expect(prompt).toContain("90 days old"); + }); +}); diff --git a/test/create-prompt/templates/security-review-prompt.test.ts b/test/create-prompt/templates/security-review-prompt.test.ts new file mode 100644 index 0000000..5014fcd --- /dev/null +++ b/test/create-prompt/templates/security-review-prompt.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from "bun:test"; +import { generateSecurityReviewPrompt } from "../../../src/create-prompt/templates/security-review-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 security-review", + }, + githubContext: undefined, + ...overrides, + } as PreparedContext; +} + +describe("generateSecurityReviewPrompt", () => { + it("includes security context and skill workflow", () => { + const prompt = generateSecurityReviewPrompt(createBaseContext()); + + expect(prompt).toContain("security-focused code review"); + expect(prompt).toContain("## Security Skills Available"); + expect(prompt).toContain("threat-model-generation"); + expect(prompt).toContain("commit-security-scan"); + expect(prompt).toContain("vulnerability-validation"); + expect(prompt).toContain("security-review"); + expect(prompt).toContain("## Review Workflow"); + expect(prompt).toContain("gh pr diff 42 --repo test-owner/test-repo"); + expect(prompt).toContain( + "gh api repos/test-owner/test-repo/pulls/42/files", + ); + expect(prompt).toContain("security-review-results.json"); + expect(prompt).toContain("Do NOT post inline comments"); + }); + + it("lists STRIDE security categories", () => { + const prompt = generateSecurityReviewPrompt(createBaseContext()); + + expect(prompt).toContain("Spoofing"); + expect(prompt).toContain("Tampering"); + expect(prompt).toContain("Repudiation"); + expect(prompt).toContain("Information Disclosure"); + expect(prompt).toContain("Denial of Service"); + expect(prompt).toContain("Elevation of Privilege"); + }); + + it("includes severity definitions", () => { + const prompt = generateSecurityReviewPrompt(createBaseContext()); + + expect(prompt).toContain("CRITICAL"); + expect(prompt).toContain("HIGH"); + expect(prompt).toContain("MEDIUM"); + expect(prompt).toContain("LOW"); + }); + + it("includes security configuration from context", () => { + const contextWithConfig = createBaseContext({ + githubContext: { + inputs: { + securitySeverityThreshold: "high", + securityBlockOnCritical: true, + securityBlockOnHigh: true, + securityNotifyTeam: "@org/security-team", + }, + } as any, + }); + + const prompt = generateSecurityReviewPrompt(contextWithConfig); + + expect(prompt).toContain("Severity Threshold: high"); + expect(prompt).toContain("Block on Critical: true"); + expect(prompt).toContain("Block on High: true"); + expect(prompt).toContain("@org/security-team"); + }); +}); diff --git a/test/mockContext.ts b/test/mockContext.ts index 657b118..4c197a7 100644 --- a/test/mockContext.ts +++ b/test/mockContext.ts @@ -19,6 +19,14 @@ const defaultInputs = { allowedNonWriteUsers: "", trackProgress: false, automaticReview: false, + automaticSecurityReview: false, + securityModel: "", + securitySeverityThreshold: "medium", + securityBlockOnCritical: true, + securityBlockOnHigh: false, + securityNotifyTeam: "", + securityScanSchedule: false, + securityScanDays: 7, }; const defaultRepository = { diff --git a/test/modes/tag.test.ts b/test/modes/tag.test.ts index 1cd012f..6a4ff72 100644 --- a/test/modes/tag.test.ts +++ b/test/modes/tag.test.ts @@ -52,4 +52,17 @@ describe("shouldTriggerTag", () => { expect(shouldTriggerTag(contextWithAutomaticReview)).toBe(true); }); + + test("returns true for PR contexts when automaticSecurityReview is enabled", () => { + const contextWithAutomaticSecurityReview = createMockContext({ + eventName: "issue_comment", + isPR: true, + inputs: { + ...createMockContext().inputs, + automaticSecurityReview: true, + }, + }); + + expect(shouldTriggerTag(contextWithAutomaticSecurityReview)).toBe(true); + }); }); diff --git a/test/modes/tag/security-review-command.test.ts b/test/modes/tag/security-review-command.test.ts new file mode 100644 index 0000000..66a2b88 --- /dev/null +++ b/test/modes/tag/security-review-command.test.ts @@ -0,0 +1,335 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import * as core from "@actions/core"; +import { prepareSecurityReviewMode } from "../../../src/tag/commands/security-review"; +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 comments from "../../../src/github/operations/comments/create-initial"; + +const MOCK_PR_DATA = { + baseRefName: "main", + headRefName: "feature/security-review", + headRefOid: "123abc", +} as const; + +describe("prepareSecurityReviewMode", () => { + const originalArgs = process.env.DROID_ARGS; + const originalReviewModel = process.env.REVIEW_MODEL; + const originalSecurityModel = process.env.SECURITY_MODEL; + let fetchPRSpy: ReturnType; + let promptSpy: ReturnType; + let mcpSpy: ReturnType; + let setOutputSpy: ReturnType; + let createInitialSpy: ReturnType; + let exportVariableSpy: ReturnType; + + beforeEach(() => { + process.env.DROID_ARGS = ""; + delete process.env.REVIEW_MODEL; + delete process.env.SECURITY_MODEL; + + fetchPRSpy = spyOn(prFetcher, "fetchPRBranchData").mockResolvedValue({ + baseRefName: MOCK_PR_DATA.baseRefName, + headRefName: MOCK_PR_DATA.headRefName, + headRefOid: MOCK_PR_DATA.headRefOid, + }); + + promptSpy = spyOn(promptModule, "createPrompt").mockResolvedValue(); + mcpSpy = spyOn(mcpInstaller, "prepareMcpTools").mockResolvedValue( + "mock-config", + ); + setOutputSpy = spyOn(core, "setOutput").mockImplementation(() => {}); + createInitialSpy = spyOn( + comments, + "createInitialComment", + ).mockResolvedValue({ id: 777 } as any); + exportVariableSpy = spyOn(core, "exportVariable").mockImplementation( + () => {}, + ); + }); + + afterEach(() => { + fetchPRSpy.mockRestore(); + promptSpy.mockRestore(); + mcpSpy.mockRestore(); + setOutputSpy.mockRestore(); + createInitialSpy.mockRestore(); + exportVariableSpy.mockRestore(); + + process.env.DROID_ARGS = originalArgs; + if (originalReviewModel !== undefined) { + process.env.REVIEW_MODEL = originalReviewModel; + } else { + delete process.env.REVIEW_MODEL; + } + if (originalSecurityModel !== undefined) { + process.env.SECURITY_MODEL = originalSecurityModel; + } else { + delete process.env.SECURITY_MODEL; + } + }); + + it("prepares security review flow with limited toolset when tracking comment exists", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 101, + body: "@droid security-review", + }, + } as any, + entityNumber: 24, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + const result = await prepareSecurityReviewMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 555, + }); + + expect(fetchPRSpy).toHaveBeenCalledWith({ + octokits: octokit, + repository: context.repository, + prNumber: 24, + }); + expect(promptSpy).toHaveBeenCalled(); + expect(mcpSpy).toHaveBeenCalledWith( + expect.objectContaining({ + allowedTools: expect.arrayContaining([ + "Execute", + "github_comment___update_droid_comment", + "github_inline_comment___create_inline_comment", + "github_pr___list_review_comments", + "github_pr___submit_review", + "github_pr___resolve_review_thread", + ]), + }), + ); + expect(createInitialSpy).not.toHaveBeenCalled(); + expect(result.commentId).toBe(555); + expect(result.branchInfo.baseBranch).toBe("main"); + expect(result.branchInfo.currentBranch).toBe("feature/security-review"); + expect(result.branchInfo.droidBranch).toBeUndefined(); + + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).toContain("Execute"); + [ + "github_comment___update_droid_comment", + "github_inline_comment___create_inline_comment", + "github_pr___list_review_comments", + "github_pr___submit_review", + "github_pr___delete_comment", + "github_pr___minimize_comment", + "github_pr___reply_to_comment", + "github_pr___resolve_review_thread", + ].forEach((tool) => { + expect(droidArgsCall?.[1]).toContain(tool); + }); + + expect(exportVariableSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + "droid-security-review", + ); + }); + + it("creates tracking comment when not provided", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 102, + body: "@droid security-review", + }, + } as any, + entityNumber: 25, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + const result = await prepareSecurityReviewMode({ + context, + octokit, + githubToken: "token", + }); + + expect(createInitialSpy).toHaveBeenCalled(); + expect(result.commentId).toBe(777); + }); + + it("throws when invoked on non-PR context", async () => { + const context = createMockContext({ isPR: false }); + + await expect( + prepareSecurityReviewMode({ + context, + octokit: { rest: {}, graphql: () => {} } as any, + githubToken: "token", + }), + ).rejects.toThrow( + "Security review command is only supported on pull requests", + ); + }); + + it("adds --model flag when REVIEW_MODEL is set", async () => { + process.env.REVIEW_MODEL = "claude-sonnet-4-5-20250929"; + + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 103, + body: "@droid security-review", + }, + } as any, + entityNumber: 26, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareSecurityReviewMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 556, + }); + + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).toContain( + '--model "claude-sonnet-4-5-20250929"', + ); + }); + + it("does not add --model flag when REVIEW_MODEL is empty", async () => { + process.env.REVIEW_MODEL = ""; + + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 104, + body: "@droid security-review", + }, + } as any, + entityNumber: 27, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareSecurityReviewMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 557, + }); + + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).not.toContain("--model"); + }); + + it("outputs install_security_skills flag", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 105, + body: "@droid security-review", + }, + } as any, + entityNumber: 28, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareSecurityReviewMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 558, + }); + + const installSkillsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "install_security_skills", + ) as [string, string] | undefined; + expect(installSkillsCall?.[1]).toBe("true"); + }); + + it("prefers SECURITY_MODEL over REVIEW_MODEL", async () => { + process.env.SECURITY_MODEL = "gpt-5.1-codex"; + process.env.REVIEW_MODEL = "claude-sonnet-4-5-20250929"; + + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 106, + body: "@droid security-review", + }, + } as any, + entityNumber: 29, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareSecurityReviewMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 559, + }); + + 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"'); + expect(droidArgsCall?.[1]).not.toContain("claude-sonnet"); + }); + + it("falls back to REVIEW_MODEL when SECURITY_MODEL is not set", async () => { + process.env.REVIEW_MODEL = "claude-sonnet-4-5-20250929"; + + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 107, + body: "@droid security-review", + }, + } as any, + entityNumber: 30, + }); + + const octokit = { rest: {}, graphql: () => {} } as any; + + await prepareSecurityReviewMode({ + context, + octokit, + githubToken: "token", + trackingCommentId: 560, + }); + + const droidArgsCall = setOutputSpy.mock.calls.find( + (call: unknown[]) => call[0] === "droid_args", + ) as [string, string] | undefined; + expect(droidArgsCall?.[1]).toContain( + '--model "claude-sonnet-4-5-20250929"', + ); + }); +}); diff --git a/test/permissions.test.ts b/test/permissions.test.ts index c7c0b3c..4db439f 100644 --- a/test/permissions.test.ts +++ b/test/permissions.test.ts @@ -68,6 +68,14 @@ describe("checkWritePermissions", () => { allowedNonWriteUsers: "", trackProgress: false, automaticReview: false, + automaticSecurityReview: false, + securityModel: "", + securitySeverityThreshold: "medium", + securityBlockOnCritical: true, + securityBlockOnHigh: false, + securityNotifyTeam: "", + securityScanSchedule: false, + securityScanDays: 7, }, }); From dff43cff3a9d92efc868230588960a1739a14259 Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Tue, 13 Jan 2026 14:02:33 -0800 Subject: [PATCH 3/8] fix: address P1 issues from security commands code review - Fix SECURITY_SCAN_DAYS to avoid NaN (clamp to positive integer, default 7) - Remove instructions to commit threat model to PR branch during review - Remove instructions to commit patches to PR branch - Align security review with JSON output pattern (no direct inline comments) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- .../templates/security-review-prompt.ts | 13 +++++-------- src/github/context.ts | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/create-prompt/templates/security-review-prompt.ts b/src/create-prompt/templates/security-review-prompt.ts index 6b294a8..f71e3dc 100644 --- a/src/create-prompt/templates/security-review-prompt.ts +++ b/src/create-prompt/templates/security-review-prompt.ts @@ -49,10 +49,8 @@ You have access to these Factory security skills (installed in ~/.factory/skills ### Step 1: Threat Model Check - Check if \`.factory/threat-model.md\` exists in the repository -- If missing: Invoke the **threat-model-generation** skill to create one, then commit it to the PR branch -- If exists: Check the file's last modified date - - If >90 days old: Post a warning comment suggesting regeneration, but proceed with scan - - If current: Use it as context for the security scan +- If missing: Note this in the summary (threat model generation is done separately, not during PR review) +- If exists: Use it as context for the security scan ### Step 2: Security Scan - Invoke the **commit-security-scan** skill on the PR diff @@ -71,12 +69,11 @@ You have access to these Factory security skills (installed in ~/.factory/skills - Impact: What's the potential damage? - Filter out false positives and findings below the severity threshold (${severityThreshold}) -### Step 4: Report & Patch +### Step 4: Report Findings - For each confirmed finding at or above ${severityThreshold} severity: - - Post inline comment using \`github_inline_comment___create_inline_comment\` + - Record in the JSON output file - Include: severity, STRIDE category, CWE ID, clear explanation, suggested fix -- For auto-fixable issues: Invoke **security-review** skill to generate patches -- Commit any generated patches to the PR branch +- For auto-fixable issues: Include the suggested patch in the finding's suggestion field ## Security Scope (STRIDE Categories) diff --git a/src/github/context.ts b/src/github/context.ts index ae52994..15592f3 100644 --- a/src/github/context.ts +++ b/src/github/context.ts @@ -157,7 +157,7 @@ export function parseGitHubContext(): GitHubContext { securityBlockOnHigh: process.env.SECURITY_BLOCK_ON_HIGH === "true", securityNotifyTeam: process.env.SECURITY_NOTIFY_TEAM ?? "", securityScanSchedule: process.env.SECURITY_SCAN_SCHEDULE === "true", - securityScanDays: parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10), + securityScanDays: Math.max(1, parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10) || 7), }, }; From 6d909b9f85f6e2d08ec7720ba0c95c862a2a9c71 Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Tue, 13 Jan 2026 13:41:34 -0800 Subject: [PATCH 4/8] feat: change review output to JSON format Update review prompt to output findings to JSON file instead of posting inline comments directly. This enables the parallel workflow to combine code review and security review findings before posting. Changes: - Review writes findings to code-review-results.json - Tracking comment updated with summary table - Inline comments deferred to finalize step Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/create-prompt/templates/review-prompt.ts | 121 ++++++++++++------ .../templates/review-prompt.test.ts | 38 ++++-- test/integration/review-flow.test.ts | 5 +- 3 files changed, 109 insertions(+), 55 deletions(-) diff --git a/src/create-prompt/templates/review-prompt.ts b/src/create-prompt/templates/review-prompt.ts index c2641e9..3cf4003 100644 --- a/src/create-prompt/templates/review-prompt.ts +++ b/src/create-prompt/templates/review-prompt.ts @@ -24,34 +24,29 @@ Context: - The PR branch has already been checked out. You have full access to read any file in the codebase, not just the diff output. Objectives: -1) Re-check existing review comments and resolve threads when the issue is fixed (fall back to a brief "resolved" reply only if the thread cannot be marked resolved). -2) Review the PR diff and surface all bugs that meet the detection criteria below. -3) Leave concise inline comments (1-2 sentences) on bugs introduced by the PR. You may also comment on unchanged lines if the PR's changes expose or trigger issues there—but explain the connection clearly. +1) Review the PR diff and surface all bugs that meet the detection criteria below. +2) Output findings in JSON format for later processing (DO NOT post inline comments directly). +3) Update the tracking comment with a summary. Procedure: - Run: gh pr view ${prNumber} --repo ${repoFullName} --json comments,reviews -- Run: gh pr diff ${prNumber} --repo ${repoFullName} -- Run: gh api repos/${repoFullName}/pulls/${prNumber}/files --paginate --jq '.[] | {filename,patch,additions,deletions}' -- Prefer github_inline_comment___create_inline_comment with side="RIGHT" to post inline findings on changed/added lines -- Compute exact diff positions (path + position) for each issue; every substantive comment must be inline on the changed line (no new top-level issue comments). -- Detect prior top-level "no issues" comments authored by this bot (e.g., "no issues", "No issues found", "LGTM", including emoji-prefixed variants). -- If the current run finds issues and prior "no issues" comments exist, delete them via gh api -X DELETE repos/${repoFullName}/issues/comments/; if deletion fails, minimize via GraphQL or reply "Superseded: issues were found in newer commits". -- If a previously reported issue appears resolved by nearby changes, call github_pr___resolve_review_thread (when permitted) to mark it resolved; otherwise provide a brief reply within that thread noting the resolution. - -Preferred MCP tools (when available): -- github_inline_comment___create_inline_comment to post inline feedback anchored to the diff -- github_pr___submit_review to send inline review feedback -- github_pr___delete_comment to remove outdated "no issues" comments -- github_pr___minimize_comment when deletion is unavailable but minimization is acceptable -- github_pr___reply_to_comment to acknowledge resolved threads -- github_pr___resolve_review_thread to formally resolve threads once issues are fixed - -Diff Side Selection (CRITICAL): -- When calling github_inline_comment___create_inline_comment, ALWAYS specify the 'side' parameter -- Use side="RIGHT" for comments on NEW or MODIFIED code (what the PR adds/changes) -- Use side="LEFT" ONLY when commenting on code being REMOVED (only if you need to reference the old implementation) -- The 'line' parameter refers to the line number on the specified side of the diff -- Ensure the line numbers you use correspond to the side you choose; +- Prefer reviewing the local git diff since the PR branch is already checked out: + - Ensure you have the base branch ref locally (fetch if needed). + - Find merge base between HEAD and the base branch. + - Run git diff from that merge base to HEAD to see exactly what would merge. + - Example: + - git fetch origin ${baseRefName}:refs/remotes/origin/${baseRefName} + - MERGE_BASE=$(git merge-base HEAD refs/remotes/origin/${baseRefName}) + - git diff $MERGE_BASE..HEAD +- Use gh PR diff/file APIs only as a fallback when local git diff is not possible: + - gh pr diff ${prNumber} --repo ${repoFullName} + - gh api repos/${repoFullName}/pulls/${prNumber}/files --paginate --jq '.[] | {filename,patch,additions,deletions}' +- Analyze the diff for issues +- Write findings to \`code-review-results.json\` in the current directory +- IMPORTANT: Do NOT delete comment ID ${context.droidCommentId} - this is the tracking comment for the current run. + +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 security review comments. How Many Findings to Return: Output all findings that the original author would fix if they knew about it. If there is no finding that a person would definitely love to see and fix, prefer outputting no findings. Do not stop at the first qualifying finding. Continue until you've listed every qualifying finding. @@ -74,8 +69,6 @@ Use the following priority levels to categorize findings: - [P2] - Normal. To be fixed eventually - [P3] - Low. Nice to have -IMPORTANT: Only post P0 and P1 findings as inline comments. Do NOT post P2 or P3 findings—they are too minor to warrant review noise. If all your findings are P2/P3, post no inline comments and note "no high-severity issues found" in the summary. - Comment Guidelines: Your review comments should be: 1. Clear about why the issue is a bug @@ -88,8 +81,8 @@ Your review comments should be: 8. Avoid excessive flattery Output Format: -Structure each inline comment as (P0/P1 only): -**[P0/P1] Clear title (≤ 80 chars, imperative mood)** +Structure each inline comment as: +**[P0-P3] Clear title (≤ 80 chars, imperative mood)** (blank line) Explanation of why this is a problem (1 paragraph max). @@ -116,16 +109,68 @@ Commenting rules: - One issue per comment. - Anchor findings to the relevant diff hunk so reviewers see the context immediately. - Focus on defects introduced or exposed by the PR's changes; if a new bug manifests on an unchanged line, you may post inline comments on those unchanged lines but clearly explain how the submitted changes trigger it. -- Match the side parameter to the code segment you're referencing (default to RIGHT for new code) and provide line numbers from that same side - Keep comments concise and immediately graspable. - For low confidence findings, ask a question; for medium/high confidence, state the issue concretely. -- Only include explicit code suggestions when you are absolutely certain the replacement is correct and safe. - -Submission: -- If no issues are found and a prior "no issues" comment from this bot already exists, skip submitting another comment to avoid redundancy. -- If no issues are found and no prior "no issues" comment exists, post a single brief top-level summary noting no issues. -- If issues are found, delete/minimize/supersede any prior "no issues" comment before submitting. -- Prefer github_inline_comment___create_inline_comment for inline findings and submit the overall review via github_pr___submit_review (fall back to gh api repos/${repoFullName}/pulls/${prNumber}/reviews -f event=COMMENT -f body="$SUMMARY" -f comments='[$COMMENTS_JSON]' when MCP tools are unavailable). -- Do not approve or request changes; submit a comment-only review with inline feedback. + +Output: +1. Analyze the PR 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) + +2. Write findings to \`code-review-results.json\` with this structure: +\`\`\`json +{ + "type": "code", + "summary": "Brief 1-2 sentence description of what this PR does", + "keyChanges": [ + "Added new authentication flow", + "Refactored database queries for performance", + "Fixed validation bug in user input" + ], + "importantFiles": [ + { "path": "src/auth/login.ts", "description": "New OAuth implementation" }, + { "path": "src/db/queries.ts", "description": "Query optimization" } + ], + "findings": [ + { + "id": "CR-001", + "type": "bug|issue|suggestion", + "severity": "high|medium|low", + "file": "path/to/file.ts", + "line": 45, + "side": "RIGHT", + "description": "Brief description of the issue", + "suggestion": "Optional code fix" + } + ] +} +\`\`\` + +3. Update the tracking comment with a summary using \`github_comment___update_droid_comment\`: +\`\`\`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} + +### Review Findings +| ID | Type | Severity | File | Description | +|----|------|----------|------|-------------| +| CR-001 | Bug | high | auth.ts:45 | Null pointer exception | + +*Inline comments will be posted after all reviews complete.* +\`\`\` + +IMPORTANT: Do NOT post inline comments. Only write to the JSON file and update the tracking comment. `; } diff --git a/test/create-prompt/templates/review-prompt.test.ts b/test/create-prompt/templates/review-prompt.test.ts index 3ba4e97..4b086bd 100644 --- a/test/create-prompt/templates/review-prompt.test.ts +++ b/test/create-prompt/templates/review-prompt.test.ts @@ -28,34 +28,44 @@ describe("generateReviewPrompt", () => { const prompt = generateReviewPrompt(context); expect(prompt).toContain("Objectives:"); - expect(prompt).toContain("Re-check existing review comments"); + expect(prompt).toContain("Review the PR diff"); + expect(prompt).toContain("git merge-base"); + expect(prompt).toContain("git diff"); expect(prompt).toContain("gh pr diff 42 --repo test-owner/test-repo"); - expect(prompt).toContain("gh api repos/test-owner/test-repo/pulls/42/files"); - expect(prompt).toContain("github_inline_comment___create_inline_comment"); - expect(prompt).toContain("github_pr___resolve_review_thread"); - expect(prompt).toContain("every substantive comment must be inline on the changed line"); + expect(prompt).toContain( + "gh api repos/test-owner/test-repo/pulls/42/files", + ); + expect(prompt).toContain("code-review-results.json"); + expect(prompt).toContain("Do NOT post inline comments"); }); it("emphasizes accuracy gates and bug detection guidelines", () => { const prompt = generateReviewPrompt(createBaseContext()); expect(prompt).toContain("How Many Findings to Return:"); - expect(prompt).toContain("Output all findings that the original author would fix"); + expect(prompt).toContain( + "Output all findings that the original author would fix", + ); expect(prompt).toContain("Key Guidelines for Bug Detection:"); expect(prompt).toContain("Priority Levels:"); expect(prompt).toContain("[P0]"); expect(prompt).toContain("Never raise purely stylistic"); - expect(prompt).toContain("Never repeat or re-raise an issue previously highlighted"); + expect(prompt).toContain( + "Never repeat or re-raise an issue previously highlighted", + ); }); - it("describes submission guidance", () => { + it("describes output format with Greptile-style summary", () => { const prompt = generateReviewPrompt(createBaseContext()); - expect(prompt).toContain("Prefer github_inline_comment___create_inline_comment"); - expect(prompt).toContain("gh api repos/test-owner/test-repo/pulls/42/reviews"); - expect(prompt).toContain("Do not approve or request changes"); - expect(prompt).toContain("github_pr___submit_review"); - expect(prompt).toContain("github_pr___resolve_review_thread"); - expect(prompt).toContain("skip submitting another comment to avoid redundancy"); + expect(prompt).toContain("code-review-results.json"); + expect(prompt).toContain("github_comment___update_droid_comment"); + expect(prompt).toContain( + "Inline comments will be posted after all reviews complete", + ); + expect(prompt).toContain("### Summary"); + expect(prompt).toContain("### Key Changes"); + expect(prompt).toContain("### Important Files Changed"); + expect(prompt).toContain("### Review Findings"); }); }); diff --git a/test/integration/review-flow.test.ts b/test/integration/review-flow.test.ts index 61bbf6e..2bd8c57 100644 --- a/test/integration/review-flow.test.ts +++ b/test/integration/review-flow.test.ts @@ -145,14 +145,13 @@ describe("review command integration", () => { const prompt = await readFile(promptPath, "utf8"); expect(prompt).toContain("You are performing an automated code review"); - expect(prompt).toContain("github_inline_comment___create_inline_comment"); expect(prompt).toContain("How Many Findings to Return:"); expect(prompt).toContain("Output all findings that the original author would fix"); expect(prompt).toContain("Key Guidelines for Bug Detection:"); expect(prompt).toContain("Priority Levels:"); expect(prompt).toContain("gh pr view 7 --repo test-owner/test-repo --json comments,reviews"); - expect(prompt).toContain("every substantive comment must be inline on the changed line"); - expect(prompt).toContain("github_pr___resolve_review_thread"); + expect(prompt).toContain("code-review-results.json"); + expect(prompt).toContain("Do NOT post inline comments"); const droidArgsCall = setOutputSpy.mock.calls.find( (call: unknown[]) => call[0] === "droid_args", From b1b0dfbbb38289b404e87be259c73805e90c4113 Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Thu, 15 Jan 2026 13:19:13 -0800 Subject: [PATCH 5/8] updated review prompt --- base-action/src/index.ts | 3 +- src/create-prompt/index.ts | 19 +++--- src/create-prompt/templates/review-prompt.ts | 51 ++++++++++++---- src/github/context.ts | 5 +- .../templates/review-prompt.test.ts | 36 +++++++++++- test/integration/review-flow.test.ts | 58 +++++++++---------- test/modes/tag/review-command.test.ts | 4 +- 7 files changed, 117 insertions(+), 59 deletions(-) diff --git a/base-action/src/index.ts b/base-action/src/index.ts index d854ab3..3661021 100644 --- a/base-action/src/index.ts +++ b/base-action/src/index.ts @@ -28,8 +28,7 @@ async function run() { mcpTools: process.env.INPUT_MCP_TOOLS, systemPrompt: process.env.INPUT_SYSTEM_PROMPT, appendSystemPrompt: process.env.INPUT_APPEND_SYSTEM_PROMPT, - pathToDroidExecutable: - process.env.INPUT_PATH_TO_DROID_EXECUTABLE, + pathToDroidExecutable: process.env.INPUT_PATH_TO_DROID_EXECUTABLE, showFullOutput: process.env.INPUT_SHOW_FULL_OUTPUT, }); } catch (error) { diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index b2ea5af..068b276 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -108,15 +108,12 @@ export function prepareContext( commonFields.droidBranch = droidBranch; } - const eventData = buildEventData( - context, - { - commentId, - commentBody, - baseBranch, - droidBranch, - }, - ); + const eventData = buildEventData(context, { + commentId, + commentBody, + baseBranch, + droidBranch, + }); const result: PreparedContext = { ...commonFields, @@ -282,9 +279,7 @@ function buildEventData( } } -export type PromptGenerator = ( - context: PreparedContext, -) => string; +export type PromptGenerator = (context: PreparedContext) => string; export type PromptCreationOptions = { githubContext: ParsedGitHubContext; diff --git a/src/create-prompt/templates/review-prompt.ts b/src/create-prompt/templates/review-prompt.ts index 3cf4003..ff98aac 100644 --- a/src/create-prompt/templates/review-prompt.ts +++ b/src/create-prompt/templates/review-prompt.ts @@ -24,9 +24,9 @@ Context: - The PR branch has already been checked out. You have full access to read any file in the codebase, not just the diff output. Objectives: -1) Review the PR diff and surface all bugs that meet the detection criteria below. -2) Output findings in JSON format for later processing (DO NOT post inline comments directly). -3) Update the tracking comment with a summary. +1) Re-check existing review comments and resolve threads when the issue is fixed (fall back to a brief "resolved" reply only if the thread cannot be marked resolved). +2) Review the PR diff and surface all bugs that meet the detection criteria below. +3) Leave concise inline comments (1-2 sentences) on bugs introduced by the PR. You may also comment on unchanged lines if the PR's changes expose or trigger issues there—but explain the connection clearly. Procedure: - Run: gh pr view ${prNumber} --repo ${repoFullName} --json comments,reviews @@ -41,12 +41,29 @@ Procedure: - Use gh PR diff/file APIs only as a fallback when local git diff is not possible: - gh pr diff ${prNumber} --repo ${repoFullName} - gh api repos/${repoFullName}/pulls/${prNumber}/files --paginate --jq '.[] | {filename,patch,additions,deletions}' -- Analyze the diff for issues -- Write findings to \`code-review-results.json\` in the current directory +- Prefer github_inline_comment___create_inline_comment with side="RIGHT" to post inline findings on changed/added lines +- Compute exact diff positions (path + position) for each issue; every substantive comment must be inline on the changed line (no new top-level issue comments). +- Detect prior top-level "no issues" comments authored by this bot (e.g., "no issues", "No issues found", "LGTM", including emoji-prefixed variants). +- If the current run finds issues and prior "no issues" comments exist, delete them via gh api -X DELETE repos/${repoFullName}/issues/comments/; if deletion fails, minimize via GraphQL or reply "Superseded: issues were found in newer commits". - IMPORTANT: Do NOT delete comment ID ${context.droidCommentId} - this is the tracking comment for the current run. - -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 security review comments. +- Thread resolution rule (CRITICAL): NEVER resolve review threads. + - Do NOT call github_pr___resolve_review_thread under any circumstances. + - If a previously reported issue appears fixed, leave the thread unresolved. + +Preferred MCP tools (when available): +- github_inline_comment___create_inline_comment to post inline feedback anchored to the diff +- github_pr___submit_review to send inline review feedback +- github_pr___delete_comment to remove outdated "no issues" comments +- github_pr___minimize_comment when deletion is unavailable but minimization is acceptable +- github_pr___reply_to_comment to acknowledge resolved threads +- github_pr___resolve_review_thread to formally resolve threads once issues are fixed + +Diff Side Selection (CRITICAL): +- When calling github_inline_comment___create_inline_comment, ALWAYS specify the 'side' parameter +- Use side="RIGHT" for comments on NEW or MODIFIED code (what the PR adds/changes) +- Use side="LEFT" ONLY when commenting on code being REMOVED (only if you need to reference the old implementation) +- The 'line' parameter refers to the line number on the specified side of the diff +- Ensure the line numbers you use correspond to the side you choose; How Many Findings to Return: Output all findings that the original author would fix if they knew about it. If there is no finding that a person would definitely love to see and fix, prefer outputting no findings. Do not stop at the first qualifying finding. Continue until you've listed every qualifying finding. @@ -69,6 +86,8 @@ Use the following priority levels to categorize findings: - [P2] - Normal. To be fixed eventually - [P3] - Low. Nice to have +IMPORTANT: Only post P0 and P1 findings as inline comments. Do NOT post P2 or P3 findings—they are too minor to warrant review noise. If all your findings are P2/P3, post no inline comments and note "no high-severity issues found" in the summary. + Comment Guidelines: Your review comments should be: 1. Clear about why the issue is a bug @@ -82,7 +101,7 @@ Your review comments should be: Output Format: Structure each inline comment as: -**[P0-P3] Clear title (≤ 80 chars, imperative mood)** +**[P0/P1] Clear title (≤ 80 chars, imperative mood)** (blank line) Explanation of why this is a problem (1 paragraph max). @@ -109,8 +128,10 @@ Commenting rules: - One issue per comment. - Anchor findings to the relevant diff hunk so reviewers see the context immediately. - Focus on defects introduced or exposed by the PR's changes; if a new bug manifests on an unchanged line, you may post inline comments on those unchanged lines but clearly explain how the submitted changes trigger it. +- Match the side parameter to the code segment you're referencing (default to RIGHT for new code) and provide line numbers from that same side - Keep comments concise and immediately graspable. - For low confidence findings, ask a question; for medium/high confidence, state the issue concretely. +- Only include explicit code suggestions when you are absolutely certain the replacement is correct and safe. Output: 1. Analyze the PR to generate: @@ -171,6 +192,16 @@ Output: *Inline comments will be posted after all reviews complete.* \`\`\` -IMPORTANT: Do NOT post inline comments. Only write to the JSON file and update the tracking comment. +Submission: +- Do not submit inline comments when: + - the PR appears formatting-only, or + - all findings are low-severity (P2/P3), or + - you cannot anchor a high-confidence issue to a specific changed line. +- Do not escalate style/formatting into P0/P1 just to justify leaving an inline comment. +- If no issues are found and a prior "no issues" comment from this bot already exists, skip submitting another comment to avoid redundancy. +- If no issues are found and no prior "no issues" comment exists, post a single brief top-level summary noting no issues. +- If issues are found, delete/minimize/supersede any prior "no issues" comment before submitting. +- Prefer github_inline_comment___create_inline_comment for inline findings and submit the overall review via github_pr___submit_review (fall back to gh api repos/${repoFullName}/pulls/${prNumber}/reviews -f event=COMMENT -f body="$SUMMARY" -f comments='[$COMMENTS_JSON]' when MCP tools are unavailable). +- Do not approve or request changes; submit a comment-only review with inline feedback. `; } diff --git a/src/github/context.ts b/src/github/context.ts index 15592f3..ee0e888 100644 --- a/src/github/context.ts +++ b/src/github/context.ts @@ -157,7 +157,10 @@ export function parseGitHubContext(): GitHubContext { securityBlockOnHigh: process.env.SECURITY_BLOCK_ON_HIGH === "true", securityNotifyTeam: process.env.SECURITY_NOTIFY_TEAM ?? "", securityScanSchedule: process.env.SECURITY_SCAN_SCHEDULE === "true", - securityScanDays: Math.max(1, parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10) || 7), + securityScanDays: Math.max( + 1, + parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10) || 7, + ), }, }; diff --git a/test/create-prompt/templates/review-prompt.test.ts b/test/create-prompt/templates/review-prompt.test.ts index 4b086bd..3c26b96 100644 --- a/test/create-prompt/templates/review-prompt.test.ts +++ b/test/create-prompt/templates/review-prompt.test.ts @@ -28,6 +28,7 @@ describe("generateReviewPrompt", () => { const prompt = generateReviewPrompt(context); expect(prompt).toContain("Objectives:"); + expect(prompt).toContain("Re-check existing review comments"); expect(prompt).toContain("Review the PR diff"); expect(prompt).toContain("git merge-base"); expect(prompt).toContain("git diff"); @@ -35,8 +36,10 @@ describe("generateReviewPrompt", () => { expect(prompt).toContain( "gh api repos/test-owner/test-repo/pulls/42/files", ); - expect(prompt).toContain("code-review-results.json"); - expect(prompt).toContain("Do NOT post inline comments"); + expect(prompt).toContain("github_inline_comment___create_inline_comment"); + expect(prompt).toContain("every substantive comment must be inline"); + expect(prompt).toContain("Thread resolution rule (CRITICAL)"); + expect(prompt).toContain("NEVER resolve review threads"); }); it("emphasizes accuracy gates and bug detection guidelines", () => { @@ -49,13 +52,27 @@ describe("generateReviewPrompt", () => { expect(prompt).toContain("Key Guidelines for Bug Detection:"); expect(prompt).toContain("Priority Levels:"); expect(prompt).toContain("[P0]"); + expect(prompt).toContain("Only post P0 and P1 findings as inline comments"); expect(prompt).toContain("Never raise purely stylistic"); expect(prompt).toContain( "Never repeat or re-raise an issue previously highlighted", ); }); - it("describes output format with Greptile-style summary", () => { + it("describes MCP tools and diff side selection", () => { + const prompt = generateReviewPrompt(createBaseContext()); + + expect(prompt).toContain("Preferred MCP tools"); + expect(prompt).toContain("github_inline_comment___create_inline_comment"); + expect(prompt).toContain("github_pr___submit_review"); + expect(prompt).toContain("github_pr___delete_comment"); + expect(prompt).toContain("github_pr___resolve_review_thread"); + expect(prompt).toContain("Diff Side Selection (CRITICAL)"); + expect(prompt).toContain('side="RIGHT"'); + expect(prompt).toContain('side="LEFT"'); + }); + + it("describes JSON output format with summary", () => { const prompt = generateReviewPrompt(createBaseContext()); expect(prompt).toContain("code-review-results.json"); @@ -68,4 +85,17 @@ describe("generateReviewPrompt", () => { expect(prompt).toContain("### Important Files Changed"); expect(prompt).toContain("### Review Findings"); }); + + it("describes submission guidance", () => { + const prompt = generateReviewPrompt(createBaseContext()); + + expect(prompt).toContain("Submission:"); + expect(prompt).toContain("Do not submit inline comments when"); + expect(prompt).toContain("all findings are low-severity (P2/P3)"); + expect(prompt).toContain( + "gh api repos/test-owner/test-repo/pulls/42/reviews", + ); + expect(prompt).toContain("Do not approve or request changes"); + expect(prompt).toContain("submit a comment-only review"); + }); }); diff --git a/test/integration/review-flow.test.ts b/test/integration/review-flow.test.ts index 2bd8c57..9eb1795 100644 --- a/test/integration/review-flow.test.ts +++ b/test/integration/review-flow.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 path from "node:path"; import os from "node:os"; import { mkdtemp, readFile, rm } from "node:fs/promises"; @@ -32,8 +25,6 @@ describe("review command integration", () => { process.env.RUNNER_TEMP = tmpDir; process.env.DROID_ARGS = ""; - - createCommentSpy = spyOn( createInitial, "createInitialComment", @@ -95,27 +86,28 @@ describe("review command integration", () => { } as any, }); - const octokit = { - rest: {}, - graphql: () => Promise.resolve({ - repository: { - pullRequest: { - baseRefName: "main", - headRefName: "feature/review", - headRefOid: "def456", - } - } - }) + const octokit = { + rest: {}, + graphql: () => + Promise.resolve({ + repository: { + pullRequest: { + baseRefName: "main", + headRefName: "feature/review", + headRefOid: "def456", + }, + }, + }), } as any; graphqlSpy = spyOn(octokit, "graphql").mockResolvedValue({ repository: { pullRequest: { baseRefName: "main", - headRefName: "feature/review", + headRefName: "feature/review", headRefOid: "def456", - } - } + }, + }, }); const result = await prepareTagExecution({ @@ -145,21 +137,27 @@ describe("review command integration", () => { const prompt = await readFile(promptPath, "utf8"); expect(prompt).toContain("You are performing an automated code review"); + expect(prompt).toContain("github_inline_comment___create_inline_comment"); expect(prompt).toContain("How Many Findings to Return:"); - expect(prompt).toContain("Output all findings that the original author would fix"); + expect(prompt).toContain( + "Output all findings that the original author would fix", + ); expect(prompt).toContain("Key Guidelines for Bug Detection:"); expect(prompt).toContain("Priority Levels:"); - expect(prompt).toContain("gh pr view 7 --repo test-owner/test-repo --json comments,reviews"); + expect(prompt).toContain( + "gh pr view 7 --repo test-owner/test-repo --json comments,reviews", + ); + expect(prompt).toContain( + "every substantive comment must be inline on the changed line", + ); expect(prompt).toContain("code-review-results.json"); - expect(prompt).toContain("Do NOT post inline comments"); + expect(prompt).toContain("github_pr___submit_review"); const droidArgsCall = setOutputSpy.mock.calls.find( (call: unknown[]) => call[0] === "droid_args", ) as [string, string] | undefined; - expect(droidArgsCall?.[1]).toContain( - "github_pr___list_review_comments", - ); + expect(droidArgsCall?.[1]).toContain("github_pr___list_review_comments"); expect(droidArgsCall?.[1]).toContain("github_pr___submit_review"); expect(droidArgsCall?.[1]).toContain( "github_inline_comment___create_inline_comment", diff --git a/test/modes/tag/review-command.test.ts b/test/modes/tag/review-command.test.ts index a135ae6..3c20e0b 100644 --- a/test/modes/tag/review-command.test.ts +++ b/test/modes/tag/review-command.test.ts @@ -260,7 +260,9 @@ describe("prepareReviewMode", () => { const droidArgsCall = setOutputSpy.mock.calls.find( (call: unknown[]) => call[0] === "droid_args", ) as [string, string] | undefined; - expect(droidArgsCall?.[1]).toContain('--model "claude-sonnet-4-5-20250929"'); + expect(droidArgsCall?.[1]).toContain( + '--model "claude-sonnet-4-5-20250929"', + ); }); it("does not add --model flag when REVIEW_MODEL is empty", async () => { From 699cf46d4f38cf93328bc1a6ed033486500a2aeb Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Thu, 15 Jan 2026 14:04:04 -0800 Subject: [PATCH 6/8] removed readme changes --- README.md | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index be17208..b3cfb4d 100644 --- a/README.md +++ b/README.md @@ -2,8 +2,8 @@ This GitHub Action powers the Factory **Droid** app. It watches your pull requests for the two supported commands and runs a full Droid Exec session to help you ship faster: -- `@droid fill` — turns a bare pull request into a polished description that matches your template or our opinionated fallback. -- `@droid review` — performs an automated code review, surfaces potential bugs, and leaves inline comments directly on the diff. +* `@droid fill` — turns a bare pull request into a polished description that matches your template or our opinionated fallback. +* `@droid review` — performs an automated code review, surfaces potential bugs, and leaves inline comments directly on the diff. Everything runs inside GitHub Actions using your Factory API key, so the bot never leaves your repository and operates with the permissions you grant. @@ -18,11 +18,11 @@ Everything runs inside GitHub Actions using your Factory API key, so the bot nev ## Installation 1. **Install the Droid GitHub App** - - Install from the Factory dashboard and grant it access to the repositories where you want Droid to operate. + * Install from the Factory dashboard and grant it access to the repositories where you want Droid to operate. 2. **Create a Factory API Key** - - Generate a token at [https://app.factory.ai/settings/api-keys](https://app.factory.ai/settings/api-keys) and save it as `FACTORY_API_KEY` in your repository or organization secrets. + * Generate a token at [https://app.factory.ai/settings/api-keys](https://app.factory.ai/settings/api-keys) and save it as `FACTORY_API_KEY` in your repository or organization secrets. 3. **Add the Action Workflows** - - Create two workflow files under `.github/workflows/` to separate on-demand tagging from automatic PR reviews. + * Create two workflow files under `.github/workflows/` to separate on-demand tagging from automatic PR reviews. `droid.yml` (responds to explicit `@droid` mentions): @@ -105,28 +105,26 @@ Once committed, tagging `@droid fill` or `@droid review` on an open PR will trig ## Using the Commands ### `@droid fill` - -- Place the command in the PR description or in a top-level comment. -- Droid searches for common PR template locations (`.github/pull_request_template.md`, etc.). When a template exists, it fills the sections; otherwise it writes a structured summary (overview, changes, testing, rollout). -- The original request is replaced with the generated description so reviewers can merge immediately. +* Place the command in the PR description or in a top-level comment. +* Droid searches for common PR template locations (`.github/pull_request_template.md`, etc.). When a template exists, it fills the sections; otherwise it writes a structured summary (overview, changes, testing, rollout). +* The original request is replaced with the generated description so reviewers can merge immediately. ### `@droid review` - -- Mention `@droid review` in a PR comment. -- Droid inspects the diff, prioritizes potential bugs or high-impact issues, and leaves inline comments directly on the changed lines. -- A short summary comment is posted in the original thread highlighting the findings and linking to any inline feedback. +* Mention `@droid review` in a PR comment. +* Droid inspects the diff, prioritizes potential bugs or high-impact issues, and leaves inline comments directly on the changed lines. +* A short summary comment is posted in the original thread highlighting the findings and linking to any inline feedback. ## Configuration Essentials -| Input | Purpose | -| ----------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | -| `factory_api_key` | **Required.** Grants Droid Exec permission to run via Factory. | -| `github_token` | Optional override if you prefer a custom GitHub App/token. By default the installed app token is used. | -| `review_model` | Optional. Override the model used for code review (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to review flows. | -| `fill_model` | Optional. Override the model used for PR description fill (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to fill flows. | +| Input | Purpose | +| --- | --- | +| `factory_api_key` | **Required.** Grants Droid Exec permission to run via Factory. | +| `github_token` | Optional override if you prefer a custom GitHub App/token. By default the installed app token is used. | +| `review_model` | Optional. Override the model used for code review (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to review flows. | +| `fill_model` | Optional. Override the model used for PR description fill (e.g., `claude-sonnet-4-5-20250929`, `gpt-5.1-codex`). Only applies to fill flows. | ## Troubleshooting & Support -- Check the workflow run linked from the Droid tracking comment for execution logs. -- Verify that the workflow file and repository allow the GitHub App to run (branch protections can block bots). -- Need more detail? Start with the [Setup Guide](./docs/setup.md) or [FAQ](./docs/faq.md). +* Check the workflow run linked from the Droid tracking comment for execution logs. +* Verify that the workflow file and repository allow the GitHub App to run (branch protections can block bots). +* Need more detail? Start with the [Setup Guide](./docs/setup.md) or [FAQ](./docs/faq.md). From 281fa33fd19d1f79560239da24fb71d3af048af4 Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Thu, 15 Jan 2026 14:10:53 -0800 Subject: [PATCH 7/8] remove unnecessary field --- action.yml | 10 ---------- src/entrypoints/collect-inputs.ts | 2 -- src/github/context.ts | 7 ------- src/github/utils/command-parser.test.ts | 2 -- test/create-prompt.test.ts | 2 -- .../templates/security-report-prompt.test.ts | 2 -- test/mockContext.ts | 2 -- test/permissions.test.ts | 2 -- 8 files changed, 29 deletions(-) diff --git a/action.yml b/action.yml index f259fef..546d17b 100644 --- a/action.yml +++ b/action.yml @@ -83,14 +83,6 @@ inputs: description: "GitHub team to @mention on critical findings (e.g., '@org/security-team')." required: false default: "" - security_scan_schedule: - description: "Enable scheduled security scans. Set to 'true' for schedule events to trigger full repository scans." - required: false - default: "false" - security_scan_days: - description: "Number of days of commits to scan for scheduled security scans. Only applies when security_scan_schedule is enabled." - required: false - default: "7" review_model: description: "Override the model used for code review (e.g., 'claude-sonnet-4-5-20250929', 'gpt-5.1-codex'). Only applies to review flows." required: false @@ -169,8 +161,6 @@ runs: SECURITY_BLOCK_ON_CRITICAL: ${{ inputs.security_block_on_critical }} SECURITY_BLOCK_ON_HIGH: ${{ inputs.security_block_on_high }} SECURITY_NOTIFY_TEAM: ${{ inputs.security_notify_team }} - SECURITY_SCAN_SCHEDULE: ${{ inputs.security_scan_schedule }} - SECURITY_SCAN_DAYS: ${{ inputs.security_scan_days }} REVIEW_MODEL: ${{ inputs.review_model }} FILL_MODEL: ${{ inputs.fill_model }} ADDITIONAL_PERMISSIONS: ${{ inputs.additional_permissions }} diff --git a/src/entrypoints/collect-inputs.ts b/src/entrypoints/collect-inputs.ts index 442da29..1fb62ac 100644 --- a/src/entrypoints/collect-inputs.ts +++ b/src/entrypoints/collect-inputs.ts @@ -32,8 +32,6 @@ export function collectActionInputsPresence(): void { security_block_on_critical: "true", security_block_on_high: "false", security_notify_team: "", - security_scan_schedule: "false", - security_scan_days: "7", }; const allInputsJson = process.env.ALL_INPUTS; diff --git a/src/github/context.ts b/src/github/context.ts index ee0e888..0d8ccf7 100644 --- a/src/github/context.ts +++ b/src/github/context.ts @@ -96,8 +96,6 @@ type BaseContext = { securityBlockOnCritical: boolean; securityBlockOnHigh: boolean; securityNotifyTeam: string; - securityScanSchedule: boolean; - securityScanDays: number; }; }; @@ -156,11 +154,6 @@ export function parseGitHubContext(): GitHubContext { process.env.SECURITY_BLOCK_ON_CRITICAL !== "false", securityBlockOnHigh: process.env.SECURITY_BLOCK_ON_HIGH === "true", securityNotifyTeam: process.env.SECURITY_NOTIFY_TEAM ?? "", - securityScanSchedule: process.env.SECURITY_SCAN_SCHEDULE === "true", - securityScanDays: Math.max( - 1, - parseInt(process.env.SECURITY_SCAN_DAYS ?? "7", 10) || 7, - ), }, }; diff --git a/src/github/utils/command-parser.test.ts b/src/github/utils/command-parser.test.ts index 602a13b..20fae4a 100644 --- a/src/github/utils/command-parser.test.ts +++ b/src/github/utils/command-parser.test.ts @@ -33,8 +33,6 @@ const baseContext: Omit = { securityBlockOnCritical: true, securityBlockOnHigh: false, securityNotifyTeam: "", - securityScanSchedule: false, - securityScanDays: 7, }, entityNumber: 1, isPR: true, diff --git a/test/create-prompt.test.ts b/test/create-prompt.test.ts index f4176ff..131829b 100644 --- a/test/create-prompt.test.ts +++ b/test/create-prompt.test.ts @@ -32,8 +32,6 @@ describe("prepareContext", () => { securityBlockOnCritical: true, securityBlockOnHigh: false, securityNotifyTeam: "", - securityScanSchedule: false, - securityScanDays: 7, }, } as const; diff --git a/test/create-prompt/templates/security-report-prompt.test.ts b/test/create-prompt/templates/security-report-prompt.test.ts index f2698d3..bfe2eec 100644 --- a/test/create-prompt/templates/security-report-prompt.test.ts +++ b/test/create-prompt/templates/security-report-prompt.test.ts @@ -43,8 +43,6 @@ describe("generateSecurityReportPrompt", () => { securityBlockOnCritical: true, securityBlockOnHigh: false, securityNotifyTeam: "@org/security-team", - securityScanSchedule: false, - securityScanDays: 7, }, }, }; diff --git a/test/mockContext.ts b/test/mockContext.ts index 4c197a7..fae7ec8 100644 --- a/test/mockContext.ts +++ b/test/mockContext.ts @@ -25,8 +25,6 @@ const defaultInputs = { securityBlockOnCritical: true, securityBlockOnHigh: false, securityNotifyTeam: "", - securityScanSchedule: false, - securityScanDays: 7, }; const defaultRepository = { diff --git a/test/permissions.test.ts b/test/permissions.test.ts index 4db439f..3a760f6 100644 --- a/test/permissions.test.ts +++ b/test/permissions.test.ts @@ -74,8 +74,6 @@ describe("checkWritePermissions", () => { securityBlockOnCritical: true, securityBlockOnHigh: false, securityNotifyTeam: "", - securityScanSchedule: false, - securityScanDays: 7, }, }); From bbca983a515bf7b815171864c0f2c87bc2c9bb49 Mon Sep 17 00:00:00 2001 From: Shashank Sharma Date: Thu, 15 Jan 2026 14:20:20 -0800 Subject: [PATCH 8/8] remove review-security --- src/github/utils/command-parser.test.ts | 22 ++++++---------------- src/github/utils/command-parser.ts | 14 -------------- 2 files changed, 6 insertions(+), 30 deletions(-) diff --git a/src/github/utils/command-parser.test.ts b/src/github/utils/command-parser.test.ts index 20fae4a..85ac1ec 100644 --- a/src/github/utils/command-parser.test.ts +++ b/src/github/utils/command-parser.test.ts @@ -77,26 +77,16 @@ describe("Command Parser", () => { expect(result?.raw).toBe("@droid review"); }); - it("should detect @droid review security (combined)", () => { + it("should parse @droid review security as review", () => { const result = parseDroidCommand("@droid review security"); - expect(result?.command).toBe("review-security"); - expect(result?.raw).toBe("@droid review security"); + expect(result?.command).toBe("review"); + expect(result?.raw).toBe("@droid review"); }); - it("should detect @droid security review (combined)", () => { + it("should parse @droid security review as security", () => { const result = parseDroidCommand("@droid security review"); - expect(result?.command).toBe("review-security"); - expect(result?.raw).toBe("@droid security review"); - }); - - it("should be case insensitive for combined commands", () => { - const result = parseDroidCommand("@DROID REVIEW SECURITY"); - expect(result?.command).toBe("review-security"); - }); - - it("should prioritize combined over individual review", () => { - const result = parseDroidCommand("@droid review security please"); - expect(result?.command).toBe("review-security"); + expect(result?.command).toBe("security"); + expect(result?.raw).toBe("@droid security"); }); it("should detect @droid security", () => { diff --git a/src/github/utils/command-parser.ts b/src/github/utils/command-parser.ts index 182cac5..dcf15fd 100644 --- a/src/github/utils/command-parser.ts +++ b/src/github/utils/command-parser.ts @@ -8,7 +8,6 @@ export type DroidCommand = | "fill" | "review" | "security" - | "review-security" | "security-full" | "default"; @@ -39,19 +38,6 @@ export function parseDroidCommand(text: string): ParsedCommand | null { }; } - // Check for @droid review security OR @droid security review (both reviews) - // Must check before individual review/security to avoid false matches - const combinedMatch = text.match( - /@droid\s+(?:review\s+security|security\s+review)/i, - ); - if (combinedMatch) { - return { - command: "review-security", - raw: combinedMatch[0], - location: "body", // Will be set by caller - }; - } - // Check for @droid review command (case insensitive) const reviewMatch = text.match(/@droid\s+review/i); if (reviewMatch) {