fix: isolate GitHub recipe temp paths#8878
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: bcf209a566
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let local_repo_path = local_repo_parent_path.to_path_buf().join(repo_name); | ||
| let local_repo_path = local_repo_parent_path | ||
| .to_path_buf() | ||
| .join(temp_child_name(&format!("{owner}/{repo_name}"))); |
There was a problem hiding this comment.
get_local_repo_path now computes a sanitized owner+repo directory (for example owner__repo), but ensure_repo_cloned still calls gh repo clone without the optional <directory> argument. The gh repo clone <repository> [<directory>] command defaults to cloning into the repository-name directory (manual example: gh repo clone cli/cli clones into cli), so first-time clones land in <tmp>/<repo> while this function returns <tmp>/<owner__repo>. That mismatch causes the next git fetch origin call to run against a path that does not exist, breaking remote recipe fetches whenever the repo is not already cloned at the new location.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8350ad8: the clone command now passes the computed isolated local repo path as the explicit gh repo clone <repo> <directory> target, so first-time clones land in the same owner-qualified directory returned by get_local_repo_path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8350ad8e6a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let mut child = String::with_capacity(name.len()); | ||
| for ch in name.chars() { | ||
| match ch { | ||
| '/' | '\\' => child.push_str("__"), |
There was a problem hiding this comment.
Replacing / and \\ with the literal string __ is not injective: recipe paths like a/b and a__b now sanitize to the same temp directory name. Because get_folder_from_github uses that sanitized name as the extraction directory and clears it before unpacking, these distinct recipes can overwrite each other when fetched concurrently (or when one call still relies on a previously returned parent_dir), producing nondeterministic recipe contents. Use a reversible encoding (e.g., percent/base64/hash) instead of a plain replacement delimiter.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
GitHub recipes are cached under the system temp directory. The previous clone path used only the repository name, so two configured recipe repos like
owner-one/sharedandowner-two/sharedwould reuse the same temp checkout and could read recipes from the wrong repository. The recipe output directory also joined the raw recipe name into the temp directory, so path separators in recipe names could point outside the intended temp child.Tests
cargo fmt --package goose-cli -- crates/goose-cli/src/recipes/github_recipe.rsgit diff --checkcargo test -p goose-cli github_recipe --no-default-featureswas attempted, but local linking fails before crate tests because CommandLineToolslibxcrun.dylibis x86_64-only while the Rust toolchain is targeting arm64/arm64e on this machine.