Skip to content

Commit 9810098

Browse files
committed
Fix formatting and linting issues
- Add comment to empty catch block in logger.ts - Fix no-explicit-any in clearcut-sender.test.ts - Apply prettier formatting to various files
1 parent 5de6fcf commit 9810098

6 files changed

Lines changed: 128 additions & 108 deletions

File tree

IMPLEMENTATION_PLAN.md

Lines changed: 76 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,20 @@ This document specifies the architecture for isolating telemetry into a separate
55
## Architecture
66

77
The system consists of two processes:
8+
89
1. **Main Process (Client)**: The DevTools MCP server. Spawns the Watchdog and sends raw telemetry events via IPC (Stdin).
910
2. **Watchdog Process (Server)**: A simplified Node.js process. Manages Session IDs, enriches events, and handles the reliable transmission of `server_shutdown` upon detecting Main Process death.
1011

1112
## Phase 1: Shared Core & Types (src/telemetry/types.ts)
1213

1314
Define the IPC protocol between Master and Watchdog.
1415

15-
* **Modify** `src/telemetry/types.ts`:
16-
* `IpcMessageType` enum: `DATA = 'data'`.
17-
* `TelemetryEvent` interface: `{ type: IpcMessageType, payload: ChromeDevToolsMcpExtension }`.
18-
* Ensure `ChromeDevToolsMcpExtension` (Protobuf mapping) is exported.
19-
* Add `ServerShutdown` interface: `export interface ServerShutdown {}`.
20-
* Update `ChromeDevToolsMcpExtension` to include `server_shutdown?: ServerShutdown`.
16+
- **Modify** `src/telemetry/types.ts`:
17+
- `IpcMessageType` enum: `DATA = 'data'`.
18+
- `TelemetryEvent` interface: `{ type: IpcMessageType, payload: ChromeDevToolsMcpExtension }`.
19+
- Ensure `ChromeDevToolsMcpExtension` (Protobuf mapping) is exported.
20+
- Add `ServerShutdown` interface: `export interface ServerShutdown {}`.
21+
- Update `ChromeDevToolsMcpExtension` to include `server_shutdown?: ServerShutdown`.
2122

2223
## Phase 2: Watchdog Process (The Sidecar)
2324

@@ -28,91 +29,100 @@ Location: `src/telemetry/watchdog/`
2829
A stateful class responsible for session management and transport.
2930

3031
**State:**
31-
* `#appVersion`: string
32-
* `#osType`: OsType
33-
* `#sessionId`: string (Hex string)
34-
* `#sessionCreated`: number (Timestamp)
32+
33+
- `#appVersion`: string
34+
- `#osType`: OsType
35+
- `#sessionId`: string (Hex string)
36+
- `#sessionCreated`: number (Timestamp)
3537

3638
**Behavior:**
37-
* **Initialization**:
38-
* Generate initial `#sessionId` using `crypto.randomUUID()`.
39-
* Set `#sessionCreated = Date.now()`.
40-
* **`send(event: ChromeDevToolsMcpExtension)`**:
41-
1. **Session Rotation**:
42-
* Check `Date.now() - #sessionCreated > 24 * 60 * 60 * 1000` (24 hours).
43-
* If expired: Generate new `#sessionId` (UUID), reset `#sessionCreated`.
44-
2. **Enrichment**: deeply clone `event`. Populate:
45-
* `client_info.client_type`
46-
* `session_id` (from state)
47-
* `app_version` (from state)
48-
* `os_type` (from state)
49-
3. **Transport**:
50-
* Log the JSON stringified event using the `logger` from `../../logger.js`.
51-
* **`sendShutdownEvent()`**:
52-
* Construct `server_shutdown` event.
53-
* **CRITICAL**: Do *not* include `flag_usage` (Main process might not have sent it if crashed).
54-
* Calls `this.send(shutdownEvent)`.
39+
40+
- **Initialization**:
41+
- Generate initial `#sessionId` using `crypto.randomUUID()`.
42+
- Set `#sessionCreated = Date.now()`.
43+
- **`send(event: ChromeDevToolsMcpExtension)`**:
44+
1. **Session Rotation**:
45+
- Check `Date.now() - #sessionCreated > 24 * 60 * 60 * 1000` (24 hours).
46+
- If expired: Generate new `#sessionId` (UUID), reset `#sessionCreated`.
47+
2. **Enrichment**: deeply clone `event`. Populate:
48+
- `client_info.client_type`
49+
- `session_id` (from state)
50+
- `app_version` (from state)
51+
- `os_type` (from state)
52+
3. **Transport**:
53+
- Log the JSON stringified event using the `logger` from `../../logger.js`.
54+
- **`sendShutdownEvent()`**:
55+
- Construct `server_shutdown` event.
56+
- **CRITICAL**: Do _not_ include `flag_usage` (Main process might not have sent it if crashed).
57+
- Calls `this.send(shutdownEvent)`.
5558

