fix(cron): validate bot-chat deliver/failure_deliver on dashboard update - #102091
Open
nftpoetrist wants to merge 1 commit into
Open
nftpoetrist wants to merge 1 commit into
nftpoetrist wants to merge 1 commit into
Conversation
The CLI and the cronjob tool both run every create/update through
tools.cronjob_tools._validate_bot_chat_deliver, rejecting a bot-chat
deliver/failure_deliver target whose named profile does not exist on
this machine at write time rather than only discovering it later when
a run (or a failure) actually needs to route through it.
The dashboard's PUT /api/cron/jobs/{job_id} never went through that
check for either field — it calls cron.jobs.update_job (via
_mutate_cron_for_profile) directly after only text-normalizing the
values. This predates failure_deliver: deliver has been exposed on
the dashboard's update endpoint unvalidated since bot-chat delivery
itself shipped, and failure_deliver simply inherited the same gap
when NS-788 added it. Today's same-day follow-up commit
("validate failure_deliver at preflight and dashboard update lanes")
did not close this — it only added failure_deliver to the dashboard's
text normalizer (empty clears vs. coalesces), not the bot-chat
profile-existence check; _preflight_check_delivery explicitly skips
bot-chat targets for both lanes, deferring entirely to this create/
update-time validator, so it was the dashboard's only safety net and
the dashboard never called it.
Wire _validate_bot_chat_deliver into _update_cron_job_sync via a new
_validate_dashboard_cron_bot_chat_deliver helper, checked right after
normalization and before the update is persisted, covering both
deliver and failure_deliver. The dashboard's CREATE endpoint does not
expose failure_deliver at all (not in CronJobCreate), so only the
update path needed this.
Empirically confirmed the gap before fixing: calling update_cron_job
with failure_deliver="bot-chat:definitely-not-a-real-profile" stored
the value with no exception raised. Added two regression tests
(deliver and failure_deliver) asserting the dashboard rejects an
unresolvable bot-chat profile with a 400, does not notify the
provider, and leaves the job's stored field unchanged; both fail
without the fix (mutation-verified via patch-file diff/revert/
reapply) and pass with it. Ran the full cron dashboard test file
(33 passed), tests/cron/test_cron_failure_deliver.py (24 passed),
and tests/tools/test_cronjob_tools.py (100 passed) — no regressions.
Contributor
PR #102091 — fix(cron): validate bot-chat deliver/failure_deliver on dashboard updates
Non-blocking:
Verdict: LGTM. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The CLI and the
cronjobtool both run every create/update throughtools.cronjob_tools._validate_bot_chat_deliver, which rejects abot-chat[:<profile>]deliver/failure_deliver target whose named profile doesn't exist on this machine — at write time, not only when a run (or a failure) later needs to route through it and finds nothing there.The dashboard's
PUT /api/cron/jobs/{job_id}never went through that check for either field. It callscron.jobs.update_job(via_mutate_cron_for_profile) directly after only text-normalizing the values in_normalize_dashboard_cron_updates.Is this a new regression or pre-existing? Pre-existing, and it predates
failure_deliver/NS-788._validate_bot_chat_deliverwas introduced ina2da0ab797(the original bot-chat delivery feature) for the CLI/tool create+update paths only — the dashboard's update lane never called it back then either, sodeliverhas been unvalidated on the dashboard since bot-chat delivery shipped.failure_deliversimply inherited the same gap when NS-788 added it.Today's same-day follow-up commit ("validate failure_deliver at preflight and dashboard update lanes", #101373) did not close this — I checked its diff to
hermes_cli/web_server.pydirectly: it only addedfailure_deliverto the dashboard's text normalizer (empty clears the override instead of coalescing to a target, matchingdeliver's normalization). It did not add the bot-chat profile-existence check. And_preflight_check_deliveryexplicitly skips bot-chat targets for bothdeliverandfailure_deliver("Unknown-profile failures surface per run inlast_delivery_error... and are validated at create time") — deferring entirely to the create/update-time validator. So_validate_bot_chat_deliverwas the only safety net for bot-chat targets, and the dashboard never called it.Fix
Wire
_validate_bot_chat_deliverinto_update_cron_job_syncvia a new_validate_dashboard_cron_bot_chat_deliverhelper, called right after normalization and before the update is persisted (same point the CLI/tool path validates relative to its own update). Covers bothdeliverandfailure_deliver.Scope note: the dashboard's CREATE endpoint (
CronJobCreate) does not exposefailure_deliverat all, so only the update path needed afailure_deliverfix.CronJobCreatedoes exposedeliverand has the identical pre-existing gap on create (also never calls_validate_bot_chat_deliver) — that's out of scope for this PR, which is focused on the update lane the same-day follow-up work targeted; happy to open a follow-up for create if wanted.Empirical verification
Before the fix, calling
update_cron_jobwithfailure_deliver="bot-chat:definitely-not-a-real-profile"persisted the value with no exception raised anywhere in the path.Added two regression tests in
tests/hermes_cli/test_web_server_cron_profiles.py:test_update_cron_job_rejects_unknown_bot_chat_failure_delivertest_update_cron_job_rejects_unknown_bot_chat_deliverBoth assert: HTTPException 400 with the same "not found on this gateway's machine" message the CLI path raises, the scheduler provider is not notified, and the job's stored field is left unchanged.
Mutation-verified via patch-file diff/revert/reapply (not
git stash— this worktree shares a.gitwith several sibling worktrees): reverted onlyhermes_cli/web_server.py, confirmed both new tests fail withDID NOT RAISE HTTPException, reapplied the patch, confirmed both pass.Test plan
tests/hermes_cli/test_web_server_cron_profiles.py— 33 passed (full file, includes the 2 new tests)tests/cron/test_cron_failure_deliver.py— 24 passedtests/tools/test_cronjob_tools.py+tests/hermes_cli/test_cron_dashboard_off_loop.py+tests/hermes_cli/test_cron_fire_dashboard.py— 100 passeddeliver: local,failure_deliver: ""clear, barebot-chat,bot-chat:<existing-profile>) still pass through unaffectedCompetitor check
Searched
gh pr list --searchfor "dashboard cron failure_deliver", "cron dashboard bot-chat validate", "_validate_bot_chat_deliver dashboard", "cron dashboard deliver validation", "bot-chat deliver dashboard", "cron dashboard bot-chat profile", "cron update_job bot-chat validate", "dashboard cron deliver bot-chat profile not found" (state=all). No open or merged PR addresses this specific gap; #101373 (merged today) is the closest hit but only did text normalization as shown above.