diff --git a/cowork/api/v1/endpoints/settings.py b/cowork/api/v1/endpoints/settings.py index ac76f406..48cab027 100644 --- a/cowork/api/v1/endpoints/settings.py +++ b/cowork/api/v1/endpoints/settings.py @@ -403,6 +403,7 @@ def write_raw_settings(body: _RawSettingsBody, session: SessionDep, request: Req require_local(request) from cowork.migrations import sync_env_vars_to_db + from cowork.common.settings.env_boundary import atomic_write_env, _is_dotenv_safe incoming = _parse_dotenv_content(body.content) @@ -410,17 +411,23 @@ def write_raw_settings(body: _RawSettingsBody, session: SessionDep, request: Req existing = _read_env_dict() existing.update(incoming) - # Sync recognised ANTON_* vars to the DB first. If validation fails, - # leave the legacy .env untouched so the DB remains authoritative. - sync_env_vars_to_db(session, existing) - - _ENV_PATH.parent.mkdir(parents=True, exist_ok=True) - lines = [f"{k}={v}" for k, v in existing.items()] - _ENV_PATH.write_text("\n".join(lines) + "\n", encoding="utf-8") - try: - _ENV_PATH.chmod(0o600) - except OSError: - pass + # Sync ONLY the recognised vars actually in THIS request to the DB — never + # the whole merged .env. The server now mirrors DB->.env (ENG-1127), so the + # file can hold a preserved/translated cluster (a stale minds-cloud line, or + # a gemini role written as openai-compatible); re-syncing all of it would + # overwrite the authoritative DB choice from the CLI's derived file. If + # validation fails, leave the legacy .env untouched so the DB stays authoritative. + sync_env_vars_to_db(session, incoming) + + # Write through the same hardened path as the managed export (atomic + # replace, 0o600, transient-Windows-lock retry) instead of a bare + # write_text, and skip any newline-bearing var so this legacy writer can't + # smuggle a second line either (ENG-1127 review). + lines = [ + f"{k}={v}" for k, v in existing.items() + if _is_dotenv_safe(k) and _is_dotenv_safe(v) + ] + atomic_write_env(_ENV_PATH, "\n".join(lines) + "\n") except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) from e except Exception as e: diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py new file mode 100644 index 00000000..052d4002 --- /dev/null +++ b/cowork/common/settings/env_boundary.py @@ -0,0 +1,494 @@ +"""The ``.env`` <-> DB settings boundary, both directions in one place. + +The DB (``UserSettings``) is the source of truth; the standalone ``anton`` CLI +still reads its config from ``.env``, so ``.env`` is a trailing dependency the +server keeps in sync. Everything that knows an ``ANTON_*`` variable name lives +here — the alias map, provider-value normalization, and the two conversions — +so ``user_settings`` stays purely about the DB model. + +- inbound (``.env`` -> DB): ``env_to_db_updates`` maps + normalizes; the caller + (SettingService / migration) validates, encrypts, and writes. +- outbound (DB -> ``.env``): ``db_to_env`` formats the stored values, then + ``merge_env_lines`` + ``atomic_write_env`` persist them, preserving unmanaged lines. + +Model keys (planning_model / coding_model) are deliberately absent from the alias +map (ENG-739): a model is CLI-only and must never ride a bulk ``.env`` sync. +""" +from __future__ import annotations + +import errno +import logging +import os +import tempfile +import time +from enum import Enum +from pathlib import Path + +from pydantic import SecretStr + +from cowork.common.settings.user_settings import ( + Provider, + UserSettings, + provider_api_key_str, +) + +logger = logging.getLogger(__name__) + +# DB setting key -> its ANTON_* .env variable, for every field that overlaps +# between AntonSettings (.env) and UserSettings (DB). Single canonical map (was +# hand-maintained in two places that drifted — ENG-1125). +SETTING_ENV_ALIASES: dict[str, str] = { + "anthropic_api_key": "ANTON_ANTHROPIC_API_KEY", + "openai_api_key": "ANTON_OPENAI_API_KEY", + "openai_compatible_api_key": "ANTON_OPENAI_API_KEY_CUSTOM", + "gemini_api_key": "ANTON_GEMINI_API_KEY", + "minds_api_key": "ANTON_MINDS_API_KEY", + "planning_provider": "ANTON_PLANNING_PROVIDER", + "coding_provider": "ANTON_CODING_PROVIDER", + "router_provider": "ANTON_ROUTER_PROVIDER", + "minds_url": "ANTON_MINDS_URL", + "openai_base_url": "ANTON_OPENAI_BASE_URL", + "memory_enabled": "ANTON_MEMORY_ENABLED", + "memory_mode": "ANTON_MEMORY_MODE", + "episodic_memory": "ANTON_EPISODIC_MEMORY", + "proactive_dashboards": "ANTON_PROACTIVE_DASHBOARDS", + "act_first": "ANTON_ACT_FIRST", + "publish_url": "ANTON_PUBLISH_URL", +} + +# Inverse view (ANTON_* -> DB key) for the inbound (.env-first) callers. +ENV_ALIAS_TO_SETTING: dict[str, str] = {v: k for k, v in SETTING_ENV_ALIASES.items()} + +# The per-role model vars the outbound export owns. Absent from SETTING_ENV_ALIASES +# because the INBOUND (.env->DB) direction must never map a model (ENG-739: a +# stale ``latest:`` line must not re-pin the picker). OUTBOUND they ARE managed — +# a provider is exported WITH its resolved model so the CLI can never end up with +# a provider/model mismatch (ENG-1127 review); being managed also means a stale +# model line is dropped when its provider is cleared. +_MODEL_ENV_VARS: tuple[str, ...] = ( + "ANTON_PLANNING_MODEL", + "ANTON_CODING_MODEL", + "ANTON_ROUTER_MODEL", +) + +# The ANTON_* vars the outbound export owns; every other .env line is preserved. +# Includes the two key vars the export no longer WRITES (ANTON_GEMINI_API_KEY, +# ANTON_OPENAI_API_KEY_CUSTOM — the pinned CLI has no field for them; gemini/oc +# creds ride the OpenAI slot) so a stale line from an older writer is still +# reconciled away. +MANAGED_ENV_VARS: tuple[str, ...] = tuple(SETTING_ENV_ALIASES.values()) + _MODEL_ENV_VARS + + +def normalize_provider_value(val: str, *, minds_key_present: bool) -> str: + """A .env / UI provider string -> the DB ``Provider`` enum value. + + Hyphen->underscore canonicalization plus the "a Minds key is present, so + ``openai-compatible`` really means ``minds_cloud``" heuristic. The inverse + (DB -> UI/.env) is ``Provider.ui_value``. + """ + canonical = val.replace("-", "_") + if canonical == Provider.OPENAI_COMPATIBLE.value and minds_key_present: + return Provider.MINDS_CLOUD.value + return canonical + + +# ── inbound: .env -> DB ─────────────────────────────────────────────── + +def env_to_db_updates(dotenv: dict[str, str]) -> dict[str, str]: + """A parsed ``.env`` dict -> ``{db_key: value}`` ready for the DB. + + Maps ANTON_* names, skips absent/empty vars, normalizes provider fields. + Pure conversion — validation, encryption and the DB write stay in the caller. + """ + updates: dict[str, str] = {} + for env_var, setting_key in ENV_ALIAS_TO_SETTING.items(): + val = dotenv.get(env_var) + if not val: + continue + if setting_key.endswith("_provider"): + val = normalize_provider_value( + val, minds_key_present=bool(dotenv.get("ANTON_MINDS_API_KEY")) + ) + updates[setting_key] = val + return updates + + +# ── outbound: DB -> .env ────────────────────────────────────────────── + +def _env_str(value: object) -> str: + """A loaded ``UserSettings`` value -> its ``.env`` string. + + Providers use the dash form the CLI expects (``Provider.ui_value``); secrets + are the decrypted plaintext; booleans are lowercased. + """ + if isinstance(value, SecretStr): + return value.get_secret_value() + if isinstance(value, Provider): + return value.ui_value + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, Enum): + return str(value.value) + return str(value) + + +def _is_dotenv_safe(value: str) -> bool: + """A value is safe to write as ``VAR=value`` only if it stays on one line. + + A CR/LF in the value would terminate the assignment and turn the remainder + into a *new* line — e.g. ``minds_url = "https://x\\nDATABASE_URI=…"`` injects + an unmanaged ``DATABASE_URI`` that survives every later merge and is consumed + on the next CLI/server start. The exported fields (keys, URLs, providers, + booleans) never legitimately contain a newline, so we reject rather than + quote — a poisoned value is dropped, not smuggled into the file. + """ + return "\n" not in value and "\r" not in value + + +# The non-provider settings still export by a straight present-gated alias: +# booleans and the publish URL have no cross-field resolution. The provider / +# key / base-url / model cluster is rendered separately (see db_to_env) because +# it needs the SAME resolution the CLI and build_llm_client apply. +_OUTBOUND_FLAG_ALIASES: dict[str, str] = { + "memory_enabled": "ANTON_MEMORY_ENABLED", + "memory_mode": "ANTON_MEMORY_MODE", + "episodic_memory": "ANTON_EPISODIC_MEMORY", + "proactive_dashboards": "ANTON_PROACTIVE_DASHBOARDS", + "act_first": "ANTON_ACT_FIRST", + "publish_url": "ANTON_PUBLISH_URL", +} + +# The flag vars vs the provider/model/key/base cluster within MANAGED_ENV_VARS. +# The cluster is preserved as a group when a config can't be represented for the +# CLI, so an unrepresentable save doesn't wipe a valid config — see env_reconcile_vars. +_FLAG_ENV_VARS: tuple[str, ...] = tuple(_OUTBOUND_FLAG_ALIASES.values()) + + +def _anton_provider_name(p: Provider) -> str: + """A cowork ``Provider`` -> the provider name Anton understands ON DISK. + + Anton has no first-class gemini provider: its own ``anton setup`` writes + gemini as ``openai-compatible`` + Google's base URL + the key in the OpenAI + slot, and ``LLMClient.from_settings`` maps ``minds-cloud`` -> openai-compatible + itself. So gemini is translated here; everything else is its kebab ui_value. + """ + return "openai-compatible" if p is Provider.GEMINI else p.ui_value + + +def _openai_slot_demand(settings: UserSettings, p: Provider) -> tuple[str, str | None] | None: + """The ``(key, base)`` a provider needs in Anton's shared OpenAI slot. + + ``None`` only for anthropic, which uses its own dedicated key slot. Everything + else — openai / openai-compatible / gemini AND minds-cloud — runs through + Anton's single ``ANTON_OPENAI_API_KEY`` / ``ANTON_OPENAI_BASE_URL`` pair, so + their demands must AGREE for a config to be representable (see + ``_env_representable``). minds-cloud is included because we export its OpenAI + slot EXPLICITLY rather than trust Anton's ``model_post_init`` derivation, which + only fires for a planning/coding openai-compatible role, never router-only + (ENG-1127 review). + """ + if p is Provider.ANTHROPIC: + return None + from cowork.services.providers import minds_chat_base_url, provider_base_url # lazy: avoid cycle + if p is Provider.MINDS_CLOUD: + return ( + provider_api_key_str(settings, p), + minds_chat_base_url(settings.minds_url) if settings.minds_url else None, + ) + return ( + provider_api_key_str(settings, p), + provider_base_url(p.ui_value, openai_base_url=settings.openai_base_url or ""), + ) + + +def _env_representable(settings: UserSettings, providers: list[Provider]) -> bool: + """Whether the resolved per-role ``providers`` fit Anton's on-disk model. + + Anton has ONE global ``ANTON_OPENAI_API_KEY`` / ``ANTON_OPENAI_BASE_URL`` pair, + handed to BOTH its openai and openai-compatible factories (and minds-cloud maps + to openai-compatible). So a config is representable only when every provider + that rides that shared slot agrees on the same ``(key, base)`` — e.g. + planning=OpenAI + coding=Gemini, or MindsHub + OpenAI, cannot be represented + and would silently misroute one role's key to the other's endpoint (ENG-1127 + review). anthropic uses an independent slot, so it never conflicts. + """ + openai_demands = {d for p in providers if (d := _openai_slot_demand(settings, p)) is not None} + return len(openai_demands) <= 1 + + +def _emit_provider_creds(out: dict[str, str], settings: UserSettings, p: Provider) -> None: + """Write ``p``'s API key (and base URL) into the ANTON_* slots the CLI reads. + + Only called for a representable set (see ``_env_representable``), so the + shared OpenAI slot is never contended. + """ + from cowork.services.providers import minds_chat_base_url, provider_base_url # lazy: avoid cycle + + key = provider_api_key_str(settings, p) + if p is Provider.ANTHROPIC: + if key: + out["ANTON_ANTHROPIC_API_KEY"] = key + elif p is Provider.MINDS_CLOUD: + # Minds runs as openai-compatible. Export the OpenAI slot EXPLICITLY — + # Anton's model_post_init only derives it for a planning/coding oc role, + # so a router-only Minds role would otherwise build with no key/base + # (ENG-1127 review). Keep the minds_* slots too for Anton's other minds + # features (datasources, etc.). + if key: + out["ANTON_MINDS_API_KEY"] = key + out["ANTON_OPENAI_API_KEY"] = key + if settings.minds_url: + out["ANTON_MINDS_URL"] = settings.minds_url + base = minds_chat_base_url(settings.minds_url) + if base: + out["ANTON_OPENAI_BASE_URL"] = base + else: # OPENAI / OPENAI_COMPATIBLE / GEMINI — all via the OpenAI slot + if key: + out["ANTON_OPENAI_API_KEY"] = key + base = provider_base_url(p.ui_value, openai_base_url=settings.openai_base_url or "") + if base: + out["ANTON_OPENAI_BASE_URL"] = base + + +def _keyed_unique_providers(settings: UserSettings) -> list[Provider]: + """The resolved planning/coding/router providers that have a key, deduped and + planning-first. The unit both the export and the reconcile decision reason over. + """ + unique: list[Provider] = [] + for p in ( + settings.resolved_planning_provider, + settings.resolved_coding_provider, + settings.resolved_router_provider, + ): + if provider_api_key_str(settings, p) and p not in unique: + unique.append(p) + return unique + + +# Provider names (as written to ANTON_*_PROVIDER) that MUST ship an +# ANTON_OPENAI_BASE_URL. Without it, Anton's OpenAIProvider defaults to +# https://api.openai.com/v1/ and would send a Minds/custom key to OpenAI +# (ENG-1127 review). anthropic/openai are safe with no base — their SDK default +# host is the correct one. +_BASE_REQUIRED_PROVIDERS = frozenset({"openai-compatible", "minds-cloud"}) + + +def _provider_cluster(settings: UserSettings) -> dict[str, str] | None: + """The ANTON_* provider/model/key/base cluster to export, validated ATOMICALLY. + + Returns ``{}`` when nothing is configured (cleared — the caller reconciles the + cluster away), the full cluster when it's faithfully representable for the CLI, + or ``None`` when it is NOT — the caller then PRESERVES the existing ``.env`` + cluster instead of writing a partial/misrouting one. Unrepresentable means: the + roles don't fit Anton's single OpenAI slot; a base-requiring provider has no + base (else the key leaks to api.openai.com); or any value isn't dotenv-safe. + Validated as a UNIT — never field-by-field — because a provider+key without its + base is worse than exporting nothing (ENG-1127 review). + """ + unique = _keyed_unique_providers(settings) + if not unique: + return {} # nothing configured / cleared — safe to reconcile the cluster away + if not _env_representable(settings, unique): + return None + + roles = ( + ("ANTON_PLANNING_PROVIDER", "ANTON_PLANNING_MODEL", + settings.resolved_planning_provider, settings.resolved_planning_model), + ("ANTON_CODING_PROVIDER", "ANTON_CODING_MODEL", + settings.resolved_coding_provider, settings.resolved_coding_model), + ("ANTON_ROUTER_PROVIDER", "ANTON_ROUTER_MODEL", + settings.resolved_router_provider, settings.resolved_router_model), + ) + cluster: dict[str, str] = {} + for prov_var, model_var, prov, model in roles: + if not provider_api_key_str(settings, prov): + continue # resolved provider has no key → nothing runnable for this role + cluster[prov_var] = _anton_provider_name(prov) + if model: + cluster[model_var] = model + for prov in unique: + _emit_provider_creds(cluster, settings, prov) + + needs_base = any( + v in _BASE_REQUIRED_PROVIDERS for k, v in cluster.items() if k.endswith("_PROVIDER") + ) + if needs_base and not cluster.get("ANTON_OPENAI_BASE_URL"): + return None # would default to api.openai.com and leak the key + if any(not _is_dotenv_safe(v) for v in cluster.values()): + return None # a poisoned value taints the whole cluster + return cluster + + +def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: + """A loaded ``UserSettings`` -> the ``{ANTON_*: value}`` the CLI can RUN. + + Not a raw field dump: the provider / model / key / base cluster is rendered in + Anton's on-disk vocabulary via the SAME resolution the server's own + ``build_llm_client`` uses (``resolved_*`` + ``provider_base_url`` + + ``provider_api_key``). So a DB ``gemini`` provider exports as + ``openai-compatible`` + Google's base URL + the key in the OpenAI slot — the + shape the standalone CLI (and ``anton setup``) understands — instead of the + literal ``provider=gemini`` the pinned CLI rejects (ENG-1127 review). Each + role's provider is written WITH its resolved model so the pair is always + valid; a role whose resolved provider has no key exports nothing, so an + unconfigured or freshly-cleared install leaves the CLI on its own defaults. + + A mixed cross-role config the CLI cannot represent (planning=OpenAI + + coding=Gemini, MindsHub + OpenAI, …) exports NO provider cluster rather than + a silently-misrouting one — the product can run those via per-role clients, + but the standalone CLI has a single OpenAI slot (ENG-1127 review). + + The remaining settings (memory flags, publish URL) have no cross-field + resolution and export by a straight present-gated alias. + """ + out: dict[str, str] = {} + + cluster = _provider_cluster(settings) + if cluster is None: + # Not representable: leave the cluster out entirely. env_reconcile_vars + # also keeps it out of the merge's drop-set, so a previously-valid CLI + # config is preserved rather than wiped (ENG-1127 review). + logger.warning( + "settings: not exporting the .env provider cluster — the resolved config can't be " + "faithfully represented for the standalone CLI; preserving its existing config" + ) + else: + out.update(cluster) # {} when nothing is configured + + # Flags have no cross-field dependency, so they export (and CR/LF-guard) per + # field — dropping one can't misconfigure the others. + for db_key, env_var in _OUTBOUND_FLAG_ALIASES.items(): + if db_key not in present_keys: + continue + value = getattr(settings, db_key, None) + if value is None: + continue + text = _env_str(value) + if text and _is_dotenv_safe(text): + out[env_var] = text + elif text: + logger.warning("settings: refusing to export %s — value spans multiple lines", env_var) + + return out + + +def env_reconcile_vars(settings: UserSettings) -> tuple[str, ...]: + """The managed vars this export is authoritative for (drop-if-absent this run). + + When the provider config can't be represented for the CLI, only the flag vars + reconcile — the provider/model/key/base cluster is PRESERVED so an + unrepresentable save doesn't wipe a previously-valid CLI config. A genuinely + cleared config (no keys anywhere) yields an empty cluster (not ``None``), so it + still reconciles away — that is how a logout still wipes the CLI's credentials + (ENG-1127 review). Keyed off the exact same ``_provider_cluster`` decision the + export uses, so "wrote nothing" and "preserve" never disagree. + """ + if _provider_cluster(settings) is not None: + return MANAGED_ENV_VARS + return _FLAG_ENV_VARS + + +def merge_env_lines( + existing: str, managed: dict[str, str], reconcile: tuple[str, ...] = MANAGED_ENV_VARS +) -> str: + """Rewrite the ``reconcile`` vars in ``existing``, preserving everything else. + + A var is dropped from ``existing`` if it's in ``reconcile`` (so a cleared key + loses its line) OR is being (re-)written from ``managed`` (so there's never a + duplicate); every other line — unmanaged (auth token, comments) AND any + managed var outside ``reconcile`` this run — keeps its place. Callers narrow + ``reconcile`` (via ``env_reconcile_vars``) to preserve the provider cluster + when the config can't be represented for the CLI. + + Newline-bearing managed values are skipped as a serialization invariant so a + single assignment can never expand into a second (injected) one, even if a + caller hands in an unsanitized dict. + """ + drop_vars = set(reconcile) | set(managed) + drop = tuple(f"{var}=" for var in drop_vars) + kept = [ln for ln in existing.split("\n") if ln and not ln.startswith(drop)] + kept.extend( + f"{var}={value}" for var, value in managed.items() if _is_dotenv_safe(value) + ) + return "\n".join(kept) + "\n" + + +# Transient Windows share-mode locks (the CLI or a version-skewed server holding +# ``.env`` open, an AV scan, a delete-pending handle still closing) abort the +# rename with one of these — the exact EPERM class that wedged onboarding on the +# CLIENT before it grew a retry (ENG-1209). Now that the server is the writer +# (ENG-1127), the same hardening has to live here. POSIX has no mandatory +# locking, so these are effectively Windows-only. +_TRANSIENT_LOCK_ERRNOS = frozenset({errno.EPERM, errno.EACCES, errno.EBUSY, errno.ENOTEMPTY}) +_REPLACE_ATTEMPTS = 6 +_REPLACE_BASE_DELAY_S = 0.06 + +# Orphaned temps from a hard-kill / power-loss between the write and the rename +# hold the full plaintext key, so they must never linger; sweep only STALE ones +# so a concurrent writer's fresh in-flight temp is spared. +_STALE_TMP_S = 5 * 60 + + +def _is_transient_lock_error(exc: OSError) -> bool: + return exc.errno in _TRANSIENT_LOCK_ERRNOS + + +def _sweep_stale_temps(directory: Path) -> None: + """Remove orphaned ``.env.*.tmp`` files older than ``_STALE_TMP_S``. + + Only stale temps go — a live writer's temp is fresh and spared, so this can't + yank one out from under a concurrent rename. + """ + try: + now = time.time() + for entry in directory.glob(".env.*.tmp"): + try: + if now - entry.stat().st_mtime > _STALE_TMP_S: + entry.unlink() + except OSError: + pass # gone already or unreadable — best-effort + except OSError: + pass # dir unreadable — nothing to sweep + + +def _replace_with_retry(tmp: str, dest: str) -> None: + """``os.replace`` the finished temp onto ``dest``, retrying transient locks. + + The temp is already written to a fresh, unlocked path; only the rename + contends with a Windows share-mode lock, so that is all we retry — with a + widening backoff (~60ms..360ms, ~1.3s total) that mirrors the client's + ``retryOnTransientLock`` (ENG-1209). A non-lock error (ENOENT, ENOTDIR, a + genuinely unwritable target) rethrows at once. + """ + for attempt in range(_REPLACE_ATTEMPTS): + try: + os.replace(tmp, dest) + return + except OSError as exc: + if attempt < _REPLACE_ATTEMPTS - 1 and _is_transient_lock_error(exc): + time.sleep(_REPLACE_BASE_DELAY_S * (attempt + 1)) + continue + raise + + +def atomic_write_env(path: Path, content: str) -> None: + """Write ``content`` to ``path`` atomically, owner-only (0o600). + + Temp file + ``os.replace`` so a crash or a concurrent CLI read never sees a + truncated ``.env``; 0o600 because the file holds plaintext API keys. The + rename is retried on transient Windows share-mode locks (ENG-1209/ENG-1127). + """ + path.parent.mkdir(parents=True, exist_ok=True) + _sweep_stale_temps(path.parent) + fd, tmp = tempfile.mkstemp(dir=str(path.parent), prefix=".env.", suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(content) + os.chmod(tmp, 0o600) + _replace_with_retry(tmp, str(path)) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise diff --git a/cowork/common/settings/user_settings.py b/cowork/common/settings/user_settings.py index 1db08b3e..d2c00fcd 100644 --- a/cowork/common/settings/user_settings.py +++ b/cowork/common/settings/user_settings.py @@ -181,77 +181,6 @@ def _harness_options() -> list[str]: return available_harness_ids() -# ── .env ↔ DB setting aliases ──────────────────────────────────────── -# -# One canonical map of DB setting key → its ANTON_* .env variable name, for -# every field that overlaps between AntonSettings (.env, read by the standalone -# ``anton`` CLI) and UserSettings (DB). This is the single source the .env→DB -# seed (``migrations``), the ``POST /settings/raw`` merge sync, and the client -# all derive from. The same map was previously hand-maintained in both -# ``migrations._ENV_TO_SETTING`` and the client's ``syncSettings.ts`` — the two -# had already drifted (the client copy silently omitted gemini, the OpenAI- -# compatible key, router_provider, memory_enabled, act_first, proactive_ -# dashboards and publish_url), which is exactly the class of bug ENG-941/ENG-1125 -# retires. -# -# planning_model / coding_model are DELIBERATELY absent (ENG-739): a model in -# .env is CLI-only and must never ride a bulk .env→DB sync, or a login / -# token-refresh would re-pin a picker choice from a stale ``latest:`` line. -# -# max_tool_rounds / max_continuations are DELIBERATELY absent too, for the -# ENG-739 reason plus a harder failure mode: anton's own CoreSettings accepts -# any int, so a stale anton-CLI line like ANTON_MAX_TOOL_ROUNDS=1000 in the -# shared ~/.cowork/.env is valid for the CLI but fails UserSettings' bounds — -# and because sync_env_vars_to_db validates every mapped key and raises on the -# first failure, one such line would 400 every credential push / token -# refresh. Budgets enter the DB only via explicit writes (Settings UI / API). -# -# General inclusion rule: a key belongs here only if (a) every value anton's -# own settings accept for it is also valid for UserSettings, and (b) -# re-syncing a stale .env line can never override a choice the user made in -# the product. When in doubt, leave it out — .env lines still work for the -# standalone anton CLI. -SETTING_ENV_ALIASES: dict[str, str] = { - "anthropic_api_key": "ANTON_ANTHROPIC_API_KEY", - "openai_api_key": "ANTON_OPENAI_API_KEY", - "openai_compatible_api_key": "ANTON_OPENAI_API_KEY_CUSTOM", - "gemini_api_key": "ANTON_GEMINI_API_KEY", - "minds_api_key": "ANTON_MINDS_API_KEY", - "planning_provider": "ANTON_PLANNING_PROVIDER", - "coding_provider": "ANTON_CODING_PROVIDER", - "router_provider": "ANTON_ROUTER_PROVIDER", - "minds_url": "ANTON_MINDS_URL", - "openai_base_url": "ANTON_OPENAI_BASE_URL", - "memory_enabled": "ANTON_MEMORY_ENABLED", - "memory_mode": "ANTON_MEMORY_MODE", - "episodic_memory": "ANTON_EPISODIC_MEMORY", - "proactive_dashboards": "ANTON_PROACTIVE_DASHBOARDS", - "act_first": "ANTON_ACT_FIRST", - "publish_url": "ANTON_PUBLISH_URL", -} - -# Inverse view (ANTON_* .env var → DB setting key) for .env-first callers, i.e. -# the first-boot migration seed and the ``POST /settings/raw`` merge sync. -ENV_ALIAS_TO_SETTING: dict[str, str] = {v: k for k, v in SETTING_ENV_ALIASES.items()} - - -def normalize_provider_value(val: str, *, minds_key_present: bool) -> str: - """Translate a .env / UI provider string to the DB ``Provider`` enum value. - - The single home for the hyphen→underscore canonicalization plus the - "a Minds key is present, so ``openai-compatible`` really means - ``minds_cloud``" heuristic. This was reimplemented in - ``migrations._normalize_provider_value``, the client's ``syncSettings.ts``, - and ``settingsTransform.js`` — three copies that could disagree, silently - routing a provider to the wrong client (the inverse direction, DB→UI, is - ``Provider.ui_value``). - """ - canonical = val.replace("-", "_") - if canonical == Provider.OPENAI_COMPATIBLE.value and minds_key_present: - return Provider.MINDS_CLOUD.value - return canonical - - class UserSettings(Settings): # The recommended-model catalog and per-provider model defaults are # global, application-level config and live in app_settings @@ -586,8 +515,11 @@ def apply_model_defaults(self) -> 'UserSettings': self.coding_provider.value, CODING_MODEL_DEFAULTS, enabled_map ) if self.router_model is None: + # Default from the router's OWN provider — deriving from coding_provider + # paired an independently-set router (e.g. router=OpenAI, coding=Anthropic) + # with the wrong vendor's model (ENG-1127 review). self.router_model = _enabled_aware_default( - self.coding_provider.value, ROUTER_MODEL_DEFAULTS, enabled_map + self.router_provider.value, ROUTER_MODEL_DEFAULTS, enabled_map ) return self diff --git a/cowork/migrations.py b/cowork/migrations.py index 6259b64c..3c7d00ec 100644 --- a/cowork/migrations.py +++ b/cowork/migrations.py @@ -33,12 +33,8 @@ from anton.core.tools.skill_format import normalize_name, DESC_MAX, SKILL_FILE from cowork.common.paths import cowork_home -from cowork.common.settings import invalidate_user_settings_cache -from cowork.common.settings.user_settings import ( - ENV_ALIAS_TO_SETTING, - UserSettings, - normalize_provider_value, -) +from cowork.common.settings.env_boundary import ENV_ALIAS_TO_SETTING, env_to_db_updates +from cowork.common.settings.user_settings import UserSettings from cowork.models.setting import Setting from cowork.models.skill import META_CREATED_AT, META_DISPLAY_NAME, Skill, SkillLegacy from cowork.services.settings import SettingService @@ -55,7 +51,7 @@ # Map of .env keys -> DB setting keys for all fields that overlap between # AntonSettings (.env) and UserSettings (DB). Derived from the single canonical -# alias map in user_settings (SETTING_ENV_ALIASES) so this table can no longer +# alias map in env_boundary (SETTING_ENV_ALIASES) so this table can no longer # drift from the client's copy — see the note there, incl. why the model keys # (ANTON_PLANNING_MODEL / ANTON_CODING_MODEL) are deliberately excluded # (ENG-739). Drives both the first-boot .env→DB seed (migrate_env_to_db) and the @@ -80,18 +76,6 @@ def _parse_env_file() -> dict[str, str]: return result -def _normalize_provider_value(val: str, dotenv: dict[str, str]) -> str: - """Translate .env provider strings to DB enum values. - - Thin adapter over the canonical ``normalize_provider_value`` in - user_settings — kept so the .env-shaped call sites here don't each repeat - the "does this dotenv carry a Minds key" check. - """ - return normalize_provider_value( - val, minds_key_present=bool(dotenv.get("ANTON_MINDS_API_KEY")) - ) - - def sync_env_vars_to_db(session: Session, dotenv: dict[str, str]) -> list[str]: """Upsert a dict of ANTON_* env vars into the settings DB. @@ -99,14 +83,7 @@ def sync_env_vars_to_db(session: Session, dotenv: dict[str, str]) -> list[str]: keys that have no mapping in ``_ENV_TO_SETTING``. """ svc = SettingService(session) - updates: dict[str, str] = {} - for env_key, setting_key in _ENV_TO_SETTING.items(): - val = dotenv.get(env_key) - if not val: - continue - if setting_key.endswith("_provider"): - val = _normalize_provider_value(val, dotenv) - updates[setting_key] = val + updates = env_to_db_updates(dotenv) for key, value in updates.items(): svc._validate_key(key) @@ -135,17 +112,15 @@ def migrate_env_to_db(session: Session) -> bool: if dotenv: migrated_keys: list[str] = [] - for env_key, setting_key in _ENV_TO_SETTING.items(): - val = dotenv.get(env_key) - if not val: - continue - if setting_key.endswith("_provider"): - val = _normalize_provider_value(val, dotenv) + for setting_key, val in env_to_db_updates(dotenv).items(): try: - svc.upsert_setting(setting_key, val) + # export_env=False: this loop seeds the DB *from* .env; a per-key + # re-export mid-loop would strip the keys not migrated yet. The + # next real write exports the reconciled file (ENG-1127). + svc.upsert_setting(setting_key, val, export_env=False) migrated_keys.append(setting_key) except Exception as e: - logger.debug("Skipping env migration for %s: %s", env_key, e) + logger.debug("Skipping env migration for %s: %s", setting_key, e) if migrated_keys: logger.info( @@ -197,7 +172,10 @@ def backfill_minds_url(session: Session) -> bool: changed.append(key) if changed: session.commit() - invalidate_user_settings_cache() + # Route through the exporting hook (not a bare cache-invalidate) so the + # rewritten minds_url also reaches the CLI's .env — otherwise the DB moves + # to the canonical host while .env keeps the dead mdb.ai one (ENG-1127 review). + svc._after_write() logger.info( "Backfilled legacy MindsHub host (mdb.ai -> %s) in: %s", canonical, ", ".join(changed), diff --git a/cowork/services/settings.py b/cowork/services/settings.py index cdbb75d1..2d04542f 100644 --- a/cowork/services/settings.py +++ b/cowork/services/settings.py @@ -1,5 +1,6 @@ import json import logging +import threading from enum import Enum from cryptography.fernet import InvalidToken @@ -7,6 +8,14 @@ from sqlmodel import Session, select from cowork.common.encryption import decrypt, encrypt +from cowork.common.paths import cowork_home +from cowork.common.settings.app_settings import get_app_settings +from cowork.common.settings.env_boundary import ( + atomic_write_env, + db_to_env, + env_reconcile_vars, + merge_env_lines, +) from cowork.common.settings.user_settings import ( UserSettings, invalidate_user_settings_cache, @@ -16,6 +25,12 @@ logger = logging.getLogger(__name__) +# Serializes the .env export's read/merge/write so two concurrent settings writes +# can't lost-update the file — the last exporter re-reads the DB under the lock and +# installs the latest committed state (ENG-1127 review). In-process; the client's +# own .env writes go away in Phase B, making the server the sole writer. +_env_export_lock = threading.Lock() + def _mask_provider_keys(providers_json: str) -> str: """Return providers_json with each card's apiKey replaced by '***'. @@ -154,11 +169,45 @@ def _write_row(self, key: str, store_val: str) -> None: row.value = store_val self.session.add(row) - def upsert_setting(self, key: str, value: str) -> SettingResponse: + def _after_write(self, *, export_env: bool = True) -> None: + """Post-commit hook shared by the settings mutators. + + Invalidates the cache and (desktop install) mirrors the DB to the CLI's + ``.env`` (ENG-1127); ``export_env=False`` skips the mirror for the seeding migration. + """ + invalidate_user_settings_cache() + if export_env: + self._export_env_for_cli() + + def _export_env_for_cli(self) -> None: + """Mirror the DB's aliased settings to the CLI's ``.env`` (best-effort). + + Local (desktop) tenancy only — a cloud pod must not spill decrypted secrets + to disk (ENG-1127). Never raises — a stale ``.env`` must not fail a save. + """ + try: + if get_app_settings().tenancy_mode != "local": + return + # Serialize the whole read/merge/write. The DB read is INSIDE the lock, + # so the last exporter to acquire it installs the latest committed + # state — no lost update between concurrent settings writes. + with _env_export_lock: + rows = self._fetch_all_rows() + settings = self._load(rows) + managed = db_to_env(settings, {row.key for row in rows}) + path = cowork_home() / ".env" + existing = path.read_text(encoding="utf-8") if path.exists() else "" + content = merge_env_lines(existing, managed, env_reconcile_vars(settings)) + if content != existing: + atomic_write_env(path, content) + except Exception as exc: # noqa: BLE001 - export is best-effort + logger.warning("settings: .env export for the CLI failed: %s", exc) + + def upsert_setting(self, key: str, value: str, *, export_env: bool = True) -> SettingResponse: store_val, validated = self._encode_for_store(key, value) self._write_row(key, store_val) self.session.commit() - invalidate_user_settings_cache() + self._after_write(export_env=export_env) return self._to_response(key, validated, True) def save_all(self, updates: dict[str, str]) -> list[str]: @@ -180,7 +229,7 @@ def save_all(self, updates: dict[str, str]) -> list[str]: self._write_row(key, store_val) if encoded: self.session.commit() - invalidate_user_settings_cache() + self._after_write() return list(encoded.keys()) def bulk_upsert(self, updates: dict[str, str]) -> list[str]: @@ -228,7 +277,7 @@ def delete_setting(self, key: str) -> bool: return False self.session.delete(row) self.session.commit() - invalidate_user_settings_cache() + self._after_write() return True def clear_credentials(self) -> list[str]: @@ -261,6 +310,6 @@ def clear_credentials(self) -> list[str]: deleted.append(key) if deleted: self.session.commit() - invalidate_user_settings_cache() + self._after_write() return deleted diff --git a/tests/conftest.py b/tests/conftest.py index 200af336..d216ddbb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,6 +20,9 @@ # File bytes too — without this, any test using FileService writes into the # developer's real ~/.cowork/files/ and orphans dirs there. os.environ["COWORK_FILES_DIR"] = str(TMP / "files") +# Home dir too: a settings write now mirrors the DB to $COWORK_HOME/.env for the +# CLI (ENG-1127), so without this a test would rewrite the real ~/.cowork/.env. +os.environ["COWORK_HOME"] = str(TMP) os.environ["ENV"] = "test" import pytest diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py new file mode 100644 index 00000000..ce090f81 --- /dev/null +++ b/tests/test_env_boundary.py @@ -0,0 +1,486 @@ +"""ENG-1127 Phase A: the server exports the DB's settings to the CLI's .env. + +Pure derivation plus the SettingService integration (local-tenancy-only, +preserves unmanaged lines, migration never re-exports). +""" +import errno +import os +from types import SimpleNamespace + +import pytest + +import cowork.services.settings as settings_mod +from cowork.common.settings import env_boundary as eb +from cowork.common.settings.env_boundary import db_to_env, merge_env_lines +from cowork.services.providers import GEMINI_BASE_URL +from cowork.common.settings.user_settings import UserSettings +from cowork.db.session import get_open_session +from cowork.services.settings import SettingService + + +# ── pure derivation ──────────────────────────────────────────────────── + +def test_db_to_env_pairs_provider_with_its_model(): + # A provider is exported WITH its resolved model (ENG-1127 review): never a + # provider line without a matching model. + s = UserSettings(minds_api_key="sk-minds", minds_url="https://mdb.example", + planning_provider="minds_cloud", coding_provider="minds_cloud") + present = {"minds_api_key", "minds_url", "planning_provider", "coding_provider"} + out = db_to_env(s, present) + assert out["ANTON_PLANNING_PROVIDER"] == "minds-cloud" # dash form + assert out["ANTON_MINDS_API_KEY"] == "sk-minds" # decrypted plaintext + assert out["ANTON_MINDS_URL"] == "https://mdb.example" + assert out["ANTON_PLANNING_MODEL"] # model rides with the provider + assert out["ANTON_CODING_MODEL"] + + +def test_db_to_env_translates_gemini_to_openai_compatible(): + # The pinned CLI has no first-class gemini provider — it runs Gemini as + # openai-compatible + Google's base URL + the key in the OpenAI slot. The + # export must render that shape, not the literal provider=gemini the CLI rejects. + s = UserSettings(gemini_api_key="sk-gem", planning_provider="gemini", coding_provider="gemini") + out = db_to_env(s, present_keys={"gemini_api_key", "planning_provider", "coding_provider"}) + assert out["ANTON_PLANNING_PROVIDER"] == "openai-compatible" + assert out["ANTON_OPENAI_API_KEY"] == "sk-gem" # gemini key rides the OpenAI slot + assert out["ANTON_OPENAI_BASE_URL"] == GEMINI_BASE_URL # Google's OpenAI-compatible endpoint + assert "ANTON_GEMINI_API_KEY" not in out # a field the CLI ignores + assert "gemini" not in out.get("ANTON_PLANNING_PROVIDER", "") + + +def test_db_to_env_allows_representable_mixed_providers(): + # anthropic + gemini use INDEPENDENT Anton slots (anthropic key vs the OpenAI + # slot), so this mixed config IS representable and exports both roles. + s = UserSettings(anthropic_api_key="sk-an", gemini_api_key="sk-ge", + planning_provider="anthropic", coding_provider="gemini", router_provider="anthropic") + out = db_to_env(s, present_keys={"anthropic_api_key", "gemini_api_key", + "planning_provider", "coding_provider", "router_provider"}) + assert out["ANTON_PLANNING_PROVIDER"] == "anthropic" + assert out["ANTON_ANTHROPIC_API_KEY"] == "sk-an" + assert out["ANTON_CODING_PROVIDER"] == "openai-compatible" # gemini -> oc + assert out["ANTON_OPENAI_API_KEY"] == "sk-ge" # gemini key, OpenAI slot + assert out["ANTON_OPENAI_BASE_URL"] == GEMINI_BASE_URL + + +def test_db_to_env_skips_unrepresentable_openai_gemini_mix(): + # planning=OpenAI + coding=Gemini both need the single OpenAI slot but with a + # DIFFERENT key+base — Anton can't represent it, so export no provider cluster + # rather than misroute OpenAI's key to Google's endpoint (ENG-1127 review). + s = UserSettings(openai_api_key="sk-oa", gemini_api_key="sk-ge", + planning_provider="openai", coding_provider="gemini", router_provider="openai") + out = db_to_env(s, present_keys={"openai_api_key", "gemini_api_key", + "planning_provider", "coding_provider", "router_provider"}) + assert not any(k.startswith("ANTON_PLANNING") or k.startswith("ANTON_CODING") + or k.startswith("ANTON_OPENAI") for k in out) + + +def test_db_to_env_skips_unrepresentable_minds_plus_openai_mix(): + # MindsHub derives its OpenAI creds in Anton's model_post_init ONLY when the + # OpenAI key is unset; a coding=OpenAI role sets it, breaking the derivation. + # So MindsHub + OpenAI is not representable either. + s = UserSettings(minds_api_key="sk-m", minds_url="https://api.mindshub.ai", openai_api_key="sk-oa", + planning_provider="minds_cloud", coding_provider="openai", router_provider="minds_cloud") + out = db_to_env(s, present_keys={"minds_api_key", "minds_url", "openai_api_key", + "planning_provider", "coding_provider", "router_provider"}) + assert not any("PROVIDER" in k or "OPENAI" in k or "MINDS" in k for k in out) + + +def _anton_roundtrip(env_dict): + """Feed a db_to_env export into the pinned Anton and build its client. + + Returns the LLMClient (raises if Anton rejects the config). Restores os.environ. + """ + from anton.config.settings import AntonSettings + from anton.core.llm.client import LLMClient + + saved = {k: os.environ[k] for k in list(os.environ) if k.startswith("ANTON_")} + for k in list(os.environ): + if k.startswith("ANTON_"): + del os.environ[k] + os.environ.update(env_dict) + try: + return LLMClient.from_settings(AntonSettings()) + finally: + for k in list(os.environ): + if k.startswith("ANTON_"): + del os.environ[k] + os.environ.update(saved) + + +def test_router_only_minds_exports_explicit_openai_slot_and_round_trips(): + # planning/coding=Anthropic, router=MindsHub. Anton's model_post_init derives + # the OpenAI slot ONLY for a planning/coding openai-compatible role, never + # router-only — so the export must write the router's OpenAI slot EXPLICITLY, + # or Anton builds the router with no key/base (ENG-1127 review). + from anton.core.llm.openai import OpenAIProvider + + s = UserSettings(anthropic_api_key="sk-an", minds_api_key="sk-m", minds_url="https://api.mindshub.ai", + planning_provider="anthropic", coding_provider="anthropic", router_provider="minds_cloud") + out = db_to_env(s, {"anthropic_api_key", "minds_api_key", "minds_url", + "planning_provider", "coding_provider", "router_provider"}) + assert out["ANTON_ROUTER_PROVIDER"] == "minds-cloud" + assert out["ANTON_OPENAI_API_KEY"] == "sk-m" # minds key in the OpenAI slot, explicit + assert out["ANTON_OPENAI_BASE_URL"] # minds base, explicit (not left to derivation) + client = _anton_roundtrip(out) # the pinned CLI accepts and builds it + assert isinstance(client.router_provider, OpenAIProvider) + + +def test_db_to_env_drops_cluster_when_openai_compatible_has_no_base(): + # An openai-compatible provider with no base would make Anton default to + # api.openai.com and leak the key. The cluster is withheld atomically rather + # than exporting provider+key without the base (ENG-1127 review). + s = UserSettings(openai_compatible_api_key="sk-oc", + planning_provider="openai_compatible", coding_provider="openai_compatible") + out = db_to_env(s, {"openai_compatible_api_key", "planning_provider", "coding_provider"}) + assert not any(k.startswith("ANTON_OPENAI") or k.endswith("_PROVIDER") for k in out) + # With a base present, the same config exports cleanly. + s2 = UserSettings(openai_compatible_api_key="sk-oc", openai_base_url="https://my.host/v1", + planning_provider="openai_compatible", coding_provider="openai_compatible") + out2 = db_to_env(s2, {"openai_compatible_api_key", "openai_base_url", + "planning_provider", "coding_provider"}) + assert out2["ANTON_OPENAI_BASE_URL"] == "https://my.host/v1" + assert out2["ANTON_OPENAI_API_KEY"] == "sk-oc" + + +def test_db_to_env_pairs_router_with_its_own_providers_model(): + # coding=Anthropic + router=OpenAI, no stored router model: the router default + # must come from router_provider (an OpenAI model), not coding_provider — else + # the CLI gets ANTON_ROUTER_PROVIDER=openai with a Claude model and fails when + # the router runs (ENG-1127 review). + s = UserSettings(anthropic_api_key="sk-an", openai_api_key="sk-oa", + planning_provider="anthropic", coding_provider="anthropic", router_provider="openai") + out = db_to_env(s, {"anthropic_api_key", "openai_api_key", + "planning_provider", "coding_provider", "router_provider"}) + assert out["ANTON_ROUTER_PROVIDER"] == "openai" + assert "claude" not in out["ANTON_ROUTER_MODEL"].lower() # not the coding (Anthropic) model + assert _anton_roundtrip(out).router_provider is not None # pinned CLI builds it + + +def test_db_to_env_exports_nothing_when_unconfigured(): + # No key anywhere → no provider/model/creds; flags export only when stored. + assert db_to_env(UserSettings(), present_keys=set()) == {} + # A resolved provider with no key exports no provider line for that role. + out = db_to_env(UserSettings(), present_keys={"planning_provider"}) + assert "ANTON_PLANNING_PROVIDER" not in out + + +def test_merge_preserves_unmanaged_and_replaces_managed(): + existing = ( + "COWORK_AUTH_TOKEN=tok-123\n" + "ANTON_PLANNING_MODEL=latest:sonnet\n" # now MANAGED (ENG-1127): dropped unless re-supplied + "ANTON_ANTHROPIC_API_KEY=stale\n" + "ANTON_FIRST_RUN_DONE=true\n" + "# a comment\n" + ) + merged = merge_env_lines(existing, {"ANTON_ANTHROPIC_API_KEY": "fresh", "ANTON_MINDS_URL": "https://m"}) + lines = merged.strip().split("\n") + # Genuinely unmanaged lines survive verbatim. + assert "COWORK_AUTH_TOKEN=tok-123" in lines + assert "ANTON_FIRST_RUN_DONE=true" in lines + assert "# a comment" in lines + # Model vars are managed now — a stale one not re-supplied is dropped (the + # export always re-supplies the resolved model, so no mismatch survives). + assert "ANTON_PLANNING_MODEL=latest:sonnet" not in lines + # The stale managed line is replaced, not duplicated. + assert "ANTON_ANTHROPIC_API_KEY=stale" not in lines + assert lines.count("ANTON_ANTHROPIC_API_KEY=fresh") == 1 + assert "ANTON_MINDS_URL=https://m" in lines + + +def test_merge_drops_cleared_managed_key(): + existing = "COWORK_AUTH_TOKEN=tok\nANTON_MINDS_API_KEY=old\n" + merged = merge_env_lines(existing, {}) + assert "ANTON_MINDS_API_KEY" not in merged + assert "COWORK_AUTH_TOKEN=tok" in merged + + +# ── dotenv-injection guard (Finding 3) ───────────────────────────────── + +def test_merge_env_lines_rejects_crlf_injection(): + # A CR/LF in a value must not become a second (unmanaged, surviving) line. + poisoned = "https://x\nDATABASE_URI=sqlite:///tmp/evil.db" + merged = merge_env_lines("", {"ANTON_MINDS_URL": poisoned, "ANTON_MINDS_API_KEY": "sk-ok"}) + assert "DATABASE_URI" not in merged # injected assignment dropped + assert "ANTON_MINDS_URL" not in merged # the poisoned value is not smuggled + assert "ANTON_MINDS_API_KEY=sk-ok" in merged # clean siblings still export + # every emitted line is a single well-formed assignment + assert all(ln.count("=") >= 1 for ln in merged.split("\n") if ln) + + +def test_db_to_env_drops_whole_cluster_on_newline_bearing_value(monkeypatch): + # A poisoned value taints the WHOLE provider cluster atomically — a provider+key + # without its (dropped) base is worse than no export (ENG-1127 review). So even + # the clean sibling key is withheld, not just the poisoned URL. + s = UserSettings(minds_api_key="sk-minds", planning_provider="minds_cloud") + monkeypatch.setattr(s, "minds_url", "https://x\nDATABASE_URI=sqlite:///evil.db", raising=False) + out = db_to_env(s, present_keys={"minds_api_key", "planning_provider"}) + assert not any(k.startswith("ANTON_MINDS") or k.startswith("ANTON_OPENAI") for k in out) + + +def test_export_never_writes_injected_line_end_to_end(local_export): + # Full path: a poisoned DB value must never reach the CLI's .env — the cluster + # is withheld atomically, so no injected line and no partial cluster land. + env_path = local_export + session = get_open_session() + try: + _cleanup(session, "minds_url", "minds_api_key") + svc = SettingService(session) + svc.save_all({"minds_api_key": "sk-clean", "minds_url": "https://h\nDATABASE_URI=x"}) + text = env_path.read_text(encoding="utf-8") if env_path.exists() else "" + assert "DATABASE_URI" not in text + assert "ANTON_MINDS_API_KEY" not in text # whole cluster withheld, not a partial one + finally: + _cleanup(session, "minds_url", "minds_api_key") + session.close() + + +# ── SettingService integration ───────────────────────────────────────── + +@pytest.fixture +def local_export(monkeypatch, tmp_path): + """Point the export at a tmp .env and pretend we're a local desktop install.""" + monkeypatch.setattr(settings_mod, "cowork_home", lambda: tmp_path) + monkeypatch.setattr(settings_mod, "get_app_settings", lambda: SimpleNamespace(tenancy_mode="local")) + return tmp_path / ".env" + + +def _cleanup(session, *keys): + svc = SettingService(session) + for k in keys: + try: + svc.delete_setting(k) + except ValueError: + pass + + +def test_save_all_exports_env_and_preserves_unmanaged(local_export): + env_path = local_export + env_path.write_text("COWORK_AUTH_TOKEN=tok-1\nANTON_PLANNING_MODEL=latest:sonnet\n", encoding="utf-8") + session = get_open_session() + try: + _cleanup(session, "minds_api_key", "minds_url", "planning_provider") + SettingService(session).save_all( + {"minds_api_key": "sk-abc", "minds_url": "https://mdb", "planning_provider": "minds_cloud"} + ) + text = env_path.read_text(encoding="utf-8") + assert "ANTON_MINDS_API_KEY=sk-abc" in text + assert "ANTON_MINDS_URL=https://mdb" in text + assert "ANTON_PLANNING_PROVIDER=minds-cloud" in text + # Genuinely unmanaged lines the server must not touch. + assert "COWORK_AUTH_TOKEN=tok-1" in text + # The model is managed now: the stale hand-set pin is replaced by the + # resolved model that pairs with the exported provider (no mismatch). + assert "ANTON_PLANNING_MODEL=latest:sonnet" not in text + assert "ANTON_PLANNING_MODEL=" in text + assert oct(env_path.stat().st_mode)[-3:] == "600" + finally: + _cleanup(session, "minds_api_key", "minds_url", "planning_provider") + session.close() + + +def test_export_skipped_for_org_tenancy(monkeypatch, tmp_path): + monkeypatch.setattr(settings_mod, "cowork_home", lambda: tmp_path) + monkeypatch.setattr(settings_mod, "get_app_settings", lambda: SimpleNamespace(tenancy_mode="org")) + session = get_open_session() + try: + _cleanup(session, "minds_url") + SettingService(session).save_all({"minds_url": "https://mdb"}) + assert not (tmp_path / ".env").exists() # cloud pod writes no .env + finally: + _cleanup(session, "minds_url") + session.close() + + +def test_clear_credentials_wipes_creds_and_orphaned_model_from_env(local_export): + env_path = local_export + env_path.write_text("COWORK_AUTH_TOKEN=keep\n", encoding="utf-8") + session = get_open_session() + try: + _cleanup(session, "minds_api_key", "minds_url", "planning_provider") + svc = SettingService(session) + svc.save_all( + {"minds_api_key": "sk-abc", "minds_url": "https://mdb", "planning_provider": "minds_cloud"} + ) + exported = env_path.read_text(encoding="utf-8") + assert "ANTON_MINDS_API_KEY=sk-abc" in exported + assert "ANTON_PLANNING_MODEL=" in exported # provider+model exported as a pair + + svc.clear_credentials() + text = env_path.read_text(encoding="utf-8") + assert "ANTON_MINDS_API_KEY" not in text # credential wiped from the CLI too + assert "ANTON_MINDS_URL" not in text + # With no key left, no provider resolves — so its now-orphaned model line + # is dropped too, rather than left mismatched against a gone provider. + assert "ANTON_PLANNING_MODEL" not in text + assert "COWORK_AUTH_TOKEN=keep" in text # unmanaged line untouched + finally: + _cleanup(session, "minds_api_key", "minds_url", "planning_provider") + session.close() + + +def test_unrepresentable_save_preserves_existing_cli_config(local_export): + # Start with a valid single-provider config in .env, then move the DB to a + # config Anton can't represent (minds + openai). The export must NOT wipe the + # working cluster — merge_env_lines only reconciles what env_reconcile_vars + # deems authoritative this run (ENG-1127 review); only a genuine clear wipes it. + env_path = local_export + keys = ("minds_api_key", "minds_url", "planning_provider", "coding_provider", "openai_api_key") + session = get_open_session() + try: + _cleanup(session, *keys) + svc = SettingService(session) + # 1) representable minds config -> a valid cluster is written. + svc.save_all({"minds_api_key": "sk-m", "minds_url": "https://api.mindshub.ai", + "planning_provider": "minds_cloud", "coding_provider": "minds_cloud"}) + first = env_path.read_text(encoding="utf-8") + assert "ANTON_MINDS_API_KEY=sk-m" in first + assert "ANTON_PLANNING_PROVIDER=minds-cloud" in first + + # 2) move coding to OpenAI (its own key) -> minds+openai is unrepresentable. + svc.save_all({"openai_api_key": "sk-oa", "coding_provider": "openai"}) + after = env_path.read_text(encoding="utf-8") + # The previously-valid cluster is preserved, not wiped. + assert "ANTON_MINDS_API_KEY=sk-m" in after + assert "ANTON_PLANNING_PROVIDER=minds-cloud" in after + finally: + _cleanup(session, *keys) + session.close() + + +def test_migration_write_does_not_export(local_export): + # migration seeds the DB from .env (export_env=False), so a seed write must not rewrite it + env_path = local_export + session = get_open_session() + try: + _cleanup(session, "minds_url") + SettingService(session).upsert_setting("minds_url", "https://seed", export_env=False) + assert not env_path.exists() + finally: + _cleanup(session, "minds_url") + session.close() + + +def test_backfill_minds_url_exports_to_env_too(local_export): + # ENG-1127 review: the legacy-host backfill writes minds_url directly, so it + # must also export — otherwise the DB moves to the canonical host while the + # CLI's .env keeps the dead mdb.ai endpoint. + from cowork.common.settings.app_settings import default_minds_api_host + from cowork.migrations import backfill_minds_url + + env_path = local_export + canonical = default_minds_api_host() + session = get_open_session() + try: + _cleanup(session, "minds_url", "minds_api_key", "planning_provider") + svc = SettingService(session) + # A real minds-cloud user (has the key) on the legacy host — only then is + # minds the resolved provider, so ANTON_MINDS_URL is part of the export. + svc.upsert_setting("minds_api_key", "sk-minds", export_env=False) + svc.upsert_setting("planning_provider", "minds_cloud", export_env=False) + svc.upsert_setting("minds_url", "https://mdb.ai", export_env=False) + env_path.write_text("ANTON_MINDS_URL=https://mdb.ai\n", encoding="utf-8") + + assert backfill_minds_url(session) is True + # Both stores land on the canonical host. + assert svc.load().minds_url == canonical + env_text = env_path.read_text(encoding="utf-8") + assert f"ANTON_MINDS_URL={canonical}" in env_text + assert "https://mdb.ai" not in env_text + finally: + _cleanup(session, "minds_url", "minds_api_key", "planning_provider") + session.close() + + +def test_export_serializes_concurrent_writers(local_export, monkeypatch): + # ENG-1127 review: the read/merge/write must be serialized so two concurrent + # exporters can't lost-update the file. Instrument the critical section and + # assert at most one thread is ever inside it. + import threading + import time + + state = {"cur": 0, "max": 0} + guard = threading.Lock() + real = settings_mod.db_to_env + + def instrumented(settings, present_keys): + with guard: + state["cur"] += 1 + state["max"] = max(state["max"], state["cur"]) + time.sleep(0.03) # widen the window an unserialized export would overlap in + with guard: + state["cur"] -= 1 + return real(settings, present_keys) + + monkeypatch.setattr(settings_mod, "db_to_env", instrumented) + + def export(): + SettingService(get_open_session())._export_env_for_cli() + + threads = [threading.Thread(target=export) for _ in range(2)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert state["max"] == 1 + + +# ── atomic_write_env: Windows share-mode lock hardening (ENG-1209/ENG-1127) ── + +def test_atomic_write_env_retries_transient_lock(tmp_path, monkeypatch): + # The CLI (or a version-skewed server) holding .env open EPERM'd the rename on + # Windows (ENG-1209). Now that the server writes it, the rename must retry. + dest = tmp_path / ".env" + real_replace = os.replace + calls = {"n": 0} + + def flaky_replace(src, dst): + calls["n"] += 1 + if calls["n"] < 3: + raise PermissionError(errno.EACCES, "share-mode lock") + return real_replace(src, dst) + + monkeypatch.setattr(eb.os, "replace", flaky_replace) + monkeypatch.setattr(eb.time, "sleep", lambda *_: None) # no real backoff in tests + + eb.atomic_write_env(dest, "ANTON_MINDS_URL=https://m\n") + + assert calls["n"] == 3 # two transient failures, third lands + assert dest.read_text(encoding="utf-8") == "ANTON_MINDS_URL=https://m\n" + assert list(tmp_path.glob(".env.*.tmp")) == [] # temp consumed by the rename + + +def test_atomic_write_env_rethrows_non_transient_and_cleans_temp(tmp_path, monkeypatch): + # A non-lock error (ENOENT, unwritable target, …) must fail fast, not burn the + # retry budget, and must never leave the plaintext-key temp behind. + dest = tmp_path / ".env" + calls = {"n": 0} + + def broken_replace(src, dst): + calls["n"] += 1 + raise OSError(errno.ENOENT, "gone") + + monkeypatch.setattr(eb.os, "replace", broken_replace) + monkeypatch.setattr(eb.time, "sleep", lambda *_: None) + + with pytest.raises(OSError): + eb.atomic_write_env(dest, "x\n") + + assert calls["n"] == 1 # rethrown on the first attempt, no retry + assert not dest.exists() + assert list(tmp_path.glob(".env.*.tmp")) == [] # temp cleaned up on failure + + +def test_atomic_write_env_sweeps_stale_temp_keeps_fresh(tmp_path): + # Orphaned temps hold the full plaintext key, so a stale one is reclaimed; a + # concurrent writer's fresh in-flight temp is spared. + stale = tmp_path / ".env.stale123.tmp" + stale.write_text("ANTON_MINDS_API_KEY=leaked\n", encoding="utf-8") + fresh = tmp_path / ".env.fresh456.tmp" + fresh.write_text("in-flight\n", encoding="utf-8") + old = os.stat(fresh).st_mtime - (eb._STALE_TMP_S + 60) + os.utime(stale, (old, old)) + + eb.atomic_write_env(tmp_path / ".env", "ANTON_MINDS_URL=https://m\n") + + assert not stale.exists() # orphaned plaintext-key temp reclaimed + assert fresh.exists() # a live writer's temp is not yanked mid-rename diff --git a/tests/test_settings_raw.py b/tests/test_settings_raw.py index a88cb4bf..2249ddb4 100644 --- a/tests/test_settings_raw.py +++ b/tests/test_settings_raw.py @@ -21,12 +21,14 @@ def _delete_settings(session, *keys: str) -> None: pass -def test_raw_settings_write_syncs_credentials_but_not_models(tmp_path, monkeypatch): - # ENG-739: /settings/raw syncs credentials + provider selection to the DB, - # but NOT model keys — a model in .env is CLI-only and must never be pushed - # to the DB, or a bulk sync (web token refresh, re-login) would re-pin a - # user who fixed a locked-model 403 via the picker. The .env line is still - # written to disk for the standalone CLI. +def test_raw_settings_write_syncs_only_incoming_not_the_whole_env(tmp_path, monkeypatch): + # ENG-1127 review: /settings/raw must sync ONLY the recognised vars in THIS + # request to the DB, never the whole merged .env. The server now mirrors + # DB->.env, so the file can hold a preserved/translated cluster (a stale + # minds-cloud line, a gemini role written as openai-compatible); re-syncing all + # of it would overwrite the authoritative DB choice from the CLI's derived + # file. Models are still never synced (ENG-739). The full .env is still written + # to disk for the standalone CLI. from cowork.api.v1.endpoints import settings as settings_endpoint from cowork.api.v1.endpoints.settings import _RawSettingsBody, write_raw_settings from cowork.db.session import get_open_session @@ -34,40 +36,43 @@ def test_raw_settings_write_syncs_credentials_but_not_models(tmp_path, monkeypat env_path = tmp_path / ".anton" / ".env" env_path.parent.mkdir(parents=True) - env_path.write_text( - "\n".join( - [ - "ANTON_MINDS_API_KEY=existing-key", - "ANTON_PLANNING_PROVIDER=openai-compatible", - ] - ) - + "\n", - encoding="utf-8", - ) + # A STALE cluster line already on disk that the incoming request does NOT touch. + env_path.write_text("ANTON_CODING_PROVIDER=minds-cloud\n", encoding="utf-8") monkeypatch.setattr(settings_endpoint, "_ENV_PATH", env_path) session = get_open_session() + keys = ("minds_api_key", "planning_provider", "planning_model", "coding_provider") try: - _delete_settings(session, "minds_api_key", "planning_provider", "planning_model") - - response = write_raw_settings(_RawSettingsBody(content="ANTON_PLANNING_MODEL=_reason_"), session, _local_request()) - + _delete_settings(session, *keys) + + response = write_raw_settings( + _RawSettingsBody(content="\n".join([ + "ANTON_MINDS_API_KEY=new-key", + "ANTON_PLANNING_PROVIDER=minds_cloud", + "ANTON_PLANNING_MODEL=_reason_", + ])), + session, _local_request(), + ) assert response == {"ok": True} - raw = settings_endpoint.read_raw_settings(_local_request()) - assert raw["ANTON_MINDS_API_KEY"] == "existing-key" - # The model line IS preserved in .env (CLI-only surface)… - assert raw["ANTON_PLANNING_MODEL"] == "_reason_" - # …but is NOT synced to the DB: the row stays unset (resolves to a - # default), while credentials + provider are synced as before. service = SettingService(session) - assert service._fetch_row("planning_model") is None loaded = service.load() - assert loaded.minds_api_key.get_secret_value() == "existing-key" + # Incoming credential + provider synced to the DB. + assert loaded.minds_api_key.get_secret_value() == "new-key" assert loaded.planning_provider.value == "minds_cloud" - assert loaded.planning_model != "_reason_" + # Model in the request is NOT synced (ENG-739). + assert service._fetch_row("planning_model") is None + # The STALE on-disk coding_provider is NOT pulled into the DB (Finding 1). + assert service._fetch_row("coding_provider") is None + + # The full .env is still written to disk for the CLI — merge preserves the + # untouched stale line and the CLI-only model line. + raw = settings_endpoint.read_raw_settings(_local_request()) + assert raw["ANTON_MINDS_API_KEY"] == "new-key" + assert raw["ANTON_CODING_PROVIDER"] == "minds-cloud" + assert raw["ANTON_PLANNING_MODEL"] == "_reason_" finally: - _delete_settings(session, "minds_api_key", "planning_provider", "planning_model") + _delete_settings(session, *keys) session.close() diff --git a/tests/test_settings_schema.py b/tests/test_settings_schema.py index b5923cad..7231008a 100644 --- a/tests/test_settings_schema.py +++ b/tests/test_settings_schema.py @@ -2,12 +2,13 @@ contract. These guards fail CI the moment the migration's env map or the provider normalization could drift from the model. """ -from cowork.common.settings.user_settings import ( +from cowork.common.settings.env_boundary import ( ENV_ALIAS_TO_SETTING, SETTING_ENV_ALIASES, - UserSettings, + env_to_db_updates, normalize_provider_value, ) +from cowork.common.settings.user_settings import UserSettings def test_env_aliases_reference_only_real_fields(): @@ -57,13 +58,14 @@ def test_normalize_provider_value_single_implementation(): ) -def test_migration_normalizer_delegates_to_the_canonical_one(): - from cowork.migrations import _normalize_provider_value - - assert ( - _normalize_provider_value( - "openai-compatible", {"ANTON_MINDS_API_KEY": "sk-x"} - ) - == "minds_cloud" +def test_env_to_db_updates_normalizes_providers(): + # The inbound .env->DB conversion: a Minds key alongside an openai-compatible + # provider means minds_cloud; without it, a genuine custom endpoint. + with_key = env_to_db_updates( + {"ANTON_PLANNING_PROVIDER": "openai-compatible", "ANTON_MINDS_API_KEY": "sk-x"} ) - assert _normalize_provider_value("openai-compatible", {}) == "openai_compatible" + assert with_key["planning_provider"] == "minds_cloud" + without_key = env_to_db_updates({"ANTON_PLANNING_PROVIDER": "openai-compatible"}) + assert without_key["planning_provider"] == "openai_compatible" + # Unmapped / empty vars are skipped. + assert env_to_db_updates({"ANTON_TERMS_CONSENT": "true", "ANTON_MINDS_URL": ""}) == {}