Save this content to: .claude/skills/evaluate/SKILL.md

# Evaluate Implementation

A self-directed loop: evaluate your own work, fix what you find, re-evaluate until clean, then run a code review for a fresh-eyes pass. Do not ask the user to fix things — fix them yourself.

## When to Use This Skill

- After implementing a feature or fixing a bug (all code is written)
- When the user says "evaluate", "check this", or "review your work"
- Before creating a PR or marking work as done

**Note:** This skill is a completion-level activity. It runs the full `backend-quality` skill (including static analysis and full test suite). Do not use this skill mid-feature — only when the implementation is done.

## Workflow

### Phase 1: Run Quality Checks (Skip If Recent)

Before running checks, review the current conversation for recent quality check results. **Skip checks that already passed clean and where no code changes were made since.**

**Skip criteria — all must be true:**
1. The check was run earlier in this conversation (not a previous session)
2. The check passed with zero errors/failures
3. No files of that type were added, removed, or changed after the check passed

**What counts as "recently passed":**
- Code style: ran with no changes needed
- Static analysis: ran with 0 errors
- Tests: ran with 0 failures (full suite or all relevant tests)
- Type checking: ran with 0 errors
- Linting: a full run ran with 0 errors (a scoped run on a subset of files does not count)

**If checks can be skipped**, state which specific checks you're skipping and why. Skipping is decided **per individual check**, not all-or-nothing for an entire skill:
> "Skipping code style and static analysis — both passed clean earlier with no backend changes since; re-running tests to verify behavior."

**If any doubt**, run the checks. It's better to re-run than to miss a failure.

**When the project runs `sandermuller/boost-pipeline`**, a recorded run counts as evidence, and
beats your own recollection. This applies only then — a project without the package runs its
checks as below, and a package repo usually has no `php artisan` to call.

Then ask it rather than reconstructing what changed since which check:

```bash
status=0; php artisan pipeline:verify --server-verified || status=$?; echo "exit: $status"
```

Keep the exit code visible, and keep the step from aborting. `|| true` reports 0 for a missing
command and for a failed check alike, and the rules below turn on that number. A bare `;` before
the `echo` loses the status under `set -e`.

**Ask `--server-verified`, never the bare call.** The bare call asks whether the whole tree is
verified, which is a gate's question. A walk that sequences agent steps can never answer yes: an
acknowledged step is a self-report, and the server never counts one as verified. This skill asks
the narrower question it can answer — what has already been checked.

**When the project declares several pipelines**, name the one to ask about:
<!--boost:conv path="quality.pipeline" mode="inline"-->not configured — ask without `--pipeline`<!--boost:conv:end-->.
Append `--pipeline=<name>` when a name is set. When none is set the command reports an error that
names the pipelines it found. Run the checks normally then, and never guess which one to trust.

Exit 0 means every check the pipeline ran passed against **the code currently on disk**. The tree
is fingerprinted, so this is a comparison rather than a judgement. It answers criterion 3 above
with a measurement instead of your memory. The fingerprint reads file content, not the commit, so
a commit of all the verified code keeps the receipt valid. The message names the steps it counted:

> Run [r-4f2a] passed all 5 step(s) the server verified against this tree: [phpstan],
> [pint-test], [typecheck], [test-js], [lint-all].

Skip the mechanical checks those names cover, and run every check they do not. Deciding which
check an id covers is judgement, and judgement is this skill's half of the split. Do not add a
mapping, and do not read `receipt.json`. Say what was skipped and on whose authority:

> "Skipping code style, static analysis, type checking, linting and tests — `pipeline:verify
> --server-verified` exit 0 names [pint-test], [phpstan], [typecheck], [lint-all] and [test-js]
> as verified against this exact tree. Continuing to the review phases."

Name only what the message named. A check the names do not cover is a check nobody ran.

**Exit 0 is never a claim that the pipeline holds the checks you care about.** A pipeline that
declares no static analysis exits 0 without any. A step that rewrites the tree — a formatter, for
example — is left out of the names on purpose: it reports that it ran, never that the result is
correct. Skip only what the names cover.

Any non-zero exit means run the checks normally. It reports evidence too weak to skip anything
on. The causes differ — no run recorded, a run against different code, a walk that did not
finish, a step the server ran and failed, a run that verified nothing, a run under an older
pipeline definition, a partial commit (the commit moved and the tree still holds uncommitted
changes), or several pipelines with none named. Treat them alike. Do not read the reason as a
reason to skip anything.

This is the whole point of the split — the pipeline owns mechanical checks, this skill owns
judgement. Without it both run the same formatter, analyser and suite, and the project pays twice
for one answer.

**For every check still to run**, choose by which files changed:
- **Backend files changed** — use the `backend-quality` skill
- **JS/TS files changed** — use the `frontend-quality` skill

Fix all failures before continuing.

### Phase 2: Review for Issues

First, resolve the **evaluation scope** once — the set of changes this run is about — and reuse it for the rest of the skill (Phases 2 and 3). Resolve it in this order, stopping at the first that applies:

1. The commit range explicitly provided for this run.
2. Explicitly provided files, limited to the changes in them that belong to this task — intersect with the task's commits, or with the staged/uncommitted diff when there is no committed task work.
3. The task's committed history — against the base branch only when the branch contains nothing but this task, otherwise the specific commit range — plus any local edits on top.
4. The staged/uncommitted diff when all work is still local.
5. If none of these pins it down — committed task work mixed with unrelated changes you cannot separate — **stop and ask the user** which files or commit range to evaluate.

Never fall back to the whole-branch diff.

Read through all files in the resolved scope and check for:

| Category | What to look for |
|----------|-----------------|
| **Edge cases** | Null handling, empty collections, zero values, boundary conditions |
| **Race conditions** | Concurrent requests causing data corruption, non-atomic operations |
| **Security** | Missing auth checks, unvalidated input, XSS, SQL injection, type confusion |
| **Logic errors** | Wrong conditions, off-by-one errors, swallowed exceptions |
| **Missing tests** | Happy paths, failure paths, and edge cases that aren't tested. A test that exists but proves nothing counts here too — the `test-value` skill judges both directions |
| **Convention violations** | Deviations from project patterns (check sibling files) |
| **Cross-version compat** | Works across every runtime and dependency version the project supports |
| **Over-engineering** | Unrequested abstractions, speculative generality, premature flexibility; hand-rolled code a stdlib/native/framework feature or an already-installed dependency replaces; anything deletable without losing required behavior. A value carried in the wrong type belongs here too — the `simplify-shape` skill carries the ladder for it, and runs after the cutting pass below |

**Judge the over-engineering row at altitude first.** Dispatch the `simplification-auditor` subagent where the session has it: the author of a change is the reader least able to see what it did not need, and that subagent judges the diff in a fresh context. Run inline when it is absent — the questions survive, the distance does not, so answer each row against the diff and the requirement rather than your memory of why you wrote it, and say in the report that the pass was not independent. Before you judge any line for it, inventory what the change added — every **file, class, interface, trait, public method, config key, flag, route, migration, event, job and test** — and ask of each: does this unit need to exist at all? A line-level pass can only shorten a file. It never removes one, and a whole file that nothing calls costs more than any line inside it.

In a package or any code with downstream consumers, answer every row against those consumers too. An interface, event, config key or hook with no second implementation, listener or caller *in this repository* may still be the published extension point.

The `simplification-auditor` subagent carries this table in fuller form, plus the ledger it reports. This copy is the fix-loop version: keep them consistent when either changes.

| Unit the change added | Ask |
|---|---|
| A class, service, or action | Does more than one caller use it? Would the caller read better with the body inlined? |
| An interface, abstract class, or trait | Is there more than one implementation today? |
| A config key, setting, or feature flag | Does anything set it to a value other than the default? |
| A migration or column | Does the value have to be stored — for querying, indexing, history, or cost — or can a cast, an accessor, or a computation replace it? |
| An event | Does anything listen to it? |
| A listener or a queued job | Is it dispatched anywhere, and does the work need to leave the caller at all? |
| A route, or a new public method | Does anything call it, here or downstream? |
| A wrapper or adapter over one dependency | Does the indirection buy a swap anyone plans? |
| An unrequested `try`/`catch` | Does it handle a failure that can happen, or hide one? |

**Brevity has a floor.** Shortening code is a win only when nothing required is lost. Never trade away input validation at trust boundaries, error / data-loss handling, security, accessibility, explicitly-requested functionality, or a test for non-trivial logic to make code smaller. Delete the unrequested, not the necessary — and apply these cuts through the Phase 4 fix loop like any other finding.

**Complexity is the second floor.** Fewer lines that are harder to follow is a loss, not a win. Reject a cut that deepens nesting, adds indirection to save a line, chains or nests ternaries, folds two guard clauses into one compound condition, removes an early return that flattens the method, replaces named steps with one opaque expression or an unreadable pipe chain, or collapses a `match` or `switch` whose branches are the domain into a lookup nobody can read. Name the floor you invoked, brevity or complexity, in the reason you reject a candidate. A line count on its own rewards dense code.

**Account for what you did not cut.** Report "nothing to over-engineer here" only with the list behind it: the units the change added, and for each one you kept, the caller or the floor that keeps it. Without that list a reader cannot tell a clean change from an unexamined one.

**DB-driven translation keys** (only when a project translation-key policy is configured):

<!--boost:conv path="translations" mode="yaml"-->not configured — skip this check<!--boost:conv:end-->

If a policy is shown above, flag any DB-driven translation key that does not match its `key_pattern`. Exempt file-based keys: top-level groups listed in `file_based_prefixes.framework_groups`, and — when `vendor_namespace_exempt` is true — any key matching a vendor namespace prefix like `package::` (keys validated by the framework's own file-based lang files are out of scope). If `rules_doc` is set, also apply its naming-quality guidance. When nothing is configured, skip this check.

**Fixture / code-sample anonymization** (only when a project anonymization policy is configured):

<!--boost:conv path="fixtures.anonymization" mode="yaml"-->not configured — skip this check<!--boost:conv:end-->

If a policy is shown above, scan the files **in the resolved evaluation scope** (do not broaden) that fall under its `scope` paths for proprietary product domain leaking into a publicly-shipped package: real product entity / class names, real table / column names, route keys, domain jargon, and comments copied from a host application. `src/` ships in the Composer dist archive — code-sample heredocs (e.g. rule `CodeSample` blocks) there are the worst leak surface; `tests/` is usually `export-ignore`d from the archive but still lives in version control. Scan both per the configured `scope`; never narrow to `tests/` alone. When `forbidden_terms` is set, treat any literal match as a deterministic blocking hit; otherwise apply the judgment criteria in the `guideline` doc. Treat every hit as a blocking issue carried into Phase 4 — apply the fix directly when it is an unambiguous rename to a placeholder (per the fix-don't-report ethos), and flag it for the user only when anonymizing needs a judgment call. When nothing is configured, skip this check.

### Phase 3: Audit Code Comments

Within the **same evaluation scope resolved in Phase 2** (do not re-derive or broaden it), find every comment **added or changed** in this work. This covers **all** comment syntaxes in the changed languages, not only the obvious ones: docblocks and `//` / `#` / `/* */`, and template comments (`{{-- --}}`, `<!-- -->`). Do not skip template comments. Never judge pre-existing comments outside that scope.

Dispatch the `comment-analyzer` subagent where the session has it. It applies the ladder below in a fresh context, and it also checks the unchanged comments that describe code this work changed — a docblock above a method whose body changed, a comment beside a changed condition — for whether they are still true. Without it, read around every changed hunk for those comments yourself, and say in the report that the audit was not independent: the author reads a comment together with the intent behind it.

A comment earns its place **only when both** are true:
1. **Without it, a competent teammate reading the code (and any linked issue / PR) would draw the wrong conclusion or break it on edit** — not merely be curious. "Is there a real WHY?" is the wrong test; almost every line has one. A real-but-inferable why — the reader would understand it, just a little slower — is not enough to keep inline; that belongs in the tracker (a tracking link), not the source.
2. There is no better way to write the code that would make the comment unnecessary.

**Density is itself a signal.** After judging comments individually, look across the scope: if one function or method accrued more than a single surviving comment, treat that as a smell that the code wants splitting or renaming, not annotating — revisit those comments with a bias to Remove/Replace.

**Judge each comment on its own — never batch-justify.** A collective verdict ("these all explain real WHYs") is how a comment that merely restates the code survives.

For each added/changed comment, apply this decision ladder **in order** and stop at the first that fits:

| Verdict | When | Action |
|---------|------|--------|
| **Remove** | Comment restates what the code already says, narrates the obvious, or is a leftover (commented-out code, "TODO" with no tracking link, scaffolding chatter) | Delete it |
| **Replace with better code** | The need for the comment disappears if the code is rewritten — rename a variable/method/class, extract a well-named private method, or split a long function | Rewrite the code, drop the comment, re-run affected tests |
| **Trim / compact** | The WHY is genuinely needed but the comment is verbose, repeats itself, or buries the point | Reduce to the minimal sentence(s) that carry the constraint — prefer linking the issue or PR over re-explaining the whole case inline |
| **Keep as-is** | Already minimal, and without it a reader (with any linked issue) would get the code **wrong** — not just be curious | Leave it |

