docs: complete project research
This commit is contained in:
+427
-385
@@ -1,511 +1,553 @@
|
||||
# Pitfalls Research
|
||||
# Domain Pitfalls
|
||||
|
||||
**Domain:** Network-traffic-to-audio synthesis CLI tool (Go)
|
||||
**Researched:** 2026-03-26 (v1.1 update — TOML config, waveform types, user-defined rules)
|
||||
**Confidence:** HIGH (TOML decoder behaviors verified against pkg.go.dev official docs and issue trackers; audio synthesis aliasing verified against DSP literature; config merging verified against BurntSushi/toml issue #47 and go-toml issue #252)
|
||||
**Domain:** NetSynth — Network traffic sonification CLI tool (Go)
|
||||
**Researched:** 2026-03-26 (v1.1 original); 2026-03-27 (v1.2 update — extended protocol coverage, grouped sound families)
|
||||
**Confidence:** HIGH (all pitfalls grounded in direct codebase inspection; audio masking values from Glasberg & Moore 1990 ERB model; TOML behaviors from BurntSushi docs and issue history; Go performance from first-principles analysis of existing code)
|
||||
|
||||
---
|
||||
|
||||
## v1.1 Milestone Pitfalls (New)
|
||||
## v1.2 Milestone Pitfalls (New)
|
||||
|
||||
These pitfalls are specific to adding TOML config, waveform types, and user-defined classification rules to the existing NetSynth codebase.
|
||||
These pitfalls are specific to adding expanded protocol classification with grouped sound families to the existing NetSynth v1.1 codebase. They are ordered by severity: critical pitfalls cause incorrect output or broken configs without obvious errors; moderate pitfalls degrade audio quality or developer experience; minor pitfalls are friction points with clear workarounds.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A1: TOML Unmarshal Silently Overwrites Pre-filled Defaults with Zero Values
|
||||
## Critical Pitfalls
|
||||
|
||||
### Pitfall C1: Frequency Rebalancing Silently Invalidates User v1.1 Configs
|
||||
|
||||
**What goes wrong:**
|
||||
You initialize a `Config` struct with built-in defaults, then call `toml.Unmarshal` to layer in user overrides. Any field the user *omits* from their TOML file is set to its Go zero value (`0`, `""`, `false`, `nil`) by the decoder — overwriting your defaults. A user who writes only `[sounds.DNS]` in their config file to change the DNS tone ends up wiping every other class back to zero Hz.
|
||||
Users who created a `netsynth.toml` under v1.1 specified absolute Hz values for built-in classes — for example `[sounds.HTTPS] frequency = 175.0`. If v1.2 rebalances that class to a new default (say 340 Hz to make room for new protocols), the user's config now overrides the new default back to the old v1.1 value (175 Hz). The user gets a v1.1 sound for HTTPS even after upgrading, with no warning that their override value has become stale relative to the rebalanced layout.
|
||||
|
||||
The reverse is also possible: a user who did NOT set a frequency override had HTTPS at 175 Hz; after rebalancing it moves silently to a different Hz value. Their soundscape has changed without explanation.
|
||||
|
||||
**Why it happens:**
|
||||
Both `BurntSushi/toml` and `pelletier/go-toml` v1 do not distinguish between "key was absent" and "key was explicitly set to zero". The decoder reflects over the struct and writes zero for every absent key. This was explicitly reported as a bug in BurntSushi/toml issue #47 and go-toml issue #252. go-toml v2 partially addresses it but still zeros primitive-type fields that are absent.
|
||||
The `merge()` function in `config/config.go` applies any `[sounds.X] frequency = Y` from the user's TOML unconditionally. There is no concept of "this override is relative to a previous default" — it is applied as a fixed Hz value. Changing the default in `ClassFreqConfigs` does not trigger any validation that existing user overrides remain intentional.
|
||||
|
||||
**Consequences:**
|
||||
- All non-overridden traffic classes play silence (0 Hz oscillator)
|
||||
- Classification rules get zeroed if user only partially fills `[[rules]]`
|
||||
- EMA tau, whisper floor, gain, and other synth parameters reset to 0
|
||||
- v1.1 user configs produce different-than-expected audio on v1.2 without any error or warning
|
||||
- Users with explicit overrides are stuck at v1.1 frequency values — the rebalancing has zero effect for them
|
||||
- Users without overrides hear an unexplained soundscape change after upgrade
|
||||
|
||||
**Prevention:**
|
||||
Use pointer fields (`*float64`, `*string`) in the decoded struct to distinguish "not provided" (nil pointer) from "explicitly set to zero" (non-nil pointer to 0). Apply a merge step: iterate over the decoded struct, and for each pointer field that is nil, keep the built-in default. For slice fields (like `[]RuleConfig`), nil slice means "user did not provide rules" — preserve defaults; non-nil empty slice (`[]RuleConfig{}`) means "user explicitly cleared rules" — respect that.
|
||||
Two complementary strategies:
|
||||
|
||||
```go
|
||||
// In config struct, use pointers for optional overrides:
|
||||
type SoundConfig struct {
|
||||
FreqHz *float64 `toml:"freq_hz"`
|
||||
Waveform *string `toml:"waveform"`
|
||||
}
|
||||
1. **Minimize rebalancing scope.** Assign new protocol classes to frequency ranges not yet occupied by v1.1 built-ins. The current v1.1 built-in range is 65–780 Hz (known protocols) and 862–1047 Hz (unknown buckets), with a gap at 781–861 Hz. New families can be allocated into ranges above 1100 Hz (e.g., 1100–4000 Hz), leaving all existing Hz assignments untouched. This eliminates the backward-compatibility problem entirely for users who have not overridden values in that range.
|
||||
|
||||
// Merge: for each class, override only non-nil fields
|
||||
func mergeSound(base synth.FreqConfig, override SoundConfig) synth.FreqConfig {
|
||||
if override.FreqHz != nil {
|
||||
base.BaseHz = *override.FreqHz
|
||||
}
|
||||
if override.Waveform != nil {
|
||||
base.WaveformType = *override.Waveform
|
||||
}
|
||||
return base
|
||||
}
|
||||
```
|
||||
2. **Changelog + --print-config.** If rebalancing IS required, document every changed Hz value in the release notes and update `--print-config` so users can diff their effective config against what they saved. Add a comment to `--print-config` output when a user override matches a value that was the v1.1 default (warn that it may be stale).
|
||||
|
||||
**Detection:**
|
||||
- User reports that classes they did not configure now produce no sound
|
||||
- Unit test: load a config that overrides only one class; verify all other classes retain built-in Hz values
|
||||
- User reports HTTPS sounds wrong after upgrade
|
||||
- `--print-config` shows `(override)` for a class the user never intentionally customized — they set it once to the default value and now that exact value is stale
|
||||
|
||||
**Phase to address:**
|
||||
Config loading phase (first phase of v1.1). Get the pointer-and-merge pattern established before wiring config into the bank. Retrofitting after the bank construction is wired is a significant churn.
|
||||
Frequency allocation design phase (first phase of v1.2). The spectrum layout must be finalized before writing any `ClassFreqConfigs` entries. Treat the 65–1047 Hz range as frozen for backward compatibility.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A2: BurntSushi/toml Silently Ignores Typos in Field Names
|
||||
### Pitfall C2: `autoAssignFreq` Range Collision With New Built-in Frequencies
|
||||
|
||||
**What goes wrong:**
|
||||
A user writes `freq_hz = 440` but the struct tag is `toml:"freq_hz"` — this works. However if the user writes `freqhz = 440` or `FreqHz = 440` or a misspelled `frek_hz = 440`, the library silently ignores the key. The user's override is never applied. No error is returned. The user thinks their config is active; it is not.
|
||||
`autoAssignFreq()` in `config/config.go` assigns custom user classes to frequencies in `[1200, 2350]` Hz using 24 steps of 50 Hz each. If new v1.2 built-in protocol classes are assigned frequencies in this same range (e.g., placing SIP at 1500 Hz or IMAP at 1200 Hz), a user's custom class may hash to the same frequency as a new built-in. The user's custom class and the new built-in will produce the same tone — the soundscape loses discriminability, and the user has no way to know their custom class has collided.
|
||||
|
||||
**Why it happens:**
|
||||
`BurntSushi/toml` by default silently discards keys that do not map to any struct field. This is the documented default behavior ("will ignore options in the TOML file that you don't use"). It is the opposite of "strict mode."
|
||||
The `[1200, 2350]` range was deliberately chosen in v1.1 as "unused 1200-2350 Hz range" (per the STACK.md annotation). If v1.2 extends built-ins into that range without also updating `autoAssignFreq`, the guarantee is broken.
|
||||
|
||||
**Consequences:**
|
||||
- Silent misconfiguration: user's customization is invisible
|
||||
- Debugging is very hard — no error to trace back to the TOML file
|
||||
- Silent frequency collision: two classes (one built-in, one user-defined) play the same tone
|
||||
- User's custom classification is perceptually indistinguishable from the colliding built-in
|
||||
- `--print-config` will show different Hz values in the TOML text, but the audio output is identical
|
||||
|
||||
**Prevention:**
|
||||
Use `toml.Decode` (not `toml.Unmarshal`) to obtain `MetaData`, then call `md.Undecoded()` and return an error listing any keys that were not decoded. This is BurntSushi's documented strict-mode pattern.
|
||||
Update `autoAssignFreq` in `config/config.go` whenever new built-in frequency ranges are allocated. Specifically:
|
||||
- After finalizing all v1.2 `ClassFreqConfigs` frequencies, compute the highest built-in Hz value
|
||||
- Set `autoAssignFreq` base above that value, e.g., `baseHz = 4500.0` with a range that is guaranteed to be clear of all built-ins
|
||||
- Add a compile-time assertion (test) that verifies no `ClassFreqConfigs` entry falls inside the auto-assign range
|
||||
|
||||
```go
|
||||
md, err := toml.Decode(string(data), &cfg)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if keys := md.Undecoded(); len(keys) > 0 {
|
||||
return fmt.Errorf("unknown config keys (check for typos): %v", keys)
|
||||
}
|
||||
```
|
||||
|
||||
**Detection:**
|
||||
- Config change that should audibly alter the sound has no effect
|
||||
- Undecoded keys present but no warning/error logged
|
||||
|
||||
**Phase to address:**
|
||||
Config loading phase. Implement strict decoding from the first config load function. Do not add this as an afterthought — it is the primary mechanism protecting users from silent misconfiguration.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A3: Naive Square/Sawtooth/Triangle Generation Produces Audible Aliasing Distortion
|
||||
|
||||
**What goes wrong:**
|
||||
Implementing waveforms by direct time-domain math — `sign(sin(phase))` for square, `2*frac(phase)-1` for sawtooth, `1-2*abs(frac(phase)-0.5)` for triangle — produces a waveform with infinite harmonics. At 44100 Hz, harmonics above 22050 Hz fold back into the audible range as aliasing. At the frequencies used in NetSynth (65–1047 Hz), aliasing from a naive square wave produces a buzzing distortion that is especially audible at higher drone frequencies and sounds like corruption rather than timbre.
|
||||
|
||||
**Why it happens:**
|
||||
The mathematical waveforms are not bandlimited — they have infinite harmonic content. Direct sampling them at 44100 Hz aliases all energy above Nyquist back into the audible band. Developers who test at low frequencies (60–120 Hz) may not notice because the aliased harmonics land at very high frequencies with low perceptual impact; the problem worsens significantly above 400 Hz where aliases fold into the 1–5 kHz perceptually prominent range.
|
||||
|
||||
**Consequences:**
|
||||
- Square/sawtooth at SSH (330 Hz) and higher frequencies sounds harsh and buzzy
|
||||
- The effect worsens at higher frequencies, making SMTP (440 Hz) and DHCP (600 Hz) drones sound distorted
|
||||
- Aliasing cannot be filtered out post-synthesis (it is interleaved with desired signal)
|
||||
|
||||
**Prevention:**
|
||||
Use additive synthesis — the approach already in use for sine waves in `oscillator.go`. The existing `Oscillator.Advance(harmonics []HarmonicDef)` computes `sin(2π * phase * ratio)` for each partial. Square, sawtooth, and triangle waveforms are all expressible as harmonic series:
|
||||
|
||||
- **Square:** odd harmonics only, amplitude `1/k` for harmonic `k`: ratios 1, 3, 5, 7, ... with amplitudes 1.0, 0.33, 0.20, 0.14, ... Truncate at Nyquist.
|
||||
- **Sawtooth:** all harmonics, amplitude `1/k`: ratios 1, 2, 3, 4, ... with amplitudes 1.0, 0.5, 0.33, 0.25, ... Truncate at Nyquist.
|
||||
- **Triangle:** odd harmonics, amplitude `1/k²`, alternating sign: ratios 1, 3, 5, ... with amplitudes 1.0, 0.11, 0.04, ... Truncate at Nyquist.
|
||||
|
||||
The truncation (only sum harmonics where `freq * ratio < sampleRate / 2`) is the critical step that makes the synthesis bandlimited. The existing `[]HarmonicDef` structure in `synth/config.go` already supports this — waveform type selection just requires generating the right harmonic series for each `FreqConfig`.
|
||||
|
||||
Waveform presets should be pre-computed `[]HarmonicDef` slices, not runtime computation of naive waveform math:
|
||||
|
||||
```go
|
||||
// BandlimitedHarmonics returns a bandlimited harmonic series for the given waveform type.
|
||||
// It truncates harmonics at Nyquist (sampleRate/2) to prevent aliasing.
|
||||
func BandlimitedHarmonics(waveform string, baseHz float64, sampleRate int) []HarmonicDef {
|
||||
nyquist := float64(sampleRate) / 2.0
|
||||
var defs []HarmonicDef
|
||||
switch waveform {
|
||||
case "square":
|
||||
for k := 1; float64(k)*baseHz < nyquist; k += 2 { // odd only
|
||||
defs = append(defs, HarmonicDef{Ratio: k, Amplitude: 1.0 / float64(k)})
|
||||
// In synth/config_test.go:
|
||||
func TestAutoAssignRangeIsEmpty(t *testing.T) {
|
||||
const autoBase = 4500.0
|
||||
const autoTop = 6000.0
|
||||
for class, cfg := range ClassFreqConfigs {
|
||||
if cfg.BaseHz >= autoBase && cfg.BaseHz <= autoTop {
|
||||
t.Errorf("built-in class %q frequency %.1f Hz falls in auto-assign range [%.0f, %.0f]",
|
||||
class, cfg.BaseHz, autoBase, autoTop)
|
||||
}
|
||||
case "sawtooth":
|
||||
for k := 1; float64(k)*baseHz < nyquist; k++ {
|
||||
defs = append(defs, HarmonicDef{Ratio: k, Amplitude: 1.0 / float64(k)})
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Detection:**
|
||||
- Two classes with different names produce identical tones
|
||||
- `--print-config` shows correct but coincidentally matching Hz values for a user class and a built-in
|
||||
|
||||
**Phase to address:**
|
||||
Frequency allocation design phase. After all new built-in Hz values are set, update `autoAssignFreq` constants and add the compile-time range check before merging.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall C3: Hardcoded `NumLayers = 14` Constant Becomes a Lie — But `GainPerLayer` Stays Wrong
|
||||
|
||||
**What goes wrong:**
|
||||
`synth/config.go` defines:
|
||||
```go
|
||||
NumLayers = 14
|
||||
GainPerLayer = 1.0 / float64(NumLayers) // 0.0714
|
||||
```
|
||||
|
||||
The current `bank.go` correctly uses `1.0 / float64(len(cfgs))` at bank construction time, so the bank itself handles more layers correctly. However, `GainPerLayer` is still exported as a package-level constant. If any code outside `bank.go` references `synth.GainPerLayer` for gain calculations (including tests, the encode pipeline, or future code added in v1.2), it will use the stale `0.0714` value even when 25+ layers are active. The bank sounds louder than expected at low class counts, or softer at high class counts, depending on which reference is used.
|
||||
|
||||
Additionally, `TestNumLayersMatchesAllClasses` in `synth/config_test.go` checks:
|
||||
```go
|
||||
if len(synth.ClassFreqConfigs) != len(classify.AllClasses())
|
||||
```
|
||||
This test enforces that `ClassFreqConfigs` and `AllClasses()` stay in sync. Adding new classes to one without the other causes this test to fail — which is the right behavior, but the fix is non-obvious: you must update both `ClassFreqConfigs` (with a new `FreqConfig` entry) AND `AllClasses()` (by appending to the return value). Miss either and the test blocks the build.
|
||||
|
||||
**Why it happens:**
|
||||
`NumLayers` was introduced in v1.0 when the layer count was static. It was not removed when `NewBank` was refactored to use `len(cfgs)` dynamically. The constant is now a documentation artifact that can mislead future code.
|
||||
|
||||
**Consequences:**
|
||||
- Any code added in v1.2 that references `synth.GainPerLayer` uses an incorrect value
|
||||
- Possible audio clipping (if gain is too high) or inaudibly quiet output (if computed with wrong layer count)
|
||||
- `TestNumLayersMatchesAllClasses` fails if `AllClasses()` and `ClassFreqConfigs` are updated independently
|
||||
|
||||
**Prevention:**
|
||||
At the start of v1.2 protocol rule addition:
|
||||
1. Remove `NumLayers` and `GainPerLayer` constants from `synth/config.go` (or mark them deprecated with a clear comment)
|
||||
2. Rename `TestNumLayersMatchesAllClasses` to `TestClassFreqConfigsMatchesAllClasses` and update its comment to explain the invariant
|
||||
3. When adding each new protocol class: update `AllClasses()` and `ClassFreqConfigs` atomically in the same commit — the test will catch any missed entry
|
||||
|
||||
**Detection:**
|
||||
- `TestNumLayersMatchesAllClasses` fails after adding new classes to only one of the two locations
|
||||
- Audio output is unexpectedly loud or quiet compared to previous version
|
||||
|
||||
**Phase to address:**
|
||||
First code phase of v1.2, before adding any new protocol classes. Removing the stale constant and renaming the test is a two-minute cleanup that prevents confusion throughout the milestone.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall C4: `TestFrequenciesInRange` Hardcodes `[60, 1100]` — Will Fail for New High-Frequency Classes
|
||||
|
||||
**What goes wrong:**
|
||||
`synth/config_test.go` contains:
|
||||
```go
|
||||
func TestFrequenciesInRange(t *testing.T) {
|
||||
for class, cfg := range synth.ClassFreqConfigs {
|
||||
if cfg.BaseHz < 60 || cfg.BaseHz > 1100 {
|
||||
t.Errorf("class %q BaseHz=%.1f is out of range [60, 1100]", class, cfg.BaseHz)
|
||||
}
|
||||
case "triangle":
|
||||
sign := 1.0
|
||||
for k := 1; float64(k)*baseHz < nyquist; k += 2 {
|
||||
defs = append(defs, HarmonicDef{Ratio: k, Amplitude: sign / float64(k*k)})
|
||||
sign = -sign
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
If any new v1.2 protocol class is assigned a frequency above 1100 Hz (which is necessary if new families extend into the 1100–4000 Hz range), this test fails immediately. The test was written for v1.0's 14-class spectrum. It will now become a false blocker, making every correct new assignment fail CI.
|
||||
|
||||
**Why it happens:**
|
||||
Range-assertion tests like this encode a snapshot of the system state at the time they were written. They do not automatically update when the valid range evolves.
|
||||
|
||||
**Consequences:**
|
||||
- CI red on every correct new class addition until the test is updated
|
||||
- Developer wastes time diagnosing a failing test that is wrong, not the code
|
||||
- Risk: developer deletes the test entirely rather than updating it, losing the coverage
|
||||
|
||||
**Prevention:**
|
||||
Update the test when the frequency allocation design is finalized. The new range should accommodate whatever spectrum is decided, e.g.:
|
||||
|
||||
```go
|
||||
func TestFrequenciesInRange(t *testing.T) {
|
||||
for class, cfg := range synth.ClassFreqConfigs {
|
||||
if cfg.BaseHz < 60 || cfg.BaseHz > 4000 {
|
||||
t.Errorf("class %q BaseHz=%.1f is out of range [60, 4000]", class, cfg.BaseHz)
|
||||
}
|
||||
default: // "sine"
|
||||
defs = []HarmonicDef{{Ratio: 1, Amplitude: 1.0}}
|
||||
}
|
||||
return defs
|
||||
}
|
||||
```
|
||||
|
||||
Alternatively, replace the range test with a `TestFrequenciesUnique` variant that only checks for collisions (already present), which remains valid regardless of range expansion. The range test can instead verify family-level groupings: all Mail family classes are in [X, Y] Hz range, all VoIP classes in [A, B] Hz range.
|
||||
|
||||
**Detection:**
|
||||
- Audible buzzing or grainy texture on drone layers above 300 Hz with non-sine waveforms
|
||||
- Square/sawtooth waveforms sound harsher than expected at high frequencies
|
||||
- CI fails on `TestFrequenciesInRange` after adding first new class above 1100 Hz
|
||||
- The test name suggests a range violation but the code is correct
|
||||
|
||||
**Phase to address:**
|
||||
Waveform type implementation phase. The design decision (additive synthesis, not direct waveform math) must be made before coding waveform support. Switching from direct math to additive after the fact requires rewriting the oscillator API.
|
||||
Immediately when frequency allocation is decided — before adding any `ClassFreqConfigs` entries outside [60, 1100].
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A4: Waveform String Validation Fails Silently, Falls Back to Silence
|
||||
### Pitfall C5: `[families]` TOML Block Rejected by Strict Unknown-Key Validation
|
||||
|
||||
**What goes wrong:**
|
||||
A user writes `waveform = "Sawtooth"` (capital S) or `waveform = "saw"` (abbreviation). The config loading code does a simple equality check (`if waveform == "sawtooth"`), finds no match, and either panics, silently emits silence, or applies a default without telling the user. In all cases the user's intent is invisible.
|
||||
If v1.2 adds a `[families]` section (or any new top-level TOML key) to the config schema to support per-family sound configuration, a user who adds this to their TOML file will get an error when running an unpatched v1.1 binary:
|
||||
|
||||
```
|
||||
config: unknown key "families" — check spelling
|
||||
```
|
||||
|
||||
The `parseFile()` function uses `md.Undecoded()` as strict mode, which rejects any key not in `rawConfig`. The `rawConfig` struct only knows about `sounds` and `rules`. Adding `families` to the user's TOML will break the tool for anyone running the v1.1 binary — even if they do not care about family features yet.
|
||||
|
||||
**Why it happens:**
|
||||
String-based enumerations in config files have no compile-time type checking. Case sensitivity and abbreviations are user expectations that must be explicitly handled.
|
||||
This is the correct and intended behavior of `md.Undecoded()` (introduced specifically to prevent silent misconfiguration). But it means the config schema is strictly versioned: any new field must be added to `rawConfig` before any user can write it to their TOML.
|
||||
|
||||
**Consequences:**
|
||||
- Silent misconfiguration: wrong waveform with no feedback
|
||||
- Hard to debug: config appears valid, sound is just wrong
|
||||
- User adds `[families]` to their config, upgrades their TOML, then tries to run v1.1 binary (e.g., from a build that hasn't shipped yet) — immediate error
|
||||
- More critically: if v1.2 adds `[families]` to `rawConfig` but the user's v1.1 config does not have `[families]` at all — this direction is fine, since absent keys are not reported by `Undecoded()`
|
||||
|
||||
**Prevention:**
|
||||
Normalize waveform strings at parse time (`strings.ToLower`, `strings.TrimSpace`), validate against the accepted set, and return an explicit error with the accepted values if the string is unrecognized:
|
||||
The direction of concern is: user writes v1.2 TOML, runs v1.1 binary. Since this project does not provide v1.1 binary distribution to end users (it's a CLI built from source), this pitfall is primarily about development workflow and test fixtures:
|
||||
|
||||
1. Update `rawConfig` struct to include `families` (or whatever the new group config key is named) before any tests or documentation reference the new config format
|
||||
2. Any test fixture `.toml` files should use the schema matching the current binary's `rawConfig` struct
|
||||
3. For documentation examples: do not publish TOML samples containing `[families]` until the code that handles it is shipped
|
||||
|
||||
The v1.1 → v1.2 migration path for the config struct should be:
|
||||
- `rawConfig` adds `Groups map[string]GroupOverride` (or similar) — unknown key validation now accepts it
|
||||
- If absent in user's TOML (the common case): `raw.Groups` is nil — safe, no behavior change
|
||||
- If present: processed in the new merge step
|
||||
|
||||
**Detection:**
|
||||
- Test fixture containing new config key causes `config: unknown key` error in a test that still uses old `rawConfig`
|
||||
- `--print-config` output contains new group annotations but user running v1.1 binary sees error
|
||||
|
||||
**Phase to address:**
|
||||
Config schema extension phase. Update `rawConfig` and `PrintConfig` before any code that generates or consumes the new TOML format.
|
||||
|
||||
---
|
||||
|
||||
## Moderate Pitfalls
|
||||
|
||||
### Pitfall C6: Within-Family Detuning Causes Critical Band Masking at High Frequencies
|
||||
|
||||
**What goes wrong:**
|
||||
The v1.2 spec calls for "within-group sound design: shared base frequency, different waveforms or slight detuning." If two classes in the same family are assigned frequencies closer than one critical bandwidth, the human auditory system treats them as a single tone rather than two distinct sounds. The "slightly detuned" design goal backfires: instead of sounding like two related-but-distinct protocols, the two tones merge perceptually into one broader tone with beating artifacts.
|
||||
|
||||
Critical bandwidth (ERB) formula: `ERB(f) = 24.7 * (4.37 * f/1000 + 1)` Hz.
|
||||
|
||||
Concrete values for the NetSynth frequency range:
|
||||
- At 100 Hz: ~35 Hz critical bandwidth (tones must be >35 Hz apart)
|
||||
- At 500 Hz: ~48 Hz critical bandwidth (tones must be >48 Hz apart)
|
||||
- At 1000 Hz: ~72 Hz critical bandwidth (tones must be >72 Hz apart)
|
||||
- At 2000 Hz: ~117 Hz critical bandwidth (tones must be >117 Hz apart)
|
||||
|
||||
The v1.1 `autoAssignFreq` used 50 Hz steps in the 1200–2350 Hz range. At 1200 Hz, critical bandwidth is ~88 Hz. A 50 Hz step at that frequency is inside the critical band — the two tones will mask each other.
|
||||
|
||||
**Why it happens:**
|
||||
Frequency step sizes that feel visually reasonable (e.g., 50 Hz) do not scale with the logarithmic nature of human pitch perception. The critical band narrows in Hz as frequency decreases but the absolute Hz separation needed for perceptual distinctness increases with frequency.
|
||||
|
||||
**Consequences:**
|
||||
- Same-family protocols sound identical rather than "related but distinct"
|
||||
- Within-family distinguishability — a core design goal — is not achieved even though the Hz values differ
|
||||
- The bug is hard to detect: `--print-config` shows different Hz values, but the audio is perceptually undifferentiated
|
||||
|
||||
**Prevention:**
|
||||
Use a logarithmic (musical interval) separation for within-family detuning rather than fixed-Hz steps. A minor third (ratio 1.2) or major second (ratio 1.125) provides psychoacoustically safe separation across the full frequency range used by NetSynth:
|
||||
|
||||
- Family base at 800 Hz, member 2 at 800 * 1.125 = 900 Hz (100 Hz gap, safe)
|
||||
- Family base at 2000 Hz, member 2 at 2000 * 1.125 = 2250 Hz (250 Hz gap, safe vs 117 Hz critical band)
|
||||
|
||||
Rule of thumb for NetSynth protocol family design: within a family, space members at least 1.25x the critical bandwidth of the lower tone. Using a minor second (semitone, ratio 1.059) as the minimum separation gives ~75 Hz at 1300 Hz — marginal. Use at least a major second (ratio 1.122) for reliable perceptual separation.
|
||||
|
||||
**Detection:**
|
||||
- Two protocols in the same family sound identical in listening test
|
||||
- The beating artifact (amplitude modulation at the difference frequency) is audible when two closely-spaced tones are both active
|
||||
|
||||
**Phase to address:**
|
||||
Frequency allocation and family sound design phase. Compute critical bandwidths for all proposed family member frequencies before finalizing the allocation. A short spreadsheet checking `ERB(f) < |f2 - f1|` for each pair catches this before any code is written.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall C7: Adding ~20 Rules to the Rule Slice Does Not Degrade Classification Performance, But Dual-Port Rules Do
|
||||
|
||||
**What goes wrong:**
|
||||
The existing `DefaultRules` slice has 12 entries. Adding 20–30 more for protocols like IMAP (143), IMAPS (993), POP3 (110), LDAP (389), RDP (3389), FTP (21), SIP (5060), SNMP (161), etc., increases the linear scan from ~12 comparisons to ~40 comparisons per packet.
|
||||
|
||||
At 44100 Hz / 22050 SamplesPerWindow = 2 windows/sec, and typical home/office network rates of 1000–5000 packets/sec, the classifier is called ~2500 times/sec. Each call does a linear scan over 40 rules. At ~4 ns per comparison (cache-warm slice iteration), 40-rule scan ≈ 160 ns per packet. For 5000 packets/sec, that is 0.8 ms/sec total classifier CPU — negligible.
|
||||
|
||||
The actual risk is not linear scan overhead but **dual-port rule confusion**: many protocols have both a plain and a TLS/secure variant on different ports (HTTP:80 and HTTPS:443, SMTP:25 and SMTPS:587, IMAP:143 and IMAPS:993, POP3:110 and POP3S:995, LDAP:389 and LDAPS:636). If these are naively placed into separate classes with different frequencies, a mail server using SMTPS at 587 will be classified differently from one using SMTP at 25 — even though they are the same protocol family. The frequency space gets overcrowded with variants that sound like separate protocols but represent the same thing.
|
||||
|
||||
**Why it happens:**
|
||||
The classification decision "same class vs separate classes" for secure and insecure protocol variants is not obvious. The default instinct is to add more rules = more specificity = better, but perceptually the user wants "I can hear that I have mail traffic" not "I can tell the exact TLS variant."
|
||||
|
||||
**Consequences:**
|
||||
- Frequency spectrum crowded with 2x the expected number of mail-related tones
|
||||
- Secure and insecure variants of the same protocol family cancel each other's coherence
|
||||
- The "family" identity becomes invisible — SMTP and SMTPS sound like different protocols
|
||||
|
||||
**Prevention:**
|
||||
Group insecure and secure variants of the same protocol into the same `TrafficClass`:
|
||||
- `ClassSMTPFamily` covers ports 25, 465, 587
|
||||
- `ClassIMAPFamily` covers ports 143, 993
|
||||
- `ClassPOP3Family` covers ports 110, 995
|
||||
|
||||
Add multiple `Rule` entries for the same class (one per port). The classifier already supports this — multiple rules with different ports mapping to the same class are correct. The frequency allocation should then be: one slot per protocol family, not one slot per port.
|
||||
|
||||
```go
|
||||
var validWaveforms = map[string]struct{}{
|
||||
"sine": {}, "square": {}, "sawtooth": {}, "triangle": {},
|
||||
// In classify/rules.go:
|
||||
{Protocol: "tcp", DstPort: 25, Class: ClassMailSMTP},
|
||||
{Protocol: "tcp", DstPort: 587, Class: ClassMailSMTP},
|
||||
{Protocol: "tcp", DstPort: 465, Class: ClassMailSMTP},
|
||||
```
|
||||
|
||||
**Detection:**
|
||||
- Frequency assignment table has 30+ entries for ~15 conceptual protocol families
|
||||
- Listening test: mail traffic sounds like 3 separate overlapping tones instead of one identifiable mail layer
|
||||
|
||||
**Phase to address:**
|
||||
Protocol list design phase (before any rule code). Define the class list (family → single class name → list of ports) before implementing rules. The family grouping decision should drive the `TrafficClass` constant list.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall C8: `AllClasses()` and `ClassFreqConfigs` Must Both Be Updated Atomically — Two Callsites, Not One
|
||||
|
||||
**What goes wrong:**
|
||||
Adding a new protocol class to NetSynth requires touching three locations:
|
||||
1. A new `ClassXxx TrafficClass = "xxx"` constant in `classify/types.go`
|
||||
2. A new entry in `classify.AllClasses()` return slice in `classify/types.go`
|
||||
3. A new entry in `synth.ClassFreqConfigs` map in `synth/config.go`
|
||||
|
||||
If any one of these is missing:
|
||||
- Missing from `AllClasses()`: `--print-config` does not emit it; `config/config.go`'s `PrintConfig` classifies it as "user-defined" rather than "built-in"; `TestNumLayersMatchesAllClasses` fails
|
||||
- Missing from `ClassFreqConfigs`: `TestAllClassesHaveConfig` fails; `copyDefaults()` does not include it; user can't override it in TOML
|
||||
- Missing constant (using a string literal instead): compiles, but typos create a second unintended class
|
||||
|
||||
When adding 20+ new classes, the three-location update is repeated 20+ times. The likelihood of a missed entry in one location is high.
|
||||
|
||||
**Why it happens:**
|
||||
Go does not have enum types that automatically enforce that a new member must be registered in every relevant collection. The `classify.TrafficClass` type is a `string` type alias — adding a constant does not force updates to `AllClasses()` or `ClassFreqConfigs`.
|
||||
|
||||
**Consequences:**
|
||||
- Test failure that is diagnostic but potentially confusing ("I added the class, why does the test fail?")
|
||||
- Less dangerous but still: `--print-config` shows wrong annotation (user-defined vs built-in) for new classes
|
||||
|
||||
**Prevention:**
|
||||
Write a single source-of-truth Go data structure that drives all three, rather than maintaining them independently:
|
||||
|
||||
```go
|
||||
// In classify/types.go: define the authoritative ordered list
|
||||
var builtinClassDefs = []struct {
|
||||
Class TrafficClass
|
||||
Display string
|
||||
}{
|
||||
{ClassICMP, "ICMP"},
|
||||
// ... all classes ...
|
||||
{ClassMailSMTP, "mail-smtp"},
|
||||
}
|
||||
func validateWaveform(s string) (string, error) {
|
||||
normalized := strings.ToLower(strings.TrimSpace(s))
|
||||
if _, ok := validWaveforms[normalized]; !ok {
|
||||
return "", fmt.Errorf("unknown waveform %q: must be one of sine, square, sawtooth, triangle", s)
|
||||
|
||||
// AllClasses() derives from this:
|
||||
func AllClasses() []TrafficClass {
|
||||
classes := make([]TrafficClass, len(builtinClassDefs))
|
||||
for i, def := range builtinClassDefs {
|
||||
classes[i] = def.Class
|
||||
}
|
||||
return normalized, nil
|
||||
return classes
|
||||
}
|
||||
```
|
||||
|
||||
Then `ClassFreqConfigs` can be validated against `AllClasses()` at test time rather than being separately maintained. Adding a new class means updating only `builtinClassDefs` — the rest is derived.
|
||||
|
||||
Alternatively: add a comment above both `AllClasses()` and `ClassFreqConfigs` stating "KEEP IN SYNC — adding a class requires updating both" and rely on the existing `TestNumLayersMatchesAllClasses` test to catch mismatches.
|
||||
|
||||
**Detection:**
|
||||
- `TestNumLayersMatchesAllClasses` fails
|
||||
- `TestAllClassesHaveConfig` fails
|
||||
|
||||
**Phase to address:**
|
||||
Config validation step (same phase as config loading). Implement all string field validation in a single `validate(cfg Config) error` function called immediately after decoding.
|
||||
First code phase of v1.2 protocol additions, before adding any new classes. Decide on the sync strategy and document it clearly so all 20+ additions follow the same pattern.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A5: User Rules Appended After Catch-All Rules Are Unreachable
|
||||
### Pitfall C9: Port-Range and Multi-Port Rules Require Protocol Rule Schema Extension
|
||||
|
||||
**What goes wrong:**
|
||||
The existing `DefaultRules` slice ends with two catch-alls:
|
||||
Some protocols use dynamic or high-number ports that cannot be expressed as a single `DstPort uint16` rule. Examples:
|
||||
- RTP (VoIP media) uses ephemeral UDP ports in a range (typically 16384–32767 or 49152–65535)
|
||||
- mDNS (multicast DNS/discovery) uses UDP port 5353 but also matches on IP multicast addresses
|
||||
- NetBIOS uses ports 137, 138, 139 — three separate ports for the same protocol family
|
||||
|
||||
```go
|
||||
{Protocol: "tcp", DstPort: 0, Class: ClassOtherTCP},
|
||||
{Protocol: "udp", DstPort: 0, Class: ClassOtherUDP},
|
||||
```
|
||||
|
||||
If user-defined rules are simply appended to this slice (`append(DefaultRules, userRules...)`), the catch-alls match first (DstPort=0 matches any port for that protocol), and the user's rules are unreachable. Every custom rule maps to `ClassOtherTCP` or `ClassOtherUDP` instead. The user gets no sound from their custom class.
|
||||
The current `Rule` struct only supports `{Protocol, DstPort, Class}`. Adding RTP and other range-based protocols cannot be represented without extending the rule schema.
|
||||
|
||||
**Why it happens:**
|
||||
The first-match-wins semantics of `Classifier.Classify()` mean ordering is semantically critical. `DefaultRules` is a named var that exists precisely as an ordered slice — the comment `// Catch-alls (must be last)` documents this constraint. But "must be last in the defaults" does not automatically mean "must be last in the final merged slice." Developers who concatenate slices without thinking about this invariant break the system.
|
||||
The v1.0 rule design was sufficient for well-known single-port protocols. Port ranges are a natural extension that was not anticipated. Extending the schema now risks breaking the existing TOML rule syntax (`[[rules]]` blocks) that v1.1 users have written.
|
||||
|
||||
**Consequences:**
|
||||
- All user-defined rules are silently swallowed by catch-alls
|
||||
- User's custom class never activates
|
||||
- No error — the pipeline works, just wrong
|
||||
- RTP, mDNS, and other range-based protocols cannot be classified with the current rule model
|
||||
- Attempting to add them as single-port rules misses the vast majority of their traffic
|
||||
- If the rule struct is extended (e.g., `DstPortMin uint16, DstPortMax uint16`), all existing rule-reading code must be updated, and the TOML format changes
|
||||
|
||||
**Prevention:**
|
||||
Always insert user rules *before* catch-all rules. The merge strategy must be: `specificDefaultRules + userRules + catchAllRules`. Implement this with an explicit split in the default rule set:
|
||||
Decide explicitly which protocols to include in v1.2 scope. If a protocol requires port-range matching, either:
|
||||
1. Exclude it from v1.2 and note it as requiring a rule schema extension in a future milestone
|
||||
2. Implement the range extension in the rule struct first — but verify it does not break existing TOML `[[rules]]` parsing (it should not, since adding optional fields to `RawRule` with pointer types is backward-compatible with existing configs that omit those fields)
|
||||
|
||||
```go
|
||||
// In classify/rules.go, split into two exported slices:
|
||||
var SpecificRules = []Rule{ /* ICMP through DHCP */ }
|
||||
var CatchAllRules = []Rule{
|
||||
{Protocol: "tcp", DstPort: 0, Class: ClassOtherTCP},
|
||||
{Protocol: "udp", DstPort: 0, Class: ClassOtherUDP},
|
||||
}
|
||||
|
||||
// Merge function used by config loading:
|
||||
func MergeRules(userRules []Rule) []Rule {
|
||||
result := make([]Rule, 0, len(SpecificRules)+len(userRules)+len(CatchAllRules))
|
||||
result = append(result, SpecificRules...)
|
||||
result = append(result, userRules...)
|
||||
result = append(result, CatchAllRules...)
|
||||
return result
|
||||
}
|
||||
```
|
||||
|
||||
Alternatively, annotate each default rule with a `CatchAll bool` field and sort before use. The split-slice approach is simpler and more explicit.
|
||||
For the v1.2 protocol list, favor protocols with well-known single static ports (IMAP:143, POP3:110, LDAP:389, RDP:3389, etc.) and defer RTP, dynamic SIP media, and NetBIOS-style multi-port protocols to a future "advanced rule types" milestone.
|
||||
|
||||
**Detection:**
|
||||
- User-defined rule that should match traffic does not produce its custom sound
|
||||
- `--verbose` output shows traffic being classified as `OtherTCP`/`OtherUDP` instead of the custom class
|
||||
- Test: write a rule for port 8080, send HTTP traffic to port 8080, verify it hits the custom class and not `ClassOtherTCP`
|
||||
- RTP traffic appears as `ClassOtherUDP` even after adding a rule
|
||||
- Attempting to write a TOML rule for RTP using a single port produces incorrect results
|
||||
|
||||
**Phase to address:**
|
||||
User-defined rules phase. The `classify/rules.go` split must be the first code change before any config loading logic references the rule slice.
|
||||
Protocol list design phase. Before implementation, filter the candidate protocol list to only those expressible with current `{Protocol, DstPort, Class}` semantics, or decide up front to extend the schema and account for the additional complexity.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A6: User-Defined Classes Have No synth.FreqConfig Entry — Bank Panics or Plays Silence
|
||||
## Minor Pitfalls
|
||||
|
||||
### Pitfall C10: `--print-config` Group Annotations Must Not Break Existing TOML Output Parsing
|
||||
|
||||
**What goes wrong:**
|
||||
`OscillatorBank.NewBank()` iterates over `classify.AllClasses()` and looks up each class in `ClassFreqConfigs`. A user-defined rule creates a new `TrafficClass` (e.g., `"my-api"`). This class is not in `AllClasses()`, so the bank has no layer for it. The aggregator increments a count for `"my-api"`, `RenderWindow` looks up `b.layers["my-api"]`, gets nil, and either panics (nil pointer dereference on `layer.AdvanceSample()`) or silently contributes nothing to the mix.
|
||||
|
||||
**Why it happens:**
|
||||
`classify.AllClasses()` is a hardcoded list of the 14 built-in classes. The synth bank is constructed once at startup from this static list. User-defined classes are a runtime extension that the bank knows nothing about.
|
||||
|
||||
**Consequences:**
|
||||
- Nil pointer panic in `RenderWindow` if the layer map lookup is not nil-guarded
|
||||
- Or silent: user-defined class traffic is captured and aggregated but never rendered to audio
|
||||
- In either case the user's primary feature request (custom sounds for custom classes) silently fails
|
||||
`PrintConfig()` in `config/config.go` emits commented TOML that users may use as a template. If v1.2 changes the output format — for example, adding group header comments like `# === Web Family ===` above related classes — and a user pipes `--print-config` output back to a config file, the comments are harmless. However, if `PrintConfig` emits actual TOML key-value pairs for a `[families]` section that the current `rawConfig` struct cannot parse, loading that output as a config file will fail with `unknown key "families"`.
|
||||
|
||||
**Prevention:**
|
||||
The bank must be constructed from the *full* set of active classes, including user-defined ones. The construction path should be:
|
||||
All new group-related output in `--print-config` must either be:
|
||||
1. Comments only (lines starting with `#`) — safe, TOML ignores them
|
||||
2. Actual config keys that `rawConfig` can parse — requires adding them to `rawConfig` first
|
||||
|
||||
1. Load config (parse TOML, validate)
|
||||
2. Compute effective rule set (built-in + user rules)
|
||||
3. Extract the complete set of `TrafficClass` values referenced by all rules
|
||||
4. Pass this full class set to `NewBank` (or equivalent) so a layer is created for every reachable class
|
||||
5. Wire user-defined class frequencies from config into the bank
|
||||
Never emit a `[families]` or `[groups]` TOML block in `--print-config` output before the corresponding struct field exists in `rawConfig`.
|
||||
|
||||
`AllClasses()` in `classify/types.go` should either remain the static built-in list (used for display/iteration of built-ins) or be replaced by a dynamic function that takes the active rule set as input. Do not rely on the hardcoded list in the bank-construction path when user-defined classes are possible.
|
||||
|
||||
**Detection:**
|
||||
- Panic: `runtime error: invalid memory address or nil pointer dereference` in `synth/bank.go:RenderWindow`
|
||||
- Or: user-defined class produces no sound, no error
|
||||
- Test: create a config with one user rule using a custom class; verify the bank is built with a layer for that class and that layer produces sound
|
||||
|
||||
**Phase to address:**
|
||||
User-defined rules phase, specifically the bank initialization step. This is the deepest integration point — it touches the pipeline at capture → classify → aggregate → synthesize.
|
||||
**Phase to address:** Config output phase.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A7: Config Auto-Discovery Follows Wrong Order or Ignores XDG Variables
|
||||
### Pitfall C11: New Class Constants Named Inconsistently With Existing Pattern
|
||||
|
||||
**What goes wrong:**
|
||||
The spec calls for auto-discovery from `./netsynth.toml` then `~/.config/netsynth/config.toml`. A naive implementation uses `os.UserHomeDir()` to build the fallback path. On systems where `$XDG_CONFIG_HOME` is set to a non-default location (common on NixOS, custom dotfile managers, CI environments), the tool ignores the user's configured config directory and looks in `~/.config` anyway. The user has a config at `$XDG_CONFIG_HOME/netsynth/config.toml` that is never found.
|
||||
|
||||
Additionally, `os.UserHomeDir()` returns an error if `$HOME` is unset (e.g., inside some Docker containers or cron jobs). If this error is not handled, the path construction silently produces `"/.config/netsynth/config.toml"` (an absolute path starting with `/.config`) rather than failing with a useful message.
|
||||
|
||||
**Why it happens:**
|
||||
Go's `os.UserConfigDir()` already implements the XDG lookup (`$XDG_CONFIG_HOME` → `~/.config` on Linux, `~/Library/Application Support` on macOS). Most developers reach for `os.UserHomeDir()` + hardcoded `".config"` string because it is the first function they find in the stdlib.
|
||||
|
||||
**Consequences:**
|
||||
- User's config is silently ignored when `$XDG_CONFIG_HOME` is non-default
|
||||
- Confusing behavior difference between development machines and CI
|
||||
The existing constants follow `ClassHTTPS`, `ClassSSH`, `ClassNTP` — protocol name in CamelCase. For grouped protocols, if constants are named `ClassMailSMTP`, `ClassMailIMAP`, `ClassMailPOP3`, the "Mail" prefix creates a new naming convention that does not match the flat naming of existing classes. The `TrafficClass` string values (e.g., `"mail-smtp"`, `"mail-imap"`) become the identifiers users reference in TOML — if these are kebab-case with family prefix (e.g., `[sounds.mail-smtp]`), that is a new pattern that does not match existing class names like `[sounds.HTTPS]` (uppercase) or `[sounds.other-TCP]` (mixed case with hyphen).
|
||||
|
||||
**Prevention:**
|
||||
Use `os.UserConfigDir()` (stdlib, Go 1.13+) for the platform-appropriate config directory. This correctly respects `$XDG_CONFIG_HOME` on Linux and `APPDATA` on Windows (if ever relevant). The discovery order should be:
|
||||
Decide the naming convention for grouped class string values before adding any constants:
|
||||
- Option A: `"smtp"`, `"imap"`, `"pop3"` — flat names, consistent with `"SSH"`, `"DNS"` (but drops family grouping in the config key)
|
||||
- Option B: `"mail-smtp"`, `"mail-imap"` — family-prefixed, makes grouping visible in TOML but is a new pattern
|
||||
|
||||
```go
|
||||
func configSearchPaths() []string {
|
||||
var paths []string
|
||||
// 1. Current directory (highest precedence)
|
||||
paths = append(paths, "netsynth.toml")
|
||||
// 2. XDG/platform config dir
|
||||
if cfgDir, err := os.UserConfigDir(); err == nil {
|
||||
paths = append(paths, filepath.Join(cfgDir, "netsynth", "config.toml"))
|
||||
}
|
||||
return paths
|
||||
}
|
||||
```
|
||||
Existing classes use all-caps for protocols (`"HTTPS"`, `"ICMP"`) and lowercase-hyphenated for non-standard ones (`"other-TCP"`, `"unknown-1"`). New classes should follow the lowercase-hyphenated pattern for multi-word names. Document the convention at the top of `classify/types.go`.
|
||||
|
||||
If `--config` flag is set, use that path exclusively and return a clear error if the file is absent (do not fall through to auto-discovery when explicit path is provided).
|
||||
|
||||
**Detection:**
|
||||
- Config not loaded on systems where `$XDG_CONFIG_HOME=/custom/path`
|
||||
- Silent "no config found" behavior when a config clearly exists at the XDG path
|
||||
|
||||
**Phase to address:**
|
||||
Config loading phase. Implement the path discovery with `os.UserConfigDir()` from the start. Fix before the feature ships.
|
||||
**Phase to address:** Protocol list design phase, before writing constants.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A8: Explicit --config Flag Does Not Error on Missing File
|
||||
### Pitfall C12: Too Many Active Layers Degrades Ambient Distinctness (Perceptual Density Threshold)
|
||||
|
||||
**What goes wrong:**
|
||||
When `--config path/to/file.toml` is specified, the user expects an error if the file does not exist. If the config loader falls through to auto-discovery when the explicit path is missing, or silently uses defaults, the user has no way to detect a typo in their `--config` argument. They run a session, get "unexpected" default sounds, and have no indication their config was never loaded.
|
||||
v1.1 has 14 layers. v1.2 will add 20–30 more, reaching a total of ~35–44 layers. When all layers are simultaneously active at whisper-floor amplitude, the combined output is 35 × WhisperFloor × gainPerLayer = 35 × 0.03 × (1/35) = 3% of max amplitude — still quiet. The whisper floor plus gain-per-layer math continues to work correctly.
|
||||
|
||||
**Why it happens:**
|
||||
Auto-discovery logic is convenient to write as "try these paths, use first found." Developers reuse this logic even for the `--config` code path.
|
||||
The perceptual problem is different: with 35 simultaneous drone layers, the ambient sound loses definition. Below 5–8 simultaneous distinct tones, listeners can track individual threads. Above 10–12, the output becomes a dense textural wash. This is not a technical bug but a UX risk: the "network fingerprint" value proposition weakens because the output sounds less like "I can identify HTTPS vs SSH" and more like "everything is one undifferentiated cloud."
|
||||
|
||||
**Prevention:**
|
||||
Separate the two code paths:
|
||||
- `--config` specified → `os.Open(flagValue)`, return error immediately if `errors.Is(err, os.ErrNotExist)`
|
||||
- No flag → `configSearchPaths()` loop, silently skip missing files, proceed with defaults if none found
|
||||
Group protocols into families specifically to mitigate this: a family's members should share enough spectral character (same or nearby frequency, similar waveform) that they fuse into a single perceptible "family layer" rather than adding N separate threads. The "distinct family tone" becomes the perceptual unit, not each individual protocol.
|
||||
|
||||
**Detection:**
|
||||
- `--config missing.toml` runs without error, uses defaults
|
||||
- User misses that their config file path has a typo
|
||||
Additionally, consider whether `WhisperFloor` should be reduced for high layer counts. At 35 layers, 35 × 0.03 × (1/35) = 0.03 (3% amplitude from whisper alone when all active) — which is fine. The math is self-correcting. The concern is purely perceptual richness, not clipping or silence.
|
||||
|
||||
**Phase to address:**
|
||||
Config loading phase. A one-line `if flagValue != "" { /* require it */ }` branch is sufficient.
|
||||
**Phase to address:** Sound design review after all frequencies are assigned. Listening test with a mix of protocols active simultaneously is the definitive check.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A9: User Rules That Target the Same Port as Built-in Rules Are Silently Shadowed
|
||||
|
||||
**What goes wrong:**
|
||||
A user writes a rule for `{Protocol: "tcp", DstPort: 443, Class: "my-api"}` intending to reclassify their internal HTTPS traffic. If built-in `ClassHTTPS` still appears before the user rule in the merged slice, the built-in rule wins every time. The user's intent ("I want my port-443 traffic to sound different") is silently defeated.
|
||||
|
||||
**Why it happens:**
|
||||
First-match-wins with `SpecificRules + userRules + CatchAllRules` means built-in specific rules still precede user rules. A user trying to *override* a built-in mapping must replace it, not add after it.
|
||||
|
||||
**Consequences:**
|
||||
- User's specific rule is unreachable if a built-in rule covers the same port/protocol
|
||||
- No error, no warning
|
||||
- Functionally the same as Pitfall A5 but for specific (non-catch-all) built-in rules
|
||||
|
||||
**Prevention:**
|
||||
Two viable strategies:
|
||||
1. **User rules first:** `userRules + specificDefaultRules + catchAllRules`. User rules always take precedence. Built-ins serve as fallback. This is the simplest design and most aligned with user expectations ("I configure what I care about; defaults handle everything else").
|
||||
2. **Conflict detection:** After merging, scan for duplicate `(protocol, dstPort)` pairs and emit a warning: `"User rule for tcp:443 shadows built-in HTTPS rule. Did you mean to replace it?"`.
|
||||
|
||||
Option 1 is recommended for simplicity. Document it clearly: "User-defined rules are evaluated before built-in rules."
|
||||
|
||||
**Detection:**
|
||||
- User-defined rule for a built-in port (80, 443, 22, etc.) never activates
|
||||
- Verbose output shows built-in class instead of user class for the expected traffic
|
||||
|
||||
**Phase to address:**
|
||||
User-defined rules phase, merge strategy design. Address at the same time as Pitfall A5.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall A10: New TrafficClass Strings From Config Are Not Validated — Empty String or Whitespace Is a Valid Key
|
||||
|
||||
**What goes wrong:**
|
||||
A user writes:
|
||||
|
||||
```toml
|
||||
[[rules]]
|
||||
protocol = "tcp"
|
||||
dst_port = 9200
|
||||
class = ""
|
||||
```
|
||||
|
||||
The string `""` decodes without error. It is a valid Go map key. It gets inserted into the `WindowSnapshot.Counts` map and the aggregator increments `Counts[""]`. The bank has no layer for `""`. The behavior is undefined — silent or panic depending on nil-guard presence.
|
||||
|
||||
Similarly, `class = " elasticsearch "` (padded spaces) decodes to a string with leading/trailing whitespace that does not match any configured sound entry (because the config sound entry key is `"elasticsearch"` without spaces).
|
||||
|
||||
**Prevention:**
|
||||
Validate all `Class` string values from user rules in the `validate()` step:
|
||||
```go
|
||||
if strings.TrimSpace(rule.Class) == "" {
|
||||
return fmt.Errorf("rule %d: class name must not be empty", i)
|
||||
}
|
||||
rule.Class = strings.TrimSpace(rule.Class)
|
||||
```
|
||||
Also validate that class names do not collide with reserved built-in class names (`"ICMP"`, `"DNS"`, etc.) unless the user is explicitly overriding a built-in sound (which is a distinct feature — it should be opt-in, not accidental).
|
||||
|
||||
**Phase to address:**
|
||||
Config validation step.
|
||||
|
||||
---
|
||||
|
||||
## v1.0 Pitfalls (Retained for Reference)
|
||||
|
||||
The following pitfalls from the initial MVP research remain valid. They are retained in condensed form for reference.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B1: Using `google/gopacket` Instead of the Active Community Fork
|
||||
|
||||
**What goes wrong:** Import of the unmaintained original — 270 open issues, Go compat degrades.
|
||||
**Prevention:** Import `github.com/gopacket/gopacket` (v1.5.0, requires Go 1.24+).
|
||||
**Phase:** Phase 1 — set correct import path from day one.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B2: CGo Destroys the "Single Binary" Promise
|
||||
|
||||
**What goes wrong:** `gopacket/pcap` (CGo + libpcap) produces a dynamically-linked binary that fails on machines without `libpcap.so`.
|
||||
**Prevention:** Use `packetcap/go-pcap` (pure Go capture, already the chosen stack). Verify with `ldd ./netsynth`.
|
||||
**Phase:** Phase 1 — foundational architecture decision.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B3: `CAP_NET_RAW` + Binary Location = Silent Failure on Linux
|
||||
|
||||
**What goes wrong:** `setcap` is silently ignored on `nosuid` filesystems. Binary appears broken from home directories.
|
||||
**Prevention:** Install to `/usr/local/bin`; document two run modes; emit clear privilege error.
|
||||
**Phase:** Phase 1 + CLI UX.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B4: Packet Buffer Overflow Under Moderate Traffic Load
|
||||
|
||||
**What goes wrong:** Default capture buffer fills faster than the classifier consumes it; silent packet drops misrepresent traffic.
|
||||
**Prevention:** Large capture buffer (32 MB); buffered channel between capture and classify goroutines.
|
||||
**Phase:** Phase 1/2 (capture pipeline architecture).
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B5: ZeroCopy Packet Data Use-After-Free
|
||||
|
||||
**What goes wrong:** `ZeroCopyReadPacketData()` invalidates previous slice on each call; silent data corruption in concurrent code.
|
||||
**Prevention:** Use `ReadPacketData()` (copying API) unless profiling proves allocation bottleneck.
|
||||
**Phase:** Phase 1 (capture/decode).
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B6: MP3 Output Is Corrupt Due to LAME Initialization Order
|
||||
|
||||
**What goes wrong:** Skipping `InitParams()` or setting parameters out of order produces unplayable MP3.
|
||||
**Prevention:** Always call `InitParams()` before writing frames; smoke test with `ffprobe`.
|
||||
**Phase:** Audio synthesis / encoding phase.
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B7: PCM Sample Overflow Produces Wrap-Around Distortion
|
||||
|
||||
**What goes wrong:** Summing `int16` layers overflows and wraps (32767 + 100 = -32667), producing buzzing distortion.
|
||||
**Prevention:** Synthesize in `float64 [-1.0, 1.0]`; clamp before int16 cast. Already implemented in `synth/mixer.go`.
|
||||
**Phase:** Audio synthesis (already addressed in v1.0).
|
||||
|
||||
---
|
||||
|
||||
### Pitfall B8: Tone-per-Protocol Mapping Produces Perceptual Chaos
|
||||
|
||||
**What goes wrong:** Frequencies too close together mask each other; output is undifferentiated buzz.
|
||||
**Prevention:** Space protocols across register bands; use harmonic/musical intervals. Already addressed in v1.0.
|
||||
**Phase:** Audio mapping (already addressed in v1.0).
|
||||
|
||||
---
|
||||
|
||||
## v1.1 Phase-Specific Warnings
|
||||
## Phase-Specific Warnings (v1.2)
|
||||
|
||||
| Phase Topic | Likely Pitfall | Mitigation |
|
||||
|-------------|---------------|------------|
|
||||
| TOML struct design | A1: zero-value overwrites defaults | Use pointer fields for all optional overrides |
|
||||
| Config strict decode | A2: typos silently ignored | Use `md.Undecoded()` as strict mode check |
|
||||
| Waveform implementation | A3: naive waveform aliases | Use additive synthesis (bandlimited harmonic series) — compatible with existing `[]HarmonicDef` API |
|
||||
| Waveform string input | A4: case/abbreviation mismatches | Normalize + validate with clear error listing accepted values |
|
||||
| Rule merge ordering (catch-alls) | A5: user rules after catch-alls are unreachable | Split `DefaultRules` into `SpecificRules` + `CatchAllRules`; user rules go in between |
|
||||
| Bank construction | A6: custom class has no synth layer | Derive full class set from merged rule slice; pass to bank constructor |
|
||||
| Config discovery | A7: XDG ignored, `~/.config` hardcoded | Use `os.UserConfigDir()` not `os.UserHomeDir() + "/.config"` |
|
||||
| --config flag path | A8: missing explicit path silently ignored | Two distinct code paths: flag path (require) vs auto-discovery (skip-missing) |
|
||||
| Rule merge ordering (specific built-ins) | A9: user rule shadowed by built-in for same port | User rules first in merged slice (`userRules + specificDefaults + catchAlls`) |
|
||||
| Class name validation | A10: empty/whitespace class name is valid Go string | Validate and trim all class strings in `validate()` |
|
||||
| Frequency spectrum design | C1: existing Hz overrides become stale | Allocate new classes above 1100 Hz; leave v1.1 range frozen |
|
||||
| Frequency spectrum design | C6: within-family tones too close | Enforce >1 critical bandwidth separation; use musical interval ratios |
|
||||
| Auto-assign range update | C2: new built-ins collide with user custom class auto-assign range | Move auto-assign base above highest new built-in Hz; add range-check test |
|
||||
| Test suite update | C4: TestFrequenciesInRange fails on new Hz values | Update range bound in test before adding any class above 1100 Hz |
|
||||
| Protocol list curation | C7: secure + insecure variants fill 2x slots | Decide: one class per family (covering all ports) or one class per variant |
|
||||
| Protocol list curation | C9: RTP and range-based protocols not expressible | Exclude from v1.2 or extend rule schema; decide before writing rules |
|
||||
| Adding class constants | C3: NumLayers stale constant misleads | Remove or document-only; update test name; keep AllClasses+ClassFreqConfigs atomic |
|
||||
| Adding class constants | C8: three-location update forgetting one | Establish single source of truth or add checklist in classify/types.go comment |
|
||||
| Group config schema | C5: new TOML key rejected by old binary | Add to rawConfig struct before documenting or emitting the key anywhere |
|
||||
| --print-config update | C10: output contains unparseable TOML | All group output must be comments only, or rawConfig must accept the new keys |
|
||||
| Class constant naming | C11: naming convention drift | Decide convention once in design phase; document in types.go |
|
||||
| Perceptual density | C12: 35+ layers is a wash | Design family groupings to fuse into ~10 perceptual units, not 35 threads |
|
||||
|
||||
---
|
||||
|
||||
## Integration Gotchas (v1.1 Additions)
|
||||
## Backward Compatibility Summary (v1.1 → v1.2)
|
||||
|
||||
| Integration | Common Mistake | Correct Approach |
|
||||
|-------------|----------------|------------------|
|
||||
| Config → Bank wire-up | Pass `classify.AllClasses()` to bank; custom classes missing | Derive layer set from `Classifier.ActiveClasses()` — all classes reachable via the effective rule set |
|
||||
| Waveform → FreqConfig | Add `WaveformType string` to `FreqConfig`; forget to generate harmonics at bank init | Generate `[]HarmonicDef` from waveform+freq at bank/layer construction time, not at sample render time |
|
||||
| User rules → Classifier | Replace `DefaultRules` var directly; breaks tests relying on it | Keep `DefaultRules` immutable; construct `mergedRules` for runtime use |
|
||||
| Config file absent | Return error if no config found | Return nil (no config = all defaults). Only error on explicit `--config` path that is missing |
|
||||
| Sound overrides for built-in class | User sets freq for "HTTPS" — must hit `ClassHTTPS` layer | Match config sound keys case-insensitively against `TrafficClass` string values; map `"HTTPS"` → `classify.ClassHTTPS` |
|
||||
| Change Type | Impact on v1.1 User Configs | Mitigation |
|
||||
|-------------|----------------------------|------------|
|
||||
| New built-in classes added | None — absent TOML keys silently default; existing overrides unaffected | Safe |
|
||||
| Existing built-in Hz values changed | User overrides silently re-apply old v1.1 Hz values, masking the change | Freeze v1.1 Hz range; do not reassign existing classes |
|
||||
| New top-level TOML key added (e.g., `[families]`) | v1.1 binary rejects config with new key via `Undecoded()` | Acceptable since user controls which binary they run |
|
||||
| Auto-assign range shifted | User custom classes get different Hz values than before | Announce in changelog; update `autoAssignFreq` constants and document |
|
||||
| `[[rules]]` schema extended (port range fields) | Existing rules without new fields: no change (pointer types are nil = absent) | Backward-compatible if new fields are optional pointers |
|
||||
| Class string values renamed | TOML `[sounds.old-name]` silently produces "unknown class" warning (not error) | Do not rename existing class strings |
|
||||
|
||||
---
|
||||
|
||||
## v1.1 Pitfalls (Retained)
|
||||
|
||||
The following pitfalls from v1.1 research remain valid and fully resolved in the codebase. They are retained in condensed form for reference. See the original v1.1 entries for full detail.
|
||||
|
||||
### Pitfall A1: TOML Unmarshal Silently Overwrites Defaults With Zero Values
|
||||
Use pointer fields (`*float64`, `*string`) — implemented in `config/config.go` via `SoundOverride`.
|
||||
|
||||
### Pitfall A2: BurntSushi/toml Silently Ignores Typos
|
||||
Use `md.Undecoded()` — implemented in `parseFile()`.
|
||||
|
||||
### Pitfall A3: Naive Square/Sawtooth/Triangle Produces Aliasing
|
||||
Use bandlimited additive synthesis — implemented in `WaveformPresetHarmonics()`.
|
||||
|
||||
### Pitfall A4: Waveform String Validation Fails Silently
|
||||
Normalize + validate — implemented in `parseWaveform()`.
|
||||
|
||||
### Pitfall A5: User Rules After Catch-All Rules Are Unreachable
|
||||
User rules prepend before built-ins — implemented: user rules inserted first in merged slice.
|
||||
|
||||
### Pitfall A6: User-Defined Classes Have No synth.FreqConfig Entry
|
||||
`addAutoFreqEntries()` handles this — implemented in `config/config.go`.
|
||||
|
||||
### Pitfall A7: Config Auto-Discovery Ignores XDG Variables
|
||||
Use `os.UserConfigDir()` — implemented in `discoverPath()`.
|
||||
|
||||
### Pitfall A8: Explicit --config Flag Does Not Error on Missing File
|
||||
Separate code paths for explicit vs auto-discovery — implemented in `resolvePath()`.
|
||||
|
||||
### Pitfall A9: User Rule Shadowed by Built-in for Same Port
|
||||
User rules evaluate first — implemented via prepend ordering.
|
||||
|
||||
### Pitfall A10: Empty/Whitespace Class Name Is a Valid Go String
|
||||
Validated in `validateRules()`.
|
||||
|
||||
---
|
||||
|
||||
## v1.0 Pitfalls (Retained, Condensed)
|
||||
|
||||
### Pitfall B1: `google/gopacket` (Unmaintained)
|
||||
Use `gopacket/gopacket` v1.5.0.
|
||||
|
||||
### Pitfall B2: CGo Destroys Single Binary
|
||||
Use `packetcap/go-pcap` (pure Go capture).
|
||||
|
||||
### Pitfall B3: CAP_NET_RAW + nosuid Filesystem
|
||||
Install to `/usr/local/bin`; emit clear privilege error.
|
||||
|
||||
### Pitfall B4: Packet Buffer Overflow
|
||||
Large capture buffer (32 MB); buffered channel between goroutines.
|
||||
|
||||
### Pitfall B5: ZeroCopy Packet Use-After-Free
|
||||
Use `ReadPacketData()` (copying API).
|
||||
|
||||
### Pitfall B6: LAME Initialization Order
|
||||
Call `InitParams()` before writing frames.
|
||||
|
||||
### Pitfall B7: PCM Sample Overflow
|
||||
Synthesize in float64 [-1, 1]; clamp before int16 cast.
|
||||
|
||||
### Pitfall B8: Tone-per-Protocol Frequency Masking
|
||||
Space protocols across register bands using musical intervals.
|
||||
|
||||
---
|
||||
|
||||
## Sources
|
||||
|
||||
- [BurntSushi/toml pkg.go.dev](https://pkg.go.dev/github.com/BurntSushi/toml) — `Undecoded()` strict mode, pointer field behavior, `MetaData` API
|
||||
- [BurntSushi/toml issue #47: Unmarshal with default values](https://github.com/BurntSushi/toml/issues/47) — confirms default-overwrite behavior
|
||||
- [pelletier/go-toml issue #252: Unmarshal overrides origin values if key is omitted](https://github.com/pelletier/go-toml/issues/252) — confirms same behavior in v1; v2 partially resolves
|
||||
- [pelletier/go-toml v2 pkg.go.dev](https://pkg.go.dev/github.com/pelletier/go-toml/v2) — strict decoder mode documentation
|
||||
- [golang/go issue #29960: os: add UserConfigDir](https://github.com/golang/go/issues/29960) — rationale for `os.UserConfigDir()` (XDG-aware)
|
||||
- [WolfSound: Basic Waveforms in Synthesis](https://thewolfsound.com/sine-saw-square-triangle-pulse-basic-waveforms-in-synthesis/) — aliasing and harmonic series for square/saw/triangle
|
||||
- [CCRMA: Alias-Free Digital Synthesis of Classic Analog Waveforms](https://ccrma.stanford.edu/~stilti/papers/blit.pdf) — bandlimited synthesis theory
|
||||
- [McGill Bandlimited Synthesis of Classic Waveforms](https://www.music.mcgill.ca/~gary/307/week5/bandlimited.html) — truncated harmonic series approach
|
||||
- [Teensy Forum: triangle & sawtooth oscillators aliasing](https://forum.pjrc.com/threads/61269-triangle-amp-sawtooth-oscillators-how-to-deal-with-aliasing) — practical aliasing impact at different frequencies
|
||||
- [adrg/xdg package](https://github.com/adrg/xdg) — XDG Base Directory Specification Go implementation (reference; stdlib `os.UserConfigDir()` is sufficient for NetSynth's needs)
|
||||
- Direct inspection of `/home/dev/workspace/yoloyolo/synth/config.go`, `config.go`, `classify/types.go`, `synth/bank.go`, `synth/config_test.go` — codebase analysis is HIGH confidence
|
||||
- [Glasberg & Moore (1990) ERB formula via Wikipedia Critical Band article](https://en.wikipedia.org/wiki/Critical_band) — critical bandwidth values at specific Hz, HIGH confidence
|
||||
- [BurntSushi/toml pkg.go.dev — Undecoded() strict mode](https://pkg.go.dev/github.com/BurntSushi/toml) — unknown key validation behavior, HIGH confidence
|
||||
- [BurntSushi/toml issue #47: default value behavior](https://github.com/BurntSushi/toml/issues/47) — TOML zero-value overwrite behavior, HIGH confidence
|
||||
- [Evanjones.ca: The Unreasonable Effectiveness of Linear Search](https://www.evanjones.ca/linear-search.html) — linear search competitive with map for N < ~100, MEDIUM confidence
|
||||
- [vitalvas.com: Slice vs Map Performance in Golang (2025)](https://blog.vitalvas.com/post/2025/10/03/slice-vs-map-performance-in-golang/) — map outperforms slice at N > 10, MEDIUM confidence; 40-rule estimate of 160 ns is first-principles, not benchmarked
|
||||
- [QSC Blog: Auditory Masking and its Effect on Perception](https://blogs.qsc.com/live-sound/auditory-masking-and-its-effect-on-our-perception-of-sound/) — masking principles, MEDIUM confidence
|
||||
- [Wikipedia: List of TCP and UDP port numbers](https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers) — port numbers for IMAP, POP3, LDAP, RDP, SIP, SNMP, Syslog, HIGH confidence
|
||||
- [RF Wireless World: Well-Known Port Numbers](https://www.rfwireless-world.com/terminology/well-known-port-numbers) — port reference, MEDIUM confidence
|
||||
|
||||
---
|
||||
*Pitfalls research for: NetSynth v1.1 — TOML config, waveform types, user-defined rules*
|
||||
*Updated: 2026-03-26*
|
||||
*Pitfalls research for: NetSynth v1.2 — Extended protocol coverage, grouped sound families*
|
||||
*Updated: 2026-03-27*
|
||||
|
||||
Reference in New Issue
Block a user