Feat/eng 2971

[cuongdo-byterover]

Summary

• Problem:
• Why it matters:
• What changed:
• What did NOT change (scope boundary):

Type of change

• Bug fix
• New feature
• Refactor (no behavior change)
• Documentation
• Test
• Chore (build, dependencies, CI)

Scope (select all touched areas)

• TUI / REPL
• Agent / Tools
• LLM Providers
• Server / Daemon
• Shared (constants, types, transport events)
• CLI Commands (oclif)
• Hub / Connectors
• Cloud Sync
• CI/CD / Infra

Linked issues

• Closes #
• Related #

Root cause (bug fixes only, otherwise write N/A)

• Root cause:
• Why this was not caught earlier:

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s):
• Key scenario(s) covered:

User-visible changes

List user-visible changes (including defaults, config, or CLI output).
If none, write None.

Evidence

Attach at least one:

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint)
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format
• Documentation updated (if applicable)
• No breaking changes (or clearly documented above)
• Branch is up to date with main

Risks and mitigations

List real risks for this PR. If none, write None.

• Risk:
  • Mitigation:

[github-actions]

Claude finished @cuongdo-byterover's task in 6m 12s —— View job

---

Review summary

Overall score: 3.5 / 5 — Solid M14 follow-up work with good test coverage for the new task_* funnel emits and per-task FIFO async correctness. Held back from a higher score by one wire-format-breaking change that needs explicit backend coordination, an unsafe as TaskType cast that papers over a real failure mode, and a counter/emit-stream inconsistency on filePath-less ops.

What's strong

• AnalyticsHook async lifecycle is tightly reasoned. The per-task FIFO chain (pendingByTask) with a never-rejecting tail + a returned next that preserves rejection is exactly the right shape for ordered emits without poisoning the chain. The integration stress test (analytics-hook-async-stress.test.ts) drives 50 ops + multi-task interleaving through the real TaskRouter and verifies arrival-order + terminal-last sequencing. Good defense-in-depth.
• M14.3 funnel emits placement. TASK_CREATED fires before M12 state init so it lands even if downstream throws; TASK_COMPLETED / TASK_FAILED fire after per-flavor emits so terminal-last ordering is preserved. The cancellation→task_failed mapping matches the schema docblock.
• Test coverage on the M14 hot paths is thorough (analytics-hook-m14.test.ts covers all four task-lifecycle combinations × curate/query/dream-scan/search).
• TASK_TYPE_VALUES consolidation removes the duplicated literal arrays across curate-run-completed / query-completed / task-* schemas. One-place-to-update for new daemon task types is the right invariant.

Issues raised inline

#	Severity	File	What
1	concern	analytics-hook.ts:31-34	daemonType as TaskType cast bypasses validation; a new daemon TaskTypeSchema value not mirrored in TASK_TYPE_VALUES would silently emit an unvalidatable payload. Violates CLAUDE.md ("Avoid as Type assertions").
2	concern	analytics-hook.ts:43-50	toRelativePath doesn't fully neutralise host-path PII — paths outside the project return ../../Users/.... Schema header sells this change as "privacy + portability"; reality is only partial.
3	blocking	curate-operation-applied.ts:19-32	Wire-format breaking change: absolute_path → relative_path, keywords/tags optional → required, schema is .strict(). Needs explicit backend coordination — PR body should confirm telemetry backend deploy ordering + rollback plan. Same applies to query-completed.read_paths_with_metadata.
4	concern	analytics-hook.ts:510-516	When op.filePath is missing, the type counter bumps but no per-op emit fires → curate_run_completed.operations_added (etc.) will diverge from COUNT(*) curate_operation_applied. Move the early continue before the switch, or emit with a sentinel.
5	nitpick	analytics-hook-toolmode-inspection.test.ts:64-74	dumpEvents writes ~8 console.log lines per test × 6 tests = ~200 lines of noise in every npm test run. CLAUDE.md: "Suppress console logging in tests." Either gate on env var, mark suite as .skip by default, or delete if it was PR-review scaffolding.

Things I didn't flag inline but worth mentioning

• The class docblock at analytics-hook.ts:115-130 only mentions M12.2 / M12.3 — could be updated to call out the M14.3 funnel emits.
• Asymmetric projectPath capture: curate state snapshots it at onTaskCreate; query state pulls from task.projectPath at terminal time. Harmless today (task.projectPath isn't mutated mid-task) but the asymmetry is surprising.
• dream is in TASK_TYPE_VALUES (task-types.ts:18) but not in the daemon's TaskTypeSchema (schemas.ts:427-436). Dead value in the analytics enum — fine, but the task-types.ts docstring at line 12 says "Mirrors the daemon's TaskInfo.type union" which is no longer literally true.
• Curate-log-handler now creates log rows for curate-html-direct tasks (curate-log-handler.ts:58), but tool-mode curate doesn't fire onToolResult yet — these entries persist with all-zero summaries. The toolmode-inspection test calls this out as a known FU-1 deferred fix, which is reasonable.

Checklist

• Read changed files
• Analyze analytics-hook.ts changes (M14.3 funnel emits + relative-path conversion + per-task FIFO)
• Analyze event schema changes (wire-format break)
• Analyze test coverage
• Post inline comments for specific issues (5 inline comments)
• Post summary with score
  · Branch: feat/ENG-2971

[github-actions]

suggestion (concern, non-blocking): the daemonType as TaskType cast bypasses validation. If a new daemon TaskTypeSchema value is added (e.g. a future vc-commit) but not mirrored in TASK_TYPE_VALUES, this silently emits a payload that will be rejected by the per-event schemas downstream (since they use z.enum(TASK_TYPE_VALUES)). Per CLAUDE.md: "Avoid as Type assertions — use type guards or proper typing instead."

A safer pattern:

const TASK_TYPE_SET: ReadonlySet<string> = new Set(TASK_TYPE_VALUES)
function toAnalyticsTaskType(daemonType: string): TaskType | undefined {
  if (daemonType === 'curate-html-direct') return TaskTypes.CURATE_TOOL_MODE
  return TASK_TYPE_SET.has(daemonType) ? (daemonType as TaskType) : undefined
}

…and have emit skip the call when task_type is undefined (with a processLog for visibility). Today the daemon's TaskTypeSchema only diverges by curate-html-direct, so the practical fallout is bounded — but the comment promises this becomes a "no-op identity" once rename lands, while in practice it would still need the safety net.

[github-actions]

issue (concern): The PII-removal claim is only partly true. path.relative('/project', '/Users/dev/foo.md') returns '../../Users/dev/foo.md' — the full host path is still on the wire, just rebased. The tests at analytics-hook.test.ts:149 (relative_path === '../a.md') and analytics-hook.test.ts:336 (['../a.md', '../b.md', '../c.md']) confirm this — these payloads still leak the parent dir info.

In real curate runs the affected files almost always live inside projectPath, so this only bites pathological inputs. But the comment header on the schema (curate-operation-applied.ts:13) sells this change as "privacy + portability across hosts" — the privacy claim doesn't hold when a path can escape the project root.

Two options worth considering:

1. Clamp escapes: if rel.startsWith('..'), emit a marker like <outside-project> (or null) instead of the host-leaking string.
2. Document the residual leak explicitly in the schema header.

Also: the '.' fallback emits a directory marker for a file operation — surprising shape for downstream consumers, but currently unreachable (filePath always points to a file).

[github-actions]

issue (blocking) — wire-format breaking change:

This rev does three things on the same emit:

• renames absolute_path → relative_path
• promotes keywords / tags from optional → required
• keeps .strict() so unknown extras (incl. the old absolute_path) are rejected

If the analytics backend (telemetry-dev / prod) was previously consuming absolute_path or treating keywords / tags as nullable, this PR will start dropping rows post-deploy until the backend is updated in lockstep. The PR description doesn't mention coordination with the telemetry team.

Please confirm in the PR body:

1. Backend has been (or will be) updated to read relative_path and require keywords/tags.
2. Whether any backfill is needed for existing dashboards.
3. Roll-forward / roll-back plan if the deploy ordering slips.

The same review tightening applies to query-completed.ts (read_paths_with_metadata[*] entries).

[github-actions]

suggestion (nitpick): dumpEvents writes 8+ console.log lines per test, and the file calls it from 6 of the 7 test cases. Per CLAUDE.md: "Suppress console logging in tests to keep output clean."

If the dumps are scaffolding for manual review against a fixed event shape that you'll keep eyeballing, that's fine — but please mark the suite as inspection-only (e.g. .skip by default + a script in package.json to run it), or guard dumpEvents on process.env.BRV_DUMP_ANALYTICS_EVENTS. As-is this adds ~200 lines of noise to every npm test run.

If the dumps were only to validate the M14 wire shape during PR review and aren't intended long-term, consider deleting dumpEvents and keeping only the expect(...) assertions that already cover the same ground in analytics-hook-m14.test.ts.

[github-actions]

issue (concern): When op.filePath is missing, processToolResult continues out at line 498 — but only AFTER the switch at lines 464–490 has already bumped the type counter. The aggregate curate_run_completed.operations_added (etc.) will count an op for which no curate_operation_applied event was ever emitted.

A downstream dashboard that reconstructs counts from curate_operation_applied (e.g. COUNT(*) GROUP BY operation_type per task_id) will diverge from the aggregate counter on curate_run_completed.

Two reasonable fixes:

1. Skip the counter bump too: move the if (!op.filePath) continue before the switch (~line 463).
2. Emit with a sentinel like relative_path: '<unknown>' so the per-op stream stays complete.

The comment at line 497 explicitly says these ops are rare, so (1) is the simplest reconciliation.