From b7ac5a8b884e0cdf86930ea4271580554acec704 Mon Sep 17 00:00:00 2001 From: Frank Song Date: Wed, 13 May 2026 12:51:14 +0800 Subject: [PATCH 1/2] Trim session tail response overhead --- CHANGELOG.md | 3 +- api/helpers.py | 77 +++++++++++++++++++++++++ api/routes.py | 30 +++++++++- tests/test_security_redaction.py | 30 ++++++++++ tests/test_session_tail_payload.py | 92 ++++++++++++++++++++++++++++++ 5 files changed, 230 insertions(+), 2 deletions(-) create mode 100644 tests/test_session_tail_payload.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a07d3e6..1097bebf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- Large session responses now skip the full credential-redaction pass for strings that contain no credential markers, and tail-window session loads no longer send historical `tool_calls` when the returned messages already carry per-message tool metadata. Both changes preserve the existing redaction and tool-card behavior while reducing session-switch latency on long transcripts. + - **PR #2179** (self-built, closes #2177) — NVIDIA NIM no longer 404s when WebUI is configured with `provider: nvidia` and a `nvidia/` id. `resolve_model_provider()` in `api/config.py` had the `_PORTAL_PROVIDERS` guard (Nous, OpenCode-Zen, OpenCode-Go, NVIDIA NIM — providers whose APIs require the full namespaced `provider/model` wire format) sitting **after** the `prefix == config_provider` strip branch. For `model_id="nvidia/nemotron-3-super-120b-a12b"` + `config_provider="nvidia"`, the strip branch fired first and returned the bare `nemotron-...` to NIM, which then 404'd because NIM requires the full path. Same bug class as #854 / #894 (Nous portal). The guard was originally added with NVIDIA in mind but the structural ordering was wrong. Fix is a pure reorder of two `if` blocks — hoist `_PORTAL_PROVIDERS` ahead of the strip — so all portal providers always preserve the full `provider/model` id regardless of whether the prefix happens to equal the provider name. Also closes a latent symmetric bug for the Nous case if a `nous/` id ever entered the catalog. Cross-tool trace against hermes-agent's `hermes_cli/models.py` (line 59, 177, 237-239) and `agent/model_metadata.py:68` confirms the agent CLI sends `nvidia/nemotron-...` verbatim — both tools now agree on the wire format. 118-line regression suite covering the reported case, cross-namespace `qwen/` and `meta/` ids, every static nvidia model in `_PROVIDER_MODELS`, the latent `nous/` ordering pin, and a non-portal-provider regression pin for the anthropic strip behavior. nesquena APPROVED with 200-line end-to-end trace + 12-shape behavioural harness + cross-tool wire-format verification. Reported on Discord by @vishnu in #report-bugs. ## [v0.51.53] — 2026-05-13 — Release AC (stage-346 — 10-PR contributor batch — stale-stream guard extension + guarded worktree remove + CSP report collector + perf + i18n + ctl fix + defense-in-depth) @@ -41,7 +43,6 @@ ## [v0.51.52] — 2026-05-12 — Release AB (stage-345 — 2-PR low-risk batch — stream-ownership guard + Refresh-usage button on provider quota card) ### Fixed - - **PR #2136** by @LumenYoung — Stale stream writebacks no longer poison the active session transcript. `cancel_stream()` intentionally clears `active_stream_id` early so the UI can accept a follow-up turn while an old worker is unwinding — but the old worker could still return later from `run_conversation()` and persist its stale result over the newer transcript, causing visible transcript / turn journal / `state.db` to disagree (especially around cancel+retry on compressed continuations). Adds a single-line ownership check `_stream_writeback_is_current(session, stream_id)` (token equality against `session.active_stream_id`) and short-circuits both finalize paths: the success path in `_run_agent_streaming` and the cancel-handler path in `cancel_stream()`. When the stream no longer owns the writeback, both paths log `Skipping stale stream/cancel writeback` and return cleanly without persisting. 89-line regression suite in `tests/test_stale_stream_writeback.py`; companion updates to `tests/test_issue1361_cancel_data_loss.py` and `tests/test_sprint42.py` for the new return-without-persist behavior. ### Added diff --git a/api/helpers.py b/api/helpers.py index 7cf010c7..664dc104 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -179,6 +179,81 @@ def _build_redact_fn(): _redact_fn_cached = _build_redact_fn() +_SENSITIVE_CASE_MARKERS = ( + "sk-", + "ghp_", + "github_pat_", + "gho_", + "ghu_", + "ghs_", + "ghr_", + "AKIA", + "xoxb-", + "xoxa-", + "xoxp-", + "xoxr-", + "xoxs-", + "hf_", + "SG.", + "AIza", + "mem0_", + "brv_", + "eyJ", + "-----BEGIN", +) +_SENSITIVE_LOWER_MARKERS = ( + "authorization: bearer ", + "private key", + "postgres://", + "postgresql://", + "mysql://", + "mongodb://", + "redis://", + "amqp://", + "access_token", + "refresh_token", + "id_token", + "api_key", + "apikey", + "client_secret", + "auth_token", + "raw_secret", + "secret_input", + "key_material", + "x-amz-signature", + "token=", + "secret=", + "password=", + "authorization=", + "key=", + '"token"', + '"secret"', + '"password"', + '"bearer"', +) +_SENSITIVE_TELEGRAM_MARKER_RE = _re.compile(r"(?:bot)?\d{8,}:[-A-Za-z0-9_]{30,}") +_SENSITIVE_DISCORD_MARKER_RE = _re.compile(r"<@!?\d{17,20}>") +_SENSITIVE_PHONE_MARKER_RE = _re.compile(r"(? bool: + """Cheap prefilter before the full agent+fallback redaction pass.""" + if not isinstance(text, str) or not text: + return False + if any(marker in text for marker in _SENSITIVE_CASE_MARKERS): + return True + lower = text.lower() + if any(marker in lower for marker in _SENSITIVE_LOWER_MARKERS): + return True + if ":" in text and _SENSITIVE_TELEGRAM_MARKER_RE.search(text): + return True + if "<@" in text and _SENSITIVE_DISCORD_MARKER_RE.search(text): + return True + if "+" in text and _SENSITIVE_PHONE_MARKER_RE.search(text): + return True + return False + + def _redact_text(text: str, *, _enabled: bool | None = None) -> str: """Redact sensitive text from API responses. Respects api_redact_enabled setting. @@ -194,6 +269,8 @@ def _redact_text(text: str, *, _enabled: bool | None = None) -> str: _enabled = bool(load_settings().get("api_redact_enabled", True)) if not _enabled: return text + if not _might_contain_sensitive_text(text): + return text return _redact_fn_cached(text) diff --git a/api/routes.py b/api/routes.py index df290280..cfa78821 100644 --- a/api/routes.py +++ b/api/routes.py @@ -1666,6 +1666,24 @@ def _is_messaging_session_record(session) -> bool: return _is_known_messaging_source(raw) +def _messages_include_tool_metadata(messages) -> bool: + """Return true when returned messages can reconstruct their own tool cards.""" + if not isinstance(messages, list): + return False + for msg in messages: + if not isinstance(msg, dict) or msg.get("role") != "assistant": + continue + if isinstance(msg.get("tool_calls"), list) and msg.get("tool_calls"): + return True + content = msg.get("content") + if isinstance(content, list) and any( + isinstance(part, dict) and part.get("type") == "tool_use" + for part in content + ): + return True + return False + + def _is_messaging_session_id(sid: str) -> bool: """Detect messaging-backed sessions from WebUI metadata or Agent rows.""" try: @@ -3362,9 +3380,19 @@ def handle_get(handler, parsed) -> bool: _persisted_cl = _fb_cl except Exception: pass + _session_tool_calls = getattr(s, "tool_calls", []) if load_messages else [] + if ( + load_messages + and msg_limit is not None + and _messages_include_tool_metadata(_truncated_msgs) + ): + # The browser ignores session-level tool_calls when the returned + # messages already carry per-message tool metadata. Avoid sending + # the full historical list with a small tail window. + _session_tool_calls = [] raw = s.compact() | { "messages": _truncated_msgs, - "tool_calls": getattr(s, "tool_calls", []) if load_messages else [], + "tool_calls": _session_tool_calls, "active_stream_id": getattr(s, "active_stream_id", None), "pending_user_message": getattr(s, "pending_user_message", None), "pending_attachments": getattr(s, "pending_attachments", []) if load_messages else [], diff --git a/tests/test_security_redaction.py b/tests/test_security_redaction.py index 08ac49d6..81d63af7 100644 --- a/tests/test_security_redaction.py +++ b/tests/test_security_redaction.py @@ -107,6 +107,36 @@ def test_redact_value_list(): assert result[1]["content"] == "safe text" +def test_redact_text_skips_full_redactor_for_safe_text(monkeypatch): + """Large ordinary transcript text should not pay the full redactor pass.""" + import api.helpers as helpers + + calls = [] + monkeypatch.setattr(helpers, "_redact_fn_cached", lambda text: calls.append(text) or text) + + safe_text = "ordinary session transcript without credential markers\n" * 500 + assert helpers._redact_text(safe_text, _enabled=True) == safe_text + assert calls == [] + + +def test_redact_text_still_runs_full_redactor_for_sensitive_markers(monkeypatch): + """The cheap prefilter must preserve the hard redaction boundary.""" + import api.helpers as helpers + + calls = [] + + def fake_redactor(text): + calls.append(text) + return text.replace(_FAKE_SK_KEY, "sk-Tes...cdef") + + monkeypatch.setattr(helpers, "_redact_fn_cached", fake_redactor) + + result = helpers._redact_text(f"token={_FAKE_SK_KEY}", _enabled=True) + + assert _FAKE_SK_KEY not in result + assert calls == [f"token={_FAKE_SK_KEY}"] + + def test_redact_value_works_with_legacy_agent_redact_signature(monkeypatch): """_redact_text must tolerate older redact_sensitive_text(text) signatures.""" fake_agent = types.ModuleType("agent") diff --git a/tests/test_session_tail_payload.py b/tests/test_session_tail_payload.py new file mode 100644 index 00000000..236551e6 --- /dev/null +++ b/tests/test_session_tail_payload.py @@ -0,0 +1,92 @@ +from types import SimpleNamespace +from unittest.mock import patch +from urllib.parse import urlparse + + +class _FakeSession: + def __init__(self, messages): + self.session_id = "tail_payload_001" + self.title = "Tail payload" + self.workspace = "/tmp" + self.model = "gpt-test" + self.model_provider = None + self.messages = messages + self.tool_calls = [ + {"name": "old-tool", "snippet": "historical snippet", "assistant_msg_idx": 0} + ] + self.input_tokens = 0 + self.output_tokens = 0 + self.estimated_cost = 0 + self.context_length = 1 + self.threshold_tokens = 0 + self.last_prompt_tokens = 0 + self.active_stream_id = None + self.pending_user_message = None + self.pending_attachments = [] + self.pending_started_at = None + self.composer_draft = {} + + def compact(self): + return { + "session_id": self.session_id, + "title": self.title, + "workspace": self.workspace, + "model": self.model, + "model_provider": self.model_provider, + "message_count": len(self.messages), + "context_length": self.context_length, + "threshold_tokens": self.threshold_tokens, + "last_prompt_tokens": self.last_prompt_tokens, + "active_stream_id": self.active_stream_id, + "pending_user_message": self.pending_user_message, + "composer_draft": self.composer_draft, + } + + +def _invoke(session): + import api.routes as routes + + captured = {} + + def fake_j(_handler, data, status=200, extra_headers=None): + captured["data"] = data + captured["status"] = status + return data + + parsed = urlparse("/api/session?session_id=tail_payload_001&messages=1&resolve_model=0&msg_limit=1") + with patch("api.routes.get_session", return_value=session), \ + patch("api.routes._clear_stale_stream_state", return_value=False), \ + patch("api.routes._lookup_cli_session_metadata", return_value={}), \ + patch("api.routes.redact_session_data", side_effect=lambda raw: raw), \ + patch("api.routes.j", side_effect=fake_j): + routes.handle_get(SimpleNamespace(), parsed) + return captured["data"]["session"] + + +def test_tail_window_omits_historical_tool_calls_when_messages_have_tool_metadata(): + session = _FakeSession([ + {"role": "user", "content": "older"}, + { + "role": "assistant", + "content": "visible", + "tool_calls": [{"id": "call_1", "function": {"name": "tool", "arguments": "{}"}}], + }, + ]) + + payload = _invoke(session) + + assert payload["messages"] == [session.messages[-1]] + assert payload["tool_calls"] == [] + assert payload["_messages_truncated"] is True + + +def test_tail_window_keeps_session_tool_calls_for_legacy_messages_without_metadata(): + session = _FakeSession([ + {"role": "user", "content": "older"}, + {"role": "assistant", "content": "visible legacy message"}, + ]) + + payload = _invoke(session) + + assert payload["messages"] == [session.messages[-1]] + assert payload["tool_calls"] == session.tool_calls From da73c00f06577159ee820d8ab6acad5c233a7385 Mon Sep 17 00:00:00 2001 From: Frank Song Date: Wed, 13 May 2026 18:57:56 +0800 Subject: [PATCH 2/2] Harden session tail redaction prefilter --- CHANGELOG.md | 2 +- api/helpers.py | 58 ++++++++++++++++++++++++++++---- tests/test_security_redaction.py | 51 ++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1097bebf..0988dc30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Large session responses now skip the full credential-redaction pass for strings that contain no credential markers, and tail-window session loads no longer send historical `tool_calls` when the returned messages already carry per-message tool metadata. Both changes preserve the existing redaction and tool-card behavior while reducing session-switch latency on long transcripts. +- Large session responses now skip the full credential-redaction pass for strings that contain no credential markers, and tail-window session loads no longer send historical `tool_calls` when the returned messages already carry per-message tool metadata. The redaction prefilter and local fallback now mirror the full prefixed-token set handled by the agent redactor, so the faster path keeps the existing hard API redaction boundary while reducing session-switch latency on long transcripts. - **PR #2179** (self-built, closes #2177) — NVIDIA NIM no longer 404s when WebUI is configured with `provider: nvidia` and a `nvidia/` id. `resolve_model_provider()` in `api/config.py` had the `_PORTAL_PROVIDERS` guard (Nous, OpenCode-Zen, OpenCode-Go, NVIDIA NIM — providers whose APIs require the full namespaced `provider/model` wire format) sitting **after** the `prefix == config_provider` strip branch. For `model_id="nvidia/nemotron-3-super-120b-a12b"` + `config_provider="nvidia"`, the strip branch fired first and returned the bare `nemotron-...` to NIM, which then 404'd because NIM requires the full path. Same bug class as #854 / #894 (Nous portal). The guard was originally added with NVIDIA in mind but the structural ordering was wrong. Fix is a pure reorder of two `if` blocks — hoist `_PORTAL_PROVIDERS` ahead of the strip — so all portal providers always preserve the full `provider/model` id regardless of whether the prefix happens to equal the provider name. Also closes a latent symmetric bug for the Nous case if a `nous/` id ever entered the catalog. Cross-tool trace against hermes-agent's `hermes_cli/models.py` (line 59, 177, 237-239) and `agent/model_metadata.py:68` confirms the agent CLI sends `nvidia/nemotron-...` verbatim — both tools now agree on the wire format. 118-line regression suite covering the reported case, cross-namespace `qwen/` and `meta/` ids, every static nvidia model in `_PROVIDER_MODELS`, the latent `nous/` ordering pin, and a non-portal-provider regression pin for the anthropic strip behavior. nesquena APPROVED with 200-line end-to-end trace + 12-shape behavioural harness + cross-tool wire-format verification. Reported on Discord by @vishnu in #report-bugs. diff --git a/api/helpers.py b/api/helpers.py index 664dc104..28873e69 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -112,7 +112,8 @@ MAX_BODY_BYTES = 20 * 1024 * 1024 # 20MB limit for non-upload POST bodies def _build_redact_fn(): """Return a redactor backed by hermes-agent plus local fallback patterns.""" - # Minimal fallback covering the most common credential prefixes. + # Fallback mirrors the agent's known credential prefixes so WebUI API + # responses remain a hard redaction boundary even without hermes-agent. # Keep this active even when hermes-agent is importable so API responses do # not regress if the agent redactor misses a token shape. _CRED_RE = _re.compile( @@ -124,10 +125,34 @@ def _build_redact_fn(): r"|ghu_[A-Za-z0-9]{10,}" # GitHub user-to-server token r"|ghs_[A-Za-z0-9]{10,}" # GitHub server-to-server token r"|ghr_[A-Za-z0-9]{10,}" # GitHub refresh token + r"|xox[baprs]-[A-Za-z0-9-]{10,}" # Slack tokens + r"|AIza[A-Za-z0-9_-]{30,}" # Google API keys + r"|pplx-[A-Za-z0-9]{10,}" # Perplexity + r"|fal_[A-Za-z0-9_-]{10,}" # Fal.ai + r"|fc-[A-Za-z0-9]{10,}" # Firecrawl + r"|bb_live_[A-Za-z0-9_-]{10,}" # BrowserBase + r"|gAAAA[A-Za-z0-9_=-]{20,}" # Codex encrypted tokens r"|AKIA[A-Z0-9]{16}" # AWS Access Key ID - r"|xox[baprs]-[A-Za-z0-9-]{10,}" # Slack tokens - r"|hf_[A-Za-z0-9]{10,}" # HuggingFace token - r"|SG\.[A-Za-z0-9_-]{10,}" # SendGrid API key + r"|sk_live_[A-Za-z0-9]{10,}" # Stripe secret key (live) + r"|sk_test_[A-Za-z0-9]{10,}" # Stripe secret key (test) + r"|rk_live_[A-Za-z0-9]{10,}" # Stripe restricted key + r"|SG\.[A-Za-z0-9_-]{10,}" # SendGrid API key + r"|hf_[A-Za-z0-9]{10,}" # HuggingFace token + r"|r8_[A-Za-z0-9]{10,}" # Replicate API token + r"|npm_[A-Za-z0-9]{10,}" # npm access token + r"|pypi-[A-Za-z0-9_-]{10,}" # PyPI API token + r"|dop_v1_[A-Za-z0-9]{10,}" # DigitalOcean PAT + r"|doo_v1_[A-Za-z0-9]{10,}" # DigitalOcean OAuth + r"|am_[A-Za-z0-9_-]{10,}" # AgentMail API key + r"|sk_[A-Za-z0-9_]{10,}" # ElevenLabs TTS key + r"|tvly-[A-Za-z0-9]{10,}" # Tavily search API key + r"|exa_[A-Za-z0-9]{10,}" # Exa search API key + r"|gsk_[A-Za-z0-9]{10,}" # Groq Cloud API key + r"|syt_[A-Za-z0-9]{10,}" # Matrix access token + r"|retaindb_[A-Za-z0-9]{10,}" # RetainDB API key + r"|hsk-[A-Za-z0-9]{10,}" # Hindsight API key + r"|mem0_[A-Za-z0-9]{10,}" # Mem0 Platform API key + r"|brv_[A-Za-z0-9]{10,}" # ByteRover API key r")(?![A-Za-z0-9_-])" ) _AUTH_HDR_RE = _re.compile(r"(Authorization:\s*Bearer\s+)(\S+)", _re.IGNORECASE) @@ -193,9 +218,30 @@ _SENSITIVE_CASE_MARKERS = ( "xoxp-", "xoxr-", "xoxs-", - "hf_", - "SG.", "AIza", + "pplx-", + "fal_", + "fc-", + "bb_live_", + "gAAAA", + "sk_live_", + "sk_test_", + "rk_live_", + "SG.", + "hf_", + "r8_", + "npm_", + "pypi-", + "dop_v1_", + "doo_v1_", + "am_", + "sk_", + "tvly-", + "exa_", + "gsk_", + "syt_", + "retaindb_", + "hsk-", "mem0_", "brv_", "eyJ", diff --git a/tests/test_security_redaction.py b/tests/test_security_redaction.py index 81d63af7..51a6033a 100644 --- a/tests/test_security_redaction.py +++ b/tests/test_security_redaction.py @@ -137,6 +137,57 @@ def test_redact_text_still_runs_full_redactor_for_sensitive_markers(monkeypatch) assert calls == [f"token={_FAKE_SK_KEY}"] +@pytest.mark.parametrize("prefix,suffix", [ + ("sk-", "TestCredential1234567890"), + ("ghp_", "TestCredential1234567890"), + ("github_pat_", "TestCredential_1234567890"), + ("gho_", "TestCredential1234567890"), + ("ghu_", "TestCredential1234567890"), + ("ghs_", "TestCredential1234567890"), + ("ghr_", "TestCredential1234567890"), + ("xoxb-", "TestCredential1234567890"), + ("xoxa-", "TestCredential1234567890"), + ("xoxp-", "TestCredential1234567890"), + ("xoxr-", "TestCredential1234567890"), + ("xoxs-", "TestCredential1234567890"), + ("AIza", "TestCredential1234567890abcdefghi"), + ("pplx-", "TestCredential1234567890"), + ("fal_", "TestCredential1234567890"), + ("fc-", "TestCredential1234567890"), + ("bb_live_", "TestCredential1234567890"), + ("gAAAA", "TestCredential1234567890abcd"), + ("AKIA", "TESTCREDENTIAL12"), + ("sk_" + "live_", "TestCredential1234567890"), + ("sk_" + "test_", "TestCredential1234567890"), + ("rk_" + "live_", "TestCredential1234567890"), + ("SG.", "TestCredential1234567890"), + ("hf_", "TestCredential1234567890"), + ("r8_", "TestCredential1234567890"), + ("npm_", "TestCredential1234567890"), + ("pypi-", "TestCredential1234567890"), + ("dop_v1_", "TestCredential1234567890"), + ("doo_v1_", "TestCredential1234567890"), + ("am_", "TestCredential1234567890"), + ("sk_", "TestCredential1234567890"), + ("tvly-", "TestCredential1234567890"), + ("exa_", "TestCredential1234567890"), + ("gsk_", "TestCredential1234567890"), + ("syt_", "TestCredential1234567890"), + ("retaindb_", "TestCredential1234567890"), + ("hsk-", "TestCredential1234567890"), + ("mem0_", "TestCredential1234567890"), + ("brv_", "TestCredential1234567890"), +]) +def test_redact_text_prefilter_covers_known_prefixed_credentials(prefix, suffix): + """Every known prefix must still reach the hard redactor.""" + import api.helpers as helpers + + token = prefix + suffix + result = helpers._redact_text(f"credential={token}", _enabled=True) + + assert token not in result + + def test_redact_value_works_with_legacy_agent_redact_signature(monkeypatch): """_redact_text must tolerate older redact_sensitive_text(text) signatures.""" fake_agent = types.ModuleType("agent")