From efd7e43c64faa9b7e0d7e17a547ec5f169c3a59d Mon Sep 17 00:00:00 2001 From: Nizar Alrifai Date: Mon, 24 Aug 2026 16:36:45 -0700 Subject: [PATCH 01/13] fix(github): survive GraphQL responses that carry no envelope `@octokit/graphql` resolves `response.data.data` and only throws when the body carries an `errors` array, so an HTTP 200 whose body is not a GraphQL envelope resolves `undefined`. `fetchPRBranchData` dereferenced that value directly, turning a transient GitHub answer into `TypeError: undefined is not an object (evaluating 'prResult.repository')` that killed the `Prepare validator` step of Droid Auto Review and skipped the review pass that posts the comments. Route both fetchers in this module through one helper that checks the response shape before reading it, retries the request via `utils/retry.ts` per the repo convention for GitHub API calls, and reports the HTTP status, content type and body preview captured through a request hook, since `@octokit/graphql` otherwise discards the body that would explain the failure. A present-but-null `repository` stays a stable "not found" answer and is not retried. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/github/data/pr-fetcher.ts | 175 ++++++++++++++--- test/github/data/pr-fetcher.test.ts | 293 ++++++++++++++++++++++++++++ 2 files changed, 444 insertions(+), 24 deletions(-) create mode 100644 test/github/data/pr-fetcher.test.ts diff --git a/src/github/data/pr-fetcher.ts b/src/github/data/pr-fetcher.ts index 44c9364c..facbfaa7 100644 --- a/src/github/data/pr-fetcher.ts +++ b/src/github/data/pr-fetcher.ts @@ -1,6 +1,7 @@ import type { Octokits } from "../api/client"; import { PR_QUERY, REPO_DEFAULT_BRANCH_QUERY } from "../api/queries/github"; import type { GitHubPullRequest } from "../types"; +import { retryWithBackoff } from "../../utils/retry"; /** * Represents the PR data needed by fill and review commands @@ -16,9 +17,125 @@ export type PRBranchData = { type PullRequestQueryResponse = { repository: { pullRequest: GitHubPullRequest | null; - }; + } | null; }; +type RepoDefaultBranchQueryResponse = { + repository: { + defaultBranchRef: { + name: string; + } | null; + } | null; +}; + +const GRAPHQL_RETRY_OPTIONS = { + maxAttempts: 3, + initialDelayMs: 1000, + maxDelayMs: 4000, +}; + +type OctokitHttpResponse = { + status: number; + headers: Record; + data: unknown; +}; + +/** + * Structural type for an `@octokit/request` hook. Octokit types + * `request.hook` as `any`, so it is spelled out here to keep the capture below + * type-checked. + */ +type OctokitRequestHook = ( + request: (options: unknown) => Promise, + options: unknown, +) => Promise; + +/** + * Summarizes a value for an error message without dumping a whole response. + */ +function describePayload(payload: unknown): string { + if (payload === undefined) { + return "undefined"; + } + if (payload === null) { + return "null"; + } + if (typeof payload === "string") { + const preview = payload.slice(0, 200); + return `string(${JSON.stringify(preview)}${payload.length > 200 ? "…" : ""})`; + } + if (typeof payload !== "object") { + return `${typeof payload}(${String(payload)})`; + } + return `object with keys [${Object.keys(payload).join(", ")}]`; +} + +/** + * Runs a GraphQL query and returns one top-level field of the response. + * + * `@octokit/graphql` resolves `response.data.data` and only throws when the + * body carries an `errors` array, so an HTTP 200 whose body is not a GraphQL + * envelope (an HTML error page, or JSON without a `data` key) resolves + * `undefined`. Dereferencing that directly turned a transient GitHub answer + * into a `TypeError` that killed the whole action step, so the shape is checked + * here and the request is retried: the same query has been observed to succeed + * minutes earlier in the same job. + * + * A present-but-null field is returned as-is; GraphQL nulls a field the token + * may not read, which is a stable "not found" answer rather than a broken + * response, and the caller reports it without burning retries. + */ +async function fetchGraphQLField({ + octokits, + query, + variables, + field, + queryDescription, +}: { + octokits: Octokits; + query: string; + variables: Record; + field: string; + queryDescription: string; +}): Promise { + return retryWithBackoff(async () => { + // The HTTP response is captured through a request hook because + // `@octokit/graphql` hands back only the `data` member, which leaves an + // unexpected body impossible to diagnose from the action log. + let httpStatus: number | undefined; + let httpContentType: string | undefined; + let httpBody: unknown; + + const captureResponse: OctokitRequestHook = async (request, options) => { + const response = await request(options); + httpStatus = response.status; + httpContentType = String(response.headers["content-type"] ?? ""); + httpBody = response.data; + return response; + }; + + const payload = await octokits.graphql>(query, { + ...variables, + request: { hook: captureResponse }, + }); + + if ( + payload === null || + typeof payload !== "object" || + !(field in payload) + ) { + throw new Error( + `${queryDescription} returned no "${field}" field ` + + `(GraphQL data: ${describePayload(payload)}; ` + + `HTTP ${httpStatus ?? "unknown"} ${httpContentType || "unknown content-type"}, ` + + `body: ${describePayload(httpBody)})`, + ); + } + + return payload[field] as T; + }, GRAPHQL_RETRY_OPTIONS); +} + /** * Fetches PR branch information needed for fill/review commands. * This is a focused function that only retrieves the branch names and SHA @@ -35,21 +152,26 @@ export async function fetchPRBranchData({ prNumber: number; }): Promise { try { - const prResult = await octokits.graphql( - PR_QUERY, - { + const repositoryResult = await fetchGraphQLField< + PullRequestQueryResponse["repository"] + >({ + octokits, + query: PR_QUERY, + variables: { owner: repository.owner, repo: repository.repo, number: prNumber, }, - ); + field: "repository", + queryDescription: `PR query for ${repository.owner}/${repository.repo}#${prNumber}`, + }); + + const pullRequest = repositoryResult?.pullRequest; - if (!prResult.repository.pullRequest) { + if (!pullRequest) { throw new Error(`PR #${prNumber} not found`); } - const pullRequest = prResult.repository.pullRequest; - return { baseRefName: pullRequest.baseRefName, headRefName: pullRequest.headRefName, @@ -59,18 +181,14 @@ export async function fetchPRBranchData({ }; } catch (error) { console.error(`Failed to fetch PR branch data:`, error); - throw new Error(`Failed to fetch PR branch data for PR #${prNumber}`); + throw new Error( + `Failed to fetch PR branch data for PR #${prNumber}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); } } -type RepoDefaultBranchQueryResponse = { - repository: { - defaultBranchRef: { - name: string; - } | null; - }; -}; - /** * Fetches the repository's default branch name. * Used by security-scan which operates without a PR context. @@ -83,25 +201,34 @@ export async function fetchRepoDefaultBranch({ repository: { owner: string; repo: string }; }): Promise { try { - const result = await octokits.graphql( - REPO_DEFAULT_BRANCH_QUERY, - { + const repositoryResult = await fetchGraphQLField< + RepoDefaultBranchQueryResponse["repository"] + >({ + octokits, + query: REPO_DEFAULT_BRANCH_QUERY, + variables: { owner: repository.owner, repo: repository.repo, }, - ); + field: "repository", + queryDescription: `Default branch query for ${repository.owner}/${repository.repo}`, + }); + + const defaultBranchRef = repositoryResult?.defaultBranchRef; - if (!result.repository.defaultBranchRef) { + if (!defaultBranchRef) { throw new Error( `Default branch not found for ${repository.owner}/${repository.repo}`, ); } - return result.repository.defaultBranchRef.name; + return 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}`, + `Failed to fetch default branch for ${repository.owner}/${repository.repo}: ${ + error instanceof Error ? error.message : String(error) + }`, ); } } diff --git a/test/github/data/pr-fetcher.test.ts b/test/github/data/pr-fetcher.test.ts new file mode 100644 index 00000000..c412d33c --- /dev/null +++ b/test/github/data/pr-fetcher.test.ts @@ -0,0 +1,293 @@ +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; +import { graphql } from "@octokit/graphql"; +import { Octokit } from "@octokit/rest"; +import type { Octokits } from "../../../src/github/api/client"; +import { + fetchPRBranchData, + fetchRepoDefaultBranch, +} from "../../../src/github/data/pr-fetcher"; + +type StubResponse = { + status?: number; + contentType?: string; + body: string; +}; + +/** + * Builds an Octokits whose GraphQL client talks to a stubbed fetch, so the real + * @octokit/graphql envelope handling stays under test. + */ +function createStubOctokits(responses: StubResponse[]): { + octokits: Octokits; + requestCount: () => number; +} { + let calls = 0; + + const stubFetch = async () => { + const response = responses[Math.min(calls, responses.length - 1)]!; + calls += 1; + return new Response(response.body, { + status: response.status ?? 200, + headers: { + "content-type": response.contentType ?? "application/json", + }, + }); + }; + + return { + octokits: { + rest: new Octokit({ auth: "stub-token" }), + graphql: graphql.defaults({ + headers: { authorization: "token stub-token" }, + request: { fetch: stubFetch }, + }), + }, + requestCount: () => calls, + }; +} + +function prEnvelope(pullRequest: Record | null): StubResponse { + return { body: JSON.stringify({ data: { repository: { pullRequest } } }) }; +} + +const VALID_PR = { + title: "fix(frontend): stabilize session navigation", + body: "description", + baseRefName: "dev", + headRefName: "ross/app-1930-session-nav", + headRefOid: "abc123", +}; + +const REPOSITORY = { owner: "Factory-AI", repo: "factory-mono" }; + +describe("pr-fetcher", () => { + let consoleErrorSpy: ReturnType; + + beforeEach(() => { + consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + consoleErrorSpy.mockRestore(); + }); + + describe("fetchPRBranchData", () => { + it("returns the branch data from a well formed response", async () => { + const { octokits, requestCount } = createStubOctokits([ + prEnvelope(VALID_PR), + ]); + + const result = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }); + + expect(result).toEqual({ + baseRefName: "dev", + headRefName: "ross/app-1930-session-nav", + headRefOid: "abc123", + title: "fix(frontend): stabilize session navigation", + body: "description", + }); + expect(requestCount()).toBe(1); + }); + + it("defaults a null PR body to an empty string", async () => { + const { octokits } = createStubOctokits([ + prEnvelope({ ...VALID_PR, body: null }), + ]); + + const result = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }); + + expect(result.body).toBe(""); + }); + + it("reports the response instead of crashing when a 200 carries no GraphQL envelope", async () => { + const { octokits } = createStubOctokits([ + { + contentType: "text/html", + body: "unavailable", + }, + ]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + // The crash signature this guards against: a TypeError from + // dereferencing `prResult.repository` on an undefined payload. + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain( + 'PR query for Factory-AI/factory-mono#19051 returned no "repository" field', + ); + expect(error!.message).toContain("HTTP 200 text/html"); + expect(error!.message).toContain("unavailable"); + expect(error!.message).toContain( + "Failed to fetch PR branch data for PR #19051", + ); + }, 15000); + + it("reports the response when a 200 body is JSON without a data member", async () => { + const { octokits } = createStubOctokits([{ body: "{}" }]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain('returned no "repository" field'); + }, 15000); + + it("recovers when only the first attempt returns a malformed response", async () => { + const { octokits, requestCount } = createStubOctokits([ + { contentType: "text/html", body: "unavailable" }, + prEnvelope(VALID_PR), + ]); + + const result = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }); + + expect(result.headRefName).toBe("ross/app-1930-session-nav"); + expect(requestCount()).toBe(2); + }, 15000); + + it("does not retry a null repository, which is a stable answer", async () => { + const { octokits, requestCount } = createStubOctokits([ + { body: JSON.stringify({ data: { repository: null } }) }, + ]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain("PR #19051 not found"); + expect(requestCount()).toBe(1); + }); + + it("does not retry a null pull request, which is a stable answer", async () => { + const { octokits, requestCount } = createStubOctokits([prEnvelope(null)]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).toContain("PR #19051 not found"); + expect(requestCount()).toBe(1); + }); + + it("surfaces GraphQL error messages returned alongside a null data member", async () => { + const { octokits } = createStubOctokits([ + { + body: JSON.stringify({ + data: null, + errors: [{ message: "Bad credentials" }], + }), + }, + ]); + + const error = await fetchPRBranchData({ + octokits, + repository: REPOSITORY, + prNumber: 19051, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).toContain("Bad credentials"); + }, 15000); + }); + + describe("fetchRepoDefaultBranch", () => { + it("returns the default branch name", async () => { + const { octokits } = createStubOctokits([ + { + body: JSON.stringify({ + data: { repository: { defaultBranchRef: { name: "dev" } } }, + }), + }, + ]); + + await expect( + fetchRepoDefaultBranch({ octokits, repository: REPOSITORY }), + ).resolves.toBe("dev"); + }); + + it("reports the response instead of crashing when a 200 carries no GraphQL envelope", async () => { + const { octokits } = createStubOctokits([ + { contentType: "text/html", body: "unavailable" }, + ]); + + const error = await fetchRepoDefaultBranch({ + octokits, + repository: REPOSITORY, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).not.toContain("is not an object"); + expect(error!.message).toContain( + 'Default branch query for Factory-AI/factory-mono returned no "repository" field', + ); + expect(error!.message).toContain( + "Failed to fetch default branch for Factory-AI/factory-mono", + ); + }, 15000); + + it("does not retry a null repository, which is a stable answer", async () => { + const { octokits, requestCount } = createStubOctokits([ + { body: JSON.stringify({ data: { repository: null } }) }, + ]); + + const error = await fetchRepoDefaultBranch({ + octokits, + repository: REPOSITORY, + }).then( + () => undefined, + (caught: unknown) => caught as Error, + ); + + expect(error).toBeDefined(); + expect(error!.message).toContain( + "Default branch not found for Factory-AI/factory-mono", + ); + expect(requestCount()).toBe(1); + }); + }); +}); From 8aeeec054eec389636dd7f9c7446e342ee151b86 Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Tue, 25 Aug 2026 14:34:03 -0700 Subject: [PATCH 02/13] feat(review): tag tracking comments for classification Add a dedicated hidden marker to PR review tracking comments and preserve it across sanitized summary updates. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/entrypoints/generate-review-prompt.ts | 1 + src/github/operations/comments/common.ts | 40 +++++++++++++++++-- src/mcp/github-comment-server.ts | 7 +++- src/mcp/install-mcp-server.ts | 5 +++ src/tag/commands/review.ts | 4 +- src/tag/commands/security-review.ts | 4 +- src/tag/index.ts | 13 +++++- test/comment-logic.test.ts | 11 +++++ test/github/comments-common.test.ts | 28 +++++++++++++ test/mcp/install-mcp-server.test.ts | 24 +++++++++++ test/modes/tag/review-command.test.ts | 7 +++- .../modes/tag/security-review-command.test.ts | 7 +++- 12 files changed, 140 insertions(+), 11 deletions(-) create mode 100644 test/mcp/install-mcp-server.test.ts diff --git a/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index a4b72d9f..46137191 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -172,6 +172,7 @@ async function run() { owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), + includePrReviewMarker: true, allowedTools, mode: "tag", context, diff --git a/src/github/operations/comments/common.ts b/src/github/operations/comments/common.ts index c23b5e8b..9c5df6a5 100644 --- a/src/github/operations/comments/common.ts +++ b/src/github/operations/comments/common.ts @@ -1,4 +1,7 @@ import { GITHUB_SERVER_URL } from "../../api/config"; +import { sanitizeContent } from "../../utils/sanitizer"; + +export const DROID_PR_REVIEW_MARKER = ""; export function createJobRunLink( owner: string, @@ -18,7 +21,36 @@ export function createBranchLink( return `\n[View branch](${branchUrl})`; } -export type CommentType = "default" | "security" | "review_and_security"; +export type CommentType = + | "default" + | "review" + | "security" + | "security_scan" + | "review_and_security"; + +function isReviewCommentType(type: CommentType): boolean { + return ( + type === "review" || type === "security" || type === "review_and_security" + ); +} + +export function appendPrReviewMarker(content: string): string { + if (content.includes(DROID_PR_REVIEW_MARKER)) { + return content; + } + const trimmedContent = content.trimEnd(); + return trimmedContent + ? `${trimmedContent}\n\n${DROID_PR_REVIEW_MARKER}` + : DROID_PR_REVIEW_MARKER; +} + +export function prepareDroidCommentBody( + content: string, + includePrReviewMarker: boolean, +): string { + const sanitized = sanitizeContent(content); + return includePrReviewMarker ? appendPrReviewMarker(sanitized) : sanitized; +} export function createCommentBody( jobRunLink: string, @@ -28,13 +60,15 @@ export function createCommentBody( let message: string; if (type === "review_and_security") { message = "Droid is reviewing code and running a security check…"; - } else if (type === "security") { + } else if (type === "security" || type === "security_scan") { message = "Droid is running a security check…"; } else { message = "Droid is working…"; } - return `${message} + const body = `${message} ${jobRunLink}${branchLink}`; + + return isReviewCommentType(type) ? appendPrReviewMarker(body) : body; } diff --git a/src/mcp/github-comment-server.ts b/src/mcp/github-comment-server.ts index 864737c6..9a6db007 100644 --- a/src/mcp/github-comment-server.ts +++ b/src/mcp/github-comment-server.ts @@ -6,7 +6,7 @@ import { z } from "zod"; import { GITHUB_API_URL } from "../github/api/config"; import { Octokit } from "@octokit/rest"; import { updateDroidComment } from "../github/operations/comments/update-droid-comment"; -import { sanitizeContent } from "../github/utils/sanitizer"; +import { prepareDroidCommentBody } from "../github/operations/comments/common"; // Get repository information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -55,7 +55,10 @@ server.tool( const isPullRequestReviewComment = eventName === "pull_request_review_comment"; - let sanitizedBody = sanitizeContent(body); + let sanitizedBody = prepareDroidCommentBody( + body, + process.env.DROID_PR_REVIEW_MARKER === "true", + ); // CI Steward keeps its lifetime run budget in a marker on this comment. // Droid replaces the whole body, so carry the marker forward or every diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 9e1dc2e6..c2a82343 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -11,6 +11,7 @@ type PrepareConfigParams = { owner: string; repo: string; droidCommentId?: string; + includePrReviewMarker?: boolean; allowedTools: string[]; mode: "tag"; context: GitHubContext; @@ -56,6 +57,7 @@ export async function prepareMcpTools( owner, repo, droidCommentId, + includePrReviewMarker, allowedTools, context, mode: _mode, @@ -108,6 +110,9 @@ export async function prepareMcpTools( REPO_OWNER: owner, REPO_NAME: repo, ...(droidCommentId && { DROID_COMMENT_ID: droidCommentId }), + ...(includePrReviewMarker && { + DROID_PR_REVIEW_MARKER: "true", + }), ...(stewardPrNumber && { STEWARD_PR_NUMBER: stewardPrNumber }), ...(process.env.STEWARD_RUN_ID && { STEWARD_RUN_ID: process.env.STEWARD_RUN_ID, diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index 48e1b040..b558633f 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -37,7 +37,8 @@ export async function prepareReviewMode({ } const commentId = - trackingCommentId ?? (await createInitialComment(octokit.rest, context)).id; + trackingCommentId ?? + (await createInitialComment(octokit.rest, context, "review")).id; const prData = await fetchPRBranchData({ octokits: octokit, @@ -153,6 +154,7 @@ export async function prepareReviewMode({ owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), + includePrReviewMarker: true, allowedTools, mode: "tag", context, diff --git a/src/tag/commands/security-review.ts b/src/tag/commands/security-review.ts index 0b3a9fff..b8d4d148 100644 --- a/src/tag/commands/security-review.ts +++ b/src/tag/commands/security-review.ts @@ -38,7 +38,8 @@ export async function prepareSecurityReviewMode({ } const commentId = - trackingCommentId ?? (await createInitialComment(octokit.rest, context)).id; + trackingCommentId ?? + (await createInitialComment(octokit.rest, context, "security")).id; const prData = await fetchPRBranchData({ octokits: octokit, @@ -143,6 +144,7 @@ export async function prepareSecurityReviewMode({ owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), + includePrReviewMarker: true, allowedTools, mode: "tag", context, diff --git a/src/tag/index.ts b/src/tag/index.ts index b7bf60c9..8e727be0 100644 --- a/src/tag/index.ts +++ b/src/tag/index.ts @@ -109,12 +109,21 @@ export async function prepareTagExecution({ (context.inputs.automaticSecurityReview || commandContext?.command === "security" || commandContext?.command === "security-full"); + const isCodeReview = + context.inputs.automaticReview || + commandContext?.command === "review" || + commandContext?.command === "default" || + !commandContext; const commentType = isDualReview ? "review_and_security" : isSecurityOnly - ? "security" - : "default"; + ? commandContext?.command === "security-full" + ? "security_scan" + : "security" + : isCodeReview + ? "review" + : "default"; const commentData = await createInitialComment( octokit.rest, diff --git a/test/comment-logic.test.ts b/test/comment-logic.test.ts index f6308e1d..cdf299bc 100644 --- a/test/comment-logic.test.ts +++ b/test/comment-logic.test.ts @@ -3,6 +3,7 @@ import { updateCommentBody, type CommentUpdateInput, } from "../src/github/operations/comment-logic"; +import { DROID_PR_REVIEW_MARKER } from "../src/github/operations/comments/common"; describe("updateCommentBody", () => { const baseInput = { @@ -107,6 +108,16 @@ describe("updateCommentBody", () => { expect(result).not.toContain("running a security check"); }); + it("preserves the hidden PR review marker in the final summary", () => { + const input = { + ...baseInput, + currentBody: `Droid is reviewing code…\n\n${DROID_PR_REVIEW_MARKER}`, + }; + + const result = updateCommentBody(input); + expect(result).toContain(DROID_PR_REVIEW_MARKER); + }); + it("includes error details when provided", () => { const input = { ...baseInput, diff --git a/test/github/comments-common.test.ts b/test/github/comments-common.test.ts index 1c281a38..4216a926 100644 --- a/test/github/comments-common.test.ts +++ b/test/github/comments-common.test.ts @@ -1,8 +1,11 @@ import { describe, expect, it } from "bun:test"; import { + DROID_PR_REVIEW_MARKER, + appendPrReviewMarker, createBranchLink, createCommentBody, createJobRunLink, + prepareDroidCommentBody, } from "../../src/github/operations/comments/common"; import { GITHUB_SERVER_URL } from "../../src/github/api/config"; @@ -42,4 +45,29 @@ describe("comments common helpers", () => { expect(body).toContain(jobLink); expect(body).not.toContain("View branch"); }); + + it("adds the hidden marker to PR review tracking comments", () => { + const jobLink = createJobRunLink("factory", "droid", "run-102"); + + expect(createCommentBody(jobLink, "", "review")).toEndWith( + DROID_PR_REVIEW_MARKER, + ); + expect(createCommentBody(jobLink, "", "security")).toEndWith( + DROID_PR_REVIEW_MARKER, + ); + expect(createCommentBody(jobLink, "", "security_scan")).not.toContain( + DROID_PR_REVIEW_MARKER, + ); + expect(createCommentBody(jobLink)).not.toContain(DROID_PR_REVIEW_MARKER); + }); + + it("restores the PR review marker after sanitizing comment updates", () => { + const body = prepareDroidCommentBody( + `Review complete\n\n${DROID_PR_REVIEW_MARKER}`, + true, + ); + + expect(body).toBe(`Review complete\n\n${DROID_PR_REVIEW_MARKER}`); + expect(appendPrReviewMarker(body)).toBe(body); + }); }); diff --git a/test/mcp/install-mcp-server.test.ts b/test/mcp/install-mcp-server.test.ts new file mode 100644 index 00000000..58a79a1b --- /dev/null +++ b/test/mcp/install-mcp-server.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "bun:test"; +import { prepareMcpTools } from "../../src/mcp/install-mcp-server"; +import { createMockContext } from "../mockContext"; + +describe("prepareMcpTools", () => { + it("passes the PR review marker flag to the comment server", async () => { + const config = JSON.parse( + await prepareMcpTools({ + githubToken: "token", + owner: "factory", + repo: "droid", + droidCommentId: "123", + includePrReviewMarker: true, + allowedTools: ["github_comment___update_droid_comment"], + mode: "tag", + context: createMockContext({ isPR: true }), + }), + ); + + expect(config.mcpServers.github_comment.env.DROID_PR_REVIEW_MARKER).toBe( + "true", + ); + }); +}); diff --git a/test/modes/tag/review-command.test.ts b/test/modes/tag/review-command.test.ts index 2d530562..aa03e117 100644 --- a/test/modes/tag/review-command.test.ts +++ b/test/modes/tag/review-command.test.ts @@ -156,6 +156,7 @@ describe("prepareReviewMode", () => { expect(promptSpy).toHaveBeenCalled(); expect(mcpSpy).toHaveBeenCalledWith( expect.objectContaining({ + includePrReviewMarker: true, allowedTools: expect.arrayContaining([ "Execute", "github_comment___update_droid_comment", @@ -243,7 +244,11 @@ describe("prepareReviewMode", () => { githubToken: "token", }); - expect(createInitialSpy).toHaveBeenCalled(); + expect(createInitialSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "review", + ); expect(result.commentId).toBe(777); }); diff --git a/test/modes/tag/security-review-command.test.ts b/test/modes/tag/security-review-command.test.ts index 69ec59ae..40c3a797 100644 --- a/test/modes/tag/security-review-command.test.ts +++ b/test/modes/tag/security-review-command.test.ts @@ -147,6 +147,7 @@ describe("prepareSecurityReviewMode", () => { expect(promptSpy).toHaveBeenCalled(); expect(mcpSpy).toHaveBeenCalledWith( expect.objectContaining({ + includePrReviewMarker: true, allowedTools: expect.arrayContaining([ "Execute", "Task", @@ -196,7 +197,11 @@ describe("prepareSecurityReviewMode", () => { githubToken: "token", }); - expect(createInitialSpy).toHaveBeenCalled(); + expect(createInitialSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "security", + ); expect(result.commentId).toBe(777); }); From 9313125fe995d654a3e14372a2c4f9f761fdce80 Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Tue, 25 Aug 2026 16:45:45 -0700 Subject: [PATCH 03/13] refactor(review): use typed validation marker Replace the review-specific marker with a generic PR validation marker carrying an explicit source value for future validation comment types. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/entrypoints/generate-review-prompt.ts | 2 +- src/github/operations/comments/common.ts | 39 ++++++++++++------- src/mcp/github-comment-server.ts | 14 ++++--- src/mcp/install-mcp-server.ts | 9 +++-- src/tag/commands/review.ts | 2 +- src/tag/commands/security-review.ts | 2 +- test/comment-logic.test.ts | 9 +++-- test/github/comments-common.test.ts | 29 +++++++------- test/mcp/install-mcp-server.test.ts | 10 ++--- test/modes/tag/review-command.test.ts | 2 +- .../modes/tag/security-review-command.test.ts | 2 +- 11 files changed, 69 insertions(+), 51 deletions(-) diff --git a/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 46137191..01d00b43 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -172,7 +172,7 @@ async function run() { owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), - includePrReviewMarker: true, + prValidationSource: "review", allowedTools, mode: "tag", context, diff --git a/src/github/operations/comments/common.ts b/src/github/operations/comments/common.ts index 9c5df6a5..488041bb 100644 --- a/src/github/operations/comments/common.ts +++ b/src/github/operations/comments/common.ts @@ -1,7 +1,11 @@ import { GITHUB_SERVER_URL } from "../../api/config"; import { sanitizeContent } from "../../utils/sanitizer"; -export const DROID_PR_REVIEW_MARKER = ""; +export type PrValidationSource = "review"; + +export function createPrValidationMarker(source: PrValidationSource): string { + return ``; +} export function createJobRunLink( owner: string, @@ -28,28 +32,36 @@ export type CommentType = | "security_scan" | "review_and_security"; -function isReviewCommentType(type: CommentType): boolean { - return ( - type === "review" || type === "security" || type === "review_and_security" - ); +function getPrValidationSource( + type: CommentType, +): PrValidationSource | undefined { + return type === "review" || + type === "security" || + type === "review_and_security" + ? "review" + : undefined; } -export function appendPrReviewMarker(content: string): string { - if (content.includes(DROID_PR_REVIEW_MARKER)) { +export function appendPrValidationMarker( + content: string, + source: PrValidationSource, +): string { + const marker = createPrValidationMarker(source); + if (content.includes(marker)) { return content; } const trimmedContent = content.trimEnd(); - return trimmedContent - ? `${trimmedContent}\n\n${DROID_PR_REVIEW_MARKER}` - : DROID_PR_REVIEW_MARKER; + return trimmedContent ? `${trimmedContent}\n\n${marker}` : marker; } export function prepareDroidCommentBody( content: string, - includePrReviewMarker: boolean, + prValidationSource?: PrValidationSource, ): string { const sanitized = sanitizeContent(content); - return includePrReviewMarker ? appendPrReviewMarker(sanitized) : sanitized; + return prValidationSource + ? appendPrValidationMarker(sanitized, prValidationSource) + : sanitized; } export function createCommentBody( @@ -70,5 +82,6 @@ export function createCommentBody( ${jobRunLink}${branchLink}`; - return isReviewCommentType(type) ? appendPrReviewMarker(body) : body; + const source = getPrValidationSource(type); + return source ? appendPrValidationMarker(body, source) : body; } diff --git a/src/mcp/github-comment-server.ts b/src/mcp/github-comment-server.ts index 9a6db007..7536f18f 100644 --- a/src/mcp/github-comment-server.ts +++ b/src/mcp/github-comment-server.ts @@ -6,7 +6,10 @@ import { z } from "zod"; import { GITHUB_API_URL } from "../github/api/config"; import { Octokit } from "@octokit/rest"; import { updateDroidComment } from "../github/operations/comments/update-droid-comment"; -import { prepareDroidCommentBody } from "../github/operations/comments/common"; +import { + prepareDroidCommentBody, + type PrValidationSource, +} from "../github/operations/comments/common"; // Get repository information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -55,10 +58,11 @@ server.tool( const isPullRequestReviewComment = eventName === "pull_request_review_comment"; - let sanitizedBody = prepareDroidCommentBody( - body, - process.env.DROID_PR_REVIEW_MARKER === "true", - ); + const prValidationSource: PrValidationSource | undefined = + process.env.DROID_PR_VALIDATION_SOURCE === "review" + ? "review" + : undefined; + let sanitizedBody = prepareDroidCommentBody(body, prValidationSource); // CI Steward keeps its lifetime run budget in a marker on this comment. // Droid replaces the whole body, so carry the marker forward or every diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index c2a82343..802a15de 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -4,6 +4,7 @@ import path from "path"; import { GITHUB_API_URL, GITHUB_SERVER_URL } from "../github/api/config"; import type { GitHubContext } from "../github/context"; import { isEntityContext } from "../github/context"; +import type { PrValidationSource } from "../github/operations/comments/common"; import { Octokit } from "@octokit/rest"; type PrepareConfigParams = { @@ -11,7 +12,7 @@ type PrepareConfigParams = { owner: string; repo: string; droidCommentId?: string; - includePrReviewMarker?: boolean; + prValidationSource?: PrValidationSource; allowedTools: string[]; mode: "tag"; context: GitHubContext; @@ -57,7 +58,7 @@ export async function prepareMcpTools( owner, repo, droidCommentId, - includePrReviewMarker, + prValidationSource, allowedTools, context, mode: _mode, @@ -110,8 +111,8 @@ export async function prepareMcpTools( REPO_OWNER: owner, REPO_NAME: repo, ...(droidCommentId && { DROID_COMMENT_ID: droidCommentId }), - ...(includePrReviewMarker && { - DROID_PR_REVIEW_MARKER: "true", + ...(prValidationSource && { + DROID_PR_VALIDATION_SOURCE: prValidationSource, }), ...(stewardPrNumber && { STEWARD_PR_NUMBER: stewardPrNumber }), ...(process.env.STEWARD_RUN_ID && { diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index b558633f..0666a218 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -154,7 +154,7 @@ export async function prepareReviewMode({ owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), - includePrReviewMarker: true, + prValidationSource: "review", allowedTools, mode: "tag", context, diff --git a/src/tag/commands/security-review.ts b/src/tag/commands/security-review.ts index b8d4d148..e17bafee 100644 --- a/src/tag/commands/security-review.ts +++ b/src/tag/commands/security-review.ts @@ -144,7 +144,7 @@ export async function prepareSecurityReviewMode({ owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), - includePrReviewMarker: true, + prValidationSource: "review", allowedTools, mode: "tag", context, diff --git a/test/comment-logic.test.ts b/test/comment-logic.test.ts index cdf299bc..e14fc0be 100644 --- a/test/comment-logic.test.ts +++ b/test/comment-logic.test.ts @@ -3,7 +3,7 @@ import { updateCommentBody, type CommentUpdateInput, } from "../src/github/operations/comment-logic"; -import { DROID_PR_REVIEW_MARKER } from "../src/github/operations/comments/common"; +import { createPrValidationMarker } from "../src/github/operations/comments/common"; describe("updateCommentBody", () => { const baseInput = { @@ -108,14 +108,15 @@ describe("updateCommentBody", () => { expect(result).not.toContain("running a security check"); }); - it("preserves the hidden PR review marker in the final summary", () => { + it("preserves the hidden PR validation marker in the final summary", () => { + const marker = createPrValidationMarker("review"); const input = { ...baseInput, - currentBody: `Droid is reviewing code…\n\n${DROID_PR_REVIEW_MARKER}`, + currentBody: `Droid is reviewing code…\n\n${marker}`, }; const result = updateCommentBody(input); - expect(result).toContain(DROID_PR_REVIEW_MARKER); + expect(result).toContain(marker); }); it("includes error details when provided", () => { diff --git a/test/github/comments-common.test.ts b/test/github/comments-common.test.ts index 4216a926..2b4ff00a 100644 --- a/test/github/comments-common.test.ts +++ b/test/github/comments-common.test.ts @@ -1,10 +1,10 @@ import { describe, expect, it } from "bun:test"; import { - DROID_PR_REVIEW_MARKER, - appendPrReviewMarker, + appendPrValidationMarker, createBranchLink, createCommentBody, createJobRunLink, + createPrValidationMarker, prepareDroidCommentBody, } from "../../src/github/operations/comments/common"; import { GITHUB_SERVER_URL } from "../../src/github/api/config"; @@ -48,26 +48,25 @@ describe("comments common helpers", () => { it("adds the hidden marker to PR review tracking comments", () => { const jobLink = createJobRunLink("factory", "droid", "run-102"); + const marker = createPrValidationMarker("review"); - expect(createCommentBody(jobLink, "", "review")).toEndWith( - DROID_PR_REVIEW_MARKER, - ); - expect(createCommentBody(jobLink, "", "security")).toEndWith( - DROID_PR_REVIEW_MARKER, - ); + expect(marker).toBe(""); + expect(createCommentBody(jobLink, "", "review")).toEndWith(marker); + expect(createCommentBody(jobLink, "", "security")).toEndWith(marker); expect(createCommentBody(jobLink, "", "security_scan")).not.toContain( - DROID_PR_REVIEW_MARKER, + marker, ); - expect(createCommentBody(jobLink)).not.toContain(DROID_PR_REVIEW_MARKER); + expect(createCommentBody(jobLink)).not.toContain(marker); }); - it("restores the PR review marker after sanitizing comment updates", () => { + it("restores the PR validation marker after sanitizing comment updates", () => { + const marker = createPrValidationMarker("review"); const body = prepareDroidCommentBody( - `Review complete\n\n${DROID_PR_REVIEW_MARKER}`, - true, + `Review complete\n\n${marker}`, + "review", ); - expect(body).toBe(`Review complete\n\n${DROID_PR_REVIEW_MARKER}`); - expect(appendPrReviewMarker(body)).toBe(body); + expect(body).toBe(`Review complete\n\n${marker}`); + expect(appendPrValidationMarker(body, "review")).toBe(body); }); }); diff --git a/test/mcp/install-mcp-server.test.ts b/test/mcp/install-mcp-server.test.ts index 58a79a1b..a63d2dfd 100644 --- a/test/mcp/install-mcp-server.test.ts +++ b/test/mcp/install-mcp-server.test.ts @@ -3,22 +3,22 @@ import { prepareMcpTools } from "../../src/mcp/install-mcp-server"; import { createMockContext } from "../mockContext"; describe("prepareMcpTools", () => { - it("passes the PR review marker flag to the comment server", async () => { + it("passes the PR validation source to the comment server", async () => { const config = JSON.parse( await prepareMcpTools({ githubToken: "token", owner: "factory", repo: "droid", droidCommentId: "123", - includePrReviewMarker: true, + prValidationSource: "review", allowedTools: ["github_comment___update_droid_comment"], mode: "tag", context: createMockContext({ isPR: true }), }), ); - expect(config.mcpServers.github_comment.env.DROID_PR_REVIEW_MARKER).toBe( - "true", - ); + expect( + config.mcpServers.github_comment.env.DROID_PR_VALIDATION_SOURCE, + ).toBe("review"); }); }); diff --git a/test/modes/tag/review-command.test.ts b/test/modes/tag/review-command.test.ts index aa03e117..a463753c 100644 --- a/test/modes/tag/review-command.test.ts +++ b/test/modes/tag/review-command.test.ts @@ -156,7 +156,7 @@ describe("prepareReviewMode", () => { expect(promptSpy).toHaveBeenCalled(); expect(mcpSpy).toHaveBeenCalledWith( expect.objectContaining({ - includePrReviewMarker: true, + prValidationSource: "review", allowedTools: expect.arrayContaining([ "Execute", "github_comment___update_droid_comment", diff --git a/test/modes/tag/security-review-command.test.ts b/test/modes/tag/security-review-command.test.ts index 40c3a797..7c5cc335 100644 --- a/test/modes/tag/security-review-command.test.ts +++ b/test/modes/tag/security-review-command.test.ts @@ -147,7 +147,7 @@ describe("prepareSecurityReviewMode", () => { expect(promptSpy).toHaveBeenCalled(); expect(mcpSpy).toHaveBeenCalledWith( expect.objectContaining({ - includePrReviewMarker: true, + prValidationSource: "review", allowedTools: expect.arrayContaining([ "Execute", "Task", From 0da0ad48d67be1a7d5d43fffe1c8501e88a79bf5 Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Tue, 25 Aug 2026 17:03:04 -0700 Subject: [PATCH 04/13] refactor: centralize droid run types Resolve a typed run identity before tracking comments are created, pass it through each mode and MCP server, and derive PR validation metadata independently of comment presentation. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/entrypoints/generate-review-prompt.ts | 13 ++-- src/entrypoints/prepare-validator.ts | 5 +- src/github/operations/comments/common.ts | 25 ++----- .../operations/comments/create-initial.ts | 12 +++- src/mcp/github-comment-server.ts | 16 ++--- src/mcp/install-mcp-server.ts | 10 ++- src/run-type.ts | 71 +++++++++++++++++++ src/steward/index.ts | 4 +- src/tag/commands/fill.ts | 10 ++- src/tag/commands/review-validator.ts | 12 +++- src/tag/commands/review.ts | 10 +-- src/tag/commands/security-review.ts | 10 +-- src/tag/commands/security-scan.ts | 7 +- src/tag/index.ts | 32 +++++---- test/github/comments-common.test.ts | 8 ++- test/integration/review-flow.test.ts | 21 ++++++ test/mcp/install-mcp-server.test.ts | 11 +-- test/modes/tag/fill-command.test.ts | 6 +- test/modes/tag/review-command.test.ts | 10 ++- .../modes/tag/security-review-command.test.ts | 9 +-- test/run-type.test.ts | 58 +++++++++++++++ test/steward.test.ts | 4 ++ 22 files changed, 269 insertions(+), 95 deletions(-) create mode 100644 src/run-type.ts create mode 100644 test/run-type.test.ts diff --git a/src/entrypoints/generate-review-prompt.ts b/src/entrypoints/generate-review-prompt.ts index 01d00b43..d66879e2 100644 --- a/src/entrypoints/generate-review-prompt.ts +++ b/src/entrypoints/generate-review-prompt.ts @@ -18,11 +18,17 @@ import { normalizeDroidArgs, parseAllowedTools } from "../utils/parse-tools"; import { resolveReviewConfig } from "../utils/review-depth"; import { applyModelPolicyFallback } from "../utils/model-policy"; import { retryWithBackoff } from "../utils/retry"; +import { DroidRunType, setDroidRunType } from "../run-type"; async function run() { try { const githubToken = process.env.GITHUB_TOKEN!; const reviewType = process.env.REVIEW_TYPE || "code"; + const runType = + reviewType === "security" + ? DroidRunType.SecurityReview + : DroidRunType.Review; + setDroidRunType(runType); const commentId = parseInt(process.env.DROID_COMMENT_ID || "0"); if (!commentId) { @@ -123,11 +129,6 @@ async function run() { includeSuggestions, }); - // Set run type - const runType = - reviewType === "security" ? "droid-security-review" : "droid-review"; - core.exportVariable("DROID_EXEC_RUN_TYPE", runType); - const rawUserArgs = process.env.DROID_ARGS || ""; const normalizedUserArgs = normalizeDroidArgs(rawUserArgs); const userAllowedMCPTools = parseAllowedTools(normalizedUserArgs).filter( @@ -172,7 +173,7 @@ async function run() { owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), - prValidationSource: "review", + runType, allowedTools, mode: "tag", context, diff --git a/src/entrypoints/prepare-validator.ts b/src/entrypoints/prepare-validator.ts index f0aa23bf..6e15e94f 100644 --- a/src/entrypoints/prepare-validator.ts +++ b/src/entrypoints/prepare-validator.ts @@ -6,6 +6,7 @@ import { setupGitHubToken } from "../github/token"; import { createOctokit } from "../github/api/client"; import { parseGitHubContext, isEntityContext } from "../github/context"; import { prepareReviewValidatorMode } from "../tag/commands/review-validator"; +import { DroidRunType, setDroidRunType } from "../run-type"; async function run() { try { @@ -14,6 +15,8 @@ async function run() { if (!isEntityContext(context) || !context.isPR) { throw new Error("prepare-validator requires a pull request context"); } + const runType = DroidRunType.Review; + setDroidRunType(runType); // Validate that Pass 1 produced a valid candidates JSON file. // If the file is missing or invalid, skip Pass 2 gracefully rather than @@ -57,12 +60,12 @@ async function run() { if (!trackingCommentId || Number.isNaN(trackingCommentId)) { throw new Error("DROID_COMMENT_ID is required for validator run"); } - const result = await prepareReviewValidatorMode({ context, octokit, githubToken, trackingCommentId, + runType, }); core.setOutput("github_token", githubToken); diff --git a/src/github/operations/comments/common.ts b/src/github/operations/comments/common.ts index 488041bb..2dad09e1 100644 --- a/src/github/operations/comments/common.ts +++ b/src/github/operations/comments/common.ts @@ -25,22 +25,7 @@ export function createBranchLink( return `\n[View branch](${branchUrl})`; } -export type CommentType = - | "default" - | "review" - | "security" - | "security_scan" - | "review_and_security"; - -function getPrValidationSource( - type: CommentType, -): PrValidationSource | undefined { - return type === "review" || - type === "security" || - type === "review_and_security" - ? "review" - : undefined; -} +export type CommentType = "default" | "security" | "review_and_security"; export function appendPrValidationMarker( content: string, @@ -68,11 +53,12 @@ export function createCommentBody( jobRunLink: string, branchLink: string = "", type: CommentType = "default", + prValidationSource?: PrValidationSource, ): string { let message: string; if (type === "review_and_security") { message = "Droid is reviewing code and running a security check…"; - } else if (type === "security" || type === "security_scan") { + } else if (type === "security") { message = "Droid is running a security check…"; } else { message = "Droid is working…"; @@ -82,6 +68,7 @@ export function createCommentBody( ${jobRunLink}${branchLink}`; - const source = getPrValidationSource(type); - return source ? appendPrValidationMarker(body, source) : body; + return prValidationSource + ? appendPrValidationMarker(body, prValidationSource) + : body; } diff --git a/src/github/operations/comments/create-initial.ts b/src/github/operations/comments/create-initial.ts index 2292fb84..0a27f445 100644 --- a/src/github/operations/comments/create-initial.ts +++ b/src/github/operations/comments/create-initial.ts @@ -17,6 +17,10 @@ import { type ParsedGitHubContext, } from "../../context"; import type { Octokit } from "@octokit/rest"; +import { + prValidationSourceForRunType, + type DroidRunType, +} from "../../../run-type"; const DROID_APP_BOT_ID = 209825114; @@ -24,11 +28,17 @@ export async function createInitialComment( octokit: Octokit, context: ParsedGitHubContext, commentType: CommentType = "default", + runType?: DroidRunType, ) { const { owner, repo } = context.repository; const jobRunLink = createJobRunLink(owner, repo, context.runId); - const initialBody = createCommentBody(jobRunLink, "", commentType); + const initialBody = createCommentBody( + jobRunLink, + "", + commentType, + prValidationSourceForRunType(runType), + ); try { let response; diff --git a/src/mcp/github-comment-server.ts b/src/mcp/github-comment-server.ts index 7536f18f..4982a5ab 100644 --- a/src/mcp/github-comment-server.ts +++ b/src/mcp/github-comment-server.ts @@ -6,10 +6,8 @@ import { z } from "zod"; import { GITHUB_API_URL } from "../github/api/config"; import { Octokit } from "@octokit/rest"; import { updateDroidComment } from "../github/operations/comments/update-droid-comment"; -import { - prepareDroidCommentBody, - type PrValidationSource, -} from "../github/operations/comments/common"; +import { prepareDroidCommentBody } from "../github/operations/comments/common"; +import { parseDroidRunType, prValidationSourceForRunType } from "../run-type"; // Get repository information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -58,11 +56,11 @@ server.tool( const isPullRequestReviewComment = eventName === "pull_request_review_comment"; - const prValidationSource: PrValidationSource | undefined = - process.env.DROID_PR_VALIDATION_SOURCE === "review" - ? "review" - : undefined; - let sanitizedBody = prepareDroidCommentBody(body, prValidationSource); + const runType = parseDroidRunType(process.env.DROID_EXEC_RUN_TYPE); + let sanitizedBody = prepareDroidCommentBody( + body, + prValidationSourceForRunType(runType), + ); // CI Steward keeps its lifetime run budget in a marker on this comment. // Droid replaces the whole body, so carry the marker forward or every diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 802a15de..8d48b286 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -4,7 +4,7 @@ import path from "path"; import { GITHUB_API_URL, GITHUB_SERVER_URL } from "../github/api/config"; import type { GitHubContext } from "../github/context"; import { isEntityContext } from "../github/context"; -import type { PrValidationSource } from "../github/operations/comments/common"; +import type { DroidRunType } from "../run-type"; import { Octokit } from "@octokit/rest"; type PrepareConfigParams = { @@ -12,7 +12,7 @@ type PrepareConfigParams = { owner: string; repo: string; droidCommentId?: string; - prValidationSource?: PrValidationSource; + runType: DroidRunType; allowedTools: string[]; mode: "tag"; context: GitHubContext; @@ -58,7 +58,7 @@ export async function prepareMcpTools( owner, repo, droidCommentId, - prValidationSource, + runType, allowedTools, context, mode: _mode, @@ -111,9 +111,7 @@ export async function prepareMcpTools( REPO_OWNER: owner, REPO_NAME: repo, ...(droidCommentId && { DROID_COMMENT_ID: droidCommentId }), - ...(prValidationSource && { - DROID_PR_VALIDATION_SOURCE: prValidationSource, - }), + DROID_EXEC_RUN_TYPE: runType, ...(stewardPrNumber && { STEWARD_PR_NUMBER: stewardPrNumber }), ...(process.env.STEWARD_RUN_ID && { STEWARD_RUN_ID: process.env.STEWARD_RUN_ID, diff --git a/src/run-type.ts b/src/run-type.ts new file mode 100644 index 00000000..191e1fcb --- /dev/null +++ b/src/run-type.ts @@ -0,0 +1,71 @@ +import * as core from "@actions/core"; +import type { DroidCommand } from "./core/review/triggers/parse-command"; +import type { PrValidationSource } from "./github/operations/comments/common"; + +export enum DroidRunType { + Review = "droid-review", + SecurityReview = "droid-security-review", + Fill = "droid-fill", + SecurityScan = "droid-security-scan", + CiSteward = "ci-steward", +} + +export function setDroidRunType(runType: DroidRunType): void { + core.exportVariable("DROID_EXEC_RUN_TYPE", runType); +} + +export function assertDroidRunType( + runType: DroidRunType, + expected: DroidRunType, +): void { + if (runType !== expected) { + throw new Error(`Expected run type ${expected}, received ${runType}`); + } +} + +export function parseDroidRunType( + value: string | undefined, +): DroidRunType | undefined { + return Object.values(DroidRunType).includes(value as DroidRunType) + ? (value as DroidRunType) + : undefined; +} + +export function prValidationSourceForRunType( + runType: DroidRunType | undefined, +): PrValidationSource | undefined { + return runType === DroidRunType.Review || + runType === DroidRunType.SecurityReview + ? "review" + : undefined; +} + +export function resolveTagRunType({ + automaticReview, + automaticSecurityReview, + command, +}: { + automaticReview: boolean; + automaticSecurityReview: boolean; + command: DroidCommand | null; +}): DroidRunType { + if (automaticReview) { + return DroidRunType.Review; + } + if (automaticSecurityReview) { + return DroidRunType.SecurityReview; + } + + switch (command) { + case "fill": + return DroidRunType.Fill; + case "security": + return DroidRunType.SecurityReview; + case "security-full": + return DroidRunType.SecurityScan; + case "review": + case "default": + case null: + return DroidRunType.Review; + } +} diff --git a/src/steward/index.ts b/src/steward/index.ts index 2eaee268..cd57639c 100644 --- a/src/steward/index.ts +++ b/src/steward/index.ts @@ -2,6 +2,7 @@ import * as core from "@actions/core"; import { mkdir, writeFile } from "fs/promises"; import type { WorkflowRunEvent } from "@octokit/webhooks-types"; import type { AutomationContext } from "../github/context"; +import { DroidRunType } from "../run-type"; import type { Octokits } from "../github/api/client"; import { prepareMcpTools } from "../mcp/install-mcp-server"; import { @@ -161,6 +162,7 @@ export async function prepareStewardMode( context: AutomationContext, octokit: Octokits, githubToken: string, + runType: DroidRunType.CiSteward = DroidRunType.CiSteward, ): Promise { if (context.eventName !== "workflow_run") { throw new Error("CI Steward requires a workflow_run event"); @@ -361,7 +363,6 @@ export async function prepareStewardMode( core.setOutput("steward_pr_number", pr.number.toString()); core.setOutput("run_code_review", "false"); core.exportVariable("STEWARD_PR_NUMBER", pr.number.toString()); - core.exportVariable("DROID_EXEC_RUN_TYPE", "ci-steward"); // The comment server rebuilds the marker from these instead of re-reading // the comment, so a transient read failure can no longer drop it and reset // the pull request's lifetime count. They are passed as two digit-only @@ -421,6 +422,7 @@ ${config.instructions || "(none)"}`; owner: context.repository.owner, repo: context.repository.repo, droidCommentId: comment.data.id.toString(), + runType, allowedTools, mode: "tag", context, diff --git a/src/tag/commands/fill.ts b/src/tag/commands/fill.ts index 9a6f9edf..d939ccda 100644 --- a/src/tag/commands/fill.ts +++ b/src/tag/commands/fill.ts @@ -10,12 +10,14 @@ import { isEntityContext } from "../../github/context"; import type { Octokits } from "../../github/api/client"; import type { PrepareResult } from "../../prepare/types"; import { applyModelPolicyFallback } from "../../utils/model-policy"; +import { assertDroidRunType, DroidRunType } from "../../run-type"; type FillCommandOptions = { context: GitHubContext; octokit: Octokits; githubToken: string; trackingCommentId?: number; + runType?: DroidRunType; }; export async function prepareFillMode({ @@ -23,6 +25,7 @@ export async function prepareFillMode({ octokit, githubToken, trackingCommentId, + runType = DroidRunType.Fill, }: FillCommandOptions): Promise { if (!isEntityContext(context)) { throw new Error("Fill command requires an entity event context"); @@ -31,9 +34,11 @@ export async function prepareFillMode({ if (!context.isPR) { throw new Error("Fill command is only supported on pull requests"); } + assertDroidRunType(runType, DroidRunType.Fill); const commentId = - trackingCommentId ?? (await createInitialComment(octokit.rest, context)).id; + trackingCommentId ?? + (await createInitialComment(octokit.rest, context, "default", runType)).id; const prData = await fetchPRBranchData({ octokits: octokit, @@ -58,8 +63,6 @@ export async function prepareFillMode({ }, generatePrompt: generateFillPrompt, }); - core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-fill"); - const rawUserArgs = process.env.DROID_ARGS || ""; const normalizedUserArgs = normalizeDroidArgs(rawUserArgs); const userAllowedMCPTools = parseAllowedTools(normalizedUserArgs).filter( @@ -85,6 +88,7 @@ export async function prepareFillMode({ owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), + runType, allowedTools, mode: "tag", context, diff --git a/src/tag/commands/review-validator.ts b/src/tag/commands/review-validator.ts index 80078fda..546d2334 100644 --- a/src/tag/commands/review-validator.ts +++ b/src/tag/commands/review-validator.ts @@ -13,17 +13,25 @@ import type { PrepareResult } from "../../prepare/types"; import { generateReviewValidatorPrompt } from "../../create-prompt/templates/review-validator-prompt"; import { resolveReviewConfig } from "../../utils/review-depth"; import { applyModelPolicyFallback } from "../../utils/model-policy"; +import { assertDroidRunType, DroidRunType } from "../../run-type"; export async function prepareReviewValidatorMode(options: { context: GitHubContext; octokit: Octokits; githubToken: string; trackingCommentId: number; + runType?: DroidRunType; }): Promise { - const { context, octokit, trackingCommentId } = options; + const { + context, + octokit, + trackingCommentId, + runType = DroidRunType.Review, + } = options; if (!isEntityContext(context) || !context.isPR) { throw new Error("review validator mode requires pull request context"); } + assertDroidRunType(runType, DroidRunType.Review); const prData = await fetchPRBranchData({ octokits: octokit, @@ -61,8 +69,6 @@ export async function prepareReviewValidatorMode(options: { includeSuggestions, }); - core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-review"); - const rawUserArgs = process.env.DROID_ARGS || ""; const normalizedUserArgs = stripToolSelectionArgs( normalizeDroidArgs(rawUserArgs), diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index 0666a218..5c6eb8a8 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -14,12 +14,14 @@ import type { PrepareResult } from "../../prepare/types"; import { resolveReviewConfig } from "../../utils/review-depth"; import { applyModelPolicyFallback } from "../../utils/model-policy"; import { retryWithBackoff } from "../../utils/retry"; +import { assertDroidRunType, DroidRunType } from "../../run-type"; type ReviewCommandOptions = { context: GitHubContext; octokit: Octokits; githubToken: string; trackingCommentId?: number; + runType?: DroidRunType; }; export async function prepareReviewMode({ @@ -27,6 +29,7 @@ export async function prepareReviewMode({ octokit, githubToken, trackingCommentId, + runType = DroidRunType.Review, }: ReviewCommandOptions): Promise { if (!isEntityContext(context)) { throw new Error("Review command requires an entity event context"); @@ -35,10 +38,11 @@ export async function prepareReviewMode({ if (!context.isPR) { throw new Error("Review command is only supported on pull requests"); } + assertDroidRunType(runType, DroidRunType.Review); const commentId = trackingCommentId ?? - (await createInitialComment(octokit.rest, context, "review")).id; + (await createInitialComment(octokit.rest, context, "default", runType)).id; const prData = await fetchPRBranchData({ octokits: octokit, @@ -109,8 +113,6 @@ export async function prepareReviewMode({ reviewArtifacts, includeSuggestions, }); - core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-review"); - const rawUserArgs = process.env.DROID_ARGS || ""; const normalizedUserArgs = normalizeDroidArgs(rawUserArgs); const userAllowedMCPTools = parseAllowedTools(normalizedUserArgs).filter( @@ -154,7 +156,7 @@ export async function prepareReviewMode({ owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), - prValidationSource: "review", + runType, allowedTools, mode: "tag", context, diff --git a/src/tag/commands/security-review.ts b/src/tag/commands/security-review.ts index e17bafee..df1790cb 100644 --- a/src/tag/commands/security-review.ts +++ b/src/tag/commands/security-review.ts @@ -13,12 +13,14 @@ import type { Octokits } from "../../github/api/client"; import type { PrepareResult } from "../../prepare/types"; import { applyModelPolicyFallback } from "../../utils/model-policy"; import { resolveReviewConfig } from "../../utils/review-depth"; +import { assertDroidRunType, DroidRunType } from "../../run-type"; type SecurityReviewCommandOptions = { context: GitHubContext; octokit: Octokits; githubToken: string; trackingCommentId?: number; + runType?: DroidRunType; }; export async function prepareSecurityReviewMode({ @@ -26,6 +28,7 @@ export async function prepareSecurityReviewMode({ octokit, githubToken, trackingCommentId, + runType = DroidRunType.SecurityReview, }: SecurityReviewCommandOptions): Promise { if (!isEntityContext(context)) { throw new Error("Security review command requires an entity event context"); @@ -36,10 +39,11 @@ export async function prepareSecurityReviewMode({ "Security review command is only supported on pull requests", ); } + assertDroidRunType(runType, DroidRunType.SecurityReview); const commentId = trackingCommentId ?? - (await createInitialComment(octokit.rest, context, "security")).id; + (await createInitialComment(octokit.rest, context, "security", runType)).id; const prData = await fetchPRBranchData({ octokits: octokit, @@ -100,8 +104,6 @@ export async function prepareSecurityReviewMode({ generatePrompt: generateSecurityCandidatesPrompt, reviewArtifacts, }); - core.exportVariable("DROID_EXEC_RUN_TYPE", "droid-security-review"); - core.setOutput("install_security_skills", "true"); const rawUserArgs = process.env.DROID_ARGS || ""; @@ -144,7 +146,7 @@ export async function prepareSecurityReviewMode({ owner: context.repository.owner, repo: context.repository.repo, droidCommentId: commentId.toString(), - prValidationSource: "review", + runType, allowedTools, mode: "tag", context, diff --git a/src/tag/commands/security-scan.ts b/src/tag/commands/security-scan.ts index ac4ccf44..5d2d1c9f 100644 --- a/src/tag/commands/security-scan.ts +++ b/src/tag/commands/security-scan.ts @@ -9,6 +9,7 @@ import { generateSecurityReportPrompt } from "../../create-prompt/templates/secu import type { Octokits } from "../../github/api/client"; import type { PrepareResult } from "../../prepare/types"; import { applyModelPolicyFallback } from "../../utils/model-policy"; +import { assertDroidRunType, DroidRunType } from "../../run-type"; export type ScanScope = { type: "full" } | { type: "scheduled"; days: number }; @@ -17,6 +18,7 @@ type SecurityScanCommandOptions = { octokit: Octokits; githubToken: string; scanScope: ScanScope; + runType?: DroidRunType; }; export async function prepareSecurityScanMode({ @@ -24,10 +26,12 @@ export async function prepareSecurityScanMode({ octokit, githubToken, scanScope, + runType = DroidRunType.SecurityScan, }: SecurityScanCommandOptions): Promise { if (!isEntityContext(context)) { throw new Error("Security scan command requires an entity event context"); } + assertDroidRunType(runType, DroidRunType.SecurityScan); // Fetch the repository's default branch (could be main, master, develop, etc.) const defaultBranch = await fetchRepoDefaultBranch({ @@ -51,8 +55,6 @@ export async function prepareSecurityScanMode({ 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"); @@ -79,6 +81,7 @@ export async function prepareSecurityScanMode({ githubToken, owner: context.repository.owner, repo: context.repository.repo, + runType, allowedTools, mode: "tag", context, diff --git a/src/tag/index.ts b/src/tag/index.ts index 8e727be0..335ad25c 100644 --- a/src/tag/index.ts +++ b/src/tag/index.ts @@ -13,6 +13,7 @@ import type { PrepareResult } from "../prepare/types"; import type { Octokits } from "../github/api/client"; import { isAutomationContext } from "../github/context"; import { prepareStewardMode } from "../steward"; +import { DroidRunType, resolveTagRunType, setDroidRunType } from "../run-type"; const DROID_APP_BOT_ID = 209825114; const SECURITY_REVIEW_MARKER = "## Security Review Summary"; @@ -81,7 +82,9 @@ export async function prepareTagExecution({ githubToken, }: PrepareTagOptions): Promise { if (isAutomationContext(context)) { - return prepareStewardMode(context, octokit, githubToken); + const runType = DroidRunType.CiSteward; + setDroidRunType(runType); + return prepareStewardMode(context, octokit, githubToken, runType); } if (!isEntityContext(context)) { throw new Error("Tag execution requires entity context"); @@ -100,6 +103,12 @@ export async function prepareTagExecution({ } const commandContext = extractCommandFromContext(context); + const runType = resolveTagRunType({ + automaticReview: context.inputs.automaticReview, + automaticSecurityReview: context.inputs.automaticSecurityReview, + command: commandContext?.command ?? null, + }); + setDroidRunType(runType); // Determine comment type based on what's being run const isDualReview = @@ -109,26 +118,18 @@ export async function prepareTagExecution({ (context.inputs.automaticSecurityReview || commandContext?.command === "security" || commandContext?.command === "security-full"); - const isCodeReview = - context.inputs.automaticReview || - commandContext?.command === "review" || - commandContext?.command === "default" || - !commandContext; const commentType = isDualReview ? "review_and_security" : isSecurityOnly - ? commandContext?.command === "security-full" - ? "security_scan" - : "security" - : isCodeReview - ? "review" - : "default"; + ? "security" + : "default"; const commentData = await createInitialComment( octokit.rest, context, commentType, + runType, ); const commentId = commentData.id; @@ -163,6 +164,7 @@ export async function prepareTagExecution({ octokit, githubToken, trackingCommentId: commentId, + runType, }); } @@ -174,6 +176,7 @@ export async function prepareTagExecution({ octokit, githubToken, trackingCommentId: commentId, + runType, }); } @@ -203,6 +206,7 @@ export async function prepareTagExecution({ octokit, githubToken, trackingCommentId: commentId, + runType, }); } @@ -212,6 +216,7 @@ export async function prepareTagExecution({ octokit, githubToken, trackingCommentId: commentId, + runType, }); } @@ -224,6 +229,7 @@ export async function prepareTagExecution({ octokit, githubToken, trackingCommentId: commentId, + runType, }); } @@ -233,6 +239,7 @@ export async function prepareTagExecution({ octokit, githubToken, scanScope: { type: "full" }, + runType, }); } @@ -249,6 +256,7 @@ export async function prepareTagExecution({ octokit, githubToken, trackingCommentId: commentId, + runType, }); } diff --git a/test/github/comments-common.test.ts b/test/github/comments-common.test.ts index 2b4ff00a..33453685 100644 --- a/test/github/comments-common.test.ts +++ b/test/github/comments-common.test.ts @@ -51,11 +51,13 @@ describe("comments common helpers", () => { const marker = createPrValidationMarker("review"); expect(marker).toBe(""); - expect(createCommentBody(jobLink, "", "review")).toEndWith(marker); - expect(createCommentBody(jobLink, "", "security")).toEndWith(marker); - expect(createCommentBody(jobLink, "", "security_scan")).not.toContain( + expect(createCommentBody(jobLink, "", "default", "review")).toEndWith( marker, ); + expect(createCommentBody(jobLink, "", "security", "review")).toEndWith( + marker, + ); + expect(createCommentBody(jobLink, "", "security")).not.toContain(marker); expect(createCommentBody(jobLink)).not.toContain(marker); }); diff --git a/test/integration/review-flow.test.ts b/test/integration/review-flow.test.ts index c430546e..83fc3a9b 100644 --- a/test/integration/review-flow.test.ts +++ b/test/integration/review-flow.test.ts @@ -11,6 +11,7 @@ import * as promptModule from "../../src/create-prompt"; import * as reviewArtifactsModule from "../../src/github/data/review-artifacts"; import * as core from "@actions/core"; import * as childProcess from "node:child_process"; +import { DroidRunType } from "../../src/run-type"; describe("review command integration", () => { const originalRunnerTemp = process.env.RUNNER_TEMP; @@ -155,6 +156,16 @@ describe("review command integration", () => { expect(result.branchInfo.baseBranch).toBe("main"); expect(result.branchInfo.currentBranch).toBe("feature/review"); expect(promptSpy).toHaveBeenCalled(); + expect(exportVarSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + DroidRunType.Review, + ); + expect(createCommentSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "default", + DroidRunType.Review, + ); // Verify output flags were set correctly for code review only const runCodeReviewCall = setOutputSpy.mock.calls.find( @@ -227,6 +238,16 @@ describe("review command integration", () => { expect(result.branchInfo.baseBranch).toBe("main"); expect(result.branchInfo.currentBranch).toBe("feature/security"); expect(promptSpy).toHaveBeenCalled(); + expect(exportVarSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + DroidRunType.SecurityReview, + ); + expect(createCommentSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "security", + DroidRunType.SecurityReview, + ); const runCodeReviewCall = setOutputSpy.mock.calls.find( (call: unknown[]) => call[0] === "run_code_review", diff --git a/test/mcp/install-mcp-server.test.ts b/test/mcp/install-mcp-server.test.ts index a63d2dfd..2e965c9b 100644 --- a/test/mcp/install-mcp-server.test.ts +++ b/test/mcp/install-mcp-server.test.ts @@ -1,24 +1,25 @@ import { describe, expect, it } from "bun:test"; import { prepareMcpTools } from "../../src/mcp/install-mcp-server"; import { createMockContext } from "../mockContext"; +import { DroidRunType } from "../../src/run-type"; describe("prepareMcpTools", () => { - it("passes the PR validation source to the comment server", async () => { + it("passes the run type to the comment server", async () => { const config = JSON.parse( await prepareMcpTools({ githubToken: "token", owner: "factory", repo: "droid", droidCommentId: "123", - prValidationSource: "review", + runType: DroidRunType.Review, allowedTools: ["github_comment___update_droid_comment"], mode: "tag", context: createMockContext({ isPR: true }), }), ); - expect( - config.mcpServers.github_comment.env.DROID_PR_VALIDATION_SOURCE, - ).toBe("review"); + expect(config.mcpServers.github_comment.env.DROID_EXEC_RUN_TYPE).toBe( + DroidRunType.Review, + ); }); }); diff --git a/test/modes/tag/fill-command.test.ts b/test/modes/tag/fill-command.test.ts index cb13b9eb..47c19a54 100644 --- a/test/modes/tag/fill-command.test.ts +++ b/test/modes/tag/fill-command.test.ts @@ -5,6 +5,7 @@ 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 { DroidRunType } from "../../../src/run-type"; const MOCK_PR_DATA = { title: "Test PR", @@ -98,6 +99,7 @@ describe("prepareFillMode", () => { expect(promptSpy).toHaveBeenCalled(); expect(mcpSpy).toHaveBeenCalledWith( expect.objectContaining({ + runType: DroidRunType.Fill, allowedTools: expect.arrayContaining([ "github_pr___update_pr_description", ]), @@ -112,10 +114,6 @@ describe("prepareFillMode", () => { ) as [string, string] | undefined; expect(droidArgsCall?.[1]).toContain("github_pr___update_pr_description"); expect(droidArgsCall?.[1]).toContain("Execute"); - expect(exportVariableSpy).toHaveBeenCalledWith( - "DROID_EXEC_RUN_TYPE", - "droid-fill", - ); }); it("throws when invoked on non-PR context", async () => { diff --git a/test/modes/tag/review-command.test.ts b/test/modes/tag/review-command.test.ts index a463753c..7ac017c7 100644 --- a/test/modes/tag/review-command.test.ts +++ b/test/modes/tag/review-command.test.ts @@ -8,6 +8,7 @@ import * as mcpInstaller from "../../../src/mcp/install-mcp-server"; import * as comments from "../../../src/github/operations/comments/create-initial"; import * as childProcess from "child_process"; import * as fsPromises from "fs/promises"; +import { DroidRunType } from "../../../src/run-type"; const MOCK_PR_DATA = { title: "PR for review", @@ -156,7 +157,7 @@ describe("prepareReviewMode", () => { expect(promptSpy).toHaveBeenCalled(); expect(mcpSpy).toHaveBeenCalledWith( expect.objectContaining({ - prValidationSource: "review", + runType: DroidRunType.Review, allowedTools: expect.arrayContaining([ "Execute", "github_comment___update_droid_comment", @@ -184,10 +185,6 @@ describe("prepareReviewMode", () => { "github_inline_comment___create_inline_comment", ); expect(droidArgsCall?.[1]).not.toContain("github_pr___submit_review"); - expect(exportVariableSpy).toHaveBeenCalledWith( - "DROID_EXEC_RUN_TYPE", - "droid-review", - ); }); it("creates tracking comment when not provided", async () => { @@ -247,7 +244,8 @@ describe("prepareReviewMode", () => { expect(createInitialSpy).toHaveBeenCalledWith( octokit.rest, context, - "review", + "default", + DroidRunType.Review, ); expect(result.commentId).toBe(777); }); diff --git a/test/modes/tag/security-review-command.test.ts b/test/modes/tag/security-review-command.test.ts index 7c5cc335..13545ed4 100644 --- a/test/modes/tag/security-review-command.test.ts +++ b/test/modes/tag/security-review-command.test.ts @@ -16,6 +16,7 @@ import * as reviewArtifacts from "../../../src/github/data/review-artifacts"; 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"; +import { DroidRunType } from "../../../src/run-type"; const MOCK_PR_DATA = { baseRefName: "main", @@ -147,7 +148,7 @@ describe("prepareSecurityReviewMode", () => { expect(promptSpy).toHaveBeenCalled(); expect(mcpSpy).toHaveBeenCalledWith( expect.objectContaining({ - prValidationSource: "review", + runType: DroidRunType.SecurityReview, allowedTools: expect.arrayContaining([ "Execute", "Task", @@ -169,11 +170,6 @@ describe("prepareSecurityReviewMode", () => { expect(result.branchInfo.baseBranch).toBe("main"); expect(result.branchInfo.currentBranch).toBe("feature/security-review"); expect(result.branchInfo.droidBranch).toBeUndefined(); - - expect(exportVariableSpy).toHaveBeenCalledWith( - "DROID_EXEC_RUN_TYPE", - "droid-security-review", - ); }); it("creates tracking comment when not provided", async () => { @@ -201,6 +197,7 @@ describe("prepareSecurityReviewMode", () => { octokit.rest, context, "security", + DroidRunType.SecurityReview, ); expect(result.commentId).toBe(777); }); diff --git a/test/run-type.test.ts b/test/run-type.test.ts new file mode 100644 index 00000000..9460fbfe --- /dev/null +++ b/test/run-type.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "bun:test"; +import { + assertDroidRunType, + DroidRunType, + parseDroidRunType, + prValidationSourceForRunType, + resolveTagRunType, +} from "../src/run-type"; + +describe("DroidRunType", () => { + it.each([ + [true, true, null, DroidRunType.Review], + [true, false, null, DroidRunType.Review], + [false, true, null, DroidRunType.SecurityReview], + [false, false, "review", DroidRunType.Review], + [false, false, "default", DroidRunType.Review], + [false, false, null, DroidRunType.Review], + [false, false, "security", DroidRunType.SecurityReview], + [false, false, "security-full", DroidRunType.SecurityScan], + [false, false, "fill", DroidRunType.Fill], + ] as const)( + "resolves automaticReview=%s automaticSecurityReview=%s command=%s", + (automaticReview, automaticSecurityReview, command, expected) => { + expect( + resolveTagRunType({ + automaticReview, + automaticSecurityReview, + command, + }), + ).toBe(expected); + }, + ); + + it("parses only known run types", () => { + expect(parseDroidRunType(DroidRunType.Review)).toBe(DroidRunType.Review); + expect(parseDroidRunType("unknown")).toBeUndefined(); + }); + + it("rejects a run type passed to the wrong mode", () => { + expect(() => + assertDroidRunType(DroidRunType.Fill, DroidRunType.Review), + ).toThrow("Expected run type droid-review, received droid-fill"); + }); + + it("maps PR review run types to the review validation source", () => { + expect(prValidationSourceForRunType(DroidRunType.Review)).toBe("review"); + expect(prValidationSourceForRunType(DroidRunType.SecurityReview)).toBe( + "review", + ); + expect(prValidationSourceForRunType(DroidRunType.Fill)).toBeUndefined(); + expect( + prValidationSourceForRunType(DroidRunType.SecurityScan), + ).toBeUndefined(); + expect( + prValidationSourceForRunType(DroidRunType.CiSteward), + ).toBeUndefined(); + }); +}); diff --git a/test/steward.test.ts b/test/steward.test.ts index 7d80ffd5..0198029f 100644 --- a/test/steward.test.ts +++ b/test/steward.test.ts @@ -10,6 +10,7 @@ import { workflowsPassedOnCommit, } from "../src/steward/gate"; import { prepareMcpTools } from "../src/mcp/install-mcp-server"; +import { DroidRunType } from "../src/run-type"; import { STEWARD_ALLOWED_TOOLS, buildActionConfig, @@ -112,6 +113,7 @@ describe("CI Steward MCP wiring", () => { owner: "owner", repo: "repo", droidCommentId: "1", + runType: DroidRunType.CiSteward, allowedTools: STEWARD_ALLOWED_TOOLS, mode: "tag", context: { @@ -151,6 +153,7 @@ describe("CI Steward MCP wiring", () => { owner: "owner", repo: "repo", droidCommentId: "1", + runType: DroidRunType.CiSteward, allowedTools: STEWARD_ALLOWED_TOOLS, mode: "tag", context: { @@ -177,6 +180,7 @@ describe("CI Steward MCP wiring", () => { owner: "owner", repo: "repo", droidCommentId: "1", + runType: DroidRunType.CiSteward, allowedTools: STEWARD_ALLOWED_TOOLS, mode: "tag", context: { From e829e87a0673474ae758ba3fc9ae7c50948f28b3 Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Tue, 25 Aug 2026 17:10:14 -0700 Subject: [PATCH 05/13] fix(review): avoid comments for skipped security runs Check automatic security run-once state before creating the tracking comment. Skip comment creation for security-only no-op runs and downgrade dual-review comments when security will not run. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/tag/index.ts | 66 ++++++------- test/integration/review-flow.test.ts | 138 +++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 33 deletions(-) diff --git a/src/tag/index.ts b/src/tag/index.ts index 335ad25c..141e938b 100644 --- a/src/tag/index.ts +++ b/src/tag/index.ts @@ -110,12 +110,37 @@ export async function prepareTagExecution({ }); setDroidRunType(runType); + let runAutomaticSecurityReview = context.inputs.automaticSecurityReview; + if (runAutomaticSecurityReview) { + const hasExisting = await hasExistingSecurityReview(octokit, context); + if (hasExisting) { + console.log( + "Security review already exists on this PR, skipping security", + ); + runAutomaticSecurityReview = false; + + if (!context.inputs.automaticReview) { + core.setOutput("run_code_review", "false"); + core.setOutput("run_security_review", "false"); + return { + skipped: true, + reason: "security_review_exists", + branchInfo: { + baseBranch: "", + currentBranch: "", + }, + mcpTools: "", + }; + } + } + } + // Determine comment type based on what's being run const isDualReview = - context.inputs.automaticReview && context.inputs.automaticSecurityReview; + context.inputs.automaticReview && runAutomaticSecurityReview; const isSecurityOnly = - !isDualReview && - (context.inputs.automaticSecurityReview || + !context.inputs.automaticReview && + (runAutomaticSecurityReview || commandContext?.command === "security" || commandContext?.command === "security-full"); @@ -138,25 +163,17 @@ export async function prepareTagExecution({ context.inputs.automaticReview && context.inputs.automaticSecurityReview ) { - let runSecurityReview = true; - - // Check if security review already exists on this PR (run once behavior) - const hasExisting = await hasExistingSecurityReview(octokit, context); - if (hasExisting) { - console.log( - "Security review already exists on this PR, skipping security", - ); - runSecurityReview = false; - } - - if (runSecurityReview) { + if (runAutomaticSecurityReview) { // Signal to the code review prompt to spawn a security-reviewer subagent core.exportVariable("SECURITY_REVIEW_ENABLED", "true"); core.setOutput("install_security_skills", "true"); } core.setOutput("run_code_review", "true"); - core.setOutput("run_security_review", runSecurityReview.toString()); + core.setOutput( + "run_security_review", + runAutomaticSecurityReview.toString(), + ); // Prepare the code review (security review runs as a subagent within pass 1) return prepareReviewMode({ @@ -181,23 +198,6 @@ export async function prepareTagExecution({ } if (context.inputs.automaticSecurityReview) { - // Check if security review already exists on this PR (run once behavior) - const hasExisting = await hasExistingSecurityReview(octokit, context); - if (hasExisting) { - console.log("Security review already exists on this PR, skipping"); - core.setOutput("run_code_review", "false"); - core.setOutput("run_security_review", "false"); - return { - skipped: true, - reason: "security_review_exists", - branchInfo: { - baseBranch: "", - currentBranch: "", - }, - mcpTools: "", - }; - } - // Standalone security review uses the two-pass pipeline (candidates + validator) core.setOutput("run_code_review", "true"); core.setOutput("run_security_review", "true"); diff --git a/test/integration/review-flow.test.ts b/test/integration/review-flow.test.ts index 83fc3a9b..9d2281a2 100644 --- a/test/integration/review-flow.test.ts +++ b/test/integration/review-flow.test.ts @@ -27,6 +27,70 @@ describe("review command integration", () => { let computeArtifactsSpy: ReturnType; let execSyncSpy: ReturnType; + function createAutomaticReviewContext(automaticReview: boolean) { + return createMockContext({ + eventName: "issue_comment", + isPR: true, + inputs: { + automaticReview, + automaticSecurityReview: true, + }, + payload: { + comment: { + id: 888, + body: "", + user: { login: "human-reviewer" }, + created_at: "2024-02-02T00:00:00Z", + }, + issue: { + number: 7, + pull_request: {}, + }, + } as any, + }); + } + + function createAutomaticReviewOctokit(hasExistingSecurityReview: boolean) { + const octokit = { + rest: { + issues: { + listComments: () => + Promise.resolve({ + data: hasExistingSecurityReview + ? [ + { + user: { id: 209825114, login: "factory-droid[bot]" }, + body: "## Security Review Summary", + }, + ] + : [], + }), + }, + }, + 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", + headRefOid: "def456", + }, + }, + }); + return octokit; + } + beforeEach(async () => { tmpDir = await mkdtemp(path.join(os.tmpdir(), "review-int-")); process.env.RUNNER_TEMP = tmpDir; @@ -260,4 +324,78 @@ describe("review command integration", () => { expect(runCodeReviewCall?.[1]).toBe("true"); expect(runSecurityReviewCall?.[1]).toBe("true"); }); + + it("does not create a comment when automatic security review is skipped", async () => { + const context = createAutomaticReviewContext(false); + const octokit = createAutomaticReviewOctokit(true); + + const result = await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + expect(result).toEqual({ + skipped: true, + reason: "security_review_exists", + branchInfo: { + baseBranch: "", + currentBranch: "", + }, + mcpTools: "", + }); + expect(createCommentSpy).not.toHaveBeenCalled(); + expect(setOutputSpy).toHaveBeenCalledWith("run_code_review", "false"); + expect(setOutputSpy).toHaveBeenCalledWith("run_security_review", "false"); + }); + + it("creates a review-only comment when automatic security is skipped", async () => { + const context = createAutomaticReviewContext(true); + const octokit = createAutomaticReviewOctokit(true); + + const result = await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + expect(result.skipped).toBeFalsy(); + expect(createCommentSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "default", + DroidRunType.Review, + ); + expect(setOutputSpy).toHaveBeenCalledWith("run_code_review", "true"); + expect(setOutputSpy).toHaveBeenCalledWith("run_security_review", "false"); + expect(exportVarSpy).not.toHaveBeenCalledWith( + "SECURITY_REVIEW_ENABLED", + "true", + ); + }); + + it("creates a combined comment when both automatic reviews will run", async () => { + const context = createAutomaticReviewContext(true); + const octokit = createAutomaticReviewOctokit(false); + + const result = await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + expect(result.skipped).toBeFalsy(); + expect(createCommentSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "review_and_security", + DroidRunType.Review, + ); + expect(setOutputSpy).toHaveBeenCalledWith("run_code_review", "true"); + expect(setOutputSpy).toHaveBeenCalledWith("run_security_review", "true"); + expect(exportVarSpy).toHaveBeenCalledWith( + "SECURITY_REVIEW_ENABLED", + "true", + ); + }); }); From 41549432490c19ef3702ac76cef598e618eafe0f Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Tue, 25 Aug 2026 17:22:11 -0700 Subject: [PATCH 06/13] refactor(review): serialize run types in markers Replace the single-value validation source with exact run-type metadata. Mark code review, security review, and full security scan comments while keeping fill and CI Steward outside the PR validation marker. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/github/operations/comments/common.ts | 23 +++++----- .../operations/comments/create-initial.ts | 7 +-- src/mcp/github-comment-server.ts | 4 +- src/run-type.ts | 15 ++++--- test/comment-logic.test.ts | 3 +- test/github/comments-common.test.ts | 43 +++++++++++++------ test/run-type.test.ts | 24 +++++------ 7 files changed, 70 insertions(+), 49 deletions(-) diff --git a/src/github/operations/comments/common.ts b/src/github/operations/comments/common.ts index 2dad09e1..408e7a72 100644 --- a/src/github/operations/comments/common.ts +++ b/src/github/operations/comments/common.ts @@ -1,10 +1,9 @@ import { GITHUB_SERVER_URL } from "../../api/config"; import { sanitizeContent } from "../../utils/sanitizer"; +import type { PrValidationRunType } from "../../../run-type"; -export type PrValidationSource = "review"; - -export function createPrValidationMarker(source: PrValidationSource): string { - return ``; +export function createPrValidationMarker(runType: PrValidationRunType): string { + return ``; } export function createJobRunLink( @@ -29,9 +28,9 @@ export type CommentType = "default" | "security" | "review_and_security"; export function appendPrValidationMarker( content: string, - source: PrValidationSource, + runType: PrValidationRunType, ): string { - const marker = createPrValidationMarker(source); + const marker = createPrValidationMarker(runType); if (content.includes(marker)) { return content; } @@ -41,11 +40,11 @@ export function appendPrValidationMarker( export function prepareDroidCommentBody( content: string, - prValidationSource?: PrValidationSource, + prValidationRunType?: PrValidationRunType, ): string { const sanitized = sanitizeContent(content); - return prValidationSource - ? appendPrValidationMarker(sanitized, prValidationSource) + return prValidationRunType + ? appendPrValidationMarker(sanitized, prValidationRunType) : sanitized; } @@ -53,7 +52,7 @@ export function createCommentBody( jobRunLink: string, branchLink: string = "", type: CommentType = "default", - prValidationSource?: PrValidationSource, + prValidationRunType?: PrValidationRunType, ): string { let message: string; if (type === "review_and_security") { @@ -68,7 +67,7 @@ export function createCommentBody( ${jobRunLink}${branchLink}`; - return prValidationSource - ? appendPrValidationMarker(body, prValidationSource) + return prValidationRunType + ? appendPrValidationMarker(body, prValidationRunType) : body; } diff --git a/src/github/operations/comments/create-initial.ts b/src/github/operations/comments/create-initial.ts index 0a27f445..a45f0b94 100644 --- a/src/github/operations/comments/create-initial.ts +++ b/src/github/operations/comments/create-initial.ts @@ -17,10 +17,7 @@ import { type ParsedGitHubContext, } from "../../context"; import type { Octokit } from "@octokit/rest"; -import { - prValidationSourceForRunType, - type DroidRunType, -} from "../../../run-type"; +import { getPrValidationRunType, type DroidRunType } from "../../../run-type"; const DROID_APP_BOT_ID = 209825114; @@ -37,7 +34,7 @@ export async function createInitialComment( jobRunLink, "", commentType, - prValidationSourceForRunType(runType), + getPrValidationRunType(runType), ); try { diff --git a/src/mcp/github-comment-server.ts b/src/mcp/github-comment-server.ts index 4982a5ab..faae3852 100644 --- a/src/mcp/github-comment-server.ts +++ b/src/mcp/github-comment-server.ts @@ -7,7 +7,7 @@ import { GITHUB_API_URL } from "../github/api/config"; import { Octokit } from "@octokit/rest"; import { updateDroidComment } from "../github/operations/comments/update-droid-comment"; import { prepareDroidCommentBody } from "../github/operations/comments/common"; -import { parseDroidRunType, prValidationSourceForRunType } from "../run-type"; +import { getPrValidationRunType, parseDroidRunType } from "../run-type"; // Get repository information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -59,7 +59,7 @@ server.tool( const runType = parseDroidRunType(process.env.DROID_EXEC_RUN_TYPE); let sanitizedBody = prepareDroidCommentBody( body, - prValidationSourceForRunType(runType), + getPrValidationRunType(runType), ); // CI Steward keeps its lifetime run budget in a marker on this comment. diff --git a/src/run-type.ts b/src/run-type.ts index 191e1fcb..be72937d 100644 --- a/src/run-type.ts +++ b/src/run-type.ts @@ -1,6 +1,5 @@ import * as core from "@actions/core"; import type { DroidCommand } from "./core/review/triggers/parse-command"; -import type { PrValidationSource } from "./github/operations/comments/common"; export enum DroidRunType { Review = "droid-review", @@ -10,6 +9,11 @@ export enum DroidRunType { CiSteward = "ci-steward", } +export type PrValidationRunType = + | DroidRunType.Review + | DroidRunType.SecurityReview + | DroidRunType.SecurityScan; + export function setDroidRunType(runType: DroidRunType): void { core.exportVariable("DROID_EXEC_RUN_TYPE", runType); } @@ -31,12 +35,13 @@ export function parseDroidRunType( : undefined; } -export function prValidationSourceForRunType( +export function getPrValidationRunType( runType: DroidRunType | undefined, -): PrValidationSource | undefined { +): PrValidationRunType | undefined { return runType === DroidRunType.Review || - runType === DroidRunType.SecurityReview - ? "review" + runType === DroidRunType.SecurityReview || + runType === DroidRunType.SecurityScan + ? runType : undefined; } diff --git a/test/comment-logic.test.ts b/test/comment-logic.test.ts index e14fc0be..23532afa 100644 --- a/test/comment-logic.test.ts +++ b/test/comment-logic.test.ts @@ -4,6 +4,7 @@ import { type CommentUpdateInput, } from "../src/github/operations/comment-logic"; import { createPrValidationMarker } from "../src/github/operations/comments/common"; +import { DroidRunType } from "../src/run-type"; describe("updateCommentBody", () => { const baseInput = { @@ -109,7 +110,7 @@ describe("updateCommentBody", () => { }); it("preserves the hidden PR validation marker in the final summary", () => { - const marker = createPrValidationMarker("review"); + const marker = createPrValidationMarker(DroidRunType.Review); const input = { ...baseInput, currentBody: `Droid is reviewing code…\n\n${marker}`, diff --git a/test/github/comments-common.test.ts b/test/github/comments-common.test.ts index 33453685..10c80e42 100644 --- a/test/github/comments-common.test.ts +++ b/test/github/comments-common.test.ts @@ -8,6 +8,7 @@ import { prepareDroidCommentBody, } from "../../src/github/operations/comments/common"; import { GITHUB_SERVER_URL } from "../../src/github/api/config"; +import { DroidRunType } from "../../src/run-type"; describe("comments common helpers", () => { it("creates a job run link using the configured GitHub server", () => { @@ -46,29 +47,47 @@ describe("comments common helpers", () => { expect(body).not.toContain("View branch"); }); - it("adds the hidden marker to PR review tracking comments", () => { + it("adds the run type marker to PR validation tracking comments", () => { const jobLink = createJobRunLink("factory", "droid", "run-102"); - const marker = createPrValidationMarker("review"); + const reviewMarker = createPrValidationMarker(DroidRunType.Review); + const securityReviewMarker = createPrValidationMarker( + DroidRunType.SecurityReview, + ); + const securityScanMarker = createPrValidationMarker( + DroidRunType.SecurityScan, + ); - expect(marker).toBe(""); - expect(createCommentBody(jobLink, "", "default", "review")).toEndWith( - marker, + expect(reviewMarker).toBe( + "", ); - expect(createCommentBody(jobLink, "", "security", "review")).toEndWith( - marker, + expect(securityReviewMarker).toBe( + "", ); - expect(createCommentBody(jobLink, "", "security")).not.toContain(marker); - expect(createCommentBody(jobLink)).not.toContain(marker); + expect(securityScanMarker).toBe( + "", + ); + expect( + createCommentBody(jobLink, "", "default", DroidRunType.Review), + ).toEndWith(reviewMarker); + expect( + createCommentBody(jobLink, "", "security", DroidRunType.SecurityReview), + ).toEndWith(securityReviewMarker); + expect( + createCommentBody(jobLink, "", "security", DroidRunType.SecurityScan), + ).toEndWith(securityScanMarker); + expect(createCommentBody(jobLink)).not.toContain("factory-pr-validation"); }); it("restores the PR validation marker after sanitizing comment updates", () => { - const marker = createPrValidationMarker("review"); + const marker = createPrValidationMarker(DroidRunType.SecurityReview); const body = prepareDroidCommentBody( `Review complete\n\n${marker}`, - "review", + DroidRunType.SecurityReview, ); expect(body).toBe(`Review complete\n\n${marker}`); - expect(appendPrValidationMarker(body, "review")).toBe(body); + expect(appendPrValidationMarker(body, DroidRunType.SecurityReview)).toBe( + body, + ); }); }); diff --git a/test/run-type.test.ts b/test/run-type.test.ts index 9460fbfe..b55b6d72 100644 --- a/test/run-type.test.ts +++ b/test/run-type.test.ts @@ -2,8 +2,8 @@ import { describe, expect, it } from "bun:test"; import { assertDroidRunType, DroidRunType, + getPrValidationRunType, parseDroidRunType, - prValidationSourceForRunType, resolveTagRunType, } from "../src/run-type"; @@ -42,17 +42,17 @@ describe("DroidRunType", () => { ).toThrow("Expected run type droid-review, received droid-fill"); }); - it("maps PR review run types to the review validation source", () => { - expect(prValidationSourceForRunType(DroidRunType.Review)).toBe("review"); - expect(prValidationSourceForRunType(DroidRunType.SecurityReview)).toBe( - "review", + it("selects the run types that produce PR validation markers", () => { + expect(getPrValidationRunType(DroidRunType.Review)).toBe( + DroidRunType.Review, ); - expect(prValidationSourceForRunType(DroidRunType.Fill)).toBeUndefined(); - expect( - prValidationSourceForRunType(DroidRunType.SecurityScan), - ).toBeUndefined(); - expect( - prValidationSourceForRunType(DroidRunType.CiSteward), - ).toBeUndefined(); + expect(getPrValidationRunType(DroidRunType.SecurityReview)).toBe( + DroidRunType.SecurityReview, + ); + expect(getPrValidationRunType(DroidRunType.SecurityScan)).toBe( + DroidRunType.SecurityScan, + ); + expect(getPrValidationRunType(DroidRunType.Fill)).toBeUndefined(); + expect(getPrValidationRunType(DroidRunType.CiSteward)).toBeUndefined(); }); }); From e5d86bd6608c738ca26607910a3b43eece3e8473 Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Tue, 25 Aug 2026 17:47:27 -0700 Subject: [PATCH 07/13] refactor: distinguish default and unresolved runs Add a dedicated droid-default run type for bare @droid commands and return null when no command or automatic mode identifies a run. Propagate unresolved runs without exporting or marking a false review identity. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- .../operations/comments/create-initial.ts | 2 +- src/mcp/install-mcp-server.ts | 4 +- src/run-type.ts | 24 ++++-- src/tag/commands/fill.ts | 2 +- src/tag/commands/review.ts | 8 +- src/tag/commands/security-review.ts | 2 +- src/tag/commands/security-scan.ts | 2 +- src/tag/index.ts | 4 +- test/github/comments-common.test.ts | 7 ++ test/integration/review-flow.test.ts | 85 +++++++++++++++++++ test/mcp/install-mcp-server.test.ts | 19 +++++ test/run-type.test.ts | 8 +- 12 files changed, 148 insertions(+), 19 deletions(-) diff --git a/src/github/operations/comments/create-initial.ts b/src/github/operations/comments/create-initial.ts index a45f0b94..a86dcb57 100644 --- a/src/github/operations/comments/create-initial.ts +++ b/src/github/operations/comments/create-initial.ts @@ -25,7 +25,7 @@ export async function createInitialComment( octokit: Octokit, context: ParsedGitHubContext, commentType: CommentType = "default", - runType?: DroidRunType, + runType?: DroidRunType | null, ) { const { owner, repo } = context.repository; diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 8d48b286..c2bdc65d 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -12,7 +12,7 @@ type PrepareConfigParams = { owner: string; repo: string; droidCommentId?: string; - runType: DroidRunType; + runType: DroidRunType | null; allowedTools: string[]; mode: "tag"; context: GitHubContext; @@ -111,7 +111,7 @@ export async function prepareMcpTools( REPO_OWNER: owner, REPO_NAME: repo, ...(droidCommentId && { DROID_COMMENT_ID: droidCommentId }), - DROID_EXEC_RUN_TYPE: runType, + ...(runType && { DROID_EXEC_RUN_TYPE: runType }), ...(stewardPrNumber && { STEWARD_PR_NUMBER: stewardPrNumber }), ...(process.env.STEWARD_RUN_ID && { STEWARD_RUN_ID: process.env.STEWARD_RUN_ID, diff --git a/src/run-type.ts b/src/run-type.ts index be72937d..61d95e5b 100644 --- a/src/run-type.ts +++ b/src/run-type.ts @@ -2,6 +2,7 @@ import * as core from "@actions/core"; import type { DroidCommand } from "./core/review/triggers/parse-command"; export enum DroidRunType { + Default = "droid-default", Review = "droid-review", SecurityReview = "droid-security-review", Fill = "droid-fill", @@ -10,6 +11,7 @@ export enum DroidRunType { } export type PrValidationRunType = + | DroidRunType.Default | DroidRunType.Review | DroidRunType.SecurityReview | DroidRunType.SecurityScan; @@ -19,11 +21,14 @@ export function setDroidRunType(runType: DroidRunType): void { } export function assertDroidRunType( - runType: DroidRunType, - expected: DroidRunType, + runType: DroidRunType | null, + expected: DroidRunType | null | readonly (DroidRunType | null)[], ): void { - if (runType !== expected) { - throw new Error(`Expected run type ${expected}, received ${runType}`); + const expectedRunTypes = Array.isArray(expected) ? expected : [expected]; + if (!expectedRunTypes.includes(runType)) { + throw new Error( + `Expected run type ${expectedRunTypes.join(" or ")}, received ${runType}`, + ); } } @@ -36,9 +41,10 @@ export function parseDroidRunType( } export function getPrValidationRunType( - runType: DroidRunType | undefined, + runType: DroidRunType | null | undefined, ): PrValidationRunType | undefined { - return runType === DroidRunType.Review || + return runType === DroidRunType.Default || + runType === DroidRunType.Review || runType === DroidRunType.SecurityReview || runType === DroidRunType.SecurityScan ? runType @@ -53,7 +59,7 @@ export function resolveTagRunType({ automaticReview: boolean; automaticSecurityReview: boolean; command: DroidCommand | null; -}): DroidRunType { +}): DroidRunType | null { if (automaticReview) { return DroidRunType.Review; } @@ -69,8 +75,10 @@ export function resolveTagRunType({ case "security-full": return DroidRunType.SecurityScan; case "review": + return DroidRunType.Review; case "default": + return DroidRunType.Default; case null: - return DroidRunType.Review; + return null; } } diff --git a/src/tag/commands/fill.ts b/src/tag/commands/fill.ts index d939ccda..8fe544f0 100644 --- a/src/tag/commands/fill.ts +++ b/src/tag/commands/fill.ts @@ -17,7 +17,7 @@ type FillCommandOptions = { octokit: Octokits; githubToken: string; trackingCommentId?: number; - runType?: DroidRunType; + runType?: DroidRunType | null; }; export async function prepareFillMode({ diff --git a/src/tag/commands/review.ts b/src/tag/commands/review.ts index 5c6eb8a8..60ea17b4 100644 --- a/src/tag/commands/review.ts +++ b/src/tag/commands/review.ts @@ -21,7 +21,7 @@ type ReviewCommandOptions = { octokit: Octokits; githubToken: string; trackingCommentId?: number; - runType?: DroidRunType; + runType?: DroidRunType | null; }; export async function prepareReviewMode({ @@ -38,7 +38,11 @@ export async function prepareReviewMode({ if (!context.isPR) { throw new Error("Review command is only supported on pull requests"); } - assertDroidRunType(runType, DroidRunType.Review); + assertDroidRunType(runType, [ + DroidRunType.Default, + DroidRunType.Review, + null, + ]); const commentId = trackingCommentId ?? diff --git a/src/tag/commands/security-review.ts b/src/tag/commands/security-review.ts index df1790cb..363e704e 100644 --- a/src/tag/commands/security-review.ts +++ b/src/tag/commands/security-review.ts @@ -20,7 +20,7 @@ type SecurityReviewCommandOptions = { octokit: Octokits; githubToken: string; trackingCommentId?: number; - runType?: DroidRunType; + runType?: DroidRunType | null; }; export async function prepareSecurityReviewMode({ diff --git a/src/tag/commands/security-scan.ts b/src/tag/commands/security-scan.ts index 5d2d1c9f..499343a8 100644 --- a/src/tag/commands/security-scan.ts +++ b/src/tag/commands/security-scan.ts @@ -18,7 +18,7 @@ type SecurityScanCommandOptions = { octokit: Octokits; githubToken: string; scanScope: ScanScope; - runType?: DroidRunType; + runType?: DroidRunType | null; }; export async function prepareSecurityScanMode({ diff --git a/src/tag/index.ts b/src/tag/index.ts index 141e938b..6830bdff 100644 --- a/src/tag/index.ts +++ b/src/tag/index.ts @@ -108,7 +108,9 @@ export async function prepareTagExecution({ automaticSecurityReview: context.inputs.automaticSecurityReview, command: commandContext?.command ?? null, }); - setDroidRunType(runType); + if (runType) { + setDroidRunType(runType); + } let runAutomaticSecurityReview = context.inputs.automaticSecurityReview; if (runAutomaticSecurityReview) { diff --git a/test/github/comments-common.test.ts b/test/github/comments-common.test.ts index 10c80e42..7bd7ab86 100644 --- a/test/github/comments-common.test.ts +++ b/test/github/comments-common.test.ts @@ -49,6 +49,7 @@ describe("comments common helpers", () => { it("adds the run type marker to PR validation tracking comments", () => { const jobLink = createJobRunLink("factory", "droid", "run-102"); + const defaultMarker = createPrValidationMarker(DroidRunType.Default); const reviewMarker = createPrValidationMarker(DroidRunType.Review); const securityReviewMarker = createPrValidationMarker( DroidRunType.SecurityReview, @@ -57,6 +58,9 @@ describe("comments common helpers", () => { DroidRunType.SecurityScan, ); + expect(defaultMarker).toBe( + "", + ); expect(reviewMarker).toBe( "", ); @@ -66,6 +70,9 @@ describe("comments common helpers", () => { expect(securityScanMarker).toBe( "", ); + expect( + createCommentBody(jobLink, "", "default", DroidRunType.Default), + ).toEndWith(defaultMarker); expect( createCommentBody(jobLink, "", "default", DroidRunType.Review), ).toEndWith(reviewMarker); diff --git a/test/integration/review-flow.test.ts b/test/integration/review-flow.test.ts index 9d2281a2..15b173a2 100644 --- a/test/integration/review-flow.test.ts +++ b/test/integration/review-flow.test.ts @@ -243,6 +243,91 @@ describe("review command integration", () => { expect(runSecurityReviewCall?.[1]).toBe("false"); }); + it("uses the default run type for a bare @droid command", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + payload: { + comment: { + id: 888, + body: "@droid", + user: { login: "human-reviewer" }, + created_at: "2024-02-02T00:00:00Z", + }, + issue: { + number: 7, + pull_request: {}, + }, + } as any, + }); + const octokit = createAutomaticReviewOctokit(false); + + const result = await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + expect(result.skipped).toBeFalsy(); + expect(exportVarSpy).toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + DroidRunType.Default, + ); + expect(createCommentSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "default", + DroidRunType.Default, + ); + expect(mcpSpy).toHaveBeenCalledWith( + expect.objectContaining({ runType: DroidRunType.Default }), + ); + }); + + it("keeps the run type null when no command is parsed", async () => { + const context = createMockContext({ + eventName: "issue_comment", + isPR: true, + inputs: { + triggerPhrase: "/droid", + }, + payload: { + comment: { + id: 888, + body: "/droid", + user: { login: "human-reviewer" }, + created_at: "2024-02-02T00:00:00Z", + }, + issue: { + number: 7, + pull_request: {}, + }, + } as any, + }); + const octokit = createAutomaticReviewOctokit(false); + + const result = await prepareTagExecution({ + context, + octokit, + githubToken: "token", + }); + + expect(result.skipped).toBeFalsy(); + expect(exportVarSpy).not.toHaveBeenCalledWith( + "DROID_EXEC_RUN_TYPE", + expect.anything(), + ); + expect(createCommentSpy).toHaveBeenCalledWith( + octokit.rest, + context, + "default", + null, + ); + expect(mcpSpy).toHaveBeenCalledWith( + expect.objectContaining({ runType: null }), + ); + }); + it("sets security flag only for @droid security", async () => { const context = createMockContext({ eventName: "issue_comment", diff --git a/test/mcp/install-mcp-server.test.ts b/test/mcp/install-mcp-server.test.ts index 2e965c9b..a2122379 100644 --- a/test/mcp/install-mcp-server.test.ts +++ b/test/mcp/install-mcp-server.test.ts @@ -22,4 +22,23 @@ describe("prepareMcpTools", () => { DroidRunType.Review, ); }); + + it("omits the run type from the comment server when unresolved", async () => { + const config = JSON.parse( + await prepareMcpTools({ + githubToken: "token", + owner: "factory", + repo: "droid", + droidCommentId: "123", + runType: null, + allowedTools: ["github_comment___update_droid_comment"], + mode: "tag", + context: createMockContext({ isPR: true }), + }), + ); + + expect(config.mcpServers.github_comment.env).not.toHaveProperty( + "DROID_EXEC_RUN_TYPE", + ); + }); }); diff --git a/test/run-type.test.ts b/test/run-type.test.ts index b55b6d72..fbb83519 100644 --- a/test/run-type.test.ts +++ b/test/run-type.test.ts @@ -13,8 +13,8 @@ describe("DroidRunType", () => { [true, false, null, DroidRunType.Review], [false, true, null, DroidRunType.SecurityReview], [false, false, "review", DroidRunType.Review], - [false, false, "default", DroidRunType.Review], - [false, false, null, DroidRunType.Review], + [false, false, "default", DroidRunType.Default], + [false, false, null, null], [false, false, "security", DroidRunType.SecurityReview], [false, false, "security-full", DroidRunType.SecurityScan], [false, false, "fill", DroidRunType.Fill], @@ -32,6 +32,7 @@ describe("DroidRunType", () => { ); it("parses only known run types", () => { + expect(parseDroidRunType(DroidRunType.Default)).toBe(DroidRunType.Default); expect(parseDroidRunType(DroidRunType.Review)).toBe(DroidRunType.Review); expect(parseDroidRunType("unknown")).toBeUndefined(); }); @@ -43,6 +44,9 @@ describe("DroidRunType", () => { }); it("selects the run types that produce PR validation markers", () => { + expect(getPrValidationRunType(DroidRunType.Default)).toBe( + DroidRunType.Default, + ); expect(getPrValidationRunType(DroidRunType.Review)).toBe( DroidRunType.Review, ); From ee580d1741a8e0972838fbb4fb668a49fe7ff589 Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Thu, 3 Sep 2026 14:01:26 -0700 Subject: [PATCH 08/13] feat(review): tag inline comments by run type Distinguish PR issue and inline comment markers so review consumers can identify both the comment surface and originating run type. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/entrypoints/github-post-review.ts | 9 ++ src/entrypoints/prepare-validator.ts | 5 +- src/github/operations/comments/common.ts | 28 ++++- .../operations/comments/create-initial.ts | 28 +++-- src/github/operations/reviews.ts | 17 ++- src/mcp/github-comment-server.ts | 17 +-- src/mcp/github-inline-comment-server.ts | 11 +- src/mcp/github-pr-server.ts | 14 +++ src/mcp/install-mcp-server.ts | 5 + src/run-type.ts | 6 + src/tag/commands/review-validator.ts | 6 +- test/comment-logic.test.ts | 7 +- test/entrypoints/github-post-review.test.ts | 24 ++++ test/github/comments-common.test.ts | 64 ++++++++--- test/github/create-initial-comment.test.ts | 103 ++++++++++++++++++ test/mcp/github-pr-server.test.ts | 30 +++++ test/mcp/install-mcp-server.test.ts | 61 ++++++++++- test/run-type.test.ts | 12 ++ 18 files changed, 397 insertions(+), 50 deletions(-) create mode 100644 test/github/create-initial-comment.test.ts diff --git a/src/entrypoints/github-post-review.ts b/src/entrypoints/github-post-review.ts index fff727ef..5945ab29 100644 --- a/src/entrypoints/github-post-review.ts +++ b/src/entrypoints/github-post-review.ts @@ -32,6 +32,10 @@ import { type GitHubReviewCommentPayload, type GitHubReviewCreateClient, } from "../github/operations/reviews"; +import { + parsePrValidationRunType, + type PrValidationRunType, +} from "../run-type"; export const MAX_GITHUB_REVIEW_COMMENTS = 30; export const MAX_GITHUB_REVIEW_BODY_BYTES = 60 * 1024; @@ -207,6 +211,7 @@ async function createReview( comments: GitHubInlineComment[], fallback: FallbackFinding[], commitId: string | null, + runType?: PrValidationRunType, ): Promise { return createGitHubCommentReview({ client, @@ -215,6 +220,7 @@ async function createReview( prNumber, comments, commitId, + runType, ...(fallback.length > 0 ? { body: fallbackReviewBody(fallback) } : {}), }); } @@ -236,6 +242,7 @@ export async function postGitHubReview(options: { comments: ValidatedReviewComment[]; diff: string; commitId?: string | null; + runType?: PrValidationRunType; }): Promise { const { client, owner, repo, prNumber, comments, diff } = options; const commitId = options.commitId ?? null; @@ -268,6 +275,7 @@ export async function postGitHubReview(options: { prepared.inline, fallback.included, commitId, + options.runType, ); result.posted = prepared.inline.length; result.fallbackPosted = fallback.included.length; @@ -341,6 +349,7 @@ export async function run( comments: parsed.approved, diff, commitId: parsed.commitId, + runType: parsePrValidationRunType(process.env.DROID_EXEC_RUN_TYPE), }); } diff --git a/src/entrypoints/prepare-validator.ts b/src/entrypoints/prepare-validator.ts index 6e15e94f..d6dc9101 100644 --- a/src/entrypoints/prepare-validator.ts +++ b/src/entrypoints/prepare-validator.ts @@ -6,7 +6,7 @@ import { setupGitHubToken } from "../github/token"; import { createOctokit } from "../github/api/client"; import { parseGitHubContext, isEntityContext } from "../github/context"; import { prepareReviewValidatorMode } from "../tag/commands/review-validator"; -import { DroidRunType, setDroidRunType } from "../run-type"; +import { DroidRunType, parseDroidRunType, setDroidRunType } from "../run-type"; async function run() { try { @@ -15,7 +15,8 @@ async function run() { if (!isEntityContext(context) || !context.isPR) { throw new Error("prepare-validator requires a pull request context"); } - const runType = DroidRunType.Review; + const runType = + parseDroidRunType(process.env.DROID_EXEC_RUN_TYPE) ?? DroidRunType.Review; setDroidRunType(runType); // Validate that Pass 1 produced a valid candidates JSON file. diff --git a/src/github/operations/comments/common.ts b/src/github/operations/comments/common.ts index 408e7a72..d8a8362f 100644 --- a/src/github/operations/comments/common.ts +++ b/src/github/operations/comments/common.ts @@ -2,8 +2,21 @@ import { GITHUB_SERVER_URL } from "../../api/config"; import { sanitizeContent } from "../../utils/sanitizer"; import type { PrValidationRunType } from "../../../run-type"; -export function createPrValidationMarker(runType: PrValidationRunType): string { - return ``; +export type PrCommentKind = "issue-comment" | "inline-comment"; + +export function parsePrCommentKind( + value: string | undefined, +): PrCommentKind | undefined { + return value === "issue-comment" || value === "inline-comment" + ? value + : undefined; +} + +export function createPrCommentMarker( + kind: PrCommentKind, + runType: PrValidationRunType, +): string { + return ``; } export function createJobRunLink( @@ -26,11 +39,12 @@ export function createBranchLink( export type CommentType = "default" | "security" | "review_and_security"; -export function appendPrValidationMarker( +export function appendPrCommentMarker( content: string, + kind: PrCommentKind, runType: PrValidationRunType, ): string { - const marker = createPrValidationMarker(runType); + const marker = createPrCommentMarker(kind, runType); if (content.includes(marker)) { return content; } @@ -41,10 +55,11 @@ export function appendPrValidationMarker( export function prepareDroidCommentBody( content: string, prValidationRunType?: PrValidationRunType, + kind: PrCommentKind = "issue-comment", ): string { const sanitized = sanitizeContent(content); return prValidationRunType - ? appendPrValidationMarker(sanitized, prValidationRunType) + ? appendPrCommentMarker(sanitized, kind, prValidationRunType) : sanitized; } @@ -53,6 +68,7 @@ export function createCommentBody( branchLink: string = "", type: CommentType = "default", prValidationRunType?: PrValidationRunType, + kind: PrCommentKind = "issue-comment", ): string { let message: string; if (type === "review_and_security") { @@ -68,6 +84,6 @@ export function createCommentBody( ${jobRunLink}${branchLink}`; return prValidationRunType - ? appendPrValidationMarker(body, prValidationRunType) + ? appendPrCommentMarker(body, kind, prValidationRunType) : body; } diff --git a/src/github/operations/comments/create-initial.ts b/src/github/operations/comments/create-initial.ts index a86dcb57..4d45ed7d 100644 --- a/src/github/operations/comments/create-initial.ts +++ b/src/github/operations/comments/create-initial.ts @@ -10,6 +10,7 @@ import { createJobRunLink, createCommentBody, type CommentType, + type PrCommentKind, } from "./common"; import { isPullRequestReviewCommentEvent, @@ -18,6 +19,7 @@ import { } from "../../context"; import type { Octokit } from "@octokit/rest"; import { getPrValidationRunType, type DroidRunType } from "../../../run-type"; +import * as core from "@actions/core"; const DROID_APP_BOT_ID = 209825114; @@ -30,15 +32,14 @@ export async function createInitialComment( const { owner, repo } = context.repository; const jobRunLink = createJobRunLink(owner, repo, context.runId); - const initialBody = createCommentBody( - jobRunLink, - "", - commentType, - getPrValidationRunType(runType), - ); + const prValidationRunType = getPrValidationRunType(runType); + const createInitialBody = (kind: PrCommentKind) => + createCommentBody(jobRunLink, "", commentType, prValidationRunType, kind); + const issueCommentBody = createInitialBody("issue-comment"); try { let response; + let createdCommentKind: PrCommentKind = "issue-comment"; if ( context.inputs.useStickyComment && @@ -55,7 +56,7 @@ export async function createInitialComment( const botNameMatch = comment.user?.type === "Bot" && comment.user?.login.toLowerCase().includes("droid"); - const bodyMatch = comment.body === initialBody; + const bodyMatch = comment.body === issueCommentBody; return idMatch || botNameMatch || bodyMatch; }); @@ -64,7 +65,7 @@ export async function createInitialComment( owner, repo, comment_id: existingComment.id, - body: initialBody, + body: issueCommentBody, }); } else { // Create new comment if no existing one found @@ -72,7 +73,7 @@ export async function createInitialComment( owner, repo, issue_number: context.entityNumber, - body: initialBody, + body: issueCommentBody, }); } } else if (isPullRequestReviewCommentEvent(context)) { @@ -82,21 +83,23 @@ export async function createInitialComment( repo, pull_number: context.entityNumber, comment_id: context.payload.comment.id, - body: initialBody, + body: createInitialBody("inline-comment"), }); + createdCommentKind = "inline-comment"; } else { // For all other cases (issues, issue comments, or missing comment_id) response = await octokit.rest.issues.createComment({ owner, repo, issue_number: context.entityNumber, - body: initialBody, + body: issueCommentBody, }); } // Output the comment ID for downstream steps using GITHUB_OUTPUT const githubOutput = process.env.GITHUB_OUTPUT!; appendFileSync(githubOutput, `droid_comment_id=${response.data.id}\n`); + core.exportVariable("DROID_PR_COMMENT_KIND", createdCommentKind); console.log(`✅ Created initial comment with ID: ${response.data.id}`); return response.data; } catch (error) { @@ -108,11 +111,12 @@ export async function createInitialComment( owner, repo, issue_number: context.entityNumber, - body: initialBody, + body: issueCommentBody, }); const githubOutput = process.env.GITHUB_OUTPUT!; appendFileSync(githubOutput, `droid_comment_id=${response.data.id}\n`); + core.exportVariable("DROID_PR_COMMENT_KIND", "issue-comment"); console.log(`✅ Created fallback comment with ID: ${response.data.id}`); return response.data; } catch (fallbackError) { diff --git a/src/github/operations/reviews.ts b/src/github/operations/reviews.ts index ebf89f25..7d1d6511 100644 --- a/src/github/operations/reviews.ts +++ b/src/github/operations/reviews.ts @@ -1,3 +1,6 @@ +import { prepareDroidCommentBody } from "./comments/common"; +import type { PrValidationRunType } from "../../run-type"; + export type GitHubReviewCommentPayload = { path: string; body: string; @@ -32,8 +35,16 @@ export async function createGitHubCommentReview(options: { body?: string; comments?: GitHubReviewCommentPayload[]; commitId?: string | null; + runType?: PrValidationRunType; }): Promise { - const { client, owner, repo, prNumber, body, comments, commitId } = options; + const { client, owner, repo, prNumber, body, comments, commitId, runType } = + options; + const preparedComments = runType + ? comments?.map((comment) => ({ + ...comment, + body: prepareDroidCommentBody(comment.body, runType, "inline-comment"), + })) + : comments; const response = await client.rest.pulls.createReview({ owner, repo, @@ -41,7 +52,9 @@ export async function createGitHubCommentReview(options: { event: "COMMENT", ...(commitId ? { commit_id: commitId } : {}), ...(body ? { body } : {}), - ...(comments && comments.length > 0 ? { comments } : {}), + ...(preparedComments && preparedComments.length > 0 + ? { comments: preparedComments } + : {}), }); return response.data.id; } diff --git a/src/mcp/github-comment-server.ts b/src/mcp/github-comment-server.ts index faae3852..0733cf20 100644 --- a/src/mcp/github-comment-server.ts +++ b/src/mcp/github-comment-server.ts @@ -6,8 +6,11 @@ import { z } from "zod"; import { GITHUB_API_URL } from "../github/api/config"; import { Octokit } from "@octokit/rest"; import { updateDroidComment } from "../github/operations/comments/update-droid-comment"; -import { prepareDroidCommentBody } from "../github/operations/comments/common"; -import { getPrValidationRunType, parseDroidRunType } from "../run-type"; +import { + parsePrCommentKind, + prepareDroidCommentBody, +} from "../github/operations/comments/common"; +import { parsePrValidationRunType } from "../run-type"; // Get repository information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -56,11 +59,11 @@ server.tool( const isPullRequestReviewComment = eventName === "pull_request_review_comment"; - const runType = parseDroidRunType(process.env.DROID_EXEC_RUN_TYPE); - let sanitizedBody = prepareDroidCommentBody( - body, - getPrValidationRunType(runType), - ); + const runType = parsePrValidationRunType(process.env.DROID_EXEC_RUN_TYPE); + const commentKind = + parsePrCommentKind(process.env.DROID_PR_COMMENT_KIND) ?? + (isPullRequestReviewComment ? "inline-comment" : "issue-comment"); + let sanitizedBody = prepareDroidCommentBody(body, runType, commentKind); // CI Steward keeps its lifetime run budget in a marker on this comment. // Droid replaces the whole body, so carry the marker forward or every diff --git a/src/mcp/github-inline-comment-server.ts b/src/mcp/github-inline-comment-server.ts index 2a51185c..c6f0a174 100644 --- a/src/mcp/github-inline-comment-server.ts +++ b/src/mcp/github-inline-comment-server.ts @@ -3,7 +3,8 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; import { z } from "zod"; import { createOctokit } from "../github/api/client"; -import { sanitizeContent } from "../github/utils/sanitizer"; +import { prepareDroidCommentBody } from "../github/operations/comments/common"; +import { parsePrValidationRunType } from "../run-type"; // Get repository and PR information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -84,8 +85,12 @@ server.tool( const octokit = createOctokit(githubToken).rest; - // Sanitize the comment body to remove any potential GitHub tokens - const sanitizedBody = sanitizeContent(body); + const runType = parsePrValidationRunType(process.env.DROID_EXEC_RUN_TYPE); + const sanitizedBody = prepareDroidCommentBody( + body, + runType, + "inline-comment", + ); // Validate that either line or both startLine and line are provided if (!line && !startLine) { diff --git a/src/mcp/github-pr-server.ts b/src/mcp/github-pr-server.ts index bf7fe590..52c19eb4 100644 --- a/src/mcp/github-pr-server.ts +++ b/src/mcp/github-pr-server.ts @@ -9,6 +9,10 @@ import { createGitHubCommentReview, type GitHubReviewCommentPayload, } from "../github/operations/reviews"; +import { + parsePrValidationRunType, + type PrValidationRunType, +} from "../run-type"; const PLACEHOLDER_REGEX = /@droid\s+fill(?:\s+description)?/gi; const PLACEHOLDER_LINE_REGEX = @@ -322,6 +326,7 @@ export async function handleSubmitReview({ prNumber, body, comments, + runType, octokit, guard, }: { @@ -330,6 +335,7 @@ export async function handleSubmitReview({ prNumber: number; body?: string; comments?: ReviewComment[]; + runType?: PrValidationRunType; octokit: OctokitLike; guard: SubmitReviewGuard; }): Promise { @@ -358,6 +364,7 @@ export async function handleSubmitReview({ prNumber, body, comments, + runType, octokit, }); } catch (error) { @@ -380,6 +387,7 @@ export async function submitReviewWithComments({ prNumber, body, comments, + runType, octokit, }: { owner: string; @@ -387,6 +395,7 @@ export async function submitReviewWithComments({ prNumber: number; body?: string; comments?: ReviewComment[]; + runType?: PrValidationRunType; octokit: OctokitLike; }): Promise { return createGitHubCommentReview({ @@ -396,6 +405,7 @@ export async function submitReviewWithComments({ prNumber, body, comments, + runType, }); } @@ -846,12 +856,16 @@ export function createGitHubPRServer({ }, async ({ pr_number, body, comments }) => { try { + const runType = parsePrValidationRunType( + process.env.DROID_EXEC_RUN_TYPE, + ); const outcome = await handleSubmitReview({ owner, repo, prNumber: pr_number, body, comments, + runType, octokit, guard: submitReviewGuard, }); diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index c2bdc65d..3695444b 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -5,6 +5,7 @@ import { GITHUB_API_URL, GITHUB_SERVER_URL } from "../github/api/config"; import type { GitHubContext } from "../github/context"; import { isEntityContext } from "../github/context"; import type { DroidRunType } from "../run-type"; +import { parsePrCommentKind } from "../github/operations/comments/common"; import { Octokit } from "@octokit/rest"; type PrepareConfigParams = { @@ -98,6 +99,7 @@ export async function prepareMcpTools( : undefined) || stewardPrNumber || ""; + const prCommentKind = parsePrCommentKind(process.env.DROID_PR_COMMENT_KIND); const baseMcpTools: { mcpServers: Record } = { mcpServers: {}, @@ -112,6 +114,7 @@ export async function prepareMcpTools( REPO_NAME: repo, ...(droidCommentId && { DROID_COMMENT_ID: droidCommentId }), ...(runType && { DROID_EXEC_RUN_TYPE: runType }), + ...(prCommentKind && { DROID_PR_COMMENT_KIND: prCommentKind }), ...(stewardPrNumber && { STEWARD_PR_NUMBER: stewardPrNumber }), ...(process.env.STEWARD_RUN_ID && { STEWARD_RUN_ID: process.env.STEWARD_RUN_ID, @@ -137,6 +140,7 @@ export async function prepareMcpTools( REPO_OWNER: owner, REPO_NAME: repo, PR_NUMBER: pullRequestNumber, + ...(runType && { DROID_EXEC_RUN_TYPE: runType }), GITHUB_API_URL: GITHUB_API_URL, }, }; @@ -183,6 +187,7 @@ export async function prepareMcpTools( REPO_OWNER: owner, REPO_NAME: repo, PR_NUMBER: context.entityNumber?.toString() || "", + ...(runType && { DROID_EXEC_RUN_TYPE: runType }), }, }; } diff --git a/src/run-type.ts b/src/run-type.ts index 61d95e5b..9eb89a8c 100644 --- a/src/run-type.ts +++ b/src/run-type.ts @@ -51,6 +51,12 @@ export function getPrValidationRunType( : undefined; } +export function parsePrValidationRunType( + value: string | undefined, +): PrValidationRunType | undefined { + return getPrValidationRunType(parseDroidRunType(value)); +} + export function resolveTagRunType({ automaticReview, automaticSecurityReview, diff --git a/src/tag/commands/review-validator.ts b/src/tag/commands/review-validator.ts index 546d2334..218ac005 100644 --- a/src/tag/commands/review-validator.ts +++ b/src/tag/commands/review-validator.ts @@ -31,7 +31,11 @@ export async function prepareReviewValidatorMode(options: { if (!isEntityContext(context) || !context.isPR) { throw new Error("review validator mode requires pull request context"); } - assertDroidRunType(runType, DroidRunType.Review); + assertDroidRunType(runType, [ + DroidRunType.Default, + DroidRunType.Review, + DroidRunType.SecurityReview, + ]); const prData = await fetchPRBranchData({ octokits: octokit, diff --git a/test/comment-logic.test.ts b/test/comment-logic.test.ts index 23532afa..00352db6 100644 --- a/test/comment-logic.test.ts +++ b/test/comment-logic.test.ts @@ -3,7 +3,7 @@ import { updateCommentBody, type CommentUpdateInput, } from "../src/github/operations/comment-logic"; -import { createPrValidationMarker } from "../src/github/operations/comments/common"; +import { createPrCommentMarker } from "../src/github/operations/comments/common"; import { DroidRunType } from "../src/run-type"; describe("updateCommentBody", () => { @@ -110,7 +110,10 @@ describe("updateCommentBody", () => { }); it("preserves the hidden PR validation marker in the final summary", () => { - const marker = createPrValidationMarker(DroidRunType.Review); + const marker = createPrCommentMarker( + "issue-comment", + DroidRunType.Review, + ); const input = { ...baseInput, currentBody: `Droid is reviewing code…\n\n${marker}`, diff --git a/test/entrypoints/github-post-review.test.ts b/test/entrypoints/github-post-review.test.ts index 2bd0d94a..d2af6c62 100644 --- a/test/entrypoints/github-post-review.test.ts +++ b/test/entrypoints/github-post-review.test.ts @@ -14,6 +14,7 @@ import { import { buildUnifiedDiffIndex } from "../../src/core/review/validated/diff"; import type { ValidatedReviewComment } from "../../src/core/review/validated/parse"; import { createMockContext } from "../mockContext"; +import { DroidRunType } from "../../src/run-type"; const DIFF = `diff --git a/src/x.ts b/src/x.ts --- a/src/x.ts @@ -155,6 +156,29 @@ describe("postGitHubReview", () => { ); }); + it("tags inline comments with the review run type", async () => { + const createReview = mock(async (_payload: any) => ({ + data: { id: 125 }, + })); + const client = { + rest: { pulls: { createReview } }, + } as GitHubReviewClient; + + await postGitHubReview({ + client, + owner: "o", + repo: "r", + prNumber: 7, + comments: [comment({ body: "[P1] [security] Finding" })], + diff: DIFF, + runType: DroidRunType.SecurityReview, + }); + + expect((createReview.mock.calls[0]![0] as any).comments[0].body).toEndWith( + "", + ); + }); + it("preserves a valid multi-line anchor in the single API call", async () => { const createReview = mock(async (_payload: any) => ({ data: { id: 456 }, diff --git a/test/github/comments-common.test.ts b/test/github/comments-common.test.ts index 7bd7ab86..cfdaa6ab 100644 --- a/test/github/comments-common.test.ts +++ b/test/github/comments-common.test.ts @@ -1,10 +1,11 @@ import { describe, expect, it } from "bun:test"; import { - appendPrValidationMarker, + appendPrCommentMarker, createBranchLink, createCommentBody, createJobRunLink, - createPrValidationMarker, + createPrCommentMarker, + parsePrCommentKind, prepareDroidCommentBody, } from "../../src/github/operations/comments/common"; import { GITHUB_SERVER_URL } from "../../src/github/api/config"; @@ -49,26 +50,34 @@ describe("comments common helpers", () => { it("adds the run type marker to PR validation tracking comments", () => { const jobLink = createJobRunLink("factory", "droid", "run-102"); - const defaultMarker = createPrValidationMarker(DroidRunType.Default); - const reviewMarker = createPrValidationMarker(DroidRunType.Review); - const securityReviewMarker = createPrValidationMarker( + const defaultMarker = createPrCommentMarker( + "issue-comment", + DroidRunType.Default, + ); + const reviewMarker = createPrCommentMarker( + "issue-comment", + DroidRunType.Review, + ); + const securityReviewMarker = createPrCommentMarker( + "issue-comment", DroidRunType.SecurityReview, ); - const securityScanMarker = createPrValidationMarker( + const securityScanMarker = createPrCommentMarker( + "issue-comment", DroidRunType.SecurityScan, ); expect(defaultMarker).toBe( - "", + "", ); expect(reviewMarker).toBe( - "", + "", ); expect(securityReviewMarker).toBe( - "", + "", ); expect(securityScanMarker).toBe( - "", + "", ); expect( createCommentBody(jobLink, "", "default", DroidRunType.Default), @@ -82,19 +91,46 @@ describe("comments common helpers", () => { expect( createCommentBody(jobLink, "", "security", DroidRunType.SecurityScan), ).toEndWith(securityScanMarker); - expect(createCommentBody(jobLink)).not.toContain("factory-pr-validation"); + expect(createCommentBody(jobLink)).not.toContain("factory-pr-"); }); it("restores the PR validation marker after sanitizing comment updates", () => { - const marker = createPrValidationMarker(DroidRunType.SecurityReview); + const marker = createPrCommentMarker( + "issue-comment", + DroidRunType.SecurityReview, + ); const body = prepareDroidCommentBody( `Review complete\n\n${marker}`, DroidRunType.SecurityReview, ); expect(body).toBe(`Review complete\n\n${marker}`); - expect(appendPrValidationMarker(body, DroidRunType.SecurityReview)).toBe( - body, + expect( + appendPrCommentMarker(body, "issue-comment", DroidRunType.SecurityReview), + ).toBe(body); + }); + + it("creates a distinct marker for inline review comments", () => { + const marker = createPrCommentMarker( + "inline-comment", + DroidRunType.SecurityReview, + ); + + expect(marker).toBe( + "", ); + expect( + prepareDroidCommentBody( + `[P1] [security] Finding\n\nDetails\n\n${marker}`, + DroidRunType.SecurityReview, + "inline-comment", + ), + ).toBe(`[P1] [security] Finding\n\nDetails\n\n${marker}`); + }); + + it("parses only known PR comment kinds", () => { + expect(parsePrCommentKind("issue-comment")).toBe("issue-comment"); + expect(parsePrCommentKind("inline-comment")).toBe("inline-comment"); + expect(parsePrCommentKind("review")).toBeUndefined(); }); }); diff --git a/test/github/create-initial-comment.test.ts b/test/github/create-initial-comment.test.ts new file mode 100644 index 00000000..86ac2fa8 --- /dev/null +++ b/test/github/create-initial-comment.test.ts @@ -0,0 +1,103 @@ +import { + afterEach, + beforeEach, + describe, + expect, + it, + mock, + spyOn, +} from "bun:test"; +import * as core from "@actions/core"; +import { mkdtempSync, rmSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; +import { createInitialComment } from "../../src/github/operations/comments/create-initial"; +import { DroidRunType } from "../../src/run-type"; +import { mockPullRequestReviewCommentContext } from "../mockContext"; + +describe("createInitialComment", () => { + const originalGitHubOutput = process.env.GITHUB_OUTPUT; + let tempDir: string; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "droid-initial-comment-")); + process.env.GITHUB_OUTPUT = join(tempDir, "github-output"); + spyOn(core, "exportVariable").mockImplementation(() => {}); + }); + + afterEach(() => { + mock.restore(); + rmSync(tempDir, { recursive: true, force: true }); + if (originalGitHubOutput === undefined) { + delete process.env.GITHUB_OUTPUT; + } else { + process.env.GITHUB_OUTPUT = originalGitHubOutput; + } + }); + + it("records an inline tracking comment when the reply succeeds", async () => { + const replyBodies: string[] = []; + const octokit = { + rest: { + pulls: { + createReplyForReviewComment: async ({ body }: { body: string }) => { + replyBodies.push(body); + return { data: { id: 123 } }; + }, + }, + issues: { + createComment: async () => ({ data: { id: 456 } }), + }, + }, + }; + + await createInitialComment( + octokit as any, + mockPullRequestReviewCommentContext, + "security", + DroidRunType.SecurityReview, + ); + + expect(replyBodies[0]).toContain( + "", + ); + expect(core.exportVariable).toHaveBeenCalledWith( + "DROID_PR_COMMENT_KIND", + "inline-comment", + ); + }); + + it("records an issue tracking comment when the inline reply falls back", async () => { + const issueBodies: string[] = []; + const octokit = { + rest: { + pulls: { + createReplyForReviewComment: async () => { + throw new Error("reply failed"); + }, + }, + issues: { + createComment: async ({ body }: { body: string }) => { + issueBodies.push(body); + return { data: { id: 456 } }; + }, + }, + }, + }; + + await createInitialComment( + octokit as any, + mockPullRequestReviewCommentContext, + "security", + DroidRunType.SecurityReview, + ); + + expect(issueBodies[0]).toContain( + "", + ); + expect(core.exportVariable).toHaveBeenCalledWith( + "DROID_PR_COMMENT_KIND", + "issue-comment", + ); + }); +}); diff --git a/test/mcp/github-pr-server.test.ts b/test/mcp/github-pr-server.test.ts index 6081e7c8..613b09a9 100644 --- a/test/mcp/github-pr-server.test.ts +++ b/test/mcp/github-pr-server.test.ts @@ -13,6 +13,7 @@ import { type OctokitLike, type ReviewComment, } from "../../src/mcp/github-pr-server"; +import { DroidRunType } from "../../src/run-type"; function createOctokitStub() { const calls = { @@ -174,6 +175,35 @@ describe("github-pr-server helpers", () => { expect(reviewId).toBe(9001); }); + it("adds the run type marker to batched inline comments", async () => { + const { client, calls } = createOctokitStub(); + + await submitReviewWithComments({ + owner: "o", + repo: "r", + prNumber: 7, + comments: [ + { + path: "file.ts", + position: 3, + body: "[P1] [security] issue", + }, + ], + runType: DroidRunType.SecurityReview, + octokit: client, + }); + + expect(calls.createReview[0][0].comments).toEqual([ + { + path: "file.ts", + position: 3, + body: + "[P1] [security] issue\n\n" + + "", + }, + ]); + }); + it("deletes issue and review comments", async () => { const { client, calls } = createOctokitStub(); diff --git a/test/mcp/install-mcp-server.test.ts b/test/mcp/install-mcp-server.test.ts index a2122379..1dc72632 100644 --- a/test/mcp/install-mcp-server.test.ts +++ b/test/mcp/install-mcp-server.test.ts @@ -1,9 +1,19 @@ -import { describe, expect, it } from "bun:test"; +import { afterEach, describe, expect, it } from "bun:test"; import { prepareMcpTools } from "../../src/mcp/install-mcp-server"; import { createMockContext } from "../mockContext"; import { DroidRunType } from "../../src/run-type"; describe("prepareMcpTools", () => { + const originalPrCommentKind = process.env.DROID_PR_COMMENT_KIND; + + afterEach(() => { + if (originalPrCommentKind === undefined) { + delete process.env.DROID_PR_COMMENT_KIND; + } else { + process.env.DROID_PR_COMMENT_KIND = originalPrCommentKind; + } + }); + it("passes the run type to the comment server", async () => { const config = JSON.parse( await prepareMcpTools({ @@ -23,6 +33,55 @@ describe("prepareMcpTools", () => { ); }); + it("passes the run type to inline comment servers", async () => { + const config = JSON.parse( + await prepareMcpTools({ + githubToken: "token", + owner: "factory", + repo: "droid", + droidCommentId: "123", + runType: DroidRunType.SecurityReview, + allowedTools: [ + "github_inline_comment___create_inline_comment", + "github_pr___submit_review", + ], + mode: "tag", + context: createMockContext({ isPR: true }), + }), + ); + + expect( + config.mcpServers.github_inline_comment.env.DROID_EXEC_RUN_TYPE, + ).toBe(DroidRunType.SecurityReview); + expect(config.mcpServers.github_pr.env.DROID_EXEC_RUN_TYPE).toBe( + DroidRunType.SecurityReview, + ); + }); + + it("passes the actual tracking comment kind to the comment server", async () => { + process.env.DROID_PR_COMMENT_KIND = "issue-comment"; + + const config = JSON.parse( + await prepareMcpTools({ + githubToken: "token", + owner: "factory", + repo: "droid", + droidCommentId: "123", + runType: DroidRunType.SecurityReview, + allowedTools: ["github_comment___update_droid_comment"], + mode: "tag", + context: createMockContext({ + isPR: true, + eventName: "pull_request_review_comment", + }), + }), + ); + + expect(config.mcpServers.github_comment.env.DROID_PR_COMMENT_KIND).toBe( + "issue-comment", + ); + }); + it("omits the run type from the comment server when unresolved", async () => { const config = JSON.parse( await prepareMcpTools({ diff --git a/test/run-type.test.ts b/test/run-type.test.ts index fbb83519..b1abc996 100644 --- a/test/run-type.test.ts +++ b/test/run-type.test.ts @@ -4,6 +4,7 @@ import { DroidRunType, getPrValidationRunType, parseDroidRunType, + parsePrValidationRunType, resolveTagRunType, } from "../src/run-type"; @@ -59,4 +60,15 @@ describe("DroidRunType", () => { expect(getPrValidationRunType(DroidRunType.Fill)).toBeUndefined(); expect(getPrValidationRunType(DroidRunType.CiSteward)).toBeUndefined(); }); + + it("parses and narrows PR validation run types", () => { + expect(parsePrValidationRunType(DroidRunType.Review)).toBe( + DroidRunType.Review, + ); + expect(parsePrValidationRunType(DroidRunType.SecurityReview)).toBe( + DroidRunType.SecurityReview, + ); + expect(parsePrValidationRunType(DroidRunType.Fill)).toBeUndefined(); + expect(parsePrValidationRunType("unknown")).toBeUndefined(); + }); }); From 9ffc2aacd4044b6aad3f088c3e91a46aaa612606 Mon Sep 17 00:00:00 2001 From: Andrew Seo Date: Thu, 10 Sep 2026 10:12:13 -0700 Subject: [PATCH 09/13] fix(review): preserve combined tracking comment markers Keep shared code/security classification across sticky reuse, progress updates, and final summaries instead of replacing or losing the marker. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- src/entrypoints/update-comment-link.ts | 5 + src/github/operations/comment-logic.ts | 21 ++- src/github/operations/comments/common.ts | 75 +++++++- .../operations/comments/create-initial.ts | 9 +- .../comments/update-tracking-comment.ts | 35 ++++ src/mcp/github-comment-server.ts | 24 +-- test/comment-logic.test.ts | 80 ++++++++- test/github/comments-common.test.ts | 86 +++++++++ test/github/create-initial-comment.test.ts | 79 ++++++++- test/github/update-tracking-comment.test.ts | 166 ++++++++++++++++++ 10 files changed, 560 insertions(+), 20 deletions(-) create mode 100644 src/github/operations/comments/update-tracking-comment.ts create mode 100644 test/github/update-tracking-comment.test.ts diff --git a/src/entrypoints/update-comment-link.ts b/src/entrypoints/update-comment-link.ts index 3b27f681..6a5f30f8 100644 --- a/src/entrypoints/update-comment-link.ts +++ b/src/entrypoints/update-comment-link.ts @@ -16,6 +16,7 @@ import { GITHUB_SERVER_URL } from "../github/api/config"; import { updateDroidComment } from "../github/operations/comments/update-droid-comment"; import { fetchDroidComment } from "../github/operations/comments/fetch-droid-comment"; import { readReviewPostOutcome } from "../core/review/tracking/results"; +import { parsePrValidationRunType } from "../run-type"; export async function readReviewPostResults() { const filePath = @@ -183,6 +184,10 @@ async function run() { notice: process.env.MODEL_FALLBACK_NOTE?.trim() || undefined, securityReviewRan: process.env.AUTOMATIC_SECURITY_REVIEW === "true", review, + prCommentRunType: parsePrValidationRunType( + process.env.DROID_EXEC_RUN_TYPE, + ), + prCommentKind: isPRReviewComment ? "inline-comment" : "issue-comment", }; const updatedBody = updateCommentBody(commentInput); diff --git a/src/github/operations/comment-logic.ts b/src/github/operations/comment-logic.ts index a125d919..5295c5ee 100644 --- a/src/github/operations/comment-logic.ts +++ b/src/github/operations/comment-logic.ts @@ -1,5 +1,11 @@ import { GITHUB_SERVER_URL } from "../api/config"; import type { ReviewPostOutcome } from "../../core/review/tracking/types"; +import { + prepareDroidTrackingCommentBody, + readPrCommentRunType, + type PrCommentKind, + type PrCommentRunType, +} from "./comments/common"; export type ExecutionDetails = { cost_usd?: number; @@ -20,6 +26,8 @@ export type CommentUpdateInput = { notice?: string; securityReviewRan?: boolean; review?: ReviewPostOutcome | null; + prCommentRunType?: PrCommentRunType; + prCommentKind?: PrCommentKind; }; const MODEL_POLICY_ERROR_PATTERN = @@ -303,5 +311,16 @@ export function updateCommentBody(input: CommentUpdateInput): string { // Add the cleaned body content newBody += bodyContent; - return newBody.trim(); + if ( + !input.prCommentRunType && + !readPrCommentRunType(originalBody, input.prCommentKind ?? "issue-comment") + ) { + return newBody.trim(); + } + return prepareDroidTrackingCommentBody( + newBody.trim(), + originalBody, + input.prCommentRunType, + input.prCommentKind, + ); } diff --git a/src/github/operations/comments/common.ts b/src/github/operations/comments/common.ts index d8a8362f..c34780d8 100644 --- a/src/github/operations/comments/common.ts +++ b/src/github/operations/comments/common.ts @@ -1,9 +1,53 @@ import { GITHUB_SERVER_URL } from "../../api/config"; import { sanitizeContent } from "../../utils/sanitizer"; -import type { PrValidationRunType } from "../../../run-type"; +import { + DroidRunType, + parsePrValidationRunType, + type PrValidationRunType, +} from "../../../run-type"; export type PrCommentKind = "issue-comment" | "inline-comment"; +// A shared tracking comment can cover more than one execution. +export const COMBINED_REVIEW_COMMENT_RUN_TYPE = "droid-review-and-security"; +export type PrCommentRunType = + | PrValidationRunType + | typeof COMBINED_REVIEW_COMMENT_RUN_TYPE; + +export function mergePrCommentRunTypes( + existing: PrCommentRunType | undefined, + current: PrCommentRunType | undefined, +): PrCommentRunType | undefined { + if ( + existing === COMBINED_REVIEW_COMMENT_RUN_TYPE || + current === COMBINED_REVIEW_COMMENT_RUN_TYPE + ) { + return COMBINED_REVIEW_COMMENT_RUN_TYPE; + } + const isCodeReview = (value: PrCommentRunType | undefined) => + value === DroidRunType.Review || value === DroidRunType.Default; + if ( + (isCodeReview(existing) && current === DroidRunType.SecurityReview) || + (existing === DroidRunType.SecurityReview && isCodeReview(current)) + ) { + return COMBINED_REVIEW_COMMENT_RUN_TYPE; + } + return current ?? existing; +} + +export function readPrCommentRunType( + body: string, + kind: PrCommentKind, +): PrCommentRunType | undefined { + const match = body.match( + new RegExp(`(?:^|\\n)\\s*$`), + ); + const value = match?.[1]; + return value === COMBINED_REVIEW_COMMENT_RUN_TYPE + ? value + : parsePrValidationRunType(value); +} + export function parsePrCommentKind( value: string | undefined, ): PrCommentKind | undefined { @@ -14,7 +58,7 @@ export function parsePrCommentKind( export function createPrCommentMarker( kind: PrCommentKind, - runType: PrValidationRunType, + runType: PrCommentRunType, ): string { return ``; } @@ -42,7 +86,7 @@ export type CommentType = "default" | "security" | "review_and_security"; export function appendPrCommentMarker( content: string, kind: PrCommentKind, - runType: PrValidationRunType, + runType: PrCommentRunType, ): string { const marker = createPrCommentMarker(kind, runType); if (content.includes(marker)) { @@ -54,7 +98,7 @@ export function appendPrCommentMarker( export function prepareDroidCommentBody( content: string, - prValidationRunType?: PrValidationRunType, + prValidationRunType?: PrCommentRunType, kind: PrCommentKind = "issue-comment", ): string { const sanitized = sanitizeContent(content); @@ -63,6 +107,21 @@ export function prepareDroidCommentBody( : sanitized; } +export function prepareDroidTrackingCommentBody( + content: string, + currentBody: string, + runType?: PrCommentRunType, + kind: PrCommentKind = "issue-comment", +): string { + // Only the stored tracking comment contributes prior classification, never + // markers in the model-provided replacement body. + const mergedRunType = mergePrCommentRunTypes( + readPrCommentRunType(currentBody, kind), + runType, + ); + return prepareDroidCommentBody(content, mergedRunType, kind); +} + export function createCommentBody( jobRunLink: string, branchLink: string = "", @@ -84,6 +143,12 @@ export function createCommentBody( ${jobRunLink}${branchLink}`; return prValidationRunType - ? appendPrCommentMarker(body, kind, prValidationRunType) + ? appendPrCommentMarker( + body, + kind, + type === "review_and_security" + ? COMBINED_REVIEW_COMMENT_RUN_TYPE + : prValidationRunType, + ) : body; } diff --git a/src/github/operations/comments/create-initial.ts b/src/github/operations/comments/create-initial.ts index 4d45ed7d..b6b1b82e 100644 --- a/src/github/operations/comments/create-initial.ts +++ b/src/github/operations/comments/create-initial.ts @@ -9,6 +9,8 @@ import { appendFileSync } from "fs"; import { createJobRunLink, createCommentBody, + prepareDroidTrackingCommentBody, + readPrCommentRunType, type CommentType, type PrCommentKind, } from "./common"; @@ -35,7 +37,7 @@ export async function createInitialComment( const prValidationRunType = getPrValidationRunType(runType); const createInitialBody = (kind: PrCommentKind) => createCommentBody(jobRunLink, "", commentType, prValidationRunType, kind); - const issueCommentBody = createInitialBody("issue-comment"); + let issueCommentBody = createInitialBody("issue-comment"); try { let response; @@ -61,6 +63,11 @@ export async function createInitialComment( return idMatch || botNameMatch || bodyMatch; }); if (existingComment) { + issueCommentBody = prepareDroidTrackingCommentBody( + issueCommentBody, + existingComment.body ?? "", + readPrCommentRunType(issueCommentBody, "issue-comment"), + ); response = await octokit.rest.issues.updateComment({ owner, repo, diff --git a/src/github/operations/comments/update-tracking-comment.ts b/src/github/operations/comments/update-tracking-comment.ts new file mode 100644 index 00000000..f7703699 --- /dev/null +++ b/src/github/operations/comments/update-tracking-comment.ts @@ -0,0 +1,35 @@ +import type { Octokits } from "../../api/client"; +import { fetchDroidComment } from "./fetch-droid-comment"; +import { + prepareDroidTrackingCommentBody, + type PrCommentRunType, +} from "./common"; +import { + updateDroidComment, + type UpdateDroidCommentParams, +} from "./update-droid-comment"; + +export async function updateDroidTrackingComment( + octokit: Octokits, + params: UpdateDroidCommentParams & { runType?: PrCommentRunType }, +) { + // A failed read must not fall through to a write that could erase the + // shared comment's existing classification. + const { comment, isPRReviewComment } = await fetchDroidComment(octokit, { + owner: params.owner, + repo: params.repo, + commentId: params.commentId, + isPullRequestReviewCommentEvent: params.isPullRequestReviewComment, + }); + const kind = isPRReviewComment ? "inline-comment" : "issue-comment"; + return updateDroidComment(octokit.rest, { + ...params, + body: prepareDroidTrackingCommentBody( + params.body, + comment.body ?? "", + params.runType, + kind, + ), + isPullRequestReviewComment: isPRReviewComment, + }); +} diff --git a/src/mcp/github-comment-server.ts b/src/mcp/github-comment-server.ts index 0733cf20..9e713b22 100644 --- a/src/mcp/github-comment-server.ts +++ b/src/mcp/github-comment-server.ts @@ -3,14 +3,14 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; import { z } from "zod"; -import { GITHUB_API_URL } from "../github/api/config"; -import { Octokit } from "@octokit/rest"; +import { createOctokit } from "../github/api/client"; import { updateDroidComment } from "../github/operations/comments/update-droid-comment"; +import { updateDroidTrackingComment } from "../github/operations/comments/update-tracking-comment"; import { parsePrCommentKind, prepareDroidCommentBody, } from "../github/operations/comments/common"; -import { parsePrValidationRunType } from "../run-type"; +import { DroidRunType, parsePrValidationRunType } from "../run-type"; // Get repository information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -51,10 +51,7 @@ server.tool( const repo = REPO_NAME; const commentId = parseInt(droidCommentId, 10); - const octokit = new Octokit({ - auth: githubToken, - baseUrl: GITHUB_API_URL, - }); + const octokit = createOctokit(githubToken); const isPullRequestReviewComment = eventName === "pull_request_review_comment"; @@ -63,7 +60,9 @@ server.tool( const commentKind = parsePrCommentKind(process.env.DROID_PR_COMMENT_KIND) ?? (isPullRequestReviewComment ? "inline-comment" : "issue-comment"); - let sanitizedBody = prepareDroidCommentBody(body, runType, commentKind); + const isSteward = + process.env.DROID_EXEC_RUN_TYPE === DroidRunType.CiSteward; + let sanitizedBody = isSteward ? prepareDroidCommentBody(body) : body; // CI Steward keeps its lifetime run budget in a marker on this comment. // Droid replaces the whole body, so carry the marker forward or every @@ -84,13 +83,16 @@ server.tool( sanitizedBody = `${sanitizedBody}\n\n`; } - const result = await updateDroidComment(octokit, { + const params = { owner, repo, commentId, body: sanitizedBody, - isPullRequestReviewComment, - }); + isPullRequestReviewComment: commentKind === "inline-comment", + }; + const result = isSteward + ? await updateDroidComment(octokit.rest, params) + : await updateDroidTrackingComment(octokit, { ...params, runType }); return { content: [ diff --git a/test/comment-logic.test.ts b/test/comment-logic.test.ts index 00352db6..048e1972 100644 --- a/test/comment-logic.test.ts +++ b/test/comment-logic.test.ts @@ -3,7 +3,10 @@ import { updateCommentBody, type CommentUpdateInput, } from "../src/github/operations/comment-logic"; -import { createPrCommentMarker } from "../src/github/operations/comments/common"; +import { + COMBINED_REVIEW_COMMENT_RUN_TYPE, + createPrCommentMarker, +} from "../src/github/operations/comments/common"; import { DroidRunType } from "../src/run-type"; describe("updateCommentBody", () => { @@ -16,6 +19,81 @@ describe("updateCommentBody", () => { triggerUsername: undefined, }; + it.each([ + DroidRunType.Review, + DroidRunType.SecurityReview, + COMBINED_REVIEW_COMMENT_RUN_TYPE, + ] as const)( + "preserves the %s marker through deterministic summaries", + (runType) => { + const marker = createPrCommentMarker("issue-comment", runType); + const result = updateCommentBody({ + ...baseInput, + currentBody: `Droid is working…\n\n${marker}`, + review: { summaryBody: "Final review summary", posted: 2 }, + }); + expect(result).toContain("Final review summary"); + expect(result).toContain("2 inline comments posted"); + expect(result).toEndWith(marker); + expect(result.match(/", + ); + }); + + it.each([ + [DroidRunType.Review, DroidRunType.SecurityReview], + [DroidRunType.SecurityReview, DroidRunType.Review], + [DroidRunType.Default, DroidRunType.SecurityReview], + [DroidRunType.SecurityReview, DroidRunType.Default], + [COMBINED_REVIEW_COMMENT_RUN_TYPE, DroidRunType.Review], + [COMBINED_REVIEW_COMMENT_RUN_TYPE, DroidRunType.SecurityReview], + ] as const)("merges stored %s with current %s", (previous, current) => { + const body = prepareDroidTrackingCommentBody( + "New progress", + createPrCommentMarker("issue-comment", previous), + current, + ); + expect(body).toBe( + "New progress\n\n", + ); + }); + + it("ignores classification supplied in replacement content", () => { + const body = prepareDroidTrackingCommentBody( + `Model output\n\n${createPrCommentMarker("issue-comment", COMBINED_REVIEW_COMMENT_RUN_TYPE)}`, + createPrCommentMarker("issue-comment", DroidRunType.Review), + DroidRunType.Review, + ); + expect(body).toBe( + "Model output\n\n", + ); + }); + + it("reads only supported markers for the requested comment surface", () => { + expect( + readPrCommentRunType( + "", + "issue-comment", + ), + ).toBeUndefined(); + expect( + readPrCommentRunType( + createPrCommentMarker("inline-comment", DroidRunType.SecurityReview), + "issue-comment", + ), + ).toBeUndefined(); + expect( + readPrCommentRunType( + [ + createPrCommentMarker("issue-comment", DroidRunType.SecurityReview), + createPrCommentMarker("issue-comment", DroidRunType.Review), + ].join("\n"), + "issue-comment", + ), + ).toBe(DroidRunType.Review); + expect( + readPrCommentRunType( + `\`\`\`html\n${createPrCommentMarker("issue-comment", DroidRunType.SecurityReview)}\n\`\`\``, + "issue-comment", + ), + ).toBeUndefined(); + }); + + it("does not combine inline findings using model-provided markers", () => { + expect( + prepareDroidCommentBody( + `Finding\n\n${createPrCommentMarker("inline-comment", DroidRunType.SecurityReview)}`, + DroidRunType.Review, + "inline-comment", + ), + ).toBe( + "Finding\n\n", + ); + }); }); diff --git a/test/github/create-initial-comment.test.ts b/test/github/create-initial-comment.test.ts index 86ac2fa8..d070719b 100644 --- a/test/github/create-initial-comment.test.ts +++ b/test/github/create-initial-comment.test.ts @@ -13,7 +13,14 @@ import { tmpdir } from "os"; import { join } from "path"; import { createInitialComment } from "../../src/github/operations/comments/create-initial"; import { DroidRunType } from "../../src/run-type"; -import { mockPullRequestReviewCommentContext } from "../mockContext"; +import { + createMockContext, + mockPullRequestReviewCommentContext, +} from "../mockContext"; +import { + COMBINED_REVIEW_COMMENT_RUN_TYPE, + createPrCommentMarker, +} from "../../src/github/operations/comments/common"; describe("createInitialComment", () => { const originalGitHubOutput = process.env.GITHUB_OUTPUT; @@ -100,4 +107,74 @@ describe("createInitialComment", () => { "issue-comment", ); }); + + it.each([ + [DroidRunType.Review, DroidRunType.SecurityReview], + [DroidRunType.SecurityReview, DroidRunType.Review], + [COMBINED_REVIEW_COMMENT_RUN_TYPE, DroidRunType.SecurityReview], + [COMBINED_REVIEW_COMMENT_RUN_TYPE, DroidRunType.Review], + ] as const)( + "preserves both reviews when sticky %s is reused by %s", + async (previous, current) => { + const updateComment = mock(async (_params: unknown) => ({ + data: { id: 123 }, + })); + const createComment = mock(async () => ({ data: { id: 456 } })); + const octokit = { + rest: { + issues: { + listComments: async () => ({ + data: [ + { + id: 123, + user: { id: 209825114 }, + body: createPrCommentMarker("issue-comment", previous), + }, + ], + }), + updateComment, + createComment, + }, + }, + }; + await createInitialComment( + octokit as any, + createMockContext({ + eventName: "pull_request", + isPR: true, + inputs: { useStickyComment: true }, + }), + current === DroidRunType.SecurityReview ? "security" : "default", + current, + ); + expect(updateComment).toHaveBeenCalledWith( + expect.objectContaining({ + comment_id: 123, + body: expect.stringContaining( + "", + ), + }), + ); + expect(createComment).not.toHaveBeenCalled(); + }, + ); + + it("creates a combined marker before either review updates the comment", async () => { + const createComment = mock(async (_params: unknown) => ({ + data: { id: 123 }, + })); + await createInitialComment( + { rest: { issues: { createComment } } } as any, + createMockContext({ eventName: "pull_request", isPR: true }), + "review_and_security", + DroidRunType.Review, + ); + expect(createComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining( + "", + ), + }), + ); + }); }); diff --git a/test/github/update-tracking-comment.test.ts b/test/github/update-tracking-comment.test.ts new file mode 100644 index 00000000..0079c990 --- /dev/null +++ b/test/github/update-tracking-comment.test.ts @@ -0,0 +1,166 @@ +import { describe, expect, it, mock } from "bun:test"; +import { updateDroidTrackingComment } from "../../src/github/operations/comments/update-tracking-comment"; +import { + COMBINED_REVIEW_COMMENT_RUN_TYPE, + createPrCommentMarker, + type PrCommentKind, +} from "../../src/github/operations/comments/common"; +import { DroidRunType } from "../../src/run-type"; + +const combinedMarker = createPrCommentMarker( + "issue-comment", + COMBINED_REVIEW_COMMENT_RUN_TYPE, +); +const params = { + owner: "o", + repo: "r", + commentId: 123, + isPullRequestReviewComment: false, +}; + +function storedComment(body: string, kind: PrCommentKind = "issue-comment") { + let currentBody = body; + const read = mock(async () => ({ data: { body: currentBody } })); + const missing = mock(async () => { + throw Object.assign(new Error("Not found"), { status: 404 }); + }); + const write = mock(async (input: { body: string; comment_id: number }) => { + expect(input.comment_id).toBe(123); + currentBody = input.body; + return { + data: { + id: 123, + html_url: "https://github.com/o/r/pull/1", + updated_at: "", + }, + }; + }); + const issues = { + getComment: kind === "issue-comment" ? read : missing, + updateComment: write, + }; + const pulls = { + getReviewComment: kind === "inline-comment" ? read : missing, + updateReviewComment: write, + }; + return { + client: { rest: { issues, pulls, rest: { issues, pulls } } } as any, + read, + write, + body: () => currentBody, + }; +} + +describe("updateDroidTrackingComment", () => { + it.each([ + [DroidRunType.Review, DroidRunType.SecurityReview], + [DroidRunType.SecurityReview, DroidRunType.Review], + ] as const)( + "combines %s and %s updates on the same comment", + async (first, second) => { + const stored = storedComment( + createPrCommentMarker("issue-comment", first), + ); + for (const runType of [second, first, second]) { + await updateDroidTrackingComment(stored.client, { + ...params, + body: `Progress from ${runType}`, + runType, + }); + expect(stored.body()).toEndWith(combinedMarker); + expect( + stored.body().match(/