Repository navigation
Conversation
Normalize scoped message and tool evidence, reasoning streams, interrupted tool requests, and essential conversation metadata using sampled Copilot run artifacts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Separate child session identity, accounting, messages and tool correlation; recover stale historical projections from retained native evidence. Preserve SDK ancestry and active turns, with CI-backed nesting regressions and specification updates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #66906 does not have the implementation label (has_implementation_label=false) and has 0 new lines of code in default business logic directories (default_business_additions=0, threshold=100). No custom .design-gate.yml present.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs (pr-triage recommended these as an architecture_change). Left 3 inline comments — none blocking merge, but worth addressing to harden the correctness guarantees this PR is built on.
📋 Key Themes & Highlights
Key Themes
- Untested boundary condition: the
includeNestedheuristic inlog_parser_shared.cjs(deciding whether child-session accounting is folded into the main result) has no direct test, despite being central to the turn-count-inflation fix this PR targets. - Dense core abstraction:
sessionEventContexts()inagent_session.cjsis a deep, well-named function, but its body packs five distinct identity/session/scope concerns into one loop body using similarly-named*Keyvariables — a light extraction would make it easier to navigate and unit-test in isolation. - Silent ambiguity drop:
scopedAgentSessions()inunified_session_render.cjssilently discardssubagent.*events when child-group matching is ambiguous (0 or >1 candidates), which could quietly reintroduce the exact kind of miscounting bug this PR fixes.
Positive Highlights
- ✅ Strong real-world validation: replayed actual nested-agent runs and reported concrete before/after turn counts (101→41, 5→12) in the PR description — exactly the kind of evidence
/diagnosing-bugslooks for. - ✅ 463 new lines of regression tests (
copilot_nested_sessions.test.cjs,copilot_session_normalization.test.cjs) with dedicated CI fixtures covering nested-agent scenarios. - ✅ Careful preservation of existing identity semantics — explicit comments like "Child initialization must not change the root session" show the invariants were thought through, not just coded.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 46.8 AIC · ⌖ 14.8 AIC · ⊞ 10.1K
Comment /matt to run again
|
|
||
| const result = projectSessionResult(logEntries); | ||
| const accountingContexts = contexts.filter((_, index) => !logEntries[index].type.startsWith("subagent.")); | ||
| const includeNested = accountingContexts.length > 0 && accountingContexts.every(context => context.nested && context.scope === accountingContexts[0].scope); |
There was a problem hiding this comment.
[/codebase-design] includeNested uses every(context => context.nested && context.scope === accountingContexts[0].scope) to decide whether a log is "fully nested" and should include child-session accounting — but there's no test exercising this specific branch (grep for includeNested across *.test.cjs returns nothing).
💡 Why this matters
This is exactly the kind of boundary condition /tdd flags: a one-line heuristic with no corresponding "when a child-only log is fully nested, result includes nested usage" test. Since the whole PR's thesis is turn-count correctness for nested agents, an untested fork like this is a likely regression magnet — e.g. what happens when accountingContexts is empty after filtering subagent events, or when scopes differ only by parentAgentId?
Suggest adding a focused unit test in copilot_nested_sessions.test.cjs that asserts convertCopilotEventsToLegacyLogEntries includes/excludes nested usage correctly for: (a) all-nested uniform scope, (b) mixed root+nested scopes, (c) empty accountingContexts.
@copilot please address this.
There was a problem hiding this comment.
Added coverage for consistent child-only accounting, mixed root/child scopes, and empty accounting inputs in copilot_nested_sessions.test.cjs; verified in commit 446f52f.
| 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]); |
There was a problem hiding this comment.
[/codebase-design] sessionEventContexts is a single ~45-line function juggling four Maps 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 agentId inference from spawningTools) into a named helper, e.g. resolveAgentIdentity(event, sourceKey, rootSessionId, sessions, agents, spawningTools), so sessionEventContexts reads 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 via copilot_nested_sessions.test.cjs end-to-end flows) for: orphan parentToolCallId with 0, 1, and >1 candidate spawningTools instances — the instances?.size === 1 branch 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.
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.
| const root = groups.find(group => !group.context.nested && group.context.sessionId === context.rootSessionId); | ||
| if (root) root.events.push(event); | ||
| const children = groups.filter(group => group.context.agentId === context.agentId && group.context.nested); | ||
| const child = children.find(group => group.context.scope === context.scope) ?? (children.length === 1 ? children[0] : undefined); |
There was a problem hiding this comment.
[/diagnosing-bugs] The child-group matching heuristic falls back to children.length === 1 ? children[0] : undefined — when there are 0 or >1 ambiguous child candidates for a subagent.* event, the event is silently dropped from every group instead of being attached or logged as unresolved. Given the PR's goal is turn-count/ancestry correctness, a silently-dropped subagent event could reintroduce the same kind of undercount bug this PR fixes, without any visible symptom.
💡 Suggested fix
Consider instrumenting the "ambiguous or missing child" case (e.g. attach to all matching children, or fall through to the root group with a diagnostic marker) rather than dropping silently, and add a regression test in copilot_nested_sessions.test.cjs for the >1-candidate case (e.g. two sibling agents sharing an agentId due to reused IDs across different parentToolCallIds — this scenario is explicitly called out in the PR description as a risk).
@copilot please address this.
There was a problem hiding this comment.
Unmatched or ambiguous lifecycle records are retained in an unresolved group or the root conversation instead of being dropped; regression tests cover both cases. Verified in commit 446f52f.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Request changes
The nested-session normalization is moving in the right direction, but the new identity plumbing still has a few correlation holes that can silently misattribute child events.
Blocking themes
- child/session correlation still depends on event ordering in
sessionEventContexts() - the SDK adapter does not persist enough ancestry to make standalone child artifacts self-describing
pendingToolCallscan still de-correlate a child tool start from its completion when the SDK switches from legacy to modern identifiers mid-stream
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 87.5 AIC · ⌖ 7.75 AIC · ⊞ 19.8K
Comment /review to run again
| 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])); |
There was a problem hiding this comment.
This keys child state off rootSessionId before the current event can establish that root ID, so a subagent.started that arrives before session.start is stored under undefined and every later lookup misses it.
💡 Why this blocks the merge
The 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 [sourceKey, undefined, agentId], then later events are read from [sourceKey, "root", agentId] and lose both ancestry and tool correlation. That splits one child conversation into multiple scopes and corrupts turn/accounting projections.
Please avoid baking rootSessionId into the agent/tool map keys until it is known, or re-key pending child entries when the root session.start / session.init arrives.
There was a problem hiding this comment.
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 446f52f.
| const metadata = Object.fromEntries(["id", "parentId", "agentId", "ephemeral"].filter(key => Object.hasOwn(native, key)).map(key => [key, native[key]])); | ||
| const entry = { type, timestamp: timestamp ?? new Date().toISOString(), ...metadata, data }; | ||
| const correlation = Object.fromEntries( | ||
| ["sessionId", "messageId", "originatingMessageId", "reasoningId", "interactionId", "turnId", "apiCallId", "model", "parentToolCallId", "parentAgentTaskId"] |
There was a problem hiding this comment.
The adapter now claims to preserve child metadata, but it still drops parentAgentId here, so standalone SDK artifacts cannot reconstruct the tree once subagent.started is missing or trimmed.
💡 Why this matters
The downstream normalization path already consumes data.parentAgentId when it is present, but writeEvent() never persists that field from native SDK events. Any partial child log, replay, or sanitized fixture that lacks the original lifecycle event therefore loses ancestry even though the SDK provided it.
Please include parentAgentId in the copied correlation fields so the persisted JSONL is self-describing instead of depending on a separate lifecycle record surviving collection.
There was a problem hiding this comment.
The SDK writer now persists parentAgentId among correlation metadata; the SDK driver test covers retained child ancestry. Verified in commit 446f52f.
|
|
||
| // Subscribe to all session events and serialise the ones we care about. | ||
| session.on(event => { | ||
| const owner = event.agentId || event.data?.agentId || event.data?.parentToolCallId || "main"; |
There was a problem hiding this comment.
This key is not stable across SDK shapes: a start can be indexed under parentToolCallId and the matching completion under agentId, which makes the completion fall back to unknown and can leave the in-flight tool map in the wrong state.
💡 Concrete failure mode
You are explicitly supporting both modern agentId and legacy parentToolCallId correlation, but pendingToolCalls chooses one or the other per event. If a child tool.execution_start is emitted before the SDK starts populating agentId, it is stored under ["spawn-left", "call-1"]; once the completion arrives with agentId: "left", lookup switches to ["left", "call-1"] and misses the original metadata.
That drops toolName / mcpServerName on completions and can also strand the original key in pendingToolCalls. Please normalize both shapes onto the same owner identity, or alias both keys before reading and writing the map.
There was a problem hiding this comment.
SDK tool ownership resolves lifecycle-backed parent-tool IDs to agent IDs before creating the pending-call key, keeping mixed identity shapes correlated. Verified in commit 446f52f.
There was a problem hiding this comment.
🟡 Changes recommended
Identity-key inconsistencies can suppress valid projections and mispair nested SDK tool completions.
2 open findings
What changed in this PR
This PR improves Copilot session normalization and isolates nested-agent conversations and accounting.
Changes:
- Adds source/session/agent-scoped message, tool, reasoning, and accounting normalization.
- Preserves nested-agent ancestry across collection, rendering, and SDK execution.
- Updates schemas, documentation, fixtures, and regression coverage.
| File | Description |
|---|---|
docs/src/content/docs/specs/unified-agent-session-specification.md |
Documents v1.7 behavior. |
docs/public/schemas/unified-session.schema.json |
Expands unified event schemas. |
docs/public/schemas/agent-session.schema.json |
Adds reasoning-token usage. |
actions/setup/js/unified_session.test.cjs |
Updates collector regressions. |
actions/setup/js/unified_session.cjs |
Excludes nested initialization from root identity. |
actions/setup/js/unified_session_render.cjs |
Separates nested conversations. |
actions/setup/js/unified_session_payload.cjs |
Compacts correlation fields. |
actions/setup/js/types/unified_session.d.ts |
Defines new unified payload types. |
actions/setup/js/types/agent_session.d.ts |
Types reasoning-token usage. |
actions/setup/js/subagent_session_render.cjs |
Protects root metrics and shows status. |
actions/setup/js/log_parser_shared.cjs |
Scopes tool pairing and accounting. |
actions/setup/js/log_parser_bootstrap.cjs |
Selects root session starts. |
actions/setup/js/fixtures/copilot_nested_ci.cjs |
Adds nested-session fixtures. |
actions/setup/js/fixtures/copilot_ci_messages.cjs |
Adds message normalization fixtures. |
actions/setup/js/copilot_session.cjs |
Implements scoped Copilot normalization. |
actions/setup/js/copilot_session_normalization.test.cjs |
Tests messages, streams, and tools. |
actions/setup/js/copilot_sdk_session.cjs |
Preserves SDK identity and turn state. |
actions/setup/js/copilot_sdk_driver.test.cjs |
Tests nested SDK behavior. |
actions/setup/js/copilot_nested_sessions.test.cjs |
Tests ancestry and accounting isolation. |
actions/setup/js/codex_session.cjs |
Normalizes reasoning-token aliases. |
actions/setup/js/agent_session.cjs |
Adds nested-context helpers. |
actions/setup/js/agent_session_render.cjs |
Preserves private publication identities. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| const owner = event.agentId || event.data?.agentId || event.data?.parentToolCallId || "main"; | ||
| const toolKey = JSON.stringify([owner, event.data?.toolCallId]); |
There was a problem hiding this comment.
Mixed SDK identity metadata now resolves through the parent-tool-to-agent alias before pending-call lookup/deletion; the regression suite passes in commit 446f52f.
| const originKey = JSON.stringify([context.sourceKey, context.agentId, context.agentId === undefined ? context.parentToolCallId : undefined, event.id, event.timestamp]); | ||
| const observedScope = context.scope; | ||
| const scope = event.copilotProjection && event.id !== undefined ? (originScopes.get(originKey) ?? observedScope) : observedScope; |
There was a problem hiding this comment.
Projection idempotency buckets now include source/session/agent scope, preventing identical IDs/timestamps in independent scopes from suppressing each other. Verified in commit 446f52f.
There was a problem hiding this comment.
Impeccable review (no UI changes — direct correctness review applied)
This PR is non-UI (session-normalization logic in actions/setup/js), so Impeccable's audit/critique/harden modes don't directly apply. Per the fallback guidance, I applied a direct review focused on correctness.
Verification performed: since npm install is unavailable offline (no vitest), I manually re-derived several of the PR's own test scenarios against the changed code using plain Node (nested-session turn/usage separation, tool-ID correlation under reused IDs, reasoning/message delta collapsing, usage dedup by apiCallId, and renderSubagentSummary output). All outputs matched the PR's stated expectations exactly.
No blocking issues found. Changes are internally consistent, well-scoped, and the accompanying spec/schema updates track the code changes.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
codeload.github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "codeload.github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 108.7 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 01b3e64
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 446f52f
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Why
Existing Copilot run artifacts lose message/request correlation, leave some reasoning and interrupted tool requests unnormalized, and mix nested-agent activity into the main session. In real nested runs, this inflated main-session turn counts from 41 to 101 and from 5 to 12.
Approach
Existing-run evidence
Replayed three Smoke Copilot runs and two genuine nested-agent runs: nested research and failed-child/fallback. Exact message text and tool counts remain unchanged. Known conversation payloads in the smoke samples are 32-36% smaller.
Nested research now reports 41 main turns and child counts of 45, 3, and 12, preserving all 156 tool starts. The failed-child run reports 5 main turns separately from 7 child turns, preserving all 9 tool starts. Both native and historical canonical artifacts produce deterministic, idempotent normalization; per-agent accounting snapshots remain unchanged.
Validation
npm --prefix actions/setup/js run typechecknpm --prefix actions/setup/js run schema:session:checkmake fmt-cjs,make lint-cjs, andmake agent-report-progresspassed.The serialized unified-session format remains version 1. Ancestry is derived from lifecycle payloads, never the native envelope's chronological
parentId; collision-avoidance tool IDs are display-only.