diff --git a/src/work_item_pipeline.rs b/src/work_item_pipeline.rs index e95822fe..61a79c2b 100644 --- a/src/work_item_pipeline.rs +++ b/src/work_item_pipeline.rs @@ -110,6 +110,22 @@ pub(crate) struct WorkItemData { /// silently skips them as unreadable. #[serde(default)] pub review_only: bool, + /// Set from `detect_tdd_mode` at dispatch time (item #179) — an + /// opt-in, item-level flag (no free-text fallback like `review_only` + /// needs, since this has no legacy callers to support) that appends + /// red-green-refactor instructions to the implementer prompt and a + /// test-first-evidence check to the task-reviewer prompt. + /// + /// If `review_only` is also set, `review_only` takes precedence and + /// `tdd` has no effect — every dispatch site branches on `review_only` + /// first, routing to the analysis-only prompts regardless of `tdd`. + /// This is intentional, not a bug: TDD is a discipline for writing + /// code, and a review-only task never writes any. + /// + /// `#[serde(default)]` for the same reason as `review_only`: runs + /// started before this field existed must still deserialize. + #[serde(default)] + pub tdd: bool, /// Provider session id last observed for each agent name dispatched in /// this run (implementer and judge/reviewer are usually different /// agents and get independent entries). Used to pass `--resume ` on @@ -412,7 +428,7 @@ pub(crate) fn build_sdd_loop_step( let prompt = if ctx.data.review_only { build_review_analyst_prompt(&task, fix_context) } else { - build_implementer_prompt(&task, fix_context) + build_implementer_prompt(&task, fix_context, ctx.data.tdd) }; (agent_name.clone(), prompt) } @@ -422,7 +438,7 @@ pub(crate) fn build_sdd_loop_step( let prompt = if ctx.data.review_only { build_review_of_analysis_prompt(&task, &report) } else { - build_task_reviewer_prompt(&task, &report) + build_task_reviewer_prompt(&task, &report, ctx.data.tdd) }; (judge_agent_name.clone(), prompt) } else { @@ -430,7 +446,7 @@ pub(crate) fn build_sdd_loop_step( let prompt = if ctx.data.review_only { build_review_analyst_prompt(&task, None) } else { - build_implementer_prompt(&task, None) + build_implementer_prompt(&task, None, ctx.data.tdd) }; (agent_name.clone(), prompt) }; @@ -987,6 +1003,8 @@ pub(crate) fn run_or_resume_with_sender( // Seeds `WorkItemData::review_only` (item #507) the same way — computed // once here so both `start_workflow` call sites below agree. let review_only = detect_review_only(&item_description, &existing_metadata); + // Seeds `WorkItemData::tdd` (item #179) the same way. + let tdd = detect_tdd_mode(&existing_metadata); let agent_name = implementer_agent.as_str().to_string(); let judge_agent_name = review_agent.as_str().to_string(); @@ -1019,6 +1037,7 @@ pub(crate) fn run_or_resume_with_sender( notify_recipient: notify_recipient.clone(), tasks: tasks.clone(), review_only, + tdd, ..Default::default() }; let run_id = match existing_run_id { @@ -1162,6 +1181,14 @@ pub(crate) fn detect_review_only(item_description: &str, metadata: &serde_json:: }) } +/// Whether TDD mode (item #179) is on for this dispatch: a deliberate, +/// item-level opt-in read straight from metadata — no free-text fallback +/// like `detect_review_only` needs, since there are no legacy callers to +/// support for a brand-new flag. +pub(crate) fn detect_tdd_mode(metadata: &serde_json::Value) -> bool { + metadata["tdd"].as_bool().unwrap_or(false) +} + /// Parses `### Task N: ` headings (the convention this codebase's /// own plans already use — see docs on item #110) into a task list; falls /// back to a single synthesized task from the item's own description when @@ -1220,8 +1247,13 @@ fn parse_task_headings(doc: &str) -> Vec<SddTask> { /// Builds the prompt for the implementer role: given a task, it must implement /// it. If `fix_context` is provided (a prior reviewer's findings), the prompt -/// instructs them to address those issues. -pub(crate) fn build_implementer_prompt(task: &SddTask, fix_context: Option<&str>) -> String { +/// instructs them to address those issues. When `tdd` is set (item #179), +/// appends explicit red-green-refactor instructions. +pub(crate) fn build_implementer_prompt( + task: &SddTask, + fix_context: Option<&str>, + tdd: bool, +) -> String { let mut prompt = format!( "You are implementing one task from a larger plan.\n\nTask: {}\n\n{}\n", task.title, task.body @@ -1231,6 +1263,11 @@ pub(crate) fn build_implementer_prompt(task: &SddTask, fix_context: Option<&str> "\nA reviewer found issues with your prior attempt:\n{ctx}\n\nAddress them, re-run any tests you touched, and reply with your status.\n" )); } + if tdd { + prompt.push_str( + "\nFollow test-driven development for this task: write a failing test first, confirm it fails, then write the minimal code to pass it, then refactor. Do not write implementation code before its test.\n" + ); + } prompt.push_str("\nReply with a short status: what you did, tests run, and any concerns.\n"); prompt } @@ -1256,9 +1293,20 @@ pub(crate) fn build_review_analyst_prompt(task: &SddTask, fix_context: Option<&s /// Builds the prompt for the task reviewer role: given a task and the /// implementer's report, review it for spec compliance and code quality. -pub(crate) fn build_task_reviewer_prompt(task: &SddTask, implementer_report: &str) -> String { +/// When `tdd` is set (item #179), also requires test-first evidence in the +/// implementer's report as a review criterion. +pub(crate) fn build_task_reviewer_prompt( + task: &SddTask, + implementer_report: &str, + tdd: bool, +) -> String { + let tdd_note = if tdd { + " Also check for test-first evidence: the report must show a failing test was written and confirmed before the implementation change, not just tests added at the end. Missing that sequence is a REVIEW_ISSUES finding even if the code otherwise works." + } else { + "" + }; format!( - "Review this task's implementation for spec compliance and code quality.\n\nTask: {}\n{}\n\nImplementer's report:\n{implementer_report}\n\nReply REVIEW_APPROVED if both spec and quality pass, or REVIEW_ISSUES: followed by a bulleted list of findings.\n", + "Review this task's implementation for spec compliance and code quality.{tdd_note}\n\nTask: {}\n{}\n\nImplementer's report:\n{implementer_report}\n\nReply REVIEW_APPROVED if both spec and quality pass, or REVIEW_ISSUES: followed by a bulleted list of findings.\n", task.title, task.body ) } @@ -1338,4 +1386,6 @@ mod sdd_test_support; #[cfg(test)] mod task_sourcing_tests; #[cfg(test)] +mod tdd_mode_tests; +#[cfg(test)] mod tests; diff --git a/src/work_item_pipeline/prompt_builder_tests.rs b/src/work_item_pipeline/prompt_builder_tests.rs index 21009b1b..bc1b0ccc 100644 --- a/src/work_item_pipeline/prompt_builder_tests.rs +++ b/src/work_item_pipeline/prompt_builder_tests.rs @@ -11,16 +11,50 @@ fn sample_task() -> SddTask { #[test] fn implementer_prompt_includes_task_body() { - let prompt = build_implementer_prompt(&sample_task(), None); + let prompt = build_implementer_prompt(&sample_task(), None, false); assert!(prompt.contains("Add --verbose")); } #[test] fn implementer_prompt_includes_fix_context_when_present() { - let prompt = build_implementer_prompt(&sample_task(), Some("Reviewer found: missing test")); + let prompt = + build_implementer_prompt(&sample_task(), Some("Reviewer found: missing test"), false); assert!(prompt.contains("Reviewer found: missing test")); } +#[test] +fn implementer_prompt_includes_tdd_instructions_when_set() { + let prompt = build_implementer_prompt(&sample_task(), None, true); + assert!(prompt.contains("test-driven development")); + assert!(prompt.contains("failing test")); +} + +#[test] +fn implementer_prompt_omits_tdd_instructions_by_default() { + let prompt = build_implementer_prompt(&sample_task(), None, false); + assert!(!prompt.contains("test-driven development")); +} + +#[test] +fn task_reviewer_prompt_includes_task_and_report() { + let prompt = build_task_reviewer_prompt(&sample_task(), "DONE: added the flag", false); + assert!(prompt.contains("Add --verbose")); + assert!(prompt.contains("DONE: added the flag")); + assert!(prompt.contains("REVIEW_APPROVED")); +} + +#[test] +fn task_reviewer_prompt_checks_test_first_evidence_when_tdd_set() { + let prompt = build_task_reviewer_prompt(&sample_task(), "DONE: added the flag", true); + assert!(prompt.contains("test-first evidence")); +} + +#[test] +fn task_reviewer_prompt_omits_test_first_check_by_default() { + let prompt = build_task_reviewer_prompt(&sample_task(), "DONE: added the flag", false); + assert!(!prompt.contains("test-first evidence")); +} + #[test] fn review_analyst_prompt_forbids_writing_code() { let prompt = build_review_analyst_prompt(&sample_task(), None); diff --git a/src/work_item_pipeline/sdd_data_tests.rs b/src/work_item_pipeline/sdd_data_tests.rs index b1539de1..4112ecf7 100644 --- a/src/work_item_pipeline/sdd_data_tests.rs +++ b/src/work_item_pipeline/sdd_data_tests.rs @@ -25,9 +25,9 @@ fn work_item_data_roundtrips_sdd_fields() { #[test] fn deserializes_persisted_state_json_from_before_review_only_existed() { - // Runs started before item #507 persisted state_json with no - // `review_only`/`review_findings` keys at all. Without `#[serde(default)]` - // on those fields, SqliteStore::load fails to deserialize these rows and + // Runs started before item #507 (or #179's `tdd` field) persisted + // state_json with none of these keys at all. Without `#[serde(default)]` + // on them, SqliteStore::load fails to deserialize these rows and // recover() silently skips them as unreadable. let pre_507_json = r#"{ "reply_text": "", @@ -46,4 +46,5 @@ fn deserializes_persisted_state_json_from_before_review_only_existed() { .expect("old state_json without review_only must still deserialize"); assert!(!data.review_only); assert!(data.review_findings.is_empty()); + assert!(!data.tdd); } diff --git a/src/work_item_pipeline/tdd_mode_tests.rs b/src/work_item_pipeline/tdd_mode_tests.rs new file mode 100644 index 00000000..acf61edf --- /dev/null +++ b/src/work_item_pipeline/tdd_mode_tests.rs @@ -0,0 +1,22 @@ +use super::detect_tdd_mode; +use serde_json::json; + +#[test] +fn missing_tdd_field_defaults_to_false() { + assert!(!detect_tdd_mode(&json!({}))); +} + +#[test] +fn tdd_true_is_detected() { + assert!(detect_tdd_mode(&json!({"tdd": true}))); +} + +#[test] +fn tdd_false_is_detected() { + assert!(!detect_tdd_mode(&json!({"tdd": false}))); +} + +#[test] +fn non_bool_tdd_value_does_not_panic_and_defaults_to_false() { + assert!(!detect_tdd_mode(&json!({"tdd": "yes"}))); +}