Skip to content

Commit d07d3b1

Browse files
Hardening v2.7claude
andcommitted
feat(usage): default-on telemetry, and the emitters it was missing
Three separate defects, all of which had to be fixed for a single row to reach production. 1. THE CLIENT FLAG COULD NEVER BE SET UsageOutbox.isEnabled() read NATIVELY_USAGE_OUTBOX_ENABLED from process.env. An app launched from the Dock or the Start menu inherits no environment and nothing in this repo set it, so the outbox shipped inert on 2026-08-14 and record() returned 'disabled' — the events were never even written locally, so they are gone rather than undelivered. Polarity is inverted: absent means ON, an explicit 0/false/off/no disables, and '' reads as absent so a mis-templated launcher config cannot silently kill it. Be honest about what that costs: =0 is now reachable only in dev, CI and a terminal launch, so the client half is NOT a production kill switch and the docblock that promised "a client can stop emitting without a server change" is gone. The lever that reaches a shipped fleet is the server's BYOK_CLIENT_EVENTS_ENABLED=0 → 503, which the outbox retries for ~15 days rather than acking away. One working lever is enough. 2. operational_telemetry_events HAD ZERO EMITTERS A migration, a route, an allowlist, a 45-day sweep — and nothing in this application ever called usageOutbox.recordTelemetry(). runTracked neither calls tracker.telemetry() nor hands the tracker to the handler. Turning OPS_TELEMETRY_ENABLED on would have changed nothing, because "enabled" and "emitting" are different facts and only one had been built. recordTurnTelemetry(trace) now maps the AnswerTrace the pipeline already produces, called from recordTurnMetrics() — the one funnel where both engines hand over a finished trace, so it needs no new instrumentation on the answer path and cannot miss a surface. That hook was chosen carefully. recordLegacyTurn() looks live (three call sites in ipcHandlers and IntelligenceEngine) but is gated on NATIVELY_CI_V3_TRACE, which a packaged app never sees — an emitter there would have shipped into dead code, the exact bug this commit exists to fix. recordTurnMetrics is reached via buildV3Prompt → orchestrate() because DEFAULT_ENABLED has been true since 2026-07-30. SUPERSEDED maps to 'interrupted', never 'completed': auto-answer supersedes turns routinely and counting them as completions inflates every rate built on the table. Metadata is 5 keys with 3 spare under MAX_METADATA_KEYS=8, because overflow does not strip — the server rejects the whole event and the outbox then drops it permanently as a poison row. llm_ttfb_ms is named for what the trace measures (time to first byte), not the llm_duration_ms the migration comment suggested. 3. recordAppStarted / recordAppShutdown HAD ZERO CALLERS Both written 2026-08-14, both unreferenced. Wired in main.ts: started once the outbox is up, shutdown as the FIRST statement in will-quit — ahead of checkpointDatabase('will-quit'), because record() is a synchronous INSERT into the file about to be checkpointed and it swallows its own errors, so writing afterwards would lose the row with no signal. SIGTERM/SIGINT call app.exit() and bypass will-quit, so a killed app records no shutdown; that is the honest outcome, not a bug to fix. KNOWN LIMITATION: the telemetry row and the _tracked() ledger row for one turn share no feature_session_id — different wrappers. Correlate on time plus app_session_id until that matters enough to thread an id. TESTING. UsageTelemetryEmission.test.mjs imports natively-api's REAL validateAuditBatch and asserts every emitted payload is accepted: the two allowlists live in different repos and drift, and a refused event is dropped rather than retried. Mutation-probed — injecting a prose metadata value turns it red with metadata_bad_value. It is a separate FILE because the emitter reaches DatabaseManager through the bundle inlined into usageInstrumentation.js, which holds its own per-process singleton; sharing a process meant earlier describes deleted the userData dir out from under it and every assertion read zero rows. Verified on macOS: 30/30 UsageOutbox + 12/12 UsageTelemetryEmission under ELECTRON_RUN_AS_NODE=1 electron, 1897/1897 test:intelligence, typecheck clean. No process.platform branch exists in this path (it appears once, as a data field), but the packaged-launch environment and the Windows shutdown path were reviewed, not executed. Submodule: natively-api 6217f1f..35e5a4d (the three server flags, same polarity). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013FjZgLYK8eDRv9zRMBVLVS
1 parent 95fdc24 commit d07d3b1

