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) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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";
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string> {
|
||||
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<string, PrThreadVerdict>();
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user