Skip to content

Commit d897070

Browse files
committed
fix: address CodeRabbit review findings
Pin release workflow actions to commit SHAs, validate the IPC sender and handle window-open rejections in the desktop app, keep the updater state across window recreation and respect the auto-update preference on manual checks, and inline single-use version-check wrappers in the CLI launcher.
1 parent 8ae5114 commit d897070

5 files changed

Lines changed: 61 additions & 27 deletions

File tree

.changeset/sdk-event-union.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pymodel/pythinker-code-sdk": minor
3+
---
4+
5+
Add question, approval, and prompt lifecycle events to the SDK session event types.

.github/workflows/desktop-release.yml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,14 @@ jobs:
1717
runs-on: macos-15
1818
steps:
1919
- name: Checkout
20-
uses: actions/checkout@v4
20+
uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # pinned from v4
2121
with:
2222
fetch-depth: 0
2323
persist-credentials: true
2424

25-
- uses: pnpm/action-setup@v6
25+
- uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # pinned from v6
2626

27-
- uses: actions/setup-node@v6
27+
- uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # pinned from v6
2828
with:
2929
node-version-file: .nvmrc
3030
cache: pnpm
@@ -64,7 +64,7 @@ jobs:
6464

6565
- name: Upload macOS artifacts for manual runs
6666
if: github.event_name == 'workflow_dispatch'
67-
uses: actions/upload-artifact@v7
67+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # pinned from v7
6868
with:
6969
name: desktop-macos
7070
path: |

apps/desktop/src/main.ts

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,13 @@ function hasOrigin(raw: string, expected: string): boolean {
163163
}
164164
}
165165

