Skip to content

fix: restore dynamic_fork_tasks_param in DynamicForkTask (re-fixes #349)#444

Merged
nthmost-orkes merged 1 commit into
mainfrom
fix/dynamic-fork-tasks-param
Jul 23, 2026
Merged

fix: restore dynamic_fork_tasks_param in DynamicForkTask (re-fixes #349)#444
nthmost-orkes merged 1 commit into
mainfrom
fix/dynamic-fork-tasks-param

Conversation

@nthmost-orkes

@nthmost-orkes nthmost-orkes commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Re-applies the one-line fix from PR #352, which was dropped from main when the branch was subsequently force-pushed/rebased (likely during the Agentspan consolidation). GitHub still shows #352 as "Merged" because it records the merge event, but commit a561d90 is not an ancestor of current main.

Actual timeline:

Setting dynamic_fork_join_tasks_param alongside dynamic_fork_tasks_input_param_name violates Conductor's validation: only one approach is valid at a time (dynamicForkJoinTasksParam alone, or dynamicForkTasksParam + dynamicForkTasksInputParamName together).

User impact: Users calling DynamicForkTask with separate tasks_param / tasks_input_param_name would get a 400 validation error from the server on workflow registration. Now registers correctly.

Test plan

  • tests/unit/workflow/test_dynamic_fork_task.py — two new unit tests cover the correct param and the join-task variant
  • Integration test generate_dynamic_fork_task in tests/integration/metadata/test_workflow_definition.py already exercises this path against a live server

Fixes #377

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
...onductor/client/workflow/task/dynamic_fork_task.py 100.00% <100.00%> (+100.00%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…param in DynamicForkTask

Fixes the regression introduced in baadd20 that undid PR #352. Setting
dynamic_fork_join_tasks_param alongside dynamic_fork_tasks_input_param_name
violates Conductor's validation (only one approach is valid at a time).

Adds unit tests to prevent this from silently regressing again.

Fixes #377
@nthmost-orkes
nthmost-orkes force-pushed the fix/dynamic-fork-tasks-param branch from ee9377a to 79141a7 Compare July 23, 2026 03:29
@nthmost-orkes
nthmost-orkes requested a review from v1r3n July 23, 2026 03:32
@mp-orkes

mp-orkes commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

The fix seems to be right but the description has the order of things backwards and CI is failing.

This PR #352 was undone by this commit: baadd20

This came first: #274
Then this: #352

A January commit can't revert an October PR. That PR was the fix. How did we "lose" the fix?

@mp-orkes mp-orkes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - but please fix the description. We need to understand how this bug came back.

@nthmost-orkes

Copy link
Copy Markdown
Contributor Author

The fix seems to be right but the description has the order of things backwards and CI is failing.

CI failure seems unrelated -- can look into it later today, posting in Slack channel for more visibility in case someone else can get to it first. (@chrishagglund-ship-it ?)

@nthmost-orkes
nthmost-orkes merged commit 7a951aa into main Jul 23, 2026
14 of 18 checks passed
@nthmost-orkes
nthmost-orkes deleted the fix/dynamic-fork-tasks-param branch July 23, 2026 20:47
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.

DynamicForkTask sets incompatible parameters causing validation error

2 participants