dev-workflow-forge

{
  "name": "dev-workflow-forge",
  "description": "A generator kit for Claude Code dev tooling. Three skills read your codebase and write a project-specific review skill, a rules set, and a self-learning feedback loop tuned to your stack, instead of handing you tooling written for someone else's project.",
  "version": "1.4.0",
  "author": {
    "name": "Ivan Misic",
    "url": "https://ivanmisic.net"
  },
  "homepage": "https://ivanmisic.net/toolshed/plugins/dev-workflow-forge",
  "repository": "https://github.com/imisic/claude-marketplace",
  "license": "MIT",
  "keywords": [
    "code-review",
    "coding-standards",
    "rules",
    "workflow",
    "scaffolding"
  ]
}

# dev-workflow-forge changelog

## 1.4.0

The generators now include the latest review, rules, and self-learning guardrails. The glob verifier carries a pinned local dependency instead of relying on a global npm install.

## 1.3.1

The README now says which commands you get after installing.

## 1.3.0

The skills in this plugin now carry an `agents/openai.yaml` sidecar, so they show up in Codex's skill picker with a proper name and one-line description instead of a raw folder name. All three are explicit-only in Codex as well, matching `disable-model-invocation` on the Claude Code side.

## 1.2.0

All three generators are now yours to invoke only. Each one writes files you then have to live with (a review skill, a rules set, proposed rule updates), so none of them should start on a model's judgement. Their descriptions are one line each instead of the 88 to 97-word trigger paragraphs they carried, which were only ever there to make auto-invocation fire.

## Earlier

1.1.0 and before predate this file. See the git history at https://github.com/imisic/claude-marketplace.

# dev-workflow-forge

A generator kit, not a fixed toolset. Three skills read your codebase and write project-specific dev tooling tuned to your stack.

Share the generator, not the generated output. A review skill someone else wrote for their PHP MVC app has no idea your project is a Rust CLI. The generators in this bundle read the actual codebase in front of them and produce a review skill, a rules set, and a self-learning loop that fit it, then get out of the way.

## The three generators

| Skill | Produces |
|-|-|
| `a-review-optimizer` | A project-specific `*-review` skill: parallel agents with non-overlapping scopes, a deterministic preflight script, fix-readiness output, `--debt`/`--full` modes where warranted |
| `a-rules-optimizer` | `.claude/rules/*.md` and a `CLAUDE.md` reference table, scoped to your directory layout and cross-checked against the review skill's checks |
| `a-self-learner` | Mines `.claude/reviews/review-issues.jsonl` for recurring findings and proposes rule or preflight updates, always with an approval gate before anything is written |

Each one is surgical: it preserves what already works in an existing skill or rule file and only changes what a concrete gap, false positive, or drift justifies. Run `a-review-optimizer` first on a new project; `a-rules-optimizer` and `a-self-learner` are more useful once a review skill exists to cross-reference.

## Use

```
/a-review-optimizer     # build or improve this project's review skill
/a-rules-optimizer      # audit or create .claude/rules/
/a-self-learner         # propose updates from accumulated review history
```

All three rewrite files you have to live with afterwards, so all three are yours to start. Claude will not reach for them on its own.

`a-rules-optimizer` includes a glob verifier that uses a pinned local `minimatch` dependency. After installing or updating this plugin, run `npm ci --prefix /path/to/this/skill/scripts` once before using the verifier. It installs beside the skill, not globally.

## The day-to-day loop

The generators produce the tooling; the everyday `fix`, `commit`, and `ship` commands that run on top of it live in the companion **dev-loop** plugin. Install both if you want the full pipeline.

## Install

```
/plugin marketplace add imisic/claude-marketplace
/plugin install dev-workflow-forge@imisic
```

Then the commands are `/a-review-optimizer`, `/a-rules-optimizer` and `/a-self-learner`.

