From f11327cc0a4e724bb3bf1ea90630f615d0efa5a4 Mon Sep 17 00:00:00 2001 From: Milind Srivastava Date: Tue, 18 Aug 2026 11:04:40 -0400 Subject: [PATCH 1/2] feat(skills): add code-design-review and test-coverage-review skills Codify recurring PR review guidance (fail loud, no magic numbers, precise naming, no half-built abstractions, negative-path test coverage) as Claude Code skills, plus a CLAUDE.md with PR-hygiene process notes. Co-Authored-By: Claude Sonnet 5 --- .claude/skills/code-design-review/SKILL.md | 107 +++++++++++++++++++ .claude/skills/test-coverage-review/SKILL.md | 40 +++++++ CLAUDE.md | 16 +++ 3 files changed, 163 insertions(+) create mode 100644 .claude/skills/code-design-review/SKILL.md create mode 100644 .claude/skills/test-coverage-review/SKILL.md create mode 100644 CLAUDE.md diff --git a/.claude/skills/code-design-review/SKILL.md b/.claude/skills/code-design-review/SKILL.md new file mode 100644 index 0000000..a1ca78c --- /dev/null +++ b/.claude/skills/code-design-review/SKILL.md @@ -0,0 +1,107 @@ +--- +name: code-design-review +description: > + ASAPQuery-specific code design checklist covering error handling, magic + numbers/config defaults, naming precision, and half-built abstractions. + Use whenever writing or editing Rust (asap-query-engine, asap-planner-rs, + asap_sketchlib) or Python (asap-tools) code in this repo, not just when + asked to "review" — apply it while implementing, refactoring, or fixing + bugs, and consult it again before finishing any PR-sized change here. +--- + +# ASAPQuery code design checklist + +Apply these when writing code, not just at review time. Catching these before the PR exists is cheaper than fixing them after. + +## 1. Fail loud + +Don't let a failure path quietly resolve to a default, an empty value, or a +swallowed error. If something goes wrong, panic, return `Result`/`Error`, or +raise — never fall through to a value that looks like success. + +**Why:** a hardcoded fallback or `unwrap_or_default()` on a failure path +turns a real bug into silent wrong output. It's much harder to find a bug +that never surfaces than one that panics immediately. + +❌ `kll_to_msgpack` returning an empty `Vec` on serialization failure — +looks like "no data" to every caller, not "this broke." + +✅ Same failure path in the CMS serializer: panics, so the break is visible +at the call site instead of corrupting output downstream. + +Applies to: error branches in Rust (`unwrap_or`, `unwrap_or_default`, +`.ok()` discarding an `Err`), Python `except: pass` / bare fallback returns, +and config resolution that silently picks a default when a required value is +missing. + +## 2. No magic numbers or strings, one source of truth for each + +Pull literal numbers (timeouts, poll intervals, thresholds) and strings into named constants, and don't let the same default get set in more than one place. + +**Why:** an inline `30` or `300` in one file, and the same 30 hardcoded +again three files over, are the same bug waiting to happen twice — someone +changes one and not the other, and now the values silently disagree. This is +the same failure mode whether it's a bare literal or a duplicated default +value, so treat them as one problem: give the value exactly one home. + +❌ Timeout/poll literals (`30`, `60`, `300`, `10`, `5`, `2`, `0.5s`) +scattered inline across several files in one module, while the rest of the +module uses named interval constants — breaks the existing pattern. + +❌ A default sketch backend set as a fallback argument in multiple +functions instead of one global/config value. + +✅ One named constant (or one config-file entry with no inline fallback +default) that every caller references. + +## 3. Names carry the truth about the value + +A name should precisely describe what it holds or does — including units for +anything time-like (`_MS`, `_SECS`) — and must be renamed the moment its +behavior changes underneath it. + +**Why:** "seconds or milliseconds?" is a question that shouldn't need to be +asked; the field name should already answer it. A stale name (a "clone" +helper that now moves, a "pieces" builder that now also does +post-processing) actively misleads the next reader into wrong assumptions. + +❌ `impl_clone_accumulator_methods` macro that now also generates +`into_accumulator`, an explicit move — the name promises cloning it doesn't +do. + +❌ `KEY_SLIDE_INTERVAL` / `KEY_WINDOW_SIZE` holding millisecond values with +no `_MS` suffix, inconsistent with the rest of the same PR's naming. + +✅ Rename on every semantic change, even mid-refactor, even if it touches +more call sites — don't defer it to "later." + +## 4. No half-built abstractions, no duplicated logic + +Either finish an abstraction's boundary (all callers go through it) or +delete it — don't leave a shim that some callers bypass. Before writing new +logic, check whether an existing helper/module already does it. + +**Why:** a half-finished shim (e.g. isolating an internal type, but callers +still reach through it directly) gives the illusion of a stable boundary +while every upstream change still breaks N call sites — worse than no +abstraction, because it hides the real blast radius. Duplicated logic (a +poll-with-retry loop, a query-string decomposer) drifts the moment one copy +gets fixed and the other doesn't. + +❌ `output/mod.rs` declaring three submodules that don't exist on disk — an +abandoned mid-refactor stub left in the tree. + +❌ A service-readiness poll loop reimplemented locally when +`DockerServiceBase._wait_for_service_ready` already provides it. + +✅ Either route every caller through the shim/abstraction, or delete it and +let callers use the underlying thing directly. Either way, grep for existing +helpers before writing a new poll/dedup/parse routine. + +## Also check: internal consistency + +Within one file or module, pick one convention (error type, dedup structure, +timeout threading) and use it everywhere in that file — don't mix +`Vec::contains` dedup in one function and `IndexSet` in the next function of +the same file, or thread a `timeout` param into one call and hardcode a +different value in the next. diff --git a/.claude/skills/test-coverage-review/SKILL.md b/.claude/skills/test-coverage-review/SKILL.md new file mode 100644 index 0000000..fe23410 --- /dev/null +++ b/.claude/skills/test-coverage-review/SKILL.md @@ -0,0 +1,40 @@ +--- +name: test-coverage-review +description: > + ASAPQuery-specific test coverage checklist: regression/negative-path + coverage and preserving why-comments on regression tests. Use whenever adding or + changing behavior in ASAPQuery. Check this before considering a feature or fix done, not only + when asked to review tests. +--- + +# ASAPQuery test coverage checklist + +## 1. Cover the negative/failure paths, not just the happy path + +New behavior — especially error branches, merge/fallback logic, and +edge-case query syntax — needs a dedicated test for what happens when it +fails, not just a test that the normal case works. + +**Why:** Failure modes are most likely to surface in production, and tests that only check happy paths won't catch these. +A missing correctness/regression test also means there's no +guard against a future refactor quietly changing behavior (e.g. a new store +implementation silently diverging from the one it replaces). + +Checklist when adding a feature or fix: +- New merge/combine/accumulator logic → test the error path, not just the + success path. +- A new query-syntax feature (e.g. new PromQL/SQL clause) → test that + existing related behavior (e.g. `topk`) still works alongside it. +- Replacing or refactoring a store/backend → add a correctness test proving + it produces the same output as what it replaces, not just a benchmark. + +## 2. Keep the explanatory comment on regression tests + +If a test exists because of a specific past bug, the comment explaining +*that bug* is part of the test, not decoration. Don't strip it during +cleanup or refactor. + +**Why:** a regression test with no comment just looks like an arbitrary edge +case to the next reader — the comment is what makes it legible why this +input is tested at all, and prevents someone "simplifying" the test away +later. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..26f0e2b --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,16 @@ +# Process conventions + +- **Keep PRs small and single-purpose.** Split a PR that mixes multiple + objectives (e.g. a rename + unrelated behavior change) into separate PRs so + the repo stays runnable and reviewable at every merge point. +- **PR titles follow Conventional Commits**: `(): `, + e.g. `fix(precompute): ...`, `feat(query-engine): ...`, + `refactor(asap-tools): ...`, `perf(precompute): ...`. Match the existing + scope names used in this repo's PR history (`precompute`, `query-engine`, + `planner`, `asap-tools`, `sql-parser`, `deps`, etc.) rather than inventing + new ones. + +# Code design and test coverage + +See the `code-design-review` and `test-coverage-review` skills — apply them +while writing code, not just when asked to review. From 8a83e0511d149fd4e6579cfb5de61bf7453199a0 Mon Sep 17 00:00:00 2001 From: Milind Srivastava Date: Sun, 23 Aug 2026 14:39:05 -0400 Subject: [PATCH 2/2] refactor(skills): make code-design/test-coverage checklists agent-agnostic Move canonical checklist content to .agents/skills/*.md and add AGENTS.md so non-Claude agents can read them too. .claude/skills/*/SKILL.md keep their frontmatter for Claude Code's auto-discovery but now just point at the shared files; CLAUDE.md points at AGENTS.md. Co-Authored-By: Claude Sonnet 5 --- .agents/skills/code-design-review.md | 101 +++++++++++++++++++ .agents/skills/test-coverage-review.md | 34 +++++++ .claude/skills/code-design-review/SKILL.md | 96 +----------------- .claude/skills/test-coverage-review/SKILL.md | 31 +----- AGENTS.md | 17 ++++ CLAUDE.md | 20 +--- 6 files changed, 160 insertions(+), 139 deletions(-) create mode 100644 .agents/skills/code-design-review.md create mode 100644 .agents/skills/test-coverage-review.md create mode 100644 AGENTS.md diff --git a/.agents/skills/code-design-review.md b/.agents/skills/code-design-review.md new file mode 100644 index 0000000..820c0cb --- /dev/null +++ b/.agents/skills/code-design-review.md @@ -0,0 +1,101 @@ +# ASAPQuery code design checklist + +Use whenever writing or editing Rust (asap-query-engine, asap-planner-rs, +asap_sketchlib) or Python (asap-tools) code in this repo, not just when +asked to "review" — apply it while implementing, refactoring, or fixing +bugs, and consult it again before finishing any PR-sized change here. + +Apply these when writing code, not just at review time. Catching these before the PR exists is cheaper than fixing them after. + +## 1. Fail loud + +Don't let a failure path quietly resolve to a default, an empty value, or a +swallowed error. If something goes wrong, panic, return `Result`/`Error`, or +raise — never fall through to a value that looks like success. + +**Why:** a hardcoded fallback or `unwrap_or_default()` on a failure path +turns a real bug into silent wrong output. It's much harder to find a bug +that never surfaces than one that panics immediately. + +❌ `kll_to_msgpack` returning an empty `Vec` on serialization failure — +looks like "no data" to every caller, not "this broke." + +✅ Same failure path in the CMS serializer: panics, so the break is visible +at the call site instead of corrupting output downstream. + +Applies to: error branches in Rust (`unwrap_or`, `unwrap_or_default`, +`.ok()` discarding an `Err`), Python `except: pass` / bare fallback returns, +and config resolution that silently picks a default when a required value is +missing. + +## 2. No magic numbers or strings, one source of truth for each + +Pull literal numbers (timeouts, poll intervals, thresholds) and strings into named constants, and don't let the same default get set in more than one place. + +**Why:** an inline `30` or `300` in one file, and the same 30 hardcoded +again three files over, are the same bug waiting to happen twice — someone +changes one and not the other, and now the values silently disagree. This is +the same failure mode whether it's a bare literal or a duplicated default +value, so treat them as one problem: give the value exactly one home. + +❌ Timeout/poll literals (`30`, `60`, `300`, `10`, `5`, `2`, `0.5s`) +scattered inline across several files in one module, while the rest of the +module uses named interval constants — breaks the existing pattern. + +❌ A default sketch backend set as a fallback argument in multiple +functions instead of one global/config value. + +✅ One named constant (or one config-file entry with no inline fallback +default) that every caller references. + +## 3. Names carry the truth about the value + +A name should precisely describe what it holds or does — including units for +anything time-like (`_MS`, `_SECS`) — and must be renamed the moment its +behavior changes underneath it. + +**Why:** "seconds or milliseconds?" is a question that shouldn't need to be +asked; the field name should already answer it. A stale name (a "clone" +helper that now moves, a "pieces" builder that now also does +post-processing) actively misleads the next reader into wrong assumptions. + +❌ `impl_clone_accumulator_methods` macro that now also generates +`into_accumulator`, an explicit move — the name promises cloning it doesn't +do. + +❌ `KEY_SLIDE_INTERVAL` / `KEY_WINDOW_SIZE` holding millisecond values with +no `_MS` suffix, inconsistent with the rest of the same PR's naming. + +✅ Rename on every semantic change, even mid-refactor, even if it touches +more call sites — don't defer it to "later." + +## 4. No half-built abstractions, no duplicated logic + +Either finish an abstraction's boundary (all callers go through it) or +delete it — don't leave a shim that some callers bypass. Before writing new +logic, check whether an existing helper/module already does it. + +**Why:** a half-finished shim (e.g. isolating an internal type, but callers +still reach through it directly) gives the illusion of a stable boundary +while every upstream change still breaks N call sites — worse than no +abstraction, because it hides the real blast radius. Duplicated logic (a +poll-with-retry loop, a query-string decomposer) drifts the moment one copy +gets fixed and the other doesn't. + +❌ `output/mod.rs` declaring three submodules that don't exist on disk — an +abandoned mid-refactor stub left in the tree. + +❌ A service-readiness poll loop reimplemented locally when +`DockerServiceBase._wait_for_service_ready` already provides it. + +✅ Either route every caller through the shim/abstraction, or delete it and +let callers use the underlying thing directly. Either way, grep for existing +helpers before writing a new poll/dedup/parse routine. + +## Also check: internal consistency + +Within one file or module, pick one convention (error type, dedup structure, +timeout threading) and use it everywhere in that file — don't mix +`Vec::contains` dedup in one function and `IndexSet` in the next function of +the same file, or thread a `timeout` param into one call and hardcode a +different value in the next. diff --git a/.agents/skills/test-coverage-review.md b/.agents/skills/test-coverage-review.md new file mode 100644 index 0000000..3bddd15 --- /dev/null +++ b/.agents/skills/test-coverage-review.md @@ -0,0 +1,34 @@ +# ASAPQuery test coverage checklist + +Use whenever adding or changing behavior in ASAPQuery. Check this before +considering a feature or fix done, not only when asked to review tests. + +## 1. Cover the negative/failure paths, not just the happy path + +New behavior — especially error branches, merge/fallback logic, and +edge-case query syntax — needs a dedicated test for what happens when it +fails, not just a test that the normal case works. + +**Why:** Failure modes are most likely to surface in production, and tests that only check happy paths won't catch these. +A missing correctness/regression test also means there's no +guard against a future refactor quietly changing behavior (e.g. a new store +implementation silently diverging from the one it replaces). + +Checklist when adding a feature or fix: +- New merge/combine/accumulator logic → test the error path, not just the + success path. +- A new query-syntax feature (e.g. new PromQL/SQL clause) → test that + existing related behavior (e.g. `topk`) still works alongside it. +- Replacing or refactoring a store/backend → add a correctness test proving + it produces the same output as what it replaces, not just a benchmark. + +## 2. Keep the explanatory comment on regression tests + +If a test exists because of a specific past bug, the comment explaining +*that bug* is part of the test, not decoration. Don't strip it during +cleanup or refactor. + +**Why:** a regression test with no comment just looks like an arbitrary edge +case to the next reader — the comment is what makes it legible why this +input is tested at all, and prevents someone "simplifying" the test away +later. diff --git a/.claude/skills/code-design-review/SKILL.md b/.claude/skills/code-design-review/SKILL.md index a1ca78c..fce934f 100644 --- a/.claude/skills/code-design-review/SKILL.md +++ b/.claude/skills/code-design-review/SKILL.md @@ -11,97 +11,5 @@ description: > # ASAPQuery code design checklist -Apply these when writing code, not just at review time. Catching these before the PR exists is cheaper than fixing them after. - -## 1. Fail loud - -Don't let a failure path quietly resolve to a default, an empty value, or a -swallowed error. If something goes wrong, panic, return `Result`/`Error`, or -raise — never fall through to a value that looks like success. - -**Why:** a hardcoded fallback or `unwrap_or_default()` on a failure path -turns a real bug into silent wrong output. It's much harder to find a bug -that never surfaces than one that panics immediately. - -❌ `kll_to_msgpack` returning an empty `Vec` on serialization failure — -looks like "no data" to every caller, not "this broke." - -✅ Same failure path in the CMS serializer: panics, so the break is visible -at the call site instead of corrupting output downstream. - -Applies to: error branches in Rust (`unwrap_or`, `unwrap_or_default`, -`.ok()` discarding an `Err`), Python `except: pass` / bare fallback returns, -and config resolution that silently picks a default when a required value is -missing. - -## 2. No magic numbers or strings, one source of truth for each - -Pull literal numbers (timeouts, poll intervals, thresholds) and strings into named constants, and don't let the same default get set in more than one place. - -**Why:** an inline `30` or `300` in one file, and the same 30 hardcoded -again three files over, are the same bug waiting to happen twice — someone -changes one and not the other, and now the values silently disagree. This is -the same failure mode whether it's a bare literal or a duplicated default -value, so treat them as one problem: give the value exactly one home. - -❌ Timeout/poll literals (`30`, `60`, `300`, `10`, `5`, `2`, `0.5s`) -scattered inline across several files in one module, while the rest of the -module uses named interval constants — breaks the existing pattern. - -❌ A default sketch backend set as a fallback argument in multiple -functions instead of one global/config value. - -✅ One named constant (or one config-file entry with no inline fallback -default) that every caller references. - -## 3. Names carry the truth about the value - -A name should precisely describe what it holds or does — including units for -anything time-like (`_MS`, `_SECS`) — and must be renamed the moment its -behavior changes underneath it. - -**Why:** "seconds or milliseconds?" is a question that shouldn't need to be -asked; the field name should already answer it. A stale name (a "clone" -helper that now moves, a "pieces" builder that now also does -post-processing) actively misleads the next reader into wrong assumptions. - -❌ `impl_clone_accumulator_methods` macro that now also generates -`into_accumulator`, an explicit move — the name promises cloning it doesn't -do. - -❌ `KEY_SLIDE_INTERVAL` / `KEY_WINDOW_SIZE` holding millisecond values with -no `_MS` suffix, inconsistent with the rest of the same PR's naming. - -✅ Rename on every semantic change, even mid-refactor, even if it touches -more call sites — don't defer it to "later." - -## 4. No half-built abstractions, no duplicated logic - -Either finish an abstraction's boundary (all callers go through it) or -delete it — don't leave a shim that some callers bypass. Before writing new -logic, check whether an existing helper/module already does it. - -**Why:** a half-finished shim (e.g. isolating an internal type, but callers -still reach through it directly) gives the illusion of a stable boundary -while every upstream change still breaks N call sites — worse than no -abstraction, because it hides the real blast radius. Duplicated logic (a -poll-with-retry loop, a query-string decomposer) drifts the moment one copy -gets fixed and the other doesn't. - -❌ `output/mod.rs` declaring three submodules that don't exist on disk — an -abandoned mid-refactor stub left in the tree. - -❌ A service-readiness poll loop reimplemented locally when -`DockerServiceBase._wait_for_service_ready` already provides it. - -✅ Either route every caller through the shim/abstraction, or delete it and -let callers use the underlying thing directly. Either way, grep for existing -helpers before writing a new poll/dedup/parse routine. - -## Also check: internal consistency - -Within one file or module, pick one convention (error type, dedup structure, -timeout threading) and use it everywhere in that file — don't mix -`Vec::contains` dedup in one function and `IndexSet` in the next function of -the same file, or thread a `timeout` param into one call and hardcode a -different value in the next. +Canonical content lives in `.agents/skills/code-design-review.md` (shared +across agents, not just Claude Code) — read that file and apply it. diff --git a/.claude/skills/test-coverage-review/SKILL.md b/.claude/skills/test-coverage-review/SKILL.md index fe23410..ef090db 100644 --- a/.claude/skills/test-coverage-review/SKILL.md +++ b/.claude/skills/test-coverage-review/SKILL.md @@ -9,32 +9,5 @@ description: > # ASAPQuery test coverage checklist -## 1. Cover the negative/failure paths, not just the happy path - -New behavior — especially error branches, merge/fallback logic, and -edge-case query syntax — needs a dedicated test for what happens when it -fails, not just a test that the normal case works. - -**Why:** Failure modes are most likely to surface in production, and tests that only check happy paths won't catch these. -A missing correctness/regression test also means there's no -guard against a future refactor quietly changing behavior (e.g. a new store -implementation silently diverging from the one it replaces). - -Checklist when adding a feature or fix: -- New merge/combine/accumulator logic → test the error path, not just the - success path. -- A new query-syntax feature (e.g. new PromQL/SQL clause) → test that - existing related behavior (e.g. `topk`) still works alongside it. -- Replacing or refactoring a store/backend → add a correctness test proving - it produces the same output as what it replaces, not just a benchmark. - -## 2. Keep the explanatory comment on regression tests - -If a test exists because of a specific past bug, the comment explaining -*that bug* is part of the test, not decoration. Don't strip it during -cleanup or refactor. - -**Why:** a regression test with no comment just looks like an arbitrary edge -case to the next reader — the comment is what makes it legible why this -input is tested at all, and prevents someone "simplifying" the test away -later. +Canonical content lives in `.agents/skills/test-coverage-review.md` (shared +across agents, not just Claude Code) — read that file and apply it. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..2e7457c --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,17 @@ +# Process conventions + +- **Keep PRs small and single-purpose.** Split a PR that mixes multiple + objectives (e.g. a rename + unrelated behavior change) into separate PRs so + the repo stays runnable and reviewable at every merge point. +- **PR titles follow Conventional Commits**: `(): `, + e.g. `fix(precompute): ...`, `feat(query-engine): ...`, + `refactor(asap-tools): ...`, `perf(precompute): ...`. Match the existing + scope names used in this repo's PR history (`precompute`, `query-engine`, + `planner`, `asap-tools`, `sql-parser`, `deps`, etc.) rather than inventing + new ones. + +# Code design and test coverage + +See `.agents/skills/code-design-review.md` and +`.agents/skills/test-coverage-review.md` — apply them while writing code, +not just when asked to review. diff --git a/CLAUDE.md b/CLAUDE.md index 26f0e2b..76c54b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,16 +1,4 @@ -# Process conventions - -- **Keep PRs small and single-purpose.** Split a PR that mixes multiple - objectives (e.g. a rename + unrelated behavior change) into separate PRs so - the repo stays runnable and reviewable at every merge point. -- **PR titles follow Conventional Commits**: `(): `, - e.g. `fix(precompute): ...`, `feat(query-engine): ...`, - `refactor(asap-tools): ...`, `perf(precompute): ...`. Match the existing - scope names used in this repo's PR history (`precompute`, `query-engine`, - `planner`, `asap-tools`, `sql-parser`, `deps`, etc.) rather than inventing - new ones. - -# Code design and test coverage - -See the `code-design-review` and `test-coverage-review` skills — apply them -while writing code, not just when asked to review. +See `AGENTS.md` for process conventions and the code design / test coverage +checklists — it applies to Claude Code the same as any other agent. The +`code-design-review` and `test-coverage-review` skills below wrap the same +checklists for Claude Code's auto-loading.