From d869ad86e20917033acdaad428f12ae71ceebf54 Mon Sep 17 00:00:00 2001 From: ozymandiashh <234437643+ozymandiashh@users.noreply.github.com> Date: Wed, 1 Jul 2026 16:57:31 +0300 Subject: [PATCH] fix(pi): classify native skill loads as Skill, not Read (#588) (#590) Pi and OMP have no dedicated skill tool: a native skill load is emitted as an ordinary `read` tool call whose path points at the skill's SKILL.md (or a `skill://` URI in newer OMP builds). The parser mapped every read to the Read tool and never populated `skills`, so Pi/OMP sessions over-counted Reads AND always showed an empty "Skills & Agents" breakdown. Detect these reads (basename === 'SKILL.md', or a skill:// URI), extract the skill name (parent directory, or the URI segment), and surface the call as the `Skill` tool with the name recorded in `skills` -- exactly how the Claude parser represents a skill invocation. That both removes the Read over-count and lets the shared classifier tag the turn `general` so the Skills & Agents breakdown picks it up (populating a field the dashboard never received before). The path is read from arguments.path with a defensive arguments.file_path fallback. Tests cover SKILL.md / skill:// / file_path detection, a non-skill read staying a Read, and an end-to-end check that a parsed skill load reaches the classifier subCategory that feeds skillBreakdown. --- docs/providers/pi.md | 1 + src/providers/pi.ts | 49 ++++++++++- tests/providers/pi.test.ts | 162 +++++++++++++++++++++++++++++++++++-- 3 files changed, 204 insertions(+), 8 deletions(-) diff --git a/docs/providers/pi.md b/docs/providers/pi.md index 9427226..ca6e623 100644 --- a/docs/providers/pi.md +++ b/docs/providers/pi.md @@ -27,6 +27,7 @@ Per `::` when a response ID is present, falling back - Undefined token fields in `message.usage` are coerced to `0` (`pi.ts:156-159`); never `undefined`. - The provider name is taken from `source.provider` (`pi.ts:182`), not hard-coded. This matters because `pi.ts` is the parser for **both** Pi and OMP; see [`omp.md`](omp.md). - Tool-call content type is extracted from the message envelope (`pi.ts:169-176`). +- Pi/OMP have no dedicated skill tool: a native skill load is a `read` whose path points at a skill's `SKILL.md` (or a `skill://` URI in newer OMP builds). The parser surfaces these as the `Skill` tool and records the name in `skills` (mirroring the Claude parser) instead of counting a `Read`, so the shared classifier tags the turn `general` and the Skills & Agents breakdown picks it up (`skillLoadName`, issue #588). ## When fixing a bug here diff --git a/src/providers/pi.ts b/src/providers/pi.ts index 055d085..85d5f25 100644 --- a/src/providers/pi.ts +++ b/src/providers/pi.ts @@ -35,6 +35,34 @@ const toolNameMap: Record = { // Pre-sorted by key length descending so longer/more-specific keys match first const modelDisplayEntries = Object.entries(modelDisplayNames).sort((a, b) => b[0].length - a[0].length) +// Pi/OMP have no dedicated skill tool the way Claude Code does. A native skill +// load is emitted as an ordinary `read` tool call whose path points at the +// skill's `SKILL.md` (Pi resolves skills from many roots: ~/.pi/agent/skills, +// project .pi/skills, .agents/skills, package skills/, --skill ), or, in +// newer OMP builds, at a `skill://` URI. Left untouched these inflate the +// Read tool count and leave the Skills dimension empty (issue #588). Return the +// skill name when a read is really a skill load, else null so it stays a Read. +function skillLoadName(name: string | undefined, args: Record | undefined): string | null { + if (name !== 'read') return null + const raw = args?.['path'] ?? args?.['file_path'] + if (typeof raw !== 'string') return null + const path = raw.trim() + if (path.length === 0) return null + + if (path.startsWith('skill://')) { + const rest = path.slice('skill://'.length).replace(/^\/+/, '') + const first = rest.split(/[/?#]/)[0]?.trim() ?? '' + return first.length > 0 ? first : null + } + + // Match on the SKILL.md basename, not a directory prefix, because skill roots + // live in many locations. Split on both separators so Windows paths work. + const segments = path.split(/[\\/]/).filter(Boolean) + if (segments[segments.length - 1] !== 'SKILL.md') return null + const parent = segments[segments.length - 2]?.trim() + return parent && parent.length > 0 ? parent : null +} + type PiEntry = { type: string id?: string @@ -169,7 +197,25 @@ function createParser(source: SessionSource, seenKeys: Set): SessionPars seenKeys.add(dedupKey) const toolCalls = normalizeContentBlocks(msg.content).filter(c => c.type === 'toolCall' && c.name) - const tools = toolCalls.map(c => toolNameMap[c.name!] ?? c.name!) + + // A SKILL.md-loading read is surfaced as the `Skill` tool (not `Read`) + // and its name is recorded in `skills`. This mirrors how the Claude + // parser represents a skill invocation, so the shared classifier tags + // the turn `general` and the "Skills & Agents" breakdown picks it up, + // instead of over-counting a Read and leaving Skills empty (#588). + // Every other call stays a normal tool. + const tools: string[] = [] + const skills: string[] = [] + for (const c of toolCalls) { + const skill = skillLoadName(c.name, c.arguments) + if (skill !== null) { + skills.push(skill) + tools.push('Skill') + continue + } + tools.push(toolNameMap[c.name!] ?? c.name!) + } + const bashCommands = toolCalls .filter(c => c.name === 'bash') .flatMap(c => { @@ -193,6 +239,7 @@ function createParser(source: SessionSource, seenKeys: Set): SessionPars costUSD, tools, bashCommands, + skills, timestamp, speed: 'standard', deduplicationKey: dedupKey, diff --git a/tests/providers/pi.test.ts b/tests/providers/pi.test.ts index de1af7a..398d509 100644 --- a/tests/providers/pi.test.ts +++ b/tests/providers/pi.test.ts @@ -5,6 +5,39 @@ import { tmpdir } from 'os' import { createPiProvider } from '../../src/providers/pi.js' import type { ParsedProviderCall } from '../../src/providers/types.js' +import { classifyTurn } from '../../src/classifier.js' +import type { ParsedApiCall, ParsedTurn } from '../../src/types.js' + +// Mirrors src/parser.ts providerCallToTurn so we can assert that a Pi call's +// skills survive the classifier into `subCategory`, which is the sole input the +// session summary reads to build the "Skills & Agents" breakdown (#588). +function turnFromPiCall(call: ParsedProviderCall, userMessage = ''): ParsedTurn { + const apiCall: ParsedApiCall = { + provider: call.provider, + model: call.model, + usage: { + inputTokens: call.inputTokens, + outputTokens: call.outputTokens, + cacheCreationInputTokens: call.cacheCreationInputTokens, + cacheReadInputTokens: call.cacheReadInputTokens, + cachedInputTokens: call.cachedInputTokens, + reasoningTokens: call.reasoningTokens, + webSearchRequests: call.webSearchRequests, + }, + costUSD: call.costUSD, + tools: call.tools, + mcpTools: call.tools.filter(t => t.startsWith('mcp__')), + skills: call.skills ?? [], + subagentTypes: call.subagentTypes ?? [], + hasAgentSpawn: call.tools.includes('Agent'), + hasPlanMode: call.tools.includes('EnterPlanMode'), + speed: call.speed, + timestamp: call.timestamp, + bashCommands: call.bashCommands, + deduplicationKey: call.deduplicationKey, + } + return { userMessage, assistantCalls: [apiCall], timestamp: call.timestamp, sessionId: call.sessionId } +} let tmpDir: string @@ -48,14 +81,20 @@ function assistantMessage(opts: { output?: number cacheRead?: number cacheWrite?: number - tools?: Array<{ name: string; command?: string }> + tools?: Array<{ name: string; command?: string; path?: string; filePath?: string }> }) { - const content = (opts.tools ?? []).map(t => ({ - type: 'toolCall', - id: `call-${t.name}`, - name: t.name, - arguments: t.command !== undefined ? { command: t.command } : {}, - })) + const content = (opts.tools ?? []).map(t => { + const args: Record = {} + if (t.command !== undefined) args['command'] = t.command + if (t.path !== undefined) args['path'] = t.path + if (t.filePath !== undefined) args['file_path'] = t.filePath + return { + type: 'toolCall', + id: `call-${t.name}`, + name: t.name, + arguments: args, + } + }) return JSON.stringify({ type: 'message', @@ -263,6 +302,115 @@ describe('pi provider - JSONL parsing', () => { expect(calls[0]!.bashCommands).toEqual(['git', 'bun']) }) + it('classifies a SKILL.md read as a skill load, not a Read (#588)', async () => { + const projectDir = join(tmpDir, '--Users-test-myproject--') + const filePath = await writeSession(projectDir, 'session.jsonl', [ + sessionMeta(), + assistantMessage({ + tools: [ + { name: 'read', path: '/Volumes/T7/repos/cuneiform/.pi/skills/bmad-create-story/SKILL.md' }, + { name: 'read', path: '/Volumes/T7/repos/cuneiform/workflow.md' }, + { name: 'edit', path: '/Volumes/T7/repos/cuneiform/workflow.md' }, + ], + }), + ]) + + const provider = createPiProvider(tmpDir) + const source = { path: filePath, project: 'myproject', provider: 'pi' } + const calls: ParsedProviderCall[] = [] + for await (const call of provider.createSessionParser(source, new Set()).parse()) { + calls.push(call) + } + + // The SKILL.md read is surfaced as the Skill tool (not Read); the plain + // read stays a Read. + expect(calls[0]!.skills).toEqual(['bmad-create-story']) + expect(calls[0]!.tools).toEqual(['Skill', 'Read', 'Edit']) + }) + + it('classifies a skill:// read as a skill load (OMP-style URI)', async () => { + const projectDir = join(tmpDir, '--Users-test-myproject--') + const filePath = await writeSession(projectDir, 'session.jsonl', [ + sessionMeta(), + assistantMessage({ + tools: [{ name: 'read', path: 'skill://commit-workflow' }], + }), + ]) + + const provider = createPiProvider(tmpDir) + const source = { path: filePath, project: 'myproject', provider: 'pi' } + const calls: ParsedProviderCall[] = [] + for await (const call of provider.createSessionParser(source, new Set()).parse()) { + calls.push(call) + } + + expect(calls[0]!.skills).toEqual(['commit-workflow']) + expect(calls[0]!.tools).toEqual(['Skill']) + }) + + it('reads the file_path key as a fallback for skill loads', async () => { + const projectDir = join(tmpDir, '--Users-test-myproject--') + const filePath = await writeSession(projectDir, 'session.jsonl', [ + sessionMeta(), + assistantMessage({ + tools: [{ name: 'read', filePath: '/home/u/.agents/skills/deep-research/SKILL.md' }], + }), + ]) + + const provider = createPiProvider(tmpDir) + const source = { path: filePath, project: 'myproject', provider: 'pi' } + const calls: ParsedProviderCall[] = [] + for await (const call of provider.createSessionParser(source, new Set()).parse()) { + calls.push(call) + } + + expect(calls[0]!.skills).toEqual(['deep-research']) + expect(calls[0]!.tools).toEqual(['Skill']) + }) + + it('leaves a normal read (no SKILL.md) as a Read with no skills', async () => { + const projectDir = join(tmpDir, '--Users-test-myproject--') + const filePath = await writeSession(projectDir, 'session.jsonl', [ + sessionMeta(), + assistantMessage({ + tools: [{ name: 'read', path: '/home/u/project/src/skill-loader.ts' }], + }), + ]) + + const provider = createPiProvider(tmpDir) + const source = { path: filePath, project: 'myproject', provider: 'pi' } + const calls: ParsedProviderCall[] = [] + for await (const call of provider.createSessionParser(source, new Set()).parse()) { + calls.push(call) + } + + expect(calls[0]!.skills).toEqual([]) + expect(calls[0]!.tools).toEqual(['Read']) + }) + + it('a pure skill-load turn classifies as general with the skill as subCategory (feeds the Skills breakdown, #588)', async () => { + const projectDir = join(tmpDir, '--Users-test-myproject--') + const filePath = await writeSession(projectDir, 'session.jsonl', [ + sessionMeta(), + assistantMessage({ + tools: [{ name: 'read', path: '/home/u/.pi/agent/skills/systematic-debugging/SKILL.md' }], + }), + ]) + + const provider = createPiProvider(tmpDir) + const source = { path: filePath, project: 'myproject', provider: 'pi' } + const calls: ParsedProviderCall[] = [] + for await (const call of provider.createSessionParser(source, new Set()).parse()) { + calls.push(call) + } + + // End to end: the parsed skill load must reach `subCategory`, or the + // Skills & Agents breakdown stays empty (the second half of #588). + const classified = classifyTurn(turnFromPiCall(calls[0]!)) + expect(classified.category).toBe('general') + expect(classified.subCategory).toBe('systematic-debugging') + }) + it('skips assistant messages with zero tokens', async () => { const projectDir = join(tmpDir, '--Users-test-myproject--') const filePath = await writeSession(projectDir, 'session.jsonl', [