Skip to content

Commit 2e354f2

Browse files
Cliveclaude
andcommitted
config v0.2: trim pasted credentials on load, name a bad config file
Two ways a config could look fine and not work, both found while writing the test suite. A whitespace-only Axle token passed is_configured() and then got a 401 from Axle, so setup reported the daemon ready and nothing worked. The trim is done on LOAD rather than at the check: trimming only at the check would fix the half nobody suffers from, leaving the daemon still sending the untrimmed value. _trim_pasted_fields() covers the four hand-pasted fields — the inverter IP, the Axle token and both Pushover keys — and nothing else, since a deliberate trailing space elsewhere is the user's business. A corrupt config.json raised a bare JSONDecodeError out of load_config() naming no file. It now raises ConfigError carrying the path, the reason, the line and column, and the command to run; main() catches it once around the dispatch and prints one line, because the exception is only half the fix and the print is the half the user sees. It deliberately never falls back to DEFAULTS. Starting on defaults after failing to read the user's config would drive the inverter to a 4 kW export target and a 10% reserve nobody chose, with only a log line to say so. Also refused now, each naming the path: valid JSON that is not an object (a bare list previously reached _deep_merge and died on .items()), a file that is not UTF-8, and one that cannot be read. A missing file is still not an error — that is the case the wizard starts from. 76 -> 93 tests, 7 mutations killed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 549dc4c commit 2e354f2

4 files changed

Lines changed: 310 additions & 22 deletions

File tree

‎config.py‎

Lines changed: 95 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,43 @@
55
# No Indigo, no Claude — plain Python, runs anywhere.
66
# Author: CliveS & Claude Opus 4.8
77
# Date: 15-06-2026
8-
# Version: 0.1
8+
# Version: 0.2
9+
#
10+
# v0.2 (20-09-2026) — two ways a config could look fine and not work:
11+
# * a token pasted with a trailing newline passed is_configured() and then got
12+
# a 401 from Axle. Trimming is done ON LOAD, not at the check, because
13+
# trimming only at the check would make setup say yes while the daemon still
14+
# sent the untrimmed value.
15+
# * a corrupt config.json raised a bare JSONDecodeError out of load_config()
16+
# naming no file. It now raises ConfigError with the path, the line and
17+
# column, and what to do about it.
918

1019
import json
1120
import os
1221

1322
CONFIG_FILENAME = "config.json"
1423

24+
25+
class ConfigError(Exception):
26+
"""A config.json that exists but cannot be used.
27+
28+
Carries the path and a plain-English reason so the daemon prints one line
29+
rather than a traceback. Deliberately NOT a fall back to defaults: running
30+
on DEFAULTS after failing to read the user's file would drive the inverter
31+
to a 4 kW export target and a 10% reserve that nobody chose, and the only
32+
sign would be a log line nobody reads. Refusing to start is the safe answer.
33+
"""
34+
35+
36+
# Values people paste by hand, which routinely arrive with a trailing newline or
37+
# a leading space when copied out of a browser or a password manager.
38+
_TRIM_FIELDS = (
39+
("inverter", "ip"),
40+
("vpp", "axle_token"),
41+
("notify", "pushover_token"),
42+
("notify", "pushover_user"),
43+
)
44+
1545
# Defaults. The setup wizard fills inverter.ip + vpp.axle_token (the only two the
1646
# user must supply) and auto-detects vpp.export_target_kw from the inverter where
1747
# it can. Everything else has a sensible default and is rarely touched.
@@ -60,20 +90,66 @@ def _deep_merge(base, override):
6090
return out
6191

6292

93+
def _trim_pasted_fields(cfg):
94+
"""Strip surrounding whitespace from the hand-pasted values, in place.
95+
96+
Done on LOAD so every consumer sees the clean value. Doing it in
97+
is_configured() instead would fix only the check: setup would report the
98+
daemon ready and Axle would still answer 401 for the untrimmed token.
99+
"""
100+
for section, key in _TRIM_FIELDS:
101+
sect = cfg.get(section)
102+
if isinstance(sect, dict) and isinstance(sect.get(key), str):
103+
sect[key] = sect[key].strip()
104+
return cfg
105+
106+
63107
def config_path(directory=None):
64108
"""Absolute path to config.json next to this script (or in `directory`)."""
65109
base = directory or os.path.dirname(os.path.abspath(__file__))
66110
return os.path.join(base, CONFIG_FILENAME)
67111

68112

69113
def load_config(path=None):
70-
"""Load config.json merged over DEFAULTS. Returns (config_dict, exists_bool)."""
114+
"""Load config.json merged over DEFAULTS. Returns (config_dict, exists_bool).
115+
116+
A MISSING file is normal — that is the (defaults, False) case the wizard
117+
starts from. A file that exists and cannot be used raises ConfigError, which
118+
names the path and says what to do; see that class for why this never falls
119+
back to defaults.
120+
"""
71121
path = path or config_path()
72122
if not os.path.exists(path):
73-
return _deep_merge(DEFAULTS, {}), False
74-
with open(path, encoding="utf-8") as fh:
75-
user = json.load(fh)
76-
return _deep_merge(DEFAULTS, user), True
123+
return _trim_pasted_fields(_deep_merge(DEFAULTS, {})), False
124+
125+
try:
126+
with open(path, encoding="utf-8") as fh:
127+
user = json.load(fh)
128+
except json.JSONDecodeError as exc:
129+
raise ConfigError(
130+
f"{path} is not valid JSON — {exc.msg}, line {exc.lineno} column {exc.colno}. "
131+
f"Fix it by hand, or delete it and run: python3 sigen_vpp.py --setup"
132+
) from exc
133+
except UnicodeDecodeError as exc:
134+
raise ConfigError(
135+
f"{path} is not readable as UTF-8 text ({exc.reason}). "
136+
f"Delete it and run: python3 sigen_vpp.py --setup"
137+
) from exc
138+
except OSError as exc:
139+
raise ConfigError(
140+
f"{path} could not be read — {exc.strerror}."
141+
) from exc
142+
143+
# Valid JSON is not necessarily a config: a bare list or string would reach
144+
# _deep_merge and die on .items() with a message about neither the file nor
145+
# the problem.
146+
if not isinstance(user, dict):
147+
raise ConfigError(
148+
f"{path} must hold a JSON object, not a {type(user).__name__}. "
149+
f"Delete it and run: python3 sigen_vpp.py --setup"
150+
)
151+
152+
return _trim_pasted_fields(_deep_merge(DEFAULTS, user)), True
77153

78154

79155
def save_config(cfg, path=None):
@@ -91,5 +167,16 @@ def save_config(cfg, path=None):
91167

92168

93169
def is_configured(cfg):
94-
"""True once the two required fields are present."""
95-
return bool(cfg["inverter"]["ip"]) and bool(cfg["vpp"]["axle_token"])
170+
"""True once the two required fields hold something other than whitespace.
171+
172+
load_config() has already trimmed them; this strips again so a config built
173+
by hand in a test or a script gets the same answer, and tolerates a section
174+
that a hand-edited file replaced with a scalar.
175+
"""
176+
def _filled(section, key):
177+
sect = cfg.get(section)
178+
if not isinstance(sect, dict):
179+
return False
180+
return bool(str(sect.get(key) or "").strip())
181+
182+
return _filled("inverter", "ip") and _filled("vpp", "axle_token")

‎sigen_vpp.py‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -473,14 +473,23 @@ def main():
473473
args = p.parse_args()
474474

