diff options
Diffstat (limited to 'docs/superpowers')
| -rw-r--r-- | docs/superpowers/plans/2026-08-09-external-providers.md | 198 |
1 files changed, 169 insertions, 29 deletions
diff --git a/docs/superpowers/plans/2026-08-09-external-providers.md b/docs/superpowers/plans/2026-08-09-external-providers.md index 72db023..d34fffc 100644 --- a/docs/superpowers/plans/2026-08-09-external-providers.md +++ b/docs/superpowers/plans/2026-08-09-external-providers.md @@ -188,11 +188,14 @@ from dataclasses import dataclass, field # prefix, so an existing session pointing at a local model still resolves. LOCAL = "local" -# A stuck pinentry must not freeze the worker thread forever. -KEY_TIMEOUT = 30 +# A stuck pinentry must not freeze the worker thread forever. Generous +# because a legitimate unlock is slow: a graphical pinentry plus a hardware +# token the user has to physically find and touch is a normal minute. This +# bounds the pathological case, it is not a deadline for the human. +KEY_TIMEOUT = 120 -class KeyError_(Exception): +class KeyResolutionError(Exception): """An api_key that could not be resolved, phrased for the user.""" @@ -530,7 +533,7 @@ def test_key_resolution(): try: resolver.resolve(missing) assert False, "a missing env var must raise" - except providers.KeyError_ as exc: + except providers.KeyResolutionError as exc: assert "gone" in str(exc) # pass: shells out. Substitute the runner rather than requiring gpg. @@ -553,6 +556,17 @@ def test_key_resolution(): assert cached.resolve(passed) == "line-one" assert len(calls) == 1 + # But the cache follows the spec, not just the name. A reloaded config + # that points the same provider at a different entry must re-resolve, + # otherwise correcting a wrong entry appears to do nothing. + moved = providers.Provider( + name="together", base_url="http://x.example.org", + api_key="pass:api/together-corrected", + ) + assert cached.resolve(moved) == "line-one" + assert len(calls) == 2 + assert calls[1][0] == ["pass", "show", "api/together-corrected"] + # A failing pass is reported, naming the provider. def boom(cmd, timeout): raise OSError("pass: entry not found") @@ -560,14 +574,69 @@ def test_key_resolution(): try: providers.KeyResolver(runner=boom).resolve(passed) assert False, "a failing pass must raise" - except providers.KeyError_ as exc: + except providers.KeyResolutionError as exc: + assert "together" in str(exc) + + # A timeout points at the pinentry never appearing, which is the silent + # case, rather than at one the user can already see. + def slow(cmd, timeout): + raise subprocess.TimeoutExpired(cmd, timeout, output="partial-secret") + + try: + providers.KeyResolver(runner=slow).resolve(passed) + assert False, "a pass timeout must raise" + except providers.KeyResolutionError as exc: + assert "gpg-agent" in str(exc) + # The partial stdout a timeout captures must never reach the message. + assert "partial-secret" not in str(exc) + # `from None` suppresses the chained traceback. It does not clear + # __context__, so this pins the display behavior, not unreachability. + assert exc.__suppress_context__ and exc.__cause__ is None + + # A non-zero exit quotes gpg's stderr, which is the only useful part, and + # never stdout, which is where the secret would be. + def refused(cmd, timeout): + raise subprocess.CalledProcessError( + 2, cmd, output="sk-test-not-a-real-key\n", + stderr="gpg: decryption failed: No secret key\n", + ) + + try: + providers.KeyResolver(runner=refused).resolve(passed) + assert False, "a non-zero pass exit must raise" + except providers.KeyResolutionError as exc: + assert "No secret key" in str(exc) + assert "sk-test-not-a-real-key" not in str(exc) + + # An empty or absent stderr falls back to the exit status rather than + # reporting a blank reason. + for blank in ("", None): + def quiet(cmd, timeout, _s=blank): + raise subprocess.CalledProcessError(3, cmd, output="", stderr=_s) + + try: + providers.KeyResolver(runner=quiet).resolve(passed) + assert False, "a non-zero pass exit must raise" + except providers.KeyResolutionError as exc: + assert "exit status 3" in str(exc) + assert exc.__suppress_context__ and exc.__cause__ is None + + # A missing `pass` binary names the binary, not just "No such file". + def absent(cmd, timeout): + raise FileNotFoundError(2, "No such file or directory", "pass") + + try: + providers.KeyResolver(runner=absent).resolve(passed) + assert False, "a missing pass binary must raise" + except providers.KeyResolutionError as exc: + assert "not installed" in str(exc) assert "together" in str(exc) # Empty output is a failure too: an empty key would 401 confusingly. try: providers.KeyResolver(runner=lambda cmd, timeout: " \n").resolve(passed) assert False, "empty pass output must raise" - except providers.KeyError_: + except providers.KeyResolutionError: pass print("ok api key resolution") ``` @@ -602,20 +671,34 @@ class KeyResolver: def __init__(self, runner=_run_pass): self._runner = runner - self._cache: dict[str, str] = {} + # Not locked, and that is only safe while every caller resolves on the + # GUI thread, where the Qt event loop serializes them. Moving a + # resolve onto a worker thread, or threading the model fan-out, breaks + # it: two threads can miss the cache together, spawn two `pass` + # processes and raise two pinentry prompts for one token. Whoever does + # that adds a threading.Lock here and holds it across the whole + # resolve() body, not just the dict access, since guarding only the + # dict leaves both threads outside the lock during the subprocess, + # which is the double-prompt window itself. + self._cache: dict[tuple[str, str], str] = {} def resolve(self, provider: Provider) -> str: """The bearer token for this provider, or '' when it needs none.""" spec = provider.api_key if not spec: return "" - if provider.name in self._cache: - return self._cache[provider.name] + # Keyed by the spec as well as the name: the config can be reloaded + # onto the same resolver, and keying by name alone would keep serving + # the old key after the user edits the entry to fix it, so the fix + # would look like it did nothing until a restart. + cache_key = (provider.name, spec) + if cache_key in self._cache: + return self._cache[cache_key] if spec.startswith("env:"): value = os.environ.get(spec[4:], "") if not value: - raise KeyError_( + raise KeyResolutionError( f"{provider.name}: environment variable {spec[4:]} is not set" ) elif spec.startswith("pass:"): @@ -623,24 +706,53 @@ class KeyResolver: else: value = spec - self._cache[provider.name] = value + self._cache[cache_key] = value return value def _from_pass(self, provider: Provider, entry: str) -> str: try: out = self._runner(["pass", "show", entry], timeout=KEY_TIMEOUT) + # Every raise below is `from None`. Both TimeoutExpired and + # CalledProcessError carry a .stdout that can hold a partial secret, + # and this keeps it out of any printed traceback. Note it suppresses + # display only: __context__ still references the original, so the + # real guarantee is that no handler here puts stdout in the message. except subprocess.TimeoutExpired: - raise KeyError_( + # The likely cause is a pinentry that never appeared, not one + # sitting in front of the user: no $DISPLAY inherited, no + # gpg-agent, or pinentry-qt failing to open and falling back to a + # curses prompt on a terminal a GUI app has no stdin for. + raise KeyResolutionError( f"{provider.name}: `pass show {entry}` timed out after " - f"{KEY_TIMEOUT}s. Is a pinentry waiting for input?" - ) + f"{KEY_TIMEOUT}s. If no pinentry appeared, check that " + f"gpg-agent is running and that pinentry-qt can open a window." + ) from None + except FileNotFoundError: + # Errno 2 alone does not say which file is missing, and this is + # the common "pass was never installed" case. + raise KeyResolutionError( + f"{provider.name}: `pass` is not installed or not on PATH" + ) from None + except subprocess.CalledProcessError as exc: + # stderr is captured, so without this it is swallowed and the user + # gets an exit code where gpg told them exactly what was wrong. + # Only stderr is quoted, never stdout, which holds the secret. + detail = (exc.stderr or "").strip().splitlines() + reason = detail[-1] if detail else f"exit status {exc.returncode}" + raise KeyResolutionError( + f"{provider.name}: `pass show {entry}` failed: {reason}" + ) from None except Exception as exc: - raise KeyError_(f"{provider.name}: `pass show {entry}` failed: {exc}") + raise KeyResolutionError( + f"{provider.name}: `pass show {entry}` failed: {exc}" + ) from None # A password store entry keeps the secret on the first line and # metadata below it. first = (out or "").strip().splitlines() if not first or not first[0].strip(): - raise KeyError_(f"{provider.name}: `pass show {entry}` returned nothing") + raise KeyResolutionError( + f"{provider.name}: `pass show {entry}` returned nothing" + ) return first[0].strip() ``` @@ -1641,7 +1753,7 @@ class MultiClient: for name, provider in self.table.items(): try: available = self._listing_client(provider).models() - except (BackendError, providers_mod.KeyError_) as exc: + except (BackendError, providers_mod.KeyResolutionError) as exc: problems.append(f"{name}: {exc}") continue kept = providers_mod.apply_filter(provider, available) @@ -2129,29 +2241,57 @@ In `_ensure_vision_model` (line 842), replace the opening check: Every call that currently does `self.client.<method>(model, ...)` must become a call on the routed client with the wire name. In `send()` (line 919) and `_start_stream()` (line 1040), the model passed to `StreamWorker` must be split. -Change `_start_stream` to resolve both up front: + +**Resolution must happen on the worker thread, not in `_start_stream`.** +`client_for()` resolves the API key, and for a `pass:` key that runs `pass show`, +which blocks on a pinentry for as long as it takes the user to find and touch +their hardware token. `_start_stream` runs on the GUI thread, before +`worker.moveToThread(thread)` and before `thread.start()`, so resolving there +freezes the whole window for that entire interaction, up to `KEY_TIMEOUT` +(120s). Pass the unresolved `model` into the worker and let the worker resolve. + +Give `StreamWorker` the `MultiClient` and the qualified `model` it already +takes, and do the routing at the top of `StreamWorker.run()`, inside the +existing try that already reports failures through the `failed` signal: ```python - def _start_stream(self, model: str, messages: list[dict]) -> None: + def run(self) -> None: try: - client = self.client.client_for(model) - except providers_mod.KeyError_ as exc: - self.show_status(str(exc), error=True) + client = self.client.client_for(self.model) + wire = self.client.wire_name(self.model) + except providers_mod.KeyResolutionError as exc: + self.failed.emit(str(exc)) return - wire = self.client.wire_name(model) + ... ``` -and pass `client` and `wire` to `StreamWorker` in place of `self.client` and -`model`. Add the import at the top of `ui.py`: +`failed` is already connected to a slot that calls `show_status(..., error=True)`, +so a key error reaches the user through the path a backend error already uses, +and it arrives as a normal queued signal on the GUI thread. Do not add a new +signal for it. Add the import at the top of `ui.py`: ```python from . import providers as providers_mod ``` -Apply the same treatment in `_start_titling` (line 1133), which builds a -`TitleWorker`: resolve `client` and `wire` the same way, and on `KeyError_` skip -titling silently rather than showing an error, since titling is never the user's -own turn. +Apply the same treatment to `TitleWorker` for `_start_titling` (line 1133): +resolve inside `TitleWorker.run()`, and on `KeyResolutionError` return without +emitting anything, so titling is skipped silently. Titling is never the user's +own turn, so a pinentry prompt or an error banner for it would be noise. This +preserves the current silent-skip behavior, it just moves where it happens. + +**This changes the threading invariant in `providers.py`.** `KeyResolver._cache` +is deliberately unlocked, and the comment on it says so, because at the time of +writing every caller resolved on the GUI thread and the Qt event loop serialized +them. Once resolution moves into `StreamWorker.run()` and `TitleWorker.run()` +that no longer holds: `_start_titling` can overlap with a live stream, so two +worker threads can call `resolve()` concurrently, miss the cache together, and +spawn two `pass` processes, raising two pinentry prompts for one token. When +executing this step, re-examine that comment and add a `threading.Lock` to +`KeyResolver` if the overlap is real. The lock must be held across the whole +`resolve()` body, not just the dict reads and writes: guarding only the dict +still leaves both threads outside the lock during the subprocess call, which is +exactly the double-prompt window. Update the `_cache` comment either way. - [ ] **Step 7: Verify the whole suite still passes** |