From dc62937fdc221409263cc0e4c329104697425928 Mon Sep 17 00:00:00 2001 From: hugo Date: Fri, 27 Feb 2026 09:53:36 +0100 Subject: [PATCH] fix: third round of workover --- PYTHON_SCRIPTING.md | 7 - src/main/ipc/handlers.ts | 6 + src/renderer/App.tsx | 1 + .../components/GitSidebar/GitSidebar.tsx | 4 + tests/engine/PythonMacroWorkerRuntime.test.ts | 298 +++++++++++++++++- tests/renderer/python/abiV1.test.ts | 133 +++++++- 6 files changed, 435 insertions(+), 14 deletions(-) diff --git a/PYTHON_SCRIPTING.md b/PYTHON_SCRIPTING.md index e783235..40ef8ee 100644 --- a/PYTHON_SCRIPTING.md +++ b/PYTHON_SCRIPTING.md @@ -111,13 +111,6 @@ These are current realities and should be treated as authoritative unless we exp - [x] Add macro execution counters (count, timeout/error counts) for real render path. - [x] `PythonMacroWorkerRuntime` exposes `macroCount`, `errorCount`, `timeoutCount` getters. - [x] `resetCounters()` method for clean state between generation runs. -- [ ] Define regression thresholds based on benchmark trends. - -## Out of Scope Until Core Gaps Close - -- [ ] AI assistant tooling exposure from Python scripts. -- [ ] General Python package/dependency policy expansion. -- [ ] Advanced bridge optimizations (only if metrics prove need). ## Acceptance Gate Before Marking Python Scripting “Complete” diff --git a/src/main/ipc/handlers.ts b/src/main/ipc/handlers.ts index 0ed7c0b..21288cc 100644 --- a/src/main/ipc/handlers.ts +++ b/src/main/ipc/handlers.ts @@ -1503,4 +1503,10 @@ export function registerIpcHandlers(): void { taskManager.on('taskProgress', forwardEvent('task:progress')); taskManager.on('taskCompleted', forwardEvent('task:completed')); taskManager.on('taskFailed', forwardEvent('task:failed')); + + const scriptEngine = getScriptEngine(); + scriptEngine.on('scriptCreated', forwardEvent('script:created')); + scriptEngine.on('scriptUpdated', forwardEvent('script:updated')); + scriptEngine.on('scriptDeleted', forwardEvent('script:deleted')); + scriptEngine.on('scriptsRebuilt', forwardEvent('scripts:rebuilt')); } diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index 6a232f0..7472ecc 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -411,6 +411,7 @@ const App: React.FC = () => { await Promise.all([ window.electronAPI?.posts.rebuildFromFiles(), window.electronAPI?.media.rebuildFromFiles(), + window.electronAPI?.scripts.rebuildFromFiles(), ]); await window.electronAPI?.media.regenerateMissingThumbnails(); } catch (error) { diff --git a/src/renderer/components/GitSidebar/GitSidebar.tsx b/src/renderer/components/GitSidebar/GitSidebar.tsx index 9713b34..670113b 100644 --- a/src/renderer/components/GitSidebar/GitSidebar.tsx +++ b/src/renderer/components/GitSidebar/GitSidebar.tsx @@ -3,6 +3,7 @@ import { useAppStore } from '../../store'; import { openGitDiffCommitTab, openGitDiffFileTab } from '../../navigation/tabPolicy'; import { useI18n } from '../../i18n'; import type { GitInitProgress, GitHistoryEntry, GitRemoteStateDto } from '../../../main/shared/electronApi'; +import { BDS_EVENT_SCRIPTS_CHANGED, dispatchWindowEvent } from '../../utils'; import './GitSidebar.css'; import '../Sidebar/Sidebar.css'; @@ -393,6 +394,9 @@ export const GitSidebar: React.FC = () => { setErrorGuidance('guidance' in result ? result.guidance || [] : []); return; } + if (action === 'pull') { + dispatchWindowEvent(BDS_EVENT_SCRIPTS_CHANGED); + } await loadRepoState(); } catch { setError(tr('gitSidebar.error.actionFailed', { action })); diff --git a/tests/engine/PythonMacroWorkerRuntime.test.ts b/tests/engine/PythonMacroWorkerRuntime.test.ts index 618d824..8e66abf 100644 --- a/tests/engine/PythonMacroWorkerRuntime.test.ts +++ b/tests/engine/PythonMacroWorkerRuntime.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { PythonMacroWorkerRuntime, type WorkerLike, type WorkerFactory } from '../../src/main/engine/PythonMacroWorkerRuntime'; -function createMockWorkerFactory(): { +function createAutoRespondFactory(): { factory: WorkerFactory; postMessages: unknown[]; } { @@ -32,7 +32,7 @@ function createMockWorkerFactory(): { } }, terminate() {}, - removeAllListeners() {}, + removeAllListeners() { messageHandler = null; }, }; setTimeout(() => { @@ -45,17 +45,65 @@ function createMockWorkerFactory(): { return { factory, postMessages }; } +function createManualFactory(): { + factory: WorkerFactory; + postMessages: unknown[]; + triggerReady: () => void; + triggerResult: (requestId: string, html: string, data?: Record, warnings?: string[]) => void; + triggerError: (requestId: string, error: string) => void; + triggerFatalError: (error: string) => void; +} { + const postMessages: unknown[] = []; + let messageHandler: ((msg: unknown) => void) | null = null; + + const triggerReady = () => { + messageHandler?.({ type: 'ready' }); + }; + + const triggerResult = (requestId: string, html: string, data?: Record, warnings?: string[]) => { + messageHandler?.({ type: 'macroResult', requestId, html, data, warnings }); + }; + + const triggerError = (requestId: string, error: string) => { + messageHandler?.({ type: 'macroError', requestId, error }); + }; + + const triggerFatalError = (error: string) => { + messageHandler?.({ type: 'error', error }); + }; + + const factory: WorkerFactory = () => { + const worker: WorkerLike = { + on(event: string, handler: (...args: unknown[]) => void) { + if (event === 'message') { + messageHandler = handler as (msg: unknown) => void; + } + }, + postMessage(message: unknown) { + postMessages.push(message); + }, + terminate() {}, + removeAllListeners() { messageHandler = null; }, + }; + + setTimeout(() => triggerReady(), 5); + return worker; + }; + + return { factory, postMessages, triggerReady, triggerResult, triggerError, triggerFatalError }; +} + describe('PythonMacroWorkerRuntime', () => { let runtime: PythonMacroWorkerRuntime; - let mockFactory: ReturnType; beforeEach(() => { vi.clearAllMocks(); - mockFactory = createMockWorkerFactory(); - runtime = new PythonMacroWorkerRuntime(mockFactory.factory); }); it('should render a Python macro successfully', async () => { + const { factory } = createAutoRespondFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + const result = await runtime.renderMacro({ scriptContent: 'def render(ctx): return {"html": "

output

"}', entrypoint: 'render', @@ -68,6 +116,8 @@ describe('PythonMacroWorkerRuntime', () => { }); it('should track macro execution counter', async () => { + const { factory } = createAutoRespondFactory(); + runtime = new PythonMacroWorkerRuntime(factory); expect(runtime.macroCount).toBe(0); await runtime.renderMacro({ @@ -80,6 +130,9 @@ describe('PythonMacroWorkerRuntime', () => { }); it('should reset counters', async () => { + const { factory } = createAutoRespondFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + await runtime.renderMacro({ scriptContent: 'def render(ctx): return {"html": ""}', entrypoint: 'render', @@ -94,6 +147,9 @@ describe('PythonMacroWorkerRuntime', () => { }); it('should dispose without errors', async () => { + const { factory } = createAutoRespondFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + await runtime.renderMacro({ scriptContent: 'def render(ctx): return {"html": ""}', entrypoint: 'render', @@ -104,6 +160,9 @@ describe('PythonMacroWorkerRuntime', () => { }); it('should pass cacheKey to the worker', async () => { + const { factory, postMessages } = createAutoRespondFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + await runtime.renderMacro({ scriptContent: 'def render(ctx): return {"html": ""}', entrypoint: 'render', @@ -111,8 +170,235 @@ describe('PythonMacroWorkerRuntime', () => { cacheKey: 'script-1:v2', }); - const lastMessage = mockFactory.postMessages[mockFactory.postMessages.length - 1] as Record; + const lastMessage = postMessages[postMessages.length - 1] as Record; expect(lastMessage.type).toBe('renderMacro'); expect(lastMessage.cacheKey).toBe('script-1:v2'); }); + + it('should reject on timeout and increment timeoutCount', async () => { + const { factory } = createManualFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + + const promise = runtime.renderMacro({ + scriptContent: 'import time; time.sleep(999)', + entrypoint: 'slow', + contextJson: '{}', + timeoutMs: 50, + }); + + await expect(promise).rejects.toThrow('Python macro timed out after 50ms'); + expect(runtime.timeoutCount).toBe(1); + expect(runtime.macroCount).toBe(0); + }); + + it('should increment errorCount when worker returns macroError', async () => { + const { factory, triggerError } = createManualFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + + const promise = runtime.renderMacro({ + scriptContent: 'def bad(): raise Exception("fail")', + entrypoint: 'bad', + contextJson: '{}', + timeoutMs: 3000, + }); + + await new Promise((r) => setTimeout(r, 20)); + triggerError('py-macro-1', 'Script execution failed'); + + await expect(promise).rejects.toThrow('Script execution failed'); + expect(runtime.errorCount).toBe(1); + expect(runtime.macroCount).toBe(1); + }); + + it('should reject on fatal worker error', async () => { + const { factory, triggerFatalError } = createManualFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + + const promise = runtime.renderMacro({ + scriptContent: '', + entrypoint: 'x', + contextJson: '{}', + timeoutMs: 3000, + }); + + await new Promise((r) => setTimeout(r, 20)); + triggerFatalError('Pyodide failed to load'); + + await expect(promise).rejects.toThrow('Pyodide failed to load'); + }); + + it('should serialize concurrent requests (queue)', async () => { + const postMessages: unknown[] = []; + let messageHandler: ((msg: unknown) => void) | null = null; + + const factory: WorkerFactory = () => { + const worker: WorkerLike = { + on(event: string, handler: (...args: unknown[]) => void) { + if (event === 'message') { + messageHandler = handler as (msg: unknown) => void; + } + }, + postMessage(message: unknown) { postMessages.push(message); }, + terminate() {}, + removeAllListeners() { messageHandler = null; }, + }; + setTimeout(() => messageHandler?.({ type: 'ready' }), 0); + return worker; + }; + + runtime = new PythonMacroWorkerRuntime(factory); + + // Prime the worker + const p0 = runtime.renderMacro({ + scriptContent: 'init', entrypoint: 'render', contextJson: '{}', timeoutMs: 3000, + }); + await new Promise((r) => setTimeout(r, 10)); + const initReq = postMessages[0] as { requestId: string }; + messageHandler?.({ type: 'macroResult', requestId: initReq.requestId, html: 'ok' }); + await p0; + postMessages.length = 0; + + // Enqueue two concurrent requests — both dispatch to the async path. + // The first will become active; the second sees activeRequest set and queues. + const p1 = runtime.renderMacro({ scriptContent: 'first', entrypoint: 'render', contextJson: '{}', timeoutMs: 3000 }); + // Let p1's dispatchNext settle so activeRequest is set before p2 enqueues + await new Promise((r) => setTimeout(r, 10)); + const p2 = runtime.renderMacro({ scriptContent: 'second', entrypoint: 'render', contextJson: '{}', timeoutMs: 3000 }); + await new Promise((r) => setTimeout(r, 10)); + + // Only first should be dispatched + expect(postMessages.length).toBe(1); + const firstReq = postMessages[0] as { requestId: string; scriptContent: string }; + expect(firstReq.scriptContent).toBe('first'); + + // Complete first → second should dispatch + messageHandler?.({ type: 'macroResult', requestId: firstReq.requestId, html: 'result-1' }); + const r1 = await p1; + expect(r1.html).toBe('result-1'); + + await new Promise((r) => setTimeout(r, 10)); + expect(postMessages.length).toBe(2); + const secondReq = postMessages[1] as { requestId: string; scriptContent: string }; + expect(secondReq.scriptContent).toBe('second'); + + messageHandler?.({ type: 'macroResult', requestId: secondReq.requestId, html: 'result-2' }); + const r2 = await p2; + expect(r2.html).toBe('result-2'); + }); + + it('should reject queued requests on dispose', async () => { + const { factory } = createManualFactory(); + runtime = new PythonMacroWorkerRuntime(factory); + + const p1 = runtime.renderMacro({ + scriptContent: '', + entrypoint: 'x', + contextJson: '{}', + timeoutMs: 5000, + }); + + await new Promise((r) => setTimeout(r, 20)); + + runtime.dispose(); + + await expect(p1).rejects.toThrow('Python macro worker runtime disposed'); + }); + + it('should recover from worker crash and accept new requests', async () => { + let workerCount = 0; + let messageHandler: ((msg: unknown) => void) | null = null; + + const factory: WorkerFactory = () => { + workerCount++; + const currentWorker = workerCount; + + const worker: WorkerLike = { + on(event: string, handler: (...args: unknown[]) => void) { + if (event === 'message') { + messageHandler = handler as (msg: unknown) => void; + } + }, + postMessage(message: unknown) { + const msg = message as { type: string; requestId: string; scriptContent: string }; + if (msg.type === 'renderMacro' && msg.scriptContent !== 'hang') { + setTimeout(() => { + messageHandler?.({ + type: 'macroResult', + requestId: msg.requestId, + html: `result-from-worker-${currentWorker}`, + }); + }, 5); + } + }, + terminate() {}, + removeAllListeners() { messageHandler = null; }, + }; + + setTimeout(() => messageHandler?.({ type: 'ready' }), 0); + return worker; + }; + + runtime = new PythonMacroWorkerRuntime(factory); + + // First request succeeds + const r1 = await runtime.renderMacro({ + scriptContent: 'ok', entrypoint: 'x', contextJson: '{}', timeoutMs: 3000, + }); + expect(r1.html).toBe('result-from-worker-1'); + + // Force crash by timing out (no response for 'hang') + const crashPromise = runtime.renderMacro({ + scriptContent: 'hang', entrypoint: 'x', contextJson: '{}', timeoutMs: 30, + }); + + await expect(crashPromise).rejects.toThrow('timed out'); + + // New request should create a new worker and succeed + const r2 = await runtime.renderMacro({ + scriptContent: 'ok', entrypoint: 'x', contextJson: '{}', timeoutMs: 3000, + }); + expect(r2.html).toBe('result-from-worker-2'); + expect(workerCount).toBe(2); + }); + + it('should forward warnings from worker result', async () => { + let messageHandler: ((msg: unknown) => void) | null = null; + + const factory: WorkerFactory = () => { + const worker: WorkerLike = { + on(event: string, handler: (...args: unknown[]) => void) { + if (event === 'message') { + messageHandler = handler as (msg: unknown) => void; + } + }, + postMessage(message: unknown) { + const msg = message as { type: string; requestId: string }; + if (msg.type === 'renderMacro') { + setTimeout(() => { + messageHandler?.({ + type: 'macroResult', + requestId: msg.requestId, + html: '

ok

', + warnings: ['deprecation notice', 'slow query'], + }); + }, 10); + } + }, + terminate() {}, + removeAllListeners() { messageHandler = null; }, + }; + setTimeout(() => messageHandler?.({ type: 'ready' }), 5); + return worker; + }; + + runtime = new PythonMacroWorkerRuntime(factory); + + const result = await runtime.renderMacro({ + scriptContent: 'def render(ctx): return {"html": "

ok

", "warnings": ["deprecation notice"]}', + entrypoint: 'render', + contextJson: '{}', + }); + + expect(result.warnings).toEqual(['deprecation notice', 'slow query']); + }); }); diff --git a/tests/renderer/python/abiV1.test.ts b/tests/renderer/python/abiV1.test.ts index 3e397fa..32c6820 100644 --- a/tests/renderer/python/abiV1.test.ts +++ b/tests/renderer/python/abiV1.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { parseMacroContextV1 } from '../../../src/renderer/python/abiV1'; +import { parseMacroContextV1, parseMacroResultV1 } from '../../../src/renderer/python/abiV1'; describe('macroContextV1Schema', () => { it('accepts optional env hook and source metadata', () => { @@ -38,4 +38,135 @@ describe('macroContextV1Schema', () => { }) ).toThrow('Invalid macro context'); }); + + it('rejects when env.isPreview is missing', () => { + expect(() => + parseMacroContextV1({ + env: {}, + }) + ).toThrow('Invalid macro context'); + }); + + it('rejects when env.isPreview is not a boolean', () => { + expect(() => + parseMacroContextV1({ + env: { isPreview: 'yes' }, + }) + ).toThrow('Invalid macro context'); + }); + + it('rejects when env is missing entirely', () => { + expect(() => + parseMacroContextV1({}) + ).toThrow('Invalid macro context'); + }); + + it('accepts minimal valid context with only env.isPreview', () => { + const parsed = parseMacroContextV1({ + env: { isPreview: false }, + }); + + expect(parsed.env.isPreview).toBe(false); + expect(parsed.params).toBeUndefined(); + expect(parsed.data).toBeUndefined(); + }); + + it('accepts params with non-string JSON values', () => { + const parsed = parseMacroContextV1({ + env: { isPreview: false }, + params: { + count: 42, + enabled: true, + tags: ['a', 'b'], + nested: { deep: { value: null } }, + }, + }); + + expect(parsed.params?.count).toBe(42); + expect(parsed.params?.enabled).toBe(true); + expect(parsed.params?.tags).toEqual(['a', 'b']); + expect(parsed.params?.nested).toEqual({ deep: { value: null } }); + }); + + it('rejects unknown top-level fields', () => { + expect(() => + parseMacroContextV1({ + env: { isPreview: true }, + extra: 'nope', + }) + ).toThrow('Invalid macro context'); + }); + + it('rejects unknown source fields', () => { + expect(() => + parseMacroContextV1({ + env: { + isPreview: true, + source: { kind: 'macro', id: '1', extra: 'bad' }, + }, + }) + ).toThrow('Invalid macro context'); + }); + + it('accepts source without optional id', () => { + const parsed = parseMacroContextV1({ + env: { + isPreview: false, + source: { kind: 'macro' }, + }, + }); + + expect(parsed.env.source).toEqual({ kind: 'macro' }); + }); +}); + +describe('macroResultV1Schema', () => { + it('accepts minimal result with html only', () => { + const parsed = parseMacroResultV1({ html: '

hello

' }); + + expect(parsed.html).toBe('

hello

'); + expect(parsed.data).toBeUndefined(); + expect(parsed.warnings).toBeUndefined(); + }); + + it('accepts result with all optional fields', () => { + const parsed = parseMacroResultV1({ + html: '
test
', + data: { count: 5, nested: { key: 'val' } }, + warnings: ['slow', 'deprecated'], + }); + + expect(parsed.html).toBe('
test
'); + expect(parsed.data).toEqual({ count: 5, nested: { key: 'val' } }); + expect(parsed.warnings).toEqual(['slow', 'deprecated']); + }); + + it('accepts empty html string', () => { + const parsed = parseMacroResultV1({ html: '' }); + expect(parsed.html).toBe(''); + }); + + it('rejects when html is missing', () => { + expect(() => + parseMacroResultV1({}) + ).toThrow('Invalid macro result'); + }); + + it('rejects when html is not a string', () => { + expect(() => + parseMacroResultV1({ html: 42 }) + ).toThrow('Invalid macro result'); + }); + + it('rejects unknown top-level fields', () => { + expect(() => + parseMacroResultV1({ html: '', extra: true }) + ).toThrow('Invalid macro result'); + }); + + it('rejects non-string warnings', () => { + expect(() => + parseMacroResultV1({ html: '', warnings: [42] }) + ).toThrow('Invalid macro result'); + }); });