diff --git a/server/src/worktree/create.rs b/server/src/worktree/create.rs index 0fcc61c1..521b6dff 100644 --- a/server/src/worktree/create.rs +++ b/server/src/worktree/create.rs @@ -14,8 +14,26 @@ use super::{WorktreeInfo, worktree_path, write_mcp_json}; /// - Creates the worktree at `{project_root}/.huskies/worktrees/{story_id}` /// on branch `feature/story-{story_id}`. /// - Writes `.mcp.json` in the worktree pointing to the MCP server at `port`. -/// - Runs setup commands from the config for each component. +/// - Runs setup commands from the config for each component **only on fresh +/// creation** — see below. /// - If the worktree/branch already exists, reuses rather than errors. +/// +/// **Idempotency on reuse:** when `wt_path` already exists, this function does +/// **not** re-run [`run_setup_commands`]. Setup commands typically include +/// destructive operations like `npm ci` (`rm -rf node_modules` then reinstall) +/// that, if run concurrently with another reuse from a different caller, leave +/// `node_modules` in a half-populated state (broken `.bin/*` symlinks pointing +/// at empty package directories). This used to be rare and tolerable, but +/// after story 1066 added a 30-second periodic reconciler that calls +/// `reconcile_worktree_create` → `create_worktree`, every Coding story got a +/// destructive `npm ci` every 30s — racing the merge-gate's own frontend +/// build and producing the `sh: 1: tsc: not found` failure that bricked +/// story 1086 retries on 2026-05-15. +/// +/// The reuse path now matches the documented contract of +/// `reconcile_worktree_create`: "no-op for stories whose worktree already +/// exists." If a worktree is in a bad state and needs re-setup, the caller +/// must explicitly delete it and call `create_worktree` again. pub async fn create_worktree( project_root: &Path, story_id: &str, @@ -30,14 +48,15 @@ pub async fn create_worktree( .unwrap_or_else(|| detect_base_branch(project_root)); let root = project_root.to_path_buf(); - // Already exists — reuse (ensure sparse checkout is configured) + // Already exists — reuse without re-running destructive setup commands. + // Sparse checkout is reconfigured (cheap, idempotent) and `.mcp.json` is + // rewritten in case the server port changed across restarts. if wt_path.exists() { let wt_clone = wt_path.clone(); tokio::task::spawn_blocking(move || configure_sparse_checkout(&wt_clone)) .await .map_err(|e| format!("spawn_blocking: {e}"))??; write_mcp_json(&wt_path, port)?; - run_setup_commands(&wt_path, config).await; return Ok(WorktreeInfo { path: wt_path, branch, @@ -374,32 +393,80 @@ mod tests { } #[tokio::test] - async fn create_worktree_reuse_succeeds_despite_setup_failure() { + async fn create_worktree_reuse_does_not_rerun_setup_commands() { + // Regression for the 2026-05-15 1086 outage: the reuse path used to + // re-run setup commands (including destructive `npm ci`). Combined + // with story 1066's 30-second periodic reconciler, this fired + // `npm ci` against every Coding story every 30s and caused + // `tsc: not found` gate failures. The reuse path must now be a + // no-op for setup commands. let tmp = TempDir::new().unwrap(); let project_root = tmp.path().join("my-project"); fs::create_dir_all(&project_root).unwrap(); init_git_repo(&project_root); // First creation — no setup commands, should succeed - create_worktree(&project_root, "173_reuse_fail", &empty_config(), 3001) + create_worktree(&project_root, "173_reuse_no_setup", &empty_config(), 3001) .await .unwrap(); - // Second call — worktree exists, setup commands fail, must still succeed + // Second call — worktree exists. Setup commands are configured to + // FAIL (`exit 1`); if the reuse path were still running them, the + // failure log would surface — but more importantly, this test + // documents that the reuse path is expected to NEVER reach + // `run_setup_commands` and therefore can never produce a setup + // failure regardless of how broken the setup config is. let result = create_worktree( &project_root, - "173_reuse_fail", + "173_reuse_no_setup", &failing_setup_config(), 3002, ) .await; assert!( result.is_ok(), - "create_worktree reuse must succeed even if setup commands fail: {:?}", + "reuse must succeed and must not run setup commands: {:?}", result.err() ); } + #[tokio::test] + async fn create_worktree_reuse_does_not_create_setup_marker_file() { + // Stronger version of the above: assert that on reuse, a setup + // command that would have created a marker file does NOT run. + let tmp = TempDir::new().unwrap(); + let project_root = tmp.path().join("my-project"); + fs::create_dir_all(&project_root).unwrap(); + init_git_repo(&project_root); + + // First creation — no setup, so no marker yet. + let info = create_worktree(&project_root, "174_reuse_marker", &empty_config(), 3001) + .await + .unwrap(); + let marker = info.path.join("__setup_ran__"); + assert!(!marker.exists(), "no marker after empty-setup creation"); + + // Second call with a setup command that WOULD create the marker if + // run. The reuse path must not run it. + let cfg = ProjectConfig { + component: vec![ComponentConfig { + name: "marker".to_string(), + path: ".".to_string(), + setup: vec!["touch __setup_ran__".to_string()], + teardown: vec![], + }], + ..empty_config() + }; + create_worktree(&project_root, "174_reuse_marker", &cfg, 3002) + .await + .unwrap(); + + assert!( + !marker.exists(), + "reuse path must not run setup commands; marker file was created" + ); + } + #[test] fn install_pre_commit_hook_creates_executable_hook_and_sets_hookspath() { let tmp = TempDir::new().unwrap();