From 96627a75b94af15d6f65ffedb5b50571760c49e0 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Tue, 7 Jul 2026 23:14:50 +0530 Subject: [PATCH 1/2] feat(ponytail): regex-free over-engineering detector + numbered review findings - Add detect_over_engineering() with substring matching (stdlib only, no deps) - Detects: lodash, moment, axios imports, JSON.parse(JSON.stringify) antipattern - Numbered findings format in review and audit skill prompts - Common patterns pre-filter table in review skill - 5 tests covering all patterns + clean code passthrough - Closes ponytail PR audit tickets #63, #78 --- crates/ponytail/src/skill-audit.md | 2 +- crates/ponytail/src/skill-review.md | 29 ++++++---- crates/ponytail/src/sub_skills.rs | 83 +++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 10 deletions(-) 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..b2e5c77f 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..387aee83 100644 --- a/crates/ponytail/src/sub_skills.rs +++ b/crates/ponytail/src/sub_skills.rs @@ -14,3 +14,86 @@ 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\"", "require(\"lodash\")", "import _"]), + ("moment", "native", "Use Intl.DateTimeFormat, Date.toLocaleDateString, or Temporal.", &["from \"moment\"", "require(\"moment\")"]), + ("axios", "native", "Use native fetch() instead of axios.", &["from \"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()); + } +} From f57425b39a5e18dc3b684e7af6f8574261f366d4 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Wed, 8 Jul 2026 00:04:39 +0530 Subject: [PATCH 2/2] =?UTF-8?q?fix(ponytail):=20address=20CodeRabbit=20rev?= =?UTF-8?q?iew=20=E2=80=94=20single-quote=20imports,=20false-positive=20fi?= =?UTF-8?q?x,=20example=20format?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add single-quoted import variants for lodash/moment/axios detection - Remove bare 'import _' false-positive, scope to lodash-specific forms - Fix skill-review.md examples to use required :L format - Add tests for single-quote imports and non-lodash _ import --- crates/ponytail/src/skill-review.md | 8 ++++---- crates/ponytail/src/sub_skills.rs | 19 ++++++++++++++++--- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/crates/ponytail/src/skill-review.md b/crates/ponytail/src/skill-review.md index b2e5c77f..9bbc5cab 100644 --- a/crates/ponytail/src/skill-review.md +++ b/crates/ponytail/src/skill-review.md @@ -42,15 +42,15 @@ Tags: ❌ "This EmailValidator class might be more complex than necessary, have you considered whether all these validation rules are needed at this stage?" -✅ `1. 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.` -✅ `2. 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.` ✅ `3. repo.py:L88: yagni: AbstractRepository with one implementation. Inline it until a second one exists.` -✅ `4. 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.` -✅ `5. 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 387aee83..190e386e 100644 --- a/crates/ponytail/src/sub_skills.rs +++ b/crates/ponytail/src/sub_skills.rs @@ -32,9 +32,9 @@ pub fn detect_over_engineering(text: &str) -> Vec { 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\"", "require(\"lodash\")", "import _"]), - ("moment", "native", "Use Intl.DateTimeFormat, Date.toLocaleDateString, or Temporal.", &["from \"moment\"", "require(\"moment\")"]), - ("axios", "native", "Use native fetch() instead of axios.", &["from \"axios\"", "require(\"axios\")"]), + ("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("]), ]; @@ -96,4 +96,17 @@ mod tests { 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()); + } }