docs(AGENTS): capture recurring PR review feedback as conventions

[polyglotAI-bot]

I reviewed the review comments on my last 10 PRs in this repo (#467, #469, #473, #474, #477, #479, #480, #481, #482, #484, plus #445/#446) and pulled out the feedback that recurred. Each rule below was asked for at least twice, or once in a way that killed a PR. Documenting them so they're followed the first time instead of costing a review round-trip.

What was repeated, and what I added

Feedback	Where it came up	Added to
"Make the changelog/releasenotes vastly more concise", "one short sentence is enough"	#474, #477, #480, #481, #482	Changelog and release notes
"Let's not include the benchmark"	#481, #482	Performance Characteristics
"Those test cases are already covered by existing ones, check for duplicates (probably in TestCases.cs)"	#480, #482	Testing Discipline
"We don't need #if NET6_0_OR_GREATER, we already only target .net 6+"	#446, #484	Code Style
"Versions before 25.8 LTS are not officially supported, we're going to skip this" — closed the PR	#469	Project Context
"I would strongly prefer integration tests over the unit tests we have"	#479, and #477 where mock tests passed against a byte layout the server never emits	Testing Discipline
"The comments say the CRLF is optional, is that actually true?" / changelog over-promising what the code guarantees	#477, #469, #479	Code Style, Changelog
"What are the performance implications? Is there some alternative approach?" on adding a guard to every numeric accessor	#445	Performance Characteristics

Two of these narrow existing guidance rather than adding new rules: the integration-test bullet already existed but didn't say why a mock passes when the server disagrees, and the framework list already said net6+ but not that guards below it are dead.

Notes

• The 25.8 floor is stated against .github/workflows/tests.yml, so it stays checkable rather than becoming stale prose.
• Utilities/TestCases.cs / GetDataTypeSamples() verified as the shared source feeding the select, parameter, bulk-copy, serialisation, Dapper and DataAdapter suites — that's why it's the first place to check before adding a type test.
• /benchmark-compare verified as a maintainer-gated PR-comment trigger (stresshouse-benchmark-compare.yml requires OWNER/MEMBER/COLLABORATOR), so the wording says to ask a maintainer for it.
• Docs-only; no changelog entry, matching docs(AGENTS): test isolation, determinism & parametrization conventions #429.

[Copilot]

Pull request overview

This PR updates AGENTS.md (the repository’s development guide) to codify several recurring review points as explicit conventions, aiming to reduce review round-trips by making expectations discoverable up-front.

Changes:

• Document the supported ClickHouse version floor (25.8 LTS+) and explicitly de-scope fixes/workarounds for older server behaviors.
• Add clearer performance guidance (avoid taxing hot paths for niche cases; prefer maintainer-run /benchmark-compare over committing benchmark additions).
• Expand testing/style guidance (integration-test rationale, avoid duplicating existing type coverage, avoid redundant NET6_0_OR_GREATER-style guards, and avoid over-claiming in comments/changelog entries).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[polyglotAI-bot]

Pushed 2a53f9d — sharpened the redundant-coverage rule to name the specific shape that keeps recurring.

The two review comments behind it (#480 "I don't think the new test cases cover anything that pre-existing tests don't", #482 "check for duplicates, probably in TestCases.cs") were both really about control cases: tests that pin behavior the change never touched, added as a safety net. Those are exactly what TestCases.cs already covers, so the rule now calls that out rather than leaving "check for duplicates" as a judgement call.

Self-audit while writing it: my own open #484 has one — Format_TimeSpanBoundToTime_ReturnsFormattedTime pins TimeSpan→Time, which that PR doesn't change, and TestCases.cs:354-361 already round-trips Time/Time64(N) with TimeSpan through SqlParameterizedSelectTests into the same formatter path. I'll drop it there.

[polyglotAI-bot]

Pushed 44576a0 — corrected the benchmark rule, which I'd overstated.

I'd written "don't commit a new benchmark file", but that conflates two things. Performance-related changes should be measured with BenchmarkDotNet — the ask on #481/#482 was about what ships, not whether to benchmark. And #445 was the opposite ask ("can you do a quick benchmark?"), so a blanket prohibition would have contradicted it.

Now: measure perf changes with BenchmarkDotNet and put the numbers in the PR description; an ad-hoc benchmark written to answer one question doesn't need to ship, but one worth re-running later belongs in the repo.