Skip to content

Normalize engine session traces into shared events - #67632

Merged
pelikhan merged 5 commits into
mainfrom
pelikhan-unified-trace-parsing
Oct 11, 2026
Merged

pelikhan merged 5 commits into
mainfrom
pelikhan-unified-trace-parsing

Conversation

@pelikhan

Copy link
Copy Markdown
Collaborator

Summary

Review the unified session parser for Copilot, Claude, Codex, Agy, and Pi, and reduce engine-specific observations without losing native trace evidence.

  • Map Claude reasoning estimates, VCS/status notices, authentication, rate limits, and tool progress into existing session.info and tool.execution_update events. Stream boundaries use assistant.message_update; argument/signature observations remain metadata on core events. Migrate supported saved status wrappers without replaying compatibility text.
  • Map Codex tool snapshots, todo lists, and content-free boundaries to shared events. Retain message snapshot history on the selected core message, preserve partial tool output, and attach observed thread identity to messages and diagnostics.
  • Preserve Pi structured/non-text content across repeated message and transcript snapshots. Keep opaque native extensions supported, including historical Pi updates.
  • Update essential payload projection, generated schema, documentation, and regression coverage. Copilot and Agy already mapped the sampled conversation/tool traces correctly and remain behaviorally unchanged.

Live trace validation

Reprocessed 11 existing GitHub Actions runs, comparing against the pre-change parser for exact conversation/tool payloads and accounting, validating canonical/unified schemas and JSONL framing/order, and checking stable reparsing. No workflow runs were triggered. Downloaded artifacts and validation outputs remain outside the repository.

Engine Run IDs Coverage
Copilot 37713903842, 37866304925 Native session events, tools, reasoning, shutdown accounting
Claude 37789661275, 37864205681 Successful tool-bearing sessions; all 172 custom status wrappers mapped to shared events
Codex 36850958249, 36909965579, 37789661484, 37863790523 MCP results, command failure, successful turns, repeated provider failures
Agy 37848575608 Agent execution succeeded; overall conformance run failed. Only one available live Agy run; additional protocol regressions cover reasoning, step correlation, and structured output
Pi 37789661712, 37865831889 Successful and failed workflows; 10-turn and 365-turn streams with partial tool output

New boundary, structured-content, signature, and snapshot cases also have synthetic protocol regressions; these are not claimed as live coverage.

Validation

  • make fmt-cjs && make lint-cjs
  • npm run typecheck -- --pretty false in actions/setup/js
  • Focused Vitest parser/projection/rendering suites: 634 tests passed across 20 files
  • go test ./actions/setup ./pkg/cli -run 'Test(SessionParser|SessionsDownload|SessionArtifactName|ValidateSessionJSONL|ParseAgentLogUsesSharedSessionParser)' -count=1
  • PATH="/opt/homebrew/bin:$PATH" make agent-report-progress (Bash 5 required on macOS): build, lint, schema freshness, and impacted JavaScript tests passed; impacted Go selection skipped because no Go sources changed

Map Claude controls and status notices and Codex snapshots into standard session events. Preserve Pi structured content, legacy status normalization, and exact trace metadata with regression coverage across all five engines.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@pelikhan
pelikhan marked this pull request as ready for review October 11, 2026 05:35
Copilot AI balanced review requested due to automatic review settings October 11, 2026 05:35
@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

Lean already. Ship.

Generated by Ponytail Reviewer for #67632

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ 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 for PR #67632: the PR does not carry the 'implementation' label (has_implementation_label=false) and has 0 new lines in default business logic directories (src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/), which is below the 100-line threshold. No custom .design-gate.yml is present.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

No GitHub write could be emitted because bash execution requires unavailable user approval in this environment; review findings are reported in the agent response instead.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Codex orphan updates can become invented starts, and Pi cross-type snapshots can produce duplicate messages.

2 open findings
What changed in this PR

Normalizes engine-specific session traces into shared events while preserving native evidence and unified projections.

Changes:

  • Maps Claude and Codex observations to shared session/tool events.
  • Preserves Pi structured content and snapshot history.
  • Updates schemas, documentation, rendering, and regression coverage.
File Description
docs/​src/​content/​docs/​specs/​unified-agent-session-specification.md Documents new normalization behavior.
docs/​public/​schemas/​unified-session.schema.json Extends the unified event schema.
actions/​setup/​js/​unified_session.test.cjs Updates session identity expectations.
actions/​setup/​js/​unified_session_payload.cjs Projects new shared event fields.
actions/​setup/​js/​types/​unified_session.d.ts Adds corresponding event types.
actions/​setup/​js/​pi_session.cjs Preserves structured Pi message blocks.
actions/​setup/​js/​parse_claude_log.cjs Exposes Claude saved-event migration.
actions/​setup/​js/​engine_session_mapping.test.cjs Adds cross-engine mapping regressions.
actions/​setup/​js/​codex_session.test.cjs Updates Codex normalization tests.
actions/​setup/​js/​codex_session.cjs Maps snapshots, todos, and thread context.
actions/​setup/​js/​claude_session.test.cjs Updates Claude mapping expectations.
actions/​setup/​js/​claude_session.cjs Normalizes Claude statuses and stream events.
actions/​setup/​js/​claude_session_audit.test.cjs Covers Claude audit trace behavior.
actions/​setup/​js/​agent_session_render.test.cjs Updates rendering regressions.
actions/​setup/​js/​agent_session_render.cjs Handles shared message-update boundaries.

