feat!: move workflow invocation contracts to transitions#329
Merged
Conversation
Contributor
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Warning Review limit reached
More reviews will be available in 46 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds transition-branch parameters and prompts to edges across client, editor, services, runtime, and DB; introduces previous-target context source; implements edge reconnection and endpoint handles; updates validation, schemas, tests, i18n, and docs accordingly. ChangesTransition parameters and reconnection end-to-end
Estimated code review effort ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
shared/serverapi/workflow.go (1)
1243-1278:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
context_modeandcontext_sourcetogether on every edge write path.Line 1253 still validates
context_sourcein isolation, and Lines 1448-1452 only add aparameterscount check for graph drafts. That meansnew_session + selected_node/previous_targetcan still be persisted through both edge-update and graph-save flows. The view layer now treats those source kinds as manual-move blockers, so a stalecontext_sourceon a non-continue_sessionedge will make a valid branch disappear after save.Suggested fix
func validateWorkflowEdgeFields(workflowID string, edgeID string, transitionGroupID string, key string, targetNodeID string, contextMode string, contextSource WorkflowContextSource, parameters []WorkflowParameter) error { _ = edgeID for _, field := range []struct{ name, value string }{{"workflow_id", workflowID}, {"transition_group_id", transitionGroupID}, {"target_node_id", targetNodeID}, {"context_mode", contextMode}} { if err := validateRequired(field.name, field.value); err != nil { return err } } if err := validateModelKey("key", key); err != nil { return err } - if err := validateWorkflowContextSource(contextSource); err != nil { + if err := validateWorkflowContextModeAndSource(contextMode, contextSource); err != nil { return err } if len(parameters) > WorkflowGraphDraftMaxFieldsPerEntity { return workflowRequestError(WorkflowRequestErrorTooLong, "parameters", fmt.Sprintf("parameters must be <= %d", WorkflowGraphDraftMaxFieldsPerEntity)) } return nil } + +func validateWorkflowContextModeAndSource(contextMode string, source WorkflowContextSource) error { + if err := validateWorkflowContextSource(source); err != nil { + return err + } + if strings.TrimSpace(contextMode) != "continue_session" { + switch strings.TrimSpace(source.Kind) { + case "", "immediate_source": + return nil + default: + return workflowRequestError(WorkflowRequestErrorInvalidValue, "context_source.kind", "non-immediate context_source requires continue_session") + } + } + return nil +}for _, edge := range graph.Edges { + if err := validateWorkflowContextModeAndSource(edge.ContextMode, edge.ContextSource); err != nil { + return err + } if len(edge.Parameters) > WorkflowGraphDraftMaxFieldsPerEntity { return workflowRequestError(WorkflowRequestErrorTooLong, "graph.edges.parameters", fmt.Sprintf("parameters must be <= %d", WorkflowGraphDraftMaxFieldsPerEntity)) } }Also applies to: 1448-1452
🤖 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 `@shared/serverapi/workflow.go` around lines 1243 - 1278, validateWorkflowEdgeFields currently validates context_mode and validateWorkflowContextSource validates context_source in isolation, which allows disallowed combos (e.g., context_mode != "continue_session" with context_source.Kind == "selected_node"/"previous_target") to be persisted; update validateWorkflowEdgeFields to enforce the combined rule: if contextMode trimmed != "continue_session" then ensure source.Kind is either "" or "immediate_source" (return WorkflowRequestErrorInvalidValue for "context_source.kind" otherwise), and if contextMode == "continue_session" allow "selected_node"/"previous_target" but still call validateWorkflowContextSource to check NodeKey rules; apply the same combined validation logic to the graph-draft/graph-save path where WorkflowGraphDraftMaxFieldsPerEntity is checked so both edge-update and graph-save flows reject invalid context_mode + context_source combos.apps/desktop/src/vendor/xyflow-react-types.ts (1)
34-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
reconnectabletyping in xyflow React type shim
onReconnect,onReconnectStart/onReconnectEnd,edgesReconnectable,reconnectRadius, andsourceHandle/targetHandleshapes inapps/desktop/src/vendor/xyflow-react-types.tsalign with the@xyflow/react12.10 API.Edge.reconnectableis incomplete: it should acceptboolean | "source" | "target"(HandleType), but the shim currently definesreconnectable?: boolean(around lines 34-45). This will incorrectly reject valid"source"/"target"reconnection configurations.🤖 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 `@apps/desktop/src/vendor/xyflow-react-types.ts` around lines 34 - 52, The Edge type's reconnectable field is typed too narrowly; update the reconnectable property in the Edge type definition (export type Edge<...>) to accept boolean | "source" | "target" (the HandleType used by `@xyflow/react`) so that existing reconnection configs like "source" and "target" are valid; ensure the Connection type and any related props (sourceHandle/targetHandle) remain unchanged but verify any code referencing Edge.reconnectable treats the new union values correctly.server/workflowstore/transitions.go (1)
198-207:⚠️ Potential issue | 🟠 MajorFix context resolution when
sourceRunis missing (hasSourceRun == false)
- In
server/workflowstore/transitions.gotheresolveContextSourceRun(..., sourceRun.PlacementID, &sourceRun, sourceSnapshot, ...)call is reached even whensourceRun/sourceSnapshotare zero-values. ForContextSourceImmediateSource,resolveContextSourceRunreturns emptyrunID/sessionID(no error) whenimmediatepoints to a zero-value record;targetRunMetadatathen stores/omitssource_run_id/source_session_id, leading to silent incorrect context.- For
ContextSourcePreviousTarget, passing emptysourcePlacementIDdisables batch scoping (batchScoped=false), so the “latest completed run” lookup may be wrong across batches.- The
workflow.Parametercopy concern at lines ~337-338 and ~364-365 doesn’t apply:workflow.Parameteris only{ Key string; Description string }, soappend([]workflow.Parameter(nil), ...)is sufficient.🤖 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 `@server/workflowstore/transitions.go` around lines 198 - 207, The code calls resolveContextSourceRun(...) even when there is no sourceRun (hasSourceRun==false), which yields empty IDs and wrong batch scoping; fix by adding a guard after resolvedContextSourceRunFromMetadata: only call resolveContextSourceRun when hasSourceRun is true (or when sourcePlacementID is non-zero for ContextSourcePreviousTarget), and for ContextSourceImmediateSource check the immediate record for a zero-value before calling resolveContextSourceRun (if immediate is zero, do not call resolveContextSourceRun and ensure targetRunMetadata does not get empty source_run_id/source_session_id). Use the symbols resolvedContextSourceRunFromMetadata, resolveContextSourceRun, ContextSourceImmediateSource, ContextSourcePreviousTarget and ensure batchScoped logic remains enabled only when a valid sourcePlacementID is provided.
🧹 Nitpick comments (3)
shared/serverapi/workflow_test.go (1)
44-48: 💤 Low valueConsider initializing the oversized parameter slice elements.
The test creates zero-valued
WorkflowParameterstructs. If parameter validation were to check individual parameter validity before checking the slice length, this test could fail for the wrong reason. However, this matches the existing pattern used for oversized nodes on line 186, and the tests pass, suggesting validation checks counts first.♻️ Optional: Initialize parameters for explicit coverage
oversizedEdge := validEdge -oversizedEdge.Parameters = make([]WorkflowParameter, WorkflowGraphDraftMaxFieldsPerEntity+1) +oversizedEdge.Parameters = make([]WorkflowParameter, WorkflowGraphDraftMaxFieldsPerEntity+1) +for i := range oversizedEdge.Parameters { + oversizedEdge.Parameters[i] = WorkflowParameter{Key: "p", Description: "d"} +} if err := oversizedEdge.Validate(); err == nil || !strings.Contains(err.Error(), "parameters") {🤖 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 `@shared/serverapi/workflow_test.go` around lines 44 - 48, The oversizedEdge test currently constructs Parameters as a slice of zero-valued WorkflowParameter which could mask individual parameter validation; update the test that creates oversizedEdge (the variable oversizedEdge and its Parameters slice) to populate each element with a minimal valid WorkflowParameter so the failure is driven by exceeding WorkflowGraphDraftMaxFieldsPerEntity rather than per-parameter validation, leaving the Validate() call and the error assertion unchanged.server/workflowstore/joins.go (1)
190-211: 💤 Low valueInconsistent return semantics:
readybool is now redundant.After the refactor,
selectedJoinOutputValuesonly returns(nil, false, error)on failure or(out, true, nil)on success. There's no valid(_, false, nil)"not ready yet" state, making thereadyreturn value dead information and the caller'sif !ready { return ..., nil }check at lines 77-79 unreachable without an accompanying error.Consider either:
- Removing the
readybool return since it's now redundant, or- Restoring a legitimate "not ready" case (return
nil, false, nil) if aggregation should silently wait when values are missingOption 1: Simplify return signature
-func selectedJoinOutputValues(join nodeContractSnapshot, outEdge edgeContractSnapshot, arrivals []joinArrival) (map[string]string, bool, error) { +func selectedJoinOutputValues(join nodeContractSnapshot, outEdge edgeContractSnapshot, arrivals []joinArrival) (map[string]string, error) { out := map[string]string{} for _, requirement := range outEdge.OutputRequirements { parameterKey := strings.TrimSpace(requirement.FieldName) if parameterKey == "" { continue } value := "" for _, arrival := range arrivals { candidate := arrival.OutputValues[parameterKey] if strings.TrimSpace(candidate) != "" { value = candidate break } } if strings.TrimSpace(value) == "" { - return nil, false, fmt.Errorf("join node %q missing aggregate parameter %q", join.ID, parameterKey) + return nil, fmt.Errorf("join node %q missing aggregate parameter %q", join.ID, parameterKey) } out[parameterKey] = value } - return out, true, nil + return out, nil }🤖 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 `@server/workflowstore/joins.go` around lines 190 - 211, The function selectedJoinOutputValues currently returns a redundant ready bool; change its signature to return (map[string]string, error) and remove the ready return value and logic inside selectedJoinOutputValues (return out, nil on success and nil, fmt.Errorf(...) on failure), then update all call sites that expect (map,string,bool,error) — e.g. the caller that does if !ready { return ..., nil } — to instead check error != nil for failure and nil error for success (remove the unreachable ready-check branch), and update any related tests and usages to the new two-value return shape.server/workflowstore/tasks.go (1)
377-389: ⚡ Quick winAdd a focused regression test for the start-edge metadata path.
This branch now serializes derived transition metadata into the initial run, but the updated suite still doesn't assert the
metadata_json/edge-snapshot contents created byStartTask. A small read-back assertion here would lock down the new contract separately from the completion path.As per coding guidelines,
**/*_test.go: All business logic covered by tests. Production code is written to be unit-testable
🤖 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 `@apps/desktop/src/vendor/xyflow-react-types.ts`:
- Around line 34-52: The Edge type's reconnectable field is typed too narrowly;
update the reconnectable property in the Edge type definition (export type
Edge<...>) to accept boolean | "source" | "target" (the HandleType used by
`@xyflow/react`) so that existing reconnection configs like "source" and "target"
are valid; ensure the Connection type and any related props
(sourceHandle/targetHandle) remain unchanged but verify any code referencing
Edge.reconnectable treats the new union values correctly.
In `@server/workflowstore/transitions.go`:
- Around line 198-207: The code calls resolveContextSourceRun(...) even when
there is no sourceRun (hasSourceRun==false), which yields empty IDs and wrong
batch scoping; fix by adding a guard after resolvedContextSourceRunFromMetadata:
only call resolveContextSourceRun when hasSourceRun is true (or when
sourcePlacementID is non-zero for ContextSourcePreviousTarget), and for
ContextSourceImmediateSource check the immediate record for a zero-value before
calling resolveContextSourceRun (if immediate is zero, do not call
resolveContextSourceRun and ensure targetRunMetadata does not get empty
source_run_id/source_session_id). Use the symbols
resolvedContextSourceRunFromMetadata, resolveContextSourceRun,
ContextSourceImmediateSource, ContextSourcePreviousTarget and ensure batchScoped
logic remains enabled only when a valid sourcePlacementID is provided.
In `@shared/serverapi/workflow.go`:
- Around line 1243-1278: validateWorkflowEdgeFields currently validates
context_mode and validateWorkflowContextSource validates context_source in
isolation, which allows disallowed combos (e.g., context_mode !=
"continue_session" with context_source.Kind ==
"selected_node"/"previous_target") to be persisted; update
validateWorkflowEdgeFields to enforce the combined rule: if contextMode trimmed
!= "continue_session" then ensure source.Kind is either "" or "immediate_source"
(return WorkflowRequestErrorInvalidValue for "context_source.kind" otherwise),
and if contextMode == "continue_session" allow "selected_node"/"previous_target"
but still call validateWorkflowContextSource to check NodeKey rules; apply the
same combined validation logic to the graph-draft/graph-save path where
WorkflowGraphDraftMaxFieldsPerEntity is checked so both edge-update and
graph-save flows reject invalid context_mode + context_source combos.
---
Nitpick comments:
In `@server/workflowstore/joins.go`:
- Around line 190-211: The function selectedJoinOutputValues currently returns a
redundant ready bool; change its signature to return (map[string]string, error)
and remove the ready return value and logic inside selectedJoinOutputValues
(return out, nil on success and nil, fmt.Errorf(...) on failure), then update
all call sites that expect (map,string,bool,error) — e.g. the caller that does
if !ready { return ..., nil } — to instead check error != nil for failure and
nil error for success (remove the unreachable ready-check branch), and update
any related tests and usages to the new two-value return shape.
In `@shared/serverapi/workflow_test.go`:
- Around line 44-48: The oversizedEdge test currently constructs Parameters as a
slice of zero-valued WorkflowParameter which could mask individual parameter
validation; update the test that creates oversizedEdge (the variable
oversizedEdge and its Parameters slice) to populate each element with a minimal
valid WorkflowParameter so the failure is driven by exceeding
WorkflowGraphDraftMaxFieldsPerEntity rather than per-parameter validation,
leaving the Validate() call and the error assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4c916e4-a93f-4a1c-b9d1-98faf42a686f
📒 Files selected for processing (88)
apps/desktop/src/api/client.test.tsapps/desktop/src/api/client.tsapps/desktop/src/api/index.tsapps/desktop/src/api/models.tsapps/desktop/src/api/schemas/common.tsapps/desktop/src/api/schemas/workflow.tsapps/desktop/src/app/workflowDeleteConfirmRoute.tsxapps/desktop/src/features/workflow-editor/WorkflowDeleteConfirmationWindow.test.tsxapps/desktop/src/features/workflow-editor/WorkflowDeleteConfirmationWindow.tsxapps/desktop/src/features/workflow-editor/WorkflowEditorRoute.test.tsxapps/desktop/src/features/workflow-editor/WorkflowEditorRoute.tsxapps/desktop/src/features/workflow-editor/WorkflowGraphCanvas.tsxapps/desktop/src/features/workflow-editor/WorkflowGraphCanvasEdgeInteraction.test.tsxapps/desktop/src/features/workflow-editor/WorkflowGraphEdge.test.tsxapps/desktop/src/features/workflow-editor/WorkflowGraphNodes.tsxapps/desktop/src/features/workflow-editor/WorkflowInspectorPrimitives.test.tsxapps/desktop/src/features/workflow-editor/WorkflowInspectorSidebar.tsxapps/desktop/src/features/workflow-editor/workflow-editor.cssapps/desktop/src/features/workflow-editor/workflowDeleteConfirmationModel.test.tsapps/desktop/src/features/workflow-editor/workflowDeleteConfirmationModel.tsapps/desktop/src/features/workflow-editor/workflowDeleteConfirmationPolicy.test.tsapps/desktop/src/features/workflow-editor/workflowDeleteConfirmationPolicy.tsapps/desktop/src/features/workflow-editor/workflowEditorDraft.test.tsapps/desktop/src/features/workflow-editor/workflowEditorDraft.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphEdgeMutations.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphMutationFixtures.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphMutationHelpers.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphMutationTypes.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphMutations.test.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphMutations.tsapps/desktop/src/features/workflow-editor/workflowGraphCanvasInteractions.test.tsapps/desktop/src/features/workflow-editor/workflowGraphCanvasInteractions.tsapps/desktop/src/features/workflow-editor/workflowGraphEdges.tsapps/desktop/src/features/workflow-editor/workflowGraphLayout.test.tsapps/desktop/src/features/workflow-editor/workflowGraphLayout.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutGeometry.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutGroups.test.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutNodes.tsapps/desktop/src/features/workflow-editor/workflowPromptTemplatePlaceholders.test.tsapps/desktop/src/features/workflow-editor/workflowPromptTemplatePlaceholders.tsapps/desktop/src/features/workflow/WorkflowValidationIssues.test.tsxapps/desktop/src/i18n/workflowEditorEn.tsapps/desktop/src/vendor/xyflow-react-types.tsdocs/dev/specs/terminology.mddocs/dev/specs/workflow-editor.mdprompts/workflow/structured_completion_instructions.mdprompts/workflow/tool_completion_instructions.mdprompts/workflow/workflow_task_instructions.mdserver/metadata/migrations/00029_workflow_previous_target_context_source.up.sqlserver/metadata/migrations/00030_workflow_transition_invocation_contract.up.sqlserver/metadata/queries.sqlserver/metadata/sqlitegen/models.goserver/metadata/sqlitegen/queries.sql.goserver/metadata/workflow_schema_test.goserver/runtime/workflow_completion_test.goserver/tools/definitions.goserver/tools/schemas/complete_node.jsonserver/workflow/derived_wiring.goserver/workflow/derived_wiring_test.goserver/workflow/prompt_refs.goserver/workflow/types.goserver/workflow/validation.goserver/workflow/validation_test.goserver/workflowrunner/starter.goserver/workflowrunner/starter_test.goserver/workflowruntime/completion.goserver/workflowruntime/completion_test.goserver/workflowstore/context_sources.goserver/workflowstore/graph_records.goserver/workflowstore/graph_save.goserver/workflowstore/joins.goserver/workflowstore/manual_moves.goserver/workflowstore/queries.goserver/workflowstore/queries/latest_transition_output_values.sqlserver/workflowstore/queries/update_workflow_edge.sqlserver/workflowstore/queries/upsert_workflow_edge.sqlserver/workflowstore/run_snapshot.goserver/workflowstore/runs.goserver/workflowstore/store.goserver/workflowstore/store_test.goserver/workflowstore/tasks.goserver/workflowstore/transitions.goserver/workflowsvc/service.goserver/workflowview/service.goserver/workflowview/service_test.goshared/serverapi/workflow.goshared/serverapi/workflow_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.ts`:
- Around line 65-67: The aligned-join routing path currently discards endpoint
port offsets by calling branchJoinEdgeRoutePoints(...) and
adjustAlignedJoinEndpointRoutePoints(...) which force center points; update the
routing so it preserves model.sourcePort and model.targetPort offsets when
computing routedPoints and when calling
adjustAlignedJoinEndpointRoutePoints/branchJoinEdgeRoutePoints — pass the
source/target port offsets (from model.sourcePort and model.targetPort or the
computed sourceHandle/targetHandle positions) into those helpers or modify the
helpers to accept/use a portOffset argument so endpoint alignment uses the
actual offset values instead of forcing center points; ensure routedPoints are
adjusted using those offsets wherever sourceAligned or targetAligned is used.
In `@prompts/skills/builder-workflows/SKILL.md`:
- Line 58: The docs claim the `edges` output includes prompt template fields,
but the workflow inspect CLI handler currently omits them; update the "workflow
inspect" command's edge-formatting logic (the function that serializes/prints
edges in the CLI inspect implementation) to include the prompt template fields
from each edge (e.g., add the prompt template and any template variables/IDs
into the printed edge record alongside context mode and approval flags) and
format them consistently with the other edge fields so the CLI output matches
the SKILL.md documentation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71470f95-5e9d-40c2-a75f-ac6403cdaff2
📒 Files selected for processing (19)
apps/desktop/src/features/workflow-editor/WorkflowInspectorSidebar.tsxapps/desktop/src/features/workflow-editor/workflowEditorDraft.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphEdgeMutations.tsapps/desktop/src/features/workflow-editor/workflowGraphLayout.test.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.tscli/builder/help/workflow.txtcli/builder/workflow_command.gocli/builder/workflow_command_test.goprompts/skills/builder-workflows/SKILL.mdscripts/capture-gui-gap-proof.shserver/core/composition_test.goserver/workflow/derived_wiring.goserver/workflow/derived_wiring_test.goserver/workflowscheduler/service_test.goserver/workflowstore/run_snapshot.goserver/workflowsvc/service_test.goserver/workflowview/service.goserver/workflowview/service_test.goserver/worktree/task_worktree_test.go
✅ Files skipped from review due to trivial changes (2)
- cli/builder/help/workflow.txt
- server/workflowscheduler/service_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/workflowstore/run_snapshot.go
- server/workflowview/service.go
- server/workflow/derived_wiring.go
- apps/desktop/src/features/workflow-editor/workflowEditorDraft.ts
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: 2
🤖 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 `@apps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.ts`:
- Around line 65-67: The aligned-join routing path currently discards endpoint
port offsets by calling branchJoinEdgeRoutePoints(...) and
adjustAlignedJoinEndpointRoutePoints(...) which force center points; update the
routing so it preserves model.sourcePort and model.targetPort offsets when
computing routedPoints and when calling
adjustAlignedJoinEndpointRoutePoints/branchJoinEdgeRoutePoints — pass the
source/target port offsets (from model.sourcePort and model.targetPort or the
computed sourceHandle/targetHandle positions) into those helpers or modify the
helpers to accept/use a portOffset argument so endpoint alignment uses the
actual offset values instead of forcing center points; ensure routedPoints are
adjusted using those offsets wherever sourceAligned or targetAligned is used.
In `@prompts/skills/builder-workflows/SKILL.md`:
- Line 58: The docs claim the `edges` output includes prompt template fields,
but the workflow inspect CLI handler currently omits them; update the "workflow
inspect" command's edge-formatting logic (the function that serializes/prints
edges in the CLI inspect implementation) to include the prompt template fields
from each edge (e.g., add the prompt template and any template variables/IDs
into the printed edge record alongside context mode and approval flags) and
format them consistently with the other edge fields so the CLI output matches
the SKILL.md documentation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71470f95-5e9d-40c2-a75f-ac6403cdaff2
📒 Files selected for processing (19)
apps/desktop/src/features/workflow-editor/WorkflowInspectorSidebar.tsxapps/desktop/src/features/workflow-editor/workflowEditorDraft.tsapps/desktop/src/features/workflow-editor/workflowEditorGraphEdgeMutations.tsapps/desktop/src/features/workflow-editor/workflowGraphLayout.test.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.tscli/builder/help/workflow.txtcli/builder/workflow_command.gocli/builder/workflow_command_test.goprompts/skills/builder-workflows/SKILL.mdscripts/capture-gui-gap-proof.shserver/core/composition_test.goserver/workflow/derived_wiring.goserver/workflow/derived_wiring_test.goserver/workflowscheduler/service_test.goserver/workflowstore/run_snapshot.goserver/workflowsvc/service_test.goserver/workflowview/service.goserver/workflowview/service_test.goserver/worktree/task_worktree_test.go
✅ Files skipped from review due to trivial changes (2)
- cli/builder/help/workflow.txt
- server/workflowscheduler/service_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/workflowstore/run_snapshot.go
- server/workflowview/service.go
- server/workflow/derived_wiring.go
- apps/desktop/src/features/workflow-editor/workflowEditorDraft.ts
🛑 Comments failed to post (2)
apps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.ts (1)
65-67:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAligned-join route endpoints ignore endpoint-port offsets
On Line 65 and Line 67, aligned-join routing now goes through helpers that force center points (Line 102 and Line 124) while
model.sourcePort/model.targetPortare unused. That can misalignroutePointsfromsourceHandle/targetHandlewhen endpoints are intentionally offset.💡 Proposed fix
export function workflowGraphEdgeRoutePoints( model: Readonly<{ sourceNodeID: string; sourcePort: WorkflowGraphEndpointPort; targetNodeID: string; targetPort: WorkflowGraphEndpointPort; }>, @@ if (isBranchToAlignedJoin(source, target, targetAligned)) { - return branchJoinEdgeRoutePoints(source, target, options.groupNodeByGroupID); + return branchJoinEdgeRoutePoints(source, target, model, options.groupNodeByGroupID); } - return adjustAlignedJoinEndpointRoutePoints(routedPoints, source, target, { sourceAligned, targetAligned }); + return adjustAlignedJoinEndpointRoutePoints(routedPoints, source, target, model, { + sourceAligned, + targetAligned, + }); } @@ function branchJoinEdgeRoutePoints( source: WorkflowGraphNodeRect, target: WorkflowGraphNodeRect, + model: Readonly<{ sourcePort: WorkflowGraphEndpointPort; targetPort: WorkflowGraphEndpointPort }>, groupNodeByGroupID: ReadonlyMap<string, WorkflowGraphNode>, ): readonly WorkflowGraphPoint[] { - const start = centeredSourcePoint(source); - const end = joinCenterTargetPoint(target); + const start = sourceHandlePoint(source, model.sourcePort); + const end = targetHandlePoint(target, model.targetPort); @@ function adjustAlignedJoinEndpointRoutePoints( points: readonly WorkflowGraphPoint[], source: WorkflowGraphNodeRect, target: WorkflowGraphNodeRect, + model: Readonly<{ sourcePort: WorkflowGraphEndpointPort; targetPort: WorkflowGraphEndpointPort }>, flags: Readonly<{ sourceAligned: boolean; targetAligned: boolean }>, ): readonly WorkflowGraphPoint[] { @@ if (flags.sourceAligned) { - adjusted[0] = centeredSourcePoint(source); + adjusted[0] = sourceHandlePoint(source, model.sourcePort); } if (flags.targetAligned) { - adjusted[adjusted.length - 1] = joinCenterTargetPoint(target); + adjusted[adjusted.length - 1] = targetHandlePoint(target, model.targetPort); } return compactRoutePoints(adjusted); } + +function sourceHandlePoint(rect: WorkflowGraphNodeRect, port: WorkflowGraphEndpointPort): WorkflowGraphPoint { + return { x: rect.x + rect.width, y: rect.y + port.y }; +} + +function targetHandlePoint(rect: WorkflowGraphNodeRect, port: WorkflowGraphEndpointPort): WorkflowGraphPoint { + return { x: rect.x, y: rect.y + port.y }; +}Also applies to: 97-135
🤖 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 `@apps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.ts` around lines 65 - 67, The aligned-join routing path currently discards endpoint port offsets by calling branchJoinEdgeRoutePoints(...) and adjustAlignedJoinEndpointRoutePoints(...) which force center points; update the routing so it preserves model.sourcePort and model.targetPort offsets when computing routedPoints and when calling adjustAlignedJoinEndpointRoutePoints/branchJoinEdgeRoutePoints — pass the source/target port offsets (from model.sourcePort and model.targetPort or the computed sourceHandle/targetHandle positions) into those helpers or modify the helpers to accept/use a portOffset argument so endpoint alignment uses the actual offset values instead of forcing center points; ensure routedPoints are adjusted using those offsets wherever sourceAligned or targetAligned is used.prompts/skills/builder-workflows/SKILL.md (1)
58-58:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
workflow inspectprompt visibility is documented incorrectly.Line 58 says the
edgessection records prompts, but current CLI inspect output (cli/builder/workflow_command.go, Line 659-670) does not print prompt template fields. This will mislead users trying to audit branch prompts.📌 Suggested doc fix
-The first section lists node IDs, keys, kinds, display names, and subagent roles. The `transition_groups` section maps source node IDs to transition IDs. The `edges` section maps transition groups to target node IDs and records prompts, context mode, and approval requirements. +The first section lists node IDs, keys, kinds, display names, and subagent roles. The `transition_groups` section maps source node IDs to transition IDs. The `edges` section maps transition groups to target node IDs and records context mode and approval requirements.📝 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.The first section lists node IDs, keys, kinds, display names, and subagent roles. The `transition_groups` section maps source node IDs to transition IDs. The `edges` section maps transition groups to target node IDs and records context mode and approval requirements.🤖 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 `@prompts/skills/builder-workflows/SKILL.md` at line 58, The docs claim the `edges` output includes prompt template fields, but the workflow inspect CLI handler currently omits them; update the "workflow inspect" command's edge-formatting logic (the function that serializes/prints edges in the CLI inspect implementation) to include the prompt template fields from each edge (e.g., add the prompt template and any template variables/IDs into the printed edge record alongside context mode and approval flags) and format them consistently with the other edge fields so the CLI output matches the SKILL.md documentation.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/features/workflow-editor/workflowGraphLayout.test.ts (1)
213-217: ⚡ Quick winPotential runtime error:
endpointPortreturnsundefinedfor non-string handles.The
endpointPortfunction returnsundefinedwhenhandleis not a string (lines 215-217), butassertEndpointHandle(line 236) throws an error ifport === undefined. This is correct behavior for assertions, but consider whether the early return at lines 215-217 is necessary—React Flow's handle IDs should always be strings when routing is configured. Ifnull/undefinedhandles indicate a test or implementation defect, consider throwing an error here instead of silently returningundefined.🤖 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 `@apps/desktop/src/features/workflow-editor/workflowGraphLayout.test.ts` around lines 213 - 217, The endpointPort function currently returns undefined for non-string handles which leads to assertEndpointHandle throwing later; instead make endpointPort validate and fail-fast: replace the early return in endpointPort so that if handle is null/undefined or not a string it throws a clear error (e.g., TypeError or Error) with context (include the handle value), so callers like assertEndpointHandle receive a deterministic failure; update endpointPort (and any callers relying on undefined behavior) to assume a string handle after this change.apps/desktop/src/features/workflow-editor/workflowGraphLayoutGroups.test.ts (1)
102-133: ⚡ Quick winCode duplication:
endpointPort,isEndpointPort, andisRecordhelpers are duplicated across test files.The helpers
endpointPort,isEndpointPort, andisRecordappear in bothworkflowGraphLayout.test.tsandworkflowGraphLayoutGroups.test.ts. InworkflowGraphLayout.test.ts,endpointPortreturnsundefinedfor invalid handles, while here it throws an error. This inconsistency could lead to confusion.♻️ Proposed consolidation
Consider extracting these shared test helpers into a separate file (e.g.,
workflowGraphLayoutTestHelpers.ts) and reconciling the error-handling strategy:// apps/desktop/src/features/workflow-editor/workflowGraphLayoutTestHelpers.ts export function endpointPort( node: WorkflowGraphNode, handleID: string | null | undefined, side: "source" | "target", ): Readonly<{ id: string; side: "source" | "target"; y: number }> { if (typeof handleID !== "string" || node.data.entityKind !== "node") { throw new Error(`Endpoint port ${handleID ?? ""} not found for ${node.id}`); } const ports: unknown = node.data.endpointPorts; if (!Array.isArray(ports)) { throw new Error(`Endpoint ports not found for ${node.id}`); } const port = ports.filter(isEndpointPort).find((item) => item.id === handleID && item.side === side); if (port === undefined) { throw new Error(`Endpoint port ${handleID} not found for ${node.id}`); } return port; } // ... export isEndpointPort, isRecord, etc.Then import and use in both test files.
🤖 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 `@apps/desktop/src/features/workflow-editor/workflowGraphLayoutGroups.test.ts` around lines 102 - 133, Consolidate the duplicated test helpers by extracting endpointPort, isEndpointPort, and isRecord into a single shared test helpers module (e.g., workflowGraphLayoutTestHelpers.ts) and import that module from both workflowGraphLayout.test.ts and workflowGraphLayoutGroups.test.ts; reconcile the inconsistent behavior by adopting a single error-handling strategy (use the throwing variant shown here) so endpointPort always throws on invalid handles, update endpointPoint to call the imported endpointPort, and replace the in-file helper copies with imports of isEndpointPort/isRecord to avoid duplication.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/features/workflow-editor/workflowGraphLayout.test.ts`:
- Around line 213-217: The endpointPort function currently returns undefined for
non-string handles which leads to assertEndpointHandle throwing later; instead
make endpointPort validate and fail-fast: replace the early return in
endpointPort so that if handle is null/undefined or not a string it throws a
clear error (e.g., TypeError or Error) with context (include the handle value),
so callers like assertEndpointHandle receive a deterministic failure; update
endpointPort (and any callers relying on undefined behavior) to assume a string
handle after this change.
In `@apps/desktop/src/features/workflow-editor/workflowGraphLayoutGroups.test.ts`:
- Around line 102-133: Consolidate the duplicated test helpers by extracting
endpointPort, isEndpointPort, and isRecord into a single shared test helpers
module (e.g., workflowGraphLayoutTestHelpers.ts) and import that module from
both workflowGraphLayout.test.ts and workflowGraphLayoutGroups.test.ts;
reconcile the inconsistent behavior by adopting a single error-handling strategy
(use the throwing variant shown here) so endpointPort always throws on invalid
handles, update endpointPoint to call the imported endpointPort, and replace the
in-file helper copies with imports of isEndpointPort/isRecord to avoid
duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45089c03-00e1-4481-bcf4-800789f05f17
📒 Files selected for processing (4)
apps/desktop/src/features/workflow-editor/workflowGraphLayout.test.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutEdgeRoutes.tsapps/desktop/src/features/workflow-editor/workflowGraphLayoutGroups.test.tsprompts/skills/builder-workflows/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- prompts/skills/builder-workflows/SKILL.md
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.
Summary
transition+ top-level parameter completion contract, and inert legacy node-owned prompt/input/output handling.Verification
pnpm --dir apps/desktop exec vitest run src/features/workflow-editor/workflowPromptTemplatePlaceholders.test.ts src/features/workflow-editor/workflowEditorDraft.test.ts src/features/workflow-editor/WorkflowEditorRoute.test.tsx src/features/workflow-editor/workflowDeleteConfirmationPolicy.test.ts src/features/workflow-editor/workflowDeleteConfirmationModel.test.ts src/features/workflow-editor/WorkflowDeleteConfirmationWindow.test.tsx src/features/workflow-editor/workflowEditorGraphMutations.test.ts src/features/workflow-editor/workflowGraphLayout.test.ts src/features/workflow-editor/workflowGraphLayoutGroups.test.ts src/features/workflow-editor/WorkflowInspectorPrimitives.test.tsx src/features/workflow/WorkflowValidationIssues.test.tsx src/api/client.test.ts src/features/workflow-editor/WorkflowGraphEdge.test.tsx --reporter=dotpnpm --dir apps/desktop exec tsc -b --pretty falsegit diff --check origin/main...HEAD./scripts/build.sh --output ./bin/builderNotes
Summary by CodeRabbit
New Features
Improvements
Behavior