perf(executor): recover approved steps on engine restart
When the engine restarts mid-step, an in-progress step may have already passed plan + code review but not yet been flipped to done by the agent's next task_update call. Previously, the next executor pass re-entered the step and replayed both reviews — measured at 5-20 min of pure waste per restart (observed in FN-2215 Step 1 and FN-2207 Step 6). recoverApprovedStepsOnResume scans the task log for any in-progress step whose most recent "code review Step N: APPROVE" entry is newer than its most recent "Step N → pending" transition, and marks those steps done before execute() runs. Safely skips steps that were reset after approval (e.g. by a workflow revision) or only received REVISE verdicts. Called from both the engine-restart path (resumeOrphaned) and the unpause path, matching the two places the task log shows as vulnerable to this race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,6 +1,16 @@
|
||||
{
|
||||
"name": "@fusion/core",
|
||||
"version": "0.1.0",
|
||||
"description": "Fusion core: task store, scheduler, settings, and shared domain types backing the Fusion AI coding agent.",
|
||||
"homepage": "https://github.com/Runfusion/Fusion#readme",
|
||||
"repository": {
|
||||
"type": "git",
|
||||
"url": "https://github.com/Runfusion/Fusion",
|
||||
"directory": "packages/core"
|
||||
},
|
||||
"bugs": {
|
||||
"url": "https://github.com/Runfusion/Fusion/issues"
|
||||
},
|
||||
"type": "module",
|
||||
"exports": {
|
||||
".": {
|
||||
|
||||
@@ -15,7 +15,7 @@
|
||||
|
||||
import { homedir } from "node:os";
|
||||
import { dirname, join } from "node:path";
|
||||
import { mkdir, readFile, writeFile, rename } from "node:fs/promises";
|
||||
import { mkdir, readFile, writeFile, rename, chmod } from "node:fs/promises";
|
||||
import { existsSync, mkdirSync, renameSync } from "node:fs";
|
||||
import type { GlobalSettings } from "./types.js";
|
||||
import { DEFAULT_GLOBAL_SETTINGS } from "./types.js";
|
||||
@@ -228,11 +228,24 @@ export class GlobalSettingsStore {
|
||||
/**
|
||||
* Atomically write settings to disk. Writes to a temp file first,
|
||||
* then renames into place (atomic on POSIX).
|
||||
*
|
||||
* The file is written with mode 0600 (owner-only read/write) because the
|
||||
* settings object can contain secrets — specifically `daemonToken`, which
|
||||
* is a bearer credential for the HTTP API. POSIX-only; no-op on Windows.
|
||||
*/
|
||||
private async atomicWrite(settings: GlobalSettings): Promise<void> {
|
||||
const tmpPath = this.settingsPath + ".tmp";
|
||||
await writeFile(tmpPath, JSON.stringify(settings, null, 2));
|
||||
await writeFile(tmpPath, JSON.stringify(settings, null, 2), { mode: 0o600 });
|
||||
await rename(tmpPath, this.settingsPath);
|
||||
// `writeFile` with `mode` honors umask on some platforms, so re-chmod the
|
||||
// final path to guarantee 0600. Ignore failures (Windows has no POSIX
|
||||
// permission bits; some filesystems may reject chmod).
|
||||
try {
|
||||
await chmod(this.settingsPath, 0o600);
|
||||
} catch {
|
||||
// Best effort — on Windows or filesystems without POSIX perms, the file
|
||||
// is already protected by the user's home directory ACL.
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -41,7 +41,7 @@ export const DEFAULT_GLOBAL_SETTINGS = {
|
||||
// Daemon mode settings
|
||||
daemonToken: undefined,
|
||||
daemonPort: 4040,
|
||||
daemonHost: "0.0.0.0",
|
||||
daemonHost: "127.0.0.1",
|
||||
// Node settings sync
|
||||
settingsSyncEnabled: false,
|
||||
settingsSyncAuth: false,
|
||||
|
||||
@@ -31,6 +31,53 @@ const TASK_ACTIVITY_LOG_OUTCOME_LIMIT = 4_000;
|
||||
const ARCHIVE_AGENT_LOG_SNAPSHOT_LIMIT = 25;
|
||||
const ARCHIVE_AGENT_LOG_SNIPPET_LIMIT = 160;
|
||||
|
||||
/**
|
||||
* Reject branch names that would be unsafe to interpolate into a shell command.
|
||||
* The allowed set is a conservative subset of git's refname rules: alphanumerics,
|
||||
* `_`, `.`, `/`, `+`, and `-`, with the same leading/trailing/segment restrictions
|
||||
* git enforces. Any branch that fails this check is rejected before reaching the
|
||||
* shell, so no branch-name value can inject shell metacharacters.
|
||||
*/
|
||||
function assertSafeGitBranchName(name: string): void {
|
||||
if (
|
||||
!name ||
|
||||
name.length > 255 ||
|
||||
name.startsWith("-") ||
|
||||
name.startsWith(".") ||
|
||||
name.startsWith("/") ||
|
||||
name.endsWith("/") ||
|
||||
name.endsWith(".") ||
|
||||
name.endsWith(".lock") ||
|
||||
name.includes("..") ||
|
||||
name.includes("@{") ||
|
||||
!/^[A-Za-z0-9._/+-]+$/.test(name)
|
||||
) {
|
||||
throw new Error(`Unsafe git branch name: ${JSON.stringify(name)}`);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Reject filesystem paths that would be unsafe to interpolate into a shell
|
||||
* command. Worktree paths are generated by fusion itself and are expected to
|
||||
* be absolute, but `task.worktree` is writable via the authenticated API, so
|
||||
* validate at the shell boundary as defense-in-depth.
|
||||
*/
|
||||
function assertSafeAbsolutePath(path: string): void {
|
||||
const isAbsolute = path.startsWith("/") || /^[A-Za-z]:[\\/]/.test(path);
|
||||
if (
|
||||
!path ||
|
||||
path.length > 4096 ||
|
||||
!isAbsolute ||
|
||||
path.startsWith("-") ||
|
||||
// Reject shell metacharacters, quotes, control chars, and NULs.
|
||||
/["'`$\n\r\t;&|<>()*?\[\]{}\\\0]/.test(
|
||||
path.replace(/^[A-Za-z]:/, ""), // ignore the drive-letter colon on Windows
|
||||
)
|
||||
) {
|
||||
throw new Error(`Unsafe path: ${JSON.stringify(path)}`);
|
||||
}
|
||||
}
|
||||
|
||||
function truncateTaskLogOutcome(outcome: string | undefined): string | undefined {
|
||||
if (!outcome || outcome.length <= TASK_ACTIVITY_LOG_OUTCOME_LIMIT) {
|
||||
return outcome;
|
||||
@@ -2916,6 +2963,13 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
|
||||
const deleted: string[] = [];
|
||||
for (const branch of branches) {
|
||||
try {
|
||||
assertSafeGitBranchName(branch);
|
||||
} catch {
|
||||
// Skip branches whose names would be unsafe to pass through a shell.
|
||||
// A malformed stored value should not become a command-injection vector.
|
||||
continue;
|
||||
}
|
||||
const verify = await this.runGitCommand(`git rev-parse --verify "${branch}"`);
|
||||
if (verify.exitCode !== 0) {
|
||||
continue;
|
||||
@@ -3034,6 +3088,9 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
const dir = this.taskDir(id);
|
||||
const task = await this.readTaskJson(dir);
|
||||
const branch = `fusion/${id.toLowerCase()}`;
|
||||
// Branch is derived from the task id (already validated at create time),
|
||||
// but assert as defense-in-depth against future id-format changes.
|
||||
assertSafeGitBranchName(branch);
|
||||
|
||||
if (task.column === "done") {
|
||||
const result: MergeResult = {
|
||||
@@ -3048,6 +3105,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
const changed = this.clearDoneTransientFields(task);
|
||||
|
||||
if (worktreePath && existsSync(worktreePath)) {
|
||||
assertSafeAbsolutePath(worktreePath);
|
||||
const removeWorktree = await this.runGitCommand(`git worktree remove "${worktreePath}" --force`, 120_000);
|
||||
if (removeWorktree.exitCode === 0) {
|
||||
result.worktreeRemoved = true;
|
||||
@@ -3130,6 +3188,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
|
||||
// 3. Remove worktree
|
||||
if (worktreePath && existsSync(worktreePath)) {
|
||||
assertSafeAbsolutePath(worktreePath);
|
||||
const removeWorktree = await this.runGitCommand(`git worktree remove "${worktreePath}" --force`, 120_000);
|
||||
if (removeWorktree.exitCode === 0) {
|
||||
result.worktreeRemoved = true;
|
||||
|
||||
@@ -863,7 +863,10 @@ export interface DaemonTokenSettings {
|
||||
daemonToken?: string;
|
||||
/** Port for daemon mode server binding. Default: 4040. */
|
||||
daemonPort?: number;
|
||||
/** Host for daemon mode server binding. Default: "0.0.0.0" (all interfaces). */
|
||||
/** Host for daemon mode server binding. Default: "127.0.0.1" (localhost only).
|
||||
* Set to "0.0.0.0" explicitly to expose the API on all interfaces — only do
|
||||
* this if you understand the implications (terminal/exec endpoints become
|
||||
* reachable from the LAN even with a bearer token). */
|
||||
daemonHost?: string;
|
||||
}
|
||||
|
||||
@@ -978,7 +981,10 @@ export interface GlobalSettings {
|
||||
daemonToken?: string;
|
||||
/** Port for daemon mode server binding. Default: 4040. */
|
||||
daemonPort?: number;
|
||||
/** Host for daemon mode server binding. Default: "0.0.0.0" (all interfaces). */
|
||||
/** Host for daemon mode server binding. Default: "127.0.0.1" (localhost only).
|
||||
* Set to "0.0.0.0" explicitly to expose the API on all interfaces — only do
|
||||
* this if you understand the implications (terminal/exec endpoints become
|
||||
* reachable from the LAN even with a bearer token). */
|
||||
daemonHost?: string;
|
||||
/** When true, enables automatic settings synchronization between nodes.
|
||||
* Settings are pushed/pulled on the configured interval. Default: false. */
|
||||
|
||||
Reference in New Issue
Block a user