From 4074004a569e99dd38d855427b09f20022bb1d2b Mon Sep 17 00:00:00 2001 From: pnewsam Date: Wed, 29 Jul 2026 11:04:03 -0700 Subject: [PATCH 01/13] feat(settings): export DB settings to the CLI's .env (ENG-1127, Phase A) Make cowork-server mirror its settings out to the standalone `anton` CLI's .env after every write, so the DB is the source of truth and .env is a derived export rather than a competing one. Runs only for local (single-user desktop) tenancy; a multi-tenant cloud pod writes no .env. Re-derives the aliased keys (SETTING_ENV_ALIASES) from the DB at the shared post-commit seam, decrypting secrets, using the CLI's dash-form provider values, and merge-preserving every unmanaged line (auth token, CLI-only model pins per ENG-739, comments). Atomic write, 0o600, skipped when unchanged, and never raises into the settings write. The startup .env->DB migration passes export_env=False so it can't rewrite the file it seeds from. Phase A of ENG-1127. Phase B (removing the client's own .env writes + login replay) is a follow-up, gated on this landing. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_export.py | 116 +++++++++++++++++++ cowork/migrations.py | 7 +- cowork/services/settings.py | 55 ++++++++- tests/conftest.py | 4 + tests/test_settings_env_export.py | 163 +++++++++++++++++++++++++++ 5 files changed, 339 insertions(+), 6 deletions(-) create mode 100644 cowork/common/settings/env_export.py create mode 100644 tests/test_settings_env_export.py diff --git a/cowork/common/settings/env_export.py b/cowork/common/settings/env_export.py new file mode 100644 index 00000000..51fbf69b --- /dev/null +++ b/cowork/common/settings/env_export.py @@ -0,0 +1,116 @@ +"""Derive the CLI's ``.env`` from the DB settings (ENG-1127, Phase A). + +The DB is the source of truth for cowork-server. The standalone ``anton`` CLI +still reads its config from ``.env``, so on a single-user desktop install the +server exports the overlapping settings back out to ``.env`` after every write, +treating the file as an export rather than a second source of truth. + +This module holds the pure (no-DB) derivation so it is unit-testable; the gate, +path resolution, and the write itself live in +``SettingService._export_env_for_cli``. + +Only the keys in ``SETTING_ENV_ALIASES`` are managed. Everything else in the +file is preserved verbatim: ``COWORK_AUTH_TOKEN`` (written by the auth +middleware), the ``ANTON_*_MODEL`` lines (CLI-only, deliberately excluded from +the sync per ENG-739 so a login/refresh can't re-pin a picker choice), +``ANTON_FIRST_RUN_DONE`` (written by the CLI itself), and any comments. +""" +from __future__ import annotations + +import os +import tempfile +from enum import Enum +from pathlib import Path + +from pydantic import SecretStr + +from cowork.common.settings.user_settings import ( + SETTING_ENV_ALIASES, + Provider, + UserSettings, +) + +# The ANTON_* variables this export owns. A line for any of these is replaced on +# export; every other line in the file is left untouched. +MANAGED_ENV_VARS: tuple[str, ...] = tuple(SETTING_ENV_ALIASES.values()) + + +def _env_str(value: object) -> str: + """Format a loaded ``UserSettings`` value as its ``.env`` string. + + Providers use the dash form the CLI expects (``minds-cloud``, not the DB's + ``minds_cloud``); secrets are the decrypted plaintext (``.env`` is plaintext + while the DB is Fernet-encrypted); 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 build_env_export(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: + """Map the aliased settings that are actually STORED to ``{ANTON_VAR: value}``. + + Only keys in ``present_keys`` (i.e. that have a DB row) are exported. This is + deliberate: ``settings`` carries a resolved default for every unset field, so + exporting on non-``None`` alone would push the server's defaults (and their + drift) onto the CLI and would leave a line behind after a key is cleared. + Mirroring only stored rows keeps ``.env`` a faithful export of the DB and + lets the CLI apply its own defaults for anything unset. Unset/empty stored + values are still skipped. Model keys are absent from ``SETTING_ENV_ALIASES`` + (ENG-739), so they are never exported here. + """ + out: dict[str, str] = {} + for db_key, env_var in SETTING_ENV_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 == "": + continue + out[env_var] = text + return out + + +def merge_env_lines(existing: str, managed: dict[str, str]) -> str: + """Rewrite the managed lines in ``existing``, preserving everything else. + + Every line for a managed ANTON_* var is dropped, then the currently-set + managed vars are appended (in ``SETTING_ENV_ALIASES`` order, so repeated + exports of the same state produce identical output). Unmanaged lines keep + their place. A managed key absent from ``managed`` (unset or cleared) loses + its line — that is how a logout also wipes credentials from the CLI's file. + """ + drop = tuple(f"{var}=" for var in MANAGED_ENV_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()) + return "\n".join(kept) + "\n" + + +def atomic_write_env(path: Path, content: str) -> None: + """Write ``content`` to ``path`` atomically, owner-only (0o600). + + Temp file in the same directory + ``os.replace`` so a crash or a concurrent + CLI read never observes a truncated ``.env``. 0o600 because the file holds + plaintext API keys. + """ + path.parent.mkdir(parents=True, exist_ok=True) + 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) + os.replace(tmp, str(path)) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise diff --git a/cowork/migrations.py b/cowork/migrations.py index 6259b64c..67754e3d 100644 --- a/cowork/migrations.py +++ b/cowork/migrations.py @@ -142,7 +142,12 @@ def migrate_env_to_db(session: Session) -> bool: if setting_key.endswith("_provider"): val = _normalize_provider_value(val, dotenv) try: - svc.upsert_setting(setting_key, val) + # export_env=False: this loop seeds the DB *from* .env; letting + # the per-key write re-export .env mid-loop would strip the keys + # not migrated yet (and blow them away on a crash). The DB is + # authoritative once seeding completes; 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) diff --git a/cowork/services/settings.py b/cowork/services/settings.py index cdbb75d1..6fb182b0 100644 --- a/cowork/services/settings.py +++ b/cowork/services/settings.py @@ -7,6 +7,13 @@ 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_export import ( + atomic_write_env, + build_env_export, + merge_env_lines, +) from cowork.common.settings.user_settings import ( UserSettings, invalidate_user_settings_cache, @@ -154,11 +161,49 @@ 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 settings cache and, on a single-user desktop install, + mirrors the DB out to the CLI's ``.env`` (ENG-1127). ``export_env=False`` + is for the startup ``.env``→DB migration, which must not rewrite the very + file it is seeding from — doing so mid-loop would strip the keys it has + not migrated yet. + """ + 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). + + The DB is the source of truth (ENG-1127); ``.env`` is a derived export + the standalone ``anton`` CLI reads. Only runs for ``local`` (single-user + desktop) tenancy — a multi-tenant cloud pod has no per-user ``.env`` or + CLI and must not spill decrypted secrets to disk. Re-derives the whole + managed set from the DB and merge-writes it, preserving every unmanaged + line (auth token, CLI-only model pins, comments). Skips the write when + the file is already current. Never raises into the write that triggered + it — a stale ``.env`` must not fail a settings save. + """ + try: + if get_app_settings().tenancy_mode != "local": + return + rows = self._fetch_all_rows() + managed = build_env_export(self._load(rows), {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) + 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 +225,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 +273,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 +306,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..33c6cd0b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,6 +20,10 @@ # 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: cowork_home() backs the .env path. A settings write now mirrors +# the DB out to $COWORK_HOME/.env for the CLI (ENG-1127), so without this a +# settings-writing test would rewrite the developer's real ~/.cowork/.env. +os.environ["COWORK_HOME"] = str(TMP) os.environ["ENV"] = "test" import pytest diff --git a/tests/test_settings_env_export.py b/tests/test_settings_env_export.py new file mode 100644 index 00000000..1ae93c40 --- /dev/null +++ b/tests/test_settings_env_export.py @@ -0,0 +1,163 @@ +"""ENG-1127 Phase A: the server exports the DB's settings back to the CLI's .env. + +Pure derivation (build_env_export / merge_env_lines) plus the SettingService +integration: a write mirrors to .env only for local tenancy, preserves unmanaged +lines (auth token, CLI model pins), and the .env→DB migration never re-exports. +""" +from types import SimpleNamespace + +import pytest + +import cowork.services.settings as settings_mod +from cowork.common.settings.env_export import build_env_export, merge_env_lines +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_build_env_export_formats_and_excludes_models(): + s = UserSettings( + anthropic_api_key="sk-secret", + planning_provider="minds_cloud", + minds_url="https://mdb.example", + planning_model="latest:sonnet", # not aliased — must NOT be exported + ) + present = {"anthropic_api_key", "planning_provider", "minds_url", "planning_model"} + out = build_env_export(s, present) + assert out["ANTON_ANTHROPIC_API_KEY"] == "sk-secret" # decrypted plaintext + assert out["ANTON_PLANNING_PROVIDER"] == "minds-cloud" # ui_value (dash form) + assert out["ANTON_MINDS_URL"] == "https://mdb.example" + # Model keys are deliberately not in SETTING_ENV_ALIASES (ENG-739). + assert not any("MODEL" in k for k in out) + + +def test_build_env_export_only_exports_stored_keys(): + # minds_url has a non-None default, but with no row present it must not be + # exported — .env mirrors stored settings, not resolved defaults. + s = UserSettings(anthropic_api_key="sk-x") + out = build_env_export(s, present_keys={"anthropic_api_key"}) + assert out == {"ANTON_ANTHROPIC_API_KEY": "sk-x"} + assert "ANTON_MINDS_URL" not in out # defaulted but not stored + # Nothing stored at all → empty export. + assert build_env_export(UserSettings(), present_keys=set()) == {} + + +def test_merge_preserves_unmanaged_and_replaces_managed(): + existing = ( + "COWORK_AUTH_TOKEN=tok-123\n" + "ANTON_PLANNING_MODEL=latest:sonnet\n" + "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") + # Unmanaged lines survive verbatim. + assert "COWORK_AUTH_TOKEN=tok-123" in lines + assert "ANTON_PLANNING_MODEL=latest:sonnet" in lines + assert "ANTON_FIRST_RUN_DONE=true" in lines + assert "# a comment" 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, {}) # nothing set → managed keys removed + assert "ANTON_MINDS_API_KEY" not in merged + assert "COWORK_AUTH_TOKEN=tok" in merged + + +# ── 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 + # A pre-existing .env with an auth token and a CLI-only model pin. + 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 # decrypted secret + assert "ANTON_MINDS_URL=https://mdb" in text + assert "ANTON_PLANNING_PROVIDER=minds-cloud" in text # dash form + # Unmanaged lines the server must not touch. + assert "COWORK_AUTH_TOKEN=tok-1" in text + assert "ANTON_PLANNING_MODEL=latest:sonnet" 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_from_env_but_keeps_model(local_export): + env_path = local_export + env_path.write_text("ANTON_PLANNING_MODEL=latest:sonnet\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"} + ) + assert "ANTON_MINDS_API_KEY=sk-abc" in env_path.read_text(encoding="utf-8") + + 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 + assert "ANTON_PLANNING_MODEL=latest:sonnet" in text # CLI model pin survives + finally: + _cleanup(session, "minds_api_key", "minds_url", "planning_provider") + session.close() + + +def test_migration_write_does_not_export(local_export): + # migrate_env_to_db seeds the DB *from* .env with export_env=False, so a + # per-key seed write must not rewrite (and mid-loop truncate) that .env. + 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() From 04666cf5eda201520a9057a2ea1367e7db87c36a Mon Sep 17 00:00:00 2001 From: pnewsam Date: Wed, 29 Jul 2026 15:00:50 -0700 Subject: [PATCH 02/13] docs(settings): trim verbose comments on the ENG-1127 Phase A change Comment/docstring-only cleanup (~half the added comment lines); no code changes. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_export.py | 49 +++++++--------------------- cowork/migrations.py | 8 ++--- cowork/services/settings.py | 17 +++------- tests/conftest.py | 5 ++- tests/test_settings_env_export.py | 30 +++++++---------- 5 files changed, 33 insertions(+), 76 deletions(-) diff --git a/cowork/common/settings/env_export.py b/cowork/common/settings/env_export.py index 51fbf69b..99634bee 100644 --- a/cowork/common/settings/env_export.py +++ b/cowork/common/settings/env_export.py @@ -1,19 +1,8 @@ """Derive the CLI's ``.env`` from the DB settings (ENG-1127, Phase A). -The DB is the source of truth for cowork-server. The standalone ``anton`` CLI -still reads its config from ``.env``, so on a single-user desktop install the -server exports the overlapping settings back out to ``.env`` after every write, -treating the file as an export rather than a second source of truth. - -This module holds the pure (no-DB) derivation so it is unit-testable; the gate, -path resolution, and the write itself live in -``SettingService._export_env_for_cli``. - -Only the keys in ``SETTING_ENV_ALIASES`` are managed. Everything else in the -file is preserved verbatim: ``COWORK_AUTH_TOKEN`` (written by the auth -middleware), the ``ANTON_*_MODEL`` lines (CLI-only, deliberately excluded from -the sync per ENG-739 so a login/refresh can't re-pin a picker choice), -``ANTON_FIRST_RUN_DONE`` (written by the CLI itself), and any comments. +DB is source of truth; the ``anton`` CLI still reads ``.env`` so a desktop install +re-exports the overlapping settings on write. Only ``SETTING_ENV_ALIASES`` keys are +managed — auth token, ``ANTON_*_MODEL`` pins (CLI-only per ENG-739), comments preserved. """ from __future__ import annotations @@ -30,17 +19,14 @@ UserSettings, ) -# The ANTON_* variables this export owns. A line for any of these is replaced on -# export; every other line in the file is left untouched. +# the ANTON_* vars this export owns; every other line is left untouched. MANAGED_ENV_VARS: tuple[str, ...] = tuple(SETTING_ENV_ALIASES.values()) def _env_str(value: object) -> str: """Format a loaded ``UserSettings`` value as its ``.env`` string. - Providers use the dash form the CLI expects (``minds-cloud``, not the DB's - ``minds_cloud``); secrets are the decrypted plaintext (``.env`` is plaintext - while the DB is Fernet-encrypted); booleans are lowercased. + Dash-form providers (``minds-cloud``), decrypted-plaintext secrets, lowercased bools. """ if isinstance(value, SecretStr): return value.get_secret_value() @@ -54,16 +40,10 @@ def _env_str(value: object) -> str: def build_env_export(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: - """Map the aliased settings that are actually STORED to ``{ANTON_VAR: value}``. - - Only keys in ``present_keys`` (i.e. that have a DB row) are exported. This is - deliberate: ``settings`` carries a resolved default for every unset field, so - exporting on non-``None`` alone would push the server's defaults (and their - drift) onto the CLI and would leave a line behind after a key is cleared. - Mirroring only stored rows keeps ``.env`` a faithful export of the DB and - lets the CLI apply its own defaults for anything unset. Unset/empty stored - values are still skipped. Model keys are absent from ``SETTING_ENV_ALIASES`` - (ENG-739), so they are never exported here. + """Map the STORED aliased settings to ``{ANTON_VAR: value}``. + + Only ``present_keys`` are exported — ``settings`` resolves a default for every + unset field, so exporting on non-``None`` alone would push server defaults to the CLI. """ out: dict[str, str] = {} for db_key, env_var in SETTING_ENV_ALIASES.items(): @@ -82,11 +62,8 @@ def build_env_export(settings: UserSettings, present_keys: set[str]) -> dict[str def merge_env_lines(existing: str, managed: dict[str, str]) -> str: """Rewrite the managed lines in ``existing``, preserving everything else. - Every line for a managed ANTON_* var is dropped, then the currently-set - managed vars are appended (in ``SETTING_ENV_ALIASES`` order, so repeated - exports of the same state produce identical output). Unmanaged lines keep - their place. A managed key absent from ``managed`` (unset or cleared) loses - its line — that is how a logout also wipes credentials from the CLI's file. + Managed ANTON_* lines are dropped then re-appended in alias order (byte-stable); + a key absent from ``managed`` loses its line — how logout wipes the CLI's creds. """ drop = tuple(f"{var}=" for var in MANAGED_ENV_VARS) kept = [ln for ln in existing.split("\n") if ln and not ln.startswith(drop)] @@ -97,9 +74,7 @@ def merge_env_lines(existing: str, managed: dict[str, str]) -> str: def atomic_write_env(path: Path, content: str) -> None: """Write ``content`` to ``path`` atomically, owner-only (0o600). - Temp file in the same directory + ``os.replace`` so a crash or a concurrent - CLI read never observes a truncated ``.env``. 0o600 because the file holds - plaintext API keys. + Temp file + ``os.replace`` so a crash/concurrent read never sees a truncated ``.env``; 0o600 for plaintext keys. """ path.parent.mkdir(parents=True, exist_ok=True) fd, tmp = tempfile.mkstemp(dir=str(path.parent), prefix=".env.", suffix=".tmp") diff --git a/cowork/migrations.py b/cowork/migrations.py index 67754e3d..dec3f25e 100644 --- a/cowork/migrations.py +++ b/cowork/migrations.py @@ -142,11 +142,9 @@ def migrate_env_to_db(session: Session) -> bool: if setting_key.endswith("_provider"): val = _normalize_provider_value(val, dotenv) try: - # export_env=False: this loop seeds the DB *from* .env; letting - # the per-key write re-export .env mid-loop would strip the keys - # not migrated yet (and blow them away on a crash). The DB is - # authoritative once seeding completes; the next real write - # exports the reconciled file (ENG-1127). + # 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: diff --git a/cowork/services/settings.py b/cowork/services/settings.py index 6fb182b0..bcfd78cc 100644 --- a/cowork/services/settings.py +++ b/cowork/services/settings.py @@ -164,11 +164,8 @@ def _write_row(self, key: str, store_val: str) -> None: def _after_write(self, *, export_env: bool = True) -> None: """Post-commit hook shared by the settings mutators. - Invalidates the settings cache and, on a single-user desktop install, - mirrors the DB out to the CLI's ``.env`` (ENG-1127). ``export_env=False`` - is for the startup ``.env``→DB migration, which must not rewrite the very - file it is seeding from — doing so mid-loop would strip the keys it has - not migrated yet. + 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: @@ -177,14 +174,8 @@ def _after_write(self, *, export_env: bool = True) -> None: def _export_env_for_cli(self) -> None: """Mirror the DB's aliased settings to the CLI's ``.env`` (best-effort). - The DB is the source of truth (ENG-1127); ``.env`` is a derived export - the standalone ``anton`` CLI reads. Only runs for ``local`` (single-user - desktop) tenancy — a multi-tenant cloud pod has no per-user ``.env`` or - CLI and must not spill decrypted secrets to disk. Re-derives the whole - managed set from the DB and merge-writes it, preserving every unmanaged - line (auth token, CLI-only model pins, comments). Skips the write when - the file is already current. Never raises into the write that triggered - it — a stale ``.env`` must not fail a settings save. + 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": diff --git a/tests/conftest.py b/tests/conftest.py index 33c6cd0b..d216ddbb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,9 +20,8 @@ # 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: cowork_home() backs the .env path. A settings write now mirrors -# the DB out to $COWORK_HOME/.env for the CLI (ENG-1127), so without this a -# settings-writing test would rewrite the developer's real ~/.cowork/.env. +# 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" diff --git a/tests/test_settings_env_export.py b/tests/test_settings_env_export.py index 1ae93c40..0974248e 100644 --- a/tests/test_settings_env_export.py +++ b/tests/test_settings_env_export.py @@ -1,8 +1,7 @@ -"""ENG-1127 Phase A: the server exports the DB's settings back to the CLI's .env. +"""ENG-1127 Phase A: the server exports the DB's settings to the CLI's .env. -Pure derivation (build_env_export / merge_env_lines) plus the SettingService -integration: a write mirrors to .env only for local tenancy, preserves unmanaged -lines (auth token, CLI model pins), and the .env→DB migration never re-exports. +Pure derivation plus the SettingService integration (local-tenancy-only, +preserves unmanaged lines, migration never re-exports). """ from types import SimpleNamespace @@ -22,25 +21,22 @@ def test_build_env_export_formats_and_excludes_models(): anthropic_api_key="sk-secret", planning_provider="minds_cloud", minds_url="https://mdb.example", - planning_model="latest:sonnet", # not aliased — must NOT be exported + planning_model="latest:sonnet", # not aliased (ENG-739) ) present = {"anthropic_api_key", "planning_provider", "minds_url", "planning_model"} out = build_env_export(s, present) assert out["ANTON_ANTHROPIC_API_KEY"] == "sk-secret" # decrypted plaintext - assert out["ANTON_PLANNING_PROVIDER"] == "minds-cloud" # ui_value (dash form) + assert out["ANTON_PLANNING_PROVIDER"] == "minds-cloud" # dash form assert out["ANTON_MINDS_URL"] == "https://mdb.example" - # Model keys are deliberately not in SETTING_ENV_ALIASES (ENG-739). assert not any("MODEL" in k for k in out) def test_build_env_export_only_exports_stored_keys(): - # minds_url has a non-None default, but with no row present it must not be - # exported — .env mirrors stored settings, not resolved defaults. + # minds_url has a non-None default but no row → must not be exported s = UserSettings(anthropic_api_key="sk-x") out = build_env_export(s, present_keys={"anthropic_api_key"}) assert out == {"ANTON_ANTHROPIC_API_KEY": "sk-x"} - assert "ANTON_MINDS_URL" not in out # defaulted but not stored - # Nothing stored at all → empty export. + assert "ANTON_MINDS_URL" not in out assert build_env_export(UserSettings(), present_keys=set()) == {} @@ -67,7 +63,7 @@ def test_merge_preserves_unmanaged_and_replaces_managed(): def test_merge_drops_cleared_managed_key(): existing = "COWORK_AUTH_TOKEN=tok\nANTON_MINDS_API_KEY=old\n" - merged = merge_env_lines(existing, {}) # nothing set → managed keys removed + merged = merge_env_lines(existing, {}) assert "ANTON_MINDS_API_KEY" not in merged assert "COWORK_AUTH_TOKEN=tok" in merged @@ -93,7 +89,6 @@ def _cleanup(session, *keys): def test_save_all_exports_env_and_preserves_unmanaged(local_export): env_path = local_export - # A pre-existing .env with an auth token and a CLI-only model pin. env_path.write_text("COWORK_AUTH_TOKEN=tok-1\nANTON_PLANNING_MODEL=latest:sonnet\n", encoding="utf-8") session = get_open_session() try: @@ -102,10 +97,10 @@ def test_save_all_exports_env_and_preserves_unmanaged(local_export): {"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 # decrypted secret + 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 # dash form - # Unmanaged lines the server must not touch. + assert "ANTON_PLANNING_PROVIDER=minds-cloud" in text + # unmanaged lines the server must not touch assert "COWORK_AUTH_TOKEN=tok-1" in text assert "ANTON_PLANNING_MODEL=latest:sonnet" in text assert oct(env_path.stat().st_mode)[-3:] == "600" @@ -150,8 +145,7 @@ def test_clear_credentials_wipes_creds_from_env_but_keeps_model(local_export): def test_migration_write_does_not_export(local_export): - # migrate_env_to_db seeds the DB *from* .env with export_env=False, so a - # per-key seed write must not rewrite (and mid-loop truncate) that .env. + # 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: From 3537f1310ac02df1cb13aac0a55e09077c86b175 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Wed, 29 Jul 2026 17:10:27 -0700 Subject: [PATCH 03/13] refactor(settings): consolidate the .env<->DB boundary into env_boundary.py (ENG-1127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move all .env-boundary knowledge out of user_settings.py into one module owning both directions: the alias maps (SETTING_ENV_ALIASES / ENV_ALIAS_TO_SETTING), normalize_provider_value, the inbound env_to_db_updates (extracted from the migration's inline map+normalize loops), the outbound db_to_env (was build_env_export), and the file I/O (merge_env_lines / atomic_write_env). user_settings.py is now purely the DB model; Provider.ui_value stays on the enum (used beyond .env). env_export.py is absorbed and removed. Pure reorg — no behavior change (env_to_db_updates reproduces the old inline loops exactly). Full server suite green apart from the pre-existing unrelated test_comments_layer::test_serve_injects_only_with_flag (fails on staging too). Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_boundary.py | 163 ++++++++++++++++++ cowork/common/settings/env_export.py | 91 ---------- cowork/common/settings/user_settings.py | 71 -------- cowork/migrations.py | 39 +---- cowork/services/settings.py | 6 +- ...ngs_env_export.py => test_env_boundary.py} | 12 +- tests/test_settings_schema.py | 24 +-- 7 files changed, 191 insertions(+), 215 deletions(-) create mode 100644 cowork/common/settings/env_boundary.py delete mode 100644 cowork/common/settings/env_export.py rename tests/{test_settings_env_export.py => test_env_boundary.py} (94%) diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py new file mode 100644 index 00000000..9143ad78 --- /dev/null +++ b/cowork/common/settings/env_boundary.py @@ -0,0 +1,163 @@ +"""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 os +import tempfile +from enum import Enum +from pathlib import Path + +from pydantic import SecretStr + +from cowork.common.settings.user_settings import Provider, UserSettings + +# 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 ANTON_* vars the outbound export owns; every other .env line is preserved. +MANAGED_ENV_VARS: tuple[str, ...] = tuple(SETTING_ENV_ALIASES.values()) + + +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 db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: + """The aliased settings that are actually STORED -> ``{ANTON_*: value}``. + + Only keys in ``present_keys`` (that have a DB row) export: ``settings`` carries + a resolved default for every unset field, so exporting on non-``None`` alone + would push server defaults onto the CLI and leave a stale line after a clear. + """ + out: dict[str, str] = {} + for db_key, env_var in SETTING_ENV_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 == "": + continue + out[env_var] = text + return out + + +def merge_env_lines(existing: str, managed: dict[str, str]) -> str: + """Rewrite the managed lines in ``existing``, preserving everything else. + + Managed ANTON_* lines are dropped then re-appended in alias order (byte-stable + across identical states); unmanaged lines (auth token, CLI model pins, + comments) keep their place. A managed key absent from ``managed`` loses its + line — that is how a logout wipes credentials from the CLI's file too. + """ + drop = tuple(f"{var}=" for var in MANAGED_ENV_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()) + return "\n".join(kept) + "\n" + + +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. + """ + path.parent.mkdir(parents=True, exist_ok=True) + 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) + os.replace(tmp, str(path)) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise diff --git a/cowork/common/settings/env_export.py b/cowork/common/settings/env_export.py deleted file mode 100644 index 99634bee..00000000 --- a/cowork/common/settings/env_export.py +++ /dev/null @@ -1,91 +0,0 @@ -"""Derive the CLI's ``.env`` from the DB settings (ENG-1127, Phase A). - -DB is source of truth; the ``anton`` CLI still reads ``.env`` so a desktop install -re-exports the overlapping settings on write. Only ``SETTING_ENV_ALIASES`` keys are -managed — auth token, ``ANTON_*_MODEL`` pins (CLI-only per ENG-739), comments preserved. -""" -from __future__ import annotations - -import os -import tempfile -from enum import Enum -from pathlib import Path - -from pydantic import SecretStr - -from cowork.common.settings.user_settings import ( - SETTING_ENV_ALIASES, - Provider, - UserSettings, -) - -# the ANTON_* vars this export owns; every other line is left untouched. -MANAGED_ENV_VARS: tuple[str, ...] = tuple(SETTING_ENV_ALIASES.values()) - - -def _env_str(value: object) -> str: - """Format a loaded ``UserSettings`` value as its ``.env`` string. - - Dash-form providers (``minds-cloud``), decrypted-plaintext secrets, lowercased bools. - """ - 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 build_env_export(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: - """Map the STORED aliased settings to ``{ANTON_VAR: value}``. - - Only ``present_keys`` are exported — ``settings`` resolves a default for every - unset field, so exporting on non-``None`` alone would push server defaults to the CLI. - """ - out: dict[str, str] = {} - for db_key, env_var in SETTING_ENV_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 == "": - continue - out[env_var] = text - return out - - -def merge_env_lines(existing: str, managed: dict[str, str]) -> str: - """Rewrite the managed lines in ``existing``, preserving everything else. - - Managed ANTON_* lines are dropped then re-appended in alias order (byte-stable); - a key absent from ``managed`` loses its line — how logout wipes the CLI's creds. - """ - drop = tuple(f"{var}=" for var in MANAGED_ENV_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()) - return "\n".join(kept) + "\n" - - -def atomic_write_env(path: Path, content: str) -> None: - """Write ``content`` to ``path`` atomically, owner-only (0o600). - - Temp file + ``os.replace`` so a crash/concurrent read never sees a truncated ``.env``; 0o600 for plaintext keys. - """ - path.parent.mkdir(parents=True, exist_ok=True) - 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) - os.replace(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..37ff9726 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 diff --git a/cowork/migrations.py b/cowork/migrations.py index dec3f25e..cd0dd471 100644 --- a/cowork/migrations.py +++ b/cowork/migrations.py @@ -34,11 +34,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 +52,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 +77,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 +84,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,12 +113,7 @@ 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: # 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 @@ -148,7 +121,7 @@ def migrate_env_to_db(session: Session) -> bool: 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( diff --git a/cowork/services/settings.py b/cowork/services/settings.py index bcfd78cc..bbf3175c 100644 --- a/cowork/services/settings.py +++ b/cowork/services/settings.py @@ -9,9 +9,9 @@ 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_export import ( +from cowork.common.settings.env_boundary import ( atomic_write_env, - build_env_export, + db_to_env, merge_env_lines, ) from cowork.common.settings.user_settings import ( @@ -181,7 +181,7 @@ def _export_env_for_cli(self) -> None: if get_app_settings().tenancy_mode != "local": return rows = self._fetch_all_rows() - managed = build_env_export(self._load(rows), {row.key for row in rows}) + managed = db_to_env(self._load(rows), {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) diff --git a/tests/test_settings_env_export.py b/tests/test_env_boundary.py similarity index 94% rename from tests/test_settings_env_export.py rename to tests/test_env_boundary.py index 0974248e..2ccd0710 100644 --- a/tests/test_settings_env_export.py +++ b/tests/test_env_boundary.py @@ -8,7 +8,7 @@ import pytest import cowork.services.settings as settings_mod -from cowork.common.settings.env_export import build_env_export, merge_env_lines +from cowork.common.settings.env_boundary import db_to_env, merge_env_lines from cowork.common.settings.user_settings import UserSettings from cowork.db.session import get_open_session from cowork.services.settings import SettingService @@ -16,7 +16,7 @@ # ── pure derivation ──────────────────────────────────────────────────── -def test_build_env_export_formats_and_excludes_models(): +def test_db_to_env_formats_and_excludes_models(): s = UserSettings( anthropic_api_key="sk-secret", planning_provider="minds_cloud", @@ -24,20 +24,20 @@ def test_build_env_export_formats_and_excludes_models(): planning_model="latest:sonnet", # not aliased (ENG-739) ) present = {"anthropic_api_key", "planning_provider", "minds_url", "planning_model"} - out = build_env_export(s, present) + out = db_to_env(s, present) assert out["ANTON_ANTHROPIC_API_KEY"] == "sk-secret" # decrypted plaintext assert out["ANTON_PLANNING_PROVIDER"] == "minds-cloud" # dash form assert out["ANTON_MINDS_URL"] == "https://mdb.example" assert not any("MODEL" in k for k in out) -def test_build_env_export_only_exports_stored_keys(): +def test_db_to_env_only_exports_stored_keys(): # minds_url has a non-None default but no row → must not be exported s = UserSettings(anthropic_api_key="sk-x") - out = build_env_export(s, present_keys={"anthropic_api_key"}) + out = db_to_env(s, present_keys={"anthropic_api_key"}) assert out == {"ANTON_ANTHROPIC_API_KEY": "sk-x"} assert "ANTON_MINDS_URL" not in out - assert build_env_export(UserSettings(), present_keys=set()) == {} + assert db_to_env(UserSettings(), present_keys=set()) == {} def test_merge_preserves_unmanaged_and_replaces_managed(): 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": ""}) == {} From 435b0b7fc23d99c7dccd9786091be74d9e7e5e18 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Wed, 29 Jul 2026 18:17:13 -0700 Subject: [PATCH 04/13] fix(settings): export .env after minds-url backfill + serialize the export (ENG-1127 review) Two code-review findings on the .env exporter: - backfill_minds_url wrote minds_url directly and only invalidated the cache, so the DB moved to the canonical MindsHub host while the CLI's .env kept the dead mdb.ai one. Route it through the exporting hook (svc._after_write) so both stores update. Regression test asserts both. - the export's read/merge/write wasn't serialized, so two concurrent settings writes could lost-update .env. Guard it with a module-level lock, re-reading the DB inside the lock so the last exporter installs the latest committed state. Regression test asserts the critical section admits one writer at a time. (Cross-process competition with the client's own .env writes is transient and self-heals via the DB re-derive; Phase B removes the client writer.) Full server suite green (702 passed) apart from the pre-existing unrelated test_comments_layer failure. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/migrations.py | 6 ++-- cowork/services/settings.py | 25 ++++++++++----- tests/test_env_boundary.py | 61 +++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 9 deletions(-) diff --git a/cowork/migrations.py b/cowork/migrations.py index cd0dd471..3c7d00ec 100644 --- a/cowork/migrations.py +++ b/cowork/migrations.py @@ -33,7 +33,6 @@ 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.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 @@ -173,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 bbf3175c..cce8356d 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 @@ -23,6 +24,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 '***'. @@ -180,13 +187,17 @@ def _export_env_for_cli(self) -> None: try: if get_app_settings().tenancy_mode != "local": return - rows = self._fetch_all_rows() - managed = db_to_env(self._load(rows), {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) - if content != existing: - atomic_write_env(path, content) + # 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() + managed = db_to_env(self._load(rows), {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) + 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) diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py index 2ccd0710..a2e2f1e9 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -155,3 +155,64 @@ def test_migration_write_does_not_export(local_export): 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") + svc = SettingService(session) + 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") + 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 From 653c1791763eeb83f3c9473d610d92a30990459f Mon Sep 17 00:00:00 2001 From: pnewsam Date: Mon, 3 Aug 2026 14:43:56 -0700 Subject: [PATCH 05/13] fix(settings): harden the .env export against Windows share-mode locks (ENG-1127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI (or a version-skewed server) holding ~/.cowork/.env open makes the final os.replace raise a transient PermissionError on Windows — the exact EPERM that wedged onboarding on the client before it grew a retry (ENG-1209). ENG-1127 moves .env writing to the server (Phase B removes the client's hardened writer), so that hardening has to live here too or the bug re-opens on the server side. atomic_write_env now retries the rename on transient lock errnos (EPERM/EACCES/EBUSY/ENOTEMPTY) with a widening backoff mirroring the client's retryOnTransientLock, and sweeps stale orphaned .env.*.tmp files (they hold the full plaintext key) while sparing a concurrent writer's fresh temp. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_boundary.py | 69 +++++++++++++++++++++++++- tests/test_env_boundary.py | 65 ++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-) diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py index 9143ad78..74185f44 100644 --- a/cowork/common/settings/env_boundary.py +++ b/cowork/common/settings/env_boundary.py @@ -16,8 +16,11 @@ """ from __future__ import annotations +import errno +import logging import os import tempfile +import time from enum import Enum from pathlib import Path @@ -142,19 +145,81 @@ def merge_env_lines(existing: str, managed: dict[str, str]) -> str: return "\n".join(kept) + "\n" +logger = logging.getLogger(__name__) + +# 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. + 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) - os.replace(tmp, str(path)) + _replace_with_retry(tmp, str(path)) except BaseException: try: os.unlink(tmp) diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py index a2e2f1e9..00d59224 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -3,11 +3,14 @@ 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.common.settings.user_settings import UserSettings from cowork.db.session import get_open_session @@ -216,3 +219,65 @@ def export(): 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 From a1a9c6baf6773139b2b7c8999ef47948cd78e82f Mon Sep 17 00:00:00 2001 From: pnewsam Date: Mon, 3 Aug 2026 15:20:17 -0700 Subject: [PATCH 06/13] fix(settings): reject dotenv injection via CR/LF in exported values (ENG-1127 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CR/LF in an exported value (e.g. a poisoned minds_url like "https://x\nDATABASE_URI=…") would terminate the ANTON_* assignment and turn the remainder into a second, unmanaged line that survives every later merge and is consumed on the next CLI/server start. The exported fields never legitimately contain a newline, so db_to_env now drops such a value (best-effort, logged) and merge_env_lines skips it as a serialization invariant. Adds round-trip/injection regression tests, incl. an end-to-end save that confirms the settings layer does not itself block newlines — so this guard is load-bearing. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_boundary.py | 30 ++++++++++++++++++--- tests/test_env_boundary.py | 36 ++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py index 74185f44..e50675aa 100644 --- a/cowork/common/settings/env_boundary.py +++ b/cowork/common/settings/env_boundary.py @@ -28,6 +28,8 @@ from cowork.common.settings.user_settings import Provider, UserSettings +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). @@ -110,6 +112,19 @@ def _env_str(value: object) -> str: 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 + + def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: """The aliased settings that are actually STORED -> ``{ANTON_*: value}``. @@ -127,6 +142,11 @@ def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: text = _env_str(value) if text == "": continue + if not _is_dotenv_safe(text): + # A newline-bearing value is a dotenv-injection vector, never a real + # key/URL — drop it rather than corrupt the file (best-effort export). + logger.warning("settings: refusing to export %s — value spans multiple lines", env_var) + continue out[env_var] = text return out @@ -138,15 +158,19 @@ def merge_env_lines(existing: str, managed: dict[str, str]) -> str: across identical states); unmanaged lines (auth token, CLI model pins, comments) keep their place. A managed key absent from ``managed`` loses its line — that is how a logout wipes credentials from the CLI's file too. + + 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 = tuple(f"{var}=" for var in MANAGED_ENV_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()) + kept.extend( + f"{var}={value}" for var, value in managed.items() if _is_dotenv_safe(value) + ) return "\n".join(kept) + "\n" -logger = logging.getLogger(__name__) - # 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 diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py index 00d59224..58fd9d9f 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -71,6 +71,42 @@ def test_merge_drops_cleared_managed_key(): 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_newline_bearing_value(monkeypatch): + s = UserSettings() + monkeypatch.setattr(s, "minds_url", "https://x\nDATABASE_URI=sqlite:///evil.db", raising=False) + out = db_to_env(s, present_keys={"minds_url"}) + assert "ANTON_MINDS_URL" not in out # poisoned value refused, not exported + + +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 as a 2nd line. + 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=sk-clean" in text # the clean sibling still lands + finally: + _cleanup(session, "minds_url", "minds_api_key") + session.close() + + # ── SettingService integration ───────────────────────────────────────── @pytest.fixture From de28960b2db4d729938b09347bcc7c3c5426c656 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Mon, 3 Aug 2026 15:49:31 -0700 Subject: [PATCH 07/13] =?UTF-8?q?fix(settings):=20export=20a=20runnable=20?= =?UTF-8?q?CLI=20config=20=E2=80=94=20resolved=20provider+model,=20gemini?= =?UTF-8?q?=20translated=20(ENG-1127=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DB->.env export was a naive field dump, which produced CLI configs the pinned anton (c51afe87) cannot run (review findings 1 & 2): - A provider was exported without its model (models are excluded from the alias map, ENG-739), so switching the DB provider left a stale, mismatched ANTON_*_MODEL line — e.g. provider=openai against a leftover Claude model. - provider=gemini was written literally, but anton has no first-class gemini provider (from_settings raises "Unknown planning provider: gemini"); it runs Gemini as openai-compatible + Google's base URL + the key in the OpenAI slot. ANTON_GEMINI_API_KEY / ANTON_OPENAI_API_KEY_CUSTOM are fields anton doesn't read, so exporting them was a silent no-op. db_to_env now renders the provider/model/key/base cluster in anton's on-disk vocabulary using the SAME resolution the server's own build_llm_client applies (resolved_*_provider/model + provider_base_url + provider_api_key): each role's provider is written WITH its resolved model (the pair is always valid), gemini is translated to openai-compatible + GEMINI_BASE_URL + the OpenAI key slot, and a role whose resolved provider has no key exports nothing. The per-role model vars are now MANAGED so a stale/orphaned model line is reconciled away. Verified all three provider shapes round-trip through the pinned anton's from_settings. Non-provider settings (memory flags, publish URL) keep the straight present-gated alias. Tests updated for the resolved+paired behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_boundary.py | 147 +++++++++++++++++++++---- tests/test_env_boundary.py | 90 +++++++++------ 2 files changed, 186 insertions(+), 51 deletions(-) diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py index e50675aa..9697c8ab 100644 --- a/cowork/common/settings/env_boundary.py +++ b/cowork/common/settings/env_boundary.py @@ -26,7 +26,11 @@ from pydantic import SecretStr -from cowork.common.settings.user_settings import Provider, UserSettings +from cowork.common.settings.user_settings import ( + Provider, + UserSettings, + provider_api_key_str, +) logger = logging.getLogger(__name__) @@ -55,8 +59,24 @@ # 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. -MANAGED_ENV_VARS: tuple[str, ...] = tuple(SETTING_ENV_ALIASES.values()) +# 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: @@ -125,30 +145,117 @@ def _is_dotenv_safe(value: str) -> bool: return "\n" not in value and "\r" not in value -def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: - """The aliased settings that are actually STORED -> ``{ANTON_*: 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", +} + + +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 _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. + + openai / openai-compatible / gemini share the single ``ANTON_OPENAI_API_KEY`` / + ``ANTON_OPENAI_BASE_URL`` slots (as they do in AntonSettings); minds-cloud uses + the dedicated ``minds_*`` slots and derives its base from them. Uses + ``setdefault`` so the first (highest-priority, planning-first) provider wins + the shared OpenAI slot — Anton can't serve two different OpenAI-compatible + endpoints at once, so a mixed cross-role config resolves deterministically. + """ + from cowork.services.providers import provider_base_url # lazy: avoid import cycle + + key = provider_api_key_str(settings, p) + if p is Provider.ANTHROPIC: + if key: + out.setdefault("ANTON_ANTHROPIC_API_KEY", key) + elif p is Provider.MINDS_CLOUD: + if key: + out.setdefault("ANTON_MINDS_API_KEY", key) + if settings.minds_url: + out.setdefault("ANTON_MINDS_URL", settings.minds_url) + else: # OPENAI / OPENAI_COMPATIBLE / GEMINI — all via the OpenAI slot + if key: + out.setdefault("ANTON_OPENAI_API_KEY", key) + base = provider_base_url(p.ui_value, openai_base_url=settings.openai_base_url or "") + if base: + out.setdefault("ANTON_OPENAI_BASE_URL", base) + - Only keys in ``present_keys`` (that have a DB row) export: ``settings`` carries - a resolved default for every unset field, so exporting on non-``None`` alone - would push server defaults onto the CLI and leave a stale line after a clear. +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. + + The remaining settings (memory flags, publish URL) have no cross-field + resolution and export by a straight present-gated alias. """ out: dict[str, str] = {} - for db_key, env_var in SETTING_ENV_ALIASES.items(): + + 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), + ) + creds_order: list[Provider] = [] # planning-first: wins the shared OpenAI slot + 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 + out[prov_var] = _anton_provider_name(prov) + if model: + out[model_var] = model + if prov not in creds_order: + creds_order.append(prov) + for prov in creds_order: + _emit_provider_creds(out, settings, prov) + + 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 == "": - continue - if not _is_dotenv_safe(text): - # A newline-bearing value is a dotenv-injection vector, never a real - # key/URL — drop it rather than corrupt the file (best-effort export). - logger.warning("settings: refusing to export %s — value spans multiple lines", env_var) - continue - out[env_var] = text - return out + if value is not None: + text = _env_str(value) + if text: + out[env_var] = text + + # One injection guard over everything emitted (keys, URLs, models, flags): a + # CR/LF-bearing value is a dotenv-injection vector, never a real setting. + safe: dict[str, str] = {} + for var, val in out.items(): + if _is_dotenv_safe(val): + safe[var] = val + else: + logger.warning("settings: refusing to export %s — value spans multiple lines", var) + return safe def merge_env_lines(existing: str, managed: dict[str, str]) -> str: diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py index 58fd9d9f..9b0fabf0 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -19,45 +19,58 @@ # ── pure derivation ──────────────────────────────────────────────────── -def test_db_to_env_formats_and_excludes_models(): - s = UserSettings( - anthropic_api_key="sk-secret", - planning_provider="minds_cloud", - minds_url="https://mdb.example", - planning_model="latest:sonnet", # not aliased (ENG-739) - ) - present = {"anthropic_api_key", "planning_provider", "minds_url", "planning_model"} +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_ANTHROPIC_API_KEY"] == "sk-secret" # decrypted plaintext - assert out["ANTON_PLANNING_PROVIDER"] == "minds-cloud" # dash form + 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 not any("MODEL" in k for k in out) + 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"].startswith("https://generativelanguage.googleapis.com") + 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_only_exports_stored_keys(): - # minds_url has a non-None default but no row → must not be exported - s = UserSettings(anthropic_api_key="sk-x") - out = db_to_env(s, present_keys={"anthropic_api_key"}) - assert out == {"ANTON_ANTHROPIC_API_KEY": "sk-x"} - assert "ANTON_MINDS_URL" not in out +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" + "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") - # Unmanaged lines survive verbatim. + # Genuinely unmanaged lines survive verbatim. assert "COWORK_AUTH_TOKEN=tok-123" in lines - assert "ANTON_PLANNING_MODEL=latest:sonnet" 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 @@ -85,10 +98,13 @@ def test_merge_env_lines_rejects_crlf_injection(): def test_db_to_env_drops_newline_bearing_value(monkeypatch): - s = UserSettings() + # A minds-cloud user so minds_url is genuinely part of the export, then poison + # it — the injection guard (not a missing key) is what must drop it. + 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_url"}) - assert "ANTON_MINDS_URL" not in out # poisoned value refused, not exported + out = db_to_env(s, present_keys={"minds_api_key", "planning_provider"}) + assert "ANTON_MINDS_API_KEY" in out # the clean sibling still exports + assert "ANTON_MINDS_URL" not in out # poisoned value refused by the guard def test_export_never_writes_injected_line_end_to_end(local_export): @@ -139,9 +155,12 @@ def test_save_all_exports_env_and_preserves_unmanaged(local_export): 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 - # unmanaged lines the server must not touch + # Genuinely unmanaged lines the server must not touch. assert "COWORK_AUTH_TOKEN=tok-1" in text - assert "ANTON_PLANNING_MODEL=latest:sonnet" 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") @@ -161,9 +180,9 @@ def test_export_skipped_for_org_tenancy(monkeypatch, tmp_path): session.close() -def test_clear_credentials_wipes_creds_from_env_but_keeps_model(local_export): +def test_clear_credentials_wipes_creds_and_orphaned_model_from_env(local_export): env_path = local_export - env_path.write_text("ANTON_PLANNING_MODEL=latest:sonnet\n", encoding="utf-8") + 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") @@ -171,13 +190,18 @@ def test_clear_credentials_wipes_creds_from_env_but_keeps_model(local_export): svc.save_all( {"minds_api_key": "sk-abc", "minds_url": "https://mdb", "planning_provider": "minds_cloud"} ) - assert "ANTON_MINDS_API_KEY=sk-abc" in env_path.read_text(encoding="utf-8") + 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 - assert "ANTON_PLANNING_MODEL=latest:sonnet" in text # CLI model pin survives + # 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() @@ -207,8 +231,12 @@ def test_backfill_minds_url_exports_to_env_too(local_export): canonical = default_minds_api_host() session = get_open_session() try: - _cleanup(session, "minds_url") + _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") @@ -219,7 +247,7 @@ def test_backfill_minds_url_exports_to_env_too(local_export): assert f"ANTON_MINDS_URL={canonical}" in env_text assert "https://mdb.ai" not in env_text finally: - _cleanup(session, "minds_url") + _cleanup(session, "minds_url", "minds_api_key", "planning_provider") session.close() From b425358a9054a4f0e19f055a02b45578d80d4c56 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Mon, 3 Aug 2026 16:26:05 -0700 Subject: [PATCH 08/13] fix(settings): don't export mixed-provider configs the CLI can't represent (ENG-1127 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The product runs mixed per-role providers via independent per-role LLM clients (build_llm_client derives each role's base URL separately), but the standalone anton CLI has a SINGLE global ANTON_OPENAI_API_KEY/ANTON_OPENAI_BASE_URL pair handed to both its openai and openai-compatible factories, and its MindsHub derivation only fires when that OpenAI key is unset. So planning=OpenAI + coding=Gemini would export OpenAI's key against Google's base (misrouting both roles), and MindsHub + OpenAI would break the Minds derivation. db_to_env now checks representability before emitting the provider cluster: every OpenAI-slot role must agree on the same (key, base), and minds-cloud must not coexist with an explicit OpenAI-slot role. A non-representable config exports NO provider/model/creds (logged) — leaving the CLI on its own config rather than a silently-misrouting one. Representable mixes that use independent slots (e.g. anthropic + gemini) still export both roles. Adds mixed-provider regression cases for both the rejected and the allowed shapes. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_boundary.py | 92 ++++++++++++++++++++------ tests/test_env_boundary.py | 37 +++++++++++ 2 files changed, 107 insertions(+), 22 deletions(-) diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py index 9697c8ab..696cbec4 100644 --- a/cowork/common/settings/env_boundary.py +++ b/cowork/common/settings/env_boundary.py @@ -170,33 +170,64 @@ def _anton_provider_name(p: Provider) -> str: 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`` for providers that don't touch that slot (anthropic uses its own + key slot; minds-cloud uses the dedicated ``minds_*`` slots and derives the + OpenAI creds in ``model_post_init``). openai / openai-compatible / gemini all + ride ``ANTON_OPENAI_API_KEY`` / ``ANTON_OPENAI_BASE_URL``, so their demands + must AGREE for a config to be representable — see ``_env_representable``. + """ + if p in (Provider.ANTHROPIC, Provider.MINDS_CLOUD): + return None + from cowork.services.providers import provider_base_url # lazy: avoid import cycle + 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 its minds + derivation only fires when that OpenAI key is unset. So a config is + representable only when every OpenAI-slot role agrees on the same ``(key, + base)`` AND minds-cloud never coexists with an explicit OpenAI-slot role + (planning=OpenAI + coding=Gemini, or MindsHub + OpenAI, otherwise silently + misroute one role's key to the other's endpoint — ENG-1127 review). + """ + openai_demands = {d for p in providers if (d := _openai_slot_demand(settings, p)) is not None} + minds_present = Provider.MINDS_CLOUD in providers + return len(openai_demands) <= 1 and not (minds_present and openai_demands) + + 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. - openai / openai-compatible / gemini share the single ``ANTON_OPENAI_API_KEY`` / - ``ANTON_OPENAI_BASE_URL`` slots (as they do in AntonSettings); minds-cloud uses - the dedicated ``minds_*`` slots and derives its base from them. Uses - ``setdefault`` so the first (highest-priority, planning-first) provider wins - the shared OpenAI slot — Anton can't serve two different OpenAI-compatible - endpoints at once, so a mixed cross-role config resolves deterministically. + Only called for a representable set (see ``_env_representable``), so the + OpenAI slot is never contended; minds-cloud uses the dedicated ``minds_*`` + slots and derives its base from them. """ from cowork.services.providers import provider_base_url # lazy: avoid import cycle key = provider_api_key_str(settings, p) if p is Provider.ANTHROPIC: if key: - out.setdefault("ANTON_ANTHROPIC_API_KEY", key) + out["ANTON_ANTHROPIC_API_KEY"] = key elif p is Provider.MINDS_CLOUD: if key: - out.setdefault("ANTON_MINDS_API_KEY", key) + out["ANTON_MINDS_API_KEY"] = key if settings.minds_url: - out.setdefault("ANTON_MINDS_URL", settings.minds_url) + out["ANTON_MINDS_URL"] = settings.minds_url else: # OPENAI / OPENAI_COMPATIBLE / GEMINI — all via the OpenAI slot if key: - out.setdefault("ANTON_OPENAI_API_KEY", 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.setdefault("ANTON_OPENAI_BASE_URL", base) + out["ANTON_OPENAI_BASE_URL"] = base def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: @@ -213,6 +244,11 @@ def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: 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. """ @@ -226,17 +262,29 @@ def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: ("ANTON_ROUTER_PROVIDER", "ANTON_ROUTER_MODEL", settings.resolved_router_provider, settings.resolved_router_model), ) - creds_order: list[Provider] = [] # planning-first: wins the shared OpenAI slot - 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 - out[prov_var] = _anton_provider_name(prov) - if model: - out[model_var] = model - if prov not in creds_order: - creds_order.append(prov) - for prov in creds_order: - _emit_provider_creds(out, settings, prov) + keyed_roles = [ + (prov_var, model_var, prov, model) + for prov_var, model_var, prov, model in roles + if provider_api_key_str(settings, prov) # resolved provider has a key → runnable + ] + unique_providers: list[Provider] = [] + for _, _, prov, _ in keyed_roles: + if prov not in unique_providers: + unique_providers.append(prov) # planning-first order preserved + + if _env_representable(settings, unique_providers): + for prov_var, model_var, prov, model in keyed_roles: + out[prov_var] = _anton_provider_name(prov) + if model: + out[model_var] = model + for prov in unique_providers: + _emit_provider_creds(out, settings, prov) + elif unique_providers: + logger.warning( + "settings: skipping .env provider export — roles resolve to providers the " + "standalone CLI cannot represent together (%s); leaving the CLI on its own config", + ", ".join(p.value for p in unique_providers), + ) for db_key, env_var in _OUTBOUND_FLAG_ALIASES.items(): if db_key not in present_keys: diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py index 9b0fabf0..fefe340a 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -46,6 +46,43 @@ def test_db_to_env_translates_gemini_to_openai_compatible(): 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"].startswith("https://generativelanguage") + + +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 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()) == {} From 76eac509b07428c485435dc50ec4d43dee3a29b0 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Mon, 3 Aug 2026 16:29:56 -0700 Subject: [PATCH 09/13] test(settings): assert gemini base URL by equality, not prefix (CodeQL) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL's incomplete-url-substring-sanitization flags any `.startswith("https://…")` prefix check on a URL host. These are test assertions, but the fix is also cleaner: compare the exported base URL for exact equality against the canonical GEMINI_BASE_URL constant instead of a prefix substring. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_env_boundary.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py index fefe340a..aa66abff 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -12,6 +12,7 @@ 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 @@ -41,7 +42,7 @@ def test_db_to_env_translates_gemini_to_openai_compatible(): 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"].startswith("https://generativelanguage.googleapis.com") + 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", "") @@ -57,7 +58,7 @@ def test_db_to_env_allows_representable_mixed_providers(): 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"].startswith("https://generativelanguage") + assert out["ANTON_OPENAI_BASE_URL"] == GEMINI_BASE_URL def test_db_to_env_skips_unrepresentable_openai_gemini_mix(): From dbe81f78c8dc9b1c7f3592e33cd01c2c1c4bfaf9 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Mon, 3 Aug 2026 17:03:29 -0700 Subject: [PATCH 10/13] fix(settings): export the OpenAI slot for router-only Minds; don't wipe a valid .env on unrepresentable saves (ENG-1127 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Major edge cases from review: 1. Router-only MindsHub. The export trusted Anton's model_post_init to derive the OpenAI slot from the minds_* creds, but pinned Anton (c51afe87) only derives when the PLANNING or CODING provider is openai-compatible — never a router-only Minds role. So planning/coding=Anthropic + router=MindsHub built the router with no key/base. Fix: fold minds-cloud into the shared-OpenAI-slot model and export ANTON_OPENAI_API_KEY / ANTON_OPENAI_BASE_URL EXPLICITLY (minds key + minds_chat_base_url), keeping the minds_* slots too. Verified the router-only shape round-trips through the pinned Anton's from_settings. This also simplifies representability to a single rule (all OpenAI-slot roles must agree). 2. Unrepresentable save wiped a valid .env. Returning no provider cluster let merge_env_lines delete every managed provider/model/cred line, wiping a previously-valid standalone CLI config on the next save. Fix: env_reconcile_vars narrows the merge's drop-set to the flag vars when the config is unrepresentable, PRESERVING the existing cluster; a genuinely cleared config (no keys) is still representable, so logout still wipes creds. merge_env_lines also drops any var it is about to write, preventing duplicates. Added an end-to-end test starting from a pre-populated .env plus the router-only round-trip test. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/common/settings/env_boundary.py | 126 +++++++++++++++++-------- cowork/services/settings.py | 6 +- tests/test_env_boundary.py | 69 ++++++++++++++ 3 files changed, 162 insertions(+), 39 deletions(-) diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py index 696cbec4..55b87a05 100644 --- a/cowork/common/settings/env_boundary.py +++ b/cowork/common/settings/env_boundary.py @@ -158,6 +158,11 @@ def _is_dotenv_safe(value: str) -> bool: "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. @@ -173,15 +178,23 @@ def _anton_provider_name(p: Provider) -> str: 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`` for providers that don't touch that slot (anthropic uses its own - key slot; minds-cloud uses the dedicated ``minds_*`` slots and derives the - OpenAI creds in ``model_post_init``). openai / openai-compatible / gemini all - ride ``ANTON_OPENAI_API_KEY`` / ``ANTON_OPENAI_BASE_URL``, so their demands - must AGREE for a config to be representable — see ``_env_representable``. + ``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 in (Provider.ANTHROPIC, Provider.MINDS_CLOUD): + if p is Provider.ANTHROPIC: return None - from cowork.services.providers import provider_base_url # lazy: avoid import cycle + 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 ""), @@ -192,36 +205,43 @@ def _env_representable(settings: UserSettings, providers: list[Provider]) -> boo """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 its minds - derivation only fires when that OpenAI key is unset. So a config is - representable only when every OpenAI-slot role agrees on the same ``(key, - base)`` AND minds-cloud never coexists with an explicit OpenAI-slot role - (planning=OpenAI + coding=Gemini, or MindsHub + OpenAI, otherwise silently - misroute one role's key to the other's endpoint — ENG-1127 review). + 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} - minds_present = Provider.MINDS_CLOUD in providers - return len(openai_demands) <= 1 and not (minds_present and openai_demands) + 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 - OpenAI slot is never contended; minds-cloud uses the dedicated ``minds_*`` - slots and derives its base from them. + shared OpenAI slot is never contended. """ - from cowork.services.providers import provider_base_url # lazy: avoid import cycle + 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 @@ -230,6 +250,21 @@ def _emit_provider_creds(out: dict[str, str], settings: UserSettings, p: Provide 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 + + def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: """A loaded ``UserSettings`` -> the ``{ANTON_*: value}`` the CLI can RUN. @@ -262,27 +297,24 @@ def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: ("ANTON_ROUTER_PROVIDER", "ANTON_ROUTER_MODEL", settings.resolved_router_provider, settings.resolved_router_model), ) - keyed_roles = [ - (prov_var, model_var, prov, model) - for prov_var, model_var, prov, model in roles - if provider_api_key_str(settings, prov) # resolved provider has a key → runnable - ] - unique_providers: list[Provider] = [] - for _, _, prov, _ in keyed_roles: - if prov not in unique_providers: - unique_providers.append(prov) # planning-first order preserved + unique_providers = _keyed_unique_providers(settings) if _env_representable(settings, unique_providers): - for prov_var, model_var, prov, model in keyed_roles: + 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 out[prov_var] = _anton_provider_name(prov) if model: out[model_var] = model for prov in unique_providers: _emit_provider_creds(out, settings, prov) elif unique_providers: + # The provider cluster is NOT written; env_reconcile_vars keeps it out of + # the merge's drop-set too, so a previously-valid CLI config is preserved + # rather than wiped (ENG-1127 review). logger.warning( - "settings: skipping .env provider export — roles resolve to providers the " - "standalone CLI cannot represent together (%s); leaving the CLI on its own config", + "settings: not exporting the .env provider cluster — roles resolve to providers " + "the standalone CLI cannot represent together (%s); preserving the CLI's existing config", ", ".join(p.value for p in unique_providers), ) @@ -306,19 +338,39 @@ def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: return safe -def merge_env_lines(existing: str, managed: dict[str, str]) -> str: - """Rewrite the managed lines in ``existing``, preserving everything else. +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) is representable, so its cluster still + reconciles away — that is how a logout still wipes the CLI's credentials + (ENG-1127 review). + """ + if _env_representable(settings, _keyed_unique_providers(settings)): + 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. - Managed ANTON_* lines are dropped then re-appended in alias order (byte-stable - across identical states); unmanaged lines (auth token, CLI model pins, - comments) keep their place. A managed key absent from ``managed`` loses its - line — that is how a logout wipes credentials from the CLI's file too. + 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 = tuple(f"{var}=" for var in MANAGED_ENV_VARS) + 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) diff --git a/cowork/services/settings.py b/cowork/services/settings.py index cce8356d..2d04542f 100644 --- a/cowork/services/settings.py +++ b/cowork/services/settings.py @@ -13,6 +13,7 @@ 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 ( @@ -192,10 +193,11 @@ def _export_env_for_cli(self) -> None: # state — no lost update between concurrent settings writes. with _env_export_lock: rows = self._fetch_all_rows() - managed = db_to_env(self._load(rows), {row.key for row in 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) + 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 diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py index aa66abff..fc9b971b 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -84,6 +84,46 @@ def test_db_to_env_skips_unrepresentable_minds_plus_openai_mix(): 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_exports_nothing_when_unconfigured(): # No key anywhere → no provider/model/creds; flags export only when stored. assert db_to_env(UserSettings(), present_keys=set()) == {} @@ -245,6 +285,35 @@ def test_clear_credentials_wipes_creds_and_orphaned_model_from_env(local_export) 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 From d39e661b7e199f99f876c0d0e45e443b346d0b76 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Mon, 3 Aug 2026 17:33:47 -0700 Subject: [PATCH 11/13] fix(settings): stop stale .env round-tripping into the DB; drop baseless clusters; pair router with its own model (ENG-1127 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three Major findings: 1. /settings/raw synced the WHOLE merged .env back into the DB, so a preserved or translated cluster (a stale minds-cloud line, or a gemini role exported as openai-compatible) would overwrite the authoritative DB choice on the next unrelated raw write (OAuth/token refresh). It now syncs ONLY the recognised vars present in the incoming request — the exact root cause the client already worked around by moving sign-in off /raw (ENG-739). Models still never sync. 2. An openai-compatible/minds provider with no base URL passed representability, and the per-field CR/LF filter could drop a base while keeping provider+key — Anton then defaults to https://api.openai.com/v1/ and leaks a Minds/custom key to OpenAI. The provider cluster is now built and validated ATOMICALLY (_provider_cluster): a base-requiring provider with no base, or any non-dotenv-safe value, drops the WHOLE cluster (and preserves the existing .env), never a lone field. env_reconcile_vars keys off the same decision so "wrote nothing" and "preserve" can't disagree. 3. apply_model_defaults derived an absent router_model from coding_provider, so coding=Anthropic + router=OpenAI exported ANTON_ROUTER_PROVIDER=openai with a Claude model. It now derives from router_provider, matching planning/coding. Adds regressions: sync-incoming-only (stale-cluster) raw test, missing-base atomic drop, router provider/model pairing (round-tripped through pinned Anton), and atomic-drop injection behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/api/v1/endpoints/settings.py | 10 +- cowork/common/settings/env_boundary.py | 123 +++++++++++++++--------- cowork/common/settings/user_settings.py | 5 +- tests/test_env_boundary.py | 46 +++++++-- tests/test_settings_raw.py | 65 +++++++------ 5 files changed, 164 insertions(+), 85 deletions(-) diff --git a/cowork/api/v1/endpoints/settings.py b/cowork/api/v1/endpoints/settings.py index ac76f406..e0678884 100644 --- a/cowork/api/v1/endpoints/settings.py +++ b/cowork/api/v1/endpoints/settings.py @@ -410,9 +410,13 @@ 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) + # 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) _ENV_PATH.parent.mkdir(parents=True, exist_ok=True) lines = [f"{k}={v}" for k, v in existing.items()] diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py index 55b87a05..052d4002 100644 --- a/cowork/common/settings/env_boundary.py +++ b/cowork/common/settings/env_boundary.py @@ -265,6 +265,60 @@ def _keyed_unique_providers(settings: UserSettings) -> list[Provider]: 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. @@ -289,53 +343,33 @@ def db_to_env(settings: UserSettings, present_keys: set[str]) -> dict[str, str]: """ out: dict[str, str] = {} - 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), - ) - unique_providers = _keyed_unique_providers(settings) - - if _env_representable(settings, unique_providers): - 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 - out[prov_var] = _anton_provider_name(prov) - if model: - out[model_var] = model - for prov in unique_providers: - _emit_provider_creds(out, settings, prov) - elif unique_providers: - # The provider cluster is NOT written; env_reconcile_vars keeps it out of - # the merge's drop-set too, so a previously-valid CLI config is preserved - # rather than wiped (ENG-1127 review). + 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 — roles resolve to providers " - "the standalone CLI cannot represent together (%s); preserving the CLI's existing config", - ", ".join(p.value for p in unique_providers), + "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 not None: - text = _env_str(value) - if text: - out[env_var] = text - - # One injection guard over everything emitted (keys, URLs, models, flags): a - # CR/LF-bearing value is a dotenv-injection vector, never a real setting. - safe: dict[str, str] = {} - for var, val in out.items(): - if _is_dotenv_safe(val): - safe[var] = val - else: - logger.warning("settings: refusing to export %s — value spans multiple lines", var) - return safe + 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, ...]: @@ -344,11 +378,12 @@ def env_reconcile_vars(settings: UserSettings) -> tuple[str, ...]: 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) is representable, so its cluster still - reconciles away — that is how a logout still wipes the CLI's credentials - (ENG-1127 review). + 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 _env_representable(settings, _keyed_unique_providers(settings)): + if _provider_cluster(settings) is not None: return MANAGED_ENV_VARS return _FLAG_ENV_VARS diff --git a/cowork/common/settings/user_settings.py b/cowork/common/settings/user_settings.py index 37ff9726..d2c00fcd 100644 --- a/cowork/common/settings/user_settings.py +++ b/cowork/common/settings/user_settings.py @@ -515,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/tests/test_env_boundary.py b/tests/test_env_boundary.py index fc9b971b..ce090f81 100644 --- a/tests/test_env_boundary.py +++ b/tests/test_env_boundary.py @@ -124,6 +124,37 @@ def test_router_only_minds_exports_explicit_openai_slot_and_round_trips(): 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()) == {} @@ -175,18 +206,19 @@ def test_merge_env_lines_rejects_crlf_injection(): assert all(ln.count("=") >= 1 for ln in merged.split("\n") if ln) -def test_db_to_env_drops_newline_bearing_value(monkeypatch): - # A minds-cloud user so minds_url is genuinely part of the export, then poison - # it — the injection guard (not a missing key) is what must drop it. +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 "ANTON_MINDS_API_KEY" in out # the clean sibling still exports - assert "ANTON_MINDS_URL" not in out # poisoned value refused by the guard + 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 as a 2nd line. + # 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: @@ -195,7 +227,7 @@ def test_export_never_writes_injected_line_end_to_end(local_export): 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=sk-clean" in text # the clean sibling still lands + 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() 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() From 89616e65c78960f2160b3e3fa89b2a4c99e05b54 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Tue, 4 Aug 2026 14:24:40 -0700 Subject: [PATCH 12/13] fix(settings): harden POST /raw's .env write via atomic_write_env (ENG-1127 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy POST /settings/raw wrote ~/.cowork/.env with a bare write_text + chmod, bypassing everything the DB->.env export hardened: temp-file+os.replace atomicity, the CR/LF dotenv-safety guard, the transient-Windows-lock retry, and the export lock. After Phase B the client no longer calls /raw, so it's dormant- legacy and loopback-only — but it remained a second, weaker .env writer that could race the managed export. Route it through atomic_write_env and skip any newline-bearing var so this path can't smuggle a second line either. Self-review of PR #253. Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/api/v1/endpoints/settings.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/cowork/api/v1/endpoints/settings.py b/cowork/api/v1/endpoints/settings.py index e0678884..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) @@ -418,13 +419,15 @@ def write_raw_settings(body: _RawSettingsBody, session: SessionDep, request: Req # validation fails, leave the legacy .env untouched so the DB stays authoritative. sync_env_vars_to_db(session, incoming) - _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 + # 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: From ec70c469288032daa44c05c23f24ea74a7ae24c0 Mon Sep 17 00:00:00 2001 From: pnewsam Date: Wed, 5 Aug 2026 12:20:33 -0700 Subject: [PATCH 13/13] refactor(settings): retire the DB->.env export; decouple the CLI (ENG-1295) Remove the DB->.env export shim (added for the standalone anton CLI) and isolate the embedded harness base, so the DB is the sole server-side settings store and the standalone CLI owns its own ~/.anton/.env. - env_boundary.py: keep the inbound .env->DB conversion (used by the one-time boot migration); delete the outbound export (db_to_env, provider-cluster / representability logic, merge_env_lines, atomic_write_env + Windows lock-retry helpers). 494 -> ~54 lines. - services/settings.py: drop _export_env_for_cli, the export_env param, and the export lock; _after_write keeps the settings-cache invalidation. - api/v1/endpoints/settings.py: remove the GET/POST /settings/raw handlers (no client caller remains after cowork #524). - harnesses/anton_harness/harness.py: build AntonSettings(_env_file=None) so a user's standalone ~/.anton/.env can't bleed into embedded Cowork sessions. - migrations.py: drop the export_env kwarg; refresh stale docstrings. - tests: delete test_env_boundary.py (export) and test_settings_raw.py (/raw); trim the /raw case from test_settings_local_guard.py. Full suite: 753 passed, 0 regressions (2 pre-existing unrelated failures). Co-Authored-By: Claude Opus 4.8 (1M context) --- cowork/api/v1/endpoints/settings.py | 93 ----- cowork/common/settings/env_boundary.py | 440 +------------------- cowork/harnesses/anton_harness/harness.py | 8 +- cowork/migrations.py | 25 +- cowork/services/settings.py | 54 +-- tests/conftest.py | 4 +- tests/test_env_boundary.py | 486 ---------------------- tests/test_settings_local_guard.py | 20 +- tests/test_settings_raw.py | 116 ------ 9 files changed, 41 insertions(+), 1205 deletions(-) delete mode 100644 tests/test_env_boundary.py delete mode 100644 tests/test_settings_raw.py diff --git a/cowork/api/v1/endpoints/settings.py b/cowork/api/v1/endpoints/settings.py index 48cab027..965bfc75 100644 --- a/cowork/api/v1/endpoints/settings.py +++ b/cowork/api/v1/endpoints/settings.py @@ -19,7 +19,6 @@ from sqlmodel import Session from cowork.api.v1.endpoints.guards import require_local -from cowork.common.paths import cowork_home from cowork.db.session import get_session from cowork.schemas.base import CamelRequest from cowork.schemas.settings import ( @@ -346,95 +345,3 @@ async def recommended_models(session: SessionDep, refresh: bool = False): "modelEnabled": model_enabled, "modelLabels": model_labels, } - - -# ── Raw .env access (legacy, used by Onboarding) ───────────────────── - -_ENV_PATH = cowork_home() / ".env" - - -def _parse_dotenv_content(content: str) -> dict[str, str]: - result: dict[str, str] = {} - for line in content.splitlines(): - line = line.strip() - if not line or line.startswith("#") or "=" not in line: - continue - key, _, val = line.partition("=") - result[key.strip()] = val.strip().strip('"').strip("'") - return result - - -def _read_env_dict() -> dict[str, str]: - if not _ENV_PATH.exists(): - return {} - try: - return _parse_dotenv_content(_ENV_PATH.read_text(encoding="utf-8")) - except Exception: - pass - return {} - - -@router.get("/raw") -def read_raw_settings(request: Request): - # /raw dumps the dotenv verbatim (all provider secrets) — same loopback - # restriction as reveal-key. - require_local(request) - return _read_env_dict() - - -class _RawSettingsBody(BaseModel): - content: str - - -@router.post("/raw") -def write_raw_settings(body: _RawSettingsBody, session: SessionDep, request: Request): - """Merge dotenv content into ~/.cowork/.env and sync recognised keys to the DB. - - Uses key-level merge (not full overwrite) because callers like the - OAuth token refresh only send a subset of keys — a full overwrite - would wipe model config and other settings from .env. - - The .env persists for the standalone ``anton`` CLI and is read by - ``GET /raw``; the DB is authoritative for cowork-server. By syncing - to both we keep them consistent regardless of which frontend code - path writes settings (onboarding, OAuth token refresh, etc.).""" - # Writing the dotenv lands provider secrets on disk — same loopback - # restriction as the /raw read. - 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) - - try: - existing = _read_env_dict() - existing.update(incoming) - - # 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: - raise HTTPException(status_code=500, detail="Settings could not be saved.") from e - - return {"ok": True} - - -# NOTE: .env → DB migration now runs at server startup via -# cowork.migrations.migrate_env_to_db(), called from dev_setup. diff --git a/cowork/common/settings/env_boundary.py b/cowork/common/settings/env_boundary.py index 052d4002..058198b4 100644 --- a/cowork/common/settings/env_boundary.py +++ b/cowork/common/settings/env_boundary.py @@ -1,38 +1,22 @@ -"""The ``.env`` <-> DB settings boundary, both directions in one place. +"""The ``.env`` -> DB settings boundary (inbound only). -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. +The DB (``UserSettings``) is the source of truth. This module owns the inbound +conversion from a legacy ``.env`` into DB updates: the ANTON_* alias map and +provider-value normalization live here 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. +The outbound DB -> ``.env`` export was removed in ENG-1295. The standalone +``anton`` CLI now owns its own ``~/.anton/.env`` (it no longer reads the Cowork +mirror), so the server no longer mirrors settings to ``~/.cowork/.env``. Only +the inbound direction remains, used by the one-time boot migration that seeds +the DB from a legacy ``.env``. 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__) +from cowork.common.settings.user_settings import Provider # DB setting key -> its ANTON_* .env variable, for every field that overlaps # between AntonSettings (.env) and UserSettings (DB). Single canonical map (was @@ -59,25 +43,6 @@ # 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. @@ -92,8 +57,6 @@ def normalize_provider_value(val: str, *, minds_key_present: bool) -> str: 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. @@ -111,384 +74,3 @@ def env_to_db_updates(dotenv: dict[str, str]) -> dict[str, str]: ) 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/harnesses/anton_harness/harness.py b/cowork/harnesses/anton_harness/harness.py index 454e5a94..c053866d 100644 --- a/cowork/harnesses/anton_harness/harness.py +++ b/cowork/harnesses/anton_harness/harness.py @@ -404,7 +404,13 @@ async def _build_chat_session( from cowork.common.settings.user_settings import get_user_settings from pydantic import SecretStr - anton_settings = AntonSettings() + # Isolate the embedded base from the user's standalone CLI config + # (ENG-1295): the standalone `anton` now owns ~/.anton/.env, so loading + # the .env chain here would bleed a user's personal CLI settings (e.g. + # ANTON_MAX_TOKENS, ANTON_MEMORY_MODE) into Cowork sessions. Build with + # no .env base so embedded settings come only from anton defaults + the + # DB overlay applied below. + anton_settings = AntonSettings(_env_file=None) anton_settings.resolve_workspace(str(base)) # Per-project skills diff --git a/cowork/migrations.py b/cowork/migrations.py index 3c7d00ec..537e0cd6 100644 --- a/cowork/migrations.py +++ b/cowork/migrations.py @@ -10,14 +10,12 @@ had the old sentinel fire against the wrong path get a fresh migration run from the correct ``~/.cowork/.env`` location. -After migration the DB is **authoritative** for all overlapping fields. -The ``.env`` file continues to exist for: - - The standalone ``anton`` CLI (reads ``AntonSettings`` from ``.env``) - - Onboarding (writes ``.env`` first, then syncs to DB) - - Fields that only exist in ``AntonSettings`` (workspace paths, etc.) - -Cowork-server runtime code should read from ``get_user_settings()`` (DB), -never from the ``.env`` directly. +After migration the DB is **authoritative**. This is a one-way seed: the +server no longer writes ``~/.cowork/.env`` back (the DB->.env export was +removed in ENG-1295, and the standalone ``anton`` CLI now owns its own +``~/.anton/.env``). Reading a legacy ``~/.cowork/.env`` here only bootstraps +the DB on first upgrade; cowork-server runtime code reads from +``get_user_settings()`` (DB), never from ``.env`` directly. """ from __future__ import annotations @@ -114,10 +112,9 @@ def migrate_env_to_db(session: Session) -> bool: migrated_keys: list[str] = [] for setting_key, val in env_to_db_updates(dotenv).items(): try: - # 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) + # Seed the DB from a legacy .env (ENG-1295). One-time, sentinel- + # guarded; the DB is authoritative thereafter. + svc.upsert_setting(setting_key, val) migrated_keys.append(setting_key) except Exception as e: logger.debug("Skipping env migration for %s: %s", setting_key, e) @@ -172,9 +169,7 @@ def backfill_minds_url(session: Session) -> bool: changed.append(key) if changed: session.commit() - # 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). + # Invalidate the settings cache so the next read sees the rewritten host. svc._after_write() logger.info( "Backfilled legacy MindsHub host (mdb.ai -> %s) in: %s", diff --git a/cowork/services/settings.py b/cowork/services/settings.py index 2d04542f..f4b10e81 100644 --- a/cowork/services/settings.py +++ b/cowork/services/settings.py @@ -1,6 +1,5 @@ import json import logging -import threading from enum import Enum from cryptography.fernet import InvalidToken @@ -8,14 +7,6 @@ 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, @@ -25,12 +16,6 @@ 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 '***'. @@ -169,45 +154,16 @@ def _write_row(self, key: str, store_val: str) -> None: row.value = store_val self.session.add(row) - 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. - """ + def _after_write(self) -> None: + """Post-commit hook shared by the settings mutators: invalidate the + user-settings cache so the next read sees the committed state.""" 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: + def upsert_setting(self, key: str, value: str) -> SettingResponse: store_val, validated = self._encode_for_store(key, value) self._write_row(key, store_val) self.session.commit() - self._after_write(export_env=export_env) + self._after_write() return self._to_response(key, validated, True) def save_all(self, updates: dict[str, str]) -> list[str]: diff --git a/tests/conftest.py b/tests/conftest.py index d216ddbb..3f6781d6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,8 +20,8 @@ # 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. +# Home dir too: the one-time .env->DB migration reads $COWORK_HOME/.env, so +# without this a test could read the developer's real ~/.cowork/.env. os.environ["COWORK_HOME"] = str(TMP) os.environ["ENV"] = "test" diff --git a/tests/test_env_boundary.py b/tests/test_env_boundary.py deleted file mode 100644 index ce090f81..00000000 --- a/tests/test_env_boundary.py +++ /dev/null @@ -1,486 +0,0 @@ -"""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_local_guard.py b/tests/test_settings_local_guard.py index 3040cca7..8927d128 100644 --- a/tests/test_settings_local_guard.py +++ b/tests/test_settings_local_guard.py @@ -1,10 +1,10 @@ -"""reveal-key and /raw must refuse non-loopback callers (ENG-457). +"""reveal-key must refuse non-loopback callers (ENG-457). -These endpoints return unmasked provider secrets (a single key, or the whole -dotenv). `guards.require_local` is defense-in-depth for a network-exposed -deployment — e.g. a self-host compose that binds 0.0.0.0 — so even with no -app-layer auth they only answer a loopback client. The desktop sidecar + UI -talk over 127.0.0.1, so the legitimate flow is unaffected. +This endpoint returns an unmasked provider secret. `guards.require_local` is +defense-in-depth for a network-exposed deployment — e.g. a self-host compose +that binds 0.0.0.0 — so even with no app-layer auth it only answers a loopback +client. The desktop sidecar + UI talk over 127.0.0.1, so the legitimate flow is +unaffected. """ from types import SimpleNamespace @@ -43,11 +43,3 @@ def test_reveal_key_blocks_non_local_before_db(): with pytest.raises(HTTPException) as exc: reveal_key("openai", session=None, request=_request("203.0.113.7")) assert exc.value.status_code == 403 - - -def test_read_raw_blocks_non_local(): - from cowork.api.v1.endpoints.settings import read_raw_settings - - with pytest.raises(HTTPException) as exc: - read_raw_settings(request=_request("203.0.113.7")) - assert exc.value.status_code == 403 diff --git a/tests/test_settings_raw.py b/tests/test_settings_raw.py deleted file mode 100644 index 2249ddb4..00000000 --- a/tests/test_settings_raw.py +++ /dev/null @@ -1,116 +0,0 @@ -from types import SimpleNamespace - -import pytest -from fastapi import HTTPException - - -def _local_request(): - """Loopback stand-in for the Request arg the raw-settings endpoints now - take — they 403 non-loopback callers (guards.require_local, ENG-457).""" - return SimpleNamespace(client=SimpleNamespace(host="127.0.0.1")) - - -def _delete_settings(session, *keys: str) -> None: - from cowork.services.settings import SettingService - - service = SettingService(session) - for key in keys: - try: - service.delete_setting(key) - except ValueError: - pass - - -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 - from cowork.services.settings import SettingService - - env_path = tmp_path / ".anton" / ".env" - env_path.parent.mkdir(parents=True) - # 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, *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} - - service = SettingService(session) - loaded = service.load() - # Incoming credential + provider synced to the DB. - assert loaded.minds_api_key.get_secret_value() == "new-key" - assert loaded.planning_provider.value == "minds_cloud" - # 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, *keys) - session.close() - - -def test_raw_settings_write_rejects_invalid_db_values_before_env_write(tmp_path, monkeypatch): - 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 - from cowork.services.settings import SettingService - - env_path = tmp_path / ".anton" / ".env" - env_path.parent.mkdir(parents=True) - env_path.write_text("ANTON_PLANNING_PROVIDER=anthropic\n", encoding="utf-8") - monkeypatch.setattr(settings_endpoint, "_ENV_PATH", env_path) - - session = get_open_session() - try: - _delete_settings(session, "planning_provider", "planning_model") - - with pytest.raises(HTTPException) as exc: - write_raw_settings( - _RawSettingsBody( - content="\n".join( - [ - "ANTON_PLANNING_PROVIDER=not-a-provider", - "ANTON_PLANNING_MODEL=_reason_", - ] - ) - ), - session, - _local_request(), - ) - - assert exc.value.status_code == 400 - assert env_path.read_text(encoding="utf-8") == "ANTON_PLANNING_PROVIDER=anthropic\n" - - service = SettingService(session) - assert service._fetch_row("planning_provider") is None - assert service._fetch_row("planning_model") is None - finally: - _delete_settings(session, "planning_provider", "planning_model") - session.close()