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.