Feat/eng 2964

[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 5m 26s —— View job

---

Claude Code PR Review

• Load review tools
• Examine PR diff against origin/proj/analytics-system-tool-mode
• Review analytics-hook.ts changes (largest delta)
• Review event schemas (task-failed, query-completed, curate-*)
• Review test coverage (new and modified tests)
• Post inline comments for specific issues
• Post final summary with score

---

Summary

This PR wires AnalyticsHook as the 4th lifecycle peer in TaskRouter, adds failure_kind to task_failed, extends TASK_TYPE_VALUES to cover the v4.0 tool-mode types (curate-tool-mode, query-tool-mode, dream-scan, dream-finalize), and tightens the M14-review wire shape (absolute_path → relative_path, required keywords / tags arrays, structured related_paths).

The implementation is solid: the per-task FIFO queue logic in onToolResult is well-reasoned, the late-bind pattern in brv-server.ts for setAnalyticsClient / isAnalyticsEnabled is clean given the boot ordering, and the test coverage is genuinely good — analytics-hook-m14.test.ts and analytics-hook-lifecycle-wiring.test.ts cover the cross-product of (task type × outcome) thoroughly, including the daemon→wire alias for curate-html-direct → curate-tool-mode.

Findings worth addressing before merge:

#	Severity	Finding
1	issue (privacy)	toRelativePath doesn't guard against .. parent traversal when the read file is outside projectPath. Symlinks, linked sources (brv source add), and unset projectPath all produce wire strings that still leak the absolute filesystem layout.
2	suggestion	classifyFailureKind uses raw .includes() for 'tool' / 'agent' / 'llm' / 'provider'. Matches 'tooltip', 'engagement', etc. → false agent_error classifications. Recommend word-boundary regex + a comment documenting the timeout > agent > unknown precedence.
3	issue (test hygiene)	analytics-hook-toolmode-inspection.test.ts writes every emitted event to console.log via dumpEvents. Matches test/**/*.test.ts so it spams every npm test. CLAUDE.md explicitly calls this out.
4	nitpick (type safety)	toAnalyticsTaskType returns daemonType as TaskType unchecked. A future daemon type added without updating TASK_TYPE_VALUES silently emits events the backend zod-rejects, with no daemon-side signal.
5	nitpick (forward-compat)	The "counters all-zero today" assertions in analytics-hook-toolmode-inspection.test.ts pin a known limitation as a positive expectation — these will read like a regression when FU-1 lands.
6	question (consistency)	curate_operation_applied.related stayed as flat string[] while query_completed.read_paths_with_metadata[].related_paths was tightened to structured objects. Asymmetry is fine if deliberate, but a TODO / tracking link would prevent the shape ossifying in two idioms.
7	praise	The lifecycleHooks[] late-bind pattern in brv-server.ts (register hook early, inject client after setupFeatureHandlers resolves) is well-shaped. One tiny defensive suggestion left inline about isEnabled defaulting to false.

None of these are merge-blockers individually — #1 is the one I'd most want to fix on this PR (it's a privacy regression masquerading as a no-op), and #3 will quietly degrade test ergonomics for everyone if it isn't caught now.

The schema additions, test coverage, async ordering invariants, and the boot-time wire-up all look correct. The PII surface area is the main thing to harden.

Overall Score: 4 / 5

Strong, well-tested implementation; the privacy guard on toRelativePath and the test-output cleanup hold it back from a 5.
· Branch feat/ENG-2964

[github-actions]

Claude finished @cuongdo-byterover's task in 3m 55s —— View job

---

Claude Code PR Review (re-review after fixups)

• Load review tools
• Diff feat/ENG-2964 against origin/proj/analytics-system-tool-mode
• Verify the 7 previous review findings were addressed in commit 190f4b4
• Spot-check analytics-hook.ts (toRelativePath, classifyFailureKind, toAnalyticsTaskType) and inspection test
• Look for any new issues introduced by the fixup
• Post final summary with updated score

---

What got addressed

#	Original finding	Status	Note
1	classifyFailureKind substring matching	✅ Resolved	Word-boundary regexes (/\b(timeout|timed out|deadline exceeded)\b/, /\b(agent|llm|provider|tool)\b/). Precedence pinned in the docblock. Two new tests in analytics-hook-m14.test.ts (tooltip/engagement → unknown; llm provider timeout → timeout).
2	toRelativePath PII leak on .. / unset projectPath	✅ Resolved	Returns <outside-project>/<basename> for parent-traversal, absolute (Windows drive switch), and projectPath===undefined. Two new tests cover both branches. Stress-test fixtures rebased onto /proj/A/op-N.md so the in-project case is also exercised.
3	toAnalyticsTaskType unchecked widening	⚠️ Partially resolved — new issue, see below	Drift logged + sentinel returned, but the sentinel 'unknown' isn't on the wire vocabulary.
4	Inspection test console.log noise	✅ Resolved	Gated behind process.env.DUMP_ANALYTICS === '1'. npm test is quiet; the diagnostic is opt-in.
5	FU-1 forward-compat comment	✅ Resolved	Block comment in analytics-hook-toolmode-inspection.test.ts:106-109 explicitly tells future readers "this flips to non-zero in FU-1 — feature, not regression."
6	curate_operation_applied.related shape asymmetry	✅ Resolved (TODO marker)	TODO(M15.x) added at curate-operation-applied.ts:27-30 pointing at the harmonization ticket.
7	Loud-fail on missing analyticsClient	✅ Resolved	brv-server.ts:686-688 throws explicitly. The first defensive note (default isAnalyticsEnabledRef to () => false) was not taken, but it's strictly defensive — the boot-time race is already documented.

---

One real new issue from the fixup ⚠️

toAnalyticsTaskType drift sentinel doesn't actually keep the event on the wire — src/server/infra/process/analytics-hook.ts:31 + 47-52

const UNKNOWN_TASK_TYPE: TaskType = 'unknown' as TaskType

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
}