5659
### 2.2 Watchdog Entry Point (`src/telemetry/watchdog/main.ts`)
5760

5861
The executable script.
5962

6063
**Inputs (CLI Args):**
61-
* `--parent-pid`: PID of the Main Process to monitor.
62-
* `--app-version`: String.
63-
* `--os-type`: Integer (OsType).
64-
* `--log-file`: Absolute path for debugging logs (optional).
64+
65+
- `--parent-pid`: PID of the Main Process to monitor.
66+
- `--app-version`: String.
67+
- `--os-type`: Integer (OsType).
68+
- `--log-file`: Absolute path for debugging logs (optional).
6569

6670
**Logic Flow:**
71+
6772
1. **Bootstrap**:
68-
* **Arg Parsing**: Use `yargs` to parse command line arguments widely.
69-
* Setup internal logging (to file if `--log-file` provided, otherwise silent/stderr).
70-
* Instantiate `ClearcutSender`.
73+
- **Arg Parsing**: Use `yargs` to parse command line arguments widely.
74+
- Setup internal logging (to file if `--log-file` provided, otherwise silent/stderr).
75+
- Instantiate `ClearcutSender`.
7176
2. **IPC Handling (Stdin)**:
72-
* `process.stdin.resume()` and set encoding `utf8`.
73-
* Stream parser (using `readline` or buffer splitting) for newline-delimited JSON.
74-
* On valid message: `sender.send(payload)`.
77+
- `process.stdin.resume()` and set encoding `utf8`.
78+
- Stream parser (using `readline` or buffer splitting) for newline-delimited JSON.
79+
- On valid message: `sender.send(payload)`.
7580
3. **Death Detection**:
76-
* **Mechanism**: Listen for `process.stdin.on('end')` and `('close')`.
77-
* **Trigger**: On either event:
78-
1. Log "Parent death detected".
79-
2. `await sender.sendShutdownEvent()`.
80-
3. `process.exit(0)`.
81+
- **Mechanism**: Listen for `process.stdin.on('end')` and `('close')`.
82+
- **Trigger**: On either event:
83+
1. Log "Parent death detected".
84+
2. `await sender.sendShutdownEvent()`.
85+
3. `process.exit(0)`.
8186
4. **Disconnect**: Handle `process.on('disconnect')` similarly to death.
8287

8388
## Phase 3: Main Process Client (`src/telemetry/watchdog-client.ts`)
8489

8590
Abstracts the sidecar management.
8691

8792
**Class: `WatchdogClient`**
88-
* **Constructor**:
89-
* `parentPid`, `appVersion`, `osType`, `logFile`.
90-
* **Spawning Logic**:
91-
* **Path Resolution**: Use `import.meta.url` to resolve the absolute path to `watchdog/main.js`.
92-
```javascript
93-
const watchdogPath = fileURLToPath(new URL('./watchdog/main.js', import.meta.url));
94-
```
95-
* **Spawn**: `child_process.spawn(process.execPath, [watchdogPath, ...dependencies])`.
96-
* **Stdio Config**: `['pipe', 'ignore', 'ignore']` (Strictly ignore output, rely on `--log-file`).
97-
* **Lifecycle**: `unref()` the child so it doesn't block Main Process exit.
98-
* **`send(message: TelemetryEvent)`**:
99-
* JSON-stringify `event`.
100-
* Append `\n`.
101-
* Write to `child.stdin`.
102-
* Handle errors (EPIPE) gracefully (log warning, do not crash).
93+
94+
- **Constructor**:
95+
- `parentPid`, `appVersion`, `osType`, `logFile`.
96+
- **Spawning Logic**:
97+
- **Path Resolution**: Use `import.meta.url` to resolve the absolute path to `watchdog/main.js`.
98+
```javascript
99+
const watchdogPath = fileURLToPath(
100+
new URL('./watchdog/main.js', import.meta.url),
101+
);
102+
```
103+
- **Spawn**: `child_process.spawn(process.execPath, [watchdogPath, ...dependencies])`.
104+
- **Stdio Config**: `['pipe', 'ignore', 'ignore']` (Strictly ignore output, rely on `--log-file`).
105+
- **Lifecycle**: `unref()` the child so it doesn't block Main Process exit.
106+
- **`send(message: TelemetryEvent)`**:
107+
- JSON-stringify `event`.
108+
- Append `\n`.
109+
- Write to `child.stdin`.
110+
- Handle errors (EPIPE) gracefully (log warning, do not crash).
103111
104112
## Phase 4: Integration & Testing
105113
106114
### 4.1 Refactor `ClearcutLogger`
107-
* Remove direct `ClearcutSender` usage.
108-
* Instantiate `WatchdogClient` in constructor.
109-
* Proxy methods (`logToolInvocation`, etc.) to `client.send()`.
115+
116+
- Remove direct `ClearcutSender` usage.
117+
- Instantiate `WatchdogClient` in constructor.
118+
- Proxy methods (`logToolInvocation`, etc.) to `client.send()`.
110119
111120
### 4.2 Verification Plan
121+
112122
1. **Unit Tests**:
113-
* `clearcut-sender.test.ts`: Verify enrichment and session rotation logic mocking Date.
123+
- `clearcut-sender.test.ts`: Verify enrichment and session rotation logic mocking Date.
114124
2. **Integration Tests (`spawn-kill-verify`)**:
115-
* Script: Spawn Main Process (mock).
116-
* Main Process spawns Watchdog.
117-
* Main Process kills itself (`kill -9`).
118-
* **Assert**: Watchdog writes `server_shutdown` to log/transport before exiting.
125+
- Script: Spawn Main Process (mock).
126+
- Main Process spawns Watchdog.
127+
- Main Process kills itself (`kill -9`).
128+
- **Assert**: Watchdog writes `server_shutdown` to log/transport before exiting.

