Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/main/__tests__/llama-error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* code-signing problem for days. This maps the real stderr to a clear reason.
*/
import { describe, it, expect } from 'vitest'
import { classifyLlamaError } from '../llama-error'
import { classifyLlamaError, isContextOverflowError } from '../llama-error'

describe('classifyLlamaError', () => {
it('flags an engine too old for the model architecture (the reported bug)', () => {
Expand Down Expand Up @@ -119,3 +119,26 @@ main: exiting due to model loading error`
expect(classifyLlamaError('')).toBeNull()
})
})

describe('isContextOverflowError', () => {
it('detects the "exceeds the available context size" family (the observed silent-fail cause)', () => {
expect(
isContextOverflowError(
'the request exceeds the available context size. try increasing the context size or enable context shift'
)
).toBe(true)
})

it('detects prompt/input too-long phrasings across engine versions', () => {
expect(isContextOverflowError('input is too large to process')).toBe(true)
expect(isContextOverflowError('prompt is too long for this context')).toBe(true)
expect(isContextOverflowError('the prompt is larger than the context window')).toBe(true)
expect(isContextOverflowError('requested tokens (5000) exceed context window (2048)')).toBe(true)
})

it('is not fooled by an unreachable / dead engine (that must stay retryable)', () => {
expect(isContextOverflowError('fetch failed: ECONNREFUSED 127.0.0.1:8439')).toBe(false)
expect(isContextOverflowError('llama-server is not running')).toBe(false)
expect(isContextOverflowError('')).toBe(false)
})
})
110 changes: 110 additions & 0 deletions src/main/__tests__/llm-lazy-settings-load.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// Regression: LLMService must read its persisted state LAZILY, not in the constructor.
//
// `llm` is a module-level singleton (`export const llm = new LLMService()`), so it is
// constructed while index.ts's IMPORTS are still evaluating — which under ESM finishes
// BEFORE index.ts's own body runs `unifyUserDataPath()` → `app.setPath('userData', …)`.
// Any path resolved during construction therefore points at the PRE-override profile.
//
// Two real consequences, both of which these tests pin:
// 1. Production: the canonical-dir migration ("My Memories" / "my-memories" →
// "Off Grid AI Desktop") has not run yet at construction, so the user's saved
// settings and active model were silently missed and replaced by defaults.
// 2. E2E/harness: an OFFGRID_USER_DATA temp profile was ignored entirely — which is
// what made `settings-sections.spec.ts` "resource mode survives a relaunch" fail.
// A probe confirmed the constructor resolving the REAL profile while
// OFFGRID_USER_DATA pointed at the temp dir.
//
// Writes never had the bug: `persist()` goes through the `settingsFile` getter, which
// resolves late. These tests assert the READ side now behaves the same way, by doing
// what production does — construct FIRST, point the data dir somewhere SECOND.
import { describe, it, expect, beforeEach, afterEach } from 'vitest'
import fs from 'fs'
import os from 'os'
import path from 'path'
import { LLMService } from '../llm'
import { configureRuntime } from '../runtime-env'

let tmp: string

/** Write an llm-settings.json into the models dir of a data dir, as `persist()` would. */
const seedSettings = (dataDir: string, settings: Record<string, unknown>): void => {
const modelsDir = path.join(dataDir, 'models')
fs.mkdirSync(modelsDir, { recursive: true })
fs.writeFileSync(path.join(modelsDir, 'llm-settings.json'), JSON.stringify(settings))
}

beforeEach(() => {
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'offgrid-llm-lazy-'))
})

afterEach(() => {
// Release the override so a later test isn't pinned to a deleted temp dir.
configureRuntime({ dataDir: undefined })
fs.rmSync(tmp, { recursive: true, force: true })
})

describe('LLMService reads persisted settings lazily (not at construction)', () => {
it('picks up a data dir configured AFTER the instance was constructed', () => {
// Construct FIRST — mirrors the module-level singleton being built during imports.
const svc = new LLMService()
// ...then point the runtime at the profile, as index.ts's body does later.
seedSettings(tmp, { performanceMode: 'extreme', temperature: 0.42 })
configureRuntime({ dataDir: tmp })

const s = svc.getSettings()
expect(s.performanceMode).toBe('extreme')
expect(s.temperature).toBe(0.42)
})
Comment on lines +47 to +57

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a lazy active-model regression case.

These tests cover llm-settings.json only. ensureLoaded() also defers active-model.json resolution.

Seed a model file and active-model.json, construct LLMService before configureRuntime(), then assert that activeModelInfo() reads the model from the profile configured at first use.

As per coding guidelines, "Add regression tests in the same change for every behavior change, including bug cases, branches, conditions, error paths, and copy or contract changes."

Proposed regression test
+const seedActiveModel = (dataDir: string, primary: string): void => {
+  const modelsDir = path.join(dataDir, 'models')
+  fs.mkdirSync(modelsDir, { recursive: true })
+  fs.writeFileSync(path.join(modelsDir, primary), '')
+  fs.writeFileSync(
+    path.join(modelsDir, 'active-model.json'),
+    JSON.stringify({ id: 'late-model', primary })
+  )
+}
+
+it('reads the active model from the profile configured at first use', () => {
+  const svc = new LLMService()
+  seedActiveModel(tmp, 'late-model.gguf')
+  configureRuntime({ dataDir: tmp })
+
+  expect(svc.activeModelInfo()).toEqual({ id: 'late-model', vision: false })
+})
📝 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
it('picks up a data dir configured AFTER the instance was constructed', () => {
// Construct FIRST — mirrors the module-level singleton being built during imports.
const svc = new LLMService()
// ...then point the runtime at the profile, as index.ts's body does later.
seedSettings(tmp, { performanceMode: 'extreme', temperature: 0.42 })
configureRuntime({ dataDir: tmp })
const s = svc.getSettings()
expect(s.performanceMode).toBe('extreme')
expect(s.temperature).toBe(0.42)
})
const seedActiveModel = (dataDir: string, primary: string): void => {
const modelsDir = path.join(dataDir, 'models')
fs.mkdirSync(modelsDir, { recursive: true })
fs.writeFileSync(path.join(modelsDir, primary), '')
fs.writeFileSync(
path.join(modelsDir, 'active-model.json'),
JSON.stringify({ id: 'late-model', primary })
)
}
it('reads the active model from the profile configured at first use', () => {
const svc = new LLMService()
seedActiveModel(tmp, 'late-model.gguf')
configureRuntime({ dataDir: tmp })
expect(svc.activeModelInfo()).toEqual({ id: 'late-model', vision: false })
})
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/__tests__/llm-lazy-settings-load.test.ts` around lines 47 - 57, Add
a regression test alongside the existing lazy settings case that seeds a model
file and active-model.json, constructs LLMService before configureRuntime(),
then configures the profile and verifies activeModelInfo() resolves the model
from that profile on first use. Ensure the test specifically covers deferred
active-model resolution through ensureLoaded().

Source: Coding guidelines


it('survives the relaunch shape: persisted mode is read back by a fresh instance', () => {
// What settings-sections.spec.ts "resource mode survives a relaunch" exercises:
// one process writes the mode, the next process constructs and must read it back.
seedSettings(tmp, { performanceMode: 'conservative' })
const relaunched = new LLMService()
configureRuntime({ dataDir: tmp })

expect(relaunched.getSettings().performanceMode).toBe('conservative')
})

it('loads once and does not re-read after the first access', () => {
seedSettings(tmp, { performanceMode: 'conservative' })
const svc = new LLMService()
configureRuntime({ dataDir: tmp })
expect(svc.getSettings().performanceMode).toBe('conservative')

// A later on-disk edit must NOT leak in: the load is once-only, so in-memory state
// stays authoritative until something explicitly persists. This guards against
// turning the lazy guard into a read-on-every-call, which would re-read the file
// on every getSettings and clobber unsaved in-memory changes.
seedSettings(tmp, { performanceMode: 'extreme' })
expect(svc.getSettings().performanceMode).toBe('conservative')
})

it('falls back to defaults when the profile has no settings file', () => {
const svc = new LLMService()
configureRuntime({ dataDir: tmp }) // seeded with nothing
expect(svc.getSettings().performanceMode).toBe('balanced')
})

// The direct guard on the defect, stated behaviourally rather than by spying on fs:
// if construction reads eagerly, it reads the profile configured AT THAT MOMENT.
// Point the runtime at profile A, construct, then switch to profile B before first
// use — a lazy reader returns B, an eager one returns A. This is the exact shape of
// the production bug (construct during imports, real profile chosen afterwards).
it('reads the profile configured at FIRST USE, not the one present at construction', () => {
const other = fs.mkdtempSync(path.join(os.tmpdir(), 'offgrid-llm-lazy-other-'))
try {
seedSettings(other, { performanceMode: 'extreme' }) // profile A
seedSettings(tmp, { performanceMode: 'conservative' }) // profile B

configureRuntime({ dataDir: other }) // A is current...
const svc = new LLMService() // ...at construction
configureRuntime({ dataDir: tmp }) // the override lands afterwards

// Eager construction would have pinned 'extreme' from profile A.
expect(svc.getSettings().performanceMode).toBe('conservative')
} finally {
fs.rmSync(other, { recursive: true, force: true })
}
})
})
20 changes: 20 additions & 0 deletions src/main/llama-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,26 @@ export function modelPortConflictReason(port: number): string {
return `Model engine port ${port} is already owned by another Off Grid AI Desktop instance. Close the other app, development server, or capture run, then restart Chat model in Settings.`
}

/**
* True when a chat/completions failure is the model rejecting a prompt that does not fit the
* running context window (n_ctx). Distinct from a dead/unreachable engine: retrying is useless
* until the context is raised or the prompt shrunk, so callers treat this as TERMINAL rather
* than backing off forever. Pure + Electron-free so it is unit-tested. llama-server phrases this
* a few ways across versions ("the request exceeds the available context size", "input is too
* large", "prompt is too long", "n_ctx" overflow), so match the family, not one string.
*/
export function isContextOverflowError(text: string): boolean {
const s = (text || '').toLowerCase()
if (!s.trim()) return false
return (
/exceed(s|ed)?\s+the\s+(available\s+)?context/.test(s) ||
/context\s+(size|window|length)\s+(exceeded|too\s+small)/.test(s) ||
/(prompt|input)\s+(is\s+)?(too\s+(long|large)|larger\s+than)/.test(s) ||
/(tokens?|prompt)\b.*\bexceed(s|ed)?\b.*\b(n_?ctx|context)/.test(s) ||
/requested\s+tokens.*exceed.*context/.test(s)
)
}

/**
* Classify the most recent llama-server stderr. Returns null if nothing in the
* text looks like a known fatal cause (so callers can fall back to a generic
Expand Down
34 changes: 33 additions & 1 deletion src/main/llm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,30 @@
return path.join(getModelsDir(), 'llm-settings.json')
}

constructor() {
/** Whether the persisted state (active model + user settings) has been read yet. */
private loaded = false

/** Read persisted state ONCE, on first use — never from the constructor.
*
* `llm` is a module-level singleton, so it is constructed while index.ts's IMPORTS
* are still evaluating, which under ESM completes before index.ts's own body runs
* `unifyUserDataPath()` → `app.setPath('userData', …)`. Resolving paths at
* construction therefore reads the PRE-override profile: an OFFGRID_USER_DATA
* harness dir is ignored, and in production the canonical-dir migration ("My
* Memories" / "my-memories" → "Off Grid AI Desktop") has not happened yet, so the
* user's active model and saved settings are silently missed and replaced by
* defaults. Writes never had this bug — `persist()` goes through the settingsFile
* getter, which resolves late. This is exactly the hazard the activeModelFile /
* settingsFile getters were introduced to avoid; calling resolveModel() and reading
* the settings file from the constructor defeated them. */
private ensureLoaded(): void {
if (this.loaded) return
this.loaded = true
this.resolveModel()
this.loadPersistedSettings()
}

private loadPersistedSettings(): void {

Check failure on line 175 in src/main/llm.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=off-grid-ai_off-grid-ai-desktop&issues=AZ_WH2VX5aQ-hGROm4Cn&open=AZ_WH2VX5aQ-hGROm4Cn&pullRequest=77
try {
const s = JSON.parse(fs.readFileSync(this.settingsFile, 'utf-8'))
if (typeof s.temperature === 'number') this.temperature = s.temperature
Expand Down Expand Up @@ -205,6 +227,7 @@
/** The model's trained context window, or null if unknown — exposed so the UI can offer the
* slider up to the model's own maximum instead of a hardcoded cap. */
modelMaxContext(): number | null {
this.ensureLoaded()
return this.trainedContext()
}

Expand Down Expand Up @@ -258,10 +281,12 @@
/** The EFFECTIVE (RAM-clamped) context window the server is actually running
* with — the real ceiling for prompt + tools + answer. */
effectiveContextSize(): number {
this.ensureLoaded()
return this.safeCtxSize(this.ctxSize)
}

getSettings(): LlmSettings {
this.ensureLoaded()
return {
temperature: this.temperature,
ctxSize: this.ctxSize,
Expand Down Expand Up @@ -289,6 +314,7 @@
* `buildLaunchArgs` (single source of truth) after applying the impure RAM clamp,
* so `_doInit` and tests build args the same way. */
launchArgs(): string[] {
this.ensureLoaded()
return this.launchArgsFor(this.safeCtxSize(this.ctxSize), this.gpuLayers)
}

Expand Down Expand Up @@ -352,6 +378,7 @@
/** Update inference settings; respawns the server if any launch-time arg changed
* (context, KV-cache type, flash-attn, GPU layers, threads, batch). */
async setSettings(s: LlmSettings): Promise<void> {
this.ensureLoaded()
// Granular launch-time fields the user sets in THIS patch become pinned: a mode
// preset (now or on a future restart / mode re-pick) must NOT clobber them. Pin
// BEFORE applying the preset so an explicit q8_0 in the same patch survives.
Expand Down Expand Up @@ -453,6 +480,7 @@

/** Switch the active model without terminating a generation already using it. */
reloadModel(): void {
this.ensureLoaded()
if (this.activeGenerations > 0) {
this.modelReloadPending = true
return
Expand Down Expand Up @@ -481,6 +509,7 @@
// on mmproj wrongly kept "Setup Required" up for an activated vision model.)
/** Whether the active chat model can read images (has a vision projector / mmproj). */
hasVision(): boolean {
this.ensureLoaded()
this.resolveModel()
return !!this.mmProjPath && fs.existsSync(this.mmProjPath)
}
Expand All @@ -496,6 +525,7 @@
}

modelsExist(): boolean {
this.ensureLoaded()
this.resolveModel()
return fs.existsSync(this.modelPath)
}
Expand All @@ -510,6 +540,7 @@
* loaded it yet (otherwise an idle/headless gateway reports no chat model).
* Returns null when no model is downloaded. */
activeModelInfo(): { id: string; vision: boolean } | null {
this.ensureLoaded()
this.resolveModel()
if (!fs.existsSync(this.modelPath)) return null
let id = path.basename(this.modelPath)
Expand All @@ -531,6 +562,7 @@
}

async init(): Promise<void> {
this.ensureLoaded()
if (this.paused) {
// A chat/tool turn needs the LLM NOW, but it's paused for a resident image
// server (unified memory can't hold both). Ask the image server to evict
Expand Down
11 changes: 6 additions & 5 deletions src/renderer/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ import type { SearchHit } from './types'
import { loadProFeaturesRenderer } from './bootstrap/loadProFeaturesRenderer'
import { renderProView, type ProViewContext } from './bootstrap/proView'
import { UpgradeScreen } from './components/pro/UpgradeScreen'
import { getProFeature, proFeatureComingSoon } from './components/pro/proCatalog'
import { currentPlatform, isMac } from './lib/device'
import { getProFeature, proFeatureComingSoon, landingView } from './components/pro/proCatalog'
import { currentPlatform } from './lib/device'
import { NotificationProvider } from './hooks/NotificationProvider'
import { useNotifications } from './hooks/useNotifications'
import { ToastProvider } from './hooks/ToastProvider'
Expand Down Expand Up @@ -226,9 +226,10 @@ function AppContent() {
}
}, [])

// Free users land on Models (download a model first, with the sidebar to
// explore); Mac Pro users land on Day. Never land on a locked or unavailable tab.
const [viewMode, setViewMode] = useState<ViewMode>(isPro && isMac() ? 'day' : 'models')
// Where to open: derived from the per-feature capability seam (see landingView), so
// the landing screen can never disagree with nav and gating about whether Day is
// available on this platform.
const [viewMode, setViewMode] = useState<ViewMode>(landingView(currentPlatform(), isPro))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Day landing flashes the upsell

On Windows Pro launches, landingView selects Day before the asynchronous Pro renderer is registered, so the null-view fallback briefly displays the Day upgrade screen to an entitled user; slower activation leaves the misleading upsell visible longer.

Comment on lines +229 to +232

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep root-path routing consistent with landingView.

landingView returns 'models' for free users and unsupported platforms. The mount-only URL effect still maps '/' to 'day' at Line 295. That effect runs after Line 232 and overwrites the selector result.

  • src/renderer/src/App.tsx#L229-L232: route / through landingView(currentPlatform(), isPro) instead of the unconditional day mapping.
  • src/renderer/src/lib/__tests__/proCatalog.lookup.test.ts#L173-L212: add an App-level root-path test for free, Windows Pro, and unsupported-platform users. The current selector-only tests cannot detect this overwrite.

As per coding guidelines, "Define mappings, routing rules, and capability checks once and reuse the single source of truth" and "Add user-behavior integration tests through real product boundaries."

📍 Affects 2 files
  • src/renderer/src/App.tsx#L229-L232 (this comment)
  • src/renderer/src/lib/__tests__/proCatalog.lookup.test.ts#L173-L212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/renderer/src/App.tsx` around lines 229 - 232, Update the root-path
routing effect in App.tsx to use landingView(currentPlatform(), isPro) instead
of unconditionally selecting day, preserving the selector’s capability-based
result for free, Windows Pro, and unsupported-platform users. Add App-level
root-path integration coverage in
src/renderer/src/lib/__tests__/proCatalog.lookup.test.ts lines 173-212 for those
three user scenarios; both sites require changes.

Source: Coding guidelines

const [selectedSessionId, setSelectedSessionId] = useState<string | null>(null)
const [selectedMemoryId, setSelectedMemoryId] = useState<number | null>(null)
// Version of a downloaded-and-staged update (null = none). Surfaced as a banner
Expand Down
11 changes: 8 additions & 3 deletions src/renderer/src/components/Settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,20 @@ export function Settings(): React.ReactElement {
summary="See capture health, recover pending frames, and control model scheduling in one place."
delay={0.14}
>
{CaptureContribution && !(proComingSoon && currentPlatform() !== 'darwin') ? (
{/* Capture runs wherever Pro is active (macOS + Windows), so render the real
registered section on every platform - the engine, its status, and the
Proactive-delivery toggle it hosts are all ported. The placeholder is only for
the free build, where pro never registers a contribution. Previously this was
gated to darwin, which stranded Windows Pro users with no capture controls and no
way to see the frame/observation health even though capture was running. */}
{CaptureContribution ? (
<CaptureContribution />
) : (
<div className="mb-5 border border-neutral-800 bg-neutral-950/40 p-3 text-xs text-neutral-500">
<span className="mr-2 text-[10px] uppercase tracking-wide text-emerald-500">
Pro
</span>
Screen capture, backlog recovery, and proactive delivery are available with Pro on
macOS.
Screen capture, backlog recovery, and proactive delivery are part of Pro.
</div>
)}
<ProcessingControls />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@
).toBeNull()
})

it('Windows Pro build withholds native capture while keeping account sections available', async () => {
it('Windows Pro build renders the registered capture section (capture is ported to Windows)', async () => {
// Capture, Day, Reflect and Proactive delivery run on Windows Pro now, so the Settings
// Capture section must render its registered owner on win32 exactly like macOS - not fall
// back to the "Pro on macOS" placeholder. Guards the fix for the gate that stayed Mac-only
// after the feature nav was ported, hiding the frame/observation health panel on Windows.
vi.resetModules()
stubApi('win32')
const { registerSettingsSection } = await import('../../bootstrap/sectionRegistry')
Expand All @@ -102,10 +106,9 @@

await waitFor(() => expect(screen.getByTestId('fake-identity')).toBeTruthy())
await user.click(screen.getByText('Capture & processing'))
expect(screen.queryByTestId('fake-capture')).toBeNull()
expect(
screen.getByText(/screen capture, backlog recovery, and proactive delivery/i)
).toBeTruthy()
await waitFor(() => expect(screen.getByTestId('fake-capture')).toBeTruthy())

Check warning on line 109 in src/renderer/src/components/__tests__/Settings.pro-sections.test.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer `findByTestId` query over using `waitFor` + `getByTestId`

See more on https://sonarcloud.io/project/issues?id=off-grid-ai_off-grid-ai-desktop&issues=AZ_WH2d95aQ-hGROm4Co&open=AZ_WH2d95aQ-hGROm4Co&pullRequest=77
// The free-build "part of Pro" placeholder must NOT show for an entitled Windows user.
expect(screen.queryByText(/screen capture, backlog recovery, and proactive delivery/i)).toBeNull()
expect(screen.getByText('Processing priority')).toBeTruthy()
})
})
Loading
Loading