- docs/architecture/models-cache-invalidation.md: add 'Invalidation modes'
section describing delete_disk=True/False and why /api/profile/switch
uses the memory-only mode (fingerprint-guarded per-profile disk cache).
- tests: replace the source-text scan of the switch route with two executed
routes.handle_post tests: one captures invalidate_models_cache kwargs and
asserts exactly [((), {'delete_disk': False})]; one runs the real
invalidate_models_cache and asserts the disk file survives while the
in-memory catalog is dropped. Both fail if the route reverts to the
default delete_disk=True.
POST /api/profile/switch called invalidate_models_cache(), whose
test-isolation contract also unlinks models_cache.<profile>.json. On a
switch nothing about the sources changed, so the delete only forced a
full cold rebuild (live provider fetch_models HTTPS calls, ~490k deepcopy
calls) on the very next /api/models — several seconds on every switch.
The disk cache is already keyed per profile and rejected on read when
_models_cache_source_fingerprint() differs, so it is safe to keep. Add a
delete_disk keyword (default True — every existing caller keeps the old
behaviour) and have the switch route pass delete_disk=False.
Regression tests cover: memory-only drop preserves the disk file, default
call still unlinks it, the next get_available_models() serves the disk
snapshot without a live rebuild, and the switch route opts out. Two
existing zero-arg stubs of invalidate_models_cache accept **kwargs now.
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`
The #7240 fix peeled the leading @custom: segment on first-colon split, but a
plain-lane custom id whose model carries a tag/variant colon
(@custom:ollamacloud/qwen3.5:397b) was still misread as slug=ollamacloud/qwen3.5,
rendering only "397b". Two changes:
- _dynamicModelLabels (the backend-populated catalog) is now consulted BEFORE
the string peel: the backend knows the true provider/model split and ships an
exact m.label per routing id, so catalogued ids — including plain-lane colon
models and collision cases — render verbatim.
- The legacy string fallback now treats a slash-bearing first segment as the
model itself (a provider slug is a config key or host:port authority and
never contains '/'), mirroring the '/' means routable rule api/config.py
applies when building ids.
Tests: add plain-lane @custom:ollamacloud/qwen3.5:397b controls, a
post-catalog dynamic-label control, and a collision case proving labels stay
distinct by full routing id. Driver now accepts an optional pre-hydrated
catalog map.
The settled current-summary card was only created when no compaction
marker was loaded at all. A transcript with older (stale) markers plus a
newer `compression_anchor_summary` that no loaded marker references
therefore lost the current summary, which master still rendered.
Decide the fallback from `_latestCompressionReferenceMessage()`
(`referenceMessageRawIdx < 0`) instead of marker presence, and feed that
decision into `_selectCompactionCardPlacements()` before any card node is
built so the fallback is the single preserved-task owner and marker cards
never attach the task list a second time.
Add a render-level regression that executes the real `renderMessages()`
over 220 messages, three stale markers, one preserved task-list message
and a newer summary: the current summary is visible and exactly one task
owner is rendered, in full, virtualized and server-truncated windows.
The renderMessages node harness added in origin/master (#7125 lineage)
evals renderMessages in isolation; the new loaded-compaction gate calls
_loadedCompactionMarkerRawIdxs, which must be stubbed like the other
render helpers (_postProcessWithAnchorSuppression, _usedModelTurnChipLabel).
Gate follow-up to #7649. _cronScheduleForEdit() consulted sched.expr before
schedule_display, so opening or duplicating a job scheduled as
`every monday 9am` populated the field with the canonical `0 9 * * 1`. The
Agent rebuilds schedule_display from whatever the WebUI submits, so the
rewrite stuck and the user silently lost their phrasing.
Only the `once at ...` label is unparseable; every other schedule_display
form round-trips through parse_schedule(), so prefer it and fall back to
expr/expression.
The existing tests asserted on the SOURCE TEXT of the helper and stayed green
through this defect. Adds four tests that EXECUTE the real helper out of
panels.js via node; the natural-language case is red before this fix.
Co-authored-by: happy5318 <happy5318@users.noreply.github.com>
## 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`
Own a private stdout descriptor before agent imports and serialize request log writes. Keep logging failures isolated from HTTP responses. Add successful chat/start subprocess delivery regressions and document header-latency semantics and cron ownership.
PR #7636 review (greptile-apps) flagged the profile-home resolve cache
scoping (profile_home_resolve_cache_scope() / _PROFILE_HOME_RESOLVE_SCOPE
in api/workspace.py) as an undocumented runtime contract per AGENTS.md.
Record what is cached, the call-scoped (not process-lifetime) lifecycle,
which single call site owns it while every other caller resolves fresh,
and the freshness guarantee this gives against symlink retargets and
transient resolve fallbacks; index the document from docs/CONTRACTS.md,
following the same pattern used for the /api/models cache invalidation
contract. Docs only.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The webhook second-pass loop in _load_cli_sessions_uncached called
get_last_workspace(profile=_cli_profile) directly per row instead of the
_cli_workspace() closure the main/cron/kanban passes already use, so it
paid its own redundant file reads/is_dir() probe on every webhook row even
though the loop already sits inside the profile-home resolve cache scope.
Flagged in PR #7636 review (greptile): this is the one concrete gap in an
otherwise-correct fix.
Also adds an integration-level regression test proving the actual N+1 this
PR targets -- Session.__init__ constructed via
_state_projection_sidecar_metadata's Session.load_metadata_only() fallback
on a cold sidecar-metadata cache -- is deduped by
@profile_home_resolve_cache_scope() on _load_cli_sessions_uncached, with a
companion test showing the same fixture scales linearly (one filesystem
resolve per row) when the scope is bypassed. This directly answers the
review claim that the cache scope doesn't reach any Session construction:
it does, verified by counting real _safe_resolve() filesystem calls rather
than call counts on the memoizing wrapper alone.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
carlotestor