-
Notifications
You must be signed in to change notification settings - Fork 47
DRAFT: Feat/OpenAI compatible api #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add BuildConfig.ts with AUTOMATED_MODE flag - Bypass OAuth authentication in automated mode - Auto-enable evaluation mode in automated mode - Configure Docker build with ARG support - Allows headless/automated usage without manual configuration
WalkthroughIntroduces an OpenAI-compatible HTTP API for the eval server (NodeJS and Python), adds examples, updates Python packaging/deps, and documents the new API. Adds an automated mode toggle for AI Chat via BuildConfig, Docker build arg, and UI/config behavior changes. Extends timeouts, updates build/manifests, and adds a local nginx Dockerfile. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client (OpenAI SDK/cURL)
participant H as OpenAI-Compatible HTTP Server
participant E as EvalServer (WS)
participant T as DevTools Tab (AI Chat)
Note over H,E: H depends on E being running and having a ready tab
C->>H: POST /v1/chat/completions
H->>H: Validate & convert OpenAI request
H->>E: Request evaluation (get ready client)
E->>T: Execute evaluation payload
T-->>E: Result (tool/chat output)
E-->>H: Evaluation result
H->>H: Convert to OpenAI response
H-->>C: 200 OK (chat completion)
alt No ready clients
H-->>C: 503 Service Unavailable
end
sequenceDiagram
autonumber
participant U as User/CI
participant D as Docker Build
participant FE as AI Chat Frontend
U->>D: docker build --build-arg AUTOMATED_MODE=true
D->>FE: Patch BuildConfig.AUTOMATED_MODE=true
U->>FE: Load UI
FE->>FE: Auto-enable evaluation if not set
FE->>FE: Hide OAuth login UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
Moving this functionality to feat/extend-openai-api branch 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Simple Dockerfile that uses pre-built local files - Uses nginx:alpine for lightweight serving - Designed for local development workflows
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
toolResult = await this.executeToolWithTimeout( | ||
tool, | ||
params.input, | ||
params.timeout || 30000, | ||
params.timeout || 2700000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Correct chat/tool default timeouts
The new default timeout constant is 2700000
, which equals 45 minutes, while the comment explicitly states “Default 15 minutes for chat” and the tool branch previously defaulted to 30 seconds. This multiplication by 90× means a failed or hanging evaluation may block the UI for three‑quarters of an hour before timing out, which is far longer than either the documented 15 minutes or the prior behaviour. Consider replacing the literal with 900000 for a true 15‑minute chat timeout and restoring a shorter default for regular tools so that broken evaluations fail promptly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts (1)
266-299
: Auth flow leaks shared secret and allows self-attestation.Server sends
serverSecretKey
to the client and the client replies with a boolean “verified”. This exposes the secret to the client and lets a malicious client respondverified: true
without proof-of-knowledge. Implement a proper challenge–response (e.g., server sends nonce; client returns HMAC(secret, nonce); server verifies) or use signed tokens, never transmit the raw secret.Proposed sketch:
- const verified = clientSecretKey === message.serverSecretKey; - const authMessage = createAuthVerifyMessage(message.clientId, verified); + // server sends: { nonce } + const signature = hmacSHA256(clientSecretKey, message.nonce); + const authMessage = createAuthVerifyMessage(message.clientId, { signature })And on the server, verify
hmacSHA256(storedSecret, nonce) === signature
.docker/README.md (1)
207-213
: Security note inaccurate: container runs as root unless USER is set.Official nginx image defaults to root. Either add
USER nginx
in Dockerfiles or adjust the README.Suggested Dockerfile snippet:
+# Drop privileges +USER nginxfront_end/panels/ai_chat/ui/AIChatPanel.ts (1)
52-61
: Stop logging secrets; gate storage instrumentation behind an explicit debug flag.Console logs currently expose key presence, lengths, and prefixes; StorageMonitor also logs value previews. This is unsafe in production.
- Redact sensitive values and remove length/prefix logs.
- Only enable StorageMonitor when a debug flag is set.
@@ - localStorage.setItem = (key: string, value: string) => { + localStorage.setItem = (key: string, value: string) => { if (key.includes('openrouter') || key.includes('ai_chat')) { logger.debug(`=== LOCALSTORAGE SET ===`); logger.debug(`Key: ${key}`); - logger.debug(`Value exists: ${!!value}`); - logger.debug(`Value length: ${value?.length || 0}`); - logger.debug(`Value preview: ${value?.substring(0, 50) + (value?.length > 50 ? '...' : '') || 'null'}`); + logger.debug(`Has value: ${Boolean(value)} (redacted)`); logger.debug(`Timestamp: ${new Date().toISOString()}`); } return this.originalSetItem(key, value); }; @@ -localStorage.removeItem = (key: string) => { +localStorage.removeItem = (key: string) => { if (key.includes('openrouter') || key.includes('ai_chat')) { logger.debug(`=== LOCALSTORAGE REMOVE ===`); logger.debug(`Key: ${key}`); logger.debug(`Timestamp: ${new Date().toISOString()}`); } return this.originalRemoveItem(key); };@@ - // Initialize storage monitoring for debugging - StorageMonitor.getInstance(); + // Initialize storage monitoring for debugging (opt-in only) + if (localStorage.getItem('ai_chat_debug_storage') === '1') { + StorageMonitor.getInstance(); + }@@ - logger.info('Storage keys for provider:'); - logger.info('- API key storage key:', storageKeys.apiKey); + logger.info('Storage keys for provider retrieved'); @@ - logger.info('Retrieved API key:'); - logger.info('- Exists:', !!apiKey); - logger.info('- Length:', apiKey?.length || 0); - logger.info('- Prefix:', apiKey?.substring(0, 8) + '...' || 'none'); + logger.info('Retrieved API key: present =', Boolean(apiKey));Also applies to: 65-72, 731-733, 1469-1474
eval-server/python/src/bo_eval_server.egg-info/requires.txt (1)
1-16
: Generated dependency list should not be versioned.Delete
requires.txt
(generated by setuptools) and rely onpyproject.toml
as the single source of truth.eval-server/README.md (1)
116-127
: Fix table inconsistency: Python has an HTTP API wrapper now.Row “HTTP API Wrapper” shows Python ❌, but the text and code add a FastAPI server. Mark Python as ✅.
-| **HTTP API Wrapper** | ✅ | ❌ | +| **HTTP API Wrapper** | ✅ | ✅ |eval-server/python/pyproject.toml (3)
50-53
: Repository URLs point to Chromium DevTools.Update URLs to this repository to avoid packaging confusion.
-[project.urls] -Homepage = "https://github.com/chromium/devtools-frontend" -Repository = "https://github.com/chromium/devtools-frontend" -Issues = "https://github.com/chromium/devtools-frontend/issues" +[project.urls] +Homepage = "https://github.com/BrowserOperator/browser-operator-core" +Repository = "https://github.com/BrowserOperator/browser-operator-core" +Issues = "https://github.com/BrowserOperator/browser-operator-core/issues"
55-59
: Console scripts targetscripts:...
but no such module is packaged.Point to functions inside your package (recommended) or add the module. This must match entry points used by setuptools.
-[project.scripts] -bo-eval-basic = "scripts:run_basic_server" -bo-eval-stack = "scripts:run_with_stack" -bo-eval-programmatic = "scripts:run_programmatic_evals" +[project.scripts] +bo-eval-basic = "bo_eval_server.scripts:run_basic_server" +bo-eval-stack = "bo_eval_server.scripts:run_with_stack" +bo-eval-programmatic = "bo_eval_server.scripts:run_programmatic_evals"If
bo_eval_server/scripts.py
doesn’t exist, I can generate a minimal one.
42-49
: Duplicate dev dependency definitions.You define dev deps in both
[project.optional-dependencies].dev
and[dependency-groups].dev
with differing pins. Keep a single source (prefer the standard[project.optional-dependencies]
), or ensure both match to avoid confusion.-[dependency-groups] -dev = [ - "black>=24.8.0", - "mypy>=1.14.1", - "pytest>=8.3.5", - "pytest-asyncio>=0.24.0", -]Also applies to: 83-90
eval-server/python/src/bo_eval_server.egg-info/PKG-INFO (1)
1-408
: Do not commit generated egg-info; add to .gitignorebo_eval_server.egg-info is a build artifact and will cause churn/merge conflicts.
Add to .gitignore and remove from VCS:
+eval-server/python/src/bo_eval_server.egg-info/
Then delete the directory from the branch.
♻️ Duplicate comments (2)
front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts (2)
457-462
: Timeout literal contradicts comment (2700000 ms ≠ 15 minutes).Use 900000 for 15 minutes, or update the comment. Prefer sourcing from a constant/config.
- params.timeout || 2700000, // Default 15 minutes for chat + params.timeout ?? 900000, // Default 15 minutes for chat
466-472
: Tool default timeout is now 45 minutes—too long for broken tools.Restore a short default for tools (e.g., 30–60s) and keep longer chat timeouts. Make both configurable.
- params.timeout || 2700000, + params.timeout ?? 60000, // 60s default for tools; override via request or configOptionally add module-level constants or read from EvaluationConfig.
🧹 Nitpick comments (34)
front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts (2)
340-349
: Avoid re-initializing tracing provider on every evaluation.Initialize once and reuse; guard with an internal flag to skip repeated expensive init.
- await this.tracingProvider.initialize(); + if (!(this as any)._tracingInitialized) { + await this.tracingProvider.initialize(); + (this as any)._tracingInitialized = true; +}
592-637
: Timeout wrapper is racy and retains timers on fast settle.Switch to
Promise.race
with a single timeout promise; ensure observation is finalized once.- return new Promise((resolve, reject) => { - const timer = setTimeout(() => { ...reject(...) }, timeout); - const executePromise = ...tool.execute(input); - executePromise.then(...).catch(...); - }); + const exec = tracingContext ? withTracingContext(tracingContext, () => tool.execute(input)) : tool.execute(input); + const timedOut = new Error(`Tool execution timeout after ${timeout}ms`); + const timeoutP = new Promise((_, rej) => setTimeout(() => rej(timedOut), timeout)); + return Promise.race([exec, timeoutP]) + .then(result => { /* update observation */ return result; }) + .catch(err => { /* update observation */ throw err; });front_end/panels/ai_chat/core/BuildConfig.ts (1)
5-20
: Fix lint: trailing space and missing EOF newline.Also small nit: keep object literal tidy.
- * + * @@ - AUTOMATED_MODE: false, // Will be set to true during Docker build -} as const; + AUTOMATED_MODE: false, // Will be set to true during Docker build +} as const; +docker/Dockerfile (2)
54-57
: Brittle sed-based source patching.Prefer a build-time define/ENV consumed by TS (e.g., GN arg or inject via generated file), avoiding in-place edits that can drift.
Proposals:
- Generate BuildConfig.ts from a template with
AUTOMATED_MODE
set by an env var.- Or read
AUTOMATED_MODE
at runtime fromwindow.__CONFIG__
injected by nginx.
39-44
: Unnecessary double build increases image time/size.You build DevTools before switching to Browser Operator fork and building again. Consider removing the first
npm run build
to speed up builds.-# Build standard DevTools first -RUN npm run builddocker/README.md (2)
38-43
: Container names/tags: keep consistent across examples.Some sections use
browser-operator
, othersdevtools-frontend
. Normalize to the chosen names to reduce confusion.-docker run -d -p 8000:8000 --name browser-operator-automated browser-operator-automated +docker run -d -p 8000:8000 --name browser-operator-automated browser-operator-automated # Ensure later examples use the same image/container names.
112-116
: Advanced usage still references devtools-frontend image.Update to the new image name(s) to avoid 404s for new users.
- --name devtools-frontend-dev \ - devtools-frontend + --name browser-operator-automated-dev \ + browser-operator-automatedfront_end/panels/ai_chat/common/EvaluationConfig.ts (2)
5-9
: Import order/lint.ESLint flags order and trailing space; reorder imports and remove trailing space.
-import { createLogger } from '../core/Logger.js'; -import { WebSocketRPCClient } from './WebSocketRPCClient.js'; -import { BUILD_CONFIG } from '../core/BuildConfig.js'; +import { BUILD_CONFIG } from '../core/BuildConfig.js'; +import { createLogger } from '../core/Logger.js'; +import { WebSocketRPCClient } from './WebSocketRPCClient.js';
97-112
: Dead field: rpcClient is never assigned.Since connections moved per-tab, drop the field and related disconnect block to avoid confusion.
- private rpcClient: WebSocketRPCClient | null = null; @@ - // Disconnect existing client if configuration changed - if (this.rpcClient) { - this.rpcClient.disconnect(); - this.rpcClient = null; - }front_end/panels/ai_chat/ui/AIChatPanel.ts (3)
23-23
: Fix import order to satisfy ESLint (import/order).Move
BUILD_CONFIG
aboveLLMClient
to resolve the linter error.import {AgentService, Events as AgentEvents} from '../core/AgentService.js'; +import { BUILD_CONFIG } from '../core/BuildConfig.js'; -import { LLMClient } from '../LLM/LLMClient.js'; -import { LLMProviderRegistry } from '../LLM/LLMProviderRegistry.js'; +import { LLMClient } from '../LLM/LLMClient.js'; +import { LLMProviderRegistry } from '../LLM/LLMProviderRegistry.js'; -import { BUILD_CONFIG } from '../core/BuildConfig.js';Also applies to: 13-20
1399-1418
: Align function behavior with its name and fix trailing whitespace.Include LiteLLM in the “any provider” check; the current conditional exclusion is surprising and can cause false negatives. Also remove trailing spaces on Line 1399.
- - const selectedProvider = localStorage.getItem(PROVIDER_SELECTION_KEY) || 'openai'; - - // Check all providers except LiteLLM (unless LiteLLM is selected) - const providers = ['openai', 'groq', 'openrouter']; - - // Only include LiteLLM if it's the selected provider - if (selectedProvider === 'litellm') { - providers.push('litellm'); - } + const providers = ['openai', 'groq', 'openrouter', 'litellm'];
100-109
: Avoid hard-coding model IDs that can drift; prefer provider-driven lists with fallbacks.These IDs change frequently and will desync the UI. Consider fetching defaults from the active provider (or cached provider lists) and using a minimal, stable fallback set.
If helpful, I can wire
LLMClient.getAvailableModels()
into the initial model list with a short cache TTL and fallbacks. Want a patch?Also applies to: 112-133
eval-server/README.md (2)
66-80
: Specify a language for the fenced ASCII diagram.Add a language to satisfy markdownlint (MD040) and improve rendering.
-``` +```text ...--- `90-103`: **Add Authorization header to examples.** Your Node wrapper supports CORS but no auth; for OpenAI-compat UX, show a Bearer token example (and consider enforcing it in code). ```diff curl -X POST http://localhost:8081/v1/chat/completions \ -H "Content-Type: application/json" \ + -H "Authorization: Bearer $OPENAI_API_KEY" \ -d '{ "model": "gpt-4.1", "messages": [ {"role": "user", "content": "Hello, how are you?"} ] }'
eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js (8)
59-73
: Set no-store caching and preflight early.Add
Cache-Control: no-store
to avoid caching sensitive responses.this.server = http.createServer((req, res) => { // Enable CORS res.setHeader('Access-Control-Allow-Origin', '*'); res.setHeader('Access-Control-Allow-Methods', 'GET, POST, OPTIONS'); res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization'); + res.setHeader('Cache-Control', 'no-store');
156-166
: Return OpenAI-like error codes/types.Map common errors to OpenAI-style
type
andcode
for compatibility.- if (error.message.includes('No connected')) { - this.sendError(res, 503, error.message); + if (error.message.includes('No connected')) { + this.sendError(res, 503, 'No connected DevTools tabs available'); } else if (error.name === 'SyntaxError') { - this.sendError(res, 400, 'Invalid JSON in request body'); + this.sendError(res, 400, 'Invalid JSON in request body'); } else { - this.sendError(res, 500, `Internal server error: ${error.message}`); + this.sendError(res, 500, 'Internal server error'); }
337-347
: Remove unused catch variable and prefer const.
jsonError
is unused;response
is not reassigned. Clean up per ESLint.- let response = result.response; + const response = result.response; if (typeof response === 'string') { try { const parsed = JSON.parse(response); if (parsed.models) { modelData = parsed.models; currentSelection = parsed.currentSelection; } - } catch (jsonError) { + } catch { // Not JSON, continue }
429-452
: OpenAI → internal message conversion is lossy.Roles/tool calls/functions are flattened to text. If you aim for broader compatibility, consider preserving structured tool calls and system prompts separately. Otherwise, call this limitation out in docs.
551-584
: Token usage is a rough estimate.Word-split counts drift from tokenizers. If usage drives billing/limits, integrate a tokenizer (optional).
628-635
: Remove unused destructured key.
clientId
isn’t used; iterate values directly.- for (const [clientId, connection] of this.evalServer.connectedClients) { + for (const connection of this.evalServer.connectedClients.values()) { if (connection.ready && connection.registered) { return connection; } }
668-686
: Align error shape closer to OpenAI.Consider including
param: null
and more specifictype
values (invalid_request_error
,authentication_error
, etc.) to maximize client compatibility.
700-700
: Add final newline.POSIX newline at EOF prevents tooling diffs/ESLint eol-last errors.
eval-server/nodejs/examples/openai-server-example.js (4)
59-70
: Arrow-parens style + disconnect payload keyESLint flags parens on single-arg arrows; also disconnect logs
clientInfo.clientId
while connect usesclient.id
. Align toid
to avoid undefined.Apply:
- evalServer.onConnect((client) => { + evalServer.onConnect(client => { @@ - evalServer.onDisconnect((clientInfo) => { - console.log(`❌ DevTools tab disconnected: ${clientInfo.clientId}`); + evalServer.onDisconnect(clientInfo => { + console.log(`❌ DevTools tab disconnected: ${clientInfo.id}`); });
79-86
: Clear the status interval on shutdown (don’t rely on beforeExit)Ensure the interval is cleared even on SIGINT/SIGTERM.
- // Graceful shutdown handler + // Graceful shutdown handler + let statusInterval; const shutdown = async () => { console.log('\n🛑 Shutting down servers...'); try { + if (statusInterval) clearInterval(statusInterval); await openaiWrapper.stop(); await evalServer.stop(); @@ - const statusInterval = setInterval(() => { + statusInterval = setInterval(() => { const evalStatus = evalServer.getStatus(); const openaiStatus = openaiWrapper.getStatus(); @@ - process.on('beforeExit', () => { - clearInterval(statusInterval); - }); + process.on('beforeExit', () => statusInterval && clearInterval(statusInterval));Also applies to: 113-121, 123-125
1-2
: Silence example-only console lint, strip trailing spaces, add EOLExamples intentionally print to console. Add a file-level disable and fix lint nits to keep CI green.
#!/usr/bin/env node +/* eslint-disable no-console */
Also remove trailing spaces and ensure a newline at EOF.
Also applies to: 18-47, 49-121, 133-143
51-56
: Optional: read host/ports/auth from env for parity with other partsKeep hardcoded defaults but allow overrides for local setups.
- const evalServer = new EvalServer({ - authKey: 'hello', - host: '127.0.0.1', - port: 8080 - }); + const evalServer = new EvalServer({ + authKey: process.env.BO_AUTH_KEY ?? 'hello', + host: process.env.BO_HOST ?? '127.0.0.1', + port: parseInt(process.env.BO_WS_PORT ?? '8080', 10) + }); @@ - const openaiWrapper = new OpenAICompatibleWrapper(evalServer, { - host: '127.0.0.1', - port: 8081, + const openaiWrapper = new OpenAICompatibleWrapper(evalServer, { + host: process.env.BO_HOST ?? '127.0.0.1', + port: parseInt(process.env.BO_HTTP_PORT ?? '8081', 10), modelCacheTTL: 300000 // 5 minutes }); @@ - console.log('🔧 Starting WebSocket evaluation server on ws://127.0.0.1:8080'); + console.log(`🔧 Starting WebSocket evaluation server on ws://${process.env.BO_HOST ?? '127.0.0.1'}:${process.env.BO_WS_PORT ?? '8080'}`); @@ - console.log('🔧 Starting OpenAI-compatible API server on http://127.0.0.1:8081'); + console.log(`🔧 Starting OpenAI-compatible API server on http://${process.env.BO_HOST ?? '127.0.0.1'}:${process.env.BO_HTTP_PORT ?? '8081'}`); @@ - console.log('📡 WebSocket Server: ws://127.0.0.1:8080 (for DevTools connections)'); - console.log('🌐 OpenAI API Server: http://127.0.0.1:8081 (for HTTP requests)'); + console.log(`📡 WebSocket Server: ws://${process.env.BO_HOST ?? '127.0.0.1'}:${process.env.BO_WS_PORT ?? '8080'} (for DevTools connections)`); + console.log(`🌐 OpenAI API Server: http://${process.env.BO_HOST ?? '127.0.0.1'}:${process.env.BO_HTTP_PORT ?? '8081'} (for HTTP requests)`);Also applies to: 72-77, 97-110
eval-server/python/examples/openai_server_example.py (1)
69-76
: Avoid bare excepts; log errors during shutdownReplace bare excepts with explicit Exception and log the failure.
- try: - await openai_server.stop() - except: - pass + try: + await openai_server.stop() + except Exception as e: + logging.exception("Error while stopping OpenAI server: %s", e) try: await eval_server.stop() - except: - pass + except Exception as e: + logging.exception("Error while stopping EvalServer: %s", e)eval-server/python/src/bo_eval_server.egg-info/PKG-INFO (1)
46-53
: Feature blurb contradicts dependenciesIt says “Only websockets and loguru required” while runtime deps include FastAPI/uvicorn/pydantic/httpx/openai. Update the long description in packaging config accordingly.
eval-server/python/src/bo_eval_server/openai_server.py (6)
118-127
: Return a useful model list when no clients are connectedCurrently, no clients => empty list. Mirror your fallback logic to return defaults for a better OpenAI UX.
async def list_models(): @@ - except Exception as e: + except Exception as e: logger.error(f"Error listing models: {e}") - raise HTTPException(status_code=503, detail="Unable to fetch models from connected tabs") + raise HTTPException(status_code=503, detail="Unable to fetch models from connected tabs") from eAnd in
_get_models_from_tabs
:- if not ready_client: - logger.warning("No connected tabs available for model listing") - return [] + if not ready_client: + logger.warning("No connected tabs available for model listing; returning defaults") + now = int(current_time) + return [ + OpenAIModelInfo(id="gpt-4.1", created=now, owned_by="browser-operator"), + OpenAIModelInfo(id="gpt-4.1-mini", created=now, owned_by="browser-operator"), + OpenAIModelInfo(id="gpt-4.1-nano", created=now, owned_by="browser-operator"), + ]
128-154
: Explicitly handle stream=True as unsupportedClients may set stream; respond clearly instead of silently ignoring.
async def chat_completions(request: OpenAIChatCompletionRequest): """Handle chat completion requests""" try: + if request.stream: + raise HTTPException(status_code=501, detail="stream=true is not supported yet") @@ - except Exception as e: - logger.error(f"Error processing chat completion: {e}") - raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + except Exception as e: + logger.error(f"Error processing chat completion: {e}") + raise HTTPException(status_code=500, detail=f"Internal server error: {e!s}") from e
155-188
: Responses API shape and error chaining
- Consider returning an OpenAI Responses object (id/object/output[]) rather than a bare list to be closer to spec.
- Chain exceptions and avoid
str(e)
per Ruff.- @self.app.post("/v1/responses") - async def responses_api(request: Dict[str, Any]): + @self.app.post("/v1/responses") + async def responses_api(request: Dict[str, Any]): @@ - except Exception as e: - logger.error(f"Error processing responses request: {e}") - raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + except Exception as e: + logger.error(f"Error processing responses request: {e}") + raise HTTPException(status_code=500, detail=f"Internal server error: {e!s}") from eIf you want a spec-shaped response, I can draft that as a follow-up.
200-286
: Model fetch: minor cleanups and cache semantics
- When falling back, also set object defaults via pydantic (fine as-is).
- Consider shorter timeout (e.g., 5s) to avoid long hangs.
- "timeout": 10000 # 10 second timeout for model listing + "timeout": 5000 # 5 second timeout for model listing
547-592
: Extraction fallback may leak large objectsReturning
json.dumps(result, indent=2)
can be verbose. Truncate to a reasonable length.- return json.dumps(result, indent=2) + raw = json.dumps(result, indent=2) + return raw if len(raw) <= 4000 else raw[:4000] + "…"
80-101
: Doc note: TTL unit differs from Node wrapperPython uses seconds; Node uses milliseconds. Add a brief note in docstring or README to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
eval-server/python/uv.lock
is excluded by!**/*.lock
📒 Files selected for processing (21)
config/gni/devtools_grd_files.gni
(1 hunks)docker/Dockerfile
(2 hunks)docker/Dockerfile.local
(1 hunks)docker/README.md
(3 hunks)eval-server/README.md
(3 hunks)eval-server/nodejs/examples/openai-server-example.js
(1 hunks)eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js
(1 hunks)eval-server/python/examples/openai_server_example.py
(1 hunks)eval-server/python/pyproject.toml
(1 hunks)eval-server/python/src/bo_eval_server.egg-info/PKG-INFO
(1 hunks)eval-server/python/src/bo_eval_server.egg-info/SOURCES.txt
(1 hunks)eval-server/python/src/bo_eval_server.egg-info/dependency_links.txt
(1 hunks)eval-server/python/src/bo_eval_server.egg-info/entry_points.txt
(1 hunks)eval-server/python/src/bo_eval_server.egg-info/requires.txt
(1 hunks)eval-server/python/src/bo_eval_server.egg-info/top_level.txt
(1 hunks)eval-server/python/src/bo_eval_server/openai_server.py
(1 hunks)front_end/panels/ai_chat/BUILD.gn
(2 hunks)front_end/panels/ai_chat/common/EvaluationConfig.ts
(2 hunks)front_end/panels/ai_chat/core/BuildConfig.ts
(1 hunks)front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts
(2 hunks)front_end/panels/ai_chat/ui/AIChatPanel.ts
(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: BrowserOperator/browser-operator-core#0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-08-29T23:08:07.342Z
Learning: Applies to eval-server/nodejs/src/config.js : Require OPENAI_API_KEY and PORT (default 8080) from environment configuration
📚 Learning: 2025-08-29T23:08:07.342Z
Learnt from: CR
PR: BrowserOperator/browser-operator-core#0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-08-29T23:08:07.342Z
Learning: Applies to eval-server/nodejs/src/config.js : Require OPENAI_API_KEY and PORT (default 8080) from environment configuration
Applied to files:
eval-server/nodejs/examples/openai-server-example.js
📚 Learning: 2025-08-29T23:08:07.342Z
Learnt from: CR
PR: BrowserOperator/browser-operator-core#0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-08-29T23:08:07.342Z
Learning: Applies to eval-server/nodejs/src/server.js : WebSocket server must accept agent connections, manage agent lifecycle (connect/ready/disconnect), orchestrate evaluation sessions, and handle bidirectional RPC communication
Applied to files:
eval-server/nodejs/examples/openai-server-example.js
eval-server/python/src/bo_eval_server.egg-info/PKG-INFO
📚 Learning: 2025-08-29T23:08:07.342Z
Learnt from: CR
PR: BrowserOperator/browser-operator-core#0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-08-29T23:08:07.342Z
Learning: Applies to eval-server/nodejs/src/logger.js : Log all RPC calls, evaluations, and connection events
Applied to files:
front_end/panels/ai_chat/common/EvaluationConfig.ts
🧬 Code graph analysis (6)
eval-server/nodejs/examples/openai-server-example.js (3)
eval-server/python/src/bo_eval_server/openai_server.py (1)
main
(619-640)eval-server/python/examples/openai_server_example.py (1)
main
(19-76)eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js (1)
OpenAICompatibleWrapper
(27-700)
eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js (1)
eval-server/nodejs/examples/openai-server-example.js (1)
evalServer
(52-56)
eval-server/python/examples/openai_server_example.py (1)
eval-server/python/src/bo_eval_server/openai_server.py (4)
OpenAICompatibleServer
(72-616)main
(619-640)start
(594-610)stop
(612-616)
front_end/panels/ai_chat/ui/AIChatPanel.ts (1)
front_end/panels/ai_chat/core/BuildConfig.ts (1)
BUILD_CONFIG
(11-20)
front_end/panels/ai_chat/common/EvaluationConfig.ts (1)
front_end/panels/ai_chat/core/BuildConfig.ts (1)
BUILD_CONFIG
(11-20)
eval-server/python/src/bo_eval_server/openai_server.py (2)
eval-server/nodejs/examples/openai-server-example.js (1)
shutdown
(80-91)eval-server/python/examples/openai_server_example.py (1)
main
(19-76)
🪛 ESLint
eval-server/nodejs/examples/openai-server-example.js
[error] 9-9: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 18-18: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 49-49: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 50-50: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 59-59: Unexpected parentheses around single function argument.
(@stylistic/arrow-parens)
[error] 60-60: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 61-61: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 62-62: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 63-63: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 64-64: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 68-68: Unexpected parentheses around single function argument.
(@stylistic/arrow-parens)
[error] 69-69: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 81-81: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 85-85: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 99-99: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 101-101: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 103-103: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 105-105: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 106-106: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 107-107: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 108-108: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 109-109: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 110-110: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 111-111: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 112-112: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 117-117: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 118-118: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 119-119: Unexpected console statement. Only these console methods are allowed: assert, context, error, timeStamp, time, timeEnd, warn.
(no-console)
[error] 121-121: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 126-126: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 138-138: Unexpected parentheses around single function argument.
(@stylistic/arrow-parens)
[error] 143-143: Newline required at end of file but not found.
(@stylistic/eol-last)
eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js
[error] 5-5: There should be at least one empty line between import groups
(import/order)
[error] 10-10: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 12-12: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 14-14: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 19-19: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 22-22: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 36-36: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 39-39: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 58-58: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 90-90: Unexpected parentheses around single function argument.
(@stylistic/arrow-parens)
[error] 157-157: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 175-175: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 267-267: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 281-281: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 329-329: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 334-334: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 337-337: 'response' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 345-345: 'jsonError' is defined but never used.
(no-unused-vars)
[error] 379-379: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 403-403: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 444-444: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 447-447: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 504-504: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 629-629: 'clientId' is assigned a value but never used.
(no-unused-vars)
[error] 658-658: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 678-678: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 680-680: Expected property shorthand.
(object-shorthand)
[error] 700-700: Newline required at end of file but not found.
(@stylistic/eol-last)
front_end/panels/ai_chat/ui/AIChatPanel.ts
[error] 23-23: ../core/BuildConfig.js
import should occur before import of ../LLM/LLMClient.js
(import/order)
[error] 1399-1399: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
front_end/panels/ai_chat/common/EvaluationConfig.ts
[error] 7-7: ../core/BuildConfig.js
import should occur before import of ../core/Logger.js
(import/order)
[error] 54-54: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
front_end/panels/ai_chat/core/BuildConfig.ts
[error] 7-7: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 20-20: Newline required at end of file but not found.
(@stylistic/eol-last)
🪛 Ruff (0.12.2)
eval-server/python/examples/openai_server_example.py
1-1: Shebang is present but file is not executable
(EXE001)
65-65: Do not catch blind exception: Exception
(BLE001)
71-71: Do not use bare except
(E722)
71-72: try
-except
-pass
detected, consider logging the exception
(S110)
75-75: Do not use bare except
(E722)
75-76: try
-except
-pass
detected, consider logging the exception
(S110)
eval-server/python/src/bo_eval_server/openai_server.py
124-124: Do not catch blind exception: Exception
(BLE001)
126-126: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
137-137: Abstract raise
to an inner function
(TRY301)
151-151: Do not catch blind exception: Exception
(BLE001)
153-153: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
153-153: Use explicit conversion flag
Replace with conversion flag
(RUF010)
161-164: Abstract raise
to an inner function
(TRY301)
171-171: Abstract raise
to an inner function
(TRY301)
185-185: Do not catch blind exception: Exception
(BLE001)
187-187: Within an except
clause, raise exceptions with raise ... from err
or raise ... from None
to distinguish them from errors in exception handling
(B904)
187-187: Use explicit conversion flag
Replace with conversion flag
(RUF010)
247-247: Consider moving this statement to an else
block
(TRY300)
249-249: Do not catch blind exception: Exception
(BLE001)
364-364: Do not catch blind exception: Exception
(BLE001)
597-597: Avoid specifying long messages outside the exception class
(TRY003)
🪛 Pylint (3.3.7)
eval-server/python/examples/openai_server_example.py
[error] 1-1: Unrecognized option found: profile, include-ids, files-output, comment, zope, required-attributes, bad-functions, max-branchs, ignore-iface-methods
(E0015)
[refactor] 1-1: Useless option value for '--disable', 'R0201' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'W0142' was removed from pylint, see https://pylint.readthedocs.io/en/latest/whatsnew/1/1.4.html#what-s-new-in-pylint-1-4-3.
(R0022)
eval-server/python/src/bo_eval_server/openai_server.py
[error] 1-1: Unrecognized option found: profile, include-ids, files-output, comment, zope, required-attributes, bad-functions, max-branchs, ignore-iface-methods
(E0015)
[refactor] 1-1: Useless option value for '--disable', 'R0201' was moved to an optional extension, see https://pylint.readthedocs.io/en/latest/whatsnew/2/2.14/summary.html#removed-checkers.
(R0022)
[refactor] 1-1: Useless option value for '--disable', 'W0142' was removed from pylint, see https://pylint.readthedocs.io/en/latest/whatsnew/1/1.4.html#what-s-new-in-pylint-1-4-3.
(R0022)
[refactor] 301-374: Too many nested blocks (6/5)
(R1702)
[refactor] 288-288: Too many branches (16/12)
(R0912)
🪛 markdownlint-cli2 (0.17.2)
eval-server/README.md
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (16)
config/gni/devtools_grd_files.gni (1)
658-659
: LLMTracingWrapper included in GRD (good catch).Matches the new TS source added in ai_chat/BUILD.gn; should satisfy release GRD checks.
docker/Dockerfile.local (1)
10-10
: No change required — nginx listens on 8000.docker/nginx.conf contains
listen 8000;
andlisten [::]:8000;
, so EXPOSE 8000 in docker/Dockerfile.local is correct.docker/Dockerfile (1)
71-71
: EXPOSE 8000 matches nginx listen port — no change required.Verified: docker/nginx.conf listens on 8000 (lines 2–3), so keeping EXPOSE 8000 in docker/Dockerfile is correct.
docker/README.md (2)
90-97
: WebSocket endpoint note is helpful.Clear, actionable guidance for eval server connectivity.
31-33
: Compose file present — no action required. Verified docker/docker-compose.yml exists in the repository; keep the docker-compose instruction as-is.front_end/panels/ai_chat/BUILD.gn (2)
51-51
: BuildConfig wired into module (good).Ensures the config is compiled and available to dependent code.
180-180
: Included in metadata sources as well.Prevents GRD metadata mismatches during release builds.
front_end/panels/ai_chat/common/EvaluationConfig.ts (1)
53-58
: Default-enable in automated mode (sensible).Good UX for headless deployments.
eval-server/python/src/bo_eval_server.egg-info/requires.txt (1)
1-10
: Verify runtime compat across FastAPI/Pydantic/Uvicorn/OpenAI.Since these are core runtime deps, confirm versions align with your supported Python versions and internal imports. Suggest pinning minimums only in
pyproject.toml
(already done) and running CI against 3.8–3.12.Would you like a tox matrix added to run tests across 3.8–3.12?
eval-server/python/src/bo_eval_server.egg-info/entry_points.txt (1)
1-4
: Incorrect —scripts
module is present; update entry-points or packaging
- Found: eval-server/python/scripts.py defines run_basic_server, run_programmatic_evals, and run_with_stack.
- Problem: entry_points.txt (eval-server/python/src/bo_eval_server.egg-info/entry_points.txt) references
scripts:...
— that only resolves if a top-levelscripts
module is installed. Fixes: move scripts.py into the package (e.g., src/bo_eval_server/scripts.py or bo_eval_server/scripts.py), or change entry points to use the package path (bo_eval_server.scripts:run_basic_server), or ensure your packaging explicitly installs the top-level scripts module.- Note: pyproject.toml not found in the repo root; do not commit generated egg-info — generate entry points from your packaging config (pyproject.toml or setup.cfg) when building/publishing.
Likely an incorrect or invalid review comment.
eval-server/python/pyproject.toml (2)
35-40
: Runtime deps for OpenAI-compatible API look reasonable.FastAPI ≥0.100 aligns with Pydantic v2; Uvicorn and httpx versions are compatible. No action needed here.
9-9
: README path verified — no action required.
README.md exists at eval-server/python/README.md; pyproject.toml's readme setting is correct.eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js (1)
281-292
: Evaluation payload: ensure required protocol fields exist.If the eval protocol requires
name
andtool
, you're good. Ifurl
isn’t used, consider omitting it to reduce ambiguity.eval-server/nodejs/examples/openai-server-example.js (1)
3-6
: Confirm/align license header in eval-server/nodejs/examples/openai-server-example.jsThe repo's LICENSE aggregates multiple third‑party licenses and many files use "The Chromium Authors" headers; this file currently has a Chromium/BSD header. Action: if this file is NOT a Chromium-derived file, replace the header with the project's canonical license (MIT) or remove the third‑party attribution; if it WAS copied from Chromium, keep the header and ensure the top-level LICENSE/NOTICE includes the required Chromium attribution.
Location: eval-server/nodejs/examples/openai-server-example.js (lines 3–6).eval-server/python/examples/openai_server_example.py (1)
1-1
: Make file executable or remove shebang (Ruff EXE001)Shebang present in eval-server/python/examples/openai_server_example.py but the file lacks the executable bit; either run
git update-index --chmod=+x eval-server/python/examples/openai_server_example.py
or remove the first line. Script produced no output — request manual verification.eval-server/python/src/bo_eval_server.egg-info/PKG-INFO (1)
7-9
: Project URLs point to Chromium DevToolsThese URLs don’t match this repo and can mislead users.
Please update the packaging metadata source (pyproject.toml/setup.cfg), not PKG-INFO directly, to point to BrowserOperator/browser-operator-core.
ARG AUTOMATED_MODE=false | ||
|
||
# Set build-time flags based on Docker arg | ||
RUN if [ "$AUTOMATED_MODE" = "true" ]; then \ | ||
sed -i 's/AUTOMATED_MODE: false/AUTOMATED_MODE: true/' \ | ||
front_end/panels/ai_chat/core/BuildConfig.ts; \ | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Doc mismatch: AUTOMATED_MODE default is false but README claims automated is default.
Either set the ARG default to true here or update README commands to pass --build-arg AUTOMATED_MODE=true
.
-ARG AUTOMATED_MODE=false
+ARG AUTOMATED_MODE=true
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
ARG AUTOMATED_MODE=false | |
# Set build-time flags based on Docker arg | |
RUN if [ "$AUTOMATED_MODE" = "true" ]; then \ | |
sed -i 's/AUTOMATED_MODE: false/AUTOMATED_MODE: true/' \ | |
front_end/panels/ai_chat/core/BuildConfig.ts; \ | |
fi | |
ARG AUTOMATED_MODE=true | |
# Set build-time flags based on Docker arg | |
RUN if [ "$AUTOMATED_MODE" = "true" ]; then \ | |
sed -i 's/AUTOMATED_MODE: false/AUTOMATED_MODE: true/' \ | |
front_end/panels/ai_chat/core/BuildConfig.ts; \ | |
fi |
🤖 Prompt for AI Agents
In docker/Dockerfile around lines 51 to 57, the Docker ARG AUTOMATED_MODE
defaults to false while the README states automated mode is the default; to
resolve this either change the ARG default to true (ARG AUTOMATED_MODE=true) so
the build-time flag and sed step reflect the README, or leave ARG as false and
update README examples to pass --build-arg AUTOMATED_MODE=true; pick one
approach and make the corresponding change consistently across Dockerfile and
README.
# Build with automated mode (default - bypasses OAuth, auto-enables evaluation) | ||
docker build -f docker/Dockerfile -t browser-operator-automated . | ||
|
||
# Build with normal mode (requires manual authentication) | ||
docker build -f docker/Dockerfile --build-arg AUTOMATED_MODE=false -t browser-operator-manual . | ||
|
||
# Or use docker-compose (recommended) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated mode “default” conflicts with Dockerfile.
Either pass --build-arg AUTOMATED_MODE=true
here or change Dockerfile ARG default to true.
-# Build with automated mode (default - bypasses OAuth, auto-enables evaluation)
-docker build -f docker/Dockerfile -t browser-operator-automated .
+# Build with automated mode
+docker build -f docker/Dockerfile --build-arg AUTOMATED_MODE=true -t browser-operator-automated .
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
# Build with automated mode (default - bypasses OAuth, auto-enables evaluation) | |
docker build -f docker/Dockerfile -t browser-operator-automated . | |
# Build with normal mode (requires manual authentication) | |
docker build -f docker/Dockerfile --build-arg AUTOMATED_MODE=false -t browser-operator-manual . | |
# Or use docker-compose (recommended) | |
# Build with automated mode | |
docker build -f docker/Dockerfile --build-arg AUTOMATED_MODE=true -t browser-operator-automated . | |
# Build with normal mode (requires manual authentication) | |
docker build -f docker/Dockerfile --build-arg AUTOMATED_MODE=false -t browser-operator-manual . | |
# Or use docker-compose (recommended) |
🤖 Prompt for AI Agents
In docker/README.md around lines 25 to 31, the README's claim that automated
mode is the default conflicts with the Dockerfile ARG default; update the README
and/or Dockerfile for consistency by either (a) changing the README build
command to pass --build-arg AUTOMATED_MODE=true for the automated build line, or
(b) change the Dockerfile ARG AUTOMATED_MODE default to true so the existing
README command is accurate; pick one approach and update the file(s) accordingly
so the documentation and Dockerfile agree.
constructor(evalServer, options = {}) { | ||
this.evalServer = evalServer; | ||
this.config = { | ||
port: options.port || 8081, | ||
host: options.host || 'localhost', | ||
modelCacheTTL: options.modelCacheTTL || 300000, // 5 minutes in ms | ||
...options | ||
}; | ||
|
||
this.server = null; | ||
this.isRunning = false; | ||
|
||
// Model list cache | ||
this.modelCache = null; | ||
this.modelCacheTime = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add auth and safety limits to config.
Expose apiKey
, maxBodySize
, and requestTimeoutMs
via options/env to harden the API.
constructor(evalServer, options = {}) {
this.evalServer = evalServer;
this.config = {
port: options.port || 8081,
host: options.host || 'localhost',
modelCacheTTL: options.modelCacheTTL || 300000, // 5 minutes in ms
+ apiKey: options.apiKey ?? process.env.OPENAI_API_KEY ?? null,
+ maxBodySize: options.maxBodySize ?? 2 * 1024 * 1024, // 2 MB
+ requestTimeoutMs: options.requestTimeoutMs ?? 300_000, // 5 minutes
...options
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
constructor(evalServer, options = {}) { | |
this.evalServer = evalServer; | |
this.config = { | |
port: options.port || 8081, | |
host: options.host || 'localhost', | |
modelCacheTTL: options.modelCacheTTL || 300000, // 5 minutes in ms | |
...options | |
}; | |
this.server = null; | |
this.isRunning = false; | |
// Model list cache | |
this.modelCache = null; | |
this.modelCacheTime = null; | |
} | |
constructor(evalServer, options = {}) { | |
this.evalServer = evalServer; | |
this.config = { | |
port: options.port || 8081, | |
host: options.host || 'localhost', | |
modelCacheTTL: options.modelCacheTTL || 300000, // 5 minutes in ms | |
apiKey: options.apiKey ?? process.env.OPENAI_API_KEY ?? null, | |
maxBodySize: options.maxBodySize ?? 2 * 1024 * 1024, // 2 MB | |
requestTimeoutMs: options.requestTimeoutMs ?? 300_000, // 5 minutes | |
...options | |
}; | |
this.server = null; | |
this.isRunning = false; | |
// Model list cache | |
this.modelCache = null; | |
this.modelCacheTime = null; | |
} |
🧰 Tools
🪛 ESLint
[error] 36-36: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
[error] 39-39: Trailing spaces not allowed.
(@stylistic/no-trailing-spaces)
🤖 Prompt for AI Agents
In eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js around lines 28 to 43,
add auth and safety limit config entries by reading options and environment
variables: include apiKey (options.apiKey || process.env.EVAL_SERVER_API_KEY ||
null), maxBodySize (parseInt(options.maxBodySize ||
process.env.EVAL_SERVER_MAX_BODY_SIZE || '1048576') for a default 1MB), and
requestTimeoutMs (parseInt(options.requestTimeoutMs ||
process.env.EVAL_SERVER_REQUEST_TIMEOUT_MS || '30000') for a default 30s) into
the this.config object; ensure numeric values are parsed and fallback defaults
are set, keep the spread ...options ordering so explicit options can override
env/defaults, and add basic validation (throw or coerce if non-numeric) so the
server uses these values when creating the HTTP server and handling requests.
this.server.listen(this.config.port, this.config.host, () => { | ||
logger.info(`OpenAI-compatible API started on http://${this.config.host}:${this.config.port}`); | ||
}); | ||
|
||
this.isRunning = true; | ||
return this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Apply server-level timeouts.
Avoid hung connections by setting requestTimeout
/headersTimeout
.
this.server.listen(this.config.port, this.config.host, () => {
logger.info(`OpenAI-compatible API started on http://${this.config.host}:${this.config.port}`);
});
this.isRunning = true;
+ // Hard timeouts
+ this.server.requestTimeout = this.config.requestTimeoutMs;
+ this.server.headersTimeout = this.config.requestTimeoutMs + 5000;
return this;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
this.server.listen(this.config.port, this.config.host, () => { | |
logger.info(`OpenAI-compatible API started on http://${this.config.host}:${this.config.port}`); | |
}); | |
this.isRunning = true; | |
return this; | |
} | |
this.server.listen(this.config.port, this.config.host, () => { | |
logger.info(`OpenAI-compatible API started on http://${this.config.host}:${this.config.port}`); | |
}); | |
this.isRunning = true; | |
// Hard timeouts | |
this.server.requestTimeout = this.config.requestTimeoutMs; | |
this.server.headersTimeout = this.config.requestTimeoutMs + 5000; | |
return this; | |
} |
🤖 Prompt for AI Agents
In eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js around lines 74 to 80,
the server is started without setting server-level timeouts; before calling
this.server.listen set this.server.headersTimeout and this.server.requestTimeout
from configuration (e.g., use this.config.headersTimeoutMs and
this.config.requestTimeoutMs with sensible defaults like 40_000 and 120_000) so
hung connections are closed; ensure these properties are assigned on this.server
prior to listen and add the new config fields to the wrapper's config
initialization or defaults.
async handleRequest(req, res) { | ||
const url = await import('url'); | ||
const parsedUrl = url.parse(req.url, true); | ||
const pathname = parsedUrl.pathname; | ||
const method = req.method; | ||
|
||
try { | ||
// Get body for POST requests | ||
let body = ''; | ||
if (method === 'POST') { | ||
for await (const chunk of req) { | ||
body += chunk; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enforce Bearer auth (optional but recommended) and body size limits.
If apiKey
is configured, require Authorization: Bearer <key>
. Also cap POST body to prevent DoS.
async handleRequest(req, res) {
const url = await import('url');
const parsedUrl = url.parse(req.url, true);
const pathname = parsedUrl.pathname;
const method = req.method;
try {
+ // Optional Bearer auth
+ if (this.config.apiKey) {
+ const auth = req.headers['authorization'] || '';
+ const token = auth.startsWith('Bearer ') ? auth.slice(7) : null;
+ if (!token || token !== this.config.apiKey) {
+ this.sendError(res, 401, 'Unauthorized: invalid or missing API key');
+ return;
+ }
+ }
+
// Get body for POST requests
let body = '';
if (method === 'POST') {
- for await (const chunk of req) {
- body += chunk;
- }
+ let size = 0;
+ for await (const chunk of req) {
+ size += chunk.length;
+ if (size > this.config.maxBodySize) {
+ this.sendError(res, 413, 'Payload too large');
+ return;
+ }
+ body += chunk;
+ }
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
async handleRequest(req, res) { | |
const url = await import('url'); | |
const parsedUrl = url.parse(req.url, true); | |
const pathname = parsedUrl.pathname; | |
const method = req.method; | |
try { | |
// Get body for POST requests | |
let body = ''; | |
if (method === 'POST') { | |
for await (const chunk of req) { | |
body += chunk; | |
} | |
} | |
async handleRequest(req, res) { | |
const url = await import('url'); | |
const parsedUrl = url.parse(req.url, true); | |
const pathname = parsedUrl.pathname; | |
const method = req.method; | |
try { | |
// Optional Bearer auth | |
if (this.config.apiKey) { | |
const auth = req.headers['authorization'] || ''; | |
const token = auth.startsWith('Bearer ') ? auth.slice(7) : null; | |
if (!token || token !== this.config.apiKey) { | |
this.sendError(res, 401, 'Unauthorized: invalid or missing API key'); | |
return; | |
} | |
} | |
// Get body for POST requests | |
let body = ''; | |
if (method === 'POST') { | |
let size = 0; | |
for await (const chunk of req) { | |
size += chunk.length; | |
if (size > this.config.maxBodySize) { | |
this.sendError(res, 413, 'Payload too large'); | |
return; | |
} | |
body += chunk; | |
} | |
} |
🤖 Prompt for AI Agents
In eval-server/nodejs/src/lib/OpenAICompatibleWrapper.js around lines 102-116,
before reading the POST body enforce optional Bearer auth and a POST body size
cap: if an apiKey is configured, validate req.headers.authorization has the form
"Bearer <key>" and return 401 if missing/invalid; then, when reading the POST
body, stop accumulating once a safe size limit (e.g. 1MB) is exceeded and
immediately return a 413 Payload Too Large response; perform the auth check
before reading the body and ensure you close the request/return after sending
the error responses.
README.md | ||
pyproject.toml | ||
src/bo_eval_server/__init__.py | ||
src/bo_eval_server/client_manager.py | ||
src/bo_eval_server/config.py | ||
src/bo_eval_server/eval_server.py | ||
src/bo_eval_server/evaluation_stack.py | ||
src/bo_eval_server/logger.py | ||
src/bo_eval_server/openai_server.py | ||
src/bo_eval_server/rpc_client.py | ||
src/bo_eval_server.egg-info/PKG-INFO | ||
src/bo_eval_server.egg-info/SOURCES.txt | ||
src/bo_eval_server.egg-info/dependency_links.txt | ||
src/bo_eval_server.egg-info/entry_points.txt | ||
src/bo_eval_server.egg-info/requires.txt | ||
src/bo_eval_server.egg-info/top_level.txt | ||
tests/test_openai_compatibility.py No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove egg-info manifest from VCS.
This manifest is regenerated each build; committing it causes churn and mismatches (e.g., if tests move).
🤖 Prompt for AI Agents
In eval-server/python/src/bo_eval_server.egg-info/SOURCES.txt lines 1-17: the
auto-generated egg-info manifest is committed and should be removed; delete the
entire src/bo_eval_server.egg-info directory from the repository (git rm -r
--cached if you want to preserve local build artifacts), commit that removal,
and add an appropriate ignore rule (e.g. add /src/bo_eval_server.egg-info/ or
*.egg-info/ to .gitignore) so the manifest isn’t re-committed on future builds.
@@ -0,0 +1 @@ | |||
bo_eval_server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove egg-info from VCS.
Same as above—this file should be generated during build, not committed.
🤖 Prompt for AI Agents
In eval-server/python/src/bo_eval_server.egg-info/top_level.txt lines 1-1, the
generated egg-info file is committed to VCS; delete the file and stop tracking
the whole bo_eval_server.egg-info directory, then add a gitignore rule (e.g.,
*.egg-info/ or specifically bo_eval_server.egg-info/) to prevent future commits;
perform git rm --cached -r eval-server/python/bo_eval_server.egg-info (or git rm
--cached eval-server/python/bo_eval_server.egg-info/top_level.txt if you
prefer), commit the removal and updated .gitignore, and ensure the build/process
that generates egg-info still produces it as part of packaging.
async def start(self): | ||
"""Start the HTTP server""" | ||
if not self.eval_server.is_running(): | ||
raise RuntimeError("EvalServer must be started before starting OpenAI-compatible server") | ||
|
||
logger.info(f"Starting OpenAI-compatible server on {self.host}:{self.port}") | ||
|
||
config = uvicorn.Config( | ||
self.app, | ||
host=self.host, | ||
port=self.port, | ||
log_level="info", | ||
access_log=True, | ||
) | ||
|
||
self.server = uvicorn.Server(config) | ||
await self.server.serve() | ||
|
||
async def stop(self): | ||
"""Stop the HTTP server""" | ||
if hasattr(self, 'server'): | ||
logger.info("Stopping OpenAI-compatible server") | ||
await self.server.shutdown() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optional: add Bearer auth support (env-configurable)
To better match OpenAI clients and prevent accidental exposure when bound beyond localhost, gate endpoints behind an API key when provided.
@@
-import asyncio
+import asyncio
+import os
@@
- def _setup_routes(self):
+ def _require_auth(self, request: Request):
+ api_key = os.getenv("OPENAI_API_KEY")
+ if not api_key:
+ return # auth disabled
+ auth = request.headers.get("authorization", "")
+ if not auth.lower().startswith("bearer ") or auth.split(" ", 1)[1].strip() != api_key:
+ raise HTTPException(status_code=401, detail="Unauthorized")
+
+ def _setup_routes(self):
@@
- @self.app.get("/v1/models", response_model=OpenAIModelsResponse)
- async def list_models():
+ @self.app.get("/v1/models", response_model=OpenAIModelsResponse)
+ async def list_models(request: Request):
+ self._require_auth(request)
@@
- @self.app.post("/v1/chat/completions", response_model=OpenAIChatCompletionResponse)
- async def chat_completions(request: OpenAIChatCompletionRequest):
+ @self.app.post("/v1/chat/completions", response_model=OpenAIChatCompletionResponse)
+ async def chat_completions(request: OpenAIChatCompletionRequest, http_request: Request):
+ self._require_auth(http_request)
@@
- @self.app.post("/v1/responses")
- async def responses_api(request: Dict[str, Any]):
+ @self.app.post("/v1/responses")
+ async def responses_api(request: Dict[str, Any], http_request: Request):
+ self._require_auth(http_request)
@@
- @self.app.get("/health")
- async def health_check():
+ @self.app.get("/health")
+ async def health_check(request: Request):
+ self._require_auth(request)
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
597-597: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In eval-server/python/src/bo_eval_server/openai_server.py around lines 594 to
616, add optional Bearer token authentication configurable via an environment
variable (e.g., OPENAI_API_KEY); when the env var is set, register middleware or
a dependency on the FastAPI app before creating the uvicorn server that verifies
incoming requests carry an Authorization: Bearer <key> header matching the
configured value and returns 401 on mismatch, and log whether auth is enabled
during startup; ensure the check applies to all OpenAI-compatible endpoints but
allows local testing when the env var is unset.
- **`GET /v1/models`** - List available models from connected DevTools tabs | ||
- **`POST /v1/chat/completions`** - OpenAI-compatible chat completions | ||
- **`GET /health`** - Health check and status | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Document all supported endpoints (missing /v1/responses).
Code implements POST /v1/responses
but README omits it. Add it to the list.
- **`GET /v1/models`** - List available models from connected DevTools tabs
- **`POST /v1/chat/completions`** - OpenAI-compatible chat completions
+- **`POST /v1/responses`** - OpenAI Responses API compatibility
- **`GET /health`** - Health check and status
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
- **`GET /v1/models`** - List available models from connected DevTools tabs | |
- **`POST /v1/chat/completions`** - OpenAI-compatible chat completions | |
- **`GET /health`** - Health check and status | |
- **`GET /v1/models`** - List available models from connected DevTools tabs | |
- **`POST /v1/chat/completions`** - OpenAI-compatible chat completions | |
- **`POST /v1/responses`** - OpenAI Responses API compatibility | |
- **`GET /health`** - Health check and status |
🤖 Prompt for AI Agents
In eval-server/README.md around lines 84 to 87, the endpoints list omits the
POST /v1/responses endpoint implemented in code; update the README to document
this endpoint by adding a new bullet (e.g., **`POST /v1/responses`** - Responses
endpoint compatible with the server's response API) including a brief
description of its purpose and expected behavior so the docs match the
implementation.
showOAuthLogin: (() => { | ||
if (BUILD_CONFIG.AUTOMATED_MODE) { | ||
return false; | ||
} | ||
const hasCredentials = this.#hasAnyProviderCredentials(); | ||
const showOAuth = !hasCredentials; | ||
logger.info('=== OAUTH LOGIN UI DECISION ==='); | ||
logger.info('hasAnyProviderCredentials:', hasCredentials); | ||
logger.info('showOAuthLogin will be set to:', showOAuth); | ||
return showOAuth; | ||
return !hasCredentials; | ||
})(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Show OAuth login only for OpenRouter and only when its credentials are missing.
Prevents misleading OAuth prompt when other providers (OpenAI/Groq/LiteLLM) are configured.
- showOAuthLogin: (() => {
- if (BUILD_CONFIG.AUTOMATED_MODE) {
- return false;
- }
- const hasCredentials = this.#hasAnyProviderCredentials();
- return !hasCredentials;
- })(),
+ showOAuthLogin: (() => {
+ if (BUILD_CONFIG.AUTOMATED_MODE) return false;
+ const selectedProvider = localStorage.getItem(PROVIDER_SELECTION_KEY) || 'openai';
+ if (selectedProvider !== 'openrouter') return false;
+ const { isValid } = LLMClient.validateProviderCredentials('openrouter');
+ return !isValid;
+ })(),
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
showOAuthLogin: (() => { | |
if (BUILD_CONFIG.AUTOMATED_MODE) { | |
return false; | |
} | |
const hasCredentials = this.#hasAnyProviderCredentials(); | |
const showOAuth = !hasCredentials; | |
logger.info('=== OAUTH LOGIN UI DECISION ==='); | |
logger.info('hasAnyProviderCredentials:', hasCredentials); | |
logger.info('showOAuthLogin will be set to:', showOAuth); | |
return showOAuth; | |
return !hasCredentials; | |
})(), | |
showOAuthLogin: (() => { | |
if (BUILD_CONFIG.AUTOMATED_MODE) return false; | |
const selectedProvider = localStorage.getItem(PROVIDER_SELECTION_KEY) || 'openai'; | |
if (selectedProvider !== 'openrouter') return false; | |
const { isValid } = LLMClient.validateProviderCredentials('openrouter'); | |
return !isValid; | |
})(), |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/AIChatPanel.ts around lines 1993 to 1999, the
current showOAuthLogin logic shows the OAuth prompt whenever no provider
credentials exist; change it to only show the OAuth prompt when the selected
provider is OpenRouter and OpenRouter credentials are missing. Update the IIFE
to first check the selected provider (e.g., this.settings.provider ===
'openrouter' or equivalent selector) and then return true only if that is the
case AND a new or existing helper that checks OpenRouter credentials (e.g.,
this.#hasOpenRouterCredentials()) returns false; otherwise return false. Ensure
you add/rename the helper method to specifically test OpenRouter credentials if
it doesn't exist and keep AUTOMATED_MODE behavior unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
# Create config | ||
config = Config() | ||
|
||
# Create and start evaluation server | ||
eval_server = EvalServer(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Initialize EvalServer with config object instead of auth key
The standalone OpenAI server creates config = Config()
but then calls EvalServer(config)
. EvalServer.__init__
expects an auth_key
string (and creates its own Config
internally). Passing the Config instance gets stored as the auth_key
, so the WebSocket server ends up using a Config
object for authentication and ignores the actual configuration values. Clients sending the expected secret will never authenticate, so the example server cannot be used. Instantiate EvalServer
with a proper auth key (or pass Config
fields explicitly) before starting.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
New Features
Documentation
Chores