diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ff2d0cd..5e98b228 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/api/streaming.py b/api/streaming.py index 3eb74111..428292c0 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -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) diff --git a/tests/test_v050257_opus_followups.py b/tests/test_v050257_opus_followups.py index 2106690e..edb896cf 100644 --- a/tests/test_v050257_opus_followups.py +++ b/tests/test_v050257_opus_followups.py @@ -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"]