fix(cron): honor profile wrap_response during delivery - #36274
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Review: APPROVE ✅
This is a clean, focused fix for a real bug in cron delivery.
The Problem
When a cron job runs under a different Hermes profile (via job.profile), run_job() uses _job_profile_context() to temporarily switch to that profile. However, _deliver_result() calls load_config() after the profile context has been restored, so it reads cron.wrap_response from the scheduler's config instead of the job's runtime profile config. This means profile-scoped jobs that set wrap_response: false get wrapped output they didn't ask for.
The Fix
Wrap the load_config() call inside _deliver_result() with _job_profile_context(job.get("id", "?"), job.get("profile")): — the same context manager already used by run_job(). This is a 1-line addition (+ 1 indent) in production code. Minimal, correct, no side effects.
Edge Cases Handled
- No profile on job:
_job_profile_contextyields None immediately →load_config()uses scheduler profile as before. Zero behavioral change for non-profile jobs. - Exception during load: Already caught by the existing try/except block;
wrap_responsedefaults toTrue(safe). - Missing profile:
_job_profile_contextlogs a warning, yields None → falls back to scheduler config. Safe.
Tests
42 lines of new test coverage verifying:
- Separate scheduler config (
wrap_response: true) and profile config (wrap_response: false) - After delivery, content is NOT wrapped (clean output matches)
- Environment is properly restored after delivery
The test uses the existing isolated_cron_profile_home fixture and proper mocking.
Ancillary Changes
- Two small test fixture additions in
test_gui_command.py— aligning with upstreammainchanges (mentioned in PR body as CI compatibility fixes) - Model catalog timestamp/version bump — harmless data sync
Verdict
Approve. This is a textbook bug fix: clear problem, minimal change, proper test, backward compatible, well-documented. No security concerns or pipe-to-interpreter patterns.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Context
Fix for cron jobs: _deliver_result() should read cron.wrap_response from the job's runtime profile config, not the scheduler profile. This is a targeted 5-line change in cron/scheduler.py.
Review
- Correctness: Wraps
load_config()in_job_profile_context()so delivery reads the job's profile config. Correct scoping — delivery is the only affected path. - Testing: New regression test verifies the fix end-to-end with mocked delivery. GUI test patches keep CI green.
- Code quality: Minimal diff, clear intent, no edge cases exposed.
Reviewed by Hermes Agent
1ee762d to
1f20f80
Compare
|
Nice fix. I've just opened #37736, which adds a per-job |
|
Thanks for the focused regression coverage. This branch predates a deliberate cron-profile design change on current main. Problems
Suggested changes
This is an automated hermes-sweeper review. |
|
Closing this because the affected per-job cron-profile path was removed from current main, including |
Summary
cron.wrap_responsefrom a cron job's runtime profile during deliverywrap_response: falseNotes
main.Test Plan
uv run --extra dev pytest tests/cron/test_cron_profile.py tests/cron/test_scheduler.py::TestDeliverResultWrapping tests/hermes_cli/test_gui_command.py tests/hermes_cli/test_model_catalog.py::TestManifestMatchesInRepoLists::test_in_repo_lists_match_manifest -quv run --extra dev ruff check cron/scheduler.py tests/cron/test_cron_profile.py tests/hermes_cli/test_gui_command.pygit diff --checkRefs #36248
Refs #36215