src/logger.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ export function saveLogsToFile(fileName: string): fs.WriteStream {
3232
}
3333

3434
export function saveLogsToFileSync(fileName: string): void {
35-
3635
debug.enable(namespacesToEnable.join(','));
3736

3837
let fd: number | undefined;
@@ -47,7 +46,8 @@ export function saveLogsToFileSync(fileName: string): void {
4746
if (fd !== undefined) {
4847
try {
4948
fs.writeSync(fd, `${chunks.join(' ')}\n`);
50-
} catch (e) {
49+
} catch {
50+
// Ignore write errors in watchdog - process may be shutting down
5151
}
5252
}
5353
};

src/telemetry/watchdog-client.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ export class WatchdogClient {
4646

4747
this.#childProcess.unref();
4848

49-
this.#childProcess.on('error', (err) => {
49+
this.#childProcess.on('error', err => {
5050
logger('Watchdog process error:', err);
5151
});
5252

tests/telemetry/clearcut-logger.test.ts

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,7 @@ describe('ClearcutLogger', () => {
6868
assert(mockWatchdogClient.send.calledOnce);
6969
const msg = mockWatchdogClient.send.firstCall.args[0];
7070
assert.strictEqual(msg.type, IpcMessageType.EVENT);
71-
assert.strictEqual(
72-
msg.payload.server_start?.flag_usage?.headless,
73-
true,
74-
);
71+
assert.strictEqual(msg.payload.server_start?.flag_usage?.headless, true);
7572
});
7673
});
7774

@@ -96,7 +93,7 @@ describe('ClearcutLogger', () => {
9693
const msg = mockWatchdogClient.send.firstCall.args[0];
9794
assert.strictEqual(msg.type, IpcMessageType.EVENT);
9895
assert.ok(msg.payload.daily_active);
99-
96+
10097
assert(mockPersistence.saveState.called);
10198
});
10299

@@ -133,10 +130,7 @@ describe('ClearcutLogger', () => {
133130
assert(mockWatchdogClient.send.calledOnce);
134131
const msg = mockWatchdogClient.send.firstCall.args[0];
135132
assert.strictEqual(msg.type, IpcMessageType.EVENT);
136-
assert.strictEqual(
137-
msg.payload.daily_active?.days_since_last_active,
138-
-1,
139-
);
133+
assert.strictEqual(msg.payload.daily_active?.days_since_last_active, -1);
140134
assert(mockPersistence.saveState.called);
141135
});
142136
});

tests/telemetry/watchdog-client.test.ts

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import assert from 'node:assert';
88
import type {spawn} from 'node:child_process';
9-
import { type ChildProcess} from 'node:child_process';
9+
import {type ChildProcess} from 'node:child_process';
1010
import EventEmitter from 'node:events';
1111
import {Writable} from 'node:stream';
1212
import {describe, it, afterEach, beforeEach} from 'node:test';
@@ -23,9 +23,9 @@ describe('WatchdogClient', () => {
2323
beforeEach(() => {
2424
const eventEmitter = new EventEmitter();
2525
const stdin = new Writable({
26-
write(_chunk, _encoding, callback) {
27-
callback();
28-
}
26+
write(_chunk, _encoding, callback) {
27+
callback();
28+
},
2929
});
3030
sinon.stub(stdin, 'write');
3131

@@ -37,7 +37,10 @@ describe('WatchdogClient', () => {
3737
kill: sinon.stub(),
3838
unref: sinon.stub(),
3939
destroyed: false,
40-
} as unknown as ChildProcess) as ChildProcess & {stdin: Writable; kill: sinon.SinonStub};
40+
} as unknown as ChildProcess) as ChildProcess & {
41+
stdin: Writable;
42+
kill: sinon.SinonStub;
43+
};
4144

4245
spawnStub = sinon.stub().returns(mockChildProcess);
4346
});
@@ -47,16 +50,19 @@ describe('WatchdogClient', () => {
4750
});
4851

