diff options
Diffstat (limited to 'docs/superpowers/plans')
| -rw-r--r-- | docs/superpowers/plans/2026-08-09-external-providers.md | 182 |
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:** |