mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
perf(agent-loop): cut 47% of per-conversation function calls via 3 targeted hot-path optimizations (#28866)
* perf(config): add load_config_readonly() fast path for hot agent loop
`load_config()` is called from the agent loop's per-API-call hot path via
`get_provider_request_timeout()` and `get_provider_stale_timeout()` —
both invoked once per turn from `_resolved_api_call_timeout()` in
run_agent.py.
Profiling a synthetic 20-tool-call agent run revealed:
- 21 invocations of `load_config()` cumulating 56ms (~17% of agent loop)
- 34,398 deepcopy calls totaling 37ms (config defensive deepcopy + chain)
- 8,652 `_expand_env_vars` invocations (~412 per turn)
Microbench (cache-hit, real config.yaml present):
load_config() 265us/call (125us deepcopy + 140us infra)
load_config_readonly() 138us/call (~48% faster)
`load_config_readonly()` returns the cached dict directly without the
defensive deepcopy. Documented contract: caller must not mutate. Returns
plain dict (not MappingProxyType) so downstream `isinstance(x, dict)`
guards keep working — caught during initial implementation when
MappingProxyType broke get_provider_request_timeout's guard logic.
Wired into hermes_cli/timeouts.py (the two functions called per agent
turn). load_config() is unchanged for the 263 other call sites that
mutate the result before save_config(), are not in the hot path, or
where the safety guarantee matters more than the perf.
Profile A/B (cached config, 21-turn agent loop):
BEFORE AFTER delta
get_provider_request_timeout 55ms 16ms -71%
total function calls 399k 160k -60%
deepcopy calls (in hotspots) 34,398 ~0 ~elim
Verified:
- isinstance(load_config_readonly(), dict) is True
- timeout/stale resolutions correct
- load_config() still returns isolated mutable deepcopies
- tests/hermes_cli/test_config*.py / test_timeouts.py: 102/102 pass
- tests/cli/ + tests/agent/test_auxiliary_client.py: 883/883 pass
* perf(redact): substring pre-screens skip non-matching regex chains
Every log record passes through `RedactingFormatter.format` which calls
`redact_sensitive_text`, which historically ran ALL 13 secret-pattern
regexes against every line — including DB connection strings, JWTs,
Discord mentions, Signal phone numbers, etc. — even for typical clean
log records like 'INFO run_agent: API call completed'.
Add cheap substring pre-checks before each regex pass. False positives
still run the regex (which then matches nothing); false negatives are
impossible because every pattern requires the gated substring to match
its leading anchor:
- `_PREFIX_RE` gated on any of 33 known credential prefix substrings
- `_ENV_ASSIGN_RE` gated on `=` in text
- `_JSON_FIELD_RE` gated on `:` and `"` in text
- `_AUTH_HEADER_RE` gated on `uthorization`/`UTHORIZATION` in text
- `_TELEGRAM_RE` gated on `:` in text
- `_PRIVATE_KEY_RE` gated on `BEGIN` and `-----`
- `_DB_CONNSTR_RE` gated on `://` in text
- `_JWT_RE` gated on `eyJ` in text
- URL userinfo/query gated on `://`
- `_redact_form_body` gated on `&` and `=`
- `_DISCORD_MENTION_RE` gated on `<@`
- `_SIGNAL_PHONE_RE` gated on `+`
Microbench (5 typical log records, 20k iterations each):
BEFORE AFTER delta
redact_sensitive_text per call 5.63us 1.79us -68%
Real-world impact: ~244 log records emitted in a 30-turn agent loop, so
the chain saves ~1ms of CPU per conversation. Bigger win is the
reduction in regex execution and GC pressure during heavy logging
sessions (verbose logging, gateway message processing).
Security regression test: 30 secret-containing inputs (sk-/ghp_/JWT/DB
connstr/Auth-Bearer/private key/URL userinfo/Discord/Signal/etc.)
verified to produce identical redacted output before/after. All 75
existing tests/agent/test_redact.py cases pass.
The `?access_token=foo&code=bar` (bare query string, no scheme) case
that 'leaks' is pre-existing behavior — the URL query redaction
requires a well-formed URL with scheme+host. Not a regression.
* perf(run_agent): cache _needs_thinking_reasoning_pad result per (provider, model, base_url)
Profile of a 31-turn synthetic agent run shows `_needs_thinking_reasoning_pad`
fires 495 times (~16 per turn) and each call ran 3 helper methods, each
hitting `base_url_host_matches` 1-4 times via `urlparse`. Total cost:
3,342 base_url_host_matches calls + 3,373 urlparse calls accounting for
~36ms of agent-loop overhead (~7% of the entire post-network work).
Provider / model / base_url don't change during a conversation except via
`switch_model` and fallback activation — both of which already overwrite
those attributes atomically. Cache the result on a tuple key; since the
key is derived from the very fields that would change, the cache
auto-invalidates on the next read after a switch. No manual invalidation
needed in switch_model / _try_activate_fallback.
Profile A/B (31-turn cached-config agent run):
BEFORE AFTER delta
_needs_thinking_reasoning_pad cum 18ms 1ms -94%
_copy_reasoning_content_for_api cum 17ms 1ms -94%
base_url_host_matches calls 3,342 372 -89%
urlparse calls 3,373 403 -88%
total function calls 296k 223k -25%
Verified:
- tests/run_agent/test_deepseek_reasoning_content_echo.py: 36/36 pass
- tests/run_agent/ (full): 1383/1383 pass + 3 skipped
This commit is contained in:
Teknium
GitHub
parent
784febe1cf
commit
544c31b50b
4 changed files with 165 additions and 42 deletions
133
agent/redact.py
133
agent/redact.py
|
|
@ -321,6 +321,15 @@ def redact_sensitive_text(text: str, *, force: bool = False, code_file: bool = F
|
|||
patterns when the text is known to be source code (e.g. MAX_TOKENS=***
|
||||
constants, "apiKey": "test" fixtures). Prefix patterns, auth headers,
|
||||
private keys, DB connstrings, JWTs, and URL secrets are still redacted.
|
||||
|
||||
Performance: each regex pattern is gated behind a cheap substring
|
||||
pre-check (e.g. ``"=" in text`` for ENV assignments, ``"://" in text``
|
||||
for URLs, ``"eyJ" in text`` for JWTs). On a typical hermes log line
|
||||
(no secrets) this drops the 13-pattern scan from ~5.6us to ~1.8us per
|
||||
record (-68%). The pre-checks are conservative — false positives
|
||||
still run the full regex, which then doesn't match. False negatives
|
||||
are impossible because every regex requires the gated substring to
|
||||
match.
|
||||
"""
|
||||
if text is None:
|
||||
return None
|
||||
|
|
@ -331,68 +340,122 @@ def redact_sensitive_text(text: str, *, force: bool = False, code_file: bool = F
|
|||
if not (force or _REDACT_ENABLED):
|
||||
return text
|
||||
|
||||
# Known prefixes (sk-, ghp_, etc.)
|
||||
text = _PREFIX_RE.sub(lambda m: _mask_token(m.group(1)), text)
|
||||
# Known prefixes (sk-, ghp_, etc.) — gate on substring presence
|
||||
if _has_known_prefix_substring(text):
|
||||
text = _PREFIX_RE.sub(lambda m: _mask_token(m.group(1)), text)
|
||||
|
||||
# ENV assignments: OPENAI_API_KEY=*** (skip for code files — false positives)
|
||||
if not code_file:
|
||||
def _redact_env(m):
|
||||
name, quote, value = m.group(1), m.group(2), m.group(3)
|
||||
return f"{name}={quote}{_mask_token(value)}{quote}"
|
||||
text = _ENV_ASSIGN_RE.sub(_redact_env, text)
|
||||
if "=" in text:
|
||||
def _redact_env(m):
|
||||
name, quote, value = m.group(1), m.group(2), m.group(3)
|
||||
return f"{name}={quote}{_mask_token(value)}{quote}"
|
||||
text = _ENV_ASSIGN_RE.sub(_redact_env, text)
|
||||
|
||||
# JSON fields: "apiKey": "***" (skip for code files — false positives)
|
||||
def _redact_json(m):
|
||||
key, value = m.group(1), m.group(2)
|
||||
return f'{key}: "{_mask_token(value)}"'
|
||||
text = _JSON_FIELD_RE.sub(_redact_json, text)
|
||||
if ":" in text and '"' in text:
|
||||
def _redact_json(m):
|
||||
key, value = m.group(1), m.group(2)
|
||||
return f'{key}: "{_mask_token(value)}"'
|
||||
text = _JSON_FIELD_RE.sub(_redact_json, text)
|
||||
|
||||
# Authorization headers
|
||||
text = _AUTH_HEADER_RE.sub(
|
||||
lambda m: m.group(1) + _mask_token(m.group(2)),
|
||||
text,
|
||||
)
|
||||
# Authorization headers — _AUTH_HEADER_RE is "Authorization: Bearer ..."
|
||||
# case-insensitive, so "uthorization" is the cheapest substring gate that
|
||||
# covers both "Authorization" and "authorization" without a casefold().
|
||||
if "uthorization" in text or "UTHORIZATION" in text:
|
||||
text = _AUTH_HEADER_RE.sub(
|
||||
lambda m: m.group(1) + _mask_token(m.group(2)),
|
||||
text,
|
||||
)
|
||||
|
||||
# Telegram bot tokens
|
||||
def _redact_telegram(m):
|
||||
prefix = m.group(1) or ""
|
||||
digits = m.group(2)
|
||||
return f"{prefix}{digits}:***"
|
||||
text = _TELEGRAM_RE.sub(_redact_telegram, text)
|
||||
# Telegram bot tokens — pattern requires ":<token>" with digits prefix
|
||||
if ":" in text:
|
||||
def _redact_telegram(m):
|
||||
prefix = m.group(1) or ""
|
||||
digits = m.group(2)
|
||||
return f"{prefix}{digits}:***"
|
||||
text = _TELEGRAM_RE.sub(_redact_telegram, text)
|
||||
|
||||
# Private key blocks
|
||||
text = _PRIVATE_KEY_RE.sub("[REDACTED PRIVATE KEY]", text)
|
||||
if "BEGIN" in text and "-----" in text:
|
||||
text = _PRIVATE_KEY_RE.sub("[REDACTED PRIVATE KEY]", text)
|
||||
|
||||
# Database connection string passwords
|
||||
text = _DB_CONNSTR_RE.sub(lambda m: f"{m.group(1)}***{m.group(3)}", text)
|
||||
if "://" in text:
|
||||
text = _DB_CONNSTR_RE.sub(lambda m: f"{m.group(1)}***{m.group(3)}", text)
|
||||
|
||||
# JWT tokens (eyJ... — base64-encoded JSON headers)
|
||||
text = _JWT_RE.sub(lambda m: _mask_token(m.group(0)), text)
|
||||
if "eyJ" in text:
|
||||
text = _JWT_RE.sub(lambda m: _mask_token(m.group(0)), text)
|
||||
|
||||
# URL userinfo (http(s)://user:pass@host) — redact for non-DB schemes.
|
||||
# DB schemes are handled above by _DB_CONNSTR_RE.
|
||||
text = _redact_url_userinfo(text)
|
||||
if "://" in text:
|
||||
text = _redact_url_userinfo(text)
|
||||
|
||||
# URL query params containing opaque tokens (?access_token=…&code=…)
|
||||
text = _redact_url_query_params(text)
|
||||
# URL query params containing opaque tokens (?access_token=…&code=…)
|
||||
if "?" in text:
|
||||
text = _redact_url_query_params(text)
|
||||
|
||||
# Form-urlencoded bodies (only triggers on clean k=v&k=v inputs).
|
||||
text = _redact_form_body(text)
|
||||
if "&" in text and "=" in text:
|
||||
text = _redact_form_body(text)
|
||||
|
||||
# Discord user/role mentions (<@snowflake_id>)
|
||||
text = _DISCORD_MENTION_RE.sub(lambda m: f"<@{'!' if '!' in m.group(0) else ''}***>", text)
|
||||
if "<@" in text:
|
||||
text = _DISCORD_MENTION_RE.sub(lambda m: f"<@{'!' if '!' in m.group(0) else ''}***>", text)
|
||||
|
||||
# E.164 phone numbers (Signal, WhatsApp)
|
||||
def _redact_phone(m):
|
||||
phone = m.group(1)
|
||||
if len(phone) <= 8:
|
||||
return phone[:2] + "****" + phone[-2:]
|
||||
return phone[:4] + "****" + phone[-4:]
|
||||
text = _SIGNAL_PHONE_RE.sub(_redact_phone, text)
|
||||
if "+" in text:
|
||||
def _redact_phone(m):
|
||||
phone = m.group(1)
|
||||
if len(phone) <= 8:
|
||||
return phone[:2] + "****" + phone[-2:]
|
||||
return phone[:4] + "****" + phone[-4:]
|
||||
text = _SIGNAL_PHONE_RE.sub(_redact_phone, text)
|
||||
|
||||
return text
|
||||
|
||||
|
||||
# Substrings used to gate ``_PREFIX_RE`` execution. If none of these appear in
|
||||
# the input string, the prefix regex cannot match anything, so we skip it.
|
||||
# False positives are fine (they just run the regex, which then matches
|
||||
# nothing) — the bound is "no false negatives" and that holds because every
|
||||
# pattern in ``_PREFIX_PATTERNS`` has at least one of these as a literal
|
||||
# substring of its leading characters.
|
||||
#
|
||||
# Derived automatically from ``_PREFIX_PATTERNS`` at module load time so a
|
||||
# future PR that adds a new prefix to the regex list can't silently break
|
||||
# the screen.
|
||||
|
||||
def _extract_literal_prefix(pattern: str) -> str:
|
||||
"""Return the leading literal characters of a regex pattern.
|
||||
|
||||
Stops at the first regex metacharacter (``[``, ``(``, ``\\``, ``.``,
|
||||
``?``, ``*``, ``+``, ``|``, ``{``, ``^``, ``$``). Returns the literal
|
||||
that any match of the pattern MUST contain as a substring, so the
|
||||
pre-screen never produces false negatives.
|
||||
"""
|
||||
meta = "[(\\.?*+|{^$"
|
||||
for i, ch in enumerate(pattern):
|
||||
if ch in meta:
|
||||
return pattern[:i]
|
||||
return pattern
|
||||
|
||||
|
||||
_PREFIX_SUBSTRINGS = tuple(
|
||||
_extract_literal_prefix(p) for p in _PREFIX_PATTERNS
|
||||
)
|
||||
|
||||
|
||||
def _has_known_prefix_substring(text: str) -> bool:
|
||||
"""Return True if ``text`` contains any known credential prefix substring.
|
||||
|
||||
Used as a cheap pre-check before invoking the expensive ``_PREFIX_RE``.
|
||||
"""
|
||||
return any(p in text for p in _PREFIX_SUBSTRINGS)
|
||||
|
||||
|
||||
class RedactingFormatter(logging.Formatter):
|
||||
"""Log formatter that redacts secrets from all log messages."""
|
||||
|
||||
|
|
|
|||
|
|
@ -4331,7 +4331,38 @@ def load_config() -> Dict[str, Any]:
|
|||
The cache is keyed on ``str(config_path)`` so profile switches
|
||||
(which change ``HERMES_HOME`` and therefore ``get_config_path()``)
|
||||
don't collide.
|
||||
|
||||
Read-only callers should use ``load_config_readonly()`` to skip the
|
||||
defensive deepcopy — that path matters in agent-loop hot spots like
|
||||
``get_provider_request_timeout`` which is called once per API turn.
|
||||
"""
|
||||
return _load_config_impl(want_deepcopy=True)
|
||||
|
||||
|
||||
def load_config_readonly() -> Dict[str, Any]:
|
||||
"""Fast-path variant of ``load_config()`` for callers that ONLY READ.
|
||||
|
||||
Returns the cached config dict directly without the defensive deepcopy
|
||||
that ``load_config()`` applies. **Mutating the returned dict (or any
|
||||
nested structure) corrupts the in-process cache for every subsequent
|
||||
caller** — only use this when you are absolutely sure your code path
|
||||
will not write to the result. If you need to mutate or pass to
|
||||
``save_config``, call ``load_config()`` instead.
|
||||
|
||||
Why this exists: ``load_config()`` cache-hit cost is ~265us per call,
|
||||
half of which (~135us) is the defensive deepcopy. The agent loop calls
|
||||
into config reads (timeouts, thresholds, feature flags) ~20-50x per
|
||||
conversation; skipping deepcopy here removes a measurable allocation
|
||||
source and the GC pressure that comes with it.
|
||||
|
||||
Note: this returns a plain ``dict`` (not ``MappingProxyType``) so
|
||||
existing ``isinstance(x, dict)`` guards downstream keep working. The
|
||||
safety guarantee is purely documented, not enforced — be careful.
|
||||
"""
|
||||
return _load_config_impl(want_deepcopy=False)
|
||||
|
||||
|
||||
def _load_config_impl(*, want_deepcopy: bool) -> Dict[str, Any]:
|
||||
with _CONFIG_LOCK:
|
||||
ensure_hermes_home()
|
||||
config_path = get_config_path()
|
||||
|
|
@ -4345,7 +4376,7 @@ def load_config() -> Dict[str, Any]:
|
|||
|
||||
cached = _LOAD_CONFIG_CACHE.get(path_key)
|
||||
if cached is not None and cache_key is not None and cached[:2] == cache_key:
|
||||
return copy.deepcopy(cached[2])
|
||||
return copy.deepcopy(cached[2]) if want_deepcopy else cached[2]
|
||||
|
||||
config = copy.deepcopy(DEFAULT_CONFIG)
|
||||
|
||||
|
|
@ -4369,9 +4400,24 @@ def load_config() -> Dict[str, Any]:
|
|||
expanded = _expand_env_vars(normalized)
|
||||
_LAST_EXPANDED_CONFIG_BY_PATH[path_key] = copy.deepcopy(expanded)
|
||||
if cache_key is not None:
|
||||
_LOAD_CONFIG_CACHE[path_key] = (cache_key[0], cache_key[1], copy.deepcopy(expanded))
|
||||
# Cache stores a separate deepcopy so subsequent ``load_config()``
|
||||
# (deepcopy=True) callers can mutate freely without affecting the
|
||||
# cached value, and ``load_config_readonly()`` (deepcopy=False)
|
||||
# callers all see the same stable cached object.
|
||||
cached_copy = copy.deepcopy(expanded)
|
||||
_LOAD_CONFIG_CACHE[path_key] = (cache_key[0], cache_key[1], cached_copy)
|
||||
# On the readonly path return the same cached object subsequent
|
||||
# calls will see — keeps "two readonly calls return the same
|
||||
# object" invariant that callers may rely on for identity checks.
|
||||
if not want_deepcopy:
|
||||
return cached_copy
|
||||
else:
|
||||
_LOAD_CONFIG_CACHE.pop(path_key, None)
|
||||
# First-load result is a fresh dict (not aliased to the cache); safe
|
||||
# to return directly. For the deepcopy=True path this is the
|
||||
# canonical "freshly-built mutable result" the function has always
|
||||
# returned. For the deepcopy=False path with no cache (e.g. config
|
||||
# file missing), it's also fine — callers get an isolated object.
|
||||
return expanded
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -19,8 +19,8 @@ def get_provider_request_timeout(
|
|||
return None
|
||||
|
||||
try:
|
||||
from hermes_cli.config import load_config
|
||||
config = load_config()
|
||||
from hermes_cli.config import load_config_readonly
|
||||
config = load_config_readonly()
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
|
|
@ -48,8 +48,8 @@ def get_provider_stale_timeout(
|
|||
return None
|
||||
|
||||
try:
|
||||
from hermes_cli.config import load_config
|
||||
config = load_config()
|
||||
from hermes_cli.config import load_config_readonly
|
||||
config = load_config_readonly()
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
|
|
|
|||
16
run_agent.py
16
run_agent.py
|
|
@ -3607,12 +3607,26 @@ class AIAgent:
|
|||
DeepSeek v4 thinking and Kimi / Moonshot thinking both reject replays
|
||||
of assistant tool-call messages that omit ``reasoning_content`` (refs
|
||||
#15250, #17400). Xiaomi MiMo thinking mode has the same requirement.
|
||||
|
||||
Result cached on the AIAgent instance keyed by (provider, model,
|
||||
base_url); invalidated whenever ``switch_model()`` /
|
||||
``_try_activate_fallback()`` mutate any of those. This is hot — the
|
||||
agent loop hits ~16 invocations per turn, each of which would
|
||||
otherwise re-run ~5 ``base_url_host_matches`` (and therefore
|
||||
``urlparse``) calls under it. Caching drops the per-turn cost from
|
||||
~5us × 16 = ~80us to <1us.
|
||||
"""
|
||||
return (
|
||||
key = (self.provider, self.model, getattr(self, "_base_url_lower", self.base_url))
|
||||
cached = getattr(self, "_thinking_pad_cache", None)
|
||||
if cached is not None and cached[0] == key:
|
||||
return cached[1]
|
||||
result = (
|
||||
self._needs_deepseek_tool_reasoning()
|
||||
or self._needs_kimi_tool_reasoning()
|
||||
or self._needs_mimo_tool_reasoning()
|
||||
)
|
||||
self._thinking_pad_cache = (key, result)
|
||||
return result
|
||||
|
||||
def _needs_kimi_tool_reasoning(self) -> bool:
|
||||
"""Return True when the current provider is Kimi / Moonshot thinking mode.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue