From 834a0361a19a0a746f47800a7a17690ba9a076d6 Mon Sep 17 00:00:00 2001 From: Dave Date: Tue, 24 Feb 2026 13:56:11 +0000 Subject: [PATCH] story-kit: merge 142_bug_quality_gates_run_after_fast_forward_to_master_instead_of_before --- server/src/agents.rs | 241 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 191 insertions(+), 50 deletions(-) diff --git a/server/src/agents.rs b/server/src/agents.rs index a71023f..b64cbe3 100644 --- a/server/src/agents.rs +++ b/server/src/agents.rs @@ -1008,8 +1008,8 @@ impl AgentPool { /// /// 1. Squash-merge the story's feature branch into the current branch (master). /// 2. If conflicts are found: abort the merge and report them. - /// 3. If the merge succeeds: run quality gates (cargo clippy + tests + pnpm). - /// 4. If all gates pass: archive the story and clean up the worktree. + /// 3. Quality gates run **inside the merge worktree** before master is touched. + /// 4. If gates pass: fast-forward master and archive the story. /// /// Returns a `MergeReport` with full details of what happened. pub async fn merge_agent_work( @@ -1023,7 +1023,8 @@ impl AgentPool { let sid = story_id.to_string(); let br = branch.clone(); - // Run blocking operations (git + cargo) off the async runtime. + // Run blocking operations (git + cargo + quality gates) off the async runtime. + // Quality gates now run inside run_squash_merge before the fast-forward. let merge_result = tokio::task::spawn_blocking(move || run_squash_merge(&root, &br, &sid)) .await @@ -1036,35 +1037,14 @@ impl AgentPool { had_conflicts: merge_result.had_conflicts, conflicts_resolved: merge_result.conflicts_resolved, conflict_details: merge_result.conflict_details, - gates_passed: false, + gates_passed: merge_result.gates_passed, gate_output: merge_result.output, worktree_cleaned_up: false, story_archived: false, }); } - // Merge succeeded — run quality gates in the project root. - let root2 = project_root.to_path_buf(); - let (gates_passed, gate_output) = - tokio::task::spawn_blocking(move || run_merge_quality_gates(&root2)) - .await - .map_err(|e| format!("Gate check task panicked: {e}"))??; - - if !gates_passed { - return Ok(MergeReport { - story_id: story_id.to_string(), - success: true, - had_conflicts: merge_result.had_conflicts, - conflicts_resolved: merge_result.conflicts_resolved, - conflict_details: merge_result.conflict_details.clone(), - gates_passed: false, - gate_output, - worktree_cleaned_up: false, - story_archived: false, - }); - } - - // Gates passed — archive the story and clean up agent entries. + // Merge + gates both passed — archive the story and clean up agent entries. let story_archived = move_story_to_archived(project_root, story_id).is_ok(); if story_archived { self.remove_agents_for_story(story_id); @@ -1088,7 +1068,7 @@ impl AgentPool { conflicts_resolved: merge_result.conflicts_resolved, conflict_details: merge_result.conflict_details, gates_passed: true, - gate_output, + gate_output: merge_result.output, worktree_cleaned_up, story_archived, }) @@ -2352,6 +2332,10 @@ struct SquashMergeResult { conflicts_resolved: bool, conflict_details: Option, output: String, + /// Whether quality gates ran and passed. `false` when `success` is `false` + /// due to a gate failure; callers can use this to distinguish gate failures + /// from merge/commit/FF failures in the `MergeReport`. + gates_passed: bool, } /// Squash-merge a feature branch into the current branch using a temporary @@ -2364,8 +2348,9 @@ struct SquashMergeResult { /// 3. Run `git merge --squash` in the temporary worktree (not the main worktree). /// 4. If conflicts arise, attempt automatic resolution for simple additive cases. /// 5. If clean (or resolved), commit in the temp worktree. -/// 6. Fast-forward master to the merge-queue commit. -/// 7. Clean up the temporary worktree and branch. +/// 6. Run quality gates **in the merge worktree** before touching master. +/// 7. If gates pass: fast-forward master to the merge-queue commit. +/// 8. Clean up the temporary worktree and branch. fn run_squash_merge( project_root: &Path, branch: &str, @@ -2459,6 +2444,7 @@ fn run_squash_merge( conflicts_resolved: false, conflict_details, output: all_output, + gates_passed: false, }); } } @@ -2477,6 +2463,7 @@ fn run_squash_merge( "Merge conflicts in branch '{branch}' (auto-resolution failed: {e}):\n{merge_stdout}{merge_stderr}" )), output: all_output, + gates_passed: false, }); } } @@ -2509,6 +2496,7 @@ fn run_squash_merge( conflicts_resolved, conflict_details, output: all_output, + gates_passed: true, }); } cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); @@ -2518,9 +2506,48 @@ fn run_squash_merge( conflicts_resolved, conflict_details, output: all_output, + gates_passed: false, }); } + // ── Quality gates in merge workspace (BEFORE fast-forward) ──── + // Run gates in the merge worktree so that failures abort before master moves. + all_output.push_str("=== Running quality gates before fast-forward ===\n"); + match run_merge_quality_gates(&merge_wt_path) { + Ok((true, gate_out)) => { + all_output.push_str(&gate_out); + all_output.push('\n'); + all_output.push_str("=== Quality gates passed ===\n"); + } + Ok((false, gate_out)) => { + all_output.push_str(&gate_out); + all_output.push('\n'); + all_output + .push_str("=== Quality gates FAILED — aborting fast-forward, master unchanged ===\n"); + cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); + return Ok(SquashMergeResult { + success: false, + had_conflicts, + conflicts_resolved, + conflict_details, + output: all_output, + gates_passed: false, + }); + } + Err(e) => { + all_output.push_str(&format!("Gate check error: {e}\n")); + cleanup_merge_workspace(project_root, &merge_wt_path, &merge_branch); + return Ok(SquashMergeResult { + success: false, + had_conflicts, + conflicts_resolved, + conflict_details, + output: all_output, + gates_passed: false, + }); + } + } + // ── Fast-forward master to the merge-queue commit ───────────── all_output.push_str(&format!( "=== Fast-forwarding master to {merge_branch} ===\n" @@ -2548,6 +2575,7 @@ fn run_squash_merge( "Fast-forward to merge-queue failed (master diverged?):\n{ff_stderr}" )), output: all_output, + gates_passed: true, }); } @@ -2561,6 +2589,7 @@ fn run_squash_merge( conflicts_resolved, conflict_details, output: all_output, + gates_passed: true, }) } @@ -2738,37 +2767,48 @@ fn resolve_simple_conflicts(content: &str) -> Option { /// Run quality gates in the project root after a successful merge. /// -/// Runs: cargo clippy, cargo nextest run / cargo test, and pnpm gates if frontend/ exists. +/// Runs: cargo clippy and tests if Cargo.toml is present, script/test if present, and pnpm gates +/// if frontend/ exists. Sections are skipped when the corresponding project artefacts are absent. /// Returns `(gates_passed, combined_output)`. fn run_merge_quality_gates(project_root: &Path) -> Result<(bool, String), String> { let mut all_output = String::new(); let mut all_passed = true; - // ── cargo clippy ────────────────────────────────────────────── - let clippy = Command::new("cargo") - .args(["clippy", "--all-targets", "--all-features"]) - .current_dir(project_root) - .output() - .map_err(|e| format!("Failed to run cargo clippy: {e}"))?; + let cargo_toml = project_root.join("Cargo.toml"); + let script_test = project_root.join("script").join("test"); - all_output.push_str("=== cargo clippy ===\n"); - let clippy_out = format!( - "{}{}", - String::from_utf8_lossy(&clippy.stdout), - String::from_utf8_lossy(&clippy.stderr) - ); - all_output.push_str(&clippy_out); - all_output.push('\n'); + // ── cargo clippy (only when a Rust project is present) ──────── + if cargo_toml.exists() { + let clippy = Command::new("cargo") + .args(["clippy", "--all-targets", "--all-features"]) + .current_dir(project_root) + .output() + .map_err(|e| format!("Failed to run cargo clippy: {e}"))?; - if !clippy.status.success() { - all_passed = false; + all_output.push_str("=== cargo clippy ===\n"); + let clippy_out = format!( + "{}{}", + String::from_utf8_lossy(&clippy.stdout), + String::from_utf8_lossy(&clippy.stderr) + ); + all_output.push_str(&clippy_out); + all_output.push('\n'); + + if !clippy.status.success() { + all_passed = false; + } } // ── tests (script/test if available, else cargo nextest/test) ─ - let (test_success, test_out) = run_project_tests(project_root)?; - all_output.push_str(&test_out); - if !test_success { - all_passed = false; + // Only run when there is something to run: a script/test entrypoint or a + // Rust project (Cargo.toml). Skipping avoids spurious failures in + // environments that have neither (e.g. test temp-dirs). + if script_test.exists() || cargo_toml.exists() { + let (test_success, test_out) = run_project_tests(project_root)?; + all_output.push_str(&test_out); + if !test_success { + all_passed = false; + } } // ── pnpm build (if frontend/ directory exists) ──────────────── @@ -3830,6 +3870,107 @@ mod tests { } } + // ── quality gate ordering test ──────────────────────────────── + + /// Regression test for bug 142: quality gates must run BEFORE the fast-forward + /// to master so that broken code never lands on master. + /// + /// Setup: a repo with a failing `script/test`, a feature branch with one commit. + /// When `run_squash_merge` is called, the gates must detect failure and abort the + /// fast-forward, leaving master HEAD unchanged. + #[cfg(unix)] + #[test] + fn quality_gates_run_before_fast_forward_to_master() { + use std::fs; + use std::os::unix::fs::PermissionsExt; + use tempfile::tempdir; + + let tmp = tempdir().unwrap(); + let repo = tmp.path(); + init_git_repo(repo); + + // Add a failing script/test so quality gates will fail. + let script_dir = repo.join("script"); + fs::create_dir_all(&script_dir).unwrap(); + let script_test = script_dir.join("test"); + fs::write(&script_test, "#!/usr/bin/env bash\nexit 1\n").unwrap(); + let mut perms = fs::metadata(&script_test).unwrap().permissions(); + perms.set_mode(0o755); + fs::set_permissions(&script_test, perms).unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add failing script/test"]) + .current_dir(repo) + .output() + .unwrap(); + + // Create a feature branch with a commit. + Command::new("git") + .args(["checkout", "-b", "feature/story-142_test"]) + .current_dir(repo) + .output() + .unwrap(); + fs::write(repo.join("change.txt"), "feature change").unwrap(); + Command::new("git") + .args(["add", "."]) + .current_dir(repo) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "feature work"]) + .current_dir(repo) + .output() + .unwrap(); + + // Switch back to master and record its HEAD. + Command::new("git") + .args(["checkout", "master"]) + .current_dir(repo) + .output() + .unwrap(); + let head_before = String::from_utf8( + Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(repo) + .output() + .unwrap() + .stdout, + ) + .unwrap() + .trim() + .to_string(); + + // Run the squash-merge. The failing script/test makes quality gates + // fail → fast-forward must NOT happen. + let result = run_squash_merge(repo, "feature/story-142_test", "142_test").unwrap(); + + let head_after = String::from_utf8( + Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(repo) + .output() + .unwrap() + .stdout, + ) + .unwrap() + .trim() + .to_string(); + + // Gates must have failed (script/test exits 1) so master should be untouched. + assert!( + !result.success, + "run_squash_merge must report failure when gates fail" + ); + assert_eq!( + head_before, head_after, + "master HEAD must not advance when quality gates fail (bug 142)" + ); + } + // ── run_project_tests tests ─────────────────────────────────── #[cfg(unix)]