Skip to content

fix: workflow incorrectly marked as completed while nodes are still executing#151

Open
tomerqodo wants to merge 4 commits into
qodo_action_req_1_base_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr13from
qodo_action_req_1_head_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr13
Open

fix: workflow incorrectly marked as completed while nodes are still executing#151
tomerqodo wants to merge 4 commits into
qodo_action_req_1_base_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr13from
qodo_action_req_1_head_fix_workflow_incorrectly_marked_as_completed_while_nodes_are_still_executing_pr13

Conversation

@tomerqodo

Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#13

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. print() in propagate_skip_from_edge 📘 Rule violation ✧ Quality
Description
• The backend code introduces a print() statement to emit runtime execution information, instead
  of using a module-level logger.
• This violates the requirement to use logging.getLogger(__name__) and logger calls for output,
  which is important for consistent observability and production log handling.
Code

api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[R61-63]

+            # Start execution and enqueue node
+            print(f"Starting execution for node: {downstream_node_id}")
+            self._state_manager.start_execution(downstream_node_id)
Evidence
PR Compliance ID 9 requires backend code to use the logging module with a module-level logger
instead of print(). The changed code in skip_propagator.py adds a print() statement during
node execution start.

AGENTS.md
api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[61-64]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Backend code uses `print()` for runtime output, which violates the requirement to use the logging module with a module-level logger.

## Issue Context
`skip_propagator.py` emits a message when starting execution for a node; this should be logged via `logger = logging.getLogger(__name__)` and a logger call rather than `print()`.

## Fix Focus Areas
- api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[61-63]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Skip propagation blocked 🐞 Bug ✓ Correctness
Description
• Outgoing edges are now marked SKIPPED only after calling propagate_skip_from_edge, so the
  downstream node still observes that incoming edge as UNKNOWN.
• propagate_skip_from_edge explicitly stops when any incoming edge is UNKNOWN, so skip state won’t
  propagate past one hop.
• This can leave nodes/edges incorrectly UNKNOWN (instead of SKIPPED) and can prevent other nodes
  from becoming ready because UNKNOWN incoming edges make is_node_ready return False.
Code

api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[R81-86]

        # Mark all outgoing edges as skipped and propagate
        outgoing_edges = self._graph.get_outgoing_edges(node_id)
        for edge in outgoing_edges:
-            self._state_manager.mark_edge_skipped(edge.id)
            # Recursively propagate skip
            self.propagate_skip_from_edge(edge.id)
+            self._state_manager.mark_edge_skipped(edge.id)
Evidence
Skip propagation stops on UNKNOWN incoming edges. With the new call order, the edge being propagated
from is still UNKNOWN when the downstream node’s incoming edges are analyzed (because
Graph.get_incoming_edges returns edges by in_edges mapping, which includes that edge). Therefore
edge_states['has_unknown'] becomes True and propagation returns early, preventing deeper skips.

api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[49-57]
api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[81-86]
api/core/workflow/graph_engine/graph_state_manager.py[139-146]
api/core/workflow/graph/graph.py[364-372]
api/core/workflow/graph_engine/graph_state_manager.py[80-92]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SkipPropagator._propagate_skip_to_node()` currently calls `propagate_skip_from_edge(edge.id)` before marking that `edge` as `SKIPPED`. Because `propagate_skip_from_edge()` stops when any incoming edge is `UNKNOWN`, downstream skip propagation will terminate early.

### Issue Context
Downstream nodes compute incoming edge states via `Graph.get_incoming_edges()` and `GraphStateManager.analyze_edge_states()`. The edge being propagated from must already be `SKIPPED` for the downstream node’s analysis to proceed.

### Fix Focus Areas
- api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[81-87]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



ⓘ The new review experience is currently in Beta. Learn more

Qodo Logo

Comment on lines +61 to +63
# Start execution and enqueue node
print(f"Starting execution for node: {downstream_node_id}")
self._state_manager.start_execution(downstream_node_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. print() in propagate_skip_from_edge 📘 Rule violation ✧ Quality

• The backend code introduces a print() statement to emit runtime execution information, instead
  of using a module-level logger.
• This violates the requirement to use logging.getLogger(__name__) and logger calls for output,
  which is important for consistent observability and production log handling.
Agent prompt
## Issue description
Backend code uses `print()` for runtime output, which violates the requirement to use the logging module with a module-level logger.

## Issue Context
`skip_propagator.py` emits a message when starting execution for a node; this should be logged via `logger = logging.getLogger(__name__)` and a logger call rather than `print()`.

## Fix Focus Areas
- api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[61-63]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines 81 to +86
# Mark all outgoing edges as skipped and propagate
outgoing_edges = self._graph.get_outgoing_edges(node_id)
for edge in outgoing_edges:
self._state_manager.mark_edge_skipped(edge.id)
# Recursively propagate skip
self.propagate_skip_from_edge(edge.id)
self._state_manager.mark_edge_skipped(edge.id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Skip propagation blocked 🐞 Bug ✓ Correctness

• Outgoing edges are now marked SKIPPED only after calling propagate_skip_from_edge, so the
  downstream node still observes that incoming edge as UNKNOWN.
• propagate_skip_from_edge explicitly stops when any incoming edge is UNKNOWN, so skip state won’t
  propagate past one hop.
• This can leave nodes/edges incorrectly UNKNOWN (instead of SKIPPED) and can prevent other nodes
  from becoming ready because UNKNOWN incoming edges make is_node_ready return False.
Agent prompt
### Issue description
`SkipPropagator._propagate_skip_to_node()` currently calls `propagate_skip_from_edge(edge.id)` before marking that `edge` as `SKIPPED`. Because `propagate_skip_from_edge()` stops when any incoming edge is `UNKNOWN`, downstream skip propagation will terminate early.

### Issue Context
Downstream nodes compute incoming edge states via `Graph.get_incoming_edges()` and `GraphStateManager.analyze_edge_states()`. The edge being propagated from must already be `SKIPPED` for the downstream node’s analysis to proceed.

### Fix Focus Areas
- api/core/workflow/graph_engine/graph_traversal/skip_propagator.py[81-87]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant