From 972a1fb160ffdf6dd42feb457d52157c3352f8dd Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Mon, 27 Jul 2026 18:42:02 -0700 Subject: [PATCH 1/4] =?UTF-8?q?feat(kernel):=20promote=20workspace=20resol?= =?UTF-8?q?ution=20=E2=80=94=20one=20rule,=20and=20only=20cwd=20walks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolve_workspace_root(explicit)` and `WORKSPACE_ENV_VAR` move beside `DB_FILENAME`, where the fact they read from the other end already lives. The server and the CLI both need the answer and import-linter forbids either importing the other, so the rule belongs above both. Precedence: an explicit path, then a non-empty `VISIONSET_WORKSPACE`, then the nearest `visionset.db` at or above the working directory, then the working directory. Only the third case walks — a stated root traded for its parent is how a credential gets minted into the wrong workspace. --- src/visionset/kernel/services/__init__.py | 4 + .../kernel/services/workspace_service.py | 67 +++++++++++++ tests/kernel/test_workspace_service.py | 99 ++++++++++++++++++- 3 files changed, 169 insertions(+), 1 deletion(-) diff --git a/src/visionset/kernel/services/__init__.py b/src/visionset/kernel/services/__init__.py index 48f37d38..21b12e9f 100644 --- a/src/visionset/kernel/services/__init__.py +++ b/src/visionset/kernel/services/__init__.py @@ -20,12 +20,15 @@ from visionset.kernel.services.workspace_service import ( BLOBS_DIRNAME, DB_FILENAME, + WORKSPACE_ENV_VAR, WorkspaceService, + resolve_workspace_root, ) __all__ = [ "BLOBS_DIRNAME", "DB_FILENAME", + "WORKSPACE_ENV_VAR", "AnnotationService", "BatchService", "DatasetService", @@ -37,4 +40,5 @@ "SourceService", "TokenService", "WorkspaceService", + "resolve_workspace_root", ] diff --git a/src/visionset/kernel/services/workspace_service.py b/src/visionset/kernel/services/workspace_service.py index 6e91732b..b430b72f 100644 --- a/src/visionset/kernel/services/workspace_service.py +++ b/src/visionset/kernel/services/workspace_service.py @@ -50,6 +50,7 @@ from __future__ import annotations +import os import shutil from collections.abc import Callable from contextlib import AbstractContextManager @@ -97,6 +98,13 @@ #: Root of the content-addressed blob store, relative to the workspace directory. BLOBS_DIRNAME = "blobs" +#: Which workspace a surface operates on when its command line says nothing. +#: +#: Deliberately not a server- or CLI-specific name: the CLI writes the tokens the +#: server reads, so the two have to agree on one spelling of "the workspace", and +#: ``docker/compose.yaml`` already sets this one. +WORKSPACE_ENV_VAR = "VISIONSET_WORKSPACE" + #: How many entries a "this directory is not empty" message names. _PREVIEW = 3 @@ -131,6 +139,65 @@ def _resolved(path: Path | str) -> Path: return Path(path).expanduser().resolve() +def resolve_workspace_root(explicit: Path | str | None = None) -> Path: + """Which workspace a surface was pointed at. Never checks that one is there. + + One rule, shared by every surface. It lives beside ``DB_FILENAME`` because it + is the same fact read from the other end — the database file is what marks a + directory as a workspace, and this is what goes looking for the mark. It + cannot live in either caller: import-linter forbids ``visionset.server`` + importing ``visionset.cli``, so the resolver the two share belongs above both. + + Precedence, first match wins: + + 1. ``explicit`` — the CLI's ``--workspace``. Somebody named a directory. + 2. ``VISIONSET_WORKSPACE``, when it is set to something non-empty. An unset + variable and one set to ``""`` are the same thing to a shell, so both fall + through rather than resolving to the filesystem root's idea of ``Path("")``. + 3. The working directory, or the nearest directory **above** it holding a + ``visionset.db`` — so ``cd assets/raw && visionset token list`` works. + 4. The working directory, when the walk finds nothing. + + **Only case 3 walks, and that asymmetry is the whole rule.** A flag and an + environment variable are somebody *stating* which workspace; if the stated + directory holds none, walking to its parent and quietly minting a credential + into whatever workspace lives up there is the worst thing this function could + do. Git draws the line in the same place — discovery walks up, ``--git-dir`` + and ``GIT_DIR`` do not — and for the same reason. + + **Finding nothing is not an error here.** This names a directory; + :meth:`WorkspaceService.open` owns "is this a workspace?" and already raises + ``NotAWorkspace`` naming the path it was given. A refusal here would be two + errors for one condition, and would make this function non-total for the + server, which calls it inside a lazily opened handle that expects exactly one + failure mode. + + **Nothing is normalized.** :func:`_resolved` is the one place a path becomes + canonical, and it runs inside ``init``/``open``; expanding ``~`` twice is how + two spellings of one workspace start looking like two workspaces. + """ + if explicit is not None: + return Path(explicit) + named = os.environ.get(WORKSPACE_ENV_VAR) + if named: + return Path(named) + cwd = Path.cwd() + return _workspace_above(cwd) or cwd + + +def _workspace_above(start: Path) -> Path | None: + """The nearest directory at or above ``start`` holding a ``DB_FILENAME``. + + Walks to the filesystem root. Private because + :func:`resolve_workspace_root` is its only caller and "how far does discovery + reach?" is that function's rule to state, not a second public knob. + """ + for candidate in (start, *start.parents): + if (candidate / DB_FILENAME).is_file(): + return candidate + return None + + class WorkspaceService: """One open workspace: its identity, its directory, and its six ports. diff --git a/tests/kernel/test_workspace_service.py b/tests/kernel/test_workspace_service.py index d6f508f5..d00c1387 100644 --- a/tests/kernel/test_workspace_service.py +++ b/tests/kernel/test_workspace_service.py @@ -20,7 +20,13 @@ from visionset.kernel.adapters.migrations import FORMAT_VERSION from visionset.kernel.domain import Project, Workspace from visionset.kernel.ports import BlobStore, MetadataStore, UnitOfWork -from visionset.kernel.services import BLOBS_DIRNAME, DB_FILENAME, WorkspaceService +from visionset.kernel.services import ( + BLOBS_DIRNAME, + DB_FILENAME, + WORKSPACE_ENV_VAR, + WorkspaceService, + resolve_workspace_root, +) def _init(tmp_path: Path, name: str = "ws") -> WorkspaceService: @@ -556,3 +562,94 @@ def test_projects_are_listed_in_the_order_they_were_created(tmp_path: Path) -> N _add_project(workspace, name) assert [p.name for p in _projects_of(workspace)] == ["first", "second", "third"] workspace.close() + + +# --- which workspace, when nobody said ---------------------------------------- +# +# These tests assume no ancestor of ``tmp_path`` holds a ``visionset.db``. That +# holds under pytest's temporary root; if one of them ever fails on a machine +# where somebody made a workspace of ``/tmp``, this is the reason. + + +@pytest.fixture(autouse=True) +def _no_ambient_workspace(monkeypatch: pytest.MonkeyPatch) -> None: + """A developer with ``VISIONSET_WORKSPACE`` exported gets CI's results.""" + monkeypatch.delenv(WORKSPACE_ENV_VAR, raising=False) + + +def test_an_explicit_path_is_returned_unchanged(tmp_path: Path) -> None: + """Not resolved, not expanded: ``init``/``open`` own canonicalization.""" + assert resolve_workspace_root(tmp_path / "nowhere") == tmp_path / "nowhere" + + +def test_an_explicit_path_wins_over_the_environment_variable( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(tmp_path / "from-the-environment")) + assert resolve_workspace_root(tmp_path / "from-the-flag") == tmp_path / "from-the-flag" + + +def test_the_environment_variable_is_used_when_no_path_is_given( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(tmp_path / "elsewhere")) + assert resolve_workspace_root() == tmp_path / "elsewhere" + + +def test_an_empty_environment_variable_is_treated_as_unset( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A shell cannot tell ``VISIONSET_WORKSPACE=`` from an unset variable.""" + monkeypatch.setenv(WORKSPACE_ENV_VAR, "") + monkeypatch.chdir(tmp_path) + assert resolve_workspace_root() == Path.cwd() + + +def test_a_workspace_in_a_parent_directory_is_found_from_a_child( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + _init(tmp_path).close() + below = tmp_path / "ws" / "assets" / "raw" + below.mkdir(parents=True) + monkeypatch.chdir(below) + assert resolve_workspace_root() == Path.cwd().parents[1] + + +def test_the_nearest_workspace_wins_when_two_are_nested( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + outer = WorkspaceService.init(tmp_path / "outer") + outer.close() + inner = WorkspaceService.init(tmp_path / "outer" / "inner") + inner.close() + monkeypatch.chdir(tmp_path / "outer" / "inner") + assert resolve_workspace_root() == Path.cwd() + + +def test_the_working_directory_is_the_answer_when_nothing_above_it_is_a_workspace( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(tmp_path) + assert resolve_workspace_root() == Path.cwd() + + +def test_an_explicit_path_does_not_walk_upward(tmp_path: Path) -> None: + """The load-bearing negative: a stated directory is never traded for its parent. + + Walking here would mint a credential into whatever workspace happens to live + above the directory somebody actually named. + """ + _init(tmp_path).close() + below = tmp_path / "ws" / "assets" + below.mkdir() + assert resolve_workspace_root(below) == below + + +def test_the_environment_variable_does_not_walk_upward( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + _init(tmp_path).close() + below = tmp_path / "ws" / "assets" + below.mkdir() + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(below)) + assert resolve_workspace_root() == below From e2822f2defeb17b0bef82f79b35437d039ca4f22 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Mon, 27 Jul 2026 18:42:02 -0700 Subject: [PATCH 2/4] refactor(server): read workspace resolution from the kernel The provisional resolver is gone; both names stay importable from `server.dependencies`, which is where the server's "which workspace do I serve?" question is documented. One deliberate behaviour change: with no `VISIONSET_WORKSPACE` set, a server started below a workspace now serves it instead of answering 500 NOT_A_WORKSPACE. --- src/visionset/server/dependencies.py | 66 +++++++++++------------ tests/server/test_workspace_dependency.py | 25 ++++++++- 2 files changed, 57 insertions(+), 34 deletions(-) diff --git a/src/visionset/server/dependencies.py b/src/visionset/server/dependencies.py index 12bf7e00..77cbad46 100644 --- a/src/visionset/server/dependencies.py +++ b/src/visionset/server/dependencies.py @@ -19,10 +19,8 @@ from __future__ import annotations -import os import threading from collections.abc import Callable, Sequence -from pathlib import Path from typing import Annotated, Final from fastapi import APIRouter, Depends, HTTPException, status @@ -30,15 +28,32 @@ from starlette.requests import Request from visionset.kernel.ports import AuthProvider -from visionset.kernel.services import WorkspaceService +from visionset.kernel.services import ( + WORKSPACE_ENV_VAR as WORKSPACE_ENV_VAR, +) +from visionset.kernel.services import ( + WorkspaceService, +) +from visionset.kernel.services import ( + resolve_workspace_root as resolve_workspace_root, +) from visionset.server.errors import ERROR_RESPONSES -WORKSPACE_ENV_VAR: Final = "VISIONSET_WORKSPACE" -"""Which workspace this server serves. - -Not a server-specific name on purpose: the CLI writes the tokens the server -reads, so the two have to agree on one spelling of "the workspace". -""" +# ``WORKSPACE_ENV_VAR`` and ``resolve_workspace_root`` are re-exported above +# rather than defined here — the redundant ``as`` aliases are the explicit +# re-export form, and are what keeps the unreferenced constant off ruff's F401. +# +# #26 promoted the rule into the kernel, beside ``DB_FILENAME``: import-linter +# forbids ``visionset.server`` importing ``visionset.cli``, so the one resolver +# the server and the CLI share can live in neither of them. Both names stay +# importable from this module because this is where the server's "which +# workspace do I serve?" question is documented, and a reader of the server +# should not have to know the answer moved. +# +# The rule the server inherited along with the promotion: with no +# ``VISIONSET_WORKSPACE`` set, a server started *below* a workspace now serves +# that workspace instead of answering 500 ``NOT_A_WORKSPACE``. See +# ``docs/workspaces.md`` for the precedence and for why only that case walks. bearer_scheme: Final = HTTPBearer( auto_error=False, @@ -61,31 +76,14 @@ """ -def resolve_workspace_root() -> Path: - """The workspace root this server was pointed at. - - **Provisional. Issue #26 owns the real rule** — the ``--workspace`` flag, the - environment variable and cwd detection, with a documented precedence that - ``visionset ui`` and the flow commands all reuse. This is the smallest thing - #26 can promote: it already reads the variable #26 will keep, so #26 replaces - the *body* of this function and nothing importing it changes. - - Deliberately missing until then: the flag, because a server started by import - string has no argv of its own; and the upward walk for a ``visionset.db`` in a - parent directory. - - A constraint #26 should not discover late: import-linter forbids - ``visionset.server`` importing ``visionset.cli``, so the promoted resolver - cannot live in the CLI. It belongs beside ``DB_FILENAME`` in - ``kernel/services/workspace_service.py``, which already owns the rule that the - database file is what marks a directory as a workspace. +def _open_configured_workspace() -> WorkspaceService: + """The workspace this server was pointed at, resolved once and opened. - Read once per open rather than per request: the workspace is opened once. + A server started by import string has no argv of its own, so it never passes + an explicit path: the environment variable and the upward walk are the two + branches it can reach. Read once per open rather than per request — the + workspace is opened once. """ - return Path(os.environ.get(WORKSPACE_ENV_VAR) or Path.cwd()) - - -def _open_configured_workspace() -> WorkspaceService: return WorkspaceService.open(resolve_workspace_root()) @@ -119,7 +117,9 @@ def get(self) -> WorkspaceService: """The open workspace, opening it on the first call. Raises: - NotAWorkspace: ``VISIONSET_WORKSPACE`` does not name a workspace. + NotAWorkspace: nothing resolved to a workspace — neither + ``VISIONSET_WORKSPACE`` nor the walk up from the working + directory found one. WorkspaceCorrupt: it names one that cannot be read. WorkspaceFormatTooNew: it was written by a later VisionSet. """ diff --git a/tests/server/test_workspace_dependency.py b/tests/server/test_workspace_dependency.py index 33ee0838..8bac43a3 100644 --- a/tests/server/test_workspace_dependency.py +++ b/tests/server/test_workspace_dependency.py @@ -120,7 +120,13 @@ def test_closing_twice_is_safe(workspace_root: Path) -> None: handle.close() -# --- resolution (provisional; #26 owns the real rule) ------------------------ +# --- resolution: the rule lives in the kernel now, re-exported here ---------- +# +# These stay in the server's test file rather than moving to the kernel's with +# the rule. They pin two things at once now: that resolution still answers what +# the server needs, and that both names are still importable from +# ``visionset.server.dependencies`` — which is the half a refactor would break +# silently. The rule's own coverage lives in ``tests/kernel/test_workspace_service.py``. def test_the_environment_variable_names_the_workspace_root( @@ -147,6 +153,23 @@ def test_an_empty_environment_variable_falls_back_to_the_working_directory( assert resolve_workspace_root() == tmp_path +def test_the_server_finds_a_workspace_above_its_working_directory( + monkeypatch: pytest.MonkeyPatch, workspace_root: Path +) -> None: + """The one behaviour the promotion gave the server, and it is deliberate. + + Before #26 this answered 500 ``NOT_A_WORKSPACE``. One resolver means the + server discovers a workspace the same way the CLI does; the asymmetry that + keeps it safe is that a *stated* root — the variable here, ``--workspace`` + there — never walks. + """ + monkeypatch.delenv(WORKSPACE_ENV_VAR, raising=False) + below = workspace_root / "logs" + below.mkdir() + monkeypatch.chdir(below) + assert resolve_workspace_root() == Path.cwd().parent + + def test_the_configured_workspace_is_the_one_that_is_served( monkeypatch: pytest.MonkeyPatch, workspace_root: Path ) -> None: From dc02f0cb884efd0e65beb2fd289ffb1152c42df2 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Mon, 27 Jul 2026 18:45:27 -0700 Subject: [PATCH 3/4] feat(cli): visionset token create/list/revoke, against a real workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three commands over `TokenService`, each resolving a workspace through the kernel's rule and calling exactly one service method. `--workspace`/`-w` is declared per command, not on the root callback: a Click group stops parsing at the first non-option token, so a callback option would reject `visionset token create --name ci --workspace X` — the invocation everybody types. Stdout is data, stderr is prose. `create` puts the secret alone on stdout so `TOKEN=$(visionset token create --name ci)` is exactly the secret, and the shown-once warning on stderr so it survives that redirection. `list` names its three columns one at a time, so neither a secret nor a digest can reach it. Exit codes: 0 success, 1 for any VisionSetError as one sentence on stderr, 2 for Click's own usage errors. --- src/visionset/cli/_errors.py | 83 +++++++++ src/visionset/cli/_workspace.py | 76 ++++++++ src/visionset/cli/main.py | 22 +-- src/visionset/cli/tokens.py | 129 ++++++++++++++ tests/cli/test_tokens.py | 267 +++++++++++++++++++++++++++++ tests/cli/test_workspace_option.py | 151 ++++++++++++++++ 6 files changed, 707 insertions(+), 21 deletions(-) create mode 100644 src/visionset/cli/_errors.py create mode 100644 src/visionset/cli/_workspace.py create mode 100644 src/visionset/cli/tokens.py create mode 100644 tests/cli/test_tokens.py create mode 100644 tests/cli/test_workspace_option.py diff --git a/src/visionset/cli/_errors.py b/src/visionset/cli/_errors.py new file mode 100644 index 00000000..ec12f82e --- /dev/null +++ b/src/visionset/cli/_errors.py @@ -0,0 +1,83 @@ +# usage: from visionset.cli._errors import domain_errors +"""What a kernel refusal looks like at a terminal: one sentence, and an exit code. + +The CLI's half of the contract ``server/errors.py`` keeps over HTTP, and +deliberately much smaller. A REST client branches on a machine-readable ``code`` +because it is a program; a shell branches on zero versus non-zero, and a person +reads the sentence. So there is **one** non-zero code for the whole +``VisionSetError`` family rather than a table mapping each error to its own — a +second public contract to keep in sync with ``ERROR_RULES`` for a distinction no +caller makes. If a command ever needs "not found" told apart from "refused" by +exit status, the table goes here, beside the constant. + +Three exit codes, and no others: + +===== ========================================================================= + 0 the command did what it said + 1 a ``VisionSetError`` — one sentence on stderr, no traceback + 2 a usage error, raised and formatted by Click itself +===== ========================================================================= + +**Nothing here prints to stdout.** Stdout is the command's *output* — the secret +from ``token create``, the rows from ``token list`` — so that redirecting it +captures data and nothing else. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from contextlib import contextmanager +from typing import Final + +import typer + +from visionset.kernel import NotAWorkspace, VisionSetError +from visionset.kernel.services import WORKSPACE_ENV_VAR + +EXIT_DOMAIN_ERROR: Final = 1 +"""Every ``VisionSetError``. See the module docstring for why it is not a table.""" + +_HINTS: Final[dict[type[BaseException], str]] = { + # The kernel's sentence ends in "use WorkspaceService.init to create one", + # which is a Python API a person at a terminal has no way to call. Rewriting + # it in the kernel would bend a domain message toward one surface; a second + # line here is the surface adding its own remedy, which is what a surface is + # for. + NotAWorkspace: f"Point at one with --workspace, or set {WORKSPACE_ENV_VAR}.", +} +"""A remedy a *terminal* can act on, printed under the error's own sentence. + +Walked by MRO, like ``server/errors.py`` walks ``ERROR_RULES``, so a subclass +inherits its nearest ancestor's hint. Sparse on purpose: most kernel messages +already carry their own remedy, and a hint that restates the message is noise. +""" + + +def _hint_for(exc: VisionSetError) -> str | None: + for cls in type(exc).__mro__: + hint = _HINTS.get(cls) + if hint is not None: + return hint + return None + + +@contextmanager +def domain_errors() -> Iterator[None]: + """Turn any kernel refusal into a readable line and a non-zero exit. + + ``typer.Exit`` rather than ``sys.exit``: Click catches it and ``CliRunner`` + records it as ``result.exit_code``, which is what makes the exit status + assertable from a test instead of only from a subprocess. + + **Only ``VisionSetError`` is caught.** An ``OSError``, a ``KeyboardInterrupt`` + or a bug is not a refusal the CLI understands, and folding one into + ``Error: [Errno 2] ...`` would hide the traceback that identifies it. + """ + try: + yield + except VisionSetError as exc: + typer.secho(f"Error: {exc}", err=True, fg=typer.colors.RED) + hint = _hint_for(exc) + if hint is not None: + typer.echo(hint, err=True) + raise typer.Exit(code=EXIT_DOMAIN_ERROR) from exc diff --git a/src/visionset/cli/_workspace.py b/src/visionset/cli/_workspace.py new file mode 100644 index 00000000..0eb234c2 --- /dev/null +++ b/src/visionset/cli/_workspace.py @@ -0,0 +1,76 @@ +# usage: from visionset.cli._workspace import WorkspaceOption, opened_workspace +"""Which workspace a command operates on, and its lifetime inside that command. + +The **rule** lives in the kernel (``resolve_workspace_root``); this module is the +CLI's half of it — the flag that feeds the rule, and the ``with`` block that opens +and closes what it names. The rule cannot live here: import-linter forbids +``visionset.server`` importing ``visionset.cli``, and the server needs the same +answer. + +``--workspace`` is declared **per command** rather than on the root callback, and +that is a Click fact rather than a preference. A group's parser stops at the first +non-option token, so an option on ``@app.callback()`` must *precede* the +subcommand: ``visionset --workspace X token create --name ci`` would work and +``visionset token create --name ci --workspace X`` would fail with "No such +option". Nobody types the first one. The alias below is how one flag is declared +once and still belongs to every command that needs it. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from contextlib import contextmanager +from pathlib import Path +from typing import Annotated + +import typer + +from visionset.cli._errors import domain_errors +from visionset.kernel.services import WORKSPACE_ENV_VAR, WorkspaceService, resolve_workspace_root + +WorkspaceOption = Annotated[ + Path | None, + typer.Option( + "--workspace", + "-w", + help=( + f"The workspace to operate on. Defaults to ${WORKSPACE_ENV_VAR}, then to " + "the nearest workspace at or above the working directory." + ), + # No ``envvar=``. Click would read the variable itself and hand the + # command a path indistinguishable from this flag — and the flag and the + # variable are deliberately not the same case: neither walks upward, but + # only one of them is what the resolver's precedence calls "explicit". + # One function owns precedence; this is only how a path gets typed. + # + # No ``exists=True`` either. A Click path check exits 2 in Click's + # wording, where the useful answer is ``NotAWorkspace``'s sentence and + # its hint at exit 1. + ), +] +"""``--workspace`` / ``-w``, for a command that needs an open workspace. + +Module-level so that ``get_type_hints`` resolves it in the importing module's +globals under ``from __future__ import annotations``; an alias built inside a +function body would not resolve. +""" + + +@contextmanager +def opened_workspace(explicit: Path | None = None) -> Iterator[WorkspaceService]: + """The workspace this command was pointed at, open for the length of the body. + + Closes on the way out, including when the body raised. A process that exits + without checkpointing leaves ``visionset.db-wal`` beside the workspace for + the next reader to recover, and a CLI runs often enough for that to matter. + + Domain errors from **both** the open and the body become one line on stderr + and exit 1. ``close`` runs first, so the message is printed against a + workspace that is already released. + """ + with domain_errors(): + workspace = WorkspaceService.open(resolve_workspace_root(explicit)) + try: + yield workspace + finally: + workspace.close() diff --git a/src/visionset/cli/main.py b/src/visionset/cli/main.py index 94b0d43c..5651abcb 100644 --- a/src/visionset/cli/main.py +++ b/src/visionset/cli/main.py @@ -7,13 +7,13 @@ import typer from visionset import __version__ +from visionset.cli.tokens import token_app app = typer.Typer( name="visionset", help="Robomous VisionSet — local-first dataset creation for computer vision.", no_args_is_help=True, ) -token_app = typer.Typer(help="Manage API tokens.", no_args_is_help=True) app.add_typer(token_app, name="token") @@ -48,23 +48,3 @@ def ui() -> None: def mcp() -> None: """Start the MCP server on stdio (stub).""" typer.echo("server would start here") - - -@token_app.command("create") -def token_create( - name: Annotated[str, typer.Option("--name", help="Human-readable token name.")], -) -> None: - """Issue an API token (stub — see issue #26). - - Persistence landed with the kernel's ``TokenService``; wiring this command to - it needs workspace resolution, which issue #26 owns along with ``token list`` - and ``token revoke``. - - Until then this refuses rather than printing something. It used to echo a - plausible ``vst_...`` string that was never stored — harmless while nothing - could authenticate, and actively misleading now that real tokens exist and - that one would not be among them. - """ - typer.echo(f"Cannot issue token {name!r} yet: the CLI has no workspace to write it to.") - typer.echo("Token issuance lands in issue #26 (visionset token create/list/revoke).") - raise typer.Exit(code=1) diff --git a/src/visionset/cli/tokens.py b/src/visionset/cli/tokens.py new file mode 100644 index 00000000..47023b9c --- /dev/null +++ b/src/visionset/cli/tokens.py @@ -0,0 +1,129 @@ +# usage: from visionset.cli.tokens import token_app +"""``visionset token`` — create, list and revoke a workspace's API tokens. + +Thin in the sense the architecture means it: each command resolves a workspace, +calls exactly one ``TokenService`` method, and prints. The rules — names unique +per workspace, the secret shown once, revocation one-way and idempotent — are the +kernel's, and not one of them is restated here. + +**Stdout is data; stderr is everything a person reads.** ``token create`` puts the +secret on stdout as the only thing on it, so ``TOKEN=$(visionset token create +--name ci)`` is exactly the secret, and puts the shown-once warning on stderr so +the warning survives the redirection that most needs it. ``token list`` does the +same with its rows. + +**Neither the secret nor its digest is ever printed by ``list``.** The columns are +named one at a time rather than dumped off the model, so a field added to +``Token`` cannot appear here by accident. A digest is not a secret, but it +verifies a guess offline, and a listing that prints one teaches a habit that ends +badly. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from typing import Annotated, Final + +import typer + +from visionset.cli._workspace import WorkspaceOption, opened_workspace +from visionset.kernel.services import TokenService + +token_app = typer.Typer(help="Manage API tokens.", no_args_is_help=True) + +_COLUMNS: Final = ("NAME", "CREATED", "REVOKED") + +_TIMESTAMP_FORMAT: Final = "%Y-%m-%dT%H:%M:%SZ" +"""Seconds, UTC, no offset. A listing is read by a person; microseconds are not.""" + +_NEVER: Final = "-" +"""What a live token shows in the REVOKED column.""" + + +def _moment(when: datetime | None) -> str: + return _NEVER if when is None else when.astimezone(UTC).strftime(_TIMESTAMP_FORMAT) + + +def _row(cells: tuple[str, ...], widths: list[int]) -> str: + return " ".join(cell.ljust(w) for cell, w in zip(cells, widths, strict=True)).rstrip() + + +@token_app.command("create") +def token_create( + name: Annotated[str, typer.Option("--name", help="Human-readable token name.")], + workspace: WorkspaceOption = None, +) -> None: + """Issue an API token and print its secret — once.""" + with opened_workspace(workspace) as service: + issued = TokenService(service).create(name) + root = service.root + typer.echo(f"Created token {issued.token.name!r} in {root}.", err=True) + typer.echo(issued.secret) + typer.secho( + "This secret is shown once and cannot be recovered. Store it now.", + err=True, + fg=typer.colors.YELLOW, + ) + + +@token_app.command("revoke") +def token_revoke( + name: Annotated[str, typer.Argument(help="The token to burn.")], + workspace: WorkspaceOption = None, + yes: Annotated[bool, typer.Option("--yes", "-y", help="Do not ask.")] = False, +) -> None: + """Burn a token. Every client holding its secret stops working. + + Resolved by name in two calls rather than by a ``revoke_by_name`` the service + does not have: the intermediate read is what lets this print the name it + actually matched — token names are unique case-**insensitively** — and + short-circuit one that is already dead. The window between the two calls is + harmless, because there is no rename and a concurrent revoke makes the second + call a no-op. + """ + with opened_workspace(workspace) as service: + tokens = TokenService(service) + token = tokens.get_by_name(name) + if token.revoked: + # The kernel's no-op, surfaced. Exit 0, and do not ask: a retried + # ``token revoke ci`` must be safe, and prompting to redo something + # already done invites a "yes" that means nothing. + typer.echo( + f"Token {token.name!r} was already revoked at {_moment(token.revoked_at)}.", + err=True, + ) + return + if not yes: + # ``ConfirmationRequired`` exists because the kernel has no terminal; + # this is the CLI's idiom for asking, and ``confirm=True`` below only + # reports that some surface did. ``abort=True`` exits non-zero on "no" + # *and* on EOF — a destructive command that cannot ask must not act. + typer.confirm( + f"Revoke token {token.name!r}? Every client holding its secret stops " + "working, and this cannot be undone.", + abort=True, + ) + tokens.revoke(token.id, confirm=True) + burned = token.name + typer.echo(f"Revoked token {burned!r}.", err=True) + + +@token_app.command("list") +def token_list(workspace: WorkspaceOption = None) -> None: + """List this workspace's tokens, revoked ones included. Never their secrets.""" + with opened_workspace(workspace) as service: + rows = [ + (token.name, _moment(token.created_at), _moment(token.revoked_at)) + for token in TokenService(service).list() + ] + root = service.root + widths = [ + max([len(header), *(len(row[i]) for row in rows)]) for i, header in enumerate(_COLUMNS) + ] + # The header prints whether or not there are rows, so ``| tail -n +2`` is + # stable; the "none" note goes to stderr, where notes go. + typer.echo(_row(_COLUMNS, widths)) + for row in rows: + typer.echo(_row(row, widths)) + if not rows: + typer.echo(f"No tokens in {root}.", err=True) diff --git a/tests/cli/test_tokens.py b/tests/cli/test_tokens.py new file mode 100644 index 00000000..92026b47 --- /dev/null +++ b/tests/cli/test_tokens.py @@ -0,0 +1,267 @@ +"""``visionset token create/list/revoke``, including what it must never print. + +Two of these carry the issue's acceptance criteria and are marked where they sit: +a token minted here authenticates against a server built by the real +``create_app()``, and a listing never shows a secret or a digest. +""" + +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient +from tests.server._probe import PROBE_PATH, workspace_app +from typer.testing import CliRunner + +from visionset.cli.main import app +from visionset.kernel.domain import SECRET_PREFIX, Token +from visionset.kernel.services import WORKSPACE_ENV_VAR, TokenService, WorkspaceService + +runner = CliRunner() + + +@pytest.fixture(autouse=True) +def _no_ambient_workspace(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv(WORKSPACE_ENV_VAR, raising=False) + + +@pytest.fixture() +def workspace_root(tmp_path: Path) -> Path: + root = tmp_path / "ws" + WorkspaceService.init(root).close() + return root + + +def _stored(root: Path) -> list[Token]: + workspace = WorkspaceService.open(root) + tokens = TokenService(workspace).list() + workspace.close() + return tokens + + +def _create(root: Path, name: str) -> str: + """Mint through the CLI and return the secret it printed.""" + result = runner.invoke(app, ["token", "create", "--name", name, "-w", str(root)]) + assert result.exit_code == 0, result.output + return result.stdout.strip() + + +def _rows(output: str) -> list[list[str]]: + return [line.split() for line in output.splitlines()] + + +# --- create ------------------------------------------------------------------- + + +def test_create_prints_the_secret_alone_on_stdout(workspace_root: Path) -> None: + """``TOKEN=$(visionset token create --name ci)`` has to be exactly the secret.""" + result = runner.invoke(app, ["token", "create", "--name", "ci", "-w", str(workspace_root)]) + + assert result.exit_code == 0, result.output + assert result.stdout.strip().startswith(SECRET_PREFIX) + assert len(result.stdout.strip().splitlines()) == 1 + + +def test_create_warns_on_stderr_that_the_secret_is_shown_once(workspace_root: Path) -> None: + """On stderr so the warning survives the redirection that most needs it.""" + result = runner.invoke(app, ["token", "create", "--name", "ci", "-w", str(workspace_root)]) + + assert "shown once" in result.stderr + assert "ci" in result.stderr + + +def test_create_stores_a_token_the_workspace_can_list(workspace_root: Path) -> None: + secret = _create(workspace_root, "ci") + + stored = _stored(workspace_root) + assert [token.name for token in stored] == ["ci"] + assert secret not in stored[0].secret_hash + + +def test_a_token_created_by_the_cli_authenticates_against_a_running_server( + workspace_root: Path, +) -> None: + """The acceptance criterion: the CLI writes what the server reads.""" + secret = _create(workspace_root, "ci") + + with TestClient(workspace_app(workspace_root)) as client: + response = client.get(PROBE_PATH, headers={"Authorization": f"Bearer {secret}"}) + + assert response.status_code == 200 + + +def test_creating_a_second_token_with_the_same_name_is_refused_with_a_readable_message( + workspace_root: Path, +) -> None: + _create(workspace_root, "ci") + + result = runner.invoke(app, ["token", "create", "--name", "CI", "-w", str(workspace_root)]) + + assert result.exit_code == 1 + assert "already exists" in result.stderr + assert result.stdout == "" + assert len(_stored(workspace_root)) == 1 + + +def test_creating_a_token_with_a_blank_name_is_refused(workspace_root: Path) -> None: + result = runner.invoke(app, ["token", "create", "--name", " ", "-w", str(workspace_root)]) + + assert result.exit_code == 1 + assert result.stdout == "" + assert _stored(workspace_root) == [] + + +def test_create_without_a_name_is_a_usage_error(workspace_root: Path) -> None: + """Click's refusal, at Click's exit code — not a domain error dressed as one.""" + result = runner.invoke(app, ["token", "create", "-w", str(workspace_root)]) + + assert result.exit_code == 2 + + +# --- list --------------------------------------------------------------------- + + +def test_list_shows_every_token_with_its_created_and_revoked_columns( + workspace_root: Path, +) -> None: + _create(workspace_root, "ci") + _create(workspace_root, "laptop") + + result = runner.invoke(app, ["token", "list", "-w", str(workspace_root)]) + + assert result.exit_code == 0, result.output + rows = _rows(result.stdout) + assert rows[0] == ["NAME", "CREATED", "REVOKED"] + assert [row[0] for row in rows[1:]] == ["ci", "laptop"] + + +def test_list_never_prints_a_secret_or_its_hash(workspace_root: Path) -> None: + """The acceptance criterion, asserted against both streams. + + The digest is not the secret, but it verifies a guess offline — a listing + that prints one teaches a habit that ends badly. + """ + secrets = [_create(workspace_root, name) for name in ("ci", "laptop")] + digests = [token.secret_hash for token in _stored(workspace_root)] + + result = runner.invoke(app, ["token", "list", "-w", str(workspace_root)]) + + assert result.exit_code == 0, result.output + for hidden in (*secrets, *digests): + assert hidden not in result.stdout + assert hidden not in result.stderr + + +def test_list_on_a_workspace_with_no_tokens_prints_only_a_header( + workspace_root: Path, +) -> None: + """The header prints either way, so ``| tail -n +2`` is stable.""" + result = runner.invoke(app, ["token", "list", "-w", str(workspace_root)]) + + assert result.exit_code == 0, result.output + assert _rows(result.stdout) == [["NAME", "CREATED", "REVOKED"]] + assert "No tokens" in result.stderr + + +def test_list_shows_a_live_token_with_no_revocation_time(workspace_root: Path) -> None: + _create(workspace_root, "ci") + + result = runner.invoke(app, ["token", "list", "-w", str(workspace_root)]) + + assert _rows(result.stdout)[1][2] == "-" + + +def test_list_shows_a_revoked_token_with_the_moment_it_died(workspace_root: Path) -> None: + _create(workspace_root, "ci") + runner.invoke(app, ["token", "revoke", "ci", "--yes", "-w", str(workspace_root)]) + + result = runner.invoke(app, ["token", "list", "-w", str(workspace_root)]) + + revoked = _rows(result.stdout)[1][2] + assert revoked.endswith("Z") + assert revoked != "-" + + +# --- revoke ------------------------------------------------------------------- + + +def test_revoke_asks_before_burning_a_credential(workspace_root: Path) -> None: + _create(workspace_root, "ci") + + result = runner.invoke(app, ["token", "revoke", "ci", "-w", str(workspace_root)], input="n\n") + + assert result.exit_code != 0 + assert _stored(workspace_root)[0].revoked is False + + +def test_revoke_with_yes_burns_the_credential_without_asking(workspace_root: Path) -> None: + """No ``input=``: a prompt here would abort on EOF and fail this test.""" + _create(workspace_root, "ci") + + result = runner.invoke(app, ["token", "revoke", "ci", "--yes", "-w", str(workspace_root)]) + + assert result.exit_code == 0, result.output + assert _stored(workspace_root)[0].revoked is True + + +def test_a_revoked_token_no_longer_authenticates(workspace_root: Path) -> None: + secret = _create(workspace_root, "ci") + runner.invoke(app, ["token", "revoke", "ci", "--yes", "-w", str(workspace_root)]) + + with TestClient(workspace_app(workspace_root)) as client: + response = client.get(PROBE_PATH, headers={"Authorization": f"Bearer {secret}"}) + + assert response.status_code == 401 + + +def test_revoke_resolves_a_name_case_insensitively(workspace_root: Path) -> None: + """Token names are unique case-insensitively, so one spelling is enough.""" + _create(workspace_root, "ci") + + result = runner.invoke(app, ["token", "revoke", "CI", "--yes", "-w", str(workspace_root)]) + + assert result.exit_code == 0, result.output + assert "'ci'" in result.stderr, "the matched name is printed, not the typed one" + assert _stored(workspace_root)[0].revoked is True + + +def test_revoking_an_unknown_name_exits_one_with_a_readable_message( + workspace_root: Path, +) -> None: + result = runner.invoke(app, ["token", "revoke", "ghost", "--yes", "-w", str(workspace_root)]) + + assert result.exit_code == 1 + assert "no token named" in result.stderr + + +def test_revoking_an_already_revoked_token_succeeds_and_says_so(workspace_root: Path) -> None: + """A retried ``token revoke ci`` has to be safe, and must not prompt.""" + _create(workspace_root, "ci") + runner.invoke(app, ["token", "revoke", "ci", "--yes", "-w", str(workspace_root)]) + died = _stored(workspace_root)[0].revoked_at + + result = runner.invoke(app, ["token", "revoke", "ci", "-w", str(workspace_root)]) + + assert result.exit_code == 0, result.output + assert "already revoked" in result.stderr + assert _stored(workspace_root)[0].revoked_at == died + + +# --- exit codes --------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("label", "argv"), + [ + ("create", ["token", "create", "--name", "fresh"]), + ("list", ["token", "list"]), + ("revoke", ["token", "revoke", "ci", "--yes"]), + ], +) +def test_every_token_command_exits_zero_when_it_succeeds( + workspace_root: Path, label: str, argv: list[str] +) -> None: + _create(workspace_root, "ci") + + result = runner.invoke(app, [*argv, "-w", str(workspace_root)]) + + assert result.exit_code == 0, f"{label}: {result.output}" diff --git a/tests/cli/test_workspace_option.py b/tests/cli/test_workspace_option.py new file mode 100644 index 00000000..0de2e7ce --- /dev/null +++ b/tests/cli/test_workspace_option.py @@ -0,0 +1,151 @@ +"""Which workspace a command operates on, end to end through the CLI. + +The rule itself is covered in ``tests/kernel/test_workspace_service.py``; these +assert that a *command* reaches it — the flag is wired, the precedence survives +Click, and a command outside any workspace refuses in a way somebody can act on. + +These assume no ancestor of ``tmp_path`` holds a ``visionset.db``. That holds +under pytest's temporary root; if one of them ever fails on a machine where +somebody made a workspace of ``/tmp``, this is the reason. +""" + +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from visionset.cli.main import app +from visionset.kernel.services import WORKSPACE_ENV_VAR, TokenService, WorkspaceService + +runner = CliRunner() + + +@pytest.fixture(autouse=True) +def _no_ambient_workspace(monkeypatch: pytest.MonkeyPatch) -> None: + """A developer with ``VISIONSET_WORKSPACE`` exported gets CI's results.""" + monkeypatch.delenv(WORKSPACE_ENV_VAR, raising=False) + + +@pytest.fixture() +def workspace_root(tmp_path: Path) -> Path: + root = tmp_path / "ws" + WorkspaceService.init(root).close() + return root + + +def _token_names(root: Path) -> list[str]: + workspace = WorkspaceService.open(root) + names = [token.name for token in TokenService(workspace).list()] + workspace.close() + return names + + +# --- the flag ----------------------------------------------------------------- + + +def test_the_workspace_flag_names_the_workspace(workspace_root: Path) -> None: + result = runner.invoke( + app, ["token", "create", "--name", "ci", "--workspace", str(workspace_root)] + ) + assert result.exit_code == 0, result.output + assert _token_names(workspace_root) == ["ci"] + + +def test_the_flag_may_follow_the_subcommand(workspace_root: Path) -> None: + """The whole reason ``--workspace`` is per command rather than on the callback. + + A Click group stops parsing at the first non-option token, so an option + declared on ``@app.callback()`` would have to *precede* ``token``, and this + invocation — the one everybody actually types — would exit 2 with "No such + option". + """ + result = runner.invoke(app, ["token", "list", "--workspace", str(workspace_root)]) + assert result.exit_code == 0, result.output + + +def test_the_short_flag_works_too(workspace_root: Path) -> None: + result = runner.invoke(app, ["token", "list", "-w", str(workspace_root)]) + assert result.exit_code == 0, result.output + + +# --- precedence --------------------------------------------------------------- + + +def test_the_environment_variable_names_the_workspace_when_no_flag_is_given( + workspace_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(workspace_root)) + result = runner.invoke(app, ["token", "create", "--name", "ci"]) + assert result.exit_code == 0, result.output + assert _token_names(workspace_root) == ["ci"] + + +def test_the_flag_wins_over_the_environment_variable( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + flagged = tmp_path / "flagged" + ambient = tmp_path / "ambient" + for root in (flagged, ambient): + WorkspaceService.init(root).close() + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(ambient)) + + result = runner.invoke(app, ["token", "create", "--name", "ci", "-w", str(flagged)]) + + assert result.exit_code == 0, result.output + assert _token_names(flagged) == ["ci"] + assert _token_names(ambient) == [] + + +# --- discovery ---------------------------------------------------------------- + + +def test_a_command_run_inside_a_workspace_finds_it( + workspace_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(workspace_root) + result = runner.invoke(app, ["token", "create", "--name", "ci"]) + assert result.exit_code == 0, result.output + assert _token_names(workspace_root) == ["ci"] + + +def test_a_command_run_below_a_workspace_finds_the_one_above( + workspace_root: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + below = workspace_root / "assets" / "raw" + below.mkdir(parents=True) + monkeypatch.chdir(below) + + result = runner.invoke(app, ["token", "create", "--name", "ci"]) + + assert result.exit_code == 0, result.output + assert _token_names(workspace_root) == ["ci"] + + +def test_the_flag_pointed_below_a_workspace_does_not_walk_up_to_it( + workspace_root: Path, +) -> None: + """A stated root is never traded for its parent — see the resolver's docstring.""" + below = workspace_root / "assets" + below.mkdir() + + result = runner.invoke(app, ["token", "create", "--name", "ci", "-w", str(below)]) + + assert result.exit_code == 1 + assert _token_names(workspace_root) == [] + + +# --- refusals ----------------------------------------------------------------- + + +def test_a_command_outside_any_workspace_exits_one_with_a_readable_message( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(tmp_path) + + result = runner.invoke(app, ["token", "list"]) + + assert result.exit_code == 1 + assert "not a VisionSet workspace" in result.stderr + assert "--workspace" in result.stderr + assert WORKSPACE_ENV_VAR in result.stderr + assert result.stdout == "" From 1d27552cf5fe19d677e56f1227d402e8ae9b790b Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Mon, 27 Jul 2026 18:48:41 -0700 Subject: [PATCH 4/4] docs: the token commands, and where 'which workspace' is decided workspaces.md gains the precedence table and the argument for why only cwd detection walks upward; auth.md gains the three commands, the stdout/stderr split, the terminal's exit-code contract, and the behaviour change the server inherited from sharing one resolver. No docs/cli.md: auth.md owns tokens and workspaces.md owns the workspace. One earns its keep when the second command family lands and it has more than a stub to hold. --- docs/README.md | 4 ++-- docs/auth.md | 60 +++++++++++++++++++++++++++++++++++++++------- docs/workspaces.md | 42 +++++++++++++++++++++++++++++++- 3 files changed, 94 insertions(+), 12 deletions(-) diff --git a/docs/README.md b/docs/README.md index 63117207..ddfcf673 100644 --- a/docs/README.md +++ b/docs/README.md @@ -6,7 +6,7 @@ contracts (kernel purity, headless annotator) are described there and enforced i | Doc | Covers | | --- | --- | -| [workspaces.md](workspaces.md) | The workspace on disk: layout, `init`/`open`, project-name uniqueness, and how services are composed | +| [workspaces.md](workspaces.md) | The workspace on disk: layout, `init`/`open`, which workspace a surface resolves to, project-name uniqueness, and how services are composed | | [projects.md](projects.md) | The project lifecycle: the 1:1 dataset, renaming, and what deletion does and does not destroy | | [sources.md](sources.md) | Where raw data comes from: the two registration methods, what a video source records from the probe, why decomposition parameters live on the source, and the idempotency rule and its named uniqueness gap | | [ingest.md](ingest.md) | Turning a source into rows: content identity versus recorded origin, the two source paths, why the decode happens outside a transaction, the run's lifecycle and pollable progress, and the per-file report | @@ -21,4 +21,4 @@ contracts (kernel purity, headless annotator) are described there and enforced i | [persistence.md](persistence.md) | The metadata store: repositories, unit of work, table layout, migrations and `format_version` | | [examples.md](examples.md) | The two runnable examples: the whole cycle in one pass, ingest on its own, and what each is built to demonstrate | | [api.md](api.md) | The REST surface: the one error body, why clients branch on `code` and not on the status, what decides 404 / 409 / 422, what a 5xx does and does not tell you, and which codes are worth retrying | -| [auth.md](auth.md) | Who may call it: per-workspace API tokens, why only a digest is stored, why every refusal is one identical 401, immediate revocation, and how a protected route is built | +| [auth.md](auth.md) | Who may call it: per-workspace API tokens, why only a digest is stored, why every refusal is one identical 401, immediate revocation, the `visionset token` commands, and how a protected route is built | diff --git a/docs/auth.md b/docs/auth.md index 59601fbf..391b62aa 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -5,7 +5,9 @@ scoped to one workspace, and holding a valid one means holding the whole workspa permissions are deliberately not here. ``` -visionset token create --name ci # prints the secret once — issue #26 +visionset token create --name ci # prints the secret once, on stdout +visionset token list # names, created, revoked — never secrets +visionset token revoke ci # asks first, unless you pass --yes ``` ``` @@ -63,6 +65,41 @@ Nothing caches a verdict. "Revoked, therefore refused" has to mean *now*, so the the workspace on every call. That read is cheap: WAL readers never block a writer, and a read-only unit of work takes no lock at all. +### At a terminal + +`ConfirmationRequired` exists because the kernel has no terminal and no user; each surface asks in +its own idiom and passes the answer down. The CLI's idiom is a prompt, skipped by `--yes`: + +``` +$ visionset token create --name ci +Created token 'ci' in /srv/vision. ← stderr +vst_YnYwMfhwzqRqfg5VjaoHwvoUFb6bx42GVYeTaICVCwI ← stdout +This secret is shown once and cannot be recovered. Store it now. ← stderr + +$ visionset token list +NAME CREATED REVOKED +ci 2026-07-28T01:43:47Z - + +$ visionset token revoke ci +Revoke token 'ci'? Every client holding its secret stops working, and this cannot be undone. [y/N]: +``` + +**Stdout is data; stderr is everything a person reads.** The secret is the only thing on stdout, so +`TOKEN=$(visionset token create --name ci)` is exactly the secret and the warning survives the +redirection that most needs it. `token list` names its three columns one at a time rather than +dumping the model, so neither the secret nor its digest can reach the terminal by accident — a +digest is not a secret, but it verifies a guess offline. + +Revoking is resolved **by name**, which is why token names are unique case-insensitively. Revoking +an already-revoked token exits 0, says when it died, and does not prompt: a retried command must be +safe, and asking somebody to re-confirm a thing already done invites a "yes" that means nothing. +A token that does not exist exits 1 with the kernel's own sentence. + +Exit codes are the whole error contract at a terminal: **0** success, **1** any domain refusal as +one sentence on stderr, **2** a usage error Click raises itself. There is no per-error code — a +shell branches on zero versus non-zero, and a person reads the sentence. That is the deliberate +difference from the REST surface, where a client branches on `code` because it is a program. + ## What a refusal looks like A missing header, a non-bearer scheme, an empty credential, an unknown token and a revoked token @@ -85,22 +122,27 @@ wrong thing. ## Which workspace the server serves -One, named by **`VISIONSET_WORKSPACE`**, defaulting to the process's working directory. It is -opened by the first request that needs it and kept for the life of the process — never at import -time, because `scripts/export_openapi.py` imports the application in a checkout that has no +One, resolved by the same rule the CLI uses — `kernel/services/workspace_service.py:: +resolve_workspace_root`. A server started by import string has no argv of its own, so of the four +branches it can only reach two: **`VISIONSET_WORKSPACE`**, then the nearest workspace at or above +the working directory. The precedence table and the argument for why only that last case walks +upward live in [workspaces.md](workspaces.md#which-workspace-when-nobody-said). + +It is opened by the first request that needs it and kept for the life of the process — never at +import time, because `scripts/export_openapi.py` imports the application in a checkout that has no workspace. +> **Changed in #26.** A server started *below* a workspace with no `VISIONSET_WORKSPACE` set now +> serves that workspace, where it used to answer 500 `NOT_A_WORKSPACE`. That is the cost of one +> resolver instead of two, and the asymmetry that keeps it safe is that a *stated* root — the +> variable here, `--workspace` at the CLI — is never traded for its parent. + A server pointed at something that is not a workspace answers **500 `NOT_A_WORKSPACE`**, opaque body plus an `incident_id`, with the path in the log only. That is a deployment fault, not a client error. It arrives *instead of* a 401 even when no token was sent, because the workspace is resolved before the credential is looked at — the ordering is what keeps authentication overridable in tests. -> The full resolution rule — a `--workspace` flag, the environment variable, cwd detection, and a -> documented precedence shared with the CLI — belongs to issue #26. -> `server/dependencies.py::resolve_workspace_root` is the provisional half, reading the variable -> #26 will keep. - ## For contributors Build every non-public router with **`protected_router()`**: diff --git a/docs/workspaces.md b/docs/workspaces.md index c5400998..1ead1483 100644 --- a/docs/workspaces.md +++ b/docs/workspaces.md @@ -90,7 +90,9 @@ directory is also non-empty, and "open it instead" is the useful message. Emptiness is strict — a stray `.DS_Store` is enough to refuse. That matches the contract literally and keeps `init` from ever writing into, say, a git repository root. Worth revisiting when the `visionset init` CLI command lands, where a friendlier rule may earn -its keep. +its keep — that command is also the one that will have to decide whether `init` with no +path means "here" or means the resolver's answer, since the resolver deliberately walks +upward and `init` must not. If anything fails midway, `init` removes what it created and nothing else: the whole directory if `init` made it, otherwise just the database and `blobs/`. "Fails safely" means @@ -135,6 +137,44 @@ on open would break opening a workspace on a read-only mount — a shared datase read-only bind mount in a container — and would make `open` non-idempotent for no invariant gain. Read `root`; treat `root_dir` as a possibly-stale hint. +## Which workspace, when nobody said + +`init` and `open` are handed a path. Deciding *which* path is a separate question, and it has one +answer for every surface: `resolve_workspace_root(explicit=None)`, beside `DB_FILENAME` in +`kernel/services/workspace_service.py`. It lives there because it is the same fact read from the +other end — the database file is what marks a directory as a workspace, and this is what goes +looking for the mark. It cannot live in either caller: import-linter forbids `visionset.server` +importing `visionset.cli`, so the rule the two share has to sit above both. + +Precedence, first match wins: + +| | Source | Walks up? | +| --- | --- | --- | +| 1 | `explicit` — the CLI's `--workspace` / `-w` | no | +| 2 | `VISIONSET_WORKSPACE`, when set to something non-empty | no | +| 3 | the nearest directory at or above the working directory holding a `visionset.db` | **yes** | +| 4 | the working directory | — | + +A server started by import string has no argv, so it reaches only 2, 3 and 4. + +**Only case 3 walks, and that asymmetry is the whole rule.** A flag and an environment variable are +somebody *stating* which workspace. If the stated directory holds none, walking to its parent and +quietly minting a credential into whatever workspace lives up there is the worst thing this +function could do. Git draws the line in the same place — discovery walks up, `--git-dir` and +`GIT_DIR` do not — and for the same reason. + +**Finding nothing is not an error here.** This names a directory; `open` owns "is this a +workspace?" and already raises `NotAWorkspace` naming the path it was given. A refusal here would +be two errors for one condition, and would make the function non-total for the server, which calls +it inside a lazily opened handle that expects exactly one failure mode. + +**Nothing is normalized.** `_resolved` is the one place a path becomes canonical and it runs inside +`init`/`open`; expanding `~` twice is how two spellings of one workspace start looking like two +workspaces. + +An empty `VISIONSET_WORKSPACE` falls through to case 3 rather than resolving to `Path("")`, because +a shell cannot tell `VISIONSET_WORKSPACE=` from an unset variable. + ## `format_version` lives in one place The database stamp in `_visionset_meta` is the sole authority. There is no sidecar marker