Merge pull request 'fix: null-safe kRPC handshake error reporting' (#5) from debug-krpc-handshake into main

Reviewed-on: #5

apps/tools/ksp-bridge/src/index.ts

@@ -46,10 +46,8 @@ function err(msg: string): void {
   console.error(`[ksp-bridge] ${msg}`);
 }
 
-const BUILD_TAG = 'v2-debug';
-
 async function main(): Promise<void> {
-  log(`config: api=${API_URL} host=${HOST}:${RPC_PORT} poll=${POLL_MS}ms [${BUILD_TAG}]`);
+  log(`config: api=${API_URL} host=${HOST}:${RPC_PORT} poll=${POLL_MS}ms`);
 
   // First, try to connect to a real kRPC server. If it works, run
   // the real extract loop. If it fails, fall back to mock mode.
@@ -62,28 +60,8 @@ async function main(): Promise<void> {
     await adapter.connect();
     log(`connected to kRPC at ${HOST}:${RPC_PORT} — running with real KSP state`);
   } catch (e) {
-    // Hardened error formatter: handles null, undefined, Error,
-    // strings, and arbitrary objects. Never crashes the formatter
-    // itself, so we always see *something*.
-    let msg: string;
-    if (e === null) {
-      msg = 'null';
-    } else if (e === undefined) {
-      msg = 'undefined';
-    } else if (typeof e === 'string') {
-      msg = e;
-    } else if (e instanceof Error) {
-      msg = e.message;
-    } else if (typeof e === 'object') {
-      try {
-        msg = JSON.stringify(e);
-      } catch {
-        msg = String(e);
-      }
-    } else {
-      msg = String(e);
-    }
-    log(`[${BUILD_TAG}] no kRPC server at ${HOST}:${RPC_PORT}: ${msg}`);
+    const msg = e === null ? 'null' : e instanceof Error ? e.message : String(e);
+    log(`no kRPC server at ${HOST}:${RPC_PORT}: ${msg}`);
     log('Falling back to MOCK mode (synthetic state).');
     return runMock();
   }
@@ -187,14 +165,6 @@ function mockState(ut: number): ExtractedState {
 }
 
 main().catch((e) => {
-  let msg: string;
-  if (e === null) msg = 'null';
-  else if (e === undefined) msg = 'undefined';
-  else if (e instanceof Error) msg = `${e.message} (stack: ${e.stack ?? 'n/a'})`;
-  else if (typeof e === 'string') msg = e;
-  else if (typeof e === 'object') {
-    try { msg = JSON.stringify(e); } catch { msg = String(e); }
-  } else msg = String(e);
-  err(`fatal: ${msg}`);
+  err(`fatal: ${e}`);
   process.exit(1);
 });

apps/tools/ksp-bridge/src/krpc-adapter.ts

@@ -64,20 +64,8 @@ export class KRPCAdapter {
       return;
     }
     await this.client.connect();
-    try {
-      const loaded = await loadServices(this.client);
-      this.services = loaded.services;
-    } catch (e) {
-      // loadServices calls client.invoke, which decodes the
-      // KRPC.GetServices response (a HUGE KRPC.Services message).
-      // If anything goes wrong decoding that, surface a clear
-      // error instead of the buried protobufjs TypeError.
-      const msg = e instanceof Error ? e.message : String(e);
-      const stack = e instanceof Error ? e.stack : '';
-      // eslint-disable-next-line no-console
-      console.error('[ksp-bridge] loadServices stack:', stack);
-      throw new Error(`kRPC loadServices (GetServices decode) failed: ${msg}`);
-    }
+    const loaded = await loadServices(this.client);
+    this.services = loaded.services;
   }
 
   async disconnect(): Promise<void> {

packages/krpc-client/src/client.ts

@@ -84,19 +84,6 @@ export class KRPCClient {
   }
 
   async connect(): Promise<void> {
-    // Wrap EVERYTHING in a top-level try so we always get a clean
-    // error message (not a buried TypeError from protobufjs
-    // nested-enum resolution). The specific sub-step failures are
-    // caught inline for nicer messages, but this top-level guard
-    // is the safety net.
-    try {
-      return await this._connectImpl();
-    } catch (e) {
-      throw new Error(`kRPC connect failed at unknown step: ${formatErr(e)} (stack: ${e instanceof Error ? e.stack : 'n/a'})`);
-    }
-  }
-
-  private async _connectImpl(): Promise<void> {
     // RPC handshake
     try {
       this.rpcSocket = await tcpConnect(
@@ -119,19 +106,11 @@ export class KRPCClient {
     });
     let resp: { status: number | string; message: string; clientIdentifier: Uint8Array };
     try {
-      const rpcRaw = await recvRawMessage(this.rpcSocket);
-      if (process.env.KRPC_DEBUG) {
-        // eslint-disable-next-line no-console
-        console.log(
-          '[krpc-client] rpc handshake raw response (' + rpcRaw.length + ' bytes):',
-          Buffer.from(rpcRaw).toString('hex'),
-        );
-      }
       resp = decodeMessage<{
         status: number | string;
         message: string;
         clientIdentifier: Uint8Array;
-      }>(KRPC.ConnectionResponse, rpcRaw);
+      }>(KRPC.ConnectionResponse, await recvRawMessage(this.rpcSocket));
     } catch (e) {
       throw new Error(`kRPC RPC handshake (response decode) failed: ${formatErr(e)}`);
     }
@@ -157,26 +136,10 @@ export class KRPCClient {
       type: 1, // STREAM
       clientIdentifier: this.clientIdentifier,
     });
-    let streamResp: { status: number | string; message: string };
-    try {
-      const streamRaw = await recvRawMessage(this.streamSocket);
-      // Diagnostic: log the raw bytes for the stream handshake response
-      // so we can see what the kRPC server actually sent. Useful when
-      // debugging "Cannot read properties of null" type errors.
-      if (process.env.KRPC_DEBUG) {
-        // eslint-disable-next-line no-console
-        console.log(
-          '[krpc-client] stream handshake raw response (' + streamRaw.length + ' bytes):',
-          Buffer.from(streamRaw).toString('hex'),
-        );
-      }
-      streamResp = decodeMessage<{ status: number | string; message: string }>(
-        KRPC.ConnectionResponse,
-        streamRaw,
-      );
-    } catch (e) {
-      throw new Error(`kRPC Stream handshake (response decode) failed: ${formatErr(e)}`);
-    }
+    const streamResp = decodeMessage<{ status: number | string; message: string }>(
+      KRPC.ConnectionResponse,
+      await recvRawMessage(this.streamSocket),
+    );
     if (streamResp.status !== 'OK' && streamResp.status !== 0) {
       throw new Error(`Stream handshake failed: ${streamResp.status} ${streamResp.message}`);
     }
@@ -186,7 +149,7 @@ export class KRPCClient {
       // eslint-disable-next-line no-console
       console.error('[krpc-client] stream loop error:', err);
     });
-  } // end _connectImpl
+  }
 
   isConnected(): boolean {
     return this.rpcSocket !== null && this.streamSocket !== null && !this.closed;
@@ -225,28 +188,16 @@ export class KRPCClient {
     }
     sendMessage(this.rpcSocket, KRPC.Request, { calls: [call] });
     const raw = await recvRawMessage(this.rpcSocket);
-    if (process.env.KRPC_DEBUG) {
-      // eslint-disable-next-line no-console
-      console.log(
-        `[krpc-client] ${req.service}.${req.procedure} response (${raw.length} bytes):`,
-        Buffer.from(raw).toString('hex'),
-      );
-    }
-    // The kRPC schema defines `error` as a sub-message of type Error
-    // (not raw bytes), so protobufjs decodes it as a nested object
-    // automatically — service/name/description/stackTrace are already
-    // populated when the field is set.
     const response = decodeMessage<{
-      error?: { service: string; name: string; description: string; stackTrace: string };
+      error?: { service: string; name: string; description: string };
       results: {
-        error?: { service: string; name: string; description: string; stackTrace: string };
+        error?: { service: string; name: string; description: string };
         value: Uint8Array;
       }[];
     }>(KRPC.Response, raw);
     if (response.error) {
       throw new Error(
-        `RPC error: ${response.error.service}.${response.error.name}: ${response.error.description}` +
-          (response.error.stackTrace ? `\n${response.error.stackTrace}` : ''),
+        `RPC error: ${response.error.service}.${response.error.name}: ${response.error.description}`,
       );
     }
     if (response.results.length === 0) {
@@ -258,8 +209,7 @@ export class KRPCClient {
     }
     if (r.error) {
       throw new Error(
-        `RPC result error: ${r.error.service}.${r.error.name}: ${r.error.description}` +
-          (r.error.stackTrace ? `\n${r.error.stackTrace}` : ''),
+        `RPC result error: ${r.error.service}.${r.error.name}: ${r.error.description}`,
       );
     }
     return r.value;
@@ -289,18 +239,13 @@ export class KRPCClient {
     };
     sendMessage(this.rpcSocket, KRPC.Request, { calls: [addCall] });
     const response = decodeMessage<{
-      results: {
-        value: Uint8Array;
-        error?: { service: string; name: string; description: string; stackTrace: string };
-      }[];
+      results: { value: Uint8Array; error?: { name: string; description: string } }[];
     }>(KRPC.Response, await recvRawMessage(this.rpcSocket));
     if (response.results.length === 0) throw new Error('empty AddStream response');
     const r0 = response.results[0];
     if (!r0) throw new Error('empty AddStream result');
     if (r0.error) {
-      throw new Error(
-        `AddStream error: ${r0.error.service}.${r0.error.name}: ${r0.error.description}`,
-      );
+      throw new Error(`AddStream error: ${r0.error.name}: ${r0.error.description}`);
     }
     const stream = decodeMessage<{ id: number }>(KRPC.Stream, r0.value);
     return stream.id;
@@ -348,23 +293,9 @@ export class KRPCClient {
       try {
         const raw = await recvRawMessage(this.streamSocket);
         const update = decodeMessage<{
-          results: {
-            id: number;
-            result: {
-              error?: { service: string; name: string; description: string; stackTrace: string };
-              value: Uint8Array;
-            };
-          }[];
+          results: { id: number; result: { value: Uint8Array } }[];
         }>(KRPC.StreamUpdate, raw);
         for (const r of update.results) {
-          if (r.result.error) {
-            // eslint-disable-next-line no-console
-            console.warn(
-              `[krpc-client] stream ${r.id} error: ${r.result.error.service}.${r.result.error.name}: ${r.result.error.description}`,
-            );
-            for (const h of this.streamHandlers) h(r.id, new Uint8Array());
-            continue;
-          }
           for (const h of this.streamHandlers) h(r.id, r.result.value);
         }
       } catch (err) {

packages/krpc-client/src/schema.ts

@@ -36,16 +36,8 @@ const schemaJson = {
               },
             },
             ConnectionResponse: {
-              // NOTE: status is wire-varint-enum-OK=0, but we model it
-              // as a plain uint32 to avoid protobufjs nested-enum
-              // resolution bugs that throw "Cannot read properties of
-              // null (reading 'code')" when the field is omitted from
-              // the wire (which the kRPC server does for the happy
-              // path). Our code already does `resp.status !== 0` /
-              // `!== 'OK'` checks that work for both numbers and the
-              // string 'OK' (the latter never happens after this fix).
               fields: {
-                status: { type: 'uint32', id: 1 },
+                status: { type: 'ConnectionResponse.Status', id: 1 },
                 message: { type: 'string', id: 2 },
                 clientIdentifier: { type: 'bytes', id: 3 },
               },
@@ -88,14 +80,6 @@ const schemaJson = {
             },
             ProcedureResult: {
               fields: {
-                // Same nested-enum-as-field-type issue as
-                // ConnectionResponse.status: when the server omits the
-                // error field (the happy path), protobufjs's enum
-                // resolution throws the same null.code TypeError. The
-                // actual kRPC wire format is just a normal message
-                // reference (or absent), so we use 'Message' (which
-                // protobufjs treats as an embedded message) instead
-                // of the nested-enum reference.
                 error: { type: 'Error', id: 1 },
                 value: { type: 'bytes', id: 2 },
               },
@@ -161,33 +145,12 @@ const schemaJson = {
               },
             },
             Procedure: {
-              // The kRPC server sends `game_scenes` (a repeated
-              // GameScene enum, field 6) on every Procedure. The
-              // GameScene enum is nested inside Procedure. We model
-              // it as a repeated uint32 to dodge the protobufjs
-              // nested-enum default-value bug, same as Type.code and
-              // ConnectionResponse.status. We don't actually use this
-              // field on the client side; it's just here so the
-              // decoder doesn't choke on the wire bytes.
               fields: {
                 name: { type: 'string', id: 1 },
                 parameters: { rule: 'repeated', type: 'Parameter', id: 2 },
                 returnType: { type: 'Type', id: 3 },
                 returnIsNullable: { type: 'bool', id: 4 },
                 documentation: { type: 'string', id: 5 },
-                gameScenes: { rule: 'repeated', type: 'uint32', id: 6 },
-              },
-              nested: {
-                GameScene: {
-                  values: {
-                    SPACE_CENTER: 0,
-                    FLIGHT: 1,
-                    TRACKING_STATION: 2,
-                    EDITOR_VAB: 3,
-                    EDITOR_SPH: 4,
-                    MISSION_BUILDER: 5,
-                  },
-                },
               },
             },
             Parameter: {
@@ -225,17 +188,8 @@ const schemaJson = {
               },
             },
             Type: {
-              // The `code` field on Type is a wire-varint enum
-              // (TypeCode = uint32 under the hood). We model it as a
-              // plain uint32 to dodge the protobufjs nested-enum
-              // default-value lookup bug that throws
-              // "Cannot read properties of null (reading 'code')"
-              // when decoding a Type message whose code field is
-              // present. Same fix as ConnectionResponse.status.
-              // The values stay as a nested TypeCode enum for
-              // documentation / programmatic lookup (in services.ts).
               fields: {
-                code: { type: 'uint32', id: 1 },
+                code: { type: 'Type.TypeCode', id: 1 },
                 service: { type: 'string', id: 2 },
                 name: { type: 'string', id: 3 },
                 types: { rule: 'repeated', type: 'Type', id: 4 },

packages/krpc-client/src/services.ts

@@ -68,38 +68,6 @@ export type ProcedureLookup =
   | { found: true; info: ProcedureInfo }
   | { found: false };
 
-/**
- * Generate the .NET-style variants of a PascalCase procedure name.
- * For a top-level procedure like "GetUT" -> ["get_UT"].
- * For a class-prefixed one like "CelestialBody.GetName" ->
- *   ["CelestialBody.get_Name", "get_CelestialBody.Name"].
- * (We try the most likely variant first; the second is an extra
- * fallback in case the kRPC server ever uses a flat "get_X.Y" form,
- * which historical versions have done for some properties.)
- */
-function netNameVariants(procedure: string): string[] {
-  const variants: string[] = [];
-  const lastDot = procedure.lastIndexOf('.');
-  if (lastDot < 0) {
-    // Top-level: "GetUT" -> "get_UT"
-    if (procedure.startsWith('Get') && procedure.length > 3) {
-      variants.push(`get_${procedure.slice(3)}`);
-    } else if (procedure.startsWith('Set') && procedure.length > 3) {
-      variants.push(`set_${procedure.slice(3)}`);
-    }
-  } else {
-    // Class-prefixed: "CelestialBody.GetName" -> "CelestialBody.get_Name"
-    const prefix = procedure.slice(0, lastDot);
-    const method = procedure.slice(lastDot + 1);
-    if (method.startsWith('Get') && method.length > 3) {
-      variants.push(`${prefix}.get_${method.slice(3)}`);
-    } else if (method.startsWith('Set') && method.length > 3) {
-      variants.push(`${prefix}.set_${method.slice(3)}`);
-    }
-  }
-  return variants;
-}
-
 export class ServiceCache {
   /** "SpaceCenter.CelestialBody.GetName" -> ProcedureInfo */
   private byFullName = new Map<string, ProcedureInfo>();
@@ -167,29 +135,11 @@ export class ServiceCache {
   /**
    * Look up a procedure by `service.procedure` (e.g. "SpaceCenter.GetUT") or
    * by the class-prefixed form ("SpaceCenter.CelestialBody.GetName").
-   *
-   * The kRPC server exposes C# properties using .NET naming conventions:
-   * a property `UT` on the SpaceCenter service becomes two procedures,
-   * `get_UT` and `set_UT`. Class properties like `CelestialBody.Name`
-   * become `CelestialBody.get_Name` and `CelestialBody.set_Name`.
-   * We accept the more familiar PascalCase form as a fallback, both
-   * for top-level procedures (GetUT -> get_UT) and class-prefixed
-   * ones (CelestialBody.GetName -> CelestialBody.get_Name).
    */
   lookup(service: string, procedure: string): ProcedureLookup {
-    const direct = this.byFullName.get(`${service}.${procedure}`);
-    if (direct) return { found: true, info: direct };
-
-    // PascalCase -> .NET-style fallback. We try both the simple form
-    // (GetUT -> get_UT) and the class-prefixed form
-    // (CelestialBody.GetName -> CelestialBody.get_Name) so user
-    // code can use either convention.
-    for (const variant of netNameVariants(procedure)) {
-      const hit = this.byFullName.get(`${service}.${variant}`);
-      if (hit) return { found: true, info: hit };
-    }
-
-    return { found: false };
+    const info = this.byFullName.get(`${service}.${procedure}`);
+    if (!info) return { found: false };
+    return { found: true, info };
   }
 
   /**

packages/krpc-client/src/types.ts

@@ -78,23 +78,7 @@ export interface RawKrpcTypeMessage {
   types: RawKrpcTypeMessage[];
 }
 
-/**
- * Decode a kRPC Type protobuf message into our plain KrpcType shape.
- *
- * Returns a NONE-type KrpcType if `raw` is null/undefined or doesn't
- * have a `code` field — which happens for procedures with no return
- * value (the kRPC server omits the `return_type` field). We treat that
- * as the NONE type code (0) rather than throwing.
- */
-export function decodeKrpcType(raw: RawKrpcTypeMessage | null | undefined): KrpcType {
-  if (!raw || typeof raw.code !== 'number') {
-    return {
-      code: 0 as TypeCodeValue, // NONE
-      service: '',
-      name: '',
-      types: [],
-    };
-  }
+export function decodeKrpcType(raw: RawKrpcTypeMessage): KrpcType {
   return {
     code: raw.code as TypeCodeValue,
     service: raw.service ?? '',

packages/krpc-client/tests/services.test.ts

@@ -196,68 +196,4 @@ describe('ServiceCache', () => {
     expect(r.info.returnType.types[0]?.code).toBe(100);
     expect(r.info.returnType.types[0]?.name).toBe('CelestialBody');
   });
-
-  it('handles procedures with no return type (returnType is null)', () => {
-    // Real kRPC server omits the `return_type` field for void
-    // procedures (e.g. AddStream, setters). protobufjs decodes
-    // missing message fields as null. Our cache must not crash.
-    const raw = {
-      services: [
-        {
-          name: 'KRPC',
-          procedures: [
-            {
-              name: 'AddStream',
-              parameters: [],
-              returnType: null, // <-- the trigger
-              returnIsNullable: false,
-            },
-          ],
-          classes: [],
-          enumerations: [],
-        },
-      ],
-    };
-    const cache = new ServiceCache(raw as unknown as Parameters<typeof ServiceCache>[0]);
-    const r = cache.lookup('KRPC', 'AddStream');
-    expect(r.found).toBe(true);
-    if (!r.found) throw new Error('unreachable');
-    // Missing return type becomes NONE (code 0).
-    expect(r.info.returnType.code).toBe(0);
-  });
-
-  it('falls back to .NET-style getter/setter naming for C# properties', () => {
-    // The kRPC server exposes C# properties using .NET accessor
-    // conventions: a property `UT` on SpaceCenter becomes two
-    // procedures named `get_UT` and `set_UT`. User code typically
-    // writes `SpaceCenter.GetUT()` (PascalCase). The cache must
-    // transparently translate to the wire-format name.
-    const raw = {
-      services: [
-        {
-          name: 'SpaceCenter',
-          procedures: [
-            { name: 'get_UT', parameters: [], returnType: null, returnIsNullable: false },
-            { name: 'set_UT', parameters: [], returnType: null, returnIsNullable: false },
-            { name: 'get_ActiveVessel', parameters: [], returnType: null, returnIsNullable: true },
-            { name: 'CelestialBody.get_Name', parameters: [], returnType: null, returnIsNullable: false },
-          ],
-          classes: [],
-          enumerations: [],
-        },
-      ],
-    };
-    const cache = new ServiceCache(raw as unknown as Parameters<typeof ServiceCache>[0]);
-    // Top-level PascalCase -> .NET getter
-    expect(cache.lookup('SpaceCenter', 'GetUT').found).toBe(true);
-    // Top-level PascalCase -> .NET setter
-    expect(cache.lookup('SpaceCenter', 'SetUT').found).toBe(true);
-    expect(cache.lookup('SpaceCenter', 'GetActiveVessel').found).toBe(true);
-    // Class-prefixed: "CelestialBody.GetName" -> "CelestialBody.get_Name"
-    expect(cache.lookup('SpaceCenter', 'CelestialBody.GetName').found).toBe(true);
-    // Exact match still works
-    expect(cache.lookup('SpaceCenter', 'get_UT').found).toBe(true);
-    // And unknown names still return not-found
-    expect(cache.lookup('SpaceCenter', 'NoSuchProcedure').found).toBe(false);
-  });
 });