Add cycle detection to AI flow config#23589
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
|
✨ Fix all issues with BitsAI or with Cursor
|
AAraKKe
left a comment
There was a problem hiding this comment.
Thanks @luisorofino , added just a couple comments which I think we already discussed. Once done lets merge.
AAraKKe
left a comment
There was a problem hiding this comment.
Hi @luisorofino, this review was generated with Claude Code and has been looked over and approved before posting. I believe the comments below are valid — feel free to discuss any you disagree with.
Thanks for the update! I have been trying to understand the algorithm and going back and forght with Claude and Codex and came up with a couple of comments. See below, once they are addressed I am ok to merge!
Comment legend
praise: no action needed, just celebrate!
note: informational, no action required.
question: seeking clarification or understanding your approach.
nit: minor, non-blocking (style, typo). Feel free to ignore.
suggestion: optional improvement, recommended but not required.
request: a change I believe is necessary before merging.
Validation ReportAll 20 validations passed. Show details
|
What does this PR do?
Adds cycle detection to
FlowConfigvalidation in the AI framework's phase configuration.A new
_detect_cyclefunction runs DFS over the phase dependency graph and returns the first cycle found as an ordered list of phase IDs.FlowConfig.cross_referencescalls it after all other dependency checks pass and raises aValidationErrorwith a readable→-separated cycle path (e.g.p1 → p2 → p3 → p1).New test coverage for this cycle detection has also been added to
tests/ai/phases/test_config.py.Also adds
from __future__ import annotationstoconfig.pyand updates forward-reference string annotations (e.g.-> "TaskConfig") to plain annotations (-> TaskConfig).Motivation
Without cycle detection, a flow config with circular dependencies (e.g.
p1depends onp2which depends onp1) would pass validation silently and cause the orchestrator to deadlock at runtime. Failing fast at load time with a clear error message is strictly better.Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged