This repository was archived by the owner on Oct 28, 2026. It is now read-only.
Repository navigation
list: scan the template dir we cloned, not the clone root - #1003
Open
rootkiller6788 wants to merge 4 commits into
Open
rootkiller6788 wants to merge 4 commits into
rootkiller6788 wants to merge 4 commits into
Conversation
fetch_remote_template returns (template_dir, temp_dir) but this unpacked it
the other way round, so every --source scan pointed at the temp root of the
clone. For adk-samples there is no python/agents/ at that level, so
agent-starter-pack list -s https://github.com/google/adk-samples
just said "No agents found" while `list --adk` on the same repo listed them
fine. Same for any remote source with a sub-path.
The old scan_from_root flag goes away with it: when the spec has no sub-path
template_dir already *is* the clone root, which is the only thing it was used
for.
create does clean up the temp dir it gets back from fetch_remote_template, list never did, so every `list -s <url>` / `list --adk` left a copy of the repo in /tmp. Drop it once the table is rendered.
Nothing exercised list_remote_agents, which is how the wrong tuple element slipped through. Point fetch_remote_template at a fake clone and check the agents show up and the temp dir is gone afterwards.
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the remote agent listing functionality to ensure that the temporary repository clone is properly cleaned up after use. It updates list_remote_agents to retrieve both the template directory and the temporary clone root from fetch_remote_template, utilizing a finally block with shutil.rmtree to clean up the clone. A new test is also added to verify this behavior. The review feedback suggests keeping the tracking variable as a pathlib.Path object and passing it directly to shutil.rmtree instead of converting it to a string, which simplifies the code and maintains type consistency.
rootkiller6788
marked this pull request as ready for review
October 16, 2026 03:52
fetch_remote_template already returns pathlib.Path for both elements, so there is no need to round-trip the clone root through str. Drop the conversion and type the variable as pathlib.Path | None.
This branch has not been deployed
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
list -s <url>never found any agents.fetch_remote_templatehands back(template_dir, temp_dir), andlist.pyread the tuple the other way round, so the scan got pointed at the temp root of the clone instead of the template dir inside it. For adk-samples that root has nopython/agents/, so this is what you get:while
list --adkon the very same repo lists them fine. Same for any remote source with a sub-path, e.g.-s google/adk-samples/python/agents/data-science.Fix is to use the first element of the tuple. The
scan_from_rootflag goes away with it: when the spec has no sub-pathtemplate_diralready is the clone root, which is all that flag did, and--adkwas its only caller.While in there I noticed
listnever removes the temp clone (createdoes), so everylist -s/list --adkleft a copy of the repo behind in /tmp. It's dropped in afinallynow.Added a test that mocks the fetch against a fake clone and checks the agents show up and the temp dir is gone. It fails on the current code.