Repository navigation
Normalize engine session traces into shared events - #67632
Conversation
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>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
🟡 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 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.
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
There was a problem hiding this comment.
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'sknowngate forassistant.message_updatehardcodes Claude's four streamsourceTypevalues. 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_updateschema definition is also missingadditionalProperties: 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.cjscross-engine suite, and careful preservation of native snapshots (CodexnativeSnapshots, Pi non-text block history) without replaying duplicate text — nice adherence to deep-module principles for the per-engine normalizers themselves.
Positive Highlights
- ✅
normalizeClaudeObservations/normalizeEngineSessionEventsmigration path cleanly separates "upgrade saved records" from "parse live records" without duplicating logic. - ✅ Idempotency is preserved (
normalizeCodexSession(events) === eventsverified 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
|
@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: f438874
|
…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>
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>
|
In response to comment 6105890046: Accepted the assessment; no additional code change is requested. This batch does not merge the PR. |
|
In response to comment 6105890290: Accepted; the gate requests no ADR change, so none was added. |
|
In response to comment 6105890611: Accepted as a completion notification. The separate skills-review findings are addressed and their threads are resolved. |
|
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. |
|
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. |
|
In response to comment 6105950816: Accepted; the three saved Claude/Codex findings are already addressed in 74aa4a2 and 512702a. Managed review memory remains unchanged. |
|
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. |

Summary
Review the unified session parser for Copilot, Claude, Codex, Agy, and Pi, and reduce engine-specific observations without losing native trace evidence.
session.infoandtool.execution_updateevents. Stream boundaries useassistant.message_update; argument/signature observations remain metadata on core events. Migrate supported saved status wrappers without replaying compatibility text.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.
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-cjsnpm run typecheck -- --pretty falseinactions/setup/jsgo test ./actions/setup ./pkg/cli -run 'Test(SessionParser|SessionsDownload|SessionArtifactName|ValidateSessionJSONL|ParseAgentLogUsesSharedSessionParser)' -count=1PATH="/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