feat(assistant): reorganize assistant components and refactor MCP arc… - #192
Conversation
…hitecture - Rename assistant feature directory from `gemini/` to `assistant/` to reflect multi-provider support - Extract MCP client logic into dedicated modules (`mcpClient.ts`, `mcpPayload.ts`, `persistentClient.ts`) for better separation of concerns - Move utility hooks to dedicated `hooks/` directory (`useAutoClearError.ts`, `useMcpDiagnostic.ts`, `usePresets.ts`) - Add new components for hardware probing and local file utilities (`HardwareProbeDisplay.tsx`, `localFileUtils.ts`) - Add new recipe catalog hook (`useRecipeCatalog.ts`) for managing optimization recipes - Add access path utilities (`passAccessors.ts`) for improved data structure navigation - Update MCP configuration to use absolute paths and PYTHONPATH for better reproducibility - Add hardware probe test coverage (`passAccessors.test.ts`) - Update documentation and workflow references to reflect new directory structure - Fix markdown formatting in README (table alignment, URL markup) - Remove pinned pnpm version from Tauri build workflow to use latest compatible version - Update tech debt plan documentation and add v0.3 agent gaps analysis
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR expands the Assistant UI, adds provider and local-engine workflows, replaces per-call MCP subprocess execution with a persistent stdio client, extracts shared pipeline utilities, and hardens documentation search against unsafe paths and malformed queries. ChangesAssistant and infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
CodeFactor found an issue: 'filteredRecipes' is assigned a value but never used. Allowed unused vars must match /^_/u. It's currently on: |
|
CodeFactor found multiple issues last seen at c22d354: React Hook useMemo has an unnecessary dependency: 'catalogReady'. Either exclude it or remove the dependency array. |
Greptile SummaryThe PR reorganizes assistant UI modules and extracts hooks, utilities, and MCP infrastructure while introducing a persistent stdio MCP client.
Confidence Score: 4/5The PR is not yet safe to merge because MCP infrastructure failures can still abandon transports and stale close callbacks can invalidate replacement connections. The existing MCP lifecycle finding remains outstanding: cleanup is limited to timeout errors, while non-timeout infrastructure failures clear references without closing the active client and Files Needing Attention: src/server/services/mcp/persistentClient.ts
|
| Filename | Overview |
|---|---|
| src/server/services/mcp/persistentClient.ts | Introduces the persistent MCP connection, but the previously reported infrastructure cleanup and transport-identity lifecycle defects remain. |
| src/server/services/mcp/client.ts | Replaces the former subprocess implementation with a compatibility re-export of the persistent client. |
| src/server/services/mcp/client.test.ts | Adds persistent-client and breaker coverage but does not verify non-timeout cleanup or stale onclose behavior. |
| src/components/features/assistant/AssistantSidebar.tsx | Relocates and renames the provider-agnostic assistant sidebar as part of the feature reorganization. |
| src/lib/passAccessors.ts | Adds typed accessors over the existing flat optimization-pass state. |
| olive-mcp-server/olive_mcp_server/tools/docs_search.py | Adds knowledge-base path containment checks and query sanitization alongside semantic-search tests. |
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR Summary by QodoReorganize assistant components and replace MCP subprocess with persistent client
AI Description
Diagram
High-Level Assessment
Files changed (67)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c22d354f49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| transport.onclose = () => { | ||
| if (state === "connected") { | ||
| state = "crashed"; | ||
| client = null; | ||
| transport = null; |
There was a problem hiding this comment.
When a tool call times out while its transport remains alive, the infrastructure-failure path discards the global references without closing that transport, and a subsequent request can establish a replacement connection. If the old transport then emits onclose, this callback sees the replacement's global state === "connected" and nulls the new client and transport. Associate the callback with the transport/client instance that created it and only clear state when that instance is still current.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9346dcc — the onclose callback now captures a reference to thisTransport at creation time and checks transport === thisTransport before clearing state. Stale close events from superseded transports are ignored.
| # Input sanitization: limit query length and strip control characters. | ||
| query = re.sub(r'[\x00-\x1f\x7f]', '', query[:2000]).strip() |
There was a problem hiding this comment.
For pasted multi-line or tab-separated queries, deleting every control character joins adjacent words—for example, quantization\ncalibration becomes quantizationcalibration. The following split() therefore produces the wrong keyword terms, and semantic/live retrieval also receives a distorted query. Replace whitespace controls with spaces while separately removing unsafe non-whitespace controls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9346dcc — query sanitization now replaces whitespace controls (\\t, \\n, \\r, etc.) with spaces first, then removes only unsafe non-whitespace control chars (\\x00-\\x08, \\x0e-\\x1f, \\x7f). Token boundaries are preserved.
| <div className="border-t border-slate-800/90 pt-2"> | ||
| <button | ||
| type="button" | ||
| onClick={() => setState({ hfModelId: "", hfDataset: "", hfTask: "", localFiles: [], azureModelPath: "" })} |
There was a problem hiding this comment.
When a local file has been selected, this button only clears Zustand fields while the mounted InputEnvironmentPanel retains selectedFileName, chunkFilesRef, reconstruction history, and any reconstruction object URL. If the user then uploads a differently named model, the old non-null selection prevents the new file from becoming active, leaving the metadata inspector empty or showing a cleared reconstruction; large reconstructed blobs also remain retained. Route this action through cleanup that resets the panel's local file state and revokes its object URL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a valid observation about component-level state not being cleared. However, the InputEnvironmentPanel's local state (selectedFileName, chunkFilesRef) is component-scoped — it resets naturally when the user navigates away and back. The Clear button's primary contract is clearing Zustand (the persisted state). Adding an imperative reset of child component refs would require a ref forwarding pattern that adds coupling. Tracking for a separate cleanup PR.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| switch (passes.quantMethod) { | ||
| case "awq": | ||
| return { ...base, method: "awq", groupSize: passes.awqGroupSize, dampPercent: passes.awqDampPercent, sym: passes.awqSym }; | ||
| case "gptq": | ||
| return { ...base, method: "gptq", blockSize: passes.gptqBlockSize, groupSize: passes.gptqGroupSize, descAct: passes.gptqDescAct }; | ||
| case "qat": | ||
| return { ...base, method: "qat", qatPrecision: passes.qatQuantPrecision, calibrateMethod: passes.qatCalibrateMethod, calibrateSteps: passes.qatCalibrateSteps }; | ||
| case "hqq": | ||
| return { ...base, method: "hqq" }; | ||
| case "rtn": | ||
| return { ...base, method: "rtn" }; | ||
| case "spinquant": | ||
| return { ...base, method: "spinquant" }; | ||
| case "quarot": | ||
| return { ...base, method: "quarot" }; | ||
| default: | ||
| return { ...base, method: "ptq" }; | ||
| } |
There was a problem hiding this comment.
3. Quant accessor default masks invalid method 🐞 Bug ≡ Correctness
getQuantConfig's switch has no explicit case "ptq"; the default branch silently returns `method: "ptq"` for any unmatched/invalid quantMethod value, hiding malformed or unexpected runtime state instead of surfacing it. Downstream code branches on config.method, so an invalid value would be silently treated as PTQ rather than causing a visible failure.
Agent Prompt
## Issue description
The `getQuantConfig` function's switch statement on `passes.quantMethod` uses a `default` branch to return `{ ...base, method: "ptq" }`, which means any unexpected/invalid runtime value for `quantMethod` (not just legitimate "ptq") is silently treated as PTQ.
## Issue Context
This function is a typed accessor meant to give compile-time and runtime safety when reading pass config; silently mapping invalid data to "ptq" defeats that purpose and could hide bugs from malformed persisted state or future added quant methods that aren't yet handled in this switch.
## Fix Focus Areas
- src/lib/passAccessors.ts[94-119]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in 9346dcc — added explicit case "ptq" and the default branch now logs a warning with the unexpected value before falling back to PTQ.
Qodo Fixer✅ Committed (1) · ☑ Fixed (1) Commits pushed directly to this PR — no separate fix PR opened. Process — 1 fixed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 67
🤖 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 @.kiro/powers/olive-mcp-tools/mcp.json:
- Around line 7-11: Update the MCP configuration around the run.py args and
PYTHONPATH values to use repository-root-relative or absolute workspace paths,
or explicitly set the server cwd to the repository root. Ensure both the
entrypoint and imports resolve when launched from a non-root working directory,
and add or run a smoke test from such a directory to verify startup.
In `@docs/v0.2-tech-debt-plan.md`:
- Line 58: Update the latency verification under “Verification” to warm the
client with an initial successful request, then time a second request using
curl’s %{time_total} output and assert that elapsed time is below 0.05 seconds.
Preserve the request payload and endpoint while ensuring failures cause the
verification to fail.
- Line 193: Update the fenced code block at the indicated location in the
documentation to specify the text language identifier, changing the opening
fence to use text while preserving the block contents.
- Line 74: Update the round-trip test description to use structural equality
rather than the identity operator: assert that
structuredToFlat(flatToStructured(DEFAULT_PASSES)) deeply equals DEFAULT_PASSES,
using the project’s established object-equality matcher or a canonical
serialized comparison.
In `@docs/v0.3-agent-gaps.md`:
- Line 25: Update the documented plan_optimization MCP contract to use versioned
JSON Schema or Pydantic models instead of TypeScript Partial<UIState> and object
types, covering ui_state_patch, current_recipe, and fixed_recipe. Require UI
responses to be validated and applied through commitUiStateUpdate in
pipelineValidation.ts, while recipe imports and preset loads use replaceState.
- Line 3: Reconcile the MCP tool count referenced by the audit header and the
registry documentation: verify both against the same live registry snapshot,
then update the count in the audit statement and the corresponding value in
POWER.md to match, or document the reason for any intentional difference.
In `@olive-mcp-server/olive_mcp_server/tools/docs_search.py`:
- Around line 90-96: Update _iter_kb_json_files() to wrap each file’s resolve()
and is_relative_to() path-validation operations in a try block catching OSError
and RuntimeError; log the affected file and continue iterating so unresolved or
invalid KB files are skipped without interrupting search or indexing.
In `@olive-mcp-server/tests/test_docs_search_semantic.py`:
- Around line 442-460: Strengthen test_search_query_sanitization by asserting
the returned sanitized query values: verify result["query"] equals
"quantization" after null-byte removal, and verify len(result2["query"]) equals
2000 after truncating the long input. Replace the non-diagnostic count >= 0
assertions with these checks while retaining the existing search calls.
In `@server.ts`:
- Around line 303-310: Update the server shutdown flow around the SIGINT/SIGTERM
handlers and the HTTP server’s close lifecycle so `shutdownMcpClient()` is also
triggered by programmatic `server.close()`. Register a close-event cleanup
handler and reuse one shared in-flight shutdown promise across all shutdown
triggers, while retaining the existing process exit behavior for signal-driven
shutdown.
In `@src/components/features/assistant/aiProviderCatalog.ts`:
- Line 15: Preserve literal provider IDs by removing the explicit type
annotation from PROVIDER_OPTIONS and using an as-const satisfies readonly
ProviderOption[] declaration. In
src/components/features/assistant/useAiProviderSettings.ts lines 353-358, retain
the non-null assertion only if the resulting ProviderId union guarantees a
match; otherwise handle an undefined find result before accessing opt.models[0].
In src/components/features/assistant/ManualProviderSetup.tsx line 30, replace
the ProviderId cast with normalizeUiProviderId(e.target.value) and ignore null
results.
- Around line 216-226: Remove the local-engine type and named re-export block
from aiProviderCatalog.ts, then update every consumer identified by the search
to import those symbols directly from the `@/lib/localEngineStarters` alias.
Preserve each consumer’s existing usage and use the alias without a .ts
extension.
In `@src/components/features/assistant/CodexAccountPanel.tsx`:
- Around line 18-28: In src/components/features/assistant/CodexAccountPanel.tsx
lines 18-28, add role="status" and aria-live="polite" to the Status and
codexMessage paragraphs; apply the same attributes to the corresponding Status
and devinMessage paragraphs in
src/components/features/assistant/DevinAccountPanel.tsx lines 18-30. In
src/components/features/assistant/ManualProviderSetup.tsx line 297, add
role="alert" to the providerSaveError paragraph.
In `@src/components/features/assistant/DevinAccountPanel.tsx`:
- Around line 59-63: Replace the stale gemini- DOM ID prefixes with assistant-
in DevinAccountPanel’s token input and the corresponding provider setup IDs in
ManualProviderSetup, including gemini-settings-devin-token,
gemini-settings-provider, gemini-settings-model, gemini-settings-api-key, and
gemini-cf-account-id. Update all tests and selectors that reference these IDs to
use the renamed assistant- identifiers.
In `@src/components/features/assistant/LocalAiSetupCard.tsx`:
- Around line 45-72: Update the EngineToggle component so both engine buttons
expose their selection state with aria-pressed based on preferredEngine, and
group the mutually exclusive controls with an appropriate accessible grouping
attribute or element. Preserve the existing onSelect behavior and visual
styling.
In `@src/components/features/assistant/LocalModelManager.test.tsx`:
- Around line 17-56: Add tests for LocalModelManager engine scoping and error
handling: render with engine="lms" and an lms.error override, asserting the
server error is displayed, and render with engine="ollama" while verifying
Ollama results appear without any local-models request. Keep mockFetch’s
existing error overrides and cover both single-engine paths.
In `@src/components/features/assistant/LocalModelManager.tsx`:
- Around line 191-241: Centralize the cancellation guard in a ref used by
refresh, replacing the optional isCancelled callback checks with the ref-backed
cancelled function. Update the effect and the handleEnable, handleUnload, and
manual Refresh button call sites to use the shared guard so all refresh
responses are ignored after unmount or engine changes and overlapping requests
cannot write stale state.
In `@src/components/features/assistant/ManualProviderSetup.tsx`:
- Around line 125-138: Update the model select rendering in the component
containing displayedModels so settingsModel is also rendered as an option when
displayedModels does not contain that id, preserving its label/value
sufficiently to keep the controlled select synchronized with saved state. Avoid
adding a duplicate option when the catalog already includes settingsModel, and
keep the existing displayedModels options unchanged.
- Around line 178-180: The keyPlaceholder in ManualProviderSetup should
accurately describe Cloudflare credentials being persisted by
saveManualCloudflareCredentials when no usable environment credential exists.
Replace the memory-only text with a Cloudflare-specific or generic prompt that
explicitly states the API key is stored on disk, while preserving the existing
environment-variable placeholder.
- Around line 33-48: Replace the category-header `<option>` pattern in the
provider select’s PROVIDER_OPTIONS rendering with `<optgroup>` elements labeled
via CATEGORY_LABELS, grouping each category’s provider options under its
corresponding group. Use an ES2022-compatible grouping approach rather than
Object.groupBy, and ensure no empty value="" option is rendered.
In `@src/components/features/assistant/MessageContent.tsx`:
- Around line 8-17: Update the fenced-code rendering logic in MessageContent to
strip the opening and closing ``` markers directly rather than using
lines.slice(1, -1), preserving inline code content for single-line fenced spans.
Remove an optional language tag only when it occupies the opening line, and
retain the existing multiline rendering behavior without the trailing fence
newline.
- Around line 22-23: Update the elems declaration in MessageContent to use
ReactNode[] instead of any[], remove the eslint suppression, and add the
type-only ReactNode import from React.
In `@src/components/features/assistant/ModelCombobox.test.tsx`:
- Around line 63-119: Add two successful-selection tests for ModelCombobox:
verify that focusing the input, using ArrowDown to create an explicit highlight,
and pressing Enter calls onChange with the highlighted option ID; and verify
that firing mouseDown on an option calls onChange with that option’s ID and
closes the list. Use the component’s existing option rendering and observable
list visibility behavior.
In `@src/components/features/assistant/ModelCombobox.tsx`:
- Around line 181-184: Update the non-option list items in ModelCombobox,
including the empty-state message and both truncation notices, to use
role="presentation" while keeping actual model entries as role="option" within
the listbox.
- Around line 101-131: Add a ref for the highlighted option and an effect keyed
to safeHighlight that scrolls the active element into view when keyboard
navigation changes it. Attach the ref to the option rendered by the list when
its index matches safeHighlight, while preserving existing navigation and
rendering behavior.
- Around line 71-80: Wrap closeList in useCallback and import useCallback from
React, preserving its existing behavior and dependencies on stable state
setters. Update the useEffect dependency array to include the stabilized
closeList reference alongside open.
In `@src/components/features/assistant/ProviderErrorBlock.test.tsx`:
- Around line 69-79: Add a test in the ProviderErrorBlock test suite covering a
provider-like message paired with kind="invalid_model_json". Assert that the
structured error renders "Model returned invalid JSON" and that "No AI Provider
Configured" is absent, pinning structured kind precedence over message
heuristics.
In `@src/components/features/assistant/ProviderErrorBlock.tsx`:
- Around line 62-78: Update ProviderErrorBlock to derive the
environment-variable hint from PROVIDER_OPTIONS.keyEnvVar for the displayed
providers instead of maintaining the hardcoded list. Ensure each supported
provider’s variable appears once in the rendered message and the catalog remains
the single source of truth.
- Around line 24-31: Update the isProviderErr classification in
ProviderErrorBlock to recognize 401 and 403 only when they appear as HTTP status
codes, using appropriate boundaries or status-pattern matching rather than bare
substring checks. Preserve detection of the other provider error phrases and
prevent unrelated numbers in messages or model identifiers from entering the
provider branch.
In `@src/components/features/assistant/SettingsPanel.tsx`:
- Around line 26-29: Extract the duplicated provider label logic into a
providerDisplayName helper in aiProviderCatalog.ts, accepting an optional
provider, applying normalizeUiProviderId, looking up PROVIDER_OPTIONS, and
falling back to the raw value or an empty string. Replace the local expression
in SettingsPanel and the providerLabel expression in AssistantSidebar with this
helper, preserving the existing display behavior.
- Around line 120-133: Update the SettingsPanel effect around
deriveAssistantSettingsMode to track the previously derived mode with a useRef,
and only call setSettingsMode when the newly derived value differs from that
stored value. Update the ref whenever derivation changes so provider-status
refreshes do not overwrite a user-selected tab.
In `@src/components/features/assistant/types.ts`:
- Around line 1-14: Make Suggestion.autofix optional in
src/components/features/assistant/types.ts (lines 1-14), preserving AuditPanel
support for suggestions without Apply actions. In
src/components/features/assistant/useAiAudit.ts (lines 50-57), validate the
complete AnalysisResult payload—including score, level, summary, suggestions,
and each suggestion’s required fields—before calling setAnalysis; set
analysisError and do not update analysis when validation fails.
In `@src/components/features/assistant/useAiAudit.ts`:
- Around line 67-68: Update resetAnalysis in useAiAudit to invalidate pending
requests by incrementing analysisRequestIdRef, then clear the displayed
analysis, loading state, and error state. Ensure responses from requests active
before reset cannot restore analysis.
In `@src/components/features/assistant/useAiProviderSettings.ts`:
- Around line 419-421: Update the Codex polling flow around the loop in
useAiProviderSettings to support cancellation: store a cancellation ref, check
it before each delayed poll/request, set it during unmount and when the relevant
panel is no longer open, and stop further state updates or requests once
cancelled. Expose a cancel action from the hook for CodexAccountPanel so Sign
in, Refresh, and Logout can abort the active poll and clear codexBusy.
- Around line 111-154: Extract the Codex authentication lifecycle into a
colocated useCodexAccount hook and the Devin token lifecycle into a colocated
useDevinAccount hook, moving their state and related operations out of the main
provider-settings hook. Compose both hooks from useAiProviderSettings and
preserve their existing behavior and returned values, while keeping provider
form and model-catalog state in the parent. Update CodexAccountPanel and
DevinAccountPanel to consume only their respective hook results rather than the
entire provider-settings object.
- Around line 423-435: Check every provider mutation response before advancing
UI state: in src/components/features/assistant/useAiProviderSettings.ts lines
423-435, capture the Codex activation response, throw on !r.ok, and only then
set the active message and call onProviderActivated(); at lines 491-496, capture
the activation response and throw on failure so the existing catch sets
providerSaveError; at lines 632-636, wrap the DELETE in try/catch, check r.ok,
set providerSaveError on failure, and call onProviderCleared() only after
success.
- Around line 199-201: Update the setModelsSource call in the fetch response
handling to default an omitted data.source to "fallback" rather than inferring
"live" from models.length. Preserve explicit server-provided sources and the
existing model application behavior.
- Around line 323-340: Update both open-time useEffect callbacks around
fetchProviderStatus, refreshCodexAccount, and refreshDevinAccount to create an
ignore/cancellation flag, guard every post-await state update and
onProviderMissing invocation with it, and return cleanup that sets the flag when
dependencies change or the effect unmounts. Ensure fetchProviderStatus receives
or otherwise honors the flag for its internal setProviderStatus write, while
preserving the existing refreshProviderModels sequence protection.
- Around line 66-96: Introduce a shared postJson helper in the
useAiProviderSettings hook that applies a consistent AbortSignal.timeout policy,
then route both fetch calls in persistApiKeyProvider through it. Update every
other fetch in the hook, including the Codex poll loop and clearProvider, to use
the same helper or timeout signal so no request can hang indefinitely while
preserving each request’s existing method, headers, body, and response handling.
- Around line 172-189: Update applyFetchedModels to return immediately when
models is empty, then assign the first model to a local first value after the
guard and replace every models[0]!.id access with first.id. Preserve the
existing state updates and selection logic for non-empty model lists.
- Around line 218-252: Update the fetch flow in the model-refresh function to
check r.ok after the /api/ai/models request, matching fetchProviderStatus. Treat
non-OK responses as errors so execution reaches the existing catch block,
removes providerId from modelsFetchedRef, and exposes the response error rather
than caching the fallback as successful.
In `@src/components/features/assistant/useLocalEngineSetup.ts`:
- Around line 53-72: Update readNdjsonLines to wrap its stream-reading loop and
line-processing in a try/finally block, and always release the reader in finally
via its existing reader cleanup API. Preserve propagation of errors from
handleInstallStreamEvent and handlePullStreamEvent while ensuring the underlying
stream is unlocked and cancelled or otherwise cleaned up on every exit path.
- Around line 236-249: Update consumeInstallStream to swallow only JSON parsing
errors by catching SyntaxError explicitly, while rethrowing other errors
regardless of their message text. Require both a successful response and an
explicit done event indicated by state.ok before calling setLocalInstallInfo or
allowing installation readiness to proceed; preserve the existing Setup failed
error for unsuccessful responses.
In `@src/components/features/execute/BatchProcessingPanel.test.tsx`:
- Around line 44-62: Update the test mocks to define mockFetchKeyedDiagnostic,
batchMcpState, mockRequestFeedback, and the pipeline store mocks via vi.hoisted
before they are referenced by any vi.mock factory. Preserve the existing mock
behavior while ensuring all factory dependencies are initialized during test
collection.
In `@src/components/features/execute/ExecutionWorkspace.test.tsx`:
- Around line 53-55: Update the useAutoClearError mock to hoist a reusable
mockSetAutoClearError with vi.hoisted, return that stable setter from the mock
factory, and allow the mocked error value to be controlled by tests instead of
permanently fixing it to an empty string. Preserve the hook’s tuple shape and
enable assertions for the “Fix Applied” state.
In `@src/components/features/ihv/HardwareProbeDisplay.tsx`:
- Around line 120-130: Update the button’s onClick handler to call
prepareProviderChange with the existing state, recommendedProvider, and
hardwareProbe arguments, then call setState only when it returns a non-null
patch; remove the fallback object that directly assigns ihvProvider.
In `@src/components/features/input/InputEnvironmentPanel.tsx`:
- Around line 290-305: Remove the unused filteredRecipes binding from the
useRecipeCatalog destructuring in InputEnvironmentPanel.tsx, leaving all other
returned values and catalog behavior unchanged.
In `@src/components/features/input/localFileUtils.ts`:
- Around line 93-104: Update getReconstructableGroups to return only groups with
at least two files whose numeric chunk suffixes are consecutive with no gaps;
reject groups such as model.bin.001 and model.bin.003 before they reach
startReconstruction, while preserving valid sequential groups.
In `@src/components/features/input/useRecipeCatalog.ts`:
- Around line 73-108: Replace the in-place SUGGESTED_RECIPES dependency with
React state holding the catalog returned by loadSuggestedRecipes(), initializing
it from the current catalog when appropriate. Update the loading effect to store
the loaded recipes, then derive filteredRecipes, localMatchSummary, and
hardwareMatchSummary from that state and remove catalogReady from their
dependency arrays.
In `@src/components/features/VramEstimateBanner.tsx`:
- Line 38: Add an optional setState prop to VramEstimateBanner and select it
with propSetState ?? storeState.setState for the clear-model write path,
ensuring controlled parents update the same state source the banner renders.
In `@src/lib/__tests__/noNotAModelCopy.test.ts`:
- Line 8: Move PROVIDER_OPTIONS and the provider catalog data from
aiProviderCatalog.ts into a shared src/lib provider-catalog module, then
re-export it from src/components/features/assistant/aiProviderCatalog.ts for
component consumers. Update noNotAModelCopy.test.ts to import the shared lib
module directly, preserving the existing exported API and data.
In `@src/lib/hardwareProbe.ts`:
- Around line 536-541: Update the DirectML branch around
computeDirectMlHardwareReady so it also requires a probe result confirming
DirectML or D3D12 hardware support, rather than relying only on
probe.platform.os. Return null only when both the Windows readiness check and
the hardware capability result are true; otherwise preserve the existing
availability-blocking path.
In `@src/lib/hooks/useMcpDiagnostic.ts`:
- Around line 51-52: Update the new-request initialization in useMcpDiagnostic
to clear the existing diagnostic state before invoking requestMcpDiagnostic,
alongside setIsDiagnosing(true) and setError(null). Preserve the rest of the
diagnostic request flow.
In `@src/lib/hooks/usePresets.ts`:
- Around line 56-70: Update the FileReader handling in the preset import flow to
assign an onerror handler before calling readAsText, and have it pass the read
failure to setError. Keep the existing reader.onload parsing and confirmation
behavior unchanged.
In `@src/lib/mcpClient.ts`:
- Around line 49-54: Extract the shared `{ result }` envelope detection and
unwrapping from `requestMcpDiagnostic` and the other call site around lines
145-149 into a single module-level helper. Replace both inline implementations
with that helper, while preserving the outer `record` in `requestMcpDiagnostic`
for fallback error lookup and retaining the existing object/non-array checks.
In `@src/server/routes/mcp.test.ts`:
- Around line 26-41: Update the `vi.mock` factory for `persistentClient.ts` so
`callOliveMcpTool` throws a clear test failure when
`mcpToolMocks.callOliveMcpToolImpl` is unset instead of delegating to the
original implementation. Remove the now-unused `importOriginal` call, `original`
binding, and spread, while preserving the existing no-op client lifecycle mocks.
- Around line 18-19: Update the existing beforeEach test reset in the MCP route
tests to set callOliveMcpToolImpl back to null, alongside execFileImpl and
execFileCalls. Ensure each test starts without the stub installed.
In `@src/server/services/mcp/client.test.ts`:
- Around line 2-5: Rename the test file from client.test.ts to
persistentClient.test.ts so it matches the persistentClient.ts module under test
and the established source-to-test naming convention. Keep the test contents and
imports unchanged.
- Around line 37-41: The ./paths.ts mock in the client test should preserve the
module’s complete export surface. Update the vi.mock factory around
getMcpPython, buildPythonEnv, and mcpServerDir to use importOriginal and spread
the real module, or add an appropriate type annotation that makes missing
exports a TypeScript error.
- Around line 105-113: Add a test alongside the existing callOliveMcpTools
transport-error test that rejects mocks.callTool with the exact 45-second
timeout message used by persistentClient.ts and verifies the result is
unavailable and mcpBreaker.status() reports one failure with open false. Ensure
the test exercises the timeout classification branch and confirms the breaker
does not record success.
In `@src/server/services/mcp/client.ts`:
- Around line 8-18: Add a brief header comment above the re-exports in the MCP
service facade explicitly stating whether it is a transitional compatibility
layer or the intended public boundary. If transitional, mention the call-site
migration plan; if public, instruct consumers to import through this facade
instead of persistentClient.ts.
In `@src/server/services/mcp/paths.ts`:
- Around line 29-33: Update buildPythonEnv() to obtain serverDir by calling the
existing mcpServerDir() helper instead of recomputing the olive-mcp-server path,
keeping PYTHONPATH construction unchanged.
- Around line 15-42: Update buildPythonEnv() and mcpServerDir() to resolve
olive-mcp-server from the application/module directory or an explicit configured
environment variable rather than process.cwd(). Keep getMcpPython() consistent
with the same launch-directory-independent base path so MCP stdio startup works
regardless of the server’s working directory.
In `@src/server/services/mcp/persistentClient.ts`:
- Around line 187-207: The timeout handling in persistentClient.ts lines 187-207
must classify timed-out calls as infrastructure failures: set hadInfraFailure,
add an unavailable result, await activeClient.close(), and break before the
isInfraError check. Add coverage in client.test.ts lines 105-113 by rejecting
callTool with the 45-second timeout message and asserting unavailable: true, one
breaker failure, and that close was called.
- Around line 86-90: Update the connection error handling in the persistent
client’s connect method to preserve the underlying error while keeping the
sanitized client-facing message. In the catch block that sets state to "crashed"
and clears client and transport, log the original error or attach it as the
thrown Error’s cause.
- Around line 285-297: Update connect() to check a shutdown guard immediately
after await client.connect(transport), closing the newly connected client and
avoiding reassignment when shutdown is in progress. Update shutdownMcpClient()
to await the existing connectingPromise before clearing client, transport,
state, and connectingPromise, and add the shuttingDown guard; reset that guard
in resetPersistentClient() to isolate tests.
- Around line 264-277: Replace the message-substring logic in isInfraError with
SDK error classification: recognize local failures via SdkError and
SdkErrorCode, and wire-level JSON-RPC failures via ProtocolError and
ProtocolErrorCode. Update callers to pass the actual error object rather than
rendered message text, and ensure application-provided tool error messages no
longer open the circuit breaker.
- Around line 46-96: Move the connectingPromise cleanup out of the async IIFE’s
finally block and clear it after assigning the IIFE result, ensuring synchronous
failures from getMcpPython, buildPythonEnv, or StdioClientTransport cannot
overwrite the cleared field with a rejected promise. Preserve retry behavior by
only clearing the field when it still references the current connection attempt.
🪄 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: 5405b1f8-097d-4241-84b7-cc890b8191f1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (67)
.github/README.md.github/workflows/tauri-build.yml.kiro/powers/olive-mcp-tools/mcp.json.kiro/powers/olive-studio-dev/POWER.mddocs/v0.2-tech-debt-plan.mddocs/v0.3-agent-gaps.mdolive-mcp-server/olive_mcp_server/tools/docs_search.pyolive-mcp-server/tests/test_docs_search_semantic.pypackage.jsonserver.tssrc/App.tsxsrc/components/features/VramEstimateBanner.tsxsrc/components/features/assistant/AssistantSidebar.flows.test.tsxsrc/components/features/assistant/AssistantSidebar.test.tsxsrc/components/features/assistant/AssistantSidebar.tsxsrc/components/features/assistant/AuditPanel.test.tsxsrc/components/features/assistant/AuditPanel.tsxsrc/components/features/assistant/ChatPanel.tsxsrc/components/features/assistant/CodexAccountPanel.tsxsrc/components/features/assistant/DevinAccountPanel.tsxsrc/components/features/assistant/LocalAiSetupCard.tsxsrc/components/features/assistant/LocalModelManager.test.tsxsrc/components/features/assistant/LocalModelManager.tsxsrc/components/features/assistant/ManualProviderSetup.tsxsrc/components/features/assistant/MessageContent.tsxsrc/components/features/assistant/ModelCombobox.test.tsxsrc/components/features/assistant/ModelCombobox.tsxsrc/components/features/assistant/ProviderErrorBlock.test.tsxsrc/components/features/assistant/ProviderErrorBlock.tsxsrc/components/features/assistant/SettingsPanel.tsxsrc/components/features/assistant/aiProviderCatalog.tssrc/components/features/assistant/types.tssrc/components/features/assistant/useAiAudit.tssrc/components/features/assistant/useAiChat.tssrc/components/features/assistant/useAiProviderSettings.tssrc/components/features/assistant/useLocalEngineSetup.tssrc/components/features/execute/BatchProcessingPanel.test.tsxsrc/components/features/execute/BatchProcessingPanel.tsxsrc/components/features/execute/ExecutionWorkspace.test.tsxsrc/components/features/execute/ExecutionWorkspace.tsxsrc/components/features/execute/MCPDiagnosticCard.test.tsxsrc/components/features/execute/MCPDiagnosticCard.tsxsrc/components/features/execute/recipe-graph/GraphCanvas.tsxsrc/components/features/execute/recipe-graph/RecipeValidationPanel.tsxsrc/components/features/execute/recipe-graph/inspectors/PruningInspector.tsxsrc/components/features/execute/recipe-graph/inspectors/QuantizationInspector.tsxsrc/components/features/ihv/HardwareProbeDisplay.tsxsrc/components/features/ihv/IHVIntegrationPanel.tsxsrc/components/features/input/InputEnvironmentPanel.tsxsrc/components/features/input/localFileUtils.tssrc/components/features/input/useRecipeCatalog.tssrc/lib/__tests__/hooks.test.tssrc/lib/__tests__/noNotAModelCopy.test.tssrc/lib/__tests__/passAccessors.test.tssrc/lib/hardwareProbe.tssrc/lib/hooks.tssrc/lib/hooks/useAutoClearError.tssrc/lib/hooks/useMcpDiagnostic.tssrc/lib/hooks/usePresets.tssrc/lib/mcpClient.tssrc/lib/mcpPayload.tssrc/lib/passAccessors.tssrc/server/routes/mcp.test.tssrc/server/services/mcp/client.test.tssrc/server/services/mcp/client.tssrc/server/services/mcp/paths.tssrc/server/services/mcp/persistentClient.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)
💤 Files with no reviewable changes (2)
- .github/workflows/tauri-build.yml
- src/lib/hooks.ts
| "olive-mcp-server/run.py" | ||
| ], | ||
| "env": { | ||
| "OLIVE_MCP_RETRIEVAL_MODE": "auto", | ||
| "PYTHONPATH": "olive-mcp-server" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
repo_root="$(git rev-parse --show-toplevel)"
python - "$repo_root" <<'PY'
import json
import sys
from pathlib import Path
root = Path(sys.argv[1])
config = root / ".kiro/powers/olive-mcp-tools/mcp.json"
entry = json.loads(config.read_text(encoding="utf-8"))["mcpServers"]["olive-mcp"]
arg = entry["args"][0]
pythonpath = entry["env"]["PYTHONPATH"]
assert (root / arg).is_file(), arg
assert (root / pythonpath).is_dir(), pythonpath
assert not Path(arg).is_absolute()
assert not Path(pythonpath).is_absolute()
print("Both paths resolve only from the repository root.")
PYRepository: tonythethompson/Olive-Studio
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- mcp.json ---\n'
cat -n .kiro/powers/olive-mcp-tools/mcp.json
printf '\n--- relevant repo references to mcp, kiro, and cwd ---\n'
rg -n "mcp|cwd|PYTHONPATH|OLIVE_MCP|olive-mcp-server" -S . --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!*.pyc' | head -200Repository: tonythethompson/Olive-Studio
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- mcp.json ---'
cat -n .kiro/powers/olive-mcp-tools/mcp.json
printf '%s\n' ''
printf '%s\n' '--- relevant repo references to mcp, kiro, and cwd ---'
rg -n "mcp|cwd|PYTHONPATH|OLIVE_MCP|olive-mcp-server" -S . --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!*.pyc' | head -200 || trueRepository: tonythethompson/Olive-Studio
Length of output: 21742
Use repository-root-relative or absolute MCP paths to avoid cwd coupling.
.kiro/powers/olive-mcp-tools/mcp.json currently uses args[0] = olive-mcp-server/run.py and PYTHONPATH = olive-mcp-server. From a non-root working directory, Python cannot find the entrypoint or import the package. Keep an explicit cwd, or normalize args[0] to an absolute workspace path before launch. A run-time smoke test from a non-root working directory should catch this.
🤖 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 @.kiro/powers/olive-mcp-tools/mcp.json around lines 7 - 11, Update the MCP
configuration around the run.py args and PYTHONPATH values to use
repository-root-relative or absolute workspace paths, or explicitly set the
server cwd to the repository root. Ensure both the entrypoint and imports
resolve when launched from a non-root working directory, and add or run a smoke
test from such a directory to verify startup.
| - Warm call latency < 50ms (mocked process) | ||
| - Graceful shutdown kills child | ||
|
|
||
| **Verification:** `curl -X POST localhost:3000/api/mcp/tool -d '{"toolName":"get_olive_passes","args":{"filter":"quantization"}}'` returns in <50ms (warm). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the latency verification measure a warm call.
The command sends one request and does not measure elapsed time. That request includes process and connection setup, so it cannot verify the <50ms warm-call target.
Send one JSON request to warm the client. Then time a second request with curl -w '%{time_total}' and assert the result.
#!/usr/bin/env bash
set -euo pipefail
payload='{"toolName":"get_olive_passes","args":{"filter":"quantization"}}'
curl --fail --silent --show-error --output /dev/null \
-X POST \
-H 'Content-Type: application/json' \
--data "$payload" \
http://localhost:3000/api/mcp/tool
elapsed="$(
curl --fail --silent --show-error --output /dev/null \
-X POST \
-H 'Content-Type: application/json' \
--data "$payload" \
-w '%{time_total}' \
http://localhost:3000/api/mcp/tool
)"
awk -v elapsed="$elapsed" 'BEGIN { exit !(elapsed < 0.05) }'🤖 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 `@docs/v0.2-tech-debt-plan.md` at line 58, Update the latency verification
under “Verification” to warm the client with an initial successful request, then
time a second request using curl’s %{time_total} output and assert that elapsed
time is below 0.05 seconds. Preserve the request payload and endpoint while
ensuring failures cause the verification to fail.
| - `getQuantConfig(passes)` → discriminated union by method (AwqConfig | GptqConfig | PtqConfig | ...) | ||
| - `getPruningConfig(passes)` → `{ enabled, sparsity, type, method, criteria } | null` | ||
| - `getPeftConfig(passes)` → `{ enabled, method, diffusionLora } | null` | ||
| 2. Round-trip test: `structuredToFlat(flatToStructured(DEFAULT_PASSES)) === DEFAULT_PASSES` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node - <<'NODE'
const original = { enabled: true, format: "onnx" };
const reconstructed = { enabled: true, format: "onnx" };
if (original === reconstructed) {
throw new Error("Object identity unexpectedly matched");
}
if (JSON.stringify(original) !== JSON.stringify(reconstructed)) {
throw new Error("Structural equality check failed");
}
console.log("Structural equality differs from ===.");
NODERepository: tonythethompson/Olive-Studio
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Doc line context:"
sed -n '60,85p' docs/v0.2-tech-debt-plan.md
echo
echo "Repository tests matching DEFAULT_PASSES, flat conversion, jest expect:"
rg -n "DEFAULT_PASSES|flatToStructured|structuredToFlat|toBe\\(|toEqual\\(" . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: tonythethompson/Olive-Studio
Length of output: 50385
Use structural equality for the round-trip assertion.
structuredToFlat(flatToStructured(DEFAULT_PASSES)) creates a new object; === checks object identity in JavaScript/TypeScript, so equal reconstructed passes fail this test. Use an object equality match such as expect(...).toEqual(DEFAULT_PASSES) or compare a canonical serialized representation.
🤖 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 `@docs/v0.2-tech-debt-plan.md` at line 74, Update the round-trip test
description to use structural equality rather than the identity operator: assert
that structuredToFlat(flatToStructured(DEFAULT_PASSES)) deeply equals
DEFAULT_PASSES, using the project’s established object-equality matcher or a
canonical serialized comparison.
|
|
||
| ## Execution Order | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced block.
The fence at Line 193 has no language identifier. markdownlint reports MD040. Use text for this execution-order block.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/v0.2-tech-debt-plan.md` at line 193, Update the fenced code block at the
indicated location in the documentation to specify the text language identifier,
changing the opening fence to use text while preserving the block contents.
Source: Linters/SAST tools
| @@ -0,0 +1,105 @@ | |||
| # v0.3 Agent — MCP Knowledge Gap Analysis | |||
|
|
|||
| > Audited 2026-08-09 against the live MCP server (27 tools, 84+ passes, 22 HW profiles). | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the audited MCP tool count.
This file states 27 tools at Line 3, while .kiro/powers/olive-studio-dev/POWER.md states 26 tools at Line 68. Update both values from the same registry snapshot or record why the counts differ.
🤖 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 `@docs/v0.3-agent-gaps.md` at line 3, Reconcile the MCP tool count referenced
by the audit header and the registry documentation: verify both against the same
live registry snapshot, then update the count in the audit statement and the
corresponding value in POWER.md to match, or document the reason for any
intentional difference.
| if (connectingPromise) return connectingPromise; | ||
|
|
||
| connectingPromise = (async () => { | ||
| state = "connecting"; | ||
| try { | ||
| const python = getMcpPython(); | ||
| const env = buildPythonEnv(); | ||
| const cwd = mcpServerDir(); | ||
|
|
||
| transport = new StdioClientTransport({ | ||
| command: python, | ||
| args: ["-m", "olive_mcp_server"], | ||
| env: { ...env } as Record<string, string>, | ||
| cwd, | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| // Log stderr from the MCP server for diagnostics (don't suppress errors). | ||
| const stderrStream = transport.stderr; | ||
| if (stderrStream && "on" in stderrStream) { | ||
| (stderrStream as NodeJS.ReadableStream).on("data", (chunk: Buffer) => { | ||
| const line = chunk.toString().trim(); | ||
| if (line) { | ||
| // Only log non-empty lines to avoid noise | ||
| process.stderr.write(`[olive-mcp] ${line}\n`); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| transport.onclose = () => { | ||
| if (state === "connected") { | ||
| state = "crashed"; | ||
| client = null; | ||
| transport = null; | ||
| } | ||
| }; | ||
|
|
||
| client = new Client({ name: "olive-studio", version: "0.1.0" }); | ||
| await client.connect(transport); | ||
| state = "connected"; | ||
| } catch { | ||
| state = "crashed"; | ||
| client = null; | ||
| transport = null; | ||
| throw new Error("Failed to connect to Olive MCP server"); | ||
| } finally { | ||
| connectingPromise = null; | ||
| } | ||
| })(); | ||
|
|
||
| return connectingPromise; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A synchronous throw inside the IIFE leaves a permanently rejected connectingPromise.
The finally block on lines 91-93 can run before the assignment on line 48 completes. If getMcpPython(), buildPythonEnv(), or new StdioClientTransport(...) throws synchronously, the async IIFE reaches catch → finally → rejection without ever awaiting. connectingPromise = null executes first; line 48 then writes the already-rejected promise back into connectingPromise.
After that, line 46 returns the same rejected promise on every later call. The connection never retries, so the breaker cooldown cannot recover and every tool call returns MCP_UNAVAILABLE_ERROR until the process restarts. That is fake unavailability, not a real server outage.
Clear the field after the assignment instead of inside the IIFE.
🐛 Proposed fix
- connectingPromise = (async () => {
+ const attempt = (async () => {
state = "connecting";
try {
const python = getMcpPython();
@@
} catch {
state = "crashed";
client = null;
transport = null;
throw new Error("Failed to connect to Olive MCP server");
- } finally {
- connectingPromise = null;
}
})();
+ connectingPromise = attempt;
+ attempt.catch(() => undefined).finally(() => {
+ if (connectingPromise === attempt) connectingPromise = null;
+ });
+
return connectingPromise;📝 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.
| if (connectingPromise) return connectingPromise; | |
| connectingPromise = (async () => { | |
| state = "connecting"; | |
| try { | |
| const python = getMcpPython(); | |
| const env = buildPythonEnv(); | |
| const cwd = mcpServerDir(); | |
| transport = new StdioClientTransport({ | |
| command: python, | |
| args: ["-m", "olive_mcp_server"], | |
| env: { ...env } as Record<string, string>, | |
| cwd, | |
| stderr: "pipe", | |
| }); | |
| // Log stderr from the MCP server for diagnostics (don't suppress errors). | |
| const stderrStream = transport.stderr; | |
| if (stderrStream && "on" in stderrStream) { | |
| (stderrStream as NodeJS.ReadableStream).on("data", (chunk: Buffer) => { | |
| const line = chunk.toString().trim(); | |
| if (line) { | |
| // Only log non-empty lines to avoid noise | |
| process.stderr.write(`[olive-mcp] ${line}\n`); | |
| } | |
| }); | |
| } | |
| transport.onclose = () => { | |
| if (state === "connected") { | |
| state = "crashed"; | |
| client = null; | |
| transport = null; | |
| } | |
| }; | |
| client = new Client({ name: "olive-studio", version: "0.1.0" }); | |
| await client.connect(transport); | |
| state = "connected"; | |
| } catch { | |
| state = "crashed"; | |
| client = null; | |
| transport = null; | |
| throw new Error("Failed to connect to Olive MCP server"); | |
| } finally { | |
| connectingPromise = null; | |
| } | |
| })(); | |
| return connectingPromise; | |
| if (connectingPromise) return connectingPromise; | |
| const attempt = (async () => { | |
| state = "connecting"; | |
| try { | |
| const python = getMcpPython(); | |
| const env = buildPythonEnv(); | |
| const cwd = mcpServerDir(); | |
| transport = new StdioClientTransport({ | |
| command: python, | |
| args: ["-m", "olive_mcp_server"], | |
| env: { ...env } as Record<string, string>, | |
| cwd, | |
| stderr: "pipe", | |
| }); | |
| // Log stderr from the MCP server for diagnostics (don't suppress errors). | |
| const stderrStream = transport.stderr; | |
| if (stderrStream && "on" in stderrStream) { | |
| (stderrStream as NodeJS.ReadableStream).on("data", (chunk: Buffer) => { | |
| const line = chunk.toString().trim(); | |
| if (line) { | |
| // Only log non-empty lines to avoid noise | |
| process.stderr.write(`[olive-mcp] ${line}\n`); | |
| } | |
| }); | |
| } | |
| transport.onclose = () => { | |
| if (state === "connected") { | |
| state = "crashed"; | |
| client = null; | |
| transport = null; | |
| } | |
| }; | |
| client = new Client({ name: "olive-studio", version: "0.1.0" }); | |
| await client.connect(transport); | |
| state = "connected"; | |
| } catch { | |
| state = "crashed"; | |
| client = null; | |
| transport = null; | |
| throw new Error("Failed to connect to Olive MCP server"); | |
| } | |
| })(); | |
| connectingPromise = attempt; | |
| attempt.catch(() => undefined).finally(() => { | |
| if (connectingPromise === attempt) connectingPromise = null; | |
| }); | |
| return connectingPromise; |
🤖 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/mcp/persistentClient.ts` around lines 46 - 96, Move the
connectingPromise cleanup out of the async IIFE’s finally block and clear it
after assigning the IIFE result, ensuring synchronous failures from
getMcpPython, buildPythonEnv, or StdioClientTransport cannot overwrite the
cleared field with a rejected promise. Preserve retry behavior by only clearing
the field when it still references the current connection attempt.
| } catch { | ||
| state = "crashed"; | ||
| client = null; | ||
| transport = null; | ||
| throw new Error("Failed to connect to Olive MCP server"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Do not discard the underlying connect error.
Lines 63-73 pipe MCP server stderr for diagnostics, but the catch here throws away the cause of the spawn failure. An operator who hits ENOENT, a missing olive_mcp_server module, or a Python version mismatch sees only "Failed to connect to Olive MCP server". Log the original error, or attach it as cause.
♻️ Proposed change
- } catch {
+ } catch (err: unknown) {
state = "crashed";
client = null;
transport = null;
- throw new Error("Failed to connect to Olive MCP server");
+ process.stderr.write(
+ `[olive-mcp] connect failed: ${err instanceof Error ? err.message : String(err)}\n`,
+ );
+ throw new Error("Failed to connect to Olive MCP server", { cause: err });
}The client-facing message stays sanitized, so no path detail reaches the HTTP response.
📝 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.
| } catch { | |
| state = "crashed"; | |
| client = null; | |
| transport = null; | |
| throw new Error("Failed to connect to Olive MCP server"); | |
| } catch (err: unknown) { | |
| state = "crashed"; | |
| client = null; | |
| transport = null; | |
| process.stderr.write( | |
| `[olive-mcp] connect failed: ${err instanceof Error ? err.message : String(err)}\n`, | |
| ); | |
| throw new Error("Failed to connect to Olive MCP server", { cause: err }); |
🤖 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/mcp/persistentClient.ts` around lines 86 - 90, Update the
connection error handling in the persistent client’s connect method to preserve
the underlying error while keeping the sanitized client-facing message. In the
catch block that sets state to "crashed" and clears client and transport, log
the original error or attach it as the thrown Error’s cause.
| } catch (err: unknown) { | ||
| // Transport/protocol failure — infrastructure error | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (msg.toLowerCase().includes("timed out")) { | ||
| // A timed-out request may still be in flight; close the session so it | ||
| // cannot poison subsequent calls on the persistent connection. | ||
| try { | ||
| void activeClient.close().catch(() => undefined); | ||
| } catch { | ||
| // Best effort — the connection is marked crashed below. | ||
| } | ||
| } | ||
| if (isInfraError(msg)) { | ||
| hadInfraFailure = true; | ||
| results.push({ error: msg || `MCP tool ${req.toolName} failed`, unavailable: true }); | ||
| // Connection may be dead — break out and fail remaining | ||
| break; | ||
| } | ||
| // Tool-level exception (e.g. method not found) | ||
| results.push({ error: msg || `MCP tool ${req.toolName} failed` }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The 45-second timeout path is both misclassified and untested. The timeout message built on persistentClient.ts line 170 contains "timed out", but isInfraError matches "timeout". A timed-out call therefore falls through to the tool-error branch, hadInfraFailure stays false, and the breaker records success after the session was closed. No test exercises this branch, so the defect ships silently.
src/server/services/mcp/persistentClient.ts#L187-L207: sethadInfraFailure = trueand break on the timeout branch instead of relying onisInfraErrorsubstring matching, and awaitactiveClient.close()before continuing.src/server/services/mcp/client.test.ts#L105-L113: add a test that rejectscallToolwith"MCP tool call timed out after 45 seconds"and assertsunavailable: true,mcpBreaker.status().failures === 1, and thatclosewas called.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 123-207: src/server/services/mcp/persistentClient.ts#L123-L207
Complex Method
📍 Affects 2 files
src/server/services/mcp/persistentClient.ts#L187-L207(this comment)src/server/services/mcp/client.test.ts#L105-L113
🤖 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/mcp/persistentClient.ts` around lines 187 - 207, The
timeout handling in persistentClient.ts lines 187-207 must classify timed-out
calls as infrastructure failures: set hadInfraFailure, add an unavailable
result, await activeClient.close(), and break before the isInfraError check. Add
coverage in client.test.ts lines 105-113 by rejecting callTool with the
45-second timeout message and asserting unavailable: true, one breaker failure,
and that close was called.
| function isInfraError(message: string): boolean { | ||
| const m = message.toLowerCase(); | ||
| return ( | ||
| m.includes("transport") || | ||
| m.includes("connection") || | ||
| m.includes("closed") || | ||
| m.includes("econnrefused") || | ||
| m.includes("epipe") || | ||
| m.includes("spawn") || | ||
| m.includes("killed") || | ||
| m.includes("timeout") || | ||
| m.includes("not connected") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@modelcontextprotocol/client 2.0.0 McpError ErrorCode exported client package
💡 Result:
In version 2.0.0 of @modelcontextprotocol/client, the legacy McpError class and ErrorCode enum have been replaced by a more granular error hierarchy to distinguish between protocol-level errors and local SDK errors [1][2][3]. Key Changes in v2.0.0: 1. ProtocolError and ProtocolErrorCode: The former McpError and ErrorCode have been renamed to ProtocolError and ProtocolErrorCode, respectively [1][4][3]. These are used exclusively for errors that are serialized and transmitted over the wire as JSON-RPC error responses [1][5][2]. 2. SdkError and SdkErrorCode: Local SDK errors—such as connection issues, request timeouts, and capability checks—that do not cross the wire are now handled by SdkError and the SdkErrorCode enum [1][2][4]. Specific legacy codes like RequestTimeout and ConnectionClosed have been migrated from the old ErrorCode to SdkErrorCode [1][3][6]. 3. Backward Compatibility: To assist with migration, later updates (such as PR #1903) introduced deprecated aliases and subclasses that restore the v1 McpError and ErrorCode names for compatibility [7]. Summary Table of Replacements: | v1 Symbol | v2 Symbol | Scope | | --- | --- | --- | | McpError | ProtocolError | Wire / JSON-RPC | | ErrorCode | ProtocolErrorCode | Wire / JSON-RPC | | ErrorCode.RequestTimeout | SdkErrorCode.RequestTimeout | Local SDK | | ErrorCode.ConnectionClosed | SdkErrorCode.ConnectionClosed | Local SDK | For implementation, you should update your imports from @modelcontextprotocol/client to use the new classes [4][3]. For example: import { ProtocolError, ProtocolErrorCode, SdkError, SdkErrorCode } from '@modelcontextprotocol/client'; [4][3] When catching errors, you should now use instanceof checks against the specific error class (ProtocolError or SdkError) and compare against the corresponding error code enum (ProtocolErrorCode or SdkErrorCode) [1][4].
Citations:
- 1: https://ts.sdk.modelcontextprotocol.io/v2/migration/upgrade-to-v2.html
- 2:
v2: Errors refactor (ProtocolError, SdkError, OAuthError) modelcontextprotocol/typescript-sdk#1454 - 3: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/docs/migration/upgrade-to-v2.md
- 4: https://github.com/modelcontextprotocol/typescript-sdk/blob/e4227d13/docs/migration.md
- 5: https://ts.sdk.modelcontextprotocol.io/v2/servers/errors.html
- 6: https://ts.sdk.modelcontextprotocol.io/v2/media/migration.md
- 7: feat(compat): McpError/ErrorCode/JSONRPCError/StreamableHTTPError + OAuth subclass aliases modelcontextprotocol/typescript-sdk#1903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package references =="
rg -n '"`@modelcontextprotocol/client`"|`@modelcontextprotocol/client`' package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null || true
echo
echo "== persistentClient outline =="
ast-grep outline src/server/services/mcp/persistentClient.ts --view expanded 2>/dev/null | sed -n '1,220p' || true
echo
echo "== relevant source slices =="
wc -l src/server/services/mcp/persistentClient.ts
sed -n '1,320p' src/server/services/mcp/persistentClient.ts
echo
echo "== related tests/call sites =="
rg -n "isInfraError|PersistentClient|circuit|breaker|client\.callTool|callTool\\(" tests src -g '*.ts' || trueRepository: tonythethompson/Olive-Studio
Length of output: 23729
Classify MCP failures using SDK error codes, not message substrings.
isInfraError currently opens the MCP circuit breaker on tool error text that contains words like "connection" or "ECONNREFUSED". With @modelcontextprotocol/client 2.0.0, use SdkError / SdkErrorCode for local SDK failures and ProtocolError / ProtocolErrorCode for wire-level JSON-RPC errors, instead of letting application text trip the breaker.
🤖 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/mcp/persistentClient.ts` around lines 264 - 277, Replace
the message-substring logic in isInfraError with SDK error classification:
recognize local failures via SdkError and SdkErrorCode, and wire-level JSON-RPC
failures via ProtocolError and ProtocolErrorCode. Update callers to pass the
actual error object rather than rendered message text, and ensure
application-provided tool error messages no longer open the circuit breaker.
| export async function shutdownMcpClient(): Promise<void> { | ||
| if (client) { | ||
| try { | ||
| await client.close(); | ||
| } catch { | ||
| // Best-effort shutdown | ||
| } | ||
| } | ||
| client = null; | ||
| transport = null; | ||
| state = "idle"; | ||
| connectingPromise = null; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Shutdown can race with an in-flight connect() and orphan the Python child process.
If shutdownMcpClient() runs while connect() is awaiting client.connect(transport), this function sets client = null, connectingPromise = null, and state = "idle". The in-flight attempt then completes and reassigns client and state = "connected" on lines 83-85. Nothing closes that transport afterwards, so python -m olive_mcp_server survives the SIGTERM path.
Await the pending attempt before tearing down, and add a shutdown guard so a late connect closes itself.
🔒️ Proposed fix
+let shuttingDown = false;
+
export async function shutdownMcpClient(): Promise<void> {
+ shuttingDown = true;
+ if (connectingPromise) {
+ await connectingPromise.catch(() => undefined);
+ }
if (client) {
try {
await client.close();
} catch {
// Best-effort shutdown
}
}
client = null;
transport = null;
state = "idle";
connectingPromise = null;
}In connect(), after await client.connect(transport), close immediately when shuttingDown is true. Reset shuttingDown in resetPersistentClient() so tests stay isolated.
🤖 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/mcp/persistentClient.ts` around lines 285 - 297, Update
connect() to check a shutdown guard immediately after await
client.connect(transport), closing the newly connected client and avoiding
reassignment when shutdown is in progress. Update shutdownMcpClient() to await
the existing connectingPromise before clearing client, transport, state, and
connectingPromise, and add the shuttingDown guard; reset that guard in
resetPersistentClient() to isolate tests.
| }: VramEstimateBannerProps) { | ||
| const storeState = usePipelineState(); | ||
| const state = propState ?? storeState.state; | ||
| const setState = storeState.setState; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every VramEstimateBanner render site and show the props passed.
rg -nP --type=tsx --type=ts -C 6 '<VramEstimateBanner\b' srcRepository: tonythethompson/Olive-Studio
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -e
printf 'Tracked banner-related files:\n'
git ls-files | grep -E 'VramEstimateBanner|ExecutionWorkspace|IHVIntegrationPanel|pipelineValidation' || true
printf '\nVramEstimateBanner occurrences in tracked files:\n'
rg -n --glob '*.ts' --glob '*.tsx' '<VramEstimateBanner\b|VramEstimateBanner' src || true
printf '\nBanner outline:\n'
ast-grep outline src/components/features/VramEstimateBanner.tsx || true
printf '\nBanner relevant lines:\n'
cat -n src/components/features/VramEstimateBanner.tsx | sed -n '1,80p;220,260p'Repository: tonythethompson/Olive-Studio
Length of output: 8537
🏁 Script executed:
#!/bin/bash
set -e
printf 'ExecutionWorkspace relevant lines around banner render:\n'
cat -n src/components/features/execute/ExecutionWorkspace.tsx | sed -n '880,955p'
printf '\nIHVIntegrationPanel relevant lines around banner render:\n'
cat -n src/components/features/ihv/IHVIntegrationPanel.tsx | sed -n '340,375p'
printf '\nSearch for commitUiStateUpdate/store setState patterns in panels:\n'
rg -n --glob '*.ts' --glob '*.tsx' 'commitUiStateUpdate|storeState\.setState|setState\(' src/components/features/execute/ExecutionWorkspace.tsx src/components/features/ihv/IHVIntegrationPanel.tsx src/lib/pipelineValidation.ts | head -n 200Repository: tonythethompson/Olive-Studio
Length of output: 7734
Pass setState into the configured write path too.
Both controlled parents pass state={state} to VramEstimateBanner, but the banner still uses storeState.setState. Clicking Clear model then updates the pipeline store while the banner renders values from the prop. Add an optional setState prop and use propSetState ?? storeState.setState, or clear by calling the parent’s update through commitUiStateUpdate.
🤖 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/components/features/VramEstimateBanner.tsx` at line 38, Add an optional
setState prop to VramEstimateBanner and select it with propSetState ??
storeState.setState for the clear-model write path, ensuring controlled parents
update the same state source the banner renders.
| import path from "node:path"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import { PROVIDER_OPTIONS } from "@/components/features/gemini/aiProviderCatalog"; | ||
| import { PROVIDER_OPTIONS } from "@/components/features/assistant/aiProviderCatalog"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the unit-test config defines the `@/` alias and covers src/lib/__tests__.
fd -t f -i 'vitest*config*' -d 2 --exec cat {} \;
rg -n 'alias|environment|include' --glob 'vitest*' --glob 'vite.config*' . || trueRepository: tonythethompson/Olive-Studio
Length of output: 1634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vitest.config.ts =="
sed -n '1,220p' vitest.config.ts
echo "== target test imports and declarations =="
sed -n '1,160p' src/lib/__tests__/noNotAModelCopy.test.ts || true
echo "== aiProviderCatalog outline/search =="
find . -path './node_modules' -prune -o -name aiProviderCatalog.ts -print -print
sed -n '1,260p' src/components/features/assistant/aiProviderCatalog.ts || true
echo "== localEngineStarters imports =="
sed -n '1,220p' src/lib/localEngineStarters.ts || true
echo "== React usage in candidate files =="
rg -n "from ['\"].*react|React|ReactDOM|useState|useEffect|createPortal|`@radix-ui`|`@tanstack/react-query`|`@/components`" src/components/features/assistant/aiProviderCatalog.ts src/lib/localEngineStarters.ts src/lib/__tests__/noNotAModelCopy.test.ts || trueRepository: tonythethompson/Olive-Studio
Length of output: 16554
Move the provider catalog data out of src/components.
vitest.config.ts defines @, includes src/**/*.test.ts with a node environment, and aiProviderCatalog.ts is data plus lib/localEngineStarters.ts exports. The import still violates layering: a src/lib test now depends on a component module. Prefer a shared src/lib provider catalog and expose it from src/components/features/assistant instead of reusing the component file in a src/lib test.
🤖 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/__tests__/noNotAModelCopy.test.ts` at line 8, Move PROVIDER_OPTIONS
and the provider catalog data from aiProviderCatalog.ts into a shared src/lib
provider-catalog module, then re-export it from
src/components/features/assistant/aiProviderCatalog.ts for component consumers.
Update noNotAModelCopy.test.ts to import the shared lib module directly,
preserving the existing exported API and data.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (33)
src/components/features/assistant/LocalAiSetupCard.tsx (1)
45-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Give the engine toggle a pressed state for assistive technology.
The two buttons act as a mutually exclusive selector, but the selected engine is communicated only through Tailwind colors. Screen-reader users get no selection state. Add
aria-pressedand group the pair.♻️ Proposed refactor
- <div className="flex items-center gap-1 p-0.5 bg-slate-900 border border-slate-800 rounded-lg"> + <div + role="group" + aria-label="Local AI engine" + className="flex items-center gap-1 p-0.5 bg-slate-900 border border-slate-800 rounded-lg" + > <button type="button" + aria-pressed={preferredEngine === "lms"} onClick={() => onSelect("lms")}Apply the same
aria-pressedattribute to the Ollama button.🤖 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/components/features/assistant/LocalAiSetupCard.tsx` around lines 45 - 72, Update the EngineToggle component so both engine buttons expose their selection state with aria-pressed based on preferredEngine, and group the mutually exclusive controls with an appropriate accessible grouping attribute or element. Preserve the existing onSelect behavior and visual styling.src/components/features/assistant/LocalModelManager.test.tsx (1)
17-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The
erroroverrides inmockFetchare never used, and theengineprop is never tested.
mockFetchacceptslms.errorandollama.errorand builds HTTP 500 responses for them at Lines 29-31 and 43-45. No test in this file passes either. So the component's error branches atLocalModelManager.tsxLines 124-133 and 138-147 stay uncovered, and this helper carries dead configuration.Every test also renders with the default
engine="all". The engine-scoped behavior — which is exactly whatLocalAiSetupCarduses when it passesengine={local.preferredEngine}— is untested. The error branches only fire whenengineis"lms"or"ollama", so both gaps close with the same test.💚 Proposed test
describe("LocalModelManager — engine scoping and errors", () => { it("surfaces the server error when a single engine is selected", async () => { mockFetch({ lms: { error: "LM Studio is not running" } }); await act(async () => { render(<LocalModelManager isOpen engine="lms" />); }); await waitFor(() => { expect(screen.getByText("LM Studio is not running")).toBeDefined(); }); }); it("does not query the other engine when scoped to ollama", async () => { mockFetch({ ollama: { installedModels: ["phi3.5:3.8b"], runningModels: [] } }); await act(async () => { render(<LocalModelManager isOpen engine="ollama" />); }); await waitFor(() => { expect(screen.getByText("phi3.5:3.8b")).toBeDefined(); }); const called = fetchSpy.mock.calls.map(([u]) => String(u)); expect(called.some((u) => u.includes("local-models"))).toBe(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/components/features/assistant/LocalModelManager.test.tsx` around lines 17 - 56, Add tests for LocalModelManager engine scoping and error handling: render with engine="lms" and an lms.error override, asserting the server error is displayed, and render with engine="ollama" while verifying Ollama results appear without any local-models request. Keep mockFetch’s existing error overrides and cover both single-engine paths.src/components/features/assistant/MessageContent.tsx (2)
8-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A single-line fenced span renders an empty code block.
Line 15 uses
lines.slice(1, -1). That assumes the fence markers sit on their own lines. If the model emits```pip install olive-ai```on one line,lineshas one entry,slice(1, -1)returns[], and the user sees an empty<pre>. The content is silently dropped.Strip the fence markers from the string instead of dropping the first and last lines.
🐛 Proposed fix
if (part.startsWith("```") && part.endsWith("```")) { - const lines = part.split("\n"); + // Drop the fences, then the optional language tag on the opening line. + const inner = part.slice(3, -3).replace(/^[^\n]*\n/, (m) => (m.trim() ? "" : m)); return ( <pre key={i} className="bg-slate-950 p-2.5 rounded-lg border border-slate-800 text-[11px] font-mono text-emerald-400 my-1.5 overflow-x-auto whitespace-pre-wrap" > - {lines.slice(1, -1).join("\n")} + {inner.replace(/\n$/, "")} </pre> ); }🧰 Tools
🪛 React Doctor (0.9.3)
[warning] 12-12: Your users can see & submit the wrong data when this list reorders or filters, so use a stable id like
key={item.id}, not the array index "i".Use a stable id from the item, like
key={item.id}orkey={item.slug}. Index keys break when the list reorders or filters.(no-array-index-as-key)
🤖 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/components/features/assistant/MessageContent.tsx` around lines 8 - 17, Update the fenced-code rendering logic in MessageContent to strip the opening and closing ``` markers directly rather than using lines.slice(1, -1), preserving inline code content for single-line fenced spans. Remove an optional language tag only when it occupies the opening line, and retain the existing multiline rendering behavior without the trailing fence newline.
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Replace
any[]withReactNode[]and drop the lint suppression.The array holds strings and JSX elements only.
ReactNodecovers both, so the@typescript-eslint/no-explicit-anyescape hatch is not needed.♻️ Proposed refactor
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const elems: any[] = []; + const elems: ReactNode[] = [];Add the type import at the top of the module:
import type { ReactNode } from "react";📝 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 { ReactNode } from "react"; const elems: ReactNode[] = [];🤖 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/components/features/assistant/MessageContent.tsx` around lines 22 - 23, Update the elems declaration in MessageContent to use ReactNode[] instead of any[], remove the eslint suppression, and add the type-only ReactNode import from React.src/components/features/assistant/ProviderErrorBlock.tsx (2)
24-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
msg.includes("401")andmsg.includes("403")match any message containing those digits.The provider branch does not render
msg. So a false positive replaces the real error text with "No AI Provider Configured", and the user loses the actual failure reason. A message such asPull failed: 403915 bytes writtenor a model id containing401triggers it.Match the status codes as HTTP statuses, not as bare substrings.
🐛 Proposed fix
const isProviderErr = !isJsonModelErr && (msg.includes("not configured") || msg.includes("API key") || msg.includes("No AI provider") || - msg.includes("401") || - msg.includes("403") || + /\b(401|403)\b/.test(msg) || msg.includes("API route not found"));🤖 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/components/features/assistant/ProviderErrorBlock.tsx` around lines 24 - 31, Update the isProviderErr classification in ProviderErrorBlock to recognize 401 and 403 only when they appear as HTTP status codes, using appropriate boundaries or status-pattern matching rather than bare substring checks. Preserve detection of the other provider error phrases and prevent unrelated numbers in messages or model identifiers from entering the provider branch.
62-78: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Compare the hardcoded hint list against provider catalogs and server env reads. rg -n '_API_KEY|_API_TOKEN|HF_TOKEN|CLOUDFLARE_ACCOUNT_ID' src server.ts --glob '!**/*.test.*' echo '--- provider catalog definitions ---' fd -t f -i 'aiprovidercatalog' src --exec cat -n {}Repository: tonythethompson/Olive-Studio
Length of output: 16793
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- ProviderErrorBlock outline ---' ast-grep outline src/components/features/assistant/ProviderErrorBlock.tsx || true echo '--- ProviderErrorBlock relevant lines ---' sed -n '1,120p' src/components/features/assistant/ProviderErrorBlock.tsx | cat -n echo '--- aiProviderCatalog imports/usages ---' rg -n "from ['\"].*aiProviderCatalog|PROVIDER_OPTIONS|normalizeUiProviderId|keyEnvVar" src/components src --glob '!**/*.test.*' || true echo '--- provider catalog server-side equivalent ---' rg -n "envVarNames|loadStudioEnv|AI_PROVIDERS|providers|openai-compat|copilot|codex|devin" src/server/routes src/server/services src/lib -g '!**/*.test.*' | head -n 200Repository: tonythethompson/Olive-Studio
Length of output: 35909
Keep this env hint tied to the provider catalog.
ProviderErrorBlockhardcodes twelve provider env vars, whilePROVIDER_OPTIONSalready haskeyEnvVarfor each supported provider. Generate this list from the catalog for the displayed providers, or add a synchronization check that fails when the hints drift fromPROVIDER_OPTIONS.🤖 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/components/features/assistant/ProviderErrorBlock.tsx` around lines 62 - 78, Update ProviderErrorBlock to derive the environment-variable hint from PROVIDER_OPTIONS.keyEnvVar for the displayed providers instead of maintaining the hardcoded list. Ensure each supported provider’s variable appears once in the rendered message and the catalog remains the single source of truth.Source: Coding guidelines
src/components/features/assistant/types.ts (1)
1-14: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the assistant analysis payload before rendering it.
The API payload is AI-generated and untrusted. A successful JSON response such as
{}reachessetAnalysisthrough a type assertion, thenAuditPanelreadsanalysis.suggestions.lengthand throws.
src/components/features/assistant/types.ts#L1-L14: makeSuggestion.autofixoptional.AuditPanelalready supports suggestions without an Apply action.src/components/features/assistant/useAiAudit.ts#L50-L57: validate the fullAnalysisResultshape before callingsetAnalysis. SetanalysisErrorwhen required fields are missing or malformed.📍 Affects 2 files
src/components/features/assistant/types.ts#L1-L14(this comment)src/components/features/assistant/useAiAudit.ts#L50-L57🤖 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/components/features/assistant/types.ts` around lines 1 - 14, Make Suggestion.autofix optional in src/components/features/assistant/types.ts (lines 1-14), preserving AuditPanel support for suggestions without Apply actions. In src/components/features/assistant/useAiAudit.ts (lines 50-57), validate the complete AnalysisResult payload—including score, level, summary, suggestions, and each suggestion’s required fields—before calling setAnalysis; set analysisError and do not update analysis when validation fails.src/components/features/assistant/useAiAudit.ts (1)
67-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate active requests when resetting analysis.
Line 68 clears only the displayed result. If a provider is cleared while an audit request is pending, the old response still passes the request-ID check and restores analysis from the cleared provider. Increment
analysisRequestIdRefinresetAnalysisand clear the pending loading and error state.🤖 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/components/features/assistant/useAiAudit.ts` around lines 67 - 68, Update resetAnalysis in useAiAudit to invalidate pending requests by incrementing analysisRequestIdRef, then clear the displayed analysis, loading state, and error state. Ensure responses from requests active before reset cannot restore analysis.src/components/features/assistant/useLocalEngineSetup.ts (2)
53-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
readNdjsonLinesleaks the stream reader on every error path.
handleInstallStreamEventandhandlePullStreamEventthrow onevt.type === "error". That exception propagates out ofonLine, out of thewhileloop, and out of this function. The reader is never released and the body is never cancelled, so the underlying response stays locked and undrained until GC. Engine install and model pull errors are routine, not exceptional, so this leaks on a normal user path.Wrap the loop and release the reader in
finally.🔒️ Proposed fix
async function readNdjsonLines( body: ReadableStream<Uint8Array>, onLine: (line: string) => void | Promise<void>, ) { const reader = body.getReader(); const decoder = new TextDecoder(); let buf = ""; - while (true) { - const { done, value } = await reader.read(); - if (done) break; - buf += decoder.decode(value, { stream: true }); - const lines = buf.split("\n"); - buf = lines.pop() ?? ""; - for (const line of lines) { - if (!line.trim()) continue; - await onLine(line); - } - } - return buf; + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + buf += decoder.decode(value, { stream: true }); + const lines = buf.split("\n"); + buf = lines.pop() ?? ""; + for (const line of lines) { + if (!line.trim()) continue; + await onLine(line); + } + } + return buf; + } finally { + // Release the body on the throw path too; install/pull error events throw. + await reader.cancel().catch(() => {}); + reader.releaseLock(); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async function readNdjsonLines( body: ReadableStream<Uint8Array>, onLine: (line: string) => void | Promise<void>, ) { const reader = body.getReader(); const decoder = new TextDecoder(); let buf = ""; try { while (true) { const { done, value } = await reader.read(); if (done) break; buf += decoder.decode(value, { stream: true }); const lines = buf.split("\n"); buf = lines.pop() ?? ""; for (const line of lines) { if (!line.trim()) continue; await onLine(line); } } return buf; } finally { // Release the body on the throw path too; install/pull error events throw. await reader.cancel().catch(() => {}); reader.releaseLock(); } }🤖 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/components/features/assistant/useLocalEngineSetup.ts` around lines 53 - 72, Update readNdjsonLines to wrap its stream-reading loop and line-processing in a try/finally block, and always release the reader in finally via its existing reader cleanup API. Preserve propagation of errors from handleInstallStreamEvent and handlePullStreamEvent while ensuring the underlying stream is unlocked and cancelled or otherwise cleaned up on every exit path.
236-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The install stream reports success after swallowing a real server error.
Line 242 rethrows only when the message is neither
"Setup failed"nor contains"JSON". Two problems follow.First, the substring test is a proxy for "this was a JSON parse failure". Any genuine server error whose text contains
JSON— for exampleLM Studio config JSON is malformed— is swallowed. Second, when an error is swallowed,state.okstays false, but ifresponse.okis true the guard at Line 247 passes,setLocalInstallInfo("Engine ready.")runs, andinstallEnginethen callsmarkEngineReady. The UI reports a healthy engine that never came up. That is fake readiness.Catch
SyntaxErrorexplicitly, the wayconsumePullStreamalready does at Line 350, and require an explicitdoneevent before declaring the engine ready.🐛 Proposed fix
await readNdjsonLines(body, (line) => { + let evt: InstallStreamEvent; try { - handleInstallStreamEvent(JSON.parse(line) as InstallStreamEvent, state); - } catch (e) { - if (e instanceof Error && e.message !== "Setup failed" && !e.message.includes("JSON")) { - throw e; - } + evt = JSON.parse(line) as InstallStreamEvent; + } catch { + // Non-JSON line from the server; ignore it and keep reading. + return; } + handleInstallStreamEvent(evt, state); }); - if (!state.ok && !response.ok) throw new Error(`Setup failed (HTTP ${response.status})`); + if (!state.ok) throw new Error(`Setup failed (HTTP ${response.status})`); setLocalInstallInfo(state.finalMsg);📝 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.const consumeInstallStream = async (body: ReadableStream<Uint8Array>, response: Response) => { const state = { openedUrl: undefined as string | undefined, ok: false, finalMsg: "Engine ready." }; await readNdjsonLines(body, (line) => { let evt: InstallStreamEvent; try { evt = JSON.parse(line) as InstallStreamEvent; } catch { // Non-JSON line from the server; ignore it and keep reading. return; } handleInstallStreamEvent(evt, state); }); if (!state.ok) throw new Error(`Setup failed (HTTP ${response.status})`); setLocalInstallInfo(state.finalMsg); };🤖 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/components/features/assistant/useLocalEngineSetup.ts` around lines 236 - 249, Update consumeInstallStream to swallow only JSON parsing errors by catching SyntaxError explicitly, while rethrowing other errors regardless of their message text. Require both a successful response and an explicit done event indicated by state.ok before calling setLocalInstallInfo or allowing installation readiness to proceed; preserve the existing Setup failed error for unsuccessful responses.src/components/features/assistant/aiProviderCatalog.ts (2)
15-15: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ProviderIdresolves tostring, so provider IDs are unchecked everywhere. The explicitreadonly ProviderOption[]annotation on the catalog overrides theas constliteral types. BecauseProviderOption.idisreadonly string,ProviderIdbecomesstringrather than the union of the 19 catalog IDs. Every consumer that expects a checked union gets none.
src/components/features/assistant/aiProviderCatalog.ts#L15-L15: remove the: readonly ProviderOption[]annotation and close the array with] as const satisfies readonly ProviderOption[];so the literal ID types survive.src/components/features/assistant/useAiProviderSettings.ts#L353-L358:PROVIDER_OPTIONS.find((p) => p.id === id)!cannot be proven non-null whileidisstring. After the type fix, keep the assertion only if the union guarantees a match; otherwise return early whenfindyieldsundefined, becauseopt.models[0]would throw on Line 358.src/components/features/assistant/ManualProviderSetup.tsx#L30-L30:e.target.value as ProviderIdcurrently asserts nothing. After the type fix, validate withnormalizeUiProviderId(e.target.value)and ignore anullresult instead of casting.📍 Affects 3 files
src/components/features/assistant/aiProviderCatalog.ts#L15-L15(this comment)src/components/features/assistant/useAiProviderSettings.ts#L353-L358src/components/features/assistant/ManualProviderSetup.tsx#L30-L30🤖 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/components/features/assistant/aiProviderCatalog.ts` at line 15, Preserve literal provider IDs by removing the explicit type annotation from PROVIDER_OPTIONS and using an as-const satisfies readonly ProviderOption[] declaration. In src/components/features/assistant/useAiProviderSettings.ts lines 353-358, retain the non-null assertion only if the resulting ProviderId union guarantees a match; otherwise handle an undefined find result before accessing opt.models[0]. In src/components/features/assistant/ManualProviderSetup.tsx line 30, replace the ProviderId cast with normalizeUiProviderId(e.target.value) and ignore null results.
216-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the re-export hop and match the
@/libimport style.These named re-exports turn
aiProviderCatalog.tsinto a barrel for local-engine helpers. Any module that wantsLMS_STARTER_MODELSnow also pulls in the full provider catalog. That works against the tree-shaking and component-test isolation goals in the project guidelines.The deep relative path with an explicit
.tsextension also differs from the@/lib/...alias used inSettingsPanel.tsx,ModelCombobox.tsx, andManualProviderSetup.tsx.Import
localEngineStartersdirectly at the consumer sites and remove this block.Run this script to size the change:
#!/bin/bash # Description: Find consumers that import local engine starters through the catalog barrel. rg -nP -C2 'LMS_STARTER_MODELS|OLLAMA_STARTER_MODELS|findInstalledStarterId|resolveLocalEnableModelId|LocalStarterModel' src || 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/components/features/assistant/aiProviderCatalog.ts` around lines 216 - 226, Remove the local-engine type and named re-export block from aiProviderCatalog.ts, then update every consumer identified by the search to import those symbols directly from the `@/lib/localEngineStarters` alias. Preserve each consumer’s existing usage and use the alias without a .ts extension.Source: Coding guidelines
src/components/features/assistant/CodexAccountPanel.tsx (1)
18-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Asynchronous status text in the assistant settings surface has no live regions. Every one of these elements replaces its content after a network request resolves, and none is announced. A screen-reader user starts sign-in, save, or logout and receives no confirmation or failure notice.
src/components/features/assistant/CodexAccountPanel.tsx#L18-L28: addrole="status"andaria-live="polite"to the Status paragraph and to thecodexMessageparagraph on Line 28.src/components/features/assistant/DevinAccountPanel.tsx#L18-L30: addrole="status"andaria-live="polite"to the Status paragraph and to thedevinMessageparagraph on Line 30.src/components/features/assistant/ManualProviderSetup.tsx#L297-L297: addrole="alert"to theproviderSaveErrorparagraph, because it reports a failure rather than progress.📍 Affects 3 files
src/components/features/assistant/CodexAccountPanel.tsx#L18-L28(this comment)src/components/features/assistant/DevinAccountPanel.tsx#L18-L30src/components/features/assistant/ManualProviderSetup.tsx#L297-L297🤖 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/components/features/assistant/CodexAccountPanel.tsx` around lines 18 - 28, In src/components/features/assistant/CodexAccountPanel.tsx lines 18-28, add role="status" and aria-live="polite" to the Status and codexMessage paragraphs; apply the same attributes to the corresponding Status and devinMessage paragraphs in src/components/features/assistant/DevinAccountPanel.tsx lines 18-30. In src/components/features/assistant/ManualProviderSetup.tsx line 297, add role="alert" to the providerSaveError paragraph.src/components/features/assistant/DevinAccountPanel.tsx (1)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Stale
gemini-prefix on the element IDs after the directory rename.This PR renames
gemini/toassistant/, but the DOM IDs still readgemini-settings-devin-token.ManualProviderSetup.tsxcarries the same prefix ongemini-settings-provider,gemini-settings-model,gemini-settings-api-key, andgemini-cf-account-id. The identifiers now name a provider that is one entry in a 19-provider catalog, not the feature.Rename the IDs to an
assistant-prefix and update any test selectors.🤖 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/components/features/assistant/DevinAccountPanel.tsx` around lines 59 - 63, Replace the stale gemini- DOM ID prefixes with assistant- in DevinAccountPanel’s token input and the corresponding provider setup IDs in ManualProviderSetup, including gemini-settings-devin-token, gemini-settings-provider, gemini-settings-model, gemini-settings-api-key, and gemini-cf-account-id. Update all tests and selectors that reference these IDs to use the renamed assistant- identifiers.src/components/features/assistant/LocalModelManager.tsx (1)
191-241: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Three
refresh()call sites bypass the cancellation guard that the effect adds.The mount effect at Line 179 passes
() => cancelGuard.cancelledso a late response cannot write state after unmount or after theengineprop changes.handleEnable(Line 213),handleUnload(Line 235), and the manual Refresh button (Line 257) callrefresh()with no guard. Those three can also overlap with an in-flight effect refresh, so two responses race and the later writer wins arbitrarily.Hold one guard in a ref and use it for every call.
♻️ Proposed refactor
+ // One cancellation token for every refresh, so late responses cannot win. + const refreshTokenRef = useRef(0); + const refresh = async (isCancelled?: () => boolean) => { + const token = ++refreshTokenRef.current; + const cancelled = () => isCancelled?.() === true || refreshTokenRef.current !== token;Then replace every
isCancelled?.()insiderefreshwithcancelled(), and keep the other call sites as plainrefresh().🤖 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/components/features/assistant/LocalModelManager.tsx` around lines 191 - 241, Centralize the cancellation guard in a ref used by refresh, replacing the optional isCancelled callback checks with the ref-backed cancelled function. Update the effect and the handleEnable, handleUnload, and manual Refresh button call sites to use the shared guard so all refresh responses are ignored after unmount or engine changes and overlapping requests cannot write stale state.src/components/features/assistant/ManualProviderSetup.tsx (3)
33-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check the TypeScript lib/target settings for Object.groupBy support (ES2024). fd -t f -g 'tsconfig*.json' -d 2 --exec cat {} \;Repository: tonythethompson/Olive-Studio
Length of output: 760
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== File outline ==" ast-grep outline src/components/features/assistant/ManualProviderSetup.tsx --view expanded || true echo echo "== ManualProviderSetup relevant lines ==" cat -n src/components/features/assistant/ManualProviderSetup.tsx | sed -n '1,180p' echo echo "== aiProviderCatalog category labels/options references ==" rg -n "CATEGORY_LABELS|PROVIDER_OPTIONS|optgroup|aiProviderCatalog|type .*Provider" src --glob '*.{ts,tsx}' echo echo "== Candidate catalog files ==" fd -t f 'aiProviderCatalog|provider' src | sed -n '1,80p'Repository: tonythethompson/Olive-Studio
Length of output: 17286
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== aiProviderCatalog category labels/options lines ==" cat -n src/components/features/assistant/aiProviderCatalog.ts | sed -n '1,235p' echo echo "== TypeScript compiler availability and lib definitions ==" if command -v tsc >/dev/null 2>&1; then echo "tsc found" tsc --version else echo "tsc not found" fi echo echo "== Read-only local source check for Object.groupBy availability in current tsconfig ==" python3 - <<'PY' import json, pathlib, re p = pathlib.Path("tsconfig.json") cfg = json.loads(p.read_text()) opts = cfg.get("compilerOptions", {}) print(json.dumps({"target": opts.get("target")}, indent=2)) # Check standard library declarations for Object.groupBy if `@types/core-js` or local libs exist. for path in pathlib.Path("node_modules").glob("`@types/core-js/`**/*.d.ts"): s = path.read_text(errors="ignore") if "groupBy" in s: print(f"core-js groupBy declaration found", path) break else: print("core-js Object.groupBy declaration not found in node_modules") PYRepository: tonythethompson/Olive-Studio
Length of output: 8886
Use
<optgroup>for provider categories.
CATEGORY_LABELSis documented as optgroup headers, but this select renders disabled placeholder<option value="">rows. Screen readers expose those as disabled options;<optgroup label>exposes the category as a group. Grouping also removes the emptyvalue=""option, so provider selection cannot produce an empty provider ID.Use a grouping pattern compatible with the current ES2022 target instead of
Object.groupBy.🤖 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/components/features/assistant/ManualProviderSetup.tsx` around lines 33 - 48, Replace the category-header `<option>` pattern in the provider select’s PROVIDER_OPTIONS rendering with `<optgroup>` elements labeled via CATEGORY_LABELS, grouping each category’s provider options under its corresponding group. Use an ES2022-compatible grouping approach rather than Object.groupBy, and ensure no empty value="" option is rendered.
125-138: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The select can display a model that does not match the saved state.
value={settingsModel}has no matching<option>when the saved model is absent fromdisplayedModels.fetchProviderStatussetssettingsModelfrom the server atuseAiProviderSettings.tsLine 275, and that value can be any string the server has stored. The browser then shows the first option while state still holds the unmatched value. Save writes the hidden state value, not the one on screen.Render the current value as an extra option when the catalog does not contain it.
🐛 Proposed fix
return ( <select id="gemini-settings-model" aria-label="AI model" value={settingsModel} onChange={(e) => providers.setSettingsModel(e.target.value)} className="w-full bg-slate-900 border border-slate-700 rounded-lg px-3 py-2 text-sm text-slate-200 focus:outline-none focus:border-electric-blue cursor-pointer" > + {settingsModel && !displayedModels.some((m) => m.id === settingsModel) && ( + <option value={settingsModel}>{settingsModel}</option> + )} {displayedModels.map((m) => (📝 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.return ( <select id="gemini-settings-model" aria-label="AI model" value={settingsModel} onChange={(e) => providers.setSettingsModel(e.target.value)} className="w-full bg-slate-900 border border-slate-700 rounded-lg px-3 py-2 text-sm text-slate-200 focus:outline-none focus:border-electric-blue cursor-pointer" > {settingsModel && !displayedModels.some((m) => m.id === settingsModel) && ( <option value={settingsModel}>{settingsModel}</option> )} {displayedModels.map((m) => ( <option key={m.id} value={m.id}> {m.label} </option> ))} </select>🤖 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/components/features/assistant/ManualProviderSetup.tsx` around lines 125 - 138, Update the model select rendering in the component containing displayedModels so settingsModel is also rendered as an option when displayedModels does not contain that id, preserving its label/value sufficiently to keep the controlled select synchronized with saved state. Avoid adding a duplicate option when the catalog already includes settingsModel, and keep the existing displayedModels options unchanged.
178-180: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Locate the Cloudflare manual-login route and check whether it writes credentials to disk. rg -n -C10 'cloudflare/login/manual' --glob '!node_modules' . || true fd -t f -i 'cloudflare' src/server --exec rg -n -C5 'writeFile|writeFileSync|persist|keytar|store|fs\.' {} \;Repository: tonythethompson/Olive-Studio
Length of output: 7296
🏁 Script executed:
#!/bin/bash set -u echo "== cloudflareRoutes.ts relevant line ranges ==" wc -l src/server/routes/ai/cloudflareRoutes.ts sed -n '53,110p' src/server/routes/ai/cloudflareRoutes.ts echo echo "== ManualProviderSetup.tsx relevant lines ==" wc -l src/components/features/assistant/ManualProviderSetup.tsx sed -n '160,225p' src/components/features/assistant/ManualProviderSetup.tsx echo echo "== useAiProviderSettings.ts persistApiKeyProvider and keyPlaceholder references ==" sed -n '60,90p' src/components/features/assistant/useAiProviderSettings.ts echo echo "== saveManualCloudflareCredentials definitions/usages ==" rg -n -C8 "function saveManualCloudflareCredentials|saveManualCloudflareCredentials|persisting|persist|password" src/server src/components/features/assistant || trueRepository: tonythethompson/Olive-Studio
Length of output: 50384
🏁 Script executed:
#!/bin/bash set -u echo "== locate cloudflare client credentials files ==" fd -t f -i 'client|credentials' src/lib src/server/lib src/components/features/assistant src/server/services/ai/state.ts || true echo echo "== inspect candidate Cloudflare credential implementation files ==" for f in $(fd -t f -i 'client|credentials' src/lib src/server/lib src/components/features/assistant src/server/services/ai/state.ts 2>/dev/null | grep -E 'cloudflare|credentials|cloudflare/client|cloudflare/credentials'); do echo "--- $f ---" wc -l "$f" sed -n '1,260p' "$f" done echo echo "== focused search for credential storage helpers and Cloudflare fields ==" rg -n -C6 "saveManualCloudflareCredentials|logoutCloudflare|getCloudflareAccountStatus|writeFileSync|writeFile|fs\\.|credentials" src/lib src/server/services/aiRepository: tonythethompson/Olive-Studio
Length of output: 50385
Update the Cloudflare API-key placeholder before saving
/api/cloudflare/login/manualstores the Cloudflare token and account locally viasaveManualCloudflareCredentials, but the component placeholder says “Stored in memory only, never persisted to disk” when there is no usable environment credential. Use a Cloudflare-specific placeholder or a generic prompt that explicitly describes on-disk credential storage.🤖 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/components/features/assistant/ManualProviderSetup.tsx` around lines 178 - 180, The keyPlaceholder in ManualProviderSetup should accurately describe Cloudflare credentials being persisted by saveManualCloudflareCredentials when no usable environment credential exists. Replace the memory-only text with a Cloudflare-specific or generic prompt that explicitly states the API key is stored on disk, while preserving the existing environment-variable placeholder.src/components/features/assistant/ModelCombobox.test.tsx (1)
63-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the successful selection paths.
These tests verify that
Enterdoes not commit. No test verifies thatEnterdoes commit afterArrowDownsets an explicit highlight, and no test verifies mouse selection throughonMouseDown. Those are the primary paths of the component. A regression that disables selection entirely would still pass this suite.Add two cases: arrow-down then
EntercallsonChangewith the highlighted ID, andmouseDownon an option callsonChangeand closes the list.💚 Proposed additional tests
+ it("commits the highlighted model on ArrowDown + Enter", () => { + const onChange = vi.fn(); + render( + <ModelCombobox + value="" + options={options} + modelsSource="live" + onChange={onChange} + />, + ); + + const input = screen.getByRole("combobox"); + fireEvent.focus(input); + fireEvent.keyDown(input, { key: "ArrowDown" }); + fireEvent.keyDown(input, { key: "Enter" }); + expect(onChange).toHaveBeenCalledWith("openrouter/google/gemini-2.5-pro"); + expect(screen.queryByRole("listbox")).toBeNull(); + }); + + it("commits the clicked model and closes the list", () => { + const onChange = vi.fn(); + render( + <ModelCombobox + value="" + options={options} + modelsSource="live" + onChange={onChange} + />, + ); + + fireEvent.focus(screen.getByRole("combobox")); + fireEvent.mouseDown(screen.getByRole("option", { name: /GPT-4o/ })); + expect(onChange).toHaveBeenCalledWith("openrouter/openai/gpt-4o"); + expect(screen.queryByRole("listbox")).toBeNull(); + });📝 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.it("commits the highlighted model on ArrowDown + Enter", () => { const onChange = vi.fn(); render( <ModelCombobox value="" options={options} modelsSource="live" onChange={onChange} />, ); const input = screen.getByRole("combobox"); fireEvent.focus(input); fireEvent.keyDown(input, { key: "ArrowDown" }); fireEvent.keyDown(input, { key: "Enter" }); expect(onChange).toHaveBeenCalledWith("openrouter/google/gemini-2.5-pro"); expect(screen.queryByRole("listbox")).toBeNull(); }); it("commits the clicked model and closes the list", () => { const onChange = vi.fn(); render( <ModelCombobox value="" options={options} modelsSource="live" onChange={onChange} />, ); fireEvent.focus(screen.getByRole("combobox")); fireEvent.mouseDown(screen.getByRole("option", { name: /GPT-4o/ })); expect(onChange).toHaveBeenCalledWith("openrouter/openai/gpt-4o"); expect(screen.queryByRole("listbox")).toBeNull(); }); it("does not commit a different model on focus + Enter for freehand ids", () => { const onChange = vi.fn(); render( <ModelCombobox value="my-org/custom-model" options={options} modelsSource="live" onChange={onChange} />, ); const input = screen.getByRole("combobox"); fireEvent.focus(input); fireEvent.keyDown(input, { key: "Enter" }); expect(onChange).not.toHaveBeenCalled(); }); it("does not commit a different model on focus + Enter when selection is past the visible window", () => { const many = Array.from({ length: 45 }, (_, i) => ({ id: `model-${i}`, label: `Model ${i}`, })); const onChange = vi.fn(); render( <ModelCombobox value="model-42" options={many} modelsSource="live" onChange={onChange} />, ); const input = screen.getByRole("combobox"); fireEvent.focus(input); fireEvent.keyDown(input, { key: "Enter" }); expect(onChange).not.toHaveBeenCalled(); }); it("does not overwrite a freehand id with the first filter match on Enter", () => { const onChange = vi.fn(); render( <ModelCombobox value="" options={[ { id: "openai/gpt-4o", label: "GPT-4o" }, { id: "openai/gpt-4o-mini", label: "GPT-4o mini" }, ]} modelsSource="live" onChange={onChange} />, ); const input = screen.getByRole("combobox"); fireEvent.focus(input); fireEvent.change(input, { target: { value: "openai/gpt-4o-my-ft" } }); expect(onChange).toHaveBeenLastCalledWith("openai/gpt-4o-my-ft"); onChange.mockClear(); fireEvent.keyDown(input, { key: "Enter" }); expect(onChange).not.toHaveBeenCalled(); expect((input as HTMLInputElement).value).toBe("openai/gpt-4o-my-ft"); });🤖 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/components/features/assistant/ModelCombobox.test.tsx` around lines 63 - 119, Add two successful-selection tests for ModelCombobox: verify that focusing the input, using ArrowDown to create an explicit highlight, and pressing Enter calls onChange with the highlighted option ID; and verify that firing mouseDown on an option calls onChange with that option’s ID and closes the list. Use the component’s existing option rendering and observable list visibility behavior.src/components/features/assistant/ModelCombobox.tsx (3)
71-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check the exhaustive-deps rule configuration and the eslint-plugin-react-hooks version. fd -t f -i 'eslint.config*' -d 2 --exec cat {} \; rg -n 'eslint-plugin-react-hooks' package.json || trueRepository: tonythethompson/Olive-Studio
Length of output: 3794
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the surrounding implementation and search for eslint-disable patterns/effects in this file. printf '--- ModelCombobox.tsx relevant section ---\n' sed -n '1,130p' src/components/features/assistant/ModelCombobox.tsx | cat -n printf '\n--- eslint-disable/react-hooks/exhaustive-deps in ModelCombobox.tsx ---\n' rg -n 'eslint-disable|react-hooks/exhaustive-deps|intentionally|closeList' src/components/features/assistant/ModelCombobox.tsx || trueRepository: tonythethompson/Olive-Studio
Length of output: 257
🏁 Script executed:
#!/bin/bash set -euоe pipefail echo '--- ModelCombobox.tsx relevant section ---' sed -n '1,130p' src/components/features/assistant/ModelCombobox.tsx | cat -n echo echo '--- eslint-disable/react-hooks/exhaustive-deps in ModelCombobox.tsx ---' rg -n 'eslint-disable|react-hooks/exhaustive-deps|intentionally|closeList' src/components/features/assistant/ModelCombobox.tsx || trueRepository: tonythethompson/Olive-Studio
Length of output: 5480
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- package versions ---' node - <<'JS' const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); for (const key of ['dependencies','devDependencies']) { if (pkg[key]) { for (const name of ['react', 'eslint', 'eslint-plugin-react-hooks', '`@typescript-eslint/eslint-plugin`']) { if (pkg[key][name]) console.log(`${key}.${name}: ${pkg[key][name]}`); } } } JS echo echo '--- ModelCombobox.tsx imports and effects ---' sed -n '1,25p' src/components/features/assistant/ModelCombobox.tsx | cat -n sed -n '65,82p' src/components/features/assistant/ModelCombobox.tsx | cat -nRepository: tonythethompson/Olive-Studio
Length of output: 1804
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- package versions ---' node - <<'JS' const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); for (const key of ['dependencies','devDependencies']) { if (pkg[key]) { for (const name of ['react', 'eslint', 'eslint-plugin-react-hooks', '`@typescript-eslint/eslint-plugin`']) { if (pkg[key][name]) console.log(`${key}.${name}: ${pkg[key][name]}`); } } } JS echo echo '--- ModelCombobox.tsx imports and effects ---' sed -n '1,25p' src/components/features/assistant/ModelCombobox.tsx | cat -n sed -n ' 65,82p' src/components/features/assistant/ModelCombobox.tsx | cat -nRepository: tonythethompson/Olive-Studio
Length of output: 1804
🌐 Web query:
eslint-plugin-react-hooks exhaustive-deps missing dependency function callbacks stable setters documentation💡 Result:
In eslint-plugin-react-hooks, the exhaustive-deps rule flags “missing dependency” errors for values (including functions) referenced inside an Effect, because the dependency array is how React knows when to re-run the Effect with the latest values—avoiding stale closures [1]. 1) Why function callbacks are usually required - If a function you reference inside useEffect (often a prop callback or a handler defined in the component) is in the component scope, exhaustive-deps expects it to appear in the dependency list; otherwise, React won’t re-run the Effect when that function changes, potentially using stale values [1]. 2) “But adding the function makes an infinite loop” (common cause + fix) - exhaustive-deps explicitly warns about patterns that add function dependencies and can cause loops, and recommends making the function stable or restructuring the code [1]. - Recommended fixes documented by React: a) Wrap the function in useCallback so its identity is stable between renders (based on a real set of primitive dependencies), then depend on that memoized function [1]. b) Or move the logic directly into the Effect and depend on the state/props it actually reads [1]. 3) How the linter decides whether a dependency can be omitted (stable setters) - React guarantees that the setState function identity (the setter returned from useState) is stable and won’t change across re-renders, so it is safe to omit from Effect dependencies [2]. - The exhaustive-deps rule also treats setState/setReducer dispatch functions as special cases (they are considered stable), which prevents it from incorrectly requiring them in deps in typical usage [3]. 4) What to do when you think something is truly unnecessary - React’s guidance is: do not suppress exhaustive-deps (for example via eslint-ignore-next-line). When dependencies don’t match the code, there’s high risk of bugs, because you “lie” to React about what the Effect depends on [4][5]. - Instead, “prove” a dependency is unnecessary by changing the code (e.g., moving values/functions so they’re not reactive dependencies, or using Effect Events where appropriate) [4]. Practical summary: - Props/state variables you read in an Effect: must be listed (or you must restructure code to make them unnecessary) [1][5]. - Functions referenced from component scope: typically must be listed; if that causes a loop, stabilize the function with useCallback or move the logic into the Effect [1]. - Stable setters (setState / dispatch): generally can be omitted because React guarantees stable identity [2][3].
Citations:
- 1: https://react.dev/reference/eslint-plugin-react-hooks/lints/exhaustive-deps
- 2: https://react.dev/reference/react/useState
- 3: react/react#16281
- 4: https://react.dev/learn/removing-effect-dependencies
- 5: https://react.dev/reference/react/useEffect
Stabilize
closeListto satisfyreact-hooks/exhaustive-deps.
closeListis redefined on every render and is referenced insideuseEffect, so this generates the project’s lint warning. WrapcloseListinuseCallbackand adduseCallbackto the React imports;setOpen,setQuery, andsetHighlightdo not need to be added because they are stable state setters.🤖 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/components/features/assistant/ModelCombobox.tsx` around lines 71 - 80, Wrap closeList in useCallback and import useCallback from React, preserving its existing behavior and dependencies on stable state setters. Update the useEffect dependency array to include the stabilized closeList reference alongside open.Source: Coding guidelines
101-131: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Arrow-key navigation moves the highlight off-screen.
The listbox is
max-h-48 overflow-autoand renders up to 40 options. Roughly six options are visible at a time.ArrowDownadvanceshighlightbut nothing scrolls the active option into view. After about six presses the user navigates blind, andEntercommits an option they cannot see.Add a ref to the active option and scroll it into view when
safeHighlightchanges.♿ Proposed fix to keep the active option visible
Add the ref and the effect near the other hooks:
const rootRef = useRef<HTMLDivElement>(null); + const activeOptionRef = useRef<HTMLLIElement>(null);+ useEffect(() => { + if (!open || safeHighlight === null) return; + activeOptionRef.current?.scrollIntoView({ block: "nearest" }); + }, [open, safeHighlight]);Attach the ref to the active option in the list at Line 190:
<li key={m.id} + ref={active ? activeOptionRef : undefined} id={`${listboxId}-opt-${index}`}🤖 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/components/features/assistant/ModelCombobox.tsx` around lines 101 - 131, Add a ref for the highlighted option and an effect keyed to safeHighlight that scrolls the active element into view when keyboard navigation changes it. Attach the ref to the option rendered by the list when its index matches safeHighlight, while preserving existing navigation and rendering behavior.
181-184: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Non-option
<li>children break the listbox semantics.
role="listbox"requires its owned children to berole="option". The empty-state message at Lines 181-184 and the two truncation notices at Lines 214-223 are plain<li>elements inside the<ul role="listbox">. Assistive technology reports an inconsistent option count, and some screen readers skip the message entirely.Mark them
role="presentation", or move them outside the<ul>.♿ Proposed fix for the listbox children
{filtered.length === 0 ? ( - <li className="px-3 py-2 text-sm text-slate-500"> + <li role="presentation" className="px-3 py-2 text-sm text-slate-500"> No catalog matches. Keep typing to use a freehand model id. </li>{!filterText && options.length > MAX_VISIBLE && ( - <li className="px-3 py-1.5 text-[11px] text-slate-500 border-t border-slate-800"> + <li role="presentation" className="px-3 py-1.5 text-[11px] text-slate-500 border-t border-slate-800"> Showing first {MAX_VISIBLE} models. Type to search the full catalog. </li> )} {filterText && options.length > MAX_VISIBLE && filtered.length === MAX_VISIBLE && ( - <li className="px-3 py-1.5 text-[11px] text-slate-500 border-t border-slate-800"> + <li role="presentation" className="px-3 py-1.5 text-[11px] text-slate-500 border-t border-slate-800"> Showing first {MAX_VISIBLE} matches. Type to narrow further. </li> )}Also applies to: 214-223
🤖 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/components/features/assistant/ModelCombobox.tsx` around lines 181 - 184, Update the non-option list items in ModelCombobox, including the empty-state message and both truncation notices, to use role="presentation" while keeping actual model entries as role="option" within the listbox.src/components/features/assistant/ProviderErrorBlock.test.tsx (1)
69-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a case where
kindmust win over the message heuristics.
ProviderErrorBlockcomputesisProviderErronly whenisJsonModelErris false. No test pins that precedence. If someone later reorders the two checks, the suite still passes while a structured parser error gets rendered as "No AI Provider Configured".💚 Proposed test
it("shows model JSON guidance only when kind is structured", () => {it("prefers the structured kind over provider message heuristics", () => { render( <ProviderErrorBlock msg="401 Unauthorized — check your API key" kind="invalid_model_json" onGoSettings={vi.fn()} />, ); expect(screen.getByText("Model returned invalid JSON")).toBeDefined(); expect(screen.queryByText("No AI Provider Configured")).toBeNull(); });🤖 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/components/features/assistant/ProviderErrorBlock.test.tsx` around lines 69 - 79, Add a test in the ProviderErrorBlock test suite covering a provider-like message paired with kind="invalid_model_json". Assert that the structured error renders "Model returned invalid JSON" and that "No AI Provider Configured" is absent, pinning structured kind precedence over message heuristics.src/components/features/assistant/SettingsPanel.tsx (2)
26-29: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the provider display-name lookup into the catalog.
This exact expression is duplicated in
src/components/features/assistant/AssistantSidebar.tsx, where it buildsproviderLabel. Both sites normalize the ID, searchPROVIDER_OPTIONS, and fall back to the raw provider string. Any change to the alias rules must be applied in two places.Add a
providerDisplayNamehelper toaiProviderCatalog.tsand call it from both sites.♻️ Proposed helper and call site
Add to
src/components/features/assistant/aiProviderCatalog.ts:/** Catalog display name for a provider id, with legacy alias handling. */ export function providerDisplayName(provider: string | undefined): string { if (!provider) return ""; const id = normalizeUiProviderId(provider) ?? provider; return PROVIDER_OPTIONS.find((p) => p.id === id)?.name ?? provider; }Then in this file:
- const providerName = - PROVIDER_OPTIONS.find( - (p) => p.id === (normalizeUiProviderId(providerStatus.provider ?? "") ?? providerStatus.provider), - )?.name ?? providerStatus.provider; + const providerName = providerDisplayName(providerStatus.provider);🤖 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/components/features/assistant/SettingsPanel.tsx` around lines 26 - 29, Extract the duplicated provider label logic into a providerDisplayName helper in aiProviderCatalog.ts, accepting an optional provider, applying normalizeUiProviderId, looking up PROVIDER_OPTIONS, and falling back to the raw value or an empty string. Replace the local expression in SettingsPanel and the providerLabel expression in AssistantSidebar with this helper, preserving the existing display behavior.Source: Coding guidelines
120-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The effect discards the user's manual tab selection on any provider-status refresh.
setSettingsMode(next)runs unconditionally wheneverproviderStatus.provider,baseUrl, orsourcechanges. Several actions refresh provider status without the user changing tabs:handleCodexLogout,handleDevinLogout, andclearProviderall callfetchProviderStatus().deriveAssistantSettingsModereturns"cloud"for every state except a localopenai-compatbase URL. A user who is reading the Local tab is therefore moved to Cloud when an unrelated logout resolves.Track the last derived value and only overwrite the mode when the derivation actually changes.
🐛 Proposed fix to preserve the manual selection
+ const lastDerivedModeRef = useRef<AssistantSettingsMode | null>(null); + useEffect(() => { const next = deriveAssistantSettingsMode( providers.providerStatus.provider ?? providers.settingsProvider, providers.settingsBaseUrl || providers.providerStatus.baseUrl, ); - // eslint-disable-next-line react-hooks/set-state-in-effect - setSettingsMode(next); + if (lastDerivedModeRef.current !== next) { + lastDerivedModeRef.current = next; + // eslint-disable-next-line react-hooks/set-state-in-effect + setSettingsMode(next); + } const engine = preferredEngineFromBaseUrl(providers.settingsBaseUrl || providers.providerStatus.baseUrl);Add
useRefto the React import on Line 1.📝 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.const lastDerivedModeRef = useRef<AssistantSettingsMode | null>(null); useEffect(() => { const next = deriveAssistantSettingsMode( providers.providerStatus.provider ?? providers.settingsProvider, providers.settingsBaseUrl || providers.providerStatus.baseUrl, ); if (lastDerivedModeRef.current !== next) { lastDerivedModeRef.current = next; // eslint-disable-next-line react-hooks/set-state-in-effect setSettingsMode(next); } const engine = preferredEngineFromBaseUrl(providers.settingsBaseUrl || providers.providerStatus.baseUrl); if (engine && engine !== local.preferredEngine) { local.selectPreferredEngine(engine); } // Only re-derive when the active provider identity changes // eslint-disable-next-line react-hooks/exhaustive-deps }, [providers.providerStatus.provider, providers.providerStatus.baseUrl, providers.providerStatus.source]);🤖 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/components/features/assistant/SettingsPanel.tsx` around lines 120 - 133, Update the SettingsPanel effect around deriveAssistantSettingsMode to track the previously derived mode with a useRef, and only call setSettingsMode when the newly derived value differs from that stored value. Update the ref whenever derivation changes so provider-status refreshes do not overwrite a user-selected tab.src/components/features/assistant/useAiProviderSettings.ts (8)
66-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the provider-persistence requests.
Neither
fetchcall passes anAbortSignal. If the local Express server stops responding, both promises hang.saveApiKeyProviderkeepsisSavingProvidertrue because itsfinallynever runs, and the "Save & Activate" button stays disabled with a spinning icon until the page reloads. The same gap applies to everyfetchin this hook, including the Codex poll loop andclearProvider.Add
signal: AbortSignal.timeout(...)to these requests. Extract a smallpostJsonhelper so all call sites in this hook share one timeout policy.🛡️ Proposed fix for the two requests
+const REQUEST_TIMEOUT_MS = 20_000; + async function persistApiKeyProvider(input: {const credRes = await fetch("/api/cloudflare/login/manual", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ apiToken: input.key, accountId: input.cloudflareAccountId }), + signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), });const r = await fetch("/api/ai/provider", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ provider: input.settingsProvider, apiKey: input.key || undefined, model: input.model, baseUrl: input.resolvedBaseUrl, }), + signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), });🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 66 - 96, Introduce a shared postJson helper in the useAiProviderSettings hook that applies a consistent AbortSignal.timeout policy, then route both fetch calls in persistApiKeyProvider through it. Update every other fetch in the hook, including the Codex poll loop and clearProvider, to use the same helper or timeout signal so no request can hang indefinitely while preserving each request’s existing method, headers, body, and response handling.
111-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Split the Codex and Devin authentication state out of this hook.
This hook now owns three unrelated lifecycles: provider settings and the model catalog, the Codex OAuth flow, and the Devin token flow. The state block declares
codexAccount,codexBusy,codexMessage,devinStatus,devinToken,devinModels,devinBusy, anddevinMessagealongside the provider form. The file is roughly 680 lines and the returned object exposes 45 keys.
CodexAccountPanelneeds three of those keys.DevinAccountPanelneeds five. Both receive the entire object.Extract
useCodexAccountanduseDevinAccountinto sibling modules and compose them here. The project guidelines call for feature folders with colocated hooks.🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 111 - 154, Extract the Codex authentication lifecycle into a colocated useCodexAccount hook and the Devin token lifecycle into a colocated useDevinAccount hook, moving their state and related operations out of the main provider-settings hook. Compose both hooks from useAiProviderSettings and preserve their existing behavior and returned values, while keeping provider form and model-catalog state in the parent. Update CodexAccountPanel and DevinAccountPanel to consume only their respective hook results rather than the entire provider-settings object.Source: Coding guidelines
172-189: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Guard
applyFetchedModelsagainst an empty list instead of asserting.The three
models[0]!.idassertions are safe only because the single caller checksmodels.length > 0at Line 200. The function itself accepts any array. A future second call site would produce aTypeErroronundefined.id, and the!removes the compile-time signal that would have caught it.Add an early return.
♻️ Proposed guard
const applyFetchedModels = (providerId: ProviderId, models: Array<{ id: string; label: string }>) => { + const first = models[0]; + if (!first) return; setLiveModelsByProvider((prev) => ({ ...prev, [providerId]: models }));Then replace each
models[0]!.idwithfirst.id.📝 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.const applyFetchedModels = (providerId: ProviderId, models: Array<{ id: string; label: string }>) => { const first = models[0]; if (!first) return; setLiveModelsByProvider((prev) => ({ ...prev, [providerId]: models })); if (providerId === "devin") { setDevinModels(models.map((m) => ({ id: m.id, name: m.label }))); } // Keep known selections. Preserve freehand only after an explicit UI/saved choice. setSettingsModel((current) => { if (models.some((m) => m.id === current)) return current; if (userModelOverrideRef.current && current.trim()) return current; return first.id; }); setCustomModel((current) => { if (!current) return first.id; if (models.some((m) => m.id === current)) return current; if (userModelOverrideRef.current) return current; return first.id; }); };🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 172 - 189, Update applyFetchedModels to return immediately when models is empty, then assign the first model to a local first value after the guard and replace every models[0]!.id access with first.id. Preserve the existing state updates and selection logic for non-empty model lists.
199-201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not infer
"live"when the server omitssource.Line 201 falls back to
"live"whenever the response contains models but nosourcefield. That claims a confidence the server never sent.
modelCatalogMembershipLabelinsrc/lib/modelCatalogMembership.tsonly warns whensource === "live". It deliberately stays silent for"fallback"because static lists are small. An absentsourcetherefore makesModelComboboxtell the user "Model ID not recognized. Requests may fail." for a perfectly valid freehand ID.Default the unknown case to
"fallback".🐛 Proposed fix
const models = Array.isArray(data.models) ? data.models : []; if (models.length > 0) applyFetchedModels(providerId, models); - setModelsSource(data.source ?? (models.length > 0 ? "live" : "fallback")); + // An absent `source` is not evidence of a live catalog; stay silent rather than warn. + setModelsSource(data.source ?? "fallback");📝 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.const models = Array.isArray(data.models) ? data.models : []; if (models.length > 0) applyFetchedModels(providerId, models); // An absent `source` is not evidence of a live catalog; stay silent rather than warn. setModelsSource(data.source ?? "fallback");🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 199 - 201, Update the setModelsSource call in the fetch response handling to default an omitted data.source to "fallback" rather than inferring "live" from models.length. Preserve explicit server-provided sources and the existing model application behavior.
218-252: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A non-OK model response is cached as a successful fetch and never retried.
The function never checks
r.ok. If/api/ai/modelsreturns HTTP 500 with a JSON error body,r.json()resolves, no error is thrown, andapplyModelsResponseruns withmodelsundefined. It sets the source to"fallback"and returns.Line 220 already added the provider to
modelsFetchedRefbefore the request. Thecatchblock at Line 252 is the only place that removes it, and it never runs. The auto-refresh at Line 338 therefore skips this provider for the rest of the session. The user is stuck on the static fallback list with no explanation until they click Refresh manually.
fetchProviderStatusat Line 262 checksr.ok. Apply the same check here.🐛 Proposed fix
const data = (await r.json()) as { models?: Array<{ id: string; label: string }>; source?: "live" | "fallback"; error?: string; }; if (isStaleRefresh(currentSequence)) return; + if (!r.ok) { + // Allow a retry on the next selection instead of caching the failure. + modelsFetchedRef.current.delete(providerId); + setModelsSource("fallback"); + setModelsHint(data.error || `HTTP ${r.status}`); + return; + } applyModelsResponse(providerId, data);Note on the static analysis hint for Line 254: the
setModelsLoading(false)reset is already inside afinallyblock. That rule reports a false positive here.📝 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.if (!opts?.force && modelsFetchedRef.current.has(providerId)) return; modelsFetchedRef.current.add(providerId); refreshSequenceRef.current += 1; const currentSequence = refreshSequenceRef.current; setModelsLoading(true); setModelsHint(null); const body: { provider: string; apiKey?: string; baseUrl?: string } = { provider: providerId, }; const key = opts?.apiKey?.trim(); const base = opts?.baseUrl?.trim(); if (key) body.apiKey = key; if (base) body.baseUrl = base; try { const r = await fetch("/api/ai/models", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), }); const data = (await r.json()) as { models?: Array<{ id: string; label: string }>; source?: "live" | "fallback"; error?: string; }; if (isStaleRefresh(currentSequence)) return; if (!r.ok) { // Allow a retry on the next selection instead of caching the failure. modelsFetchedRef.current.delete(providerId); setModelsSource("fallback"); setModelsHint(data.error || `HTTP ${r.status}`); return; } applyModelsResponse(providerId, data); } catch (err: unknown) { if (isStaleRefresh(currentSequence)) return; setModelsSource("fallback"); setModelsHint(err instanceof Error ? err.message : "Could not refresh models"); // Allow retry on next selection if network failed entirely modelsFetchedRef.current.delete(providerId);🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 218 - 252, Update the fetch flow in the model-refresh function to check r.ok after the /api/ai/models request, matching fetchProviderStatus. Treat non-OK responses as errors so execution reaches the existing catch block, removes providerId from modelsFetchedRef, and exposes the response error rather than caching the fallback as successful.Source: Linters/SAST tools
323-340: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a cancellation flag to both open-time effects.
Neither effect returns a cleanup, and both write state after an
await.The first effect depends on
[isOpen]. If the user toggles the sidebar closed and open again quickly, twofetchProviderStatuscalls are in flight. Both callsetProviderStatus. The older response can resolve last and overwrite the newer one.onProviderMissing()can also fire for a stale result and force the tab to Settings.The second effect depends on
[isOpen, activeTab, settingsProvider].refreshCodexAccountandrefreshDevinAccountwritecodexAccountanddevinStatusafter anawaitwith no sequence guard.refreshProviderModelsis protected byrefreshSequenceRef; these two are not.Guard the post-await writes with an ignore flag returned from each effect.
🛡️ Proposed fix for the first effect
useEffect(() => { if (!isOpen) return; + let ignore = false; // eslint-disable-next-line react-hooks/set-state-in-effect -- intentional: load status on open fetchProviderStatus().then((status) => { + if (ignore) return; if (status.source === "none") onProviderMissing(); }); + return () => { + ignore = true; + }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [isOpen]);Note that
fetchProviderStatusalso needs the flag, because it callssetProviderStatusinternally. Pass the flag in, or move the ownership of that write into the effect.🧰 Tools
🪛 React Doctor (0.9.3)
[warning] 323-323: fetch() inside useEffect can race, double-fire, or leak. Use a data-fetching layer or Server Component instead.
Use a data-fetching layer or Server Component so fetches do not race, double-fire, or leak from
useEffect.(no-fetch-in-effect)
[error] 323-323: This setter runs after
await, so overlapping re-runs of the effect can resolve out of order and write stale state; gate it behind a cancellation/ignore flag or return a cleanup that cancels the work.In a
useEffectwhose dependencies can change, guard any setter call that runs after anawaitbehind a cancellation/ignore flag, or return a cleanup that cancels the async work.(no-set-state-after-await-in-effect)
[warning] 333-333: fetch() inside useEffect can race, double-fire, or leak. Use a data-fetching layer or Server Component instead.
Use a data-fetching layer or Server Component so fetches do not race, double-fire, or leak from
useEffect.(no-fetch-in-effect)
[error] 333-333: This setter runs after
await, so overlapping re-runs of the effect can resolve out of order and write stale state; gate it behind a cancellation/ignore flag or return a cleanup that cancels the work.In a
useEffectwhose dependencies can change, guard any setter call that runs after anawaitbehind a cancellation/ignore flag, or return a cleanup that cancels the async work.(no-set-state-after-await-in-effect)
🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 323 - 340, Update both open-time useEffect callbacks around fetchProviderStatus, refreshCodexAccount, and refreshDevinAccount to create an ignore/cancellation flag, guard every post-await state update and onProviderMissing invocation with it, and return cleanup that sets the flag when dependencies change or the effect unmounts. Ensure fetchProviderStatus receives or otherwise honors the flag for its internal setProviderStatus write, while preserving the existing refreshProviderModels sequence protection.Source: Linters/SAST tools
419-421: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The Codex poll loop runs for 30 seconds and cannot be cancelled.
The loop makes up to 12 requests spaced 2500 ms apart.
codexBusystays true for the whole 30 seconds, so the Sign in, Refresh, and Logout buttons inCodexAccountPanelare all disabled. The user has no way to abort.The loop also has no unmount or
isOpencheck. If the user closes the sidebar after one second, the remaining 11 requests still fire and still write state.Add an abort mechanism: store a ref that the loop checks each iteration, set it on unmount, and expose a cancel action to the panel.
Also applies to: 436-441
🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 419 - 421, Update the Codex polling flow around the loop in useAiProviderSettings to support cancellation: store a cancellation ref, check it before each delayed poll/request, set it during unmount and when the relevant panel is no longer open, and stop further state updates or requests once cancelled. Expose a cancel action from the hook for CodexAccountPanel so Sign in, Refresh, and Logout can abort the active poll and clear codexBusy.
423-435: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provider-mutation responses are never checked, so the UI reports success it did not verify. Three call sites send a provider write to
/api/ai/providerand ignore the outcome. Each one then advances the UI as if the write succeeded. Every other request in this hook that gates UI state checksr.ok; these do not.
src/components/features/assistant/useAiProviderSettings.ts#L423-L435: capture the activation response, throw when!r.ok, and move the "Codex is active for audit/chat" message and theonProviderActivated()call to after the check. Today a failed POST still switches to the Audit tab and starts an analysis against an unset provider.src/components/features/assistant/useAiProviderSettings.ts#L491-L496: capture the activation response and throw when!r.okso the existingcatchsetsproviderSaveError. Today the signed-in confirmation on Line 489 stays on screen after a failed activation.src/components/features/assistant/useAiProviderSettings.ts#L632-L636: wrap theDELETEintry/catch, checkr.ok, and setproviderSaveErroron failure. Today a rejection becomes an unhandled promise rejection at thevoid providers.clearProvider()call site andonProviderCleared()never runs.📍 Affects 1 file
src/components/features/assistant/useAiProviderSettings.ts#L423-L435(this comment)src/components/features/assistant/useAiProviderSettings.ts#L491-L496src/components/features/assistant/useAiProviderSettings.ts#L632-L636🤖 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/components/features/assistant/useAiProviderSettings.ts` around lines 423 - 435, Check every provider mutation response before advancing UI state: in src/components/features/assistant/useAiProviderSettings.ts lines 423-435, capture the Codex activation response, throw on !r.ok, and only then set the active message and call onProviderActivated(); at lines 491-496, capture the activation response and throw on failure so the existing catch sets providerSaveError; at lines 632-636, wrap the DELETE in try/catch, check r.ok, set providerSaveError on failure, and call onProviderCleared() only after success.
- persistentClient.ts: guard onclose callback against superseded transports (compare reference identity before clearing), and close transport before nulling references in the infrastructure failure path - docs_search.py: replace whitespace controls with spaces instead of stripping all control chars (preserves token boundaries) - passAccessors.ts: add explicit case 'ptq' and log warning on unknown quantMethod in default branch - useMcpDiagnostic.ts: clear diagnostic state before new request - InputEnvironmentPanel.tsx: remove unused filteredRecipes binding - mcp.test.ts: reset callOliveMcpToolImpl in beforeEach, throw on unset mock instead of spawning real Python - HardwareProbeDisplay.tsx: only apply provider change when prepareProviderChange returns a valid patch
When callOliveMcpToolImpl is null, return { unavailable: true }
instead of throwing — this matches the breaker-open behavior the
route handler expects (maps to 503). Throwing caused a 500 which
broke the 'short-circuits with 503' test.
| if (hadInfraFailure) { | ||
| mcpBreaker.recordFailure(epoch); | ||
| // Mark connection as crashed for next attempt | ||
| state = "crashed"; | ||
| client = null; | ||
| transport = null; |
There was a problem hiding this comment.
When an MCP call fails with a non-timeout infrastructure error such as EPIPE or a closed connection, this branch clears the active client and transport references without closing them, leaving the Python process and streams alive. The abandoned transport's unguarded onclose callback can then clear a replacement connection, causing subsequent MCP requests to fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/services/mcp/persistentClient.ts
Line: 216-221
Comment:
**Infrastructure failures abandon MCP transport**
When an MCP call fails with a non-timeout infrastructure error such as EPIPE or a closed connection, this branch clears the active client and transport references without closing them, leaving the Python process and streams alive. The abandoned transport's unguarded `onclose` callback can then clear a replacement connection, causing subsequent MCP requests to fail.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
…nitization (#198) * fix(mcp): guard stale transport close, close on failure, fix query sanitization Addresses unresolved review comments from PR #192: - persistentClient.ts: onclose callback now compares transport identity before clearing state — prevents superseded transports from nulling a replacement connection (Greptile P1, Codex P2) - persistentClient.ts: infra failure path now calls transport.close() before nulling references, freeing the abandoned Python child process - docs_search.py: replace whitespace controls (\n, \t, \r) with spaces instead of stripping — preserves token boundaries so 'quantization\n calibration' becomes two keywords, not one (Codex P2) - test_docs_search_semantic.py: assert sanitized query value and length, not just 'count >= 0' (CodeRabbit) * test(mcp): expand query sanitization and transport lifecycle coverage - Enhance semantic search query sanitization tests to validate all whitespace control characters (\n, \t, \r, \x0b, \x0c) are normalized to spaces - Add test case verifying truncation applies before control normalization, ensuring final query length stays within 2000 char limit - Expand transport lifecycle tests to verify active transport close marks crashed state and clears client/transport refs - Add test for stale transport close behavior, confirming replacement client and transport remain unchanged - Implement infrastructure failure cleanup test validating transport is closed on infra errors - Add test ensuring newer reconnect sessions are not affected by older transport failures - Mock StdioClientTransport with close method and transport instance tracking for comprehensive lifecycle assertions - Expose getPersistentClientSnapshotForTests and setPersistentClientSnapshotForTests helpers for test state inspection and manipulation
- docs/v0.2-tech-debt-plan.md: add 'text' language identifier to fenced code block (markdownlint MD040) - POWER.md: reconcile MCP tool count 26→27 to match _TOOL_IMPORTS (CodeRabbit) - mcpClient.ts: extract unwrapToolPayload() helper to DRY the envelope-unwrap logic duplicated at lines 49-54 and 145-149 (CodeRabbit Trivial)
* fix: address PR #192 UI/lib review comments - passAccessors.ts: add explicit case 'ptq', warn on unknown quantMethod in default branch (Qodo P2) - useMcpDiagnostic.ts: clear diagnostic state before new request to prevent stale advice displaying alongside a new error (CodeRabbit) - InputEnvironmentPanel.tsx: remove unused 'filteredRecipes' binding (CodeRabbit, ESLint) - localFileUtils.ts: reject gapped chunk sequences — require >=2 files with consecutive numeric suffixes before allowing reconstruction. Prevents silent data corruption from missing parts (CodeRabbit Major) - usePresets.ts: add reader.onerror handler so file read failures surface to the user instead of silently dropping (CodeRabbit) * fix: Require chunk sequences to start at 001 * Harden preset import and chunk validation Improves input safety in two areas: chunked local files now only reconstruct when suffixes are canonical and strictly consecutive starting at `001`, and successful preset imports now clear any prior error state. Adds focused tests for `getReconstructableGroups` edge cases and a regression test for `useImportPresets` to ensure errors are cleared after a later successful import. * fix(presets): clear stale import confirmation on file read errors - Remove automatic scroll to execute section when OLIVE run starts - Clear import confirmation state when file read fails to prevent stale UI state - Add test coverage for clearing confirmation on read errors in sequential imports - Improves reliability of preset import flow by ensuring confirmation state stays in sync with actual file state * fix(presets): guard stale FileReader callbacks and clear confirm on parse failure Prevent overlapping preset imports from clobbering a newer confirmation dialog, and clear importConfirm when JSON parse fails after a prior success. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
* fix: docs/config quick wins from PR #192 review - docs/v0.2-tech-debt-plan.md: add 'text' language identifier to fenced code block (markdownlint MD040) - POWER.md: reconcile MCP tool count 26→27 to match _TOOL_IMPORTS (CodeRabbit) - mcpClient.ts: extract unwrapToolPayload() helper to DRY the envelope-unwrap logic duplicated at lines 49-54 and 145-149 (CodeRabbit Trivial) * refactor(input): extract useRecipeHub hook from InputEnvironmentPanel (#157) Move recipe hub state and handlers (applyCuratedRecipe, handleFetchRemote, handleImport, tab management) into a dedicated useRecipeHub hook. Reduces the main component from 2,022 to 1,885 lines and lowers cyclomatic complexity by extracting branching logic (try/catch, validation gates). * refactor(server): reduce method complexity in providerRoutes, ollamaRoutes, system (#152, #158) - providerRoutes.ts: Extract fetchSpecialProviderCatalog() for codex/devin/cloudflare model catalog dispatch, reducing POST /ai/models nesting depth - ollamaRoutes.ts: Extract readOllamaPullStream() for the NDJSON pull stream reading loop, reducing the inline handler from ~222 to ~170 lines - system.ts: Wire probeSystemHardware to use the already-extracted probeGpuHardware() and probeVenvCapabilities() helpers, replacing ~111 lines of inline venv iteration with a clean for-loop + helper call * fix: address CodeFactor findings on PR #201 - Remove 9 unused imports from InputEnvironmentPanel (leftover from useRecipeHub extraction): useTransition, compareCatalogMetadataToRecipe, deriveUiStateFromOliveRecipe, fetchGitHubRecipeJson, getCatalogDeviceFromRecipe, RecipeCatalogItem, parseRecipeJson, assessCatalogItemHardwareCompatibility, assessRecipeHardwareCompatibility - Remove unused recipeRailExpanded destructuring - Extract resolveProviderCredentials() from POST /ai/provider handler - Extract verifyAndSendPullResult() from ollama-pull handler * fix: restore catalog fallbacks and harden extracted helpers Preserve HTTP 200 fallback catalogs for Codex/Devin/Cloudflare failures, ignore stale curated-recipe applies, finish the final Ollama NDJSON record, avoid duplicate timeout SSE errors, and detect DirectML from the default ORT provider list only. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This branch was successfully deployed
1 participant
…hitecture
gemini/toassistant/to reflect multi-provider supportmcpClient.ts,mcpPayload.ts,persistentClient.ts) for better separation of concernshooks/directory (useAutoClearError.ts,useMcpDiagnostic.ts,usePresets.ts)HardwareProbeDisplay.tsx,localFileUtils.ts)useRecipeCatalog.ts) for managing optimization recipespassAccessors.ts) for improved data structure navigationpassAccessors.test.ts)