aboutsummaryrefslogtreecommitdiffstats
path: root/docs
diff options
context:
space:
mode:
Diffstat (limited to 'docs')
-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()`: