fix: workflow incorrectly marked as completed while nodes are still executing#13
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7172e34ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Recursively propagate skip | ||
| self.propagate_skip_from_edge(edge.id) | ||
| self._state_manager.mark_edge_skipped(edge.id) |
There was a problem hiding this comment.
Calling propagate_skip_from_edge before mark_edge_skipped means the downstream node will still see the current edge as UNKNOWN, so analyze_edge_states returns has_unknown=True and the skip propagation stops. This leaves downstream nodes in UNKNOWN even when all their incoming edges should be skipped, which can stall traversal or prevent completion for graphs where the skipped node is the sole upstream. The previous ordering avoided this by marking the edge skipped before the recursive propagate.
Useful? React with 👍 / 👎.
| # Start execution and enqueue node | ||
| print(f"Starting execution for node: {downstream_node_id}") | ||
| self._state_manager.start_execution(downstream_node_id) |
There was a problem hiding this comment.
This introduces a direct print() in backend code, which bypasses the logging subsystem (no log level, no context, and it can be dropped or spam stdout under workers). Repository rules require module-level logging instead of print, so this should use the existing logger pattern to ensure observability and consistent production behavior.
Useful? React with 👍 / 👎.
1 participant
Benchmark PR from agentic-review-benchmarks#13