Closes#7622.
Round-3 follow-up to the maintainer review at
https://github.com/nesquena/hermes-webui/pull/7730#issuecomment-5786493511.
The round-2 'primary === "en"' heuristic overrode users who had
genuinely picked English: when the server-stored 'language' was 'en'
the resolver treated it as the schema default and skipped it, falling
through to localStorage / the browser hint. This round addresses all
three maintainer findings:
1. BRICK — server stops injecting the schema default. Drop
'"language": "en"' from api/config.py:_SETTINGS_DEFAULTS so a
fresh install returns settings['language'] == None. Existing
settings.json files that already carry 'language: "en"' still
report 'en' (the field is just absent from the defaults dict; the
per-file load path is unchanged). 'language' is added back to
_SETTINGS_ALLOWED_KEYS so the Settings modal save path still
validates and persists explicit user picks (the BCP-47 regex is
unchanged).
2. CORE — browser-language detection is now in one guarded helper.
static/i18n.js exposes _detectBrowserLanguageHint(), used by
loadLocale(), boot.js, and panels.js. A throwing navigator
accessor in some embedded webviews can no longer abort boot or
settings hydration before the model/provider/plugin/extension
sections are populated.
3. CI RED — restore the test shape. boot.js and panels.js call
resolvePreferredLocale(s.language, localStorage.getItem('hermes-lang'),
_detectBrowserLanguageHint()) inline so
tests/test_language_precedence.py::test_boot_and_settings_panel_use_shared_locale_precedence
matches the expected argument list. The TestComposedResolverPrecedence
tests in tests/test_issue7622_initial_locale_from_browser.py are
re-pinned to the round-3 contract (server primary is None for a
fresh install; an explicit 'en' primary from a user-saved file
beats a non-English browser hint).
Tests (22/22 i18n-related pass; 833/833 settings+i18n+locale pass; ruff
forward-gate --diff origin/master is clean):
- test_language_precedence.py: 8/8 (added guarded-helper, server-default,
composed-resolver, and cross-file first-visit tests)
- test_issue7622_initial_locale_from_browser.py: 14/14 (re-pinned to
round-3 contract; added explicit-saved-English regression test)
- test_login_locale.py and friends: unchanged behaviour for the
save/load round trip
Gate fixes on top of @rodrigogs's #6658, both independently reproduced first.
do_GET called bare check_auth while every write path used check_auth_or_close,
so a body-bearing GET failing top-level auth left its bytes queued on a
keep-alive socket. check_auth_or_close is already body-pending-aware, so the
bodyless login-redirect flow keeps its keep-alive. Removing the now-unused
check_auth import (ruff F401) broke 9 tests that monkeypatch that symbol
through a module alias; rewritten to the new name.
_declared_content_lengths ran isdigit() across the whole raw header value, so
a comma-combined 'Content-Length: 0, 0' -- the same bodyless request as two
repeated headers under RFC 9110 5.3 -- read as unreadable and over-closed a
healthy connection. Now splits on commas before validating each member;
disagreeing lists still fail closed.
Verified: 16/16 adversarial CL vectors still fail closed (no fail-open
reopened), 13/13 over-close cases fixed, live-socket drive shows close armed
on a body-bearing reject and keep-alive preserved on a bodyless one, and a
chunked request behaves byte-identically to master (master never supported
chunked either, so no proxy deployment regresses).
Co-authored-by: rodrigogs <rodrigogs@users.noreply.github.com>
The mirror image of the sidecar over-close already fixed here: the sibling
reject-before-read sites still armed `arm_connection_close()` unconditionally,
so a request with NO declared body was answered with `Connection: close` and
the client's pipelined follow-up was dropped -- a healthy pooled connection
killed for no framing reason. Reproduced on the wire, pipelined down one
socket against the production handler (`responses seen: 1` at each site):
POST /api/session/new off-session auth -> 401 + Connection: close
POST /api/session/new bad Origin -> 403 + Connection: close
POST /api/session/new no CSRF token -> 403 + Connection: close
POST /api/csp-report limiter tripped -> 204 + Connection: close
POST /api/health/restart success -> 200 + Connection: close
POST /api/process-complete-ack -> 410 + Connection: close
POST /api/upload no boundary -> 400 + Connection: close
Each now routes through `arm_connection_close_if_body_pending()` (the helper
the sidecar path already used), so the framing decides in both directions:
body-bearing or unreadable framing still closes, an absent or agreeing-zero
Content-Length keeps keep-alive. `check_auth_or_close()` cannot swap the helper
in directly -- it has to arm before `check_auth()` writes its 401/302 and undo
that on success -- so it gates the arming on `request_declares_body()` instead
and keeps the success-path restore.
`/api/health/restart` is armed once, ahead of the outcome, so all four results
(completed / in_progress / busy / error) follow the framing rather than the
result; the success path was closing on every call the WebUI makes.
Only the boundary check in the upload preflight can reject a bodyless request:
every other raise there (any Transfer-Encoding, an unreadable or conflicting
length, an over-cap length) already declares a body and still closes.
Five tests pinned the unconditional close on stubs that declared no body --
i.e. they pinned the bug. Each is re-grounded on real body-bearing framing
(Content-Length, chunked, and an unreadable length) so the close half stays
pinned, with a bodyless sibling asserting keep-alive. New real-socket
regressions cover all seven sites in both directions.
`identity` was exempt as a coding that "frames nothing", so a request
carrying it read back as body-less however many payload bytes followed.
It frames nothing either: RFC 9112 6.3 says a request whose final
transfer coding is not `chunked` has a body length no recipient can
determine, RFC 7230 removed `identity` from the transfer codings
outright, and `http.server` decodes none of them (`rfile` is the raw
socket). Every present value has to reject-and-close.
Reproduced on 8c326003b, pipelined down one socket:
GET /api/extensions/probe/sidecar/ping `Transfer-Encoding: identity`
{"stale": true}
-> HTTP/1.1 403 Forbidden (no Connection: close)
-> HTTP/1.1 400 Bad request syntax
('{"stale": true}GET /api/health/agent HTTP/1.1')
POST /api/upload (and /upload/extract, /workspace/upload, /transcribe)
-> HTTP/1.1 400 Bad Request ("No file field in request", no close)
-> HTTP/1.1 400 Bad request syntax ('--x')
Deletes the `_UNFRAMED_TRANSFER_CODINGS` allowlist and the fold-safety
test that existed only to guard its membership check; `_BLANK_TRANSFER_CODING`
stays, since a present-but-empty header still needs a named, non-falsy
report. The identity rows of the framing tables and of
`test_bodyless_framing_keeps_the_pooled_socket_alive` move to the
closes-the-socket side, with real-socket coverage on the sidecar path and
all four upload endpoints. The keep-alive half stays pinned by the cases
that genuinely have no body: no framing header at all, `Content-Length: 0`,
an OWS-padded zero, two agreeing zeroes.
Swept the rest of the coding surface for the same class: every case
spelling, OWS and non-OWS padding, `identity, chunked`, `chunked, identity`,
`identity,identity`, a lone comma, an unknown coding, duplicated headers,
and a coding beside both a real and a zero Content-Length.
`_declared_content_lengths()` parsed with `int()`, which is far looser than RFC
9110's `Content-Length = 1*DIGIT`: it accepts a leading sign, PEP 515
underscores and non-ASCII digits, and `str.strip()` ahead of it also erased
non-ASCII whitespace. Every such spelling of ZERO therefore parsed to an honest
`0`, took the "every declared length agrees on zero" keep-alive branch, and left
the payload in rfile for the server to parse as the next request line -- the
same defect the blank value had, one layer down. Reproduced live at the previous
head, pipelined down one socket:
GET /api/extensions/probe/sidecar/ping `Content-Length: +0` {"stale": true}
-> HTTP/1.1 403 Forbidden (no Connection: close)
-> HTTP/1.1 400 Bad request syntax
('{"stale": true}GET /api/health/agent HTTP/1.1')
byte-for-byte the trace greptile filed the P1 for, and identical for `-0`,
`0_0`, `+00`, `0_0_0`, `+0_0`, `\xa00`, `0\xa0` and `\x850`. On /api/upload the
same values slipped `unreadable_content_length()` into `int('0_0')` == 0 and
answered 400 keep-alive then `400 Bad request syntax ('--x')`.
A sign was reported safe last round because it was probed as `+15`, where a
wrong parse closes anyway; only the zero-valued spellings expose it.
The parse is now the grammar: an unsigned run of ASCII digits, or the framing is
unreadable and a body is assumed pending. The `int()` guard stays -- a digit run
longer than `sys.get_int_max_str_digits()` (4300) raises ValueError too, and a
raise on a rejection path becomes a spurious 500.
`Transfer-Encoding` gets the same audit. Trimming is now SP/HTAB only, RFC 9110
OWS and nothing else, because `str.strip()` erased U+00A0/U+0085 there too:
`Transfer-Encoding: \xa0identity` -- a spelling no token grammar allows -- read
back as the `identity` token, so it counted as unframed and poisoned the socket
with the same 403-then-400 trace (411 and close on uploads). Both characters
reach the wire, since a request line decodes as latin-1. Membership in
`_UNFRAMED_TRANSFER_CODINGS` is tested through `str.lower()`, which maps some
non-ASCII characters ONTO ASCII ones (U+212A -> `k`), so the set is now
documented as ASCII-token-only and a test scans every codepoint to prove no
non-ASCII spelling can reach it.
Non-ASCII digits are unreadable by the same rule and pinned at the helper level:
U+0660/U+FF10 cannot cross an HTTP/1.1 header line (latin-1 leaves only the
U+00B2/B3/B9 superscripts, which `int()` already rejects), but the helpers take
any headers mapping and enforce the grammar rather than trusting the transport.
The no-body contract is unchanged and pinned in both directions on the wire and
at the helper: no length header, `0`, `00`, an OWS-padded zero, two AGREEING
zeroes, and `identity` in any ASCII case still keep the pooled connection alive.
`Content-Length:` with nothing after the colon reads back as '' from
`Message.get_all()`, and `_declared_content_lengths()` SKIPPED the empty value.
The result was "no length declared", so `request_declares_body()` reported no
pending body and the sidecar provenance rejection answered 403 with keep-alive
intact -- leaving the payload in rfile for the server to parse as the next
request line. Reproduced live, pipelined down one socket:
GET /api/extensions/probe/sidecar/ping `Content-Length:` {"stale": true}
-> HTTP/1.1 403 Forbidden (no Connection: close)
-> HTTP/1.1 400 Bad request syntax
('{"stale": true}GET /api/health/agent HTTP/1.1')
A blank value cannot prove a body's absence, so it now joins `banana` and two
disagreeing lengths as unreadable framing: bytes are assumed pending and the
connection closes. Absence is something only the wire can say, and an absent
header already yields no values at all.
Three adjacent holes of the same class were found by probing and are fixed by
the same rule (each reproduced live with the same bad-syntax 400):
* whitespace-only `Content-Length: ` -- `Message` collapses it to the same '',
as it does a duplicated blank or a blank paired with a real length.
* the four multipart upload handlers: a blank length slipped past
`unreadable_content_length()`, reached `int('' or 0)` == 0, and answered 400
"No file field in request" with the payload still queued. Now 400 "Invalid
Content-Length" with the connection closed.
* blank `Transfer-Encoding:` -- '' sat in `_UNFRAMED_TRANSFER_CODINGS` beside
`identity`, so a header naming no coding read as "nothing to frame". A blank
coding is undecodable, not unframed; uploads answer 411 and close.
The no-body contract is unchanged and pinned in both directions: no length
header, a padded zero, two AGREEING zeroes and `identity` all still keep the
pooled connection alive.
Adversarially probing the re-gated head found the same poisoning reachable by
DUPLICATING a framing header instead of omitting one. `Message.get()` returns
only the first occurrence, so `Content-Length: 0` sent ahead of the real length
made the body invisible to every reader: nothing was drained and the real bytes
were parsed as the next request line. Reproduced live on the otherwise-fixed
head, over one pooled socket:
HTTP/1.1 403 Forbidden <- sidecar provenance reject
HTTP/1.1 400 Bad request syntax ('{"stale": true}GET /api/health/agent ...')
HTTP/1.1 400 Bad Request <- /api/upload reject
HTTP/1.1 400 Bad request syntax ('--x')
api/helpers.py now reads framing through `_framing_header_values()`, which
returns every value the wire carried. `request_declares_body()` treats
disagreeing or unparseable lengths as a declared body (they cannot prove the
body's absence), and `unsupported_transfer_encoding()` inspects every declared
coding so `identity` cannot mask a `chunked` behind it. Two agreeing zeroes are
still no body -- repetition alone is not a conflict.
New `unreadable_content_length()` gates the multipart preflight: with no honest
byte count there is nothing to drain, so all four handlers refuse with 400
"Invalid Content-Length" before touching rfile, connection armed for close.
13 regressions, each verified red without this commit: the helper predicates on
a real email.message.Message, the production sidecar 403, all four multipart
handlers, and five pipelined real-socket tests proving the follow-up request is
never answered off a poisoned socket.
Two framing gaps the re-gate reproduced on the production handlers, both the
same class: something other than the wire was used to decide whether unread
body bytes are queued.
1. api/routes.py — the sidecar provenance 403 gated its close on
`read_request_body`, a static per-method flag. That is wrong in both
directions: a GET carrying `Content-Length`/`Transfer-Encoding` was answered
with keep-alive and its body then parsed as the next request line
(`501 Unsupported method ('{}GET')`), while a body-less DELETE/PUT/PATCH had
a healthy pooled connection closed for nothing.
2. api/upload.py — all four multipart handlers read framing from
`Content-Length` alone, so a chunked upload (which has none) was treated as
length zero, matched no parts and was rejected 400 "No file field in
request" with keep-alive intact. Wire trace before the fix:
HTTP/1.1 400 Bad Request <- the upload rejection
HTTP/1.1 400 Bad request syntax ('54') <- our own chunk-size line
New primitive in api/helpers.py: `request_declares_body()` (Content-Length
present and non-zero, or any Transfer-Encoding other than identity; an
unparseable length counts as present, since it cannot prove absence) plus
`arm_connection_close_if_body_pending()` and `unsupported_transfer_encoding()`.
api/upload.py now routes all four handlers through one preflight,
`_read_multipart_or_reject()`, which refuses an undecodable transfer coding
with 411 before touching rfile — http.server never decodes chunked framing, so
that request can only be rejected — and keeps every previous status and message
(413 oversize, 400 invalid Content-Length/boundary) with its close.
Regressions: chunked reject that closes and a body-less reject that does not,
at all four upload handlers and at the sidecar 403, plus two pipelined
real-socket tests against the running server proving the follow-up request is
never answered off a poisoned socket.
advertise_connection_close() guarded its two attribute reads but not the
dedup scan over _headers_buffer. A stub whose buffer is truthy and
non-iterable (a bare Mock) raised TypeError from inside end_headers(),
which _handle_write's generic except would swallow into a spurious 500 --
the same failure class as the missing close_connection guard.
Replaces the inline `handler.close_connection = True` at each reject-before-read
site with api.helpers.arm_connection_close(), the shared mechanism the review
asked for. Behaviour is unchanged at every real handler; the helper adds the
same stub tolerance the read side just got, so a handler that refuses the
attribute can never turn a 4xx into a 500.
Sites: _check_csrf (origin + token), the sidecar provenance 403 (body-reading
calls only), the rate-limited CSP report, /api/health/restart, the deprecated
process-complete-ack 410, and all four upload handlers' pre-read
length/boundary rejections.
The first cut of the close advertisement had two defects, both caught by CI
(9 red jobs) and by the deep-review gate's own MUST-FIX 1 wording.
1. Dedup was keyed off a private `_close_advertised` flag that end_headers()
set itself. Every SSE endpoint already sends its own `Connection: close`,
and BaseHTTPRequestHandler.send_header flips close_connection when it does
— so end_headers() appended a SECOND copy to every stream response.
Dedup now derives from the pending header buffer, which sees those
site-level headers too.
2. `self.close_connection` was read unguarded. The attribute only exists once
a request line has been parsed, so the many suite handlers built with
`Handler.__new__(Handler)` raised AttributeError instead of emitting
headers — and in _handle_write that AttributeError became a spurious 500.
Both reads are now defensive, and check_auth_or_close() no longer invents
the attribute on a handler that never had it (which would have left a bogus
keep-alive verdict behind).
The mechanism moves to api/helpers.advertise_connection_close(), which also
puts server.py back under the original 750-line guard, so the test's ceiling
bump to 760 is reverted rather than shipped.
Deep-review gate (2026-08-14) findings, all reproduced:
1. close_connection was never serialized: BaseHTTPRequestHandler does not
emit Connection: close from the flag, so pooled clients kept reusing the
socket and died with BrokenPipeError. Handler.end_headers() now emits
the header (deduped) whenever close_connection is armed.
2. check_auth_or_close armed the flag too late (after the 401/302 was
already flushed); it now arms before check_auth() and restores the
prior flag on success.
3. The sidecar provenance rejection closed body-less GETs too; the close
is now gated on read_request_body=True.
4. Remaining reject-before-read sites now close-and-advertise through the
same mechanism: rate-limited CSP reports (204), the deprecated
/api/process-complete-ack (410), /api/health/restart (all outcomes),
and the four upload handlers (413 + pre-read Content-Length/boundary
validation via _reject_before_read).
Tests: pooled-client regression against the production handler (reject
with a body, then follow-up on the same pooled connection succeeds and the
rejection carries Connection: close); unit coverage for the sidecar GET
keep-alive, CSP rate-limit, health/restart and upload oversize paths. The
server.py line guard is bumped to 760 with rationale: the deep-review
requires the emission in Handler.end_headers(), which cannot live outside
server.py.
Same class as the auth/CSRF close-on-reject: the sidecar proxy's
same-origin provenance check runs before read_body(), so an HTTP/1.1
connection reused after a 403 would parse the unread body as the next
request line. Flag close_connection on this path too.
Found by Greptile review of #6658; covered by a new unit test.
Move the close-on-reject decision into api.auth.check_auth_or_close so the
added lines land outside server.py, restoring it to 749 lines. Trim the
inline comment to a one-line docstring on the helper, per review feedback.
The rejected-write connection-close test now patches api.auth.check_auth
(the dependency the helper calls) instead of server.check_auth.
## 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.
## 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).
Gate follow-up for #7056. Two defects in the conditional password form:
1. The {{LOGIN_INVALID_PW}} / {{LOGIN_CONN_FAILED}} .replace() calls were
deleted with the moved block, but those placeholders live on the <form>
tag which ALWAYS renders. login.js reads data-invalid-pw back, and the
literal is non-empty so its || fallback never fired: a wrong password
displayed the raw text {{LOGIN_INVALID_PW}} in all 14 locales, even with
password auth ENABLED. Proven by a clean-master control (test_login_locale
3/3 on master, 2/3 on the PR).
2. is_password_auth_enabled() also gated the PASSKEY button, but passkeys are
an independent auth method (auth.py is_auth_enabled, and settings expose
passwordless_enabled = passkeys AND not password). A passwordless-passkey
instance lost its only login control. login.js additionally bailed at
'if (!form || !input) return' before ever wiring the passkey handler.
## 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.
Core's 2026-09-09 catalog sync (hermes_cli/models_catalog_static.py, unreleased
as of v2026.9.7) dropped ox-alpha-free — the Go relay delisted it (GET
/zen/go/v1/models omits it, POST returns 401) — and added glm-5.3-flash and
muse-spark-1.3-contributor. Mirror core main (29 ids, verified exact match).
Keeping the delisted id would offer a model that always 401s — the original
#5311 failure class.
The installed-core equality test now skips while released cores still carry
the delisted id (transition window documented in the test); verified 7/7
green with PYTHONPATH pointed at core main where equality binds.
The WebUI fallback list mirrors Hermes core's curated opencode-go list
(hermes_cli/models.py, synced 2026-08-20/21 against the live
/zen/go/v1/models endpoint and the public Go docs). Core owns the sync
duty; WebUI only mirrors. Adds the nine ids core gained since the
2026-07-07 snapshot (kimi-k3, gpt-5.6-luna, glm-5.3, grok-4.5,
qwen3.8-max, hy3, hy3-preview, muse-spark-1.2-contributor,
ox-alpha-free) and reorders to core's order so future diffs are
mechanical. Snapshot test updated accordingly.
Remove the #5611 stopgap that skipped the live /v1/models probe for
OpenCode Go. Its premise — the probe returning public-catalog models
not enabled on the Go tier — no longer holds: Hermes core v0.20.5
(fcbd1076) ships a Go-specific provider profile probing
https://opencode.ai/zen/go/v1/models directly and merges results
live-first with its curated Go list (#49129).
WebUI again delegates like every other provider (#1240): live catalog
first, curated static _PROVIDER_MODELS as offline fallback only.
Runtime tests pin the behavior (sentinel live id must reach the Go
group; probe failure falls back to static; providers.opencode-go.models
allowlist still wins #644).
Settle removed approval producers before fallible projection work, and fail-close every producer owned by a terminal gateway run across completion, failure, cancellation, and teardown while preserving unrelated runs.
Two repository-guide violations raised in review on #7570.
Selection is what this fix changes, but the regression test seeded only the
producer being retired. It now seeds a sibling approval on the same run and a
producer belonging to an unrelated run, then asserts the target is consumed,
both survivors remain queued, and reconciliation does not resurrect the denied
approval's mirror. Mutation-checked so the new assertions have teeth: an
over-aggressive filter that retires every same-run entry fails on the sibling
assertion, and the unfixed implementation fails on the target assertion.
Runtime behaviour also changed without tracked documentation of the invariant.
The api/route_approvals.py module docstring now records the producer/mirror
ownership direction and the retirement rules, including why an unattributable
same-run producer is retired with its run.
Additive commit: the previous commit is published and under review, so the
reviewed history is preserved rather than rewritten.
Stop the permanent ~7% core burn from re-fingerprinting state.db every 5s
and spinning on 0.1s stop-checks while nobody is listening. Block on
Event.wait for the poll interval, and sleep on _idle_wake until the first
subscriber arrives (maint #3035).
Co-authored-by: Cursor <cursoragent@cursor.com>
Metering telemetry is journaled at ~10 Hz during active turns, so marathon
reasoning runs balloon the durable run journal (12.4 MB of a single 18 MB run
was metering). On a mid-run refresh the reconnect path replays the entire
journal as one unpaced SSE burst, OOM-crashing the browser tab in a
crash-refresh loop while the server stays healthy.
- Write time: RunJournalWriter.append_sse_event() skips metering rows via
REPLAY_SKIPPED_SSE_EVENTS, returning None so caller journal-id plumbing is
untouched and remaining journaled seqs stay contiguous.
- Replay time: _replay_run_journal() and emit_replay() filter metering rows
through journal_replay_visible(), so legacy journals never stream the
backlog either.
- Readers (read_run_events / read_session_run_events) deliberately keep every
row: cursor math and the offline-gap coverage check count seqs.
Refs #7272
The cache-only Steer path only special-cased backend == "gateway" and sent
every other value — including a missing, empty, or unknown backend — to the
cached local agent. Another runtime could own the run while guidance was
enqueued on a stale local agent.
Fail closed instead: the in-process worker now registers its active run with
the explicit local backend tag (WEBUI_LOCAL_CHAT_BACKEND, the same "legacy"
value webui_chat_backend_mode() returns), and cache-only Steer only enqueues
agent.steer() on that tag. Gateway keeps its gateway_steer_queued outcome; a
missing, empty, or foreign backend returns stream_dead without steering.
Extends the positive-ownership regression with backend cases and adds a
worker-side check that the local backend tag is actually registered.
The cache-only Steer branch (no AGENT_INSTANCES worker) only rejected
*conflicting* ownership, so a missing stream owner or a missing
active-run session_id was treated as non-conflicting and let
agent.steer() run and report accepted without proving the requesting
session owns the run.
Fail closed instead: both the stream owner and the active-run session
must be present and strictly equal to the requesting session (backend
and phase are still revalidated) under the same STREAMS_LOCK ->
ACTIVE_RUNS_LOCK edge; any absent identity returns stream_dead without
calling agent.steer(). No cache eviction, no agent closing, HTTP write
still outside the locks.
Regressions: missing owner, missing/empty run session, both missing,
missing run entry -> stream_dead + steer never called; both present and
equal -> accepted, cache untouched. Legacy happy-path fixtures now
register the ownership they rely on.
The cache-only compatibility path selected the matching cached agent,
checked stream liveness under STREAMS_LOCK, released the lock, and only
then called agent.steer(). A Stop running in that gap published
phase='cancelling' and detached STREAMS/AGENT_INSTANCES while the
earlier Steer still enqueued into the cached worker, so the HTTP
response reported accepted guidance that cancellation already owned.
Revalidate stream membership, owner, and active-run
session/backend/phase and call agent.steer() under the same
STREAMS_LOCK -> ACTIVE_RUNS_LOCK edge Stop uses; interrupt,
persistence, and HTTP writes stay outside the lock. Cache eviction and
agent closing remain absent.
Extend tests/test_steer_cancel_ordering.py with a cache-only reverse-
order test (AGENT_INSTANCES absent, matching cached worker, decoy cache
entry) gating both unlocked edges around the stream-lock reacquire.
Stop-first must return stream_dead without calling agent.steer(); the
selected cache object is asserted to be the intended worker. Proven
red on the pre-fix code (order ['cancel','steer']) and green after.
## 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`
## 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