mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(settings): accept documented HEADROOM_* env names as settings keys (#2833)
## Description Settings validation only accepted short JSON/API keys, so documented HEADROOM_* env names were rejected as unknown. Users following the docs (for example HEADROOM_LOSSLESS) hit SettingsValidationError / PUT /settings 400 even though those names are already on each registry field. This normalizes known env aliases to their short keys before validate/save, keeps existing short-key behavior, and rejects conflicting env+key pairs for the same field. Closes #2812 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - Added _BY_ENV and _normalize_values() in settings_store to map documented env names to short keys - Call normalization at the start of validate() and save() so clear/retain paths also accept env aliases - Reject payloads that supply both an env alias and its short key with different values - Add unit coverage for accept/clear/conflict/same-value paths and update registry monkeypatches to rebuild _BY_ENV ## Testing - [x] Unit tests pass (pytest) - [x] Linting passes (ruff check .) - [ ] Type checking passes (mypy headroom) - [x] New tests added for new functionality - [ ] Manual testing performed ### Test Output ```text python -m pytest tests/test_proxy/test_settings_store.py -q -k "env_alias or validate_accepts or save_rejects or same_env or conflicting or save_accepts or env_alias_clear or anthropic_extra_headers_retain or TestValidation" 23 passed, 11 deselected ruff check headroom/settings_store.py tests/test_proxy/test_settings_store.py tests/test_proxy_settings_endpoints.py All checks passed! ruff format --check headroom/settings_store.py tests/test_proxy/test_settings_store.py tests/test_proxy_settings_endpoints.py 3 files already formatted ``` ## Real Behavior Proof - Environment: Linux x86_64, Python 3.14.5 via contributor venv, worktree of headroom main at7940c05eplus commite4c87edf- Exact command / steps: pytest tests/test_proxy/test_settings_store.py focused selection; ruff check and ruff format --check on the three touched files; settings_store.validate({"HEADROOM_LOSSLESS": True}) returns {"lossless": True} - Observed result: Env aliases coerce and persist under short keys; unknown short keys still error; conflicting env+key pairs raise SettingsValidationError; ruff clean on touched files - Not tested: Live dashboard PUT /settings through a running proxy (HTTP suite needs native headroom._core); mypy; full monorepo CI ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project style guidelines - [x] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I did not edit CHANGELOG.md - it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this) ## Screenshots (if applicable) N/A ## Additional Notes - Scoped to settings key normalization only - Registry drift Click test was not exercised here because this environment lacks tomlkit for an unrelated import path
This commit is contained in:
parent
fd4628d821
commit
de9e0523da
3 changed files with 66 additions and 0 deletions
|
|
@ -697,6 +697,7 @@ SETTINGS: tuple[SettingField, ...] = (
|
||||||
)
|
)
|
||||||
|
|
||||||
_BY_KEY: dict[str, SettingField] = {f.key: f for f in SETTINGS}
|
_BY_KEY: dict[str, SettingField] = {f.key: f for f in SETTINGS}
|
||||||
|
_BY_ENV: dict[str, SettingField] = {f.env: f for f in SETTINGS}
|
||||||
|
|
||||||
|
|
||||||
class SettingsValidationError(Exception):
|
class SettingsValidationError(Exception):
|
||||||
|
|
@ -714,6 +715,27 @@ class SettingsValidationError(Exception):
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _normalize_values(values: dict[str, Any]) -> dict[str, Any]:
|
||||||
|
"""Rewrite known env aliases to their JSON/API keys."""
|
||||||
|
normalized: dict[str, Any] = {}
|
||||||
|
source_keys: dict[str, str] = {}
|
||||||
|
conflicts: dict[str, str] = {}
|
||||||
|
for incoming_key, value in values.items():
|
||||||
|
field = _BY_ENV.get(incoming_key)
|
||||||
|
key = field.key if field is not None else incoming_key
|
||||||
|
if key in normalized:
|
||||||
|
if normalized[key] != value:
|
||||||
|
conflicts[key] = (
|
||||||
|
f"conflicting values supplied for {source_keys[key]!r} and {incoming_key!r}"
|
||||||
|
)
|
||||||
|
continue
|
||||||
|
normalized[key] = value
|
||||||
|
source_keys[key] = incoming_key
|
||||||
|
if conflicts:
|
||||||
|
raise SettingsValidationError([], conflicts)
|
||||||
|
return normalized
|
||||||
|
|
||||||
|
|
||||||
def _coerce(field: SettingField, value: Any) -> Any:
|
def _coerce(field: SettingField, value: Any) -> Any:
|
||||||
"""Coerce a raw JSON/env value to the field's Python type.
|
"""Coerce a raw JSON/env value to the field's Python type.
|
||||||
|
|
||||||
|
|
@ -792,6 +814,7 @@ def validate(values: dict[str, Any]) -> dict[str, Any]:
|
||||||
Raises :class:`SettingsValidationError` when any key is unknown or any value
|
Raises :class:`SettingsValidationError` when any key is unknown or any value
|
||||||
fails coercion. Returns the coerced dict (``None`` values dropped) on success.
|
fails coercion. Returns the coerced dict (``None`` values dropped) on success.
|
||||||
"""
|
"""
|
||||||
|
values = _normalize_values(values)
|
||||||
unknown = [key for key in values if key not in _BY_KEY]
|
unknown = [key for key in values if key not in _BY_KEY]
|
||||||
field_errors: dict[str, str] = {}
|
field_errors: dict[str, str] = {}
|
||||||
coerced: dict[str, Any] = {}
|
coerced: dict[str, Any] = {}
|
||||||
|
|
@ -879,6 +902,7 @@ def save(values: dict[str, Any]) -> None:
|
||||||
secret's display value verbatim when the user hasn't touched it; anything
|
secret's display value verbatim when the user hasn't touched it; anything
|
||||||
else is validated/coerced and stored.
|
else is validated/coerced and stored.
|
||||||
"""
|
"""
|
||||||
|
values = _normalize_values(values)
|
||||||
clear_keys = {key for key, value in values.items() if value is None and key in _BY_KEY}
|
clear_keys = {key for key, value in values.items() if value is None and key in _BY_KEY}
|
||||||
retained_keys = {
|
retained_keys = {
|
||||||
key
|
key
|
||||||
|
|
|
||||||
|
|
@ -63,6 +63,37 @@ class TestRoundTrip:
|
||||||
|
|
||||||
|
|
||||||
class TestValidation:
|
class TestValidation:
|
||||||
|
def test_validate_accepts_env_aliases(self, workspace):
|
||||||
|
assert settings_store.validate({"HEADROOM_LOSSLESS": True, "HEADROOM_RPM": "30"}) == {
|
||||||
|
"lossless": True,
|
||||||
|
"rpm": 30,
|
||||||
|
}
|
||||||
|
|
||||||
|
def test_save_accepts_env_alias_and_persists_short_key(self, workspace):
|
||||||
|
settings_store.save({"HEADROOM_LOSSLESS": True})
|
||||||
|
|
||||||
|
assert settings_store.load() == {"lossless": True}
|
||||||
|
|
||||||
|
def test_env_alias_clear_removes_short_key(self, workspace):
|
||||||
|
settings_store.save({"lossless": True})
|
||||||
|
|
||||||
|
settings_store.save({"HEADROOM_LOSSLESS": None})
|
||||||
|
|
||||||
|
assert settings_store.load() == {}
|
||||||
|
|
||||||
|
def test_conflicting_env_alias_and_short_key_rejected(self, workspace):
|
||||||
|
with pytest.raises(settings_store.SettingsValidationError) as exc:
|
||||||
|
settings_store.validate({"HEADROOM_LOSSLESS": True, "lossless": False})
|
||||||
|
|
||||||
|
assert exc.value.unknown_keys == []
|
||||||
|
assert "lossless" in exc.value.field_errors
|
||||||
|
assert "HEADROOM_LOSSLESS" in exc.value.field_errors["lossless"]
|
||||||
|
|
||||||
|
def test_same_env_alias_and_short_key_value_is_accepted(self, workspace):
|
||||||
|
assert settings_store.validate({"HEADROOM_LOSSLESS": True, "lossless": True}) == {
|
||||||
|
"lossless": True
|
||||||
|
}
|
||||||
|
|
||||||
def test_save_rejects_unknown_key(self, workspace):
|
def test_save_rejects_unknown_key(self, workspace):
|
||||||
with pytest.raises(settings_store.SettingsValidationError) as exc:
|
with pytest.raises(settings_store.SettingsValidationError) as exc:
|
||||||
settings_store.save({"nope": 1})
|
settings_store.save({"nope": 1})
|
||||||
|
|
@ -156,6 +187,7 @@ class TestSecretMasking:
|
||||||
)
|
)
|
||||||
monkeypatch.setattr(settings_store, "SETTINGS", registry)
|
monkeypatch.setattr(settings_store, "SETTINGS", registry)
|
||||||
monkeypatch.setattr(settings_store, "_BY_KEY", {f.key: f for f in registry})
|
monkeypatch.setattr(settings_store, "_BY_KEY", {f.key: f for f in registry})
|
||||||
|
monkeypatch.setattr(settings_store, "_BY_ENV", {f.env: f for f in registry})
|
||||||
settings_store.save({"log_file": "/tmp/secret.log"})
|
settings_store.save({"log_file": "/tmp/secret.log"})
|
||||||
|
|
||||||
stored = settings_store.stored_values()
|
stored = settings_store.stored_values()
|
||||||
|
|
@ -187,6 +219,15 @@ class TestSecretMasking:
|
||||||
"Saving _MASK should retain the stored value, not overwrite it"
|
"Saving _MASK should retain the stored value, not overwrite it"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
def test_anthropic_extra_headers_retain_on_env_alias_mask(self, workspace, monkeypatch):
|
||||||
|
"""Saving _MASK through the env alias retains the stored value."""
|
||||||
|
_clear_env(monkeypatch)
|
||||||
|
settings_store.save({"anthropic_extra_headers": '{"Api-Key": "secret123"}'})
|
||||||
|
|
||||||
|
settings_store.save({"ANTHROPIC_TARGET_API_HEADERS": settings_store._MASK})
|
||||||
|
|
||||||
|
assert settings_store.load().get("anthropic_extra_headers") == '{"Api-Key": "secret123"}'
|
||||||
|
|
||||||
def test_anthropic_extra_headers_clear_on_none(self, workspace, monkeypatch):
|
def test_anthropic_extra_headers_clear_on_none(self, workspace, monkeypatch):
|
||||||
"""Saving None for anthropic_extra_headers removes it."""
|
"""Saving None for anthropic_extra_headers removes it."""
|
||||||
_clear_env(monkeypatch)
|
_clear_env(monkeypatch)
|
||||||
|
|
|
||||||
|
|
@ -207,6 +207,7 @@ class TestSecretMasking:
|
||||||
)
|
)
|
||||||
monkeypatch.setattr(settings_store, "SETTINGS", registry)
|
monkeypatch.setattr(settings_store, "SETTINGS", registry)
|
||||||
monkeypatch.setattr(settings_store, "_BY_KEY", {f.key: f for f in registry})
|
monkeypatch.setattr(settings_store, "_BY_KEY", {f.key: f for f in registry})
|
||||||
|
monkeypatch.setattr(settings_store, "_BY_ENV", {f.env: f for f in registry})
|
||||||
settings_store.save({"log_file": "/tmp/secret.log"})
|
settings_store.save({"log_file": "/tmp/secret.log"})
|
||||||
|
|
||||||
assert client.get("/settings").json()["log_file"] == settings_store._MASK
|
assert client.get("/settings").json()["log_file"] == settings_store._MASK
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue