diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml index cccf03b..4c9160d 100644 --- a/.github/workflows/release-please.yml +++ b/.github/workflows/release-please.yml @@ -41,6 +41,9 @@ jobs: - name: Publish pi-arc to npm run: node scripts/npm-publish-workspace-if-needed.mjs @sentiolabs/pi-arc + - name: Publish pi-code-quality to npm + run: node scripts/npm-publish-workspace-if-needed.mjs @sentiolabs/pi-code-quality + - name: Publish pi-frontend-design to npm run: node scripts/npm-publish-workspace-if-needed.mjs @sentiolabs/pi-frontend-design diff --git a/.release-please-manifest.json b/.release-please-manifest.json index cddfb90..67d40fc 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,5 +1,6 @@ { "packages/pi-arc": "0.10.1", + "packages/pi-code-quality": "0.1.0", "packages/pi-frontend-design": "0.1.1", "packages/pi-scriptable-statusline": "0.2.0" } diff --git a/README.md b/README.md index df69cf4..04bc7fd 100644 --- a/README.md +++ b/README.md @@ -9,10 +9,11 @@ This repo uses npm workspaces with one package per directory under `packages/*`. | Package | Path | Description | |---|---|---| | [`@sentiolabs/pi-arc`](packages/pi-arc) | `packages/pi-arc` | Arc issue tracker integration for Pi: skills, prompts, extension commands, session context, bundled checklist/question support, and optional Arc specialist integration through external `pi-subagents`. | +| [`@sentiolabs/pi-code-quality`](packages/pi-code-quality) | `packages/pi-code-quality` | Pi skills and prompts for code-quality review, including 4-lens AI slop review and PR/branch size review for stack/split decisions. | | [`@sentiolabs/pi-frontend-design`](packages/pi-frontend-design) | `packages/pi-frontend-design` | Frontend design skill for distinctive, production-grade Pi UI work. | | [`@sentiolabs/pi-scriptable-statusline`](packages/pi-scriptable-statusline) | `packages/pi-scriptable-statusline` | Scriptable footer and statusline UI package for Pi: owns the footer, supports scriptable above/below-editor widgets, and includes a natural-language setup skill. | -Future packages such as `pi-slop` should be added under `packages/*` when their sources are ready. +Future packages should be added under `packages/*` when their sources are ready. ## Development @@ -27,6 +28,7 @@ Test the package locally with Pi from the monorepo root: ```bash pi -e ./packages/pi-arc +pi -e ./packages/pi-code-quality pi -e ./packages/pi-frontend-design pi -e ./packages/pi-scriptable-statusline ``` @@ -40,6 +42,7 @@ Releases are independent per package through Release Please. See [`docs/releasin - [`docs/development.md`](docs/development.md) - [`docs/releasing.md`](docs/releasing.md) - [`docs/packages/pi-arc.md`](docs/packages/pi-arc.md) +- [`docs/packages/pi-code-quality.md`](docs/packages/pi-code-quality.md) - [`docs/packages/pi-frontend-design.md`](docs/packages/pi-frontend-design.md) - [`docs/packages/pi-scriptable-statusline.md`](docs/packages/pi-scriptable-statusline.md) - [`packages/pi-arc/README.md`](packages/pi-arc/README.md) diff --git a/docs/packages/pi-code-quality.md b/docs/packages/pi-code-quality.md new file mode 100644 index 0000000..df949db --- /dev/null +++ b/docs/packages/pi-code-quality.md @@ -0,0 +1,45 @@ +# `@sentiolabs/pi-code-quality` + +`@sentiolabs/pi-code-quality` provides Pi-native code-quality skills and prompts for slop review and size review. + +## Included resources + +- Skill: `/skill:slop-review` +- Prompt alias: `/code-quality-slop [scope]` +- Skill: `/skill:size-review` +- Prompt alias: `/code-quality-size [scope]` +- References: Go, Python, Rust, and Svelte/TypeScript slop-review guidance; default size-review exclusions + +## Slop review workflow + +`slop-review` reviews code through four lenses: AI authorship signals, idiom fluency, code quality, and architecture/solution fit. It is the workflow for deciding whether the implementation itself looks suspect. + +## Size review workflow + +`size-review` reviews how a change is packaged for human review: raw versus post-exclusion size, stacked branch shape, viable seams, split effort, and concrete stack plans. + +## Usage + +```text +/code-quality-slop +/code-quality-slop src/ +/code-quality-slop #123 +/skill:slop-review + +/code-quality-size +/code-quality-size #123 +/code-quality-size feature/my-branch +/skill:size-review +``` + +## Portability + +The package does not require Arc or `pi-subagents`. It uses parallel agent tools when available and falls back to sequential lens passes otherwise. + +## Local development + +```bash +npm test --workspace @sentiolabs/pi-code-quality +npm run pack:dry-run --workspace @sentiolabs/pi-code-quality +pi -e ./packages/pi-code-quality +``` diff --git a/docs/releasing.md b/docs/releasing.md index 0d52787..426c263 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -27,6 +27,20 @@ Current package entries include: } ] }, + "packages/pi-code-quality": { + "component": "pi-code-quality", + "package-name": "@sentiolabs/pi-code-quality", + "release-type": "node", + "initial-version": "0.1.0", + "changelog-path": "CHANGELOG.md", + "extra-files": [ + { + "type": "json", + "path": "/package-lock.json", + "jsonpath": "$.packages['packages/pi-code-quality'].version" + } + ] + }, "packages/pi-frontend-design": { "component": "pi-frontend-design", "package-name": "@sentiolabs/pi-frontend-design", @@ -63,6 +77,7 @@ Publishing uses GitHub Actions and npm provenance through `scripts/npm-publish-w ```bash node scripts/npm-publish-workspace-if-needed.mjs @sentiolabs/pi-arc +node scripts/npm-publish-workspace-if-needed.mjs @sentiolabs/pi-code-quality node scripts/npm-publish-workspace-if-needed.mjs @sentiolabs/pi-frontend-design node scripts/npm-publish-workspace-if-needed.mjs @sentiolabs/pi-scriptable-statusline ``` diff --git a/package-lock.json b/package-lock.json index 172eb00..02eb5a7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1338,6 +1338,10 @@ "resolved": "packages/pi-arc", "link": true }, + "node_modules/@sentiolabs/pi-code-quality": { + "resolved": "packages/pi-code-quality", + "link": true + }, "node_modules/@sentiolabs/pi-frontend-design": { "resolved": "packages/pi-frontend-design", "link": true @@ -3900,6 +3904,14 @@ } } }, + "packages/pi-code-quality": { + "name": "@sentiolabs/pi-code-quality", + "version": "0.1.0", + "license": "MIT", + "engines": { + "node": ">=24.0.0" + } + }, "packages/pi-frontend-design": { "name": "@sentiolabs/pi-frontend-design", "version": "0.1.1", diff --git a/packages/pi-code-quality/CHANGELOG.md b/packages/pi-code-quality/CHANGELOG.md new file mode 100644 index 0000000..154afa7 --- /dev/null +++ b/packages/pi-code-quality/CHANGELOG.md @@ -0,0 +1,5 @@ +# Changelog + +## 0.1.0 + +- Initial Pi package for AI slop review, PR/branch size review, and code quality reviewability workflows. diff --git a/packages/pi-code-quality/LICENSE b/packages/pi-code-quality/LICENSE new file mode 100644 index 0000000..a520894 --- /dev/null +++ b/packages/pi-code-quality/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 Sentio Labs + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/packages/pi-code-quality/README.md b/packages/pi-code-quality/README.md new file mode 100644 index 0000000..211ba02 --- /dev/null +++ b/packages/pi-code-quality/README.md @@ -0,0 +1,63 @@ +# Pi Code Quality Package + +> Monorepo location: this package lives at `packages/pi-code-quality` in the `pi-nexus` workspace. From the monorepo root, test it with `npm test --workspace @sentiolabs/pi-code-quality` and load it locally with `pi -e ./packages/pi-code-quality`. + +Pi skills and prompts for AI slop review, PR/branch size review, and code reviewability analysis. + +This package ports the Claude Code `code-quality` plugin's `slop-review` and `size-review` workflows to Pi. + +## What is included + +- `/skill:slop-review` — 4-lens slop review for AI authorship signals, idiom drift, code quality issues, and architecture/solution-fit problems. +- `/code-quality-slop [scope]` — prompt alias for reviewing current changes, files, directories, PRs, or broad codebase scopes. +- `/skill:size-review` — PR/branch size review that decides whether a change should be split, stacked, cleaned up, or shipped as-is. +- `/code-quality-size [scope]` — prompt alias for reviewing the current branch, a PR, or a named branch for reviewability and stack seams. +- Slop-review language references for Go, Python, Rust, and Svelte/TypeScript. +- Size-review default exclusions for generated files, lockfiles, vendored output, and common machine-generated artifacts. + +## Workflow distinction + +`slop-review` evaluates what the code does and whether the implementation quality or solution fit is suspect. `size-review` evaluates how the change is packaged for human review: raw vs post-exclusion size, stacked branch shape, viable seams, split effort, and concrete stack plans. + +## Portable execution + +The `slop-review` skill is portable. When the current Pi session exposes a parallel agent tool, the review can run Step 0 first and then run the applicable Phase 1 lenses in parallel. When no parallel tool is available, the same lenses run sequentially in the current agent context with separated findings. + +Parallelism is opportunistic; the review methodology, calibration, scoring, and output format are the contract. + +## Install from npmjs.org + +```bash +pi install npm:@sentiolabs/pi-code-quality +``` + +## Install locally + +From this monorepo: + +```bash +pi -e ./packages/pi-code-quality +``` + +## Usage + +```text +/code-quality-slop +/code-quality-slop src/ +/code-quality-slop path/to/file.go +/code-quality-slop #123 +/skill:slop-review + +/code-quality-size +/code-quality-size #123 +/code-quality-size feature/my-branch +/skill:size-review +``` + +## Development + +```bash +npm test --workspace @sentiolabs/pi-code-quality +npm run pack:dry-run --workspace @sentiolabs/pi-code-quality +pi -e ./packages/pi-code-quality +``` diff --git a/packages/pi-code-quality/package.json b/packages/pi-code-quality/package.json new file mode 100644 index 0000000..0856dba --- /dev/null +++ b/packages/pi-code-quality/package.json @@ -0,0 +1,57 @@ +{ + "name": "@sentiolabs/pi-code-quality", + "version": "0.1.0", + "description": "Pi skills and prompts for AI slop review, PR/branch size review, and code reviewability analysis.", + "type": "module", + "keywords": [ + "pi-package", + "pi-skill", + "code-quality", + "code-review", + "ai-slop", + "idiom-fluency", + "ai-authorship", + "solution-fit", + "architecture-review", + "pr-size", + "branch-review", + "stacked-prs", + "reviewability" + ], + "license": "MIT", + "author": { + "name": "Sentio Labs", + "url": "https://github.com/sentiolabs" + }, + "repository": { + "type": "git", + "url": "git+ssh://git@github.com/SentioLabs/pi-nexus.git", + "directory": "packages/pi-code-quality" + }, + "homepage": "https://github.com/SentioLabs/pi-nexus/tree/main/packages/pi-code-quality#readme", + "bugs": { + "url": "https://github.com/SentioLabs/pi-nexus/issues" + }, + "engines": { + "node": ">=24.0.0" + }, + "scripts": { + "test": "node --test tests/*.test.mjs", + "pack:dry-run": "npm pack --dry-run", + "prepublishOnly": "npm test && npm run pack:dry-run" + }, + "files": [ + "skills/", + "prompts/", + "README.md", + "CHANGELOG.md", + "LICENSE" + ], + "publishConfig": { + "access": "public" + }, + "pi": { + "skills": ["./skills"], + "prompts": ["./prompts/*.md"] + } +} diff --git a/packages/pi-code-quality/prompts/code-quality-size.md b/packages/pi-code-quality/prompts/code-quality-size.md new file mode 100644 index 0000000..2ebc779 --- /dev/null +++ b/packages/pi-code-quality/prompts/code-quality-size.md @@ -0,0 +1,8 @@ +--- +description: Run a PR or branch size review to decide whether the change should be split, stacked, cleaned up, or shipped as-is +argument-hint: "[scope]" +--- + +Use the `size-review` skill for this code quality size request. + +If no scope is provided, review the current branch against its merge-base with trunk. Treat `$ARGUMENTS` as the requested scope when present; it may be a PR number, PR URL, branch name, or explicit branch/base scope. diff --git a/packages/pi-code-quality/prompts/code-quality-slop.md b/packages/pi-code-quality/prompts/code-quality-slop.md new file mode 100644 index 0000000..1bcee55 --- /dev/null +++ b/packages/pi-code-quality/prompts/code-quality-slop.md @@ -0,0 +1,23 @@ +--- +description: Run an AI slop/code-quality review on files, directories, PRs, current changes, or the full codebase +argument-hint: "[scope]" +--- + +# Slop Review + +Use the `slop-review` skill against the specified target. + +## Usage + +- `/code-quality-slop` -- review current git diff (unstaged changes) +- `/code-quality-slop src/` -- review a directory +- `/code-quality-slop path/to/file.go` -- review specific files +- `/code-quality-slop PR` or `/code-quality-slop #123` -- review a pull request + +## Instructions + +Invoke the `slop-review` skill with the user's specified scope. +If no scope is provided, default to reviewing the current git diff. +Pass any arguments the user provided as the scope for the review. + +Use `$ARGUMENTS` as the requested scope when present. diff --git a/packages/pi-code-quality/skills/size-review/SKILL.md b/packages/pi-code-quality/skills/size-review/SKILL.md new file mode 100644 index 0000000..c6fc488 --- /dev/null +++ b/packages/pi-code-quality/skills/size-review/SKILL.md @@ -0,0 +1,510 @@ +--- +name: size-review +description: > + Decide whether a pull request or branch should be split into multiple smaller + PRs (preferring git-spice-style stacked Change Requests) and rate the effort + to do so. Use this skill when the user asks "should I split this PR", "is + this PR too big", "can this be stacked", "review the size of this PR", "is + this reviewable", or describes a branch with many files, many commits, or + cross-cutting changes that resist single-pass review. Trigger proactively + when a PR exceeds 20 changed files or 500 added lines and the user asks for + any kind of pre-merge review. Produces a verdict, an effort rating + (easy/moderate/difficult), and — if a split is recommended — a concrete + stack plan with CR titles, file mapping, and dependency direction. +license: MIT +--- + +# Size Review + +Decide whether a PR or branch should be split into multiple smaller PRs and +rate the effort to do so. The output is a verdict, an effort rating, and — if +splitting is recommended — a concrete stack plan the author can execute. + +This skill is shape analysis, not content review. It pairs naturally with +`slop-review` (which evaluates *what* the code does) — this one evaluates +*how the change is packaged for review*. + +## Why this matters + +Large PRs cost teams real time. Reviewers fatigue past ~400 lines of diff, +miss bugs, and rubber-stamp later sections. Authors lose momentum waiting +for a single approval that gates everything. Stacked CRs (git-spice +`gs branch create`, Graphite, Sapling) let the author keep shipping in +small slices while reviewers see the full intent — without the all-or-nothing +review thread of one mega-PR. + +But splitting isn't always the right call. Some PRs are large because the +underlying change is genuinely indivisible (cross-cutting refactor where +each layer depends on the layer below). Some are part of an existing stack +already. Some are large only because of generated code that reviewers will +skim. The job of this skill is to make a *calibrated* call, not to +reflexively flag everything over a threshold. + +--- + +## Threshold + +Run the full analysis when ANY of these hold against the PR's base branch +*after exclusions are applied* (see Step 2): + +- More than **20 files changed** +- More than **500 lines added** (deletions don't count toward "size" — pure + deletions almost always make a PR easier to review, not harder) +- More than **30 commits** in the branch (review fatigue from rebase noise + and fixup commits, even if total LOC is modest) +- **3 or more top-level directories touched** (cross-cutting blast radius + matters even if line count is modest) + +Below all of these, write a one-line "Appropriately sized — ship as-is" report +and stop. Don't manufacture concerns where there are none. + +If the threshold check passes *only because* exclusions removed most of the +size (e.g., a 50-file PR that's 48 generated files plus 2 hand-written +changes), say so in the report — the size signal was dominated by generated +content, which is useful context for the reviewer. + +--- + +## Cost and runtime guidance + +This skill is **shape analysis, not content review**. The default workflow needs only +file *names*, *line counts*, *commit titles*, *PR body*, and *reviewer comments* — not +full file contents. Most reviews fit comfortably in 50-100k tokens of input. + +- **Default to base 200k context.** The `[1M]` context tier is wasted capacity for + shape analysis; do not escalate unless a specific seam check requires reading large + source files. Most reviews don't. +- **Sonnet is sufficient for the mechanical steps** (Steps 1-3: scope discovery, + exclusions, structural signals). The judgment-heavy steps (Steps 4-6: seam + viability, effort rating, recommendation) benefit from Opus when available, but + Sonnet handles them adequately at substantially lower cost. +- **Don't fetch full file contents** unless verifying a specific seam viability check + (Step 4). A name + commit-title + PR-body view is the load-bearing input. +- **Run as a single-pass analysis.** Unlike slop-review, this skill doesn't need + parallel subagents — the steps are sequential and each consumes the previous step's + output. + +For high-volume CI usage (every PR), consider running this skill at Sonnet by default +and reserving Opus only for explicit deep-dive requests or PRs flagged by other gates. + +--- + +## Workflow + +### Step 1: Scope, base, and stack detection + +Determine what to analyze. **Before running stats**, check whether the +branch is already part of a stack — this changes how stats should be +reported. + +**For a PR review:** + +```bash +gh pr view --json title,body,additions,deletions,changedFiles,commits,baseRefName,url +gh pr diff --name-only +``` + +If `baseRefName` is not the trunk (typically `main` or `master`), the +branch is **stacked on a non-trunk parent**. Also scan the body for +phrases like "Builds on #X", "Stacked on #Y", "Supersedes #Z" — these +are author-provided stack hints even when `baseRefName` happens to be +trunk (e.g., the parent already merged). + +**For a branch review:** + +Identify the trunk (`git symbolic-ref refs/remotes/origin/HEAD` or check +project conventions). Then check whether the user has named a base +explicitly, or whether the branch was created on top of a non-trunk +parent. `git for-each-ref refs/remotes/origin/` and the local branch +graph can help. + +**If the branch is stacked, compute and report two stat views:** + +- **Cumulative** (vs trunk): how big is the whole stack of work this + branch contributes to? This is the question reviewers face if they + squash-merge the whole stack. +- **Slice** (vs immediate parent): how big is *this* PR's slice? This + is the question for reviewing this CR alone. + +Both matter. Cumulative shows scope; slice shows reviewability. The +verdict and recommendation should usually weight the slice view more +heavily — if the slice is reasonably sized, the work has already been +properly stacked even if the cumulative view is large. + +If not stacked, just compute against trunk. + +**Capture for downstream steps:** + +- Title and full body +- `+adds` / `-dels` / files-changed / commit count (per base view) +- Commit list with subject lines +- Full file list + +### Step 2: Apply file exclusions + +Some files inflate raw size stats without representing reviewable human +authorship — generated code, lockfiles, mock outputs, vendored +dependencies. Exclude them from the threshold check, the seam analysis, +and the file counts. **Report them separately** so reviewers know they +were considered (transparency, not silent dropping). + +Build the exclusion glob list from up to three sources, in this order: + +1. **Bundled universal defaults** — read + `references/default-exclusions.md`. + Covers patterns generated in nearly every repo (lockfiles, common + Go/Python/JS generated outputs, protobuf bindings). +2. **Repo-local overrides** — if `.code-quality/size-review-exclude` + exists at the repo root, read it. Gitignore-style globs, comments + with `#`, blank lines OK. These *augment* the defaults (do not + replace). The repo file is where teams encode their own + conventions: ent ORM trees, Atlas migrations, OpenAPI bundles, + vendored SDKs. +3. **`.gitattributes` `linguist-generated=true`** — if the repo marks + files as generated via the GitHub linguist attribute, honor that. + Optional convenience layer. + +After resolving the union of globs, partition the diff file list: + +- **Excluded files**: drop from the threshold check and seam analysis. + Track them so the report can show counts and a brief sample. +- **Included files**: continue with these for everything downstream. + +When reporting stats post-exclusion, the report should show *both* the +raw counts and the post-exclusion counts so the impact of exclusions is +visible. Example: + +``` +Stats (slice): 33 files raw → 27 after exclusions, +1938/-209 raw → +697/-92 after exclusions +Excluded 6 files: openapi.gen.go (881 lines), openapi.apiclient.gen.go (322 lines), +go.sum (38 lines), apps/core-api/internal/mocks/firmware_service_mock.go (140 lines), +docker-compose.yaml (12 lines), .gitignore (4 lines) +``` + +### Step 3: Structural signals + +The strongest splittability signals are *structural*, not size-based. Read +each one and write down what you find: + +1. **Author-signaled stages** (strongest signal). Does the PR body + enumerate sub-sections like "Core Feature / S3 Integration / CLI + Uploader / Infrastructure / Testing / Documentation"? That's the + author already sketching a stack — they just shipped it as one PR. + Each section in their description is a candidate CR. + +2. **Commit phase structure**. Group commits by intent: refactor / feat / + test / chore / fix. Phases like "scaffold → implement → wire up" or + "rename → adapt → extend" indicate clean staging. Many "address + reviewer feedback" / "fix lint" / "merge from main" commits indicate + fixup noise — separable concern from the underlying intent. A long + commit history with phase boundaries (group of feats followed by + group of tests followed by group of fixups) is splittable; an + interleaved soup of commits is not. + +3. **File grouping by top-level directory**. Count files per top-level dir + (`apps/foo`, `tools/bar`, `infra/`, `pkg/`, `docs/`). Spread across + many top-level dirs is a strong split signal. Concentration in one + top-level dir is a weaker signal — the seams are subtler. + +4. **File-type taxonomy**. Source code / tests / infra-as-code / docs / + config / generated. Mixing infra-as-code (Pulumi, Terraform, helm) + with feature code is a common split point — infra can usually land + first or last independently. + +5. **Existing stacking signal**. From Step 1 — if the branch is already + stacked or the body says "Builds on #X", the author already practices + stacking and *this* PR may be the right size for its slice. The + recommendation should bias toward "no further split needed" unless + the slice itself is over threshold. + +### Step 4: Seam analysis + +For each candidate seam, verify it before recommending it: + +- Which **files and which commits** go on which side? +- **Direction of dependency**: what merges first? +- **Can the lower slice compile and test alone?** (If no, the seam is + fake and forcing a split there will produce broken intermediate states.) + +Common splittable seams (in rough order of how often they apply): + +- **Per-layer**: repository → service → handler → middleware/controller. + Common in layered backends. Each layer's tests can live in its own CR. +- **Per-app**: `apps/core-api` vs `apps/web` vs `tools/cli` vs `infra/`. + Multi-app PRs almost always have natural app-boundary seams. +- **Refactor-then-feature**: prep commits (renames, type changes, + introducing a helper) before feature commits that consume them. The + refactor lands first as a no-behavior-change CR, the feature builds on it. +- **Scaffold-then-implement**: stub handler / interface / type definition + before the real implementation. The scaffold can be reviewed for shape; + the implementation for correctness. + - **Watch out:** if the "stub" gets *replaced* rather than *built upon* + by the real implementation, this seam is fake — you'd be asking + reviewers to read code that's about to be thrown away. Genuine + scaffold-then-implement leaves the scaffold in place. +- **Test-infra-as-CR**: reusable test scaffolding (LocalStack helpers, + fixture factories, mock generators) merged before the tests that use + it. The test infra is a small, reviewable CR; the tests are noise once + you have the helpers. +- **Infra-vs-app-code**: Pulumi/Terraform/helm changes can almost always + land independently of application code, either before (provisioning + the resource) or after (wiring up the new endpoint). +- **Docs-as-separate-CR**: when docs add >100 lines and don't gate the + code behavior. Skip this seam if docs are small or tightly coupled + (e.g., openapi spec changes that drive code generation). +- **Drive-by extraction**: changes that don't belong in this PR at all — + schema renames, tangentially related fixes, unrelated cleanups + introduced by a merge or convenience commit. These should ship as + their own **independent PRs** (not as stack members of the main work), + because they carry no logical dependency on the main feature. Sign: + a commit or set of files that, if you removed them, the rest of the + PR would still hang together. Strong sign: the PR title doesn't + mention them. + +For each candidate seam, decide: **viable** (passes the three checks) or +**fake** (and why). Only viable seams enter the stack plan. + +### Step 5: Effort rating + +Rate the effort to actually perform the split. Be honest about restructuring +cost — recommending a split that takes the author 4 hours of git surgery +is bad advice if the review savings are 30 minutes. + +- **Easy** — Commits already partition along the seam. A `git rebase -i` + or `gs branch split` produces clean slices with no conflicts. Each + slice independently compilable and testable. Author can execute the + split in under 30 minutes. + +- **Moderate** — Some commits need to be split (one commit touches files + on both sides of the seam). 1-2 cross-cutting fixups need extraction. + Tests partially mixed with code but separable. Author needs an hour + or two and some `git rebase -i` comfort. No deep conflict risk. + +- **Difficult** — Deeply intermixed commits (every commit touches both + sides of every seam). Schema migrations coupled with code changes. + Very large commit count (50+) with heavy fixup noise that must be + squashed before slicing. Cross-cutting refactor where each layer + depends on the layer below — seams exist but each slice is still + large. Author needs half a day or more, with real conflict risk and + the chance of breaking intermediate states. + +**Critical distinction: file-level seams vs commit-level seams.** + +A seam can be **viable at the file level** (Slice A's files are clean and +self-contained, Slice B's files are clean and self-contained, no shared +files) but **fake at the commit level** (every commit touches files in +both slices, so cherry-picking commits would produce broken or partial +slices). When this happens, the author has to **rewrite** the commits, +not reorder them — squash everything down, then re-split along the file +seam by hand. This is the single most common reason effort jumps from +moderate to difficult. + +If file seams are clean but commit seams are tangled, say so explicitly +in the report. Example: "Files partition cleanly along [per-layer], but +the 23 feature commits each touch both repository and service layers — +splitting requires rewriting commit history (squash + re-split), not +reordering." + +Effort and splittability are independent axes. A PR can have very strong +file-level seams but be **difficult** to split because commits are +tangled. Conversely, a PR can be borderline splittable but **easy** to +split because the author already staged commits cleanly. + +### Step 6: Recommendation + +Combine splittability and effort: + +| Splittable | Effort | Recommendation | +|------------|--------|----------------| +| No (under threshold post-exclusions) | n/a | Ship as-is. | +| No (over threshold but cohesive — single feature, no seams) | n/a | Ship as-is. Note in the report why the size is justified so reviewers know it was considered. | +| Yes | Easy | **Split now.** Concrete stack plan below. | +| Yes | Moderate | Split if review is bottlenecked, change is high-risk, or the author has time. Otherwise note seams explicitly so the *next* PR in this area is shaped better. | +| Yes | Difficult | Usually ship and learn — the cost of restructuring exceeds the reviewer benefit. Split only if the change is high-stakes (security-sensitive, schema migrations on prod data, public API surface) where reviewer mistakes are costly. | + +**Two pre-merge improvements, not one.** Splitting into multiple PRs and +cleaning up commit history are *separable* concerns: + +- **PR split** = restructure the work into multiple Change Requests with + their own review threads. Often medium-to-high effort, sometimes high + value (review fatigue, blast radius, rollback granularity). +- **Commit cleanup** = squash fixup/lint/CI-debug/merge-from-main commits + into the feature commits they correspond to. Always low effort + (5-30 minutes with `git rebase -i`), reliably high value (reviewers + see intent, not iteration noise). + +If a PR has heavy fixup noise (10+ "address review feedback", "fix lint", +"CI debug" commits) but is otherwise the right size and shape, the +recommendation should be **commit cleanup, not split**. Both can also +apply — commit cleanup often needs to happen *before* a split is +mechanically possible. + +**Prefer git-spice-style stacked CRs over independent PRs when**: +- Slices share a logical thread (refactor enabling feature) +- Lower slice is small enough to land fast, but reviewers can see the + end goal in the stack +- The author can use `gs stack submit` to push the whole stack at once + and get review on the bottom while writing the top + +**Prefer independent PRs when**: +- Slices have no dependency (e.g., `apps/core-api` change vs + `tools/bacstackcli` change with no shared code) +- One slice could reasonably land days or weeks before the other +- Slices have different reviewers or owners +- The seam is a drive-by extraction (unrelated work that snuck in) + +### Step 7: Stack plan (only if recommending split) + +Produce a concrete plan the author can execute. Each row in the stack: + +- A **draft CR title** (conventional commits style if the repo uses it) +- The **files or path globs** that belong in the CR +- The **kind** (refactor / scaffold / feature / infra / tests / docs) +- **Depends on** — the parent CR (or `trunk` for the bottom of the stack) + +Order the stack from base to top. Bottom CR depends on trunk; each +subsequent CR depends on the previous (or any earlier CR if the stack +branches). + +--- + +## Output Format + +For PRs over threshold: + +```markdown +## Size Review: or + +**Stats:** +- vs trunk (cumulative): N files raw → M after exclusions, +adds/-dels raw → +adds/-dels after exclusions, K commits +- vs (slice): N files raw → M after exclusions, +adds/-dels raw → +adds/-dels after exclusions, K commits + (omit slice line if not stacked) +- Excluded N files: + +**Verdict:** [Splittable — easy / Splittable — moderate / Splittable — difficult / Over threshold but indivisible / Already split — appropriately sized at slice level] +**Recommendation:** [Split now / Note seams for next time / Commit cleanup only / Ship as-is] + +### Shape + +<3-5 bullet observations from Step 3's structural signals. What shape is +this PR? Why is it the size it is? What did the author signal? If +existing stack, note that.> + +### Seams Identified + +| # | Seam type | Slice A | Slice B | Direction | Viability | +|---|-----------|---------|---------|-----------|-----------| +| 1 | refactor-then-feature | rename commits in pkg/foo/ | feat commits in apps/bar/ | A first | viable | +| 2 | infra-vs-app-code | infra/foo/components/ | apps/bar/api/handlers/ | independent | viable | +| 3 | drive-by extraction | unrelated phone validation files | rest of PR | independent PR | viable | + +For each seam, note whether it's **viable** or **fake** (and the reason). +Only viable seams should drive the stack plan. + +### Effort: [Easy / Moderate / Difficult] + + +- Commit-level vs file-level fakeness (the critical distinction from Step 5) +- Conflict risk if commits get reordered or split +- Test-infra coupling (do tests need to move with the code they cover?) +- Drive-by extractions that need to come out separately + +### Stack Plan + +(Only present this section if recommendation is "Split now" or "Split if +review is bottlenecked".) + +| CR | Title | Files | Kind | Depends on | +|----|-------|-------|------|------------| +| 1 | refactor(foo): rename Bar to Baz | pkg/foo/*.go | refactor | trunk | +| 2 | feat(bar): add new endpoint | apps/bar/api/*.go | feature | CR1 | +| 3 | test(bar): cover new endpoint | apps/bar/api/*_test.go | tests | CR2 | + +If the recommendation involves drive-by extraction, list those as +**independent PRs** in a separate sub-section, not as stack members. + +### Suggested git-spice flow + +\`\`\`bash +# If the PR has heavy fixup noise, squash by section first: +git rebase -i +# squash fix/lint/CI-debug commits into their parent feature commits + +# Then start the stack from trunk: +git checkout +gs branch create refactor-foo-rename --target main +# cherry-pick or restack the relevant commits +gs branch create feat-bar-add-endpoint --target refactor-foo-rename +# ... +gs stack submit +\`\`\` + +If a compatible git-spice stacking workflow is available in the current Pi installation, +point them at it for the full workflow. +``` + +For PRs under threshold: + +```markdown +## Size Review: or + +**Stats:** N files raw → M after exclusions, +adds/-dels, K commits +(if exclusions were significant: "size signal dominated by N excluded +generated files") +**Verdict:** Appropriately sized +**Recommendation:** Ship as-is. +``` + +--- + +## Common pitfalls to avoid + +- **Don't anchor on raw size**. A 1500-line cohesive feature with clean + commits is more reviewable than a 400-line cross-cutting refactor with + 50 fixup commits. Use the structural signals from Step 3. +- **Don't recommend splits that produce broken intermediate states**. + Verify the lower slice can compile and test alone before listing it + as CR1. +- **Don't ignore an existing stack**. If the branch is already stacked + on a non-trunk parent or the PR body says "Builds on #X", the author + already practices stacking. Don't tell them to split unless the slice + itself is too big. +- **Don't conflate effort with value**. A high-effort split can be the + right call for high-stakes changes; a low-effort split can be wasted + motion for cohesive ones. +- **Don't recommend stacking when independence is the right shape**. If + two slices truly don't depend on each other (different apps, different + concerns, drive-by extraction), separate independent PRs are simpler + than a stack — reviewers don't have to track the dependency. +- **Don't manufacture seams**. If the analysis reveals no viable seam, + say so directly and recommend ship-as-is. A "splittable but not + worth the effort" verdict is honest; inventing a contrived split is + not. +- **Don't forget commit cleanup as a separate lever**. If the PR is the + right size but the commit history is noisy, recommend a `git rebase -i` + squash pass — that's a reviewability win independent of any split. +- **Don't silently drop excluded files**. The report should always show + what was excluded and why so the reviewer can sanity-check the + exclusion list. + +--- + +## Output Actions + +After producing the report, ask the user how to surface it. In interactive Pi +sessions, use `ask_user_question` when available; otherwise ask in chat. In +explicit CI/non-interactive contexts, do not call interactive question tools. +Prefer PR comments for PR-scoped reviews and inline/markdown output when no PR +is detected: + +- **Inline** — return the markdown in the chat. Default for ad-hoc reviews. +- **PR comment** — post the report as a top-level review comment on the + GitHub PR (`gh pr comment --body-file `). Default + for PR-scoped reviews. +- **Branch + markdown** — write to `SIZE_REVIEW.md` and commit on + a `/size-review` branch for archival. + +If a stack plan was produced and the user wants to act on it, offer to +hand off to an available git-spice stacking tool or skill in the current Pi +installation, which can drive the actual split end-to-end. If no such tool is +available, leave the stack plan as executable guidance for the author. diff --git a/packages/pi-code-quality/skills/size-review/references/default-exclusions.md b/packages/pi-code-quality/skills/size-review/references/default-exclusions.md new file mode 100644 index 0000000..6343c07 --- /dev/null +++ b/packages/pi-code-quality/skills/size-review/references/default-exclusions.md @@ -0,0 +1,86 @@ +# Universal default exclusions for size-review + +Files that match these globs are excluded from PR-size threshold checks +and seam analysis. They represent generated content, lockfiles, or +machine output rather than reviewable human authorship. + +Repos can supply additional exclusions in `.code-quality/size-review-exclude` +at the repo root using the same gitignore-style format. The repo file +*augments* this list (does not replace). + +Format: gitignore-style globs. `**` matches any directory depth. +Lines starting with `#` are comments. Blank lines are ignored. + +```gitignore +# --- Lockfiles --- +go.sum +**/go.sum +go.work.sum +**/uv.lock +**/poetry.lock +**/Pipfile.lock +**/package-lock.json +**/yarn.lock +**/pnpm-lock.yaml +**/bun.lockb +**/Cargo.lock +**/composer.lock +**/Gemfile.lock + +# --- Common Go generated patterns --- +**/*.gen.go +**/*_gen.go +**/zz_generated_*.go +**/zz_generated.*.go +**/mock_*.go +**/*_mock.go +**/mocks/*.go + +# --- Protobuf / gRPC generated bindings --- +**/*.pb.go +**/*.pb.gw.go +**/*_pb2.py +**/*_pb2_grpc.py +**/*.pb.cc +**/*.pb.h + +# --- TypeScript / JavaScript generated --- +**/*.d.ts.map +**/*.js.map +**/*.css.map +**/dist/** +**/build/** + +# --- Python compiled / cache --- +**/__pycache__/** +**/*.pyc + +# --- Generated OpenAPI / Swagger output --- +# Note: hand-written OpenAPI source files (api_index.yaml, etc.) should NOT +# be excluded; only the generated bundle output. Repos that use Redocly or +# similar bundlers should add the bundle path to their repo override file. + +# --- Generated GraphQL schemas / clients --- +**/*.generated.ts +**/*.generated.tsx +**/__generated__/** +``` + +## Notes for skill consumers + +- These defaults bias toward **safe exclusions** — patterns that are generated + in nearly every repo that uses the language/tool. Repo-specific generated + trees (ent ORM, Atlas migrations, custom codegen output, Postman bundles, + vendored SDK output) belong in the per-repo override file. +- When in doubt, *don't* exclude something at the universal level. A + false negative (counting generated code as size) is recoverable with a + repo override. A false positive (silently dropping a hand-written file + from review-size analysis) is harder to catch. +- The repo override file at `.code-quality/size-review-exclude` is where + teams encode their own conventions. Examples of patterns that commonly + belong there: + - `apps/*/ent/*.go` (Ent ORM generated tree, except `schema/` which is hand-written) + - `**/migrations/*.sql` (Atlas-generated migrations) + - `**/postman/*.json` (Postman collection exports) + - `**/api.yaml` if it's a Redocly-bundled output (but not if it's the source of truth) + - Vendored SDKs, generated client libraries, build artifacts checked into the repo diff --git a/packages/pi-code-quality/skills/slop-review/SKILL.md b/packages/pi-code-quality/skills/slop-review/SKILL.md new file mode 100644 index 0000000..9d1dc56 --- /dev/null +++ b/packages/pi-code-quality/skills/slop-review/SKILL.md @@ -0,0 +1,1241 @@ +--- +name: slop-review +description: Detect low-quality AI-generated code, idiom drift, code quality issues, and architecture/solution-fit problems in files, directories, branches, PRs, or whole codebases. Use when the user asks to review code for slop, AI-written patterns, idiomaticity, solution fit, maintainability, or code quality. +license: MIT +--- + +# AI Slop Review + +Identify low-quality, likely AI-generated code and solution-level slop through a 4-lens +parallel review architecture. Specialized agents scan in parallel for AI authorship +signals, idiom violations, code quality issues, and whether the implementation is the +right solution to the problem. A calibration agent then scores and filters findings. +Only findings that survive calibration appear in the final report. + +## Why four lenses matter + +A single reviewer either blurs concerns together (mixing "is this AI-generated?" with +"is this good code?" and "is this the right solution?") or anchors too heavily on one +dimension. The 4-lens architecture separates these concerns so each agent can focus +deeply: + +- **Phase 1a (AI Authorship Detection):** Looks for patterns that betray machine + generation -- contextual blindness, boilerplate residue, aspirational documentation, + mechanical uniformity. Not a code review; a forensic analysis. +- **Phase 1b (Idiom Fluency):** Checks whether the code reads like it was written by + someone fluent in the language and its ecosystem. Compares against the project's own + idiom baseline, not abstract ideals. +- **Phase 1c (Code Quality):** Traditional quality review -- dead code, stale docs, + debug artifacts, test quality, security, DRY violations. Deliberately agnostic about + whether code is AI-generated. +- **Phase 1d (Architecture and Solution-Fit):** Asks whether the implementation should + exist in this shape. Locally clean code can still be slop if it patches a symptom, + chooses the wrong owner, or ignores an existing tool or framework mechanism. + +After the parallel scan, a calibration agent scores every finding on a 0-100 scale, +cross-references across lenses, and produces a filtered, verdict-bearing report. + +The review must answer two separate questions: + +- Is the code locally slop? +- Is the solution itself slop? + +## Execution Model and Model Tier Intent + +This skill is portable across Pi installations. Pi does not guarantee a built-in subagent tool, so preserve the review methodology while adapting to the tools available in the current session. + +| Step | Review role | Model-tier intent | +|------|-------------|-------------------| +| Step 0 | Scope, problem reconstruction, context gathering, idiom baseline | small/fast model equivalent when supported | +| Phase 1a | AI Authorship Detection | strongest available reasoning model or large tier | +| Phase 1b | Idiom Fluency | strongest available reasoning model or large tier | +| Phase 1c | Code Quality | standard review model | +| Phase 1d | Architecture and Solution-Fit | strongest available reasoning model or large tier | +| Phase 2 | Calibration | strongest available reasoning model or large tier | +| Phase 3 | Synthesis | inline in the current agent | + +### Execution ladder + +1. If the current Pi session exposes a parallel task/subagent tool, run Step 0 first, then run the applicable Phase 1 lenses in parallel with the tailored context bundles defined below. +2. If no parallel task/subagent tool is available, run the same Phase 1 lenses sequentially in the current agent context. Keep each lens's findings structured and separated exactly as if separate agents produced them. +3. Use model selection only when the available tool supports it. Do not hardcode provider-specific model names that are unavailable in the current Pi session. + +The scoring, calibration, and final report format are identical in parallel and sequential execution. + +## Workflow + +### Step 0: Determine scope, reconstruct the problem, gather context, and build idiom baseline + +Launch a subagent with `model: "haiku"` for this step. + +**Scope:** Determine what to review based on the user's request: +- If the user specifies files/directories, use those +- If the user says "review this PR" or "review my changes", use `git diff` to identify changed files +- If the user says "review the codebase" or similar broad request, scan `src/` or the main + source directory, applying the exclusion list defined under "Definition of 'after + exclusions'" below. Hand-authored schema sources (e.g. `pkg/ent/schema/`, + `prisma/schema.prisma`, `*.proto`, `migrations/*.sql`) are always in scope even when + the rest of their generated output is excluded + +**Problem reconstruction** (do this before any review -- it prevents solution-level false negatives): + +For PRs and non-trivial changes, produce a short problem statement before launching Phase 1: + +1. Identify the stated problem from PR title, description, linked issues, commits, and + human reviewer comments +2. Identify the inferred actual failure mode from changed code, tests, logs, commands, + and reproduction evidence +3. Identify existing mechanisms that already own the problem area: framework features, + package managers, build tools, platform APIs, repo scripts, or established team flows +4. Identify the minimal solution that would solve the problem without new abstractions +5. Record unanswered questions where the PR does not explain why the chosen approach is necessary + +For PR reviews, always read human reviewer comments before final grading. Treat comments +as context signals about requirements, missing evidence, tool mental models, and +solution-level objections -- not just as line-level code review inputs. + +When PR comments include phrases like "why", "what problem", "anti-pattern", "wrong +layer", "should just work", "too much baggage", "AI fix this", or "do we need this", +route them to Phase 1d. These are usually architecture or solution-fit objections. + +**Context gathering** (do this before any review -- it prevents false positives): + +1. Read any project guidance files in the repo root and relevant subdirectories, especially + `CLAUDE.md`, `AGENTS.md`, `README.md`, and contributor docs that define conventions, + style rules, or architectural decisions +2. Sample 2-3 existing files in the same directory/package as the code under review to + establish the project's baseline patterns: + - Error handling style (how does this project handle errors?) + - Import conventions (aliased? grouped? sorted?) + - Naming patterns (camelCase? snake_case? abbreviations?) + - Logging approach (which logger? structured? what level conventions?) + - Test style (table-driven? fixtures? mocks? what framework?) +3. Detect the primary language(s) and load the appropriate reference file(s) from + `references/` -- only read reference files + for languages actually present in the review scope + +**Idiom baseline** (document this explicitly so Phase 1b has a concrete reference): + +Produce a structured idiom baseline for each language in scope. This baseline is the +authority for Phase 1b -- anything matching it is NOT flagged. Include: + +- **Language version:** e.g., Go 1.22, Python 3.12, Rust 2021 edition +- **Modern features in use:** e.g., `slog` vs `log`, `itertools` usage, `?` operator patterns +- **Stdlib preferences:** which standard library packages the project favors over third-party alternatives +- **Error handling convention:** e.g., sentinel errors vs custom types, `errors.Is`/`As` usage, bare `except` policy +- **Test framework:** e.g., `testing` + `testify`, `pytest`, `rstest` +- **Import conventions:** grouping order, aliasing patterns, relative vs absolute +- **Naming conventions:** abbreviation norms, exported/unexported patterns, file naming + +**Acceptance file** (skip if absent): + +If `.code-quality/slop-acceptances.md` exists at the repo root, read it and store the +verbatim contents alongside the rest of Step 0 output. The file is a project-level +"do not bring this up again" list, written by the maintainers, that tells Phase 2 +calibration to dismiss findings that match an entry. Only Phase 2 needs the file — +Phase 1 lenses scan blind so they still produce evidence the maintainers can +re-evaluate when removing an entry. + +If the file is missing, Phase 2 grades normally with no acceptances applied. Do not +fabricate acceptances; do not infer them from CLAUDE.md or other docs. + +**Scope adaptation for PR reviews:** + +When reviewing a PR, also gather the base branch versions of changed files so that +Phase 1 agents can distinguish between pre-existing patterns and newly introduced ones. +Use `git show :` for each changed file. + +Also gather the PR title, description, linked issues, commit list, changed-file list, and +human reviewer comments. Prefer `gh pr view --comments` plus the appropriate `gh api` +review-comment endpoints when available. + +Store all gathered context (problem reconstruction, codebase context, idiom baseline, +base branch files, reviewer comments, and acceptances file if present) -- all Phase 1 +agents and Phase 2 need it (Phase 1 sees everything except the acceptances file). + +**Phase 1d decision** (required output of Step 0): + +Before launching Phase 1, emit a structured `Phase 1d Decision` block that evaluates +the force-include triggers and skip criteria from the "Phase 1d decision" section +below. The block must show the file count, authored line count, exclusions applied, +which trigger (if any) fired, and the verdict. A skip with no checklist evaluation +is not allowed — when in doubt, mark `REQUIRED`. This block exists so the decision +is auditable rather than buried inside one orchestrator turn. + +--- + +### Phase 1: Parallel 4-lens scan + +Launch the applicable subagents in parallel. **Tailor each lens's context bundle** to +what that lens actually needs — broadcasting the full Step 0 context to every agent +multiplies input cost by 4× without adding signal. Each lens's prompt below specifies +which context elements to include. + +**Important:** Use available generic parallel agent tools when present. Do not require a specialized review agent or an Arc-specific agent. If no parallel agent tool is available, run the four lenses sequentially and keep their outputs separated by phase. The methodology is required; the parallelism is opportunistic. + +For large reviews (>10 files), split each lens across multiple parallel subagents by +directory or module. Phase 1d should stay cross-cutting unless the PR spans genuinely +independent systems. + +**Per-lens context budget** (deliver these subsets to each lens, not the full bundle): + +| Lens | Files under review | Base branch files | Project guidance | Idiom baseline | Reviewer comments | Problem reconstruction | Language refs | Acceptances | +|------|:------------------:|:-----------------:|:----------------:|:--------------:|:-----------------:|:----------------------:|:-------------:|:-----------:| +| Phase 1a (AI Authorship) | ✓ | ✓ | ✓ | – | – | – | – | – | +| Phase 1b (Idiom Fluency) | ✓ | ✓ | ✓ | ✓ | – | – | ✓ | – | +| Phase 1c (Code Quality) | ✓ | – | ✓ | – | – | – | – | – | +| Phase 1d (Solution-Fit) | ✓ | ✓ | ✓ | – | ✓ | ✓ | – | – | +| Phase 2 (Calibration) | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | + +The "✓" columns are required for that lens's analysis; "–" elements would be ignored or +add noise. Phase 2 calibration receives the union (it's the cross-lens synthesis step +and must see what every lens saw). Acceptances are deliberately Phase-2-only: Phase 1 +lenses scan blind so the underlying evidence stays visible to maintainers reviewing +whether to keep an acceptance. + +### Phase 1d decision: when to require, when to skip + +Phase 1d is the most reasoning-heavy lens, but it is also the only one that asks +*"should this code exist in this shape?"*. Skip it too eagerly and you grade +locally clean code as A while the solution itself was misframed. + +**Step 0 MUST emit a `Phase 1d Decision` block** in its output containing: + +- File count and authored line count after exclusions (show the math, not just + the result) +- Which exclusions were applied (list the paths/categories dropped) +- Which force-include trigger fired, if any +- Verdict: `REQUIRED` or `SKIPPED` +- One-sentence justification + +If the verdict cannot be expressed as a checklist evaluation in this block, default +to `REQUIRED`. The decision is auditable; do not skip silently. + +#### Force-include triggers (any one fires → Phase 1d is REQUIRED) + +If any of these is true, Phase 1d runs regardless of size: + +- The diff adds or modifies a **schema definition** — ORM schemas + (ent/Prisma/SQLAlchemy/GORM/Diesel models), GraphQL schemas, OpenAPI specs, + protobuf, JSON Schema, or any file under `migrations/`, `db/migrate/`, or + matching `*.sql` +- The diff adds a new **repository, service, handler, controller, command, or + background-worker** file — these are abstraction-boundary decisions even at + small line counts +- The diff modifies **build/tooling/dev-experience config** — `Makefile`, + `mise.toml`, `.tool-versions`, `package.json` scripts, `pyproject.toml` build + config, CI workflow files (`.github/workflows/*`, `.gitlab-ci.yml`), + `Dockerfile`, devcontainer, or `*.nix` files +- The diff touches **multiple architectural layers** in a single change + (schema + repository + migration; or handler + service + repository; etc.) +- Reviewer comments include solution-level signals — phrases like "why", + "what problem", "anti-pattern", "wrong layer", "should just use", "do we + need", "too much baggage", "AI fix this" + +#### Skip criteria (Phase 1d may be skipped only when ALL hold AND no trigger fired) + +- Fewer than 5 **authored** files changed (after exclusions, see below) +- Fewer than 100 **authored** lines added (after exclusions) +- No PR title/body mention of: workflow, CI, scripts, infra, deploy, migration, + refactor, dependency, build, tooling, abstraction, layer, pattern, schema, + data-layer +- No reviewer comments raising solution-level objections (see trigger list above) + +Soft-sounding rationalizations like "pure data-layer add" or "just adding an +entity" are themselves Phase 1d judgments — if you find yourself reaching for +one, the answer is that Phase 1d should run, not that it can be skipped. + +#### Definition of "after exclusions" + +This definition is shared with the "review the codebase" scope rule above — +applying different exclusion lists in those two places is what produced the +PR-435 regression where `pkg/ent/schema/` was nearly missed by the count even +though the schema is the *source* the rest of `pkg/ent/` is generated from. + +The principle: **exclude generator output, never exclude generator inputs.** +ORMs and codegen tools have a small hand-authored source surface (the schema) +and a large generated surface. Phase 1d cares about the source. + +When counting authored files and lines, **exclude**: + +- Suffix-tagged generated files: `*_generated.go`, `*.gen.go`, `*.pb.go`, + `*_pb2.py`, `*_pb2_grpc.py`, `*_mock.go`, `*.g.dart`, `*.freezed.dart` +- Whole generated directories — everything under the directory **except** the + hand-authored schema subdirectory: + - **ent (Go):** everything under `pkg/ent/` (or wherever ent generates) **except + `pkg/ent/schema/`**, which is the hand-authored source and is in scope + - **Prisma:** `node_modules/.prisma/`, `**/generated/` — but `prisma/schema.prisma` + is in scope + - **sqlc:** generated `db/sqlc/*.go` (or wherever the config emits) — but the + `*.sql` query files and `sqlc.yaml` are in scope + - **oapi-codegen / openapi-generator:** the generated client/server code — but + the OpenAPI spec is in scope + - **protobuf:** the generated `*.pb.go` / `*_pb2.py` — but the `*.proto` files + are in scope + - **Diesel (Rust):** `src/schema.rs` (printed by `diesel print-schema`) — but + the migration SQL is in scope +- Atlas/migration tool emissions: `atlas.sum`, `migrate.sum` +- Lockfiles: `go.sum`, `package-lock.json`, `yarn.lock`, `pnpm-lock.yaml`, + `Cargo.lock`, `poetry.lock`, `uv.lock`, `Gemfile.lock` +- Snapshot test fixtures, golden files, and large data fixtures +- Vendored dependencies: `vendor/`, `third_party/`, `node_modules/` + +**Do NOT exclude** (keep in scope even if they live under a "generated" tree): + +- Hand-authored schema sources: `pkg/ent/schema/*`, `prisma/schema.prisma`, + `*.proto`, `*.graphql`, `*.sql` query files, hand-edited migration SQL +- Build/tooling config: `Makefile`, `mise.toml`, `.tool-versions`, CI workflows, + `Dockerfile`, codegen config (`sqlc.yaml`, `oapi-codegen.yaml`, `buf.yaml`) +- Tests, domain types, repositories, services, handlers, business logic + +If you cannot tell whether a file is generated, default to **including** it. +Underestimating authored surface drops Phase 1d for changes it should review; +overestimating only adds one Opus pass. Quick generated-file tells: a header +comment like `// Code generated by ... DO NOT EDIT.`, file size disproportionate +to apparent intent (a 600-line CRUD file from a 40-line schema), or sibling +files that share suspiciously uniform structure. + +For tiny local edits that pass all skip criteria with no force-include trigger +(the prototypical case: a one-file bug fix or a comment cleanup), Phase 1a + 1b ++ 1c are sufficient — solution-fit objections don't apply at that scope. + +#### Phase 1a: AI Authorship Detection (model: "opus") + +> You are an AI authorship forensic analyst. Your sole job is to identify code that was +> likely generated by an AI assistant rather than written by a human developer. You are +> NOT doing a general code review. Ignore human-style mistakes -- typos, inconsistent +> spacing, TODO hacks, quick-and-dirty solutions. Those are human signals, not problems +> for you to flag. +> +> Focus exclusively on these AI authorship signals: +> +> 1. **Contextual blindness** -- code that is locally coherent but unaware of its +> surroundings: different error handling than the file it lives in, a utility that +> duplicates one nearby, an abstraction that ignores established patterns, a different +> logger/serializer/HTTP client than everything else uses. This is the strongest signal. +> 2. **Boilerplate residue** -- scaffolding, placeholder comments, template structure that +> was never customized. Code that looks like it was accepted from a suggestion without +> adaptation. +> 3. **Aspirational documentation** -- docstrings/comments that describe what the code +> *should* do rather than what it *does*. README sections that describe features not +> yet implemented. Comments that are more detailed than the code warrants. +> 4. **Over-engineering** -- abstractions with one implementation, factory patterns used +> once, configuration for single-use code, defensive checks for impossible conditions. +> AI models build for generality; humans build for the case at hand. +> 5. **Uniform mechanical style** -- suspiciously consistent formatting, identical +> try/catch shapes across unrelated functions, uniform comment density. Human code +> has texture and variation. +> +> For each finding, report: +> - **File** and **line number(s)** +> - The specific **code snippet** +> - **Signal category** (one of the five above) +> - **Reasoning** -- why this pattern indicates AI generation rather than human authorship +> - **Confidence** (0-100) +> +> At the end, produce a **per-file authorship assessment**. Score with quality +> polarity — **higher = more human-like, lower = more AI-generated**: +> | File | Authorship Score (0-100) | Primary Signals | Notes | +> |------|-------------------------|-----------------|-------| +> +> Tag every finding with `[AI_AUTHORSHIP]`. Keep findings terse: 2-4 sentences each. +> Aim for under 5,000 tokens of total output — Phase 2 calibration consumes structured +> findings, not essays. + +#### Phase 1b: Idiom Fluency (model: "opus") + +> You are a language idiom expert. Your job is to identify code that is not idiomatic +> for its language, framework, and project context. You have the project's idiom baseline +> -- do NOT flag patterns that match the project's idiom baseline. Only flag deviations +> from established project conventions or from modern language best practices that the +> project has adopted. +> +> Focus on: +> +> 1. **Modern language features** -- using old patterns when the project's language version +> supports better alternatives (e.g., `os.Open` error handling without `errors.Is` in a +> Go 1.20+ project, manual loops instead of comprehensions in Python 3.10+) +> 2. **Stdlib usage** -- using third-party libraries for things the stdlib handles well, +> or using deprecated stdlib APIs when modern replacements exist in the project's version +> 3. **Error handling** -- patterns that deviate from the project's established convention +> (not from abstract ideals) +> 4. **Framework conventions** -- using a framework against its grain (e.g., fighting +> Dagster's asset model, bypassing Django's ORM patterns when the project uses them) +> 5. **Naming and structure** -- names that don't follow the project's conventions, +> file organization that breaks the established module structure +> +> For each finding, report: +> - **File** and **line number(s)** +> - The specific **code snippet** +> - **Signal category** (one of the five above) +> - **Idiomatic alternative** -- what the code should look like +> - **Reasoning** -- why the current code is non-idiomatic in this project's context +> - **Confidence** (0-100) +> +> Tag every finding with `[IDIOM]`. Keep findings terse: 2-4 sentences each. Aim for +> under 5,000 tokens of total output. + +#### Phase 1c: Code Quality (model: "sonnet") + +> You are a code quality reviewer. Your job is to find concrete quality issues -- dead +> code, stale documentation, debug artifacts, test problems, security concerns, and DRY +> violations. Tag every finding as `[CODE_QUALITY]`. Do not speculate about whether code +> is AI-generated -- that is another reviewer's job. Focus only on whether the code is +> correct, maintainable, secure, and well-tested. +> +> Focus on: +> +> 1. **Dead code** -- unused imports, unreachable branches, commented-out code, unused +> variables/functions +> 2. **Stale documentation** -- comments/docstrings that don't match the current code +> behavior, outdated README sections, wrong parameter descriptions +> 3. **Debug artifacts** -- leftover print statements, hardcoded test values, disabled +> tests, temporary workarounds marked TODO with no tracking +> 4. **Test quality** -- tests that don't test behavior, missing edge case coverage, +> mocks that mock too much, tests that would pass even if the code were broken +> 5. **Security** -- SQL injection, path traversal, hardcoded secrets, unsafe +> deserialization, missing input validation on external boundaries +> 6. **DRY violations** -- copy-pasted logic that should be extracted, duplicated +> constants, repeated patterns that indicate missing abstractions +> +> For each finding, report: +> - **File** and **line number(s)** +> - The specific **code snippet** +> - **Signal category** (one of the six above) +> - **Reasoning** -- what the concrete quality issue is +> - **Confidence** (0-100) +> +> Tag every finding with `[CODE_QUALITY]`. Keep findings terse: 2-4 sentences each. +> Aim for under 5,000 tokens of total output. + +#### Phase 1d: Architecture and Solution-Fit Review (model: "opus") + +Required for PRs and non-trivial changes. Optional for tiny single-file edits where the +user only asks about local code style and no architecture or workflow choice is involved. + +> You are an adversarial architecture and solution-fit reviewer. Your job is to decide +> whether the implementation is the right solution to the problem, regardless of whether +> the changed code is locally correct. +> +> Do NOT focus on formatting, style, or small bugs. Focus on whether the PR should exist +> in this shape. +> +> Review these dimensions: +> +> 1. **Problem fit** -- Does the PR solve the actual problem, or only a symptom? +> 2. **Abstraction boundary** -- Is the solution implemented at the right layer, or does +> it bypass the component, tool, or owner that should own the behavior? +> 3. **Existing mechanisms** -- Does the repo, framework, platform, package manager, or +> third-party tool already provide a better solution? +> 4. **Scope control** -- Does the PR spread one issue across too many files, docs, +> scripts, configs, workflows, or user surfaces? +> 5. **Maintenance cost** -- Does the solution create custom code that must track external +> behavior, file formats, CLI output, or conventions unnecessarily? +> 6. **Operational behavior** -- Does the solution change user workflows, CI behavior, +> failure modes, or target semantics in ways not justified by the problem? +> 7. **Evidence quality** -- Does the PR prove the problem and chosen solution, or does it +> look like an "AI fix this" response to a guessed root cause? +> 8. **Education opportunity** -- If the author seems to misunderstand a tool, framework, +> or architecture boundary, identify the missing mental model factually and +> non-personally. +> +> For each finding, report: +> - **File(s) or PR area involved** +> - The **claimed or inferred problem** +> - Why the solution is **mismatched or over-scoped** +> - The **existing mechanism or simpler alternative** +> - **Evidence** from the repo, docs, commands, or reviewer comments +> - **Confidence** (0-100) +> - **Severity**: Low, Medium, High +> +> At the end, produce a per-dimension table. Score with quality polarity — +> **higher = better fit, lower = worse fit**: +> +> | Dimension | Score (0-100) | Finding | Better Direction | +> |-----------|--------------:|---------|------------------| +> +> Tag every finding with `[SOLUTION_FIT]`. Keep findings terse: 3-5 sentences each +> (slightly longer than other lenses because architectural reasoning often needs +> explanation). Aim for under 7,000 tokens of total output. + +--- + +### Phase 2: Calibration review (model: "opus") + +Launch a **separate, independent** subagent with `model: "opus"`. This agent receives +ALL findings from all Phase 1 lenses, the original files, the problem reconstruction, +reviewer comments, the codebase context, and the idiom baseline. + +> You are a senior staff engineer performing calibration review. You are fair, precise, +> and allergic to false positives. Your job is to take findings from the parallel +> reviewers (AI Authorship, Idiom Fluency, Code Quality, Architecture and Solution-Fit) +> and produce a unified, calibrated assessment. +> +> **Accepted deviations.** If Step 0 supplied a `.code-quality/slop-acceptances.md` +> file, you MUST apply it before grading. For each pending finding from +> Phase 1a/1b/1c/1d: +> +> 1. Read the acceptances file as plain prose. +> 2. Decide whether the finding is substantively the same concern as any entry in +> the file. Use semantic judgment, not literal match — entries describe a class +> of finding (e.g., "plugin marketplace pinning"), not a specific finding ID. +> 3. If yes, set verdict = `DISMISSED (Accepted)` with a reason of the form +> "Accepted in slop-acceptances.md: ". +> 4. Do NOT include accepted findings in the main report tables, in the borderline +> appendix, or in the dismissed-findings collapse. Instead, list them once in a +> new "Accepted Deviations" section near the bottom of the report (see Output +> Format). +> 5. Suppressed findings still influence the per-file authorship table (they are +> evidence of AI-shaping the code) but do NOT contribute to per-file Idiom or +> Quality scores, and the matched solution-fit findings do NOT contribute to +> `solution_fit_score`. +> +> The acceptance file is the project owner's pre-registered "do not bring this up +> again" list. Trust it. If you genuinely think an entry is unsafe (e.g., it +> suppresses a real security issue or masks a regression), include a single +> `ESCALATED` finding flagging the acceptance itself with a clear reason — but the +> default posture is to honor the file. Never silently ignore an acceptance you +> disagree with; surface it. +> +> If no acceptances file was supplied, skip this step entirely and grade as normal. +> +> **For each finding, you must:** +> +> 1. Read the actual code at the referenced file:line +> 2. Read the surrounding context (the full function, the file's imports, nearby code) +> 3. Check the codebase context and idiom baseline -- does this project have a convention +> that makes this OK? +> 4. Assign a **confidence score (0-100)** using this rubric: +> - **0-25:** False positive. The finding is wrong or irrelevant. +> - **26-50:** Nitpick. Technically true but not worth acting on. +> - **51-70:** Low severity. Real issue but minor impact. +> - **71-85:** Verified real. Clear problem that should be fixed. +> - **86-100:** Confirmed critical. Significant issue affecting correctness, security, +> or maintainability. +> 5. Render a **verdict**: +> - **CONFIRMED** -- this is a real finding. Explain why it survives scrutiny. +> - **DOWNGRADED** -- real but less severe than the scanner claimed. Adjust score and explain. +> - **DISMISSED** -- false positive or nitpick. Explain what the scanner got wrong. +> - **ESCALATED** -- worse than the scanner realized. Explain the additional concern. +> 6. **Re-tag** if the finding was categorized under the wrong lens (e.g., an idiom +> finding tagged `[CODE_QUALITY]` should be re-tagged `[IDIOM]`). +> 7. Explicitly answer the solution-fit questions: +> - Could this code be locally acceptable but still the wrong solution? +> - Did the implementation choose the wrong owner or abstraction boundary? +> - Did reviewer comments reveal a system-level objection the code lenses missed? +> - Are there signs the engineer or AI assistant misunderstood a tool, framework, or +> repo convention? +> - Should the grade change because the solution is strategically poor even if the diff +> is small? +> +> **Cross-finding analysis:** +> +> After processing individual findings, perform cross-lens analysis: +> - **Missed findings:** Flag anything the Phase 1 scanners missed that you notice while +> verifying. The scanners may have been so focused on their checklists that they +> overlooked issues hiding in plain sight. +> - **Cross-lens patterns:** Identify cases where findings from different lenses +> reinforce each other (e.g., an `[AI_AUTHORSHIP]` contextual blindness finding +> combined with an `[IDIOM]` finding on the same code strongly suggests AI generation). +> Note these correlations explicitly. +> - **Solution-fit patterns:** Do not treat `[SOLUTION_FIT]` findings as optional +> appendices. If the implementation strategy is wrong, it must affect the top-line grade. +> - **Reviewer comment classification:** Classify each substantive human reviewer comment: +> +> | Status | Meaning | +> |--------|---------| +> | Supported | Evidence confirms the reviewer is raising a real solution or code issue. | +> | Partially supported | The concern is directionally right, but narrower or lower severity. | +> | Not supported | The reviewer concern does not hold after checking repo reality. | +> | Needs clarification | The PR does not contain enough evidence to decide. | +> +> **File-level authorship table:** +> +> Produce a per-file authorship assessment for EVERY file in scope, incorporating +> Phase 1a's assessments and your own calibration. Authorship Score uses quality +> polarity — **higher = more human-like, lower = more AI-generated**: +> +> | File | Authorship Score (0-100) | Calibrated Confidence | Key Signals | Verdict | +> |------|-------------------------|----------------------|-------------|---------| +> +> Your output is the complete calibrated finding list with scores, verdicts, reasoning, +> cross-lens correlations, reviewer-comment classifications, solution_fit_score, and the +> file-level authorship table. + +Provide the subagent with: +- All Phase 1a, 1b, 1c, and 1d findings +- The original files under review (so it can re-read them independently) +- The problem reconstruction, reviewer comments, codebase context, and idiom baseline from Step 0 + +--- + +### Phase 3: Synthesize, grade, and report + +Merge the calibrated findings into the output format below. Apply these +thresholds for finding inclusion. **These gate on per-finding *confidence* +(reviewer certainty that the finding is real), not on the file/quality +scores defined later in this section** — confidence and quality use the +same polarity (higher = stronger), but they're different axes. + +- **Confidence >= 70:** Include in the main report sections +- **Confidence 50-69:** Include in a borderline appendix +- **Confidence < 50:** Include in the dismissed findings section + +#### Grading algorithm + +Compute local code scores first, then combine them with solution-fit for the final grade. + +**Step 1: Per-file dimension scores (quality polarity — higher = better)** + +All dimension scores in this report use quality polarity: **100 = clean, +0 = pervasive slop**. Per-finding *confidence* values use the same convention +(higher = more confident the finding is real). Compute each dimension from +its confirmed findings via a density-weighted defect intensity, then invert +to quality polarity: + +- **Authorship Score** -- use the calibrated per-file score from Phase 2 (0-100, + higher = more human-like). If Phase 1a reported in probability-of-AI form, + convert via `authorship_score = 100 - ai_likelihood`. +- **Idiom Score** -- compute `defect = min(100, mean(finding_confidences) * (1 + log2(count)))` + over confirmed `[IDIOM]` findings for the file, then `idiom_score = 100 - defect`. + If no idiom findings, score is **100** (no detected violations). +- **Quality Score** -- compute `defect = min(100, mean(finding_confidences) * (1 + log2(count)))` + over confirmed `[CODE_QUALITY]` findings, then `quality_score = 100 - defect`. + If no quality findings, score is **100**. + +**Step 2: Weighted file score** + +``` +file_score = (0.10 * authorship_score) + (0.40 * idiom_score) + (0.50 * quality_score) +``` + +All inputs are quality-polarity, so `file_score` is too (higher = better). +Weights reflect that this is a *slop* review, not an *authorship* review. Good +AI-written code that follows idioms and has no quality issues should score high. +Authorship signals serve as corroborating evidence, not a primary driver. + +**Step 3: Local code rollup** + +``` +code_local_score = Σ(file_score * file_loc) / Σ(file_loc) +``` + +Weight by lines of code so a 500-line file with issues matters more than a 10-line +utility. + +**Step 4: Solution-fit score** + +Use the calibrated Phase 1d and Phase 2 result as `solution_fit_score` (0-100, +higher = better solution fit). If Phase 1d was not applicable because the scope +was a tiny local edit, omit `solution_fit_score` and use `code_local_score` as +the final score. + +For PRs and non-trivial changes: + +``` +final_score = (0.60 * code_local_score) + (0.40 * solution_fit_score) +``` + +For PRs whose purpose is architecture, tooling, workflows, infrastructure, developer +experience, or process, solution fit matters equally: + +``` +final_score = (0.50 * code_local_score) + (0.50 * solution_fit_score) +``` + +This matters because AI-generated PRs often have clean syntax and decent local hygiene +while choosing the wrong overall approach. + +**Step 5: Letter grade and verdict** + +Quality polarity — **higher score = better grade**: + +| Grade | Score | Verdict | +|-------|-------|---------| +| A | 81-100 | Clean | +| B | 61-80 | Mild concerns | +| C | 41-60 | Significant concerns | +| D | 21-40 | Strong slop signals | +| F | 0-20 | Pervasive slop | + +--- + +## Universal Slop Signals + +These apply to every language. The language-specific reference files add to these, +they don't replace them. + +### Structural tells +- Functions named after *what they do* rather than *what they represent* + (`processDataAndValidateInput`, `handleRequestAndReturnResponse`) +- Comments that restate the code verbatim -- no "why", only "what" +- Abstractions with exactly one implementation (premature interface/protocol/trait invention) +- Happy-path-only logic -- edge cases (nil/null/empty/zero/overflow) simply absent +- Hardcoded values that belong in config or named constants +- Inconsistent error message casing/formatting vs. the rest of the codebase + +### Defensive over-engineering +- `try/except` or error handling around operations that cannot fail in context +- Redundant nil/null checks on values the type system or caller already guarantees +- Validation of internal function arguments that are only called from trusted code +- Feature flags, backwards-compatibility shims, or configuration for single-use code +- Factory/builder/strategy patterns used exactly once + +### Documentation noise +- Docstrings that restate the function signature in prose ("Takes an X and returns a Y") +- `# increment counter` above `counter += 1` +- Module-level docstrings that describe what the file contains rather than why it exists +- Every function documented even when the name + signature is self-explanatory +- Type annotations in docstrings that duplicate the actual type annotations + +### Copy-paste signatures +- Multiple functions with near-identical parameter lists suggesting generated boilerplate +- Repeated structural patterns (same try/catch shape, same logging preamble) across + unrelated functions -- human code tends to vary more +- Suspiciously uniform formatting that doesn't match the rest of the file + +### Test quality signals +- Tests named `TestSuccess` / `TestFailure` / `test_basic` with no scenario specificity +- Mocks that mock so much they don't test anything real +- No property-based, table-driven, or parametrized tests where the problem calls for them +- Assertions that only check happy-path return values, never error payloads or side effects +- Missing coverage for concurrency, timeout, and cancellation paths +- Test functions that verify the code compiles/runs, not that it *behaves* correctly + +### The strongest signal: contextual blindness + +Code that would pass review in isolation but is clearly unaware of its surroundings: +- Different error handling style than the file it lives in +- A new utility function that duplicates one nearby +- A new abstraction that ignores the established codebase pattern +- A different logger, serializer, HTTP client, or ORM pattern than everything else uses +- Import style that doesn't match the rest of the project + +AI generates locally coherent code. It rarely generates *contextually* coherent code. +This is the single most reliable signal and should be weighted heavily. + +### Solution-level slop signals + +Generated work can look competent file-by-file while still choosing the wrong solution. +Flag these as `[SOLUTION_FIT]` when evidence supports them: + +| Signal | Description | +|--------|-------------| +| Symptom patching | The PR fixes the observed error but not the root cause. | +| Wrong owner | Logic is added outside the component, tool, or layer that should own it. | +| Custom wrapper over managed tool | New scripts parse or enforce behavior already owned by a package manager, framework, or platform. | +| Multi-surface workaround | One issue is patched in code, scripts, docs, and CI without proving why all are needed. | +| Evidence-free root cause | The PR assumes a cause but does not reproduce or verify it. | +| Defensive generality | A generic framework is created before there is a repeated need. | +| Policy split | Two commands or code paths now enforce different rules for the same concern. | +| Documentation as retrofit | Docs are updated to justify the new workaround rather than explain established team workflow. | + +Concrete regression scenario: `BACtrack/bacstack#430` +(`https://github.com/BACtrack/bacstack/pull/430`) should be treated as a pressure test. +The improved review should identify PATH/tool resolution drift as the actual problem, +check whether `mise exec -- ...` already provides the command execution boundary, mark a +custom `scripts/check_tool_version.sh` wrapper as the wrong solution boundary if evidence +confirms it, classify reviewer comments as solution-level signals, and downgrade the +overall grade even if local shell quality is acceptable. + +When identifying a skill or mental-model gap, phrase it as an education opportunity, not +personal criticism. Good: "The PR suggests a mise mental-model gap: `mise.toml` was +treated as a manifest to parse manually rather than making `mise exec` the execution +boundary for managed tools." Bad: "The author does not understand mise." + +--- + +## Output Format + +```markdown +## AI Slop Review: + +**Scope:** +**Grade:** [A-F] (/100) +**Local Code Score:** /100 +**Solution-Fit Score:** /100 or "Not applicable for this scope" +**Verdict:** [Clean / Mild concerns / Significant concerns / Strong slop signals / Pervasive slop] +**Confidence:** [High / Medium / Low] -- how confident the review is in its verdict + +> **Reading the scores:** All `/100` quality scores in this report use the +> convention **higher is better** — 100 = clean, 0 = pervasive slop. Per-finding +> *Confidence* values follow the same convention (higher = more confident the +> finding is real). Letter grades map intuitively: 90 → A, 50 → C, 10 → F. + +### Solution-Level Assessment + +| Dimension | Score | Finding | Better Direction | +|-----------|------:|---------|------------------| +| Problem understanding | 30 | ... | ... | +| Solution fit | 14 | ... | ... | +| Maintenance burden | 18 | ... | ... | +| Target ownership | 24 | ... | ... | +| Documentation scope | 35 | ... | ... | + +### Evidence Checked + +| Check | Observed Result | Assessment | +|-------|-----------------|------------| +| command, repo fact, reviewer comment, or code path | output/result | why it matters | + +### Reviewer Comment Classification + +| Comment | Status | Evidence | Assessment | +|---------|--------|----------|------------| +| reviewer concern | Supported / Partially supported / Not supported / Needs clarification | checked fact | what it means | + +### Education Opportunity + + + +### Solution-Fit Findings + +| # | Area | Signal | Finding | Better Direction | Confidence | Verdict | +|---|------|--------|---------|------------------|------------|---------| +| 1 | Makefile/scripts/docs | Wrong owner | description | use existing mechanism | 86 | CONFIRMED | + +### File-Level Assessment + +| File | LOC | Authorship (0.10) | Idiom (0.40) | Quality (0.50) | Score | Grade | +|------|-----|-------------------|--------------|----------------|-------|-------| +| path/to/file.go | 245 | 28 | 35 | 20 | 26.8 | D | + +### AI Authorship Signals +| # | File:Line | Signal | Finding | Confidence | Verdict | +|---|-----------|--------|---------|------------|---------| +| 1 | path:42 | Contextual blindness | description | 85 | CONFIRMED | + +### Idiom Violations +| # | File:Line | Signal | Finding | Idiomatic Alternative | Confidence | Verdict | +|---|-----------|--------|---------|----------------------|------------|---------| +| 1 | path:17 | Modern features | description | what it should look like | 78 | CONFIRMED | + +### Code Quality +| # | File:Line | Signal | Finding | Confidence | Verdict | +|---|-----------|--------|---------|------------|---------| +| 1 | path:99 | Dead code | description | 90 | CONFIRMED | + +### Positive Signals +- + +### Borderline Findings (confidence 50-69) +| # | File:Line | Lens | Finding | Confidence | Verdict | +|---|-----------|------|---------|------------|---------| + +### Accepted Deviations + + +| Topic | Phase 1 score | Lens | Acceptance reason | +|-------|--------------:|------|-------------------| +| | | AI / Idiom / Quality / Solution-Fit | | + +### Dismissed Findings + +``` + +If the code is clean or only has minor issues, say so directly. The goal is an honest, +calibrated assessment -- not finding problems for their own sake. + +--- + +## Language Reference Files + +Language-specific signals live in `references/`. +Only read the ones relevant to the code under review. Each reference file includes a +"What Idiomatic Looks Like" section that Phase 1b uses alongside the project's idiom baseline: + +**Pi/source-fidelity guard:** The language reference files preserve the source skill's modern-version guidance. Version-specific advice applies only when Step 0 confirms that the project uses that language version or has adopted that convention. If the project targets an older version or has an established local convention, the Step 0 idiom baseline wins and Phase 1b must not flag the reference default as an idiom violation. + +- `references/go.md` -- Go idioms, error handling, context propagation, concurrency +- `references/python.md` -- Python idioms, type hints, async, common footguns +- `references/rust.md` -- Rust ownership, error handling, type system, unsafe +- `references/svelte-ts.md` -- Svelte reactivity, SvelteKit patterns, TypeScript usage + +If the code is in a language not covered by a reference file, rely on the universal +signals and your general knowledge of that language's idioms. + +**Cost optimization — reference file caching.** These reference files are static between +runs against the same codebase. In runtimes that support prompt caching (Claude Code's +session cache, Anthropic SDK `cache_control` markers, etc.), include the loaded language +reference content in a cached prefix so repeat reviews against the same repo amortize +the input cost. In Claude Code this is automatic for skill content. In headless / CI +contexts (GitHub Actions via Pi/headless automation), set `cache_control: {"type": "ephemeral"}` +on the reference-file content blocks for the largest savings. + +--- + +## Adapting to the codebase + +Every codebase has its own conventions. Before flagging something as slop, check: + +1. **Project guidance** -- Do `CLAUDE.md`, `AGENTS.md`, `README.md`, or nearby contributor docs make this pattern OK? +2. **Existing code** -- Is this pattern used elsewhere in the project? If yes, it's a + convention, not slop -- even if it wouldn't be idiomatic in a greenfield project. +3. **Framework conventions** -- Some frameworks encourage patterns that look odd in + isolation (e.g., Dagster's `@asset` decorators, Django's class-based views). + Don't flag framework-conventional code as slop. +4. **Team size and stage** -- A 2-person startup codebase has different quality norms + than a 50-person team's production system. Calibrate accordingly. +5. **Acceptances file** -- Has the project pre-registered the concern in + `.code-quality/slop-acceptances.md`? If yes, Phase 2 will dismiss the finding + automatically; Phase 1 still scans blind so the evidence remains visible. + +The Phase 1 scanners should flag potential issues regardless. The Phase 2 calibration +reviewer is where this nuance gets applied. + +--- + +## Step 4: Output Actions + +After the review is synthesized, surface the findings. The default flow is: +**detect mode → detect a PR → ask the user (only when interactive) → render and deliver**. + +### 4.1 Detect interactive vs. non-interactive (CI/CD) mode + +The skill runs in two contexts: + +- **Interactive** — a human is in the loop (Pi session, IDE + extension). `ask_user_question` works. +- **Non-interactive** — running headless in CI/CD (GitHub Actions via the + Pi/headless automation, scheduled cron job, automation). `ask_user_question` + has no human to answer it; either it errors or it stalls the job. + +Detect non-interactive mode in Pi if any of these are true: + +```bash +[ "${CI:-}" = "true" ] || \ +[ "${GITHUB_ACTIONS:-}" = "true" ] || \ +[ "${GITLAB_CI:-}" = "true" ] || \ +[ "${BUILDKITE:-}" = "true" ] +``` + +Also treat the run as non-interactive if the user explicitly asks for "non-interactive mode", "headless", "CI mode", or "auto-post". Do **not** use `[ ! -t 0 ]` in Pi; tool subprocesses may be non-TTY even during an interactive session. + +In non-interactive mode: + +- **Skip `ask_user_question` entirely.** Never call it — it is interactive + by design. +- **Default behavior depends on PR detection** (next section): + - PR detected → post the rendered PR comment automatically. + - No PR detected → write `SLOP_REVIEW.md` to the working directory and + additionally print a one-line summary (verdict + grade + final score) + to stdout so the CI log captures it. +- **Never prompt for confirmation before posting.** In CI the user has + already opted in to auto-posting by triggering the workflow; an + unanswered confirm would block the job. +- **Surface failures visibly.** If `gh pr comment` fails (auth, rate + limit, repo permissions), exit non-zero with the error so the workflow + step fails loudly. Do not silently fall back. + +### 4.2 Detect whether a PR exists + +Determine whether a pull request is in scope, in priority order: + +1. **Explicit PR in the original request.** If the user passed a PR number + (`/code-quality-slop #436`) or URL, use it directly. +2. **GitHub Actions event payload.** If running under GitHub Actions and + the triggering event is a pull request, read the PR number from + `GITHUB_EVENT_PATH`: + + ```bash + if [ -n "${GITHUB_EVENT_PATH:-}" ] && [ -f "$GITHUB_EVENT_PATH" ]; then + jq -r '.pull_request.number // empty' "$GITHUB_EVENT_PATH" + # repo: $GITHUB_REPOSITORY (owner/name) + fi + ``` + + This works on `pull_request` and `pull_request_target` triggers without + requiring a checked-out PR branch. +3. **Current branch's open PR.** Otherwise run: + + ```bash + gh pr view --json number,url,headRepository,baseRepository \ + --jq '{number, url, repo: (.headRepository.owner.login + "/" + .headRepository.name)}' \ + 2>/dev/null + ``` + + If this returns a PR number, "Post comment to PR" is available. If it + fails (no PR open, not a GitHub repo, no `gh` auth), the option is + unavailable. + +### 4.3 Ask the user (interactive mode only) + +**Skip this section entirely in non-interactive mode** (per §4.1). In CI: +post the PR comment via §4.4 if a PR was detected, otherwise write +`SLOP_REVIEW.md` via §4.6. + +In interactive mode, use the `ask_user_question` tool. The exact options +depend on whether a PR was detected: + +**PR detected — present two options:** + +```text +question: "How would you like to surface these findings?" +header: "Output" +options: + 1. label: "Post comment to PR #" + description: "Post the rendered review as a single PR comment on PR + # via gh pr comment. Recommended for PR-scoped reviews." + 2. label: "Write SLOP_REVIEW.md" + description: "Write the full markdown report to SLOP_REVIEW.md at the + repo root. Does not commit or push." +``` + +`ask_user_question` automatically appends an **"Other"** choice — that is the +"type something else" escape hatch. Do not add it manually. Mark the PR +option as `(Recommended)` in its label when a PR is detected. + +**No PR detected — skip the question.** Write `SLOP_REVIEW.md` directly and +tell the user: "No open PR found for this branch — wrote findings to +`SLOP_REVIEW.md` (untracked)." If the user wants something else they can +ask in their next turn. Do not present a 1-option menu; `ask_user_question` +requires at least 2 options and a single-choice ask is friction without +information. + +If the user picks **"Other"**, parse their free-form text. Common requests +to handle: + +- Review branch + markdown — see §4.7 +- GitHub issues for each confirmed finding — see §4.7 +- Inline review comments at specific lines — see §4.7 +- Print to terminal only — just emit the markdown report and exit + +### 4.4 Posting to a PR + +When the user selects "Post comment to PR" (interactive) OR when running +in non-interactive mode with a PR detected (CI), render the report using +the **PR Comment Format** in §4.5. **This is structurally different from +the full markdown report** — the report is exhaustive; the PR comment is +glanceable with collapsibles for the deep tables. + +Steps: + +1. Render the comment to a temp file (e.g., `/tmp/slop-review-.md`). +2. **Interactive mode only:** show the user a brief preview hint (top 5 + lines + section list) and confirm — even though they already chose + this option, the comment contents weren't visible at the time of + choice. A confirmation here avoids posting a comment they wouldn't + have approved. **Skip the confirm in non-interactive mode** — the user + pre-authorized auto-posting by triggering the workflow. +3. Post: + + ```bash + gh pr comment --body-file --repo / + ``` + + `--repo` is required when the PR is in a different repository than the + current working directory; §4.2's detection returns the value to use. + In GitHub Actions the value is `$GITHUB_REPOSITORY`. + +4. Echo the comment URL returned by `gh pr comment` back to the user (or + to stdout in CI) so they can verify. + +5. **CI failure handling.** If `gh pr comment` fails in CI (auth, rate + limit, repo permissions, branch protection), exit non-zero with the + error so the workflow step fails loudly. Do not silently fall back to + writing `SLOP_REVIEW.md` — that hides the failure. + +**Do not** reference `SLOP_REVIEW.md` or other uncommitted files in the +posted comment — links to untracked paths 404 from the PR view. Attribution +should be a plain `` footer with no links. + +### 4.5 PR Comment Format + +The PR comment is rendered for fast skimming inside a PR conversation. +Use this exact structure: + +```markdown +## 🤖 AI Slop Review — `` + +**Verdict:** · **Grade:** · **Confidence:** + +| Local Code | Solution-Fit | Final | Scale | +|:----------:|:------------:|:-----:|:-----:| +| **** / 100 | **** / 100 | **** / 100 | 100 = clean, 0 = pervasive slop · *higher is better* | + +> [!NOTE] +> code-local vs. solution-fit shape of this PR.> + +--- + +### 🔥 Must-fix before merge + + + +- [ ] **** — ``. + +### 💡 Worth considering + + + +- [ ] **** — ``. + +--- + +### 🏗️ + +> [!WARNING] +> needed. Use [!WARNING] for items that change merge-readiness, [!IMPORTANT] +> for must-read context, [!CAUTION] for risk-of-regression. Use at most +> two alert blocks per comment.> + +--- + +### 🧪 + + + +--- + +
+📊 File-level scorecard (click to expand) + + + +
+ +
+🔍 All findings by lens (click to expand) + +#### AI Authorship Signals + + +#### Idiom Violations + + +#### Code Quality + + +#### Solution-Fit + + +
+ +
+✅ Positive signals + + + +
+ +--- + +### 📚 Education opportunity + + + +--- + +Generated by `/code-quality-slop` · 4-lens parallel scan + Opus calibration +``` + +**Rendering rules for the PR comment:** + +1. **Lead with the score table.** Most informative thing in a 5-line glance. +2. **Use GitHub alert syntax** (`> [!NOTE]`, `> [!WARNING]`, `> [!IMPORTANT]`, + `> [!CAUTION]`, `> [!TIP]`) sparingly — at most two per comment. Match + semantic weight to visual weight; a [!WARNING] should mean "this changes + whether I'd merge". +3. **Use task-list checkboxes** (`- [ ]`) for fixable items. They become + interactive in the PR UI so the author can check them off as they fix — + the comment doubles as a punch list. +4. **Push lens detail tables into `
` blocks.** A skimmable comment + beats an exhaustive one. Open by default only the score table and the + must-fix list. +5. **Trim file paths.** If every entry shares a common prefix, drop it. + Narrow viewports collapse long paths and lose the file name. +6. **Use the emoji vocabulary consistently.** 🔥 must-fix · 💡 nice-to-have · + 🏗️ architecture · 🧪 testing/idiom · 📊 scorecard · 🔍 lens detail · + ✅ positives · 📚 education. Don't reach for emoji elsewhere. +7. **No broken links.** Do not reference `SLOP_REVIEW.md` or any other + uncommitted file. The `` footer is enough attribution. +8. **Footer attribution** uses `` for de-emphasis. Keep it one line + with no links. + +### 4.6 Writing SLOP_REVIEW.md + +When the user selects "Write SLOP_REVIEW.md" (interactive) OR when running +non-interactively with no PR detected (CI), write the full markdown report +(per the **Output Format** section above) to `SLOP_REVIEW.md` at the repo +root. Do not commit, do not push. + +- **Interactive:** tell the user the file was written and that it is + currently untracked. +- **Non-interactive (CI):** also print a single-line summary to stdout — + `slop-review: · grade · /100 · wrote + SLOP_REVIEW.md` — so the workflow log captures the result. If the CI is + expected to upload `SLOP_REVIEW.md` as a workflow artifact, the path + should remain at the repo root unless the workflow specifies otherwise. + +If `SLOP_REVIEW.md` already exists: + +- **Interactive:** ask whether to overwrite, append, or write to a + date-stamped filename (e.g., `SLOP_REVIEW..md`). +- **Non-interactive:** overwrite without prompting. CI runs are expected + to be reproducible; appending across runs would corrupt artifacts. + +### 4.7 Other delivery shapes (when the user picks "Other") + +These are fallbacks — only use when the user explicitly asks via the +"Other" free-form input. + +**Review branch with markdown report.** Best for full-codebase audits and +archival. Create a new branch `/slop-review`, write to +`CLAUDE_SLOP_REVIEW.md` at the repo root, commit, and push. Tell the user +the branch is ready and they can open a PR for team discussion. + +**GitHub issues.** Best for tech-debt tracking. For each confirmed finding +(or group of related findings), create a GitHub issue with: descriptive +title, SHA-pinned permalink(s) to the offending code, signal category and +severity, suggested fix, and appropriate labels (`ai-slop`, severity +labels). Group related findings into single issues where it makes sense +("4 instances of bare except Exception: pass" is one issue, not four). +Ask whether to create a milestone (e.g., "AI Slop Cleanup") before opening +issues. + +**Inline PR review comments.** Best when findings map to specific changed +lines and the team prefers per-line review. For each confirmed finding, +post an inline review comment at the exact file and line using +`gh api repos/{owner}/{repo}/pulls/{pr}/reviews`: + +```bash +gh api repos/{owner}/{repo}/pulls/{pr}/reviews -f event=COMMENT \ + -f body="AI Slop Review: found N issues" \ + -f 'comments[][path]=...' -f 'comments[][line]=...' \ + -f 'comments[][body]=...' +``` + +Group related findings into a single review submission. Format each +inline comment as: + +```text +**[Signal: ]** + + +``` + +Keep inline comments concise — a reviewer, not an essay writer. + +**Combined.** The user may want both an archival markdown AND actionable +items. If so, do the markdown delivery first, then the actionable +delivery. Update issue/comment bodies to reference the markdown only if +that file has been committed and pushed (otherwise the link 404s). diff --git a/packages/pi-code-quality/skills/slop-review/references/go.md b/packages/pi-code-quality/skills/slop-review/references/go.md new file mode 100644 index 0000000..71babf5 --- /dev/null +++ b/packages/pi-code-quality/skills/slop-review/references/go.md @@ -0,0 +1,401 @@ +# Go AI Slop Signals + +Language-specific signals for Go codebases. These supplement the universal signals +in SKILL.md — apply both. + +## What idiomatic Go looks like + +### At version 1.26+ +- `log/slog` for structured logging (stdlib since 1.21) +- Range-over-func iterators (since 1.23) +- Generic `slices`/`maps` functions over hand-rolled loops +- `cmp.Or` for default values +- Structured concurrency patterns with `errgroup` +- `maps.Clone`, `slices.Concat`, `slices.Contains` etc. +- New `http.ServeMux` with method-based routing (1.22+) + +### Stdlib preferences +- Logging: `log/slog` over `log` or third-party loggers in new code +- Slices: `slices` package over manual loops for search/sort/compare +- Maps: `maps` package for clone/keys/values operations +- HTTP: `http.NewServeMux` patterns with method routing (1.22+) +- Testing: `testing.TB` helpers, `t.Cleanup` over deferred cleanup + +### Error handling convention +- Always wrap errors with `fmt.Errorf("context: %w", err)` +- Use `errors.Is`/`errors.As` for comparison, never string matching +- Return errors to caller, don't `log.Fatal` in library/handler code +- Sentinel errors for expected conditions, wrapped errors for unexpected + +### Project adaptation +Before flagging any idiom violation, check if the project's idiom baseline uses a different convention. The baseline overrides these defaults. + +--- + +## Priority order for Go + +1. Error handling — this is where AI Go code fails most visibly +2. Context propagation — threading context.Context correctly +3. Concurrency — goroutine lifecycle, channel ownership, sync primitives +4. Interface and type design — idiomatic Go vs. Java-in-Go +5. Testing — table-driven tests, test helpers, subtests + +--- + +## Error handling (highest priority) + +This is the single biggest tell in AI-generated Go. Humans who write Go daily internalize +error handling patterns; AI frequently gets them subtly wrong. + +**Fatal/print instead of return:** +```go +// SLOP — log.Fatal in a library or handler function +if err != nil { + log.Fatal(err) +} + +// SLOP — fmt.Println for errors +if err != nil { + fmt.Println("error:", err) +} + +// IDIOMATIC — return the error to the caller +if err != nil { + return fmt.Errorf("fetching user %d: %w", id, err) +} +``` + +**Missing error wrapping:** +```go +// SLOP — no context, breaks error unwrapping +if err != nil { + return err +} + +// SLOP — wraps but without %w, breaks errors.Is/As +if err != nil { + return fmt.Errorf("failed to connect: %v", err) +} + +// IDIOMATIC +if err != nil { + return fmt.Errorf("connecting to %s: %w", addr, err) +} +``` + +**String comparison on errors:** +```go +// SLOP +if err.Error() == "not found" { + // ... +} + +// IDIOMATIC +if errors.Is(err, ErrNotFound) { + // ... +} +``` + +**Swallowed errors:** +```go +// SLOP — error silently discarded +result, _ := doSomething() +``` + +--- + +## Context propagation + +**Missing context.Context in signatures:** +```go +// SLOP — no context parameter +func FetchUser(id int) (*User, error) { + +// IDIOMATIC — context is first parameter +func FetchUser(ctx context.Context, id int) (*User, error) { +``` + +**context.Background() mid-callstack:** +```go +// SLOP — creates a new root context deep in the call chain +func (s *Service) Process(data []byte) error { + ctx := context.Background() // should come from caller + return s.store.Save(ctx, data) +} +``` + +**No cancellation handling:** +```go +// SLOP — ignores context cancellation +func worker(ctx context.Context) { + for { + doWork() + time.Sleep(time.Second) + } +} + +// IDIOMATIC +func worker(ctx context.Context) { + for { + select { + case <-ctx.Done(): + return + default: + doWork() + } + select { + case <-ctx.Done(): + return + case <-time.After(time.Second): + } + } +} +``` + +--- + +## Concurrency + +**Goroutine leaks — no cancellation path:** +```go +// SLOP — goroutine runs forever, no way to stop it +go func() { + for { + process(queue) + } +}() + +// IDIOMATIC — cancellable via context +go func() { + for { + select { + case <-ctx.Done(): + return + case item := <-queue: + process(item) + } + } +}() +``` + +**Channel ownership confusion:** +- Channel created by one goroutine, closed by another (race condition risk) +- Channel created but never closed (goroutines blocked on range forever) +- Unbuffered channel used where buffered is needed (deadlock risk) + +**sync.Mutex overuse:** +```go +// SLOP — mutex protecting a counter that should use atomic +var mu sync.Mutex +var count int + +// BETTER +var count atomic.Int64 +``` + +**sync.Mutex where single-goroutine ownership works:** +If a struct is only accessed from one goroutine, it doesn't need a mutex. +AI often adds mutexes "just in case" — a strong slop signal. + +--- + +## Type and interface misuse + +**Interface defined in the wrong package:** +```go +// SLOP — interface defined next to its implementation +// (Go convention: interfaces belong in the consuming package) +type UserStore interface { + GetUser(ctx context.Context, id int) (*User, error) +} + +type userStore struct { ... } +func (s *userStore) GetUser(...) { ... } +``` + +**Premature interface:** +```go +// SLOP — interface with exactly one implementation and one consumer +type Processor interface { + Process(data []byte) error +} +``` + +**`interface{}` instead of `any`, or `any` instead of generics:** +```go +// SLOP — interface{} is an alias for any since 1.18; using the old syntax is dated +func Contains(slice []interface{}, item interface{}) bool { + +// STILL SLOP at 1.26+ — hand-rolled contains; use slices.Contains +func Contains[T comparable](slice []T, item T) bool { + +// IDIOMATIC at 1.26+ — use the stdlib slices package +import "slices" +found := slices.Contains(slice, item) +``` + +**Value receiver on mutating method:** +```go +// SLOP — s is a copy, mutation is lost +func (s Service) SetTimeout(d time.Duration) { + s.timeout = d +} + +// CORRECT +func (s *Service) SetTimeout(d time.Duration) { + s.timeout = d +} +``` + +--- + +## Other Go idioms + +**defer inside a loop:** +```go +// SLOP — defers accumulate until function return, not loop iteration +for _, f := range files { + fd, _ := os.Open(f) + defer fd.Close() // all closes happen at function exit +} + +// CORRECT — extract to helper or close explicitly +for _, f := range files { + if err := processFile(f); err != nil { + return err + } +} +``` + +**Hand-rolled slice/map operations (1.26+):** +```go +// SLOP — manual contains check +found := false +for _, v := range items { + if v == target { + found = true + break + } +} + +// IDIOMATIC at 1.26+ — use slices package +found := slices.Contains(items, target) + +// SLOP — manual sort +sort.Slice(items, func(i, j int) bool { return items[i] < items[j] }) + +// IDIOMATIC at 1.26+ +slices.Sort(items) + +// SLOP — manual map key collection +keys := make([]string, 0, len(m)) +for k := range m { + keys = append(keys, k) +} + +// IDIOMATIC at 1.26+ +keys := slices.Collect(maps.Keys(m)) +``` + +**Old-style logging instead of slog (1.26+):** +```go +// SLOP in new code — unstructured logging +log.Printf("user %d logged in from %s", id, ip) + +// IDIOMATIC at 1.26+ — structured logging with log/slog +slog.Info("user logged in", "user_id", id, "ip", ip) +``` + +**Default value chains without cmp.Or (1.26+):** +```go +// SLOP — verbose conditional default +port := os.Getenv("PORT") +if port == "" { + port = "8080" +} + +// IDIOMATIC at 1.26+ +port := cmp.Or(os.Getenv("PORT"), "8080") +``` + +**HTTP server without timeouts:** +```go +// SLOP +http.ListenAndServe(":8080", handler) + +// IDIOMATIC at 1.26+ — use method-based routing on ServeMux +mux := http.NewServeMux() +mux.HandleFunc("GET /users/{id}", getUser) +mux.HandleFunc("POST /users", createUser) + +srv := &http.Server{ + Addr: ":8080", + Handler: mux, + ReadTimeout: 15 * time.Second, + WriteTimeout: 15 * time.Second, + IdleTimeout: 60 * time.Second, +} +srv.ListenAndServe() +``` + +**Global singletons via init():** +```go +// SLOP +var db *sql.DB + +func init() { + var err error + db, err = sql.Open("postgres", os.Getenv("DATABASE_URL")) + if err != nil { + log.Fatal(err) + } +} + +// BETTER — dependency injection, initialized in main() +``` + +--- + +## Security signals + +- SQL via `fmt.Sprintf` instead of parameterized queries (`db.Query(query, args...)`) +- `http.ListenAndServe` with no TLS and no reverse proxy documented +- `os/exec.Command` with user input concatenated into the command string +- `http.Get` / `http.Post` with no timeout (uses `http.DefaultClient` which has no timeout) + +--- + +## Go-specific test signals + +**Not using table-driven tests** — the canonical Go testing pattern: +```go +// SLOP — separate test functions for each case +func TestParseValid(t *testing.T) { ... } +func TestParseEmpty(t *testing.T) { ... } +func TestParseInvalid(t *testing.T) { ... } + +// IDIOMATIC +func TestParse(t *testing.T) { + tests := []struct { + name string + input string + want Result + wantErr bool + }{ + {"valid", "good", Result{...}, false}, + {"empty", "", Result{}, true}, + {"invalid", "bad", Result{}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Parse(tt.input) + // ... + }) + } +} +``` + +- `t.Log` / `fmt.Println` instead of assertion — test always passes +- No `t.Helper()` on test helper functions (error locations point to helper, not caller) +- No `t.Parallel()` on tests that are safe to parallelize +- `testify` used in a codebase that uses stdlib `testing` everywhere else (or vice versa) diff --git a/packages/pi-code-quality/skills/slop-review/references/python.md b/packages/pi-code-quality/skills/slop-review/references/python.md new file mode 100644 index 0000000..d2c1292 --- /dev/null +++ b/packages/pi-code-quality/skills/slop-review/references/python.md @@ -0,0 +1,263 @@ +# Python AI Slop Signals + +Language-specific signals for Python codebases. These supplement the universal signals +in SKILL.md — apply both. + +## What idiomatic Python looks like + +### At version 3.13+ + +- Union types: `X | None` not `Optional[X]`, `X | Y` not `Union[X, Y]` +- Built-in generics: `list[str]` not `typing.List[str]`, `dict[str, int]` not `typing.Dict[str, int]` +- `@deprecated` decorator from `warnings` +- `StrEnum` for string enumerations (stdlib since 3.11) +- `tomllib` for TOML parsing (stdlib since 3.11) +- `ExceptionGroup` and `except*` for concurrent error handling +- Walrus operator `:=` where it reduces duplication +- Per-interpreter GIL / free-threaded mode awareness + +### Stdlib preferences + +- Filesystem: `pathlib.Path` over `os.path` in new code +- Caching: `functools.cache` (3.9+) or `lru_cache` over hand-rolled memoization +- Data containers: `dataclasses` or `NamedTuple` over plain dicts for structured data +- Iteration: `itertools` (product, chain, groupby) over nested manual loops +- Context managers: `contextlib.contextmanager` over manual `__enter__`/`__exit__` +- Temporary files: `tempfile` with context managers, never hardcoded paths + +### Error handling convention + +- Specific exceptions over bare `except Exception` +- `raise X from e` to preserve tracebacks +- EAFP for duck typing, LBYL for everything else +- `.get()` over try/except KeyError for dict access + +### Project adaptation + +Before flagging any idiom violation, check if the project's idiom baseline (from Step 0) +uses a different convention. The baseline overrides these defaults. + +--- + +## Priority order for Python + +1. Error handling — bare excepts, swallowed errors, exceptions for control flow +2. Idiomatic Python — are they writing Python or Java-in-Python? +3. Type system usage — modern typing vs. no types vs. over-typed +4. Async correctness — if async is used, is it used properly? +5. Security — eval, exec, shell injection, pickle + +--- + +## Error handling + +**Bare exception swallowing** — the single most common AI Python tell: +```python +# SLOP: silent failure +try: + result = do_thing() +except Exception: + pass + +# SLOP: catching too broadly then re-raising generically +try: + data = parse(raw) +except Exception as e: + raise ValueError(f"Parse failed: {e}") # loses traceback, catches too much + +# BETTER: specific exceptions, context preserved +try: + data = parse(raw) +except json.JSONDecodeError as e: + raise ParseError(f"Invalid JSON at position {e.pos}") from e +``` + +**Exceptions for control flow** — using try/except where a conditional or `.get()` works: +```python +# SLOP +try: + value = data["key"] +except KeyError: + value = default + +# IDIOMATIC +value = data.get("key", default) +``` + +**Over-defensive error handling** — wrapping operations that cannot fail: +```python +# SLOP: len() on a list cannot raise +try: + count = len(items) +except Exception: + count = 0 +``` + +--- + +## Classic footguns AI still produces + +**Mutable default arguments:** +```python +# SLOP — the list is shared across all calls +def append_to(item, target=[]): + target.append(item) + return target + +# CORRECT +def append_to(item, target=None): + if target is None: + target = [] + target.append(item) + return target +``` + +**Global state as a crutch:** +```python +# SLOP +_cache = {} +def get_user(user_id): + global _cache + if user_id not in _cache: + _cache[user_id] = fetch(user_id) + return _cache[user_id] + +# BETTER: encapsulate state, or use functools.lru_cache +``` + +**Resources without context managers:** +```python +# SLOP +f = open("data.csv") +data = f.read() +f.close() + +# IDIOMATIC +with open("data.csv") as f: + data = f.read() +``` + +--- + +## Type and style signals + +**Missing or outdated type hints** — with 3.13+ as the minimum floor, these are all +non-idiomatic and should be flagged: +- No type hints at all on function signatures +- `Optional[X]` instead of `X | None` — the `Optional` alias is legacy +- `typing.List`, `typing.Dict`, `typing.Tuple`, `typing.Set` instead of `list`, `dict`, `tuple`, `set` — built-in generics have been available since 3.9 +- `typing.Union[X, Y]` instead of `X | Y` — the pipe syntax has been available since 3.10 +- Importing from `typing` for constructs that now live in the stdlib (e.g., `typing.NamedTuple` when `typing` import is the only reason) + +**isinstance chains instead of structural typing:** +```python +# SLOP +def process(item): + if isinstance(item, str): + return handle_str(item) + elif isinstance(item, int): + return handle_int(item) + elif isinstance(item, list): + return handle_list(item) + +# BETTER: Protocol, singledispatch, or rethink the interface +``` + +**Avoiding the stdlib:** +- `os.path.join` instead of `pathlib.Path` in new code +- Manual iteration instead of `itertools` (product, chain, groupby) +- Hand-rolled memoization instead of `functools.lru_cache` or `cache` +- Manual context managers instead of `contextlib.contextmanager` +- Custom data containers instead of `dataclasses` or `NamedTuple` +- `collections.OrderedDict` in Python 3.7+ (regular dicts are ordered) + +**`*args, **kwargs` on internal functions:** +```python +# SLOP — lazy interface design, impossible to type-check +def create_user(*args, **kwargs): + return User(*args, **kwargs) + +# BETTER: explicit parameters with types +def create_user(name: str, email: str, role: Role = Role.USER) -> User: + return User(name=name, email=email, role=role) +``` + +--- + +## Async signals + +**Blocking calls in async code** — the most insidious AI async mistake: +```python +# SLOP — requests blocks the event loop +async def fetch_data(url): + response = requests.get(url) # BLOCKS + return response.json() + +# SLOP — time.sleep blocks the event loop +async def poll(): + while True: + await check() + time.sleep(5) # BLOCKS — should be await asyncio.sleep(5) +``` + +**Deprecated async patterns:** +```python +# SLOP (deprecated since 3.10) +loop = asyncio.get_event_loop() +loop.run_until_complete(main()) + +# IDIOMATIC +asyncio.run(main()) +``` + +**Missing await** — code runs but the coroutine never executes: +```python +# SLOP — result is a coroutine object, not the actual result +async def process(): + result = fetch_data() # missing await + return result +``` + +--- + +## Security signals + +These are serious — flag as Critical regardless of other context: + +- `eval()` or `exec()` on any external or user-influenced input +- Shell commands with `shell=True` and string formatting for the command +- `pickle.loads()` / `pickle.load()` on untrusted data +- Secrets in default argument values, module-level constants, or committed `.env` files +- `yaml.load()` without `Loader=SafeLoader` (allows arbitrary code execution) +- SQL built via f-strings or `.format()` instead of parameterized queries +- `hashlib.md5()` or `hashlib.sha1()` for security purposes (use sha256+) +- `random` module for security-sensitive values (use `secrets` module) + +--- + +## Python-specific test signals + +- `unittest.TestCase` subclasses in a project that uses `pytest` everywhere else +- `mock.patch` on everything — tests that mock the entire world test nothing +- No `parametrize` / `params` where the function has obvious input variations +- `assert result is not None` as the only assertion (proves nothing about correctness) +- Test files that import and call the function but don't assert meaningful behavior +- `conftest.py` with fixtures that do too much setup (hiding test complexity) +- No `tmp_path` or `tmp_path_factory` — tests writing to fixed filesystem paths + +--- + +## Framework-specific notes + +Be aware that some frameworks have conventions that look unusual: + +- **Dagster:** `@asset` decorators, `@op`, resource injection via type annotations, + `MaterializeResult` returns. These are framework conventions, not slop. +- **Django:** Class-based views, `Meta` inner classes, `models.Manager` subclasses. +- **FastAPI:** `Depends()` injection, Pydantic models for validation, `async def` route + handlers that may legitimately use sync ORM calls via `run_in_executor`. +- **SQLAlchemy:** `Column()`, `relationship()`, declarative base patterns. +- **Pydantic:** `model_validator`, `field_validator`, `ConfigDict` — these are idiomatic. + +Don't flag framework-conventional patterns. Do flag when framework patterns are mixed +incorrectly (e.g., raw SQL in a project that uses SQLAlchemy ORM everywhere else). diff --git a/packages/pi-code-quality/skills/slop-review/references/rust.md b/packages/pi-code-quality/skills/slop-review/references/rust.md new file mode 100644 index 0000000..fab4159 --- /dev/null +++ b/packages/pi-code-quality/skills/slop-review/references/rust.md @@ -0,0 +1,296 @@ +# Rust AI Slop Signals + +Language-specific signals for Rust codebases. These supplement the universal signals +in SKILL.md — apply both. + +## What idiomatic Rust looks like + +### At version 1.94+ (Edition 2024) +- `async fn` in traits — no more `async-trait` crate for simple cases +- `impl Trait` in more return positions +- `LazyLock`/`LazyCell` from stdlib over `once_cell`/`lazy_static` crates +- Let chains for cleaner pattern matching +- `#[must_use]` on fallible functions and important return values +- `#[diagnostic::on_unimplemented]` for better error messages on traits + +### Stdlib preferences +- Lazy init: `std::sync::LazyLock` over `once_cell::sync::Lazy` +- Error types: `thiserror` for library errors, `anyhow`/`eyre` for application errors +- Async runtime: `tokio` (check project convention) +- Serialization: `serde` with derive macros +- CLI: `clap` with derive macros + +### Error handling convention +- `?` propagation, never manual `match Ok/Err` for simple forwarding +- Typed error enums with `thiserror` in library code +- `anyhow::Result` acceptable in binary/CLI code, not libraries +- `expect()` only in `main()` or provably unreachable paths +- `unwrap()` only in tests + +### Type system usage +- Enums over bools for state/mode parameters +- Newtype pattern for domain values (UserId, OrderId) +- `From`/`Into` implementations for error conversion +- `Default` derive when obvious default exists +- Private fields with public constructors for invariant enforcement + +### Project adaptation +Before flagging any idiom violation, check if the project's baseline uses different conventions. + +--- + +## Priority order for Rust + +1. Error handling — unwrap/expect abuse, missing ? propagation +2. Ownership and borrowing — clone storms, RefCell overuse +3. Type system usage — enums vs. bools, newtypes, trait implementations +4. Unsafe and panic safety — library code that panics or uses unsafe carelessly +5. Clippy and tooling — suppressed warnings, unnecessary pub visibility + +--- + +## Error handling (highest priority) + +**unwrap() outside of tests:** +```rust +// SLOP — panics in production on any failure +let config = read_config().unwrap(); +let conn = db.connect().unwrap(); + +// IDIOMATIC — propagate with ? +let config = read_config()?; +let conn = db.connect().context("connecting to database")?; +``` + +**expect() as a substitute for error propagation:** +```rust +// SLOP — slightly better than unwrap but still panics +let user = find_user(id).expect("user should exist"); + +// expect() is OK for genuinely unrecoverable cases in main() or bootstrap +// but not in library code or request handlers +``` + +**Box in library code:** +```rust +// SLOP — erases the error type, consumers can't match on it +fn process(data: &[u8]) -> Result> { + +// IDIOMATIC — typed error enum (thiserror) +#[derive(Debug, thiserror::Error)] +enum ProcessError { + #[error("invalid format: {0}")] + InvalidFormat(String), + #[error("io error")] + Io(#[from] std::io::Error), +} + +fn process(data: &[u8]) -> Result { +``` + +**Not using ? propagation:** +```rust +// SLOP — manual match on every Result +let data = match read_file(path) { + Ok(d) => d, + Err(e) => return Err(e.into()), +}; + +// IDIOMATIC +let data = read_file(path)?; +``` + +--- + +## Borrow checker fights + +**Excessive .clone() — the #1 Rust slop signal:** +```rust +// SLOP — cloning to avoid borrow checker errors +fn process(items: &[Item]) -> Vec { + let cloned = items.to_vec(); // unnecessary clone + cloned.iter().map(|item| { + let name = item.name.clone(); // another unnecessary clone + transform(name) + }).collect() +} +``` + +When you see `.clone()` more than once or twice in a function, check whether +ownership could be restructured instead. Some clones are necessary and correct — +the signal is *pervasive* cloning as a pattern. + +**Rc> when ownership could be restructured:** +```rust +// SLOP — interior mutability as a crutch +let shared_state = Rc::new(RefCell::new(State::new())); + +// Often the real fix is restructuring so one owner holds the state +// and others get references or communicate via channels +``` + +Note: with Edition 2024, `async fn` in traits eliminates some cases where +`Rc>` was used as a workaround for trait object limitations in +async code. If you see this pattern in async trait implementations, check +whether native `async fn` in traits makes it unnecessary. + +**Arc> overuse:** +Same pattern as Rc> but in async/threaded code. If every piece of +shared state is behind Arc>, the author likely fought the borrow checker +rather than designing for ownership. + +--- + +## Type system underuse + +**Bool flags instead of enums:** +```rust +// SLOP — what does (true, false) mean? +fn create_user(name: &str, is_admin: bool, is_active: bool) -> User { + +// IDIOMATIC — illegal states unrepresentable +enum Role { Admin, User } +enum Status { Active, Inactive } +fn create_user(name: &str, role: Role, status: Status) -> User { +``` + +**Stringly-typed state:** +```rust +// SLOP +fn set_status(status: &str) { + match status { + "active" | "inactive" | "pending" => { ... } + _ => panic!("invalid status"), // runtime error for a compile-time problem + } +} + +// IDIOMATIC +enum Status { Active, Inactive, Pending } +fn set_status(status: Status) { ... } +``` + +**Missing standard trait implementations:** +- `Debug` derived but `Display` not implemented (users see `MyError { kind: ... }` instead of a message) +- No `From`/`Into` implementations for error types (forces manual conversion everywhere) +- No `Default` when there's an obvious default configuration +- Missing `Clone`, `PartialEq`, `Hash` on types that clearly need them + +**Raw primitives for domain values:** +```rust +// SLOP — easy to pass user_id where order_id is expected +fn get_order(user_id: u64, order_id: u64) -> Order { + +// IDIOMATIC — newtype pattern +struct UserId(u64); +struct OrderId(u64); +fn get_order(user_id: UserId, order_id: OrderId) -> Order { +``` + +--- + +## Unsafe and panic safety + +**Unsafe without SAFETY comment:** +```rust +// SLOP +unsafe { + ptr::write(dest, value); +} + +// REQUIRED +// SAFETY: dest is guaranteed to be valid and aligned because +// it was allocated by Vec::with_capacity and index < len. +unsafe { + ptr::write(dest, value); +} +``` + +**Panicking in library code:** +- `panic!()`, `unreachable!()`, `todo!()` in non-test code +- `.unwrap()` on user-influenced data paths +- `assert!()` for input validation (use `Result` instead) +- Array indexing without bounds checks where the index comes from external input + +**transmute without justification:** +`std::mem::transmute` should be extremely rare and always documented. If AI generated +a transmute, it almost certainly found a Stack Overflow answer from 2016 and +copy-pasted it. + +--- + +## Build and tooling signals + +**`lazy_static!` or `once_cell` for lazy initialization (Edition 2024+):** +```rust +// SLOP — unnecessary third-party crate since Rust 1.80+ +use lazy_static::lazy_static; +lazy_static! { + static ref CONFIG: Config = load_config(); +} + +// SLOP — same issue with once_cell +use once_cell::sync::Lazy; +static CONFIG: Lazy = Lazy::new(|| load_config()); + +// IDIOMATIC — stdlib LazyLock (stable since 1.80, preferred in Edition 2024) +use std::sync::LazyLock; +static CONFIG: LazyLock = LazyLock::new(|| load_config()); +``` + +If you see `lazy_static` or `once_cell::sync::Lazy` in new code targeting +Rust 1.80+, flag it. For `once_cell::sync::OnceCell`, the stdlib equivalent +is `std::sync::OnceLock`. Existing projects may still use the crates for MSRV +reasons — check `Cargo.toml` for `rust-version` before flagging. + +**`async-trait` crate in Edition 2024 code:** +```rust +// SLOP — unnecessary with native async fn in traits (Edition 2024) +#[async_trait] +trait MyService { + async fn handle(&self) -> Result<()>; +} + +// IDIOMATIC — native async fn in traits +trait MyService { + async fn handle(&self) -> Result<()>; +} +``` + +The `async-trait` crate is still needed for `dyn Trait` usage or when trait +objects require `Send` bounds that native async traits don't yet handle well. +Flag it only when the trait is used with static dispatch (generics/`impl Trait`). + +**Suppressed warnings:** +```rust +// SLOP — hiding problems instead of fixing them +#[allow(unused_variables)] +#[allow(dead_code)] +#[allow(unused_imports)] +``` + +A few `#[allow]` annotations are normal. Dozens scattered across a file means the +code was generated and then warnings were suppressed to make it compile. + +**Over-broad pub visibility:** +```rust +// SLOP — everything public for no reason +pub struct Config { + pub db_url: String, + pub db_pool_size: u32, + pub secret_key: String, // this should NOT be pub +} +``` + +In idiomatic Rust, items are private by default and only made `pub` when needed +by the module's public API. If every struct, field, function, and module is `pub`, +it's likely AI-generated with no thought about encapsulation. + +--- + +## Rust-specific test signals + +- No `#[should_panic]` or explicit error matching on tests for error paths +- `assert!(result.is_ok())` instead of `let value = result.unwrap()` + assertions on value +- Tests that don't use `#[test]` helper patterns (common setup extracted to functions) +- Integration tests in `src/` instead of `tests/` directory +- No `proptest` or `quickcheck` where the function has clear algebraic properties diff --git a/packages/pi-code-quality/skills/slop-review/references/svelte-ts.md b/packages/pi-code-quality/skills/slop-review/references/svelte-ts.md new file mode 100644 index 0000000..16eeb85 --- /dev/null +++ b/packages/pi-code-quality/skills/slop-review/references/svelte-ts.md @@ -0,0 +1,384 @@ +# Svelte / TypeScript AI Slop Signals + +Language-specific signals for Svelte and TypeScript codebases. These supplement the +universal signals in SKILL.md — apply both. + +## What idiomatic Svelte/TypeScript looks like + +### Svelte 24.x LTS (Svelte 5) +- Runes: `$state`, `$derived`, `$effect` — no more `$:` reactive declarations +- `$props()` with typed interface — no more `export let` +- `$bindable()` for two-way binding props +- Snippets over slots for content composition +- `$inspect()` for debugging (dev-only) +- Fine-grained reactivity — no more immutable assignment patterns needed for arrays/objects + +### TypeScript 5.9+ +- `satisfies` operator for type narrowing without widening +- `using` declarations for resource management (Explicit Resource Management) +- `const` type parameters for literal inference +- `NoInfer` utility type +- Discriminated unions over optional fields +- `readonly` on config/prop interfaces +- Template literal types for string validation + +### SvelteKit patterns +- `+page.server.ts` load functions over `onMount` fetching +- Form actions over manual fetch POST +- `$page.data` for layout-shared data +- SvelteKit's `fetch` (not axios/node-fetch) — handles cookies, SSR, relative URLs +- `$env/static/private` for secrets (never in `+page.ts`) + +### Project adaptation +Before flagging any idiom violation, check the project's baseline conventions. + +## Priority order for Svelte/TypeScript + +1. TypeScript type safety — any abuse, type assertions, missing discriminated unions +2. Svelte reactivity model — correct reactive updates, cleanup, binding usage +3. SvelteKit patterns — load functions, form actions, data flow +4. Component design — prop drilling, store usage, composition +5. Security — XSS via {@html}, client-side secrets + +--- + +## TypeScript misuse + +**`any` as an escape hatch:** +```typescript +// SLOP — gives up type safety entirely +const response: any = await fetch('/api/users'); +const data: any = await response.json(); + +// IDIOMATIC — define the shape +interface User { + id: string; + name: string; + email: string; +} +const response = await fetch('/api/users'); +const data: User[] = await response.json(); + +// EVEN BETTER — runtime validation +const data = UserArraySchema.parse(await response.json()); +``` + +**@ts-ignore without explanation:** +```typescript +// SLOP +// @ts-ignore +const result = thing.method(); + +// IF NECESSARY — explain why +// @ts-expect-error: library types are wrong, see https://github.com/lib/issues/123 +const result = thing.method(); +``` + +**Type assertions without runtime validation:** +```typescript +// SLOP — trusts external data blindly +const user = JSON.parse(body) as User; + +// IDIOMATIC — validate at the boundary +const user = userSchema.parse(JSON.parse(body)); +``` + +**Optionals everywhere instead of discriminated unions:** +```typescript +// SLOP — which combinations are valid? Who knows +interface ApiResponse { + data?: User[]; + error?: string; + loading?: boolean; + total?: number; +} + +// IDIOMATIC — each state is explicit +type ApiResponse = + | { status: 'loading' } + | { status: 'error'; error: string } + | { status: 'success'; data: User[]; total: number }; +``` + +**Missing readonly:** +```typescript +// SLOP — props and config objects should be immutable +interface Config { + apiUrl: string; + timeout: number; +} + +// IDIOMATIC +interface Config { + readonly apiUrl: string; + readonly timeout: number; +} +``` + +--- + +## Svelte reactivity model violations + +**Using Svelte 4 reactive declarations instead of runes:** +```svelte + +``` + +**Imperative mutation instead of reactive state:** +```svelte + +``` + +**Missing $effect cleanup:** +```svelte + +``` + +**Direct DOM manipulation bypassing Svelte:** +```svelte + +
+``` + +**Missing UI states — the classic AI tell:** +```svelte + +{#if data} + {#each data as item} + + {/each} +{/if} + + +{#if loading} + +{:else if error} + +{:else if data.length === 0} + +{:else} + {#each data as item} + + {/each} +{/if} +``` + +--- + +## SvelteKit patterns + +**Data fetching in onMount instead of load function:** +```svelte + +``` + +```typescript +// IDIOMATIC — in +page.ts or +page.server.ts +export const load = async ({ fetch }) => { + const res = await fetch('/api/users'); + return { users: await res.json() }; +}; +``` + +**Not using SvelteKit's fetch:** +```typescript +// SLOP — loses cookie forwarding, SSR compatibility +import axios from 'axios'; +const data = await axios.get('/api/data'); + +// IDIOMATIC — SvelteKit's fetch handles cookies, SSR, and relative URLs +export const load = async ({ fetch }) => { + const res = await fetch('/api/data'); + return { data: await res.json() }; +}; +``` + +**Manual form handling instead of form actions:** +```svelte + +
{ e.preventDefault(); handleSubmit(); }}> + + +
+ + +``` + +```typescript +// IDIOMATIC — in +page.server.ts +export const actions = { + default: async ({ request }) => { + const data = await request.formData(); + const name = data.get('name'); + // validate and save + } +}; +``` + +**Ignoring layout data and page stores:** +- Not using `$page.data` for shared data from layout load functions +- Not using `$navigating` for navigation state +- Duplicating data fetching that a parent layout already provides + +--- + +## Component design + +**Prop drilling (4+ levels):** +```svelte + + + + + + + + + + + + +``` + +**One-off component duplication:** +If a component exists that does 90% of what's needed, and AI created a new component +with 90% identical code plus a small variation — that's slop. The existing component +should be extended with a prop or snippet. + +**Untyped props and events:** +```svelte + +``` + +**Using `export let` (Svelte 4 legacy):** +```svelte + +``` + +--- + +## Security signals + +**XSS via {@html}:** +```svelte + +{@html userComment} + + +{userComment} +``` + +**Client-side secrets:** +```typescript +// SLOP — exposed to the browser +const API_KEY = 'sk-1234567890'; + +// IDIOMATIC — server-side only, in +page.server.ts or via $env/static/private +import { API_KEY } from '$env/static/private'; +``` + +- Any `import` from `$env/static/private` in a `+page.ts` (non-server) file +- API keys in `.env` without the `PUBLIC_` prefix that are used client-side +- Sensitive data in `+page.ts` load functions instead of `+page.server.ts` + +--- + +## TypeScript-specific test signals + +- Tests that use `as any` to bypass type errors instead of providing proper test data +- No `vitest` or `playwright` in a SvelteKit project (the default test runners) +- Component tests that only check rendering, not interaction or state changes +- Missing `@testing-library/svelte` patterns — testing implementation details instead of behavior +- No type-level tests for discriminated unions and conditional types +- E2E tests that use `data-testid` on everything instead of accessible selectors diff --git a/packages/pi-code-quality/tests/package.test.mjs b/packages/pi-code-quality/tests/package.test.mjs new file mode 100644 index 0000000..27e4042 --- /dev/null +++ b/packages/pi-code-quality/tests/package.test.mjs @@ -0,0 +1,124 @@ +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import test from "node:test"; + +const readText = (path) => readFileSync(new URL(`../${path}`, import.meta.url), "utf8"); +const readJson = (path) => JSON.parse(readText(path)); + +test("package exposes code-quality skills and prompts to Pi", () => { + const pkg = readJson("package.json"); + + assert.equal(pkg.name, "@sentiolabs/pi-code-quality"); + assert.equal(pkg.version, "0.1.0"); + assert.equal(pkg.license, "MIT"); + assert.equal(pkg.repository.directory, "packages/pi-code-quality"); + assert.deepEqual(pkg.pi.skills, ["./skills"]); + assert.deepEqual(pkg.pi.prompts, ["./prompts/*.md"]); + assert.ok(pkg.files.includes("skills/")); + assert.ok(pkg.files.includes("prompts/")); + assert.ok(pkg.keywords.includes("pi-package")); + assert.ok(pkg.keywords.includes("ai-slop")); + assert.ok(pkg.keywords.includes("pr-size")); + assert.ok(pkg.keywords.includes("reviewability")); +}); + +test("slop-review skill frontmatter is valid for Pi discovery", () => { + const skill = readText("skills/slop-review/SKILL.md"); + + assert.match(skill, /^---\n/); + assert.match(skill, /\nname: slop-review\n/); + const description = skill.match(/\ndescription: (.+)\n/)?.[1] ?? ""; + assert.ok(description.length >= 20, "description should be descriptive"); + assert.ok(description.length <= 1024, "description should fit Pi skill metadata limits"); + assert.match(skill, /\nlicense: MIT\n/); +}); + +test("slop-review skill contains Pi-specific portability guards", () => { + const skill = readText("skills/slop-review/SKILL.md"); + + assert.match(skill, /Execution Model and Model Tier Intent/); + assert.match(skill, /Pi\/source-fidelity guard/); + assert.match(skill, /Do \*\*not\*\* use `\[ ! -t 0 \]` in Pi/); + assert.match(skill, /ask_user_question/); + assert.doesNotMatch(skill, /\/code-quality:slop/); + assert.doesNotMatch(skill, /\$\{CLAUDE_PLUGIN_ROOT\}/); + assert.doesNotMatch(skill, /AskUserQuestion/); +}); + +test("size-review skill frontmatter is valid for Pi discovery", () => { + const skill = readText("skills/size-review/SKILL.md"); + + assert.match(skill, /^---\n/); + assert.match(skill, /\nname: size-review\n/); + const description = (skill.match(/\ndescription:\s*(?:[>|]\n)?([\s\S]+?)\nlicense:\s*MIT\n/)?.[1] ?? "") + .replace(/\s+/g, " ") + .trim(); + assert.ok(description.length >= 20, "description should be descriptive"); + assert.ok(description.length <= 1024, "description should fit Pi skill metadata limits"); + assert.match(skill, /\nlicense: MIT\n/); +}); + +test("size-review preserves source threshold and stack analysis behavior", () => { + const skill = readText("skills/size-review/SKILL.md"); + + assert.match(skill, /More than \*\*20 files changed\*\*/); + assert.match(skill, /More than \*\*500 lines added\*\*/); + assert.match(skill, /More than \*\*30 commits\*\*/); + assert.match(skill, /\*\*3 or more top-level directories touched\*\*/); + assert.match(skill, /raw → .* after exclusions/); + assert.match(skill, /Cumulative/); + assert.match(skill, /Slice/); + assert.match(skill, /references\/default-exclusions\.md/); + assert.match(skill, /ask_user_question/); + assert.doesNotMatch(skill, /\/code-quality:size/); + assert.doesNotMatch(skill, /\$\{CLAUDE_PLUGIN_ROOT\}/); + assert.doesNotMatch(skill, /CLAUDE_SIZE_REVIEW\.md/); +}); + +test("prompt aliases point at the correct skills", () => { + const slopPrompt = readText("prompts/code-quality-slop.md"); + const sizePrompt = readText("prompts/code-quality-size.md"); + + assert.match(slopPrompt, /^---\n/); + assert.match(slopPrompt, /description: Run an AI slop\/code-quality review/); + assert.match(slopPrompt, /argument-hint: "\[scope\]"/); + assert.match(slopPrompt, /Use the `slop-review` skill/); + assert.match(slopPrompt, /\$ARGUMENTS/); + assert.doesNotMatch(slopPrompt, /\/code-quality:slop/); + + assert.match(sizePrompt, /^---\n/); + assert.match(sizePrompt, /description: Run a PR or branch size review/); + assert.match(sizePrompt, /argument-hint: "\[scope\]"/); + assert.match(sizePrompt, /Use the `size-review` skill/); + assert.match(sizePrompt, /\$ARGUMENTS/); + assert.doesNotMatch(sizePrompt, /\/code-quality:size/); +}); + +test("slop-review language reference files are bundled", () => { + for (const reference of ["go", "python", "rust", "svelte-ts"]) { + const content = readText(`skills/slop-review/references/${reference}.md`); + assert.match(content, /^# .+AI Slop Signals/m); + } +}); + +test("size-review default exclusions are bundled", () => { + const content = readText("skills/size-review/references/default-exclusions.md"); + + assert.match(content, /^# Universal default exclusions for size-review/m); + assert.match(content, /go\.sum/); + assert.match(content, /\*\*\/package-lock\.json/); + assert.match(content, /\*\*\/\*\.pb\.go/); + assert.match(content, /\*\*\/__generated__\/\*\*/); +}); + +test("README documents both portable review workflows", () => { + const readme = readText("README.md"); + + assert.match(readme, /parallel agent tool/); + assert.match(readme, /sequential/i); + assert.match(readme, /\/code-quality-slop/); + assert.match(readme, /\/skill:slop-review/); + assert.match(readme, /\/code-quality-size/); + assert.match(readme, /\/skill:size-review/); + assert.match(readme, /raw vs post-exclusion size/); +}); diff --git a/release-please-config.json b/release-please-config.json index 00d7015..3f328c4 100644 --- a/release-please-config.json +++ b/release-please-config.json @@ -18,6 +18,20 @@ } ] }, + "packages/pi-code-quality": { + "component": "pi-code-quality", + "package-name": "@sentiolabs/pi-code-quality", + "release-type": "node", + "initial-version": "0.1.0", + "changelog-path": "CHANGELOG.md", + "extra-files": [ + { + "type": "json", + "path": "/package-lock.json", + "jsonpath": "$.packages['packages/pi-code-quality'].version" + } + ] + }, "packages/pi-frontend-design": { "component": "pi-frontend-design", "package-name": "@sentiolabs/pi-frontend-design", diff --git a/tests/workspace-contract.test.mjs b/tests/workspace-contract.test.mjs index 46e891b..15d10e5 100644 --- a/tests/workspace-contract.test.mjs +++ b/tests/workspace-contract.test.mjs @@ -42,6 +42,26 @@ test("release-please tracks pi-arc as an independent package", () => { assertReleaseManifestTracksPackage("packages/pi-arc"); }); +test("release-please tracks pi-code-quality as an independent package", () => { + const config = readJson("release-please-config.json"); + const codeQuality = config.packages["packages/pi-code-quality"]; + + assert.ok(codeQuality); + assert.equal(codeQuality.component, "pi-code-quality"); + assert.equal(codeQuality["package-name"], "@sentiolabs/pi-code-quality"); + assert.equal(codeQuality["release-type"], "node"); + assert.equal(codeQuality["initial-version"], "0.1.0"); + assert.equal(codeQuality["changelog-path"], "CHANGELOG.md"); + assert.deepEqual(codeQuality["extra-files"], [ + { + type: "json", + path: "/package-lock.json", + jsonpath: "$.packages['packages/pi-code-quality'].version", + }, + ]); + assertReleaseManifestTracksPackage("packages/pi-code-quality"); +}); + test("pi-arc package metadata points at the workspace package", () => { const pkg = readJson("packages/pi-arc/package.json"); @@ -58,6 +78,19 @@ test("pi-arc package metadata points at the workspace package", () => { assert.ok(!pkg.bundledDependencies.includes("pi-subagents")); }); +test("pi-code-quality package metadata points at the workspace package", () => { + const pkg = readJson("packages/pi-code-quality/package.json"); + + assert.equal(pkg.name, "@sentiolabs/pi-code-quality"); + assertReleaseManifestTracksPackage("packages/pi-code-quality"); + assert.equal(pkg.repository.directory, "packages/pi-code-quality"); + assert.equal(pkg.repository.url, "git+ssh://git@github.com/SentioLabs/pi-nexus.git"); + assert.equal(pkg.homepage, "https://github.com/SentioLabs/pi-nexus/tree/main/packages/pi-code-quality#readme"); + assert.equal(pkg.bugs.url, "https://github.com/SentioLabs/pi-nexus/issues"); + assert.equal(pkg.engines.node, ">=24.0.0"); + assert.equal(pkg.publishConfig.access, "public"); +}); + test("release-please tracks pi-frontend-design as an independent package", () => { const config = readJson("release-please-config.json"); const frontendDesign = config.packages["packages/pi-frontend-design"]; @@ -104,6 +137,7 @@ test("release workflow uses idempotent npm publishing helper", () => { assert.match(workflow, /node scripts\/npm-publish-workspace-if-needed\.mjs @sentiolabs\/pi-arc/); assert.match(workflow, /node scripts\/npm-publish-workspace-if-needed\.mjs @sentiolabs\/pi-frontend-design/); assert.match(workflow, /node scripts\/npm-publish-workspace-if-needed\.mjs @sentiolabs\/pi-scriptable-statusline/); + assert.match(workflow, /node scripts\/npm-publish-workspace-if-needed\.mjs @sentiolabs\/pi-code-quality/); assert.doesNotMatch(workflow, /npm publish --workspace/); assert.doesNotMatch(workflow, /release_created/); });