fix(review): apply autofix feedback

This commit is contained in:
gsxdsm
2026-06-05 01:53:42 -07:00
parent e208842606
commit 6c9a81a05f
16 changed files with 122 additions and 36 deletions

View File

@@ -21,7 +21,7 @@ These tools are **not** part of the user-invokable extension surface. They are i
| `fn_workflow_create` | executor | Create a custom workflow definition from a graph IR (validated server-side). v2 IR supports step-inversion constructs: `parse-steps`, `foreach` (mode/isolation/concurrency/maxReworkCycles), `step-execute`, `step-review`, `code` nodes, `rework` edges, plus `artifacts`, custom `fields`, and typed `settings` declarations | `name` (string), `description?` (string), `ir` (object), `layout?` (object) |
| `fn_workflow_update` | executor | Update a custom workflow definition's name/description/ir/layout (built-ins cannot be edited; same step-inversion IR constructs as create; editing `fields` orphans rather than destroys existing task values; editing `settings` declarations drops orphaned setting values on resolution) | `workflow_id` (string), `name?` (string), `description?` (string), `ir?` (object), `layout?` (object), `rehome_to?` (string) |
| `fn_workflow_delete` | executor | Delete a custom workflow definition (built-ins cannot be deleted); selecting tasks are re-homed to the default workflow's entry column | `workflow_id` (string) |
| `fn_workflow_settings` | executor | Read/write a workflow's per-`(workflow, project)` setting **values** (`get` returns `{stored, effective}`; `set` writes `values`, with `null` clearing an override). Validated against the named workflow's declared settings; built-in **values** are writable though built-in **declarations** are not; invalid values return a typed rejection list and persist nothing | `action` (`get` \| `set`), `workflow_id` (string), `values?` (object keyed by setting id) |
| `fn_workflow_settings` | executor | Read/write a workflow's per-`(workflow, project)` setting **values** (`get` returns `{stored, effective, orphaned}`; `set` writes `values` and returns `{stored, effective, orphaned}`, with `null` clearing an override — including any stored value for an orphaned key). Validated against the named workflow's declared settings; built-in **values** are writable though built-in **declarations** are not; invalid values return a typed rejection list and persist nothing | `action` (`get` \| `set`), `workflow_id` (string), `values?` (object keyed by setting id) |
| `fn_task_promote` | executor | Promote a held task out of a manual-release hold column (defaults to the current task) | `task_id?` (string) |
| `fn_trait_list` | executor | List the registered column trait catalog (built-in and plugin traits) | none |
| `fn_memory_search` | triage, executor, heartbeat | Search project memory plus per-agent layered memory snippets | `query` (string), `limit?` (number) |
@@ -124,12 +124,15 @@ An invalid value (e.g. an enum violation) is rejected with a typed list and pers
// [{ "code": "enum-violation", "settingId": "reviewHandoffPolicy", "message": "..." }]
```
Read values — `effective` is what the engine actually consumes (declaration defaults filled in, orphaned values dropped); `stored` is the raw override map:
Read values — `effective` is what the engine actually consumes (declaration defaults filled in, orphaned values dropped); `stored` is the raw override map; `orphaned` lists stored entries with no current declaration (or a value that no longer validates). `set` returns the same `{stored, effective, orphaned}` shape:
```jsonc
// fn_workflow_settings
{ "action": "get", "workflow_id": "builtin:coding" }
// → { "workflowId": "builtin:coding",
// "stored": { "workflowStepTimeoutMs": 600000 },
// "effective": { "workflowStepTimeoutMs": 600000, "reviewHandoffPolicy": "disabled", ... } }
// "effective": { "workflowStepTimeoutMs": 600000, "reviewHandoffPolicy": "disabled", ... },
// "orphaned": [] }
```
Patching a key to `null` clears any stored value for it — including a value left behind under an orphaned key — so `set` doubles as the way to drop orphans. To see the full declaration catalog (every setting id, type, and default) call `fn_workflow_get` on `builtin:coding`, whose IR `settings` array is the canonical catalog.

View File

@@ -110,6 +110,9 @@ export async function runSettingsImport(
if (result.projectCount > 0) {
console.log(` Imported ${result.projectCount} project setting(s)`);
}
if (result.workflowSettingsCount > 0) {
console.log(` Upgraded ${result.workflowSettingsCount} workflow setting value(s)`);
}
console.log();
process.exit(0);

View File

@@ -1045,6 +1045,11 @@ describe("schema migration", () => {
const valuesColumn = columns.find((column) => column.name === "values");
expect(valuesColumn?.dflt_value).toBe("'{}'");
// The per-projectId lookup index is created alongside the table so migrated
// DBs match the fresh schema.
const indexes = db.prepare("PRAGMA index_list(workflow_settings)").all() as Array<{ name: string }>;
expect(indexes.some((index) => index.name === "idx_workflow_settings_project")).toBe(true);
expect(db.getSchemaVersion()).toBe(109);
db.close();
});

View File

@@ -625,6 +625,7 @@ CREATE TABLE IF NOT EXISTS workflow_settings (
updatedAt TEXT NOT NULL,
PRIMARY KEY (workflowId, projectId)
);
CREATE INDEX IF NOT EXISTS idx_workflow_settings_project ON workflow_settings(projectId);
-- Task documents (key-value store per task with revision tracking)
CREATE TABLE IF NOT EXISTS task_documents (
@@ -4317,6 +4318,7 @@ export class Database {
updatedAt TEXT NOT NULL,
PRIMARY KEY (workflowId, projectId)
);
CREATE INDEX IF NOT EXISTS idx_workflow_settings_project ON workflow_settings(projectId);
`);
});
}

View File

@@ -25,6 +25,9 @@ import {
MOVED_SETTINGS_KEYS,
stripMovedSettingsKeys,
} from "./moved-settings.js";
import { createLogger } from "./logger.js";
const log = createLogger("settings-export");
/** Current export format version emitted by {@link exportSettings}. */
export const SETTINGS_EXPORT_VERSION = 2;
@@ -279,18 +282,18 @@ async function applyWorkflowSettingsSection(
const rejectedIds = extractRejectedSettingIds(err);
if (rejectedIds.length === 0) {
// Unknown error (not a value-rejection) — log and skip this workflow.
console.warn(
`[settings-import] skipped workflow setting values for '${workflowId}': ${
err instanceof Error ? err.message : String(err)
}`,
);
log.warn("[settings-import] skipped workflow setting values", {
workflowId,
error: err instanceof Error ? err.message : String(err),
});
break;
}
for (const id of rejectedIds) {
delete patch[id];
console.warn(
`[settings-import] dropped invalid workflow setting value '${id}' for workflow '${workflowId}'`,
);
log.warn("[settings-import] dropped invalid workflow setting value", {
workflowId,
settingId: id,
});
}
}
}

View File

@@ -13,8 +13,14 @@ type CompleteSettings<T> = { [K in keyof Required<T>]: Required<T>[K] | undefine
* on the `ProjectSettings` type for the engine's flat `settings.<key>` reads and
* the U3 effective-settings merge. `DEFAULT_PROJECT_SETTINGS` is therefore
* type-checked against `ProjectSettings` MINUS these keys — the type-vs-schema
* split documented in `moved-settings.ts`. This union MUST stay in lockstep with
* `MOVED_SETTINGS_KEYS` (the parity/consistency tests enforce coherence).
* split documented in `moved-settings.ts`.
*
* This union is NOT compile-time-enforced against `MOVED_SETTINGS_KEYS`.
* Enforcement lives in `src/__tests__/settings-consistency.test.ts` (every key
* must belong to exactly one regime). A STALE entry here only loosens the `Omit`
* type — at worst it lets `DEFAULT_PROJECT_SETTINGS` drop a key it should keep;
* it can never re-add a key to the schema object. A MISSING entry surfaces as a
* type error on `DEFAULT_PROJECT_SETTINGS` if that key still has a default.
*/
type MovedProjectSettingsKey =
| "workflowStepTimeoutMs"

View File

@@ -16,17 +16,26 @@
* shapes (`form.<key>` and `<key>:`) and explicitly allow descriptor mentions.
*/
import { describe, it, expect } from "vitest";
import { readFileSync } from "node:fs";
import { readdirSync, readFileSync } from "node:fs";
import { fileURLToPath } from "node:url";
import { dirname, join } from "node:path";
import { MOVED_SETTINGS_KEYS } from "@fusion/core";
const here = dirname(fileURLToPath(import.meta.url));
const componentsDir = join(here, "..", "components");
const sectionsDir = join(componentsDir, "settings", "sections");
/** Files that compose the modal's editable surface (shell + Project sections). */
/**
* Files that compose the modal's editable surface: the shell plus every
* extracted section component. The sections are discovered by walking the
* directory (not a hardcoded list) so a newly added section is swept
* automatically and a moved-key binding cannot slip in unnoticed.
*/
const SURFACE_FILES = [
"SettingsModal.tsx",
{ dir: componentsDir, file: "SettingsModal.tsx" },
...readdirSync(sectionsDir)
.filter((name) => name.endsWith(".tsx"))
.map((file) => ({ dir: sectionsDir, file })),
];
/**
@@ -41,8 +50,8 @@ const PRESET_NESTED_KEYS = new Set([
]);
describe("SettingsModal moved-key removal sweep", () => {
for (const file of SURFACE_FILES) {
const source = readFileSync(join(componentsDir, file), "utf8");
for (const { dir, file } of SURFACE_FILES) {
const source = readFileSync(join(dir, file), "utf8");
for (const key of MOVED_SETTINGS_KEYS) {
it(`${file} does not read form.${key}`, () => {

View File

@@ -14,7 +14,7 @@
*/
import { describe, it, expect } from "vitest";
import { isGlobalSettingsKey, isProjectSettingsKey } from "@fusion/core";
import { splitSettingsSave } from "../components/settings/save-split";
import { splitSettingsSave, MODEL_LANE_KEYS } from "../components/settings/save-split";
// Sanity-anchor the scope of the concrete keys this test relies on, so the
// assertions below remain meaningful if core's catalog ever shifts.
@@ -25,6 +25,16 @@ describe("scope anchors", () => {
expect(isProjectSettingsKey("maxConcurrent")).toBe(true);
expect(isProjectSettingsKey("integrationBranch")).toBe(true);
});
it("every MODEL_LANE_KEYS entry is a project settings key", () => {
// MODEL_LANE_KEYS only gates project-branch behavior, which is reached only
// for keys that pass isProjectSettingsKey. Any entry that fails this check is
// dead (e.g. a per-phase model lane that moved to workflow settings).
expect(MODEL_LANE_KEYS.length).toBeGreaterThan(0);
for (const key of MODEL_LANE_KEYS) {
expect(isProjectSettingsKey(key)).toBe(true);
}
});
});
describe("splitSettingsSave", () => {

View File

@@ -6275,6 +6275,7 @@ export interface SettingsImportResponse {
success: boolean;
globalCount: number;
projectCount: number;
workflowSettingsCount: number;
error?: string;
}

View File

@@ -550,7 +550,7 @@
background: var(--text-muted);
}
.settings-content > * {
animation: settingsFadeIn var(--transition-normal);
animation: settingsFadeIn var(--duration-normal) ease;
}
@keyframes settingsFadeIn {
from {

View File

@@ -423,6 +423,9 @@ export function SettingsModal({
webhookEvents: undefined,
});
const [loading, setLoading] = useState(true);
// Guards the Save action against double-submit (rapid clicks / Enter) while the
// parallel global+project writes are in flight.
const [isSaving, setIsSaving] = useState(false);
// Track initial values to detect explicit clears for null-as-delete semantics
const [initialValues, setInitialValues] = useState<Settings | null>(null);
// Track scoped settings for inheritance detection (fetched alongside merged settings)
@@ -1605,6 +1608,7 @@ export function SettingsModal({
const parts: string[] = [];
if (result.globalCount > 0) parts.push(`${result.globalCount} global`);
if (result.projectCount > 0) parts.push(`${result.projectCount} project`);
if (result.workflowSettingsCount > 0) parts.push(`${result.workflowSettingsCount} workflow setting value(s)`);
addToast(`Imported ${parts.join(", ")} setting(s)`, "success");
setImportDialogOpen(false);
setImportPreview(null);
@@ -1916,6 +1920,7 @@ export function SettingsModal({
}, []);
const handleSave = useCallback(async () => {
if (isSaving) return;
if (prefixError || presetDraft) return;
const limits = form.researchSettings?.limits;
@@ -1937,6 +1942,7 @@ export function SettingsModal({
}
setResearchLimitError(null);
setIsSaving(true);
try {
const payload = {
...form,
@@ -1982,8 +1988,10 @@ export function SettingsModal({
onClose();
} catch (err) {
addToast(getErrorMessage(err), "error");
} finally {
setIsSaving(false);
}
}, [form, globalMaxConcurrent, prefixError, presetDraft, initialValues, initialScopedValues, onClose, addToast, projectId, activeSection]);
}, [form, globalMaxConcurrent, prefixError, presetDraft, initialValues, initialScopedValues, onClose, addToast, projectId, activeSection, isSaving, t]);
const handleSaveMemory = useCallback(async () => {
try {
@@ -2709,7 +2717,7 @@ export function SettingsModal({
<button className="btn btn-sm" onClick={onClose}>
{t("settings.actions.cancel", "Cancel")}
</button>
<button className="btn btn-primary btn-sm" onClick={handleSave} disabled={loading}>
<button className="btn btn-primary btn-sm" onClick={handleSave} disabled={loading || isSaving}>
{t("settings.actions.save", "Save")}
</button>
</div>

View File

@@ -254,6 +254,15 @@ function InnerEditor({
}
}, [initialPanel, activeWorkflow]);
// Reset the one-shot scroll latch whenever the deep-link target changes (e.g.
// the panel is closed and re-opened with `?panel=settings`), so a fresh open
// scrolls the settings panel into view again instead of staying latched.
useEffect(() => {
return () => {
didScrollToSettings.current = false;
};
}, [initialPanel]);
// Server-reported node error (e.g. seam-in-branch) attributed to a node id.
const [serverNodeError, setServerNodeError] = useState<{ nodeId: string; message: string } | null>(null);

View File

@@ -771,7 +771,7 @@ function ValuesTab({
<button
className="wf-settings-orphan-delete"
aria-label={t("workflowSettings.deleteOrphan", "Delete orphaned value")}
disabled={staleContext}
disabled={staleContext || saving}
onClick={() => deleteOrphan(o.id)}
>
<Trash2 size={12} />
@@ -848,6 +848,7 @@ export function WorkflowSettingsPanel({
<DefinitionsTab settings={settings} onChange={onChange} readOnly={readOnly} addToast={addToast} />
) : (
<ValuesTab
key={workflowId}
workflowId={workflowId}
settings={settings}
boundProjectId={boundProjectId}

View File

@@ -25,16 +25,18 @@
import { isGlobalSettingsKey, isProjectSettingsKey } from "@fusion/core";
import type { GlobalSettings, Settings } from "@fusion/core";
/** Model-lane keys whose project overrides track inheritance explicitly. */
/**
* Project-scoped model-override keys whose overrides track inheritance
* explicitly (changed-only writes with null-as-delete in the project branch).
*
* The per-phase model lanes (planning/validator/execution/titleSummarizer
* providers, models, and fallbacks) MOVED to workflow settings (U4) and are no
* longer project keys, so `isProjectSettingsKey` filters them out before the
* project branch is reached — listing them here would be dead. Only the two
* project-level default overrides remain.
*/
export const MODEL_LANE_KEYS = [
"planningProvider", "planningModelId",
"validatorProvider", "validatorModelId",
"executionProvider", "executionModelId",
"titleSummarizerProvider", "titleSummarizerModelId",
"defaultProviderOverride", "defaultModelIdOverride",
"planningFallbackProvider", "planningFallbackModelId",
"validatorFallbackProvider", "validatorFallbackModelId",
"titleSummarizerFallbackProvider", "titleSummarizerFallbackModelId",
] as const;
const MODEL_LANE_KEY_SET = new Set<string>(MODEL_LANE_KEYS);

View File

@@ -2353,6 +2353,7 @@ export function registerSettingsMemoryRoutes(ctx: ApiRoutesContext, deps: Settin
throw new ApiError(500, result.error ?? "Import failed", {
globalCount: result.globalCount,
projectCount: result.projectCount,
workflowSettingsCount: result.workflowSettingsCount,
});
}
@@ -2360,6 +2361,7 @@ export function registerSettingsMemoryRoutes(ctx: ApiRoutesContext, deps: Settin
success: true,
globalCount: result.globalCount,
projectCount: result.projectCount,
workflowSettingsCount: result.workflowSettingsCount,
});
} catch (err: unknown) {
if (err instanceof ApiError) {

View File

@@ -11,8 +11,8 @@ import { appendFile, mkdir, readFile, readdir, stat, writeFile } from "node:fs/p
import { existsSync } from "node:fs";
import { createHash } from "node:crypto";
import { join, relative, resolve } from "node:path";
import type { AgentStore, AgentState, AgentCapability, AgentUpdateInput, TaskDocument, TaskDocumentCreateInput, TaskStore, RunMutationContext, MessageStore, Message, SourceType, Settings, ResearchRun, ResearchRunStatus, TaskCreateInput, ReflectionStore, ApprovalRequestStore, ProjectSettings, ChatStore } from "@fusion/core";
import { listTraits, isBuiltinWorkflowId, WorkflowSettingRejectionError, resolveEffectiveSettingsById } from "@fusion/core";
import type { AgentStore, AgentState, AgentCapability, AgentUpdateInput, TaskDocument, TaskDocumentCreateInput, TaskStore, RunMutationContext, MessageStore, Message, SourceType, Settings, ResearchRun, ResearchRunStatus, TaskCreateInput, ReflectionStore, ApprovalRequestStore, ProjectSettings, ChatStore, WorkflowSettingDefinition } from "@fusion/core";
import { listTraits, isBuiltinWorkflowId, WorkflowSettingRejectionError, resolveEffectiveSettingsById, resolveWorkflowIrById, findOrphanedSettingValues, BUILTIN_WORKFLOW_SETTINGS } from "@fusion/core";
import { promoteHeldTask } from "./hold-release.js";
import { DASHBOARD_USER_ID, canAgentTakeImplementationTaskForExplicitRouting, dailyMemoryPath, ensureOpenClawMemoryFiles, extractAgentProvisioningRequest, formatRoleMismatchReason, getMemoryBackendCapabilities, getProjectMemory, isEphemeralAgent, memoryLongTermPath, normalizeMessageParticipant, reconcileDeterministicDuplicate, resolveAgentProvisioningPolicy, resolveMemoryBackend, resolveResearchSettings, resolveTaskGithubTracking, resolveTitleSummarizerSettingsModel, runDeterministicDuplicateGuard, scheduleQmdProjectMemoryRefresh, searchProjectMemory, shouldSkipBackgroundQmdRefresh, summarizeTitle } from "@fusion/core";
import { ResearchOrchestrator } from "./research-orchestrator.js";
@@ -1407,6 +1407,23 @@ export function createWorkflowDeleteTool(store: TaskStore): ToolDefinition {
* values (post drop-on-orphan), so an agent sees what it wrote and what the engine
* will actually consume.
*/
/**
* Resolve the setting DECLARATIONS for a workflow. Mirrors the store's private
* `resolveWorkflowSettingDeclarations` (and the dashboard route helper of the
* same shape): the resolved IR's `settings` when present, else the built-in
* catalog for built-in ids. Used to compute the `orphaned` value entries.
*/
async function resolveWorkflowSettingDeclarationsForTool(
store: TaskStore,
workflowId: string,
): Promise<WorkflowSettingDefinition[] | undefined> {
const ir = await resolveWorkflowIrById(store, workflowId);
const declared = ir.version === "v2" ? ir.settings : undefined;
if (declared && declared.length > 0) return declared;
if (isBuiltinWorkflowId(workflowId)) return BUILTIN_WORKFLOW_SETTINGS;
return declared;
}
export function createWorkflowSettingsTool(store: TaskStore): ToolDefinition {
return {
name: "fn_workflow_settings",
@@ -1447,12 +1464,14 @@ export function createWorkflowSettingsTool(store: TaskStore): ToolDefinition {
try {
const stored = store.getWorkflowSettingValues(workflowId, projectId);
const effective = await resolveEffectiveSettingsById(store, workflowId, projectId);
const declarations = await resolveWorkflowSettingDeclarationsForTool(store, workflowId);
const orphaned = findOrphanedSettingValues(declarations, stored);
return {
content: [{
type: "text" as const,
text: JSON.stringify({ workflowId, stored, effective }, null, 2),
text: JSON.stringify({ workflowId, stored, effective, orphaned }, null, 2),
}],
details: { workflowId, stored, effective },
details: { workflowId, stored, effective, orphaned },
};
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (err: any) {
@@ -1475,12 +1494,15 @@ export function createWorkflowSettingsTool(store: TaskStore): ToolDefinition {
}
try {
const next = await store.updateWorkflowSettingValues(workflowId, projectId, values);
const effective = await resolveEffectiveSettingsById(store, workflowId, projectId);
const declarations = await resolveWorkflowSettingDeclarationsForTool(store, workflowId);
const orphaned = findOrphanedSettingValues(declarations, next);
return {
content: [{
type: "text" as const,
text: `Updated workflow settings for ${workflowId}: ${JSON.stringify(next)}`,
}],
details: { workflowId, stored: next },
details: { workflowId, stored: next, effective, orphaned },
};
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (err: any) {