docs(08): research phase test and constant cleanup

2026-03-27 08:46:43 +01:00
parent 5a7855f3ff
commit ceba57ed0e
1 changed files with 338 additions and 0 deletions
@@ -0,0 +1,338 @@
+# Phase 8: Test and Constant Cleanup - Research
+
+**Researched:** 2026-03-27
+**Domain:** Go test cleanup, dead code removal, test assertion generalization
+**Confidence:** HIGH
+
+<user_constraints>
+## User Constraints (from CONTEXT.md)
+
+### Locked Decisions
+
+- **D-01:** Delete `NumLayers` and `GainPerLayer` constants entirely from `synth/config.go`. They are dead code — `NewBank` already computes `gainPerLayer` dynamically as `1.0 / float64(len(cfgs))` (bank.go:21). No external callers reference either constant outside the test file.
+- **D-02:** Replace the hardcoded `[60, 1100]` bounds in `TestFrequenciesInRange` with dynamic validation — derive the valid range from the `ClassFreqConfigs` data itself (e.g., check that all frequencies are positive and below Nyquist) rather than hardcoding a new magic number that would need manual updating when Phase 9/10 add classes above 1100 Hz.
+- **D-03:** Rename `TestNumLayersMatchesAllClasses` to `TestClassFreqConfigsMatchAllClasses` (or similar) to reflect the actual invariant being tested after `NumLayers` removal.
+
+### Claude's Discretion
+
+- Whether to use a generous static upper bound vs a computed Nyquist-based bound for D-02 — either approach satisfies the constraint
+- Whether `WhisperFloor` or other constants in config.go need any adjustment (they don't reference NumLayers, so likely no)
+- Whether `TestClassFreqConfigsComplete` (line 53) should be consolidated with the renamed test since both verify the same invariant
+
+### Deferred Ideas (OUT OF SCOPE)
+
+- Adding protocols (IMAP, POP3, SNMP, FTP, etc.) — this is Phase 10 scope; Phase 8 only removes blockers
+</user_constraints>
+
+<phase_requirements>
+## Phase Requirements
+
+| ID | Description | Research Support |
+|----|-------------|------------------|
+| CLEAN-01 | Remove stale `NumLayers` constant and hardcoded frequency range test assertions that would block new class additions | D-01 removes the constants; D-02 replaces hardcoded bounds with future-proof validation; D-03 renames stale test function |
+</phase_requirements>
+
+---
+
+## Summary
+
+Phase 8 is a pure cleanup phase with two narrowly scoped targets: (1) two dead exported constants in `synth/config.go` and (2) three test functions in `synth/config_test.go` that need renaming or rewiring. No new features, no new packages, no new dependencies.
+
+`NumLayers = 14` and `GainPerLayer = 1.0 / float64(NumLayers)` in `synth/config.go` are provably dead code. `synth/bank.go:NewBank` computes `gainPerLayer` dynamically at line 21 as `1.0 / float64(len(cfgs))`. Neither constant is referenced anywhere in the production code path — only in the test file's function name `TestNumLayersMatchesAllClasses` (which itself does not use either constant in its body). Deleting both constants removes a misleading signal and eliminates the risk of future callers accidentally hardcoding the stale count 14.
+
+`TestFrequenciesInRange` asserts `cfg.BaseHz < 60 || cfg.BaseHz > 1100`. Phase 9 will redistribute frequencies and Phase 10 will add classes whose frequencies will exceed 1100 Hz. The test will produce false failures the moment any `ClassFreqConfigs` entry above 1100 Hz is added. Replacing the hardcoded upper bound with a Nyquist-based check (or a generous static bound like 8000 Hz) makes the test structurally future-proof without encoding new domain knowledge in this phase.
+
+**Primary recommendation:** Three surgical edits to two files — delete 2 lines in `config.go`, update 1 test function body + rename 2 test functions in `config_test.go`. Total change surface is under 15 lines.
+
+---
+
+## Standard Stack
+
+No new dependencies. This phase touches only existing Go source files.
+
+### Core
+| Library | Version | Purpose | Why Standard |
+|---------|---------|---------|--------------|
+| `testing` | stdlib | Test assertions | Already used throughout the codebase |
+
+**Installation:** None required — no new packages.
+
+---
+
+## Architecture Patterns
+
+### Files Modified (exhaustive list)
+
+```
+synth/
+├── config.go       # Delete NumLayers and GainPerLayer constants (lines 9-10)
+└── config_test.go  # Update TestFrequenciesInRange body; rename two test functions
+```
+
+No other files are modified. The CONTEXT.md explicitly states: "Only `synth/config.go` and `synth/config_test.go` are modified — no downstream package changes expected."
+
+### Pattern 1: Dead Constant Removal
+
+**What:** Delete lines 9-10 from `synth/config.go`.
+
+**Current state:**
+```go
+// synth/config.go lines 5-12
+const (
+    SampleRate       = 44100
+    WindowMs         = 500
+    SamplesPerWindow = SampleRate * WindowMs / 1000   // 22050
+    NumLayers        = 14
+    GainPerLayer     = 1.0 / float64(NumLayers)       // D-10: ~0.0714
+    WhisperFloor     = 0.03
+)
+```
+
+**After deletion:**
+```go
+const (
+    SampleRate       = 44100
+    WindowMs         = 500
+    SamplesPerWindow = SampleRate * WindowMs / 1000   // 22050
+    WhisperFloor     = 0.03
+)
+```
+
+**Verification:** `grep -r "NumLayers\|GainPerLayer" .` must return zero hits in `*.go` files after deletion. The only non-test reference is in `.planning/research/ARCHITECTURE.md` (planning docs — not compiled).
+
+### Pattern 2: Nyquist-Based Frequency Range Validation
+
+**What:** Replace the hardcoded `[60, 1100]` upper bound in `TestFrequenciesInRange`.
+
+**Recommended approach (Nyquist-based):** Phase 9 will add classes up to ~4000 Hz. Nyquist at 44100 Hz sample rate is 22050 Hz. A Nyquist check is mathematically correct and never needs updating regardless of how many new classes are added:
+
+```go
+func TestFrequenciesInRange(t *testing.T) {
+    const nyquist = float64(synth.SampleRate) / 2.0  // 22050 Hz
+    for class, cfg := range synth.ClassFreqConfigs {
+        if cfg.BaseHz <= 0 {
+            t.Errorf("class %q BaseHz=%.1f must be positive", class, cfg.BaseHz)
+        }
+        if cfg.BaseHz >= nyquist {
+            t.Errorf("class %q BaseHz=%.1f exceeds Nyquist (%.1f Hz)", class, cfg.BaseHz, nyquist)
+        }
+    }
+}
+```
+
+**Alternative approach (generous static bound):** A bound of `[20, 8000]` also satisfies D-02's constraint since all planned Phase 9/10 frequencies are under 4000 Hz. However the Nyquist approach is self-documenting — it explains *why* there's an upper bound rather than encoding an arbitrary number. Either is acceptable per Claude's Discretion.
+
+**Key invariant preserved:** Both approaches ensure adding a new class at any humanly-audible frequency (20 Hz – 20 kHz, well within Nyquist) will NOT require editing this test.
+
+### Pattern 3: Test Function Rename
+
+**What:** Rename `TestNumLayersMatchesAllClasses` at line 61. The test body already tests the correct invariant (`len(synth.ClassFreqConfigs) == len(classify.AllClasses())`); only the name is stale.
+
+**Current:**
+```go
+func TestNumLayersMatchesAllClasses(t *testing.T) {
+    if len(synth.ClassFreqConfigs) != len(classify.AllClasses()) {
+        t.Errorf("ClassFreqConfigs has %d entries but AllClasses() has %d entries",
+            len(synth.ClassFreqConfigs), len(classify.AllClasses()))
+    }
+}
+```
+
+**After rename:**
+```go
+func TestClassFreqConfigsMatchAllClasses(t *testing.T) {
+    if len(synth.ClassFreqConfigs) != len(classify.AllClasses()) {
+        t.Errorf("ClassFreqConfigs has %d entries but AllClasses() has %d entries",
+            len(synth.ClassFreqConfigs), len(classify.AllClasses()))
+    }
+}
+```
+
+### Pattern 4: Consolidation Decision (Claude's Discretion)
+
+`TestAllClassesHaveConfig` (lines 10-16) and `TestClassFreqConfigsComplete` (lines 53-59) test the same invariant: every class in `AllClasses()` has an entry in `ClassFreqConfigs`. They are exact duplicates in semantics (different error messages but identical logic). The renamed `TestClassFreqConfigsMatchAllClasses` (formerly `TestNumLayersMatchesAllClasses`) tests the converse: lengths match.
+
+**Recommendation:** Remove `TestClassFreqConfigsComplete` (lines 53-59) as a duplicate of `TestAllClassesHaveConfig`. This leaves three non-overlapping coverage tests:
+- `TestAllClassesHaveConfig` — every AllClasses() member has a map entry
+- `TestClassFreqConfigsMatchAllClasses` — count parity (catches extra entries not in AllClasses)
+- `TestFrequenciesInRange` — all BaseHz values are positive and below Nyquist
+
+Alternatively, leave both functions if deduplication is not worth the discussion. Both pass and both protect the invariant. This is truly Claude's discretion.
+
+### Anti-Patterns to Avoid
+
+- **Updating `NumLayers` instead of deleting it:** The decision (D-01) is deletion, not update. An updated constant would still be a maintenance burden.
+- **Replacing [60, 1100] with [60, 4000]:** A new hardcoded number has the same fragility as the old one — it becomes stale when the frequency spectrum changes again in a future milestone.
+- **Touching `bank.go`:** The dynamic `gainPerLayer` computation in `bank.go` is already correct. No changes needed.
+- **Touching `classify/types.go`:** `AllClasses()` is not modified in this phase.
+- **Touching `WhisperFloor`:** It does not reference `NumLayers` or `GainPerLayer`; leave it unchanged.
+
+---
+
+## Don't Hand-Roll
+
+Not applicable. This phase contains no algorithmic code — it is deletion and test rewriting.
+
+---
+
+## Common Pitfalls
+
+### Pitfall 1: Leaving the `GainPerLayer` comment reference orphaned
+
+**What goes wrong:** `GainPerLayer` at config.go line 10 has a comment `// D-10: ~0.0714`. After deletion, the decision reference D-10 disappears from the source. This is fine — D-10 is still documented in the planning research files. But if the comment is moved to `bank.go` line 21 (where the dynamic computation lives), it improves traceability without leaving an orphan.
+
+**How to avoid:** Either delete both lines cleanly with no compensation, or add `// D-10: gain is 1/N computed dynamically` to `bank.go:21`. Both are acceptable.
+
+**Warning signs:** Go compiler catches unused constants — if `NumLayers` or `GainPerLayer` are deleted and the code still compiles, they were indeed dead.
+
+### Pitfall 2: Using `synth.SampleRate` in the test without verifying the export
+
+**What goes wrong:** `SampleRate` is an exported constant in `synth/config.go`. The test file is in package `synth_test` (external test package), so it accesses `synth.SampleRate`. Verify `SampleRate` is exported (capital S) before referencing it from the test.
+
+**How to avoid:** Already confirmed — `SampleRate = 44100` is exported at config.go line 6. No issue.
+
+**Warning signs:** Compiler error `synth.sampleRate undefined` would indicate a lowercase constant.
+
+### Pitfall 3: Test duplication confusion
+
+**What goes wrong:** `TestAllClassesHaveConfig` and `TestClassFreqConfigsComplete` look different but test the same invariant. During code review or future debugging, someone might wonder why there are two tests for the same thing.
+
+**How to avoid:** If consolidating (removing `TestClassFreqConfigsComplete`), add a comment to `TestAllClassesHaveConfig` noting it replaced the duplicate. If not consolidating, no action needed.
+
+---
+
+## Code Examples
+
+### Resulting `synth/config.go` constant block
+
+```go
+// Source: synth/config.go — after Phase 8 cleanup
+const (
+    SampleRate       = 44100
+    WindowMs         = 500
+    SamplesPerWindow = SampleRate * WindowMs / 1000 // 22050
+    WhisperFloor     = 0.03                         // D-08/D-09: 3% of max amplitude
+)
+```
+
+### Resulting `TestFrequenciesInRange` (Nyquist approach)
+
+```go
+// Source: synth/config_test.go — after Phase 8 cleanup
+func TestFrequenciesInRange(t *testing.T) {
+    const nyquist = float64(synth.SampleRate) / 2.0
+    for class, cfg := range synth.ClassFreqConfigs {
+        if cfg.BaseHz <= 0 {
+            t.Errorf("class %q BaseHz=%.1f must be positive", class, cfg.BaseHz)
+        }
+        if cfg.BaseHz >= nyquist {
+            t.Errorf("class %q BaseHz=%.1f exceeds Nyquist (%.1f Hz)", class, cfg.BaseHz, nyquist)
+        }
+    }
+}
+```
+
+### Resulting `TestClassFreqConfigsMatchAllClasses`
+
+```go
+// Source: synth/config_test.go — after rename from TestNumLayersMatchesAllClasses
+func TestClassFreqConfigsMatchAllClasses(t *testing.T) {
+    if len(synth.ClassFreqConfigs) != len(classify.AllClasses()) {
+        t.Errorf("ClassFreqConfigs has %d entries but AllClasses() has %d entries",
+            len(synth.ClassFreqConfigs), len(classify.AllClasses()))
+    }
+}
+```
+
+---
+
+## State of the Art
+
+| Old Approach | Current Approach | When Changed | Impact |
+|--------------|------------------|--------------|--------|
+| `NumLayers = 14` static constant | Dynamic `1.0 / float64(len(cfgs))` in `NewBank` | Phase 6/7 (v1.1) | Static constant is now dead code; remove it |
+| `TestFrequenciesInRange` checks `[60, 1100]` | Nyquist-based check (this phase) | Phase 8 (v1.2) | Test survives any future frequency allocation |
+
+---
+
+## Validation Architecture
+
+### Test Framework
+
+| Property | Value |
+|----------|-------|
+| Framework | `testing` stdlib, Go 1.24 |
+| Config file | none (standard `go test`) |
+| Quick run command | `go test ./synth/...` |
+| Full suite command | `go test ./...` |
+
+### Phase Requirements → Test Map
+
+| Req ID | Behavior | Test Type | Automated Command | File Exists? |
+|--------|----------|-----------|-------------------|-------------|
+| CLEAN-01 (constant removal) | `NumLayers` and `GainPerLayer` are not exported from `synth` package | unit — compile check | `go build ./synth/...` | ✅ (config.go exists; delete lines) |
+| CLEAN-01 (no broken references) | Full test suite passes after deletion | integration | `go test ./...` | ✅ |
+| CLEAN-01 (range test future-proof) | `TestFrequenciesInRange` passes with any BaseHz in (0, Nyquist) range | unit | `go test ./synth/... -run TestFrequenciesInRange` | ✅ (config_test.go exists; update body) |
+| CLEAN-01 (test rename) | `TestClassFreqConfigsMatchAllClasses` exists and passes | unit | `go test ./synth/... -run TestClassFreqConfigsMatchAllClasses` | ✅ (rename existing function) |
+
+### Sampling Rate
+
+- **Per task commit:** `go test ./synth/...`
+- **Per wave merge:** `go test ./...`
+- **Phase gate:** `go test ./...` green before `/gsd:verify-work`
+
+### Wave 0 Gaps
+
+None — existing test infrastructure covers all phase requirements. No new test files, fixtures, or framework setup needed.
+
+---
+
+## Environment Availability
+
+Step 2.6: SKIPPED (no external dependencies — pure Go source edits, no new tools or services required).
+
+Current test suite state confirmed: `go test ./...` passes on all 7 packages.
+
+---
+
+## Open Questions
+
+1. **Consolidate `TestAllClassesHaveConfig` and `TestClassFreqConfigsComplete`?**
+   - What we know: They test the same invariant; both currently pass; no correctness issue either way
+   - What's unclear: Whether the planner wants one clean authoritative test or is fine leaving both
+   - Recommendation: Remove `TestClassFreqConfigsComplete` (lines 53-59) as it duplicates `TestAllClassesHaveConfig`. The named `TestAllClassesHaveConfig` is more expressive. If this causes any concern, leave both — both are correct.
+
+2. **Add D-10 comment to `bank.go` after deleting `GainPerLayer`?**
+   - What we know: `GainPerLayer` carries `// D-10: ~0.0714`; bank.go line 21 is where the actual computation lives
+   - What's unclear: Whether the project wants decision-reference comments preserved at the implementation site
+   - Recommendation: Add `// D-10: gainPerLayer = 1/N so all N layers at full amplitude sum to 1.0` to bank.go line 21. Low-cost, improves traceability.
+
+---
+
+## Sources
+
+### Primary (HIGH confidence)
+
+- `synth/config.go` — Direct inspection: `NumLayers = 14`, `GainPerLayer = 1.0 / float64(NumLayers)` at lines 9-10; `SampleRate = 44100` at line 6
+- `synth/bank.go` — Direct inspection: `gainPerLayer: 1.0 / float64(len(cfgs))` at line 21 — confirms constants are dead code
+- `synth/config_test.go` — Direct inspection: `TestFrequenciesInRange` body at lines 18-25; `TestNumLayersMatchesAllClasses` at lines 61-66; `TestClassFreqConfigsComplete` at lines 53-59
+- `classify/types.go` — Direct inspection: `AllClasses()` returns 14 entries; `SampleRate = 44100` used for Nyquist calculation
+- `.planning/phases/08-test-and-constant-cleanup/08-CONTEXT.md` — Locked decisions D-01, D-02, D-03
+- `.planning/research/PITFALLS.md` — Pitfall C3 (NumLayers stale constant) and C4 (TestFrequenciesInRange hardcoding)
+- `.planning/research/ARCHITECTURE.md` lines 414-422 — NumLayers/GainPerLayer dead code analysis
+
+### Secondary (MEDIUM confidence)
+
+- `go test ./...` output — All 7 packages pass; current baseline confirmed
+
+---
+
+## Metadata
+
+**Confidence breakdown:**
+- Standard stack: HIGH — no new dependencies; pure stdlib
+- Architecture: HIGH — all target lines verified by direct file inspection
+- Pitfalls: HIGH — sourced from project research files and direct code inspection
+
+**Research date:** 2026-03-27
+**Valid until:** Until Phase 9 begins (frequency redistribution) — this research is tied to current `synth/config.go` line numbers which Phase 9 will change