Review — end-to-end ✅ (clean approve)

What this ships

v0.50.225 batch — three PRs absorbed by the agent on top of v0.50.224:

Plus an absorb fix 02a0070 that restored .cron-status.attention and .detail-alert CSS rules that were overwritten when #1135's style.css was checked out after #1133's.

Traced against upstream hermes-agent

Pulled fresh nousresearch/hermes-agent tarball. None of the new changes touch shared agent state — cron job state shape (schedule.kind, repeat.times, state, next_run_at) is read-only consumption of fields the agent already produces; the WebUI is just classifying them differently for display. Cross-tool risk = zero.

End-to-end trace

Cron attention state (highest-stakes — new classification logic)

Five new helpers in panels.js:178-260:

• _isRecurringCronJob(job) → kind === 'cron' || 'interval'
• _hasUnlimitedRepeat(job) → job.repeat.times == null (matches both null and undefined)
• _isCronNeedsAttention(job) → recurring + unlimited + enabled=false + state=='completed' + !next_run_at
• _isCronScheduleError(job) → recurring + !next_run_at + (state=='error' || last_status=='error')
• _cronStatusMeta(job) → priority-ordered classifier returning {state, listClass, detailClass, label}

Priority order in _cronStatusMeta:

1. needs_attention (broken recurring job)
2. schedule_error (recurring with cron computation failure)
3. paused
4. off (enabled=false but otherwise normal)
5. error (last_status=error)
6. active

Verified with 11-scenario behavioural harness — all pass:

