feat(plan): support multiple negative and boundary checks
Change Plan Mode to allow multiple negative and boundary checks per item, while keeping happy path as a single check. Schema change: - checks.negative: Check -> Vec<Check> (>=1 required) - checks.boundary: Check -> Vec<Check> (>=1 required) - checks.happy: Check (unchanged, single) This better reflects real-world tasks where there are often multiple error conditions and edge cases worth tracking. Changes: - Update Checks struct to use Vec<Check> for negative/boundary - Update validation to require at least 1 of each - Update prompts and tool definitions with new array syntax - Add 4 new tests for multi-check scenarios
This commit is contained in:
@@ -49,10 +49,10 @@ Each plan item MUST have:
|
|||||||
- `description`: What will be done
|
- `description`: What will be done
|
||||||
- `state`: todo | doing | done | blocked
|
- `state`: todo | doing | done | blocked
|
||||||
- `touches`: Paths/modules this affects (forces \"where does this live?\")
|
- `touches`: Paths/modules this affects (forces \"where does this live?\")
|
||||||
- `checks`: Three required perspectives:
|
- `checks`: Required test perspectives:
|
||||||
- `happy`: {desc, target} - Normal successful operation
|
- `happy`: {desc, target} - Normal successful operation
|
||||||
- `negative`: {desc, target} - Error handling, invalid input
|
- `negative`: [{desc, target}, ...] - Error handling, invalid input (>=1 required)
|
||||||
- `boundary`: {desc, target} - Edge cases, limits
|
- `boundary`: [{desc, target}, ...] - Edge cases, limits (>=1 required)
|
||||||
- `evidence`: (required when done) File:line refs, test names
|
- `evidence`: (required when done) File:line refs, test names
|
||||||
- `notes`: (required when done) Short implementation explanation
|
- `notes`: (required when done) Short implementation explanation
|
||||||
|
|
||||||
@@ -79,10 +79,14 @@ When updating a plan:
|
|||||||
desc: \"Valid CSV imports 3 comics\"
|
desc: \"Valid CSV imports 3 comics\"
|
||||||
target: \"import::csv\"
|
target: \"import::csv\"
|
||||||
negative:
|
negative:
|
||||||
desc: \"Missing column errors with MissingColumn\"
|
- desc: \"Missing column errors with MissingColumn\"
|
||||||
|
target: \"import::csv\"
|
||||||
|
- desc: \"Malformed row errors with ParseError\"
|
||||||
target: \"import::csv\"
|
target: \"import::csv\"
|
||||||
boundary:
|
boundary:
|
||||||
desc: \"Empty file yields empty import without error\"
|
- desc: \"Empty file yields empty import without error\"
|
||||||
|
target: \"import::csv\"
|
||||||
|
- desc: \"File with only headers yields empty import\"
|
||||||
target: \"import::csv\"
|
target: \"import::csv\"
|
||||||
```
|
```
|
||||||
|
|
||||||
@@ -306,7 +310,7 @@ Short description for providers without native calling specs:
|
|||||||
|
|
||||||
- **plan_write**: Create or update the Plan with YAML content
|
- **plan_write**: Create or update the Plan with YAML content
|
||||||
- Format: {\"tool\": \"plan_write\", \"args\": {\"plan\": \"plan_id: my-plan\\nitems: [...]\"}}
|
- Format: {\"tool\": \"plan_write\", \"args\": {\"plan\": \"plan_id: my-plan\\nitems: [...]\"}}
|
||||||
- Example: {\"tool\": \"plan_write\", \"args\": {\"plan\": \"plan_id: feature-x\\nitems:\\n - id: I1\\n description: Add feature\\n state: todo\\n touches: [src/lib.rs]\\n checks:\\n happy: {desc: Works, target: lib}\\n negative: {desc: Errors, target: lib}\\n boundary: {desc: Edge, target: lib}\"}}
|
- Example: {\"tool\": \"plan_write\", \"args\": {\"plan\": \"plan_id: feature-x\\nitems:\\n - id: I1\\n description: Add feature\\n state: todo\\n touches: [src/lib.rs]\\n checks:\\n happy: {desc: Works, target: lib}\\n negative:\\n - {desc: Errors, target: lib}\\n boundary:\\n - {desc: Edge, target: lib}\"}}
|
||||||
|
|
||||||
- **plan_approve**: Approve the current plan revision (called by user)
|
- **plan_approve**: Approve the current plan revision (called by user)
|
||||||
- Format: {\"tool\": \"plan_approve\", \"args\": {}}
|
- Format: {\"tool\": \"plan_approve\", \"args\": {}}
|
||||||
|
|||||||
@@ -289,14 +289,14 @@ fn create_core_tools(exclude_research: bool) -> Vec<Tool> {
|
|||||||
- touches: Array of paths/modules affected
|
- touches: Array of paths/modules affected
|
||||||
- checks:
|
- checks:
|
||||||
happy: {desc, target} - Normal successful operation
|
happy: {desc, target} - Normal successful operation
|
||||||
negative: {desc, target} - Error handling, invalid input
|
negative: [{desc, target}, ...] - Error handling, invalid input (>=1 required)
|
||||||
boundary: {desc, target} - Edge cases, limits
|
boundary: [{desc, target}, ...] - Edge cases, limits (>=1 required)
|
||||||
- evidence: Array of file:line refs, test names (required when done)
|
- evidence: Array of file:line refs, test names (required when done)
|
||||||
- notes: Implementation explanation (required when done)
|
- notes: Implementation explanation (required when done)
|
||||||
|
|
||||||
Rules:
|
Rules:
|
||||||
- Keep items ≤ 7 by default
|
- Keep items ≤ 7 by default
|
||||||
- All three checks (happy, negative, boundary) are required
|
- All checks required: 1 happy, 1+ negative, 1+ boundary
|
||||||
- Cannot remove items from an approved plan (mark as blocked instead)
|
- Cannot remove items from an approved plan (mark as blocked instead)
|
||||||
- Evidence and notes required when marking item as done"#.to_string(),
|
- Evidence and notes required when marking item as done"#.to_string(),
|
||||||
input_schema: json!({
|
input_schema: json!({
|
||||||
|
|||||||
@@ -96,18 +96,30 @@ impl Check {
|
|||||||
pub struct Checks {
|
pub struct Checks {
|
||||||
/// Happy path check - normal successful operation
|
/// Happy path check - normal successful operation
|
||||||
pub happy: Check,
|
pub happy: Check,
|
||||||
/// Negative case check - error handling, invalid input
|
/// Negative case checks - error handling, invalid input (at least 1 required)
|
||||||
pub negative: Check,
|
pub negative: Vec<Check>,
|
||||||
/// Boundary condition check - edge cases, limits
|
/// Boundary condition checks - edge cases, limits (at least 1 required)
|
||||||
pub boundary: Check,
|
pub boundary: Vec<Check>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Checks {
|
impl Checks {
|
||||||
/// Validate all three checks.
|
/// Validate all checks (1 happy, 1+ negative, 1+ boundary).
|
||||||
pub fn validate(&self) -> Result<()> {
|
pub fn validate(&self) -> Result<()> {
|
||||||
self.happy.validate().map_err(|e| anyhow!("happy check: {}", e))?;
|
self.happy.validate().map_err(|e| anyhow!("happy check: {}", e))?;
|
||||||
self.negative.validate().map_err(|e| anyhow!("negative check: {}", e))?;
|
|
||||||
self.boundary.validate().map_err(|e| anyhow!("boundary check: {}", e))?;
|
if self.negative.is_empty() {
|
||||||
|
return Err(anyhow!("at least one negative check is required"));
|
||||||
|
}
|
||||||
|
for (i, check) in self.negative.iter().enumerate() {
|
||||||
|
check.validate().map_err(|e| anyhow!("negative check [{}]: {}", i, e))?;
|
||||||
|
}
|
||||||
|
|
||||||
|
if self.boundary.is_empty() {
|
||||||
|
return Err(anyhow!("at least one boundary check is required"));
|
||||||
|
}
|
||||||
|
for (i, check) in self.boundary.iter().enumerate() {
|
||||||
|
check.validate().map_err(|e| anyhow!("boundary check [{}]: {}", i, e))?;
|
||||||
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -927,8 +939,8 @@ mod tests {
|
|||||||
fn make_test_checks() -> Checks {
|
fn make_test_checks() -> Checks {
|
||||||
Checks {
|
Checks {
|
||||||
happy: make_test_check(),
|
happy: make_test_check(),
|
||||||
negative: make_test_check(),
|
negative: vec![make_test_check()],
|
||||||
boundary: make_test_check(),
|
boundary: vec![make_test_check()],
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -969,6 +981,55 @@ mod tests {
|
|||||||
assert!(empty_target.validate().is_err());
|
assert!(empty_target.validate().is_err());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_checks_validation_multiple_negative_and_boundary() {
|
||||||
|
// Multiple negative and boundary checks should validate
|
||||||
|
let checks = Checks {
|
||||||
|
happy: make_test_check(),
|
||||||
|
negative: vec![
|
||||||
|
Check::new("Invalid input", "parse::input"),
|
||||||
|
Check::new("Missing file", "io::read"),
|
||||||
|
Check::new("Network error", "net::connect"),
|
||||||
|
],
|
||||||
|
boundary: vec![
|
||||||
|
Check::new("Empty input", "parse::input"),
|
||||||
|
Check::new("Max size input", "parse::input"),
|
||||||
|
],
|
||||||
|
};
|
||||||
|
assert!(checks.validate().is_ok());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_checks_validation_empty_negative_fails() {
|
||||||
|
let checks = Checks {
|
||||||
|
happy: make_test_check(),
|
||||||
|
negative: vec![], // Empty - should fail
|
||||||
|
boundary: vec![make_test_check()],
|
||||||
|
};
|
||||||
|
let err = checks.validate().unwrap_err();
|
||||||
|
assert!(err.to_string().contains("at least one negative check"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_checks_validation_empty_boundary_fails() {
|
||||||
|
let checks = Checks {
|
||||||
|
happy: make_test_check(),
|
||||||
|
negative: vec![make_test_check()],
|
||||||
|
boundary: vec![], // Empty - should fail
|
||||||
|
};
|
||||||
|
let err = checks.validate().unwrap_err();
|
||||||
|
assert!(err.to_string().contains("at least one boundary check"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_checks_validation_single_of_each_passes() {
|
||||||
|
// Minimum case: exactly 1 of each
|
||||||
|
let checks = make_test_checks();
|
||||||
|
assert!(checks.validate().is_ok());
|
||||||
|
assert_eq!(checks.negative.len(), 1);
|
||||||
|
assert_eq!(checks.boundary.len(), 1);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_plan_item_validation() {
|
fn test_plan_item_validation() {
|
||||||
let item = make_test_item("I1");
|
let item = make_test_item("I1");
|
||||||
|
|||||||
Reference in New Issue
Block a user