From efd0097f767bb3f7f1041ea36e3486b8badb95f1 Mon Sep 17 00:00:00 2001 From: Huskies Agent Date: Thu, 16 Jul 2026 16:17:14 +0000 Subject: [PATCH] huskies: merge 1176 bug base_branch fallback hardcodes master instead of auto-detecting --- server/src/agents/merge/squash/mod.rs | 18 +-- server/src/agents/merge/squash/tests_basic.rs | 73 +++++++++ server/src/agents/pool/start/spawn.rs | 9 +- server/src/chat/commands/diff.rs | 15 +- server/src/config/mod.rs | 9 +- server/src/config/tests.rs | 56 ++++++- server/src/http/mcp/story_tools/criteria.rs | 142 +++++++++++++++++- server/src/worktree/create.rs | 7 +- server/src/worktree/git.rs | 13 ++ server/src/worktree/mod.rs | 2 +- server/src/worktree/remove.rs | 7 +- 11 files changed, 301 insertions(+), 50 deletions(-) diff --git a/server/src/agents/merge/squash/mod.rs b/server/src/agents/merge/squash/mod.rs index bf451200..be70e6ec 100644 --- a/server/src/agents/merge/squash/mod.rs +++ b/server/src/agents/merge/squash/mod.rs @@ -17,6 +17,14 @@ use crate::config::ProjectConfig; /// causing `git cherry-pick merge-queue/…` to fail with "bad revision". static MERGE_LOCK: Mutex<()> = Mutex::new(()); +/// Resolve the base branch for `project_root` from config, or auto-detect it. +fn resolve_base_branch(project_root: &Path) -> String { + let configured = crate::config::ProjectConfig::load(project_root) + .ok() + .and_then(|c| c.base_branch); + crate::worktree::resolve_base_branch(project_root, configured.as_deref()) +} + pub(crate) fn run_squash_merge( project_root: &Path, branch: &str, @@ -31,10 +39,7 @@ pub(crate) fn run_squash_merge( // A zero-commit branch produces an empty squash and a silent "nothing to // commit" failure. Catch it early with a grep-able error before any merge // work starts. - let base_branch = crate::config::ProjectConfig::load(project_root) - .ok() - .and_then(|c| c.base_branch.clone()) - .unwrap_or_else(|| "master".to_string()); + let base_branch = resolve_base_branch(project_root); let ahead_out = Command::new("git") .args(["rev-list", "--count", &format!("{base_branch}..{branch}")]) @@ -316,11 +321,6 @@ pub(crate) fn run_squash_merge( .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) .unwrap_or_default(); - let base_branch = crate::config::ProjectConfig::load(project_root) - .ok() - .and_then(|c| c.base_branch.clone()) - .unwrap_or_else(|| "master".to_string()); - if current_branch != base_branch { all_output.push_str(&format!( "=== VERIFICATION FAILED: expected branch '{base_branch}' but HEAD is on \ diff --git a/server/src/agents/merge/squash/tests_basic.rs b/server/src/agents/merge/squash/tests_basic.rs index c65b811f..5a154d84 100644 --- a/server/src/agents/merge/squash/tests_basic.rs +++ b/server/src/agents/merge/squash/tests_basic.rs @@ -178,6 +178,79 @@ async fn squash_merge_clean_merge_succeeds() { ); } +#[tokio::test] +async fn squash_merge_succeeds_on_main_based_repo_with_base_branch_unset() { + use std::fs; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + + // Repo whose default branch is `main` — no `master` branch exists at all, + // and no `.huskies/project.toml` sets `base_branch`. run_squash_merge must + // auto-detect `main` instead of assuming `master` (bug 1176). + Command::new("git") + .args(["init", "-b", "main"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["config", "user.email", "test@test.com"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["config", "user.name", "Test"]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "--allow-empty", "-m", "init"]) + .current_dir(repo) + .output() + .unwrap(); + + Command::new("git") + .args(["checkout", "-b", "feature/story-main_test"]) + .current_dir(repo) + .output() + .unwrap(); + fs::write(repo.join("new_file.txt"), "new content").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add new file"]) + .current_dir(repo) + .output() + .unwrap(); + + Command::new("git") + .args(["checkout", "main"]) + .current_dir(repo) + .output() + .unwrap(); + + let result = run_squash_merge(repo, "feature/story-main_test", "main_test").unwrap(); + + assert!( + matches!( + result, + super::MergeResult::Success { + conflicts_resolved: false, + .. + } + ), + "clean merge should succeed on a main-based repo; got: {result:?}" + ); + assert!( + repo.join("new_file.txt").exists(), + "merged file should exist on main" + ); +} + #[tokio::test] async fn squash_merge_nonexistent_branch_fails() { use tempfile::tempdir; diff --git a/server/src/agents/pool/start/spawn.rs b/server/src/agents/pool/start/spawn.rs index 3acd008a..83091f64 100644 --- a/server/src/agents/pool/start/spawn.rs +++ b/server/src/agents/pool/start/spawn.rs @@ -189,10 +189,10 @@ pub(super) async fn run_agent_spawn( let wt_info = { let wt_path = crate::worktree::worktree_path(&project_root_clone, &sid); let branch = format!("feature/story-{sid}"); - let base_branch = config_clone - .base_branch - .clone() - .unwrap_or_else(|| crate::worktree::detect_base_branch(&project_root_clone)); + let base_branch = crate::worktree::resolve_base_branch( + &project_root_clone, + config_clone.base_branch.as_deref(), + ); let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(worktree_wait_secs); loop { @@ -260,6 +260,7 @@ pub(super) async fn run_agent_spawn( } let (command, mut args, mut prompt) = match config_clone.render_agent_args( + &project_root_clone, &wt_path_str, &sid, Some(&aname), diff --git a/server/src/chat/commands/diff.rs b/server/src/chat/commands/diff.rs index d828fd34..816bf1ac 100644 --- a/server/src/chat/commands/diff.rs +++ b/server/src/chat/commands/diff.rs @@ -90,19 +90,10 @@ fn find_story_id(num_str: &str) -> Option { /// Return the configured base branch, or auto-detect it from the project root HEAD. fn resolve_base_branch(project_root: &Path) -> String { - crate::config::ProjectConfig::load(project_root) + let configured = crate::config::ProjectConfig::load(project_root) .ok() - .and_then(|c| c.base_branch) - .unwrap_or_else(|| { - Command::new("git") - .args(["rev-parse", "--abbrev-ref", "HEAD"]) - .current_dir(project_root) - .output() - .ok() - .filter(|o| o.status.success()) - .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) - .unwrap_or_else(|| "master".to_string()) - }) + .and_then(|c| c.base_branch); + crate::worktree::resolve_base_branch(project_root, configured.as_deref()) } /// Run a git command in `dir`, returning trimmed stdout (empty string on failure). diff --git a/server/src/config/mod.rs b/server/src/config/mod.rs index 78627ef3..18c53879 100644 --- a/server/src/config/mod.rs +++ b/server/src/config/mod.rs @@ -593,6 +593,7 @@ impl ProjectConfig { /// If `agent_name` is None, uses the first (default) agent. pub fn render_agent_args( &self, + project_root: &Path, worktree_path: &str, story_id: &str, agent_name: Option<&str>, @@ -607,9 +608,11 @@ impl ProjectConfig { .ok_or_else(|| "No agents configured".to_string())?, }; - let bb = base_branch - .or(self.base_branch.as_deref()) - .unwrap_or("master"); + let bb_owned = crate::worktree::resolve_base_branch( + project_root, + base_branch.or(self.base_branch.as_deref()), + ); + let bb = bb_owned.as_str(); let aname = agent.name.as_str(); let render = |s: &str| { s.replace("{{worktree_path}}", worktree_path) diff --git a/server/src/config/tests.rs b/server/src/config/tests.rs index ce1252c0..53d62f5f 100644 --- a/server/src/config/tests.rs +++ b/server/src/config/tests.rs @@ -129,7 +129,13 @@ max_turns = 0 fn render_agent_args_default() { let config = ProjectConfig::default(); let (cmd, args, prompt) = config - .render_agent_args("/tmp/wt", "42_foo", None, None) + .render_agent_args( + std::path::Path::new("/tmp/wt"), + "/tmp/wt", + "42_foo", + None, + None, + ) .unwrap(); assert_eq!(cmd, "claude"); assert!(args.is_empty()); @@ -155,7 +161,13 @@ max_turns = 30 let config = ProjectConfig::parse(toml_str).unwrap(); let (cmd, args, prompt) = config - .render_agent_args("/tmp/wt", "42_foo", Some("supervisor"), Some("master")) + .render_agent_args( + std::path::Path::new("/tmp/wt"), + "/tmp/wt", + "42_foo", + Some("supervisor"), + Some("master"), + ) .unwrap(); assert_eq!(cmd, "claude"); assert!(args.contains(&"--model".to_string())); @@ -173,7 +185,13 @@ max_turns = 30 // Render for coder let (_, coder_args, _) = config - .render_agent_args("/tmp/wt", "42_foo", Some("coder"), Some("master")) + .render_agent_args( + std::path::Path::new("/tmp/wt"), + "/tmp/wt", + "42_foo", + Some("coder"), + Some("master"), + ) .unwrap(); assert!(coder_args.contains(&"sonnet".to_string())); assert!(coder_args.contains(&"30".to_string())); @@ -184,7 +202,13 @@ max_turns = 30 #[test] fn render_agent_args_not_found() { let config = ProjectConfig::default(); - let result = config.render_agent_args("/tmp/wt", "42_foo", Some("nonexistent"), None); + let result = config.render_agent_args( + std::path::Path::new("/tmp/wt"), + "/tmp/wt", + "42_foo", + Some("nonexistent"), + None, + ); assert!(result.is_err()); assert!(result.unwrap_err().contains("No agent named 'nonexistent'")); } @@ -576,7 +600,13 @@ prompt = "git difftool {{base_branch}}...HEAD" "#; let config = ProjectConfig::parse(toml_str).unwrap(); let (_, _, prompt) = config - .render_agent_args("/tmp/wt", "42_foo", None, None) + .render_agent_args( + std::path::Path::new("/tmp/wt"), + "/tmp/wt", + "42_foo", + None, + None, + ) .unwrap(); assert!( prompt.contains("develop"), @@ -595,7 +625,13 @@ prompt = "git difftool {{base_branch}}...HEAD" "#; let config = ProjectConfig::parse(toml_str).unwrap(); let (_, _, prompt) = config - .render_agent_args("/tmp/wt", "42_foo", None, Some("feature-x")) + .render_agent_args( + std::path::Path::new("/tmp/wt"), + "/tmp/wt", + "42_foo", + None, + Some("feature-x"), + ) .unwrap(); assert!( prompt.contains("feature-x"), @@ -674,7 +710,13 @@ disallowed_tools = ["ScheduleWakeup", "SomeTool"] let config = ProjectConfig::parse(toml_str).unwrap(); let (_, args, _) = config - .render_agent_args("/tmp/wt", "42_foo", None, None) + .render_agent_args( + std::path::Path::new("/tmp/wt"), + "/tmp/wt", + "42_foo", + None, + None, + ) .unwrap(); assert!( args.contains(&"--disallowedTools".to_string()), diff --git a/server/src/http/mcp/story_tools/criteria.rs b/server/src/http/mcp/story_tools/criteria.rs index c2e85f49..862fc408 100644 --- a/server/src/http/mcp/story_tools/criteria.rs +++ b/server/src/http/mcp/story_tools/criteria.rs @@ -185,10 +185,16 @@ fn validate_criterion_check( workflow: &WorkflowState, ) -> Result<(), String> { let branch = format!("feature/story-{story_id}"); + let base_branch = { + let configured = crate::config::ProjectConfig::load(project_root) + .ok() + .and_then(|c| c.base_branch); + crate::worktree::resolve_base_branch(project_root, configured.as_deref()) + }; - // ── A: branch has commits vs master ────────────────────────────────────── + // ── A: branch has commits vs base branch ─────────────────────────────── let commits = Command::new("git") - .args(["log", &format!("master..{branch}"), "--oneline"]) + .args(["log", &format!("{base_branch}..{branch}"), "--oneline"]) .current_dir(project_root) .output() .ok() @@ -204,7 +210,7 @@ fn validate_criterion_check( // ── B: AC text mentions a file touched by the branch ───────────────────── let changed_files: Vec = Command::new("git") - .args(["diff", &format!("master...{branch}"), "--name-only"]) + .args(["diff", &format!("{base_branch}...{branch}"), "--name-only"]) .current_dir(project_root) .output() .ok() @@ -254,7 +260,7 @@ fn validate_criterion_check( Err(format!( "No corroborating evidence for criterion '{ac_text}'. \ - To proceed: commit your work to '{branch}' (currently has no commits vs master), \ + To proceed: commit your work to '{branch}' (currently has no commits vs {base_branch}), \ add a passing test whose name matches the criterion, \ or change a file mentioned in the criterion text." )) @@ -652,6 +658,134 @@ mod tests { ); } + #[test] + fn tool_check_criterion_succeeds_on_main_based_repo_with_base_branch_unset() { + let tmp = tempfile::tempdir().unwrap(); + + // Repo whose default branch is `main` — no `master` branch exists at + // all, and no `.huskies/project.toml` sets `base_branch`. The evidence + // gate must diff against the auto-detected `main`, not a hardcoded + // `master` (bug 1176). + std::process::Command::new("git") + .args(["init", "-b", "main"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["config", "user.email", "test@test.com"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["config", "user.name", "Test"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["commit", "--allow-empty", "-m", "init"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["checkout", "-b", "feature/story-9998_main_branch"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["commit", "--allow-empty", "-m", "feature work"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["checkout", "main"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + crate::db::ensure_content_store(); + crate::db::write_item_with_content( + "9998_main_branch", + "2_current", + "---\nname: Main Branch Test\n---\n## AC\n- [ ] Implement the feature\n", + crate::db::ItemMeta::named("Main Branch Test"), + ); + + let ctx = test_ctx(tmp.path()); + let result = tool_check_criterion( + &json!({"story_id": "9998_main_branch", "criterion_index": 0}), + &ctx, + ); + + assert!( + result.is_ok(), + "Expected ok on main-based repo with commits ahead of main: {result:?}" + ); + } + + #[test] + fn tool_check_criterion_succeeds_on_main_based_repo() { + let tmp = tempfile::tempdir().unwrap(); + + // Repo whose default branch is `main` — no `master` branch exists at + // all. The evidence-gate diff must resolve against `main`, not a + // hardcoded `master` (bug 1176), or `git log master..branch` fails as + // an invalid revision range and blocks every check_criterion call. + std::process::Command::new("git") + .args(["init", "-b", "main"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["config", "user.email", "test@test.com"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["config", "user.name", "Test"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["commit", "--allow-empty", "-m", "init"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["checkout", "-b", "feature/story-9996_main_based"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["commit", "--allow-empty", "-m", "feature work"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["checkout", "main"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + crate::db::ensure_content_store(); + crate::db::write_item_with_content( + "9996_main_based", + "2_current", + "---\nname: Main Based\n---\n## AC\n- [ ] Implement the feature\n", + crate::db::ItemMeta::named("Main Based"), + ); + + let ctx = test_ctx(tmp.path()); + let result = tool_check_criterion( + &json!({"story_id": "9996_main_based", "criterion_index": 0}), + &ctx, + ); + + assert!( + result.is_ok(), + "Expected check_criterion to succeed on a main-based repo: {result:?}" + ); + } + #[test] fn tool_check_criterion_missing_story_id() { let tmp = tempfile::tempdir().unwrap(); diff --git a/server/src/worktree/create.rs b/server/src/worktree/create.rs index 6bf269be..ecd3b8f8 100644 --- a/server/src/worktree/create.rs +++ b/server/src/worktree/create.rs @@ -5,7 +5,7 @@ use std::path::Path; use std::process::Command; use super::git::{ - branch_name, configure_sparse_checkout, create_worktree_sync, detect_base_branch, + branch_name, configure_sparse_checkout, create_worktree_sync, resolve_base_branch, }; use super::{WorktreeInfo, worktree_path, write_mcp_json}; @@ -42,10 +42,7 @@ pub async fn create_worktree( ) -> Result { let wt_path = worktree_path(project_root, story_id); let branch = branch_name(story_id); - let base_branch = config - .base_branch - .clone() - .unwrap_or_else(|| detect_base_branch(project_root)); + let base_branch = resolve_base_branch(project_root, config.base_branch.as_deref()); let root = project_root.to_path_buf(); // Already exists — reuse without re-running destructive setup commands. diff --git a/server/src/worktree/git.rs b/server/src/worktree/git.rs index 3844645d..667d54f0 100644 --- a/server/src/worktree/git.rs +++ b/server/src/worktree/git.rs @@ -24,6 +24,19 @@ pub(crate) fn detect_base_branch(project_root: &Path) -> String { .unwrap_or_else(|| "master".to_string()) } +/// Resolve the base branch to use for `project_root`. +/// +/// Returns `configured` (typically `ProjectConfig.base_branch`) when set; +/// otherwise auto-detects the repository's default branch via +/// [`detect_base_branch`]. This is the single shared resolver for base-branch +/// fallback — production call sites must use it rather than hardcoding +/// `"master"` directly. +pub(crate) fn resolve_base_branch(project_root: &Path, configured: Option<&str>) -> String { + configured + .map(str::to_string) + .unwrap_or_else(|| detect_base_branch(project_root)) +} + /// Placeholder for worktree isolation of `.huskies/work/`. /// /// Previous approaches (sparse checkout, skip-worktree) all leaked state diff --git a/server/src/worktree/mod.rs b/server/src/worktree/mod.rs index bf09522c..f2c4753b 100644 --- a/server/src/worktree/mod.rs +++ b/server/src/worktree/mod.rs @@ -10,8 +10,8 @@ mod sweep; pub use cleanup::{format_report, run_cleanup}; pub use create::create_worktree; pub use create::install_pre_commit_hook; -pub(crate) use git::detect_base_branch; pub use git::migrate_slug_paths; +pub(crate) use git::resolve_base_branch; pub use remove::remove_worktree_by_story_id; #[derive(Debug, Clone)] diff --git a/server/src/worktree/remove.rs b/server/src/worktree/remove.rs index 894368b2..26d12b27 100644 --- a/server/src/worktree/remove.rs +++ b/server/src/worktree/remove.rs @@ -3,7 +3,7 @@ use crate::config::ProjectConfig; use std::path::Path; use super::create::run_teardown_commands; -use super::git::{branch_name, detect_base_branch, remove_worktree_sync}; +use super::git::{branch_name, remove_worktree_sync, resolve_base_branch}; use super::{WorktreeInfo, worktree_path}; /// Remove a git worktree and its branch. @@ -34,10 +34,7 @@ pub async fn remove_worktree_by_story_id( return Err(format!("Worktree not found for story: {story_id}")); } let branch = branch_name(story_id); - let base_branch = config - .base_branch - .clone() - .unwrap_or_else(|| detect_base_branch(project_root)); + let base_branch = resolve_base_branch(project_root, config.base_branch.as_deref()); let info = WorktreeInfo { path, branch,