feat: step inversion — steps as workflow-modelable nodes (foreach/step-review/parse-steps/code, parallel execution, custom task fields) - #1424
Merged
Conversation
…k edges, dependsOn parsing (FN step-inversion) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…th blast-radius guard (RETHINK extraction) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s.customFields), instance CRUD trio, literal sweep Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…arse-steps/code, rework edge inspector, template round-trip Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…unded rework cycles, step-execute seam Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-delete reconciliation, coerce gate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-source projection discipline, pluggable step-parser registry - step-review node: reviewStep seam, verdict→outcome edges, UNAVAILABLE limiter, rethink reset-on-rework, split-branch advisory-only - updateStep source:'graph': dependency-order done guard, audit-loud suppression, auto-reinit bypass; projection-first ordering - runGraphTaskStep: per-step step-session physics pinned for graph-owned runs (closes U3 interim) - step-parsers.ts registry (step-headings byte-identical move + json-steps), store delegates via registry Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ction, card badges, PATCH route, board-workflows fields payload, TUI chips) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ter, code node runner (esbuild + child-process harness) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h live badge preview (U13 completion) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r-instance worktrees, ordered integration, conflict→rework (KTD-11) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… parity & invariant suite; agent-tool surfaces for custom fields and IR authoring Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ype exports, skill doc, step-inversion docs + changeset Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… lint cleanup of dead helpers Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rktree-leak cleanup, rework re-execution, instance pruning, SIGKILL fallback, type dedup, board memo split, agent-native field-schema context + fn_workflow_get 17 findings from 12-reviewer code review applied: - P1 runId trio (pin-probe/resume/markIntegrated used placeholder runId; 4-reviewer corroboration) + production-wiring tests - P1 worktree/branch release on instance failure/exhaustion/abort - P2 runGraphTaskStep no longer masks step-session failures; rejected memo cleared so rework re-executes - P2 clearStaleInstanceStates wired at run start/end (mirrors branch pruning) - P2 code-node timeout killSignal SIGKILL; dead template-recursion removed - P1/P2 field-type re-declarations replaced with @fusion/core imports (stale comments removed) - P2 Board memo split + TaskCard comparator stringify guard + modal prop-driven field defs - HIGH agent-native: executor prompt injects custom-field schema/values; self-correcting rejection text; fn_workflow_get; fn_task_update bare-call guard; integration-conflict task log Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…path (foreach group + template children + rework edges) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughAdds workflow IR v2 step-inversion (parse-steps/foreach/step-review/code), per-instance persistence and integration (schema v108), code-node runner, parser registry and plugin adapters, engine executor wiring, dashboard/editor/CLI surfaces, many tests, locales, and docs. ChangesStep inversion, persistence, engine, and UI
Sequence Diagram(s)sequenceDiagram
participant Client
participant DashboardAPI
participant Store
participant Executor
participant Git
Client->>DashboardAPI: PATCH /tasks/:id/custom-fields
DashboardAPI->>Store: updateTaskCustomFields(patch)
Store-->>DashboardAPI: Task (customFields merged)
DashboardAPI-->>Client: 200 OK (Task)
Client->>Executor: Run task (graph)
Executor->>Store: load workflow IR/fields
Executor->>Git: allocate worktree (parallel mode)
Executor->>Executor: foreach expand instances
Executor->>Executor: step-execute (capture baseline/checkpoint)
Executor->>Executor: step-review (APPROVE/REVISE/RETHINK)
alt APPROVE
Executor->>Store: updateStep(done, source=graph)
else RETHINK
Executor->>Git: reset --hard baseline
Executor->>Executor: rewind session checkpoint
Executor->>Store: updateStep(pending)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…xt-muted) + roadmap plugin schema literal 107→108 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… before Save, extend badge waitFor timeout Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (no engine, fresh bundle, free port) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/architecture.md (1)
957-961:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLazy-loaded views list is stale against the current 19-view contract.
This section still says 15 views and omits required entries (
ReliabilityView,EvalsView,StashRecoveryView, plus full 19-view wording). Please update the list/count so architecture docs match the implemented lazy-load contract.As per coding guidelines,
packages/dashboard/app/**/*.{ts,tsx}must lazy-load the specified 19 views viaReact.lazy()with<Suspense fallback={null}>.🤖 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/architecture.md` around lines 957 - 961, Update the architecture doc to reflect the current 19-view lazy-load contract: change the count from 15 to 19, fix the backtick/typo for `AgentDetailView`, and list all 19 views that must be lazy-loaded via React.lazy() with <Suspense fallback={null}> — include `AgentsView`, `TodoView`, `NodesView`, `ChatView`, `MemoryView`, `ResearchView`, `DevServerView`, `InsightsView`, `DocumentsView`, `SkillsView`, `SetupWizardModal`, `PluginManager`, `PiExtensionsManager`, `AgentDetailView`, and add the missing `ReliabilityView`, `EvalsView`, and `StashRecoveryView` so the doc matches packages/dashboard/app/**/*.{ts,tsx}.packages/engine/src/executor.ts (1)
5135-5143:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGraph-pinned step sessions still mark steps done before review approves them.
When
graphStepSessionPinnedforces the step-session path, this callback still writesdonefor every successful step. That bypasses the newmarkDoneOnSuccess: falsepath instepExecutefor foreach templates with astep-reviewnode, so review-gated steps become terminal before the review seam returnsAPPROVE. The projection write needs to stay suppressed in graph-owned review-gated runs orstep-reviewstops being the single done authority.🤖 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 5135 - 5143, The onStepComplete handler currently always writes "done" on successful steps which bypasses review gating; modify the callback in onStepComplete to skip calling this.store.updateStep(..., result.success ? "done" : "skipped") when the run is graph-owned or the step is supposed to defer marking done (i.e., respect graphStepSessionPinned and markDoneOnSuccess semantics). Concretely, before calling this.store.updateStep in onStepComplete, check the run/task/session ownership flag (graphStepSessionPinned) and/or consult the step metadata (the stepExecute path that sets markDoneOnSuccess: false or the step definition for a step-review node) and only call updateStep to mark "done" when graphStepSessionPinned is false and markDoneOnSuccess is true; otherwise suppress the projection write and log that the update was intentionally skipped using executorLog and task.id/stepIndex.
🟡 Minor comments (17)
docs/workflow-steps.md-80-86 (1)
80-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced config block.
Markdown lint will keep flagging this block until it has an explicit language.
Suggested edit
-``` +```ts { source: "task-steps", template: { nodes, edges }, mode?: "sequential" | "parallel", // default sequential isolation?: "shared" | "worktree", // default: shared (sequential), worktree (parallel) concurrency?: number, // parallel only, 1..8, default 2 maxReworkCycles?: number } // default 3, cap 10</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/workflow-steps.mdaround lines 80 - 86, Add an explicit language tag to
the fenced config block that starts with { source: "task-steps", template: {
nodes, edges } so Markdown lint stops flagging it; update the openingto include "ts" (i.e.,ts) for that block and leave the block contents
unchanged.</details> </blockquote></details> <details> <summary>packages/engine/src/__tests__/executor-step-session.test.ts-3628-3670 (1)</summary><blockquote> `3628-3670`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Assert that `fn_review_step` completed successfully, not just that some rewind side effects happened.** `runRethinkScenario()` captures `reviewToolError`, but neither test checks it. If `fn_review_step` starts throwing after `resetStepToBaseline()` has already reset git/session/step state, these assertions can still pass while the executor falls into the retry/failure path. Please assert `getReviewToolError()` is `undefined` and that no fallback failure/retry transition occurred so this really locks the delegation contract. As per coding guidelines "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)". Also applies to: 3673-3713 <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 `@packages/engine/src/__tests__/executor-step-session.test.ts` around lines 3628 - 3670, The test currently captures reviewToolError via mockedCreateFnAgent but never asserts it; update the test (the runRethinkScenario usage) to assert getReviewToolError() is undefined and additionally assert no fallback retry/failure transition occurred (e.g., verify the task/step state in the store or baseTask did not move to a "failed" or "retry" status after executor.run — reference mockedCreateFnAgent, reviewToolError, getReviewToolError(), runRethinkScenario() and the TaskExecutor/baseTask state) so the test guarantees fn_review_step completed successfully rather than only checking side-effects. ``` </details> </blockquote></details> <details> <summary>packages/dashboard/app/components/WorkflowFieldsPanel.css-119-125 (1)</summary><blockquote> `119-125`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Scope checkbox styling to the fields panel to avoid global CSS collisions.** Line 119 reuses `.wf-field--checkbox`, which is also defined in `packages/dashboard/app/components/WorkflowNodeEditor.css` (Line 263). Because these are global selectors, load order can silently alter checkbox spacing/cursor styles across panels. <details> <summary>Suggested fix</summary> ```diff -.wf-field--checkbox { +.wf-fields-panel .wf-field--checkbox { display: inline-flex; align-items: center; gap: 4px; font-size: 0.7rem; color: var(--text-muted); } ``` </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 `@packages/dashboard/app/components/WorkflowFieldsPanel.css` around lines 119 - 125, The .wf-field--checkbox rule in WorkflowFieldsPanel.css is too generic and conflicts with the same selector in WorkflowNodeEditor.css; scope it to the panel by prefixing the selector with the panel/container class used by this component (e.g. .workflow-fields-panel or the component's root class) so the rule becomes specific to WorkflowFieldsPanel (update the selector referencing .wf-field--checkbox inside the CSS file and any related JSX/TSX if necessary to use the same container class). ``` </details> </blockquote></details> <details> <summary>packages/i18n/locales/zh-TW/app.json-6763-6763 (1)</summary><blockquote> `6763-6763`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fill `workflowNodes.splitNote` instead of leaving it blank** Line 6763 sets `workflowNodes.splitNote` to `""`; if rendered directly, the UI shows a missing/blank label. <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 `@packages/i18n/locales/zh-TW/app.json` at line 6763, Replace the empty string currently assigned to workflowNodes.splitNote with an appropriate Traditional Chinese translation (non-empty) for the UI label so it doesn't render blank; locate the key workflowNodes.splitNote in the locales JSON and set a meaningful zh-TW string, following the style of nearby keys and keeping punctuation/format consistent with other entries. ``` </details> </blockquote></details> <details> <summary>packages/core/src/__tests__/mission-store.test.ts-3748-3749 (1)</summary><blockquote> `3748-3749`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Update the test title to match schema v108.** The assertion now checks `108`, but the test name still says `101`, which makes failures/debugging misleading for maintainers. <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 `@packages/core/src/__tests__/mission-store.test.ts` around lines 3748 - 3749, Update the unit test title string to match the asserted schema version: change the it() description "schema version is 101 after migration" to "schema version is 108 after migration" so it correctly reflects the expectation on db.getSchemaVersion() === 108; locate the test containing the it(...) declaration near the assertion calling getSchemaVersion() and only modify the human-readable title string. ``` </details> </blockquote></details> <details> <summary>packages/core/src/__tests__/run-audit.test.ts-586-587 (1)</summary><blockquote> `586-587`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix stale schema version text in test name.** The test still says `bumped to 40` while asserting `108`; rename it so the test intent matches behavior. <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 `@packages/core/src/__tests__/run-audit.test.ts` around lines 586 - 587, Rename the test description so it matches the asserted schema version: update the it(...) string that currently reads "schema version is bumped to 40" to reflect the expected value (e.g., "schema version is bumped to 108") in the test where db.getSchemaVersion() is asserted toBe(108); ensure the test name change is applied in the test case containing the expect(db.getSchemaVersion()).toBe(108). ``` </details> </blockquote></details> <details> <summary>packages/i18n/locales/zh-CN/app.json-6720-6762 (1)</summary><blockquote> `6720-6762`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Localize newly added zh-CN strings instead of shipping English copy.** These new `workflowNodes`/`workflowFields` values are still English in the `zh-CN` locale, so users will see mixed-language UI across the new workflow/task-field surfaces. Also applies to: 6803-6834 <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 `@packages/i18n/locales/zh-CN/app.json` around lines 6720 - 6762, The listed keys (e.g., "failureCollect", "failureFailFast", "failurePolicy", "gateBlocks", "gateMode", "joinAll", "joinAny", "joinMode", "joinQuorum", "mergeBoundaryNote", "quorumN", "releaseCapacity", "releaseCondition", "releaseDependency", "releaseExternal", "releaseManual", "releaseTimer" and the workflowNodes/workflowFields entries around lines ~6803-6834) are still English in the zh-CN locale; replace each English value with the correct Simplified Chinese translation (keeping placeholder tokens like {{condition}} intact) so the UI is fully localized, and ensure translations follow existing style/terminology used elsewhere in the locale file. ``` </details> </blockquote></details> <details> <summary>packages/i18n/locales/zh-CN/app.json-6763-6763 (1)</summary><blockquote> `6763-6763`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Populate `workflowNodes.splitNote` with non-empty copy.** `splitNote` is now empty, which can surface as missing instructional text in the node editor. <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 `@packages/i18n/locales/zh-CN/app.json` at line 6763, The localization key workflowNodes.splitNote is currently an empty string which causes missing instructional text in the node editor; update the value of "splitNote" in the zh-CN app.json to a non-empty, user-facing Chinese copy that describes the node's purpose or instructions (match tone/length of sibling keys like workflowNodes.*), making sure the key name workflowNodes.splitNote remains unchanged and the translation is clear and concise for the editor UI. ``` </details> </blockquote></details> <details> <summary>packages/dashboard/app/components/TaskDetailModal.tsx-624-630 (1)</summary><blockquote> `624-630`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Clear field-level custom-field errors when the modal switches tasks.** `customFieldError` survives prop changes, so a rejection from task A can render under task B if they share a field id. Reset it alongside the `customFieldValues` sync. <details> <summary>Suggested fix</summary> ```diff useEffect(() => { setCustomFieldValues(task.customFields ?? {}); + setCustomFieldError(null); }, [task.id, task.customFields]); ``` </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 `@packages/dashboard/app/components/TaskDetailModal.tsx` around lines 624 - 630, When the modal switches tasks the hook that syncs customFieldValues doesn't clear previous errors, so add a reset for customFieldError inside the same useEffect that currently calls setCustomFieldValues(task.customFields ?? {}); specifically update the effect that depends on task.id and task.customFields to also call setCustomFieldError(null) so any field-level rejection from a previous task is cleared when task changes. ``` </details> </blockquote></details> <details> <summary>packages/dashboard/app/components/__tests__/TaskCard.test.tsx-4216-4225 (1)</summary><blockquote> `4216-4225`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **The one-sided comparator case is not actually exercising a `customFields` change.** Both sides reuse the same `task`, so this only proves that `cardFieldDefs` asymmetry forces inequality. If the comparator still ignored changed `customFields` in the one-sided case, this test would still pass. Use two tasks with different `customFields` and add the reverse prev/next ordering so the memo invariant is covered both ways. Based on learnings: "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)." <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 `@packages/dashboard/app/components/__tests__/TaskCard.test.tsx` around lines 4216 - 4225, The test currently reuses the same task which only proves cardFieldDefs asymmetry triggers inequality; update the case in the "detects customFields change when only one side has cardFieldDefs" test to create two distinct tasks (e.g., taskA = makeTask({ customFields: { sev: "low" } }) and taskB = makeTask({ customFields: { sev: "high" } })), then call __test_areTaskCardPropsEqual twice swapping prev/next (one comparing { task: taskA, cardFieldDefs: undefined } vs { task: taskB, cardFieldDefs: defs } and the other reversed) to ensure the comparator (__test_areTaskCardPropsEqual) detects customFields changes in the one-sided cardFieldDefs scenario; keep noop for handlers and assert .toBe(false) for both calls. ``` </details> </blockquote></details> <details> <summary>packages/engine/src/__tests__/workflow-graph-step-rerun.test.ts-37-73 (1)</summary><blockquote> `37-73`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add the concurrent rejection → retry case to finish this memo regression.** The suite proves sequential rejection clearing and concurrent single-flight success separately, but the poisoned-promise bug is most likely when both callers share the same rejected in-flight promise. Add one narrow case where `Promise.all` observes that shared rejection, then a third call succeeds and increments `calls` again. Based on learnings: "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)." <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 `@packages/engine/src/__tests__/workflow-graph-step-rerun.test.ts` around lines 37 - 73, Add a new test that reproduces the poisoned-promise regression by making two concurrent callers share an in-flight rejected promise and then verifying a subsequent call re-invokes the implementation; use makeExecutor and mock executor.runImplementationPhase so the first invocation throws and later invocations succeed, call Promise.all on two concurrent runGraphTaskStep(task, 0) to observe the shared rejection, then call runGraphTaskStep(task, 0) again and assert the mock was invoked twice and the final call succeeds. Ensure you reference and modify the existing test suite in workflow-graph-step-rerun.test.ts and use the same helpers (makeExecutor, executor.runImplementationPhase, runGraphTaskStep) so the new case covers concurrent rejection → retry. ``` </details> </blockquote></details> <details> <summary>packages/dashboard/app/components/TaskFieldsSection.css-213-213 (1)</summary><blockquote> `213-213`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Replace deprecated `word-break` value.** The value `break-word` for the `word-break` property is deprecated. Use `overflow-wrap: break-word` instead for equivalent modern behavior. <details> <summary>🔧 Proposed fix</summary> ```diff .task-field-orphaned-value { font-size: 13px; color: var(--text-muted, `#b4b8c0`); - word-break: break-word; + overflow-wrap: break-word; } ``` </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 `@packages/dashboard/app/components/TaskFieldsSection.css` at line 213, Replace the deprecated CSS declaration "word-break: break-word;" with the modern equivalent by removing that property and adding "overflow-wrap: break-word;" in the same selector (e.g., in TaskFieldsSection.css where "word-break: break-word;" appears) so text wrapping behavior remains the same using supported CSS. ``` </details> </blockquote></details> <details> <summary>packages/dashboard/app/components/WorkflowFieldsPanel.tsx-240-294 (1)</summary><blockquote> `240-294`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Multi-enum default editor only supports single selection.** The default value editor for `multi-enum` fields displays only the first element of the array (line 256-258) and commits a single-element array when changed (line 267). This is inconsistent with multi-enum semantics, which allow multiple selected values. Users cannot author a default like `["high", "urgent"]` through this UI. Consider replacing the select dropdown with a chips widget (similar to the runtime field editor) to allow selecting multiple default values for multi-enum fields. <details> <summary>♻️ Suggested approach to support multi-value defaults</summary> Replace the select-based editor for multi-enum defaults with a chips UI similar to lines 402-471 (the options editor), allowing users to toggle multiple default values on/off. </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 `@packages/dashboard/app/components/WorkflowFieldsPanel.tsx` around lines 240 - 294, renderDefaultInput currently treats "multi-enum" as a single-value select (using the first element and committing [v]), which prevents authoring multi-value defaults; update renderDefaultInput's isEnumKind branch to detect field.type === "multi-enum" and render a chips/multi-toggle UI (reusing the options editor pattern used elsewhere for options) that: reads the current default as string[] (or [] if undefined), displays field.options as selectable chips/toggles, allows toggling multiple items on/off, respects readOnly, and calls commit(updatedArray) (or commit(undefined) when array becomes empty) so the stored default is an array of selected values. Ensure aria labels and t(...) usage mirror the existing single-select for accessibility. ``` </details> </blockquote></details> <details> <summary>packages/i18n/locales/es/app.json-6763-6763 (1)</summary><blockquote> `6763-6763`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **`workflowNodes.splitNote` is an empty string.** An explicit empty value can render blank helper/label text instead of a fallback, which degrades the workflow editor UX. Please provide Spanish copy or remove the key so fallback logic can apply. <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 `@packages/i18n/locales/es/app.json` at line 6763, The JSON key workflowNodes.splitNote currently has an empty string value; either supply an appropriate Spanish translation for splitNote (matching tone/length of other workflowNodes entries) or delete the splitNote key so the i18n fallback will be used; locate the "splitNote" entry in the es app.json and update it accordingly. ``` </details> </blockquote></details> <details> <summary>packages/i18n/locales/es/app.json-6720-6762 (1)</summary><blockquote> `6720-6762`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Spanish locale contains new English UI copy.** These new entries are not localized (e.g., `codeNote`, `foreachNote`, and most `workflowFields.*` keys), so Spanish users will see mixed-language UI in workflow/task-field surfaces. Please translate these strings in `es/app.json` (or intentionally remove keys to allow fallback behavior if that is your i18n strategy). Also applies to: 6803-6834 <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 `@packages/i18n/locales/es/app.json` around lines 6720 - 6762, The Spanish locale contains many untranslated English keys (e.g., "codeNote", "codeSource", "codeTimeout", "edgeConditionLabel", "edgeRework", "edgeReworkNote", "edgeVerdict", "foreachConcurrency", "foreachEmptyHint", "foreachIsolation", "foreachMaxRework", "foreachMode", "foreachNote", "foreachParallel", "foreachSequential", "foreachShared", "foreachWorktree", "parseArtifact", "parseParser", "reviewCode", "reviewModel", "reviewNote", "reviewPlan", "reviewType" and other workflowFields.* keys referenced around lines 6803-6834); update those entries in es/app.json with proper Spanish translations (or remove the keys if you intentionally rely on fallback) so UI strings are fully localized, and ensure pluralization/placeholders (e.g., "{{condition}}") are preserved in the translated values. ``` </details> </blockquote></details> <details> <summary>packages/i18n/locales/fr/app.json-6803-6834 (1)</summary><blockquote> `6803-6834`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Translate `workflowFields` values to French in the FR locale bundle.** The entire `workflowFields` section is still English, which will render untranslated UI in French mode. <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 `@packages/i18n/locales/fr/app.json` around lines 6803 - 6834, The workflowFields entries in the FR locale bundle are still English; translate every string value inside the "workflowFields" object (e.g., keys like "add", "addOption", "badge", "default", "defaultLabel", "defaultTrue", "duplicateId", "editId", "empty", "idLabel", "idWarn", "nameLabel", "newFieldName", "newOptionLabel", "noDefault", "optionColor", "optionLabel", "optionN", "optionValue", "options", "placement", "placementCard", "placementDetail", "placementSection", "readOnlyHint", "remove", "removeOption", "required", "title", "typeLabel", "widget", "widgetDefault") into French; keep the keys unchanged and only replace the English string values with accurate French translations, preserving placeholders like "{{n}}" and punctuation/formatting. ``` </details> </blockquote></details> <details> <summary>packages/i18n/locales/fr/app.json-6720-6764 (1)</summary><blockquote> `6720-6764`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Localize new `workflowNodes` strings to French (and avoid blank `splitNote`).** This FR locale block is currently mixed-language (English values like “Runs sandboxed TypeScript…”, “Concurrency”, etc.), and `splitNote` is empty at Line 6763. Please provide French translations for consistency and to avoid blank UI text. <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 `@packages/i18n/locales/fr/app.json` around lines 6720 - 6764, Replace the English values with proper French translations for the workflowNodes keys shown (e.g., "codeNote", "codeSource", "codeTimeout", "edgeConditionLabel", "edgeInspector", "edgeNoVerdict", "edgeRework", "edgeReworkNote", "edgeVerdict", "foreachConcurrency", "foreachEmptyHint", "foreachIsolation", "foreachMaxRework", "foreachMode", "foreachNote", "foreachParallel", "foreachSequential", "foreachShared", "foreachWorktree", "parseArtifact", "parseParser", "reviewCode", "reviewModel", "reviewNote", "reviewPlan", "reviewType", "stepExecuteLabel") and provide meaningful French strings for all currently empty keys (e.g., "failureCollect", "failureFailFast", "failurePolicy", "gateBlocks", "gateMode", "joinAll", "joinAny", "joinMode", "joinQuorum", "mergeBoundaryNote", "quorumN", "releaseCapacity", "releaseCondition", "releaseDependency", "releaseExternal", "releaseManual", "releaseTimer", "splitNote") so no UI text is blank and the locale is consistently French; keep placeholders (like {{condition}}) intact in translated values and preserve the original key names exactly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (5)</summary><blockquote> <details> <summary>packages/dashboard/app/components/__tests__/TaskDetailModal.custom-fields.test.tsx (1)</summary><blockquote> `97-117`: _💤 Low value_ **Optional: Consider using `waitFor` instead of setTimeout.** Line 114 uses `await new Promise((r) => setTimeout(r, 50))` to give React a tick. While this is not slow (50ms), per coding guidelines (FN-5048) prefer fake timers or Testing Library's built-in async utilities over real time waits. You could rely on the implicit settling or use `waitFor` with a trivial check. <details> <summary>Alternative approach</summary> ```diff - // Give React a tick to settle; no section should appear. - await new Promise((r) => setTimeout(r, 50)); expect(screen.queryByTestId("task-fields-section")).toBeNull(); ``` The assertion alone may be sufficient since the component renders synchronously when `workflowFieldDefs={[]}` is provided (no async fetch). </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 `@packages/dashboard/app/components/__tests__/TaskDetailModal.custom-fields.test.tsx` around lines 97 - 117, The test "renders no fields section when workflowFieldDefs prop is an empty array" uses a real-time setTimeout to wait for React to settle; replace the await new Promise((r) => setTimeout(r, 50)) with Testing Library's waitFor (or remove the wait entirely) so the assertion is deterministic and faster—wrap the expectations that depend on async state in waitFor(()=> { expect(screen.queryByTestId("task-fields-section")).toBeNull(); expect(fetchSpy).not.toHaveBeenCalled(); }) or simply drop the delay if TaskDetailModal with workflowFieldDefs={[]} renders synchronously; target the test block and the call sites of TaskDetailModal and dashboardApi.fetchBoardWorkflows when making the change. ``` </details> </blockquote></details> <details> <summary>packages/engine/src/step-runner.ts (2)</summary><blockquote> `273-273`: _💤 Low value_ **Defensive coding: validate SHA format before shell interpolation.** Lines 273 and 381 interpolate `baselineSha` directly into shell commands without validation. While `baselineSha` typically comes from `git rev-parse HEAD` output (a safe hex string), there's no format validation before use. If `baselineSha` were ever sourced from untrusted input (e.g., stored instance data without validation), this could enable command injection. <details> <summary>Add defensive validation</summary> ```diff +// Validate SHA format (40-char hex for full SHA, 4+ for short) +function isValidGitSha(sha: string): boolean { + return /^[0-9a-f]{4,40}$/i.test(sha); +} + export async function resetStepToBaseline( // ... ): Promise<ResetStepResult> { // ... if (reviewType === "code" && baselineSha) { + if (!isValidGitSha(baselineSha)) { + executorLog.error(`${taskId}: invalid baseline SHA format: ${baselineSha}`); + return { ok: false, reason: "invalid baseline SHA format" }; + } try { await execAsync(`git reset --hard ${baselineSha}`, { cwd: worktreePath }); ``` </details> Also applies to: 381-381 <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 `@packages/engine/src/step-runner.ts` at line 273, Validate the git SHA before interpolating it into shell commands: ensure the variable baselineSha (used where execAsync is called) matches an expected hex SHA pattern (e.g. 7–40 hex chars) and reject or throw if not; after validation, continue to use baselineSha, or better, avoid direct shell interpolation by passing the SHA as a safe argument/parameter to execAsync (or properly escape it) where execAsync(`git reset --hard ${baselineSha}`, { cwd: worktreePath }) and the other execAsync call use baselineSha. Ensure the check is performed in the function that sets/consumes baselineSha so all uses (including the two execAsync sites) are protected. ``` </details> --- `306-306`: _💤 Low value_ **Minor inconsistency in error handling.** Line 306 `await store.updateStep(taskId, stepIndex, "pending");` is not wrapped in try/catch, unlike the `updateStep` calls in `runTaskStep` (lines 133, 167). If this projection write fails after git reset and session rewind have already mutated state (lines 273-300), the function will throw with git/session out of sync with the projection. This is probably intentional (projection write failure should fail the operation loudly), but the pattern inconsistency makes the intention unclear. <details> <summary>Consider wrapping for consistency</summary> ```diff // ── Reset step status to pending (projection sink). ────────────────────── - await store.updateStep(taskId, stepIndex, "pending"); + try { + await store.updateStep(taskId, stepIndex, "pending"); + } catch (err) { + executorLog.error(`${taskId}: resetStepToBaseline failed to mark step ${stepIndex} pending: ${errMsg(err)}`); + // If projection write fails after git/session reset, we want to fail loudly + throw err; + } ``` </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 `@packages/engine/src/step-runner.ts` at line 306, The call to await store.updateStep(taskId, stepIndex, "pending") is not wrapped in a try/catch like the other updateStep calls in runTaskStep, causing inconsistent error-handling and unclear intent; wrap this specific store.updateStep(taskId, stepIndex, "pending") invocation in a try/catch that mirrors the pattern used in runTaskStep: catch any error, log contextual details (taskId, stepIndex and operation) via the module logger, then rethrow the error so the failure remains loud but the log provides clear context. Ensure you reference the same store.updateStep call and align behavior with the other updateStep error handlers. ``` </details> </blockquote></details> <details> <summary>packages/dashboard/app/components/workflow-flow-mapping.ts (1)</summary><blockquote> `296-297`: _⚡ Quick win_ **Remove unused `groupIds` variable.** `groupIds` is computed at line 296 but never referenced; the `void groupIds` at line 357 only suppresses the linter warning. The edge filtering at lines 350-354 uses `childIdToGroup` instead. This is dead code. <details> <summary>♻️ Proposed fix</summary> ```diff - const groupIds = new Set(topNodes.filter((n) => n.data.kind === "foreach").map((n) => n.id)); const hasFields = Array.isArray(fields) && fields.length > 0; ... - void groupIds; ``` </details> Also applies to: 357-357 <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 `@packages/dashboard/app/components/workflow-flow-mapping.ts` around lines 296 - 297, Remove the unused Set variable `groupIds` (currently created from `topNodes.filter(...).map(...`) and the `void groupIds` no-op; the edge filtering logic uses `childIdToGroup` so `groupIds` is dead code—delete the `const groupIds = ...` declaration and any suppression referencing it to clean up the unused variable. ``` </details> </blockquote></details> <details> <summary>packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx (1)</summary><blockquote> `398-431`: _💤 Low value_ **Consider relocating pure mapping tests to `workflow-flow-mapping.test.ts`.** This test exercises `irToFlow`/`flowToIr` directly without rendering `WorkflowNodeEditor`. The same pattern is already used in `workflow-flow-mapping.test.ts` (e.g., the "foreach + rework round-trip" suite). Colocating these pure-mapping tests would improve cohesion. <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 `@packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx` around lines 398 - 431, This test in WorkflowNodeEditor.test.tsx is a pure mapping round-trip that exercises irToFlow and flowToIr without rendering WorkflowNodeEditor; move it to the existing mapping test file to improve cohesion. Copy the test (the "round-trips a rework edge..." block) into workflow-flow-mapping.test.ts alongside the other foreach/rework mapping suites, keep the same assertions that use irToFlow and flowToIr, and remove the block from WorkflowNodeEditor.test.tsx; ensure imports used by the test (irToFlow, flowToIr, stepwiseDef) are present in workflow-flow-mapping.test.ts and adjust any test setup variables (like columns) as needed. ``` </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.Inline comments:
In@packages/core/src/store.ts:
- Line 1868: restoreFromArchive currently doesn't copy entry.customFields back
to the live task, so unarchiving loses custom field values; update the
restoreFromArchive function to set the live task's customFields from archive
entry.customFields (same way archive() writes customFields), e.g., when merging
archived data into the restored item ensure you assign entry.customFields ??
undefined to the target object's customFields property so custom fields persist
across archive→unarchive round-trips.- Around line 12382-12410: The current loop builds occupantsWithFields by
skipping tasks with empty customFields, so pendingFieldReconcile only reconciles
tasks that already have values; change the loop so every taskId from
occupantTaskIds is included in the list passed into pendingFieldReconcile (keep
computing occupantsByField counts as currently done) — i.e., do not continue
when Object.keys(values).length === 0: still add taskId to the array
(occupantsWithFields or rename to occupantTaskIdsForReconcile) and only skip the
per-field counting when values are empty; ensure
pendingFieldReconcile.occupantTaskIds receives the full set so all occupants are
reconciled after a field-schema edit and keep computeIncompatibleFieldChanges
usage unchanged.In
@packages/dashboard/app/components/TaskFieldsSection.tsx:
- Around line 95-101: The commit function in TaskFieldsSection currently
fire-and-forgets onSave, allowing overlapping PATCHes for the same field; modify
commit (and related code that handles onSave responses in TaskDetailModal) to
serialize saves per field or ignore stale responses by attaching a monotonically
increasing request id/timestamp for each field update: increment a
field-specific counter before calling onSave({ [field.id]: next }), capture that
id, and when the save promise resolves only apply the response if the captured
id matches the latest known id for field.id (or alternatively await the previous
promise for that field to serialize). Ensure you update the commit closure (and
any state that stores lastRequestId per field) and treat onSave as returning a
Promise so stale responses are detected and dropped.- Around line 219-285: The inputs use uncontrolled defaultValue so they don't
update when props change; make them controlled and sync to prop updates: for
each branch that computes current (the date/text/number/url branches in
TaskFieldsSection.tsx using controlId, field, value, commit), introduce a local
state (e.g., localValue/setLocalValue via useState) initialized from current and
add a useEffect to update localValue whenever the incoming prop-derived current
changes; replace defaultValue={current} with value={localValue} and update
localValue on onChange, but keep the existing onBlur logic (compare against the
latest prop-derived current before calling commit) so blurs won't overwrite
refreshed customFields values.In
@packages/dashboard/app/components/WorkflowNodeEditor.tsx:
- Around line 96-99: The editor currently hardcodes BUILTIN_STEP_PARSERS
(["step-headings","json-steps"]) so plugin parsers never appear; update
WorkflowNodeEditor to merge runtime/plugin parser adapters into the options
presented for parse-steps nodes by retrieving the parser registry/adapters (from
props, context, or the existing runtime client) and concatenating their
identifiers with BUILTIN_STEP_PARSERS (e.g., build a combined array like
[...BUILTIN_STEP_PARSERS, ...pluginIds]) before rendering the parser selector;
ensure the code paths that validate or serialize the selected parser (the
parse-steps node handling code) accept these plugin IDs too.In
@packages/engine/src/agent-tools.ts:
- Around line 1077-1087: The returned payload from fn_workflow_get is dropping
the workflow layout; update the payload construction in fn_workflow_get (the
block that builds "payload" from def) to include layout: def.layout so the
response preserves editor node positions; ensure you add layout to the JSON
payload returned (and that details/workflowId logic remains unchanged) so
read→modify→write cycles won't strip layout when using
store.getWorkflowDefinition().In
@packages/engine/src/code-node-runner.ts:
- Around line 405-407: The current check uses truthy template?.nodes before
calling validateCodeNodeSources which can pass non-array values and cause a
runtime error inside validateCodeNodeSources; update the guard to only call
validateCodeNodeSources when template?.nodes is an array (e.g.
Array.isArray(template.nodes)), and if template.nodes exists but is not an
array, push a validation error onto failures (or convert it to the same
structure returned by validateCodeNodeSources) so failures.push(...(…)) only
receives an array of errors; adjust the branch around validateCodeNodeSources,
referencing template.nodes, validateCodeNodeSources, and failures.push.In
@packages/engine/src/executor.ts:
- Around line 3247-3253: graphStepActiveContext is currently keyed by task.id
which causes races when multiple instances of the same task run in parallel;
update the map to key by instanceId (or a composite key like
${task.id}:${instanceId}) instead of task.id wherever graphStepActiveContext
is set/read (e.g., in stepExecute and any code that pins/clears the
ForeachActiveContext), and ensure runGraphTaskStep reads the context by
instanceId so deferDoneToReview and baseline context are per-instance and
cleared with the existing per-run pins.- Around line 4030-4044: graphStepRunOnce currently only removes the memoized
promise when runImplementationPhase rejects, so a later RETHINK that calls
applyGraphRethinkReset ends up reusing the resolved promise; fix by ensuring the
memo is cleared on a successful implementation when a RETHINK reset occurs:
update the logic so either applyGraphRethinkReset calls
this.graphStepRunOnce.delete(task.id) for affected task ids, or after awaiting
phase in stepExecute/step handling check for the RETHINK/reset condition and
delete this.graphStepRunOnce entry for task.id so runImplementationPhase will be
rerun (reference symbols: graphStepRunOnce, runImplementationPhase,
applyGraphRethinkReset, stepExecute).- Around line 3656-3683: The integrate() implementation runs git rebase and git
rebase --abort from mainWorktree(), which fails because branchName is checked
out in its instance worktree; change integrate to perform the rebase and any
--abort in the instance worktree and do the final git checkout/merge from
mainWorktree(). Concretely: inside integrate(branchName, stepIndex) call the
helper that returns the per-instance worktree path (the same place
allocateInstanceWorktree adds the branch; e.g. an instanceWorktree(branchName)
or equivalent) and use that cwd for execAsync(git rebase ${target} ${branchName}, { cwd: instanceCwd }) and for execAsync("git rebase --abort", {
cwd: instanceCwd }); keep restoring/checking out target and running git merge
--ff-only ${branchName} from mainWorktree() (cwd = await mainWorktree()) so the
merge can ff-only from the main worktree. Ensure executorLog.warn and
getConflictedFiles still run against the appropriate cwd where conflicts
occurred (instanceCwd).In
@packages/i18n/locales/ko/app.json:
- Around line 6720-6764: Several newly added keys in the Korean locale (e.g.,
"edgeConditionLabel", "edgeInspector", "edgeRework", "edgeReworkNote",
"failureCollect", "failureFailFast", "foreachConcurrency", "foreachEmptyHint",
"foreachIsolation", "foreachMaxRework", "foreachMode", "foreachNote",
"foreachParallel", "foreachSequential", "foreachShared", "foreachWorktree",
"gateBlocks", "gateMode", "joinAll", "joinAny", "joinMode", "joinQuorum",
"mergeBoundaryNote", "quorumN", "releaseCapacity", "releaseCondition",
"releaseDependency", "releaseExternal", "releaseManual", "releaseTimer",
"splitNote", etc.) are still in English; translate each value into Korean in
packages/i18n/locales/ko/app.json while preserving interpolation tokens like
{{n}} and {{condition}} and keeping the original key names and punctuation/HTML.
Ensure translations match existing tone and terminology used elsewhere in the
file and update the entries around the blocks starting near the current diff
(lines shown in the review) as well as the other group referenced (around
6802–6834).In
@packages/i18n/locales/zh-TW/app.json:
- Around line 6720-6762: The zh-TW locale file contains new English strings that
must be localized: replace the English values for keys such as "codeNote",
"codeSource", "codeTimeout", "edgeConditionLabel", "edgeInspector",
"edgeNoVerdict", "edgeRework", "edgeReworkNote", "edgeVerdict",
"failureCollect", "failureFailFast", "failurePolicy", "foreachConcurrency",
"foreachEmptyHint", "foreachIsolation", "foreachMaxRework", "foreachMode",
"foreachNote", "foreachParallel", "foreachSequential", "foreachShared",
"foreachWorktree", "gateBlocks", "gateMode", "joinAll", "joinAny", "joinMode",
"joinQuorum", "mergeBoundaryNote", "parseArtifact", "parseParser", "quorumN",
"releaseCapacity", "releaseCondition", "releaseDependency", "releaseExternal",
"releaseManual", "releaseTimer", "reviewCode", "reviewModel", "reviewNote",
"reviewPlan", and "reviewType" (and the same set in the second block around the
other range); translate each English value into proper Traditional Chinese
(zh-TW) and replace the English text in the locale file so the
workflow/task-field editors show localized labels instead of mixed-language
strings. Ensure translations preserve placeholders like {{condition}} and keep
empty values only where intentional.
Outside diff comments:
In@docs/architecture.md:
- Around line 957-961: Update the architecture doc to reflect the current
19-view lazy-load contract: change the count from 15 to 19, fix the
backtick/typo forAgentDetailView, and list all 19 views that must be
lazy-loaded via React.lazy() with — include
AgentsView,TodoView,NodesView,ChatView,MemoryView,ResearchView,
DevServerView,InsightsView,DocumentsView,SkillsView,
SetupWizardModal,PluginManager,PiExtensionsManager,AgentDetailView,
and add the missingReliabilityView,EvalsView, andStashRecoveryViewso
the doc matches packages/dashboard/app/**/*.{ts,tsx}.In
@packages/engine/src/executor.ts:
- Around line 5135-5143: The onStepComplete handler currently always writes
"done" on successful steps which bypasses review gating; modify the callback in
onStepComplete to skip calling this.store.updateStep(..., result.success ?
"done" : "skipped") when the run is graph-owned or the step is supposed to defer
marking done (i.e., respect graphStepSessionPinned and markDoneOnSuccess
semantics). Concretely, before calling this.store.updateStep in onStepComplete,
check the run/task/session ownership flag (graphStepSessionPinned) and/or
consult the step metadata (the stepExecute path that sets markDoneOnSuccess:
false or the step definition for a step-review node) and only call updateStep to
mark "done" when graphStepSessionPinned is false and markDoneOnSuccess is true;
otherwise suppress the projection write and log that the update was
intentionally skipped using executorLog and task.id/stepIndex.
Minor comments:
In@docs/workflow-steps.md:
- Around line 80-86: Add an explicit language tag to the fenced config block
that starts with { source: "task-steps", template: { nodes, edges } so Markdown
lint stops flagging it; update the openingto include "ts" (i.e.,ts) for
that block and leave the block contents unchanged.In
@packages/core/src/__tests__/mission-store.test.ts:
- Around line 3748-3749: Update the unit test title string to match the asserted
schema version: change the it() description "schema version is 101 after
migration" to "schema version is 108 after migration" so it correctly reflects
the expectation on db.getSchemaVersion() === 108; locate the test containing the
it(...) declaration near the assertion calling getSchemaVersion() and only
modify the human-readable title string.In
@packages/core/src/__tests__/run-audit.test.ts:
- Around line 586-587: Rename the test description so it matches the asserted
schema version: update the it(...) string that currently reads "schema version
is bumped to 40" to reflect the expected value (e.g., "schema version is bumped
to 108") in the test where db.getSchemaVersion() is asserted toBe(108); ensure
the test name change is applied in the test case containing the
expect(db.getSchemaVersion()).toBe(108).In
@packages/dashboard/app/components/__tests__/TaskCard.test.tsx:
- Around line 4216-4225: The test currently reuses the same task which only
proves cardFieldDefs asymmetry triggers inequality; update the case in the
"detects customFields change when only one side has cardFieldDefs" test to
create two distinct tasks (e.g., taskA = makeTask({ customFields: { sev: "low" }
}) and taskB = makeTask({ customFields: { sev: "high" } })), then call
__test_areTaskCardPropsEqual twice swapping prev/next (one comparing { task:
taskA, cardFieldDefs: undefined } vs { task: taskB, cardFieldDefs: defs } and
the other reversed) to ensure the comparator (__test_areTaskCardPropsEqual)
detects customFields changes in the one-sided cardFieldDefs scenario; keep noop
for handlers and assert .toBe(false) for both calls.In
@packages/dashboard/app/components/TaskDetailModal.tsx:
- Around line 624-630: When the modal switches tasks the hook that syncs
customFieldValues doesn't clear previous errors, so add a reset for
customFieldError inside the same useEffect that currently calls
setCustomFieldValues(task.customFields ?? {}); specifically update the effect
that depends on task.id and task.customFields to also call
setCustomFieldError(null) so any field-level rejection from a previous task is
cleared when task changes.In
@packages/dashboard/app/components/TaskFieldsSection.css:
- Line 213: Replace the deprecated CSS declaration "word-break: break-word;"
with the modern equivalent by removing that property and adding "overflow-wrap:
break-word;" in the same selector (e.g., in TaskFieldsSection.css where
"word-break: break-word;" appears) so text wrapping behavior remains the same
using supported CSS.In
@packages/dashboard/app/components/WorkflowFieldsPanel.css:
- Around line 119-125: The .wf-field--checkbox rule in WorkflowFieldsPanel.css
is too generic and conflicts with the same selector in WorkflowNodeEditor.css;
scope it to the panel by prefixing the selector with the panel/container class
used by this component (e.g. .workflow-fields-panel or the component's root
class) so the rule becomes specific to WorkflowFieldsPanel (update the selector
referencing .wf-field--checkbox inside the CSS file and any related JSX/TSX if
necessary to use the same container class).In
@packages/dashboard/app/components/WorkflowFieldsPanel.tsx:
- Around line 240-294: renderDefaultInput currently treats "multi-enum" as a
single-value select (using the first element and committing [v]), which prevents
authoring multi-value defaults; update renderDefaultInput's isEnumKind branch to
detect field.type === "multi-enum" and render a chips/multi-toggle UI (reusing
the options editor pattern used elsewhere for options) that: reads the current
default as string[] (or [] if undefined), displays field.options as selectable
chips/toggles, allows toggling multiple items on/off, respects readOnly, and
calls commit(updatedArray) (or commit(undefined) when array becomes empty) so
the stored default is an array of selected values. Ensure aria labels and t(...)
usage mirror the existing single-select for accessibility.In
@packages/engine/src/__tests__/executor-step-session.test.ts:
- Around line 3628-3670: The test currently captures reviewToolError via
mockedCreateFnAgent but never asserts it; update the test (the
runRethinkScenario usage) to assert getReviewToolError() is undefined and
additionally assert no fallback retry/failure transition occurred (e.g., verify
the task/step state in the store or baseTask did not move to a "failed" or
"retry" status after executor.run — reference mockedCreateFnAgent,
reviewToolError, getReviewToolError(), runRethinkScenario() and the
TaskExecutor/baseTask state) so the test guarantees fn_review_step completed
successfully rather than only checking side-effects.In
@packages/engine/src/__tests__/workflow-graph-step-rerun.test.ts:
- Around line 37-73: Add a new test that reproduces the poisoned-promise
regression by making two concurrent callers share an in-flight rejected promise
and then verifying a subsequent call re-invokes the implementation; use
makeExecutor and mock executor.runImplementationPhase so the first invocation
throws and later invocations succeed, call Promise.all on two concurrent
runGraphTaskStep(task, 0) to observe the shared rejection, then call
runGraphTaskStep(task, 0) again and assert the mock was invoked twice and the
final call succeeds. Ensure you reference and modify the existing test suite in
workflow-graph-step-rerun.test.ts and use the same helpers (makeExecutor,
executor.runImplementationPhase, runGraphTaskStep) so the new case covers
concurrent rejection → retry.In
@packages/i18n/locales/es/app.json:
- Line 6763: The JSON key workflowNodes.splitNote currently has an empty string
value; either supply an appropriate Spanish translation for splitNote (matching
tone/length of other workflowNodes entries) or delete the splitNote key so the
i18n fallback will be used; locate the "splitNote" entry in the es app.json and
update it accordingly.- Around line 6720-6762: The Spanish locale contains many untranslated English
keys (e.g., "codeNote", "codeSource", "codeTimeout", "edgeConditionLabel",
"edgeRework", "edgeReworkNote", "edgeVerdict", "foreachConcurrency",
"foreachEmptyHint", "foreachIsolation", "foreachMaxRework", "foreachMode",
"foreachNote", "foreachParallel", "foreachSequential", "foreachShared",
"foreachWorktree", "parseArtifact", "parseParser", "reviewCode", "reviewModel",
"reviewNote", "reviewPlan", "reviewType" and other workflowFields.* keys
referenced around lines 6803-6834); update those entries in es/app.json with
proper Spanish translations (or remove the keys if you intentionally rely on
fallback) so UI strings are fully localized, and ensure
pluralization/placeholders (e.g., "{{condition}}") are preserved in the
translated values.In
@packages/i18n/locales/fr/app.json:
- Around line 6803-6834: The workflowFields entries in the FR locale bundle are
still English; translate every string value inside the "workflowFields" object
(e.g., keys like "add", "addOption", "badge", "default", "defaultLabel",
"defaultTrue", "duplicateId", "editId", "empty", "idLabel", "idWarn",
"nameLabel", "newFieldName", "newOptionLabel", "noDefault", "optionColor",
"optionLabel", "optionN", "optionValue", "options", "placement",
"placementCard", "placementDetail", "placementSection", "readOnlyHint",
"remove", "removeOption", "required", "title", "typeLabel", "widget",
"widgetDefault") into French; keep the keys unchanged and only replace the
English string values with accurate French translations, preserving placeholders
like "{{n}}" and punctuation/formatting.- Around line 6720-6764: Replace the English values with proper French
translations for the workflowNodes keys shown (e.g., "codeNote", "codeSource",
"codeTimeout", "edgeConditionLabel", "edgeInspector", "edgeNoVerdict",
"edgeRework", "edgeReworkNote", "edgeVerdict", "foreachConcurrency",
"foreachEmptyHint", "foreachIsolation", "foreachMaxRework", "foreachMode",
"foreachNote", "foreachParallel", "foreachSequential", "foreachShared",
"foreachWorktree", "parseArtifact", "parseParser", "reviewCode", "reviewModel",
"reviewNote", "reviewPlan", "reviewType", "stepExecuteLabel") and provide
meaningful French strings for all currently empty keys (e.g., "failureCollect",
"failureFailFast", "failurePolicy", "gateBlocks", "gateMode", "joinAll",
"joinAny", "joinMode", "joinQuorum", "mergeBoundaryNote", "quorumN",
"releaseCapacity", "releaseCondition", "releaseDependency", "releaseExternal",
"releaseManual", "releaseTimer", "splitNote") so no UI text is blank and the
locale is consistently French; keep placeholders (like {{condition}}) intact in
translated values and preserve the original key names exactly.In
@packages/i18n/locales/zh-CN/app.json:
- Around line 6720-6762: The listed keys (e.g., "failureCollect",
"failureFailFast", "failurePolicy", "gateBlocks", "gateMode", "joinAll",
"joinAny", "joinMode", "joinQuorum", "mergeBoundaryNote", "quorumN",
"releaseCapacity", "releaseCondition", "releaseDependency", "releaseExternal",
"releaseManual", "releaseTimer" and the workflowNodes/workflowFields entries
around lines ~6803-6834) are still English in the zh-CN locale; replace each
English value with the correct Simplified Chinese translation (keeping
placeholder tokens like {{condition}} intact) so the UI is fully localized, and
ensure translations follow existing style/terminology used elsewhere in the
locale file.- Line 6763: The localization key workflowNodes.splitNote is currently an empty
string which causes missing instructional text in the node editor; update the
value of "splitNote" in the zh-CN app.json to a non-empty, user-facing Chinese
copy that describes the node's purpose or instructions (match tone/length of
sibling keys like workflowNodes.*), making sure the key name
workflowNodes.splitNote remains unchanged and the translation is clear and
concise for the editor UI.In
@packages/i18n/locales/zh-TW/app.json:
- Line 6763: Replace the empty string currently assigned to
workflowNodes.splitNote with an appropriate Traditional Chinese translation
(non-empty) for the UI label so it doesn't render blank; locate the key
workflowNodes.splitNote in the locales JSON and set a meaningful zh-TW string,
following the style of nearby keys and keeping punctuation/format consistent
with other entries.
Nitpick comments:
In
@packages/dashboard/app/components/__tests__/TaskDetailModal.custom-fields.test.tsx:
- Around line 97-117: The test "renders no fields section when workflowFieldDefs
prop is an empty array" uses a real-time setTimeout to wait for React to settle;
replace the await new Promise((r) => setTimeout(r, 50)) with Testing Library's
waitFor (or remove the wait entirely) so the assertion is deterministic and
faster—wrap the expectations that depend on async state in waitFor(()=> {
expect(screen.queryByTestId("task-fields-section")).toBeNull();
expect(fetchSpy).not.toHaveBeenCalled(); }) or simply drop the delay if
TaskDetailModal with workflowFieldDefs={[]} renders synchronously; target the
test block and the call sites of TaskDetailModal and
dashboardApi.fetchBoardWorkflows when making the change.In
@packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx:
- Around line 398-431: This test in WorkflowNodeEditor.test.tsx is a pure
mapping round-trip that exercises irToFlow and flowToIr without rendering
WorkflowNodeEditor; move it to the existing mapping test file to improve
cohesion. Copy the test (the "round-trips a rework edge..." block) into
workflow-flow-mapping.test.ts alongside the other foreach/rework mapping suites,
keep the same assertions that use irToFlow and flowToIr, and remove the block
from WorkflowNodeEditor.test.tsx; ensure imports used by the test (irToFlow,
flowToIr, stepwiseDef) are present in workflow-flow-mapping.test.ts and adjust
any test setup variables (like columns) as needed.In
@packages/dashboard/app/components/workflow-flow-mapping.ts:
- Around line 296-297: Remove the unused Set variable
groupIds(currently
created fromtopNodes.filter(...).map(...) and thevoid groupIdsno-op; the
edge filtering logic useschildIdToGroupsogroupIdsis dead code—delete the
const groupIds = ...declaration and any suppression referencing it to clean
up the unused variable.In
@packages/engine/src/step-runner.ts:
- Line 273: Validate the git SHA before interpolating it into shell commands:
ensure the variable baselineSha (used where execAsync is called) matches an
expected hex SHA pattern (e.g. 7–40 hex chars) and reject or throw if not; after
validation, continue to use baselineSha, or better, avoid direct shell
interpolation by passing the SHA as a safe argument/parameter to execAsync (or
properly escape it) where execAsync(git reset --hard ${baselineSha}, { cwd:
worktreePath }) and the other execAsync call use baselineSha. Ensure the check
is performed in the function that sets/consumes baselineSha so all uses
(including the two execAsync sites) are protected.- Line 306: The call to await store.updateStep(taskId, stepIndex, "pending") is
not wrapped in a try/catch like the other updateStep calls in runTaskStep,
causing inconsistent error-handling and unclear intent; wrap this specific
store.updateStep(taskId, stepIndex, "pending") invocation in a try/catch that
mirrors the pattern used in runTaskStep: catch any error, log contextual details
(taskId, stepIndex and operation) via the module logger, then rethrow the error
so the failure remains loud but the log provides clear context. Ensure you
reference the same store.updateStep call and align behavior with the other
updateStep error handlers.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx (1)
392-392:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMock
fetchModelswith the full response shape to avoid undefined-path test flakes.
ModelsResponseincludes favorites arrays; returning onlymodelscan trigger runtime issues if UI paths read favorites during this test.Suggested patch
- vi.mocked(fetchModels).mockResolvedValue({ models: [] }); + vi.mocked(fetchModels).mockResolvedValue({ + models: [], + favoriteProviders: [], + favoriteModels: [], + });🤖 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/__tests__/WorkflowNodeEditor.test.tsx` at line 392, The test mocks fetchModels with only { models: [] } which doesn't match the ModelsResponse shape and can cause undefined-path flakes; update the mock used by vi.mocked(fetchModels) to return the full ModelsResponse shape (include the favorites arrays and any other required keys, e.g. favorites: [], modelFavorites: [], etc.) so consumers of fetchModels in WorkflowNodeEditor tests always see the expected properties.
🤖 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.
Outside diff comments:
In `@packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx`:
- Line 392: The test mocks fetchModels with only { models: [] } which doesn't
match the ModelsResponse shape and can cause undefined-path flakes; update the
mock used by vi.mocked(fetchModels) to return the full ModelsResponse shape
(include the favorites arrays and any other required keys, e.g. favorites: [],
modelFavorites: [], etc.) so consumers of fetchModels in WorkflowNodeEditor
tests always see the expected properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5809f6e-04b9-49cf-9911-7df0b91d29c3
📒 Files selected for processing (2)
docs/solutions/developer-experience/browser-testing-dashboard-from-worktree-safely.mdpackages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx
- restore customFields on unarchive; reconcile all occupants on field-schema edits (store.ts) - serialize per-field saves + controlled inputs in TaskFieldsSection (race fixes) - fn_workflow_get includes layout; Array.isArray guards in validateCodeNodeSources - per-instance graphStepActiveContext keying; rebase in instance worktree; clear run-once memo on RETHINK - GET /api/step-parsers + registry-backed parser select (plugin parsers reachable from editor) - translate new workflowNodes/workflowFields strings across all 5 non-en locales Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gsxdsm
added a commit
that referenced
this pull request
Jun 5, 2026
…erge race Main went red when the fast-tests quality-backfill projects (PR #1385) landed alongside the workflow-columns stream (PR #1424) — the new test projects were written against pre-stream code: - TaskFieldsSection.css toggle knob used background: #fff, violating the theme-token assertion in AgentListModal's styling-parity test; use var(--card) per the SkillsView toggle convention - ListView.test.tsx api mock lacked fetchBoardWorkflows (TaskDetailModal now calls it on mount) - chat.test.ts and routes-agent-import.test.ts @fusion/core mocks lacked registerTraitHookImpl (engine merge-trait registers hooks at import) - auto-merge-toggle-blank.mobile and board-mobile-initial-render used vi.runAllTimers(), which never terminates now that sse-bus starts a keepalive setInterval; use vi.runOnlyPendingTimers() Both quality-backfill projects now pass fully: 7151/7151 across 414 files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 5, 2026
This was referenced Jun 21, 2026
This was referenced Jul 31, 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
1 participant
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.
Implements
docs/plans/2026-06-04-001-feat-step-inversion-workflow-modelable-steps-plan.md— the next increment of the engine→workflow inversion (stacked on #1418): steps become workflow-modelable graph structure, with parallel execution and a workflow-defined task shape. All behindexperimentalFeatures.workflowGraphExecutor; the default workflow is byte-identical (characterization + parity suites).Pillar 1 — Steps as graph nodes
foreachtemplate region expanded once per planned step (iterative region sub-walk; recursive walk's cycle detector untouched); deterministic instance ids; pinned step count +pin-mismatchfail-safestep-execute/step-reviewseams over new substrate pairrunTaskStep/resetStepToBaseline(RETHINK extraction with blast-radius guard; baselines/checkpoints persisted, not in-memory)reworkedges are the only legal cycles; single-writer verdict authority (split-branch reviews advisory-only)Task.steps[]stays the projection sink viaupdateStep(source:"graph")(dependency-order guard, audit-loud suppression); merge-blocker/dashboard/TUI/reconcile unchangedworkflow_run_step_instances+tasks.customFields(additive; pruned per run; crash-resume reconstruction)Pillar 2 — Parallel step execution
### Step N (depends: 1,2):annotations →TaskStep.dependsOn; foreachmode: sequential|parallel×isolation: shared|worktree(parallel+shared validator-rejected)integration-conflict→ rework on updated base, budget-counted; branch-scoped RETHINKPillar 3 — Workflow-defined task shape
parse-stepsnode (artifact + parser;no-steps/parse-erroroutcomes; pin protection) over a pluggable parser registry (step-headings= byte-identical extraction,json-steps, plugin parsers fail-closed)codenode: inline TypeScript, esbuild-compiled (save-time validation), child-process harness (restricted env, SIGKILL timeout, capped output), customFields writes through the validation authorityartifactsdeclarations; custom task fields (8 types, enum options, render hints) with a single validating store authority, orphan-not-delete reconciliation,coercegatefn_task_update custom_fields,fn_workflow_getSafety
Residual Review Findings
See
docs/residual-review-findings/gsxdsm-step-inversion.mdfor the tracked list. Headline items:executor.ts:3945): the memoized implementation pass runs in the main worktree; instance branches receive no per-step commits, so parallel-mode integration rebases empty branches. Scheduling/integration/conflict machinery is real and tested; true write-isolation needs a per-stepStepSessionExecutorscoped to the instance worktree. Experimental-flag path only.recoverStaleTransitionPendinganalogue) not yet implemented (per-run resume seeding exists)🤖 Generated with Claude Code
CI Note
pr-checks.yml(the authoritative PR CI) only triggers for PRs targetingmain; it will run automatically when this PR retargets after #1418 merges. A manualci.ymldispatch (3-shard layout) was run twice on the branch: all named failures were fixed (0451172bf— theme-token conventions + roadmap schema literal; shards 1/2 + Lint green), but the engine suite in shard 3 dies silently mid-run with no failing test or vitest summary (runner OOM signature — the 3-shard layout packs both engine projects into one job). The full engine suite passes locally in both shard halves (420 files / 6,321 tests + reliability 445) and under the 4-shardpr-checks.ymllayout.Summary by CodeRabbit
New Features
Enhancements
Schema Migration