fix(agents-api): rafactor get and list executions queries to not use latest_executions #1188
Conversation
…latest_executions
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
WalkthroughThe recent update focuses on improving the execution queries by refining selection criteria and enhancing status determination logic. The Changes
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| executions e | ||
| LEFT JOIN transitions lt ON e.execution_id = lt.execution_id | ||
| WHERE e.execution_id = $1 | ||
| ORDER BY lt.created_at DESC |
There was a problem hiding this comment.
The ORDER BY lt.created_at DESC with LEFT JOIN can return incorrect status when there are no transitions, since NULL values from lt.created_at will be ordered last. Should use COALESCE(lt.created_at, e.created_at) in ORDER BY.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| executions e | |
| LEFT JOIN transitions lt ON e.execution_id = lt.execution_id | |
| WHERE e.execution_id = $1 | |
| ORDER BY lt.created_at DESC | |
| executions e | |
| LEFT JOIN transitions lt ON e.execution_id = lt.execution_id | |
| WHERE e.execution_id = $1 | |
| ORDER BY COALESCE(lt.created_at, e.created_at) DESC |
| [ | ||
| developer_id, | ||
| task_id, | ||
| sort_by, | ||
| # sort_by, | ||
| direction, | ||
| limit, | ||
| offset, |
There was a problem hiding this comment.
Parameter binding mismatch after removing sort_by - $3 now binds to direction but query assumes it's for sort field comparison, leading to incorrect sorting.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| [ | |
| developer_id, | |
| task_id, | |
| sort_by, | |
| # sort_by, | |
| direction, | |
| limit, | |
| offset, | |
| [ | |
| developer_id, | |
| task_id, | |
| direction, | |
| limit, | |
| offset, |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 0678317 in 2 minutes and 29 seconds
More details
- Looked at
132lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. agents-api/agents_api/queries/executions/get_execution.py:12
-
Draft comment:
Consider usingSELECT * FROM latest_executions WHERE execution_id = $1instead of duplicating the status derivation logic. -
view
latest_executions(000013_executions_continuous_view.up.sql) -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The suggestion seems reasonable at first since it would reduce code duplication. However, there are subtle differences between the implementations. The query specifically orders by created_at and takes the latest transition, while the view uses alatest_transitionstable which may have different semantics. Without understanding the full context of howlatest_transitionsworks vs ordering by created_at, we can't be certain these approaches are equivalent.
The implementations may have intentionally different semantics - the direct query with ORDER BY could be more reliable or performant than using the view in this specific case.
While code duplication is generally bad, in this case the slight differences and lack of context about the equivalence of these approaches means we can't be confident the suggestion is correct.
We should not keep this comment since we don't have enough context to be certain that using the view would be equivalent and correct.
2. agents-api/agents_api/queries/executions/get_execution.py:47
- Draft comment:
Consider using a LEFT JOIN LATERAL (as in the list query) to guarantee only the latest transition per execution is selected. Without it, multiple matching transitions may lead to non-deterministic results. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment raises a valid point about query determinism. Both approaches (LEFT JOIN + ORDER BY + LIMIT 1 and LEFT JOIN LATERAL) would work to get the latest transition, but LEFT JOIN LATERAL is more explicit and potentially more performant as it clearly scopes the subquery per execution. However, since we're querying for a single execution_id, the current approach is already deterministic - there's no risk of mixing transitions from different executions.
Since we're querying by a specific execution_id, the current approach is already deterministic. The suggested change might be a premature optimization.
While the current approach works correctly, LEFT JOIN LATERAL would make the intent clearer and could be more maintainable if the query is modified in the future.
The comment suggests a valid alternative approach, but the current implementation is already correct and deterministic for this specific use case.
3. agents-api/agents_api/queries/executions/list_executions.py:129
- Draft comment:
The 'sort_by' parameter is validated and present in the function signature but commented out in the parameters passed to the query. Either remove it or pass it along to ensure consistent behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code shows clear signs of an intentional refactoring in progress. The SQL query has been modified to only support created_at sorting, with updated_at support commented out. The FIXME comment indicates this is a known limitation. The commented out sort_by parameter aligns with this temporary simplification. This appears to be an intentional intermediate state rather than an oversight.
I could be wrong about this being intentional - maybe the developer forgot to update the function signature and validation to match the new query structure. The inconsistency between signature and implementation could cause confusion.
The presence of the FIXME comment and the carefully commented-out sections in both the SQL and parameters strongly suggests this is an intentional temporary state rather than an oversight.
The comment should be deleted as it's highlighting an intentional temporary state that's already acknowledged by the FIXME comment, not a bug that needs fixing.
4. agents-api/agents_api/queries/executions/get_execution.py:47
- Draft comment:
Consider using a LATERAL join (as in list_executions) to consistently select the latest transition, reducing ambiguity when multiple transitions exist. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
Both approaches (LATERAL join vs ORDER BY + LIMIT) can work to get the latest transition. While LATERAL join might be slightly more performant, the current approach is clear and functional. Without seeing performance metrics or understanding the scale of data, it's hard to say if this optimization would have meaningful impact. The comment is somewhat speculative about potential ambiguity issues.
I might be underestimating the performance impact of LATERAL joins. There could be edge cases where the current approach leads to race conditions or inconsistencies.
While those concerns are valid, the current implementation is clear and functional. Without concrete evidence of performance issues or actual ambiguity problems, this feels like premature optimization.
The comment should be removed as it suggests an alternative implementation without clear evidence that it would be better than the current approach.
5. agents-api/agents_api/queries/executions/list_executions.py:129
- Draft comment:
The 'sort_by' parameter is validated but commented out in the parameter list and not used in the ORDER BY clause. This ignores ordering by updated_at. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_eFDXgc75TetqN6yG
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| developer_id = $1 AND | ||
| task_id = $2 | ||
| e.developer_id = $1 AND | ||
| e.task_id = $2 | ||
| ORDER BY |
There was a problem hiding this comment.
The ORDER BY only handles 'created_at' based on direction, ignoring the validated 'sort_by' (which includes 'updated_at'). Update the ordering logic or remove 'sort_by' if not needed.
PR Type
Bug fix, Enhancement
Description
Refactored
get_execution_queryto replacelatest_executionswith a new query structure.Updated
list_executions_queryto remove reliance onlatest_executionsand improve query logic.Introduced detailed status and error handling in both queries.
Enhanced query structure with better joins and ordering mechanisms.
Changes walkthrough 📝
get_execution.py
Refactor `get_execution_query` to remove `latest_executions`agents-api/agents_api/queries/executions/get_execution.py
latest_executionswith a new query structure.transitionstable for enhanced dataretrieval.
created_atandupdated_at.list_executions.py
Refactor `list_executions_query` to remove `latest_executions`agents-api/agents_api/queries/executions/list_executions.py
latest_executionswith a new query structure.EntelligenceAI PR Summary
Purpose:
Changes:
get_executionquery to include additional fields (developer_id,task_id,task_version) and improved status mapping.list_executionsquery to join with the latest transition data, offering a detailed execution state snapshot and adjusted ordering bycreated_at.Impact: