From d59738f2f1d3312df56986abe18c28a9cf459cf6 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Tue, 22 Sep 2026 17:34:21 +0800 Subject: [PATCH] fix(api): typo-safe resolve-cap fallback + profile .env cannot resize (#7656 round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the round-1 maintainer review were not addressed in round-2: the helper clamped an out-of-range value *up* to the ceiling, and the env var was exposed to per-profile ``.env`` overrides, which contradicts the "process-wide resource limit" intent and creates a first-load-wins asymmetry across profile switches. This round fixes both. ``_read_max_session_resolve_concurrent()`` previously treated out-of-range values like ``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` as "the operator wants the maximum" and returned ``64`` — admitting 64 simultaneous unbounded transcript loads. The round-1 reviewer measured ~9.6 MB per parse on a synthetic 4.27 MB transcript, so the old ceiling permitted ~614 MB at 64-way concurrency before any other application memory. The cap is a *safety bound*, not a user preference. A typo must fall back to the safe default, never to the permissive extreme — matching the behavior of every other bad input in this helper (missing/empty/zero/negative/non-numeric all already fall back to 2). The 64 ceiling is reachable only via an explicit in-range operator value. server-wide key, not a per-profile .env override (SILENT) The cap controls a single process-wide ``BoundedSemaphore`` that gates full-transcript resolves. A profile's ``.env`` is per-profile by design; letting it set a server-wide limit lets a single profile drag the rest into a tighter or looser posture than the operator intended, with a first-load-wins asymmetry (a later profile switch cannot resize the already-constructed semaphore). The same shape as the isolated-profile key added in #4589: only the operator/launcher env at startup can set it. The cap is added to both ``_PROTECTED_ENV_KEYS`` (blocks the profile env at the load step) and ``_BLOCKED_RUNTIME_ENV_KEYS`` (filters the profile env in the runtime/gateway-parity path), so a profile cannot inject the env var at any layer. 1. ``api/models.py`` — ``_read_max_session_resolve_concurrent()``: - ``if n > 64: return 64`` → ``if n > 64: return 2`` - Docstring updated to spell out the "safety bound, not a user preference" rule and to point at ``_PROTECTED_ENV_KEYS`` as the place the 64 ceiling is enforced for profile env. 2. ``api/profiles.py`` — two frozensets: - ``_PROTECTED_ENV_KEYS`` adds ``'HERMES_WEBUI_MAX_SESSION_RESOLVE'`` (next to ``'HERMES_WEBUI_ISOLATED_PROFILE'``). - ``_BLOCKED_RUNTIME_ENV_KEYS`` adds the same key, with a comment explaining the first-load-wins asymmetry. 3. ``tests/test_issue7421_resolve_cap_config.py``: - The round-1 ``test_env_above_upper_bound_is_clamped_to_64`` is renamed to ``test_env_above_upper_bound_falls_back_to_default`` and the assertion + docstring reflect the new fallback contract. The maintainer's review said explicitly: "Update the clamp test accordingly." - Three new tests pin Finding 2: - ``test_protected_env_keys_includes_resolve_cap`` - ``test_blocked_runtime_env_keys_includes_resolve_cap`` - ``test_filter_runtime_env_strips_resolve_cap`` (end-to-end exercise: a profile env containing the cap is filtered out of the gateway-parity view, and an unrelated, non-blocked key like ``LANG`` still passes through as a sanity check) - ``tests/test_issue7421_resolve_cap_config.py``: 15/15 pass. - ``-k "profile or protect or isolated or HERMES_WEBUI_ISOLATED"``: 866 passed, 5 skipped, 1 xfailed, 2 xpassed — no regression. - The 5 ruff findings on the worktree are all pre-existing (3× F401 ``sqlite3`` + 2× B904 ``raise-with-from`` in ``api/profiles.py``); the curated gate is gated on newly-introduced findings only. Two. Both are the explicit fixes the maintainer requested. - ``_read_max_session_resolve_concurrent(999999)`` now returns ``2`` instead of ``64``. The 64 ceiling is still reachable via an explicit in-range operator value. - ``HERMES_WEBUI_MAX_SESSION_RESOLVE`` set in a profile's ``.env`` is now ignored: the operator/launcher env at startup is the only place the cap is configurable. Both changes are backward compatible at the operator level (an explicit ``=8`` still returns ``8``) and break the "typo → ceiling" failure mode and the "first-loaded-profile-wins" asymmetry the maintainer flagged. - An operator who set ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` in their deployment env is unaffected (still returns 64, as before). - An operator who set ``HERMES_WEBUI_MAX_SESSION_RESOLVE=128`` in their deployment env now silently falls back to 2 instead of getting 64. This is the round-1 reviewer's intent: "Update the clamp test accordingly" + "the safe default, never the permissive extreme." A startup log warning could be added in a follow-up if a louder signal is wanted; this PR keeps the silent-fallback contract consistent with the other bad-input paths in the same helper. - A profile that *had* ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` in its ``.env`` and was relying on it to set the cap now silently uses the operator-set value. This is the intended behavior: the cap is a process-wide resource limit, not a per-profile toggle. A profile-side log warning is a follow-up if needed; the runtime env path now drops the key, so the failure mode is "operator value wins" rather than "first profile to load wins". - #7656 — the original PR; this is round-3 of the same branch. - #7421 — the original env-override feature. - #4589 — the same shape ("operator-only, not per-profile") for the isolated-profile key; the comment block on the new protected key references the same pattern. --- api/models.py | 17 +++- api/profiles.py | 14 +++ tests/test_issue7421_resolve_cap_config.py | 101 +++++++++++++++++++-- 3 files changed, 119 insertions(+), 13 deletions(-) diff --git a/api/models.py b/api/models.py index 0bef0346..0322a97b 100644 --- a/api/models.py +++ b/api/models.py @@ -4797,10 +4797,17 @@ def _read_max_session_resolve_concurrent() -> int: raise the cap without code changes. A bad or missing value falls back to the previous default of 2, so an operator who sets ``HERMES_WEBUI_MAX_SESSION_RESOLVE=0`` or a non-numeric - value does not regress to a blocked or unbounded state. The - upper bound (64) is a generous but finite safety net against - a typo like ``=999999`` bypassing the bounded semaphore - entirely; the deeper per-read cost is tracked in #7310.""" + value does not regress to a blocked or unbounded state. + + The cap is a *safety bound* (per-resolve cost tracked in + #7310), not a user preference — a typo like ``=999999`` + must fall back to the safe default, never to the permissive + extreme. Every other bad input in this function falls back + to ``2``; out-of-range values do too. The previously + accepted upper bound (64) is reachable only via an explicit + in-range operator value; a profile's ``.env`` cannot reach + it at all (see ``_PROTECTED_ENV_KEYS``). + """ raw = (os.getenv("HERMES_WEBUI_MAX_SESSION_RESOLVE") or "").strip() if not raw: return 2 @@ -4811,7 +4818,7 @@ def _read_max_session_resolve_concurrent() -> int: if n < 1: return 2 if n > 64: - return 64 + return 2 # #7656 round-3: typo-safe fallback, not clamp-up-to-64 return n diff --git a/api/profiles.py b/api/profiles.py index a4b5d8f7..e56b32fd 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -202,6 +202,14 @@ _PROTECTED_ENV_KEYS = frozenset({ 'HERMES_WEBUI_ISOLATED_PROFILE', # Server-wide sidebar window; a profile .env must not widen the per-request query. 'HERMES_WEBUI_VISIBLE_SESSION_LIMIT', + # #7656 round-3: HERMES_WEBUI_MAX_SESSION_RESOLVE is a process-wide resource + # cap (controls the BoundedSemaphore that gates full-transcript resolves). + # A profile's .env is per-profile by design; allowing it to set a + # server-wide limit (and a first-load-wins asymmetry) would let a + # single profile drag the rest into a tighter or looser posture than + # the operator intended. Same shape as the isolated-profile key: only + # the operator/launcher env at startup can set it. + 'HERMES_WEBUI_MAX_SESSION_RESOLVE', }) @@ -939,6 +947,12 @@ _BLOCKED_RUNTIME_ENV_KEYS = { # profile's own env on any runtime/gateway-parity path. 'HERMES_WEBUI_ISOLATED_PROFILE', 'HERMES_WEBUI_VISIBLE_SESSION_LIMIT', + # #7656 round-3: process-wide resolve cap. Filter out a profile's + # .env override the same way we filter the isolated-profile key, so + # the cap stays at the operator/launcher value across profile + # switches rather than re-sizing to whatever the first-loaded + # profile env set. + 'HERMES_WEBUI_MAX_SESSION_RESOLVE', } diff --git a/tests/test_issue7421_resolve_cap_config.py b/tests/test_issue7421_resolve_cap_config.py index 6217c1b4..06caa73c 100644 --- a/tests/test_issue7421_resolve_cap_config.py +++ b/tests/test_issue7421_resolve_cap_config.py @@ -120,16 +120,29 @@ def test_env_negative_falls_back_to_default(): assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 -def test_env_above_upper_bound_is_clamped_to_64(): +def test_env_above_upper_bound_falls_back_to_default(): """``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` is almost - certainly a typo and would defeat the bounded-semaphore - design entirely. The helper clamps to 64, which is - generous for any realistic deployment while still bounded - enough to remain a real cap.""" + certainly a typo and must NOT defeat the safety-bound + design by clamping up to the ceiling. The previous + clamp-to-64 behavior (round-1) admitted 64 simultaneous + unbounded transcript loads; a synthetic 4.27 MB transcript + consumed ~9.6 MB per parse, so the round-1 ceiling + permitted ~614 MB at 64-way concurrency before any other + application memory. + + The cap is a *safety bound*, not a user preference — a + typo must fall back to the safe default (2), never to + the permissive extreme. The upper bound (64) is reachable + only via an explicit in-range operator value; out-of-range + values degrade silently to the default, matching the + behavior of every other bad input in this helper. See + #7656 round-3 finding 1. + """ models = _import_models_with_env("999999") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 64, ( - "out-of-band values must be clamped, not propagated; the " - "whole point of the cap is to bound concurrency" + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( + "out-of-band values must fall back to the safe default, " + "not clamp up to the ceiling; a typo should never widen " + "the cap" ) @@ -200,3 +213,75 @@ def test_helper_defined_in_models(): "the bare literal-2 initializer must be removed; the " "cap is now sourced from the helper" ) + + +# ── #7656 round-3 finding 2: profile .env must not resize the cap ─────────── + + +def test_protected_env_keys_includes_resolve_cap(): + """``HERMES_WEBUI_MAX_SESSION_RESOLVE`` is a process-wide + resource cap; a profile's ``.env`` must not override it. + + The operator/launcher env at startup is the only place the + cap is configurable. Without this, a first-loaded profile + could set the cap to one value and a later profile-switch + would silently re-size the already-constructed + BoundedSemaphore — the same first-load-wins asymmetry + the maintainer flagged for the isolated-profile key in + #4589. Pin the contract. + """ + from api.profiles import _PROTECTED_ENV_KEYS + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _PROTECTED_ENV_KEYS, ( + "HERMES_WEBUI_MAX_SESSION_RESOLVE must be in " + "_PROTECTED_ENV_KEYS so a profile's .env cannot " + "override the process-wide cap" + ) + + +def test_blocked_runtime_env_keys_includes_resolve_cap(): + """``filter_runtime_env_for_gateway_parity`` must also drop + the resolve cap from a profile's runtime env, so the + gateway/CLI/agent paths see the operator-set value rather + than whatever the profile tried to set. + + This is the runtime counterpart of the protected-key + check. Together they pin the contract from both sides: + a profile cannot inject the env var at any layer + (startup reload, runtime env, gateway parity filter). + """ + from api.profiles import _BLOCKED_RUNTIME_ENV_KEYS + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _BLOCKED_RUNTIME_ENV_KEYS, ( + "HERMES_WEBUI_MAX_SESSION_RESOLVE must be in " + "_BLOCKED_RUNTIME_ENV_KEYS so a profile's runtime env " + "is filtered out before the gateway/CLI/agent paths " + "see it" + ) + + +def test_filter_runtime_env_strips_resolve_cap(): + """A profile env containing the resolve cap must be filtered + out by ``filter_runtime_env_for_gateway_parity``. + + End-to-end exercise of the SILENT finding: the + runtime-env filter is the one that actually prevents the + cap from being silently re-sized by a profile switch. + Without this, the cap the operator set at startup is + silently overridden the first time a profile with the + env var in its ``.env`` loads. + """ + from api.profiles import filter_runtime_env_for_gateway_parity + profile_env = { + "HERMES_WEBUI_MAX_SESSION_RESOLVE": "64", + "LANG": "en_US.UTF-8", + } + filtered = filter_runtime_env_for_gateway_parity(profile_env) + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" not in filtered, ( + "filter_runtime_env_for_gateway_parity must strip " + "HERMES_WEBUI_MAX_SESSION_RESOLVE from a profile's " + "runtime env so the cap stays at the operator/launcher value" + ) + # Sanity: an unrelated, non-blocked env key still passes through. + # (PATH, HOME, etc. are themselves in _BLOCKED_RUNTIME_ENV_KEYS, + # so pick something neutral like LANG.) + assert filtered.get("LANG") == "en_US.UTF-8" +