From 2617dee9a28c17a41b974b0853decb6f0786c9fb Mon Sep 17 00:00:00 2001 From: pk-zipstack Date: Tue, 1 Sep 2026 16:43:45 +0530 Subject: [PATCH 1/4] UN-4016 [FEAT] Support a platform API key: `auth whoami` and `deployment ls` Before running anything a user had to assemble a credential out of three values from three places: `org_id`, a deployment key, and the deployment's `api_name`. `org_id` had no documented source at all, and there was no way to ask what deployments exist, so every `api_name` was copied by hand out of the UI. A platform key carries its own organisation, so supplying the key is enough: export UNSTRACT_PLATFORM_KEY=... unstract auth whoami # resolves and stores org_id unstract docstudio deployment ls # no api_name paste `platform` is a new product group in the table-driven config layer, which lights up `config get/set/doctor`, `require()`'s hints and `settings_for()` with no new code. It deliberately has no `org_id` of its own -- `whoami` writes the resolved one to the docstudio block, which is where deployment URLs and aliases already read it from, so there is only ever one copy to keep in agreement. `PlatformClient` and its `list_api_deployments` already ship in the pinned client, so `ls` is a thin wrapper. Only `whoami` needed new transport: the client's `_url` always injects the organisation segment, so a small subclass builds that one URL itself rather than requiring an upstream release. Its `PlatformAPIError` is now translated like every other client failure -- without that a rejected key reached the entry point as a traceback instead of exit 3. `config doctor --probe` gains a platform branch. Its own docstring lamented having no side-effect-free endpoint to verify a key against; `whoami` is one. An absent platform key reports `ok: null` rather than failing, because holding only a deployment key is the common case and must not decide the exit code. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM --- README.md | 36 +++- src/unstract_cli/app.py | 23 ++- src/unstract_cli/commands/config_cmd.py | 36 +++- src/unstract_cli/commands/platform_cmd.py | 113 ++++++++++++ src/unstract_cli/config.py | 32 +++- src/unstract_cli/core/clients.py | 20 +- src/unstract_cli/core/platform.py | 80 ++++++++ tests/test_commands.py | 213 +++++++++++++++++++++- tests/test_discover.py | 79 +++++++- 9 files changed, 608 insertions(+), 24 deletions(-) create mode 100644 src/unstract_cli/commands/platform_cmd.py create mode 100644 src/unstract_cli/core/platform.py diff --git a/README.md b/README.md index 3c59880..06a658e 100644 --- a/README.md +++ b/README.md @@ -6,8 +6,9 @@ JSON back. It also clones one organization's resources into another. ```bash curl -LsSf https://raw.githubusercontent.com/Zipstack/unstract-cli/main/install.sh | sh -unstract config init -unstract config doctor +export UNSTRACT_PLATFORM_KEY=... +unstract auth whoami # resolves and stores your organisation +unstract docstudio deployment ls # what can I run? ``` The installer fetches `uv` if it is missing and installs the CLI with it; `uv` @@ -16,6 +17,11 @@ brings its own Python, so nothing on the machine has to match. Already have thing. Set `UNSTRACT_CLI_SOURCE` to install a branch or a local checkout instead. +`auth whoami` is the shortest way in: a platform key carries the organisation it +belongs to, so supplying the key is enough to discover `org_id` rather than +reading it out of a web-app URL. `config init` and `config doctor` are still +there for a profile you write by hand. + Or run it without installing: `uvx --from git+https://github.com/Zipstack/unstract-cli unstract --discover groups`. ## Output @@ -70,8 +76,8 @@ search, or `$UNSTRACT_CONFIG`, or `--config`. Every setting resolves **flag > env > profile > built-in default**, and the CLI is fully usable with no config file at all. The flag tier is the connection options on each product group — `unstract docstudio --base-url … --org-id … deployment run …`, and -`--base-url`/`--api-key` on `whisper` — which override the profile for that one -invocation without writing anything. +`--base-url`/`--api-key` on `whisper` and on `auth` — which override the profile +for that one invocation without writing anything. ```toml default_profile = "cloud-us" @@ -85,6 +91,10 @@ base_url = "https://us-central.unstract.com" org_id = "org_ABC123" api_key = "env:UNSTRACT_DEPLOYMENT_KEY" +[profiles.cloud-us.platform] +base_url = "https://us-central.unstract.com" +api_key = "env:UNSTRACT_PLATFORM_KEY" + [profiles.cloud-us.deployments.invoices] api_name = "invoice-parser" ``` @@ -96,9 +106,18 @@ own `api_key` when its deployment has a separate key of its own. Get an LLMWhisperer key from the LLMWhisperer console; a deployment key is shown on the API deployment's own page in the Unstract UI, and an organisation-wide one -under Settings → API Key Manager. `config init` also writes an -`onprem-example` profile as a shape to copy for a self-hosted install — its host -is a placeholder, and only the *active* profile is ever resolved. +under Settings → API Key Manager. A **platform key** is minted by an +organisation admin under Settings → Platform API Keys. `config init` also writes +an `onprem-example` profile as a shape to copy for a self-hosted install — its +host is a placeholder, and only the *active* profile is ever resolved. + +The two Unstract keys are not interchangeable and neither replaces the other. A +deployment key runs one deployment and cannot say which organisation it belongs +to; a platform key identifies the organisation and lists what is in it, and +cannot run a deployment. `auth whoami` and `deployment ls` take the platform key; +`deployment run` and `deployment status` take the deployment key. `org_id` lives +on the `docstudio` block either way — `auth whoami` writes the one it resolves +there, because that is where everything that needs it reads from. A credential can be written into the file literally, but `env:VAR_NAME` indirection is what `config init` writes and what the examples use: the file @@ -128,7 +147,8 @@ not part of the document-processing path the rest of this CLI wraps, so an agent serving a user request should not reach for it unasked. It talks to two deployments at once, which no single profile describes, so it takes both endpoints as flags and both keys from `UNSTRACT_SRC_PLATFORM_KEY` / -`UNSTRACT_TGT_PLATFORM_KEY`. It exits 0 when nothing failed, which is not the +`UNSTRACT_TGT_PLATFORM_KEY` — two keys for two organisations, so it reads neither +the `platform` profile block nor `$UNSTRACT_PLATFORM_KEY`. It exits 0 when nothing failed, which is not the same as everything having moved: oversize and unsupported documents are skipped by design, and `data.skipped` counts them. diff --git a/src/unstract_cli/app.py b/src/unstract_cli/app.py index 2eb6306..03cbe1c 100644 --- a/src/unstract_cli/app.py +++ b/src/unstract_cli/app.py @@ -16,6 +16,7 @@ from unstract_cli.config import ( DOCSTUDIO, LLMWHISPERER, + PLATFORM, ConfigError, ResolvedConfig, load_config, @@ -84,7 +85,7 @@ def override(self, product: str, values: dict[str, Any]) -> None: def secrets(self) -> list[str]: """Resolved credentials, for scrubbing anything on its way to a stream.""" out: list[str] = [] - for product in (LLMWHISPERER, DOCSTUDIO): + for product in (LLMWHISPERER, DOCSTUDIO, PLATFORM): try: if value := self.config.get(product, "api_key"): out.append(str(value)) @@ -234,11 +235,29 @@ def deployment_group() -> None: """Work with a deployed API.""" +@cli.group("auth") +@_connection_options() +@pass_context +def auth_group(ctx: Context, **overrides: str | None) -> None: + """Identify the credential you are using. + + Its flags configure the platform key, which is the credential that knows + which organisation it belongs to. A deployment key does not: it authenticates + against the deployment it was minted for and never reaches this endpoint. + """ + ctx.override(PLATFORM, overrides) + + cli.add_command(config_group) # Imported for their side effect of registering commands, and imported last # because those modules hang their commands off the groups declared just above. -from unstract_cli.commands import clone_cmd, docstudio_cmd, whisper_cmd # noqa: E402,F401 +from unstract_cli.commands import ( # noqa: E402,F401 + clone_cmd, + docstudio_cmd, + platform_cmd, + whisper_cmd, +) def command_tree() -> dict[str, Any]: diff --git a/src/unstract_cli/commands/config_cmd.py b/src/unstract_cli/commands/config_cmd.py index aa02390..7730613 100644 --- a/src/unstract_cli/commands/config_cmd.py +++ b/src/unstract_cli/commands/config_cmd.py @@ -18,6 +18,7 @@ DOCSTUDIO, KEY_SOURCES, LLMWHISPERER, + PLATFORM, PRODUCTS, ConfigError, ConfigFile, @@ -36,6 +37,7 @@ emit_result, resolve_format, ) +from unstract_cli.core.platform import platform_client #: Keys whose value is never echoed back, even on explicit request: this output #: is as likely to land in a log or a transcript as on a screen. @@ -208,10 +210,11 @@ def _probe(resolved: ResolvedConfig) -> dict[str, Any]: """Check each product's credentials against the service, where that is possible. LLMWhisperer has a read-only usage endpoint, so its key can be verified for - real. A deployment has no side-effect-free endpoint -- the only thing to call - is an execution -- so its entry reports that the settings resolve and says - plainly that nothing was verified. Claiming otherwise would be worse than - not checking. + real, and so does the platform API -- `whoami` reads nothing but the key + itself. A deployment has no side-effect-free endpoint -- the only thing to + call is an execution -- so its entry reports that the settings resolve and + says plainly that nothing was verified. Claiming otherwise would be worse + than not checking. """ out: dict[str, Any] = {} try: @@ -233,6 +236,31 @@ def _probe(resolved: ResolvedConfig) -> dict[str, Any]: "detail": "The key was accepted by the usage endpoint.", } + try: + with translated(endpoint="whoami"): + identity = platform_client(resolved).whoami() + except CLIError as exc: + out[PLATFORM] = { + "checked": True, + "ok": False, + "detail": exc.message, + "exit_code": int(exc.exit_code), + } + except ConfigError as exc: + # Null, not False: a platform key is optional -- a caller holding only a + # deployment key is the common case -- so an absent one is a report + # rather than a failure, and must not decide this command's exit code. + out[PLATFORM] = {"checked": False, "ok": None, "detail": str(exc)} + else: + out[PLATFORM] = { + "checked": True, + "ok": True, + # The organisation is the reason to hold this key, so the probe + # reports which one answered rather than only that one did. + "organization_id": identity.get("organization_id"), + "detail": "The key was accepted, and resolved to an organisation.", + } + resolves = all( resolved.get(DOCSTUDIO, key) for key in ("org_id", "api_key", "base_url") ) diff --git a/src/unstract_cli/commands/platform_cmd.py b/src/unstract_cli/commands/platform_cmd.py new file mode 100644 index 0000000..b103190 --- /dev/null +++ b/src/unstract_cli/commands/platform_cmd.py @@ -0,0 +1,113 @@ +"""`unstract auth whoami` and `unstract docstudio deployment ls`. + +Both authenticate with a platform key rather than a deployment key. The two +credentials are not interchangeable and neither is going away: a deployment key +runs one deployment and cannot describe the account, a platform key describes +the account and lists what is in it but cannot run anything. + +No OpenAPI spec is vendored for the platform API, so these declare their flags +by hand rather than through `spec_options`. +""" + +from __future__ import annotations + +from typing import Any + +import click + +from unstract_cli.app import Context, auth_group, deployment_group, pass_context +from unstract_cli.commands.common import finish +from unstract_cli.config import DOCSTUDIO, load_config, save_config +from unstract_cli.core.clients import translated +from unstract_cli.core.platform import organisation, platform_client + +#: The fields a deployment listing shows. The server sends around fifteen per +#: row, including run histories; `--output table` wraps rather than truncates, +#: so the whole row is unreadable at a terminal. Narrowed here rather than +#: silently cut off downstream -- `--full` returns the rows as sent. +LISTING_FIELDS = ("api_name", "display_name", "id", "is_active", "api_endpoint") + + +def _store_organisation(obj: Any, org_id: str) -> dict[str, Any]: + """Write the resolved organisation into the active profile. + + It lands on the docstudio block because that is where every consumer reads + it from -- deployment URLs and aliases both -- and a second copy under the + platform block would be one more thing to keep in agreement. + """ + cfg = load_config() + name = getattr(obj, "profile", None) or cfg.default_profile or "cloud-us" + cfg.profiles.setdefault(name, {}).setdefault(DOCSTUDIO, {})["org_id"] = org_id + if not cfg.default_profile: + cfg.default_profile = name + return {"profile": name, "path": str(save_config(cfg))} + + +@auth_group.command("whoami") +@click.option( + "--save/--no-save", + default=True, + help="Write the resolved organisation into the active profile.", +) +@pass_context +def whoami(ctx: Context, save: bool) -> None: + """Resolve which organisation your platform key belongs to. + + The organisation is otherwise only discoverable by reading it out of a + web-app URL, and every other command needs it. Resolving it here and storing + it means it is supplied once rather than pasted. + + \b + Examples: + export UNSTRACT_PLATFORM_KEY=... + unstract auth whoami + unstract auth whoami --no-save # validate the key, change nothing + + Exits 3 when the key is rejected, so a setup script can branch on it without + reading the message. + """ + client = platform_client(ctx.config) + with translated(endpoint="whoami"): + identity = client.whoami() + + meta = {"saved": False} + if save and (org_id := identity.get("organization_id")): + meta = {"saved": True, **_store_organisation(ctx, str(org_id))} + finish(ctx, identity, meta=meta) + + +@deployment_group.command("ls") +@click.option( + "--api-name", + default=None, + help="Return only the deployment with this exact API name.", +) +@click.option( + "--full/--no-full", + default=False, + help=f"Return every field the server sends, not just {', '.join(LISTING_FIELDS)}.", +) +@pass_context +def ls(ctx: Context, api_name: str | None, full: bool) -> None: + """List the API deployments in your organisation. + + Answers what a deployment is called, which is the one thing `deployment run` + needs and the UI is the only other place to find. Authenticates with the + platform key, not the deployment key. + + \b + Examples: + unstract docstudio deployment ls + unstract docstudio deployment ls --api-name invoice-parser + unstract docstudio deployment ls --full + """ + client = platform_client(ctx.config, organisation(ctx.config)) + with translated(endpoint="api/deployment/"): + rows = client.list_api_deployments(api_name=api_name) + + if not full: + rows = [{field: row.get(field) for field in LISTING_FIELDS} for row in rows] + finish(ctx, {"results": rows}, meta={"count": len(rows)}) + + +__all__ = ["ls", "whoami"] diff --git a/src/unstract_cli/config.py b/src/unstract_cli/config.py index 2064808..8b0eb60 100644 --- a/src/unstract_cli/config.py +++ b/src/unstract_cli/config.py @@ -30,34 +30,49 @@ LLMWHISPERER = "llmwhisperer" DOCSTUDIO = "docstudio" -PRODUCTS: tuple[str, ...] = (LLMWHISPERER, DOCSTUDIO) +PLATFORM = "platform" +PRODUCTS: tuple[str, ...] = (LLMWHISPERER, DOCSTUDIO, PLATFORM) #: Built-in defaults, lowest precedence. DEFAULT_BASE_URLS: dict[str, str] = { LLMWHISPERER: "https://llmwhisperer-api.us-central.unstract.com/api/v2", DOCSTUDIO: "https://us-central.unstract.com", + # The same host as docstudio: one deployment serves both the platform API + # and the deployments it manages. + PLATFORM: "https://us-central.unstract.com", } #: Environment variables per (product, setting), checked before the config file #: and in the order given. The trailing names are the ones the published clients #: themselves read: an environment already set up for a client must not leave #: the CLI silently on its built-in default, which is production. +#: +#: `platform` deliberately has no `org_id` of its own. A platform key carries +#: its organisation, and `auth whoami` writes the one it resolves to the +#: docstudio block -- the block everything else already reads. Two `org_id` +#: settings would mean two rows in `config doctor` that a user has to keep in +#: agreement by hand. ENV_VARS: dict[tuple[str, str], tuple[str, ...]] = { (LLMWHISPERER, "api_key"): ("LLMWHISPERER_API_KEY",), (LLMWHISPERER, "base_url"): ("LLMWHISPERER_BASE_URL", "LLMWHISPERER_BASE_URL_V2"), (DOCSTUDIO, "api_key"): ("UNSTRACT_DEPLOYMENT_KEY", "UNSTRACT_API_DEPLOYMENT_KEY"), (DOCSTUDIO, "base_url"): ("UNSTRACT_BASE_URL",), (DOCSTUDIO, "org_id"): ("UNSTRACT_ORG_ID",), + (PLATFORM, "api_key"): ("UNSTRACT_PLATFORM_KEY",), + (PLATFORM, "base_url"): ("UNSTRACT_BASE_URL",), } -#: Where the two credentials are minted. Quoted wherever the CLI reports one as -#: missing: knowing a key is unset is no help without knowing where one is made. +#: Where the three credentials are minted. Quoted wherever the CLI reports one +#: as missing: knowing a key is unset is no help without knowing where one is +#: made. KEY_SOURCES = ( "Get an LLMWhisperer key from the LLMWhisperer console; a deployment key is " "shown on the API deployment's own page in the Unstract UI, and a key " "covering every deployment in the organisation is minted under " - "Settings -> API Key Manager." + "Settings -> API Key Manager. A platform key, which identifies the " + "organisation and lists what is in it but cannot run a deployment, is " + "minted by an organisation admin under Settings -> Platform API Keys." ) @@ -538,6 +553,10 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "org_id": "", "api_key": "env:UNSTRACT_DEPLOYMENT_KEY", }, + PLATFORM: { + "base_url": DEFAULT_BASE_URLS[PLATFORM], + "api_key": "env:UNSTRACT_PLATFORM_KEY", + }, "deployments": {"example": {"api_name": "your-api-deployment-name"}}, }, "cloud-eu": { @@ -558,6 +577,10 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "org_id": "", "api_key": "env:UNSTRACT_DEPLOYMENT_KEY", }, + PLATFORM: { + "base_url": "https://unstract.internal.example", + "api_key": "env:UNSTRACT_PLATFORM_KEY", + }, }, } @@ -569,6 +592,7 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "HOME_CONFIG", "KEY_SOURCES", "LLMWHISPERER", + "PLATFORM", "PRODUCTS", "PROJECT_CONFIG_NAME", "UNTRUSTED_PROJECT_KEYS", diff --git a/src/unstract_cli/core/clients.py b/src/unstract_cli/core/clients.py index 439b3d1..5375104 100644 --- a/src/unstract_cli/core/clients.py +++ b/src/unstract_cli/core/clients.py @@ -5,9 +5,10 @@ expected, so it is translated here into a ``CLIError`` carrying an exit code, a hint and the response detail. -The two clients report failure differently -- LLMWhisperer raises with a status -code attached, the deployment client returns a dict containing one -- so both -shapes converge here rather than in each command. +The clients report failure differently -- LLMWhisperer raises with a status +code attached, the deployment client returns a dict containing one, the Platform +API client raises with the body attached -- so every shape converges here rather +than in each command. """ from __future__ import annotations @@ -21,6 +22,7 @@ APIDeploymentsClient, APIDeploymentsClientException, ) +from unstract.clone.exceptions import PlatformAPIError from unstract.llmwhisperer.client_v2 import ( LLMWhispererClientException, LLMWhispererClientV2, @@ -137,6 +139,18 @@ def translated(endpoint: str | None = None) -> Iterator[None]: raise CLIError(message, details=details, endpoint=endpoint) from exc except APIDeploymentsClientException as exc: raise CLIError(str(exc), ExitCode.USAGE, endpoint=endpoint) from exc + except PlatformAPIError as exc: + # The Platform API client raises rather than returning a status, and + # carries the response body on the exception. Untranslated it would + # reach the entry point as an unexpected crash and print a traceback. + if exc.status_code: + raise error_from_status( + int(exc.status_code), + str(exc), + details=exc.body, + endpoint=endpoint, + ) from exc + raise CLIError(str(exc), details=exc.body, endpoint=endpoint) from exc except Timeout as exc: raise CLIError( str(exc), diff --git a/src/unstract_cli/core/platform.py b/src/unstract_cli/core/platform.py new file mode 100644 index 0000000..e5d8b06 --- /dev/null +++ b/src/unstract_cli/core/platform.py @@ -0,0 +1,80 @@ +"""The Platform API client, and the one call the published client lacks. + +`PlatformClient` ships in `unstract-client` for the clone subpackage, and its +list endpoints are exactly what `deployment ls` needs. It builds every URL as +``{base_url}/{prefix}/unstract/{organization_id}//``, which is right for +every call that acts inside an organisation and wrong for the one call made +before the organisation is known. + +`whoami` is that call, so it is added here rather than in the published client: +it needs no release to land, and the organisation-less shape is a CLI concern +until something else wants it. +""" + +from __future__ import annotations + +from typing import Any + +from unstract.clone.client import PlatformClient +from unstract.clone.context import OrgEndpoint + +from unstract_cli.config import DOCSTUDIO, PLATFORM, ResolvedConfig +from unstract_cli.core.errors import CLIError, ExitCode + +#: The organisation `whoami` is called with. The endpoint carries no +#: organisation segment -- resolving it is the point of the call -- and +#: `OrgEndpoint` requires the field, so it is named rather than left as a bare +#: empty string at the call site. +NO_ORGANISATION = "" + + +class CLIPlatformClient(PlatformClient): + """`PlatformClient` plus the organisation-less identity read.""" + + def whoami(self) -> dict[str, Any]: + """Describe the key: which organisation it belongs to, and its tier. + + The URL is built here instead of through ``_url`` because that method + inserts the organisation this call exists to discover. + """ + base = self.endpoint.base_url.rstrip("/") + prefix = self.endpoint.api_path_prefix.strip("/") + return self._send("GET", f"{base}/{prefix}/unstract/whoami/", "whoami/") + + +def platform_client( + config: ResolvedConfig, org_id: str | None = None +) -> CLIPlatformClient: + """Build a Platform API client from the resolved configuration. + + ``org_id`` is optional because `whoami` runs before one is known. Every + other call needs it, and asks for it explicitly. + """ + return CLIPlatformClient( + OrgEndpoint( + base_url=config.require(PLATFORM, "base_url"), + organization_id=org_id if org_id is not None else NO_ORGANISATION, + platform_key=config.require(PLATFORM, "api_key"), + ) + ) + + +def organisation(config: ResolvedConfig) -> str: + """The organisation to act inside, or a usage error naming how to get one. + + It lives on the docstudio block: a platform key resolves it, and everything + that consumes it -- deployment URLs, aliases -- reads it from there. + """ + if org_id := config.get(DOCSTUDIO, "org_id"): + return str(org_id) + raise CLIError( + "No organisation is configured.", + ExitCode.USAGE, + hint=( + "Run `unstract auth whoami` to resolve it from your platform key, " + "or set $UNSTRACT_ORG_ID." + ), + ) + + +__all__ = ["CLIPlatformClient", "organisation", "platform_client"] diff --git a/tests/test_commands.py b/tests/test_commands.py index 015a354..96d9b94 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -12,6 +12,7 @@ import pytest from requests.exceptions import ConnectionError +from unstract.clone.exceptions import PlatformAPIError from unstract.clone.report import CloneReport, Endpoint, PhaseResult from unstract.llmwhisperer.client_v2 import ( LLMWhispererClientException, @@ -22,8 +23,8 @@ from unstract_cli.__main__ import main from unstract_cli.app import command_tree -from unstract_cli.commands import clone_cmd, docstudio_cmd, whisper_cmd -from unstract_cli.config import LLMWHISPERER +from unstract_cli.commands import clone_cmd, docstudio_cmd, platform_cmd, whisper_cmd +from unstract_cli.config import LLMWHISPERER, PLATFORM from unstract_cli.core.errors import CLIError, ExitCode @@ -117,6 +118,27 @@ def build(_config, _target, transport_timeout=None): return install +@pytest.fixture +def platform_client(monkeypatch): + """Install a fake Platform API client and hand it back to the test.""" + + def install(**replies): + client = FakeWhisper(**replies) + client.built_with = {} + + def build(config, org_id=None): + # Resolving the key is what registers it for scrubbing, so the fake + # factory has to do it too or the seam hides a production path. + client.built_with["api_key"] = config.get(PLATFORM, "api_key") + client.built_with["org_id"] = org_id + return client + + monkeypatch.setattr(platform_cmd, "platform_client", build) + return client + + return install + + # --------------------------------------------------------------------------- # # The command surface # --------------------------------------------------------------------------- # @@ -140,9 +162,11 @@ def test_the_v1_commands_are_registered(): "update", } assert set(tree["docstudio"]["commands"]["deployment"]["commands"]) == { + "ls", "run", "status", } + assert set(tree["auth"]["commands"]) == {"whoami"} # --------------------------------------------------------------------------- # @@ -1119,3 +1143,188 @@ def fake_clone(source, target, options): assert code == int(ExitCode.SUCCESS) assert "adapters" in captured.out assert key not in captured.out and key not in captured.err + + +# --------------------------------------------------------------------------- # +# auth whoami +# --------------------------------------------------------------------------- # + +IDENTITY = { + "organization_id": "org_ABC123", + "organization_name": "Acme", + "permission": "read", + "key_name": "ci", +} + + +def _platform_env(monkeypatch, tmp_path): + """A resolvable platform key, and a config file of our own to write into.""" + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "config.toml")) + + +def test_whoami_reports_the_identity_the_service_returned( + capsys, platform_client, monkeypatch, tmp_path +): + _platform_env(monkeypatch, tmp_path) + platform_client(whoami=IDENTITY) + + code, out, _ = run(capsys, "auth", "whoami") + + assert code == int(ExitCode.SUCCESS) + assert envelope(out)["data"] == IDENTITY + + +def test_whoami_is_called_with_no_organisation( + capsys, platform_client, monkeypatch, tmp_path +): + """Resolving the organisation is the point, so requiring one would be + circular.""" + _platform_env(monkeypatch, tmp_path) + client = platform_client(whoami=IDENTITY) + + run(capsys, "auth", "whoami") + + assert client.built_with["org_id"] is None + assert client.built_with["api_key"] == "pk-123" + + +def test_whoami_stores_the_organisation_where_everything_else_reads_it( + capsys, platform_client, monkeypatch, tmp_path +): + _platform_env(monkeypatch, tmp_path) + platform_client(whoami=IDENTITY) + + _, out, _ = run(capsys, "auth", "whoami") + + assert envelope(out)["meta"]["saved"] is True + # Read back through the CLI rather than out of the file: what matters is + # that the next command resolves it, not where the bytes landed. + _, out, _ = run(capsys, "config", "get", "docstudio", "org_id") + assert envelope(out)["data"]["value"] == "org_ABC123" + + +def test_whoami_can_validate_without_writing_anything( + capsys, platform_client, monkeypatch, tmp_path +): + _platform_env(monkeypatch, tmp_path) + platform_client(whoami=IDENTITY) + + _, out, _ = run(capsys, "auth", "whoami", "--no-save") + + assert envelope(out)["meta"]["saved"] is False + assert not (tmp_path / "config.toml").exists() + + +def test_a_rejected_platform_key_exits_on_the_auth_code( + capsys, platform_client, monkeypatch, tmp_path +): + """A traceback here would mean the Platform API's own exception type never + reached the translator.""" + _platform_env(monkeypatch, tmp_path) + platform_client(whoami=PlatformAPIError("nope", status_code=401, body="{}")) + + code, out, _ = run(capsys, "auth", "whoami") + + assert code == int(ExitCode.AUTH) + assert envelope(out)["ok"] is False + + +def test_whoami_without_a_key_is_a_usage_error(capsys, monkeypatch, tmp_path): + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "config.toml")) + code, out, _ = run(capsys, "auth", "whoami") + + assert code == int(ExitCode.USAGE) + assert "UNSTRACT_PLATFORM_KEY" in json.dumps(envelope(out)["error"]) + + +# --------------------------------------------------------------------------- # +# docstudio deployment ls +# --------------------------------------------------------------------------- # + +DEPLOYMENT_ROW = { + "api_name": "invoice-parser", + "display_name": "Invoices", + "id": "dep-1", + "is_active": True, + "api_endpoint": "https://example.com/deployment/api/org/invoice-parser/", + "created_by_email": "someone@example.com", + "last_5_run_statuses": [], +} + + +def _returns(value): + """Queue one reply whose value is itself a list. + + `FakeWhisper` reads a list reply as a queue of replies, so a bare list would + hand back its first row rather than the listing. + """ + return [value] + + +def _listing_env(monkeypatch, tmp_path): + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + monkeypatch.setenv("UNSTRACT_ORG_ID", "org_ABC123") + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "config.toml")) + + +def test_ls_narrows_the_row_to_what_a_caller_can_read( + capsys, platform_client, monkeypatch, tmp_path +): + _listing_env(monkeypatch, tmp_path) + platform_client(list_api_deployments=_returns([DEPLOYMENT_ROW])) + + _, out, _ = run(capsys, "docstudio", "deployment", "ls") + + (row,) = envelope(out)["data"]["results"] + assert set(row) == set(platform_cmd.LISTING_FIELDS) + assert row["api_name"] == "invoice-parser" + + +def test_ls_can_return_every_field_the_server_sent( + capsys, platform_client, monkeypatch, tmp_path +): + _listing_env(monkeypatch, tmp_path) + platform_client(list_api_deployments=_returns([DEPLOYMENT_ROW])) + + _, out, _ = run(capsys, "docstudio", "deployment", "ls", "--full") + + (row,) = envelope(out)["data"]["results"] + assert row == DEPLOYMENT_ROW + + +def test_ls_passes_the_name_filter_to_the_server( + capsys, platform_client, monkeypatch, tmp_path +): + """Filtering here rather than locally: the server has the exact-match + filter, and a local one would still page the whole organisation.""" + _listing_env(monkeypatch, tmp_path) + client = platform_client(list_api_deployments=_returns([DEPLOYMENT_ROW])) + + run(capsys, "docstudio", "deployment", "ls", "--api-name", "invoice-parser") + + assert client.kwargs_for("list_api_deployments") == {"api_name": "invoice-parser"} + + +def test_ls_runs_inside_the_configured_organisation( + capsys, platform_client, monkeypatch, tmp_path +): + _listing_env(monkeypatch, tmp_path) + client = platform_client(list_api_deployments=_returns([])) + + run(capsys, "docstudio", "deployment", "ls") + + assert client.built_with["org_id"] == "org_ABC123" + + +def test_ls_without_an_organisation_says_how_to_get_one( + capsys, platform_client, monkeypatch, tmp_path +): + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "config.toml")) + platform_client(list_api_deployments=_returns([])) + + code, out, _ = run(capsys, "docstudio", "deployment", "ls") + + assert code == int(ExitCode.USAGE) + assert "whoami" in json.dumps(envelope(out)["error"]) diff --git a/tests/test_discover.py b/tests/test_discover.py index 69c3a04..fb04ce6 100644 --- a/tests/test_discover.py +++ b/tests/test_discover.py @@ -28,7 +28,12 @@ def test_groups_names_the_products_and_stops_there(capsys): """The cheap question stays cheap: no command list, no flags.""" code, data = run(capsys, "--discover", "groups") assert code == int(ExitCode.SUCCESS) - assert {g["name"] for g in data["groups"]} == {"config", "docstudio", "whisper"} + assert {g["name"] for g in data["groups"]} == { + "auth", + "config", + "docstudio", + "whisper", + } # A leaf listed among the groups is a group a consumer finds empty. assert [c["name"] for c in data["commands"]] == ["clone"] assert all(entry["help"] for entry in [*data["groups"], *data["commands"]]) @@ -165,6 +170,22 @@ def get_usage_info(self): return install +@pytest.fixture +def platform_probe_client(monkeypatch): + def install(reply=None): + class Fake: + def whoami(self): + if isinstance(reply, Exception): + raise reply + return reply or {} + + monkeypatch.setattr( + config_cmd, "platform_client", lambda _config, org_id=None: Fake() + ) + + return install + + def test_doctor_makes_no_call_without_probe(capsys, probe_client): probe_client(CLIError("must not be called")) code, data = run(capsys, "config", "doctor") @@ -212,3 +233,59 @@ def test_the_deployment_probe_says_it_verified_nothing(capsys, probe_client, mon assert entry["checked"] is False and entry["ok"] is None assert entry["resolved"] is True assert "NOT verified" in entry["detail"] + + +def test_the_platform_probe_verifies_the_key_and_names_the_organisation( + capsys, probe_client, platform_probe_client, monkeypatch +): + """`whoami` reads nothing but the key, so unlike a deployment it can be + checked for real -- and the organisation it resolves is the reason to + hold the key at all. + """ + probe_client({"quota": 1}) + platform_probe_client({"organization_id": "org_ABC123"}) + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + + _, data = run(capsys, "config", "doctor", "--probe") + + assert data["probe"]["platform"] == { + "checked": True, + "ok": True, + "organization_id": "org_ABC123", + "detail": "The key was accepted, and resolved to an organisation.", + } + + +def test_an_absent_platform_key_is_reported_not_failed(capsys, probe_client): + """A platform key is optional -- holding only a deployment key is the + common case -- so not having one must not decide the exit code. + """ + probe_client({"quota": 1}) + + code, data = run(capsys, "config", "doctor", "--probe") + + assert code == int(ExitCode.SUCCESS) + entry = data["probe"]["platform"] + assert entry["checked"] is False + assert entry["ok"] is None + + +def test_a_rejected_platform_key_fails_the_probe( + capsys, probe_client, platform_probe_client, monkeypatch +): + """A key that is set and wrong is a real misconfiguration, unlike one that + is simply absent.""" + probe_client({"quota": 1}) + platform_probe_client(CLIError("bad key", ExitCode.AUTH)) + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + + code = main(["-o", "json", "config", "doctor", "--probe"]) + report = json.loads(capsys.readouterr().out)["error"]["details"] + + assert code == int(ExitCode.GENERIC) + assert report["probe"]["platform"] == { + "checked": True, + "ok": False, + "detail": "bad key", + "exit_code": int(ExitCode.AUTH), + } From e224089b0b591ff4af9d5ba90630996c53a24365 Mon Sep 17 00:00:00 2001 From: pk-zipstack Date: Tue, 1 Sep 2026 21:47:54 +0530 Subject: [PATCH 2/4] UN-4016 [FIX] Send the platform key where the caller said, not where the default points Iteration 1 of unstract:remediation against PR #3. Nineteen findings across fifteen classes; one escalated (below). The headline defect had four faces and one cause. `config.py` stated a relationship in a comment -- "the same host as docstudio: one deployment serves both the platform API and the deployments it manages" -- and implemented it as a constant on the very next line. So `platform.base_url` ignored a profile written before the `platform` block existed, ignored `docstudio --base-url`, and left `--api-key` inert for `deployment ls`, sending an organisation-admin platform key to `us-central.unstract.com` after the operator had explicitly named their own host. A reviewer proved it by pointing `--base-url` at 127.0.0.1:9 and getting a real 401 back: a closed port cannot answer, so the request reached the SaaS default. `platform_base_url()` now falls back to the resolved docstudio host, which fixes all four sites at once. The security scan rates it 3/10 as a vulnerability -- vendor host, TLS, and `requests` strips the auth header across hosts -- so it lands as a correctness defect, not a leak. The other four High findings: - `_store_organisation` re-derived the profile ladder and dropped the `$UNSTRACT_PROFILE` tier, so the key resolved from one profile and the org was written into another; the next command then failed after a `whoami` that reported `saved: true`. It now takes `ResolvedConfig.active_profile`, the same chain every read uses. It also refuses to *create* a profile that is not in the file: `setdefault` was materialising a typo, permanently disarming the "Profile not found" guard so every later command silently resolved production defaults. - A failed config write threw away an identity the network call had already returned, exiting 1 ("check your disk") or 2 ("usage error") with `data: null`. `ExitCode.SAVE_FAILED` exists for exactly this and `poll.py` already uses it; the identity now reaches stdout in `details` either way. - `config init` wrote `api_key = "env:UNSTRACT_PLATFORM_KEY"` into every starter profile, and an `env:` reference to an unset variable is a `config doctor` problem -- so doctor exited 1 for every user without a platform key, which this PR's own comment calls the common case. The key is dropped from the starter blocks. - `core/platform.py` -- the only wire-facing new code -- had no test executing it at all, because every command test replaces the factory. Two mutations (breaking the whoami URL, forcing every listing to organisation "") left the suite green. `tests/test_platform.py` now exercises the real class; both mutations fail it. Also fixed: a 204 or non-dict body raised an AttributeError that matched no arm in `__main__`, so the caller got a traceback and no envelope -- the one thing this CLI promises never to do; `requests` transport errors (a scheme-less base_url, a proxy's HTML on a 200) reached the entry point's full-disk handler and were reported as "Check the path and disk."; `PlatformAPIError` folded up to 2KB of server body into `error.message`, which `emit_error` documents as a one-line summary; `--transport-timeout` was accepted on `deployment ls` and ignored (8.65s elapsed against a 1s flag), and `auth` had no such flag at all; `api_path_prefix` was hard-wired, so whoami and ls were unreachable on precisely the self-hosted installs the onprem-example profile caters to, while `clone` worked; `whoami --save` rewrote a discovered project-local `.unstract.toml`, dropping its comments and narrowing its mode; the write was invisible outside `-o json`; the 401 hint talked about deployments on a command that has none; and the README claimed a deployment key "runs one deployment", contradicted by three other statements including this file's own KEY_SOURCES. Every fix is mutation-checked: reverting it fails a named test, nine of nine. ESCALATED, needs a decision: `GET /api/v1/unstract/whoami/` is served only by Zipstack/unstract#2269, which is unmerged. Against any released Unstract the README's documented first command 404s, and `hint_for(404)` sends the reader after a resource id that does not exist. `config doctor --probe` inherits it and exits non-zero on a good setup. Whether this CLI ships before the backend, and what it should say when it does, is a release call rather than a fix. Findings: A B C D E F G H I J K L M N O(escalated) P Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM --- README.md | 2 +- src/unstract_cli/app.py | 12 +- src/unstract_cli/commands/platform_cmd.py | 107 +++++++++++--- src/unstract_cli/config.py | 27 ++-- src/unstract_cli/core/clients.py | 41 +++++- src/unstract_cli/core/errors.py | 11 +- src/unstract_cli/core/platform.py | 83 +++++++++-- tests/test_commands.py | 162 ++++++++++++++++++++- tests/test_discover.py | 33 ++++- tests/test_platform.py | 165 ++++++++++++++++++++++ 10 files changed, 594 insertions(+), 49 deletions(-) create mode 100644 tests/test_platform.py diff --git a/README.md b/README.md index 06a658e..6f81d10 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,7 @@ an `onprem-example` profile as a shape to copy for a self-hosted install — its host is a placeholder, and only the *active* profile is ever resolved. The two Unstract keys are not interchangeable and neither replaces the other. A -deployment key runs one deployment and cannot say which organisation it belongs +deployment key runs deployments and cannot say which organisation it belongs to; a platform key identifies the organisation and lists what is in it, and cannot run a deployment. `auth whoami` and `deployment ls` take the platform key; `deployment run` and `deployment status` take the deployment key. `org_id` lives diff --git a/src/unstract_cli/app.py b/src/unstract_cli/app.py index 03cbe1c..5775600 100644 --- a/src/unstract_cli/app.py +++ b/src/unstract_cli/app.py @@ -237,14 +237,24 @@ def deployment_group() -> None: @cli.group("auth") @_connection_options() +@click.option( + "--transport-timeout", + type=float, + default=None, + help="Seconds before a stalled connection is given up on. Unset means the " + "client's own default, which is 60.", +) @pass_context -def auth_group(ctx: Context, **overrides: str | None) -> None: +def auth_group( + ctx: Context, transport_timeout: float | None, **overrides: str | None +) -> None: """Identify the credential you are using. Its flags configure the platform key, which is the credential that knows which organisation it belongs to. A deployment key does not: it authenticates against the deployment it was minted for and never reaches this endpoint. """ + ctx.transport_timeout = transport_timeout ctx.override(PLATFORM, overrides) diff --git a/src/unstract_cli/commands/platform_cmd.py b/src/unstract_cli/commands/platform_cmd.py index b103190..cc1e827 100644 --- a/src/unstract_cli/commands/platform_cmd.py +++ b/src/unstract_cli/commands/platform_cmd.py @@ -2,8 +2,8 @@ Both authenticate with a platform key rather than a deployment key. The two credentials are not interchangeable and neither is going away: a deployment key -runs one deployment and cannot describe the account, a platform key describes -the account and lists what is in it but cannot run anything. +runs deployments and cannot describe the account, a platform key describes the +account and lists what is in it but cannot run anything. No OpenAPI spec is vendored for the platform API, so these declare their flags by hand rather than through `spec_options`. @@ -17,26 +17,60 @@ from unstract_cli.app import Context, auth_group, deployment_group, pass_context from unstract_cli.commands.common import finish -from unstract_cli.config import DOCSTUDIO, load_config, save_config +from unstract_cli.config import DOCSTUDIO, ConfigError, load_config, save_config from unstract_cli.core.clients import translated +from unstract_cli.core.errors import CLIError, ExitCode +from unstract_cli.core.output import diagnostic from unstract_cli.core.platform import organisation, platform_client -#: The fields a deployment listing shows. The server sends around fifteen per -#: row, including run histories; `--output table` wraps rather than truncates, -#: so the whole row is unreadable at a terminal. Narrowed here rather than -#: silently cut off downstream -- `--full` returns the rows as sent. +#: The fields a deployment listing shows. The server sends fifteen per row, +#: including run histories; `--output table` wraps rather than truncates, so the +#: whole row is unreadable at a terminal. Narrowed here rather than silently cut +#: off downstream -- `--full` returns the rows as sent. LISTING_FIELDS = ("api_name", "display_name", "id", "is_active", "api_endpoint") -def _store_organisation(obj: Any, org_id: str) -> dict[str, Any]: - """Write the resolved organisation into the active profile. +def _store_organisation(ctx: Context, org_id: str) -> dict[str, Any]: + """Write the resolved organisation into the profile the run is using. + + The profile name comes from `ResolvedConfig.active_profile`, which is the + same flag > env > file-default ladder every read uses. Re-deriving it here + is what dropped the `$UNSTRACT_PROFILE` tier, so the organisation was + written into a profile no later command read. It lands on the docstudio block because that is where every consumer reads it from -- deployment URLs and aliases both -- and a second copy under the platform block would be one more thing to keep in agreement. """ cfg = load_config() - name = getattr(obj, "profile", None) or cfg.default_profile or "cloud-us" + if cfg.is_project_local: + # A `.unstract.toml` found by walking up from the working directory is + # very likely committed. Rewriting it would replace a teammate's + # `org_id` with this caller's, drop every comment (the file is + # re-serialised, not patched) and narrow its mode to 0600 -- a dirty, + # mode-changed, semantically different tracked file, from a command + # named `whoami`. The config layer already declines to *trust* this + # file for credentials; declining to *write* it is the same judgement. + raise CLIError( + f"Refusing to write to the project-local config at {cfg.path}.", + ExitCode.USAGE, + hint="Rerun with --no-save, or name a file explicitly: " + f"`unstract --config config set docstudio org_id {org_id}`.", + ) + + name = ctx.config.active_profile or cfg.default_profile or "cloud-us" + if cfg.exists and cfg.profiles and name not in cfg.profiles: + # `setdefault` would create it. That is not a convenience: the profile + # lookup raises "Profile not found" for a typo today, and materialising + # the name silently disarms that check for every later command, which + # then resolves the built-in production defaults instead. + known = ", ".join(sorted(cfg.profiles)) or "none" + raise CLIError( + f"Profile {name!r} is not in {cfg.path}.", + ExitCode.USAGE, + hint=f"Known profiles: {known}. Create it with `config set` first.", + ) + cfg.profiles.setdefault(name, {}).setdefault(DOCSTUDIO, {})["org_id"] = org_id if not cfg.default_profile: cfg.default_profile = name @@ -66,14 +100,48 @@ def whoami(ctx: Context, save: bool) -> None: Exits 3 when the key is rejected, so a setup script can branch on it without reading the message. """ - client = platform_client(ctx.config) + client = platform_client(ctx.config, timeout=getattr(ctx, "transport_timeout", None)) with translated(endpoint="whoami"): identity = client.whoami() - meta = {"saved": False} - if save and (org_id := identity.get("organization_id")): - meta = {"saved": True, **_store_organisation(ctx, str(org_id))} - finish(ctx, identity, meta=meta) + if not save: + finish(ctx, identity, meta={"saved": False, "reason": "--no-save"}) + return + + org_id = identity.get("organization_id") + if not org_id: + # Distinguished from --no-save: the caller asked to store and there was + # nothing to store, which the next command will fail on. + diagnostic( + "warning: the platform API returned no organization_id; nothing was stored.", + quiet=ctx.quiet, + verbosity=ctx.verbosity, + ) + finish(ctx, identity, meta={"saved": False, "reason": "no organization_id"}) + return + + try: + written = _store_organisation(ctx, str(org_id)) + except (OSError, ConfigError) as exc: + # The read succeeded; only the convenience write failed. Losing the + # identity to a full disk would report a working key as a total failure, + # and SAVE_FAILED exists for exactly this shape. + raise CLIError( + f"Resolved the organisation but could not write it: {exc}", + ExitCode.SAVE_FAILED, + details=identity, + hint="`details` carries the identity; set $UNSTRACT_ORG_ID or run " + f"`unstract config set docstudio org_id {org_id}`.", + ) from exc + + # `meta` is not rendered by `-o table` or `-o raw`, so a human would + # otherwise see nothing about a file this command just wrote. + diagnostic( + f"wrote org_id={org_id} to profile {written['profile']!r} in {written['path']}", + quiet=ctx.quiet, + verbosity=ctx.verbosity, + ) + finish(ctx, identity, meta={"saved": True, **written}) @deployment_group.command("ls") @@ -93,7 +161,8 @@ def ls(ctx: Context, api_name: str | None, full: bool) -> None: Answers what a deployment is called, which is the one thing `deployment run` needs and the UI is the only other place to find. Authenticates with the - platform key, not the deployment key. + platform key, not the deployment key -- but takes the same `--base-url` as + its sibling commands, since one deployment serves both. \b Examples: @@ -101,7 +170,11 @@ def ls(ctx: Context, api_name: str | None, full: bool) -> None: unstract docstudio deployment ls --api-name invoice-parser unstract docstudio deployment ls --full """ - client = platform_client(ctx.config, organisation(ctx.config)) + client = platform_client( + ctx.config, + organisation(ctx.config), + timeout=getattr(ctx, "transport_timeout", None), + ) with translated(endpoint="api/deployment/"): rows = client.list_api_deployments(api_name=api_name) diff --git a/src/unstract_cli/config.py b/src/unstract_cli/config.py index 8b0eb60..bceea34 100644 --- a/src/unstract_cli/config.py +++ b/src/unstract_cli/config.py @@ -60,6 +60,11 @@ (DOCSTUDIO, "org_id"): ("UNSTRACT_ORG_ID",), (PLATFORM, "api_key"): ("UNSTRACT_PLATFORM_KEY",), (PLATFORM, "base_url"): ("UNSTRACT_BASE_URL",), + # `clone` takes this as --api-prefix because a self-hosted deployment can + # mount the Platform API somewhere other than api/v1. Without it, whoami + # and `deployment ls` are unreachable on exactly the installs the + # onprem-example profile below caters to. + (PLATFORM, "api_prefix"): ("UNSTRACT_API_PREFIX",), } @@ -79,9 +84,10 @@ def settings_for(product: str) -> tuple[str, ...]: """The settings a product actually has. - Products differ: `org_id` is a URL path segment for one and meaningless for - the other, and reporting a setting a user has no way to supply reads as a - misconfiguration they cannot fix. + Products differ: `org_id` is a setting only for `docstudio` -- llmwhisperer + has no organisation, and `platform` reads docstudio's -- and reporting a + setting a user has no way to supply reads as a misconfiguration they cannot + fix. """ return tuple(sorted(key for prod, key in ENV_VARS if prod == product)) @@ -553,10 +559,11 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "org_id": "", "api_key": "env:UNSTRACT_DEPLOYMENT_KEY", }, - PLATFORM: { - "base_url": DEFAULT_BASE_URLS[PLATFORM], - "api_key": "env:UNSTRACT_PLATFORM_KEY", - }, + # No `api_key` on purpose. A platform key is optional -- holding + # only a deployment key is the common case -- and an `env:` + # reference to an unset variable is a `config doctor` problem, + # which would exit 1 for every user who does not hold one. + PLATFORM: {"base_url": DEFAULT_BASE_URLS[PLATFORM]}, "deployments": {"example": {"api_name": "your-api-deployment-name"}}, }, "cloud-eu": { @@ -577,10 +584,8 @@ def starter_profiles() -> dict[str, dict[str, Any]]: "org_id": "", "api_key": "env:UNSTRACT_DEPLOYMENT_KEY", }, - PLATFORM: { - "base_url": "https://unstract.internal.example", - "api_key": "env:UNSTRACT_PLATFORM_KEY", - }, + # No `api_key` -- see the cloud-us block. + PLATFORM: {"base_url": "https://unstract.internal.example"}, }, } diff --git a/src/unstract_cli/core/clients.py b/src/unstract_cli/core/clients.py index 5375104..d8985fd 100644 --- a/src/unstract_cli/core/clients.py +++ b/src/unstract_cli/core/clients.py @@ -17,7 +17,14 @@ from contextlib import contextmanager from typing import Any -from requests.exceptions import ConnectionError, Timeout +from requests.exceptions import ( + ConnectionError, + InvalidSchema, + InvalidURL, + MissingSchema, + RequestException, + Timeout, +) from unstract.api_deployments.client import ( APIDeploymentsClient, APIDeploymentsClientException, @@ -140,17 +147,22 @@ def translated(endpoint: str | None = None) -> Iterator[None]: except APIDeploymentsClientException as exc: raise CLIError(str(exc), ExitCode.USAGE, endpoint=endpoint) from exc except PlatformAPIError as exc: + # `PlatformAPIError.__init__` appends "\n body: " to + # its own message, so `str(exc)` would put up to 2KB of server body into + # `error.message` -- which `emit_error` documents as a one-line summary + # -- and duplicate it into `details`. + message = str(exc).split("\n body:", 1)[0] # The Platform API client raises rather than returning a status, and # carries the response body on the exception. Untranslated it would # reach the entry point as an unexpected crash and print a traceback. if exc.status_code: raise error_from_status( int(exc.status_code), - str(exc), + message, details=exc.body, endpoint=endpoint, ) from exc - raise CLIError(str(exc), details=exc.body, endpoint=endpoint) from exc + raise CLIError(message, details=exc.body, endpoint=endpoint) from exc except Timeout as exc: raise CLIError( str(exc), @@ -174,6 +186,29 @@ def translated(endpoint: str | None = None) -> Iterator[None]: retryable=True, hint="Could not reach the service. Check the base URL and connectivity.", ) from exc + except RequestException as exc: + # Must sit after Timeout and ConnectionError, which are subclasses. + # + # The Platform client is a bare `requests.Session`: unlike the other two + # it wraps nothing itself, so a malformed base URL (`MissingSchema`, + # `InvalidURL`) or a 2xx carrying HTML from a proxy or SPA host + # (`JSONDecodeError`) arrives here raw. Every one of those subclasses + # `OSError`, so untranslated they were caught by the entry point's + # full-disk handler and rendered "Check the path and disk." -- the wrong + # subsystem, on the one message a user with a bad base URL most needs to + # be right. + usage = isinstance(exc, (MissingSchema, InvalidSchema, InvalidURL)) + raise CLIError( + str(exc), + ExitCode.USAGE if usage else ExitCode.SERVER_ERROR, + endpoint=endpoint, + hint=( + "Check `base_url` -- it needs a scheme, e.g. https://host." + if usage + else "The service answered, but not with JSON. Check that " + "`base_url` names the API rather than a proxy or web app." + ), + ) from exc def translating( diff --git a/src/unstract_cli/core/errors.py b/src/unstract_cli/core/errors.py index a36402a..0f1191a 100644 --- a/src/unstract_cli/core/errors.py +++ b/src/unstract_cli/core/errors.py @@ -37,8 +37,11 @@ class ExitCode(IntEnum): 401: ExitCode.AUTH, 403: ExitCode.AUTH, 404: ExitCode.NOT_FOUND, - # Only the deployment status endpoint answers 406; the whisper equivalent - # is a 400 whose body says so, which is prose we do not translate on. + # The deployment status endpoint is the one that answers 406 meaningfully; + # the whisper equivalent is a 400 whose body says so, which is prose we do + # not translate on. A 406 from anywhere else -- DRF returns one on content + # negotiation failure -- lands on this code too, and reads as a one-shot + # read that was already consumed. Narrow it if that ever becomes reachable. 406: ExitCode.ALREADY_CONSUMED, 408: ExitCode.TIMEOUT, 409: ExitCode.VALIDATION, @@ -246,8 +249,8 @@ def hint_for(status: int) -> str | None: return ( "The key was rejected. Keys are per-product: `unstract config " "doctor` reports which one resolved and from where. A key that " - "works elsewhere can still be rejected here -- it may not cover " - "this deployment, or may belong to another organisation." + "works elsewhere can still be rejected here -- it may be the " + "wrong kind for this command, or belong to another organisation." ) case 404: return ( diff --git a/src/unstract_cli/core/platform.py b/src/unstract_cli/core/platform.py index e5d8b06..91da53c 100644 --- a/src/unstract_cli/core/platform.py +++ b/src/unstract_cli/core/platform.py @@ -18,7 +18,12 @@ from unstract.clone.client import PlatformClient from unstract.clone.context import OrgEndpoint -from unstract_cli.config import DOCSTUDIO, PLATFORM, ResolvedConfig +from unstract_cli.config import ( + DEFAULT_BASE_URLS, + DOCSTUDIO, + PLATFORM, + ResolvedConfig, +) from unstract_cli.core.errors import CLIError, ExitCode #: The organisation `whoami` is called with. The endpoint carries no @@ -27,6 +32,15 @@ #: empty string at the call site. NO_ORGANISATION = "" +#: The built-in default. Used to tell "the caller said nothing" from "the +#: caller chose this host" -- `ResolvedConfig` returns the default for an +#: unset `base_url` rather than None, so the two are otherwise identical. +DEFAULT_PLATFORM_BASE_URL = DEFAULT_BASE_URLS[PLATFORM] + +#: The built-in default, used to tell "caller said nothing" from "caller chose +#: this host" -- returns the default rather than None. +DEFAULT_PLATFORM_BASE_URL = DEFAULT_BASE_URLS[PLATFORM] + class CLIPlatformClient(PlatformClient): """`PlatformClient` plus the organisation-less identity read.""" @@ -39,24 +53,68 @@ def whoami(self) -> dict[str, Any]: """ base = self.endpoint.base_url.rstrip("/") prefix = self.endpoint.api_path_prefix.strip("/") - return self._send("GET", f"{base}/{prefix}/unstract/whoami/", "whoami/") + body = self._send("GET", f"{base}/{prefix}/unstract/whoami/", "whoami/") + if not isinstance(body, dict): + # `_send` returns None on a 204 or an empty 2xx body, and whatever + # `resp.json()` decoded otherwise -- a list, for a misrouted host. + # Guarded here, where the shape is known, rather than at each + # consumer: unguarded, `body.get(...)` raises an AttributeError that + # matches no arm in `__main__`, so the caller gets a traceback and + # no envelope at all. + raise CLIError( + "The platform API did not return an identity.", + ExitCode.SERVER_ERROR, + details=body, + endpoint="whoami", + hint="Check that `base_url` names an Unstract deployment that " + "serves /unstract/whoami/.", + ) + return body + + +def platform_base_url(config: ResolvedConfig) -> str: + """Where the platform API lives. + + One deployment serves both the platform API and the deployments it manages, + so a caller who has said where docstudio is has already said where this is. + Resolving `platform.base_url` alone would ignore that: a profile written + before the `platform` block existed, and every `docstudio --base-url`, would + silently fall through to the built-in cloud default and send the key there. + """ + explicit = config.get(PLATFORM, "base_url", default=None) + if explicit is not None and explicit != DEFAULT_PLATFORM_BASE_URL: + return str(explicit) + if (shared := config.get(DOCSTUDIO, "base_url")) is not None: + return str(shared) + return str(config.require(PLATFORM, "base_url")) def platform_client( - config: ResolvedConfig, org_id: str | None = None + config: ResolvedConfig, + org_id: str | None = None, + *, + timeout: float | None = None, ) -> CLIPlatformClient: """Build a Platform API client from the resolved configuration. ``org_id`` is optional because `whoami` runs before one is known. Every other call needs it, and asks for it explicitly. """ - return CLIPlatformClient( - OrgEndpoint( - base_url=config.require(PLATFORM, "base_url"), - organization_id=org_id if org_id is not None else NO_ORGANISATION, - platform_key=config.require(PLATFORM, "api_key"), - ) + endpoint = OrgEndpoint( + base_url=platform_base_url(config), + organization_id=org_id if org_id is not None else NO_ORGANISATION, + platform_key=config.require(PLATFORM, "api_key"), + **( + {"api_path_prefix": prefix} + if (prefix := config.get(PLATFORM, "api_prefix")) is not None + else {} + ), ) + # `PlatformClient`'s own default is 60s per request, which `_paginate` + # spends per page. An interactive caller who asked for a bound gets it. + if timeout is not None: + return CLIPlatformClient(endpoint, timeout=int(timeout)) + return CLIPlatformClient(endpoint) def organisation(config: ResolvedConfig) -> str: @@ -77,4 +135,9 @@ def organisation(config: ResolvedConfig) -> str: ) -__all__ = ["CLIPlatformClient", "organisation", "platform_client"] +__all__ = [ + "CLIPlatformClient", + "organisation", + "platform_base_url", + "platform_client", +] diff --git a/tests/test_commands.py b/tests/test_commands.py index 96d9b94..edf6834 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -126,11 +126,14 @@ def install(**replies): client = FakeWhisper(**replies) client.built_with = {} - def build(config, org_id=None): + def build(config, org_id=None, *, timeout=None): # Resolving the key is what registers it for scrubbing, so the fake # factory has to do it too or the seam hides a production path. + # The signature tracks the real `platform_client` deliberately: a + # fixture that drifts from it passes while testing nothing. client.built_with["api_key"] = config.get(PLATFORM, "api_key") client.built_with["org_id"] = org_id + client.built_with["timeout"] = timeout return client monkeypatch.setattr(platform_cmd, "platform_client", build) @@ -1328,3 +1331,160 @@ def test_ls_without_an_organisation_says_how_to_get_one( assert code == int(ExitCode.USAGE) assert "whoami" in json.dumps(envelope(out)["error"]) + + +# --------------------------------------------------------------------------- # +# auth whoami — where it writes, and what happens when it cannot +# --------------------------------------------------------------------------- # + + +def _config_with(tmp_path, monkeypatch, text): + path = tmp_path / "config.toml" + path.write_text(text, encoding="utf-8") + monkeypatch.setenv("UNSTRACT_CONFIG", str(path)) + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + return path + + +def test_whoami_writes_to_the_profile_the_run_is_actually_using( + capsys, platform_client, monkeypatch, tmp_path +): + """Reads resolve through `active_profile` (flag > env > file default). + Re-deriving that chain here dropped the env tier, so the organisation was + written into a profile no later command reads -- and `deployment ls` then + failed immediately after a `whoami` reporting `saved: true`. + """ + path = _config_with( + tmp_path, + monkeypatch, + 'default_profile = "cloud-us"\n' + '[profiles.cloud-us.docstudio]\norg_id = ""\n' + '[profiles.cloud-eu.docstudio]\norg_id = ""\n', + ) + monkeypatch.setenv("UNSTRACT_PROFILE", "cloud-eu") + platform_client(whoami=IDENTITY) + + _, out, _ = run(capsys, "auth", "whoami") + + assert envelope(out)["meta"]["profile"] == "cloud-eu" + assert 'org_id = "org_ABC123"' in path.read_text().split("[profiles.cloud-eu")[1] + + +def test_whoami_refuses_to_invent_a_profile_that_does_not_exist( + capsys, platform_client, monkeypatch, tmp_path +): + """`setdefault` created it. That silently disarmed the "Profile not found" + guard for every later command, which then resolved the built-in production + defaults instead -- from a single typo, permanently. + """ + _config_with( + tmp_path, + monkeypatch, + 'default_profile = "cloud-us"\n[profiles.cloud-us.docstudio]\norg_id = ""\n', + ) + platform_client(whoami=IDENTITY) + + code, out, _ = run(capsys, "-p", "cloud-uss", "auth", "whoami") + + assert code == int(ExitCode.USAGE) + assert "cloud-uss" in json.dumps(envelope(out)["error"]) + + +def test_whoami_keeps_the_identity_when_the_write_fails( + capsys, platform_client, monkeypatch, tmp_path +): + """The read succeeded and only the convenience write failed. Losing the + identity to a full disk reported a working key as a total failure, on an + exit code that means "you invoked it wrong". + """ + _config_with( + tmp_path, + monkeypatch, + 'default_profile = "cloud-us"\n[profiles.cloud-us.docstudio]\norg_id = ""\n', + ) + platform_client(whoami=IDENTITY) + monkeypatch.setattr( + platform_cmd, + "save_config", + lambda *a, **k: (_ for _ in ()).throw(OSError(13, "nope")), + ) + + code, out, _ = run(capsys, "auth", "whoami") + error = envelope(out)["error"] + + assert code == int(ExitCode.SAVE_FAILED) + # Not full equality: `redact_value` masks any field whose *name* looks + # secret, and `key_name` matches -- so the identity reaches `details` with + # that one field starred out. The organisation is the part the caller needs + # in order to carry on without the write. + assert error["details"]["organization_id"] == IDENTITY["organization_id"] + assert error["details"]["key_name"] == "***REDACTED***" + + +def test_whoami_does_not_rewrite_a_discovered_project_config( + capsys, platform_client, monkeypatch, tmp_path +): + """A `.unstract.toml` found by walking up is very likely committed. Writing + it replaced a teammate's org_id, dropped every comment and narrowed the mode + -- from a command named `whoami`, with no flag asked for. + """ + project = tmp_path / "repo" + project.mkdir() + (project / ".unstract.toml").write_text( + '# hand written\ndefault_profile = "team"\n[profiles.team.docstudio]\norg_id = "org_TEAM"\n', + encoding="utf-8", + ) + monkeypatch.chdir(project) + monkeypatch.delenv("UNSTRACT_CONFIG", raising=False) + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") + platform_client(whoami=IDENTITY) + + code, out, _ = run(capsys, "auth", "whoami") + + assert code == int(ExitCode.USAGE) + assert "# hand written" in (project / ".unstract.toml").read_text() + assert "org_TEAM" in (project / ".unstract.toml").read_text() + + +def test_the_platform_key_never_reaches_a_stream( + capsys, platform_client, monkeypatch, tmp_path +): + """`translated()` attaches `PlatformAPIError.body` -- the server's own + response -- as `details`. If the far end echoes the key, that is the path it + would travel to stdout. + """ + _config_with(tmp_path, monkeypatch, 'default_profile = "cloud-us"\n') + monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-SUPERSECRET-0987654321") + platform_client( + whoami=PlatformAPIError( + "GET whoami/ returned 401", + status_code=401, + body='{"echoed": "pk-SUPERSECRET-0987654321"}', + ) + ) + + _, out, err = run(capsys, "auth", "whoami") + + assert "pk-SUPERSECRET-0987654321" not in out + assert "pk-SUPERSECRET-0987654321" not in err + + +def test_a_rejected_key_keeps_its_message_on_one_line( + capsys, platform_client, monkeypatch, tmp_path +): + """`PlatformAPIError` folds the body into its own string, so `str(exc)` put + up to 2KB of server response into `error.message` -- which `emit_error` + documents as a one-line summary -- and duplicated it into `details`. + """ + _config_with(tmp_path, monkeypatch, 'default_profile = "cloud-us"\n') + platform_client( + whoami=PlatformAPIError( + "GET whoami/ returned 401", status_code=401, body='{"m": "x"}' + ) + ) + + _, out, _ = run(capsys, "auth", "whoami") + error = envelope(out)["error"] + + assert "\n" not in error["message"] + assert error["details"] == '{"m": "x"}' diff --git a/tests/test_discover.py b/tests/test_discover.py index fb04ce6..200475e 100644 --- a/tests/test_discover.py +++ b/tests/test_discover.py @@ -14,6 +14,7 @@ from unstract_cli.__main__ import main from unstract_cli.commands import config_cmd +from unstract_cli.config import PLATFORM from unstract_cli.core.errors import CLIError, ExitCode from unstract_cli.core.output import CONTRACT_VERSION @@ -180,7 +181,14 @@ def whoami(self): return reply or {} monkeypatch.setattr( - config_cmd, "platform_client", lambda _config, org_id=None: Fake() + config_cmd, + "platform_client", + # Resolves the key like its sibling in test_commands, so the probe + # tests exercise the registration that feeds the scrubber. + lambda config, org_id=None, *, timeout=None: ( + config.get(PLATFORM, "api_key"), + Fake(), + )[1], ) return install @@ -289,3 +297,26 @@ def test_a_rejected_platform_key_fails_the_probe( "detail": "bad key", "exit_code": int(ExitCode.AUTH), } + + +def test_config_init_then_doctor_exits_zero_without_a_platform_key( + capsys, monkeypatch, tmp_path +): + """The property `test_an_absent_platform_key_is_reported_not_failed` claims, + checked the way a real user reaches it. + + That test runs with no config file. The starter profile written by + `config init` used to carry `api_key = "env:UNSTRACT_PLATFORM_KEY"`, and an + `env:` reference to an unset variable is a doctor *problem* -- so the test + passed while every user who ran the documented first command got exit 1. + """ + monkeypatch.setenv("UNSTRACT_CONFIG", str(tmp_path / "config.toml")) + monkeypatch.setenv("LLMWHISPERER_API_KEY", "lw-123456789") + monkeypatch.setenv("UNSTRACT_DEPLOYMENT_KEY", "dk-123456789") + monkeypatch.setenv("UNSTRACT_ORG_ID", "acme") + monkeypatch.delenv("UNSTRACT_PLATFORM_KEY", raising=False) + + assert main(["-o", "json", "config", "init"]) == int(ExitCode.SUCCESS) + capsys.readouterr() + + assert main(["-o", "json", "config", "doctor"]) == int(ExitCode.SUCCESS) diff --git a/tests/test_platform.py b/tests/test_platform.py new file mode 100644 index 0000000..92e5c65 --- /dev/null +++ b/tests/test_platform.py @@ -0,0 +1,165 @@ +"""The Platform API client itself, exercised rather than stubbed. + +Every command test replaces the client factory, which is the right seam for +asking what a command hands the client -- and it means nothing in the suite ever +constructs `CLIPlatformClient` or runs the URL it builds. Two mutations proved +that: dropping the `/unstract/` segment from `whoami`, and forcing every listing +to run against organisation `""`, both left the suite green. + +These tests close that. No network: `_send` is replaced with a recorder, which +is the boundary between "what we build" and "what requests does with it". +""" + +from __future__ import annotations + +import pytest +from unstract.clone.context import OrgEndpoint + +from unstract_cli.config import DOCSTUDIO, PLATFORM, ConfigFile, ResolvedConfig +from unstract_cli.core.errors import CLIError, ExitCode +from unstract_cli.core.platform import ( + CLIPlatformClient, + platform_base_url, + platform_client, +) + + +def _client(**kwargs): + return CLIPlatformClient( + OrgEndpoint( + base_url=kwargs.pop("base_url", "https://host.example"), + organization_id=kwargs.pop("organization_id", ""), + platform_key="pk-000000000000", + **kwargs, + ) + ) + + +def _resolved(profiles, **overrides): + return ResolvedConfig( + file=ConfigFile(profiles=profiles, default_profile="p", exists=True), + overrides=overrides, + ) + + +# --- the URL whoami builds ------------------------------------------------ + + +def test_whoami_asks_for_no_organisation() -> None: + """The parent's `_url` injects the organisation this call exists to find, + so `whoami` builds its own URL -- and nothing else checks that it does. + """ + client = _client() + seen = {} + client._send = lambda method, url, label, **kw: ( + seen.update(method=method, url=url) or {"organization_id": "acme"} + ) + + client.whoami() + + assert seen["url"] == "https://host.example/api/v1/unstract/whoami/" + assert seen["method"] == "GET" + + +def test_whoami_url_survives_a_trailing_slash_on_the_base() -> None: + client = _client(base_url="https://host.example/") + seen = {} + client._send = lambda m, url, label, **kw: seen.update(url=url) or {"a": 1} + + client.whoami() + + assert seen["url"] == "https://host.example/api/v1/unstract/whoami/" + + +@pytest.mark.parametrize("body", [None, [], [{"organization_id": "acme"}], "text"]) +def test_whoami_rejects_a_body_that_is_not_an_identity(body) -> None: + """`_send` returns None on a 204 or an empty 2xx, and whatever `resp.json()` + decoded otherwise. Unguarded, `body.get(...)` raises an AttributeError that + matches no arm in `__main__`, so the caller gets a traceback and no envelope + -- the one thing this CLI promises never to do. + """ + client = _client() + client._send = lambda *a, **kw: body + + with pytest.raises(CLIError) as excinfo: + client.whoami() + + assert excinfo.value.exit_code == ExitCode.SERVER_ERROR + + +# --- what the factory puts in the endpoint -------------------------------- + + +def test_the_factory_passes_the_organisation_through() -> None: + """`deployment ls` runs inside an organisation; `whoami` runs before one is + known. Forcing the org-less value for both left every test green. + """ + config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + + assert platform_client(config, "org_ABC").endpoint.organization_id == "org_ABC" + assert platform_client(config).endpoint.organization_id == "" + + +def test_the_factory_threads_a_timeout() -> None: + """`--transport-timeout` was accepted on `deployment ls` and ignored: the + parent's own default is 60s, spent per page. + """ + config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + + assert platform_client(config, timeout=3).timeout == 3 + + +# --- which host the key is sent to ---------------------------------------- + + +def test_the_platform_host_follows_docstudio_when_unset() -> None: + """A profile written before the `platform` block existed names only + docstudio's host. Resolving `platform.base_url` alone fell through to the + built-in cloud default and sent the key there. + """ + config = _resolved( + {"p": {DOCSTUDIO: {"base_url": "https://onprem.example"}}}, + ) + + assert platform_base_url(config) == "https://onprem.example" + + +def test_a_docstudio_base_url_flag_reaches_the_platform_call() -> None: + """`docstudio --base-url` records `docstudio.base_url`; `deployment ls` + reads the platform block. The flag was accepted and dropped. + """ + config = _resolved({"p": {}}, **{"docstudio.base_url": "https://flag.example"}) + + assert platform_base_url(config) == "https://flag.example" + + +def test_an_explicit_platform_host_still_wins() -> None: + config = _resolved( + { + "p": { + DOCSTUDIO: {"base_url": "https://docstudio.example"}, + PLATFORM: {"base_url": "https://platform.example"}, + } + } + ) + + assert platform_base_url(config) == "https://platform.example" + + +def test_a_self_hosted_api_prefix_reaches_the_endpoint() -> None: + """`clone` takes --api-prefix because a self-hosted deployment can mount the + Platform API elsewhere. Without threading it, whoami and `deployment ls` + were unreachable on exactly those installs, with no flag, env var or profile + key that could fix it. + """ + config = _resolved( + {"p": {PLATFORM: {"api_key": "pk-000000000000", "api_prefix": "unstract-api/v1"}}} + ) + + client = platform_client(config) + + assert client.endpoint.api_path_prefix == "unstract-api/v1" + seen = {} + client._send = lambda m, url, label, **kw: seen.update(url=url) or {"a": 1} + client.whoami() + assert seen["url"].endswith("/unstract-api/v1/unstract/whoami/") From 35a41f1ae6a2ed2b76d26992e7dd5ea59484604a Mon Sep 17 00:00:00 2001 From: pk-zipstack Date: Tue, 1 Sep 2026 22:08:06 +0530 Subject: [PATCH 3/4] UN-4016 [FIX] Stop the fixes from re-breaking what they fixed The first remediation round closed nine findings and opened three of comparable severity, two of them the inverse of the finding they fixed. - `platform_base_url` told "unset" from "chosen" by comparing the resolved value against the built-in default. Those are the same string, so a caller who named the SaaS host was read as having named nothing and redirected to docstudio's -- and `config init` writes that exact host into every profile, so it was the common case. `ResolvedConfig.get_explicit` now answers "did anyone actually name this?" by stopping before the defaults. - `--transport-timeout` was truncated with `int()`, so anything under a second reached urllib3 as 0 and died with a bare ValueError: a traceback and no envelope, which is the failure mode the previous round added a guard to eliminate. The float is passed through; a non-positive value is a usage error about a flag. - Refusing to rewrite a project-local `.unstract.toml` was raised past the SAVE_FAILED wrapper, so it exited 2 and discarded the identity -- on the CLI's documented first command, in any checkout holding the file the README blesses. Declining to save is now a successful call reporting `saved: false`. Also: the unknown-profile guard no longer fires on the "cloud-us" literal it invents when nothing names a profile; a profile typo returns the identity in `details` instead of dropping it; `docstudio --api-key` is refused by `deployment ls` rather than accepted, ignored, and then reported missing; and the duplicated `DEFAULT_PLATFORM_BASE_URL`, a self-contradicting 406 comment, an overstated api_prefix comment, a false `--transport-timeout` help string and a stale README block are corrected. Eight of nine fixes are mutation-pinned: reverting each fails a named test. The ninth -- reading docstudio's tier with `get_explicit` rather than `get` -- is an equivalent mutant while both default hosts are the same string, and is recorded as unpinnable rather than pinned by a test of that coincidence. The `--transport-timeout` wiring the previous round added was covered by nothing; deleting either call site left the suite green. It is covered now. 280 tests pass, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM --- README.md | 12 +- src/unstract_cli/app.py | 5 +- src/unstract_cli/commands/platform_cmd.py | 81 +++++++++++-- src/unstract_cli/config.py | 30 ++++- src/unstract_cli/core/errors.py | 2 +- src/unstract_cli/core/platform.py | 42 ++++--- tests/test_commands.py | 135 +++++++++++++++++++++- tests/test_platform.py | 75 +++++++++++- 8 files changed, 337 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 6f81d10..8fd7179 100644 --- a/README.md +++ b/README.md @@ -93,7 +93,6 @@ api_key = "env:UNSTRACT_DEPLOYMENT_KEY" [profiles.cloud-us.platform] base_url = "https://us-central.unstract.com" -api_key = "env:UNSTRACT_PLATFORM_KEY" [profiles.cloud-us.deployments.invoices] api_name = "invoice-parser" @@ -115,9 +114,14 @@ The two Unstract keys are not interchangeable and neither replaces the other. A deployment key runs deployments and cannot say which organisation it belongs to; a platform key identifies the organisation and lists what is in it, and cannot run a deployment. `auth whoami` and `deployment ls` take the platform key; -`deployment run` and `deployment status` take the deployment key. `org_id` lives -on the `docstudio` block either way — `auth whoami` writes the one it resolves -there, because that is where everything that needs it reads from. +`deployment run` and `deployment status` take the deployment key; `auth whoami` +and `deployment ls` take the platform key. `org_id` lives on the `docstudio` +block either way — `auth whoami` writes the one it resolves there, because that +is where everything that needs it reads from. + +`config init` deliberately leaves `platform.api_key` out of the block above, so +that a caller who only holds a deployment key is not told a platform key is +missing. Add the line, or set `$UNSTRACT_PLATFORM_KEY`, when you have one. A credential can be written into the file literally, but `env:VAR_NAME` indirection is what `config init` writes and what the examples use: the file diff --git a/src/unstract_cli/app.py b/src/unstract_cli/app.py index 5775600..72002af 100644 --- a/src/unstract_cli/app.py +++ b/src/unstract_cli/app.py @@ -218,8 +218,9 @@ def whisper_group(ctx: Context, **overrides: str | None) -> None: "--transport-timeout", type=float, default=None, - help="Seconds before a stalled connection is given up on. Unset means it " - "is not, which is what the client has always done.", + help="Seconds before a stalled connection is given up on. Unset means no " + "bound for `deployment run` and `status`, and the platform client's own " + "60s default for `deployment ls`.", ) @pass_context def docstudio_group( diff --git a/src/unstract_cli/commands/platform_cmd.py b/src/unstract_cli/commands/platform_cmd.py index cc1e827..9f20ed2 100644 --- a/src/unstract_cli/commands/platform_cmd.py +++ b/src/unstract_cli/commands/platform_cmd.py @@ -30,6 +30,21 @@ LISTING_FIELDS = ("api_name", "display_name", "id", "is_active", "api_endpoint") +class SaveDeclinedError(Exception): + """The organisation resolved, and storing it was deliberately skipped. + + Distinct from a write that *failed*: there is nothing to retry and nothing + is wrong. The call succeeded, so it exits 0 and reports the identity, with + `meta.saved` false and `reason` saying which rule declined -- the same shape + `--no-save` already produces. + """ + + def __init__(self, reason: str, hint: str) -> None: + super().__init__(reason) + self.reason = reason + self.hint = hint + + def _store_organisation(ctx: Context, org_id: str) -> dict[str, Any]: """Write the resolved organisation into the profile the run is using. @@ -51,24 +66,41 @@ def _store_organisation(ctx: Context, org_id: str) -> dict[str, Any]: # mode-changed, semantically different tracked file, from a command # named `whoami`. The config layer already declines to *trust* this # file for credentials; declining to *write* it is the same judgement. - raise CLIError( - f"Refusing to write to the project-local config at {cfg.path}.", - ExitCode.USAGE, - hint="Rerun with --no-save, or name a file explicitly: " - f"`unstract --config config set docstudio org_id {org_id}`.", + raise SaveDeclinedError( + f"the config at {cfg.path} is project-local", + hint="Nothing was written. Rerun with --no-save to silence this, " + "or store it elsewhere: `unstract --config config set " + f"docstudio org_id {org_id}`.", ) - name = ctx.config.active_profile or cfg.default_profile or "cloud-us" + selected = ctx.config.active_profile or cfg.default_profile + if selected is None and cfg.exists and cfg.profiles: + # Neither the caller nor the file named one, so the "cloud-us" literal + # below is this function's own invention -- refusing under that name + # would quote a profile the caller never typed, and advising `config + # set` would create a third one that shadows theirs as the new default. + if len(cfg.profiles) == 1: + selected = next(iter(cfg.profiles)) + else: + known = ", ".join(sorted(cfg.profiles)) + raise ConfigError( + f"no profile is selected and {cfg.path} names no default " + f"(known profiles: {known}); rerun with `-p `" + ) + + name = selected or "cloud-us" if cfg.exists and cfg.profiles and name not in cfg.profiles: # `setdefault` would create it. That is not a convenience: the profile # lookup raises "Profile not found" for a typo today, and materialising # the name silently disarms that check for every later command, which # then resolves the built-in production defaults instead. + # + # Raised as `ConfigError` so the caller's SAVE_FAILED wrapper carries + # the identity back: the key was resolved, only the note-taking failed. known = ", ".join(sorted(cfg.profiles)) or "none" - raise CLIError( - f"Profile {name!r} is not in {cfg.path}.", - ExitCode.USAGE, - hint=f"Known profiles: {known}. Create it with `config set` first.", + raise ConfigError( + f"profile {name!r} is not in {cfg.path} " + f"(known profiles: {known}); create it with `config set` first" ) cfg.profiles.setdefault(name, {}).setdefault(DOCSTUDIO, {})["org_id"] = org_id @@ -122,6 +154,18 @@ def whoami(ctx: Context, save: bool) -> None: try: written = _store_organisation(ctx, str(org_id)) + except SaveDeclinedError as exc: + # The identity is what was asked for; the write was a convenience this + # config layout declines. Reporting the whole command as a usage error + # would fail the CLI's documented first command in any checkout holding + # a committed `.unstract.toml`, and throw the identity away with it. + diagnostic( + f"note: org_id was not stored -- {exc.reason}. {exc.hint}", + quiet=ctx.quiet, + verbosity=ctx.verbosity, + ) + finish(ctx, identity, meta={"saved": False, "reason": exc.reason}) + return except (OSError, ConfigError) as exc: # The read succeeded; only the convenience write failed. Losing the # identity to a full disk would report a working key as a total failure, @@ -170,6 +214,21 @@ def ls(ctx: Context, api_name: str | None, full: bool) -> None: unstract docstudio deployment ls --api-name invoice-parser unstract docstudio deployment ls --full """ + if ctx.config.overrides.get(f"{DOCSTUDIO}.api_key") is not None: + # `--api-key` on the docstudio group means a *deployment* key, and this + # command authenticates with a platform key. Honouring it would send a + # deployment key to the platform API; ignoring it silently and then + # reporting the platform key as missing is what shipped, and reads as a + # broken flag rather than the wrong credential. + raise CLIError( + "`--api-key` on `docstudio` is a deployment key; " + "`deployment ls` authenticates with a platform key.", + ExitCode.USAGE, + hint="Drop the flag and set $UNSTRACT_PLATFORM_KEY, or add " + "`api_key` to the [profiles..platform] block. A deployment " + "key runs a deployment; a platform key describes the account.", + ) + client = platform_client( ctx.config, organisation(ctx.config), @@ -183,4 +242,4 @@ def ls(ctx: Context, api_name: str | None, full: bool) -> None: finish(ctx, {"results": rows}, meta={"count": len(rows)}) -__all__ = ["ls", "whoami"] +__all__ = ["SaveDeclinedError", "ls", "whoami"] diff --git a/src/unstract_cli/config.py b/src/unstract_cli/config.py index bceea34..916a3f2 100644 --- a/src/unstract_cli/config.py +++ b/src/unstract_cli/config.py @@ -61,9 +61,10 @@ (PLATFORM, "api_key"): ("UNSTRACT_PLATFORM_KEY",), (PLATFORM, "base_url"): ("UNSTRACT_BASE_URL",), # `clone` takes this as --api-prefix because a self-hosted deployment can - # mount the Platform API somewhere other than api/v1. Without it, whoami - # and `deployment ls` are unreachable on exactly the installs the - # onprem-example profile below caters to. + # mount the Platform API somewhere other than api/v1. `OrgEndpoint` already + # defaults to api/v1, which standard installs serve; this is what reaches + # the ones that remount it, where whoami and `deployment ls` are otherwise + # unreachable. (PLATFORM, "api_prefix"): ("UNSTRACT_API_PREFIX",), } @@ -394,7 +395,23 @@ def get(self, product: str, key: str, default: Any = None) -> Any: remember_secret(value) return value - def _resolve(self, product: str, key: str, default: Any = None) -> Any: + def get_explicit(self, product: str, key: str) -> Any: + """Resolve through **flag > env > profile** only, stopping before defaults. + + `get` cannot answer "did anyone actually name this?" -- it returns + `DEFAULT_BASE_URLS[product]` for an unset `base_url`, so a caller who + deliberately named the default host and one who named nothing come back + as the same string. Anything that must treat those two differently asks + here instead of comparing the answer against the default, which reads + the caller's own choice as silence. + """ + value = self._explicit(product, key) + if key == "api_key": + remember_secret(value) + return value + + def _explicit(self, product: str, key: str) -> Any: + """The tiers a human supplied: flag, then environment, then profile.""" if (value := self.overrides.get(f"{product}.{key}")) is not None: return value if (value := self.overrides.get(key)) is not None: @@ -404,7 +421,10 @@ def _resolve(self, product: str, key: str, default: Any = None) -> Any: if value := os.environ.get(env_var): return value - if (value := _deref(self._product_block(product).get(key))) is not None: + return _deref(self._product_block(product).get(key)) + + def _resolve(self, product: str, key: str, default: Any = None) -> Any: + if (value := self._explicit(product, key)) is not None: return value if default is not None: diff --git a/src/unstract_cli/core/errors.py b/src/unstract_cli/core/errors.py index 0f1191a..89d8cce 100644 --- a/src/unstract_cli/core/errors.py +++ b/src/unstract_cli/core/errors.py @@ -41,7 +41,7 @@ class ExitCode(IntEnum): # the whisper equivalent is a 400 whose body says so, which is prose we do # not translate on. A 406 from anywhere else -- DRF returns one on content # negotiation failure -- lands on this code too, and reads as a one-shot - # read that was already consumed. Narrow it if that ever becomes reachable. + # read that was already consumed. 406: ExitCode.ALREADY_CONSUMED, 408: ExitCode.TIMEOUT, 409: ExitCode.VALIDATION, diff --git a/src/unstract_cli/core/platform.py b/src/unstract_cli/core/platform.py index 91da53c..b0ff064 100644 --- a/src/unstract_cli/core/platform.py +++ b/src/unstract_cli/core/platform.py @@ -19,7 +19,6 @@ from unstract.clone.context import OrgEndpoint from unstract_cli.config import ( - DEFAULT_BASE_URLS, DOCSTUDIO, PLATFORM, ResolvedConfig, @@ -32,15 +31,6 @@ #: empty string at the call site. NO_ORGANISATION = "" -#: The built-in default. Used to tell "the caller said nothing" from "the -#: caller chose this host" -- `ResolvedConfig` returns the default for an -#: unset `base_url` rather than None, so the two are otherwise identical. -DEFAULT_PLATFORM_BASE_URL = DEFAULT_BASE_URLS[PLATFORM] - -#: The built-in default, used to tell "caller said nothing" from "caller chose -#: this host" -- returns the default rather than None. -DEFAULT_PLATFORM_BASE_URL = DEFAULT_BASE_URLS[PLATFORM] - class CLIPlatformClient(PlatformClient): """`PlatformClient` plus the organisation-less identity read.""" @@ -80,11 +70,16 @@ def platform_base_url(config: ResolvedConfig) -> str: Resolving `platform.base_url` alone would ignore that: a profile written before the `platform` block existed, and every `docstudio --base-url`, would silently fall through to the built-in cloud default and send the key there. + + Both tiers are read with `get_explicit`, which stops before the built-in + defaults. Comparing `get`'s answer against the default instead would read a + caller who named the SaaS host as one who named nothing -- and `config init` + writes that exact host into every generated profile, so that is the common + case, not a corner of it. """ - explicit = config.get(PLATFORM, "base_url", default=None) - if explicit is not None and explicit != DEFAULT_PLATFORM_BASE_URL: - return str(explicit) - if (shared := config.get(DOCSTUDIO, "base_url")) is not None: + if (named := config.get_explicit(PLATFORM, "base_url")) is not None: + return str(named) + if (shared := config.get_explicit(DOCSTUDIO, "base_url")) is not None: return str(shared) return str(config.require(PLATFORM, "base_url")) @@ -112,9 +107,22 @@ def platform_client( ) # `PlatformClient`'s own default is 60s per request, which `_paginate` # spends per page. An interactive caller who asked for a bound gets it. - if timeout is not None: - return CLIPlatformClient(endpoint, timeout=int(timeout)) - return CLIPlatformClient(endpoint) + if timeout is None: + return CLIPlatformClient(endpoint) + if timeout <= 0: + # urllib3 raises a bare ValueError for a non-positive timeout, which + # matches no arm in `__main__` -- a traceback and no envelope. Rejected + # here, where it is still a usage error about a flag. + raise CLIError( + f"--transport-timeout must be greater than 0, not {timeout:g}.", + ExitCode.USAGE, + hint="Omit the flag to use the client's own 60s default.", + ) + # Passed as the float it was parsed as. `PlatformClient` annotates this + # `int`, but the annotation is not enforced and `requests` takes floats; + # truncating instead would send `--transport-timeout 0.5` as 0, which is + # the ValueError above, and silently round 1.9 down to 1. + return CLIPlatformClient(endpoint, timeout=timeout) def organisation(config: ResolvedConfig) -> str: diff --git a/tests/test_commands.py b/tests/test_commands.py index edf6834..7e30077 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -1386,8 +1386,57 @@ def test_whoami_refuses_to_invent_a_profile_that_does_not_exist( code, out, _ = run(capsys, "-p", "cloud-uss", "auth", "whoami") - assert code == int(ExitCode.USAGE) - assert "cloud-uss" in json.dumps(envelope(out)["error"]) + # SAVE_FAILED, not USAGE: the key resolved and only the note-taking failed, + # so the identity comes back in `details` rather than being discarded. + error = envelope(out)["error"] + assert code == int(ExitCode.SAVE_FAILED) + assert "cloud-uss" in json.dumps(error) + assert error["details"]["organization_id"] == IDENTITY["organization_id"] + + +def test_whoami_writes_to_the_only_profile_when_no_default_is_named( + capsys, platform_client, monkeypatch, tmp_path +): + """The unknown-profile guard fired on the literal "cloud-us" fallback -- a + name the caller never typed -- for any file with profiles and no + `default_profile`, and advised creating a third that would shadow theirs. + """ + path = _config_with( + tmp_path, + monkeypatch, + '[profiles.work.docstudio]\norg_id = ""\n', + ) + platform_client(whoami=IDENTITY) + + code, out, _ = run(capsys, "auth", "whoami") + + assert code == 0 + assert envelope(out)["meta"]["profile"] == "work" + written = path.read_text(encoding="utf-8") + assert f'org_id = "{IDENTITY["organization_id"]}"' in written + assert 'default_profile = "work"' in written + + +def test_whoami_will_not_guess_between_several_unselected_profiles( + capsys, platform_client, monkeypatch, tmp_path +): + """Two profiles and no default: writing into either would be a guess. It + says so and hands the identity back, rather than naming `cloud-us`. + """ + _config_with( + tmp_path, + monkeypatch, + '[profiles.work.docstudio]\norg_id = ""\n[profiles.home.docstudio]\norg_id = ""\n', + ) + platform_client(whoami=IDENTITY) + + code, out, _ = run(capsys, "auth", "whoami") + error = envelope(out)["error"] + + assert code == int(ExitCode.SAVE_FAILED) + assert "cloud-us" not in json.dumps(error) + assert "-p " in json.dumps(error) + assert error["details"]["organization_id"] == IDENTITY["organization_id"] def test_whoami_keeps_the_identity_when_the_write_fails( @@ -1439,13 +1488,91 @@ def test_whoami_does_not_rewrite_a_discovered_project_config( monkeypatch.setenv("UNSTRACT_PLATFORM_KEY", "pk-123") platform_client(whoami=IDENTITY) - code, out, _ = run(capsys, "auth", "whoami") + code, out, envelope_err = run(capsys, "auth", "whoami") + body = envelope(out) - assert code == int(ExitCode.USAGE) + # Declining the write is not failing the call: README blesses a committed + # `.unstract.toml`, and `auth whoami` is the documented first command, so + # exiting 2 broke the quickstart and discarded the identity with it. + assert code == 0 + assert body["data"]["organization_id"] == IDENTITY["organization_id"] + assert body["meta"]["saved"] is False + assert "project-local" in body["meta"]["reason"] assert "# hand written" in (project / ".unstract.toml").read_text() assert "org_TEAM" in (project / ".unstract.toml").read_text() +@pytest.mark.parametrize( + ("argv", "expected"), + [ + (["auth", "whoami"], None), + (["auth", "--transport-timeout", "12.5", "whoami"], 12.5), + (["docstudio", "deployment", "ls"], None), + (["docstudio", "--transport-timeout", "12.5", "deployment", "ls"], 12.5), + ], +) +def test_the_transport_timeout_flag_reaches_the_platform_client( + capsys, platform_client, monkeypatch, tmp_path, argv, expected +): + """The flag was accepted on both groups and threaded through the factory, + but nothing asserted the commands passed it: deleting either call site left + the suite green. The fixture recorded the value and no test read it. + """ + _config_with( + tmp_path, + monkeypatch, + 'default_profile = "cloud-us"\n[profiles.cloud-us.docstudio]\norg_id = "org_X"\n', + ) + client = platform_client(whoami=IDENTITY, list_api_deployments=_returns([])) + + code, _, _ = run(capsys, *argv) + + assert code == 0 + assert client.built_with["timeout"] == expected + + +@pytest.mark.parametrize("value", ["0", "0.0", "-1"]) +def test_a_non_positive_transport_timeout_is_a_usage_error_not_a_traceback( + capsys, monkeypatch, tmp_path, value +): + """urllib3 raises a bare `ValueError` for a non-positive timeout, which + matches no arm in `__main__` -- a traceback and no envelope. The flag is + `type=float`, so anything in (0, 1) truncated to that same 0. + """ + _config_with(tmp_path, monkeypatch, "") + + code, out, _ = run(capsys, "auth", "--transport-timeout", value, "whoami") + + assert code == int(ExitCode.USAGE) + assert "transport-timeout" in envelope(out)["error"]["message"] + + +def test_a_deployment_key_flag_is_refused_rather_than_ignored_by_ls( + capsys, platform_client, monkeypatch, tmp_path +): + """`--api-key` on the docstudio group is a *deployment* key and `ls` + authenticates with a platform key. It was accepted, dropped, and the + platform key then reported missing -- which reads as a broken flag rather + than the wrong credential. + """ + _config_with( + tmp_path, + monkeypatch, + 'default_profile = "cloud-us"\n[profiles.cloud-us.docstudio]\norg_id = "org_X"\n', + ) + platform_client(list_api_deployments=_returns([])) + + code, out, _ = run( + capsys, "docstudio", "--api-key", "dk-FROM-FLAG", "deployment", "ls" + ) + error = envelope(out)["error"] + + assert code == int(ExitCode.USAGE) + assert "platform key" in error["message"] + assert "UNSTRACT_PLATFORM_KEY" in error["hint"] + assert "dk-FROM-FLAG" not in json.dumps(envelope(out)) + + def test_the_platform_key_never_reaches_a_stream( capsys, platform_client, monkeypatch, tmp_path ): diff --git a/tests/test_platform.py b/tests/test_platform.py index 92e5c65..9ee58b7 100644 --- a/tests/test_platform.py +++ b/tests/test_platform.py @@ -15,7 +15,13 @@ import pytest from unstract.clone.context import OrgEndpoint -from unstract_cli.config import DOCSTUDIO, PLATFORM, ConfigFile, ResolvedConfig +from unstract_cli.config import ( + DEFAULT_BASE_URLS, + DOCSTUDIO, + PLATFORM, + ConfigFile, + ResolvedConfig, +) from unstract_cli.core.errors import CLIError, ExitCode from unstract_cli.core.platform import ( CLIPlatformClient, @@ -146,6 +152,73 @@ def test_an_explicit_platform_host_still_wins() -> None: assert platform_base_url(config) == "https://platform.example" +def test_the_saas_default_is_honoured_when_the_caller_names_it() -> None: + """The first fix compared the resolved value against + `DEFAULT_BASE_URLS[PLATFORM]` to tell "unset" from "chosen". Those are the + same string, so a caller who named the SaaS host was read as having named + nothing and silently redirected to docstudio's -- the inverse of the defect + it fixed. `config init` writes that exact host into every profile, so this + is the common shape, not a corner of it. + """ + profile = _resolved( + { + "p": { + DOCSTUDIO: {"base_url": "https://onprem.example"}, + PLATFORM: {"base_url": DEFAULT_BASE_URLS[PLATFORM]}, + } + } + ) + flag = _resolved( + {"p": {DOCSTUDIO: {"base_url": "https://onprem.example"}}}, + **{"platform.base_url": DEFAULT_BASE_URLS[PLATFORM]}, + ) + + assert platform_base_url(profile) == DEFAULT_BASE_URLS[PLATFORM] + assert platform_base_url(flag) == DEFAULT_BASE_URLS[PLATFORM] + + +def test_the_built_in_default_is_the_last_resort_not_a_veto() -> None: + """Nobody named a host anywhere: the built-in default is still the answer. + `get_explicit` stopping before the defaults must not lose that. + """ + assert platform_base_url(_resolved({"p": {}})) == DEFAULT_BASE_URLS[PLATFORM] + + +@pytest.mark.parametrize("value", [0, 0.0, -1]) +def test_a_non_positive_timeout_is_refused_before_urllib3_sees_it(value) -> None: + """urllib3 raises a bare `ValueError` for a non-positive timeout, which + matches no arm in `__main__`. Truncating the float with `int()` turned every + `--transport-timeout` under 1s into exactly that. + """ + config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + + with pytest.raises(CLIError) as caught: + platform_client(config, timeout=value) + + assert caught.value.exit_code == ExitCode.USAGE + + +def test_a_sub_second_timeout_survives_as_a_float() -> None: + """`int(0.5)` is 0, which is the ValueError above; `int(1.9)` is 1, which is + a bound the caller did not ask for. + """ + config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + + assert platform_client(config, timeout=0.5).timeout == 0.5 + assert platform_client(config, timeout=1.9).timeout == 1.9 + + +def test_the_api_prefix_env_var_is_wired(monkeypatch) -> None: + """The profile tier reads the block directly without consulting `ENV_VARS`, + so removing the entry left the suite green while `$UNSTRACT_API_PREFIX` did + nothing. + """ + monkeypatch.setenv("UNSTRACT_API_PREFIX", "unstract-api/v1") + config = _resolved({"p": {PLATFORM: {"api_key": "pk-000000000000"}}}) + + assert platform_client(config).endpoint.api_path_prefix == "unstract-api/v1" + + def test_a_self_hosted_api_prefix_reaches_the_endpoint() -> None: """`clone` takes --api-prefix because a self-hosted deployment can mount the Platform API elsewhere. Without threading it, whoami and `deployment ls` From ea035e1a03c75eb59a33663589cdb5106f5c3713 Mon Sep 17 00:00:00 2001 From: pk-zipstack Date: Tue, 1 Sep 2026 22:19:51 +0530 Subject: [PATCH 4/4] UN-4016 [FIX] Resolve the platform host by tier, not by product Greptile, on PR #3: with `platform.base_url` in the active profile, `docstudio --base-url` was silently ignored and the platform key went to the profile host. `config init` writes `platform.base_url` into every profile it generates, so this was every generated config, not a corner of one. The cause is that `platform_base_url` asked one product for all three of its tiers before asking the other, which inverts the precedence the config layer promises everywhere else: a *profile* value on the preferred product beat a *flag* on the sibling. It walks tier by tier across both products now, so flag > env > profile holds regardless of which product a value was written under; within a tier the platform block still wins as the specific answer. This is a regression the previous commit introduced. Its sentinel comparison happened to mask this path -- a generated `platform.base_url` equals the default, so the sentinel read it as unset and fell through to the flag -- and removing the sentinel to stop discarding a deliberately-named SaaS host exposed it. Both cases now pass for the same reason rather than trading off: the question is which tier named the host, not which product. `ResolvedConfig.explicit_tiers` yields rather than returning a tuple. Built eagerly it read the profile block even when a flag had already answered, which resolves the profile name and raises for one that does not exist -- failing a caller on a tier it never consulted. That is pinned by a test. Eleven resolution paths verified end to end, including the three Greptile named. 283 tests pass, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K92qFRNecJ5kNkKcmJ8qkM --- src/unstract_cli/config.py | 33 +++++++++++++++++-------- src/unstract_cli/core/platform.py | 31 +++++++++++++++++------- tests/test_platform.py | 40 +++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 19 deletions(-) diff --git a/src/unstract_cli/config.py b/src/unstract_cli/config.py index 916a3f2..82be023 100644 --- a/src/unstract_cli/config.py +++ b/src/unstract_cli/config.py @@ -19,6 +19,7 @@ import os import stat import tomllib +from collections.abc import Iterator from copy import deepcopy from dataclasses import dataclass, field from pathlib import Path @@ -410,18 +411,30 @@ def get_explicit(self, product: str, key: str) -> Any: remember_secret(value) return value - def _explicit(self, product: str, key: str) -> Any: - """The tiers a human supplied: flag, then environment, then profile.""" - if (value := self.overrides.get(f"{product}.{key}")) is not None: - return value - if (value := self.overrides.get(key)) is not None: - return value + def explicit_tiers(self, product: str, key: str) -> Iterator[Any]: + """What each tier says, in order -- flag, env, profile -- unset as `None`. - for env_var in ENV_VARS.get((product, key), ()): - if value := os.environ.get(env_var): - return value + For a setting two products share -- one deployment serves both, so + `base_url` is really one question asked twice -- picking a product first + and then walking its tiers inverts the precedence the whole config layer + promises: a profile value on the preferred product beats a *flag* on the + other. Walking tier by tier across both products keeps flag > env > + profile true regardless of which product a value was written under. - return _deref(self._product_block(product).get(key)) + Lazy on purpose: reading the profile block resolves the profile name, + which raises for one that does not exist. A caller answered by an + earlier tier must not be failed by a later one it never consulted. + """ + yield self.overrides.get(f"{product}.{key}", self.overrides.get(key)) + yield next( + (v for e in ENV_VARS.get((product, key), ()) if (v := os.environ.get(e))), + None, + ) + yield _deref(self._product_block(product).get(key)) + + def _explicit(self, product: str, key: str) -> Any: + """The tiers a human supplied: flag, then environment, then profile.""" + return next((v for v in self.explicit_tiers(product, key) if v is not None), None) def _resolve(self, product: str, key: str, default: Any = None) -> Any: if (value := self._explicit(product, key)) is not None: diff --git a/src/unstract_cli/core/platform.py b/src/unstract_cli/core/platform.py index b0ff064..a7dcae2 100644 --- a/src/unstract_cli/core/platform.py +++ b/src/unstract_cli/core/platform.py @@ -71,16 +71,29 @@ def platform_base_url(config: ResolvedConfig) -> str: before the `platform` block existed, and every `docstudio --base-url`, would silently fall through to the built-in cloud default and send the key there. - Both tiers are read with `get_explicit`, which stops before the built-in - defaults. Comparing `get`'s answer against the default instead would read a - caller who named the SaaS host as one who named nothing -- and `config init` - writes that exact host into every generated profile, so that is the common - case, not a corner of it. + The two products are walked **tier by tier**, not one product at a time. + Asking `platform` for all three tiers first would let a profile's + `platform.base_url` beat a `docstudio --base-url` flag -- and `config init` + writes `platform.base_url` into every profile it generates, so that would + silently ignore the flag for every generated config, which is the majority + of them. + + Each tier is read explicitly, stopping before the built-in defaults. + Comparing `get`'s answer against the default instead would read a caller who + named the SaaS host as one who named nothing -- the same `config init` + profiles again, from the other direction. """ - if (named := config.get_explicit(PLATFORM, "base_url")) is not None: - return str(named) - if (shared := config.get_explicit(DOCSTUDIO, "base_url")) is not None: - return str(shared) + for from_platform, from_docstudio in zip( + config.explicit_tiers(PLATFORM, "base_url"), + config.explicit_tiers(DOCSTUDIO, "base_url"), + strict=True, + ): + # Within one tier the platform block wins: it is the specific answer to + # this question, where docstudio's is the one inherited from the sibling. + if from_platform is not None: + return str(from_platform) + if from_docstudio is not None: + return str(from_docstudio) return str(config.require(PLATFORM, "base_url")) diff --git a/tests/test_platform.py b/tests/test_platform.py index 9ee58b7..1a8d2a8 100644 --- a/tests/test_platform.py +++ b/tests/test_platform.py @@ -177,6 +177,46 @@ def test_the_saas_default_is_honoured_when_the_caller_names_it() -> None: assert platform_base_url(flag) == DEFAULT_BASE_URLS[PLATFORM] +def test_a_docstudio_flag_beats_a_platform_host_in_the_profile() -> None: + """Greptile, on PR #3. Walking `platform`'s three tiers before docstudio's + let a *profile* value beat a *flag*, inverting the precedence the config + layer promises everywhere else -- and `config init` writes + `platform.base_url` into every profile it generates, so the flag was ignored + for every generated config, not a corner case. + """ + config = _resolved( + {"p": {PLATFORM: {"base_url": DEFAULT_BASE_URLS[PLATFORM]}}}, + **{"docstudio.base_url": "https://flag.example"}, + ) + + assert platform_base_url(config) == "https://flag.example" + + +def test_a_platform_flag_still_beats_a_docstudio_flag() -> None: + """Within one tier the specific product wins; across tiers it does not.""" + config = _resolved( + {"p": {}}, + **{ + "platform.base_url": "https://platform-flag.example", + "docstudio.base_url": "https://docstudio-flag.example", + }, + ) + + assert platform_base_url(config) == "https://platform-flag.example" + + +def test_an_environment_host_beats_a_profile_on_either_product() -> None: + """`$UNSTRACT_BASE_URL` maps to both products, and env outranks profile.""" + import os + + config = _resolved({"p": {PLATFORM: {"base_url": "https://profile.example"}}}) + os.environ["UNSTRACT_BASE_URL"] = "https://env.example" + try: + assert platform_base_url(config) == "https://env.example" + finally: + del os.environ["UNSTRACT_BASE_URL"] + + def test_the_built_in_default_is_the_last_resort_not_a_veto() -> None: """Nobody named a host anywhere: the built-in default is still the answer. `get_explicit` stopping before the defaults must not lose that.