diff --git a/backend/openmlr/routes/terminal.py b/backend/openmlr/routes/terminal.py index 99b3f2b..b7a0f22 100644 --- a/backend/openmlr/routes/terminal.py +++ b/backend/openmlr/routes/terminal.py @@ -6,7 +6,7 @@ Security: - Minimal environment (no server secrets leaked) - Workspace path validated against WORKSPACES_ROOT -- Shell spawned via subprocess (not os.fork) to avoid async corruption +- Shell spawned via asyncio.create_subprocess_exec (not os.fork) to avoid async corruption - --norc --noprofile to prevent .bashrc injection - Proper zombie process cleanup with SIGKILL escalation """ @@ -19,7 +19,6 @@ import pty import signal import struct -import subprocess import termios from pathlib import Path @@ -185,15 +184,17 @@ async def terminal_websocket( await websocket.accept() - # Spawn PTY using subprocess instead of os.fork() to avoid + # Spawn PTY using asyncio.create_subprocess_exec instead of os.fork() to avoid # corrupting the async event loop and leaking file descriptors. master_fd, slave_fd = pty.openpty() env = _build_safe_env(workspace_path) shell = "/bin/bash" try: - proc = subprocess.Popen( - [shell, "--norc", "--noprofile"], + proc = await asyncio.create_subprocess_exec( + shell, + "--norc", + "--noprofile", stdin=slave_fd, stdout=slave_fd, stderr=slave_fd, diff --git a/backend/tests/test_sandbox_types.py b/backend/tests/test_sandbox_types.py index bcc7be2..08c36a5 100644 --- a/backend/tests/test_sandbox_types.py +++ b/backend/tests/test_sandbox_types.py @@ -53,7 +53,7 @@ def test_failure(self): r = ExecutionResult(output="error", success=False, exit_code=1, duration_seconds=2.5) assert r.success is False assert r.exit_code == 1 - assert r.duration_seconds == 2.5 + assert r.duration_seconds == pytest.approx(2.5) def test_truncation_handled_by_caller(self): # Truncation is done by the tools, not the dataclass diff --git a/backend/tests/test_tools_workspace.py b/backend/tests/test_tools_workspace.py index 69edca8..1726eaf 100644 --- a/backend/tests/test_tools_workspace.py +++ b/backend/tests/test_tools_workspace.py @@ -1,5 +1,6 @@ """Tests for workspace agent tools.""" +import asyncio import os import tempfile @@ -177,10 +178,14 @@ async def test_recent_failures_empty(self, workspace_dir): assert "No recent" in result async def test_search_operation(self, workspace_dir): - # Create a test file + # Create a test file using async-compatible file write test_file = os.path.join(workspace_dir, "code", "test.py") - with open(test_file, "w") as f: - f.write("import torch\nmodel = TransformerModel()") + + def _write_file(): + with open(test_file, "w") as f: + f.write("import torch\nmodel = TransformerModel()") + + await asyncio.to_thread(_write_file) result, success = await _handle_workspace( operation="search", diff --git a/frontend/src/__tests__/ConfirmDialog.test.tsx b/frontend/src/__tests__/ConfirmDialog.test.tsx index 553bd48..f12cf58 100644 --- a/frontend/src/__tests__/ConfirmDialog.test.tsx +++ b/frontend/src/__tests__/ConfirmDialog.test.tsx @@ -98,7 +98,9 @@ describe('ConfirmDialog', () => { onCancel={onCancel} /> ); - fireEvent.keyDown(window, { key: 'Escape' }); + // The dialog handles escape via the native cancel event + const dialog = document.querySelector('dialog'); + fireEvent(dialog!, new Event('cancel')); expect(onCancel).toHaveBeenCalled(); }); diff --git a/frontend/src/__tests__/ModelModal.test.tsx b/frontend/src/__tests__/ModelModal.test.tsx index b24a35e..35f33d2 100644 --- a/frontend/src/__tests__/ModelModal.test.tsx +++ b/frontend/src/__tests__/ModelModal.test.tsx @@ -162,8 +162,9 @@ describe('ModelModal', () => { fireEvent.click(screen.getByText('Close')); + const dialog = document.querySelector('dialog'); await waitFor(() => { - expect(screen.queryByText('GPT-4o Mini')).not.toBeInTheDocument(); + expect(dialog).not.toHaveAttribute('open'); }); }); @@ -205,12 +206,12 @@ describe('ModelModal', () => { expect(screen.getAllByText('GPT-4o').length).toBeGreaterThanOrEqual(1); }); - // Find the overlay (the fixed div with bg-black/60) - const overlay = document.querySelector('.fixed.inset-0'); - fireEvent.click(overlay!); + // Click on the dialog element itself (backdrop area) to close + const dialog = document.querySelector('dialog'); + fireEvent.click(dialog!); await waitFor(() => { - expect(screen.queryByText('GPT-4o Mini')).not.toBeInTheDocument(); + expect(dialog).not.toHaveAttribute('open'); }); }); diff --git a/frontend/src/__tests__/Sidebar.test.tsx b/frontend/src/__tests__/Sidebar.test.tsx index f38b962..690e90b 100644 --- a/frontend/src/__tests__/Sidebar.test.tsx +++ b/frontend/src/__tests__/Sidebar.test.tsx @@ -93,7 +93,8 @@ describe('Sidebar', () => { ); - fireEvent.click(screen.getByText('First conversation')); + // The conversation item uses an overlay button with aria-label + fireEvent.click(screen.getByRole('button', { name: /Switch to conversation: First conversation/i })); expect(onSwitch).toHaveBeenCalledWith('conv-1'); }); diff --git a/frontend/src/__tests__/setup.ts b/frontend/src/__tests__/setup.ts index bb02c60..6c0d8cc 100644 --- a/frontend/src/__tests__/setup.ts +++ b/frontend/src/__tests__/setup.ts @@ -1 +1,10 @@ import '@testing-library/jest-dom/vitest'; + +// Mock HTMLDialogElement methods not implemented in jsdom +HTMLDialogElement.prototype.showModal = function() { + this.setAttribute('open', ''); +}; + +HTMLDialogElement.prototype.close = function() { + this.removeAttribute('open'); +}; diff --git a/frontend/src/components/ConfirmDialog.tsx b/frontend/src/components/ConfirmDialog.tsx index 30ae352..5c8225a 100644 --- a/frontend/src/components/ConfirmDialog.tsx +++ b/frontend/src/components/ConfirmDialog.tsx @@ -19,54 +19,70 @@ export function ConfirmDialog({ onCancel, danger = false, }: Props) { + const dialogRef = useRef(null); const cancelRef = useRef(null); useEffect(() => { + const dialog = dialogRef.current; + if (dialog && !dialog.open) { + dialog.showModal(); + } + // Focus cancel button by default for safety cancelRef.current?.focus(); - // Handle escape key - const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Escape') { - onCancel(); - } + // Handle escape key via dialog's native cancel event + const handleCancel = (e: Event) => { + e.preventDefault(); + onCancel(); }; - window.addEventListener('keydown', handleKeyDown); - return () => window.removeEventListener('keydown', handleKeyDown); + + dialog?.addEventListener('cancel', handleCancel); + return () => dialog?.removeEventListener('cancel', handleCancel); }, [onCancel]); + const handleBackdropClick = (e: React.MouseEvent) => { + // Close when clicking the backdrop (outside the dialog content) + if (e.target === dialogRef.current) { + onCancel(); + } + }; + return ( -
-
e.stopPropagation()} - > -

{title}

-

{message}

- -
- - +
+
+

{title}

+

{message}

+ +
+ + +
-
+ ); } diff --git a/frontend/src/components/FileTree.tsx b/frontend/src/components/FileTree.tsx index df49f94..7b6aa93 100644 --- a/frontend/src/components/FileTree.tsx +++ b/frontend/src/components/FileTree.tsx @@ -164,7 +164,7 @@ export function FileTree({ projectUuid, refreshKey, onFileSelect }: Props) { return (data.entries || []).map((entry: FileNode) => ({ ...entry, expanded: false, - children: entry.is_dir ? undefined : undefined, + children: undefined, })); } catch (err: any) { setError(err.message); diff --git a/frontend/src/components/ModelModal.tsx b/frontend/src/components/ModelModal.tsx index a13581b..9aa6378 100644 --- a/frontend/src/components/ModelModal.tsx +++ b/frontend/src/components/ModelModal.tsx @@ -1,6 +1,6 @@ -import { useState, useEffect, useMemo, useCallback } from 'react'; +import { useState, useEffect, useMemo, useCallback, useRef } from 'react'; import { Search, ChevronDown, Check, X, Filter, Save } from 'lucide-react'; import { api } from '../api'; import { useNavigate } from 'react-router-dom'; @@ -89,6 +89,7 @@ function LoadingSkeleton() { export function ModelModal({ currentModel, onModelChange }: Props) { const navigate = useNavigate(); + const dialogRef = useRef(null); const [open, setOpen] = useState(false); const [tab, setTab] = useState('models'); const [providers, setProviders] = useState([]); @@ -123,6 +124,26 @@ export function ModelModal({ currentModel, onModelChange }: Props) { loadData(); }, [open, loadData]); + // Handle dialog open/close + useEffect(() => { + const dialog = dialogRef.current; + if (!dialog) return; + + if (open && !dialog.open) { + dialog.showModal(); + } else if (!open && dialog.open) { + dialog.close(); + } + + const handleCancel = (e: Event) => { + e.preventDefault(); + setOpen(false); + }; + + dialog.addEventListener('cancel', handleCancel); + return () => dialog.removeEventListener('cancel', handleCancel); + }, [open]); + // Get only configured providers that are in the models category const configuredModelProviders = useMemo(() => { return providers.filter((p) => p.configured && p.categories?.includes('models')); @@ -248,20 +269,24 @@ export function ModelModal({ currentModel, onModelChange }: Props) { - {open && ( -
setOpen(false)} - > + { + if (e.target === dialogRef.current) setOpen(false); + }} + aria-labelledby="model-modal-title" + > +
{/* Fixed-size modal — never changes dimensions between loading and loaded */}
e.stopPropagation()} > {/* Tabs */}
- )} +
); } diff --git a/frontend/src/components/OnboardingModal.tsx b/frontend/src/components/OnboardingModal.tsx index 66844f1..7e859ab 100644 --- a/frontend/src/components/OnboardingModal.tsx +++ b/frontend/src/components/OnboardingModal.tsx @@ -280,10 +280,11 @@ export function OnboardingModal({ onComplete }: Props) { with files, knowledge graph, and conversation history.

-
-