Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 8 additions & 3 deletions docs/skills/ce-simplify-code.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ The compound-engineering ideation chain is `/ce-ideate → /ce-brainstorm → /c
|----------|--------|
| What does it do? | Spawns three parallel reviewer agents on the recently-changed code, applies their findings, and verifies behavior is preserved |
| When to use it | Before opening a PR; after writing a feature; after AI generated code that works but feels heavy |
| What it produces | Updated code (in place) + a summary of what was changed, what was good as-is, which checks ran, and a quantified impact by dimension (fixes applied per reuse/quality/efficiency, skipped count, verification result) |
| What it produces | Updated code (in place) + a summary of what was changed, what was good as-is, which checks ran, and a quantified impact by dimension (fixes applied per reuse/quality/efficiency, skipped count, verification result); design-level "should this construct exist" findings are reported with tradeoffs, not auto-applied |
| What's next | Open the PR via `/ce-commit-push-pr` |

---
Expand Down Expand Up @@ -54,8 +54,8 @@ The orchestrator aggregates their findings, applies fixes, and runs typecheck +

A single "review and improve" prompt collapses into the agent's most-trained directions. Three reviewers each focused on one dimension cover meaningfully more ground:

- **Reuse** — searches for existing utilities and helpers; flags new functions that duplicate existing ones; flags inline logic that could use an existing utility; flags diff code that reimplements a language standard-library or runtime primitive (gated on behavior-equivalence, excluding UX-changing swaps)
- **Quality** — redundant state, parameter sprawl, copy-paste with variation, leaky abstractions, stringly-typed code, unnecessary wrappers (in component-tree UI frameworks), deeply nested conditionals, unnecessary comments, dead code / unused imports / unused exports
- **Reuse** — searches for existing utilities and helpers; flags new functions that duplicate existing ones; flags inline logic that could use an existing utility; flags diff code that reimplements a language standard-library or runtime primitive (gated on behavior-equivalence, excluding UX-changing swaps); flags code that hand-maintains a guarantee the platform, framework, or downstream layer already provides (e.g., a field list mirroring a schema the serving layer already projects to)
- **Quality** — redundant state, parameter sprawl, copy-paste with variation (checking whether the duplicated construct can be eliminated entirely before proposing a merge), leaky abstractions, stringly-typed code, unnecessary wrappers (in component-tree UI frameworks), deeply nested conditionals, unnecessary comments, dead code / unused imports / unused exports — without inventing intent to clear a suspicious construct it hasn't verified
- **Efficiency** — unnecessary work (redundant computations, repeat reads), missed concurrency, hot-path bloat, recurring no-op updates, TOCTOU pre-checks, memory issues, overly broad operations

### 2. Smart scope detection — user-named > git diff > recent edits
Expand All @@ -64,6 +64,8 @@ The skill resolves the simplification scope in priority order: explicit user-nam

### 3. Behavior preservation verification

Behavior is judged at the observable contract boundary — API responses, persisted data, emitted events, errors, side effects — with explicit proof required when internal shape changes but the external contract doesn't. Findings that would restructure internal (non-contractual) shape are **reported as design-level recommendations with tradeoffs, never auto-applied**. Before applying anything, the orchestrator also checks whether multiple findings cluster on one construct — a signal the construct itself may be unnecessary — and resolves that necessity question before applying fixes that would entrench it.

After applying fixes, the skill runs typecheck and lint over the project and runs tests scoped to the changed paths (broadening when the change has wide reach — e.g., a heavily-imported utility was rewritten). Failures are surfaced clearly with the failing check name and relevant output. **The skill refuses to relax assertions, weaken type signatures, or skip tests to make checks pass** — either fix the underlying break or revert the specific simplification that caused it. It also **never simplifies away a safety check** — input validation at trust boundaries, data-loss-preventing error handling, security checks, and accessibility affordances are preserved even when a finding frames them as removable boilerplate.

### 4. Mid-tier model selection — cost-aware
Expand Down Expand Up @@ -190,6 +192,9 @@ The orchestrator aggregates findings and applies them directly. If a finding is
**What if applying fixes breaks tests?**
The skill won't relax assertions, weaken type signatures, or skip tests to paper over the break. Either it fixes the underlying issue introduced by the simplification, or it reverts the specific change that caused the regression. The premise is preservation of exact functionality.

**What if the simpler design isn't strictly behavior-preserving?**
Some of the highest-value findings are "this construct may not need to exist at all" — a hand-maintained list a downstream layer already makes redundant, duplicate blocks that could be deleted rather than merged. When acting on one would change internal (non-contractual) shape, the skill reports it prominently in the summary with the tradeoffs both ways instead of applying it — that call belongs to you. This keeps the pass from entrenching a questionable construct by polishing around it.

**Why isn't simplification just part of the original write?**
It can be, but in practice the moment to find an existing utility is when you're searching for it, not when you're writing the feature. A separate refinement pass with parallel cross-cutting search catches things the original write didn't.

Expand Down
8 changes: 7 additions & 1 deletion skills/ce-simplify-code/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ Do not paraphrase these rubrics from memory — read each file and pass it verba

Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on. Do not argue with the finding or raise questions to the user, just skip it.

Before applying each fix, confirm it preserves behavior: same output for every input, same error behavior, and same side effects and ordering. If a fix can't clear that test, skip it — automated checks in Step 4 don't cover every behavior.
**Synthesis before fixing.** Before applying anything, look across the aggregated findings for clustering: if two or more findings touch the same construct (the same helper, field list, mapping table, wrapper, config constant) — including one reviewer *clearing* it while another proposes cleanup around it — treat the cluster as a signal that the construct itself may be the issue. Run the necessity question on it first: can it be derived from an existing source of truth, or does the platform/framework/downstream layer already provide its guarantee? Do not apply pattern-level fixes (merging duplicates, optimizing around it) that would entrench a construct whose necessity is unresolved; resolve necessity first, and if the answer is "shouldn't exist," report that as a design-level finding instead.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make cleared constructs visible to synthesis

The synthesis rule depends on detecting when one reviewer clears a construct while another proposes cleanup around it, but the reviewers are only instructed to return findings or say there is nothing to flag, so this state is not present in the aggregated findings. In that scenario the orchestrator cannot form the intended cluster and may still apply the cleanup/merge that entrenches the construct; require reviewers to emit explicit clears for suspicious constructs or remove this case from the synthesis trigger.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f2afac5. Both personas' return format now carries an explicit clear/question channel: findings may be question findings (no concrete fix required), and a nothing-to-flag report must briefly note suspicious constructs considered-and-cleared with the reason. The synthesis trigger's clears clause now references data the reviewers actually emit.

Comment thread
chreho marked this conversation as resolved.

Before applying each fix, confirm it preserves behavior **at the observable contract boundary**: same API responses, same persisted data, same emitted events, same error behavior, and same side effects and ordering as seen by external consumers. Byte-identical internal values are not required when the external contract is provably unchanged (e.g., a downstream layer projects the result before it leaves the system) — but that proof must be explicit, not assumed. Clearing this test is necessary but not sufficient — a fix that removes or restructures a construct routes to the design-level channel below regardless of proof. If a fix can't clear that test, skip it — automated checks in Step 4 don't cover every behavior.

**Design-level findings are reported, not auto-applied.** A finding that would remove or restructure a construct in a way that changes internal (non-contractual) shape — payload transiting an internal boundary, logged structure, which fields get serialized — may well be the better design, but it trades on judgment the user owns (payload limits, defense-in-depth boundaries, future consumers). Include it prominently in the Step 5 summary with the tradeoffs stated both ways; do not silently apply it and do not silently drop it.
Comment thread
chreho marked this conversation as resolved.

**Never simplify away a safety check.** Input validation at trust boundaries, error handling that prevents data loss, security checks (authorization, escaping, sanitization), and accessibility affordances are not removable boilerplate — preserve them even when a finding frames them as redundant or inline-able. Code that drops one of these is not simpler, it is unfinished. If a proposed simplification would thin or remove one, skip it.

Expand All @@ -61,4 +65,6 @@ If no test suite, lint, or typecheck is configured, state that explicitly in the

Briefly summarize what was good vs improved and fixed, including which checks were run and their results. If there were no findings to act on, confirm the code didn't require any changes.

**Surface design-level findings first.** Any "this construct may not need to exist" finding from the synthesis pass (Step 3) leads the summary — stated as a recommendation with tradeoffs both ways, explicitly marked as not applied. These are the highest-value output of the pass; a summary that lists applied polish but buries an unresolved necessity question inverts the priority.

**Quantify the impact by dimension.** Report what was actually applied, not a line count: fixes applied per reviewer dimension (reuse, quality, efficiency), how many findings were skipped as false-positive or not worth addressing, and the behavior-preservation result (checks run and outcome). For example: "Applied 6 — reuse 2, quality 3, efficiency 1; skipped 2 false positives; typecheck + lint clean, 11 scoped tests pass." Do not headline a net-lines-removed figure or frame fewer lines as the win — many clarity, safety, and efficiency fixes preserve or add lines. The measure is what improved and that behavior held, not how much code shrank.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ You are the **Code Quality Reviewer**. You receive recently changed code as a di

1. **Redundant state**: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls
2. **Parameter sprawl**: adding new parameters to a function instead of generalizing or restructuring existing ones
3. **Copy-paste with slight variation**: near-duplicate code blocks that should be unified with a shared abstraction
3. **Copy-paste with slight variation**: near-duplicate code blocks. Before proposing a shared abstraction, first ask whether the duplicated construct can be **eliminated entirely** — derived from an existing source of truth, or made unnecessary by a guarantee the platform/framework/downstream layer already provides. Propose the merge only if elimination fails; consolidating an unnecessary thing entrenches it.
4. **Leaky abstractions**: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
5. **Stringly-typed code**: using raw strings where constants, enums (string unions), or branded types already exist in the codebase
6. **Unnecessary wrapper elements (framework-gated)**: in codebases that use a component-tree UI framework (React/JSX, Vue, Svelte, SwiftUI, Jetpack Compose, etc.), flag wrapper containers that add no layout value — check if inner component props (flexShrink, alignItems, etc.) already provide the needed behavior. Skip this rule entirely on codebases without such a framework.
Expand All @@ -12,4 +12,6 @@ You are the **Code Quality Reviewer**. You receive recently changed code as a di

**Balance — avoid over-simplification.** Every flag above has a failure mode in the opposite direction; fewer lines is not the goal, faster comprehension is. Do not inline a helper that gives a concept a name, merge unrelated logic into one function, or remove an abstraction that exists for testability/extensibility or whose purpose you haven't confirmed is obsolete (check `git blame` for the original intent). If a proposed change would be longer or harder to follow than the original, don't flag it.

Return each finding as: location (`file:line`), the issue, and the concrete fix. If there is nothing to flag, say so explicitly.
**But do not invent intent to clear a structure.** When you are about to bless a suspicious construct as "deliberate design," verify the claimed benefit is actually load-bearing — e.g., a "public-API allowlist" is only load-bearing if no downstream layer (serializer config, API gateway, GraphQL projection, ORM column set) already enforces the same boundary. A justification you inferred but could not verify is a **question finding** ("is this needed given X already does Y?"), not a cleared item. A hand-maintained list that mirrors another artifact (a schema, an enum, a table's columns) is a standing red flag: report it unless you confirmed it cannot be derived and no layer already provides the guarantee.

Return each finding as: location (`file:line`), the issue, and the concrete fix — or, for a question finding (an unverified justification per the guard above), the specific question. Whether or not you have findings, also briefly note any suspicious construct you considered and cleared and why — a clear alongside findings is what lets the orchestrator weigh it against other reviewers' findings on the same construct. If there is nothing to flag, say so explicitly.
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ You are the **Code Reuse Reviewer**. You receive recently changed code as a diff
2. **Flag any new function that duplicates existing functionality.** Suggest the existing function to use instead.
3. **Flag any inline logic that could use an existing utility** — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.
4. **Flag diff code that reimplements a language standard-library or runtime primitive** — a hand-written routine the built-in stdlib/runtime API already provides (e.g., a manual array-dedup loop where the language ships a set-based idiom, a hand-rolled deep-clone/deep-merge where the runtime has one). Suggest the built-in **only when it is behavior-equivalent** for the inputs actually in play. Do not propose swaps that change behavior or UX: native UI controls (e.g., a custom date picker to `<input type=date>`), locale/`Intl`-dependent formatting, sort-stability assumptions, and serialization edge cases differ from their hand-rolled versions and are out of scope for a behavior-preserving pass.
5. **Flag diff code that hand-maintains a guarantee the platform, framework, or downstream layer already provides.** "Existing functionality" is not limited to importable code — it includes infrastructure behavior: an API gateway/GraphQL layer that already projects responses to the schema or selection set, a serializer that already enumerates exactly the declared fields (e.g., pydantic `model_dump()`, an ORM's `__table__.columns`), a framework that already validates, escapes, or retries. New code duplicating one of these (a field whitelist mirroring a schema, manual response projection, re-validation of already-validated input) is missed reuse: name the layer that provides the guarantee and what the code collapses to without the hand-rolled version. If removal preserves the external contract but changes what transits an internal boundary (payload size, logged shape), report the finding **with that tradeoff stated** — do not stay silent, and do not present it as behavior-identical.

Return each finding as: location (`file:line`), the duplication or missed reuse, and the existing utility or built-in to use instead. If there is nothing to flag, say so explicitly.
Return each finding as: location (`file:line`), the duplication or missed reuse, and the existing utility or built-in to use instead — or, for a question finding (a construct whose necessity you could not verify either way), the specific question. Whether or not you have findings, also briefly note any suspicious construct you considered and cleared and why — a clear alongside findings is what lets the orchestrator weigh it against other reviewers' findings on the same construct. If there is nothing to flag, say so explicitly.