refactor(code): remove legacy resolve_scalar config path - #5755
Merged
Mason Daugherty (mdrxy) merged 57 commits intoAug 24, 2026
Conversation
PR #5736 introduced `ConfigResolver` with ranked providers but left every production reader on the legacy `resolve_scalar(option, toml_data=...)` wrapper, which re-parses `config.toml` on every call. Migrate the plain single-key readers to `get_config_resolver().get(option)` so they share the resolver's cached snapshot instead: - `app.py`: `_load_float_option` (drops its now-pointless `toml_data` parameter), `_load_cursor_style_preference`, and the compact-on-resume threshold read. - `config.py`: `is_langsmith_redaction_enabled`, `is_memory_auto_save_enabled`, `is_yolo_switcher_enabled`, `is_openai_prompt_cache_key_enabled`, `resolve_goal_auto_accept_criteria`, and the managed-tier read in `resolve_auto_classifier_model_with_problem`. - `cold_cache.py`, `cost_tracking.py`, `main.py`, `plugins/discovery.py`, `tui/widgets/_links.py`, `tui/widgets/_paste_textarea.py`. `resolve_scalar` emitted provider diagnostics internally; the resolver returns them on the `ResolvedValue` instead, so each migrated site now calls `_emit_ranked_diagnostics(option, resolved)` explicitly to preserve the existing logging behavior. `/reload` also re-resolved through the legacy path, which left the shared resolver cache stale after a reload. `Settings._reload_values` now calls `get_config_resolver(refresh_managed=True).reload()` before resolving, so a real reload and later `get_config_resolver()` readers observe the same generation. A preview (`refresh_managed=False`) still refreshes nothing, and the env tier is still taken from the `env` argument rather than the resolver (whose env provider reads `os.environ` directly), so the preview semantics and the "failed or blocked reload never drops policy in force" invariant are unchanged. Tests that patched the now-dead `load_config_toml` seam for these readers write the redirected `DEFAULT_CONFIG_PATH` file (or a managed file via `redirect_managed_config`) instead; the monkeypatched `config_manifest.resolve_scalar` test in `test_app.py` now stubs `ConfigResolver.get` at the same semantic level.
…pers Call sites that pass explicit `toml_data=`/`managed_toml_data=` deliberately inspect a specific file generation rather than process state, so they now build an ad-hoc resolver with `resolver_from_snapshots(TomlSnapshot(...), TomlSnapshot(...))` and call `.get(option)` instead of the legacy wrappers: - `client/commands/config.py`: `_resolve` and `_option_provenance` report the one managed/user generation the command snapshots per invocation. - `update_check.py`: `_resolve_update_setting` reports source health next to the value, so both must come from the same snapshot read. - `integrations/sandbox_config.py` and `theme.py`: a non-default `config_path` deliberately excludes managed policy, which the shared process cache cannot express. - `configuration/service.py`: `resolve_managed_option` inspects a supplied managed generation — often a candidate being validated before it takes force — against an empty user tier. - `app.py`: the post-save managed-policy probe resolves the freshly loaded managed table against an empty user tier. - `config.py`: the blank-env veto path in `resolve_auto_classifier_model_with_problem` names the user-level value it overrides, so it resolves the already-read table with the managed tier excluded. No site turned out to be reading explicit data without reason, so none moved to the shared cache.
With every production reader on `ConfigResolver`, remove the compatibility wrappers from `config_manifest`: - `resolve_scalar` and `resolve_ranked_scalar` are deleted. The manifest's remaining bespoke readers (`resolve_read_project_dotenv`, `load_bool_display_preference`, the bounded auto-classifier/recursion resolvers, and the `startup.mode`/`auto_classifier` source renderers) now share a private `_resolve_option` helper that builds an ad-hoc `resolver_from_snapshots` resolver from the caller's table generation. - `_coerce_env` is deleted; its delegate-rejection guard lives in `providers.coerce_environment_value`, and the one test that exercised the wrapper now asserts that `Invalid` result directly. `_coerce_toml` stays: the public `option_accepts_toml` still validates through it. - The migration-parity test (`test_resolver_get_matches_resolve_scalar_for_every_manifest_option`) and its fixture guard are deleted — with the wrapper gone, they would test nothing. - Tests that drove the wrappers directly now build resolvers via `resolver_from_snapshots` through a test-local helper with the same `(value, source)` surface, keeping every coercion, precedence, and diagnostics assertion unchanged. - `test_run_get_section_skips_store_when_no_credentials` no longer depends on process-global bootstrap state: it pins `_bootstrap_state.done` so the store-read counts do not depend on which tests ran earlier in the worker.
github-actions
Bot
added
dcode
![open-swe[bot]](https://avatars.githubusercontent.com/in/1395372?s=60&v=4){kind=small}
Mason Daugherty (mdrxy)
changed the title
resolve_scalar config path
`preview_reload_from_environment` shares `_reload_values` with `refresh_managed=False`, so the shared resolver is not reloaded — and its cached user snapshot may predate the `[shell].allow_list` edit being previewed. The preview then reports no change while the accepted reload applies the edit, diverging on a security-sensitive auto-approval setting. Resolve the preview's shell allow-list against a fresh user-file read built from `resolver_from_snapshots` while keeping the current managed snapshot (a preview still must not refresh policy the process is enforcing). The skills read needs no fresh user tier: it only accepts managed-decided values.
`_save_ui_bool_result` was the one reader migrated off `resolve_scalar` that did not pick up the wrapper's implicit diagnostics call. The wrapper emitted provider rejections on every resolution; `resolver_from_snapshots(...).get()` does not. The effect: with a malformed managed entry (say `[ui] show_message_timestamps = "false"`), toggling the preference reports "Preference saved" and the rejection is logged nowhere -- removing the one signal an administrator has that their policy is inert. This is also the moment the user is told policy still wins, so the rejection is what makes that message actionable. Pair the resolution with `_emit_ranked_diagnostics`, matching every other converted site.
`get_config_resolver(refresh_managed=True)` already calls `resolver.reload()` internally on a cache hit and builds a fresh resolver on a miss, so the explicit `.reload()` in `_reload_values` was a second full reload. One `/reload` read the managed file four times and `config.toml` twice, and each `get_managed_snapshot(refresh=True)` re-runs `managed_policy_violations`, which resolves every enforced key through a freshly built resolver. On a managed deployment that work repeated on every `/reload` and every accepted cwd switch. Pass `refresh_managed` straight through instead. Behavior is unchanged on both paths: True reloads once, False keeps serving the cached snapshot.
`test_populated_tiers_actually_reach_the_resolver` was deleted alongside the `resolve_scalar` equivalence test it guarded, but it did not depend on that oracle -- and it was the only test asserting `MergeStrategy` wiring for real manifest options. The remaining merge tests use synthetic providers, and `merge_strategy` appears nowhere else in the suite. Restore it under a name that states what it protects, with a docstring that no longer refers to the deleted oracle. Mutation-checked both arms: flipping `threads.columns` from DEEP_MERGE to REPLACE, or `mcp.disabled_servers` from UNION to REPLACE, fails the test. Before this commit both mutants passed the full suite.
Readers moved from a per-call `load_config_toml()` parse to the shared cached resolver, but several comments still describe the old behavior. One of them promised a guarantee the migration removed, which is how the next reader reintroduces the bug it warns about. - `main.py`: `_resume_term_program` no longer reads `config.toml` from disk. - `cost_tracking.py`: re-resolving no longer re-reads the file; the surviving reason is that the updater thread starts once and is never stopped. - `config_manifest.py`: `option_accepts_toml` is the public coercion seam, not the private `_coerce_toml`; `THEME_DELEGATE` is resolved by `_resolve_theme` outside the ranked resolver, not by a provider branch; `_ranked_source` is now the only source-label producer, so "compatibility" is vestigial; and `_emit_ranked_diagnostics` no longer cites the deleted wrapper it inherited its contract from. - `_resolve_option`: the property that separates it from `get_config_resolver()` is that it resolves a freshly parsed generation, not that callers supply the table -- two of its callers parse the file themselves. Also record that it emits no diagnostics, and retain the `TomlSnapshot` status/table invariant that was lost with the wrapper docstring. - tests: two references to the deleted `_coerce_env`, one of them a test name.
Reverts 77c0a75. Making one option re-read the file per turn treated the symptom and entrenched the cause: a per-option split between live and cached reads means two settings edited in the same sitting behave differently, with nothing to tell a user or a reader which is which. Long-running Unix services settled this a long time ago -- read config at start, change it on an explicit signal. `nginx`, `sshd`, and `postfix` all reload rather than watch, and an nginx reload builds a whole new configuration and swaps it in one step rather than letting values drift in independently. A partly applied config is a worse failure than a stale one. `warnings.trusted_cache_endpoints` goes back to the shared generation, and the two comments that justified themselves with a per-turn parse cost are corrected to describe what the code now does.
`_resolve_option` required a parsed table, so its callers each fell back to their own `load_config_toml()` -- re-parsing the file per access and leaving half the manifest on a different generation from the readers already migrated to the shared resolver. Make `toml_data` optional: with no caller-supplied tables, resolution goes through the shared process resolver. Three bounded resolvers also eagerly parsed managed config, which forced the explicit-snapshot path even when the caller supplied nothing; they now let the parameter flow down. Converted: the display-preference loader (seven `display.*` options, previously parsed on every render), the auto-classifier model and timeout resolvers, and `runtime.recursion_limit`. Their bounded retries -- which re-resolve with one env var popped -- pass the same `None` through, so a retry stays on the generation the first pass used. Two readers deliberately keep a concrete parse: - `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. Both are consumed once per process -- the first at startup, the second only by `dcode config`, which passes an explicit generation -- so neither is on a live path and cadence is moot for both. `test_show_diff_line_numbers_reads_app_config` stubbed `load_config_toml`, which the display loader no longer calls; it now writes the real file under the test-redirected config path, matching the other converted tests.
This was referenced Aug 24, 2026
`model_config` edits `config.toml` in place rather than through `configuration.writer.update_user_config`, so it never reached `refresh_shared_resolver`. Once the manifest readers moved onto the shared generation, a saved preference landed on disk and stayed invisible to the process that wrote it: the UI reported "saved" and the old value kept deciding behavior until the next restart or `/reload`. Eleven writers were affected. Four cleared `_default_config_cache`, which covers only the `[models]` table; seven -- the `warnings`, `mcp`, and `threads` writers -- invalidated nothing at all. `_invalidate_config_caches` now drops both caches together at every committed write, and `refresh_shared_resolver` loses its underscore so a sibling module can reuse the default-path rule and the best-effort failure handling rather than restating them. `models.auto_classifier` decides which model reviews gated actions, so a stale read there is the security-relevant case: `test_round_trips_through_the_launch_resolver` was already red on this branch. `TestWritesReachTheSharedResolver` pins the general property for saves and clears; all five tests die when the refresh is removed.
The configuration section claimed every reader resolves against one generation, so no two parts of the process can disagree. A grep disproves it: `get_config_sources` calls `TomlFileProvider(...).load()` on each invocation, and roughly a dozen readers -- `model_config`, `theme`, `sandbox_config`, `agent`, `mcp_disabled` among them -- reach the file through it. The env tier is live by design as well. Also corrected: the generation is built on the first read, not at startup (nothing forces it early), and an in-app write refreshes it only on the default path. The exceptions are now named, and the closing paragraph distinguishes what the policy actually forbids -- an option that is live for one reader and cached for another -- from a caller deliberately snapshotting one file generation, which is allowed and reported next to its health.
The comment justified the local parse with two claims, both wrong. `EnvProvider` is `durable = False` and reads `os.environ` inside `get()` (`configuration/providers.py:739`), so seeding the shared generation here could not "capture an env tier that later readers do not see" -- only the TOML tiers are cached. And the answer is not "consumed once, at startup": `config._load_dotenv` re-runs on every cwd switch, and `_preview_dotenv_environ` on every reload preview. The real reason is the tier shape. This resolution layers `global_dotenv` between the env and TOML tiers, and the resolver has no provider for it, so the resolver's own TOML tier is discarded on the fall-through path. Keeping the parse local additionally keeps dotenv bootstrap from establishing the process generation as a side effect. A carve-out is only as good as its stated reason; this one would not have survived the first reader who checked it.
`resolve_scalar` emitted ranked diagnostics itself, so no caller could forget them. Retiring it replaced that guarantee with a two-line convention repeated at 19 readers -- and the convention was already dropped once, in `_save_ui_bool_result`, where a malformed managed `[ui]` entry stopped being reported anywhere. Nothing caught it: deleting every external `_emit_ranked_diagnostics` call leaves the suite green, because the failure is invisible at runtime. The value falls back to its default and no diagnostic is written. This walks the package AST for functions that build a resolver and read an option, and requires a paired emit. Five readers report health through another channel (`dcode config` provenance, `dcode doctor`, the managed validators) and are listed with that reason, as is `_resolve_option`, which is the resolution primitive rather than a reader. The check matches a *call*, not the name: these readers import the helper inside the function body, so a substring check still passes after the call is deleted -- the first version of this test failed to kill that mutant.
`require_healthy_managed_config` gates the managed file only. A user file that fails to parse retained the previous snapshot -- the right runtime behavior -- but said so only through a `logger.warning`, which `install_log_buffer` routes to the in-memory debug buffer. The report the user reads printed "Configuration reloaded. No changes detected." So managed corruption was surfaced as a notice and user corruption was silent, which is backwards: the user just edited `config.toml` themselves and is watching for the edit to take. `_reload_values` already carries a notice slot for the managed block, and the two cases cannot collide because a managed block returns early. An unusable user provider now fills the same slot. The preview reports its own fresh read rather than the shared resolver's status, so what a user accepts matches what they were shown. The notice reports a rejection, not a rollback -- the retained value stays in force, which the tests assert alongside it.
`_resolve` mapped `managed_toml_data=None` to an empty managed tier, so an
omitted argument reported the user or env value as effective while managed
policy actually decided. The retired `resolve_ranked_scalar` loaded the file
in that case, so the fail-open shape arrived with the migration.
All three production callers pass `_load_managed_generation()[0]`, which is
never `None`, so nothing misreports today. Dropping the default makes the
type checker reject the next caller that omits it, rather than leaving
`or {}` to silently decide.
An invocation with no policy installed still passes an empty table -- that
says something different from "not supplied", and only one of the two is now
expressible.
Fifteen test call sites relied on the default and now state the empty tier
outright.
The bounded readers' managed fall-through recursed with
`managed_toml_data={}` while leaving `toml_data=None`, so `_resolve_option`
took its ad-hoc path and re-parsed `config.toml` from disk instead of using
the cached process generation. A hand edit after startup could then change
`models.auto_classifier_timeout` or `runtime.recursion_limit` for a
subsequently built agent without `/reload`, while other readers stayed on
the old generation.
Add `_resolve_option_without_managed`, which masks the managed tier while
pairing the caller's user table -- or, when none was supplied, the shared
resolver's cached user snapshot via the new `ConfigResolver.toml_snapshot`
accessor -- so the fall-through observes the same generation as every other
reader. The env tier still reads `os.environ` live.
`_invalidate_config_caches` was inserted directly below `clear_caches`, and the `invalidate_thread_config_cache()` call that was `clear_caches`'s last statement became the new function's last statement instead of being added to it. `_thread_config_cache` was then left with no invalidator other than the four in-app thread writers, and its read path deliberately has none, so a hand edit to `[threads]` was never picked up for the life of the process -- contradicting the `/reload` contract this branch writes into ARCHITECTURE.md. Both functions need the call: `clear_caches` for `/reload` and the auth and model-selector resets, `_invalidate_config_caches` for a committed write. The regression left the suite green because the four thread writers invalidate the cache themselves, so only hand edits were affected. Pinned with a test that fails without the restored call.
`TestManagedVerdict` exercises `_managed_verdict` in isolation, so nothing connected the helper to the messages it exists to produce. Deleting the whole probe from `_save_goal_auto_accept_preference`, or the `rejected` branch from `_save_ui_bool_result`, left the full suite green. Both gaps sit on the same claim: a write reports "saved" whether or not policy overrode it, so the probe is the only thing that tells the user otherwise -- and for a rejected entry it is the only signal the administrator's policy is inert. The `decided` branch of `_save_ui_bool_result` was already covered; this adds its `rejected` sibling and all three outcomes for the goal path. The goal-preference test drives the real coroutine with a recorder in place of the app, since the method uses nothing but `self.notify`.
The restored merge test names three options, so it pinned those three and nothing else -- flipping `display.terminal_themes`, `agents.async_subagents`, `sandboxes.providers`, or `mcp.disabled_project_servers` to `REPLACE` passed the whole suite. That is the same bug class the named options were restored for, on the six that were not named. Covers all nine non-`REPLACE` options against a frozen table of expected strategies, plus a test that the table still matches the manifest so a new composing option cannot ship uncovered. The table has to be written down rather than derived: reading the expected strategy from `option.merge_strategy` is a tautology, because a downgraded option drops out of the enumeration and stops being tested. A first draft did exactly that and caught none of the five mutations; this version catches all of them.
`TestWritesReachTheSharedResolver` covers five writers by name out of eleven, so removing `_invalidate_config_caches` from `save_thread_columns` or `clear_default_agent` left the suite green. The failure is invisible at runtime -- the file on disk is correct and the UI reports "saved" -- so the class needs a structural guard, the same shape as the diagnostics guard that already covers resolver readers. A writer is a function taking a `config_path` that commits it with an atomic replace, which matches the eleven writers exactly. `touch_recent_model` writes the MRU state file the same way but takes a `state_dir`, so it is correctly not counted.
`get_config_resolver` honors `managed_snapshot` on a cache miss and installs it on a refresh, but a cache hit without `refresh_managed` returned the cached resolver and dropped the argument with no signal. The parameter had two behaviors for one argument. That is correct for the preview path, which passes the generation it is already enforcing -- but only by coincidence: `get_healthy_managed_snapshot` with `refresh=False` happens to return the same object, and nothing in the signature preserves it. A caller passing a newer snapshot got silence. Rejects a mismatch instead of discarding it, so the invariant that makes the preview safe is checked rather than assumed. The `refresh_managed` docstring also claimed it re-reads all providers, which inverts what the branch does: `reload_with_replacements` reloads only the providers it is not replacing, so the managed tier is the one tier deliberately not re-read.
`data` was annotated `Mapping[str, Any]`, but 17 sites across the package tell a table from a scalar with `isinstance(value, dict)`. Any `Mapping` that is not a `dict` -- a `mappingproxy`, a `ChainMap`, a test double -- satisfied the annotation at construction and then made every option under a nested table fall through to its next source, with no error and no diagnostic. The docstring already described the `MappingProxyType` case as a deliberate exclusion, but the exclusion is general, and a comment cannot stop a caller from passing one. The narrower annotation makes it a type error at the call site instead. Propagates through the `toml_data` / `managed_toml_data` / `managed_data` parameters that feed a snapshot, and through `load_managed_config_toml` and `_load_managed_generation`, which return one. No behavior change: every production caller already passes a `tomllib` parse result.
`_managed_verdict` built its managed snapshot as `TomlSnapshot(load_managed_config_toml(), ProviderStatus(..., OK))`. `load_managed_config_toml` returns an empty table for a file it could not read, so asserting `OK` over it threw away the one thing that distinguishes "no policy installed" from "policy unreadable". The consequences were all silent: the provider never attached a rejection, the managed tier resolved as `Unset` rather than reporting a problem, and the user got a bare "Preference saved." The administrator who wrote the broken file is not at that keyboard, which is the same reasoning the function's own docstring uses to argue the probe should exist at all. Passes the snapshot whole so its health travels with its data, which is also the invariant `TomlSnapshot.__post_init__` exists to enforce.
The `default` read degraded to `None` with nothing logged anywhere, while its sibling `providers` read inspected `tier_health` and warned. A managed `sandboxes.default = 3` was rejected by coercion, fell through, and left the process with no default sandbox and no explanation -- on the option that decides which sandbox executes agent code. The reader is in `_SILENT_RESOLVER_READERS` under the rationale that `dcode doctor` reports file health, but file health is not value health: the file parses, and `doctor` confirms it parses. The rejected value was reported nowhere.
Dropping `{raw!r}` was the price of removing `_raw_toml_auto_classifier`, but
the message is returned to the caller for display on "a surface the user
actually reads" -- so the user was told their `[models].auto_classifier` is
malformed with no indication of what they wrote, on the setting that decides
whether the agent grades its own gated actions.
The rejection is already carried by the user tier's `Invalid.reason`
("Ignoring [models].auto_classifier=42 in config.toml (expected str)"), so the
value comes back without reopening a possibly newer file generation -- which
is the reason the raw read was removed in the first place.
`_warned_non_table_paths` was cleared in `reload_with_replacements` only. The resolver cache is keyed on `(user path, managed path)`, so installing or removing managed policy takes the cache-miss branch and builds a whole new resolver -- a new generation, with the previous generation's dedup set still live. The module docstring's promise that "each generation gets to report its own problems" did not hold across that transition, and `reset_config_resolver` had the same hole. Also moves the clear behind a public `config_manifest.reset_source_diagnostics` instead of reaching into the private set from `configuration.resolver`, whose own docstring says it is "intentionally unaware of the manifest". The test asserts on the set rather than on log records: rejection reasons carry the source paths, so they differ across a rebuild and counting warnings passes either way. It also moves the user path rather than installing policy, because the managed route runs through `invalidate_config_sources`, which clears the set itself and masks the path under test. Both wrong versions were tried first and caught nothing.
`refresh_shared_resolver` caught only `OSError`, but `reload()` can also raise
`ValueError` from the `TomlSnapshot` and `ResolvedValue` invariants or from
repeated provider ranks, and `RuntimeError` from a provider that produced no
snapshot or a resolver with no fallback.
`model_config._save_toml_field` and its siblings call this from the success
branch of their own write and return `bool`, so any of those escaped into UI
code after the bytes were already on disk -- exactly the outcome the
function's docstring says must not happen ("reporting a stale in-process view
as a failed write sends the user to retry a file that is already correct").
Broad by intent, with the reason stated: the write is committed and the
refresh is best-effort, so there is no failure here worth propagating.
`reload_with_replacements` installs its replacements without reloading them and without checking health, which bypasses the guarantee `TomlFileProvider` .reload` exists to give: a snapshot the source cannot use never displaces the last usable one. An unusable snapshot carries an empty table, which resolution reads as "this source declares nothing" -- so installing one at `MANAGED_RANK` drops policy and lets lower ranks win. Latent, not live: the only caller runs `get_healthy_managed_snapshot` first, which raises. But the guard was in the caller while the contract permitted the fail-open, and the method is public. Also adds the first direct coverage for this method -- both the documented unknown-rank `ValueError` and the new one were previously unexercised.
The comment sweep on this branch was driven by grep for the retired wrapper names, so what it missed is everything that described the old behavior without naming it. Five were factually wrong: - `cold_cache.load_trusted_cache_endpoints` promised its `config=None` path "loaded from disk"; it resolves through the shared generation. The module docstring 60 lines up was fixed and the function's own contract was not, which is the half a caller actually reads. - `_emit_ranked_diagnostics` said two rejections are deduplicated "once per process". There are three, and the scope is per generation -- the opposite of what the same file's `_warned_non_table_paths` docstring says. The local was named `once_per_process` too; both now say generation. - `_resolve_option_without_managed` credited "the shared resolver's `EnvProvider`" with reading `os.environ` live. It builds a fresh `EnvProvider` and takes only the user snapshot from the shared resolver, so a reader chasing a stale-env bug was pointed at the wrong object. - `require_healthy_managed_config` lost its `Raises:` when the body moved into `get_healthy_managed_snapshot`, though it still fails startup two ways. - `load_config_toml` is now the one public per-call parse and said nothing about sitting outside the generation. Also: `ConfigResolver.reload` re-arms the diagnostic dedup, which its one-line docstring did not mention while `writer.py` calls it on every write; `toml_snapshot` propagates a `RuntimeError`; `_resolve_option` had prose wedged between `Args:` and `Returns:` where napoleon folds it into the args; and the guard test's comment claimed "all five" over a seven-entry set whose channel list named one command that is not in it. Records the three blind spots the diagnostics guard cannot close, in the style the file already uses elsewhere.
Three factual problems in the new paragraphs: - "`/reload` swaps the result in atomically -- a config that fails to parse leaves the previous generation in force rather than half-applying the new one" overstated it. Retention is per provider, so a reload where the managed file parses and `config.toml` does not advances one tier and retains the other -- a half-applied generation, which the sentence promised cannot happen. `config.py` states it correctly and scoped; the doc generalized it. - "Two kinds of reader sit outside that generation" read as a closed list and omitted at least four, three of them in-app: the two deliberate fresh-parse manifest readers, `update_check`, and the reload preview's fresh user read. A maintainer trusting the list would assume any reader not on it resolves through the shared generation. - `dcode doctor` was grouped with callers that "inspect one file generation". It fabricates an empty user tier and inspects the managed file only. Downgrades the unguarded claim that no option "is" live for one reader and cached for another to what the code actually enforces, which is intent: the option key is assembled from a string at one call site, so nothing structural holds it. Also drops the appeal to "the convention every long-running Unix service uses" -- unfalsifiable, and the next sentence is the real reason and stands on its own -- and replaces the ` -- ` dashes, which appeared on only those three lines of a 79-line file. Shorter sentences throughout, per ASD-STE100.
`_managed_verdict` justified `refresh=False` with "the write just made cannot have changed managed policy, and re-reading it here would swap the snapshot every other reader observes." The second half was already false by the time the probe runs: the write goes through `refresh_shared_resolver` -> `reload()` -> `_reload_enforceable_managed_snapshot`, which re-reads the managed file with `refresh=True` and installs it. So every in-app preference toggle picks up administrator policy installed since startup. That follows from keeping one generation -- refreshing only the user tier would leave it ahead of the policy tier, the split state the design exists to prevent -- but it was documented nowhere, and the one docstring that touched it argued the opposite. Says so where a reader looks, and restates the probe's own reason in terms of what actually holds: it reads the generation in force, and re-reading would risk a second generation inside one user action.
`test_invalid_cursor_style_falls_back_to_the_default` asserted only `is_cursor_style` and that the default is a member of the valid set. It never called `_load_cursor_style_preference`, so the fallback its name claimed to cover was unexercised, and reverting that fallback to the original unchecked `cast` left it green. Renames it to what it tests, and adds one that drives the reader end to end. Records what the new test found: coercion rejects the string at the provider, so the reader receives the manifest default and its own `is_cursor_style` guard never fires for a TOML value. The guard is defense in depth against a value that bypassed coercion, not the branch that handles a bad config -- so reverting it still passes, and the docstring says so rather than implying coverage the test does not give.
`resolve_startup_mode_with_source` asserted `cast("str", resolved.value)` and
argued no non-`str` can reach it. The argument is sound, but it is the odd one
out in a branch that introduced `is_cursor_style` and `is_valid_recursion_limit`
as `TypeIs` predicates for exactly this shape -- and the two fail in opposite
directions. A predicate falls back; a `cast` lets a value that did slip past
coercion propagate typed as `str` into the display path.
The comment also cited a `VALID_STARTUP_MODES` that does not exist anywhere in
the package.
Removes the last `cast` from `config_manifest`, so the import goes with it.
The theme and terminal-mapping writes were the two siblings the managed-policy reporting commit did not reach. They cannot use `_managed_verdict`: theme resolution is bespoke, since a `[ui.terminal_themes]` entry decides only when it matches the running terminal, so a per-option probe would announce policy as effective when it is not. What they did share was the health bug. Both read `load_managed_config_toml()`, which returns an empty table for a file it could not read, so an unreadable policy file produced the plain "Theme preference saved." -- indistinguishable from no policy, and no signal to the administrator either way. Adds `_managed_theme_note`, which checks the snapshot's health before asking whether policy decides the theme, and collapses the duplicated block at both call sites.
Each returns a usable value and says nothing, so the failure looks like a setting the user never wrote: - `_load_float_option` pre-seeds `value` with the valid default, so the invalid-value check below cannot fire for an unregistered key -- an unknown option returned the default with nothing logged, while every sibling reader reports it. - `_load_user_themes` returned early on a `[themes]` table that is not a table. The reader emits no ranked diagnostics, so a managed or user theme block rejected as malformed left every user theme inert and unreported. - `_resolve_option_without_managed` substitutes an empty user tier when the shared resolver has no user provider. That cannot happen without a programming error, and quietly resolving against nothing is indistinguishable from a user who declared nothing.
… key Three shapes where the type was leaving a real decision to copy-paste. `TomlSnapshot` was built inline at nine call sites, and the three that wanted an empty user tier picked three different healths for it: `OK`, `MISSING`, and `INDETERMINATE`. `ProviderStatus.usable` gates whether a source takes part in resolution, so that is a fail-open/fail-closed choice. Adds `from_table`, `declaring_nothing`, `absent`, and `unknown_origin` so each site states which one it means. The resolver cache key was `tuple[object, ...]`, in a docstring that argues carefully for one field over two and then leaves the key half untyped. A key built with different arity or field order compares unequal, silently rebuilds the resolver, and loses the single-generation guarantee with no error. Now a frozen `_ResolverKey(user_path, managed_path)`. `resolver_from_snapshots` is keyword-only, with a docstring about how a transposed positional argument puts user data at `MANAGED_RANK` and nothing downstream catches it -- and then built both providers with six positional arguments, including a bare `True` for `durable`. Its own argument applies. Also drops the `TomlSnapshot(snapshot.data, status)` re-wrap in `doctor.py`. `status` round-trips from a provider seeded with that same snapshot, so it was a no-op that read as meaningful, and the one place `__post_init__` could raise inside the command whose job is to survive a broken config.
Two public functions this branch relies on had no direct test. `toml_snapshot` was reached only through `_resolve_option_without_managed`, and `get_healthy_managed_snapshot` only through `_reload_values` -- it is the startup gate for managed policy, so its own contract is worth pinning: an enforceable file passes, a known section declared as a scalar raises `ManagedPolicyError`, and a file that does not parse raises `ManagedConfigError`.
`reset_config_resolver` cleared the source diagnostics through a lazy `config_manifest` import, which broke the session-stats test that stubs that module out of `sys.modules` to simulate an ImportError -- the failure landed in the conftest teardown, not the test body. The clear was redundant regardless: dropping the cache entry makes the next `get_config_resolver` take the cache-miss branch, which re-arms the diagnostics as part of building the new generation. Mutation-checked that the rebuild-path clear is still the one being pinned.
Mason Daugherty (mdrxy)
deleted the
mdrxy/code/retire-legacy-config-wrap
branch
August 24, 2026 16:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ConfigResolver(with its cached per-process snapshot) shipped in #5736, but every production reader still went through the legacyresolve_scalar/resolve_ranked_scalarwrappers, which re-parseconfig.tomlon 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.tomlwhile 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.mdand 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
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 callget_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_valuesresolves throughget_config_resolver(refresh_managed=refresh_managed), so/reloadand later readers observe the same generation instead of the cache going stale; the env tier still comes from the method'senvargument, and the "a failed or blocked reload never drops policy in force" invariant is unchanged.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 — theconfigCLI (one read per invocation),update_check(health reported next to the value), the sandbox/theme loaders (a non-defaultconfig_pathexcludes managed policy), and the managed-policy validators (a candidate generation not yet in force) — it buildsresolver_from_snapshots(TomlSnapshot(...), TomlSnapshot(...))and calls.get(option).The legacy path is deleted.
resolve_scalar,resolve_ranked_scalar, and the now-dead_coerce_envhelper are gone fromconfig_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 throughConfigResolver/resolver_from_snapshotsor real TOML files under the test-redirected config path.The remaining fresh-parse readers, and why. Two keep a concrete parse, and neither is on a live path:
resolve_read_project_dotenvruns during dotenv bootstrap, before the project.envis layered intoos.environ; seeding the shared generation there would capture an env tier later readers do not see.resolve_startup_mode_with_sourceinspects 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
/reloadpreviews read a stale user tier. The preview path shares_reload_valueswithrefresh_managed=False, so its cached user snapshot could predate the[shell].allow_listedit 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 asproject_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._save_ui_bool_resultwas 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./reloadreloaded every provider twice.get_config_resolver(refresh_managed=True)already reloads on a cache hit, so the explicit.reload()was redundant: one/reloadread the managed file four times and re-ranmanaged_policy_violationswith each.test_populated_tiers_actually_reach_the_resolverdid not depend on theresolve_scalaroracle, andmerge_strategyappeared nowhere else in the suite — so flippingthreads.columnsfromDEEP_MERGEtoREPLACE, ormcp.disabled_serversfromUNIONtoREPLACE, passed the full suite. Restored and mutation-checked in both directions.refresh_managedwas untested. Removing the refresh from/reloadkept the suite green, because the existing reload tests drive env rather than a file edit and monkeypatchDEFAULT_CONFIG_PATH(which changes the resolver cache key and rebuilds a fresh resolver, hiding staleness). Now pinned in both directions.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_diagnosticsand_ranked_sourcenow have nine external importers, and the snapshot-construction block appears at six production sites. A smallresolve_for_display(option)seam (get + emit + label) would restore the encapsulation the wrapper provided and make the missing-emit bug above structurally impossible./reloadwould close it.cast(...)calls exist only becauseConfigResolver.getreturnsResolvedValue[object]; three sit behind validating predicates that could beTypeGuards.AGENTS.mdtreatscastas a last resort.