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
## 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`
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).
- 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.
- 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.
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.
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.
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.
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
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)
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.
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.
- _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
/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).
* 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>
* 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>
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.
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
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.
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.
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).
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.
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.
- 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
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.
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>
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.
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.
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.
The JavaScript _normalizeConfiguredModelKey function had the same bug as the
Python _norm_model_id function that was fixed in commit d6164cd. It used
substring(indexOf(':')+1) which only removes 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 duplicate Primary badges to appear in the model dropdown when using
custom providers with @provider:model ID format.
Changes:
- Replace substring(indexOf(':')+1) with split(':').pop() to strip all colon prefixes
- Add provider name to badge label for clarity (e.g., 'Primary (jingdong)')
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.
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.
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.
happy5318