fix(agents-api, memory-store): fix temporal workflows, queries, and migrations #994
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh Tomer <[email protected]>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14144715 | Triggered | Generic Password | 15f5a89 | memory-store/docker-compose.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
CI Failure Feedback 🧐(Checks updated until commit b5a8313)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![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 b5a8313 in 2 minutes and 28 seconds
More details
- Looked at
846lines of code in20files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/common/protocol/tasks.py:245
- Draft comment:
Theinclude_remoteparameter is marked as deprecated. Consider removing or replacing it to avoid confusion and potential maintenance issues. - Reason this comment was not posted:
Confidence changes required:50%
In theprepare_execution_inputfunction, theinclude_remoteparameter is marked as deprecated with aFIXMEcomment. This indicates that the parameter should be removed or replaced in the future. However, the current implementation still uses it, which might lead to confusion or maintenance issues.
2. agents-api/agents_api/queries/tasks/create_or_update_task.py:94
- Draft comment:
TheON CONFLICTclause for thetaskstable updates theversionby incrementingtasks.version + 1, which seems incorrect. The version should only be incremented when a new version is created, not on conflict. Consider revisiting this logic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The version handling is complex here. The CTE calculates next_version as MAX(version) + 1. When inserting, it uses this next_version. If there's a conflict, it increments the version again. This double increment could be intentional to skip a version number on conflicts, ensuring unique versions even in race conditions. Without deeper knowledge of the system's versioning requirements, I can't be certain this is wrong.
I might be missing important business logic around version numbering. Perhaps there's a requirement that conflict resolution should maintain sequential version numbers without gaps.
While the versioning logic seems unusual, without clear evidence that it's incorrect and understanding of the system's versioning requirements, we shouldn't assume it's a bug.
The comment makes an assumption about versioning requirements without strong evidence. The current implementation could be intentional for handling race conditions.
3. agents-api/agents_api/queries/agents/create_or_update_agent.py:56
- Draft comment:
Updatingcanonical_namein theON CONFLICTclause might lead to inconsistencies, as it is typically a unique identifier that should not change once set. Consider revisiting this logic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code actually handles canonical_name carefully - it first tries to preserve the existing one through the CTE, and only generates a new one if needed. The ON CONFLICT clause isn't blindly overwriting it, it's using the same value that was just computed in the INSERT portion. This makes the comment's concern invalid - there won't be inconsistencies because the canonical_name logic is consistent between INSERT and UPDATE paths.
I could be missing some business logic requirements around canonical_name immutability. There might be other parts of the system that depend on canonical_name never changing.
Even if there were such requirements, the code already preserves existing canonical_names through the CTE, so this implementation is safe. The comment is raising a theoretical concern that the code already handles correctly.
The comment should be deleted because it raises a concern that is already properly handled by the code's careful treatment of canonical_name values.
Workflow ID: wflow_keWXJzUuAqLdRTxB
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.
HamadaSalhab
approved these changes
Dec 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Type
Bug fix, Enhancement
Description
Major fixes and enhancements to temporal workflows and task execution:
Fixed critical bugs in temporal workflow handling:
Enhanced error handling and monitoring:
Improved query performance and data consistency:
Added configuration improvements:
Changes walkthrough 📝
transition_step.py
Simplify transition step creation parametersagents-api/agents_api/activities/task_steps/transition_step.py
function call
get_task.py
Enhance task query with proper aggregationagents-api/agents_api/queries/tasks/get_task.py
create_task_execution.py
Improve task execution error handlingagents-api/agents_api/routers/tasks/create_task_execution.py
temporal.py
Fix temporal workflow arguments handlingagents-api/agents_api/clients/temporal.py
000013_executions_continuous_view.up.sql
Fix executions continuous view aggregationmemory-store/migrations/000013_executions_continuous_view.up.sql