feat(skills): add code-design-review and test-coverage-review skills

.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<u8>` 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.

.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.

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**: `<type>(<scope>): <summary>`,
+  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.