mirror of
https://github.com/nesquena/hermes-webui.git
synced 2026-05-25 11:10:18 +00:00
v0.50.257: CRITICAL Opus finding — fix non-functional per-session toolset override
Opus pre-release advisor caught a 5th issue not covered by my initial follow-up sweep, this one CRITICAL: PR #1402 #493 per-session toolset override silently no-op'd every time. Bug: api/streaming.py:1755 called _session_meta.get('enabled_toolsets') on the result of Session.load_metadata_only(). It returns a Session INSTANCE, not a dict. .get() raised AttributeError, which the surrounding bare except swallowed silently. The toolset chip in the UI saved correctly to disk, but the streaming agent always ran with global toolsets. Fix: use getattr(_session_meta, 'enabled_toolsets', None). Two new regression tests: - Source-level: forbid the .get() / [] dict-access shape. - Runtime: Session.load_metadata_only must return a Session instance. Full suite: 3604 passed, 0 failed.
This commit is contained in:
@@ -21,6 +21,8 @@
|
||||
|
||||
- **`_handle_cron_history` clamps `offset` and `limit`** — raw `int(qs.get("offset", ["0"])[0])` raised `ValueError` on `?offset=foo` and surfaced as a generic 500. No upper bound on `limit` either. Now wrapped in `try/except (ValueError, TypeError)` returning a 400 on bad input, and `limit` clamped to `[1, 500]`. (`api/routes.py`)
|
||||
|
||||
- **CRITICAL: per-session toolset override (#493) was non-functional** — `_run_agent_streaming` called `_session_meta.get('enabled_toolsets')` on the result of `Session.load_metadata_only()`, which returns a Session **instance** (not a dict). The `AttributeError` was swallowed by the surrounding `except Exception:` block, so the user's toolset chip silently no-op'd every time and the agent always ran with the global toolsets. Caught by Opus pre-release advisor on the empirical streaming path (CI green, contributor tests green — would have shipped non-functional). Fix uses `getattr(_session_meta, 'enabled_toolsets', None)`. Source-level negative-pattern test prevents the dict-access shape from returning. (`api/streaming.py`, `tests/test_v050257_opus_followups.py`)
|
||||
|
||||
|
||||
## [v0.50.256] — 2026-05-01
|
||||
|
||||
|
||||
+9
-2
@@ -1752,8 +1752,15 @@ def _run_agent_streaming(
|
||||
_session_path = SESSION_DIR / f"{session_id}.json"
|
||||
if _session_path.exists():
|
||||
_session_meta = Session.load_metadata_only(session_id)
|
||||
if _session_meta and _session_meta.get('enabled_toolsets'):
|
||||
_toolsets = _session_meta['enabled_toolsets']
|
||||
# load_metadata_only returns a Session INSTANCE, not a dict.
|
||||
# The previous .get('enabled_toolsets') raised AttributeError
|
||||
# which was swallowed by the bare except below — the entire
|
||||
# per-session toolset override silently no-op'd. Use
|
||||
# getattr() to read the attribute correctly.
|
||||
# (Opus pre-release advisor finding for v0.50.257.)
|
||||
_override = getattr(_session_meta, 'enabled_toolsets', None) if _session_meta else None
|
||||
if _override:
|
||||
_toolsets = _override
|
||||
except Exception as _ts_err:
|
||||
print(f"[webui] WARNING: failed to read per-session toolsets for {session_id}: {_ts_err}", flush=True)
|
||||
|
||||
|
||||
@@ -131,3 +131,79 @@ def test_cron_history_clamps_offset_and_limit():
|
||||
"_handle_cron_history must clamp `limit` to a sane upper bound (500 chosen) "
|
||||
"to prevent DoS via `?limit=999999999`."
|
||||
)
|
||||
|
||||
|
||||
|
||||
# ── Critical Opus finding: enabled_toolsets actually applies ─────────────────
|
||||
|
||||
|
||||
def test_run_agent_streaming_uses_session_enabled_toolsets():
|
||||
"""The per-session toolset override (#493) was non-functional in PR #1402:
|
||||
`Session.load_metadata_only()` returns a Session INSTANCE, but the code
|
||||
called `.get('enabled_toolsets')` on it. AttributeError was swallowed by
|
||||
the surrounding `except Exception`, so the user's toolset chip silently
|
||||
no-op'd every time. Pin the source-level invariant so this exact regression
|
||||
can't return."""
|
||||
src = (REPO / "api" / "streaming.py").read_text(encoding="utf-8")
|
||||
|
||||
# The bug shape that must NOT come back: dict-style access on the result.
|
||||
# Negative-pattern guard (prevents revert).
|
||||
bad_pattern = "_session_meta.get('enabled_toolsets')"
|
||||
assert bad_pattern not in src, (
|
||||
f"streaming.py contains {bad_pattern!r} — Session.load_metadata_only() "
|
||||
f"returns a Session INSTANCE, not a dict, so .get() raises AttributeError. "
|
||||
f"The bare `except Exception:` swallows the failure silently and the "
|
||||
f"per-session toolset override is non-functional. Use getattr() instead. "
|
||||
f"(Opus pre-release advisor caught this in v0.50.257.)"
|
||||
)
|
||||
|
||||
bad_pattern2 = "_session_meta['enabled_toolsets']"
|
||||
assert bad_pattern2 not in src, (
|
||||
f"streaming.py contains {bad_pattern2!r} — same bug shape. "
|
||||
f"Session.load_metadata_only() returns an instance, not a dict."
|
||||
)
|
||||
|
||||
# Positive pattern: getattr() must be used.
|
||||
assert "getattr(_session_meta, 'enabled_toolsets'" in src, (
|
||||
"streaming.py must use getattr(_session_meta, 'enabled_toolsets', None) "
|
||||
"since load_metadata_only returns a Session instance."
|
||||
)
|
||||
|
||||
|
||||
def test_session_load_metadata_only_returns_instance_not_dict():
|
||||
"""End-to-end: Session.load_metadata_only must return a Session instance,
|
||||
not a dict. This is the contract that breaks PR #1402's toolset override
|
||||
if a future change converts it to a dict."""
|
||||
sys.path.insert(0, str(REPO))
|
||||
from api.models import Session
|
||||
import tempfile
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmpd:
|
||||
# Create a fake session file
|
||||
import json as _json
|
||||
sid = "test1234abcd"
|
||||
from api import models
|
||||
original = models.SESSION_DIR
|
||||
models.SESSION_DIR = Path(tmpd)
|
||||
try:
|
||||
session_file = Path(tmpd) / f"{sid}.json"
|
||||
session_file.write_text(_json.dumps({
|
||||
"session_id": sid,
|
||||
"title": "Test",
|
||||
"workspace": tmpd,
|
||||
"model": "test/model",
|
||||
"enabled_toolsets": ["bash", "file"],
|
||||
"messages": [],
|
||||
"tool_calls": [],
|
||||
}))
|
||||
result = Session.load_metadata_only(sid)
|
||||
finally:
|
||||
models.SESSION_DIR = original
|
||||
|
||||
# Result must be a Session instance — not None and not a dict.
|
||||
assert result is not None, "load_metadata_only returned None for valid session"
|
||||
assert isinstance(result, Session), (
|
||||
f"load_metadata_only must return Session instance, got {type(result)}"
|
||||
)
|
||||
# And the enabled_toolsets must be readable via getattr
|
||||
assert getattr(result, 'enabled_toolsets', None) == ["bash", "file"]
|
||||
|
||||
Reference in New Issue
Block a user