superseded: folded into #111 - #113
tonythethompson wants to merge 5 commits into
Conversation
Add client-safe venv family policy and server-only family specs with isolated build + transactional promote, migration journal, and dual runtime status. Wire ensureVenv to the default-family seam so existing OpenVINO/TRT callers keep working until PR2 routes by provider. Co-authored-by: Anthony Thompson <github@trackdub.com>
Add ensureProviderCapability, route olive/CUDA/TRT/OpenVINO installs to default vs cuda families with python -m pip + PATH isolation, drop onnxruntime-openvino so OpenVINO cannot replace DirectML ORT, and make DmlExecutionProvider first-class across catalog, hub, validation, and probe. Co-authored-by: Anthony Thompson <github@trackdub.com>
Align system probe notes and recipe package inference with the Python-openvino-only default-family stack. Co-authored-by: Anthony Thompson <github@trackdub.com>
Wire pipInstallForFamily so OpenVINO cannot swap default ORT and CUDA/TRT keeps the pinned onnxruntime-gpu pin. Add unknown-provider 400 coverage and OpenVINO recipe inference without onnxruntime-openvino. Co-authored-by: Anthony Thompson <github@trackdub.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChangesDirectML provider and runtime support
Virtual-environment families
Olive execution routing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/recipeHardwareCompatibility.ts (1)
136-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not classify macOS as Windows.
isWindowsProbe()uses.includes("win"). The platform value"Darwin"matches that condition. Line 142 can therefore mark a DirectML recipe as compatible on macOS, although DirectML requires Windows.Use an exact platform identifier or a
startsWith("win")check. Add a regression test for"Darwin".Proposed fix
function isWindowsProbe(probe: HardwareProbeResult): boolean { - return probe.platform.os.toLowerCase().includes("win"); + return probe.platform.os.trim().toLowerCase().startsWith("win"); }🤖 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/lib/recipeHardwareCompatibility.ts` around lines 136 - 149, Update isWindowsProbe so it recognizes Windows only through an exact Windows platform identifier or a startsWith("win") check, preventing "Darwin" from being classified as Windows. Preserve the DirectML compatibility branching in the targetDevice === "DirectML" path, and add a regression test confirming a "Darwin" probe is unavailable.src/server/services/venv/paths.test.ts (1)
11-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the default-family assertions so they cannot pass for the CUDA root.
.venvs/cudacontains the substring.venv. Lines 13, 19, and 25 therefore still pass ifgetVenvPython()regresses to the CUDA root. The new CUDA test proves the CUDA paths, but nothing proves the default family stays under.venv. Assert againstgetFamilyRoot("default")instead, which is already imported.💚 Proposed fix to bind default assertions to the default family root
it("getVenvPython returns a path inside VENV_DIR by default", () => { const python = getVenvPython(); - expect(python).toContain(".venv"); + expect(python.startsWith(getFamilyRoot("default"))).toBe(true); expect(python).toMatch(/python(\.exe)?$/); }); it("getVenvPip returns a path inside VENV_DIR", () => { const pip = getVenvPip(); - expect(pip).toContain(".venv"); + expect(pip.startsWith(getFamilyRoot("default"))).toBe(true); expect(pip).toMatch(/pip(\.exe)?$/); }); it("getVenvScriptsDir returns a path inside VENV_DIR", () => { const scripts = getVenvScriptsDir(); - expect(scripts).toContain(".venv"); + expect(scripts.startsWith(getFamilyRoot("default"))).toBe(true); });🤖 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/paths.test.ts` around lines 11 - 26, Update the default-family assertions in getVenvPython, getVenvPip, and getVenvScriptsDir tests to verify their paths are under getFamilyRoot("default") rather than merely containing ".venv". Keep the existing filename and executable suffix assertions unchanged.
🤖 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/lib/venvFamily.ts`:
- Around line 39-52: Update the provider-token resolution logic in venvFamily.ts
to use an exact case-insensitive alias map containing only documented aliases
and canonical provider IDs, replacing the current substring checks while
preserving the returned execution-provider names. Ensure tokens such as
unsupported-cpu-backend, fake-cuda, and openvino-custom remain unresolved so the
existing unknown-provider path can return HTTP 400, and add negative tests for
provider-name-containing strings.
In `@src/server/routes/olive.cancel.test.ts`:
- Around line 19-27: In the cancel test’s mocked venv setup, rename the release
promise variable from releaseEnsureVenv to match ensureProviderCapability, and
update the header, gate, and setup-release comments that still reference
ensureVenv. Keep the mock implementation and its other imported symbols
unchanged.
In `@src/server/routes/olive.providerRouting.test.ts`:
- Around line 5-17: Remove the unused getVenvPython entry from the
../services/venv/index.ts mock in the test; if future coverage requires stubbing
it, add a separate mock for ../services/venv/paths.ts instead.
- Around line 80-95: Add positive routing coverage in the providerRouting suite
for known providers, asserting mock.calls on ensureProviderCapability and/or
resolveOliveCommand to verify CUDA-family and default-family selection. Update
the fixed resolveOliveCommand mock or success-path spawn behavior as needed so
the test can reach and inspect routing calls, while preserving the existing
unknown-provider rejection test.
In `@src/server/routes/olive.ts`:
- Around line 89-100: Move the normalizeIhvProvider validation for providerRaw
above job construction and registration, using the already validated recipe
data. Return the existing 400 response immediately when normalization fails,
without creating, registering, or cleaning up a job; then remove the later
validation and teardown block around jobRegistry registration.
In `@src/server/routes/system.ts`:
- Around line 264-271: The readiness flags must reflect only executable provider
families. In src/server/routes/system.ts lines 264-271, remove the assignment
that sets cudaVenvLoadable from the default-family probe, leaving CUDA readiness
sourced only from the CUDA-family probe; in src/server/routes/system.ts line
419, require DmlExecutionProvider in the probed onnxRuntimeProviders alongside
the win32 platform check before setting hasDirectMl.
- Around line 232-243: Capture fs.existsSync(cudaPython) once alongside the
pythonCandidates setup in the surrounding route handler, storing it as
cudaVenvExists. Use this captured value when deciding whether to add cudaPython
and replace every later fs.existsSync(cudaPython) stage-gate check in the probe
loop with cudaVenvExists, keeping all other candidate and probing behavior
unchanged.
In `@src/server/services/olive/cuda.ts`:
- Around line 90-92: Remove the duplicated assertCudaOrtPin adapter from the
CUDA and TensorRT service modules. Reuse a single shared helper exported from
packageConstraints.ts and import it where needed, or replace both call sites
with assertFamilyOrtConstraints("cuda", python) directly, preserving the
existing behavior.
- Around line 137-145: Ensure all four listed installer call sites preserve
their result-object contracts by catching installer errors and returning the
appropriate failure result: update src/server/services/olive/cuda.ts lines
137-145 around the pinnedOrtGpuInstallArgs() call and remove the redundant
assertCudaOrtPin block; update src/server/services/olive/tensorrt.ts lines
116-117 in ensureOnnxRuntimeGpu (or its line 251 caller) to catch failures;
update lines 275-282 around pinnedTensorRtInstallArgs() with a failure return
and remove its redundant assertCudaOrtPin block; and update
src/server/services/olive/tensorrt-rtx.ts lines 231-234 around
tensorrtRtxInstallArgs(), matching the existing ensureTensorRtRtxEpAbi guard.
In `@src/server/services/olive/openvino.ts`:
- Around line 105-120: Update the OpenVINO install retry success path in
probeOpenVino to require retry.optimumIntel?.available in addition to the
runtime check, so a partial installation is not reported as available/loadable.
When the bridge import remains unavailable, return retry.optimumIntel?.detail as
the failure detail, and add a regression test covering runtime-available but
bridge-unavailable after installation.
In `@src/server/services/olive/tensorrt-rtx.ts`:
- Around line 183-191: Remove the unused env parameter from
ensureTensorRtRtxEpAbi and its call site, while preserving family isolation by
passing the family-specific env to every probeTensorRtRtxLoadable invocation in
this module. Update the RTX probe call in src/server/services/olive/tensorrt.ts
similarly, matching ensureTensorRt’s isolated-environment behavior.
In `@src/server/services/olive/tensorrt.ts`:
- Around line 310-315: Update ensureDeps to use the CUDA family environment for
every dependency probe and child process: pass { env } to the execFileAsync
calls and pass env into probeTensorRtLoadable and probeTensorRtRtxLoadable.
Extend probeTensorRtRtxLoadable to accept and use an environment parameter,
preserving its existing behavior while preventing ambient or other-family PATH
resolution.
In `@src/server/services/venv/capabilityEnsure.ts`:
- Around line 91-113: Move the provider ensure-function imports used by
ensureProviderCapability to the module top and replace the inline await import
calls for ensureOpenVino, ensureOnnxRuntimeGpu, ensureTensorRt, and
ensureTensorRtRtx with those static bindings. Preserve the existing result
handling and keep inline imports only where a documented circular dependency
requires them.
- Around line 61-64: Update the capability handling in the surrounding readiness
function to distinguish providers intentionally lacking a capability slot from
family/provider mismatches. Define and use a PROVIDERS_WITHOUT_CAPABILITY_SLOT
set next to resolveVenvFamily for QNN, ROCm, and WebGPU; return ok: true with
the family base only for those providers, while treating cap === undefined for
all other providers as not ready rather than silently succeeding.
In `@src/server/services/venv/familyEnsure.ts`:
- Around line 175-193: Clean up the migration section around buildFamilyIsolated
by removing the contradictory historical plan comments, trailing whitespace, and
the redundant clearBuildingRoot("cuda") call because buildFamilyIsolated already
clears it. Remove the ineffective family === "default" || family === "cuda"
guard near the migration logic, since it covers the entire VenvFamily union,
while preserving the existing build, journal, and promotion behavior.
- Around line 103-105: Update the family spec in `spec.ts` to define a
documented, supported `oliveRequirement` version or range, use that requirement
instead of bare `olive-ai` in the installation flow around `runPythonModule`,
and persist it in the family manifest consumed by `familyNeedsRebuild` so
changing the supported Olive requirement triggers a rebuild.
- Around line 140-161: Update familyNeedsRebuild to use
conflictingOrtDistributions from ./spec.ts for detecting foreign ORT
distributions, replacing the nested dists.some/filter logic. Trigger a rebuild
only when that helper identifies a conflicting distribution, while preserving
the existing required-distribution check.
- Around line 206-218: Update the backup selection in the familyEnsure migration
flow to choose the newest `.venv.backup-*` directory before calling
`fs.renameSync`. Sort candidates using their timestamp suffix when that ordering
is chronological; otherwise stat them and select the greatest `mtimeMs`, while
preserving the existing legacy-exists check and migration journal behavior.
- Around line 197-203: Update the failure branch in the default build flow
around buildFamilyIsolated and defBuild.ok to write the "building" journal state
instead of "default_built" when the build fails, preserving the existing error
propagation and successful "default_promoted" state.
In `@src/server/services/venv/index.ts`:
- Around line 170-193: Update the affected public signatures to use the shared
VenvFamily type instead of the inline "default" | "cuda" union, including the
return type of the Olive launch-argument helper and the family parameter of
buildOliveRunEnvironment. Keep family as an explicit parameter for now, and
document in the function contract that when it differs from provider, the
supplied family takes precedence.
In `@src/server/services/venv/migration.test.ts`:
- Around line 38-45: Update the fixture in the “detects cuda-contaminated
default venv” test to create the Python executable at the platform-specific path
expected by inspectDefaultVenvIntent, using bin/python on Unix-like systems and
Scripts/python.exe on Windows. Keep the existing CUDA package intent assertion
unchanged.
In `@src/server/services/venv/migration.ts`:
- Around line 79-85: Update the ORT distribution classification flow around
probeOrtDists so probe failures do not return "default"; propagate the error or
return an explicit "unknown" result. Ensure the caller handles that failure
state by rebuilding or stopping before reporting the environment as ready, while
preserving the existing classifications for successful probes.
In `@src/server/services/venv/packageConstraints.ts`:
- Around line 103-151: Update pipInstallForFamily to install into an isolated
family environment under the .building tree rather than the live family
interpreter. Run assertFamilyOrtConstraints, including
listInstalledOrtDistributions, against that isolated environment and promote the
built tree to the live runtime only after validation succeeds; discard the
failed build without modifying the existing runtime.
In `@src/server/services/venv/pathIsolation.test.ts`:
- Around line 5-9: Remove the no-op pathJoin helper and its usages in the
pathIsolation tests. Update the CUDA directory assertions in the test around
allFamilyScriptsDirs to use the actual platform-aware path.join(".venvs",
"cuda") layout, adding the appropriate path import, while preserving the
existing directory expectations.
- Around line 11-25: The test named “strips both Scripts dirs then prepends
selected family” does not verify the prepend behavior and omits the default
Scripts-dir removal assertion. Update this test around envForFamily and
allFamilyScriptsDirs to use an existing deterministic directory when asserting
the selected family is prepended, and explicitly assert that the default Scripts
directory is absent; alternatively, rename it to cover stripping only, but still
add the missing default-directory assertion.
In `@src/server/services/venv/pathIsolation.ts`:
- Around line 28-51: Update envForFamily to sanitize the returned family-scoped
environment: remove PYTHONPATH and PYTHONHOME, and set VIRTUAL_ENV to the
selected family’s root directory. Use the existing family/venv path symbols to
derive that root, while preserving the current PATH filtering and prepending
behavior.
In `@src/server/services/venv/promote.test.ts`:
- Around line 40-55: Update the test “backs up live then promotes; rolls back
rename failure when building missing mid-flight” to mock fs.renameSync so the
live-to-backup rename succeeds but the building-to-live rename fails. Assert
result.ok is false, verify the original live tree is restored, and confirm the
building tree remains available after rollback.
In `@src/server/services/venv/spec.ts`:
- Around line 29-34: Replace the module-level MIGRATION_JOURNAL_PATH constant
with a getMigrationJournalPath() function that derives the path from the current
working directory when called. Update every migration journal operation to
invoke this function at use time, keeping journal storage aligned with the
family-root resolution and working-directory changes.
In `@src/server/services/venv/status.ts`:
- Around line 5-11: Merge the separate imports from ../../../lib/venvFamily.ts
into a single import declaration, preserving the existing VenvFamily type,
emptyFamilyFlags, RuntimeFamilyFlags type, and VENV_FAMILIES imports.
- Around line 206-210: Update the OpenVINO capability decision in the status
logic to require both probe.openvino and probe.optimum_intel (the existing
FAMILY_PROBE result) before calling usable(). Keep the missing-capability path
for either absent dependency, so caps.openvino only reports readiness when the
Optimum-Intel bridge is available.
---
Outside diff comments:
In `@src/lib/recipeHardwareCompatibility.ts`:
- Around line 136-149: Update isWindowsProbe so it recognizes Windows only
through an exact Windows platform identifier or a startsWith("win") check,
preventing "Darwin" from being classified as Windows. Preserve the DirectML
compatibility branching in the targetDevice === "DirectML" path, and add a
regression test confirming a "Darwin" probe is unavailable.
In `@src/server/services/venv/paths.test.ts`:
- Around line 11-26: Update the default-family assertions in getVenvPython,
getVenvPip, and getVenvScriptsDir tests to verify their paths are under
getFamilyRoot("default") rather than merely containing ".venv". Keep the
existing filename and executable suffix assertions unchanged.
🪄 Autofix
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: 0ba4a948-d9e0-4c67-9c19-9b45a68159f3
📒 Files selected for processing (52)
.gitignoresrc/components/features/HardwareProviderCard.tsxsrc/components/features/recipe-graph/RecipeValidationPanel.tsxsrc/lib/__tests__/oliveRecipeHub.directml.test.tssrc/lib/__tests__/providerCatalog.test.tssrc/lib/__tests__/venvFamily.test.tssrc/lib/aiWorkspaceContext.tssrc/lib/auditAutofix.tssrc/lib/chatActions.tssrc/lib/hardwareProbe.tssrc/lib/memoryOffload.tssrc/lib/oliveRecipeBuilder.tssrc/lib/oliveRecipeHub.tssrc/lib/openvinoDeps.test.tssrc/lib/openvinoDeps.tssrc/lib/passParameterValidation.tssrc/lib/pipelineValidation.tssrc/lib/presetVramEstimate.tssrc/lib/providerCatalog.tssrc/lib/recipeHardwareCompatibility.tssrc/lib/venvFamily.tssrc/lib/vramEstimate.tssrc/server/routes/env.tssrc/server/routes/olive.cancel.test.tssrc/server/routes/olive.providerRouting.test.tssrc/server/routes/olive.tssrc/server/routes/system.tssrc/server/services/olive/cuda.tssrc/server/services/olive/openvino.tssrc/server/services/olive/recipe.openvino.test.tssrc/server/services/olive/recipe.tssrc/server/services/olive/tensorrt-rtx.tssrc/server/services/olive/tensorrt.tssrc/server/services/shared/pipInstall.tssrc/server/services/venv/capabilityEnsure.tssrc/server/services/venv/config.tssrc/server/services/venv/familyEnsure.tssrc/server/services/venv/index.tssrc/server/services/venv/migration.test.tssrc/server/services/venv/migration.tssrc/server/services/venv/packageConstraints.test.tssrc/server/services/venv/packageConstraints.tssrc/server/services/venv/pathIsolation.test.tssrc/server/services/venv/pathIsolation.tssrc/server/services/venv/paths.test.tssrc/server/services/venv/paths.tssrc/server/services/venv/promote.test.tssrc/server/services/venv/promote.tssrc/server/services/venv/spec.tssrc/server/services/venv/status.tssrc/server/services/venv/systemPython.tssrc/types.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: python-tests
🧰 Additional context used
📓 Path-based instructions (14)
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.
src/**/*.{ts,tsx}: Keep validation logic in shared libraries rather than duplicating it in UI cell helpers or inspectors.
Split theInputEnvironmentPanel,IHVIntegrationPanel, andExecutionWorkspacemega-panels into feature folders with colocated hooks and tests.
Keep server and UI AI provider catalogs synchronized, preferably through a shared provider ID list or synchronization test; register new providers in both catalogs.
Add test coverage forrecipe-graph/,passCatalog,oliveRecipeHub,jobHistoryStore, andvramEstimate, and strengthen component tests for the large panels.
src/**/*.{ts,tsx}: All UI state mutations must go throughcommitUiStateUpdateto enforce invariants; useusePipelineState()for state access andreplaceStatefor recipe imports or preset loads.
UseusePipelineStoreas the single Zustand store for application state; do not introduce separate UI state stores without an architectural reason.
Use the module-level running-state singleton insrc/lib/pipelineNavigation.tsto block navigation while an Olive job is active.
Avoidexport *barrel imports; import directly from the actual module file.
Do not trigger live Olive executions or batch runs in CI or VM environments; recipe building, JSON export, and validation must remain CPU-only.
Do not assume APIs based on prior React or Vite conventions; account for React 19 and Vite 8 breaking changes.
src/**/*.{ts,tsx}: Do not trigger actual Olive optimization runs, including Execute Live or batch runs, in CI or VM environments; use CPU-only recipe building, JSON export, and validation flows instead.
Treat ESLint warnings as acceptable up to the configured limit; only non-zero exits or reported errors are failures.
Do not implement the listed backburner AI p...
Files:
src/server/services/olive/recipe.openvino.test.tssrc/lib/__tests__/providerCatalog.test.tssrc/lib/presetVramEstimate.tssrc/components/features/HardwareProviderCard.tsxsrc/lib/pipelineValidation.tssrc/lib/oliveRecipeBuilder.tssrc/server/services/venv/pathIsolation.tssrc/server/services/venv/migration.test.tssrc/lib/vramEstimate.tssrc/lib/__tests__/venvFamily.test.tssrc/server/routes/env.tssrc/server/services/venv/promote.tssrc/lib/__tests__/oliveRecipeHub.directml.test.tssrc/types.tssrc/server/services/venv/packageConstraints.test.tssrc/server/routes/olive.tssrc/lib/venvFamily.tssrc/server/services/venv/config.tssrc/server/services/venv/capabilityEnsure.tssrc/lib/passParameterValidation.tssrc/lib/memoryOffload.tssrc/lib/oliveRecipeHub.tssrc/server/services/venv/pathIsolation.test.tssrc/server/routes/olive.providerRouting.test.tssrc/server/services/olive/recipe.tssrc/lib/aiWorkspaceContext.tssrc/components/features/recipe-graph/RecipeValidationPanel.tsxsrc/server/services/venv/packageConstraints.tssrc/lib/recipeHardwareCompatibility.tssrc/lib/openvinoDeps.test.tssrc/lib/providerCatalog.tssrc/server/services/venv/paths.test.tssrc/server/services/olive/tensorrt-rtx.tssrc/server/services/venv/promote.test.tssrc/lib/chatActions.tssrc/lib/openvinoDeps.tssrc/server/services/venv/migration.tssrc/server/routes/system.tssrc/server/services/venv/spec.tssrc/server/services/venv/systemPython.tssrc/server/routes/olive.cancel.test.tssrc/server/services/venv/paths.tssrc/lib/auditAutofix.tssrc/server/services/olive/tensorrt.tssrc/server/services/venv/familyEnsure.tssrc/server/services/venv/status.tssrc/server/services/venv/index.tssrc/lib/hardwareProbe.tssrc/server/services/olive/openvino.tssrc/server/services/shared/pipInstall.tssrc/server/services/olive/cuda.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx}: Place imports at the top of modules; use inline imports only for a documented circular dependency.
Run linting and ensure typecheck-related CI checks pass before submitting changes.
For UI or server changes, manually smoke-test development startup, recipe loading/building, validation banners, and live execution when execution behavior is touched.Target Node.js >=22.16 for JavaScript and TypeScript code.
Files:
src/server/services/olive/recipe.openvino.test.tssrc/lib/__tests__/providerCatalog.test.tssrc/lib/presetVramEstimate.tssrc/components/features/HardwareProviderCard.tsxsrc/lib/pipelineValidation.tssrc/lib/oliveRecipeBuilder.tssrc/server/services/venv/pathIsolation.tssrc/server/services/venv/migration.test.tssrc/lib/vramEstimate.tssrc/lib/__tests__/venvFamily.test.tssrc/server/routes/env.tssrc/server/services/venv/promote.tssrc/lib/__tests__/oliveRecipeHub.directml.test.tssrc/types.tssrc/server/services/venv/packageConstraints.test.tssrc/server/routes/olive.tssrc/lib/venvFamily.tssrc/server/services/venv/config.tssrc/server/services/venv/capabilityEnsure.tssrc/lib/passParameterValidation.tssrc/lib/memoryOffload.tssrc/lib/oliveRecipeHub.tssrc/server/services/venv/pathIsolation.test.tssrc/server/routes/olive.providerRouting.test.tssrc/server/services/olive/recipe.tssrc/lib/aiWorkspaceContext.tssrc/components/features/recipe-graph/RecipeValidationPanel.tsxsrc/server/services/venv/packageConstraints.tssrc/lib/recipeHardwareCompatibility.tssrc/lib/openvinoDeps.test.tssrc/lib/providerCatalog.tssrc/server/services/venv/paths.test.tssrc/server/services/olive/tensorrt-rtx.tssrc/server/services/venv/promote.test.tssrc/lib/chatActions.tssrc/lib/openvinoDeps.tssrc/server/services/venv/migration.tssrc/server/routes/system.tssrc/server/services/venv/spec.tssrc/server/services/venv/systemPython.tssrc/server/routes/olive.cancel.test.tssrc/server/services/venv/paths.tssrc/lib/auditAutofix.tssrc/server/services/olive/tensorrt.tssrc/server/services/venv/familyEnsure.tssrc/server/services/venv/status.tssrc/server/services/venv/index.tssrc/lib/hardwareProbe.tssrc/server/services/olive/openvino.tssrc/server/services/shared/pipInstall.tssrc/server/services/olive/cuda.ts
src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the appropriate Vitest configuration for test scope:
src/libunit tests,src/serverserver tests, integration tests with mocked externals, and component tests with jsdom and Testing Library.
Files:
src/server/services/olive/recipe.openvino.test.tssrc/lib/__tests__/providerCatalog.test.tssrc/server/services/venv/migration.test.tssrc/lib/__tests__/venvFamily.test.tssrc/lib/__tests__/oliveRecipeHub.directml.test.tssrc/server/services/venv/packageConstraints.test.tssrc/server/services/venv/pathIsolation.test.tssrc/server/routes/olive.providerRouting.test.tssrc/lib/openvinoDeps.test.tssrc/server/services/venv/paths.test.tssrc/server/services/venv/promote.test.tssrc/server/routes/olive.cancel.test.ts
src/server/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests must mock child_process, AI providers, and fetch while starting a real Express server on a random port.
Files:
src/server/services/olive/recipe.openvino.test.tssrc/server/services/venv/pathIsolation.tssrc/server/services/venv/migration.test.tssrc/server/routes/env.tssrc/server/services/venv/promote.tssrc/server/services/venv/packageConstraints.test.tssrc/server/routes/olive.tssrc/server/services/venv/config.tssrc/server/services/venv/capabilityEnsure.tssrc/server/services/venv/pathIsolation.test.tssrc/server/routes/olive.providerRouting.test.tssrc/server/services/olive/recipe.tssrc/server/services/venv/packageConstraints.tssrc/server/services/venv/paths.test.tssrc/server/services/olive/tensorrt-rtx.tssrc/server/services/venv/promote.test.tssrc/server/services/venv/migration.tssrc/server/routes/system.tssrc/server/services/venv/spec.tssrc/server/services/venv/systemPython.tssrc/server/routes/olive.cancel.test.tssrc/server/services/venv/paths.tssrc/server/services/olive/tensorrt.tssrc/server/services/venv/familyEnsure.tssrc/server/services/venv/status.tssrc/server/services/venv/index.tssrc/server/services/olive/openvino.tssrc/server/services/shared/pipInstall.tssrc/server/services/olive/cuda.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In React code, eliminate request waterfalls, avoid barrel imports, and defer non-critical third-party libraries.
Files:
src/server/services/olive/recipe.openvino.test.tssrc/lib/__tests__/providerCatalog.test.tssrc/lib/presetVramEstimate.tssrc/components/features/HardwareProviderCard.tsxsrc/lib/pipelineValidation.tssrc/lib/oliveRecipeBuilder.tssrc/server/services/venv/pathIsolation.tssrc/server/services/venv/migration.test.tssrc/lib/vramEstimate.tssrc/lib/__tests__/venvFamily.test.tssrc/server/routes/env.tssrc/server/services/venv/promote.tssrc/lib/__tests__/oliveRecipeHub.directml.test.tssrc/types.tssrc/server/services/venv/packageConstraints.test.tssrc/server/routes/olive.tssrc/lib/venvFamily.tssrc/server/services/venv/config.tssrc/server/services/venv/capabilityEnsure.tssrc/lib/passParameterValidation.tssrc/lib/memoryOffload.tssrc/lib/oliveRecipeHub.tssrc/server/services/venv/pathIsolation.test.tssrc/server/routes/olive.providerRouting.test.tssrc/server/services/olive/recipe.tssrc/lib/aiWorkspaceContext.tssrc/components/features/recipe-graph/RecipeValidationPanel.tsxsrc/server/services/venv/packageConstraints.tssrc/lib/recipeHardwareCompatibility.tssrc/lib/openvinoDeps.test.tssrc/lib/providerCatalog.tssrc/server/services/venv/paths.test.tssrc/server/services/olive/tensorrt-rtx.tssrc/server/services/venv/promote.test.tssrc/lib/chatActions.tssrc/lib/openvinoDeps.tssrc/server/services/venv/migration.tssrc/server/routes/system.tssrc/server/services/venv/spec.tssrc/server/services/venv/systemPython.tssrc/server/routes/olive.cancel.test.tssrc/server/services/venv/paths.tssrc/lib/auditAutofix.tssrc/server/services/olive/tensorrt.tssrc/server/services/venv/familyEnsure.tssrc/server/services/venv/status.tssrc/server/services/venv/index.tssrc/lib/hardwareProbe.tssrc/server/services/olive/openvino.tssrc/server/services/shared/pipInstall.tssrc/server/services/olive/cuda.ts
src/server/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Organize server-side services under
src/server/services/, including AI providers, Olive/virtual-environment services, and the job registry.
Files:
src/server/services/olive/recipe.openvino.test.tssrc/server/services/venv/pathIsolation.tssrc/server/services/venv/migration.test.tssrc/server/services/venv/promote.tssrc/server/services/venv/packageConstraints.test.tssrc/server/services/venv/config.tssrc/server/services/venv/capabilityEnsure.tssrc/server/services/venv/pathIsolation.test.tssrc/server/services/olive/recipe.tssrc/server/services/venv/packageConstraints.tssrc/server/services/venv/paths.test.tssrc/server/services/olive/tensorrt-rtx.tssrc/server/services/venv/promote.test.tssrc/server/services/venv/migration.tssrc/server/services/venv/spec.tssrc/server/services/venv/systemPython.tssrc/server/services/venv/paths.tssrc/server/services/olive/tensorrt.tssrc/server/services/venv/familyEnsure.tssrc/server/services/venv/status.tssrc/server/services/venv/index.tssrc/server/services/olive/openvino.tssrc/server/services/shared/pipInstall.tssrc/server/services/olive/cuda.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use the documented React best practices from
docs/REACT_BEST_PRACTICES.md, particularly avoiding waterfalls, barrel imports, and eagerly loading non-critical third-party libraries.
Files:
src/components/features/HardwareProviderCard.tsxsrc/components/features/recipe-graph/RecipeValidationPanel.tsx
src/lib/pipelineValidation.ts
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Extend validation rules in
pipelineValidation.tswhen pass-to-provider compatibility changes.
Files:
src/lib/pipelineValidation.ts
src/lib/oliveRecipeBuilder.ts
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When changing how UI state maps to Olive JSON, update
oliveRecipeBuilder.ts.
Files:
src/lib/oliveRecipeBuilder.ts
src/server/services/venv/**/*.{ts,js}
📄 CodeRabbit inference engine (REVIEW.md)
Pin the runtime
olive-aiinstallation to a supported version or range and document the supported Olive versions.
Files:
src/server/services/venv/pathIsolation.tssrc/server/services/venv/migration.test.tssrc/server/services/venv/promote.tssrc/server/services/venv/packageConstraints.test.tssrc/server/services/venv/config.tssrc/server/services/venv/capabilityEnsure.tssrc/server/services/venv/pathIsolation.test.tssrc/server/services/venv/packageConstraints.tssrc/server/services/venv/paths.test.tssrc/server/services/venv/promote.test.tssrc/server/services/venv/migration.tssrc/server/services/venv/spec.tssrc/server/services/venv/systemPython.tssrc/server/services/venv/paths.tssrc/server/services/venv/familyEnsure.tssrc/server/services/venv/status.tssrc/server/services/venv/index.ts
src/server/routes/{ai,mcp,olive,env}.ts
📄 CodeRabbit inference engine (REVIEW.md)
Apply rate limits to heavy, costly, or secret-mutating endpoints, including AI chat, Codex requests, Ollama pulls, HF token updates, and MCP tool execution.
Files:
src/server/routes/env.tssrc/server/routes/olive.ts
src/server/routes/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Each API route file must export a
mountXxxRoutes(router)function and be wired intoserver.ts.Organize Express server routes under
src/server/routes/, includingai.ts,mcp.ts,olive.ts,env.ts,system.ts, andgithub.ts.
Files:
src/server/routes/env.tssrc/server/routes/olive.tssrc/server/routes/olive.providerRouting.test.tssrc/server/routes/system.tssrc/server/routes/olive.cancel.test.ts
src/server/routes/olive.ts
📄 CodeRabbit inference engine (REVIEW.md)
src/server/routes/olive.ts: Constrain recipe filesystem paths to configured model roots or the working directory; reject..traversal and out-of-root absolute paths before spawning Olive.
Protect job logs from unauthorized access; do not make logs readable solely by an unguessable job ID on an open API.
Files:
src/server/routes/olive.ts
src/lib/oliveRecipeHub.ts
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When changing UI-to-Olive JSON mapping, update related import/export logic in
oliveRecipeHub.ts.
Files:
src/lib/oliveRecipeHub.ts
🧠 Learnings (1)
📚 Learning: 2026-08-04T12:36:02.655Z
Learnt from: tonythethompson
Repo: tonythethompson/Olive-Studio PR: 97
File: src/components/features/BatchProcessingPanel.tsx:0-0
Timestamp: 2026-08-04T12:36:02.655Z
Learning: When updating pipeline state through usePipelineState().setState in React components, do not wrap the update in another commitUiStateUpdate call. PipelineStore.setState already invokes commitUiStateUpdate(store.state, partial) to enforce UI state invariants; a second commit can duplicate the operation and merge against a stale component state snapshot.
Applied to files:
src/components/features/HardwareProviderCard.tsxsrc/components/features/recipe-graph/RecipeValidationPanel.tsx
🪛 ast-grep (0.45.0)
src/server/services/venv/migration.test.ts
[warning] 41-41: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(path.join(pyDir, "python"), "")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
src/server/services/venv/promote.ts
[warning] 29-29: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(file, JSON.stringify(manifest, null, 2), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 36-36: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(file, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
src/server/services/venv/packageConstraints.test.ts
[warning] 59-59: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(constraintPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
src/server/routes/olive.providerRouting.test.ts
[warning] 54-54: Express application should use Helmet
Context: express()
Note: [CWE-693] Protection Mechanism Failure (Express app without Helmet security headers).
(missing-helmet-typescript)
src/server/services/venv/packageConstraints.ts
[warning] 116-116: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(file, ${constraints.join("\n")}\n, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
src/server/services/venv/promote.test.ts
[warning] 25-25: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(path.join(building, "marker"), "new")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 43-43: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(path.join(live, "old"), "1")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 45-45: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(path.join(building, "new"), "2")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
src/server/services/venv/migration.ts
[warning] 44-44: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(MIGRATION_JOURNAL_PATH, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 58-58: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(MIGRATION_JOURNAL_PATH, JSON.stringify(state, null, 2), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
src/server/services/venv/familyEnsure.ts
[warning] 6-6: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
src/server/services/shared/pipInstall.ts
[warning] 9-9: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 GitHub Check: CodeFactor
src/server/services/venv/pathIsolation.ts
[notice] 8-8: src/server/services/venv/pathIsolation.ts#L8
'../../../lib/venvFamily.ts' import is duplicated. (no-duplicate-imports)
src/server/services/olive/tensorrt-rtx.ts
[notice] 186-186: src/server/services/olive/tensorrt-rtx.ts#L186
'env' is defined but never used. Allowed unused args must match /^_/u. (@typescript-eslint/no-unused-vars)
src/server/services/olive/tensorrt.ts
[notice] 315-315: src/server/services/olive/tensorrt.ts#L315
'env' is assigned a value but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
src/server/services/venv/status.ts
[notice] 7-7: src/server/services/venv/status.ts#L7
'../../../lib/venvFamily.ts' import is duplicated. (no-duplicate-imports)
[notice] 178-240: src/server/services/venv/status.ts#L178-L240
Complex Method
🪛 GitHub Check: validate
src/server/services/venv/pathIsolation.ts
[warning] 8-8:
'../../../lib/venvFamily.ts' import is duplicated
src/server/services/olive/tensorrt-rtx.ts
[warning] 186-186:
'env' is defined but never used. Allowed unused args must match /^_/u
src/server/services/olive/tensorrt.ts
[warning] 315-315:
'env' is assigned a value but never used. Allowed unused vars must match /^_/u
src/server/services/venv/status.ts
[warning] 7-7:
'../../../lib/venvFamily.ts' import is duplicated
🔍 Remote MCP GitHub Copilot
Relevant review context
- PR
#113is stacked on PR#111, which introduced the dual-venv foundation; its commits apply provider routing and package constraints afterward. - Runtime specs select
onnxruntime-directmlfor the Windows default family, standardonnxruntimeelsewhere, and pinnedonnxruntime-gpu==1.26.0for the CUDA family. Provider setup resolves the family, installs capability packages, then re-probes it. - Earlier PR
#108installed and requiredonnxruntime-openvino; this PR intentionally removes that requirement. OpenVINO now probesopenvino/optimum-intelonly and preserves the family’s canonical ORT wheel. - Review discrepancy: initial OpenVINO setup requires both OpenVINO and Optimum-Intel, but the post-install retry accepts only
retry.available; runtime capability status likewise checks only the OpenVINO package, not Optimum-Intel. - DirectML is reported for every Windows probe (
process.platform === "win32"), while its capability installer performs no package installation. It is excluded fromisGpuExecutionProvider, but included in GPU/VRAM-related policy lists;getSelectedGpuVramGbhas no DirectML branch and therefore returns no provider-specific VRAM. - Migration currently promotes the CUDA family before rebuilding/promoting the default family; the code comments explicitly acknowledge this sequential approach rather than a single dual-family swap. Recovery behavior around partial migration is worth checking.
- At retrieval time, validation, security, Docker, CodeQL, CodeFactor, and Vercel checks had passed;
python-testswas still in progress.
🔇 Additional comments (38)
src/lib/memoryOffload.ts (1)
12-12: LGTM!src/types.ts (1)
41-41: LGTM!src/lib/providerCatalog.ts (1)
108-121: LGTM!src/lib/auditAutofix.ts (1)
214-214: LGTM!Also applies to: 251-253
src/lib/chatActions.ts (1)
14-14: LGTM!src/lib/passParameterValidation.ts (1)
50-59: LGTM!src/lib/oliveRecipeBuilder.ts (1)
10-10: LGTM!src/lib/pipelineValidation.ts (1)
52-52: 🎯 Functional CorrectnessVerify DirectML-specific quantization compatibility.
These changes use generic GPU membership as a proxy for quantizer support. DirectML now passes the AWQ, GPTQ, SpinQuant, and QuaRot checks in
isQuantMethodAllowed, and the chat layer suppresses its AWQ warning. The DirectML catalog recommends PTQ INT8 and describes INT4 support as limited by operator support.Confirm each affected pass before merge. If a pass is not supported, use provider-specific capability rules instead of the generic GPU set.
src/lib/pipelineValidation.ts#L52-L52: replace the generic quantization gate with a provider-specific capability map, or add explicit DirectML compatibility tests.src/lib/aiWorkspaceContext.ts#L502-L502: use a separate AWQ-compatible provider check so DirectML does not suppress guidance without verified support.As per coding guidelines, validation logic must cover pass-to-provider compatibility when provider support changes.
Based onsrc/lib/providerCatalog.tsLines 108-121, DirectML recommends PTQ INT8 and limits INT4 support to operator compatibility.Source: Coding guidelines
src/lib/__tests__/providerCatalog.test.ts (1)
47-47: LGTM!src/lib/oliveRecipeHub.ts (1)
171-171: LGTM!Also applies to: 225-234
src/lib/presetVramEstimate.ts (1)
18-19: LGTM!src/lib/recipeHardwareCompatibility.ts (1)
91-92: LGTM!src/lib/__tests__/oliveRecipeHub.directml.test.ts (1)
1-33: LGTM!src/components/features/recipe-graph/RecipeValidationPanel.tsx (1)
45-59: LGTM!src/lib/hardwareProbe.ts (1)
144-144: LGTM!Also applies to: 192-242, 266-266, 356-357
src/lib/vramEstimate.ts (1)
204-204: LGTM!src/components/features/HardwareProviderCard.tsx (1)
406-408: LGTM!src/lib/openvinoDeps.ts (1)
2-22: LGTM!Also applies to: 39-39, 55-56
src/lib/openvinoDeps.test.ts (1)
12-20: LGTM!Also applies to: 26-27, 30-36
src/server/services/olive/recipe.ts (1)
10-10: LGTM!Also applies to: 107-114, 137-137
src/server/services/olive/recipe.openvino.test.ts (1)
1-36: LGTM!src/server/services/olive/openvino.ts (1)
4-15: LGTM!Also applies to: 69-79, 121-194
src/server/services/venv/paths.ts (1)
2-30: LGTM!src/server/services/venv/config.ts (1)
6-18: LGTM!src/server/routes/system.ts (1)
363-372: LGTM!src/server/routes/olive.ts (1)
117-130: LGTM!Also applies to: 153-153
src/server/services/venv/systemPython.ts (1)
75-78: 🎯 Functional CorrectnessNo change needed: the supported Python range is consistent.
PYTHON_MAX_RECOMMENDED.minoris13, so Python3.13is accepted by discovery, validation, and the user-facing readiness messages.src/server/services/venv/status.ts (1)
242-305: LGTM!Also applies to: 307-319, 321-358, 361-387
src/server/services/venv/familyEnsure.ts (1)
43-49: LGTM!Also applies to: 278-314
src/server/services/shared/pipInstall.ts (1)
19-42: LGTM!Also applies to: 48-63, 65-73
src/server/services/venv/capabilityEnsure.ts (1)
33-58: LGTM!Also applies to: 79-90
src/server/services/olive/cuda.ts (1)
58-58: LGTM!Also applies to: 98-116
src/server/services/olive/tensorrt.ts (1)
234-253: LGTM!Also applies to: 284-284, 426-432
src/server/services/olive/tensorrt-rtx.ts (1)
199-213: LGTM!Also applies to: 253-253
src/server/routes/env.ts (2)
12-12: LGTM!Also applies to: 152-153, 159-159
156-161: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDenial of Service (CWE-770): Allocation of Resources Without Limits or Throttling
Reachability: External
Reachability path
● Entry src/server/services/olive/openvino.ts:145 ensureOpenVino │ ▼ ● Hop src/server/services/shared/pipInstall.ts:19 pipInstallViaPython │ ▼ ● Hop src/server/services/olive/tensorrt-rtx.ts:183 ensureTensorRtRtxEpAbi │ ▼ ● Hop src/server/services/olive/cuda.ts:98 ensureOnnxRuntimeGpu: install below │ ▼ ● Hop src/server/services/venv/familyEnsure.ts:34 SetupListener │ ▼ ● Hop src/server/services/venv/capabilityEnsure.ts:33 ensureProviderCapability: Providers without a capability slot (QNN/ROCm/WebGPU) only need the family base. │ ▼ ● Hop src/server/services/olive/tensorrt.ts:95 ensureOnnxRuntimeGpu: install below │ ▼ ● Sink src/server/routes/env.ts
DELETE /env/python-pathwrites config with no rate limit.
POST /env/python-pathat line 130 appliesfsWriteRateLimit. This handler performs the same class of mutation: it writes the studio config to disk, deletesOLIVE_STUDIO_PYTHON, and invalidates the runtime status cache. A caller can repeat it without limit and reset the configured interpreter for every in-flight setup.Apply the same limiter.
🛡️ Proposed fix
- router.delete("/env/python-path", async (_req, res) => { + router.delete("/env/python-path", fsWriteRateLimit, async (_req, res) => {As per coding guidelines for
src/server/routes/{ai,mcp,olive,env}.ts: "Apply rate limits to heavy, costly, or secret-mutating endpoints".#!/bin/bash # Confirm which env routes carry a limiter and whether auth middleware fronts the router. rg -n -C2 'router\.(post|delete|put)\(' src/server/routes/env.ts rg -n -C3 'mountEnvRoutes|use\(' src/server/server.ts rg -n -C3 'fsWriteRateLimit|heavyCommandRateLimit' src/server/middleware/rateLimit.tssrc/server/routes/olive.cancel.test.ts (1)
36-40: LGTM!.gitignore (1)
3-6: LGTM!
| const lower = token.toLowerCase(); | ||
| if (lower.includes("nvtensorrtrtx") || lower.includes("tensorrtrtx")) { | ||
| return "NvTensorRTRTXExecutionProvider"; | ||
| } | ||
| if (lower.includes("tensorrt") || lower === "trt") return "TensorrtExecutionProvider"; | ||
| if (lower.includes("directml") || lower === "dml" || lower.includes("dmlexecution")) { | ||
| return "DmlExecutionProvider"; | ||
| } | ||
| if (lower.includes("cuda")) return "CUDAExecutionProvider"; | ||
| if (lower.includes("openvino")) return "OpenVINOExecutionProvider"; | ||
| if (lower.includes("qnn")) return "QNNExecutionProvider"; | ||
| if (lower.includes("rocm")) return "ROCMExecutionProvider"; | ||
| if (lower.includes("webgpu")) return "WebGpuExecutionProvider"; | ||
| if (lower.includes("cpu")) return "CPUExecutionProvider"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unknown provider tokens instead of using substring matches.
The alias logic accepts arbitrary tokens such as unsupported-cpu-backend, fake-cuda, and openvino-custom. These tokens then bypass the required unknown-provider HTTP 400 response and select a runtime family.
Use an exact, case-insensitive alias map. Include only documented aliases and canonical provider IDs. Add negative tests for strings that only contain a provider name.
🤖 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/lib/venvFamily.ts` around lines 39 - 52, Update the provider-token
resolution logic in venvFamily.ts to use an exact case-insensitive alias map
containing only documented aliases and canonical provider IDs, replacing the
current substring checks while preserving the returned execution-provider names.
Ensure tokens such as unsupported-cpu-backend, fake-cuda, and openvino-custom
remain unresolved so the existing unknown-provider path can return HTTP 400, and
add negative tests for provider-name-containing strings.
| vi.mock("../services/venv/index.ts", () => ({ | ||
| ensureVenv: vi.fn(async (onLine: (line: string) => void) => { | ||
| ensureProviderCapability: vi.fn(async (_provider: string, onLine: (line: string) => void) => { | ||
| onLine("[setup] Using default runtime"); | ||
| onLine("[setup] (mock) creating venv…"); | ||
| await new Promise<void>((resolve) => { | ||
| releaseEnsureVenv = resolve; | ||
| }); | ||
| return { ok: true }; | ||
| return { ok: true, family: "default", python: "/tmp/mock-python" }; | ||
| }), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List every symbol olive.ts imports from services/venv/index.ts, then compare with the mock's keys.
rg -nP -A12 'from "\.\./services/venv/index\.ts"' src/server/routes/olive.ts
rg -n 'ensureVenv' src/server/routes/olive.cancel.test.tsRepository: tonythethompson/Olive-Studio
Length of output: 1153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== olive.ts imports from services/venv =="
rg -n 'from ["'\'']\.\./services/venv/index\.ts["'\'']|from ["'\'']\.\./services/venv/index["'\'']|from ["'\'']\.\./services/venv['\''"]' src/server/routes/olive.ts -A 40 -B 5
echo
echo "== olive.cancel.test.ts relevant header and mock =="
sed -n '1,80p' src/server/routes/olive.cancel.test.ts
echo
echo "== all ensureVenv/releaseEnsure mentions in test route files =="
rg -n 'ensureVenv|releaseEnsure|ensureProviderCapability|releaseProviderCapability' src/server/routes/olive* src/server -g '*.ts'Repository: tonythethompson/Olive-Studio
Length of output: 11473
Update the stale ensureVenv references in the cancel test.
olive.ts now imports ensureProviderCapability, buildOliveRunEnvironment, resolveOliveCommand, and detachVenvListener from the mocked module, and the mock object defines all four. Keep the mock unchanged, but rename releaseEnsureVenv to match ensureProviderCapability and update the header, gate comment, and setup-release comments that still say ensureVenv.
🤖 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/olive.cancel.test.ts` around lines 19 - 27, In the cancel
test’s mocked venv setup, rename the release promise variable from
releaseEnsureVenv to match ensureProviderCapability, and update the header,
gate, and setup-release comments that still reference ensureVenv. Keep the mock
implementation and its other imported symbols unchanged.
| vi.mock("../services/venv/index.ts", () => ({ | ||
| ensureProviderCapability: vi.fn(async () => { | ||
| throw new Error("ensureProviderCapability must not run for unknown providers"); | ||
| }), | ||
| buildOliveRunEnvironment: vi.fn(async () => ({}) as NodeJS.ProcessEnv), | ||
| resolveOliveCommand: vi.fn(() => ({ | ||
| executable: "python", | ||
| args: ["-m", "olive"], | ||
| family: "default", | ||
| })), | ||
| detachVenvListener: vi.fn(), | ||
| getVenvPython: vi.fn(() => "/tmp/mock-python"), | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The getVenvPython mock entry is dead.
src/server/routes/olive.ts imports getVenvPython from ../services/venv/paths.ts at its line 25, not from ../services/venv/index.ts. This factory entry is therefore never used. The current test does not reach line 129 of the route, so nothing breaks now. A future success-path test in this file would silently call the real paths.ts helper while this mock suggests otherwise.
Remove the entry, or mock ../services/venv/paths.ts if a later test needs the stub.
♻️ Proposed fix
detachVenvListener: vi.fn(),
- getVenvPython: vi.fn(() => "/tmp/mock-python"),
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.mock("../services/venv/index.ts", () => ({ | |
| ensureProviderCapability: vi.fn(async () => { | |
| throw new Error("ensureProviderCapability must not run for unknown providers"); | |
| }), | |
| buildOliveRunEnvironment: vi.fn(async () => ({}) as NodeJS.ProcessEnv), | |
| resolveOliveCommand: vi.fn(() => ({ | |
| executable: "python", | |
| args: ["-m", "olive"], | |
| family: "default", | |
| })), | |
| detachVenvListener: vi.fn(), | |
| getVenvPython: vi.fn(() => "/tmp/mock-python"), | |
| })); | |
| vi.mock("../services/venv/index.ts", () => ({ | |
| ensureProviderCapability: vi.fn(async () => { | |
| throw new Error("ensureProviderCapability must not run for unknown providers"); | |
| }), | |
| buildOliveRunEnvironment: vi.fn(async () => ({}) as NodeJS.ProcessEnv), | |
| resolveOliveCommand: vi.fn(() => ({ | |
| executable: "python", | |
| args: ["-m", "olive"], | |
| family: "default", | |
| })), | |
| detachVenvListener: vi.fn(), | |
| })); |
🤖 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/olive.providerRouting.test.ts` around lines 5 - 17, Remove
the unused getVenvPython entry from the ../services/venv/index.ts mock in the
test; if future coverage requires stubbing it, add a separate mock for
../services/venv/paths.ts instead.
| describe("POST /olive/run provider routing", () => { | ||
| it("returns 400 for unknown execution providers without ensuring a family", async () => { | ||
| const res = await fetch(`${baseUrl}/api/olive/run`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| recipeJson: JSON.stringify(recipeWithProvider("NotARealExecutionProvider")), | ||
| }), | ||
| }); | ||
| expect(res.status).toBe(400); | ||
| const body = (await res.json()) as { ok: boolean; error?: string }; | ||
| expect(body.ok).toBe(false); | ||
| expect(body.error).toMatch(/Unknown execution provider/i); | ||
| expect(venv.ensureProviderCapability).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add positive family-routing coverage, or rename the suite.
The file is named providerRouting and the PR routes CUDA, TensorRT, and TRT-RTX to the cuda family and OpenVINO to the default family. This suite only proves the rejection path. No test asserts which family a known provider selects.
The harness currently cannot assert it either: resolveOliveCommand is mocked at lines 10-14 to return a fixed family: "default", which is the value under test. To cover routing, assert on the arguments instead. ensureProviderCapability and resolveOliveCommand are already vi.fn(), so a success-path test can read mock.calls and check the provider and family passed through.
💚 Sketch of a routing assertion
+ it("routes a CUDA provider to the cuda family", async () => {
+ vi.mocked(venv.ensureProviderCapability).mockResolvedValueOnce({
+ ok: true,
+ family: "cuda",
+ python: "/tmp/mock-python",
+ });
+ await fetch(`${baseUrl}/api/olive/run`, {
+ method: "POST",
+ headers: { "Content-Type": "application/json" },
+ body: JSON.stringify({
+ recipeJson: JSON.stringify(recipeWithProvider("CUDAExecutionProvider")),
+ }),
+ });
+ expect(venv.ensureProviderCapability).toHaveBeenCalledWith(
+ "CUDAExecutionProvider",
+ expect.any(Function),
+ );
+ expect(vi.mocked(venv.resolveOliveCommand).mock.calls[0]?.[3]).toBe("cuda");
+ });Note that spawn is mocked to throw at lines 23-25, so the route resolves through its catch and returns 500. Either widen that mock for the success path, or assert only on the recorded calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("POST /olive/run provider routing", () => { | |
| it("returns 400 for unknown execution providers without ensuring a family", async () => { | |
| const res = await fetch(`${baseUrl}/api/olive/run`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| recipeJson: JSON.stringify(recipeWithProvider("NotARealExecutionProvider")), | |
| }), | |
| }); | |
| expect(res.status).toBe(400); | |
| const body = (await res.json()) as { ok: boolean; error?: string }; | |
| expect(body.ok).toBe(false); | |
| expect(body.error).toMatch(/Unknown execution provider/i); | |
| expect(venv.ensureProviderCapability).not.toHaveBeenCalled(); | |
| }); | |
| }); | |
| describe("POST /olive/run provider routing", () => { | |
| it("returns 400 for unknown execution providers without ensuring a family", async () => { | |
| const res = await fetch(`${baseUrl}/api/olive/run`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| recipeJson: JSON.stringify(recipeWithProvider("NotARealExecutionProvider")), | |
| }), | |
| }); | |
| expect(res.status).toBe(400); | |
| const body = (await res.json()) as { ok: boolean; error?: string }; | |
| expect(body.ok).toBe(false); | |
| expect(body.error).toMatch(/Unknown execution provider/i); | |
| expect(venv.ensureProviderCapability).not.toHaveBeenCalled(); | |
| }); | |
| it("routes a CUDA provider to the cuda family", async () => { | |
| vi.mocked(venv.ensureProviderCapability).mockResolvedValueOnce({ | |
| ok: true, | |
| family: "cuda", | |
| python: "/tmp/mock-python", | |
| }); | |
| await fetch(`${baseUrl}/api/olive/run`, { | |
| method: "POST", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| recipeJson: JSON.stringify(recipeWithProvider("CUDAExecutionProvider")), | |
| }), | |
| }); | |
| expect(venv.ensureProviderCapability).toHaveBeenCalledWith( | |
| "CUDAExecutionProvider", | |
| expect.any(Function), | |
| ); | |
| expect(vi.mocked(venv.resolveOliveCommand).mock.calls[0]?.[3]).toBe("cuda"); | |
| }); | |
| }); |
🤖 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/olive.providerRouting.test.ts` around lines 80 - 95, Add
positive routing coverage in the providerRouting suite for known providers,
asserting mock.calls on ensureProviderCapability and/or resolveOliveCommand to
verify CUDA-family and default-family selection. Update the fixed
resolveOliveCommand mock or success-path spawn behavior as needed so the test
can reach and inspect routing calls, while preserving the existing
unknown-provider rejection test.
| const providerRaw = | ||
| recipe.systems?.local_system?.config?.accelerators?.[0]?.execution_providers?.[0] ?? | ||
| "CPUExecutionProvider"; | ||
| const provider = normalizeIhvProvider(providerRaw); | ||
| if (!provider) { | ||
| cleanupJobArtifacts(job); | ||
| jobRegistry.delete(jobId); | ||
| return res.status(400).json({ | ||
| ok: false, | ||
| error: `Unknown execution provider: ${String(providerRaw)}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Validate the provider before you register the job.
The provider comes only from recipe, which is parsed and structurally validated at lines 61-69. The code still registers the job at line 87 first, then deletes it at line 95 when normalization fails. The cleanupJobArtifacts(job) call at line 94 is a no-op here, because job.tempRecipePath is not assigned until line 147.
Move the normalization above the job construction. The 400 path then allocates nothing and needs no teardown.
♻️ Proposed reordering
+ const providerRaw =
+ recipe.systems?.local_system?.config?.accelerators?.[0]?.execution_providers?.[0] ??
+ "CPUExecutionProvider";
+ const provider = normalizeIhvProvider(providerRaw);
+ if (!provider) {
+ return res.status(400).json({
+ ok: false,
+ error: `Unknown execution provider: ${String(providerRaw)}`,
+ });
+ }
+
const jobId = uuidv4();
const job: OliveJob = {Then delete the block at lines 89-100.
🤖 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/olive.ts` around lines 89 - 100, Move the
normalizeIhvProvider validation for providerRaw above job construction and
registration, using the already validated recipe data. Return the existing 400
response immediately when normalization fails, without creating, registering, or
cleaning up a job; then remove the later validation and teardown block around
jobRegistry registration.
| export function envForFamily( | ||
| family: VenvFamily, | ||
| base: NodeJS.ProcessEnv = process.env, | ||
| ): NodeJS.ProcessEnv { | ||
| const pathKey = process.platform === "win32" ? "Path" : "PATH"; | ||
| const sep = process.platform === "win32" ? ";" : ":"; | ||
| const strip = new Set(allFamilyScriptsDirs().map(normalizeDir)); | ||
| const existing = (base[pathKey] ?? process.env[pathKey] ?? "") | ||
| .split(sep) | ||
| .filter(Boolean) | ||
| .filter((p) => !strip.has(normalizeDir(p))); | ||
|
|
||
| const env: NodeJS.ProcessEnv = { ...base, [pathKey]: existing.join(sep) }; | ||
|
|
||
| const dirs: string[] = []; | ||
| const scripts = getVenvScriptsDir(family); | ||
| if (fs.existsSync(scripts)) dirs.push(scripts); | ||
| const cfgPy = appConfig.systemPython; | ||
| if (cfgPy) { | ||
| const dir = path.dirname(cfgPy); | ||
| if (fs.existsSync(dir)) dirs.push(dir); | ||
| } | ||
| return envWithPrependedPaths(env, dirs); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find any code or config that sets PYTHONPATH / PYTHONHOME / VIRTUAL_ENV, and confirm no spawn path sanitizes them.
set -uo pipefail
rg -n --type=ts --type=js --type=json -C3 'PYTHONPATH|PYTHONHOME|VIRTUAL_ENV'
rg -n --type=sh -C3 'PYTHONPATH|PYTHONHOME|VIRTUAL_ENV' || true
fd -e yml -e yaml -e env --hidden --exec rg -n -H 'PYTHONPATH|PYTHONHOME|VIRTUAL_ENV' {} \; || true
# Confirm every env-building seam for family spawns.
ast-grep run --pattern 'export function envForFamily($$$) { $$$ }' --lang typescript src/serverRepository: tonythethompson/Olive-Studio
Length of output: 490
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== files =="
git ls-files 'src/server/services/venv/*' 'src/server/**/*.ts' | sed -n '1,200p'
echo
echo "== pathIsolation outline/content =="
wc -l src/server/services/venv/pathIsolation.ts
cat -n src/server/services/venv/pathIsolation.ts
echo
echo "== venv service files =="
fd -e ts . src/server/services/venv -x sh -c 'echo "--- $1"; wc -l "$1"; rg -n "envForFamily|pipInstallForFamily|runPythonModule|virtualenv|venv|python" "$1" || true' sh {}
echo
echo "== broader env vars search in tracked source/config =="
git ls-files | rg '\.(ts|tsx|js|jsx|json|yml|yaml|env|md)$' | xargs rg -n -C2 'PYTHONPATH|PYTHONHOME|VIRTUAL_ENV|envForFamily|virtualenv|venv|python.*PATH|Path|site-packages' || trueRepository: tonythethompson/Olive-Studio
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== path isolation file =="
cat -n src/server/services/venv/pathIsolation.ts | sed -n '1,220p'
echo
echo "== exact family env usages =="
rg -n -C 5 "envForFamily|pipInstallForFamily|runPythonModule|PYTHONPATH|PYTHONHOME|VIRTUAL_ENV|assertFamilyOrtConstraints" src/server/src src/server/services/venv src/server --glob '!src/data/*.ts' || true
echo
echo "== focused spawn/env snippets in services =="
for f in $(rg -l "envForFamily|pipInstallForFamily|runPythonModule" src/server/services/venv src/server --glob '!src/data/*.ts'); do
echo "--- $f"
wc -l "$f"
rg -n -C 8 "spawn|spawnSync|exec|execFile|execFileSync|pipe.*python|python|pip|env" "$f" || true
done
echo
echo "== focused config/env var writes =="
rg -n -C 4 "process\.env|environment|env\s*=" src/server src/lib --glob '!src/data/*.ts' || trueRepository: tonythethompson/Olive-Studio
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== focused venv envForFamily callers =="
rg -n -C 4 "envForFamily\(" src/server/services src/server --glob '!src/data/*.ts' | head -n 240
echo
echo "== focused python spawn / exec env arguments =="
rg -n -C 5 "python.*spawn|spawn.*python|py:|venvPython|execFileAsync|spawnSync|spawn\(" src/server/services/venv src/server/services/shared src/server/services/olive --glob '!src/data/*.ts' | head -n 260
echo
echo "== envForFamily build and callers =="
cat -n src/server/services/venv/pathIsolation.ts | sed -n '1,80p'
cat -n src/server/services/venv/familyEnsure.ts | sed -n '50,125p'
cat -n src/server/services/shared/pipInstall.ts | sed -n '1,85p'
cat -n src/server/services/venv/index.ts | sed -n '170,195p'
cat -n src/server/services/tensorrtDeps.ts | sed -n '1,80p' 2>/dev/null || true
echo
echo "== Python import path behavior probe (no repo execution) =="
python3 - <<'PY'
from pathlib import Path
import tempfile
import os
p = Path.cwd()
for i in range(10):
p = p.parent if str(p) != "/" else p
x = Path.home()
for i in range(10):
x = x.parent if str(x) != "/" else x
print({"cwd": str(Path.cwd()), "home": str(Path.home()), "parent_chain_cwd": str(p), "parent_chain_home": str(x), "abs_import_path_matches_home": str(Path.home()).lower() in os.path.abspath('.' ).lower()})
PYRepository: tonythethompson/Olive-Studio
Length of output: 44674
Sanitize Python import environment in envForFamily.
envForFamily only changes PATH, but pipInstallForFamily, family setup, and probe paths pass the returned env to Python modules. PYTHONPATH can add another family’s site-packages to sys.path, so the CUDA/default ORT invariant can report the wrong wheel. In family-scoped envs, delete PYTHONPATH and PYTHONHOME, and set VIRTUAL_ENV to the selected family root.
🤖 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/pathIsolation.ts` around lines 28 - 51, Update
envForFamily to sanitize the returned family-scoped environment: remove
PYTHONPATH and PYTHONHOME, and set VIRTUAL_ENV to the selected family’s root
directory. Use the existing family/venv path symbols to derive that root, while
preserving the current PATH filtering and prepending behavior.
Source: Linters/SAST tools
| it("backs up live then promotes; rolls back rename failure when building missing mid-flight", () => { | ||
| const live = getFamilyRoot("default"); | ||
| const building = getFamilyBuildingRoot("default"); | ||
| fs.mkdirSync(live, { recursive: true }); | ||
| fs.writeFileSync(path.join(live, "old"), "1"); | ||
| fs.mkdirSync(building, { recursive: true }); | ||
| fs.writeFileSync(path.join(building, "new"), "2"); | ||
|
|
||
| const result = promoteBuildingToLive("default"); | ||
| expect(result.ok).toBe(true); | ||
| expect(fs.existsSync(path.join(live, "new"))).toBe(true); | ||
| // Backup retained under .venv.backup-* | ||
| const backups = fs.readdirSync(tmp).filter((n) => n.startsWith(".venv.backup-")); | ||
| expect(backups.length).toBe(1); | ||
| expect(fs.existsSync(path.join(tmp, backups[0]!, "old"))).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test the rename-failure rollback path or correct the test name.
This test performs a successful promotion. It does not cause the second rename to fail. It also does not verify that rollback restores the old live tree and the building tree.
Mock fs.renameSync so the building → live rename fails after live → backup succeeds. Then assert that the old live tree is restored and that result.ok is false.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 43-43: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(path.join(live, "old"), "1")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 45-45: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(path.join(building, "new"), "2")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/promote.test.ts` around lines 40 - 55, Update the
test “backs up live then promotes; rolls back rename failure when building
missing mid-flight” to mock fs.renameSync so the live-to-backup rename succeeds
but the building-to-live rename fails. Assert result.ok is false, verify the
original live tree is restored, and confirm the building tree remains available
after rollback.
| /** Journal lives outside either venv so it survives directory swaps. */ | ||
| export const MIGRATION_JOURNAL_PATH = path.join( | ||
| process.cwd(), | ||
| ".olive-studio", | ||
| "runtime-migration.json", | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Resolve the migration journal path from the current family root context.
MIGRATION_JOURNAL_PATH captures process.cwd() during module evaluation. The family root functions resolve it when called. If the working directory changes, the journal and virtual environments use different workspaces.
This also defeats the isolation in migration.test.ts. That test changes the working directory after importing this constant, so it writes the journal into the original checkout.
Replace the constant with a function such as getMigrationJournalPath() and resolve it at each journal operation.
🤖 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/spec.ts` around lines 29 - 34, Replace the
module-level MIGRATION_JOURNAL_PATH constant with a getMigrationJournalPath()
function that derives the path from the current working directory when called.
Update every migration journal operation to invoke this function at use time,
keeping journal storage aligned with the family-root resolution and
working-directory changes.
| import type { IHVProvider } from "../../../types.ts"; | ||
| import type { VenvFamily } from "../../../lib/venvFamily.ts"; | ||
| import { | ||
| emptyFamilyFlags, | ||
| type RuntimeFamilyFlags, | ||
| VENV_FAMILIES, | ||
| } from "../../../lib/venvFamily.ts"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Merge the duplicated venvFamily.ts import.
Line 6 and lines 7-11 both import from ../../../lib/venvFamily.ts. The validate check and CodeFactor report no-duplicate-imports. Merge them into one statement.
♻️ Proposed fix
import type { IHVProvider } from "../../../types.ts";
-import type { VenvFamily } from "../../../lib/venvFamily.ts";
import {
emptyFamilyFlags,
type RuntimeFamilyFlags,
+ type VenvFamily,
VENV_FAMILIES,
} from "../../../lib/venvFamily.ts";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { IHVProvider } from "../../../types.ts"; | |
| import type { VenvFamily } from "../../../lib/venvFamily.ts"; | |
| import { | |
| emptyFamilyFlags, | |
| type RuntimeFamilyFlags, | |
| VENV_FAMILIES, | |
| } from "../../../lib/venvFamily.ts"; | |
| import type { IHVProvider } from "../../../types.ts"; | |
| import { | |
| emptyFamilyFlags, | |
| type RuntimeFamilyFlags, | |
| type VenvFamily, | |
| VENV_FAMILIES, | |
| } from "../../../lib/venvFamily.ts"; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 7-7: src/server/services/venv/status.ts#L7
'../../../lib/venvFamily.ts' import is duplicated. (no-duplicate-imports)
🪛 GitHub Check: validate
[warning] 7-7:
'../../../lib/venvFamily.ts' import is duplicated
🤖 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/status.ts` around lines 5 - 11, Merge the separate
imports from ../../../lib/venvFamily.ts into a single import declaration,
preserving the existing VenvFamily type, emptyFamilyFlags, RuntimeFamilyFlags
type, and VENV_FAMILIES imports.
Source: Linters/SAST tools
Gate hasDirectMl on a loadable DmlExecutionProvider, run CUDA/TRT hardware probes under envForFamily, and require Optimum-Intel after OpenVINO install. Co-authored-by: Anthony Thompson <github@trackdub.com>
cursor
Bot
changed the title
Tighten IHV provider alias matching, sanitize family env (PYTHONPATH/HOME, VIRTUAL_ENV), fail-closed migration probing, pin olive-ai, heal ORT contamination after capability pip installs, and wrap CUDA/TRT ensure installs to return ok:false instead of throwing. Co-authored-by: Anthony Thompson <github@trackdub.com>
…ng test Fix spawn mock typing so tsc --noEmit passes for the CUDA success-path test. Co-authored-by: Anthony Thompson <github@trackdub.com>
* feat: dual-venv family foundation (PR1) Add client-safe venv family policy and server-only family specs with isolated build + transactional promote, migration journal, and dual runtime status. Wire ensureVenv to the default-family seam so existing OpenVINO/TRT callers keep working until PR2 routes by provider. Co-authored-by: Anthony Thompson <github@trackdub.com> * feat: route runtimes by venv family and first-class DirectML (PR2) Add ensureProviderCapability, route olive/CUDA/TRT/OpenVINO installs to default vs cuda families with python -m pip + PATH isolation, drop onnxruntime-openvino so OpenVINO cannot replace DirectML ORT, and make DmlExecutionProvider first-class across catalog, hub, validation, and probe. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: remove remaining onnxruntime-openvino install hints Align system probe notes and recipe package inference with the Python-openvino-only default-family stack. Co-authored-by: Anthony Thompson <github@trackdub.com> * feat: enforce family packageConstraints on capability installs Wire pipInstallForFamily so OpenVINO cannot swap default ORT and CUDA/TRT keeps the pinned onnxruntime-gpu pin. Add unknown-provider 400 coverage and OpenVINO recipe inference without onnxruntime-openvino. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: tighten DirectML detection, CUDA probe isolation, OpenVINO ensure Gate hasDirectMl on a loadable DmlExecutionProvider, run CUDA/TRT hardware probes under envForFamily, and require Optimum-Intel after OpenVINO install. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: repair contaminated default even when CUDA exists Always migrate GPU-contaminated .venv, build both family trees before promote, and roll back CUDA if default promote fails mid-migration. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: Pass isolated environment to RTX probes * fix: Fail fast on invalid ORT constraints * fix: address CodeRabbit findings from #113 Tighten IHV provider alias matching, sanitize family env (PYTHONPATH/HOME, VIRTUAL_ENV), fail-closed migration probing, pin olive-ai, heal ORT contamination after capability pip installs, and wrap CUDA/TRT ensure installs to return ok:false instead of throwing. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: address CodeRabbit findings from #113 — typing in provider routing test Fix spawn mock typing so tsc --noEmit passes for the CUDA success-path test. Co-authored-by: Anthony Thompson <github@trackdub.com> * Apply CodeRabbit fixes: DML quant gating, WebGPU mapping, venv hardening - Remove DmlExecutionProvider from GPU_PROVIDERS (block AWQ/GPTQ/SpinQuant/QuaRot/QLoRA on DirectML) - DirectML recipe compat: install hint when EP missing on Windows; WebGPU catalog mapping - Canonicalize EP in olive route before serialize; openvino probe uses envForFamily - venv: pythonPathForRoot helper, journal validation, PEP 503 normalize, spec constraint split - status: openvino requires optimum-intel, integrityHealthy checks manifest, parallel probe - migration rollback mirrors try-path semantics on cuda_promoted failure Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: isolate buildFamilyTree env against the building root envForFamily pointed VIRTUAL_ENV/PATH at the live family during .building installs. Add envForVenvRoot and use it for isolated tree builds. Co-authored-by: Anthony Thompson <github@trackdub.com> * feat(venv): add isolated OpenVINO runtime family and fix hardware probe policy - Add openvino venv family (.venvs/openvino) with onnxruntime-openvino, openvino, and optimum-intel; route OpenVINOExecutionProvider to it - Keep default/cuda forbidding onnxruntime-openvino via packageConstraints - Probe all three families in getDualRuntimeStatus; openvino capability requires ORT EP plus Python stack on the openvino family only - Rewrite ensureOpenVino to target the openvino family - Fix system hardware probe: per-family ORT providers, default-only DML detection, TRT loadable flags on default fallback when cuda venv absent - Add systemHardwareProbePolicy helpers with unit tests Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: route OpenVINO recipe deps to openvino-family ORT inferRequiredPackages now installs onnxruntime-openvino for OpenVINO recipes, matching the isolated .venvs/openvino runtime. Tighten DML detection helper signature (default-runtime only). Co-authored-by: Anthony Thompson <github@trackdub.com> * feat: add DirectML one-click install and refresh OpenVINO runtime copy Wire POST /api/env/install-directml through ensureDirectMl into the Hardware panel, use a DirectX-neutral recipe install hint, and point OpenVINO install UI at the isolated .venvs/openvino ORT family. Co-authored-by: Anthony Thompson <github@trackdub.com> * feat: wire OpenVINO CPU/GPU/NPU target device into recipes Add openvinoTargetDevice to UI state, map it through providerToAccelerator to Olive accelerator.device, probe-gate the Hardware Target Device select, and import the device from OpenVINO recipes. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: address remaining dual-venv CodeRabbit findings Gate CUDA diagnostics on cudaVenvLoadable instead of the merged ORT provider list, fix migration journal phases on default-promote failure, serialize pipInstallForFamily per VenvFamily, and cover building-root path isolation. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: address second-pass dual-venv CodeRabbit findings Pin the OpenVINO family stack, gate DirectML on shared Windows/DX12 readiness, fix controlled-state auto-provider patching, serialize PATH keys case-insensitively, and harden createVenvAt / probe timeouts. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: sync controlled IHV state ref in an effect Move controlledStateRef updates out of render to satisfy react-hooks/refs and unblock CI / validate. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: skip GPU migration when default is already clean Re-check inspectDefaultVenvIntent under the migration lock so a second queued ensure (default + cuda) does not rebuild and re-journal after the first migration already promoted a clean default runtime. Co-authored-by: Anthony Thompson <github@trackdub.com> * fix: tighten DirectML host checks and OpenVINO/venv isolation Correct DirectML deprecation guidance, preserve empty PATH in env isolation, gate DirectML install on computeDirectMlHardwareReady (macOS-safe), force-reinstall OpenVINO ORT when EP is missing, and isolate createVenvAt via envForVenvRoot. Co-authored-by: Anthony Thompson <github@trackdub.com> * chore: merge duplicate venvFamily imports in pathIsolation Co-authored-by: Anthony Thompson <github@trackdub.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
This branch was successfully deployed
2 participants
Superseded
This branch was fast-forward merged into #111 (
cursor/dual-venv-directml-d95f). Further dual-venv / DirectML / OpenVINO routing work lands there.Safe to close once #111 is the active review target.