Sync with llama-swap v190

[napmany]

Summary by CodeRabbit

• New Features

  • New Svelte-based UI with playground (chat, images, speech, transcription), model/metrics views, and capture viewer.
  • Streaming chat, image generation, audio transcription and speech synthesis endpoints plus a voices list endpoint.
  • Request/response capture storage and in-UI capture retrieval.

• Security

  • API key authentication for protected endpoints and peer requests.

• Configuration

  • New captureBuffer, apiKeys, and peers configuration and env-var macros.

• Infrastructure

  • ROCm platform support and updated CI/workflows.

• Documentation

  • Expanded configuration and endpoint docs.

[coderabbitai]

Walkthrough

Adds a new Svelte frontend, peer-based remote model routing and proxying, request/response capture with compression handling, environment-variable macro and peers/apiKeys config support, API-key-protected endpoints, and numerous CI/Docker updates. Many new tests and UI components were added and the old React UI removed.

Changes

Cohort / File(s)	Summary
Top-level config & examples config-schema.json, config.example.yaml, docker/config.example.yaml	Added captureBuffer, apiKeys, peers, logToStdout, setParams support; updated examples and macros; added z-image docker example.
Config parsing & macros proxy/config/config.go, proxy/config/*_test.go, proxy/config/filters.go, proxy/config/model_config.go, proxy/config/peer.go	Env-var macro substitution, sanitize/validation helpers, CaptureBuffer/RequiredAPIKeys/Peers fields, Filters with SetParams, ModelFilters backwards-compat unmarshalling, PeerConfig types and validations, and extensive tests.
Peer routing & proxying proxy/peerproxy.go, proxy/peerproxy_test.go, proxy/proxymanager.go, proxy/proxymanager_api.go, proxy/proxymanager_test.go	New PeerProxy, model→peer mapping, API-key injection into proxied requests, peer-aware listing, apiKeyAuth middleware, /api/captures handler, and tests covering peer flows and auth.
Metrics & captures proxy/metrics_monitor.go, proxy/metrics_monitor_test.go	ReqRespCapture type, in-memory FIFO capture buffer, HasCapture flag, compression-aware decompression, header redaction, capture storage/eviction, and expanded tests.
Proxy static serving proxy/ui_compress.go, proxy/ui_compress_test.go	ServeCompressedFile and selectEncoding for pre-compressed assets (br/gzip) with tests and fallbacks.
CLI/service endpoint cmd/simple-responder/simple-responder.go	Added GET /v1/audio/voices endpoint returning voices and echoing model query param.
Docker & build scripts docker/build-container.sh, docker/llmsnap-sd.Containerfile, .github/workflows/containers.yml, Makefile	Added fetch_llama_tag helper, logging helpers, rocm/musa/vulkan support, SD layering, BASE/SD image args, rocm cleanup step, and updated Makefile UI targets.
CI / workflows .github/workflows/*.yml	Added ui-svelte UI tests workflow, updated Node to v24, refined path filters for Go CI, added rocm to matrix and conditional cleanup step.
Svelte UI (new) ui-svelte/** (many files; see top-level list)	Complete Svelte frontend: app shell, many components (Chat/Image/Speech/Audio interfaces, CaptureDialog, panels, playground), stores (api, theme, persistent), API clients (chatApi, imageApi, audioApi, speechApi), markdown/KaTeX, vite/svelte config, packaging and tests.
React UI removal ui/** (many files removed)	Deleted legacy React UI, configs, ESLint, tsconfigs, and related assets.
Docs & README README.md, docs/configuration.md, CLAUDE.md	Documented new endpoints, peers/apiKeys configuration, SD notes, and updated contributor/testing/commit guidelines.
Misc tests & helpers proxy/helpers_test.go, proxy/process_test.go, proxy/processgroup_test.go	Cross-platform path handling in tests and short-mode skips for slow tests.
Build container UI config .coderabbit.yaml	Added flags to disable sequence diagrams, finishing-touches docstrings, and issue enrichment planning.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant ProxyManager
    participant PeerProxy
    participant RemotePeer
    participant MetricsMonitor

    Client->>ProxyManager: POST /v1/chat/completions (model=peer-model)
    ProxyManager->>ProxyManager: apiKeyAuth() check
    ProxyManager->>PeerProxy: HasPeerModel(model)
    PeerProxy-->>ProxyManager: true
    ProxyManager->>MetricsMonitor: create capture (if enabled)
    ProxyManager->>PeerProxy: ProxyRequest(model, request)
    PeerProxy->>PeerProxy: inject Authorization (if configured)
    PeerProxy->>RemotePeer: forward request (preserve Host)
    RemotePeer-->>PeerProxy: stream response (SSE or chunked)
    PeerProxy->>ProxyManager: stream back response (disable buffering for SSE)
    ProxyManager->>MetricsMonitor: store capture and associate metric ID
    ProxyManager-->>Client: stream response
    Client->>ProxyManager: GET /api/captures/:id
    ProxyManager->>MetricsMonitor: getCaptureByID(id)
    MetricsMonitor-->>ProxyManager: ReqRespCapture JSON
    ProxyManager-->>Client: 200 Capture JSON

Loading

sequenceDiagram
    participant Browser
    participant SvelteUI
    participant APIStore
    participant ProxyServer

    Browser->>SvelteUI: load app
    SvelteUI->>APIStore: enableAPIEvents(true)
    APIStore->>ProxyServer: EventSource /api/events
    ProxyServer-->>APIStore: modelStatus / metrics / logData
    APIStore->>SvelteUI: update stores (models, metrics, logs)
    Browser->>SvelteUI: user sends prompt (streaming)
    SvelteUI->>ProxyServer: POST /v1/chat/completions (stream:true)
    ProxyServer-->>SvelteUI: SSE chunks (content + reasoning_content)
    SvelteUI->>Browser: render streamed chunks in ChatMessage component

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~80 minutes

Possibly related PRs

• Fix containers build action #5: overlaps changes to container workflows and docker/build-container.sh (rocm and build flow).
• Sync with llama-swap #7: overlaps metrics/capture and proxy changes touching proxy/metrics_monitor.go and proxymanager flow.
• Sync with llama-swap v178 #11: related to peer-based routing and peers/apiKey configuration schema changes.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Sync with llama-swap v190' accurately reflects the main purpose of this changeset: synchronizing the codebase with a specific upstream version. The change encompasses configuration updates, workflow modifications, UI framework migration (React to Svelte), and several feature additions, all aligned with a version sync objective.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch sync-v190

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 12

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

proxy/config/config.go (3)

311-329: ⚠️ Potential issue | 🟡 Minor

Duplicate sleep/wake endpoint substitution — likely merge artifact.

Lines 312-319 and 322-329 are identical blocks that substitute macros in SleepEndpoints and WakeEndpoints. The substitution runs twice per macro per endpoint. While strings.ReplaceAll is idempotent so this won't produce incorrect results, it's clearly a copy-paste duplication.

🧹 Remove the duplicate block

 			for j := range modelConfig.WakeEndpoints {
 				modelConfig.WakeEndpoints[j].Endpoint = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Endpoint, macroSlug, macroStr)
 				modelConfig.WakeEndpoints[j].Body = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Body, macroSlug, macroStr)
 			}
 
-			// Substitute in sleep/wake endpoint arrays
-			for j := range modelConfig.SleepEndpoints {
-				modelConfig.SleepEndpoints[j].Endpoint = strings.ReplaceAll(modelConfig.SleepEndpoints[j].Endpoint, macroSlug, macroStr)
-				modelConfig.SleepEndpoints[j].Body = strings.ReplaceAll(modelConfig.SleepEndpoints[j].Body, macroSlug, macroStr)
-			}
-			for j := range modelConfig.WakeEndpoints {
-				modelConfig.WakeEndpoints[j].Endpoint = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Endpoint, macroSlug, macroStr)
-				modelConfig.WakeEndpoints[j].Body = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Body, macroSlug, macroStr)
-			}
-
 			// Substitute in metadata (type-preserving)

---

356-374: ⚠️ Potential issue | 🟡 Minor

Duplicate PORT substitution in sleep/wake endpoints.

Same duplication pattern as the macro substitution above — lines 357-364 and 367-374 are identical blocks for PORT substitution.

🧹 Remove the duplicate block

 			for j := range modelConfig.WakeEndpoints {
 				modelConfig.WakeEndpoints[j].Endpoint = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Endpoint, macroSlug, macroStr)
 				modelConfig.WakeEndpoints[j].Body = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Body, macroSlug, macroStr)
 			}
 
-			// Substitute PORT in sleep/wake endpoint arrays
-			for j := range modelConfig.SleepEndpoints {
-				modelConfig.SleepEndpoints[j].Endpoint = strings.ReplaceAll(modelConfig.SleepEndpoints[j].Endpoint, macroSlug, macroStr)
-				modelConfig.SleepEndpoints[j].Body = strings.ReplaceAll(modelConfig.SleepEndpoints[j].Body, macroSlug, macroStr)
-			}
-			for j := range modelConfig.WakeEndpoints {
-				modelConfig.WakeEndpoints[j].Endpoint = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Endpoint, macroSlug, macroStr)
-				modelConfig.WakeEndpoints[j].Body = strings.ReplaceAll(modelConfig.WakeEndpoints[j].Body, macroSlug, macroStr)
-			}
-
 			if len(modelConfig.Metadata) > 0 {

---

410-424: ⚠️ Potential issue | 🟡 Minor

Duplicate validateEndpointMacros calls.

Lines 411-416 and 418-424 call the same validation twice for both SleepEndpoints and WakeEndpoints.

🧹 Remove the duplicate block

 		if err := validateEndpointMacros(modelConfig.WakeEndpoints, modelId, "wakeEndpoints"); err != nil {
 			return Config{}, err
 		}
 
-		// Check sleep/wake endpoint arrays for unknown macros
-		if err := validateEndpointMacros(modelConfig.SleepEndpoints, modelId, "sleepEndpoints"); err != nil {
-			return Config{}, err
-		}
-		if err := validateEndpointMacros(modelConfig.WakeEndpoints, modelId, "wakeEndpoints"); err != nil {
-			return Config{}, err
-		}
-
 		if len(modelConfig.Metadata) > 0 {

🤖 Fix all issues with AI agents

In `@docker/build-container.sh`:
- Around line 106-122: The script currently aborts early because the global
errexit makes the subshell assignment LCPP_TAG=$(fetch_llama_tag ...) fail the
whole script if fetch_llama_tag returns non-zero; update the LCPP_TAG
assignments (the two places calling fetch_llama_tag) to temporarily disable
errexit or otherwise capture the command failure (e.g., use set +e before the
call and restore set -e after, or append || true to the command and then check
the exit status/empty value), then check LCPP_TAG for emptiness and run the
existing log_info + exit 1 branch; keep references to fetch_llama_tag and the
LCPP_TAG variable so the friendly error path remains reachable.

In `@proxy/config/filters_test.go`:
- Around line 152-160: The test currently only asserts primitive types inside
the loop over tt.wantParams/gotParams using a type switch, so complex nested
values like the "provider" map and "transforms" slice are never compared;
replace the type-switch branch with a single deep-equality assertion (use
assert.Equal) for gotValue vs wantValue for every key, ensuring nested
maps/slices are properly compared while still keeping the existence check for
gotParams[key].

In `@proxy/config/peer.go`:
- Around line 35-39: The parsed URL check using url.Parse is too permissive:
after parsing defaults.Proxy into parsedURL, verify parsedURL.Scheme is
non-empty and one of the expected schemes (e.g., "http" or "https"); if the
scheme is empty or unsupported, return a formatted error like "invalid peer
proxy URL (…): missing or unsupported scheme". Update the block that parses
defaults.Proxy (the parsedURL, err := url.Parse(defaults.Proxy) section) to
perform this scheme validation and return an error when invalid, and run make
test-dev to ensure tests and staticcheck pass.

In `@proxy/proxymanager_api.go`:
- Around line 98-120: The code duplicates the loop that appends peer models to
the models slice, causing each peer model from pm.peerProxy.ListPeers() to be
added twice; remove the second identical block (the repeated for peerID, peer :=
range pm.peerProxy.ListPeers() { ... models = append(models, Model{ Id: modelID,
PeerID: peerID }) }) so that models is populated only once, leaving a single
iteration over pm.peerProxy.ListPeers(); after the change run make test-dev to
validate with go test and staticcheck.

In `@proxy/proxymanager.go`:
- Around line 956-963: The API key equality check in the proxy manager (the loop
over pm.config.RequiredAPIKeys comparing providedKey to key) is vulnerable to
timing attacks; replace the plain string comparison with a constant-time
comparison by importing "crypto/subtle" and using subtle.ConstantTimeCompare on
the byte slices of providedKey and each key from pm.config.RequiredAPIKeys
(e.g., convert both to []byte and treat a result of 1 as a match) to set valid,
leaving the rest of the validation logic intact.

In `@proxy/ui_compress.go`:
- Around line 48-52: Remove the dead open/close block that calls fs.Open(name)
and assigns to origFile (the unused origFile variable) in proxy/ui_compress.go;
delete the two lines that open and immediately close origFile (fs.Open(name) and
origFile.Close()) and any related error handling surrounding them since
http.ServeContent already handles content-type inference from name and nothing
else uses origFile (verify no other code depends on origFile or that err from
fs.Open is unused before removing).

In `@ui-svelte/package.json`:
- Around line 14-27: The package.json devDependencies list contains non-existent
versions for `@types/node` and vitest; update the version strings for
"@types/node" from "^25.1.0" to "^25.0.3" and for "vitest" from "^4.0.18" to
"^4.0.16" in the devDependencies block so npm can resolve the packages; locate
these keys in package.json and change their version values accordingly.

In `@ui-svelte/src/components/playground/ChatMessage.svelte`:
- Around line 25-29: The markdown rendering pipeline in renderMarkdown (used to
compute renderedContent) is currently allowing raw HTML via allowDangerousHtml
and exposes XSS when inserted with {`@html` renderedContent}; update the pipeline
to sanitize output by adding rehype-sanitize (import rehypeSanitize and add
.use(rehypeSanitize) into the remarkRehype/rehype pipeline before
rehypeStringify) so dangerous elements/scripts are stripped while preserving
safe formatting, and ensure renderedContent continues to use the sanitized
result.

In `@ui-svelte/src/components/playground/SpeechInterface.svelte`:
- Around line 143-149: The component leaks blob URLs because generatedAudioUrl
is not revoked when the component is destroyed; add a Svelte onDestroy cleanup
that checks generatedAudioUrl and calls URL.revokeObjectURL(generatedAudioUrl)
(and clears generatedAudioUrl) to free the blob URL; import onDestroy from
'svelte' if not already present and register the handler near the top of the
script so generatedAudioUrl is always revoked on component unmount.

In `@ui-svelte/src/lib/markdown.ts`:
- Around line 63-70: The pipeline currently allows dangerous HTML and needs
sanitization: import rehypeSanitize and defaultSchema, build a sanitizeSchema
that spreads defaultSchema and extends attributes for code (allow className,
"hljs", and /^language-.*/) and span (allow className matching /^hljs(-.+)?$/)
so KaTeX and highlight.js classes are preserved, then append
.use(rehypeSanitize, sanitizeSchema) to the unified processor after the existing
.use(rehypeStringify, { allowDangerousHtml: true }) call; keep remarkRehype,
rehypeKatex, rehypeHighlight, and rehypeStringify in place but ensure processor
uses rehypeSanitize to prevent XSS from LLM content.

In `@ui-svelte/src/routes/Activity.svelte`:
- Around line 48-56: The function viewCapture currently sets loadingCaptureId
before awaiting getCapture but never clears it if getCapture throws; wrap the
await in a try/finally inside viewCapture so loadingCaptureId is always reset to
null in the finally block, then only set selectedCapture and dialogOpen when
capture is truthy (after the try), ensuring getCapture errors don't leave the UI
stuck; reference viewCapture, loadingCaptureId, getCapture, selectedCapture, and
dialogOpen when making the change.

In `@ui-svelte/src/stores/api.ts`:
- Around line 27-102: The reconnect setTimeout in onerror can fire after
enableAPIEvents(false) and resurrect the EventSource; fix by tracking the
reconnect timer ID (e.g., add a module-scoped let reconnectTimer:
ReturnType<typeof setTimeout> | null), replace setTimeout(connect, delay) with
reconnectTimer = setTimeout(connect, delay), clear and null out that timer when
disabling (inside enableAPIEvents when !enabled) and also clear any existing
reconnectTimer before scheduling a new one in connect/onerror so stale timers
never reopen apiEventSource; update connect to reset reconnectTimer = null on
successful onopen and ensure apiEventSource is only created when enabled.

🟡 Minor comments (18)

Makefile-38-43 (1)

38-43: ⚠️ Potential issue | 🟡 Minor

Make target ui/node_modules never matches the actual directory, so npm install runs every time.

The target name ui/node_modules is a file-based prerequisite, but npm install now creates ui-svelte/node_modules. Make will never see the target as satisfied, causing unnecessary reinstalls on every build. Also, the comment on line 41 still references React.

Proposed fix

-ui/node_modules:
-	cd ui-svelte && npm install
+ui-svelte/node_modules:
+	cd ui-svelte && npm install
 
-# build react UI
-ui: ui/node_modules
+# build svelte UI
+ui: ui-svelte/node_modules
 	cd ui-svelte && npm run build

CLAUDE.md-10-10 (1)

10-10: ⚠️ Potential issue | 🟡 Minor

Tech stack description is outdated — still references React and ui/.

This PR replaces the React UI with Svelte under ui-svelte/. Update this line to reflect the current stack.

-- typescript, vite and react for UI (located in ui/)
+- typescript, vite and svelte for UI (located in ui-svelte/)

README.md-16-16 (1)

16-16: ⚠️ Potential issue | 🟡 Minor

Minor grammar: hyphenate "OpenAI-compatible".

Since this line was touched, consider fixing the compound adjective.

-- ✅ Use any local OpenAI compatible server (llama.cpp, vllm, tabbyAPI, stable-diffusion.cpp, etc.)
+- ✅ Use any local OpenAI-compatible server (llama.cpp, vllm, tabbyAPI, stable-diffusion.cpp, etc.)

docker/build-container.sh-30-36 (1)

30-36: ⚠️ Potential issue | 🟡 Minor

ShellCheck SC2145: Use ${ALLOWED_ARCHS[*]} when interpolating inside a string.

${ALLOWED_ARCHS[@]} inside a double-quoted string produces multiple arguments. Use * instead to join elements into a single string for the log message.

Proposed fix

-  log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}"
+  log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[*]}"

ui-svelte/src/components/TokenHistogram.svelte-24-30 (1)

24-30: ⚠️ Potential issue | 🟡 Minor

Division by zero when range is 0 or bins is empty.

If data.min === data.max (single unique value), range is 0 and getXPosition divides by zero, producing NaN/Infinity SVG coordinates. Similarly, an empty bins array causes Math.max() → -Infinity and barWidth → Infinity.

The upstream StatsPanel likely guards against these, but the component should be self-defensive.

🛡️ Proposed defensive guards

- let maxCount = $derived(Math.max(...data.bins));
- let barWidth = $derived(chartWidth / data.bins.length);
- let range = $derived(data.max - data.min);
+ let maxCount = $derived(data.bins.length > 0 ? Math.max(...data.bins) : 0);
+ let barWidth = $derived(data.bins.length > 0 ? chartWidth / data.bins.length : 0);
+ let range = $derived(data.max - data.min || 1);

docker/config.example.yaml-22-32 (1)

22-32: ⚠️ Potential issue | 🟡 Minor

Use cmd: > for consistency with other models.

The other models use the folded block scalar (>), which is the project convention. While both syntaxes (| and >) are functionally equivalent due to the SanitizeCommand() function using shlex to parse arguments (which treats newlines and spaces identically), maintaining consistent YAML style is preferable for maintainability.

docs/configuration.md-541-541 (1)

541-541: ⚠️ Potential issue | 🟡 Minor

Typo: "keys is the peer'd ID" → "key is the peer's ID".

📝 Proposed fix

-  # keys is the peer'd ID
+  # key is the peer's ID

docs/configuration.md-90-90 (1)

90-90: ⚠️ Potential issue | 🟡 Minor

Minor: "up to date" → "up-to-date".

As a compound adjective modifying "reference," it should be hyphenated.

📝 Proposed fix

-> Always check [config.example.yaml](https://github.com/napmany/llmsnap/blob/main/config.example.yaml) for the most up to date reference for all example configurations.
+> Always check [config.example.yaml](https://github.com/napmany/llmsnap/blob/main/config.example.yaml) for the most up-to-date reference for all example configurations.

proxy/proxymanager_api.go-27-28 (1)

27-28: ⚠️ Potential issue | 🟡 Minor

Stale comment: "React" should say "Svelte" (or be made generic).

The comment still references React but this PR replaces it with a Svelte-based frontend.

Proposed fix

-	// Add API endpoints for React to consume
+	// Add API endpoints for the UI to consume

ui-svelte/src/components/playground/AudioInterface.svelte-123-131 (1)

123-131: ⚠️ Potential issue | 🟡 Minor

navigator.clipboard.writeText can reject — unhandled promise.

writeText returns a Promise that can reject (e.g., on non-secure contexts or if the document isn't focused). Swallowing the rejection silently may cause an unhandled promise rejection warning and leaves the user with no feedback that copying failed.

Proposed fix

  function copyToClipboard() {
    if (transcriptionResult) {
-     navigator.clipboard.writeText(transcriptionResult);
-     copied = true;
-     setTimeout(() => {
-       copied = false;
-     }, 2000);
+     navigator.clipboard.writeText(transcriptionResult).then(() => {
+       copied = true;
+       setTimeout(() => {
+         copied = false;
+       }, 2000);
+     }).catch(() => {
+       error = "Failed to copy to clipboard";
+     });
    }
  }

ui-svelte/src/components/playground/ImageInterface.svelte-206-228 (1)

206-228: ⚠️ Potential issue | 🟡 Minor

Fullscreen dialog does not programmatically receive focus on open.

The overlay has tabindex="-1" and an onkeydown handler for Escape, but there's no code to focus the element when showFullscreen becomes true. Keyboard-only users won't be able to dismiss via Escape until they manually tab/click into the dialog.

A $effect or use: action that calls .focus() on the dialog element when it mounts would resolve this.

ui-svelte/src/components/playground/SpeechInterface.svelte-118-125 (1)

118-125: ⚠️ Potential issue | 🟡 Minor

Auto-play effect triggers too broadly.

This $effect runs whenever any of generatedAudioUrl, $autoPlayStore, or audioElement changes — not just when new audio is generated. This means:

• Toggling auto-play ON while audio already exists will immediately start playback.
• If audioElement rebinds (e.g., DOM remount), it may replay.

Consider gating auto-play with a flag that's only set during generate(), or tracking the previous generatedAudioUrl value to detect actual changes.

ui-svelte/src/components/playground/ImageInterface.svelte-38-42 (1)

38-42: ⚠️ Potential issue | 🟡 Minor

Hardcoded image/png MIME type may not match the actual image format.

The base64 data URI is constructed assuming PNG (data:image/png;base64,...), but the API response may return JPEG, WebP, or other formats. If the server returns a non-PNG image, the browser may still render it (browsers are lenient), but the MIME type is technically incorrect.

Consider deriving the MIME type from response headers or the image data, or using a generic data:image/*;base64,... approach. Similarly, the download filename on line 71 assumes .png.

ui-svelte/src/components/playground/SpeechInterface.svelte-168-170 (1)

168-170: ⚠️ Potential issue | 🟡 Minor

clearInput only clears text — inconsistent with ImageInterface.clearImage().

ImageInterface.clearImage() clears the generated image, error, and prompt. Here, clearInput() only clears the text, leaving the generated audio, error, and metadata in place. If the button is meant to reset the workspace, it should also clear generatedAudioUrl (and revoke it), error, generatedVoice, and generatedTimestamp.

ui-svelte/src/components/StatsPanel.svelte-40-50 (1)

40-50: ⚠️ Potential issue | 🟡 Minor

Division by zero when all token speeds are identical (max === min).

When every metric has the same tokens/sec, binSize becomes 0, making the bin-index calculation on line 48 produce NaN. All bins stay at zero and the histogram renders empty.

🐛 Proposed fix

     const min = Math.min(...tokensPerSecond);
     const max = Math.max(...tokensPerSecond);
+
+    // Guard: if all values are identical, skip histogram
+    if (max === min) {
+      return {
+        totalRequests,
+        totalInputTokens,
+        totalOutputTokens,
+        tokenStats: {
+          p99: p99.toFixed(2),
+          p95: p95.toFixed(2),
+          p50: p50.toFixed(2),
+        },
+        histogramData: null,
+      };
+    }
+
     const binCount = Math.min(30, Math.max(10, Math.floor(tokensPerSecond.length / 5)));
     const binSize = (max - min) / binCount;

config.example.yaml-65-68 (1)

65-68: ⚠️ Potential issue | 🟡 Minor

Incorrect default value in comment for captureBuffer.

The comment says default: 10, but the actual default in proxy/config/config.go (Line 209) is 5, and config-schema.json (Line 105) also specifies default: 5.

📝 Fix

-# captureBuffer: how many MBs to allocate for storing request/response captures
-# - optional, default: 10
+# captureBuffer: how many MBs to allocate for storing request/response captures  
+# - optional, default: 5
 # - set to 0 to disable
 captureBuffer: 15

ui-svelte/src/components/playground/ChatInterface.svelte-28-36 (1)

28-36: ⚠️ Potential issue | 🟡 Minor

Auto-scroll doesn't trigger during streaming content updates.

The $effect depends on messages.length, which only changes when a message is added/removed — not when the content of the last message is updated during streaming. This means the chat won't auto-scroll as the assistant response grows.

Consider also tracking content of the last message or using a more targeted scroll trigger:

♻️ Suggested approach

   $effect(() => {
-    if (messages.length > 0 && messagesContainer) {
+    // Track both message count and last message content to scroll during streaming
+    const lastMsg = messages[messages.length - 1];
+    const _trigger = lastMsg?.content;
+    if (messages.length > 0 && messagesContainer) {
       messagesContainer.scrollTo({
         top: messagesContainer.scrollHeight,
         behavior: "smooth",
       });
     }
   });

ui-svelte/src/stores/api.ts-176-188 (1)

176-188: ⚠️ Potential issue | 🟡 Minor

Apply URL encoding to model name parameters in path segments.

The model parameter is interpolated directly into URL paths in loadModel (line 178), unloadSingleModel (line 150), and sleepModel (line 164). Model names like author/model work because the server's router interprets the extra path segment correctly, but names containing ?, #, or % would break the URL structure before reaching the server. Query parameters correctly use encodeURIComponent elsewhere in the codebase (e.g., SpeechInterface.svelte), but path segments do not.

Proposed fix

 export async function loadModel(model: string): Promise<void> {
   try {
-    const response = await fetch(`/upstream/${model}/`, {
+    const response = await fetch(`/upstream/${encodeURIComponent(model)}/`, {
       method: "GET",
     });

Apply the same pattern to unloadSingleModel and sleepModel. The server correctly URL-decodes path parameters, so author/model encoded as author%2Fmodel will be properly restored.

🧹 Nitpick comments (32)

ui-svelte/src/components/Tooltip.svelte (1)

9-20: Consider adding basic accessibility attributes.

The tooltip is CSS-only and invisible to keyboard and screen-reader users. Adding role="tooltip", an aria-describedby link, and tabindex="0" on the trigger would improve accessibility. This mirrors the same gap in the React original, so fine to defer.

♿ Optional a11y improvement

-<div class="relative group inline-block">
-  <span class="cursor-help">&#9432;</span>
+<div class="relative group inline-block" role="tooltip">
+  <span class="cursor-help" tabindex="0" aria-label={content}>&#9432;</span>

proxy/ui_compress.go (2)

63-65: Minor: err.Error() may leak internal filesystem paths to the client.

Consider using a generic "not found" message instead of exposing the raw error string.

Proposed fix

-		http.Error(w, err.Error(), http.StatusNotFound)
+		http.NotFound(w, r)

---

10-30: selectEncoding ignores quality values (q=...), always preferring br over gzip.

This is a pragmatic simplification, but it means a client sending gzip;q=1.0, br;q=0 (explicitly disabling brotli) would still get brotli. For serving your own UI assets this is likely fine, but worth noting as a known limitation. A q=0 value per RFC 7231 means "not acceptable."

.github/workflows/ui-tests.yml (1)

28-33: Node.js 24 is not yet LTS.

Node.js 24 won't reach LTS status until October 2026. If CI stability is a priority, consider using Node 22 (current LTS). Not a blocker if the project intentionally targets the latest current release.

CLAUDE.md (1)

28-40: Add a language identifier to the fenced code block.

Static analysis flags the missing language specifier. Since this is a commit message example, text works:

-```
+```text
 proxy: add new feature

docker/build-container.sh (2)

61-104: ShellCheck SC2155: local + assignment masks return values from curl and jq.

On lines 69, 74, and 86, local var=$(...) discards the exit code. If curl fails silently (e.g., network error returning an empty string), the function may proceed with empty/garbage data rather than detecting the failure.

♻️ Declare and assign separately

-        local response=$(curl -s -H "Authorization: Bearer $GITHUB_TOKEN" \
+        local response
+        response=$(curl -s -H "Authorization: Bearer $GITHUB_TOKEN" \
             "https://api.github.com/users/ggml-org/packages/container/llama.cpp/versions?per_page=${per_page}&page=${page}")

Apply the same pattern to error_msg (line 74) and found_tag (line 86).

---

5-5: ShellCheck SC2046: Quote the command substitution to handle paths with spaces.

Proposed fix

-cd $(dirname "$0")
+cd "$(dirname "$0")"

ui-svelte/src/components/playground/ExpandableTextarea.svelte (1)

75-121: Consider closing the overlay on backdrop click.

Clicking the semi-transparent backdrop (bg-black/50 div) doesn't close the overlay — only the explicit Cancel/Close/Escape actions do. Users commonly expect clicking outside a modal to dismiss it.

♻️ Add backdrop click handler

- <div class="fixed inset-0 z-50 flex items-center justify-center bg-black/50 p-4">
+ <!-- svelte-ignore a11y_no_static_element_interactions -->
+ <div class="fixed inset-0 z-50 flex items-center justify-center bg-black/50 p-4"
+   onclick={(e) => { if (e.target === e.currentTarget) closeExpanded(); }}
+   onkeydown={() => {}}
+ >

ui-svelte/src/components/CaptureDialog.svelte (1)

66-72: Content-Type header lookup only checks two casings.

HTTP headers are case-insensitive, but getContentType only checks "Content-Type" and "content-type". Headers like "CONTENT-TYPE" or "Content-type" would be missed. If the backend normalizes headers before populating ReqRespCapture, this is fine.

♻️ Case-insensitive header lookup

  function getContentType(
    headers: Record<string, string> | null | undefined,
  ): string {
    if (!headers) return "";
-   const ct = headers["Content-Type"] || headers["content-type"] || "";
+   const key = Object.keys(headers).find((k) => k.toLowerCase() === "content-type");
+   const ct = key ? headers[key] : "";
    return ct.toLowerCase();
  }

ui-svelte/src/components/ResizablePanels.svelte (1)

36-86: Document-level event listeners aren't cleaned up if the component unmounts mid-drag.

If the user navigates away while actively dragging, mousemove/mouseup (or touchmove/touchend) listeners remain on document. This is unlikely but could cause errors if handlers reference the destroyed component.

♻️ Add cleanup via onDestroy

- import { onMount } from "svelte";
+ import { onMount, onDestroy } from "svelte";

  // ... after existing onMount ...

+ onDestroy(() => {
+   document.removeEventListener("mousemove", handleMouseMove);
+   document.removeEventListener("mouseup", handleMouseUp);
+   document.removeEventListener("touchmove", handleTouchMove);
+   document.removeEventListener("touchend", handleTouchEnd);
+ });

docker/config.example.yaml (1)

20-21: Proxy field has a default value — explicit specification would be better for consistency.

The proxy field defaults to "http://localhost:${PORT}" when omitted, so z-image will receive routing. However, qwen2.5 and smollm2 explicitly specify proxy: "http://127.0.0.1:9999" to avoid relying on macro substitution. For consistency and clarity, add the same explicit proxy to z-image.

proxy/config/model_config_test.go (1)

76-106: Consider asserting the stop parameter value.

The test verifies that "stop" is present in the sorted keys but doesn't assert its value. Adding an assertion for the stop list would improve coverage of the setParams deserialization path.

💡 Suggested addition

 	assert.Equal(t, 0.7, setParams["temperature"])
 	assert.Equal(t, 0.9, setParams["top_p"])
+	assert.Equal(t, []any{"<|end|>", "<|stop|>"}, setParams["stop"])
 }

proxy/config/peer_test.go (1)

128-139: Replace custom string search with strings.Contains.

These two helper functions replicate strings.Contains from the standard library. Using the stdlib version is simpler and avoids maintaining custom code.

♻️ Proposed fix

Add to imports:

 import (
+	"strings"
 	"testing"
 
 	"gopkg.in/yaml.v3"
 )

Then remove the custom helpers and update usage:

-func contains(s, substr string) bool {
-	return len(s) >= len(substr) && searchSubstring(s, substr)
-}
-
-func searchSubstring(s, substr string) bool {
-	for i := 0; i <= len(s)-len(substr); i++ {
-		if s[i:i+len(substr)] == substr {
-			return true
-		}
-	}
-	return false
-}

And replace contains(...) calls (e.g., line 90) with strings.Contains(...).

ui-svelte/src/components/playground/AudioInterface.svelte (1)

39-53: Duplicated file-validation-and-assignment logic between handleFileSelect and handleDrop.

Both handlers perform the same validate → assign-or-error flow. Extracting a shared helper (e.g., processFile(file: File)) would remove the duplication.

Proposed refactor

+ function processFile(file: File) {
+   const validation = validateFile(file);
+   if (validation.valid) {
+     selectedFile = file;
+     error = null;
+     transcriptionResult = null;
+   } else {
+     error = validation.error || "Invalid file";
+     selectedFile = null;
+   }
+ }
+
  function handleFileSelect(event: Event) {
    const target = event.target as HTMLInputElement;
    const file = target.files?.[0];
-   if (file) {
-     const validation = validateFile(file);
-     if (validation.valid) {
-       selectedFile = file;
-       error = null;
-       transcriptionResult = null;
-     } else {
-       error = validation.error || "Invalid file";
-       selectedFile = null;
-     }
-   }
+   if (file) processFile(file);
  }

Apply the same extraction in handleDrop.

Also applies to: 64-80

ui-svelte/src/routes/Playground.svelte (2)

49-61: Mobile dropdown doesn't close on outside click.

When the dropdown is open, tapping outside of it won't dismiss it — the user must either pick a tab or tap the toggle button again. Consider adding a click-outside handler or an invisible backdrop overlay for better mobile UX.

---

78-92: All four tab panels are always mounted — state is preserved but resources are consumed.

This is a valid design choice for preserving in-progress work across tab switches. Just noting that all components (including their stores, listeners, and potential network connections) remain active in the background. If any of them hold long-lived resources (e.g., SSE connections, timers), ensure they are paused or cleaned up when hidden.

proxy/config/peer.go (1)

12-12: Go naming convention: ApiKey should be APIKey.

Go convention (and staticcheck ST1003) prefers APIKey over ApiKey for initialisms. The YAML tag yaml:"apiKey" would remain unchanged.

proxy/config/filters.go (1)

50-50: Inconsistent use of slices.Sort vs sort.Strings.

Line 50 uses slices.Sort(cleaned) while line 74 uses sort.Strings(keys). Both sort []string but via different packages. Prefer slices.Sort consistently since it's the modern Go approach (1.21+) and slices is already imported.

Proposed fix

-	sort.Strings(keys)
+	slices.Sort(keys)

And remove the "sort" import if no longer needed.

Also applies to: 74-74

proxy/metrics_monitor.go (2)

126-138: captureOrder slice reslicing leaks backing-array memory over time.

mp.captureOrder = mp.captureOrder[1:] advances the start pointer but never releases the underlying array. In a long-running server with continuous capture turnover, the backing array grows unboundedly while only a small window is live.

Consider periodically copying into a fresh slice, or switching to a container/list for O(1) removal without the leak.

♻️ Suggested fix using periodic compaction

 	// Evict oldest (FIFO) until room available
 	for mp.captureSize+captureSize > mp.maxCaptureSize && len(mp.captureOrder) > 0 {
 		oldestID := mp.captureOrder[0]
 		mp.captureOrder = mp.captureOrder[1:]
 		if evicted, exists := mp.captures[oldestID]; exists {
 			mp.captureSize -= evicted.Size()
 			delete(mp.captures, oldestID)
 		}
 	}
+
+	// Compact captureOrder to reclaim backing array memory
+	if cap(mp.captureOrder) > 2*len(mp.captureOrder)+64 {
+		compacted := make([]int, len(mp.captureOrder))
+		copy(compacted, mp.captureOrder)
+		mp.captureOrder = compacted
+	}

---

434-451: decompressBody has no limit on decompressed size.

io.ReadAll on a gzip/deflate reader could expand a small compressed payload into gigabytes (decompression bomb). While the data comes from a trusted backend, consider wrapping the reader with io.LimitReader to bound memory usage defensively.

return io.ReadAll(io.LimitReader(reader, 64<<20)) // e.g. 64 MB cap

proxy/config/model_config.go (1)

191-195: SanitizedStripParams always returns a nil error — consider simplifying the caller.

The wrapper delegates to Filters.SanitizedStripParams() which returns []string only, so the error in the ([]string, error) return is always nil. This means the error branch in proxymanager.go (around line 653) is dead code. If backwards compatibility requires this signature now, a // error is always nil, kept for API compatibility doc comment would help.

ui-svelte/src/components/ModelsPanel.svelte (1)

69-110: Dropdown menu has no click-outside dismiss handler.

When the narrow-mode menu is open, clicking anywhere outside the dropdown doesn't close it — the user must click the hamburger button again. Consider adding a click:outside handler or a backdrop overlay to close the menu.

ui-svelte/src/stores/theme.ts (1)

46-52: Consider debouncing the resize listener.

checkScreenWidth fires on every resize event, which can trigger many rapid store updates during window resizing. A lightweight debounce (e.g., requestAnimationFrame or a small timeout) would reduce unnecessary reactive updates.

♻️ Suggested approach

 export function initScreenWidth(): () => void {
   checkScreenWidth();
-  window.addEventListener("resize", checkScreenWidth);
+  let rafId: number;
+  const onResize = () => {
+    cancelAnimationFrame(rafId);
+    rafId = requestAnimationFrame(checkScreenWidth);
+  };
+  window.addEventListener("resize", onResize);
 
   return () => {
-    window.removeEventListener("resize", checkScreenWidth);
+    cancelAnimationFrame(rafId);
+    window.removeEventListener("resize", onResize);
   };
 }

ui-svelte/src/components/playground/ChatMessage.svelte (2)

267-288: Modal should use role="dialog" instead of role="button".

The full-screen image overlay is semantically a dialog, not a button. Using role="button" with tabindex="-1" misrepresents the element to assistive technologies. Also, the modal won't trap focus, allowing tab navigation to reach elements behind it.

♻️ Suggested fix

   <div
     class="fixed inset-0 z-50 flex items-center justify-center bg-black/80 p-4"
     onclick={(e) => closeModal(e)}
     onkeydown={handleModalKeyDown}
-    role="button"
-    tabindex="-1"
+    role="dialog"
+    aria-modal="true"
+    aria-label="Full-size image"
   >

---

84-96: Body overflow style may leak if component unmounts while modal is open.

openModal sets document.body.style.overflow = "hidden", but if the component is destroyed before closeModal is called, the style won't be reset. Consider using Svelte's onDestroy or an $effect cleanup to restore overflow.

proxy/config/config.go (2)

492-500: No-op assignment in API key validation loop.

Line 499 (config.RequiredAPIKeys[i] = apikey) writes back the exact same value since apikey is never modified in the loop body. Either remove it or add strings.TrimSpace if trimming was intended.

♻️ Option: trim or remove

 	for i, apikey := range config.RequiredAPIKeys {
+		apikey = strings.TrimSpace(apikey)
 		if apikey == "" {
 			return Config{}, fmt.Errorf("empty api key found in apiKeys")
 		}
 		if strings.Contains(apikey, " ") {
 			return Config{}, fmt.Errorf("api key cannot contain spaces: `%s`", apikey)
 		}
 		config.RequiredAPIKeys[i] = apikey
 	}

Or simply remove line 499 if no trimming is needed.

---

818-834: Env value escaping may surprise users in unquoted YAML contexts.

sanitizeEnvValueForYAML escapes \ → \\ and " → \". In double-quoted YAML strings this is correct, but in unquoted or single-quoted contexts these escape sequences appear literally. The inline comment acknowledges this trade-off. Consider documenting this behavior in the configuration docs so users know env values work best inside double-quoted YAML strings.

proxy/peerproxy.go (1)

127-141: model_id parameter uses snake_case instead of Go's camelCase convention.

Go convention uses modelID for parameter names. This is a minor style inconsistency.

♻️ Suggested fix

-func (p *PeerProxy) ProxyRequest(model_id string, writer http.ResponseWriter, request *http.Request) error {
-	pp, found := p.proxyMap[model_id]
+func (p *PeerProxy) ProxyRequest(modelID string, writer http.ResponseWriter, request *http.Request) error {
+	pp, found := p.proxyMap[modelID]
 	if !found {
-		return fmt.Errorf("no peer proxy found for model %s", model_id)
+		return fmt.Errorf("no peer proxy found for model %s", modelID)
 	}

ui-svelte/src/components/playground/ChatInterface.svelte (1)

330-343: Using array index as {#each} key can cause stale DOM nodes after regeneration.

When regenerateFromIndex truncates messages and appends a new assistant message, the same index values map to different message objects. Svelte may reuse DOM nodes for the wrong messages.

Consider assigning a unique ID to each message:

♻️ Suggested approach

Add an id field to messages (e.g., Date.now() or an incrementing counter) and use it as the key:

-        {`#each` messages as message, idx (idx)}
+        {`#each` messages as message, idx (message.id)}

With corresponding changes when creating messages:

let nextId = 0;
// When adding a message:
messages = [...messages, { id: nextId++, role: "user", content }];

ui-svelte/src/lib/types.ts (1)

91-127: ChatMessage.role may be too restrictive — consider including "tool" and "developer".

The OpenAI chat completion API supports additional roles beyond "user" | "assistant" | "system" (e.g., "tool", "function", and the newer "developer" role). If the proxy is intended to transparently forward requests to various backends, restricting the role union here could cause type errors when constructing or parsing messages with these roles.

Proposed fix

 export interface ChatMessage {
-  role: "user" | "assistant" | "system";
+  role: "user" | "assistant" | "system" | "tool" | "developer";
   content: string | ContentPart[];
   reasoning_content?: string;
   reasoningTimeMs?: number;
 }

proxy/proxymanager_test.go (1)

1436-1474: Potential data race on peerCalled — consider using atomic.Bool.

peerCalled is written in the httptest.Server handler goroutine (Line 1440) and read in the test goroutine (Line 1473). While the HTTP round-trip provides implicit synchronization in practice, the Go race detector may flag this since peerCalled is a plain bool without explicit synchronization.

Proposed fix using atomic.Bool

 	t.Run("local models take precedence over peer models", func(t *testing.T) {
 		// Create a test server to act as the peer - should NOT be called
-		peerCalled := false
+		var peerCalled atomic.Bool
 		peerServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			peerCalled = true
+			peerCalled.Store(true)
 			w.Header().Set("Content-Type", "application/json")
 			w.WriteHeader(http.StatusOK)
 			w.Write([]byte(`{"response":"from-peer"}`))
 		}))
 		defer peerServer.Close()

and at the assertion:

-		assert.False(t, peerCalled, "peer should not be called when local model exists")
+		assert.False(t, peerCalled.Load(), "peer should not be called when local model exists")

ui-svelte/src/stores/api.ts (1)

44-52: Clearing all stores on reconnect resets user-visible data.

When onopen fires (Line 46–49), all logs, metrics, and models are wiped. If a transient network blip causes a reconnect, the user loses all accumulated log and metrics history. This may be intentional to keep data in sync with the server's SSE stream, but it's worth calling out since it can be a surprising UX during brief disconnections.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

set -e causes early abort when fetch_llama_tag returns non-zero, bypassing the friendly error message.

With set -euo pipefail (line 3), if fetch_llama_tag returns 1 (tag not found, API error, or pagination exhausted), LCPP_TAG=$(fetch_llama_tag ...) will immediately terminate the script. The graceful error handling at line 117–119 is never reached.

🐛 Proposed fix — suppress errexit for the fetch call

 if [ "$ARCH" == "cpu" ]; then
-    LCPP_TAG=$(fetch_llama_tag "server")
+    LCPP_TAG=$(fetch_llama_tag "server") || true
     BASE_TAG=server-${LCPP_TAG}
 else
-    LCPP_TAG=$(fetch_llama_tag "server-${ARCH}")
+    LCPP_TAG=$(fetch_llama_tag "server-${ARCH}") || true
     BASE_TAG=server-${ARCH}-${LCPP_TAG}
 fi

🤖 Prompt for AI Agents

In `@docker/build-container.sh` around lines 106 - 122, The script currently
aborts early because the global errexit makes the subshell assignment
LCPP_TAG=$(fetch_llama_tag ...) fail the whole script if fetch_llama_tag returns
non-zero; update the LCPP_TAG assignments (the two places calling
fetch_llama_tag) to temporarily disable errexit or otherwise capture the command
failure (e.g., use set +e before the call and restore set -e after, or append ||
true to the command and then check the exit status/empty value), then check
LCPP_TAG for emptiness and run the existing log_info + exit 1 branch; keep
references to fetch_llama_tag and the LCPP_TAG variable so the friendly error
path remains reachable.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Complex/nested values are never actually asserted — the switch only matches primitive types.

The "complex nested values" test case (lines 117-133) expects map[string]any and []string values, but the type switch on line 156 only handles string, int, float64, bool. For keys like "provider" and "transforms", the switch falls through silently, so their values are never compared. The test passes even if the implementation returns wrong nested data.

Proposed fix — use `assert.Equal` directly for all types

-			assert.Equal(t, len(tt.wantParams), len(gotParams), "params length mismatch")
-			for key, wantValue := range tt.wantParams {
-				gotValue, exists := gotParams[key]
-				assert.True(t, exists, "missing key: %s", key)
-				// Simple comparison for basic types
-				switch v := wantValue.(type) {
-				case string, int, float64, bool:
-					assert.Equal(t, v, gotValue, "value mismatch for key %s", key)
-				}
-			}
+			assert.Equal(t, tt.wantParams, gotParams, "params mismatch")

assert.Equal from testify performs deep comparison and handles maps, slices, and nested structures correctly.

🤖 Prompt for AI Agents

In `@proxy/config/filters_test.go` around lines 152 - 160, The test currently only
asserts primitive types inside the loop over tt.wantParams/gotParams using a
type switch, so complex nested values like the "provider" map and "transforms"
slice are never compared; replace the type-switch branch with a single
deep-equality assertion (use assert.Equal) for gotValue vs wantValue for every
key, ensuring nested maps/slices are properly compared while still keeping the
existence check for gotParams[key].

[coderabbitai]

⚠️ Potential issue | 🟠 Major

url.Parse is too lenient — invalid proxy URLs pass validation silently.

url.Parse almost never returns an error. Strings like "not-a-url" or "foobar" parse successfully into a url.URL with no scheme. This means misconfigured proxy values won't be caught at config load time and will fail at runtime with obscure errors.

Add a scheme check after parsing:

Proposed fix

 	parsedURL, err := url.Parse(defaults.Proxy)
 	if err != nil {
 		return fmt.Errorf("invalid peer proxy URL (%s): %w", defaults.Proxy, err)
 	}
+	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
+		return fmt.Errorf("peer proxy URL must have http or https scheme, got: %s", defaults.Proxy)
+	}
 	defaults.ProxyURL = parsedURL

As per coding guidelines, "Run make test-dev when making iterative changes to code under the proxy/ directory - includes go test and staticcheck".

📝 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.

Suggested change

	// Validate proxy is a valid URL and store the parsed value
	parsedURL, err := url.Parse(defaults.Proxy)
	if err != nil {
	return fmt.Errorf("invalid peer proxy URL (%s): %w", defaults.Proxy, err)
	}
	// Validate proxy is a valid URL and store the parsed value
	parsedURL, err := url.Parse(defaults.Proxy)
	if err != nil {
	return fmt.Errorf("invalid peer proxy URL (%s): %w", defaults.Proxy, err)
	}
	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
	return fmt.Errorf("peer proxy URL must have http or https scheme, got: %s", defaults.Proxy)
	}
	defaults.ProxyURL = parsedURL

🤖 Prompt for AI Agents

In `@proxy/config/peer.go` around lines 35 - 39, The parsed URL check using
url.Parse is too permissive: after parsing defaults.Proxy into parsedURL, verify
parsedURL.Scheme is non-empty and one of the expected schemes (e.g., "http" or
"https"); if the scheme is empty or unsupported, return a formatted error like
"invalid peer proxy URL (…): missing or unsupported scheme". Update the block
that parses defaults.Proxy (the parsedURL, err := url.Parse(defaults.Proxy)
section) to perform this scheme validation and return an error when invalid, and
run make test-dev to ensure tests and staticcheck pass.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

API key comparison is vulnerable to timing side-channel attacks.

providedKey == key uses Go's standard string equality which short-circuits on the first differing byte. For secret comparison, use crypto/subtle.ConstantTimeCompare to prevent timing-based key extraction.

🔒 Proposed fix

Add "crypto/subtle" to imports, then:

 		valid := false
 		for _, key := range pm.config.RequiredAPIKeys {
-			if providedKey == key {
+			if subtle.ConstantTimeCompare([]byte(providedKey), []byte(key)) == 1 {
 				valid = true
 				break
 			}
 		}

🤖 Prompt for AI Agents

In `@proxy/proxymanager.go` around lines 956 - 963, The API key equality check in
the proxy manager (the loop over pm.config.RequiredAPIKeys comparing providedKey
to key) is vulnerable to timing attacks; replace the plain string comparison
with a constant-time comparison by importing "crypto/subtle" and using
subtle.ConstantTimeCompare on the byte slices of providedKey and each key from
pm.config.RequiredAPIKeys (e.g., convert both to []byte and treat a result of 1
as a match) to set valid, leaving the rest of the validation logic intact.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd "renderMarkdown" --type f -e js -e ts -e svelte

Repository: napmany/llmsnap

Length of output: 41

---

🏁 Script executed:

rg "renderMarkdown" -t js -t ts -t svelte -l

Repository: napmany/llmsnap

Length of output: 89

---

🏁 Script executed:

rg "export.*renderMarkdown\|function renderMarkdown\|const renderMarkdown" --type ts --type js -A 15

Repository: napmany/llmsnap

Length of output: 41

---

🏁 Script executed:

rg "renderMarkdown" --no-type-list

Repository: napmany/llmsnap

Length of output: 502

---

🏁 Script executed:

find . -name "ChatMessage.svelte" -type f

Repository: napmany/llmsnap

Length of output: 115

---

🏁 Script executed:

find . -type f \( -name "*.js" -o -name "*.ts" \) | xargs grep -l "renderMarkdown" 2>/dev/null

Repository: napmany/llmsnap

Length of output: 127

---

🏁 Script executed:

cat -n ./ui-svelte/src/components/playground/ChatMessage.svelte | head -50

Repository: napmany/llmsnap

Length of output: 2230

---

🏁 Script executed:

cat -n ./ui-svelte/src/lib/markdown.ts

Repository: napmany/llmsnap

Length of output: 3208

---

🏁 Script executed:

cat -n ./ui-svelte/src/lib/markdown.test.ts

Repository: napmany/llmsnap

Length of output: 6710

---

🏁 Script executed:

sed -n '165,175p' ./ui-svelte/src/components/playground/ChatMessage.svelte | cat -n

Repository: napmany/llmsnap

Length of output: 537

---

renderMarkdown does not sanitize HTML—add a sanitization library to prevent XSS.

The markdown rendering pipeline uses allowDangerousHtml: true in both remarkRehype and rehypeStringify, which explicitly allows raw HTML from markdown input to pass through unchanged. When this output is rendered with {@html renderedContent} (lines 27 and 171), any malicious HTML or scripts in assistant responses will execute. In a chat playground where LLM outputs are untrusted, this is a security vulnerability.

Add rehype-sanitize to the pipeline to strip dangerous HTML elements while preserving safe markdown formatting:

.use(rehypeSanitize)

🤖 Prompt for AI Agents

In `@ui-svelte/src/components/playground/ChatMessage.svelte` around lines 25 - 29,
The markdown rendering pipeline in renderMarkdown (used to compute
renderedContent) is currently allowing raw HTML via allowDangerousHtml and
exposes XSS when inserted with {`@html` renderedContent}; update the pipeline to
sanitize output by adding rehype-sanitize (import rehypeSanitize and add
.use(rehypeSanitize) into the remarkRehype/rehype pipeline before
rehypeStringify) so dangerous elements/scripts are stripped while preserving
safe formatting, and ensure renderedContent continues to use the sanitized
result.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Object URL is not revoked on component unmount — memory leak.

URL.createObjectURL allocates a blob URL that persists until explicitly revoked. The code correctly revokes the previous URL before creating a new one (line 145), but if the component is destroyed while generatedAudioUrl is set, the URL is leaked.

Add cleanup on destroy:

Proposed fix (add near top of script)

+ import { onDestroy } from "svelte";
+
+ onDestroy(() => {
+   if (generatedAudioUrl) {
+     URL.revokeObjectURL(generatedAudioUrl);
+   }
+ });

🤖 Prompt for AI Agents

In `@ui-svelte/src/components/playground/SpeechInterface.svelte` around lines 143
- 149, The component leaks blob URLs because generatedAudioUrl is not revoked
when the component is destroyed; add a Svelte onDestroy cleanup that checks
generatedAudioUrl and calls URL.revokeObjectURL(generatedAudioUrl) (and clears
generatedAudioUrl) to free the blob URL; import onDestroy from 'svelte' if not
already present and register the handler near the top of the script so
generatedAudioUrl is always revoked on component unmount.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and read the relevant section
head -100 ui-svelte/src/lib/markdown.ts | tail -50

Repository: napmany/llmsnap

Length of output: 1282

---

🌐 Web query:

rehype-sanitize KaTeX highlight.js compatible configuration schema

💡 Result:

Below are two rehype-sanitize schema patterns that are known to work well with KaTeX and highlight.js-style code blocks.

1) Recommended: sanitize first, then run KaTeX + highlighting

This avoids having to allow KaTeX/HLJS’ large generated HTML/CSS through the sanitizer (rehype-sanitize explicitly recommends this approach for math + highlighting). (github.com)

import rehypeSanitize, { defaultSchema } from "rehype-sanitize";

export const sanitizeForKatexAndCodeBlocks = {
  ...defaultSchema,
  attributes: {
    ...defaultSchema.attributes,

    // Allow language-* (already allowed by defaultSchema as a regex) PLUS
    // KaTeX entrypoint classes used by rehype-katex/rehype-mathjax.
    code: [
      ...(defaultSchema.attributes.code || []),
      ["className", /^language-./, "math-inline", "math-display"],
    ],
  },
};

Pipeline idea:

• rehypeSanitize(sanitizeForKatexAndCodeBlocks)
• rehypeKatex
• rehypeHighlight (or your highlighter)

(Separately, highlight.js-compatible markup is typically <pre><code class="language-...">...</code></pre>.) (highlightjs.org)

2) If you highlight before sanitizing (must allow HLJS token classes)

rehype-sanitize’s docs show that you need to allow hljs on <code> and many hljs-* token classes on <span>. (github.com)

import rehypeSanitize, { defaultSchema } from "rehype-sanitize";

export const sanitizeAfterHighlight = {
  ...defaultSchema,
  attributes: {
    ...defaultSchema.attributes,
    code: [
      ...(defaultSchema.attributes.code || []),
      ["className", "hljs", /^language-./],
    ],
    span: [
      ...(defaultSchema.attributes.span || []),
      // allow the hljs token classes your highlighter emits
      ["className", /^hljs(-.+)?$/],
    ],
  },
};

If you tell me your exact pipeline (rehype-katex vs remark-math+rehype-katex, rehype-highlight vs rehype-pretty-code vs highlight.js output), I can tailor the allowlist to the minimal set of classes you actually need.

Citations:

• 1: https://github.com/rehypejs/rehype-sanitize?utm_source=openai
• 2: https://highlightjs.org/?utm_source=openai
• 3: https://github.com/rehypejs/rehype-sanitize?utm_source=openai

---

🏁 Script executed:

# Search for usage of renderMarkdown to understand data sources
rg -n "renderMarkdown" ui-svelte/src --type ts --type svelte -B 2 -A 2

Repository: napmany/llmsnap

Length of output: 89

---

🏁 Script executed:

# Check for existing sanitization or security measures
rg -n "sanitize|xss|dangeroushtml" ui-svelte/src --type ts --type svelte -i

Repository: napmany/llmsnap

Length of output: 89

---

🏁 Script executed:

# Check if rehype-sanitize is already in dependencies
cat package.json | grep -A 20 -B 5 "rehype\|dependencies"

Repository: napmany/llmsnap

Length of output: 103

---

🏁 Script executed:

# Find package.json
fd -n package.json

Repository: napmany/llmsnap

Length of output: 289

---

🏁 Script executed:

# Search for renderMarkdown calls to understand data sources
rg -n "renderMarkdown" ui-svelte/src --type ts -B 3 -A 3

Repository: napmany/llmsnap

Length of output: 10091

---

🏁 Script executed:

# Check the full markdown.ts file to see imports and complete context
cat ui-svelte/src/lib/markdown.ts

Repository: napmany/llmsnap

Length of output: 2620

---

🏁 Script executed:

# Look for AI integration or user input handling
rg -n "ai|api|response|user.*input" ui-svelte/src --type ts -i -B 2 -A 2

Repository: napmany/llmsnap

Length of output: 21505

---

🏁 Script executed:

# Search for where renderMarkdown is actually called in the UI
rg "renderMarkdown" ui-svelte/src --type ts -B 3 -A 3 | head -60

Repository: napmany/llmsnap

Length of output: 3993

---

🏁 Script executed:

# Look for chat or message rendering components that use this
fd -type f "message\|chat\|response" ui-svelte/src --type ts | head -20

Repository: napmany/llmsnap

Length of output: 229

---

🏁 Script executed:

# Search for components that render markdown or use HTML rendering
rg "@html|innerHTML|renderMarkdown" ui-svelte/src --type ts -l

Repository: napmany/llmsnap

Length of output: 123

---

🏁 Script executed:

# Look for component files that might use markdown rendering
fd -type f \( -name "*.svelte" -o -name "*Message*" -o -name "*Chat*" \) ui-svelte/src | head -20

Repository: napmany/llmsnap

Length of output: 229

---

🏁 Script executed:

# Search across all files for renderMarkdown usage
rg "renderMarkdown" ui-svelte/src

Repository: napmany/llmsnap

Length of output: 2549

---

🏁 Script executed:

# Search for `@html` directives in svelte files
rg "@html" ui-svelte/src

Repository: napmany/llmsnap

Length of output: 147

---

🏁 Script executed:

# List svelte component files
find ui-svelte/src -name "*.svelte" | head -20

Repository: napmany/llmsnap

Length of output: 1019

---

🏁 Script executed:

# Check the ChatMessage.svelte component to see how renderMarkdown is used
cat ui-svelte/src/components/playground/ChatMessage.svelte

Repository: napmany/llmsnap

Length of output: 11805

---

🏁 Script executed:

# Check the ChatInterface.svelte to see the data flow
cat ui-svelte/src/components/playground/ChatInterface.svelte | head -100

Repository: napmany/llmsnap

Length of output: 3449

---

allowDangerousHtml: true creates an XSS vulnerability for AI-generated content.

The renderMarkdown function is used to render assistant responses from the LLM API in ChatMessage.svelte (line 31, rendered via {@html renderedContent}). Since this content originates from an untrusted LLM API, the allowDangerousHtml: true settings on both remarkRehype (line 67) and rehypeStringify (line 70) allow malicious HTML to pass through—for example, <script>alert('xss')</script> or <img onerror=...> injected into model responses.

Add rehype-sanitize to the pipeline after rehypeStringify. Configure the schema to allow KaTeX classes (used by rehypeKatex) and highlight.js token classes (used by the custom rehypeHighlight plugin):

import rehypeSanitize, { defaultSchema } from "rehype-sanitize";

const sanitizeSchema = {
  ...defaultSchema,
  attributes: {
    ...defaultSchema.attributes,
    code: [
      ...(defaultSchema.attributes.code || []),
      ["className", "hljs", /^language-.*/],
    ],
    span: [
      ...(defaultSchema.attributes.span || []),
      ["className", /^hljs(-.+)?$/],
    ],
  },
};

const processor = unified()
  .use(remarkParse)
  .use(remarkGfm)
  .use(remarkMath)
  .use(remarkRehype, { allowDangerousHtml: true })
  .use(rehypeKatex)
  .use(rehypeHighlight)
  .use(rehypeStringify, { allowDangerousHtml: true })
  .use(rehypeSanitize, sanitizeSchema);

🤖 Prompt for AI Agents

In `@ui-svelte/src/lib/markdown.ts` around lines 63 - 70, The pipeline currently
allows dangerous HTML and needs sanitization: import rehypeSanitize and
defaultSchema, build a sanitizeSchema that spreads defaultSchema and extends
attributes for code (allow className, "hljs", and /^language-.*/) and span
(allow className matching /^hljs(-.+)?$/) so KaTeX and highlight.js classes are
preserved, then append .use(rehypeSanitize, sanitizeSchema) to the unified
processor after the existing .use(rehypeStringify, { allowDangerousHtml: true })
call; keep remarkRehype, rehypeKatex, rehypeHighlight, and rehypeStringify in
place but ensure processor uses rehypeSanitize to prevent XSS from LLM content.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

loadingCaptureId is never reset if getCapture rejects.

If the fetch fails, the await on line 50 throws before line 51 executes, leaving the button permanently stuck showing "...".

🐛 Proposed fix using try/finally

   async function viewCapture(id: number) {
     loadingCaptureId = id;
-    const capture = await getCapture(id);
-    loadingCaptureId = null;
-    if (capture) {
-      selectedCapture = capture;
-      dialogOpen = true;
+    try {
+      const capture = await getCapture(id);
+      if (capture) {
+        selectedCapture = capture;
+        dialogOpen = true;
+      }
+    } finally {
+      loadingCaptureId = null;
     }
   }

📝 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.

Suggested change

	async function viewCapture(id: number) {
	loadingCaptureId = id;
	const capture = await getCapture(id);
	loadingCaptureId = null;
	if (capture) {
	selectedCapture = capture;
	dialogOpen = true;
	}
	}
	async function viewCapture(id: number) {
	loadingCaptureId = id;
	try {
	const capture = await getCapture(id);
	if (capture) {
	selectedCapture = capture;
	dialogOpen = true;
	}
	} finally {
	loadingCaptureId = null;
	}
	}

🤖 Prompt for AI Agents

In `@ui-svelte/src/routes/Activity.svelte` around lines 48 - 56, The function
viewCapture currently sets loadingCaptureId before awaiting getCapture but never
clears it if getCapture throws; wrap the await in a try/finally inside
viewCapture so loadingCaptureId is always reset to null in the finally block,
then only set selectedCapture and dialogOpen when capture is truthy (after the
try), ensuring getCapture errors don't leave the UI stuck; reference
viewCapture, loadingCaptureId, getCapture, selectedCapture, and dialogOpen when
making the change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Reconnect timer fires after enableAPIEvents(false), re-opening a closed connection.

When onerror triggers, it schedules setTimeout(connect, delay). If enableAPIEvents(false) is called before that timer fires, apiEventSource is set to null and the connection is torn down — but the pending setTimeout callback still holds a closure over connect and will execute, creating a new EventSource and re-establishing the connection unexpectedly.

Proposed fix: track and clear the reconnect timer

 let apiEventSource: EventSource | null = null;
+let reconnectTimer: ReturnType<typeof setTimeout> | null = null;

 // ...

 export function enableAPIEvents(enabled: boolean): void {
   if (!enabled) {
     apiEventSource?.close();
     apiEventSource = null;
+    if (reconnectTimer) {
+      clearTimeout(reconnectTimer);
+      reconnectTimer = null;
+    }
     metrics.set([]);
     return;
   }

   // ...

     apiEventSource.onerror = () => {
       apiEventSource?.close();
       retryCount++;
       const delay = Math.min(initialDelay * Math.pow(2, retryCount - 1), 5000);
       connectionState.set("disconnected");
-      setTimeout(connect, delay);
+      reconnectTimer = setTimeout(connect, delay);
     };

🤖 Prompt for AI Agents

In `@ui-svelte/src/stores/api.ts` around lines 27 - 102, The reconnect setTimeout
in onerror can fire after enableAPIEvents(false) and resurrect the EventSource;
fix by tracking the reconnect timer ID (e.g., add a module-scoped let
reconnectTimer: ReturnType<typeof setTimeout> | null), replace
setTimeout(connect, delay) with reconnectTimer = setTimeout(connect, delay),
clear and null out that timer when disabling (inside enableAPIEvents when
!enabled) and also clear any existing reconnectTimer before scheduling a new one
in connect/onerror so stale timers never reopen apiEventSource; update connect
to reset reconnectTimer = null on successful onopen and ensure apiEventSource is
only created when enabled.

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@proxy/config/config.go`:
- Around line 793-805: The sanitizeEnvValueForYAML function currently only
escapes backslashes/quotes and rejects newlines/nulls but still allows
YAML-special characters that can truncate or alter unquoted YAML (e.g., '#', ':'
followed by space, '[', ']', '{', '}'); update sanitizeEnvValueForYAML to either
(preferred) reject and return an error when the value contains any of these
YAML-special characters or whitespace-then-# / colon+space patterns (so callers
know to quote) OR (if you control insertion) always return a value wrapped in
double quotes and properly escaped for double-quoted YAML; implement the chosen
approach inside sanitizeEnvValueForYAML (check for '#', ':', '[', ']', '{', '}',
and the pattern ': ' and ' #') and adjust callers to handle the error or to
accept the quoted output accordingly.
- Around line 748-762: The fallback path in substituteEnvMacros currently calls
substituteEnvMacrosInString(s, s) when yaml.Unmarshal fails, which scans the
original input (including YAML comments) and can erroneously treat env macros
inside comments as real; update the fallback to strip YAML comments before
calling substituteEnvMacrosInString so only actual YAML content is scanned. In
practice, modify substituteEnvMacros to, on unmarshal error, produce a
comment-stripped version of s (e.g., remove lines or inline comments starting
with '#' while respecting YAML quoting) and pass that cleaned string to
substituteEnvMacrosInString instead of the raw s; keep the successful path using
yaml.Unmarshal/yaml.Marshal unchanged. Ensure the change is applied inside
substituteEnvMacros and references substituteEnvMacrosInString, yaml.Unmarshal
and yaml.Marshal.
- Around line 463-472: The loop validating config.RequiredAPIKeys contains a
no-op write back; either remove the redundant assignment
config.RequiredAPIKeys[i] = apikey or, if the intent was to trim surrounding
whitespace, replace it by assigning the trimmed value (e.g., use
strings.TrimSpace on apikey) so that the loop both validates and normalizes each
entry in config.RequiredAPIKeys.

🧹 Nitpick comments (2)

proxy/proxymanager_api.go (1)

27-29: Stale comment: references "React" but the UI has been replaced with Svelte.

Suggested fix

-	// Add API endpoints for React to consume
-	// Protected with API key authentication
+	// Add API endpoints for the frontend to consume
+	// Protected with API key authentication

proxy/config/config.go (1)

767-788: Duplicate env var lookups for repeated macro references.

If the same ${env.VAR} appears multiple times in the YAML, FindAllStringSubmatch returns a match for each occurrence, causing redundant os.LookupEnv + sanitizeEnvValueForYAML calls followed by a strings.ReplaceAll that already handles all occurrences in one pass.

♻️ Suggested deduplication

 func substituteEnvMacrosInString(target, scanStr string) (string, error) {
 	result := target
 	matches := envMacroRegex.FindAllStringSubmatch(scanStr, -1)
+	seen := make(map[string]bool)
 	for _, match := range matches {
 		fullMatch := match[0] // ${env.VAR_NAME}
 		varName := match[1]   // VAR_NAME
+		if seen[fullMatch] {
+			continue
+		}
+		seen[fullMatch] = true
 
 		value, exists := os.LookupEnv(varName)

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

No-op assignment on Line 471.

config.RequiredAPIKeys[i] = apikey writes back the same value that was read. If the intent was to trim whitespace (like config.Hooks.OnStartup.Preload does at line 452), the strings.TrimSpace call is missing. Otherwise this line can be removed.

♻️ Suggested fix (if trimming was intended)

 	for i, apikey := range config.RequiredAPIKeys {
+		apikey = strings.TrimSpace(apikey)
 		if apikey == "" {
 			return Config{}, fmt.Errorf("empty api key found in apiKeys")
 		}

📝 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.

Suggested change

	// Validate API keys (env macros already substituted at string level)
	for i, apikey := range config.RequiredAPIKeys {
	if apikey == "" {
	return Config{}, fmt.Errorf("empty api key found in apiKeys")
	}
	if strings.Contains(apikey, " ") {
	return Config{}, fmt.Errorf("api key cannot contain spaces: `%s`", apikey)
	}
	config.RequiredAPIKeys[i] = apikey
	}
	// Validate API keys (env macros already substituted at string level)
	for i, apikey := range config.RequiredAPIKeys {
	apikey = strings.TrimSpace(apikey)
	if apikey == "" {
	return Config{}, fmt.Errorf("empty api key found in apiKeys")
	}
	if strings.Contains(apikey, " ") {
	return Config{}, fmt.Errorf("api key cannot contain spaces: `%s`", apikey)
	}
	config.RequiredAPIKeys[i] = apikey
	}

🤖 Prompt for AI Agents

In `@proxy/config/config.go` around lines 463 - 472, The loop validating
config.RequiredAPIKeys contains a no-op write back; either remove the redundant
assignment config.RequiredAPIKeys[i] = apikey or, if the intent was to trim
surrounding whitespace, replace it by assigning the trimmed value (e.g., use
strings.TrimSpace on apikey) so that the loop both validates and normalizes each
entry in config.RequiredAPIKeys.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Note on the fallback path (lines 752-755).

When initial YAML parse fails, the fallback scans the original string (including comments) for env macros. This means env macros appearing only in YAML comments would trigger "not set" errors in this edge case. This is a minor inconsistency with the happy path but unlikely to be hit in practice since ${env.VAR} is valid unquoted YAML.

🤖 Prompt for AI Agents

In `@proxy/config/config.go` around lines 748 - 762, The fallback path in
substituteEnvMacros currently calls substituteEnvMacrosInString(s, s) when
yaml.Unmarshal fails, which scans the original input (including YAML comments)
and can erroneously treat env macros inside comments as real; update the
fallback to strip YAML comments before calling substituteEnvMacrosInString so
only actual YAML content is scanned. In practice, modify substituteEnvMacros to,
on unmarshal error, produce a comment-stripped version of s (e.g., remove lines
or inline comments starting with '#' while respecting YAML quoting) and pass
that cleaned string to substituteEnvMacrosInString instead of the raw s; keep
the successful path using yaml.Unmarshal/yaml.Marshal unchanged. Ensure the
change is applied inside substituteEnvMacros and references
substituteEnvMacrosInString, yaml.Unmarshal and yaml.Marshal.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Env values with YAML-special characters can corrupt or truncate config values in unquoted contexts.

The sanitization only guards against newlines, null bytes, backslashes, and double quotes. However, many YAML-special characters remain unhandled when the macro is used in an unquoted context:

• # (after space) starts a comment → value truncation (e.g., key: hello #world`` parses as "hello")
• : followed by space can create unexpected mappings
• [, ], {, } can start flow sequences/mappings

Additionally, the backslash escaping (\ → \\) is correct for double-quoted YAML strings but will produce literal \\ in unquoted contexts, corrupting paths on Windows.

Consider either:

1. Wrapping substituted values in double quotes at the YAML level (if you control the insertion point), or
2. Rejecting (or warning about) values containing YAML-special characters like #, :, [, {, etc.

🤖 Prompt for AI Agents

In `@proxy/config/config.go` around lines 793 - 805, The sanitizeEnvValueForYAML
function currently only escapes backslashes/quotes and rejects newlines/nulls
but still allows YAML-special characters that can truncate or alter unquoted
YAML (e.g., '#', ':' followed by space, '[', ']', '{', '}'); update
sanitizeEnvValueForYAML to either (preferred) reject and return an error when
the value contains any of these YAML-special characters or whitespace-then-# /
colon+space patterns (so callers know to quote) OR (if you control insertion)
always return a value wrapped in double quotes and properly escaped for
double-quoted YAML; implement the chosen approach inside sanitizeEnvValueForYAML
(check for '#', ':', '[', ']', '{', '}', and the pattern ': ' and ' #') and
adjust callers to handle the error or to accept the quoted output accordingly.