feat(cron): opt-in clarify support for cron agents via cron.allow_clarify - #74113
feat(cron): opt-in clarify support for cron agents via cron.allow_clarify#74113agastya-raj wants to merge 3 commits into
Conversation
|
Thanks for preserving the unattended default and for covering the direct callback round trip. I found two delivery-path gaps that need correction. Problems
Suggested changes
Automated hermes-sweeper review. |
88b5b2f to
0b3b280
Compare
|
Thanks @hermes-sweeper — both findings were real. Fixed in 1. Text-reply session-key mismatch — root cause confirmed: the gateway intercepts typed clarify answers by the inbound chat's session key ( Fix: new 2. Relay transport not preserved — fix mirrors the existing convention rather than inventing a parallel one: cron delivery already stamps Tests (
Live-verified on a production gateway against the previous revision (button render + timeout path); re-verifying this revision now and will report. |
|
Round 2 — an independent review of the round-1 diff found real holes in the reply-key fix. All addressed in 1. DM misdetection (was HIGH). Correct: 2. Unrealistic test origins (was HIGH, masked #1). Tests no longer hand-craft origins — they build them through the real 3. Missing 4. Hardcoded 5. Discord explicit 6. 7–9. Send future is cancelled on timeout/failure (no late buttons for a discarded entry); test threads are daemon + loop closed; sub-minute timeouts render Docs — the "typed answers always resolve for threads and DMs" claim was wrong; replaced with the actual supported matrix (DM ✓ / thread ✓ / origin group ✓ / fan-out group ✗ buttons-only) and a known-limitations block (FIFO-oldest cross-talk with interactive clarifies in the same chat, any-member button clicks on fan-out, multiplex/ Tests — Live-verified on a production gateway (buttons render from a ticker-fired job, registration log shows the delivery-chat key, timeout path clean); typed-reply resolution itself can't be live-proven without a human typing — unit coverage is the proof for that direction. |
0b3b280 to
0707957
Compare
What does this PR do?
Cron agents currently cannot use the
clarifytool at all: 4494c0b hard-disabled theclarifytoolset in cron context because jobs run unattended — "there's nobody to ask questions to". That is the right default, but it also makes a legitimate use case impossible even as an opt-in: scheduled jobs that need a human-in-the-loop decision (e.g. a nightly job that asks "deploy this migration? yes/no" before acting, or a weekly audit that asks which finding to escalate). The job already has a delivery channel to the user — delivery just happens at the end of the run, whileclarifyneeds it mid-run.This PR adds an opt-in config gate,
cron.allow_clarify(defaultfalse, preserving current behavior exactly). When enabled and the job is fired by the gateway ticker (live platform adapters + event loop present):_resolve_cron_disabled_toolsetsno longer hard-disablesclarify.run_jobbuilds a clarify callback via the new_build_cron_clarify_callback: it resolves the job's first delivery target, finds the live adapter for that platform (via the existingresolve_delivery_transport), and renders the clarify prompt through it — e.g. as Discord buttons in the job's delivery channel. The design mirrorsgateway/run.py's_clarify_callback_sync; the only cron-specific difference is that the target chat comes from the job's delivery config instead of an inbound message event (a cron session has no attached chat).agent.platform_hints.cronoverride in config.yaml still wins.Key behaviors:
hermes cron runfires have no live adapter, so no callback is attached andclarifykeeps reporting its standard "not available in this execution context" — unchanged from today. Same when the delivery platform's adapter lackssend_clarify, or gateway config fails to load.clarify_gateway.wait_for_response, which polls in 1-second slices and heartbeats the activity tracker, so the cron inactivity watchdog (HERMES_CRON_TIMEOUT, default 600s) does not kill the run while the user is deciding.agent.clarify_timeout(default 1h,0= unlimited) or a failed send, the tool returns a sentinel string ([user did not respond within Nm]/[clarify prompt could not be delivered]) so the agent proceeds autonomously instead of hanging — identical to the gateway path._cron_clarify_reply_session_keywith platform-aware chat-type resolution (live adapterget_chat_info→ originchat_type/scope_idstamp → Slack/Telegram id heuristics). Supported matrix: DM targets ✓, thread/forum targets ✓ (participant-shared; Discordparent:threadtargets remap to the thread id), group/channel the job was created from ✓ (bound to the scheduling member), group/channel fan-out ✗ (typed replies only — button clicks resolve by clarify_id either way). Multiplex profiles andthread_sessions_per_user: truelimit typed-reply binding; a cron clarify and an interactive clarify pending in the same chat share FIFO-oldest text resolution.DeliveryTransport.is_relay, the clarify send stamps the logical platform via the existing_relay_logical_platformmetadata escape hatch (the same convention cron delivery uses), extended to the Relay prompt lane — a scheduled send has no inbound event to populate the relay's per-chat platform map.Related Issue
No existing issue or PR covers this. Searched open + closed issues and PRs for
clarify/croncombinations; the nearest neighbors are #36731 (CLI wording of cron delivery error status — unrelated) and #61438 (clarify constrained-choice UX across surfaces + profile-scoped cron storage — itscron/scheduler.pychange is limited tocontext_fromoutput paths, no overlap). The hard-disable this PR gates was introduced in 4494c0b.Type of Change
Changes Made
cron/scheduler.py_resolve_cron_disabled_toolsets:clarifyonly hard-disabled whencron.allow_clarifyis not set (docstring updated to match)._CRON_CLARIFY_PLATFORM_HINT+_build_cron_clarify_callback(job, adapters, loop): delivery-target resolution, live-adapter lookup, fire-timeget_chat_infochat-type hint, gateway-style clarify registration under the delivery chat's reply-resolution session key, Relay-aware send metadata, targeted send-failure cleanup + future cancel, watchdog-safe wait, sentinel returns on send failure/timeout._cron_clarify_reply_session_key+_platform_dm_hint: platform-aware reply-key computation (Discord/Telegram/Slack DM & thread shapes, Slackscope_id, Discordparent:threadremap, origin-user group binding, fan-out/thread_sessions_per_usernull-outs).run_job: new optionaladapters/loopkwargs (defaultNone— every existing caller unchanged; docstring updated); attaches the callback + hint swap afterAIAgentconstruction, non-fatally.run_one_job: plumbs its existingadapters/loopthrough torun_job.tools/cronjob_tools.py:_origin_from_envstampschat_type+scope_idfrom the bound session source so chat-created jobs carry the chat's true type/scope (justification: without it, DM detection for Discord/Telegram origins is impossible at fire time).tools/clarify_gateway.py: newdiscard(clarify_id)targeted-removal API (justification: the only removal wasclear_session, which cancels every pending clarify in a chat — including an unrelated interactive session's prompt).gateway/relay/adapter.py:_send_promptlearns the_relay_logical_platformmetadata escape hatch (10 lines, mirrorssend()exactly; interactive path unchanged — it never sets the key).hermes_cli/config_defaults.py: registers thecron.allow_clarify: Falsedefault with explanatory comment.website/docs/user-guide/features/cron.md: new "Human-in-the-loop jobs (clarify)" section (config, timeout semantics, the typed-reply supported matrix, known limitations, Relay note, platform-hint override note).tests/cron/test_cron_clarify.py(new): toolset-gate tests; reply-key matrix unit tests (origin-stamp/adapter-hint/id-heuristic DM detection, Slack scope, Telegram DM/forum/group shapes, Discordparent:threadremap, Slack thread,thread_sessions_per_usernull-out); callback-construction degradation tests; deliver + button-resolve round-trip; targeted send-failure discard (interactive entry survives); seconds-rendering timeout sentinel; text-reply binding tests with origins built through the real_origin_from_env; Relay metadata stamping tests;run_jobwiring tests (gate + attach + hint swap + operator override + gate-off).tests/gateway/relay/test_relay_interactive.py:_send_prompthonors_relay_logical_platformand strips it from outbound metadata.tests/cron/test_run_one_job.py,tests/cron/test_scheduler.py,tests/cron/test_parallel_pool.py,tests/cron/test_execution_ledger.py: existingrun_jobfakes/lambdas updated to accept the new kwargs; new test intest_run_one_job.pyassertingrun_one_jobforwardsadapters/looptorun_job.How to Test
scripts/run_tests.sh tests/cron/ -q— new + existing cron tests pass.cron.allow_clarify: truein~/.hermes/config.yaml, create a job whose prompt calls the clarify tool and whosedelivertargets a Discord channel, and let the gateway ticker fire it. The clarify prompt renders as buttons in the delivery channel; clicking resolves the tool call and the job completes.agent.clarify_timeoutthe agent receives[user did not respond within Nm]and continues; the scheduler does not stall and the inactivity watchdog does not fire._resolve_cron_disabled_toolsetsstill returnsclarifyand behavior is identical to before.Tested on Ubuntu 24.04 (x86_64), Python 3.11:
pytest tests/cron/ tests/gateway/ -q→ 4869 passed, 7 failed — the 7 failures are pre-existing suite-order flakiness intest_discord_send.py/test_send_multiple_images.py/test_session_store_prune.py/test_compression_concurrent_sessions.py: the same files fail on pristinemain @ 470cf66b0without this branch (the exact set wobbles run-to-run) and all pass in isolation.clarify text replies bind to session key agent:main:discord:group:<chat>:<user>), and the timeout path completed cleanly — the clarify tool returned after exactlyagent.clarify_timeout(600s), the agent continued autonomously, the final response was delivered, and the scheduler ticker stayed healthy throughout. An earlier revision was additionally verified live with button rendering and a real user click resolving the clarify call (sameresolve_gateway_clarifyround-trip the interactive path uses).Checklist
Code
feat(cron): …)pytest tests/ -qand all tests pass —tests/cron/+tests/gateway/green apart from 7 pre-existing suite-order flakes that fail identically on pristine main (see "How to Test")Documentation & Housekeeping
website/docs/user-guide/features/cron.md(English; i18n zh-Hans copy intentionally untouched)cli-config.yaml.exampleif I added/changed config keys — N/A: the example file has nocron:section at all (cron keys live inhermes_cli/config_defaults.pydefaults); the new key is registered there with a commentCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Aclarifytool schema/handler is unchanged; only its availability in cron context is gatedScreenshots / Logs
Live verification log lines from a ticker-fired run of THIS branch with
cron.allow_clarify: true(origin-bound Discord delivery, no config-level platform-hint override):The Discord delivery channel showed the clarify prompt and the pending entry registered under the delivery chat's own gateway session key (the key an inbound typed reply resolves against); when the prompt expired unanswered the agent reported the timeout and its final response was delivered as usual, with the next tick on schedule. An earlier revision of this change was additionally verified with rendered buttons and a real user click resolving the clarify call (same
resolve_gateway_clarifyround-trip).