Skip to content

refactor(adapters-cursor): isolate desktop executor type contract (split S04 L0/5) - #3557

Closed
lidge-jun wants to merge 2 commits into
devfrom
codex/split-cursor-desktop-executor-contract
Closed

refactor(adapters-cursor): isolate desktop executor type contract (split S04 L0/5)#3557
lidge-jun wants to merge 2 commits into
devfrom
codex/split-cursor-desktop-executor-contract

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • Pure move: DesktopExecutorConfig (15 lines, src/adapters/cursor/native-exec-desktop.ts:23–37) moves verbatim to a dependency-free src/adapters/cursor/desktop-executor-contract.ts; native-exec-desktop.ts keeps the historical export via export type { DesktopExecutorConfig } from "./desktop-executor-contract" plus a local import type; src/types/provider.ts:701 retargets its inline import() type to the contract.
  • Why: breaks the type-only edge src/types/provider.ts → native-exec-desktop.ts → native-exec-tools.ts → tool-definitions.ts → src/types.ts so the upcoming cursor adapter split layers cannot join that cycle. Zero runtime change (every added/changed binding is erased).
  • Plan and evidence: devlog/_plan/260905_now_split_train/105_cursor_desktop_executor_contract.md; parent decisions in 003_parent_decisions.md (TYPE-CYCLE-01, PURE-MOVE-SIZE-01).

Stack (merge bottom-up; S04 adapters-cursor):

# PR Branch Base Review focus
5 TBD codex/split-adapters-cursor-protobuf-events codex/split-adapters-cursor-tool-definitions protobuf-events
4 TBD codex/split-adapters-cursor-request-builder codex/split-adapters-cursor-images request-builder
3 TBD codex/split-adapters-cursor-images codex/split-cursor-desktop-executor-contract images
2 TBD codex/split-adapters-cursor-catalog codex/split-cursor-desktop-executor-contract catalog
1 TBD codex/split-adapters-cursor-tool-definitions codex/split-cursor-desktop-executor-contract tool-definitions
0 this PR codex/split-cursor-desktop-executor-contract ← you are here dev desktop executor type contract

Base: dev. Review this PR's diff only (3 files, +18/−16; non-move diff 4 lines). Move-aware view: git diff --color-moved=dimmed-zebra dev...HEAD.

Verification

  • bun run typecheck → exit 0
  • bun test tests/providers/cursor/cursor-desktop-exec.test.ts → 14 pass / 0 fail
  • bun run privacy:scan → passed
  • wc -l: contract 15, native-exec-desktop 194 (was 207), provider.ts 723 (unchanged count)
  • Export parity (origin/dev vs HEAD): {DesktopExecutorConfig, desktopDepsFromConfig} both sides; independent read-only review (gpt-6-astra) confirmed byte-identical move and zero new runtime imports.
  • Full suite on the remote CI host (lidge) at this exact SHA: recorded in the devlog doc once it completes.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed (devlog unit records the layer; no user-facing behavior change).
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults (type-only move; no runtime path touched).

Summary by CodeRabbit

  • Refactor
    • Clarified the configuration contract for optional external desktop executors.
    • Configuration can specify commands for computer-use and screen recording, along with an optional working directory, environment variables, and timeout.
    • The configuration contract is now available through a dedicated public interface for consistent use across supported integrations.
    • Existing desktop executor behavior remains unchanged.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 5, 2026 00:13
@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_regression_test — Behavior changed under src/ or gui/src/ without a test change. Add focused coverage or obtain test-exception-approved.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed.

Hygiene

Deterministic PR hygiene checks passed.

@github-actions
github-actions Bot marked this pull request as draft September 5, 2026 00:14
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 1d49714b-8d57-40f7-9b9d-045484bf8bc7

📥 Commits

Reviewing files that changed from the base of the PR and between e950b27 and 97df515.

📒 Files selected for processing (1)
  • tests/providers/cursor/cursor-desktop-exec.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The PR moves DesktopExecutorConfig into a dedicated contract module. native-exec-desktop.ts re-exports the type, and OcxProviderConfig.desktopExecutor references the new module. Tests verify type compatibility and import boundaries.

Changes

Desktop executor contract

Layer / File(s) Summary
Contract extraction and provider wiring
src/adapters/cursor/desktop-executor-contract.ts, src/adapters/cursor/native-exec-desktop.ts, src/types/provider.ts, tests/providers/cursor/cursor-desktop-exec.test.ts
Lines 1–15 define the exported DesktopExecutorConfig interface. Lines 20–24 import and re-export the type from native-exec-desktop.ts. Line 701 updates OcxProviderConfig.desktopExecutor to reference the dedicated contract module. The tests verify compatible exports, a dependency-free contract, and the provider import path.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 97df5

This change relocates the desktop executor configuration type without changing runtime execution behavior. Compatibility and import-boundary coverage are present, so no merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the primary change: isolating the Cursor adapter's desktop executor type contract. It is specific, concise, and related to the full changeset.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/split-cursor-desktop-executor-contract

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 64 / 80

이 PR은 Cursor 어댑터 분할 열차(S04)의 맨 아래 칸(L0/5)입니다. 지금 dev HEAD(4dde2db97, package 2.43.0)에서 src/types/provider.tsdesktopExecutor 필드가 import("../adapters/cursor/native-exec-desktop").DesktopExecutorConfig를 가리킵니다. 그 파일은 런타임으로 native-exec-tools.ts를 끌어오고, 그 타입/상수는 다시 tool-definitions.ts를 통해 src/types.ts 배럴을 읽습니다. 배럴은 ./types/provider를 다시내보냅니다. 결과적으로 타입만으로도 provider → native-exec-desktop → native-exec-tools → tool-definitions → types → provider 순환이 생깁니다. 위층(catalog / images / request-builder / tool-definitions / protobuf-events)을 잘라 내려면 이 고리를 먼저 끊어야 합니다. 이 PR은 런타임 동작을 바꾸지 않습니다. DesktopExecutorConfig 인터페이스(약 15줄)만 의존성 없는 src/adapters/cursor/desktop-executor-contract.ts로 그대로 옮기고, native-exec-desktop.ts는 예전 경로로 export type { DesktopExecutorConfig } 재수출을 유지하며, provider.ts의 inline import()만 새 계약 파일로 돌립니다. desktopDepsFromConfig와 외부 프로세스 실행 로직은 그대로 native-exec-desktop.ts에 남습니다. 기존 테스트 tests/providers/cursor/cursor-desktop-exec.test.ts가 이미 그 런타임 경로를 덮고 있어, 타입만 옮긴 diff에 새 테스트가 없는 것은 정상입니다. 다만 봇 위생 검사(missing_regression_test)가 이 PR을 draft로 묶었고 intake: hygiene-blocked 라벨이 붙어 있습니다. 순수 타입 이동이라 동작 회귀 여지가 거의 없으므로, test-exception-approved로 예외를 열거나 위생 규칙을 타입-only move에 맞게 좁히는 쪽이 맞습니다. PR 본문이 가리키는 계획 문서(devlog/_plan/260905_now_split_train/105_cursor_desktop_executor_contract.md, 003_parent_decisions.md)는 이 브랜치에 없습니다. 근거 문서는 후속 PR에 넣거나 이 브랜치에 추가하는 편이 좋습니다. 지금 dev 방향(사용량 가격 표준화·Windows 테스트 스택·키 페일오버 등)과 충돌하지 않고, 타입/설정 분할 캠페인의 다음 층으로 자연스럽게 이어집니다.

