Starting a Controller script writes its env file every time and rejects unknown parameters

The upgrade service reads BRANCH from /tmp/wrolpi-upgrade.env, which upgrade.sh removes when it finishes.  The
Controller wrote that file only when a request had parameters, so starting an upgrade without a branch (the simple UI
sends no body when the field is empty) left the service unable to start, while the API answered success.  A request
like {"branch": "master"} was accepted with the key ignored, the same way.

Now the env file is always written, and a missing branch is the current one, as upgrade.sh chooses; a file left by an
interrupted run no longer chooses it.  Unknown parameters and unknown request keys are errors.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

controller/api/schemas.py

@@ -4,7 +4,7 @@ Pydantic models for Controller API requests and responses.
 
 from typing import Optional
 
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, ConfigDict, Field
 
 
 # ============================================================================
@@ -495,6 +495,9 @@ class ScriptOutputResponse(BaseModel):
 class ScriptStartRequest(BaseModel):
     """Request model for starting a script."""
 
+    # Parameters belong in `params`; any other key is a mistake, not something to ignore.
+    model_config = ConfigDict(extra="forbid")
+
     params: Optional[dict[str, str]] = Field(default=None, description="Parameter values (e.g., {'branch': 'release'})")
 
 

controller/lib/scripts.py

@@ -239,17 +239,30 @@ def start_script(name: str, params: Optional[dict] = None) -> dict:
 
     script_config = AVAILABLE_SCRIPTS[name]
     service = script_config["service_name"]
+    parameters = script_config.get("parameters", [])
 
-    # Write parameters to env file if script has one configured
+    # A misspelled parameter would otherwise be dropped, and the script run with its default.
+    unknown = sorted(set(params or {}) - {i["name"] for i in parameters})
+    if unknown:
+        return {
+            "success": False,
+            "error": f"Unknown parameters for script '{name}': {', '.join(unknown)}",
+        }
+
+    # Always write the env file: the service cannot start without it, and a file left by an interrupted run must
+    # not choose the parameters.
     env_file = script_config.get("env_file")
-    if env_file and params:
+    if env_file:
+        values = {key: value for key, value in (params or {}).items() if value}
+        for parameter in parameters:
+            if parameter["type"] == "branch" and not values.get(parameter["name"]):
+                # upgrade.sh upgrades the current branch when it is not given one.
+                values[parameter["name"]] = get_current_branch() or "release"
         try:
             with open(env_file, "w") as f:
-                for key, value in params.items():
-                    if value:  # Only write non-empty values
-                        # Convert param name to uppercase for env var
-                        env_name = key.upper()
-                        f.write(f"{env_name}={value}\n")
+                for key, value in values.items():
+                    # Convert param name to uppercase for env var
+                    f.write(f"{key.upper()}={value}\n")
         except OSError as e:
             return {
                 "success": False,

controller/test/test_scripts.py

@@ -240,6 +240,46 @@ class TestStartScript:
                     assert "wrolpi-repair.service" in call_args
 
 
+    def _start_upgrade(self, tmp_path, params, current_branch="master"):
+        """Start the upgrade script with its env file in `tmp_path`.  Returns (result, env file, Popen mock)."""
+        env_file = tmp_path / "wrolpi-upgrade.env"
+        with mock.patch.dict(AVAILABLE_SCRIPTS["upgrade"], {"env_file": str(env_file)}), \
+                mock.patch("controller.lib.scripts.is_docker_mode", return_value=False), \
+                mock.patch("controller.lib.scripts.get_script_status", return_value={"running": False}), \
+                mock.patch("controller.lib.scripts.get_current_branch", return_value=current_branch), \
+                mock.patch("subprocess.Popen") as popen:
+            result = start_script("upgrade", params=params)
+        return result, env_file, popen
+
+    def test_upgrade_without_branch_writes_current_branch(self, tmp_path):
+        """The upgrade service reads BRANCH from its env file, and fails to start without the file; upgrade.sh
+        removes it when it finishes.  With no branch, upgrade the current one, as upgrade.sh does."""
+        for params in (None, {}, {"branch": ""}):
+            result, env_file, popen = self._start_upgrade(tmp_path, params)
+            assert result["success"] is True, params
+            assert env_file.read_text() == "BRANCH=master\n", params
+            popen.assert_called_once()
+            env_file.unlink()
+
+    def test_upgrade_replaces_stale_env_file(self, tmp_path):
+        """A file left by an interrupted upgrade does not choose the branch."""
+        (tmp_path / "wrolpi-upgrade.env").write_text("BRANCH=old\n")
+        result, env_file, _ = self._start_upgrade(tmp_path, None, current_branch="release")
+        assert result["success"] is True
+        assert env_file.read_text() == "BRANCH=release\n"
+
+        result, env_file, _ = self._start_upgrade(tmp_path, {"branch": "master"}, current_branch="release")
+        assert env_file.read_text() == "BRANCH=master\n"
+
+    def test_unknown_param_is_rejected(self, tmp_path):
+        """A misspelled parameter is an error, not silently ignored."""
+        result, env_file, popen = self._start_upgrade(tmp_path, {"brnch": "master"})
+        assert result["success"] is False
+        assert "brnch" in result["error"]
+        assert not env_file.exists()
+        popen.assert_not_called()
+
+
 class TestGetScriptOutput:
     """Tests for get_script_output function."""
 

controller/test/test_scripts_api.py

@@ -101,6 +101,16 @@ class TestScriptsStartEndpoint:
                 assert response.status_code == 400
                 assert "already running" in response.json()["detail"].lower()
 
+    def test_scripts_start_rejects_unknown_body_keys(self, test_client):
+        """Parameters go in `params`; a body like {"branch": ...} is an error, not an upgrade of some other
+        branch."""
+        with mock.patch("controller.lib.scripts.is_docker_mode", return_value=False), \
+                mock.patch("controller.lib.scripts.get_script_status", return_value={"running": False}), \
+                mock.patch("subprocess.Popen") as popen:
+            response = test_client.post("/api/scripts/upgrade/start", json={"branch": "master"})
+        assert response.status_code == 422
+        popen.assert_not_called()
+
 
 class TestScriptsOutputEndpoint:
     """Tests for /api/scripts/{name}/output endpoint."""