aboutsummaryrefslogtreecommitdiffstats
path: root/docs
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-09 15:14:14 +0200
committerDanilo M. <danix@danix.xyz>2026-08-09 15:14:14 +0200
commit009496882033fa9f01e0faaffe9d6d797650b621 (patch)
tree7a20b0fa43818e482dd6467e75c195006f644a69 /docs
parent05184385699325efa830a18c4b05380df79bc12c (diff)
downloadllamachat-009496882033fa9f01e0faaffe9d6d797650b621.tar.gz
llamachat-009496882033fa9f01e0faaffe9d6d797650b621.zip
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. The guards are isinstance tests rather than a try/except because the shapes raise three different exception types, and the local entry is checked the same way: [[providers.local]] is a truthy list that raised during the bare base_url merge, before the loop could skip it. 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> Claude-Session: https://claude.ai/code/session_0176tzAW6H1i2Kz8vm2XXVGV
Diffstat (limited to 'docs')
-rw-r--r--docs/superpowers/plans/2026-08-09-external-providers.md182
1 files changed, 182 insertions, 0 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:**