7 files changed

Lines changed: 581 additions & 10 deletions

File tree

electron/context-intelligence/observability/rollout-metrics.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,20 @@ const bump = (m: Record<string, number>, k: string | undefined) => {
127127
* the answer. Callers are not expected to wrap it.
128128
*/
129129
export function recordTurnMetrics(trace: AnswerTrace | null | undefined): void {
130+
// Operational telemetry (layer B), emitted from the same funnel for the same
131+
// reason the counters are: this is the ONE place both engines hand over a
132+
// finished trace, so an emitter here needs no new instrumentation on the
133+
// answer path and cannot miss a surface.
134+
//
135+
// Lazily required and independently wrapped. UsageOutbox reaches
136+
// InstallPingManager, which touches app.getPath('userData') — an eager import
137+
// would throw in every context without a ready Electron app, which is exactly
138+
// the offline evaluators and replay harnesses that load this module.
139+
try {
140+
// eslint-disable-next-line @typescript-eslint/no-var-requires
141+
require('../../services/usageInstrumentation').recordTurnTelemetry(trace);
142+
} catch { /* observability only */ }
143+
130144
try {
131145
if (!trace) return;
132146
const t = trace as unknown as Record<string, any>;

electron/main.ts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8425,15 +8425,30 @@ async function initializeApp() {
84258425
//
84268426
// Started AFTER credentials are loaded, but the key is passed as a GETTER
84278427
// rather than a value: a user who pastes their Natively key ten minutes from
8428-
// now must not need a restart before their queued events can drain. Inert
8429-
// unless NATIVELY_USAGE_OUTBOX_ENABLED is set, so shipping this changes
8430-
// nothing until the flag is switched on.
8428+
// now must not need a restart before their queued events can drain.
8429+
//
8430+
// ON BY DEFAULT since 2026-08-27. It was gated behind an unset env var from
8431+
// 2026-08-14 until then, which meant the whole ledger shipped inert and
8432+
// collected nothing in production for the entire period. Setting
8433+
// NATIVELY_USAGE_OUTBOX_ENABLED=0 turns it back off, but only where an
8434+
// environment can actually be set (dev, CI, a terminal launch) — a packaged
8435+
// app inherits none. The production kill switch is server-side; see
8436+
// UsageOutbox.isEnabled().
84318437
try {
84328438
const { usageOutbox } = require('./services/UsageOutbox');
84338439
usageOutbox.start(() => CredentialsManager.getInstance().getNativelyApiKey());
84348440
// Drain anything queued while the app was closed, without waiting a full
84358441
// dispatch interval. Deliberately not awaited — startup must not block on it.
84368442
setTimeout(() => { void usageOutbox.dispatchOnce(); }, 5000);
8443+
8444+
// §5 application lifecycle. recordAppStarted/recordAppShutdown were written
8445+
// on 2026-08-14 and had ZERO callers until 2026-08-27 — the functions
8446+
// existed, the taxonomy reserved app_started/app_shutdown, and nothing ever
8447+
// emitted either. Started is recorded here rather than at whenReady so it
8448+
// means "the app came up far enough to be usable", which is the only
8449+
// reading a launch-failure investigation can act on.
8450+
const { recordAppStarted } = require('./services/usageInstrumentation');
8451+
recordAppStarted();
84378452
} catch (err: any) {
84388453
console.warn('[UsageOutbox] startup failed (non-fatal):', err?.message || err);
84398454
}
@@ -8992,6 +9007,19 @@ if (process.env.THINKING_MATRIX === '1') {
89929007
}
89939008

89949009
app.on('will-quit', () => {
9010+
// FIRST, and deliberately so: record() is a synchronous INSERT into the
9011+
// same SQLite file that checkpointDatabase('will-quit') below is about to
9012+
// checkpoint, and record() swallows its own errors — so writing after the
9013+
// checkpoint would lose the row with no signal at all. Ordering is the
9014+
// whole correctness argument here.
9015+
//
9016+
// Only the graceful path emits this. SIGTERM/SIGINT call app.exit(), which
9017+
// bypasses will-quit — so a killed app records no shutdown, which is the
9018+
// honest outcome rather than a fabricated one.
9019+
try {
9020+
const { recordAppShutdown } = require('./services/usageInstrumentation');
9021+
recordAppShutdown();
9022+
} catch { /* instrumentation must never block a quit */ }
89959023
appState.stopNativeOomTraceSampling();
89969024
nativeOomTrace.stop('will-quit');
89979025
stopAppManagedHindsight('will-quit');

electron/services/UsageOutbox.ts

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,9 @@
3232
* and returns. No network, no await for the caller.
3333
* 2. Never throws into a caller. Every entry point is wrapped. An audit bug
3434
* must not be able to fail a meeting.
35-
* 3. Inert unless enabled. Two flags, checked per call.
35+
* 3. On by default, killable server-side. Both flags are checked per call;
36+
* each defaults ON and is turned off with an explicit `=0`. The lever that
37+
* reaches a shipped fleet is the server's, not this one — see isEnabled().
3638
* 4. Bounded. 10,000 rows / oldest-undelivered dropped past the cap, counted.
3739
* 5. No content. Only identifiers, enums, counts and durations — the server
3840
* allowlist rejects anything else, and this file must never try to send it.
@@ -93,6 +95,21 @@ const BACKOFF_MS = [
9395
20 * 60_000, 30 * 60_000, 60 * 60_000,
9496
];
9597

98+
/**
99+
* Flag polarity for this subsystem: **absent means ON**. Only an explicit
100+
* off-value disables.
101+
*
102+
* `''` is treated as absent on purpose. `FOO=$UNSET_VAR` exports an empty
103+
* string, which is how a mis-templated CI or launcher config silently kills a
104+
* subsystem it never meant to touch. An empty value is missing information, not
105+
* an instruction to stop recording.
106+
*/
107+
export function usageFlagEnabled(v: string | undefined): boolean {
108+
if (v === undefined || v === '') return true;
109+
const s = v.trim().toLowerCase();
110+
return !(s === '0' || s === 'false' || s === 'off' || s === 'no');
111+
}
112+
96113
export type UsageLayer = 'ledger' | 'telemetry';
97114

98115
export interface UsageEventInput {
@@ -143,13 +160,24 @@ export class UsageOutbox {
143160
}
144161

145162
/**
146-
* Both flags must be on. The client half is separately switchable from the
147-
* server half so a bad client release can be silenced server-side without a
148-
* client update, and a client can stop emitting without a server change.
163+
* On by default. `NATIVELY_USAGE_OUTBOX_ENABLED=0` (or `false`/`off`/`no`)
164+
* turns it off.
165+
*
166+
* HONEST LIMIT OF THIS SWITCH. It reads `process.env`, and a packaged app
167+
* launched from the Dock or the Start menu inherits no environment — so
168+
* once this defaults ON, `=0` is reachable only in development, in CI, and
169+
* from a terminal launch. It is NOT a production kill switch, and the
170+
* previous claim here that "a client can stop emitting without a server
171+
* change" is no longer true. Do not rely on it during an incident.
172+
*
173+
* The switch that still works against a shipped fleet is the server's
174+
* `BYOK_CLIENT_EVENTS_ENABLED=0`: /v1/usage/audit answers 503, and this
175+
* outbox treats 503 as retryable and HOLDS the events (~15 days, see
176+
* MAX_ATTEMPTS) instead of acking them away. Silencing a bad client release
177+
* therefore costs no data, which is why one lever is enough.
149178
*/
150179
public isEnabled(): boolean {
151-
const v = process.env.NATIVELY_USAGE_OUTBOX_ENABLED;
152-
return v === '1' || v === 'true';
180+
return usageFlagEnabled(process.env.NATIVELY_USAGE_OUTBOX_ENABLED);
153181
}
154182

155183
/**

electron/services/__tests__/UsageOutbox.test.mjs

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,3 +400,87 @@ describe('runTracked outcome classification', () => {
400400
}
401401
});
402402
});
403+
404+
// ── Flag polarity (2026-08-27) ───────────────────────────────────────────────
405+
//
406+
// The outbox shipped on 2026-08-14 gated behind NATIVELY_USAGE_OUTBOX_ENABLED,
407+
// a variable a packaged Electron app can never see — it is not set by the app,
408+
// and a launch from the Dock or the Start menu inherits no environment. The
409+
// result was a subsystem that looked shipped and recorded nothing at all in
410+
// production. These tests pin the inverted default so that cannot recur.
411+
describe('outbox flag polarity', { skip: HAVE_BUILD ? false : 'run `npm run build:electron` first' }, () => {
412+
const OUTBOX_PATH = path.join(REPO, 'dist-electron/electron/services/UsageOutbox.js');
413+
const ORIGINAL = process.env.NATIVELY_USAGE_OUTBOX_ENABLED;
414+
415+
after(() => {
416+
if (ORIGINAL === undefined) delete process.env.NATIVELY_USAGE_OUTBOX_ENABLED;
417+
else process.env.NATIVELY_USAGE_OUTBOX_ENABLED = ORIGINAL;
418+
});
419+
420+
test('an absent flag enables the outbox', () => {
421+
const { usageOutbox } = require(OUTBOX_PATH);
422+
delete process.env.NATIVELY_USAGE_OUTBOX_ENABLED;
423+
assert.equal(usageOutbox.isEnabled(), true, 'absent must mean on');
424+
});
425+
426+
test('only an explicit off-value disables it', () => {
427+
const { usageOutbox } = require(OUTBOX_PATH);
428+
for (const off of ['0', 'false', 'FALSE', 'off', 'no', ' 0 ']) {
429+
process.env.NATIVELY_USAGE_OUTBOX_ENABLED = off;
430+
assert.equal(usageOutbox.isEnabled(), false, `${JSON.stringify(off)} must disable`);
431+
}
432+
for (const on of ['1', 'true', 'yes', 'anything-else']) {
433+
process.env.NATIVELY_USAGE_OUTBOX_ENABLED = on;
434+
assert.equal(usageOutbox.isEnabled(), true, `${JSON.stringify(on)} must not disable`);
435+
}
436+
});
437+
438+
test('an empty value reads as absent, not as off', () => {
439+
// `FOO=$UNSET_VAR` exports an empty string. A mis-templated launcher or
440+
// CI config must not be able to silently take the outbox dark.
441+
const { usageOutbox } = require(OUTBOX_PATH);
442+
process.env.NATIVELY_USAGE_OUTBOX_ENABLED = '';
443+
assert.equal(usageOutbox.isEnabled(), true, 'empty must mean absent');
444+
});
445+
446+
// The three tests above only prove a boolean. This one proves the thing the
447+
// boolean is FOR: that with no flag set, a recorded event actually lands in
448+
// the local table. That is the exact failure that went unnoticed for two
449+
// weeks — record() returned 'disabled' and wrote nothing, so the events were
450+
// gone rather than merely undelivered, and no amount of fixing the server
451+
// could get them back.
452+
test('with no flag set, record() actually persists a row to the outbox table', () => {
453+
const tdir = fs.mkdtempSync(path.join(os.tmpdir(), 'natively-polarity-test-'));
454+
const prevUserData = process.env.NATIVELY_TEST_USERDATA;
455+
process.env.NATIVELY_TEST_USERDATA = tdir;
456+
delete process.env.NATIVELY_USAGE_OUTBOX_ENABLED;
457+
458+
const { DatabaseManager } = require(DBM_PATH);
459+
DatabaseManager.instance = null;
460+
const pdb = DatabaseManager.getInstance();
461+
try {
462+
const { usageOutbox } = require(OUTBOX_PATH);
463+
const result = usageOutbox.record({
464+
event_type: 'feature_completed',
465+
event_status: 'completed',
466+
feature: 'mode_execution',
467+
reported_duration_ms: 1234,
468+
});
469+
assert.notEqual(result, 'disabled', 'an absent flag must not disable recording');
470+
471+
const rows = pdb.claimUsageOutboxBatch(100, Date.now() + 10 ** 12);
472+
assert.equal(rows.length, 1, 'exactly one row must be queued for delivery');
473+
const payload = rows[0].payload;
474+
assert.equal(payload.layer, 'ledger');
475+
assert.equal(payload.event_type, 'feature_completed');
476+
assert.equal(payload.feature, 'mode_execution');
477+
assert.equal(payload.reported_duration_ms, 1234);
478+
} finally {
479+
try { pdb?.close?.(); } catch { /* ignore */ }
480+
DatabaseManager.instance = null;
481+
if (prevUserData === undefined) delete process.env.NATIVELY_TEST_USERDATA;
482+
else process.env.NATIVELY_TEST_USERDATA = prevUserData;
483+
try { fs.rmSync(tdir, { recursive: true, force: true }); } catch { /* ignore */ }
484+
}
485+
});
486+
});

0 commit comments

Comments
 (0)