fix(analytics): countNewHtmlArtifacts skips failed tool ops

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

@@ -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<string, { isError: boolean }>();
+  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<string>();
   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;

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);
   });