From 2cd05f09e4550688d32bc956132e6707cd6c35c7 Mon Sep 17 00:00:00 2001 From: lefarcen <935902669@qq.com> Date: Thu, 21 May 2026 22:37:02 +0800 Subject: [PATCH] fix(analytics): countNewHtmlArtifacts skips failed tool ops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fix on PR #2590 from mrcfps (2026-05-21 14:30, on commit 9e9a0019). `countNewHtmlArtifacts` counted every `Write` / `Edit` tool_use on a `.html` path regardless of whether the matching `tool_result` came back with `isError: true`. A permission denied `Write index.html`, a path-outside-cwd refusal, or a parent-missing failure all still bumped `run_finished.artifact_count` to 1 — which is exactly the corruption pattern this helper was introduced to fix (hard-coded zero → spuriously > 0 is the same class of broken funnel signal). Fix: mirror the web-side `apps/web/src/runtime/file-ops.ts` pattern. Build a `resultByToolUseId` map in a first pass, then in the second pass only count a tool_use whose paired result exists AND `isError !== true`. A tool_use with no matching result is treated as "still in flight" and not counted; the dashboard would rather under-count attempts than promise artifacts we can't confirm landed. Tests grow 3 → 13: - successful Write pair counts (canonical path) - isError=true result does NOT count - unpaired tool_use does NOT count - Write-success-then-Edit-fail on same path still counts (artifact is on disk; later edit failure doesn't unmake it) - existing dedup / distinct-paths / alias / case / malformed / read-skip cases all updated to use the new pair() helper --- apps/daemon/src/run-artifacts.ts | 67 ++++++++++- apps/daemon/tests/run-artifacts.test.ts | 142 ++++++++++++++++++------ 2 files changed, 175 insertions(+), 34 deletions(-) diff --git a/apps/daemon/src/run-artifacts.ts b/apps/daemon/src/run-artifacts.ts index d9d39c846d..76ca35e181 100644 --- a/apps/daemon/src/run-artifacts.ts +++ b/apps/daemon/src/run-artifacts.ts @@ -11,10 +11,18 @@ // agent often writes a skeleton then edits to fill it in; both end // in a new file state at run end. // - Read-only ops never count. +// - Failed ops never count. A `tool_use` whose matching `tool_result` +// reports `isError: true` (permission denied, path outside cwd, +// parent missing, etc.) does NOT produce an artifact even though +// the tool name and path were correct. Earlier the helper skipped +// this join, so a "Write index.html" that errored still bumped +// `artifact_count` to 1 and corrupted the +// "generation_success → artifact_produced" funnel (mrcfps review +// on PR #2590). // -// Earlier `server.ts:11061` hard-coded `artifact_count: 0`, which -// produced uniform zero on PostHog and made the v2 "generation -// success → produced artifact" funnel useless. +// Earlier `server.ts` hard-coded `artifact_count: 0`, which produced +// uniform zero on PostHog and made the same funnel useless from the +// other direction. // Tool names cover Claude-style, Codex-style, and the ACP/MCP shapes // the daemon proxies. Keep aligned with the web-side `WRITE_NAMES` / @@ -45,8 +53,51 @@ export interface RunEventLike { data?: unknown; } +// Join key the daemon-stream uses for tool_use → tool_result pairing. +// Claude / Codex / ACP all stamp the same `id` onto the `tool_use` +// event and reference it via `toolUseId` on the subsequent +// `tool_result`; see `apps/daemon/src/langfuse-bridge.ts#collectToolCalls` +// for the canonical implementation. +function readToolUseId(data: unknown): string | null { + if (!data || typeof data !== 'object') return null; + const obj = data as { id?: unknown }; + return typeof obj.id === 'string' && obj.id ? obj.id : null; +} + +function readToolResultId(data: unknown): string | null { + if (!data || typeof data !== 'object') return null; + const obj = data as { toolUseId?: unknown }; + return typeof obj.toolUseId === 'string' && obj.toolUseId + ? obj.toolUseId + : null; +} + +function readToolResultIsError(data: unknown): boolean { + if (!data || typeof data !== 'object') return false; + return (data as { isError?: unknown }).isError === true; +} + export function countNewHtmlArtifacts(events: readonly RunEventLike[]): number { if (!events || events.length === 0) return 0; + + // First pass: collect tool_result records keyed by toolUseId so the + // second pass can resolve each tool_use's outcome without quadratic + // scanning. Default outcome for a tool_use with no matching result + // (run still streaming when this is called, or adapter swallowed + // the result) is "still in flight" — we treat that as NOT counting, + // since we can't confirm the artifact actually landed. The web-side + // `deriveFileOps` makes the same choice (status='running' for + // unmatched tool_use); aligning here keeps both pipelines in sync. + const resultByToolUseId = new Map(); + for (const rec of events) { + if (rec?.event !== 'agent') continue; + const data = rec.data as { type?: string } | null | undefined; + if (data?.type !== 'tool_result') continue; + const id = readToolResultId(rec.data); + if (!id) continue; + resultByToolUseId.set(id, { isError: readToolResultIsError(rec.data) }); + } + const writtenPaths = new Set(); for (const rec of events) { if (rec?.event !== 'agent') continue; @@ -60,6 +111,16 @@ export function countNewHtmlArtifacts(events: readonly RunEventLike[]): number { const path = extractToolFilePath(data.input); if (!path) continue; if (!isHtmlPath(path)) continue; + const toolUseId = readToolUseId(rec.data); + // No id: legacy / synthetic stream shapes that don't pair. Be + // conservative and skip; the dashboard would rather under-count + // than count attempts that may have failed silently. + if (!toolUseId) continue; + const outcome = resultByToolUseId.get(toolUseId); + // No matching result: tool still in flight at snapshot time, OR + // adapter never emitted one. Don't count either way. + if (!outcome) continue; + if (outcome.isError) continue; writtenPaths.add(path); } return writtenPaths.size; diff --git a/apps/daemon/tests/run-artifacts.test.ts b/apps/daemon/tests/run-artifacts.test.ts index f651420c2d..80ccb728ee 100644 --- a/apps/daemon/tests/run-artifacts.test.ts +++ b/apps/daemon/tests/run-artifacts.test.ts @@ -1,7 +1,12 @@ // Unit coverage for `countNewHtmlArtifacts`. Pins the v2 // `run_finished.artifact_count` invariant: incremental count of // distinct `.html` paths the run produced or modified, deduped by -// path, with Read ops never counted. +// path, with Read ops never counted and FAILED ops never counted +// (mrcfps review on PR #2590 — earlier version counted every +// matching `tool_use` regardless of whether the matching +// `tool_result` landed `isError: true`, so a permission-denied +// `Write index.html` still bumped `artifact_count` to 1 and +// corrupted the same funnel this helper is trying to repair). // // `server.ts` previously emitted `artifact_count: 0` literally, which // suppressed every dashboard tile that breaks "generation success" by @@ -13,16 +18,56 @@ import { describe, expect, it } from 'vitest'; import { countNewHtmlArtifacts } from '../src/run-artifacts.js'; -function toolUse(name: string, filePath: string, id = 'tool-1') { - return { - event: 'agent', - data: { - type: 'tool_use', - id, - name, - input: { file_path: filePath }, +let nextId = 0; +function freshId(prefix = 'tool'): string { + nextId += 1; + return `${prefix}-${nextId}`; +} + +// Helper: emit a tool_use+tool_result pair. `isError` defaults to +// false so the common "successful Write on .html" case stays one +// line at the call site. +function pair( + name: string, + filePath: string, + isError = false, + id = freshId(), +) { + return [ + { + event: 'agent', + data: { + type: 'tool_use', + id, + name, + input: { file_path: filePath }, + }, }, - }; + { + event: 'agent', + data: { + type: 'tool_result', + toolUseId: id, + isError, + }, + }, + ]; +} + +// Helper: emit a tool_use with no matching tool_result. Used to pin +// the "tool still in flight" / "adapter swallowed result" behavior. +function unfinished(name: string, filePath: string, id = freshId()) { + return [ + { + event: 'agent', + data: { + type: 'tool_use', + id, + name, + input: { file_path: filePath }, + }, + }, + ]; } describe('countNewHtmlArtifacts', () => { @@ -33,25 +78,55 @@ describe('countNewHtmlArtifacts', () => { it('returns 0 when no tool_use targets a .html file', () => { expect( countNewHtmlArtifacts([ - toolUse('Write', '/proj/notes.md'), - toolUse('Edit', '/proj/styles.css'), - toolUse('Read', '/proj/index.html'), // Read doesn't count + ...pair('Write', '/proj/notes.md'), + ...pair('Edit', '/proj/styles.css'), + ...pair('Read', '/proj/index.html'), ]), ).toBe(0); }); - it('counts a single Write on a .html path', () => { + it('counts a single successful Write on a .html path', () => { expect( - countNewHtmlArtifacts([toolUse('Write', '/proj/index.html')]), + countNewHtmlArtifacts(pair('Write', '/proj/index.html')), ).toBe(1); }); - it('dedupes multiple Write/Edit ops on the same path (one artifact per file)', () => { + it('does NOT count a tool_use whose tool_result reports isError=true', () => { + // Permission denied, path outside cwd, parent missing — all bounce + // the same way through the tool_result.isError channel and must + // not increment artifact_count. + expect( + countNewHtmlArtifacts(pair('Write', '/proj/index.html', true)), + ).toBe(0); + }); + + it('does NOT count a tool_use that has no matching tool_result yet', () => { + // Run was sampled while a Write was still in flight; the + // safe-default is to undercount rather than promise an + // artifact we can't confirm landed. + expect( + countNewHtmlArtifacts(unfinished('Write', '/proj/index.html')), + ).toBe(0); + }); + + it('dedupes multiple successful Write/Edit ops on the same path', () => { expect( countNewHtmlArtifacts([ - toolUse('Write', '/proj/index.html', 't1'), - toolUse('Edit', '/proj/index.html', 't2'), - toolUse('MultiEdit', '/proj/index.html', 't3'), + ...pair('Write', '/proj/index.html'), + ...pair('Edit', '/proj/index.html'), + ...pair('MultiEdit', '/proj/index.html'), + ]), + ).toBe(1); + }); + + it('keeps the path counted when a later edit on the same path fails', () => { + // First Write succeeded — the artifact exists. A subsequent Edit + // that errors doesn't take that fact away; the file is still on + // disk. So this still reports 1. + expect( + countNewHtmlArtifacts([ + ...pair('Write', '/proj/index.html'), + ...pair('Edit', '/proj/index.html', true), ]), ).toBe(1); }); @@ -59,9 +134,9 @@ describe('countNewHtmlArtifacts', () => { it('counts distinct .html paths separately', () => { expect( countNewHtmlArtifacts([ - toolUse('Write', '/proj/index.html'), - toolUse('Write', '/proj/about.html'), - toolUse('Write', '/proj/contact.html'), + ...pair('Write', '/proj/index.html'), + ...pair('Write', '/proj/about.html'), + ...pair('Write', '/proj/contact.html'), ]), ).toBe(3); }); @@ -69,24 +144,29 @@ describe('countNewHtmlArtifacts', () => { it('handles the Codex `create_file` / `str_replace_edit` aliases', () => { expect( countNewHtmlArtifacts([ - toolUse('create_file', '/proj/a.html'), - toolUse('str_replace_edit', '/proj/b.html'), + ...pair('create_file', '/proj/a.html'), + ...pair('str_replace_edit', '/proj/b.html'), ]), ).toBe(2); }); it('accepts both `file_path` and `path` input shapes', () => { + const id = freshId(); expect( countNewHtmlArtifacts([ { event: 'agent', data: { type: 'tool_use', - id: 't1', + id, name: 'Write', input: { path: '/proj/page.html' }, }, }, + { + event: 'agent', + data: { type: 'tool_result', toolUseId: id, isError: false }, + }, ]), ).toBe(1); }); @@ -94,8 +174,8 @@ describe('countNewHtmlArtifacts', () => { it('treats .HTML / .Html case-insensitively', () => { expect( countNewHtmlArtifacts([ - toolUse('Write', '/proj/Page.HTML'), - toolUse('Write', '/proj/Other.Html'), + ...pair('Write', '/proj/Page.HTML'), + ...pair('Write', '/proj/Other.Html'), ]), ).toBe(2); }); @@ -107,7 +187,7 @@ describe('countNewHtmlArtifacts', () => { { event: 'stderr', data: { chunk: 'log' } }, { event: 'agent', data: null }, { event: 'agent', data: { type: 'text_delta', text: 'hi' } }, - toolUse('Write', '/proj/index.html'), + ...pair('Write', '/proj/index.html'), ]), ).toBe(1); }); @@ -115,9 +195,9 @@ describe('countNewHtmlArtifacts', () => { it('ignores Read / Grep / Bash even when their input names a .html file', () => { expect( countNewHtmlArtifacts([ - toolUse('Read', '/proj/index.html'), - toolUse('Grep', '/proj/index.html'), - toolUse('Bash', '/proj/index.html'), + ...pair('Read', '/proj/index.html'), + ...pair('Grep', '/proj/index.html'), + ...pair('Bash', '/proj/index.html'), ]), ).toBe(0); });