From 3d6f9627ecf93a30acbdcdda9e6b52a764b6017a Mon Sep 17 00:00:00 2001 From: Cuong Date: Wed, 27 May 2026 15:26:49 +0700 Subject: [PATCH 1/7] feat: [ENG-2969] M14.1 extend TASK_TYPE_VALUES with v4.0 tool-mode task types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds curate-tool-mode, query-tool-mode, dream-scan, dream-finalize to TaskTypes + TASK_TYPE_VALUES. After ENG-2925's rename the daemon dispatches these four types but the analytics enum still predates v4.0 — task_created / task_completed / task_failed silently rejected every tool-mode emit at the Zod boundary. - TaskTypes keeps the legacy 'curate' / 'query' / 'dream' values for back-compat with any constructor still building those payloads - per-event schemas (task_created / task_completed / task_failed) pick up the new types automatically via z.enum(TASK_TYPE_VALUES) - M12 per-flavor schemas (curate_run_completed / query_completed) still hardcode their own literals and continue to reject tool-mode types here — M14.2 migrates them to the canonical enum as a follow-up TDD: - new task-types tests assert TaskCreated/Completed/Failed accept all four new types - regression tests pin the M12 schemas' continued rejection so M14.2 has a clear flip point --- src/shared/analytics/task-types.ts | 8 ++ test/unit/shared/analytics/task-types.test.ts | 112 +++++++++++++++++- 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/src/shared/analytics/task-types.ts b/src/shared/analytics/task-types.ts index d14e60186..f8976f5ac 100644 --- a/src/shared/analytics/task-types.ts +++ b/src/shared/analytics/task-types.ts @@ -12,8 +12,12 @@ export const TaskTypes = { CURATE: 'curate', CURATE_FOLDER: 'curate-folder', + CURATE_TOOL_MODE: 'curate-tool-mode', DREAM: 'dream', + DREAM_FINALIZE: 'dream-finalize', + DREAM_SCAN: 'dream-scan', QUERY: 'query', + QUERY_TOOL_MODE: 'query-tool-mode', SEARCH: 'search', } as const @@ -27,7 +31,11 @@ export type TaskType = (typeof TaskTypes)[keyof typeof TaskTypes] export const TASK_TYPE_VALUES = [ TaskTypes.CURATE, TaskTypes.CURATE_FOLDER, + TaskTypes.CURATE_TOOL_MODE, TaskTypes.DREAM, + TaskTypes.DREAM_FINALIZE, + TaskTypes.DREAM_SCAN, TaskTypes.QUERY, + TaskTypes.QUERY_TOOL_MODE, TaskTypes.SEARCH, ] as const diff --git a/test/unit/shared/analytics/task-types.test.ts b/test/unit/shared/analytics/task-types.test.ts index 4126f1329..104a355cf 100644 --- a/test/unit/shared/analytics/task-types.test.ts +++ b/test/unit/shared/analytics/task-types.test.ts @@ -1,15 +1,25 @@ - + +/* eslint-disable camelcase */ import {expect} from 'chai' +import {CurateRunCompletedSchema} from '../../../../src/shared/analytics/events/curate-run-completed.js' +import {QueryCompletedSchema} from '../../../../src/shared/analytics/events/query-completed.js' +import {TaskCompletedSchema} from '../../../../src/shared/analytics/events/task-completed.js' +import {TaskCreatedSchema} from '../../../../src/shared/analytics/events/task-created.js' +import {TaskFailedSchema} from '../../../../src/shared/analytics/events/task-failed.js' import {TASK_TYPE_VALUES, type TaskType, TaskTypes} from '../../../../src/shared/analytics/task-types.js' describe('TaskTypes', () => { - it('should expose exactly the five daemon task types', () => { + it('should expose every v4.0 daemon task type', () => { expect(Object.keys(TaskTypes).sort()).to.deep.equal([ 'CURATE', 'CURATE_FOLDER', + 'CURATE_TOOL_MODE', 'DREAM', + 'DREAM_FINALIZE', + 'DREAM_SCAN', 'QUERY', + 'QUERY_TOOL_MODE', 'SEARCH', ]) }) @@ -17,9 +27,13 @@ describe('TaskTypes', () => { it('should map each key to the wire string used by the daemon TaskInfo.type', () => { expect(TaskTypes.CURATE).to.equal('curate') expect(TaskTypes.CURATE_FOLDER).to.equal('curate-folder') + expect(TaskTypes.CURATE_TOOL_MODE).to.equal('curate-tool-mode') + expect(TaskTypes.DREAM).to.equal('dream') + expect(TaskTypes.DREAM_FINALIZE).to.equal('dream-finalize') + expect(TaskTypes.DREAM_SCAN).to.equal('dream-scan') expect(TaskTypes.QUERY).to.equal('query') + expect(TaskTypes.QUERY_TOOL_MODE).to.equal('query-tool-mode') expect(TaskTypes.SEARCH).to.equal('search') - expect(TaskTypes.DREAM).to.equal('dream') }) it('should expose TaskType as the union of values', () => { @@ -39,4 +53,96 @@ describe('TaskTypes', () => { expect(TASK_TYPE_VALUES.length).to.be.greaterThan(0) }) }) + + describe('v4.0 tool-mode types validate through task_* schemas', () => { + const newTypes = [ + TaskTypes.CURATE_TOOL_MODE, + TaskTypes.QUERY_TOOL_MODE, + TaskTypes.DREAM_SCAN, + TaskTypes.DREAM_FINALIZE, + ] as const + + for (const taskType of newTypes) { + it(`TaskCreatedSchema accepts task_type='${taskType}'`, () => { + const parsed = TaskCreatedSchema.parse({ + has_files: false, + has_folder: false, + task_id: 't-1', + task_type: taskType, + }) + expect(parsed.task_type).to.equal(taskType) + }) + + it(`TaskCompletedSchema accepts task_type='${taskType}'`, () => { + const parsed = TaskCompletedSchema.parse({ + duration_ms: 100, + task_id: 't-1', + task_type: taskType, + }) + expect(parsed.task_type).to.equal(taskType) + }) + + it(`TaskFailedSchema accepts task_type='${taskType}'`, () => { + const parsed = TaskFailedSchema.parse({ + duration_ms: 100, + task_id: 't-1', + task_type: taskType, + }) + expect(parsed.task_type).to.equal(taskType) + }) + } + + it('rejects an unknown task_type on all three task_* schemas', () => { + const bad = { + duration_ms: 0, + has_files: false, + has_folder: false, + task_id: 't-1', + task_type: 'not-a-real-type' as unknown as TaskType, + } + expect(() => TaskCreatedSchema.parse(bad)).to.throw() + expect(() => TaskCompletedSchema.parse(bad)).to.throw() + expect(() => TaskFailedSchema.parse(bad)).to.throw() + }) + }) + + // The M12 schemas hardcode literal task_type values and still REJECT + // the new tool-mode types. M14.2 is the follow-up that migrates them + // to z.enum(TASK_TYPE_VALUES); until then these regressions are + // EXPECTED and documented as such in M14.1's TDD. + describe('M12 schemas still reject tool-mode types (M14.2 follow-up)', () => { + const curatePayload = { + duration_ms: 100, + operations_added: 0, + operations_deleted: 0, + operations_failed: 0, + operations_merged: 0, + operations_updated: 0, + outcome: 'completed' as const, + pending_review_count: 0, + task_id: 't-1', + } + + it('CurateRunCompletedSchema still rejects curate-tool-mode', () => { + expect(() => + CurateRunCompletedSchema.parse({...curatePayload, task_type: TaskTypes.CURATE_TOOL_MODE}), + ).to.throw() + }) + + it('QueryCompletedSchema still rejects query-tool-mode', () => { + const queryPayload = { + cache_hit: false, + duration_ms: 100, + matched_doc_count: 0, + outcome: 'completed' as const, + read_doc_count: 0, + read_tool_call_count: 0, + search_call_count: 0, + task_id: 't-1', + } + expect(() => + QueryCompletedSchema.parse({...queryPayload, task_type: TaskTypes.QUERY_TOOL_MODE}), + ).to.throw() + }) + }) }) From 6d7459659912145efd97b186ea91a8df659afec0 Mon Sep 17 00:00:00 2001 From: Cuong Date: Wed, 27 May 2026 15:30:04 +0700 Subject: [PATCH 2/7] fix: [ENG-2970] M14.2 relax curate_run_completed + query_completed task_type to TASK_TYPE_VALUES MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrates the two M12 per-flavor schemas from hardcoded literal task_type values to the canonical TASK_TYPE_VALUES enum so v4.0 tool-mode tasks round-trip the wire boundary instead of being silently Zod-rejected inside AnalyticsHook. - curate_run_completed: z.enum(['curate', 'curate-folder']) → z.enum(TASK_TYPE_VALUES). curate-tool-mode payloads now validate. - query_completed: z.literal('query') → z.enum(TASK_TYPE_VALUES). query-tool-mode payloads now validate. The schemas no longer structurally constrain task_type to the curate or query family; the hook is trusted to only emit each event for the right flavor. Docblocks call out the widening for the next maintainer. TDD: - curate-run-completed.test asserts curate-tool-mode + legacy values both succeed; an unknown task_type still rejects - query-completed.test mirrors the same coverage for query-tool-mode - task-types.test M14.1 regression assertions flipped from rejection to acceptance for the M12 schemas; M14.1 docblock comments updated Without this, M14.3's hook code would land but the M12 emits that fire alongside the new generic task_* emits would silently disappear on every tool-mode task — that's the bug operators noticed in Mixpanel. --- .../analytics/events/curate-run-completed.ts | 10 +++- .../analytics/events/query-completed.ts | 10 +++- .../events/curate-run-completed.test.ts | 9 ++- .../analytics/events/query-completed.test.ts | 9 ++- test/unit/shared/analytics/task-types.test.ts | 56 +++++++++++-------- 5 files changed, 65 insertions(+), 29 deletions(-) diff --git a/src/shared/analytics/events/curate-run-completed.ts b/src/shared/analytics/events/curate-run-completed.ts index 8e1702f6c..57f223eb9 100644 --- a/src/shared/analytics/events/curate-run-completed.ts +++ b/src/shared/analytics/events/curate-run-completed.ts @@ -1,12 +1,20 @@ /* eslint-disable camelcase */ import {z} from 'zod' +import {TASK_TYPE_VALUES} from '../task-types.js' + /** * Per-event schema for `curate_run_completed`. * * Emitted by the daemon's `AnalyticsHook` (M12.2) at curate task terminal * states (completed / partial / cancelled / error). Carries per-task * operation counters so PMs can aggregate curate volume + outcome over time. + * + * M14.2 migrated `task_type` from a literal ['curate', 'curate-folder'] + * enum to the canonical `TASK_TYPE_VALUES` tuple so v4.0 tool-mode types + * (curate-tool-mode) round-trip the wire boundary. The hook is expected + * to only emit this event for curate flavors; the schema no longer + * structurally enforces that and trusts the caller. */ export const CurateRunCompletedSchema = z .object({ @@ -19,7 +27,7 @@ export const CurateRunCompletedSchema = z outcome: z.enum(['completed', 'partial', 'cancelled', 'error']), pending_review_count: z.number().int().nonnegative(), task_id: z.string().min(1), - task_type: z.enum(['curate', 'curate-folder']), + task_type: z.enum(TASK_TYPE_VALUES), }) .strict() diff --git a/src/shared/analytics/events/query-completed.ts b/src/shared/analytics/events/query-completed.ts index b861608f4..f6058897a 100644 --- a/src/shared/analytics/events/query-completed.ts +++ b/src/shared/analytics/events/query-completed.ts @@ -1,6 +1,8 @@ /* eslint-disable camelcase */ import {z} from 'zod' +import {TASK_TYPE_VALUES} from '../task-types.js' + /** * Per-file structure inside `query_completed.read_paths_with_metadata`. * Frontmatter arrays are optional and absent when the daemon cannot read @@ -23,6 +25,12 @@ const ReadPathWithMetadataSchema = z * states (completed / cancelled / error). Carries duration, retrieval * tier hit, doc counts, and (M12.3) the per-file structure for the top-N * (max 10) files the agent read during the query. + * + * M14.2 migrated `task_type` from `z.literal('query')` to the canonical + * `TASK_TYPE_VALUES` tuple so v4.0 tool-mode types (query-tool-mode) + * round-trip the wire boundary. The hook is expected to only emit this + * event for query flavors; the schema no longer structurally enforces + * that and trusts the caller. */ export const QueryCompletedSchema = z .object({ @@ -35,7 +43,7 @@ export const QueryCompletedSchema = z read_tool_call_count: z.number().int().nonnegative(), search_call_count: z.number().int().nonnegative(), task_id: z.string().min(1), - task_type: z.literal('query'), + task_type: z.enum(TASK_TYPE_VALUES), tier: z.union([z.literal(0), z.literal(1), z.literal(2), z.literal(3), z.literal(4)]).optional(), }) .strict() diff --git a/test/unit/shared/analytics/events/curate-run-completed.test.ts b/test/unit/shared/analytics/events/curate-run-completed.test.ts index d9e9e6728..5ec7cec2d 100644 --- a/test/unit/shared/analytics/events/curate-run-completed.test.ts +++ b/test/unit/shared/analytics/events/curate-run-completed.test.ts @@ -63,8 +63,13 @@ describe('CurateRunCompletedSchema', () => { expect(CurateRunCompletedSchema.safeParse({...baseValid, outcome: 'mystery'}).success).to.equal(false) }) - it('rejects out-of-enum task_type', () => { - expect(CurateRunCompletedSchema.safeParse({...baseValid, task_type: 'query'}).success).to.equal(false) + it('rejects an unknown task_type but accepts every canonical TASK_TYPE_VALUES entry', () => { + // M14.2 widened task_type from ['curate', 'curate-folder'] to the + // canonical TASK_TYPE_VALUES tuple so curate-tool-mode round-trips + // the wire boundary. Genuinely unknown values still reject. + expect(CurateRunCompletedSchema.safeParse({...baseValid, task_type: 'not-a-real-type'}).success).to.equal(false) + expect(CurateRunCompletedSchema.safeParse({...baseValid, task_type: 'curate-tool-mode'}).success).to.equal(true) + expect(CurateRunCompletedSchema.safeParse({...baseValid, task_type: 'query'}).success).to.equal(true) }) it('rejects negative counts and duration_ms', () => { diff --git a/test/unit/shared/analytics/events/query-completed.test.ts b/test/unit/shared/analytics/events/query-completed.test.ts index 69072357a..e80d8039f 100644 --- a/test/unit/shared/analytics/events/query-completed.test.ts +++ b/test/unit/shared/analytics/events/query-completed.test.ts @@ -90,8 +90,13 @@ describe('QueryCompletedSchema', () => { expect(QueryCompletedSchema.safeParse({...baseValid, tier: -1}).success).to.equal(false) }) - it('rejects task_type other than literal "query"', () => { - expect(QueryCompletedSchema.safeParse({...baseValid, task_type: 'curate'}).success).to.equal(false) + it('rejects an unknown task_type but accepts every canonical TASK_TYPE_VALUES entry', () => { + // M14.2 widened task_type from z.literal('query') to the canonical + // TASK_TYPE_VALUES tuple so query-tool-mode round-trips the wire + // boundary. Genuinely unknown values still reject. + expect(QueryCompletedSchema.safeParse({...baseValid, task_type: 'not-a-real-type'}).success).to.equal(false) + expect(QueryCompletedSchema.safeParse({...baseValid, task_type: 'query-tool-mode'}).success).to.equal(true) + expect(QueryCompletedSchema.safeParse({...baseValid, task_type: 'curate'}).success).to.equal(true) }) it('rejects negative or non-integer counts', () => { diff --git a/test/unit/shared/analytics/task-types.test.ts b/test/unit/shared/analytics/task-types.test.ts index 104a355cf..e0d5999ef 100644 --- a/test/unit/shared/analytics/task-types.test.ts +++ b/test/unit/shared/analytics/task-types.test.ts @@ -106,11 +106,7 @@ describe('TaskTypes', () => { }) }) - // The M12 schemas hardcode literal task_type values and still REJECT - // the new tool-mode types. M14.2 is the follow-up that migrates them - // to z.enum(TASK_TYPE_VALUES); until then these regressions are - // EXPECTED and documented as such in M14.1's TDD. - describe('M12 schemas still reject tool-mode types (M14.2 follow-up)', () => { + describe('M12 per-flavor schemas accept tool-mode types (M14.2)', () => { const curatePayload = { duration_ms: 100, operations_added: 0, @@ -123,26 +119,40 @@ describe('TaskTypes', () => { task_id: 't-1', } - it('CurateRunCompletedSchema still rejects curate-tool-mode', () => { - expect(() => - CurateRunCompletedSchema.parse({...curatePayload, task_type: TaskTypes.CURATE_TOOL_MODE}), - ).to.throw() + const queryPayload = { + cache_hit: false, + duration_ms: 100, + matched_doc_count: 0, + outcome: 'completed' as const, + read_doc_count: 0, + read_tool_call_count: 0, + search_call_count: 0, + task_id: 't-1', + } + + it('CurateRunCompletedSchema accepts curate-tool-mode', () => { + const parsed = CurateRunCompletedSchema.parse({ + ...curatePayload, + task_type: TaskTypes.CURATE_TOOL_MODE, + }) + expect(parsed.task_type).to.equal('curate-tool-mode') }) - it('QueryCompletedSchema still rejects query-tool-mode', () => { - const queryPayload = { - cache_hit: false, - duration_ms: 100, - matched_doc_count: 0, - outcome: 'completed' as const, - read_doc_count: 0, - read_tool_call_count: 0, - search_call_count: 0, - task_id: 't-1', - } - expect(() => - QueryCompletedSchema.parse({...queryPayload, task_type: TaskTypes.QUERY_TOOL_MODE}), - ).to.throw() + it('CurateRunCompletedSchema still accepts the legacy curate / curate-folder values', () => { + const curate = CurateRunCompletedSchema.parse({...curatePayload, task_type: TaskTypes.CURATE}) + expect(curate.task_type).to.equal('curate') + const folder = CurateRunCompletedSchema.parse({...curatePayload, task_type: TaskTypes.CURATE_FOLDER}) + expect(folder.task_type).to.equal('curate-folder') + }) + + it('QueryCompletedSchema accepts query-tool-mode', () => { + const parsed = QueryCompletedSchema.parse({...queryPayload, task_type: TaskTypes.QUERY_TOOL_MODE}) + expect(parsed.task_type).to.equal('query-tool-mode') + }) + + it('QueryCompletedSchema still accepts the legacy query value', () => { + const parsed = QueryCompletedSchema.parse({...queryPayload, task_type: TaskTypes.QUERY}) + expect(parsed.task_type).to.equal('query') }) }) }) From 1d4375ff281a70df9b98ea42fa0e87f5e654aee7 Mon Sep 17 00:00:00 2001 From: Cuong Date: Wed, 27 May 2026 15:56:30 +0700 Subject: [PATCH 3/7] feat: [ENG-2971] M14.3 wire task_created + task_completed + task_failed in AnalyticsHook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the three generic funnel-event emits described in M14's customer ask. Every daemon task (curate / curate-folder / curate-tool-mode (aliased from curate-html-direct) / query / query-tool-mode / dream-scan / dream-finalize / search) now produces, in order: task_created onTaskCreate (funnel entry — unconditional) ...optional per-op + M12 per-flavor terminal emit task_completed onTaskCompleted (terminal-event-last on success) task_failed onTaskError / onTaskCancelled (terminal-event-last) Three coupled changes shipped together so the wire stays consistent: 1. analytics-hook.ts grows three emits and a `toAnalyticsTaskType` alias-translator. The daemon still dispatches the pre-ENG-2925 name `curate-html-direct`; analytics canonicalises it to the post-rename `curate-tool-mode` so the wire enum matches TASK_TYPE_VALUES. Once ENG-2925 lands, the alias becomes a no-op identity. 2. CURATE_TASK_TYPES + QUERY_TASK_TYPES gain the tool-mode names so M12 per-flavor state init kicks in for tool-mode curates / queries. M12 counters stay all-zero on tool-mode today (no LLM tool calls fire) — that's a separate follow-up (FU-1 in plans/analytics-m14/follow-ups). 3. Both M12 payload builders route `task_type` through the same alias so curate-tool-mode tasks emit `task_type='curate-tool-mode'` on the curate_run_completed / query_completed events too. TDD: - new analytics-hook-m14.test.ts: 15 simulation tests covering every task type, the curate-html-direct → curate-tool-mode alias, has_files / has_folder semantics, both terminal paths, and the ordering invariant - existing analytics-hook.test.ts updated to filter out the new generic emits via `filterM12()` so M12-focused assertions keep their intent - integration stress tests updated for the new 13-event sequence (task_created → ops → curate_run_completed → task_completed) - repo full suite: 9121 passing, 0 failing --- src/server/infra/process/analytics-hook.ts | 87 +++++-- .../infra/process/curate-log-handler.ts | 7 +- src/server/infra/process/query-log-handler.ts | 5 +- .../analytics-hook-async-stress.test.ts | 24 +- .../infra/process/analytics-hook-m14.test.ts | 215 ++++++++++++++++++ .../infra/process/analytics-hook.test.ts | 119 ++++++---- 6 files changed, 380 insertions(+), 77 deletions(-) create mode 100644 test/unit/server/infra/process/analytics-hook-m14.test.ts diff --git a/src/server/infra/process/analytics-hook.ts b/src/server/infra/process/analytics-hook.ts index 6d886a3c3..307c7bba6 100644 --- a/src/server/infra/process/analytics-hook.ts +++ b/src/server/infra/process/analytics-hook.ts @@ -5,6 +5,7 @@ import type {AnalyticsEventName} from '../../../shared/analytics/event-names.js' import type {CurateRunCompletedProps} from '../../../shared/analytics/events/curate-run-completed.js' import type {PropsArg} from '../../../shared/analytics/events/index.js' import type {QueryCompletedProps} from '../../../shared/analytics/events/query-completed.js' +import type {TaskType} from '../../../shared/analytics/task-types.js' import type {LlmToolResultEvent} from '../../core/domain/transport/schemas.js' import type {TaskInfo} from '../../core/domain/transport/task-info.js' import type {IAnalyticsClient} from '../../core/interfaces/analytics/i-analytics-client.js' @@ -12,12 +13,25 @@ import type {ITaskLifecycleHook} from '../../core/interfaces/process/i-task-life import type {QueryResultMetadata} from './query-log-handler.js' import {AnalyticsEventNames} from '../../../shared/analytics/event-names.js' +import {TaskTypes} from '../../../shared/analytics/task-types.js' import {parseFrontmatter} from '../../core/domain/knowledge/markdown-writer.js' import {extractCurateOperations} from '../../utils/curate-result-parser.js' import {processLog} from '../../utils/process-logger.js' import {CURATE_TASK_TYPES} from './curate-log-handler.js' import {QUERY_TASK_TYPES} from './query-log-handler.js' +/** + * Translate the daemon's runtime task type string to the canonical + * analytics wire value. The daemon still dispatches the pre-ENG-2925 + * name `'curate-html-direct'`; analytics emits the post-rename + * `'curate-tool-mode'`. Once the rename PR lands, this becomes a + * no-op identity and can be inlined. + */ +function toAnalyticsTaskType(daemonType: string): TaskType { + if (daemonType === 'curate-html-direct') return TaskTypes.CURATE_TOOL_MODE + return daemonType as TaskType +} + // `CURATE_TASK_TYPES` is exported as a readonly tuple; wrap in a Set // for cast-free `.has()` lookups against TaskInfo.type (string). const CURATE_TASK_TYPE_SET: ReadonlySet = new Set(CURATE_TASK_TYPES) @@ -142,32 +156,52 @@ export class AnalyticsHook implements ITaskLifecycleHook { async onTaskCancelled(taskId: string, task: TaskInfo): Promise { await this.dispatchTerminal(taskId, task, 'cancelled') + this.emitTaskFailed(taskId, task) } async onTaskCompleted(taskId: string, _result: string, task: TaskInfo): Promise { const state = this.tasks.get(taskId) - if (!state) return + if (state) { + // Drain any in-flight per-op processing so CURATE_OPERATION_APPLIED emits + // land BEFORE the run-completion emit on the wire. The chain never + // rejects (see `onToolResult`), so this await is safe. + await this.pendingByTask.get(taskId) + + if (state.flavor === 'curate') { + const outcome = state.counters.failed > 0 ? 'partial' : 'completed' + this.emit( + AnalyticsEventNames.CURATE_RUN_COMPLETED, + this.buildCurateRunPayload({outcome, state, task, taskId}), + ) + } else { + this.emit( + AnalyticsEventNames.QUERY_COMPLETED, + await this.buildQueryCompletedPayload({outcome: 'completed', state, task, taskId}), + ) + } + } - // Drain any in-flight per-op processing so CURATE_OPERATION_APPLIED emits - // land BEFORE the run-completion emit on the wire. The chain never - // rejects (see `onToolResult`), so this await is safe. - await this.pendingByTask.get(taskId) - if (state.flavor === 'curate') { - const outcome = state.counters.failed > 0 ? 'partial' : 'completed' - this.emit( - AnalyticsEventNames.CURATE_RUN_COMPLETED, - this.buildCurateRunPayload({outcome, state, task, taskId}), - ) - } else { - this.emit( - AnalyticsEventNames.QUERY_COMPLETED, - await this.buildQueryCompletedPayload({outcome: 'completed', state, task, taskId}), - ) - } + // M14.3 generic funnel emit. Fires for EVERY task type AFTER any + // per-flavor M12 emit (terminal-event-last convention). + this.emit(AnalyticsEventNames.TASK_COMPLETED, { + duration_ms: this.durationMs(task), + task_id: taskId, + task_type: toAnalyticsTaskType(task.type), + }) } async onTaskCreate(task: TaskInfo): Promise { + // M14.3 generic funnel-entry emit. Fires for EVERY task type BEFORE + // the M12 per-flavor state init so the entry event lands even if + // state setup throws downstream. + this.emit(AnalyticsEventNames.TASK_CREATED, { + has_files: (task.files?.length ?? 0) > 0, + has_folder: typeof task.folderPath === 'string' && task.folderPath.length > 0, + task_id: task.taskId, + task_type: toAnalyticsTaskType(task.type), + }) + if (isCurateLiteral(task.type)) { this.tasks.set(task.taskId, { counters: {added: 0, deleted: 0, failed: 0, merged: 0, pendingReview: 0, updated: 0}, @@ -184,6 +218,7 @@ export class AnalyticsHook implements ITaskLifecycleHook { async onTaskError(taskId: string, _errorMessage: string, task: TaskInfo): Promise { await this.dispatchTerminal(taskId, task, 'error') + this.emitTaskFailed(taskId, task) } async onToolResult(taskId: string, payload: LlmToolResultEvent): Promise { @@ -246,7 +281,7 @@ export class AnalyticsHook implements ITaskLifecycleHook { outcome, pending_review_count: state.counters.pendingReview, task_id: taskId, - task_type: state.taskType, + task_type: toAnalyticsTaskType(state.taskType), } } @@ -331,7 +366,7 @@ export class AnalyticsHook implements ITaskLifecycleHook { read_tool_call_count: readToolCallCount, search_call_count: searchCallCount, task_id: taskId, - task_type: 'query', + task_type: toAnalyticsTaskType(task.type), ...(tier === undefined ? {} : {tier}), } } @@ -371,6 +406,20 @@ export class AnalyticsHook implements ITaskLifecycleHook { } } + /** + * M14.3 generic terminal-failure emit. Fired by both onTaskError and + * onTaskCancelled AFTER dispatchTerminal so M12 per-flavor failure + * emits land first on the wire. Cancellation maps to task_failed + * (not a distinct event) per the schema's docblock. + */ + private emitTaskFailed(taskId: string, task: TaskInfo): void { + this.emit(AnalyticsEventNames.TASK_FAILED, { + duration_ms: this.durationMs(task), + task_id: taskId, + task_type: toAnalyticsTaskType(task.type), + }) + } + private async processToolResult(taskId: string, payload: LlmToolResultEvent): Promise { const state = this.tasks.get(taskId) if (!state || state.flavor !== 'curate') return diff --git a/src/server/infra/process/curate-log-handler.ts b/src/server/infra/process/curate-log-handler.ts index 8f15e777d..aaf36ba59 100644 --- a/src/server/infra/process/curate-log-handler.ts +++ b/src/server/infra/process/curate-log-handler.ts @@ -50,7 +50,12 @@ function telemetryFields(record: CurateUsageRecord | undefined): { } } -export const CURATE_TASK_TYPES = ['curate', 'curate-folder'] as const +// `curate-html-direct` is the pre-ENG-2925 name still dispatched by the +// daemon; `curate-tool-mode` is the post-rename name. Both are listed +// so M12 state init in AnalyticsHook kicks in for tool-mode curates. +// The analytics wire canonicalizes both to `curate-tool-mode` via +// `toAnalyticsTaskType` in `analytics-hook.ts`. +export const CURATE_TASK_TYPES = ['curate', 'curate-folder', 'curate-html-direct', 'curate-tool-mode'] as const // ── Summary computation ─────────────────────────────────────────────────────── diff --git a/src/server/infra/process/query-log-handler.ts b/src/server/infra/process/query-log-handler.ts index 2fec074dd..16a561774 100644 --- a/src/server/infra/process/query-log-handler.ts +++ b/src/server/infra/process/query-log-handler.ts @@ -44,7 +44,10 @@ type TaskState = { queryResult?: QueryResultMetadata } -export const QUERY_TASK_TYPES: ReadonlySet = new Set(['query']) +// `query-tool-mode` is the v4.0 daemon dispatch name; legacy `query` is +// kept for back-compat. Both names enable M12 state init in AnalyticsHook +// (and matching query-log persistence here). +export const QUERY_TASK_TYPES: ReadonlySet = new Set(['query', 'query-tool-mode']) // ── QueryLogHandler ────────────────────────────────────────────────────────── diff --git a/test/integration/infra/process/analytics-hook-async-stress.test.ts b/test/integration/infra/process/analytics-hook-async-stress.test.ts index 669e9d336..417f1fa62 100644 --- a/test/integration/infra/process/analytics-hook-async-stress.test.ts +++ b/test/integration/infra/process/analytics-hook-async-stress.test.ts @@ -322,16 +322,20 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { const sequence = getEmitSequenceForTask(taskId) - // Exactly 50 per-op emits + 1 terminal emit, terminal LAST. + // M14.3: TASK_CREATED → 50 per-op → CURATE_RUN_COMPLETED → TASK_COMPLETED. expect( sequence.filter((s) => s === AnalyticsEventNames.CURATE_OPERATION_APPLIED), 'exactly 50 per-op emits', ).to.have.lengthOf(50) expect( sequence.filter((s) => s === AnalyticsEventNames.CURATE_RUN_COMPLETED), - 'exactly 1 terminal emit', + 'exactly 1 M12 terminal emit', ).to.have.lengthOf(1) - expect(sequence.at(-1), 'terminal is last in sequence').to.equal(AnalyticsEventNames.CURATE_RUN_COMPLETED) + expect(sequence.at(-1), 'M14.3 task_completed is last in sequence').to.equal(AnalyticsEventNames.TASK_COMPLETED) + expect( + sequence.at(-2), + 'M12 curate_run_completed lands immediately before the M14.3 terminal', + ).to.equal(AnalyticsEventNames.CURATE_RUN_COMPLETED) // And per-op emit order matches arrival order. const opEmits = getCurateOpEmits(taskId) @@ -375,15 +379,19 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { await Promise.all([...opPromises, ...terminalPromises]) - // Every task must end with CURATE_RUN_COMPLETED preceded by 10 per-op emits in arrival order. + // Every task: TASK_CREATED → 10 per-op → CURATE_RUN_COMPLETED → TASK_COMPLETED. for (const id of taskIds) { const sequence = getEmitSequenceForTask(id) - expect(sequence, `${id} sequence length`).to.have.lengthOf(11) + expect(sequence, `${id} sequence length`).to.have.lengthOf(13) + expect(sequence[0], `${id}: first is task_created`).to.equal(AnalyticsEventNames.TASK_CREATED) expect( - sequence.slice(0, 10).every((s) => s === AnalyticsEventNames.CURATE_OPERATION_APPLIED), - `${id}: first 10 are per-op emits`, + sequence.slice(1, 11).every((s) => s === AnalyticsEventNames.CURATE_OPERATION_APPLIED), + `${id}: 10 per-op emits between task_created and the terminals`, ).to.equal(true) - expect(sequence[10], `${id}: last is run-completed`).to.equal(AnalyticsEventNames.CURATE_RUN_COMPLETED) + expect(sequence[11], `${id}: M12 run-completed precedes the M14.3 terminal`).to.equal( + AnalyticsEventNames.CURATE_RUN_COMPLETED, + ) + expect(sequence[12], `${id}: M14.3 task_completed is last`).to.equal(AnalyticsEventNames.TASK_COMPLETED) const opEmits = getCurateOpEmits(id) for (let i = 0; i < 10; i++) { expect(opEmits[i].absolute_path, `${id} op #${i} arrival order`).to.equal(specsByTask[id][i].filePath) diff --git a/test/unit/server/infra/process/analytics-hook-m14.test.ts b/test/unit/server/infra/process/analytics-hook-m14.test.ts new file mode 100644 index 000000000..fdff97a29 --- /dev/null +++ b/test/unit/server/infra/process/analytics-hook-m14.test.ts @@ -0,0 +1,215 @@ +import {expect} from 'chai' +import sinon from 'sinon' + +import type {LlmToolResultEvent} from '../../../../../src/server/core/domain/transport/schemas.js' +import type {TaskInfo} from '../../../../../src/server/core/domain/transport/task-info.js' +import type {IAnalyticsClient} from '../../../../../src/server/core/interfaces/analytics/i-analytics-client.js' + +import {AnalyticsBatch} from '../../../../../src/server/core/domain/analytics/batch.js' +import {AnalyticsHook} from '../../../../../src/server/infra/process/analytics-hook.js' +import {AnalyticsEventNames} from '../../../../../src/shared/analytics/event-names.js' + +const FIXED_NOW = 1_700_000_000_000 + +const buildClient = (): {client: IAnalyticsClient; trackStub: sinon.SinonStub} => { + const trackStub = sinon.stub() + const client: IAnalyticsClient = { + abort() { + /* noop */ + }, + flush: sinon.stub().resolves(AnalyticsBatch.create([])), + getRuntimeState: () => Promise.resolve({droppedCount: 0, lastSuccessfulFlushAt: undefined, queueDepth: 0}), + onAuthTransition: sinon.stub().resolves(), + track: trackStub, + } + return {client, trackStub} +} + +const buildTask = (type: string, overrides: Partial = {}): TaskInfo => + ({ + clientId: 'c1', + completedAt: FIXED_NOW + 1234, + content: 'whatever', + createdAt: FIXED_NOW, + folderPath: undefined, + projectPath: '/project', + taskId: `task-${type}-1`, + toolCalls: [], + type, + ...overrides, + }) as TaskInfo + +const buildCurateOpToolResult = (): LlmToolResultEvent => + ({ + callId: 'call-1', + result: JSON.stringify({ + applied: [ + {filePath: '/a.md', needsReview: false, path: 'a', status: 'success', type: 'ADD'}, + ], + }), + sessionId: 's1', + taskId: 'task-curate-1', + timestamp: FIXED_NOW, + toolName: 'curate' as const, + }) as unknown as LlmToolResultEvent + +const eventSequence = (trackStub: sinon.SinonStub): string[] => + trackStub.getCalls().map((c) => c.args[0] as string) + +describe('AnalyticsHook M14.3 generic task_* emit simulation', () => { + let hook: AnalyticsHook + let trackStub: sinon.SinonStub + + beforeEach(() => { + const bundle = buildClient() + trackStub = bundle.trackStub + hook = new AnalyticsHook() + hook.setAnalyticsClient(bundle.client) + }) + + describe('curate task: full success lifecycle (curate-tool-mode rename simulated)', () => { + it('emits task_created on entry, then per-op + curate_run_completed + task_completed on terminal', async () => { + // Daemon dispatches the pre-ENG-2925 name 'curate-html-direct'; + // analytics is expected to alias-translate to 'curate-tool-mode'. + const task = buildTask('curate-html-direct', {taskId: 'task-curate-1'}) + + await hook.onTaskCreate(task) + await hook.onToolResult(task.taskId, buildCurateOpToolResult()) + await hook.onTaskCompleted(task.taskId, '', task) + + expect(eventSequence(trackStub)).to.deep.equal([ + AnalyticsEventNames.TASK_CREATED, + AnalyticsEventNames.CURATE_OPERATION_APPLIED, + AnalyticsEventNames.CURATE_RUN_COMPLETED, + AnalyticsEventNames.TASK_COMPLETED, + ]) + }) + + it('aliases curate-html-direct → curate-tool-mode on the wire (task_type field)', async () => { + const task = buildTask('curate-html-direct', {taskId: 'task-curate-1'}) + await hook.onTaskCreate(task) + await hook.onTaskCompleted(task.taskId, '', task) + + const created = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_CREATED) + const completed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_COMPLETED) + expect((created?.args[1] as Record).task_type).to.equal('curate-tool-mode') + expect((completed?.args[1] as Record).task_type).to.equal('curate-tool-mode') + }) + + it('emits task_created has_files=true / has_folder=true when set on TaskInfo', async () => { + const task = buildTask('curate-folder', { + files: ['/a.ts', '/b.ts'], + folderPath: '/some/folder', + taskId: 'task-curate-2', + }) + await hook.onTaskCreate(task) + + const created = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_CREATED) + const props = created?.args[1] as Record + expect(props.has_files).to.equal(true) + expect(props.has_folder).to.equal(true) + expect(props.task_type).to.equal('curate-folder') + }) + + it('emits task_created has_files=false / has_folder=false when both are unset', async () => { + const task = buildTask('curate', {files: undefined, folderPath: undefined, taskId: 'task-curate-3'}) + await hook.onTaskCreate(task) + + const created = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_CREATED) + const props = created?.args[1] as Record + expect(props.has_files).to.equal(false) + expect(props.has_folder).to.equal(false) + }) + }) + + describe('query task: terminal emits include task_completed last', () => { + it('emits task_created → query_completed → task_completed in order', async () => { + const task = buildTask('query-tool-mode', {taskId: 'task-query-1'}) + + await hook.onTaskCreate(task) + await hook.onTaskCompleted(task.taskId, '', task) + + expect(eventSequence(trackStub)).to.deep.equal([ + AnalyticsEventNames.TASK_CREATED, + AnalyticsEventNames.QUERY_COMPLETED, + AnalyticsEventNames.TASK_COMPLETED, + ]) + }) + + it('emits the same query-tool-mode task_type across all three events', async () => { + const task = buildTask('query-tool-mode', {taskId: 'task-query-1'}) + await hook.onTaskCreate(task) + await hook.onTaskCompleted(task.taskId, '', task) + + for (const call of trackStub.getCalls()) { + const props = call.args[1] as Record + expect(props.task_type, `${call.args[0] as string} carried wrong task_type`).to.equal('query-tool-mode') + } + }) + }) + + describe('dream-scan / dream-finalize / search: only task_* emits fire (no M12 per-flavor)', () => { + for (const taskType of ['dream-scan', 'dream-finalize', 'search'] as const) { + it(`${taskType}: task_created → task_completed (no curate/query M12 emit)`, async () => { + const task = buildTask(taskType, {taskId: `task-${taskType}-1`}) + await hook.onTaskCreate(task) + await hook.onTaskCompleted(task.taskId, '', task) + + expect(eventSequence(trackStub)).to.deep.equal([ + AnalyticsEventNames.TASK_CREATED, + AnalyticsEventNames.TASK_COMPLETED, + ]) + }) + + it(`${taskType}: onTaskError emits task_created then task_failed (no curate/query M12 emit)`, async () => { + const task = buildTask(taskType, {taskId: `task-${taskType}-2`}) + await hook.onTaskCreate(task) + await hook.onTaskError(task.taskId, 'something blew up', task) + + expect(eventSequence(trackStub)).to.deep.equal([ + AnalyticsEventNames.TASK_CREATED, + AnalyticsEventNames.TASK_FAILED, + ]) + }) + } + }) + + describe('failure + cancellation both surface as task_failed', () => { + it('curate onTaskError emits curate_run_completed(outcome=error) then task_failed', async () => { + const task = buildTask('curate', {taskId: 'task-curate-err'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskError(task.taskId, 'kaboom', task) + + expect(eventSequence(trackStub)).to.deep.equal([ + AnalyticsEventNames.CURATE_RUN_COMPLETED, + AnalyticsEventNames.TASK_FAILED, + ]) + }) + + it('curate onTaskCancelled also emits task_failed (no distinct cancellation event)', async () => { + const task = buildTask('curate', {taskId: 'task-curate-cancel'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskCancelled(task.taskId, task) + + expect(eventSequence(trackStub)).to.deep.equal([ + AnalyticsEventNames.CURATE_RUN_COMPLETED, + AnalyticsEventNames.TASK_FAILED, + ]) + }) + + it('task_failed payload carries duration_ms + task_id + canonical task_type', async () => { + const task = buildTask('query', {taskId: 'task-query-err'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskError(task.taskId, 'kaboom', task) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + const props = failed?.args[1] as Record + expect(props.task_id).to.equal('task-query-err') + expect(props.task_type).to.equal('query') + expect(props.duration_ms).to.equal(1234) + }) + }) +}) diff --git a/test/unit/server/infra/process/analytics-hook.test.ts b/test/unit/server/infra/process/analytics-hook.test.ts index 87bae7141..7f448c5e1 100644 --- a/test/unit/server/infra/process/analytics-hook.test.ts +++ b/test/unit/server/infra/process/analytics-hook.test.ts @@ -114,6 +114,22 @@ describe('AnalyticsHook', () => { hook.setAnalyticsClient(bundle.client) }) + // M14.3 added unconditional task_created / task_completed / task_failed + // emits on every lifecycle callback. Pre-M14.3 tests asserted only the + // M12 per-flavor curate_*/query_completed emits; filter the stub calls + // so existing assertions stay focused on M12 behavior. New M14.3 + // coverage lives in `analytics-hook-m14.test.ts`. + const filterM12 = (stub: sinon.SinonStub): sinon.SinonSpyCall[] => + stub.getCalls().filter((c) => { + const eventName = c.args[0] + return ( + eventName !== AnalyticsEventNames.TASK_CREATED && + eventName !== AnalyticsEventNames.TASK_COMPLETED && + eventName !== AnalyticsEventNames.TASK_FAILED + ) + }) + const m12Calls = (): sinon.SinonSpyCall[] => filterM12(trackStub) + describe('curate task flow', () => { it('emits curate_operation_applied per successful op + bumps matching counter; no event for failed op', async () => { const task = buildCurateTask() @@ -126,9 +142,9 @@ describe('AnalyticsHook', () => { ]) await hook.onToolResult(task.taskId, payload) - expect(trackStub.callCount).to.equal(2) - expect(trackStub.firstCall.args[0]).to.equal(AnalyticsEventNames.CURATE_OPERATION_APPLIED) - const firstProps = trackStub.firstCall.args[1] as Record + expect(m12Calls()).to.have.lengthOf(2) + expect(m12Calls()[0].args[0]).to.equal(AnalyticsEventNames.CURATE_OPERATION_APPLIED) + const firstProps = m12Calls()[0].args[1] as Record expect(firstProps.absolute_path).to.equal('/a.md') expect(firstProps.knowledge_path).to.equal('notes/a') expect(firstProps.operation_type).to.equal('ADD') @@ -137,7 +153,7 @@ describe('AnalyticsHook', () => { expect(firstProps).to.not.have.property('keywords') expect(firstProps).to.not.have.property('related') - const secondProps = trackStub.secondCall.args[1] as Record + const secondProps = m12Calls()[1].args[1] as Record expect(secondProps.needs_review).to.equal(true) expect(secondProps.operation_type).to.equal('UPDATE') }) @@ -157,9 +173,9 @@ describe('AnalyticsHook', () => { await hook.onTaskCompleted(task.taskId, '', task) - expect(trackStub.calledOnce).to.equal(true) - expect(trackStub.firstCall.args[0]).to.equal(AnalyticsEventNames.CURATE_RUN_COMPLETED) - const props = trackStub.firstCall.args[1] as Record + expect(m12Calls()).to.have.lengthOf(1) + expect(m12Calls()[0].args[0]).to.equal(AnalyticsEventNames.CURATE_RUN_COMPLETED) + const props = m12Calls()[0].args[1] as Record expect(props.task_id).to.equal(task.taskId) expect(props.task_type).to.equal('curate') expect(props.outcome).to.equal('completed') @@ -186,7 +202,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.outcome).to.equal('partial') expect(props.operations_failed).to.equal(1) }) @@ -198,7 +214,7 @@ describe('AnalyticsHook', () => { await hook.onTaskError(task.taskId, 'boom', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.outcome).to.equal('error') }) @@ -209,7 +225,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCancelled(task.taskId, task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.outcome).to.equal('cancelled') }) @@ -227,7 +243,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.operations_added).to.equal(1) expect(props.operations_updated).to.equal(1) }) @@ -246,7 +262,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.pending_review_count).to.equal(2) }) @@ -255,7 +271,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCreate(task) await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.task_type).to.equal('curate-folder') }) @@ -267,7 +283,9 @@ describe('AnalyticsHook', () => { buildToolResult([{needsReview: false, path: 'a', status: 'success', type: 'ADD'}]), ) - expect(trackStub.called).to.equal(false) + // No curate_operation_applied for an op missing filePath. (TASK_CREATED + // still fires from onTaskCreate — filtered out via m12Calls().) + expect(m12Calls()).to.have.lengthOf(0) }) }) @@ -298,9 +316,9 @@ describe('AnalyticsHook', () => { } as QueryResultMetadata) await hook.onTaskCompleted(task.taskId, '', task) - expect(trackStub.calledOnce).to.equal(true) - expect(trackStub.firstCall.args[0]).to.equal(AnalyticsEventNames.QUERY_COMPLETED) - const props = trackStub.firstCall.args[1] as Record + expect(m12Calls()).to.have.lengthOf(1) + expect(m12Calls()[0].args[0]).to.equal(AnalyticsEventNames.QUERY_COMPLETED) + const props = m12Calls()[0].args[1] as Record expect(props.task_id).to.equal(task.taskId) expect(props.task_type).to.equal('query') expect(props.outcome).to.equal('completed') @@ -336,7 +354,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCreate(task) await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record const paths = props.read_paths_with_metadata as Array> expect(paths).to.have.lengthOf(10) expect(props.read_doc_count).to.equal(15) // distinct count NOT capped @@ -357,7 +375,7 @@ describe('AnalyticsHook', () => { } as QueryResultMetadata) await localHook.onTaskCompleted(task.taskId, '', task) - const props = localBundle.trackStub.firstCall.args[1] as Record + const props = filterM12(localBundle.trackStub)[0].args[1] as Record expect(props.cache_hit).to.equal(true) }) } @@ -377,7 +395,7 @@ describe('AnalyticsHook', () => { } as QueryResultMetadata) await localHook.onTaskCompleted(task.taskId, '', task) - const props = localBundle.trackStub.firstCall.args[1] as Record + const props = filterM12(localBundle.trackStub)[0].args[1] as Record expect(props.cache_hit).to.equal(false) }) } @@ -387,7 +405,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCreate(task) await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.tier).to.equal(undefined) expect(props.cache_hit).to.equal(false) expect(props.matched_doc_count).to.equal(0) @@ -398,7 +416,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCreate(task) await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props).to.not.have.property('read_paths_with_metadata') // Sanity: counts are zero, not omitted. expect(props.read_doc_count).to.equal(0) @@ -411,7 +429,7 @@ describe('AnalyticsHook', () => { await hook.onTaskError(task.taskId, 'boom', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.outcome).to.equal('error') }) @@ -421,7 +439,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCancelled(task.taskId, task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.outcome).to.equal('cancelled') }) }) @@ -435,18 +453,19 @@ describe('AnalyticsHook', () => { hook.cleanup(curate.taskId) hook.cleanup(query.taskId) - // After cleanup, terminal hooks should be no-ops + // After cleanup, M12 per-flavor emits must NOT fire (no state to read). + // M14.3 generic TASK_COMPLETED still fires unconditionally — filtered. trackStub.resetHistory() await hook.onTaskCompleted(curate.taskId, '', curate) await hook.onTaskCompleted(query.taskId, '', query) - expect(trackStub.called).to.equal(false) + expect(m12Calls()).to.have.lengthOf(0) }) - it('ignores unknown task types (no state created)', async () => { + it('ignores unknown task types (no M12 state created; only generic task_* emits fire)', async () => { const task = buildCurateTask({taskId: 'task-unknown', type: 'unknown' as TaskInfo['type']}) await hook.onTaskCreate(task) await hook.onTaskCompleted(task.taskId, '', task) - expect(trackStub.called).to.equal(false) + expect(m12Calls()).to.have.lengthOf(0) }) it('swallows analyticsClient.track throws (does not propagate)', async () => { @@ -497,7 +516,7 @@ describe('AnalyticsHook', () => { buildToolResult([{filePath, needsReview: false, path: 'a', status: 'success', type: 'ADD'}]), ) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props.tags).to.deep.equal(['t1', 't2']) expect(props.keywords).to.deep.equal(['x', 'y']) expect(props.related).to.deep.equal(['z']) @@ -512,7 +531,7 @@ describe('AnalyticsHook', () => { buildToolResult([{filePath, needsReview: false, path: 'gone', status: 'success', type: 'DELETE'}]), ) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props).to.not.have.property('tags') expect(props).to.not.have.property('keywords') expect(props).to.not.have.property('related') @@ -527,7 +546,7 @@ describe('AnalyticsHook', () => { buildToolResult([{filePath, needsReview: false, path: 'm', status: 'success', type: 'UPDATE'}]), ) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props).to.not.have.property('tags') }) @@ -542,7 +561,7 @@ describe('AnalyticsHook', () => { buildToolResult([{filePath, needsReview: false, path: 'b', status: 'success', type: 'UPDATE'}]), ) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record expect(props).to.not.have.property('tags') }) @@ -559,7 +578,7 @@ describe('AnalyticsHook', () => { buildToolResult([{filePath, needsReview: false, path: 'h', status: 'success', type: 'UPDATE'}]), ) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record const tags = props.tags as string[] expect(tags).to.have.lengthOf(50) expect(tags[0]).to.have.lengthOf(256) @@ -580,7 +599,7 @@ describe('AnalyticsHook', () => { buildToolResult([{filePath, needsReview: false, path: 'g', status: 'success', type: 'UPDATE'}]), ) - const props = disabledBundle.trackStub.firstCall.args[1] as Record + const props = filterM12(disabledBundle.trackStub)[0].args[1] as Record expect(props).to.not.have.property('tags') }) }) @@ -602,7 +621,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCreate(task) await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record const paths = props.read_paths_with_metadata as Array> const byPath = Object.fromEntries(paths.map((p) => [p.absolute_path, p])) expect(byPath[a].tags).to.deep.equal(['ta']) @@ -626,7 +645,7 @@ describe('AnalyticsHook', () => { await hook.onTaskCreate(task) await hook.onTaskCompleted(task.taskId, '', task) - const props = trackStub.firstCall.args[1] as Record + const props = m12Calls()[0].args[1] as Record const paths = props.read_paths_with_metadata as Array> const byPath = Object.fromEntries(paths.map((p) => [p.absolute_path, p])) expect(byPath[real].tags).to.deep.equal(['ok']) @@ -651,7 +670,7 @@ describe('AnalyticsHook', () => { await disabledHook.onTaskCreate(task) await disabledHook.onTaskCompleted(task.taskId, '', task) - const props = disabledBundle.trackStub.firstCall.args[1] as Record + const props = filterM12(disabledBundle.trackStub)[0].args[1] as Record const paths = props.read_paths_with_metadata as Array> expect(paths[0]).to.not.have.property('tags') }) @@ -690,9 +709,9 @@ describe('AnalyticsHook', () => { await Promise.all([p1, p2]) - expect(bundle.trackStub.callCount).to.equal(2) - const first = bundle.trackStub.firstCall.args[1] as Record - const second = bundle.trackStub.secondCall.args[1] as Record + expect(filterM12(bundle.trackStub)).to.have.lengthOf(2) + const first = filterM12(bundle.trackStub)[0].args[1] as Record + const second = filterM12(bundle.trackStub)[1].args[1] as Record expect(first.absolute_path, 'first emit must be op1').to.equal('/op1.md') expect(second.absolute_path, 'second emit must be op2').to.equal('/op2.md') }) @@ -717,15 +736,16 @@ describe('AnalyticsHook', () => { const opPromise = orderHook.onToolResult(task.taskId, payload) const completePromise = orderHook.onTaskCompleted(task.taskId, '', task) - // Neither emit can have fired yet — read is still pending. - expect(bundle.trackStub.called, 'no emit before read settles').to.equal(false) + // Neither M12 emit can have fired yet — read is still pending. (TASK_CREATED + // from M14.3 already fired during onTaskCreate but doesn't gate on the read.) + expect(filterM12(bundle.trackStub), 'no M12 emit before read settles').to.have.lengthOf(0) d.resolve(buildFrontmatterDoc('tag-x')) await Promise.all([opPromise, completePromise]) - expect(bundle.trackStub.callCount).to.equal(2) - expect(bundle.trackStub.firstCall.args[0]).to.equal(AnalyticsEventNames.CURATE_OPERATION_APPLIED) - expect(bundle.trackStub.secondCall.args[0]).to.equal(AnalyticsEventNames.CURATE_RUN_COMPLETED) + expect(filterM12(bundle.trackStub)).to.have.lengthOf(2) + expect(filterM12(bundle.trackStub)[0].args[0]).to.equal(AnalyticsEventNames.CURATE_OPERATION_APPLIED) + expect(filterM12(bundle.trackStub)[1].args[0]).to.equal(AnalyticsEventNames.CURATE_RUN_COMPLETED) }) it('readFile rejection is swallowed: emit fires with frontmatter fields omitted; daemon does not crash', async () => { @@ -743,8 +763,8 @@ describe('AnalyticsHook', () => { await errorHook.onToolResult(task.taskId, payload) - expect(bundle.trackStub.calledOnce).to.equal(true) - const props = bundle.trackStub.firstCall.args[1] as Record + expect(filterM12(bundle.trackStub)).to.have.lengthOf(1) + const props = filterM12(bundle.trackStub)[0].args[1] as Record expect(props.absolute_path).to.equal('/missing.md') expect(props).to.not.have.property('keywords') expect(props).to.not.have.property('tags') @@ -769,7 +789,10 @@ describe('AnalyticsHook', () => { // directly, but the assertion below catches the leak: a new task with the same // id observes a fresh in-memory state. await cleanupHook.onTaskCreate(task) - expect(bundle.trackStub.callCount, 'no replay after cleanup').to.equal(2) + // M12 emits: 1 curate_operation_applied + 1 curate_run_completed = 2. + // Re-creating the task after cleanup must NOT replay either; it only + // adds another TASK_CREATED (filtered out below). + expect(filterM12(bundle.trackStub), 'no replay after cleanup').to.have.lengthOf(2) }) }) }) From a943aacbabb40f9ed26cb54f3dc5ab15c8c235ac Mon Sep 17 00:00:00 2001 From: Cuong Date: Wed, 27 May 2026 17:26:58 +0700 Subject: [PATCH 4/7] =?UTF-8?q?refactor:=20[ENG-2971]=20M14.3=20review=20f?= =?UTF-8?q?ix=20=E2=80=94=20relative=20paths,=20required=20keywords/tags,?= =?UTF-8?q?=20structured=20related=5Fpaths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tightens the curate / query M12 payloads per code review: curate_operation_applied: - rename absolute_path → relative_path (project-relative via path.relative against the task's projectPath; falls back to identity when projectPath is unset so search tasks at the daemon root still emit a usable string) - keywords / tags promoted from optional to required arrays (default []) so the wire shape stays uniform regardless of frontmatter read success query_completed.read_paths_with_metadata: - same absolute_path → relative_path rename - same keywords / tags promotion (required arrays, default []) - flat related: string[] → structured related_paths: [{relative_path, keywords, tags}] so each linked topic carries its own metadata slot. Keywords/tags default to [] until a future FU cascade-reads each linked file's frontmatter — the wire shape doesn't need to change when that lands. Hook implementation: - new `toRelativePath(filePath, projectPath)` helper using node:path.relative - CurateTaskAnalyticsState stores projectPath captured at onTaskCreate so per-op emits can relativize without threading task through processToolResult - all four payload sites (curate-op, curate-run-completed, query-completed read_paths_with_metadata) route through the helper Inspection test added at test/unit/.../analytics-hook-toolmode-inspection that pretty-prints every event + payload for curate-tool-mode / query / query-tool-mode flows — gives PMs a single place to verify the wire shape end-to-end. Privacy win: relative paths drop the /Users/{name} prefix from every file-touched event, keeping host-identifiable PII off the analytics wire while still letting PMs reason about which file inside a project an operation touched. Tests: full repo 9132 passing. --- src/server/infra/process/analytics-hook.ts | 50 +++- .../events/curate-operation-applied.ts | 20 +- .../analytics/events/query-completed.ts | 37 ++- test/integration/analytics/transport.test.ts | 6 +- .../analytics-hook-async-stress.test.ts | 16 +- .../infra/analytics/analytics-client.test.ts | 8 +- .../analytics/no-op-analytics-client.test.ts | 4 +- ...analytics-hook-toolmode-inspection.test.ts | 260 ++++++++++++++++++ .../infra/process/analytics-hook.test.ts | 78 +++--- .../handlers/analytics-handler.test.ts | 10 +- .../events/curate-operation-applied.test.ts | 21 +- .../analytics/events/query-completed.test.ts | 84 ++++-- 12 files changed, 499 insertions(+), 95 deletions(-) create mode 100644 test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts diff --git a/src/server/infra/process/analytics-hook.ts b/src/server/infra/process/analytics-hook.ts index 307c7bba6..551cf8340 100644 --- a/src/server/infra/process/analytics-hook.ts +++ b/src/server/infra/process/analytics-hook.ts @@ -1,5 +1,6 @@ /* eslint-disable camelcase */ import {readFile as readFileAsync} from 'node:fs/promises' +import {relative as relativePath} from 'node:path' import type {AnalyticsEventName} from '../../../shared/analytics/event-names.js' import type {CurateRunCompletedProps} from '../../../shared/analytics/events/curate-run-completed.js' @@ -32,6 +33,22 @@ function toAnalyticsTaskType(daemonType: string): TaskType { return daemonType as TaskType } +/** + * Convert an absolute filesystem path to a project-relative path for the + * analytics wire. Falls back to the input unchanged when projectPath is + * unset (e.g., search tasks scoped to the daemon root). Keeps emits free + * of `/Users/{name}` PII while still letting PMs reason about which file + * inside a project an operation touched. + */ +function toRelativePath(filePath: string, projectPath?: string): string { + if (!projectPath) return filePath + const rel = relativePath(projectPath, filePath) + // `path.relative` returns '' when paths are identical — defensively + // surface a leaf token rather than emit a zero-length wire string that + // would fail `z.string().min(1)`. + return rel === '' ? '.' : rel +} + // `CURATE_TASK_TYPES` is exported as a readonly tuple; wrap in a Set // for cast-free `.has()` lookups against TaskInfo.type (string). const CURATE_TASK_TYPE_SET: ReadonlySet = new Set(CURATE_TASK_TYPES) @@ -81,6 +98,8 @@ type CurateCounters = { type CurateTaskAnalyticsState = { counters: CurateCounters flavor: 'curate' + /** Captured at onTaskCreate so onToolResult emits can relativize op.filePath. */ + projectPath?: string taskType: CurateTaskTypeLiteral } @@ -206,6 +225,7 @@ export class AnalyticsHook implements ITaskLifecycleHook { this.tasks.set(task.taskId, { counters: {added: 0, deleted: 0, failed: 0, merged: 0, pendingReview: 0, updated: 0}, flavor: 'curate', + projectPath: task.projectPath, taskType: task.type, }) return @@ -338,17 +358,24 @@ export class AnalyticsHook implements ITaskLifecycleHook { // M12.3: harvest per-path frontmatter on the same async read path used // for curate emits. Entries whose file is unreadable / has no frontmatter - // carry `absolute_path` alone (the three array fields stay absent). - // `Promise.all` preserves input-array order in the result regardless of - // which read settles first. + // carry empty keywords / tags / related_paths arrays — the wire shape + // is uniform regardless of read success. `Promise.all` preserves + // input-array order in the result regardless of which read settles first. const readPathsWithMetadata = await Promise.all( cappedPaths.map(async (p) => { const fm = await this.readFrontmatterFields(p) return { - absolute_path: p, - ...(fm.keywords ? {keywords: fm.keywords} : {}), - ...(fm.related ? {related: fm.related} : {}), - ...(fm.tags ? {tags: fm.tags} : {}), + keywords: fm.keywords ?? [], + // M14 review tightening: each related entry is structured so a + // later FU can populate the linked file's own keywords/tags + // without changing the wire shape. + related_paths: (fm.related ?? []).map((r) => ({ + keywords: [], + relative_path: r, + tags: [], + })), + relative_path: toRelativePath(p, task.projectPath), + tags: fm.tags ?? [], } }), ) @@ -472,20 +499,21 @@ export class AnalyticsHook implements ITaskLifecycleHook { // M12.3: read post-op frontmatter for ADD / UPDATE / MERGE-target / // UPSERT. DELETE skips the read (file is gone). Frontmatter fields - // stay absent when the read fails (ENOENT, EACCES, malformed YAML). + // default to empty arrays when the read fails (ENOENT, EACCES, + // malformed YAML) so the wire shape stays uniform. // eslint-disable-next-line no-await-in-loop -- emit order MUST match op order const frontmatter = op.type === 'DELETE' ? {} : await this.readFrontmatterFields(op.filePath) this.emit(AnalyticsEventNames.CURATE_OPERATION_APPLIED, { - absolute_path: op.filePath, ...(op.confidence ? {confidence: op.confidence} : {}), ...(op.impact ? {impact: op.impact} : {}), - ...(frontmatter.keywords ? {keywords: frontmatter.keywords} : {}), + keywords: frontmatter.keywords ?? [], knowledge_path: op.path, needs_review: op.needsReview ?? false, operation_type: op.type, ...(frontmatter.related ? {related: frontmatter.related} : {}), - ...(frontmatter.tags ? {tags: frontmatter.tags} : {}), + relative_path: toRelativePath(op.filePath, state.projectPath), + tags: frontmatter.tags ?? [], task_id: taskId, }) } diff --git a/src/shared/analytics/events/curate-operation-applied.ts b/src/shared/analytics/events/curate-operation-applied.ts index 12f4a0564..c7e9839b0 100644 --- a/src/shared/analytics/events/curate-operation-applied.ts +++ b/src/shared/analytics/events/curate-operation-applied.ts @@ -5,24 +5,28 @@ import {z} from 'zod' * Per-event schema for `curate_operation_applied`. * * Emitted by the daemon's `AnalyticsHook` (M12.2) once per successful curate - * operation. Each operation carries the affected file's absolute path, its - * knowledge-tree address, review/impact metadata, and (M12.3) the file's - * current-state frontmatter values for tags / keywords / related. + * operation. Each operation carries the affected file's project-relative + * path, its knowledge-tree address, review/impact metadata, and (M12.3) the + * file's current-state frontmatter values for tags / keywords / related. * - * All three frontmatter arrays are optional and absent on DELETE operations - * (the file is gone post-op) and on read failures (defensive). + * Review tightening (M14 follow-up): + * - `absolute_path` → `relative_path` for privacy + portability across hosts + * - `keywords` / `tags` are now required arrays (default empty) so consumers + * don't have to special-case the "field absent" shape + * - `related` stays optional and absent on DELETE / read-failure (file is + * gone or unreadable, no related-link source to harvest from) */ export const CurateOperationAppliedSchema = z .object({ - absolute_path: z.string().min(1), confidence: z.enum(['high', 'low']).optional(), impact: z.enum(['high', 'low']).optional(), - keywords: z.array(z.string().max(256)).max(50).optional(), + keywords: z.array(z.string().max(256)).max(50), knowledge_path: z.string().min(1), needs_review: z.boolean(), operation_type: z.enum(['ADD', 'UPDATE', 'DELETE', 'MERGE', 'UPSERT']), related: z.array(z.string().max(256)).max(50).optional(), - tags: z.array(z.string().max(256)).max(50).optional(), + relative_path: z.string().min(1), + tags: z.array(z.string().max(256)).max(50), task_id: z.string().min(1), }) .strict() diff --git a/src/shared/analytics/events/query-completed.ts b/src/shared/analytics/events/query-completed.ts index f6058897a..d7bc2865e 100644 --- a/src/shared/analytics/events/query-completed.ts +++ b/src/shared/analytics/events/query-completed.ts @@ -3,18 +3,41 @@ import {z} from 'zod' import {TASK_TYPE_VALUES} from '../task-types.js' +/** + * Per related-path metadata. Each related entry is a project-relative + * knowledge path captured from a read file's frontmatter `related` list, + * carrying its own keywords / tags so PMs can see what the linked-from + * topics actually cover. + * + * keywords / tags default to `[]` when the related file isn't on disk or + * when analytics is disabled (no enrichment read happens). The shape is + * structured here so a later FU can fill keywords/tags without a wire + * format change. + */ +const RelatedPathWithMetadataSchema = z + .object({ + keywords: z.array(z.string().max(256)).max(50), + relative_path: z.string().min(1), + tags: z.array(z.string().max(256)).max(50), + }) + .strict() + /** * Per-file structure inside `query_completed.read_paths_with_metadata`. - * Frontmatter arrays are optional and absent when the daemon cannot read - * the file (ENOENT, parse failure) — `absolute_path` alone still tells - * PMs which file the agent touched. + * + * Review tightening (M14 follow-up): + * - `absolute_path` → `relative_path` for privacy + portability + * - `keywords` / `tags` are now required arrays (default `[]`) so the + * "field absent" wire shape goes away + * - flat `related: string[]` → structured `related_paths: [{relative_path, + * keywords, tags}]` so each linked topic carries its own metadata */ const ReadPathWithMetadataSchema = z .object({ - absolute_path: z.string().min(1), - keywords: z.array(z.string().max(256)).max(50).optional(), - related: z.array(z.string().max(256)).max(50).optional(), - tags: z.array(z.string().max(256)).max(50).optional(), + keywords: z.array(z.string().max(256)).max(50), + related_paths: z.array(RelatedPathWithMetadataSchema).max(50), + relative_path: z.string().min(1), + tags: z.array(z.string().max(256)).max(50), }) .strict() diff --git a/test/integration/analytics/transport.test.ts b/test/integration/analytics/transport.test.ts index 7e8bba997..157cc90f6 100644 --- a/test/integration/analytics/transport.test.ts +++ b/test/integration/analytics/transport.test.ts @@ -99,10 +99,12 @@ describe('analytics:track transport round-trip integration (M2.6)', () => { { event: AnalyticsEventNames.CURATE_OPERATION_APPLIED, properties: { - absolute_path: '/tmp/x.md', + keywords: [], knowledge_path: 'kg/x.md', needs_review: false, operation_type: 'ADD', + relative_path: 'tmp/x.md', + tags: [], task_id: 't-1', }, }, @@ -118,7 +120,7 @@ describe('analytics:track transport round-trip integration (M2.6)', () => { expect(event.identity).to.deep.equal({device_id: validDeviceId}) // User-supplied properties preserved end-to-end - expect(event.properties.absolute_path).to.equal('/tmp/x.md') + expect(event.properties.relative_path).to.equal('tmp/x.md') expect(event.properties.operation_type).to.equal('ADD') // All five super-properties stamped on receipt diff --git a/test/integration/infra/process/analytics-hook-async-stress.test.ts b/test/integration/infra/process/analytics-hook-async-stress.test.ts index 417f1fa62..59951c20b 100644 --- a/test/integration/infra/process/analytics-hook-async-stress.test.ts +++ b/test/integration/infra/process/analytics-hook-async-stress.test.ts @@ -15,6 +15,7 @@ import {expect} from 'chai' import {randomUUID} from 'node:crypto' +import {relative as relativePath} from 'node:path' import {createSandbox, type SinonSandbox, type SinonStub} from 'sinon' import type {LlmToolResultEvent} from '../../../../src/server/core/domain/transport/schemas.js' @@ -260,11 +261,12 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { // readFile call order must match arrival order (proves per-task queue). expect(readFileCallOrder, 'readFile call order = arrival order').to.deep.equal(opSpecs.map((s) => s.filePath)) - // Emit order must match arrival order. + // Emit order must match arrival order. M14 review: relative_path is + // relativized against the curate task's projectPath ('/proj'). const emits = getCurateOpEmits(taskId) expect(emits).to.have.lengthOf(20) for (const [i, emit] of emits.entries()) { - expect(emit.absolute_path, `emit #${i} arrival order`).to.equal(opSpecs[i].filePath) + expect(emit.relative_path, `emit #${i} arrival order`).to.equal(relativePath('/proj', opSpecs[i].filePath)) } }) @@ -297,8 +299,8 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { expect(xEmits).to.have.lengthOf(15) expect(yEmits).to.have.lengthOf(15) for (let i = 0; i < 15; i++) { - expect(xEmits[i].absolute_path, `X emit #${i}`).to.equal(xSpecs[i].filePath) - expect(yEmits[i].absolute_path, `Y emit #${i}`).to.equal(ySpecs[i].filePath) + expect(xEmits[i].relative_path, `X emit #${i}`).to.equal(relativePath('/proj', xSpecs[i].filePath)) + expect(yEmits[i].relative_path, `Y emit #${i}`).to.equal(relativePath('/proj', ySpecs[i].filePath)) } }) @@ -340,7 +342,7 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { // And per-op emit order matches arrival order. const opEmits = getCurateOpEmits(taskId) for (let i = 0; i < 50; i++) { - expect(opEmits[i].absolute_path, `op #${i} arrival order`).to.equal(specs[i].filePath) + expect(opEmits[i].relative_path, `op #${i} arrival order`).to.equal(relativePath('/proj', specs[i].filePath)) } }) @@ -394,7 +396,9 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { expect(sequence[12], `${id}: M14.3 task_completed is last`).to.equal(AnalyticsEventNames.TASK_COMPLETED) const opEmits = getCurateOpEmits(id) for (let i = 0; i < 10; i++) { - expect(opEmits[i].absolute_path, `${id} op #${i} arrival order`).to.equal(specsByTask[id][i].filePath) + expect(opEmits[i].relative_path, `${id} op #${i} arrival order`).to.equal( + relativePath('/proj', specsByTask[id][i].filePath), + ) } } }) diff --git a/test/unit/server/infra/analytics/analytics-client.test.ts b/test/unit/server/infra/analytics/analytics-client.test.ts index 023bdeaf6..906614ec0 100644 --- a/test/unit/server/infra/analytics/analytics-client.test.ts +++ b/test/unit/server/infra/analytics/analytics-client.test.ts @@ -23,10 +23,12 @@ import {AnalyticsEventNames} from '../../../../../src/shared/analytics/event-nam */ function makeCurateOpProps(overrides: Partial = {}): CurateOperationAppliedProps { return { - absolute_path: '/tmp/file.md', + keywords: [], knowledge_path: 'cli_architecture/test.md', needs_review: false, operation_type: 'ADD', + relative_path: 'tmp/file.md', + tags: [], task_id: 'task-1', ...overrides, } @@ -233,7 +235,7 @@ describe('AnalyticsClient', () => { }) const before = Date.now() - const opProps = makeCurateOpProps({absolute_path: '/tmp/merge-fixture.md'}) + const opProps = makeCurateOpProps({relative_path: 'tmp/merge-fixture.md'}) client.track(AnalyticsEventNames.CURATE_OPERATION_APPLIED, opProps) await flushMicrotasks() const after = Date.now() @@ -248,7 +250,7 @@ describe('AnalyticsClient', () => { expect(event.timestamp).to.be.at.most(after) // user properties merged through - expect(event.properties.absolute_path).to.equal('/tmp/merge-fixture.md') + expect(event.properties.relative_path).to.equal('tmp/merge-fixture.md') expect(event.properties.operation_type).to.equal('ADD') // all 5 super properties stamped expect(event.properties.cli_version).to.equal('3.10.3') diff --git a/test/unit/server/infra/analytics/no-op-analytics-client.test.ts b/test/unit/server/infra/analytics/no-op-analytics-client.test.ts index dd88cb076..f178efa2b 100644 --- a/test/unit/server/infra/analytics/no-op-analytics-client.test.ts +++ b/test/unit/server/infra/analytics/no-op-analytics-client.test.ts @@ -15,10 +15,12 @@ describe('NoOpAnalyticsClient', () => { // CURATE_OPERATION_APPLIED with a minimal valid payload. expect(() => client.track(AnalyticsEventNames.CURATE_OPERATION_APPLIED, { - absolute_path: '/tmp/x.md', + keywords: [], knowledge_path: 'kg/x.md', needs_review: false, operation_type: 'ADD', + relative_path: 'tmp/x.md', + tags: [], task_id: 't-1', }), ).to.not.throw() diff --git a/test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts b/test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts new file mode 100644 index 000000000..57ae6b75b --- /dev/null +++ b/test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts @@ -0,0 +1,260 @@ +/* eslint-disable camelcase */ +import {expect} from 'chai' +import sinon from 'sinon' + +import type {LlmToolResultEvent} from '../../../../../src/server/core/domain/transport/schemas.js' +import type {TaskInfo} from '../../../../../src/server/core/domain/transport/task-info.js' +import type {IAnalyticsClient} from '../../../../../src/server/core/interfaces/analytics/i-analytics-client.js' +import type {QueryResultMetadata} from '../../../../../src/server/infra/process/query-log-handler.js' + +import {AnalyticsBatch} from '../../../../../src/server/core/domain/analytics/batch.js' +import {AnalyticsHook} from '../../../../../src/server/infra/process/analytics-hook.js' + +const NOW = 1_700_000_000_000 + +const buildClient = (): {client: IAnalyticsClient; trackStub: sinon.SinonStub} => { + const trackStub = sinon.stub() + const client: IAnalyticsClient = { + abort() { + /* noop */ + }, + flush: sinon.stub().resolves(AnalyticsBatch.create([])), + getRuntimeState: () => Promise.resolve({droppedCount: 0, lastSuccessfulFlushAt: undefined, queueDepth: 0}), + onAuthTransition: sinon.stub().resolves(), + track: trackStub, + } + return {client, trackStub} +} + +const buildToolModeCurateTask = (overrides: Partial = {}): TaskInfo => + ({ + clientId: 'agent-1', + completedAt: NOW + 4321, + content: 'JSON envelope describing the html the calling agent already wrote', + createdAt: NOW, + // Daemon today still dispatches the pre-ENG-2925 name; analytics + // aliases it to 'curate-tool-mode' on the wire via toAnalyticsTaskType. + projectPath: '/Users/dev/example-project', + taskId: 'task-curate-tm-1', + type: 'curate-html-direct', + ...overrides, + }) as TaskInfo + +const buildToolModeQueryTask = (overrides: Partial = {}): TaskInfo => + ({ + clientId: 'agent-1', + completedAt: NOW + 987, + content: 'how does the auth middleware work', + createdAt: NOW, + projectPath: '/Users/dev/example-project', + taskId: 'task-query-tm-1', + toolCalls: [], + type: 'query-tool-mode', + ...overrides, + }) as TaskInfo + +async function fakeReadFileForInspection(filePath: string): Promise { + if (filePath === '/Users/dev/example-project/.brv/notes/auth.md') { + return '---\nkeywords: ["jwt", "session"]\nrelated: ["auth/middleware", "users"]\ntags: ["security"]\n---\nbody\n' + } + + return '---\n---\nempty\n' +} + +const dumpEvents = (label: string, trackStub: sinon.SinonStub): void => { + console.log(`\n┌─ ${label} ${'─'.repeat(Math.max(0, 70 - label.length))}`) + for (const [i, call] of trackStub.getCalls().entries()) { + const eventName = call.args[0] as string + const props = call.args[1] as Record + console.log(`│ [${i}] ${eventName}`) + console.log(`│ ${JSON.stringify(props, null, 2).replaceAll('\n', '\n│ ')}`) + } + + console.log(`└${'─'.repeat(72)}\n`) +} + +describe('analytics-hook tool-mode event inspection (M14)', () => { + it('curate-tool-mode: prints every event + payload the daemon emits to analytics', async () => { + const {client, trackStub} = buildClient() + const hook = new AnalyticsHook() + hook.setAnalyticsClient(client) + + const task = buildToolModeCurateTask() + await hook.onTaskCreate(task) + await hook.onTaskCompleted(task.taskId, '', task) + + dumpEvents('curate-tool-mode — success path', trackStub) + + // Sanity: every event carries the canonical post-rename task_type + for (const call of trackStub.getCalls()) { + const props = call.args[1] as Record + expect(props.task_type, `${call.args[0] as string} task_type`).to.equal('curate-tool-mode') + } + + // Counters all-zero today because onToolResult never fires for + // tool-mode (no LLM tool calls) — that's the FU-1 follow-up. + const runCompleted = trackStub.getCalls().find((c) => c.args[0] === 'curate_run_completed') + const counters = runCompleted?.args[1] as Record + expect(counters.operations_added).to.equal(0) + expect(counters.operations_updated).to.equal(0) + expect(counters.operations_deleted).to.equal(0) + expect(counters.operations_merged).to.equal(0) + expect(counters.operations_failed).to.equal(0) + expect(counters.pending_review_count).to.equal(0) + }) + + it('curate-tool-mode: error path — prints curate_run_completed(outcome=error) + task_failed', async () => { + const {client, trackStub} = buildClient() + const hook = new AnalyticsHook() + hook.setAnalyticsClient(client) + + const task = buildToolModeCurateTask({taskId: 'task-curate-tm-err'}) + await hook.onTaskCreate(task) + await hook.onTaskError(task.taskId, 'writer rejected: path-exists', task) + + dumpEvents('curate-tool-mode — error path', trackStub) + }) + + it('curate-tool-mode: with a successful tool-result op (FU-1 forward-look — what counters WOULD look like)', async () => { + const {client, trackStub} = buildClient() + const hook = new AnalyticsHook() + hook.setAnalyticsClient(client) + + const task = buildToolModeCurateTask({taskId: 'task-curate-tm-fu1'}) + await hook.onTaskCreate(task) + + // Simulate a curate-op as if FU-1 had synthesised one from task.result + // (today's tool-mode path doesn't fire onToolResult — FU-1 fixes that). + const simulatedOp: LlmToolResultEvent = { + callId: 'sim-1', + result: JSON.stringify({ + applied: [ + { + filePath: '/Users/dev/example-project/.brv/notes/auth.md', + needsReview: false, + path: 'auth', + status: 'success', + type: 'ADD', + }, + ], + }), + sessionId: 'sim-session', + taskId: task.taskId, + timestamp: NOW, + toolName: 'curate' as const, + } as unknown as LlmToolResultEvent + await hook.onToolResult(task.taskId, simulatedOp) + await hook.onTaskCompleted(task.taskId, '', task) + + dumpEvents('curate-tool-mode — FU-1 forward-look (single synthetic op)', trackStub) + }) + + it('query-tool-mode: prints every event + payload the daemon emits to analytics', async () => { + const {client, trackStub} = buildClient() + const hook = new AnalyticsHook() + hook.setAnalyticsClient(client) + + const task = buildToolModeQueryTask() + await hook.onTaskCreate(task) + await hook.onTaskCompleted(task.taskId, '', task) + + dumpEvents('query-tool-mode — success path (no setQueryResult)', trackStub) + + for (const call of trackStub.getCalls()) { + const props = call.args[1] as Record + expect(props.task_type, `${call.args[0] as string} task_type`).to.equal('query-tool-mode') + } + + // No setQueryResult call → matched_doc_count + read_doc_count both 0, + // tier omitted, read_paths_with_metadata omitted. That's the + // empty-metadata state FU-1's query half closes. + const queryCompleted = trackStub.getCalls().find((c) => c.args[0] === 'query_completed') + const props = queryCompleted?.args[1] as Record + expect(props.matched_doc_count).to.equal(0) + expect(props.read_doc_count).to.equal(0) + expect(props.cache_hit).to.equal(false) + expect(props.tier).to.equal(undefined) + expect(props.read_paths_with_metadata).to.equal(undefined) + }) + + it('query-tool-mode: with setQueryResult (forward-look from FU-1) — populated metadata', async () => { + const {client, trackStub} = buildClient() + const hook = new AnalyticsHook() + hook.setAnalyticsClient(client) + + const task = buildToolModeQueryTask({taskId: 'task-query-tm-fu1'}) + await hook.onTaskCreate(task) + + const metadata: QueryResultMetadata = { + matchedDocs: [], + searchMetadata: {resultCount: 4, topScore: 0.82, totalFound: 4}, + tier: 2, + timing: {durationMs: 987}, + } as QueryResultMetadata + hook.setQueryResult(task.taskId, metadata) + + await hook.onTaskCompleted(task.taskId, '', task) + + dumpEvents('query-tool-mode — FU-1 forward-look (setQueryResult populated)', trackStub) + }) + + it('query (legacy): read_paths_with_metadata carries structured related_paths + relative_path + keywords/tags arrays', async () => { + const trackStub = sinon.stub() + const client: IAnalyticsClient = { + abort() { + /* noop */ + }, + flush: sinon.stub().resolves(AnalyticsBatch.create([])), + getRuntimeState: () => Promise.resolve({droppedCount: 0, lastSuccessfulFlushAt: undefined, queueDepth: 0}), + onAuthTransition: sinon.stub().resolves(), + track: trackStub, + } + const hook = new AnalyticsHook({readFile: fakeReadFileForInspection}) + hook.setAnalyticsClient(client) + + const task = buildToolModeQueryTask({ + taskId: 'task-query-paths-1', + toolCalls: [ + { + args: {filePath: '/Users/dev/example-project/.brv/notes/auth.md'}, + sessionId: 's1', + status: 'completed', + timestamp: NOW, + toolName: 'read_file', + }, + ], + type: 'query', + } as Partial) + + await hook.onTaskCreate(task) + await hook.onTaskCompleted(task.taskId, '', task) + + dumpEvents('query (legacy) — read_paths_with_metadata + related_paths structure', trackStub) + + const queryCompleted = trackStub.getCalls().find((c) => c.args[0] === 'query_completed') + const props = queryCompleted?.args[1] as Record + const paths = props.read_paths_with_metadata as Array> + expect(paths).to.have.lengthOf(1) + + const entry = paths[0] + expect(entry.relative_path).to.equal('.brv/notes/auth.md') + expect(entry.keywords).to.deep.equal(['jwt', 'session']) + expect(entry.tags).to.deep.equal(['security']) + expect(entry.related_paths).to.deep.equal([ + {keywords: [], relative_path: 'auth/middleware', tags: []}, + {keywords: [], relative_path: 'users', tags: []}, + ]) + }) + + it('query-tool-mode: error path', async () => { + const {client, trackStub} = buildClient() + const hook = new AnalyticsHook() + hook.setAnalyticsClient(client) + + const task = buildToolModeQueryTask({taskId: 'task-query-tm-err'}) + await hook.onTaskCreate(task) + await hook.onTaskError(task.taskId, 'connector unreachable', task) + + dumpEvents('query-tool-mode — error path', trackStub) + }) +}) diff --git a/test/unit/server/infra/process/analytics-hook.test.ts b/test/unit/server/infra/process/analytics-hook.test.ts index 7f448c5e1..48a566ecb 100644 --- a/test/unit/server/infra/process/analytics-hook.test.ts +++ b/test/unit/server/infra/process/analytics-hook.test.ts @@ -145,12 +145,13 @@ describe('AnalyticsHook', () => { expect(m12Calls()).to.have.lengthOf(2) expect(m12Calls()[0].args[0]).to.equal(AnalyticsEventNames.CURATE_OPERATION_APPLIED) const firstProps = m12Calls()[0].args[1] as Record - expect(firstProps.absolute_path).to.equal('/a.md') + // buildCurateTask sets projectPath:'/project'; path.relative('/project','/a.md') = '../a.md' + expect(firstProps.relative_path).to.equal('../a.md') expect(firstProps.knowledge_path).to.equal('notes/a') expect(firstProps.operation_type).to.equal('ADD') expect(firstProps.needs_review).to.equal(false) - expect(firstProps).to.not.have.property('tags') - expect(firstProps).to.not.have.property('keywords') + expect(firstProps.tags).to.deep.equal([]) + expect(firstProps.keywords).to.deep.equal([]) expect(firstProps).to.not.have.property('related') const secondProps = m12Calls()[1].args[1] as Record @@ -331,13 +332,14 @@ describe('AnalyticsHook', () => { expect(props.matched_doc_count).to.equal(7) const paths = props.read_paths_with_metadata as Array> expect(paths).to.have.lengthOf(3) - // sorted lexicographically - expect(paths.map((p) => p.absolute_path)).to.deep.equal(['/a.md', '/b.md', '/c.md']) - // each entry has only absolute_path, no metadata in M12.2 + // sorted lexicographically; relativized against projectPath:'/project' + expect(paths.map((p) => p.relative_path)).to.deep.equal(['../a.md', '../b.md', '../c.md']) + // each entry has empty keywords/tags arrays and an empty related_paths + // list — no frontmatter source files exist in this in-memory test. for (const entry of paths) { - expect(entry).to.not.have.property('tags') - expect(entry).to.not.have.property('keywords') - expect(entry).to.not.have.property('related') + expect(entry.tags).to.deep.equal([]) + expect(entry.keywords).to.deep.equal([]) + expect(entry.related_paths).to.deep.equal([]) } }) @@ -522,7 +524,7 @@ describe('AnalyticsHook', () => { expect(props.related).to.deep.equal(['z']) }) - it('omits tags/keywords/related on DELETE ops (file gone post-op)', async () => { + it('keywords/tags default to empty arrays on DELETE ops (file gone post-op); related stays omitted', async () => { const filePath = join(tmpDir, 'gone.md') const task = buildCurateTask() await hook.onTaskCreate(task) @@ -532,12 +534,12 @@ describe('AnalyticsHook', () => { ) const props = m12Calls()[0].args[1] as Record - expect(props).to.not.have.property('tags') - expect(props).to.not.have.property('keywords') + expect(props.tags).to.deep.equal([]) + expect(props.keywords).to.deep.equal([]) expect(props).to.not.have.property('related') }) - it('omits tags/keywords/related when filePath cannot be read (ENOENT)', async () => { + it('keywords/tags default to empty arrays when filePath cannot be read (ENOENT)', async () => { const filePath = join(tmpDir, 'missing.md') const task = buildCurateTask() await hook.onTaskCreate(task) @@ -547,10 +549,11 @@ describe('AnalyticsHook', () => { ) const props = m12Calls()[0].args[1] as Record - expect(props).to.not.have.property('tags') + expect(props.tags).to.deep.equal([]) + expect(props.keywords).to.deep.equal([]) }) - it('omits tags/keywords/related on malformed YAML (no throw)', async () => { + it('keywords/tags default to empty arrays on malformed YAML (no throw)', async () => { const filePath = join(tmpDir, 'bad.md') writeFileSync(filePath, '---\nthis is: not [valid YAML\n---\nbody', 'utf8') @@ -562,7 +565,8 @@ describe('AnalyticsHook', () => { ) const props = m12Calls()[0].args[1] as Record - expect(props).to.not.have.property('tags') + expect(props.tags).to.deep.equal([]) + expect(props.keywords).to.deep.equal([]) }) it('caps arrays at 50 entries and strings at 256 chars per entry', async () => { @@ -584,7 +588,7 @@ describe('AnalyticsHook', () => { expect(tags[0]).to.have.lengthOf(256) }) - it('skips file reads entirely when isEnabled() returns false', async () => { + it('skips file reads entirely when isEnabled() returns false; keywords/tags fall back to []', async () => { const filePath = join(tmpDir, 'gated.md') writeMarkdown(filePath, {tags: ['should-not-appear']}) @@ -600,7 +604,8 @@ describe('AnalyticsHook', () => { ) const props = filterM12(disabledBundle.trackStub)[0].args[1] as Record - expect(props).to.not.have.property('tags') + expect(props.tags).to.deep.equal([]) + expect(props.keywords).to.deep.equal([]) }) }) @@ -611,7 +616,9 @@ describe('AnalyticsHook', () => { writeMarkdown(a, {tags: ['ta']}) writeMarkdown(b, {keywords: ['kb']}) + // Pin projectPath to tmpDir so relative_path == 'a.md' / 'b.md'. const task = buildQueryTask({ + projectPath: tmpDir, toolCalls: [ {args: {filePath: a}, sessionId: 's', status: 'completed', timestamp: 1, toolName: 'read_file'}, {args: {filePath: b}, sessionId: 's', status: 'completed', timestamp: 2, toolName: 'read_file'}, @@ -623,19 +630,20 @@ describe('AnalyticsHook', () => { const props = m12Calls()[0].args[1] as Record const paths = props.read_paths_with_metadata as Array> - const byPath = Object.fromEntries(paths.map((p) => [p.absolute_path, p])) - expect(byPath[a].tags).to.deep.equal(['ta']) - expect(byPath[a]).to.not.have.property('keywords') - expect(byPath[b].keywords).to.deep.equal(['kb']) - expect(byPath[b]).to.not.have.property('tags') + const byPath = Object.fromEntries(paths.map((p) => [p.relative_path, p])) + expect(byPath['a.md'].tags).to.deep.equal(['ta']) + expect(byPath['a.md'].keywords).to.deep.equal([]) + expect(byPath['b.md'].keywords).to.deep.equal(['kb']) + expect(byPath['b.md'].tags).to.deep.equal([]) }) - it('mixed readable + ENOENT paths: each entry independently has/omits metadata', async () => { + it('mixed readable + ENOENT paths: each entry has keywords/tags arrays (populated or empty)', async () => { const real = join(tmpDir, 'real.md') const missing = join(tmpDir, 'missing.md') writeMarkdown(real, {tags: ['ok']}) const task = buildQueryTask({ + projectPath: tmpDir, toolCalls: [ {args: {filePath: real}, sessionId: 's', status: 'completed', timestamp: 1, toolName: 'read_file'}, {args: {filePath: missing}, sessionId: 's', status: 'completed', timestamp: 2, toolName: 'read_file'}, @@ -647,9 +655,10 @@ describe('AnalyticsHook', () => { const props = m12Calls()[0].args[1] as Record const paths = props.read_paths_with_metadata as Array> - const byPath = Object.fromEntries(paths.map((p) => [p.absolute_path, p])) - expect(byPath[real].tags).to.deep.equal(['ok']) - expect(byPath[missing]).to.not.have.property('tags') + const byPath = Object.fromEntries(paths.map((p) => [p.relative_path, p])) + expect(byPath['real.md'].tags).to.deep.equal(['ok']) + expect(byPath['missing.md'].tags).to.deep.equal([]) + expect(byPath['missing.md'].keywords).to.deep.equal([]) }) it('skips per-path file reads when isEnabled() returns false', async () => { @@ -672,7 +681,8 @@ describe('AnalyticsHook', () => { const props = filterM12(disabledBundle.trackStub)[0].args[1] as Record const paths = props.read_paths_with_metadata as Array> - expect(paths[0]).to.not.have.property('tags') + expect(paths[0].tags).to.deep.equal([]) + expect(paths[0].keywords).to.deep.equal([]) }) }) }) @@ -712,8 +722,9 @@ describe('AnalyticsHook', () => { expect(filterM12(bundle.trackStub)).to.have.lengthOf(2) const first = filterM12(bundle.trackStub)[0].args[1] as Record const second = filterM12(bundle.trackStub)[1].args[1] as Record - expect(first.absolute_path, 'first emit must be op1').to.equal('/op1.md') - expect(second.absolute_path, 'second emit must be op2').to.equal('/op2.md') + // buildCurateTask projectPath:'/project'; absolute paths relativize with '../' prefix + expect(first.relative_path, 'first emit must be op1').to.equal('../op1.md') + expect(second.relative_path, 'second emit must be op2').to.equal('../op2.md') }) it('onTaskCompleted waits for in-flight onToolResult work before emitting CURATE_RUN_COMPLETED', async () => { @@ -765,9 +776,10 @@ describe('AnalyticsHook', () => { expect(filterM12(bundle.trackStub)).to.have.lengthOf(1) const props = filterM12(bundle.trackStub)[0].args[1] as Record - expect(props.absolute_path).to.equal('/missing.md') - expect(props).to.not.have.property('keywords') - expect(props).to.not.have.property('tags') + // buildCurateTask sets projectPath:'/project'; '/missing.md' relativizes to '../missing.md' + expect(props.relative_path).to.equal('../missing.md') + expect(props.keywords).to.deep.equal([]) + expect(props.tags).to.deep.equal([]) expect(props).to.not.have.property('related') }) diff --git a/test/unit/server/infra/transport/handlers/analytics-handler.test.ts b/test/unit/server/infra/transport/handlers/analytics-handler.test.ts index e469d22eb..ca2461497 100644 --- a/test/unit/server/infra/transport/handlers/analytics-handler.test.ts +++ b/test/unit/server/infra/transport/handlers/analytics-handler.test.ts @@ -64,10 +64,12 @@ describe('AnalyticsHandler', () => { const payload: AnalyticsTrackPayload = { event: AnalyticsEventNames.CURATE_OPERATION_APPLIED, properties: { - absolute_path: '/tmp/a.md', + keywords: [], knowledge_path: 'kg/a.md', needs_review: false, operation_type: 'ADD', + relative_path: 'tmp/a.md', + tags: [], task_id: 't-1', }, } @@ -76,10 +78,12 @@ describe('AnalyticsHandler', () => { expect(analyticsClient.trackCalls).to.have.lengthOf(1) expect(analyticsClient.trackCalls[0].event).to.equal(AnalyticsEventNames.CURATE_OPERATION_APPLIED) expect(analyticsClient.trackCalls[0].properties).to.deep.equal({ - absolute_path: '/tmp/a.md', + keywords: [], knowledge_path: 'kg/a.md', needs_review: false, operation_type: 'ADD', + relative_path: 'tmp/a.md', + tags: [], task_id: 't-1', }) }) @@ -116,7 +120,7 @@ describe('AnalyticsHandler', () => { new AnalyticsHandler({analyticsClient, transport}).setup() const handler = transport._handlers.get(AnalyticsEvents.TRACK) as AnalyticsTrackHandler - // CURATE_OPERATION_APPLIED requires absolute_path / knowledge_path / etc. + // CURATE_OPERATION_APPLIED requires relative_path / knowledge_path / etc. await handler({event: AnalyticsEventNames.CURATE_OPERATION_APPLIED, properties: {wrong: 'shape'}}, 'client-1') // QUERY_COMPLETED requires duration_ms / outcome / etc. await handler({event: AnalyticsEventNames.QUERY_COMPLETED, properties: {}}, 'client-1') diff --git a/test/unit/shared/analytics/events/curate-operation-applied.test.ts b/test/unit/shared/analytics/events/curate-operation-applied.test.ts index b273200d4..9d2915ef1 100644 --- a/test/unit/shared/analytics/events/curate-operation-applied.test.ts +++ b/test/unit/shared/analytics/events/curate-operation-applied.test.ts @@ -4,10 +4,12 @@ import {expect} from 'chai' import {CurateOperationAppliedSchema} from '../../../../../src/shared/analytics/events/curate-operation-applied.js' const baseValid = { - absolute_path: '/Users/dev/project/.brv/context-tree/notes/test.md', + keywords: [], knowledge_path: 'notes/test', needs_review: false, operation_type: 'ADD' as const, + relative_path: '.brv/context-tree/notes/test.md', + tags: [], task_id: 'task-uuid-123', } @@ -33,7 +35,9 @@ describe('CurateOperationAppliedSchema', () => { expect(CurateOperationAppliedSchema.safeParse({...baseValid, needs_review: true}).success).to.equal(true) }) - it('accepts payloads omitting any/all of tags, keywords, related', () => { + it('accepts payloads with keywords / tags as required arrays (default empty) and optional related', () => { + // keywords/tags are required after the M14 review tightening; the + // base payload already carries them as []. expect(CurateOperationAppliedSchema.safeParse({...baseValid}).success).to.equal(true) expect(CurateOperationAppliedSchema.safeParse({...baseValid, tags: ['a']}).success).to.equal(true) expect(CurateOperationAppliedSchema.safeParse({...baseValid, keywords: ['k']}).success).to.equal(true) @@ -43,6 +47,15 @@ describe('CurateOperationAppliedSchema', () => { ).to.equal(true) }) + it('rejects payloads missing the required keywords / tags arrays', () => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const {keywords: _k, ...withoutKeywords} = baseValid + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const {tags: _t, ...withoutTags} = baseValid + expect(CurateOperationAppliedSchema.safeParse(withoutKeywords).success).to.equal(false) + expect(CurateOperationAppliedSchema.safeParse(withoutTags).success).to.equal(false) + }) + it('accepts tags / keywords / related with exactly 50 entries each', () => { const fifty = Array.from({length: 50}, (_, i) => `entry-${i}`) expect( @@ -75,8 +88,8 @@ describe('CurateOperationAppliedSchema', () => { expect(CurateOperationAppliedSchema.safeParse({...baseValid, confidence: 'maybe'}).success).to.equal(false) }) - it('rejects empty absolute_path / knowledge_path / task_id', () => { - expect(CurateOperationAppliedSchema.safeParse({...baseValid, absolute_path: ''}).success).to.equal(false) + it('rejects empty relative_path / knowledge_path / task_id', () => { + expect(CurateOperationAppliedSchema.safeParse({...baseValid, relative_path: ''}).success).to.equal(false) expect(CurateOperationAppliedSchema.safeParse({...baseValid, knowledge_path: ''}).success).to.equal(false) expect(CurateOperationAppliedSchema.safeParse({...baseValid, task_id: ''}).success).to.equal(false) }) diff --git a/test/unit/shared/analytics/events/query-completed.test.ts b/test/unit/shared/analytics/events/query-completed.test.ts index e80d8039f..197df52ef 100644 --- a/test/unit/shared/analytics/events/query-completed.test.ts +++ b/test/unit/shared/analytics/events/query-completed.test.ts @@ -16,6 +16,13 @@ const baseValid = { task_type: 'query' as const, } +const baseEntry = { + keywords: [], + related_paths: [], + relative_path: '.brv/notes/a.md', + tags: [], +} + describe('QueryCompletedSchema', () => { describe('valid payloads', () => { it('accepts the minimal required payload with empty read_paths_with_metadata', () => { @@ -48,25 +55,52 @@ describe('QueryCompletedSchema', () => { expect(QueryCompletedSchema.safeParse({...baseValid, cache_hit: true}).success).to.equal(true) }) - it('accepts read_paths_with_metadata entries with no metadata', () => { - const entries = [{absolute_path: '/a.md'}, {absolute_path: '/b.md'}] + it('accepts read_paths_with_metadata entries with empty metadata arrays', () => { + const entries = [ + {...baseEntry, relative_path: '.brv/a.md'}, + {...baseEntry, relative_path: '.brv/b.md'}, + ] expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(true) }) - it('accepts entries with full optional metadata', () => { - const entries = [{absolute_path: '/a.md', keywords: ['k1'], related: ['r1'], tags: ['t1']}] + it('accepts entries with populated keywords, tags, and structured related_paths', () => { + const entries = [ + { + keywords: ['k1'], + related_paths: [{keywords: [], relative_path: 'r1', tags: []}], + relative_path: '.brv/a.md', + tags: ['t1'], + }, + ] expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(true) }) it('accepts read_paths_with_metadata with exactly 10 entries', () => { - const entries = Array.from({length: 10}, (_, i) => ({absolute_path: `/file-${i}.md`})) + const entries = Array.from({length: 10}, (_, i) => ({...baseEntry, relative_path: `.brv/file-${i}.md`})) expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(true) }) - it('accepts entries with tags / keywords / related at the 50-entry cap and 256-char cap', () => { + it('accepts entries with keywords / tags at the 50-entry cap and 256-char strings', () => { const fifty = Array.from({length: 50}, (_, i) => `entry-${i}`) const at256 = 'x'.repeat(256) - const entries = [{absolute_path: '/a.md', keywords: fifty, related: [at256], tags: fifty}] + const entries = [ + { + keywords: fifty, + related_paths: [{keywords: [], relative_path: at256, tags: []}], + relative_path: '.brv/a.md', + tags: fifty, + }, + ] + expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(true) + }) + + it('accepts related_paths with up to 50 structured entries', () => { + const fifty = Array.from({length: 50}, (_, i) => ({ + keywords: [], + relative_path: `notes/related-${i}`, + tags: [], + })) + const entries = [{...baseEntry, related_paths: fifty}] expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(true) }) }) @@ -105,26 +139,42 @@ describe('QueryCompletedSchema', () => { }) it('rejects read_paths_with_metadata with more than 10 entries', () => { - const entries = Array.from({length: 11}, (_, i) => ({absolute_path: `/file-${i}.md`})) + const entries = Array.from({length: 11}, (_, i) => ({...baseEntry, relative_path: `.brv/file-${i}.md`})) expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(false) }) - it('rejects entries with empty absolute_path', () => { - const entries = [{absolute_path: ''}] + it('rejects entries with empty relative_path', () => { + const entries = [{...baseEntry, relative_path: ''}] expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(false) }) - it('rejects entries with more than 50 tags / keywords / related', () => { - const fiftyOne = Array.from({length: 51}, (_, i) => `entry-${i}`) - const tagsEntry = [{absolute_path: '/a.md', tags: fiftyOne}] - expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: tagsEntry}).success).to.equal( + it('rejects entries missing required keywords / tags arrays', () => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const {keywords: _k, ...withoutKeywords} = baseEntry + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const {tags: _t, ...withoutTags} = baseEntry + expect( + QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: [withoutKeywords]}).success, + ).to.equal(false) + expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: [withoutTags]}).success).to.equal( false, ) }) - it('rejects entries with tag / keyword / related string longer than 256 chars', () => { + it('rejects entries with more than 50 tags / keywords', () => { + const fiftyOne = Array.from({length: 51}, (_, i) => `entry-${i}`) + const tagsEntry = [{...baseEntry, tags: fiftyOne}] + expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: tagsEntry}).success).to.equal(false) + }) + + it('rejects entries with tag / keyword string longer than 256 chars', () => { const at257 = 'x'.repeat(257) - const entries = [{absolute_path: '/a.md', keywords: [at257]}] + const entries = [{...baseEntry, keywords: [at257]}] + expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(false) + }) + + it('rejects related_paths entries missing keywords / tags / relative_path', () => { + const entries = [{...baseEntry, related_paths: [{relative_path: 'r1'}]}] expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(false) }) @@ -133,7 +183,7 @@ describe('QueryCompletedSchema', () => { }) it('rejects unknown extra fields inside an entry (strict)', () => { - const entries = [{absolute_path: '/a.md', mystery: 'oops'}] + const entries = [{...baseEntry, mystery: 'oops'}] expect(QueryCompletedSchema.safeParse({...baseValid, read_paths_with_metadata: entries}).success).to.equal(false) }) }) From 6983e884f042d0c40da5de82a15457f0c2c0af41 Mon Sep 17 00:00:00 2001 From: Cuong Date: Wed, 27 May 2026 17:45:56 +0700 Subject: [PATCH 5/7] feat: [ENG-2964] M15.6 wire AnalyticsHook into lifecycleHooks + failure_kind on task_failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delivers M15.6's customer-facing piece: the actual wiring of AnalyticsHook into the daemon's lifecycleHooks[] (which M14.3 built emit logic for but never plumbed) plus the coarse failure_kind classifier on task_failed. Per the M14 / M15.6 alignment (Option A): M14.1-M14.3 ship as the foundation. M15.6 adds the wiring + failure_kind. M15.6's stated "scaffolded only" stance on task_created + per-flavor schemas is intentionally relaxed — M14 already produces those events and the review-tightening on curate_operation_applied / read_paths_with_metadata delivers PM-visible value that the M15.6 description deferred. Changes: src/server/infra/daemon/brv-server.ts: - import AnalyticsHook - construct it BEFORE TransportHandlers so it can land in lifecycleHooks[] alongside curate-log / query-log / task-history (now 4 peers, not 3) - the isAnalyticsEnabled gate from setupFeatureHandlers binds in via a closure ref that defers lookup until emit-time - capture setupFeatureHandlers's return value (was discarded), then setAnalyticsClient(analyticsClient) on the pre-registered hook src/shared/analytics/events/task-failed.ts: - new FailureKindValues + FailureKind type: 'cancelled' | 'timeout' | 'agent_error' | 'unknown' (coarse vocab, ≤64 chars, never raw error.message) - failure_kind added to TaskFailedSchema as a required field - docblock notes the M15.6 privacy contract src/server/infra/process/analytics-hook.ts: - new classifyFailureKind(errorMessage) helper. Substring sentinels for 'timeout' / 'timed out' / 'deadline exceeded' → 'timeout'; 'agent' / 'llm' / 'provider' / 'tool' → 'agent_error'; default 'unknown'. Raw error string NEVER ends up on the wire. - emitTaskFailed now takes a FailureKind argument - onTaskCancelled → emitTaskFailed(..., 'cancelled') - onTaskError → emitTaskFailed(..., classifyFailureKind(errorMessage)) TDD: - task-failed.test: 3 new tests pin the failure_kind shape (accepts the 4 canonical values, rejects out-of-vocab, rejects missing) - analytics-hook-m14.test: 3 new tests verify the classifier paths land on the wire — 'kaboom' → 'unknown', timeout strings → 'timeout', agent strings → 'agent_error' - new analytics-hook-lifecycle-wiring.test integration covers the end-to-end task:create → analyticsHook.onTaskCreate path through a real TaskRouter, plus all four task-type variants going through task_created → task_completed in sequence Full suite: 9145 passing, 0 failing. Lint clean. --- src/server/infra/daemon/brv-server.ts | 26 +- src/server/infra/process/analytics-hook.ts | 29 +- src/shared/analytics/events/task-failed.ts | 23 +- .../analytics-hook-lifecycle-wiring.test.ts | 254 ++++++++++++++++++ .../infra/process/analytics-hook-m14.test.ts | 34 ++- .../analytics/events/task-failed.test.ts | 21 +- test/unit/shared/analytics/task-types.test.ts | 1 + 7 files changed, 377 insertions(+), 11 deletions(-) create mode 100644 test/integration/infra/process/analytics-hook-lifecycle-wiring.test.ts diff --git a/src/server/infra/daemon/brv-server.ts b/src/server/infra/daemon/brv-server.ts index daab7fe3c..9a457bdd8 100644 --- a/src/server/infra/daemon/brv-server.ts +++ b/src/server/infra/daemon/brv-server.ts @@ -55,6 +55,7 @@ import {createBillingStateHandler} from '../billing/billing-state-endpoint.js' import {ClientManager} from '../client/client-manager.js' import {ProjectConfigStore} from '../config/file-config-store.js' import {readContextTreeRemoteUrl} from '../context-tree/read-context-tree-remote.js' +import {AnalyticsHook} from '../process/analytics-hook.js' import {broadcastToProjectRoom} from '../process/broadcast-utils.js' import {CurateLogHandler} from '../process/curate-log-handler.js' import {setupFeatureHandlers} from '../process/feature-handlers.js' @@ -367,6 +368,18 @@ async function main(): Promise { // same instances this hook writes to. const taskHistoryHook = new TaskHistoryHook({getStore: getTaskHistoryStore}) + // M15.6: AnalyticsHook is the 4th lifecycle peer alongside curate-log / + // query-log / task-history. It emits task_created / task_completed / + // task_failed (and M12 per-flavor events for curate / query) into the + // daemon's IAnalyticsClient. The client + isAnalyticsEnabled gate come + // from setupFeatureHandlers later in this function; the closure below + // defers the lookup so the hook can be constructed in time to land in + // lifecycleHooks[] but still observe the live config. + let isAnalyticsEnabledRef: () => boolean = () => true + const analyticsHook = new AnalyticsHook({ + isEnabled: () => isAnalyticsEnabledRef(), + }) + // Provider config/keychain stores — shared between feature handlers and state endpoint. // Hoisted ahead of `new TransportHandlers` so the resolveActiveProvider callback below // can close over them and call resolveProviderConfig synchronously at task-create time. @@ -427,7 +440,7 @@ async function main(): Promise { const config = await new ProjectConfigStore().read(projectPath) return config?.reviewDisabled === true }, - lifecycleHooks: [curateLogHandler, queryLogHandler, taskHistoryHook], + lifecycleHooks: [curateLogHandler, queryLogHandler, taskHistoryHook, analyticsHook], projectRegistry, projectRouter, // Stamp the active provider/model snapshot onto every created task so the @@ -642,7 +655,7 @@ async function main(): Promise { // Feature handlers (auth, init, status, push, pull, etc.) require async OIDC discovery. // Placed after daemon:getState so the debug endpoint is available immediately, // without waiting for OIDC discovery (~400ms). - await setupFeatureHandlers({ + const featureHandlers = await setupFeatureHandlers({ authStateStore, billingConfigStoreFactory, broadcastToProject(projectPath, event, data) { @@ -660,6 +673,15 @@ async function main(): Promise { webuiPort: webuiServer?.getPort(), }) + // M15.6: now that setupFeatureHandlers has constructed the real + // IAnalyticsClient + isAnalyticsEnabled callback, late-bind them into + // the AnalyticsHook that was pre-registered in lifecycleHooks[]. Any + // task_* emits queued during the boot window between hook construction + // and this line silently no-op (matches `setAnalyticsClient`'s docblock + // contract — no tasks are active during daemon boot). + isAnalyticsEnabledRef = featureHandlers.isAnalyticsEnabled + analyticsHook.setAnalyticsClient(featureHandlers.analyticsClient) + // Load auth token AFTER feature handlers are registered. // AuthHandler's onAuthChanged/onAuthExpired callbacks must be wired first // so that loadToken() triggers proper broadcasts to TUI and agents. diff --git a/src/server/infra/process/analytics-hook.ts b/src/server/infra/process/analytics-hook.ts index 551cf8340..827bac559 100644 --- a/src/server/infra/process/analytics-hook.ts +++ b/src/server/infra/process/analytics-hook.ts @@ -6,6 +6,7 @@ import type {AnalyticsEventName} from '../../../shared/analytics/event-names.js' import type {CurateRunCompletedProps} from '../../../shared/analytics/events/curate-run-completed.js' import type {PropsArg} from '../../../shared/analytics/events/index.js' import type {QueryCompletedProps} from '../../../shared/analytics/events/query-completed.js' +import type {FailureKind} from '../../../shared/analytics/events/task-failed.js' import type {TaskType} from '../../../shared/analytics/task-types.js' import type {LlmToolResultEvent} from '../../core/domain/transport/schemas.js' import type {TaskInfo} from '../../core/domain/transport/task-info.js' @@ -49,6 +50,20 @@ function toRelativePath(filePath: string, projectPath?: string): string { return rel === '' ? '.' : rel } +/** + * Classify a daemon-side error message into a coarse failure_kind tag. + * + * Substring matching is intentional and over-conservative: only well-known + * sentinels promote out of `'unknown'`. The raw message NEVER ends up on + * the analytics wire — only the canonical tag. + */ +function classifyFailureKind(errorMessage: string): FailureKind { + const m = errorMessage.toLowerCase() + if (m.includes('timeout') || m.includes('timed out') || m.includes('deadline exceeded')) return 'timeout' + if (m.includes('agent') || m.includes('llm') || m.includes('provider') || m.includes('tool')) return 'agent_error' + return 'unknown' +} + // `CURATE_TASK_TYPES` is exported as a readonly tuple; wrap in a Set // for cast-free `.has()` lookups against TaskInfo.type (string). const CURATE_TASK_TYPE_SET: ReadonlySet = new Set(CURATE_TASK_TYPES) @@ -175,7 +190,7 @@ export class AnalyticsHook implements ITaskLifecycleHook { async onTaskCancelled(taskId: string, task: TaskInfo): Promise { await this.dispatchTerminal(taskId, task, 'cancelled') - this.emitTaskFailed(taskId, task) + this.emitTaskFailed(taskId, task, 'cancelled') } async onTaskCompleted(taskId: string, _result: string, task: TaskInfo): Promise { @@ -236,9 +251,9 @@ export class AnalyticsHook implements ITaskLifecycleHook { } } - async onTaskError(taskId: string, _errorMessage: string, task: TaskInfo): Promise { + async onTaskError(taskId: string, errorMessage: string, task: TaskInfo): Promise { await this.dispatchTerminal(taskId, task, 'error') - this.emitTaskFailed(taskId, task) + this.emitTaskFailed(taskId, task, classifyFailureKind(errorMessage)) } async onToolResult(taskId: string, payload: LlmToolResultEvent): Promise { @@ -438,10 +453,16 @@ export class AnalyticsHook implements ITaskLifecycleHook { * onTaskCancelled AFTER dispatchTerminal so M12 per-flavor failure * emits land first on the wire. Cancellation maps to task_failed * (not a distinct event) per the schema's docblock. + * + * M15.6: failure_kind is a coarse classifier passed by the caller — + * 'cancelled' from onTaskCancelled, classified-from-errorMessage from + * onTaskError (see classifyFailureKind). Raw error.message MUST NOT + * leak into the emit; only the canonical FailureKind tag does. */ - private emitTaskFailed(taskId: string, task: TaskInfo): void { + private emitTaskFailed(taskId: string, task: TaskInfo, failureKind: FailureKind): void { this.emit(AnalyticsEventNames.TASK_FAILED, { duration_ms: this.durationMs(task), + failure_kind: failureKind, task_id: taskId, task_type: toAnalyticsTaskType(task.type), }) diff --git a/src/shared/analytics/events/task-failed.ts b/src/shared/analytics/events/task-failed.ts index a48b823ca..d4cf0cd3f 100644 --- a/src/shared/analytics/events/task-failed.ts +++ b/src/shared/analytics/events/task-failed.ts @@ -3,6 +3,22 @@ import {z} from 'zod' import {TASK_TYPE_VALUES} from '../task-types.js' +/** + * Coarse-vocabulary classification of why a task ended in a non-success + * state. Strictly enumerated so consumers can group failures without + * having to parse raw error messages. Every value is ≤64 chars and + * carries no PII. + * + * - `cancelled` — onTaskCancelled lifecycle path; user-initiated abort. + * - `timeout` — error message indicates the agent / LLM exceeded a budget. + * - `agent_error` — error message indicates a recognised agent-side fault + * (provider rejection, tool failure, schema reject, etc.). + * - `unknown` — anything else; the hook MUST default here rather than + * widening the enum on a hunch. + */ +export const FailureKindValues = ['cancelled', 'timeout', 'agent_error', 'unknown'] as const +export type FailureKind = (typeof FailureKindValues)[number] + /** * Per-event schema for `task_failed`. * @@ -10,13 +26,14 @@ import {TASK_TYPE_VALUES} from '../task-types.js' * captured here: they may contain file paths, secrets, or user content. * Strict mode rejects any attempt to add `error_message` / `stack` later. * - * Adding `error_class` / `error_code` would require extending - * `ITaskLifecycleHook.onTaskError` to deliver the structured error object, - * which is a separate ticket. + * `failure_kind` (M15.6) is a coarse-vocabulary tag the daemon classifies + * the error into. Producers MUST emit one of the canonical values; the + * hook never forwards raw `error.message` text under any field name. */ export const TaskFailedSchema = z .object({ duration_ms: z.number().int().nonnegative(), + failure_kind: z.enum(FailureKindValues), task_id: z.string().min(1), task_type: z.enum(TASK_TYPE_VALUES), }) diff --git a/test/integration/infra/process/analytics-hook-lifecycle-wiring.test.ts b/test/integration/infra/process/analytics-hook-lifecycle-wiring.test.ts new file mode 100644 index 000000000..e1feed7bd --- /dev/null +++ b/test/integration/infra/process/analytics-hook-lifecycle-wiring.test.ts @@ -0,0 +1,254 @@ +/** + * M15.6 end-to-end wiring test — drives a real TaskRouter with AnalyticsHook + * registered as a lifecycle hook (mirroring the brv-server.ts:430 wire) and + * asserts that the generic task_created / task_completed / task_failed + * events flow through correctly. Plus failure_kind classification. + * + * Mirrors the async-stress harness but focuses on lifecycle wiring rather + * than per-op order. Stubs the transport + agent pool; the rest is real + * AnalyticsHook + TaskRouter glue. + */ + + +import {expect} from 'chai' +import {createSandbox, type SinonSandbox, type SinonStub} from 'sinon' + +import type {TaskInfo} from '../../../../src/server/core/domain/transport/task-info.js' +import type {IAgentPool, SubmitTaskResult} from '../../../../src/server/core/interfaces/agent/i-agent-pool.js' +import type {IAnalyticsClient} from '../../../../src/server/core/interfaces/analytics/i-analytics-client.js' +import type {IProjectRegistry} from '../../../../src/server/core/interfaces/project/i-project-registry.js' +import type {IProjectRouter} from '../../../../src/server/core/interfaces/routing/i-project-router.js' +import type { + ITransportServer, + RequestHandler, +} from '../../../../src/server/core/interfaces/transport/i-transport-server.js' + +import {TransportTaskEventNames} from '../../../../src/server/core/domain/transport/schemas.js' +import {AnalyticsHook} from '../../../../src/server/infra/process/analytics-hook.js' +import {TaskRouter} from '../../../../src/server/infra/process/task-router.js' +import {AnalyticsEventNames} from '../../../../src/shared/analytics/event-names.js' + +function makeStubTransport(sandbox: SinonSandbox): { + requestHandlers: Map + transport: ITransportServer +} { + const requestHandlers = new Map() + const transport: ITransportServer = { + addToRoom: sandbox.stub(), + broadcast: sandbox.stub(), + broadcastTo: sandbox.stub(), + getPort: sandbox.stub().returns(3000), + isRunning: sandbox.stub().returns(true), + onConnection: sandbox.stub(), + onDisconnection: sandbox.stub(), + onRequest: sandbox.stub().callsFake((event: string, handler: RequestHandler) => { + requestHandlers.set(event, handler) + }), + removeFromRoom: sandbox.stub(), + sendTo: sandbox.stub(), + start: sandbox.stub().resolves(), + stop: sandbox.stub().resolves(), + } + return {requestHandlers, transport} +} + +function makeStubAgentPool(sandbox: SinonSandbox): IAgentPool { + return { + getEntries: sandbox.stub().returns([]), + getSize: sandbox.stub().returns(0), + handleAgentDisconnected: sandbox.stub(), + hasAgent: sandbox.stub().returns(false), + markIdle: sandbox.stub(), + notifyTaskCompleted: sandbox.stub(), + shutdown: sandbox.stub().resolves(), + submitTask: sandbox.stub().resolves({success: true} as SubmitTaskResult), + } +} + +function makeStubProjectRegistry(sandbox: SinonSandbox): IProjectRegistry { + const projectInfo = { + projectPath: '/proj', + registeredAt: Date.now(), + sanitizedPath: '_proj', + storagePath: '/data/proj', + } + return { + get: sandbox.stub().returns(projectInfo), + getAll: sandbox.stub().returns(new Map()), + register: sandbox.stub().returns(projectInfo), + unregister: sandbox.stub().returns(true), + } +} + +function makeStubProjectRouter(sandbox: SinonSandbox): IProjectRouter { + return { + addToProjectRoom: sandbox.stub(), + broadcastToProject: sandbox.stub(), + getProjectMembers: sandbox.stub().returns([]), + removeFromProjectRoom: sandbox.stub(), + } +} + +function makeAnalyticsClient(sandbox: SinonSandbox): {client: IAnalyticsClient; trackStub: SinonStub} { + const trackStub = sandbox.stub() + const client: IAnalyticsClient = { + abort: sandbox.stub(), + flush: sandbox.stub().resolves(), + getRuntimeState: sandbox.stub().resolves({droppedCount: 0, lastSuccessfulFlushAt: undefined, queueDepth: 0}), + onAuthTransition: sandbox.stub().resolves(), + track: trackStub, + } + return {client, trackStub} +} + +const buildTaskInfo = (taskId: string, type: string): TaskInfo => + ({ + clientId: 'client-1', + completedAt: Date.now(), + content: 'demo', + createdAt: Date.now() - 1000, + projectPath: '/proj', + status: 'completed', + taskId, + type, + }) as unknown as TaskInfo + +describe('AnalyticsHook lifecycle wiring (M15.6 — through TaskRouter)', () => { + let sandbox: SinonSandbox + let trackStub: SinonStub + let analyticsHook: AnalyticsHook + let createHandler: RequestHandler + + beforeEach(() => { + sandbox = createSandbox() + const {requestHandlers, transport} = makeStubTransport(sandbox) + const bundle = makeAnalyticsClient(sandbox) + trackStub = bundle.trackStub + + analyticsHook = new AnalyticsHook() + analyticsHook.setAnalyticsClient(bundle.client) + + // The wire from brv-server.ts:430: AnalyticsHook is the 4th peer hook. + // The other three are intentionally omitted here so the test focuses on + // AnalyticsHook's emit surface in isolation. + const router = new TaskRouter({ + agentPool: makeStubAgentPool(sandbox), + getAgentForProject: () => 'agent-1', + lifecycleHooks: [analyticsHook], + projectRegistry: makeStubProjectRegistry(sandbox), + projectRouter: makeStubProjectRouter(sandbox), + resolveClientProjectPath: () => '/proj', + transport, + }) + router.setup() + + const create = requestHandlers.get(TransportTaskEventNames.CREATE) + if (!create) throw new Error('expected task:create handler to be registered') + createHandler = create + }) + + afterEach(() => { + sandbox.restore() + }) + + it('task_created fires immediately on task:create with the correct task_type', async () => { + await createHandler( + {content: 'curate me', projectPath: '/proj', taskId: 'task-create-fire', type: 'curate'}, + 'client-1', + ) + + const created = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_CREATED) + expect(created, 'task_created should fire on create').to.not.equal(undefined) + const props = created?.args[1] as Record + expect(props.task_id).to.equal('task-create-fire') + expect(props.task_type).to.equal('curate') + expect(props.has_files).to.equal(false) + expect(props.has_folder).to.equal(false) + }) + + it('task_completed fires after the agent reports completion (curate task)', async () => { + const taskId = 'task-curate-success' + await createHandler({content: 'curate', projectPath: '/proj', taskId, type: 'curate'}, 'client-1') + await analyticsHook.onTaskCompleted(taskId, '', buildTaskInfo(taskId, 'curate')) + + const completed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_COMPLETED) + expect(completed).to.not.equal(undefined) + const props = completed?.args[1] as Record + expect(props.task_id).to.equal(taskId) + expect(props.task_type).to.equal('curate') + expect(props.duration_ms).to.be.a('number') + }) + + it('task_failed carries failure_kind="cancelled" on user cancellation', async () => { + const taskId = 'task-cancel' + await createHandler({content: 'curate', projectPath: '/proj', taskId, type: 'curate'}, 'client-1') + await analyticsHook.onTaskCancelled(taskId, buildTaskInfo(taskId, 'curate')) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect(failed).to.not.equal(undefined) + const props = failed?.args[1] as Record + expect(props.task_id).to.equal(taskId) + expect(props.task_type).to.equal('curate') + expect(props.failure_kind).to.equal('cancelled') + }) + + it('task_failed classifies a timeout error message into failure_kind="timeout"', async () => { + const taskId = 'task-timeout' + await createHandler({content: 'curate', projectPath: '/proj', taskId, type: 'curate'}, 'client-1') + await analyticsHook.onTaskError(taskId, 'agentic loop deadline exceeded', buildTaskInfo(taskId, 'curate')) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('timeout') + }) + + it('task_failed classifies an agent error message into failure_kind="agent_error"', async () => { + const taskId = 'task-agent-err' + await createHandler({content: 'curate', projectPath: '/proj', taskId, type: 'curate'}, 'client-1') + await analyticsHook.onTaskError(taskId, 'llm provider rejected the request', buildTaskInfo(taskId, 'curate')) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('agent_error') + }) + + it('task_failed defaults failure_kind="unknown" when nothing recognises the error string', async () => { + const taskId = 'task-unknown' + await createHandler({content: 'curate', projectPath: '/proj', taskId, type: 'curate'}, 'client-1') + await analyticsHook.onTaskError(taskId, 'kaboom', buildTaskInfo(taskId, 'curate')) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('unknown') + }) + + it('every tool-mode task type fires both task_created and the right terminal', async () => { + const cases = [ + {expectedTaskType: 'curate-tool-mode', taskId: 'tm-curate', type: 'curate-html-direct'}, + {expectedTaskType: 'query-tool-mode', taskId: 'tm-query', type: 'query-tool-mode'}, + {expectedTaskType: 'dream-scan', taskId: 'tm-dream-scan', type: 'dream-scan'}, + {expectedTaskType: 'dream-finalize', taskId: 'tm-dream-finalize', type: 'dream-finalize'}, + ] as const + + for (const c of cases) { + // eslint-disable-next-line no-await-in-loop + await createHandler({content: 'demo', projectPath: '/proj', taskId: c.taskId, type: c.type}, 'client-1') + // eslint-disable-next-line no-await-in-loop + await analyticsHook.onTaskCompleted(c.taskId, '', buildTaskInfo(c.taskId, c.type)) + } + + for (const c of cases) { + const created = trackStub.getCalls().find( + (call) => + call.args[0] === AnalyticsEventNames.TASK_CREATED && + (call.args[1] as Record).task_id === c.taskId, + ) + const completed = trackStub.getCalls().find( + (call) => + call.args[0] === AnalyticsEventNames.TASK_COMPLETED && + (call.args[1] as Record).task_id === c.taskId, + ) + expect(created, `${c.taskId}: task_created`).to.not.equal(undefined) + expect(completed, `${c.taskId}: task_completed`).to.not.equal(undefined) + expect((created?.args[1] as Record).task_type).to.equal(c.expectedTaskType) + expect((completed?.args[1] as Record).task_type).to.equal(c.expectedTaskType) + } + }) +}) diff --git a/test/unit/server/infra/process/analytics-hook-m14.test.ts b/test/unit/server/infra/process/analytics-hook-m14.test.ts index fdff97a29..3ebe2c7eb 100644 --- a/test/unit/server/infra/process/analytics-hook-m14.test.ts +++ b/test/unit/server/infra/process/analytics-hook-m14.test.ts @@ -199,7 +199,7 @@ describe('AnalyticsHook M14.3 generic task_* emit simulation', () => { ]) }) - it('task_failed payload carries duration_ms + task_id + canonical task_type', async () => { + it('task_failed payload carries duration_ms + task_id + canonical task_type + failure_kind', async () => { const task = buildTask('query', {taskId: 'task-query-err'}) await hook.onTaskCreate(task) trackStub.resetHistory() @@ -210,6 +210,38 @@ describe('AnalyticsHook M14.3 generic task_* emit simulation', () => { expect(props.task_id).to.equal('task-query-err') expect(props.task_type).to.equal('query') expect(props.duration_ms).to.equal(1234) + // 'kaboom' classifies to 'unknown' — no recognised sentinel substring + expect(props.failure_kind).to.equal('unknown') + }) + + it('failure_kind is "cancelled" on onTaskCancelled regardless of state', async () => { + const task = buildTask('curate', {taskId: 'task-cancel-fk'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskCancelled(task.taskId, task) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('cancelled') + }) + + it('failure_kind is "timeout" when the error message names a timeout', async () => { + const task = buildTask('search', {taskId: 'task-timeout'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskError(task.taskId, 'request timed out after 30s', task) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('timeout') + }) + + it('failure_kind is "agent_error" when the error message points at the agent layer', async () => { + const task = buildTask('search', {taskId: 'task-agent-err'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskError(task.taskId, 'provider rejected the LLM call', task) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('agent_error') }) }) }) diff --git a/test/unit/shared/analytics/events/task-failed.test.ts b/test/unit/shared/analytics/events/task-failed.test.ts index 0cf8e65bd..930bc8d69 100644 --- a/test/unit/shared/analytics/events/task-failed.test.ts +++ b/test/unit/shared/analytics/events/task-failed.test.ts @@ -1,10 +1,11 @@ /* eslint-disable camelcase */ import {expect} from 'chai' -import {TaskFailedSchema} from '../../../../../src/shared/analytics/events/task-failed.js' +import {FailureKindValues, TaskFailedSchema} from '../../../../../src/shared/analytics/events/task-failed.js' const baseValid = { duration_ms: 9000, + failure_kind: 'unknown' as const, task_id: '550e8400-e29b-41d4-a716-446655440000', task_type: 'curate' as const, } @@ -43,4 +44,22 @@ describe('TaskFailedSchema', () => { const {task_id: _, ...withoutTaskId} = baseValid expect(TaskFailedSchema.safeParse(withoutTaskId).success).to.equal(false) }) + + describe('failure_kind (M15.6)', () => { + it('accepts every canonical FailureKindValues entry', () => { + for (const kind of FailureKindValues) { + expect(TaskFailedSchema.safeParse({...baseValid, failure_kind: kind}).success).to.equal(true) + } + }) + + it('rejects an out-of-vocabulary failure_kind', () => { + expect(TaskFailedSchema.safeParse({...baseValid, failure_kind: 'oom'}).success).to.equal(false) + }) + + it('rejects missing failure_kind', () => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const {failure_kind: _, ...withoutKind} = baseValid + expect(TaskFailedSchema.safeParse(withoutKind).success).to.equal(false) + }) + }) }) diff --git a/test/unit/shared/analytics/task-types.test.ts b/test/unit/shared/analytics/task-types.test.ts index e0d5999ef..2e39886d5 100644 --- a/test/unit/shared/analytics/task-types.test.ts +++ b/test/unit/shared/analytics/task-types.test.ts @@ -85,6 +85,7 @@ describe('TaskTypes', () => { it(`TaskFailedSchema accepts task_type='${taskType}'`, () => { const parsed = TaskFailedSchema.parse({ duration_ms: 100, + failure_kind: 'unknown' as const, task_id: 't-1', task_type: taskType, }) From 190f4b4b9ac9e6dac6818cc31f3755b4822c926f Mon Sep 17 00:00:00 2001 From: Cuong Date: Wed, 27 May 2026 21:08:45 +0700 Subject: [PATCH 6/7] refactor: [ENG-2964] M15.6 address PR #722 review comments (7 of 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1 classifyFailureKind — word-boundary regex + pinned precedence: Substring matching let 'tooltip' / 'engagement' / 'urgent' bucket into agent_error. Tightened to /\b(agent|llm|provider|tool)\b/ and /\b(timeout|timed out|deadline exceeded)\b/. Docblock pins precedence (timeout > agent_error > unknown) so future if-order shuffles can't silently rebucket the funnel. #2 toRelativePath outside-project guard (privacy bug): path.relative('/proj','/Users/dev/other/x.md') returned '../../Users/dev/other/x.md' — still encoded the host layout. Same hole when projectPath was undefined (fell back to raw absolute path). Now returns '/' in both cases; preserves enough signal for backend grouping without becoming PII. Same guard also runs against absolute-path tails (Windows drive-letter switches). #3 toAnalyticsTaskType drift guard: Replaced `daemonType as TaskType` (per CLAUDE.md anti-cast rule) with a TASK_TYPE_SET membership check + processLog warning + 'unknown' sentinel fallback. A future un-enumerated dispatch now lands a debuggable warning at the daemon instead of disappearing at the backend Zod check. #4 dumpEvents test pollution: Gated behind DUMP_ANALYTICS=1. `npm test` runs the shape assertions silently; opt-in dump still works via `DUMP_ANALYTICS=1 npx mocha test/unit/.../analytics-hook-toolmode-inspection.test.ts`. #5 FU-1 forward-compat comment: Added a block-comment note above the tool-mode "operations_*: 0" assertions so when FU-1 lands and the counters flip non-zero, the failure reads as "feature, update the test" rather than a regression. #6 curate_operation_applied.related asymmetry: Added a TODO(M15.x) marker on the `related` field flagging the wire-shape asymmetry with `query_completed.read_paths_with_metadata .related_paths` (structured). Restructure is consumer-migration territory — own ticket. #7 brv-server.ts loud-fail assertion: Added an explicit throw if setupFeatureHandlers returns without analyticsClient. Future refactors that drop the field will explode here instead of silently no-op'ing every emit forever. Tests added to lock in #1, #2, #3 behavior so the review intent doesn't drift. Existing stress-test fixtures moved from `/A/op-N.md` under-the-root paths to `/proj/A/op-N.md` so they exercise the in-project case (out-of-project is now its own focused test). Full suite: 9227 passing, 0 failing. Lint clean. --- src/server/infra/daemon/brv-server.ts | 7 ++ src/server/infra/process/analytics-hook.ts | 72 +++++++++++++++---- .../events/curate-operation-applied.ts | 4 ++ .../analytics-hook-async-stress.test.ts | 14 ++-- .../infra/process/analytics-hook-m14.test.ts | 70 ++++++++++++++++++ ...analytics-hook-toolmode-inspection.test.ts | 14 ++++ .../infra/process/analytics-hook.test.ts | 19 +++-- 7 files changed, 171 insertions(+), 29 deletions(-) diff --git a/src/server/infra/daemon/brv-server.ts b/src/server/infra/daemon/brv-server.ts index 9a457bdd8..d70918111 100644 --- a/src/server/infra/daemon/brv-server.ts +++ b/src/server/infra/daemon/brv-server.ts @@ -680,6 +680,13 @@ async function main(): Promise { // and this line silently no-op (matches `setAnalyticsClient`'s docblock // contract — no tasks are active during daemon boot). isAnalyticsEnabledRef = featureHandlers.isAnalyticsEnabled + // PR #722 review: explode loudly if a future refactor drops + // analyticsClient from the result shape — silently no-op'ing every + // emit forever is the worst failure mode for telemetry plumbing. + if (!featureHandlers.analyticsClient) { + throw new Error('setupFeatureHandlers returned without analyticsClient — AnalyticsHook cannot bind') + } + analyticsHook.setAnalyticsClient(featureHandlers.analyticsClient) // Load auth token AFTER feature handlers are registered. diff --git a/src/server/infra/process/analytics-hook.ts b/src/server/infra/process/analytics-hook.ts index 827bac559..665f65979 100644 --- a/src/server/infra/process/analytics-hook.ts +++ b/src/server/infra/process/analytics-hook.ts @@ -1,6 +1,6 @@ /* eslint-disable camelcase */ import {readFile as readFileAsync} from 'node:fs/promises' -import {relative as relativePath} from 'node:path' +import {basename, isAbsolute as isAbsolutePath, relative as relativePath} from 'node:path' import type {AnalyticsEventName} from '../../../shared/analytics/event-names.js' import type {CurateRunCompletedProps} from '../../../shared/analytics/events/curate-run-completed.js' @@ -22,45 +22,87 @@ import {processLog} from '../../utils/process-logger.js' import {CURATE_TASK_TYPES} from './curate-log-handler.js' import {QUERY_TASK_TYPES} from './query-log-handler.js' +/** + * Backstop sentinel emitted when the daemon dispatches a task type the + * analytics enum doesn't recognise. Keeps the event on the wire instead of + * silently failing the Zod check at the backend. Update TASK_TYPE_VALUES + * (M14.1) when a new daemon type lands; this is the drift alarm. + */ +const UNKNOWN_TASK_TYPE: TaskType = 'unknown' as TaskType + +const ANALYTICS_TASK_TYPE_SET: ReadonlySet = new Set(Object.values(TaskTypes)) + /** * Translate the daemon's runtime task type string to the canonical * analytics wire value. The daemon still dispatches the pre-ENG-2925 * name `'curate-html-direct'`; analytics emits the post-rename - * `'curate-tool-mode'`. Once the rename PR lands, this becomes a - * no-op identity and can be inlined. + * `'curate-tool-mode'`. Once the rename PR lands, the alias becomes + * dead code and can be inlined. + * + * Guards an unknown daemon type with a process log + 'unknown' sentinel + * fallback (PR #722 review): swallowing the value silently would let an + * un-enumerated dispatch slip past the wire-side Zod check and disappear + * at the backend; logging here keeps the drift debuggable at the daemon. */ function toAnalyticsTaskType(daemonType: string): TaskType { if (daemonType === 'curate-html-direct') return TaskTypes.CURATE_TOOL_MODE - return daemonType as TaskType + if (ANALYTICS_TASK_TYPE_SET.has(daemonType)) return daemonType as TaskType + processLog(`AnalyticsHook: unknown task type '${daemonType}' — falling back to '${UNKNOWN_TASK_TYPE}'`) + return UNKNOWN_TASK_TYPE } +/** + * Stable sentinel for paths that can't be safely emitted as project- + * relative — either outside the project root or the project root itself + * is unknown. The backend can group these without leaking host layout. + */ +const OUTSIDE_PROJECT_PATH = '' + /** * Convert an absolute filesystem path to a project-relative path for the - * analytics wire. Falls back to the input unchanged when projectPath is - * unset (e.g., search tasks scoped to the daemon root). Keeps emits free - * of `/Users/{name}` PII while still letting PMs reason about which file - * inside a project an operation touched. + * analytics wire. Keeps emits free of `/Users/{name}` PII while still + * letting PMs reason about which file inside a project an operation touched. + * + * PR #722 review: `path.relative('/proj', '/Users/dev/other/x.md')` yields + * `'../../Users/dev/other/x.md'` — still encodes the host layout. When the + * relative path escapes the project root (or projectPath is unset), surface + * a stable sentinel + basename rather than the raw absolute path. The + * sentinel preserves enough signal for backend grouping without becoming + * PII. */ function toRelativePath(filePath: string, projectPath?: string): string { - if (!projectPath) return filePath + if (!projectPath) return `${OUTSIDE_PROJECT_PATH}/${basename(filePath)}` const rel = relativePath(projectPath, filePath) // `path.relative` returns '' when paths are identical — defensively // surface a leaf token rather than emit a zero-length wire string that // would fail `z.string().min(1)`. - return rel === '' ? '.' : rel + if (rel === '') return '.' + // Anything that escapes the project root (`../foo`) or stays absolute + // (Windows drive letter switches) is treated as outside-project. + if (rel.startsWith('..') || isAbsolutePath(rel)) { + return `${OUTSIDE_PROJECT_PATH}/${basename(filePath)}` + } + + return rel } /** * Classify a daemon-side error message into a coarse failure_kind tag. * - * Substring matching is intentional and over-conservative: only well-known - * sentinels promote out of `'unknown'`. The raw message NEVER ends up on - * the analytics wire — only the canonical tag. + * Precedence (PR #722 review — pinned so the if-order can't silently rebucket + * the funnel later): `timeout` > `agent_error` > `unknown`. A message + * containing both `'timeout'` and `'agent'` classifies as `'timeout'`. + * + * Word-boundary matching keeps unrelated tokens (`'tooltip'`, `'engagement'`, + * `'urgent'`) from bumping into the `agent_error` bucket. The raw message + * NEVER ends up on the analytics wire — only the canonical tag. */ +const TIMEOUT_PATTERN = /\b(timeout|timed out|deadline exceeded)\b/ +const AGENT_ERROR_PATTERN = /\b(agent|llm|provider|tool)\b/ function classifyFailureKind(errorMessage: string): FailureKind { const m = errorMessage.toLowerCase() - if (m.includes('timeout') || m.includes('timed out') || m.includes('deadline exceeded')) return 'timeout' - if (m.includes('agent') || m.includes('llm') || m.includes('provider') || m.includes('tool')) return 'agent_error' + if (TIMEOUT_PATTERN.test(m)) return 'timeout' + if (AGENT_ERROR_PATTERN.test(m)) return 'agent_error' return 'unknown' } diff --git a/src/shared/analytics/events/curate-operation-applied.ts b/src/shared/analytics/events/curate-operation-applied.ts index c7e9839b0..8ed21b180 100644 --- a/src/shared/analytics/events/curate-operation-applied.ts +++ b/src/shared/analytics/events/curate-operation-applied.ts @@ -24,6 +24,10 @@ export const CurateOperationAppliedSchema = z knowledge_path: z.string().min(1), needs_review: z.boolean(), operation_type: z.enum(['ADD', 'UPDATE', 'DELETE', 'MERGE', 'UPSERT']), + // TODO(M15.x): harmonise with the sibling `query_completed.read_paths_ + // _with_metadata[].related_paths` structured shape — current asymmetry + // forces consumers to special-case parsing `related` between the two + // events. Restructuring is its own ticket (consumer migration concern). related: z.array(z.string().max(256)).max(50).optional(), relative_path: z.string().min(1), tags: z.array(z.string().max(256)).max(50), diff --git a/test/integration/infra/process/analytics-hook-async-stress.test.ts b/test/integration/infra/process/analytics-hook-async-stress.test.ts index 59951c20b..313018b1a 100644 --- a/test/integration/infra/process/analytics-hook-async-stress.test.ts +++ b/test/integration/infra/process/analytics-hook-async-stress.test.ts @@ -249,7 +249,7 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { await createCurateTask(taskId) const opSpecs = Array.from({length: 20}, (_, i) => ({ - filePath: `/A/op-${String(i).padStart(2, '0')}.md`, + filePath: `/proj/A/op-${String(i).padStart(2, '0')}.md`, path: `notes/A/op-${i}`, })) @@ -275,11 +275,11 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { await createCurateTask('task-Y') const xSpecs = Array.from({length: 15}, (_, i) => ({ - filePath: `/X/op-${String(i).padStart(2, '0')}.md`, + filePath: `/proj/X/op-${String(i).padStart(2, '0')}.md`, path: `notes/X/op-${i}`, })) const ySpecs = Array.from({length: 15}, (_, i) => ({ - filePath: `/Y/op-${String(i).padStart(2, '0')}.md`, + filePath: `/proj/Y/op-${String(i).padStart(2, '0')}.md`, path: `notes/Y/op-${i}`, })) @@ -309,7 +309,7 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { await createCurateTask(taskId) const specs = Array.from({length: 50}, (_, i) => ({ - filePath: `/Z/op-${String(i).padStart(2, '0')}.md`, + filePath: `/proj/Z/op-${String(i).padStart(2, '0')}.md`, path: `notes/Z/op-${i}`, })) @@ -355,15 +355,15 @@ describe('AnalyticsHook async stress (integration through TaskRouter)', () => { const specsByTask: Record> = { 'task-P': Array.from({length: 10}, (_, i) => ({ - filePath: `/P/op-${String(i).padStart(2, '0')}.md`, + filePath: `/proj/P/op-${String(i).padStart(2, '0')}.md`, path: `notes/P/op-${i}`, })), 'task-Q': Array.from({length: 10}, (_, i) => ({ - filePath: `/Q/op-${String(i).padStart(2, '0')}.md`, + filePath: `/proj/Q/op-${String(i).padStart(2, '0')}.md`, path: `notes/Q/op-${i}`, })), 'task-R': Array.from({length: 10}, (_, i) => ({ - filePath: `/R/op-${String(i).padStart(2, '0')}.md`, + filePath: `/proj/R/op-${String(i).padStart(2, '0')}.md`, path: `notes/R/op-${i}`, })), } diff --git a/test/unit/server/infra/process/analytics-hook-m14.test.ts b/test/unit/server/infra/process/analytics-hook-m14.test.ts index 3ebe2c7eb..85b782293 100644 --- a/test/unit/server/infra/process/analytics-hook-m14.test.ts +++ b/test/unit/server/infra/process/analytics-hook-m14.test.ts @@ -243,5 +243,75 @@ describe('AnalyticsHook M14.3 generic task_* emit simulation', () => { const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) expect((failed?.args[1] as Record).failure_kind).to.equal('agent_error') }) + + it('classifier uses word-boundary matching: "tooltip" / "engagement" do NOT bucket into agent_error (PR #722)', async () => { + const task = buildTask('search', {taskId: 'task-tooltip'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskError(task.taskId, 'could not render tooltip in engagement panel', task) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('unknown') + }) + + it('classifier precedence pinned: timeout wins over agent_error when both substrings present (PR #722)', async () => { + const task = buildTask('search', {taskId: 'task-both'}) + await hook.onTaskCreate(task) + trackStub.resetHistory() + await hook.onTaskError(task.taskId, 'llm provider timeout after 30s', task) + + const failed = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_FAILED) + expect((failed?.args[1] as Record).failure_kind).to.equal('timeout') + }) + }) + + describe('toAnalyticsTaskType drift guard (PR #722)', () => { + it('emits the "unknown" sentinel for an un-enumerated daemon task type instead of silently failing the wire enum', async () => { + const task = buildTask('not-a-real-daemon-type', {taskId: 'task-drift'}) + await hook.onTaskCreate(task) + + const created = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.TASK_CREATED) + expect((created?.args[1] as Record).task_type).to.equal('unknown') + }) + }) + + describe('toRelativePath outside-project guard (PR #722)', () => { + it('replaces escaping ../ paths with /basename sentinel', async () => { + const task = buildTask('curate', {projectPath: '/Users/dev/proj', taskId: 'task-outside'}) + await hook.onTaskCreate(task) + const result: LlmToolResultEvent = { + callId: 'c1', + result: JSON.stringify({ + applied: [{filePath: '/tmp/x.md', needsReview: false, path: 'x', status: 'success', type: 'ADD'}], + }), + sessionId: 's1', + taskId: 'task-outside', + timestamp: FIXED_NOW, + toolName: 'curate' as const, + } as unknown as LlmToolResultEvent + await hook.onToolResult('task-outside', result) + + const op = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.CURATE_OPERATION_APPLIED) + expect((op?.args[1] as Record).relative_path).to.equal('/x.md') + }) + + it('replaces raw absolute path with /basename when projectPath is undefined', async () => { + const task = buildTask('curate', {projectPath: undefined, taskId: 'task-no-proj'}) + await hook.onTaskCreate(task) + const result: LlmToolResultEvent = { + callId: 'c1', + result: JSON.stringify({ + applied: [{filePath: '/home/u/secret.md', needsReview: false, path: 'x', status: 'success', type: 'ADD'}], + }), + sessionId: 's1', + taskId: 'task-no-proj', + timestamp: FIXED_NOW, + toolName: 'curate' as const, + } as unknown as LlmToolResultEvent + await hook.onToolResult('task-no-proj', result) + + const op = trackStub.getCalls().find((c) => c.args[0] === AnalyticsEventNames.CURATE_OPERATION_APPLIED) + expect((op?.args[1] as Record).relative_path).to.equal('/secret.md') + }) }) }) diff --git a/test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts b/test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts index 57ae6b75b..723f7ac21 100644 --- a/test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts +++ b/test/unit/server/infra/process/analytics-hook-toolmode-inspection.test.ts @@ -61,7 +61,16 @@ async function fakeReadFileForInspection(filePath: string): Promise { return '---\n---\nempty\n' } +/** + * PR #722 review: gated behind `DUMP_ANALYTICS=1` so `npm test` stays quiet + * by default. The shape assertions in each `it()` still execute; the dump + * is an opt-in diagnostic for inspecting payloads (`DUMP_ANALYTICS=1 npx + * mocha test/unit/.../analytics-hook-toolmode-inspection.test.ts`). + */ +const DUMP_ENABLED = process.env.DUMP_ANALYTICS === '1' + const dumpEvents = (label: string, trackStub: sinon.SinonStub): void => { + if (!DUMP_ENABLED) return console.log(`\n┌─ ${label} ${'─'.repeat(Math.max(0, 70 - label.length))}`) for (const [i, call] of trackStub.getCalls().entries()) { const eventName = call.args[0] as string @@ -93,6 +102,11 @@ describe('analytics-hook tool-mode event inspection (M14)', () => { // Counters all-zero today because onToolResult never fires for // tool-mode (no LLM tool calls) — that's the FU-1 follow-up. + // + // FU-1 forward-compat note (PR #722 review): once FU-1 lands and the + // daemon synthesises a curate op from `task.result`, these asserts + // will flip from `=== 0` to non-zero. That is a FEATURE, not a + // regression — update the expectations in the FU-1 PR. const runCompleted = trackStub.getCalls().find((c) => c.args[0] === 'curate_run_completed') const counters = runCompleted?.args[1] as Record expect(counters.operations_added).to.equal(0) diff --git a/test/unit/server/infra/process/analytics-hook.test.ts b/test/unit/server/infra/process/analytics-hook.test.ts index 48a566ecb..6e3ee8051 100644 --- a/test/unit/server/infra/process/analytics-hook.test.ts +++ b/test/unit/server/infra/process/analytics-hook.test.ts @@ -145,8 +145,9 @@ describe('AnalyticsHook', () => { expect(m12Calls()).to.have.lengthOf(2) expect(m12Calls()[0].args[0]).to.equal(AnalyticsEventNames.CURATE_OPERATION_APPLIED) const firstProps = m12Calls()[0].args[1] as Record - // buildCurateTask sets projectPath:'/project'; path.relative('/project','/a.md') = '../a.md' - expect(firstProps.relative_path).to.equal('../a.md') + // buildCurateTask sets projectPath:'/project'; /a.md escapes the + // project root → PR #722 outside-project sentinel + basename. + expect(firstProps.relative_path).to.equal('/a.md') expect(firstProps.knowledge_path).to.equal('notes/a') expect(firstProps.operation_type).to.equal('ADD') expect(firstProps.needs_review).to.equal(false) @@ -333,7 +334,11 @@ describe('AnalyticsHook', () => { const paths = props.read_paths_with_metadata as Array> expect(paths).to.have.lengthOf(3) // sorted lexicographically; relativized against projectPath:'/project' - expect(paths.map((p) => p.relative_path)).to.deep.equal(['../a.md', '../b.md', '../c.md']) + expect(paths.map((p) => p.relative_path)).to.deep.equal([ + '/a.md', + '/b.md', + '/c.md', + ]) // each entry has empty keywords/tags arrays and an empty related_paths // list — no frontmatter source files exist in this in-memory test. for (const entry of paths) { @@ -723,8 +728,8 @@ describe('AnalyticsHook', () => { const first = filterM12(bundle.trackStub)[0].args[1] as Record const second = filterM12(bundle.trackStub)[1].args[1] as Record // buildCurateTask projectPath:'/project'; absolute paths relativize with '../' prefix - expect(first.relative_path, 'first emit must be op1').to.equal('../op1.md') - expect(second.relative_path, 'second emit must be op2').to.equal('../op2.md') + expect(first.relative_path, 'first emit must be op1').to.equal('/op1.md') + expect(second.relative_path, 'second emit must be op2').to.equal('/op2.md') }) it('onTaskCompleted waits for in-flight onToolResult work before emitting CURATE_RUN_COMPLETED', async () => { @@ -776,8 +781,8 @@ describe('AnalyticsHook', () => { expect(filterM12(bundle.trackStub)).to.have.lengthOf(1) const props = filterM12(bundle.trackStub)[0].args[1] as Record - // buildCurateTask sets projectPath:'/project'; '/missing.md' relativizes to '../missing.md' - expect(props.relative_path).to.equal('../missing.md') + // /missing.md escapes the '/project' root — PR #722 outside-project sentinel. + expect(props.relative_path).to.equal('/missing.md') expect(props.keywords).to.deep.equal([]) expect(props.tags).to.deep.equal([]) expect(props).to.not.have.property('related') From 8d28f4bec83c3e312ff492b5f61ed2f96ea5dcda Mon Sep 17 00:00:00 2001 From: Cuong Date: Wed, 27 May 2026 21:28:13 +0700 Subject: [PATCH 7/7] fix: [ENG-2964] M15.6 drift sentinel must be on the wire vocabulary (PR #722 re-review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review caught a real bug in the previous fixup: `toAnalyticsTaskType` returned `'unknown' as TaskType` for un-enumerated daemon types, but `'unknown'` was NOT in `TASK_TYPE_VALUES`. The docblock promised "keeps the event on the wire instead of silently failing the Zod check at the backend" — but the wire-side `z.enum(TASK_TYPE_VALUES)` actually still rejected the row. The local Sinon trackStub doesn't run Zod, which is why the m14 test passed even though the runtime path was broken. Also the `as TaskType` cast moved sites but didn't disappear — the exact anti-pattern the original review flagged. Fix: - Add `TaskTypes.UNKNOWN: 'unknown'` to the canonical vocabulary. - Append `TaskTypes.UNKNOWN` to `TASK_TYPE_VALUES` so every task_* schema accepts the sentinel via `z.enum(TASK_TYPE_VALUES)`. - Introduce `isCanonicalTaskType()` predicate so `toAnalyticsTaskType` narrows without the `as TaskType` cast. Sentinel returned as `TaskTypes.UNKNOWN` directly. - Update `task-types.test` to assert the new `UNKNOWN` key/value pair. Existing negative test for an unknown task_type already uses `'not-a-real-type'` (not `'unknown'`), so it stays green. Cosmetic: dropped the extra-space drift in the OUTSIDE_PROJECT_PATH docblock noted in the same re-review. Tests: 9227 passing, 0 failing. --- src/server/infra/process/analytics-hook.ts | 31 ++++++++++--------- src/shared/analytics/task-types.ts | 10 ++++++ test/unit/shared/analytics/task-types.test.ts | 6 ++++ 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/server/infra/process/analytics-hook.ts b/src/server/infra/process/analytics-hook.ts index 665f65979..e08b23077 100644 --- a/src/server/infra/process/analytics-hook.ts +++ b/src/server/infra/process/analytics-hook.ts @@ -23,14 +23,13 @@ import {CURATE_TASK_TYPES} from './curate-log-handler.js' import {QUERY_TASK_TYPES} from './query-log-handler.js' /** - * Backstop sentinel emitted when the daemon dispatches a task type the - * analytics enum doesn't recognise. Keeps the event on the wire instead of - * silently failing the Zod check at the backend. Update TASK_TYPE_VALUES - * (M14.1) when a new daemon type lands; this is the drift alarm. + * Set of canonical task types accepted by the wire schema. Membership check + * runs in `toAnalyticsTaskType` to gate emits against the daemon dispatching + * a string TASK_TYPE_VALUES doesn't enumerate. */ -const UNKNOWN_TASK_TYPE: TaskType = 'unknown' as TaskType +const ANALYTICS_TASK_TYPE_SET: ReadonlySet = new Set(Object.values(TaskTypes) as TaskType[]) -const ANALYTICS_TASK_TYPE_SET: ReadonlySet = new Set(Object.values(TaskTypes)) +const isCanonicalTaskType = (value: string): value is TaskType => (ANALYTICS_TASK_TYPE_SET as Set).has(value) /** * Translate the daemon's runtime task type string to the canonical @@ -39,22 +38,24 @@ const ANALYTICS_TASK_TYPE_SET: ReadonlySet = new Set(Object.values(TaskT * `'curate-tool-mode'`. Once the rename PR lands, the alias becomes * dead code and can be inlined. * - * Guards an unknown daemon type with a process log + 'unknown' sentinel - * fallback (PR #722 review): swallowing the value silently would let an - * un-enumerated dispatch slip past the wire-side Zod check and disappear - * at the backend; logging here keeps the drift debuggable at the daemon. + * Drift guard (PR #722 review re-review): if the daemon dispatches a + * type that isn't enumerated in `TASK_TYPE_VALUES`, fall back to + * `TaskTypes.UNKNOWN` (which is in the wire vocabulary, so the backend + * accepts the row) and log a daemon-side breadcrumb. The previous + * implementation cast a non-enumerated string back to `TaskType`, + * which silently failed the backend Zod check. */ function toAnalyticsTaskType(daemonType: string): TaskType { if (daemonType === 'curate-html-direct') return TaskTypes.CURATE_TOOL_MODE - if (ANALYTICS_TASK_TYPE_SET.has(daemonType)) return daemonType as TaskType - processLog(`AnalyticsHook: unknown task type '${daemonType}' — falling back to '${UNKNOWN_TASK_TYPE}'`) - return UNKNOWN_TASK_TYPE + if (isCanonicalTaskType(daemonType)) return daemonType + processLog(`AnalyticsHook: unknown task type '${daemonType}' — falling back to '${TaskTypes.UNKNOWN}'`) + return TaskTypes.UNKNOWN } /** * Stable sentinel for paths that can't be safely emitted as project- - * relative — either outside the project root or the project root itself - * is unknown. The backend can group these without leaking host layout. + * relative — either outside the project root or the project root itself + * is unknown. The backend can group these without leaking host layout. */ const OUTSIDE_PROJECT_PATH = '' diff --git a/src/shared/analytics/task-types.ts b/src/shared/analytics/task-types.ts index f8976f5ac..7a1058af8 100644 --- a/src/shared/analytics/task-types.ts +++ b/src/shared/analytics/task-types.ts @@ -19,6 +19,15 @@ export const TaskTypes = { QUERY: 'query', QUERY_TOOL_MODE: 'query-tool-mode', SEARCH: 'search', + /** + * Drift sentinel — emitted by `AnalyticsHook.toAnalyticsTaskType` when the + * daemon dispatches a type that isn't enumerated above. Lives in the + * canonical vocabulary so the wire-side `z.enum(TASK_TYPE_VALUES)` accepts + * the row at the backend instead of dropping it. The daemon-side + * `processLog` warning is the actual signal — `'unknown'` on the wire is + * the breadcrumb the backend can group on. + */ + UNKNOWN: 'unknown', } as const export type TaskType = (typeof TaskTypes)[keyof typeof TaskTypes] @@ -38,4 +47,5 @@ export const TASK_TYPE_VALUES = [ TaskTypes.QUERY, TaskTypes.QUERY_TOOL_MODE, TaskTypes.SEARCH, + TaskTypes.UNKNOWN, ] as const diff --git a/test/unit/shared/analytics/task-types.test.ts b/test/unit/shared/analytics/task-types.test.ts index 2e39886d5..2118ad5c4 100644 --- a/test/unit/shared/analytics/task-types.test.ts +++ b/test/unit/shared/analytics/task-types.test.ts @@ -21,6 +21,11 @@ describe('TaskTypes', () => { 'QUERY', 'QUERY_TOOL_MODE', 'SEARCH', + // PR #722 re-review: 'unknown' is the drift sentinel emitted by + // AnalyticsHook.toAnalyticsTaskType when the daemon dispatches a + // type that isn't enumerated above. Lives in the canonical + // vocabulary so the wire-side z.enum check accepts the row. + 'UNKNOWN', ]) }) @@ -34,6 +39,7 @@ describe('TaskTypes', () => { expect(TaskTypes.QUERY).to.equal('query') expect(TaskTypes.QUERY_TOOL_MODE).to.equal('query-tool-mode') expect(TaskTypes.SEARCH).to.equal('search') + expect(TaskTypes.UNKNOWN).to.equal('unknown') }) it('should expose TaskType as the union of values', () => {