The longer write-up lives on the storefront: [ivanmisic.net/toolshed/plugins/dev-workflow-forge](https://ivanmisic.net/toolshed/plugins/dev-workflow-forge).

---
name: a-review-optimizer
description: Read this codebase and write, or sharpen, a review skill fitted to it.
disable-model-invocation: true
---

# Review Skill Optimizer

Takes an existing review skill and makes it sharper by deeply analyzing the actual codebase. The approach is surgical: keep everything that works, fix what doesn't, add what's missing. The user built that skill with real experience. Don't throw it away.

**Input:** optional path to a review skill's SKILL.md (e.g., `.claude/skills/example-review/SKILL.md`)

If no path given, auto-detect: scan `.claude/skills/*/SKILL.md` for review-related skills (look for "review" in name, description, or content mentioning agents/preflight/findings). If exactly one found, use it. If multiple found, list them and ask which one. If none found, create a new review skill from scratch.

---

## Core Principle: Preserve First, Improve Surgically

The existing skill reflects real project knowledge: conventions the user discovered, false positives they already solved, agent scopes they tuned through experience. Treat it as the baseline, not a rough draft.

**Default behavior:** Keep every section of the existing skill unless the gap analysis gives a concrete reason to change it. When you do change something, the change must be traceable to a specific gap, overlap, or false positive you found.

**What gets preserved:**
- Agent names and their general scope (unless overlaps are found)
- Known-correct pattern whitelists (these are hard-won project knowledge)
- Output format structure (unless it lacks fix readiness fields)
- Existing preflight checks that still work
- Project-specific context sections and conventions documentation
- Anything the user invested effort into getting right

**What gets changed (with justification):**
- Gaps: missing checks the project needs but the skill doesn't cover
- False positives: patterns the skill flags that are actually correct in this project
- Overlaps: two agents checking the same thing (tighten scopes)
- Stale references: file:line examples pointing to code that no longer exists
- Missing preflight: deterministic checks that should exist but don't
- Missing fix readiness: findings without file:line + current code + proposed fix + why

---

## What This Skill Produces

By the end, you deliver back to the user an improved (or new) review skill package:

1. **Improved SKILL.md**: the existing skill with surgical changes: new checks added, false positive whitelists updated, agent scopes tightened, concrete examples refreshed from current codebase
2. **Preflight script**: new or improved deterministic checks (grep/rg + Python) tailored to patterns actually found in the project
3. **Gap report**: what you found and what you changed, so the user can verify you didn't break what was working
4. **Capture script**: `.claude/scripts/capture-finding.sh` if the skill doesn't already have one, so confirmed findings feed the self-learning loop (see `references/skill-scaffold.md` §6)

A fully-featured generated skill also carries a flag surface, a parallel-dispatch skeleton, a growing "Context Awareness (DO NOT flag)" whitelist, `--debt` scoring, `--full` architecture diagrams, self-learning capture, and rule-candidate surfacing. `references/skill-scaffold.md` documents every one of these sections. When improving, use it as a coverage checklist (missing section → `ADD`); when building from scratch, use it as the blueprint. Emit only the modes the project actually needs.

---

## Phase 1: Ingest the Existing Skill

Read the target SKILL.md completely. Extract, catalog, and **tag each section for preservation**:

- **Agents defined**: names, scopes, what each checks
- **Scope overlaps**: where two agents could flag the same thing
- **Coverage gaps**: categories of issues not assigned to any agent
- **Preflight scripts**: what exists, what checks they run, how results feed into agents
- **Output format**: what findings look like, whether they enforce fix readiness (file:line + current code + proposed fix + why)
- **Known-correct patterns**: whitelists, things agents are told to skip
- **Project-specific rules**: anything hardcoded to this project's conventions

For each section, assign a tag:

| Tag | Meaning | Action |
|-|-|-|
| **KEEP** | Works well, no issues found | Preserve verbatim |
| **UPDATE** | Needs extension (append checks) or correction (stale ref, scope overlap, false positive) | Targeted edit, preserve surrounding context |
| **ADD** | Needed but doesn't exist yet | Add a new section |
| **REMOVE** | Covers a pattern the project no longer uses (rare, prefer UPDATE for most cleanup) | Delete the section |
| **MOVE** | Correct content, wrong agent or scope | Relocate to the right place |

The tag vocabulary is shared with `a-rules-optimizer` so both skills' diff summaries read the same way.

Default tag is **KEEP**. Only change what you have evidence to change.

If no existing review skill was found, skip this phase and proceed to Phase 2 to build one from scratch.

---

## Phase 2: Deep Project Analysis

This is the core of the optimizer. You need to understand the project as well as a senior engineer who's worked on it for 6 months. Don't skim: actually read the code.

### Step 1: Project Profile

The manifest checks below cover Python, JavaScript/TypeScript, PHP, Ruby, Go, Rust, and .NET. For Java add `pom.xml` / `build.gradle`, for Elixir `mix.exs`, for Swift `Package.swift`. **Adapt the stack-specific commands below to whatever language you detect**: the shape is the same (list the manifest, grep dependencies), only the filenames change.

```bash
# Language & framework detection
ls composer.json package.json requirements.txt Pipfile pyproject.toml go.mod Cargo.toml Gemfile *.sln pom.xml build.gradle mix.exs Package.swift 2>/dev/null
cat composer.json 2>/dev/null | grep -A5 '"require"' | head -20
cat package.json 2>/dev/null | grep -A10 '"dependencies"' | head -20
cat requirements.txt pyproject.toml 2>/dev/null | head -30

# Project structure
find . -maxdepth 3 -type d -not -path '*/\.*' -not -path '*/node_modules/*' -not -path '*/vendor/*' -not -path '*/__pycache__/*' -not -path '*/.git/*' | sort

# Entry points & architecture indicators
ls index.php main.py app.py manage.py server.* cmd/ src/ lib/ app/ 2>/dev/null
ls Dockerfile docker-compose.yml Makefile .github/workflows/ 2>/dev/null

# Config patterns
ls .env .env.example config/ *.yaml *.yml 2>/dev/null

# Test structure
find . -path '*/test*' -name '*.py' -o -path '*/test*' -name '*.php' -o -path '*/test*' -name '*.ts' -o -path '*/__tests__/*' -name '*.js' 2>/dev/null | head -20

# Size & complexity overview
find src/ app/ lib/ -name '*.py' -o -name '*.php' -o -name '*.ts' -o -name '*.js' 2>/dev/null | xargs wc -l 2>/dev/null | sort -rn | head -20
```

### Step 2: Convention Discovery

Read the 5-10 largest/most important files and document the conventions the project actually uses. You're looking for:

**Architecture patterns:**
- How is code organized? (MVC, service-repository, layered, modular, flat)
- How do modules communicate? (direct imports, events, message passing, DI container)
- Where does business logic live? (services, models, controllers, standalone functions)

**Error handling patterns:**
- How does the project handle failures? (exceptions, result tuples, error codes, Either types)
- Are there project-specific error classes?
- What's the logging strategy? (logger, print, console, Rich)

**State management:**
- Web: session, state store, context, global
- CLI: config objects, env vars, argument parsing
- Framework-specific: st.session_state (Streamlit), request context (Flask), etc.

**Config access:**
- How are settings loaded? (env, YAML, JSON, PHP config, .ini)
- Is there a centralized config manager or is it scattered?
- How are secrets handled?

**Data access patterns:**
- ORM or raw queries?
- Repository pattern or inline queries?
- How are connections managed?

**Registration patterns:**
- How are routes/views/commands registered?
- Are there maps, decorators, or auto-discovery?
- What needs to be exported/registered when adding new code?

**Return value conventions:**
- Do functions return specific shapes? (e.g., `(bool, str)` tuples, Result objects, response dicts)
- Are there project-specific conventions for success/failure?

### Step 3: Pattern Inventory

Programmatically scan for the patterns that a review skill should know about. Read `references/pattern-detection.md` for the full detection script library. Pick the scans relevant to this project's stack.

The goal: build a concrete list of "things that exist in this codebase" so the improved skill can reference real file:line examples.

```bash
# Example: find all exception handling patterns used in the project
rg -n 'except\s+\w+' --type py src/ | awk -F: '{print $3}' | sort | uniq -c | sort -rn

# Example: find all subprocess usage patterns
rg -n 'subprocess\.' --type py src/

# Example: find all route/view registrations
rg -n 'PAGE_MAP|url_patterns|@app.route|router\.' src/

# Example: find all cache patterns
rg -n 'cache|@st.cache|@lru_cache|Redis' src/
```

Adapt these scans to whatever stack you detected. The output feeds into Phase 3.

### Step 4: Find Real Issues (Sampling)

Actually review 3-5 files from the project yourself, applying the existing skill's checklist. Note:
- Issues the existing skill WOULD catch (working correctly)
- Issues the existing skill WOULD MISS (gaps)
- Things the existing skill WOULD falsely flag (false positives due to project conventions)

This sampling gives you concrete evidence for Phase 3.

---

## Phase 3: Gap Analysis

Compare what the existing skill checks against what the project actually needs. Produce a structured gap report.

> **Output framing.** The reports in 3a-3d are **internal**: Phase 3 is your planning artifact. Do not show these raw blocks to the user. The user-facing deliverable is the Phase 5 **Diff Summary**, which traces each change back to one of these Phase 3 findings.

### 3a: Missing Checks

For each issue category, ask: "Does the existing skill cover this for this specific project?"

Read `references/review-dimensions.md` for the full taxonomy of review dimensions. Cross-reference each dimension against the existing skill's agent prompts.

Output:
```
MISSING CHECKS
==============
[SECURITY] No check for pickle.load(): project uses pickle in src/cache/serializer.py
[ARCHITECTURE] No check for view registration: project uses PAGE_MAP in src/web/views/__init__.py
[QUALITY] No check for Python 3.10+ type modernization: project uses Optional[] in 14 files
[PERFORMANCE] No check for N+1: project uses SQLAlchemy with lazy loading
```

### 3b: False Positive Sources

Identify patterns the existing skill flags (or would flag) that are actually correct in this project.

Output:
```
FALSE POSITIVES TO WHITELIST
============================
[ARCHITECTURE] BackupEngine returns (bool, str) tuples: this is the project convention, not a code smell
[QUALITY] Click command functions appear unused: they're invoked by the Click framework via decorators
[QUALITY] Streamlit render functions appear uncalled: they're dispatched via PAGE_MAP string lookup
[SECURITY] .example config files are committed: this is intentional, real configs are gitignored
```

### 3c: Scope Overlaps

Check every pair of agents for items that both could flag. **Then check the review skill against its sibling skills in the same project**, not just its own agents. A project with a `*-perf`, `*-seo` or `*-docs` skill has a second tool claiming some of the same ground, usually with its own preflight and its own noise profile, and neither skill says who wins. The recurring pair is review-Quality against perf on N+1 and dead CSS.

Resolve by consequence, not by topic: the review skill keeps the findings with a correctness consequence (a cron read that OOMs the worker, a cache with no invalidation serving stale data) and the perf skill keeps "this could be faster". Write the boundary into the review skill's scope table, and do **not** edit the sibling skill: the same `*-perf` skill may exist in other projects where it is the only owner, so a one-sided note in the skill you were asked to improve is the safe form.

Output:
```
SCOPE OVERLAPS
==============
[Security × Architecture] Both check exception handling: Security checks "secrets in error messages", Architecture checks "bare except". These OVERLAP on: except blocks that log sensitive data. Fix: Security owns "what's in the error message", Architecture owns "how the exception is structured"
[Architecture × Quality] Both could flag "missing registration": Architecture checks view registration, Quality checks dead code. Fix: Architecture owns registration, Quality skips functions that appear in PAGE_MAP/route decorators
```

### 3d: Preflight Gaps

What deterministic checks should exist but don't? Compare the project's patterns against `references/pattern-detection.md`:

Output:
```
MISSING PREFLIGHT CHECKS
=========================
[NEED] subprocess calls without timeout: project has 8 subprocess calls, none checked by preflight
[NEED] st.rerun() without preceding invalidate(): project pattern requires cache invalidation before rerun
[NEED] View files not registered in PAGE_MAP: project uses explicit registration
[HAVE] Bare except check: already covered, working
[SKIP] innerHTML check: no HTML rendering in this project
```

---

## Phase 4: Generate Improved Skill

Apply targeted improvements based on the gap analysis. Follow the preservation tags from Phase 1. Default is KEEP: only touch what has a concrete reason to change.

### 4a: Enhance Agent Prompts

For each agent, check its preservation tag. If it's KEEP, don't touch it. If UPDATE, apply these additions/edits while keeping the existing prompt structure intact:

**Add if missing: YOUR SCOPE**: explicit, non-overlapping list. Use the overlap resolution from Phase 3c.

**KNOWN CORRECT PATTERNS (DO NOT FLAG)**: from Phase 3b. Include actual file:line references:
```
These patterns are correct in this project. Do not flag them:
- BackupEngine methods return (bool, str) tuples: see src/core/backup_engine.py:45
- @st.cache_resource used for heavy component init: see src/web/state.py:12
- Storage paths dict uses keys 'local' and 'sync': see src/core/config_manager.py:88
```

**CONCRETE EXAMPLES TO FIND**: from Phase 2 Step 4. Show agents what real issues look like in THIS codebase:
```
Look for patterns like these (found during analysis):
- src/cli.py:622: subprocess.run() without timeout (similar calls may exist elsewhere)
- src/web/views/text_sanitizer.py:45: st.rerun() without invalidate() in preceding lines
```

**PREFLIGHT KNOWN ISSUES**: placeholder for runtime injection from the preflight script:
```
The preflight scan found these issues in your scope. Verify each one:
${PREFLIGHT_SECURITY_FINDINGS}
For each: confirm it's real, add context about why it matters here, and look for similar patterns nearby.
```

**OUTPUT CONSTRAINT**: every finding must include all 5 fields:
```
Every finding MUST include:
1. File:Line: exact location
2. Current code: the line(s) as they exist
3. Proposed fix: what it should look like
4. Why: one sentence, specific to this project (not generic)
5. Intent ruled out: what you checked to confirm this is not deliberate. A comment on
   the thing itself, a sibling doing the same, a rule or allowlist naming it. If the
   current behaviour has a plausible reason to exist, state it and why it still fails.
   If you cannot find the reason, mark the finding intent-unverified rather than
   asserting a defect.
Findings missing any field are incomplete. Do not include them.
```

Field 5 is the one that stops inverted fixes: a correct claim about how something works does not establish that it is wrong. Ask what would break if the fix landed and the behaviour turned out to be intentional. The whitelists this skill maintains are the reactive half of the same problem, paid one false positive at a time; field 5 is the preventive half.

### 4b: Generate Preflight Script

Based on the preflight gaps from Phase 3d, write a bash script (with embedded Python for multi-line detection) that:

1. Runs checks specific to this project's stack and patterns
2. Outputs structured JSON with check IDs, status, file:line locations, and messages
3. Routes results to the correct agent by check ID prefix

Follow the pattern in `references/preflight-template.md` for the script structure.

Every check must:
- Have a unique ID (e.g., SEC-01, SUB-01, WEB-01)
- Map to exactly one agent
- Output file:line locations (not just counts)
- Handle the "not found" case gracefully (don't error on clean code)

For multi-line pattern detection (the #1 source of false negatives in grep-based scripts), use inline Python:
- Read full expressions from `(` to matching `)`
- Scan backward/forward N lines for required companion patterns
- Parse indent-based blocks for Python

### 4c: Add Reconciliation Rules

Add a post-review section that ensures nothing falls through cracks:

```
## Post-Flight Reconciliation

After all agents return:

1. DEDUP: Same file:line from multiple agents → keep the one from the owning agent
2. PREFLIGHT CHECK: Every preflight finding must appear in an agent report as either:
   - Confirmed (with expanded context and fix)
   - Dismissed (with specific reason why it's a false positive)
   If a preflight finding is missing from all reports, it was dropped. Flag it.
3. COMPLETENESS: Reject findings missing file:line, current code, proposed fix, or why.
4. SEVERITY GATE: Apply severity levels based on project-specific impact, not generic rules.
```

### 4d: Apply Changes Using Preservation Tags

Go through the existing skill section by section, applying the tags from Phase 1:

**KEEP sections:** Copy verbatim. Don't rephrase, don't "improve" wording, don't reorganize. If it works, leave it alone. The user will notice if their carefully worded whitelist entry got rephrased into something subtly different.

**UPDATE sections, append mode:** Keep the existing content as-is, then append new items below a clear marker:
```markdown
## Security Agent
[... existing checks preserved exactly ...]

### Added by a-review-optimizer [DATE]
- [NEW] Check for pickle.load(): found in src/cache/serializer.py:22
- [NEW] Check for yaml.load without SafeLoader: found in src/config/loader.py:8
```

**UPDATE sections, correction mode:** Make the minimal targeted edit. Show a before/after in the diff summary so the user can verify:
```
[UPDATE] Security agent scope: removed "exception handling" (was overlapping with Architecture)
  Before: "Check for injection, path traversal, credentials, exception handling"
  After:  "Check for injection, path traversal, credentials"
```

**ADD sections:** Add as new sections at the end of the relevant part of the skill, clearly marked:
```markdown
### [NEW] Preflight Automated Scan
Added by a-review-optimizer: this section did not exist in the original skill.
[... new content ...]
```

**REMOVE / MOVE sections:** Rare for review skills. Only remove content when a whole check targets a pattern the project provably no longer uses (confirmed via grep of current code). Only move when content is correct but clearly belongs under a different agent or phase. In both cases, note the rationale in the Phase 5 diff summary so the user can verify.

**What you must NEVER do:**
- Rewrite agent prompts from scratch when they just need a few additions
- Remove whitelist entries without confirming they're stale (check if the referenced file/pattern still exists)
- Change agent names or restructure the skill's overall flow
- Replace working preflight scripts with new ones (add checks to existing, or create a new script alongside)
- Remove existing check items just because they're not in your gap analysis (they might catch things you haven't seen yet)
- Rephrase project-specific conventions documentation (the user wrote it in their own words for a reason)

### 4e: Wire Modes, Self-Learning, and Rule Candidates

Beyond the agent prompts and preflight, a mature skill carries a set of structural sections that make it as capable as a hand-tuned one. Read `references/skill-scaffold.md` and, for each section, check whether the target skill has it. Missing sections are `ADD`; thin ones are `UPDATE`. Preserve any the user already tuned. This step only ever adds.

- **Flag surface** (§1): `--changed` default, plus `--full` / `--security-only` / `--debt` / `--all` where the project warrants them.
- **Execution / dispatch** (§2): dimensions dispatched through a mechanism that actually returns each agent's report (in Claude Code that is the Workflow tool; a bare batch of background Agent calls is the superseded form, because those agents go idle without delivering anything back and the review silently reports nothing), an Owns / Does-NOT-check scope table (this is where Phase 3c overlap resolutions get written down), preflight findings split by check-ID prefix into each agent's `PREFLIGHT KNOWN ISSUES` block, and an adversarial verify stage on every finding.
- **Context Awareness (DO NOT flag)** (§3): a centralized whitelist that GROWS: every preflight finding the review *dismisses* as a false positive gets appended here under a dated `### Added by a-review-optimizer [DATE]` marker with a real file:line referent. This is the same append discipline as 4d; do it for the whitelist, not just the agent prompts.
- **`--debt` mode** (§4): points-per-severity scoring (Critical 8 / High 4 / Medium 2 / Low 1, cap 100), a per-agent breakdown table, tightened thresholds, and an Auto-Fixable safe-vs-needs-confirmation split.
- **`--full` mode** (§5): component + layer Mermaid diagrams (scan the stack's import mechanism, cross-layer edges only) and a weighted Architecture Health Score. Skip for projects with no real layered structure.
- **Self-learning capture loop** (§6): emit `.claude/scripts/capture-finding.sh`, a `Capture Findings for Self-Learning (MANDATORY)` section (one RUN_ID per run, one call per confirmed finding), and a seeded stable category-slug table. This wires the skill into `a-self-learner`.
- **Rule candidates** (§7): surface any pattern hit in 3+ locations as a proposed rule for `.claude/rules/`, asking before adding.
- **Stack detection** (§8): if the project ships more than one stack (and most do: a server language plus browser JS plus SQL migrations plus CLI tooling), move stack specifics into `references/stack-*.md` gated on the changed paths. Check whether the target skill's briefs assume one language; if they do, the minority stacks are being reviewed with the wrong checklist, and that is a real gap rather than a tidiness issue.

Only emit modes the project needs: don't bolt `--debt` query scoring onto a static site or draw architecture diagrams for a 3-file script. But if the target skill is missing the self-learning loop, the whitelist-growth discipline, or the flag surface, those are real capability gaps: add them.

---

## Phase 5: Deliver

Present to the user:

1. **Gap Report**: summarize what you found (missing checks, false positives, overlaps, preflight gaps). Keep it concise: this is context, not the deliverable.

2. **Improved SKILL.md**: the existing skill with targeted changes applied, ready to drop in. The user should be able to diff this against the original and see exactly what changed. Say where it goes: a review skill activates as `/<name>` only when its `SKILL.md` sits at `.claude/skills/<name>/SKILL.md` (and its preflight script at `.claude/scripts/`), so name that write target explicitly instead of handing the user a file with no destination.

3. **Preflight Script**: if the project needs one (most do), the generated script.

4. **Diff Summary**: this is the external deliverable (contrast with the internal Phase 3 gap report). What changed, what stayed, and why. Every change must be traceable to a Phase 3 finding:
   ```
   CHANGES FROM ORIGINAL
   =====================
   [KEEP] Severity gating: no issues found, preserved as-is
   [KEEP] Known-correct patterns: all references still valid
   [KEEP] Output format structure: already had fix readiness columns
   [UPDATE] Security agent prompt: appended 2 checks (pickle.load, yaml.load) found in codebase
   [UPDATE] Preflight script: appended 4 new checks (SUB-01, WEB-01, TYPE-01, TYPE-02)
   [UPDATE] Agent scope overlap: Security no longer checks exception handling (Architecture owns it)
   [UPDATE] Stale reference: updated src/web/views/old_view.py:33 → src/web/views/dashboard.py:45
   [ADD] Fix Readiness Requirement: findings now require 5 fields
   [ADD] Post-flight reconciliation: preflight findings verified against agent output
   ```

5. **Verification checklist**: specific things the user should test to confirm the improved skill works:
   ```
   RUN THESE TO VERIFY
   ====================
   1. bash .claude/scripts/preflight-comprehensive.sh | python3 -m json.tool  (valid JSON?)
   2. /example-review --full  (catches known issues X, Y, Z from our analysis?)
   3. Check: zero duplicate findings across agents
   4. Check: every finding has file:line + current code + fix + why + intent ruled out
   ```

---

## When No Review Skill Exists

Building from scratch is fundamentally different from improving an existing skill: plan on **2-3× the time** and expect fewer project-specific insights unless Phase 2 is done deeply. Don't rush Phase 2.

The from-scratch workflow:

1. **Phase 1: SKIP**: nothing to ingest, no preservation tags to apply.
2. **Phase 2: FULL**: this is now your primary input. Read the code carefully; conventions you miss here become gaps you won't catch.
3. **Phase 3: SKIP gap-against-existing**: there's no baseline. Instead, treat `references/review-dimensions.md` as the complete checklist and decide which dimensions this project actually needs (a CLI-only Python tool doesn't need a "Web Patterns" agent; a static site doesn't need SSRF checks).
4. **Phase 4: Build from zero, not from a generic template.**
   - Decide how many agents make sense (2-4, based on project complexity, don't over-fragment).
   - Assign scopes based on Phase 2 findings + the dimensions you kept from Phase 3. Scopes must be non-overlapping.
   - Generate agent prompts with **concrete file:line examples from the codebase you actually read**: no generic "look for bare except" lines. Every check needs a real referent.
   - Generate a preflight script using only the checks that detect patterns the project actually uses.
   - Use the output format template from `references/output-template.md`.
   - Build out the structural sections from `references/skill-scaffold.md`: the flag surface, the dispatch mechanism and its scope table, a starter Context Awareness whitelist (from your Phase 2/3b false positives), the `--debt` / `--full` modes the project warrants, the self-learning capture loop (emit `capture-finding.sh` + the seeded slug table), and rule-candidate surfacing. A skill missing these reads generic. A mature one has all of them.
5. **Phase 5:** deliver as usual; the Diff Summary just reads "built from scratch, no prior version."

The result should be what an experienced engineer would write after working on the project for months, not a generic checklist with the project name swapped in. If the output reads generic, go back to Phase 2.

---

## Capability Checklist

Before delivering (improve or from-scratch), confirm the skill can produce each of these. A gap here is a `MISSING CHECK` for Phase 3; close it (stack-appropriately) unless the project genuinely doesn't need it. Don't remove a capability the user already has.

- [ ] **3+ non-overlapping dimensions, dispatched so the reports come back**: each with an explicit YOUR SCOPE / Owns list and a Does-NOT-check column; overlaps resolved (Phase 3c, `skill-scaffold.md` §2). A skill that fires a batch of background agents and never collects them is a `MISSING CHECK`, not a style preference: that form silently returns nothing.
- [ ] **Adversarial verify stage**: every finding refuted-by-default by a second agent that traces the chain from source; the report states what was refuted alongside what was confirmed (`skill-scaffold.md` §2).
- [ ] **Deterministic preflight**: unique check IDs each mapped to one agent, JSON with file:line (not just counts), graceful clean handling, inline-Python for multi-line patterns (`preflight-template.md`).
- [ ] **Hostile-content preflight (INJ-*)**: invisible/bidi Unicode and AI-directed instruction phrases scanned deterministically in every generated preflight, whatever the stack. This is reviewer self-defense: the payload's target is the agent reading the file, so the check can never live in an agent prompt (`pattern-detection.md` > Hostile Content Detection).
- [ ] **Silent-failure coverage per stack**: empty or log-only catch detection for every language the project ships (not just the majority one), suppression-operator scans, fail-open return-default detection, and the shell-script checks (SH-*) when the project carries `.sh` files (`pattern-detection.md` > Error Handling, Shell Script Detection).
- [ ] **LLM dimension when applicable**: if the project calls an LLM API, the skill has a prompt-injection / output-handling agent scope and LLM-* preflight checks; if not, it has none (`review-dimensions.md` > LLM Integration Dimensions).
- [ ] **Fix readiness**: every finding carries file:line + current code + proposed fix + why + intent-ruled-out + an impact field; incomplete findings rejected (`output-template.md`).
- [ ] **Impact field split by kind**: `failure_scenario` for correctness, counted `cost` for maintainability. A skill requiring `failure_scenario` on *every* finding is a `MISSING CHECK`, not a style preference: duplication and dead-code findings cannot produce one, so they get invented or dropped at the gate (`output-template.md` > Fix Readiness Rules).
- [ ] **Skipped table**: a confirmed finding deliberately not acted on is reported with a reason and a revisit-when, so a skip is distinguishable from an oversight (`output-template.md`).
- [ ] **Stack detection**: stack specifics live in `references/stack-*.md` and load only when the diff touches that stack; `SKILL.md` stays stack-neutral (`skill-scaffold.md` §8). A skill applying one language's lens to every file is a `MISSING CHECK` for every other stack it ships.
- [ ] **Severity gating**: BLOCK/WARN/INFO tied to project-specific impact (`output-template.md`).
- [ ] **`--debt` mode**: points-per-severity score, per-agent breakdown table, Auto-Fixable safe-vs-needs-confirmation split (`skill-scaffold.md` §4).
- [ ] **`--full` mode**: component + layer Mermaid diagrams and an Architecture Health Score (`skill-scaffold.md` §5).
- [ ] **Flag surface**: `--changed` (default), `--full`, `--security-only`, `--debt`, `--all` (`skill-scaffold.md` §1).
- [ ] **Post-flight reconciliation**: dedup by owning agent, every preflight finding accounted (confirmed/dismissed), completeness reject, severity gate (Phase 4c).
- [ ] **Self-learning loop**: emits `capture-finding.sh`, a mandatory capture section with one RUN_ID per run, and a stable kebab-case slug table feeding `a-self-learner` (`skill-scaffold.md` §6).
- [ ] **Rule candidates**: recurring pattern (3+ locations) → propose a rule for `.claude/rules/`, ask before adding (`skill-scaffold.md` §7).
- [ ] **Context Awareness whitelist that grows**: a centralized DO-NOT-flag section; dismissed false positives appended each run under a dated marker (`skill-scaffold.md` §3).

---

## Reference Files

Read these as needed during analysis:

| File | When to Read | Contains |
|-|-|-|
| `references/review-dimensions.md` | Phase 3a (finding missing checks) | Complete taxonomy of review categories across all stacks |
| `references/pattern-detection.md` | Phase 2 Step 3 and Phase 4b | Library of grep/rg/Python detection scripts per language |
| `references/preflight-template.md` | Phase 4b (generating preflight script) | Bash+Python script template with JSON output |
| `references/output-template.md` | Phase 4 (when building from scratch) | Standard output format with fix readiness columns |
| `references/skill-scaffold.md` | Phase 4d and from-scratch build | Structural sections outside the agents: flags, dispatch + adversarial verify, growing whitelist, `--debt`/`--full` modes, self-learning capture loop, rule candidates, stack detection + per-stack references (§8) |
| `references/_shared.md` | Before editing `review-dimensions.md` or `pattern-detection.md` | Those two files are deliberately duplicated into `a-rules-optimizer/references/` so each skill stays self-contained. Any edit to either must update both copies in the same commit; `_shared.md` carries the parity check |

interface:
  display_name: "Review Optimizer"
  short_description: "Write a review skill fitted to this codebase"
policy:
  allow_implicit_invocation: false

# Shared Reference Files

The following files in this directory are **duplicated** into `a-rules-optimizer/references/`:

- `review-dimensions.md`: taxonomy of review categories
- `pattern-detection.md`: detection script library by language

Both `a-review-optimizer` and `a-rules-optimizer` read these. Each skill keeps its own copy so it stays self-contained and can be installed on its own.

## When editing one, update the other

If you change `review-dimensions.md` or `pattern-detection.md` here, **update the copy in `a-rules-optimizer/references/` in the same commit**. The two copies must stay identical.

Quick parity check, run from this skill's directory:

```bash
diff -q references/review-dimensions.md ../a-rules-optimizer/references/review-dimensions.md
diff -q references/pattern-detection.md ../a-rules-optimizer/references/pattern-detection.md
```

Both should print nothing. If they diverge, pick the authoritative version and sync.

# Review Output Format Template

Use this when building a new review skill from scratch (Phase 4, no existing skill). The format enforces fix readiness on every finding.

## Report Template

```markdown
# Code Review Report
**Project:** [project name]
**Stack:** [languages + frameworks]
**Target:** [X files: scope description]
**Date:** [timestamp]
**Preflight:** [X checks run, Y findings fed to agents]

## Summary
| Severity | Count |
|-|-|
| Critical | X |
| High | X |
| Medium | X |
| Low | X |

## Critical Issues (Fix Before Merge)
| # | Agent | File:Line | Current Code | Proposed Fix | Why | Intent Ruled Out |
|-|-|-|-|-|-|-|
| 1 | Security | src/auth.py:44 | admin credential hardcoded as a string literal | value read from an environment variable instead | Hardcoded credential accessible to anyone with repo access | No test-fixture marker, no allowlist entry, and the two sibling auth paths both read from env |

## High Priority
| # | Agent | File:Line | Current Code | Proposed Fix | Why | Intent Ruled Out |
|-|-|-|-|-|-|-|

## Medium Priority
| # | Agent | File:Line | Current Code | Proposed Fix | Why | Intent Ruled Out |
|-|-|-|-|-|-|-|

## Low Priority
[Summary count by category: individual items not listed]
- Type modernization: X items
- Naming: X items
- Style: X items

## Skipped (real, not acted on)
| Finding | Why skipped | Revisit when |
|-|-|-|
| 6-view page shell duplicated | Fix reaches 3 files outside the diff | Next time one of those views changes |

*A confirmed finding deliberately not fixed goes here, not into silence. Legitimate reasons: the fix changes intended behaviour, it needs a decision that is the user's, or it reaches well outside the reviewed scope. Without this table a deliberate skip and an oversight look identical, and the next run rediscovers it cold.*

## Preflight Reconciliation
| Check ID | Status | Agent | Verdict |
|-|-|-|-|
| SEC-01 | fail | Security | Confirmed: see finding #1 |
| SUB-01 | warn | Architecture | Dismissed: timeout not needed (local-only script) |
| WEB-01 | warn | Architecture | Confirmed: see finding #4 |

## Auto-Fixable Issues
**Safe (no logic changes):**
- src/utils.py:1-3: remove unused imports (os, sys, re)
- src/models.py:22: `Optional[str]` → `str | None`
- src/views.py:8: `List[dict]` → `list[dict]`

**Needs confirmation:**
- src/backup.py:140-180: duplicate sync block, extract to helper? (logic might differ subtly)

## Recommended Fix Order
1. [Critical #1] Fix hardcoded credential: security exposure
2. [High #3] Add timeout to subprocess.run: blocks indefinitely on large DBs
3. [Auto-fix batch] Run safe auto-fixes (imports, types)
4. [Medium items] Address in next sprint

## Tech Debt Score (if requested)
**Score: X/100**
| Category | Points |
|-|-|
| Security | X |
| Architecture | X |
| Quality | X |
| Performance | X |

Breakdown: 0 = pristine, 25 = healthy, 50 = needs attention, 75+ = stop and fix
```

## Fix Readiness Rules

Every finding at Critical, High, or Medium severity MUST include all 6 columns:

| Column | Required | What it contains |
|-|-|-|
| File:Line | Always | Exact location, e.g., `src/backup.py:88` |
| Current Code | Always | The actual code as it exists (verbatim, can be truncated with `...`) |
| Proposed Fix | Always | What the code should look like after fixing |
| Why | Always | One sentence, specific to this project, explaining impact |
| Intent Ruled Out | Always | What was checked to confirm the behaviour is not deliberate, or `intent-unverified` |
| Impact | Always | `failure_scenario` if the finding is correctness, `cost` if it is maintainability. See below |

**Pick the impact field by kind, and never force the wrong one.**

- **correctness** → `failure_scenario`: concrete inputs or state producing a specific wrong outcome.
- **maintainability** (duplication, dead code, reuse, altitude) → `cost`: something counted. Copies removed, queries or bytes saved per request, files that must now change together. "Harder to maintain" is not a cost; "the same narrowing is written 11 times and 3 already disagree" is.

Requiring a `failure_scenario` on every finding is the trap this replaces. Nothing goes wrong when a helper exists 11 times, so a single mandatory failure field makes real cleanup findings unfileable: the agent either invents a scenario or drops the finding at the completeness gate. In practice a review skill carrying a 12-bullet reuse checklist still reported none of an 11-copy drift, a 3x duplicated SELECT list, or a 6-view duplicated page shell, because none of them could produce a failure scenario to pass the gate.

**Incomplete findings are rejected.** If an agent can't fill the fields, it either needs to investigate more or the finding isn't actionable. Reject on the wrong impact field and you delete the cleanup half of the review.

Low-priority findings are summarized by category (no individual table entries) to keep the report scannable.

## Severity Definitions

| Level | Label | Criteria | Action |
|-|-|-|-|
| BLOCK | Critical | Exploitable security vuln, data loss risk, broken core functionality | Fix before merge |
| WARN | High | Missing error handling on critical path, N+1 on hot path, broken contract, auth gap | Fix this sprint |
| WARN | Medium | Dead code, missing types on public API, duplication, modernization with clear benefit | Fix when touching file |
| INFO | Low | Style, naming, minor modernization | Optional cleanup |

## Agent Output Constraint

Include this in every agent prompt to enforce the format:

```
OUTPUT RULES:
- Final response under 3000 characters
- List findings, not your reasoning process
- Every finding: File:Line | Current Code | Proposed Fix | Why | Intent Ruled Out | Impact
- Impact is failure_scenario (correctness) or cost (maintainability), never both, never neither.
  Do NOT invent a failure_scenario for a duplication finding so it passes the gate; count the cost
- If you can't fill the fields, investigate more or drop the finding
- Intent Ruled Out: name the evidence that this is not deliberate (an adjacent comment, a
  sibling doing the same, a rule or allowlist). If you cannot find it, say intent-unverified
  instead of asserting a defect
- Group by severity: Critical first, then High, Medium
- Do not list Low-severity items individually: summarize count by category
- End with: "FINDINGS: X critical, X high, X medium, X low"
```

# Pattern Detection Script Library

Use this during the pattern-inventory phase and when generating preflight scripts. Pick the scripts relevant to the project's stack.

All scripts are designed to be safe (|| true suffix, head limits on output) and produce file:line locations.

## Universal Detection Scripts

### Credentials & Secrets
```bash
# Hardcoded secret assignments (exclude env/config accessors)
rg -n '(password|api_key|secret_key|api_secret|token|private_key)\s*=\s*["\x27][^"\x27]{4,}' \
  --type-add 'code:*.{py,php,js,ts,rb,go,java}' -t code src/ \
  | grep -v 'getenv\|os.environ\|get_setting\|\.get(' || true

# Files that shouldn't be tracked
git ls-files -- '*.env' '.env.*' '*.key' '*.pem' 2>/dev/null \
  | grep -v '.example' | grep -v '.sample' || true
```

### Error Handling
```bash
# Bare except (Python)
rg -n '^\s*except\s*:' --type py src/ || true

# catch(Exception) or catch(\Exception) (PHP)
rg -n 'catch\s*\(\s*\\?Exception' --type php src/ || true

# Generic catch(e) (JS/TS)
rg -n 'catch\s*\(\s*\w+\s*\)\s*\{' --type js --type ts src/ || true

# Silent exception handlers (Python: except block with only pass/continue)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text().splitlines()
    for i, line in enumerate(lines):
        if re.match(r'\s*except\b', line):
            # Check next non-blank lines in the except block
            body_lines = []
            base_indent = len(line) - len(line.lstrip())
            for j in range(i+1, min(i+5, len(lines))):
                stripped = lines[j].strip()
                indent = len(lines[j]) - len(lines[j].lstrip())
                if indent <= base_indent and stripped: break
                if stripped: body_lines.append(stripped)
            if body_lines and all(b in ('pass', 'continue', '...') for b in body_lines):
                print(f'{f}:{i+1}: silent except, body is only {body_lines[0]}')
" 2>/dev/null || true

# Empty catch blocks (PHP: brace-balanced scan, catches multi-line)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.php'):
    text = f.read_text(errors='ignore')
    for m in re.finditer(r'catch\s*\([^)]*\)\s*\{', text):
        depth, pos = 1, m.end()
        while pos < len(text) and depth > 0:
            if text[pos] == '{': depth += 1
            elif text[pos] == '}': depth -= 1
            pos += 1
        body = text[m.end():pos-1].strip()
        line = text[:m.start()].count('\n') + 1
        if not body or re.fullmatch(r'(//[^\n]*|\s)*', body):
            print(f'{f}:{line}: empty catch block, error discarded')
" 2>/dev/null || true

# Empty catch blocks (JS/TS: brace-balanced scan) + log-only promise .catch
python3 -c "
import re, pathlib
for ext in ('*.js', '*.ts', '*.jsx', '*.tsx'):
    for f in pathlib.Path('src').rglob(ext):
        text = f.read_text(errors='ignore')
        for m in re.finditer(r'catch\s*(\([^)]*\))?\s*\{', text):
            depth, pos = 1, m.end()
            while pos < len(text) and depth > 0:
                if text[pos] == '{': depth += 1
                elif text[pos] == '}': depth -= 1
                pos += 1
            body = text[m.end():pos-1].strip()
            line = text[:m.start()].count('\n') + 1
            if not body or re.fullmatch(r'(//[^\n]*|\s)*', body):
                print(f'{f}:{line}: empty catch block, error discarded')
" 2>/dev/null || true
rg -n '\.catch\s*\(\s*(\(\s*\)|\(?\w*\)?)\s*=>\s*\{?\s*\}?\s*\)|\.catch\s*\(\s*console\.(log|error)\s*\)' --type js --type ts src/ || true

# Error suppression operators (PHP)
rg -n '@\s*(file_get_contents|file_put_contents|unlink|fopen|mkdir|rmdir|copy|rename|include|require|mysqli_|json_decode|simplexml_|\$)' --type php src/ || true
rg -n 'error_reporting\s*\(\s*0\s*\)' --type php src/ || true

# Fail-open except (Python): error branch returns a success-looking default with no raise/log
python3 -c "
import re, pathlib
DEFAULTS = re.compile(r'return\s+(None|True|False|\[\]|\{\}|0|[\"\x27][\"\x27])\s*(#.*)?$')
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text(errors='ignore').splitlines()
    for i, line in enumerate(lines):
        if not re.match(r'\s*except\b', line): continue
        base = len(line) - len(line.lstrip())
        body = []
        for j in range(i+1, min(i+8, len(lines))):
            s = lines[j].strip()
            ind = len(lines[j]) - len(lines[j].lstrip())
            if s and ind <= base: break
            if s: body.append(s)
        if not body: continue
        has_default_return = any(DEFAULTS.match(b) for b in body)
        has_signal = any(re.search(r'raise|log|warn|print', b) for b in body)
        if has_default_return and not has_signal:
            print(f'{f}:{i+1}: fail-open except, returns default without raising or logging')
" 2>/dev/null || true
```

### Code Metrics
```bash
# Files over 300 lines
find src/ app/ lib/ -name '*.py' -o -name '*.php' -o -name '*.ts' -o -name '*.js' 2>/dev/null \
  | xargs wc -l 2>/dev/null | awk '$1 > 300 && !/total$/' | sort -rn || true

# Deep nesting (5+ levels = 20+ leading spaces)
rg -n '^\s{20,}\S' --type-add 'code:*.{py,php,js,ts}' -t code src/ | head -20 || true

# Long functions (Python: def to next def at same/lower indent, >50 lines)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text().splitlines()
    func_start = None
    func_name = ''
    func_indent = 0
    for i, line in enumerate(lines):
        m = re.match(r'^(\s*)def\s+(\w+)', line)
        if m:
            if func_start and (i - func_start) > 50:
                print(f'{f}:{func_start+1}: {func_name}() is {i - func_start} lines')
            func_start = i
            func_name = m.group(2)
            func_indent = len(m.group(1))
    if func_start and (len(lines) - func_start) > 50:
        print(f'{f}:{func_start+1}: {func_name}() is {len(lines) - func_start} lines')
" 2>/dev/null || true

# print/var_dump/console.log in production code
rg -n '\bprint\(' --type py src/ --glob '!*test*' --glob '!*__pycache__*' 2>/dev/null | head -20 || true
rg -n '\bvar_dump\(|\bdd\(' --type php src/ 2>/dev/null | head -20 || true
rg -n '\bconsole\.(log|debug)\(' --type js --type ts src/ --glob '!*test*' 2>/dev/null | head -20 || true

# Commented-out code (3+ consecutive comment lines with code patterns)
python3 -c "
import re, pathlib
code_pattern = re.compile(r'#\s*(def |class |import |return |if |for |while |print|self\.|=\s)')
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text().splitlines()
    streak = 0
    streak_start = 0
    for i, line in enumerate(lines):
        if code_pattern.match(line.strip()):
            if streak == 0: streak_start = i
            streak += 1
        else:
            if streak >= 3:
                print(f'{f}:{streak_start+1}: {streak} lines of commented-out code')
            streak = 0
    if streak >= 3:
        print(f'{f}:{streak_start+1}: {streak} lines of commented-out code')
" 2>/dev/null || true
```

## Shell Script Detection

Silent failure in bash is the fleet's most common script defect: the happy path works, the unhappy path exits 0. Distinguish state-changing commands (rm, mv, cp, rsync, docker, systemctl) from read-only probes; `|| true` on a probe is deliberate, on a mutation it masks real failures.

```bash
# Missing safety flags in scripts that change state (>20 lines as a proxy)
for f in $(find . -name '*.sh' -not -path '*/node_modules/*' -not -path '*/vendor/*' -not -path '*/.git/*' 2>/dev/null); do
  [ "$(wc -l < "$f")" -lt 20 ] && continue
  head -10 "$f" | grep -q 'set -e' || echo "$f:1: no set -e"
  grep -q 'pipefail' "$f" || echo "$f:1: no pipefail, failures inside pipelines are invisible"
done

# || true or || : on state-changing commands
rg -n '\b(rm|mv|cp|mkdir|rsync|scp|docker|systemctl|crontab|chown|chmod)\b[^|#]*\|\|\s*(true|:)' --glob '*.sh' . || true

# Blanket stderr suppression on state-changing commands
rg -n '\b(rm|mv|cp|rsync|scp|docker|systemctl)\b[^#]*2>\s*/dev/null' --glob '*.sh' . || true

# cd without failure guard (subsequent commands run in the wrong directory)
rg -n '^\s*cd\s+[^&|;]+$' --glob '*.sh' . || true

# mktemp without an EXIT trap in the same file
for f in $(rg -l 'mktemp' --glob '*.sh' . 2>/dev/null); do
  grep -q 'trap.*EXIT' "$f" || echo "$f: mktemp without EXIT trap cleanup"
done

# Opportunistic: real linter when installed (catches quoting, word splitting, and much more)
command -v shellcheck >/dev/null 2>&1 && shellcheck -f gcc -S warning $(find . -name '*.sh' -not -path '*/.git/*') 2>/dev/null | head -30 || true
```

## Hostile Content Detection (reviewer integrity)

MANDATORY in every generated preflight, regardless of stack. These checks defend the AI reviewer itself: a prompt-injection payload in a comment can hijack the agent reading the file, so detection MUST be deterministic. A regex cannot be sweet-talked; the agent can. Never move these checks into an agent prompt.

```bash
# INJ-01: Invisible / bidirectional Unicode (Trojan Source, CVE-2021-42574)
python3 -c "
import pathlib
BAD = {0x200B, 0x200C, 0x200D, 0x200E, 0x200F, 0x2060, 0xFEFF} | set(range(0x202A, 0x202F)) | set(range(0x2066, 0x206A))
EXTS = {'.py', '.php', '.js', '.ts', '.jsx', '.tsx', '.sh', '.md', '.yml', '.yaml', '.json', '.html', '.css', '.sql', '.env', '.txt'}
SKIP = {'.git', 'node_modules', 'vendor', '__pycache__', 'dist', 'build'}
for f in pathlib.Path('.').rglob('*'):
    if f.is_dir() or set(f.parts) & SKIP or f.suffix not in EXTS: continue
    try: text = f.read_text(encoding='utf-8')
    except Exception: continue
    for i, line in enumerate(text.splitlines(), 1):
        hits = sorted({hex(ord(c)) for c in line if ord(c) in BAD})
        if hits:
            print(f'{f}:{i}: invisible/bidi characters {hits}')
" 2>/dev/null || true

# INJ-02: AI-directed instruction phrases in comments/strings/docs
rg -ni 'ignore (all |any )?(previous|prior|above|earlier) (instructions|prompts|rules)|disregard (the |your )?(system|previous|above)|you are now (a|an|in)|new (system )?instructions:|do not (flag|report|mention|include) (this|the following)|(assistant|claude|copilot|gpt|reviewer)[,:]? (please )?(approve|ignore|skip|omit)' \
  --glob '!*.lock' --glob '!node_modules/**' --glob '!vendor/**' . | head -20 || true

# INJ-03: Large base64 blobs in comments (hidden payloads)
rg -n '(#|//|/\*|<!--|;)\s*[A-Za-z0-9+/]{120,}={0,2}' --glob '!*.lock' --glob '!*.min.*' --glob '!*.svg' . | head -10 || true
```

Whitelist note: security tooling, test fixtures, and this file itself legitimately contain INJ-02 phrases. Findings inside `*test*`, `*fixture*`, or the review skill's own tree get dismissed with a reason, not silently skipped.

## LLM Integration Detection (conditional)

Only include these when the gate check finds LLM usage. The greps surface candidate sites; whether the interpolated content is actually untrusted is the agent's judgment call.

```bash
# Gate: does the project call an LLM at all? (empty output = skip the LLM-* group entirely)
rg -l 'import anthropic|from anthropic|import openai|from openai|import ollama|messages\.create|chat\.completions|api\.anthropic\.com|api\.openai\.com' src/ || true

# LLM-01: Prompt-building sites with interpolation (candidate injection points)
rg -n '(prompt|messages|system_prompt|user_content|content)\s*[=:+].{0,50}(f["\x27]|\.format\(|%s|\$\{|\+\s*\w)' --type py --type js --type ts src/ | head -20 || true

# LLM-02: LLM output rendered or executed
rg -n '(response|completion|\.content|message\.content|\.text|output)\w*.{0,60}(innerHTML|dangerouslySetInnerHTML|v-html|st\.markdown|st\.html|eval\(|exec\(|subprocess|os\.system|shell)' src/ | head -20 || true

# LLM-03: API calls inside loops without an iteration bound nearby (cost/DoS)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text(errors='ignore').splitlines()
    for i, line in enumerate(lines):
        if re.search(r'messages\.create|chat\.completions|\.generate\(', line):
            window = lines[max(0, i-15):i]
            in_loop = any(re.match(r'\s*(while|for)\b', w) for w in window)
            bounded = any(re.search(r'range\(|max_|limit|\[:\d', w) for w in window)
            if in_loop and not bounded:
                print(f'{f}:{i+1}: LLM call inside loop with no visible bound')
" 2>/dev/null || true
```

## Python-Specific Detection

### Security
```bash
# shell=True
rg -n 'shell\s*=\s*True' --type py src/ || true

# os.system()
rg -n 'os\.system\(' --type py src/ || true

# Unsafe deserialization
rg -n 'pickle\.loads?\(' --type py src/ || true
rg -n 'yaml\.load\(' --type py src/ | grep -v SafeLoader || true
rg -n '\beval\s*\(' --type py src/ | grep -v 'ast.literal_eval' || true
```

### Subprocess (multi-line aware)
```bash
# subprocess without timeout: MUST use Python for multi-line detection
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    text = f.read_text()
    for m in re.finditer(r'subprocess\.(run|call|check_output|check_call|Popen)\s*\(', text):
        start = m.start()
        depth, pos = 1, m.end()
        while pos < len(text) and depth > 0:
            if text[pos] == '(': depth += 1
            elif text[pos] == ')': depth -= 1
            pos += 1
        call_text = text[m.start():pos]
        if 'timeout' not in call_text:
            line_num = text[:start].count('\n') + 1
            print(f'{f}:{line_num}: subprocess.{m.group(1)}() without timeout=')
" 2>/dev/null || true

# String interpolation in subprocess args
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    text = f.read_text()
    for m in re.finditer(r'subprocess\.\w+\s*\(', text):
        start = m.start()
        depth, pos = 1, m.end()
        while pos < len(text) and depth > 0:
            if text[pos] == '(': depth += 1
            elif text[pos] == ')': depth -= 1
            pos += 1
        call_text = text[m.start():pos]
        if re.search(r'f[\"\\x27]|\.format\(|%\s', call_text) and 'shell' not in call_text:
            line_num = text[:start].count('\n') + 1
            print(f'{f}:{line_num}: f-string/format in subprocess args')
" 2>/dev/null || true
```

### Type Modernization (Python 3.10+)
```bash
# Old typing imports
rg -n 'from typing import.*(Optional|List|Dict|Tuple|Set|Union)' --type py src/ || true

# Public functions without return type
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    for i, line in enumerate(f.read_text().splitlines(), 1):
        if re.match(r'^(\s{0,8})def\s+(?!_|test_)\w+\(.*\)\s*:', line) and '->' not in line:
            print(f'{f}:{i}: {line.strip()[:80]}')
" 2>/dev/null | head -30 || true

# Unsafe nested dict access on external data
rg -n '\[.+\]\[.+\]' --type py src/ | grep -v 'test' | head -20 || true
```

### Streamlit-Specific
```bash
# st.rerun() without invalidate() in preceding N lines
python3 -c "
import pathlib
for f in pathlib.Path('src/web').rglob('*.py') if pathlib.Path('src/web').exists() else []:
    lines = f.read_text().splitlines()
    for i, line in enumerate(lines):
        if 'st.rerun()' in line:
            window = lines[max(0,i-10):i]
            if not any('invalidate' in w for w in window):
                print(f'{f}:{i+1}: st.rerun() without invalidate() in preceding 10 lines')
" 2>/dev/null || true

# @st.cache_data without TTL
rg -n '@st\.cache_data' --type py src/ | grep -v 'ttl' || true

# print() in web layer
rg -n '\bprint\(' --type py src/web/ 2>/dev/null | grep -v 'console' || true
```

## PHP-Specific Detection

### Security
```bash
# SQL with string interpolation
rg -n '(query|execute|prepare)\s*\(.*[\$"]' --type php src/ \
  | grep -v 'prepare.*?\?' | grep -v bindParam || true

# Shell execution functions
rg -n '\b(exec|system|shell_exec|passthru|proc_open|popen)\s*\(' --type php src/ || true

# File operations with variables (path traversal risk)
rg -n '(include|require|file_get_contents|fopen|unlink|rmdir)\s*\(\s*\$' --type php src/ || true

# echo/print without escaping
rg -n '(echo|print)\s+\$' --type php src/ | grep -v 'htmlspecialchars\|htmlentities' | head -20 || true

# unserialize on potentially untrusted data
rg -n 'unserialize\s*\(' --type php src/ || true
```

### Architecture
```bash
# env() outside config files (Laravel: returns null when cached)
rg -n '\benv\(' --type php src/ --glob '!config/*' 2>/dev/null | head -20 || true

# Missing $fillable/$guarded on Eloquent models
for f in $(find src/ app/ -name '*.php' 2>/dev/null | xargs grep -l 'extends Model' 2>/dev/null); do
  grep -L 'fillable\|guarded' "$f" && echo "$f: missing \$fillable/\$guarded"
done 2>/dev/null || true
```

### Modernization
```bash
# Legacy array() syntax
rg -n '\barray\s*\(' --type php src/ | head -20 || true

# strpos instead of str_contains (PHP 8.0+)
rg -n 'strpos\s*\(' --type php src/ | head -20 || true
```

## JavaScript/TypeScript-Specific Detection

### Security
```bash
# XSS vectors
rg -n 'innerHTML|outerHTML|document\.write|dangerouslySetInnerHTML|v-html' --type js --type ts src/ || true

# eval / Function constructor
rg -n '\beval\s*\(|new\s+Function\s*\(' --type js --type ts src/ || true

# Hardcoded JWT secrets
rg -n 'jwt\.(sign|verify)\s*\(' --type js --type ts src/ | head -10 || true
```

### TypeScript Quality
```bash
# any type usage
rg -n ':\s*any\b' --type ts src/ | head -20 || true

# Type assertions (hiding real errors)
rg -n '\bas\s+\w' --type ts src/ | head -20 || true

# @ts-ignore without explanation
rg -n '@ts-ignore|@ts-expect-error' --type ts src/ || true

# Non-null assertions
rg -n '\w+!' --type ts src/ | grep -v '!=\|!=' | head -20 || true
```

### React Patterns
```bash
# useEffect without cleanup (missing return in useEffect callback)
# Approximate: agents should verify
rg -n 'useEffect\(' --type ts --type js src/ | head -20 || true

# Missing key prop indicator (map without key)
rg -n '\.map\(' --type ts --type js src/ | head -20 || true

# Large components (>200 lines)
find src/ -name '*.tsx' -o -name '*.jsx' 2>/dev/null | xargs wc -l 2>/dev/null \
  | awk '$1 > 200 && !/total$/' | sort -rn || true
```

### Node.js
```bash
# Sync file operations in non-config code
rg -n 'readFileSync|writeFileSync|existsSync' --type js --type ts src/ \
  | grep -v 'config\|setup\|init' | head -20 || true

# Missing error handling middleware (Express)
rg -n 'app\.(get|post|put|delete|patch)\(' --type js --type ts src/ | head -10 || true
rg -n 'err,\s*req,\s*res,\s*next' --type js --type ts src/ || true
```

## Cross-Reference Detection

These checks compare two sources of truth. They're project-specific by nature; the optimizer should generate them based on what registration patterns the project uses.

### Registration Completeness Template
```bash
# Template: check that all X are registered in Y
# Adapt the patterns to the project's registration mechanism

# Example: Python views registered in PAGE_MAP
python3 -c "
import pathlib, re
view_dir = pathlib.Path('src/web/views')
init_file = view_dir / '__init__.py'
if not init_file.exists(): exit()
init_text = init_file.read_text()
for f in view_dir.glob('*.py'):
    if f.name.startswith('_'): continue
    for m in re.finditer(r'def (render_\w+)', f.read_text()):
        if m.group(1) not in init_text:
            print(f'{f}:{0}: {m.group(1)} not registered in PAGE_MAP')
" 2>/dev/null || true

# Example: PHP routes vs controllers
# Example: React pages vs router config
# Example: CLI commands vs command group registration
```

### Export Completeness Template
```bash
# Template: check that modules export what they define
# Example: Python __init__.py exports
python3 -c "
import pathlib, re
pkg = pathlib.Path('src/web/components')
init = pkg / '__init__.py'
if not init.exists(): exit()
init_text = init.read_text()
for f in pkg.glob('*.py'):
    if f.name.startswith('_'): continue
    for m in re.finditer(r'^def (\w+)|^class (\w+)', f.read_text(), re.MULTILINE):
        name = m.group(1) or m.group(2)
        if name and not name.startswith('_') and name not in init_text:
            print(f'{f}: {name} not exported in __init__.py')
" 2>/dev/null || true
```

# Preflight Script Template

Use this as the skeleton when generating a project-specific preflight script in Phase 4b. Replace the placeholder checks with actual checks derived from the project analysis.

## Script Structure

```bash
#!/usr/bin/env bash
# Preflight review checks for [PROJECT NAME]
# Generated by a-review-optimizer on [DATE]
# Runs deterministic pattern checks, outputs JSON for agent consumption.

set -euo pipefail

SRC_DIR="${1:-src/}"
RESULTS_FILE=$(mktemp)

# Initialize JSON output
echo '{' > "$RESULTS_FILE"
FIRST=true

emit() {
    local id="$1" status="$2" count="$3" message="$4"
    shift 4
    # Remaining args are locations
    local locations="[]"
    if [ $# -gt 0 ]; then
        locations=$(printf '%s\n' "$@" | python3 -c "
import sys, json
print(json.dumps([l.strip() for l in sys.stdin if l.strip()]))")
    fi

    if [ "$FIRST" = true ]; then
        FIRST=false
    else
        echo ',' >> "$RESULTS_FILE"
    fi

    cat >> "$RESULTS_FILE" <<ITEM
  "$id": {
    "status": "$status",
    "count": $count,
    "message": "$message",
    "locations": $locations
  }
ITEM
}

# ============================================================
# GROUP A: Security Checks (route to Security Agent)
# ============================================================

# SEC-01: [description]
HITS=$(rg -n '[PATTERN]' --type [TYPE] "$SRC_DIR" 2>/dev/null || true)
COUNT=$(echo "$HITS" | grep -c . 2>/dev/null || echo 0)
if [ "$COUNT" -gt 0 ]; then
    LOCS=$(echo "$HITS" | awk -F: '{print $1":"$2}')
    emit "SEC-01" "fail" "$COUNT" "$COUNT [description]" $LOCS
else
    emit "SEC-01" "pass" "0" "No [description] found"
fi

# SEC-02: [next check...]
# ... repeat pattern for each check

# ============================================================
# GROUP B: Subprocess/External Call Checks (route to Architecture Agent)
# ============================================================

# SUB-01: Multi-line subprocess check (Python required for balanced-paren scan)
# Example filled in; replace with a pattern that matches THIS project.
# See references/pattern-detection.md for more detection recipes.
HITS=$(python3 -c "
import re, pathlib
for f in pathlib.Path('$SRC_DIR').rglob('*.py'):
    text = f.read_text()
    for m in re.finditer(r'subprocess\.(run|call|check_output|check_call|Popen)\s*\(', text):
        start = m.start()
        depth, pos = 1, m.end()
        while pos < len(text) and depth > 0:
            if text[pos] == '(': depth += 1
            elif text[pos] == ')': depth -= 1
            pos += 1
        call_text = text[m.start():pos]
        if 'timeout' not in call_text:
            line_num = text[:start].count('\n') + 1
            print(f'{f}:{line_num}: subprocess.{m.group(1)}() without timeout=')
" 2>/dev/null || true)
COUNT=$(echo "$HITS" | grep -c . 2>/dev/null || echo 0)
if [ "$COUNT" -gt 0 ]; then
    LOCS=$(echo "$HITS" | awk -F: '{print $1":"$2}')
    emit "SUB-01" "warn" "$COUNT" "$COUNT subprocess calls without timeout" $LOCS
else
    emit "SUB-01" "pass" "0" "All subprocess calls have timeout"
fi

# ============================================================
# GROUP C: Framework-Specific Checks (route varies by check)
# ============================================================

# WEB-01, WEB-02, etc.: only include if project uses a web framework
# CLI-01, CLI-02, etc.: only include if project has CLI commands

# ============================================================
# GROUP D: Type Safety Checks (route to Quality Agent)
# ============================================================

# TYPE-01: [description]
# ...

# ============================================================
# GROUP E: Code Quality Checks (route to Quality Agent)
# ============================================================

# QUAL-01: Files over 300 lines
HITS=$(find "$SRC_DIR" -name '*.py' -o -name '*.php' -o -name '*.ts' -o -name '*.js' 2>/dev/null \
    | xargs wc -l 2>/dev/null | awk '$1 > 300 && !/total$/ {print $2":"$1}' || true)
COUNT=$(echo "$HITS" | grep -c . 2>/dev/null || echo 0)
if [ "$COUNT" -gt 0 ]; then
    LOCS=$(echo "$HITS" | awk -F: '{print $1}')
    emit "QUAL-01" "info" "$COUNT" "$COUNT files over 300 lines" $LOCS
else
    emit "QUAL-01" "pass" "0" "No oversized files"
fi

# ============================================================
# GROUP F: Performance Checks (route to Performance Agent)
# ============================================================

# PERF-01: [description]
# ...

# ============================================================
# Close JSON and output
# ============================================================

echo '' >> "$RESULTS_FILE"
echo '}' >> "$RESULTS_FILE"

cat "$RESULTS_FILE"
rm -f "$RESULTS_FILE"
```

## Check ID Convention

| Prefix | Agent Target | Domain |
|-|-|-|
| SEC-* | Security | Injection, credentials, permissions |
| SUB-* | Architecture | Subprocess safety, external call patterns |
| EXC-* | Architecture | Exception handling patterns |
| WEB-* | Architecture | Web framework patterns (Streamlit, Django, React, etc.) |
| CLI-* | Architecture | CLI framework patterns (Click, argparse, etc.) |
| REG-* | Architecture | Registration completeness (routes, views, exports) |
| TYPE-* | Quality | Type annotations and safety |
| QUAL-* | Quality | Complexity, dead code, formatting |
| DEAD-* | Quality | Dead code detection |
| PERF-* | Performance | Query patterns, caching, I/O |
| SH-* | Architecture | Shell script silent failures (set -e, pipefail, || true on mutations, EXIT traps) |
| INJ-* | Security | Hostile content aimed at the AI reviewer (invisible Unicode, injection phrases). MANDATORY in every generated script |
| LLM-* | Security | LLM integration (prompt injection sites, output handling, unbounded API loops). Only when the project calls an LLM |

The check-ID prefix is the routing key: the skill splits the preflight JSON by prefix and injects each group into the owning agent's `PREFLIGHT KNOWN ISSUES` block. For an unambiguous mapping, mature scripts also emit an explicit `"agent"` (and human-readable `"name"`) field per check, e.g.:

```json
"SEC-04": { "name": "POST forms without CSRF", "agent": "security", "status": "fail", "count": 2, "files": ["app/views/x.php:12", "app/views/y.php:40"] }
```

That way the dispatcher can route on the field directly and the JSON is self-documenting. A top-level `"status"` (`pass`/`warn`/`fail`) summarizing the worst check is also useful for a fast gate.

## Rules for Good Preflight Checks

1. **Every check must output file:line locations.** Counts alone aren't useful.
2. **Use Python for multi-line patterns.** grep/rg work for single-line patterns. For anything that spans lines (subprocess calls, except blocks, function signatures with defaults on next line), use inline Python.
3. **Handle "not found" gracefully.** `|| true` on every command. Empty output = pass, not error.
4. **Limit output.** `| head -20` on checks that could produce hundreds of results. The agent will do the full scan.
5. **Include whitelist comments.** If a check has known false positives for this project, document them as comments so the next person maintaining the script understands.
6. **Status meanings:**
   - `pass`: check ran, nothing found
   - `warn`: found something that needs agent review (might be false positive)
   - `fail`: found something almost certainly wrong
   - `info`: metric/count, no action needed (e.g., file size counts)
7. **INJ-* checks are mandatory in every generated script, whatever the stack.** They defend the reviewer itself: a prompt-injection payload in a comment can hijack the agent that reads the file, so these checks must stay deterministic and must never be delegated to an agent prompt. Copy them from `pattern-detection.md` > Hostile Content Detection. An INJ hit is always `fail`, and the reconciliation step must treat a dismissed INJ finding as requiring a written reason.
8. **Use real linters opportunistically, greps as the guaranteed baseline.** The hand-rolled checks run on any machine; external tools catch classes regex cannot. Gate each on availability and never let a missing tool fail the script:

   ```bash
   # TOOL-01: shellcheck when installed (apt install shellcheck)
   if command -v shellcheck >/dev/null 2>&1; then
       HITS=$(shellcheck -f gcc -S warning $(find . -name '*.sh' -not -path '*/.git/*') 2>/dev/null | head -30 || true)
       # emit as usual
   else
       emit "TOOL-01" "info" "0" "shellcheck not installed, skipped"
   fi

   # TOOL-02/03: Python projects, via uvx (no install needed where uv is standard)
   command -v uvx >/dev/null 2>&1 && uvx ruff check --output-format concise "$SRC_DIR" 2>/dev/null | head -30 || true
   command -v uvx >/dev/null 2>&1 && uvx bandit -q -r "$SRC_DIR" -f custom --msg-template '{relpath}:{line}: {test_id} {msg}' 2>/dev/null | head -30 || true
   ```

# Review Dimensions Taxonomy

Use this during the gap-analysis phase to identify what the existing skill or rule set is missing. For each dimension, check whether it is already covered AND whether the project actually needs it.

Not every project needs every dimension. A CLI tool doesn't need "WebSocket security." A static site doesn't need "N+1 query detection." Skip what's irrelevant, flag what's missing and needed.

## Security Dimensions

### Input Handling
- SQL injection (parameterized queries, ORM safety)
- Command injection (subprocess, exec, system calls)
- Path traversal (filesystem operations with user input)
- XSS (output encoding, template escaping)
- SSRF (URL construction from user input)
- Deserialization (pickle, unserialize, YAML load, eval)
- ReDoS (catastrophic regex backtracking on user input)
- Header injection (CRLF in user-controlled headers)
- Template injection (SSTI in Jinja2, Twig, EJS)

### Credentials & Secrets
- Hardcoded passwords, API keys, tokens in source
- Secrets in logs, error messages, or stack traces
- Secrets committed to git (even in history)
- Encryption keys with wrong file permissions
- Default/weak passwords in config examples
- Credentials in URLs (basic auth in connection strings)

### Authentication & Authorization
- Missing auth checks on endpoints/routes
- Auth without authz (logged in but not permitted)
- Session fixation (no regeneration after login)
- CSRF protection on state-changing requests
- JWT: hardcoded secret, missing expiry, alg:none
- Cookie flags: httpOnly, secure, sameSite
- Timing attacks on password/token comparison
- Password hashing: bcrypt/argon2 vs MD5/SHA1

### Data Protection
- Sensitive data in localStorage/sessionStorage
- PII in logs or analytics
- Missing encryption at rest for sensitive data
- Broad CORS configuration with credentials
- Missing rate limiting on auth endpoints

### Hostile Content (reviewer integrity)
These run as deterministic preflight checks ONLY, never as agent-prompt checks: the AI reviewer is the attack target here, and a hijacked reviewer cannot be trusted to flag its own hijack.
- Invisible or bidirectional Unicode in source (Trojan Source: U+202A-202E, U+2066-2069, zero-width U+200B-200D, U+FEFF)
- Instruction-like phrases aimed at AI tools in comments/strings/docs ("ignore previous instructions", "you are now", "do not flag this")
- Large encoded blobs (base64) in comments with no stated purpose

## LLM Integration Dimensions (conditional)

Only when the project calls an LLM API (imports anthropic/openai/ollama, or hits an LLM HTTP endpoint). Skip entirely otherwise.

### Prompt Injection
- Untrusted content (user input, scraped pages, file contents, third-party API responses) concatenated into prompts without delimiting or marking
- System instructions and untrusted content mixed into the same message role
- LLM output fed into subsequent prompts unsanitized (injection laundering across calls)

### Output Handling
- LLM output rendered as HTML/markdown without escaping
- LLM output parsed as code, SQL, or shell, or passed to eval/exec
- LLM output driving decisions (scores, filters, routing) without schema or bounds validation
- Unbounded loops calling the API (no max iterations, no cost guard)

### Tool Use & Data Exposure
- Tools/functions exposed to the model broader than the task needs
- Secrets or PII included in prompt context
- Model-controlled parameters reaching filesystem, network, or DB operations without validation

## Architecture Dimensions

### Configuration Management
- Hardcoded values that should be configurable
- Config access scattered vs centralized
- Missing defaults for optional config
- Environment-specific logic in business code
- Secrets mixed with non-secret config

### Error Handling Strategy
- Generic catch-all exceptions
- Swallowed exceptions (catch + pass/ignore)
- Empty or log-only catch blocks (catch that discards the error and continues)
- Error suppression operators (@ in PHP, error_reporting(0), blanket 2>/dev/null in shell)
- Fail-open error paths (error branch returns a success-looking default: None, true, empty list)
- Missing cleanup on failure (partial state)
- Missing timeouts on external calls
- Error messages that don't help diagnose
- Inconsistent error return shapes
- Missing retry logic with backoff where needed

### Shell Scripts
- Missing set -euo pipefail (or targeted equivalents) in scripts that change state
- || true or 2>/dev/null on state-changing commands (masks real failures; fine on read-only probes)
- cd without failure guard (subsequent commands run in the wrong directory)
- mktemp or sensitive output files without an EXIT trap cleanup
- Unquoted variable expansions in paths (word splitting on spaces)

### State Management
- Framework-appropriate state patterns (session_state, context, store)
- Mutable shared state without synchronization
- State scattered across globals
- Cache invalidation after mutations
- Stale state after redirects/reruns

### Dependency & Coupling
- Circular imports/dependencies
- Layer violations (presentation ↔ data)
- God objects everything depends on
- Components reaching into other components' internals
- Business logic in infrastructure code

### Registration & Wiring
- Routes/views/commands registered correctly
- Middleware/interceptors applied where needed
- Event listeners registered for dispatched events
- Components exported from package indexes
- DI container bindings for all interfaces

### API Design
- Consistent response shapes
- Input validation at boundaries
- Versioning strategy for external APIs
- Batch operations where N individual calls exist
- Pagination on list endpoints

### Consistency
- Same pattern used differently across files
- Naming conventions followed/violated
- Import ordering convention
- File/directory organization convention

## Quality Dimensions

### Type Safety
- Missing type annotations on public interfaces
- Nullable access without null check
- Generic "any"/"mixed" where specific types work
- Type assertions hiding real type errors
- Dict/array access on external data without .get()
- Inconsistent use of type aliases

### Dead Code
- Functions never called from any module
- Unused imports
- Commented-out code blocks (>3 lines)
- Unreachable code after return/throw
- Feature flags always true/false
- Config keys defined but never accessed
- Event handlers never triggered
- Store actions never dispatched

### Complexity
- Files over 300 lines
- Functions over 50 lines
- Nesting over 4 levels
- Cyclomatic complexity over 10
- Parameter count over 5
- Boolean parameters (should be named/enum)

### Duplication
- Same logic in 2+ places
- Similar error handling repeated
- Similar validation in multiple endpoints
- Copy-pasted data transformations
- Similar test setup across test files

### Modernization
- Outdated syntax for the language version
- Old library APIs when newer alternatives exist
- Manual implementations of things the stdlib handles
- Deprecated function/method usage

### Naming
- Misleading names (function does more than name suggests)
- Boolean variables not starting with is/has/can/should
- Unclear abbreviations
- Inconsistent naming convention within a module

### Test Coverage
- Public methods with complex logic but no tests
- Edge cases not covered (empty input, null, overflow)
- Error paths not tested
- Integration points not tested
- Tests that only test the happy path

## Performance Dimensions

### Query Efficiency
- N+1 queries (loop + query pattern)
- SELECT * when specific columns suffice
- Missing LIMIT on unbounded queries
- Filtering/sorting in app instead of DB
- Missing indexes on filtered/joined columns
- Missing connection pooling
- Repeated identical queries per request

### I/O Efficiency
- File/HTTP reads inside loops
- Synchronous I/O in async context
- Loading full files when streaming works
- Not closing handles/connections (resource leaks)
- Missing batching on external API calls

### Caching
- Same expensive computation repeated per request
- Rarely-changing data fetched every time
- Missing cache invalidation after writes
- Cache keys not accounting for all parameters

### Memory
- Full dataset loaded when pagination/chunking works
- String concatenation in loops (vs join/builder)
- Large objects retained when no longer needed
- Missing generators/iterators for large sequences

### Algorithmic
- O(n²) when O(n) or O(n log n) is possible
- Linear search where hash/set lookup works
- Repeated sorts on same data
- Unnecessary serialization/deserialization cycles

### Response/Payload
- API returning more data than consumer needs
- Missing compression on large responses
- Missing pagination
- Full page re-render when partial update suffices

## AI Slop Dimensions (optional, for --slop equivalent)

### Over-Abstraction
- Interface with exactly one implementation
- Factory for something instantiated once
- Strategy with one strategy
- Wrapper that adds no behavior

### Premature Generalization
- Config options no code path exercises
- Parameters always called with same value
- Plugin systems with zero plugins

### Confident-But-Wrong
- Error handling that doesn't actually handle anything
- Retry without idempotency consideration
- Pagination that breaks on last page
- Auth checking login but not permissions

### Copy-Paste Artifacts
- Variable names from a different context
- Comments describing what code used to do
- Imports for unused libraries
- Exception types from wrong framework

# Generated-Skill Scaffold

The parts of a mature review skill that live *outside* the agent prompts and the report format. `output-template.md` covers the report body and severity definitions; this file covers everything else a hand-tuned review skill contains: the flag surface, the parallel-dispatch skeleton, the growing false-positive whitelist, the `--debt` and `--full` modes, the self-learning capture loop, and rule-candidate surfacing.

Use it two ways:
- **Improving an existing skill (Phase 3/4):** treat the section list below as a coverage checklist. For each section, does the target skill have it? Missing ones are `ADD`; thin ones are `UPDATE`. Never rip out a section the user already tuned.
- **From scratch (Phase 4 "When No Review Skill Exists"):** this is the blueprint. Emit the sections the project actually needs (a static site skips `--debt` query scoring; a CLI tool skips web XSS whitelists), each populated with real file:line referents from Phase 2, never generic placeholders.

Everything here is stack-agnostic. The examples span PHP, Python, JS/TS, Go, and Rust on purpose; pick the idiom that matches the project you analyzed.

---

## 1. Flag surface

A mature skill exposes a small, stable set of scope/depth flags. Emit only the ones the project needs, but keep the names identical across projects so muscle memory transfers.

```markdown
## Flags

| Flag | Behavior |
|-|-|
| `--changed` | Default. Review only files changed since HEAD (`git diff --name-only HEAD`) |
| `--full` | Review the whole source tree + architecture diagrams + health score |
| `--security-only` | Run the Security agent alone |
| `--debt` | Add tech-debt scoring, stricter thresholds, and extra checks to every agent |
| `--all` | `--full` + `--debt` combined (most thorough) |

Flags combine where logical (e.g. `--security-only --debt`).
```

Guidance:
- `--changed` is always the default. State the exact git command so the skill is deterministic about scope.
- `--debt` is a *modifier*, not a separate scope: it lowers thresholds (file-length, function-length) and turns on extra checks that are too noisy for every run.
- Only ship `--full`'s diagram half if the project has a real layered architecture to draw. A 3-file script doesn't.

---

## 2. Execution / dispatch

Every mature skill runs its dimensions concurrently and hands each one only its slice of the preflight output. **Dispatch them through a mechanism that returns each agent's report, not through a batch of fire-and-forget Agent calls.** In Claude Code that mechanism is the Workflow tool, and the rest of this section is written against it. On a harness without Workflow, keep the requirement and substitute its own orchestration primitive: the non-negotiable part is that every dispatched agent's findings come back to the caller, structured, before reconciliation runs.

### Why Workflow, not a batch of Agent calls

The batch-of-Agent-calls form was the recommendation here until 2026-08-05. It fails in a way that is easy to mistake for success: the agents spawn, run to completion, and then go idle without ever returning their report. You get an `idle_notification`, no findings, and a review that looks like it ran. `run_in_background: false` does **not** reliably prevent it (the flag has been observed ignored), `TaskList` does not show the agents, and `SendMessage` nudges return another idle notification rather than the report. Observed twice on the same skill, two runs apart.

The same work expressed as a Workflow script ran 25 agents with zero errors on the run that replaced it. Workflow also buys three things the batch form cannot express: a structured return schema per agent (so findings arrive parseable instead of as prose you re-read), an adversarial verify stage, and a journal you can inspect when a result looks wrong.

Teach the generated skill this shape:

```markdown
## Execution

Dispatch via the Workflow tool. One finder per dimension, then an adversarial
verify pass on every finding the finders return.

| Agent | Owns (checks these) | Does NOT check |
|-|-|-|
| Security | injection, authn/authz, secrets, unsafe deserialization, SSRF | error-handling structure, types, dead code |
| Architecture | layering, registration/wiring, error-handling structure, resource cleanup | injection content, type modernization |
| Quality | types, dead code, duplication, complexity, modernization | anything Security or Architecture owns |

Each finder's prompt carries: the shared context file, its own brief, its slice of
the preflight output split by check-ID prefix (SEC-* → Security, ARCH-*/EXC-*/WEB-* →
Architecture, TYPE-*/QUAL-* → Quality), and the repo root.

If `--security-only` is set, run only the Security dimension.
After the workflow returns, run Post-Flight Reconciliation on the confirmed findings.
```

Use `pipeline()` so each dimension's findings start verifying as soon as that dimension finishes, rather than waiting on the slowest finder. Give every agent a `schema` so findings come back structured. Require a `failure_scenario` field: concrete inputs or state leading to a specific wrong outcome. A finding whose author cannot fill that field is the kind that wastes the reviewer's afternoon.

**The verify stage is not optional.** Prompt it to *refute*, defaulting to refuted when uncertain, and require it to trace the chain from source rather than accept the finder's reasoning. On the 2026-08-05 run this refuted 6 of 18 raw findings, including three whose stated mechanism was accurate but whose claimed consequence did not follow. That failure mode (true mechanism, wrong conclusion) is the dominant one on a mature codebase, and only an adversarial second pass catches it.

Tell the generated skill to state in its report what the verify stage refuted, not just what it confirmed. A run that reports 12 findings and silently drops 6 reads as less trustworthy than one that shows both numbers.

The **Owns / Does NOT check** two-column form is the anti-overlap device: a finding belongs to exactly one agent, and each agent is told in writing what to leave alone. This is where the Phase 3c overlap resolutions get written down. 3+ dimensions is the norm; 2 is acceptable for a small single-concern project, more than 4 usually means you fragmented a scope that should be one dimension.

Workflow requires the user to have opted into multi-agent orchestration. A user-invoked review skill whose instructions say to call Workflow satisfies that, so the generated skill should say so explicitly in its execution section.

---

## 3. Context Awareness (DO NOT flag), and how it grows

The single highest-value section in a mature skill. It records hard-won false positives so the review stops re-flagging correct project conventions. A from-scratch skill starts this section from Phase 2/3b findings; every later optimizer run **appends** to it.

```markdown
## Context Awareness (DO NOT flag)

These patterns are correct in THIS project; agents must not flag them:
- Base class handles prepared statements via inherited query methods (all Models extend it)
- `<Framework>` command functions look unused; they're invoked by the framework via decorators/registry
- `.example` / `.sample` config files are committed intentionally; real configs are gitignored
- Result-tuple return shape `(ok, msg)` is the project convention, not a smell; see src/core/engine.<ext>:45

### Added by a-review-optimizer [YYYY-MM-DD]
- <newly confirmed false positive>, with a real file:line referent and one-line reason
```

**The growth mechanism (make the optimizer do this every run):**
- When Post-Flight Reconciliation *dismisses* a preflight finding as a false positive, that dismissal is knowledge. Add it here under a dated `### Added by a-review-optimizer [DATE]` marker, with the file:line and the one-line reason it's safe.
- Never delete an existing whitelist entry unless you confirmed (via grep of current code) the referenced pattern is gone. Stale-looking entries still catch things.
- Keep the user's original wording verbatim; only append. A rephrased whitelist entry is a whitelist entry the user can no longer trust.
- Prefer a real referent (`file:line`) over prose; it lets the next run verify the entry is still live.

This section and the self-learning log (§6) are two halves of the same loop: confirmed findings feed the log; dismissed findings feed this whitelist.

---

## 4. `--debt` mode

Turns the review into a scored artifact. Only computed when `--debt` (or `--all`) is set.

```markdown
## Debt Scoring (--debt / --all only)

Assign points per confirmed finding, then sum (cap at 100, lower is better):

| Severity | Points |
|-|-|
| Critical | 8 |
| High | 4 |
| Medium | 2 |
| Low | 1 |

### Score Breakdown
| Agent | Critical | High | Medium | Low | Score |
|-|-|-|-|-|-|
| Security | X | X | X | X | X |
| Architecture | X | X | X | X | X |
| Quality | X | X | X | X | X |
| **Total** | X | X | X | X | **X/100** |

Bands: 0 pristine · 25 healthy · 50 needs attention · 75+ stop and fix.

### Auto-Fixable
**Safe (no logic change):**
- [ ] <mechanical fix, e.g. add missing strict/type declarations in N files>
- [ ] <remove unused imports in N files>
- [ ] <swap debug-print for logger in N locations>

**Needs confirmation (behavior may change):**
- [ ] <narrow a broad catch; verify intended behavior>
- [ ] <remove apparently-dead code; confirm no dynamic dispatch>
- [ ] <add null/None check after lookup; decide the miss behavior>
```

`--debt` also *tightens thresholds*: e.g. file-length flag drops from >500 to >300 lines, function-length from >50 to >30, and checks that are `info` on a normal run escalate to `warn`. Wire those threshold shifts into the agent prompts (a small "`--debt` / `--all` only" subsection under the relevant checks), not just the score.

The safe-vs-needs-confirmation split is load-bearing: it's what lets a downstream `fix` skill batch-apply the safe half without a human in the loop.

---

## 5. `--full` mode: architecture diagrams + health score

Only when `--full` (or `--all`) is set. Skip entirely for projects without a real layered structure.

**Dependency diagram**: scan the import mechanism for the stack (`use` in PHP, `import`/`from` in Python, `import`/`require` in JS/TS, `import` in Go, `use` in Rust) and draw cross-layer edges only (drop same-layer noise):

```markdown
### Component Dependencies
​```mermaid
graph LR
  subgraph Controllers
    BlogController
  end
  subgraph Services
    BlogService
  end
  subgraph Models
    BlogPost
  end
  BlogController --> BlogService
  BlogService --> BlogPost
​```
```

**Layer diagram**: the *allowed* dependency direction. Flag arrows going the wrong way (a violation) with red styling:

```markdown
### Layer Dependencies
​```mermaid
graph TD
  Views --> Controllers
  Controllers --> Services
  Controllers --> Models
  Services --> Models
  Models --> Core
​```
```

The layer names come from Phase 2 (MVC → Views/Controllers/Services/Models; clean/hexagonal → presentation/domain/data; pick what the project actually uses).

**Architecture Health Score**: weighted, project-specific. Weights should sum to 100 and reflect what matters for *this* architecture:

```markdown
### Architecture Health Score: X/100
| Check | Weight | Scoring |
|-|-|-|
| Layer compliance | 30 | -5 per violation |
| Base-class / interface compliance | 20 | -5 per non-compliant unit |
| Boundary pattern (mutations through services/repos) | 20 | -5 per bypass |
| Registration / wiring completeness | 15 | -3 per missing registration |
| File organization / naming | 15 | -3 per misplaced file |
```

---

## 6. Self-learning capture loop (MANDATORY in the generated skill)

A mature review skill doesn't just report; it **records** every confirmed finding to a per-project log so recurring issues can later harden rules and preflight (via `a-self-learner`). Scaffold all three pieces so a from-scratch skill wires the loop, not just the review:

### 6a. Emit the capture script

Write `.claude/scripts/capture-finding.sh` (stack-agnostic: it only appends JSON). Generic version to emit verbatim, adjusting nothing but staying project-neutral:

```bash
#!/usr/bin/env bash
# capture-finding.sh: append one finding to .claude/reviews/review-issues.jsonl
# Call once per confirmed finding. Safe to call from any review skill in any project.
set -euo pipefail

PROJECT=""; SKILL=""; RUN_ID=""; DIMENSION=""; SEVERITY=""
CATEGORY=""; FILE=""; LINE="0"; MESSAGE=""
FIX=""; AGENT=""; CHECK_ID=""; WHITELISTED="false"

while [ $# -gt 0 ]; do
  case "$1" in
    --project) PROJECT="$2"; shift 2 ;;
    --skill) SKILL="$2"; shift 2 ;;
    --run-id) RUN_ID="$2"; shift 2 ;;
    --dimension) DIMENSION="$2"; shift 2 ;;
    --severity) SEVERITY="$2"; shift 2 ;;
    --category) CATEGORY="$2"; shift 2 ;;
    --file) FILE="$2"; shift 2 ;;
    --line) LINE="$2"; shift 2 ;;
    --message) MESSAGE="$2"; shift 2 ;;
    --fix) FIX="$2"; shift 2 ;;
    --agent) AGENT="$2"; shift 2 ;;
    --check-id) CHECK_ID="$2"; shift 2 ;;
    --whitelisted) WHITELISTED="true"; shift ;;
    *) echo "Unknown arg: $1" >&2; exit 2 ;;
  esac
done

for v in PROJECT SKILL RUN_ID DIMENSION SEVERITY CATEGORY FILE MESSAGE; do
  if [ -z "${!v}" ]; then echo "capture-finding.sh: missing required --${v,,}" >&2; exit 2; fi
done

DATE=$(date -u +%Y-%m-%d)
REVIEWS_DIR=".claude/reviews"; LOG="$REVIEWS_DIR/review-issues.jsonl"
mkdir -p "$REVIEWS_DIR"

# Normalize the clustering signal: lowercase, collapse spaces, mask numbers and quoted literals
NORM=$(printf '%s' "$MESSAGE" | tr '[:upper:]' '[:lower:]' \
  | sed -E 's/[[:space:]]+/ /g; s/^ //; s/ $//; s/[0-9]+/#/g; s/"[^"]*"/""/g')
HASH=$(printf '%s\x01%s\x01%s' "$DIMENSION" "$CATEGORY" "$NORM" | sha256sum | cut -c1-16)
FINDING_ID=$(head -c 8 /dev/urandom | od -An -tx1 | tr -d ' \n')

python3 - "$DATE" "$PROJECT" "$SKILL" "$RUN_ID" "$FINDING_ID" "$DIMENSION" \
  "$SEVERITY" "$CATEGORY" "$FILE" "$LINE" "$MESSAGE" "$FIX" "$AGENT" \
  "$CHECK_ID" "$HASH" "$WHITELISTED" "$LOG" <<'PY'
import json, sys
(date, project, skill, run_id, finding_id, dimension, severity, category,
 file_, line, message, fix, agent, check_id, category_hash, whitelisted, log) = sys.argv[1:]
rec = {"date": date, "project": project, "skill": skill, "run_id": run_id,
       "finding_id": finding_id, "dimension": dimension, "severity": severity,
       "category": category, "file": file_, "line": int(line or 0),
       "message": message, "category_hash": category_hash,
       "whitelisted": whitelisted == "true"}
if fix: rec["fix_proposed"] = fix
if agent: rec["agent"] = agent
if check_id: rec["check_id"] = check_id
with open(log, "a") as f:
    f.write(json.dumps(rec, separators=(",", ":")) + "\n")
PY
```

### 6b. Emit the capture section in the generated SKILL.md

```markdown
## Capture Findings for Self-Learning (MANDATORY)

After reconciliation, append every CONFIRMED finding (all severities, including INFO)
to `.claude/reviews/review-issues.jsonl` via `.claude/scripts/capture-finding.sh`.
This feeds `/a-self-learner` so recurring issues harden rules/preflight over time.

Generate one `RUN_ID` per review (e.g. `r-YYYYMMDD-HHMM`); reuse it for every finding
in the run. Call once per finding:

​```bash
bash .claude/scripts/capture-finding.sh \
  --project <project> --skill <this-skill> --run-id "$RUN_ID" \
  --dimension <security|architecture|quality|performance> \
  --severity <critical|high|medium|low|info> \
  --category <stable-slug> \
  --file <repo-relative-path> --line <n> \
  --message "<one-line description>" \
  [--fix "<proposed fix>"] [--agent <agent>] [--check-id <preflight id>]
​```

Whitelisted/dismissed findings are NOT captured, only confirmed ones. (Dismissed
findings go to the Context Awareness whitelist instead.)
```

### 6c. Seed the stable category-slug table

The `--category` slug is the clustering key: the **same class of issue must get the same slug every run**, or `a-self-learner` can't see the recurrence. Seed a generic table and let the skill coin project-specific slugs (lowercase kebab-case noun phrases) as new patterns appear:

| Pattern | Slug |
|-|-|
| SQL built by string concatenation/interpolation | `sql-injection-string-concat` |
| Output rendered without escaping (XSS) | `xss-unescaped-output` |
| State-changing endpoint without CSRF check | `missing-csrf-validation` |
| Secret/API key hardcoded in source | `hardcoded-secret` |
| Token/hash compared with `==` not constant-time | `non-constant-time-compare` |
| URL field used without protocol allowlist | `missing-url-protocol-validation` |
| Unsafe deserialization (pickle/unserialize/yaml.load) | `unsafe-deserialization` |
| Subprocess/exec without timeout | `subprocess-without-timeout` |
| Command built with string interpolation | `command-injection-interpolation` |
| Path from input used without containment check | `path-traversal-unchecked` |
| Bare/silent catch that swallows the error | `swallowed-exception` |
| Broad catch with no logging | `broad-catch-no-log` |
| DB/data access outside the data layer | `data-access-outside-layer` |
| Mutation bypassing the service/repository layer | `mutation-bypasses-service` |
| Component/route/view not registered where required | `missing-registration` |
| Check-then-act on the filesystem (TOCTOU) | `toctou-filesystem` |
| Missing null/None check after a lookup | `missing-null-check` |
| Unused import/use statement | `unused-import` |
| Dead private method / unreachable code | `dead-code` |
| Function longer than the project limit | `function-too-long` |
| File longer than the project limit | `file-too-long` |
| Duplicated logic that should be shared | `duplicate-logic` |
| Stringly-typed value where an enum/const exists | `stringly-typed-value` |
| Unbounded query / missing LIMIT | `unbounded-query-no-limit` |
| Query inside a loop (N+1) | `n-plus-one-query` |
| Debug print/log left in source | `debug-statement-left-in` |

---

## 7. Rule-candidate surfacing

When a review keeps finding the same class of issue, the fix is a *rule*, not another finding. Teach the generated skill to surface candidates at the end of every run:

```markdown
## Rule Candidates

A finding is a rule candidate when: the same issue appears in 3+ locations, OR it
reflects an undocumented project convention, OR the fix needs knowledge not obvious
from the code. For each candidate, check `.claude/rules/` (or CLAUDE.md); if it's
not already covered, propose adding it to the right file. Rules are one actionable
statement, no code blocks (use `Reference: path/file.ext` instead).

| Pattern | Occurrences | Proposed Rule | Target File |
|-|-|-|-|
| <recurring pattern> | N | <one-line actionable rule> | <rules file> |

Ask before adding; never write a rule silently.
```

This is the manual, in-the-moment counterpart to `a-self-learner` (which mines the accumulated `review-issues.jsonl` later). Both routes end at `a-rules-optimizer`, which owns the actual rule edit.

---

## 8. Stack detection and per-stack references

A review skill that applies one lens to every file reviews the minority stacks badly. Most projects are not monolingual: a PHP app also ships browser JS, SQL migrations and CLI tooling, and a checklist written for the dominant language says nothing useful about the others.

Split the generated skill three ways:

- `SKILL.md` carries the trigger, the shared spine and the dispatch. Nothing stack-specific.
- `references/agent-*.md` carry the per-dimension scope, as today.
- `references/stack-*.md` carry stack specifics and load **only when the diff touches that stack**.

### Detect from real signals, matched against the diff

Match changed paths, not repo contents. A PHP-only change must not pay for the SQL reference. Derive the signal list from what Phase 2 actually found; do not paste a menu of every ecosystem.

```markdown
| Changed paths match | Add to prompt | Goes to |
|-|-|-|
| `**/*.php`                   | `references/stack-php.md`            | all |
| `public/assets/js/**`        | `references/stack-browser-js.md`     | Quality + Security |
| `tests/*.mjs`                | `references/stack-node-tests.md`     | Quality |
| `database/migrations/*.sql`  | `references/stack-sql-migrations.md` | Architecture |
```

Anchor detection on manifests and markers that exist (`composer.json` with a `require.php` floor, a `package.json` and its absence, `tsconfig.json`, `go.mod`, a migrations directory plus a ledger table), not on file extensions alone. The absence of a manifest is itself a signal: browser JS with no `package.json` and no bundler is a different review problem from a bundled React app, and the checks barely overlap.

### The token argument

`SKILL.md` loads in full on every run. Stack knowledge welded into an agent brief is paid for by every review of every file type, and it grows without bound as stacks are added. Behind detection, a typical single-stack diff loads the spine plus one reference, and adding a fifth stack costs one new file rather than another 40 lines that every future run carries.

### What a stack reference contains

Concrete checks with a verification command, not topic headings. "Check for N+1 queries" is worthless; a command whose output is unambiguous, plus what a hit means, is a check:

```markdown
## A new script must be registered in build.php
There is no discovery. A view can load a file the build never minifies.
Verify: `php build.php check; echo "exit=$?"`   Non-zero means unregistered.
```

If a check cannot be verified by running something or by reading one named file, it belongs in an agent brief as judgement, not in a stack reference as a check.

### Per-project, not user-level

Stack references live in the project's own skill. Every good check needs a real referent and a command that only means something in that repo (`php build.php check`, `bin/migrate.php status`, that project's PHPStan level). A shared user-level `stack-php.md` has to be true for every PHP project, which sands it down to the generic advice this whole skill exists to avoid. The cost is real: two PHP projects re-derive overlapping files and drift apart. Take it. One file that is true for neither is worse than two that are each true for one.

The **spine** is the opposite case and stays here, in `review-dimensions.md`: boundary and input handling, authorization on every path touching data, error paths and dependency-down behaviour, secrets and config leakage, unbounded queries and N+1, data migration safety, dead code and near-duplicate logic, and whether tests cover the paths that changed. Those are true everywhere. Justify each against something Phase 2 actually found in the project and drop the ones you cannot; a checklist item nobody can fail is noise.

---
name: a-rules-optimizer
description: Audit .claude/rules/ and CLAUDE.md against what the codebase actually does.
disable-model-invocation: true
---

# Rules Optimizer

Audit `.claude/rules/` and `CLAUDE.md` against the actual codebase. Creates missing rules, fixes drift, ensures coverage. Works on any project regardless of stack.

**Input:** optional flags
- `--full`: full audit and optimization (default)
- `--audit-only`: report drift without making changes
- `--sync`: just fix stale references and drift, skip coverage analysis

If no `.claude/rules/` directory exists, create it and build rules from scratch. That's the whole point of running this skill.

---

## Core Principles

**Rules are instructions, not code examples.** Each rule is a declarative statement: "do X", "never Y", "Z must always include W". No code blocks longer than a single line. If a pattern needs showing, use `Reference: path/file.py:42`.

**Rules prevent what reviews catch.** Every rule should map to something that would otherwise be flagged during code review. If a rule doesn't prevent a real mistake, it's noise.

**Path scoping saves context.** Rules scoped to `src/web/**/*.py` don't load when editing `src/core/`. Use the narrowest glob that covers the files where the rule matters.

**`@imports` to rule files defeat path-scoping.** If `CLAUDE.md` references a rule file with `@.claude/rules/foo.md`, that file is force-loaded at session start regardless of its `paths:` frontmatter. Always reference rule files by name only ("see `services.md`") so the path scoping the user wrote actually takes effect.

**Rules are pointers, not inventories.** Anything derivable by `ls`, `grep`, or reading one well-known file (base classes, enums, route configs) does not belong in a rule file. Lists of controllers, models, services, helpers, partials, URL routes, directory trees, base-class method signatures, design tokens, all rot the moment the code changes. Replace with one-liners: `Inventory: ls app/src/Models/`, `Methods: read app/src/Core/View.php`, `Routes: app/config/routes.php`. Keep the rule (the "must"/"never"), drop the catalog.

**Preserve the user's voice.** If rules already exist, the user wrote them from experience. Keep their wording, organization, and emphasis. Only change what has a concrete reason (drift, gap, stale reference).

---

## Phase 1: Project Profile

Discover the tech stack, architecture, and patterns. This drives everything else.

### 1a: Stack Detection

Build a project profile. Pick **one** of these two ways:

- **Option 1 (default, preferred):** Dispatch an `Explore` agent with the brief below. Useful when the project is large or you want the analysis to stay out of your own context window.
- **Option 2 (fallback):** If no agent is available, run the commands yourself and summarize the findings.

The brief (paste into the agent's prompt, or walk through it yourself):

```
Analyze the project at [CWD]. Report back concisely:

1. STACK: Language(s), framework(s), runtime version requirements
   - Check: package.json, composer.json, requirements.txt, pyproject.toml, go.mod, Cargo.toml, Gemfile, *.sln
   - Check: Dockerfile, docker-compose.yml if present

2. STRUCTURE: Top-level directory layout. Which directories contain source code vs config vs tests vs assets?
   - Run: find . -maxdepth 3 -type d (excluding .git, node_modules, vendor, __pycache__, venv)

3. ARCHITECTURE PATTERN: How is the code organized?
   - MVC, service-repository, layered, modular, flat, monorepo?
   - Where does business logic live?
   - How do modules communicate?

4. KEY PATTERNS (read the 5 largest source files to discover):
   - Error handling: exceptions, result tuples, error codes?
   - Config access: env vars, YAML, JSON, centralized manager?
   - Data access: ORM, raw queries, repository pattern?
   - State management: framework-specific patterns?
   - Registration: how are routes/views/commands/components wired?
   - Logging: which logger, what patterns?

5. BOUNDARIES: Where does external input enter the system?
   - CLI args, HTTP requests, file uploads, API responses, user config?

6. FILE METRICS:
   - Total source files and lines (by language)
   - Largest files (top 10)
   - Test presence and structure

7. DOMAIN VOCABULARY (drives project-specific rules a generic taxonomy can't):
   - Enums / status fields and the meaning of their values (which value gates a downstream action?)
   - Status machines / lifecycle transitions (draft→published, pending→approved)
   - Distinct subsystems with their own conventions (scraper, consolidator, scorer, feature-flag registry, importer)
   - Cross-entity invariants ("excluded rows never get positive flags", "counts never overwritten on sync")
   - Content-integrity constraints (fields that must not be edited via raw SQL, unique keys, derivation provenance)

Report in structured sections, not prose. Keep under 3000 chars.
```

### 1b: Existing Rules Inventory

If `.claude/rules/` exists, read every rule file. For each one, extract:
- File name and path scope (from frontmatter `paths:` or `alwaysApply:`)
- Section headings (these are the rule categories)
- Individual rules (bullet points under each heading)
- Any file:line references (these need freshness checks)

Build a flat list: `[rule_file, category, rule_text, referenced_paths[]]`

Also read `CLAUDE.md` and extract:
- Sections that describe architecture, patterns, or conventions
- Any references to `.claude/rules/`
- File paths or line numbers mentioned anywhere

### 1c: Review Skill Cross-Reference

If a review skill exists (check `.claude/skills/*/SKILL.md` for review-related skills), read it and extract:
- What checks each agent performs
- What preflight scripts exist and what they scan for
- Known-correct pattern whitelists

This tells you what the review process catches, so rules can prevent those same issues upstream.

If preflight scripts exist (`.claude/scripts/preflight-*.sh`), read them and extract check IDs and what they detect.

---

## Phase 2: Coverage Analysis

Cross-reference what rules exist against what the project needs.

### 2a: Dimension Mapping

Read `references/review-dimensions.md` for the full taxonomy. For each dimension, ask two questions:

1. **Is this dimension relevant to this project?** A CLI tool doesn't need WebSocket rules. A static site doesn't need N+1 query rules. Skip irrelevant dimensions.

2. **Is this dimension covered by existing rules?** Map each relevant dimension to the rule file and bullet point that covers it. Mark uncovered dimensions.

**What "covered" means.** A dimension is covered only when a rule **explicitly forbids or prescribes** the pattern, with a concrete signal (file path, function name, framework primitive), not just a passing mention. "Validate input" alone is not coverage for SQL injection; "All SQL goes through `$pdo->prepare()`; never concatenate `$_POST` into a query string" is. If a rule only names the topic, mark it PARTIAL.

**Derive domain dimensions (do not stop at the fixed taxonomy).** `review-dimensions.md` covers generic engineering concerns (security/architecture/quality/performance). It cannot list the rules that only this codebase needs; those come from the DOMAIN VOCABULARY in the Phase 1 profile. For each item there, add a project-specific dimension row to the matrix and check coverage:

- **Enum/status semantics** → is there a rule stating what each value permits or forbids (which value gates outreach/publishing/deletion), and the `NONE`-vs-`NULL` style distinctions?
- **Status-machine transitions** → is the legal lifecycle written down, and are illegal transitions forbidden?
- **Subsystem invariants** → does each distinct subsystem (scraper, consolidator, importer, feature-flag registry) have a rule file or section capturing its non-obvious contract (SSRF/redirect model, dedup key, sync-safety)?
- **Cross-entity invariants** → is the integrity rule that must hold across tables/modules stated ("excluded rows never get positive flags", "analytics counts never overwritten on import")?
- **Content-integrity constraints** → are the "never edit this field via raw SQL", unique-key, and provenance rules present?

A rule set that covers every generic dimension but none of the domain dimensions is under-covering. These project-specific rules are usually the highest-value ones; they encode knowledge no linter or generic reviewer has. Mark each derived domain dimension Covered / Partial / Gap just like the taxonomy rows.

Output a coverage matrix:

```
COVERAGE MATRIX
===============
Dimension                    | Relevant? | Covered by           | Gap?
SQL injection                | YES       | security.md:Input    | NO
Command injection            | YES       | security.md:Subproc  | NO
Path traversal               | YES       | security.md:File     | NO
XSS                          | YES       | security.md:XSS      | NO
SSRF                         | YES       | security.md:Network  | NO
Cache invalidation           | YES       | web-layer.md:Cache   | NO
N+1 queries                  | NO (no ORM)| -                   | -
Registration/wiring          | YES       | web-layer.md:Reg     | NO
Type safety                  | YES       | code-quality.md:Type | NO
Dead code                    | YES       | code-quality.md:Dead | PARTIAL (no unused import rule)
Atomic file writes           | YES       | (none)               | YES: need file-operations.md
...
```

### 2b: Pattern Scan

Use detection scripts from `references/pattern-detection.md` (pick ones matching the project's stack) to find what actually exists in the codebase. This grounds the analysis in reality rather than theory.

For each pattern found, check: "Is there a rule that would have prevented/guided this?"

### 2c: Drift Detection

**First, check CLAUDE.md for `@imports` that defeat path-scoping.** Run:

```bash
rg -n '^[^`]*@\.claude/rules/' CLAUDE.md 2>/dev/null
```

Any match is a bug: the imported rule file is force-loaded every turn, ignoring its `paths:` frontmatter. Flag each one for removal in Phase 3 (replace the `@path/file.md` reference with a plain `file.md` mention so the path-scoping kicks in).

**Then check inventory bloat.** For each rule file, scan for content that's derivable from the codebase rather than rule content:

- ASCII directory trees (covered by `ls -F`)
- Tables of controllers / models / services / partials / routes (covered by `ls` of the relevant dir)
- Base-class method dumps (covered by reading the base class file)
- Helper function lists (covered by reading `helpers.php` or equivalent)
- URL route enumerations (covered by reading `routes.php` or equivalent)
- Token/color/spacing references (covered by reading the tokens file)

Mark every such block for replacement with a one-line pointer in Phase 3.

**Then existing reference checks.** For every file:line reference in existing rules:
- Does the file still exist?
- Does the referenced line still contain what the rule describes?
- Has the file been moved or renamed?

**Detect probable renames** (file gone but a sibling with a close name exists):

```bash
# For each missing file path referenced by a rule, look for a similar name
# in the same directory. Adjust the basename match to your tolerance.
for ref in $(rg -oN 'Reference:\s*`?([^`\s]+\.(py|php|ts|tsx|js|go|rb|java))' \
                 -r '$1' .claude/rules/ 2>/dev/null | sort -u); do
    if [ ! -f "$ref" ]; then
        dir=$(dirname "$ref")
        base=$(basename "$ref" | sed 's/\.[^.]*$//')
        # Candidates in the same directory whose name shares a token with the old name
        candidates=$(find "$dir" -maxdepth 1 -type f 2>/dev/null \
                     | grep -iE "$(echo "$base" | tr '_-' '|')" || true)
        echo "DRIFT: $ref missing; candidates: ${candidates:-none}"
    fi
done
```

For every pattern described in rules:
- Does the codebase still use this pattern?
- Has the convention changed since the rule was written?

For every path scope in rule frontmatter:
- Do files matching this glob still exist?
- Are there new directories that should be included?

### 2d: Redundancy Check

- Are any rules duplicated across files?
- Do any rules contradict each other?
- Are there rules that duplicate what's already in `CLAUDE.md`?
- Are there rules that duplicate what's in the user's global `~/.claude/CLAUDE.md`?

---

### 2e: Context Budget

An unconditional rule file is paid for on every turn of every session; a path-scoped one is paid for only when it fires. This phase measures the bill per file, then decides file by file whether the lever is scoping or compression. The two are not interchangeable. Scope covers both `.claude/rules/` and every `CLAUDE.md` that loads for the project; running this skill in any project should optimize both in one pass.

**Measure the rule files first.** Size and frontmatter class, largest first:

```bash
for f in .claude/rules/*.md; do
    [ -f "$f" ] || continue
    if head -n 15 "$f" | rg -q '^paths:'; then cls=scoped
    elif head -n 15 "$f" | rg -q '^alwaysApply:'; then cls=cursor-legacy
    else cls=unscoped; fi
    printf '%7d  %-8s  %s\n' "$(wc -c < "$f")" "$cls" "$f"
done | sort -rn
```

`unscoped` means the file carries no `paths:` and therefore loads every session: "Rules without a `paths` field are loaded unconditionally", per the Claude Code memory docs, and directly observable in any session whose system prompt inlines its frontmatter-less rule files verbatim. `cursor-legacy` flags an `alwaysApply:` key, a Cursor rules convention that Claude Code neither documents nor honors (zero mentions across the memory and settings docs; the installed 2.1.220 binary contains the literal `alwaysApply: false` exactly once, a template artifact). Such a file is unconditional because it lacks `paths:`, not because of the marker. Report the marker as legacy noise, and never add one to mark a file unconditional. Flag every file over ~2KB that is not `scoped`. A flag is a question to answer with the decision rule below, not a verdict.

**Then verify the globs actually resolve.** Size and class are only half the picture: a scoped rule whose glob matches nothing never loads, and in an audit it reads as covered, which is strictly worse than having no rule. Run `scripts/verify-rule-globs.js`, giving the script its path inside this skill directory while keeping the **project root** as the working directory, since it reads `.claude/rules/` and shells out to `git ls-files` relative to wherever it is invoked:

```bash
cd /path/to/project && node /path/to/this/skill/scripts/verify-rule-globs.js
```

It resolves the same minimatch Claude Code bundles (the 2.1.220 binary carries minimatch's own `exports.GLOBSTAR` and `Minimatch` class), tests every `paths:` entry against `git ls-files` (falling back to an on-disk check for a literal path, so a gitignored-but-real target like `.env` isn't called dead), prints a match count per pattern, and exits 1 when a rule file has no live pattern at all. `--for <path>` answers the reverse question: which rules load when a given file is read. After installing or updating this plugin, run `npm ci --prefix /path/to/this/skill/scripts` once. The pinned dependency stays beside the skill, not in your global npm installation. Never substitute a shell glob for this check. `**` semantics differ between shells and minimatch, so a pattern you rewrote to make the shell happy proves nothing about the real matcher.

**Then measure what ONE read actually costs, and optimize that number rather than per-file size.** Per-file bytes are a proxy. The bill actually paid is the sum of every rule that loads together for one file, and rules that individually look reasonable stack into something that isn't. Take a baseline before changing anything, on five or six files chosen to span the tree (one per subsystem, plus one in the purest layer the architecture has):

```bash
for p in <representative files>; do
    printf '%9s  %s\n' \
      "$(node /path/to/this/skill/scripts/verify-rule-globs.js --for "$p" | rg -o 'read: [0-9]+' | rg -o '[0-9]+')" "$p"
done
```

Then read the baseline for two things:

- **The floor**: what EVERY file pays. A rule scoped to `src/**/*.py` is effectively unconditional for that language, so it belongs in the floor even though the measurement calls it `scoped`. Sum the floor and ask, rule by rule, whether a file in the project's purest layer can actually violate it. On one hex-architecture project the answer was no for a 16KB build rule that loaded on a `core/` file forbidden from doing I/O at all.
- **The stack**: which files pay several large rules at once. Those are where a split pays, and they are usually leaf adapters, which is also where most edits land.

Re-run the identical command after the changes and report before/after per file. A total-bytes figure across the directory is the wrong headline: a good pass often moves total bytes barely at all, because nothing was deleted, it just stopped loading where it does not apply.

**What `paths:` actually does** (verified 2026-07-27, three controlled runs with an `InstructionsLoaded` hook). These were measured, not assumed. Don't re-derive them:

- A path-scoped rule loads when Claude **reads** a file matching the glob. The hook logs `load_reason: path_glob_match` with a `trigger_file_path`.
- It does **not** load when Claude **writes** a matching file without having read one first. No warning, no log line, no rule.
- When the read is delegated to the `Explore` subagent, the rule loads into **that subagent**, not the main thread. The main thread then writes the file without it. Claude auto-delegates reads to `Explore` routinely, so this is the common case, not a corner case.

Three more, measured the same way on 2026-07-28 against 2.1.220 by reading back the `globs` array the hook reports:

- **One inline `paths:` value may hold several comma-separated globs, and Claude Code splits them itself.** `paths: a/**/*.php, b/**/*.php` was reported back as two patterns; the space after the comma is optional. This is a normal authoring form. Never rewrite it into a YAML block list, and never report it as a dead glob.
- **Braces are expanded before matching.** `app/{foo,bar}/**/*.php` came back already expanded to two patterns, so a comma inside braces is not a separator. Expand braces first, then split on commas, which is the order `verify-rule-globs.js` uses.
- **A rule loads when ANY one of its patterns matches.** A dead pattern beside a live one is dead weight worth pruning, not an unreachable rule. Only a file whose patterns ALL match nothing truly never loads; that is the case the report must escalate.

An earlier `verify-rule-globs.js` pushed the whole inline value as one glob and so reported every multi-pattern rule file as dead. On the first project it ran against that was 9 of 16 files, all of them working. If this check ever reports a wall of dead globs, suspect the parser before the rules.

**Decision rule.** For each flagged file, pick a column. Don't blanket-recommend `paths:`.

| Scope it with `paths:` when | Keep it unconditional when |
|-|-|
| It is reference material: an API shape, a schema, a convention catalog, a pattern index | It is a prohibition ("never `flush ruleset`", "never force-push to a shared branch") |
| Its absence degrades an answer rather than causing an incident | It is an access inventory or credential map, where absence produces a wrong conclusion ("I have no access to that host") |
| Claude reliably reads a matching file before acting | Its absence causes an incident rather than a worse answer |
| The governed workflow is read-then-edit | The governed workflow mostly **writes** files: new configs, generated scripts, scaffolded modules |

For a file that stays unconditional, the lever is compression, not scoping. Shrink it against the never-strip list below and leave the frontmatter alone. A 6KB prohibition file compressed to 3KB is a real win; the same file scoped with `paths:` is a silent regression.

**There is a third lever, and it beats both when the file covers more than one subsystem: split it.** A file that is already scoped and still large is usually not badly written, it is carrying several disjoint rule sets behind one union of paths, so every file in the union pays for all of them. The tell is a `paths:` list long enough that no single file matches most of it. A measured case: one 62KB rule file carried 28 paths spanning four subsystems, so editing a single adapter in one of them loaded all four. Split into four siblings with disjoint paths, that adapter's total dropped 43%.

Test for it: **would a competent reviewer of file X need section S?** Group sections by the answer, and each group with a distinct answer is a file. Splitting when the answer is the same for everything just creates two files that always load together, which is strictly worse than one.

Five rules for doing it without regressing:

- **Never copy the parent's `paths:` onto each child.** Four 15KB files each carrying the parent's 28-path list is a net loss and the single easiest way to make this pass worthless. Each child gets only the files its own rules govern.
- **Keep the original filename for the largest or most-referenced piece, and split the others out as siblings.** Renaming is what breaks things: check inbound references first with `rg -l '<name>\.md'` across the repo. One file had roughly 14 references including a comment in a source file, a CHANGELOG entry and three decision records, so it kept its name and the bulk moved to `<name>-<topic>.md` beside it.
- **Prefer flat prefixed names over subdirectories.** The docs say `.claude/rules/` is discovered recursively, so `rules/<subsystem>/input.md` works. But other tooling may not walk it: check for a flat `rules/*.md` glob in `AGENTS.md`, preflight scripts and CI before nesting. `<subsystem>-input.md` sorts identically and nothing has to be taught about it.
- **A prohibition does not split by path even when its subject does.** Split off the reference half and leave the rule half broad. A subprocess rule set divides cleanly into "never `shell=True`, always bound the wait" (governs the call site that does not exist yet, so it stays repo-wide) and "here are the 17 existing call sites and why each is safe" (reference material, read before touching one, so it scopes to those 17 files).
- **Say in `CLAUDE.md` that the globs are now narrow.** Narrower scoping has a real maintenance cost: a new file in a subsystem gets no rule until it is added to the matching `paths:`. That trade is worth making and worth writing down, next to the `--for` command that checks it.

**`CLAUDE.md` is unconditional by construction.** It can't be path-scoped, so compression is its only lever. Discover and measure every one that loads for this project, in lines, which is the unit the official guidance uses:

```bash
d=$PWD
{ while :; do
    for f in "$d/CLAUDE.md" "$d/CLAUDE.local.md" "$d/.claude/CLAUDE.md"; do
      [ -f "$f" ] && printf '%5d  %s\n' "$(wc -l < "$f")" "$f"
    done
    [ "$d" = "/" ] && break
    d=$(dirname "$d")
  done
  [ -f "$HOME/.claude/CLAUDE.md" ] && printf '%5d  %s\n' "$(wc -l < "$HOME/.claude/CLAUDE.md")" "$HOME/.claude/CLAUDE.md"
} | awk '!seen[$2]++'
```

Ancestor directories count. A `CLAUDE.md` three levels up loads for this project too and is the one people forget. Flag every file over 200 lines, per the target in the Claude Code memory docs (`docs.claude.com/en/docs/claude-code/memory`): "target under 200 lines per CLAUDE.md file. Longer files consume more context and reduce adherence." **Adherence is the point.** An oversized `CLAUDE.md` doesn't just cost tokens, it dilutes every instruction inside it, so the trim is a correctness fix worth proposing even when context is plentiful. The user's global `~/.claude/CLAUDE.md` belongs in the measurement because it loads, but it isn't this project's file: report it and get an explicit go before editing it.

**What to cut, what to keep in `CLAUDE.md`.** The same criteria `/doctor` applies:

| Cut it: Claude can derive it | Keep it: Claude cannot derive it |
|-|-|
| Directory layouts, file inventories | Pitfalls and gotchas, especially ones that already bit |
| Dependency lists | The rationale behind a convention |
| Generic architecture overviews | Conventions that differ from the tool's default |
| Restated framework defaults and language idioms | Exact commands, with their flags |
| Anything one `ls`, one manifest read, or one `rg` answers | Access pointers: hosts, tokens, where a credential lives |
| | Hard prohibitions ("never X") |

A cut block isn't always a deletion. If it is a real constraint that only applies to part of the tree, move it into a scoped rule file instead of dropping it; that is the trim and the rules analysis paying for each other.

**Check derivability, don't assume it.** Before cutting any block, prove the codebase actually says the same thing:

- Documented directory layout: run `ls` on that directory and compare it to the text.
- Dependency list: read `package.json` / `composer.json` / `pyproject.toml` / `go.mod` and compare.
- "This project uses pattern X": `rg` for it and confirm it's the dominant pattern rather than an aspiration.

A block that reproduces reality is a safe cut. A block that turns out **stale or wrong** is not: that's a separate finding. Report it as drift (Phase 2c) with the correct value, then either fix it in place or cut it with the correction recorded. Never silently delete something because it disagreed with the codebase. The disagreement is usually the most interesting thing the audit found.

**Complement `/doctor`, don't duplicate it.** Claude Code 2.1.206+ already proposes `CLAUDE.md` trims in `/doctor`. This skill's value is doing the trim in the same pass as the rules analysis, so a cut block can land in a scoped rule file instead of being lost, and doing it under the never-strip list and verify step below, which `/doctor` doesn't enforce. If `/doctor` has already trimmed the file, measure and move on rather than re-cutting.

**Rules that duplicate a skill: excise the section, keep the file.** Phase 2d already checks duplication against sibling rule files, `CLAUDE.md`, and the global `~/.claude/CLAUDE.md`. Extend the same check to installed skills (`.claude/skills/*/SKILL.md` and `~/.claude/skills/*/SKILL.md`), because a rule file often carries a catalog a skill has since absorbed. The typical shape: a project's `voice.md` holds a banned-vocabulary list that a writing skill already owns, while the rest of that file is a legitimate per-project overlay that has to stay. The output is an excision plus a one-line pointer ("banned vocabulary: `<skill-name>`"), not a deleted file. Delete the whole file only when every section of it is duplicated.

**Multi-step procedures are skill-migration candidates, with one caveat.** A rule file that has grown a numbered runbook (deploy sequence, release checklist, migration steps) is carrying procedure in a slot meant for constraints, and procedure belongs in a skill: loaded on demand, priced only when invoked. The caveat decides it: a skill fires on its `description`, and a vague `description` silently never fires. A rule that costs 3KB every turn but always applies beats a skill that costs nothing and never loads. Propose the migration only when you can write a `description` naming the concrete triggers (the tool, the command, the phrasing the user actually types), and have the rule file keep a one-line pointer to the skill.

**Never-strip list.** Any compression proposed anywhere in this skill, in rule files and in `CLAUDE.md` alike, must preserve, verbatim:

- Incident dates and version numbers
- Hostnames and full IP addresses. Never abbreviate `198.51.100.40` to `.40`
- Exact commands, including their flags
- File paths and config keys
- The sentence explaining why the rule exists

**Verify every compression.** Snapshot before editing, then confirm each identifier survived. Same procedure for a rule file and for `CLAUDE.md`, only `T` changes:

Match identifier **shapes**, never backtick pairs. A ``​`...`​`` pattern looks like the obvious way to catch commands and paths, and it silently breaks: any literal backtick inside a code span (a shell regex such as ``^[a-zA-Z0-9 !#$%&'*+-.^_`|~=/]*$`` contains one) throws off the pairing for the rest of the line, so tokens after it are never extracted and the check reports losses that did not happen. Unwrapping prose makes this worse, because one paragraph is now one long line and the mis-pairing cascades across all of it. This produced a full page of false LOST lines on a real run.

```bash
T=.claude/rules/foo.md                     # or CLAUDE.md
S=/tmp/trim-$(basename "$T")               # per-file name; distinct per file in a batch
git show "HEAD:$T" > "$S.pre"              # or cp "$T" "$S.pre" if not committed
# after editing, extract shapes from the pre-image and substring-check each one:
rg -oN -e '[0-9]{1,3}(\.[0-9]{1,3}){3}(:[0-9]+)?' \
       -e '[0-9]{4}-[0-9]{2}-[0-9]{2}' \
       -e '[A-Za-z0-9_*.-]+\.(lan|net|com|local|io|dev)\b' \
       -e '(/[A-Za-z0-9._-]+){2,}' \
       -e '[A-Za-z0-9_-]+\.(yml|yaml|json|conf|sh|py|md|service|sock)\b' \
       -e '\b[A-Z][A-Z0-9]{2,}(_[A-Z0-9]+)+\b' \
       -e '\b[a-z]+([A-Z][a-z0-9]+)+\b' \
       "$S.pre" | sort -u > "$S.ids"
while IFS= read -r id; do rg -qNF -- "$id" "$T" || echo "LOST: $id"; done < "$S.ids"
```

Any `LOST:` line is a regression: restore the identifier or abandon that edit. Two refinements learned from running this for real. First, on a `CLAUDE.md` trim a missing identifier does not always mean data loss: if the block was de-duplicated into a file the session still loads (a rule file, or the doc that CLAUDE.md names as source of truth), the identifier moved rather than died. So re-check each hit against the whole repo, `rg -F -- "$id" .claude/rules/ docs/`, and only treat it as a loss when it survives nowhere the session will read. Second, an identifier that genuinely vanished usually means a block you classified as derivable was carrying an access pointer or an exact command in passing, which is precisely what the never-strip list protects.

The pattern cannot check the last item on that list, so read the diff yourself as a separate pass and confirm the "why this exists" sentence is still there. A compressed rule that no longer says which incident produced it reads as unmotivated and gets deleted by the next audit.

When several files were produced from one pre-image, check each identifier against the **whole set** at once (`rg -qNF -- "$id" newA.md newB.md newC.md`), not against one file, or every relocated line reports as lost.

**After a split, run a second, different check: per-path fit.** The identifier sweep proves each identifier survives somewhere in the union of the new files. It does NOT prove a rule still loads for the file it governs, and those are separable: a rule can pass the grep and stop firing. Text that moved into a sibling whose `paths:` does not include the governed file is invisible exactly where it was needed. The union stays intact, so nothing looks wrong.

For every path in the ORIGINAL file's `paths:`, confirm the split still reaches it:

```bash
git show HEAD:.claude/rules/<original>.md | sed -n '/^paths:/,/^---$/p' \
  | rg -o '"[^"]+"' | tr -d '"' | while read -r p; do
    [ -e "$p" ] || { printf '%-55s MISSING FILE\n' "$p"; continue; }
    loaded=$(node /path/to/this/skill/scripts/verify-rule-globs.js --for "$p" | rg -o '<prefix>[a-z-]*\.md' | sort -u | tr '\n' ' ')
    printf '%-58s %s\n' "$p" "${loaded:-NONE}"
  done
```

`NONE` is a hard failure. But a single-entry row needs judgement too, and that is where the real miss hides: read what the file actually declares and ask whether the sibling it landed in covers it. On one split, a port file declaring eight interfaces matched only the first child, while the rules describing four of those interfaces had moved to the second; the fix is one path entry, and nothing else in the audit would have surfaced it.

Two more things this pass should report rather than hide:

- **Read the glob output unfiltered at least once.** `verify-rule-globs.js` exits non-zero only when a file has NO live pattern, so a file can carry several dead patterns beside one live one and still pass. After authoring dozens of new path entries, grep the full output for `0 files` instead of trusting the exit code.
- **A per-file cost that goes UP is not automatically a regression.** When a narrower glob newly includes a file that the old union missed, that file starts paying a rule that genuinely governs it, which is a coverage fix. Report it as one, with the reason, rather than quietly reverting to keep the numbers clean.

---

## Phase 3: Generate Changes

Based on the coverage analysis, determine what needs to change. Follow these priorities:

### Priority 1: Fix Drift (accuracy)
- Update stale file:line references
- Remove rules for patterns the project no longer uses
- Update path scopes to match current directory structure

### Priority 2: Restore Path-Scoping (efficiency)
- Replace any `@.claude/rules/foo.md` reference in `CLAUDE.md` with a plain mention (`see services.md`). The `@` prefix forces the file into context every turn and defeats the `paths:` frontmatter the user wrote.
- Verify each rule file actually has `paths:` (or a deliberate `alwaysApply: true`). Add scoping where missing.

### Priority 2b: Split Multi-Subsystem Files (efficiency)
- Any scoped file that is still large after the scoping pass, and whose `paths:` list no single file matches most of, is a split candidate. Apply the "would a reviewer of file X need section S?" test and the five rules above.
- This is usually the biggest single win available, and it is invisible to a size-only audit: the files look fine individually, and only the per-read baseline shows them stacking.
- The split is not a rewrite. Move sections whole, preserve wording, and let each child keep the author's voice. Compression, if any, is a separate decision per section.
- Update `CLAUDE.md`'s rules table in the same pass. A split makes that table wrong immediately, and the table is how the next reader (and the review skill) maps scope to file.

### Priority 3: Strip Inventory Bloat (efficiency)
- Replace directory trees with `Layout: ls dir/`.
- Replace controller/model/service/partial/route inventories with `Inventory: ls path/`.
- Replace base-class method dumps with `Read app/path/Base.php for the API`.
- Replace helper/token/route enumerations with a one-line pointer to the source-of-truth file.
- Keep the rule (the "must"/"never"); drop the catalog. Inventories rot the moment files are added or renamed; pointers don't.

### Priority 4: Fill Gaps (completeness)
- Add missing rules for relevant uncovered dimensions
- Prefer adding to existing rule files over creating new ones
- New rules must be written as instructions, not code examples

### Priority 5: Reduce Redundancy (clarity)
- Remove duplicates (keep in the most specific file)
- Collapse near-identical rules into one clear statement
- Remove rules that restate what the framework already enforces
- **Exception: path-visibility duplication is intentional.** Before deleting a "duplicate," check the two files' `paths:` scopes. If the source-of-truth file is scoped to a directory that never loads when editing the file type where the rule is actually violated, the restatement is load-bearing, not noise. (Real case: a "no inline event handlers" rule in `js-standards.md` scoped `public/js/**` never loaded while editing PHP views, where every regression happened; the rule had to be restated in the views-scoped file with a cross-reference.) Keep both; ensure the restatement carries a one-line "source of truth: X" pointer.

### What NOT to Change
- The user's organizational structure (number of files, file names, section headings)
- Working rules that are correctly scoped and accurate
- Project-specific conventions the user documented from experience
- The level of detail the user chose (some projects have dense rules, some have sparse ones)

---

## Phase 4: Apply

### For Existing Rules (`--full` or `--sync`)

Edit rule files using preservation tags:

| Tag | Meaning | Action |
|-|-|-|
| **KEEP** | Accurate, well-scoped, no issues | Don't touch |
| **UPDATE** | Stale reference or path | Minimal targeted edit |
| **ADD** | Missing rule for a real gap | Append to relevant section |
| **REMOVE** | Covers a pattern that no longer exists | Delete the rule |
| **MOVE** | Rule in wrong file or wrong scope | Move to correct file |

For CLAUDE.md:
- Update the rules reference table if rule files were added/removed
- Fix any stale file paths or line numbers
- Don't restructure or rewrite sections that aren't about rules

### For New Rules (no existing `.claude/rules/`)

**First: pick the organization pattern.** Three patterns exist. Default to the first unless you have a specific reason:

| Pattern | When to use | Example files |
|-|-|-|
| **By Domain** (default) | Most projects. Rules grouped around the concern they protect. | `security.md`, `database.md`, `code-quality.md`, `web-layer.md` |
| **By Layer** | Architecture is strictly layered (controllers → services → repositories) and layer-specific rules actually differ. Don't use if "layer" is notional. | `controllers.md`, `services.md`, `repositories.md` |
| **By Platform** | Monorepo with distinct stacks that share a repo (e.g. backend PHP + mobile Dart + web TS). Each stack has its own conventions. | `be-security.md`, `mo-components.md`, `we-architecture.md` + one shared `code-quality.md` |

Pick one and stick to it: don't mix (`security.md` + `controllers.md` + `be-api.md` is noise).

Then determine rule files based on what the project actually needs. Common patterns across stacks:

**Python projects:**
- `security.md`: scoped to `src/**/*.py`
- `error-handling.md`: scoped to `src/**/*.py`
- `code-quality.md`: scoped to `src/**/*.py`
- Framework-specific (e.g., `web-layer.md` for Streamlit/Flask/Django): scoped to web directories
- Domain-specific if the project has distinct subsystems (e.g., `backup-engine.md`, `api.md`)

**PHP projects:**
- `security.md`: scoped to source directories
- `architecture.md`: typically `alwaysApply: true` for service-layer/MVC rules
- `database.md`: scoped to repository/model files
- `php-standards.md`: scoped to all PHP source
- Layer-specific files for each architectural layer (controllers, processors/services, repositories)
- `frontend.md`: scoped to view/template/JS/CSS files

**JavaScript/TypeScript projects:**
- `security.md`: scoped to `src/**/*.{ts,tsx}`
- `architecture.md`: scoped to source directories
- `api-integration.md`: scoped to API/service files
- `components.md`: scoped to component directories
- `state-management.md`: scoped to state/store files

**Multi-platform projects:**
- Prefix with platform: `be-security.md`, `mo-components.md`, `we-architecture.md`
- One cross-platform file for shared rules: `code-quality.md`

Whatever the stack, two things hold for a freshly generated set:

- **Exactly one always-on `security.md`:** either `alwaysApply: true` or scoped to all source. Security review applies to every file; it's the one dimension that shouldn't wait for a path match.
- **Domain rule files derived from the Phase 1 DOMAIN VOCABULARY**, not just the generic stack list above. If the profile surfaced distinct subsystems (scraper, consolidator, scorer, feature-flag registry) or a rich enum/status vocabulary, generate a file (or a dedicated section) per subsystem, scoped to the files that implement it. These are usually the highest-value rules; don't ship a set that's all generic and zero domain.

Write rules as declarative instructions. Each rule is one bullet point. Group related rules under `##` headings. No code blocks longer than a one-liner. Use `Reference: path/file` when showing a pattern would be clearer than describing it. Where a recurring create-flow exists (adding a model/content-type/migration/endpoint), close the relevant file with a numbered "Adding a new X" checklist whose steps point to the sibling rule files; see `references/rule-style-guide.md` "Rule Shapes."

### 4d: Review generated rules against anti-patterns

Before declaring Phase 4 done, scan every file you created or touched against the anti-patterns in `references/rule-style-guide.md` (the "Anti-patterns" section near the bottom). Flag and fix any of these:

- **Tutorial file**: prose explaining *what* the framework does instead of *what this project does with it*.
- **Kitchen sink**: one file covers unrelated concerns (security + style + performance mixed together).
- **Aspirational rules**: rules that describe ideal behavior the codebase doesn't actually follow. Either remove or fix the code first.
- **Duplicates**: same rule appears in two files (or restates something already in `CLAUDE.md` or the global `~/.claude/CLAUDE.md`).
- **Stale references**: file paths or line numbers that no longer match the code (drift).
- **Unscoped when it should be scoped**: `alwaysApply: true` on a rule file that only applies to one part of the tree.
- **Inventory dump**: directory trees, lists of controllers/models/services/partials/routes, base-class method signatures, helper indexes, design-token catalogs. Replace with one-line pointers (`Inventory: ls path/`, `Methods: read path/Base.php`).
- **`@import` trap**: `CLAUDE.md` references a rule file with `@.claude/rules/foo.md`. That force-loads it every turn and breaks `paths:` scoping. Replace with a plain mention (`see foo.md`).
- **All-generic, zero-domain.** The set covers security/quality/architecture but encodes none of the project's own enum semantics, status machines, subsystem contracts, or cross-entity invariants (Phase 2a domain dimensions). A rule set a reviewer could have written without reading this codebase is under-covering it.
- **Missing always-on security.** No `security.md`, or it's path-scoped so narrowly it doesn't load on most edits. Security should be the one always-on file.

If you fix any, note them in the Phase 5 report so the user can see why.

After creating rules, add a reference section to CLAUDE.md:

```markdown
## Coding Rules (`.claude/rules/`)

Path-scoped rules auto-loaded when editing matching files.

| Rule file | Scope | Covers |
|-|-|-|
| `security.md` | `src/**/*.py` | Subprocess safety, input validation, secrets, XSS |
| ... | ... | ... |
```

---

## Phase 5: Report

Present the results:

### For `--audit-only`

```
## Rules Audit Report

### Coverage: X/Y relevant dimensions covered (Z%)

### Drift Found
| File | Rule | Issue |
|-|-|-|
| security.md | "ConfigManager handles YAML" | ConfigManager renamed to SettingsManager |
| web-layer.md | path scope `src/web/**` | New directory src/dashboard/ not covered |

### Gaps Found
| Dimension | Severity | Suggested Rule |
|-|-|-|
| Atomic file writes | HIGH | "Critical files must be written atomically via temp + rename" |
| Missing timeouts | MEDIUM | "All subprocess calls must include timeout=" |

### Redundancies
| Rule | Appears in | Keep in |
|-|-|-|
| "Use parameterized queries" | security.md, database.md | database.md |

### Context Budget
| File | Size | Class | Verdict |
|-|-|-|-|
| CLAUDE.md | 412 lines | always | TRIM to <200: §Layout + §Dependencies derivable (checked via `ls` and `package.json`), §Deploy steps move to `deploy.md` |
| ../CLAUDE.md (ancestor) | 240 lines | always | TRIM: loads for this project, user unaware |
| access-inventory.md | 5158 B | unscoped | KEEP unconditional (prohibition + credential map), compress to ~3k |
| api-shapes.md | 6120 B | unscoped | SCOPE to `src/api/**` (reference material, always read before edit) |
| voice.md | 3277 B | unscoped | EXCISE §3 (duplicates an installed writing skill), keep the rest |

Loaded every turn: N bytes across M files.

Looked derivable but wasn't (reported as drift, not cut):
- CLAUDE.md §Layout claimed `src/api/v1/`, actual is `src/api/` since the v2 merge

### Recommendations
- Add file-operations.md with 3 rules for atomic writes, TOCTOU, locking
- Update path scope in web-layer.md to include src/dashboard/
- Remove duplicate SQL injection rule from security.md (covered in database.md)
```

### For `--full` or `--sync` (including new rule creation)

```
## Rules Optimization Report

### Changes Applied

#### Updated Files
| File | Change | Reason |
|-|-|-|
| security.md | Updated 1 stale reference | ConfigManager → SettingsManager at line 15 |
| web-layer.md | Expanded path scope | Added src/dashboard/** to cover new directory |

#### New Files
| File | Scope | Rules added | Covers |
|-|-|-|
| file-operations.md | src/core/**/*.py | 3 rules | Atomic writes, TOCTOU, locking |

#### Removed
| File | Rule | Reason |
|-|-|-|
| security.md | "Use parameterized queries" | Duplicate of database.md rule |

### Coverage After: X/Y relevant dimensions (Z%)

### Context Budget After: N bytes loaded every turn across M unconditional files (was P bytes / Q files). CLAUDE.md X lines (was Y, target <200)

### Review Skill Alignment
- Rules now cover N of M checks from the review skill
- Remaining uncovered checks are runtime-only (can't be prevented by rules)
```

---

## Reference Files

Read as needed during analysis:

| File | When to Read | Contains |
|-|-|-|
| `references/review-dimensions.md` | Phase 2a: dimension mapping | Full taxonomy of review categories |
| `references/pattern-detection.md` | Phase 2b: pattern scanning | Detection scripts per language/stack |
| `references/rule-style-guide.md` | Phase 4: writing new rules | Format, scoping, and style conventions |
| `scripts/verify-rule-globs.js` | Phase 2e: scoping check, and after writing any new `paths:` | Runnable. Invoke by its path in this skill directory with the project root as the working directory. Tests every glob with the minimatch Claude Code bundles; exits 1 on a dead glob. `--for <path>` lists which rules load for one file. After install or update, run `npm ci --prefix /path/to/this/skill/scripts` once. |

interface:
  display_name: "Rules Optimizer"
  short_description: "Audit rules against what the code actually does"
policy:
  allow_implicit_invocation: false

# Shared Reference Files

The following files in this directory are **duplicated** from `a-review-optimizer/references/`:

- `review-dimensions.md`: taxonomy of review categories
- `pattern-detection.md`: detection script library by language

Both `a-rules-optimizer` and `a-review-optimizer` read these. Each skill keeps its own copy so it stays self-contained and can be installed on its own.

## When editing one, update the other

If you change `review-dimensions.md` or `pattern-detection.md` here, **update the copy in `a-review-optimizer/references/` in the same commit**. The two copies must stay identical.

Quick parity check, run from this skill's directory:

```bash
diff -q references/review-dimensions.md ../a-review-optimizer/references/review-dimensions.md
diff -q references/pattern-detection.md ../a-review-optimizer/references/pattern-detection.md
```

Both should print nothing. If they diverge, pick the authoritative version and sync.

# Pattern Detection Script Library

Use this during the pattern-inventory phase and when generating preflight scripts. Pick the scripts relevant to the project's stack.

All scripts are designed to be safe (|| true suffix, head limits on output) and produce file:line locations.

## Universal Detection Scripts

### Credentials & Secrets
```bash
# Hardcoded secret assignments (exclude env/config accessors)
rg -n '(password|api_key|secret_key|api_secret|token|private_key)\s*=\s*["\x27][^"\x27]{4,}' \
  --type-add 'code:*.{py,php,js,ts,rb,go,java}' -t code src/ \
  | grep -v 'getenv\|os.environ\|get_setting\|\.get(' || true

# Files that shouldn't be tracked
git ls-files -- '*.env' '.env.*' '*.key' '*.pem' 2>/dev/null \
  | grep -v '.example' | grep -v '.sample' || true
```

### Error Handling
```bash
# Bare except (Python)
rg -n '^\s*except\s*:' --type py src/ || true

# catch(Exception) or catch(\Exception) (PHP)
rg -n 'catch\s*\(\s*\\?Exception' --type php src/ || true

# Generic catch(e) (JS/TS)
rg -n 'catch\s*\(\s*\w+\s*\)\s*\{' --type js --type ts src/ || true

# Silent exception handlers (Python: except block with only pass/continue)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text().splitlines()
    for i, line in enumerate(lines):
        if re.match(r'\s*except\b', line):
            # Check next non-blank lines in the except block
            body_lines = []
            base_indent = len(line) - len(line.lstrip())
            for j in range(i+1, min(i+5, len(lines))):
                stripped = lines[j].strip()
                indent = len(lines[j]) - len(lines[j].lstrip())
                if indent <= base_indent and stripped: break
                if stripped: body_lines.append(stripped)
            if body_lines and all(b in ('pass', 'continue', '...') for b in body_lines):
                print(f'{f}:{i+1}: silent except, body is only {body_lines[0]}')
" 2>/dev/null || true

# Empty catch blocks (PHP: brace-balanced scan, catches multi-line)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.php'):
    text = f.read_text(errors='ignore')
    for m in re.finditer(r'catch\s*\([^)]*\)\s*\{', text):
        depth, pos = 1, m.end()
        while pos < len(text) and depth > 0:
            if text[pos] == '{': depth += 1
            elif text[pos] == '}': depth -= 1
            pos += 1
        body = text[m.end():pos-1].strip()
        line = text[:m.start()].count('\n') + 1
        if not body or re.fullmatch(r'(//[^\n]*|\s)*', body):
            print(f'{f}:{line}: empty catch block, error discarded')
" 2>/dev/null || true

# Empty catch blocks (JS/TS: brace-balanced scan) + log-only promise .catch
python3 -c "
import re, pathlib
for ext in ('*.js', '*.ts', '*.jsx', '*.tsx'):
    for f in pathlib.Path('src').rglob(ext):
        text = f.read_text(errors='ignore')
        for m in re.finditer(r'catch\s*(\([^)]*\))?\s*\{', text):
            depth, pos = 1, m.end()
            while pos < len(text) and depth > 0:
                if text[pos] == '{': depth += 1
                elif text[pos] == '}': depth -= 1
                pos += 1
            body = text[m.end():pos-1].strip()
            line = text[:m.start()].count('\n') + 1
            if not body or re.fullmatch(r'(//[^\n]*|\s)*', body):
                print(f'{f}:{line}: empty catch block, error discarded')
" 2>/dev/null || true
rg -n '\.catch\s*\(\s*(\(\s*\)|\(?\w*\)?)\s*=>\s*\{?\s*\}?\s*\)|\.catch\s*\(\s*console\.(log|error)\s*\)' --type js --type ts src/ || true

# Error suppression operators (PHP)
rg -n '@\s*(file_get_contents|file_put_contents|unlink|fopen|mkdir|rmdir|copy|rename|include|require|mysqli_|json_decode|simplexml_|\$)' --type php src/ || true
rg -n 'error_reporting\s*\(\s*0\s*\)' --type php src/ || true

# Fail-open except (Python): error branch returns a success-looking default with no raise/log
python3 -c "
import re, pathlib
DEFAULTS = re.compile(r'return\s+(None|True|False|\[\]|\{\}|0|[\"\x27][\"\x27])\s*(#.*)?$')
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text(errors='ignore').splitlines()
    for i, line in enumerate(lines):
        if not re.match(r'\s*except\b', line): continue
        base = len(line) - len(line.lstrip())
        body = []
        for j in range(i+1, min(i+8, len(lines))):
            s = lines[j].strip()
            ind = len(lines[j]) - len(lines[j].lstrip())
            if s and ind <= base: break
            if s: body.append(s)
        if not body: continue
        has_default_return = any(DEFAULTS.match(b) for b in body)
        has_signal = any(re.search(r'raise|log|warn|print', b) for b in body)
        if has_default_return and not has_signal:
            print(f'{f}:{i+1}: fail-open except, returns default without raising or logging')
" 2>/dev/null || true
```

### Code Metrics
```bash
# Files over 300 lines
find src/ app/ lib/ -name '*.py' -o -name '*.php' -o -name '*.ts' -o -name '*.js' 2>/dev/null \
  | xargs wc -l 2>/dev/null | awk '$1 > 300 && !/total$/' | sort -rn || true

# Deep nesting (5+ levels = 20+ leading spaces)
rg -n '^\s{20,}\S' --type-add 'code:*.{py,php,js,ts}' -t code src/ | head -20 || true

# Long functions (Python: def to next def at same/lower indent, >50 lines)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text().splitlines()
    func_start = None
    func_name = ''
    func_indent = 0
    for i, line in enumerate(lines):
        m = re.match(r'^(\s*)def\s+(\w+)', line)
        if m:
            if func_start and (i - func_start) > 50:
                print(f'{f}:{func_start+1}: {func_name}() is {i - func_start} lines')
            func_start = i
            func_name = m.group(2)
            func_indent = len(m.group(1))
    if func_start and (len(lines) - func_start) > 50:
        print(f'{f}:{func_start+1}: {func_name}() is {len(lines) - func_start} lines')
" 2>/dev/null || true

# print/var_dump/console.log in production code
rg -n '\bprint\(' --type py src/ --glob '!*test*' --glob '!*__pycache__*' 2>/dev/null | head -20 || true
rg -n '\bvar_dump\(|\bdd\(' --type php src/ 2>/dev/null | head -20 || true
rg -n '\bconsole\.(log|debug)\(' --type js --type ts src/ --glob '!*test*' 2>/dev/null | head -20 || true

# Commented-out code (3+ consecutive comment lines with code patterns)
python3 -c "
import re, pathlib
code_pattern = re.compile(r'#\s*(def |class |import |return |if |for |while |print|self\.|=\s)')
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text().splitlines()
    streak = 0
    streak_start = 0
    for i, line in enumerate(lines):
        if code_pattern.match(line.strip()):
            if streak == 0: streak_start = i
            streak += 1
        else:
            if streak >= 3:
                print(f'{f}:{streak_start+1}: {streak} lines of commented-out code')
            streak = 0
    if streak >= 3:
        print(f'{f}:{streak_start+1}: {streak} lines of commented-out code')
" 2>/dev/null || true
```

## Shell Script Detection

Silent failure in bash is the fleet's most common script defect: the happy path works, the unhappy path exits 0. Distinguish state-changing commands (rm, mv, cp, rsync, docker, systemctl) from read-only probes; `|| true` on a probe is deliberate, on a mutation it masks real failures.

```bash
# Missing safety flags in scripts that change state (>20 lines as a proxy)
for f in $(find . -name '*.sh' -not -path '*/node_modules/*' -not -path '*/vendor/*' -not -path '*/.git/*' 2>/dev/null); do
  [ "$(wc -l < "$f")" -lt 20 ] && continue
  head -10 "$f" | grep -q 'set -e' || echo "$f:1: no set -e"
  grep -q 'pipefail' "$f" || echo "$f:1: no pipefail, failures inside pipelines are invisible"
done

# || true or || : on state-changing commands
rg -n '\b(rm|mv|cp|mkdir|rsync|scp|docker|systemctl|crontab|chown|chmod)\b[^|#]*\|\|\s*(true|:)' --glob '*.sh' . || true

# Blanket stderr suppression on state-changing commands
rg -n '\b(rm|mv|cp|rsync|scp|docker|systemctl)\b[^#]*2>\s*/dev/null' --glob '*.sh' . || true

# cd without failure guard (subsequent commands run in the wrong directory)
rg -n '^\s*cd\s+[^&|;]+$' --glob '*.sh' . || true

# mktemp without an EXIT trap in the same file
for f in $(rg -l 'mktemp' --glob '*.sh' . 2>/dev/null); do
  grep -q 'trap.*EXIT' "$f" || echo "$f: mktemp without EXIT trap cleanup"
done

# Opportunistic: real linter when installed (catches quoting, word splitting, and much more)
command -v shellcheck >/dev/null 2>&1 && shellcheck -f gcc -S warning $(find . -name '*.sh' -not -path '*/.git/*') 2>/dev/null | head -30 || true
```

## Hostile Content Detection (reviewer integrity)

MANDATORY in every generated preflight, regardless of stack. These checks defend the AI reviewer itself: a prompt-injection payload in a comment can hijack the agent reading the file, so detection MUST be deterministic. A regex cannot be sweet-talked; the agent can. Never move these checks into an agent prompt.

```bash
# INJ-01: Invisible / bidirectional Unicode (Trojan Source, CVE-2021-42574)
python3 -c "
import pathlib
BAD = {0x200B, 0x200C, 0x200D, 0x200E, 0x200F, 0x2060, 0xFEFF} | set(range(0x202A, 0x202F)) | set(range(0x2066, 0x206A))
EXTS = {'.py', '.php', '.js', '.ts', '.jsx', '.tsx', '.sh', '.md', '.yml', '.yaml', '.json', '.html', '.css', '.sql', '.env', '.txt'}
SKIP = {'.git', 'node_modules', 'vendor', '__pycache__', 'dist', 'build'}
for f in pathlib.Path('.').rglob('*'):
    if f.is_dir() or set(f.parts) & SKIP or f.suffix not in EXTS: continue
    try: text = f.read_text(encoding='utf-8')
    except Exception: continue
    for i, line in enumerate(text.splitlines(), 1):
        hits = sorted({hex(ord(c)) for c in line if ord(c) in BAD})
        if hits:
            print(f'{f}:{i}: invisible/bidi characters {hits}')
" 2>/dev/null || true

# INJ-02: AI-directed instruction phrases in comments/strings/docs
rg -ni 'ignore (all |any )?(previous|prior|above|earlier) (instructions|prompts|rules)|disregard (the |your )?(system|previous|above)|you are now (a|an|in)|new (system )?instructions:|do not (flag|report|mention|include) (this|the following)|(assistant|claude|copilot|gpt|reviewer)[,:]? (please )?(approve|ignore|skip|omit)' \
  --glob '!*.lock' --glob '!node_modules/**' --glob '!vendor/**' . | head -20 || true

# INJ-03: Large base64 blobs in comments (hidden payloads)
rg -n '(#|//|/\*|<!--|;)\s*[A-Za-z0-9+/]{120,}={0,2}' --glob '!*.lock' --glob '!*.min.*' --glob '!*.svg' . | head -10 || true
```

Whitelist note: security tooling, test fixtures, and this file itself legitimately contain INJ-02 phrases. Findings inside `*test*`, `*fixture*`, or the review skill's own tree get dismissed with a reason, not silently skipped.

## LLM Integration Detection (conditional)

Only include these when the gate check finds LLM usage. The greps surface candidate sites; whether the interpolated content is actually untrusted is the agent's judgment call.

```bash
# Gate: does the project call an LLM at all? (empty output = skip the LLM-* group entirely)
rg -l 'import anthropic|from anthropic|import openai|from openai|import ollama|messages\.create|chat\.completions|api\.anthropic\.com|api\.openai\.com' src/ || true

# LLM-01: Prompt-building sites with interpolation (candidate injection points)
rg -n '(prompt|messages|system_prompt|user_content|content)\s*[=:+].{0,50}(f["\x27]|\.format\(|%s|\$\{|\+\s*\w)' --type py --type js --type ts src/ | head -20 || true

# LLM-02: LLM output rendered or executed
rg -n '(response|completion|\.content|message\.content|\.text|output)\w*.{0,60}(innerHTML|dangerouslySetInnerHTML|v-html|st\.markdown|st\.html|eval\(|exec\(|subprocess|os\.system|shell)' src/ | head -20 || true

# LLM-03: API calls inside loops without an iteration bound nearby (cost/DoS)
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    lines = f.read_text(errors='ignore').splitlines()
    for i, line in enumerate(lines):
        if re.search(r'messages\.create|chat\.completions|\.generate\(', line):
            window = lines[max(0, i-15):i]
            in_loop = any(re.match(r'\s*(while|for)\b', w) for w in window)
            bounded = any(re.search(r'range\(|max_|limit|\[:\d', w) for w in window)
            if in_loop and not bounded:
                print(f'{f}:{i+1}: LLM call inside loop with no visible bound')
" 2>/dev/null || true
```

## Python-Specific Detection

### Security
```bash
# shell=True
rg -n 'shell\s*=\s*True' --type py src/ || true

# os.system()
rg -n 'os\.system\(' --type py src/ || true

# Unsafe deserialization
rg -n 'pickle\.loads?\(' --type py src/ || true
rg -n 'yaml\.load\(' --type py src/ | grep -v SafeLoader || true
rg -n '\beval\s*\(' --type py src/ | grep -v 'ast.literal_eval' || true
```

### Subprocess (multi-line aware)
```bash
# subprocess without timeout: MUST use Python for multi-line detection
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    text = f.read_text()
    for m in re.finditer(r'subprocess\.(run|call|check_output|check_call|Popen)\s*\(', text):
        start = m.start()
        depth, pos = 1, m.end()
        while pos < len(text) and depth > 0:
            if text[pos] == '(': depth += 1
            elif text[pos] == ')': depth -= 1
            pos += 1
        call_text = text[m.start():pos]
        if 'timeout' not in call_text:
            line_num = text[:start].count('\n') + 1
            print(f'{f}:{line_num}: subprocess.{m.group(1)}() without timeout=')
" 2>/dev/null || true

# String interpolation in subprocess args
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    text = f.read_text()
    for m in re.finditer(r'subprocess\.\w+\s*\(', text):
        start = m.start()
        depth, pos = 1, m.end()
        while pos < len(text) and depth > 0:
            if text[pos] == '(': depth += 1
            elif text[pos] == ')': depth -= 1
            pos += 1
        call_text = text[m.start():pos]
        if re.search(r'f[\"\\x27]|\.format\(|%\s', call_text) and 'shell' not in call_text:
            line_num = text[:start].count('\n') + 1
            print(f'{f}:{line_num}: f-string/format in subprocess args')
" 2>/dev/null || true
```

### Type Modernization (Python 3.10+)
```bash
# Old typing imports
rg -n 'from typing import.*(Optional|List|Dict|Tuple|Set|Union)' --type py src/ || true

# Public functions without return type
python3 -c "
import re, pathlib
for f in pathlib.Path('src').rglob('*.py'):
    for i, line in enumerate(f.read_text().splitlines(), 1):
        if re.match(r'^(\s{0,8})def\s+(?!_|test_)\w+\(.*\)\s*:', line) and '->' not in line:
            print(f'{f}:{i}: {line.strip()[:80]}')
" 2>/dev/null | head -30 || true

# Unsafe nested dict access on external data
rg -n '\[.+\]\[.+\]' --type py src/ | grep -v 'test' | head -20 || true
```

### Streamlit-Specific
```bash
# st.rerun() without invalidate() in preceding N lines
python3 -c "
import pathlib
for f in pathlib.Path('src/web').rglob('*.py') if pathlib.Path('src/web').exists() else []:
    lines = f.read_text().splitlines()
    for i, line in enumerate(lines):
        if 'st.rerun()' in line:
            window = lines[max(0,i-10):i]
            if not any('invalidate' in w for w in window):
                print(f'{f}:{i+1}: st.rerun() without invalidate() in preceding 10 lines')
" 2>/dev/null || true

# @st.cache_data without TTL
rg -n '@st\.cache_data' --type py src/ | grep -v 'ttl' || true

# print() in web layer
rg -n '\bprint\(' --type py src/web/ 2>/dev/null | grep -v 'console' || true
```

## PHP-Specific Detection

### Security
```bash
# SQL with string interpolation
rg -n '(query|execute|prepare)\s*\(.*[\$"]' --type php src/ \
  | grep -v 'prepare.*?\?' | grep -v bindParam || true

# Shell execution functions
rg -n '\b(exec|system|shell_exec|passthru|proc_open|popen)\s*\(' --type php src/ || true

# File operations with variables (path traversal risk)
rg -n '(include|require|file_get_contents|fopen|unlink|rmdir)\s*\(\s*\$' --type php src/ || true

# echo/print without escaping
rg -n '(echo|print)\s+\$' --type php src/ | grep -v 'htmlspecialchars\|htmlentities' | head -20 || true

# unserialize on potentially untrusted data
rg -n 'unserialize\s*\(' --type php src/ || true
```

### Architecture
```bash
# env() outside config files (Laravel: returns null when cached)
rg -n '\benv\(' --type php src/ --glob '!config/*' 2>/dev/null | head -20 || true

# Missing $fillable/$guarded on Eloquent models
for f in $(find src/ app/ -name '*.php' 2>/dev/null | xargs grep -l 'extends Model' 2>/dev/null); do
  grep -L 'fillable\|guarded' "$f" && echo "$f: missing \$fillable/\$guarded"
done 2>/dev/null || true
```

### Modernization
```bash
# Legacy array() syntax
rg -n '\barray\s*\(' --type php src/ | head -20 || true

# strpos instead of str_contains (PHP 8.0+)
rg -n 'strpos\s*\(' --type php src/ | head -20 || true
```

## JavaScript/TypeScript-Specific Detection

### Security
```bash
# XSS vectors
rg -n 'innerHTML|outerHTML|document\.write|dangerouslySetInnerHTML|v-html' --type js --type ts src/ || true

# eval / Function constructor
rg -n '\beval\s*\(|new\s+Function\s*\(' --type js --type ts src/ || true

# Hardcoded JWT secrets
rg -n 'jwt\.(sign|verify)\s*\(' --type js --type ts src/ | head -10 || true
```

### TypeScript Quality
```bash
# any type usage
rg -n ':\s*any\b' --type ts src/ | head -20 || true

# Type assertions (hiding real errors)
rg -n '\bas\s+\w' --type ts src/ | head -20 || true

# @ts-ignore without explanation
rg -n '@ts-ignore|@ts-expect-error' --type ts src/ || true

# Non-null assertions
rg -n '\w+!' --type ts src/ | grep -v '!=\|!=' | head -20 || true
```

### React Patterns
```bash
# useEffect without cleanup (missing return in useEffect callback)
# Approximate: agents should verify
rg -n 'useEffect\(' --type ts --type js src/ | head -20 || true

# Missing key prop indicator (map without key)
rg -n '\.map\(' --type ts --type js src/ | head -20 || true

# Large components (>200 lines)
find src/ -name '*.tsx' -o -name '*.jsx' 2>/dev/null | xargs wc -l 2>/dev/null \
  | awk '$1 > 200 && !/total$/' | sort -rn || true
```

### Node.js
```bash
# Sync file operations in non-config code
rg -n 'readFileSync|writeFileSync|existsSync' --type js --type ts src/ \
  | grep -v 'config\|setup\|init' | head -20 || true

# Missing error handling middleware (Express)
rg -n 'app\.(get|post|put|delete|patch)\(' --type js --type ts src/ | head -10 || true
rg -n 'err,\s*req,\s*res,\s*next' --type js --type ts src/ || true
```

## Cross-Reference Detection

These checks compare two sources of truth. They're project-specific by nature; the optimizer should generate them based on what registration patterns the project uses.

### Registration Completeness Template
```bash
# Template: check that all X are registered in Y
# Adapt the patterns to the project's registration mechanism

# Example: Python views registered in PAGE_MAP
python3 -c "
import pathlib, re
view_dir = pathlib.Path('src/web/views')
init_file = view_dir / '__init__.py'
if not init_file.exists(): exit()
init_text = init_file.read_text()
for f in view_dir.glob('*.py'):
    if f.name.startswith('_'): continue
    for m in re.finditer(r'def (render_\w+)', f.read_text()):
        if m.group(1) not in init_text:
            print(f'{f}:{0}: {m.group(1)} not registered in PAGE_MAP')
" 2>/dev/null || true

# Example: PHP routes vs controllers
# Example: React pages vs router config
# Example: CLI commands vs command group registration
```

### Export Completeness Template
```bash
# Template: check that modules export what they define
# Example: Python __init__.py exports
python3 -c "
import pathlib, re
pkg = pathlib.Path('src/web/components')
init = pkg / '__init__.py'
if not init.exists(): exit()
init_text = init.read_text()
for f in pkg.glob('*.py'):
    if f.name.startswith('_'): continue
    for m in re.finditer(r'^def (\w+)|^class (\w+)', f.read_text(), re.MULTILINE):
        name = m.group(1) or m.group(2)
        if name and not name.startswith('_') and name not in init_text:
            print(f'{f}: {name} not exported in __init__.py')
" 2>/dev/null || true
```

# Review Dimensions Taxonomy

Use this during the gap-analysis phase to identify what the existing skill or rule set is missing. For each dimension, check whether it is already covered AND whether the project actually needs it.

Not every project needs every dimension. A CLI tool doesn't need "WebSocket security." A static site doesn't need "N+1 query detection." Skip what's irrelevant, flag what's missing and needed.

## Security Dimensions

### Input Handling
- SQL injection (parameterized queries, ORM safety)
- Command injection (subprocess, exec, system calls)
- Path traversal (filesystem operations with user input)
- XSS (output encoding, template escaping)
- SSRF (URL construction from user input)
- Deserialization (pickle, unserialize, YAML load, eval)
- ReDoS (catastrophic regex backtracking on user input)
- Header injection (CRLF in user-controlled headers)
- Template injection (SSTI in Jinja2, Twig, EJS)

### Credentials & Secrets
- Hardcoded passwords, API keys, tokens in source
- Secrets in logs, error messages, or stack traces
- Secrets committed to git (even in history)
- Encryption keys with wrong file permissions
- Default/weak passwords in config examples
- Credentials in URLs (basic auth in connection strings)

### Authentication & Authorization
- Missing auth checks on endpoints/routes
- Auth without authz (logged in but not permitted)
- Session fixation (no regeneration after login)
- CSRF protection on state-changing requests
- JWT: hardcoded secret, missing expiry, alg:none
- Cookie flags: httpOnly, secure, sameSite
- Timing attacks on password/token comparison
- Password hashing: bcrypt/argon2 vs MD5/SHA1

### Data Protection
- Sensitive data in localStorage/sessionStorage
- PII in logs or analytics
- Missing encryption at rest for sensitive data
- Broad CORS configuration with credentials
- Missing rate limiting on auth endpoints

### Hostile Content (reviewer integrity)
These run as deterministic preflight checks ONLY, never as agent-prompt checks: the AI reviewer is the attack target here, and a hijacked reviewer cannot be trusted to flag its own hijack.
- Invisible or bidirectional Unicode in source (Trojan Source: U+202A-202E, U+2066-2069, zero-width U+200B-200D, U+FEFF)
- Instruction-like phrases aimed at AI tools in comments/strings/docs ("ignore previous instructions", "you are now", "do not flag this")
- Large encoded blobs (base64) in comments with no stated purpose

## LLM Integration Dimensions (conditional)

Only when the project calls an LLM API (imports anthropic/openai/ollama, or hits an LLM HTTP endpoint). Skip entirely otherwise.

### Prompt Injection
- Untrusted content (user input, scraped pages, file contents, third-party API responses) concatenated into prompts without delimiting or marking
- System instructions and untrusted content mixed into the same message role
- LLM output fed into subsequent prompts unsanitized (injection laundering across calls)

### Output Handling
- LLM output rendered as HTML/markdown without escaping
- LLM output parsed as code, SQL, or shell, or passed to eval/exec
- LLM output driving decisions (scores, filters, routing) without schema or bounds validation
- Unbounded loops calling the API (no max iterations, no cost guard)

### Tool Use & Data Exposure
- Tools/functions exposed to the model broader than the task needs
- Secrets or PII included in prompt context
- Model-controlled parameters reaching filesystem, network, or DB operations without validation

## Architecture Dimensions

### Configuration Management
- Hardcoded values that should be configurable
- Config access scattered vs centralized
- Missing defaults for optional config
- Environment-specific logic in business code
- Secrets mixed with non-secret config

### Error Handling Strategy
- Generic catch-all exceptions
- Swallowed exceptions (catch + pass/ignore)
- Empty or log-only catch blocks (catch that discards the error and continues)
- Error suppression operators (@ in PHP, error_reporting(0), blanket 2>/dev/null in shell)
- Fail-open error paths (error branch returns a success-looking default: None, true, empty list)
- Missing cleanup on failure (partial state)
- Missing timeouts on external calls
- Error messages that don't help diagnose
- Inconsistent error return shapes
- Missing retry logic with backoff where needed

### Shell Scripts
- Missing set -euo pipefail (or targeted equivalents) in scripts that change state
- || true or 2>/dev/null on state-changing commands (masks real failures; fine on read-only probes)
- cd without failure guard (subsequent commands run in the wrong directory)
- mktemp or sensitive output files without an EXIT trap cleanup
- Unquoted variable expansions in paths (word splitting on spaces)

### State Management
- Framework-appropriate state patterns (session_state, context, store)
- Mutable shared state without synchronization
- State scattered across globals
- Cache invalidation after mutations
- Stale state after redirects/reruns

### Dependency & Coupling
- Circular imports/dependencies
- Layer violations (presentation ↔ data)
- God objects everything depends on
- Components reaching into other components' internals
- Business logic in infrastructure code

### Registration & Wiring
- Routes/views/commands registered correctly
- Middleware/interceptors applied where needed
- Event listeners registered for dispatched events
- Components exported from package indexes
- DI container bindings for all interfaces

### API Design
- Consistent response shapes
- Input validation at boundaries
- Versioning strategy for external APIs
- Batch operations where N individual calls exist
- Pagination on list endpoints

### Consistency
- Same pattern used differently across files
- Naming conventions followed/violated
- Import ordering convention
- File/directory organization convention

## Quality Dimensions

### Type Safety
- Missing type annotations on public interfaces
- Nullable access without null check
- Generic "any"/"mixed" where specific types work
- Type assertions hiding real type errors
- Dict/array access on external data without .get()
- Inconsistent use of type aliases

### Dead Code
- Functions never called from any module
- Unused imports
- Commented-out code blocks (>3 lines)
- Unreachable code after return/throw
- Feature flags always true/false
- Config keys defined but never accessed
- Event handlers never triggered
- Store actions never dispatched

### Complexity
- Files over 300 lines
- Functions over 50 lines
- Nesting over 4 levels
- Cyclomatic complexity over 10
- Parameter count over 5
- Boolean parameters (should be named/enum)

### Duplication
- Same logic in 2+ places
- Similar error handling repeated
- Similar validation in multiple endpoints
- Copy-pasted data transformations
- Similar test setup across test files

### Modernization
- Outdated syntax for the language version
- Old library APIs when newer alternatives exist
- Manual implementations of things the stdlib handles
- Deprecated function/method usage

### Naming
- Misleading names (function does more than name suggests)
- Boolean variables not starting with is/has/can/should
- Unclear abbreviations
- Inconsistent naming convention within a module

### Test Coverage
- Public methods with complex logic but no tests
- Edge cases not covered (empty input, null, overflow)
- Error paths not tested
- Integration points not tested
- Tests that only test the happy path

## Performance Dimensions

### Query Efficiency
- N+1 queries (loop + query pattern)
- SELECT * when specific columns suffice
- Missing LIMIT on unbounded queries
- Filtering/sorting in app instead of DB
- Missing indexes on filtered/joined columns
- Missing connection pooling
- Repeated identical queries per request

### I/O Efficiency
- File/HTTP reads inside loops
- Synchronous I/O in async context
- Loading full files when streaming works
- Not closing handles/connections (resource leaks)
- Missing batching on external API calls

### Caching
- Same expensive computation repeated per request
- Rarely-changing data fetched every time
- Missing cache invalidation after writes
- Cache keys not accounting for all parameters

### Memory
- Full dataset loaded when pagination/chunking works
- String concatenation in loops (vs join/builder)
- Large objects retained when no longer needed
- Missing generators/iterators for large sequences

### Algorithmic
- O(n²) when O(n) or O(n log n) is possible
- Linear search where hash/set lookup works
- Repeated sorts on same data
- Unnecessary serialization/deserialization cycles

### Response/Payload
- API returning more data than consumer needs
- Missing compression on large responses
- Missing pagination
- Full page re-render when partial update suffices

## AI Slop Dimensions (optional, for --slop equivalent)

### Over-Abstraction
- Interface with exactly one implementation
- Factory for something instantiated once
- Strategy with one strategy
- Wrapper that adds no behavior

### Premature Generalization
- Config options no code path exercises
- Parameters always called with same value
- Plugin systems with zero plugins

### Confident-But-Wrong
- Error handling that doesn't actually handle anything
- Retry without idempotency consideration
- Pagination that breaks on last page
- Auth checking login but not permissions

### Copy-Paste Artifacts
- Variable names from a different context
- Comments describing what code used to do
- Imports for unused libraries
- Exception types from wrong framework

# Rule Style Guide

How to write `.claude/rules/` files that are effective, concise, and correctly scoped.

## File Structure

Every rule file starts with YAML frontmatter for scoping, then markdown content.

### Path-Scoped (most files)
```yaml
---
paths:
  - "src/**/*.py"
  - "lib/**/*.py"
---
```

### Always-Applied (rare, for project-wide architecture rules)
```yaml
---
alwaysApply: true
---
```

Use `alwaysApply` only for rules that genuinely matter regardless of which file is being edited (e.g., architecture layer rules, fundamental conventions). Default to `paths:` scoping.

That default holds for reference material only. `paths:` fires when Claude **reads** a matching file and stays silent when it **writes** one, so prohibitions, access inventories, and any rule governing a write-heavy workflow belong in `alwaysApply: true` however large they are. Shrink those rather than scoping them. Full decision rule and the measured trigger behavior: SKILL.md Phase 2e.

### Additional frontmatter notes

- **Two `paths:` forms are equally valid:** the block-list form above and an inline comma-separated form (`paths: app/**/*.php, public/index.php, composer.json`). They behave identically. Match whichever the existing rule set already uses; don't rewrite one into the other during an audit.
- **`paths:` entries can be exact files, not just globs.** A subsystem rule scopes cleanly to the handful of files that implement it: `paths: src/consolidator.py`, or `paths: app/src/Core/FeatureFlags.php, app/src/Services/OAuthService.php, .env`. The rule loads exactly when those files are edited and stays out of context otherwise.
- **`security.md` is the canonical always-on file.** Security review applies to every source file, so mark it `alwaysApply: true` or scope it to all source (`src/**/*.py`, `app/**/*.php`). Every mature rule set has exactly one always-on security file.

## Scoping Guidelines

| Pattern | When to use |
|-|-|
| `src/**/*.py` | Language-wide rules (security, error handling, quality) |
| `src/web/**/*.py` | Framework-specific rules (Streamlit, Flask, Django) |
| `src/core/**/*.py` | Domain-specific rules (backup engine, data processing) |
| `backend/**/*.php` | Platform rules in a monorepo |
| `tests/**/*.py` | Test-specific conventions |

**Too broad:** `**/*.py` loads on test files, scripts, config generators. Scope to where source code actually lives.

**Too narrow:** `src/core/backup/engine.py` only loads for one file. Broaden to `src/core/**/*.py` unless the rule truly applies to only that file.

**When single-file scope IS right:** the "unless it truly applies to only that file" case is common, since a rule governing one subsystem/module (a scraper, a consolidator, a dashboard, a feature-flag registry) should scope to the single file (or small set) that implements it. Only broaden a single-file scope when the rule is really a language-wide concern mis-filed under one file: a generic "catch specific exceptions" rule pinned to `engine.py` belongs in the language-wide error-handling file instead.

**Multi-pattern:** Use when rules apply to related but separate directories:
```yaml
paths:
  - "src/core/**/*.py"
  - "src/utils/**/*.py"
```

## Writing Rules

### Format

```markdown
## Section Heading

- Rule statement as a declarative instruction
- Another rule, direct and specific
- Rule with rationale: "Never do X because Y" (only when the reason isn't obvious)
```

### Style

**DO:**
- Write imperative statements: "Always set timeout= on subprocess calls"
- Be specific: "Use `html.escape()` before embedding in `unsafe_allow_html=True`"
- Group related rules under `##` headings
- One concern per bullet point
- Reference files when a pattern is project-specific: `Reference: src/core/config_manager.py`

**DON'T:**
- Write code blocks longer than one line (this is a rule file, not a tutorial)
- Explain how the language works ("In Python, exceptions are...")
- Include examples of correct code (describe the rule, don't demonstrate it)
- Duplicate rules across files (put each rule in the most specific file)
- Restate what the language/framework already enforces
- Use passive voice ("Exceptions should be caught" → "Catch specific exception types")

### Granularity

A good rule is:
- Actionable in the moment of writing code
- Specific enough that you can tell if code violates it
- General enough to apply to more than one line of code

**Too vague:** "Write secure code"
**Too specific:** "Line 42 of engine.py must use parameterized queries"
**Right level:** "All YAML operations go through ConfigManager, never raw yaml.load()"

### When to Include Rationale

Most rules don't need a "because" clause. Include one only when:
- The rule is counterintuitive ("Allow direct Repository access from Controllers for reads, because Processor overhead isn't justified for simple GETs")
- The rule has project-specific history ("Never use offset-based pagination in the validator, because validated rows drop out of the result set")
- Violating the rule causes a non-obvious failure ("Write metadata JSON atomically via temp+rename, because a crash mid-write corrupts the file")

## Rule Shapes Beyond the One-Line "Do X / Never Y"

Most rules are single declarative bullets. Mature rule sets also use two other shapes; recognize and preserve them:

### Procedural create-flow checklist

A numbered "Adding a new X" list that enumerates the must-not-skip steps for a recurring create-flow (a new model, content type, migration, feature, endpoint). This IS a rule (it prevents the "forgot step 4" class of mistake), and it belongs at the bottom of the relevant rule file. Each step points to the sibling rule/file that governs it. Keep the steps; don't inline the details of each (those live in the sibling file).

Example shape (not literal content): `Adding a new content type: 1) add route + feature flag, 2) create model (see models.md), 3) create category table, 4) add pivot table, 5) add Tag methods, 6) update this doc.` Derive the actual steps from how the codebase's existing entities were wired.

### Domain / business-invariant rule

Rules that encode facts a generic engineering taxonomy can't know: which enum value blocks a downstream action, what a status transition means, the integrity constraint that must hold across entities ("excluded records never receive positive flags"), the recommended count bound ("3-7 tags per item"), the SSRF/redirect model a fetcher must follow. These come from the project's enums, status machines, subsystem boundaries, and cross-entity invariants, not from the review-dimensions taxonomy. Every mature project has a handful of these; a rule set without any is almost certainly under-covering the domain. See SKILL.md Phase 2a "Derive domain dimensions."

## Organization Patterns

### By Domain (recommended for most projects)
```
rules/
├── security.md          # Input validation, secrets, injection prevention
├── error-handling.md    # Exception patterns, logging, failure modes
├── code-quality.md      # Size limits, types, dead code, performance
├── database.md          # Query safety, access patterns, schema conventions
├── web-layer.md         # Framework-specific UI patterns
└── architecture.md      # Layer rules, module boundaries, conventions
```

### By Architectural Layer (for strict layered architectures)
```
rules/
├── controllers.md       # HTTP layer rules
├── processors.md        # Business logic layer rules
├── repositories.md      # Data access layer rules
├── security.md          # Cross-cutting security rules
└── php-standards.md     # Language conventions
```

### By Platform (for monorepos)
```
rules/
├── be-security.md       # Backend security
├── be-database.md       # Backend data access
├── mo-components.md     # Mobile component rules
├── mo-state.md          # Mobile state management
├── we-architecture.md   # Web frontend architecture
└── code-quality.md      # Cross-platform quality rules
```

## CLAUDE.md Integration

When rules exist, CLAUDE.md should reference them with a summary table:

```markdown
## Coding Rules (`.claude/rules/`)

Path-scoped rules auto-loaded when editing matching files.

| Rule file | Scope | Covers |
|-|-|-|
| `security.md` | `src/**/*.py` | Subprocess safety, input validation, secrets |
| `web-layer.md` | `src/web/**/*.py` | Cache invalidation, rerun discipline |
```

Keep it to a one-line description per file. The rules themselves have the detail.

**Critical: never use `@` import syntax for rule files in CLAUDE.md.** Writing `@.claude/rules/security.md` force-loads that file at session start, every turn, regardless of its `paths:` frontmatter. The whole point of `paths:` scoping is on-demand loading; `@imports` defeat it. Reference rule files by name only (`see security.md`, or in a table as above).

## Anti-Patterns

**The tutorial file:** A rule file that explains how to use the framework, with code examples and documentation. Rules files are instructions, not onboarding docs.

**The kitchen sink:** One file with 100+ rules covering everything. Split by domain and scope properly.

**The aspirational rule:** Rules for patterns the project doesn't use yet. Only write rules for things that exist or are about to be built.

**The duplicate rule:** Same rule in security.md and database.md. Keep each rule in exactly one file. *One deliberate exception:* when a rule's source-of-truth file is path-scoped to a directory the rule *also* needs to govern from a different file type, the scoping hides it. Real case: a "no inline event handlers" rule lived in `js-standards.md` (scoped `public/js/**`), but the regressions all happened in PHP view files, where that file never loads. The fix is to *restate* the rule in the in-scope file (`views.md`, scoped to the views dir) with a one-line cross-reference to the source of truth: deliberate duplication for path-visibility. Only do this when path-scoping genuinely hides the rule from where violations occur; don't use it as license to copy rules around freely.

**The stale reference:** `Reference: src/old_module.py:42` pointing to a file that was renamed 3 months ago. References need freshness checks.

**The inventory dump:** A rule file padded with content that's derivable from the codebase: directory trees, lists of controllers/models/services/partials, URL route enumerations, base-class method signatures, helper function indexes, design-token catalogs. These rot the moment a file is added or renamed and contribute zero rule signal. Replace with one-line pointers: `Inventory: ls app/src/Models/`, `Methods: read app/src/Core/View.php`, `Routes: app/config/routes.php`, `Tokens: public/css/tokens.css`. Keep the rule (the "must"/"never"); drop the catalog.

*Carve-out: enum/status semantics are not inventory.* The *list* of enum values is derivable (`SHOW COLUMNS`, read the enum class, use a pointer). The *meaning attached to each value* is not: "which `email_tier` values are outreach-eligible," "`NONE` means checked-and-empty vs `NULL` means not-yet-evaluated," "archived tools stay publicly linkable but drafts don't." That semantic layer is load-bearing domain knowledge no schema query returns; keep it. Drop only the mechanically-derivable column list; preserve the invariants and eligibility rules bolted to the values.

**The `@import` trap:** Referencing a rule file from `CLAUDE.md` with `@.claude/rules/foo.md` force-loads it every turn and overrides whatever `paths:` scoping the file declared. Always reference rule files by name only.

{
  "name": "a-rules-optimizer-glob-verifier",
  "version": "1.0.0",
  "lockfileVersion": 3,
  "requires": true,
  "packages": {
    "": {
      "name": "a-rules-optimizer-glob-verifier",
      "version": "1.0.0",
      "dependencies": {
        "minimatch": "10.2.6"
      }
    },
    "node_modules/balanced-match": {
      "version": "4.0.4",
      "resolved": "https://registry.npmjs.org/balanced-match/-/balanced-match-4.0.4.tgz",
      "integrity": "sha512-BLrgEcRTwX2o6gGxGOCNyMvGSp35YofuYzw9h1IMTRmKqttAZZVU67bdb9Pr2vUHA8+j3i2tJfjO6C6+4myGTA==",
      "license": "MIT",
      "engines": {
        "node": "18 || 20 || >=22"
      }
    },
    "node_modules/brace-expansion": {
      "version": "5.0.9",
      "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.9.tgz",
      "integrity": "sha512-ScQ4IuvIEF1TMlP7Zt+vjJ//9zlPb2SDcxWxM3bk8s6t6GGdJ7KO1dCcTidOPJKePW30LE/2cT7wCyPho9/Wxg==",
      "license": "MIT",
      "dependencies": {
        "balanced-match": "^4.0.2"
      },
      "engines": {
        "node": "20 || >=22"
      }
    },
    "node_modules/minimatch": {
      "version": "10.2.6",
      "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.2.6.tgz",
      "integrity": "sha512-vpLQEs+VLCr1nU0BXS07maYoFwlDAH0gngQuuttxIwutDFEMHq2blX+8vpgxDdK3J1PwjCJiep77OitTZ4Ll1A==",
      "license": "BlueOak-1.0.0",
      "dependencies": {
        "brace-expansion": "^5.0.8"
      },
      "engines": {
        "node": "18 || 20 || >=22"
      },
      "funding": {
        "url": "https://github.com/sponsors/isaacs"
      }
    }
  }
}

{
  "name": "a-rules-optimizer-glob-verifier",
  "private": true,
  "version": "1.0.0",
  "description": "Pinned runtime dependency for the a-rules-optimizer glob verifier.",
  "dependencies": {
    "minimatch": "10.2.6"
  }
}

#!/usr/bin/env node
/**
 * Verify .claude/rules/ path scoping against the real matcher.
 *
 * Claude Code bundles minimatch (confirmed in the 2.1.220 binary: the compiled
 * bundle carries minimatch's own exports.GLOBSTAR / Minimatch class). This
 * script resolves that same library so a `paths:` glob is tested with the
 * matcher that actually decides whether a rule loads, instead of a shell glob
 * that only approximates it.
 *
 * Why this exists: a scoped rule whose glob matches nothing never fires, and it
 * reads as covered in an audit. That is strictly worse than having no rule.
 *
 * How Claude Code actually parses `paths:` (verified 2026-07-28 against 2.1.220
 * with an InstructionsLoaded hook, reading the `globs` array it reports back):
 *   - One inline value holding several comma-separated globs IS split into
 *     several patterns. `a/**\/*.php, b/**\/*.php` was reported as two globs.
 *     Spaces after the comma are optional.
 *   - Braces are expanded before matching: `app/{foo,bar}/**\/*.php` came back
 *     as ["app/foo/**\/*.php", "app/bar/**\/*.php"].
 *   - A rule loads when ANY one of its patterns matches. The others may be dead
 *     without blocking the load.
 * An earlier version of this script pushed the whole inline value as a single
 * glob, so every multi-pattern rule file reported DEAD GLOB. That is the exact
 * false-positive this script exists to prevent, pointed the wrong way: it would
 * have condemned 9 working rule files in the first project it was run against.
 *
 * Usage, from a project root:
 *   node verify-rule-globs.js               report every rule file and its globs
 *   node verify-rule-globs.js --for <path>  which rules load when editing <path>
 *   node verify-rule-globs.js --rules <dir> non-default rules directory
 *
 * Exit codes: 0 clean, 1 at least one dead glob, 2 setup problem.
 */

'use strict';

const fs = require('fs');
const path = require('path');
const { execSync } = require('child_process');

function loadMinimatch() {
    try {
        return require('minimatch');
    } catch {}
    console.error('minimatch is not installed for this skill. Run: npm ci --prefix "' + __dirname + '"');
    process.exit(2);
}

/**
 * One frontmatter value to the pattern list Claude Code derives from it:
 * brace-expand first, then split the result on commas. Applying it in that
 * order keeps a comma inside `{a,b}` from being treated as a separator.
 */
function expandPatterns(raw, braceExpand) {
    const out = [];
    let expanded;
    try {
        expanded = braceExpand(raw);
    } catch {
        expanded = [raw];
    }
    for (const chunk of expanded) {
        for (const piece of chunk.split(',')) {
            const p = piece.trim().replace(/^["']|["']$/g, '');
            if (p) out.push(p);
        }
    }
    return [...new Set(out)];
}

/** Frontmatter paths, supporting `paths: glob` and the block-list form. */
function parseFrontmatter(text, braceExpand) {
    const m = text.match(/^---\r?\n([\s\S]*?)\r?\n---/);
    if (!m) return { paths: [], legacyMarker: false, hasFrontmatter: false };

    const body = m[1];
    const legacyMarker = /^alwaysApply\s*:/m.test(body);
    const paths = [];

    // Horizontal whitespace only: \s would match the newline after `paths:` and
    // swallow the first list item as if it were an inline value.
    const inline = body.match(/^paths[ \t]*:[ \t]*(\S.*)$/m);
    if (inline) {
        paths.push(...expandPatterns(inline[1].trim(), braceExpand));
    } else if (/^paths[ \t]*:[ \t]*$/m.test(body)) {
        let inBlock = false;
        for (const line of body.split(/\r?\n/)) {
            if (/^paths[ \t]*:[ \t]*$/.test(line)) { inBlock = true; continue; }
            if (!inBlock) continue;
            const item = line.match(/^\s*-\s*(.+?)\s*$/);
            if (item) paths.push(...expandPatterns(item[1], braceExpand));
            else if (line.trim() !== '') break; // next key ends the block
        }
    }
    return { paths, legacyMarker, hasFrontmatter: true };
}

/** A pattern with no glob metacharacter is a literal path, matchable on disk alone. */
function isLiteral(p) {
    return !/[*?[\]{}!+@]/.test(p);
}

/** Tracked files are the right universe: respects .gitignore, skips vendor/node_modules. */
function repoFiles() {
    try {
        return execSync('git ls-files', { encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 })
            .split('\n').filter(Boolean);
    } catch {
        console.error('Not a git repository. Run this from a project root.');
        process.exit(2);
    }
}

function ruleFiles(rulesDir) {
    if (!fs.existsSync(rulesDir)) {
        console.error(`No rules directory at ${rulesDir}`);
        process.exit(2);
    }
    return fs.readdirSync(rulesDir).filter(f => f.endsWith('.md')).sort()
        .map(f => path.join(rulesDir, f));
}

function reportForFile(rules, minimatch, braceExpand, target) {
    const norm = target.replace(/^\.\//, '');
    console.log(`Rules that load when reading ${norm}:\n`);
    let any = false;
    let bytes = 0;
    for (const rf of rules) {
        const { paths, hasFrontmatter } = parseFrontmatter(fs.readFileSync(rf, 'utf8'), braceExpand);
        const name = path.basename(rf);
        const size = fs.statSync(rf).size;
        if (!hasFrontmatter || paths.length === 0) {
            console.log(`  [always]  ${name}  ${size} B`);
            any = true;
            bytes += size;
            continue;
        }
        const hit = paths.find(p => minimatch(norm, p));
        if (hit) {
            console.log(`  [scoped]  ${name}  ${size} B   via  ${hit}`);
            any = true;
            bytes += size;
        }
    }
    if (!any) console.log('  (none)');
    else console.log(`\n  Total in context for this read: ${bytes} B`);
    console.log('\nNote: a scoped rule fires on a READ of a matching file. It does not');
    console.log('fire when such a file is written without a matching read first.');
}

function reportAll(rules, minimatch, braceExpand, files) {
    let dead = 0, alwaysBytes = 0, alwaysCount = 0, scopedCount = 0, neverLoads = 0;

    for (const rf of rules) {
        const text = fs.readFileSync(rf, 'utf8');
        const { paths, legacyMarker, hasFrontmatter } = parseFrontmatter(text, braceExpand);
        const name = path.basename(rf);
        const bytes = Buffer.byteLength(text);

        if (!hasFrontmatter || paths.length === 0) {
            alwaysCount++;
            alwaysBytes += bytes;
            const note = legacyMarker
                ? '  (alwaysApply: is a Cursor convention Claude Code ignores; this file is'
                  + '\n            always-on because it has no paths:, not because of the marker)'
                : '';
            console.log(`ALWAYS  ${name}  ${bytes} B${note}`);
            continue;
        }

        scopedCount++;
        const matchedAll = new Set();
        const lines = [];
        for (const p of paths) {
            const hits = files.filter(f => minimatch(f, p));
            hits.forEach(h => matchedAll.add(h));
            // git ls-files can't see a gitignored file, but Claude Code matches
            // real reads. `.env` is the usual case: present, scoped, untracked.
            if (hits.length === 0 && isLiteral(p) && fs.existsSync(p)) {
                matchedAll.add(p);
                lines.push(`             1 file   ${p}  (untracked on disk)`);
                continue;
            }
            if (hits.length === 0) dead++;
            lines.push(`          ${hits.length === 0 ? 'DEAD GLOB ->' : String(hits.length).padStart(4) + ' files'}  ${p}`);
        }
        // A rule loads if ANY one pattern matches (verified against 2.1.220), so
        // a dead pattern beside a live one is dead weight, not a dead rule.
        const status = matchedAll.size === 0 ? '  <- NEVER LOADS' : '';
        if (matchedAll.size === 0) neverLoads++;
        console.log(`SCOPED  ${name}  ${bytes} B  ${matchedAll.size} distinct files${status}`);
        lines.forEach(l => console.log(l));
    }

    console.log(`\nAlways-on: ${alwaysBytes} B across ${alwaysCount} file(s), paid every turn.`);
    console.log(`Scoped:    ${scopedCount} file(s), paid only on a matching read.`);
    if (neverLoads > 0) {
        console.log(`\n${neverLoads} rule file(s) NEVER LOAD: every pattern is dead. Fix or remove them.`);
        if (dead > neverLoads) console.log(`${dead} dead pattern(s) in total; the rest sit beside a live pattern and are dead weight only.`);
        return 1;
    }
    if (dead > 0) {
        console.log(`\nNo unreachable rule files. ${dead} dead pattern(s) sit beside a live pattern:`);
        console.log('the rule still loads, but that pattern matches nothing. Worth pruning, not urgent.');
        return 0;
    }
    console.log('\nNo dead globs.');
    return 0;
}

function main() {
    const argv = process.argv.slice(2);
    const forIdx = argv.indexOf('--for');
    const rulesIdx = argv.indexOf('--rules');
    const rulesDir = rulesIdx !== -1 ? argv[rulesIdx + 1] : '.claude/rules';

    const mm = loadMinimatch();
    const minimatch = mm.minimatch;
    const braceExpand = mm.braceExpand;
    const rules = ruleFiles(rulesDir);

    if (forIdx !== -1) {
        const target = argv[forIdx + 1];
        if (!target) {
            console.error('--for needs a file path');
            process.exit(2);
        }
        reportForFile(rules, minimatch, braceExpand, target);
        process.exit(0);
    }

    process.exit(reportAll(rules, minimatch, braceExpand, repoFiles()));
}

main();

---
name: a-self-learner
description: Turn recurring review findings into proposed rule and skill updates.
disable-model-invocation: true
---

# Self-Learner: Review → Rule/Skill Feedback Loop

Closes the loop between review skills and rule/skill files. When a project review catches the same class of issue repeatedly, this skill proposes a preventive update (a new rule, a new preflight check, or a whitelist entry) and hands it off to `a-rules-optimizer` or `a-review-optimizer` for application.

**Input:** optional flags
- `--dry-run` (default): analyze and propose, never write
- `--apply`: after each proposal, ask user, apply on approval
- `--since YYYY-MM-DD`: only consider findings from this date forward
- `--threshold N`: override recurrence threshold (default 3 for recurring, 5 for chronic)

If the project has no `.claude/reviews/` directory yet, the skill reports "no history to learn from" and offers to scaffold the convention (see `references/review-log-schema.md`).

---

## Core Principles

**Propose, never silently write.** Every rule/skill change must be shown to the user as a diff with rationale, then applied only after explicit approval. Propose only; never auto-apply and never auto-commit. A proposal workflow is not commit authorization, whatever your project or global commit rules say.

**Delegate writes to the other optimizers.** This skill never edits rule files or review SKILL.md directly. It generates a structured proposal and invokes `a-rules-optimizer` (for rule writes) or `a-review-optimizer` (for preflight/agent writes). Keeps each skill's scope tight.

**Evidence > opinion.** A recurrence claim needs ≥ N findings of the same category across distinct dates. "Feels recurring" is not good enough; the cluster must survive the grouping algorithm in `references/recurrence-detection.md`.

**Record rejections.** Proposals the user declines get logged to `rejected-proposals.md` with the reason. Next run shouldn't re-raise the same rejection: only surface if new evidence appears (e.g., the issue recurred another 5 times since rejection).

**Per-project scope.** Each project owns its own `.claude/reviews/` directory. The skill does not cross-pollinate learnings between projects (that's a future extension; for now, deliberate isolation keeps project conventions separate).

---

## Phase 1: Ingest

Read the signals. The skill needs data before it can cluster anything.

### 1a: Review log

```bash
test -f .claude/reviews/review-issues.jsonl && wc -l .claude/reviews/review-issues.jsonl
test -f .claude/reviews/review-issues-archive.jsonl && wc -l .claude/reviews/review-issues-archive.jsonl
```

If neither exists, this project hasn't been capturing findings yet. Stop here and report:

> No review history found at `.claude/reviews/review-issues.jsonl`. To enable self-learning, add capture calls to this project's `*-review` skill (see `references/review-log-schema.md` for the format and `references/capture-finding.sh` for the helper). Until then, there's nothing for this skill to learn from.

**Optionally, offer to scaffold the convention** so the user doesn't have to wire it by hand. If they accept, this is the one thing `a-self-learner` writes directly (inert plumbing, not a rule/skill change; still show what lands and get an explicit yes first):

1. **Drop the helper.** Copy `references/capture-finding.sh` to `.claude/scripts/capture-finding.sh` and `chmod +x` it. It creates `.claude/reviews/` on first call, computes `category_hash`, and appends one JSON line per finding.
2. **Seed the ledgers.** Create `.claude/reviews/` with empty `applied-learnings.md` and `rejected-proposals.md` (just their `#` header lines from `references/review-log-schema.md`). The `review-issues.jsonl` and `review-issues-archive.jsonl` files appear on the first capture and first archive respectively; don't pre-create them.
3. **Git posture.** These files are meant to be tracked per-project (they document how defenses evolved). Do NOT gitignore them.
4. **Wire the capture calls (delegate, don't hand-edit).** The review skill's SKILL.md must call `capture-finding.sh` once per confirmed finding. Editing that review skill is `a-review-optimizer`'s job, not this skill's. Emit a proposal for `a-review-optimizer`: "add a capture-finding.sh call per confirmed finding, passing `--project/--skill/--run-id/--dimension/--severity/--category/--file/--line/--message`; slug the `--category` as one-pattern-one-slug (see `references/review-log-schema.md` slug hygiene)." Route it through the normal Phase 4 approval gate. If no `*-review` skill exists yet, tell the user to create one (via `a-review-optimizer`) first, since capture has nothing to hook into otherwise.

Either way, stop for this run: even scaffolded, the log is empty until the next review populates it. Re-run after findings accumulate.

If the file exists, read every line as JSON. Drop malformed lines with a warning (don't silently discard, print them so the user can fix the capture).

### 1b: Project memory (optional convention)

These sources exist only if the project keeps an agent-memory convention; many don't. Treat every one as optional: if a path is absent, skip it silently and lean on the review log and `rejected-proposals.md`. Do not stall waiting for files a fresh project never had.

Also ingest:

- Project `MEMORY.md` "Review History" section, if present (existing convention in some projects, surface debt scores and round summaries).
- `.claude/projects/<project-id>/memory/feedback_fp_*.md`: documented false positives. These become the KEEP-THIS-WHITELISTED signal for Phase 3.
- `.claude/projects/<project-id>/memory/feedback_review_*.md`: severity / priority feedback (e.g. "deprioritize X for this threat model").

### 1c: Existing review & rules context

- Read the project's `*-review` SKILL.md (whichever skill writes to the log). Extract: agent names, current check IDs, known-correct pattern whitelists.
- Read `.claude/scripts/preflight*.sh` if present. Extract check IDs.
- Read `.claude/rules/` index.

Knowing what's already checked prevents proposing duplicates.

### 1d: Previously processed

Read `.claude/reviews/applied-learnings.md` and `rejected-proposals.md` if present. Cluster IDs already applied or rejected don't need re-proposing unless new evidence arrived.

---

## Phase 2: Cluster

Group findings into patterns. See `references/recurrence-detection.md` for the full algorithm. Summary:

1. **Group** by `category_hash`: precomputed by the capture helper as `sha256(category)[:16]`, a pure function of the stable `category` slug. One cluster per category.
2. The hash deliberately excludes the free-text `message` (method names / paths vary per finding) and the `dimension` (the same pattern gets tagged differently by different agents): including either one fragmented a single pattern into many hashes. See `references/review-log-schema.md`.
3. **Count** occurrences per hash across all ingested findings.
4. **Classify**:
   - `Recurring`: ≥ 3 occurrences across ≥ 2 distinct dates
   - `Chronic`: ≥ 5 occurrences OR spans > 30 days OR in ≥ 3 distinct files
   - `False Positive`: user dismissed ≥ 2 times (from feedback_fp_* or explicit rejection)
   - `One-off`: everything else; ignore
5. **Attribute** each cluster to a review dimension (security / architecture / quality / performance). For a cluster whose findings carry more than one dimension (~4% of categories drift this way), pick the dominant one (most frequent, ties broken by highest severity). Determines which optimizer gets the proposal.

Between grouping (2) and counting (3), run the **near-duplicate slug gate** (`recurrence-detection.md` Step 2b): the hash is the slug, so slug drift (`inline-event-handler` vs `inline-event-handler-in-view`) splits one pattern into several sub-threshold clusters. Surface candidate merge-families for human confirmation before classifying: don't auto-merge.

Emit `recurring-patterns.md` as the Phase 2 artifact: one section per cluster with the data from `references/recurrence-detection.md` output format.

---

## Phase 3: Propose

For each cluster, decide the target and draft a proposal. See `references/proposal-template.md` for the exact format.

### 3a: Recurring / Chronic → preventive action

Pick one of these target actions based on the cluster's dimension and what's already in place:

| Cluster dimension | No existing check | Check exists but misses | Pattern-level (architectural) |
|-|-|-|-|
| Security | Propose preflight check via `a-review-optimizer` | Update check pattern via `a-review-optimizer` | Propose rule file via `a-rules-optimizer` |
| Architecture | Propose preflight check | Update check pattern | Propose rule file |
| Quality | Propose preflight check | Update check pattern | Propose rule, possibly scoped narrowly |
| Performance | Propose preflight check | Update check pattern | Propose rule file |

**When in doubt, lean toward `a-rules-optimizer`**: rules prevent issues before code is written; preflight checks catch them after. Prevention is cheaper.

**Before proposing a NEW rule, check whether the rule already exists but isn't loading.** A cluster can keep recurring while the governing rule already exists, because it lives in a file whose `paths:` scope never matches the file type where violations happen (e.g. a JS rule scoped `public/js/**` that never loads while editing PHP views, where every regression sits). Signal: the cluster's `file` values cluster in one file type, and grep finds the rule already stated in a differently-scoped rule file. When you see this, the proposal is not "add a rule"; it's "restate the existing rule in a file scoped to where the violations occur, with a one-line cross-reference to the source of truth," routed to `a-rules-optimizer`. This path-visibility fix is cheaper and truer than inventing a duplicate rule. Name the root cause explicitly in the proposal's `rationale`.

### 3b: False Positive clusters → whitelist action

Target: the review skill's "KNOWN CORRECT PATTERNS (DO NOT FLAG)" section (see `a-review-optimizer`'s Phase 4a output). Propose adding:

```
- <normalized pattern>: <project-specific reason>, see <file:line of representative occurrence>
```

Use the existing `feedback_fp_*.md` text verbatim where possible: the user wrote it in their own words for a reason.

### 3c: Proposal structure

Every proposal must include (see `references/proposal-template.md`):

- `cluster_id`: stable identifier (hash prefix)
- `pattern`: one-line human description
- `evidence`: list of findings supporting the cluster (file:line + date + severity)
- `rationale`: why a preventive change is warranted (count, span, severity distribution)
- `target_skill`: `a-rules-optimizer` or `a-review-optimizer`
- `target_file`: which file the other skill should modify
- `proposed_diff`: concrete change, not a description of one
- `expected_effect`: what future reviews should differ about after this lands

Missing fields → proposal is incomplete, don't show it to the user.

---

## Phase 4: User approval gate (hard stop)

**This phase must pause for user input.** Present proposals one at a time, batched by priority (Chronic first, then Recurring, then False Positive whitelists). For each:

1. Show the proposal in human-readable form (pattern, evidence count, rationale, the diff).
2. Ask: "Apply this? [y/n/skip/details]"
3. On `y` → delegate to the target skill with the proposal as input. That skill writes the file. Do not write anything directly from `a-self-learner`.
4. On `n` → append to `rejected-proposals.md` with reason (ask the user for the reason if `n` alone; "no reason given" is acceptable).
5. On `skip` → leave for next run, neither apply nor reject.
6. On `details` → show evidence in full (all finding lines, not just count), then re-prompt.

**Commit policy.** Even after approval, this skill does not commit. The target skill writes the file; the user decides when to stage and commit per the global per-commit-ask rule. Do not invoke `git commit` from this workflow.

---

## Phase 5: Record

After the approval loop finishes:

1. **`applied-learnings.md`**: append one entry per applied proposal: date, cluster_id, target, summary, file(s) modified. This becomes the project's "how our defenses hardened" changelog.
2. **`rejected-proposals.md`**: already appended to in Phase 4 for each rejection. Also record the evidence count at rejection time so re-proposal threshold can be computed ("this was rejected when count was 4; only re-raise when count exceeds 8").
3. **Archive processed AND resolved findings**: move them from `review-issues.jsonl` to `review-issues-archive.jsonl`, stamped with a processed-date, so the next run starts from a smaller log and doesn't re-cluster old data. Archive a finding when ANY of these holds:
   - it participated in an applied or rejected cluster this run;
   - a resolution row pairs with it on `(category, file)`, such as a `FIXED:` row written by the project's `*-fix` skill;
   - it carries a `covered_by` stamp naming the check or rule that now owns it.

   **Archiving is not deletion, and it does not require re-verifying the fix.** The archive file is kept, and if the issue is still present the next review re-captures it. Being conservative here is exactly what breaks the loop: a resolved finding left in the active window re-clusters on every future run and buries the live signal under it.

   Archiving only the first case is the common mistake. It is how two projects reached 491 and 140 active rows that were roughly 85% resolved pairs, and on one of them a single commit fixed four findings *in the same commit that appended them to the log*, leaving all four reading as open weeks later.
4. **Report summary**: count of proposals applied / rejected / skipped, list of files modified, suggestion for when to re-run (typically: "after the next 5 reviews, or in ~4 weeks").

---

## What this skill does NOT do

- **Does not write rule files or review SKILLs directly.** Always delegates.
- **Does not auto-commit.** User owns commits per CLAUDE.md.
- **Does not cross-pollinate between projects.** Each project's feedback loop is isolated.
- **Does not re-review the codebase.** It only operates on the review log and feedback memory. If the log is empty, the skill has nothing to do.
- **Does not replace `a-rules-optimizer` or `a-review-optimizer`.** Those remain the authoritative ways to audit against the codebase. This skill adds a *historical-evidence* input that those skills can consume.

---

## Reference files

| File | When to read | Contains |
|-|-|-|
| `references/review-log-schema.md` | Phase 1a (understanding the log format) and any capture-side integration | JSONL schema, field definitions, `.claude/reviews/` directory convention |
| `references/recurrence-detection.md` | Phase 2 (clustering) | Grouping, hashing, thresholds, cluster classification |
| `references/proposal-template.md` | Phase 3 (drafting proposals) | Required fields, format, target-skill routing rules |
| `references/capture-finding.sh` | Setup: helper that existing review skills call to append findings | Small bash script (~30 lines), safe to source and call |

interface:
  display_name: "Self Learner"
  short_description: "Turn recurring review findings into rule updates"
policy:
  allow_implicit_invocation: false

#!/usr/bin/env bash
# capture-finding.sh: append one finding to .claude/reviews/review-issues.jsonl
#
# Helper for project *-review skills. Call once per finding. Safe to call from
# any review skill across any project.
#
# Usage:
#   bash capture-finding.sh \
#       --project example-api \
#       --skill example-review \
#       --run-id "$RUN_ID" \
#       --dimension security \
#       --severity high \
#       --category subprocess-without-timeout \
#       --file src/core/backup_engine.py \
#       --line 142 \
#       --message "subprocess.run() without timeout=" \
#       [--fix "add timeout=30"] \
#       [--agent security] \
#       [--check-id SUB-01] \
#       [--evidence "build check -> exit 1"]
#
# The helper creates .claude/reviews/ if missing, computes category_hash,
# assigns a finding_id, and appends one JSON line. No other side effects.

set -euo pipefail

# Defaults
PROJECT=""; SKILL=""; RUN_ID=""; DIMENSION=""; SEVERITY=""
CATEGORY=""; FILE=""; LINE="0"; MESSAGE=""
FIX=""; AGENT=""; CHECK_ID=""; WHITELISTED="false"; EVIDENCE=""

while [ $# -gt 0 ]; do
    case "$1" in
        --project)     PROJECT="$2"; shift 2 ;;
        --skill)       SKILL="$2"; shift 2 ;;
        --run-id)      RUN_ID="$2"; shift 2 ;;
        --dimension)   DIMENSION="$2"; shift 2 ;;
        --severity)    SEVERITY="$2"; shift 2 ;;
        --category)    CATEGORY="$2"; shift 2 ;;
        --file)        FILE="$2"; shift 2 ;;
        --line)        LINE="$2"; shift 2 ;;
        --message)     MESSAGE="$2"; shift 2 ;;
        --fix)         FIX="$2"; shift 2 ;;
        --agent)       AGENT="$2"; shift 2 ;;
        --check-id)    CHECK_ID="$2"; shift 2 ;;
        --evidence)    EVIDENCE="$2"; shift 2 ;;
        --whitelisted) WHITELISTED="true"; shift ;;
        *)             echo "Unknown arg: $1" >&2; exit 2 ;;
    esac
done

# Required-field check
for v in PROJECT SKILL RUN_ID DIMENSION SEVERITY CATEGORY FILE MESSAGE; do
    if [ -z "${!v}" ]; then
        echo "capture-finding.sh: missing required --${v,,}" >&2
        exit 2
    fi
done

DATE=$(date -u +%Y-%m-%d)
REVIEWS_DIR=".claude/reviews"
LOG="$REVIEWS_DIR/review-issues.jsonl"
mkdir -p "$REVIEWS_DIR"

# category_hash groups findings into pattern classes for a-self-learner.
# The clustering key is the CATEGORY slug alone (lowercased): NOT the free-text
# message (which carries method names / paths / identifiers that fragment one
# pattern into dozens of hashes), and NOT the dimension (the same pattern is
# tagged 'architecture' by one agent and 'quality' by another, which would
# re-split it). Slug hygiene is load-bearing: keep slugs specific enough that one
# slug == one pattern, and consistent enough that one pattern == one slug (no
# 'inline-event-handler' vs 'inline-event-handler-in-view' drift).
# See review-log-schema.md "category_hash computation".
HASH=$(printf '%s' "$CATEGORY" | tr '[:upper:]' '[:lower:]' | sha256sum | cut -c1-16)

# Simple UUID-ish id: 16 hex chars from urandom
FINDING_ID=$(head -c 8 /dev/urandom | od -An -tx1 | tr -d ' \n')

# Build JSON object. python3 handles escaping cleanly.
python3 - "$DATE" "$PROJECT" "$SKILL" "$RUN_ID" "$FINDING_ID" "$DIMENSION" \
               "$SEVERITY" "$CATEGORY" "$FILE" "$LINE" "$MESSAGE" \
               "$FIX" "$AGENT" "$CHECK_ID" "$HASH" "$WHITELISTED" "$EVIDENCE" "$LOG" <<'PY'
import json, sys
date, project, skill, run_id, finding_id, dimension, severity, category, \
    file_, line, message, fix, agent, check_id, category_hash, whitelisted, evidence, log = sys.argv[1:]

rec = {
    "date": date, "project": project, "skill": skill, "run_id": run_id,
    "finding_id": finding_id, "dimension": dimension, "severity": severity,
    "category": category, "file": file_, "line": int(line or 0),
    "message": message, "category_hash": category_hash,
    "whitelisted": whitelisted == "true",
}
if fix: rec["fix_proposed"] = fix
if agent: rec["agent"] = agent
if check_id: rec["check_id"] = check_id
if evidence: rec["evidence"] = evidence

with open(log, "a") as f:
    f.write(json.dumps(rec, separators=(",", ":")) + "\n")
PY

# Proposal Template

The shape of the proposal that `a-self-learner` Phase 3 generates and Phase 4 presents. Every proposal must be complete: missing fields mean the proposal isn't ready to show the user.

