yoloyolo/.planning/milestones/v1.2-phases/08-test-and-constant-cleanup/08-RESEARCH.md

# 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