From 4e8a206db0ae71c9aedd418c2b26da2db3a01747 Mon Sep 17 00:00:00 2001 From: iancooper Date: Sun, 11 Oct 2026 19:43:07 +0100 Subject: [PATCH 1/5] docs: state where comments may go and add /refactor:tidy-comments documentation.md gains a "Comments in Code" section: XML docs on public members only, at most a comment above the signature elsewhere, none inside method bodies, and no issue/spec references, history or reviewer notes. code_style.md and CONTRIBUTING.md now align with it, and the authoring and review skills carry the rule. /refactor:tidy-comments applies it to a unit of work and reports comments that side-stepped a refactoring. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7 --- .agent_instructions/code_style.md | 10 +- .agent_instructions/documentation.md | 34 ++++- .agent_instructions/skills_overview.md | 6 + .claude/commands/README.md | 3 + .claude/commands/bugfix/fix.md | 5 + .claude/commands/refactor/README.md | 23 +++ .claude/commands/refactor/smells.md | 4 +- .claude/commands/refactor/tidy-comments.md | 169 +++++++++++++++++++++ .claude/commands/refactor/tidy-first.md | 2 +- .claude/commands/spec/implement.md | 1 + .claude/commands/spec/ralph-implement.md | 2 + .claude/commands/spec/review.md | 3 +- .claude/commands/tdd/test-first.md | 3 +- CLAUDE.md | 2 + CONTRIBUTING.md | 18 ++- 15 files changed, 264 insertions(+), 21 deletions(-) create mode 100644 .claude/commands/refactor/tidy-comments.md diff --git a/.agent_instructions/code_style.md b/.agent_instructions/code_style.md index 4a51c3ab41..b7675436fc 100644 --- a/.agent_instructions/code_style.md +++ b/.agent_instructions/code_style.md @@ -36,9 +36,9 @@ - As we support multiple schedulers, these should use their own assembly. - As we support multiple locking providers, these should use their own assembly. - Default to a class per source file approach, unless one class clearly exists as the details of another. -- Comments - - Do not put links or references to issues or PRs (e.g. `(issue #4089)`, `#4207`) in code or XML doc comments. The traceability lives in git history and the ADR, not in the source. - - A comment is for code that needs explanation — typically the *why* behind a non-obvious decision. Do not narrate *what* the code does, restate the code, or describe the change relative to how the code used to be. - - Inline comments are not notes to a reviewer. Context about the fix, the alternatives considered, the previous behaviour, or the history of a change is reviewer-facing rationale and belongs in the ADR, not in an inline comment. A future maintainer reading the code only needs what helps them understand the code as it now stands. - - Keep comments concise. Prefer one tight line over a paragraph. +- Comments — the rules are in [documentation.md § Comments in Code](documentation.md#comments-in-code). In short: + - `///` XML documentation on public and protected members of public types only. + - On anything else, at most a `//` or `/* */` comment before the signature, when the method is complex or guards against a common trap. + - No comments inside method bodies, as a rule: extract a method to express intent instead (except `//Arrange` `//Act` `//Assert` in tests). + - No issue, PR or spec references, no change history, no notes to a reviewer. Those belong in the ADR and git history. diff --git a/.agent_instructions/documentation.md b/.agent_instructions/documentation.md index b954dc63ea..9907885348 100644 --- a/.agent_instructions/documentation.md +++ b/.agent_instructions/documentation.md @@ -26,12 +26,9 @@ public string Status { get; set; } ``` -- Use `` for advice to developers or LLMs working with the code directly. Include information on how the method is implemented where it is not obvious from the code or significant design decisions have been made. Consider what you would want to know if maintaining this method. Use `` if you need to link to external documentation. This can also be used for more detailed information than could be included in the ``. - - Prefer to use good variable and method names to express intent, over inline comments. - - Use the refactoring "Extract Method To Express Intent" to encapsulate code in a named method that explains intent, over using a comment. - - Do not add comments for what may be easily inferred from the code. - - In tests you may use //Arrange, //Act, //Assert. - - If code has a complex algorithm or non-obvious implementation, prefer to use `/// ` +- The elements divide into two kinds: + - **How to use the API** — ``, ``, ``, ``, ``, ``. What it is for and what to pass to get the behaviour you want. + - **Notes to the user** — ``. Caveats, side effects, threading or ordering constraints, and how it is implemented where that affects how you use it, or where a significant design decision has been made. Use `` if you need to link to external documentation. This can also be used for more detailed information than could be included in the ``. - Example: ```csharp @@ -46,6 +43,31 @@ public string Status { get; set; } - Documentation comments should be changed when APIs change. - Document new features and changes in the Docs repository of the BrighterCommand organization. +## Comments in Code + +Where a comment may go depends on what it is attached to. + +| Where | What is allowed | +|-------|-----------------| +| Public and protected members of public types | `///` XML documentation, as above. Required. | +| Internal or private types and members | No XML documentation. Where a method is complex to understand, or protects against a common trap, a `//` or `/* */` comment **immediately before the signature** saying what it does or which trap it guards against. Otherwise nothing. | +| Inside a method body | **No comments, as a rule.** | + +- Express intent with code, not comments, inside a method body. If a block needs a comment to be understood, use the refactoring "Extract Method To Express Intent": give the block a name that says what it does, and, if the name is not enough, put the comment above the new method's signature. + - The same goes for the smaller refactorings: Extract Variable for a condition that needs explaining, a named constant for a magic literal, a rename for a variable whose name needs a comment. + - A comment that warns about a trap ("must be called before…", "can be null when…", "do not reorder…") often hides a refactoring that removes the trap: make the ordering impossible to get wrong, make the type say it can be null, or guard it with an assertion. +- The exceptions inside a method body are: + - `//Arrange`, `//Act`, `//Assert` in tests. + - The reason on an analyser suppression (`#pragma warning disable`, `// ReSharper disable`), on the same or preceding line. + - A single line marking a trap that cannot be extracted, such as a workaround for a bug in a third-party library at one call site. This is rare; reach for extraction first. +- Every comment, wherever it sits: + - Explains the code as it now stands. It does not narrate *what* the code does, restate the code, or describe the change relative to how the code used to be. + - Is not a note to a reviewer. The fix, the alternatives considered, the previous behaviour and the history of a change are reviewer-facing rationale; they belong in the ADR or the commit message. + - Carries no links or references to issues, PRs or specs (e.g. `(issue #4089)`, `#4207`, `Spec 0027`). The traceability lives in git history and the ADR. Referring to an ADR by number where the design decision explains the code is fine. + - Is concise. Prefer one tight line over a paragraph. +- Delete commented-out code. Git remembers it. +- `/refactor:tidy-comments` brings existing code into line with these rules. + ## Architecture Decision Records **Recommended Tool**: Use the `/adr ` command (see [.claude/commands/adr/adr.md](../../.claude/commands/adr/adr.md)) to create properly formatted ADRs. This automates numbering, template application, and spec linking. diff --git a/.agent_instructions/skills_overview.md b/.agent_instructions/skills_overview.md index 713280a52a..6bd14d84b7 100644 --- a/.agent_instructions/skills_overview.md +++ b/.agent_instructions/skills_overview.md @@ -15,6 +15,7 @@ Skills are slash commands that automate multi-step workflows and enforce Brighte | `/test-first` | TDD with an approval gate, armed by default | `/test-first <behavior description>` | | `/tidy-first` | Separate refactoring from features | `/tidy-first <change description>` | | `/refactor:smells` | Find code smells in completed work, schedule refactorings | `/refactor:smells [base-ref \| base..head \| path...]` | +| `/refactor:tidy-comments` | Bring comments into line with the comment rules; surface refactorings a comment side-stepped | `/refactor:tidy-comments [base-ref \| base..head \| path...]` | | `/adr` | Create Architecture Decision Record | `/adr <title>` | | `/bugfix:*` | Diagnosis-first bug workflow (Confirm gate) | `/bugfix:triage [issue \| description]` | @@ -75,6 +76,11 @@ Skills are slash commands that automate multi-step workflows and enforce Brighte • Opt-in, after a unit of work is green: find Fowler's code smells • Reports and schedules refactorings; does not change code + /refactor:tidy-comments [base-ref | base..head | path...] + • Delete, trim or move comments that break documentation.md § Comments in Code + • Replaces explanatory comments with Extract Method / Variable / Rename + • Reports comments that side-stepped a real refactoring, and schedules it + 📋 ARCHITECTURE DECISIONS /adr <title> • Auto-numbered, properly formatted ADRs diff --git a/.claude/commands/README.md b/.claude/commands/README.md index 1a231a04f3..55ab0d3a80 100644 --- a/.claude/commands/README.md +++ b/.claude/commands/README.md @@ -11,6 +11,7 @@ Skills are invoked using slash commands in Claude Code: /adr <title> # Create Architecture Decision Record /tidy-first <change description> # Separate structural from behavioral changes /refactor:smells [base-ref | base..head | path...] # Find code smells in completed work, schedule refactorings +/refactor:tidy-comments [base-ref | base..head | path...] # Replace comments with intent; find side-stepped refactorings ``` ## Available Skills @@ -151,6 +152,7 @@ Skills are invoked using slash commands in Claude Code: - **`/test-first`** - TDD with an approval gate, armed by default (shift it with `/spec:gear`) - **`/tidy-first`** - Safe refactoring workflow - **`/refactor:smells`** - Opt-in code smell review of completed work; schedules refactorings +- **`/refactor:tidy-comments`** - Brings comments into line with the comment rules; schedules refactorings a comment side-stepped - **`/bugfix:*`** - Diagnosis-first bug workflow (Triage → Confirm → Test-first → Fix → Verify) ### Documentation Skills @@ -241,6 +243,7 @@ These skills enforce practices documented in `.agent_instructions/`: | `/adr` | ADR creation standards | [documentation.md](../../.agent_instructions/documentation.md) lines 49-62 | | `/tidy-first` | Structural/behavioral separation | [code_style.md](../../.agent_instructions/code_style.md) lines 74-83 | | `/refactor:smells` | Fowler's code smells, calibrated to Brighter's design principles | [design_principles.md](../../.agent_instructions/design_principles.md) | +| `/refactor:tidy-comments` | Where comments may go, and Extract Method over comments | [documentation.md](../../.agent_instructions/documentation.md) → "Comments in Code" | The first three make **mandatory workflows enforceable** rather than just documented. `/refactor:smells` is opt-in. diff --git a/.claude/commands/bugfix/fix.md b/.claude/commands/bugfix/fix.md index 7bc7d01e5d..8a3ca6007c 100644 --- a/.claude/commands/bugfix/fix.md +++ b/.claude/commands/bugfix/fix.md @@ -45,6 +45,11 @@ Read the suspect code (Confirmed Root Cause + Suspected Location). "improvements", no default changes (CLAUDE.md → "Change Scope"). 2. Follow `.agent_instructions/code_style.md` (naming, RDD, nullable, XML docs on public members, MIT header on new files) and `.agent_instructions/documentation.md`. + - **Do not explain the fix in a comment.** The root cause, the previous behaviour and why this + fix was chosen belong in `bugfix.md`, the commit message and the PR — not in the code. No + comments inside method bodies, and no issue or bugfix numbers anywhere in the source + (`documentation.md` § *Comments in Code*). If the fixed code is hard to read without a + comment, extract a method whose name says what it guards against. 3. If the Scope Notes listed additional defects, fix those too — they share the confirmed cause and each has its own red test from `/bugfix:test`. 4. **Cross-backend parity**: if the cause touches one backend (MSSQL/PostgreSQL/MySQL/SQLite/ diff --git a/.claude/commands/refactor/README.md b/.claude/commands/refactor/README.md index a036744314..591ddc21e6 100644 --- a/.claude/commands/refactor/README.md +++ b/.claude/commands/refactor/README.md @@ -37,6 +37,29 @@ marked `structural` (a `/tidy-first` job) or `public API change` (needs a decisi /refactor:smells src/Paramore.Brighter/CommandProcessor.cs ``` +### `/refactor:tidy-comments [base-ref | base..head | path...]` + +Brings the comments in a unit of work into line with +[documentation.md § Comments in Code](../../../.agent_instructions/documentation.md#comments-in-code): +XML docs on public members only, at most a comment above the signature elsewhere, and none inside +method bodies. + +**Purpose**: Remove comments that narrate the code, record its history, cite issues or specs, or talk +to a reviewer; replace comments that explain a block or expression with Extract Method, Extract +Variable, a named constant or a rename. Comments that were written *instead of* a refactoring +("must be called first", "can be null when…") are reported and scheduled, as `/refactor:smells` does. + +**It changes code, structurally only**: a comment-only `refactor:` commit, then an extraction +`refactor:` commit, with tests green and no public signature changed. Reviewer rationale missing from +the ADR is shown to you rather than deleted. + +**Usage:** +```bash +/refactor:tidy-comments # comments this branch added or changed +/refactor:tidy-comments HEAD~3 # comments changed since a ref +/refactor:tidy-comments src/Paramore.Brighter/CommandProcessor.cs # every comment in a file with no changes +``` + ## The Core Principle From [.agent_instructions/code_style.md](../../../.agent_instructions/code_style.md): diff --git a/.claude/commands/refactor/smells.md b/.claude/commands/refactor/smells.md index fb07bbce47..f212b6f4af 100644 --- a/.claude/commands/refactor/smells.md +++ b/.claude/commands/refactor/smells.md @@ -109,7 +109,9 @@ cases. **Do not report** these as smells: - **Primitives used for serialisation or interoperability**, which the design principles allow. - **XML documentation comments** on public members — required by [documentation.md](../../../.agent_instructions/documentation.md). The Comments smell is about - comments inside method bodies that narrate the code. + comments inside method bodies that narrate the code. Where the change under review has many of + them, report one finding and point to `/refactor:tidy-comments`, which applies the full rules, + rather than listing every comment. - **Sync/async pairs** (`Send`/`SendAsync`, `IAmA*`/`IAmA*Async`) with parallel bodies — the duplication is deliberate across the sync and async pipelines. Report it only where shared logic could be extracted without crossing that line. diff --git a/.claude/commands/refactor/tidy-comments.md b/.claude/commands/refactor/tidy-comments.md new file mode 100644 index 0000000000..e7fa96877a --- /dev/null +++ b/.claude/commands/refactor/tidy-comments.md @@ -0,0 +1,169 @@ +--- +allowed-tools: Read, Edit, Glob, Grep, Bash, AskUserQuestion, TodoWrite +description: Bring comments into line with Brighter's comment rules, and surface refactorings a comment side-stepped +argument-hint: [base-ref | base..head | path...] +--- + +# Tidy Comments - Replace Comments With Intent + +You are bringing the comments in a unit of work into line with Brighter's rules, and finding the +places where a comment was written **instead of** a refactoring. + +This is a structural change. Nothing you do here may change behaviour. + +## Scope + +$ARGUMENTS + +## The Rules + +The source of truth is [documentation.md § Comments in Code](../../../.agent_instructions/documentation.md#comments-in-code). +Read it before you start. In short: + +| Where | What is allowed | +|-------|-----------------| +| Public and protected members of public types | `///` XML documentation. `<summary>`, `<param>`, `<returns>` etc. say how to use the API; `<remarks>` holds notes to the user. | +| Internal or private types and members | No XML documentation. At most a `//` or `/* */` comment immediately before the signature, when the method is complex or guards against a common trap. | +| Inside a method body | Nothing, as a rule — except `//Arrange` `//Act` `//Assert` in tests, the reason on an analyser suppression, and the rare one-line warning about a trap that cannot be extracted. | + +And for every comment: no narration of what the code does, no history of the change, no notes to a +reviewer, no issue, PR or spec references. Commented-out code is deleted. + +## Phase 1: Establish the Change Under Review + +Resolve the scope from the arguments: + +| Arguments | Scope | +|-----------|-------| +| none | The current branch against its merge-base with `master`, plus uncommitted changes: `git diff $(git merge-base HEAD master)` | +| a git ref (e.g. `HEAD~3`) | `git diff <ref>` | +| a range `<base>..<head>` | `git diff <base>..<head>` — work already committed or merged | +| one or more paths | The changes to those paths on the current branch; if they have no changes, **every** comment in those files | + +Then: + +1. List the changed `*.cs` files with `git diff --stat` and show them to the user. +2. **Exclude** generated tests (see `.agent_instructions/generated_tests.md`) — fix the template + instead, if the template is in scope — and `*.Designer.cs`, migrations, and non-code files. +3. With a diff scope, tidy only the comments **this diff added or changed**. Untouched comments are + out of scope; churning them hides the real change in review. Read each file in full anyway — you + need the surrounding method to decide whether a comment can become an extraction. +4. Check the build is green and the relevant tests pass. If the work is still moving or the tests + fail, say so and stop: the extractions below need a green baseline. + +If the scope is empty, say so and stop. + +## Phase 2: Classify Every Comment + +Find each comment in scope (`//`, `/* */`, `///`) and give it **one** action. Read the code each +comment sits beside before deciding: the action depends on what the code would need to say for +itself. + +| Action | When | What you do | +|--------|------|-------------| +| **Keep** | It conforms to the rules | Nothing | +| **Delete** | It narrates or restates the code; describes how the code used to be; is a note to a reviewer whose substance is already in the ADR or commit; or is commented-out code | Remove it | +| **Strip reference** | Its content is fine but it cites an issue, PR or spec (`#4207`, `Spec 0027`) | Remove the reference, keep the rest. An ADR reference that explains the design is fine. | +| **Trim** | It is right in kind but a paragraph where a line would do | Cut it to what a maintainer of the code as it now stands needs | +| **Move to signature** | A genuine "what is this for" or "beware this trap" comment inside the body of a method that already does one thing | Move it above the signature — `//` for a non-public member, into `<remarks>` for a public or protected one | +| **Convert XML** | `///` on an internal or private type or member | Replace with a `//` comment above the signature if it says something the name does not; otherwise delete | +| **Extract** | The comment labels a block, or explains an expression, a literal or a variable | Let the code say it. **Extract Method** for a block (the comment becomes the method name); **Extract Variable** or **Decompose Conditional** for a condition; a **named constant** for a magic literal; **Rename** for a variable or parameter. Keep a comment above the new method's signature only if the name cannot carry it | +| **Rationale to ADR** | Reviewer-facing rationale (why this approach, alternatives rejected) that is **not** already in an ADR | Do not silently delete it. List it for the user: add it to the relevant ADR, or drop it | +| **Side-stepped refactoring** | The comment papers over a design problem that a local extraction does not fix — see below | Leave the code alone. Report it and schedule it in Phase 5 | + +### Spotting a side-stepped refactoring + +These are the most valuable findings. A comment often marks the spot where someone noticed a problem +and wrote it down instead of fixing it: + +| The comment says… | The refactoring it avoided | +|-------------------|----------------------------| +| "must be called before / after X", "do not reorder" | Temporal coupling. Make the order impossible to get wrong: combine the steps, or return a type that only exposes the next valid step | +| "can be null when…", "only set if…" | The type does not tell the truth. Nullable annotation, Introduce Special Case, or Extract Class for a Temporary Field | +| "x is in seconds", "this string is a topic name" | Primitive Obsession. Replace Primitive with Object | +| "the caller must ensure…", "assumes…" | Introduce Assertion or a guard clause — **a behaviour change** if it adds a throw | +| "same as in Foo", "keep in sync with…" | Duplicated Code or Shotgun Surgery. Extract Function, Move Function | +| "step 1 / step 2 / step 3" across a long method | Long Function. Split Phase, Extract Function | +| "hack", "workaround", "TODO", "for now" | Unfinished work. Ask whether it is still true; if it is, it is a task, not a comment | +| "this is public so tests can reach it" | Insider Trading — Brighter's visibility rule (`testing.md`, `design_principles.md`) | + +Name the refactoring from Fowler's catalogue, as `/refactor:smells` does. If the refactoring would +change behaviour (a new guard that throws, a changed type a caller depends on) or change a `public` or +`protected` signature, say so: it belongs in `/test-first` or needs a decision, not a tidy. + +## Phase 3: Present the Plan + +Show the plan grouped by file, then a count by action: + +``` +## Comment tidy for <scope> + +### src/Paramore.Brighter/Foo.cs +- L42-46 **Delete** — narrates the history of the fix (substance is in ADR 0057) +- L71 **Strip reference** — "(Spec 0027, #2541)" +- L88-90 **Extract** — "validate the header" labels L91-104 → `private bool HasValidHeader(Message)` +- L130 **Side-stepped refactoring** — "must call Connect() first": temporal coupling + +Delete 6 · Strip 3 · Trim 2 · Move 1 · Convert 2 · Extract 3 · Keep 4 +Rationale to ADR: 1 · Side-stepped refactorings: 2 +``` + +For every **Keep** of a comment inside a method body, say which exception it falls under. For every +**Rationale to ADR**, quote the text and name the ADR you would add it to. + +Then ask the user, with `AskUserQuestion`, whether to apply the plan as shown, apply it with +changes they name, or stop. Also ask whether to commit as you go or leave the commits to them. + +## Phase 4: Apply + +Two passes, so a reviewer can read each one quickly: + +1. **Comment-only pass** — Delete, Strip reference, Trim, Move to signature, Convert XML. + - Build (`dotnet build` on the affected projects). A comment-only change cannot change behaviour, + but a broken `/* */` or a malformed `///` can break the build or add warnings. + - Commit, if the user chose to: `refactor: tidy comments in <area>` +2. **Extraction pass** — Extract actions only. + - Extracted members are `private` (or `private static`). Do not change any public or protected + signature. + - Run the tests that cover each changed file. They must stay green without being edited; if a test + has to change, the extraction changed behaviour — revert it and report it as a side-stepped + refactoring instead. + - Commit, if the user chose to: `refactor: extract methods to express intent in <area>` + +If the user approved adding rationale to an ADR, make that edit as its own `docs:` commit. + +Show the user the diff stat for each pass. + +## Phase 5: Schedule the Side-Stepped Refactorings + +Report each side-stepped refactoring in the `/refactor:smells` format — location, evidence (the +comment and the code it excuses), the Fowler refactoring, kind (`structural`, `behaviour change`, or +`public API change`) and size. Then offer, with `AskUserQuestion`: + +1. **Run with `/tidy-first`** — for structural ones. Give the exact command. Do not run it yourself. +2. **Run with `/test-first`** — for behaviour changes. +3. **Add to the spec's `tasks.md`** — when a spec is active. Follow `/refactor:smells` Phase 4 for + the task format; show the text before editing; do not commit. +4. **Record only**, or **Skip**. + +Removing the comment over a side-stepped refactoring is the refactoring's job, not this one's: leave +the comment until the refactoring makes it unnecessary. + +## Rules + +- **No behaviour changes.** If a tidy needs one, it is not a tidy. +- **No public or protected signature changes.** +- **Only the change under review** with a diff scope. Whole files only when the user names paths. +- **Do not lose information silently.** Rationale not in an ADR goes to the user, not the bin. +- **Do not add comments** beyond moving or condensing ones that were already there; and do not + write XML documentation for undocumented public members. If public members lack it, mention it in + the report as a documentation gap for the user to schedule. +- **Generated tests are fixed in their template**, never in the generated file. +- Leave the `#region Licence` block and `// <auto-generated>` markers alone. + +## Related + +- [documentation.md § Comments in Code](../../../.agent_instructions/documentation.md#comments-in-code) — the rules +- [`/refactor:smells`](smells.md) — the broader smell review; its **Comments** smell is this command's subject +- [`/tidy-first`](tidy-first.md) — carries out the side-stepped refactorings this command schedules +- [`/test-first`](../tdd/test-first.md) — for any that need a behaviour change diff --git a/.claude/commands/refactor/tidy-first.md b/.claude/commands/refactor/tidy-first.md index 3a8f3d5faa..14b3943a63 100644 --- a/.claude/commands/refactor/tidy-first.md +++ b/.claude/commands/refactor/tidy-first.md @@ -175,7 +175,7 @@ Options: 1. Make the behavioral changes you identified in Phase 1 2. Follow code style and documentation guidelines 3. Add proper error handling -4. Update or add XML documentation for new/changed public APIs +4. Update or add XML documentation for new/changed public APIs; no comments inside method bodies ([documentation.md § Comments in Code](../../../.agent_instructions/documentation.md#comments-in-code)) 5. Follow test-first if adding new behavior (consider using `/test-first` for this) **If adding significant new behavior:** diff --git a/.claude/commands/spec/implement.md b/.claude/commands/spec/implement.md index cbed6b90db..920ee79276 100644 --- a/.claude/commands/spec/implement.md +++ b/.claude/commands/spec/implement.md @@ -228,6 +228,7 @@ for the routine case, not a gag order. - Enable nullable reference types - Follow Responsibility Driven Design principles - Avoid primitive obsession + - Comments: XML docs on public members only; no comments inside method bodies — extract a method to express intent; no issue/spec references, history or notes to a reviewer ([documentation.md § Comments in Code](../../../.agent_instructions/documentation.md#comments-in-code)) 4. **Create/Update Implementation Files**: Use Write or Edit tool diff --git a/.claude/commands/spec/ralph-implement.md b/.claude/commands/spec/ralph-implement.md index bd9b3e50fb..2b062b4e95 100644 --- a/.claude/commands/spec/ralph-implement.md +++ b/.claude/commands/spec/ralph-implement.md @@ -263,6 +263,8 @@ from conversation. constants, expression-bodied members for simple members, `readonly` where appropriate, nullable reference types enabled, Responsibility-Driven Design, avoid primitive obsession. XML documentation on new public members; MIT licence header on new files. + No comments inside method bodies — extract a method to express intent; no issue/spec + references, history or notes to a reviewer (`documentation.md` § *Comments in Code*). - Run the verify command and confirm the test **PASSES**. - Run the **full suite** for the affected project(s) — `dotnet test {project}`, not just the filter — to catch regressions. If a regression appears, fix it before finishing. diff --git a/.claude/commands/spec/review.md b/.claude/commands/spec/review.md index 248e5e5036..09362bb29f 100644 --- a/.claude/commands/spec/review.md +++ b/.claude/commands/spec/review.md @@ -342,7 +342,8 @@ Every finding MUST cite concrete evidence: a file path, a line range, a diff hun - Primary constructors for new classes (project default) - `Async` suffix on async methods; `ValueTask` vs `Task` consistency with surrounding code - Nullability annotations consistent with the file's existing style -- XML doc comments on public APIs (project requires them) +- XML doc comments on public APIs (project requires them), and not on internal/private members +- Comments follow `.agent_instructions/documentation.md` § *Comments in Code*: no comments inside method bodies (beyond `//Arrange` `//Act` `//Assert` and suppression reasons), no issue/PR/spec references, no change history or reviewer notes. A violation is a finding; suggest `/refactor:tidy-comments` - No dead code, speculative abstractions, or commented-out blocks - No mixing of structural and behavioral changes in the same commit (tidy-first rule) diff --git a/.claude/commands/tdd/test-first.md b/.claude/commands/tdd/test-first.md index 4dc20be9c0..17356d8ecc 100644 --- a/.claude/commands/tdd/test-first.md +++ b/.claude/commands/tdd/test-first.md @@ -146,7 +146,8 @@ proceed once RED is proved. **Documentation Requirements** (from [.agent_instructions/documentation.md](../../../.agent_instructions/documentation.md)): 1. Add XML documentation comments (`///`) for all public members 2. Use `<summary>`, `<param>`, `<returns>`, `<exception>` tags -3. Add `<remarks>` for complex implementation details +3. Add `<remarks>` for notes to the user of a public API — caveats, side effects, implementation details that affect use +4. No comments inside method bodies (tests may use `//Arrange` `//Act` `//Assert`) — extract a method to express intent instead. No XML docs on non-public members; at most a `//` above the signature when the method is complex or guards against a trap. No issue/spec references, change history or notes to a reviewer. See [documentation.md § Comments in Code](../../../.agent_instructions/documentation.md#comments-in-code) **Steps:** 1. Write ONLY the code needed to make this specific test pass diff --git a/CLAUDE.md b/CLAUDE.md index 97e1ad159e..9d3012c08f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,6 +76,7 @@ Claude Code skills automate common workflows and enforce mandatory engineering p - `/test-first <behavior>` - TDD workflow with an approval gate before implementation, armed by default ([docs](.claude/commands/tdd/README.md)) - `/tidy-first <change>` - Separate structural (refactoring) from behavioral (feature) changes ([docs](.claude/commands/refactor/README.md)) - `/refactor:smells [base-ref | base..head | path...]` - Opt-in review of a completed, green unit of work for Fowler's code smells; reports and schedules refactorings (as `/tidy-first` runs or `STRUCTURAL` tasks) without changing code ([docs](.claude/commands/refactor/README.md)) +- `/refactor:tidy-comments [base-ref | base..head | path...]` - Bring comments in a unit of work into line with [Comments in Code](.agent_instructions/documentation.md#comments-in-code): delete narration, history and issue references; replace explanatory comments with Extract Method; report comments that side-stepped a real refactoring ([docs](.claude/commands/refactor/README.md)) - `/adr <title>` - Create Architecture Decision Records ([docs](.claude/commands/adr/README.md)) - `/bugfix:*` - Lightweight diagnosis-first bug workflow: Triage → Confirm (✋ gate) → Test-first → Fix → Verify. Use for a bug whose root cause is not yet proven, or that arrived with a suggested fix you should verify first ([docs](.claude/commands/bugfix/README.md)) @@ -90,6 +91,7 @@ Claude Code skills automate common workflows and enforce mandatory engineering p again (downshift) — never a hand-written waiver - Use `/tidy-first` when code needs refactoring before/during feature work - Use `/refactor:smells` when the user elects to review finished work for code smells — never run it on their behalf +- Use `/refactor:tidy-comments` when comments have crept into method bodies, or carry issue numbers, history or reviewer notes - Use `/adr` when documenting architectural decisions - Use `/spec:*` commands for full feature development from requirements to implementation diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 93efdb1d53..0c2585a271 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -316,12 +316,9 @@ recipe. public string Status { get; set; } ``` -- Use `<remarks>` for advice to developers or LLMs working with the code directly. Include information on how the method is implemented where it is not obvious from the code or significant design decisions have been made. Consider what you would want to know if maintaining this method. Use `<see href="">` if you need to link to external documentation. This can also be used for more detailed information than could be included in the `<summary>`. - - Prefer to use good variable and method names to express intent, over inline comments. - - Use the refactoring "Extract Method To Express Intent" to encapsulate code in a named method that explains intent, over using a comment. - - Do not add comments for what may be easily inferred from the code. - - In tests you may use //Arrange, //Act, //Assert. - - If code has a complex algorithm or non-obvious implementation, prefer to use `/// <remarks>` +- The elements divide into two kinds: + - **How to use the API** — `<summary>`, `<param>`, `<typeparam>`, `<returns>`, `<value>`, `<exception>`. What it is for and what to pass to get the behaviour you want. + - **Notes to the user** — `<remarks>`. Caveats, side effects, threading or ordering constraints, and how it is implemented where that affects how you use it, or where a significant design decision has been made. Use `<see href="">` if you need to link to external documentation. This can also be used for more detailed information than could be included in the `<summary>`. - Example: ```csharp @@ -336,6 +333,15 @@ public string Status { get; set; } - Documentation comments should be changed when APIs change. - Document new features and changes in the Docs repository of the BrighterCommand organization. +#### Comments in Code + +- `///` XML documentation goes on public and protected members of public types only. +- On internal or private types and members, no XML documentation. Where a method is complex to understand, or protects against a common trap, put a `//` or `/* */` comment immediately before the signature saying what it does or which trap it guards against. +- Inside a method body, no comments as a rule. If a block needs a comment to be understood, use "Extract Method To Express Intent" and, if the name is not enough, comment above the new method's signature. Use Extract Variable, a named constant or a rename for the smaller cases. + - The exceptions are `//Arrange`, `//Act`, `//Assert` in tests, the reason on an analyser suppression, and the rare one-line warning about a trap that cannot be extracted. +- No comment narrates what the code does, describes how it used to be, or is a note to a reviewer. No issue, PR or spec references. That context belongs in the ADR, the commit message, or the pull request. +- Delete commented-out code. + ### Licensing - We add a license comment to every src file From 669a339cca2344e18c967319cb94929fdcc21442 Mon Sep 17 00:00:00 2001 From: iancooper <ian_hammond_cooper@yahoo.co.uk> Date: Sun, 11 Oct 2026 19:48:02 +0100 Subject: [PATCH 2/5] docs: record provisioning order and shared-connection rationale in ADR 0058 Moves the reasoning from inline comments in BoxProvisioningHostedService and SqlBoxProvisioner into the ADR, ahead of tidying those comments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7 --- docs/adr/0058-box-provisioning-rdd-role-interfaces.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/adr/0058-box-provisioning-rdd-role-interfaces.md b/docs/adr/0058-box-provisioning-rdd-role-interfaces.md index c0ed12b152..d2b5dfcd21 100644 --- a/docs/adr/0058-box-provisioning-rdd-role-interfaces.md +++ b/docs/adr/0058-box-provisioning-rdd-role-interfaces.md @@ -701,6 +701,14 @@ The deviation from `*Base` suffix is documented here to satisfy parent C4 / AC9 - **TFM matrix unchanged** (NF11 / parent C6 / C7). The base uses plain generic class declaration; both type constraints (`where TConnection : DbConnection` and `where TTransaction : DbTransaction`) are pre-net5 compatible. No static virtual / static abstract / `IReadOnlySet<T>` introduced. - **Public surface delta**: +1 type (`SqlBoxProvisioner<TConnection, TTransaction>` — abstract) in the shared `Paramore.Brighter.BoxProvisioning` assembly. Per parent AC8 / NF6 this is a runtime production type, not testability-motivated — derived classes are the production surface; the base is the algorithm host. +#### One connection for pre-lock detection and payload validation + +`SqlBoxProvisioner.ProvisionAsync` runs pre-lock detection and payload-mode validation sequentially against the same open connection rather than opening one for each. That saves a connection round-trip on every provisioning run, and avoids connection-pool churn in deployments with strict per-pod connection caps. + +#### Provisioning order + +`BoxProvisioningHostedService` provisions every outbox before any inbox; `OrderingOrdinal` is the single place that order is defined, and it throws for an unknown `BoxType` so a new box type fails loudly at startup rather than being silently mis-ordered. Within the outbox or inbox group, the order across several provisioners (for example one per tenant) is deliberately left to the order in which the DI container resolves them. Microsoft.Extensions.DependencyInjection preserves registration order, but third-party containers need not. We do not add a deterministic secondary sort by `BoxTableName`: the per-table startup log already says which provisioner is running, so a non-deterministic order within a group is tolerable, and a secondary sort would entrench an ordering that callers cannot influence. + --- ## Adding a new BoxProvisioning backend From 0dcac0a6e062e54ac48aa033b78fd43c167974b6 Mon Sep 17 00:00:00 2001 From: iancooper <ian_hammond_cooper@yahoo.co.uk> Date: Sun, 11 Oct 2026 19:49:37 +0100 Subject: [PATCH 3/5] refactor: tidy comments in BoxProvisioning Delete comments that narrate code or record change history, strip spec, issue and PR references, convert XML docs on a private method to a // comment, and move the span-tag and pre-lock-hint rationale above the signatures of the methods they explain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7 --- .../BoxProvisioningHostedService.cs | 17 +------ .../BoxProvisioningOptions.cs | 8 ++-- ...righterBuilderBoxProvisioningExtensions.cs | 4 +- .../IAmABoxMigrationDetectionHelper.cs | 6 +-- .../IAmABoxMigrationRunner.cs | 5 +- .../IAmAProvisioningUnitOfWork.cs | 3 +- .../Identifiers.cs | 3 -- .../SqlBoxMigrationRunner.cs | 46 ++++--------------- .../SqlBoxProvisioner.cs | 34 +++----------- 9 files changed, 27 insertions(+), 99 deletions(-) diff --git a/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningHostedService.cs b/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningHostedService.cs index 3cfcc98aff..2c8a44fe1c 100644 --- a/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningHostedService.cs +++ b/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningHostedService.cs @@ -70,15 +70,6 @@ public BoxProvisioningHostedService( /// The original failure is preserved as the inner exception.</exception> public async Task StartAsync(CancellationToken cancellationToken) { - // OrderBy is stable, but the upstream IEnumerable<IAmABoxProvisioner> resolution order - // is not guaranteed across DI containers — Microsoft.Extensions.DependencyInjection - // preserves registration order today, but third-party containers (Autofac with - // resolve-by-tag, Lamar, etc.) may not. Within an outbox/inbox group, ordering across - // multiple provisioners (e.g. one tenant per provisioner) is intentionally - // implementation-defined: the per-table startup log ("Provisioning Outbox '{BoxTableName}'…") - // disambiguates which provisioner is running, so a non-deterministic intra-group order - // is operationally tolerable and a deterministic secondary sort by BoxTableName would - // entrench an ordering callers cannot influence today. var ordered = _provisioners.OrderBy(p => OrderingOrdinal(p.BoxType)); foreach (var provisioner in ordered) @@ -91,9 +82,6 @@ public async Task StartAsync(CancellationToken cancellationToken) } catch (OperationCanceledException) { - // Propagate cancellation as-is. Wrapping it in ConfigurationException would - // misreport host-shutdown / startup-deadline timeouts as configuration errors — - // particularly confusing for k8s readiness-probe diagnostics. throw; } catch (Exception ex) @@ -119,10 +107,7 @@ public async Task StartAsync(CancellationToken cancellationToken) /// <returns>A <see cref="Task"/> that has already run to completion.</returns> public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; - // Single source of truth for provisioner ordering. The default arm converts a silent - // mis-ordering (intermittent, DI-registration-order-dependent) into a loud startup - // failure that names the file and method to update — so the next contributor adding - // a BoxType value (Lockbox / DeadLetterBox / etc.) sees exactly where to add the arm. + // Throws for an unknown BoxType, so a new box type fails loudly at startup instead of being silently mis-ordered. private static int OrderingOrdinal(BoxType type) => type switch { BoxType.Outbox => 0, diff --git a/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningOptions.cs b/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningOptions.cs index 7a3127acd1..c235b6d80b 100644 --- a/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningOptions.cs +++ b/src/Paramore.Brighter.BoxProvisioning/BoxProvisioningOptions.cs @@ -55,8 +55,7 @@ public class BoxProvisioningOptions /// sequentially (one outbox/inbox table at a time per registered backend). For a deployment /// with T provisioned tables on a single replica, the cumulative wall-clock window in the /// contended worst case is <c>MigrationLockTimeout × T</c>. Multi-tenant deployments with - /// many tables should size their readiness-probe tolerance accordingly. Parallelising the - /// per-phase loop is tracked at <see href="https://github.com/BrighterCommand/Brighter/issues/4140"/>. + /// many tables should size their readiness-probe tolerance accordingly. /// </para> /// <para> /// MySQL/SQLite-specific caveat: MySQL's <c>GET_LOCK</c> takes a timeout argument as an @@ -67,15 +66,14 @@ public class BoxProvisioningOptions /// on contention even if a shorter timeout is configured, and <see cref="TimeSpan.Zero"/> /// is NOT fail-fast. This is necessary divergence from MSSQL (<c>sp_getapplock</c> accepts /// milliseconds and supports fail-fast 0-ms) and PostgreSQL (<c>pg_try_advisory_lock</c> - /// is non-blocking and the runner loops with a monotonic deadline). Per PR #4039 reviewer - /// items M2-6 and F2-6. + /// is non-blocking and the runner loops with a monotonic deadline). /// </para> /// </remarks> public TimeSpan MigrationLockTimeout { get; set; } = TimeSpan.FromSeconds(30); /// <summary> /// Controls where the box migration-history table is physically placed. Default: - /// <see cref="MigrationHistoryScope.Global"/> — identical to behaviour prior to this feature. + /// <see cref="MigrationHistoryScope.Global"/>. /// </summary> /// <remarks> /// <see cref="MigrationHistoryScope.PerSchema"/> places history in the configured diff --git a/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs b/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs index 68c8047fb7..1eca93d290 100644 --- a/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs +++ b/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs @@ -93,8 +93,6 @@ public static IBrighterBuilder UseBoxProvisioning( return builder; } - // Marker singleton used to detect a second UseBoxProvisioning call on the same builder. - // Internal because callers should never resolve or depend on it directly — its only role - // is to flip the "already invoked" check on subsequent calls. + // Marks a builder on which UseBoxProvisioning has already run. internal sealed class BoxProvisioningMarker { } } diff --git a/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationDetectionHelper.cs b/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationDetectionHelper.cs index ba2ad0591a..4534e9bb28 100644 --- a/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationDetectionHelper.cs +++ b/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationDetectionHelper.cs @@ -94,8 +94,7 @@ Task<bool> DoesTableExistAsync( /// rows by their <c>SchemaName</c> column. Null is substituted with the backend default by /// each implementation — see <see cref="DoesTableExistAsync"/> for the substitution rule.</param> /// <param name="historySchema">Optional. The physical schema that holds the history table. - /// <c>null</c> means "the backend default schema" — i.e. the behaviour prior to per-schema - /// history placement. Distinct from <paramref name="schemaName"/>: under + /// <c>null</c> means "the backend default schema". Distinct from <paramref name="schemaName"/>: under /// <see cref="MigrationHistoryScope.Global"/> the two may differ (box in <c>billing</c>, /// history in <c>dbo</c>); under <see cref="MigrationHistoryScope.PerSchema"/> they coincide. /// Ignored by backends without a distinct schema concept (MySQL/SQLite/Spanner).</param> @@ -116,8 +115,7 @@ Task<bool> DoesHistoryExistAsync( /// by their <c>SchemaName</c> column. Null is substituted with the backend default by each /// implementation — see <see cref="DoesTableExistAsync"/> for the substitution rule.</param> /// <param name="historySchema">Optional. The physical schema that holds the history table. - /// <c>null</c> means "the backend default schema" — i.e. the behaviour prior to per-schema - /// history placement. See <see cref="DoesHistoryExistAsync"/> for the distinction from + /// <c>null</c> means "the backend default schema". See <see cref="DoesHistoryExistAsync"/> for the distinction from /// <paramref name="schemaName"/>.</param> /// <param name="cancellationToken">Optional cancellation.</param> /// <param name="transaction">Optional. See <see cref="DoesTableExistAsync"/>.</param> diff --git a/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationRunner.cs b/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationRunner.cs index 022edcab7d..6fe4973db7 100644 --- a/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationRunner.cs +++ b/src/Paramore.Brighter.BoxProvisioning/IAmABoxMigrationRunner.cs @@ -31,9 +31,8 @@ namespace Paramore.Brighter.BoxProvisioning; /// versions have been applied. /// </summary> /// <remarks> -/// Spec 0027 R1 (PR #4039 part 2): the runner retrieves its migration chain and -/// fresh-install DDL from an injected <see cref="IAmABoxMigrationCatalog"/>, so the -/// caller no longer threads the migration list through every invocation. The Spanner +/// The runner retrieves its migration chain and fresh-install DDL from an injected +/// <see cref="IAmABoxMigrationCatalog"/>, so callers do not pass the migration list. The Spanner /// runner is exempt from <see cref="IAmABoxMigrationCatalog"/> per ADR 0057 §6 and /// implements this interface directly with its own per-backend fresh-install logic. /// </remarks> diff --git a/src/Paramore.Brighter.BoxProvisioning/IAmAProvisioningUnitOfWork.cs b/src/Paramore.Brighter.BoxProvisioning/IAmAProvisioningUnitOfWork.cs index 2985ecf121..e4f8a857d9 100644 --- a/src/Paramore.Brighter.BoxProvisioning/IAmAProvisioningUnitOfWork.cs +++ b/src/Paramore.Brighter.BoxProvisioning/IAmAProvisioningUnitOfWork.cs @@ -113,8 +113,7 @@ public interface IAmAProvisioningUnitOfWork<TTransaction> : IAsyncDisposable /// <c>RollbackAsync</c> MUST NOT throw — disposal-style semantics. If the /// underlying transaction-rollback or lock-release fails internally, the /// implementation logs a Warning (with backend-specific diagnostic — e.g. - /// MySQL preserves its <c>RELEASE_LOCK</c> tri-state per spec 0027 Item M / - /// ADR 0057 §5b) and returns. The runner passes + /// MySQL preserves its <c>RELEASE_LOCK</c> tri-state per ADR 0057 §5b) and returns. The runner passes /// <see cref="CancellationToken.None"/> on rollback (NOT the caller's token) /// so a signalled token does not abandon the unwind. See ADR 0058 §B.3. /// </remarks> diff --git a/src/Paramore.Brighter.BoxProvisioning/Identifiers.cs b/src/Paramore.Brighter.BoxProvisioning/Identifiers.cs index 2d21856719..6e042d1f53 100644 --- a/src/Paramore.Brighter.BoxProvisioning/Identifiers.cs +++ b/src/Paramore.Brighter.BoxProvisioning/Identifiers.cs @@ -59,9 +59,6 @@ public static class Identifiers /// with an underscore.</exception> public static void AssertSafe(string identifier, string parameterName) { - // Split missing-configuration (null) from malformed-identifier so operators see the real - // root cause. Pointing at the regex when no identifier was supplied at all sends them - // looking for an "invalid character" that does not exist. if (identifier is null) { throw new ConfigurationException( diff --git a/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs b/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs index 0f69d84cae..33df98923d 100644 --- a/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs +++ b/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs @@ -119,8 +119,7 @@ public abstract class SqlBoxMigrationRunner<TConnection, TTransaction> /// <see cref="MigrateAsync"/> emits a migration span on the tracer's /// <see cref="System.Diagnostics.ActivitySource"/>. Defaults to null (no instrumentation).</param> /// <param name="scope">Controls where the migration-history table is physically placed. - /// Defaults to <see cref="MigrationHistoryScope.Global"/> (today's behaviour). Threaded on - /// the same call path as <paramref name="lockTimeout"/>.</param> + /// Defaults to <see cref="MigrationHistoryScope.Global"/>.</param> protected SqlBoxMigrationRunner( IAmAVersionDetectingMigrationHelper<TConnection, TTransaction> detectionHelper, IAmABoxMigrationCatalog catalog, @@ -149,8 +148,7 @@ protected SqlBoxMigrationRunner( /// The backend default schema in which history lives under /// <see cref="MigrationHistoryScope.Global"/>: MSSQL <c>"dbo"</c>, PostgreSQL <c>"public"</c>, /// or <c>null</c> for backends with no distinct schema concept (MySQL — history lives in the - /// connection-bound database; SQLite). This is the value the runner used unconditionally - /// prior to this feature. + /// connection-bound database; SQLite). /// </summary> protected abstract string? DefaultHistorySchema { get; } @@ -166,8 +164,7 @@ protected SqlBoxMigrationRunner( /// Resolves the physical schema that holds the migration-history table for this run. Under /// <see cref="MigrationHistoryScope.PerSchema"/> on a placement backend it is the configured /// <see cref="IAmARelationalDatabaseConfiguration.SchemaName"/> (guaranteed non-null by the - /// misconfiguration guard); otherwise it is <see cref="DefaultHistorySchema"/> — i.e. today's - /// behaviour. This is the single source of truth handed to both the write side (CREATE/INSERT) + /// misconfiguration guard); otherwise it is <see cref="DefaultHistorySchema"/>. This is the single source of truth handed to both the write side (CREATE/INSERT) /// and the read side (detection helper), so they cannot diverge. /// </summary> protected string? ResolveHistorySchema() => @@ -232,9 +229,6 @@ public async Task MigrateAsync( ValidateMigrationsMonotonic(schemaName?.Value, tableName.Value, migrations); - // Span name follows OTel DB convention "{operation} {target}" — the table name is the - // operator-meaningful target. Tags carry backend/schema/box-type so a multi-table - // startup trace can be filtered without re-parsing the display name. using var activity = StartMigrationActivity(tableName.Value, schemaName?.Value, boxType); // Spec 0029 NF5/AC7 (ADR 0060 D6): surface the placement decision per run so an operator @@ -247,13 +241,6 @@ public async Task MigrateAsync( "Box migration history for {BoxTable} resolved to schema {HistorySchema} (scope {Scope})", tableName, ResolveHistorySchema() ?? "<backend default>", _scope); - // Bootstrap resources are held in locals so the outer try/finally can guarantee - // disposal whether the body succeeds, the body throws, or the bootstrap itself - // fails before any disposer would have run. The inner bootstrap try/catch records - // bootstrap failures (connection refused, lock timeout from BeginAsync per - // ADR 0058 §B.3) onto the migration activity — without this narrow handler those - // failures escape with ActivityStatusCode.Unset because the success-path catch - // only wraps the body. // Sync Dispose for the connection: DbConnection does not implement IAsyncDisposable // on netstandard2.0, so DisposeAsync is unavailable across the shared-assembly TFM // matrix. The UoW implements IAsyncDisposable (see IAmAProvisioningUnitOfWork) and @@ -288,11 +275,6 @@ public async Task MigrateAsync( { activity?.SetTag(BrighterSemanticConventions.BoxMigrationPath, "fresh"); activity?.AddEvent(new ActivityEvent(BrighterSemanticConventions.BoxMigrationEventFreshInstall)); - // The fresh-install fast path no longer reads migrations[0].UpScript — V1 in - // the chain now carries its honest historical baseline DDL (spec 0027 R1). - // The live builder DDL comes from the catalog's FreshInstallDdl hook so the - // post-install column set always matches V_latest regardless of how the - // historical V1 looked. var freshInstallDdl = _catalog.FreshInstallDdl(_configuration); var latestVersion = migrations.Count == 0 ? (MigrationVersion)0 : migrations[migrations.Count - 1].Version; await RunFreshPathAsync( @@ -359,10 +341,10 @@ await RunNormalPathAsync( } } + // The span name follows the OTel DB convention "{operation} {target}". With no schema, + // db.namespace is left off rather than set to null, so "namespace is missing" filters work. private Activity? StartMigrationActivity(string tableName, string? schemaName, BoxType boxType) { - // ActivitySource.StartActivity returns null when no listener is registered or sampling - // discards — caller handles null gracefully via the `?.` chain on every Set/AddEvent. var activity = Tracer?.ActivitySource.StartActivity( $"{BrighterSemanticConventions.BoxMigration} {tableName}", ActivityKind.Internal); @@ -371,8 +353,6 @@ await RunNormalPathAsync( activity.SetTag(BrighterSemanticConventions.DbTable, tableName); if (schemaName is not null) { - // SQLite has no schema concept — leaving the tag out (vs. setting it to "null") - // keeps a "filter where db.namespace is missing" query meaningful. activity.SetTag(BrighterSemanticConventions.DbNamespace, schemaName); } activity.SetTag(BrighterSemanticConventions.BoxType, boxType.ToString()); @@ -423,9 +403,9 @@ protected abstract Task EnsureHistoryTableAsync( /// Applies the fresh-install path: executes <paramref name="freshInstallDdl"/> (the live /// V_latest-shape DDL sourced from <see cref="IAmABoxMigrationCatalog.FreshInstallDdl"/>) /// and stamps history at <paramref name="latestVersion"/>. Invoked when re-detection inside - /// the UoW reports no table. The fresh path no longer reads the migration chain — the - /// historical V1 in <see cref="IAmABoxMigrationCatalog.All"/> may carry a pre-V_latest - /// baseline that does not match the current builder (spec 0027 R1). + /// the UoW reports no table. The fresh path does not read the migration chain: the + /// historical V1 in <see cref="IAmABoxMigrationCatalog.All"/> may carry an older baseline + /// that does not match the current builder. /// </summary> protected abstract Task RunFreshPathAsync( TConnection connection, TTransaction? transaction, string? schemaName, string tableName, @@ -468,14 +448,8 @@ protected abstract Task RunNormalPathAsync( return (tableExists, historyExists); } - /// <summary> - /// Rejects duplicate, gap, or out-of-order migration versions before any work begins. - /// A malformed list corrupts every path branch (PK violation on history insert, skipped - /// ALTERs, double-applied DDL), so the validation sits at <see cref="MigrateAsync"/> entry. - /// Lifted from spec 0027 Items H/I/Q. Private because the orchestration is owned by the - /// base; derived classes implement only the per-backend hooks and have no reason to - /// re-invoke (or override the algorithm of) the pre-flight monotonicity check. - /// </summary> + // Rejects duplicate, gap or out-of-order versions before any work begins: a malformed chain + // corrupts every path (PK violation on history insert, skipped ALTERs, double-applied DDL). private static void ValidateMigrationsMonotonic( string? schemaName, string tableName, IReadOnlyList<IAmABoxMigration> migrations) { diff --git a/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs b/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs index 7783d2fedd..221202c09e 100644 --- a/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs +++ b/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs @@ -53,7 +53,7 @@ public abstract class SqlBoxProvisioner<TConnection, TTransaction> where TTransaction : DbTransaction { // Static logger keeps the ctor surface unchanged across the 8 concrete provisioners; only - // exercised by the pre-lock-hint failure swallow below (Spec 0029 T-PERM). + // exercised by the pre-lock-hint failure swallow below. private static readonly ILogger s_logger = ApplicationLogging.CreateLogger<SqlBoxProvisioner<TConnection, TTransaction>>(); @@ -111,13 +111,7 @@ public async Task ProvisionAsync(CancellationToken cancellationToken = default) var migrations = _catalog.All(_configuration); - // Sync `using` for the connection: DbConnection does not implement IAsyncDisposable - // on netstandard2.0, so `await using` would not compile across the shared-assembly - // TFM matrix. Mirrors the §B.2 precedent at SqlBoxMigrationRunner.cs:112-116. - // - // Detection and validation run sequentially against the same open - // connection, so we cut one connection round-trip per provisioning run (and avoid the - // connection-pool churn in deployments with strict per-pod connection caps). + // Sync `using`: DbConnection has no DisposeAsync on netstandard2.0. BoxTableState tableState; using (var connection = CreateConnection(_configuration.ConnectionString)) { @@ -133,10 +127,6 @@ await _payloadValidator.ValidateAsync( } } - // The runner sources its migration chain from its own injected - // IAmABoxMigrationCatalog (see SqlBoxMigrationRunner ctor) — the provisioner no longer - // forwards the list. The provisioner still calls _catalog.All(...) above so the pre-lock - // detection helper can infer current version from the column set. await _migrationRunner.MigrateAsync( BoxTableName, _configuration.SchemaName != null ? (SchemaName)_configuration.SchemaName : null, @@ -145,6 +135,11 @@ await _migrationRunner.MigrateAsync( cancellationToken); } + // Pre-lock detection is only a hint: the runner re-detects authoritatively under the lock and + // resolves the history schema there (ADR 0060). So history reads use the backend-default + // schema, a DbException from them (commonly tenant credentials lacking SELECT on that schema) + // is logged at Debug and treated as "no history", and a negative detected version is clamped + // to 0 rather than rejected. private async Task<BoxTableState> DetectTableStateAsync( TConnection connection, IReadOnlyList<IAmABoxMigration> migrations, @@ -155,16 +150,6 @@ private async Task<BoxTableState> DetectTableStateAsync( if (!tableExists) return new BoxTableState(TableExists: false, HistoryExists: false, CurrentVersion: 0); - // Pre-lock read is an explicitly discarded hint (the runner re-detects authoritatively - // under the lock and resolves the history schema there), so this site passes - // historySchema: null = backend default — see ADR 0060 D4. A provider exception on the - // hint is most commonly tenant-isolated credentials lacking SELECT on the backend-default - // history schema (Spec 0029 FR5 / T-PERM). Swallow it: the runner's under-lock detection - // resolves the per-schema history under PerSchema scope and its D5 seed converts any - // legacy-read failure into a ConfigurationException with the documented operator-facing - // message. Logged at Debug so the swallow is observable in diagnostics. DbException is - // the common base for SqlException / NpgsqlException / MySqlException / SqliteException - // and the only failure family the hint genuinely cannot reason about. bool historyExists; try { @@ -181,9 +166,6 @@ private async Task<BoxTableState> DetectTableStateAsync( if (!historyExists) { - // Pre-lock detection is a hint for the caller; the runner re-detects under the lock. - // Negative or zero return values are not gated here — the runner is the single source - // of truth for discriminator violations and unknown-schema rejections. var detectedVersion = await _detectionHelper.DetectCurrentVersionAsync( connection, BoxTableName.Value, EffectiveSchemaName, BoxType, migrations, cancellationToken); @@ -192,8 +174,6 @@ private async Task<BoxTableState> DetectTableStateAsync( CurrentVersion: detectedVersion < 0 ? 0 : detectedVersion); } - // Same hint-swallow rationale as historyExists above: a provider failure here is a hint - // signal only, the runner re-detects authoritatively. int maxVersion; try { From 5253c5133725ac80623435032c731cffee01d387 Mon Sep 17 00:00:00 2001 From: iancooper <ian_hammond_cooper@yahoo.co.uk> Date: Sun, 11 Oct 2026 19:52:01 +0100 Subject: [PATCH 4/5] refactor: extract methods to express intent in BoxProvisioning Replace explanatory comments in SqlBoxMigrationRunner.MigrateAsync, SqlBoxProvisioner.ProvisionAsync and UseBoxProvisioning with private methods whose names carry the intent (GuardIdentifiers, GetMigrations, RollbackWithoutMaskingAsync, ReleaseAsync, ReadHistoryExistsHintAsync, RegisterProvisionersThenMarker, ...), keeping a short comment above a signature only where the name cannot carry a trap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7 --- ...righterBuilderBoxProvisioningExtensions.cs | 26 +-- .../SqlBoxMigrationRunner.cs | 169 +++++++++--------- .../SqlBoxProvisioner.cs | 100 ++++++----- 3 files changed, 151 insertions(+), 144 deletions(-) diff --git a/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs b/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs index 1eca93d290..66aed55ae8 100644 --- a/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs +++ b/src/Paramore.Brighter.BoxProvisioning/BrighterBuilderBoxProvisioningExtensions.cs @@ -71,26 +71,26 @@ public static IBrighterBuilder UseBoxProvisioning( + "hosted service would run each migration twice."); } - // Run configure(options) AND apply the queued registrations BEFORE registering the - // marker. Any throw — from configure itself or from a per-backend registration callback - // inside the foreach (e.g. an NRE in an Add{Backend}{Box} extension) — must NOT leak - // the marker, so the operator's natural retry surfaces the actual underlying error - // rather than the misleading "UseBoxProvisioning has already been called" diagnostic - // from the duplicate-call guard above. (Partial registrations from a half-applied - // foreach can still leak into builder.Services on failure; the contract here is - // narrower — marker isolation, not full transactional rollback.) + RegisterProvisionersThenMarker(builder.Services, configure); + + builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IHostedService, BoxProvisioningHostedService>()); + return builder; + } + + // The marker is added only once configuration and registration have succeeded, so a retry + // after a failure reports the real error, not "already called". Registrations made before + // the failure are not rolled back. + private static void RegisterProvisionersThenMarker(IServiceCollection services, Action<BoxProvisioningOptions> configure) + { var options = new BoxProvisioningOptions(); configure(options); foreach (var registration in options.Registrations) { - registration(builder.Services); + registration(services); } - builder.Services.AddSingleton<BoxProvisioningMarker>(); - - builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IHostedService, BoxProvisioningHostedService>()); - return builder; + services.AddSingleton<BoxProvisioningMarker>(); } // Marks a builder on which UseBoxProvisioning has already run. diff --git a/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs b/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs index 33df98923d..491109e0ee 100644 --- a/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs +++ b/src/Paramore.Brighter.BoxProvisioning/SqlBoxMigrationRunner.cs @@ -180,71 +180,16 @@ public async Task MigrateAsync( BoxTableState tableState, CancellationToken cancellationToken = default) { - // Defence-in-depth at the framework chokepoint. Catalogs gate AssertSafe at the entry to - // All(...), but the runner is independently reachable (callers can construct a migration - // list and invoke MigrateAsync directly), so the public entry point must validate too. - Identifiers.AssertSafe(tableName.Value, nameof(tableName)); - - // schemaName is optional: SQLite has no schema concept per ADR 0057 §6, so an absent value - // is legitimate and must not be rejected as "missing". A SchemaName is "absent" when it is - // a null reference OR wraps a null/empty string — callers commonly reach the latter via the - // implicit string->SchemaName conversion of a null/empty value (e.g. an unconfigured - // RelationalDatabaseConfiguration.SchemaName). Collapse every absent form to a single null - // using SchemaName.IsNullOrEmpty (the canonical "absent" check, mirroring Id.IsNullOrEmpty) - // so the AssertSafe gate and every downstream `schemaName?.Value ?? <backend default>` treat - // them identically — preserving the semantics of the `string?` parameter this value type - // replaced (spec 0030), which a bare `is not null` reference check silently broke. - if (SchemaName.IsNullOrEmpty(schemaName)) - { - schemaName = null; - } - else - { - Identifiers.AssertSafe(schemaName.Value, nameof(schemaName)); - } - - // D3 misconfiguration guard (FR1a/AC1a): on a placement backend, PerSchema needs a schema - // to place history in. A null SchemaName is an operator misconfiguration — reject it at the - // entry point rather than silently falling back to Global. Backends that do not support - // PerSchema (MySQL/SQLite) never trip this; for them PerSchema is a no-op. - if (Scope == MigrationHistoryScope.PerSchema && SupportsPerSchemaHistory && Configuration.SchemaName is null) - { - throw new ConfigurationException( - "MigrationHistoryScope.PerSchema requires a non-null SchemaName; there is no schema to place history in."); - } - - // The runner sources its migration chain and fresh-install DDL - // from the injected catalog rather than via parameter. A catalog returning null for - // either would surface inside ValidateMigrationsMonotonic / ExecuteFreshInstallAsync as - // an opaque NRE; replace with a descriptive operator-facing diagnostic. Empty migration - // lists are intentionally permitted: relational catalogs always return ≥V1, and - // internal tests use Array.Empty<IAmABoxMigration>() as a "don't care" payload when - // exercising hook-ordering, re-detection, or failure-path contracts. - var migrations = _catalog.All(_configuration); - if (migrations is null) - { - throw new ConfigurationException( - $"Migration list for '{(schemaName is null ? tableName : $"{schemaName}.{tableName}")}' was null. The injected IAmABoxMigrationCatalog must return a non-null list from All(...)."); - } + schemaName = GuardIdentifiers(tableName, schemaName); + ThrowIfPerSchemaHasNoSchema(); + var migrations = GetMigrations(tableName, schemaName); ValidateMigrationsMonotonic(schemaName?.Value, tableName.Value, migrations); using var activity = StartMigrationActivity(tableName.Value, schemaName?.Value, boxType); - // Spec 0029 NF5/AC7 (ADR 0060 D6): surface the placement decision per run so an operator - // can answer "where did THIS tenant's migration history go?" without correlating to the - // provisioner config. The single Information log carries the table, the resolved physical - // history schema (or the placeholder "<backend default>" on backends with no distinct - // history schema concept, e.g. SQLite / MySQL), and the active MigrationHistoryScope. - // Fires once per run regardless of which path (fresh / bootstrap / normal) dispatches. - Logger.LogInformation( - "Box migration history for {BoxTable} resolved to schema {HistorySchema} (scope {Scope})", - tableName, ResolveHistorySchema() ?? "<backend default>", _scope); + LogHistoryPlacement(tableName); - // Sync Dispose for the connection: DbConnection does not implement IAsyncDisposable - // on netstandard2.0, so DisposeAsync is unavailable across the shared-assembly TFM - // matrix. The UoW implements IAsyncDisposable (see IAmAProvisioningUnitOfWork) and - // is awaited explicitly in the finally. TConnection? connection = null; IAmAProvisioningUnitOfWork<TTransaction>? uow = null; try @@ -303,42 +248,88 @@ await RunNormalPathAsync( { activity?.SetStatus(ActivityStatusCode.Error, ex.Message); activity?.AddException(ex); - // Pass CancellationToken.None: a signalled caller token must not abandon the unwind. - // See ADR 0058 §B.3 cancellation contract. - try - { - await uow.RollbackAsync(CancellationToken.None); - } - catch (Exception rollbackEx) - { - // Defense in depth: the IAmAProvisioningUnitOfWork contract says RollbackAsync - // MUST NOT throw, and the four per-backend impls comply (PG and MySQL tightened - // explicitly in 3c8417fd6). If a future regression breaks that, do NOT let the - // rollback exception mask the original triggering exception — the primary cause - // is more diagnostically valuable than a defect in our own unwind, and callers - // pattern-match on the type they actually threw. Log the rollback failure at - // Error level so operators see both: the primary failure surfaces via rethrow, - // the rollback defect surfaces via the log. - Logger.LogError(rollbackEx, - "Box migration unit-of-work RollbackAsync threw while unwinding from a primary failure for table '{Table}'; rollback exception is logged, primary exception is rethrown.", - tableName); - } + await RollbackWithoutMaskingAsync(uow, tableName); throw; } } finally { - // Disposal order mirrors the previous `await using var uow` / `using var connection` - // pair: UoW first (releases any per-backend advisory lock + transaction state), - // then the connection. Null checks defend the bootstrap-failure-before-assignment - // case: if OpenConnectionAsync threw, both locals stay null; if BeginAsync threw, - // both are assigned and need disposal. - if (uow is not null) - { - await uow.DisposeAsync(); - } - connection?.Dispose(); + await ReleaseAsync(uow, connection); + } + } + + // The runner is callable without going through a catalog, so it asserts identifier safety + // itself. A SchemaName that wraps a null or empty string is absent, the same as a null one. + private static SchemaName? GuardIdentifiers(BoxTableName tableName, SchemaName? schemaName) + { + Identifiers.AssertSafe(tableName.Value, nameof(tableName)); + + if (SchemaName.IsNullOrEmpty(schemaName)) + { + return null; + } + + Identifiers.AssertSafe(schemaName.Value, nameof(schemaName)); + return schemaName; + } + + private void ThrowIfPerSchemaHasNoSchema() + { + if (Scope == MigrationHistoryScope.PerSchema && SupportsPerSchemaHistory && Configuration.SchemaName is null) + { + throw new ConfigurationException( + "MigrationHistoryScope.PerSchema requires a non-null SchemaName; there is no schema to place history in."); + } + } + + // An empty chain is allowed; a null one is a catalog defect, reported here rather than as an + // NRE further in. + private IReadOnlyList<IAmABoxMigration> GetMigrations(BoxTableName tableName, SchemaName? schemaName) + { + var migrations = _catalog.All(_configuration); + if (migrations is null) + { + throw new ConfigurationException( + $"Migration list for '{(schemaName is null ? tableName : $"{schemaName}.{tableName}")}' was null. The injected IAmABoxMigrationCatalog must return a non-null list from All(...)."); + } + + return migrations; + } + + // Lets an operator see where this run's migration history lives without reading the + // provisioner configuration (ADR 0060). + private void LogHistoryPlacement(BoxTableName tableName) => + Logger.LogInformation( + "Box migration history for {BoxTable} resolved to schema {HistorySchema} (scope {Scope})", + tableName, ResolveHistorySchema() ?? "<backend default>", _scope); + + // Rolls back with CancellationToken.None so a cancelled caller cannot abandon the unwind + // (ADR 0058 §B.3). RollbackAsync must not throw; if it does, the failure is logged rather + // than allowed to mask the primary exception the caller is about to see. + private async Task RollbackWithoutMaskingAsync(IAmAProvisioningUnitOfWork<TTransaction> uow, BoxTableName tableName) + { + try + { + await uow.RollbackAsync(CancellationToken.None); + } + catch (Exception rollbackEx) + { + Logger.LogError(rollbackEx, + "Box migration unit-of-work RollbackAsync threw while unwinding from a primary failure for table '{Table}'; rollback exception is logged, primary exception is rethrown.", + tableName); + } + } + + // Releases the unit of work (lock and transaction) before the connection. Either may be null + // if setup failed part way. The connection is disposed synchronously because DbConnection has + // no DisposeAsync on netstandard2.0. + private static async Task ReleaseAsync(IAmAProvisioningUnitOfWork<TTransaction>? uow, TConnection? connection) + { + if (uow is not null) + { + await uow.DisposeAsync(); } + connection?.Dispose(); } // The span name follows the OTel DB convention "{operation} {target}". With no schema, diff --git a/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs b/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs index 221202c09e..e8e0c07332 100644 --- a/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs +++ b/src/Paramore.Brighter.BoxProvisioning/SqlBoxProvisioner.cs @@ -94,11 +94,24 @@ protected SqlBoxProvisioner( /// <inheritdoc /> public async Task ProvisionAsync(CancellationToken cancellationToken = default) { - // Defence-in-depth at the framework chokepoint. Catalogs gate AssertSafe at the entry to - // All(...), but the provisioner opens connections and runs detection BEFORE the catalog - // returns, so an unsafe identifier from configuration reaches the live DB connection - // unless the provisioner guards at its own entry. _configuration.SchemaName is nullable: - // SQLite has no schema concept per ADR 0057 §6, so a null value is legitimate. + GuardIdentifiers(); + + var migrations = _catalog.All(_configuration); + + var tableState = await DetectAndValidateAsync(migrations, cancellationToken); + + await _migrationRunner.MigrateAsync( + BoxTableName, + _configuration.SchemaName != null ? (SchemaName)_configuration.SchemaName : null, + BoxType, + tableState, + cancellationToken); + } + + // Validates identifiers from configuration before they reach a live connection, rather than + // relying on the catalog to have done so. + private void GuardIdentifiers() + { var boxTableNameParam = BoxType == BoxType.Outbox ? nameof(IAmARelationalDatabaseConfiguration.OutBoxTableName) : nameof(IAmARelationalDatabaseConfiguration.InBoxTableName); @@ -108,31 +121,26 @@ public async Task ProvisionAsync(CancellationToken cancellationToken = default) Identifiers.AssertSafe( _configuration.SchemaName, nameof(IAmARelationalDatabaseConfiguration.SchemaName)); } + } - var migrations = _catalog.All(_configuration); - - // Sync `using`: DbConnection has no DisposeAsync on netstandard2.0. - BoxTableState tableState; - using (var connection = CreateConnection(_configuration.ConnectionString)) - { - await connection.OpenAsync(cancellationToken); + // Sync `using`: DbConnection has no DisposeAsync on netstandard2.0. + private async Task<BoxTableState> DetectAndValidateAsync( + IReadOnlyList<IAmABoxMigration> migrations, + CancellationToken cancellationToken) + { + using var connection = CreateConnection(_configuration.ConnectionString); + await connection.OpenAsync(cancellationToken); - tableState = await DetectTableStateAsync(connection, migrations, cancellationToken); + var tableState = await DetectTableStateAsync(connection, migrations, cancellationToken); - if (tableState.TableExists) - { - await _payloadValidator.ValidateAsync( - connection, BoxTableName.Value, EffectiveSchemaName, - PayloadColumnName, _configuration.BinaryMessagePayload, cancellationToken); - } + if (tableState.TableExists) + { + await _payloadValidator.ValidateAsync( + connection, BoxTableName.Value, EffectiveSchemaName, + PayloadColumnName, _configuration.BinaryMessagePayload, cancellationToken); } - await _migrationRunner.MigrateAsync( - BoxTableName, - _configuration.SchemaName != null ? (SchemaName)_configuration.SchemaName : null, - BoxType, - tableState, - cancellationToken); + return tableState; } // Pre-lock detection is only a hint: the runner re-detects authoritatively under the lock and @@ -150,19 +158,7 @@ private async Task<BoxTableState> DetectTableStateAsync( if (!tableExists) return new BoxTableState(TableExists: false, HistoryExists: false, CurrentVersion: 0); - bool historyExists; - try - { - historyExists = await _detectionHelper.DoesHistoryExistAsync( - connection, BoxTableName.Value, EffectiveSchemaName, historySchema: null, cancellationToken); - } - catch (DbException ex) - { - s_logger.LogDebug(ex, - "Pre-lock historyExists hint for '{Schema}.{Table}' unavailable; deferring to the runner's under-lock authoritative detection.", - EffectiveSchemaName, BoxTableName); - historyExists = false; - } + var historyExists = await ReadHistoryExistsHintAsync(connection, cancellationToken); if (!historyExists) { @@ -174,10 +170,31 @@ private async Task<BoxTableState> DetectTableStateAsync( CurrentVersion: detectedVersion < 0 ? 0 : detectedVersion); } - int maxVersion; + var maxVersion = await ReadMaxVersionHintAsync(connection, cancellationToken); + return new BoxTableState(TableExists: true, HistoryExists: true, CurrentVersion: maxVersion); + } + + private async Task<bool> ReadHistoryExistsHintAsync(TConnection connection, CancellationToken cancellationToken) + { + try + { + return await _detectionHelper.DoesHistoryExistAsync( + connection, BoxTableName.Value, EffectiveSchemaName, historySchema: null, cancellationToken); + } + catch (DbException ex) + { + s_logger.LogDebug(ex, + "Pre-lock historyExists hint for '{Schema}.{Table}' unavailable; deferring to the runner's under-lock authoritative detection.", + EffectiveSchemaName, BoxTableName); + return false; + } + } + + private async Task<int> ReadMaxVersionHintAsync(TConnection connection, CancellationToken cancellationToken) + { try { - maxVersion = await _detectionHelper.GetMaxVersionAsync( + return await _detectionHelper.GetMaxVersionAsync( connection, BoxTableName.Value, EffectiveSchemaName, historySchema: null, cancellationToken); } catch (DbException ex) @@ -185,9 +202,8 @@ private async Task<BoxTableState> DetectTableStateAsync( s_logger.LogDebug(ex, "Pre-lock maxVersion hint for '{Schema}.{Table}' unavailable; deferring to the runner's under-lock authoritative detection.", EffectiveSchemaName, BoxTableName); - maxVersion = 0; + return 0; } - return new BoxTableState(TableExists: true, HistoryExists: true, CurrentVersion: maxVersion); } /// <summary> From 509ac601a7e791eb3b2b8b1c24f5221538b68ea6 Mon Sep 17 00:00:00 2001 From: iancooper <ian_hammond_cooper@yahoo.co.uk> Date: Sun, 11 Oct 2026 21:31:18 +0100 Subject: [PATCH 5/5] docs: teach /refactor:tidy-comments to slice large scopes and handle old ranges Size the scope before reading it and propose one-project slices, named as '<range> <path>' commands; tidy files as they are now when a range is old, marking comments added after it for the user to include or not; track the slice queue in PROMPT.md; add 'file a GitHub issue' to Phase 5. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7 --- .claude/commands/refactor/tidy-comments.md | 46 ++++++++++++++++++++-- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/.claude/commands/refactor/tidy-comments.md b/.claude/commands/refactor/tidy-comments.md index e7fa96877a..e64ef2203c 100644 --- a/.claude/commands/refactor/tidy-comments.md +++ b/.claude/commands/refactor/tidy-comments.md @@ -1,7 +1,7 @@ --- allowed-tools: Read, Edit, Glob, Grep, Bash, AskUserQuestion, TodoWrite description: Bring comments into line with Brighter's comment rules, and surface refactorings a comment side-stepped -argument-hint: [base-ref | base..head | path...] +argument-hint: [base-ref | base..head] [path...] --- # Tidy Comments - Replace Comments With Intent @@ -39,16 +39,36 @@ Resolve the scope from the arguments: | a git ref (e.g. `HEAD~3`) | `git diff <ref>` | | a range `<base>..<head>` | `git diff <base>..<head>` — work already committed or merged | | one or more paths | The changes to those paths on the current branch; if they have no changes, **every** comment in those files | +| a ref or range, then paths (e.g. `abc123^1..abc123 src/Paramore.Brighter.Foo`) | The ref or range, limited to those paths. This is how a slice is named (see step 3 below) | Then: 1. List the changed `*.cs` files with `git diff --stat` and show them to the user. 2. **Exclude** generated tests (see `.agent_instructions/generated_tests.md`) — fix the template instead, if the template is in scope — and `*.Designer.cs`, migrations, and non-code files. -3. With a diff scope, tidy only the comments **this diff added or changed**. Untouched comments are +3. **Size the scope before reading it.** Count the comment lines the scope added, split into `//` + or `/* */` lines and `///` lines, per project (`git diff <scope> -- <project> | grep -E '^\+\s*//'`). + If the scope is more than one project, or more than about 150 `//` lines, a single plan is too big + to review. Propose slices with `AskUserQuestion`: + - one project per slice, `src/` before `tests/`; + - start with the project the others depend on (a shared base package before its backends), so + the first slice calibrates the decisions the rest repeat; + - name each slice as a command — `/refactor:tidy-comments <ref-or-range> <project-path>`. + + Work only the first slice in this run. Record the remaining slices in `PROMPT.md` (see *Tracking + Slices* below). +4. **If the scope is a range or ref in the past, the files may have moved on.** Check with + `git log --oneline <head>..HEAD -- <path>`. You tidy the files as they are now, not as they were at + `<head>`: + - A comment the range added that has since been deleted is gone; skip it. + - A comment the range added that has since been edited is still in scope. + - A comment added **after** the range, in a file the range touched, is out of scope by the + rule in step 5. Mark these comments **†** in the plan and ask the user, with the plan + approval, whether to include them. Tidying a file once is cheaper than coming back to it. +5. With a diff scope, tidy only the comments **this diff added or changed**. Untouched comments are out of scope; churning them hides the real change in review. Read each file in full anyway — you need the surrounding method to decide whether a comment can become an extraction. -4. Check the build is green and the relevant tests pass. If the work is still moving or the tests +6. Check the build is green and the relevant tests pass. If the work is still moving or the tests fail, say so and stop: the extractions below need a green baseline. If the scope is empty, say so and stop. @@ -144,11 +164,29 @@ comment and the code it excuses), the Fowler refactoring, kind (`structural`, `b 2. **Run with `/test-first`** — for behaviour changes. 3. **Add to the spec's `tasks.md`** — when a spec is active. Follow `/refactor:smells` Phase 4 for the task format; show the text before editing; do not commit. -4. **Record only**, or **Skip**. +4. **File a GitHub issue** — show the text before creating it. +5. **Record only**, or **Skip**. + +If this run was one slice of a larger scope, update `PROMPT.md` (see *Tracking Slices*). Removing the comment over a side-stepped refactoring is the refactoring's job, not this one's: leave the comment until the refactoring makes it unnecessary. +## Tracking Slices + +When a scope is sliced, a fresh session must be able to pick up the next slice. `PROMPT.md` in the +repo root (gitignored, local) is where that state lives; see CLAUDE.md § *Context Management*. + +Keep a `## Comment tidy` section in `PROMPT.md` with: + +- the branch the tidy commits land on; +- the scope as a whole, and the decisions made on the first slice that later slices should repeat + (for example whether † comments are included, or whether issue links in XML docs are stripped); +- a checklist of slices, each as its exact command, ticked with its commit SHAs when it lands; +- the issues or tasks filed in Phase 5, so they are not filed twice. + +Update it at the end of each run. Show the user the section before writing it, the first time. + ## Rules - **No behaviour changes.** If a tidy needs one, it is not a tidy.