## Required fields

```yaml
cluster_id:        <hash prefix, e.g. "a3f1c2b4">
pattern:           <one-line human description>
dimension:         <security | architecture | quality | performance>
classification:    <Recurring | Chronic | RE-EMERGENT | False Positive>

evidence:
  count:           <integer>
  span_days:       <integer>
  distinct_files:  <integer>
  first_seen:      <YYYY-MM-DD>
  last_seen:       <YYYY-MM-DD>
  examples:        <list of 3-5 findings: file:line, severity, date>

rationale:         <why this warrants a preventive change, 1-3 sentences tied to the evidence>

target_skill:      <a-rules-optimizer | a-review-optimizer>
target_file:       <specific file the target skill should modify>
proposed_change:   <concrete diff or rule text, NOT a description>

expected_effect:   <what should differ about future reviews after this lands>

re_propose_if:     <condition that would justify re-raising if rejected>
```

## Routing: which skill writes what

| Target file | Target skill | Use when |
|-|-|-|
| `.claude/rules/<file>.md` (new or existing) | `a-rules-optimizer` | Pattern-level issue that a rule could prevent upstream (e.g. "all SQL uses prepared statements"). Preferred when the pattern represents a project convention. |
| `.claude/rules/<in-scope file>.md` (restate existing rule) | `a-rules-optimizer` | The governing rule already exists but its `paths:` scope never loads on the file type where violations occur. Propose restating it in a file scoped to those files, with a cross-reference to the source of truth, not a brand-new rule. |
| `.claude/scripts/preflight*.sh` | `a-review-optimizer` | Issue is best caught by a deterministic grep/python check. Preferred when the pattern has a mechanical signature. |
| Review skill's capture calls | `a-review-optimizer` | Scaffolding a project's capture mechanism: the review SKILL.md must call `capture-finding.sh` per confirmed finding. `a-self-learner` drops the helper script, but wiring calls into the review skill is `a-review-optimizer`'s edit. |
| Review skill's `KNOWN CORRECT PATTERNS` section | `a-review-optimizer` | False-positive whitelist additions. |
| Review skill's agent prompt | `a-review-optimizer` | New checks that require LLM judgment (not mechanical). |

