Skip to content

Commit 409a675

Browse files
Yogthosyogthos
authored andcommitted
Add regression test coverage for merged PRs #13-#20
Adds targeted regression tests for each bug fix and behavioral guarantee introduced across the eight feature PRs. Total: 205 → 254 tests (+49). - background-tasks: 0 → 11 tests. Cover get()-evicts-on-read, truncation-by-chars on Completed/Failed, running tasks NOT evicted, shared state across clones (Arc<Mutex>), no-op on missing. - apply-patch: 8 → 16 tests. Multi-op stop-on-failure with prior ops staying applied, ambiguous-update rejection, 1MB create cap (incl. off-by-one), nested-dir creation, PatchOp deserialization. - glob-tool: 5 → 11 tests. Real-FS integration via TempTree: walks files, empty result returns empty string (not 'no files matched'), mtime sort with explicit path != CWD, respects .gitignore, regex metachars escaped, * doesn't cross directory boundaries. - plan-tools: 5 → 8 tests. Source-level regression test that plan_exit has no fs::write / PLAN.md side-effects, channel-unavailable + reply-dropped error paths. - web-tools: 7 → 12 / 4 → 7 tests. Wrap-width regression (long paragraph must wrap), input validation (empty/too many URLs), partial-field formatting in search results, 500-char snippet cap. - task-status: 6 → 9 tests. Completed task evicts after one read, wait=true returns on Failed, wait=true on missing errors promptly (bounded timeout guard against infinite-loop regression). - question-tool: 5 → 10 tests. Header → markdown ## heading, channel + reply error paths, default custom=true, multi-select comma-join. - agent-reminders: 0 → 5 tests. Extracted append_mode_reminder() pure fn so the reminder injection is unit-testable. Covers plan/review/ code modes, PLAN.md gate on code mode, unknown prompts pass through unchanged, section-separator prefix. Each regression test has a comment explaining the bug it guards against.
1 parent 14460fb commit 409a675

9 files changed

Lines changed: 1018 additions & 19 deletions

File tree

src/agent/builder.rs

Lines changed: 82 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -109,25 +109,11 @@ pub async fn build_agent_inner<M: CompletionModel + 'static>(
109109

110110
// Inject mode-specific reminders
111111
if let Some(prompt_name) = &context.current_prompt_name {
112-
match prompt_name.as_str() {
113-
"plan" => {
114-
preamble.push_str("\n\n---\n\nYou are now in PLAN mode. Create a detailed implementation plan. Save it to PLAN.md in the current directory. Analyze the task, break it into concrete steps, consider edge cases and trade-offs. Do NOT write any code or run any commands until the user reviews and approves the plan.");
115-
}
116-
"review" | "review-security" => {
117-
preamble.push_str("\n\n---\n\nYou are now in REVIEW mode. Review the code or plan carefully. Identify bugs, security issues, performance problems, and design flaws. Be thorough and specific. Provide actionable feedback.");
118-
}
119-
"code" => {
120-
let plan_path = std::env::current_dir()
121-
.unwrap_or_else(|_| ".".into())
122-
.join("PLAN.md");
123-
if plan_path.exists() {
124-
preamble.push_str(
125-
"\n\n---\n\nA plan file exists at PLAN.md. Execute the plan step by step. Write and test code following the plan. Report progress after each step. The plan is your guide — follow it closely."
126-
);
127-
}
128-
}
129-
_ => {}
130-
}
112+
let plan_exists = std::env::current_dir()
113+
.unwrap_or_else(|_| ".".into())
114+
.join("PLAN.md")
115+
.exists();
116+
append_mode_reminder(&mut preamble, prompt_name, plan_exists);
131117
}
132118

133119
let mut builder = AgentBuilder::new(model).preamble(&preamble);
@@ -314,3 +300,80 @@ pub fn create_client(api_key: Option<&str>) -> anyhow::Result<openrouter::Client
314300
})?;
315301
Ok(openrouter::Client::new(String::from(key))?)
316302
}
303+
304+
/// Append a mode-specific reminder to `preamble` based on the active prompt
305+
/// name. `plan_exists` reports whether `PLAN.md` is present in CWD — only
306+
/// consulted for the `code` mode reminder. Unknown prompt names produce no
307+
/// reminder so custom prompts don't accidentally pick up plan/review semantics.
308+
pub(crate) fn append_mode_reminder(preamble: &mut String, prompt_name: &str, plan_exists: bool) {
309+
match prompt_name {
310+
"plan" => {
311+
preamble.push_str("\n\n---\n\nYou are now in PLAN mode. Create a detailed implementation plan. Save it to PLAN.md in the current directory. Analyze the task, break it into concrete steps, consider edge cases and trade-offs. Do NOT write any code or run any commands until the user reviews and approves the plan.");
312+
}
313+
"review" | "review-security" => {
314+
preamble.push_str("\n\n---\n\nYou are now in REVIEW mode. Review the code or plan carefully. Identify bugs, security issues, performance problems, and design flaws. Be thorough and specific. Provide actionable feedback.");
315+
}
316+
"code" if plan_exists => {
317+
preamble.push_str(
318+
"\n\n---\n\nA plan file exists at PLAN.md. Execute the plan step by step. Write and test code following the plan. Report progress after each step. The plan is your guide — follow it closely.",
319+
);
320+
}
321+
_ => {}
322+
}
323+
}
324+
325+
#[cfg(test)]
326+
mod reminder_tests {
327+
use super::append_mode_reminder;
328+
329+
#[test]
330+
fn plan_mode_injects_plan_reminder() {
331+
let mut p = String::from("base");
332+
append_mode_reminder(&mut p, "plan", false);
333+
assert!(p.contains("PLAN mode"));
334+
assert!(p.contains("PLAN.md"));
335+
assert!(p.contains("Do NOT write any code"));
336+
}
337+
338+
#[test]
339+
fn review_modes_inject_review_reminder() {
340+
for mode in &["review", "review-security"] {
341+
let mut p = String::from("base");
342+
append_mode_reminder(&mut p, mode, false);
343+
assert!(p.contains("REVIEW mode"), "mode={mode}");
344+
assert!(p.contains("Identify bugs"), "mode={mode}");
345+
}
346+
}
347+
348+
// Regression: the `code` reminder must only appear when PLAN.md exists.
349+
// Without that guard every code-mode session would have a stale "execute
350+
// the plan" instruction even with no plan written.
351+
#[test]
352+
fn regression_code_mode_reminder_requires_plan_md() {
353+
let mut p_with = String::from("base");
354+
append_mode_reminder(&mut p_with, "code", true);
355+
assert!(p_with.contains("plan file exists"));
356+
357+
let mut p_without = String::from("base");
358+
append_mode_reminder(&mut p_without, "code", false);
359+
assert_eq!(p_without, "base", "no reminder must be added");
360+
}
361+
362+
// Unknown prompts (custom user prompts) must produce no reminder so the
363+
// plan/review semantics don't bleed into other modes.
364+
#[test]
365+
fn unknown_prompt_name_appends_nothing() {
366+
let mut p = String::from("base");
367+
append_mode_reminder(&mut p, "my-custom-prompt", true);
368+
assert_eq!(p, "base");
369+
}
370+
371+
// Each reminder is prefixed by the section separator so it visually
372+
// detaches from the prior prompt — regression-guards the leading "\n\n---".
373+
#[test]
374+
fn reminders_use_section_separator() {
375+
let mut p = String::new();
376+
append_mode_reminder(&mut p, "plan", false);
377+
assert!(p.starts_with("\n\n---\n\n"), "got: {p:?}");
378+
}
379+
}

src/agent/tools/apply_patch.rs

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,4 +294,221 @@ mod tests {
294294
let def = tool.definition(String::new()).await;
295295
assert_eq!(def.name, "apply_patch");
296296
}
297+
298+
// Regression: update is documented as text-find-and-replace and must reject
299+
// ambiguous matches rather than silently replacing the first one. Without
300+
// this guard the agent could clobber wrong code in a file with repeated
301+
// boilerplate (use statements, similar function bodies, etc.).
302+
#[test]
303+
fn regression_update_rejects_multiple_matches() {
304+
let tf = TestFile::new("update-ambiguous.txt");
305+
std::fs::write(&tf.path, "foo bar foo baz foo").unwrap();
306+
let result = apply_update(&tf.path, "foo", "qux");
307+
assert!(result.is_err());
308+
let msg = result.unwrap_err();
309+
assert!(msg.contains("3 locations"), "got: {msg}");
310+
// File should be untouched.
311+
assert_eq!(
312+
std::fs::read_to_string(&tf.path).unwrap(),
313+
"foo bar foo baz foo"
314+
);
315+
}
316+
317+
// Regression: prior to the fix, multi-op patches were documented as
318+
// "atomic" but in fact left earlier successful ops applied when a later op
319+
// failed. We now stop on first failure AND the prior ops MUST stay applied
320+
// (no rollback). The error report must explicitly call out which op failed
321+
// and ops after the failure must NOT execute.
322+
#[tokio::test]
323+
async fn regression_multi_op_stops_on_failure_prior_ops_remain() {
324+
let a = TestFile::new("multi-op-a.txt");
325+
let b_existing = TestFile::new("multi-op-b.txt");
326+
let c_should_not_exist = TestFile::new("multi-op-c.txt");
327+
328+
// Pre-create B so the second op (create B) fails.
329+
std::fs::write(&b_existing.path, "already here").unwrap();
330+
331+
let tool = ApplyPatchTool::new(None, None);
332+
let result = tool
333+
.call(ApplyPatchArgs {
334+
operations: vec![
335+
PatchOp::Create {
336+
path: a.path.clone(),
337+
content: "A content".into(),
338+
},
339+
PatchOp::Create {
340+
path: b_existing.path.clone(),
341+
content: "B content".into(),
342+
},
343+
PatchOp::Create {
344+
path: c_should_not_exist.path.clone(),
345+
content: "C content".into(),
346+
},
347+
],
348+
})
349+
.await
350+
.unwrap();
351+
352+
// A was created.
353+
assert!(Path::new(&a.path).exists(), "A must remain applied");
354+
assert_eq!(std::fs::read_to_string(&a.path).unwrap(), "A content");
355+
// B was not overwritten.
356+
assert_eq!(
357+
std::fs::read_to_string(&b_existing.path).unwrap(),
358+
"already here"
359+
);
360+
// C was never attempted.
361+
assert!(
362+
!Path::new(&c_should_not_exist.path).exists(),
363+
"C must not run after failure"
364+
);
365+
// Report names both the success and the failure.
366+
assert!(result.contains("created"), "got: {result}");
367+
assert!(result.contains("FAILED"), "got: {result}");
368+
}
369+
370+
// Regression: create previously had no size cap; the agent could write
371+
// multi-GB files by accident. 1MB limit must be enforced before touching
372+
// the filesystem, and the operation must not produce a partial write.
373+
#[tokio::test]
374+
async fn regression_create_rejects_oversized_content() {
375+
let tf = TestFile::new("oversize.txt");
376+
let too_big = "x".repeat(1_048_577); // 1MB + 1 byte
377+
378+
let tool = ApplyPatchTool::new(None, None);
379+
let result = tool
380+
.call(ApplyPatchArgs {
381+
operations: vec![PatchOp::Create {
382+
path: tf.path.clone(),
383+
content: too_big,
384+
}],
385+
})
386+
.await
387+
.unwrap();
388+
389+
assert!(result.contains("FAILED"), "got: {result}");
390+
assert!(result.contains("exceeds"), "got: {result}");
391+
assert!(
392+
!Path::new(&tf.path).exists(),
393+
"no file should exist after size-limit rejection"
394+
);
395+
}
396+
397+
// Right at the limit must succeed; off-by-one boundary check.
398+
#[tokio::test]
399+
async fn create_accepts_content_at_size_limit() {
400+
let tf = TestFile::new("at-limit.txt");
401+
let at_limit = "x".repeat(1_048_576); // exactly 1MB
402+
403+
let tool = ApplyPatchTool::new(None, None);
404+
let result = tool
405+
.call(ApplyPatchArgs {
406+
operations: vec![PatchOp::Create {
407+
path: tf.path.clone(),
408+
content: at_limit,
409+
}],
410+
})
411+
.await
412+
.unwrap();
413+
414+
assert!(!result.contains("FAILED"), "got: {result}");
415+
assert!(Path::new(&tf.path).exists());
416+
assert_eq!(std::fs::metadata(&tf.path).unwrap().len(), 1_048_576);
417+
}
418+
419+
// create_dir_all is called on the parent — confirms nested-path creates work.
420+
#[test]
421+
fn create_creates_parent_dirs() {
422+
let dir = std::env::temp_dir().join(format!("dirge-test-nested-{}", std::process::id()));
423+
let _ = std::fs::remove_dir_all(&dir);
424+
let nested = dir.join("a/b/c/file.txt");
425+
let path_str = nested.to_str().unwrap();
426+
427+
let result = apply_create(path_str, "deep content");
428+
assert!(result.is_ok());
429+
assert_eq!(std::fs::read_to_string(&nested).unwrap(), "deep content");
430+
431+
let _ = std::fs::remove_dir_all(&dir);
432+
}
433+
434+
#[test]
435+
fn delete_missing_file_returns_err() {
436+
let path = format!("/tmp/dirge-test-delete-ghost-{}.txt", std::process::id());
437+
let _ = std::fs::remove_file(&path);
438+
let result = apply_delete(&path);
439+
assert!(result.is_err());
440+
}
441+
442+
// Multi-op happy path: create + update + rename + delete in sequence,
443+
// touching different files. Regression-tests that the loop applies each op
444+
// in declaration order and the report lists each.
445+
#[tokio::test]
446+
async fn multi_op_happy_path_executes_in_order() {
447+
let a = TestFile::new("multi-happy-a.txt");
448+
let b = TestFile::new("multi-happy-b.txt");
449+
let renamed = format!(
450+
"/tmp/dirge-test-multi-happy-renamed-{}.txt",
451+
std::process::id()
452+
);
453+
let _ = std::fs::remove_file(&renamed);
454+
455+
let tool = ApplyPatchTool::new(None, None);
456+
let result = tool
457+
.call(ApplyPatchArgs {
458+
operations: vec![
459+
PatchOp::Create {
460+
path: a.path.clone(),
461+
content: "hello".into(),
462+
},
463+
PatchOp::Update {
464+
path: a.path.clone(),
465+
old_text: "hello".into(),
466+
new_text: "HELLO".into(),
467+
},
468+
PatchOp::Create {
469+
path: b.path.clone(),
470+
content: "scratch".into(),
471+
},
472+
PatchOp::Rename {
473+
path: a.path.clone(),
474+
new_path: renamed.clone(),
475+
},
476+
PatchOp::Delete {
477+
path: b.path.clone(),
478+
},
479+
],
480+
})
481+
.await
482+
.unwrap();
483+
484+
assert!(!result.contains("FAILED"), "got: {result}");
485+
assert!(!Path::new(&a.path).exists()); // renamed away
486+
assert!(!Path::new(&b.path).exists()); // deleted
487+
assert_eq!(std::fs::read_to_string(&renamed).unwrap(), "HELLO");
488+
let _ = std::fs::remove_file(&renamed);
489+
490+
// Each successful op contributes a line to the report.
491+
assert_eq!(
492+
result.lines().filter(|l| !l.is_empty()).count(),
493+
5,
494+
"report: {result}"
495+
);
496+
}
497+
498+
// Regression: PatchOp deserializes via internally-tagged `action` enum.
499+
// Schema mismatch (e.g. missing `content` for create) must fail at deserialize.
500+
#[test]
501+
fn patch_op_deserializes_each_variant() {
502+
let json = serde_json::json!([
503+
{"action": "create", "path": "/tmp/x", "content": "hi"},
504+
{"action": "update", "path": "/tmp/x", "old_text": "a", "new_text": "b"},
505+
{"action": "delete", "path": "/tmp/x"},
506+
{"action": "rename", "path": "/tmp/x", "new_path": "/tmp/y"},
507+
]);
508+
let ops: Vec<PatchOp> = serde_json::from_value(json).unwrap();
509+
assert!(matches!(ops[0], PatchOp::Create { .. }));
510+
assert!(matches!(ops[1], PatchOp::Update { .. }));
511+
assert!(matches!(ops[2], PatchOp::Delete { .. }));
512+
assert!(matches!(ops[3], PatchOp::Rename { .. }));
513+
}
297514
}

0 commit comments

Comments
 (0)