475475
_setup_logging(args.verbose)
476-
if args.setup:
477-
return cmd_setup(args)
478-
if args.status:
479-
return cmd_status(args)
480-
if getattr(args, "test_export"):
481-
return cmd_test_export(args)
482-
if args.run:
483-
return cmd_run(args)
476+
477+
# Every command starts by loading config.json, so catch a bad one ONCE here
478+
# rather than in each of the four. ConfigError already reads as a finished
479+
# sentence naming the file and the fix, so it is printed as-is — a traceback
480+
# would bury it.
481+
try:
482+
if args.setup:
483+
return cmd_setup(args)
484+
if args.status:
485+
return cmd_status(args)
486+
if getattr(args, "test_export"):
487+
return cmd_test_export(args)
488+
if args.run:
489+
return cmd_run(args)
490+
except cfgmod.ConfigError as exc:
491+
print(f"Configuration problem: {exc}")
492+
return 1
484493
return 1
485494

486495

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
#! /usr/bin/env python
2+
# -*- coding: utf-8 -*-
3+
# Filename: tests/test_cli_reports_bad_config.py
4+
# Description: main() must turn a ConfigError into one readable line and exit 1,
5+
# for every command. The exception being raised is only half the fix;
6+
# this is the half the user actually sees.
7+
# Author: CliveS & Claude Opus 5
8+
# Date: 20-09-2026
9+
# Version: 1.0
10+
11+
import io
12+
import os
13+
import tempfile
14+
import unittest
15+
from contextlib import redirect_stdout
16+
from unittest.mock import patch
17+
18+
import config as cfgmod
19+
import sigen_vpp
20+
21+
22+
class TestCliReportsBadConfig(unittest.TestCase):
23+
def _run(self, argv):
24+
out = io.StringIO()
25+
with patch.object(sigen_vpp.sys, "argv", ["sigen_vpp.py"] + argv), \
26+
redirect_stdout(out):
27+
rc = sigen_vpp.main()
28+
return rc, out.getvalue()
29+
30+
def test_every_command_reports_it_rather_than_raising(self):
31+
# One try/except in main() covers all four; this is what proves it, and
32+
# what would fail if a fifth command were added outside the block.
33+
d = tempfile.mkdtemp()
34+
path = os.path.join(d, "config.json")
35+
with open(path, "w", encoding="utf-8") as fh:
36+
fh.write("{ not json")
37+
38+
with patch.object(cfgmod, "config_path", return_value=path):
39+
for cmd in ("--status", "--run", "--test-export", "--setup"):
40+
with self.subTest(cmd):
41+
rc, out = self._run([cmd])
42+
self.assertEqual(rc, 1)
43+
self.assertIn("Configuration problem", out)
44+
self.assertIn(path, out)
45+
self.assertNotIn("Traceback", out)
46+
47+
def test_the_message_tells_the_user_what_to_do(self):
48+
d = tempfile.mkdtemp()
49+
path = os.path.join(d, "config.json")
50+
with open(path, "w", encoding="utf-8") as fh:
51+
fh.write("[1, 2]")
52+
with patch.object(cfgmod, "config_path", return_value=path):
53+
rc, out = self._run(["--status"])
54+
self.assertEqual(rc, 1)
55+
self.assertIn("--setup", out)
56+
57+
def test_a_missing_config_still_gets_its_own_friendlier_message(self):
58+
# Absent is not corrupt — it must keep saying "run --setup", not
59+
# "Configuration problem".
60+
d = tempfile.mkdtemp()
61+
path = os.path.join(d, "config.json")
62+
with patch.object(cfgmod, "config_path", return_value=path):
63+
rc, out = self._run(["--status"])
64+
self.assertEqual(rc, 1)
65+
self.assertNotIn("Configuration problem", out)
66+
self.assertIn("--setup", out)
67+
68+
69+
if __name__ == "__main__":
70+
unittest.main()

‎tests/test_config.py‎

Lines changed: 128 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,11 @@
22
# -*- coding: utf-8 -*-
33
# Filename: tests/test_config.py
44
# Description: Unit tests for config.py — defaults, deep merge, load/save round trip
5-
# and the 0600 permissions that protect the Axle token.
5+
# the 0600 permissions that protect the Axle token, and the two
6+
# v0.2 fixes: whitespace trimming and a named ConfigError.
67
# Author: CliveS & Claude Opus 5
78
# Date: 20-09-2026
8-
# Version: 1.0
9+
# Version: 1.1
910

1011
import json
1112
import os
@@ -84,6 +85,112 @@ def test_a_partial_file_never_loses_a_required_key(self):
8485
self.assertIn(key, cfg[section], f"{section}.{key} lost by the merge")
8586

8687

88+
class TestTrimsPastedFields(unittest.TestCase):
89+
"""A token copied out of a browser routinely carries a trailing newline.
90+
Trimming happens on LOAD, not at the check — trimming only at the check would
91+
let setup report ready while the daemon still sent the untrimmed value."""
92+
93+
def _loaded(self, user):
94+
with tempfile.TemporaryDirectory() as d:
95+
p = os.path.join(d, "config.json")
96+
with open(p, "w", encoding="utf-8") as fh:
97+
json.dump(user, fh)
98+
return cfgmod.load_config(p)[0]
99+
100+
def test_the_token_and_ip_are_trimmed(self):
101+
cfg = self._loaded({"inverter": {"ip": " 192.168.1.50\n"},
102+
"vpp": {"axle_token": "\ttok "}})
103+
self.assertEqual(cfg["inverter"]["ip"], "192.168.1.50")
104+
self.assertEqual(cfg["vpp"]["axle_token"], "tok")
105+
106+
def test_pushover_keys_are_trimmed_too(self):
107+
cfg = self._loaded({"notify": {"pushover_token": " a\n", "pushover_user": "b "}})
108+
self.assertEqual(cfg["notify"]["pushover_token"], "a")
109+
self.assertEqual(cfg["notify"]["pushover_user"], "b")
110+
111+
def test_a_whitespace_only_token_becomes_empty_so_the_check_fails(self):
112+
cfg = self._loaded({"inverter": {"ip": "192.168.1.50"},
113+
"vpp": {"axle_token": " \n"}})
114+
self.assertEqual(cfg["vpp"]["axle_token"], "")
115+
self.assertFalse(cfgmod.is_configured(cfg))
116+
117+
def test_non_string_values_are_left_alone(self):
118+
cfg = self._loaded({"inverter": {"ip": 123}})
119+
self.assertEqual(cfg["inverter"]["ip"], 123)
120+
121+
def test_other_fields_are_not_trimmed(self):
122+
# Only the hand-pasted ones. A deliberate trailing space elsewhere is the
123+
# user's business.
124+
cfg = self._loaded({"web": {"bind": " 0.0.0.0 "}})
125+
self.assertEqual(cfg["web"]["bind"], " 0.0.0.0 ")
126+
127+
128+
class TestConfigError(unittest.TestCase):
129+
"""A config that exists but cannot be used must name itself and refuse to
130+
start. It must NEVER fall back to DEFAULTS — that would silently drive the
131+
inverter to a 4 kW target and a 10% reserve nobody chose."""
132+
133+
def _load(self, text, mode="w"):
134+
d = tempfile.mkdtemp()
135+
p = os.path.join(d, "config.json")
136+
with open(p, mode) as fh:
137+
fh.write(text)
138+
return p
139+
140+
def test_malformed_json_names_the_file_and_the_place(self):
141+
p = self._load('{"vpp": {"axle_token": "abc",}')
142+
with self.assertRaises(cfgmod.ConfigError) as cm:
143+
cfgmod.load_config(p)
144+
msg = str(cm.exception)
145+
self.assertIn(p, msg)
146+
self.assertIn("line 1", msg)
147+
self.assertIn("--setup", msg)
148+
149+
def test_the_original_error_is_kept_as_the_cause(self):
150+
p = self._load("{not json")
151+
with self.assertRaises(cfgmod.ConfigError) as cm:
152+
cfgmod.load_config(p)
153+
self.assertIsInstance(cm.exception.__cause__, json.JSONDecodeError)
154+
155+
def test_valid_json_that_is_not_an_object_is_refused(self):
156+
for label, text in (("a list", "[1, 2]"), ("a string", '"hello"'),
157+
("null", "null"), ("a number", "42")):
158+
with self.subTest(label):
159+
p = self._load(text)
160+
with self.assertRaises(cfgmod.ConfigError) as cm:
161+
cfgmod.load_config(p)
162+
self.assertIn("JSON object", str(cm.exception))
163+
164+
def test_a_file_that_is_not_utf8_is_refused(self):
165+
p = self._load(b"\xff\xfe{}", mode="wb")
166+
with self.assertRaises(cfgmod.ConfigError) as cm:
167+
cfgmod.load_config(p)
168+
self.assertIn("UTF-8", str(cm.exception))
169+
170+
def test_an_unreadable_file_is_refused_with_its_reason(self):
171+
p = self._load("{}")
172+
os.chmod(p, 0o000)
173+
self.addCleanup(os.chmod, p, 0o600)
174+
if os.geteuid() == 0:
175+
self.skipTest("running as root — permissions are not enforced")
176+
with self.assertRaises(cfgmod.ConfigError) as cm:
177+
cfgmod.load_config(p)
178+
self.assertIn(p, str(cm.exception))
179+
180+
def test_it_never_silently_falls_back_to_defaults(self):
181+
# THE point. A corrupt file returning (DEFAULTS, True) would start the
182+
# daemon on an export target and reserve floor the user never chose.
183+
p = self._load("{ broken")
184+
with self.assertRaises(cfgmod.ConfigError):
185+
cfgmod.load_config(p)
186+
187+
def test_a_missing_file_is_still_not_an_error(self):
188+
with tempfile.TemporaryDirectory() as d:
189+
cfg, existed = cfgmod.load_config(os.path.join(d, "absent.json"))
190+
self.assertFalse(existed)
191+
self.assertEqual(cfg["vpp"]["export_target_kw"], 4.0)
192+
193+
87194
class TestSaveConfig(unittest.TestCase):
88195
def test_round_trip(self):
89196
with tempfile.TemporaryDirectory() as d:
@@ -132,10 +239,25 @@ def test_needs_both(self):
132239
self.assertEqual(cfgmod.is_configured(self._cfg(ip, token)), want)
133240

