aboutsummaryrefslogtreecommitdiffstats
path: root/docs/superpowers
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-09 15:21:32 +0200
committerDanilo M. <danix@danix.xyz>2026-08-09 15:21:32 +0200
commit86c5cdb4bb94b8d6da93c42ac48ec1abb6fa87ef (patch)
tree47a68f23f5597b732589b8670be86998a50fbc8f /docs/superpowers
parent009496882033fa9f01e0faaffe9d6d797650b621 (diff)
downloadllamachat-86c5cdb4bb94b8d6da93c42ac48ec1abb6fa87ef.tar.gz
llamachat-86c5cdb4bb94b8d6da93c42ac48ec1abb6fa87ef.zip
fix: warn when [[providers.local]] discards its fields
A malformed local entry was the one skip that said nothing, on the reasoning that local still ends up working. It does, but the merge rebuilds it from the bare base_url, so an api_key, filter or ctx_size set on that entry is dropped without a word. The app then comes up looking healthy, which is the strongest possible signal that nothing is wrong, making this the case that most needs saying, not least. Local surviving at all depends on DEFAULTS supplying base_url, since that is what the rebuild reads. Noted in config.py so the key is not removed as redundant, and pinned by a test. The not-a-table wording now follows what was written: a list gets the single-bracket fix by name, while a scalar entry does not, since telling someone to fix a [[...]] they never typed points at the wrong line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0176tzAW6H1i2Kz8vm2XXVGV
Diffstat (limited to 'docs/superpowers')
-rw-r--r--docs/superpowers/plans/2026-08-09-external-providers.md54
1 files changed, 51 insertions, 3 deletions
diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md
index 4a428a1..3fd1e2f 100644
--- a/docs/superpowers/plans/2026-08-09-external-providers.md
+++ b/docs/superpowers/plans/2026-08-09-external-providers.md
@@ -177,6 +177,12 @@ Expected: FAIL with `ModuleNotFoundError: No module named 'llamachat.providers'`
Create `llamachat/providers.py`. Copy the 14-line GPL header verbatim from the top
of `llamachat/config.py`, then:
+> **This block is the state at the end of Task 1, not the final module.**
+> Tasks 2, 3 and 4b all amend `parse()` in place. In particular, `parse()` as
+> written here crashes on several malformed config shapes; Task 4b replaces
+> its coercion and loop guards. Read top-down and implement in order, but do
+> not copy this block as the finished function.
+
```python
"""Provider definitions, model-id namespacing and API key resolution."""
@@ -905,6 +911,10 @@ 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.
+`[[providers.local]]` is the worst of the eight, and the easiest to
+under-rate. It is the only one where the app comes up looking healthy, so it
+needs the loudest warning rather than the quietest. See Step 3.
+
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.
@@ -921,6 +931,12 @@ 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`).
+For `[[providers.local]]`, pin the loss and not only the survival: assert the
+warning is emitted, that `base_url` falls back to the bare one, and that
+`api_key`, `filter` and `ctx_size` set on that entry are discarded. Pin the
+message wording both ways too, that a list case says `not [[providers.x]]`
+and a scalar case does not mention brackets at all.
+
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
@@ -985,15 +1001,35 @@ Replace the table coercion and the local merge at the top of `parse()`:
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:
+raise here before the loop could skip it. Warn at this site too: the loop
+never sees this entry, because the merge replaces it:
```python
bare = values.get("base_url")
- local = table.get(LOCAL) if isinstance(table.get(LOCAL), dict) else {}
+ raw_local = table.get(LOCAL)
+ if raw_local is not None and not isinstance(raw_local, dict):
+ # Warned about here rather than left to the loop, which never sees it:
+ # the merge below replaces it with a synthesized entry, so every field
+ # the user set on it is dropped. That makes this the case where saying
+ # something matters most, not least. The app comes up working, local
+ # answers, and an api_key or ctx_size they set is simply gone, with a
+ # healthy-looking window as the only feedback.
+ _skipped(LOCAL, _not_a_table(LOCAL, raw_local))
+ local = raw_local if isinstance(raw_local, dict) else {}
if bare and not local.get("base_url"):
table[LOCAL] = {**local, "base_url": bare}
```
+This one is easy to talk yourself out of, since local still works afterwards.
+It is the case that most needs the warning: a working app is the strongest
+possible signal that nothing is wrong, so the discarded `api_key` or
+`ctx_size` has nothing else to announce it.
+
+Note also that local only survives because `config.DEFAULTS` always supplies
+`base_url` for the rebuild. Without a bare URL, `[[providers.local]]` yields
+no local provider at all. Add a comment in `config.py` saying that key is
+load-bearing, so nobody removes it as redundant.
+
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:
@@ -1005,7 +1041,7 @@ case, so nothing is lost by dropping it:
# 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")
+ _skipped(name, _not_a_table(name, entry))
continue
```
@@ -1013,6 +1049,18 @@ 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")`.
+The wording of the not-a-table message depends on what was written, so it
+lives in its own helper. A list is the double-bracket slip and should name the
+single-bracket fix; anything else was written as `x = 5`, where pointing at a
+bracket the user never typed would send them to fix the wrong line:
+
+```python
+def _not_a_table(name: str, entry) -> str:
+ if isinstance(entry, list):
+ return f"use [providers.{name}], not [[providers.{name}]]"
+ return f"a provider must be a [providers.{name}] table"
+```
+
- [ ] **Step 4: Fix `__main__.py`**
Wrap the `config.load()` call in `main()`: