From 320be659c081a61a8e0aba9f4a70086af04e0866 Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 29 Apr 2026 17:52:32 +0000 Subject: [PATCH] huskies: merge 816 --- server/src/http/mcp/story_tools/criteria.rs | 190 +++++++++++++++++++- 1 file changed, 189 insertions(+), 1 deletion(-) diff --git a/server/src/http/mcp/story_tools/criteria.rs b/server/src/http/mcp/story_tools/criteria.rs index f790c224..2a35e70b 100644 --- a/server/src/http/mcp/story_tools/criteria.rs +++ b/server/src/http/mcp/story_tools/criteria.rs @@ -18,10 +18,14 @@ use crate::io::story_metadata::{ use crate::service::story::parse_test_cases; use crate::slog_warn; #[allow(unused_imports)] -use crate::workflow::{TestCaseResult, TestStatus, evaluate_acceptance_with_coverage}; +use crate::workflow::{ + TestCaseResult, TestStatus, WorkflowState, evaluate_acceptance_with_coverage, +}; use serde_json::{Value, json}; use std::collections::HashMap; use std::fs; +use std::path::Path; +use std::process::Command; pub(crate) fn tool_get_story_todos(args: &Value, ctx: &AppContext) -> Result { let story_id = args @@ -134,6 +138,16 @@ pub(crate) fn tool_check_criterion(args: &Value, ctx: &AppContext) -> Result Result Option { + let mut count = 0usize; + for line in contents.lines() { + let trimmed = line.trim(); + if let Some(rest) = trimmed.strip_prefix("- [ ] ") { + if count == criterion_index { + return Some(rest.to_string()); + } + count += 1; + } + } + None +} + +/// Run best-effort validation before marking a criterion as checked. +/// +/// Checks three signals (OR-joined): +/// - A: feature branch has at least one commit since master +/// - B: AC text mentions a file path that the branch's commits touched +/// - C: a recorded test name fuzzy-matches the AC text +/// +/// Logs a WARN if none of the signals are satisfied. Always returns `()` so +/// the caller can proceed regardless (operator override). +fn validate_criterion_check( + project_root: &Path, + story_id: &str, + ac_text: &str, + workflow: &WorkflowState, +) { + let branch = format!("feature/story-{story_id}"); + + // ── A: branch has commits vs master ────────────────────────────────────── + let commits = Command::new("git") + .args(["log", &format!("master..{branch}"), "--oneline"]) + .current_dir(project_root) + .output() + .ok() + .filter(|o| o.status.success()) + .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_string()) + .unwrap_or_default(); + + if !commits.is_empty() { + // A passes — branch has real work. Validation satisfied. + return; + } + + // A fails. Check B and C as fallback evidence. + + // ── B: AC text mentions a file touched by the branch ───────────────────── + let changed_files: Vec = Command::new("git") + .args(["diff", &format!("master...{branch}"), "--name-only"]) + .current_dir(project_root) + .output() + .ok() + .filter(|o| o.status.success()) + .map(|o| { + String::from_utf8_lossy(&o.stdout) + .lines() + .map(|l| l.to_string()) + .collect() + }) + .unwrap_or_default(); + + let ac_lower = ac_text.to_lowercase(); + let file_mentioned = changed_files.iter().any(|f| { + let fname = Path::new(f) + .file_name() + .map(|n| n.to_string_lossy().to_lowercase()) + .unwrap_or_default(); + ac_lower.contains(f) || (!fname.is_empty() && ac_lower.contains(fname.as_str())) + }); + + if file_mentioned { + return; + } + + // ── C: a recorded test name fuzzy-matches the AC text ──────────────────── + let ac_words: Vec<&str> = ac_lower + .split(|c: char| !c.is_alphanumeric() && c != '_') + .filter(|w| w.len() >= 3) + .collect(); + + let test_match = workflow.results.get(story_id).is_some_and(|results| { + let names: Vec = results + .unit + .iter() + .chain(results.integration.iter()) + .map(|t| t.name.to_lowercase()) + .collect(); + ac_words + .iter() + .any(|word| names.iter().any(|n| n.contains(word))) + }); + + if !test_match { + slog_warn!( + "[check_criterion] story '{}': no corroborating evidence for criterion '{}' \ + — branch '{}' has no commits vs master and no matching files or tests", + story_id, + ac_text, + branch + ); + } +} + pub(crate) fn tool_edit_criterion(args: &Value, ctx: &AppContext) -> Result { let story_id = args .get("story_id") @@ -456,6 +576,74 @@ mod tests { assert!(result.unwrap_err().contains("blocked")); } + #[test] + fn tool_check_criterion_empty_branch_logs_warning() { + let tmp = tempfile::tempdir().unwrap(); + + // Init a git repo with an initial commit on master. + std::process::Command::new("git") + .args(["init"]) + .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(); + // Create an empty feature branch (no commits vs master). + std::process::Command::new("git") + .args(["checkout", "-b", "feature/story-9997_empty_branch"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + std::process::Command::new("git") + .args(["checkout", "master"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + crate::db::ensure_content_store(); + crate::db::write_item_with_content( + "9997_empty_branch", + "2_current", + "---\nname: Empty Branch Test\n---\n## AC\n- [ ] Implement the feature\n", + ); + + let ctx = test_ctx(tmp.path()); + let result = tool_check_criterion( + &json!({"story_id": "9997_empty_branch", "criterion_index": 0}), + &ctx, + ); + + // Validation is best-effort — check still proceeds. + assert!( + result.is_ok(), + "Expected ok despite empty branch: {result:?}" + ); + + // A warning should be in the global log buffer. + let warnings = crate::log_buffer::global().get_recent( + 200, + Some("9997_empty_branch"), + Some(&crate::log_buffer::LogLevel::Warn), + ); + assert!( + !warnings.is_empty(), + "Expected a validation warning for empty branch check_criterion" + ); + } + #[test] fn tool_check_criterion_missing_story_id() { let tmp = tempfile::tempdir().unwrap();