From 82c6e3060d443f5b33726dbc58c93bbaff5390e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 14:59:33 +0000 Subject: [PATCH] A bare backend name was read as a model name, and the CLI's failures said nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects that hid each other. Both found by installing 0.1.2 from PyPI into a clean venv and driving it against a real `claude` v2.1.220. **`split_spec` only consulted BACKENDS when the spec contained a slash.** A bare backend name fell through to "assume the default backend, keep the whole string as the model", so: split_spec("claude-cli") -> ("claude-cli", "claude-cli") split_spec("mock") -> ("claude-cli", "mock") `--model claude-cli` therefore shelled out to `claude -p --model claude-cli` and was refused by the CLI on every call (3/3), while `grapharc models --check` went on reporting the backend `usable` and `grapharc models claude-cli` printed `model: claude-cli` without complaint — two commands whose job is to say whether a spec will work, both saying yes about one that never did. `--model mock` was the worse half: it named the *paid* subscription backend and spawned the real binary, so the double `models --check` describes as "scripted test double; never reaches a provider" reached for one. It happened to fail before billing only because `mock` is not a model name. This is the same "silently folded into a model name … fails much later with a confusing error" failure `split_spec` already refuses for a mistyped backend *with* a slash; it just could not see the case without one. A bare backend name now resolves to that backend. `claude-cli` takes its own default model and `mock` takes the scripted double (which ignores the model segment entirely). `openrouter`, `openai` and `ollama` front catalogues rather than a model, so a bare name there is refused with a spelling that works rather than a guess about what to bill someone for. Slash forms and bare *model* names are untouched. **The gateway read the wrong stream.** `claude -p` fails with a non-zero exit, an empty stderr, and its whole explanation in the JSON envelope on stdout: {"is_error": true, "result": "There's an issue with the selected model (claude-cli). It may not exist or you may not have access to it."} `_invoke_cli` reported `proc.stderr`, so the error was `claude -p exited 1: ` — a sentence that stops at the colon. That is how the bug above presented itself: as no message at all. The one string that would have diagnosed it in seconds was captured, held in `proc.stdout`, and discarded. Note the code already parses this shape a few lines below, but only on the `returncode == 0` path, and the CLI sets `is_error` *and* exits non-zero, so it took the branch that ignores stdout. stdout is read first now, falling back to stderr when it is not the documented envelope. The recovered text also feeds `_cli_failure`, which classifies transient-vs-deterministic and was previously classifying from `""`. Tests: every new test was confirmed to fail against the old code and pass against the new — including the mock one, which asserts no subprocess is *created* rather than just checking the returned type, since the type was what was wrong and a refactor could fix the type and still shell out. `BARE_BACKEND_MODEL["claude-cli"]` is a second copy of the model class's default so the registry need not import a backend to split a string, and a test pins the two together the way CI already pins `__version__` to the packaged version. Verified: `grapharc demo stage1 --model claude-cli` now completes (8 nodes, target_met, exit 0) where it previously died with an empty error; `--model mock` resolves to the double with no subprocess; `--model openrouter` exits 2 with an example. Full suite green on 3.12 and 3.13; ruff clean. Co-Authored-By: Claude Opus 5 --- README.md | 3 +- grapharc/gateway/claude_cli.py | 34 +++++++- grapharc/gateway/registry.py | 45 ++++++++++ tests/test_gateway.py | 147 +++++++++++++++++++++++++++++++++ 4 files changed, 226 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 8f6360c..ed47f93 100644 --- a/README.md +++ b/README.md @@ -529,7 +529,8 @@ Re-derived on 2026-07-28 by running each item, not by reading the commit log. - **A planning round is an envelope, not a measurement.** A `round` event used to carry the planner's `tokens` and the round's `duration_ms`, both of which `metrics`, `cost` and `replay` add on top of node totals — and the planner's spend was already reported by its own `plan` event, so it was counted twice, and a round's duration encloses the plan plus every node it ran. Neither is on the event now; both are on its `state_delta` as `round_tokens` / `round_iterations` / `round_duration_ms`, where no reader sums them. `RoundRecord.iterations` also holds a figure now rather than always `0`. - **The Claude CLI backend is completion-only.** Tool calling and structured output need one of the OpenAI-wire backends: `openrouter`, `openai`, or a local `ollama`. - **A session turn is synchronous**, and a runner claim is a claim rather than a lease — nothing reclaims a session whose runner died holding it. -- **A bare model spec resolves to the paid `claude-cli` backend.** `--model mock` does not reach the scripted double; it becomes the model name `mock` on the subscription backend. Only the slash form (`mock/anything`) reaches the double. A mistyped backend *with* a slash is rejected properly, exit 2. +- *Closed:* a **bare backend name was read as a model name**, because `split_spec` only consulted the backend list when the spec contained a slash. `--model claude-cli` — the backend `models --check` reports as `usable` — shelled out to `claude -p --model claude-cli` and was refused by the CLI on *every* call, and `--model mock` named the paid subscription backend and spawned the real binary, so the double documented as "never reaches a provider" reached for one. A bare backend name now resolves to that backend (`claude-cli` to its own default model, `mock` to the scripted double, which ignores the model segment anyway); `openrouter`, `openai` and `ollama` front catalogues rather than a model, so those are refused with an example spelling instead of a guess about what to bill you for. The slash forms and bare *model* names are unchanged. +- *Closed:* a failing `claude -p` **reported no reason at all**. The CLI exits non-zero with an empty stderr and puts its explanation in the JSON envelope on stdout; the gateway read only stderr, so the error was `claude -p exited 1: ` — a sentence that stops at the colon. Since the wrong-model bug above presented itself exactly that way, the two hid each other. stdout is read first now, and the recovered text also feeds the transient-vs-deterministic classifier, which was previously deciding from `""`. - **`.env` is found by walking up parent directories; `grapharc.toml` is not.** The config layer refuses an upward search on purpose — a run must not be governed by a file you did not know about. The credential loader predates that decision and still searches upward, so the thing that *spends money* is discovered more eagerly than the thing that *constrains* it. - **`grapharc run` has no budget unless you give it one.** Set any of `--max-tokens`, `--max-iterations`, `--max-seconds`, or `--max-concurrency`; without them each dimension is unlimited and the gate admits a topology of any worst-case cost. diff --git a/grapharc/gateway/claude_cli.py b/grapharc/gateway/claude_cli.py index f9c0677..ba1fd57 100644 --- a/grapharc/gateway/claude_cli.py +++ b/grapharc/gateway/claude_cli.py @@ -121,6 +121,34 @@ def _canonical_model(model: str) -> str: return model.split("/", 1)[1] if model.startswith("anthropic/") else model +def _payload_error(stdout: str) -> str: + """The CLI's own explanation of a failure, which it writes to *stdout*. + + `claude -p` reports a refused model, an expired login and their siblings + with a non-zero exit code, an **empty stderr**, and the whole reason in the + JSON envelope on stdout:: + + {"is_error": true, "result": "There's an issue with the selected model + (claude-cli). It may not exist or you may not have access to it."} + + Reading only stderr therefore produced ``claude -p exited 1: `` — a + sentence that stops at the colon — and discarded the one string that said + what was wrong. It matters for classification too: `_cli_failure` decides + transient-vs-deterministic from this text, and it was deciding from "". + + Returns "" when stdout is not the documented envelope, so the caller falls + back to stderr rather than inventing a reason. + """ + try: + payload = json.loads(stdout) + except (TypeError, ValueError): # JSONDecodeError is a ValueError + return "" + if not isinstance(payload, dict): + return "" + result = payload.get("result") + return str(result).strip() if result else "" + + def _cli_failure(message: str, evidence: str) -> GatewayError: """Build the right error class for a CLI failure from its own text. @@ -219,8 +247,10 @@ def _invoke_cli(self, argv: list[str], prompt: str, cwd: str) -> dict[str, Any]: ) from exc if proc.returncode != 0: - stderr = proc.stderr.strip() - raise _cli_failure(f"claude -p exited {proc.returncode}: {stderr[:500]}", stderr) + # stdout first: the CLI puts its reason in the JSON envelope there + # and leaves stderr empty. See `_payload_error`. + detail = _payload_error(proc.stdout) or proc.stderr.strip() + raise _cli_failure(f"claude -p exited {proc.returncode}: {detail[:500]}", detail) try: payload = json.loads(proc.stdout) diff --git a/grapharc/gateway/registry.py b/grapharc/gateway/registry.py index cb3845f..a581568 100644 --- a/grapharc/gateway/registry.py +++ b/grapharc/gateway/registry.py @@ -49,6 +49,31 @@ class UnknownBackendError(Exception): "mock": "mock", } +#: What a *bare* backend name resolves to. A backend belongs here only when it +#: can pick a model without being told: `claude-cli` has a documented default, +#: and the `mock` double ignores the model segment entirely (`get_model` builds +#: a `ScriptedChatModel` with no model argument at all). The remote catalogues +#: are deliberately absent — `openrouter` alone fronts thousands of models and +#: no default would be defensible, so a bare `openrouter` is an error rather +#: than a guess about which model to bill you for. +#: +#: The `claude-cli` value is a second copy of `ClaudeCodeCLIChatModel.model`'s +#: own default, kept here so this module does not import a backend just to +#: split a string. `test_the_bare_claude_cli_default_matches_the_model_class` +#: fails if the two ever drift, the same way CI already pins `__version__` to +#: the packaged version. +BARE_BACKEND_MODEL = { + "claude-cli": "claude-sonnet-5", + "mock": "mock", +} + +#: A concrete spec to show someone who typed a bare backend that needs a model. +_BARE_BACKEND_EXAMPLE = { + "openrouter": "openrouter/anthropic/claude-sonnet-4.5", + "openai": "openai/gpt-4o-mini", + "ollama": "ollama/llama3.1", +} + def split_spec(spec: str) -> tuple[str, str]: """Split `backend/model` — bare names get the default backend. @@ -66,10 +91,30 @@ def split_spec(spec: str) -> tuple[str, str]: someone typing it means. Reaching the same model through the broker is still `openrouter/openai/gpt-4o-mini`, because only the first segment is ever read as a backend. + + A **bare backend name** is read as a backend too, which it was not before: + the slash test above meant `claude-cli` fell through to the last line and + became the *model* `claude-cli`, so `--model claude-cli` shelled out to + `claude -p --model claude-cli` and was refused by the CLI on every call — + while `models --check` went on reporting the backend `usable`. `--model + mock` was worse than useless: it named the paid subscription backend and + spawned the real binary, so the double documented as "never reaches a + provider" reached for one. That is the same "silently folded into a model + name … fails much later with a confusing error" failure this function + already refuses for a mistyped backend *with* a slash; it just could not + see the case without one. """ head, sep, rest = spec.partition("/") if sep and head in BACKENDS: return head, rest + if not sep and spec in BACKENDS: + if spec in BARE_BACKEND_MODEL: + return spec, BARE_BACKEND_MODEL[spec] + raise UnknownBackendError( + f"{spec!r} names a backend, not a model, and it has no default " + f"model to fall back on — write {spec}/, for example " + f"{_BARE_BACKEND_EXAMPLE[spec]!r}" + ) if sep and head not in KNOWN_AUTHORS: raise UnknownBackendError( f"unknown backend {head!r} in spec {spec!r}; expected one of: " diff --git a/tests/test_gateway.py b/tests/test_gateway.py index 1697f35..2f2d30b 100644 --- a/tests/test_gateway.py +++ b/tests/test_gateway.py @@ -81,3 +81,150 @@ def test_live_cli_completes_a_prompt(): assert "pong" in msg.content.lower() assert model.last_usage is not None assert model.last_usage["total_tokens"] > 0 + + +# ---- a bare backend name is a backend, not a model --------------------------- +# +# `split_spec` only consulted `BACKENDS` when the spec contained a slash, so a +# bare backend name fell through to "assume the default backend, keep the whole +# string as the model". Two live failures came out of that, and both are gates +# below rather than prose. + + +@pytest.mark.parametrize( + "spec, expected", + [ + ("claude-cli", ("claude-cli", "claude-sonnet-5")), + ("mock", ("mock", "mock")), + # The slash forms and the bare-model form must be untouched by the fix. + ("claude-cli/claude-sonnet-5", ("claude-cli", "claude-sonnet-5")), + ("mock/whatever", ("mock", "whatever")), + ("claude-sonnet-5", ("claude-cli", "claude-sonnet-5")), + ("anthropic/claude-haiku-4.5", ("claude-cli", "anthropic/claude-haiku-4.5")), + ], +) +def test_a_bare_backend_name_resolves_to_that_backend(spec, expected): + """`--model claude-cli` used to mean "the model called claude-cli". + + It shelled out to `claude -p --model claude-cli`, which the CLI refuses on + every call — `There's an issue with the selected model (claude-cli)` — + while `grapharc models --check` went on reporting the backend `usable` and + `grapharc models claude-cli` printed `model: claude-cli` without complaint. + Both commands exist to say whether a spec will work, and both said yes + about one that never did. + """ + from grapharc.gateway.registry import split_spec + + assert split_spec(spec) == expected + + +def test_the_mock_double_never_reaches_a_provider(): + """`grapharc models --check` calls mock a "scripted test double; never + reaches a provider". Bare `mock` used to resolve to the *paid* Claude CLI + backend and spawn the real binary — the one guarantee a double exists to + make, broken in the direction that costs money. + + Asserted on the behaviour (no subprocess is created), not just on the + returned type, because the type is what was wrong and a future refactor + could get the type right and still shell out. + """ + import subprocess + + from grapharc.gateway import get_model + from grapharc.testing import ScriptedChatModel + + spawned: list = [] + real_run, real_popen = subprocess.run, subprocess.Popen + subprocess.run = lambda *a, **k: spawned.append(a[0]) or real_run(*a, **k) + subprocess.Popen = lambda *a, **k: spawned.append(a[0]) or real_popen(*a, **k) + try: + model = get_model("mock", responses=["hi"]) + assert isinstance(model, ScriptedChatModel) + assert model.invoke("anything").content == "hi" + finally: + subprocess.run, subprocess.Popen = real_run, real_popen + + assert spawned == [], f"the mock double spawned a subprocess: {spawned}" + + +@pytest.mark.parametrize("backend", ["openrouter", "openai", "ollama"]) +def test_a_bare_backend_with_no_default_model_is_refused_with_an_example(backend): + """These front catalogues, not a model. Guessing one would be guessing what + to bill someone for, so the spec is refused — and the message has to carry + a spelling that works, because "that is wrong" without "this is right" is + what sent people to `--model openrouter` in the first place. + """ + from grapharc.gateway.registry import UnknownBackendError, split_spec + + with pytest.raises(UnknownBackendError) as caught: + split_spec(backend) + message = str(caught.value) + assert "names a backend, not a model" in message + assert f"{backend}/" in message + + +def test_the_bare_claude_cli_default_matches_the_model_class(): + """`BARE_BACKEND_MODEL` holds a second copy of the model class's own default + so the registry does not import a backend just to split a string. Two + declarations of one value drift; this is the check that they have not, in + the same spirit as CI pinning `__version__` to the packaged version. + """ + from grapharc.gateway.registry import BARE_BACKEND_MODEL + + assert ( + BARE_BACKEND_MODEL["claude-cli"] + == ClaudeCodeCLIChatModel.model_fields["model"].default + ) + + +# ---- the CLI's failures explain themselves ---------------------------------- + + +def test_a_failure_reports_the_reason_the_cli_wrote_to_stdout(): + """`claude -p` fails with a non-zero exit, an *empty stderr*, and the whole + reason in its JSON envelope on stdout. Reading only stderr produced + `claude -p exited 1: ` — a sentence that stops at the colon — and threw + away the one string that said what was wrong. That is how a wrong model + spec presented itself: as no message at all. + """ + import json + + from grapharc.gateway.claude_cli import _payload_error + + # The apostrophe in "There's" is the reason this is built with `json.dumps` + # and not an f-string with quotes swapped. + reason = "There's an issue with the selected model (claude-cli)." + stdout = json.dumps({"is_error": True, "result": reason}) + + assert _payload_error(stdout) == reason + # Not the envelope -> "" so the caller falls back to stderr rather than + # inventing a reason from whatever happened to be on stdout. + assert _payload_error("boom, not json") == "" + assert _payload_error("[1, 2]") == "" + assert _payload_error('{"is_error": true}') == "" + + +def test_a_failure_with_a_stdout_reason_beats_an_empty_stderr(monkeypatch): + """End to end through `_invoke_cli`, because the bug was in which stream it + read, not in parsing: the payload reader above can be perfect and the error + still be blank if the call site keeps reaching for `proc.stderr`. + """ + import subprocess + + from grapharc.gateway.errors import GatewayError + + reason = "There's an issue with the selected model (nope)." + + class _Proc: + returncode = 1 + stdout = __import__("json").dumps({"is_error": True, "result": reason}) + stderr = "" + + monkeypatch.setattr(subprocess, "run", lambda *a, **k: _Proc()) + model = ClaudeCodeCLIChatModel(model="nope") + + with pytest.raises(GatewayError) as caught: + model._invoke_cli(model._build_argv(None), "hi", ".") + + assert reason in str(caught.value) + assert not str(caught.value).endswith(": "), "the message stopped at the colon again"