feat(pr): U18 — surface + gate auto-resolution of PR review comments
Adds autoResolveReviewComments project setting (default on) gating the existing Review-response loop, a single-sourced summarizePrThreadActivity counter, and fixed/acted thread counts in the dashboard PR summary. Resolution stays independent of the auto-merge gate (disabled merge still resolves threads).
This commit is contained in:
@@ -5,8 +5,19 @@ import {
|
||||
isPrEntityActionable,
|
||||
isPrEntityActive,
|
||||
isPrEntityAutoMergeReady,
|
||||
summarizePrThreadActivity,
|
||||
} from "../pr-entity.js";
|
||||
import type { PrEntity } from "../types.js";
|
||||
import type { PrEntity, PrThreadState } from "../types.js";
|
||||
|
||||
function thread(outcome: PrThreadState["outcome"], threadId = "th"): PrThreadState {
|
||||
return {
|
||||
prEntityId: "PR-1",
|
||||
threadId,
|
||||
headOid: "deadbeef",
|
||||
outcome,
|
||||
updatedAt: 1,
|
||||
};
|
||||
}
|
||||
|
||||
function entity(overrides: Partial<PrEntity> = {}): PrEntity {
|
||||
return {
|
||||
@@ -88,3 +99,32 @@ describe("PR entity predicates", () => {
|
||||
expect(autoMergeGateReason({ ...ready, mergeable: "unknown" })).toBe("Waiting for checks");
|
||||
});
|
||||
});
|
||||
|
||||
describe("summarizePrThreadActivity (U18, R15)", () => {
|
||||
it("counts fixed vs disagreed vs pending and derives acted/total", () => {
|
||||
const activity = summarizePrThreadActivity([
|
||||
thread("fixed", "a"),
|
||||
thread("fixed", "b"),
|
||||
thread("disagreed", "c"),
|
||||
thread("pending", "d"),
|
||||
]);
|
||||
expect(activity).toEqual({ total: 4, acted: 3, fixed: 2, disagreed: 1, pending: 1 });
|
||||
});
|
||||
|
||||
it("empty input returns zeroed counts, not nulls", () => {
|
||||
expect(summarizePrThreadActivity([])).toEqual({
|
||||
total: 0,
|
||||
acted: 0,
|
||||
fixed: 0,
|
||||
disagreed: 0,
|
||||
pending: 0,
|
||||
});
|
||||
});
|
||||
|
||||
it("acted excludes pending (in-flight, not yet GitHub-confirmed)", () => {
|
||||
const activity = summarizePrThreadActivity([thread("pending"), thread("pending", "x")]);
|
||||
expect(activity.acted).toBe(0);
|
||||
expect(activity.total).toBe(2);
|
||||
expect(activity.pending).toBe(2);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
// and the reconcile all consult one definition and cannot drift — the same
|
||||
// discipline that put isBranchGroupMemberLanded in branch-group-completion.ts.
|
||||
|
||||
import type { PrEntity } from "./types.js";
|
||||
import type { PrEntity, PrThreadState } from "./types.js";
|
||||
|
||||
/** Non-terminal lifecycle states — the entity is "live". */
|
||||
export function isPrEntityActive(entity: Pick<PrEntity, "state">): boolean {
|
||||
@@ -64,6 +64,50 @@ export function isPrEntityAutoMergeReady(
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Aggregate Review-response-loop activity for a single PR entity (U18, R15).
|
||||
*
|
||||
* A lightweight, dependency-free read seam so the Command Center / Mission
|
||||
* Control can surface what the Review-response loop actually did — threads acted
|
||||
* on, and the fixed-vs-disagreed split — without each surface re-deriving the
|
||||
* counts from raw `PrThreadState[]` (and silently disagreeing with one another).
|
||||
*
|
||||
* `acted` = fixed + disagreed (threads the loop reached a terminal verdict on).
|
||||
* `pending` rows are in-flight (recorded before GitHub confirmed) and are NOT
|
||||
* counted as acted-on. The same discipline that put `isPrEntityAutoMergeReady`
|
||||
* in @fusion/core keeps this single-sourced.
|
||||
*/
|
||||
export interface PrThreadActivity {
|
||||
/** Total threads with a recorded outcome (fixed + disagreed + pending). */
|
||||
total: number;
|
||||
/** Threads the loop reached a terminal verdict on (fixed + disagreed). */
|
||||
acted: number;
|
||||
/** Threads fixed (a change was pushed and the thread replied/resolved). */
|
||||
fixed: number;
|
||||
/** Threads the loop disagreed on (reasoning posted, thread left open). */
|
||||
disagreed: number;
|
||||
/** Threads recorded but not yet GitHub-confirmed (in-flight). */
|
||||
pending: number;
|
||||
}
|
||||
|
||||
export function summarizePrThreadActivity(threads: PrThreadState[]): PrThreadActivity {
|
||||
let fixed = 0;
|
||||
let disagreed = 0;
|
||||
let pending = 0;
|
||||
for (const t of threads) {
|
||||
if (t.outcome === "fixed") fixed += 1;
|
||||
else if (t.outcome === "disagreed") disagreed += 1;
|
||||
else if (t.outcome === "pending") pending += 1;
|
||||
}
|
||||
return {
|
||||
total: threads.length,
|
||||
acted: fixed + disagreed,
|
||||
fixed,
|
||||
disagreed,
|
||||
pending,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* The live auto-merge gate reason shown next to the toggle (R11). Mirrors the
|
||||
* auto-merge-ready predicate ordering so every surface (the dashboard route and
|
||||
|
||||
@@ -252,6 +252,9 @@ export const DEFAULT_PROJECT_SETTINGS = {
|
||||
groupOverlappingFiles: true,
|
||||
overlapIgnorePaths: [],
|
||||
autoMerge: true,
|
||||
// U18 (R15): the Review-response loop is default-on. Independent of `autoMerge` —
|
||||
// with this on but auto-merge off, review threads are resolved but the PR is not merged.
|
||||
autoResolveReviewComments: true,
|
||||
testMode: undefined,
|
||||
mergeRequestContractShadowEnabled: false,
|
||||
mergeStrategy: "direct",
|
||||
|
||||
@@ -3382,6 +3382,13 @@ export interface ProjectSettings {
|
||||
* be enforced server-side. Only applies when `mergeStrategy === "pull-request"`.
|
||||
* Default: false. */
|
||||
requirePrApproval?: boolean;
|
||||
/** When true (default), the Review-response loop automatically acts on PR review
|
||||
* threads (human + bot): it dispatches an agent that fixes + pushes + replies, or
|
||||
* disagrees with reasoning. When false, the loop is inert — review threads are left
|
||||
* untouched for a human to handle. Independent of `autoMerge`: with auto-resolution
|
||||
* on but auto-merge off, threads are still resolved but the PR is NOT merged (the
|
||||
* human checkpoint remains merge). U18, R15. Default: true. */
|
||||
autoResolveReviewComments?: boolean;
|
||||
/** Direct-merge commit routing mode.
|
||||
* - "auto": squash single-substantive branches, preserve history for multi-substantive branches
|
||||
* - "always-squash": always use the legacy squash path for direct merges
|
||||
|
||||
@@ -75,6 +75,7 @@ describe("pull request routes", () => {
|
||||
threads = [
|
||||
{ prEntityId: "PR-1", threadId: "T1", headOid: "abc", outcome: "pending", updatedAt: Date.now() },
|
||||
{ prEntityId: "PR-1", threadId: "T2", headOid: "abc", outcome: "disagreed", updatedAt: Date.now() },
|
||||
{ prEntityId: "PR-1", threadId: "T3", headOid: "abc", outcome: "fixed", updatedAt: Date.now() },
|
||||
];
|
||||
});
|
||||
|
||||
@@ -85,12 +86,15 @@ describe("pull request routes", () => {
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.pullRequests).toHaveLength(1);
|
||||
const pr = res.body.pullRequests[0];
|
||||
expect(pr.threads).toHaveLength(2);
|
||||
expect(pr.threads).toHaveLength(3);
|
||||
expect(pr.summary.checksRollup).toBe("success");
|
||||
expect(pr.summary.mergeable).toBe("clean");
|
||||
expect(pr.summary.conflicting).toBe(false);
|
||||
expect(pr.summary.pendingThreads).toBe(1);
|
||||
expect(pr.summary.disagreedThreads).toBe(1);
|
||||
// U18 (R15): Review-response activity exposed for the Command Center.
|
||||
expect(pr.summary.fixedThreads).toBe(1);
|
||||
expect(pr.summary.actedThreads).toBe(2); // fixed + disagreed, excludes pending
|
||||
});
|
||||
|
||||
it("GET list filters by repo and status", async () => {
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
isPrEntityActionable,
|
||||
isPrEntityAutoMergeReady,
|
||||
autoMergeGateReason,
|
||||
summarizePrThreadActivity,
|
||||
} from "@fusion/core";
|
||||
import { badRequest, notFound, ApiError } from "../api-error.js";
|
||||
|
||||
@@ -82,8 +83,9 @@ export function isBackwardMoveBlockedByOpenPr(input: {
|
||||
* Pure derivation from authoritative entity state.
|
||||
*/
|
||||
export function buildPrSummary(entity: PrEntity, threads: PrThreadState[]) {
|
||||
const pendingThreads = threads.filter((t) => t.outcome === "pending").length;
|
||||
const disagreedThreads = threads.filter((t) => t.outcome === "disagreed").length;
|
||||
// U18 (R15): single-source the Review-response activity counts from @fusion/core
|
||||
// so the dashboard, the CLI, and the Command Center never derive divergent numbers.
|
||||
const activity = summarizePrThreadActivity(threads);
|
||||
return {
|
||||
mergeable: entity.mergeable ?? "unknown",
|
||||
reviewDecision: entity.reviewDecision ?? null,
|
||||
@@ -94,8 +96,12 @@ export function buildPrSummary(entity: PrEntity, threads: PrThreadState[]) {
|
||||
autoMergeReady: isPrEntityAutoMergeReady(entity),
|
||||
actionable: isPrEntityActionable(entity),
|
||||
active: isPrEntityActive(entity),
|
||||
pendingThreads,
|
||||
disagreedThreads,
|
||||
pendingThreads: activity.pending,
|
||||
disagreedThreads: activity.disagreed,
|
||||
// U18: threads the loop fixed, and the total it acted on (fixed + disagreed),
|
||||
// exposed so the Command Center / Mission Control can read resolution activity.
|
||||
fixedThreads: activity.fixed,
|
||||
actedThreads: activity.acted,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -18,11 +18,14 @@ import { TaskStore } from "@fusion/core";
|
||||
import type { TaskDetail, WorkflowIrNode } from "@fusion/core";
|
||||
|
||||
import {
|
||||
buildRespondCallback,
|
||||
createPrNodeHandlers,
|
||||
type PrMergeCallResult,
|
||||
type PrNodeDeps,
|
||||
type PrRespondGithubOps,
|
||||
type PrSourceDescriptor,
|
||||
} from "../pr-nodes.js";
|
||||
import type { PrEntity } from "@fusion/core";
|
||||
import { createDefaultNodeHandlers, createNoopLegacySeams } from "../workflow-node-handlers.js";
|
||||
import type { WorkflowNodeExecutionContext } from "../workflow-graph-executor.js";
|
||||
|
||||
@@ -242,3 +245,112 @@ describe("PR node handlers (U3)", () => {
|
||||
expect(result).toEqual({ outcome: "success", value: "open" });
|
||||
});
|
||||
});
|
||||
|
||||
// ── U18 (R15): the autoResolveReviewComments setting gates the loop ────────────
|
||||
// buildRespondCallback reads settings.autoResolveReviewComments. When false the
|
||||
// loop is inert: it dispatches no agent, fetches no threads, pushes nothing, and
|
||||
// replies to no thread — review threads are left for a human. Default (true /
|
||||
// undefined) preserves today's always-on behavior. This is INDEPENDENT of the
|
||||
// auto-merge gate (a separate graph node), so disabling auto-merge does not turn
|
||||
// off resolution and enabling resolution does not force a merge.
|
||||
describe("Review-response auto-resolution setting gate (U18)", () => {
|
||||
let rootDir: string;
|
||||
let store: TaskStore;
|
||||
let entity: PrEntity;
|
||||
|
||||
beforeEach(async () => {
|
||||
rootDir = makeTmpDir();
|
||||
store = new TaskStore(rootDir, join(rootDir, ".fusion-global"));
|
||||
await store.init();
|
||||
entity = store.ensurePrEntityForSource({ ...SOURCE, state: "open", prNumber: 9 });
|
||||
entity = store.updatePrEntity(entity.id, { headOid: "head-1", unverified: false });
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
store.close();
|
||||
await rm(rootDir, { recursive: true, force: true, maxRetries: 5, retryDelay: 50 });
|
||||
});
|
||||
|
||||
function respondOps(over: Partial<PrRespondGithubOps> = {}): {
|
||||
ops: PrRespondGithubOps;
|
||||
calls: { getReviewThreads: number; replies: number; resolves: number };
|
||||
} {
|
||||
const calls = { getReviewThreads: 0, replies: 0, resolves: 0 };
|
||||
const ops: PrRespondGithubOps = {
|
||||
// Return NO actionable threads. The enabled-path tests only need to prove the
|
||||
// gate let the loop through (getReviewThreads ran); with no actionable thread
|
||||
// the run returns early WITHOUT dispatching the mutating agent — which keeps
|
||||
// these unit tests off the real-AI-CLI path. A disabled loop never even gets
|
||||
// here (it short-circuits before fetching threads).
|
||||
getReviewThreads: async () => {
|
||||
calls.getReviewThreads += 1;
|
||||
return [];
|
||||
},
|
||||
getViewerLogin: async () => "fusion-bot",
|
||||
checkPrStillOpen: async () => ({ open: true, headOid: "head-1" }),
|
||||
replyToThread: async () => {
|
||||
calls.replies += 1;
|
||||
},
|
||||
resolveThread: async () => {
|
||||
calls.resolves += 1;
|
||||
},
|
||||
getCwd: () => rootDir,
|
||||
getTaskId: () => "T-1",
|
||||
...over,
|
||||
};
|
||||
return { ops, calls };
|
||||
}
|
||||
|
||||
it("disabled → loop is inert: no thread fetch, no reply, returns disagreed-only", async () => {
|
||||
await store.updateSettings({ autoResolveReviewComments: false });
|
||||
const { ops, calls } = respondOps();
|
||||
const audited: string[] = [];
|
||||
const respond = buildRespondCallback(() => store, ops, (reason) => audited.push(reason));
|
||||
|
||||
const result = await respond({
|
||||
task: { id: "T-1" } as unknown as TaskDetail,
|
||||
node: { id: "r", kind: "pr-respond" } as WorkflowIrNode,
|
||||
entity,
|
||||
context: {},
|
||||
});
|
||||
|
||||
expect(result).toEqual({ value: "disagreed-only" });
|
||||
// Inert: the loop never even fetched threads, never replied, never resolved.
|
||||
expect(calls.getReviewThreads).toBe(0);
|
||||
expect(calls.replies).toBe(0);
|
||||
expect(calls.resolves).toBe(0);
|
||||
expect(audited).toContain("pr-respond-auto-resolve-disabled");
|
||||
});
|
||||
|
||||
it("default (setting unset) → loop runs: fetches threads (always-on preserved)", async () => {
|
||||
// Do NOT touch the setting; the default is true.
|
||||
const { ops, calls } = respondOps();
|
||||
const respond = buildRespondCallback(() => store, ops);
|
||||
|
||||
await respond({
|
||||
task: { id: "T-1" } as unknown as TaskDetail,
|
||||
node: { id: "r", kind: "pr-respond" } as WorkflowIrNode,
|
||||
entity,
|
||||
context: {},
|
||||
});
|
||||
|
||||
// The loop proceeded far enough to fetch review threads — it is NOT inert.
|
||||
expect(calls.getReviewThreads).toBe(1);
|
||||
});
|
||||
|
||||
it("explicitly enabled → loop runs (independent of auto-merge being off)", async () => {
|
||||
await store.updateSettings({ autoResolveReviewComments: true, autoMerge: false });
|
||||
const { ops, calls } = respondOps();
|
||||
const respond = buildRespondCallback(() => store, ops);
|
||||
|
||||
await respond({
|
||||
task: { id: "T-1" } as unknown as TaskDetail,
|
||||
node: { id: "r", kind: "pr-respond" } as WorkflowIrNode,
|
||||
entity,
|
||||
context: {},
|
||||
});
|
||||
|
||||
// Resolution ran even though auto-merge is off — the two gates are independent.
|
||||
expect(calls.getReviewThreads).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -222,6 +222,23 @@ export function buildRespondCallback(
|
||||
// agent runner + git ops need its settings + worktree. Resolve at run time.
|
||||
const fullStore = store as unknown as import("@fusion/core").TaskStore;
|
||||
const settings = await fullStore.getSettings();
|
||||
|
||||
// U18 (R15): auto-resolution of review comments is a first-class, configurable,
|
||||
// default-ON capability. When disabled, the loop is inert — it dispatches no
|
||||
// agent, pushes nothing, and replies to no thread; review threads are left for a
|
||||
// human. This is INDEPENDENT of the auto-merge gate (a separate graph node): with
|
||||
// resolution on but auto-merge off, threads are still resolved but the PR is not
|
||||
// merged. Default true preserves today's always-on behavior. `disagreed-only` is
|
||||
// the benign routing value (loops back to await-review like the U3 inert default),
|
||||
// so a disabled loop never advances the PR on its own.
|
||||
if (settings.autoResolveReviewComments === false) {
|
||||
audit?.(
|
||||
"pr-respond-auto-resolve-disabled",
|
||||
`entity ${entity.id}: autoResolveReviewComments off; leaving review threads for a human`,
|
||||
);
|
||||
return { value: "disagreed-only" };
|
||||
}
|
||||
|
||||
const taskId = ops.getTaskId(entity);
|
||||
const cwd = ops.getCwd(entity);
|
||||
const runAgent = makePrResponseAgentRunner(settings, taskId, cwd);
|
||||
|
||||
Reference in New Issue
Block a user