Skip to content

chore: update vendored skills… #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marola-pr-bot wants to merge 1 commit into
base: main
Choose a base branch
from
+224 −186
Open

chore: update vendored skills… #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 89 additions & 27 deletions plugins/marola-devkit/skills/ponytail-audit/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,103 @@
---
name: ponytail-audit
description: >
Whole-repo audit for over-engineering. Like ponytail-review, but scans the
entire codebase instead of a diff: a ranked list of what to delete, simplify,
or replace with stdlib/native equivalents. Use when the user says "audit this
codebase", "audit for over-engineering", "what can I delete from this repo",
"find bloat", "ponytail-audit", or "/ponytail-audit". One-shot report, does
not apply fixes.
Quality audit of a whole repo: bugs, security holes, what breaks under real
load, risky code without tests, slow paths, and what to delete, merge or
split. Ranked, each finding explained in plain English. One-shot report,
changes nothing. Use for "audit this codebase", "review the whole repo",
"find bloat", "what can I delete", /ponytail-audit.
---

ponytail-review, repo-wide. Scan the whole tree instead of a diff. Rank
findings biggest cut first.
Audit the whole repo like the senior developer who just inherited it and
will be paged when it breaks. Order of importance: correct, safe, holds under
load, tested, fast, lean. Lean still matters: every extra line must be read,
tested and fixed later. This is a report the user asked for, so give it in full.

## Tags
## 1. Map first

Same as ponytail-review:
- Audit what the user names: a folder, a package, or the whole repo.
Nothing named: the whole repo.
- Read the README, the deploy and build config, the dependency list, the
entry points (main, routes, handlers, jobs, CLI commands) and the tests.
- Find the expected load: one person running a script, or many users and
processes at once. Judge scale against that, and say which load you assumed.
- Trace the main flows end to end: where data comes in, what is stored,
what goes out. Read those paths fully: input from users, money, auth,
data writes, background jobs, anything shared between processes.
- Big repo: go deep where a mistake costs the most, not file by file. Say
which parts you did not read.

- `delete:` dead code, unused flexibility, speculative feature. Replacement: nothing.
- `stdlib:` hand-rolled thing the standard library ships. Name the function.
- `native:` dependency or code doing what the platform already does. Name the feature.
- `yagni:` abstraction with one implementation, config nobody sets, layer with one caller.
- `shrink:` same logic, fewer lines. Show the shorter form.
## 2. Look for

## Hunt
1. **Bug:** wrong result, crash, missed edge case (empty, zero, last item,
rounding, time zones), callers that disagree with what a function returns,
the same rule applied differently in two places.
2. **Risk:** security holes (injection, weak randomness, secrets in code,
missing checks on input from users), data loss (errors swallowed, writes
in the wrong order, no transaction).
3. **Scale:** fine for one user, wrong for many: check-then-write races, the
same work done by every process, memory or lists that only grow, a query
per item, O(n^2) on big input, per-process state that must be shared.
4. **Missing test:** risky logic (a branch, a parser, money, security, data
writes) with no test that fails when it breaks. One good test, not coverage.
5. **Speed:** big slowdowns are problems. Small wins (work repeated in a hot
loop) are suggestions; some software counts every millisecond.
6. **Lean:** code that should not exist or should be smaller.
- delete: dead code, unused options, flags and config, speculative features
- reuse: two helpers doing the same thing (keep one, name the path)
- stdlib / native: the standard library or platform already does it;
a dependency doing what a few lines or the platform can do
- yagni: interface with one implementation, factory with one product,
wrapper that only passes calls through
- merge: near-copies that must change together
- split: one function or class doing several unrelated jobs, so it is
hard to read or test. Split by job, never by line count, and never into
helpers that exist only to make a function shorter.

Deps the stdlib or platform already ships, single-implementation interfaces,
factories with one product, wrappers that only delegate, files exporting one
thing, dead flags and config, hand-rolled stdlib.
## 3. Check before you report

