gostations/ISSUES.md

# GoStations Issues

**Project:** gostations (console radio station browser/player)  
**Analysis date:** 2026-06-05  
**Scope:** Focused review of obvious bugs, crash paths, error handling failures, and high-impact correctness issues. Performed via source inspection, test runs (`go test`), live execution, and forced error-path reproduction (bad DNS, connection failures, config edge cases).

This document captures findings from the current source tree. The existing `build/gostations` binary is stale (different build paths in debug info) — always rebuild from current sources for testing.

---

## Critical Bugs (Panics and Hard Crashes)

### 1. Nil pointer dereference on `http.Get` failure (most common network error path)

**Location:** `browser.go:50` (GetStations)

```go
resp, err := http.Get(urlstr)
if err != nil {
	log.Print(err.Error())
}
defer func(Body io.ReadCloser) {
	err := Body.Close()
	...
}(resp.Body)   // <--- resp is nil on most errors
...
err = json.NewDecoder(resp.Body).Decode(&data)
```

**Impact:** Any transient or permanent failure contacting the radio-browser API (DNS, connect refused, timeout, etc.) causes an immediate panic instead of graceful degradation.

**Reproduction:**
- Set `radio_browser.api=localhost` (or any host that resolves but refuses HTTPS on 443) in a temp `radiostations.ini` under `XDG_CONFIG_HOME`.
- Run `gostations -c "Gambia"`.
- Observed: `panic: runtime error: invalid memory address or nil pointer dereference` at the `defer` evaluation line.

**Related:** No `http.Client` with timeout/context is used anywhere. Searches can hang indefinitely.

### 2. Panic in API host selection on DNS failure or empty result

**Location:** `browser.go:25-38` (RandomIP, nslookup, reverseLookup, GetApiHost)

```go
func RandomIP(iplist []net.IP) net.IP {
	rand.NewSource(time.Now().Unix())   // result discarded!
	randomIndex := rand.Intn(len(iplist))  // panics if len==0
	...
}

func nslookup(hostname string) net.IP {
	iprecords, _ := net.LookupIP(hostname)  // error ignored
	return RandomIP(iprecords)
}

func reverseLookup(ipAddr net.IP) string {
	ptr, _ := net.LookupAddr(ipAddr.String())
	return ptr[0]  // panics if len==0
}
```

**Impact:** `GetApiHost()` is called on **every** `StationSearch` (before the actual station query). Failure here kills the entire program with either:
- `panic: invalid argument to Intn`
- index-out-of-range panic

**Reproduction:** Set `radio_browser.api=nonexistent.invalid.host.example` → full panic stack through `StationSearch` → `main`.

**Additional problems:**
- The `NewSource` call is dead code (return value thrown away; does not affect the global RNG used by `Intn`). In Go 1.14-era this made "random" API host selection deterministic. Modern autoseeding hides the bug but the intent is broken.
- No caching of the chosen API host (DNS + reverse lookup repeated for every search).
- `GetStations` is called with the result of `GetApiHost()` with zero defensive coding.

### 3. `subExecute` always returns an error on success + produces garbage output

**Location:** `commander.go:28-38`

```go
func subExecute(program string, args ...string) ([]byte, error) {
	cmd := exec.Command(program, args...)
	cmd.Stdin = os.Stdin
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	err := cmd.Run()
	if err != nil {
		fmt.Printf("%v\n", err)
	}
	return cmd.CombinedOutput()  // !!!
}
```

**Impact:**
- `CombinedOutput()` sees `Stdout` already set → returns `nil, errors.New("exec: Stdout already set")`.
- Callers ignore the error: `stdout, _ := subExecute(...)`; `fmt.Println(stdout)`.
- After every successful play (mpv exits cleanly on 'q'), the user sees a stray `[]` line, then the menu reappears.
- This is the origin of the `[]` visible in the README examples after "Exiting... (Quit)".

**Test failure:** `TestSubExecute_Live` (under `-short`) fails with exactly this error.

**Usage site:** `radiomenu.go:32` (inside the wmenu action callback, after the blocking play).

### 4. `createIniFile` continues writing after failures (fragile, relies on runtime luck)

**Location:** `filer.go:10-46`

- Records `MkdirAll` / `Create` errors but proceeds unconditionally to `file.Write`.
- On failure, `file` is `nil` (documented `os` behavior).
- Relies on `*os.File` methods returning errors instead of panicking when called on `nil` (observed behavior in current Go; not guaranteed or clean).
- Defer for `Close` is registered even on failure paths.
- Multiple "invalid argument" errors get appended for every subsequent write.

**Impact:** Auto-generation of `radiostations.ini` on first run (or missing file) is not robust. `configStat` then does `log.Fatal` on the first error in the list (with a typo: "Erorr").

**Test note:** The happy-path tests pass because they exercise success cases only. Error-collection tests would expose the mess.

---

## Error Handling, Correctness, and Robustness Issues

### 5. Widespread ignored errors + lossy error paths

- `stations.go:61`: `stations, _ := StationSearch(...)` — search errors are silently dropped. Main proceeds with potentially `nil` slice.
- `browser.go:94-102` (StationSearch): On any `GetStations` error the data is discarded and `nil` is returned. On `notok` path, `err` is returned even when it is `nil`.
- `browser.go:62-66` (GetStations): `err = json...` overwrites any prior `http.Get` error variable; partial data + last `err` is returned in some cases.
- `config.go:75,80,85,90`: All the convenience wrappers (`api()`, `player()`, `options()`, `maxitems()`) discard `Config` errors.
- Lookups in `nslookup`/`reverseLookup` discard errors.
- `radiomenu.go:35`: `log.Fatal` inside a menu action callback.

### 6. Misleading / broken config error message

**Location:** `config.go:69`

```go
if optval == "" {
	return "", errors.New("no value for option '%s'")  // %s is never formatted
}
```

Always returns the literal string containing `'%s'`. Reproduced via temp ini missing expected keys.

### 7. Command injection risk (Unix `isInstalled`)

**Location:** `commander.go:19`

```go
cmd = exec.Command("/bin/sh", "-c", "command -v "+name)
```

`name` comes from the user-editable `player.command` value in `radiostations.ini`. A malicious or accidentally malformed value executes arbitrary shell commands during the precheck.

(`subExecute` itself uses `exec.Command(program, ...)` directly and is safer for the player step, but the check is not.)

### 8. Inverted test logic for "live" tests + missing coverage

- `*_Live` tests in `*_test.go` files only run when `-short` is passed (the `if !testing.Short() { t.Skip }` guard is backwards).
- `browser_test.go` is essentially empty.
- No tests exercise the panic paths in `GetStations`, `RandomIP`, `nslookup`, config missing-option, `createIniFile` failure, etc.
- `TestPrecheck_Live` etc. rely on real global state (player present in PATH, config file creation).

### 9. Other correctness / quality issues

- `stations.go:33-40`: Custom `flag.Usage` + `flag.PrintDefaults()` produces slightly mangled help (tabs immediately after bool flags like `-v<tab>Show...`).
- `main` has a zero-arg special case for usage, but `-h` is handled by the `flag` package (works by accident). No explicit help flag handling.
- `radiomenu.go:34`: Recursive `menu.Run()` inside the selection action after playback (potential for deep stacks on repeated listen/return cycles; state of the wmenu instance is reused in a non-obvious way).
- `config.go:52`: `configparser.Delimiter = "="` is mutated globally on every `Config()` call.
- `filer.go` + `config.go`: Manual string concatenation for paths (`+ "/gostations/" + ...`) instead of `filepath.Join`. `configStat` also hard-codes the `gostations` subdirectory name.
- `str2int` (config.go:14) returns 9999 on any `Atoi` error (including overflow on 32-bit platforms). Test expects `4294967296` which only works on 64-bit `int`.
- `GetStations` URL: `?...&limit=...` when `params.Encode() == ""` produces a leading `?&`.
- No version information is embedded at runtime except via fragile `-ldflags "-X main.version=..."` (the `var version string` at package level).
- Heavy use of `log.Print`/`log.Fatal` mixes diagnostic output with user-facing behavior and aborts without cleanup.

---

## Non-Bug Recommendations / Improvement Opportunities