The docblock promises this "keeps the event on the wire instead of silently failing the Zod check at the backend." It doesn't — 'unknown' is not in TASK_TYPE_VALUES:

• src/shared/analytics/task-types.ts:31-41 — TASK_TYPE_VALUES lists 9 types, none of them 'unknown'.
• Every task_* schema validates via z.enum(TASK_TYPE_VALUES) — see task-failed.ts:38, task-created.ts:21, task-completed.ts:17, query-completed.ts:69, curate-run-completed.ts:30.
• test/unit/shared/analytics/task-types.test.ts:96-107 is the smoking gun: the suite already asserts the schemas reject unknown task_types.

What actually happens at runtime: AnalyticsClient.track() (analytics-client.ts:230-249) and JsonlAnalyticsStore.append() don't run the per-event schema, so the event persists locally and the daemon logs nothing wrong. But the moment the batch ships, the backend Zod check fails the row — the same failure mode the guard claims to prevent, just with a noisier wire payload.

Secondary concern: 'unknown' as TaskType is the exact as Type widening CLAUDE.md forbids and that the original review #4 called out. The cast at the unsafe site moved to a cast on the constant.

Two ways to actually close the gap, pick one:

1. Add 'unknown' to the canonical vocabulary — TaskTypes.UNKNOWN: 'unknown' in TaskTypes, appended to TASK_TYPE_VALUES. The Zod schema then accepts the sentinel, the cast disappears, and the m14 test at line 269-275 holds for real.
2. Don't emit the funnel event at all for unknown types — return undefined from toAnalyticsTaskType, have callers skip the emit. The log line still gives a daemon-side breadcrumb. Trade-off: an un-enumerated type vanishes from analytics entirely (no funnel row) instead of bucketing as unknown.

Option 1 matches the docblock's stated intent better and is the smaller change. Either way, this is the right PR to close the gap because the new m14 drift-guard test (analytics-hook-m14.test.ts:269-275) currently uses a Sinon stub that never runs Zod — a real integration would catch this.

Fix this →

---

Smaller observations (non-blocking)

• src/server/infra/process/analytics-hook.ts:55-59 — the docblock for OUTSIDE_PROJECT_PATH has a leading-space indentation drift (lines 56-57 carry an extra space). Cosmetic.
• src/server/infra/process/analytics-hook.ts:74 — basename is taken from the absolute path. On Windows running against a POSIX-style absolute string (or vice versa), the basename split won't honor the foreign separator. Realistically the daemon and the agent are on the same host so this never bites, but worth a thought if cross-platform mocking ever enters scope.
• analytics-hook.test.ts was substantially reworked (+121/−81). The analytics-hook-async-stress.test.ts fixture rebase to /proj/... is the right move — it isolates the outside-project case to its own focused test instead of conflating it with serialization invariants.

Praise (post-fix)

