mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(config): close unreadable-overwrite bug class at a single chokepoint
The unreadable-config-overwrite bug (an existing config.yaml that reads as
{} on a permission/IO error gets replaced with only defaults or the edited
section) is not limited to save_config / config set / auth. The same
read-then-atomic_yaml_write pattern lives at ~7 other independent write
sites that don't route through those functions:
- gateway/slash_commands.py: _save_config_key, memory/skills write_approval
toggles, tool_progress toggle, runtime_footer toggle, personality set
- hermes_cli/doctor.py --fix (stale root-key migration)
- gateway/platforms/yuanbao.py auto-sethome
- plugins/platforms/telegram/adapter.py topic thread_id persistence
- tui_gateway/server.py _save_cfg
- agent/onboarding.py mark_seen
Rather than sprinkle require_readable_config_before_write() at each site,
add a single fail-closed chokepoint, atomic_config_write(), that runs the
guard then delegates to atomic_yaml_write, and route every config.yaml
write through it. Root cause remains that read_raw_config() can't tell an
absent file from an unreadable one (returns {} for both) — read-only
callers correctly stay fail-open, but any full-file replacement now fails
closed in one enforced place instead of relying on each caller to remember
the guard.
save_config / set_config_value / auth keep the contributor's original
guard calls (their commit); this commit widens the fix to the sibling
call paths and adds a regression test on the chokepoint (fails closed on
unreadable existing file + still creates a genuinely absent file).
This commit is contained in:
parent
b109adede6
commit
123c6f3a23
8 changed files with 73 additions and 20 deletions
|
|
@ -209,7 +209,7 @@ def mark_seen(config_path: Path, flag: str) -> bool:
|
|||
"""
|
||||
try:
|
||||
import yaml
|
||||
from utils import atomic_yaml_write
|
||||
from hermes_cli.config import atomic_config_write
|
||||
except Exception as e: # pragma: no cover — dependency issue
|
||||
logger.debug("onboarding: failed to import yaml/utils: %s", e)
|
||||
return False
|
||||
|
|
@ -228,7 +228,7 @@ def mark_seen(config_path: Path, flag: str) -> bool:
|
|||
if seen.get(flag) is True:
|
||||
return True # already marked — nothing to do
|
||||
seen[flag] = True
|
||||
atomic_yaml_write(config_path, cfg)
|
||||
atomic_config_write(config_path, cfg)
|
||||
return True
|
||||
except Exception as e:
|
||||
logger.debug("onboarding: failed to mark flag %s: %s", flag, e)
|
||||
|
|
|
|||
|
|
@ -1650,7 +1650,7 @@ class AutoSetHomeMiddleware(InboundMiddleware):
|
|||
if _should_set:
|
||||
try:
|
||||
from hermes_constants import get_hermes_home
|
||||
from utils import atomic_yaml_write
|
||||
from hermes_cli.config import atomic_config_write
|
||||
import yaml
|
||||
|
||||
_home = get_hermes_home()
|
||||
|
|
@ -1660,7 +1660,7 @@ class AutoSetHomeMiddleware(InboundMiddleware):
|
|||
with open(config_path, encoding="utf-8") as f:
|
||||
user_config = yaml.safe_load(f) or {}
|
||||
user_config["YUANBAO_HOME_CHANNEL"] = ctx.chat_id
|
||||
atomic_yaml_write(config_path, user_config)
|
||||
atomic_config_write(config_path, user_config)
|
||||
os.environ["YUANBAO_HOME_CHANNEL"] = str(ctx.chat_id)
|
||||
logger.info(
|
||||
"[%s] Auto-sethome: designated %s (%s) as Yuanbao home channel",
|
||||
|
|
|
|||
|
|
@ -38,10 +38,9 @@ from gateway.session import (
|
|||
build_session_key,
|
||||
is_shared_multi_user_session,
|
||||
)
|
||||
from hermes_cli.config import cfg_get, clear_model_endpoint_credentials
|
||||
from hermes_cli.config import atomic_config_write, cfg_get, clear_model_endpoint_credentials
|
||||
from utils import (
|
||||
atomic_json_write,
|
||||
atomic_yaml_write,
|
||||
base_url_host_matches,
|
||||
is_truthy_value,
|
||||
)
|
||||
|
|
@ -2093,7 +2092,7 @@ class GatewaySlashCommandsMixin:
|
|||
if "agent" not in config or not isinstance(config.get("agent"), dict):
|
||||
config["agent"] = {}
|
||||
config["agent"]["system_prompt"] = ""
|
||||
atomic_yaml_write(config_path, config)
|
||||
atomic_config_write(config_path, config)
|
||||
except Exception as e:
|
||||
return t("gateway.personality.save_failed", error=str(e))
|
||||
self._ephemeral_system_prompt = ""
|
||||
|
|
@ -2106,7 +2105,7 @@ class GatewaySlashCommandsMixin:
|
|||
if "agent" not in config or not isinstance(config.get("agent"), dict):
|
||||
config["agent"] = {}
|
||||
config["agent"]["system_prompt"] = new_prompt
|
||||
atomic_yaml_write(config_path, config)
|
||||
atomic_config_write(config_path, config)
|
||||
except Exception as e:
|
||||
return t("gateway.personality.save_failed", error=str(e))
|
||||
|
||||
|
|
@ -2652,7 +2651,7 @@ class GatewaySlashCommandsMixin:
|
|||
current[k] = {}
|
||||
current = current[k]
|
||||
current[keys[-1]] = value
|
||||
atomic_yaml_write(config_path, user_config)
|
||||
atomic_config_write(config_path, user_config)
|
||||
return True
|
||||
except Exception as e:
|
||||
logger.error("Failed to save config key %s: %s", key_path, e)
|
||||
|
|
@ -2755,7 +2754,7 @@ class GatewaySlashCommandsMixin:
|
|||
with open(config_path, encoding="utf-8") as f:
|
||||
user_config = yaml.safe_load(f) or {}
|
||||
user_config.setdefault("memory", {})["write_approval"] = bool(enabled)
|
||||
atomic_yaml_write(config_path, user_config)
|
||||
atomic_config_write(config_path, user_config)
|
||||
# New setting must take effect next message → drop cached agent.
|
||||
self._evict_cached_agent(session_key)
|
||||
|
||||
|
|
@ -2811,7 +2810,7 @@ class GatewaySlashCommandsMixin:
|
|||
with open(config_path, encoding="utf-8") as f:
|
||||
user_config = yaml.safe_load(f) or {}
|
||||
user_config.setdefault("skills", {})["write_approval"] = bool(enabled)
|
||||
atomic_yaml_write(config_path, user_config)
|
||||
atomic_config_write(config_path, user_config)
|
||||
# New setting must take effect next message → drop cached agent.
|
||||
self._evict_cached_agent(session_key)
|
||||
|
||||
|
|
@ -2863,7 +2862,7 @@ class GatewaySlashCommandsMixin:
|
|||
current[k] = {}
|
||||
current = current[k]
|
||||
current[keys[-1]] = value
|
||||
atomic_yaml_write(config_path, user_config)
|
||||
atomic_config_write(config_path, user_config)
|
||||
return True
|
||||
except Exception as e:
|
||||
logger.error("Failed to save config key %s: %s", key_path, e)
|
||||
|
|
@ -2960,7 +2959,7 @@ class GatewaySlashCommandsMixin:
|
|||
if platform_key not in display["platforms"] or not isinstance(display["platforms"].get(platform_key), dict):
|
||||
display["platforms"][platform_key] = {}
|
||||
display["platforms"][platform_key]["tool_progress"] = new_mode
|
||||
atomic_yaml_write(config_path, user_config)
|
||||
atomic_config_write(config_path, user_config)
|
||||
return (
|
||||
f"{descriptions[new_mode]}\n"
|
||||
+ t("gateway.verbose.saved_suffix", platform=platform_key)
|
||||
|
|
@ -3035,7 +3034,7 @@ class GatewaySlashCommandsMixin:
|
|||
if not isinstance(display.get("runtime_footer"), dict):
|
||||
display["runtime_footer"] = {}
|
||||
display["runtime_footer"]["enabled"] = new_state
|
||||
atomic_yaml_write(config_path, user_config)
|
||||
atomic_config_write(config_path, user_config)
|
||||
except Exception as e:
|
||||
logger.warning("Failed to save runtime_footer.enabled: %s", e)
|
||||
return t("gateway.config_save_failed", error=e)
|
||||
|
|
|
|||
|
|
@ -6523,6 +6523,32 @@ def require_readable_config_before_write(config_path: Optional[Path] = None) ->
|
|||
) from exc
|
||||
|
||||
|
||||
def atomic_config_write(config_path: "Path", data: Any, **kwargs: Any) -> None:
|
||||
"""Fail-closed atomic write for ``config.yaml``.
|
||||
|
||||
The single chokepoint every config-update path should use instead of
|
||||
calling :func:`utils.atomic_yaml_write` directly. It runs
|
||||
:func:`require_readable_config_before_write` first, so a full-file
|
||||
replacement can never silently clobber an existing ``config.yaml`` that
|
||||
degraded to an empty dict on read (permission error, broken mount,
|
||||
transient I/O). New-file creation still works when the path is absent.
|
||||
|
||||
Root cause this guards: ``read_raw_config()`` returns ``{}`` for BOTH an
|
||||
absent file and an unreadable-but-present file. Callers that read then
|
||||
overwrite can't tell the two apart, so an unreadable config would be
|
||||
replaced with only defaults or the single edited section. Routing every
|
||||
write through this helper enforces the invariant in one place rather than
|
||||
relying on each of ~15 independent write sites to remember the guard.
|
||||
|
||||
``kwargs`` are forwarded verbatim to ``atomic_yaml_write``
|
||||
(``sort_keys``, ``default_flow_style``, ``extra_content``, ...).
|
||||
"""
|
||||
from utils import atomic_yaml_write
|
||||
|
||||
require_readable_config_before_write(config_path)
|
||||
atomic_yaml_write(config_path, data, **kwargs)
|
||||
|
||||
|
||||
def load_config() -> Dict[str, Any]:
|
||||
"""Load configuration from ~/.hermes/config.yaml.
|
||||
|
||||
|
|
|
|||
|
|
@ -983,8 +983,8 @@ def run_doctor(args):
|
|||
model_section[k] = raw_config.pop(k)
|
||||
else:
|
||||
raw_config.pop(k)
|
||||
from utils import atomic_yaml_write
|
||||
atomic_yaml_write(config_path, raw_config)
|
||||
from hermes_cli.config import atomic_config_write
|
||||
atomic_config_write(config_path, raw_config)
|
||||
check_ok("Migrated stale root-level keys into model section")
|
||||
fixed_count += 1
|
||||
else:
|
||||
|
|
|
|||
|
|
@ -2727,9 +2727,9 @@ class TelegramAdapter(BasePlatformAdapter):
|
|||
changed = True
|
||||
|
||||
if changed:
|
||||
from utils import atomic_yaml_write
|
||||
from hermes_cli.config import atomic_config_write
|
||||
|
||||
atomic_yaml_write(
|
||||
atomic_config_write(
|
||||
config_path,
|
||||
config,
|
||||
default_flow_style=False,
|
||||
|
|
|
|||
|
|
@ -318,6 +318,34 @@ class TestSaveAndLoadRoundtrip:
|
|||
|
||||
assert config_path.read_text(encoding="utf-8") == original
|
||||
|
||||
def test_atomic_config_write_refuses_unreadable_existing_config(self, tmp_path):
|
||||
"""The shared chokepoint every sibling write site routes through must
|
||||
fail closed on an unreadable existing config.yaml — this locks in the
|
||||
whole bug class (gateway slash commands, doctor --fix, yuanbao/telegram
|
||||
auto-sethome, tui_gateway _save_cfg), not just the three named paths."""
|
||||
from hermes_cli.config import atomic_config_write
|
||||
|
||||
config_path = tmp_path / "config.yaml"
|
||||
original = "model:\n provider: openrouter\n"
|
||||
config_path.write_text(original, encoding="utf-8")
|
||||
|
||||
with patch("builtins.open", side_effect=self._deny_config_reads(config_path)):
|
||||
with pytest.raises(RuntimeError, match="Refusing to overwrite"):
|
||||
atomic_config_write(config_path, {"model": {"provider": "openai"}})
|
||||
|
||||
assert config_path.read_text(encoding="utf-8") == original
|
||||
|
||||
def test_atomic_config_write_creates_new_file(self, tmp_path):
|
||||
"""A genuinely absent config.yaml must still be created — the guard
|
||||
only refuses to clobber an existing-but-unreadable file."""
|
||||
from hermes_cli.config import atomic_config_write
|
||||
|
||||
config_path = tmp_path / "config.yaml"
|
||||
assert not config_path.exists()
|
||||
atomic_config_write(config_path, {"model": {"provider": "openrouter"}})
|
||||
assert config_path.exists()
|
||||
assert "openrouter" in config_path.read_text(encoding="utf-8")
|
||||
|
||||
def test_save_config_normalizes_legacy_root_level_max_turns(self, tmp_path):
|
||||
with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}):
|
||||
save_config({"model": "test/custom-model", "max_turns": 37})
|
||||
|
|
|
|||
|
|
@ -1805,10 +1805,10 @@ def _apply_managed(cfg: dict) -> dict:
|
|||
def _save_cfg(cfg: dict):
|
||||
global _cfg_cache, _cfg_mtime, _cfg_path
|
||||
|
||||
from utils import atomic_yaml_write
|
||||
from hermes_cli.config import atomic_config_write
|
||||
|
||||
path = _hermes_home / "config.yaml"
|
||||
atomic_yaml_write(path, cfg)
|
||||
atomic_config_write(path, cfg)
|
||||
with _cfg_lock:
|
||||
_cfg_cache = copy.deepcopy(cfg)
|
||||
_cfg_path = path
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue