diff --git a/crates/ponytail/src/skill-audit.md b/crates/ponytail/src/skill-audit.md index 5582d103..2e495241 100644 --- a/crates/ponytail/src/skill-audit.md +++ b/crates/ponytail/src/skill-audit.md @@ -30,7 +30,7 @@ thing, dead flags and config, hand-rolled stdlib. ## Output -One line per finding, ranked: ` . . [path]`. +Number each finding. One line per finding, ranked: `. . . [path]`. End with `net: - lines, - deps possible.` Nothing to cut: `Lean already. Ship.` ## Boundaries diff --git a/crates/ponytail/src/skill-review.md b/crates/ponytail/src/skill-review.md index e137a855..9bbc5cab 100644 --- a/crates/ponytail/src/skill-review.md +++ b/crates/ponytail/src/skill-review.md @@ -10,13 +10,13 @@ description: > hunts complexity. --- -Review diffs for unnecessary complexity. One line per finding: location, what -to cut, what replaces it. The diff's best outcome is getting shorter. +Review diffs for unnecessary complexity. Number each finding sequentially. +One line per finding: location, what to cut, what replaces it. +The diff's best outcome is getting shorter. ## Format -`L: . .`, or `:L: ...` for -multi-file diffs. +`. :L: . .` Tags: @@ -26,20 +26,31 @@ Tags: - `yagni:` abstraction with one implementation, config nobody sets, layer with one caller. - `shrink:` same logic, fewer lines. Show the shorter form. +## Common patterns (pre-filter these before deeper analysis) + +| Pattern | Tag | Why | +|---------|-----|-----| +| moment.js import | native | Intl.DateTimeFormat or Temporal | +| lodash import | stdlib | Array.map, Array.filter, Object.keys | +| axios import | native | fetch() is built-in | +| JSON.parse(JSON.stringify(...)) | stdlib | structuredClone() | +| Trivial getter/setter class | yagni | Plain object, no class needed | +| Pass-through wrapper | yagni | Direct call, skip wrapper | + ## Examples ❌ "This EmailValidator class might be more complex than necessary, have you considered whether all these validation rules are needed at this stage?" -✅ `L12-38: stdlib: 27-line validator class. "@" in email, 1 line, real validation is the confirmation mail.` +✅ `1. :L12-38: stdlib: 27-line validator class. "@" in email, 1 line, real validation is the confirmation mail.` -✅ `L4: native: moment.js imported for one format call. Intl.DateTimeFormat, 0 deps.` +✅ `2. :L4: native: moment.js imported for one format call. Intl.DateTimeFormat, 0 deps.` -✅ `repo.py:L88: yagni: AbstractRepository with one implementation. Inline it until a second one exists.` +✅ `3. repo.py:L88: yagni: AbstractRepository with one implementation. Inline it until a second one exists.` -✅ `L52-71: delete: retry wrapper around an idempotent local call. Nothing replaces it.` +✅ `4. :L52-71: delete: retry wrapper around an idempotent local call. Nothing replaces it.` -✅ `L30-44: shrink: manual loop builds dict. dict(zip(keys, values)), 1 line.` +✅ `5. :L30-44: shrink: manual loop builds dict. dict(zip(keys, values)), 1 line.` ## Scoring diff --git a/crates/ponytail/src/sub_skills.rs b/crates/ponytail/src/sub_skills.rs index 0669781e..190e386e 100644 --- a/crates/ponytail/src/sub_skills.rs +++ b/crates/ponytail/src/sub_skills.rs @@ -14,3 +14,99 @@ pub fn get(name: &str) -> Option<&'static str> { _ => None, } } + +pub struct Finding { + pub line: usize, + pub tag: String, + pub problem: String, + pub replacement: String, + pub snippet: String, +} + +pub fn detect_over_engineering(text: &str) -> Vec { + let mut findings = Vec::new(); + ponytail_engineering_check_internal(text, &mut findings, 0); + findings.sort_by_key(|f| f.line); + findings +} + +fn ponytail_engineering_check_internal(text: &str, findings: &mut Vec, base_line: usize) { + let patterns: &[(&str, &str, &str, &[&str])] = &[ + ("lodash", "stdlib", "Use native JS methods: Array.map, Array.filter, Object.keys.", &["from \"lodash\"", "from 'lodash'", "require(\"lodash\")", "require('lodash')"]), + ("moment", "native", "Use Intl.DateTimeFormat, Date.toLocaleDateString, or Temporal.", &["from \"moment\"", "from 'moment'", "require(\"moment\")", "require('moment')"]), + ("axios", "native", "Use native fetch() instead of axios.", &["from \"axios\"", "from 'axios'", "require(\"axios\")", "require('axios')"]), + ("JSON.parse(JSON.stringify(", "stdlib", "Use structuredClone() for deep copy.", &["JSON.parse(JSON.stringify("]), + ]; + + for (line_num, line) in text.lines().enumerate() { + let lower = line.to_lowercase(); + for (_name, tag, replacement, needles) in patterns { + if needles.iter().any(|n| lower.contains(&n.to_lowercase())) { + let trimmed = line.trim().to_string(); + if !findings.iter().any(|f| f.line == base_line + line_num + 1 && f.problem == trimmed) { + findings.push(Finding { + line: base_line + line_num + 1, + tag: (*tag).to_string(), + problem: trimmed, + replacement: (*replacement).to_string(), + snippet: line.to_string(), + }); + } + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn detects_lodash_import() { + let findings = detect_over_engineering("import _ from \"lodash\";\nconst x = 1;"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].tag, "stdlib"); + assert!(findings[0].replacement.contains("Array.map")); + } + + #[test] + fn detects_moment_import() { + let findings = detect_over_engineering("import moment from \"moment\";"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].tag, "native"); + } + + #[test] + fn detects_axios_import() { + let findings = detect_over_engineering("const axios = require(\"axios\");"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].tag, "native"); + } + + #[test] + fn detects_deep_clone_antipattern() { + let findings = detect_over_engineering("const copy = JSON.parse(JSON.stringify(obj));"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].tag, "stdlib"); + assert!(findings[0].replacement.contains("structuredClone")); + } + + #[test] + fn clean_code_returns_empty() { + let findings = detect_over_engineering("const x = 1;\nfn foo() { Ok(()) }"); + assert!(findings.is_empty()); + } + + #[test] + fn detects_single_quoted_imports() { + let findings = detect_over_engineering("import lodash from 'lodash';"); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].tag, "stdlib"); + } + + #[test] + fn ignores_non_lodash_underscore_import() { + let findings = detect_over_engineering("import _ from \"underscore\";"); + assert!(findings.is_empty()); + } +}