feat: enrich delegate events with agent, sub_session_id, and metadata#78
feat: enrich delegate events with agent, sub_session_id, and metadata#78colombod wants to merge 1 commit intomicrosoft:mainfrom
Conversation
bkrabach
left a comment
There was a problem hiding this comment.
Thanks for working on this. The bug fixes (DEL-1, DEL-2) address real inconsistencies and the constants refactor is a clear improvement. A few things to adjust:
Accept: Constants refactor + DEL-1 + DEL-2
String-to-constant refactor, adding sub_session_id and agent to resume-path events — all good.
Field normalization: just pick one name, no aliases
Since there are zero known consumers of these events, there's no backward compatibility concern. Just normalize to sub_session_id everywhere — drop the session_id alias entirely. Clean is better than compatible-with-nothing.
Drop _extract_agent_from_session_id
The rsplit("_", 1) approach is fragile (breaks for agent names containing underscores). Rather than shipping it with fragility caveats, don't ship it. If you can't get the agent name reliably on the resume path, that's fine — leave it out of those events rather than guessing.
Drop _build_delegate_metadata and the namespace/agent_name decomposition
Don't decompose the raw agent string into metadata. The agent field carries the full qualified name (e.g., "foundation:explorer") and consumers should split it at their own site if needed. Any real helpers for this belong in the consuming hook, not in foundation — we follow the principle of not promoting patterns to this level until we have 2+ examples.
Do keep metadata on all event payloads — as an empty bag
We want "metadata": None on every delegate event. This is a deliberate philosophy choice — metadata is our established property bag for experimentation and extensibility across the ecosystem. Just don't populate it with the decomposed agent string. The hook under development can put what it needs in there; foundation provides the slot.
Summary of what the revised PR should look like:
- Constants refactor ✓
sub_session_idon all events (nosession_idalias) ✓agenton resume-path events where reliably available ✓metadata: Noneon all events (empty bag, no decomposition) ✓- Remove
_build_delegate_metadata()and_extract_agent_from_session_id()helpers
Distill learnings from reviewing PRs #14 (amplifier-core) and #78 (amplifier-foundation) with the project owner. Captures metadata philosophy, extensibility patterns, and code quality standards. New entries: - Ecosystem Conventions & Event Design subsection (4 entries): metadata naming, proactive extensibility slots, raw-data-through pattern, promotion threshold for helpers - PR Review subsection additions (3 entries): zero-consumer compat burden, no fragile approaches, exemplar contract docs 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…tion, and metadata slot Refactor delegate tool event emissions for consistency and extensibility: - Extract event name constants (DELEGATE_AGENT_SPAWNED, etc.) replacing inline string literals throughout the module - Normalize all events to use sub_session_id (remove session_id backward- compat aliases from resume-path events) - Add metadata: None extensibility slot on all events for consuming hooks to populate as needed - Remove _build_delegate_metadata() and _extract_agent_from_session_id() helpers — decomposing agent names and guessing agents from session IDs was fragile and outside this module's responsibility - Remove agent field from resume-path events where it can't be reliably determined; spawn-path events retain it since the agent name is known - Update README lifecycle events documentation to reflect the revised event shapes 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
e1566e2 to
4c904fc
Compare
|
Thanks for the detailed review Brian. I have addressed all your points in the updated commit: Constants refactor + DEL-1 + DEL-2 — Kept as accepted. Field normalization: just pick one name, no aliases — Done. Dropped all Drop Drop Keep README updated to reflect the revised event shapes and document that |
Summary
Fixes inconsistencies in delegate lifecycle event payloads and adds an extensible
metadataproperty bag to all delegate events.Changes
Bug Fixes
sub_session_idto all resume-path events to normalize the child session ID key name. Keptsession_idfor backward compatibility.agentfield to all resume-path events (delegate:agent_completed,delegate:error). Agent name is extracted from the session ID suffix.Enhancements
metadata: dict | nullproperty bag to all 10 delegate event emit sites. Contains namespace decomposition (namespace,agent_name) when the agent is a qualified string (e.g.,"foundation:explorer").DELEGATE_AGENT_SPAWNED, etc.) to prevent typo errors._build_delegate_metadata(),_extract_agent_from_session_id().tool-delegate/README.md.Before (resume-path events)
After (all paths normalized)
Backward Compatibility
All changes are additive — no existing fields removed or renamed:
session_idkept in resume-path events alongside newsub_session_idmetadataisnullwhen agent name cannot be decomposedtool-delegate, not inamplifier-coreTesting
main— they reference_spawn_new_sessionwhich does not exist)