Skip to content

[Bug] GraphWorkflow.visualize sanitizes the workflow name in a branch that never runs (#1853 item 5) - #1863

Open
ayaangazali wants to merge 1 commit into
kyegomez:masterfrom
ayaangazali:fix/visualize-name-sanitization
Open

[Bug] GraphWorkflow.visualize sanitizes the workflow name in a branch that never runs (#1853 item 5)#1863
ayaangazali wants to merge 1 commit into
kyegomez:masterfrom
ayaangazali:fix/visualize-name-sanitization

Conversation

@ayaangazali

Copy link
Copy Markdown
Contributor

Addresses item 5 in #1853.

What

GraphWorkflow.visualize sanitized the workflow name in a branch that can never run, and used the raw name in the branch that always does.

output_path is assigned unconditionally at the top of the method:

output_path = f"{self.name}_visualization_{str(uuid.uuid4())}"

so the if output_path is None: guard ~190 lines later — the only place safe_name was ever built — was unreachable. graphviz treats its argument as a filesystem path, so a workflow named team/alpha rendered to team/alpha_visualization_<uuid>, a directory that does not exist, and the call failed. The sanitization written to prevent exactly that was sitting in dead code.

before:  team/alpha_visualization_<uuid>   -> renders into a missing directory
after:   team_alpha_visualization_<uuid>   -> renders

Fix

Moved the sanitization into the live assignment and deleted the unreachable branch (6 lines). Net effect is the behaviour the dead code intended, in the path that runs.

The uuid4() suffix is kept — the dead branch omitted it, so had it ever run it would also have collided between two visualizations of the same workflow.

Test

Appended to tests/structs/test_graph_workflow.py — no new file. It asserts the dead branch is gone, that safe_name is built in the live path, and that a name containing / maps to team_alpha.

master source + this test:   1 failed
this branch:                 1 passed
tests/structs/test_graph_workflow.py:  48 passed, 11 skipped
black --check --line-length 70, ruff:  clean

Not included

The other items in #1853 are deliberately out of scope here, and two of them are already covered:

That leaves item 3 (the bogus await on a sync _handle_run_error) genuinely unclaimed; happy to take it separately.

The red checks are the repo-wide ones (build never installs the package, test-main-features dies on Poetry 2.x --no-dev), both addressed in #1812.

Copilot AI lite review requested due to automatic review settings August 9, 2026 23:37
@ayaangazali
ayaangazali requested a review from kyegomez as a code owner August 9, 2026 23:37

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

Hello there, thank you for opening an PR ! 🙏🏻 The team was notified and they will get back to you asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants