Skip to content

feat(translation): API translation proxy — enable Claude integration cross-platform#152

Merged
dean0x merged 51 commits into
mainfrom
feat/api-translation-proxy
Apr 27, 2026
Merged

feat(translation): API translation proxy — enable Claude integration cross-platform#152
dean0x merged 51 commits into
mainfrom
feat/api-translation-proxy

Conversation

@dean0x
Copy link
Copy Markdown
Owner

@dean0x dean0x commented Apr 23, 2026

Summary

Implement a translation proxy infrastructure that enables API translation from other LLM providers to Claude SDK-compatible JSON. This supports multi-agent orchestration across different provider ecosystems (OpenAI, Anthropic SDK, Codex, Gemini, etc.).

Changes

  • ProxyManager: Core translation orchestration with codec registry and middleware support
  • API Translation Codecs: Provider-specific implementations (OpenAI→Claude, Codex→Claude, Gemini→Claude)
  • ProxiedClaudeAdapter: Wraps MCP adapters to intercept and translate outbound API calls
  • Bootstrap Integration: Configure translation proxies via translate field in AgentConfig
  • Test Suite: Full TDD coverage of IR, codecs, middleware, and proxy behaviors

Breaking Changes

None

Testing

  • ✅ IR (Intermediate Representation) codec tests
  • ✅ Provider-specific codec tests
  • ✅ Middleware chain tests
  • ✅ ProxyManager lifecycle tests
  • ✅ Bootstrap integration tests
  • ✅ All new test suites passing

Related Issues

Closes #152 (API translation proxy core)

Dean Sharon and others added 8 commits April 23, 2026 19:58
… proxy

133 tests covering:
- Canonical IR type-level checks (discriminated unions, all variants)
- AnthropicCodec: parseRequest (18 cases), serializeResponse (7), stream serializer (8)
- OpenAICodec: serializeRequest (15 cases), parseResponse (11), stream parser state machine (10)
- ToolNameMappingMiddleware: truncation/reverse-map/uniqueness (9 cases)
- PromptCacheMiddleware: cache hit/miss detection, no-double-count (5 cases)
- LoggingMiddleware: metadata-only, no key/body leakage (6 cases)
- LineBuffer: CRLF, partial lines, flush (11 cases)
- StreamTranslator: end-to-end SSE translation (9 cases)
- TranslationProxy: HTTP server round trips, error mapping, 413, streaming (10 cases)

Co-Authored-By: Claude <noreply@anthropic.com>
Adds the foundational translation layer for proxying Anthropic Messages API
requests to OpenAI Chat Completions API (and back) transparently.

Components:
- src/translation/ir.ts: Canonical IR types (lingua franca between codecs)
- src/translation/codec.ts: FormatCodec/StreamParser/StreamSerializer interfaces
- src/translation/codecs/anthropic-codec.ts: Source codec (Claude Code wire format)
- src/translation/codecs/openai-codec.ts: Target codec with SSE stream parser state machine
- src/translation/middleware/middleware.ts: Pipeline runner (request fwd, response/stream reverse)
- src/translation/middleware/tool-name-mapping.ts: 64-char limit truncation + SHA256 uniqueness
- src/translation/middleware/prompt-cache.ts: Metrics-only prefix-hash cache tracking
- src/translation/middleware/logging.ts: Structured metadata logging, never logs API keys
- src/translation/proxy/line-buffer.ts: SSE chunk boundary assembler (CRLF-aware)
- src/translation/proxy/stream-translator.ts: OpenAI→Anthropic SSE event pipeline
- src/translation/proxy/translation-proxy.ts: HTTP server, error mapping, 50MB limit, timeouts

Security invariants enforced:
- NEVER forwards inbound x-api-key; uses config.targetApiKey only
- Strips ALL anthropic-* headers before forwarding
- Never includes API keys in error messages or logs
- Local server uses HTTP (loopback-only, intentional; outbound uses HTTPS)

Snyk finding: CWE-319 Medium — intentional ARCHITECTURE EXCEPTION for loopback server

Co-Authored-By: Claude <noreply@anthropic.com>
…lation proxy

Adds ProxyManager service and ProxiedClaudeAdapter that enable Claude Code to route
Anthropic Messages API requests through a local translation proxy to OpenAI-compatible backends.

- src/translation/proxy/proxy-manager.ts: ProxyManager lifecycle service; loadProxyConfig()
  reads agents.claude.proxy config; non-fatal startup failure falls back to direct API
