88236dbc26249ed2f6830bc92c43bf62fa414350
47
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 happy5318
|
88236dbc26 |
docs(models): pin empty-allowlist semantics with regression coverage
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 [] == [...]`. |
||
|
happy5318
|
4a1fb47ce5 |
fix(models): respect config model allowlist in live custom-provider catalog
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).
|
||
|
happy5318
|
d698fb4772 |
fix(routes): mirror the detail-load endpoint's 409 contract in every profile-mismatch check (#7710)
## Thinking Path The detail-load endpoint (and the foreign-session synthesizer) already distinguish "session owned by a KNOWN other profile" (return ``409 session_profile_mismatch`` so the client can offer to switch to it per #5419) from "session missing/legacy with no profile stamped" (return ``404 Session not found`` so the frontend's self-heal clears the stale URL + localStorage). The generic request-guard ``_session_id_visible_to_request_profile`` flattened both cases to a ``404 Session not found``, so any cross-profile POST/PATCH/DELETE (archive, rename, pin, delete, session-attach, anchor-scene, …) and the stream-related status/cancel/replay paths failed with a misleading "Session not found" instead of the actionable 409 the detail endpoint emits. The reporter's wire evidence: ``` POST /api/session/archive {"session_id":"<id>","archived":true} -> 404 {"error": "Session not found"} GET /api/session?session_id=<same id> -> 409 {"error": "Session belongs to a different profile", "code": "session_profile_mismatch", "session_id": "<id>", "profile": "def"} ``` Same session id, same moment: the detail endpoint reports a profile mismatch, archive masks it as 404. The fix mirrors the detail endpoint's contract in the generic guard and in every other inline check that previously went to ``bad(handler, "Session not found", 404)`` for a known-other-profile session. ## What Changed 1. ``api/routes.py:595`` — ``_session_id_visible_to_request_profile`` now emits ``409 session_profile_mismatch`` for a session owned by a known other profile, and keeps ``404 Session not found`` only for the None-profile (unknown/legacy) case so the frontend self-heal still fires for actually-missing sids. 2. ``api/routes.py:5348`` (anchor-scene persist), ``:23994`` (compression-recovery start), ``:24385`` (chat/start with the inline placeholder-retag rules preserved), ``:28621`` (session import_cli's already-imported branch) — the same 409-vs-404 split, with the existing branches kept intact (the chat/start retag path, the import_cli ``allow_all_profiles`` short circuit). 3. The ``_stream_id_visible_to_request_profile`` wrapper already delegates to ``_session_id_visible_to_request_profile`` for the owner-session check, so the stream status / cancel / replay endpoints get the new contract for free. ## Why It Matters A user picking the configured default model in the dropdown sees two visually identical entries for the same model id, picks the wrong "prefixed" one (the one matching the dropdown's order), the agent returns 400, and the session state is corrupted with a non-functional model id until the user manually clears it. The dropdown's own deduplication already handles `@provider:model` correctly via the documented prefix form; only the synthesised ``{provider}/{model}`` form was wrong. The cross-profile write paths (anchor-scene persist, compression-recovery start, chat/start, session import) all had the same gap: a successful auth under profile A could persist or recover state against a session owned by profile B, surfacing a 404 to profile A's user that reads like "the session is gone" — but the session is in fact intact, just owned by another profile. The 409 contract is what closes the gap: the client gets a structured signal it can act on, and the session state stays consistent. ## Verification - ``tests/test_issue7710_cross_profile_404_to_409.py`` (new, 6 tests): - 5 behavioural: profile-mismatch path returns 409 with the ``session_profile_mismatch`` payload, the None-profile path keeps 404 for the frontend self-heal, visible session passes silently, missing session passes silently, ``emit_error=False`` suppresses the response. - 1 source-shape: the helper body MUST contain a 409 branch with the ``session_profile_mismatch`` code; a silent reversion to the single-line ``bad(..., 404)`` fails the suite. - ``tests/test_issue4812_session_sse_stream.py`` — 1 assertion updated to the new 409 payload (the SSE route walks through the generic guard). - ``tests/test_session_active_profile_authorization.py`` — 7 assertions updated to the 409 payload (duplicate, file, chat/start foreign, chat/stream/status, chat/stream/cancel, chat/stream replay). Stream tests use ``foreign_session`` (the owner session id), NOT the stream id, because ``_stream_id_visible_to_request_profile`` delegates to the owner-session check. - ``tests/test_anchor_scene_persistence.py`` — 1 assertion updated to the 409 payload. The 3 ``Assistant message not found`` 404 cases (lines 454/561/611) are kept as-is — they're not profile mismatches, they're missing-message 404s. - ``tests/test_issue4067_import_cli_cross_profile_guard.py`` — 1 assertion updated to the 409 payload, plus the ``_capture`` mock signature updated to accept the ``status`` keyword that ``j(handler, payload, status=200, ...)`` already passes. Other tests in the existing suite: **161 passed across 8 related test files** (session active-profile authorization, anchor scene, issue 4067, issue 4812 SSE, issue 7710 new, chat start claim, workspace stale recovery, issue 5420 profile switch). ## Contract Change The following routes now return ``409 session_profile_mismatch`` (``{error, code, session_id, profile}``) for a session owned by a known other profile, where they previously returned ``404 Session not found``: - ``POST /api/session/duplicate`` (now also goes through ``_guard_request_session_visibility`` for the body session id) - ``GET /api/file?session_id=…`` - ``POST /api/chat/start`` (the inline profile check after ``_get_or_materialize_session``) - ``GET /api/chat/stream/status?stream_id=…`` - ``GET /api/chat/stream/cancel?stream_id=…`` - ``GET /api/chat/stream?stream_id=…`` (the dead-stream replay branch) - ``POST /api/session/anchor-scene`` - ``POST /api/session/compression-recovery/start`` - ``POST /api/session/import_cli`` (the already-imported branch when ``allow_all_profiles`` is False) The ``404 Session not found`` self-heal path is preserved for the None-profile (unknown/legacy) case across all of these routes, so the frontend's existing stale-URL/localStorage cleanup behaviour is unchanged for actually-missing sessions. ## Risks - A consumer that previously relied on the 404 to detect "this session belongs to another profile" will now see a 409 with a ``code`` field. The detail-load endpoint has emitted this exact contract since #5419, and the frontend already handles it (offers to switch to the owning profile). The cross-profile write paths now match the detail-load contract; the frontend's already-shipped switcher UI handles both surfaces consistently. - The anchor-scene / compression-recovery / chat-start tests that were updated are a small set (5 total); the existing ``Assistant message not found`` 404 paths in ``test_anchor_scene_persistence`` are kept untouched (those are not profile mismatches). ## Related - #7710 — original issue (elkpvl-commits, 2026-09-22) with the full repro and the suggested fix shape. - #7549 — the underlying cross-profile archive bug, already tracked by @webtecnica in #7559 (which is the "business-level" fix: row-profile carry + frontend sends profile + materialize into the row's own profile). This PR is the orthogonal error-code-level fix: the request-guard's 404-vs-409 split. - #5419 — the original auto-switch-profiles-for-session-links feature, whose frontend switcher UI consumes the ``session_profile_mismatch`` code on the 409. |
||
|
happy5318
|
5518e4ceca |
fix(config): drop synthetic `{provider}/{model}` alias from configured-model badges (#7290)
## Thinking Path
When the active provider is a built-in plugin provider (e.g. ``commandcode``)
and the configured default model id contains a slash (e.g.
``deepseek/deepseek-v4-flash``), ``_build_configured_model_badges`` and
``_configured_model_badges_from_static_catalog`` used to publish three
raw_candidates for the same configured identity:
- the bare native id (``deepseek/deepseek-v4-flash``)
- a slash-prefixed alias (``commandcode/deepseek/deepseek-v4-flash``)
- the ``@provider:model``-prefixed form (``@commandcode:deepseek/deepseek-v4-flash``)
The slash-prefixed alias was synthetic — the catalog never actually
exposed that key. The WebUI dropdown rendered it as a real option,
selecting it sent ``model=commandcode/deepseek/deepseek-v4-flash`` to
the agent, and the provider rejected it with ``HTTP 400: Model ...
is not supported on this endpoint``. The session also persisted the
wrong id, so reload replayed the failure.
Verified by reading the live source at master: the catalog exposes
only the bare native id for built-in plugin providers; the
slash-prefixed alias was a three-line entry in the ``raw_candidates``
tuple. Maintainer's 9/25 pickup contract on the issue explicitly
prohibits positional slash-splitting for ownership inference: slashes
are valid in model ids (multi-slash, URI-scheme, host:port). The fix
drops only the synthetic prefix form; the bare native id (the actual
catalog form) and the documented ``@provider:model`` prefix stay.
## What Changed
1. ``api/config.py:8617`` (``_build_configured_model_badges``) — drop
the ``f"{provider}/{model}"`` entry from the ``raw_candidates``
tuple. The tuple now contains only ``model`` and
``f"@{provider}:{model}"``. Also type-annotate the local list.
2. ``api/config.py:6946`` (``_configured_model_badges_from_static_catalog``)
— same one-line change for the static-catalog fallback path so the
two builders stay in sync.
The match_id / norm_lookup / exact_match logic downstream is unchanged
— those already prefer the matching provider, and with the synthetic
alias gone the bare native id is the only remaining self-collision
candidate (which the existing ``test_duplicate_slash_id_primary_badge_sticks_to_matching_provider_only``
already pins).
## Why It Matters
A user picking the configured default model in the dropdown sees two
visually identical entries for the same model id, picks the wrong
"prefixed" one (the one matching the dropdown's order), the agent
returns 400, and the session state is corrupted with a non-functional
model id until the user manually clears it. The dropdown's own
deduplication already handles `@provider:model` correctly via the
documented prefix form; only the synthesised ``{provider}/{model}``
form was wrong.
## Verification
- ``tests/test_issue7290_no_slash_prefixed_alias.py`` (new, 10 tests):
- ``test_commandcode_deepseek_does_not_emit_slash_prefixed_alias`` —
the literal repro from the issue: ``commandcode/deepseek/deepseek-v4-flash``
MUST NOT appear in the badge map; the bare id ``deepseek/deepseek-v4-flash``
is the primary badge.
- ``test_commandcode_deepseek_at_prefixed_alias_still_present`` — the
``@provider:model`` form is preserved.
- ``test_duplicate_slash_id_does_not_pollute_other_provider`` — per
contract 4: dedupe only when provider + full native model match.
- ``test_non_slash_models_unchanged`` (parametrized × 4) — the bare,
@-prefixed, URI-scheme, and URL model id forms all keep working.
- ``test_synthetic_slash_prefix_alias_is_not_in_source`` (parametrized × 2)
— the synthesised ``f"{provider}/{model}"`` literal MUST NOT appear
in either builder; a silent reversion to the bug fails the suite.
- ``test_raw_candidates_only_holds_bare_and_at_prefix`` — pins the
shape of the ``for candidate in (...)`` tuple so a future
regression that adds any *other* synthetic prefix form
(``f"{provider}::{model}"``, etc.) also fails.
- ``tests/test_model_picker_badges.py::test_available_models_exposes_primary_and_fallback_badges``
updated to assert the bare-id form (``claude-haiku-4.5``) and the
``@provider:model`` form (``@anthropic:claude-haiku-4.5``) carry the
fallback badge; the deleted slash-prefixed form is explicitly
asserted absent. All other test_model_picker_badges cases unchanged.
- The 4 sibling tests
(``test_issue3360_multi_slash_model_collision``,
``test_issue3429_uri_scheme_model_ids``,
``test_issue3959_model_suffix_dedup``,
``test_norm_model_id_trailing_empty_guard``) still pass — they pin
``_norm_model_id`` behaviour which is unchanged by this PR.
- ``pytest tests/ -k 'model or picker or badge'``: 1507 passed,
1 skipped, 2 failed. The 2 failures (test_issue1426_free_tier,
test_issue2245_mixed_case) are the documented upstream shard-race
flake — both pass when run individually (verified). Pre-existing
per issue #7656's notes.
- ``ruff check api/config.py tests/test_issue7290_no_slash_prefixed_alias.py``:
clean on the new test file. (The 4 pre-existing F811/import-order
findings in api/config.py are on untouched lines.)
## Contract Change
No API contract change. The ``/api/models`` payload's
``configured_model_badges`` map drops one synthetic key per configured
entry (the slash-prefixed alias). Real consumers that look up a badge
by exact native id or by ``@provider:model`` are unaffected. The
dropdown's existing dedupe is now correct without a manual workaround.
## Risks
- A consumer that depended on the slash-prefixed alias as a stable
lookup key would need to switch to the bare native id. No consumer
in this repo matches that pattern — ``test_duplicate_slash_id_*
_primary_badge_sticks_to_matching_provider_only`` was already
asserting that the bare id is the canonical lookup.
- The dropdown's option value (the string sent as ``model=`` to the
agent) is the bare native id; the test runner, the session
restore path, and the request payload all use the bare id
consistently per maintainer contract 3.
## Model Used
Hermes Agent (step-5-preview) with pytest, ruff. Tests execute the
live ``_build_configured_model_badges`` from the production source
(``exec`` the function body with inline ``_norm_model_id``), so a
revert to the original ``raw_candidates`` tuple fails the suite
immediately on the source-shape test.
## Related
- #7290 — original issue (Porter-Key, 2026-08-25) with the full repro
and the maintainer's 9/25 pickup contract (6 items).
- #3478 — named custom-provider ``@custom:name:model`` prefix.
- #7241, #6884 — adjacent ``@provider:model`` prefix lanes.
- #3360 — multi-slash model id collision (the ``_norm_model_id``
fix predates this PR and is preserved).
- #3429 — URI-scheme model id preservation (also preserved).
- #3959 — colon-suffix model id preservation (also preserved).
|
||
|
happy5318
|
c53226dcc4 |
fix(login): hide the password form when `is_password_auth_enabled()` is False (#7056)
## Thinking Path
With native OIDC configured and ``HERMES_WEBUI_PASSWORD`` unset, the
``/api/auth/status`` endpoint correctly reports
``password_auth_enabled: false``, but ``/login`` still rendered the
password input, the "Sign in" button, and the "Continue with SSO"
entry point — and silently 401-ed every submit. The reporter's wire
evidence:
```json
GET /api/auth/status -> {
"auth_enabled": true,
"oidc_enabled": true,
"password_auth_enabled": false
}
GET /login -> still shows:
"Enter your password to continue"
+ password <input>
+ "Sign in" button
+ "Continue with SSO" link
POST /api/auth/login -> 401 {"error":"Invalid password"}
```
The fix routes the password input / submit / passkey controls through
a single ``{{PASSWORD_FORM_HTML}}`` placeholder in the template, which
the handler populates with the live markup only when
``is_password_auth_enabled()`` is True. The OIDC SSO entry point stays
the sole path when password auth is disabled.
## What Changed
1. ``api/routes.py`` template literal (``_LOGIN_PAGE_HTML``): the
three inline elements (password input, submit button, passkey
button) are replaced by a single ``{{PASSWORD_FORM_HTML}}``
placeholder. The ``{{OIDC_LOGIN_HTML}}`` placeholder is kept
untouched.
2. ``api/routes.py`` login branch: the handler imports
``is_password_auth_enabled`` from ``api.auth``, computes the
password form markup when the predicate returns True, and an
empty string otherwise. The ``.replace("{{PASSWORD_FORM_HTML}}", _password_form_html)``
call slots the result into the template. The
``{{LOGIN_PLACEHOLDER}}``, ``{{LOGIN_BTN}}``, ``{{LOGIN_INVALID_PW}}``,
``{{LOGIN_CONN_FAILED}}`` placeholders are no longer referenced
from the template (the password input is built with its own
``placeholder`` attribute), so the four corresponding
``.replace(...)`` calls are removed.
## Why It Matters
A user with OIDC-only auth clicking "Sign in" on the password form
gets a 401 with no UI signal — the OIDC entry point is right there,
but the password prompt was so prominent it read like the primary
path. Hiding the form when password auth is off makes OIDC the only
visible option, so the user is funneled to the working path.
## Verification
- ``tests/test_issue7056_login_hides_password_form.py`` (new, 6 tests):
- **Source-shape (2 tests):** the template uses the
``{{PASSWORD_FORM_HTML}}`` placeholder AND does not carry an
inline ``<input type="password">``; a silent reversion to the
pre-fix multi-line shape fails the suite. The login branch
imports ``is_password_auth_enabled`` and populates the
placeholder.
- **Behavioural (3 tests):** the rendered ``/login`` HTML contains
the password input / submit / passkey controls when
``is_password_auth_enabled()`` is True; does NOT contain them
when False; and keeps the ``<a id="oidc-login">`` SSO entry
point when OIDC is enabled (so the user can still log in).
- **Sanity (1 test):** with both password and OIDC disabled, the
page renders neither control.
- 51 tests pass across the related auth/login files
(``test_issue7056_login_hides_password_form``, ``test_auth_settings_safety``,
``test_issue3825_oidc_auth``) — no pre-existing test is broken.
## Contract Change
None. The ``/login`` HTML body changes shape only when
``is_password_auth_enabled()`` is False; the password input + submit
+ passkey controls are gone, but the OIDC SSO entry point stays
present. The ``/api/auth/status`` contract is unchanged — it already
reported the right ``password_auth_enabled`` flag; the fix is purely
about the page matching the status.
## Risks
- A consumer that scraped the ``/login`` HTML for the password
input (e.g. a browser extension) will see the input disappear
when ``HERMES_WEBUI_PASSWORD`` is unset. The OIDC entry point
remains the only path, which is the intended behaviour when
password auth is disabled.
- The handler now imports ``is_password_auth_enabled`` at request
time (same module as the other auth predicates already imported);
if the auth module is unavailable the import fails fast and the
``/login`` route surfaces the underlying exception, matching the
behaviour of the other auth imports in the same branch.
## Related
- #7056 — original issue (segator, 2026-08-15) with the wire evidence
and the expected-vs-actual contract.
- The OIDC SSO entry point lives in ``_oidc_login_html`` (line 11160)
and is unaffected by this PR — the fix only gates the password
controls, not the SSO button.
## Model Used
Hermes Agent (step-5-preview) with pytest, ruff. Tests use
``_FakeHandler`` with an ``io.BytesIO`` ``wfile`` (matching
``_safe_write``'s ``handler.wfile.write(body)`` expectation) and
mock the OIDC predicate through ``_stub_oidc`` so the
behavioural assertions don't depend on real OIDC env config.
|
||
|
happy5318
|
a26bc45f3c |
fix(session-stream): stop the hidden-tab poll on 404/410 so deleted sessions don't loop forever (#7299)
## Thinking Path
When a tab is hidden, Hermes WebUI replaces the persistent
per-session SSE connection with a lightweight ``GET /api/session/status``
poll every six seconds. If the tracked session has been deleted
or no longer exists in the active state directory, the endpoint
returns ``404`` — but the client previously converted every non-2xx
response to ``null`` and kept the interval alive forever. This
produces an invisible stream of permanent 404 requests from stale
background tabs (one tab generates ~10 requests per minute when
browser timer throttling allows it; multiple stale tabs multiply
the noise).
The fix narrows the stop condition to the truly terminal responses
(``404 Not Found`` and ``410 Gone``) and calls
``_stopHiddenActiveStreamPoll()`` so the interval and the bound
``_sessionStreamHiddenPollSid`` are cleared together. Transient
failures (5xx, rate-limit, network error) keep polling because they
do not necessarily mean the session is gone; a later tick may catch
a server-initiated turn starting while the tab remains hidden.
## What Changed
``static/messages.js`` — ``_startHiddenActiveStreamPoll()``'s fetch
chain now branches on ``r.status`` before the ``r.ok ? r.json() : null``
fallback:
```js
fetch(...)
.then(r => {
// #7299: 404 Not Found / 410 Gone are TERMINAL for this
// session-owned poll. ... only the missing
// session itself is terminal.
if ((r.status === 404 || r.status === 410) && _sessionStreamHiddenPollSid === sid) {
_stopHiddenActiveStreamPoll();
return null;
}
return r.ok ? r.json() : null;
})
.then(d => { ... });
```
The pre-existing ``r.ok ? r.json() : null`` fallback is preserved
for any non-2xx, non-terminal response, so a 5xx / rate-limit /
network error still produces ``null`` and the next tick re-fires.
A regression that broadened the stop path to all non-2xx would
re-introduce the original bug in the opposite direction (the poll
would die on a single 503 and never recover).
## Why It Matters
A stale background tab with a deleted session could hold an
interval running for hours, firing a 404 every six seconds. The
user sees nothing because the tab is hidden; the server sees
inbound requests that are guaranteed to fail; the request log
fills with noise that an operator has to triage. The fix is
mechanical — the server already returns the right status code, the
client just needs to honor it.
## Verification
- ``tests/test_issue7299_hidden_poll_stops_on_404.py`` (new, 5 tests):
- **404 stops the poll** — source-shape check on the response
handler; a silent reversion to the pre-fix
"convert all non-2xx to null and keep polling" form fails
the suite.
- **410 stops the poll** — same shape, separate code branch
so a future refactor cannot satisfy the 404 check with a
single OR expression.
- **bound sid guards the stop** — a stale fetch's response
landing after a session switch must not tear down the *new*
session's poll.
- **5xx / rate-limit / network errors keep polling** — the
``r.ok ? r.json() : null`` fallback must remain.
- **6000ms interval + immediate-fire preserved** — the fix is
in the response handling, not the cadence.
- ``tests/test_hidden_tab_server_initiated_turn.py`` — pre-existing
tests for the hidden-tab poll (8 tests). The 4 that use
``MESSAGES_JS[start:start + N]`` slicing had their window widened
(2400/3200 → 8000) to cover the new 404/410 branch.
- 33 tests pass across the related session/stream/hidden files
(``test_hidden_tab_server_initiated_turn``,
``test_issue7299_hidden_poll_stops_on_404``,
``test_issue3718_live_models_custom_probe``,
``test_issue6419_pending_user_reconnect_order``,
``test_session_search_bfcache_822``) — no pre-existing test
is broken.
## Contract Change
None. The server already returned the right status code
(``404`` for a missing session); the fix is purely about the
client honoring it. No new public API, no breaking change
to the SSE lifecycle, no change to the visible-tab path.
## Risks
- A regression that broadens the stop path to all non-2xx would
re-introduce the original bug in the opposite direction (the
poll would die on a single 503 and never recover). The new
``test_hidden_poll_preserves_transient_failure_retry`` pins
this contract.
- The 404/410 stop path checks the bound
``_sessionStreamHiddenPollSid === sid`` before stopping, so a
stale fetch's response landing after a session switch cannot
tear down the *new* session's poll. The new
``test_hidden_poll_404_stop_guards_on_bound_sid`` pins this
contract.
## Related
- #7299 — original issue with the wire evidence and the
expected-vs-actual contract.
|
||
|
happy5318
|
3557982881 | docs(run-state-rfc): note the window-free echo de-duplication semantics | ||
|
happy5318
|
3369bc6a6d |
fix(run-journal): match echo suffixes with a window-free backward walk
Review follow-up: the bounded echo probe folded at most len(text)*3 raw characters, so a compact-equivalent suffix whose raw span was stretched by interior whitespace was missed - the echo strip was dropped and the interim text was duplicated in the restored transcript. The same retired window existed in the reverse helper (_strip_compact_echo_suffix), pinned by its own test. Both now share _find_compact_echo_suffix_start: a backward walk over value/suffix that skips whitespace in value, inspects only the echo span, and allocates nothing. It restores the previous output semantics (origin/master folded the whole reasoning text) and is cheaper than the windowed version (no per-call tail fold). Tests: new journal-reconstruction regression (a 5000-space-stretched echo must appear exactly once; genuine interim text must not be treated as an echo), the suffix-helper oracle updated to window-free semantics, and the window-contract test now pins that a stretched echo IS stripped. |
||
|
happy5318
|
df7db1d3e2 | test(run-journal): pin single-parse locate + summary fallback seam | ||
|
happy5318
|
c799716461 |
perf(run-journal): rebuild live snapshots without quadratic work
`_run_journal_live_snapshot` rebuilds the live UI state for an active run by replaying the whole durable journal. On a long run (22.7k events / 15MB) that rebuild took ~4.4s per session load and blocked the HTTP thread - the dominant cost of "switching to a session with a task running takes forever". Four output-preserving fixes: 1. Locate the journal without parsing it (`find_run_file`) and derive the durable summary from the single `read_run_events` parse instead of parsing the file twice (`find_run_summary` + `read_run_events`). The historical `find_run_summary` lookup is kept as the fallback when no journal file exists on disk. 2. Skip `metering` rows in the projection loop (~60% of a long live journal's bytes; nothing in the snapshot is built from them), while preserving the last_ts watermark they carry. 3. Collect reasoning deltas into a list and materialize lazily instead of `reasoning_text += chunk` (quadratic inside these closures: ~2.3s of the replay on a 9.7k-chunk run). 4. Fold only a bounded tail of the reasoning transcript for the interim-echo suffix check instead of `re.sub` over the whole growing text (~1.5s over 100+ probes). Measured on a 15MB / 22.7k-row live journal: 4.34s -> 0.49s (8.9x) with byte-identical snapshot output (sha256 of the canonical JSON unchanged). Related work: #7310 (slow /api/session under load), #7534 (incremental journal read cache - complementary: this removes one of the two parses entirely and fixes the replay-loop hotspots), #7273/#7291 (metering write-side skip - the read-side loop skip here complements them for legacy journals that still carry metering rows). |
||
|
happy5318
|
bc37675bc2 |
fix(tests): drop unused sys/types imports in #7565 codex commentary test
The test file imports `sys` and `types` but never references either. Ruff F401 flags them. No test or runtime behavior change. - Remove the 2 unused import lines - 11 tests still pass - No new ruff violations |
||
|
happy5318
|
8e31ae9f93 |
# Session MEDIA auth and snapshot capture inspect Codex commentary sidecars (#7565)
## Thinking Path The Agent's OpenAI Codex Responses adapter persists user-visible assistant progress in `codex_message_items` with `phase: "commentary"` while the outer `content` field is empty. The Agent already stamps these items with `role: assistant` and a normalized phase (`agent/codex_responses_adapter.py::_message_item` and `_OutputScan._message`), and the rendering lane is owned by #6828. The WebUI's two consumers of those persisted rows, however, only read the outer `content` field: - `api/routes.py::_session_media_token_allows_path` flattens `message.get("content")` through `_message_content_text()` and scans it for `MEDIA:` tokens. - `api/media_snapshots.py::annotate_media_snapshots` does the same when annotating the settled transcript so previews can survive an in-place overwrite of the source file. For a safe image emitted only inside commentary, the browser request hits the real `_handle_media` route, owns the correct session_id and exact path, but is denied 403 by the auth predicate because it cannot see the assistant's persisted `MEDIA:` token. Symmetrically, the snapshot annotator never captures commentary artifacts, so a subsequent in-place overwrite rewrites history without a recoverable snapshot. The fix is the maintainer's narrow recommendation: one shared fail-closed public-assistant-text iterator in `api/media_snapshots.py` (`codex_commentary_text`), consumed by both the auth predicate and the snapshot annotator. Keeping the extraction in one neutral helper means the auth and capture paths cannot drift apart on a future change. The fail-closed predicate is a strict chain — outer role is `assistant`; item type is `message`; item role is `assistant`; item phase normalizes to `commentary`; content is a list; the part is textual `output_text` with a string `text` value. Every step that fails returns the safe empty string, so the helper is safe to concatenate with the existing top-level extraction. User rows, user sidecars, reasoning items, analysis/final phases, malformed parts, non-string `text` values, and arbitrary nested fields are all skipped. ## What Changed - **`api/media_snapshots.py::codex_commentary_text`** — new module-level helper that returns the concatenation of all public-text `output_text` parts inside assistant-owned `codex_message_items` with `phase: "commentary"`. The outer row role, item type, item role, item phase, content shape, and part type are all gated, so the helper is fail-closed and safe to concatenate with the existing top-level text extraction. - **`api/routes.py::_session_media_token_allows_path`** — inside the existing per-message loop, the per-row `text` is now the concatenation of `_message_content_text(message.get("content"))` and the new `codex_commentary_text(message)`. The exact-path, owning-session, safe-MIME, URL-rejection, hard-denied, and symlink guards are unchanged. The commentary helper is imported lazily inside the loop so this file does not pick up a new top-level dependency. - **`api/media_snapshots.py::annotate_media_snapshots`** — the per-row `text` is now the concatenation of the existing top-level string `content` and the new commentary helper output. The existing `media_re.findall()` and resolve/allow paths are unchanged, so otherwise-capture-eligible commentary previews are now snapshotted at settle time. The string pre-filter on `"MEDIA:" in text` is kept so the helper short-circuits before regex extraction when neither source contains a token. - **`tests/test_issue7565_codex_commentary_media.py`** — new regression module with eleven tests: a positive valid-shape test, six fail-closed tests (user outer role, wrong item type, wrong item role, wrong phase, non-output_text parts, non-string output_text text), a case-insensitive commentary-phase test, a malformed-input test, and two integration static tests pinning that the auth predicate and the snapshot annotator both consume the new helper. ## Why - A user-visible artifact that the agent emitted only inside commentary was returning 403 at the real `_handle_media` route boundary even with the correct owning session_id, and the artifact was never snapshotted at settle time. The fix restores parity with top-level public assistant content for the commentary phase specifically. - Symmetry between auth and capture matters: a token that auth admits must be one capture can stamp, otherwise the served bytes can desynchronize from the snapshot pointer and the in-place-overwrite survivor feature (#6922) silently breaks. Sharing the helper makes drift impossible. ## Verification ``` $ .venv/bin/pytest \ tests/test_issue7565_codex_commentary_media.py \ tests/test_media_inline.py \ tests/test_media_message_snapshots.py \ -q --no-header … ============================= 115 passed in 6.16s ============================== ``` Eleven new tests cover the helper contract and the integration points. The 104 surrounding media_inline / media_message_snapshots tests stay green. ## Contract | Outer role | Item shape | Helper result | Auth result | Snapshot result | | --- | --- | --- | --- | --- | | `assistant` | `{type:"message", role:"assistant", phase:"commentary", content:[{type:"output_text", text:"…MEDIA:…"}]}` | text surfaced | `MEDIA:` token found; existing exact-path / safe-MIME / owning-session guards apply | `MEDIA:` token snapshotted at settle time | | `assistant` | `{type:"message", role:"assistant", phase:"analysis", content:[{type:"output_text", text:"…"}]}` | empty | unchanged from existing top-level-only behavior | unchanged | | `assistant` | `{type:"message", role:"assistant", phase:"final", content:[{type:"output_text", text:"…"}]}` | empty | unchanged | unchanged | | `assistant` | `{type:"reasoning", …}` | empty | unchanged | unchanged | | `assistant` | item role `user` | empty | unchanged | unchanged | | `assistant` | item content not a list | empty | unchanged | unchanged | | `assistant` | part type `tool_call` / `image` / `input_text` (even with text containing `MEDIA:…`) | empty | unchanged | unchanged | | `assistant` | `output_text` with non-string `text` (dict / list / number) | empty | unchanged | unchanged | | `user` (outer role) | any sidecar | empty | unchanged (user rows already skipped) | unchanged | | `assistant` | no `codex_message_items` | empty | unchanged | unchanged | | malformed (`None`, `""`, list, string, missing role) | n/a | empty | unchanged | unchanged | The fix only widens the inspection surface for the one public-text sidecar shape; everything else stays exactly as it was, so existing safe-MIME, hard-denied, exact-path-equality, owning-session, URL-rejection, and symlink guards remain intact. ## Risks - **Commentary widening the inspection surface**: yes, by design. The widening is narrowly scoped to the outer-assistant / item-message / item-role-assistant / phase-commentary / part-output_text / string-text chain. The new tests pin every step in that chain and every out-of-chain shape that must remain rejected. - **Existing safe-MIME / hard-denied / exact-path / symlink guards weakened?** No. The `media_capture_allowed` predicate, the `MIME_MAP` lookup, the `Path(ref).expanduser().resolve() == target_resolved` equality check, the `if "://" in ref` URL rejection, the hard-denied state/secret paths, and the symlink protections are all unchanged. The only change is what text the regex sees. - **Agent producer change**: the helper reads the same shape the agent already stamps (`role: assistant`, `phase: "commentary"`, `content: [{type: "output_text", text: "..."}]`). A future producer change that adds a new phase or part type requires an explicit follow-up PR; the helper's comments document the precise chain. - **Helper duplication drift risk avoided**: by placing the helper in `api/media_snapshots.py` (the storage half) and having `api/routes.py` import it lazily inside the loop, both consumers go through one definition. A follow-up change to the helper's contract automatically applies to both call sites. ## Model Used - Generated with `MiniMax-M3` via `happy5318/hermes-webui` worktree `/tmp/webui-worktrees/pr7565-codex-media`, branch `fix/codex-commentary-media` (off `origin/master` via `ghfast` mirror — `origin` was still TLS recv-failing at PR creation time; the mirror was used for `git fetch` and `git worktree add` only; push goes through the `fork` remote as usual). ## Related - Closes #7565 - Surface code: - `api/media_snapshots.py` (new) `codex_commentary_text` helper - `api/routes.py::_session_media_token_allows_path` (modified to concatenate the helper output) - `api/media_snapshots.py::annotate_media_snapshots` (modified to concatenate the helper output) - Adjacent lanes (intentionally not folded in): - #7294 / #7297 (safe-MIME, session_id, snapshot query parameters) — independent contract - #6828 (persisted-commentary rendering / recovery) — frontend UI; the WebUI auth predicate does not assume or expand its rendering scope - Open PRs #7367 and #6923 — overlap the token-extraction sites but neither implements this commentary ownership rule, per the maintainer's open-PR search - Test surface: new `tests/test_issue7565_codex_commentary_media.py` |
||
|
happy5318
|
3163a91d1a |
# Cron edit/duplicate must round-trip a parseable schedule; bad input returns 400 (#7352)
## Thinking Path
The WebUI populates the editable schedule field from `schedule_display`,
which is the human-readable label `once at 2026-08-28 16:00` for one-shot
jobs. `saveCronForm()` then submits that exact string back to
`POST /api/crons/update`. The Agent's `cron.jobs.parse_schedule()`
rejects the display form with `ValueError` (it only accepts the canonical
ISO timestamp). The route's earlier `try/except ValueError` block
covered input normalization but not the `update_job()` call, so the
parser exception escaped as HTTP 500.
There are two independent failures to fix:
1. **Frontend** ships the unsafe display label as editable input. Both
`openCronEdit()` (`static/panels.js:1593`) and `duplicateCurrentCron()`
(`static/panels.js:1532`) use the same `schedule_display ||
schedule.expression` fallback. The cleanest fix is a shared helper
that returns the canonical Agent-parseable value by schedule kind,
matching the Agent's own schema:
- `once` → `schedule.run_at`
- recurring cron → `schedule.expr` (current schema; `expression`
accepted for legacy payloads)
- interval / legacy → the stored value, with `schedule_display`
kept as a final fallback only when it isn't the `once at …` label.
2. **Backend** converts `ValueError` from `update_job()` into HTTP 400
with the parser message, so the WebUI can show a real validation
toast instead of an opaque Internal Server Error. The 404 path
(job not found) is preserved.
## What Changed
- **`static/panels.js`** — add `_cronScheduleForEdit(job)` helper and
use it in both `openCronEdit()` and `duplicateCurrentCron()`. The
helper returns `schedule.run_at` for one-shot jobs,
`schedule.expr` (or legacy `schedule.expression`) for recurring
cron, and falls back to `schedule_display` only when it is not the
`once at …` presentation form. `saveCronForm()` is unchanged — it
still submits `schEl.value`, which is now the canonical field
instead of the display label.
- **`api/routes.py::_handle_cron_update`** — wrap the `update_job()`
call in `try/except ValueError` and return HTTP 400 with the parser
message. The earlier normalization try block stays untouched; the
404 check (`if not job`) sits outside the new try block so a missing
job still returns 404.
- **`tests/test_cron_edit_round_trip.py`** — new regression module
with seven tests covering the helper contract (helper exists, both
call sites use it, `saveCronForm` still submits the field by name),
the route's 400-on-`ValueError` behavior, the preserved 404 path,
and the happy-path 200.
## Why
- Editing a one-shot cron job's scheduled time has been returning
HTTP 500 since the WebUI's initial public release (per #7352
historical note), because the editable field round-trips the
`once at …` display label straight to the Agent parser.
- The backend regression is independent: even with a correct
frontend, any user-typed invalid schedule text (or a programmatic
caller) used to produce a 500 instead of a validation response.
Both halves are required to deliver a "no more 500" user
experience.
## Verification
```
$ .venv/bin/pytest \
tests/test_cron_edit_round_trip.py \
tests/test_cron_no_agent_edit.py \
tests/test_cron_model_override.py \
tests/test_cron_model_provider_picker.py \
tests/test_issue617_cron_profile_selector.py \
-v --no-header
…
============================== 27 passed in 4.84s ==============================
```
Seven new tests cover both layers; twenty surrounding cron tests
(no-agent edit, model override, model/provider picker, profile
selector) stay green.
## Contract
| Scenario | Before | After |
| --- | --- | --- |
| Open one-shot job for edit, change time, save | 500 (display label round-trips to parser) | 200, persisted with new `run_at` |
| Open recurring cron job for edit | 200 (legacy `schedule.expression` chain) | 200, unchanged canonical field (`schedule.expr` first, `expression` as legacy fallback) |
| Duplicate a one-shot job | 500 on first save | 200, populated with `schedule.run_at` from the source job |
| Submit a `once at …` schedule value (programmatic / pre-fix frontend) | 500 | 400 with the parser message |
| Submit any other unparseable schedule | 500 | 400 with the parser message |
| Update a job that does not exist | 404 | 404 (preserved) |
| Update a job with a valid canonical schedule | 200 | 200 (preserved) |
## Risks
- **Frontend helper change**: I switched the editable field source
for both edit and duplicate in one commit. `saveCronForm()` is
untouched, so anything already rendered with the legacy chain
(rare in practice — the helper is only called from these two
functions) keeps working. The regression `test_save_cron_form_still_submits_schedule_value`
pins the submit field name.
- **New `try/except ValueError` in `_handle_cron_update`**: the
earlier normalization try block (lines 24986–24998) is left
exactly as-is. The new try block sits directly around
`update_job()` only, so a `ValueError` from input parsing still
surfaces before any persistence side effect. The 404 check is
outside the new try, so a missing-job path cannot be misreported
as 400. Both `test_cron_update_preserves_404_when_job_missing` and
`test_cron_update_valid_schedule_still_succeeds` pin those
contracts.
- **Legacy `schedule.expression` payloads**: accepted as a
back-compat fallback in `_cronScheduleForEdit` and in the route
(the Agent schema is `expr`, but the parser still handles
`expression` per the existing contract). If you prefer to drop
the legacy fallback, the helper can be trimmed to `sched.expr`
only.
## Model Used
- Generated with `MiniMax-M3` via `happy5318/hermes-webui` worktree
`/tmp/webui-worktrees/pr7352-cron-edit`, branch
`fix/cron-edit-round-trip` (off `origin/master`).
## Related
- Closes #7352
- Surface code:
- `static/panels.js:1510` `duplicateCurrentCron` (uses helper)
- `static/panels.js:1585` `openCronEdit` (uses helper)
- `static/panels.js:1905` `saveCronForm` (submit path, unchanged)
- `api/routes.py:24979` `_handle_cron_update` (new try/except)
- `cron/jobs.py:785` `parse_schedule` (rejects the display form;
Agent-side, not modified in this PR)
- `cron/jobs.py:2199` `update_job` (the call that raises)
- Test surface: new `tests/test_cron_edit_round_trip.py`
|
||
|
happy5318
|
8841662768 |
fix: close snapshot capture/serve exfiltration holes + prefix-URL corruption (review round 2)
Addresses nesquena-hermes Round-2 review on #6979. All three MUST-FIX items reproduced end-to-end are fixed; three SHOULD-FIX items folded in. MUST-FIX 1 (data exfiltration) — capture/serve deny parity + source binding: - The #3234 state/profile deny model is extracted into a single shared predicate (routes._media_deny_reason) used by BOTH /api/media serving and snapshot capture, so anything the serve path refuses is never captured in the first place (STATE_DIR subdirs, webui_state subdirs, named-profile roots, secret basenames, the snapshot store itself). - Every captured digest now carries a server-owned canonical source-path association (<digest>.src.json sidecar, tmp+rename atomic). /api/media serves a digest only for the exact path it was captured from — replaying a valid digest through a different allowed path falls back to the live file instead of leaking stored bytes. - Snapshot blobs are opened ANCHORED inside the store root (no-follow). - Committed end-to-end regression: capture a denied state file and prove it cannot be served through any path. MUST-FIX 2 (data exfiltration) — configured store root denied regardless of name: - get_snapshot_dir() is resolved and any bare path= target at or below it is rejected BEFORE the allow/serve decision, so a custom HERMES_WEBUI_MEDIA_SNAPSHOT_DIR (e.g. /tmp/custom-store-name) can no longer be read via a plain path= request. MUST-FIX 3 (silent corruption) — prefix-safe URL stamping: - _stampMediaSnapshots now rewrites each COMPLETE path= query value atomically (boundary-aware regex, decode whole value, exact map lookup) instead of substring split/join. One path being a prefix of another (/tmp/a.png vs /tmp/a.png.backup) can no longer corrupt the longer URL or drop its digest. data-path placeholders use the same value-level rewrite with HTML-entity unescaping. Prefix-path regression test committed. SHOULD-FIX folded in: - A recorded digest is FINAL: quota eviction no longer causes a later settle to re-capture the current live bytes and rebind the historical message (immutability restored + no quota-thrash I/O). - is_valid_digest uses whole-string fullmatch (\Z) — a trailing newline can no longer pass the gate. - PDF/HTML fallback, download and open-full-page links now carry snap so they open/download the frozen bytes. Deferred (noted in review thread): emission-time capture (streaming.py) and worklog scene-prose stamping — scene rows do not carry parent-message snapshot metadata; threading it through the scene reconciler is a separate data-pipeline change. Main transcript + transparent ordered segments remain the old/new comparison surface. Tests: +11 backend/frontend regressions (36 total in test_media_message_snapshots.py), incl. the exact Round-2 repros. Reverse-verified: the new tests fail on the round-1 head (8 failed). |
||
|
happy5318
|
1db353a1f0 |
fix: scope snapshot stamping to main+transparent paths (no scene render changes)
- Revert all worklog/scene render-path changes: the scene render chain is exercised by tests that extract function bodies and eval them in isolation (_renderAnchorSceneRowsIntoWorklog, _anchorSceneNodeForRow, etc.); adding helper references breaks those harnesses with ReferenceError. - Snapshot stamping now covers the main transcript path and transparent ordered segments only — the comparison surface for old/new previews. - Transparent segment stamping is inlined in the msg-body template guarded by typeof m, keeping the source-window string intact for test_issue3397 and keeping the eval'd ordered-block self-contained for test_status_command_card. - PDF/HTML lazy loaders keep the exact publicMediaUrl source string the tests assert; snap is appended to mediaUrl only. |
||
|
happy5318
|
e5ae65f510 |
fix: preserve transparent-part source window; clean lint in snapshot tests
- Keep `const partBodyHtml=_getCachedRender(partDisplayText,false);` line intact (test_issue3397 asserts the exact source string), apply snapshot stamping on the following line via partBodyHtmlStamped. - Drop unused roots_before variable and unused import in test_media_message_snapshots.py (ruff F841/F401 on new lines). - Update the source-invariant assertion to the two-line stamp shape. |
||
|
happy5318
|
4eb98889ba |
fix: freeze per-message media previews with content-addressed snapshots
PR #6922 made /api/media revalidate on every use, so overwriting a file in place (same filename) also rewrote every historical chat preview that referenced it — the old/new comparison was lost. At settle time, snapshot each local-file MEDIA: reference into a content-addressed store (<STATE_DIR>/media_snapshots/<sha256>.snap) and stamp the message with _media_snapshots {path: digest}. The frontend appends &snap=<digest> to historical preview URLs; /api/media serves the frozen bytes with immutable caching when the digest is valid, and falls back to the live file otherwise (eviction/never-captured). The existing path allow/deny checks still gate the request — snap only selects which bytes to serve, it never widens access. - api/media_snapshots.py: content-addressed store (sha256 key, dedup, 50MB per-file / 2GB total caps with FIFO eviction), MEDIA ref resolver, capture allow-list mirroring the /api/media roots + #3234 deny list - streaming.py: annotate settled assistant messages (idempotent, best-effort) - routes.py: snap= serving branch + download_name override + deny direct store-path access - models.py: _media_snapshots registered in display-metadata merge keys - ui.js: _stampMediaSnapshots post-cache stamping (main transcript, transparent ordered segments, worklog scene prose) + data-snap threading through the five lazy preview loaders 25 new tests incl. node-executed frontend behavior tests; reverse-verified. |
||
|
happy5318
|
8228dbe4da |
fix: contain ALL post-commit body errors, not just disconnects
The post-commit body-transmission block only caught _CLIENT_DISCONNECT_ERRORS. A non-disconnect error during body write — a generic OSError/EIO from f.read(), or a PermissionError — escaped _serve_file_bytes entirely, and Handler.do_GET (server.py:388-395) caught it and emitted a second status line after the already-committed 200/206, corrupting the HTTP stream. Broaden the post-commit except to swallow every Exception (keeping the disconnect branch for its debug message). Regression tests drive both the snapshot and streaming paths with generic OSError/EIO and PermissionError mid-body, asserting the status sequence is exactly [200] with no trailing 500. |
||
|
happy5318
|
1ff002d170 |
fix: never double-respond after end_headers() in file serving
Move ETag/snapshot computation into a pre-commit try/except OSError so read errors still convert to a clean 500 before any status line is sent. Remove the outer OSError catch around body transmission: once end_headers() returns the response is committed, and a client disconnect (BrokenPipeError et al.) during the snapshot write or fdopen stream would previously have triggered bad() -> send_response(500) as a second status line on a dead stream. Body paths now catch _CLIENT_DISCONNECT_ERRORS (same tuple as _safe_write()), log at debug, and return True. Add regression tests with a fake writer that raises BrokenPipeError after end_headers() for both body paths, asserting exactly one send_response call. |
||
|
happy5318
|
fe34105b22 |
fix: preserve captured snapshot on short-read (round-6)
When a file is truncated between fstat and the snapshot read, we now return (None, data, actual_size) instead of (None, None, actual_size). This preserves the captured bytes so the body path serves the immutable snapshot instead of re-reading the fd after headers are committed. This closes the second-order TOCTOU window: if the file shrinks AGAIN between the first read and body transmission, Content-Length (from actual_size) and body length (from snapshot) can never diverge. Changes: - api/routes.py:18494: return None, data, actual_size (preserve snapshot) - tests: remove unused original_read (ruff F841) - test_shrink_again_during_body_transmission_200: 200 advertised 6 / wrote 6 - test_shrink_again_during_body_transmission_range_206: 206 advertised 4 / wrote 4 |
||
|
happy5318
|
f5ff9b1465 |
fix: short-read safety + actual_size reconciliation for ETag snapshot
Round 5 fix: when a file is truncated between fstat and the snapshot read, Content-Length/Range headers must match the actual bytes read, not the stale fstat size. Changes: 1. Replace os.pread with os.lseek + looped os.read (cross-platform + short-read safe) 2. _etag_and_snapshot now returns 3 values: (etag, snapshot, actual_size) 3. Caller uses actual_size for Content-Length/Range calculations 4. If actual_size != file_size (truncation), etag=None → streaming fallback Regressions: - test_short_read_truncation_uses_actual_size: Content-Length == body length - test_short_read_no_etag_on_truncation: no ETag on truncated snapshot - Updated test_etag_no_body_on_pread_failure for os.read (was os.pread) |
||
|
happy5318
|
a959a4c754 |
fix: add Windows O_BINARY guard for binary file reads
Native Windows opens files in text mode by default, which corrupts binary media files via CRLF translation. O_BINARY prevents this. This completes the portability story — the PR was already correct on all supported platforms (Linux/macOS/WSL2), but the round-3 change was explicitly about portability, so we close the Windows path too. |
||
|
happy5318
|
f7887bfc87 |
fix: clear stale compression state on session switch
When switching sessions in loadSession(), the window._compressionUi state was not cleared, causing phantom 'Compressing context' barriers to appear on fresh sessions that never triggered compression. This complements PR #6572 which handles the case where compressed SSE events are lost/delayed, but does not cover the session switch scenario. Root cause: loadSession() tears down approval/clarify/composer state but missed the compression UI state, which is per-session and must not leak. Fix: Clear compression state immediately after hideClarifyCard() in the loadSession() teardown sequence. |
||
|
happy5318
|
72cab65817 |
fix: pread snapshot + size cap + TOCTOU — address round-2 review (3 issues)
1. _ETAG_SIZE_CAP (10MB): files above skip ETag to avoid hashing large media / Range requests. 2. _etag_and_snapshot: single pread into immutable bytes; the ETag describes the exact snapshot that is sent, eliminating TOCTOU. 3. ETag computation moved inside fd-lifecycle try/finally; pread failures raise OSError caught by the outer except -> 500 + fd closed. 4. no-store content (HTML previews) skips ETag unconditionally. 5. Tests: large-file cap, pread-failure 500+fd-closed, TOCTOU snapshot correctness. Related tests: 20/20 targeted + 33 neighboring passed. |
||
|
happy5318
|
650159d11f |
fix: address review — content-digest ETag, fd cleanup, 304 + cache policy
- _file_etag streams a SHA-256 digest through the already-authorized fd instead of using mtime+size metadata, so same-size in-place replacements on coarse-mtime filesystems (2s granularity) produce a new ETag — the exact stale-cache bug this feature sets out to fix. fd is rewound before the normal/Range send. - close fd on the 304 early-return path (was leaking a descriptor per revalidated hit) - drop Content-Length: 0 from 304 responses (RFC 9110 §15.4.5) - non-HTML /api/media cache policy is now "private, no-cache" so shared intermediary caches cannot store per-user/per-session media - regression test: equal-size + equal-mtime (ns-pinned) replacement must change the ETag and return 200, not a stale 304 |
||
|
happy5318
|
b59a9cadd1 |
fix: ETag revalidation for /api/media so in-place file updates show immediately
/api/media served every file type with Cache-Control: private, max-age=3600, so the browser reused a cached response for 1 hour without revalidating. When an agent overwrites a file in place (same name, new content — e.g. a regenerated image), inline previews kept showing the stale version until a manual full-page refresh. Changes: - _serve_file_bytes now computes a weak ETag from file metadata (mtime_ns + size) and honors If-None-Match with a 304 short-circuit (RFC 7232 weak comparison, W/ prefixes ignored, '*' support), processed before the Range block since a matched conditional request skips the entity entirely. 200/206 responses carry the ETag. - _handle_media splits the cache strategy by MIME: text/html keeps no-store (inline previews change constantly, PR #6706); all other media (images, audio, video, PDF) switch from max-age=3600 to no-cache so the browser MAY cache but MUST revalidate on every use — in-place updates are picked up immediately while unchanged files still short-circuit with 304. Why weak ETag: metadata-based equality is sufficient for the overwrite case and avoids hashing large files per request. Why no-cache over must-revalidate: no-cache forces revalidation before every use regardless of freshness; must-revalidate only fires after max-age elapses, which does not help when the browser believes its cached copy is still fresh. Tests: tests/test_media_etag_revalidation.py (15 tests) covering ETag emission, 304 short-circuit (match/star/mismatch/list/weak-comparison), ETag change on in-place replacement, Range+ETag interaction, 304-before- Range precedence, and the _handle_media end-to-end path (no-cache for images/PDF, no-store preserved for HTML). |
||
|
|
cdc50e232a |
fix(streaming): route image input mode by the session's actual provider/model (#6882)
* fix(streaming): route image input mode by the session's actual provider/model Image routing (native embed vs vision_analyze text pipeline) previously resolved the provider/model from the global config default only. A session switched to a text-only model (e.g. deepseek-v4-flash) still had uploaded images forwarded natively to a model that cannot see them. Thread the session's resolved provider/model (active_provider/active_model) and its pre-canonicalization identity (requested_provider) through _resolve_image_input_mode, _build_native_multimodal_message, _sanitize_messages_for_api, and both gateway chat paths. When no session identity is present the behaviour is unchanged (global default). Tests: upstream parity, session-level native/text routing, requested_provider capability lookup, and multimodal/sanitize forwarding. * test: use generic provider/model names in fixtures and docstrings Replace real provider/model names (custom:newapi, jd-kimi-k2.6, stepfunplan, step-3.7-flash, deepseek-v4-flash) with neutral placeholders (mygateway, my-vision-model, my-text-model, my-vision) so the test suite does not reference any particular deployment's configuration. test_active_vision_model_routes_native now passes requested_provider to keep the assertion independent of models.dev capability data. * fix(streaming): preserve session provider identity across auth-heal retry Both credential-healing retry branches unconditionally overwrote _session_requested_provider with the already-canonicalized resolved_provider, so a named custom:slug session lost its exact vision-capability entry after an auth-heal retry -- a text-only custom model would then wrongly RESTORE the historical image_url parts it correctly stripped on the first attempt. Make the heal-path assignment conditional (initialize only when empty), preserving the pre-canonicalization identity captured at first resolve. Regression test asserts both arms: identity preserved -> images stripped; identity lost -> images kept (the bug). Addresses Codex gate CORE finding + greptile P1 on #6882. Co-authored-by: happy5318 <happy5318@users.noreply.github.com> * test(vision-routing): skip capability-based tests when hermes-agent absent The capability-derived 'text' verdict delegates to agent.image_routing (the single source of truth), which is NOT installed in the WebUI standalone CI environment -- there _resolve_image_input_mode falls back to native, so the 7 tests asserting a capability-based text verdict failed on CI (they only ever passed locally where the agent package is on PYTHONPATH). Guard them with a module-level agent-availability probe + skipif (the same pytest.importorskip pattern used across the suite for agent-dependent tests): they run fully where the agent is present and skip honestly where it is not. Agent-free tests (native carve-out, explicit-signal fallback) still run on CI. * test(vision-routing): drop unused imports (base64, os, TemporaryDirectory) Pre-existing F401 unused-imports that ruff's diff-vs-master flagged once the import block entered the changed-line scope. Path is retained (used in the _make_png / _normalized_attachments type annotations). --------- Co-authored-by: happy5318 <happy5318@users.noreply.github.com> Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com> |
||
|
|
49437e20ab |
fix: phantom 'Compressing context' barrier when compressed SSE is lost/delayed (#6572)
* fix: clear stale compression barrier on turn-done when compressed SSE is lost When a turn completes (done SSE event) but the "compressed" SSE event was lost or delayed, the compression UI state remained in phase="running". The done handler unconditionally rebound the stale state sessionId to the continuation session, surfacing a phantom "Compressing context" barrier on a session that never actually compressed. Fix: when the done event session_id differs from the active session AND the compression UI is still in "running" phase, clear it via clearCompressionUi() instead of rebinding. Non-running states (e.g. "done") are still rebound as before. Added regression tests verifying: - stale running state is cleared (not rebound) on session change - non-running states are still rebound - the old unconditional rebind is gone * fix: also clear phantom compression on same-session done (A->A case) Reviewer feedback on PR #6572: the previous fix only cleared stale running compression when the session ID changed (A->B). But the backend also supports compression without session rotation (A->A), where the compressed SSE being lost leaves the same phantom barrier. Fix: in the done handler, clear any owned automatic compression state that is still in "running" phase, regardless of whether the session ID changed. The condition is now phase==='running' alone, not session_id change AND running. Added behavioral state-transition tests for all four cases: - running A->B (rotation, compressed lost): cleared - running A->A (no rotation, compressed lost): cleared - non-running A->B (rotation, compressed seen): rebound - non-running A->A (no rotation, compressed seen): rebound * test: real SSE regression for compression phantom barrier Rewrite test_compression_phantom_barrier.py to use real browser + SSE instead of handwritten Python simulation. The new tests: - Boot real WebUI server with isolated state - Use deterministic mock gateway (Runs API mode) - Verify done handler clears stale running compression state - Cover both A→B (rotation) and A→A (no rotation) cases Key insight: /v1/capabilities must return run_approval_response=true to enable the Runs API path in gateway_chat.py. Fixes reviewer feedback: simulation never called attachLiveStream, clearCompressionUi, or registered done listeners. * test: replace Playwright test with hermetic source-grep suite for compression phantom barrier The previous Playwright-based test failed CI in no-egress sandboxes due to external resource errors (net::ERR_INTERNET_DISCONNECTED) and was structurally unsound: the mock window.S shadowed the lexical S, so the compressing handler short-circuited before the barrier could be established, producing a false- positive assertion. New suite uses static source analysis (same convention as test_issue840, test_issue3040, test_bg_task_complete_focus_drop): - No browser, no network, no playwright dependency - Proves the done handler clears phase:'running' without gating on session ID change (A->A case coverage) - Asserts gating conditions: automatic && owner && done session exists - Negative controls: non-owner / manual compression are ignored - Source-level regression proof: reverting the production hunk breaks tests 13 tests. All pass in <3s on bare Python. * test: exercise compression cleanup through real SSE handlers Replace source-grep assertions with a hermetic Playwright harness that loads the shipping classic scripts and dispatches MockEventSource events through attachLiveStream. Cover A-to-A/A-to-B cleanup, non-running rebind, ownership controls, canonical cleanup side effects, and a production-branch mutation proof. --------- Co-authored-by: happy5318 <happy5318@users.noreply.github.com> Co-authored-by: nesquena-hermes <nesquena+hermes@gmail.com> |
||
|
happy5318
|
5cda58fc9b |
fix: match active turn by exact identity (token/epsilon), not 1.5s proximity
Review round 2: the 1.5s timestamp tolerance over-deduped a legitimate rapid repeat — two identical-text turns <1.5s apart collapsed into one, hiding the second turn and copying its attachments onto the earlier row (the mirror risk of #6649). _pendingActiveTurnUserMessage now matches only on unambiguous identity: - the row carries the server-stamped _active_turn_token (stream_id + started_at, per build_active_turn_token), or - its timestamp equals pending_started_at within a precision-only epsilon (1e-6, absorbs float/state.db drift, never a full second). Anything wider (whole-second truncation, ~1s rapid repeat) returns null so getPendingSessionMessage() materializes the pending turn — fail toward the harmless transient duplicate the settle render clears. Regressions added: two completed identical-text turns ~1s apart → second pending row returned + first row's attachments untouched; token-identity row adopted even when its timestamp is outside the epsilon. |
||
|
happy5318
|
22b155e8d0 |
test: strengthen HTML preview cache regression coverage
Per review feedback:
- Frontend: assert exact fetch(mediaUrl, {cache:'no-store'}) in
loadHtmlInline body, not just 'fetch(mediaUrl' prefix (too broad,
other fetch calls match the prefix)
- Backend: assert Cache-Control: no-store on both attachment and inline
HTML responses in test_html_media_endpoint_inline_requires_csp_sandbox
- Both assertions fail independently if either the frontend no-store
option or backend no-store policy is reverted
|
||
|
happy5318
|
5f5ebd259e | test: update fetch assertion for no-store cache option | ||
|
happy5318
|
bb5e778154 |
fix: HTML inline preview shows stale content after file update
The /api/media endpoint served all file types with Cache-Control:
private, max-age=3600. For HTML inline previews (rendered via
loadHtmlInline() → fetch() → iframe srcdoc), this meant the browser
cached the response for 1 hour. When an agent edited and re-rendered
the same HTML file, the inline preview showed stale content — only a
manual full-page refresh revealed the update.
Two changes:
- Backend: serve text/html with no-store instead of max-age=3600
(images and other media keep the 1-hour cache)
- Frontend: loadHtmlInline() fetch with {cache:'no-store'} for
belt-and-suspenders defense against intermediate caches
Note: the /api/file/raw path already used no-store for HTML; this
fixes the /api/media path which was missed.
|
||
|
happy5318
|
55af555846 |
fix: do not duplicate the active turn's user row on mid-run reload
While a turn is live the server can reconcile that turn's user row into the transcript returned by /api/session (the sidecar/state.db merge picks it up from state.db, where the agent writes it immediately) while pending_user_message is still set. The row is then followed by the same turn's assistant/tool rows. _pendingCurrentTailUserMessage() scans the tail backwards and returns null as soon as it meets a completed assistant, so in that state it reports "the current turn has no user row". getPendingSessionMessage() then materializes pending_user_message a second time and the pane renders two identical user bubbles until the settle render replaces the list wholesale (which is why the duplicate disappears when the answer lands, and why nothing wrong is ever persisted). Scanning past assistant/tool rows on text alone would regress #3300: a user who submits the same text twice in a row legitimately gets two identical user turns. Use pending_started_at as the discriminator instead — the server stamps the active turn's user row with it, and no earlier turn can share it. Text equality is still required, so a false match needs identical text AND a near-identical timestamp. Absent/invalid pending_started_at disables the fallback and keeps the previous strict-tail behaviour. Verified: the new midRunReloadDedupe assertion fails on the unpatched ui.js and passes with the fix; the existing repeat-prompt, compaction-boundary and strict-tail assertions still pass. |
||
|
happy5318
|
f52be26610 |
docs: update comments to reflect preflight exclusion reasoning
Clarify that preflight compression is intentionally excluded because the later authoritative 'Compacting context' marker from conversation_compression is the real signal that compression proceeded. The preflight status can fire even when compression exits before compaction (e.g. durable guard refresh, Codex-Hermes mode with no active thread). |
||
|
happy5318
|
9c1c1d8063 |
fix: stop false Compressing-context card for preflight compression
PR #6184 narrowed the agent-status-to-SSE bridge to real compression start notices, but the 'preflight compression:' pattern matches the Hermes agent's preflight check message ("Preflight compression: ~X tokens >= Y threshold"). This fires on every new session where the system prompt alone exceeds the threshold, showing a "Compressing context" card before any model output — a false positive. Remove 'preflight compression:' from the pattern. The preflight check is a one-time pre-model-call estimate, not a mid-conversation compression event that the user needs to see. |
||
|
happy5318
|
a57d576805 |
fix: make prefixed Qwen detection positive-only to avoid shadowing hybrid IDs
Gate review found that the broadened token-prefix check caused
deepseek-r1-distill-qwen2.5-bakeneko-32b to enter the Qwen branch
(qwen2 token), return False, and prevent the DeepSeek detector from
running — a silent regression.
Fix: split into two phases:
1. Positive-only scan: any token matching qwen(\d+) with major >= 3
immediately returns True. No False path — Qwen 2.x in hybrid IDs
falls through to subsequent detectors.
2. Terminal guard (original behavior): standalone "qwen" token or
normalized.startswith("qwen") still applies the 3+ gate with
return False for 2.x, since those ARE pure Qwen models.
Add regression test for deepseek-r1-distill-qwen2.5-bakeneko-32b.
|
||
|
happy5318
|
90250adfb7 |
fix: address review — remove redundant guard, add regression tests
- Drop the now-subsumed `normalized.startswith("qwen")` guard
(token-prefix matching already covers the bare case)
- Add test_qwen_prefixed_alias_reasoning_detection covering:
prefixed Qwen 3+ (al-/sn-), bare Qwen 3+, and Qwen 2.x exclusion
|
||
|
happy5318
|
041e311f1e |
fix: Qwen reasoning heuristic fails for prefixed model IDs
The Qwen branch in _candidate_supports_reasoning() checks
"qwen" in token_set, but model IDs like "al-qwen3.8-max-preview"
(New-API alias prefixes) normalize to tokens ["al", "qwen3", "8", ...]
where "qwen" is not a standalone token. This causes the reasoning
effort toggle to disappear for any prefixed Qwen model.
Switch to token-prefix matching (token.startswith("qwen")), consistent
with the position-independent approach already used for DeepSeek
(tokens.index). This handles arbitrary vendor/alias prefixes while
still correctly excluding Qwen 2.x via the existing version gate.
|
||
|
happy5318andhappy5318
|
76690c66e5 |
fix: deduplicate configured model badges (#6221)
Deduplicate configured model badges so one configured model shows a single picker entry, with provider-collision + colon-bearing-id routing correctness. Thanks @happy5318. Co-authored-by: happy5318 <happy5318@users.noreply.github.com> |
||
|
happy5318
|
2892d3d6c0 |
fix: also filter GH_TOKEN env var from Copilot credential pool
Follow-up to PR #3382 review feedback. There are three GitHub-related env vars that can be auto-detected: - COPILOT_GITHUB_TOKEN: user-specific config, should NOT be filtered - GITHUB_TOKEN: classic PAT, should be filtered (already done) - GH_TOKEN: another env var, should also be filtered (this commit) Use a frozenset (_AMBIENT_GH_ENV_SOURCES) to clearly define which env var sources are ambient vs user-configured. |
||
|
happy5318
|
02a1849ca4 |
fix: filter GITHUB_TOKEN env var from Copilot credential pool
Copilot API does not support classic PATs (ghp_*), only OAuth tokens (gho_*) or fine-grained PATs (github_pat_*). When GITHUB_TOKEN env var contains a classic PAT, it should be filtered from the credential pool to prevent Copilot from appearing in the model picker dropdown. The _is_ambient_gh_cli_entry() function now also filters entries with source='env:GITHUB_TOKEN', treating them as ambient/auto-detected rather than user-explicitly-added credentials. Fixes issue where Copilot group appeared in WebUI model picker even when gh auth token was an unsupported classic PAT. |
||
|
happy5318
|
a6599cd68e |
fix: show same model from different custom providers instead of deduplicating
When multiple custom providers expose the same model ID (e.g. baidu,
huoshan, and liantong all offering glm-5.1), only the first provider's
entry was shown in the model dropdown.
Root cause (backend): used the bare model ID as the
dedup key, so the second and subsequent providers with the same model
were silently skipped.
Root cause (frontend): stripped the @provider: prefix before
comparing, so @custom:baidu:glm-5.1 and @custom:huoshan:glm-5.1 were
treated as duplicates.
Fix:
- Backend: change _seen_custom_ids key to '{slug}:{model_id}' so each
provider's models are tracked independently.
- Frontend: add _providerOf() helper and deduplicate on the composite
(normId, provider) key instead of normId alone. Bare model IDs
(without @provider: prefix) still deduplicate on normId for backward
compatibility.
|
||
|
happy5318
|
3f23431bb7 |
Fix: add TCP keepalive to prevent CLOSE-WAIT zombie connections (v2)
- Add server_bind() to QuietHTTPServer with SO_REUSEADDR and TCP keepalive - Add setup() to Handler for per-connection aggressive keepalive - Server level: 60s idle, 10s interval, 3 probes = 90s detection - Connection level: 10s idle, 5s interval, 3 probes = 25s detection - Prevents zombie connections from blocking API on long-running servers - Cross-platform safe with try/except for platforms without TCP_KEEP* constants Fixes #1580 |
||
|
happy5318
|
29a23115bc |
Fix _normalizeConfiguredModelKey in frontend to match backend behavior
The JavaScript _normalizeConfiguredModelKey function had the same bug as the
Python _norm_model_id function that was fixed in commit
|
||
|
happy5318
|
d6164cdadb |
Fix _norm_model_id to properly strip provider prefixes
The _norm_model_id function was using split(':', 1)[1] which only removed
the first colon-separated segment, leaving provider names in the normalized
model ID. For example, '@custom:jingdong:GLM-5' became 'jingdong:glm.5'
instead of 'glm.5'.
This caused the default model injection check to fail, resulting in a
duplicate 'Default' group being added to the model list even when the
model already existed with a provider prefix.
Changes:
- Use split(':')[-1] to get the last segment after all colons
- Use split('/')[-1] consistently for slash-separated paths
- Replace local _norm lambda with _norm_model_id function call
Fixes duplicate Default group appearing in model dropdown when using
custom providers with @provider:model ID format.
|
||
|
happy5318
|
cc45175ee5 |
docs: add thread safety comment for SESSION_AGENT_CACHE
All LRU cache operations (get, set, move_to_end, popitem) are already protected by SESSION_AGENT_CACHE_LOCK. This addresses the reviewer's concern about thread safety in multi-threaded ASGI servers. |
||
|
happy5318
|
65e5690772 |
fix: add LRU limit to SESSION_AGENT_CACHE to prevent memory bloat
The agent cache stores full AIAgent instances (each holding complete conversation history) without size limit. Long-running servers with many sessions can accumulate unbounded memory usage. Changes: - Replace dict with OrderedDict for LRU tracking - Add SESSION_AGENT_CACHE_MAX = 50 limit - Evict least-recently-used entries when cache exceeds limit - Call move_to_end() on cache hits to maintain LRU order This prevents memory exhaustion on servers with many active sessions. |