라인 desktop-executor-contract.ts:6–15 - DesktopExecutorConfig를 의존성 없는 계약 파일로 옮긴 순수 이동입니다. 필드·JSDoc이 기존과 같고 런타임 import가 없어 순환을 끊는 목적에 맞습니다.
라인 native-exec-desktop.ts:20–21 - 로컬 import type와 역사적 export type { DesktopExecutorConfig } 재수출을 함께 둡니다. 기존 import 경로를 깨지 않으면서 계약 파일로 책임을 옮긴 형태가 맞습니다.
라인 provider.ts:701 - inline import() 대상만 desktop-executor-contract로 바꿉니다. OcxProviderConfig.desktopExecutor의 의미는 그대로이고, 타입 그래프만 가벼워집니다.
경로 tests 부재 / hygiene draft - src/ 변경인데 테스트 diff가 없어 missing_regression_test로 draft가 됐습니다. 타입 erase 전용 이동이라 새 테스트 가치는 거의 없습니다. 이미 cursor-desktop-exec.test.ts가 런타임을 덮습니다.
경로 계획 문서 - 본문이 인용한 105_cursor_desktop_executor_contract.md / 003_parent_decisions.md가 브랜치에 없습니다. 분할 열차 근거를 남기려면 문서를 추가하거나 본문 링크를 고치세요.
경로 상위 스택(L1–L5) - catalog/images/request-builder/tool-definitions/protobuf-events PR은 아직 없습니다. L0만 먼저 dev에 넣어도 안전합니다. 상위 층은 이 계약 위를 베이스로 열면 됩니다.

메인테이너의 판단이 필요한 지점

  • 순수 타입 이동에 test-exception-approved를 붙여 위생 draft를 풀지, 아니면 위생 규칙을 타입-only/ re-export 이동에서 제외할지
  • 계획 문서를 이 PR에 포함할지, 다음 층 PR에 몰아 넣을지
  • L0만 지금 머지하고 L1–L5를 이어서 열지, 아니면 상위 층 PR이 뜬 뒤 한 묶음으로 리뷰할지

너의 추천
test-exception-approved(또는 동등한 예외)로 위생을 풀고 ready로 올린 뒤, typecheck와 cursor-desktop-exec.test.ts가 초록이면 L0로 바로 머지하세요. 런타임 동작이 없어서 상위 층이 열릴 때까지 기다릴 필요는 없습니다. 머지 후 이 브랜치를 catalog/images/tool-definitions 등 L1+ 베이스로 쓰세요. 계획 문서는 후속 PR에 넣어도 되지만, 본문 링크는 실제 경로와 맞춰 주세요.

이 댓글은 grok-bot이 작성했습니다

@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 5, 2026
@github-actions
github-actions Bot marked this pull request as ready for review September 5, 2026 00:17

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on exact head 97df51515c22ccd610665989aa940f15bc3bca24.

This is a clean type-graph boundary change: DesktopExecutorConfig moved unchanged into a dependency-free contract leaf, the historical type export from native-exec-desktop.ts remains compatible, and OcxProviderConfig now references only the erased contract path. The added regression verifies both import surfaces remain structurally identical and prevents src/types/provider.ts from regaining the implementation edge that closes the cycle.

No runtime execution path or configuration semantics changed. Diff hygiene, the focused Cursor executor coverage in the exact-head suite, typecheck, privacy gates, React Doctor, and full cross-platform CI are green. No unresolved current-head review thread remains.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Landed via #3668 at 6585e6a

@lidge-jun lidge-jun added the landed-via-maintainer Original PR closed after landing via a maintainer merge train label Sep 5, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

Superseded by aggregate PR #3668, admin-merged into dev as 6585e6a after exact-head CI and tested-tree verification. This original PR was not individually merged; its rebased content and historical records were consolidated in #3668. The original branch is preserved. Further unimplemented debt layers remain deferred.

@lidge-jun lidge-jun closed this Sep 5, 2026
@lidge-jun
lidge-jun deleted the codex/split-cursor-desktop-executor-contract branch September 6, 2026 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). landed-via-maintainer Original PR closed after landing via a maintainer merge train

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants