From 31a98f511d7121413554f764943d7d0b57e9e230 Mon Sep 17 00:00:00 2001
From: atlas <atlas@hyperhive.local>
Date: Wed, 2 Sep 2026 13:26:12 +0200
Subject: [PATCH] hive-agent: pin what a zero means for each turn-loop knob
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

turn.rs has no tests. The knobs read env through two helpers and the
difference between them is load-bearing: env_u64 keeps a parsed 0, so
HIVE_TURN_IDLE_SECS=0 disables the watchdog and
HIVE_AUTO_RESET_WATERMARK_TOKENS=0 disables auto-reset; env_u64_positive
discards it, so a 0 sleep or cache TTL falls back to the default. Calling
the wrong one is a one-word edit that changes whether 0 turns a feature
off or does nothing.

Splitting the parse and zero-rejection halves out of the env lookup makes
both testable — std::env is process-global, so the lookup itself is not
safely settable from a threaded test runner, and the env-to-knob mapping
stays uncovered for that reason.

No behaviour change: each of the four knobs already agreed with its own
doc comment.
---
 hive-agent/src/turn.rs | 71 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 4 deletions(-)

diff --git a/hive-agent/src/turn.rs b/hive-agent/src/turn.rs
index 903fffec..69929dc5 100644
--- a/hive-agent/src/turn.rs
+++ b/hive-agent/src/turn.rs
@@ -200,15 +200,29 @@ pub enum TurnError {
 /// Parse an env var as `u64`, ignoring absent / blank / unparseable values.
 /// Returns the raw value including `0` (several knobs use `0` as "disable").
 fn env_u64(name: &str) -> Option<u64> {
-    std::env::var(name)
-        .ok()
-        .and_then(|s| s.trim().parse::<u64>().ok())
+    parse_u64(std::env::var(name).ok().as_deref())
+}
+
+/// The parsing half of [`env_u64`], split from the lookup so it can be
+/// exercised without setting a process-global variable.
+fn parse_u64(raw: Option<&str>) -> Option<u64> {
+    raw.and_then(|s| s.trim().parse::<u64>().ok())
 }
 
 /// Like [`env_u64`] but also rejects `0`, falling back to `default` — for
 /// knobs where `0` is meaningless rather than a "disable" sentinel.
 fn env_u64_positive(name: &str, default: u64) -> u64 {
-    env_u64(name).filter(|&v| v > 0).unwrap_or(default)
+    nonzero_or(env_u64(name), default)
+}
+
+/// The `0`-rejecting half of [`env_u64_positive`].
+///
+/// Which of the two a knob uses is the whole distinction: a knob reading
+/// [`env_u64`] treats `0` as "disable" and keeps it, one reading
+/// [`env_u64_positive`] treats `0` as nonsense and falls back. Picking the
+/// wrong one silently changes when the loop sleeps, compacts or resets.
+fn nonzero_or(parsed: Option<u64>, default: u64) -> u64 {
+    parsed.filter(|&v| v > 0).unwrap_or(default)
 }
 
 /// How long to sleep after a rate-limit before re-entering the serve loop.
@@ -726,3 +740,52 @@ fn archive_session(bus: &Bus) {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{nonzero_or, parse_u64};
+
+    #[test]
+    fn an_absent_or_blank_value_is_not_a_number() {
+        assert_eq!(parse_u64(None), None);
+        assert_eq!(parse_u64(Some("")), None);
+        assert_eq!(parse_u64(Some("   ")), None);
+    }
+
+    #[test]
+    fn surrounding_whitespace_is_trimmed() {
+        assert_eq!(parse_u64(Some("12")), Some(12));
+        assert_eq!(parse_u64(Some("  12\n")), Some(12));
+    }
+
+    #[test]
+    fn a_value_that_is_not_a_u64_is_rejected_rather_than_coerced() {
+        for raw in ["abc", "1.5", "-1", "12s", "0x10"] {
+            assert_eq!(parse_u64(Some(raw)), None, "for {raw}");
+        }
+        // One past u64::MAX: rejected, not wrapped or saturated.
+        assert_eq!(parse_u64(Some("18446744073709551616")), None);
+        assert_eq!(parse_u64(Some("18446744073709551615")), Some(u64::MAX));
+    }
+
+    #[test]
+    fn nonzero_or_falls_back_when_there_is_no_value() {
+        assert_eq!(nonzero_or(None, 9), 9);
+        assert_eq!(nonzero_or(Some(5), 9), 5);
+    }
+
+    /// The distinction the two helpers exist for. A knob reading the raw
+    /// parse keeps `0` and treats it as "disable"; one reading
+    /// `nonzero_or` discards it and uses its default. Swapping which
+    /// helper a knob calls is a one-word edit that silently changes
+    /// whether `0` turns the feature off or does nothing at all.
+    #[test]
+    fn zero_survives_parsing_but_is_rejected_as_a_positive_knob() {
+        assert_eq!(parse_u64(Some("0")), Some(0), "kept: 0 can mean disable");
+        assert_eq!(
+            nonzero_or(parse_u64(Some("0")), 7),
+            7,
+            "rejected: 0 is not a duration"
+        );
+    }
+}