chore: catalog magic numbers — propose policy + constants architecture #101

Summary

Several timeouts, channel buffers, and sampling rates are hard-coded as unnamed numeric literals across internal/ and cmd/. They cluster in a few categories — query/HTTP timeouts, JetStream consumer settings, observability sampling, KV history depth — and most are duplicated 2–3 times across files without being shared. This makes them hard to find, easy to drift out of sync, impossible to tune without recompiling, and dangerous to change (no one knows the rationale).

The codebase already does this well in spots — query.DefaultMaxRows, the api.ContextKey* block, policy.kvBucket/kvKey, pipes.kvBucket. We just haven't been consistent.

This issue catalogues the worst offenders, proposes where named constants should live, and drafts a contributor-policy update to keep new code from re-introducing the pattern.

Egregious examples

All citations are against main as of writing.

1. Duplicated 30-second query timeout (3 sites)

The same hard-coded 30s ClickHouse query timeout appears in three handlers with no shared definition. If we tune one, the others drift silently.

• internal/api/query.go:87 — context.WithTimeout(r.Context(), 30*time.Second)
• internal/api/structured_query.go:129 — timeout := 30 * time.Second (with a per-role override path, but the default lives nowhere reusable)
• internal/api/pipes.go:139 — context.WithTimeout(r.Context(), 30*time.Second)

Why bad: Three places to remember, no rationale captured, no operator knob. The structured-query path already accepts a per-role MaxExecutionTimeMs override — the default should be the named anchor that the override falls back to.

2. Duplicated 10-second ReadHeaderTimeout (2 binaries)

• cmd/wavehouse/main.go:275 — ReadHeaderTimeout: 10 * time.Second
• cmd/wavehouse-api/main.go:258 — ReadHeaderTimeout: 10 * time.Second

Why bad: Slowloris-mitigation knob duplicated across both binaries. Should be one named constant (or, since cfg.Server.ShutdownTimeout is already a config field, this could plausibly be one too).

3. JetStream consumer tuning duplicated across SSE + WS

• internal/api/stream_sse.go:179 — InactiveThreshold: 5 * time.Second
• internal/api/stream_sse.go:186 — jetstream.FetchMaxWait(500 * time.Millisecond)
• internal/api/stream_ws.go:258 — InactiveThreshold: 5 * time.Second
• internal/api/stream_ws.go:267 — jetstream.FetchMaxWait(500 * time.Millisecond)

Why bad: Both stream handlers gap-fill from NATS the same way. The thresholds are an implicit contract between the two — if a future tweak only updates one, the streams diverge.

4. Stream subscriber channel buffer with an unresolved TODO

• internal/api/stream_sse.go:51 — ch := make(chan []byte, 64) // TODO: need to test how many are actually needed, as this is ~1.6KB per subscriber channel...
• internal/api/stream_ws.go:72 — make(chan []byte, 64)
• internal/api/stream_ws.go:84 — make(chan []byte, 64)

Why bad: TODO admits this value was a guess, but it's now hard-coded in three places. At ~1.6KB per channel × number-of-subscribers × 3 sites, this directly bounds memory under fan-out. Should be one named constant with a sizing-rationale comment, or made config-driven.

5. Observability sampling rates buried as inline floats

• internal/observability/provider.go:72 — trace.WithSampler(trace.TraceIDRatioBased(0.10)) (trailing // TRACE SAMPLE RATE comment, but no name)
• internal/observability/logger.go:48 — Rate: 0.1, // Keep 10% of logs

Why bad: Two independently-tuned 10% rates that almost certainly should move together (a trace without correlated logs is much less useful). Neither is reachable from config — operators can't dial sampling up during an incident without redeploying.

6. Observability batch/export intervals

• internal/observability/provider.go:70 — trace.WithBatchTimeout(time.Second*5)
• internal/observability/provider.go:87 — metric.WithInterval(15*time.Second)
• internal/observability/provider.go:93 — runtime.WithMinimumReadMemStatsInterval(15 * time.Second)

Why bad: Two 15*time.Second values that look coincidentally identical — is the intent that runtime memstats line up with the metric export tick, or did two people pick "15s feels fine" independently? A named constant makes the intent explicit.

7. Ingest pipeline tickers

• internal/ingest/bento.go:115 — time.NewTicker(10 * time.Millisecond) (busy-loop while draining in-flight inserts before a DELETE)
• internal/ingest/bento.go:244 — context.WithTimeout(ctx, 10*time.Second) (Bento setup timeout)
• internal/ingest/sweeper.go:40 — time.NewTicker(1 * time.Minute) (NATS message GC interval)

Why bad: The 10ms drain-poll is on the critical path of every DELETE and silently sets a floor on delete latency. The sweep interval is documented in AGENTS.md as design-relevant (gap-fill semantics) but lives as a bare 1 * time.Minute. Both deserve named constants with rationale comments.

8. KV History depth — inconsistent between buckets

• internal/pipes/pipes.go:51 — History: 3
• internal/policy/store.go:35 — History: 5

Why bad: Two NATS KV buckets, two different history depths, no rationale for either. Is policy more audit-sensitive than pipes? Maybe — but the code doesn't say so, and the next contributor adding a third KV bucket has no anchor for what to pick.

What we already do well

Cite these as the target pattern:

• internal/query/builder.go:18 — const DefaultMaxRows = 10000 with a comment explaining the invariant it protects.
• internal/api/middleware.go:15-20 — grouped const block with doc comments for context keys.
• internal/policy/store.go:17-19 and internal/pipes/pipes.go:19 — bucket/key strings extracted to constants (the bucket name is part of the on-disk contract — leaving it inline would be a footgun).
• internal/config/config.go — operator-tunable knobs all flow through one struct with yaml/env/env-default tags.

The pattern is already there. We just haven't applied it uniformly.

Proposed organizational architecture

Three tiers, picked by who tunes the value and on what cadence:

Decision rule for reviewers: if a numeric literal isn't 0, 1, -1, or used purely as an index/length in trivial arithmetic, it should either be a named constant or be loaded from config. A gocritic-style threshold of "≥ 2 same-typed literals" is fine in practice.

Where rationale lives: in a // comment immediately above the constant. The comment must answer "why this value?" — citing the trade-off, the bound it enforces, or the spec it follows. "Why 30s? Because ClickHouse default network timeout is 60s and we want to fail before the connection drops" is the bar.

Proposed contributor-policy update

Add a new section to AGENTS.md (with a one-paragraph mirror in CONTRIBUTING.md under Code Style):

Magic numbers

Don't embed bare numeric literals in code. Every non-trivial number — timeouts, buffer/batch sizes, byte counts, retry/limit thresholds, sampling rates, ports, byte shifts — must be either:

1. A named const in the same package, with a comment explaining why this value, not just what it is; or
2. A config field in internal/config/config.go with yaml/env/env-default tags, if an operator should be able to tune it at deploy time.

Use config (tier 1) for anything that depends on deployment shape, traffic, or hardware. Use a package-local const (tier 2) for internal invariants. If the same value appears in two or more packages, lift it into the consumer's package or internal/config/ — don't copy the literal.

Bare literals exempt from this rule: 0, 1, -1, array/slice indices, and trivial arithmetic factors (* 2, + 1). HTTP status codes must use net/http constants (http.StatusBadRequest, etc.) — never raw 400/500.

Reviewers (human and AI): flag any new bare literal in a PR diff that doesn't fall under the exemptions. Existing literals are tech debt tracked in [#TBD]; new ones are a review blocker.

Also update .gemini/styleguide.md and the Claude review prompt to call this out explicitly so the AI reviewers flag it.

Suggested follow-up (phasing)

Filed as one issue but the cleanup should be split into reviewable chunks:

1. Policy first — land the AGENTS.md / CONTRIBUTING.md change and the reviewer prompt updates so new code stops adding to the pile.
2. Tier-3 consolidation — extract the duplicated values (query timeout × 3, ReadHeaderTimeout × 2, JetStream consumer settings × 2, channel buffer size × 3) into config or shared package-local constants. This is the highest leverage and biggest correctness win.
3. Tier-1 promotion — move observability sampling rates and the query timeout default into internal/config/config.go so we can tune them at deploy time.
4. Tier-2 cleanup — add named constants + rationale comments for the remaining single-site cases (bento.go tickers, sweeper interval, KV History depths).

Each step is a separate PR.

cc reviewers when the policy PR lands so it gets explicit sign-off — this changes the bar for every future PR.