Python: fix(python): stop executor named __global__ from colliding with the global kwargs slot - #8333
Closed
Yufeng He (he-yufeng) wants to merge 1 commit into
Closed
Yufeng He (he-yufeng) wants to merge 1 commit into
Yufeng He (he-yufeng) wants to merge 1 commit into
Conversation
…the global kwargs slot
_resolve_invocation_kwargs normalized both per-executor and global
invocation kwargs into one flat dict keyed by executor ID, with the
global mapping under the reserved "__global__" key. An executor whose
ID is literally "__global__" then collided with that slot: a plain
mapping keyed "__global__" was normalized as per-executor kwargs, but
_resolve_executor_kwargs read the same key back as the global mapping
and leaked the entry to every untargeted executor in the graph.
Per-executor entries now nest under a separate framework slot
("__per_executor__") so the two namespaces cannot overlap:
- typed WorkflowInvocationKwargs: {"__global__": ..., "__per_executor__": {...}}
- plain per-executor mapping: {"__per_executor__": {...}} (no global
slot, preserving the "untargeted executor gets None" convention)
- plain global mapping: {"__global__": ..., "__per_executor__": {}}
AgentExecutor._resolve_executor_kwargs reads the nested shape and
falls back to the legacy flat shape for checkpoints serialized before
this change; WorkflowExecutor applies the same unwrapping so
subworkflows keep receiving the plain mapping their own resolution
expects.
Fixes microsoft#8310
Yufeng He (he-yufeng)
deployed
to
github-app-auth
September 12, 2026 14:24 — with 
GitHub Actions
Active
GitHub Actions
Active
Yufeng He (he-yufeng)
deployed
to
github-app-auth
September 12, 2026 14:24 — with
GitHub Actions
Active
GitHub Actions
Active
Yufeng He (he-yufeng)
deployed
to
github-app-auth
September 12, 2026 14:24 — with
GitHub Actions
Active
GitHub Actions
Active
Yufeng He (he-yufeng)
deployed
to
github-app-auth
September 12, 2026 14:24 — with
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
__global__ from colliding with the global kwargs slot__global__ from colliding with the global kwargs slot
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Plain __global__ overrides regress, and legacy checkpoints can collide with the new discriminator.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Separates global and executor-specific workflow kwargs to avoid __global__ executor collisions.
Changes:
- Adds the
__per_executor__namespace. - Updates kwargs resolution and subworkflow propagation.
- Adds collision and compatibility tests.
File summaries
| File | Description |
|---|---|
_const.py |
Defines the per-executor namespace key. |
_workflow.py |
Produces the nested kwargs representation. |
_agent_executor.py |
Resolves nested and legacy kwargs. |
_workflow_executor.py |
Propagates kwargs into subworkflows. |
test_workflow_kwargs.py |
Tests workflow-level collision behavior. |
test_agent_executor.py |
Tests nested and legacy resolution. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| # (``function_invocation_kwargs={}``) is preserved and merges to {} | ||
| global_kwargs: Any = resolved.get(GLOBAL_KWARGS_KEY) | ||
| executor_kwargs: Any = resolved.get(self.id) | ||
| if EXECUTOR_KWARGS_KEY in resolved: |
| matched_ids, | ||
| ) | ||
| return dict(kwargs) | ||
| return {EXECUTOR_KWARGS_KEY: dict(kwargs)} |
Comment on lines
+400
to
+404
| if EXECUTOR_KWARGS_KEY in normalized_dict: | ||
| # post-#8310 shape: unwrap the global slot only when no | ||
| # per-executor entries ride alongside it | ||
| if not normalized_dict.get(EXECUTOR_KWARGS_KEY): | ||
| normalized = normalized_dict[GLOBAL_KWARGS_KEY] |
Contributor
There was a problem hiding this comment.
What happens when a legacy checkpoint targets a WorkflowExecutor named __per_executor__ with empty kwargs? Its stored value is {"__per_executor__": {}}, so this branch classifies it as the new envelope and reads the absent __global__ key, raising KeyError before the child workflow runs. Could the legacy and new formats be distinguished unambiguously on this path?
Comment on lines
398
to
406
| if isinstance(normalized, dict): | ||
| normalized_dict = cast(dict[str, Any], normalized) | ||
| if len(normalized_dict) == 1 and GLOBAL_KWARGS_KEY in normalized_dict: | ||
| if EXECUTOR_KWARGS_KEY in normalized_dict: | ||
| # post-#8310 shape: unwrap the global slot only when no | ||
| # per-executor entries ride alongside it | ||
| if not normalized_dict.get(EXECUTOR_KWARGS_KEY): | ||
| normalized = normalized_dict[GLOBAL_KWARGS_KEY] | ||
| elif len(normalized_dict) == 1 and GLOBAL_KWARGS_KEY in normalized_dict: | ||
| normalized = normalized_dict[GLOBAL_KWARGS_KEY] |
Contributor
There was a problem hiding this comment.
Would it make sense to put the normalized-kwargs format behind one internal encoder and decoder? _workflow.py creates the two-slot representation, _agent_executor.py detects it, and this block independently unwraps it for child workflows. Every compatibility change now requires three synchronized edits, and the legal __per_executor__ ID already makes these readers disagree between silent data loss and a crash.
Member
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
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.
Motivation & Context
WorkflowInvocationKwargsexposes global and executor-specific kwargs as separate namespaces, but_resolve_invocation_kwargsnormalizes both public input forms into one flat dict where the global mapping lives under the reserved__global__key alongside bare executor IDs. An executor whose ID is literally__global__then collides with that slot: in the typed form its entry replaces the genuine global mapping, and in the plain-mapping form its entry is later read back as the global mapping and leaks to every untargeted executor in the graph.Description & Review Guide
__per_executor__in_const.py) so the two namespaces cannot overlap. The normalized shapes are: typed wrapper ->{"__global__": ..., "__per_executor__": {...}}; plain per-executor mapping ->{"__per_executor__": {...}}with the global slot absent, preserving the existing convention that an untargeted executor resolves toNone; plain global mapping ->{"__global__": ..., "__per_executor__": {}}.AgentExecutor._resolve_executor_kwargsreads the nested shape and falls back to the legacy flat shape for checkpoints serialized before this change.WorkflowExecutorunwraps the nested shape so a subworkflow keeps receiving the plain mapping its own resolution expects.__global__-named executor now matches the expected tables in the issue: its entry reaches only itself, the genuine global mapping is no longer lost in the typed form, and nothing leaks into unrelated executors. Existing checkpoints with the flat shape keep working through the legacy branch._resolve_executor_kwargs(nested vs legacy) and the empty-global-slot semantics: an explicitly empty global dict still merges to{}(the "clear previous kwargs" path), while a pure per-executor mapping leaves the global slot absent so untargeted executors getNone. Both conventions are pinned by existing tests, which now run against the new shape.Heads-up on ordering with #8314 (also mine, also touching
_agent_executor.py): that one adds a module-levelresolve_executor_run_kwargshelper with the same routing logic. Whichever lands second should rebase and give that helper the same nested-shape branch. The conflict is small and mechanical either way.Related Issue
Fixes #8310
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.