Build FlowDefinition from Flow DSL metadata#6017
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces runtime/AST-based flow introspection with a serializable FlowDefinition contract (Pydantic), adds router ChangesFlow definition contract refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai/src/crewai/flow/human_feedback.py (1)
633-640:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep router metadata when wrapping an existing router.
This wrapper still drops
__is_router__and__router_paths__. If someone stacks@human_feedbackaround@router(...)withoutemit=..., the resulting method stops being recognized as a router by bothFlowMetaandbuild_flow_definition().Suggested fix
for attr in [ "__is_start_method__", "__trigger_methods__", "__condition_type__", "__trigger_condition__", "__is_flow_method__", + "__is_router__", + "__router_paths__", "__flow_persistence_config__", ]: if hasattr(func, attr): setattr(wrapper, attr, getattr(func, attr))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/flow/human_feedback.py` around lines 633 - 640, The wrapper currently strips router metadata when wrapping an existing router; update the attribute-preservation loop in the human_feedback wrapper to also copy "__is_router__" and "__router_paths__" (in addition to the existing attributes like "__is_flow_method__") from the original function to the wrapped function so that FlowMeta and build_flow_definition() still recognize methods wrapped with `@router`(... ) then `@human_feedback`; ensure the code checks for hasattr(orig, attr) and sets the same attribute on the wrapper for both "__is_router__" and "__router_paths__".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/flow/dsl.py`:
- Around line 298-302: Explicit router paths assigned to
wrapper.__router_paths__ are copied verbatim, causing duplicates; change the
assignment so explicit paths are normalized and deduplicated the same way
inferred paths are (convert each path to str and preserve order while removing
duplicates, e.g., by using an order-preserving unique operation such as
dict.fromkeys or equivalent) so that both the single-string branch and the
iterable branch produce a deduplicated list of string paths.
In `@lib/crewai/src/crewai/flow/flow_definition.py`:
- Around line 147-153: from_dict currently replaces any diagnostics present in
the serialized input with fresh validation results; preserve any serialized
diagnostics instead of overwriting them by reading diagnostics from the input
(e.g., get diagnostics from data before calling model_validate), deserialize or
convert them into the same Diagnostic type used by FlowDefinition, run
validate_contract() to produce new diagnostics, merge the two diagnostic
lists/sets (avoid duplicates) into definition.diagnostics, and then call
definition.log_diagnostics(); update FlowDefinition.from_dict to use
model_validate, preserve/deserialize input diagnostics, merge with
validate_contract() output, and assign the merged collection to
definition.diagnostics.
In `@lib/crewai/src/crewai/flow/persistence/decorators.py`:
- Around line 69-78: The allowlist _PRESERVED_FLOW_ATTRS in decorators.py is
missing the "_hf_llm" attribute so methods wrapped by `@human_feedback` lose the
live LLM handle when also wrapped by `@persist`; update the tuple
_PRESERVED_FLOW_ATTRS to include the string "_hf_llm" so the `@persist` wrapper
preserves the live LLM handle (refer to the _PRESERVED_FLOW_ATTRS constant and
the `@human_feedback/`@persist decorator logic to validate behavior).
In `@lib/crewai/src/crewai/flow/visualization/analysis.py`:
- Around line 82-89: The current router-path matching uses _condition_atoms()
which treats composite listeners (e.g., {"and": [...]}) as matches and thus
misaligns with builder._extract_direct_or_triggers() and build_flow_structure();
replace uses of _condition_atoms() with _direct_or_triggers() when checking if a
path belongs to a child in the loop that iterates _router_paths(method)
(affecting the block that populates parent_children for router_name and the
similar block around lines 220-230), so that only direct-OR triggers are
considered when appending child_name to parent_children[router_name], keeping
levels, ancestors, and outgoing-edge counts consistent with the builder
semantics.
- Around line 162-169: The DFS stops when a node is first visited so ancestors
discovered later through another parent never propagate; update
_build_ancestor_dict/_dfs_ancestors to use an iterative worklist or change
propagation approach: compute initial ancestor sets (each node includes its
direct parents), then repeatedly process nodes (or children) and merge parent
ancestor sets into a child, re-enqueue child when its ancestor set grows until
no changes remain; ensure _dfs_ancestors no longer short-circuits on a simple
visited flag but instead triggers downstream propagation when an ancestor set is
expanded so merged graph shapes like A->B->D->E and A->C->D correctly add C into
ancestors["E"].
---
Outside diff comments:
In `@lib/crewai/src/crewai/flow/human_feedback.py`:
- Around line 633-640: The wrapper currently strips router metadata when
wrapping an existing router; update the attribute-preservation loop in the
human_feedback wrapper to also copy "__is_router__" and "__router_paths__" (in
addition to the existing attributes like "__is_flow_method__") from the original
function to the wrapped function so that FlowMeta and build_flow_definition()
still recognize methods wrapped with `@router`(... ) then `@human_feedback`; ensure
the code checks for hasattr(orig, attr) and sets the same attribute on the
wrapper for both "__is_router__" and "__router_paths__".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab9eca7c-fe53-40f2-b7a6-c4744694598b
📒 Files selected for processing (16)
lib/crewai/src/crewai/flow/__init__.pylib/crewai/src/crewai/flow/dsl.pylib/crewai/src/crewai/flow/flow_definition.pylib/crewai/src/crewai/flow/flow_serializer.pylib/crewai/src/crewai/flow/flow_wrappers.pylib/crewai/src/crewai/flow/human_feedback.pylib/crewai/src/crewai/flow/persistence/decorators.pylib/crewai/src/crewai/flow/runtime.pylib/crewai/src/crewai/flow/utils.pylib/crewai/src/crewai/flow/visualization/analysis.pylib/crewai/src/crewai/flow/visualization/builder.pylib/crewai/src/crewai/flow/visualization/schema.pylib/crewai/src/crewai/flow/visualization/types.pylib/crewai/tests/test_flow_definition_contract.pylib/crewai/tests/test_flow_serializer.pylib/crewai/tests/test_flow_visualization.py
💤 Files with no reviewable changes (4)
- lib/crewai/src/crewai/flow/utils.py
- lib/crewai/src/crewai/flow/flow_serializer.py
- lib/crewai/src/crewai/flow/visualization/schema.py
- lib/crewai/tests/test_flow_serializer.py
e6d877e to
29d7911
Compare
bf6354c to
60d5d48
Compare
d6bf045 to
424806c
Compare
e3fa660 to
7c96d06
Compare
7c96d06 to
0213cb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
lib/crewai/src/crewai/flow/visualization/types.py (1)
22-29: ⚡ Quick winTighten
StructureEdgetyping: makeis_router_eventrequired (router_event stays optional).
edge["is_router_event"]is read unconditionally incalculate_execution_paths()and the interactive renderer, and everyStructureEdge(...)creation inlib/crewai/src/crewai/flow/visualization/builder.pystampsis_router_event(Falsefor non-router edges,Truefor router edges). So there’s no runtimeKeyErrorrisk fromtotal=False.The type contract can be improved for static checking by making
is_router_eventa requiredTypedDictkey (e.g., usingtyping_extensions.Required[bool]) while keepingrouter_eventoptional, since non-router edges don’t set it and the renderer guards with"router_event" in edge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/flow/visualization/types.py` around lines 22 - 29, StructureEdge should require is_router_event while keeping router_event optional: update the TypedDict definition (StructureEdge) to mark is_router_event as a required bool using typing_extensions.Required[bool] and keep router_event as Optional[str] (or absent) so static checkers know is_router_event always exists; add the necessary import from typing_extensions and adjust the type on StructureEdge in lib/crewai/src/crewai/flow/visualization/types.py so callers like calculate_execution_paths and the interactive renderer (and the builder that constructs edges) satisfy the stronger contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/flow/dsl.py`:
- Around line 790-800: The FlowHumanFeedbackDefinition currently embeds
human_feedback.metadata verbatim which can include non-serializable objects;
update the constructor call that builds FlowHumanFeedbackDefinition (in the
function/method around FlowHumanFeedbackDefinition creation) to serialize
metadata using _serialize_static_value just like llm and provider do — i.e.,
replace the direct metadata=getattr(config, "metadata", None) with
metadata=_serialize_static_value(getattr(config, "metadata", None), diagnostics,
f"{path}.metadata") so metadata is normalized for the contract.
- Around line 216-227: The helper _string_values_from_annotation currently
recurses into every generic argument, so annotations like list[Literal["a"]] or
dict[str, Literal["b"]] incorrectly contribute to FlowMethodDefinition.emit;
change _string_values_from_annotation to only recurse when the annotation origin
is a union-like wrapper or Annotated (e.g., origins matching typing.Union / "|"
or where getattr(origin,"__name__","") == "Annotated"), and stop recursion for
other container origins (list, dict, Sequence, Mapping, etc.); update
_get_router_return_events (which calls _string_values_from_annotation) to rely
on the tightened behavior so emit is derived only from
Union/Optional/Annotated-wrapped Literals and not from arbitrary containers.
In `@lib/crewai/src/crewai/flow/flow_definition.py`:
- Around line 167-179: The validation in validate_contract currently treats only
start is None as "no start", so a method with {"router": true, "start": false}
slips through; update validate_contract to use the FlowMethodDefinition.is_start
semantic (or otherwise treat falsy start the same as None) when checking for a
trigger: for each method in self.methods, if method.router is true and not
method.is_start() and method.listen is None then append the same
FlowDefinitionDiagnostic (code "router_without_trigger"); this ensures start:
false is considered "no start" like FlowMethodDefinition.is_start defines.
In `@lib/crewai/src/crewai/flow/visualization/assets/interactive.js`:
- Around line 1663-1672: The router-event chips currently reuse the generic
drawer-code-link click path which calls highlightTriggeredBy(eventName) and
stops after the first matching outgoing dashed edge; change the chip behavior so
each chip triggers an event-aware highlighter instead of highlightTriggeredBy:
create or call a handler like highlightRouterEvent(eventName) (or similar) that
finds and highlights all edges/listeners tied to that router_event (e.g.,
collect all graph edges/nodes whose metadata or data attributes reference the
given router_event and apply the highlight to each), and wire the chip click to
that handler via the .drawer-code-link for uniqueRouterEvents; alternatively, if
you prefer not to implement a new highlighter, make the per-event chips
non-clickable and preserve the existing title-level “highlight all” action.
---
Nitpick comments:
In `@lib/crewai/src/crewai/flow/visualization/types.py`:
- Around line 22-29: StructureEdge should require is_router_event while keeping
router_event optional: update the TypedDict definition (StructureEdge) to mark
is_router_event as a required bool using typing_extensions.Required[bool] and
keep router_event as Optional[str] (or absent) so static checkers know
is_router_event always exists; add the necessary import from typing_extensions
and adjust the type on StructureEdge in
lib/crewai/src/crewai/flow/visualization/types.py so callers like
calculate_execution_paths and the interactive renderer (and the builder that
constructs edges) satisfy the stronger contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1aefe16d-0302-4936-b6d1-160e81f26335
📒 Files selected for processing (24)
lib/crewai/src/crewai/flow/__init__.pylib/crewai/src/crewai/flow/dsl.pylib/crewai/src/crewai/flow/flow.pylib/crewai/src/crewai/flow/flow_definition.pylib/crewai/src/crewai/flow/flow_serializer.pylib/crewai/src/crewai/flow/flow_wrappers.pylib/crewai/src/crewai/flow/human_feedback.pylib/crewai/src/crewai/flow/persistence/__init__.pylib/crewai/src/crewai/flow/persistence/decorators.pylib/crewai/src/crewai/flow/runtime.pylib/crewai/src/crewai/flow/types.pylib/crewai/src/crewai/flow/utils.pylib/crewai/src/crewai/flow/visualization/assets/interactive.jslib/crewai/src/crewai/flow/visualization/builder.pylib/crewai/src/crewai/flow/visualization/renderers/interactive.pylib/crewai/src/crewai/flow/visualization/schema.pylib/crewai/src/crewai/flow/visualization/types.pylib/crewai/tests/test_async_human_feedback.pylib/crewai/tests/test_flow.pylib/crewai/tests/test_flow_definition.pylib/crewai/tests/test_flow_serializer.pylib/crewai/tests/test_flow_visualization.pylib/crewai/tests/test_human_feedback_decorator.pylib/crewai/tests/test_human_feedback_integration.py
💤 Files with no reviewable changes (7)
- lib/crewai/src/crewai/flow/persistence/init.py
- lib/crewai/tests/test_flow_serializer.py
- lib/crewai/src/crewai/flow/visualization/schema.py
- lib/crewai/src/crewai/flow/utils.py
- lib/crewai/src/crewai/flow/init.py
- lib/crewai/src/crewai/flow/types.py
- lib/crewai/src/crewai/flow/flow_serializer.py
✅ Files skipped from review due to trivial changes (1)
- lib/crewai/src/crewai/flow/flow.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/crewai/src/crewai/flow/visualization/builder.py
0213cb6 to
513468d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/tests/test_flow_definition.py`:
- Around line 441-457: Change the brittle caplog assertion to only consider
records from the specific logger under test: filter caplog.records by
record.name (or record.logger.name) == "crewai.flow.flow_definition" and assert
that filtered list is empty (e.g., records = [r for r in caplog.records if
r.name == "crewai.flow.flow_definition"]; assert records == []). Apply the same
change in test_dynamic_router_does_not_log_at_class_definition_time (both
occurrences) so the tests only assert against the intended logger and ignore
unrelated warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f1c6303f-cc67-4791-91e7-ec1e23e87e9d
📒 Files selected for processing (24)
lib/crewai/src/crewai/flow/__init__.pylib/crewai/src/crewai/flow/dsl.pylib/crewai/src/crewai/flow/flow.pylib/crewai/src/crewai/flow/flow_definition.pylib/crewai/src/crewai/flow/flow_serializer.pylib/crewai/src/crewai/flow/flow_wrappers.pylib/crewai/src/crewai/flow/human_feedback.pylib/crewai/src/crewai/flow/persistence/__init__.pylib/crewai/src/crewai/flow/persistence/decorators.pylib/crewai/src/crewai/flow/runtime.pylib/crewai/src/crewai/flow/types.pylib/crewai/src/crewai/flow/utils.pylib/crewai/src/crewai/flow/visualization/assets/interactive.jslib/crewai/src/crewai/flow/visualization/builder.pylib/crewai/src/crewai/flow/visualization/renderers/interactive.pylib/crewai/src/crewai/flow/visualization/schema.pylib/crewai/src/crewai/flow/visualization/types.pylib/crewai/tests/test_async_human_feedback.pylib/crewai/tests/test_flow.pylib/crewai/tests/test_flow_definition.pylib/crewai/tests/test_flow_serializer.pylib/crewai/tests/test_flow_visualization.pylib/crewai/tests/test_human_feedback_decorator.pylib/crewai/tests/test_human_feedback_integration.py
💤 Files with no reviewable changes (7)
- lib/crewai/src/crewai/flow/types.py
- lib/crewai/src/crewai/flow/persistence/init.py
- lib/crewai/src/crewai/flow/init.py
- lib/crewai/tests/test_flow_serializer.py
- lib/crewai/src/crewai/flow/flow_serializer.py
- lib/crewai/src/crewai/flow/visualization/schema.py
- lib/crewai/src/crewai/flow/utils.py
✅ Files skipped from review due to trivial changes (1)
- lib/crewai/src/crewai/flow/flow.py
🚧 Files skipped from review as they are similar to previous changes (15)
- lib/crewai/src/crewai/flow/visualization/types.py
- lib/crewai/tests/test_human_feedback_decorator.py
- lib/crewai/src/crewai/flow/visualization/renderers/interactive.py
- lib/crewai/src/crewai/flow/flow_wrappers.py
- lib/crewai/tests/test_human_feedback_integration.py
- lib/crewai/src/crewai/flow/runtime.py
- lib/crewai/src/crewai/flow/visualization/assets/interactive.js
- lib/crewai/tests/test_flow.py
- lib/crewai/src/crewai/flow/visualization/builder.py
- lib/crewai/src/crewai/flow/human_feedback.py
- lib/crewai/src/crewai/flow/persistence/decorators.py
- lib/crewai/src/crewai/flow/dsl.py
- lib/crewai/tests/test_async_human_feedback.py
- lib/crewai/src/crewai/flow/flow_definition.py
- lib/crewai/tests/test_flow_visualization.py
7f8e756 to
a82fae9
Compare
a82fae9 to
667d26b
Compare
Introduce `FlowDefinition`, a serializable model built from the Flow DSL's runtime metadata. It becomes the structural contract for Flow methods, triggers, routers, state, and configuration. The visualization layer is the first consumer: `flow_structure` and `build_flow_structure` now project from the definition instead of re-introspecting the class. The runner still executes from live registries, but the definition gives future runners a single static contract to read. This replaces AST source parsing for router return values, crew references, and state schema with runtime metadata plus explicit `@router(paths=...)` or `Literal`/`Enum` return hints. AST parsing was fragile and could silently fail for dynamic or non-inspectable methods. The refactor removes obsolete introspection and serializer code: * Delete `flow_serializer.py`, `flow/utils.py`, and `visualization/schema.py` * Move flow structure modeling into `flow_definition.py` * Simplify visualization building around the static definition contract
0b50ea9 to
c9e171d
Compare
Introduce
FlowDefinition, a serializable model built from the Flow DSL's runtime metadata. It becomes the structural contract for Flow methods, triggers, routers, state, and configuration.The visualization layer is the first consumer:
flow_structureandbuild_flow_structurenow project from the definition instead of re-introspecting the class. The runner still executes from live registries, but the definition gives future runners a single static contract to read.This replaces AST source parsing for router return values, crew references, and state schema with runtime metadata plus explicit
@router(paths=...)orLiteral/Enumreturn hints. AST parsing was fragile and could silently fail for dynamic or non-inspectable methods.The refactor removes obsolete introspection and serializer code:
flow_serializer.py,flow/utils.py, andvisualization/schema.pyflow_definition.pyNote
Medium Risk
Large refactor of flow structure, visualization, and router metadata with deleted public shims; runtime behavior should stay registry-based but static graphs may differ when router emits are not declared or inferrable.
Overview
Adds a serializable
FlowDefinitioncontract (methods, triggers, state, config, diagnostics, JSON/YAML export) and moves Python class → definition projection intodslviabuild_flow_definition, with decorators recordingFlowMethodDefinitionfragments at decoration time.Flow.flow_definition()is built lazily on first access; runtime still uses live registries but_router_pathsbecomes_router_emit.@routergains optionalemit=, withLiteral/Enumreturn hints when omitted, replacing AST parsing for static router outputs.Visualization now projects from
FlowDefinitiononly (no method source/signatures/OpenAPI); UI terminology shifts from router paths to router events. Removesflow_serializer,flow/utils,visualization/schema, and the publicflow_structureexport;human_feedback/persistalign metadata (__router_emit__,_human_feedback_llm, persistence stamping) with the new contract.Reviewed by Cursor Bugbot for commit c9e171d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores