Repository navigation
Python: preserve distinct Mermaid workflow node identities - #9082
Eduard van Valkenburg (eavanvalkenburg) merged 3 commits into
Conversation
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 15dc97a4a10c
Model: gpt-5.6-sol
Overview
The PR preallocates Mermaid IDs across the full workflow and uses scoped identity keys, candidate reservation, and deterministic ordering to preserve distinct nodes without changing ordinary IDs. The added tests cover executor, namespace, subgraph, and fan-in collisions. However, the allocator omits synthetic internal-edge sources, so the existing include_internal_executors=True export path now raises instead of producing Mermaid output.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_workflows/_viz.py
Motivation & Context
WorkflowViz.to_mermaid()can assign one diagram ID to distinct executors:step-aandstep_aboth becomestep_a, turning a real edge into a self-edge. The same issue affects namespaced children and generated fan-in nodes that collide with legal executor IDs.Description & Review Guide
include_internal_executors=True. Synthetic internal sources use a distinct identity kind so legal executors such asinternal:foocannot reuse their aliases.f847d015(Windows/Python 3.12.14): all 8 new cases pass (4 normal controls and 4 previously raisingKeyError); visualization tests pass 34 cases with the existing SVG test skipped. The related workflow suite passes 1334 tests, with 1 skip, 2 expected failures and 10 experimental/deprecation warnings, and 91% workflow coverage. Ruff, source Pyright, all five test type checkers, applicable prek hooks, wheel and sdist builds pass. Independent public-API controls confirm default Mermaid output and DOT output remain unchanged. No online model/service, browser layout or SVG rendering was tested.Official CI on the previous
15dc97a4head reported one Windows/Python 3.14 timeout infoundry_hosting's cancellation test (16745 passed, 37 skipped, 2 expected failures); that failure is separate from the internal-endpoint regression. The unmodified test passed three focused Windows/Python 3.14.7 runs on each immutable main/head snapshot; the official timeout cause remains unconfirmed. The previous merge gate also reported two cancelled auxiliary checks. Results from that head do not certify the follow-up commit.Current head
6aa65a609766211388dfe442c99c1f45eed04ef2keeps real executors and synthetic internal sources distinct throughout alias allocation and edge emission. Regressions includefoo/internal:foocollisions at top level and within nested workflows, withinclude_internal_executorsenabled and disabled.Local Windows/Python 3.12.14 validation used exact current package source/test snapshots with existing dependencies (Core 1.20.0 / pytest 9.1.1): the same 12 focused internal-endpoint controls changed from 2 failures / 10 passes on
f847d015to 12 passes; the complete visualization test file passed 38 tests with the existing SVG case skipped. No full core/workspace suite, online model/service, browser layout, SVG rendering, or latest-main merge tree was tested in this follow-up. The earlier quality/type/build results above belong to the earlier head.Current official CI on this head has completed all six previously approval-gated workflows at attempt 2: 27 successful / 15 condition-skipped / 0 failed actual jobs. Python Tests passed all 11 test matrix jobs (Ubuntu Python 3.10–3.15 and Windows Python 3.10–3.14), with 16,282–18,137 passes, 37–56 skips and 2 expected failures per job. The resolved aggregate command selects
not integration, excludes devui/lab by default and filters packages by supported Python versions. Coverage reports 91.5% overall; the 85% non-exempt Beta-or-higher package gate passed. Python Merge Tests actually passed DuckDB 91, Qdrant 84, MongoDB 5 and PostgreSQL 43 tests; its integration report totals 132 passes from the latter three jobs, with DuckDB reported separately. Nine remaining Unit/provider test jobs, five .NET build/test/coverage/report jobs and Python coverage upload were condition-skipped. Code quality and merge-gate checks passed; API compatibility, labeling and CLA remain separate static/metadata checks. The actual checkout was48a447ecdff089af47c07296407aeb911687bfe4, with parents847665f853a1ef1a6eb2b090e040054b0a7877a1and this head. These results do not certify later main revisions, skipped providers or online models.Related Issue
Fixes #9081
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.