Code Review Bench PR #30251 - fix: workflow incorrectly marked as completed while nodes are still executing

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#13

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

[gitar-bot-staging]

🚨 Bug: Edge marked as skipped AFTER propagation breaks recursion

The reordering of mark_edge_skipped() to occur AFTER propagate_skip_from_edge() breaks the recursive skip propagation entirely.

How it fails:

1. _propagate_skip_to_node("A") is called for a skipped node A
2. For each outgoing edge from A, propagate_skip_from_edge(edge.id) is called BEFORE mark_edge_skipped(edge.id)
3. Inside propagate_skip_from_edge, it calls analyze_edge_states(incoming_edges) on the downstream node
4. Since the edge has NOT been marked as skipped yet (still UNKNOWN), analyze_edge_states returns has_unknown=True
5. The method returns immediately without propagating — the skip chain is broken

Evidence from the same file: The skip_branch_paths() method (line 95-97) correctly marks the edge as skipped BEFORE calling propagate_skip_from_edge(). The original order in _propagate_skip_to_node was also correct (mark first, then propagate).

Impact: Skip states will not propagate beyond the first level of outgoing edges. Downstream nodes that should be skipped will remain in UNKNOWN state, potentially causing the workflow to hang waiting for nodes that will never execute.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	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)
	for edge in outgoing_edges:
	self._state_manager.mark_edge_skipped(edge.id)
	# Recursively propagate skip
	self.propagate_skip_from_edge(edge.id)

• Apply suggested fix

[gitar-bot-staging]

⚠️ Quality: Debug print() statement left in production code

A print(f"Starting execution for node: {downstream_node_id}") statement is left in production workflow execution code. This will write to stdout on every workflow node execution where skip propagation triggers the "has_taken" path.

Impact:

• Pollutes application logs/stdout in production
• In containerized/serverless environments, this adds noise to structured logging
• Performance overhead from string formatting on every invocation (minor but unnecessary)

This should either be removed or replaced with proper logging using the logging module at DEBUG level.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	print(f"Starting execution for node: {downstream_node_id}")
	# Start execution and enqueue node
	self._state_manager.start_execution(downstream_node_id)

• Apply suggested fix

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}

[gitar-bot-staging]

Code Review 🚫 Blocked 0 resolved / 3 findings

The start_execution() fix is correct and addresses the premature-completion bug, but the edge marking reorder in _propagate_skip_to_node introduces a critical regression: edges are marked as skipped AFTER recursive propagation, causing analyze_edge_states to see them as UNKNOWN and halt propagation. Also, a debug print() statement was left in production code.

🚨 Bug: Edge marked as skipped AFTER propagation breaks recursion

📄 api/core/workflow/graph_engine/graph_traversal/skip_propagator.py:83-86

The reordering of mark_edge_skipped() to occur AFTER propagate_skip_from_edge() breaks the recursive skip propagation entirely.

How it fails:

1. _propagate_skip_to_node("A") is called for a skipped node A
2. For each outgoing edge from A, propagate_skip_from_edge(edge.id) is called BEFORE mark_edge_skipped(edge.id)
3. Inside propagate_skip_from_edge, it calls analyze_edge_states(incoming_edges) on the downstream node
4. Since the edge has NOT been marked as skipped yet (still UNKNOWN), analyze_edge_states returns has_unknown=True
5. The method returns immediately without propagating — the skip chain is broken

Evidence from the same file: The skip_branch_paths() method (line 95-97) correctly marks the edge as skipped BEFORE calling propagate_skip_from_edge(). The original order in _propagate_skip_to_node was also correct (mark first, then propagate).

Impact: Skip states will not propagate beyond the first level of outgoing edges. Downstream nodes that should be skipped will remain in UNKNOWN state, potentially causing the workflow to hang waiting for nodes that will never execute.

Suggested fix

        for edge in outgoing_edges:
            self._state_manager.mark_edge_skipped(edge.id)
            # Recursively propagate skip
            self.propagate_skip_from_edge(edge.id)

⚠️ Quality: Debug print() statement left in production code

📄 api/core/workflow/graph_engine/graph_traversal/skip_propagator.py:62

A print(f"Starting execution for node: {downstream_node_id}") statement is left in production workflow execution code. This will write to stdout on every workflow node execution where skip propagation triggers the "has_taken" path.

Impact:

• Pollutes application logs/stdout in production
• In containerized/serverless environments, this adds noise to structured logging
• Performance overhead from string formatting on every invocation (minor but unnecessary)

This should either be removed or replaced with proper logging using the logging module at DEBUG level.

Suggested fix

            # Start execution and enqueue node
            self._state_manager.start_execution(downstream_node_id)

💡 Quality: Test doesn't verify start_execution is called before enqueue_node

📄 api/tests/unit_tests/core/workflow/graph_engine/graph_traversal/test_skip_propagator.py:78-80

The test test_propagate_skip_from_edge_with_taken_edge_enqueues_node verifies that both start_execution and enqueue_node are called, but doesn't verify the ordering. Since the whole point of this PR is to ensure start_execution() happens before enqueue_node() (to prevent premature workflow completion), the test should assert the call order.

You can use mock.assert_has_calls([call.start_execution("node_2"), call.enqueue_node("node_2")]) or check mock_state_manager.mock_calls to verify ordering.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[kayagokalp]

Benchmark Evaluation Results

Ground Truth Bugs (Recall: 1/1 = 100.0%)

ID	Severity	Title	Found?
GT-DIFY-13-001	Critical	Edge marked as skipped AFTER propagation instead of before,	Yes

Extra Gitar Findings

File	Line	Valid Bug?	Notes
api/core/workflow/graph_engine/graph_tra	62	Yes	Debug print() statement left in production code
api/tests/unit_tests/core/workflow/graph	78	No	Test doesn't verify start_execution is called befo

Summary

• Recall: 100.0% (1/1)
• Precision: 66.7% (2/3)
• Precision counts ground truth matches + valid extra findings as true positives