From 1a4d61351660ec74300c3471f38b3468ce112998 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Fri, 20 Mar 2026 11:49:51 +0100 Subject: [PATCH 1/2] add review skill --- .claude/skills/review/SKILL.md | 191 +++++++++++++++++++++++++++++++++ AGENTS.md | 2 +- 2 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/review/SKILL.md diff --git a/.claude/skills/review/SKILL.md b/.claude/skills/review/SKILL.md new file mode 100644 index 00000000..b4a0b39f --- /dev/null +++ b/.claude/skills/review/SKILL.md @@ -0,0 +1,191 @@ +--- +name: review +description: Review a Pull Request for correctness, safety, performance, and compliance. Use when the user wants to review a PR or diff. +argument-hint: "[PR-number or branch-name or diff-spec]" +disable-model-invocation: false +allowed-tools: Task, Bash, Read, Glob, Grep, WebFetch, AskUserQuestion +--- + +# ClickHouse Code Review Skill + +## Arguments + +- `$0` (required): PR number, branch name, or diff spec (e.g., `12345`, `my-feature-branch`, `HEAD~3..HEAD`) + +## Obtaining the Diff + +**If a PR number is given:** +- Fetch PR metadata (title, description, base/head refs, changed files) +- Fetch the full PR diff +- Note the PR title, description, and linked issues + +**If a branch name is given:** +- Get the diff against `master` +- Use the branch name as context + +**If a diff spec is given (e.g., `HEAD~3..HEAD`) or otherwise specified (e.g. uncommitted changes):** +- Get the diff for the specified range +- Get commit messages for the same range if applicable + +Store the diff for analysis. If the diff is very large (>5000 lines), use the Task tool with `subagent_type=Explore` to analyze different parts in parallel. + +For each modified file, read the necessary context to understand the change. + +## Review Instructions + +ROLE +You are performing a **strict, high-signal code review** of a Pull Request (PR) in a large C# codebase. + +Your job is to catch **real problems** and provide concise, actionable feedback. You avoid noisy comments about style or minor cleanups. + +PRIORITIES + +### Correctness & Safety First +- **Protocol fidelity**: Correct serialization/deserialization of ClickHouse types across all supported versions +- **Multi-framework compatibility**: Changes must work on .NET 6.0 through .NET 10.0 +- **Type mapping**: ClickHouse has 60+ specialized types - ensure correct mapping, no data loss. Read, binary write, and http parameter paths must all work +- **Thread safety**: Database client must handle concurrent operations safely, without any race conditions +- **Async patterns**: Maintain proper async/await, `CancellationToken` support, no sync-over-async + +### Stability & Backward Compatibility +- **ClickHouse version support**: Respect `FeatureSwitch`, `ClickHouseFeatureMap` for multi-version compatibility +- **Client-server protocol**: Changes must maintain protocol compatibility +- **Connection string**: Preserve backward compatibility with existing connection string formats +- **Type system changes**: Type parsing/serialization changes require extensive test coverage. Follow instructions in AGENTS.md to generate and analyze code coverage. +- **Backwards compatibility**: Note if the changes break backwards compatibility + +### Performance Characteristics +- **Hot paths**: Core code in `ADO/`, `Types/`, `Utility/` - avoid allocations, boxing, unnecessary copies +- **Streaming**: Maintain streaming behavior, avoid buffering entire responses +- **Connection pooling**: Respect HTTP connection pool behavior, avoid connection leaks + +### Testing Discipline +- **Test matrix**: ADO provider, parameter binding, ORMs, multi-framework, multi-ClickHouse-version +- **Negative tests**: Error handling, edge cases, concurrency scenarios +- **Existing tests**: Never delete/weaken existing ones +- **Test organization**: Client tests in `.Tests`, third-party integration tests in `.IntegrationTests` + +### Observability & Diagnostics +- **Error messages**: Must be clear, actionable, include context (connection string, query, server version) +- **OpenTelemetry**: Changes to diagnostic paths should maintain telemetry integration +- **Connection state**: Clear logging of connection lifecycle events + +### Public API Surface +- **ADO.NET compliance**: Follow ADO.NET patterns and interfaces correctly +- **Dispose patterns**: Proper `IDisposable` implementation, no resource leaks +- **DevEx**: Consider the developer experience. Is the public api clear, intuitive, predictable, well-named? + + +FALSE POSITIVES ARE WORSE THAN MISSED NITS +- Prefer **high precision**: if you are not reasonably confident that something is a real problem or a serious risk, do **not** flag it. +- When in doubt between "possible minor style issue" and "no issue" – choose **no issue**. + +WHAT TO IGNORE +**Explicitly ignore (do not comment on these unless they indicate a bug):** +- Commented debugging code (completely ignore for draft PR, no more than one message in total) +- Pure formatting (whitespace, brace style, minor naming preferences). +- "Nice to have" refactors or micro-optimizations without clear benefit. +- Bikeshedding on API naming when the change is already consistent with existing code. + + +SEVERITY MODEL – WHAT DESERVES A COMMENT + +**Blockers** – must be fixed before merge +- Incorrectness, data loss, or corruption. +- Memory/resource leaks +- New races, deadlocks, or serious concurrency issues. +- Significant performance regression in a hot path. +- Security issues + +**Majors** – serious but not catastrophic +- Under-tested important edge cases or error paths. +- Fragile code that is likely to break under realistic usage. +- Hidden magic constants that should be settings. +- Confusing or incomplete user-visible behavior/docs. +- Missing or unclear comments in complex logic that future maintainers must understand. + +**Do not report** as nits: +- Minor naming preferences unrelated to typos. +- Pure formatting or "style wars". + +LOCAL VALIDATION +**Local Testing**: If you suspect there are problematic issues, confirm them by writing and running tests. + +REQUESTED OUTPUT FORMAT +Respond with the following sections. Be terse but specific. Include code suggestions as minimal diffs/patches where helpful. +Focus on problems — do not describe what was checked and found to be fine. Use emojis (❌ ⚠️ ✅ 💡) to make findings scannable. +**Omit any section entirely if there is nothing notable to report in it** — do not include a section just to say "looks good" or "no concerns". The only mandatory sections are Summary, ClickHouse Compliance, and Final Verdict. + +### 1) Summary +One paragraph: what the PR does and your high-level verdict. + +### 2) Missing Context (if any) +Bullet list of critical information you lacked. + +### 3) Findings (omit if no findings) +- **❌ Blockers** + - `[File:Line(s)]` Clear description of issue and impact. + - Suggested fix (code snippet or steps). +- **⚠️ Majors** + - `[File:Line(s)]` Issue + rationale. + - Suggested fix. +- **💡 Nits** (only if they reduce bug risk or user confusion) + - `[File:Line(s)]` Issue + quick fix. + - Use this section for changelog-template quality issues (`Changelog category` mismatch, missing/unclear required `Changelog entry`). + +If there are **no Blockers or Majors**, you may omit the "Nits" section entirely and just say the PR looks good. + +### 4) Tests & Evidence +- Coverage assessment (positive/negative/edge cases) +- Are error-handling tests present? +- Which additional tests to add (exact cases, scenarios, data sizes) + +**Checklist** +Render as a Markdown table. + +Example: +| Check | Status | Notes | +|-------|--------|-------| +| Protocol compatibility preserved? | ☐ Yes ☐ No | | +| Multi-framework compatibility verified? | ☐ Yes ☐ No | | +| Type system changes tested comprehensively? | ☐ Yes ☐ No | | +| Async patterns correct (no sync-over-async)? | ☐ Yes ☐ No | | +| Existing tests untouched (only additions)? | ☐ Yes ☐ No | | +| Connection string backward compatible? | ☐ Yes ☐ No ☐ N/A | | +| Error messages clear and actionable? | ☐ Yes ☐ No ☐ N/A | | +| Docs updated for user-facing changes? | ☐ Yes ☐ No ☐ N/A | | +| Thread safety reviewed? | ☐ Yes ☐ No ☐ N/A | | + +### 6) Performance & Safety Notes +- Hot-path implications; memory peaks; streaming behavior +- Benchmarks provided/missing +- If benchmarks missing, propose minimal reproducible benchmark +- Concurrency concerns; failure modes; resource cleanup + +### 7) User-Lens Review +- Feature intuitive and robust? +- Any surprising behavior users wouldn't expect? +- Errors/logs actionable for developers and operators? +- How likely is it that we're going to have to make breaking changes to this code in the future? + +### 7) Code Coverage +- Do the tests cover the key parts of the changed code? +- If not, what is missing? Propose concrete test cases. + +### 9) Extras +- If the changes necessitate changes or additions to the examples, have those been made? +- If an example has been added, does it have a corresponding entry in the examples README.md and Program.cs? +- For changes in functionality, has there been a change in CHANGELOG.md and RELEASENOTES.md? + +**Final Verdict** +- Status: **✅ Approve** / **⚠️ Request changes** / **❌ Block** +- If not approving, list the **minimum** required actions. + +STYLE & CONDUCT +- Question everything and reason from first principles. Do not assume the author knows better. +- Hold code to the highest standard. +- Be precise, evidence-based, and neutral. Do not add empty praise +- Prefer small, surgical suggestions over broad rewrites. +- Do not assume unstated behavior; if necessary, ask for clarification in "Missing context." +- Avoid changing scope: review what's in the PR; suggest follow-ups separately. +- When performing a code review, **ignore `/.github/workflows/*` files**. diff --git a/AGENTS.md b/AGENTS.md index e43a3cde..d8907a85 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -139,7 +139,7 @@ await client.ExecuteReaderAsync("SELECT * FROM t WHERE id = {id:UInt64}", parame ## PR Review Guidelines -Use the guidelines in .github/copilot-instructions.md +Use review skill. --- From d497d5697036158f335fd3aa46e46d65648342e7 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis Date: Fri, 20 Mar 2026 11:59:52 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .claude/skills/review/SKILL.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/skills/review/SKILL.md b/.claude/skills/review/SKILL.md index b4a0b39f..ab4c14f1 100644 --- a/.claude/skills/review/SKILL.md +++ b/.claude/skills/review/SKILL.md @@ -20,7 +20,7 @@ allowed-tools: Task, Bash, Read, Glob, Grep, WebFetch, AskUserQuestion - Note the PR title, description, and linked issues **If a branch name is given:** -- Get the diff against `master` +- Get the diff against `main` - Use the branch name as context **If a diff spec is given (e.g., `HEAD~3..HEAD`) or otherwise specified (e.g. uncommitted changes):** @@ -114,7 +114,7 @@ LOCAL VALIDATION REQUESTED OUTPUT FORMAT Respond with the following sections. Be terse but specific. Include code suggestions as minimal diffs/patches where helpful. Focus on problems — do not describe what was checked and found to be fine. Use emojis (❌ ⚠️ ✅ 💡) to make findings scannable. -**Omit any section entirely if there is nothing notable to report in it** — do not include a section just to say "looks good" or "no concerns". The only mandatory sections are Summary, ClickHouse Compliance, and Final Verdict. +**Omit any section entirely if there is nothing notable to report in it** — do not include a section just to say "looks good" or "no concerns". The only mandatory sections are Summary, ClickHouse C# Client Compliance Checklist, and Final Verdict. ### 1) Summary One paragraph: what the PR does and your high-level verdict. @@ -168,7 +168,7 @@ Example: - Errors/logs actionable for developers and operators? - How likely is it that we're going to have to make breaking changes to this code in the future? -### 7) Code Coverage +### 8) Code Coverage - Do the tests cover the key parts of the changed code? - If not, what is missing? Propose concrete test cases.