Skip to content

Commit b376da3

Browse files
waltossclaude
andcommitted
Code-quality pass on DAP refactor
Addresses review feedback on the initial refactor: - DapClient JSDoc: explain wire format, responsibilities, and link to the canonical DAP spec at microsoft.github.io/debug-adapter-protocol/overview. JSDoc on `pid` explaining why it's undefined for TCP transports. - Zod-validated message envelopes: `handleMessage` now parses via schemas in `src/dap/protocol.ts` (ProtocolMessage / Response / Event) instead of casting. JSON parse and schema errors are logged via the existing dap logger (warn level) instead of silently swallowed. - `handleIncomingData` extracted from the inline transport callback. - Strongly-typed `DapClient.send`: new `DapCommandMap` / `DapEventMap` in `src/dap/protocol.ts` give typed args and return the response body directly (drops `response.body as X` at ~13 call sites in session.ts). Optional-args commands (`configurationDone`, `disconnect`) get a rest-tuple conditional type so they remain callable without `undefined`. Vendor extensions like Java's `redefineClasses` fall through to the untyped string overload. - `DapClient.waitForEvent(name, timeoutMs)` helper replaces the hand-rolled `armInitializedWaiter` — listener + timer cleanup live in one place. - Session features moved to a strategy pattern: `SessionCapabilities` → `SessionFeatures` (rename avoids collision with DAP's `DebugProtocol.Capabilities` on the adapter side). `DapRuntimeConfig.features` is now `Partial<SessionFeatures>` — runtime configs only list overrides on top of `DEFAULT_DAP_FEATURES`. `DapSession` no longer has the `isJava` conditional. - Transport description in `wsUrl`: `dap://python/tcp(127.0.0.1:5678)` vs `dap://lldb/spawn(lldb-dap)` makes `dbg status` clear about whether the adapter is spawned or attached. - `_runtime` narrowed via zod: new `CanonicalRuntimeSchema` derives a `CanonicalRuntime` union from a single const tuple. `RUNTIME_CONFIGS` is typed `Record<CanonicalRuntime, DapRuntimeConfig>` so adding a new runtime without a config is a compile error. `resolveRuntime` now narrows `string` → `CanonicalRuntime`. - `getDap()` → `client` getter on `DapSession`. All ~20 call sites read `this.client.send(...)`. Throw semantics preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0523003 commit b376da3

13 files changed

Lines changed: 532 additions & 277 deletions

File tree

src/cdp/session.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import type { RemoteObject } from "../formatter/values.ts";
55
import { formatValue } from "../formatter/values.ts";
66
import { createLogger, type Logger } from "../logger/index.ts";
77
import { BaseSession, type WaitForStopOptions } from "../session/base-session.ts";
8-
import type { BreakpointListItem, SessionCapabilities, SourceMapInfo } from "../session/session.ts";
8+
import type { BreakpointListItem, SessionFeatures, SourceMapInfo } from "../session/session.ts";
99
import type {
1010
AttachResult,
1111
ConsoleMessage,
@@ -102,7 +102,7 @@ export class CdpSession extends BaseSession {
102102
private log: Logger<"session">;
103103
private cdpLog: Logger<"cdp">;
104104

105-
readonly capabilities: SessionCapabilities = {
105+
readonly features: SessionFeatures = {
106106
functionBreakpoints: false,
107107
logpoints: true,
108108
hotpatch: true,

src/daemon/entry.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ server.onRequest(async (req: DaemonRequest): Promise<DaemonResponse> => {
150150
case "break-fn": {
151151
const session = requireSession();
152152
if (isError(session)) return session;
153-
if (!session.capabilities.functionBreakpoints || !session.setFunctionBreakpoint) {
153+
if (!session.features.functionBreakpoints || !session.setFunctionBreakpoint) {
154154
return {
155155
ok: false,
156156
error: "Function breakpoints are only supported with DAP runtimes (e.g. --runtime lldb)",
@@ -365,7 +365,7 @@ server.onRequest(async (req: DaemonRequest): Promise<DaemonResponse> => {
365365
case "modules": {
366366
const session = requireSession();
367367
if (isError(session)) return session;
368-
if (!session.capabilities.modules || !session.getModules) {
368+
if (!session.features.modules || !session.getModules) {
369369
return {
370370
ok: false,
371371
error: "Modules are only available in DAP mode (e.g. --runtime lldb)",

src/dap/client.ts

Lines changed: 138 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -2,22 +2,67 @@ import type { DebugProtocol } from "@vscode/debugprotocol";
22

33
import { REQUEST_TIMEOUT_MS } from "../constants.ts";
44
import type { Logger } from "../logger/index.ts";
5+
import {
6+
type DapBody,
7+
type DapCommand,
8+
type DapEventMap,
9+
type DapEventName,
10+
type DapSendRest,
11+
parseProtocolMessage,
12+
} from "./protocol.ts";
513
import type { DapTransport } from "./transport.ts";
614

7-
// biome-ignore lint/suspicious/noExplicitAny: Required for handler map that stores both typed and untyped handlers
15+
// biome-ignore lint/suspicious/noExplicitAny: handler map stores heterogeneous typed handlers
816
type AnyHandler = (...args: any[]) => void;
917

1018
interface PendingRequest {
11-
resolve: (result: DebugProtocol.Response) => void;
19+
command: string;
20+
resolve: (body: unknown) => void;
1221
reject: (error: Error) => void;
1322
timer: ReturnType<typeof setTimeout>;
1423
}
1524

1625
/**
17-
* DAP (Debug Adapter Protocol) client. Owns the wire format (Content-Length
18-
* headers + JSON) and the request/response/event bookkeeping. Bytes flow
19-
* through a DapTransport, so this class is agnostic to whether the adapter
20-
* is a spawned subprocess or a remote TCP server.
26+
* Framed, request/response/event DAP (Debug Adapter Protocol) client.
27+
*
28+
* DAP is Microsoft's language-agnostic debugger wire protocol. The payloads
29+
* here — `Content-Length`-framed JSON messages split into three types
30+
* (request, response, event) correlated via `seq` / `request_seq` — are
31+
* exactly what the spec defines. For the full protocol reference see:
32+
*
33+
* - Overview: https://microsoft.github.io/debug-adapter-protocol/overview
34+
* - Specification: https://microsoft.github.io/debug-adapter-protocol/specification
35+
*
36+
* Typed requests/responses come from {@link DapCommandMap} in `protocol.ts`
37+
* (derived from `@vscode/debugprotocol`); typed events from {@link DapEventMap}.
38+
*
39+
* What this class owns:
40+
* - **Wire format**: framing bytes in/out of a {@link DapTransport}
41+
* (Content-Length header + JSON body).
42+
* - **Request/response correlation**: each outgoing request gets a fresh
43+
* `seq`, and the matching response (or a per-request timeout) resolves
44+
* the promise returned by {@link send}.
45+
* - **Event fan-out**: incoming events are routed to listeners registered
46+
* via {@link on} / {@link off}. Typed via {@link DapEventMap}.
47+
* - **Connection lifecycle**: on transport close, pending requests reject
48+
* and new {@link send} calls throw.
49+
*
50+
* What this class does NOT own:
51+
* - **Byte transport**: how those bytes travel (stdio vs TCP) is the
52+
* {@link DapTransport} implementation's concern. See
53+
* {@link StdioTransport} and {@link TcpTransport}.
54+
* - **Protocol semantics**: ordering `initialize` → launch/attach →
55+
* `configurationDone`, waiting for the `initialized` event, mapping
56+
* DAP stopped-events to session state, etc. — that's `DapSession`.
57+
*
58+
* @example
59+
* ```ts
60+
* const transport = await new SpawnAdapterConnector(["lldb-dap"]).connect();
61+
* const dap = new DapClient(transport);
62+
* const caps = await dap.send("initialize", { adapterID: "lldb", ... });
63+
* dap.on("stopped", (body) => console.log("stopped at", body));
64+
* await dap.send("launch", { program: "./a.out" });
65+
* ```
2166
*/
2267
export class DapClient {
2368
private nextSeq = 1;
@@ -34,47 +79,48 @@ export class DapClient {
3479
this.logger = logger ?? null;
3580
this.isConnected = true;
3681
transport.start({
37-
onData: (chunk) => {
38-
this.buffer += chunk;
39-
this.processBuffer();
40-
},
82+
onData: (chunk) => this.handleIncomingData(chunk),
4183
onClose: () => this.handleClose(),
4284
});
4385
}
4486

4587
/**
46-
* Send a DAP request and wait for the response.
88+
* Send a typed DAP request and resolve with the response body. Known
89+
* commands (see {@link DapCommandMap}) type both `args` and the returned
90+
* body. Unknown command strings fall through to the untyped overload for
91+
* vendor extensions (e.g. Java's `redefineClasses`).
92+
*
93+
* Rejects if the adapter returns `success: false`, the per-request
94+
* timeout elapses, or the transport closes before the response arrives.
4795
*/
48-
async send(command: string, args?: Record<string, unknown>): Promise<DebugProtocol.Response> {
96+
async send<C extends DapCommand>(command: C, ...rest: DapSendRest<C>): Promise<DapBody<C>>;
97+
async send(command: string, args?: Record<string, unknown>): Promise<unknown>;
98+
async send(command: string, args?: Record<string, unknown>): Promise<unknown> {
4999
if (!this.isConnected) {
50100
throw new Error("DAP client is not connected");
51101
}
52102

53103
const seq = this.nextSeq++;
54-
const request: DebugProtocol.Request = {
55-
seq,
56-
type: "request",
57-
command,
58-
};
59-
if (args !== undefined) {
60-
request.arguments = args;
61-
}
104+
const request: DebugProtocol.Request = { seq, type: "request", command };
105+
if (args !== undefined) request.arguments = args;
62106

63107
this.logger?.trace("send", { command, seq, args });
64108

65-
return new Promise<DebugProtocol.Response>((resolve, reject) => {
109+
return new Promise<unknown>((resolve, reject) => {
66110
const timer = setTimeout(() => {
67111
this.pending.delete(seq);
68112
reject(new Error(`DAP request timed out: ${command} (seq=${seq})`));
69113
}, REQUEST_TIMEOUT_MS);
70114

71-
this.pending.set(seq, { resolve, reject, timer });
115+
this.pending.set(seq, { command, resolve, reject, timer });
72116
this.writeMessage(request);
73117
});
74118
}
75119

76-
/** Register an event listener for a DAP event type (e.g. "stopped", "output"). */
77-
on(event: string, handler: (body: unknown) => void): void {
120+
/** Register a typed event listener. */
121+
on<E extends DapEventName>(event: E, handler: (body: DapEventMap[E]) => void): void;
122+
on(event: string, handler: (body: unknown) => void): void;
123+
on(event: string, handler: AnyHandler): void {
78124
let handlers = this.listeners.get(event);
79125
if (!handlers) {
80126
handlers = new Set();
@@ -84,16 +130,36 @@ export class DapClient {
84130
}
85131

86132
/** Remove an event listener. */
87-
off(event: string, handler: (body: unknown) => void): void {
133+
off<E extends DapEventName>(event: E, handler: (body: DapEventMap[E]) => void): void;
134+
off(event: string, handler: (body: unknown) => void): void;
135+
off(event: string, handler: AnyHandler): void {
88136
const handlers = this.listeners.get(event);
89137
if (handlers) {
90138
handlers.delete(handler);
91-
if (handlers.size === 0) {
92-
this.listeners.delete(event);
93-
}
139+
if (handlers.size === 0) this.listeners.delete(event);
94140
}
95141
}
96142

143+
/**
144+
* Register a one-shot listener for `event` and resolve when it fires, or
145+
* reject after `timeoutMs`. Cleans up both the listener and the timer in
146+
* either outcome so there's no leak on timeout.
147+
*/
148+
waitForEvent<E extends DapEventName>(event: E, timeoutMs: number): Promise<DapEventMap[E]> {
149+
return new Promise<DapEventMap[E]>((resolve, reject) => {
150+
const handler = (body: DapEventMap[E]) => {
151+
clearTimeout(timer);
152+
this.off(event, handler);
153+
resolve(body);
154+
};
155+
const timer = setTimeout(() => {
156+
this.off(event, handler);
157+
reject(new Error(`Timed out waiting for DAP "${event}" event (${timeoutMs}ms)`));
158+
}, timeoutMs);
159+
this.on(event, handler);
160+
});
161+
}
162+
97163
/** Disconnect from the debug adapter, releasing the transport. */
98164
disconnect(): void {
99165
if (!this.isConnected) return;
@@ -109,10 +175,16 @@ export class DapClient {
109175
this.transport.close();
110176
}
111177

178+
/** Whether the underlying transport is still open. */
112179
get connected(): boolean {
113180
return this.isConnected;
114181
}
115182

183+
/**
184+
* PID of the adapter subprocess. Undefined for TCP transports since the
185+
* adapter is a separate process owned by the user (e.g. the Python process
186+
* started with `python -m debugpy --listen`).
187+
*/
116188
get pid(): number | undefined {
117189
return this.transport.pid;
118190
}
@@ -130,6 +202,11 @@ export class DapClient {
130202
this.transport.write(header + json);
131203
}
132204

205+
private handleIncomingData(chunk: string): void {
206+
this.buffer += chunk;
207+
this.processBuffer();
208+
}
209+
133210
private processBuffer(): void {
134211
while (true) {
135212
// Look for Content-Length header
@@ -139,7 +216,8 @@ export class DapClient {
139216
const header = this.buffer.slice(0, headerEnd);
140217
const match = /Content-Length:\s*(\d+)/i.exec(header);
141218
if (!match?.[1]) {
142-
// Malformed header, skip past it
219+
// Malformed header; skip past it and log so we know something's off.
220+
this.logger?.warn("malformed_header", { header: header.slice(0, 120) });
143221
this.buffer = this.buffer.slice(headerEnd + 4);
144222
continue;
145223
}
@@ -159,43 +237,51 @@ export class DapClient {
159237
}
160238

161239
private handleMessage(data: string): void {
162-
let parsed: DebugProtocol.ProtocolMessage;
240+
let raw: unknown;
163241
try {
164-
parsed = JSON.parse(data) as DebugProtocol.ProtocolMessage;
165-
} catch {
242+
raw = JSON.parse(data);
243+
} catch (err) {
244+
this.logger?.warn("json_parse_error", {
245+
error: err instanceof Error ? err.message : String(err),
246+
data: data.slice(0, 200),
247+
});
248+
return;
249+
}
250+
251+
const parsed = parseProtocolMessage(raw);
252+
if (!parsed) {
253+
this.logger?.warn("schema_parse_error", { data: data.slice(0, 200) });
166254
return;
167255
}
256+
168257
if (parsed.type === "response") {
169-
const response = parsed as DebugProtocol.Response;
170258
this.logger?.trace("recv", {
171-
command: response.command,
172-
seq: response.request_seq,
173-
success: response.success,
174-
body: response.body,
259+
command: parsed.command,
260+
seq: parsed.request_seq,
261+
success: parsed.success,
262+
body: parsed.body,
175263
});
176264

177-
const pending = this.pending.get(response.request_seq);
265+
const pending = this.pending.get(parsed.request_seq);
178266
if (!pending) return;
179-
this.pending.delete(response.request_seq);
267+
this.pending.delete(parsed.request_seq);
180268
clearTimeout(pending.timer);
181269

182-
if (!response.success) {
270+
if (!parsed.success) {
183271
pending.reject(
184-
new Error(`DAP error (${response.command}): ${response.message ?? "unknown error"}`),
272+
new Error(`DAP error (${parsed.command}): ${parsed.message ?? "unknown error"}`),
185273
);
186274
} else {
187-
pending.resolve(response);
188-
}
189-
} else if (parsed.type === "event") {
190-
const event = parsed as DebugProtocol.Event;
191-
this.logger?.trace("event", { event: event.event, body: event.body });
192-
193-
const handlers = this.listeners.get(event.event);
194-
if (handlers) {
195-
for (const handler of handlers) {
196-
handler(event.body);
197-
}
275+
pending.resolve(parsed.body);
198276
}
277+
return;
278+
}
279+
280+
// Event
281+
this.logger?.trace("event", { event: parsed.event, body: parsed.body });
282+
const handlers = this.listeners.get(parsed.event);
283+
if (handlers) {
284+
for (const handler of handlers) handler(parsed.body);
199285
}
200286
}
201287

0 commit comments

Comments
 (0)