Imported from piers-sinclair/Dostar (.claude/skills/code-quality/SKILL.md). Install upstream with npx skills add piers-sinclair/Dostar --skill code-quality. Copyright stays with the author.

code-quality

Audit code for quality issues across a defined set of principles and report findings before making any changes.

Usage

/code-quality [path]

• No argument → audits files changed on the current branch (git diff main...HEAD --name-only)
• Path given → audits all source files under that path recursively

Workflow

1. Determine scope: resolve the file list (diff or path)
2. Read every file in scope
3. Audit each file against every category below
4. Report all findings grouped by category, with severity, file reference, what was found, and a suggested fix
5. Ask the user which findings (if any) they want applied — do not touch any file until they confirm
6. For any bug or logic error finding the user confirms: write a failing test that reproduces the bug before applying the fix (TDD regression rule). Choose the layer that matches the bug: unit test for business logic, integration test for HTTP/DB behaviour. The test must fail on the unfixed code and pass after.

Severity levels

Reporting format

For each finding, output:

[<Severity>] <Category> — <file>:<line or range>
  Found: <what was found>
  Fix:   <suggested change>

Group findings by category. At the end, print a summary table:

Then ask: "Which findings would you like me to fix? List numbers, 'all', or 'none'."

Audit categories

Shared (all file types)

1. SOLID

All five principles must be checked. For each one, either report a finding or explicitly confirm it is clean. Do not silently skip any principle.

• SRP (Single Responsibility): flag classes or files doing multiple unrelated things — e.g. a service that also owns HTTP-response shaping, a module class mixing business logic with DI wiring.
• OCP (Open/Closed): flag if/switch chains that would need editing to add a new case where a strategy or polymorphism pattern would avoid the edit.
• LSP (Liskov Substitution): flag subclasses that override base behaviour in ways that break the base contract (throwing where the base didn't, returning null where the base promised a value).
• ISP (Interface Segregation): flag interfaces with methods that some implementors leave unimplemented or stub out.
• DIP (Dependency Inversion): flag high-level classes that directly instantiate (new ConcreteType()) low-level dependencies that should be injected.

2. DRY (Do Not Repeat Yourself)

Flag proven duplicate logic only — the same algorithm, query expression, or decision tree appearing in two or more places verbatim or near-verbatim. Do not flag structural similarity alone (two classes that both have a GetById method are not a DRY violation unless the bodies are the same).

3. Composition over Inheritance

Must be checked for all classes. Report "clean" explicitly if no violations are found.

Flag inheritance chains deeper than one level (excluding framework base classes such as DbContext, AbstractValidator, IEndpointFilter). Suggest an interface + composition alternative when the inheritance is purely for code reuse rather than a true is-a relationship.

4. Separation of Concerns

Must be checked for all layers. Report "clean" explicitly if no violations are found.

Flag:

• Business or domain logic inside endpoint handlers (should be in a service)
• Data-access logic (EF Core queries) outside the DbContext or a dedicated repository
• Cross-cutting concerns (logging, auth checks, validation) inlined in business code rather than handled by middleware or endpoint filters

5. Loose Coupling

Must be checked for all modules. Report "clean" explicitly if no violations are found.

Flag:

• new ConcreteType() for a dependency that should be injected via constructor
• A module referencing another module's .Implementation project instead of .Contracts
• Hard-coded external URLs or connection strings embedded in business logic

6. KISS (Keep It Simple)

Flag:

• Deeply nested if/for blocks that could be early-returned or flattened
• Abstractions (base classes, generics, design patterns) added speculatively with only one concrete use today
• Overly clever one-liners that reduce readability (complex LINQ chains, ternary-in-ternary, etc.)

7. Magic strings and numbers

Flag any string or numeric literal that:

• Has domain or configuration meaning (status codes, route segments, header names, connection string keys, permission names, etc.)
• Appears more than once, OR
• Appears once but is non-obvious without context

Expected fix: extract to a named private const or static readonly field in the same class/file.

8. Naming

Flag:

• Abbreviations (mgr, svc, tmp, d, e outside of catch clauses)
• Misleading names (a method called GetUser that also saves to the DB)
• Generic names when a specific one exists (data, result, item, obj)
• Inconsistent casing (mixing camelCase and PascalCase for the same kind of symbol)
• Boolean variables or properties not phrased as a question (isActive, hasPermission, not active, permission)
• Extension methods misused as general-purpose utilities: a method in a static class XxxExtensions should attach behaviour to the type of its this parameter. If neither parameter is clearly the receiver (e.g. a math utility taking two decimal values, or a converter taking two unrelated types), it belongs in a named static class (MathUtils, DateUtils, etc.) — not an extension method, which would imply a type relationship that doesn't exist

9. Comments

Default: no comments. Code should explain itself through naming, types, and structure. A comment is only warranted when none of those can express the information.

Before adding a comment, ask: can I restructure the code so the comment is unnecessary? If yes, restructure — do not add the comment.

Allow only (and only when restructuring is not an option):

• Workaround comments: "X is intentional because of [specific external constraint or bug]"
• Non-obvious invariant explanations that cannot be expressed in types or names
• External constraint references (spec section, RFC, third-party quirk)

Flag:

• Any comment that explains what the code does — the code itself should do that
• Any comment that could be avoided by renaming, extracting a method, using a better type, or restructuring control flow

Do not flag TODO/FIXME/HACK if they reference a known issue or ticket.

Backend (.NET-specific)

Apply these checks only to .cs files.

10. Strict nullability

Flag:

• ! (null-forgiving operator) — first ask whether restructuring eliminates it entirely. Common alternatives:
  • Use a field initializer with a primary constructor parameter instead of assigning in a lifecycle method (_db = fixture.CreateDbContext() rather than _db = null! + assignment in InitializeAsync)
  • Use required on properties set by the caller
  • Use = string.Empty or = [] for collections/strings
  • Only if none of those apply and null! is truly unavoidable: add a comment explaining the non-obvious invariant that makes null impossible (apply Category 9 standards — WHY, not WHAT)
• Reference-type or string properties that are neither required, initialised (= string.Empty, = [], etc.), nor explicitly nullable (?) — these silently produce nullable warnings or require suppression
• Nullable return type (T?) on a method where null is semantically impossible (prefer an exception or Option pattern)
• Missing ? on a return type or parameter that can legitimately be null

Do not flag required on properties that have a default factory or are optional by design.

11. Latest .NET syntax

Flag where a modern equivalent exists and improves clarity:

Do not flag framework-mandated patterns (e.g. EF Core entity classes cannot always be records).

12. Exception handling

Flag:

• catch (Exception) or catch (Exception e) — too broad; catch the most specific exception type
• Swallowed exceptions: catch { }, catch (Exception) { }, or catch that only logs without rethrowing or returning an error result
• Exceptions used for control flow (throwing to signal an expected "not found" state)
• try/catch blocks that simply let the exception propagate unchanged — this is redundant; delete the try/catch and let the global UseExceptionHandler middleware in Program.cs handle it

Local try/catch is appropriate only for genuinely local recovery: retrying a transient operation, translating a third-party exception into a domain type, or releasing a resource that using cannot manage.

13. Async/await correctness

Flag:

• .Result, .Wait(), or .GetAwaiter().GetResult() on a Task — these block threads and risk deadlocks
• async void methods (except event handlers — explain if this is one)
• A method named without the Async suffix that returns Task or Task<T>
• A method that has a CancellationToken available (e.g. via HttpContext.RequestAborted, a parameter, or stoppingToken) but does not pass it to async calls that accept one
• Redundant await: return await SomeAsync() at the end of a non-try method — can be return SomeAsync()

14. EF Core / PostgreSQL best practices

Apply only to files that use DbContext or EF Core query syntax.

Flag:

• Missing AsNoTracking() on read-only queries (queries whose results are never mutated and saved back) — unnecessary change tracking wastes memory
• N+1 query patterns — iterating a collection and calling the DB per item; fix with Include()/ThenInclude() or a single projected query
• Loading full entities when a projection suffices — db.Todos.ToListAsync() then mapping in code instead of db.Todos.Select(x => new TodoDto(...)).ToListAsync()
• Synchronous EF Core methods — ToList(), FirstOrDefault(), Count(), etc. must be ToListAsync(), FirstOrDefaultAsync(), CountAsync() etc.
• Bulk operations that load entities unnecessarily — loading entities only to delete or update them; prefer ExecuteDeleteAsync() / ExecuteUpdateAsync() (EF Core 7+) for set-based operations. Note: these methods are not supported by the InMemory provider — only apply this optimisation if unit tests use a real database (e.g. Testcontainers); otherwise keep the load-then-remove pattern
• DateTime instead of DateTimeOffset for timestamp columns — DateTimeOffset maps to timestamptz in PostgreSQL and preserves timezone context; DateTime maps to timestamp without time zone
• Case-insensitive string comparisons using ToLower()/ToUpper() — x.Title.ToLower() == input.ToLower() generates LOWER() SQL that prevents index use; prefer EF.Functions.ILike(x.Title, input) for PostgreSQL case-insensitive search
• Raw SQL with string interpolation — FromSqlRaw($"... {value}") is an injection risk; use FromSqlInterpolated($"... {value}") or parameterised FromSqlRaw("... {0}", value)
• Guid.NewGuid() as a primary key where sequential ordering matters — Guid.CreateVersion7() generates time-ordered GUIDs that perform better as clustered index keys in PostgreSQL

15. Test quality

Apply only to test files (*Tests.cs, *Tests/*.cs).

Flag:

• Method name does not follow Method_Scenario_ExpectedBehaviour pattern — use WhenCalled as the scenario when there is no special precondition (e.g. LiveEndpoint_WhenCalled_ReturnsHealthy)
• Shared mutable state between tests (static fields, shared DbContext instances)
• Assertions using Assert.* (xUnit) or FluentAssertions — must use Shouldly
• A single [Fact] covering multiple unrelated scenarios (each scenario needs its own [Fact])
• InMemoryDatabase not using Guid.NewGuid().ToString() as the DB name — tests must be fully isolated
• Infrastructure classes missing [ExcludeFromCodeCoverage]: DbContext subclasses and service classes whose methods call provider-specific APIs (e.g. EF.Functions.ILike) cannot be meaningfully unit tested — they must run against a real database. Without [ExcludeFromCodeCoverage], they silently drain the 80% line-coverage threshold, making the gate misleading. Flag any such class that is covered exclusively by integration tests and is not decorated.

16. Access modifiers / Least Exposure (.NET)

Apply the principle of least privilege to every type and member. Use the most restrictive access modifier that still satisfies all callers.

Flag:

• public types that are never used outside their own assembly — they should be internal. Common examples:
  • Middleware classes in the same assembly as Program.cs (e.g. public class FooMiddleware → internal sealed class FooMiddleware)
  • Helper/utility classes only consumed within one project
  • Domain entities and DbContext that never cross an assembly boundary
• public or internal members that are only used within the same class — they should be private
• Instance members that could be static — if a method or field has no instance state, declare it static
• Mutable public properties that are only ever set by the owning class — prefer { get; private set; } or { get; init; }

Exceptions — leave public when:

• The type is used by cross-assembly callers (e.g. a type referenced from a different project)
• The type participates in System.Text.Json deserialization via reflection — JSON deserialization requires a public type and public constructor (e.g. request record types in Minimal API handlers)
• The type is registered with the DI container and activated via ActivatorUtilities — the default activator requires a public constructor on the implementation type (e.g. service classes, validators)
• The type inherits from or implements a framework-mandated public base (e.g. DbContext, AbstractValidator<T>)
• xUnit test classes and fixtures — xUnit requires public for test discovery and IClassFixture<T> resolution

Frontend (TypeScript/React-specific)

Apply these checks only to .ts and .tsx files.

17. TypeScript strictness

Flag:

• any type — suggest unknown and a type guard, or a proper named type
• ! non-null assertion, except on well-known always-present DOM nodes (e.g. document.getElementById('root')! in main.tsx is acceptable; add a comment if it is not obvious)
• as T type assertion — first ask whether restructuring (overloads, narrower types, type guards) eliminates the cast. If the cast is genuinely unavoidable (e.g. a well-typed generic function where the caller's type parameter determines the shape), no comment is needed. Only add a comment if the cast is non-obvious AND restructuring is not an option (apply Category 9 standards)
• Exported functions or React components missing an explicit return type annotation — for React components use JSX.Element; note that JSX is not a global in projects using the new React JSX transform, so add import type { JSX } from 'react' (or inline it: import { useState, type JSX } from 'react') to any file that uses it

18. Access modifiers / Least Exposure (TypeScript)

Flag:

• export on functions, constants, or types that are only used within the same file — remove the export
• Module-level const/let that could be scoped inside the function that uses it — move it inward
• Missing readonly on class properties and object fields that are never reassigned after construction
• public class members that should be private — if a method or property is only called from within the same class

19. Test quality (frontend)

Apply only to test files (*.test.ts, *.test.tsx, *.spec.ts, *.spec.tsx).

Flag:

• Tests not following Arrange / Act / Assert structure
• Shared mutable state between tests
• A single test covering multiple unrelated scenarios

Conventions reminder

• Never apply a fix before the user confirms which findings to address.
• When applying fixes, change only the code required to resolve the finding — no opportunistic refactoring.
• If a finding requires a judgement call (e.g. whether a method truly violates SRP), present both sides briefly and let the user decide.
• After applying fixes, run dotnet build (for backend changes) or pnpm build from frontend/ (for frontend changes) and confirm 0 errors/warnings before reporting done.