The m14.test.ts additions are the right shape: every new branch (word-boundary classifier, outside-project guard, drift-guard sentinel, classifier precedence) gets a tight named test with a (PR #722) tag in the description. That makes review-comment provenance auditable from the test suite years from now — quietly excellent practice.

---

Overall Score: 4 / 5

The privacy fix, classifier hardening, test hygiene gating, and loud-fail assertion all landed clean and the new test coverage is thorough. Holding at 4 because the drift-guard sentinel (#3 in the previous review) didn't actually close the gap it claims to — 'unknown' isn't a valid wire value, so the backend still drops the row. Adding TaskTypes.UNKNOWN to the canonical vocabulary closes the loop and gets this to 5.
· Branch feat/ENG-2964

[github-actions]

Claude finished @cuongdo-byterover's task in 2m 19s —— View job

---

Claude Code PR Review (3rd pass — drift-sentinel fix)

• Load review tools
• Diff current HEAD against origin/proj/analytics-system-tool-mode (focus on commit 8d28f4b)
• Verify drift sentinel (TaskTypes.UNKNOWN) is now on the wire vocabulary
• Re-check analytics-hook.ts, task-types.ts, and the m14 drift-guard test
• Look for any new issues introduced by the fixup
• Post final summary with updated score

---

What landed in 8d28f4b

#	Previous-review issue	Status	Evidence
3 (re-review)	Drift sentinel 'unknown' returned but not in TASK_TYPE_VALUES → backend Zod still drops the row	✅ Resolved	TaskTypes.UNKNOWN: 'unknown' added at src/shared/analytics/task-types.ts:30; appended to TASK_TYPE_VALUES at :50. Every task_* schema validates via z.enum(TASK_TYPE_VALUES) (task-created.ts:21, task-completed.ts:17, task-failed.ts:38, query-completed.ts:69, curate-run-completed.ts:30) — the sentinel now actually rides the wire.
3 (orig)	daemonType as TaskType cast	✅ Resolved	Replaced with a isCanonicalTaskType(value): value is TaskType user-defined type guard at analytics-hook.ts:32. The unsafe cast at the call site is gone. The cast moved to one tightly scoped (ANALYTICS_TASK_TYPE_SET as Set<string>).has(value) inside the predicate, which is the standard pattern for widening a Set<T>.has argument.
—	OUTSIDE_PROJECT_PATH docblock cosmetic drift	✅ Resolved	Leading-space drift on lines 56–57 cleaned up; cosmetic only.
—	Default isAnalyticsEnabledRef to () => false (optional defensive nit)	⚪ Not taken	brv-server.ts:378 still defaults to () => true. Acknowledged in the previous review as "strictly defensive — boot-time race is already documented." Fine to leave.

Verification chain (why this is genuinely closed, not just moved)

1. TaskTypes.UNKNOWN is now part of Object.values(TaskTypes).
2. TASK_TYPE_VALUES is asserted-equal to Object.values(TaskTypes) by the invariant test at test/unit/shared/analytics/task-types.test.ts:51-53 — so any future drift between the const-object and the tuple fails CI.
3. All five task_* schemas validate task_type against z.enum(TASK_TYPE_VALUES) (grep confirmed). The wire-side check now accepts 'unknown' as a legitimate row value rather than dropping it.
4. The negative test "rejects an unknown task_type" (task-types.test.ts:102-113) uses 'not-a-real-type', not 'unknown', so it's untouched by adding the sentinel.
5. The m14 drift-guard test (analytics-hook-m14.test.ts:268-275) still uses a Sinon stub (no live Zod), but the invariant in step 2 covers the gap the previous review flagged — a regression where TaskTypes.UNKNOWN gets added without TASK_TYPE_VALUES being updated would fail at task-types.test.ts:52, before the analytics-hook layer ever runs.

One small forward-looking observation (non-blocking)

isCanonicalTaskType performs membership against Object.values(TaskTypes). That's a strict superset of TASK_TYPE_VALUES only if those two drift — but the invariant test from step 2 above pins them equal, so this is currently safe. If a future contributor finds a reason to maintain those independently (e.g. internal-only TaskTypes keys not on the wire), the predicate would lie. The TASK_TYPE_VALUES-equality test is the load-bearing invariant; worth keeping in mind for any future refactor.

Praise

The fix is exactly the right shape: smallest change that closes the real gap (TaskTypes.UNKNOWN lives in the canonical vocabulary, predicate eliminates the cast). The commit message accurately diagnoses why the previous fixup didn't work (Sinon stub doesn't run Zod) and explains what changed. That diagnosis is more valuable than the diff itself — it's the kind of failure analysis that prevents the same class of bug in a year.

---

Overall Score: 5 / 5

The drift-sentinel gap is genuinely closed — 'unknown' is on the wire vocabulary, every task_* schema accepts it via z.enum(TASK_TYPE_VALUES), and the invariant test at task-types.test.ts:51-53 is the load-bearing safety net for future schema/vocabulary drift. The unchecked as TaskType widening that CLAUDE.md forbids is gone. Privacy guard (toRelativePath), classifier hardening, test hygiene, and the loud-fail assertion from the prior pass all remain intact. Ready to merge.
· Branch feat/ENG-2964