Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 51 additions & 3 deletions crates/goose-cli/src/recipes/github_recipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,35 @@ pub fn ensure_gh_authenticated() -> Result<()> {
}
}

fn temp_child_name(name: &str) -> String {
let mut child = String::with_capacity(name.len());
for ch in name.chars() {
match ch {
'/' | '\\' => child.push_str("__"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve unique temp child names when replacing separators

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 👍 / 👎.

ch if ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' || ch == '.' => {
child.push(ch)
}
_ => child.push('_'),
}
}

if child.is_empty() {
"_".to_string()
} else {
child
}
}

fn get_local_repo_path(
local_repo_parent_path: &Path,
recipe_repo_full_name: &str,
) -> Result<PathBuf> {
let (_, repo_name) = recipe_repo_full_name
let (owner, repo_name) = recipe_repo_full_name
.split_once('/')
.ok_or_else(|| anyhow::anyhow!("Invalid repository name format"))?;
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}")));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Clone into computed local_repo_path

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 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Ok(local_repo_path)
}

Expand Down Expand Up @@ -184,7 +205,7 @@ fn fetch_origin(local_repo_path: &Path) -> Result<()> {

fn get_folder_from_github(local_repo_path: &Path, recipe_name: &str) -> Result<PathBuf> {
let ref_and_path = format!("origin/main:{}", recipe_name);
let output_dir = env::temp_dir().join(recipe_name);
let output_dir = env::temp_dir().join(temp_child_name(recipe_name));

if output_dir.exists() {
fs::remove_dir_all(&output_dir)?;
Expand Down Expand Up @@ -353,3 +374,30 @@ fn get_github_recipe_info(repo: &str, dir_name: &str, recipe_filename: &str) ->

Err(anyhow!("Failed to get recipe content from GitHub"))
}

#[cfg(test)]
mod tests {
use super::*;
use std::path::Path;

#[test]
fn local_repo_path_includes_owner_to_avoid_collisions() {
let parent = Path::new("goose-recipes");
let first = get_local_repo_path(parent, "owner-one/shared").unwrap();
let second = get_local_repo_path(parent, "owner-two/shared").unwrap();

assert_ne!(first, second);
assert_eq!(first, parent.join("owner-one__shared"));
assert_eq!(second, parent.join("owner-two__shared"));
}

#[test]
fn temp_child_name_keeps_recipe_downloads_under_temp_dir() {
let parent = Path::new("goose-recipes");
let child = temp_child_name("../outside");
let output_dir = parent.join(&child);

assert_eq!(child, "..__outside");
assert!(output_dir.starts_with(parent));
}
}