## Output
- Every finding needs a concrete case: "this input or situation leads to this
wrong result". No case, no finding.
- Before calling code unused, grep the whole tree for it, including tests,
fixtures, config, and string or dynamic references.
- A shortcut marked with a `shortcut:` (or older `ponytail:`) comment that names its limit is a
decision, not a finding, unless the expected load already crosses it.
- Propose the smallest fix that works. Prefer fixes that delete code. Never
add layers, frameworks or config the problem does not need.
- No style taste, no "consider", no vague worries.

One line per finding, ranked: `<tag> <what to cut>. <replacement>. [path]`.
End with `net: -<N> lines, -<M> deps possible.` Nothing to cut: `Lean already. Ship.`
## 4. Output

## Boundaries
Very simple English: short sentences, everyday words. Explain a technical
term the first time you use it. The reader may never have seen this code.

Scope: over-engineering and complexity only. Correctness bugs, security holes,
and performance are explicitly out of scope. Route them to a normal review
pass. Lists findings, applies nothing. One-shot.
"stop ponytail-audit" or "normal mode" to revert.
Start with `What this repo does:` in two or three sentences, and the load
you assumed.

Then the findings in three groups, most important first, skip empty groups:
- **Must fix:** bug, security, data loss, breaks at the expected load.
- **Should fix:** risky code without a test, real slowness, duplication, a
function that mixes jobs, code that should not exist.
- **Nice to have:** small speed-ups, shorter forms.

Number findings across all groups, so the user can say "fix 2 and 5". At
most 20 findings; if you left smaller ones out, say how many.
Every finding has all four parts, each one or two short sentences:

2. **Orders land on the wrong day** (`billing/close_day.py:L40-52`)
- **What this is:** At midnight this job closes the day and bills all orders of that day.
- **Problem:** It takes "today" from the server clock, which runs in UTC. An order placed
at 00:30 in Berlin is billed on the day before.
- **Fix:** Compute the day once in the shop's time zone:
`datetime.now(ZoneInfo("Europe/Berlin")).date()`. One line, nothing else changes.
- **If we skip it:** Late orders show the wrong date, and accounting fixes them by hand.

End with:
- `Verdict:` one line: healthy, or what to fix first.
- `Lean: -<N> lines, -<M> dependencies possible.` when lean findings exist.
- `Not checked:` the parts you did not read or could not run.

Nothing found: `What this repo does:`, then `Healthy. Nothing to fix.` and
one line on what you checked.

One-shot report, changes no code.
148 changes: 96 additions & 52 deletions plugins/marola-devkit/skills/ponytail-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,101 @@
---
name: ponytail-review
description: >
Code review focused exclusively on over-engineering. Finds what to delete:
reinvented standard library, unneeded dependencies, speculative abstractions,
dead flexibility. One line per finding: location, what to cut, what replaces
it. Use when the user says "review for over-engineering", "what can we
delete", "is this over-engineered", "simplify review", or invokes
/ponytail-review. Complements correctness-focused review, this one only
hunts complexity.
Quality review of a change: is the logic right, is it safe, does it hold
under real load, is risky code tested, is it fast enough, and is every line
needed. Reads the connected code, not only the diff. Each finding is
explained in plain English. Use for "review this", "code review", "review
the last commit", "review my PR", "is this over-engineered", /ponytail-review.
---

Review diffs for unnecessary complexity. One line per finding: location, what
to cut, what replaces it. The diff's best outcome is getting shorter.

## Format

`L<line>: <tag> <what>. <replacement>.`, or `<file>:L<line>: ...` for
multi-file diffs.

Tags:

- `delete:` dead code, unused flexibility, speculative feature. Replacement: nothing.
- `stdlib:` hand-rolled thing the standard library ships. Name the function.
- `native:` dependency or code doing what the platform already does. Name the feature.
- `yagni:` abstraction with one implementation, config nobody sets, layer with one caller.
- `shrink:` same logic, fewer lines. Show the shorter form.

## 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.`

✅ `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.`

✅ `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.`

## Scoring

End with the only metric that matters: `net: -<N> lines possible.`

If there is nothing to cut, say `Lean already. Ship.` and stop.

## Boundaries

Scope: over-engineering and complexity only. Correctness bugs, security holes,
and performance are explicitly out of scope. Route them to a normal review
pass, not this one. A single smoke test or `assert`-based
self-check is the ponytail minimum, not bloat, never flag it for deletion.
Does not apply the fixes, only lists them.
"stop ponytail-review" or "normal mode": revert to verbose review style.
Review a change like the senior developer who will be paged when it breaks.
Order of importance: correct, safe, holds under load, tested, fast, lean.
Lean still matters: every extra line must be read, tested and fixed later.
This is a report the user asked for, so give it in full.

## 1. Understand first

- Review what the user names: uncommitted or staged changes, a branch, a PR
link, or files. Nothing named: the uncommitted changes, or the last commit
if there are none.
- Read the diff, then the code it touches: callers of every changed function,
the functions it calls, the tests, the README.
- Trace the real flow: where data comes in, what is stored, what goes out.
- A change can break code it does not touch. When a signature, return value
or behavior changes, grep every caller.
- Find the expected load in the repo (README, deploy config): one person
running a script, or many users and processes at once. Judge scale against
that, and say which load you assumed.

## 2. Look for

1. **Bug:** wrong result, crash, missed edge case (empty, zero, last item,
rounding, time zones), a caller broken by the change, a fix applied in one
caller while the shared function stays broken.
2. **Risk:** security holes (injection, weak randomness, secrets, missing
checks on input from users), data loss (errors swallowed, writes in the
wrong order, no transaction).
3. **Scale:** fine for one user, wrong for many: check-then-write races, the
same work done by every process, memory or lists that only grow, a query
per item, O(n^2) on big input, per-process state that must be shared.
4. **Missing test:** risky new logic (a branch, a parser, money, security,
data writes, a bug fix) with no test that fails when it breaks. One good
test, not coverage.
5. **Speed:** big slowdowns are problems. Small wins (work repeated in a hot
loop) are suggestions; some software counts every millisecond.
6. **Lean:** code that should not exist or should be smaller.
- delete: dead code, unused options, speculative features
- reuse: the repo already has this helper (name the path)
- stdlib / native: the standard library or platform already does it; a
new dependency for a few lines
- yagni: abstraction with one implementation, config nobody sets
- merge: near-copies that must change together
- split: one function doing several unrelated jobs, so it is hard to read
or test. Split by job, never by line count, and never into helpers that
exist only to make a function shorter.

## 3. Check before you report

- Every finding needs a concrete case: "this input or situation leads to this
wrong result". No case, no finding.
- Re-read the lines and confirm: the caller exists, the value can really be
empty, the code really is unused.
- A shortcut marked with a `shortcut:` (or older `ponytail:`) comment that names its limit is a
decision, not a finding, unless the expected load already crosses it.
- Propose the smallest fix that works. Prefer fixes that delete code. Never
add layers, frameworks or config the problem does not need.
- No style taste, no "consider", no vague worries.

## 4. Output

Very simple English: short sentences, everyday words. Explain a technical
term the first time you use it. The reader may never have seen this code.

Start with `What this change does:` in two or three sentences.

Then the findings in three groups, skip empty groups:
- **Must fix:** bug, security, data loss, breaks at the expected load.
- **Should fix:** risky code without a test, real slowness, duplication, a
function that mixes jobs, code that should not exist.
- **Nice to have:** small speed-ups, shorter forms.

Number findings across all groups, so the user can say "fix 2 and 5".
Every finding has all four parts, each one or two short sentences:

2. **Orders land on the wrong day** (`billing/close_day.py:L40-52`)
- **What this is:** At midnight this job closes the day and bills all orders of that day.
- **Problem:** It takes "today" from the server clock, which runs in UTC. An order placed
at 00:30 in Berlin is billed on the day before.
- **Fix:** Compute the day once in the shop's time zone:
`datetime.now(ZoneInfo("Europe/Berlin")).date()`. One line, nothing else changes.
- **If we skip it:** Late orders show the wrong date, and accounting fixes them by hand.

End with:
- `Verdict: Ship.` or `Verdict: fix 1 and 3 first.`
- `Lean: -<N> lines possible.` when lean findings exist.
- `Not checked:` one line, if something mattered and you could not check it.

Nothing found: `What this change does:`, then `Looks good. Ship.` and one line
on what you checked.

Lists findings, changes no code.
Loading
Loading