Read the environment with typed parsers and apply the documented defaults

set_defaults() assigned os.getenv() results directly, which overwrote the
argparse defaults with None whenever a variable was unset. A default container
therefore ran "seaf-cli config -k delete_confirm_threshold -v None", and the
same for upload_limit and download_limit: none of the three documented
defaults were ever applied.

SEAFILE_UID and SEAFILE_GID had a second problem. argparse type=int applies
only to command line flags, never to set_defaults, so both stayed strings.
The check in setup_uid compares them against pwd integers, and a string is
never equal to one, so usermod ran on every start.

dsc/config.py reads each variable with its documented default and rejects a
malformed or negative value at startup, naming the variable, instead of
passing it on. An empty variable counts as unset.

Booleans are now rendered as true and false rather than Python's True and
False. The daemon reads disable_verify_certificate with its boolean accessor,
which does not recognise the capitalised form. I could not run the daemon to
confirm the previous value's effect, since the AppImage is x86_64 only.

seaf-cli is no longer given a None value for any option.

IMPROVEMENTS.md

@@ -84,12 +84,19 @@ Acceptance criteria:
 
 ### Correct environment parsing and defaults
 
-- [ ] Replace direct `os.getenv()` assignments in `start.py` with typed parsers.
+- [x] Replace direct `os.getenv()` assignments in `start.py` with typed parsers.
-- [ ] Preserve the documented defaults for upload limit, download limit, and
+  See `dsc/config.py` and `defaults_from_env()`.
-  delete-confirm threshold when variables are absent.
+- [x] Preserve the documented defaults for upload limit, download limit, and
-- [ ] Validate UID, GID, limits, threshold, and booleans before daemon startup.
+  delete-confirm threshold when variables are absent. They were not: an unset
-- [ ] Reject negative and malformed numeric values with useful messages.
+  variable overwrote the argparse default with `None`, so a default container
-- [ ] Do not pass `None` as a Seafile configuration value.
+  sent the literal string `None` for all three.
+- [x] Validate UID, GID, limits, threshold, and booleans before daemon startup.
+  UID and GID are now integers; as strings they never equalled the integers
+  they were compared against, so `usermod` ran on every start.
+- [x] Reject negative and malformed numeric values with useful messages.
+- [x] Do not pass `None` as a Seafile configuration value. Booleans are also
+  rendered as `true`/`false` rather than Python's `True`/`False`, since the
+  daemon reads that setting with its boolean accessor.
 
 Acceptance criteria:
 

README.md

@@ -46,10 +46,15 @@ volumes:
  - `DELETE_CONFIRM_THRESHOLD` - represents the number of files that require
  confirmation before being deleted simultaneously. Default value is _500_.
  - `DISABLE_VERIFY_CERTIFICATE` - set to _true_ to disable server's certificate
- verification. Default value is _false_.
+ verification. Default value is _false_. Booleans accept _true_/_false_,
+ _1_/_0_, _yes_/_no_ and _on_/_off_, in any case.
  - `UPLOAD_LIMIT` / `DOWNLOAD_LIMIT` -  upload/download speed limit in B/s
  (bytes per second). Default values are _0_ (unlimited).
 
+Numeric variables must be whole, non-negative numbers. A malformed or negative
+ value stops the container at startup with a message naming the variable,
+ rather than being passed on to the Seafile client.
+
 ### Using Docker secrets:
 Mount the secret and point `PASSWORD_FILE` at it, so the password never enters
  the environment:

dsc/client.py

@@ -14,7 +14,7 @@ import seafile
 
 from dsc import const
 from dsc.errors import ConfigError, DaemonError, DaemonTimeout
-from dsc.misc import create_dir, hide_secrets, user_cmd
+from dsc.misc import config_value, create_dir, hide_secrets, user_cmd
 
 _lg = logging.getLogger(__name__)
 
@@ -304,6 +304,9 @@ class SeafileClient:
             if key not in const.AVAILABLE_SEAFCLI_OPTIONS:
                 continue
 
+            if value is None:
+                raise ConfigError(f"No value for seafile option {key}")
+
             # check current value
             cmd = ["seaf-cli", "config", "-k", key]
             _lg.info("Checking seafile client option: %s", " ".join(cmd))
@@ -314,11 +317,11 @@ class SeafileClient:
                 cur_value = cur_value.split(sep="=")[1].strip()
             except IndexError:
                 cur_value = None
-            if cur_value == str(value):
+            if cur_value == config_value(value):
                 continue
 
             # set new value
-            cmd = ["seaf-cli", "config", "-k", key, "-v", str(value)]
+            cmd = ["seaf-cli", "config", "-k", key, "-v", config_value(value)]
             _lg.info("Setting seafile client option: %s", " ".join(cmd))
             self.__run(cmd)
             need_restart = True

dsc/config.py

@@ -0,0 +1,65 @@
+"""Typed reading of the container's environment.
+
+Everything in the environment is text, including values the client later
+compares as numbers or passes to seaf-cli. Reading them directly leaves two
+failures that are silent until much later: an unset variable becomes None and
+reaches seaf-cli as the string "None", and a numeric setting stays a string
+that never equals the integer it is compared against.
+"""
+
+import os
+
+from dsc.errors import ConfigError
+
+_TRUE_VALUES = {"true", "1", "yes", "on"}
+_FALSE_VALUES = {"false", "0", "no", "off"}
+
+
+def env_str(name: str, default: str = None) -> str:
+    """Return a text setting. An empty variable counts as unset."""
+    value = os.getenv(name)
+    if value is None or value == "":
+        return default
+    return value
+
+
+def env_int(name: str, default: int, minimum: int = 0) -> int:
+    """
+    Return a numeric setting as an int, never as text and never as None.
+
+    Values below ``minimum`` are rejected: a negative speed limit or deletion
+    threshold is not something to pass on to seaf-cli and hope for the best.
+    """
+    raw = env_str(name)
+    if raw is None:
+        return default
+
+    stripped = raw.strip()
+    try:
+        value = int(stripped, 10)
+    except ValueError:
+        raise ConfigError(
+            f"{name} must be a whole number, got {stripped!r}"
+        )
+
+    if value < minimum:
+        raise ConfigError(
+            f"{name} must be {minimum} or greater, got {stripped!r}"
+        )
+    return value
+
+
+def env_bool(name: str, default: bool = False) -> bool:
+    """Return a boolean setting, accepting the usual spellings."""
+    raw = env_str(name)
+    if raw is None:
+        return default
+
+    value = raw.strip().lower()
+    if value in _TRUE_VALUES:
+        return True
+    if value in _FALSE_VALUES:
+        return False
+
+    accepted = ", ".join(sorted(_TRUE_VALUES | _FALSE_VALUES))
+    raise ConfigError(f"{name} must be one of {accepted}, got {raw.strip()!r}")

dsc/misc.py

@@ -65,3 +65,15 @@ def hide_secrets(cmd: list, *secrets: str) -> list:
             if secret in arg:
                 cmd[i] = arg.replace(secret, "********")
     return cmd
+
+
+def config_value(value) -> str:
+    """
+    Render a setting the way seaf-cli stores it.
+
+    Booleans are written as "true" and "false": the daemon reads this setting
+    with its boolean accessor, and Python's str() would produce "True".
+    """
+    if isinstance(value, bool):
+        return "true" if value else "false"
+    return str(value)

start.py

@@ -7,6 +7,7 @@ import signal
 import sys
 
 from dsc import SeafileClient, const
+from dsc.config import env_bool, env_int, env_str
 from dsc.errors import ConfigError, DscError, GracefulShutdown
 from dsc.misc import setup_uid, create_dir
 from dsc.paths import plan_lib_dirs
@@ -26,6 +27,30 @@ def install_signal_handlers():
         signal.signal(signum, handle_signal)
 
 
+def defaults_from_env() -> dict:
+    """
+    Read every setting from the environment with its documented default.
+
+    Reading these directly leaves an unset variable as None, which reaches
+    seaf-cli as the string "None" and silently replaces the documented value.
+    """
+    return dict(
+        server=env_str("SERVER_HOST"),
+        username=env_str("USERNAME"),
+        password=env_str("PASSWORD"),
+        password_file=env_str("PASSWORD_FILE"),
+        token=env_str("TOKEN"),
+        token_file=env_str("TOKEN_FILE"),
+        libraries=env_str("LIBRARY_ID"),
+        uid=env_int("SEAFILE_UID", 1000),
+        gid=env_int("SEAFILE_GID", 1000),
+        upload_limit=env_int("UPLOAD_LIMIT", 0),
+        download_limit=env_int("DOWNLOAD_LIMIT", 0),
+        delete_confirm_threshold=env_int("DELETE_CONFIRM_THRESHOLD", 500),
+        disable_verify_certificate=env_bool("DISABLE_VERIFY_CERTIFICATE", False),
+    )
+
+
 def resolve_libraries(client, requested: str, server: str) -> set:
     """Map the requested names or IDs to library IDs that are not synced yet."""
     libs_to_sync = set()
