feat(#187 AC#2): embedded LLM model downloader#247
Merged
Conversation
The launch-gating piece: a fresh install can now fetch its own model with no config, no API keys, no per-message cost. Settings → Local AI brain card detects RAM bracket via navigator.deviceMemory, recommends the highest-quality model that fits, click Download → 3-15 minutes later the twin works fully offline. Substrate (#246) gave us the catalog. This PR makes downloading happen. Ships: - 039 migration: model_downloads (status state machine, paused_at, completed_at, sha256_expected, target_path) - modelDownloadRepository: create, findActive (idempotency on user+model), updateProgress, setStatus, recoverOrphanedDownloads - apps/api/src/embedded-llm/downloader.ts: streams fetch() body to <target_path>.partial, atomic rename on success; HTTP Range for resume (graceful fallback when server ignores Range); SHA-256 verify post-download (skipped when registry hash is placeholder all-zeros); in-flight AbortController map for pause/cancel; default dir ~/.skytwin/models/llama matches runtime detector's read path. - /api/embedded-llm/downloads/{start,:id,user/:userId,pause,resume,cancel} endpoints; boot-time recovery wired into apps/api/src/index.ts. - apps/web/public/js/components/embedded-llm-card.js: detects bracket, recommends default, override select, progress bar (reuses .confidence-bar), pause/resume/cancel, polls every 1s during active. 14 new route tests; api total 477 passing. All 34 packages build clean. Out of scope: real SHA-256 hashes in registry (currently placeholder), first-run dashboard integration, paused-on-restart proactive toast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements AC#2 of #187 by adding persistent, resumable embedded-model downloads: a DB-backed model_downloads table + repository, API endpoints to start/pause/resume/cancel and query progress, and a Settings “Local AI brain” UI card that recommends a model by RAM bracket and drives the download flow.
Changes:
- Added
model_downloadsschema + repository utilities, including orphan recovery on API boot. - Added embedded-LLM download engine (HTTP Range resume, atomic rename, optional SHA-256 verification) and REST endpoints for download lifecycle management.
- Added Settings UI card + web API client helpers for selecting a model and tracking download progress via polling.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/src/schemas/schema.sql | Adds model_downloads table + indexes to canonical schema snapshot. |
| packages/db/src/migrations/039-model-downloads.sql | Introduces migration creating model_downloads table and indexes. |
| packages/db/src/repositories/model-download-repository.ts | New repository for creating/updating download rows and boot-time orphan recovery. |
| packages/db/src/repositories/index.ts | Exposes the new repository/types from the repositories barrel. |
| packages/db/src/index.ts | Re-exports repository/types from the package entrypoint. |
| apps/api/src/embedded-llm/downloader.ts | New downloader implementation (resume, pause/cancel, verification, install). |
| apps/api/src/routes/embedded-llm.ts | Adds download lifecycle endpoints and JSON shaping for polling UI. |
| apps/api/src/index.ts | Wires boot-time orphan recovery for downloads during API startup. |
| apps/api/src/tests/embedded-llm-downloads-routes.test.ts | Adds route-layer tests for download endpoints and JSON shape. |
| apps/web/public/js/api-client.js | Adds client helpers for registry/model-dir/download endpoints. |
| apps/web/public/js/components/embedded-llm-card.js | New Settings card UI for model selection and download management. |
| apps/web/public/js/pages/settings.js | Mounts the embedded-LLM card on the Settings page. |
| CHANGELOG.md | Documents the embedded downloader feature in the unreleased changelog. |
Comments suppressed due to low confidence (1)
CHANGELOG.md:97
- There are multiple blank lines and a stray sentence ("Closes the a11y commitments...") that is no longer under any section header, which breaks the changelog structure and makes it unclear which release entry it belongs to. Either move this line under the appropriate
## [unreleased]header or remove it along with the extra blank lines.
Resume button on next Settings visit, which is the desired UX. A
toast / banner alerting them proactively is a small follow-up.
Closes the a11y commitments of #194 Child 4. Detailed entry in PR #244.
## [unreleased] — Crisis modes: recovery codes + vacation mode (#194 Child 3 partial)
Closes 2 of 4 sub-features in #194 Child 3: recovery codes + vacation mode. Detailed entry in PR #245.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+79
to
+90
| const userId = body['userId']; | ||
| const modelId = body['modelId']; | ||
| if (typeof userId !== 'string' || userId.length === 0) { | ||
| res.status(400).json({ error: 'userId required' }); | ||
| return; | ||
| } | ||
| if (typeof modelId !== 'string' || modelId.length === 0) { | ||
| res.status(400).json({ error: 'modelId required' }); | ||
| return; | ||
| } | ||
| try { | ||
| const result = await startDownload(userId, modelId); |
| const { id } = req.params; | ||
| if (!id) { res.status(400).json({ error: 'id required' }); return; } | ||
| const row = await modelDownloadRepository.findById(id); | ||
| if (!row) { res.status(404).json({ error: 'download not found' }); return; } |
Comment on lines
+131
to
+167
| router.post('/downloads/:id/pause', async (req, res, next) => { | ||
| try { | ||
| const { id } = req.params; | ||
| if (!id) { res.status(400).json({ error: 'id required' }); return; } | ||
| const ok = await pauseDownload(id); | ||
| res.json({ ok }); | ||
| } catch (err) { | ||
| next(err); | ||
| } | ||
| }); | ||
|
|
||
| router.post('/downloads/:id/resume', async (req, res, next) => { | ||
| try { | ||
| const { id } = req.params; | ||
| if (!id) { res.status(400).json({ error: 'id required' }); return; } | ||
| const row = await modelDownloadRepository.findById(id); | ||
| if (!row) { res.status(404).json({ error: 'download not found' }); return; } | ||
| // Resume re-runs `startDownload` for this row's (userId, modelId). | ||
| // The function's idempotent-on-active-row behavior picks up the | ||
| // existing row and continues from `bytes_downloaded`. | ||
| const result = await startDownload(row.user_id, row.model_id); | ||
| res.json({ download: rowToJson(result.download), resumed: result.resumed }); | ||
| } catch (err) { | ||
| next(err); | ||
| } | ||
| }); | ||
|
|
||
| router.post('/downloads/:id/cancel', async (req, res, next) => { | ||
| try { | ||
| const { id } = req.params; | ||
| if (!id) { res.status(400).json({ error: 'id required' }); return; } | ||
| const ok = await cancelDownload(id); | ||
| res.json({ ok }); | ||
| } catch (err) { | ||
| next(err); | ||
| } | ||
| }); |
Comment on lines
+109
to
+116
| // Kick off the async transfer. Caller doesn't await this — it polls | ||
| // the row for status. Errors land in `error` + status='failed'. | ||
| void runDownload(download).catch((err) => { | ||
| log.warn('download runner threw', { | ||
| downloadId: download.id, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| }); |
Comment on lines
+321
to
+327
| const expectedHash = model.sha256.toLowerCase(); | ||
| const isPlaceholder = /^0+$/.test(expectedHash); | ||
| if (!isPlaceholder) { | ||
| const fs = await import('node:fs/promises'); | ||
| const buf = await fs.readFile(partialPath); | ||
| const actual = createHash('sha256').update(buf).digest('hex'); | ||
| if (actual !== expectedHash) { |
Comment on lines
+227
to
+230
| labelLine.firstElementChild?.replaceWith?.(document.createTextNode('')); | ||
| // Simpler: replace the size+percent text node only | ||
| const sizeSpan = labelLine.querySelector('span'); | ||
| if (sizeSpan) { |
Comment on lines
+248
to
+273
| let _listenerWired = false; | ||
| function ensureListener(container, userId) { | ||
| if (_listenerWired) return; | ||
| _listenerWired = true; | ||
|
|
||
| document.addEventListener('click', async (e) => { | ||
| const hash = (window.location.hash || '').split('?')[0]; | ||
| if (hash !== '#/settings') return; | ||
| const target = e.target instanceof Element ? e.target : null; | ||
| if (!target) return; | ||
| const el = target.closest('[data-action]'); | ||
| if (!el) return; | ||
| const action = el.getAttribute('data-action'); | ||
|
|
||
| if (action === 'embedded-start-download') { | ||
| const sel = container.querySelector('#embedded-model-select'); | ||
| const modelId = sel instanceof HTMLSelectElement ? sel.value : ''; | ||
| if (!modelId) return; | ||
| try { | ||
| await startModelDownload(userId, modelId); | ||
| showSavedToast('Download started'); | ||
| await renderCardInto(container, userId); | ||
| } catch (err) { | ||
| showErrorToast(`Couldn't start download: ${err?.friendlyMessage || err?.message || 'unknown error'}`); | ||
| } | ||
| return; |
| CREATE INDEX IF NOT EXISTS model_downloads_user_active_idx | ||
| ON model_downloads (user_id, started_at DESC) | ||
| WHERE status NOT IN ('complete', 'failed', 'cancelled'); | ||
|
|
Comment on lines
+55
to
+64
| function buildApp(userId: string | null = USER_ID): Express { | ||
| const app = express(); | ||
| app.use(express.json()); | ||
| if (userId !== null) { | ||
| app.use((req, _res, next) => { | ||
| (req as unknown as { user: { id: string } }).user = { id: userId }; | ||
| next(); | ||
| }); | ||
| } | ||
| app.use('/api/embedded-llm', createEmbeddedLlmRouter()); |
Comment on lines
+338
to
+342
| CREATE INDEX IF NOT EXISTS model_downloads_user_active_idx | ||
| ON model_downloads (user_id, started_at DESC) | ||
| WHERE status NOT IN ('complete', 'failed', 'cancelled'); | ||
| CREATE INDEX IF NOT EXISTS model_downloads_user_all_idx | ||
| ON model_downloads (user_id, started_at DESC); |
6 tasks
12 substantive findings from Copilot's review of PR #247, all fixed: Security (3 IDOR vulns): - POST /downloads/start: requireOwnership middleware (body.userId must match req.authenticatedUserId) - GET /downloads/:id and pause/resume/cancel: loadOwnedDownload() helper fetches row, rejects 403 if row.user_id ≠ authenticated user Correctness: - SHA-256 verify: stream via createReadStream so multi-GB GGUFs don't OOM the API process (was readFile() into memory) - Schema: UNIQUE partial index on (user_id, model_id) for non-terminal statuses; startDownload() catches 23505 race - inFlight guard in startDownload() prevents two concurrent runners writing same .partial when caller rapidly retries - Persist bytes_downloaded=0 immediately when server ignores Range - Persist total_bytes when Content-Length differs from registry approxBytes (added updateTotalBytes repo method) Frontend: - Stable data-role selectors (embedded-llm-progress-text / -status) replace the brittle "labelLine.firstElementChild" walk that was clobbering action buttons after first poll - ensureListener() no longer closes over container/userId from first mount; re-derives both at click time via getElementById(CARD_TARGET_ID) + getCurrentUserId() Tests: - Switched from injecting req.user.id to req.authenticatedUserId to match production sessionAuth contract - 7 new cross-user 403 tests covering every mutating endpoint + dev-bypass-allowed path. Total: 25 tests (was 14), all passing. Build clean. 486 API tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Pushed 255d446 addressing all 12 Copilot findings. Summary by category: Security — 3 IDOR vulns (the most important)
Correctness
Frontend
Tests
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
CHANGELOG.md:150
- There’s a stray sentence (“Closes the a11y commitments of #194 Child 4…”) that is no longer under its own
## [unreleased]header, so the changelog reads like it’s part of the embedded-LLM section. Either reintroduce the a11y header or remove/move this line so each unreleased entry is properly scoped.
Closes the a11y commitments of #194 Child 4. Detailed entry in PR #244.
## [unreleased] — Crisis modes: recovery codes + vacation mode (#194 Child 3 partial)
Closes 2 of 4 sub-features in #194 Child 3: recovery codes + vacation mode. Detailed entry in PR #245.
Comment on lines
+339
to
+343
| inFlight.delete(download.id); | ||
| await modelDownloadRepository.setStatus(download.id, 'verifying', { | ||
| bytesDownloaded: totalBytes, | ||
| }); | ||
|
|
Comment on lines
+221
to
+229
| let resumeFrom = 0; | ||
| if (existsSync(partialPath)) { | ||
| try { | ||
| resumeFrom = statSync(partialPath).size; | ||
| } catch { | ||
| resumeFrom = 0; | ||
| } | ||
| } | ||
|
|
Comment on lines
+96
to
+108
| const isActive = ACTIVE_STATUSES.has(status); | ||
| const isPaused = status === 'paused'; | ||
| const isError = status === 'failed'; | ||
|
|
||
| const actionButtons = (() => { | ||
| if (isActive) { | ||
| return `<button class="btn btn-outline btn-sm" data-action="embedded-pause-download" data-download-id="${escapeHtml(download.id)}">Pause</button> | ||
| <button class="btn btn-outline btn-sm" data-action="embedded-cancel-download" data-download-id="${escapeHtml(download.id)}">Cancel</button>`; | ||
| } | ||
| if (isPaused) { | ||
| return `<button class="btn btn-primary btn-sm" data-action="embedded-resume-download" data-download-id="${escapeHtml(download.id)}">Resume</button> | ||
| <button class="btn btn-outline btn-sm" data-action="embedded-cancel-download" data-download-id="${escapeHtml(download.id)}">Cancel</button>`; | ||
| } |
Comment on lines
+407
to
+415
| * Helper used by the dirname-only check in tests. Exported so the | ||
| * route handler doesn't need to import `node:path` separately. | ||
| */ | ||
| export function ensureDirectory(filePath: string): void { | ||
| const dir = dirname(filePath); | ||
| if (!existsSync(dir)) mkdirSync(dir, { recursive: true }); | ||
| } | ||
|
|
||
| /** |
…code Four findings from Copilot's re-review of the security/correctness fixes: - **Cancel couldn't stop verify/install**: the runner removed the inFlight handle BEFORE verification, so cancelDownload() couldn't abort SHA-256 or rename. After verify completed, the runner would overwrite the user's cancelled status with complete/failed. Now the handle stays in inFlight through verify+install, and the runner checks handle.cancelled at every phase boundary (pre-verify, post-hash, pre-install, post-rename) and bails without writing terminal status if cancel fired. inFlight.delete moved to the final boundary. - **Stale .partial corrupting fresh resume**: a previous attempt that failed to delete .partial (e.g., Windows file lock during cancel) could cause a brand-new download row to "resume" from stale bytes. Now we only resume when bytes_downloaded > 0 on the row; otherwise we delete any pre-existing partial before starting fresh. - **UI showed Pause for verifying/installing but backend rejected it**: pauseDownload() returns ok:false for those statuses. Added PAUSABLE_STATUSES set (pending + downloading only); the action bar shows Cancel-only during verify/install. Cancel now actually works through those phases (per the runner change above). - **ensureDirectory() dead code**: exported but unused. Removed, along with the now-unused `dirname` import. Build clean. 25 route tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Pushed ccb2708 addressing all 4 findings from your re-review: Cancel couldn't stop verify/install (downloader.ts:343) — fixed. The runner was deleting the The handle now stays in Stale if (download.bytes_downloaded > 0 && existsSync(partialPath)) {
resumeFrom = statSync(partialPath).size;
} else if (existsSync(partialPath)) {
unlinkSync(partialPath); // wipe stale bytes from a prior failed cancel
}A fresh row ( UI showed Pause for verifying/installing (embedded-llm-card.js:108) — fixed. Added
Build clean. 25 route tests still passing. |
Comment on lines
+39
to
+44
| * Compute the absolute target path for a registry model. The basename | ||
| * is `<modelId>.gguf`. We never use the registry's URL filename — that | ||
| * could be anything (or could change over time without our control). | ||
| */ | ||
| export function targetPathFor(modelId: string): string { | ||
| return join(resolveModelDir(), `${modelId}.gguf`); |
Comment on lines
+160
to
+167
| if (row.status !== 'downloading' && row.status !== 'pending') return false; | ||
|
|
||
| const handle = inFlight.get(downloadId); | ||
| if (handle) { | ||
| handle.paused = true; | ||
| handle.controller.abort(); | ||
| inFlight.delete(downloadId); | ||
| } |
Comment on lines
+211
to
+244
| async function runDownload(download: ModelDownloadRow): Promise<void> { | ||
| const model = findModelById(download.model_id); | ||
| if (!model) { | ||
| await modelDownloadRepository.setStatus(download.id, 'failed', { | ||
| error: `unknown model id: ${download.model_id}`, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const partialPath = partialPathFor(download.target_path); | ||
| // Only resume from the partial if THIS row had progress recorded. | ||
| // A fresh row (bytes_downloaded === 0) finding a partial on disk | ||
| // means the previous attempt didn't clean up — e.g., a cancel that | ||
| // failed to unlink on Windows. Resuming from those stale bytes | ||
| // would corrupt the download. | ||
| let resumeFrom = 0; | ||
| if (download.bytes_downloaded > 0 && existsSync(partialPath)) { | ||
| try { | ||
| resumeFrom = statSync(partialPath).size; | ||
| } catch { | ||
| resumeFrom = 0; | ||
| } | ||
| } else if (existsSync(partialPath)) { | ||
| try { unlinkSync(partialPath); } catch { /* best effort */ } | ||
| } | ||
|
|
||
| const controller = new AbortController(); | ||
| const handle: InFlightDownload = { controller, paused: false, cancelled: false }; | ||
| inFlight.set(download.id, handle); | ||
|
|
||
| await modelDownloadRepository.setStatus(download.id, 'downloading', { | ||
| bytesDownloaded: resumeFrom, | ||
| }); | ||
|
|
Comment on lines
+225
to
+256
| function startPolling(container, userId, downloadId) { | ||
| if (_activeDownloadId === downloadId && _pollTimer !== null) return; | ||
| stopPolling(); | ||
| _activeDownloadId = downloadId; | ||
| _pollTimer = setInterval(async () => { | ||
| try { | ||
| const data = await fetchModelDownload(downloadId); | ||
| const dl = data?.download; | ||
| if (!dl || !ACTIVE_STATUSES.has(dl.status)) { | ||
| // Status changed — re-render entire card (transitions to | ||
| // verifying / installing / complete / failed). | ||
| await renderCardInto(container, userId); | ||
| return; | ||
| } | ||
| // Active mid-download: just update the visible progress without | ||
| // re-fetching the registry. Targeted updates via data-role | ||
| // selectors so the action-button span isn't disturbed. | ||
| const fillEl = container.querySelector('.confidence-fill'); | ||
| if (fillEl instanceof HTMLElement) { | ||
| fillEl.style.width = `${dl.percent}%`; | ||
| } | ||
| const bar = container.querySelector('[role="progressbar"]'); | ||
| if (bar) bar.setAttribute('aria-valuenow', String(dl.percent)); | ||
| const progressText = container.querySelector('[data-role="embedded-llm-progress-text"]'); | ||
| if (progressText) { | ||
| progressText.textContent = `${formatBytes(dl.bytesDownloaded)} / ${formatBytes(dl.totalBytes)} · ${dl.percent}%`; | ||
| } | ||
| } catch { | ||
| // Best-effort poll. The next render will pick up state. | ||
| } | ||
| }, POLL_INTERVAL_MS); | ||
| } |
5 tasks
jayzalowitz
added a commit
that referenced
this pull request
May 9, 2026
…ration Three findings from Copilot's review of PR #249: - **Install destroys good shared file on rename failure**: the previous "unlink target → renameSync" sequence had a window where, if rename threw or got cancelled mid-flight, the host was left with no GGUF at all — even though a perfectly good copy had been deleted moments before. Since the final path is now content-addressable (SHA-256 verified before this point), an existing target IS the model. New install path: if target exists → just discard our partial, treat as installed; otherwise renameSync directly without unlinking; if the rename fails AND target now exists (race lost to another row), treat as installed. - **Legacy non-namespaced partials orphaned by migration**: PR #247 shipped with `<target>.partial`; PR #249 namespaces by row id. Existing pre-#249 partials would sit on disk indefinitely. Added unconditional cleanup at the top of runDownload — if a legacy `<target>.partial` exists, delete it before the row's namespaced partial is created. - **Stale comment in cancelDownload**: docstring still mentioned `<target_path>.partial`. Updated to the namespaced format. Build clean. 25 route tests still passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jayzalowitz
added a commit
that referenced
this pull request
May 9, 2026
… polling Copilot's third-round review of PR #247 landed post-merge. Four substantive findings, all addressed: - **Multi-user partial-file collision**: two users on the same API host downloading the same model both wrote to <modelDir>/<modelId>.gguf.partial — concurrent streams could corrupt each other and one user's cancel could delete the other's partial. Final GGUF stays shared at <modelDir>/<modelId>.gguf (content-addressable, SHA-256 verified before rename), but partials now namespace by download row id. - **Pause-on-pending was silently overridden**: pauseDownload() set DB to 'paused', but the already-kicked-off runDownload() would start anyway and overwrite back to 'downloading'. Same path for pending→cancelled. Fixed by re-fetching the row at the top of runDownload() and bailing early if status flipped to paused / cancelled / complete / failed between startDownload returning and the async runner picking up. - **Polling continued after navigating away from Settings**: the 1s poll callback in embedded-llm-card.js had no termination tied to page navigation. Going to Approvals while a download was in flight kept hitting /api/embedded-llm/downloads/:id every second forever and held a reference to a detached #embedded-llm-card-target node. Now checks window.location.hash !== '#/settings' and document.getElementById(CARD_TARGET_ID) !== container at the top of each tick and stops itself. - **ensureDirectory dead code**: already removed in the round-2 fix. Build clean. 25 route tests still passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jayzalowitz
added a commit
that referenced
this pull request
May 9, 2026
…ration Three findings from Copilot's review of PR #249: - **Install destroys good shared file on rename failure**: the previous "unlink target → renameSync" sequence had a window where, if rename threw or got cancelled mid-flight, the host was left with no GGUF at all — even though a perfectly good copy had been deleted moments before. Since the final path is now content-addressable (SHA-256 verified before this point), an existing target IS the model. New install path: if target exists → just discard our partial, treat as installed; otherwise renameSync directly without unlinking; if the rename fails AND target now exists (race lost to another row), treat as installed. - **Legacy non-namespaced partials orphaned by migration**: PR #247 shipped with `<target>.partial`; PR #249 namespaces by row id. Existing pre-#249 partials would sit on disk indefinitely. Added unconditional cleanup at the top of runDownload — if a legacy `<target>.partial` exists, delete it before the row's namespaced partial is created. - **Stale comment in cancelDownload**: docstring still mentioned `<target_path>.partial`. Updated to the namespaced format. Build clean. 25 route tests still passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jayzalowitz
added a commit
that referenced
this pull request
May 11, 2026
… polling (#249) * fix(#187 AC#2 r3): multi-user partial collision + pause race + zombie polling Copilot's third-round review of PR #247 landed post-merge. Four substantive findings, all addressed: - **Multi-user partial-file collision**: two users on the same API host downloading the same model both wrote to <modelDir>/<modelId>.gguf.partial — concurrent streams could corrupt each other and one user's cancel could delete the other's partial. Final GGUF stays shared at <modelDir>/<modelId>.gguf (content-addressable, SHA-256 verified before rename), but partials now namespace by download row id. - **Pause-on-pending was silently overridden**: pauseDownload() set DB to 'paused', but the already-kicked-off runDownload() would start anyway and overwrite back to 'downloading'. Same path for pending→cancelled. Fixed by re-fetching the row at the top of runDownload() and bailing early if status flipped to paused / cancelled / complete / failed between startDownload returning and the async runner picking up. - **Polling continued after navigating away from Settings**: the 1s poll callback in embedded-llm-card.js had no termination tied to page navigation. Going to Approvals while a download was in flight kept hitting /api/embedded-llm/downloads/:id every second forever and held a reference to a detached #embedded-llm-card-target node. Now checks window.location.hash !== '#/settings' and document.getElementById(CARD_TARGET_ID) !== container at the top of each tick and stops itself. - **ensureDirectory dead code**: already removed in the round-2 fix. Build clean. 25 route tests still passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(#187 AC#2 r3 follow-up): install idempotency + legacy partial migration Three findings from Copilot's review of PR #249: - **Install destroys good shared file on rename failure**: the previous "unlink target → renameSync" sequence had a window where, if rename threw or got cancelled mid-flight, the host was left with no GGUF at all — even though a perfectly good copy had been deleted moments before. Since the final path is now content-addressable (SHA-256 verified before this point), an existing target IS the model. New install path: if target exists → just discard our partial, treat as installed; otherwise renameSync directly without unlinking; if the rename fails AND target now exists (race lost to another row), treat as installed. - **Legacy non-namespaced partials orphaned by migration**: PR #247 shipped with `<target>.partial`; PR #249 namespaces by row id. Existing pre-#249 partials would sit on disk indefinitely. Added unconditional cleanup at the top of runDownload — if a legacy `<target>.partial` exists, delete it before the row's namespaced partial is created. - **Stale comment in cancelDownload**: docstring still mentioned `<target_path>.partial`. Updated to the namespaced format. Build clean. 25 route tests still passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The launch-gating piece. A non-technical user opening SkyTwin previously hit a wall: configure llama.cpp + GGUF + env vars, or buy API keys. With this PR: open Settings → "Local AI brain" card detects RAM bracket via
navigator.deviceMemory, recommends the highest-quality model that fits, click Download → 3-15 minutes later the twin works fully offline. No API keys, no per-message cost, no manual install.The registry shipped in #246 told us which model to download. This PR makes it actually happen — with pause / resume / cancel and persistence across API restarts.
What ships
039-model-downloads.sqlmigration +modelDownloadRepositoryapps/api/src/embedded-llm/downloader.ts— fetch + Range + atomic rename + SHA-256 verify + AbortController-based pause/cancel + boot-time orphan recovery/api/embedded-llm/downloads/{start,:id,user/:userId,pause,resume,cancel}endpointsapps/web/public/js/components/embedded-llm-card.js— Settings card, polls every 1s during active download, reuses.confidence-barstylesWhy this fits the theme
Pure boring-deterministic infrastructure:
fetch()+Range, SHA-256, atomic rename. No LLM in the cryptography or the download path. Every byte accounted for; every state transition reflected in the row; every button has a deterministic backend consequence.Closes for #187
5 of 8 ACs closed. Remaining are distribution (AC#1 bundling), missing binary (AC#4), or small UI follow-ups.
Test plan
pnpm buildclean across all 34 packagespnpm --filter @skytwin/api test— 477 passingvi.mockensures tests never touch real HTTP / filesystemOut of scope
🤖 Generated with Claude Code