diff --git a/src/opendox/cli_model_binding.py b/src/opendox/cli_model_binding.py index 2331034d..f5c15ad9 100644 --- a/src/opendox/cli_model_binding.py +++ b/src/opendox/cli_model_binding.py @@ -50,7 +50,14 @@ def _declared_binding(args: argparse.Namespace) -> "binding_mod.ModelProviderBin id=args.id, label=args.label, provider=args.provider, credential_ref=args.credential_ref, auth_kind=args.auth_kind, approved_by=args.approved_by, endpoint=args.endpoint, - dialect=args.dialect, broker_argv=tuple(args.broker_argv)) + dialect=args.dialect, model=args.model, + broker_argv=tuple(args.broker_argv)) + + +#: What `list` prints for a binding that declares no model (#1144 box 16.2). +#: The request then names the binding's id, as every request did before the +#: field existed, and the operator reading the list should see that. +NO_MODEL_DECLARED = "(none declared: the request names this binding's id)" def cmd_model_binding_list(args: argparse.Namespace) -> int: @@ -77,6 +84,9 @@ def cmd_model_binding_list(args: argparse.Namespace) -> int: print(f" credential ref {record['credential_ref']}") print(f" endpoint {record['endpoint']}") print(f" dialect {record['dialect']}") + model = record["model"] + print(f" model " + f"{model if model is not None else NO_MODEL_DECLARED}") print(f" broker argv {record['broker_argv']}") print(f" custody {record['credential_custody']}") return 0 @@ -203,6 +213,15 @@ def _add_binding_declaration_args(parser: argparse.ArgumentParser) -> None: parser.add_argument("--dialect", required=True, choices=list(binding_mod.DIALECTS), help="the request grammar that endpoint speaks") + # THE MODEL THE PROVIDER RECEIVES (#1144 box 16.2), the route's third fact. + # OPTIONAL, and that keeps a binding declared without it meaning what it + # always meant: the request names the binding's id. `edit` replaces the + # whole binding, as it always has, so an edit that omits `--model` declares + # none. + parser.add_argument("--model", default=None, + help="the model name the provider receives in each " + "request (default: none declared, and the " + "request names this binding's id)") # A POSITIONAL, taken after a bare `--`, and that is the fix for a real # trap rather than a style choice: a broker invocation is full of # option-shaped members (`--binding`, `--ref`), and as a flag's value they diff --git a/src/opendox/doxbench_binding.py b/src/opendox/doxbench_binding.py index 3b1e2131..c76ca89a 100644 --- a/src/opendox/doxbench_binding.py +++ b/src/opendox/doxbench_binding.py @@ -31,7 +31,9 @@ would then be accountable for. So provider routing is the CONSUMER's fact, and the consumer's declared record is where a fact the consumer owns belongs. Declaring a route is not holding a transport: nothing here opens a socket, and -the module still names no provider host of its own. +the module still names no provider host of its own. #1144 box 16.2 gave the +route a third fact, `model`: the model name the provider receives. It is the +consumer's fact for the same reason. THE BROKER INVOCATION IS DECLARED, NOT WRITTEN INTO CODE — the program and its fixed leading arguments. `broker_argv` is the BASE invocation and names no @@ -133,9 +135,14 @@ #: `provider` and `approved_by` are REQUIRED flags of the declared `intake` #: (`--provider`, `--approved-by`; the second because `credential-contracts` #: holds that a grant without an approver is invalid), and `endpoint`/`dialect` -#: are the provider route the mint answer deliberately does not carry. STILL NO -#: SECRET FIELD: nine fields, and the absence of a tenth is the same point the -#: absence of a sixth was. +#: are the provider route the mint answer deliberately does not carry. +#: +#: AND FROM NINE TO TEN BY #1144 box 16.2 (plan 034 T079): `model`, the model +#: name the provider receives as the request's model. It sits with the route +#: it belongs to, after `dialect`. Before it, the request named the catalog +#: handle, which is this binding's `id`, so no provider model could be named. +#: STILL NO SECRET FIELD: ten fields, and the absence of an eleventh is the +#: same point the absence of a sixth was. BINDING_FIELDS: tuple[str, ...] = ( "id", "label", @@ -145,16 +152,25 @@ "approved_by", "endpoint", "dialect", + "model", "broker_argv", ) +#: The one field a stored record may leave out. A record without `model` was +#: declared before the field existed, and it keeps the meaning it had: the +#: request names the catalog handle, this binding's `id`. Every other field is +#: required, as it always was. +OPTIONAL_BINDING_FIELDS: tuple[str, ...] = ("model",) + #: The CLOSED placeholder vocabulary an argv template may name. Every member is #: a field of the binding itself, which is the property that matters: a template #: can only ever be filled with facts the binding already discloses, so no #: substitution can smuggle a value the record does not carry. A template naming #: anything outside this set is refused at construction rather than at #: execution — an operator finds out when they declare the binding, not when a -#: turn fails. +#: turn fails. `model` is not a member: a broker's invocation is about custody, +#: never about which model a turn asks for, and an undeclared model has no +#: value to fill a placeholder with. ARGV_PLACEHOLDERS: tuple[str, ...] = ( "binding_id", "label", "provider", "credential_ref", "auth_kind", "approved_by", "endpoint", "dialect") @@ -195,12 +211,19 @@ def _require_non_blank_str(field: str, value: object) -> str: class ModelProviderBinding: """ONE model provider, as settings hold it. - Nine fields, and the absence of a tenth is the point (see the module + Ten fields, and the absence of an eleventh is the point (see the module docstring). `broker_argv` is the DECLARED BASE invocation as a tuple of argv members — argv, never a shell string, so no operator's label and no credential reference can ever be read as shell syntax. It names the program and its fixed leading arguments and NOT the operation: the operation is a declared subcommand `doxbench_provider` appends. + + `model` (#1144 box 16.2) is the model name the provider receives as the + request's model. It is KEYWORD-ONLY and defaults to None, so every + construction written before it existed still builds the binding it built. + That binding keeps its old meaning: with no model declared, the request + names the catalog handle, which is the binding's `id`, exactly as before. + A declared model is a non-blank string. """ id: str @@ -211,12 +234,15 @@ class ModelProviderBinding: approved_by: str endpoint: str dialect: str + model: str | None = dataclasses.field(default=None, kw_only=True) broker_argv: tuple[str, ...] def __post_init__(self) -> None: for field in ("id", "label", "provider", "credential_ref", "auth_kind", "approved_by", "endpoint", "dialect"): _require_non_blank_str(field, getattr(self, field)) + if self.model is not None: + _require_non_blank_str("model", self.model) if self.auth_kind not in AUTH_KINDS: raise BindingRefused( f"auth_kind {self.auth_kind!r} is outside the closed " @@ -259,7 +285,8 @@ def __post_init__(self) -> None: def as_record(self) -> dict: """The STORED record: the record kind, then exactly ``BINDING_FIELDS`` in order. `broker_argv` becomes a list because that is what YAML round - trips; nothing else changes shape.""" + trips. An undeclared `model` is written as null, so every stored + record carries all ten keys; nothing else changes shape.""" return { "kind": BINDING_KIND, "id": self.id, @@ -270,6 +297,7 @@ def as_record(self) -> dict: "approved_by": self.approved_by, "endpoint": self.endpoint, "dialect": self.dialect, + "model": self.model, "broker_argv": list(self.broker_argv), } @@ -334,7 +362,9 @@ def from_record(cls, record: object) -> "ModelProviderBinding": raise BindingRefused( f"a binding record declares kind {declared_kind!r}, not " f"{BINDING_KIND!r}") - missing = [field for field in BINDING_FIELDS if field not in record] + missing = [field for field in BINDING_FIELDS + if field not in record + and field not in OPTIONAL_BINDING_FIELDS] if missing: raise BindingRefused( f"a binding record is missing {missing}") @@ -347,6 +377,7 @@ def from_record(cls, record: object) -> "ModelProviderBinding": approved_by=record["approved_by"], endpoint=record["endpoint"], dialect=record["dialect"], + model=record.get("model"), broker_argv=record["broker_argv"], ) diff --git a/src/opendox/doxbench_intake.py b/src/opendox/doxbench_intake.py index caaeb4a6..74fead6f 100644 --- a/src/opendox/doxbench_intake.py +++ b/src/opendox/doxbench_intake.py @@ -20,21 +20,22 @@ never sees a credential — which is why it is safe for it to be the surface a browser talks to. -WHAT IT DOES NOT WIDEN, also deliberately: the BINDING's closed nine-field -record (`doxbench_binding.BINDING_FIELDS`) and the CATALOG entry's closed public -shape (`doxbench_model.DECLARABLE_ENTRY_FIELDS`). The count is named by that -tuple rather than restated here, because the shape has grown twice by governing -release since this module was written — the routing declaration at -contract-v1.38 and the input-modality declaration at contract-v2.2 — and this -paragraph's claim is that THIS module widens nothing, which is unchanged by -either. Proposed-versus- +WHAT IT DOES NOT WIDEN, also deliberately: the BINDING's closed record +(`doxbench_binding.BINDING_FIELDS`) and the CATALOG entry's closed public +shape (`doxbench_model.DECLARABLE_ENTRY_FIELDS`). Each count is named by its +tuple rather than restated here, because both shapes have grown since this +module was written. The catalog entry's shape grew twice by governing release +(the routing declaration at contract-v1.38 and the input-modality declaration +at contract-v2.2), and the binding's record grew once, by `model` (#1144 box +16.2, plan 034 T079). This paragraph's claim is that THIS module widens +nothing, which is unchanged by any of them. Proposed-versus- approved is a SERVER-SIDE distinction and a pending declaration is simply not in the catalog, so NEITHER SHAPE GAINS A FIELD FROM THIS MODULE and this module needs no release act. (Both statements are scoped to this module deliberately. The catalog shape HAS gained fields — by the governing releases named above — and each of those was a release act; what has never happened, and is what this paragraph promises, is this module widening either shape.) The declaration is a -SECOND record beside the binding, not a tenth field on it. +SECOND record beside the binding, not a field on it. WHY PENDING-NESS IS A DECLARED FACT AND NOT A DEFAULT. A binding this document says nothing about is UNAFFECTED: it resolves exactly as it resolved before this diff --git a/src/opendox/doxbench_provider.py b/src/opendox/doxbench_provider.py index 42e9ad9e..9317a995 100644 --- a/src/opendox/doxbench_provider.py +++ b/src/opendox/doxbench_provider.py @@ -704,10 +704,14 @@ def _chat_answer(document: dict) -> str: } -def _post_to_provider(token: MintedToken, *, model_id: str, prompt: str, +def _post_to_provider(token: MintedToken, *, model: str, prompt: str, timeout: float, opener) -> str: """The ONE place a provider is contacted. Returns the assistant prose. + `model` is the model name the request carries, which the port chose (the + binding's declared `model`, or the catalog handle for a binding that + declares none). + The token travels in the request's authorization header and nowhere else; it is not in the URL (which a proxy logs), not in the body (which an error handler might echo), and not in this function's return value. @@ -731,7 +735,7 @@ def _post_to_provider(token: MintedToken, *, model_id: str, prompt: str, f"{DIALECTS}; the binding refuses it at declaration, so no turn " "can carry one") build_request, read_answer = arm - body = json.dumps(build_request(model_id, prompt)).encode("utf-8") + body = json.dumps(build_request(model, prompt)).encode("utf-8") request = urllib.request.Request( # noqa: S310 - endpoint declared on the binding by its operator, carried on the minted token token.endpoint, data=body, method="POST") request.add_header("Content-Type", "application/json") @@ -909,15 +913,21 @@ def dispatch(self, prompt_envelope: object) -> object: unrelated issuances. The expired mint's reference is read off the token this turn is holding and lives no longer than the turn; * a SECOND expiry inside the same turn raises the standard refusal. - No third call is bought.""" - model_id = getattr(prompt_envelope, "model_id", None) - if not isinstance(model_id, str) or not model_id: + No third call is bought. + + THE REQUEST'S MODEL IS THE BINDING'S DECLARED `model` (#1144 box + 16.2). A binding that declares none sends the catalog handle, which is + what every request sent before the field existed, byte for byte.""" + handle = getattr(prompt_envelope, "model_id", None) + if not isinstance(handle, str) or not handle: entries = self._declared_catalog.entries - model_id = entries[0].model_id if entries else "" + handle = entries[0].model_id if entries else "" + declared_model = self._binding.model + model = declared_model if declared_model is not None else handle prompt = bridge_mod.render_prompt_message(prompt_envelope) token = self._current_token(REASON_FIRST_MINT) try: - prose = _post_to_provider(token, model_id=model_id, prompt=prompt, + prose = _post_to_provider(token, model=model, prompt=prompt, timeout=self._timeout_seconds, opener=self._opener) except _TokenExpired: @@ -932,7 +942,7 @@ def dispatch(self, prompt_envelope: object) -> object: self._record(REASON_PAID_RETRY) try: prose = _post_to_provider( - token, model_id=model_id, prompt=prompt, + token, model=model, prompt=prompt, timeout=self._timeout_seconds, opener=self._opener) except _TokenExpired: self._forget_token() diff --git a/tests/test_model_provider_broker.py b/tests/test_model_provider_broker.py index 76cbd2ec..b22c0001 100644 --- a/tests/test_model_provider_broker.py +++ b/tests/test_model_provider_broker.py @@ -22,7 +22,8 @@ UNCONFIGURED posture is byte-for-byte what it was before this change. A SIXTH LAYER, (f), holds #1144 Group 16's binding and provider boxes (plan -034 phase 3, slice P3-B). 16.1 is the OpenAI-compatible dialect (T078). +034 phase 3, slice P3-B). 16.1 is the OpenAI-compatible dialect (T078), and +16.2 is the model name the provider receives (T079). THE FAKE BROKER SPEAKS THE DECLARED CONTRACT (task 2.6). It was this repository's own invented stdin/stdout protocol until the reconciliation, which @@ -120,8 +121,9 @@ def test_the_binding_declares_exactly_the_fields_the_seam_needs(): "secret", "api_key", "token", "credential", "value", "password"]) def test_no_secret_field_exists_in_the_shape_to_populate(secret_field): """NOT OPTIONAL — ABSENT. The dataclass is slotted and frozen, so a secret - cannot be passed in and cannot be attached afterwards. Nine fields now - rather than five, and the absence of a tenth is the same claim.""" + cannot be passed in and cannot be attached afterwards. Ten fields now + rather than five (#1144 box 16.2 added `model`), and the absence of an + eleventh is the same claim.""" with pytest.raises(TypeError): _binding(**{secret_field: SENTINEL_CREDENTIAL}) binding = _binding() @@ -874,9 +876,11 @@ def _expired_error(): def _port(tmp_path, *outcomes, expires=None, notice=None, clock=time.time, - endpoint=ENDPOINT, dialect=binding_mod.DIALECT_XFACTORY_PROMPT_V1): + endpoint=ENDPOINT, dialect=binding_mod.DIALECT_XFACTORY_PROMPT_V1, + model=None): script = _write_broker(tmp_path, expires=expires) - binding = _broker_binding(script, endpoint=endpoint, dialect=dialect) + binding = _broker_binding(script, endpoint=endpoint, dialect=dialect, + model=model) opener = _Opener(*outcomes) port = provider_mod.BrokeredProviderPort( binding, install_mod.brokered_catalog(binding), @@ -1582,3 +1586,197 @@ def test_the_cli_declares_a_chat_binding(tmp_path, capsys): capsys.readouterr() store = binding_mod.BindingStore(binding_mod.bindings_path(checkout)) assert store.get("local-chat").dialect == OPENAI_CHAT + + +# 16.2, the model name the provider receives (T079). The record gains `model`, +# sent as the request's model and set by `model-binding add|edit --model`. The +# field list grows from nine to ten, and still no field can hold a secret. A +# binding that declares no model sends the catalog handle, its `id`, exactly as +# every request did before the field existed. + +DECLARED_MODEL = "stand-in-model-7b" + + +def test_f16_1_the_record_names_a_model(): + """F16.1's field assertion, as #1144 writes it: + `assert "model" in b.BINDING_FIELDS`. Ten fields, in their declared order, + with `model` beside the route it belongs to.""" + assert "model" in binding_mod.BINDING_FIELDS, ( + f"the record names no model: {binding_mod.BINDING_FIELDS}") + assert binding_mod.BINDING_FIELDS == ( + "id", "label", "provider", "credential_ref", "auth_kind", + "approved_by", "endpoint", "dialect", "model", "broker_argv") + assert binding_mod.OPTIONAL_BINDING_FIELDS == ("model",) + + +def test_the_model_is_keyword_only_and_undeclared_by_default(): + """Every construction written before the field existed builds the + binding it built, which declares no model.""" + import inspect + parameter = inspect.signature( + binding_mod.ModelProviderBinding).parameters["model"] + assert parameter.kind is inspect.Parameter.KEYWORD_ONLY + assert parameter.default is None + assert _binding().model is None + assert _binding(model=DECLARED_MODEL).model == DECLARED_MODEL + + +@pytest.mark.parametrize("bad", ["", " ", 7, ["a-model"]]) +def test_a_declared_model_is_non_blank_text(bad): + with pytest.raises(binding_mod.BindingRefused): + _binding(model=bad) + + +@pytest.mark.parametrize("dialect,answer,grammar_key", [ + (binding_mod.DIALECT_XFACTORY_PROMPT_V1, {"assistant_prose": "a"}, "prompt"), + (OPENAI_CHAT, _chat_completion("a"), "messages"), +]) +def test_the_declared_model_is_what_the_provider_receives(tmp_path, dialect, + answer, grammar_key): + port, opener = _port(tmp_path, answer, dialect=dialect, + model=DECLARED_MODEL) + assert port.dispatch(_Envelope())["assistant_prose"] == "a" + body = json.loads(opener.requests[0].data.decode("utf-8")) + assert body["model"] == DECLARED_MODEL + assert set(body) == {"model", grammar_key} + + +@pytest.mark.parametrize("dialect,answer", [ + (binding_mod.DIALECT_XFACTORY_PROMPT_V1, {"assistant_prose": "a"}), + (OPENAI_CHAT, _chat_completion("a")), +]) +def test_a_binding_with_no_model_still_sends_the_catalog_handle(tmp_path, + dialect, + answer): + """What every request sent before #1144 box 16.2, byte for byte.""" + port, opener = _port(tmp_path, answer, dialect=dialect) + port.dispatch(_Envelope()) + assert json.loads(opener.requests[0].data.decode("utf-8"))["model"] == \ + "openprofiler-demo" + + +def test_the_catalog_handle_stays_the_bindings_id(tmp_path): + """The model is what the PROVIDER receives. The menu's handle is still the + binding's id, so a chosen entry still resolves back to its binding.""" + binding = _binding(model=DECLARED_MODEL) + entries = install_mod.brokered_catalog(binding).entries + assert [entry.model_id for entry in entries] == [binding.id] + + +def test_a_declared_model_survives_the_expiry_retry(tmp_path): + port, opener = _port(tmp_path, _expired_error(), _chat_completion("b"), + dialect=OPENAI_CHAT, model=DECLARED_MODEL) + assert port.dispatch(_Envelope())["assistant_prose"] == "b" + assert [json.loads(request.data.decode("utf-8"))["model"] + for request in opener.requests] == [DECLARED_MODEL] * 2 + + +def test_the_model_is_not_an_argv_placeholder(): + """A broker's invocation is about custody, never about the model a turn + asks for, so the closed placeholder vocabulary does not grow.""" + assert "model" not in binding_mod.ARGV_PLACEHOLDERS + with pytest.raises(binding_mod.BindingRefused): + _binding(model=DECLARED_MODEL, + broker_argv=("openprofiler-broker", "--for", "{model}")) + + +def test_a_stored_record_carries_its_model_and_round_trips(tmp_path): + store = _store(tmp_path) + store.add(_binding(model=DECLARED_MODEL)) + store.add(_binding(id="undeclared", label="No model")) + import yaml + document = yaml.safe_load(store.path.read_text(encoding="utf-8")) + first, second = document["bindings"] + assert list(first) == ["kind", *binding_mod.BINDING_FIELDS] + assert first["model"] == DECLARED_MODEL + assert second["model"] is None, "an undeclared model is written as null" + assert store.get("openprofiler-demo").model == DECLARED_MODEL + assert store.get("undeclared").model is None + assert store.read_back()["bindings"][0]["model"] == DECLARED_MODEL + + +def test_a_record_declared_before_the_field_existed_still_reads(tmp_path): + """A nine-field record, as every stored document held until #1144 box + 16.2, reads as a binding that declares no model.""" + record = _binding().as_record() + del record["model"] + assert set(record) == {"kind", *binding_mod.BINDING_FIELDS} - {"model"} + path = tmp_path / "bindings.yaml" + path.write_text(json.dumps({"schema_version": 1, + "kind": binding_mod.BINDINGS_KIND, + "bindings": [record]}), encoding="utf-8") + (binding,) = binding_mod.BindingStore(path).list() + assert binding.model is None + assert binding == _binding() + + +def test_a_record_missing_a_required_field_still_refuses(): + """`model` is the one field a record may leave out, and only that one.""" + for field in binding_mod.BINDING_FIELDS: + if field in binding_mod.OPTIONAL_BINDING_FIELDS: + continue + record = _binding().as_record() + del record[field] + with pytest.raises(binding_mod.BindingRefused) as caught: + binding_mod.ModelProviderBinding.from_record(record) + assert field in str(caught.value) + + +def test_the_cli_sets_the_model_on_add_and_edit(tmp_path, capsys): + checkout = tmp_path / "checkout" + checkout.mkdir() + parser = cli_mod.build_parser() + + def run(*argv) -> int: + args = parser.parse_args(list(argv)) + return args.func(args) + + root = ["--repo-root", str(checkout)] + declaration = ["--id", "local-chat", "--label", "Local chat", + "--provider", "local", "--credential-ref", FAKE_REFERENCE, + "--auth-kind", "api_key", + "--credential-approver", "brett@opensoft.one", + "--endpoint", "http://127.0.0.1:9/v1/chat/completions", + "--dialect", OPENAI_CHAT] + store = binding_mod.BindingStore(binding_mod.bindings_path(checkout)) + + assert run("model-binding", "add", *root, *declaration, + "--model", DECLARED_MODEL, "--", "openprofiler-broker") == 0 + capsys.readouterr() + assert store.get("local-chat").model == DECLARED_MODEL + assert run("model-binding", "list", *root) == 0 + assert f"model {DECLARED_MODEL}" in capsys.readouterr().out + + assert run("model-binding", "edit", *root, *declaration, + "--model", "another-model", "--", "openprofiler-broker") == 0 + capsys.readouterr() + assert store.get("local-chat").model == "another-model" + + # `edit` replaces the whole binding, so an edit without `--model` declares + # none, and the list says what the request then names + assert run("model-binding", "edit", *root, *declaration, + "--", "openprofiler-broker") == 0 + capsys.readouterr() + assert store.get("local-chat").model is None + assert run("model-binding", "list", *root) == 0 + from opendox import cli_model_binding as cmb + assert cmb.NO_MODEL_DECLARED in capsys.readouterr().out + + # a blank model refuses THROUGH THE VERB, not only through the record + assert run("model-binding", "edit", *root, *declaration, + "--model", " ", "--", "openprofiler-broker") == 1 + capsys.readouterr() + assert store.get("local-chat").model is None + + +def test_a_stand_in_chat_server_receives_the_declared_model(tmp_path): + with _stand_in_provider(_ChatCompletionsHandler) as base: + binding = _broker_binding( + _write_broker(tmp_path), endpoint=f"{base}/v1/chat/completions", + dialect=OPENAI_CHAT, model=DECLARED_MODEL) + provider_mod.BrokeredProviderPort( + binding, install_mod.brokered_catalog(binding), + notice=lambda _text: None).dispatch(_Envelope()) + assert _ChatCompletionsHandler.seen["body"] == { + "model": DECLARED_MODEL, + "messages": [{"role": "user", "content": "assembled prompt"}]}