refactor: address second-pass review feedback

BNasraoui · BNasraoui · commit f1e56e7efd6d · 2026-04-15T13:52:48.000+10:00
Four cleanup items from the second review:

1. Delete dead translateArgsSnakeToCamel — the new translateToolArgsJsonString
   inlined its logic on the JSON-string path. The exported function was
   imported but never called. Removed the export too; nothing else uses it.

2. Consolidate outbound translation into translateArgsOpencodeToClaude.
   The inbound path was cleanly centralized in translateToolArgsJsonString
   but the outbound path in index.ts was still a wall of inline
   if (block.name === …) branches. Now both directions are single-call,
   so the next bug fix touches one place. Adds a round-trip unit test to
   verify the two functions are proper inverses for translatable keys.

3. Skip JSON.parse + JSON.stringify for pass-through events in stream.ts.
   Previously every ping, message_start, message_delta, and text_delta
   got round-tripped "so tests can rely on uniformity" — that's tests
   dictating production behavior, and a measurable waste on the hot
   streaming path. peekType() does a cheap regex sniff of the "type"
   field; only content_block_* get the full parse. Adds tests asserting
   byte-exact passthrough for ping/message_start/text_delta.

4. flush() now logs a debug warning when a tool_use block is abandoned
   mid-stream (upstream disconnected after content_block_start + deltas
   but before content_block_stop). Previously silent — the consumer would
   see half a tool call with no args and nothing would explain why.
diff --git a/src/claude-tools.ts b/src/claude-tools.ts
@@ -142,41 +142,46 @@ export const PARAM_CAMEL_TO_SNAKE: Record<string, Record<string, string>> = {
 };
 
 /**
- * Translate tool_use arguments from Claude's snake_case to OpenCode's camelCase.
- * Returns a new object with translated keys.
+ * Translate tool_use arguments from OpenCode's schema to Claude's schema.
+ * Used on the outbound path (message history being sent back to the API).
+ *
+ * This is the counterpart to translateToolArgsJsonString — they must stay
+ * in lockstep so round-trips preserve meaning.
  */
