Round-up: 11 low-priority findings from the full-repository review

[LarsLaskowski]

Summary

This issue collects 11 low-priority findings from a full review of the repository. None warrants its own issue: each is small, none is user-visible as a bug, and several are judgement calls that may reasonably be declined.

Treat each item as independently actionable. Do not attempt all eleven in one pull request — they touch unrelated files and would be unreviewable together. Pick one or a few closely related items, open a focused PR, and tick the box here. Feel free to close individual items as "won't do" with a short reason; that is a perfectly good outcome for several of these.

Each item states what it is, why it is low priority, and what a fix would look like.

---

Security

- [ ] 1. CSP is missing base-uri, form-action and object-src

internal/httpapi/middleware.go, withSecurityHeaders:

h.Set("Content-Security-Policy",
    "default-src 'self'; img-src 'self' data:; script-src 'self'; style-src 'self'; frame-ancestors 'none'")

base-uri and form-action are not covered by default-src, so neither is currently restricted. object-src is covered by default-src 'self' but is conventionally set to 'none' explicitly.

Why low: these harden against injected markup, and the dashboard has an enforced no-innerHTML rule (internal/web/xss_test.go) plus a <form> with no action that never actually submits. There is no known injection vector to defend against.

Fix: append ; base-uri 'none'; form-action 'self'; object-src 'none'. Update the expected header string in TestSecurityHeaders_SetOnAllResponses (internal/httpapi/middleware_test.go), which asserts the exact value, and verify in a browser that the API-key dialog still works.

- [ ] 2. Release artifacts are checksummed but not signed; goreleaser-action is unpinned

.github/workflows/release.yml pins actions/checkout and actions/setup-go to commit SHAs but uses version: latest for GoReleaser itself. .goreleaser.yaml produces checksums.txt with no signature.

Why low: the checksum file defends against corrupted downloads, and latest for GoReleaser is a common trade-off. Signing (cosign keyless / Sigstore, or SLSA provenance via goreleaser/goreleaser-action's built-in support) is meaningful mainly once the project has enough users for supply-chain attacks to be worth mounting.

Fix: pin the GoReleaser version, and optionally add cosign signing of checksums.txt. If signing is added, document verification in the README install steps — an unverifiable signature helps nobody.

---

Performance

- [ ] 3. gzip is applied even to tiny responses

withGzip compresses every response to a client advertising gzip, including GET /api/v1/config (~300 bytes). Below roughly 1 KB the gzip framing plus the CPU cost exceeds the saving.

Why low: /api/v1/config is fetched once per page load. The endpoints that are actually polled (metrics, metrics/history) are well above any sensible threshold and benefit greatly.

Fix: buffer the first write and only engage gzip past a threshold (~1 KB), or exempt /api/v1/config from the middleware. The buffering approach is more correct but more code; note that Content-Encoding is currently set before the handler runs, so a threshold implementation must defer that header until the size is known. Weigh whether the complexity is worth it — declining this item is defensible.

- [ ] 4. CPUFreqCollector re-globs sysfs on every fast tick

internal/collector/cpufreq.go, Collect() runs filepath.Glob("/sys/devices/system/cpu/cpu[0-9]*/cpufreq") plus two os.ReadFile calls per core, every poll_interval_seconds. On a Pi 4 that is one glob and eight reads every 5 seconds.

Why low: sysfs reads are cheap and served from memory.

Fix: cache the directory list and re-glob only occasionally (mirroring the detectRetryInterval pattern already used by TemperatureCollector and vcgencmdRunner). Careful: CPU cores can genuinely go offline and online at runtime, so the cache must expire — do not resolve the list once at startup. Given that constraint, the saving may not justify the added state; measure first.

- [ ] 5. statfsWithTimeout spawns a goroutine, channel and timer per mountpoint per tick

internal/collector/disk.go runs every statfs call in its own goroutine with a watchdog timer, even though the overwhelmingly common case returns in microseconds.

Why low: goroutines are cheap, and the watchdog exists for a good reason (a hung mount must not stall collection — see the defaultStatfsTimeout and badUntil comments). This is deliberate, documented defensive design.

Fix (if any): call statfs inline and only escalate to the watchdog path for mountpoints that have previously been slow. This makes the fast path faster but the dangerous path riskier — the first hang on a new mount would then block the tick for real. The existing design is arguably correct; this item is listed for completeness, and "won't do" is a reasonable answer.

- [ ] 6. Webhook responses are not drained before Close()

internal/alert/notify.go, post():

defer func() { _ = resp.Body.Close() }()

Go's HTTP transport only returns a connection to the idle pool if the body is read to EOF. Closing without draining means every webhook delivery opens a fresh TCP (and TLS) connection.

Why low: webhook deliveries are rare by design (debounced, rate-limited).

Fix: before closing, _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 64<<10)). The LimitReader matters — an unbounded copy from a hostile or broken endpoint is its own problem. io is already imported in the package.

