fix(config): strip unknown top-level keys from config file on load - #2974
Conversation
When load_user_config() encounters foreign keys (from fuzzing artifacts, server PUT, or external tooling), it now removes them from disk in addition to warning. This means the warning fires once and then the file is clean, satisfying the acceptance criteria of #2969. - Add _KNOWN_CONFIG_KEYS constant (the 9 recognised top-level sections) - Add _strip_unknown_config_keys() helper that rewrites the file without the offending keys, using tomlkit to preserve formatting of everything else - Update the warning message to say "stripping from <path>" so users know the cleanup is automatic - Apply stripping to both main config.toml and config.local.toml when a local override file exists - Add 4 regression tests covering: basic stripping, known-key preservation, local-config stripping, and [plugin.*] section survival Closes #2969
Greptile SummaryThis PR fixes a repeated-warning annoyance (#2969) by writing the auto-detected unknown top-level config keys back to disk on the first load, so subsequent invocations produce no warning. It also addresses two follow-up issues from prior review rounds: the write is now wrapped in
Confidence Score: 5/5Safe to merge — the stripping logic is correct for both single-file and merged-config scenarios, and the OSError guard makes write failures survivable. The detection and stripping logic correctly operates on individually-parsed files using the unknown-key set derived from the merged in-memory dict, with No files require special attention; both changed files are straightforward and well-covered by tests. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[load_user_config called] --> B[_load_config_doc: read config.toml]
B --> C{config.local.toml exists?}
C -- yes --> D[Load & merge config.local.toml]
C -- no --> E[Use main config only]
D --> F[pop all known keys from merged dict]
E --> F
F --> G{Any keys remain?}
G -- no --> H[Build & return UserConfig]
G -- yes --> I[Log warning with strip_targets]
I --> J[_strip_unknown_config_keys: config.toml]
J --> K{OSError?}
K -- yes --> L[Log warning, skip write]
K -- no --> M[Write cleaned file]
M --> N{has_local?}
L --> N
N -- yes --> O[_strip_unknown_config_keys: config.local.toml]
O --> H
N -- no --> H
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[load_user_config called] --> B[_load_config_doc: read config.toml]
B --> C{config.local.toml exists?}
C -- yes --> D[Load & merge config.local.toml]
C -- no --> E[Use main config only]
D --> F[pop all known keys from merged dict]
E --> F
F --> G{Any keys remain?}
G -- no --> H[Build & return UserConfig]
G -- yes --> I[Log warning with strip_targets]
I --> J[_strip_unknown_config_keys: config.toml]
J --> K{OSError?}
K -- yes --> L[Log warning, skip write]
K -- no --> M[Write cleaned file]
M --> N{has_local?}
L --> N
N -- yes --> O[_strip_unknown_config_keys: config.local.toml]
O --> H
N -- no --> H
Reviews (3): Last reviewed commit: "fix(config): catch OSError when strippin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Top-level keys recognised by load_user_config(). Anything else is foreign. | ||
| _KNOWN_CONFIG_KEYS: frozenset[str] = frozenset( | ||
| { | ||
| "prompt", | ||
| "user", | ||
| "env", | ||
| "mcp", | ||
| "providers", | ||
| "models", | ||
| "lessons", | ||
| "plugins", | ||
| "plugin", | ||
| } | ||
| ) |
There was a problem hiding this comment.
_KNOWN_CONFIG_KEYS is never referenced anywhere in the code — it is dead code. The unknown-key detection works through the pop() pattern in load_user_config, and _strip_unknown_config_keys receives the already-computed set of unknowns from the caller. Because the constant is not used to drive any logic, it creates a maintenance trap: if a future contributor adds a new section via config.pop(…) but forgets to update this set (or vice versa), nothing breaks at runtime but the constant silently misrepresents the truth. Either wire the constant into the detection logic or remove it.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 26c6706: removed the dead _KNOWN_CONFIG_KEYS constant entirely. The detection logic via config.pop() is the single source of truth — no separate constant to drift.
There was a problem hiding this comment.
Fixed in 26c6706 — _KNOWN_CONFIG_KEYS was removed.
| logger.warning( | ||
| f"Unknown keys in config: {sorted(unknown)} — stripping from" | ||
| f" {path_with_tilde(config_file)}" | ||
| ) | ||
| _strip_unknown_config_keys(str(config_file), unknown) | ||
| if has_local: | ||
| _strip_unknown_config_keys(str(local_path), unknown) |
There was a problem hiding this comment.
config.local.toml when it exists. A user whose unknown key lives exclusively in config.local.toml will see "stripping from config.toml" in the log and look in the wrong place. The message should mention both paths when the local file is also being modified.
| logger.warning( | |
| f"Unknown keys in config: {sorted(unknown)} — stripping from" | |
| f" {path_with_tilde(config_file)}" | |
| ) | |
| _strip_unknown_config_keys(str(config_file), unknown) | |
| if has_local: | |
| _strip_unknown_config_keys(str(local_path), unknown) | |
| strip_targets = str(path_with_tilde(config_file)) | |
| if has_local: | |
| strip_targets += f" and {path_with_tilde(local_path)}" | |
| logger.warning( | |
| f"Unknown keys in config: {sorted(unknown)} — stripping from" | |
| f" {strip_targets}" | |
| ) | |
| _strip_unknown_config_keys(str(config_file), unknown) | |
| if has_local: | |
| _strip_unknown_config_keys(str(local_path), unknown) |
There was a problem hiding this comment.
Fixed in 26c6706: warning now builds strip_targets mentioning both paths when has_local is true, matching the suggestion.
There was a problem hiding this comment.
Fixed in 26c6706 — warning now includes both paths when config.local.toml is also modified.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… both config paths
|
@greptileai review |
| if changed: | ||
| with open(path, "w") as f: | ||
| tomlkit.dump(doc, f) |
There was a problem hiding this comment.
_strip_unknown_config_keys has no exception handling around the file write. If config.toml is read-only (e.g., bind-mounted read-only in a container, owned by root, or placed in a read-only filesystem), open(path, "w") will raise PermissionError and propagate all the way out of load_user_config. Before this PR, encountering unknown keys was survivable — only a warning was logged. After this PR, the same situation causes a hard crash. Wrapping the write in a try/except OSError and logging a warning instead would preserve the original resilience.
There was a problem hiding this comment.
Fixed in f9227a5 — wrapped the write in try/except OSError so a read-only config file degrades to a warning instead of crashing load_user_config.
If config.toml is read-only (e.g. bind-mounted in a container), the open() call would raise PermissionError and crash load_user_config. Before this PR, unknown keys only logged a warning — wrap the write in try/except OSError to preserve that resilience.
|
@greptileai review |
1 participant
Summary
load_user_config()now strips unknown top-level keys from disk after warning about them, so the warning fires only once instead of on every invocation_KNOWN_CONFIG_KEYSconstant listing the 9 recognised top-level sections_strip_unknown_config_keys()helper that rewrites the file with tomlkit (preserving TOML formatting and comments for known keys)config.tomlandconfig.local.tomlwhen the latter exists[plugin.*]section survivalFixes #2969.
Test plan
uv run pytest tests/test_config.py— 70 tests pass~/.config/gptme/config.tomlwith an unknown key, rungptme-util tools list— warning fires once, key is gone on next invocation