From 6c9a81a05f6bdfb8f492dfa604ada0e308fae4ce Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 5 Jun 2026 01:53:42 -0700 Subject: [PATCH] fix(review): apply autofix feedback --- .../skill/fusion/references/engine-tools.md | 9 ++++-- packages/cli/src/commands/settings-import.ts | 3 ++ .../core/src/__tests__/db-migrate.test.ts | 5 +++ packages/core/src/db.ts | 2 ++ packages/core/src/settings-export.ts | 19 ++++++----- packages/core/src/settings-schema.ts | 10 ++++-- .../app/__tests__/settings-moved-keys.test.ts | 19 ++++++++--- .../app/__tests__/settings-save-split.test.ts | 12 ++++++- packages/dashboard/app/api/legacy.ts | 1 + .../app/components/SettingsModal.css | 2 +- .../app/components/SettingsModal.tsx | 12 +++++-- .../app/components/WorkflowNodeEditor.tsx | 9 ++++++ .../app/components/WorkflowSettingsPanel.tsx | 3 +- .../app/components/settings/save-split.ts | 18 ++++++----- .../routes/register-settings-memory-routes.ts | 2 ++ packages/engine/src/agent-tools.ts | 32 ++++++++++++++++--- 16 files changed, 122 insertions(+), 36 deletions(-) diff --git a/packages/cli/skill/fusion/references/engine-tools.md b/packages/cli/skill/fusion/references/engine-tools.md index f8950725e8..3798f7dd0e 100644 --- a/packages/cli/skill/fusion/references/engine-tools.md +++ b/packages/cli/skill/fusion/references/engine-tools.md @@ -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. diff --git a/packages/cli/src/commands/settings-import.ts b/packages/cli/src/commands/settings-import.ts index adf7346956..37d7e2c968 100644 --- a/packages/cli/src/commands/settings-import.ts +++ b/packages/cli/src/commands/settings-import.ts @@ -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); diff --git a/packages/core/src/__tests__/db-migrate.test.ts b/packages/core/src/__tests__/db-migrate.test.ts index fa2616266d..4dd3c8fc39 100644 --- a/packages/core/src/__tests__/db-migrate.test.ts +++ b/packages/core/src/__tests__/db-migrate.test.ts @@ -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(); }); diff --git a/packages/core/src/db.ts b/packages/core/src/db.ts index 8caf5b9118..16eeafa0b3 100644 --- a/packages/core/src/db.ts +++ b/packages/core/src/db.ts @@ -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); `); }); } diff --git a/packages/core/src/settings-export.ts b/packages/core/src/settings-export.ts index 2198e95941..6e2b180ac1 100644 --- a/packages/core/src/settings-export.ts +++ b/packages/core/src/settings-export.ts @@ -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, + }); } } } diff --git a/packages/core/src/settings-schema.ts b/packages/core/src/settings-schema.ts index 3c35ef9a30..5122b215af 100644 --- a/packages/core/src/settings-schema.ts +++ b/packages/core/src/settings-schema.ts @@ -13,8 +13,14 @@ type CompleteSettings = { [K in keyof Required]: Required[K] | undefine * on the `ProjectSettings` type for the engine's flat `settings.` 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" diff --git a/packages/dashboard/app/__tests__/settings-moved-keys.test.ts b/packages/dashboard/app/__tests__/settings-moved-keys.test.ts index 2ca176afeb..03f8dc4127 100644 --- a/packages/dashboard/app/__tests__/settings-moved-keys.test.ts +++ b/packages/dashboard/app/__tests__/settings-moved-keys.test.ts @@ -16,17 +16,26 @@ * shapes (`form.` and `:`) 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}`, () => { diff --git a/packages/dashboard/app/__tests__/settings-save-split.test.ts b/packages/dashboard/app/__tests__/settings-save-split.test.ts index cbf04f6bbc..0cf494dee2 100644 --- a/packages/dashboard/app/__tests__/settings-save-split.test.ts +++ b/packages/dashboard/app/__tests__/settings-save-split.test.ts @@ -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", () => { diff --git a/packages/dashboard/app/api/legacy.ts b/packages/dashboard/app/api/legacy.ts index ee6534266a..8598e9fa71 100644 --- a/packages/dashboard/app/api/legacy.ts +++ b/packages/dashboard/app/api/legacy.ts @@ -6275,6 +6275,7 @@ export interface SettingsImportResponse { success: boolean; globalCount: number; projectCount: number; + workflowSettingsCount: number; error?: string; } diff --git a/packages/dashboard/app/components/SettingsModal.css b/packages/dashboard/app/components/SettingsModal.css index b726bd574b..65907bff91 100644 --- a/packages/dashboard/app/components/SettingsModal.css +++ b/packages/dashboard/app/components/SettingsModal.css @@ -550,7 +550,7 @@ background: var(--text-muted); } .settings-content > * { - animation: settingsFadeIn var(--transition-normal); + animation: settingsFadeIn var(--duration-normal) ease; } @keyframes settingsFadeIn { from { diff --git a/packages/dashboard/app/components/SettingsModal.tsx b/packages/dashboard/app/components/SettingsModal.tsx index 6556421ee5..bb97acae65 100644 --- a/packages/dashboard/app/components/SettingsModal.tsx +++ b/packages/dashboard/app/components/SettingsModal.tsx @@ -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(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({ - diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.tsx b/packages/dashboard/app/components/WorkflowNodeEditor.tsx index e2568dc48a..2213c82e87 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.tsx +++ b/packages/dashboard/app/components/WorkflowNodeEditor.tsx @@ -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); diff --git a/packages/dashboard/app/components/WorkflowSettingsPanel.tsx b/packages/dashboard/app/components/WorkflowSettingsPanel.tsx index 9d33f7d28b..b0a3877b67 100644 --- a/packages/dashboard/app/components/WorkflowSettingsPanel.tsx +++ b/packages/dashboard/app/components/WorkflowSettingsPanel.tsx @@ -771,7 +771,7 @@ function ValuesTab({