---

Architecture

- [ ] 7. internal/collector imports internal/config

collector.Config embeds config.Thresholds, so a low-level metric-source package depends on the application-configuration package. The natural direction is the reverse.

Why low: it is a single value type with no behaviour, and the coupling causes no practical problem today.

Fix: move the threshold type into internal/alert (which owns the >= semantics) and have both config and collector depend on that. This overlaps directly with the separate issue on threshold duplication across four places — check whether that one is in progress and coordinate, or the two will conflict.

- [ ] 8. collector.Collector is a god object

At ~505 lines it owns sampling orchestration, the latest snapshot, all history ring buffers, disk persistence, alert evaluation and notifier fan-out.

Why low: it is well-organised, thoroughly commented and 90.8 % covered. This is a readability concern, not a defect.

Fix (if pursued): extract history + persistence into its own type (historyStore) holding the ring buffer maps, Snapshot/Fill, eviction and load/persist. Collector would then own sampling and delegate. This is a substantial refactor of well-tested code — only worth doing if the file is becoming an obstacle in practice. Do not start it speculatively.

- [ ] 9. writeFileAtomic does not fsync the parent directory

internal/collector/persist.go syncs the temp file before renaming, but not the containing directory afterwards. On most Linux filesystems a power cut immediately after os.Rename can therefore lose the rename even though the contents were durable.

Why low: the history file is disposable derived state — loadHistory treats a missing file as a normal first start.

Fix: after os.Rename, open the parent directory and Sync() it, ignoring errors on platforms that do not support it. Note the interaction with the async-persistence issue, which also modifies this function.

- [ ] 10. Notifier.lastSent grows without bound

internal/alert/notify.go: each webhookWorker keeps lastSent map[string]time.Time keyed by (metric, resource). Nothing ever removes entries, so a system where mountpoints churn (containers, removable media) accumulates keys for the process lifetime.

Why low: an entry is a short string plus a time.Time, and a key is only created when an alert actually fires for that resource — not merely when the device is seen. Reaching a meaningful size requires a pathological workload.

Fix: prune entries older than a few multiples of minInterval when the map exceeds some size, or on each dispatch. The collector already has evictStaleSeries and the engine has pruneDisks; this is the one place with the same shape and no cleanup, which is the real argument for fixing it — consistency.

---

Documentation

- [ ] 11. AGENTS.md points at a directory that does not exist, and the skill files have drifted

AGENTS.md states:

Project-specific Codex skills live under .Codex/skills/

No such directory exists. The repository has .agents/skills/ and .claude/skills/, each containing create-pr, fix-issue and review-pr. All three files differ between the two directories, with no stated source of truth:

$ diff -q .agents/skills/create-pr/SKILL.md .claude/skills/create-pr/SKILL.md
Files ... differ    # same for fix-issue and review-pr

Fix: correct the path in AGENTS.md to .agents/skills/. Then decide deliberately whether the two trees should be identical: if yes, reconcile them and consider a guard test in the style of internal/web/xss_test.go asserting they match; if the divergence is intentional (different agent capabilities), say so in both AGENTS.md and CLAUDE.md so the next reader is not left guessing.

Also in scope while there: docs/CONTRIBUTING.md says contributors should "run golangci-lint run … CI enforces the same configuration", but no .golangci.yml is committed — both local and CI runs use the tool's built-in defaults. Either commit a config (making the claim true and the rule set explicit) or reword to say the project relies on golangci-lint's defaults at the version CI pins.

---

Testing requirements

Per docs/TESTS.md, tests are mandatory for every item that changes behaviour — items 1, 3, 4, 5, 6, 7, 8, 9, 10. Items 2 and 11 are configuration and documentation; state explicitly in those PRs that no Go tests apply rather than leaving the checklist ambiguous.

Note especially:

• Item 1 breaks the existing TestSecurityHeaders_SetOnAllResponses, which asserts the exact CSP string.
• Items 6, 9 and 10 touch concurrent code — run go test ./... -race.
• Item 7 changes package boundaries; expect import updates across collector, config and their tests.

How to work through this

One PR per item (or per closely-related pair), each referencing this issue. Tick the box above as each lands, and close the issue once every item is either done or explicitly declined.

---

⚠️ Note on this issue

This issue was drafted by an AI code review of the repository. What is written here is not law — the analysis was produced by an AI and may contain mistakes, including in its severity judgements and in its claims about what the current code does. Several items here are explicitly matters of taste where declining is the right call.

Before implementing any item, first verify that it is still factually correct. Implementation may happen considerably later than this issue was written, and the source may have been changed in the meantime by unrelated work (or an item may already be fixed). Re-read the referenced files at their current state, confirm the described behaviour still holds, and adjust — or tick off as no-longer-applicable — anything that has moved on.