From 00b85f9fccf12e4ddcc8df787b6738d56383f7d7 Mon Sep 17 00:00:00 2001 From: Jason Barden Date: Mon, 6 Jul 2026 01:57:12 +0100 Subject: [PATCH] docs: codify recurring Phase 0-3 review findings as agent rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CLAUDE.md: REFACTOR RULES section — behaviour-frozen definition (concurrency, exception types, error paths) + mandatory declared, test-pinned, approved DEVIATIONS - c-sharp-dev: pre-report self-review checklist (consistency sweep, guards on new public APIs, factory-over-ctor, mirror-type parity, DEVIATIONS section) - c-sharp-qa: freeze tests pin exception types/error paths; new constants classes get pinning tests Co-Authored-By: Claude Fable 5 --- .claude/agents/c-sharp-dev.md | 10 ++++++++++ .claude/agents/c-sharp-qa.md | 1 + CLAUDE.md | 6 ++++++ 3 files changed, 17 insertions(+) diff --git a/.claude/agents/c-sharp-dev.md b/.claude/agents/c-sharp-dev.md index fdd1840..b62880b 100644 --- a/.claude/agents/c-sharp-dev.md +++ b/.claude/agents/c-sharp-dev.md @@ -135,6 +135,16 @@ Omitting causes `AVLN2100` build errors. All new public methods need full unit tests exercising all branches wherever possible. +## Pre-report self-review checklist (mandatory before claiming done) + +Every phase review so far has flagged the same misses. Run this sweep over the FULL diff (`git diff` + new files) before reporting completion: + +1. **Consistency sweep.** Grep the whole diff for every magic literal a new constant replaces — zero stragglers. Every new `await` gets `ConfigureAwait(false)` where the file's pattern uses it. `var` when the type is obvious from the RHS. Blank-line-before-return per style rules. +2. **Guards on new public APIs.** Every new public method and every `Factory` method validates reference-type arguments (`GuardAgainst.Null` / `ArgumentNullException.ThrowIfNull`) and degenerate inputs (empty collections) per repo convention. The reviewer has flagged this in every phase — do not ship without it. +3. **Factories, not ctors.** Records that have a `Factory` are never constructed via `new` at call sites — including pre-existing records touched by your change. +4. **Mirror-type parity.** When adding a type that mirrors an existing one (e.g. a new DU alongside `Result`), diff its public API surface against the mirror: overload matrix, implicit operators, Task/ValueTask-receiver async overloads. A missing overload here was the only review blocker in four phases. +5. **DEVIATIONS section.** For refactor/extraction tasks, list EVERY observable difference from the original — concurrency/sequencing, exception types and messages, error paths, delay sites — even ones you consider improvements. Default is to revert to frozen behaviour; a deviation ships only if pinned by a test and declared. Silent "improvements" are defects. + ## Code review checklist - [ ] Mid-level dev understands in 30 s without comments? diff --git a/.claude/agents/c-sharp-qa.md b/.claude/agents/c-sharp-qa.md index 7d2b975..3f4b602 100644 --- a/.claude/agents/c-sharp-qa.md +++ b/.claude/agents/c-sharp-qa.md @@ -14,6 +14,7 @@ You are a senior QA engineer specialising in C# 14 / .NET 10 TDD in the AStar.De 2. **Passing Tests.** All tests must pass. No exceptions. This includes tests not affected by the update. 3. **One logical concept per test.** A test that asserts more than one distinct behaviour is a design smell — split it. 4. **Test ADTs, not implementation details.** Test public API and observable behaviour, not private methods or internal state. +5. **Freeze tests pin the full failure contract.** When pinning behaviour ahead of a refactor, assert exception types and error paths — not only happy-path values. Green happy-path tests are blind to concurrency and exception-type changes. New constants classes get pinning tests in the same commit. ## Stack and tooling diff --git a/CLAUDE.md b/CLAUDE.md index 97180e3..a348c85 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -41,6 +41,12 @@ Read the request literally. "Make the exports work" means fix the bug that cause ### Stop immediately when told to stop If the user says "DO NOT CHANGE ANYTHING", stop all tool calls immediately. Explain in text only. Do not continue making edits while explaining a mistake — that compounds the error. +## REFACTOR RULES + +Behaviour-frozen means NO observable change: sequencing/concurrency (e.g. a sequential `await` loop must not become `Task.WhenAll`), exception types and messages, error paths, timing/delay sites. Green tests do NOT prove behaviour is frozen — tests are blind to concurrency and exception types unless they pin them. + +Any deviation from frozen behaviour must be (a) pinned by a test, (b) listed in an explicit DEVIATIONS section of the dev agent's report, and (c) approved before commit. Undeclared deviations found in review are workflow failures, not nits. Default when unsure: revert to the frozen behaviour — never improve silently. + ## Architecture Targets **net10.0** across all projects. `TreatWarningsAsErrors` enabled in Debug and Release.