feat(settings): export DB settings to the CLI's .env - #253
Conversation
No PR environment for this pull requestAdd the Updated on every push to this PR. |
pnewsam
left a comment
There was a problem hiding this comment.
Code Review
Verdict: COMMENT — merge-blocking findings
The new boundary is well-factored and the focused settings tests pass, but two consistency gaps still undermine the DB-as-source-of-truth guarantee: the startup Minds URL backfill bypasses export, and concurrent exporters can atomically install a stale snapshot.
Validation
- Targeted settings suite: 30 passed
- Full suite: 698 passed, 1 skipped; 3 unrelated/environment failures
The row-presence handling, tenancy guard, secret permissions, and focused test coverage are otherwise solid.
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) <noreply@anthropic.com>
Comment/docstring-only cleanup (~half the added comment lines); no code changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ary.py (ENG-1127) 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) <noreply@anthropic.com>
…xport (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) <noreply@anthropic.com>
90ad595 to
435b0b7
Compare
…s (ENG-1127) 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) <noreply@anthropic.com>
pnewsam
left a comment
There was a problem hiding this comment.
Code Review
Verdict: COMMENT — merge-blocking findings
The earlier backfill and in-process lost-update findings are fixed, but the current export can still produce CLI configurations that Anton cannot run, and it writes DB strings into a startup-sensitive dotenv file without escaping. Overall merge risk is High until the provider/model contract, pinned Anton compatibility, and dotenv serialization findings are addressed. The plaintext-key duplication, retained Electron//settings/raw writer race, best-effort stale export behavior, and isolated-build COWORK_HOME mismatch remain explicit rollout risks.
Validation
- Re-read all 7 changed-file patches at head
653c1791763eeb83f3c9473d610d92a30990459f. - Inspected the settings write/read paths and existing review resolutions.
- Compared the exported schema with the exact
anton-agentcommit pinned by this head'suv.lock(c51afe87). - Confirmed the visible GitHub build/deploy workflow succeeded; no current test status contexts were exposed.
- Source review only; I did not rerun the suite locally.
…ENG-1127 review) 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) <noreply@anthropic.com>
…, gemini translated (ENG-1127 review) 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) <noreply@anthropic.com>
pnewsam
left a comment
There was a problem hiding this comment.
Code Review
Verdict: COMMENT — one merge-blocking finding remains
The provider/model pairing and CR/LF dotenv-injection findings are addressed at this head. The single-provider Gemini/OpenAI-compatible translation also works, but mixed planning/coding/router configurations can still produce a global Anton key/base combination that does not represent any role correctly. Bottom line: keep the Anton-compatibility thread open until mixed providers are handled or explicitly rejected.
Validation
- Re-read the updated settings boundary and regression tests at head
de28960b2db4d729938b09347bcc7c3c5426c656. - Compared the emitted variables with the exact pinned
anton-agentsettings/provider construction atc51afe87. - Confirmed the current visible build/deploy workflow succeeded.
- Source review only; I did not rerun the server test suite locally.
…esent (ENG-1127 review) 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
pnewsam
left a comment
There was a problem hiding this comment.
Code Review
Verdict: COMMENT
The earlier provider/model pairing, Gemini translation, shared OpenAI-slot collision, and CR/LF-injection findings are addressed. Two Major compatibility/state-management edge cases remain, so I do not think this is merge-ready yet.
Validation
- Re-reviewed all 7 changed files at head
76eac509b07428c485435dc50ec4d43dee3a29b0. - Compared the export behavior with the pinned Anton dependency at
c51afe87add12b49bf030190446aab61b55b3a88. - Confirmed all earlier review threads are resolved.
- GitHub Actions run 527 and the Snyk commit status are successful.
…pe a valid .env on unrepresentable saves (ENG-1127 review) 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) <noreply@anthropic.com>
pnewsam
left a comment
There was a problem hiding this comment.
Code Review
Verdict: COMMENT
At head dbe81f7, the new export still violates the DB-source-of-truth contract and can produce invalid or misrouted CLI configurations. Three Major findings remain; these should be addressed before merge.
Validation
- Re-reviewed all 7 changed files and the resolved discussion at the current head.
- Relevant settings suites: 85 passed.
- Reproduced the stale raw-sync, missing-base, and router provider/model cases against the pinned Anton dependency.
git diff --checkpassed.- GitHub Actions run 528 and the Snyk commit status are successful.
…ess clusters; pair router with its own model (ENG-1127 review) 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) <noreply@anthropic.com>
pnewsam
left a comment
There was a problem hiding this comment.
Code review — Phase A (DB→.env export).
The one-way export is sound and unusually well-guarded (atomic write, 0o600, CR/LF injection reject, representability + atomic cluster validation, Windows-lock retry), and the regression coverage is strong. Ran pytest tests/test_env_boundary.py test_settings_raw.py test_settings_schema.py → 32 passed. No blocking issues; a few minor/nit items inline. Verdict: COMMENT.
…G-1127 review) 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) <noreply@anthropic.com>
|
Preserving Claude's notes: Exceptions (pre-existing, collapsed in Phase B)
Known limitationThe server writes |
ZoranPandovski
left a comment
There was a problem hiding this comment.
Not blocking this PR, but I think this approach adds more complexity to an already complicated settings flow.
To simplify it long term, we should:
- Update the desktop client so it no longer reads settings from
.envand writes them back into the DB. - Update the standalone Anton CLI so that, when it starts, it gets the current provider, model, base URL, and credentials from the local Cowork server instead of relying on a mirrored
.envfile. - Once older desktop clients are no longer relying on the current behavior, remove the automatic DB-to-
.envexport.
I’m fine with this as a temporary compatibility layer, but I think we should make the removal plan explicit so it does not become permanent.
|
@ZoranPandovski That makes sense, this PR adds a lot of code just to accommodate a flow we're already planning to deprecate. So in that sense it may not be a step in the right direction. I think we can go ahead and remove the .env dependency from cowork, if we just include updates on the anton side as well. I'll see if I can rework these PRs to align with that approach. |
lucas-koontz
left a comment
There was a problem hiding this comment.
Reviewed at 89616e6, as a direction review rather than line by line, since @ZoranPandovski's pushback and your reply land this in "replaced by #280" territory and env_boundary.py is slated for deletion. Line-reviewing 494 doomed lines would just spend your attention twice. Four things worth recording before this closes.
1. #280 fully supersedes this, and the salvage list is empty. I diffed it rather than assuming: env_boundary.py goes 494 to 76 lines (only SETTING_ENV_ALIASES, normalize_provider_value, env_to_db_updates survive), _export_env_for_cli plus the export lock and the export_env kwarg are gone, and GET/POST /settings/raw is deleted outright; anton#313 drops ~/.cowork/.env from _build_env_files() so the CLI reads only ~/.anton/.env.
Everything independently valuable is already carried over: the apply_model_defaults fix deriving router_model from router_provider is byte-identical in #280 (and safe, since router_provider: Provider has a non-None default at user_settings.py:265), along with the alias map, normalize_provider_value, env_to_db_updates, and the COWORK_HOME conftest guard. What gets dropped is moot under the new shape: atomic_write_env, the Windows-lock retry and _is_dotenv_safe have no writer left in cowork-server, and the /raw hardening plus the sync_env_vars_to_db(incoming) narrowing die with the endpoint. Nothing needs porting.
One thing does land untested though: router_model-from-router_provider had its only coverage in tests/test_env_boundary.py, which #280 deletes with the module. Worth a dedicated test in #280.
Recommend closing this in favour of #280 rather than keeping it as a fallback. The two designs write the same file from opposite ends, so "fall back" would mean un-merging #280, not flipping a switch.
2. The one real gap the decoupling opens, worth raising on #280 while it is still a draft. After #524 + #280 + anton#313, nothing writes or clears ~/.cowork/.env, but it still holds plaintext provider keys. Two consequences: any anton install not yet upgraded past #313 keeps reading it and silently runs on frozen credentials (the MindsHub token refresh currently rewrites that file through POST /raw, documented as a caller at settings.py:394, so once the endpoint is gone the CLI keeps the pre-rotation key and 401s with no signal); and logout no longer clears it, so a signed-out machine leaves live keys on disk indefinitely.
flowchart LR
ref["MindsHub token refresh"] -->|"today: POST /raw"| env["~/.cowork/.env"]
ref -.->|"after #280: DB only"| db[("cowork.db")]
env --> old["anton < #313"]
old --> stale["runs the pre-rotation key, 401s"]
db --> new["anton >= #313, ~/.anton/.env"]
A one-shot cleanup in #280 that strips the ANTON_* credential lines (or renames the file) after the DB is seeded closes both. It needs its own sentinel: _env_migrated_v2 (migrations.py:50, 107-109) has already fired on every existing install, so migrate_env_to_db returns early there and cannot carry it.
3. question, for @ZoranPandovski: your step 2 was "the standalone Anton CLI gets the current provider, model, base URL and credentials from the local Cowork server". anton#313 does something materially different: the CLI gets its own ~/.anton/.env and the desktop-to-CLI inheritance is dropped, so a desktop-configured user re-onboards the CLI once. The reasoning (the standalone CLI has to run with no server) is sound and probably better, but it currently lives only in anton#313's description in another repo. Losing "configure once" is a product call rather than a refactor detail, so worth an explicit sign-off here rather than finding it in a draft elsewhere.
4. Two loose ends before this closes. Nothing on this PR points at the replacement: ENG-1295, #280, anton#313 and the reworked cowork#524 are only discoverable from their own descriptions, so the "make the removal plan explicit" ask reads as unanswered on the thread where it was raised. A closing comment linking all four settles it. Separately, #280 keeps sync_env_vars_to_db (migrations.py:79) but deletes its only caller along with POST /raw, leaving it referenced only from a comment in a test; cheap to drop while that PR is still a draft.
For the record, since it is easy to lose when a PR closes: the round-trip test pattern in tests/test_env_boundary.py, feeding the rendered env through the pinned AntonSettings + LLMClient.from_settings, is the best idea in that file. It dies with #280 because nothing renders env anymore, which is correct, but it is the kind of test worth reaching for again the next time we serialize config for another process. I also confirmed the CodeQL "incomplete URL substring sanitization" alert is genuinely fixed rather than merely outdated (head asserts equality against GEMINI_BASE_URL at tests/test_env_boundary.py:45,61 instead of the substring check), so nothing carries forward there.
Description
This continues efforts to refactor some of the code around updating settings. Because we have multiple sources of truth (.env file and sqlite DB), issues are inevitable unless we write/read to/from these sources in an consistent manner.
This simplifies by enforcing a new one-way data flow. Settings are always written to the DB then replicated to the .env file. In future we may deprecate the .env file but this reduces inconsistency in the meantime. The meatier part of this work is on the client-side PR where we can remove significant code to align around a more consistent write path.
flowchart LR write["settings change<br/>(form save / login / logout)"] --> svc["SettingService"] svc -->|"1 · commit"| db[("cowork.db<br/>source of truth")] svc -->|"2 · derive + write"| env["$COWORK_HOME/.env<br/>(export for the CLI)"] env --> cli["anton CLI reads"]