@@ -94,21 +119,11 @@ def main():
     parser.add_argument("--disable-verify-certificate", action="store_true")
     parser.add_argument("--delete-confirm-threshold", type=int, default=500)
 
-    parser.set_defaults(
+    try:
-        server=os.getenv("SERVER_HOST"),
+        parser.set_defaults(**defaults_from_env())
-        username=os.getenv("USERNAME"),
+    except ConfigError as err:
-        password=os.getenv("PASSWORD"),
+        _lg.error("%s", err)
-        password_file=os.getenv("PASSWORD_FILE"),
+        return 2
-        token=os.getenv("TOKEN"),
-        token_file=os.getenv("TOKEN_FILE"),
-        libraries=os.getenv("LIBRARY_ID"),
-        uid=os.getenv("SEAFILE_UID", default=1000),
-        gid=os.getenv("SEAFILE_GID", default=1000),
-        upload_limit=os.getenv("UPLOAD_LIMIT"),
-        download_limit=os.getenv("DOWNLOAD_LIMIT"),
-        disable_verify_certificate=os.getenv("DISABLE_VERIFY_CERTIFICATE") in ("true", "1", "True"),
-        delete_confirm_threshold=os.getenv("DELETE_CONFIRM_THRESHOLD"),
-    )
     args = parser.parse_args()
     if not args.server:
         parser.error("Seafile server is not specified")

tests/test_commands.py

@@ -8,7 +8,7 @@ import pytest
 
 from dsc import client as client_module
 from dsc import const
-from dsc.errors import DaemonError
+from dsc.errors import ConfigError, DaemonError
 from dsc.client import SeafileClient
 from dsc.misc import hide_secrets, user_cmd
 
@@ -235,3 +235,33 @@ def test_local_libraries_error_keeps_the_original_cause(client, monkeypatch):
     with pytest.raises(DaemonError) as err:
         client.get_local_libraries()
     assert err.value.__cause__ is not None
+
+
+def test_configure_never_sends_none_as_a_value(client, calls):
+    """A None here reaches seaf-cli as the literal string "None"."""
+    with pytest.raises(ConfigError):
+        client.configure(argparse.Namespace(upload_limit=None), check_for_daemon=False)
+
+    for argv, _ in calls:
+        assert "None" not in argv
+
+
+@pytest.mark.parametrize("value, expected", [
+    (True, "true"),
+    (False, "false"),
+    (0, "0"),
+    (500, "500"),
+])
+def test_configure_renders_values_the_way_seaf_cli_stores_them(
+        client, calls, monkeypatch, value, expected):
+    # Changing an option restarts the daemon; keep this on the value only.
+    monkeypatch.setattr(client, "stop_daemon", lambda: None)
+    monkeypatch.setattr(client, "start_daemon", lambda: None)
+
+    client.configure(argparse.Namespace(disable_verify_certificate=value)
+                     if isinstance(value, bool)
+                     else argparse.Namespace(upload_limit=value),
+                     check_for_daemon=False)
+
+    setting = [argv for argv, _ in calls if "-v" in argv][-1]
+    assert setting[setting.index("-v") + 1] == expected

tests/test_config.py

@@ -0,0 +1,120 @@
+"""The environment is untyped text; everything derived from it is checked."""
+
+import pytest
+
+import start
+from dsc.config import env_bool, env_int, env_str
+from dsc.errors import ConfigError
+
+
+@pytest.fixture
+def env(monkeypatch):
+    def set_env(**values):
+        for name, value in values.items():
+            if value is None:
+                monkeypatch.delenv(name, raising=False)
+            else:
+                monkeypatch.setenv(name, value)
+    return set_env
+
+
+@pytest.mark.parametrize("raw, expected", [
+    (None, 7),        # unset keeps the documented default
+    ("", 7),          # an empty variable is the same as unset
+    ("0", 0),         # zero is a real value, not "missing"
+    ("1", 1),
+    ("4096", 4096),
+    (" 42 ", 42),
+    # int() accepts any Unicode decimal digit and normalises it; the value
+    # leaves as an ASCII integer, so this is harmless rather than rejected.
+    ("\u0663", 3),
+])
+def test_env_int_accepts(env, raw, expected):
+    env(LIMIT=raw)
+    assert env_int("LIMIT", 7) == expected
+
+
+@pytest.mark.parametrize("raw", ["-1", "-0.5", "abc", "1.5", "1e3", "0x10", "1 2", "٣ x"])
+def test_env_int_rejects(env, raw):
+    env(LIMIT=raw)
+    with pytest.raises(ConfigError) as err:
+        env_int("LIMIT", 7)
+    assert "LIMIT" in str(err.value)
+    assert raw.strip() in str(err.value)
+
+
+def test_env_int_allows_a_negative_value_when_permitted(env):
+    env(LIMIT="-1")
+    assert env_int("LIMIT", 7, minimum=-1) == -1
+
+
+@pytest.mark.parametrize("raw, expected", [
+    (None, False),
+    ("", False),
+    ("true", True), ("True", True), ("TRUE", True),
+    ("1", True), ("yes", True), ("on", True),
+    ("false", False), ("False", False), ("0", False),
+    ("no", False), ("off", False), (" true ", True),
+])
+def test_env_bool_accepts(env, raw, expected):
+    env(FLAG=raw)
+    assert env_bool("FLAG", False) is expected
+
+
+@pytest.mark.parametrize("raw", ["maybe", "2", "-1", "y3s", "truthy"])
+def test_env_bool_rejects(env, raw):
+    env(FLAG=raw)
+    with pytest.raises(ConfigError) as err:
+        env_bool("FLAG", False)
+    assert "FLAG" in str(err.value)
+
+
+def test_env_bool_keeps_a_true_default_when_unset(env):
+    env(FLAG=None)
+    assert env_bool("FLAG", True) is True
+
+
+@pytest.mark.parametrize("raw, expected", [
+    (None, None), ("", None), ("value", "value"), ("  spaced  ", "  spaced  "),
+])
+def test_env_str(env, raw, expected):
+    env(NAME=raw)
+    assert env_str("NAME") == expected
+
+
+def test_a_default_container_gets_exactly_the_documented_defaults(monkeypatch):
+    """The values the README promises when nothing optional is configured."""
+    for name in ("UPLOAD_LIMIT", "DOWNLOAD_LIMIT", "DELETE_CONFIRM_THRESHOLD",
+                 "SEAFILE_UID", "SEAFILE_GID", "DISABLE_VERIFY_CERTIFICATE"):
+        monkeypatch.delenv(name, raising=False)
+
+    defaults = start.defaults_from_env()
+
+    assert defaults["upload_limit"] == 0
+    assert defaults["download_limit"] == 0
+    assert defaults["delete_confirm_threshold"] == 500
+    assert defaults["uid"] == 1000
+    assert defaults["gid"] == 1000
+    assert defaults["disable_verify_certificate"] is False
+
+
+def test_no_default_is_none_where_a_value_is_documented(monkeypatch):
+    """A None here reaches seaf-cli as the literal string "None"."""
+    for name in ("UPLOAD_LIMIT", "DOWNLOAD_LIMIT", "DELETE_CONFIRM_THRESHOLD",
+                 "SEAFILE_UID", "SEAFILE_GID"):
+        monkeypatch.delenv(name, raising=False)
+
+    defaults = start.defaults_from_env()
+    for key in ("upload_limit", "download_limit", "delete_confirm_threshold",
+                "uid", "gid"):
+        assert defaults[key] is not None, f"{key} must never default to None"
+
+
+def test_numeric_settings_are_integers_not_strings(monkeypatch):
+    """A string UID never equals the integer it is compared against."""
+    monkeypatch.setenv("SEAFILE_UID", "1000")
+    monkeypatch.setenv("UPLOAD_LIMIT", "50")
+
+    defaults = start.defaults_from_env()
+    assert defaults["uid"] == 1000 and isinstance(defaults["uid"], int)
+    assert defaults["upload_limit"] == 50 and isinstance(defaults["upload_limit"], int)