mirror of
https://github.com/Fission-AI/OpenSpec.git
synced 2026-10-02 05:24:34 +08:00
fix(telemetry): stop building inside a test hook, restore run context
The end-to-end test built dist/ in beforeAll, which exceeds the hook timeout on CI; the suite already runs against a build. Restores install_kind, stdout_tty, and the five-bucket change count. All three answer questions that are asked — how people install, whether a human or an agent is driving, and whether a project is a trial or real work — and the earlier cut traded them for less entropy than it saved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
e56ab331c3
commit
afe0c65431
@@ -255,13 +255,14 @@ That prints every event to stderr and sends nothing. It works even if you have o
|
||||
| `duration` | `<100`, `100-500`, `500-2000`, `2000-10000`, `10000+` milliseconds |
|
||||
| `previous_outcome`, `previous_command_same` | Whether your last run failed, and whether it was the same command |
|
||||
| `platform`, `node_major` | `darwin`/`linux`/`win32`; the Node major version |
|
||||
| `install_kind` | `global`, `npx`, `source`, `other` |
|
||||
| `invoker` | Which coding agent is running the command, from a fixed list, or `terminal`/`unknown` |
|
||||
| `json_mode`, `prompted`, `first_run` | Booleans |
|
||||
| `stdout_tty`, `json_mode`, `prompted`, `first_run` | Booleans |
|
||||
| `profile`, `delivery` | Your install profile and delivery mode |
|
||||
| `tools_count` | How many AI tools are configured: `0`, `1`, `2-3`, `4+` |
|
||||
| `schema_source` | `package`, `project`, or `user` |
|
||||
| `store_in_use` | Whether this run resolved through a store rather than a local root. Never which one |
|
||||
| `changes` | Whether the project has no active changes, a few, or many: `00`, `01-10`, `11+` |
|
||||
| `changes` | How many active changes: `00`, `01-03`, `04-10`, `11-30`, `31+` |
|
||||
| `milestone`, `time_to_reach` | The first time you reach each of `install` (your first run), `init`, `propose` (`openspec new change`), `apply` (`openspec validate`), and `archive`, and how long it took |
|
||||
| `tool` | Each AI tool you have configured, reported once, as its own event carrying no run context and no run id |
|
||||
| `run_id`, `work_session_id` | Random ids correlating one run, and runs less than 30 minutes apart |
|
||||
|
||||
@@ -251,7 +251,7 @@ Only the outcome label and the previous command name SHALL be stored, and the na
|
||||
### Requirement: Bounded run context
|
||||
The system SHALL attach run context to `command_completed`. Every context property SHALL satisfy the bounded property contract.
|
||||
|
||||
The context SHALL be limited to: `platform` (`darwin`, `linux`, `win32`, `other`), `node_major` (a label from a fixed list of supported majors, `other` otherwise), `invoker`, `json_mode` (boolean), `prompted` (boolean), `profile`, `delivery`, `tools_count` (bucket), `schema_source` (`package`, `project`, `user`), `store_in_use` (boolean), `changes` (bucket), and `first_run` (boolean).
|
||||
The context SHALL be limited to: `platform` (`darwin`, `linux`, `win32`, `other`), `node_major` (a label from a fixed list of supported majors, `other` otherwise), `install_kind` (`global`, `npx`, `source`, `other`), `invoker`, `stdout_tty` (boolean), `json_mode` (boolean), `prompted` (boolean), `profile`, `delivery`, `tools_count` (bucket), `schema_source` (`package`, `project`, `user`), `store_in_use` (boolean), `changes` (bucket), and `first_run` (boolean).
|
||||
|
||||
Context SHALL be kept to what a decision actually turns on. Each property is a bit of entropy in a row that already carries a persistent id, and bits accumulate into a fingerprint whether or not any single one looks harmful. A property nobody would act on is not neutral — it is cost with no return, and it SHALL be removed rather than kept for completeness.
|
||||
|
||||
@@ -265,7 +265,7 @@ Prompts SHALL be loaded through a single seam so the timing is applied once rath
|
||||
|
||||
`first_run` SHALL be true only on the invocation during which the anonymous id is generated. It is not per-project.
|
||||
|
||||
Count buckets SHALL use the fixed labels `00`, `01-10`, `11+`. A finer count is the highest-entropy value in the event and it drifts as a project grows, so a sequence of them traces a recognizable trajectory; empty, working, and heavy is the whole of what a decision here needs.
|
||||
Count buckets SHALL use the fixed labels `00`, `01-03`, `04-10`, `11-30`, `31+`.
|
||||
|
||||
Bucket labels SHALL be written so they sort in their natural order under a lexicographic sort, because that is how they are ordered wherever they are charted. A scrambled histogram is worse than no histogram.
|
||||
|
||||
|
||||
@@ -243,6 +243,7 @@ program.hook('postAction', async (_thisCommand, actionCommand) => {
|
||||
exitCode: process.exitCode === undefined ? 0 : Number(process.exitCode),
|
||||
jsonMode: isJsonRun(actionCommand),
|
||||
projectRoot: localRoot,
|
||||
installDir: getInstallDir(),
|
||||
// No local root but a store configured means this run resolved through
|
||||
// one. The store's id, remote, and path are never read, let alone sent.
|
||||
storeInUse:
|
||||
|
||||
@@ -168,6 +168,7 @@ export interface CompletionInput {
|
||||
exitCode: number | undefined;
|
||||
jsonMode: boolean;
|
||||
projectRoot?: string | null;
|
||||
installDir?: string | null;
|
||||
storeInUse?: boolean;
|
||||
schemaSource?: 'package' | 'project' | 'user';
|
||||
toolIds?: string[];
|
||||
@@ -217,6 +218,7 @@ export async function finishRun(input: CompletionInput): Promise<void> {
|
||||
try {
|
||||
context = await collectRunContext({
|
||||
projectRoot: input.projectRoot,
|
||||
installDir: input.installDir,
|
||||
stdoutIsTty: Boolean(process.stdout.isTTY),
|
||||
jsonMode: input.jsonMode,
|
||||
prompted: wasPrompted(),
|
||||
|
||||
@@ -115,8 +115,27 @@ export function detectSchemaSource(
|
||||
return 'package';
|
||||
}
|
||||
|
||||
/**
|
||||
* How this copy of the CLI was installed. `npx` runs out of a cache directory,
|
||||
* a clone runs out of a checkout, and everything else is a global install.
|
||||
* Answers whether upgrade advice is reachable, and how much of the userbase
|
||||
* is trying the tool through `npx` rather than installing it.
|
||||
*/
|
||||
export function detectInstallKind(
|
||||
installDir: string | null,
|
||||
env: NodeJS.ProcessEnv = process.env
|
||||
): 'global' | 'npx' | 'source' | 'other' {
|
||||
if (env.npm_command === 'exec' || env.npm_lifecycle_event === 'npx') return 'npx';
|
||||
if (!installDir) return 'other';
|
||||
const normalized = installDir.replace(/\\/g, '/');
|
||||
if (normalized.includes('/_npx/')) return 'npx';
|
||||
if (normalized.endsWith('/src') || normalized.includes('/OpenSpec/')) return 'source';
|
||||
return 'global';
|
||||
}
|
||||
|
||||
export interface RunContextInput {
|
||||
projectRoot?: string | null;
|
||||
installDir?: string | null;
|
||||
stdoutIsTty: boolean;
|
||||
jsonMode: boolean;
|
||||
prompted: boolean;
|
||||
@@ -134,7 +153,9 @@ export async function collectRunContext(
|
||||
const context: Record<string, unknown> = {
|
||||
platform: bucketPlatform(process.platform),
|
||||
node_major: bucketNodeMajor(process.versions.node),
|
||||
install_kind: detectInstallKind(input.installDir ?? null, env),
|
||||
invoker: detectInvoker(env, input.stdoutIsTty),
|
||||
stdout_tty: input.stdoutIsTty,
|
||||
json_mode: input.jsonMode,
|
||||
prompted: input.prompted,
|
||||
first_run: input.firstRun,
|
||||
|
||||
@@ -65,6 +65,7 @@ export const ERROR_CLASSES = [
|
||||
export type ErrorClass = (typeof ERROR_CLASSES)[number];
|
||||
|
||||
export const PLATFORMS = ['darwin', 'linux', 'win32', 'other'] as const;
|
||||
export const INSTALL_KINDS = ['global', 'npx', 'source', 'other'] as const;
|
||||
export const SCHEMA_SOURCES = ['package', 'project', 'user'] as const;
|
||||
export const EXIT_CODES = ['0', '1', '130', 'other'] as const;
|
||||
/**
|
||||
@@ -75,7 +76,7 @@ export const EXIT_CODES = ['0', '1', '130', 'other'] as const;
|
||||
* ranges that mix units carry an ordinal prefix, since no padding rescues
|
||||
* `<1h` against `31d+`.
|
||||
*/
|
||||
export const COUNT_BUCKETS = ['00', '01-10', '11+'] as const;
|
||||
export const COUNT_BUCKETS = ['00', '01-03', '04-10', '11-30', '31+'] as const;
|
||||
export const TOOL_COUNT_BUCKETS = ['0', '1', '2-3', '4+'] as const;
|
||||
export const DURATION_BUCKETS = [
|
||||
'1_under_100ms',
|
||||
@@ -143,7 +144,9 @@ const PROPERTY_VALUES = {
|
||||
// Run context
|
||||
platform: PLATFORMS,
|
||||
node_major: NODE_MAJORS,
|
||||
install_kind: INSTALL_KINDS,
|
||||
invoker: INVOKERS,
|
||||
stdout_tty: 'boolean',
|
||||
json_mode: 'boolean',
|
||||
prompted: 'boolean',
|
||||
first_run: 'boolean',
|
||||
@@ -262,15 +265,16 @@ export function sanitizeProperties(
|
||||
|
||||
/** Bucket a count into the fixed labels. */
|
||||
/**
|
||||
* Three buckets, not five. A finer count is the highest-entropy field in the
|
||||
* event and it drifts as a project grows, so a sequence of them traces a
|
||||
* recognizable trajectory. Empty / working / heavy is all any decision here
|
||||
* has ever needed.
|
||||
* Bucketed, never exact: the count separates a trial from real use, which is
|
||||
* the question it exists for, and the exact number would say more about the
|
||||
* project than any decision needs.
|
||||
*/
|
||||
export function bucketCount(count: number): (typeof COUNT_BUCKETS)[number] {
|
||||
if (count <= 0) return '00';
|
||||
if (count <= 10) return '01-10';
|
||||
return '11+';
|
||||
if (count <= 3) return '01-03';
|
||||
if (count <= 10) return '04-10';
|
||||
if (count <= 30) return '11-30';
|
||||
return '31+';
|
||||
}
|
||||
|
||||
export function bucketToolCount(count: number): (typeof TOOL_COUNT_BUCKETS)[number] {
|
||||
|
||||
@@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest';
|
||||
import { promises as fs } from 'fs';
|
||||
import os from 'os';
|
||||
import path from 'path';
|
||||
import { detectInvoker, collectRunContext } from '../../src/telemetry/context.js';
|
||||
import { detectInvoker, detectInstallKind, collectRunContext } from '../../src/telemetry/context.js';
|
||||
import { sanitizeProperties, setRegistryChecks } from '../../src/telemetry/properties.js';
|
||||
|
||||
setRegistryChecks({ isCommand: () => true, isTool: () => true });
|
||||
@@ -26,6 +26,15 @@ describe('detectInvoker', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('detectInstallKind', () => {
|
||||
it('recognizes npx, a checkout, and a global install', () => {
|
||||
expect(detectInstallKind('/Users/j/.npm/_npx/abc/node_modules/openspec', {})).toBe('npx');
|
||||
expect(detectInstallKind(null, { npm_command: 'exec' })).toBe('npx');
|
||||
expect(detectInstallKind('/usr/local/lib/node_modules/openspec', {})).toBe('global');
|
||||
expect(detectInstallKind(null, {})).toBe('other');
|
||||
});
|
||||
});
|
||||
|
||||
describe('collectRunContext', () => {
|
||||
it('buckets the change count and keeps no names', async () => {
|
||||
const root = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-ctx-'));
|
||||
@@ -45,7 +54,7 @@ describe('collectRunContext', () => {
|
||||
env: {},
|
||||
});
|
||||
|
||||
expect(context.changes).toBe('01-10');
|
||||
expect(context.changes).toBe('01-03');
|
||||
expect(context.tools_count).toBe('2-3');
|
||||
expect(context.store_in_use).toBe(true);
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
|
||||
import { execFileSync, spawnSync } from 'node:child_process';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
import * as os from 'node:os';
|
||||
@@ -54,7 +54,8 @@ describe('telemetry end to end', () => {
|
||||
beforeAll(() => {
|
||||
home = fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-e2e-tel-'));
|
||||
fs.mkdirSync(path.join(home, 'proj'), { recursive: true });
|
||||
execFileSync('npm', ['run', 'build'], { cwd: repoRoot, stdio: 'ignore' });
|
||||
// No build here: the suite already runs against a built dist/, and building
|
||||
// inside a hook blows the hook timeout on CI.
|
||||
});
|
||||
|
||||
afterAll(() => fs.rmSync(home, { recursive: true, force: true }));
|
||||
@@ -102,7 +103,7 @@ describe('telemetry end to end', () => {
|
||||
expect(serialized).not.toContain('acme-billing-rewrite');
|
||||
expect(serialized).not.toContain(home);
|
||||
// The count is still reported, as a bucket.
|
||||
expect(events.find((e) => e.event === 'command_completed')?.properties.changes).toBe('01-10');
|
||||
expect(events.find((e) => e.event === 'command_completed')?.properties.changes).toBe('01-03');
|
||||
fs.rmSync(named, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
|
||||
@@ -65,7 +65,7 @@ describe('telemetry events', () => {
|
||||
errorClass: 'archive_blocked',
|
||||
exitCode: 1,
|
||||
durationMs: 1500,
|
||||
context: { platform: 'darwin', changes: '01-10' },
|
||||
context: { platform: 'darwin', changes: '04-10' },
|
||||
});
|
||||
await shutdown();
|
||||
|
||||
@@ -80,7 +80,7 @@ describe('telemetry events', () => {
|
||||
previous_outcome: 'none',
|
||||
previous_command_same: false,
|
||||
platform: 'darwin',
|
||||
changes: '01-10',
|
||||
changes: '04-10',
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -115,9 +115,9 @@ describe('event names', () => {
|
||||
describe('buckets', () => {
|
||||
it('buckets counts', () => {
|
||||
expect(bucketCount(0)).toBe('00');
|
||||
expect(bucketCount(3)).toBe('01-10');
|
||||
expect(bucketCount(11)).toBe('11+');
|
||||
expect(bucketCount(3500)).toBe('11+');
|
||||
expect(bucketCount(3)).toBe('01-03');
|
||||
expect(bucketCount(11)).toBe('11-30');
|
||||
expect(bucketCount(3500)).toBe('31+');
|
||||
});
|
||||
|
||||
it('buckets tool counts', () => {
|
||||
|
||||
Reference in New Issue
Block a user