refactor: modular server architecture with service extraction and int… - #25
Merged
Merged
Conversation
…egration tests Phase 1-4: Split server.ts into modular architecture - server.ts: 4800 to 199 lines (-96%) — only CORS + route wiring + Vite/static - 6 new service modules: venv/, olive/ (recipe, cuda, tensorrt, tensorrt-rtx), ai/, shared/ - 7 route modules: ai, env, mcp, olive, system, github, tensorrt - 496 unit tests + 40 integration tests (all pass) - CI-ready vitest config with mocked deps (Python, AI, LM Studio, Ollama, spawn) - pnpm test:integration script + Python 3.10-3.13 doc updates
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @tonythethompson, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR modularizes the Express server, adds AI provider and runtime services, introduces typed API and frontend components, adds integration and component testing infrastructure, and updates Python requirements, MCP guidance, repository tooling, and pass-catalog synchronization. ChangesOlive Studio application
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (5) 🔗 Fix PR: #26 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 5 fixed
|
…ixes - Restore GET /api/health (required by Tauri desktop bootstrap) - Parallelize Ollama /api/tags + /api/ps with Promise.all() - Fix JSX && with string values in AuditPanel and LocalModelManager
Two tests: status/uptime shape validation, and uptime monotonicity check
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 80
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.cursor/skills/studio/SKILL.md:
- Line 52: Update the fenced code blocks in .cursor/skills/studio/SKILL.md:52-52
by declaring the Task(...) example as javascript or typescript, and in
.cursor/skills/studio/SKILL.md:147-149 by declaring the installation command as
bash.
- Around line 147-149: Update the cssstudio installation command in the setup
instructions to invoke an explicit reviewed package version instead of the
unpinned cssstudio package. Preserve the existing install behavior while
ensuring future upstream releases cannot change the MCP setup silently.
In `@scripts/sync-pass-catalog.mjs`:
- Around line 93-99: Update loadExisting so filesystem or JSON parse errors are
surfaced instead of returning an empty object. Ensure the sync flow aborts
before writing when loading OUT_FILE fails, leaving the existing catalog
untouched and preserving valid successful-load behavior.
- Around line 102-119: Update mergePasses to separate the catalog envelope
metadata (version and last_updated) from the pass map before iterating, then
merge only actual pass entries so the result remains a valid PassesJson shape
and _passCount is accurate. Preserve existing manual _manual and notes values,
and also retain the intended description and category overrides when merging
existing entries.
- Around line 80-87: Update the live pass extraction flow around the raw
FALLBACK handling and JSON.parse catch so registry, subprocess, and parsing
failures propagate an error instead of returning null. Adjust main() to
terminate with a nonzero exit status when extraction fails, while preserving
successful catalog updates.
In `@server.ts`:
- Around line 7-8: Merge the separate imports from
"./src/server/routes/system.ts" into a single module import in server.ts,
preserving both mountSystemRoutes and the type-only SystemProbeOptions usage so
CI validation passes.
- Around line 194-199: Update the direct-execution block around startServer so
its rejected promise is explicitly observed and startup failures are reported
with a clear error before terminating with a nonzero exit status. Preserve the
existing isMain guard and normal successful startup behavior.
- Around line 178-190: Update the SIGINT and SIGTERM handlers in server.ts to
invoke the existing Olive job cleanup logic, such as cleanupAllJobs or an
equivalent shutdown helper, before process.exit(0). Ensure active Python/Olive
children tracked by jobRegistry are terminated for both signals, while
preserving the current shutdown logging and exit behavior.
- Around line 71-73: Secure the AI route mounting around aiRouter and
mountAiRoutes by requiring the existing session/auth middleware for provider
changes, ensuring unauthenticated clients cannot update process-global AI
provider state. Configure the server listener to bind to loopback by default,
while preserving an explicit opt-in for intentional remote access.
In `@src/components/features/AuditPanel.tsx`:
- Around line 112-118: Update the icon class names in the status rendering
within AuditPanel to replace the unsupported text-rose-450 and text-emerald-450
shades with existing Tailwind color shades, such as 500. Preserve the current
warning, success, and fallback icon behavior.
In `@src/components/features/LocalModelManager.test.tsx`:
- Around line 17-56: Add tests in the LocalModelManager test suite that use
mockFetch with lms.error and ollama.error, covering failed responses from both
backend endpoints and asserting the component’s expected error behavior. Keep
the existing success-path tests unchanged and exercise each backend’s failure
path independently.
- Around line 72-79: Update the “shows refreshing text while loading” test to
await a waitFor-wrapped assertion for “Refreshing…”, matching the asynchronous
state assertion pattern used by the other tests in this file while preserving
the never-resolving fetch mock.
In `@src/lib/apiClient.ts`:
- Around line 211-220: Update the request setup in the API client method
containing this fetch call to construct a Headers instance from rest.headers,
then add the JSON Content-Type without discarding valid HeadersInit forms such
as Headers objects or tuple arrays. Change the body condition to serialize
whenever body !== undefined, preserving false, 0, and null payloads while still
omitting an undefined body.
- Around line 250-265: Update the streaming reader loop around reader.read and
onLine to flush the decoder after the loop ends, append any remaining decoded
UTF-8 bytes to buffer, and process the leftover buffer as a final line. Preserve
the existing JSON parsing and skip-unparseable-lines behavior for this final
buffered content.
In `@src/server/__tests__/routes.integration.test.ts`:
- Around line 275-302: Replace the non-discriminating assertions in the affected
integration tests, including “rejects toolName with command injection
characters,” “attempts to call a valid tool and returns JSON,” the line-63
source assertion, and the tests around lines 441–464. Use the deterministic
responses configured by setup.integration.ts to assert the exact expected
status, content type, and payload; remove any SSE/JSON or success/error
alternatives that allow arbitrary responses.
- Around line 469-499: Update the “streams setup log lines and ends with a done
event” test to assert the final parsed done payload’s ok value is true, rather
than only checking that the ok property exists. Keep the existing NDJSON parsing
and done-type assertions unchanged.
In `@src/server/config.ts`:
- Around line 81-83: Update the config accessors, including systemPython, to
reuse an in-memory parsed result instead of invoking readDiskConfig on every
property access. Add cache invalidation to writeDiskConfig after persisting
changes, ensuring subsequent reads reload the latest disk configuration while
preserving existing config values and behavior.
- Around line 78-139: Annotate the exported appConfig singleton with an explicit
AppConfig-based type that also declares its readDisk, writeDisk, resetRuntime,
and snapshot helpers. Ensure the existing getters, setters, and helper
implementations satisfy that contract so TypeScript catches future shape or type
drift.
- Around line 103-111: Update the appConfig hfToken getter and setter to
delegate to the runtime token singleton in services/olive/state.ts, using its
existing getRuntimeHfToken and setRuntimeHfToken symbols instead of the local
_hfToken store. Preserve the current string-or-null validation while ensuring
reads, writes, and resetRuntime() observe the shared runtime token.
- Around line 23-26: Remove the duplicate StudioConfig interface from the config
module and import the existing StudioConfig declaration from the server types
module. Update references to use that imported type while preserving the
optional systemPython property and its current behavior.
In `@src/server/routes/ai.ts`:
- Around line 293-297: Update the GET /ai/provider handler to include the source
field in the populated response as well as the null response. Use the source
value from cfg so clients can distinguish environment-detected providers from
runtime overrides, while preserving the existing provider and model fields.
- Around line 729-735: Update the error handler around the LMS server startup
retry loop to use a bare catch without an unused binding, and emit a client log
via send({ type: "log", ... }) indicating that the server failed to start.
Preserve the existing retry behavior while ensuring the failure is communicated
to the client.
- Around line 424-444: The /ai/local-models handler currently makes identical
requests for installed and loaded models, so loadedModels is not real readiness
data. Update the second fetch in the Promise.allSettled call to use LM Studio’s
actual loaded-models endpoint, while preserving the existing response parsing
and fallback behavior.
- Around line 188-253: The ensureOllamaReady flow currently performs potentially
ten-minute installation and readiness work synchronously within request
handlers. Promote this work to a tracked background job with observable status
for the affected Ollama endpoints, and enforce an overall deadline covering
installation, CLI discovery, and server readiness polling; ensure jobs can
terminate or report failure when the deadline is reached or the client
disconnects.
- Around line 336-361: Sanitize the caller-provided base URL in the /ai/models
handler before resolving the API key or calling fetchLiveModelCatalog. Reuse
sanitizeProviderBaseUrl, as the /ai/provider flow does, and pass only its
validated result (or the established safe default) onward so untrusted URLs
cannot receive server-side credentials; ensure any trailing-slash normalization
occurs after sanitization.
- Line 366: Clean up the route handler by removing or using the unused
destructured bindings workspaceContext and state, along with imports
readEnvApiKey, buildCodexPrompt, and devinChat. Before the chat history is
concatenated and passed to callAI, validate every client-supplied entry against
AIChatMessage so role and content have the expected shape.
- Around line 85-122: Move cachedLmsCli out of findLmsCli and declare it at
module scope so successful CLI discovery persists across calls; retain the
existing lookup and return behavior. Also update findOllamaCli to use an
equivalent module-level cache, ensuring repeated health checks and pulls avoid
rerunning blocking execSync PATH probes.
- Around line 506-547: Protect the /ai/local-pull handler by applying the
existing installEngineRateLimiter before spawning the LM Studio process, and add
a req close handler after spawn that terminates the child process when the
client disconnects. Ensure disconnect cleanup prevents further output handling
and avoids ending the response twice, while preserving normal close, error, and
successful completion behavior.
- Around line 299-312: Update the POST /ai/provider handler around
setRuntimeAiProvider to validate apiKey and model before sanitizing or writing
runtime state: reject values that are not strings or are empty, returning a 400
response with a clear validation error. Preserve the existing provider
allow-listing, baseUrl sanitization, and successful runtime update behavior for
valid inputs.
- Around line 906-911: Replace the runtime require of Express in
registerAiRoutes with a top-level ESM import for Router, then instantiate the
router through that imported symbol. Keep mountAiRoutes and app.use behavior
unchanged.
In `@src/server/routes/env.ts`:
- Around line 99-118: Add a module-level in-progress guard for the POST
/env/venv-install handler so concurrent requests receive HTTP 409 without
calling ensureVenv, mirroring the existing /api/mcp/sync behavior. Set and clear
the guard around the full ensureVenv operation, including failures, and avoid
writing to a disconnected response by checking the response state in the
streaming callbacks and completion paths; log or abort the install on client
disconnect if the existing infrastructure supports it.
- Line 17: Update the import used by the env route to reference the service
module containing ensureTensorRtRtx directly, rather than importing it through
the routes-layer tensorrt.ts re-export shim. Leave the shim available for
external or backward-compatible consumers.
In `@src/server/routes/github.ts`:
- Around line 38-45: Update the upstream fetch in the GitHub route handler to
use an AbortSignal.timeout with an appropriate finite duration, ensuring hung
raw.githubusercontent.com requests are aborted. Also bound the response body
read before JSON.parse by enforcing a maximum text size, rejecting oversized
content before parsing while preserving normal responses.
In `@src/server/routes/mcp.ts`:
- Around line 67-80: Update readPassesJson to distinguish a missing passes.json
from permission, I/O, and JSON-parse failures: return null only for the
filesystem not-found condition, and propagate all other errors so the route can
produce HTTP 500 instead of reporting the catalog as unavailable. Preserve the
existing successful JSON parsing behavior.
- Around line 119-138: Update the /mcp/sync-kb handler to restore the
SYNC_KB_TOKEN or same-origin authorization gate before changing sync state.
Invoke olive-mcp-server/scripts/update_kb.py, verify its successful report, then
reread passes.json, call reloadPassSchemas, and invalidate the KB status cache;
only return ok with a new lastSync after the update succeeds.
- Around line 40-48: Update the execFileAsync invocation in the MCP tool-call
flow so argsJson is passed as data through sys.argv or stdin rather than
interpolated into the Python source. Use a fixed Python snippet that imports
json and parses the payload with json.loads(), while preserving safeName
handling and existing tool-call behavior for all JSON values, including true,
false, and null.
In `@src/server/routes/olive.ts`:
- Around line 25-29: Add rate limiting middleware to the POST "/olive/run"
route, using the existing rate-limit implementation in
src/server/middleware/rateLimit.ts. In the route handler, also reject requests
while an Olive run is already active with a 409 response, matching the
concurrent-sync behavior used by the MCP sync endpoint; ensure the active state
is cleared when the run completes or fails.
- Around line 43-56: Update the Olive job lifecycle across the job creation
flow, gpu.ts pushLog/SSE replay, and process close handler: cap job.logs by
dropping oldest entries past a fixed limit, indicate truncated history during
SSE replay, unlink recipe-${jobId}.json from .olive-runs when the process
closes, and schedule finished jobs for TTL-based removal from jobRegistry.
Preserve active-job tracking and existing log delivery behavior.
- Around line 61-83: Move the Olive setup and execution flow currently beginning
at ensureVenv inside the request handler into a background task after
registering the job. Return res.json({ ok: true, jobId }) immediately so clients
can subscribe to /olive/stream/:jobId, and preserve progress and failure
reporting through pushLog and the existing job status updates.
- Around line 176-185: Update the job close-handler logic near the process
lifecycle code to preserve an intentional cancellation, so a SIGTERM-triggered
exit cannot overwrite job.status with "failed"; track cancellation intent on the
job and have the close handler retain "cancelled". In the "/olive/cancel" route,
stop GPU metrics via stopGpuMetricsTimer(job) when cancelling, including when no
close event arrives; add SIGKILL escalation only if consistent with the existing
process-management design.
- Around line 132-158: Update the /olive/stream/:jobId handler to terminate SSE
connections when the job reaches a terminal state: emit a final data event
containing done status and relevant completion/error details, then call
res.end() from the job’s close/error handling. Handle already-completed jobs
without registering a subscriber, and add periodic SSE comment heartbeats for
active idle connections, clearing the timer on request close or stream
termination.
- Around line 79-89: Harden the `/api/olive/run` execution boundary around
`resolveOliveCommand` and `spawn`: require the project’s trusted-caller
authentication before accepting requests, sanitize or allowlist executable
recipe fields beyond structural validation, and launch Olive with
least-privileged sandbox constraints while preserving the existing job setup
flow.
In `@src/server/routes/system.ts`:
- Around line 115-128: The documentation above SystemProbeOptions incorrectly
claims the injected TensorRT probes depend on server.ts helpers and avoid a
circular dependency. Update the comment to describe their actual purpose as a
test seam, or remove SystemProbeOptions and import probeTensorRtLoadable and
probeTensorRtRtxLoadable directly from their service modules; keep the route
behavior unchanged.
- Around line 148-177: Update the system capability probing flow around the
existing probe function and request handler to memoize the in-flight probe
promise, so concurrent requests share one probe operation and refresh requests
cannot trigger unbounded duplicate work. Inside the pythonCandidates loop, start
probePythonRuntime, probeTensorRtLoadable, and probeTensorRtRtxLoadable
concurrently with Promise.all, while preserving the existing result-selection
and notes behavior. Ensure the shared promise is cleared or replaced after
completion so the normal cache expiry behavior remains intact.
In `@src/server/services/ai/anthropic.ts`:
- Around line 31-32: Update the response handling in the Anthropic request
method to select the first content block with a text value rather than assuming
content[0] is text. If no text block exists, throw an explicit error instead of
returning an empty string, while preserving the existing text response path.
In `@src/server/services/ai/devin.ts`:
- Around line 11-18: In the Devin chat request, simplify the role mapping in the
messages transformation to use the existing user/assistant role value without
the unreachable system fallback. Hoist the repeated “swe-1-6” model literal into
a shared constant and reuse it in the default model and buildConfig-related
logic, including the cfg.model fallback.
In `@src/server/services/ai/gemini.ts`:
- Line 16: Update the Gemini request URL construction to remove cfg.apiKey from
the query string, percent-encode cfg.model as the path segment, and pass the API
key through the request’s x-goog-api-key header instead.
In `@src/server/services/ai/index.ts`:
- Around line 12-18: Add the missing provider side-effect imports in
src/server/services/ai/index.ts at lines 12-18 for mistral, xai, openrouter,
groq, together, kilocode, copilot, openai-compat, and chatgpt-sub so the
registry matches the advertised catalog. In
src/server/services/ai/registry.test.ts lines 86-92, if any referenced plugin
modules are unavailable, reduce EXPECTED_PROVIDERS and the minimum-count
assertions to the providers actually shipped.
In `@src/server/services/ai/openai.ts`:
- Around line 59-80: The provider base URL mapping is duplicated instead of
using each registered plugin’s defaultBaseUrl. In
src/server/services/ai/openai.ts lines 59-80, update resolveOpenAiCompatBase to
obtain the registered provider’s defaultBaseUrl and retain the OpenAI URL as
fallback while preserving explicit cfg.baseUrl handling; in
src/server/routes/ai.ts lines 876-893, remove defaultBaseUrl() and resolve the
URL through the provider registry instead.
- Around line 44-48: Introduce a shared fetchWithTimeout helper using
AbortSignal.timeout, then route every listed provider request through it:
openai.ts lines 44-48 in callOpenAICompat, openai.ts lines 119-133 for Copilot,
gemini.ts lines 25-29 for generateContent, and anthropic.ts lines 13-26 for the
messages request. Ensure each call uses the common timeout signal and preserves
existing request behavior.
- Line 281: Remove GITHUB_TOKEN from the envVarNames auto-detection list for the
Copilot provider, leaving GITHUB_COPILOT_TOKEN and COPILOT_GITHUB_TOKEN as
detection variables. Preserve GITHUB_TOKEN as a supported manual/runtime
override through the existing provider configuration path.
- Around line 292-303: Update the openai-compat provider registration and its
buildConfig/resolveOpenAiCompatBase flow so an explicit base URL is required,
rather than falling back to api.openai.com. Validate the configured base URL and
fail loudly when it is absent, while preserving the existing OpenAI-compatible
request behavior when provided.
- Around line 82-92: Update supportsJsonResponseFormat to delegate to the
registry’s providerSupportsJsonResponse(cfg) helper instead of maintaining its
own provider-name list. Remove the duplicated provider switch while preserving
the registry’s existing supportsJsonResponseFormat flags as the single source of
truth.
In `@src/server/services/ai/registry.test.ts`:
- Around line 290-301: Update the provider registry flow used by
registerProvider and the ALLOWED_AI_PROVIDERS snapshot so tests do not leave
test-dispatch in module-global state or create divergent provider views. Prefer
adding an internal unregister/reset mechanism and invoke it in the callProvider
dispatch suite teardown, or otherwise make ALLOWED_AI_PROVIDERS derive the
current registry lazily while preserving existing provider behavior.
In `@src/server/services/ai/registry.ts`:
- Around line 80-88: Update the provider plugin definition to include an
explicit priority value, then modify detectEnvProvider to evaluate providers in
descending priority order before checking environment keys. Ensure ties use a
deterministic secondary ordering so detection precedence does not depend on
module import or registration order.
In `@src/server/services/ai/security.ts`:
- Around line 67-70: The provider base-URL validation currently allows providers
missing from ALLOWED_BASE_URL_PREFIX_BY_PROVIDER to use any public HTTPS host.
Make the allow-list total for every ProviderConfig provider, use an explicit
wildcard sentinel only for openai-compat, and represent providers that reject
caller-supplied baseUrl values with an empty list; update the validation logic
to distinguish these cases and fail closed for future providers.
In `@src/server/services/olive/cuda.ts`:
- Around line 26-36: Update pickCudaTag to return an explicit unsupported result
instead of "cu118" when the CUDA version is below the supported tiers, including
CUDA 11.7 and older. Update detectCudaTag to recognize that result, select CPU
or emit a clear compatibility error, and preserve existing tag selection for
supported versions.
In `@src/server/services/olive/gpu.ts`:
- Around line 32-87: Introduce a shared short-TTL bounded cache around
sampleGpuMetrics so concurrent job timers reuse one system-wide nvidia-smi
result instead of spawning duplicate subprocesses. Update sampleGpuMetrics to
return the cached metrics while fresh and refresh the cache after expiry,
preserving its existing null-on-error behavior and per-job push flow in
startGpuMetricsTimer.
- Around line 8-30: Collapse the duplicated subscriber notification loops in
pushLog and pushGpuMetrics into one generic helper that accepts the value and
subscriber collection, while keeping each function’s existing job-field mutation
and swallowed subscriber errors unchanged.
- Around line 69-87: Update startGpuMetricsTimer so the initial running-status
check occurs before scheduling any sampling or interval. Ensure non-running jobs
return without calling sampleGpuMetrics or assigning job.metricsTimer, while
preserving the existing sampling guard and timer behavior for running jobs.
In `@src/server/services/olive/recipe.ts`:
- Around line 37-45: Update getRecipeIhvProvider to accept only recognized
IHVProvider values before returning them, and retain CUDAExecutionProvider as
the fallback for missing or invalid execution providers. Export this helper and
replace the duplicated accelerator lookup in the olive route with
getRecipeIhvProvider(recipe), ensuring dependency inference and execution use
the same provider resolution.
In `@src/server/services/olive/state.ts`:
- Around line 3-4: Update the Olive job lifecycle around the jobRegistry to
evict completed, failed, and cancelled jobs after a bounded TTL or via a bounded
LRU, while preserving active jobs. Also cap retained log lines for each job so
logs, subscriber lists, and ChildProcess references cannot grow without bound.
In `@src/server/services/olive/tensorrt-rtx.ts`:
- Around line 93-102: Add an error listener to the pip child process created in
the TensorRT RTX installation flow, alongside the existing close handler, and
reject the surrounding Promise with the spawn error so failures such as missing
pip settle the request and preserve the `{ ok: false }` response path.
In `@src/server/services/olive/tensorrt.ts`:
- Around line 174-175: Move the probeTensorRtRtxLoadable and PkgDef imports from
their current mid-file location to the module’s top-level import section in
tensorrt.ts. Preserve their existing import paths and type-only import
semantics, and do not retain inline imports since no circular-dependency
justification is documented.
- Around line 121-128: Update the TensorRT flow around probeTensorRtLoadable and
ensureTensorRt so the native GPU library paths resolved during probing are
returned with the probe result and reused to build libsDir in both success
paths. Remove the separate getNativeGpuLibPaths calls after probing, ensuring
each ensureTensorRt call resolves the paths only once.
- Around line 191-209: Extend PkgDef with an explicit variant field and set
torch and onnxruntime recipe entries to "gpu" or "cpu" based on isGpu. Update
the torch check around the importName === "torch" branch to use this variant
instead of installArgs.includes("cpu"), and update the onnxruntime branch to
compare against the selected CPU/GPU variant rather than always
pinnedOrtGpuInstallArgs(). Add a unit test confirming an already-installed CPU
torch is not reinstalled.
- Around line 143-154: Both pip-install promises in
src/server/services/olive/tensorrt.ts#L143-L154 and
src/server/services/olive/tensorrt.ts#L272-L281 must reject when the spawned
process emits an error instead of hanging; add proc error handlers at both sites
that reject with contextual pip-install startup errors, or reuse the existing
helper from src/server/services/venv/index.ts#L92-L139.
- Around line 93-101: Update the TensorRT probe around execFileAsync to enforce
a finite timeout so a hanging import or native library load cannot block the
hardware-probe request indefinitely. Replace the broad out.includes("ok")
success check with an exact comparison of the trimmed final output line to "ok",
while preserving the existing failure-detail fallback.
In `@src/server/services/venv/config.ts`:
- Line 10: Consolidate subprocess execution on the shared execFileAsync helper
and configure it with sane timeout and maxBuffer defaults. In
src/server/services/venv/config.ts:10-10, remove the local promisified
definition and re-export or import the shared helper; update venv/index.ts to
import it directly from ../shared/exec.ts. In
src/server/services/venv/gpu.ts:5-5 and src/server/routes/system.ts:22-22,
remove the local child_process/util promisify definitions and imports, then
import execFileAsync from ../shared/exec.ts.
- Around line 64-77: Update the venv PATH setup function around the profile
selection and existing-entry check: choose the shell-appropriate startup file
from $SHELL, falling back to ~/.profile, and return the selected profile path so
the UI can identify the file changed and advise opening a new shell. Replace
existing.includes(resolved) with an exact match against an active export entry,
excluding comments and unrelated parent-path mentions, while preserving the
current success/error result behavior.
- Around line 36-62: Update the Windows PowerShell flow around the `ps` script
and `execFileAsync` so `resolved` is passed as data, such as through an
environment variable or command argument, instead of interpolated with
`JSON.stringify`. Read that value inside PowerShell and preserve the existing
comparison, prepend, and result behavior without emitting doubled backslashes or
allowing path quotes to alter the script.
- Around line 37-71: Safely handle resolved in the PATH-update flow: reject any
CR/LF before constructing exportLine or the PowerShell command, shell-quote
resolved for the ~/.profile export, and pass it to PowerShell as data rather
than interpolating it into ps. Update the affected logic in the surrounding venv
PATH function while preserving duplicate detection and existing success/error
results.
In `@src/server/services/venv/gpu.ts`:
- Around line 39-46: Update the PATH parsing in the GPU virtual-environment
discovery function to split on Node’s path.delimiter instead of the
process.platform ternary. Import or reuse the path module as appropriate, and
apply the same delimiter change in the corresponding parsing logic in the venv
service index module.
- Around line 28-38: Guard the site-package discovery around the visible `site =
Path(__import__("site").getsitepackages()[0])` logic so `AttributeError`,
`IndexError`, or other lookup failures leave previously discovered `dirs` intact
and still reach the final print. Also update the surrounding `execFileAsync`
invocation in the request path to include an appropriate timeout, without
discarding the existing discovered directories on timeout or discovery failure.
In `@src/server/services/venv/index.ts`:
- Around line 144-185: Optimize getRuntimeEnvStatus and findSystemPython by
caching the resolved system interpreter, invalidating that cache when the
systemPython configuration changes, and reusing it across callers such as
probeSystemHardware. Within getRuntimeEnvStatus, run the Olive probe and
user-PATH retrieval concurrently with Promise.all(), while preserving existing
fallback behavior and platform-specific PATH handling.
- Around line 32-35: Update isSupportedOlivePython to enforce the documented
Python version range by rejecting versions above the defined maximum, and ensure
the Windows candidate list does not probe unsupported Python314. Centralize the
boundary using the existing or newly defined PYTHON_MAX symbol, preserving
support for Python 3.10–3.13 and keeping getRuntimeEnvStatus consistent with the
user-facing messages.
- Around line 92-139: Refactor the repeated spawn-and-stream promise blocks in
the virtual-environment setup flow into one helper reused by venv creation and
both package installs, preserving each command and failure label. The helper
must settle on both process close and spawn error, including the latter for
missing executables. Add the same timeout option used by other execFileAsync
calls to the import olive and import requests probes so ensureVenv cannot hang.
In `@src/server/types.ts`:
- Line 116: Update the process property in the relevant server type to use a
top-level child_process type import instead of the inline
import("child_process") expression, while preserving the ChildProcess | null
type.
In `@vitest.integration.config.ts`:
- Around line 27-35: Both coverage configurations include test files and
integration setup files in measured source, inflating coverage results. Add the
same coverage exclude patterns, "**/*.test.ts" and "src/server/__tests__/**", to
the coverage blocks in vitest.integration.config.ts (lines 27-35) and
vitest.server.config.ts (lines 18-28).
In `@vitest.server.config.ts`:
- Around line 14-17: Update the test include pattern in the Vitest server
configuration to exclude integration test files, ensuring only the unit-test
suite runs without integration mocks. Preserve integration coverage under
vitest.integration.config.ts, where its setupFiles provide the required boundary
mocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b918f87-03f3-4147-b18e-a7b957fa859b
📒 Files selected for processing (61)
.cursor/skills/studio/SKILL.md.gitignoreABOUT.mdCONTRIBUTING.mdORIGINAL_REQUEST.mdREADME.mdpackage.jsonscripts/sync-pass-catalog.mjsserver.tssrc-tauri/src/lib.rssrc/components/features/AuditPanel.test.tsxsrc/components/features/AuditPanel.tsxsrc/components/features/GeminiSidebar.tsxsrc/components/features/LocalModelManager.test.tsxsrc/components/features/LocalModelManager.tsxsrc/components/features/ProviderErrorBlock.test.tsxsrc/components/features/ProviderErrorBlock.tsxsrc/components/features/RuntimeEnvControls.tsxsrc/lib/apiClient.tssrc/server/__tests__/routes.integration.test.tssrc/server/__tests__/setup.integration.tssrc/server/config.tssrc/server/middleware/cors.tssrc/server/middleware/rateLimit.tssrc/server/routes/ai.tssrc/server/routes/env.tssrc/server/routes/github.tssrc/server/routes/mcp.tssrc/server/routes/olive.tssrc/server/routes/system.tssrc/server/routes/tensorrt.tssrc/server/services/ai/anthropic.tssrc/server/services/ai/codex.tssrc/server/services/ai/detect.test.tssrc/server/services/ai/detect.tssrc/server/services/ai/devin.tssrc/server/services/ai/gemini.tssrc/server/services/ai/index.tssrc/server/services/ai/openai.tssrc/server/services/ai/registry.test.tssrc/server/services/ai/registry.tssrc/server/services/ai/security.test.tssrc/server/services/ai/security.tssrc/server/services/ai/state.tssrc/server/services/mcp/state.tssrc/server/services/olive/cuda.tssrc/server/services/olive/gpu.tssrc/server/services/olive/recipe.tssrc/server/services/olive/state.tssrc/server/services/olive/tensorrt-rtx.tssrc/server/services/olive/tensorrt.tssrc/server/services/shared/exec.tssrc/server/services/venv/config.tssrc/server/services/venv/gpu.tssrc/server/services/venv/index.tssrc/server/services/venv/paths.test.tssrc/server/services/venv/paths.tssrc/server/types.tsvitest.component.config.tsvitest.integration.config.tsvitest.server.config.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Code review by qodo was updated up to the latest commit e969a61 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.ts`:
- Around line 95-98: Update the /api/health handler and startup flow to restore
the shared readiness state: initialize the flag as not ready, set it only after
successful startup in the listen callback, and return HTTP 503 until then while
preserving the existing healthy response afterward. Update the direct-app
integration harness to explicitly initialize the same readiness state before
exercising the app.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c35cf5c-1ee5-4f68-ba1e-e7d1ac18c891
📒 Files selected for processing (5)
server.tssrc/components/features/AuditPanel.tsxsrc/components/features/LocalModelManager.tsxsrc/server/__tests__/routes.integration.test.tssrc/server/routes/ai.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Evaluate cheap synchronous conditions before awaiting flags or remote values.
Defer await operations until the branch that needs their results.
Use dependency-based parallelization, such asbetter-all, for partially dependent operations.
UsePromise.all()for independent asynchronous operations.
Avoid barrel-file imports; prefer statically optimized package imports or direct source imports.
Load large modules or data conditionally only when the feature is activated.
Prefer literal paths or explicit maps for dynamic imports and filesystem access.
Chain dependent nested fetches within each item’s promise so ready items are not blocked by slow items.
UseReact.cache()for per-request deduplication of authentication, database, filesystem, and other non-fetch async work.
Use{ passive: true }for touch and wheel listeners that do not callpreventDefault().
Version and minimizelocalStoragedata, wrap storage access intry/catch, and avoid storing sensitive data.
Batch DOM writes separately from layout reads to avoid forced synchronous layout.
BuildMapindexes for repeated keyed lookups instead of repeatedly calling.find().
Cache repeated property accesses and loop lengths in hot loops.
Cache repeated function results when identical inputs recur during rendering or hot paths.
Cache synchronous storage and cookie reads in memory, and invalidate caches on external changes.
Combine independent array filters or maps into one iteration when this is a hot-path optimization.
Schedule non-critical browser work withrequestIdleCallback, with a fallback where necessary.
Check array lengths before expensive equality, sorting, serialization, or deep comparison.
Return early once a function’s result is determined.
UseflatMapfor one-pass transformation and filtering where appropriate.
Use a linear loop rather than sorting an entire collection to find minimum or maximum values.
UseSetorMapfor repeated membership and keyed lookups...
Files:
src/components/features/AuditPanel.tsxsrc/components/features/LocalModelManager.tsxserver.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/ai.ts
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{jsx,tsx}: Use strategic ReactSuspenseboundaries so wrapper UI is not blocked by data fetching.
Defer non-critical third-party libraries until after hydration.
Usenext/dynamicfor heavy components that are not required on the initial render.
Preload heavy bundles based on user intent such as hover, focus, or enabled feature flags.
Avoid passing duplicate transformed references across React Server/Client boundaries.
Do not use mutable module-level state for request- or user-specific data during SSR or RSC rendering.
Minimize fields passed across RSC boundaries; send only data the client uses.
Use component composition to parallelize independent Server Component data fetching.
Deduplicate global event listeners across component instances.
Use SWR for client data-fetching deduplication, caching, and revalidation.
Calculate derived state during rendering instead of storing it in state or synchronizing it with effects.
Defer dynamic state reads to the callback or usage point when no subscription is needed.
Do not useuseMemofor simple expressions returning primitive values.
Do not define React components inside other components; extract them and pass props.
Extract non-primitive default parameter values of memoized components into stable constants.
Extract expensive work into memoized components so loading or early-return paths skip it.
Use the narrowest primitive effect dependencies possible.
Put interaction-triggered side effects in event handlers rather than state-plus-effect sequences.
Split hook computations and effects with independent dependencies into separate hooks.
Subscribe to derived state, such as a media-query boolean, instead of rapidly changing raw values.
Use functionalsetStateupdates whenever the new value depends on previous state.
Use lazyuseState(() => initialValue)initialization for expensive initial computations.
Use transitions for frequent non-urgent state updates.
UseuseDeferredValuefor expensive derived renders driv...
Files:
src/components/features/AuditPanel.tsxsrc/components/features/LocalModelManager.tsx
**/*.{jsx,tsx,html}
📄 CodeRabbit inference engine (AGENTS.md)
Use
deferorasyncfor raw scripts, or the appropriatenext/scriptstrategy in Next.js.
Files:
src/components/features/AuditPanel.tsxsrc/components/features/LocalModelManager.tsx
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Hoist reusable regular expressions or memoize dynamically constructed expressions; beware mutable global regex state.
Files:
src/components/features/AuditPanel.tsxsrc/components/features/LocalModelManager.tsxserver.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/ai.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Match existing naming, file layout, and TypeScript patterns in
src/.
Files:
src/components/features/AuditPanel.tsxsrc/components/features/LocalModelManager.tsxsrc/server/__tests__/routes.integration.test.tssrc/server/routes/ai.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Place imports at the top of modules; use inline imports only when required for a documented circular dependency.
Files:
src/components/features/AuditPanel.tsxsrc/components/features/LocalModelManager.tsxserver.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/ai.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Authenticate and authorize every Next.js Server Action inside the action itself, and validate its input.
Use a bounded cross-request LRU cache when data should be reused across requests.
Files:
server.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/ai.ts
server.ts
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep Olive spawning, dependency installation, and PATH logic in
server.ts.
Files:
server.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: tonythethompson/Olive-Studio
Timestamp: 2026-07-29T17:46:50.766Z
Learning: Run `pnpm lint` and `pnpm validate:recipe` before opening a pull request.
Learnt from: CR
Repo: tonythethompson/Olive-Studio
Timestamp: 2026-07-29T17:46:50.766Z
Learning: For UI or server changes, manually smoke-test development startup, recipe validation banners, and live execution when applicable.
Learnt from: CR
Repo: tonythethompson/Olive-Studio
Timestamp: 2026-07-29T17:46:50.766Z
Learning: Use Conventional Commits prefixes such as `feat:`, `fix:`, `docs:`, `refactor:`, `test:`, and `chore:`.
Learnt from: CR
Repo: tonythethompson/Olive-Studio
Timestamp: 2026-07-29T17:46:50.766Z
Learning: Keep changes focused and prefer small, reviewable pull requests over large refactors.
🔍 Remote MCP DeepWiki, GitHub Copilot
Relevant review context
- KB sync contract: The earlier implementation required
SYNC_KB_TOKENandx-sync-tokenon every/api/mcp/sync-kbrequest, rejected missing configuration with503, invalid tokens with401, disallowed origins with403, and concurrent syncs with409. Verify the modularmcproute preserves these protections. - KB success semantics:
update_kb.pywritesupdate_report.json, records per-source statuses, setssuccess, and exits nonzero when any source reports"error". The route should reload schemas only after a successful report. - Catalog consistency: The related MCP PR introduced a local catalog advertised as 40 passes, while the current review context reports 84 documented passes. The new sync/merge logic should not silently regress catalog contents or metadata.
- Health/readiness: The prior server exposed
/api/healthas503until startup completed and performed job cleanup on termination. The refactor’s simplified signal handlers and direct-start guard should be checked against those lifecycle expectations. - Repository-specific DeepWiki lookup could not be performed:
tonythethompson/Olive-Studiowas not indexed/found by DeepWiki.
🔇 Additional comments (4)
src/server/routes/ai.ts (1)
553-556: LGTM!src/components/features/AuditPanel.tsx (1)
83-83: LGTM!src/components/features/LocalModelManager.tsx (1)
322-322: LGTM!src/server/__tests__/routes.integration.test.ts (1)
321-349: LGTM!
Add rate limits for auth, heavy commands, Olive runs, GitHub proxy, and static serving. Sanitize AI model catalog base URLs and strip trailing slashes without ReDoS-prone regex. Harden GitHub raw proxy host checks, Python path validation before exec/fs use, and PowerShell PATH quoting. Detect CPU torch via index URL, enforce Python 3.10–3.13 upper bound, unify HF token via appConfig, tighten pass-catalog sync, and exclude integration tests from the server vitest config.
Validate interpreters against allowlisted roots (startsWith containment), probe absolute paths via a fixed node helper so execFile never takes a user-supplied executable, and call PATH python commands as literals.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.ts`:
- Around line 160-162: Update the middleware setup around static serving so
staticServeRateLimit is mounted once before both express.static and the SPA
fallback handler; remove it from the individual app.use calls while preserving
their existing request handling order.
In `@src/server/routes/ai.ts`:
- Around line 875-877: Update the model-catalog fetch in the surrounding route
handler to use a finite timeout via an AbortController signal, and catch
timeout/abort failures so the handler falls back to the existing response.
Preserve the current successful fetch and non-timeout error behavior.
In `@src/server/routes/env.ts`:
- Around line 30-33: Update the path validation around pythonPath so
path.isAbsolute checks the trimmed input before path.resolve is called; only
resolve the value after confirming it is absolute, while preserving the existing
error response for relative paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8217a7f4-e75b-42fa-94c8-112abe978eba
📒 Files selected for processing (15)
scripts/sync-pass-catalog.mjsserver.tssrc/server/middleware/rateLimit.tssrc/server/routes/ai.tssrc/server/routes/env.tssrc/server/routes/github.tssrc/server/routes/olive.tssrc/server/services/ai/openai.tssrc/server/services/ai/security.test.tssrc/server/services/ai/security.tssrc/server/services/olive/state.tssrc/server/services/olive/tensorrt.tssrc/server/services/venv/config.tssrc/server/services/venv/index.tsvitest.server.config.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Evaluate cheap synchronous conditions before awaiting flags or remote values.
Defer await operations until the branch that needs their results.
Use dependency-based parallelization, such asbetter-all, for partially dependent operations.
UsePromise.all()for independent asynchronous operations.
Avoid barrel-file imports; prefer statically optimized package imports or direct source imports.
Load large modules or data conditionally only when the feature is activated.
Prefer literal paths or explicit maps for dynamic imports and filesystem access.
Chain dependent nested fetches within each item’s promise so ready items are not blocked by slow items.
UseReact.cache()for per-request deduplication of authentication, database, filesystem, and other non-fetch async work.
Use{ passive: true }for touch and wheel listeners that do not callpreventDefault().
Version and minimizelocalStoragedata, wrap storage access intry/catch, and avoid storing sensitive data.
Batch DOM writes separately from layout reads to avoid forced synchronous layout.
BuildMapindexes for repeated keyed lookups instead of repeatedly calling.find().
Cache repeated property accesses and loop lengths in hot loops.
Cache repeated function results when identical inputs recur during rendering or hot paths.
Cache synchronous storage and cookie reads in memory, and invalidate caches on external changes.
Combine independent array filters or maps into one iteration when this is a hot-path optimization.
Schedule non-critical browser work withrequestIdleCallback, with a fallback where necessary.
Check array lengths before expensive equality, sorting, serialization, or deep comparison.
Return early once a function’s result is determined.
UseflatMapfor one-pass transformation and filtering where appropriate.
Use a linear loop rather than sorting an entire collection to find minimum or maximum values.
UseSetorMapfor repeated membership and keyed lookups...
Files:
src/server/services/ai/security.test.tsvitest.server.config.tssrc/server/services/ai/security.tssrc/server/services/olive/state.tssrc/server/middleware/rateLimit.tssrc/server/routes/github.tssrc/server/services/venv/config.tssrc/server/routes/olive.tssrc/server/routes/env.tssrc/server/services/ai/openai.tsserver.tssrc/server/services/olive/tensorrt.tssrc/server/routes/ai.tssrc/server/services/venv/index.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Authenticate and authorize every Next.js Server Action inside the action itself, and validate its input.
Use a bounded cross-request LRU cache when data should be reused across requests.
Files:
src/server/services/ai/security.test.tsvitest.server.config.tssrc/server/services/ai/security.tssrc/server/services/olive/state.tssrc/server/middleware/rateLimit.tssrc/server/routes/github.tssrc/server/services/venv/config.tssrc/server/routes/olive.tssrc/server/routes/env.tssrc/server/services/ai/openai.tsserver.tssrc/server/services/olive/tensorrt.tssrc/server/routes/ai.tssrc/server/services/venv/index.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Hoist reusable regular expressions or memoize dynamically constructed expressions; beware mutable global regex state.
Files:
src/server/services/ai/security.test.tsvitest.server.config.tssrc/server/services/ai/security.tssrc/server/services/olive/state.tssrc/server/middleware/rateLimit.tssrc/server/routes/github.tssrc/server/services/venv/config.tssrc/server/routes/olive.tssrc/server/routes/env.tssrc/server/services/ai/openai.tsserver.tssrc/server/services/olive/tensorrt.tssrc/server/routes/ai.tssrc/server/services/venv/index.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Match existing naming, file layout, and TypeScript patterns in
src/.
Files:
src/server/services/ai/security.test.tssrc/server/services/ai/security.tssrc/server/services/olive/state.tssrc/server/middleware/rateLimit.tssrc/server/routes/github.tssrc/server/services/venv/config.tssrc/server/routes/olive.tssrc/server/routes/env.tssrc/server/services/ai/openai.tssrc/server/services/olive/tensorrt.tssrc/server/routes/ai.tssrc/server/services/venv/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Place imports at the top of modules; use inline imports only for a documented circular dependency.
Files:
src/server/services/ai/security.test.tsvitest.server.config.tssrc/server/services/ai/security.tssrc/server/services/olive/state.tssrc/server/middleware/rateLimit.tssrc/server/routes/github.tssrc/server/services/venv/config.tssrc/server/routes/olive.tssrc/server/routes/env.tssrc/server/services/ai/openai.tsserver.tssrc/server/services/olive/tensorrt.tssrc/server/routes/ai.tssrc/server/services/venv/index.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Runpnpm lintandpnpm validate:recipebefore opening a pull request.
For UI or server changes, manually smoke-test development startup, recipe validation banners, and live execution when execution code is touched.
Use Conventional Commits prefixes such asfeat:,fix:,docs:,refactor:,test:, andchore:.
Files:
src/server/services/ai/security.test.tsvitest.server.config.tssrc/server/services/ai/security.tssrc/server/services/olive/state.tssrc/server/middleware/rateLimit.tsscripts/sync-pass-catalog.mjssrc/server/routes/github.tssrc/server/services/venv/config.tssrc/server/routes/olive.tssrc/server/routes/env.tssrc/server/services/ai/openai.tsserver.tssrc/server/services/olive/tensorrt.tssrc/server/routes/ai.tssrc/server/services/venv/index.ts
{server.ts,scripts/olive_gpu_launcher.py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep Olive spawning, dependency installation, and PATH logic in
server.tsandscripts/olive_gpu_launcher.py.
Files:
server.ts
🪛 GitHub Check: CodeQL
src/server/routes/env.ts
[failure] 36-36: Uncontrolled data used in path expression
This path depends on a user-provided value.
src/server/services/venv/index.ts
[failure] 32-32: Uncontrolled data used in path expression
This path depends on a user-provided value.
[failure] 47-47: Uncontrolled command line
This command line depends on a user-provided value.
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- DeepWiki could not provide repository context because
tonythethompson/Olive-Studiois not indexed. - Related PR
#14established KB sync behavior: token authentication whenSYNC_KB_TOKENis configured, same-origin protection otherwise,409concurrency protection, and schema reload only after successful KB refresh. - The current indexed
server.tsimplements those updated semantics and the client sendsVITE_SYNC_KB_TOKENasx-sync-tokenwhen available. - The current indexed server still reports
/api/healthas503beforeserverReady, sets readiness afterlisten, and invokescleanupAllJobs()onSIGINT,SIGTERM, and process exit. This is worth checking against the PR summary’s claim that readiness assignment and shutdown cleanup were removed. - The current indexed
ensureVenverror text still says “Python 3.9+”, despite the PR’s documentation and stated runtime requirement of Python 3.10–3.13.
🔇 Additional comments (16)
scripts/sync-pass-catalog.mjs (1)
102-117: LGTM!Also applies to: 133-135
src/server/middleware/rateLimit.ts (1)
19-70: LGTM!src/server/routes/ai.ts (1)
351-364: LGTM!Also applies to: 517-517, 770-770, 786-786, 842-842
src/server/routes/github.ts (1)
6-60: LGTM!src/server/routes/olive.ts (1)
19-25: LGTM!src/server/services/ai/openai.ts (1)
9-9: LGTM!Also applies to: 60-61, 103-103
src/server/services/ai/security.ts (1)
16-20: LGTM!Also applies to: 73-73
src/server/routes/env.ts (2)
17-17: Import the TensorRT RTX service directly.This remains the previously reported route-layer import shim issue.
131-150: Prevent concurrent venv installation.This remains the previously reported missing in-progress guard: overlapping requests can mutate the same
.venvconcurrently.src/server/services/olive/state.ts (2)
4-5: Bound terminal job retention and log growth.This remains the previously reported unbounded
jobRegistrylifecycle issue.
7-13: LGTM!src/server/services/ai/security.test.ts (1)
2-7: LGTM!Also applies to: 78-96
src/server/services/olive/tensorrt.ts (1)
191-208: LGTM!src/server/services/venv/config.ts (1)
36-59: LGTM!src/server/services/venv/index.ts (1)
19-59: LGTM!vitest.server.config.ts (1)
17-18: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/routes/env.ts (1)
42-52: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDenial of Service (CWE-770): Allocation of Resources Without Limits or Throttling
Reachability: External
● Entry src/server/routes/env.ts:72 getPythonVersion │ ▼ ● Hop src/server/services/venv/index.ts:28 execPythonVersionFromPathCmd: Call sites must pass string literals only (`"python3"` / `"python"`) so CodeQL │ ▼ ● Sink src/server/services/venv/pythonGuard.tsRate limiting was only added to one of several filesystem/process-mutating routes.
fsWriteRateLimitis applied to/env/python-path(Line 65) only./env/install-tensorrt-rtx(Lines 42-52),/env/venv-install(Lines 97-116), and/env/venv-path(Lines 118-126) all trigger real filesystem/process work (installs, venv creation, PATH/registry writes) but remain unthrottled, matching the still-open CodeQL "Missing rate limiting" finding on this file.🔧 Proposed fix
- router.post("/env/install-tensorrt-rtx", async (_req, res) => { + router.post("/env/install-tensorrt-rtx", fsWriteRateLimit, async (_req, res) => { ... - router.post("/env/venv-install", async (_req, res) => { + router.post("/env/venv-install", fsWriteRateLimit, async (_req, res) => { ... - router.post("/env/venv-path", async (_req, res) => { + router.post("/env/venv-path", fsWriteRateLimit, async (_req, res) => {Also applies to: 97-116, 118-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/routes/env.ts` around lines 42 - 52, Apply the existing fsWriteRateLimit middleware to the /env/install-tensorrt-rtx, /env/venv-install, and /env/venv-path route registrations, matching its use on /env/python-path. Keep the route handlers and response behavior unchanged while ensuring all filesystem/process-mutating endpoints are throttled.
♻️ Duplicate comments (1)
src/server/routes/env.ts (1)
97-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
/env/venv-installstill has no concurrency guard or client-disconnect handling.Two overlapping requests both call
ensureVenvagainst the same.venvdirectory concurrently (python -m venv+ pip installs racing each other), and if the client disconnects mid-stream,res.writecalls after that point are wasted work with no abort of the underlying process./api/mcp/sync-kbalready models the right pattern (409 on concurrent sync); this handler should do the same.🔧 Proposed fix
+let venvInstallInProgress = false; + router.post("/env/venv-install", async (_req, res) => { + if (venvInstallInProgress) { + return res.status(409).json({ ok: false, error: "Install already in progress" }); + } + venvInstallInProgress = true; res.setHeader("Content-Type", "application/x-ndjson; charset=utf-8"); ... try { const result = await ensureVenv(onLine); res.write(`${JSON.stringify({ type: "done", ...result })}\n`); res.end(); } catch (err: unknown) { ... + } finally { + venvInstallInProgress = false; } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/routes/env.ts` around lines 97 - 116, Update the /env/venv-install handler around ensureVenv to add a shared concurrency guard that returns HTTP 409 immediately when another installation is active, and release the guard on completion or failure. Track client disconnect/response closure, stop writing streamed log or completion data after disconnect, and propagate cancellation to ensureVenv if its API supports an abort signal; otherwise ensure the underlying installation process is terminated through the existing cancellation mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/probe-python-version.mjs`:
- Around line 50-61: Update the executable validation before the execFileSync
call: require path.isAbsolute(target) before resolving, canonicalize the
candidate with fs.realpathSync(), then apply isUnderRoot and PYTHON_BASENAME_RE
to the canonical path and execute that same canonical path. Preserve the
existing rejection behavior for invalid, missing, or non-file paths.
In `@src/server/services/venv/index.ts`:
- Around line 109-118: Remove the unsupported "314" entry from the Windows
version array in findSystemPython, leaving only Python 3.10 through 3.13
candidates so isRunnablePython cannot resolve Python 3.14 as the system
interpreter.
- Line 20: Remove the Python314 candidate from the Windows interpreter list used
by findSystemPython(), keeping only the supported Python 3.10–3.13 candidates
and leaving the probe script configuration unchanged.
In `@src/server/services/venv/pythonGuard.ts`:
- Around line 67-70: In the Python path validation flow, check whether the
trimmed input is absolute before calling path.resolve(), so relative values such
as “scripts/python” are rejected under the documented contract. Update the
validation around the resolved variable in pythonGuard to preserve the absolute
path for subsequent containment and basename checks.
---
Outside diff comments:
In `@src/server/routes/env.ts`:
- Around line 42-52: Apply the existing fsWriteRateLimit middleware to the
/env/install-tensorrt-rtx, /env/venv-install, and /env/venv-path route
registrations, matching its use on /env/python-path. Keep the route handlers and
response behavior unchanged while ensuring all filesystem/process-mutating
endpoints are throttled.
---
Duplicate comments:
In `@src/server/routes/env.ts`:
- Around line 97-116: Update the /env/venv-install handler around ensureVenv to
add a shared concurrency guard that returns HTTP 409 immediately when another
installation is active, and release the guard on completion or failure. Track
client disconnect/response closure, stop writing streamed log or completion data
after disconnect, and propagate cancellation to ensureVenv if its API supports
an abort signal; otherwise ensure the underlying installation process is
terminated through the existing cancellation mechanism.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 946f1239-6655-41f6-9734-f9a522d43b1c
📒 Files selected for processing (4)
scripts/probe-python-version.mjssrc/server/routes/env.tssrc/server/services/venv/index.tssrc/server/services/venv/pythonGuard.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Runpnpm lintandpnpm validate:recipebefore opening a pull request.
Ensure CI passes, including typecheck and the recipe-builder smoke test.
Use Conventional Commits prefixes such asfeat:,fix:,docs:,refactor:,test:, andchore:.
Files:
scripts/probe-python-version.mjssrc/server/services/venv/pythonGuard.tssrc/server/routes/env.tssrc/server/services/venv/index.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Evaluate cheap synchronous conditions before awaiting flags or remote values.
Defer await operations until the branch that needs their results.
Use dependency-based parallelization, such asbetter-all, for partially dependent operations.
UsePromise.all()for independent asynchronous operations.
Avoid barrel-file imports; prefer statically optimized package imports or direct source imports.
Load large modules or data conditionally only when the feature is activated.
Prefer literal paths or explicit maps for dynamic imports and filesystem access.
Chain dependent nested fetches within each item’s promise so ready items are not blocked by slow items.
UseReact.cache()for per-request deduplication of authentication, database, filesystem, and other non-fetch async work.
Use{ passive: true }for touch and wheel listeners that do not callpreventDefault().
Version and minimizelocalStoragedata, wrap storage access intry/catch, and avoid storing sensitive data.
Batch DOM writes separately from layout reads to avoid forced synchronous layout.
BuildMapindexes for repeated keyed lookups instead of repeatedly calling.find().
Cache repeated property accesses and loop lengths in hot loops.
Cache repeated function results when identical inputs recur during rendering or hot paths.
Cache synchronous storage and cookie reads in memory, and invalidate caches on external changes.
Combine independent array filters or maps into one iteration when this is a hot-path optimization.
Schedule non-critical browser work withrequestIdleCallback, with a fallback where necessary.
Check array lengths before expensive equality, sorting, serialization, or deep comparison.
Return early once a function’s result is determined.
UseflatMapfor one-pass transformation and filtering where appropriate.
Use a linear loop rather than sorting an entire collection to find minimum or maximum values.
UseSetorMapfor repeated membership and keyed lookups...
Files:
src/server/services/venv/pythonGuard.tssrc/server/routes/env.tssrc/server/services/venv/index.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Authenticate and authorize every Next.js Server Action inside the action itself, and validate its input.
Use a bounded cross-request LRU cache when data should be reused across requests.
Files:
src/server/services/venv/pythonGuard.tssrc/server/routes/env.tssrc/server/services/venv/index.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Hoist reusable regular expressions or memoize dynamically constructed expressions; beware mutable global regex state.
Files:
src/server/services/venv/pythonGuard.tssrc/server/routes/env.tssrc/server/services/venv/index.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/**/*.{ts,tsx}: Match existing naming, file layout, and TypeScript patterns insrc/.
Place imports at the top of modules; do not use inline imports unless required for a documented circular dependency.
Files:
src/server/services/venv/pythonGuard.tssrc/server/routes/env.tssrc/server/services/venv/index.ts
🪛 GitHub Check: CodeQL
src/server/services/venv/pythonGuard.ts
[failure] 86-86: Uncontrolled data used in path expression
This path depends on a user-provided value.
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
-
Server lifecycle invariants: The indexed baseline exposes
/api/healthas503untilserverReadyis set afterapp.listen; it also callscleanupAllJobs()onSIGINT,SIGTERM, andexit. The PR summary claims readiness assignment and shutdown cleanup were removed, so this is a behavior change requiring validation. -
Existing lifecycle tests:
serverLifecycle.test.tsexplicitly tests cancellation ofrunning/setting_upjobs and SSE-disconnect cancellation. Removing shutdown cleanup or changing cancellation semantics may leave these expectations stale or permit active jobs to survive process termination. -
KB sync contract: The baseline
/api/mcp/sync-kbrequiresx-sync-tokenwhenSYNC_KB_TOKENis configured, otherwise enforces same-origin requests; it also returns409for concurrent syncs and reloads pass schemas only after a successful refresh. The client sendsVITE_SYNC_KB_TOKENasx-sync-token. These behaviors should remain intact after route extraction. -
Python-version inconsistency: The indexed baseline still reports “Install Python 3.9+” in
ensureVenv, while the PR changes documentation and the required message to Python 3.10–3.13, with 3.12 recommended. Confirm the runtime error and validation logic were updated consistently, not only the documentation. -
Repository architecture context unavailable: DeepWiki reports that
tonythethompson/Olive-Studiois not indexed, so it could not validate cross-module invariants independently.
🔇 Additional comments (2)
src/server/services/venv/pythonGuard.ts (2)
84-89: CodeQL flag onfs.statSynclooks mitigated by the preceding containment + basename checks.By the time
resolvedreachesfs.statSync, it has already passedisUnderAllowedPythonRoot(prefix-containment sanitizer pattern) and thePYTHON_BASENAME_REcheck, so this should be a false positive assuming CodeQL recognizes the containment check as a sanitizer (per the inline comment at Lines 12-14). No further action needed here beyond what's already flagged.Source: Linters/SAST tools
16-23: 🔒 Security & Privacy | ⚡ Quick winPath Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability
● Entry src/server/routes/env.ts:72 getPythonVersion │ ▼ ● Hop src/server/services/venv/index.ts:28 execPythonVersionFromPathCmd: Call sites must pass string literals only (`"python3"` / `"python"`) so CodeQL │ ▼ ● Sink src/server/services/venv/pythonGuard.tsAllowlist roots are broader than the stated intent.
The comment on
resolveAllowedPythonFile's error says "under an allowed install location (e.g. /usr, ~/.local, Program Files)", butgetAllowedPythonRootsallowlists the entire/homedirectory and the entireos.homedir()(Line 21-22). That means any file anywhere under a user's home directory whose basename matchespython(...)/python.exepasses containment — effectively defeating the purpose of a narrow install-location allowlist (e.g.~/Downloads/python3would pass).Narrow this to specific known install subpaths (
~/.local,~/.pyenv,~/.pyenv/versions/*, etc.) rather than the whole home directory.
Use path.relative + path.join against allowlisted roots before any fs.statSync/exec so CodeQL recognizes the path-injection barrier.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/probe-python-version.mjs (1)
34-65: 🔒 Security & Privacy | 🟠 MajorPath Traversal (CWE-59)
Reachability: External
Symlink escape still possible — rebase fixes the prefix bug but doesn't canonicalize the path.
rebaseOntoRootcorrectly reconstructs the path viapath.relative/path.join(the CodeQL-recognized sanitizer shape), which is a real improvement over a naive prefix check. ButsafePathis never passed throughfs.realpathSync()beforefs.existsSync/fs.statSync/execFileSyncat Lines 59-65. A symlink placed inside an allowed root (e.g..venv/bin/python, typically CWD/user-writable) that points outside the allowlist will pass the root and basename checks, then getstat'd and executed via its unresolved path — Node/the OS follows the symlink at execution time regardless of the string-level check. This is the exact residual concern raised in the linked past review comment (missingfs.realpathSync()before authorization), which remains open here.🔒 Proposed fix: canonicalize before authorizing
const safePath = rebaseOntoRoot(resolved); if (!safePath || !PYTHON_BASENAME_RE.test(path.basename(safePath))) { process.stderr.write("python path not allowed\n"); process.exit(2); } -if (!fs.existsSync(safePath) || !fs.statSync(safePath).isFile()) { +let real; +try { + real = fs.realpathSync(safePath); +} catch { + process.stderr.write("python path not a file\n"); + process.exit(2); +} +if (!rebaseOntoRoot(real) || !fs.statSync(real).isFile()) { process.stderr.write("python path not a file\n"); process.exit(2); } +// execute the canonical path, not the symlink🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/probe-python-version.mjs` around lines 34 - 65, Canonicalize the rebased path with fs.realpathSync before authorization and file validation, then use that canonical path for the allowlisted-root and Python-basename checks, exists/stat checks, and execFileSync call. Update rebaseOntoRoot or the surrounding target-resolution flow so symlinks escaping allowedRoots are rejected, while preserving the existing error exits.
♻️ Duplicate comments (1)
src/server/services/venv/pythonGuard.ts (1)
84-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDead absolute-path check remains unfixed.
path.isAbsolute(path.resolve(pythonPath.trim()))is alwaystrue—path.resolve()guarantees an absolute result, so relative inputs like"scripts/python"silently resolve againstprocess.cwd()instead of being rejected, contrary to the "absolute path required" contract. This is the same issue flagged in the prior review and it's unchanged in this diff.🐛 Proposed fix
- const resolved = path.resolve(pythonPath.trim()); - if (!path.isAbsolute(resolved)) { - return { ok: false, error: "pythonPath must be an absolute path" }; - } + const rawPath = pythonPath.trim(); + if (!path.isAbsolute(rawPath)) { + return { ok: false, error: "pythonPath must be an absolute path" }; + } + const resolved = path.resolve(rawPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/services/venv/pythonGuard.ts` around lines 84 - 87, Update the absolute-path validation in the Python guard to check the trimmed pythonPath before calling path.resolve, rejecting relative values such as "scripts/python" with the existing error. Only resolve the path after validation, and preserve the existing resolved-path behavior for valid absolute inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/services/venv/pythonGuard.ts`:
- Around line 38-61: Update resolveAllowedPythonFile to canonicalize the rebased
safePath with fs.realpathSync before validating or stat’ing it, then run the
allowed-root containment check against that canonical target and return the
canonical path. Preserve the existing null/error handling and ensure downstream
getPythonVersion and execPythonVersionFromFile receive only a realpath proven to
remain within an allowed root.
- Around line 89-115: Canonicalize the interpreter path with fs.realpathSync
before calling rebaseOntoAllowedPythonRoot or applying PYTHON_BASENAME_RE, and
use that canonical path for authorization, stat, and the returned result in the
Python guard flow. Preserve the existing not-found error handling for realpath
failures. Apply the same canonicalization and downstream path usage in
scripts/probe-python-version.mjs.
---
Outside diff comments:
In `@scripts/probe-python-version.mjs`:
- Around line 34-65: Canonicalize the rebased path with fs.realpathSync before
authorization and file validation, then use that canonical path for the
allowlisted-root and Python-basename checks, exists/stat checks, and
execFileSync call. Update rebaseOntoRoot or the surrounding target-resolution
flow so symlinks escaping allowedRoots are rejected, while preserving the
existing error exits.
---
Duplicate comments:
In `@src/server/services/venv/pythonGuard.ts`:
- Around line 84-87: Update the absolute-path validation in the Python guard to
check the trimmed pythonPath before calling path.resolve, rejecting relative
values such as "scripts/python" with the existing error. Only resolve the path
after validation, and preserve the existing resolved-path behavior for valid
absolute inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1a5f6e7-cd62-4f80-adfd-eff4be03aa0c
📒 Files selected for processing (2)
scripts/probe-python-version.mjssrc/server/services/venv/pythonGuard.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Regenerate the catalog only when upstreamolive-recipespaths change, usingpnpm generate:recipes.
Ensure CI passes, including typecheck and the recipe-builder smoke test, before opening a pull request.
Use Conventional Commits prefixes:feat:,fix:,docs:,refactor:,test:, andchore:according to the change type.
Files:
scripts/probe-python-version.mjssrc/server/services/venv/pythonGuard.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Evaluate cheap synchronous conditions before awaiting flags or remote values.
Defer await operations until the branch that needs their results.
Use dependency-based parallelization, such asbetter-all, for partially dependent operations.
UsePromise.all()for independent asynchronous operations.
Avoid barrel-file imports; prefer statically optimized package imports or direct source imports.
Load large modules or data conditionally only when the feature is activated.
Prefer literal paths or explicit maps for dynamic imports and filesystem access.
Chain dependent nested fetches within each item’s promise so ready items are not blocked by slow items.
UseReact.cache()for per-request deduplication of authentication, database, filesystem, and other non-fetch async work.
Use{ passive: true }for touch and wheel listeners that do not callpreventDefault().
Version and minimizelocalStoragedata, wrap storage access intry/catch, and avoid storing sensitive data.
Batch DOM writes separately from layout reads to avoid forced synchronous layout.
BuildMapindexes for repeated keyed lookups instead of repeatedly calling.find().
Cache repeated property accesses and loop lengths in hot loops.
Cache repeated function results when identical inputs recur during rendering or hot paths.
Cache synchronous storage and cookie reads in memory, and invalidate caches on external changes.
Combine independent array filters or maps into one iteration when this is a hot-path optimization.
Schedule non-critical browser work withrequestIdleCallback, with a fallback where necessary.
Check array lengths before expensive equality, sorting, serialization, or deep comparison.
Return early once a function’s result is determined.
UseflatMapfor one-pass transformation and filtering where appropriate.
Use a linear loop rather than sorting an entire collection to find minimum or maximum values.
UseSetorMapfor repeated membership and keyed lookups...
Files:
src/server/services/venv/pythonGuard.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Authenticate and authorize every Next.js Server Action inside the action itself, and validate its input.
Use a bounded cross-request LRU cache when data should be reused across requests.
Files:
src/server/services/venv/pythonGuard.ts
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Hoist reusable regular expressions or memoize dynamically constructed expressions; beware mutable global regex state.
Files:
src/server/services/venv/pythonGuard.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/**/*.{ts,tsx}: Match existing naming, file layout, and TypeScript patterns insrc/.
Put shared recipe logic insrc/lib/, especiallypipelineValidation.ts,oliveRecipeBuilder.ts, andrecipePipeline.ts.
Files:
src/server/services/venv/pythonGuard.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Place imports at the top of modules; use inline imports only for a documented circular dependency.
Files:
src/server/services/venv/pythonGuard.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run lint and recipe validation before submitting changes:
pnpm lintandpnpm validate:recipe.
Files:
src/server/services/venv/pythonGuard.ts
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
-
The baseline
server.tsgatesGET /api/healthbehindserverReady, returning 503 untilapp.listencompletes and then 200. The PR’s removal of the readiness assignment changes this contract. -
The baseline registers shutdown handlers for
SIGINT,SIGTERM, andexit; each invokescleanupAllJobs(), which cancels jobs inrunningorsetting_upstates. Removing these handlers can leave child Olive processes active during shutdown. -
/api/mcp/sync-kbpreviously enforced:x-sync-tokenmatchingSYNC_KB_TOKENwhen configured;- same-origin requests when no token is configured;
- HTTP 409 for concurrent syncs;
- schema reload only after a successful KB refresh.
Route extraction should preserve all four behaviors.
-
The client explicitly sends
VITE_SYNC_KB_TOKENas thex-sync-tokenheader, so renaming or dropping this header in the new route would break authenticated synchronization. -
DeepWiki could not provide repository-specific architecture context because
tonythethompson/Olive-Studiois not indexed.
🔇 Additional comments (1)
scripts/probe-python-version.mjs (1)
1-13: LGTM!
Replace require(\"express\") in registerAiRoutes with an ESM Router import, add spawn error handling for TensorRT RTX pip install, and document that OpenAI-compatible providers register via openai.ts.
Prevent unhandled spawn errors on missing pip, and merge the duplicated system routes import flagged by review/CI lint.
Reject the install promise on ENOENT/launch failure instead of hanging or crashing the process with an unhandled child error event.
Module-level cachedLmsCli so findLmsCli actually caches across requests. Remove unused imports/bindings flagged by review and eslint.
Contributor
This was referenced Jul 30, 2026
tonythethompson
added a commit
that referenced
this pull request
Jul 30, 2026
feat: server hardening follow-ups from PR #25 review
tonythethompson
added a commit
that referenced
this pull request
Jul 30, 2026
* feat: server hardening follow-ups from PR #25 review Addresses deferred review items on top of the modularization PR: - Shared fetchWithTimeout helper; apply to all AI provider calls (openai, copilot, gemini, anthropic) so a hung upstream can't pin a request - openai.ts: resolve base URL + JSON-format support from the registry (single source of truth, drops the duplicated per-provider tables) - anthropic: throw on empty response instead of silently returning "" - olive: job TTL sweeper + temp recipe file cleanup on terminal state; SSE stream heartbeat + termination (no more hanging streams) - venv: concurrent-install lock around ensureVenv; add ~/.zshrc (macOS) and ~/.bashrc PATH targets so the export is actually sourced - cuda: return "cpu" for CUDA < 11.8 instead of forcing an incompatible cu118 - mcp: KB error taxonomy (missing vs unreadable vs invalid) instead of a flat "unavailable" - dedupe execFileAsync onto shared/exec.ts (olive/gpu, venv/gpu, venv/config) Tests: unit 529, server 94 (+15 new), integration 42; lint clean. * fix: address CodeRabbit review on PR #28 - olive: set finishedAt on the venv-setup failure path so the TTL sweeper can reclaim the job; centralize terminal handling via finalizeJob() - olive SSE: notify subscribers immediately on terminal state (done-subscribers) instead of waiting up to one heartbeat interval; heartbeat kept as fallback - openai: throw on empty model output (parity with anthropic/gemini/copilot); require an explicit baseUrl for the generic openai-compat provider - ai: thread ProviderConfig.timeoutMs through fetchWithTimeout for all providers - mcp: runtime-validate parsed passes.json (schema) and return stable client-safe error messages while logging detail server-side - venv: also target bash login profile (~/.bash_profile/.bash_login) so the PATH export is sourced by login shells Tests: unit 532, server 97 (+ finalizeJob cases), integration 42; lint clean. * fix: address Qodo review on PR #28 - venv: ensureVenv now fans setup progress out to every attached onLine listener (Set + broadcast), so concurrent /env/venv-install and /olive/run callers still receive live install output instead of going silent - olive sweeper: never evict a terminal job whose child process has not exited (guards against orphaning a cancelled-but-hung process); null out job.process in close/error handlers so the sweeper can reclaim genuinely-exited jobs Tests: server 99 (+2 sweeper cases), unit 534, integration 42; lint clean. * fix: address follow-up review nits on PR #28 - venv: single safe notification path in ensureVenv (notifyListener) that drops a listener when its callback throws, so a closed SSE stream isn't retried on every subsequent line - olive: in the venv-setup failure branch, push the error log before finalizeJob so it reaches any stream before the done-callbacks drain it - olive: proc "error" handler no longer clears job.process or finalizes; terminal cleanup stays in the "close" handler (fires after error on spawn failure) so a post-spawn/kill error can't drop the handle of a live child - olive state: cleanupJobArtifacts returns success; sweepJobRegistry only evicts a job after cleanup succeeds, retaining it (and its path) for retry on permission/transient FS errors Tests: server 101, unit 536, integration 42; lint + tsc clean. * fix: make Olive cancellation effective during setup (PR #28 review) - /olive/cancel now records + finalizes cancellation even when no child process exists yet (setting_up phase), and no-ops on already-terminal jobs - /olive/run checks the cancelled state after each setup await (ensureVenv, buildOliveRunEnvironment) and aborts before writing the recipe or spawning - add route-level test: cancelling a setting_up job prevents spawn and the run aborts with status "cancelled"; cancel on a finished job returns its status Tests: server 103, unit 538, integration 42; tsc + lint clean. * fix: harden MCP KB validation + Olive recipe write cleanup (PR #28 review) - mcp: /mcp/sync-kb returns HTTP 500 (not 200) on KB read failure so the client sync hook enters its error path; { ok: false } payload preserved - mcp: isValidPassesJson now validates the full pass-entry shape consumed by buildParamSchemas (type/class/description/*_formats/required_params/ optional_params/hardware_requirements/gotchas), rejecting malformed present fields before schema rebuild; absent fields still fall back to defaults - olive: set job.tempRecipePath before writeFileSync so a failed/partial write is still reclaimed by cleanupJobArtifacts (rmSync force:true tolerates a never-created file) - tests: new mcp.test.ts (missing/unreadable/malformed/schema-invalid + sanitized client errors + non-2xx sync + status caching) and an olive write-failure cleanup case Tests: server 111, unit 546, integration 42; tsc + lint clean. * fix: detach venv listener on cancel + cover build-phase cancellation (PR #28 review) - venv: add detachVenvListener() to unregister a job's ensureVenv progress listener; olive route stores the listener on the job and detaches it in /olive/cancel so a cancelled job stops receiving install output (and releases its closure) while shared setup keeps running for other callers - test: add a gated buildOliveRunEnvironment cancellation case — cancel after venv but during env build, assert the run aborts as cancelled, no recipe file is written, and spawn is never called Tests: server 112, unit 547, integration 42; tsc + lint clean. * fix: address PR #32 review on cancel, venv path, and Node engines Escalate Olive cancel from SIGTERM to SIGKILL, avoid creating ~/.bash_profile, dedupe shell exports on the exact line, pin Node to >=22.16, and add coverage for empty AI responses and venv profile targeting. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: unblock pre-push typecheck for cancel test and fetch mock Cast the cancel setTimeout spy through unknown and preserve fetch.preconnect on the integration fetch mock for newer DOM typings. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: preserve cancel status and harden review follow-ups Keep cancelled Olive jobs from flipping to failed on setup errors, restore PATH after venv path tests, and read fetch.preconnect via Reflect.get. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
3 tasks
This was referenced Jul 31, 2026
tonythethompson
added a commit
that referenced
this pull request
Aug 4, 2026
CUDA install UX (#12, #13, #16, #17, #18, #19, #21, #22, #25) - hardwareProbe.mergeDetectedProviders: simplify cudaOk ternary to `input.cudaLoadable !== false` — same semantic, half the surface. - hardwareProbe's pre-Maxwell box reason + recipeHardwareCompatibility's CUDA-floor reason now name Maxwell SM 5.0 as 'GeForce GTX 750 Ti or GTX 9xx series' instead of mistakenly borrowing 'GeForce RTX 20xx+' (RTX 20xx is Turing SM 7.5 — the floor above — which previously implied a Pascal SM 6.x owner had an unsupported card). - recipeHardwareCompatibility widens tensorRtInstallHint to ePInstallHint and routes the onnxruntime-gpu install hint through it so the probe's detail string (driver/wheel mismatch, missing module, etc.) is preserved end-to-end. - IHVIntegrationPanel gates the CUDA toolkit download-link paragraph on `cudaEpInVenv` so it only renders when the EP is actually registered — removes the contradictory 'CUDA EP detected / install button shown' state. - InputEnvironmentPanel renders the requiresInstall hint directly so the label and the pip-install command can never disagree again (the old code branched on `kind` and silently printed the TensorRT label next to the onnxruntime install command). Drop the now-dead pinnedTensorRtLabel + tensorrtRtxEpAbiLabel imports. - system.ts hardens the ORT GPU probe with an explicit 30 s timeout (a broken driver install can leave the onnxruntime import hanging and bind the HTTP request) and derives the pinned-version string from pinnedOrtGpuLabel() instead of hardcoding '1.26.0' — a wheel bump in oliveGpuRuntime now propagates to every error message and install hint without further edits. - system.ts notes generator now derives its suggested pip command from pinnedOrtGpuInstallCommand() instead of a literal 'pip install onnxruntime-gpu==1.26.0' string. Shared install helpers (#23, #24, #26) - src/server/services/shared/pipInstall.ts is the single NDJSON-aware pip install helper used by every install route; the local copies in cuda.ts + tensorrt-rtx.ts deleted (kept the exact byte-shape so the UIs NDJSON parser keeps working unchanged). - cuda.ts exports ensureOnnxRuntimeGpu; tensorrt-rtx.ts calls it directly instead of carrying a private duplicate (mirror drift was the actual bug — they were diverging in subtle ways). - cuda.ts ensureOnnxRuntimeGpu explicitly returns libsDir: null on success so the documented return-type contract is honoured and external callers can distinguish "no library directory available" from "field not set". - tensorrt-rtx.ts uses platform-appropriate native-lib extensions: onnxruntime_providers_nv_tensorrt_rtx.dll on win32, libonnxruntime_providers_nv_tensorrt_rtx.so on Linux, .dylib on Darwin (a hardcoded .dll hides the real reason an import fails on non-Windows platforms). The probe script picks the right extension at runtime. Manifest / dedupe (#5, #20, #6) - cudaDeps.cudaDownloadUrlForOs routes darwin to the archive landing page (Apple dropped CUDA toolkit support after CUDA 11.6) and uses word-boundary regexes so 'darwin' no longer accidentally hits the Windows branch via the substring 'win'. - tensorrtRtxDeps.tensorrtRtxEpAbiInstallCommand now derives the manual pip command from the args list (same delegation pattern pinnedTensorRtInstallCommand uses), so an index/version bump cannot desync the server-side install and the user-facing fallback hint. - src/server/shared/anyDotVenvDir.ts is the single chokidar ANY_DOT_VENV_DIR regex; vite.config.ts and server.ts both import from here so a rename/back-up of the venv directory is filtered out by both Vite watchlists in lockstep. Test fixtures (#14, #15) - cudaDeps.test isPreMaxwellNvidiaBox describe block: split the mislabeled 'every card is at or above the floor' assertion into two focused tests (every-card-above-floor / every-card-below-floor); add a darwin routing suite (asserts darwin does NOT hit the Windows branch via substring 'win' and lands on the archive landing page). - providerCatalog.test cross-family lockstep test title now uses a template literal so the floor number interpolates into the test name (was a verbatim '$floor' string). e2e/scroll-bounds-guardrail.spec.ts (#7, #8, #9) - Page.evaluate wraps Radix Tooltip elements in the full Provider/Root/Trigger/Portal/Content hierarchy using React.createElement so the elements go through the JSX reconciler (plain RdxTooltip.Portal(...) / Content(...) function calls were missing Radix's Provider context and the sentinel never mounted). - DOM-fallback branch preserves the actual import-error string so a future failure is filed with the real reason, not a synthetic 'Radix bare imports did not resolve' placeholder. - Added paint-time hit-test: document.elementFromPoint at the bbox centre must resolve to the sentinel (or one of its ancestors up to #root). getClientRects alone cannot detect overflow:hidden clipping because clip preserves the rect coordinates; the hit-test is the actual guardrail. - Restrict the lint-disable comment to the placeholder line so the prettier/sonar warnings stay clean. Verified: tsc clean, 734/734 unit tests + 229/229 server tests pass, pnpm validate:recipe ok, pnpm lint 9 pre-existing warnings / 0 new errors, live /api/system/hardware-probe?refresh=1 reflects the updated probe fields.
tonythethompson
added a commit
that referenced
this pull request
Aug 4, 2026
* feat: CUDA+TensorRT install UX, lock SM-floor drift, bound scroll overshoot Provider compat & install - CUDA gets the same one-click install UX TRT/TRT-RTX already have: dedicated /api/env/install-onnxruntime-gpu route pip-installs the pinned 1.26.0 wheel into .venv with NDJSON progress, alongside the existing tensorrt / tensorrt- rtx routes (src/server/routes/env.ts, src/server/services/olive/cuda.ts). - IHV panel surfaces the right CTA per state: external link to NVIDIA's CUDA Toolkit archive (system-level install), pip button for onnxruntime-gpu, or rose terminator for pre-Maxwell GPUs that no install can recover. - Probe split into 4 user-facing states: no GPU / pre-Maxwell / driver+wheel missing / driver+toolkit+EP mismatch — the hidden one-line "NVIDIA CUDA was not detected" is gone. Hardware probe + recipe compat - Add cudaToolkit? and cuda? fields to HardwareProbeResult, with cudaLoadable gate on mergeDetectedProviders so recipe compat and IHV panel install button fire in lockstep. - Pre-Maxwell SM 5.0 short-circuit on CUDA recipes, mirroring the existing pre-Turing SM 7.5 short-circuit: never advertise an install on a card that cannot run the EP. - Center the SM floors in cudaDeps.ts and tensorrtRtxDeps so they're the single source of truth and importable by tests. SM-floor drift guards (NEW) - providerCatalog.ts TRT-RTX chip: chip + tooltip.requirements both cite 7.5 numerically, name Turing / RTX 20xx, call out Maxwell/Pascal/Kepler. - providerCatalog.ts full-TensorRT chip: same treatment — previously had no numeric anchor, just "Turing or newer (GeForce RTX 20xx+)". - recipeHardwareCompatibility pre-Turing reason: drift-guard describe block imports the constant and asserts it appears in BOTH TRT and TRT-RTX reasons; locks the "Turing / RTX 20xx+" tie-in phrase and ensures the install hint stays undefined on pre-Turing boxes. - providerCatalog.test.ts: full sibling describe block for the full-TensorRT half of the family, including a cross-family lockstep assertion (both halves must contain the same numeric constant). - Future Bump of TENSORRT_FAMILY_MIN_COMPUTE_CAPABILITY fails CI in 3 test files (hardwareProbe / providerCatalog / recipeHardwareCompatibility) and forces all 4 user-facing surfaces to update atomically. Scroll bound + Vite watcher - App-level, IHV panel, and InputEnvironmentPanel clipped to min-h-0 overflow-hidden under a properly bounded scroll container so the page can't be scrolled past the end into empty space (CSS-only, no JS). - e2e/scroll-bounds-guardrail.spec.ts mounts a Radix portal at #root and asserts it isn't clipped — guards the next person from re-applying the #root overflow lock that would break portals. - vite.config.ts: ignore .venv*, .venv.bak, .venv.old, .venv-renamed so venv tooling (renames, swaps) no longer triggers page reloads. Tests - src/lib/cudaDeps.test.ts (18) — SM 5.0 floor lock + pre-Maxwell classification + pinned install command. - src/lib/tensorrtRtxDeps.test.ts — RTF EP-ABI install command shape. - src/lib/__tests__/providerCatalog.test.ts (16) — SM 7.5 lockstep across catalog chip and hardware-probe reason for BOTH halves of the family. - src/lib/__tests__/recipeHardwareCompatibility.test.ts (23) — full SM-floor drift-guard describe block plus install-needed scenarios for CUDA/TRT/ TRT-RTX on supported and pre-floor hardware. - src/lib/hardwareProbe.test.ts + tensorrtDeps.test.ts — extended for CUDA 4-state branching and TRT/TRT-RTX install-hint gating. Verified - pnpm exec tsc --noEmit: clean - pnpm lint: 9 pre-existing warnings, 0 new, 0 errors - pnpm vitest run --config vitest.config.ts: 731/731 passing - Live preview at http://127.0.0.1:3000/ verified wiring (cudaToolkit field now surfaces on /api/system/hardware-probe). * fix: Use .venv-scoped CUDA readiness * fix: Compute scroll position relative to container * feat: OpenVINO stack install button mirroring TensorRT UX - Add openvino + optimum-intel[openvino] install via NDJSON /api/env/install-openvino into .venv - Probe openvino version, Core().available_devices and optimum.intel availability - Wire requiresInstall/openvinoNeedsInstall badge and install button in IHVIntegrationPanel - Link to Intel GPU/NPU driver docs when discrete devices are absent - Update recipe inference and hardwareProbe/pickRecommendedProvider for OpenVINO - Add unit test for openvino stack install args * refactor(openvino): split probe script, parse and install logic to reduce complexity * fix: 2 findings — Unify shared venv install mutex; Rate-limit OpenVINO i - Unify shared venv install mutex - Rate-limit OpenVINO installation * fix: 2 findings — Preserve partial OpenVINO probe results; Parallelize h - Preserve partial OpenVINO probe results - Parallelize hardware probe calls * fix: address Greptile review feedback - Add optimum.intel import check in recipe inference - Restrict OpenVINO hardware detection to Intel-branded CPUs - Surface optimum.intel import errors in probe detail - Apply heavyCommandRateLimit to all venv install routes * Update src/server/services/olive/openvino.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Update src/lib/cudaDeps.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * fix: apply CodeRabbit auto-fixes Fixed 6 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * fix: install and verify onnxruntime-openvino for OpenVINO EP Codex P1: openvino + optimum-intel alone do not register OpenVINOExecutionProvider. Install onnxruntime-openvino, remove conflicting ORT wheels with a warning, and gate readiness on the EP. P2 (shared venv pip mutex) was already unified on this branch. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: address PR #106 review feedback across 23 outstanding threads CUDA install UX (#12, #13, #16, #17, #18, #19, #21, #22, #25) - hardwareProbe.mergeDetectedProviders: simplify cudaOk ternary to `input.cudaLoadable !== false` — same semantic, half the surface. - hardwareProbe's pre-Maxwell box reason + recipeHardwareCompatibility's CUDA-floor reason now name Maxwell SM 5.0 as 'GeForce GTX 750 Ti or GTX 9xx series' instead of mistakenly borrowing 'GeForce RTX 20xx+' (RTX 20xx is Turing SM 7.5 — the floor above — which previously implied a Pascal SM 6.x owner had an unsupported card). - recipeHardwareCompatibility widens tensorRtInstallHint to ePInstallHint and routes the onnxruntime-gpu install hint through it so the probe's detail string (driver/wheel mismatch, missing module, etc.) is preserved end-to-end. - IHVIntegrationPanel gates the CUDA toolkit download-link paragraph on `cudaEpInVenv` so it only renders when the EP is actually registered — removes the contradictory 'CUDA EP detected / install button shown' state. - InputEnvironmentPanel renders the requiresInstall hint directly so the label and the pip-install command can never disagree again (the old code branched on `kind` and silently printed the TensorRT label next to the onnxruntime install command). Drop the now-dead pinnedTensorRtLabel + tensorrtRtxEpAbiLabel imports. - system.ts hardens the ORT GPU probe with an explicit 30 s timeout (a broken driver install can leave the onnxruntime import hanging and bind the HTTP request) and derives the pinned-version string from pinnedOrtGpuLabel() instead of hardcoding '1.26.0' — a wheel bump in oliveGpuRuntime now propagates to every error message and install hint without further edits. - system.ts notes generator now derives its suggested pip command from pinnedOrtGpuInstallCommand() instead of a literal 'pip install onnxruntime-gpu==1.26.0' string. Shared install helpers (#23, #24, #26) - src/server/services/shared/pipInstall.ts is the single NDJSON-aware pip install helper used by every install route; the local copies in cuda.ts + tensorrt-rtx.ts deleted (kept the exact byte-shape so the UIs NDJSON parser keeps working unchanged). - cuda.ts exports ensureOnnxRuntimeGpu; tensorrt-rtx.ts calls it directly instead of carrying a private duplicate (mirror drift was the actual bug — they were diverging in subtle ways). - cuda.ts ensureOnnxRuntimeGpu explicitly returns libsDir: null on success so the documented return-type contract is honoured and external callers can distinguish "no library directory available" from "field not set". - tensorrt-rtx.ts uses platform-appropriate native-lib extensions: onnxruntime_providers_nv_tensorrt_rtx.dll on win32, libonnxruntime_providers_nv_tensorrt_rtx.so on Linux, .dylib on Darwin (a hardcoded .dll hides the real reason an import fails on non-Windows platforms). The probe script picks the right extension at runtime. Manifest / dedupe (#5, #20, #6) - cudaDeps.cudaDownloadUrlForOs routes darwin to the archive landing page (Apple dropped CUDA toolkit support after CUDA 11.6) and uses word-boundary regexes so 'darwin' no longer accidentally hits the Windows branch via the substring 'win'. - tensorrtRtxDeps.tensorrtRtxEpAbiInstallCommand now derives the manual pip command from the args list (same delegation pattern pinnedTensorRtInstallCommand uses), so an index/version bump cannot desync the server-side install and the user-facing fallback hint. - src/server/shared/anyDotVenvDir.ts is the single chokidar ANY_DOT_VENV_DIR regex; vite.config.ts and server.ts both import from here so a rename/back-up of the venv directory is filtered out by both Vite watchlists in lockstep. Test fixtures (#14, #15) - cudaDeps.test isPreMaxwellNvidiaBox describe block: split the mislabeled 'every card is at or above the floor' assertion into two focused tests (every-card-above-floor / every-card-below-floor); add a darwin routing suite (asserts darwin does NOT hit the Windows branch via substring 'win' and lands on the archive landing page). - providerCatalog.test cross-family lockstep test title now uses a template literal so the floor number interpolates into the test name (was a verbatim '$floor' string). e2e/scroll-bounds-guardrail.spec.ts (#7, #8, #9) - Page.evaluate wraps Radix Tooltip elements in the full Provider/Root/Trigger/Portal/Content hierarchy using React.createElement so the elements go through the JSX reconciler (plain RdxTooltip.Portal(...) / Content(...) function calls were missing Radix's Provider context and the sentinel never mounted). - DOM-fallback branch preserves the actual import-error string so a future failure is filed with the real reason, not a synthetic 'Radix bare imports did not resolve' placeholder. - Added paint-time hit-test: document.elementFromPoint at the bbox centre must resolve to the sentinel (or one of its ancestors up to #root). getClientRects alone cannot detect overflow:hidden clipping because clip preserves the rect coordinates; the hit-test is the actual guardrail. - Restrict the lint-disable comment to the placeholder line so the prettier/sonar warnings stay clean. Verified: tsc clean, 734/734 unit tests + 229/229 server tests pass, pnpm validate:recipe ok, pnpm lint 9 pre-existing warnings / 0 new errors, live /api/system/hardware-probe?refresh=1 reflects the updated probe fields. * fix: add missing useOpenVinoInstall hook for CI typecheck CodeRabbit's auto-fix imported @/components/features/useOpenVinoInstall but never added the module, so pnpm lint (tsc) failed with TS2307. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: address follow-up review on tensorrt.ts pipInstall, install mutex, state-4 test - tensorrt.ts: extract private pipInstall to ../shared/pipInstall.ts; local spawn-based copy removed, all 4 install call-sites now route through the shared helper so cuda/tensorrt/tensorrt-rtx agree on error contract and the '[deps]' output prefix. - IHVIntegrationPanel.tsx: replace installingTrt | installingTrtRtx | installingOrtGpu trio of mutex flags with a single installInProgress derived from all three. The three handler guards, setInstalling(true) cleanup, and React Button disabled props all use the unified flag so a concurrent pip install can no longer race the shared .venv. - hardwareProbe.test.ts: state-4 fixture now passes cuda.loadable:true so the install-hint branch (state 3) is bypassed and the cascade reaches the state-4 driver/wheel mismatch reason. Without cuda.loadable the fixture was exercising state 3 instead of state 4, so the assertion originally committed failed to validate the intended branch. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com> * Update src/components/features/IHVIntegrationPanel.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: extract HardwareProviderCard to clear CodeFactor complexity Move the provider-card map (Very Complex Method) out of IHVIntegrationPanel into a dedicated component with chrome helpers and install/conflict subcomponents so CodeFactor can pass on PR 108. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: address CodeRabbit OpenVINO review findings Share NDJSON install helper with residual-frame flush, add hook tests, require --upgrade with eager strategy, tighten Intel hardware detection via computeOpenVinoCompatibleHardware + lspci/Win32 probe, and gate ORT wheel uninstall with install-failure recovery messaging. Co-authored-by: Anthony Thompson <github@trackdub.com> --------- Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Codebuff <noreply@codebuff.com>
This was referenced Aug 5, 2026
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…egration tests
Phase 1-4: Split server.ts into modular architecture
server.ts: 4800 to 199 lines (-96%) — only CORS + route wiring + Vite/static
6 new service modules: venv/, olive/ (recipe, cuda, tensorrt, tensorrt-rtx), ai/, shared/
7 route modules: ai, env, mcp, olive, system, github, tensorrt
496 unit tests + 40 integration tests (all pass)
CI-ready vitest config with mocked deps (Python, AI, LM Studio, Ollama, spawn)
pnpm test:integration script + Python 3.10-3.13 doc updates
Summary by cubic
Refactored the Node server into modular services/routes with tighter security and rate limits, cutting
server.tsby 96%. Adds audits/local models UI and Python 3.10–3.13 support, with CodeQL-recognized path guards, safer TensorRT/RTX installs, and a readiness-aware health check.Refactors
venv,oliveincl. CUDA/TensorRT/RTX,ai,shared) and routes (ai,env,mcp,olive,system,github,tensorrt);server.tsnow only CORS, route wiring, and Vite/static.python3/python), and rebased absolute paths viapath.relative+path.join.GET /api/healthwith monotonic-uptime checks; returns 503 untilmarkServerReady()after listen; Tauri desktop waits for readiness to avoid half-ready fetches.vitest(pnpm test:server,pnpm test:integration), including health endpoint regression tests.appConfig.StudioConfigfrom shared types.Routerimport in AI routes; spawn error handling for TensorRT and TensorRT RTX pip installs; deduped system route import; hoisted LM Studio CLI cache; preserved cancelled Olive job status across process close; dropped unused imports.New Features
AuditPanel,LocalModelManager, andProviderErrorBlockcomponents with tests; streamlinedGeminiSidebar.src/lib/apiClient.ts).Written for commit 868ce13. Summary will update on new commits.