**When ambiguous, prefer `a-rules-optimizer` over `a-review-optimizer`.** Rules prevent at write-time; preflight catches at review-time. Prevention is cheaper.

## Writing `proposed_change`

The proposal must be **executable** by the target skill, not descriptive. Two examples:

### Good: rule file proposal (target: `a-rules-optimizer`)

```markdown
target_file: .claude/rules/security.md

proposed_change: |
  Append to `security.md` under a new section `## Subprocess Safety`:

    - Every `subprocess.*` call must include `timeout=`: no exceptions for "quick" commands.
    - `shell=True` is forbidden outside `scripts/`: argv form only.
    - Reference: src/core/backup_engine.py:142 for the canonical pattern.
```

### Bad: too vague to act on

```markdown
target_file: .claude/rules/security.md

proposed_change: "Add a rule about subprocess safety."
```

The second form forces the target skill to reinvent the proposal. Don't do that.

### Good: preflight check proposal (target: `a-review-optimizer`)

```markdown
target_file: .claude/scripts/preflight.sh

proposed_change: |
  Replace the current SUB-01 Python block with:

    ```python
    import re, pathlib
    for f in pathlib.Path('src').rglob('*.py'):
        text = f.read_text()
        for m in re.finditer(r'subprocess\.(run|call|check_output|check_call|Popen)\s*\(', text):
            ...  # balanced-paren scan checking 'timeout' not in call_text
    ```

  Reason: current regex misses `.check_output` and `.Popen` forms: evidence at src/cli.py:622, scripts/ingest.py:394.
