Skip to content

Conversation

@lokitoth
Copy link
Member

@lokitoth lokitoth commented Aug 26, 2025

Before this change, Executorish binding has some unintuitive behaviour. When a user adds an executor with an id of an executor that already exists, we silently replace it, if the user provides it inside of add_edge. When a user introduces an executor via an unbound id, the user must bind it via BindExecutor, even though the registration is created implicitly when an edge id added.

The change will remove the invisible update in favor of a "best efforts" check of type and instance equality.

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass, and I have added new tests where possible
  • I didn't break anyone 😄

Right now Executorish binding has some unintutitive behaviour. When a user adds an eecutor with an id of an executor that already exists, we silently replace it, if the user provides it inside of add_edge. When a user introduces an executor via an unbound id, the user must bind it via BindExecutor, even though the registration is created implicitly when an edge id added.

The change will remove the invisible update in favor of a "best efforts" check of type and instance equality.
Copilot AI review requested due to automatic review settings August 26, 2025 18:10
@github-actions github-actions bot changed the title Make WorkflowBuilder more intuititve .NET: Make WorkflowBuilder more intuititve Aug 26, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the WorkflowBuilder's executor binding behavior by replacing silent replacement with validation checks. Instead of automatically overwriting existing executors with the same ID, the system now validates that new bindings match existing ones by type and instance reference.

Key changes:

  • Added validation to prevent binding executors with same ID but different types or instances
  • Enhanced ExecutorRegistration to track raw executor data for comparison
  • Added comprehensive test coverage for the new binding validation behavior

Reviewed Changes

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

File Description
WorkflowBuilderSmokeTests.cs Adds test cases covering late binding, implicit binding, and validation scenarios
WorkflowBuilder.cs Implements validation logic to check type and instance equality before binding
ExecutorRegistration.cs Adds RawExecutorishData property to store original executor data for comparison
ExecutorIsh.cs Adds RawData property and updates Registration to include raw data

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@lokitoth lokitoth added this pull request to the merge queue Aug 26, 2025
Merged via the queue into main with commit 78125f0 Aug 26, 2025
14 checks passed
@crickman crickman deleted the dev/dotnet_workflow/executor_binding_improvement branch September 2, 2025 15:25
ReubenBond pushed a commit to ReubenBond/agent-framework that referenced this pull request Oct 28, 2025
* feat: Make WorkflowBuilder more intutitve

Right now Executorish binding has some unintutitive behaviour. When a user adds an eecutor with an id of an executor that already exists, we silently replace it, if the user provides it inside of add_edge. When a user introduces an executor via an unbound id, the user must bind it via BindExecutor, even though the registration is created implicitly when an edge id added.

The change will remove the invisible update in favor of a "best efforts" check of type and instance equality.

* Expand errors when rebinding to disallowed

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants