Add design rules and review questions (#4)

Fold maintainability practices into the skill: a Design section in
SKILL.md (ownership, boundaries, effects, failure, dependencies, change
surface, tests, plus a conflict priority order), domain-specific items
in the matching quality-gate rows, and a review-questions list applied
to Substantial work and above. README mirrors the new behavior.

Author: elkaix

README.md

@@ -19,6 +19,9 @@ the risk, and an honest `COMPLETE` / `PARTIAL` / `BLOCKED` report.
   to the matching step.
 - **Evidence scaled to risk.** Pre-fix evidence, recorded commands, and independent review
   scale with the mode. A skipped or unavailable check never counts as a pass.
+- **Design rules.** Seven rule groups for the code the agent writes (ownership, boundaries,
+  effects, failure, dependencies, change surface, tests) with a priority order for when
+  they conflict, and review questions to answer against non-trivial diffs.
 - **References on demand.** [Quality gates](references/quality-gates.md) for affected
   domains and a [report template](references/report-template.md) for non-trivial work.
 

SKILL.md

@@ -56,7 +56,8 @@ deterministic reproducer or directly observed failure. Never fabricate a red run
 
 **Implement.** Fix the invariant at the smallest correct shared layer. Infer architecture
 from the code and reuse its conventions and helpers; do not impose patterns because they
-are familiar. Wire every required path: unused helpers and fake success paths are not
+are familiar, and apply the design rules in section 3 to the code you add. Wire every
+required path: unused helpers and fake success paths are not
 delivery. Validate untrusted input at boundaries and make failure explicit; never swallow
 errors, fabricate data, or weaken authorization. Preserve compatibility unless the change
 is meant to break it. Regenerate derived files with project tooling. Add a dependency only
@@ -82,10 +83,61 @@ correctness when available, without blocking required work; its absence is a rep
 limitation, not by itself a reason for `PARTIAL`. Self-review, including of a child agent's
 work, is not independent. Rerun affected checks after the last relevant edit; an old green
 run does not verify a new tree. When available, load
-[quality gates](references/quality-gates.md) only for affected domains; if a reference is
+[quality gates](references/quality-gates.md) only for affected domains, and for Substantial
+work and above answer its review questions against the final diff; if a reference is
 missing, use this protocol and note it only when it materially reduces verification.
 
-## 3. Safety
+## 3. Design
+
+Rules for the code you write, applied within the repository's own structure. The test for
+every addition: am I adding code, or a new place future readers must understand?
+
+- **Ownership.** One coherent responsibility per function, class, and module, named after
+  the concept it owns. Every rule (threshold, validation, mapping, permission, option
+  list, formula, schema, retry policy) has one authoritative owner: duplicated code is
+  tolerable, duplicated rules are defects. Keep policy (what should happen) apart from
+  mechanism (how it happens).
+- **Boundaries.** Parse, validate, and normalize once where data crosses a network, file,
+  database, process, trust, or serialization boundary, then pass a typed canonical value
+  inward. Choose types that make invalid states unrepresentable: one status enum over
+  several booleans. Model lifecycles as explicit state machines with named transitions.
+- **Effects.** Compute first, mutate second. For consequential operations, prepare →
+  validate → commit, with the irreversible step as small as possible. Side effects show in
+  names and signatures. Every resource has an owner whose cleanup runs on success,
+  exception, cancellation, timeout, and partial initialization.
+- **Failure.** Every external call has a deliberate timeout. Bound anything that grows:
+  retries, queues, concurrency, input and payload size, batches, recursion, caches,
+  request duration, log volume. Retry only transient failures (timeout, rate limit,
+  unavailable), never permanent ones (invalid input, authorization, violated invariant),
+  and make retried operations idempotent. Errors carry what failed, where, on which
+  input, whether retry is appropriate, and the cause; expected outcomes are values,
+  exceptions are for the exceptional.
+- **Dependencies.** Pass volatile dependencies in (clock, randomness, identifiers,
+  filesystem, network, database, providers) instead of reading globals. Keep interfaces
+  smaller than implementations and translate provider-specific details at the owning
+  boundary. An abstraction earns its place by removing knowledge from callers; otherwise
+  delete it. Generalize recurring concepts, not hypothetical requirements: one focused
+  module beats a configurable internal framework.
+- **Change surface.** One feature touches few places: a new provider adds an
+  implementation, a new status adds an enum member. Name constants beside the policy that
+  owns them; use enums for closed domains, keyword arguments over positional booleans, and
+  tables when behavior varies only by configuration. Comments explain why: constraints,
+  tradeoffs, external requirements. Consolidate overlapping implementations and give every
+  compatibility layer a deletion condition.
+- **Tests.** Capture existing behavior before restructuring; change behavior in a separate
+  step. Test observable behavior over private internals. Test boundaries (invalid,
+  malformed, empty, maximum, timeout, cancellation, duplicate execution, partial failure,
+  missing resource, permission failure, concurrent access) and invariants (balance never
+  negative, path never escapes root, completed job never reruns, duplicate request yields
+  one effect, resource always released). Control clocks, randomness, identifiers, and
+  external services so tests fail only when code is wrong, through the same architecture
+  production uses.
+
+When rules conflict: correctness → explicit invariants → clear ownership → controlled side
+effects → bounded resources → failure safety → locality → replaceability → extensibility →
+optimization.
+
+## 4. Safety
 
 - Stay inside the workspace and task scope. Never print environment values, credentials,
   or secret files; check a variable's presence, not its value.
@@ -96,7 +148,7 @@ missing, use this protocol and note it only when it materially reduces verificat
 - Commit, push, open a PR, merge, and deploy are separate permissions. When committing,
   stage exact paths and inspect the staged diff.
 
-## 4. Findings and evidence
+## 5. Findings and evidence
 
 Record material defects you discover as you go. Interrupt only when one changes scope,
 safety, strategy, or completion status; otherwise list it in the report with a next action.
@@ -108,7 +160,7 @@ Keep enough evidence to reproduce each material claim. For performance, migratio
 security, or environment-dependent results, record exact commands, environment, and
 revision. Report only results you observed.
 
-## 5. Report
+## 6. Report
 
 | Status | Meaning |
 | --- | --- |

references/quality-gates.md

@@ -6,15 +6,15 @@ Apply only the rows the change touches. Use the repository's own commands. A gat
 | Surface | Requirements | Evidence |
 | --- | --- | --- |
 | Architecture | Preserve dependency direction; one owner per invariant and state transition; explicit, cohesive interfaces; no speculative abstraction. | Trace entry points and callers; check the final diff for duplicated policy or dead wiring. |
-| APIs and inputs | Validate type, size, range, encoding, ownership; preserve error semantics and compatibility; bound pagination and uploads. | Malformed, oversized, empty, and boundary inputs; consumer compatibility; unauthorized and cross-tenant access. |
+| APIs and inputs | Validate type, size, range, encoding, ownership; preserve error semantics and compatibility; keep public contracts (schemas, interfaces, event and persisted formats, command behavior) stable and version incompatible changes explicitly; bound pagination and uploads. | Malformed, oversized, empty, and boundary inputs; consumer compatibility; unauthorized and cross-tenant access. |
 | State and persistence | Atomicity, constraints, uniqueness, and concurrency control; exact representations where required. | Duplicate requests, lost updates, partial failure, recovery, realistic migration data. |
-| Concurrency | Clear ownership of shared state; atomic check-then-act; cancellation and deadlines propagate; ordering assumptions stated; operations idempotent where retried or redelivered. | Races, duplicate and out-of-order delivery, retry exhaustion, cancellation mid-operation, shutdown, deadlock. |
+| Concurrency | Clear ownership of shared state; atomic check-then-act; cancellation and deadlines propagate; ordering assumptions stated; operations idempotent where retried or redelivered; locks released before network, disk, inference, subprocess, or callback work; immutable values, message passing, and ownership transfer over shared mutable state. | Races, duplicate and out-of-order delivery, retry exhaustion, cancellation mid-operation, shutdown, deadlock. |
 | Resources | Pair acquisition with release; bound memory, files, sockets, tasks, queues, and pools. | Cleanup on error and cancel, repeated runs, leak checks. |
-| Security and privacy | Least privilege; authorize each sensitive operation; safe parsing and encoding; no embedded secrets; minimal sensitive data in logs. | Negative access tests; injection, path traversal, SSRF where relevant; log redaction. |
+| Security and privacy | Least privilege; secure defaults; authorize each sensitive operation in one central place regardless of entry interface; stronger validation on delete, overwrite, publish, and charge than on reads; safe parsing and encoding; no embedded secrets; minimal sensitive data in logs. | Negative access tests; injection, path traversal, SSRF where relevant; log redaction. |
 | UI and accessibility | Semantic controls, keyboard and focus, labels, responsive layout; loading, empty, error, success states; no duplicate destructive submits. | Component or browser tests, keyboard checks, supported viewports, accessibility tooling. |
-| Performance | Find the bottleneck before optimizing; keep correctness checks alongside speed. | Before/after on a representative workload with warmup, repetitions, variance, and comparable hardware and versions. |
+| Performance | Find the bottleneck before optimizing; algorithmic and architectural wins before micro-optimizations; optimize behind existing interfaces, with every fast path keeping a correct generic fallback; keep correctness checks alongside speed. | Before/after on a representative workload with warmup, repetitions, variance, and comparable hardware and versions. |
 | Build and supply chain | Honor pinned versions, lockfiles, generated sources, licenses; minimal dependencies and permissions. | Clean build, lockfile consistency, dependency review. |
-| Operations | Actionable config errors; defined rollout and rollback; no secrets or unbounded cardinality in telemetry. | Health checks, structured errors, metrics or traces, rollback steps. |
+| Operations | Typed configuration loaded and validated once at startup, with business logic receiving values rather than reading the environment; actionable config errors; defined rollout and rollback; structured logs and metrics designed with the feature that report decisions without determining them; no secrets or unbounded cardinality in telemetry. | Health checks, structured errors, metrics or traces, rollback steps. |
 
 ## Migration and rollback
 
@@ -28,3 +28,22 @@ credential changes, or deployments without explicit authorization.
 
 Characterize existing behavior before changing it. Map each affected package to its own
 toolchain; a green root command may not cover every package.
+
+## Review questions
+
+For Substantial work and above, answer each against the final diff:
+
+1. What responsibility changed, where does it live, and does another part of the code
+   already know the same rule?
+2. Does the change stay local, with dependencies pointing inward and no external
+   representation leaking into core logic?
+3. Which additions are policy and which are mechanism, and are they kept apart?
+4. What new invariant exists, and what invalid states are now possible?
+5. What happens on failure, timeout, cancellation, and duplicate execution?
+6. Who owns cleanup, what can grow without a bound, and what happens under concurrency?
+7. Is each interface smaller and more stable than its implementation, and does each
+   abstraction remove knowledge from callers?
+8. Does this make the next related change easier, and can obsolete code now be deleted?
+9. Do tests protect behavior rather than implementation details?
+10. Is the complexity justified by a real requirement, and would another engineer know
+    where to modify this behavior six months from now?