[rendering-scripts] Fix Pi log parser for the v3 streaming schema (empty step summaries)#45414
Conversation
The Pi CLI's `--print --mode json` output now emits a v3 streaming schema (session / turn_start / turn_end / tool_execution_* / agent_end), but parse_pi_log.cjs only understood the legacy flat schema (init/assistant/tool_use/tool_result/result). As a result, every current Pi-engine run rendered an empty step summary — no conversation, no tool calls, no token/turn stats — and the OTEL result-entry enrichment was skipped because no `result` event exists in v3. Add additive v3 support: detect the schema and, for v3, render from the finalized `turn_end` messages (text + toolCall) paired with `tool_execution_end` results by id, emitting tool_use before tool_result so the shared renderer resolves tool names and orders the conversation correctly. Token stats sum per-turn output and take peak per-turn input. The legacy path is unchanged. Adds v3 unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
automated-fix
javascript
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions in default business logic directories: src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds Pi v3 streaming-schema support while preserving legacy parsing.
Changes:
- Detects and transforms v3 conversation/tool events.
- Reconstructs token statistics and telemetry results.
- Adds v3 parser tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/parse_pi_log.cjs |
Implements v3 parsing and statistics. |
actions/setup/js/parse_pi_log.test.cjs |
Adds v3 behavior tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Medium
| type: "tool_result", | ||
| tool_use_id: id, | ||
| content: extractPiV3ResultText(res.result), | ||
| is_error: isPiV3ResultError(res.result), |
There was a problem hiding this comment.
Fixed in the latest commit — is_error now evaluates res.isError || isPiV3ResultError(res.result), reading the top-level event flag first (matching the Pi driver's event.isError check at pi_agent_core_driver.cjs:348).
| it("marks failed tool results as errors", () => { | ||
| const entries = transformPiV3Entries([ | ||
| { type: "session", version: 3, id: "s" }, | ||
| { type: "tool_execution_end", toolCallId: "t1", result: { status: "error", content: [{ type: "text", text: "boom" }] } }, |
There was a problem hiding this comment.
Updated in the latest commit — the test event now carries isError: true at the top level (no status: "error" in result), matching the real v3 event shape from the Pi driver.
| if (typeof usage.input === "number" && usage.input > maxInputTokens) { | ||
| maxInputTokens = usage.input; | ||
| } |
There was a problem hiding this comment.
Fixed — computePiV3Stats now sums input tokens across all turns (inputTokens += usage.input), matching the Pi driver's accumulation at pi_agent_core_driver.cjs:360. The "max" approach and its comment have been removed.
| expect(stats).not.toBeNull(); | ||
| expect(stats.turns).toBe(2); | ||
| expect(stats.output_tokens).toBe(55); // 40 + 15 | ||
| expect(stats.input_tokens).toBe(1200); // max(1000, 1200) |
There was a problem hiding this comment.
Updated — both stat assertions now expect 2,200 input tokens (1000 + 1200), and the result entry assertion reflects the same sum.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (6 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is well-structured and correct. The dual-schema detection is robust, the tool_use-before-tool_result ordering is properly enforced, and input-token peak vs output-token sum logic matches the described Pi v3 behaviour. Legacy path is untouched and the new tests cover all key scenarios (detection, ordering, stats, error results, OTEL entry). No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.1 AIC · ⌖ 5.12 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two low-severity findings, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Coverage gap for legacy path:
legacyPiStatsis unexported and has no direct test; a field rename would produce silent zero-stats. - Misleading fallback stats:
computePiV3Statsreturns a zero-token object rather thannullwhen turns are present but all lack usage data.
Positive Highlights
- ✅ Clean schema-detection heuristic (
isPiV3Schema) with a clear null-guard loop - ✅ Good ordering discipline: tool_use before tool_result within each turn
- ✅ Well-documented token-accounting rationale (input = max, output = sum) in JSDoc
- ✅ New test suite covers the v3 happy path, ordering, stats, error flags — solid baseline
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 40.9 AIC · ⌖ 4.58 AIC · ⊞ 6.6K
Comment /matt to run again
| isPiV3Schema, | ||
| transformPiV3Entries, | ||
| computePiV3Stats, | ||
| }; |
There was a problem hiding this comment.
[/tdd] legacyPiStats is not exported, so it can only be exercised indirectly via parsePiLog — but no legacy-schema fixture exists in the new test suite. A rename of result.stats field names would silently produce zero stats with no failing test.
💡 Suggested fix
Add legacyPiStats to module.exports and add unit tests:
it('extracts stats from a legacy result event', () => {
const stats = legacyPiStats([{ type: 'result', stats: { input_tokens: 100, output_tokens: 50, turns: 3, duration_ms: 1500 } }]);
expect(stats).toEqual({ input_tokens: 100, output_tokens: 50, turns: 3, duration_ms: 1500 });
});
it('returns null when no legacy result event', () => {
expect(legacyPiStats([{ type: 'init' }])).toBeNull();
});@copilot please address this.
| } | ||
| } | ||
|
|
||
| if (turns === 0 && !sawUsage) { |
There was a problem hiding this comment.
[/diagnosing-bugs] When turns > 0 but no usage is present (e.g. a future Pi build stops emitting per-turn usage), computePiV3Stats returns {input_tokens:0, output_tokens:0, turns:N} rather than null. The step summary would show a stats section with all-zero tokens, which is misleading rather than absent.
💡 Suggested fix
Change the guard to also return null when no usage data was observed:
if (!sawUsage) {
return null; // no token data available — omit the stats block entirely
}And add a test case:
it('returns null when turns exist but no usage fields', () => {
expect(computePiV3Stats([{ type: 'turn_end', message: { content: [] } }])).toBeNull();
});@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — 3 issues must be addressed before merge
The fix correctly identifies and solves the root cause (v3 streaming schema not handled). The logic for rendering turn_end messages, pairing tool calls with results, and preserving the legacy path is sound. However three issues in the new code need to be fixed:
Blocking issues
1. computePiV3Stats null-guard is wrong (line ~395): When turn_end events exist but none carry a usage field, the function returns a zero-filled stats object instead of null. OTEL token-usage metrics will receive turns with zero tokens rather than absent data — this is a silent data-quality regression.
2. isPiV3Schema triggers on a single v3 event (line ~217): A single stray envelope event (e.g. agent_start) in a legacy log forces the entire log through the v3 transformer, silently dropping all legacy assistant/tool_use/result content. Requiring at least one turn_end (the load-bearing event) alongside a v3 marker is a much stronger and safer heuristic.
3. duration_ms is hardcoded to 0 (line ~251): Every v3 run will silently report zero latency to OTEL. The session event already carries a timestamp; if agent_end does too, compute the wall-clock duration rather than hard-coding zero.
🔎 Code quality review by PR Code Quality Reviewer · 45.4 AIC · ⌖ 4.82 AIC · ⊞ 5.4K
Comment /review to run again
| } | ||
|
|
||
| if (turns === 0 && !sawUsage) { | ||
| return null; |
There was a problem hiding this comment.
computePiV3Stats returns misleading zero-token stats instead of null when turn_end events exist but carry no usage data.
💡 Detail and suggested fix
The null-guard at line ~395 is:
if (turns === 0 && !sawUsage) {
return null;
}If turn_end entries exist but carry no usage field (older v3 build, streaming error, truncated log), turns > 0 while sawUsage stays false. The function returns { input_tokens: 0, output_tokens: 0, turns: N, duration_ms: 0 } instead of null, so the caller emits a synthetic stats block with all-zero token counts into the OTEL enrichment path — downstream gh-aw.turns / token-usage metrics show turns with zero tokens rather than absent data.
Fix the guard:
if (!sawUsage) {
return turns === 0 ? null : { input_tokens: 0, output_tokens: 0, turns, duration_ms: 0 };
}Or simply return null when !sawUsage if downstream consumers prefer fully absent stats over partial data.
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
isPiV3Schema returns true on any single v3 event, forcing the entire log through the v3 path — a single stray v3 event in a legacy log silently drops the whole conversation.
💡 Detail and suggested fix
Any single matching event (including a bare {type:"agent_start"} injected by a wrapper script or log aggregator) causes isPiV3Schema to return true, after which transformPiV3Entries processes the log as v3 and ignores all legacy assistant / tool_use / result events. The resulting conversation will be empty or severely truncated with no error surfaced.
Two concrete failure scenarios:
- A legacy log that happens to include a session-management envelope at the top (e.g. from a launcher) returns
trueand renders nothing. - A partial v3 log with both schema types (mid-migration Pi build) silently loses all legacy-schema content.
Consider requiring at least one turn_end (the load-bearing event for v3 content) before committing to the v3 path:
This is a stronger signal: v3 logs always finalize turns with turn_end, so requiring both a v3 marker and at least one turn_end eliminates false positives from stray envelope events.
| } | ||
| } | ||
|
|
||
| // Initialization entry from the session event, with the model taken from the first |
There was a problem hiding this comment.
computePiV3Stats hardcodes duration_ms: 0, silently emitting incorrect latency data to OTEL consumers.
💡 Detail and suggested fix
The v3 stream includes timing data that makes duration_ms derivable. The session event has a timestamp field, and agent_end typically carries a timestamp too. Hardcoding 0 means every v3 run reports zero duration in OTEL spans and the step summary information section.
If the upstream agent_end event carries a timestamp, compute the wall-clock duration:
const sessionEvent = rawEntries.find(e => e.type === "session");
const agentEndEvent = rawEntries.find(e => e.type === "agent_end");
let duration_ms = 0;
if (sessionEvent && sessionEvent.timestamp && agentEndEvent && agentEndEvent.timestamp) {
duration_ms = new Date(agentEndEvent.timestamp) - new Date(sessionEvent.timestamp);
if (!isFinite(duration_ms) || duration_ms < 0) duration_ms = 0;
}If agent_end does not carry a timestamp in the real schema, document that duration_ms is intentionally unavailable rather than silently emitting 0.
Sighthound Security Scan — Run 29320112653126 total findings (113 outside testdata directories). Triage SummaryMost findings are false positives for this project:
Potentially Actionable Findings
Recommended Actions
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ob syntax Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Summary
Fixes the Pi log parser (
actions/setup/js/parse_pi_log.cjs) to handle the Pi CLI v3 streaming schema, which replaced the legacy flat schema in current Pi builds. Without this fix, step summaries for Pi-backed agent runs are empty — no conversation, no token stats.Problem
The Pi CLI
--mode jsonoutput schema changed between versions:init / assistant / tool_use / tool_result / resultevents.session,turn_start,turn_end,tool_execution_start,tool_execution_end,agent_end.The parser only handled the legacy schema, so v3 logs produced empty step summaries and no token usage stats.
Changes
actions/setup/js/parse_pi_log.cjsisPiV3Schema(rawEntries): scans entries for v3 marker event types and returns a boolean.transformPiV3Entries(rawEntries): rebuilds canonical log entries from v3 events. Indexestool_execution_endresults bytoolCallId, derives model from firstturn_end.message.model, emits tool_use entries before their paired tool_result entries.computePiV3Stats(rawEntries): sumsusage.inputandusage.outputacross allturn_endevents.legacyPiStats(rawEntries): extractsresult.statsfrom the legacy schema.parsePiLog(): detects schema once, dispatches to the correct transform and stats functions.isPiV3Schema,transformPiV3Entries,computePiV3Statsadded tomodule.exports.actions/setup/js/parse_pi_log.test.cjsNew
"Pi v3 streaming schema"test suite (6 tests): schema detection, end-to-end rendering, tool_use/tool_result ordering invariant, token aggregation across turns, OTEL enrichment on result entry, and error flag propagation..github/skills/agentic-workflows/SKILL.mdAdded
configure-agentic-engine.mdto the lazy-load file list.Compatibility
Legacy Pi logs are unchanged —
isPiV3Schemareturnsfalseand the originallegacyPiStats/transformPiEntriespaths are used as before.