Skip to content

Make tools cloneable#445

Merged
nforro merged 1 commit intopackit:mainfrom
nforro:cloning
May 7, 2026
Merged

Make tools cloneable#445
nforro merged 1 commit intopackit:mainfrom
nforro:cloning

Conversation

@nforro
Copy link
Copy Markdown
Member

@nforro nforro commented May 4, 2026

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a CloneableTool class in ymir/tools/base.py that extends the base Tool class with an asynchronous clone method. This new base class is then integrated across multiple privileged and unprivileged tool modules by replacing the standard Tool import. Feedback was provided to enhance the clone method's reliability by adding safety checks for None values and preventing shared state issues with the options dictionary through explicit copying.

Comment thread ymir/tools/base.py
@nforro nforro force-pushed the cloning branch 2 times, most recently from 983d6cb to 0600415 Compare May 5, 2026 07:33
@nforro
Copy link
Copy Markdown
Member Author

nforro commented May 5, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a CloneableTool base class in ymir/tools/base.py and updates various privileged and unprivileged tools to use this new class. The primary goal is to provide a mechanism for cloning tool instances. A review comment correctly identifies several critical issues in the clone method implementation, including potential AttributeError exceptions when modifying middlewares and accessing cache, as well as the risk of shared state between clones due to shallow copying of the options attribute.

Comment thread ymir/tools/base.py
@nforro nforro force-pushed the cloning branch 2 times, most recently from 28d83f4 to 093b333 Compare May 5, 2026 08:25
Signed-off-by: Nikola Forró <nforro@redhat.com>
Assisted-by: Claude Opus 4.6 via Claude Code
@opohorel
Copy link
Copy Markdown
Collaborator

opohorel commented May 7, 2026

do I understand correctly that you've fixed this issue in BeeAI upstream, so we don't have to apply this workaround?

@nforro
Copy link
Copy Markdown
Member Author

nforro commented May 7, 2026

do I understand correctly that you've fixed this issue in BeeAI upstream, so we don't have to apply this workaround?

No. This isn't a workaround and there is no upstream issue, not unless upstream decides their implementation is wrong/unfortunate, then there would be no need for this PR.

@opohorel
Copy link
Copy Markdown
Collaborator

opohorel commented May 7, 2026

do I understand correctly that you've fixed this issue in BeeAI upstream, so we don't have to apply this workaround?

No. This isn't a workaround and there is no upstream issue, not unless upstream decides their implementation is wrong/unfortunate, then there would be no need for this PR.

oh, I see. I've got confused with the PR you did to BeeAI

@nforro nforro merged commit a5288ab into packit:main May 7, 2026
9 checks passed
@nforro nforro deleted the cloning branch May 7, 2026 08:54
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.

2 participants