diff --git a/server/src/http/mcp/story_tools/criteria.rs b/server/src/http/mcp/story_tools/criteria.rs index 51f1e519..b4199728 100644 --- a/server/src/http/mcp/story_tools/criteria.rs +++ b/server/src/http/mcp/story_tools/criteria.rs @@ -140,13 +140,14 @@ pub(crate) fn tool_check_criterion(args: &Value, ctx: &AppContext) -> Result Opti None } -/// Run best-effort validation before marking a criterion as checked. +/// Validate that there is corroborating evidence before marking a criterion done. /// /// 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). +/// Returns `Ok(())` when at least one signal passes, or `Err(message)` describing +/// what evidence is missing so the agent knows what to do next. fn validate_criterion_check( project_root: &Path, story_id: &str, ac_text: &str, workflow: &WorkflowState, -) { +) -> Result<(), String> { let branch = format!("feature/story-{story_id}"); // ── A: branch has commits vs master ────────────────────────────────────── @@ -199,8 +200,7 @@ fn validate_criterion_check( .unwrap_or_default(); if !commits.is_empty() { - // A passes — branch has real work. Validation satisfied. - return; + return Ok(()); } // A fails. Check B and C as fallback evidence. @@ -230,7 +230,7 @@ fn validate_criterion_check( }); if file_mentioned { - return; + return Ok(()); } // ── C: a recorded test name fuzzy-matches the AC text ──────────────────── @@ -251,15 +251,16 @@ fn validate_criterion_check( .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 - ); + if test_match { + return Ok(()); } + + Err(format!( + "No corroborating evidence for criterion '{ac_text}'. \ + To proceed: commit your work to '{branch}' (currently has no commits vs master), \ + add a passing test whose name matches the criterion, \ + or change a file mentioned in the criterion text." + )) } pub(crate) fn tool_edit_criterion(args: &Value, ctx: &AppContext) -> Result { @@ -587,7 +588,7 @@ mod tests { } #[test] - fn tool_check_criterion_empty_branch_logs_warning() { + fn tool_check_criterion_empty_branch_returns_error() { let tmp = tempfile::tempdir().unwrap(); // Init a git repo with an initial commit on master. @@ -639,21 +640,27 @@ mod tests { &ctx, ); - // Validation is best-effort — check still proceeds. + // No evidence — must error and NOT mark the criterion. assert!( - result.is_ok(), - "Expected ok despite empty branch: {result:?}" + result.is_err(), + "Expected error when branch has no commits: {result:?}" + ); + let err = result.unwrap_err(); + assert!( + err.contains("No corroborating evidence"), + "Error should describe missing evidence, got: {err}" + ); + assert!( + err.contains("feature/story-9997_empty_branch"), + "Error should name the branch, got: {err}" ); - // 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), - ); + // Criterion must still be unchecked in the CRDT. + let contents = crate::db::read_content("9997_empty_branch") + .expect("story content should still be in CRDT"); assert!( - !warnings.is_empty(), - "Expected a validation warning for empty branch check_criterion" + contents.contains("- [ ] Implement the feature"), + "Criterion should remain unchecked after rejected check_criterion" ); } @@ -691,6 +698,18 @@ mod tests { ); let ctx = test_ctx(tmp.path()); + + // Provide signal-C evidence: a test whose name matches "first" from the criterion text. + tool_record_tests( + &json!({ + "story_id": "9904_test", + "unit": [{"name": "first_criterion_check", "status": "pass"}], + "integration": [] + }), + &ctx, + ) + .unwrap(); + let result = tool_check_criterion( &json!({"story_id": "9904_test", "criterion_index": 0}), &ctx,