feat(cursor): add ConnectRPC protobuf encoder/decoder and HTTP/2 executor

[kaitranntt]

Summary

• Implements ConnectRPC binary protobuf encoding/decoding for Cursor API communication
• HTTP/2 executor with Jyh Cipher checksum header, gzip compression, streaming support
• OpenAI-compatible request/response translation layer
• Part of feat: add Cursor IDE proxy integration with protobuf support #517, Closes feat: Cursor IDE — core protobuf encoder/decoder and executor #518

Files Added (6)

• src/cursor/cursor-protobuf-schema.ts — Protobuf field/wire type definitions
• src/cursor/cursor-protobuf-encoder.ts — Binary protobuf message encoding
• src/cursor/cursor-protobuf-decoder.ts — Protobuf message decoding with field mapping
• src/cursor/cursor-protobuf.ts — Request/response serialization orchestrator
• src/cursor/cursor-translator.ts — OpenAI ↔ Cursor format translation
• src/cursor/cursor-executor.ts — HTTP/2 executor with ConnectRPC protocol

Edge Cases Fixed (code review)

• CRITICAL: CursorCredentials interface unified with types.ts (flat machineId/ghostMode)
• HIGH: 18+ non-null assertions replaced with guard clauses
• HIGH: 3 unused params prefixed with _
• HIGH: HTTP/2 client leak on error — added client.close()
• MEDIUM: Varint decode max 5 bytes, buffer bounds check, empty messages validation
• MEDIUM: DRY — export concatArrays from encoder, remove duplicate

Test plan

• TypeScript compiles clean (tsc --noEmit)
• ESLint passes
• Prettier format check passes
• Integration test with Cursor API endpoint

[ccs-reviewer]

Code Review: PR #525 — feat(cursor): add ConnectRPC protobuf encoder/decoder and HTTP/2 executor

---

Summary

Adds 6 new files (~1,850 LOC) implementing a ConnectRPC protobuf encoding/decoding layer and HTTP/2 executor for communicating with the Cursor API, with an OpenAI-compatible translation layer. The module decomposition is well-structured (schema, encoder, decoder, orchestrator, translator, executor), but the PR ships with zero test coverage for complex binary protocol handling code, and contains several correctness issues in bitwise arithmetic and buffer handling that need attention.

---

Strengths

1. Clean module decomposition — Schema, encoder, decoder, translator, and executor are well-separated with clear single responsibilities. Each file stays under ~225 LOC except the executor (~783 LOC), which bundles multiple transport strategies.
2. Guard clauses over non-null assertions — The fix commit (cc5a903) replaced 18+ ! assertions with proper null checks throughout the decoder and executor. Good defensive coding (cursor-protobuf-decoder.ts:151-154, cursor-executor.ts:468-469).
3. Dual transport support — HTTP/2 with automatic fallback to fetch (cursor-executor.ts:354-358) provides resilience across different environments.
4. Varint bounds enforcement — decodeVarint enforces a 5-byte max to prevent overflow (cursor-protobuf-decoder.ts:17-19).
5. ConnectRPC frame validation — Both parseConnectRPCFrame and the executor's frame parser validate offset + 5 + length before reading (cursor-protobuf-decoder.ts:107-112, cursor-executor.ts:425-430).
6. HTTP/2 client leak fix — client.close() is called in all error/end paths for the HTTP/2 session (cursor-executor.ts:297, 316, 323-324, 332-333).

---

Observations & Suggestions

1. [CORRECTNESS] Bitwise shift overflow in generateChecksum — Priority: HIGH

File: cursor-executor.ts:165-173

JavaScript bitwise operators work on 32-bit signed integers. The shifts >> 40, >> 32 wrap modulo 32, so >> 40 is effectively >> 8 and >> 32 is >> 0. This means the first two bytes don't actually extract upper bits — they silently produce wrong values (which happen to be the same as bytes 3-4 for values fitting in 32 bits).

// Current (misleading — >> 40 and >> 32 don't work as intended in JS)
const byteArray = new Uint8Array([
  (timestamp >> 40) & 0xff,  // Actually (timestamp >> 8) & 0xff
  (timestamp >> 32) & 0xff,  // Actually (timestamp >> 0) & 0xff
  (timestamp >> 24) & 0xff,
  ...
]);

Since Math.floor(Date.now() / 1000000) currently produces values ~1,739 (fits in 11 bits), the upper bytes are 0 regardless, so this "works" today. But it's technically incorrect and will silently produce wrong checksums if the timestamp ever exceeds 32-bit range. If this is an intentional match of Cursor's original implementation, add a comment noting the 32-bit limitation.

2. [CORRECTNESS] Missing bounds check for FIXED64/FIXED32 decoding — Priority: HIGH

File: cursor-protobuf-decoder.ts:58-64

No bounds check before slicing for fixed-width types:

} else if (wireType === WIRE_TYPE.FIXED64) {
  value = buffer.slice(pos, pos + 8);  // No check: pos + 8 <= buffer.length
  pos += 8;
} else if (wireType === WIRE_TYPE.FIXED32) {
  value = buffer.slice(pos, pos + 4);  // No check: pos + 4 <= buffer.length
  pos += 4;
}

If the buffer is truncated, slice will silently return a shorter-than-expected array. Suggest adding bounds checks similar to what's done for WIRE_TYPE.LEN:

} else if (wireType === WIRE_TYPE.FIXED64) {
  if (pos + 8 > buffer.length) return [null, null, null, buffer.length];
  value = buffer.slice(pos, pos + 8);
  pos += 8;
}

3. [DRY] COMPRESS_FLAG defined in two places with divergent values — Priority: MEDIUM

Files: cursor-protobuf-schema.ts:201-204 and cursor-executor.ts:14-19

The schema defines { NONE: 0x00, GZIP: 0x01 } while the executor defines { NONE: 0x00, GZIP: 0x01, GZIP_ALT: 0x02, GZIP_BOTH: 0x03 }. This divergence means the encoder only knows about two flags but the decoder/executor handles four. Either:

• Extend the schema's COMPRESS_FLAG with all four values and import it in the executor, or
• Document why the executor needs a superset (response decompression handling) and name them differently to avoid confusion.

4. [CORRECTNESS] Token split edge case — Priority: MEDIUM

File: cursor-executor.ts:212

const cleanToken = accessToken.includes('::') ? accessToken.split('::')[1] : accessToken;

If accessToken is "prefix::middle::suffix", split('::')[1] returns "middle", discarding "suffix". If the token format allows multiple :: delimiters, this truncates. Consider:

const cleanToken = accessToken.includes('::')
  ? accessToken.slice(accessToken.indexOf('::') + 2)
  : accessToken;

5. [RESOURCE LEAK] AbortSignal listener never removed — Priority: MEDIUM

File: cursor-executor.ts:328-336

The TODO documents this correctly, but the leak is real: each request permanently adds a listener to the AbortSignal. For long-lived AbortController instances reused across requests, listeners accumulate.

// Current
signal.addEventListener('abort', () => { ... });

// Fix: store reference, remove on completion
const onAbort = () => { req.close(); client.close(); reject(new Error('Request aborted')); };
signal.addEventListener('abort', onAbort);
req.on('end', () => { signal.removeEventListener('abort', onAbort); ... });
req.on('error', () => { signal.removeEventListener('abort', onAbort); ... });

6. [TESTING] Zero test coverage for binary protocol code — Priority: HIGH

No test files exist for any of the 6 new modules. Protobuf encoding/decoding is particularly error-prone due to off-by-one errors, varint edge cases, and wire type handling. The project's existing test structure (tests/unit/) has coverage for comparable modules (cliproxy/, glmt/, auth/). At minimum, unit tests should cover:

• Varint encode/decode roundtrip (including edge values: 0, 127, 128, max 32-bit)
• Field encode/decode roundtrip for each wire type
• ConnectRPC frame wrapping/parsing with and without gzip
• extractTextFromResponse with text, tool calls, errors, and thinking payloads
• convertMessages edge cases (system messages, tool results accumulation, empty messages)
• generateChecksum deterministic output for known inputs

7. [ARCHITECTURE] Field number namespace collision risk — Priority: LOW

File: cursor-protobuf-schema.ts:34-140

The flat FIELD object reuses numbers across message types (e.g., REQUEST: 1, MESSAGES: 1, MSG_CONTENT: 1, RESPONSE_TEXT: 1 are all 1). This is correct per protobuf semantics (field numbers are scoped per message), but the flat namespace means a typo like FIELD.MSG_CONTENT where FIELD.MESSAGES was intended compiles and runs without error. Consider either:

• Grouping by message type: FIELD.Request.MESSAGES, FIELD.Message.CONTENT, or
• Adding a comment block documenting which constants belong to which message type (partially done but could be more explicit)

8. [PERFORMANCE] Synchronous gzip in response processing — Priority: LOW

Files: cursor-executor.ts:112, cursor-protobuf-decoder.ts:119

zlib.gunzipSync() blocks the event loop. For the current buffered implementation this is acceptable, but when the TODO to implement true streaming (line 555) is addressed, these should become async zlib.gunzip() calls.

9. [MAINTAINABILITY] SSE chunk building duplication — Priority: LOW

File: cursor-executor.ts:619-728

The transformProtobufToSSE method has ~110 lines of deeply nested JSON.stringify calls for building SSE chunks with repeated structure. A helper like buildSSEChunk(id, model, created, delta, finishReason) would reduce the method to ~40 lines and eliminate the duplication.

---

Security Considerations

Check	Status	Notes
Injection (command/SQL/XSS)	OK	No shell commands or DOM interaction
Auth token handling	OK	Token passed via header, not logged
Secrets in source	OK	No hardcoded secrets
Input validation	WARN	No message content size limits before protobuf encoding
SSRF risk	OK	baseUrl is hardcoded to api2.cursor.sh
Dependency security	OK	Uses only Node.js built-ins (crypto, zlib, http2)
Error message leakage	OK	Errors are wrapped in generic OpenAI-format responses

---

Code Quality Checklist

Check	Status	Notes
TypeScript strict mode	OK	All files compile under strict
No any types	OK	Uses unknown where needed
No non-null assertions (!)	OK	Replaced with guard clauses in fix commit
prefer-const / no-var	OK	Consistent const/let usage
Unused variables prefixed _	OK	_body, _model, _stream, _credentials
Conventional commit format	OK	feat(cursor): and fix(cursor):
No emojis in CLI output	OK	No CLI output in these modules
Cross-platform	OK	Uses process.platform/process.arch for OS detection
Test coverage	FAIL	No tests for any of the 6 new files
DRY compliance	WARN	COMPRESS_FLAG duplicated with different values

---

Recommendations

Priority	Issue	Action
HIGH	No tests	Add unit tests for encoder/decoder roundtrips, frame parsing, message translation, and checksum generation
HIGH	FIXED64/FIXED32 missing bounds check	Add buffer length validation before slicing
HIGH	Bitwise shift overflow in checksum	Add comment documenting 32-bit limitation or use BigInt / Number arithmetic
MEDIUM	COMPRESS_FLAG duplication	Consolidate into schema or differentiate naming
MEDIUM	Token split edge case	Use slice + indexOf instead of split
MEDIUM	AbortSignal listener leak	Store handler reference and remove on completion
LOW	Field number namespace collision risk	Consider grouping or documenting per-message scope more explicitly
LOW	SSE chunk building duplication	Extract helper function for building SSE data lines
LOW	Synchronous gzip	Convert to async when streaming is implemented

---

Overall Assessment

APPROVED WITH NOTES

The architecture is clean and well-decomposed. The fix commit addresses many edge cases raised in prior review. However, the complete absence of tests for binary protocol handling code is a significant risk — protobuf off-by-one errors and varint edge cases are exactly the kind of bugs that only surface in production. The FIXED64/FIXED32 bounds check gap and the misleading bitwise shifts in generateChecksum should be addressed before this code handles real traffic.

I recommend merging to dev for integration testing but strongly suggest adding unit tests as a fast-follow before promoting to main.

Reviewed by gemini-claude-opus-4-6-thinking

[ccs-reviewer]

Code Review: PR #525 — feat(cursor): add ConnectRPC protobuf encoder/decoder and HTTP/2 executor

---

📋 Summary

This PR adds 6 new files (1,861 LOC) implementing a Cursor API integration layer: a custom protobuf encoder/decoder for ConnectRPC wire format, an OpenAI-to-Cursor message translator, and an HTTP/2 executor with checksum header generation and streaming support. The code is well-structured with clean separation of concerns (schema → encoder → decoder → orchestrator → translator → executor), but ships with zero tests for 1,861 lines of new code and has several issues that warrant changes before merge.

---

✅ Strengths

1. ⭐ Clean modular architecture — Logical separation into schema, encoder, decoder, orchestrator, translator, and executor mirrors the ConnectRPC protocol layers (cursor-protobuf-schema.ts, cursor-protobuf-encoder.ts, etc.)
2. Guard clauses over non-null assertions — Per the PR description, 18+ ! assertions were replaced with proper null checks (e.g., cursor-protobuf-decoder.ts:96-98, cursor-executor.ts:481-483)
3. HTTP/2 resource cleanup — client.close() is called on all code paths: success (cursor-executor.ts:325), client error (:306), request error (:333), and abort (:339-340)
4. Varint safety — Decoder limits varint reads to 5 bytes with buffer bounds checking (cursor-protobuf-decoder.ts:22)
5. Abort signal support — Proper AbortSignal handling with listener cleanup in both HTTP/2 and fetch paths (cursor-executor.ts:337-350)
6. ConnectRPC frame parsing — Correct implementation of the 5-byte header (flags + big-endian length) with gzip decompression fallback (cursor-protobuf-encoder.ts:177-195)

---

🔍 Observations & Suggestions

🔴 P0 — No tests

1,861 lines of new code with zero unit tests. This is the most critical gap. The protobuf encoder/decoder and varint codec are excellent candidates for deterministic unit tests.

Files needing tests:

• cursor-protobuf-encoder.ts — encodeVarint, encodeField, wrapConnectRPCFrame round-trip with decoder
• cursor-protobuf-decoder.ts — decodeVarint, decodeField, decodeMessage, extractTextFromResponse
• cursor-translator.ts — buildCursorRequest with system/tool/assistant messages
• cursor-executor.ts — generateChecksum, buildHeaders, transformProtobufToJSON, transformProtobufToSSE

Minimum viable test:

import { describe, it, expect } from 'bun:test';
import { encodeVarint } from '../../../src/cursor/cursor-protobuf-encoder';
import { decodeVarint } from '../../../src/cursor/cursor-protobuf-decoder';

describe('varint round-trip', () => {
  it('encodes and decodes correctly', () => {
    for (const val of [0, 1, 127, 128, 16383, 2097151, 0xFFFFFFFF]) {
      const encoded = encodeVarint(val);
      const [decoded] = decodeVarint(encoded, 0);
      expect(decoded).toBe(val >>> 0);
    }
  });
});

---

🔴 P0 — SSE "streaming" is fully buffered

cursor-executor.ts:569-570 has a TODO acknowledging the streaming response is entirely buffered before being transformed to SSE format. This defeats the purpose of streaming for long responses and will cause significant latency for users.

// TODO: Implement true streaming — currently buffers entire response before transforming.
// This should pipe HTTP/2 data events through a TransformStream for incremental SSE output.

The current implementation buffers the full HTTP/2 response, then parses all protobuf frames, then joins them into a single string and returns it as a Response. This means the user sees no output until the entire response is received from Cursor's API. For an LLM streaming use case, this is functionally broken. If this is intentionally deferred, it should be documented as a known limitation and tracked as a follow-up issue.

---

🟡 P1 — CursorCredentials not unified with shared types

The PR description states: "CRITICAL: CursorCredentials interface unified with types.ts (flat machineId/ghostMode)" — but the interface is defined locally in cursor-executor.ts:16-20 and not exported or shared. If other files need to reference CursorCredentials (e.g., the config layer that supplies credentials), this will lead to duplicate type definitions.

Suggestion: Export CursorCredentials from cursor-protobuf-schema.ts alongside the other shared interfaces.

---

🟡 P1 — Flat FIELD constant has ambiguous overlapping values

cursor-protobuf-schema.ts — The FIELD constant flattens field numbers from different protobuf message types into a single object. Multiple keys map to the same numeric value:

Value	Keys
1	REQUEST, MESSAGES, MSG_CONTENT, TOOL_RESULT_CALL_ID, MODEL_NAME, INSTRUCTION_TEXT, SETTING_PATH, META_PLATFORM, MSGID_ID, MCP_TOOL_NAME, TOOL_CALL, MCP_TOOLS_LIST, MCP_NESTED_NAME, RESPONSE_TEXT, THINKING_TEXT, SETTING6_FIELD_1
2	UNKNOWN_2, MSG_ROLE, TOOL_RESULT_NAME, RESPONSE, MODEL_EMPTY, ...

This works correctly because each field number is used only within its message context, but it makes the code fragile — a developer could easily use FIELD.REQUEST when they meant FIELD.MESSAGES and get the same result by accident. Consider grouping by message type:

export const FIELD = {
  TopLevel: { REQUEST: 1 },
  ChatRequest: { MESSAGES: 1, UNKNOWN_2: 2, INSTRUCTION: 3, ... },
  Message: { CONTENT: 1, ROLE: 2, ID: 13, ... },
  // ...
} as const;

---

🟡 P1 — Hardcoded values that will go stale

Location	Value	Risk
cursor-executor.ts:160	https://api2.cursor.sh	Cannot be overridden for testing or alternative endpoints
cursor-executor.ts:232	2.3.41 (client version)	Will become outdated; Cursor may reject old versions
cursor-executor.ts:228	connect-es/1.6.1 (user-agent)	Same concern

Suggestion: Make these configurable via constructor params or constants that can be updated in one place.

---

🟡 P1 — Silent error swallowing

Multiple catch blocks silently discard errors with only // Continue comments:

• cursor-executor.ts:107-109 — JSON parse failure during decompression
• cursor-executor.ts:456-458 — JSON parse failure during protobuf-to-JSON transform
• cursor-executor.ts:611-613 — JSON parse failure during SSE transform
• cursor-protobuf-decoder.ts:70-72 — Thinking parse error
• cursor-protobuf-decoder.ts:107-109 — MCP parse error
• cursor-executor.ts:75-77 — EdgeRuntime detection

For a protocol decoder, silently dropping data can cause hard-to-diagnose bugs. At minimum, consider logging at debug level (the project has CCS_DEBUG support).

---

🟡 P2 — stream !== false comparison is semantically loose

cursor-executor.ts:393:

const transformedResponse =
  stream !== false
    ? this.transformProtobufToSSE(response.body, model, body)
    : this.transformProtobufToJSON(response.body, model, body);

This means stream: undefined is treated as true (streaming). Given stream is typed as boolean in ExecutorParams, this should be a simple truthiness check (stream ? ... : ...) or strict equality (stream === true).

---

🟢 P3 — Deprecated Buffer.slice

cursor-executor.ts:446,601 and cursor-protobuf-decoder.ts:55,62,67,120 use buffer.slice() which creates a copy. In Node.js 17+, Buffer.slice is deprecated in favor of buffer.subarray() (which shares memory). Since the code already uses Uint8Array.slice in some places and buffer.slice in others, standardize on subarray for consistency and to avoid deprecation warnings.

---

🟢 P3 — Unused exported type FieldNumber