```

## Presentation to the user

When Phase 4 shows a proposal, display it as:

```
────────────────────────────────────────────────
Proposal a3f1c2b4: subprocess-without-timeout
Classification: Chronic (10 findings, 2026-01-15 → 2026-04-19, 4 files)

Rationale:
  Current preflight catches .run() but misses .check_output() and .Popen().
  Pattern has recurred for 3 months across 4 files.

Target: a-review-optimizer → .claude/scripts/preflight.sh
Change: update SUB-01 regex to cover all subprocess variants (see diff above)

Expected effect:
  Next review should catch subprocess timeout issues in cli.py:622 and
  scripts/ingest.py:394 (both currently missed).

Apply this? [y/n/skip/details]
────────────────────────────────────────────────
```

Keep the prompt terse. The user reads the rationale, looks at the diff, decides. Don't pad with hedging or alternatives: the skill already picked one option; the user either agrees or doesn't.

## After approval

Invoke the target skill with the proposal as structured input. Example (illustrative, actual invocation shape depends on how the skill wrapper is called):

```
Delegate to a-review-optimizer with:
  action: update-preflight-check
  check_id: SUB-01
  file: .claude/scripts/preflight.sh
  proposed_change: <the diff text from the proposal>
  source_cluster: a3f1c2b4