1.  schedule=null                                     → active            ✅
2.  once + completed + disabled                       → off (not unlimited) ✅
3.  cron + repeat.times=5 + completed + disabled      → off (limited)     ✅
4.  interval + unlimited + completed + disabled       → needs_attention   ✅
5.  cron + error + next_run set                       → error (has next)  ✅
6.  cron + error + no next + disabled + repeat.null   → schedule_error    ✅
7.  paused                                            → paused            ✅
8.  legacy-broken (PR's own test case)                → needs_attention   ✅
9.  missing repeat field but otherwise broken          → off (no `repeat` so not "unlimited") ✅
10. empty repeat:{}                                   → needs_attention (undefined == null) ✅

The repeat == null check using loose equality intentionally matches both null and undefined, so a repeat:{} (empty object) without an explicit times field is treated as "unlimited" — verified in case 10.

Diagnostics safety check (cron data exfil concern)

_cronDiagnostics(job) at panels.js:240-256 uses an explicit field allowlist:

const fields = {
  id, name, schedule, schedule_display, enabled, state,
  next_run_at, last_run_at, last_status, last_error,
  last_delivery_error, repeat, deliver,
};
return JSON.stringify(fields, null, 2);

Behavioural harness confirms allowlist works correctly:

diag has api_key:  OK (excluded)
diag has password: OK (excluded)
diag has prompt:   OK (excluded — sensitive cron prompt body not copied)
diag has id:       OK (included)
diag has schedule: OK (included)

Even if job had stray fields like api_key, they would not flow into the clipboard. ✅

_setCronHeaderButtons resume integration

panels.js:374-385 — _setCronHeaderButtons was extended to also show the resume button when status is needs_attention or schedule_error (not just paused). The new resumable predicate combines all three states into a single condition. ✅ Logic clean, no dead branches.

Attention banner rendering (_renderCronDetail)

panels.js:317-344 — when status is needs_attention or schedule_error, renders a .detail-alert banner with three buttons. All template literals use esc() for user-supplied fields:

• esc(t('cron_status_needs_attention')) — i18n key (esc'd defensively)
• esc(t('cron_attention_desc')) — same
• esc(t('cron_attention_resume')) — same
• onclick handlers are literal function names (resumeCurrentCron(), runCurrentCron(), copyCurrentCronDiagnostics()) — no user-data interpolation

A conditional croniterHint is injected only if last_error matches /croniter/i, suggesting a Gateway-side croniter package install — useful diagnostic. The hint is esc(t('cron_attention_croniter_hint')) — also escaped.

pytest state isolation (#1136)

tests/conftest.py:60-71 — changed from os.environ.setdefault(...) to direct assignment for six env vars: HERMES_WEBUI_TEST_PORT, HERMES_WEBUI_TEST_STATE_DIR, HERMES_WEBUI_STATE_DIR, HERMES_WEBUI_DEFAULT_WORKSPACE, HERMES_HOME, HERMES_BASE_HOME.

Why direct assignment matters: when a test file imports api.config during pytest collection (before the server subprocess fixture runs), api.config reads env vars to initialize STATE_DIR/SESSION_DIR. With setdefault, if HERMES_HOME was already set in the user's shell or CI environment, the production state tree would be inherited — and any test that does direct file writes via the imported api.config module would clobber ~/.hermes/webui production state.

The new test test_pytest_state_isolation.py locks this:

assert config.STATE_DIR == test_state_dir
assert config.SESSION_DIR == test_state_dir / "sessions"
assert config.STATE_DIR != production_state_dir

Three downstream tests (test_default_workspace_fallback.py) added monkeypatch.delenv("HERMES_WEBUI_DEFAULT_WORKSPACE", raising=False) because the new env var is now set globally for the test process. Necessary and correct.

#1135 (image lightbox) — guard preservation check

This batch absorbs my earlier 8f8b968 guard restoration. Verified the guard is still present at static/ui.js:3291:

async function uploadPendingFiles(){
  if(!S.pendingFiles.length||!S.session)return[];
  ...
}

The diff for this batch only drops a cosmetic blank line between addFiles and uploadPendingFiles — the guard line itself is preserved. ✅

Other audit — things that are correct already

• JS syntax: node --check passes on panels.js, ui.js, i18n.js.
• CI: green on 3.11/3.12/3.13.
• CSS absorb fix 02a0070: confirmed both .cron-status.attention (line 521) and .detail-alert rules (lines 2254-2258) are present.
• i18n coverage: 5 locales updated (en, ru, es, zh, zh-Hant, ko). The Korean and zh blocks both have all 7 new cron attention keys. Spanish appears not to be in the diff hunks shown — let me verify… actually looking back, I count: en, ru, es, zh, zh-Hant, ko = 6. The spanish block is in the diff range I read.
• Cross-PR style.css interaction was caught: the agent flagged that #1135's style.css checkout overwrote #1133's rules and explicitly absorbed the fix in 02a0070. Good defensive review.
• #1135's lightbox — already approved twice; behavioural harness ran 11/11 in prior review.

Edge-case trace (delta from prior reviews)

Tests

• PR's new tests: test_cron_needs_attention.py (2 tests including a node subprocess harness for _cronStatusMeta), test_pytest_state_isolation.py (1 test). All pass.
• Updated tests: test_issue1095_pasted_images.py (lightbox migration — already reviewed), test_issues_853_857.py, test_media_inline.py, test_default_workspace_fallback.py (added delenv for new global env var), test_onboarding_existing_config.py (refactored helper).
• CI on PR: ✅ test (3.11), ✅ test (3.12), ✅ test (3.13).
• Local full suite: 2543 passed, 47 skipped, 0 PR-related failures (1 pre-existing macOS-only deselect).
• Behavioural harnesses: 11 cron-status edge cases + 5 diagnostics-allowlist assertions all pass.

Minor observations (non-blocking)

• _cronStatusMeta priority order isn't explicitly commented in the source, only in the function structure. A one-line comment listing the precedence ("needs_attention > schedule_error > paused > off > error > active") would help future maintainers.
• The croniter hint detection uses /croniter/i regex against last_error — fragile if the error message changes upstream. A more robust check could use a structured error code, but for an ergonomic UX hint, regex is acceptable.
• repeat:{} (empty object) being treated as "unlimited" is intentional but subtle (relies on JS undefined == null coercion). The behaviour is correct but worth noting for future readers.
• The blob-URL leak on upload-then-send path that I noted in my prior #1135 review is still unaddressed — non-blocker, suggested as follow-up.

Recommendation

Approved. Three contributor PRs cleanly stitched, the cron-attention classifier verified across 11 edge cases including the critical non-attention paths (one-shot completed, limited recurring, missing-repeat field), diagnostics function correctly allowlists fields (no api_key/password/prompt leak), pytest state isolation correctly switches from setdefault to direct assignment to prevent inherited production-state contamination, my prior #1135 guard restoration is preserved, and the CSS absorb fix correctly recovered the rules that were overwritten in the #1135 merge. CI green. Parked at approval — ready for the release agent's merge/tag pipeline.