Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 57 additions & 7 deletions src/work_item_pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 <id>` on
Expand Down Expand Up @@ -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)
}
Expand All @@ -422,15 +438,15 @@ 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 {
// Fresh task, nothing dispatched yet.
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)
};
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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: <title>` 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
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand All @@ -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
)
}
Expand Down Expand Up @@ -1338,4 +1386,6 @@ mod sdd_test_support;
#[cfg(test)]
mod task_sourcing_tests;
#[cfg(test)]
mod tdd_mode_tests;
#[cfg(test)]
mod tests;
38 changes: 36 additions & 2 deletions src/work_item_pipeline/prompt_builder_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 4 additions & 3 deletions src/work_item_pipeline/sdd_data_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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": "",
Expand All @@ -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);
}
22 changes: 22 additions & 0 deletions src/work_item_pipeline/tdd_mode_tests.rs
Original file line number Diff line number Diff line change
@@ -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"})));
}
Loading