🧠 Review effort: Balanced


💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread actions/setup/js/codex_session.cjs
Comment thread actions/setup/js/pi_session.cjs
@github-actions github-actions Bot mentioned this pull request Oct 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

Peek at saved memory (pr-code-quality-reviewer)
reviewed_at: 2026-10-11T05:38:45.094+00:00
review_event: REQUEST_CHANGES
top_themes:
  - claude-status-notices-misclassified-as-assistant-messages
  - claude-malformed-tool-argument-stream-leaks-native-wrapper
  - codex-content-free-user-or-reasoning-boundaries-mislabeled
files_reviewed:
  - actions/setup/js/claude_session.cjs
  - actions/setup/js/codex_session.cjs
  - actions/setup/js/pi_session.cjs
  - actions/setup/js/unified_session_payload.cjs
  - actions/setup/js/agent_session_render.cjs
  - actions/setup/js/engine_session_mapping.test.cjs
comment_count: 3

Note

This comment is managed by comment memory.

Expand the saved memory block to view or edit the persistent context for this thread.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 67.5 AIC · ⌖ 6.22 AIC · ⊞ 21.2K · ◷
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: the new shared-event mappings still misclassify some provider-control records, so the normalized trace can report notices as assistant replies and leak engine-specific wrappers on malformed stream input.

### Blocking themes- Claude `system/status` / `compact_boundary` now map to `session.info`, but the same records still fall through into `assistant.message` text, so a provider notice becomes a fake model answer. - Claude malformed `input_json_delta` currently escapes normalization and leaves a raw `claude.stream_event` in the output. - Codex content-free `user_message` / `reasoning` snapshots are emitted as `assistant.message_update`, which changes the speaker/type before any content exists.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 67.5 AIC · ⌖ 6.22 AIC · ⊞ 21.2K
Comment /review to run again

Comment thread actions/setup/js/claude_session.cjs Outdated
Comment thread actions/setup/js/codex_session.cjs Outdated
Comment thread actions/setup/js/claude_session.cjs Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /codebase-design and /tdd — requesting changes on one concrete field-leakage bug in the shared assistant.message_update projection seam.

📋 Key Themes & Highlights

Key Themes

  • Leaky seam: unified_session_payload.cjs's known gate for assistant.message_update hardcodes Claude's four stream sourceType values. Codex (item.started/item.updated/item.completed) and Pi (message_update, future deltas) message-update events fall through to the unfiltered opaque-clone path, bypassing the field allowlist that exists precisely to bound what reaches the unified trace. Verified via direct repro (see inline comment).
  • Schema gap compounds it: the new assistant.message_update schema definition is also missing additionalProperties: false, unlike every sibling event block, so schema validation wouldn't flag the leak either.
  • Scope/method is otherwise sound: good use of live-run validation (11 runs), comprehensive new engine_session_mapping.test.cjs cross-engine suite, and careful preservation of native snapshots (Codex nativeSnapshots, Pi non-text block history) without replaying duplicate text — nice adherence to deep-module principles for the per-engine normalizers themselves.

Positive Highlights

  • ✅ normalizeClaudeObservations/normalizeEngineSessionEvents migration path cleanly separates "upgrade saved records" from "parse live records" without duplicating logic.
  • ✅ Idempotency is preserved (normalizeCodexSession(events) === events verified independently) and snapshot/history handling avoids mutating source records.
  • ✅ Spec and schema docs were updated alongside behavior, keeping the unified-session spec as the source of truth.

Note: given the diff size, review focused on the five highest-impact files identified by triage (claude_session.cjs, codex_session.cjs, unified_session_payload.cjs, the new test file, and the type definitions); exhaustive line-by-line coverage of docs/schema prose was not attempted.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 115.3 AIC · ⌖ 14.8 AIC · ⊞ 10.3K
Comment /matt to run again

Comment thread actions/setup/js/unified_session_payload.cjs Outdated
Comment thread docs/public/schemas/unified-session.schema.json
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (actions/setup/js/codex_session.cjs:207): An orphan item.updated that includes invocation fields still enters the first branch and is emitted as tool.execution_start; this new update mapping only runs when a prior start exists (or the update lacks invocation fields). That contradicts the documented item.updated lifecycle and can report an execution start that was never observed. Handle item.updated first as a partial update while caching its invocation fields for a later completion. - Normalize engine session traces into shared events #67632 (comment)
  3. Review (actions/setup/js/pi_session.cjs:290): Repeated Pi snapshots are only reconciled when the block at an index stays in the same category. If an observed atomic block is finalized as text/thinking/tool content (or vice versa), the prior event remains in its separate state map and this branch emits a second core message. For example, an image at index 0 followed by finalized text at index 0 currently produces both messages instead of treating the finalized block as the snapshot and retaining the earlier value as history. Reconcile content by index across all block categories before dispatching by part.type. - Normalize engine session traces into shared events #67632 (comment)
  4. Review (actions/setup/js/claude_session.cjs:328): The catch { continue; } on malformed input_json_delta leaves content_block_stop as raw claude.stream_event, so interrupted tool-call streams regress back to engine-specific wrappers. - Normalize engine session traces into shared events #67632 (comment)
  5. Review (actions/setup/js/codex_session.cjs:237): This fallback turns every content-free user_message and reasoning item into assistant.message_update, which swaps the speaker before the real content arrives. - Normalize engine session traces into shared events #67632 (comment)
  6. Review (actions/setup/js/claude_session.cjs:472): Mapped Claude status notices still fall through into emitBlock, so a provider notice becomes both session.info and assistant.message instead of staying non-conversational. - Normalize engine session traces into shared events #67632 (comment)
  7. Review (actions/setup/js/unified_session_payload.cjs:426): [/codebase-design] The seam between "known shape, select fields" and "unknown shape, pass through opaque" is now inconsistent for assistant.message_update: it only recognizes Claude's four sourceType values, so Codex (item.started/item.updated/item.completed) and Pi (message_update, or future deltas) events fall through to the opaque structuredClone(source) branch and leak every native field unfiltered into the unified trace — defeating the whole point of EVENT_FIELDS/MESSAGE_FIELDS for this event type. - Normalize engine session traces into shared events #67632 (comment)
  8. Review (docs/public/schemas/unified-session.schema.json:39): [/tdd] The new assistant.message_update schema block (unlike its siblings at lines 49/100/132/161/196) omits "additionalProperties": false, so the generated schema can't catch the field-leakage described in the other comment — a JSON-Schema regression test here would have failed loudly instead of silently passing extra fields through. - Normalize engine session traces into shared events #67632 (comment)

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: f438874
Sous-chef work: 1022f90d4298b46e11ca8941c8f6fa5bc02558c673ff5416113e4ea667497a90 31fd60802b5f25ef3bae73f337bd59617eb67410748fcbf65d70f6455f65c55e 4ef7a9e41119f8c87aa8179e25593463e17fe0cb688674add288851ab2f76b3e 6111df656269f249d4fccf9c0639e77d2d11b372049f5654ba42ab767fb1c4ba a1e8552796ad3694a816025e3d2981d9f627b41dab49a611bc783a3a87fff8a1 b477b740382ce9f1287d986eefaca7b1c20d6b88b5e7820f319e6413529e4148 ff0c1fb535ffb8eec599d6a2bc16a216755e6f2340a3b638b21848cc70cdd02f
Sous-chef state: 7524a40af80d0c73b053a8c1c6b4e1384c991f87846e1228729a11022685128f

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 10.5 AIC · ⌖ 6.59 AIC · ⊞ 1K · ◷
Comment /souschef to run again

Copilot AI and others added 2 commits October 11, 2026 07:01
…e-parsing

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
pelikhan and others added 2 commits October 11, 2026 00:36
Preserve observed Pi execution starts across category corrections and retain essential historical update metadata in the closed shared projection. Expand lifecycle, boundary, speaker and schema regressions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@pelikhan

Copy link
Copy Markdown
Collaborator Author

In response to comment 6105890046: Accepted the assessment; no additional code change is requested. This batch does not merge the PR.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

In response to comment 6105890290: Accepted; the gate requests no ADR change, so none was added.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

In response to comment 6105890611: Accepted as a completion notification. The separate skills-review findings are addressed and their threads are resolved.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

In response to comment 6105892021: Partially accepted; this notification contains no actionable finding itself. The separately listed parser findings are addressed in 74aa4a2 and 512702a.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

In response to comment 6105893475: Accepted the reported limitation; skipped analysis is not a test-quality pass. Separate local regression validation and 11 recorded live traces passed; no CI rerun was triggered.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

In response to comment 6105950816: Accepted; the three saved Claude/Codex findings are already addressed in 74aa4a2 and 512702a. Managed review memory remains unchanged.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

In response to comment 6106447019: Accepted; all seven findings are addressed and resolved, and make merge-main updated the branch through 6b97fe9 in merge commit 7222a08. The final local gate passed and the update was pushed; no CI rerun or PR merge was requested or performed.

@pelikhan
pelikhan merged commit ea474b5 into main Oct 11, 2026
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants