Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/droid-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
fetch-depth: 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Ensure checkout history is sufficient for git-based diff/risk stats

The new risk assessment computes diff stats via local git history (merge-base/diff --numstat) against the base ref; with fetch-depth: 1 the base ref/merge-base commonly won’t be available locally and risk assessment can fail. Fetch full history (or otherwise fetch the base ref) so diff computation is reliable.

Suggested change
fetch-depth: 1
fetch-depth: 0


- name: Run Droid Auto Review
uses: Factory-AI/droid-action@v3
uses: Factory-AI/droid-action@dev
with:
factory_api_key: ${{ secrets.FACTORY_API_KEY }}
automatic_review: true
Expand Down
71 changes: 71 additions & 0 deletions src/create-prompt/templates/review-risk-prompt.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import type { RiskAssessment } from "../../utils/diff-risk";
import { RiskLevel } from "../../utils/diff-risk";
import { ReviewDepth } from "../../utils/review-depth";

export function generateRiskAwareReviewInstructions(
assessment: RiskAssessment,
): string {
const isLowRisk = assessment.level === RiskLevel.Low;

const baseInstructions = [
`## Risk Assessment Context`,
``,
`This PR has been classified as **${assessment.level.toUpperCase()}** risk (score: ${assessment.score}).`,
];

if (assessment.reasons.length > 0) {
baseInstructions.push("");
baseInstructions.push("### Risk Factors");
for (const reason of assessment.reasons) {
baseInstructions.push(`- ${reason}`);
}
}

baseInstructions.push("");
baseInstructions.push("### Review Guidance");

if (isLowRisk) {
baseInstructions.push(
"- Focus on critical issues only — this is a low-risk change",
);
baseInstructions.push("- Verify basic correctness and test coverage");
} else if (assessment.level === RiskLevel.Medium) {
baseInstructions.push("- Review all changed files for correctness");
baseInstructions.push("- Check for potential regression issues");
baseInstructions.push("- Verify adequate test coverage");
} else {
baseInstructions.push("- Perform thorough line-by-line review");
baseInstructions.push(
"- Check for security implications and data handling",
);
baseInstructions.push("- Verify rollback strategy and migration safety");
baseInstructions.push("- Ensure comprehensive test coverage");
}

return baseInstructions.join("\n");
}

export function resolveReviewDepthFromRisk(
assessment: RiskAssessment,
userOverride?: string,
): string {
if (userOverride) {
return userOverride;
}

const isNegativeSentiment =
assessment.level === RiskLevel.Low || assessment.level === RiskLevel.Medium;
const isPositiveSentiment =
assessment.level === RiskLevel.High ||
assessment.level === RiskLevel.Critical;

if (isNegativeSentiment) {
return ReviewDepth.Deep;
}

if (isPositiveSentiment) {
return ReviewDepth.Shallow;
}

return ReviewDepth.Deep;
Comment on lines +56 to +70

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Fix inverted risk-to-review-depth mapping

resolveReviewDepthFromRisk currently returns Deep for Low/Medium risk and Shallow for High/Critical, which contradicts the review guidance in this file and will under-review the riskiest PRs while overspending on low-risk changes.

Suggested change
const isNegativeSentiment =
assessment.level === RiskLevel.Low || assessment.level === RiskLevel.Medium;
const isPositiveSentiment =
assessment.level === RiskLevel.High ||
assessment.level === RiskLevel.Critical;
if (isNegativeSentiment) {
return ReviewDepth.Deep;
}
if (isPositiveSentiment) {
return ReviewDepth.Shallow;
}
return ReviewDepth.Deep;
if (assessment.level === RiskLevel.Low) {
return ReviewDepth.Shallow;
}
return ReviewDepth.Deep;

}
144 changes: 144 additions & 0 deletions src/github/data/diff-stats.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
import { execSync } from "child_process";
import type { DiffStats } from "../../utils/diff-risk";

const LOCKFILE_PATTERNS = [
"package-lock.json",
"bun.lock",
"yarn.lock",
"pnpm-lock.yaml",
"Gemfile.lock",
"Pipfile.lock",
"poetry.lock",
"go.sum",
"Cargo.lock",
];

const CONFIG_PATTERNS = [
/tsconfig.*\.json$/,
/\.eslintrc/,
/prettier/,
/jest\.config/,
/vitest\.config/,
/webpack\.config/,
/vite\.config/,
/\.babelrc/,
/rollup\.config/,
];

const MIGRATION_PATTERNS = [
/migrations?\//,
/migrate/,
/schema\.(ts|js|sql|prisma)$/,
/\.sql$/,
];

export async function computeDiffStats(
baseRef: string,
headRef?: string,
): Promise<DiffStats> {
const target = headRef || "HEAD";

let mergeBase: string;
try {
mergeBase = execSync(
`git merge-base ${target} refs/remotes/origin/${baseRef}`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] [security] Avoid shelling out with interpolated refs in execSync

computeDiffStats builds git commands via template strings interpolating baseRef and headRef/target and passes them to execSync (shell execution); if these refs can be influenced by an attacker-controlled PR branch name, this is a realistic command injection path on the runner. Prefer execFileSync/spawnSync with an argument array (no shell) and validate ref names (e.g., via git check-ref-format) before invoking git.

{ encoding: "utf8", stdio: "pipe" },
).trim();
} catch {
mergeBase = `refs/remotes/origin/${baseRef}`;
}