```

The target skill applies the change and reports back with the modified file paths. `a-self-learner` then appends to `applied-learnings.md` with the cluster_id, target, and files modified.

## After rejection

Append to `rejected-proposals.md`:

- Cluster ID, pattern, date of rejection
- Evidence count at rejection time
- Reason (quote the user's reply or note "no reason given")
- Re-proposal threshold (typically: count must exceed rejected_count × 2 or a new distinct file must appear)

Re-proposal thresholds prevent the skill from pestering the user about rejected ideas.

# Recurrence Detection Algorithm

How `a-self-learner` Phase 2 turns a pile of findings into clustered patterns ready for Phase 3 proposals.

## Inputs

- `review-issues.jsonl` (current window)
- `review-issues-archive.jsonl` (optional, only read if `--deep` flag set or if window is very small)
- `feedback_fp_*.md` files from project memory: for marking clusters as False Positive

## Step 1: Parse and filter

```python
import json
from pathlib import Path
from datetime import datetime

findings = []
for line in Path(".claude/reviews/review-issues.jsonl").read_text().splitlines():
    line = line.strip()
    if not line:
        continue
    try:
        f = json.loads(line)
    except json.JSONDecodeError as e:
        print(f"MALFORMED: {line[:80]}... ({e})")
        continue
    findings.append(f)
```

Drop findings where:
- `whitelisted = true` (already dismissed in-run; keep the dismissal data for FP analysis, but don't cluster into recurring)
- `severity = info` (not actionable by itself)
- `dimension = other` and no explicit category
- `skill` ends in `-fix` (a resolution row, not an occurrence)

That last one is not cosmetic. Fix skills log a `FIXED:` row carrying the **same** `category` as the finding they closed, so counting both doubles every cluster and a well-fixed pattern accumulates evidence that it is chronic. Measured on a real project log: 75 of 192 rows were resolutions, and one category read as 22 occurrences when it was 11 findings plus 11 fixes. Since these counts are what escalate a prose rule into a fail-gating preflight check, the inflation buys checks nobody needed.

Keep the resolution rows in the log. They are what lets you spot a category that keeps coming back after being fixed, which is a different and more interesting signal than raw recurrence. Read them deliberately in Step 4; don't let them into the Step 2 counts.

## Step 2: Group by `category_hash`

```python
from collections import defaultdict

clusters = defaultdict(list)
for f in findings:
    clusters[f["category_hash"]].append(f)
```

`category_hash` is `sha256(category)[:16]` (see `review-log-schema.md`), so this is identical to grouping by `category`: one cluster per pattern class. Two different categories sharing a hash would be a sha256 collision (effectively impossible). If you ever see one category split across multiple hashes, the log predates a hash-definition change and needs a backfill: don't treat the fragments as distinct patterns.

## Step 2b: Surface near-duplicate slug families (human merge gate)

Because `category_hash` is the slug, clustering is only as good as slug hygiene, and review skills drift: the same pattern arrives as `inline-event-handler`, `inline-event-handler-in-view`, `inline-event-handler-js`. Each becomes its own cluster, and the split can push a genuinely-recurring pattern below the Step 3 thresholds (one Chronic cluster of 7 becomes one Recurring of 3 plus three ignored One-offs).

Before classifying, detect candidate merge-families and **ask the human**: never auto-merge (some near-twins are intentionally distinct, e.g. `method-over-30-lines` vs `method-over-50-lines`).

```python
import difflib
from itertools import combinations

def _toks(slug): return set(slug.split("-"))

def near_dup(a, b):
    ta, tb = _toks(a), _toks(b)
    jaccard = len(ta & tb) / len(ta | tb)
    ratio = difflib.SequenceMatcher(None, a, b).ratio()
    return (a in b or b in a) or jaccard >= 0.5 or ratio >= 0.72

# union-find over near-dup edges → connected components of slugs
slugs = sorted(clusters)                       # one slug per category_hash
parent = {s: s for s in slugs}
def find(x):
    while parent[x] != x:
        parent[x] = parent[parent[x]]; x = parent[x]
    return x
for a, b in combinations(slugs, 2):
    if near_dup(a, b):
        parent[find(a)] = find(b)

families = {}
for s in slugs:
    families.setdefault(find(s), []).append(s)
families = {root: m for root, m in families.items() if len(m) > 1}
```

For each family with >1 member, present it sorted by finding count and ask:

```
Possible slug drift: one pattern under several slugs?
  inline-event-handler-in-view   (11 findings, 2 dates)
  inline-event-handler           (5,  1 date)
  inline-event-handler-js        (1)
  inline-event-handlers          (1)
  inline-event-handlers-admin    (1)
Merge for this run? [canonical slug / n = keep separate]
```

On confirm, treat the family as **one cluster for this run's Step 3** (merge in-memory under the chosen canonical slug; sum the counts/dates/files). Don't rewrite `category` on historical findings unless you deliberately want to normalize the log: that mutates the authoritative clustering key. To make a merge *durable*, do one of: record the canonical mapping in a `slug-aliases.md` note read at the top of Step 2b, or fix the slug at source so the review skill emits the canonical form going forward. Record confirmed keep-separate decisions too, so the gate stops re-asking about intentional twins.

Thresholds (Jaccard ≥ 0.5, ratio ≥ 0.72, or substring) are tuned to catch real families without flooding: loosen to surface more, tighten if it over-suggests. Union-find chains transitively (A~B~C even when A and C aren't alike), so a family may contain a stray: in practice the gate groups `method-over-30-lines` + `method-over-50-lines` correctly but also drags in `file-over-500-lines` via the shared `over…lines` tokens. Split strays when confirming. This is a suggestion engine, not an authority: the human decides every merge.

## Step 3: Classify each cluster

For each cluster, compute:

- `count` = number of findings in the cluster
- `distinct_dates` = number of distinct `date` values
- `distinct_files` = number of distinct `file` values
- `first_seen`, `last_seen` = min / max of `date`
- `span_days` = (last_seen - first_seen).days
- `severity_mix` = count by severity
- `is_fp_feedback_present` = True if any `feedback_fp_*.md` file mentions this `category`

Classification:

```
if is_fp_feedback_present:
    cls = "False Positive"
elif count >= 5 or span_days > 30 or distinct_files >= 3:
    cls = "Chronic"
elif count >= 3 and distinct_dates >= 2:
    cls = "Recurring"
else:
    cls = "One-off"
```

Thresholds are adjustable via the skill's `--threshold N` flag (overrides the `3` for Recurring; `Chronic` uses `threshold + 2`).

## Step 4: Skip clusters already resolved

For each cluster, check `applied-learnings.md` and `rejected-proposals.md`:

- If cluster_id already in `applied-learnings.md` AND no new findings since the applied date → skip (already addressed, and hasn't re-emerged).
- If cluster_id already in `applied-learnings.md` AND new findings since → **re-surface with a "RE-EMERGENT" flag**. The prior fix didn't hold. Investigate.
- If cluster_id in `rejected-proposals.md` AND current count ≤ re-proposal threshold stored there → skip.
- If cluster_id in `rejected-proposals.md` AND current count exceeds the threshold → resurface for a new decision.

## Step 5: Rank for presentation

Order clusters for Phase 3 presentation:

1. **RE-EMERGENT** clusters first: a prior fix failed; this is highest value to investigate.
2. **Chronic** clusters (broad, long-standing patterns) tend to translate into rule files.
3. **Recurring** clusters (narrower, newer) tend to translate into preflight checks.
4. **False Positive** clusters: whitelist proposals.

Within each tier, sort by `count` descending. Present top-N per tier (default N = 5; user can override).

## Step 6: Emit `recurring-patterns.md`

See `review-log-schema.md` for the output format. Include in each section:

- cluster_id, pattern name, dimension
- classification and counts
- first/last seen dates
- distinct files affected
- representative findings (3-5 example lines with file:line and severity)
- recommended action (preflight vs rule vs whitelist)
- target skill (`a-review-optimizer` vs `a-rules-optimizer`)

## Edge cases

- **Cluster with one dominant file.** If 10 findings are all in `src/legacy/*.py`, the fix might be "deprecate that module" rather than "add a rule." Flag these as `LOCALIZED`: they may deserve a different proposal (code fix vs rule) or a path-scoped rule.
- **Cluster that spans multiple skills.** If `skill` varies within a cluster (e.g. both `example-review` and an older `example-review-legacy` captured it), note it: the fix might need to apply to both review skills.
- **Runaway cluster from a single bad run.** If all findings share the same `run_id` and the cluster has no findings from other runs, treat as One-off regardless of count. A single bad PR scan that flagged 30 things isn't a recurring pattern.
- **Empty project history.** Zero findings → skill reports "no history, cannot learn" and exits cleanly. Not an error.

## Calibration

These thresholds (3 for Recurring, 5 for Chronic, 30 days for span, 3 files) are defaults. For a new capture convention, findings accumulate slowly: thresholds may need lowering in the first few months. Users can pass `--threshold 2` to see near-patterns early.

Thresholds should NOT be tuned per-project in the skill itself. Pass them as arguments. Keeps behavior predictable.

# Review Log Schema

The `.claude/reviews/` directory holds per-project review history that `a-self-learner` consumes. Each project owns its own directory: no cross-project sharing.

## Directory layout

```
.claude/reviews/
├── review-issues.jsonl           Append-only log of findings (current window).
├── review-issues-archive.jsonl   Findings already processed by a-self-learner.
├── recurring-patterns.md         Generated by a-self-learner Phase 2.
├── applied-learnings.md          Changelog of accepted proposals.
└── rejected-proposals.md         Proposals the user declined, with reasons.
```

All files are meant to be git-tracked (per-project). They document how the project's defenses evolved. Size budget: `review-issues.jsonl` rolls to archive after each `a-self-learner` run; archive can grow indefinitely but is rarely read.

## review-issues.jsonl: one JSON per line

Every line is a complete JSON object. No arrays, no pretty-printing across lines. This makes `wc -l`, `grep`, `head`, and `jq -c` work naturally.

### Required fields

| Field | Type | Meaning |
|-|-|-|
| `date` | ISO 8601 date (YYYY-MM-DD) | When the finding was produced |
| `project` | string | Project name (e.g. `example-api`, `example-web`) |
| `skill` | string | Which review skill produced it (e.g. `example-review`, `web-review`) |
| `run_id` | UUID or timestamp | Groups findings from the same review run |
| `finding_id` | UUID | Unique per finding |
| `dimension` | enum | `security` / `architecture` / `quality` / `performance` / `other` |
| `severity` | enum | `critical` / `high` / `medium` / `low` / `info` |
| `category` | string | Short slug: the pattern class (e.g. `subprocess-without-timeout`, `st-rerun-without-invalidate`, `raw-sql-with-interpolation`). **Stable across runs**, this is the clustering key. |
| `file` | string | Repo-relative path |
| `line` | integer | Line number (1-based); use 0 if file-level without a line |
| `message` | string | Human-readable description of the issue |
| `category_hash` | string (sha256 hex, 16 chars) | Pre-computed `sha256(category)[:16]`: a pure function of the `category` slug, for fast grouping. The capture helper fills this. |

### Optional fields

| Field | Type | Meaning |
|-|-|-|
| `fix_proposed` | string | The proposed fix shown to the user |
| `agent` | string | Which review agent flagged it (e.g. `security`, `architecture`) |
| `check_id` | string | Preflight check that surfaced it (e.g. `SEC-14`, `SUB-01`) |
| `evidence` | string | The command run or the file:line read that supports the claim, e.g. `build check -> exit 1` or `read OrderRepository.php:79-95, no $afterId param` |
| `whitelisted` | boolean | True if the review ultimately dismissed via a known-correct whitelist |
| `confirmed_by_user` | boolean | True if the user explicitly acknowledged the finding during review |

`evidence` exists because two passes over the same method can reach opposite verdicts on whether it pages, an hour apart, and the log cannot adjudicate: both rows carry a confident claim and neither carries the read behind it. Fill it whenever a finding rests on a judgement rather than on a preflight hit (a preflight hit already has `check_id`).

### Example lines

```json
{"date":"2026-04-19","project":"example-api","skill":"example-review","run_id":"r-20260419-1","finding_id":"f-9a2c","dimension":"security","severity":"high","category":"subprocess-without-timeout","file":"src/core/backup_engine.py","line":142,"message":"subprocess.run() without timeout=","fix_proposed":"add timeout=30","category_hash":"a3f1c2b4d5e6f789","agent":"security","check_id":"SUB-01"}
{"date":"2026-04-19","project":"example-api","skill":"example-review","run_id":"r-20260419-1","finding_id":"f-9a2d","dimension":"architecture","severity":"medium","category":"st-rerun-without-invalidate","file":"src/web/views/dashboard.py","line":88,"message":"st.rerun() without preceding invalidate()","fix_proposed":"invalidate() then st.rerun()","category_hash":"b7e2f3a4c5d6e890","agent":"architecture","check_id":"WEB-03"}
```

## `category_hash` computation

The capture helper computes `category_hash = sha256(category.lower())[:16]`: a pure function of the lowercased `category` slug, nothing else:

```bash
HASH=$(printf '%s' "$CATEGORY" | tr '[:upper:]' '[:lower:]' | sha256sum | cut -c1-16)
```

Lowercasing is so `Dead-Code` and `dead-code` co-cluster (case is not semantic in a slug). `category` is the **load-bearing clustering key**, so the hash deliberately excludes:

- **The free-text `message`**: it carries method names, file paths, and identifiers that vary per finding. Hashing it fragmented a single pattern class into dozens of hashes (e.g. `method-over-30-lines` → 71 hashes), defeating grouping. This was the pre-2026-06-14 bug.
- **The `dimension`**: the same pattern is tagged `architecture` by one agent and `quality` by another (~4% of categories drift this way), which would re-split the cluster. Dimension is decided per-cluster at Phase 2 instead.

Consequence: grouping by `category_hash` is identical to grouping by `category`. Two different categories cannot collide (barring a sha256 collision); one category cannot fragment. When this definition changes, recompute (backfill) the hash for historical findings so old and new captures still group together.

**Cross-implementation invariant.** Any code that recomputes the hash (the python backfill, an alternate capture path) MUST produce byte-identical output: `hashlib.sha256(category.lower().encode("utf-8")).hexdigest()[:16]`. The shell `printf '%s'` emits no trailing newline, so the hashed bytes are exactly the lowercased category string, no separator, no newline.

**Slug hygiene is load-bearing.** Because the hash is the slug, clustering is only as good as the slugs review skills emit. Near-duplicate slugs for one pattern (`inline-event-handler` vs `inline-event-handler-in-view`, `file-put-contents-no-lock` vs `file-put-contents-without-lock`) form separate clusters and can drop a genuinely-recurring pattern below the recurrence thresholds. Phase 2 (`recurrence-detection.md`) should surface near-duplicate slug families for human merge before classifying.

## recurring-patterns.md: generated output

Generated by `a-self-learner` Phase 2. One section per cluster:

```markdown
## [cluster a3f1c2b4] subprocess-without-timeout

**Dimension:** security
**Severity:** high (8 findings), medium (2 findings)
**Classification:** Chronic (10 occurrences, spans 2026-01-15 → 2026-04-19, 4 distinct files)
**First seen:** 2026-01-15
**Last seen:** 2026-04-19
**Files affected:**
- src/core/backup_engine.py:142
- src/cli.py:622
- src/sync/worker.py:88
- scripts/ingest.py:394

**Recommended action:** Update preflight check SUB-01 pattern: it's catching some but missing others (lines 622 and 394 don't fit the current regex). Target skill: `a-review-optimizer`.
```

## applied-learnings.md: human-readable changelog

Append-only. One entry per applied proposal:

```markdown
### 2026-04-19: [cluster a3f1c2b4] Subprocess timeout enforcement

**Source:** Recurring pattern: 10 findings across 2026-01-15..2026-04-19.
**Applied via:** a-review-optimizer
**Files modified:** `.claude/scripts/preflight.sh` (updated SUB-01 regex to catch `subprocess.run`, `.call`, `.check_output`, and `.Popen` forms).
**Expected effect:** Next review should catch all 4 previously-missed files on the first run.
```

## rejected-proposals.md: declined decisions

Append-only. One entry per rejection:

```markdown
### 2026-04-19: [cluster b2d4e6f8] Ban print() in src/

**Evidence at rejection:** 4 findings (`src/cli.py`, `src/debug_helpers.py`).
**Reason:** User maintains that print() in `src/cli.py` is the intentional CLI output channel and in `debug_helpers.py` is intentional debug output. Not an issue.
**Re-proposal threshold:** Do not re-raise unless count exceeds 12 AND appears outside `src/cli.py` / `debug_helpers.py`.
```

## Size / retention policy

- `review-issues.jsonl`: active window; should rarely exceed a few thousand lines. `a-self-learner` archives processed findings on each run.
- `review-issues-archive.jsonl`: grows indefinitely but is cold storage. Never read during normal clustering; only referenced when investigating a specific historical finding.
- `recurring-patterns.md`: regenerated each run; treat as derived, not authoritative.
- `applied-learnings.md` / `rejected-proposals.md`: authoritative, append-only, never rewrite.