feat: workflow settings, hard-move migration, Settings UI redesign - #1445
Merged
Conversation
… UI redesign plan
…ble, validating write authority, drop-on-orphan resolution
…er entry merge, fallback alignment
…ting-value routes, flow-mapping round-trip
…parity, plugin-sdk types, engine-tools docs
…to workflow setting values 30 moved keys (step execution, review/approval, per-phase model lanes) leave DEFAULT_PROJECT_SETTINGS; marker-gated idempotent per-project migration writes customized values to every in-use (workflowId, projectId); stale-writer guard; tombstone allowlist derived from the builtin catalog.
…flowSettings, sync moved-key filtering, CLI redirect hints, consistency guard
…ctions into shared-primitive components with pure save-split helper
…ow-editor redirect stubs, target-IA regroup, secrets/prompts extraction
…s pre-existing on main (infinite-timer abort, FN-4574 suite)
…sModal is now a shell (7,883 → 2,905 lines)
…n; docs and changeset
Contributor
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds typed workflow-level settings (IR + per-(workflow,project) values), an idempotent per-project hard‑move migration moving specific keys into workflow values (with tombstoning), effective‑settings resolution merged into engine paths, dashboard WorkflowSettings editor (Definitions/Values), export/import v2, DB schema/table, CLI/tool/docs, and extensive tests. ChangesWorkflow Settings Mechanism
Sequence DiagramsequenceDiagram
participant Dashboard
participant API
participant Store
participant Resolver
Dashboard->>API: PATCH /api/workflows/:id/setting-values { values }
API->>Store: updateWorkflowSettingValues(workflowId, projectId, patch)
Store-->>API: stored
API->>Resolver: resolveEffectiveSettingsById(store, workflowId, projectId)
Resolver->>Store: getWorkflowSettingValues(workflowId, projectId)
Store-->>Resolver: storedValues
Resolver-->>API: { effective, orphaned, storedKeys }
API-->>Dashboard: { stored, effective, orphaned }
Estimated code review effort Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/settings-reference.md (1)
1049-1063:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate examples to avoid moved workflow keys under project settings.
This example still sets
runStepsInNewSessions/maxParallelStepsunder projectsettings, which contradicts the new “moved keys are workflow settings” guidance above and can lead to no-op config attempts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/settings-reference.md` around lines 1049 - 1063, The example places workflow-only keys runStepsInNewSessions and maxParallelSteps inside the project "settings" object which contradicts the moved-keys guidance and makes them no-ops; update the example by removing runStepsInNewSessions and maxParallelSteps from the project "settings" block and instead document them as workflow-level settings (mentioning the keys runStepsInNewSessions and maxParallelSteps) so the example reflects the correct location and behavior.packages/engine/src/executor.ts (1)
8311-8312:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the latest task snapshot when merging effective settings in
fn_review_step.
fn_review_stepmerges with captureddetailfrom execute-start, even though this tool call already loadscurrentTask. If workflow selection changes mid-session, review settings can resolve from the wrong workflow.💡 Suggested fix
- const settings = await mergeEffectiveSettings(store, detail, await store.getSettings()); + const settings = await mergeEffectiveSettings(store, currentTask, await store.getSettings());Also applies to: 8345-8348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/executor.ts` around lines 8311 - 8312, fn_review_step is merging review settings using the captured execute-start "detail" instead of the live task snapshot; update the merge to use the loaded currentTask (e.g., use currentTask.steps / currentTask.settings or currentTask.* fields) when computing taskSteps and any effective review settings so workflow selection changes mid-session resolve from the latest snapshot; apply the same change to the analogous merge at the other site referenced (around the block currently using detail at the 8345-8348 region).
🧹 Nitpick comments (11)
packages/dashboard/app/components/settings/save-split.ts (1)
78-86: 💤 Low valueConsider skipping undefined values explicitly.
When both
valueandinitialValueare undefined, line 84 writesundefinedtoglobalPatch. While harmless (JSON.stringify drops it), it's cleaner to only write defined values:if (isGlobalSettingsKey(key)) { // null-as-delete: explicit clear is sent as null, plain undefined dropped. const initialValue = initialValues?.[key as keyof GlobalSettings]; if (value === undefined && initialValue !== undefined) { (globalPatch as Record<string, unknown>)[key] = null; - } else { + } else if (value !== undefined) { (globalPatch as Record<string, unknown>)[key] = value; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/settings/save-split.ts` around lines 78 - 86, The current isGlobalSettingsKey branch may assign undefined to globalPatch when both value and initialValue are undefined; change the logic in the block (inside isGlobalSettingsKey) so you only write to globalPatch when necessary: if value === undefined and initialValue !== undefined set (globalPatch as Record<string, unknown>)[key] = null; else if value !== undefined set (globalPatch as Record<string, unknown>)[key] = value; otherwise do not set the key. Use the existing identifiers isGlobalSettingsKey, initialValues, globalPatch, key, value, and initialValue to locate and update the code.packages/core/src/__tests__/db-migrate.test.ts (1)
1007-1054: ⚡ Quick winAssert foreign-key cascade contract for
workflow_settingsin the migration test.This test validates schema shape well, but it misses the deletion contract. Please also assert
PRAGMA foreign_key_list(workflow_settings)includes the expectedON DELETE CASCADEmapping so migrated DBs match fresh DB behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/db-migrate.test.ts` around lines 1007 - 1054, The test for migrating to schema version 109 is missing an assertion that the workflow_settings table enforces the expected foreign-key cascade behavior; update the test (the "adds workflow_settings table when migrating from schema version 108" case) to query PRAGMA foreign_key_list(workflow_settings) and assert that the relevant foreign key row(s) include "ON DELETE" set to "CASCADE" (i.e., the mapping for the parent column(s) points to CASCADE), ensuring migrated DBs match fresh DB behavior; keep this assertion alongside the existing checks for table columns, default values, index (idx_workflow_settings_project), and schema version (db.getSchemaVersion()).packages/core/src/__tests__/workflow-ir-settings.test.ts (1)
209-217: ⚡ Quick winExpand parity assertions beyond the coding built-in workflow.
The moved-key parity guard currently validates only one built-in workflow. Please assert the same invariant across the other known built-in workflow surfaces so catalog drift can’t hide outside this single path.
As per coding guidelines, regression tests should assert the general invariant across ALL known surfaces, not only a single reproduction (FN-5893).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/workflow-ir-settings.test.ts` around lines 209 - 217, The test only checks parity for BUILTIN_CODING_WORKFLOW_IR against BUILTIN_WORKFLOW_SETTINGS; extend it to assert the same invariant for all built-in workflow IR surfaces by iterating over the collection of built-in workflow IR constants (add the other BUILTIN_*_WORKFLOW_IR values used in the codebase) and for each one perform the same checks: build declaredIds from workflow.settings, ensure every entry in BUILTIN_WORKFLOW_SETTINGS is present, and finally expect(workflow.settings).toEqual(BUILTIN_WORKFLOW_SETTINGS); update the test description accordingly and reuse the existing assertions (the current logic around BUILTIN_CODING_WORKFLOW_IR and BUILTIN_WORKFLOW_SETTINGS) for each built-in workflow IR.packages/core/src/__tests__/run-audit.test.ts (1)
586-587: ⚡ Quick winAlign the test title with the new schema assertion.
Line 586 still references version
40while Line 587 asserts109. Rename the test description to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/run-audit.test.ts` around lines 586 - 587, The test description string in the it(...) block is out of sync with the assertion: update the test title used in the it(...) call (currently "schema version is bumped to 40") to match the asserted schema version (109), e.g., change the description to "schema version is bumped to 109" so the test name aligns with the expect(db.getSchemaVersion()).toBe(109) assertion.packages/core/src/__tests__/mission-store.test.ts (1)
3748-3749: ⚡ Quick winUpdate the test name to match the asserted schema version.
Line 3748 says
101, but Line 3749 asserts109. Please rename the test title so failures are not misleading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/mission-store.test.ts` around lines 3748 - 3749, Rename the test title string to match the asserted schema version: change the it(...) description "schema version is 101 after migration" to reflect 109 so it matches the expectation in the test where expect(db.getSchemaVersion()).toBe(109); ensure the updated description precisely references 109 to avoid misleading failures.packages/dashboard/app/components/settings/SettingsSelectRow.tsx (1)
38-50: ⚡ Quick winConsider rendering an explicit empty option when
valueis nullable.The select displays
value ?? ""but does not explicitly render an<option value="">unless it exists indescriptor.options. If no such option is provided, the select will display the first option's label whenvalueisnull, creating a visual mismatch. Users also cannot manually deselect tonullwithout the clear button.🛠️ Proposed fix to add explicit empty option
> <select id={key} className="settings-select" value={value ?? ""} disabled={disabled} onChange={(e) => onChange(e.target.value)} > + {clearable && <option value="">—</option>} {options.map((opt) => ( <option key={opt.value} value={opt.value}> {opt.label} </option> ))} </select>Alternatively, if the caller always provides an empty option in
descriptor.options, document that contract in a comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/settings/SettingsSelectRow.tsx` around lines 38 - 50, The select sets value to value ?? "" but doesn't ensure an explicit empty <option> exists, causing the first option to appear when value is null; update SettingsSelectRow to render an explicit empty option (e.g., an <option value="">—</option>) when value is null or when descriptor.options does not include an empty value so the UI matches the controlled value and users can clear to null, and ensure onChange continues to call onChange(e.target.value) (or convert "" back to null before calling if your prop expects null) so the component and descriptor.options handling remain consistent.packages/dashboard/app/components/settings/sections/ExperimentalSection.tsx (1)
34-43: 💤 Low valueVerify
experimentalFeaturesvsform.experimentalFeaturesconsistency.Line 34 defines
experimentalFeatures = form.experimentalFeatures ?? {}, then line 42 callsisFeatureEnabled(experimentalFeatures, key). This is correct sinceexperimentalFeaturesis the local snapshot. However, to reduce confusion and keep the pattern clear, consider whether the localexperimentalFeaturesvariable is necessary or if it's clearer to useform.experimentalFeatures ?? {}inline at line 42 to match the toggle handler (lines 67-70).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/settings/sections/ExperimentalSection.tsx` around lines 34 - 43, The local variable experimentalFeatures introduces potential confusion with form.experimentalFeatures; remove the const experimentalFeatures = form.experimentalFeatures ?? {} and instead use form.experimentalFeatures ?? {} inline where used (notably in the allFeatureKeys map and the isFeatureEnabled call) so the rendering and the toggle handler reference the same expression; update any references to experimentalFeatures (e.g., in the featureFlags mapping and any subsequent uses) to use form.experimentalFeatures ?? {} to keep the pattern consistent with the toggle handler that already uses form.experimentalFeatures.packages/dashboard/app/components/settings/sections/SchedulingSection.tsx (1)
77-77: ⚡ Quick winRemove unnecessary type assertions.
The
as SettingsFormStatecasts are unnecessary because the object spread already produces the correct type. Removing them improves type safety by allowing TypeScript to catch any genuine type mismatches.♻️ Proposed fix
- setForm((f) => ({ ...f, maxConcurrent: val === "" ? undefined : Number(val) } as SettingsFormState)); + setForm((f) => ({ ...f, maxConcurrent: val === "" ? undefined : Number(val) }));Apply the same pattern to lines 91 and 106.
Also applies to: 91-91, 106-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/settings/sections/SchedulingSection.tsx` at line 77, Remove the unnecessary "as SettingsFormState" type assertions on the setForm updates; locate the setForm calls (e.g., the one updating maxConcurrent and the other two similar setters) and simply return the spread object ({ ...f, ... }) without casting so TypeScript can infer the correct type and catch real mismatches in functions like setForm and the SettingsFormState usage.packages/dashboard/app/components/settings/sections/WorktreesSection.tsx (2)
265-265: 💤 Low valuePrefer CSS class over inline style.
The inline
style={{ color: "var(--color-error)" }}should use a CSS class for consistency with the rest of the codebase (e.g.,className="text-error").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/settings/sections/WorktreesSection.tsx` at line 265, The small tag in the WorktreesSection component is using an inline style for the error color; replace style={{ color: "var(--color-error)" }} with the existing CSS utility class (e.g., className="text-error") so it matches the codebase conventions — update the small element that renders worktrunkInstall.error (in WorktreesSection.tsx) to use className="text-error" instead of the inline style.
52-52: ⚡ Quick winRemove unnecessary type assertions.
The
as SettingsFormStatecasts are unnecessary because the object spread already preserves the correct type. Removing them allows TypeScript to catch type errors.♻️ Proposed fix
- setForm((f) => ({ ...f, maxWorktrees: val === "" ? undefined : Number(val) } as SettingsFormState)); + setForm((f) => ({ ...f, maxWorktrees: val === "" ? undefined : Number(val) }));Apply the same pattern to lines 77 and 106.
Also applies to: 77-77, 106-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/settings/sections/WorktreesSection.tsx` at line 52, Remove the unnecessary "as SettingsFormState" type assertions in the WorktreesSection component: locate the setForm callbacks that spread the previous form state and add/modify maxWorktrees (and the other two similar setForm calls) and delete the trailing "as SettingsFormState" so TypeScript can infer the type from the spread; ensure setForm's signature remains appropriately typed so no further annotations are needed.packages/dashboard/app/components/settings/sections/ResearchGlobalSection.tsx (1)
46-55: ⚖️ Poor tradeoffData duplication risk: provider and limit values written to multiple form keys.
The
setSearchProviderfunction (lines 46-55) writes the provider value to bothresearchGlobalWebSearchProviderandresearchGlobalDefaults.searchProvider. Similarly, the max sources input handler (lines 159-167) writes to bothresearchGlobalMaxSourcesPerRunandresearchGlobalDefaults.maxSourcesPerRun.If other code updates one field without updating its pair, the form state will become inconsistent. Consider whether both representations are necessary, or whether a single source of truth with a computed derived value would be safer.
Also applies to: 159-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/settings/sections/ResearchGlobalSection.tsx` around lines 46 - 55, setSearchProvider (and the max-sources handler) are duplicating state by writing the same value into both the top-level keys (researchGlobalWebSearchProvider, researchGlobalMaxSourcesPerRun) and into researchGlobalDefaults.*; consolidate to a single source of truth by choosing one canonical field (either keep only researchGlobalDefaults.searchProvider/maxSourcesPerRun or keep the top-level keys) and remove the duplicate writes in setSearchProvider and the max-sources handler; update any readers to derive the secondary value (e.g., read from researchGlobalDefaults or compute a fallback) so all setters update only the canonical key and prevent divergence between researchGlobalDefaults and the top-level fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/__tests__/goals-schema.test.ts`:
- Around line 93-95: Update the test case title to match the asserted schema
version: change the it(...) description from "reports schema version 101" to
"reports schema version 109" so the test name aligns with the assertion
expect(db.getSchemaVersion()).toBe(109); and any test-run or search references
use the correct version.
In `@packages/core/src/central-core.ts`:
- Around line 3663-3672: The code calls stripMovedSettingsKeys(payload.global)
and assigns to cleanGlobal but never replaces payload.global, so moved keys can
be resurrected; update the code in CentralCore to assign the cleaned map back
into the payload (e.g., payload.global = cleanGlobal) before returning/handing
payload to the caller and keep globalCount = Object.keys(cleanGlobal).length so
the caller uses the stripped settings (alternatively, change the return to
include the cleaned global), referencing stripMovedSettingsKeys, payload.global,
cleanGlobal and the existing project-path pattern that uses
cleanRemote/updateProject as a model.
In `@packages/core/src/store.ts`:
- Around line 7158-7190: The updateWorkflowSettingValues function performs a
read-modify-write without a transaction causing lost updates; wrap the
read/merge/upsert in a DB transaction so concurrent calls serialize: start a
transaction, select the current "values" for (workflowId, projectId) inside it
(rather than calling getWorkflowSettingValues outside), merge the validated
result.accepted into that snapshot, perform the INSERT ... ON CONFLICT upsert
(using this.db.prepare(...).run) and bumpLastModified, then commit; modify
updateWorkflowSettingValues to use the DB transaction API available on this.db
(and avoid calling getWorkflowSettingValues outside the transaction) so the
whole operation is atomic.
- Around line 11839-11845: The migration is deleting shared global moved keys
while only marking this single project's migration via
markerRow/SETTINGS_MIGRATION_MARKER_KEY/SETTINGS_MIGRATION_VERSION, which causes
other projects to lose their source values; instead, stop removing keys from the
shared global settings store in the moved keys deletion block — snapshot/copy
any required global values into this project's DB and set the per-project marker
(via markerKey) to indicate completion, and defer or gate any global-store
deletions until all projects have been migrated (or move that cleanup to a
separate process); update the code that currently deletes the shared keys to
only write into the project DB and set the per-project marker.
In `@packages/core/src/workflow-settings-resolver.ts`:
- Around line 172-178: When getWorkflowSettingsProjectId() throws, the error
path currently calls effectiveFrom(store, ir, undefined, "") which passes
workflowId=undefined and prevents declarationsFromIr from applying the
BUILTIN_WORKFLOW_SETTINGS fallback; change that call to pass effectiveWorkflowId
(computed earlier) instead of undefined so declarationsFromIr can detect builtin
workflows and return declaration defaults for legacy builtin workflows (update
the return in the catch to call effectiveFrom(store, ir, effectiveWorkflowId,
""), referencing store.getWorkflowSettingsProjectId, effectiveFrom,
declarationsFromIr, and BUILTIN_WORKFLOW_SETTINGS).
In `@packages/dashboard/app/__tests__/settings-save-split.test.ts`:
- Around line 162-181: The test currently checks that githubTrackingDefaultRepo
is not put into onProject.globalPatch but never asserts where it should end up;
update the assertion after creating onProject (the result of calling
splitSettingsSave) to assert the intended routing (most likely that
onProject.projectPatch contains githubTrackingDefaultRepo), e.g. add an
assertion that onProject.projectPatch matches { githubTrackingDefaultRepo:
"org/repo" } (or, if the correct behavior is to drop it, assert that it is
absent from both patches) so the test fully specifies splitSettingsSave's
behavior for the "general" section.
In `@packages/dashboard/app/components/settings/sections/GlobalModelsSection.tsx`:
- Around line 128-154: The visibility check for the "Thinking Effort" selector
is inverted: in the IIFE that computes selectedModel (using availableModels.find
with form.defaultProvider and form.defaultModelId) change the guard so reasoning
models hide the selector; replace the current condition that returns early when
selectedModel.reasoning is false with a condition that returns early when
selectedModel.reasoning is true (i.e., if (selectedModel &&
selectedModel.reasoning) return null) so the selector (bound to
form.defaultThinkingLevel, THINKING_LEVELS and updated via setForm) is shown
only for non-reasoning models and hidden for reasoning models or when no model
is selected.
In `@packages/dashboard/app/components/settings/sections/MergeSection.tsx`:
- Around line 255-270: The select default currently falls back to "auto" via
value={form.directMergeCommitStrategy ?? "auto"}, which conflicts with the
project default of "always-squash"; update the fallback to "always-squash" so
the UI reflects the project default (adjust the expression referencing
directMergeCommitStrategy in MergeSection.tsx), and ensure any initialization or
form population code for form/directMergeCommitStrategy (and the setForm
onChange) remains compatible with the new fallback.
In `@packages/dashboard/src/routes/register-workflow-routes.ts`:
- Around line 257-273: The route handler for
router.get("/workflows/:id/setting-values") currently lets "unknown-workflow"
failures bubble to rethrowAsApiError (causing 500); catch the specific
unknown-workflow condition thrown by resolveSettingDeclarations /
store.getWorkflowSettingValues (e.g. error.name === "UnknownWorkflowError" or
error message includes "unknown-workflow") and convert it into an ApiError with
status 404 before calling rethrowAsApiError; adjust the same pattern in the
analogous handler (the other /workflows/:id block) so both places map the
missing workflow case to new ApiError(404, ...) instead of rethrowing.
In `@packages/engine/src/self-healing.ts`:
- Around line 5019-5023: The loop that calls mergeEffectiveSettings(task) can
throw and currently aborts the entire sweep; wrap the per-task call in a
try/catch around mergeEffectiveSettings(this.store, task, settings) inside the
for (const task of tasks) loop (symbols: mergeEffectiveSettings, maxFixesByTask,
tasks, task.id) so a single-task failure is logged (include task.id and the
error) and the code continues; on error set a safe default in maxFixesByTask
(e.g., 1 or eff.maxPostReviewFixes fallback) or mark the task as skipped so
other tasks are still processed.
---
Outside diff comments:
In `@docs/settings-reference.md`:
- Around line 1049-1063: The example places workflow-only keys
runStepsInNewSessions and maxParallelSteps inside the project "settings" object
which contradicts the moved-keys guidance and makes them no-ops; update the
example by removing runStepsInNewSessions and maxParallelSteps from the project
"settings" block and instead document them as workflow-level settings
(mentioning the keys runStepsInNewSessions and maxParallelSteps) so the example
reflects the correct location and behavior.
In `@packages/engine/src/executor.ts`:
- Around line 8311-8312: fn_review_step is merging review settings using the
captured execute-start "detail" instead of the live task snapshot; update the
merge to use the loaded currentTask (e.g., use currentTask.steps /
currentTask.settings or currentTask.* fields) when computing taskSteps and any
effective review settings so workflow selection changes mid-session resolve from
the latest snapshot; apply the same change to the analogous merge at the other
site referenced (around the block currently using detail at the 8345-8348
region).
---
Nitpick comments:
In `@packages/core/src/__tests__/db-migrate.test.ts`:
- Around line 1007-1054: The test for migrating to schema version 109 is missing
an assertion that the workflow_settings table enforces the expected foreign-key
cascade behavior; update the test (the "adds workflow_settings table when
migrating from schema version 108" case) to query PRAGMA
foreign_key_list(workflow_settings) and assert that the relevant foreign key
row(s) include "ON DELETE" set to "CASCADE" (i.e., the mapping for the parent
column(s) points to CASCADE), ensuring migrated DBs match fresh DB behavior;
keep this assertion alongside the existing checks for table columns, default
values, index (idx_workflow_settings_project), and schema version
(db.getSchemaVersion()).
In `@packages/core/src/__tests__/mission-store.test.ts`:
- Around line 3748-3749: Rename the test title string to match the asserted
schema version: change the it(...) description "schema version is 101 after
migration" to reflect 109 so it matches the expectation in the test where
expect(db.getSchemaVersion()).toBe(109); ensure the updated description
precisely references 109 to avoid misleading failures.
In `@packages/core/src/__tests__/run-audit.test.ts`:
- Around line 586-587: The test description string in the it(...) block is out
of sync with the assertion: update the test title used in the it(...) call
(currently "schema version is bumped to 40") to match the asserted schema
version (109), e.g., change the description to "schema version is bumped to 109"
so the test name aligns with the expect(db.getSchemaVersion()).toBe(109)
assertion.
In `@packages/core/src/__tests__/workflow-ir-settings.test.ts`:
- Around line 209-217: The test only checks parity for
BUILTIN_CODING_WORKFLOW_IR against BUILTIN_WORKFLOW_SETTINGS; extend it to
assert the same invariant for all built-in workflow IR surfaces by iterating
over the collection of built-in workflow IR constants (add the other
BUILTIN_*_WORKFLOW_IR values used in the codebase) and for each one perform the
same checks: build declaredIds from workflow.settings, ensure every entry in
BUILTIN_WORKFLOW_SETTINGS is present, and finally
expect(workflow.settings).toEqual(BUILTIN_WORKFLOW_SETTINGS); update the test
description accordingly and reuse the existing assertions (the current logic
around BUILTIN_CODING_WORKFLOW_IR and BUILTIN_WORKFLOW_SETTINGS) for each
built-in workflow IR.
In `@packages/dashboard/app/components/settings/save-split.ts`:
- Around line 78-86: The current isGlobalSettingsKey branch may assign undefined
to globalPatch when both value and initialValue are undefined; change the logic
in the block (inside isGlobalSettingsKey) so you only write to globalPatch when
necessary: if value === undefined and initialValue !== undefined set
(globalPatch as Record<string, unknown>)[key] = null; else if value !==
undefined set (globalPatch as Record<string, unknown>)[key] = value; otherwise
do not set the key. Use the existing identifiers isGlobalSettingsKey,
initialValues, globalPatch, key, value, and initialValue to locate and update
the code.
In `@packages/dashboard/app/components/settings/sections/ExperimentalSection.tsx`:
- Around line 34-43: The local variable experimentalFeatures introduces
potential confusion with form.experimentalFeatures; remove the const
experimentalFeatures = form.experimentalFeatures ?? {} and instead use
form.experimentalFeatures ?? {} inline where used (notably in the allFeatureKeys
map and the isFeatureEnabled call) so the rendering and the toggle handler
reference the same expression; update any references to experimentalFeatures
(e.g., in the featureFlags mapping and any subsequent uses) to use
form.experimentalFeatures ?? {} to keep the pattern consistent with the toggle
handler that already uses form.experimentalFeatures.
In
`@packages/dashboard/app/components/settings/sections/ResearchGlobalSection.tsx`:
- Around line 46-55: setSearchProvider (and the max-sources handler) are
duplicating state by writing the same value into both the top-level keys
(researchGlobalWebSearchProvider, researchGlobalMaxSourcesPerRun) and into
researchGlobalDefaults.*; consolidate to a single source of truth by choosing
one canonical field (either keep only
researchGlobalDefaults.searchProvider/maxSourcesPerRun or keep the top-level
keys) and remove the duplicate writes in setSearchProvider and the max-sources
handler; update any readers to derive the secondary value (e.g., read from
researchGlobalDefaults or compute a fallback) so all setters update only the
canonical key and prevent divergence between researchGlobalDefaults and the
top-level fields.
In `@packages/dashboard/app/components/settings/sections/SchedulingSection.tsx`:
- Line 77: Remove the unnecessary "as SettingsFormState" type assertions on the
setForm updates; locate the setForm calls (e.g., the one updating maxConcurrent
and the other two similar setters) and simply return the spread object ({ ...f,
... }) without casting so TypeScript can infer the correct type and catch real
mismatches in functions like setForm and the SettingsFormState usage.
In `@packages/dashboard/app/components/settings/sections/WorktreesSection.tsx`:
- Line 265: The small tag in the WorktreesSection component is using an inline
style for the error color; replace style={{ color: "var(--color-error)" }} with
the existing CSS utility class (e.g., className="text-error") so it matches the
codebase conventions — update the small element that renders
worktrunkInstall.error (in WorktreesSection.tsx) to use className="text-error"
instead of the inline style.
- Line 52: Remove the unnecessary "as SettingsFormState" type assertions in the
WorktreesSection component: locate the setForm callbacks that spread the
previous form state and add/modify maxWorktrees (and the other two similar
setForm calls) and delete the trailing "as SettingsFormState" so TypeScript can
infer the type from the spread; ensure setForm's signature remains appropriately
typed so no further annotations are needed.
In `@packages/dashboard/app/components/settings/SettingsSelectRow.tsx`:
- Around line 38-50: The select sets value to value ?? "" but doesn't ensure an
explicit empty <option> exists, causing the first option to appear when value is
null; update SettingsSelectRow to render an explicit empty option (e.g., an
<option value="">—</option>) when value is null or when descriptor.options does
not include an empty value so the UI matches the controlled value and users can
clear to null, and ensure onChange continues to call onChange(e.target.value)
(or convert "" back to null before calling if your prop expects null) so the
component and descriptor.options handling remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af586643-9945-46db-a99b-c139cb2a73fe
📒 Files selected for processing (127)
.changeset/workflow-settings-mechanism.mdCONCEPTS.mddocs/plans/2026-06-04-002-feat-workflow-settings-mechanism-plan.mddocs/residual-review-findings/gsxdsm-cleanupsettings.mddocs/settings-reference.mddocs/workflow-steps.mdpackages/cli/skill/fusion/references/engine-tools.mdpackages/cli/src/commands/__tests__/settings.test.tspackages/cli/src/commands/__tests__/task-lifecycle.test.tspackages/cli/src/commands/settings-import.tspackages/cli/src/commands/settings.tspackages/cli/src/commands/task-lifecycle.tspackages/core/src/__tests__/db-migrate.test.tspackages/core/src/__tests__/db.test.tspackages/core/src/__tests__/goals-schema.test.tspackages/core/src/__tests__/insight-store.test.tspackages/core/src/__tests__/merge-request-record.test.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/__tests__/run-audit.test.tspackages/core/src/__tests__/settings-consistency.test.tspackages/core/src/__tests__/settings-export.test.tspackages/core/src/__tests__/settings-migration.test.tspackages/core/src/__tests__/settings-parity.test.tspackages/core/src/__tests__/store-merge-queue.test.tspackages/core/src/__tests__/store-settings.test.tspackages/core/src/__tests__/task-creation-hook.test.tspackages/core/src/__tests__/task-documents.test.tspackages/core/src/__tests__/workflow-ir-settings.test.tspackages/core/src/__tests__/workflow-settings-e2e.test.tspackages/core/src/__tests__/workflow-settings-resolver.test.tspackages/core/src/__tests__/workflow-settings.test.tspackages/core/src/builtin-coding-workflow-ir.tspackages/core/src/builtin-stepwise-coding-workflow-ir.tspackages/core/src/builtin-workflow-settings.tspackages/core/src/builtin-workflows.tspackages/core/src/central-core.tspackages/core/src/db.tspackages/core/src/index.tspackages/core/src/moved-settings.tspackages/core/src/settings-export.tspackages/core/src/settings-schema.tspackages/core/src/store.tspackages/core/src/workflow-ir-types.tspackages/core/src/workflow-ir.tspackages/core/src/workflow-settings-resolver.tspackages/core/src/workflow-settings.tspackages/dashboard/app/__tests__/settings-moved-keys.test.tspackages/dashboard/app/__tests__/settings-primitives.test.tsxpackages/dashboard/app/__tests__/settings-save-split.test.tspackages/dashboard/app/__tests__/settings-sections.test.tsxpackages/dashboard/app/api/legacy.tspackages/dashboard/app/components/AppModals.tsxpackages/dashboard/app/components/SettingsModal.csspackages/dashboard/app/components/SettingsModal.tsxpackages/dashboard/app/components/WorkflowNodeEditor.tsxpackages/dashboard/app/components/WorkflowSettingsPanel.csspackages/dashboard/app/components/WorkflowSettingsPanel.tsxpackages/dashboard/app/components/__tests__/SettingsModal.test.tsxpackages/dashboard/app/components/__tests__/WorkflowSettingsPanel.test.tsxpackages/dashboard/app/components/settings/SettingsFieldRow.csspackages/dashboard/app/components/settings/SettingsFieldRow.tsxpackages/dashboard/app/components/settings/SettingsNumberRow.csspackages/dashboard/app/components/settings/SettingsNumberRow.tsxpackages/dashboard/app/components/settings/SettingsSection.csspackages/dashboard/app/components/settings/SettingsSection.tsxpackages/dashboard/app/components/settings/SettingsSelectRow.csspackages/dashboard/app/components/settings/SettingsSelectRow.tsxpackages/dashboard/app/components/settings/SettingsTextRow.csspackages/dashboard/app/components/settings/SettingsTextRow.tsxpackages/dashboard/app/components/settings/SettingsTextareaRow.csspackages/dashboard/app/components/settings/SettingsTextareaRow.tsxpackages/dashboard/app/components/settings/SettingsToggleRow.csspackages/dashboard/app/components/settings/SettingsToggleRow.tsxpackages/dashboard/app/components/settings/index.tspackages/dashboard/app/components/settings/save-split.tspackages/dashboard/app/components/settings/sections/AgentPermissionsSection.tsxpackages/dashboard/app/components/settings/sections/AppearanceSection.tsxpackages/dashboard/app/components/settings/sections/AuthenticationSection.tsxpackages/dashboard/app/components/settings/sections/BackupsSection.tsxpackages/dashboard/app/components/settings/sections/CommandsSection.tsxpackages/dashboard/app/components/settings/sections/ExperimentalSection.tsxpackages/dashboard/app/components/settings/sections/GeneralSection.tsxpackages/dashboard/app/components/settings/sections/GlobalGeneralSection.tsxpackages/dashboard/app/components/settings/sections/GlobalModelsSection.tsxpackages/dashboard/app/components/settings/sections/MemorySection.tsxpackages/dashboard/app/components/settings/sections/MergeSection.tsxpackages/dashboard/app/components/settings/sections/MovedSettingsStub.csspackages/dashboard/app/components/settings/sections/MovedSettingsStub.tsxpackages/dashboard/app/components/settings/sections/NodeRoutingSection.tsxpackages/dashboard/app/components/settings/sections/NodeSyncSection.tsxpackages/dashboard/app/components/settings/sections/NotificationsSection.tsxpackages/dashboard/app/components/settings/sections/PluginsSection.tsxpackages/dashboard/app/components/settings/sections/ProjectModelsSection.tsxpackages/dashboard/app/components/settings/sections/PromptsSection.tsxpackages/dashboard/app/components/settings/sections/RemoteSection.tsxpackages/dashboard/app/components/settings/sections/ResearchGlobalSection.tsxpackages/dashboard/app/components/settings/sections/ResearchProjectSection.tsxpackages/dashboard/app/components/settings/sections/RuntimesSections.tsxpackages/dashboard/app/components/settings/sections/ScheduledEvalsSection.tsxpackages/dashboard/app/components/settings/sections/SchedulingSection.tsxpackages/dashboard/app/components/settings/sections/SecretsSection.tsxpackages/dashboard/app/components/settings/sections/WorktreesSection.tsxpackages/dashboard/app/components/settings/sections/context.tspackages/dashboard/app/components/settings/types.tspackages/dashboard/app/components/workflow-flow-mapping.tspackages/dashboard/app/hooks/useModalManager.tspackages/dashboard/src/__tests__/routes-nodes-sync.test.tspackages/dashboard/src/__tests__/workflow-routes.test.tspackages/dashboard/src/routes/register-settings-memory-routes.tspackages/dashboard/src/routes/register-settings-sync-inbound-routes.tspackages/dashboard/src/routes/register-settings-sync-routes.tspackages/dashboard/src/routes/register-workflow-routes.tspackages/dashboard/vitest.config.tspackages/engine/src/__tests__/agent-tools-workflow-settings.test.tspackages/engine/src/__tests__/effective-settings-merge.test.tspackages/engine/src/__tests__/effective-settings-model-lane.test.tspackages/engine/src/__tests__/workflow-settings-fallback-alignment.test.tspackages/engine/src/agent-tools.tspackages/engine/src/effective-settings.tspackages/engine/src/executor.tspackages/engine/src/merger.tspackages/engine/src/mission-execution-loop.tspackages/engine/src/reviewer.tspackages/engine/src/self-healing.tspackages/engine/src/triage.tspackages/i18n/locales/en/app.jsonpackages/plugin-sdk/src/index.ts
…109 sweep (plugins/ not covered by packages/ grep)
…union executor/agent-tools/routes imports, both tool-description texts, both route helpers
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/solutions/test-failures/schema-version-sweep-must-include-plugin-workspaces.md (1)
35-39: 💤 Low valueConsider adding a language identifier to the fenced code block.
The fenced code block containing the test failure output lacks a language identifier. While this is a terminal output rather than source code, adding
textorshellwould satisfy the linter and improve clarity.📝 Suggested improvement
-``` +```text FAIL plugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts RoadmapStore > schema version > schema version is 108 after init AssertionError: expected 109 to be 108</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/solutions/test-failures/schema-version-sweep-must-include-plugin-workspaces.md
around lines 35 - 39, Update the fenced code block that shows the test failure
output by adding a language identifier (e.g., text or shell) after the opening
backticks so the linter recognizes it — locate the fenced block containing "FAIL
plugins/fusion-plugin-roadmap/src/store/tests/roadmap-store.test.ts" and
change the openingtotext (or ```shell).</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Nitpick comments:
In
@docs/solutions/test-failures/schema-version-sweep-must-include-plugin-workspaces.md:
- Around line 35-39: Update the fenced code block that shows the test failure
output by adding a language identifier (e.g., text or shell) after the opening
backticks so the linter recognizes it — locate the fenced block containing "FAIL
plugins/fusion-plugin-roadmap/src/store/tests/roadmap-store.test.ts" and
change the openingtotext (or ```shell).</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `cd8c754b-b748-4281-a868-317f91c7770f` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 06f6a0aa28a38941f9149c537b71e767181b622c and b7fa21c25e916ab82ae49af02b717de80748a0ff. </details> <details> <summary>📒 Files selected for processing (14)</summary> * `CONCEPTS.md` * `docs/solutions/test-failures/schema-version-sweep-must-include-plugin-workspaces.md` * `docs/workflow-steps.md` * `packages/core/src/index.ts` * `packages/core/src/workflow-ir-types.ts` * `packages/core/src/workflow-ir.ts` * `packages/dashboard/app/components/WorkflowNodeEditor.tsx` * `packages/dashboard/app/components/workflow-flow-mapping.ts` * `packages/dashboard/src/__tests__/workflow-routes.test.ts` * `packages/dashboard/src/routes/register-workflow-routes.ts` * `packages/engine/src/agent-tools.ts` * `packages/engine/src/executor.ts` * `packages/i18n/locales/en/app.json` * `packages/plugin-sdk/src/index.ts` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * docs/workflow-steps.md * packages/i18n/locales/en/app.json </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (10)</summary> * packages/plugin-sdk/src/index.ts * packages/core/src/workflow-ir-types.ts * packages/core/src/index.ts * packages/dashboard/app/components/workflow-flow-mapping.ts * packages/dashboard/src/routes/register-workflow-routes.ts * packages/dashboard/src/__tests__/workflow-routes.test.ts * packages/core/src/workflow-ir.ts * packages/dashboard/app/components/WorkflowNodeEditor.tsx * packages/engine/src/agent-tools.ts * packages/engine/src/executor.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…umber workflow_settings migration to 110, settings panel joins the sidebar disclosures, full-workspace literal sweep
gsxdsm
force-pushed
the
gsxdsm/cleanupsettings
branch
from
June 5, 2026 22:24
4803760 to
6d16c07
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts (1)
746-749: ⚡ Quick winImport
SCHEMA_VERSIONinstead of hard-coding110.This assertion is supposed to mirror core's exported schema contract, so the literal will force a plugin-side edit on every core migration even when nothing in the plugin is wrong. Importing
SCHEMA_VERSIONkeeps the test coupled to the real source of truth.♻️ Proposed refactor
-import { Database, createDatabase } from "`@fusion/core`"; +import { Database, SCHEMA_VERSION, createDatabase } from "`@fusion/core`"; ... - expect(db.getSchemaVersion()).toBe(110); + expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts` around lines 746 - 749, Replace the hard-coded literal 110 in the test assertion with the canonical SCHEMA_VERSION export from core: add an import for SCHEMA_VERSION (the exported symbol from `@fusion/core` or the core module used by this plugin) at the top of the test file and change expect(db.getSchemaVersion()).toBe(110) to expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION) so the test tracks core's actual schema constant; update the import list in roadmap-store.test.ts to include SCHEMA_VERSION and run tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/__tests__/db-migrate.test.ts`:
- Around line 1114-1121: Rename and update the test description and inline
comment that reference migration 109 so they match the asserted schema state
110: change the it(...) title from "migration 109 is idempotent on re-init" to
something like "migration 110 is idempotent on re-init" and update the comment
that says "already at 109" to reflect "already at 110" (the test body uses
Database, db.init(), db.getSchemaVersion() and reopened variables), so the
wording aligns with the expect(db.getSchemaVersion()).toBe(110) assertion.
In `@packages/core/src/__tests__/run-audit.test.ts`:
- Around line 586-587: The test description text and its assertion are
inconsistent: the it(...) description says "schema version is bumped to 109" but
the assertion calls db.getSchemaVersion() and expects 110; update the test
description string in the it block to reflect the expected value (e.g., change
"schema version is bumped to 109" to "schema version is bumped to 110") so it
matches the assertion using db.getSchemaVersion().
In `@packages/core/src/store.ts`:
- Around line 13627-13639: The migration currently reads the default with
getDefaultWorkflowId() before attempting to set it, which can race with other
writers; modify the write path around
setDefaultWorkflowId(result.combinedWorkflowId) so it becomes a compare-and-set:
inside the same locked/try block where you call setDefaultWorkflowId (the path
that logs "migrateLegacyWorkflowSteps:set-default"), re-read the current default
via getDefaultWorkflowId(), and only call setDefaultWorkflowId if that re-read
is still falsy (or equals the expected old value); if it changed, skip the write
and log/return accordingly to avoid overwriting a concurrent setter. Ensure you
keep the existing error handling via storeLog.warn and preserve the
combinedWorkflowId in the log.
---
Nitpick comments:
In `@plugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts`:
- Around line 746-749: Replace the hard-coded literal 110 in the test assertion
with the canonical SCHEMA_VERSION export from core: add an import for
SCHEMA_VERSION (the exported symbol from `@fusion/core` or the core module used by
this plugin) at the top of the test file and change
expect(db.getSchemaVersion()).toBe(110) to
expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION) so the test tracks core's
actual schema constant; update the import list in roadmap-store.test.ts to
include SCHEMA_VERSION and run tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 985d1da4-e95a-4e8e-89f9-9a208d94423a
📒 Files selected for processing (26)
CONCEPTS.mdpackages/cli/skill/fusion/references/engine-tools.mdpackages/core/src/__tests__/db-migrate.test.tspackages/core/src/__tests__/db.test.tspackages/core/src/__tests__/goals-schema.test.tspackages/core/src/__tests__/insight-store.test.tspackages/core/src/__tests__/merge-request-record.test.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/__tests__/run-audit.test.tspackages/core/src/__tests__/store-merge-queue.test.tspackages/core/src/__tests__/task-documents.test.tspackages/core/src/builtin-workflows.tspackages/core/src/db.tspackages/core/src/index.tspackages/core/src/store.tspackages/core/src/workflow-ir.tspackages/dashboard/app/api/legacy.tspackages/dashboard/app/components/AppModals.tsxpackages/dashboard/app/components/WorkflowNodeEditor.tsxpackages/dashboard/app/components/workflow-flow-mapping.tspackages/dashboard/app/hooks/useModalManager.tspackages/dashboard/src/routes/register-workflow-routes.tspackages/engine/src/agent-tools.tspackages/engine/src/executor.tspackages/i18n/locales/en/app.jsonplugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts
💤 Files with no reviewable changes (2)
- packages/dashboard/app/components/AppModals.tsx
- packages/dashboard/app/hooks/useModalManager.ts
✅ Files skipped from review due to trivial changes (4)
- packages/core/src/tests/mission-store.test.ts
- packages/i18n/locales/en/app.json
- CONCEPTS.md
- packages/core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/core/src/tests/merge-request-record.test.ts
- packages/core/src/builtin-workflows.ts
- packages/engine/src/agent-tools.ts
- packages/dashboard/src/routes/register-workflow-routes.ts
- packages/dashboard/app/api/legacy.ts
- packages/dashboard/app/components/workflow-flow-mapping.ts
- packages/dashboard/app/components/WorkflowNodeEditor.tsx
- packages/engine/src/executor.ts
- packages/cli/skill/fusion/references/engine-tools.md
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts (1)
746-749: ⚡ Quick winImport
SCHEMA_VERSIONinstead of hard-coding110.This assertion is supposed to mirror core's exported schema contract, so the literal will force a plugin-side edit on every core migration even when nothing in the plugin is wrong. Importing
SCHEMA_VERSIONkeeps the test coupled to the real source of truth.♻️ Proposed refactor
-import { Database, createDatabase } from "`@fusion/core`"; +import { Database, SCHEMA_VERSION, createDatabase } from "`@fusion/core`"; ... - expect(db.getSchemaVersion()).toBe(110); + expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts` around lines 746 - 749, Replace the hard-coded literal 110 in the test assertion with the canonical SCHEMA_VERSION export from core: add an import for SCHEMA_VERSION (the exported symbol from `@fusion/core` or the core module used by this plugin) at the top of the test file and change expect(db.getSchemaVersion()).toBe(110) to expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION) so the test tracks core's actual schema constant; update the import list in roadmap-store.test.ts to include SCHEMA_VERSION and run tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/__tests__/db-migrate.test.ts`:
- Around line 1114-1121: Rename and update the test description and inline
comment that reference migration 109 so they match the asserted schema state
110: change the it(...) title from "migration 109 is idempotent on re-init" to
something like "migration 110 is idempotent on re-init" and update the comment
that says "already at 109" to reflect "already at 110" (the test body uses
Database, db.init(), db.getSchemaVersion() and reopened variables), so the
wording aligns with the expect(db.getSchemaVersion()).toBe(110) assertion.
In `@packages/core/src/__tests__/run-audit.test.ts`:
- Around line 586-587: The test description text and its assertion are
inconsistent: the it(...) description says "schema version is bumped to 109" but
the assertion calls db.getSchemaVersion() and expects 110; update the test
description string in the it block to reflect the expected value (e.g., change
"schema version is bumped to 109" to "schema version is bumped to 110") so it
matches the assertion using db.getSchemaVersion().
In `@packages/core/src/store.ts`:
- Around line 13627-13639: The migration currently reads the default with
getDefaultWorkflowId() before attempting to set it, which can race with other
writers; modify the write path around
setDefaultWorkflowId(result.combinedWorkflowId) so it becomes a compare-and-set:
inside the same locked/try block where you call setDefaultWorkflowId (the path
that logs "migrateLegacyWorkflowSteps:set-default"), re-read the current default
via getDefaultWorkflowId(), and only call setDefaultWorkflowId if that re-read
is still falsy (or equals the expected old value); if it changed, skip the write
and log/return accordingly to avoid overwriting a concurrent setter. Ensure you
keep the existing error handling via storeLog.warn and preserve the
combinedWorkflowId in the log.
---
Nitpick comments:
In `@plugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts`:
- Around line 746-749: Replace the hard-coded literal 110 in the test assertion
with the canonical SCHEMA_VERSION export from core: add an import for
SCHEMA_VERSION (the exported symbol from `@fusion/core` or the core module used by
this plugin) at the top of the test file and change
expect(db.getSchemaVersion()).toBe(110) to
expect(db.getSchemaVersion()).toBe(SCHEMA_VERSION) so the test tracks core's
actual schema constant; update the import list in roadmap-store.test.ts to
include SCHEMA_VERSION and run tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 985d1da4-e95a-4e8e-89f9-9a208d94423a
📒 Files selected for processing (26)
CONCEPTS.mdpackages/cli/skill/fusion/references/engine-tools.mdpackages/core/src/__tests__/db-migrate.test.tspackages/core/src/__tests__/db.test.tspackages/core/src/__tests__/goals-schema.test.tspackages/core/src/__tests__/insight-store.test.tspackages/core/src/__tests__/merge-request-record.test.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/__tests__/run-audit.test.tspackages/core/src/__tests__/store-merge-queue.test.tspackages/core/src/__tests__/task-documents.test.tspackages/core/src/builtin-workflows.tspackages/core/src/db.tspackages/core/src/index.tspackages/core/src/store.tspackages/core/src/workflow-ir.tspackages/dashboard/app/api/legacy.tspackages/dashboard/app/components/AppModals.tsxpackages/dashboard/app/components/WorkflowNodeEditor.tsxpackages/dashboard/app/components/workflow-flow-mapping.tspackages/dashboard/app/hooks/useModalManager.tspackages/dashboard/src/routes/register-workflow-routes.tspackages/engine/src/agent-tools.tspackages/engine/src/executor.tspackages/i18n/locales/en/app.jsonplugins/fusion-plugin-roadmap/src/store/__tests__/roadmap-store.test.ts
💤 Files with no reviewable changes (2)
- packages/dashboard/app/components/AppModals.tsx
- packages/dashboard/app/hooks/useModalManager.ts
✅ Files skipped from review due to trivial changes (4)
- packages/core/src/tests/mission-store.test.ts
- packages/i18n/locales/en/app.json
- CONCEPTS.md
- packages/core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/core/src/tests/merge-request-record.test.ts
- packages/core/src/builtin-workflows.ts
- packages/engine/src/agent-tools.ts
- packages/dashboard/src/routes/register-workflow-routes.ts
- packages/dashboard/app/api/legacy.ts
- packages/dashboard/app/components/workflow-flow-mapping.ts
- packages/dashboard/app/components/WorkflowNodeEditor.tsx
- packages/engine/src/executor.ts
- packages/cli/skill/fusion/references/engine-tools.md
🛑 Comments failed to post (3)
packages/core/src/__tests__/db-migrate.test.ts (1)
1114-1121:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the idempotency test wording to match the asserted schema state.
The test name/comment says “migration 109” and “already at 109”, but the assertions validate schema
110. Please align wording to avoid migration-debug confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/db-migrate.test.ts` around lines 1114 - 1121, Rename and update the test description and inline comment that reference migration 109 so they match the asserted schema state 110: change the it(...) title from "migration 109 is idempotent on re-init" to something like "migration 110 is idempotent on re-init" and update the comment that says "already at 109" to reflect "already at 110" (the test body uses Database, db.init(), db.getSchemaVersion() and reopened variables), so the wording aligns with the expect(db.getSchemaVersion()).toBe(110) assertion.packages/core/src/__tests__/run-audit.test.ts (1)
586-587:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate test name to match the assertion.
The test name says "bumped to 109" but the assertion expects
110.📝 Proposed fix
- it("schema version is bumped to 109", () => { + it("schema version is bumped to 110", () => { expect(db.getSchemaVersion()).toBe(110); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.it("schema version is bumped to 110", () => { expect(db.getSchemaVersion()).toBe(110); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/run-audit.test.ts` around lines 586 - 587, The test description text and its assertion are inconsistent: the it(...) description says "schema version is bumped to 109" but the assertion calls db.getSchemaVersion() and expects 110; update the test description string in the it block to reflect the expected value (e.g., change "schema version is bumped to 109" to "schema version is bumped to 110") so it matches the assertion using db.getSchemaVersion().packages/core/src/store.ts (1)
13627-13639:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the migrated-default write a real compare-and-set.
The
getDefaultWorkflowId()check happens before the write, so another writer can setdefaultWorkflowIdin between and this migration will overwrite that newer choice. Re-check inside the locked write path before persisting.Suggested fix
if (result.combinedWorkflowId) { - const currentDefaultId = await this.getDefaultWorkflowId(); - if (!currentDefaultId) { - try { - await this.setDefaultWorkflowId(result.combinedWorkflowId); - } catch (err) { - storeLog.warn("Failed to set migrated combined workflow as project default", { - phase: "migrateLegacyWorkflowSteps:set-default", - combinedWorkflowId: result.combinedWorkflowId, - error: err instanceof Error ? err.message : String(err), - }); - } + try { + await this.withConfigLock(async () => { + const config = this.readConfigFast(); + const currentDefaultId = + (config.settings as { defaultWorkflowId?: string } | undefined)?.defaultWorkflowId?.trim(); + if (currentDefaultId) return; + + config.settings = { + ...(config.settings ?? {}), + defaultWorkflowId: result.combinedWorkflowId, + } as Settings; + await this.writeConfig(config); + }); + } catch (err) { + storeLog.warn("Failed to set migrated combined workflow as project default", { + phase: "migrateLegacyWorkflowSteps:set-default", + combinedWorkflowId: result.combinedWorkflowId, + error: err instanceof Error ? err.message : String(err), + }); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/store.ts` around lines 13627 - 13639, The migration currently reads the default with getDefaultWorkflowId() before attempting to set it, which can race with other writers; modify the write path around setDefaultWorkflowId(result.combinedWorkflowId) so it becomes a compare-and-set: inside the same locked/try block where you call setDefaultWorkflowId (the path that logs "migrateLegacyWorkflowSteps:set-default"), re-read the current default via getDefaultWorkflowId(), and only call setDefaultWorkflowId if that re-read is still falsy (or equals the expected old value); if it changed, skip the write and log/return accordingly to avoid overwriting a concurrent setter. Ensure you keep the existing error handling via storeLog.warn and preserve the combinedWorkflowId in the log.
…migration to 112 behind main's cli_sessions(110)/adapter(111), full-workspace literal sweep, i18n union
- applyRemoteSettings now applies the stripped global payload (moved keys can't resurrect) - resolver error path keeps effective workflowId so builtin fallback survives identity failure - updateWorkflowSettingValues read-merge-upsert wrapped in transactionImmediate (lost-update race) - MergeSection directMergeCommitStrategy UI fallback aligned to schema default (always-squash) - save-split section-routing test asserts the positive case - setting-values routes 404 on unknown workflow ids (+ tests)
Security Review - PASS WITH ADVISORIESScan Date: 2026-06-06 01:29:44 UTC SummaryAPPROVED with non-blocking advisories Static Security Scan Results
Code Security AnalysisNew Database Table: workflow_settings
API Endpoints Added:
Validation Layer:
Change Metrics
CI StatusAll checks passing (Build, Lint, Typecheck, Gate, CodeRabbit) Non-Blocking Advisories
RecommendationSAFE TO MERGE - Well-architected feature with proper validation, comprehensive tests, and documented edge cases. Automated security scan - Part of CTO agent loop |
Security Review - PASS WITH NOTESScan Date: 2026-06-05 Security Checks PerformedPASS - Hardcoded Secrets: No credentials found Detailed Analysisexec() Usage Review:
All exec() patterns reviewed are legitimate and safe:
Changes Overview (24,456 lines):
Risk Level: LOW No security vulnerabilities detected. The exec() patterns are safe database and regex operations. |
This was referenced Jun 8, 2026
This was referenced Jul 16, 2026
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Workflows can now carry their own typed settings. Step-execution policy, review/approval gates, and per-phase model lanes — previously ambient project settings with no relationship to the workflow that consumes them — are declared on the workflow IR and resolved per task at engine entry. A one-time migration hard-moves 30 keys out of
DEFAULT_PROJECT_SETTINGSinto per-(workflowId, projectId)values, and the Settings modal is rebuilt from a 7,883-line monolith into a 2,905-line shell composing ~25 section components.Plan:
docs/plans/2026-06-04-002-feat-workflow-settings-mechanism-plan.md(all R1–R11 / U1–U10 shipped).What changed
WorkflowIrV2.settings: WorkflowSettingDefinition[]mirroring the custom-task-fields pattern;validateSettingsinparseWorkflowIr; built-ins declare the full moved-key catalog with defaults byte-equal to the legacy literals (the migration parity anchor)workflow_settingstable (schema v109), single validating write authority with typed rejections; built-in workflows accept values while their declarations stay non-editable; cascade-delete on workflow removalresolveEffectiveSettings(never-throw, drop-on-orphan) merged two-tier at every engine entry: a stored workflow value overrides the base; a declaration default only fills keys absent from the base — untuned projects are behavior-identicalbuiltin:coding), nulls raw keys in one transaction, then removes the keys from the schema. Tombstone allowlist (MOVED_SETTINGS_KEYS) shields sync diffs, v1 imports, and stale writers from resurrecting moved keysworkflowSettingssection + v1-upgrade import), cross-node sync filtering with an explicit "not synced yet" note, CLI redirect hints,fn_workflow_settingsagent tool (get/set with stored/effective/orphaned), plugin-sdk typesKey decisions
DEFAULT_*values after deletion).completionDocumentationMode(triage-scope reader),buildTimeoutMs,reflectionIntervalMs/reflectionAfterTask(no engine readers).Testing
?? literalequals its declaration default), migration scenarios (mixed pinning, unset default, crash convergence, double-run, stale writers), export v2 round-trip + v1 upgrade, consistency guard, moved-keys DOM sweep across all section components, save-split characterization, panel/route/agent-tool parity, and an end-to-end journey (customize → migrate → engine parity → edit → export → wipe → import).pnpm lintandtsc --noEmitgreen across core/engine/dashboard/cli; multi-agent code review (13 reviewers) applied 11 safe fixes on-branch.Residual Review Findings
Hardening follow-ups from code review, filed as issues (none block merge): #1434 (migration identity-keying), #1435 (delete-cascade transactionality), #1436 (v1-import overwrite semantics), #1437 (in-flight edit clobber), #1438 (merger getTask race), #1439 (sweep over-resolution), #1440 (rejection-shape divergence), #1441 (push-path tombstone filter), #1442 (silent migration-failure signal), #1443 (custom-workflow declaration gap), #1444 (test-helper dedup). Full record:
docs/residual-review-findings/gsxdsm-cleanupsettings.md.Post-Deploy Monitoring & Validation
SELECT value FROM __meta WHERE key = 'settingsMigrationVersion'should be1per project; raw project/global settings JSON should contain noMOVED_SETTINGS_KEYSentries;SELECT workflowId, projectId, json_valid("values") FROM workflow_settingsall valid.settings-exportstructured warns (dropped import values) and the store-open migration warn (task-storelogger) — repeated migration warnings on the same project are the failure signal (see Repeated silent settings-migration failure leaves a project un-migrated with no surfaced signal #1442).fn_workflow_settings getor the editor's Values tab).workflow_settingsrows before reverting; a binary downgrade runs moved policy at defaults (documented forward-only posture).CI Failures Unresolved
--shard=2/2step dies silently on the CI runner (stdout truncates mid-run with no vitest summary; same signature on two consecutive runs). Pre-existing onmain: the run for merge-base e5bab64 (PR 1385 merge) fails the same job (along with shards 1/4 and 4/4 and the curated-gate guard there) — see https://github.com/Runfusion/Fusion/actions/runs/26990133210. The identical shard passes locally on this branch: 269 files / 3,275 tests. Latest failing run: https://github.com/Runfusion/Fusion/actions/runs/27006791180/job/79700270028. No code change in this branch can address it; it needs a CI-runner investigation on main (likely the known engine-runner memory/SIGSEGV class).Summary by CodeRabbit
New Features
Tests
Documentation