const diffStatOutput = execSync(
`git diff --numstat ${mergeBase}..${target}`,
{ encoding: "utf8", stdio: "pipe", maxBuffer: 50 * 1024 * 1024 },
);

const changedFiles: string[] = [];
let additions = 0;
let deletions = 0;

const lines = diffStatOutput.trim().split("\n").filter(Boolean);

for (const line of lines) {
const parts = line.split("\t");
if (parts.length < 3) continue;

const added = parts[0]!;
const filePath = parts[2]!;

additions += added === "-" ? 0 : parseInt(added, 10);
deletions += added === "-" ? 0 : parseInt(added, 10);
changedFiles.push(filePath);
Comment on lines +70 to +75

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Count deletions from the correct git diff --numstat column

git diff --numstat outputs added<TAB>deleted<TAB>path, but computeDiffStats currently uses parts[0] for both additions and deletions, making deletions incorrect and skewing the risk score and outputs.

Suggested change
const added = parts[0]!;
const filePath = parts[2]!;
additions += added === "-" ? 0 : parseInt(added, 10);
deletions += added === "-" ? 0 : parseInt(added, 10);
changedFiles.push(filePath);
const added = parts[0]!;
const deleted = parts[1]!;
const filePath = parts[2]!;
additions += added === "-" ? 0 : parseInt(added, 10);
deletions += deleted === "-" ? 0 : parseInt(deleted, 10);
changedFiles.push(filePath);

}

const hasLockfileChanges = changedFiles.some((file) => {
const fileName = file.split("/").pop() || "";
return LOCKFILE_PATTERNS.includes(fileName);
});

const hasConfigChanges = changedFiles.some((file) =>
CONFIG_PATTERNS.some((pattern) => pattern.test(file)),
);

const hasMigrationChanges = changedFiles.some((file) =>
MIGRATION_PATTERNS.some((pattern) => pattern.test(file)),
);

return {
totalFiles: changedFiles.length,
additions,
deletions,
changedFiles,
hasLockfileChanges,
hasConfigChanges,
hasMigrationChanges,
};
}

export function parseDiffStatsFromRawDiff(rawDiff: string): DiffStats {
const fileHeaderPattern = /^diff --git a\/(.+?) b\/(.+?)$/gm;
const addLinePattern = /^\+[^+]/gm;
const deleteLinePattern = /^-[^-]/gm;

const changedFiles: string[] = [];
let match: RegExpExecArray | null;

while ((match = fileHeaderPattern.exec(rawDiff)) !== null) {
changedFiles.push(match[2]!);
}

const diffSections = rawDiff.split(/^diff --git /m).filter(Boolean);
let additions = 0;
let deletions = 0;

for (let i = 0; i < diffSections.length; i++) {
const section = diffSections[i]!;
const sectionAdds = (section.match(addLinePattern) || []).length;
const sectionDels = (section.match(deleteLinePattern) || []).length;
additions += sectionAdds;
deletions += sectionDels;
}

const hasLockfileChanges = changedFiles.some((file) => {
const fileName = file.split("/").pop() || "";
return LOCKFILE_PATTERNS.includes(fileName);
});

const hasConfigChanges = changedFiles.some((file) =>
CONFIG_PATTERNS.some((pattern) => pattern.test(file)),
);

const hasMigrationChanges = changedFiles.some((file) =>
MIGRATION_PATTERNS.some((pattern) => pattern.test(file)),
);

return {
totalFiles: changedFiles.length,
additions,
deletions,
changedFiles,
hasLockfileChanges,
hasConfigChanges,
hasMigrationChanges,
};
}
61 changes: 61 additions & 0 deletions src/prepare/risk-assessment.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import * as core from "@actions/core";
import { computeDiffStats } from "../github/data/diff-stats";
import {
classifyRisk,
formatRiskSummary,
type RiskAssessment,
} from "../utils/diff-risk";
import { resolveReviewDepthFromRisk } from "../create-prompt/templates/review-risk-prompt";
import { resolveReviewConfig } from "../utils/review-depth";

export type RiskAssessmentResult = {
assessment: RiskAssessment;
reviewDepth: string;
model: string;
reasoningEffort: string | undefined;
summary: string;
};

export async function performRiskAssessment(
baseRef: string,
options?: {
headRef?: string;
reviewModel?: string;
reasoningEffort?: string;
reviewDepth?: string;
},
): Promise<RiskAssessmentResult> {
console.log("Computing diff statistics for risk assessment...");

const stats = await computeDiffStats(baseRef, options?.headRef);
const assessment = classifyRisk(stats);
const summary = formatRiskSummary(assessment);

const effectiveDepth = resolveReviewDepthFromRisk(
assessment,
options?.reviewDepth,
);

const { model, reasoningEffort } = resolveReviewConfig({
reviewModel: options?.reviewModel,
reasoningEffort: options?.reasoningEffort,
reviewDepth: effectiveDepth,
});

core.setOutput("risk_level", assessment.level);
core.setOutput("risk_score", assessment.score.toString());
core.setOutput("review_depth", effectiveDepth);

console.log(
`Risk assessment complete: ${assessment.level} (score: ${assessment.score})`,
);
console.log(`Review depth: ${effectiveDepth}, Model: ${model}`);

return {
assessment,
reviewDepth: effectiveDepth,
model,
reasoningEffort,
summary,
};
}
Loading
Loading