story-kit: merge 142_bug_quality_gates_run_after_fast_forward_to_master_instead_of_before

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).
+    /// 3. Quality gates run **inside the merge worktree** before master is touched.
-    /// 4. If all gates pass: archive the story and clean up the worktree.
+    /// 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.
+        // Merge + gates both passed — archive the story and clean up agent entries.
-        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.
         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<String>,
     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.
+/// 6. Run quality gates **in the merge worktree** before touching master.
-/// 7. Clean up the temporary worktree and branch.
+/// 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<String> {
 
 /// 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 cargo_toml = project_root.join("Cargo.toml");
-    let clippy = Command::new("cargo")
+    let script_test = project_root.join("script").join("test");
-        .args(["clippy", "--all-targets", "--all-features"])
-        .current_dir(project_root)
-        .output()
-        .map_err(|e| format!("Failed to run cargo clippy: {e}"))?;
 
-    all_output.push_str("=== cargo clippy ===\n");
+    // ── cargo clippy (only when a Rust project is present) ────────
-    let clippy_out = format!(
+    if cargo_toml.exists() {
-        "{}{}",
+        let clippy = Command::new("cargo")
-        String::from_utf8_lossy(&clippy.stdout),
+            .args(["clippy", "--all-targets", "--all-features"])
-        String::from_utf8_lossy(&clippy.stderr)
+            .current_dir(project_root)
-    );
+            .output()
-    all_output.push_str(&clippy_out);
+            .map_err(|e| format!("Failed to run cargo clippy: {e}"))?;
-    all_output.push('\n');
 
-    if !clippy.status.success() {
+        all_output.push_str("=== cargo clippy ===\n");
-        all_passed = false;
+        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)?;
+    // Only run when there is something to run: a script/test entrypoint or a
-    all_output.push_str(&test_out);
+    // Rust project (Cargo.toml).  Skipping avoids spurious failures in
-    if !test_success {
+    // environments that have neither (e.g. test temp-dirs).
-        all_passed = false;
+    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)]