- **Caching & performance:** Resolve the radio-browser API host once and reuse it (or fall back to a static list of known endpoints). The current DNS dance is expensive and repeated.
- **HTTP client:** Use a single `*http.Client` with reasonable `Timeout` and context support.
- **Player execution:** Rewrite or replace `subExecute`. For long-running players the current "live + capture" approach is fundamentally conflicted. Consider just letting the player inherit stdio and not trying to capture/return anything after it exits.
- **Menu restart:** After playback, either restart the menu cleanly (new `RadioMenu` call) or use wmenu's built-in looping/return mechanisms instead of recursion + `os.Exit(0)` from inside an action.
- **Config handling:** Perform validation once at startup (as noted in the existing README TODO). Cache the parsed config. Use `fmt.Errorf` with proper verbs. Provide a `--config` flag or at least clear diagnostics when the ini is invalid/missing keys.
- **Error strategy:** Return errors up to `main`, print a user-friendly message, and exit with non-zero status. Avoid `log.Fatal` for normal control flow.
- **Randomness:** Fix `RandomIP` to actually use a per-call `*rand.Rand` if the goal is to pick among A records.
- **Testing:** Add table-driven tests (or httptest) for the error paths in browser.go. Fix the `-short` guard logic or rename the tests. Add a build tag or separate integration suite.
- **Modernization:**
  - Stop using vendoring (or at least stop committing the entire `vendor/` tree for a small CLI).
  - Update ancient dependencies (testify v1.4.0, configparser from 2019, etc.).
  - Use `//go:embed` or `debug.ReadBuildInfo` for version instead of ldflags + global.
  - Use `os.UserConfigDir()` + `filepath.Join` properly (with fallback for the old XDG logic if desired).
- **Security / robustness:** Validate/sanitize the player command (or at least document that the ini must be trusted). Add a `--player` override flag.
- **UX:** Truncation at 40 chars is aggressive on modern terminals. Consider making the menu nicer (the existing TODO mentions color/dashboard).
- **Existing README TODO items** (still relevant):
  - "Change the precheck, to do ini file validation once, rather than every time a config value is called for."
  - "Add a way to capture favorite selections."
  - "Add color or a dashboard to the menu and player."

---

## How to Reproduce Key Issues Quickly

1. **DNS / host selection panic:**
   ```sh
   mkdir -p /tmp/badgost/gostations
   cat > /tmp/badgost/gostations/radiostations.ini <<EOF
   [DEFAULT]
   radio_browser.api=nonexistent.invalid.example
   player.command=mpv
   ...
   EOF
   XDG_CONFIG_HOME=/tmp/badgost ./gostations -c "Gambia"
   ```

2. **HTTP nil-resp panic:** Same technique with `radio_browser.api=localhost`.

3. **Post-play `[]`:** Normal run selecting a station; press `q` in mpv. Observe the `[]` before the menu reprints.

4. **Config error message:** Create an ini that is missing `radio_browser.api` (or has only unknown keys) and call any station search.

---

## Suggested Next Steps

1. Fix the two panic sites in `browser.go` first (guard nils/lengths + add missing `return` after `http.Get` err). These are the most user-visible "it just crashes" problems.
2. Refactor `subExecute` (or its contract) so that live output works without the bogus post-run `CombinedOutput` call.
3. Add defensive checks + proper error returns in the config and lookup helpers.
4. Expand test coverage for the error paths (and fix the live test guards).
5. Consider a small integration test that exercises a full "search + menu quit" path without actually spawning mpv (or with a stub).

These issues are all fixable with localized changes; the program is small and the core happy-path logic is reasonable.

---

*Generated from direct inspection of the source in `apps/gostations/`. Re-run `go test -short`, force error paths, and review `go vet` / `staticcheck` after fixes.*

## Implementation Status (as of reorg work)
- Phase 0/1 started: go.mod bumped, vendor/ removed, build scripts modernized (no forced vendor/GOPATH).
- Critical browser panics addressed structurally (new internal/radio with proper err returns, nil guards, cached host resolution, context+timeout http, no discarded rand sources).
- Verified: bad DNS now gives "warning: station search: resolve api host: ..." + graceful empty menu (no Intn or nil deref panic).
- subExecute bug: legacy path in internal/player now uses clean Run-only (no post-Run CombinedOutput); radiomenu updated to use it (no more guaranteed "[]").
- Quick wins: fixed unformatted config error msg and "Erorr" typo.
- Tests: fixed one inverted -short guard for Live tests; -short now cleanly passes units.
- Reorg skeleton: internal/{config,radio,player,version,data,ui} dirs + initial cleaned code + shims for compat. Stations/radiomenu wired to new paths.
- Next: complete phase1 (player integration, more test coverage, full removal of duplicated buggy code from root files), then TUI etc per PLAN.md.