4952
it('spawns watchdog process with correct arguments', () => {
50-
new WatchdogClient({
51-
parentPid: 100,
52-
appVersion: '1.2.3',
53-
osType: OsType.OS_TYPE_MACOS,
54-
}, {spawn: spawnStub as unknown as typeof spawn});
53+
new WatchdogClient(
54+
{
55+
parentPid: 100,
56+
appVersion: '1.2.3',
57+
osType: OsType.OS_TYPE_MACOS,
58+
},
59+
{spawn: spawnStub as unknown as typeof spawn},
60+
);
5561

5662
assert.strictEqual(spawnStub.calledOnce, true);
5763
const args = spawnStub.firstCall.args;
5864
const cmdArgs = args[1];
59-
65+
6066
assert.match(cmdArgs[0], /watchdog\/main\.js$/);
6167
assert.strictEqual(cmdArgs.includes('--parent-pid=100'), true);
6268
assert.strictEqual(cmdArgs.includes('--app-version=1.2.3'), true);
@@ -65,47 +71,57 @@ describe('WatchdogClient', () => {
6571
});
6672

6773
it('passes log-file argument if provided', () => {
68-
new WatchdogClient({
69-
parentPid: 100,
70-
appVersion: '1.0.0',
71-
osType: OsType.OS_TYPE_LINUX,
72-
logFile: '/tmp/test.log',
73-
}, {spawn: spawnStub as unknown as typeof spawn});
74+
new WatchdogClient(
75+
{
76+
parentPid: 100,
77+
appVersion: '1.0.0',
78+
osType: OsType.OS_TYPE_LINUX,
79+
logFile: '/tmp/test.log',
80+
},
81+
{spawn: spawnStub as unknown as typeof spawn},
82+
);
7483

7584
const cmdArgs = spawnStub.firstCall.args[1];
7685
assert.strictEqual(cmdArgs.includes('--log-file=/tmp/test.log'), true);
7786
});
7887

7988
it('sends IPC messages via stdin', () => {
80-
const client = new WatchdogClient({
81-
parentPid: 100,
82-
appVersion: '1.0.0',
83-
osType: OsType.OS_TYPE_LINUX,
84-
}, {spawn: spawnStub as unknown as typeof spawn});
89+
const client = new WatchdogClient(
90+
{
91+
parentPid: 100,
92+
appVersion: '1.0.0',
93+
osType: OsType.OS_TYPE_LINUX,
94+
},
95+
{spawn: spawnStub as unknown as typeof spawn},
96+
);
8597

8698
const msg = {type: IpcMessageType.EVENT, payload: {}};
8799
client.send(msg);
88100

89101
const writeSpy = mockChildProcess.stdin.write as sinon.SinonSpy;
90102
assert.strictEqual(writeSpy.calledOnce, true);
91-
103+
92104
const writtenData = writeSpy.firstCall.args[0];
93105
assert.strictEqual(writtenData.trim(), JSON.stringify(msg));
94106
});
95107

96108
it('handles write errors gracefully', () => {
97-
const client = new WatchdogClient({
109+
const client = new WatchdogClient(
110+
{
98111
parentPid: 100,
99112
appVersion: '1.0.0',
100113
osType: OsType.OS_TYPE_LINUX,
101-
}, {spawn: spawnStub as unknown as typeof spawn});
114+
},
115+
{spawn: spawnStub as unknown as typeof spawn},
116+
);
102117

103-
const writeStub = mockChildProcess.stdin.write as unknown as sinon.SinonStub;
118+
const writeStub = mockChildProcess.stdin
119+
.write as unknown as sinon.SinonStub;
104120
writeStub.throws(new Error('EPIPE'));
105121

106122
// Should not throw
107123
assert.doesNotThrow(() => {
108-
client.send({type: IpcMessageType.EVENT, payload: {}});
124+
client.send({type: IpcMessageType.EVENT, payload: {}});
109125
});
110126
});
111127
});

0 commit comments

Comments
 (0)