Comment rules, /refactor:tidy-comments, and a first tidy of BoxProvisioning

[iancooper]

Why

Comments written by agents have been creeping into method bodies: about 1,100 // lines were added to src/ between 1 September and 11 October, many carrying spec or issue numbers, change history or notes to a reviewer. The rules about where comments go were spread across documentation.md and code_style.md, partly contradicted each other, and were missing from the skills agents actually work from.

What

The rule — .agent_instructions/documentation.md gains a Comments in Code section, which is now the single statement of the rule:

• /// XML documentation on public and protected members of public types only. <summary>, <param> and so on say how to use the API; <remarks> holds notes to the user.
• On anything else, at most a // or /* */ comment above the signature, when the method is complex or guards against a trap.
• No comments inside method bodies, as a rule. Use Extract Method to express intent instead. The exceptions are //Arrange //Act //Assert, the reason on an analyser suppression, and a rare one-line trap that cannot be extracted.
• No issue, PR or spec references, no change history, no notes to a reviewer. Commented-out code is deleted.

code_style.md and CONTRIBUTING.md now align with it, and /test-first, /spec:implement, /spec:ralph-implement, /tidy-first, /bugfix:fix and /spec:review all carry it.

The skill — /refactor:tidy-comments [ref | range] [path...] gives each comment in a unit of work one action: keep, delete, strip a reference, trim, move above the signature, convert /// to //, or replace with Extract Method/Variable. It applies them as a comment-only refactor: commit and then an extraction refactor: commit, with tests unedited and no public signature changed. Rationale missing from an ADR goes to the user, not the bin. It also reports comments that were written instead of a refactoring ("must be called first", "can be null when…") and schedules them. A large scope is sliced per project and the queue is tracked in PROMPT.md.

First slice — src/Paramore.Brighter.BoxProvisioning, from the PR #4039 merge (83e0e5e47):

• ADR 0058 records two decisions that only lived in comments: why provisioners within a group run in whatever order DI resolves them, and why pre-lock detection and payload validation share one connection.
• A comment-only pass across 9 files (net −72 lines).
• Ten private methods replace explanatory comments in SqlBoxMigrationRunner.MigrateAsync, SqlBoxProvisioner.ProvisionAsync and UseBoxProvisioning, e.g. GuardIdentifiers, RollbackWithoutMaskingAsync, ReleaseAsync, ReadHistoryExistsHintAsync, RegisterProvisionersThenMarker. Comments inside method bodies in this package go from 116 lines to none.

Two refactorings the comments had been papering over are filed rather than done here: #4588 (static ApplicationLogging logger in SqlBoxProvisioner) and #4589 (SchemaName conversion from a null or empty string yields a non-null SchemaName).

Testing

• No behaviour change, and no public or protected signature changed.
• Paramore.Brighter.BoxProvisioning.Tests: 111/111 passing on net9.0 and net10.0, before and after, with no test edited.
• Paramore.Brighter.Sqlite.Tests --filter BoxProvisioning: 66/66 passing on net9.0 and net10.0, before and after.
• The package builds with 0 warnings.
• The MSSQL, PostgreSQL, MySQL and Spanner box-provisioning tests were not run locally (they need containers); CI covers them.

Next

The remaining slices (the BoxProvisioning backends, the rest of src/, then tests/) follow as separate PRs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7