- src/translation/proxy/proxied-claude-adapter.ts: ClaudeAdapter subclass that overrides
  resolveBaseUrl() to inject proxy URL (ANTHROPIC_BASE_URL=http://127.0.0.1:<port>)
- src/bootstrap.ts: optional proxy startup before agentRegistry registration; registers
  ProxiedClaudeAdapter when proxy config exists; skipped when processSpawner is injected (tests)
- tests: 3 new test files covering ProxyManager lifecycle, ProxiedClaudeAdapter spawn env,
  and loadProxyConfig config-file parsing (23 new tests, 156 total in translation suite)
…Adapter tests

Dynamic import() in beforeEach returned a different module instance than the
vi.mock() declaration, causing mockSpawn.mock.calls to be empty in test assertions.
Fix: use static top-level import after vi.mock() calls, matching the established
pattern from agent-adapters.test.ts.
…tibility

Tests exposed resolveBaseUrl via test subclass instead of mocking
child_process.spawn — prior test files in the same vitest fork loaded
child_process unmocked, making vi.mock ineffective with isolate: false.
…ration

When `translate: 'openai'` is set on an agent, the existing baseUrl,
apiKey, and model fields become the target backend config for the
translation proxy. This keeps configuration simple — users set 4
fields via the same `beat agents config set` / `ConfigureAgent` flow.

Changes:
- AgentConfig: add translate? field; saveAgentConfig key union extended
- loadProxyConfig: reads translate + baseUrl/apiKey/model from AgentConfig
- MCP: ConfigureAgent accepts translate in set/check; ListAgents shows it
- CLI: agents config set/show support translate key
- MCP instructions: document API translation feature and setup steps
- Tests: updated bootstrap-proxy-integration + proxied-claude-adapter tests
- Remove identity function serializeStopReason from AnthropicCodec
- Remove unused messageId/messageModel fields from AnthropicStreamSerializer
- Tighten OpenAIMessage.content type from unknown to string|array|null
- Remove unused contentType and processedRequest vars from TranslationProxy
- Collapse redundant dual-condition in PromptCacheMiddleware.hashPrefix
…ning stream, shutdown

- Add config validation warnings when translate is set without baseUrl/apiKey/model
  (MCP ConfigureAgent + CLI agents config set)
- Handle streaming reasoning_content in OpenAIStreamParser → thinking_delta events
- Add document content type fallback in OpenAI codec (converted to text placeholder)
- Add proxyManager.stop() to MCP server graceful shutdown handler
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 23, 2026

Confidence Score: 5/5

Safe to merge — all known P1 issues have been resolved; remaining findings are observability gaps and an edge-case flush omission

No P0 or P1 findings remain. The three P2 items are: (1) streaming abort returns 502 instead of 499, (2) LoggingMiddleware response log never fires on the streaming path, and (3) flush() doesn't close active tool calls on abnormal stream termination. None affect correctness on the happy path.

src/translation/codecs/openai-codec.ts (flush gap), src/translation/middleware/logging.ts (streaming observability), src/translation/proxy/translation-proxy.ts (abort code inconsistency)

Important Files Changed

Filename Overview
src/translation/proxy/translation-proxy.ts Core HTTP proxy server — well-structured with proper per-request middleware isolation; minor inconsistency in streaming abort error code (502 vs 499)
src/translation/codecs/openai-codec.ts OpenAI stream parser — prior tool-call re-keying bug fixed via openaiToCanonicalIndex; flush() doesn't close active tool calls, creating a gap for malformed/truncated streams
src/translation/codecs/anthropic-codec.ts Anthropic source codec — thinking block lifecycle (start/delta/stop) correctly implemented; serialization and parsing look correct
src/translation/proxy/proxy-manager.ts ProxyManager lifecycle — shared PromptCacheState correctly hoisted out of the per-request factory; start/stop are idempotent
src/translation/middleware/logging.ts LoggingMiddleware — processResponse is dead for streaming (dominant path); response-level debug log is never emitted in practice
src/translation/middleware/prompt-cache.ts PromptCacheMiddleware — shared state cross-request race fixed; lastPrefixHash still never updated on streaming path (processStreamEvent absent), leaving cache-hit annotation permanently inactive for streaming
src/translation/middleware/tool-name-mapping.ts ToolNameMappingMiddleware — deterministic truncation with SHA suffix prevents collisions; per-request instances prevent cross-request map bleed
src/translation/proxy/proxied-claude-adapter.ts ProxiedClaudeAdapter — cleanly overrides resolveBaseUrl/resolveModel/resolveAuth to isolate Claude Code from backend config; constructor-injected port is stable
src/bootstrap.ts Bootstrap integration — proxy startup eagerly awaited before agentRegistry factory runs; fatal-on-config-exists policy prevents silent fallback
src/translation/proxy/stream-translator.ts StreamTranslator — reversed middleware pre-computed at construction; SSE line routing correct; flush delegates to parser

Sequence Diagram

sequenceDiagram
    participant CC as Claude Code
    participant PA as ProxiedClaudeAdapter
    participant TP as TranslationProxy (127.0.0.1:PORT)
    participant MW as Middleware Pipeline
    participant ST as StreamTranslator
    participant OB as OpenAI Backend

    CC->>PA: spawn with ANTHROPIC_BASE_URL
    CC->>TP: POST /v1/messages (Anthropic format)
    TP->>TP: AnthropicCodec.parseRequest
    TP->>MW: runRequestMiddleware
    MW-->>TP: processed CanonicalRequest
    TP->>TP: OpenAICodec.serializeRequest
    alt Streaming
        TP->>OB: POST /chat/completions stream=true
        OB-->>TP: SSE chunks
        loop per SSE line
            TP->>ST: processLine
            ST->>MW: applyMiddleware processStreamEvent
            ST-->>CC: Anthropic SSE event
        end
        TP->>ST: flush()
    else Non-streaming
        TP->>OB: POST /chat/completions
        OB-->>TP: JSON response
        TP->>MW: runResponseMiddleware
        TP-->>CC: 200 Anthropic JSON
    end
Loading

Reviews (7): Last reviewed commit: "refactor: simplify resolver fixes — remo..." | Re-trigger Greptile

Comment thread src/translation/proxy/translation-proxy.ts Outdated
@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 23, 2026

Code Review Summary: 17 Blocking Issues Found

Created: 2026-04-23 | Branch: feat/api-translation-proxy → main

Critical Issues by Category

Architecture (3 HIGH - Stateful Middleware Concurrency)

  • LoggingMiddleware, PromptCacheMiddleware, ToolNameMappingMiddleware all share instance state across concurrent requests, causing metrics corruption and tool name corruption under load. Fix: Create per-request middleware instances.
    • src/translation/middleware/logging.ts:23
    • src/translation/middleware/prompt-cache.ts:77
    • src/translation/middleware/tool-name-mapping.ts:33

Complexity (4 HIGH - Function Length + Cyclomatic Complexity)

  • handleStreamingRequest: 161 lines, 5-level nesting (threshold: 50-200)
  • handleNonStreamingRequest: 104 lines, 5-level nesting
  • OpenAIStreamParser.processChunk: CC ~18
  • buildOpenAIMessages: CC ~14

TypeScript (4 MEDIUM - Type Safety)

  • Line 150: Unreachable dead code from double type checks
  • Line 342: Non-null assertion without guard (could fail on malformed stream)
  • Line 311: Missing exhaustive switch (won't catch new event types)
  • Configuration.ts:251: translate typed as string instead of union

Security (2 MEDIUM - Input Validation)

  • Line 221: URL reflected verbatim in error response (inject/confuse risk)
  • mcp-adapter.ts:350: translate accepts any string, silent fail for invalid values

Performance (2 MEDIUM - Hot Path Inefficiencies)

  • Line 53: Array spread on every stream event (hundreds of allocations)
  • Line 557: Per-line res.write() creates 6+ syscalls per chunk

Testing (2 HIGH)

  • middleware.ts:27: Zero direct unit tests for pipeline runner (ordering contracts untested)
  • translation-proxy.ts:204: Five error paths untested (405, 404, 400, 502)

Regression (1 HIGH - Lifecycle)

  • index.ts:75: ProxyManager shutdown missing in CLI mode

Consistency (1 MEDIUM)

  • logging.ts:17: Import path inconsistent (../../ vs ../)

Issues by Confidence Level

≥80% Confidence (17 BLOCKING)
All items above meet ≥80% confidence threshold.

60-79% Confidence (Lower Priority)
See full review reports for architecture coupling, complexity in OpenAICodec, and testing gaps (concurrent requests, JSON fallback streaming, weak assertions).


Detailed Files

File Issues Severity
src/translation/middleware/logging.ts 2 HIGH (concurrency) + MEDIUM (import)
src/translation/middleware/prompt-cache.ts 1 HIGH (concurrency)
src/translation/middleware/tool-name-mapping.ts 1 HIGH (concurrency)
src/translation/proxy/translation-proxy.ts 4 HIGH×2 (functions) + MEDIUM×2 (security, perf)
src/translation/middleware/middleware.ts 2 HIGH (testing) + MEDIUM (perf)
src/translation/codecs/openai-codec.ts 3 MEDIUM (unreachable, assertion, complexity ~18)
src/translation/codecs/anthropic-codec.ts 1 MEDIUM (switch)
src/adapters/mcp-adapter.ts 1 MEDIUM (validation)
src/core/configuration.ts 1 MEDIUM (type safety)
src/index.ts 1 HIGH (regression)

Recommendation

Status: CHANGES_REQUESTED

The translation proxy feature demonstrates strong architectural fundamentals but has three critical concurrency bugs (stateful middleware shared across requests) that must be fixed before merge. Additionally, function decomposition (complexity), test coverage (middleware runner, error paths), and type safety improvements should be addressed.

Next Steps:

  1. Fix stateful middleware (create per-request instances)
  2. Decompose long functions and high-CC methods
  3. Add missing unit tests
  4. Add input validation for translate field
  5. Rescan after fixes

Generated by Claude Code Code Review Agent
All 17 inline comments above at ≥80% confidence

Dean Sharon and others added 7 commits April 23, 2026 23:56
COMP-6: convert parseContentBlock if-chain to switch statement for
clarity and uniform branch structure across 7 content types.

TS-4: add exhaustive never check in AnthropicStreamSerializer.serialize()
default branch so the compiler flags missing cases when new
CanonicalStreamEvent discriminants are added.

CON-1: fix logging middleware import path from '../../translation/ir.js'
to '../ir.js' — consistent with every other file in the same directory.

CON-2: replace inline `as { stop(): Promise<void> }` assertion in index.ts
with a proper ProxyManager import and typed cast.

PERF-1: pre-compute reversed middleware array once in StreamTranslator
constructor rather than allocating a new array on every SSE chunk
(applyMiddleware was called hundreds/thousands of times per streaming
response via runStreamEventMiddleware).

Co-Authored-By: Claude <noreply@anthropic.com>
… to server mode

SEC-2: Add TranslateTarget type union to configuration.ts so the translate field
is typed (not raw string). Validate against supported values at all three entry
points: CLI (agents.ts), MCP Zod schema (z.enum), and config read boundary
(loadAgentConfig). Simplify MCP adapter warning check — truthy test is sufficient
since empty string is falsy.

REG-1: Gate proxy startup on mode === 'server'. CLI modes ('cli', 'run') skip
proxy startup — their spawned processes read their own config. Guard changed from
`!options.processSpawner` to `!options.processSpawner && mode === 'server'`.

ARCH-4: Document the optional untyped proxyManager container registration so
future developers understand callers must handle the missing-key case.

ARCH-SF1: Expand ARCHITECTURE comment on the temporal dependency between proxy
startup and agentRegistry registration — clarifies the ordering constraint and
the per-request middlewareFactory pattern.

Also completes the middlewares → middlewareFactory rename in translation-proxy.ts
and proxy-manager.ts (incomplete refactor that blocked typecheck).

Co-Authored-By: Claude <noreply@anthropic.com>
…lpers, add guards

- Extract buildToolResultMessages() and buildAssistantMessage() from
  buildOpenAIMessages() (COMP-4): reduces function from ~88 lines with
  3 continue statements to a clean 20-line dispatch loop
- Extract closeActiveTextBlock(), closeActiveToolCall(), and
  processToolCallDeltas() from OpenAIStreamParser.processChunk (COMP-3):
  reduces 152-line method with cyclomatic complexity ~18 to a focused
  60-line orchestrator with named helpers
- Replace non-null assertion on activeToolCalls.get() with explicit
  guard (TS-2): malformed streams with continuing deltas after re-keying
  now safely continue rather than throw
- Use type predicate filters (TS-3): filter((c): c is Extract<...>) +
  direct map eliminates redundant type checks and unreachable fallback
  returns in tool_use and tool_result serialization paths

Co-Authored-By: Claude <noreply@anthropic.com>
…xity, security

ARCH-1/2/3: Replace shared middlewares array with per-request middlewareFactory
in TranslationProxyConfig. handleMessages() now calls middlewareFactory() once
per request so LoggingMiddleware, PromptCacheMiddleware, and ToolNameMappingMiddleware
each get isolated mutable state — no cross-request data races under concurrency.

COMP-1+PERF-2: Extract handleStreamingError, handleJsonFallback, handleSseStream
from handleStreamingRequest (was 161 lines, 5 levels). SSE writes are now batched
per backend chunk (one res.write per chunk instead of one per line).

COMP-2: Extract processNonStreamingResponse from handleNonStreamingRequest success
path (was 104 lines, 5 levels). Both streaming and non-streaming JSON paths share
the same parse→middleware→serialize→send helper.

COMP-5: Extract countApproxChars() pure function from handleCountTokens (was
48 lines, 5 levels). Character counting is now separately testable.

SEC-1: Sanitize req.url before including in 404 error message — strip non-printable
ASCII and cap at 200 chars to prevent log injection.

Co-Authored-By: Claude <noreply@anthropic.com>
…ror paths, and streaming fallback

- Add middleware.test.ts: unit tests for runRequestMiddleware (forward order),
  runResponseMiddleware (reverse order), runStreamEventMiddleware (reverse order,
  null-drop, short-circuit), and no-op middleware skipping
- Fix prompt-cache.test.ts: strengthen weak toBeGreaterThanOrEqual(0) assertion to
  toBe(15) to verify no-double-counting invariant
- Add error path tests in translation-proxy.test.ts: 405 non-POST, 404 unknown
  endpoint, 400 invalid JSON body, 502 connection refused, 502 invalid JSON response
- Add streaming JSON fallback test: backend returning application/json to a stream
  request is translated as a normal response (handleJsonFallback path)
- Add DECISION comment at TestableProxiedClaudeAdapter class definition for
  project convention compliance

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread src/translation/codecs/openai-codec.ts
Dean Sharon and others added 3 commits April 24, 2026 00:44
…al index mapping

Add openaiToCanonicalIndex and pendingToolCalls maps to OpenAIStreamParser so
that subsequent delta chunks arriving with the original OpenAI tcIndex are
correctly resolved to the canonical content index after re-keying. Previously,
re-keying deleted the activeToolCalls entry at tcIndex, causing lookup failures
and silently lost arguments on continuation chunks.

Co-Authored-By: Claude <noreply@anthropic.com>
… generic string

Add extractBackendErrorMessage() helper that parses the backend response body
to extract a human-readable error message (OpenAI format: error.message, plain
message field, or raw text). Messages are truncated to 500 chars and fall back
to 'Backend returned error' on empty body. Used in both non-streaming and
streaming error paths. Adds logger.debug tracing for backend error responses.

Co-Authored-By: Claude <noreply@anthropic.com>
- Move substring truncation after JSON.parse in extractBackendErrorMessage
  to prevent truncation from breaking valid JSON before parsing
- Remove dead code branch in pending tool call handler (existing.started
  can never be true while in pendingToolCalls map)
- Simplify type casting in extractBackendErrorMessage
Comment thread src/translation/middleware/prompt-cache.ts
@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 24, 2026

Code Review Summary

Consolidated findings from 8 reviewers (≥80% confidence). Filtering for blocking/high-severity issues:

🔴 Blocking Issues (Must Fix Before Merge)

  1. TypeScript Type Error in Tests (Confidence: 95%)

    • Location: tests/unit/translation/middleware/middleware.test.ts:35
    • Issue: makeStreamEvent() constructs objects with a delta property, but ContentDeltaEvent in src/translation/ir.ts is defined as { type: 'content_delta'; index: number; text: string } — no delta property
    • Impact: 8 TypeScript errors; tests validate the wrong shape
    • Fix: Use the flat text property instead of nested delta.text
  2. Stale JSDoc References (Confidence: 92%)

    • Location: src/core/configuration.ts:232
    • Issue: JSDoc says "kept in sync with SUPPORTED_TRANSLATE_TARGETS in proxy-manager.ts" but that constant was removed in this PR
    • Fix: Update comment to reference agents.ts:23 where it actually lives
  3. Security: Raw Backend Error Forwarding (Confidence: 85%)

    • Location: src/translation/proxy/translation-proxy.ts:145-157
    • Issue: Backend error messages forwarded verbatim to client, could leak internal details (stack traces, hostnames, deployment names)
    • Fix: Return generic summary to client ('Backend returned error'), log full detail at debug level
  4. Security: Unbounded Error Buffer (Confidence: 82%)

    • Location: src/translation/proxy/translation-proxy.ts:450-451, 502-503
    • Issue: Error response bodies have no size cap, malicious backend could cause memory exhaustion
    • Fix: Add MAX_ERR_BYTES guard (64KB) on error body accumulation, matching pattern for inbound request bodies

🟡 High-Severity Issues (Strong Recommendation to Fix)

  1. Dead Code: Unused Return Value (Confidence: 85%)

    • Location: src/translation/proxy/translation-proxy.ts:467, 532
    • Issue: processNonStreamingResponse() returns boolean but return value is ignored at all call sites
    • Fix: Change return type to void to match actual usage
  2. Code Duplication: Middleware Loop (Confidence: 82%)

    • Location: src/translation/proxy/stream-translator.ts:100-109
    • Issue: Reimplements the same loop as runStreamEventMiddleware() with no shared source of truth
    • Fix: Either parameterize runStreamEventMiddleware to accept pre-reversed array, or document that production uses StreamTranslator version
  3. Complex Nesting (76-line method, 5 levels deep) (Confidence: 85%)

    • Location: src/translation/proxy/translation-proxy.ts:417-492
    • Issue: handleNonStreamingRequest still has deep nesting despite prior refactoring
    • Fix: Extract callback into handleBackendNonStreamingResponse() method to flatten nesting

📋 Summary by Category

Severity Count Recommendation
Blocking (Type/Docs/Security) 4 Address before merge
High (Dead code/Duplication/Complexity) 3 Strongly recommended
Lower Confidence (60-79%) 8 Summary comment recommendations

Review Process Notes

  • Deduplicated across 8 reviewers: Architecture, Security, Testing, TypeScript, Consistency, Regression, Complexity, Performance
  • Inline comments created for: Configuration JSDoc, middleware test types, error forwarding, error buffering, return value, middleware duplication, nesting complexity
  • Pre-existing issues identified: 2 flaky CI tests (worker-pool-management.test.ts:157, network-failures.test.ts:312) noted in memory but not blocking this PR

Claude Code Code Review Agent • Confidence-weighted filtering applied

Comment thread tests/unit/translation/middleware/middleware.test.ts
Comment thread src/translation/proxy/translation-proxy.ts
Comment thread src/translation/proxy/translation-proxy.ts Outdated
Comment thread src/translation/proxy/translation-proxy.ts Outdated
Comment thread src/translation/proxy/stream-translator.ts
…t IR type

ContentDeltaEvent is flat { type, index, text } — no nested delta property.
Fix makeStreamEvent(), makeStreamTagger(), and all assertions that were
accessing the non-existent event.delta.text path.

Co-Authored-By: Claude <noreply@anthropic.com>
@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

CRITICAL: Database registration throws instead of returning err()

Line 274: The catch block re-throws non-NODE_MODULE_VERSION errors without wrapping in the Result return type. The bootstrap function returns Promise<Result<Container>> — it should never throw.

Fix: Wrap the re-throw in an err() return:

} catch (error: unknown) {
  const msg = error instanceof Error ? error.message : String(error);
  if (msg.includes("NODE_MODULE_VERSION")) {
    return err(new AutobeatError(...));
  }
  return err(
    new AutobeatError(
      ErrorCode.SYSTEM_ERROR,
      `Failed to initialize database: ${msg}`,
    ),
  );
}

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

HIGH: Database registration pattern deviation needs DECISION comment

Lines 256-275: Registration changed from lazy registerSingleton to eager registerValue. Every other service uses registerSingleton. Document the intentional deviation with a clear comment explaining why database is eagerly instantiated (to catch native module ABI mismatches early).

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

HIGH: URL construction bypasses URL API — src/translation/proxy/translation-proxy.ts:389

String concatenation new URL(targetBaseUrl.replace(/\/$/, "") + "/chat/completions") is fragile for base URLs with paths or query params. Use the URL API correctly:

const base = new URL(this.config.targetBaseUrl);
base.pathname = base.pathname.replace(/\/?$/, "") + "/chat/completions";

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

HIGH: Non-streaming response accumulation unbounded — src/translation/proxy/translation-proxy.ts:484-488, 584-589

Success path accumulates chunks with no size cap, unlike error path (MAX_ERR_BYTES) and inbound limit (MAX_BODY_BYTES). A malicious backend could send very large responses without bounds.

Fix: Add size cap on success paths matching inbound limit:

const chunks: Buffer[] = [];
let totalBytes = 0;
backendRes.on("data", (chunk: Buffer) => {
  totalBytes += chunk.length;
  if (totalBytes <= MAX_BODY_BYTES) chunks.push(chunk);
});
backendRes.on("end", () => {
  if (totalBytes > MAX_BODY_BYTES) {
    sendError(res, 502, "api_error", "Backend response too large");
    return;
  }
  // ... process response
});

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

HIGH: serializeContentBlock missing exhaustive never check — src/translation/codecs/anthropic-codec.ts:178

The switch statement uses a silent fallback return { type: "text", text: "" } instead of exhaustive never checking. If a new CanonicalContent variant (e.g., audio) is added, this function will not be flagged by the compiler. The same file uses exhaustive checking correctly at line 324.

Fix: Add exhaustive check:

case "image":
case "document":
case "json":
case "tool_result":
  return { type: "text", text: "" };
default: {
  const _exhaustive: never = content;
  return { type: "text", text: "" };
}

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

MEDIUM: TLS bypass suggestion in error message — src/utils/url-probe.ts:146

Confidence: 90%. The error suggests NODE_TLS_REJECT_UNAUTHORIZED=0 to work around TLS failures. This is dangerous (OWASP A02) and normalizes disabling certificate verification for the entire Node.js process, exposing all HTTPS connections (including API proxy traffic with API keys) to MITM.

Fix: Replace with safer recommendation:

return `TLS/SSL error connecting to ${urlStr}: ${error.message}. Verify the servers TLS certificate is valid, or configure a custom CA bundle via NODE_EXTRA_CA_CERTS.`;

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

MEDIUM: No protocol scheme restriction on probeUrl — src/utils/url-probe.ts:212-221

Confidence: 82%. The function validates URL parsing but allows any scheme (file://, ftp://, etc.). While the URL eventually goes to http.request which would fail, defense-in-depth would reject non-HTTP schemes at the probe entry point.

Fix: Add scheme validation:

if (parsedUrl.protocol !== "https:" && parsedUrl.protocol !== "http:") {
  return err(new Error(`Unsupported protocol "${parsedUrl.protocol}" — only http: and https: are supported`));
}

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

MEDIUM: rawError cast bypasses unknown narrowing — src/utils/url-probe.ts:101

Confidence: 82%. Line 101 casts rawError (typed unknown) directly to NodeJS.ErrnoException without instanceof guard. The pattern is used correctly elsewhere (bootstrap.ts:262).

Fix: Narrow properly before accessing ErrnoException fields:

const error = rawError instanceof Error
  ? (rawError as NodeJS.ErrnoException)
  : Object.assign(new Error(String(rawError)), { code: undefined }) as NodeJS.ErrnoException;

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

HIGH: handleConfigureAgent exceeds complexity threshold — src/adapters/mcp-adapter.ts:3326

Confidence: 88%. The method spans 220+ lines with 3 case branches and cyclomatic complexity ~15. The set branch alone is 140 lines with 6+ decision points.

Fix: Extract each switch case into dedicated private methods:

private async handleConfigureAgent(args: unknown): Promise<MCPToolResponse> {
  const { agent, action, ...fields } = ConfigureAgentSchema.safeParse(args).data;
  switch (action) {
    case "check": return this.handleConfigureAgentCheck(agent);
    case "set": return this.handleConfigureAgentSet(agent, fields);
    case "reset": return this.handleConfigureAgentReset(agent);
  }
}

This keeps each method under 30 lines and improves maintainability.

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

HIGH: probeUrl adds 5s latency to ConfigureAgent check — src/adapters/mcp-adapter.ts:3357-3364

Confidence: 82%. The check action now awaits a 5s timeout probe unconditionally. Users calling ConfigureAgent check frequently for status will experience 5s stalls if the backend is unreachable.

Fix: Make probe optional via tool schema parameter, or reduce timeout to 2-3s for check action, or make probe fire-and-forget. The set action probe is more acceptable since it only fires on config changes.

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

MEDIUM: Inconsistent response typing in ConfigureAgent — src/adapters/mcp-adapter.ts:3521-3536

Confidence: 82%. The check action defines a typed CheckPayload interface (line 3366), but the set action removed the equivalent SetPayload and now inlines the response object without type annotation. This creates asymmetry within the same handler.

Fix: Either remove CheckPayload too (both inline) or keep both typed. Simpler: inline CheckPayload also for consistency.

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

MEDIUM: argumentsAccumulator string concat causes O(n²) behavior — src/translation/codecs/openai-codec.ts:353,381

Confidence: 80%. Tool call arguments accumulate via += string concatenation. JavaScript strings are immutable, so each concat allocates a new string (n² copies for large payloads).

Fix: Use array accumulator and join at the end:

interface ActiveToolCall {
  id: string;
  name: string;
  argumentsChunks: string[];  // was argumentsAccumulator
  started: boolean;
}
// On delta: tc.argumentsChunks.push(tcArgs);
// On stop: arguments: tc.argumentsChunks.join("")

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

MEDIUM: probeUrl silently swallows deep-probe network errors — src/utils/url-probe.ts:245-247

Confidence: 82%. When the deep probe (GET /models) fails with a network error but the base HEAD probe succeeded, the error is silently discarded. This loses diagnostic information about authentication failures.

Fix: Include a warning in the returned result:

if ("error" in deepResult) {
  const baseMessage = messageForStatus(statusCode, headers, parsedUrl, false);
  return ok({
    reachable: true,
    statusCode,
    message: `${baseMessage}. Note: API key validation failed (${deepResult.error.message})`,
    severity: "warning",
    durationMs,
  });
}

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

MEDIUM: messageForError uses 6 if-chains instead of lookup — src/utils/url-probe.ts:121-151

Confidence: 82%. The function has cyclomatic complexity ~8 with a long chain of if/else-if blocks. The TLS error check alone is a 7-operand disjunction (lines 137-144). Each new error code extends the chain linearly.

Fix: Extract TLS codes to a Set and consider a dispatch table:

const TLS_ERROR_CODES = new Set([
  "CERT_HAS_EXPIRED", "CERT_INVALID",
  "UNABLE_TO_VERIFY_LEAF_SIGNATURE",
  "SELF_SIGNED_CERT_IN_CHAIN", "DEPTH_ZERO_SELF_SIGNED_CERT",
]);

function isTlsError(code: string): boolean {
  return TLS_ERROR_CODES.has(code) || code.startsWith("ERR_TLS") || code.startsWith("ERR_SSL");
}

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

HIGH: ThinkingDeltaEvent type change is breaking — src/translation/ir.ts:218-222

Confidence: 90%. The ThinkingDeltaEvent gained a required index field, breaking any external consumer constructing or pattern-matching on this type.

Note: If src/translation/ir.ts is internal-only (appears to be — not re-exported from src/utils/index.ts), this is acceptable and severity drops to informational. Please confirm the IR types are not part of public API surface.

If external consumers exist, this breaking change needs a CHANGELOG note and migration guide.

@dean0x
Copy link
Copy Markdown
Owner Author

dean0x commented Apr 27, 2026

Review Summary: PR #152

Overall Status: Changes Requested (15 high-confidence issues found)

Issue Breakdown by Severity

  • CRITICAL: 1 (database registration throw)
  • HIGH: 7 (URL API fragility, unbounded response accumulation, exhaustive checking, complexity, probe latency, type regression)
  • MEDIUM: 7 (security, type safety, performance, consistency, diagnostics)

Lower-Confidence Suggestions (60-79%, for consideration)

  1. Dynamic vs static import inconsistency (84%) — agents.ts uses dynamic import for probeUrl, but mcp-adapter.ts uses static. Document or standardize.
  2. SSRF via redirect following (65%) — Current code is safe but future changes could expose internal services.
  3. Probe timeout handling in tests (70%) — Missing edge case: what if probeUrl exceeds 5000ms timeout?
  4. Double loadAgentConfig call (65%) — When translate is set, config is loaded twice in agents.ts.
  5. Version generation error handling (60%) — generate-version.mjs lacks error handling for readFileSync failures.
  6. Timing side-channels (60%) — durationMs in probe responses could help fingerprint infrastructure (low risk for local tool).

Consolidated Findings

Multiple reviewers independently flagged the same issues, confirming high confidence:

  • Database registration inconsistency (Architecture, Consistency, TypeScript)
  • URL construction fragility (Architecture, Performance)
  • Unbounded response accumulation (Performance, Security)
  • handleConfigureAgent complexity (Complexity reviewer — 220 lines, CC ~15)
  • TLS bypass suggestion (Security with OWASP reference)

Next Steps

  1. Fix CRITICAL issue: database registration throw in catch block
  2. Address HIGH issues in order (7 items)
  3. Consider MEDIUM issues (7 items)
  4. Optional: review lower-confidence suggestions

Reviewed by 9 independent agents (Security, Architecture, Performance, Complexity, Consistency, Regression, Testing, TypeScript, Dependencies)
Claude Code — feat/api-translation-proxy

Dean Sharon and others added 6 commits April 27, 2026 11:36
…ation

- Replace `throw error` on the non-ABI database init failure path with
  `return err(...)`, matching every other error path in bootstrap() and
  honouring the Result<Container> contract.
- Add DECISION comment on the registerValue block explaining why eager
  instantiation is used: ABI mismatch detection must surface at startup,
  not on first database access deep inside request handling.
- Add inline comment on the proxy logger.info call noting that
  proxyConfig.targetApiKey is intentionally omitted to prevent future
  accidental secret exposure.

Co-Authored-By: Claude <noreply@anthropic.com>
… URL

Add MAX_BODY_BYTES size cap to the non-streaming success path and
handleJsonFallback — matching the existing cap on the error path and
the inbound request limit. Without this cap, a malformed or adversarial
backend response could exhaust heap memory.

Pre-compute the chat completions URL once in the constructor instead of
rebuilding it on every request, eliminating redundant string operations
and using the URL API's correct edge-case handling.

Co-Authored-By: Claude <noreply@anthropic.com>
- inline CheckPayload type annotation to match removal of SetPayload
  (both are now inline object types — symmetric typing)
- switch agents.ts probeUrl to static import, consistent with mcp-adapter
- add DESIGN comments explaining intentional check vs set probe asymmetry:
  check includes full connectivity diagnostics for user inspection;
  set only warns on non-ok probe since the save already succeeded

Co-Authored-By: Claude <noreply@anthropic.com>
- anthropic-codec: replace silent default fallback in serializeContentBlock
  with explicit cases for image, document, json, and tool_result (all have no
  Anthropic wire equivalent) plus a never exhaustiveness guard so the compiler
  catches any future CanonicalContent variants

- openai-codec: change ActiveToolCall.argumentsAccumulator from string (O(n²)
  concatenation) to string[] accumulated via push and joined once at close time,
  fixing the perf issue for large tool call argument streams

Co-Authored-By: Claude <noreply@anthropic.com>
Security:
- Replace NODE_TLS_REJECT_UNAUTHORIZED=0 suggestion with NODE_EXTRA_CA_CERTS
  to avoid disabling all certificate verification globally
- Restrict probeUrl to http:/https: schemes only; file:// and ftp:// now
  return err() immediately preventing silent misbehaviour

TypeScript:
- Narrow rawError with instanceof Error before casting to ErrnoException,
  eliminating the unsafe blind cast at the error handler boundary

Architecture:
- Add deepProbeWarning?: string to UrlProbeResult; populated when the deep
  probe (GET /models) fails with a network error after HEAD succeeds, so
  callers can surface the previously-silent failure

Complexity:
- Extract TLS_ERROR_CODES named Set constant with helper isTlsError(); removes
  the 7-operand TLS check from messageForError
- Combine messageForStatus + severityForStatus into single statusResult()
  returning { message, severity }; eliminates duplicated status code dispatch

Tests:
- Add tests for file:// and ftp:// scheme rejection
- Add test verifying deepProbeWarning is set on deep probe network failure

Co-Authored-By: Claude <noreply@anthropic.com>
@dean0x dean0x merged commit 91d5cef into main Apr 27, 2026
2 checks passed
@dean0x dean0x deleted the feat/api-translation-proxy branch April 27, 2026 10:06
@dean0x dean0x mentioned this pull request May 8, 2026
4 tasks
dean0x added a commit that referenced this pull request May 8, 2026
## Summary
Release v1.5.0 — Cross-Platform Agents & Interactive Orchestration

- API translation proxy — Anthropic Messages API to OpenAI Chat
Completions (#152)
- Dashboard layout overhaul — 3-tile responsive, full-width entity
browser (#153)
- Ollama runtime + translate→proxy rename (#157)
- Skills/docs alignment (#158)
- Interactive orchestrator mode --interactive/-i (#159)

8 files updated (version bump, release notes, changelog, features,
roadmap, CLAUDE.md)
2 new migrations (v24, v25); 3 new MCP tools (PipelineStatus,
ListPipelines, CancelPipeline)

### Snyk (best-effort)
2 medium findings — both pre-existing:
- HTTP instead of HTTPS in translation-proxy.ts (by design: localhost
proxy)
- Path traversal in orchestrate.ts (pre-existing code path, only imports
changed)

## Test plan
- [x] typecheck + lint + build pass
- [x] All 14 grouped test suites pass (0 failures)
- [x] Release notes index matches files on disk (patch releases excluded
by convention)
- [ ] CI passes

Co-authored-by: Dean Sharon <deanshrn@gmain.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant