feat(ponytail): regex-free over-engineering detector + numbered review findings - #84
Conversation
…w 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
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new ChangesOver-engineering Detector and Output Format
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/ponytail/src/sub_skills.rs (2)
60-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for single-quote imports and the
"import _"false-positive risk.All current tests use double-quoted import syntax. Given the gaps noted above, consider adding a test asserting
import _ from "underscore"(or similar non-lodashimport _) does not produce a lodash finding, and a test for single-quotedfrom 'lodash'/require('axios')style once that support is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ponytail/src/sub_skills.rs` around lines 60 - 99, Add coverage in the tests module for import parsing edge cases: extend the existing detect_over_engineering tests to verify single-quoted module syntax is handled for lodash/axios once supported, and add a regression test around detect_over_engineering / the lodash detection logic to ensure a generic import _ from a non-lodash package like underscore does not trigger a false-positive lodash finding. Use the existing test names in the tests module as anchors and keep the assertions aligned with the current findings tag/replacement behavior.
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
base_lineparameter adds speculative generality.
ponytail_engineering_check_internalis only ever invoked withbase_line = 0(Line 28), and no other call site exists in the provided code. The parameter and itsbase_line + line_num + 1arithmetic add complexity without current benefit — somewhat ironic for a detector built to flag over-engineering.If there's a planned future use (e.g. scanning multi-chunk diffs with an offset), consider adding a short comment noting that; otherwise inline the
0and drop the parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ponytail/src/sub_skills.rs` around lines 26 - 33, The helper in ponytail_engineering_check_internal has an unused speculative base_line parameter that is only passed as 0 from detect_over_engineering and adds unnecessary arithmetic. Remove the base_line parameter from ponytail_engineering_check_internal and inline the current line calculation so callers and internal logic stay simple, or if you intend to keep offset support, add a brief comment and use a real caller that passes a nonzero offset; update detect_over_engineering accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ponytail/src/skill-review.md`:
- Around line 45-47: The example bullets in the skill review content are not
following the required format because they omit the file prefix and use bare
line references; update the examples to match the exact template shape using the
same unique symbols and structure already present in this document. Keep the two
example entries consistent with the stricter guidance by preserving the required
file-scoped reference format and avoiding shorthand line notation.
In `@crates/ponytail/src/sub_skills.rs`:
- Line 35: The lodash detection in sub_skills.rs uses an overly broad needle, so
fix the lodash entry in the sub_skills list to match only lodash imports rather
than any default import named _. Update the needles associated with the lodash
rule so the matching logic in that data entry is scoped to lodash-specific
import forms, using the existing symbols around the "lodash" tuple in
sub_skills.rs.
- Around line 35-38: The needle list in sub_skills.rs only matches double-quoted
import forms, so single-quoted JS/TS imports can slip through undetected. Update
the pattern entries for the lodash, moment, and axios sub-skills to include
single-quote variants alongside the existing double-quote needles, keeping the
same intent in the common-patterns table. Use the existing tuple entries in
sub_skills.rs as the place to expand the import needles.
---
Nitpick comments:
In `@crates/ponytail/src/sub_skills.rs`:
- Around line 60-99: Add coverage in the tests module for import parsing edge
cases: extend the existing detect_over_engineering tests to verify single-quoted
module syntax is handled for lodash/axios once supported, and add a regression
test around detect_over_engineering / the lodash detection logic to ensure a
generic import _ from a non-lodash package like underscore does not trigger a
false-positive lodash finding. Use the existing test names in the tests module
as anchors and keep the assertions aligned with the current findings
tag/replacement behavior.
- Around line 26-33: The helper in ponytail_engineering_check_internal has an
unused speculative base_line parameter that is only passed as 0 from
detect_over_engineering and adds unnecessary arithmetic. Remove the base_line
parameter from ponytail_engineering_check_internal and inline the current line
calculation so callers and internal logic stay simple, or if you intend to keep
offset support, add a brief comment and use a real caller that passes a nonzero
offset; update detect_over_engineering accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6c343a0-eefa-4916-ba9c-8d43b8393c9b
📒 Files selected for processing (3)
crates/ponytail/src/skill-audit.mdcrates/ponytail/src/skill-review.mdcrates/ponytail/src/sub_skills.rs
…e-positive fix, example format - 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 <file>:L format - Add tests for single-quote imports and non-lodash _ import
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary by CodeRabbit