From e617ce65d9763ffa37bc0f8804e85a66a82b2940 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 15 Jun 2026 19:44:17 -0700 Subject: [PATCH] =?UTF-8?q?feat(pr):=20U18=20=E2=80=94=20surface=20+=20gat?= =?UTF-8?q?e=20auto-resolution=20of=20PR=20review=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- packages/core/src/__tests__/pr-entity.test.ts | 42 ++++++- packages/core/src/pr-entity.ts | 46 ++++++- packages/core/src/settings-schema.ts | 3 + packages/core/src/types.ts | 7 ++ .../__tests__/routes-pull-requests.test.ts | 6 +- .../routes/register-pull-requests-routes.ts | 14 ++- .../engine/src/__tests__/pr-nodes.test.ts | 112 ++++++++++++++++++ packages/engine/src/pr-nodes.ts | 17 +++ 8 files changed, 240 insertions(+), 7 deletions(-) diff --git a/packages/core/src/__tests__/pr-entity.test.ts b/packages/core/src/__tests__/pr-entity.test.ts index 11d2c4b512..76fc53d452 100644 --- a/packages/core/src/__tests__/pr-entity.test.ts +++ b/packages/core/src/__tests__/pr-entity.test.ts @@ -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 { 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); + }); +}); diff --git a/packages/core/src/pr-entity.ts b/packages/core/src/pr-entity.ts index 39f071a1da..09ac6b7164 100644 --- a/packages/core/src/pr-entity.ts +++ b/packages/core/src/pr-entity.ts @@ -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): 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 diff --git a/packages/core/src/settings-schema.ts b/packages/core/src/settings-schema.ts index 3aefafdfd7..5ba21535e9 100644 --- a/packages/core/src/settings-schema.ts +++ b/packages/core/src/settings-schema.ts @@ -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", diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 517e96c4e3..f6564b3343 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -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 diff --git a/packages/dashboard/src/__tests__/routes-pull-requests.test.ts b/packages/dashboard/src/__tests__/routes-pull-requests.test.ts index 12d4b8e947..77d9f8bd6d 100644 --- a/packages/dashboard/src/__tests__/routes-pull-requests.test.ts +++ b/packages/dashboard/src/__tests__/routes-pull-requests.test.ts @@ -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 () => { diff --git a/packages/dashboard/src/routes/register-pull-requests-routes.ts b/packages/dashboard/src/routes/register-pull-requests-routes.ts index 294168c3a8..c94f3fecbb 100644 --- a/packages/dashboard/src/routes/register-pull-requests-routes.ts +++ b/packages/dashboard/src/routes/register-pull-requests-routes.ts @@ -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, }; } diff --git a/packages/engine/src/__tests__/pr-nodes.test.ts b/packages/engine/src/__tests__/pr-nodes.test.ts index d5b6d1cc59..cb765474c1 100644 --- a/packages/engine/src/__tests__/pr-nodes.test.ts +++ b/packages/engine/src/__tests__/pr-nodes.test.ts @@ -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 = {}): { + 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); + }); +}); diff --git a/packages/engine/src/pr-nodes.ts b/packages/engine/src/pr-nodes.ts index 40cbedb213..ad54d8e593 100644 --- a/packages/engine/src/pr-nodes.ts +++ b/packages/engine/src/pr-nodes.ts @@ -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);