cursor-protobuf-schema.ts:147 exports FieldNumber but it is never imported anywhere in the codebase. With noUnusedLocals: true in tsconfig, this may cause a typecheck error if not consumed (exports are exempt, but it's still dead code).

---

🟢 P3 — Redundant default export

cursor-executor.ts:785 — export default CursorExecutor is redundant since the class is already a named export (export class CursorExecutor). The codebase generally uses named exports (per exploration); only router files and a few legacy modules use default exports.

---

🟢 P3 — isCloudEnv() may be unnecessary

cursor-executor.ts:53-79 — Checks for Cloudflare Workers (caches in globalThis) and EdgeRuntime. This is a Node.js CLI tool — when would it run in a cloud edge environment? If this is defensive code for potential future use, consider removing it per YAGNI.

---

🔒 Security Considerations

Check	Status	Notes
No hardcoded secrets	✅	Tokens passed at runtime via credentials parameter
No command injection	✅	No shell commands executed
Token handling	✅	accessToken is extracted and passed via HTTPS headers only
No eval() or dynamic code	✅	Clean
Input validation	⚠️	machineId is validated as non-empty (cursor-executor.ts:216-218), but accessToken is not validated before use
Buffer parsing bounds checks	✅	Varint decoder, field decoder, and frame parser all check buffer bounds
HTTPS only	✅	baseUrl hardcoded to https://
No sensitive data in logs	✅	No logging of tokens or headers
Compression bomb risk	⚠️	zlib.gunzipSync (cursor-executor.ts:118, cursor-protobuf-decoder.ts:125) has no size limit on decompressed output

---

📊 Code Quality Checklist

Check	Status	Notes
TypeScript strict mode compliance	✅	No any, no ! assertions
ESLint compliance	✅	Unused params prefixed with _
Conventional commit format	✅	feat(cursor): ...
No emojis in CLI output	✅	No CLI output in this PR
ASCII status markers	N/A	No terminal output
Cross-platform compatibility	✅	process.platform used for OS detection
Test coverage	❌	Zero tests for 1,861 LOC
Help text updated	N/A	No new CLI commands
Documentation updated	⚠️	PR description is thorough, no docs/ changes

---

💡 Recommendations

🔴 High Priority

1. Add unit tests — At minimum: varint round-trip, protobuf encode/decode, ConnectRPC frame parsing, message translation, checksum generation, and header building. This is 1,861 LOC of binary protocol handling with no test coverage.
2. Address buffered streaming — Either implement true streaming via TransformStream or document this as a known limitation and create a tracking issue. Users will experience the full response latency before seeing any output.

🟡 Medium Priority

3. Export CursorCredentials from the schema file and import it in the executor to unify the type as claimed in the PR description.
4. Make hardcoded values configurable — baseUrl, clientVersion, and userAgent should be constructor parameters or class constants for easier updates.
5. Add debug-level logging in catch blocks instead of silent swallowing, using the project's existing CCS_DEBUG pattern.

🟢 Low Priority

6. Replace Buffer.slice() with Buffer.subarray() to avoid deprecation warnings.
7. Remove unused FieldNumber export and redundant export default.
8. Consider removing isCloudEnv() per YAGNI unless there's a concrete use case.

---

🎯 Overall Assessment

⚠️ APPROVED WITH NOTES

The implementation is architecturally sound with good separation of concerns and careful buffer handling. The protobuf codec is well-implemented with proper bounds checking. However, shipping 1,861 lines of binary protocol code with zero tests is a significant risk — protobuf encoding bugs are notoriously hard to debug in production. The fake streaming (full buffering before SSE output) is also a functional gap that should be explicitly tracked.

Recommended action: Merge after adding at least encoder/decoder round-trip tests and creating a follow-up issue for true streaming support.

---

🤖 Reviewed by gemini-claude-opus-4-6-thinking

[kaitranntt]

/review

[ccs-reviewer]

Code Review: PR #525 — feat(cursor): add ConnectRPC protobuf encoder/decoder and HTTP/2 executor

---

📋 Summary

This PR introduces a complete Cursor API integration layer with ConnectRPC binary protobuf encoding/decoding, HTTP/2 transport with fallback to fetch, OpenAI-compatible request/response translation, and Jyh Cipher checksum header generation. The module is well-structured across 6 source files with clean separation of concerns, following the class-based pattern established by HeadlessExecutor. The overall quality is solid but there are several correctness and maintainability issues worth addressing.

---

✅ Strengths

1. Clean modular architecture — Schema, encoder, decoder, translator, executor are properly separated with single responsibilities (src/cursor/cursor-protobuf-schema.ts, cursor-protobuf-encoder.ts, etc.)
2. Guard clauses over non-null assertions — The PR replaced 18+ ! assertions with explicit guards like if (!existing) continue; (cursor-executor.ts:478-479, cursor-protobuf-decoder.ts:161-164). This follows the project's ESLint no-non-null-assertion: error rule.
3. HTTP/2 client leak prevention — client.close() is called on all code paths: success (cursor-executor.ts:319), client error (cursor-executor.ts:300), request error (cursor-executor.ts:327), and abort (cursor-executor.ts:334).
4. ⭐ Robust varint implementation — 5-byte max limit prevents malicious/corrupt data from causing infinite loops (cursor-protobuf-decoder.ts:17-25), and >>> 0 ensures unsigned semantics (cursor-protobuf-decoder.ts:27).
5. Consistent debug logging — All catch blocks gate logging behind CCS_DEBUG env var, avoiding noisy output in production.
6. No emojis in CLI output — Compliant with CLAUDE.md constraint; uses [cursor] prefix for debug messages.
7. ⭐ Well-structured test file — Round-trip tests for varint, field encoding, ConnectRPC framing, and message translation cover the core encode/decode contract (tests/unit/cursor/cursor-protobuf.test.ts).
8. Correct bitwise arithmetic for timestamps — Math.trunc division is used for upper bytes exceeding 32-bit range (cursor-executor.ts:169-170), avoiding JavaScript's >> truncation to 32 bits.

---

🔍 Observations & Suggestions

🔴 HIGH — Significant code duplication between JSON and SSE transformers

transformProtobufToJSON (lines 410-562) and transformProtobufToSSE (lines 564-794) share ~60% identical logic: buffer iteration, decompression, error detection, JSON error parsing, tool call accumulation. This violates DRY and means bug fixes must be applied in two places.

File: cursor-executor.ts:410-794

Suggestion: Extract the shared buffer parsing loop into a private method that yields parsed results, then have each transformer format the output:

private *parseProtobufFrames(buffer: Buffer): Generator<{
  text: string | null;
  error: string | null;
  toolCall: ToolCallResult | null;
}> {
  let offset = 0;
  while (offset < buffer.length) {
    if (offset + 5 > buffer.length) break;
    const flags = buffer[offset];
    const length = buffer.readUInt32BE(offset + 1);
    if (offset + 5 + length > buffer.length) break;
    let payload = buffer.slice(offset + 5, offset + 5 + length);
    offset += 5 + length;
    payload = decompressPayload(payload, flags);
    // ... JSON error check, extractTextFromResponse ...
    yield result;
  }
}

🔴 HIGH — parseConnectRPCFrame uses hardcoded compression flags

parseConnectRPCFrame at cursor-protobuf-decoder.ts:124 uses hardcoded values 0x01, 0x02, 0x03 for gzip detection, while the rest of the codebase uses COMPRESS_FLAG.GZIP, COMPRESS_FLAG.GZIP_ALT, COMPRESS_FLAG.GZIP_BOTH. This inconsistency means if flag values change, this function won't be updated.

File: cursor-protobuf-decoder.ts:124

Suggestion:

import { COMPRESS_FLAG } from './cursor-protobuf-schema.js';

// Replace:
if (flags === 0x01 || flags === 0x02 || flags === 0x03) {
// With:
if (flags === COMPRESS_FLAG.GZIP || flags === COMPRESS_FLAG.GZIP_ALT || flags === COMPRESS_FLAG.GZIP_BOTH) {

🟡 MEDIUM — Silent data corruption on gzip decompression failure

decompressPayload at cursor-executor.ts:108-113 catches gzip errors and returns the raw (compressed) payload. This raw data is then passed to extractTextFromResponse which attempts protobuf decoding. Compressed bytes interpreted as protobuf will produce garbage silently.

File: cursor-executor.ts:106-114

Suggestion: Either throw the error (fail fast) or return a sentinel/error value instead of pretending the decompression succeeded:

if (compress flag matches) {
  try {
    return zlib.gunzipSync(payload);
  } catch (err) {
    if (process.env.CCS_DEBUG) {
      console.error('[cursor] gzip decompression failed:', err);
    }
    // Return empty payload rather than corrupted data
    return Buffer.alloc(0);
  }
}

🟡 MEDIUM — convertMessages silently drops unknown roles

Messages with roles other than system, tool, user, assistant are silently ignored in cursor-translator.ts:38-117. If a new OpenAI role is introduced or there's a typo, messages will be silently dropped without any warning.

File: cursor-translator.ts:38-117

Suggestion: Add a debug warning for unknown roles:

if (process.env.CCS_DEBUG) {
  console.error(`[cursor] Unknown message role: ${msg.role}, skipping`);
}

🟡 MEDIUM — Flat FIELD namespace is a footgun

The FIELD object uses the same numeric values across different protobuf message scopes (e.g., REQUEST = 1, MESSAGES = 1, MSG_CONTENT = 1, RESPONSE_TEXT = 1, MCP_TOOLS_LIST = 1 are all 1). While this works because fields are accessed within nested decodeMessage calls, a developer using the wrong constant would get silent incorrect behavior since the numeric values match.

File: cursor-protobuf-schema.ts:34-140

Suggestion: Consider namespacing to make misuse harder:

export const FIELD = {
  TopLevel: { REQUEST: 1 },
  Request: { MESSAGES: 1, INSTRUCTION: 3, MODEL: 5 /* ... */ },
  Message: { CONTENT: 1, ROLE: 2 /* ... */ },
  Response: { TEXT: 1, THINKING: 25 },
  // ...
} as const;

This is a larger refactor so it could be tracked as a follow-up issue.

🟡 MEDIUM — Missing test coverage for critical paths

Component	Tested	Missing
Varint encode/decode	✅	—
Field encode/decode	✅	—
ConnectRPC frame	✅	—
Message translation	✅	Unknown role handling
transformProtobufToJSON	✅ (basic)	Error responses, tool calls
transformProtobufToSSE	❌	Entire function untested
decompressPayload	❌	Gzip path, error fallback
encodeRequest / generateCursorBody	❌	Full request construction
extractToolCall	❌	Tool call decoding
execute	❌	Integration flow
Malformed input	❌	Truncated buffers, corrupt proto

🟢 LOW — Misleading test for checksum time variance

The test at cursor-protobuf.test.ts:300-314 waits 10ms then checks if checksums differ. The checksum timestamp granularity is Math.floor(Date.now() / 1000000) ≈ 16.7 minutes. A 10ms wait will almost never produce a different timestamp. The test passes because it doesn't actually assert inequality, but the name "should generate different checksums over time" and comment are misleading.

File: tests/unit/cursor/cursor-protobuf.test.ts:300-314

Suggestion: Either increase the description accuracy ("should generate valid checksums at different times") or remove the test in favor of a deterministic one that mocks Date.now.

🟢 LOW — extractTextFromResponse swallows all exceptions

At cursor-protobuf-decoder.ts:326-330, all parse errors are caught and return {text: null, error: null, ...}. This makes debugging protobuf decoding issues difficult even in development — a corrupted response is indistinguishable from an empty one.

File: cursor-protobuf-decoder.ts:326-331

🟢 LOW — Debug log in SSE transformer has wrong function name

At cursor-executor.ts:611, the error message says transformProtobufToJSON but the code is in transformProtobufToSSE.

File: cursor-executor.ts:611

// Current (wrong):
console.error('[cursor] transformProtobufToJSON error parsing failed:', err);
// Should be:
console.error('[cursor] transformProtobufToSSE error parsing failed:', err);

🟢 LOW — TOOL_RESULT_RESULT defined but never used

FIELD.TOOL_RESULT_RESULT = 8 is defined in the schema but never referenced in encoder or decoder. If it's needed for future work, a comment explaining its purpose would help.

File: cursor-protobuf-schema.ts:78

---

🔒 Security Considerations

Check	Status	Notes
No credential logging	✅	Tokens never logged, even in debug mode
Token sanitization	✅	:: prefix stripped from access tokens (cursor-executor.ts:214-215)
SHA-256 hashing for client key	✅	Token not sent in plaintext as client key (cursor-executor.ts:224)
HTTPS only	✅	baseUrl is hardcoded to https:// (cursor-executor.ts:152)
No command injection	✅	No shell commands or user input interpolation
Input validation	✅	machineId required check (cursor-executor.ts:210-212), empty messages check (cursor-protobuf.ts:49-51)
Buffer bounds checking	✅	All buffer reads check offset + N > buffer.length before access
Metadata exposure	⚠️	process.cwd(), process.version, process.platform sent to API (cursor-protobuf-encoder.ts:166-170) — standard for this type of API client but worth documenting

---

📊 Code Quality Checklist

Criterion	Status	Notes
TypeScript strict mode	✅	No any, proper type narrowing
No non-null assertions in src	✅	Guard clauses used throughout
ESLint compliance	✅	Based on passing validation
Conventional commit format	✅	feat(cursor): ...
ASCII-only CLI output	✅	[cursor] prefix, no emojis
Cross-platform	✅	Platform detection for OS headers, no OS-specific paths
Error handling	✅	All error paths produce valid JSON responses
HTTP/2 resource cleanup	✅	client.close() on all paths
DRY	⚠️	JSON/SSE transformers share ~60% code
Test coverage	⚠️	Core encode/decode covered, SSE/tool calls/error paths missing

---

💡 Recommendations

Priority	Item	Effort
🔴 High	Extract shared buffer parsing from JSON/SSE transformers	Moderate
🔴 High	Use COMPRESS_FLAG constants in parseConnectRPCFrame	Trivial
🟡 Medium	Fix silent data corruption on gzip failure — return empty buffer	Trivial
🟡 Medium	Fix copy-paste debug log name in SSE transformer (line 611)	Trivial
🟡 Medium	Add tests for transformProtobufToSSE and error response paths	Moderate
🟡 Medium	Add debug warning for unknown message roles in translator	Trivial
🟢 Low	Track FIELD namespace refactoring as follow-up issue	—
🟢 Low	Fix misleading checksum time-variance test	Trivial
🟢 Low	Document or remove unused TOOL_RESULT_RESULT field	Trivial

---

🎯 Overall Assessment

⚠️ APPROVED WITH NOTES

The module is architecturally sound with good separation of concerns, proper security practices, and solid encode/decode test coverage. The critical issues are:

1. The debug log copy-paste error (transformProtobufToJSON in the SSE function) — trivial fix.
2. Hardcoded compression flags in parseConnectRPCFrame — trivial fix.
3. DRY violation in the two transform methods — worth addressing before the module grows further.

None of these are blocking for merge, but the first two should ideally be fixed before merging. The DRY refactoring and additional test coverage can be tracked as follow-up work.

---

🤖 Reviewed by claude-opus-4-6-thinking

[ccs-reviewer]

Code Review: PR #525 — feat(cursor): add ConnectRPC protobuf encoder/decoder and HTTP/2 executor

---

Summary

This PR adds 7 files (2,509 LOC including tests) implementing a Cursor API integration: custom ConnectRPC protobuf codec, OpenAI-to-Cursor message translator, HTTP/2 executor with Jyh Cipher checksum, and 602 lines of unit tests. The architecture is clean with well-separated concerns (schema, encoder, decoder, orchestrator, translator, executor). Previous review rounds addressed many issues (guard clauses over ! assertions, HTTP/2 client leak fixes, COMPRESS_FLAG consolidation, debug logging). This review focuses on remaining issues after the fix commits.

---

Strengths

1. Clean modular architecture — Six source files with single responsibilities mirror the ConnectRPC protocol layers. Each stays under ~225 LOC except the executor (~801 LOC), which is justified by transport + transform logic (src/cursor/cursor-protobuf-schema.ts through cursor-executor.ts)
2. Guard clauses throughout — 18+ non-null assertions replaced with proper if (!x) continue/return guards (cursor-protobuf-decoder.ts:165, cursor-executor.ts:519)
3. HTTP/2 resource cleanup — client.close() on all paths: success (:319), client error (:300), request error (:327), abort (:334). AbortSignal listener properly cleaned up with { once: true } and explicit removeEventListener (:337-343)
4. Robust varint codec — 5-byte max prevents infinite loops on malicious data (cursor-protobuf-decoder.ts:17), >>> 0 ensures unsigned semantics (:27)
5. DRY improvement — parseProtobufFrames generator extracts shared buffer parsing logic between JSON and SSE transformers (cursor-executor.ts:414-486), eliminating the ~60% code duplication flagged in prior reviews
6. Correct 64-bit timestamp arithmetic — Math.trunc(timestamp / 2 ** 40) avoids JavaScript's 32-bit >> limitation (cursor-executor.ts:169-170), with a clear comment explaining why
7. Good test coverage for core codec — Round-trip tests for varint (6 boundary values), field encoding (3 wire types), ConnectRPC framing (compressed, uncompressed, incomplete), message translation (5 scenarios), and executor public API (checksum, headers, URL, JSON/SSE transforms)

---

Observations & Suggestions

1. [BUG] Test calls wrapConnectRPCFrame with wrong argument type — Priority: HIGH

File: tests/unit/cursor/cursor-protobuf.test.ts:497-499, 527-529

Two tests pass { compress: false } (an object) as the second argument to wrapConnectRPCFrame, which expects boolean:

// Current (WRONG — object is truthy, so compress = true)
const buffer = Buffer.from(
  wrapConnectRPCFrame(textPayload, {
    compress: false,
  })
);

// Correct
const buffer = Buffer.from(
  wrapConnectRPCFrame(textPayload, false)
);

Since { compress: false } is truthy, the function actually gzips the payload when the test intends uncompressed. This should also be a TypeScript error (boolean vs object). These tests are testing the wrong behavior.

---

2. [BUG] Test passes extra property stream: true in body type — Priority: MEDIUM

File: tests/unit/cursor/cursor-protobuf.test.ts:503-505, 533-535

const result = executor.transformProtobufToSSE(buffer, 'test-model', {
  messages: [],
  stream: true,  // <-- Not in ExecutorParams['body'] type
});

ExecutorParams['body'] does not have a stream property. While this doesn't cause runtime errors (extra properties are ignored), it indicates the test was written against a different interface.

---

3. [BUG] Duplicate describe('transformProtobufToSSE') blocks — Priority: MEDIUM

File: tests/unit/cursor/cursor-protobuf.test.ts:423, 485

There are two describe('transformProtobufToSSE') blocks inside the CursorExecutor describe. The first (line 423) uses properly constructed protobuf frames via encodeField; the second (line 485) uses manually constructed byte arrays with the wrong wrapConnectRPCFrame call signature. These should be merged, and the duplicates at line 485 use a broken API call.

---

4. [CORRECTNESS] System message content not type-narrowed — Priority: MEDIUM

File: cursor-translator.ts:41-47

if (msg.role === 'system') {
  result.push({
    role: 'user',
    content: `[System Instructions]\n${msg.content}`,  // msg.content can be Array
  });
}

OpenAIMessage.content is typed as string | Array<{ type: string; text?: string }>. If a system message has array content, this produces [System Instructions]\n[object Object],[object Object]. The array-to-string extraction logic used for user/assistant messages (lines 77-85) should also apply here:

if (msg.role === 'system') {
  let content = '';
  if (typeof msg.content === 'string') {
    content = msg.content;
  } else if (Array.isArray(msg.content)) {
    for (const part of msg.content) {
      if (part.type === 'text' && part.text) content += part.text;
    }
  }
  result.push({ role: 'user', content: `[System Instructions]\n${content}` });
}

---

5. [CORRECTNESS] All protobuf-decoded errors hardcoded as 429 rate_limit — Priority: MEDIUM

File: cursor-executor.ts:457-476

When extractTextFromResponse returns an error string, parseProtobufFrames unconditionally emits a 429 rate_limit_error response:

if (result.error) {
  yield {
    type: 'error',
    response: new Response(JSON.stringify({
      error: {
        message: result.error,
        type: 'rate_limit_error',  // Always rate_limit regardless of actual error
        code: 'rate_limited',
      },
    }), { status: 429, ... }),
  };
}

Not all protobuf-decoded errors are rate limits. The error type should be inferred from the error message or a separate field rather than hardcoded.

---

6. [CORRECTNESS] Number(responseHeaders[':status']) can produce NaN — Priority: LOW

File: cursor-executor.ts:321

If the HTTP/2 :status pseudo-header is missing (unusual but possible with malformed responses), Number(undefined) returns NaN, which propagates as the response status.

// Current
status: Number(responseHeaders[':status']),

// Safer
status: Number(responseHeaders[':status']) || 500,

---

7. [CORRECTNESS] Cleaned token not validated as non-empty — Priority: LOW

File: cursor-executor.ts:214-215

const delimIdx = accessToken.indexOf('::');
const cleanToken = delimIdx !== -1 ? accessToken.slice(delimIdx + 2) : accessToken;

If accessToken is "prefix::", cleanToken is "", which would send Authorization: Bearer (empty). An early check would prevent a confusing 401 from the API:

if (!cleanToken) {
  throw new Error('Access token is empty after parsing');
}

---

8. [YAGNI] isCloudEnv() checks for edge runtimes — Priority: LOW

File: cursor-executor.ts:46-62

CCS is a CLI tool. The Cloudflare Workers (caches in globalThis) and EdgeRuntime detection are unreachable code paths. Per CLAUDE.md's YAGNI principle, this should be removed unless there's a concrete plan to run this in an edge environment.

---

9. [TESTING] Missing test coverage for critical paths — Priority: MEDIUM

Component	Status	Notes
Varint encode/decode	Covered	6 boundary values
Field encode/decode	Covered	VARINT, LEN (string + binary)
ConnectRPC framing	Covered	Compressed, uncompressed, incomplete
Message translation	Covered	5 scenarios
Checksum generation	Covered	Format + determinism
Header building	Covered	Required headers, token parsing, ghostMode, missing machineId
transformProtobufToJSON	Covered	Text + error
transformProtobufToSSE	Partially	Has duplicated/broken tests (see items 1-3)
encodeRequest / generateCursorBody	Missing	Full request construction
extractToolCall (decoder)	Missing	Tool call decoding
convertMessages edge cases	Missing	Trailing tool results, empty content
Malformed/truncated input	Missing	Corrupt protobuf, truncated frames
parseProtobufFrames generator	Missing	Multi-frame buffers

---

Security Considerations

Check	Status	Notes
No hardcoded secrets	OK	Tokens passed at runtime
No command injection	OK	No shell commands
Token handling	OK	SHA-256 hashed for x-client-key, not logged
HTTPS only	OK	baseUrl hardcoded to https://
Buffer bounds checking	OK	All reads check offset + N > buffer.length
No eval() / dynamic code	OK	Clean
Compression bomb risk	WARN	zlib.gunzipSync has no decompressed size limit
Metadata exposure	INFO	process.cwd(), process.version sent to API — standard for API clients
Error message leakage	OK	Errors wrapped in generic OpenAI format
Input validation	OK	machineId required, empty messages throw

---

Code Quality Checklist

Criterion	Status	Notes
TypeScript strict mode	OK	No any, proper type narrowing
No non-null assertions	OK	Guard clauses throughout
ESLint compliance	OK	Unused params prefixed _
Conventional commit format	OK	feat(cursor):, fix(cursor):
ASCII-only CLI output	OK	[cursor] prefix for debug, no emojis
Cross-platform	OK	process.platform/process.arch for headers
DRY	OK	parseProtobufFrames generator extracts shared logic
YAGNI	WARN	isCloudEnv() for CLI tool
Test correctness	FAIL	Two tests use wrong wrapConnectRPCFrame call signature
Test coverage	WARN	Core codec covered; SSE, tool calls, edge cases missing

---

Recommendations

Priority	Issue	Action
HIGH	Test bug: wrapConnectRPCFrame called with object instead of boolean	Fix lines 497-499 and 527-529 to pass false instead of { compress: false }
MEDIUM	System message content array not handled	Add type narrowing for array content in convertMessages system branch
MEDIUM	Duplicate/broken SSE test describe blocks	Merge the two transformProtobufToSSE blocks, remove the broken tests
MEDIUM	Hardcoded 429 for all protobuf errors	Infer error type from message content or add an error code field
MEDIUM	Add tests for encodeRequest, extractToolCall, edge cases	Expand test coverage for untested paths
LOW	NaN status from missing HTTP/2 header	Default to 500 if :status is missing
LOW	Empty cleaned token not validated	Add non-empty check after :: stripping
LOW	Remove isCloudEnv() per YAGNI	Dead code for CLI context

---

Overall Assessment

APPROVED WITH NOTES

The implementation is architecturally solid with clean module separation, proper buffer safety, and good security practices. The fix commits have addressed most of the critical issues from prior reviews (DRY extraction via parseProtobufFrames, guard clauses, COMPRESS_FLAG consolidation, abort listener cleanup, debug logging).

Remaining blockers for clean merge:

1. The test file has two calls with wrong argument types to wrapConnectRPCFrame (object instead of boolean) — these tests are testing compressed behavior while claiming to test uncompressed. This is a straightforward fix.
2. The system message content type mismatch could produce garbage output for array-format system messages.

Non-blocking follow-ups:

• Expand test coverage for tool calls, encodeRequest, and malformed input
• Track true streaming implementation (currently buffered SSE, documented via TODO)
• Remove isCloudEnv() per YAGNI

---

Reviewed by claude-opus-4-6-thinking

[ccs-reviewer]

Code Review: PR #525 — feat(cursor): add ConnectRPC protobuf encoder/decoder and HTTP/2 executor

---

📋 Summary

This PR adds a complete ConnectRPC binary protobuf encoding/decoding layer for Cursor API communication, including an HTTP/2 executor with Jyh Cipher checksum, gzip compression, streaming support, and an OpenAI-compatible translation layer. The implementation is well-structured across 6 source files and 1 test file (2,557 additions), with clear separation of concerns between schema, encoder, decoder, orchestrator, translator, and executor. The code demonstrates strong defensive programming with proper buffer bounds checks, guard clauses replacing non-null assertions, and comprehensive error handling throughout.

---

✅ Strengths

1. ⭐ Excellent module architecture — Clean separation into schema/encoder/decoder/translator/executor follows single-responsibility principle and mirrors existing patterns (GLMT, cliproxy). Each file stays under 200 LOC except the executor (786 LOC), which is justified by its orchestration role. (src/cursor/)

2. ⭐ Robust buffer safety — Varint decode enforces 5-byte max (cursor-protobuf-decoder.ts:17-19), FIXED64/FIXED32 bounds checks prevent out-of-range reads (cursor-protobuf-decoder.ts:58-69), and frame parsing validates offset + 5 + length > buffer.length before slicing (cursor-executor.ts:412-417).

3. Correct JS bitwise handling — The generateChecksum method correctly uses Math.trunc division for bytes above 32-bit range where >> would wrap modulo 32 (cursor-executor.ts:144-153). The >>> 0 unsigned right shift is properly used in varint encode/decode.

4. Proper HTTP/2 resource cleanup — client.close() is called on both error and success paths (cursor-executor.ts:282,301,309), and the abort signal listener is cleaned up with stored handler ref + removeEventListener (cursor-executor.ts:314-326).

5. Comprehensive test coverage — 27+ test cases covering varint round-trips, field encoding/decoding, ConnectRPC frame handling, message translation (system/user/assistant/tool roles), checksum format, headers, error paths, and edge cases (malformed frames, truncated payloads, multi-frame buffers). (tests/unit/cursor/cursor-protobuf.test.ts)

6. Full TypeScript strict compliance — No any types, no non-null assertions, unused params properly prefixed with _, consistent const usage. Aligns with tsconfig.json strict mode and ESLint no-explicit-any/no-non-null-assertion error rules.

7. DRY refactoring — parseProtobufFrames generator method (cursor-executor.ts:396-471) extracts shared frame parsing between JSON and SSE transformers, concatArrays is exported from encoder and shared across modules.

8. Token parsing edge case — indexOf('::') + slice instead of split('::')[1] handles tokens containing multiple :: delimiters correctly (cursor-executor.ts:192-193).

---

🔍 Observations & Suggestions

🔴 HIGH — Fragile Rate Limit Detection via String Matching

File: cursor-executor.ts:441-443

const isRateLimit =
  result.error.toLowerCase().includes('rate') ||
  result.error.toLowerCase().includes('limit');

This will false-positive on any error containing "rate" or "limit" (e.g., "character limit exceeded", "generation rate changed", "moderate content"). The JSON error path at line 112 correctly checks for code === 'resource_exhausted', but the protobuf error path uses substring matching.

Suggestion: Match on more specific patterns or use the structured error code when available:

const isRateLimit =
  result.error.toLowerCase().includes('rate limit') ||
  result.error.toLowerCase().includes('resource_exhausted') ||
  result.error.toLowerCase().includes('too many requests');

---

🟡 MEDIUM — Duplicate Decompression Logic

Files: cursor-executor.ts:64-94 (decompressPayload) and cursor-protobuf-decoder.ts:124-137 (inside parseConnectRPCFrame)

Both functions implement the same gzip decompression with COMPRESS_FLAG checking. The executor's version additionally checks for JSON error payloads before decompression, but the core gzip logic is duplicated.

Currently parseProtobufFrames in the executor does its own decompression via decompressPayload and doesn't use parseConnectRPCFrame from the decoder. This means the decoder's decompression code path is only reachable through direct parseConnectRPCFrame calls (e.g., in tests).

Suggestion: Consider having parseProtobufFrames delegate to parseConnectRPCFrame from the decoder, or consolidate the decompression into a shared utility. This would also eliminate the slightly different error handling between the two paths (executor returns Buffer.alloc(0), decoder keeps raw payload).

---

🟡 MEDIUM — Inconsistent Frame Length Parsing

File: cursor-protobuf-decoder.ts:117

const length = (buffer[1] << 24) | (buffer[2] << 16) | (buffer[3] << 8) | buffer[4];

File: cursor-executor.ts:415

const length = buffer.readUInt32BE(offset + 1);

The decoder uses bitwise shifts while the executor uses readUInt32BE. When buffer[1] >= 128, the << 24 operation produces a negative signed 32-bit integer. While payloads > 2GB are practically impossible, this is inconsistent and could produce unexpected behavior with buffer.length < 5 + length checks if length is negative.

Suggestion: Use readUInt32BE consistently, or apply >>> 0 to the bitwise result:

const length = ((buffer[1] << 24) | (buffer[2] << 16) | (buffer[3] << 8) | buffer[4]) >>> 0;

---

🟡 MEDIUM — encodeField Silently Returns Empty Array for FIXED32/FIXED64

File: cursor-protobuf-encoder.ts:60

return new Uint8Array(0);

The encoder handles VARINT and LEN wire types but silently returns an empty array for FIXED32 and FIXED64. These wire types are defined in the schema and handled in the decoder, so if encoding ever needs them, this would silently produce corrupt protobuf. An explicit throw for unsupported wire types would be safer.

throw new Error(`Unsupported wire type for encoding: ${wireType}`);

---

🟡 MEDIUM — FIELD Constant Namespace Collision Risk

File: cursor-protobuf-schema.ts:34-140

The FIELD object uses the same numeric values for fields in different message types (e.g., field 1 is REQUEST, MESSAGES, MSG_CONTENT, TOOL_RESULT_CALL_ID, MODEL_NAME, TOOL_CALL, MCP_TOOLS_LIST, MCP_NESTED_NAME, SETTING_PATH, META_PLATFORM, MSGID_ID, RESPONSE_TEXT, THINKING_TEXT, INSTRUCTION_TEXT, SETTING6_FIELD_1). This is semantically correct for protobuf (field numbers are scoped per message), but the flat constant structure means TypeScript can't prevent using the wrong field in the wrong context.

The JSDoc grouping comments added in review round 2 help, and issue #535 tracks a future refactoring to namespaced constants. This is acknowledged — just noting it's the highest-risk area for future bugs.

---

🟢 LOW — Duplicate Test Cases for Decompression Failure

File: tests/unit/cursor/cursor-protobuf.test.ts:577-595 and 597-624

Two test cases both named "should return empty buffer on decompression failure" test essentially the same code path (invalid gzip data in a frame processed by transformProtobufToJSON). The second test (error handling describe block) is a near-duplicate of the first (decompressPayload error handling).

Suggestion: Remove one of them or differentiate their test scenarios (e.g., test with different invalid gzip patterns, or test the decoder's parseConnectRPCFrame decompression path separately).

---

🟢 LOW — buildCursorRequest Spread Leaks Extra Properties

File: cursor-translator.ts:151-154

return {
  ...body,
  messages,
};

This spreads the entire OpenAIRequestBody into the return value, which means any extra/unexpected properties from the input (e.g., temperature, top_p, max_tokens) will be included in the output. This is likely intentional for tools passthrough, but could leak unexpected properties to downstream consumers.

---

🟢 LOW — Missing Test for Empty Messages Validation

The encodeRequest function (cursor-protobuf.ts:49-51) throws if messages.length === 0, but there's no test for this behavior. Consider adding:

it('should throw on empty messages array', () => {
  expect(() => generateCursorBody([], 'gpt-4', [], null)).toThrow('Messages array must not be empty');
});

---

🔒 Security Considerations

Check	Status	Notes
No hardcoded secrets	✅	Tokens passed via credentials, not stored
Token handling	✅	Bearer token extracted safely; x-client-key is SHA-256 hash, not raw token
Input validation	✅	Buffer bounds checked, varint max bytes enforced, empty messages rejected
No command injection	✅	No shell execution; pure data transformation
No path traversal	✅	process.cwd() used for metadata only, not for file operations
Debug logging safety	✅	Only logs errors, not token values; gated behind CCS_DEBUG
HTTP/2 TLS	✅	Always connects via https:// (cursor-executor.ts:130)
Dependency security	✅	Uses only Node.js built-ins (crypto, zlib, http2) — no external deps
Abort signal cleanup	✅	Prevents listener leaks via stored ref + once: true + removeEventListener

---

📊 Code Quality Checklist

Criterion	Status	Notes
TypeScript strict mode	✅	Full compliance, no any, no ! assertions
ESLint rules	✅	No violations; unused params use _ prefix
No emojis in CLI output	✅	Debug logs use [cursor] prefix only
Error handling	✅	All catch blocks log with CCS_DEBUG, graceful fallbacks
Test coverage	✅	27+ tests covering encoder, decoder, translator, executor, edge cases
DRY	✅	parseProtobufFrames generator, shared concatArrays, COMPRESS_FLAG
KISS	✅	No over-abstraction; functional style with clear data flow
YAGNI	✅	No speculative features; TODO/issues track future work
Cross-platform	✅	OS detection for headers, no platform-specific code
Documentation	✅	JSDoc on all exported functions, grouping comments on FIELD constant
Conventional commits	✅	All 7 commits follow feat(cursor):/fix(cursor): pattern
Module isolation	✅	Fully self-contained, no imports from or exports to other modules

---

💡 Recommendations

Priority	Item	Location
🔴 High	Fix rate limit detection to use more specific string matching	cursor-executor.ts:441-443
🟡 Medium	Consolidate duplicate decompression logic	cursor-executor.ts:64-94 / cursor-protobuf-decoder.ts:124-137
🟡 Medium	Make frame length parsing consistent (readUInt32BE or >>> 0)	cursor-protobuf-decoder.ts:117
🟡 Medium	Throw on unsupported wire types in encodeField instead of silent empty return	cursor-protobuf-encoder.ts:60
🟢 Low	Remove duplicate decompression failure test	cursor-protobuf.test.ts:577-624
🟢 Low	Add test for empty messages validation	cursor-protobuf.ts:49-51

---

🎯 Overall Assessment

⚠️ APPROVED WITH NOTES

This is a high-quality, well-architected implementation of a complex protocol layer. The code demonstrates excellent TypeScript practices, thorough defensive programming, and proper resource management. The module is fully self-contained, follows existing codebase patterns, and has comprehensive test coverage.

The one high-priority item (fragile rate limit detection) should be addressed before merging to prevent false-positive rate limit errors in production. The medium-priority items are code quality improvements that can be addressed in follow-up PRs without blocking this one.

The phased rollout approach (PR #519 auth, #525 protobuf core, #520 daemon/CLI, #521 config/dashboard) is a sound architecture decision that keeps each PR reviewable and testable in isolation.

🤖 Reviewed by claude-opus-4-6-thinking

[ccs-reviewer]

Code Review: PR #525 — feat(cursor): add ConnectRPC protobuf encoder/decoder and HTTP/2 executor

---

📋 Summary

This PR adds 7 files (2,559 LOC) implementing a complete Cursor API integration layer: ConnectRPC binary protobuf codec (schema, encoder, decoder, orchestrator), an OpenAI-to-Cursor message translator, an HTTP/2 executor with Jyh Cipher checksum and gzip support, and 657 lines of unit tests. Five rounds of review feedback have been incorporated, resulting in solid defensive programming throughout. This review evaluates the current state of all files after the fix commits.

---

✅ Strengths

1. ⭐ Excellent modular architecture — Six source files with clear single-responsibility boundaries mirror the ConnectRPC protocol layers (schema → encoder → decoder → orchestrator → translator → executor). This follows the established executor pattern used by HeadlessExecutor, CopilotExecutor, and CLIProxyExecutor (src/cursor/)

2. ⭐ Robust buffer safety throughout — Varint decode enforces 5-byte max to prevent infinite loops on malicious data (cursor-protobuf-decoder.ts:17-19), FIXED64/FIXED32 bounds checks return sentinel values for truncated buffers (cursor-protobuf-decoder.ts:58-69), and frame parsing validates offset + 5 + length > buffer.length before reading (cursor-executor.ts:412-417)

3. ⭐ DRY refactoring via generator — parseProtobufFrames generator (cursor-executor.ts:396-471) extracts the shared buffer parsing loop used by both transformProtobufToJSON and transformProtobufToSSE, eliminating the ~60% code duplication flagged in prior reviews

4. Correct 64-bit timestamp arithmetic — Math.trunc(timestamp / 2 ** 40) properly handles bytes above 32-bit range where JavaScript's >> operator would wrap modulo 32 (cursor-executor.ts:147-153), with a clear inline comment explaining the rationale

5. Full HTTP/2 resource cleanup — client.close() called on all paths: success (:301), client error (:282), request error (:309), and abort (:315-316). AbortSignal listener is properly cleaned up with stored handler ref + removeEventListener on both end and error events (:319-326)

6. Comprehensive guard clauses — 18+ non-null assertions replaced with proper if (!x) continue/return patterns throughout decoder and executor. Compliant with ESLint no-non-null-assertion: error rule

7. Thorough test coverage — 27+ test cases covering varint round-trips (6 boundary values including 0, 127, 128, 0xFFFFFFFF), field encoding/decoding (3 wire types), ConnectRPC framing (compressed/uncompressed/incomplete), message translation (system/user/assistant/tool roles with array content), checksum format validation, header building, JSON/SSE response transforms, and error handling edge cases

8. Token parsing edge case handled — indexOf('::') + slice instead of split('::')[1] correctly handles tokens with multiple :: delimiters (cursor-executor.ts:192-193), plus empty token validation at line 195-197

---

🔍 Observations & Suggestions

1. [DRY] Duplicate decompression logic — Priority: MEDIUM

Files: cursor-executor.ts:64-94 and cursor-protobuf-decoder.ts:124-137

Both decompressPayload (executor) and parseConnectRPCFrame (decoder) implement gzip decompression with COMPRESS_FLAG checking. The executor's version additionally checks for JSON error payloads. Currently parseProtobufFrames uses decompressPayload directly and does not delegate to parseConnectRPCFrame, meaning the decoder's decompression path is only exercised via direct parseConnectRPCFrame calls.

More importantly, the two paths handle failure differently:

• Executor returns Buffer.alloc(0) on gzip failure → graceful empty result
• Decoder keeps the raw (compressed) payload → may be parsed as corrupt protobuf

Suggestion: Either have parseProtobufFrames delegate to parseConnectRPCFrame, or consolidate the decompression into a shared utility with consistent failure behavior. This can be a follow-up PR.

---

2. [CORRECTNESS] Inconsistent frame length parsing — Priority: MEDIUM

File: cursor-protobuf-decoder.ts:117

const length = (buffer[1] << 24) | (buffer[2] << 16) | (buffer[3] << 8) | buffer[4];

File: cursor-executor.ts:415

const length = buffer.readUInt32BE(offset + 1);

The decoder uses bitwise shifts while the executor uses readUInt32BE. When buffer[1] >= 128, << 24 produces a negative signed 32-bit integer, which would make the subsequent buffer.length < 5 + length check behave unexpectedly (though 2GB+ payloads are practically impossible). The executor's readUInt32BE always returns unsigned.

Suggestion: Use readUInt32BE consistently, or apply >>> 0 to the bitwise result:

const length = ((buffer[1] << 24) | (buffer[2] << 16) | (buffer[3] << 8) | buffer[4]) >>> 0;

---

3. [CORRECTNESS] encodeField silently returns empty array for unsupported wire types — Priority: MEDIUM

File: cursor-protobuf-encoder.ts:60

// Falls through for FIXED32, FIXED64, and any other wire type
return new Uint8Array(0);

FIXED32 and FIXED64 are defined in the schema and handled by the decoder. If encoding ever needs to use them (e.g., encoding a response for testing), the current code would silently produce an empty result. Since encoding unsupported wire types is a programming error, an explicit throw is safer:

throw new Error(`Unsupported wire type for encoding: ${wireType}`);

---

4. [TESTING] Duplicate decompression failure test cases — Priority: LOW

File: tests/unit/cursor/cursor-protobuf.test.ts:577-595 and 597-624

Two test cases both verify that invalid gzip data in a frame doesn't crash transformProtobufToJSON. The second test (error handling → should return empty buffer on decompression failure) is a near-duplicate of the first (decompressPayload error handling → should return empty buffer on decompression failure), differing only in the specific byte patterns used.

Suggestion: Remove one or differentiate them (e.g., test the decoder's parseConnectRPCFrame path separately from the executor's decompressPayload path).

---

5. [CORRECTNESS] buildCursorRequest spread leaks extra properties — Priority: LOW

File: cursor-translator.ts:151-154

return {
  ...body,
  messages,
};

This spreads the entire input body, meaning properties like temperature, top_p, max_tokens, reasoning_effort pass through to the output even though Cursor's protobuf format doesn't use them. The downstream generateCursorBody only accesses messages, so the extra properties are harmless, but the spread is broader than necessary.

Suggestion: Return only the fields that downstream consumers need:

return {
  messages,
  tools: body.tools,
};

---

6. [TESTING] Missing test for empty messages validation — Priority: LOW

File: cursor-protobuf.ts:49-51

encodeRequest throws 'Messages array must not be empty' when given an empty array, but there's no test for this behavior:

it('should throw on empty messages array', () => {
  expect(() => generateCursorBody([], 'gpt-4', [], null)).toThrow(
    'Messages array must not be empty'
  );
});

---

7. [MAINTAINABILITY] Unused exports — Priority: LOW

• cursor-protobuf-schema.ts:147 — FieldNumber type is exported but never imported anywhere
• cursor-executor.ts:788 — export default CursorExecutor is redundant since the class already has a named export. The codebase convention favors named exports
• cursor-protobuf-schema.ts:78 — TOOL_RESULT_RESULT = 8 is defined but never referenced in encoder or decoder (has comment "Reserved for future tool result parsing")

---

8. [PERFORMANCE] No decompressed size limit on gunzipSync — Priority: LOW

Files: cursor-executor.ts:85, cursor-protobuf-decoder.ts:130

zlib.gunzipSync has no limit on decompressed output size. A malicious server could send a small gzip payload that decompresses to gigabytes (compression bomb). For a CLI tool connecting to a trusted endpoint (api2.cursor.sh), this risk is minimal, but worth documenting as a known limitation if the executor is ever pointed at untrusted endpoints.

---

🔒 Security Considerations

Check	Status	Notes
No hardcoded secrets	✅	Tokens passed via credentials parameter at runtime
Token handling	✅	SHA-256 hash for x-client-key, Bearer token via HTTPS, not logged
No command injection	✅	No shell commands; pure data transformation
Input validation	✅	Buffer bounds checks, varint max bytes, empty messages rejected, machineId required, empty token check
HTTPS only	✅	baseUrl hardcoded to https://api2.cursor.sh
Debug logging safety	✅	Only logs errors, never token values; gated behind CCS_DEBUG
Dependency security	✅	Node.js built-ins only (crypto, zlib, http2) — no external deps
Abort signal cleanup	✅	Stored handler ref + removeEventListener prevents listener leaks
Error message leakage	✅	Errors wrapped in generic OpenAI-format responses
Compression bomb	⚠️	No size limit on gunzipSync output (low risk for trusted endpoint)

---

📊 Code Quality Checklist

Criterion	Status	Notes
TypeScript strict mode	✅	No any, no ! assertions, proper type narrowing
ESLint compliance	✅	Unused params prefixed _, prefer-const, eqeqeq
No emojis in CLI output	✅	Debug logs use [cursor] prefix, no emojis
Conventional commits	✅	All commits follow feat(cursor):/fix(cursor):
Cross-platform	✅	process.platform/process.arch for OS detection headers
Error handling	✅	All catch blocks log with CCS_DEBUG, graceful fallbacks
DRY	✅	parseProtobufFrames generator, shared concatArrays, unified COMPRESS_FLAG
KISS	✅	No over-abstraction; clear data flow through encode → frame → send → receive → decode → translate
YAGNI	✅	No speculative features; TODOs track future work with issue refs
Test coverage	✅	27+ tests covering core codec, translator, executor, and edge cases
Module isolation	✅	Fully self-contained; no imports from or exports to other CCS modules

---

💡 Recommendations

Priority	Item	Location
🟡 Medium	Consolidate duplicate decompression logic (or align failure behavior)	cursor-executor.ts:64-94 / cursor-protobuf-decoder.ts:124-137
🟡 Medium	Use consistent unsigned frame length parsing	cursor-protobuf-decoder.ts:117
🟡 Medium	Throw on unsupported wire types in encodeField	cursor-protobuf-encoder.ts:60
🟢 Low	Remove duplicate decompression failure test	cursor-protobuf.test.ts:577-624
🟢 Low	Narrow buildCursorRequest spread to needed fields only	cursor-translator.ts:151-154
🟢 Low	Add test for empty messages validation	cursor-protobuf.ts:49-51
🟢 Low	Remove unused exports (FieldNumber, export default, TOOL_RESULT_RESULT)	schema + executor

---

🎯 Overall Assessment

✅ APPROVED

This is a well-architected, thoroughly hardened implementation of a complex binary protocol layer. The code demonstrates strong TypeScript practices (strict mode compliance, no any or ! assertions), proper resource management (HTTP/2 client cleanup, abort signal handling), and comprehensive defensive programming (buffer bounds checking at every level).

Five rounds of review feedback have been systematically addressed: guard clauses replaced all non-null assertions, parseProtobufFrames generator eliminated DRY violations, COMPRESS_FLAG was consolidated, rate limit detection was tightened with specific string matching, system message array content is properly handled, and token parsing edge cases are covered.

The remaining items are all non-blocking improvements (decompression consolidation, consistent frame length parsing, unsupported wire type handling, minor test cleanup). These can be addressed in follow-up work without risk to the current implementation.

The phased rollout approach (PR #519 auth → #525 protobuf core → #520 daemon/CLI → #521 config/dashboard) keeps each PR reviewable and testable in isolation.

---

🤖 Reviewed by claude-opus-4-6-thinking