134241
def test_whitespace_is_not_a_token(self):
135-
# Documents CURRENT behaviour: a config whose token is a stray space passes
136-
# is_configured() and then fails at Axle with a 401. Flagged, not silently
137-
# "fixed" here — changing it is a behaviour change, not a test.
138-
self.assertTrue(cfgmod.is_configured(self._cfg("192.168.1.50", " ")))
242+
# Was a real flaw until v0.2: a token of stray whitespace passed the check
243+
# and then got a 401 from Axle, so setup said ready and nothing worked.
244+
for label, ip, token in (
245+
("space token", "192.168.1.50", " "),
246+
("newline token", "192.168.1.50", "\n"),
247+
("tab ip", "\t", "tok"),
248+
):
249+
with self.subTest(label):
250+
self.assertFalse(cfgmod.is_configured(self._cfg(ip, token)))
251+
252+
def test_a_non_dict_section_is_not_configured(self):
253+
# A hand-edited file can replace a whole section with a scalar. Indexing
254+
# it would raise AttributeError out of a boolean check.
255+
self.assertFalse(cfgmod.is_configured({"inverter": "", "vpp": {"axle_token": "t"}}))
256+
self.assertFalse(cfgmod.is_configured({}))
257+
258+
def test_a_non_string_value_does_not_explode(self):
259+
self.assertTrue(cfgmod.is_configured(
260+
{"inverter": {"ip": 19216815}, "vpp": {"axle_token": "tok"}}))
139261

140262

141263
if __name__ == "__main__":

0 commit comments

Comments
 (0)