166+
function assertTrustedSender(event: Electron.IpcMainInvokeEvent): void {
167+
const frame = event.senderFrame
168+
if (hostOrigin === undefined || frame === null || frame !== event.sender.mainFrame || !hasOrigin(frame.url, hostOrigin)) {
169+
throw new Error('desktop update IPC rejected an untrusted sender')
170+
}
171+
}
172+
166173
/** Install navigation and permission policy before the first renderer loads. */
167174
function hardenSession(): void {
168175
const desktopSession = session.defaultSession
@@ -234,24 +241,40 @@ async function createMainWindow(): Promise<BrowserWindow> {
234241
return window
235242
}
236243

237-
ipcMain.handle('pythinker:update:get', () => getUpdateState())
238-
ipcMain.handle('pythinker:update:set-auto', (_event, enabled: unknown) => {
244+
function showWindowSafely(): void {
245+
void lifecycle?.showWindow().catch((error: unknown) => {
246+
console.error('desktop window failed to open:', error)
247+
})
248+
}
249+
250+
ipcMain.handle('pythinker:update:get', (event) => {
251+
assertTrustedSender(event)
252+
return getUpdateState()
253+
})
254+
ipcMain.handle('pythinker:update:set-auto', (event, enabled: unknown) => {
255+
assertTrustedSender(event)
239256
if (typeof enabled !== 'boolean') throw new TypeError('automatic updates must be a boolean')
240257
return setAutoUpdate(enabled)
241258
})
242-
ipcMain.handle('pythinker:update:check', () => checkForUpdatesNow())
243-
ipcMain.handle('pythinker:update:install', () => quitAndInstallNow())
259+
ipcMain.handle('pythinker:update:check', (event) => {
260+
assertTrustedSender(event)
261+
return checkForUpdatesNow()
262+
})
263+
ipcMain.handle('pythinker:update:install', (event) => {
264+
assertTrustedSender(event)
265+
return quitAndInstallNow()
266+
})
244267

245268
function createTray(images: TrayImages): void {
246269
tray = new Tray(images.idle)
247270
tray.setToolTip(APP_NAME)
248271
const template: MenuItemConstructorOptions[] = [
249-
{ label: 'Open Pythinker', click: () => { void lifecycle?.showWindow() } },
272+
{ label: 'Open Pythinker', click: showWindowSafely },
250273
{ type: 'separator' },
251274
{ label: 'Quit', click: () => { void requestAppQuit() } },
252275
]
253276
tray.setContextMenu(Menu.buildFromTemplate(template))
254-
tray.on('click', () => { void lifecycle?.showWindow() })
277+
tray.on('click', showWindowSafely)
255278
}
256279

257280
function releaseAppQuit(): void {
@@ -333,8 +356,8 @@ async function boot(): Promise<void> {
333356
if (!app.requestSingleInstanceLock()) {
334357
app.quit()
335358
} else {
336-
app.on('second-instance', () => { void lifecycle?.showWindow() })
337-
app.on('activate', () => { void lifecycle?.showWindow() })
359+
app.on('second-instance', showWindowSafely)
360+
app.on('activate', showWindowSafely)
338361
app.on('window-all-closed', () => {
339362
// Tray and Host own application lifetime on every platform.
340363
})

apps/desktop/src/updater.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ let getWindow: (() => BrowserWindow | undefined) | undefined
5353
let initialCheckTimer: ReturnType<typeof setTimeout> | undefined
5454
let checkInterval: ReturnType<typeof setInterval> | undefined
5555
let listenersWired = false
56+
let initialized = false
5657
let updateTelemetryTrack: UpdateTelemetryTrack = () => {}
5758

5859
export function trackUpdateTransition(
@@ -162,6 +163,13 @@ export function initUpdater(
162163
windowGetter: () => BrowserWindow | undefined,
163164
track: UpdateTelemetryTrack = () => {},
164165
): void {
166+
if (initialized) {
167+
getWindow = windowGetter
168+
updateTelemetryTrack = track
169+
updateState({})
170+
return
171+
}
172+
initialized = true
165173
getWindow = windowGetter
166174
updateTelemetryTrack = track
167175
settings = readUpdateSettings(app.getPath('userData'))
@@ -176,7 +184,7 @@ export function initUpdater(
176184

177185
try {
178186
autoUpdater.autoDownload = settings.autoUpdate
179-
autoUpdater.autoInstallOnAppQuit = true
187+
autoUpdater.autoInstallOnAppQuit = settings.autoUpdate
180188
} catch (error) {
181189
stateError(error)
182190
return
@@ -205,15 +213,16 @@ export function setAutoUpdate(enabled: boolean): UpdateState {
205213

206214
try {
207215
autoUpdater.autoDownload = enabled
216+
autoUpdater.autoInstallOnAppQuit = enabled
208217
} catch (error) {
209218
stateError(error)
210219
return state
211220
}
212-
if (!enabled) {
213-
clearTimers()
214-
} else {
221+
if (enabled) {
215222
scheduleChecks()
216223
if (!wasEnabled) void checkForUpdatesNow()
224+
} else {
225+
clearTimers()
217226
}
218227
return state
219228
}
@@ -230,7 +239,11 @@ export async function checkForUpdatesNow(): Promise<UpdateState> {
230239
stateError(error)
231240
return state
232241
}
233-
return runCheck()
242+
try {
243+
return await runCheck()
244+
} finally {
245+
autoUpdater.autoDownload = settings.autoUpdate
246+
}
234247
}
235248

236249
export function quitAndInstallNow(): UpdateState {

apps/pythinker-code/src/launcher.ts

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { spawn } from 'node:child_process';
33
const FFI_FLAG = '--experimental-ffi';
44
const FFI_WARNING_FLAG = '--disable-warning=ExperimentalWarning';
55
const FFI_CHILD_ENV = 'PYTHINKER_CODE_FFI_CHILD';
6+
// Local on purpose: the FFI launcher test executes this file standalone, so it must stay import-free beyond node builtins.
67
const REQUIRED_RUNTIME = 'Node.js 20 or newer';
78
const MINIMUM_NODE = [20, 0, 0] as const;
89
const FFI_NODE = [26, 4, 0] as const;
@@ -27,14 +28,6 @@ function isVersionBelow(
2728
return patch < reqPatch;
2829
}
2930

30-
function isRuntimeTooOld(): boolean {
31-
return isVersionBelow(process.versions.node, MINIMUM_NODE);
32-
}
33-
34-
function supportsFfi(): boolean {
35-
return !isVersionBelow(process.versions.node, FFI_NODE);
36-
}
37-
3831
function isFfiProcess(): boolean {
3932
// Only execArgv decides: a stale env marker must never bypass the FFI re-exec.
4033
return process.execArgv.includes(FFI_FLAG);
@@ -96,7 +89,7 @@ function launchWindowsFallback(
9689
}
9790

9891
async function launch(): Promise<void> {
99-
if (isRuntimeTooOld()) {
92+
if (isVersionBelow(process.versions.node, MINIMUM_NODE)) {
10093
process.stderr.write(
10194
`Pythinker Code requires ${REQUIRED_RUNTIME}; you are running Node.js ${process.versions.node}.\n` +
10295
`${NATIVE_INSTALL_HINT}\n`,
@@ -105,7 +98,7 @@ async function launch(): Promise<void> {
10598
return;
10699
}
107100

108-
if (!supportsFfi()) {
101+
if (isVersionBelow(process.versions.node, FFI_NODE)) {
109102
await import(new URL('./main.mjs', import.meta.url).href);
110103
return;
111104
}

0 commit comments

Comments
 (0)