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>
The process-lifetime cache from the previous commit was flagged in PR
review (nesquena-hermes): Path.resolve() is a filesystem call, not a
pure function of process-startup constants. A profile-home symlink can
be retargeted while the long-running webui process stays up, and
_safe_resolve() deliberately falls back to the unresolved input after a
transient OSError/RuntimeError/ValueError -- a process-lifetime cache
would serve either a stale symlink target or a stale fallback forever.
Redesign: replace the module-level dict with a contextvars.ContextVar
that defaults to None (no active scope -> resolve fresh, exactly like
pre-PR behavior). profile_home_resolve_cache_scope() sets it to a fresh
dict for the duration of one call and resets it in a finally block, so
no cache entry survives past that call. It now wraps only
_load_cli_sessions_uncached as a decorator -- the actual N+1 hot loop
(hundreds of Session.__init__ calls per sidebar build, each resolving
the same profile argument). All other call sites of
_resolve_profile_home_param (confirmed via repo-wide grep: all internal
to api/workspace.py, none elsewhere) get fresh resolution on every call,
same as before this PR existed.
Rewrote tests/test_profile_home_resolve_caching.py to prove: (a)
repeated resolves within one scope still hit the cache once, (b) two
separate scopes (simulating two requests) do not share state -- a
symlink change between them is observed on the second scope, and (c)
calls outside any scope never cache at all.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round-3 gate finding: the downstream table regex lacks an end-of-line anchor, so
it accepts a prefix that the physical-line guard rejects (e.g.
'| h<br>x | n |\n|---|---| trailing').
Measured against MASTER rather than against the intermediate commit: master does
not render that input as a table either, so the stage is at parity, not
regressed. Across the edge-case matrix (indented, CRLF, trailing-space, padded
separator, blank-line delimited, mid-document) the stage is equal to master on
the two malformed inputs and strictly better on six genuine tables.
Pinned as a regression so any future change to the table guard has to make a
deliberate decision about this input instead of changing it silently.
Third gate finding: the table-row predicate accepted ANY pipe-wrapped line, but
the downstream table parser only accepts a run of pipe-lines whose SECOND line
is a separator. So pipe-delimited prose such as '| note<br># heading |' was
treated as a table row and kept its literal <br>, silently losing heading/list
rendering that master renders correctly.
The guard now classifies complete blocks with the parser's own grammar
(row regex + separator check) and converts <br> on every unclassified line.
Verified: all non-table cases now match master byte-for-byte, while genuine
multi-row tables keep <br> in every data cell.
Co-authored-by: JayaMegelar <JayaMegelar@users.noreply.github.com>
The release gate (Codex, adversarial pass) reproduced two SILENT regressions
introduced by the original implementation. Both are fixed here with
revert-sensitive regressions.
1. Collidable sentinel. Table-row <br> was stashed as a fixed \x00BR\x00 token
and that token was unconditionally rewritten back to <br> afterwards, so
user-supplied text containing the literal token was corrupted -- in prose it
became a <br>, and inside a link URL it broke the anchor apart. Replaced with
conditional conversion: <br> becomes a newline only on non-table-row lines,
so no placeholder is ever introduced and nothing user-suppliable is rewritten.
2. Lost HTML emphasis. The stricter italic regex (which correctly stops
'a * b * c' from italicising) also rejected the '* x *' produced by the
<em>/<i> pre-pass, so '<em> italic </em>' degraded to literal asterisks -- and
to a bullet list at line start. Boundary whitespace is now kept OUTSIDE the
generated delimiters.
Verified against the real renderMd(): output for both cases now matches master,
while the table-cell <br> and spaced-asterisk fixes still hold. A revert probe
confirms all five new regressions fail against the defective implementation.
Co-authored-by: JayaMegelar <JayaMegelar@users.noreply.github.com>
_resolve_profile_home_param has two branches feeding the same
_cached_safe_resolve_profile_home() memoization: an explicit Path, and a
logical profile-name string resolved via get_hermes_home_for_profile().
The existing regression test only covered the Path branch. Add a matching
test that repeatedly resolves a profile-name string and confirms it
returns the correct home and that _safe_resolve is invoked only once
across repeated calls, i.e. the cache is genuinely hit for the
string-keyed path too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_resolve_profile_home_param() called Path.resolve() on the profile's home
directory with no caching. The webui session list builds one Session per
CLI session row, and Session.__init__ calls this function once per row, so
with ~484 sessions on the same profile it redundantly re-resolved the
identical path hundreds of times per request. Individually cheap (~1ms),
but under host load this compounded into multi-second sidebar hangs,
confirmed via journalctl showing repeated 'Slow WebUI request still
running' warnings (5-17s) pinned to this exact call site.
The mapping from a profile argument to its home path is a pure function of
process-startup constants (_DEFAULT_HERMES_HOME / _INITIAL_HERMES_HOME)
that are resolved once at import time and never mutated afterwards, so
memoizing per profile argument for the life of the process is safe. Profile
creation only creates the directory at the already-deterministic path; it
does not change what path a profile argument resolves to.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The all-profiles sidebar scan walks every profile's state.db in one
request. When it lazily minted a "Webhooks" / "Cron Jobs" project for a
profile that had webhook/cron rows, ensure_webhook_project() and
ensure_cron_project() tagged the new entry with get_active_profile_name()
-- the UI-selected profile -- instead of the profile whose rows triggered
it. A webhook-only profile therefore leaked a "Webhooks" project onto
whichever profile was selected; deleting it did nothing because the next
sidebar poll re-minted it, and switching profiles minted one per profile.
ensure_webhook_project / ensure_cron_project / _profile_has_user_projects
now take an explicit profile (default: active profile), and the scan
passes _cli_profile through, so the opt-in gate and the tag both key on
the scanned profile. Existing stubs in tests updated to accept the kwargs.
Disabled skills are excluded from the backend skill-command map, but the WebUI
picker listed every /api/skills entry regardless of skills.disabled -- so a
disabled skill stayed selectable in both '/use <skill>' sub-args and the
skill-command suggestions (#7509). A single _isSkillDisabled() gate now filters
both surfaces.
Also fences the skill caches across a profile switch: invalidateSlashSkillCaches()
bumps a generation counter that an in-flight /api/skills reply checks before
committing, so a payload generated for the outgoing profile can no longer land
after the switch and keep hiding a skill that is enabled in the new profile.
Maintainer follow-up folded in: loadSkillCommands() published readiness in a
finally block that ran even when /api/skills rejected, leaving
_skillCommandCacheReady=true with an empty cache. Because
ensureSkillCommandsLoadedForAutocomplete() only re-loads while
!ready && !promise, one transient failure PERMANENTLY emptied the slash-command
picker until a page reload. Readiness is now published only after a successful
current-generation commit. This wedge is PRE-EXISTING on master (its finally sets
readiness unconditionally) and is not introduced by this PR; fixed here because
this change already reworks the surrounding generation handling. Added a
reject-once/recover regression plus a __failSkillsOnce() harness knob.
Gate: Codex SAFE TO SHIP on the fixed head — confirmed the wedge is closed for
every failure interleaving, the stale-generation fence is NOT weakened (a stale
rejection landing after a fresh success leaves the fresh cache intact), and
_skillCommandCacheReady=_committed is correct rather than ||-ing with the old
value (which would re-create the wedge after a forced refresh erased the cache).
Verified the API really emits a boolean 'disabled' per skill (api/routes.py:931)
so the predicate matches the true payload. Full suite 15,612 passed with only the
known box-baseline singleton; 9/9 own tests, 34/34 neighboring slash tests;
16/16 required checks green.
Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
The local /skills handler swallowed every argument into its own skill-name
search, so the agent-owned write-approval subcommands never reached
hermes_cli/write_approval_commands.py -- '/skills pending' and friends silently
ran a search instead. cmdSkills() now returns false for those subcommands, using
the pre-existing documented opt-out contract honored at commands.js:180 (the same
one /reasoning already uses), so they fall through to the normal send path.
Maintainer follow-up folded in: the submitted list covered 6 subcommands but the
agent handler also accepts the aliases apply, deny and drop, so '/skills apply 3'
and '/skills deny 3' were still being swallowed. Completed the class against the
live handler's alias sets and verified all 18 alias/query cases route correctly.
Gate: node -c + ESLint runtime gate clean; 16/16 required checks green.
Co-authored-by: totalitarian <totalitarian@users.noreply.github.com>
loadSkillCommands() published readiness in a finally block that ran even when
/api/skills rejected, so one transient failure left
_skillCommandCacheReady=true with an empty cache. Because
ensureSkillCommandsLoadedForAutocomplete() only re-loads while
!_skillCommandCacheReady && !_skillCommandLoadPromise, no later picker pass ever
retried: skill commands stayed missing until a page reload even after the API
recovered.
Readiness is now set only after a successful current-generation commit; the
promise is cleared either way so the next pass can retry. The stale-generation
fence is unchanged (verified a reply from the outgoing profile still refuses to
commit).
This wedge is PRE-EXISTING on master (its finally sets ready unconditionally),
not introduced by this PR — fixed here because this change already reworks the
surrounding generation handling. Adds a reject-once/recover regression plus a
__failSkillsOnce() harness knob; reverting the fix turns it red.
Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
The slash-skill caches (_slashSkillCache, _slashSkillCachePromise,
_skillCommandCache) outlived a profile switch, so the outgoing profile's
/api/skills payload kept hiding skills that the incoming profile enables
(#7509).
- call invalidateSlashSkillCaches() from both switch handlers
(static/panels.js switchToProfile, static/sessions.js
_switchProfileForSessionLoad), after the /api/profile/switch request
- bump a generation counter in invalidateSlashSkillCaches() and capture it
in both loaders: a reply issued for the outgoing profile can no longer
commit once the switch has landed
- tests: in-flight race and cache-refresh regression on both picker
surfaces, plus a check that both switch paths call the invalidation
`skills.disabled` entries are already excluded from the backend skill-command
map, but the composer still offered them as selectable slash commands and as
`/use` sub-args: both picker surfaces read `/api/skills` and ignored the
`disabled` flag the endpoint reports per skill.
Add one `_isSkillDisabled()` gate in static/commands.js and apply it at the two
places that turn `/api/skills` rows into completions:
* `_buildSkillCommandEntry()` returns null for a disabled skill, which keeps
`_skillCommandCache` (and every consumer of it, including the dropdown) clean;
* `_loadSlashSkillSubArgs()` skips disabled skills when building `/use`
sub-args.
Both caches still rebuild from `/api/skills`, so a skill toggled off in the
skills panel disappears from the picker on the next load.
Adds tests/test_issue7509_disabled_skills_slash_picker.py, which runs the real
static/commands.js in a vm context with a mocked `api()` and asserts on
`getSlashAutocompleteMatches()` for both surfaces, over 0 / 1 / many skills,
enabled / disabled / flag-absent entries and a disable-then-refresh cycle.
Fixes#7509
Gate found two blocking defects in the new caches:
1. The large-string LRU was keyed on input text only. The installed
agent.redact registry can extend the secret matcher at runtime via
register_redaction_patterns() and exposes no policy generation or
invalidation hook, so a >16k string primed before a registration was
served with the new secret intact (reproduced against the installed
registry). Drop the large-string cache: strings above 16,384 chars run
the redactor uncached again. Regression primes a large string, registers
a runtime pattern on the real registry and proves the next
redact_session_data() response is redacted.
2. The prefilter LRU was count-bounded (8192) but its per-entry gate was a
character cap, which is not a byte bound: 16k four-byte code points is
~64 KiB per key, ~512 MiB at capacity, and the workload died with
MemoryError under 96 MiB of headroom. Gate each entry on
sys.getsizeof(text) <= 2048 so the retained-key ceiling is a hard
16 MiB. Regressions cover max-width Unicode, sensitive verdict pairs,
eviction past the cap, and a low-memory RLIMIT_AS subprocess.
Once large strings stopped being re-redacted on every response, cProfile
showed the cheap prefilter had become the dominant cost of the redaction
pass: 2.98s of 3.7s, spent testing 71 literal markers against each of the
~74k strings (~19MB) of a large session, i.e. ~3.1M interpreted iterations
per request — repeated by every tab and every poll for a constant verdict.
Memoize the verdict behind a bounded LRU, mirroring the existing redaction
caches: entry count and per-entry size are both capped, so retained bytes
stay bounded and oversized blobs simply run the scan uncached. Non-strings
never reach the cache, so an unhashable value cannot raise.
Measured on a real 22MB session, warm: full redaction 2.07s -> 1.07s (-49%),
output byte-identical. Combined with the large-string cache, the recurring
redaction cost drops from ~8.4s to ~1.1s.
Rejected alternative, kept in a comment: collapsing the 71 `in` scans into
one compiled alternation is the obvious-looking fix but measured 38% SLOWER
(2.60s vs 1.88s) — CPython's substring search is already a tuned C routine.
Security: the prefilter gates the entire redaction pass, so equivalence is
pinned by a differential test suite against a copy of the previous
implementation — every marker individually, case variants, embeddings,
near-misses, unicode lowercase traps, 4000 fuzz cases, plus oversized and
unhashable inputs. Verdicts are identical on all 73,658 strings of a real
session (1935 flagged before and after).
Broad selection: 22 failures before and after, identical set — all
pre-existing.
`_redact_fn_cached` skipped memoization for any string over 16KB, so those
strings re-ran the full ~15-pass redactor on every single request. On a real
22MB session that is 59 large strings (29 unique, 0.76MB) costing 1.68s per
request — 99.9% of the recurring redaction cost once the small-string cache is
warm — repaid by every tab and every poll for a byte-identical result.
Large strings now get their own bounded cache, separate from the 4096-entry
small-string one so a large blob can never evict the thousands of small
recurring strings. Entry count and per-entry size are both capped; anything
above the ceiling stays uncached, keeping retained bytes bounded.
`_redact_value` additionally stops rebuilding containers whose descendants
were unchanged. Most of a transcript carries no credential marker, so the
unconditional dict/list comprehension deep-copied the whole payload to
reproduce an identical structure — pure CPU under the GIL, which serializes
concurrent tab loads on a threaded server. Callers treat redacted output as
read-only (`_public_message_projection` and `redact_session_data` both build
fresh containers), so sharing an unmodified subtree cannot leak a later
mutation back into session state. As soon as one string is masked, every
container on the path to it is rebuilt and the caller's original is untouched.
Measured on the live 22.6MB session, steady state: redact_session_data 3.78s
-> 1.96s single tab (-48%), 7.93s -> 4.06s with two concurrent tabs (-49%),
byte-identical output.
nesquena-hermes