diff options
Diffstat (limited to 'docs/superpowers')
| -rw-r--r-- | docs/superpowers/plans/2026-08-09-external-providers.md | 199 |
1 files changed, 177 insertions, 22 deletions
diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md index 3fd1e2f..88e050c 100644 --- a/docs/superpowers/plans/2026-08-09-external-providers.md +++ b/docs/superpowers/plans/2026-08-09-external-providers.md @@ -1175,15 +1175,84 @@ def test_models_store(): # A model id with a colon must survive being an ini section name. again.save("together:org/name:v2", models.ModelInfo(ctx_size=4096)) assert models.ModelStore(path).get("together:org/name:v2").ctx_size == 4096 + + # False must survive as False, not degrade to None: "no vision" is a + # real answer that shadows a provider default, unlike "unknown". + # Pins the lowercase wire format too, which Task 15 documents for + # hand-editing. (Collapsing the bool branch in save() would still + # pass, since _get_bool lowercases: this guards the meaning, not + # that one branch.) + again.save("together:novision", models.ModelInfo(vision=False)) + assert models.ModelStore(path).get("together:novision").vision is False + assert "vision = false" in path.read_text(encoding="utf-8") + + # Cancelling a dialog over a model we already know must not erase it, + # nor mark it skipped: the marker means "no real keys", so a section + # holding both would be a state no reader is written to expect. + again.mark_skipped("together:Qwen/Qwen2.5") + assert models.ModelStore(path).get("together:Qwen/Qwen2.5").ctx_size == 32768 + assert models.SKIPPED not in path.read_text(encoding="utf-8").split( + "[together:Qwen/Qwen2.5]" + )[1].split("[")[0] + + # A model literally named DEFAULT must not write a [DEFAULT] section: + # a stock-configparser reader, which Task 15 invites by documenting + # this file, would read it as inherited defaults for every model. + again.save("DEFAULT", models.ModelInfo(ctx_size=2048)) + assert "[DEFAULT]" not in path.read_text(encoding="utf-8") + escaped = models.ModelStore(path) + assert escaped.get("DEFAULT").ctx_size == 2048 + assert escaped.was_offered("DEFAULT") is True + # The escape must not swallow a neighbouring id. + assert escaped.was_offered("DEFAULTS") is False + + # A hand-edited file must degrade, not raise: "32k" is not an int. + # The [DEFAULT] value is deliberately a *valid* int, so this catches + # the leak itself rather than an unparseable value hiding it. + path.write_text( + "[DEFAULT]\nctx_size = 999\n\n" + "[together:junk]\nctx_size = 32k\nprice_in = free\n\n" + "[together:empty]\n", + encoding="utf-8", + ) + edited = models.ModelStore(path) + assert edited.get("together:junk") is None + assert edited.was_offered("together:junk") is True + # Would be ctx_size 999, inherited from [DEFAULT], if the store used + # configparser's real default section. + assert edited.get("together:empty") is None + + # nan and inf parse cleanly through float(), so they would reach the + # cost arithmetic and render as "$nan". Unknown is the honest answer. + path.write_text( + "[together:nan]\nprice_in = nan\nprice_out = inf\n\n" + "[together:neg]\nprice_in = -inf\nctx_size = 8192\n", + encoding="utf-8", + ) + weird = models.ModelStore(path) + assert weird.get("together:nan") is None + # A bad price must not take the good ctx_size down with it. + assert weird.get("together:neg").price_in is None + assert weird.get("together:neg").ctx_size == 8192 + + # A file that is not ini at all reads as empty rather than raising. + path.write_text("this is not an ini file\n", encoding="utf-8") + assert models.ModelStore(path).was_offered("together:junk") is False + + # Nor may a binary file take the app down at startup: that raises + # UnicodeDecodeError, which is not a configparser.Error. + path.write_bytes(b"\xff\xfe\x00not utf-8 at all\x00") + assert models.ModelStore(path).was_offered("together:junk") is False print("ok models.ini storage") ``` + Register it after `test_config_providers()`. - [ ] **Step 2: Run the suite to verify it fails** Run: `./test_llamachat.py` -Expected: FAIL with `ModuleNotFoundError: No module named 'llamachat.models'` +Expected: FAIL with `ImportError: cannot import name 'models' from 'llamachat'` - [ ] **Step 3: Write the implementation** @@ -1193,16 +1262,34 @@ Create `llamachat/models.py` with the GPL header copied from `config.py`, then: """Per-model metadata: what presets.ini cannot answer for a cloud model.""" import configparser +import math +import os from dataclasses import dataclass from pathlib import Path +# configparser reads a literal [DEFAULT] as inherited defaults for every +# other section. A model genuinely named DEFAULT is stored under this name +# instead, so the file stays safe for the stock-configparser readers that +# Task 15 invites by documenting it for hand-editing. +# +# ponytail: only the exact id "DEFAULT" collides. A file already containing +# a hand-written [DEFAULT] is handled by _empty() below, not here, and no +# other [DEFAULT]-alike is defended against. Provider ids arrive as +# "provider:model", so a bare DEFAULT can only be a local model. +_DEFAULT_ID = "DEFAULT" +_DEFAULT_ESCAPED = "\x00llamachat-model-DEFAULT" + # Recorded when the dialog is cancelled, so a model tried once never nags. -SKIPPED = "configured" +# Only the section's presence is read, never this value; the key exists to +# give an otherwise empty section something readable in the file. It means +# "no real keys here", and mark_skipped() is its only writer. +SKIPPED = "offered" @dataclass class ModelInfo: """What a model can tell us. Every field may be unknown.""" + ctx_size: int | None = None vision: bool | None = None price_in: float | None = None @@ -1220,9 +1307,18 @@ def _get(section, key, cast): if not raw: return None try: - return cast(raw) + value = cast(raw) except ValueError: + # A hand-edited "32k" or "3.7" for an int degrades to unknown rather + # than raising: the file is user-editable, and one bad line must not + # take down every other model's metadata with it. return None + # float() accepts "nan" and "inf", which would survive every later check + # and surface as a "$nan" cost. Caught here rather than at each cost call + # site, so the arithmetic downstream can assume a real number. + if cast is float and not math.isfinite(value): + return None + return value def _get_bool(section, key): @@ -1242,23 +1338,51 @@ class ModelStore: """ def __init__(self, path: Path): - self.path = path + self.path = Path(path) # Model ids contain colons and slashes, so no key/value delimiter - # may be inferred from a section name. Sections are safe as-is. - self.parser = configparser.ConfigParser(interpolation=None) - if path.exists(): + # may be inferred from a section name. Sections are safe as-is: + # brackets, colons, equals and percent signs all round-trip, and + # interpolation is off so a "%" in an id or value is literal. + self.parser = self._empty() + if self.path.exists(): try: - self.parser.read(path, encoding="utf-8") - except configparser.Error: + self.parser.read(self.path, encoding="utf-8") + except (configparser.Error, OSError, UnicodeDecodeError): # ponytail: a corrupt file reads as empty; the dialog can - # rewrite it. Failing to start over metadata is worse. - self.parser = configparser.ConfigParser(interpolation=None) + # rewrite it. Failing to start over metadata is worse. The + # cost is that the next save silently discards whatever was + # readable, which is acceptable for a cache of prices. + self.parser = self._empty() + + @staticmethod + def _empty(): + # default_section is renamed away from "DEFAULT" so that a hand-added + # [DEFAULT] in the file is read as an ordinary section rather than as + # defaults inherited by every other one. Without this, one such block + # silently gives every model its ctx_size and prices, including + # models with no entry at all. Reserving a name containing a NUL, + # which no model id can hold, keeps every section independent. + return configparser.ConfigParser( + interpolation=None, default_section="\x00llamachat-default" + ) + + @staticmethod + def _key(model_id: str) -> str: + """The section name for a model id. Only 'DEFAULT' is rewritten.""" + return _DEFAULT_ESCAPED if model_id == _DEFAULT_ID else model_id def get(self, model_id: str) -> ModelInfo | None: - """Stored metadata, or None when there is none worth having.""" - if not self.parser.has_section(model_id): + """Stored metadata, or None when there is none worth having. + + None and was_offered() True together mean "asked, learned nothing": + either the dialog was cancelled or every field was left blank. The + caller wants exactly that distinction, so it is not a trap: get() + answers "what do we know", was_offered() answers "should we ask". + """ + name = self._key(model_id) + if not self.parser.has_section(name): return None - section = self.parser[model_id] + section = self.parser[name] info = ModelInfo( ctx_size=_get(section, "ctx_size", int), vision=_get_bool(section, "vision"), @@ -1269,7 +1393,7 @@ class ModelStore: def was_offered(self, model_id: str) -> bool: """Whether the dialog has already been shown for this model.""" - return self.parser.has_section(model_id) + return self.parser.has_section(self._key(model_id)) def save(self, model_id: str, info: ModelInfo) -> None: section = self._section(model_id) @@ -1282,30 +1406,61 @@ class ModelStore: if value is None: section.pop(key, None) elif isinstance(value, bool): + # Before the numeric branch: bool is a subclass of int, so + # str(False) would otherwise write "False" for vision. section[key] = "true" if value else "false" else: section[key] = str(value) + # The marker means "no real keys", and mark_skipped() is its only + # writer. Dropping it here keeps that invariant with one enforcer + # instead of two that have to agree. A save with every field blank + # leaves an empty section, which configparser round-trips fine. section.pop(SKIPPED, None) self._write() def mark_skipped(self, model_id: str) -> None: - """Record a cancelled dialog: offered, declined, do not ask again.""" + """Record a cancelled dialog: offered, declined, do not ask again. + + Never clears fields already stored. Cancelling a dialog opened over + a model we know about must not delete what we know. + """ section = self._section(model_id) if not any(k != SKIPPED for k in section): - section[SKIPPED] = "false" + section[SKIPPED] = "true" self._write() def _section(self, model_id: str): - if not self.parser.has_section(model_id): - self.parser.add_section(model_id) - return self.parser[model_id] + name = self._key(model_id) + if not self.parser.has_section(name): + self.parser.add_section(name) + return self.parser[name] def _write(self) -> None: + """Rewrite the whole file from the in-memory parser. + + ponytail: last writer wins. Each store holds a full parser, so two + live instances would drop each other's sections rather than merge. + Not defended against because ipc.py enforces a single instance and + the window holds exactly one store, making a second writer a case + that cannot arise. The upgrade path, if the app ever grows a second + writer, is to reread before each save and merge. + """ self.path.parent.mkdir(parents=True, exist_ok=True) - with open(self.path, "w", encoding="utf-8") as fh: - self.parser.write(fh) + # Written to a sibling and renamed: os.replace is atomic within a + # directory, so a crash mid-write leaves the old file intact instead + # of a truncated one. Cheap here, and the alternative is losing every + # recorded price to one bad moment. + tmp = self.path.with_name(self.path.name + f".{os.getpid()}.tmp") + try: + with open(tmp, "w", encoding="utf-8") as fh: + self.parser.write(fh) + os.replace(tmp, self.path) + except OSError: + tmp.unlink(missing_ok=True) + raise ``` + - [ ] **Step 4: Run the suite to verify it passes** Run: `./test_llamachat.py` |