feat(adapter): polymorphic cron-delivery metadata hook - #29291
Closed
deestax wants to merge 1 commit into
Closed
Conversation
Adds BasePlatformAdapter.build_delivery_metadata(job, status_hint, base_metadata) so adapters can enrich the metadata passed to adapter.send() during cron delivery without patching the scheduler. cron/scheduler.py:_deliver_result calls the hook on the resolved runtime adapter instead of constructing send_metadata inline. Mirrors the existing on_processing_start / on_processing_complete optional-method pattern on BasePlatformAdapter. Same shape as NousResearch#5295 (removed hardcoded honcho argparse from main.py): platform-specific logic moves out of core into adapter overrides. The default returns base_metadata unchanged (or None when None), so all shipped adapters remain byte-identical. The hook fires only on the live-runtime-adapter delivery path; standalone_sender_fn is unchanged. Hook exceptions are caught and fall back to base_metadata, matching _run_processing_hook's pattern. Tests: 5 unit tests for the default hook + 3 integration tests for the scheduler path (hook called and result reaches send; exception fallback; tick() forwards status_hint='error'). Focused suite 135/135. Docs: adding-platform-adapters.md gains "Enriching delivery metadata" with an override template; cron-internals.md gains "Adapter Metadata Enrichment" with a cross-reference.
13 tasks
Author
|
Closing in favor of #26012 (which I missed during my pre-PR search — apologies). After a careful read of Brian's approach + the underlying issue #26004, his structured-side-channel via
This PR's polymorphic-hook design is more general but the generality isn't asked for by #26004, and pushing it now would compete with a PR that's already on the right track. Heading over to #26012 with a couple of extension suggestions (status field, ran_at) that the failure-route and audit-logging use cases would benefit from — if those are out of scope for the minimal patch I'm happy to defer them. Thanks to @briandevans for #26012 and to @GiorgioRegni for the original issue. |
19 tasks
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.
What does this PR do?
Adds an optional polymorphic hook
BasePlatformAdapter.build_delivery_metadata(...)so platform adapters can enrich the metadata kwarg passed toadapter.send()during cron delivery, without modifyingcron/scheduler.py. The default implementation is a no-op (returnsbase_metadataunchanged, orNonewhenNone), so every existing adapter behaves identically.Mirrors the existing optional-method pattern on
BasePlatformAdapter(on_processing_start,on_processing_complete). Follows the same shape as PR #5295 (moved hardcoded honcho argparse out ofmain.pyinto a generic plugin surface): platform-specific logic moves out of core into adapter overrides.Related Issue
No related issue filed yet. Happy to open one first if preferred — design questions for maintainers are inline at the bottom of this description.
Fixes #
Type of Change
Changes Made
gateway/platforms/base.py— addsBasePlatformAdapter.build_delivery_metadata(job, status_hint='ok', base_metadata=None) -> Optional[Dict[str, Any]]. Default returnsdict(base_metadata)if notNone, elseNone. (+39 lines)cron/scheduler.py—_deliver_resultcalls the hook on the resolved runtime adapter; addsstatus_hintparameter (default'ok', set to'error'bytick()on failed runs); hook call is exception-guarded with a fallback tobase_metadata. (+37 lines, -3 lines)tests/gateway/test_build_delivery_metadata.py— 5 new unit tests for the default hook (no-op behavior,Nonewhenbase_metadataisNone, copy semantics, signature stability,status_hintagnosticism). (+98 lines, new file)tests/cron/test_scheduler.py— 3 new integration tests for the scheduler hook path (live-adapter override is called and its return value reachesadapter.send; exception in the hook falls back tobase_metadata;tick()forwardsstatus_hint='error'on failed runs). 5 existing tests gain a one-line mock of the new method on theirAsyncMock/MagicMockadapters. (+158 lines)website/docs/developer-guide/adding-platform-adapters.md— new "Enriching delivery metadata" subsection under "Cron Delivery" with override template, behavior contract, and a note that the hook fires only on the live-adapter path. (+27 lines)website/docs/developer-guide/cron-internals.md— new "Adapter Metadata Enrichment" subsection under "Delivery Model" with a cross-reference to the adapter-author guide. (+8 lines)Total: 6 files, +364 / -3 lines.
Motivation
cron/scheduler.py:_deliver_resultcurrently constructssend_metadataas a plain{"thread_id": ...}dict (orNonewhen no thread is set) before callingadapter.send(..., metadata=send_metadata). Adapters that need richer delivery context have no clean override point — the only path today is to fork the scheduler.Concrete adapter classes that would benefit:
job_id,job_name,status,ran_atto reconstruct delivery context for the receiver.trace_id,user_id,severityfor structured logging requirements.tenant_id,account_id,project_id,lead_idfor routing.In-tree adapters don't need this today (so this PR is strictly additive with zero behavior change), but several future features would become one-method overrides on the adapter rather than scheduler-touching PRs:
message_thread_idapplied_tags; or cron-into-thread viathread_idthread_ts; @-mention support viauser_idm.in_reply_tofor reply chainingIn-Reply-To/Referencesheaders for threading; MIME priorityNone of those are in scope here; listed as evidence the missing extension point is broadly applicable.
Scope clarification
The hook fires only on the in-gateway live-runtime-adapter delivery path — when
cron.scheduler._deliver_resultresolves aruntime_adapterfrom theadaptersdict and the gateway's event loop is running. The out-of-processstandalone_sender_fnpath (used whenhermes cron runis invoked outside the gateway process) continues to use the existing keyword-argument shape and bypasses the hook. This is documented in both updated guides.If an adapter needs metadata enrichment in both paths, the override logic should be mirrored inside its
standalone_sender_fn.How to Test
Expected: 135 tests pass.
Manual verification on shipped adapters: cron deliveries continue to produce the same
metadatapayload as before this PR. No behavior change for Telegram, Discord, Slack, WhatsApp, Signal, Matrix, Mattermost, Email, SMS, Home Assistant, DingTalk, Feishu, WeCom, Weixin, BlueBubbles, or QQ — they all inherit the no-op default and produce a byte-identicalsend_metadatavalue (Nonewhen no thread is configured,{"thread_id": X}when one is). Only adapters that explicitly overridebuild_delivery_metadatasee different behavior.Checklist
Code
feat(adapter): ...)bash scripts/run_tests.sh(focused suite —tests/gateway/test_build_delivery_metadata.py+tests/cron/test_scheduler.py) and all 135 tests passDocumentation & Housekeeping
adding-platform-adapters.mdandcron-internals.mdcli-config.yaml.exampleif I added/changed config keys — N/A (no config keys added)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/ADesign questions for maintainers
I'd appreciate feedback on a few open shape questions before assuming this is final:
Method name —
build_delivery_metadatavsenrich_delivery_metadatavsmetadata_for_delivery? Slight preference forbuild_since the default constructs (a copy of) the dict frombase_metadata, but happy to rename.Sync vs async — neighboring
on_processing_start/on_processing_completeareasync. This hook is sync because the cron scheduler dispatch path calling it (_deliver_result) is synchronous. Happy to make itasync deffor consistency if you prefer — the call site would need a small bridge toasyncio.run_coroutine_threadsafesimilar tosafe_schedule_threadsafeusage elsewhere in the file.status_hintparameter — currently(self, job, status_hint='ok', base_metadata=None). Open to dropping it if you'd rather adapters derive status from the job dict, or to making it a richer enum.Adapter method vs generic plugin hook — could alternatively be a
register_hook("cron_build_delivery_metadata", ...)-style observer. Adapter method seems right for per-platform customization (the adapter owns its delivery shape); a generic hook seems right for cross-cutting observers. Open to the alternative if you prefer.Screenshots / Logs
scripts/run_tests.sh tests/gateway/test_build_delivery_metadata.py tests/cron/test_scheduler.pyfinal summary: