software-engineering-discipline

Engineering discipline for AI coding agents working on maintained code. Apply when implementing features, fixing bugs, refactoring, reviewing code, or designing and evaluating test…

Garming

@wujiaming88

Install

$ openclaw skills install @wujiaming88/software-engineering-discipline

Software Engineering Discipline

You already know SOLID, DRY, YAGNI, design-by-contract, Clean Architecture. This skill does not re-teach them. It fixes the two things you actually get wrong: making the wrong trade-off call (over-abstracting, over-engineering, guessing) and crossing a hard limit (data loss, silent contract breaks, calling things that don't exist).

Prime directive: Leave the codebase more coherent than you found it, and write code a top engineer would sign off on — clear, cohesive, correctly scoped. A passing test is necessary, not sufficient: code that works but corrodes the design is a defect.

For genuine throwaway work — a scratch script, a spike you will delete, a one-off you won't commit — relax the process ceremony, never the RED LINES. Size is not the test; "will anyone maintain this?" is.


RED LINES (never cross, regardless of how small the change is)

Do not size-gate these. A 5-line migration or 10-line auth change is exactly where these bite.

  1. Verify before you use it. Every API, method, import, flag, config key, constant — confirm it exists before calling it (read the source / grep the symbol / check the installed version, not your memory). Never present a guess as fact. If you genuinely cannot verify (e.g. a prod-only config key unreachable from here), write # [UNVERIFIED: reason] as a code comment AND flag it in the handoff — this is an escalation, not a free pass. A guess you could have checked but didn't is a violation; [UNVERIFIED] does not launder it.
  2. Never break a published contract silently. A signature/behavior/wire-format/schema change is an API change: find the callers, migrate or version them.
  3. Migrations are expand → migrate → contract. Never drop/rename a column (or remove a field) in the same release as the code that stops using it. Reads stay backward-compatible.
  4. Never weaken the test signal to make it pass. Do not delete, skip, quarantine, loosen assertions, blindly update snapshots, or add retries to hide a failure. Fix the code or the test's correctness. Never claim an unexecuted check passed. For a bug, capture the failing regression first and retain it.
  5. Never swallow an error into a silent default. No except: return None / empty catch {}. Handle it, or let it propagate with context.
  6. Secrets/PII never touch code or logs. Read from the existing secret/config mechanism; never hardcode (incl. tests/fixtures), never commit .env, redact in errors.
  7. Authorize at the boundary — not just validate shape. Parameterize queries; treat all external input as hostile.
  8. Every remote call has a timeout and a defined failure behavior (retry-with-backoff / fallback / fail-closed). Retried or at-least-once operations need an idempotency key.
  9. Scope is bounded. Every changed line must trace to the request. A refactor and a feature never share one commit — split them even when the refactor is necessary to land the feature. Only mechanical scope the feature forces (a rename it requires, a migration it needs) may ride along; a behavior-preserving restructure gets its own commit first. Unrelated "improvements" never ride along.

TRADE-OFF CALLS (where your defaults are wrong — bias as stated)

You default to too much structure. These pull you back. When unsure, pick the simpler option. An explicit user request overrides these defaults — if the user tells you to extract/abstract/split now, do it (don't argue with the trade-off); these govern only your own unprompted choices.

  • Abstraction: Introduce an interface/base class only when ≥2 real implementations exist now, or a real boundary needs a test double. One implementation → concrete class. Do not create a framework or a new abstraction only to raise coverage.
  • Branch vs. polymorphism: A plain if is usually correct. Reach for a strategy/subclass only when variants form a stable, growing set added by different owners. Unsure → if.
  • DRY: Deduplicate the same knowledge/decision, not code that merely looks alike. Two functions with similar shape but different reasons to change → leave them. Wait for the third occurrence before extracting coincidental similarity.
  • Cohesion: A unit does one thing. If its name needs an "and", split it. Don't smear one responsibility across files, or pile unrelated ones into one class.
  • Naming > cleverness: A clear name + a plain function beats a clever one-liner. Code is read far more than written.
  • Follow the local convention even if you'd personally do it differently. Match the sibling code's style, error handling, and layering before importing your own taste.
  • Boundaries: Call the public interface, don't reach into a layer's internals. Core/domain logic must not import IO/framework/UI. If a clean change needs to cross a boundary, say so — don't smuggle it.

BEFORE YOU WRITE (for any non-trivial change)

State these first. If you can't, you don't understand the change yet — go read.

  • Understand: read the module you're touching AND its callers/callees; trace data in/out; read the existing tests (they encode the contract); find the sibling pattern and follow it.
  • Contract: for the unit you're adding/changing, state inputs · outputs · preconditions · postconditions · error modes. Validate untrusted input at the edge; assert impossible-to-violate invariants in the core (don't sprinkle asserts everywhere).
  • Blast radius: files/callers touched · contracts or wire-formats that move · migrations/flags introduced · how to roll back. If the radius is large or a boundary/contract/migration moves, surface it before implementing.
  • Test strategy: map each changed behavior to its risks, the cheapest stable test boundary, relevant success/edge/error cases, and repository-verified commands. Reuse existing tests when they already prove the behavior.

TESTING — prove the change, not the checkbox

Use Contract → Risk → Test boundary → Cases → Evidence:

  1. Contract: state the observable behavior, not the implementation detail.
  2. Risk: identify what this change can break and the cost of missing it.
  3. Test boundary: choose the cheapest stable boundary that can prove the behavior.
  4. Cases: cover success plus the relevant boundaries and declared error modes.
  5. Evidence: run the checks and report exactly what their scope proves.

Choose the smallest reliable test portfolio:

  • Unit: pure logic, business rules, and state transitions.
  • Boundary: exercise actual input or state limits — below/at/above, empty, null, or invalid only where the contract admits them.
  • Integration: use when behavior crosses a database, filesystem, queue, network adapter, or module boundary; prefer controlled real components when practical.
  • Contract: use when a published API, event, schema, or serialization format changes.
  • E2E: reserve for a few critical user journeys; do not make it the default proof for local logic.

Apply these quality rules:

  • For a bug, observe the regression test fail before the fix and pass after it. For a behavior-preserving refactor, reuse existing tests or add characterization tests only where behavior is not pinned.
  • Assert public behavior, state, and side effects. Mock only true external boundaries; do not mock away the behavior under test or replace it with a giant snapshot.
  • Control clocks, randomness, network, global state, and ordering. Treat flaky tests as defects; never hide them with retries.
  • Honor existing line/branch coverage gates and never lower them. When existing tooling covers the affected scope, run and report it; do not invent a percentage or treat coverage as proof of assertion quality.
  • Treat unexpected testing difficulty as a design signal, not automatic permission to refactor. Make only the smallest seam the requested behavior genuinely needs.
  • If the environment blocks required evidence, do not fabricate it; report the blocker, the closest reproducible check, and the exact command still needed.

Verify from cheapest to broadest: targeted tests → affected suite → repository-required type/lint/build checks → full suite when required or reasonably affordable. Discover commands from the repository; never guess them.

DEPENDENCIES & OBSERVABILITY

  • Don't add a dependency for something trivial. Prefer stdlib/existing deps; check the manifest before importing; don't pull a package (and its tree) to solve a one-liner; watch for typosquat-adjacent names.
  • Emit signals at boundaries, state transitions, and failure paths — structured, with correlation/request IDs. Not inside hot loops, not on the happy path of pure functions.

DELIVERY GATE — must pass before you report "done"

Run this gate before reporting "done". In the handoff, provide compact evidence instead of pasting a ritual checklist unless the user asks for it. For review-only work, report missing test evidence and risk; do not force code or test changes.

□ Contract declared and matches the implementation
□ Changed behavior mapped to the smallest reliable tests; relevant boundaries and declared error modes covered
□ If applicable: bug regression observed red→green; refactor behavior pinned by existing or characterization tests
□ Exact commands run; results distinguish targeted, affected-suite, and full-suite scope and name the new cases
□ Existing coverage gates honored; affected coverage reported when repository tooling exists
□ Every API/import/config/constant was verified — NOT ticked by slapping [UNVERIFIED] on a guess. An **un-escalated** unverified RED-LINE-#1 symbol = NOT done. A genuinely un-checkable item is allowed only as [UNVERIFIED] + ⚠ + escalation in the handoff
□ If the environment blocked running tests/migrations (no net/DB/sandbox): do NOT tick "tests ran" — mark it ⚠, state exactly what you couldn't run and why, and hand off the exact command for the user to run
□ No RED LINE crossed
□ Blast radius stated; every changed line traces to the request
□ Handoff note: what changed · why · what you verified · risks · what you deliberately did NOT do

Loop: Understand → Contract + Blast radius → (confirm if large) → Implement (simplest fit, follow local convention) → Test edges & errors → Delivery gate.

Top skills in this category