feat(telemetry): start Nerve app telemetry implementation - #309
feat(telemetry): start Nerve app telemetry implementation#309kimbotai1337 wants to merge 39 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full telemetry subsystem: docs, installer/setup stamps and flags for fresh installs, server-side telemetry store/metadata/transport/runtime, route and middleware instrumentation, client telemetry surface and UI notice, and extensive tests. Changes are gated by telemetry mode and implemented as best-effort. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Installer as install.sh
participant EnvWriter as env-writer (tsx)
participant Stamp as telemetry-stamp CLI
participant Disk as Filesystem
Installer->>Installer: detect fresh clone (IS_FRESH_INSTALL)
Installer->>EnvWriter: generate .env (inject NERVE_TELEMETRY_MODE=minimal if fresh)
EnvWriter->>Disk: write `.env`
Installer->>Stamp: stamp bootstrap/install-method (async, tolerant)
Stamp->>Disk: write `.nerve/telemetry/*` JSON markers
sequenceDiagram
autonumber
participant Client as Browser/UI
participant AppRoute as /api/telemetry/events
participant Runtime as TelemetryRuntime
participant Store as TelemetryStore
participant Transport as Telemetry HTTP
participant Phase2 as Phase 2 Endpoint
Client->>AppRoute: POST branch_switched
AppRoute->>Runtime: recordClientDetailedEvent(payload)
Runtime->>Runtime: resolve mode (off|minimal|detailed)
alt mode == detailed
Runtime->>Store: persist/store event snapshot
Runtime->>Transport: postJson(/v2/events, event)
Transport->>Phase2: deliver payload
Phase2-->>Transport: 202
end
AppRoute-->>Client: 200 { ok: true }
sequenceDiagram
autonumber
participant Server as App startup
participant Runtime as TelemetryRuntime
participant Metadata as InstallMetadata
participant Store as TelemetryStore
participant Transport as Telemetry HTTP
Server->>Runtime: createTelemetryRuntime(...)
Runtime->>Metadata: ensureInstanceId(), read bootstrap/install markers
Runtime->>Store: readWindow()
Runtime->>Runtime: resolveTelemetryMode()
alt telemetry enabled
Runtime->>Transport: send first_seen/version_change heartbeats (async)
Transport->>Endpoint: POST /v1/heartbeat
Endpoint-->>Transport: 202
Runtime->>Store: noteHeartbeatSent(...)
Runtime->>Runtime: schedule daily heartbeat with jitter
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e563643685
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (messages.length > 0) { | ||
| markSettingsFeatureUsed(); | ||
| } |
There was a problem hiding this comment.
The new settings telemetry hook is only invoked in PUT /api/transcribe/config, but PUT /api/language also persists user settings (NERVE_LANGUAGE and EDGE_VOICE_GENDER) without calling markSettingsFeatureUsed(). In environments where the UI uses /api/language for language/gender changes, features_used_24h.settings will remain false and under-report real settings usage, which skews the new telemetry metrics.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (28)
server/lib/telemetry/error-reporting.ts (1)
57-67: Optional: harden'code' in params.erroragainst throwing getters.Reading
params.error.codeon an arbitrary object will execute a getter if defined, which could throw (or, in pathological cases, perform side effects). Wrapping the access intry { ... } catch { return 'UNKNOWN'; }would make the builder fully resilient to hostile error shapes. Low priority since real-worldErrorsubclasses don't do this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/error-reporting.ts` around lines 57 - 67, The resolveErrorCode function currently checks 'code' in params.error which can invoke a throwing getter; to harden it, wrap the branch that reads params.error.code in a try/catch and return 'UNKNOWN' on any thrown error, while still calling normalizeErrorCode when safe; specifically update resolveErrorCode to perform a guarded access to params.error.code (inside try { if (params.error && typeof params.error === 'object' && 'code' in params.error) { return normalizeErrorCode(params.error.code); } } catch { return 'UNKNOWN'; }) so normalizeErrorCode is only called with a safely-read value..env.example (1)
75-79: Consider documentingNERVE_TELEMETRY_MODEhere too.
.env.exampleis a primary discovery surface for configuration knobs. The PR adds support forNERVE_TELEMETRY_MODE(off/minimal/detailed) inscripts/lib/env-writer.tsand the server config, but it's not surfaced here. Adding a commented entry next to these endpoint overrides would make opt-out and mode selection more discoverable than requiring users to readdocs/TELEMETRY.md.📝 Suggested addition
+# Telemetry mode: off | minimal | detailed (default: minimal on fresh installs, off on legacy upgrades). +# See docs/TELEMETRY.md for what each mode collects. +# NERVE_TELEMETRY_MODE=off + # Optional telemetry endpoint overrides. Defaults already point at the current # Netlify deployment. Keep detailed mode off until /v1/events exists there. # NERVE_TELEMETRY_PHASE1_BASE_URL=https://telemetry-nerve-zone.netlify.app # NERVE_TELEMETRY_PHASE2_BASE_URL=https://telemetry-nerve-zone.netlify.app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 75 - 79, Add a commented example and short description for NERVE_TELEMETRY_MODE to .env.example next to the telemetry endpoint overrides so users can discover the setting without reading docs; document the allowed values (off / minimal / detailed), the default behavior, and an example line like `# NERVE_TELEMETRY_MODE=off` and reference the env key name that scripts/lib/env-writer.ts and the server config read so it’s easy to spot and change.server/index.ts (1)
31-35: Non-blocking startup is fine; consider hardening the error log.
(err as Error).messagewill printundefinedif a non-Errorvalue is thrown. A small guard avoids confusing operator output.♻️ Suggested tweak
-void telemetryRuntime.start().catch((err) => { - console.warn('[telemetry] Failed to start telemetry runtime:', (err as Error).message); -}); +void telemetryRuntime.start().catch((err: unknown) => { + const msg = err instanceof Error ? err.message : String(err); + console.warn('[telemetry] Failed to start telemetry runtime:', msg); +});The runtime safely guards method calls during the brief pre-initialization window: all methods check
telemetryEnabled()which returnsfalseuntilinitialize()completes, causing them to silently return rather than fail.Note:
stop()is fire-and-forget at shutdown (line 170), so pending I/O or heartbeats may not complete before the 5s force-exit timeout—consider awaitingstop()if final event flushing is critical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.ts` around lines 31 - 35, The catch handler for telemetryRuntime.start() logs (err as Error).message which becomes undefined for non-Error throws; update the catch to safely stringify the thrown value (e.g. use err instanceof Error ? err.message : String(err) and include err.stack when available) so the processLogger/console.warn includes usable details; locate the start() invocation on telemetryRuntime and the surrounding setTelemetryRuntime call and replace the current catch logging to defensively handle non-Error values and provide clearer context.server/routes/api-keys.test.ts (1)
85-112: Optional: add a no-op-body case to lock in theresults.length > 0gate.The two new tests cover successful update and invalid-JSON failure, but the route's
if (results.length > 0)branch inserver/routes/api-keys.tslines 64-66 has a third path: a well-formed body that contains none of the three known key fields (e.g.,{}). That should still return 200 with'No changes'and must NOT mark settings used. A short test here prevents future regressions if someone moves themarkSettingsFeatureUsed()call out of the conditional.🧪 Proposed additional test
+ it('does not mark settings used when body contains no known key fields', async () => { + mockDeps(); + const app = await buildApp(); + + const res = await app.request('/api/keys', { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({}), + }); + + expect(res.status).toBe(200); + const json = await res.json() as Record<string, unknown>; + expect(json.message).toBe('No changes'); + expect(telemetryRuntimeMock.markFeatureUsed).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/api-keys.test.ts` around lines 85 - 112, Add a third test case in server/routes/api-keys.test.ts that sends a well-formed empty JSON body (e.g., {}) to the PUT '/api/keys' endpoint and asserts the handler returns 200 with the "No changes" behavior and that telemetryRuntimeMock.markFeatureUsed (or the markSettingsFeatureUsed wrapper if used) is NOT called; this locks in the results.length > 0 gate in the route handler and prevents marking settings used when no known key fields are present.src/App.tsx (1)
684-693: Add a.catchto keep the new.thenchain from leaking unhandled rejections.
requestWorkspaceTransitionconstructs an explicitPromise<boolean>withrejectwired throughpendingSwitch.reject(error)(lines 646, 668). The new.then((didSwitch) => …)doesn't attach a rejection handler, so any rejected switch escapes as an unhandled promise rejection.voidonly discards the value, not the rejection.♻️ Proposed fix
const handleSessionChange = useCallback((key: string) => { const previousSessionKey = currentSession; void requestWorkspaceTransition(key, getWorkspaceSwitchLabel(key), async () => { setCurrentSession(key); }).then((didSwitch) => { if (didSwitch) { maybeEmitBranchSwitchTelemetry(previousSessionKey, key); } - }); + }).catch(() => { + // Switch failed (handled inside the dialog); avoid unhandled rejection. + }); }, [currentSession, getWorkspaceSwitchLabel, maybeEmitBranchSwitchTelemetry, requestWorkspaceTransition, setCurrentSession]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 684 - 693, The then-only promise chain in handleSessionChange can leak unhandled rejections when requestWorkspaceTransition rejects; update handleSessionChange to attach a .catch handler to the Promise returned by requestWorkspaceTransition (after the .then((didSwitch) => …) chain) to handle errors from requestWorkspaceTransition and prevent unhandled promise rejections—e.g., call .catch(error => { /* log/handle error */ }) on the chain that starts with requestWorkspaceTransition(...) while keeping the existing setCurrentSession and maybeEmitBranchSwitchTelemetry logic intact.server/routes/api-keys.ts (1)
17-25: Extract these telemetry helpers into a shared module to remove triple duplication.The exact same
runTelemetryInBackground+markSettingsFeatureUsedpair is copy-pasted inserver/routes/voice-phrases.ts(lines 23-31) andserver/routes/transcribe.ts(lines 37-45). Any future change (e.g., adding logging, switching to a different feature key, or hardening error handling) has to be made in three places and is easy to drift. Consider hoisting to something likeserver/lib/telemetry/feature-helpers.tsand importing from each route.♻️ Proposed extraction
New file
server/lib/telemetry/feature-helpers.ts:import { getTelemetryRuntime, type TelemetryFeature } from './runtime.js'; export function runTelemetryInBackground(work: Promise<unknown>): void { void work.catch(() => {}); } export function markFeatureUsedInBackground(feature: TelemetryFeature): void { const telemetry = getTelemetryRuntime(); if (!telemetry) return; runTelemetryInBackground(telemetry.markFeatureUsed(feature)); }Then in
server/routes/api-keys.ts(and the other two route files):-import { getTelemetryRuntime } from '../lib/telemetry/runtime.js'; +import { markFeatureUsedInBackground } from '../lib/telemetry/feature-helpers.js'; const app = new Hono(); -function runTelemetryInBackground(work: Promise<unknown>): void { - void work.catch(() => {}); -} - -function markSettingsFeatureUsed(): void { - const telemetry = getTelemetryRuntime(); - if (!telemetry) return; - runTelemetryInBackground(telemetry.markFeatureUsed('settings')); -} - @@ - markSettingsFeatureUsed(); + markFeatureUsedInBackground('settings');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/api-keys.ts` around lines 17 - 25, Extract the duplicated helpers runTelemetryInBackground and markSettingsFeatureUsed into a single shared telemetry helper module and import them from each route; move the implementation of runTelemetryInBackground and a generalized markFeatureUsedInBackground(feature) that calls getTelemetryRuntime() and invokes telemetry.markFeatureUsed(feature) (using runTelemetryInBackground to swallow promise errors) into that module, then replace the local markSettingsFeatureUsed usages in api-keys.ts (and the copies in voice-phrases.ts and transcribe.ts) with an import of markFeatureUsedInBackground('settings') (or the equivalent feature identifier).server/routes/sessions.ts (1)
78-83: Defensive:runInBackgroundwon't catch synchronous throws.
runInBackgroundaccepts aPromise<unknown>, so the call expressiontelemetry.recordSessionCreated({...})is evaluated first. IfrecordSessionCreatedever throws synchronously (e.g., a guard clause or future refactor), the exception bypasses the.catch()and propagates out of the route handler — turning a best-effort telemetry call into a 500. Wrapping the work in a thunk (() => Promise<unknown>) and invoking it inside atry/catch(or viaPromise.resolve().then(fn)) would harden this against future changes.🛡️ Proposed defensive refactor
-function runInBackground(work: Promise<unknown>): void { - void work.catch((err) => { +function runInBackground(work: () => Promise<unknown>): void { + void Promise.resolve().then(work).catch((err) => { const message = err instanceof Error ? err.message : String(err); console.warn('[sessions] spawn-subagent telemetry failed:', message); }); } @@ - runInBackground(telemetry.recordSessionCreated({ + runInBackground(() => telemetry.recordSessionCreated({ sessionKey: result.sessionKey, surface: 'sessions', explicit: true, - })); + }));Also applies to: 392-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/sessions.ts` around lines 78 - 83, runInBackground currently takes a Promise and won't catch synchronous throws during promise construction; change its signature to accept a thunk () => Promise<unknown> (rename param to workThunk) and invoke it inside a try/catch so both sync and async errors are handled: in try { const p = workThunk(); void p.catch(...) } catch (err) { /* same logging */ }. Update all call sites (e.g., where telemetry.recordSessionCreated(...) is passed at the runInBackground call near the top and the similar call around lines marked 392-399) to pass a lambda () => telemetry.recordSessionCreated({...}) instead of the raw promise.server/lib/config.ts (1)
32-33: Document why Phase 1 and Phase 2 default URLs are identical.Both defaults point to the same Netlify deployment, but per the PR notes the current deployment does not expose
POST /v1/events(Phase 2). If a user setsNERVE_TELEMETRY_MODE=detailedagainst the default, detailed events will be POSTed to a non-existent endpoint and silently dropped by the transport. That matches the "non-blocking" contract, but a brief comment here will make future operators less likely to trip over it.📝 Suggested clarification
-const DEFAULT_TELEMETRY_PHASE1_BASE_URL = 'https://telemetry-nerve-zone.netlify.app'; -const DEFAULT_TELEMETRY_PHASE2_BASE_URL = 'https://telemetry-nerve-zone.netlify.app'; +// Phase 1 (heartbeat + scrubbed errors) is live on this deployment. +// Phase 2 (POST /v1/events) is not yet exposed there; `detailed` mode is +// expected to be configured against a separate endpoint until Phase 2 ships. +const DEFAULT_TELEMETRY_PHASE1_BASE_URL = 'https://telemetry-nerve-zone.netlify.app'; +const DEFAULT_TELEMETRY_PHASE2_BASE_URL = 'https://telemetry-nerve-zone.netlify.app';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/config.ts` around lines 32 - 33, Add a brief explanatory comment near the constants DEFAULT_TELEMETRY_PHASE1_BASE_URL and DEFAULT_TELEMETRY_PHASE2_BASE_URL in server/lib/config.ts stating that both defaults intentionally point to the same Netlify deployment, but that the current deployment does not expose the Phase 2 POST /v1/events endpoint; note that setting NERVE_TELEMETRY_MODE=detailed with the default base URL will cause detailed events to be POSTed to a non-existent endpoint and silently dropped by the transport (non-blocking behavior), so operators should change DEFAULT_TELEMETRY_PHASE2_BASE_URL or the env var if they expect Phase 2 ingestion to work.server/routes/kanban.ts (1)
76-95: Optional: extract telemetry helpers to a shared module.
runTelemetryInBackground/markKanbanFeatureUsed/recordKanbanTaskCreatedTelemetryare clean and correctly non-blocking. Per the PR summary,server/lib/ws-proxy.tsand the settings/sessions code use similar patterns — pulling these intoserver/lib/telemetry/route-helpers.ts(e.g.,markFeatureInBackground(featureName),recordEventInBackground(...)) would prevent the pattern from drifting per file. Defer if you'd rather not expand the diff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/kanban.ts` around lines 76 - 95, Extract the non-blocking telemetry helpers into a shared module to avoid duplication: move runTelemetryInBackground, markKanbanFeatureUsed, and recordKanbanTaskCreatedTelemetry into a new module (e.g., server/lib/telemetry/route-helpers.ts) and expose generic helpers like markFeatureInBackground(featureName) and recordEventInBackground(eventName, payload). Update server/routes/kanban.ts to import and use markFeatureInBackground('kanban') and recordEventInBackground('kanbanTaskCreated', {...}) (or reuse the preserved names) and remove the local implementations; ensure getTelemetryRuntime() usage stays inside the shared helpers so callers remain simple and non-blocking.src/features/telemetry/telemetryClient.ts (1)
10-16: Optional: considerkeepalive: truefor fire-and-forget telemetry.
emitBranchSwitched,emitSessionOpened, andemitBranchCreatedmay fire just before navigation or tab-unload events in some flows. Addingkeepalive: truelets the browser deliver the request even if the page is being torn down. The payload is small enough to fit within the keepalive byte budget. Not a defect — current callers don't navigate synchronously — but it's a cheap reliability win.♻️ Suggested tweak
await fetch('/api/telemetry/events', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(payload), + keepalive: true, }).catch(() => {});Browser support for
keepaliveis broad (Chromium, Firefox, Safari) — please confirm the project's supported browsers cover it before adopting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/telemetry/telemetryClient.ts` around lines 10 - 16, Update postTelemetryEvent to include the fetch option keepalive: true so fire-and-forget telemetry requests can complete during page unload; specifically add keepalive: true to the options object passed to fetch in postTelemetryEvent (which is used by emitBranchSwitched, emitSessionOpened, and emitBranchCreated), keeping the same method, headers, and body handling and preserving the existing catch behavior.server/middleware/error-handler.test.ts (1)
148-178: Microtask-drain pattern is fragile; consider waitFor/flushPromises.The two
await Promise.resolve()calls assumereportErrorfinishes within two microtask ticks. That holds today (postJson rejects → caught by inner try/catch in runtime.ts), but ifbuildErrorPayloadever gains another await, this test will start missing the/v1/errorcall and silently pass on the negative assertions. Avi.waitFor(() => expect(postJson).toHaveBeenCalled())(or polling formock.calls.find(...)) would be more robust without changing semantics.♻️ Suggested tweak
- await Promise.resolve(); - await Promise.resolve(); - - const errorCall = postJson.mock.calls.find(([target]) => target === '/v1/error'); - expect(errorCall).toBeTruthy(); + const errorCall = await vi.waitFor(() => { + const call = postJson.mock.calls.find(([target]) => target === '/v1/error'); + expect(call).toBeTruthy(); + return call; + }); expect(JSON.stringify(errorCall?.[1])).not.toContain('Bearer top-secret'); expect(JSON.stringify(errorCall?.[1])).not.toContain('/Users/alice/nerve');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/error-handler.test.ts` around lines 148 - 178, The test uses two await Promise.resolve() microtask drains which is fragile if buildErrorPayload or reportError ever add async steps; replace those drains with a deterministic wait for the telemetry mock to be called (e.g., use vi.waitFor(() => expect(postJson).toHaveBeenCalled()) or poll mock.calls for an entry matching '/v1/error') so the assertions that inspect postJson.mock.calls run only after reportError has completed; update the test around createTelemetryRuntime/runtime.start and the postJson assertions to wait for the call before checking the payload contents.src/App.test.tsx (1)
928-951: Optional: consolidate duplicated test setup.The new
App telemetry workspace switchingsuite duplicatesmatchMediamocking and partial state resets (already present in the prior suites). A sharedsetupAppTestEnvironment()helper could reduce drift if any of these mocks evolve. No functional concern for the current tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.test.tsx` around lines 928 - 951, The test suite "App telemetry workspace switching" duplicates matchMedia mocking and state resets in its beforeEach; create a shared helper function (e.g., setupAppTestEnvironment) and move the repeated logic—localStorage.clear(), sessionContext.currentSession assignment, sessionContext.setCurrentSession.mockReset(), sessionContext.spawnSession.mockReset(), emitBranchSwitchedMock.mockReset(), dirtyStateByAgent resets, and the window.matchMedia mock—into that helper, then call setupAppTestEnvironment() from this describe's beforeEach to consolidate setup and avoid drift.server/lib/telemetry/types.ts (1)
33-41: Optional: deriveemptyFeaturesUsed24hfromTELEMETRY_FEATURE_NAMES.The hand-listed object is a duplicate source of truth with
TELEMETRY_FEATURE_NAMES. Deriving it keeps the two in lock-step automatically:Proposed refactor
-export function emptyFeaturesUsed24h(): FeaturesUsed24h { - return { - chat: false, - sessions: false, - branches: false, - kanban: false, - settings: false, - }; -} +export function emptyFeaturesUsed24h(): FeaturesUsed24h { + return Object.fromEntries( + TELEMETRY_FEATURE_NAMES.map((name) => [name, false]), + ) as FeaturesUsed24h; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/types.ts` around lines 33 - 41, The function emptyFeaturesUsed24h currently returns a hard-coded FeaturesUsed24h object duplicating TELEMETRY_FEATURE_NAMES; change emptyFeaturesUsed24h to build and return the object by iterating over TELEMETRY_FEATURE_NAMES (e.g., reduce/map) and setting each feature key to false so the shape is derived from TELEMETRY_FEATURE_NAMES and stays in sync with it while still satisfying the FeaturesUsed24h return type.server/routes/voice-phrases.test.ts (1)
98-110: Optional: also assert no telemetry on body-validation 400s.The route has a second pre-telemetry exit point (missing
stopPhrases/cancelPhrases/wakePhrasesand JSON parse errors). Adding a case that hits a 400 via invalid body would lock in the "telemetry only on success" contract more completely.Suggested additional test
+ it('emits no settings telemetry when the body has no phrases', async () => { + mockDeps(); + const app = await buildApp(); + + const res = await app.request('/api/voice-phrases/de', { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({}), + }); + + expect(res.status).toBe(400); + expect(telemetryRuntimeMock.markFeatureUsed).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/voice-phrases.test.ts` around lines 98 - 110, Add a new test in server/routes/voice-phrases.test.ts that exercises the alternate 400 pre-telemetry exit by sending a PUT to '/api/voice-phrases/xx' with an invalid body (e.g., JSON missing stopPhrases/cancelPhrases/wakePhrases or malformed JSON) and assert the response status is 400 and telemetryRuntimeMock.markFeatureUsed was not called; place it similarly to the existing "emits no settings telemetry when voice phrase updates fail" test and reuse buildApp() / mockDeps() setup so the test targets the JSON/body-validation branch rather than the update-failure branch.server/routes/telemetry.ts (1)
34-61: Line 57 is unreachable — the union has only three handled events.
uiTelemetryEventSchemais a union ofsession_opened,branch_created,branch_switched, all handled with early returns above.await telemetry.recordClientDetailedEvent(payload)on line 57 cannot execute. It can be safely removed (or left as a defensive// exhaustiveness checkwith anevercast if you want compile-time enforcement).♻️ Suggested cleanup
if (payload.event === 'branch_switched') { await Promise.allSettled([ telemetry.markFeatureUsed('branches'), telemetry.recordClientDetailedEvent(payload), ]); return; } - - await telemetry.recordClientDetailedEvent(payload); } catch { return; }Or, for exhaustiveness:
- await telemetry.recordClientDetailedEvent(payload); + const _exhaustive: never = payload; + void _exhaustive;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/telemetry.ts` around lines 34 - 61, In recordUiTelemetryEvent, the final await telemetry.recordClientDetailedEvent(payload) is unreachable because uiTelemetryEventSchema only includes 'session_opened', 'branch_created', and 'branch_switched', each handled with early returns; remove that unreachable call or replace it with an explicit exhaustiveness check (cast payload to never and/or throw in the default branch) to satisfy compile-time guarantees while keeping the handled branches (markFeatureUsed and recordClientDetailedEvent for branch_switched) intact.src/features/telemetry/TelemetryNotice.tsx (2)
40-69: Optional: add ARIA semantics to the dismissible notice.The wrapping
<div>has no role/landmark, so screen readers will not announce the telemetry disclosure when it appears. Considerrole="status"(polite) so assistive tech surfaces the notice without interrupting other interactions.- <div className="fixed left-1/2 top-28 z-40 flex max-w-[min(calc(100vw-1.067rem),48rem)] -translate-x-1/2 items-start gap-3 rounded-2xl border border-orange/25 bg-card/94 px-4 py-3 text-xs text-foreground shadow-[0_20px_48px_rgba(0,0,0,0.28)] backdrop-blur-xl"> + <div role="status" aria-live="polite" className="fixed left-1/2 top-28 z-40 flex max-w-[min(calc(100vw-1.067rem),48rem)] -translate-x-1/2 items-start gap-3 rounded-2xl border border-orange/25 bg-card/94 px-4 py-3 text-xs text-foreground shadow-[0_20px_48px_rgba(0,0,0,0.28)] backdrop-blur-xl">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/telemetry/TelemetryNotice.tsx` around lines 40 - 69, The telemetry notice div in TelemetryNotice.tsx is missing ARIA semantics so screen readers won't announce it; update the outer wrapper element (the div containing the AlertTriangle, message, and the dismiss button tied to handleDismiss) to include role="status" (polite) so assistive tech will surface the disclosure when it appears—keep the existing aria-label on the dismiss button and do not change handleDismiss, mode, or publicDocUrl.
5-10: Mode prop is wider than the rendered copy supports.The component accepts
mode: 'off' | 'minimal' | 'detailed'and interpolates it into the heading on line 45, but line 47 hardcodes a description that only applies tominimal. If this notice is ever rendered withmode='detailed', the user sees a contradiction ("...using detailed telemetry. Minimal telemetry sends heartbeat snapshots...").Per
docs/TELEMETRY.md(line 404), the fresh-install notice is only shown when the effective mode isminimal, so the prop type is wider than reality. Either narrow the prop or make the description mode-dependent.♻️ Suggested fix — narrow the prop
interface TelemetryNoticeProps { visible: boolean; - mode: 'off' | 'minimal' | 'detailed'; + mode: 'minimal'; publicDocUrl: string; noticeId?: string; }Also applies to: 45-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/telemetry/TelemetryNotice.tsx` around lines 5 - 10, The TelemetryNoticeProps.mode type is too broad for the rendered copy; update TelemetryNoticeProps (and any callers) to restrict mode to 'off' | 'minimal' (removing 'detailed') so the interpolated heading and hardcoded description in TelemetryNotice remain consistent, or alternatively make the description text in the TelemetryNotice component conditional on the mode (use the mode value to render a detailed-specific message when mode === 'detailed'); adjust TelemetryNoticeProps and the TelemetryNotice component (and tests/call sites) accordingly so types and rendered copy match.docs/TELEMETRY.md (1)
421-425: Awkward structure under "Retention policy".Line 423 ends with a colon ("Current public retention targets are:") which sets up a list, but line 425 follows with an explanatory paragraph rather than the list itself. Consider reordering so the disclaimer precedes the lead-in, or dropping the colon.
📝 Suggested restructure
## Retention policy -Current public retention targets are: - -These are service-side retention targets for Nerve-operated telemetry backends. The app does not enforce remote retention from the client side. +These are service-side retention targets for Nerve-operated telemetry backends. The app does not enforce remote retention from the client side. + +Current public retention targets are:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TELEMETRY.md` around lines 421 - 425, Under the "Retention policy" section, fix the awkward flow by moving the explanatory disclaimer ("These are service-side retention targets for Nerve-operated telemetry backends. The app does not enforce remote retention from the client side.") to appear before the lead-in line "Current public retention targets are:" or by removing the trailing colon from that lead-in so it doesn't imply a list; update the sentence order around the "Retention policy" header to make the disclaimer precede or rephrase the lead-in for clarity.install.sh (1)
134-158: Optional: extract the inlinetsx --evalscript to a dedicated file.Embedding TypeScript via
--evalin a heredoc-like single-quoted string makes the install path harder to lint, type-check, and edit (no tsserver/eslint coverage). A small wrapper atscripts/lib/write-env.tsinvoked as./node_modules/.bin/tsx scripts/lib/write-env.tswould behave the same and stay maintainable alongsideenv-writer.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 134 - 158, The inline TS eval in write_env_with_writer should be moved into a small script file to improve linting and maintainability: create scripts/lib/write-env.ts that imports writeEnvFile from "./env-writer.ts", reads process.env.NERVE_SETUP_* into the same config object (GATEWAY_URL, GATEWAY_TOKEN, PORT, optional NERVE_TELEMETRY_MODE), and calls writeEnvFile(".env", config); then update write_env_with_writer to invoke ./node_modules/.bin/tsx scripts/lib/write-env.ts with the same environment variables (NERVE_SETUP_GATEWAY_URL, NERVE_SETUP_GATEWAY_TOKEN, NERVE_SETUP_PORT, NERVE_SETUP_TELEMETRY_MODE) instead of using --eval.server/lib/ws-proxy.test.ts (1)
1053-1120: Real-timesetTimeout(1100)adds latency and bucket-boundary flakiness risk.The 1.1s sleep reliably lands in the
1_5sduration bucket on most machines, but a slow CI runner or GC pause near the 1s/5s boundary could push elapsed time outside the expected bucket. Consider injecting a clock seam (e.g.,Date.nowspy orvi.useFakeTimers()if your bucketing function is timer-driven) so the assertion is deterministic and the suite stays fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/ws-proxy.test.ts` around lines 1053 - 1120, The test 'emits tool_call_completed with coerced tool family and duration bucket on tool result' uses a real setTimeout(1100) which is flaky and slow; replace the real sleep with a controllable clock: either call vi.useFakeTimers() (or spy Date.now) before sending the 'start' event, trigger/mock advancing time to the desired elapsed interval that maps to the '1_5s' bucket, then restore timers (vi.useRealTimers()) after the assertions; ensure the code paths that compute duration (the telemetryRuntimeMock.recordToolCompleted invocation or the bucketing function) observe the mocked time by referencing Date.now or the fake timer so the assertion for payload.properties.duration_bucket === '1_5s' is deterministic.server/lib/ws-proxy.ts (1)
318-326: Implementation is correct. The test at lines 1185–1231 validates thatflushPendingToolsForRuncorrectly marks remaining pending tools as failed whenlifecycle.endis received without a precedingtool.result. By the timelifecycle.endis processed, alltool.resultframes that arrived before it have already been removed frompendingTools, so only genuinely incomplete tools remain.This relies on the gateway protocol guarantee that
tool.resultis always sent beforelifecycle.endfor successful tools. Consider adding a brief comment influshPendingToolsForRundocumenting this assumption for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/ws-proxy.ts` around lines 318 - 326, Add a brief comment in the flushPendingToolsForRun function documenting the protocol assumption it relies on: that successful tool executions will always have a preceding tool.result frame removed from pendingTools before lifecycle.end arrives, so this function only marks genuinely incomplete tools as failed; reference pendingTools and recordToolCompleted in the comment to clarify why the loop only handles remaining entries.scripts/lib/setup-telemetry.ts (1)
1-3: Optional: normalize input casing/whitespace.
resolveSetupInstallMethodis fedprocess.env.NERVE_SETUP_INSTALL_METHODdirectly, so values like'release 'or'Release'silently downgrade to'source'. If install scripts ever set this with stray whitespace or different casing, the telemetry stamp would be wrong. Consider trimming/lowercasing before comparison.♻️ Suggested change
export function resolveSetupInstallMethod(value: string | undefined | null): 'release' | 'source' { - return value === 'release' ? 'release' : 'source'; + return value?.trim().toLowerCase() === 'release' ? 'release' : 'source'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/setup-telemetry.ts` around lines 1 - 3, The function resolveSetupInstallMethod incorrectly treats values like "Release" or "release " as 'source'; normalize the input by trimming whitespace and lowercasing before comparison so process.env.NERVE_SETUP_INSTALL_METHOD is handled robustly. Update resolveSetupInstallMethod to guard for null/undefined, compute a normalized string (e.g., normalized = value?.trim().toLowerCase() ?? ''), then return 'release' if normalized === 'release' else 'source'. Ensure you reference resolveSetupInstallMethod when making the change so all callers receive the normalized behavior.server/lib/telemetry/heartbeat.ts (1)
58-68: Optional: clarify thatjitterMsis the exact offset, not a max.
nextDailyHeartbeatAtusesjitterMsdirectly as the offset from UTC midnight. The name "jitter" usually implies the function randomizes within[0, jitterMs]. Since callers (the runtime) are responsible for picking the per-instance random offset, a one-line JSDoc would prevent the natural misreading and avoid two callers ever passing the maximum value verbatim and producing a synchronized heartbeat thundering herd.♻️ Suggested doc
+/** + * Returns the next daily heartbeat instant, anchored at UTC midnight + `jitterMs`. + * `jitterMs` is the exact offset to apply (e.g., a per-instance random value picked + * once at startup), not an upper bound — callers are responsible for randomization. + */ export function nextDailyHeartbeatAt(now: Date, jitterMs: number): Date {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/heartbeat.ts` around lines 58 - 68, Add a one-line JSDoc to nextDailyHeartbeatAt clarifying that the jitterMs parameter is an exact per-instance offset in milliseconds from UTC midnight (not a maximum or range); update the function comment above nextDailyHeartbeatAt to state callers must choose a randomized offset themselves (e.g., "jitterMs is an exact offset in ms from UTC midnight; callers should randomize per-instance"), so future readers won't assume jitterMs is a max bound to sample from.server/lib/telemetry/detailed-events.ts (1)
177-177: Redundant literal in thesession_opsbranch.
'sessions_yield'already satisfies.startsWith('session'), so the explicit equality check never adds a match. Drop the redundancy:- if (normalized === 'sessions_yield' || normalized.startsWith('session')) return 'session_ops'; + if (normalized.startsWith('session')) return 'session_ops';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/detailed-events.ts` at line 177, The condition that maps to 'session_ops' is redundant because "sessions_yield" already matches startsWith('session'); update the branch that currently reads "if (normalized === 'sessions_yield' || normalized.startsWith('session')) return 'session_ops';" to a single check using normalized.startsWith('session') (referencing the normalized variable and the branch that returns 'session_ops') so the equality clause is removed.server/lib/telemetry/http.ts (1)
28-66: Add exponential backoff between retry attempts to avoid amplifying load during outages.The retry loop issues up to
maxRetries+1requests with zero delay between them. During a 5xx burst or transientfetchfailure, telemetry volume is low but every app instance hammers the endpoint in sync. A small exponential backoff with jitter (e.g., 250ms × 2^attempt) preserves the best-effort semantics while avoiding this thundering-herd pattern.♻️ Sketch of backoff implementation
+function backoffMs(attempt: number): number { + const base = 250 * Math.pow(2, attempt); + return base + Math.floor(Math.random() * base); +} + return { async postJson(path, body) { try { const serialized = JSON.stringify(body); if (Buffer.byteLength(serialized, 'utf8') > maxRequestBytes) { return false; } for (let attempt = 0; attempt <= maxRetries; attempt += 1) { try { const response = await fetchImpl(url, { /* … */ }); if (response.ok) return true; if (!shouldRetry(response.status) || attempt === maxRetries) return false; } catch { if (attempt === maxRetries) return false; } + if (attempt < maxRetries) await new Promise((r) => setTimeout(r, backoffMs(attempt))); } } catch { return false; } }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/http.ts` around lines 28 - 66, The retry loop in postJson should sleep with an exponential backoff + jitter between attempts to avoid synchronized retries; after a failed attempt (inside the for loop where you currently decide to retry based on shouldRetry(response.status) or in the catch block) compute a delay like baseDelayMs * 2**attempt plus a small random jitter, await that delay (e.g., await new Promise(r => setTimeout(r, delay))) before the next fetch, and ensure you do not wait after the final attempt (use maxRetries and attempt checks). Update the postJson function to reference existing symbols (postJson, maxRetries, attempt, shouldRetry, fetchImpl, timeoutMs, maxRequestBytes, baseUrl) and keep the first attempt immediate while applying backoff only when about to retry.server/lib/telemetry/install-metadata.ts (1)
28-46: Optional: extractPROJECT_ROOT/telemetryDir()resolution into a shared helper.The same
PROJECT_ROOT = process.env.NERVE_PROJECT_ROOT || path.resolve(__dirname, '..', '..', '..')andtelemetryDir = process.env.NERVE_TELEMETRY_DIR || path.join(PROJECT_ROOT, '.nerve', 'telemetry')pattern is duplicated inserver/lib/telemetry/store.ts(Line 16 and Line 71). Two minor concerns:
- Duplication — divergence risk if one is ever updated without the other.
- Caching mismatch —
PROJECT_ROOTis captured at module load, whileNERVE_TELEMETRY_DIRis read on everytelemetryDir()call. MutatingNERVE_PROJECT_ROOTafter import (e.g., in tests) won't take effect, whileNERVE_TELEMETRY_DIRwill.A small
telemetryPaths.tshelper that reads both env vars on each call would unify behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/install-metadata.ts` around lines 28 - 46, Duplicate environment-resolution logic and inconsistent caching between the module-level PROJECT_ROOT and the telemetryDir() function causes divergence; extract this into a shared helper (e.g., telemetryPaths.ts) that exposes functions getProjectRoot() and telemetryDir() which read process.env.NERVE_PROJECT_ROOT and process.env.NERVE_TELEMETRY_DIR on each call, then update this file to remove the module-level PROJECT_ROOT constant and have telemetryDir(), telemetryPath(), and ensureTelemetryDir() call the shared helper functions; also update server/lib/telemetry/store.ts to import and use the same helper so both modules resolve env vars consistently at runtime.server/lib/telemetry/store.ts (2)
270-277: Nit: doublepruneStateis defensive but redundant for current callers.
pruneStateis invoked once before the mutator and once after. The current mutators only pushnowIso(which is by definition in-window), so the second prune is a no-op. Harmless, but a comment explaining the intent (e.g., "second prune protects against mutators that could reintroduce out-of-window timestamps") would prevent future readers from "simplifying" it away.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/store.ts` around lines 270 - 277, The update function calls pruneState twice (once on the read state and again after applying mutator) as a defensive measure; add a brief inline comment above the second pruneState(nextState, nowIso) explaining that the second prune protects against mutators that might reintroduce out-of-window timestamps so future readers don't remove the seemingly redundant call. Reference the update function, the mutator parameter, nowIso (from resolveAt), pruneState, readState and writeState in the comment to make intent clear.
256-262: Optional: makewriteStateatomic to avoid silent state loss on crash.
fs.writeFileis not atomic — a process crash mid-write leaves a truncated/invalid JSON, whichreadState(Lines 247-254) silently swallows and resets to defaults. That resetsheartbeats.lastSentAtByReasonandlastAppVersion, which can cause a spuriousfirst_seenheartbeat on next startup (pershouldSendFirstSeenlogic in the runtime).A standard temp-file + rename pattern fixes this:
♻️ Suggested fix
async function writeState(state: Phase1State): Promise<void> { await fs.mkdir(path.dirname(stateFile), { recursive: true, mode: 0o700 }); - await fs.writeFile(stateFile, JSON.stringify(state, null, 2) + '\n', { - encoding: 'utf8', - mode: 0o600, - }); + const tmpFile = `${stateFile}.${process.pid}.tmp`; + await fs.writeFile(tmpFile, JSON.stringify(state, null, 2) + '\n', { + encoding: 'utf8', + mode: 0o600, + }); + await fs.rename(tmpFile, stateFile); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/store.ts` around lines 256 - 262, writeState currently writes stateFile directly which can leave a truncated JSON on crash; change writeState to perform an atomic write: create the parent dir as before, write the JSON to a temporary file in the same directory (e.g., `${stateFile}.tmp-${process.pid}`) with mode 0o600, fsync the temp file (ensure data is flushed), then fs.rename(tempPath, stateFile) to atomically replace the file; optionally fsync the parent directory after rename. Keep the function signature writeState(state: Phase1State): Promise<void>, and ensure readState continues to read stateFile unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7656e80-d018-420c-8c28-2a99d3a1c3dd
📒 Files selected for processing (59)
.env.example.gitignoreREADME.mddocs/CONFIGURATION.mddocs/INSTALL.mddocs/TELEMETRY.mddocs/UPDATING.mdinstall.shscripts/lib/env-writer.tsscripts/lib/setup-telemetry.test.tsscripts/lib/setup-telemetry.tsscripts/lib/telemetry-stamp.mjsscripts/lib/telemetry-stamp.test.tsscripts/setup.tsserver/app.tsserver/index.tsserver/lib/config.test.tsserver/lib/config.tsserver/lib/telemetry/detailed-events.test.tsserver/lib/telemetry/detailed-events.tsserver/lib/telemetry/error-reporting.test.tsserver/lib/telemetry/error-reporting.tsserver/lib/telemetry/heartbeat.test.tsserver/lib/telemetry/heartbeat.tsserver/lib/telemetry/http.test.tsserver/lib/telemetry/http.tsserver/lib/telemetry/install-metadata.test.tsserver/lib/telemetry/install-metadata.tsserver/lib/telemetry/runtime.test.tsserver/lib/telemetry/runtime.tsserver/lib/telemetry/store.test.tsserver/lib/telemetry/store.tsserver/lib/telemetry/types.tsserver/lib/ws-proxy.test.tsserver/lib/ws-proxy.tsserver/middleware/error-handler.test.tsserver/middleware/error-handler.tsserver/routes/api-keys.test.tsserver/routes/api-keys.tsserver/routes/kanban.test.tsserver/routes/kanban.tsserver/routes/server-info.test.tsserver/routes/server-info.tsserver/routes/sessions.test.tsserver/routes/sessions.tsserver/routes/telemetry.test.tsserver/routes/telemetry.tsserver/routes/transcribe.test.tsserver/routes/transcribe.tsserver/routes/voice-phrases.test.tsserver/routes/voice-phrases.tssrc/App.test.tsxsrc/App.tsxsrc/contexts/SessionContext.test.tsxsrc/contexts/SessionContext.tsxsrc/features/telemetry/TelemetryNotice.test.tsxsrc/features/telemetry/TelemetryNotice.tsxsrc/features/telemetry/telemetryClient.tssrc/features/telemetry/telemetryNoticeStorage.ts
| it('warns and exits successfully when install-method metadata cannot be written', () => { | ||
| const readOnlyDir = makeReadOnlyDir('readonly-install-method'); | ||
|
|
||
| const result = runStampResult(readOnlyDir, 'install-method', 'release', '--source', 'install.sh'); | ||
|
|
||
| expect(result.status).toBe(0); | ||
| expect(readStampFromDir(readOnlyDir, 'install-method')).toBeUndefined(); | ||
| expect(result.stderr).toContain('Failed to write install-method.json'); | ||
| }); |
There was a problem hiding this comment.
Read-only directory tests may flake when run as root.
fs.chmodSync(dir, 0o500) does not prevent writes when the test process runs as uid 0 (some CI/docker containers). In that case, the stamp script will succeed in writing, the file will exist, and these assertions will fail. Consider either gating these cases on process.getuid?.() !== 0 or asserting against a mocked fs.writeFileSync to keep the test environment-independent.
🛡️ Optional guard
+ if (process.getuid?.() === 0) {
+ // chmod 0o500 doesn't restrict root; skip filesystem-permission scenario.
+ return;
+ }
const readOnlyDir = makeReadOnlyDir('readonly-install-method');Also applies to: 180-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/telemetry-stamp.test.ts` around lines 131 - 139, The read-only
directory tests (using makeReadOnlyDir, runStampResult, readStampFromDir) can
succeed when run as root because fs.chmodSync(..., 0o500) doesn't block uid 0;
guard these tests by skipping them when running as root: wrap the test (and the
similar one at lines ~180-188) with a check like if (process.getuid?.() === 0) {
test.skip(...) } or early-return/skip inside the it block so they only run when
process.getuid?.() !== 0; alternatively replace the filesystem-based write-test
with a mocked fs.writeFileSync to simulate a write failure (mocking fs in the
test that calls runStampResult) if you prefer to keep root-friendly behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
scripts/setup.ts (1)
369-381: Optional: extract the fresh-install telemetry finalization to one helper.The pre-write "default mode to
minimal" + post-writefinalizeSetupTelemetry(...)pair is duplicated betweenmain()andrunDefaults(). Consolidating keeps the two flows from drifting (e.g., one forgetting to seed the mode or call the stamp).♻️ Suggested shape
+function applyFreshInstallTelemetryDefaults(config: EnvConfig, isFreshInstall: boolean): void { + if (isFreshInstall && !config.NERVE_TELEMETRY_MODE) { + config.NERVE_TELEMETRY_MODE = 'minimal'; + } +}Then in both flows:
- if (isFreshInstall && !config.NERVE_TELEMETRY_MODE) { - config.NERVE_TELEMETRY_MODE = 'minimal'; - } + applyFreshInstallTelemetryDefaults(config, isFreshInstall); ... writeEnvFile(ENV_PATH, config); finalizeSetupTelemetry(isFreshInstall);Also applies to: 1291-1301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.ts` around lines 369 - 381, Duplicate telemetry finalization logic (setting NERVE_TELEMETRY_MODE to 'minimal' for fresh installs and calling finalizeSetupTelemetry) appears in both main() and runDefaults(); extract this into a single helper (e.g., seedAndFinalizeTelemetry or ensureTelemetryDefaults) and call it from both flows. The helper should accept the config object and isFreshInstall flag, set config.NERVE_TELEMETRY_MODE = 'minimal' when isFreshInstall and no NERVE_TELEMETRY_MODE is present, then call finalizeSetupTelemetry(isFreshInstall) after writeEnvFile is invoked by the caller; update calls in main() and runDefaults() to use this new helper and remove the duplicated pre-write/post-write lines around collectInteractive/writeEnvFile/finalizeSetupTelemetry.src/contexts/SessionContext.tsx (1)
916-927: Verify thatsession_openedemissions match "opened from the UI" semantics in the telemetry contract.Inside the root-spawn branch:
await refreshSessions()(Line 924) callspickDefaultSessionKey(...)which may shift the current session if the preferred key is no longer available.setCurrentSession(sessionKey)(Line 925) then emitssession_openedwhen the spawned key differs from the current one.void emitBranchCreated()(Line 926) fires last.The telemetry contract defines the
sessionsfeature boolean as marked when a session is "opened from the UI" (among other explicit user actions like rename/delete). However,session_openedcurrently emits wheneversetCurrentSessionchanges the key, including refresh-driven shifts frompickDefaultSessionKeyat Lines 725, 733, 770, 802, and the 30s polling interval (Line 839). These internal session selections are not user-driven and may not align with the "opened from the UI" trigger. Consider gatingsetCurrentSessionto emit only on explicit user navigation—for example, add an internalsetCurrentSessionSilentvariant for refresh paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/SessionContext.tsx` around lines 916 - 927, The current flow calls refreshSessions() which may change the selected session via pickDefaultSessionKey and then calls setCurrentSession(sessionKey), causing session_opened telemetry to fire for non-UI-driven shifts; create a silent setter (e.g., setCurrentSessionSilent) or add a boolean flag to setCurrentSession to suppress telemetry/emissions when invoked from internal flows, update refreshSessions(), the polling handler (the 30s interval), and any pickDefaultSessionKey paths to use the silent variant, and ensure only explicit UI actions (user navigation/rename/delete handlers that currently call setCurrentSession) call the telemetry-emitting setCurrentSession so session_opened reflects "opened from the UI" semantics; keep emitBranchCreated() behavior unchanged but call it only after the appropriate (silent vs normal) session selection as needed.server/lib/telemetry/runtime.ts (1)
372-455: Optional: extend thestoppedguard to record/emit paths for consistent shutdown semantics.Heartbeat paths now correctly check
stopped(Lines 237, 243, 311), butrecordSessionCreated,recordMessageSubmitted,recordToolCompleted,recordKanbanTaskCreated, andrecordClientDetailedEventonly gate ontelemetryEnabled()/detailedTelemetryEnabled(). If a caller invokes one of these afterstop()(e.g., a request finishing during shutdown, or in tests that swap runtimes), the store write and the backgroundsendDetailedEventHTTP post will still run. Same applies tomarkFeatureUsed,clearSessionSeen, andreportError.This is best-effort behavior so it isn't broken, but for clean shutdown / test isolation it'd be nicer to fold
stoppedinto the gate.♻️ Suggested approach
function detailedTelemetryEnabled(): boolean { - return mode === 'detailed' && !!instanceId; + return mode === 'detailed' && !!instanceId && !stopped; } @@ function telemetryEnabled(): boolean { - return mode !== 'off'; + return mode !== 'off' && !stopped; }
getServerInfoDisclosure()andgetMode()should keep usingmodedirectly (they're pure status accessors), so consider exposing a separate predicate or inlining the disclosure check rather than reusingtelemetryEnabled()there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/runtime.ts` around lines 372 - 455, The listed telemetry methods (recordSessionCreated, recordMessageSubmitted, recordToolCompleted, recordKanbanTaskCreated, recordClientDetailedEvent, and likewise markFeatureUsed, clearSessionSeen, reportError) should bail out early when the runtime has been stopped to avoid store writes and outbound sendDetailedEvent calls during shutdown/tests; add a pre-check like "if (stopped) return;" (or combine stopped into the existing gating) at the top of each of those functions before any runStoreWrite or sendDetailedEvent is invoked, leaving pure accessors like getServerInfoDisclosure() and getMode() unchanged.server/routes/transcribe.ts (1)
37-45: Consider centralizing telemetry helpers to reduce duplication.
runTelemetryInBackgroundandmarkSettingsFeatureUsedare duplicated across four route files:transcribe.ts,voice-phrases.ts,kanban.ts, andapi-keys.ts. Moving these into a shared module (e.g.,server/lib/telemetry/helpers.ts) would eliminate repetition and ensure the error-swallowing policy remains consistent across all instrumentation sites. This is a low-priority quality improvement that can be addressed in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/transcribe.ts` around lines 37 - 45, Centralize the duplicated telemetry helpers by extracting runTelemetryInBackground and markSettingsFeatureUsed into a shared module (e.g., server/lib/telemetry/helpers.ts); implement runTelemetryInBackground to accept a Promise and swallow errors (void work.catch(() => {})), implement markSettingsFeatureUsed to call getTelemetryRuntime() and, if present, call runTelemetryInBackground(telemetry.markFeatureUsed('settings')), then update the four route files (transcribe.ts, voice-phrases.ts, kanban.ts, api-keys.ts) to import and use these exported helpers instead of their local copies so the error-swallowing policy and telemetry usage (getTelemetryRuntime, markFeatureUsed) are consistent across all sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/setup.ts`:
- Around line 369-381: Duplicate telemetry finalization logic (setting
NERVE_TELEMETRY_MODE to 'minimal' for fresh installs and calling
finalizeSetupTelemetry) appears in both main() and runDefaults(); extract this
into a single helper (e.g., seedAndFinalizeTelemetry or ensureTelemetryDefaults)
and call it from both flows. The helper should accept the config object and
isFreshInstall flag, set config.NERVE_TELEMETRY_MODE = 'minimal' when
isFreshInstall and no NERVE_TELEMETRY_MODE is present, then call
finalizeSetupTelemetry(isFreshInstall) after writeEnvFile is invoked by the
caller; update calls in main() and runDefaults() to use this new helper and
remove the duplicated pre-write/post-write lines around
collectInteractive/writeEnvFile/finalizeSetupTelemetry.
In `@server/lib/telemetry/runtime.ts`:
- Around line 372-455: The listed telemetry methods (recordSessionCreated,
recordMessageSubmitted, recordToolCompleted, recordKanbanTaskCreated,
recordClientDetailedEvent, and likewise markFeatureUsed, clearSessionSeen,
reportError) should bail out early when the runtime has been stopped to avoid
store writes and outbound sendDetailedEvent calls during shutdown/tests; add a
pre-check like "if (stopped) return;" (or combine stopped into the existing
gating) at the top of each of those functions before any runStoreWrite or
sendDetailedEvent is invoked, leaving pure accessors like
getServerInfoDisclosure() and getMode() unchanged.
In `@server/routes/transcribe.ts`:
- Around line 37-45: Centralize the duplicated telemetry helpers by extracting
runTelemetryInBackground and markSettingsFeatureUsed into a shared module (e.g.,
server/lib/telemetry/helpers.ts); implement runTelemetryInBackground to accept a
Promise and swallow errors (void work.catch(() => {})), implement
markSettingsFeatureUsed to call getTelemetryRuntime() and, if present, call
runTelemetryInBackground(telemetry.markFeatureUsed('settings')), then update the
four route files (transcribe.ts, voice-phrases.ts, kanban.ts, api-keys.ts) to
import and use these exported helpers instead of their local copies so the
error-swallowing policy and telemetry usage (getTelemetryRuntime,
markFeatureUsed) are consistent across all sites.
In `@src/contexts/SessionContext.tsx`:
- Around line 916-927: The current flow calls refreshSessions() which may change
the selected session via pickDefaultSessionKey and then calls
setCurrentSession(sessionKey), causing session_opened telemetry to fire for
non-UI-driven shifts; create a silent setter (e.g., setCurrentSessionSilent) or
add a boolean flag to setCurrentSession to suppress telemetry/emissions when
invoked from internal flows, update refreshSessions(), the polling handler (the
30s interval), and any pickDefaultSessionKey paths to use the silent variant,
and ensure only explicit UI actions (user navigation/rename/delete handlers that
currently call setCurrentSession) call the telemetry-emitting setCurrentSession
so session_opened reflects "opened from the UI" semantics; keep
emitBranchCreated() behavior unchanged but call it only after the appropriate
(silent vs normal) session selection as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24bb71cb-0ccd-4b28-ab73-aef26e536da9
📒 Files selected for processing (7)
scripts/setup.tsserver/lib/telemetry/runtime.test.tsserver/lib/telemetry/runtime.tsserver/routes/transcribe.test.tsserver/routes/transcribe.tssrc/contexts/SessionContext.test.tsxsrc/contexts/SessionContext.tsx
✅ Files skipped from review due to trivial changes (1)
- server/lib/telemetry/runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/contexts/SessionContext.test.tsx
- server/routes/transcribe.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a434ba2b5b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function flushPendingToolsForRun(runId: string | undefined, success: boolean): void { | ||
| if (!runId) return; |
There was a problem hiding this comment.
flushPendingToolsForRun returns immediately when runId is absent, but tool starts are still tracked with runId: undefined. If the gateway emits a tool start event without a runId and that tool never reaches a result (for example a failed/aborted run), the pending entry is never flushed, so the failed tool call is never recorded and pendingTools can grow for long-lived sockets. This undercounts tool_calls and biases success telemetry in environments where run IDs are not consistently populated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec76f89568
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (payload.stream === 'tool' && data?.phase === 'start' && typeof data.toolCallId === 'string' && typeof data.name === 'string') { | ||
| pendingTools.set(data.toolCallId, { | ||
| runId, | ||
| toolName: data.name, |
There was a problem hiding this comment.
This code starts tracking tool calls in pendingTools, but those entries are only flushed on tool/result or lifecycle/chat error frames. If the browser or gateway socket closes before those frames arrive (tab close, network drop, server shutdown), the pending entries are discarded with no recordToolCompleted call, so aborted tool calls are silently undercounted and success telemetry is biased upward for long-lived sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58c4a1ccbe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }).then((didSwitch) => { | ||
| if (didSwitch) { | ||
| maybeEmitBranchSwitchTelemetry(previousSessionKey, key); | ||
| } | ||
| }); |
There was a problem hiding this comment.
branch_switched is only emitted from the explicit select/spawn handlers here, but SessionContext.deleteSession can also switch the active workspace root by calling setCurrentSession after deleting the current root. That delete-driven root switch bypasses this telemetry path, so branch-switch activity is undercounted (and detailed-mode branch_switched events are missed) whenever users switch roots by deleting the active session.
Useful? React with 👍 / 👎.
| function flushPendingToolsForRun(runId: string | undefined, success: boolean): void { | ||
| flushPendingTools(success, (entry) => ( | ||
| runId ? entry.runId === runId : entry.runId === undefined | ||
| )); |
There was a problem hiding this comment.
When a lifecycle/chat failure frame arrives without runId, this predicate only flushes entries whose entry.runId is also undefined. The client already treats run IDs as optional and falls back to an active run when missing, so runId-less failure frames are expected; if tool starts were recorded with a runId, they remain pending and their failed tool_call_completed telemetry is skipped until teardown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/lib/telemetry/install-metadata.test.ts (1)
103-112: Optional: tighten the UUID assertion.
crypto.randomUUID()always returns a lowercase v4 UUID, so a stricter regex would catch a regression where some other id-shape sneaks in (the current pattern accepts any 36-char hex/dash sequence, including obvious garbage like 36 dashes).♻️ Proposed assertion
- expect(first).toMatch(/^[0-9a-f-]{36}$/i); + expect(first).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/install-metadata.test.ts` around lines 103 - 112, The UUID assertion in the test for ensureInstanceId is too permissive; replace the loose regex /^[0-9a-f-]{36}$/i with a strict lowercase v4 UUID pattern such as /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ to ensure ensureInstanceId() (and the persisted identity.json) returns a lowercase RFC4122 version 4 UUID; update the expect(first).toMatch(...) line accordingly and keep the stored.instanceId equality check.server/lib/telemetry/install-metadata.ts (1)
60-70: Optional: make metadata writes atomic.
writeFileSyncdirectly to the target path can leave a half-writtenidentity.json/bootstrap.json/install-method.jsonif the process is killed mid-write, which would then read back asundefinedand (for identity) silently regenerate a new instance id on the next start. Best-effort telemetry tolerates this, but a tmp-write-then-rename keeps the on-disk file always-valid for free.♻️ Proposed atomic write
function writeJsonFile(fileName: string, value: unknown): void { try { ensureTelemetryDir(); - fs.writeFileSync(telemetryPath(fileName), JSON.stringify(value, null, 2) + '\n', { + const target = telemetryPath(fileName); + const tmp = `${target}.${process.pid}.${Date.now()}.tmp`; + fs.writeFileSync(tmp, JSON.stringify(value, null, 2) + '\n', { encoding: 'utf8', mode: 0o600, }); + fs.renameSync(tmp, target); } catch (error) { console.warn(`[telemetry] Failed to write ${fileName}:`, telemetryWriteErrorMessage(error)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/telemetry/install-metadata.ts` around lines 60 - 70, writeJsonFile currently writes directly to the final path which can leave a partially-written file if killed; change it to write atomically by writing the JSON to a temp file in the same directory (e.g. telemetryPath(`${fileName}.tmp-${process.pid}`)), ensure same encoding and mode when writing, fsync/close the temp file if using lower-level ops (or use writeFileSync then fsync via open/write/fsync/close), then rename the temp file to the final path with fs.renameSync; keep existing error handling using telemetryWriteErrorMessage and ensure ensureTelemetryDir() is still called before writing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/telemetry/install-metadata.ts`:
- Around line 119-126: ensureInstanceId currently returns a newly generated UUID
even when writeIdentity fails, causing the instance id to change across calls if
the identity file cannot be written; update ensureInstanceId to cache the
generated instanceId in a module-level variable (e.g., a private let
cachedInstanceId) and return that cached value on subsequent calls if
writeIdentity throws or the file remains unreadable, and also add a concise
comment on the degradation behavior; reference the ensureInstanceId,
readIdentity, and writeIdentity symbols when locating the change.
---
Nitpick comments:
In `@server/lib/telemetry/install-metadata.test.ts`:
- Around line 103-112: The UUID assertion in the test for ensureInstanceId is
too permissive; replace the loose regex /^[0-9a-f-]{36}$/i with a strict
lowercase v4 UUID pattern such as
/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ to
ensure ensureInstanceId() (and the persisted identity.json) returns a lowercase
RFC4122 version 4 UUID; update the expect(first).toMatch(...) line accordingly
and keep the stored.instanceId equality check.
In `@server/lib/telemetry/install-metadata.ts`:
- Around line 60-70: writeJsonFile currently writes directly to the final path
which can leave a partially-written file if killed; change it to write
atomically by writing the JSON to a temp file in the same directory (e.g.
telemetryPath(`${fileName}.tmp-${process.pid}`)), ensure same encoding and mode
when writing, fsync/close the temp file if using lower-level ops (or use
writeFileSync then fsync via open/write/fsync/close), then rename the temp file
to the final path with fs.renameSync; keep existing error handling using
telemetryWriteErrorMessage and ensure ensureTelemetryDir() is still called
before writing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3de20c6f-8461-46db-877b-3e895e89df8c
📒 Files selected for processing (2)
server/lib/telemetry/install-metadata.test.tsserver/lib/telemetry/install-metadata.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19f16fb028
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const runId = typeof payload.runId === 'string' ? payload.runId : undefined; | ||
| const data = isRecord(payload.data) ? payload.data : null; | ||
| if (payload.stream === 'tool' && data?.phase === 'start' && typeof data.toolCallId === 'string' && typeof data.name === 'string') { | ||
| pendingTools.set(data.toolCallId, { |
There was a problem hiding this comment.
pendingTools is keyed only by toolCallId, so a later tool.start with the same ID overwrites an earlier pending entry before its result arrives. On multiplexed sockets (multiple sessions/runs) or gateways that scope tool IDs per run, this misattributes completions and can drop one tool completion entirely, skewing tool_calls and success/failure telemetry. Use a run-aware key (for example runId + toolCallId) or a nested map keyed by run first to avoid collisions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aadbce55a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (runId) { | ||
| flushPendingTools(false, (entry) => entry.runId === runId); | ||
| return; |
There was a problem hiding this comment.
When a tool.start frame is recorded without runId, the pending entry is stored with entry.runId === undefined, but flushPendingToolsForFailure exits early whenever a later lifecycle/chat failure includes a runId. In that mixed-ID case, the failed tool is never flushed because only entry.runId === runId is checked, so tool_call_completed is skipped and the pending map can retain stale entries until teardown. Please fall back to a session-scoped (or all) flush when a run-scoped flush finds no matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31de5bcec0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function getPendingToolKey(runId: string | undefined, toolCallId: string): string { | ||
| return `${runId ?? ''}\u0000${toolCallId}`; | ||
| } |
There was a problem hiding this comment.
When runId is absent, getPendingToolKey collapses every pending start to "\0" + toolCallId, so a second tool.start with the same toolCallId overwrites the first entry before either result arrives. This still loses or misattributes tool_call_completed telemetry in mixed/legacy gateways that omit run IDs and only scope tool IDs per session or run. Include sessionKey (or another stable discriminator) in the runless key so concurrent runless starts cannot clobber each other.
Useful? React with 👍 / 👎.
Applied a batch of fixes from a multi-agent code review of this PR. Touched
the agreed-on items across 9 files plus a new barrel.
safe_auto fixes:
- .env.example: document NERVE_TELEMETRY_MODE and NERVE_TELEMETRY_DIR
alongside the URL overrides (CONTRIBUTING.md requires new env vars to
be discoverable here; both are documented in docs/CONFIGURATION.md but
were absent from the env template). [3-way: project-standards,
previous-comments, agent-native]
- server/routes/telemetry.ts: remove the dead unreachable
recordClientDetailedEvent call after the discriminated-union branches;
replace with a never-exhaustiveness check so future event types break
the build instead of silently falling through. [3-way: maintainability,
previous-comments, kieran-typescript]
- server/routes/telemetry.ts: void the recordUiTelemetryEvent call so the
POST /api/telemetry/events handler returns 200 without awaiting the
store-write chain. Matches the runInBackground pattern in
ws-proxy.ts and honours the PR's non-blocking-telemetry claim.
[2-way: performance, reliability]
- server/routes/telemetry.ts: return JSON error envelopes for the 400
and 503 responses instead of plain text so the new endpoints match the
/api/* error contract used everywhere else. [api-contract]
- src/App.tsx: add the missing `if (!fromRootSessionKey) return;` guard
to maybeEmitBranchSwitchTelemetry. First session pick after a fresh
start now does not emit a spurious branch_switched (the original
test that asserted the buggy behavior is flipped). [2-way:
previous-comments, correctness]
- src/App.tsx: attach a .catch to the handleSessionChange .then chain
so a rejected requestWorkspaceTransition no longer escapes as an
unhandled promise rejection. [previous-comments]
- src/features/telemetry/index.ts: new barrel exporting TelemetryNotice,
emitSessionOpened, emitBranchCreated, emitBranchSwitched, and
buildTelemetryNoticeDismissKey, per the CONTRIBUTING.md frontend
convention. [project-standards]
- server/lib/telemetry/detailed-events.ts: bucketDurationMs upper bound
changed from `<= 30_000` to `< 30_000` so all three boundaries use
strict less-than. [previous-comments]
- server/lib/ws-proxy.ts: getPendingToolKey now includes sessionKey in
the runless variant so the same toolCallId from different sessions
no longer collides on a multiplexed connection. [previous-comments,
Codex 2026-05-22]
gated_auto fixes:
- server/lib/telemetry/store.ts: cap seenSessionHashes at 500 entries
inside markSessionSeen, trimming oldest first. Phase 1 state no
longer grows the array unbounded over the lifetime of a long-running
install. [3-way: performance, reliability, previous-comments]
- server/lib/telemetry/store.ts: writeState now writes to a tmp file
then renames into place. A crash or disk-full mid-write no longer
corrupts the file and triggers a 24h-counter reset on next start.
[2-way: reliability, adversarial]
- server/lib/telemetry/install-metadata.ts: writeJsonFile uses the
same tmp+rename pattern for identity.json, install-method.json,
and bootstrap.json so a partial write cannot rotate the anonymous
instance_id on next start. [2-way: reliability, adversarial]
- server/lib/telemetry/install-metadata.ts: readJsonFile returns
unknown instead of T; callers now validate the shape via asRecord
before reading fields. [kieran-typescript]
- server/lib/telemetry/http.ts: HTTP retry loop now sleeps with
exponential backoff + jitter between attempts (500ms base, 250ms
jitter ceiling). Also clamps maxRetries to >= 0 so a caller passing
-1 no longer produces a silent no-op postJson. [reliability,
correctness-7]
Validation:
- tsc clean on config/tsconfig.{server,app}.json.
- npx vitest scripts/lib/access-plan, server/lib/telemetry, server/routes/telemetry,
server/lib/ws-proxy, server/middleware/error-handler, src/App,
src/contexts/SessionContext, src/features/telemetry: 159/159 pass.
- Full suite: 1979/1988 pass. The 9 failures are pre-existing baseline
noise in files this PR does not touch (server/routes/files.test.ts,
server/routes/upload-reference.test.ts, server/lib/beads.test.ts,
server/lib/upload-reference.test.ts), matching the PR description's
note about unrelated full-suite failures.
Deferred (not applied this round):
- pendingRequests TTL/cap (needs a sweep mechanism).
- mutableConfig cast bypass for telemetry URLs (refactor through
MutableConfigMap).
- timestamp-resolver helper consolidation across detailed-events /
error-reporting / store (3-file refactor).
- Tests for non_error_throwable, recordKanbanTaskCreated gating, and
ok=false chat.send suppression (additive coverage; not blocking).
- Localhost spoofing on /api/telemetry/events (design call).
- SIGTERM drain hook (architectural).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 263eb5761c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const runlessMatch = matches.find(([, entry]) => entry.runId === undefined); | ||
| if (runlessMatch) { | ||
| return runlessMatch[0]; |
There was a problem hiding this comment.
When a tool.result frame arrives without runId/sessionKey, resolvePendingToolKey currently picks the first pending runless match for that toolCallId. If two sessions have concurrent runless tool.start entries with the same toolCallId (which this commit now allows via session-scoped keys), the completion is recorded for an arbitrary session and the real entry remains pending until a later failure/teardown flush. That skews success/failure telemetry and can leave stale pending entries; ambiguous matches should be rejected instead of selecting the first one.
Useful? React with 👍 / 👎.
2 participants
Summary
Nerve now has the full app-side telemetry contract for
off,minimal, anddetailed.This adds the local telemetry runtime, server-owned instrumentation, the internal browser relay, fresh-install disclosure, and the public docs that explain exactly what is collected.
It also closes the app-side verification gate for this slice, while keeping real Phase 1 and Phase 2 egress blocked on the external first-party telemetry services.
What Changed
This work implements the Nerve-side telemetry design end to end.
It keeps telemetry best-effort and non-blocking, and it keeps the privacy boundary on the Nerve server instead of the browser.
Telemetry runtime and install/bootstrap contract
fresh_installvsupgrade_legacymarkersoff,minimal, anddetailedServer-owned telemetry instrumentation
session_createdmessage_submittedtool_call_completedkanban_task_createdserver/lib/ws-proxy.ts.branch_switchedso it also marks the Phase 1branchesfeature boolean.Client relay and disclosure UX
/api/telemetry/eventsrelay route with strict validation./api/server-infotelemetry disclosure fields for the UI.session_openedbranch_createdbranch_switchedanalytics.nerve.zone.Public docs
docs/TELEMETRY.md.README.md,docs/CONFIGURATION.md,docs/INSTALL.md, anddocs/UPDATING.md.Verification / ship-gate
master, so they are out of scope for this PR.Testing
I tested the telemetry-specific paths directly and re-ran the ship-gate checks under Node 22.
The app-side telemetry slice is verified, but the external collector and relay infrastructure are not part of this PR.
Targeted telemetry suite:
Result:
17files,321tests passedSupplemental install/bootstrap verification:
Production build:
PATH=/Users/cd0x23/.nvm/versions/node/v22.18.0/bin:$PATH npm run buildFull suite check:
Result: only unrelated baseline failures reproduced identically on local
master:server/lib/beads.test.tsserver/lib/upload-reference.test.tsserver/routes/files.test.tsserver/routes/upload-reference.test.tsRisks
The app-side implementation is complete, but real telemetry egress still depends on external first-party services that are not part of this repo.
That means the code here is ready for the contract, not for fully live end-to-end telemetry by itself.
detailedshould stay constrained to the approved Nerve-side allowlist until the external relay stack is live.act(...)warnings insrc/App.test.tsxare still present and predate this PR.bacff45.Follow-ups
This PR finishes the Nerve app slice.
The remaining work is external infrastructure, not more app-side telemetry plumbing.
telemetry.nerve.zonefor real Phase 1 heartbeat and scrubbed error ingestanalytics.nerve.zoneas the strict Phase 2 relay in front of self-hosted PostHogSummary by CodeRabbit
New Features
Configuration
Documentation
Chores