Skip to content

Conversation

@kaitranntt
Copy link
Owner

Summary

Files Modified (6) / Added (2)

  • Modified: src/ccs.ts, src/commands/help-command.ts, src/config/reserved-names.ts, src/config/unified-config-loader.ts, src/config/unified-config-types.ts, src/web-server/routes/index.ts
  • Added: src/web-server/routes/cursor-routes.ts, src/web-server/routes/cursor-settings-routes.ts
  • Stub: src/commands/cursor-command.ts (16 lines, replaced by feat: Cursor IDE — daemon lifecycle, models catalog, and CLI commands #520's full impl during merge)

Edge Cases Fixed (code review)

  • MEDIUM: Input validation on PUT /api/cursor/settings (port=number, auto_start/ghost_mode=boolean)

Merge Note

src/commands/cursor-command.ts exists as a 16-line stub here. During merge, keep the 239-line implementation from #520.

Test plan

  • Config loads cursor section correctly
  • Dashboard routes respond with cursor status
  • Settings PUT validates input types
  • Reserved name prevents cursor as profile name

@ccs-reviewer
Copy link

ccs-reviewer bot commented Feb 11, 2026

Code Review: PR #528 — feat(cursor): integrate cursor provider into config, dashboard, and reserved names


📋 Summary

This PR adds Cursor IDE integration to CCS: a CursorConfig type with defaults, dashboard routes for status/settings/auth, CLI command routing for ccs cursor, reserved name registration, and a full protobuf-based executor/translator layer for the Cursor API. The config and dashboard pieces are well-structured and follow existing patterns, but several files in src/cursor/ appear to have been imported from an external source with incompatible formatting, style conventions, and ESLint-violating patterns that will fail bun run validate.


✅ Strengths

  1. Proper use of getCcsDir() — All credential/config path access uses the test-isolation-safe getCcsDir() from config-manager.ts, never os.homedir() directly. (src/cursor/cursor-auth.ts:167, src/web-server/routes/cursor-settings-routes.ts:48)
  2. Consistent config patternCursorConfig, DEFAULT_CURSOR_CONFIG, getCursorConfig(), and mergeWithDefaults follow the exact same pattern as CopilotConfig. (src/config/unified-config-types.ts:244-260, src/config/unified-config-loader.ts:283-287)
  3. Input validation on settings routes — PUT /api/cursor/settings validates port is a number, auto_start/ghost_mode are booleans before persisting. (src/web-server/routes/cursor-settings-routes.ts:40-53)
  4. Atomic file writes — Raw settings save uses write-to-temp + rename pattern to prevent corruption. (src/web-server/routes/cursor-settings-routes.ts:107-108)
  5. Optimistic concurrency controlexpectedMtime check on PUT /raw prevents silent overwrites from concurrent edits. (src/web-server/routes/cursor-settings-routes.ts:97-103)
  6. Clean command routing — The CURSOR_SUBCOMMANDS guard in src/ccs.ts:556-562 ensures bare ccs cursor still works as a profile name, only routing known subcommands to the handler.

🔍 Observations & Suggestions

🔴 P0 — Blocking Issues

1. Formatting: Tabs instead of spaces in all src/cursor/*.ts files (except cursor-auth.ts and types.ts)

The project enforces 2-space indentation via .prettierrc (tabWidth: 2). Six files use tabs throughout:

  • cursor-executor.ts (786 lines)
  • cursor-protobuf-decoder.ts (301 lines)
  • cursor-protobuf-encoder.ts (262 lines)
  • cursor-protobuf-schema.ts (205 lines)
  • cursor-protobuf.ts (212 lines)
  • cursor-translator.ts (145 lines)

These will fail bun run format:check and therefore bun run validate. Run bun run format to fix.

2. Double quotes instead of single quotes in the same files

The project uses singleQuote: true in Prettier. All six tab-indented files use double quotes for strings. Same fix — bun run format.

3. Non-null assertion operator (!) — violates @typescript-eslint/no-non-null-assertion

Multiple occurrences across the cursor files:

File Example Location Count
cursor-protobuf-decoder.ts Lines 139, 140, 153, etc. ~15
cursor-executor.ts Lines 95, 97 (via toolCallsMap.get(id)!) ~8
cursor-protobuf-encoder.ts ~2

These are all ESLint errors. Replace with proper null checks or use Map.get() with fallback patterns:

// Before (violates rule)
const existing = toolCallsMap.get(tc.id)!;

// After
const existing = toolCallsMap.get(tc.id);
if (!existing) continue;

4. .js extensions in TypeScript imports

The six cursor files use .js extensions in import paths ("./cursor-protobuf-schema.js"), while the rest of the codebase uses extensionless imports. This may work with certain bundler configs but is inconsistent with project conventions and could break the build depending on tsconfig module resolution settings.

🔴 P1 — Security

5. Command injection risk in queryStateDb

src/cursor/cursor-auth.ts:57-63:

const result = execSync(
  `sqlite3 "${dbPath}" "SELECT value FROM itemTable WHERE key='${key}'" 2>/dev/null`,
  { encoding: 'utf8', timeout: 5000 }
).trim();

Both dbPath and key are interpolated into a shell command string. While key is currently hardcoded at call sites, dbPath is derived from os.homedir() + platform paths and could contain shell metacharacters (e.g., usernames with special characters).

Recommended fix: Use execFileSync with argument array to avoid shell interpretation:

const result = execFileSync('sqlite3', [
  dbPath,
  `SELECT value FROM itemTable WHERE key='${key}'`
], { encoding: 'utf8', timeout: 5000 }).trim();

6. Plaintext credential storage without restrictive permissions

src/cursor/cursor-auth.ts:183 writes credentials.json (containing the raw Cursor access token) via fs.writeFileSync without setting file permissions to owner-only:

// Current
fs.writeFileSync(credPath, JSON.stringify(credentials, null, 2), 'utf8');

// Recommended: restrict to owner read/write only
fs.writeFileSync(credPath, JSON.stringify(credentials, null, 2), { encoding: 'utf8', mode: 0o600 });

🟡 P2 — Correctness / Design

7. Duplicate CursorCredentials interface

CursorCredentials is defined in both:

  • src/cursor/types.ts:15-28 — has machineId, email, authMethod, importedAt
  • src/cursor/cursor-executor.ts:16-22 — has accessToken, providerSpecificData.machineId

These are structurally different and will cause confusion. The executor should import from types.ts or define a separate ExecutorCredentials type.

8. Duplicate concatArrays utility

Defined identically in both:

  • src/cursor/cursor-protobuf-encoder.ts:96-105
  • src/cursor/cursor-protobuf.ts:186-194

Should be defined once and imported.

9. Port range validation missing

src/web-server/routes/cursor-settings-routes.ts:42 validates port is a number but doesn't check the valid range:

if ('port' in updates && (typeof updates.port !== 'number' || updates.port < 1 || updates.port > 65535)) {
  res.status(400).json({ error: 'port must be a number between 1 and 65535' });
  return;
}

10. model field mentioned in PR summary but missing from CursorConfig

The PR description says "Adds cursor section to unified config schema with port, model, auto_start, ghost_mode" but CursorConfig only has port, auto_start, ghost_mode. If model is intentionally deferred, the PR description should be corrected.

11. Varint decoder precision for large numbers

src/cursor/cursor-protobuf-decoder.ts:41: result |= (b & 0x7f) << shift uses JavaScript's 32-bit bitwise shift. Varints larger than 2^32 (which protobuf supports for uint64) will silently produce incorrect values. This is acceptable if only small field numbers/lengths are expected, but worth a comment.

🟢 P3 — Minor / Style

12. Hardcoded Cursor client versionsrc/cursor/cursor-executor.ts:247: "x-cursor-client-version": "2.3.41" will go stale. Consider making this configurable or adding a comment about update cadence.

13. Unused importsrandomUUID is imported in cursor-protobuf-encoder.ts:3 but never used in that file (it's used in cursor-protobuf.ts). Will likely fail noUnusedLocals.


🔒 Security Considerations

Check Status Notes
No secrets in code Tokens read from SQLite at runtime
Input validation on API routes Type checks on PUT settings
SQL injection prevention queryStateDb uses string interpolation in shell command
Command injection prevention execSync with template literal, not execFileSync
File permission on credentials Written with default permissions (world-readable)
Auth on dashboard routes Routes mounted under existing auth middleware
Path traversal Paths constructed from getCcsDir(), not user input

📊 Code Quality Checklist

Check Status Notes
Follows project formatting (Prettier) 6 files use tabs + double quotes
Passes ESLint rules ! assertions, unused import
TypeScript strict mode compliant ⚠️ Multiple as casts; acceptable for protobuf work
Tests added 0 tests for 2,721 new lines
No duplicate code concatArrays, CursorCredentials duplicated
Consistent import style .js extensions in cursor files only
getCcsDir() used (not os.homedir()) Correct everywhere
ASCII-only CLI output [!] prefix used in stub
Help command updated Major section 5 added
Reserved name registered 'cursor' added

💡 Recommendations

Priority Item Effort
🔴 High Run bun run format to fix tabs/quotes in 6 cursor files Low
🔴 High Fix ! non-null assertions to pass ESLint Medium
🔴 High Replace execSync with execFileSync in queryStateDb Low
🔴 High Remove unused randomUUID import from cursor-protobuf-encoder.ts Low
🔴 High Remove .js extensions from imports or confirm build config supports them Low
🟡 Medium Set file permissions 0o600 on credentials.json Low
🟡 Medium Deduplicate CursorCredentials and concatArrays Low
🟡 Medium Add port range validation (1-65535) Low
🟡 Medium Add unit tests for cursor-auth.ts, cursor-settings-routes.ts Medium
🟢 Low Fix PR description (model field discrepancy) Low
🟢 Low Add comment about varint 32-bit limitation Low

🎯 Overall Assessment

❌ CHANGES REQUESTED

The config integration, dashboard routes, and CLI routing are well-designed, follow existing patterns, and are ready to merge.

However, the src/cursor/ protobuf/executor layer (1,911 lines across 6 files) appears to have been brought in from an external source without reformatting for this project. These files will fail bun run validate due to:

  • Tab indentation (project uses 2 spaces)
  • Double quotes (project uses single quotes)
  • ! non-null assertions (ESLint error)
  • Unused imports (noUnusedLocals)
  • .js import extensions (inconsistent)

The security issues in queryStateDb (command injection via execSync) and plaintext credential storage without restrictive permissions should also be addressed before merge.

Minimum required for approval:

  1. Run bun run format && bun run lint:fix on all new files
  2. Fix remaining ESLint errors that auto-fix can't handle (! assertions)
  3. Switch execSync to execFileSync in queryStateDb
  4. Confirm bun run validate passes

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

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