Skip to content

Only track conditionless edges in the conditionless-edge dedup set#507

Open
PratikDhanave wants to merge 5 commits into
microsoft:mainfrom
PratikDhanave:fix-builder-conditional-edge-dedup
Open

Only track conditionless edges in the conditionless-edge dedup set#507
PratikDhanave wants to merge 5 commits into
microsoft:mainfrom
PratikDhanave:fix-builder-conditional-edge-dedup

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

Summary

Builder.AddDirectEdge appends every connection to wb.conditionlessConnections (builder.go), including conditional edges, but the duplicate check fires only when condition == nil and treats any entry in that list as a pre-existing conditionless edge. So a conditional edge added first on a source→target pair poisons the dedup set: a subsequent legitimate conditionless edge on the same pair is wrongly rejected.

Concrete trigger:

b.AddDirectEdge(a, b, false, func(any) bool { return true }) // conditional
b.AddEdge(a, b)                                              // conditionless
// => wb.err = "an edge from 'a' to 'b' already exists without a condition"

even though no conditionless edge exists. Via AddChain (which calls AddDirectEdge with idempotent=true), the same poisoning instead silently drops the chain edge (the dedup returns early without adding it).

Fix

Guard the append with condition == nil, so only conditionless edges populate the conditionless-edge dedup set — matching the field's name and sole purpose (it is read only by that dedup check).

Public API

No exported symbols change.

Tests

  • TestBuilder_ConditionalEdgeDoesNotBlockConditionlessEdge — conditional then conditionless edge on the same pair now builds without error (fails before this change with the misleading duplicate error).
  • TestBuilder_ConditionalEdgeDoesNotDropIdempotentConditionlessEdge — the idempotent path doesn't silently drop the conditionless edge.

go test ./workflow passes; gofmt clean.

Builder.AddDirectEdge appended every connection to conditionlessConnections,
including conditional edges, but the duplicate check only fires for
condition==nil and treats any entry as a pre-existing conditionless edge. So
a conditional edge added first on a source->target pair made a subsequent
legitimate conditionless edge on the same pair fail the dedup check with
"an edge ... already exists without a condition" (or, via AddChain's
idempotent path, be silently dropped).

Guard the append with condition==nil so only conditionless edges populate
the dedup set, matching the field's name and purpose.

Adds tests for both the error path and the idempotent-drop path.
@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 16, 2026 08:32
Copilot AI review requested due to automatic review settings July 16, 2026 08:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in workflow.Builder.AddDirectEdge where conditional edges were incorrectly recorded in the conditionless-edge dedup tracking set, causing subsequent legitimate conditionless edges on the same source→target pair to be rejected (or silently dropped on idempotent paths like AddChain).

Changes:

  • Only append to wb.conditionlessConnections when condition == nil, aligning the data structure with its intended purpose.
  • Add regression tests covering “conditional then conditionless” and the idempotent conditionless-edge path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
workflow/builder.go Fixes conditionless-edge dedup bookkeeping so conditional edges don’t poison the conditionless dedup set.
workflow/builder_conditional_edge_test.go Adds regression tests for conditional-vs-conditionless edge interactions and idempotent insertion behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread workflow/builder_conditional_edge_test.go Outdated
Strengthen the idempotent test to inspect the built workflow's edges and
confirm both a conditional and a conditionless edge from start exist,
rather than only checking that Build succeeded (which held even when the
edge was silently dropped).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants