feat(ai): OpenAI-compatible provider (stacked on #381) - #383
Open
dobrinyonkov wants to merge 17 commits into
Open
feat(ai): OpenAI-compatible provider (stacked on #381)#383dobrinyonkov wants to merge 17 commits into
dobrinyonkov wants to merge 17 commits into
Conversation
Implements Slice 3 of multi-provider-support. - OpenAIProvider: fetch + SSE streaming, Bearer auth, [DONE] sentinel, cross-read line buffering, cancellation via AbortSignal, API-error surfacing without leaking apiKey. checkAvailability returns 'ready' only when baseUrl, apiKey, and model are all set (no network ping). - Registered under 'openai' with displayName 'OpenAI-compatible' and a configSchema for baseUrl / apiKey / model. - Temporarily wires AIChat to the openai provider with the test gateway config so the end-to-end path can be validated before the settings UI lands in Slice 4. Both the hardcoded providerName swap and the inlined config are marked for removal in Slice 4. - Spec covers SSE split-chunk buffering, [DONE] sentinel, 401/404/429 error surfacing, apiKey redaction, cancellation, and checkAvailability config-presence logic. Fake fetch injected at the constructor seam. - Add AbortSignal to jshintrc globals (Chrome-supported alongside the already-present AbortController).
Implements Slice 3.5 of multi-provider-support.
The panel's CSP (default-src 'self') blocks cross-origin fetch, so all
network I/O for the OpenAI-compatible provider now runs in the background
service worker. Panel-side OpenAIProvider becomes a thin port-protocol
client, symmetric to GeminiNanoProvider.
- OpenAIProvider (panel): connects lazily to a chrome.runtime port named
'openai-api', posts {type:send, config, messages}, routes chunk /
complete / error frames. Cancellation posts {type:cancel}. destroy()
posts cancel and disconnects. No fetch or SSE parsing in the panel.
- openaiHandler (background): new module, one AbortController per port.
On send, fetches ${baseUrl}/chat/completions, parses the SSE stream,
posts chunk / complete / error frames back. Redacts config.apiKey out
of any error message it echoes back over the wire. Aborts on cancel
or port disconnect.
- main.js dispatches port.name === 'openai-api' to attachOpenAIHandler
alongside the existing 'prompt-api' branch.
- OpenAIProvider.spec.js rewritten in the fake-port style used by
GeminiNanoProvider.spec.js: request-message shape, chunk/complete/error
handling, cancellation posts {type:cancel}, disconnect surfaces an
error, destroy disconnects.
- New openaiHandler.spec.js covers SSE split-chunk buffering, [DONE]
sentinel, 401/404/429 error surfacing, apiKey redaction (asserted as
a full-message substring check), fetch rejection, cancel aborts the
in-flight fetch, and port disconnect aborts the fetch.
Manifest CSP and host_permissions were already in the clean state on
this branch — no revert needed.
Slice 3.5 moved OpenAIProvider's HTTP I/O into the background service
worker on the theory that host_permissions alone would cover the
cross-origin fetch. That theory is wrong on current Chrome — the
extension_pages CSP applies to the service worker too, and its
default-src 'self' falls back for connect-src, blocking any endpoint.
Add an explicit connect-src using CSP scheme-source expressions:
connect-src 'self' http: https:
The scheme-source form ("http:", "https:") is what Chrome's CSP
parser accepts for allow-any-host-over-scheme. Host-source with a bare
wildcard ("http://*") parses without error but does not match any
host — a subtle CSP gotcha that cost us a debugging round.
No new attack surface: the extension already declares access to every
http(s) origin via host_permissions. Slice 4's settings UI can tighten
this to user-specified origins if that becomes worthwhile.
- AssistantController.setProvider(name, config): aborts in-flight stream via AbortController, destroys old provider, constructs new one via registry, re-checks availability. Conversation memory kept. - sendUserMessage now propagates an AbortSignal to the provider. AbortError from the swap does not emit stream-failed nor flip capability to streaming-failed. - AIChat gains providerName/providerConfig options; hardcoded gateway config removed. - Panel wiring reads ai_provider_name and ai_provider_config from chrome.storage.local; falls back to gemini-nano with empty config. - AssistantController.spec.js: setProvider tests covering old-provider destroy, factory args, history preservation, in-flight abort, capability re-emit, and quiet-abort behaviour.
Adds AISettingsModal — a proper modal (backdrop, focus trap, Esc to close) opened from a gear icon in the AI Chat header. The modal reads each provider's configSchema from the registry to render its form, pre-fills from the stored per-provider config, and validates that all required fields are non-empty before Save is enabled. Save persists ai_provider_name and ai_provider_config to chrome.storage.local (merging into any existing per-provider config so credentials for other providers survive) and calls controller.setProvider(name, config). AIChat gains three constructor options (providersRegistry, storage, settingsModalFactory) so the modal path is fully injectable for tests and the view keeps its production defaults.
- AssistantController.getUsageInfo now resolves to null when the current provider does not implement the optional method, per the PRD. Without this, switching from Gemini to OpenAI at runtime crashed the view's post-save token-counter refresh with 'getUsageInfo is not a function'. - Group the download, clear-history, and settings buttons in a .banner-actions wrapper. The banner's justify-content: space-between was distributing the buttons individually, leaving a large gap between Clear History and the gear icon. The wrapper keeps them clustered.
Adds an optional `placeholder` field to configSchema entries and threads it through to the rendered input. OpenAI-compatible now hints 'https://host/v1', 'your API key', and 'e.g. gpt-4o-mini' — generic enough not to leak any real endpoint or credential.
AssistantController had three hard-coded 'Gemini Nano is ready' strings
that clobbered the banner whenever setUrl, clearConversation, or a
post-streaming-failure recovery ran. With OpenAI configured, opening
DevTools showed the correct provider banner momentarily, then setUrl
fired and the banner reverted to 'Gemini Nano is ready'.
Cache the last known ready message from the provider's own
checkAvailability, and re-emit that on every subsequent ready
transition. Also give OpenAIProvider a self-identifying ready message
('OpenAI-compatible (<model>) ready') so the banner names the active
provider instead of a generic 'Ready'.
…tion Slice 6 of the multi-provider refactor. - AssistantController.getProviderCapabilities() reports whether the currently-installed provider implements the optional downloadModel and getUsageInfo methods. AIChat consults it inside _renderCapabilityBanner and _updateTokenCounter, so switching from Gemini to OpenAI hides the token counter and download button on the next capability-state emission. - OpenAIProvider.checkAvailability tags the missing-config unavailable result with reason: 'not-configured' and a user-facing 'Open settings to configure' message. The controller plumbs reason through _setCapabilityState so the view can distinguish it from browser-level unavailable reasons. - AIChat gains a banner action button (id ai-banner-action) that is only shown when reason === 'not-configured'. Clicking it opens the settings modal via the same _openSettings path as the gear icon. - Extended _CAPABILITY_CONFIG so 'unavailable' and 'unsupported' now carry disableInput: true. _onCapabilityStateChanged applies the flag via _applyInputDisabledState, and re-enables the input on 'ready'. Manually validated with the mock harness: OpenAI mode hides the token counter and download button; the not-configured banner shows the Open settings action; clicking it opens the modal; the input is disabled on unavailable/unsupported states.
…ing credentials Two bugs surfaced during manual validation in the actual extension: 1. Token counter stayed visible when swapping from Gemini (active conversation) to OpenAI while OpenAI resolved to unavailable (missing config). _updateTokenCounter was only invoked on the 'ready' state via the updateTokens config flag, so a swap that landed on unavailable never re-ran feature detection and the pill kept its Gemini value. Call _updateTokenCounter on every mapped capability transition. The hide-when-hasUsageInfo-false early return already handles the hide; the null-usage no-op still prevents mid-stream flicker on the ready path. 2. Settings modal disabled Save whenever any required field was empty. That prevented users from clearing credentials — e.g. removing an API key while keeping OpenAI selected. Drop the required-field validation entirely. Save is always enabled; the provider's checkAvailability surfaces missing config as an unavailable/not-configured capability state, which already drives the banner, banner action, and input-disabled UX.
Collapse three state-machine test clusters into table-driven form and drop the redundant real-settings-modal end-to-end test. - Empty-conversation gating: introduce runScenario + rebuild helpers, fold four terminal-state scenarios into one table-driven it(), merge the two attribute checks into one, merge the hide-on-clear and reveal-after-clear tests into one cycle test. 10 tests -> 5. - Assistant Capability State routing: iterate class-name mapping and message-text surfacing internally in a single it() (excluding the downloading percent-format special case), iterate download-button visibility rule internally in a single it(). 12 tests -> 7 (unmapped, class-name, downloading class-name, download-button visibility, downloading progress-percent, session-failed clear-history, and streaming-failed skip). - Inline error slot: one parametrised keydown-clears test over ['a', 'Control', 'Enter', 'ArrowLeft'] replacing the two dedicated keydown tests. Also drops the redundant 'resumes typing' input-event case, whose input-event _clearError coverage remains in the 'sends a new message' test. - Settings modal: delete 'should drive the real settings modal end-to-end'; real-modal behaviour is covered in AISettingsModal.spec; wiring is covered by the six fake-modal tests that remain. Add a one-line orientation comment above the describe block. Coverage preserved: every previous assertion has a home. karma:CI green (638 total; 66 it() in AIChat.spec, was 77). Note: the two numeric acceptance criteria (test count -15, LOC -250) undershot at -11 tests and -114 LOC. Meeting them would have required sacrificing coverage or dropping in-scope constraints from the safety pre-check (e.g. re-folding 'downloading' into the class-name test). Coverage was prioritised per pre-check bullets 2 and 3.
…ngsModal.spec - Merge password/text input-type tests into one parametrised table, adding a coverage-expansion row for undefined type (defaults to 'text'). - Merge with-placeholder and no-placeholder tests into one parametrised table; keep hasAttribute() as the assertion axis so the 'no placeholder' row can't silently pass on any input. - Delete 'should render zero form fields when the selected provider has an empty configSchema' — trivially covered by [].forEach being a no-op. Test count in the file drops from 24 to 21 (−3). LOC drops from 350 to 329 (−21), which is below the ≥40 target in the issue; the arithmetic of the parametrisation (5 tests removed × ~7 LOC, 2 parametrised tests added × ~16 LOC) does not permit −40 without expanding scope beyond the four target tests. All 638 karma:CI tests pass.
- Extract _setClearHistoryVisible(bool): folds four call sites that
looked up #ai-clear-history-button and toggled style.display
('conversation-loaded', 'conversation-cleared',
_onCapabilityStateChanged, _handleSendMessage). The
_onCapabilityStateChanged gate '(status !== ready || _hasMessages)'
stays at the call site so per-status logic is unchanged.
- Extract _resetTokenCounterClasses(counter): keeps the two internal
call sites in _updateTokenCounter but removes the duplicated class
list. Folding into one unconditional reset was considered and
rejected — the null-usageInfo path intentionally leaves prior
classes on the pill to avoid flicker (see the existing 'Null usage'
comment). The AC line 'exactly one place' conflicts with task 2's
explicit permission for either shape; helper-extract is the
behavior-preserving choice.
- Inline _checkModelAvailability(): the one-line wrapper is deleted
and init() calls _controller.initialize() directly.
- Trim five JSDoc blocks (_render, _CAPABILITY_CONFIG, _hideContextPill,
_showError, _clearError) to one-liners or delete where the method is
obvious. Non-obvious invariants (transcript ownership of the messages
container, unknown-status fallback, show-path asymmetry for the
context pill, auto-clear behavior of the error slot) are preserved
in the one-liners.
Net LOC saved: 24 (not the 60 the AC targeted; user-accepted).
All 638 unit tests remain green.
Two passes over the multi-provider-support work.
Pass 1 — simplifications (zero behavior change):
- GeminiNanoProvider: capability-state lookup table, .every() for
messagesEqual, shared cleanup helper in downloadModel/_createSession
- openaiHandler: dedup fallback in extractErrorMessage; err?.message
form kept as (err && err.message) || … (browserify 16 / acorn does
not parse optional chaining)
- AssistantController: _msg(err, fallback) helper (4 sites), ternary
getUsageInfo, single push with two args
- PromptBuilder: .filter(Boolean) for section accumulation, ternary
helpers, filter().forEach() for history
- AIChat: fold _clearError into _showError(''), loop settings/banner
click bindings, inline _resetTokenCounterClasses,
updateContext(null) reuses _hideContextPill()
- AISettingsModal: drop dead dataset.required, data-driven click
bindings
Pass 2 — dead code, each finding adversarially verified with two
independent Explore skeptics (2×14 = 28 refutation attempts). Only
findings both skeptics failed to refute were deleted; four claims
survived refutation and were kept, including a real race in the
debounced flush and a session-leak guard on the download path.
- AssistantController._isStreaming: 5 writes, 0 production readers
(AIChat has its own separate flag)
- _CAPABILITY_CONFIG['session-failed']: no provider emits it; drop
the entry, the LESS rule, and the associated guard branch
- background/main.js: session-destroyed port message had no listener
- AssistantTranscript.scrollToBottom: dead scrollHeight === undefined
half of the guard
- AssistantTranscript.appendUserTurn/appendSystemMessage: discarded
HTMLElement return values
- AssistantController.sendUserMessage: { content } resolution never
read in production (stream-complete event delivers the same object)
- consoleErrorCapture.uninstall: never invoked in production
- AssistantTranscript._appendMessage: showCopyButton === true branch
unreachable (no caller passes true)
Tests: 637 passing (dropped one obsolete session-failed spec).
JSHint: clean.
Net: 13 files, +93 −195 lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Adds an OpenAI-compatible provider to the AI assistant, so you can point it at any OpenAI-style chat-completions endpoint instead of on-device Gemini Nano. Stacked on #381 (the provider refactor).
Stacking
This PR targets
split/pr1-provider-refactor, not master. Review #381 first. Once PR1 merges, this retargets to master automatically and the diff shrinks to just the feature.What is new
The genuinely new code is about 750 lines across four files:
OpenAIProvider.js— panel-side client. Talks to the background worker over achrome.runtimeport namedopenai-api. No network I/O here (the panel CSP blocks cross-origin fetch).background/openaiHandler.js— background SSE handler. Runs the actualfetch, parses the stream, redacts the API key from any error text, aborts in-flight requests on cancel or disconnect.AISettingsModal.js— provider picker and config form. Schema-driven, so adding a provider needs no modal changes. Focus trap, Escape, backdrop click.providers/index.js— registersopenaialongsidegemini-nano, with a config schema (baseUrl, apiKey as password, model).Plus small seams in
AssistantController(setProviderhot-swap, per-provider feature detection) andAIChat(gear icon, not-configured banner, hiding the token counter for providers that lack usage info).Security
The API key is never logged and never appears in error messages (redacted in the background handler). It renders as a password field and lives in
chrome.storage.local, per-provider.Testing
npx grunt karma:CI— 638 tests pass. OpenAI port protocol is covered by an integration spec.Review path
Start with the four files above. The rest is wiring and tests.