Repository navigation
Normalize Copilot session messages and preserve nested agent context #66906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| input_tokens: "inputTokens", | ||
| output_tokens: "outputTokens", | ||
| total_tokens: "totalTokens", | ||
| reasoning_output_tokens: "reasoningOutputTokens", | ||
| cache_creation_input_tokens: "cacheCreationInputTokens", | ||
| cache_read_input_tokens: "cacheReadInputTokens", | ||
| }; | ||
|
|
@@ -32,6 +33,65 @@ | |
| return typeof value === "number" && Number.isFinite(value) && value >= 0; | ||
| } | ||
|
|
||
| /** @param {any} event @returns {boolean} */ | ||
| function isNestedSessionEvent(event) { | ||
| if (event == null) return false; | ||
| return [event.agentId, event.data?.agentId, event.parent_tool_use_id, event.parentToolUseId, event.parentToolCallId, event.data?.parentToolUseId, event.data?.parent_tool_use_id, event.data?.parentToolCallId].some( | ||
| value => typeof value === "string" && value.length > 0 | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * An agent instance is authoritative; deprecated parent-tool markers are only | ||
| * the identity fallback. Child initialization must not change the root session. | ||
| * @param {Array<any>} events | ||
| */ | ||
| function sessionEventContexts(events) { | ||
| const sessions = new Map(); | ||
| const agents = new Map(); | ||
| const spawningTools = new Map(); | ||
| return events.map(event => { | ||
| const data = event.data ?? {}; | ||
| const sourceKey = JSON.stringify([event.provenance?.component, event.provenance?.phase, event.provenance?.path]); | ||
| const rootKey = JSON.stringify([sourceKey]); | ||
| const rootSessionId = sessions.get(rootKey); | ||
| let agentId = event.agentId || data.agentId || undefined; | ||
| let parentToolCallId = event.parent_tool_use_id || event.parentToolUseId || event.parentToolCallId || data.parentToolUseId || data.parent_tool_use_id || data.parentToolCallId || undefined; | ||
| if (event.type === "subagent.started" && agentId !== undefined) { | ||
| const agentKey = JSON.stringify([sourceKey, rootSessionId, agentId]); | ||
| agents.set(agentKey, { parentToolCallId: data.toolCallId, parentAgentId: data.parentId }); | ||
| if (data.toolCallId !== undefined) { | ||
| const toolKey = JSON.stringify([sourceKey, rootSessionId, data.toolCallId]); | ||
| const instances = spawningTools.get(toolKey) ?? new Set(); | ||
| instances.add(agentId); | ||
| spawningTools.set(toolKey, instances); | ||
| } | ||
| } | ||
| if (agentId === undefined && parentToolCallId !== undefined) { | ||
| const instances = spawningTools.get(JSON.stringify([sourceKey, rootSessionId, parentToolCallId])); | ||
| if (instances?.size === 1) agentId = instances.values().next().value; | ||
| } | ||
| const agent = agents.get(JSON.stringify([sourceKey, rootSessionId, agentId])); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This keys child state off 💡 Why this blocks the mergeThe new correlation logic assumes the root session has already been seen, but merged/untimed streams can delay lifecycle records. In that ordering, the child is inserted under Please avoid baking
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Root session IDs are resolved across the event stream before agent identity registration, so child starts preceding root initialization correlate with later child events. Covered by a regression test in commit |
||
| parentToolCallId ??= agent?.parentToolCallId; | ||
| const parentAgentId = data.parentAgentId ?? agent?.parentAgentId; | ||
| const ownerKey = agentId !== undefined ? ["agent", agentId] : parentToolCallId !== undefined ? ["tool", parentToolCallId] : []; | ||
| const sessionKey = JSON.stringify([sourceKey, ...ownerKey]); | ||
| const explicitSessionId = event.session_id ?? event.sessionId ?? data.sessionId ?? data.session_id; | ||
| if (["session.start", "session.init"].includes(event.type) && explicitSessionId !== undefined) sessions.set(sessionKey, explicitSessionId); | ||
| const sessionId = explicitSessionId ?? sessions.get(sessionKey) ?? rootSessionId; | ||
| return { | ||
| sourceKey, | ||
| rootSessionId: sessions.get(rootKey), | ||
| sessionId, | ||
| agentId, | ||
| parentAgentId, | ||
| parentToolCallId, | ||
| nested: agentId !== undefined || parentToolCallId !== undefined, | ||
| scope: JSON.stringify([sourceKey, sessionId, ...ownerKey]), | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| /** @param {any} usage @returns {Record<string, any>|undefined} */ | ||
| function normalizeSessionUsage(usage) { | ||
| // Usage is a named-field object; arrays cannot represent its token fields. | ||
|
|
@@ -173,7 +233,7 @@ | |
| if (typeof input === "string") { | ||
| try { | ||
| input = JSON.parse(input); | ||
| } catch { | ||
|
Check warning on line 236 in actions/setup/js/agent_session.cjs
|
||
| // Preserve malformed provider arguments verbatim. | ||
| } | ||
| } | ||
|
|
@@ -196,7 +256,7 @@ | |
| if (typeof input === "string") { | ||
| try { | ||
| input = JSON.parse(input); | ||
| } catch { | ||
|
Check warning on line 259 in actions/setup/js/agent_session.cjs
|
||
| // Preserve malformed provider arguments verbatim. | ||
| } | ||
| } | ||
|
|
@@ -331,13 +391,14 @@ | |
| /** | ||
| * Select snapshots field by field, never sum snapshots or erase prior known values. | ||
| * @param {Array<any>} events | ||
| * @param {{includeNested?: boolean}} [options] | ||
| * @returns {Record<string, any>|undefined} | ||
| */ | ||
| function selectSessionResult(events) { | ||
| function selectSessionResult(events, { includeNested = false } = {}) { | ||
| const normalized = normalizeAgentSession(events); | ||
| const claudeSessions = new Map(); | ||
| for (const event of normalized) { | ||
| if (event.type !== "session.result" || event.data.sourceEngine !== "claude" || event.parent_tool_use_id || typeof event.session_id !== "string") continue; | ||
| if (event.type !== "session.result" || event.data.sourceEngine !== "claude" || isNestedSessionEvent(event) || typeof event.session_id !== "string") continue; | ||
| claudeSessions.set(event.session_id, [...(claudeSessions.get(event.session_id) ?? []), { ...event, session_id: undefined }]); | ||
| } | ||
| if (claudeSessions.size > 1) { | ||
|
|
@@ -354,7 +415,7 @@ | |
| } | ||
| } | ||
| for (const event of normalized) { | ||
| if (event.type !== "session.result" || event.data.sourceEngine !== "claude" || event.parent_tool_use_id || typeof event.session_id === "string") continue; | ||
| if (event.type !== "session.result" || event.data.sourceEngine !== "claude" || isNestedSessionEvent(event) || typeof event.session_id === "string") continue; | ||
| // Unassigned diagnostics are valid evidence, but their usage may overlap a named session. | ||
| for (const key of ["errors", "permissionDenials"]) { | ||
| if (Array.isArray(event.data[key])) aggregate[key] = [...(aggregate[key] ?? []), ...structuredClone(event.data[key])]; | ||
|
|
@@ -367,7 +428,7 @@ | |
| let result; | ||
| for (const event of normalized) { | ||
| if (event.type !== "session.result") continue; | ||
| if (event.data.sourceEngine === "claude" && event.parent_tool_use_id) continue; | ||
| if (!includeNested && isNestedSessionEvent(event)) continue; | ||
| result ??= {}; | ||
| const data = event.data; | ||
| for (const [key, value] of Object.entries(data)) { | ||
|
|
@@ -384,9 +445,9 @@ | |
| return result; | ||
| } | ||
|
|
||
| /** @param {Array<any>} events @returns {any|undefined} */ | ||
| function projectSessionResult(events) { | ||
| const data = selectSessionResult(events); | ||
| /** @param {Array<any>} events @param {{includeNested?: boolean}} [options] @returns {any|undefined} */ | ||
| function projectSessionResult(events, options) { | ||
| const data = selectSessionResult(events, options); | ||
| if (!data) return undefined; | ||
| return { | ||
| type: "result", | ||
|
|
@@ -404,6 +465,7 @@ | |
| let data; | ||
| for (const event of normalizeAgentSession(events)) { | ||
| if (event.type !== "session.init" && event.type !== "session.start") continue; | ||
| if (isNestedSessionEvent(event)) continue; | ||
| data ??= {}; | ||
| for (const [key, value] of Object.entries(event.data)) { | ||
| if (value !== undefined) data = { ...data, [key]: structuredClone(value) }; | ||
|
|
@@ -535,6 +597,7 @@ | |
| function observedSessionModel(events) { | ||
| let model; | ||
| for (const event of events) { | ||
| if (isNestedSessionEvent(event)) continue; | ||
| const value = event?.type === "system" && event.subtype === "init" ? event.model : ["session.init", "session.start", "pi.message_snapshot"].includes(event?.type) ? event.data?.model : undefined; | ||
| if (typeof value === "string" && value.length) model = value; | ||
| } | ||
|
|
@@ -545,6 +608,8 @@ | |
| isSessionEvent, | ||
| isTokenCount, | ||
| isMetric, | ||
| isNestedSessionEvent, | ||
| sessionEventContexts, | ||
| normalizeSessionUsage, | ||
| accumulateSessionUsage, | ||
| reconcileSessionUsage, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
sessionEventContextsis a single ~45-line function juggling fourMaps and five derived JSON-string keys (sourceKey,rootKey,agentKey,toolKey,sessionKey) to resolve agent identity, session, and scope in one pass. The interface is deep (simple call, rich logic) but the body reuses the word "key" for five different concepts, which makes it hard to trace without re-reading line by line.💡 Suggested refactor
Consider extracting the agent-identity resolution (lines ~58–72: subagent registration, orphan
agentIdinference fromspawningTools) into a named helper, e.g.resolveAgentIdentity(event, sourceKey, rootSessionId, sessions, agents, spawningTools), sosessionEventContextsreads as: resolve identity → resolve session → build scope. This keeps the rich behavior but gives each phase a name that matches the domain vocabulary already used in comments ("agent instance is authoritative", "deprecated parent-tool markers are only the identity fallback").Also worth a few focused unit tests directly against
sessionEventContexts(not just viacopilot_nested_sessions.test.cjsend-to-end flows) for: orphanparentToolCallIdwith 0, 1, and >1 candidatespawningToolsinstances — theinstances?.size === 1branch silently no-ops for the >1 case, which is a plausible real-world input (concurrent subagent calls from one tool).@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored identity resolution into named helpers with distinct identity terminology, and added tests for zero, one, and ambiguous lifecycle candidates plus child-before-root ordering. Verified in commit
446f52f.