-export function translateArgsSnakeToCamel(
+export function translateArgsOpencodeToClaude(
   toolName: string,
   args: Record<string, unknown>,
 ): Record<string, unknown> {
-  const map = PARAM_SNAKE_TO_CAMEL[toolName];
-  if (!map || Object.keys(map).length === 0) return args;
-
-  const result: Record<string, unknown> = {};
-  for (const [key, value] of Object.entries(args)) {
-    const newKey = map[key] || key;
-    result[newKey] = value;
+  // 1. Top-level key renames (camelCase → snake_case)
+  const map = PARAM_CAMEL_TO_SNAKE[toolName];
+  let out: Record<string, unknown> = args;
+  if (map && Object.keys(map).length > 0) {
+    out = {};
+    for (const [k, v] of Object.entries(args)) {
+      out[map[k] || k] = v;
+    }
   }
-  return result;
-}
 
-/**
- * Translate tool_use arguments from OpenCode's camelCase to Claude's snake_case.
- * Returns a new object with translated keys.
- */
-export function translateArgsCamelToSnake(
-  toolName: string,
-  args: Record<string, unknown>,
-): Record<string, unknown> {
-  const map = PARAM_CAMEL_TO_SNAKE[toolName];
-  if (!map || Object.keys(map).length === 0) return args;
+  // 2. Agent: translate OpenCode subagent_type values → Claude values
+  if (toolName === "Agent" && typeof out.subagent_type === "string") {
+    const mapped = AGENT_TYPE_OPENCODE_TO_CLAUDE[out.subagent_type];
+    if (mapped) out.subagent_type = mapped;
+  }
 
-  const result: Record<string, unknown> = {};
-  for (const [key, value] of Object.entries(args)) {
-    const newKey = map[key] || key;
-    result[newKey] = value;
+  // 3. TodoWrite: OpenCode { priority, status∈{…,cancelled} } → Claude { activeForm, status∈{pending,in_progress,completed} }
+  if (toolName === "TodoWrite" && Array.isArray(out.todos)) {
+    for (const item of out.todos as Array<Record<string, unknown>>) {
+      if (item.priority && !item.activeForm) {
+        item.activeForm = item.priority;
+        delete item.priority;
+      }
+      if (item.status === "cancelled") {
+        item.status = "completed";
+      }
+    }
   }
-  return result;
+
+  return out;
 }
 
 /**
diff --git a/src/index.test.ts b/src/index.test.ts
@@ -12,6 +12,7 @@ import assert from "node:assert/strict";
 
 import {
   translateToolArgsJsonString,
+  translateArgsOpencodeToClaude,
   AGENT_TYPE_CLAUDE_TO_OPENCODE,
   AGENT_TYPE_OPENCODE_TO_CLAUDE,
 } from "./claude-tools.js";
@@ -234,6 +235,56 @@ describe("translateToolArgsJsonString", () => {
   });
 });
 
+// ── translateArgsOpencodeToClaude: outbound translation on parsed objects ───
+
+describe("translateArgsOpencodeToClaude", () => {
+  it("renames camelCase keys to snake_case for Edit", () => {
+    const out = translateArgsOpencodeToClaude("Edit", {
+      filePath: "/f.ts",
+      oldString: "a",
+      newString: "b",
+      replaceAll: true,
+    });
+    assert.deepEqual(out, {
+      file_path: "/f.ts",
+      old_string: "a",
+      new_string: "b",
+      replace_all: true,
+    });
+  });
+
+  it("maps OpenCode subagent_type values back to Claude", () => {
+    assert.equal(translateArgsOpencodeToClaude("Agent", { subagent_type: "general" }).subagent_type, "general-purpose");
+    assert.equal(translateArgsOpencodeToClaude("Agent", { subagent_type: "build" }).subagent_type, "general-purpose");
+    assert.equal(translateArgsOpencodeToClaude("Agent", { subagent_type: "explore" }).subagent_type, "Explore");
+    assert.equal(translateArgsOpencodeToClaude("Agent", { subagent_type: "plan" }).subagent_type, "Plan");
+  });
+
+  it("renames priority → activeForm in TodoWrite and collapses cancelled → completed", () => {
+    const out = translateArgsOpencodeToClaude("TodoWrite", {
+      todos: [
+        { content: "a", status: "pending", priority: "Doing a" },
+        { content: "b", status: "cancelled", priority: "Was doing b" },
+      ],
+    });
+    assert.deepEqual(out, {
+      todos: [
+        { content: "a", status: "pending", activeForm: "Doing a" },
+        { content: "b", status: "completed", activeForm: "Was doing b" },
+      ],
+    });
+  });
+
+  it("is the inverse of translateToolArgsJsonString for the Edit round trip", () => {
+    // Round-trip: Claude inbound → OpenCode → Claude outbound should be identity
+    // for keys that have a bidirectional mapping.
+    const claudeArgs = { file_path: "/f.ts", old_string: "a", new_string: "b" };
+    const opencodeArgs = JSON.parse(translateToolArgsJsonString(JSON.stringify(claudeArgs), "Edit"));
+    const roundTripped = translateArgsOpencodeToClaude("Edit", opencodeArgs);
+    assert.deepEqual(roundTripped, claudeArgs);
+  });
+});
+
 // ── Agent type map integrity ─────────────────────────────────────────────────
 
 describe("Agent type maps", () => {
@@ -587,6 +638,81 @@ describe("SSE processor: flush", () => {
     // And state is cleared after flush
     assert.equal(proc.flush(), "");
   });
+
+  it("logs a debug warning when a tool_use block is abandoned mid-stream", () => {
+    // Simulates upstream disconnect after start + some deltas but before stop.
+    const messages: string[] = [];
+    const proc = createSseProcessor({
+      inboundToolNameMap: INBOUND_TOOL_NAME_MAP,
+      translateToolArgs: translateToolArgsJsonString,
+      debug: (m) => messages.push(m),
+    });
+    proc.feedChunk(sseEvent("content_block_start", {
+      type: "content_block_start",
+      index: 1,
+      content_block: { type: "tool_use", id: "tu_1", name: "Read", input: {} },
+    }));
+    proc.feedChunk(sseEvent("content_block_delta", {
+      type: "content_block_delta",
+      index: 1,
+      delta: { type: "input_json_delta", partial_json: '{"file_path"' },
+    }));
+    // ...and then the stream ends without a content_block_stop.
+    proc.flush();
+    assert.ok(
+      messages.some((m) => m.includes("abandoned") && m.includes("index=1") && m.includes("tool=Read")),
+      `Expected abandoned-block warning, got: ${JSON.stringify(messages)}`,
+    );
+  });
+});
+
+describe("SSE processor: pass-through optimization", () => {
+  // Pass-through events (message_start, ping, message_delta, text
+  // content_block_delta, etc.) should not be round-tripped through
+  // JSON.parse + JSON.stringify — the output bytes should match the input
+  // exactly when no translation is needed.
+  it("emits the exact input bytes for ping events (no reserialize)", () => {
+    const proc = makeProcessor();
+    const pingFrame = sseEvent("ping", { type: "ping" });
+    const out = proc.feedChunk(pingFrame);
+    assert.equal(out, pingFrame);
+  });
+
+  it("emits the exact input bytes for message_start events", () => {
+    const proc = makeProcessor();
+    // Anthropic message_start payloads include nested objects and arrays —
+    // passthrough must preserve byte-for-byte equality, including any
+    // particular key order the API happens to emit.
+    const frame =
+      "event: message_start\n" +
+      'data: {"type":"message_start","message":{"id":"msg_1","role":"assistant","content":[],"model":"claude","stop_reason":null,"stop_sequence":null,"usage":{"input_tokens":1}}}' +
+      "\n\n";
+    const out = proc.feedChunk(frame);
+    assert.equal(out, frame);
+  });
+
+  it("emits the exact input bytes for text_delta events", () => {
+    const proc = makeProcessor();
+    const frame = sseEvent("content_block_delta", {
+      type: "content_block_delta",
+      index: 0,
+      delta: { type: "text_delta", text: "hello world" },
+    });
+    const out = proc.feedChunk(frame);
+    assert.equal(out, frame);
+  });
+
+  it("still transforms tool_use content_block_start (optimization does not skip interesting events)", () => {
+    const proc = makeProcessor();
+    const input = sseEvent("content_block_start", {
+      type: "content_block_start",
+      index: 1,
+      content_block: { type: "tool_use", id: "tu_1", name: "Read", input: {} },
+    });
+    const out = proc.feedChunk(input);
+    assert.notEqual(out, input, "tool_use start should be transformed (name mapped)");
+    assert.equal(toolUseStartName(out, 1), "read");
+  });
 });
 
 describe("parseSseEvent / buildSseEvent round trip", () => {
diff --git a/src/index.ts b/src/index.ts
@@ -26,10 +26,8 @@ import {
 } from "./constants.js";
 import {
   getClaudeTools,
-  translateArgsSnakeToCamel,
-  translateArgsCamelToSnake,
+  translateArgsOpencodeToClaude,
   translateToolArgsJsonString,
-  AGENT_TYPE_OPENCODE_TO_CLAUDE,
   computeFingerprint,
   extractFirstUserMessageText,
 } from "./claude-tools.js";
@@ -615,34 +613,16 @@ const OpenCodeClaudeBridge = async ({ client }: { client: PluginClient }) => {
                       }
                       if (block.type === "tool_use" && block.name) {
                         block.name = mapOutboundToolName(block.name);
-                        // Translate arguments from OpenCode's camelCase to Claude's snake_case
+                        // Consolidated OpenCode → Claude argument translation:
+                        // key renames, Agent subagent_type values, TodoWrite
+                        // field/status mapping. Mirrors the inbound
+                        // translateToolArgsJsonString so round-trips preserve
+                        // meaning.
                         if (block.input && typeof block.input === "object") {
-                          block.input = translateArgsCamelToSnake(
+                          block.input = translateArgsOpencodeToClaude(
                             block.name,
                             block.input as Record<string, unknown>,
                           );
-                          // Agent: translate OpenCode subagent_type values → Claude values
-                          if (block.name === "Agent" && typeof (block.input as Record<string, unknown>).subagent_type === "string") {
-                            const cur = (block.input as Record<string, unknown>).subagent_type as string;
-                            if (AGENT_TYPE_OPENCODE_TO_CLAUDE[cur]) {
-                              (block.input as Record<string, unknown>).subagent_type = AGENT_TYPE_OPENCODE_TO_CLAUDE[cur];
-                            }
-                          }
-                          // TodoWrite: translate OpenCode fields → Claude fields
-                          // OpenCode: { content, status, priority } with status ∈ {pending, in_progress, completed, cancelled}
-                          // Claude:   { content, status, activeForm } with status ∈ {pending, in_progress, completed}
-                          if (block.name === "TodoWrite" && Array.isArray((block.input as Record<string, unknown>).todos)) {
-                            for (const item of (block.input as Record<string, unknown>).todos as Array<Record<string, unknown>>) {
-                              if (item.priority && !item.activeForm) {
-                                item.activeForm = item.priority;
-                                delete item.priority;
-                              }
-                              // Map "cancelled" → "completed" (Claude doesn't have cancelled)
-                              if (item.status === "cancelled") {
-                                item.status = "completed";
-                              }
-                            }
-                          }
                         }
                       }
                       // tool_result blocks reference tool names via tool_use_id,
diff --git a/src/stream.ts b/src/stream.ts
@@ -40,6 +40,22 @@ export function buildSseEvent(event: string | null, data: string): string {
   return (event ? `event: ${event}\n` : "") + `data: ${data}\n\n`;
 }
 
+/**
+ * Cheap extraction of the `type` field from an SSE data JSON string
+ * without parsing the whole payload. Used to decide whether an event is
+ * interesting enough to round-trip through JSON.parse + JSON.stringify.
+ * Returns null if the type can't be determined from the first few
+ * properties — caller should fall back to a full parse in that case.
+ */
+function peekType(data: string): string | null {
+  // Match the first top-level "type": "..." occurrence. Doesn't handle
+  // escaped quotes in the value, but Anthropic type names are plain
+  // identifiers so that's fine. If anything weird happens, return null
+  // and force a full parse.
+  const m = data.match(/^\s*\{\s*"type"\s*:\s*"([a-z_]+)"/);
+  return m ? m[1] : null;
+}
+
 export type StreamProcessorDeps = {
   /**
    * Map from Claude's tool name (e.g. "Read") to OpenCode's tool name
@@ -80,11 +96,28 @@ export function createSseProcessor(deps: StreamProcessorDeps) {
   const toolBlocks = new Map<number, ToolBlockState>();
   let sseBuffer = "";
 
+  // Event types that require peeking into the JSON to decide whether/how to
+  // mutate. Every other event type is a pure pass-through — no need to
+  // re-serialize, just forward the original bytes.
+  const INTERESTING_TYPES = new Set([
+    "content_block_start",
+    "content_block_delta",
+    "content_block_stop",
+  ]);
+
   function processEvent(block: string): string {
     const parsed = parseSseEvent(block);
     if (!parsed) return block; // comment/keepalive line — pass through
     const { event, data } = parsed;
 
+    // Cheap sniff: if the event isn't one we care about, forward the raw
+    // frame. Saves a JSON.parse + JSON.stringify round trip on every
+    // message_start, ping, message_delta, and text_delta.
+    const cheapType = peekType(data);
+    if (cheapType !== null && !INTERESTING_TYPES.has(cheapType)) {
+      return block;
+    }
+
     let obj: unknown;
     try {
       obj = JSON.parse(data);
@@ -94,7 +127,7 @@ export function createSseProcessor(deps: StreamProcessorDeps) {
     }
 
     if (obj === null || typeof obj !== "object") {
-      return buildSseEvent(event, JSON.stringify(obj));
+      return block;
     }
     const rec = obj as Record<string, any>;
     const type = rec.type as string | undefined;
@@ -128,7 +161,7 @@ export function createSseProcessor(deps: StreamProcessorDeps) {
         state.partialJson += String(rec.delta.partial_json ?? "");
         return "";
       }
-      return buildSseEvent(event, JSON.stringify(rec));
+      return block;
     }
 
     // content_block_stop: if this closed a tool_use block, flush a synthetic
@@ -152,16 +185,15 @@ export function createSseProcessor(deps: StreamProcessorDeps) {
         };
         return (
           buildSseEvent("content_block_delta", JSON.stringify(synthDelta)) +
-          buildSseEvent(event, JSON.stringify(rec))
+          block
         );
       }
-      return buildSseEvent(event, JSON.stringify(rec));
+      return block;
     }
 
-    // Everything else (message_start, ping, text deltas, etc.): pass through.
-    // We still round-trip through JSON.parse/stringify so every event goes
-    // through the same path — tests can rely on this.
-    return buildSseEvent(event, JSON.stringify(rec));
+    // Interesting type we didn't end up mutating (e.g. a non-tool_use
+    // content_block_start, or a text_delta inside a content_block_delta).
+    return block;
   }
 
   function feedChunk(text: string): string {
@@ -180,6 +212,18 @@ export function createSseProcessor(deps: StreamProcessorDeps) {
   function flush(): string {
     const remaining = sseBuffer;
     sseBuffer = "";
+    if (toolBlocks.size > 0) {
+      // The upstream disconnected mid-tool_use without sending
+      // content_block_stop, so we never emitted a synthetic translated
+      // delta for these blocks. The consumer will see the start but no
+      // completed args. Surface it so the operator isn't debugging a
+      // ghost.
+      const orphaned = Array.from(toolBlocks.entries()).map(
+        ([idx, s]) => `index=${idx} tool=${s.toolName} bufferedBytes=${s.partialJson.length}`,
+      );
+      deps.debug?.(`flush: ${toolBlocks.size} tool_use block(s) abandoned mid-stream: ${orphaned.join("; ")}`);
+      toolBlocks.clear();
+    }
     return remaining;
   }
 
