feat(engine): harden review pipeline with strict scope, build retry, and E2E tests
Improve the plan→review→approve→merge agent pipeline: - Harden verdict extraction with JSON block parsing and anchored regexes - Consolidate legacy/new merger conflict APIs into thin deprecated wrappers - Add configurable strict scope enforcement (strictScopeEnforcement setting) - Add build retry with timeout to merger (buildRetryCount, buildTimeoutMs) - Add handleChangesRequested to PrCommentHandler for review feedback loop - Remove dead code: handleFsChange, processTaskChange, unused imports/fields - Add E2E multi-verdict sequence tests for the full review pipeline - Fix unused parameter warnings across engine and core packages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -317,7 +317,7 @@ export class Database {
|
||||
/** Tracks transaction nesting depth for savepoint-based nested transactions. */
|
||||
private transactionDepth = 0;
|
||||
|
||||
constructor(private kbDir: string) {
|
||||
constructor(kbDir: string) {
|
||||
this.dbPath = join(kbDir, "fusion.db");
|
||||
|
||||
// Ensure .fusion directory exists
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
import { EventEmitter } from "node:events";
|
||||
import { execSync } from "node:child_process";
|
||||
import { appendFile, mkdir, readFile, writeFile, readdir, rename, unlink } from "node:fs/promises";
|
||||
import { join, sep } from "node:path";
|
||||
import { existsSync, watch, type FSWatcher, readFileSync } from "node:fs";
|
||||
import { appendFile, mkdir, readFile, writeFile, rename, unlink } from "node:fs/promises";
|
||||
import { join } from "node:path";
|
||||
import { existsSync, watch, type FSWatcher } from "node:fs";
|
||||
import type { Task, TaskDetail, TaskCreateInput, TaskAttachment, AgentLogEntry, BoardConfig, Column, MergeResult, Settings, GlobalSettings, ProjectSettings, ActivityLogEntry, ActivityEventType } from "./types.js";
|
||||
import { VALID_TRANSITIONS, DEFAULT_SETTINGS, DEFAULT_GLOBAL_SETTINGS, DEFAULT_PROJECT_SETTINGS, GLOBAL_SETTINGS_KEYS } from "./types.js";
|
||||
import { VALID_TRANSITIONS, DEFAULT_SETTINGS, GLOBAL_SETTINGS_KEYS } from "./types.js";
|
||||
import { GlobalSettingsStore } from "./global-settings.js";
|
||||
import { Database, toJson, toJsonNullable, fromJson } from "./db.js";
|
||||
import { detectLegacyData, migrateFromLegacy } from "./db-migrate.js";
|
||||
@@ -62,8 +62,6 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
private kbDir: string;
|
||||
private tasksDir: string;
|
||||
private configPath: string;
|
||||
private archiveLogPath: string;
|
||||
private activityLogPath: string;
|
||||
/** SQLite database for structured data storage */
|
||||
private _db: Database | null = null;
|
||||
|
||||
@@ -101,8 +99,6 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
this.kbDir = join(rootDir, ".fusion");
|
||||
this.tasksDir = join(this.kbDir, "tasks");
|
||||
this.configPath = join(this.kbDir, "config.json");
|
||||
this.archiveLogPath = join(this.kbDir, "archive.jsonl");
|
||||
this.activityLogPath = join(this.kbDir, "activity-log.jsonl");
|
||||
this.globalSettingsStore = new GlobalSettingsStore(globalSettingsDir);
|
||||
}
|
||||
|
||||
@@ -1004,7 +1000,6 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
task.dependencies = updates.dependencies;
|
||||
|
||||
if (hasNewDeps && task.column === "todo") {
|
||||
const fromColumn = task.column;
|
||||
task.column = "triage";
|
||||
task.status = undefined;
|
||||
task.columnMovedAt = new Date().toISOString();
|
||||
@@ -1371,7 +1366,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
});
|
||||
}
|
||||
|
||||
private collectMergeDetails(id: string, branch: string, task: Task, commitMessage: string): import("./types.js").MergeDetails {
|
||||
private collectMergeDetails(_id: string, _branch: string, task: Task, commitMessage: string): import("./types.js").MergeDetails {
|
||||
const mergedAt = new Date().toISOString();
|
||||
let commitSha: string | undefined;
|
||||
let filesChanged: number | undefined;
|
||||
@@ -1757,7 +1752,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
|
||||
// Use a sentinel watcher object so existing code that checks `this.watcher` still works
|
||||
try {
|
||||
this.watcher = watch(this.tasksDir, { recursive: true }, (_event, filename) => {
|
||||
this.watcher = watch(this.tasksDir, { recursive: true }, (_event, _filename) => {
|
||||
// No-op - we use polling now, but keep watcher for API compat
|
||||
});
|
||||
this.watcher.on("error", () => {
|
||||
@@ -1849,86 +1844,6 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
}, this.debounceMs + 100);
|
||||
}
|
||||
|
||||
/**
|
||||
* Handle a raw fs.watch callback. `filename` is relative to tasksDir.
|
||||
*/
|
||||
private handleFsChange(filename: string): void {
|
||||
// We only care about task.json files
|
||||
const parts = filename.split(sep);
|
||||
// Normalize for platforms that may use forward slashes
|
||||
const normalizedParts = parts.length === 1 ? filename.split("/") : parts;
|
||||
|
||||
if (normalizedParts.length < 2) return;
|
||||
const taskId = normalizedParts[0];
|
||||
const file = normalizedParts[normalizedParts.length - 1];
|
||||
if (file !== "task.json") return;
|
||||
if (!/^[A-Z]+-\d+$/.test(taskId)) return;
|
||||
|
||||
const fullPath = join(this.tasksDir, taskId, "task.json");
|
||||
|
||||
// Check suppression
|
||||
if (this.recentlyWritten.has(fullPath)) return;
|
||||
|
||||
// Debounce per task ID
|
||||
const existing = this.debounceTimers.get(taskId);
|
||||
if (existing) clearTimeout(existing);
|
||||
|
||||
this.debounceTimers.set(
|
||||
taskId,
|
||||
setTimeout(() => {
|
||||
this.debounceTimers.delete(taskId);
|
||||
this.processTaskChange(taskId, fullPath).catch(() => {
|
||||
// Ignore errors (file may have been deleted mid-read)
|
||||
});
|
||||
}, this.debounceMs),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Read a task.json from disk and diff against the cache to emit the right event.
|
||||
*/
|
||||
private async processTaskChange(taskId: string, filePath: string): Promise<void> {
|
||||
const cached = this.taskCache.get(taskId);
|
||||
|
||||
if (!existsSync(filePath)) {
|
||||
// Task was deleted
|
||||
if (cached) {
|
||||
this.taskCache.delete(taskId);
|
||||
this.emit("task:deleted", cached);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
let task: Task;
|
||||
try {
|
||||
const taskDir = join(this.tasksDir, taskId);
|
||||
task = await this.readTaskJson(taskDir);
|
||||
} catch {
|
||||
return; // File not readable or invalid JSON
|
||||
}
|
||||
|
||||
if (!cached) {
|
||||
// New task
|
||||
this.taskCache.set(taskId, { ...task });
|
||||
this.emit("task:created", task);
|
||||
return;
|
||||
}
|
||||
|
||||
// Check for column change → task:moved
|
||||
if (cached.column !== task.column) {
|
||||
const from = cached.column;
|
||||
this.taskCache.set(taskId, { ...task });
|
||||
this.emit("task:moved", { task, from, to: task.column });
|
||||
return;
|
||||
}
|
||||
|
||||
// Check for any other field change → task:updated
|
||||
if (JSON.stringify(cached) !== JSON.stringify(task)) {
|
||||
this.taskCache.set(taskId, { ...task });
|
||||
this.emit("task:updated", task);
|
||||
}
|
||||
}
|
||||
|
||||
private static ALLOWED_MIME_TYPES = new Set([
|
||||
"image/png",
|
||||
"image/jpeg",
|
||||
|
||||
@@ -628,7 +628,10 @@ export interface GlobalSettings {
|
||||
* at the very top of model selection dropdowns, before provider groups. Order is
|
||||
* preserved - earlier entries appear higher. */
|
||||
favoriteModels?: string[];
|
||||
|
||||
/** When true, the dashboard eagerly fetches the latest model catalog from
|
||||
* the OpenRouter API at startup so the model picker shows all available
|
||||
* OpenRouter models (not just the static built-in list). Default: true. */
|
||||
openrouterModelSync?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -739,6 +742,15 @@ export interface ProjectSettings {
|
||||
* lock files (ours), generated files (theirs), and trivial whitespace conflicts
|
||||
* without spawning an AI agent. Default: true. */
|
||||
smartConflictResolution?: boolean;
|
||||
/** When true, out-of-scope file changes block merge instead of just logging warnings.
|
||||
* Useful for teams that want strict enforcement of declared File Scope.
|
||||
* Default: false (soft guardrail — warnings only). */
|
||||
strictScopeEnforcement?: boolean;
|
||||
/** Maximum number of build retry attempts during merge when a build fails with a
|
||||
* transient error. Default: 0 (no retry). Set to 1 to allow one retry. */
|
||||
buildRetryCount?: number;
|
||||
/** Timeout in milliseconds for build commands during merge. Default: 300000 (5 min). */
|
||||
buildTimeoutMs?: number;
|
||||
/** When enabled, AI-generated task specifications require manual approval
|
||||
* before the task can move from triage to todo. Tasks with approved specs
|
||||
* remain in triage with status "awaiting-approval" until a user approves
|
||||
@@ -834,6 +846,7 @@ export const DEFAULT_GLOBAL_SETTINGS: Required<Pick<GlobalSettings, "themeMode"
|
||||
ntfyTopic: undefined,
|
||||
ntfyEvents: ["in-review", "merged", "failed"],
|
||||
ntfyDashboardHost: undefined,
|
||||
openrouterModelSync: true,
|
||||
};
|
||||
|
||||
/** Default values for project-level settings. */
|
||||
@@ -864,6 +877,9 @@ export const DEFAULT_PROJECT_SETTINGS: ProjectSettings = {
|
||||
defaultPresetBySize: {},
|
||||
autoResolveConflicts: true,
|
||||
smartConflictResolution: true,
|
||||
strictScopeEnforcement: false,
|
||||
buildRetryCount: 0,
|
||||
buildTimeoutMs: 300_000,
|
||||
requirePlanApproval: false,
|
||||
taskStuckTimeoutMs: undefined,
|
||||
autoUnpauseEnabled: true,
|
||||
@@ -908,6 +924,7 @@ export const GLOBAL_SETTINGS_KEYS: ReadonlyArray<keyof GlobalSettings> = [
|
||||
"ntfyEvents",
|
||||
"ntfyDashboardHost",
|
||||
"defaultProjectId",
|
||||
"openrouterModelSync",
|
||||
] as const;
|
||||
|
||||
/** Keys that belong to the project settings scope. */
|
||||
@@ -971,6 +988,8 @@ export interface MergeResult extends MergeDetails {
|
||||
worktreeRemoved: boolean;
|
||||
branchDeleted: boolean;
|
||||
error?: string;
|
||||
/** Internal flag to track if a build retry has been attempted. Not persisted. */
|
||||
_buildRetried?: boolean;
|
||||
}
|
||||
|
||||
export const COLUMN_LABELS: Record<Column, string> = {
|
||||
|
||||
Reference in New Issue
Block a user