T100 follow-on: the trust store's review findings (plan 034)

conftest.py

@@ -13,9 +13,12 @@
 
 from __future__ import annotations
 
+import itertools
 import sys
 from pathlib import Path
 
+import pytest
+
 SRC = Path(__file__).resolve().parent / "src"
 
 if SRC.is_dir():
@@ -102,3 +105,47 @@ class _SuiteProfile:
 
 if _domain_profile is not None and not _domain_profile.is_registered():
     _domain_profile.register(_SuiteProfile())
+
+
+# ---------------------------------------------------------------------------
+# NO CASE READS OR WRITES THIS MACHINE'S TRUST (T100 follow-on, A15).
+#
+# The per-machine trust store (`opendox.doxbench_trust.MachineTrust`) lives in
+# openDox's state directory, `OPENDOX_STATE_DIR` or, where that is unset, the
+# per-user one under the operator's home. A case that serves a console through
+# the trust-gated factory without naming a state directory of its own read
+# the OPERATOR'S store: what the case saw depended on what this machine
+# trusts, and a case that recorded trust wrote it there. So every case gets
+# a scratch state directory of its own, which does not exist until something
+# records into it (an absent store trusts nothing), and the session gets one
+# too, for what a module- or session-scoped fixture builds before any case.
+# The trust seam is emptied after every case, so no policy a case registered,
+# and no default a case's console registered over a scratch directory, is
+# the next case's.
+#
+# `tests_runtime/conftest.py` clears every `OPENDOX_*` setting before each of
+# its cases, this one included; those cases name the state directory they
+# mean. A case that names its own (`monkeypatch.setenv`) wins, as it runs
+# after this fixture.
+_STATE_SETTING = "OPENDOX_STATE_DIR"
+_scratch_cases = itertools.count()
+
+
+@pytest.fixture(scope="session", autouse=True)
+def _scratch_state_base(tmp_path_factory: pytest.TempPathFactory):
+    """The session's scratch state directory, and the base of each case's."""
+    base = tmp_path_factory.mktemp("opendox-state")
+    with pytest.MonkeyPatch.context() as patch:
+        patch.setenv(_STATE_SETTING, str(base / "session"))
+        yield base
+
+
+@pytest.fixture(autouse=True)
+def _scratch_state_directory(_scratch_state_base, monkeypatch: pytest.MonkeyPatch):
+    """A case's own scratch state directory; the trust seam emptied after."""
+    monkeypatch.setenv(_STATE_SETTING, str(
+        _scratch_state_base / f"case-{next(_scratch_cases)}"))
+    yield
+    trust = sys.modules.get("opendox.doxbench_trust")
+    if trust is not None:
+        trust.unregister()

src/opendox/doxbench_binding.py

@@ -89,7 +89,11 @@
 from __future__ import annotations
 
 import dataclasses
+import json
 import re
+import secrets
+import stat
+import tempfile
 from collections.abc import Iterable, Mapping
 from pathlib import Path
 from typing import NamedTuple
@@ -529,6 +533,155 @@
     caller declaring a binding has exactly one thing to catch."""
 
 
+def linked_component(path: Path | str, relpath: str) -> Path | None:
+    """The symbolic link on the way to a settings document, or None (T100
+    follow-on, N1): the document itself and, where its path ends with
+    `relpath` (a checkout's own default for it), every directory of `relpath`
+    above it. A clone carries a link as readily as a file, so a document
+    reached through one could be read from, or written to, anywhere the link
+    points: a write through it would create or overwrite a file outside the
+    repository. Directories above `relpath`, the checkout's own path, are
+    the operator's. A path that IS `relpath`, relative to the working
+    directory, is judged the same way (Copilot at openDox-code#86,
+    r4179076919)."""
+    path = Path(path)
+    candidates = [path]
+    parts = Path(relpath).parts
+    if len(path.parts) >= len(parts) and path.parts[-len(parts):] == parts:
+        candidates += list(path.parents)[:len(parts) - 1]
+    for candidate in candidates:
+        if candidate.is_symlink():
+            return candidate
+    return None
+
+
+#: What a store says of a settings document reached through a link. Both
+#: paths are filled in `shown_path`'s form.
+LINKED_DOCUMENT = (
+    "the {what} at {path} is reached through a symbolic link ({link}), which "
+    "a clone can carry to point anywhere, so it is neither read nor written; "
+    "replace the link with the file or directory itself")
+
+
+def shown_path(path: Path | str) -> str:
+    """A path as a refusal prints it: in a JSON string's form, as
+    `doxbench_trust.shown` prints every value a repository wrote, so a
+    newline or a terminal control sequence in a checkout's path is escaped
+    and cannot forge or hide output (Copilot at openDox-code#86,
+    r4179076973, r4179076986)."""
+    return json.dumps(str(path), ensure_ascii=True)
+
+
+def _write_all(handle, text: str) -> None:
+    """Write `text` to the open file `handle`. A seam of its own, so a case
+    can fail a write part way, as a full disk does."""
+    handle.write(text)
+
+
+def write_settings_document(path: Path, text: str) -> None:
+    """Replace the settings document at `path` with `text`, ATOMICALLY
+    (Copilot at openDox-code#86, r4179076956): written whole to a new file
+    beside it, created exclusively and never through a link, then renamed
+    over it, keeping the document's mode. A write that fails part way (a
+    full disk, an I/O error) leaves the document as it was, and the new
+    file is removed, so a refusal can say nothing in it changed. A document
+    this user cannot write is refused as it always was, by the system's own
+    error, rather than replaced.
+
+    A DOCUMENT THAT DOES NOT EXIST YET is written whole to a new file beside
+    it, created exclusively (so with the mode a new file takes here), and
+    then published with a hard link (`os.link`), which never replaces a
+    document made in the meantime (Copilot at openDox-code#86, r4180041167;
+    the holder's ruling, openxFactory#656 comment 5986391296). So no reader
+    sees part of one, a write that fails leaves none, and one made meanwhile
+    refuses this write (`FileExistsError`) rather than being overwritten."""
+    path.parent.mkdir(parents=True, exist_ok=True)
+    if not path.exists():
+        staged = path.parent / (f".{path.name}.{secrets.token_hex(8)}"
+                                ".opendox-new")
+        try:
+            with staged.open("x", encoding="utf-8") as handle:
+                _write_all(handle, text)
+            path.hardlink_to(staged)
+        finally:
+            staged.unlink(missing_ok=True)
+        return
+    with path.open("a", encoding="utf-8"):
+        pass                    # this user may write it, or PermissionError
+    mode = stat.S_IMODE(path.stat().st_mode)
+    handle = tempfile.NamedTemporaryFile(
+        "w", encoding="utf-8", dir=path.parent, prefix=f".{path.name}.",
+        suffix=".opendox-new", delete=False)
+    temporary = Path(handle.name)
+    try:
+        with handle:
+            _write_all(handle, text)
+        temporary.chmod(mode)
+        temporary.replace(path)
+    except BaseException:
+        temporary.unlink(missing_ok=True)
+        raise
+
+
+def cannot_read(path: Path, what: str, error: OSError) -> str:
+    """The refusal of a settings document the system will not read for
+    this user, by the system's own short word for why."""
+    return (f"the {what} at {shown_path(path)} cannot be read "
+            f"({error.strerror or type(error).__name__})")
+
+
+def document_present(path: Path) -> bool:
+    """Whether a settings document is there: a regular file at `path`. Only
+    "no such file", or a file where a directory belongs on the way, is its
+    absence; any other failure to look, such as a directory on the way this
+    user cannot search, raises, so the caller refuses it by name (Copilot at
+    openDox-code#86, r4179241603) on every Python, where `Path.is_file`
+    swallows some of them."""
+    try:
+        return stat.S_ISREG(path.stat().st_mode)
+    except (FileNotFoundError, NotADirectoryError):
+        return False
+
+
+#: The most broker command members one bindings document may declare
+#: across its bindings, every one of which is judged where trust is asked
+#: (`doxbench_trust.broker_command_refused`). Past it the document is
+#: refused by name, FAIL-CLOSED: it lives in the served repository, so a
+#: pull must not be able to hang verdict computation, and each command's
+#: judgment is bounded (`doxbench_trust._WORK_BUDGET`) while their number
+#: is bounded here (Copilot at openDox-code#86, r4182002696; the holder's
+#: ruling, openxFactory#656 comment 5990845570).
+MAX_JUDGED_MEMBERS = 256
+
+
+def read_settings_document(path: Path, *, what: str, yaml, refused):
+    """The YAML document at `path`, parsed, or a refusal BY NAME (`refused`,
+    the caller's own refusal class) for one that cannot be read (T100
+    follow-on, N2): one the system will not read for this user, not UTF-8,
+    nested past what the parser can descend, or not YAML. A console's start
+    reads it, so none of these may surface as a raw error there. A
+    document that parses but holds a value its constructor rejects (a
+    timestamp such as `2024-13-01`) is not readable YAML either (the
+    holder's ruling, openxFactory#656 comment 5985046107, C3)."""
+    shown = shown_path(path)
+    try:
+        return yaml.safe_load(path.read_text(encoding="utf-8"))
+    except OSError as error:
+        raise refused(cannot_read(path, what, error)) from None
+    except UnicodeDecodeError:
+        # before ValueError, of which it is a kind, so it keeps its words
+        raise refused(f"the {what} at {shown} is not UTF-8 text") from None
+    except ValueError:
+        raise refused(f"the {what} at {shown} is not readable "
+                      "YAML") from None
+    except RecursionError:
+        raise refused(f"the {what} at {shown} nests too deeply to "
+                      "read") from None
+    except yaml.YAMLError as error:
+        raise refused(f"the {what} at {shown} is not readable "
+                      "YAML") from error
+
+
 def _require_non_blank_str(field: str, value: object) -> str:
     if not isinstance(value, str):
         raise BindingRefused(
@@ -1027,19 +1180,34 @@
 
     # -- the document ------------------------------------------------------
 
+    def _refuse_a_link(self) -> None:
+        """No link on the way to the document, which a clone could carry
+        (T100 follow-on, N1; `linked_component`)."""
+        link = linked_component(self.path, DEFAULT_BINDINGS_RELPATH)
+        if link is not None:
+            raise BindingRefused(LINKED_DOCUMENT.format(
+                what="bindings document", path=shown_path(self.path),
+                link=shown_path(link)))
+
     def _load(self) -> list[ModelProviderBinding]:
-        if not self.path.is_file():
+        try:
+            # The look before the read refuses BY NAME too (Copilot at
+            # openDox-code#86, r4179241603): a directory on the way that
+            # this user cannot search fails `lstat` and `stat` themselves.
+            self._refuse_a_link()
+            present = document_present(self.path)
+        except OSError as error:
+            raise BindingRefused(cannot_read(
+                self.path, "bindings document", error)) from None
+        if not present:
             # THE HOSTED PATH, and the reason the import below is lazy: an
             # install with no bindings document answers here and never needs a
             # YAML parser at all.
             return []
         yaml = _yaml_or_refused()
-        try:
-            document = yaml.safe_load(self.path.read_text(encoding="utf-8"))
-        except yaml.YAMLError as error:
-            raise BindingRefused(
-                f"the bindings document at {self.path} is not readable YAML"
-            ) from error
+        document = read_settings_document(
+            self.path, what="bindings document", yaml=yaml,
+            refused=BindingRefused)
         if document is None:
             return []
         if not isinstance(document, Mapping):
@@ -1068,19 +1236,24 @@
                     f"the bindings document at {self.path} declares the id "
                     f"{binding.id!r} twice")
             seen.add(binding.id)
+        judged = sum(len(binding.broker_argv) for binding in bindings)
+        if judged > MAX_JUDGED_MEMBERS:
+            raise BindingRefused(
+                f"the bindings document at {self.path} declares {judged} "
+                "broker command members across its bindings, more than the "
+                f"{MAX_JUDGED_MEMBERS} judged in one document")
         return bindings
 
     def _save(self, bindings: Iterable[ModelProviderBinding]) -> None:
+        self._refuse_a_link()
         yaml = _yaml_or_refused()
         document = {
             "schema_version": SCHEMA_VERSION,
             "kind": BINDINGS_KIND,
             "bindings": [binding.as_record() for binding in bindings],
         }
-        self.path.parent.mkdir(parents=True, exist_ok=True)
-        self.path.write_text(
-            yaml.safe_dump(document, sort_keys=False, allow_unicode=True),
-            encoding="utf-8")
+        write_settings_document(self.path, yaml.safe_dump(
+            document, sort_keys=False, allow_unicode=True))
 
 
 #: What a removal does and does not do, stated once so no surface invents its

src/opendox/doxbench_install.py

@@ -338,9 +338,11 @@ def trust_gated_model_port_factory(binding, *, checkout_root: Path | str,
     the store says (Copilot at openDox-code#82, r4174783280): its id or its
     label is not one `brokered_catalog` can list, so the verdict refuses it
     before any policy is asked (`doxbench_trust.unservable_because`), and
-    the start declares the refusing port over an empty catalog rather than
-    fail on what a repository wrote. So `brokered_catalog` below is only
-    ever built for a binding it accepts.
+    this declares the refusing port over an empty catalog rather than fail
+    on what a repository wrote. So `brokered_catalog` below is only ever
+    built for a binding it accepts. The console's start never hands one
+    here: `declared_model_port_factory` passes it over (T100 follow-on, A3),
+    and this refusal is the defence beneath that, for any other caller.
 
     `bindings_path` is the document the binding was read from, where a
     caller named one, so the command the refusal prints reads that document
@@ -405,6 +407,13 @@ def declared_model_port_factory(session_root: Path | str, *,
     at a time. Choosing among several declared bindings needs a selection rule
     this change does not have and must not invent — see tasks.md 2.5.
 
+    A BINDING THE MODEL CATALOG CANNOT LIST IS PASSED OVER as a pending one
+    is (T100 follow-on, A3). It is no model at all: no turn could name it,
+    and declaring it would leave the console with an empty catalog whose
+    rail line ("No model configured") offers two remedies, a harness on PATH
+    or another binding, neither of which could then take effect. Passed
+    over, both do. It is said on stderr, by name, with its remedy.
+
     A PENDING DECLARATION IS SKIPPED (add-doxchat-model-intake task 3.1). A
     binding the intake flow wrote is DECLARED and not yet APPROVED, and "not yet
     approved" has to mean something at the one seam where availability is
@@ -434,17 +443,29 @@ def declared_model_port_factory(session_root: Path | str, *,
             f"[model-provider] the bindings document could not be read "
             f"({error}); reading it as declaring no binding\n")
         declared = ()
+    from opendox import doxbench_trust as trust_mod
+
     pending = intake_mod.pending_binding_ids(checkout_root)
+    unservable = tuple(binding for binding in declared
+                       if binding.id not in pending
+                       and trust_mod.unservable_because(binding) is not None)
+    for binding in unservable:
+        sys.stderr.write(
+            f"[model-provider] model binding {trust_mod.shown(binding.id)} "
+            f"is passed over: {trust_mod.REASON_UNSERVABLE}. "
+            f"{trust_mod.REMEDY_UNSERVABLE}\n")
     approved = tuple(binding for binding in declared
-                     if binding.id not in pending)
-    if len(approved) != len(declared):
+                     if binding.id not in pending
+                     and binding not in unservable)
+    if len(approved) + len(unservable) != len(declared):
         # SAID OUT LOUD, on the same stderr channel the unreadable-document
         # fallback uses: an operator who declared a model through the wizard and
         # then wondered why the selector still has nothing in it deserves to
         # read the reason in their own console rather than infer it.
         sys.stderr.write(
             "[model-provider] "
-            f"{len(declared) - len(approved)} declared binding(s) are pending "
+            f"{len(declared) - len(approved) - len(unservable)} declared "
+            "binding(s) are pending "
             "human approval and contribute no available model; approve them "
             "from the console's model intake flow\n")
     if not approved: