fix(starlark): the six review findings that were still real

CodeRabbit raised eight on this PR. Two were already fixed by later
commits (the F541 f-string prefixes, and the twenty-four CodeQL
path-injection alerts, now zero open and the last two dismissed as
false positives). These are the rest, each verified against the code as
it stands rather than as it was reviewed.

Manifest writes shared one temp file. `with_suffix('.tmp')` is a fixed
path, and five routes write the manifest while Flask serves requests
concurrently -- two writers opened `manifest.tmp`, interleaved their
json.dump output and both renamed. The rename is atomic; the content it
published was the mixture, which the next read cannot parse. mkstemp in
the destination directory gives each writer its own file.

A failed import stayed cached. Both dynamic importers insert into
sys.modules before exec_module, which they must, but left the
half-initialised module behind on failure -- every later call then took
the cache branch and raised AttributeError on the missing class instead
of retrying, so one transient failure disabled the repository or the
renderer for the life of the process. The keys stay unprefixed: the
plugin's own manager.py does `from pixlet_renderer import
PixletRenderer`, and sharing that key is what keeps one module object.

Config was applied before it was saved. A false from save_config()
answers 500, but the loaded app kept the new values, so GET config
reported settings that were never written and the plugin rendered with
them until a restart silently put the old ones back. Snapshot and
restore on the failure branch, and do not re-render.

The toggle reported success it had not earned. `_update_manifest_safe`
returns False on failure and the result was dropped, and the in-memory
flag was flipped first -- so a failed write left a toggle that reverted
itself on the next restart. Persist first, then update memory.

The toggle also could not find what the list had shown.
`_starlark_virtual_plugins` publishes the raw manifest key, while the
toggle re-slugified it through `_validate_and_sanitize_app_id`
(lowercase, every character outside [a-z0-9_] rewritten): an app stored
as `My-App` was offered as `starlark:My-App` and looked up as `my_app`,
so toggling an app the page had just drawn answered 404. Check for
traversal with `_validate_starlark_app_path` and keep the key.

`star_file` was stored absolute. Readers join it to the app's own
directory and default to a bare `<app_id>.star`, so the key had two
meanings -- and `Path.__truediv__` discards the left side when the right
is absolute, pinning the manifest to whatever PROJECT_ROOT installed it.
Store the filename.

Two store tests reached GitHub. browse and categories call
list_all_apps_cached(), which fetches the app index on a cold cache --
slow, network-dependent, rate-limitable, and since both only assert "not
a 404" a rate-limited 500 passed and hid it. The file now runs in 1.5s
rather than 17.5s.

13 new tests, all six findings covered. Whole suite: no new failures
against this branch, 3892 passed against 3879.

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

test/web_interface/test_starlark_pixlet_routes.py

@@ -15,6 +15,11 @@ reads, so a future rewrite of this file cannot silently drop them again.
 """
 
 import json
+import os
+import sys
+import tempfile
+import threading
+import types
 from unittest.mock import MagicMock, patch
 
 import pytest
@@ -28,6 +33,24 @@ def client():
         yield c
 
 
+@pytest.fixture
+def offline_repository():
+    """Stand in for TronbyteRepository so the store routes stay offline.
+
+    The browse and categories routes call list_all_apps_cached(), which on a
+    cold server-side cache fetches the app index from GitHub. That would make
+    the suite slow, network-dependent and rate-limitable -- and because these
+    tests only assert "not a 404", a rate-limited 500 would pass and hide it.
+    """
+    repo = MagicMock()
+    repo.list_all_apps_cached.return_value = {
+        'apps': [], 'categories': [], 'authors': [], 'count': 0, 'cached': True,
+    }
+    with patch('web_interface.blueprints.api_v3._get_tronbyte_repository_class',
+               return_value=lambda *a, **kw: repo):
+        yield repo
+
+
 class TestRoutesAreRegistered:
     """The failure was a missing route, so check the URL map directly.
 
@@ -133,12 +156,12 @@ class TestTheAppStoreFlow:
     an empty store.
     """
 
-    def test_browse_does_not_404(self, client):
+    def test_browse_does_not_404(self, client, offline_repository):
         resp = client.get('/api/v3/starlark/repository/browse')
         assert resp.status_code != 404, "the store cannot list anything"
         assert resp.get_json().get('message') != 'Resource not found'
 
-    def test_categories_does_not_404(self, client):
+    def test_categories_does_not_404(self, client, offline_repository):
         resp = client.get('/api/v3/starlark/repository/categories')
         assert resp.status_code != 404
         assert resp.get_json().get('message') != 'Resource not found'
@@ -199,3 +222,286 @@ class TestNoStarlarkRouteIsMissing:
             except NotFound:
                 missing.append(u)
         assert not missing, f"the frontend calls these and they are not registered: {missing}"
+
+
+class TestInstalledAppsAppearWithTheOtherPlugins:
+    """An installed .star app must be manageable like any other plugin.
+
+    #253 surfaced installed apps in /plugins/installed as `starlark:<app_id>`
+    entries, so they could be seen and enabled/disabled from the same list as
+    everything else, and routed `starlark:` toggles to the Starlark manifest.
+    #330 removed both. The result was an app that installs successfully, then
+    appears nowhere and cannot be turned on or off.
+    """
+
+    APPS = {'apps': {'quoteoftheday': {'name': 'A Quote A Day', 'enabled': True}}}
+
+    def test_an_installed_app_is_listed(self, client):
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._read_starlark_manifest', return_value=self.APPS):
+            resp = client.get('/api/v3/plugins/installed')
+        ids = [p['id'] for p in resp.get_json()['data']['plugins']]
+        assert 'starlark:quoteoftheday' in ids, \
+            "an installed Starlark app does not appear among the plugins"
+
+    def test_the_entry_carries_what_the_ui_needs(self, client):
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._read_starlark_manifest', return_value=self.APPS):
+            resp = client.get('/api/v3/plugins/installed')
+        entry = next(p for p in resp.get_json()['data']['plugins']
+                     if p['id'] == 'starlark:quoteoftheday')
+        assert entry['name'] == 'A Quote A Day'
+        assert entry['enabled'] is True
+        assert entry['is_starlark_app'] is True, "the UI keys its Starlark handling off this"
+        assert entry['category'] == 'Starlark App'
+
+    def test_a_starlark_failure_does_not_empty_the_plugin_list(self, client):
+        # The virtual entries are appended to the real ones; a broken manifest
+        # must cost the Starlark rows, not everybody else's.
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin',
+                   side_effect=RuntimeError('boom')):
+            resp = client.get('/api/v3/plugins/installed')
+        assert resp.status_code == 200
+        assert resp.get_json()['status'] == 'success'
+
+    def test_toggling_an_app_does_not_report_plugin_not_found(self, client):
+        written = {}
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._read_starlark_manifest',
+                   return_value={'apps': {'quoteoftheday': {'enabled': True}}}), \
+             patch('web_interface.blueprints.api_v3._write_starlark_manifest',
+                   side_effect=lambda m: written.update(m) or True):
+            resp = client.post('/api/v3/plugins/toggle',
+                               json={'plugin_id': 'starlark:quoteoftheday', 'enabled': False})
+        body = resp.get_json()
+        assert body['status'] == 'success', body
+        assert body['enabled'] is False
+        assert written['apps']['quoteoftheday']['enabled'] is False, \
+            "the manifest was not actually updated"
+
+    def test_toggling_an_unknown_app_says_so(self, client):
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._read_starlark_manifest',
+                   return_value={'apps': {}}):
+            resp = client.post('/api/v3/plugins/toggle',
+                               json={'plugin_id': 'starlark:nope', 'enabled': True})
+        assert resp.status_code == 404
+        assert 'nope' in resp.get_json()['message']
+
+    def test_a_traversal_app_id_is_rejected_before_touching_the_manifest(self, client):
+        resp = client.post('/api/v3/plugins/toggle',
+                           json={'plugin_id': 'starlark:../../etc/passwd', 'enabled': True})
+        assert resp.status_code == 400
+        assert 'traversal' in resp.get_json()['message']
+
+    def test_an_app_id_the_listing_published_can_be_toggled(self, client):
+        """The id here is the one _starlark_virtual_plugins publishes.
+
+        It used to be re-slugified on the way in -- lowercased, with every
+        character outside [a-z0-9_] replaced -- so 'My-App' was listed as
+        'starlark:My-App' and looked up as 'my_app', and toggling an app the
+        page had just drawn answered 404.
+        """
+        manifest = {'apps': {'My-App': {'name': 'My App', 'enabled': False}}}
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._read_starlark_manifest',
+                   return_value=manifest), \
+             patch('web_interface.blueprints.api_v3._write_starlark_manifest',
+                   return_value=True) as write:
+            resp = client.post('/api/v3/plugins/toggle',
+                               json={'plugin_id': 'starlark:My-App', 'enabled': True})
+        assert resp.status_code == 200, resp.get_json()
+        assert write.called, "the toggle never reached the manifest"
+        assert manifest['apps']['My-App']['enabled'] is True
+
+    def test_a_failed_manifest_write_is_not_reported_as_success(self, client):
+        """The toggle answered 200 while the change was never persisted."""
+        manifest = {'apps': {'demo': {'name': 'Demo', 'enabled': False}}}
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._read_starlark_manifest',
+                   return_value=manifest), \
+             patch('web_interface.blueprints.api_v3._write_starlark_manifest',
+                   return_value=False):
+            resp = client.post('/api/v3/plugins/toggle',
+                               json={'plugin_id': 'starlark:demo', 'enabled': True})
+        assert resp.status_code == 500
+
+    def test_a_loaded_app_is_not_flipped_when_the_manifest_write_fails(self, client):
+        """Persist first, then update memory -- otherwise the UI shows a
+        toggle that silently reverts on the next restart."""
+        app = MagicMock()
+        app.manifest = {'enabled': False}
+        plugin = MagicMock()
+        plugin.apps = {'demo': app}
+        plugin._update_manifest_safe.return_value = False
+
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin):
+            resp = client.post('/api/v3/plugins/toggle',
+                               json={'plugin_id': 'starlark:demo', 'enabled': True})
+        assert resp.status_code == 500
+        assert app.manifest['enabled'] is False, "in-memory state changed without being saved"
+
+
+class TestTheManifestSurvivesConcurrentWriters:
+    """Flask serves requests concurrently and five routes write this file.
+
+    The atomic-write pattern used one fixed temp name, `manifest.tmp`, shared
+    by every writer: two of them opened it, interleaved their json.dump output,
+    and both renamed. The rename is atomic; the content it published was the
+    mixture, which the next read could not parse.
+    """
+
+    @pytest.fixture
+    def starlark_dir(self, tmp_path, monkeypatch):
+        from web_interface.blueprints import api_v3 as module
+        apps_dir = tmp_path / "starlark-apps"
+        apps_dir.mkdir()
+        monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir)
+        monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json')
+        return apps_dir
+
+    def test_each_writer_gets_its_own_temp_file(self, starlark_dir):
+        from web_interface.blueprints import api_v3 as module
+
+        seen = []
+        real_mkstemp = tempfile.mkstemp
+
+        def recording_mkstemp(*args, **kwargs):
+            fd, name = real_mkstemp(*args, **kwargs)
+            seen.append(name)
+            return fd, name
+
+        with patch.object(module.tempfile, 'mkstemp', side_effect=recording_mkstemp):
+            for i in range(5):
+                assert module._write_starlark_manifest({'apps': {f'app{i}': {}}})
+
+        assert len(set(seen)) == 5, f"writers shared a temp file: {seen}"
+
+    def test_concurrent_writes_leave_readable_json(self, starlark_dir):
+        from web_interface.blueprints import api_v3 as module
+
+        manifests = [{'apps': {f'app{i}': {'name': 'x' * 400}}} for i in range(8)]
+        errors = []
+
+        def write(m):
+            try:
+                module._write_starlark_manifest(m)
+            except Exception as exc:  # noqa: BLE001 - surfaced by the assert below
+                errors.append(exc)
+
+        threads = [threading.Thread(target=write, args=(m,)) for m in manifests]
+        for t in threads:
+            t.start()
+        for t in threads:
+            t.join()
+
+        assert not errors
+        loaded = json.loads((starlark_dir / 'manifest.json').read_text())
+        assert loaded in manifests, "the published manifest was a mix of two writers"
+
+    def test_no_temp_files_are_left_behind(self, starlark_dir):
+        from web_interface.blueprints import api_v3 as module
+        module._write_starlark_manifest({'apps': {}})
+        assert list(starlark_dir.glob('*.tmp')) == []
+
+    def test_the_installed_star_file_is_recorded_by_name(self, starlark_dir, tmp_path):
+        """Readers join this to the app's own directory and default to a bare
+        '<app_id>.star'. An absolute value gave the key two meanings and, since
+        Path.__truediv__ discards the left side when the right is absolute,
+        pinned the manifest to the PROJECT_ROOT it was installed under."""
+        from web_interface.blueprints import api_v3 as module
+
+        source = tmp_path / "source.star"
+        source.write_text("# app")
+        with patch.object(module, '_get_pixlet_renderer_class',
+                          side_effect=ImportError("no pixlet here")):
+            assert module._install_star_file('demo', str(source), {'name': 'Demo'})
+
+        entry = json.loads((starlark_dir / 'manifest.json').read_text())['apps']['demo']
+        assert entry['star_file'] == 'demo.star'
+        assert not os.path.isabs(entry['star_file'])
+
+
+class TestATransientImportFailureIsNotPermanent:
+    """Both importers insert into sys.modules before executing the module.
+
+    That order is required -- a module has to be findable while it runs -- but
+    a failure left the half-initialised object cached, so every later call took
+    the cache branch and raised AttributeError on the missing class instead of
+    retrying. One transient failure disabled the repository or the renderer for
+    the life of the process.
+    """
+
+    @pytest.mark.parametrize("getter,key", [
+        ('_get_tronbyte_repository_class', 'tronbyte_repository'),
+        ('_get_pixlet_renderer_class', 'pixlet_renderer'),
+    ])
+    def test_a_failed_import_leaves_no_entry_behind(self, getter, key):
+        from web_interface.blueprints import api_v3 as module
+
+        original = sys.modules.pop(key, None)
+        try:
+            with patch('importlib.util.module_from_spec') as from_spec:
+                from_spec.return_value = types.ModuleType(key)
+                with patch('importlib.util.spec_from_file_location') as spec_from:
+                    spec = MagicMock()
+                    spec.loader.exec_module.side_effect = RuntimeError("network down")
+                    spec_from.return_value = spec
+
+                    with pytest.raises(RuntimeError):
+                        getattr(module, getter)()
+
+            assert key not in sys.modules, "a half-initialised module stayed cached"
+        finally:
+            if original is not None:
+                sys.modules[key] = original
+            else:
+                sys.modules.pop(key, None)
+
+
+class TestConfigIsNotAppliedUntilItIsSaved:
+    """save_config() returning False answers 500, but the loaded app kept the
+    new values -- so GET config reported settings that were never written and
+    the plugin rendered with them until a restart silently reverted them."""
+
+    def _plugin_with_app(self, save_ok):
+        app = MagicMock()
+        app.config = {'city': 'Philadelphia'}
+        app.manifest = {'render_interval': 300, 'display_duration': 15}
+        app.save_config.return_value = save_ok
+        plugin = MagicMock()
+        plugin.apps = {'demo': app}
+        plugin._update_manifest_safe.return_value = True
+        return plugin, app
+
+    def test_a_failed_save_leaves_the_config_untouched(self, client):
+        plugin, app = self._plugin_with_app(save_ok=False)
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'city': 'Pittsburgh'})
+        assert resp.status_code == 500
+        assert app.config == {'city': 'Philadelphia'}
+
+    def test_a_failed_save_leaves_the_timing_untouched(self, client):
+        plugin, app = self._plugin_with_app(save_ok=False)
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'render_interval': 60})
+        assert resp.status_code == 500
+        assert app.manifest['render_interval'] == 300
+
+    def test_a_failed_save_does_not_re_render_with_values_it_did_not_keep(self, client):
+        plugin, _ = self._plugin_with_app(save_ok=False)
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin):
+            client.put('/api/v3/starlark/apps/demo/config', json={'city': 'Pittsburgh'})
+        plugin._render_app.assert_not_called()
+
+    def test_a_successful_save_still_applies(self, client):
+        plugin, app = self._plugin_with_app(save_ok=True)
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'city': 'Pittsburgh', 'render_interval': 60})
+        assert resp.status_code == 200
+        assert app.config['city'] == 'Pittsburgh'
+        assert app.manifest['render_interval'] == 60
+        plugin._render_app.assert_called_once()

web_interface/blueprints/api_v3.py

@@ -2813,6 +2813,7 @@ def get_installed_plugins():
         with ThreadPoolExecutor(max_workers=8) as executor:
             results = list(executor.map(_build_plugin_entry, all_plugin_info))
         plugins = [r for r in results if r is not None]
+        plugins.extend(_starlark_virtual_plugins())
 
         return jsonify({'status': 'success', 'data': {'plugins': plugins}})
     except Exception as e:
@@ -3119,6 +3120,13 @@ def toggle_plugin():
                 current_enabled = config.get(plugin_id, {}).get('enabled', False)
                 enabled = not current_enabled
 
+        # A Starlark app is not a plugin in plugin_manager's sense -- it is an
+        # entry in starlark-apps' own manifest -- so its enable/disable is
+        # handled here rather than falling through to the check below, which
+        # would answer "Plugin not found".
+        if plugin_id.startswith('starlark:'):
+            return _toggle_starlark_app(plugin_id[len('starlark:'):], enabled)
+
         # Check if plugin exists in manifests (discovered but may not be loaded)
         if plugin_id not in api_v3.plugin_manager.plugin_manifests:
             return jsonify({'status': 'error', 'message': 'Plugin not found'}), 404
@@ -8825,8 +8833,21 @@ def _get_tronbyte_repository_class() -> Type[Any]:
     if module is None:
         raise ImportError("Failed to create module from spec for tronbyte_repository")
 
+    # The entry has to come out again if execution fails. It is inserted
+    # first because a module has to be in sys.modules before it runs (its own
+    # imports can reach back for it), but leaving a half-initialised module
+    # cached means every later call takes the branch above and raises
+    # AttributeError on the missing class instead of retrying -- one transient
+    # failure would disable the repository for the life of the process.
+    #
+    # The key stays unprefixed to match _get_pixlet_renderer_class, where the
+    # short name is load-bearing (see the note there).
     sys.modules["tronbyte_repository"] = module
-    spec.loader.exec_module(module)
+    try:
+        spec.loader.exec_module(module)
+    except Exception:
+        sys.modules.pop("tronbyte_repository", None)
+        raise
     return module.TronbyteRepository
 
 def _get_pixlet_renderer_class() -> Type[Any]:
@@ -8850,8 +8871,19 @@ def _get_pixlet_renderer_class() -> Type[Any]:
     if module is None:
         raise ImportError("Failed to create module from spec for pixlet_renderer")
 
+    # Uncache on failure, as in _get_tronbyte_repository_class, so a transient
+    # import error does not disable rendering for the rest of the process.
+    #
+    # The short key is load-bearing here: the plugin's own manager.py does
+    # `from pixlet_renderer import PixletRenderer`, and this key is what makes
+    # both loaders share one module object. Prefixing it would quietly produce
+    # a second copy of the class, so isinstance across the two would fail.
     sys.modules["pixlet_renderer"] = module
-    spec.loader.exec_module(module)
+    try:
+        spec.loader.exec_module(module)
+    except Exception:
+        sys.modules.pop("pixlet_renderer", None)
+        raise
     return module.PixletRenderer
 
 def _validate_and_sanitize_app_id(app_id: Optional[str], fallback_source: Optional[str] = None) -> Tuple[Optional[str], Optional[str]]:
@@ -8884,34 +8916,48 @@ def _validate_timing_value(value: Any, field_name: str, min_val: int = 1, max_va
         return None, f"{field_name} must be at most {max_val}"
     return int_value, None
 
-def _validate_starlark_app_path(app_id: str) -> Tuple[bool, Optional[str]]:
+def _validate_starlark_app_path(app_id: str) -> Tuple[Optional[Path], Optional[str]]:
-    """
+    """The app's directory, or an error if app_id could escape the base dir.
-    Validate app_id for path traversal attacks before filesystem access.
 
-    Args:
+    Returns the *resolved* path rather than a boolean, and every caller uses
-        app_id: App identifier from user input
+    what it returns instead of re-joining ``_STARLARK_APPS_DIR / app_id``
+    afterwards. The old shape validated in one place and rebuilt the path in
+    another, which is two things that have to stay in step -- and is why
+    CodeQL reported twenty-four path-injection alerts across these handlers
+    even though the guard was effective: a boolean is not a sanitiser it can
+    follow, and the value reaching the filesystem was the raw one.
 
-    Returns:
+    The name is unchanged so the call sites read the same.
-        Tuple of (is_valid, error_message)
     """
-    # Check for path traversal characters
+    if not isinstance(app_id, str) or not app_id:
+        return None, "Invalid app_id"
+
+    # Reject the traversal characters outright before touching the filesystem.
     if '..' in app_id or '/' in app_id or '\\' in app_id:
-        return False, f"Invalid app_id: contains path traversal characters"
+        return None, "Invalid app_id: contains path traversal characters"
+
+    # os.path.basename strips any directory component, so what is joined below
+    # cannot carry one. The equality check means this rejects rather than
+    # silently truncates -- behaviour is identical to the character test above,
+    # and it is the sanitiser CodeQL's path-injection query actually follows.
+    # relative_to() alone is a check it cannot trace, which is why twenty-four
+    # of these stayed flagged after the value was threaded through properly.
+    safe_name = os.path.basename(app_id)
+    if safe_name != app_id or safe_name in ('', '.', '..'):
+        return None, "Invalid app_id: contains path traversal characters"
 
-    # Construct and resolve the path
     try:
-        app_path = (_STARLARK_APPS_DIR / app_id).resolve()
         base_path = _STARLARK_APPS_DIR.resolve()
-
+        app_path = (base_path / safe_name).resolve()
-        # Verify the resolved path is within the base directory
         try:
             app_path.relative_to(base_path)
-            return True, None
         except ValueError:
-            return False, f"Invalid app_id: path traversal attempt"
+            return None, "Invalid app_id: path traversal attempt"
-    except Exception as e:
+        return app_path, None
-        logger.warning(f"Path validation error for app_id '{app_id}': {e}")
+    except OSError as e:
-        return False, f"Invalid app_id"
+        logger.warning("Path validation error for app_id %r: %s", app_id, e)
+        return None, "Invalid app_id"
+
 
 def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[str]]:
     """Render a Starlark app via pixlet directly (no plugin required).
@@ -8932,7 +8978,11 @@ def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[st
     if not app_data:
         return False, 404, f"App not found: {app_id}"
 
-    app_dir = _STARLARK_APPS_DIR / app_id
+    # Validated here as well as at the handler: this is reachable on its own,
+    # and a path built from app_id should never be assembled without it.
+    app_dir, path_error = _validate_starlark_app_path(app_id)
+    if path_error:
+        return False, 400, path_error
     star_file = app_dir / app_data.get('star_file', f'{app_id}.star')
     if not star_file.exists():
         return False, 404, f"Star file not found: {star_file}"
@@ -8964,9 +9014,11 @@ def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[st
             with open(config_file) as f:
                 app_config = json.load(f)
         except json.JSONDecodeError as e:
-            return False, 400, f"Invalid config.json for {app_id} ({config_file}): {e}"
+            logger.warning("Invalid config.json for %r at %s: %s", app_id, config_file, e)
+            return False, 400, f"Invalid config.json for {app_id}"
         except OSError as e:
-            return False, 400, f"Cannot read config.json for {app_id} ({config_file}): {e}"
+            logger.warning("Cannot read config.json for %r at %s: %s", app_id, config_file, e)
+            return False, 400, f"Cannot read config.json for {app_id}"
         if not isinstance(app_config, dict):
             return False, 400, (
                 f"config.json for {app_id} must be a JSON object, "
@@ -8995,7 +9047,8 @@ def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[st
     except subprocess.TimeoutExpired:
         return False, 504, "Render timed out after 30s"
     except Exception as e:
-        return False, 500, f"Render error: {e}"
+        logger.exception("Starlark render failed for %r", app_id)
+        return False, 500, "Render error"
 
 def _write_starlark_manifest(manifest: Dict[str, Any]) -> bool:
     """Write the starlark-apps manifest.json to disk with atomic write."""
@@ -9003,9 +9056,20 @@ def _write_starlark_manifest(manifest: Dict[str, Any]) -> bool:
     try:
         _STARLARK_APPS_DIR.mkdir(parents=True, exist_ok=True)
 
-        # Atomic write pattern: write to temp file, then rename
+        # Atomic write pattern: write to temp file, then rename.
-        temp_file = _STARLARK_MANIFEST_FILE.with_suffix('.tmp')
+        #
-        with open(temp_file, 'w') as f:
+        # The temp name has to be unique per writer, not a fixed
+        # manifest.tmp. Flask serves requests concurrently and this is called
+        # from the upload, uninstall, config, toggle and plugin-toggle routes,
+        # so two writers shared one file: both wrote into it, their json.dump
+        # output interleaved, and both renamed. The rename is still atomic --
+        # what it publishes is the mixture, which the next read cannot parse.
+        # mkstemp in the destination directory gives each writer its own file
+        # while keeping the rename on one filesystem.
+        fd, temp_name = tempfile.mkstemp(
+            dir=str(_STARLARK_APPS_DIR), prefix='.manifest-', suffix='.tmp')
+        temp_file = Path(temp_name)
+        with os.fdopen(fd, 'w') as f:
             json.dump(manifest, f, indent=2)
             f.flush()
             os.fsync(f.fileno())  # Ensure data is written to disk
@@ -9027,7 +9091,10 @@ def _install_star_file(app_id: str, star_file_path: str, metadata: Dict[str, Any
     """Install a .star file and update the manifest (standalone, no plugin needed)."""
     import shutil
     import json
-    app_dir = _STARLARK_APPS_DIR / app_id
+    app_dir, path_error = _validate_starlark_app_path(app_id)
+    if path_error:
+        logger.warning("Refusing to install %r: %s", app_id, path_error)
+        return False
     app_dir.mkdir(parents=True, exist_ok=True)
     dest = app_dir / f"{app_id}.star"
     shutil.copy2(star_file_path, str(dest))
@@ -9080,7 +9147,13 @@ def _install_star_file(app_id: str, star_file_path: str, metadata: Dict[str, Any
         'render_interval': metadata.get('render_interval', 300),
         'display_duration': metadata.get('display_duration', 15),
         'config': metadata.get('config', {}),
-        'star_file': str(dest),
+        # The filename, not the full path. Readers join this to the app's own
+        # directory and default to a bare '<app_id>.star', so an absolute
+        # value gave the key two meanings -- and Path.__truediv__ discards the
+        # left side when the right is absolute, which pinned the manifest to
+        # whatever PROJECT_ROOT was at install time. Moving or redeploying the
+        # install then left the app unable to find its own file.
+        'star_file': dest.name,
     }
     return _write_starlark_manifest(manifest)
 
@@ -9131,8 +9204,8 @@ def get_starlark_app(app_id):
     """Get details for a specific Starlark app."""
     try:
         # Validate app_id before any filesystem access
-        is_valid, error_msg = _validate_starlark_app_path(app_id)
+        app_dir, error_msg = _validate_starlark_app_path(app_id)
-        if not is_valid:
+        if error_msg:
             return jsonify({'status': 'error', 'message': error_msg}), 400
 
         starlark_plugin = _get_starlark_plugin()
@@ -9164,7 +9237,7 @@ def get_starlark_app(app_id):
 
         # Load schema from schema.json if it exists (path already validated above)
         schema = None
-        schema_file = _STARLARK_APPS_DIR / app_id / 'schema.json'
+        schema_file = app_dir / 'schema.json'
         if schema_file.exists():
             try:
                 with open(schema_file, 'r') as f:
@@ -9257,11 +9330,14 @@ def upload_starlark_app():
                 pass
 
     except (OSError, IOError) as err:
+        # The detail goes to the log, not the response: it names absolute
+        # paths on the device, which the caller has no business seeing. The
+        # generic Exception arm below already did this; these two did not.
         logger.exception("[Starlark] File error uploading starlark app: %s", err)
-        return jsonify({'status': 'error', 'message': f'File error during upload: {err}'}), 500
+        return jsonify({'status': 'error', 'message': 'File error during upload'}), 500
     except ImportError as err:
         logger.exception("[Starlark] Module load error uploading starlark app: %s", err)
-        return jsonify({'status': 'error', 'message': f'Failed to load app module: {err}'}), 500
+        return jsonify({'status': 'error', 'message': 'Failed to load app module'}), 500
     except Exception as err:
         logger.exception("[Starlark] Unexpected error uploading starlark app: %s", err)
         return jsonify({'status': 'error', 'message': 'Failed to upload app'}), 500
@@ -9271,18 +9347,17 @@ def uninstall_starlark_app(app_id):
     """Uninstall a Starlark app."""
     try:
         # Validate app_id before any filesystem access
-        is_valid, error_msg = _validate_starlark_app_path(app_id)
+        app_dir, error_msg = _validate_starlark_app_path(app_id)
-        if not is_valid:
+        if error_msg:
             return jsonify({'status': 'error', 'message': error_msg}), 400
 
         starlark_plugin = _get_starlark_plugin()
         if starlark_plugin:
             success = starlark_plugin.uninstall_app(app_id)
         else:
-            # Standalone: remove app dir and manifest entry (path already validated)
+            # Standalone: remove app dir and manifest entry. app_dir is the
+            # path _validate_starlark_app_path checked, not a fresh join.
             import shutil
-            app_dir = _STARLARK_APPS_DIR / app_id
-
             if app_dir.exists():
                 shutil.rmtree(app_dir)
             manifest = _read_starlark_manifest()
@@ -9303,8 +9378,8 @@ def get_starlark_app_config(app_id):
     """Get configuration for a Starlark app."""
     try:
         # Validate app_id before any filesystem access
-        is_valid, error_msg = _validate_starlark_app_path(app_id)
+        app_dir, error_msg = _validate_starlark_app_path(app_id)
-        if not is_valid:
+        if error_msg:
             return jsonify({'status': 'error', 'message': error_msg}), 400
 
         starlark_plugin = _get_starlark_plugin()
@@ -9314,8 +9389,8 @@ def get_starlark_app_config(app_id):
                 return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404
             return jsonify({'status': 'success', 'config': app.config, 'schema': app.schema})
 
-        # Standalone: read from config.json file (path already validated)
+        # Standalone: read from config.json. app_dir is the path
-        app_dir = _STARLARK_APPS_DIR / app_id
+        # _validate_starlark_app_path checked, not a fresh join.
         config_file = app_dir / "config.json"
 
         if not app_dir.exists():
@@ -9350,8 +9425,8 @@ def update_starlark_app_config(app_id):
     """Update configuration for a Starlark app."""
     try:
         # Validate app_id before any filesystem access
-        is_valid, error_msg = _validate_starlark_app_path(app_id)
+        app_dir, error_msg = _validate_starlark_app_path(app_id)
-        if not is_valid:
+        if error_msg:
             return jsonify({'status': 'error', 'message': error_msg}), 400
 
         data = request.get_json(silent=True)
@@ -9380,6 +9455,15 @@ def update_starlark_app_config(app_id):
             render_interval = data.pop('render_interval', None)
             display_duration = data.pop('display_duration', None)
 
+            # Snapshot before mutating, so a failed save leaves nothing
+            # behind. save_config() returning False answers 500, but the
+            # loaded app kept the new values: the config endpoint then
+            # reported configuration that was never written, and the plugin
+            # went on rendering with it until the next restart silently put
+            # the old values back.
+            previous_config = dict(app.config)
+            previous_manifest = dict(app.manifest)
+
             # Update config with non-timing fields only
             app.config.update(data)
 
@@ -9391,27 +9475,32 @@ def update_starlark_app_config(app_id):
             if display_duration is not None:
                 app.manifest['display_duration'] = display_duration
                 timing_changed = True
-            if app.save_config():
+            if not app.save_config():
-                # Persist manifest if timing changed (same pattern as toggle endpoint)
+                app.config.clear()
-                if timing_changed:
+                app.config.update(previous_config)
-                    try:
+                app.manifest.clear()
-                        # Use safe manifest update to prevent race conditions
+                app.manifest.update(previous_manifest)
-                        timing_updates = {}
-                        if render_interval is not None:
-                            timing_updates['render_interval'] = render_interval
-                        if display_duration is not None:
-                            timing_updates['display_duration'] = display_duration
-
-                        def update_fn(manifest):
-                            manifest['apps'][app_id].update(timing_updates)
-                        starlark_plugin._update_manifest_safe(update_fn)
-                    except Exception as e:
-                        logger.warning(f"Failed to persist timing to manifest for {app_id}: {e}")
-                starlark_plugin._render_app(app, force=True)
-                return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': app.config})
-            else:
                 return jsonify({'status': 'error', 'message': 'Failed to save configuration'}), 500
 
+            # Persist manifest if timing changed (same pattern as toggle endpoint)
+            if timing_changed:
+                try:
+                    # Use safe manifest update to prevent race conditions
+                    timing_updates = {}
+                    if render_interval is not None:
+                        timing_updates['render_interval'] = render_interval
+                    if display_duration is not None:
+                        timing_updates['display_duration'] = display_duration
+
+                    def update_fn(manifest):
+                        manifest['apps'][app_id].update(timing_updates)
+                    if not starlark_plugin._update_manifest_safe(update_fn):
+                        logger.warning("Timing for %s was not persisted to the manifest", app_id)
+                except Exception as e:
+                    logger.warning(f"Failed to persist timing to manifest for {app_id}: {e}")
+            starlark_plugin._render_app(app, force=True)
+            return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': app.config})
+
         # Standalone: update both config.json and manifest
         manifest = _read_starlark_manifest()
         app_data = manifest.get('apps', {}).get(app_id)
@@ -9428,8 +9517,8 @@ def update_starlark_app_config(app_id):
         if display_duration is not None:
             app_data['display_duration'] = display_duration
 
-        # Load current config from config.json
+        # Load current config from config.json. app_dir is the path
-        app_dir = _STARLARK_APPS_DIR / app_id
+        # _validate_starlark_app_path checked, not a fresh join.
         config_file = app_dir / "config.json"
         current_config = {}
         if config_file.exists():
@@ -9448,7 +9537,8 @@ def update_starlark_app_config(app_id):
                 json.dump(current_config, f, indent=2)
         except Exception as e:
             logger.error(f"Failed to save config.json for {app_id}: {e}")
-            return jsonify({'status': 'error', 'message': f'Failed to save configuration: {e}'}), 500
+            logger.exception("Failed to save Starlark configuration for %r", app_id)
+            return jsonify({'status': 'error', 'message': 'Failed to save configuration'}), 500
 
         # Also update manifest for backward compatibility
         app_data.setdefault('config', {}).update(data)
@@ -9506,8 +9596,8 @@ def toggle_starlark_app(app_id):
 def render_starlark_app(app_id):
     """Force render a Starlark app."""
     try:
-        is_valid, err = _validate_starlark_app_path(app_id)
+        app_dir, err = _validate_starlark_app_path(app_id)
-        if not is_valid:
+        if err:
             return jsonify({'status': 'error', 'message': err}), 400
 
         starlark_plugin = _get_starlark_plugin()
@@ -9667,3 +9757,97 @@ def get_tronbyte_categories():
     except Exception as e:
         logger.exception("[Starlark] get_tronbyte_categories failed")
         return jsonify({'status': 'error', 'message': 'Failed to fetch categories'}), 500
+
+
+def _starlark_virtual_plugins() -> list:
+    """Installed Starlark apps, shaped like plugin entries.
+
+    #253 surfaced these alongside real plugins so an installed .star app can
+    be seen, enabled and disabled from the same list as everything else; #330
+    dropped it with the rest of the Starlark code, which is why an app
+    installs successfully and then appears nowhere.
+
+    Reads the loaded plugin when there is one and the on-disk manifest
+    otherwise, so the list is right before starlark-apps has been loaded too.
+    """
+    entries = []
+    base = {
+        'version': 'starlark', 'category': 'Starlark App', 'tags': ['starlark'],
+        'verified': False, 'last_updated': None, 'last_commit': None,
+        'last_commit_message': None, 'branch': None, 'web_ui_actions': [],
+        'vegas_mode': 'fixed', 'vegas_content_type': 'multi',
+        'is_starlark_app': True,
+    }
+    try:
+        plugin = _get_starlark_plugin()
+        if plugin is not None and hasattr(plugin, 'apps'):
+            for app_id, app in plugin.apps.items():
+                m = getattr(app, 'manifest', {}) or {}
+                entries.append({**base,
+                                'id': f'starlark:{app_id}',
+                                'name': m.get('name', app_id),
+                                'author': m.get('author', 'Tronbyte Community'),
+                                'description': m.get('summary', 'Starlark app'),
+                                'enabled': app.is_enabled(),
+                                'loaded': True})
+            return entries
+
+        for app_id, data in (_read_starlark_manifest().get('apps', {}) or {}).items():
+            entries.append({**base,
+                            'id': f'starlark:{app_id}',
+                            'name': data.get('name', app_id),
+                            'author': data.get('author', 'Tronbyte Community'),
+                            'description': data.get('summary', 'Starlark app'),
+                            'enabled': data.get('enabled', True),
+                            'loaded': False})
+    except Exception:
+        # Never let a Starlark problem empty the whole plugins list.
+        logger.exception('Could not build Starlark virtual plugin entries')
+    return entries
+
+
+def _toggle_starlark_app(app_id: str, enabled: bool):
+    """Enable or disable one Starlark app, loaded or not."""
+    # Check for traversal but keep the key as it was listed.
+    # _validate_and_sanitize_app_id lowercases and rewrites every character
+    # outside [a-z0-9_], while _starlark_virtual_plugins publishes the raw
+    # manifest key -- so an app stored as 'My-App' was offered to the UI as
+    # 'starlark:My-App' and looked up here as 'my_app', and toggling it
+    # answered 404 for an app the page had just drawn. Keys written by
+    # _install_star_file are already sanitised; ones written by the plugin or
+    # edited by hand are not.
+    _, err = _validate_starlark_app_path(app_id)
+    if err:
+        # err already names app_id; do not prefix it a second time.
+        return jsonify({'status': 'error', 'message': err}), 400
+    safe_id = app_id
+
+    plugin = _get_starlark_plugin()
+    if plugin is not None and safe_id in getattr(plugin, 'apps', {}):
+        def _update(manifest):
+            # setdefault rather than indexing: the app is loaded, but the
+            # on-disk entry may not exist yet, and _update_manifest_safe does
+            # not catch KeyError -- it would escape as a 500 instead of the
+            # error this returns.
+            manifest.setdefault('apps', {}).setdefault(safe_id, {})['enabled'] = enabled
+
+        # Persist first, then update what is loaded. The other order reported
+        # success and left the in-memory app disagreeing with the manifest
+        # whenever the write failed, so the UI showed a toggle that reverted
+        # itself on the next restart.
+        if not plugin._update_manifest_safe(_update):
+            return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500
+        plugin.apps[safe_id].manifest['enabled'] = enabled
+    else:
+        manifest = _read_starlark_manifest()
+        app_data = manifest.get('apps', {}).get(safe_id)
+        if not app_data:
+            return jsonify({'status': 'error',
+                            'message': f'Starlark app not found: {safe_id}'}), 404
+        app_data['enabled'] = enabled
+        if not _write_starlark_manifest(manifest):
+            return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500
+
+    return jsonify({'status': 'success',
+                    'message': f"Starlark app {'enabled' if enabled else 'disabled'}",
+                    'enabled': enabled})