Prefer **Remove** and **Replace** over **Trim** — a comment that can be designed away is better than a shorter comment. Default to no comment; the bar to keep one is high.

Exempt — do not touch:
- Comments required by tooling or convention (e.g. static-analysis annotations, `@var` and type-hint docblocks the project's conventions mandate, IDE/linter directives, license headers).
- Comments that describe code the change did not touch. An unchanged comment on code this work changed is not exempt: check that it is still true.

Apply the Remove/Replace/Trim edits as part of this phase (this is your own work), then continue to Phase 4. If a rewrite needs a design decision, ask the user.

Re-run this audit over comments added by any **later** phase of the same run — a Phase 4 fix, or a comment written while applying review feedback. Those phases add comments without auditing them, so an audit that ran before them is not finished.

### Phase 4: Fix Issues

For each issue found:

1. Fix it yourself — do not list it as a suggestion for the user
2. Run the affected tests again to verify the fix
3. If the fix requires a design decision, ask the user

### Phase 5: Re-evaluate (Loop Until Clean)

After fixing issues, re-run only the checks affected by your fixes (e.g., if you only fixed backend files, skip frontend checks). Repeat until a full pass finds no new issues. Only then move to Phase 6.

### Phase 6: Code Review

Once the evaluate-fix loop is clean, run the `code-review` skill for a structured review from a different angle (functionality, UX/UI, security, testing). Fix any findings from the code review and re-verify.

### Phase 7: Codex Review

Run the `codex-review` skill for a multi-model second opinion. Critically evaluate and implement any warranted feedback, then re-verify. This phase applies regardless of how evaluate was invoked — directly by the user or as the closeout step of another flow (e.g. right before PR creation).

**Skip this phase only when both are true** (the same dedup logic as Phase 1, and the same freshness window the `pull-requests` pre-PR gate checks):

1. `codex-review` already ran in this conversation and ended clean — no warranted findings remaining (every finding applied or dismissed with reasoning)
2. No task files were added, removed, or changed since that clean run — any file counts (code, docs, skills, config), not only code; only edits inside agent-managed paths are exempt, mirroring the gate's `since_last_code_change` semantics

If in doubt, run it. If `codex-review` cannot run (Codex CLI missing or unauthenticated), note that in the Phase 8 report instead of silently skipping.

### Phase 8: Report

Summarize what you found and fixed across all passes:

```markdown
## Evaluation Summary

### Issues Found & Fixed
1. **[Issue]** — [What was wrong and how you fixed it]

### Verified
- All tests pass (X tests, Y assertions)
- Static analysis clean
- Code style clean

### No Issues Found In
- [Categories that were clean]
```

If no issues were found, say so briefly and move on.

## Parallel Execution

The phase order above is the canonical flow. When the harness can run independent subtasks concurrently (subagents, parallel tasks, a background shell), use that capability — as an optimization, never a requirement:

- **Phase 1** — backend and frontend checks are independent toolchains; run them concurrently.
- **Phases 2 + 3, finding only** — the review dimensions and the comment inventory are read-only lenses over the same resolved scope; fan them out as parallel subtasks and merge the findings before fixing anything.
- **Phases 6 + 7** — the external review (Phase 7) may be launched in the background when Phase 6 starts, since both review the same scope. Trade-off: if Phase 6 findings change code, the external review is stale — the `codex-review` skill's re-review rules then apply.

Hard rules, regardless of harness:

- **Fan out read-only work only.** All edits — comment cleanups (Phase 3), issue fixes (Phase 4), re-evaluation fixes (Phase 5) — happen serially in the main context: parallel editors conflict, and design decisions go through the user.
- **Do not nest.** If this skill is already running inside a subagent, run sequentially.
- **Degrade gracefully.** Concurrency semantics differ per harness (some block until all subtasks finish, some delegate only on explicit request, some have none) — when unsure, the sequential order is always correct. Harnesses with a scripted workflow feature may consolidate the fan-out into one workflow run; that is an optional escalation, never a dependency.

## Guidelines

- **Fix, don't report** — the point of this skill is to catch and fix issues, not to generate a list for the user
- **Loop until clean** — do not stop after the first fix pass; re-evaluate until nothing remains
- **Be thorough but fast** — check all dimensions but don't over-analyze obvious code
- **Run tests after every fix** — don't batch fixes and hope they all work
- **Trust existing patterns** — if the codebase does something a certain way consistently, follow it