diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-09-external-providers.md | 182 | ||||
| -rw-r--r-- | llamachat/__main__.py | 16 | ||||
| -rw-r--r-- | llamachat/providers.py | 41 | ||||
| -rwxr-xr-x | test_llamachat.py | 92 |
4 files changed, 326 insertions, 5 deletions
diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md index 158306b..4a428a1 100644 --- a/docs/superpowers/plans/2026-08-09-external-providers.md +++ b/docs/superpowers/plans/2026-08-09-external-providers.md @@ -890,6 +890,188 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" --- +## Task 4b: Harden the startup path + +Not in the original plan. Added after a review of Task 4 found that malformed +config shapes crash the app during `config.load()`, which runs before any +window exists, so the user gets a bare traceback on a terminal a desktop +launch does not have. + +Eight shapes, all reachable from valid TOML a hand-editing user can write. +Three die coercing the table itself (`providers = "oops"`, `providers = ["a"]`, +`providers = 5`), four die on a single entry that is not a table (`a = 5`, +`a = "x"`, `a = ["x"]`, and `[[providers.a]]`), and one dies at the local +merge before the loop is reached (`[[providers.local]]`). The bracket slip is +the one that matters most: `[[providers.together]]` for `[providers.together]` +is an easy miscount and reads as a list of tables. + +Note the first group raises two different exception types, `ValueError` for a +string or list and `TypeError` for a number. That is why the guard is an +isinstance test rather than a try/except. + +**Files:** +- Modify: `llamachat/providers.py` +- Modify: `llamachat/__main__.py` +- Modify: `test_llamachat.py` + +- [ ] **Step 1: Write the failing tests** + +Add `test_provider_malformed_shapes()` after `test_provider_parsing()`, +asserting that every shape above leaves a working local provider, that a +`None` entry is skipped, and that each skip names the provider on stderr +(capture it with `contextlib.redirect_stderr`). + +Add `test_unusable_config_exits()`, which runs the real entry point in a +child process. It must be a subprocess: `CONFIG_PATH` is read from the +environment at import time and bound into `load()`'s default argument, so +rebinding the constant after import does nothing. Only a fresh interpreter +under a redirected `XDG_CONFIG_HOME` moves the file the app actually reads. +Use a syntax error that carries a line number, such as `socket = = ""`; +"unclosed array" is reported without one. Assert `returncode == 1` and, the +load-bearing one, that `"Traceback"` is absent from stderr. + +Register both in the `__main__` block: + +```python + test_provider_parsing() + test_provider_malformed_shapes() + test_unusable_config_exits() +``` + +- [ ] **Step 2: Run the suite to verify it fails** + +Run: `./test_llamachat.py` +Expected: FAIL with `ValueError: dictionary update sequence element #0 has +length 1; 2 is required`, and, for the entry-point test, an `AssertionError` +showing a real traceback captured from the child. + +- [ ] **Step 3: Fix `providers.py`** + +Skip, do not raise. This matches the convention Tasks 1 and 2 set for an +empty name, a colon in a name, and a missing `base_url`. The reasoning: a +`providers` key that is not a table is not a partial table, so falling +through to the bare-`base_url` synthesis means the local provider still +works and the user loses only what they never had. A malformed single entry +costs one cloud provider, and the local path is untouched. + +Add `import sys`, and a helper above `parse()`: + +```python +def _skipped(name: str, reason: str) -> None: + """Say that a provider was dropped, so the loss is not silent. + + A skipped provider is invisible: its models are simply absent from the + picker, which looks identical to the provider being down. One stderr line + gives a user running from a terminal something to act on. + + ponytail: stderr only, so a launch from a .desktop file still says + nothing. Surfacing this in the window is Task 12's job; there is no + window to put it in yet. + """ + print(f"llamachat: ignoring provider {name!r}: {reason}", file=sys.stderr) +``` + +Replace the table coercion and the local merge at the top of `parse()`: + +```python + raw = values.get("providers") + # `providers = "oops"` is valid TOML, and coercing it raises: ValueError + # for a string or list, TypeError for a number, which is why this is an + # isinstance test rather than a try/except. A providers key that is not a + # table is not a partial table, it is a different shape entirely, so fall + # through to the bare base_url path below and keep local working. + table = dict(raw) if isinstance(raw, dict) else {} +``` + +and, in the merge below it, read the local entry through `isinstance` rather +than truthiness, because `[[providers.local]]` is a truthy list that would +raise here before the loop could skip it: + +```python + bare = values.get("base_url") + local = table.get(LOCAL) if isinstance(table.get(LOCAL), dict) else {} + if bare and not local.get("base_url"): + table[LOCAL] = {**local, "base_url": bare} +``` + +In the loop, replace the `entry = entry or {}` guard, which caught falsy +values but not a truthy non-dict. The isinstance test subsumes the `None` +case, so nothing is lost by dropping it: + +```python + for name, entry in table.items(): + name = str(name) + if not isinstance(entry, dict): + # Most likely [[providers.x]] written for [providers.x], which + # TOML reads as a list of tables. Skipping keeps every other + # provider working, including local. + _skipped(name, "it is not a [providers.<name>] table") + continue +``` + +Give the two existing skips the same voice, naming what was wrong: +`_skipped(name, "a provider name cannot be empty or contain ':'")` and +`_skipped(name, "it has no base_url")`. + +- [ ] **Step 4: Fix `__main__.py`** + +Wrap the `config.load()` call in `main()`: + +```python + try: + cfg = config.load() + except Exception as exc: + # Deliberately broad. The point is that no config problem may kill a + # launch silently, and enumerating the coercion errors would be both + # longer and out of date the next time a config key is added. + # + # Failing beats falling back to DEFAULTS: that would quietly point the + # app at localhost:8181 and a presets.ini the user may not have, which + # is a different app than the one they configured. + print( + f"llamachat: {config.CONFIG_PATH} is unusable: {exc}", + file=sys.stderr, + ) + return 1 +``` + +`return 1` rather than a `DEFAULTS` fallback is deliberate, for the reason in +the comment. The broad `except` is deliberate too and should not be narrowed. +`tomllib.TOMLDecodeError` messages already carry line numbers, so a plain +syntax error produces a good message through this path with no extra work. + +- [ ] **Step 5: Run the suite to verify it passes** + +Run: `./test_llamachat.py` +Expected: PASS, including `ok malformed provider shapes are skipped, not +raised` and `ok unusable config exits without a traceback`. + +Then verify against the real entry point, which is what the review actually +reproduced. For each shape, write it to a temp config and run +`XDG_CONFIG_HOME=$T python3 -m llamachat --ping`. Every provider shape must +exit 0 with `local` still pointing at its configured URL; the syntax error +must exit 1 with the path and line number and no traceback. + +- [ ] **Step 6: Commit** + +```bash +git add llamachat/providers.py llamachat/__main__.py test_llamachat.py \ + docs/superpowers/plans/2026-08-09-external-providers.md +git commit -m "fix: survive malformed provider config at startup + +Eight config shapes, all valid TOML, crashed config.load() before any +window existed, so the user got a traceback on a terminal a desktop +launch does not have. Skip malformed providers instead, keeping the +local path working, and say on stderr which one was dropped and why. + +A config that cannot load at all now reports the file and exits rather +than half-starting on defaults the user never configured. + +Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" +``` + +--- + ## Task 5: models.ini storage **Files:** diff --git a/llamachat/__main__.py b/llamachat/__main__.py index 0faf88a..b8979e4 100644 --- a/llamachat/__main__.py +++ b/llamachat/__main__.py @@ -64,7 +64,21 @@ def main(argv: list[str] | None = None) -> int: print(f"llamachat {__version__}") return 0 - cfg = config.load() + try: + cfg = config.load() + except Exception as exc: + # Deliberately broad. The point is that no config problem may kill a + # launch silently, and enumerating the coercion errors would be both + # longer and out of date the next time a config key is added. + # + # Failing beats falling back to DEFAULTS: that would quietly point the + # app at localhost:8181 and a presets.ini the user may not have, which + # is a different app than the one they configured. + print( + f"llamachat: {config.CONFIG_PATH} is unusable: {exc}", + file=sys.stderr, + ) + return 1 for name in ("toggle", "show", "hide", "ping", "quit"): if getattr(args, name): diff --git a/llamachat/providers.py b/llamachat/providers.py index 56466a3..52a1132 100644 --- a/llamachat/providers.py +++ b/llamachat/providers.py @@ -15,6 +15,7 @@ import os import subprocess +import sys from dataclasses import dataclass, field # The local llama.cpp router. Its models are shown and stored without a @@ -58,13 +59,33 @@ def _number(raw, cast): return None +def _skipped(name: str, reason: str) -> None: + """Say that a provider was dropped, so the loss is not silent. + + A skipped provider is invisible: its models are simply absent from the + picker, which looks identical to the provider being down. One stderr line + gives a user running from a terminal something to act on. + + ponytail: stderr only, so a launch from a .desktop file still says + nothing. Surfacing this in the window is Task 12's job; there is no + window to put it in yet. + """ + print(f"llamachat: ignoring provider {name!r}: {reason}", file=sys.stderr) + + def parse(values: dict) -> dict[str, Provider]: """Build the provider table from already-loaded config values. Takes the raw dict rather than a path so config.py owns file reading and this stays testable without touching disk. """ - table = dict(values.get("providers") or {}) + raw = values.get("providers") + # `providers = "oops"` is valid TOML, and coercing it raises: ValueError + # for a string or list, TypeError for a number, which is why this is an + # isinstance test rather than a try/except. A providers key that is not a + # table is not a partial table, it is a different shape entirely, so fall + # through to the bare base_url path below and keep local working. + table = dict(raw) if isinstance(raw, dict) else {} # An old config has only a bare base_url. Fill it in as the local # provider's URL so nothing needs migrating, but never override an @@ -72,24 +93,36 @@ def parse(values: dict) -> dict[str, Provider]: # [providers.local] that only sets an api_key is adding detail to the # provider the user already has, not replacing it, and treating it as a # replacement would silently delete local entirely. + # + # The local entry is read through isinstance too, not just truthiness: a + # [[providers.local]] is a truthy list, and merging into it would raise + # here, before the loop below ever gets a chance to skip it. bare = values.get("base_url") - if bare and not (table.get(LOCAL) or {}).get("base_url"): - table[LOCAL] = {**(table.get(LOCAL) or {}), "base_url": bare} + local = table.get(LOCAL) if isinstance(table.get(LOCAL), dict) else {} + if bare and not local.get("base_url"): + table[LOCAL] = {**local, "base_url": bare} out: dict[str, Provider] = {} for name, entry in table.items(): - entry = entry or {} name = str(name) + if not isinstance(entry, dict): + # Most likely [[providers.x]] written for [providers.x], which + # TOML reads as a list of tables. Skipping keeps every other + # provider working, including local. + _skipped(name, "it is not a [providers.<name>] table") + continue if not name or ":" in name: # The id scheme splits on the first colon, so a name containing # one builds ids that split back to something else entirely, and # an empty name builds ":model". Both route to local under a # nonsense name, silently. Skipping is the only honest option. + _skipped(name, "a provider name cannot be empty or contain ':'") continue base_url = str(entry.get("base_url") or "").rstrip("/") if not base_url: # ponytail: a provider with no URL is misconfigured, not a # partial one. Skipping beats inventing a default endpoint. + _skipped(name, "it has no base_url") continue # filter = "qwen" is an easy TOML slip for filter = ["qwen"], and # iterating the string would turn it into four single-character diff --git a/test_llamachat.py b/test_llamachat.py index 0eea9e1..ed50995 100755 --- a/test_llamachat.py +++ b/test_llamachat.py @@ -1003,6 +1003,96 @@ def test_provider_parsing(): print("ok provider config parsing") +def test_provider_malformed_shapes(): + """Config shapes that are not tables are skipped, never raised on. + + Every case here is valid TOML a hand-editing user can write, and every + one of them used to reach the GUI as a traceback before any window + existed. The invariant is that the local provider survives all of them. + """ + import io + from contextlib import redirect_stderr + + from llamachat import providers + + # `providers` itself is not a table. Three shapes, and note they used to + # raise two different exception types, which is why the guard is an + # isinstance test and not a try/except. + for bad in ("oops", ["a"], 5): + fallen_back = providers.parse( + {"base_url": "http://localhost:8181", "providers": bad} + ) + assert set(fallen_back) == {"local"}, bad + assert fallen_back["local"].base_url == "http://localhost:8181" + + # A single entry that is not a table. The list case is the one that + # matters: [[providers.a]] is a plausible slip for [providers.a]. + for bad in (5, "x", ["x"], [{"base_url": "http://y.example.org"}]): + mixed = providers.parse( + {"providers": {"local": {"base_url": "http://x.example.org"}, + "a": bad}} + ) + assert set(mixed) == {"local"}, bad + + # A None entry, which TOML cannot produce but callers can, is still + # skipped rather than crashing. + assert providers.parse({"providers": {"a": None}}) == {} + + # Each skip says which provider it dropped and why, so a user running + # from a terminal has something to act on. + err = io.StringIO() + with redirect_stderr(err): + providers.parse( + { + "providers": { + "listy": [{"base_url": "http://x.example.org"}], + "urlless": {"api_key": "env:SOME_VAR"}, + "a:b": {"base_url": "http://y.example.org"}, + } + } + ) + messages = err.getvalue() + assert "listy" in messages and "urlless" in messages and "a:b" in messages + assert "base_url" in messages # the missing-URL case names what is missing + print("ok malformed provider shapes are skipped, not raised") + + +def test_unusable_config_exits(): + """A config that cannot load exits 1 with a message, not a traceback. + + Runs the real entry point in a child process rather than calling main() + in-process. CONFIG_PATH is read from the environment at import time and + baked into load()'s default argument, so rebinding the constant after + import does nothing: only a fresh interpreter with a redirected + XDG_CONFIG_HOME actually moves the file the app reads. + """ + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "llamachat" / "config.toml" + path.parent.mkdir(parents=True) + # A plain syntax error, the commonest way a hand-edited file breaks. + path.write_text('base_url = "http://localhost:8181"\nsocket = = ""\n') + + env = dict(os.environ, XDG_CONFIG_HOME=tmp) + # --ping is the cheapest path that still loads the config, and it + # fails before it looks for a socket, so an instance actually running + # on this machine cannot turn this into a false pass. + proc = subprocess.run( + [sys.executable, "-m", "llamachat", "--ping"], + capture_output=True, text=True, env=env, + cwd=str(Path(__file__).resolve().parent), + ) + + assert proc.returncode == 1, proc.returncode + # A traceback here would mean the app died rather than reported. + assert "Traceback" not in proc.stderr, proc.stderr + # The path is what makes the message actionable: it says which file to fix. + assert str(path) in proc.stderr, proc.stderr + assert "unusable" in proc.stderr + # tomllib names the line, and that detail survives into the report. + assert "line" in proc.stderr, proc.stderr + print("ok unusable config exits without a traceback") + + def test_model_ids_and_filtering(): """Ids are provider:model, local stays bare, filters are substrings.""" from llamachat import providers @@ -1986,6 +2076,8 @@ if __name__ == "__main__": test_venv_discovery() test_config_defaults() test_provider_parsing() + test_provider_malformed_shapes() + test_unusable_config_exits() test_model_ids_and_filtering() test_key_resolution() test_config_providers() |