mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(config): guard xai migration writer + drop gratuitous annotation
Phase-2 review follow-ups on the unreadable-config chokepoint work:
- hermes_cli/xai_retirement.py apply_migration() is a full-file config.yaml
rewriter (ruamel round-trip + plain open("w")) that lives outside the
atomic_yaml_write path, so the chokepoint didn't cover it. It reads the
file first (which already fails closed on an unreadable file), but add
require_readable_config_before_write() right before the write as a
backstop for the read-then-write window, and a regression test asserting
the original bytes survive an unreadable config.
- Drop the unnecessary "Path" string quotes on atomic_config_write's
annotation — Path is imported eagerly at module top, no forward ref needed.
auth.py _update_config_for_provider / _reset_config_provider intentionally
keep their standalone require_readable_config_before_write guard + bare
atomic_yaml_write: the guard must fire BEFORE the read (fail-fast) at those
read-then-write sites, and a test pins the atomic_yaml_write call. Both are
already fully guarded against the bug; routing them through the wrapper
would move the check to write time for no benefit.
This commit is contained in:
parent
123c6f3a23
commit
beaa1a08e6
3 changed files with 31 additions and 1 deletions
|
|
@ -6523,7 +6523,7 @@ 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:
|
||||
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
|
||||
|
|
|
|||
|
|
@ -242,6 +242,9 @@ def apply_migration(
|
|||
)
|
||||
shutil.copy2(config_path, backup_path)
|
||||
|
||||
from hermes_cli.config import require_readable_config_before_write
|
||||
|
||||
require_readable_config_before_write(config_path)
|
||||
with config_path.open("w", encoding="utf-8") as fh:
|
||||
yaml.dump(doc, fh)
|
||||
|
||||
|
|
|
|||
|
|
@ -221,3 +221,30 @@ class TestIdempotence:
|
|||
assert issues_2 == []
|
||||
result_2 = apply_migration(trap_config, issues_2)
|
||||
assert result_2.config_changed is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Fail-closed on unreadable existing config
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestUnreadableExistingConfig:
|
||||
def test_apply_refuses_to_overwrite_unreadable_config(self, trap_config: Path):
|
||||
"""apply_migration must not clobber an existing config.yaml it can't
|
||||
read. It reads the file first (which raises on an unreadable file), and
|
||||
the require_readable_config_before_write guard before the write is a
|
||||
belt-and-suspenders backstop for the read-then-write window. Either way
|
||||
the original bytes must survive."""
|
||||
import os
|
||||
|
||||
issues = find_retired_xai_refs(_parse(trap_config))
|
||||
assert issues # sanity: trap_config has retired refs
|
||||
original = trap_config.read_bytes()
|
||||
|
||||
os.chmod(trap_config, 0o000)
|
||||
try:
|
||||
with pytest.raises((PermissionError, RuntimeError, OSError)):
|
||||
apply_migration(trap_config, issues, backup=False)
|
||||
finally:
|
||||
os.chmod(trap_config, 0o644)
|
||||
|
||||
assert trap_config.read_bytes() == original
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue