The suppression predicate _transcript_already_advanced_past_pending()
required a run-journal terminal 'completed' state, but its caller in
_apply_core_sync_or_error_marker already returns early on that same
condition. The predicate branch was therefore unreachable dead code: it
could never suppress anything, and stale-cancel recovery could still
append a duplicate recovered user row, a journal clone and an
interruption marker for a turn that actually finished.
Derive the completion evidence from the crash-safe turn journal instead:
_turn_journal_records_completion() reports True when the exact-stream
'completed' event is the latest terminal state for the pending stream id.
The run journal's terminal write is not durable evidence for this path
(it is consumed by the caller's early return), so the turn journal is the
only still-observable source. A cancelled/crashed turn (interrupted)
keeps recovery active, and completion for another stream of the same
session never settles the pending one.
Also record the same completion event on the Gateway success writeback
path, so Gateway-backed runs have the equivalent completion persistence
instead of leaving the run journal's lost terminal write unrecoverable.
Regression coverage:
- test_turn_journal_helper_reports_completed_for_matching_stream
- test_turn_journal_helper_rejects_interrupted_turn
- test_advances_past_pending_from_turn_journal_without_run_journal_terminal
- test_full_recovery_suppresses_duplicates_on_turn_journal_completion
- test_full_recovery_appends_rows_on_turn_journal_completion_loss
- test_gateway_chat_worker_records_turn_journal_completion
Reverting the predicate gate turns the two new CORE regressions red;
reverting the Gateway write turns the Gateway regression red.
A persisted message_count is only authoritative when the write that
produced the messages array also vouched for it. Sidecars written by an
older writer (recovery materialization copied the denormalized state.db
count over real rows) could carry a count stale against the array beside
it, and the prefix shortcut then reads a real shrink as a growth and
skips the #1558 backup.
save() now stamps _mc_v alongside message_count, and _prefix_message_count
refuses any count without the current marker: those files get the full
parse, exactly as on the legacy path. One save re-marks the file, so the
fast path resumes for steady state.
Regression covers the unmarked stale-count shape and the re-marking
transition; mutation-checked by removing the gate (the unmarked fixture
then skips the backup, as the review's reproduction predicted).
The #7535 ownership guard skipped the global model.base_url backfill for
two shapes that master handled:
- A `model` section with `base_url` but no `provider` declares no owner,
so `_providers_match_for_context("", <session provider>)` was false and
the endpoint was dropped, silently falling back to 256,000 instead of
querying the configured /v1/models (the profile-setup path writes this
shape).
- `opencode_go` and `opencode-go` are two spellings of the same built-in
provider, but `_canonical_context_provider` does not fold underscores,
so the owner session lost its URL.
Allow the backfill when no owner is declared, and compare both sides
through api.config._canonicalise_provider_id, which folds `_` to `-` while
keeping distinct `custom:*` slugs distinct. The #7535 case (a custom:ark
owner vs an opencode-go session) still resolves to an empty base_url.
_poll_once() resets the parity timestamp when it runs with no subscribers so
the next subscription gets a fresh projection, but since the idle-park change
(#7694) the poll loop parks before calling _poll_once() once the last
subscriber leaves. A tab that reconnects inside the parity interval then
resumes with the stale cheap fingerprint and skips the projection.
Reset _last_cheap_fp and _last_full_projection_at in unsubscribe() when the
last subscriber is removed (not for an unknown queue, not while others
remain).
Re-review #7165 (nesquena-hermes 2026-09-23T06:21:57Z, CHANGES_REQUESTED).
CORE: a dict-shaped custom_providers[].models mapping (what Hermes Agent's
setup flow writes, e.g. {chat-a: {context_length: 128000}}) was treated as an
allowlist by iterating its keys, collapsing the live /v1/models catalog to a
single saved default while the CLI live-probed the full list. The Agent defines
exactly this contract in hermes_cli/model_switch.py::_models_config_is_allowlist:
a mapping is per-model metadata, NOT a catalog narrow.
Fix (all 3 spec points):
1. api/routes.py::_custom_provider_allowlist_ids: added
_custom_provider_models_discover_is_false(_cp) (mirrors
api.config._provider_discover_allowed / Agent
model_switch_providers.py:557: bool False or string false/no/0,
case-insensitive). In the dict branch, when discovery is NOT explicitly
false, return no allowlist so the live probe returns the full catalog.
Only discover_models: false opts into treating dict keys as a pinned
allowlist. List / JSON-array-string / Python-literal shapes stay plain
allowlists.
2. tests/test_issue3718_live_models_custom_probe.py: removed the
mapping-metadata param that asserted the regression; added two
real-handler tests: test_dict_models_without_discover_false_does_not_gate
(dict -> full _LIVE_CATALOG) and test_dict_models_with_discover_false_pins_to_keys
(bool False / "false" / "no" / "0" -> pinned to keys).
3. docs/architecture/live-models-allowlist.md: documented that list and
string shapes filter, and a dict filters only with discover_models: false.
Companion test update (blocker reported by the fix itself):
tests/test_byok_model_dropdown.py rightcode-codex entry encoded the removed
dict-as-allowlist behavior (models: {"gpt-5.5-mini": {}} asserting the response
collapsed to just that key). Added discover_models: False to preserve the
provider-scoping intent (no sibling leak) under the new contract.
Red/green verified: removing the dict-branch guard turns
test_dict_models_without_discover_false_does_not_gate red (collapsed catalog);
restoring it turns it green.
Verification: 3718 18 passed; byok 24 passed; neighbour sweep (46 files)
520 passed (2 pre-existing order-dependent flakes reproduce identically on
clean HEAD, verified via git stash); ruff curated gate OK.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #1558 backup safeguard in Session.save() compares the on-disk message
count with the incoming one and writes <sid>.json.bak only when the save would
shrink the array. Sound -- but it obtained the on-disk count by
json.loads(self.path.read_text()), the entire file, on EVERY save, including
the 99.9% of saves that grow the conversation and never back anything up.
Measured in production (Mac docker stack), in the running container:
sessions/fa3bca34a0c6.json 203,439,398 bytes / 266,940 messages
read_text 17,453 ms
json.loads 2,924 ms
TOTAL to obtain one integer: 20,377 ms <- paid on every save
That was the 39,357 ms /api/session request stuck at t6_after_json_write,
after the read-side re-parse had already been fixed. It scales linearly with
the file, and the file grows for as long as the conversation does.
The decision below needs ONE integer. save() already writes that integer into
the metadata prefix, before `messages`, as `message_count` -- exactly so
readers can have it without parsing the body, which is how
load_metadata_only() and the sidebar freshness check already work. So save()
now reads THAT, through the same bounded 64 KiB prefix reader, and falls back
to the full read + parse whenever the prefix does not carry a usable count: a
legacy pre-#5854 sidecar whose count serialized after the scenes, a file with
no top-level `messages` key at all, a corrupt or truncated prefix, metadata
alone that overflows the budget, or an unreadable path. Fail-open is the
contract -- never "assume no shrink". The full text is still read for one
thing only: the .bak body, when a shrink is actually detected.
An in-memory stat identity was tried first and is NOT sufficient, so it is
gone. The first revision of this branch stamped the (inode, size, mtime_ns) of
the bytes it wrote and trusted the remembered count while that tuple matched,
on the reasoning that "stat says nothing touched it, so the array is the one I
wrote". Review (nesquena-hermes, 2026-09-21) reproduced the hole: a stat tuple
is not a content identity. A same-length in-place rewrite inside one mtime tick
keeps inode, size AND mtime_ns -- ext4 stamps mtime from a coarse clock, so two
writes in the same tick share one mtime_ns -- and the next shrinking save then
reads a stale count, concludes it is growing, overwrites the longer transcript
and writes no .bak. That is the #1558 data-loss shape the safeguard exists to
prevent, reachable on this branch. A count read from the file's own bytes
cannot be fooled that way: any rewrite of the bytes rewrites the count.
So _disk_identity() is removed along with the identity fields on Session, the
stamp after the write, and the seeding in load(). _prefix_message_count() is
the replacement: bounded prefix -> the number the writer itself recorded.
One trap survives the redesign and is pinned. load() de-duplicates adjacent
partials and immediately saves the shorter transcript (#2592), and that save
must produce a .bak because it shrinks the array on purpose. The count that
decides it is the one the writer stamped on the file -- the ON-DISK length it
wrote -- not the post-collapse length of the object. Read the object's own
length instead and the #2592 backup disappears silently.
session_recovery._msg_count() pays the same cost at boot and is deliberately
left alone. Its "torn file -> -1" contract is what makes recovery restore a
.bak over a truncated live file, and no bounded read can prove a file is not
truncated. A boot pays it once; a save paid it every time.
Tests: 9 in tests/test_save_count_without_full_parse.py. The review's
reproduction is the last one -- a same-inode, same-byte-length, same-mtime_ns
in-place rewrite of 2 -> 5 messages, then a save of 3, which must leave the
5-message array recoverable. It fails against the removed stat-identity design
(re-verified by restoring that revision: no .bak is written and the
5-message transcript is lost) and passes here. Mutation-checked the same way:
recording the post-collapse length fails exactly the #2592 test. 783 tests
pass across the 41 files that touch this surface (save/load, metadata-only,
collapse, .bak recovery, sidecar reconciliation); ruff is clean on the diff.
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.
[rebase onto current master — same content as 5a7f7d8, no functional
changes beyond a single context-line merge from master]
Squash-rebase of #7656 onto current master so the 3-way merge CI
test no longer pulls in the unmerged stage/7618-7619-renderer-i18n
dirty diff that 5a7f7d8 was based on. The actual change set is
unchanged: 2 files, +231/-1.
What changed (from #7656 / 5a7f7d8):
- api/models.py::_read_max_session_resolve_concurrent — new
module-level helper that reads the cap from the env var
HERMES_WEBUI_MAX_SESSION_RESOLVE with strict validation
(positive int in [1, 64], malformed → default 2).
- tests/test_issue7421_resolve_cap_config.py — 12 regression
tests covering unset/empty/whitespace, headline cases (4/8/64),
safety paths (0/-1/999999/non-numeric), whitespace tolerance,
BoundedSemaphore capacity check, and a static wiring assertion.
Model: MiniMax-M3
Round-3 removed the `"language": "en"` schema default so fresh installs stopped
reporting a fake explicit English, and dropped the `primary === 'en'` skip.
The maintainer's round-4 gate asked to complete the tri-state shape:
1. `load_settings()` now emits an explicit `None` for the unset case
(`settings.setdefault("language", None)`) so `/api/settings` carries a
real three-way signal instead of omitting the key: `null` = no
preference, "en" = explicitly saved English, any other code = chosen
locale. Stored values (including legacy pre-change English picks) still
win via the merge and are untouched.
2. New `TestReviewerTableServerTriState` regression loads the real
`static/i18n.js` in a Node `vm` sandbox and drives
`resolvePreferredLocale(s.language, localStorage.getItem('hermes-lang'),
_detectBrowserLanguageHint())` — the exact boot.js call shape — for all
four rows of the maintainer's reproduction table:
- `en` + new browser (zh-CN) -> en (was zh under round-2 skip)
- `en` + stale ja + en-US -> en (was ja under round-2 skip)
- `en` + empty + en-US -> en
- `fr` + empty + zh-CN -> fr
Verification:
- `python3 -c "import api.config"` OK
- 15/15 in test_issue7622_initial_locale_from_browser.py (14 + new table test)
- 8/8 in test_language_precedence.py
- 60/60 settings + 13/13 password-env + login-locale/autosave/sprint26
collateral tests pass
- ruff_lint --diff origin/master: 0 new violations
Hermes persists discovery results as models: {...} + models_discovered: true.
_custom_provider_allowlist_ids() treated any plural models value as an
explicit allowlist, so after one discovery /api/models/live was permanently
pinned to the first-discovery set and newly available upstream models were
silently dropped (gate repro: new-live-c missing vs merge-base).
Gate the helper on api.config._provider_models_are_discovered_catalog() —
the same predicate the /api/models path uses — and return no allowlist when
it holds. discover_models: false still pins via the predicate; a hand-written
models: list without the discovered marker still filters; probe-failure
fallback still serves saved ids.
Real-handler tests: discovered catalog preserves full live catalog (RED on
the previous head), discover_models: false control stays pinned, failed-probe
control still returns saved models.
Greptile flagged that `models: []` / `models: "[]"` falls through the
non-empty allowlist gate and leaves the full live catalog selectable. The
symptom is real, but implementing the ask (gate on declared-ness instead)
measurably makes things worse: it emits an EMPTY picker and leaves the
provider unselectable.
Measured both branches against the real handler before deciding:
models key absent declared=False -> [chat-a, chat-b, image-gen-1, audio-tts-1]
models: [] declared=True -> [] EMPTY PICKER
models: "[]" declared=True -> [] EMPTY PICKER
models: [chat-a] declared=True -> [chat-a]
An unselectable provider is a harder failure than a few extra visible
models, and it is unrecoverable from the UI: `custom_providers` has no
WebUI write path, so it is hand-edited in config.yaml. There is also no
deny-all use case — a provider the user wants hidden is removed from
`custom_providers` outright, not left behind with an empty allowlist.
No production behavior change. Records the trade-off in the filter block
and adds `test_empty_allowlist_is_treated_as_not_configured` (native empty
list, serialized "[]", empty mapping, blank scalar) so a future "fix" of
this edge case fails loudly instead of silently emptying the picker.
Verified non-vacuous: applying the declared-ness gate fails all 4 new
cases with `assert [] == [...]`.
Custom providers (esp. New-API-style gateways) expose their ENTIRE catalog
via /v1/models, including image/audio models that aren't chat models. The
live-catalog enrichment surfaced all of them in the model picker regardless
of the `models` allowlist declared in config.yaml.
Filter the live catalog down to the explicit plural `models` allowlist, then
append any allowlisted models the upstream didn't return. Only the plural
`models` key gates the filter — a provider configured with just a singular
`model` (sticky/default metadata) keeps the unfiltered live catalog so
discovery still works.
Decode the plural value through `_parse_config_string_list()` so serialized
shapes are honored: `hermes config set` and JSON-mode editor saves persist
lists as quoted JSON-array strings ('["chat-a","chat-b"]') or Python
literals ("['chat-a']"). A dict/list-only parser dropped those to [], which
the caller reads as "no allowlist configured" → full catalog floods the
picker (same class as the skills.disabled regression, #7120/#7134). Native
dict/list values are still walked directly so dict entries keep their
id|model|name metadata, which the decoder would str()-ify.
Tests invoke the real `_handle_live_models` and assert on the emitted
`models` payload, covering: native list, JSON-array string, Python-literal
string, mapping metadata, list-of-dicts, singular-model-only (not gated),
malformed serialized input (safe), allowlisted model missing from live
(appended), and probe failure (config fallback).
Verified non-vacuous: removing the production filter block fails 7 of 9
cases; reverting only the string decode fails exactly the 3 serialized
cases with the documented flood symptom (image/audio models reappear).
start_session_turn() answers 409 retryable=True while the Agent runtime is
stale, 409 process_wakeup_paused while wake-ups are paused, and 409 with
active_stream_id while the session is busy with another turn. The bridge
released the durable claim as a plain failure each time, so a burst of
refusals within seconds terminally dropped a completion whose origin session
was alive and waiting.
release_async_delegation_delivery(..., retryable=True) now forwards the
refund to an Agent core whose release_event_delivery() accepts the
`retryable` keyword (NousResearch/hermes-agent#120028), detected through
inspect.signature, and falls back to the historical plain release on an
older core. Hard rejections keep the bounded budget.
When another Hermes process holds a session's turn lease, the Agent
(agent/turn_facade_lease.py) emits lifecycle statuses while waiting and on
admission, and a warn status when the wait times out and the message was
not processed. _agent_status_callback dropped all of them, so a WebUI turn
looked stuck with no explanation.
Classify these notices by Agent status kind (lifecycle/warn) plus the
Agent's exact phrases, and relay them as {'type': 'session_lease_wait'}
warning events rendered by the existing messages.js warning listener.
Re-review #7682 (nesquena-hermes 2026-09-22T19:18:00Z, CHANGES_REQUESTED).
`_transcript_already_advanced_past_pending` suppressed recovery on a
transcript-only heuristic: the first assistant row with final visible text
inside the pending turn's boundary. An interrupted multi-segment turn shaped
like
user -> assistant "Let me check the logs first." (unflagged interim prose)
-> tool -> tool ... (stream dies here)
passed the predicate as "finished" even though the interim sentence is not a
final answer and the stream was cut off mid-tool. `_apply_core_sync_or_error_marker`
then cleared pending state and returned early: the run journal was never
replayed and no interruption marker was added — a half-finished turn rendered
as complete, journaled tool output never appearing.
Fix: a genuine final answer inside the turn is now NECESSARY BUT NOT
SUFFICIENT. The suppression also requires durable same-stream terminal
evidence — `_run_journal_terminal_state(session, active_stream_id) == 'completed'`
(a `done`/`stream_end` event). Without it, fall through to the normal
journal-recovery path so the partial output is replayed and the interruption
is marked.
Regression tests (red/green verified — both fail with the gate removed):
- test_does_not_advance_when_unflagged_interim_prose_precedes_tools
- test_full_recovery_runs_journal_replay_for_unflagged_interim_prose
(drives `_apply_core_sync_or_error_marker`, asserts journal replay +
interruption marker present)
- test_does_advance_when_journal_marked_done_after_interim_prose (positive)
Existing `_FakeSession` fixtures updated to carry a session_id and write a
`done` journal event so the durable-evidence gate is satisfied for the
settled-turn tests.
Verification: test_issue6366 35 passed; anchor_scene + cancelled_turn
neighbours 91 passed. Red/green proven on both regressions.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nesquena-hermes