diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index de5b4504..1d3e301a 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -177,6 +177,16 @@ pub enum SubAgentType { Plan, /// Code review - read + analysis tools. Review, + /// Implementation — focused on writing / patching code to satisfy + /// a specific change. Distinct from `General` in that the prompt + /// posture pushes hard on landing the change cleanly with the + /// minimum surrounding edit (#404). + Implementer, + /// Verification — focused on running the test suite or other + /// validation gates and reporting pass/fail with evidence. + /// Distinct from `Review` in that Review reads code and grades it; + /// Verifier *runs* tests and reports the outcome (#404). + Verifier, /// Custom tool access defined at spawn time. Custom, } @@ -192,6 +202,8 @@ impl SubAgentType { "explore" | "exploration" | "explorer" => Some(Self::Explore), "plan" | "planning" | "awaiter" => Some(Self::Plan), "review" | "code-review" | "code_review" | "reviewer" => Some(Self::Review), + "implementer" | "implement" | "implementation" | "builder" => Some(Self::Implementer), + "verifier" | "verify" | "verification" | "validator" | "tester" => Some(Self::Verifier), "custom" => Some(Self::Custom), _ => None, } @@ -204,6 +216,8 @@ impl SubAgentType { Self::Explore => "explore", Self::Plan => "plan", Self::Review => "review", + Self::Implementer => "implementer", + Self::Verifier => "verifier", Self::Custom => "custom", } } @@ -216,6 +230,8 @@ impl SubAgentType { Self::Explore => EXPLORE_AGENT_PROMPT.to_string(), Self::Plan => PLAN_AGENT_PROMPT.to_string(), Self::Review => REVIEW_AGENT_PROMPT.to_string(), + Self::Implementer => IMPLEMENTER_AGENT_PROMPT.to_string(), + Self::Verifier => VERIFIER_AGENT_PROMPT.to_string(), Self::Custom => CUSTOM_AGENT_PROMPT.to_string(), } } @@ -289,6 +305,44 @@ impl SubAgentType { "todo_list", ], Self::Review => vec!["list_dir", "read_file", "grep_files", "file_search", "note"], + Self::Implementer => vec![ + "list_dir", + "read_file", + "write_file", + "edit_file", + "apply_patch", + "grep_files", + "file_search", + "exec_shell", + "exec_shell_wait", + "exec_shell_interact", + "exec_wait", + "exec_interact", + "note", + "checklist_write", + "checklist_add", + "checklist_update", + "checklist_list", + "todo_write", + "todo_add", + "todo_update", + "todo_list", + "update_plan", + ], + Self::Verifier => vec![ + "list_dir", + "read_file", + "grep_files", + "file_search", + "exec_shell", + "exec_shell_wait", + "exec_shell_interact", + "exec_wait", + "exec_interact", + "run_tests", + "diagnostics", + "note", + ], Self::Custom => vec![], // Must be provided by caller. } } @@ -3489,6 +3543,61 @@ const CUSTOM_AGENT_PROMPT: &str = concat!( include_str!("../../prompts/subagent_output_format.md"), ); +const IMPLEMENTER_AGENT_PROMPT: &str = concat!( + "You are an implementation sub-agent. Your job is to land the change\n", + "the parent assigned to you — write the code, modify the files, satisfy\n", + "the contract — with the *minimum* surrounding edit. You do not refactor\n", + "adjacent code. You do not rename unused variables. You do not 'tidy up'\n", + "while you're in the file. If you see related work that should happen,\n", + "surface it under RISKS or BLOCKERS rather than starting it.\n", + "\n", + "Method:\n", + "- Read the target file(s) end-to-end before editing. Edits made without\n", + " reading the file produce structurally wrong patches.\n", + "- Prefer `edit_file` (single search/replace) for narrow changes.\n", + " Reach for `apply_patch` only when the change spans multiple hunks\n", + " or is structurally tricky.\n", + "- After every batch of edits, run a quick verification: a relevant\n", + " `cargo check` / `npm run lint` / `pytest -k ` so you don't\n", + " hand the parent a half-baked implementation.\n", + "- If the change requires writing tests, write them first or alongside\n", + " the implementation — never as a follow-up the parent has to ask for.\n", + "\n", + "CHANGES is the load-bearing section for implementers. List every file\n", + "you modified with a one-line summary of what changed and why. The parent\n", + "uses CHANGES to decide what to inspect next.\n", + "\n", + include_str!("../../prompts/subagent_output_format.md"), +); + +const VERIFIER_AGENT_PROMPT: &str = concat!( + "You are a verification sub-agent. Your job is to *run* the project's\n", + "test suite (or other validation gates) and report pass/fail with the\n", + "evidence the parent needs to act. You are read-only by convention —\n", + "do not patch failing tests, do not 'fix' lints, do not modify code.\n", + "If a fix seems obvious, describe it under RISKS so the parent can\n", + "spawn an Implementer.\n", + "\n", + "Method:\n", + "- Run the right gate for the language: `cargo test --workspace`,\n", + " `npm test`, `pytest`, `go test ./...`. Use `run_tests` when it's\n", + " available; fall back to `exec_shell` when the project has a custom\n", + " invocation.\n", + "- Run lints if requested: `cargo clippy -- -D warnings`,\n", + " `npm run lint`, `ruff check .`. Don't run lints the parent didn't\n", + " ask for; lint noise drowns the signal you were spawned to surface.\n", + "- Capture the exact failing assertion plus the stack trace / file:line\n", + " in EVIDENCE. A failure summarised as 'cargo test failed' is useless;\n", + " the parent needs the actual panic.\n", + "\n", + "OUTCOME goes at the top of SUMMARY: PASS / FAIL / FLAKY. If FLAKY,\n", + "say which test and how many runs you tried.\n", + "\n", + "CHANGES will almost always be \"None.\" for a verifier.\n", + "\n", + include_str!("../../prompts/subagent_output_format.md"), +); + // === Tests === #[cfg(test)] diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 4c5b82f6..443f08fa 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -51,6 +51,123 @@ fn test_agent_type_from_str() { assert_eq!(SubAgentType::from_str("invalid"), None); } +#[test] +fn test_agent_type_implementer_aliases() { + // #404 — Implementer accepts the obvious aliases the model is + // likely to reach for when the user says "build this". + for alias in ["implementer", "implement", "implementation", "builder"] { + assert_eq!( + SubAgentType::from_str(alias), + Some(SubAgentType::Implementer), + "alias {alias} should resolve to Implementer" + ); + } + // Case-insensitive. + assert_eq!( + SubAgentType::from_str("IMPLEMENTER"), + Some(SubAgentType::Implementer) + ); +} + +#[test] +fn test_agent_type_verifier_aliases() { + // #404 — Verifier accepts test/validate aliases distinct from + // Reviewer, which is for *grading* code rather than *running* it. + for alias in ["verifier", "verify", "verification", "validator", "tester"] { + assert_eq!( + SubAgentType::from_str(alias), + Some(SubAgentType::Verifier), + "alias {alias} should resolve to Verifier" + ); + } + assert_eq!( + SubAgentType::from_str("VERIFY"), + Some(SubAgentType::Verifier) + ); +} + +#[test] +fn test_agent_type_round_trips_via_as_str() { + // Every type should serialize to a string that round-trips back + // through `from_str`. Catches missed variants when adding a new + // role. + for t in [ + SubAgentType::General, + SubAgentType::Explore, + SubAgentType::Plan, + SubAgentType::Review, + SubAgentType::Implementer, + SubAgentType::Verifier, + SubAgentType::Custom, + ] { + let label = t.as_str(); + let back = SubAgentType::from_str(label) + .unwrap_or_else(|| panic!("as_str label {label:?} doesn't round-trip via from_str")); + assert_eq!(back, t, "round-trip failed for {t:?} via {label:?}"); + } +} + +#[test] +fn test_implementer_and_verifier_have_distinct_prompts() { + // The whole point of adding the types is that they carry distinct + // posture. Defensive guard: catch the easy bug where copy-paste + // leaves two new variants with the same prompt as `General`. + let implementer = SubAgentType::Implementer.system_prompt(); + let verifier = SubAgentType::Verifier.system_prompt(); + let general = SubAgentType::General.system_prompt(); + assert_ne!( + implementer, general, + "Implementer prompt must differ from General" + ); + assert_ne!( + verifier, general, + "Verifier prompt must differ from General" + ); + assert_ne!( + implementer, verifier, + "Implementer and Verifier must differ" + ); + // Sanity: each prompt mentions the role's defining verb so the + // model has clear direction. + assert!( + implementer.to_lowercase().contains("implement") + || implementer.to_lowercase().contains("write the code"), + "Implementer prompt should reference its role: {implementer}" + ); + assert!( + verifier.to_lowercase().contains("verif") + || verifier.to_lowercase().contains("test suite") + || verifier.to_lowercase().contains("validation"), + "Verifier prompt should reference its role: {verifier}" + ); +} + +#[test] +fn test_implementer_allowed_tools_include_writes() { + // Implementer is the write-heavy role; the deprecated + // `allowed_tools()` advisory list should reflect that the role + // can write/edit/patch even if today's runtime grants full + // inheritance. + #[allow(deprecated)] + let tools = SubAgentType::Implementer.allowed_tools(); + assert!(tools.contains(&"write_file")); + assert!(tools.contains(&"edit_file")); + assert!(tools.contains(&"apply_patch")); +} + +#[test] +fn test_verifier_allowed_tools_include_test_runner_but_no_writes() { + // Verifier runs validation; it should not have write tools in + // its advisory list. The runtime will still gate writes through + // approval, but the advisory list signals intent. + #[allow(deprecated)] + let tools = SubAgentType::Verifier.allowed_tools(); + assert!(tools.contains(&"run_tests")); + assert!(tools.contains(&"diagnostics")); + assert!(!tools.contains(&"write_file")); + assert!(!tools.contains(&"apply_patch")); +} + #[test] fn test_parse_spawn_request_accepts_message_and_agent_type_aliases() { let input = json!({ diff --git a/crates/tui/src/tui/views/mod.rs b/crates/tui/src/tui/views/mod.rs index 715a3d62..5169a1df 100644 --- a/crates/tui/src/tui/views/mod.rs +++ b/crates/tui/src/tui/views/mod.rs @@ -1644,19 +1644,17 @@ fn agent_type_order(agent_type: &SubAgentType) -> u8 { SubAgentType::General => 0, SubAgentType::Explore => 1, SubAgentType::Plan => 2, - SubAgentType::Review => 3, - SubAgentType::Custom => 4, + SubAgentType::Implementer => 3, + SubAgentType::Verifier => 4, + SubAgentType::Review => 5, + SubAgentType::Custom => 6, } } fn format_agent_type(agent_type: &SubAgentType) -> &'static str { - match agent_type { - SubAgentType::General => "general", - SubAgentType::Explore => "explore", - SubAgentType::Plan => "plan", - SubAgentType::Review => "review", - SubAgentType::Custom => "custom", - } + // Source of truth lives on the enum so any new role lands in both + // the user-visible label and the sort order via the as_str() helper. + agent_type.as_str() } fn format_agent_status(