mirror of
https://github.com/langchain-ai/deepagents.git
synced 2026-08-28 05:00:04 -04:00
Mason Daugherty
a564f8eead
`ConfigResolver` (with its cached per-process snapshot) shipped in #5736, but every production reader still went through the legacy `resolve_scalar` / `resolve_ranked_scalar` wrappers, which re-parse `config.toml` on every call. This PR retires those wrappers and settles the question the wrappers had been hiding: **when is configuration read?** ## Behavior change Configuration files are now read once into a single process-wide generation. Every reader resolves against that generation, so no two parts of the process can disagree about a setting. **Editing `config.toml` while the app is running therefore has no effect until the generation advances**, which happens in exactly two places: an in-app write (toggling a preference refreshes the generation itself) and `/reload`. A file that fails to parse leaves the previous generation in force rather than half-applying the new one. This is the convention every long-running Unix service uses — read at start, change on an explicit signal. Watching files for edits is deliberately not done: a partly applied configuration is a worse failure than a stale one, and per-option exceptions (some live, some cached) would make the effective configuration unpredictable per setting. The policy is now written down in `ARCHITECTURE.md` and pinned by tests, so it stops being an accident of which call path a reader happens to take. Users who edit the file by hand and expect immediate effect will need `/reload`. Worth a release note. ## Retiring the wrappers 1. **Plain single-key readers move to the shared resolver.** `app.py`, `config.py`, `cold_cache.py`, `cost_tracking.py`, `main.py`, `plugins/discovery.py`, and the two TUI widgets now call `get_config_resolver().get(option)`, replacing per-call file parsing with the resolver's cached snapshot. Diagnostics behavior is preserved by calling `_emit_ranked_diagnostics(option, resolved)` explicitly at each site. `Settings._reload_values` resolves through `get_config_resolver(refresh_managed=refresh_managed)`, so `/reload` and later readers observe the same generation instead of the cache going stale; the env tier still comes from the method's `env` argument, and the "a failed or blocked reload never drops policy in force" invariant is unchanged. 2. **Explicit-snapshot callers build ad-hoc resolvers.** Callers that pass `toml_data=`/`managed_toml_data=` deliberately inspect a specific file generation rather than process state, so pointing them at the shared cache would be wrong. The decision rule: if the caller snapshots one generation itself — the `config` CLI (one read per invocation), `update_check` (health reported next to the value), the sandbox/theme loaders (a non-default `config_path` excludes managed policy), and the managed-policy validators (a candidate generation not yet in force) — it builds `resolver_from_snapshots(TomlSnapshot(...), TomlSnapshot(...))` and calls `.get(option)`. 3. **The legacy path is deleted.** `resolve_scalar`, `resolve_ranked_scalar`, and the now-dead `_coerce_env` helper are gone from `config_manifest`. The manifest's remaining bespoke readers share a private `_resolve_option`, which resolves through the shared generation when the caller supplies no tables. The migration-parity equivalence test is deleted with them — its job (proving the two paths agree) is done. Tests that drove the wrappers directly now exercise the same coercion, precedence, and diagnostics assertions through `ConfigResolver`/`resolver_from_snapshots` or real TOML files under the test-redirected config path. 4. **The remaining fresh-parse readers, and why.** Two keep a concrete parse, and neither is on a live path: - `resolve_read_project_dotenv` runs during dotenv bootstrap, before the project `.env` is layered into `os.environ`; seeding the shared generation there would capture an env tier later readers do not see. - `resolve_startup_mode_with_source` inspects the raw user table on its fall-through path, which the resolver does not expose. Its only production caller (`dcode config`) passes an explicit generation. ## Fixes found while reviewing this branch - **`/reload` previews read a stale user tier.** The preview path shares `_reload_values` with `refresh_managed=False`, so its cached user snapshot could predate the `[shell].allow_list` edit being previewed. The preview then reported no change while the accepted reload applied it — diverging on a security-sensitive auto-approval setting, and reaching the cwd-switch consent prompt as `project_settings_change_detected=False`. The preview now reads the user file fresh while keeping the managed snapshot the process is enforcing (a preview must not refresh policy in force). This is the atomic-swap half of the policy, not a liveness exception: preview and apply must agree on one generation. - **A lost diagnostic on UI preference writes.** `_save_ui_bool_result` was the one migrated reader that did not pick up the wrapper's implicit diagnostics call, so a malformed managed `[ui]` entry was reported nowhere — removing the one signal an administrator has that their policy is inert. - **`/reload` reloaded every provider twice.** `get_config_resolver(refresh_managed=True)` already reloads on a cache hit, so the explicit `.reload()` was redundant: one `/reload` read the managed file four times and re-ran `managed_policy_violations` with each. - **Merge strategies for real manifest options were unasserted.** The deleted `test_populated_tiers_actually_reach_the_resolver` did not depend on the `resolve_scalar` oracle, and `merge_strategy` appeared nowhere else in the suite — so flipping `threads.columns` from `DEEP_MERGE` to `REPLACE`, or `mcp.disabled_servers` from `UNION` to `REPLACE`, passed the full suite. Restored and mutation-checked in both directions. - **`refresh_managed` was untested.** Removing the refresh from `/reload` kept the suite green, because the existing reload tests drive env rather than a file edit and monkeypatch `DEFAULT_CONFIG_PATH` (which changes the resolver cache key and rebuilds a fresh resolver, hiding staleness). Now pinned in both directions. - **Comment rot.** Roughly ten comments still described per-call file parsing, including one that promised live edits took effect without a restart — the exact behavior the migration removed. `get_option`, `get_config_options`, `_emit_ranked_diagnostics`, and the manifest types stay public and unchanged; managed config remains read-only; no new dependencies. ## Follow-ups - `_emit_ranked_diagnostics` and `_ranked_source` now have nine external importers, and the snapshot-construction block appears at six production sites. A small `resolve_for_display(option)` seam (get + emit + label) would restore the encapsulation the wrapper provided and make the missing-emit bug above structurally impossible. - A file corrupted *after* its generation is taken is not reported. Corruption present when the file is read still logs with exact line and column; only mid-session corruption is silent. A cheap mtime/size check on `/reload` would close it. - Five `cast(...)` calls exist only because `ConfigResolver.get` returns `ResolvedValue[object]`; three sit behind validating predicates that could be `TypeGuard`s. `AGENTS.md` treats `cast` as a last resort.