From ceba57ed0e23e83600f85c689d9c0dcea0a30d4b Mon Sep 17 00:00:00 2001 From: Markus Graf Date: Fri, 27 Mar 2026 08:46:43 +0100 Subject: [PATCH] docs(08): research phase test and constant cleanup --- .../08-RESEARCH.md | 338 ++++++++++++++++++ 1 file changed, 338 insertions(+) create mode 100644 .planning/phases/08-test-and-constant-cleanup/08-RESEARCH.md diff --git a/.planning/phases/08-test-and-constant-cleanup/08-RESEARCH.md b/.planning/phases/08-test-and-constant-cleanup/08-RESEARCH.md new file mode 100644 index 0000000..3a5205d --- /dev/null +++ b/.planning/phases/08-test-and-constant-cleanup/08-RESEARCH.md @@ -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 (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 + + + +## 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 | + + +--- + +## 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