From 9c66ed991feda0cad2394a3c93a10e1f601eab06 Mon Sep 17 00:00:00 2001 From: elkaix Date: Thu, 1 Oct 2026 19:04:17 -0400 Subject: [PATCH] Add design rules and review questions 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. --- README.md | 3 ++ SKILL.md | 62 ++++++++++++++++++++++++++++++++++--- references/quality-gates.md | 29 ++++++++++++++--- 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index d57ae28..73dd241 100644 --- a/README.md +++ b/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. diff --git a/SKILL.md b/SKILL.md index d4014e4..27aacf7 100644 --- a/SKILL.md +++ b/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 | | --- | --- | diff --git a/references/quality-gates.md b/references/quality-gates.md index 31f4683..a29fee9 100644 --- a/references/quality-gates.md +++ b/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?