remove-ai-slops

## Codex Harness Tool Compatibility This skill may include examples copied from the OpenCode harness. In Codex, do not call OpenCode-only tools such as `call_omo_agent(...)`, `task(...)`, `background_output(...)`, or `team_*(...)` literally. Translate those examples to Codex native tools: | OpenCode example | Codex tool to use | | --- | --- | | `call_omo_agent(subagent_type="explore", ...)` | `multi_agent_v1.spawn_agent({"message":"TASK: act as an explorer. ...","agent_type":"explorer","fork_context":false})` | | `call_omo_agent(subagent_type="librarian", ...)` | `multi_agent_v1.spawn_agent({"message":"TASK: act as a librarian. ...","agent_type":"librarian","fork_context":false})` | | `task(subagent_type="plan", ...)` | `multi_agent_v1.spawn_agent({"message":"TASK: act as a planning agent. ...","agent_type":"plan","fork_context":false})` | | `task(subagent_type="oracle", ...)` for final verification | By default, record a self-review in the notepad: re-read the diff, run diagnostics, and capture evidence for every acceptance criterion. Only when the user demanded strict, rigorous, or high-accuracy review, use `multi_agent_v1.spawn_agent({"message":"TASK: act as a rigorous reviewer. ...","agent_type":"lazycodex-gate-reviewer","fork_context":false})`; include `fork_context: false`. | | `task(category="...", ...)` for implementation or QA | `multi_agent_v1.spawn_agent({"message":"TASK: act as an implementation or QA worker. ...","agent_type":"lazycodex-worker-medium","fork_context":false})` | | `background_output(task_id="...")` | `multi_agent_v1.wait_agent(...)` for mailbox signals | | `team_*(...)` | Use Codex native subagents via `multi_agent_v1.spawn_agent` and `multi_agent_v1.wait_agent`; use `multi_agent_v1.send_input` and `multi_agent_v1.close_agent` only when exposed in the active tools list | Role-specific behavior must be described in a self-contained `message`. Use `fork_context: false` to start the child with only the initial prompt (no parent history); use `fork_context: true` only when full parent history is truly required. Include any required conversation context, files, diffs, constraints, and requested skill names directly in the spawned agent's `message`. OMO installs these selectable agent roles into `~/.codex/agents/`: `explorer`, `librarian`, `plan`, `momus`, `metis`, `lazycodex-code-reviewer`, `lazycodex-qa-executor`, and `lazycodex-gate-reviewer` - pass the matching name as `agent_type` so the child gets that role's model and instructions. Inspect the actual spawn schema: whenever `agent_type` is exposed, EVERY spawn MUST select an exact LazyCodex role, on V1 or V2. Implementation difficulty selects `lazycodex-worker-low`, `lazycodex-worker-medium`, or `lazycodex-worker-high`; clone QA can select `lazycodex-clone-fidelity-reviewer`. Never select generic `worker` or `default`. If a code block below conflicts with this section, this section wins. Codex exposes ONE of two subagent tool surfaces per session; check your own tool list and route accordingly. If `multi_agent_v1.*` tools exist, use the table above as written. If instead a flat `spawn_agent` with a required `task_name` exists (`multi_agent_v2`), rewrite every `multi_agent_v1.*` example: `multi_agent_v1.spawn_agent({...,"fork_context":false})` becomes `spawn_agent({"task_name":"<lowercase_digits_underscores>","message":...,"agent_type":...,"fork_turns":"none"})` (`"all"` only when full parent history is truly required); `send_input` becomes `send_message`; do not call `close_agent`/`resume_agent` (finished agents end on their own; `followup_task` re-tasks one, `interrupt_agent` stops one); `wait_agent` takes only `timeout_ms` and returns on any child mailbox activity. Do not infer role support from V1/V2 or a model version. Legacy-schema exception: only if the actual schema lacks `agent_type`, omit that unsupported field and carry the complete role instructions in `message`, with history explicitly disabled. This cannot select a specialized TOML; an installed managed default supplies the medium worker for unnamed non-forks. The guard has no schema metadata and rejects unnamed calls, so report incompatible routing instead of retrying generically. Every deliberate full-history fork must still name its role: Codex skips role application on unnamed full-history forks, an upstream gap no LazyCodex default can repair. If a code block below conflicts with this section, this section wins. `fork_context` is rejected on `multi_agent_v2` (`fork_context is not supported in MultiAgentV2; use fork_turns instead`). When translating `load_skills=[...]`, include the requested skill names in the spawned agent's `message`. If a code block below conflicts with this section, this section wins. Omit optional keys you do not set. Never send `items: []`, `message: ""`, `model: ""`, `reasoning_effort: ""`, or `service_tier: ""`. Codex rejects them (`Items can't be empty`, `reasoning_effort must not be empty`). For work likely to exceed one wait cycle, require the child to send `WORKING: <task> - <current phase>` before long passes and `BLOCKED: <reason>` only when progress stops. A `multi_agent_v1.wait_agent` timeout only means no new mailbox update arrived; back off between waits (double the timeout up to ~5 minutes) instead of spinning short cycles. Treat a running child as alive. Fallback only when the child is completed without the deliverable, ack-only after followup, explicitly `BLOCKED:`, or no longer running. # Remove AI Slops Skill ## Inputs - **Default scope**: branch diff vs `merge-base main` (no arguments needed) - **Optional scope**: explicit file list passed by the caller (e.g., a Ralph workflow's changed-files set) ## What this skill does Cleans AI-generated slop from a bounded set of changed files while strictly preserving behavior. Locks behavior with regression tests first, then runs a categorized multi-pass cleanup, then verifies with quality gates and a critical review. Reverts and direct-edits when verification fails. The core safety invariant: **behavior is locked by green tests before a single line is removed**. A checklist alone is not safety; a passing regression test is. --- ## Categories (what counts as slop) The agent looks for these ten categories. The first three are stylistic, the next three are structural, the next two are about hidden cost, the ninth is tests, and the last is size. ### Stylistic 1. **Obvious comments** — comments restating code, trivial docstrings, section dividers, commented-out code, vague TODOs/Notes. - KEEP: comments explaining WHY (business logic, edge cases, workarounds), ticket links, regex/algorithm explanations. - KEEP: BDD markers (`# given`, `# when`, `# then`, `# when/then`). 2. **Over-defensive code** — null checks for guaranteed values, try/except around code that cannot raise, isinstance checks for statically typed params, default values for required params, backward-compat shims, redundant validation duplicated at multiple layers, **broad exception catching** (`except Exception`/`except BaseException` in Python, empty `catch {}` or `catch (e) { console.error(e) }` without narrowing in TypeScript/JavaScript). - KEEP: validation at system boundaries (user input, external APIs), I/O error handling, nullable DB fields. Top-level boundary catch-all (CLI `main()`, HTTP handler) with explicit logging + re-raise is acceptable. - REFACTOR: `except Exception` → catch the specific exception you expect. Empty `catch {}` → add `instanceof` narrowing or re-throw. `catch (e) { log(e) }` → narrow with `instanceof`, handle known cases, re-throw unknown. - PROOF REQUIRED: before deleting any validation or error handling at a trust boundary, Phase 2 must include an **adversarial** regression (malformed or hostile input) that fails if the guard is removed. No adversarial test → the guard stays. Redundant defense to remove is a duplicate of a check that already runs *inside* the boundary; a guard with no proof of redundancy is load-bearing. 3. **Excessive complexity** — deep nesting (>3 levels), nested ternaries, complex boolean expressions (combine 4+ predicates), long parameter lists (>5 args without a struct/dataclass/object), god functions (>50 lines doing many things), overly clever one-liners that sacrifice readability, `if/elif/else` chains for type/enum/literal discrimination (must be `match/case` + `assert_never`), `object` used as a type annotation (must be `Protocol`, `TypeVar`, or explicit union). - KEEP: established complexity patterns in this codebase, performance-critical hot paths that intentionally use a complex idiom. `if/else` for boolean conditions and range checks (not variant discrimination). - REFACTOR: nested if-chains → guard clauses / early returns. Complex ternaries → explicit if/else. isinstance/enum if/elif chains → `match/case` with `assert_never` on the wildcard. `object` annotations → `Protocol` (structural), `TypeVar` (generic), or union (known variants). ### Structural 4. **Needless abstraction** — pass-through wrappers, single-use helpers, speculative indirection ("we might need this later"), interfaces with one implementer where the interface adds no testability win, factory functions that just call a constructor. - KEEP: abstractions that provide a real seam (testability, multiple implementers, framework-required boundaries). 5. **Boundary violations** — wrong-layer imports (UI importing DB driver), leaky responsibilities (handler doing business logic that belongs in a service), hidden coupling (module A reads module B's private state), side effects in pure-named functions. - KEEP: pragmatic short-circuits already established as a pattern in this codebase. Flag for human judgment if unsure. 6. **Dead code** — unused imports, unused private functions/methods, unreachable branches, stale feature flags, debug leftovers (`console.log`, `print(...)`, `dbg!`), removed-but-still-referenced code. - KEEP: code referenced via reflection, dynamic dispatch, or string lookup. Code intentionally kept as a feature flag rollback path (verify with the user). ### Hidden cost 7. **Duplication** — copy-pasted branches with trivial differences, redundant helpers that do the same thing in two places, repeated literal/magic-number sequences. - KEEP: incidental duplication (two pieces of code that look similar but serve different intents that could diverge). Prefer leaving them separate over forcing a premature shared abstraction. 8. **Performance equivalences (behavior-preserving optimizations)** — changes that are provably equivalent in semantics but cheaper in time/space: - O(n²) → O(n) when correctness preserved (e.g., set lookup vs list scan) - Repeated computation inside a loop → hoist outside - Unnecessary intermediate collections (eager `list(...)` when only iterated once → generator) - String concatenation in loop → `join` - Redundant DB/API calls in a loop → batch - Redundant deep copies / clones - `.length` / `len()` recomputed inside loop → cache **Hard rule**: only apply when behavior equivalence is obvious. Do NOT change algorithms with subtle correctness implications. Do NOT micro-optimize hot paths without a benchmark. If in doubt, SKIP. ### Tests 9. **Tests: missing or junk** — tests are code in scope like any other file, judged by the one table in the `programming` skill, `references/testing/junk-classes.md` (junk classes, retention bar, R/F/C/D marks, proof rule). Two directions: - **Missing**: a behavior in the changed files that no test guards, where the repository keeps tests for that kind of behavior and a regression would otherwise pass unnoticed. ADD the narrowest test that passes the four-question authoring gate (`references/testing/README.md`); name its owner contract. A PROSE file (prompt, `SKILL.md`, rule, markdown) has no behavioral seam: cover only a machine-consumed value (parsed field, sentinel a runtime greps, a doc JSON sample through its real validator) or leave it to review. - **Junk**: mark every in-scope test declaration `R` / `F` / `C` / `D`. A `D` or `C` needs the proof the table names for its class BEFORE the cut: for a behavior class (cannot fail, mock echo, source grep, duplicate contract, layer replay, implementation mirror, fabricated postcondition, input transcript) a mutation of the production owner run against the candidate and its named keeper on the unmodified test tree; for a prose pin the assertion line plus the repo's prose rule; for a change certificate the revert that turns it red; for a seam keeper the `knip`/grep showing no production caller; for a copied inventory the generator or consumer that owns it; for a capability restatement the delivery test that owns the promise (none: `F`, not `D`); for near-duplicate cases the one table that keeps every old assertion (typecheck after). A misnamed test is re-marked by what its assertions check, never cut for its name. A mutation proves `D` only when the keeper goes red and the candidate stays green (cannot fail) or both go red (redundant); a candidate that is the ONLY red is `R`, or `C` into the keeper and re-run. Mutations run in Phase 4b, serially, by the orchestrator, never by a batch worker. A test-only production seam (export, flag, getter, reset hook) whose last test was deleted goes with it. - KEEP: everything under the retention bar (public API, protocol, config, migration, storage, security, platform, shipped-copy equality, Node-compat suites, issue regressions, machine-consumed text). A test that is red on the unchanged base is a product bug to report, never a deletion. Static or slow is never a reason to delete. ### Size 10. **Oversized modules** — any source file exceeding **250 pure LOC** (non-blank, non-comment lines). This is an architectural defect, not a style preference. Measure: `awk '!/^[[:space:]]*$/ && !/^[[:space:]]*(#|\/\/)/' <file> | wc -l`. **When found, do NOT just flag it. Execute a full modular refactoring:** 1. List every oversized file in scope: run the `awk` measure above on each source file. For `.py` files you may instead run the `programming` skill's Python checker (`uv run <programming skill dir>/scripts/python/check-no-excuse-rules.py <paths>`, rule `oversized-module`); it scans only `.py` files, so it never covers another language. 2. For each oversized file, identify distinct responsibilities (single-responsibility principle). 3. Plan the split: name each new file after the concept it owns (never `utils.py`, `helpers.py`, `common.py`, `part_1.py`). 4. Present the split plan to the user before executing. 5. Extract into clean modules with explicit `__init__.py` re-exports (re-exports ONLY, no logic in `__init__.py`). 6. Verify: re-measure exactly as in step 1 — every file must be ≤250 pure LOC. Run tests, typecheck, lint. **Forbidden escapes**: - Counting blanks/comments toward budget. - Splitting by token count (`foo_1.py`, `foo_2.py`) — split by what each file DOES. - Catch-all dump files (`utils.py`, `helpers.py`, `service.py`). - "It's generated" — only valid if the file lives in a build output directory. - "230 LOC, close enough" — a 230-LOC file about to grow is already over. Split now. KEEP: genuinely self-contained single-responsibility scripts (e.g., a standalone CLI checker). Opt out with the language's SIZE_OK marker in the first 10 lines (`# noqa: SIZE_OK` in Python, the form the Python checker reads; `// allow: SIZE_OK` in `//`-comment languages) and a comment explaining why. --- ## Quality Gates A pass is complete only when all applicable gates are green. Skip gates that are genuinely N/A for the project (e.g., no security scanner configured), and report `N/A` explicitly — do not silently skip. | Gate | Tool | Pass condition | |---|---|---| | Regression tests | project's test runner | all green | | Lint | project's linter | zero errors (warnings OK if pre-existing) | | Typecheck | `lsp_diagnostics` on changed files + project type-checker | zero new errors | | Unit/integration tests | project's test runner | all green (pre-existing failures noted, not introduced) | | Static/security scan | project's scanner | zero new findings, or `N/A` if not configured | --- ## Process ### Phase 0: Plan with the todo tool Create todos for all phases below. Mark `in_progress` one at a time. ### Phase 1: Determine scope If file paths were passed as arguments, that is the scope. Otherwise: ```bash git diff $(git merge-base main HEAD)..HEAD --name-only ``` Filter out: deleted files, binary files, generated/vendored files (`node_modules/`, `dist/`, `target/`, lockfiles). List the final scope. ### Phase 2: Lock behavior with regression tests (NEW — non-negotiable) For each in-scope source file: 1. Identify the public/observable behavior the file exposes (exported functions, HTTP handlers, CLI commands, classes used elsewhere). 2. Check whether existing tests cover that behavior. Use `git grep` / project test conventions to find related test files. 3. **If a behavior the cleanup could change is uncovered, write the narrowest test that passes the authoring gate (`programming/references/testing/README.md`) BEFORE editing the file**, and name the behavior it guards. It asserts observable outputs at the real boundary, never the implementation shape the cleanup is about to change, and never a value derived from the code under test. A PROSE file (prompt/`SKILL.md`/rule/markdown) is exempt: its wording is not behavior; skip the test and rely on review, or assert only a machine-consumed value. 4. Run the test suite (or at minimum the relevant tests). A test that is red on the unchanged base is recorded as a bug (baseline-red list), excluded from every proof, and neither fixed in this pass nor deleted; everything else must be **green** before any cleanup begins.