From f1e9ab558761e72df80d2b64a04829953c635826 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 5 Jun 2026 22:49:39 -0700 Subject: [PATCH] fix(review): apply autofix feedback Security hardening + cleanups from code review: parseAgentVerdicts now ignores out-of-batch thread ids and fails safe (disagree) on conflicting duplicate verdicts for one thread; drop the unused store param from makePrResponseAgentRunner; correct the rework-bound doc (to-node, not from-node) and the migration idempotency comment (PK is the re-run guard). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/db.ts | 6 ++++-- packages/core/src/workflow-ir-types.ts | 5 +++-- packages/engine/src/pr-nodes.ts | 2 +- packages/engine/src/pr-response-run-ops.ts | 19 +++++++++++++++++-- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/packages/core/src/db.ts b/packages/core/src/db.ts index 8286ebfd92..99299221cf 100644 --- a/packages/core/src/db.ts +++ b/packages/core/src/db.ts @@ -4363,8 +4363,10 @@ export class Database { const now = Date.now(); // Copy legacy branch-group PRs (only groups that claim an open/merged PR) // into entities. INSERT OR IGNORE makes the copy idempotent across a - // re-run after a partial migration: rows that already landed collide on - // the open-source / open-branch / number indexes and are skipped. + // re-run after a partial migration: the deterministic PRIMARY KEY + // ('pr-bg-' || bg.id) collides for any row that already landed and is + // skipped (terminal-state rows are excluded from the open-* partial + // indexes, so the PK — not those indexes — is the re-run guard). this.db .prepare( `INSERT OR IGNORE INTO pull_requests diff --git a/packages/core/src/workflow-ir-types.ts b/packages/core/src/workflow-ir-types.ts index 720c89cf70..d1c7b7f495 100644 --- a/packages/core/src/workflow-ir-types.ts +++ b/packages/core/src/workflow-ir-types.ts @@ -54,8 +54,9 @@ export interface WorkflowIrEdge { * by the foreach `maxReworkCycles`; U6 generalizes the same mechanism to the * top-level walk so a PR review region (await-review → pr-respond → back to * await-review) is a legal bounded cycle too. The bound on a top-level rework - * edge is `maxReworkCycles` on this edge's `from` node config (the loop head), - * defaulting to {@link DEFAULT_MAX_REWORK_CYCLES}. Either way, rework edges are + * edge is `maxReworkCycles` on this edge's `to` node config (the loop-region + * head, which must set `reworkRegion: true`), defaulting to + * {@link DEFAULT_MAX_REWORK_CYCLES}. Either way, rework edges are * exempt from "Cycle detected"; every other back-edge still throws. */ kind?: "rework"; } diff --git a/packages/engine/src/pr-nodes.ts b/packages/engine/src/pr-nodes.ts index c82c735a30..d1a9c8f0e0 100644 --- a/packages/engine/src/pr-nodes.ts +++ b/packages/engine/src/pr-nodes.ts @@ -208,7 +208,7 @@ export function buildRespondCallback( const settings = await fullStore.getSettings(); const taskId = ops.getTaskId(entity); const cwd = ops.getCwd(entity); - const runAgent = makePrResponseAgentRunner(fullStore, settings, taskId, cwd); + const runAgent = makePrResponseAgentRunner(settings, taskId, cwd); const result = await runPrResponseRun({ entity, diff --git a/packages/engine/src/pr-response-run-ops.ts b/packages/engine/src/pr-response-run-ops.ts index de2c7988d7..fcab87b013 100644 --- a/packages/engine/src/pr-response-run-ops.ts +++ b/packages/engine/src/pr-response-run-ops.ts @@ -9,7 +9,7 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; -import type { PrEntity, Settings, TaskStore } from "@fusion/core"; +import type { PrEntity, Settings } from "@fusion/core"; import { resolveAgentPrompt } from "@fusion/core"; import { createResolvedAgentSession, resolveMergerSessionModel } from "./agent-session-helpers.js"; import { promptWithFallback } from "./pi.js"; @@ -43,12 +43,28 @@ async function git(args: string[], cwd: string): Promise { const VERDICT_LINE_RE = /^PR_THREAD:\s*(\S+)\s+(fix|disagree)\b\s*(.*)$/i; export function parseAgentVerdicts(text: string, threadIds: string[]): PrThreadVerdict[] { + const dispatched = new Set(threadIds); const byThread = new Map(); for (const line of (text ?? "").split(/\r?\n/)) { const m = VERDICT_LINE_RE.exec(line.trim()); if (!m) continue; const [, threadId, decisionRaw, reply] = m; + // Security: only honor verdicts for threads we actually dispatched. An + // out-of-batch thread id is either model confusion or an injected/echoed + // forgery from untrusted comment text — ignore it. + if (!dispatched.has(threadId)) continue; const decision = decisionRaw.toLowerCase() === "fix" ? "fix" : "disagree"; + const prior = byThread.get(threadId); + // Conflicting duplicate verdicts for the same thread fail safe to disagree + // (never auto-resolve a thread on an ambiguous signal). + if (prior && prior.decision !== decision) { + byThread.set(threadId, { + threadId, + decision: "disagree", + reply: "Conflicting verdicts emitted for this thread; leaving it for human review.", + }); + continue; + } byThread.set(threadId, { threadId, decision, reply: reply.trim() || "(no reasoning provided)" }); } // Fail-safe default for any thread the agent did not emit a verdict for. @@ -67,7 +83,6 @@ export function parseAgentVerdicts(text: string, threadIds: string[]): PrThreadV /** Build the engine-owned mutating agent runner for the response run. */ export function makePrResponseAgentRunner( - store: TaskStore, settings: Settings, taskId: string, cwd: string,