Python: avoid repeated workflow reference traversal - #8510
Conversation
Walk shared workflow mappings and lists once by identity, preserve environment-reference discovery, and reject ancestor cycles with a definition error. Add deterministic regressions and document the discovery contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Walk shared workflow mappings and lists once by identity, preserve environment-reference discovery, and reject ancestor cycles with a definition error. Add deterministic regressions and document the discovery contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
documentation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation correctly distinguishes shared containers from cycles and is comprehensively tested.
Pull request overview
Replaces recursive environment-reference discovery with cycle-safe, identity-aware traversal.
Changes:
- Adds iterative traversal with shared-container deduplication and cycle detection.
- Adds regression tests for aliases, cycles, immutability, and environment precedence.
- Documents discovery behavior and limitations.
File summaries
| File | Description |
|---|---|
_declarative_base.py |
Implements iterative reference discovery. |
test_workflow_factory.py |
Adds regression coverage. |
README.md |
Documents the behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Code Coverage OverviewLanguages: Python Python / code-coverage/pythonThe overall line coverage in commit ffa376c in the Show a line coverage summary of the most covered files.
Updated |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): a7cf9b267c8b
Model: gpt-5.6-sol-fast
Overview
The PR replaces recursive environment-reference discovery with an identity-aware iterative graph walk, preserving expression filtering and environment precedence while rejecting cycles deterministically. Separate active and visited registries, strong-reference retention, and regression coverage for shared aliases, YAML/dictionary cycles, immutability, and configuration fallback provide concrete guardrails. No Critical, High, or Medium defect was established.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
Preserve the original traversal commit while adopting the published branch and its current base. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Declare the heterogeneous workflow fixture as dict[str, Any] so all test type checkers can validate its nested immutability assertions without changing runtime behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
Remove the detailed traversal section from the package README. Keep traversal limitations with the helper and document cycle errors on workflow creation methods. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Active
Motivation & Context
Workflow definitions can reuse mappings and lists through YAML aliases or shared
objects in the dictionary API. Environment-reference discovery should collect the
same names without repeatedly walking those shared containers. Cyclic definitions
should produce a clear definition error rather than fail through Python recursion.
Description & Review Guide
with an iterative graph walk that scans each mapping or list once by identity.
Track active ancestors separately from completed containers so finite shared
aliases remain supported and cycles raise a fixed
DeclarativeWorkflowError.Document this behavior and add ten small deterministic regression cases.
contents and shared identities are preserved. Only string values beginning with
=contribute names; mapping keys and plain text remain excluded. Processenvironment fallback remains opt-in, restricted to discovered names, with
caller configuration taking precedence. This change concerns discovery only:
it introduces no numerical size, depth, parsing-time or execution budget.
completed shared container and encountering an active ancestor, and preservation
of the existing discovery and environment configuration rules. Regression cases
compare shared containers with equivalent ordinary trees using visit counts,
exercise mapping/list cycles through YAML and dictionary entry points, and
check ordinary nested values, input immutability, and environment precedence.
All 446 targeted factory, graph, model, expression-compatibility and state-path
tests pass, including the new cases. Changed files pass source/test Pyright,
Ruff lint/format checks and applicable configured pre-commit hooks. The full
unit suite and package build were not run.
Related Issue
No linked public issue.
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.