diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index e52ff1bd920..05e9310f074 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -35,6 +35,23 @@ jobs: - name: Lint run: pnpm exec oxlint --format github + # Why: project-owned type declarations must live in .ts so tsc + # actually checks them. TypeScript's skipLibCheck: true (inherited + # from @electron-toolkit/tsconfig) silently widens unresolved names + # in .d.ts to `any`, which is how #1186 shipped a broken IPC signature + # past typecheck. See docs/preload-typecheck-hole.md. + - name: Guard against project-owned .d.ts in preload/shared + run: | + matches=$(find src/preload src/shared -name '*.d.ts' 2>/dev/null || true) + if [ -n "$matches" ]; then + echo "::error::Project-owned .d.ts files are not allowed under src/preload or src/shared." + echo "Move type declarations into a .ts file so skipLibCheck does not hide errors." + echo "See docs/preload-typecheck-hole.md." + echo "Found:" + echo "$matches" + exit 1 + fi + - name: Typecheck run: pnpm typecheck diff --git a/AGENTS.md b/AGENTS.md index f69c354dbe5..44863f5494c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,4 +24,8 @@ Orca targets macOS, Linux, and Windows. Keep all platform-dependent behavior beh ## GitHub CLI Usage -Be mindful of the user's `gh` CLI API rate limit — batch requests where possible and avoid unnecessary calls. All code, commands, and scripts must be compatible with macOS, Linux, and Windows. \ No newline at end of file +Be mindful of the user's `gh` CLI API rate limit — batch requests where possible and avoid unnecessary calls. All code, commands, and scripts must be compatible with macOS, Linux, and Windows. + +## Type Declarations: Prefer `.ts` Over `.d.ts` + +Project-owned type declarations belong in `.ts` files. `.d.ts` is reserved for ambient shims (e.g., `env.d.ts`, `vite/client.d.ts`). TypeScript's `skipLibCheck: true` setting applies globally, including to our own `.d.ts` files, which means any unresolved type reference in a `.d.ts` silently becomes `any` at its call sites. Write your types in `.ts` files so the compiler actually checks them. CI enforces this for `src/preload/` and `src/shared/` — see `docs/preload-typecheck-hole.md`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e50afd39e97..6b24738879d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,6 +44,12 @@ Add high-quality tests for behavior changes and bug fixes. Prefer tests that wou If your change affects UI or interaction behavior, verify it on the platforms it could impact. +## Type Declarations: Prefer `.ts` Over `.d.ts` + +Project-owned type declarations belong in `.ts` files. `.d.ts` is reserved for ambient shims (e.g., `env.d.ts`, `vite/client.d.ts`). TypeScript's `skipLibCheck: true` setting applies globally, including to our own `.d.ts` files, which means any unresolved type reference in a `.d.ts` silently becomes `any` at its call sites. Write your types in `.ts` files so the compiler actually checks them. + +CI enforces this for `src/preload/` and `src/shared/` — see `docs/preload-typecheck-hole.md`. + ## Pull Requests Each pull request should: diff --git a/config/tsconfig.tc.web.json b/config/tsconfig.tc.web.json index cd57b935728..eca58242c7b 100644 --- a/config/tsconfig.tc.web.json +++ b/config/tsconfig.tc.web.json @@ -4,7 +4,7 @@ "../src/renderer/src/env.d.ts", "../src/renderer/src/**/*", "../src/renderer/src/**/*.tsx", - "../src/preload/*.d.ts", + "../src/preload/api-types.ts", "../src/shared/**/*", "../src/main/ipc/worktree-logic.ts", "../src/main/wsl.ts" diff --git a/config/tsconfig.web.json b/config/tsconfig.web.json index dace5cb3fc2..844bf185d3c 100644 --- a/config/tsconfig.web.json +++ b/config/tsconfig.web.json @@ -4,7 +4,7 @@ "../src/renderer/src/env.d.ts", "../src/renderer/src/**/*", "../src/renderer/src/**/*.tsx", - "../src/preload/*.d.ts", + "../src/preload/api-types.ts", "../src/shared/**/*" ], "compilerOptions": { diff --git a/docs/preload-typecheck-hole.md b/docs/preload-typecheck-hole.md new file mode 100644 index 00000000000..dad51a267a5 --- /dev/null +++ b/docs/preload-typecheck-hole.md @@ -0,0 +1,101 @@ +# Preload typecheck hole: why project-owned types live in `.ts` + +## The rule + +Project-owned type declarations under `src/preload/` and `src/shared/` +**must live in `.ts` files, not `.d.ts`**. The CI step +"Guard against project-owned .d.ts in preload/shared" in +`.github/workflows/pr.yml` enforces this. + +## Why + +Orca inherits `skipLibCheck: true` from `@electron-toolkit/tsconfig`. +That setting is the ecosystem default — it exists so a broken `.d.ts` +in some random `node_modules` package can't block your build. TypeScript +has no way to scope it to `node_modules`, so **`skipLibCheck` +applies to our own `.d.ts` files too**. + +In a project-owned `.d.ts`, any type reference that fails to resolve +silently becomes `any` at its call sites instead of erroring. Downstream +assignments against that `any` are also silently accepted. The error +never surfaces during `pnpm typecheck`. + +For example, with `skipLibCheck: true`: + +```ts +// in src/preload/index.d.ts — Worktree is never imported +type WorktreesApi = { + list: () => Promise // silently becomes Promise +} +``` + +and at the call site: + +```ts +// in any renderer file +window.api.worktrees.list().then((arr) => { + setWorktreeName(arr) // setWorktreeName expects string; accepted anyway because arr is any[] +}) +``` + +No compile error. Crashes at runtime. + +The standard TS convention that sidesteps this: put project-owned types in +`.ts` (which are always checked), reserve `.d.ts` for ambient shims +(`env.d.ts`, `vite/client.d.ts`, etc.). The CI guard encodes that +convention mechanically. + +## Incident that forced the fix + +PR #1186 changed the `repos:getBaseRefDefault` IPC return shape from +`Promise` to `Promise` (an envelope +object). Two of three renderer callers were updated; the third +(`StartFromField.tsx`) wasn't. That caller passed the envelope object +into a `setState` setter, which rendered as JSX and threw +React error #31 (`Objects are not valid as a React child`). + +**The call site should have been a compile error.** It wasn't, because +`src/preload/index.d.ts` (now deleted) was a 246-line project-owned +`.d.ts` that referenced ~20 type names it never imported (`Worktree`, +`PRInfo`, `GlobalSettings`, `BaseRefDefaultResult`, and more). Under +`skipLibCheck`, each unresolved name became `any`, which widened the +`.then((ref) => …)` callback parameter to `any` at the consuming call +site. `setDefaultBaseRef(ref: any)` compiled cleanly. + +The crash is fixed by #1189. The typecheck hole is fixed by this PR +(#1197), which collapses the two preload type files (`index.d.ts` + +`api-types.d.ts`) into a single type-checked `api-types.ts`. Full design +discussion, alternatives considered, and rollout notes live in PR #1197. + +## Non-obvious subtleties worth remembering + +- **It's not the hand-authored types that failed — it was the missing + imports.** The types in the old `index.d.ts` were individually fine; + the file only went wrong because names like `Worktree` and `PRInfo` + weren't imported and `skipLibCheck` swallowed the error. A future + contributor copy-pasting types out of a `.d.ts` into `.ts` may be + surprised by a wall of "Cannot find name 'X'" errors — that's the + flag catching its target, not a real regression. +- **`.d.ts` is still legitimate for ambient shims.** `env.d.ts`, + `mermaid.d.ts`, `hosted-git-info.d.ts` all live *outside* the CI + guard's scan roots (`src/preload/` and `src/shared/`) and stay as + `.d.ts`. If a future file under those roots genuinely needs to be + `.d.ts` (e.g., an ambient module shim for a third-party package that + can't live in `.ts`), add it to an allowlist in `pr.yml` at that + time — don't relax the guard wholesale. +- **Intersection types on `window.api` are what actually widened the + `.then` callback to `any`.** The old layout used + `type Api = PreloadApi & { repos: ReposApi, worktrees: WorktreesApi, … }`. + TypeScript's intersection-of-function-types resolution widens callback + parameters to `any` when one side of the intersection has unresolved + names, *even though static-inspection views + (`ReturnType`) still report the correct type*. So + `ReturnType` printed + `Promise` during debugging while the live + `.then((ref) => …)` callback treated `ref` as `any`. Don't re-introduce + intersection typing on the preload surface for any reason. +- **Don't try to fix this by flipping `skipLibCheck: false` globally.** + It would force every transitive `@types/*` package to type-check + cleanly, which is why the ecosystem-wide default is `true`. The + structural fix (project-owned types in `.ts`) removes our last + reason to care about the flag for our code. diff --git a/src/preload/api-types.d.ts b/src/preload/api-types.ts similarity index 94% rename from src/preload/api-types.d.ts rename to src/preload/api-types.ts index 60b5e0c7992..50e96dba3b7 100644 --- a/src/preload/api-types.d.ts +++ b/src/preload/api-types.ts @@ -2,7 +2,6 @@ import type { BaseRefDefaultResult, BrowserCookieImportResult, - BrowserCookieImportSummary, BrowserLoadError, BrowserSessionProfile, BrowserSessionProfileScope, @@ -51,7 +50,7 @@ import type { WorktreeMeta, WorktreeSetupLaunch, WorkspaceSessionState -} from '../../shared/types' +} from '../shared/types' import type { BrowserSetGrabModeArgs, BrowserSetGrabModeResult, @@ -62,7 +61,7 @@ import type { BrowserCaptureSelectionScreenshotResult, BrowserExtractHoverArgs, BrowserExtractHoverResult -} from '../../shared/browser-grab-types' +} from '../shared/browser-grab-types' import type { BrowserContextMenuDismissedEvent, BrowserContextMenuRequestedEvent, @@ -71,11 +70,13 @@ import type { BrowserDownloadRequestedEvent, BrowserPermissionDeniedEvent, BrowserPopupEvent -} from '../../shared/browser-guest-events' -import type { CliInstallStatus } from '../../shared/cli-install-types' -import type { E2EConfig } from '../../shared/e2e-config' -import type { AgentHookInstallStatus } from '../../shared/agent-hook-types' -import type { RuntimeStatus, RuntimeSyncWindowGraph } from '../../shared/runtime-types' +} from '../shared/browser-guest-events' +import type { ElectronAPI } from '@electron-toolkit/preload' +import type { CliInstallStatus } from '../shared/cli-install-types' +import type { E2EConfig } from '../shared/e2e-config' +import type { AgentHookInstallStatus } from '../shared/agent-hook-types' +import type { AgentStatusState } from '../shared/agent-status-types' +import type { RuntimeStatus, RuntimeSyncWindowGraph } from '../shared/runtime-types' import type { ClaudeUsageBreakdownKind, ClaudeUsageBreakdownRow, @@ -85,14 +86,14 @@ import type { ClaudeUsageScope, ClaudeUsageSessionRow, ClaudeUsageSummary -} from '../../shared/claude-usage-types' -import type { RateLimitState } from '../../shared/rate-limit-types' +} from '../shared/claude-usage-types' +import type { RateLimitState } from '../shared/rate-limit-types' import type { SshConnectionState, SshTarget, PortForwardEntry, DetectedPort -} from '../../shared/ssh-types' +} from '../shared/ssh-types' import type { CodexUsageBreakdownKind, CodexUsageBreakdownRow, @@ -102,7 +103,7 @@ import type { CodexUsageScope, CodexUsageSessionRow, CodexUsageSummary -} from '../../shared/codex-usage-types' +} from '../shared/codex-usage-types' export type BrowserApi = { registerGuest: (args: { @@ -352,6 +353,10 @@ export type PreloadApi = { connectionId?: string | null worktreeId?: string sessionId?: string + // Why: lets a single tab open in a different shell than the user's default. + // Preserved from the deleted index.d.ts PtyApi duplicate during the + // single-source-of-truth collapse (see docs/preload-typecheck-hole.md §1). + shellOverride?: string }) => Promise<{ id: string snapshot?: string @@ -524,6 +529,7 @@ export type PreloadApi = { claudeStatus: () => Promise codexStatus: () => Promise geminiStatus: () => Promise + cursorStatus: () => Promise } preflight: PreflightApi notifications: { @@ -860,4 +866,32 @@ export type PreloadApi = { onCredentialResolved: (callback: (data: { requestId: string }) => void) => () => void submitCredential: (args: { requestId: string; value: string | null }) => Promise } + wsl: { + isAvailable: () => Promise + } + agentStatus: { + /** Listen for agent status updates forwarded from native hook receivers. */ + onSet: ( + callback: (data: { + paneKey: string + tabId?: string + worktreeId?: string + state: AgentStatusState + prompt?: string + agentType?: string + toolName?: string + toolInput?: string + lastAssistantMessage?: string + interrupted?: boolean + }) => void + ) => () => void + } +} + +declare global { + // oxlint-disable-next-line typescript-eslint/consistent-type-definitions -- declaration merging requires interface + interface Window { + electron: ElectronAPI + api: PreloadApi + } } diff --git a/src/preload/index.d.ts b/src/preload/index.d.ts deleted file mode 100644 index 0b23b83ea3b..00000000000 --- a/src/preload/index.d.ts +++ /dev/null @@ -1,246 +0,0 @@ -import type { ElectronAPI } from '@electron-toolkit/preload' -import type { - BaseRefDefaultResult, - CreateWorktreeResult, - GhosttyImportPreview, - GitHubPRFile, - GitHubPRFileContents, - GitHubWorkItem, - GitHubWorkItemDetails, - GitHubViewer, - CreateWorktreeArgs -} from '../../shared/types' -import type { SshTarget, SshConnectionState } from '../../shared/ssh-types' -import type { AgentStatusState } from '../../shared/agent-status-types' -import type { PreloadApi } from './api-types' - -type ReposApi = { - list: () => Promise - add: (args: { - path: string - kind?: 'git' | 'folder' - }) => Promise<{ repo: Repo } | { error: string }> - addRemote: (args: { - connectionId: string - remotePath: string - displayName?: string - kind?: 'git' | 'folder' - }) => Promise<{ repo: Repo } | { error: string }> - remove: (args: { repoId: string }) => Promise - update: (args: { - repoId: string - updates: Partial< - Pick - > - }) => Promise - pickFolder: () => Promise - pickDirectory: () => Promise - clone: (args: { url: string; destination: string }) => Promise - cloneAbort: () => Promise - onCloneProgress: (callback: (data: { phase: string; percent: number }) => void) => () => void - getGitUsername: (args: { repoId: string }) => Promise - getBaseRefDefault: (args: { repoId: string }) => Promise - searchBaseRefs: (args: { repoId: string; query: string; limit?: number }) => Promise - onChanged: (callback: () => void) => () => void -} - -type WorktreesApi = { - list: (args: { repoId: string }) => Promise - listAll: () => Promise - create: (args: CreateWorktreeArgs) => Promise - remove: (args: { worktreeId: string; force?: boolean }) => Promise - updateMeta: (args: { worktreeId: string; updates: Partial }) => Promise - persistSortOrder: (args: { orderedIds: string[] }) => Promise - onChanged: (callback: (data: { repoId: string }) => void) => () => void -} - -type WslApi = { - isAvailable: () => Promise -} - -type PtyApi = { - spawn: (opts: { - cols: number - rows: number - cwd?: string - env?: Record - command?: string - connectionId?: string | null - worktreeId?: string - sessionId?: string - shellOverride?: string - }) => Promise<{ - id: string - snapshot?: string - snapshotCols?: number - snapshotRows?: number - isReattach?: boolean - isAlternateScreen?: boolean - replay?: string - sessionExpired?: boolean - coldRestore?: { scrollback: string; cwd: string } - }> - write: (id: string, data: string) => void - resize: (id: string, cols: number, rows: number) => void - signal: (id: string, signal: string) => void - kill: (id: string) => Promise - ackColdRestore: (id: string) => void - hasChildProcesses: (id: string) => Promise - getForegroundProcess: (id: string) => Promise - listSessions: () => Promise<{ id: string; cwd: string; title: string }[]> - onData: (callback: (data: { id: string; data: string }) => void) => () => void - onExit: (callback: (data: { id: string; code: number }) => void) => () => void -} - -type GhApi = { - viewer: () => Promise - repoSlug: (args: { repoPath: string }) => Promise<{ owner: string; repo: string } | null> - prForBranch: (args: { repoPath: string; branch: string }) => Promise - issue: (args: { repoPath: string; number: number }) => Promise - // Why: main-process mappers don't know the Orca Repo.id, so IPC returns - // items without `repoId`. The renderer stamps repoId based on the requesting - // repo before exposing items to UI code. - workItem: (args: { - repoPath: string - number: number - }) => Promise | null> - workItemDetails: (args: { - repoPath: string - number: number - }) => Promise - prFileContents: (args: { - repoPath: string - prNumber: number - path: string - oldPath?: string - status: GitHubPRFile['status'] - headSha: string - baseSha: string - }) => Promise - listIssues: (args: { repoPath: string; limit?: number }) => Promise - listWorkItems: (args: { - repoPath: string - limit?: number - query?: string - }) => Promise[]> - prChecks: (args: { - repoPath: string - prNumber: number - headSha?: string - noCache?: boolean - }) => Promise - prComments: (args: { - repoPath: string - prNumber: number - noCache?: boolean - }) => Promise - resolveReviewThread: (args: { - repoPath: string - threadId: string - resolve: boolean - }) => Promise - updatePRTitle: (args: { repoPath: string; prNumber: number; title: string }) => Promise - mergePR: (args: { - repoPath: string - prNumber: number - method?: 'merge' | 'squash' | 'rebase' - }) => Promise<{ ok: true } | { ok: false; error: string }> - checkOrcaStarred: () => Promise - starOrca: () => Promise -} - -type SettingsApi = { - get: () => Promise - set: (args: Partial) => Promise - listFonts: () => Promise - previewGhosttyImport: () => Promise -} - -type CliApi = { - getInstallStatus: () => Promise - install: () => Promise - remove: () => Promise -} - -type NotificationsApi = { - dispatch: (args: NotificationDispatchRequest) => Promise - openSystemSettings: () => Promise -} - -type ShellApi = { - openPath: (path: string) => Promise - openUrl: (url: string) => Promise - openFilePath: (path: string) => Promise - openFileUri: (uri: string) => Promise - pathExists: (path: string) => Promise - pickAttachment: () => Promise - pickImage: () => Promise - pickDirectory: (args: { defaultPath?: string }) => Promise - copyFile: (args: { srcPath: string; destPath: string }) => Promise -} - -type SshApi = { - listTargets: () => Promise - addTarget: (args: { target: Omit }) => Promise - updateTarget: (args: { - id: string - updates: Partial> - }) => Promise - removeTarget: (args: { id: string }) => Promise - importConfig: () => Promise - connect: (args: { targetId: string }) => Promise - disconnect: (args: { targetId: string }) => Promise - getState: (args: { targetId: string }) => Promise - testConnection: (args: { targetId: string }) => Promise<{ success: boolean; error?: string }> - onStateChanged: ( - callback: (data: { targetId: string; state: SshConnectionState }) => void - ) => () => void - browseDir: (args: { targetId: string; dirPath: string }) => Promise<{ - entries: { name: string; isDirectory: boolean }[] - resolvedPath: string - }> -} - -type AgentStatusApi = { - /** Listen for agent status updates forwarded from native hook receivers. */ - onSet: ( - callback: (data: { - paneKey: string - tabId?: string - worktreeId?: string - state: AgentStatusState - prompt?: string - agentType?: string - toolName?: string - toolInput?: string - lastAssistantMessage?: string - interrupted?: boolean - }) => void - ) => () => void -} - -// Why: Only locally-defined *Api types are listed here. Keys like preflight, -// hooks, cache, session, updater, fs, git, ui, and runtime are inherited via -// the PreloadApi intersection (see ./api-types), so re-declaring them would -// reference undefined type names and risk drifting from the canonical surface. -type Api = PreloadApi & { - repos: ReposApi - worktrees: WorktreesApi - pty: PtyApi - ssh: SshApi - gh: GhApi - settings: SettingsApi - cli: CliApi - notifications: NotificationsApi - shell: ShellApi - agentStatus: AgentStatusApi - wsl: WslApi -} - -declare global { - // oxlint-disable-next-line typescript-eslint/consistent-type-definitions -- declaration merging requires interface - interface Window { - electron: ElectronAPI - api: Api - } -}